[PATCH v12 2/3] pwm: Add Allwinner's D1/T113-S3/R329 SoCs PWM support

[Uwe Kleine-König]

Hello Andre,

On Wed, May 28, 2025 at 01:29:02PM +0100, Andre Przywara wrote:
> On Wed, 28 May 2025 13:08:40 +0200
> Uwe Kleine-König <ukleinek at kernel.org> wrote:
> > On Sat, May 24, 2025 at 12:07:28PM +0300, Александр Шубин wrote:
> > > вт, 13 мая 2025 г. в 01:39, Andre Przywara <andre.przywara at arm.com>:  
> > > >
> > > > On Sun, 27 Apr 2025 17:24:54 +0300
> > > > Aleksandr Shubin <privatesub2 at gmail.com> wrote:  
> > > > > +              */
> > > > > +             use_bus_clk = false;
> > > > > +             val = mul_u64_u64_div_u64(state->period, hosc_rate, NSEC_PER_SEC);
> > > > > +             /*
> > > > > +              * If the calculated value is ≤ 1, the period is too short
> > > > > +              * for proper PWM operation
> > > > > +              */
> > > > > +             if (val <= 1) {  
> > > >
> > > > So if I get the code correctly, it prefers HOSC over APB? Is that
> > > > really the best way? Shouldn't it be the other way around: we use the
> > > > faster clock, since this will not limit the sibling channel?
> > > >
> > > > And another thing to consider are rounding errors due to integer
> > > > division: certain period rates might be better achievable with one or
> > > > the other source clock: 3 MHz works best as 24MHz/8, 3.125MHz as
> > > > 100MHz/32.
> > > > So shall we calculate the values and compare the errors instead?
> > > > Oh, and also we need to consider bypassing, I feel like this should be
> > > > checked first.
> > > >
> > > > In any case I think there should be a comment describing the strategy
> > > > and give some rationale, I think.  
> > > 
> > > I like the idea of comparing the quantization error for each clock source
> > > (i.e. computing the actual period for both APB and HOSC and choosing
> > > whichever is closer to the requested period).
> > > I can try to implement that error-minimization approach in the next
> > > series of patches and add a comment explaining the strategy.  
> > 
> > Consumers have different needs. Some might prefer a better match for
> > period, but in my experience most would go for a fine-grained selection
> > of duty_cycle, so prefering the faster clock sounds sane.
> > 
> > I don't say minimizing the error is wrong, but if it's unclear that
> > this matches what a consumer wants I object to make the procedure to
> > select the hardware settings considerably more complicated and run-time
> > intensive.
> 
> Yes, I agree. There seems to be another use case here, which is to provide
> clocks on output pins. The PWM IP has a bypass switch (per channel, after
> the divider), and this feature is already required to supply the
> "internal" (co-packaged) Ethernet PHY on the Allwinner H616 with its clock.
> With the two possible input clocks and those pre-dividers there is actually
> quite a number of possible frequencies to deliver on output pins.
> 
> Since we need some algorithm to decide when we need to use the bypass
> mode, should we check for that if the duty cycle is 50%, to see if we can
> reach the frequency with just the pre-dividers?
> Chances are we need this anyway, since for instance the 24MHz required for
> the PHY cannot be achieved otherwise.

And the clk output is the output after the predividers I assume? I would
prefer to make the driver create a clk_provider instead of guessing
which requests are supposed to have a meaning for the clk output.

> > > > > +static int sun20i_pwm_probe(struct platform_device *pdev)
> > > > > +{
> > > > > +     struct pwm_chip *chip;
> > > > > +     struct sun20i_pwm_chip *sun20i_chip;
> > > > > +     struct clk *clk_bus;
> > > > > +     struct reset_control *rst;
> > > > > +     u32 npwm;
> > > > > +     int ret;
> > > > > +
> > > > > +     ret = of_property_read_u32(pdev->dev.of_node, "allwinner,npwms", &npwm);
> > > > > +     if (ret < 0)
> > > > > +             npwm = 8; /* Default value */
> > > > > +
> > > > > +     if (npwm > 16) {
> > > > > +             dev_info(&pdev->dev, "Limiting number of PWM lines from %u to 16", npwm);  
> > > >
> > > > I don't think we should proceed if the firmware information is clearly
> > > > wrong. Just bail out with -EINVAL or so here, so that gets fixed in the
> > > > DT.  
> > 
> > To me it's not obvious that the "firmware information is clearly wrong".
> > Maybe the next Allwinner SoC will have 24 outputs and the problem is
> > only that this driver isn't prepared to cope for that number of outputs?
> 
> But then it would be an error, regardless?
> The MMIO register frame of this IP here has a hard limit on 16 channels,
> both by the bit assignments in each register (2 bits per channel in a
> 32-bit register), but also by the layout of the registers (max 8
> registers, each for a pair of 2 PWM channels). So anything with more than
> 16 channels cannot be compatible with what this driver supports.
> So as this driver here stands right now, more than 16 channels is an
> error, simple as that. If we extend the driver later on, to cover more
> advanced IP, we would naturally amend this check, of course.

Agreed. In that case I don't care much if .probe() fails or just limits
the number of PWMs to 16.

Best regards
Uwe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20250618/d79e7f8a/attachment.sig>