Message-ID: <hetih6ul7hdj3kflhy2s2zkkh3r7pcupgwde3xnwmjzs6cujp3@vcw4pde76bdb>
Date: Wed, 28 May 2025 13:08:40 +0200
From: Uwe Kleine-König <ukleinek@...nel.org>
To: Александр Шубин <privatesub2@...il.com>
Cc: Andre Przywara <andre.przywara@....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

Hello,

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.

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

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().

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

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

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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists