Message-ID: <20250528132902.70f634cd@donnerap.manchester.arm.com>
Date: Wed, 28 May 2025 13:29:02 +0100
From: Andre Przywara <andre.przywara@....com>
To: Uwe Kleine-König <ukleinek@...nel.org>
Cc: Александр Шубин
 <privatesub2@...il.com>, linux-kernel@...r.kernel.org, Brandon Cheo Fusi
 <fusibrandon13@...il.com>, Rob Herring <robh@...nel.org>, Krzysztof
 Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Chen-Yu
 Tsai <wens@...e.org>, Jernej Skrabec <jernej.skrabec@...il.com>, Samuel
 Holland <samuel@...lland.org>, Paul Walmsley <paul.walmsley@...ive.com>,
 Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>,
 Alexandre Ghiti <alex@...ti.fr>, Philipp Zabel <p.zabel@...gutronix.de>,
 linux-pwm@...r.kernel.org, devicetree@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
 linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v12 2/3] pwm: Add Allwinner's D1/T113-S3/R329 SoCs PWM
 support

On Wed, 28 May 2025 13:08:40 +0200
Uwe Kleine-König <ukleinek@...nel.org> wrote:

Hi Uwe,

> On Sat, May 24, 2025 at 12:07:28PM +0300, Александр Шубин wrote:
> > вт, 13 мая 2025 г. в 01:39, Andre Przywara <andre.przywara@....com>:  
> > >
> > > On Sun, 27 Apr 2025 17:24:54 +0300
> > > Aleksandr Shubin <privatesub2@...il.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.

> > > > +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.

> If that really happens it's arguable if it's better to refuse completely
> or just cope for the 16 outputs that the driver is able to. IMHO it's
> better to continue because a partially workable pwmchip is better than
> no chip at all. But I'd upgrade the message to dev_warn().

I don't understand why we should continue. If the firmware information
(DT) is wrong, we should make this clear, to force people to fix
that, instead of somehow papering over it.

But it's really an academic discussion, I don't expect anyone to put more
than 16 channels in the DT. It's in the per-SoC .dtsi anyway, so nothing
that board DT authors would touch.

> > > > +             npwm = 16;
> > > > +     }
> > > > +
> > > > +     chip = devm_pwmchip_alloc(&pdev->dev, npwm, sizeof(*sun20i_chip));
> > > > +     if (IS_ERR(chip))
> > > > +             return PTR_ERR(chip);
> > > > +     sun20i_chip = to_sun20i_pwm_chip(chip);
> > > > +
> > > > +     sun20i_chip->base = devm_platform_ioremap_resource(pdev, 0);
> > > > +     if (IS_ERR(sun20i_chip->base))
> > > > +             return PTR_ERR(sun20i_chip->base);
> > > > +
> > > > +     clk_bus = devm_clk_get_enabled(&pdev->dev, "bus");
> > > > +     if (IS_ERR(clk_bus))
> > > > +             return dev_err_probe(&pdev->dev, PTR_ERR(clk_bus),
> > > > +                                  "Failed to get bus clock\n");
> > > > +
> > > > +     sun20i_chip->clk_hosc = devm_clk_get_enabled(&pdev->dev, "hosc");
> > > > +     if (IS_ERR(sun20i_chip->clk_hosc))
> > > > +             return dev_err_probe(&pdev->dev, PTR_ERR(sun20i_chip->clk_hosc),
> > > > +                                  "Failed to get hosc clock\n");
> > > > +
> > > > +     ret = devm_clk_rate_exclusive_get(&pdev->dev, sun20i_chip->clk_hosc);  
> > >
> > > Just ignoring for a bit that the 24 MHz oscillator is a fixed clock
> > > anyway, but why would we want exclusivity already at probe time? Isn't
> > > that too limiting, as no one might ever use any PWM channels, but it
> > > would still "belong to us"?  
> 
> That's a soft concept of "belong to us". Other consumers can still use
> it and even also call clk_rate_exclusive_get(). IMHO it's a good idea to
> call clk_rate_exclusive_get() for each clock that a driver relies on not
> to change. You could make the driver more flexible and only call that
> when the rate is actually relied on, but that's again a compromise with
> complexity of the driver. And if the clock rate is fixed anyhow, it
> doesn't hurt to do it here, right?

Sure, just wanted to point that out. Indeed we don't need to boil the
ocean here.

> > > > +     if (ret)
> > > > +             return dev_err_probe(&pdev->dev, ret,
> > > > +                                  "Failed to get hosc exclusive rate\n");
> > > > +
> > > > +     sun20i_chip->clk_apb = devm_clk_get_enabled(&pdev->dev, "apb");
> > > > +     if (IS_ERR(sun20i_chip->clk_apb))
> > > > +             return dev_err_probe(&pdev->dev, PTR_ERR(sun20i_chip->clk_apb),
> > > > +                                  "Failed to get apb clock\n");
> > > > +
> > > > +     ret = devm_clk_rate_exclusive_get(&pdev->dev, sun20i_chip->clk_apb);  
> > >
> > > Just for the records: APB is practically also a fixed clock, set up
> > > once in firmware and never changed, since it drives a lot of other
> > > peripherals.
> > > But same question as above, why do we lock its rate already here?  
> > 
> > That step was actually recommended by Uwe Kleine-König,
> > so the decision on whether to keep or drop exclusive reservation
> > is really a question for him—please coordinate with Uwe
> > to agree on how best to proceed here.  
> 
> Same as above. Iff the driver relies on the rate of this clock to keep
> constant, calling clk_rate_exclusive_get() is right.

It technically doesn't until a channel is actually programmed, but fair
enough, it doesn't matter anyway.

Cheers,
Andre

> > > > +     if (ret)
> > > > +             return dev_err_probe(&pdev->dev, ret,
> > > > +                                  "Failed to get apb exclusive rate\n");
> > > > +
> > > > +     if (clk_get_rate(sun20i_chip->clk_apb) <= clk_get_rate(sun20i_chip->clk_hosc))
> > > > +             dev_info(&pdev->dev, "APB clock must be greater than hosc clock");  
> > >
> > > Why this check? Does the code make any assumptions about this relation?
> > > If yes, we must surely deny this and bail out.
> > > If not (and I feel we should handle it this way), we can just ignore
> > > this and not print anything.  
> 
> ack.
> 
> Best regards
> Uwe

Powered by blists - more mailing lists