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 [off-list ref] wrote:

quoted

On Sat, May 24, 2025 at 12:07:28PM +0300, Александр Шубин wrote:

quoted

вт, 13 мая 2025 г. в 01:39, Andre Przywara [off-list ref]:  

quoted

On Sun, 27 Apr 2025 17:24:54 +0300
Aleksandr Shubin [off-list ref] wrote:  

quoted

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

quoted

quoted

quoted

quoted

+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