[PATCH 0/2] pwm: brcmstb: Some cleanups

[Uwe Kleine-König]

Hello Florian,

great to get answers from you in such a timely fashion. That helps a
lot!

On Mon, Mar 07, 2022 at 11:11:05AM -0800, Florian Fainelli wrote:
> On 3/7/22 10:47 AM, Uwe Kleine-König wrote:
> > I have a few questions here looking in more detail into the brcmstb
> > driver:
> > 
> >  - What happens on PWM_ON(channel) = 0?
> >    I guess it just emits a flat inactive line, and refusing a small
> >    duty_cycle that results in PWM_ON(channel) = 0 is just artificial?
> > 
> >  - There is a line describing:
> > 
> >    	W = cword, if cword < 2 ^ 15 else 16-bit 2's complement of cword
> > 
> >    The driver only considers powers of two <= 2^15 for cword. Is the
> >    implementation just lazy, or is the comment misleading?
> >    At least s/</<=/ ?
> >    There is no sense in using a value > 2^15 as for each such value
> >    there is a smaller value with the same result, right?
> 
> This was copied from the specification which now that you mention it,
> seems off by one in its formula, it should be <=. This is further
> confirmed with:
> 
> pwm1_cword[15:0] must be less than or equal to 32768 when the
> variable-frequency PWM is used as a clock for the constant-frequency PWM.
> Reset value is 0x0.
> 
> so I believe that the comment is wrong and so is the specification of
> the block that was used to write the driver.

OK, so the right thing would be:

	W = cword with cword <= 32768

and there is no limitation to powers of two. (However it's unclear to me
how this works in hardware for arbitrary values.)

> >  - clk_get_rate(p->clk) is expected to return 27 MHz, right?
> >    (Just for my understanding, not about to hardcode this in the code)
> 
> That is right.

ok.

> >  - The explanation about period in the comment is:
> > 
> >    	The period is: (period + 1) / Fv
> > 
> >    so I would have expected:
> > 
> >    	pc = (period_ns * clkrate * cword / (NSEC_PER_SEC << 16)) - 1
> > 
> >    (assuming no overflows). However the -1 isn't in the code.

You didn't comment on that one, I still assume this to be correct, i.e.
the -1 must be coped for in the code.

> >  - Duty-cycle calculation is unclear, the docs say:
> > 
> >  	"on" time is on / (period + 1)
> > 
> >    I suspect on time is $on / Fv though?
> 
> Yes, that is also what the specification says, not sure why I wrote that
> down TBH.

OK. on / (period + 1) would be the relative duty cycle then.

> >    But even with that I don't understand the +1 in
> > 
> >  	dc = mul_u64_u64_div_u64(duty_ns + 1, rate, NSEC_PER_SEC);
> > 
> > Can you enlighten me?
> 
> I wish, this was 7 years ago, and I do not remember why there was a +1
> being added here, it might have been that this should have been a
> DIV_ROUND_UP().

The most usual thing to do is to round down in .apply().

To sum up: The hardware works in quantums Q = 2^16 / (W * 27 MHz).
(This is nice for W = 2^n: Q = 2^(16 - n) / (27 MHz))

The period length is 

	(PWM_PERIOD(channel) + 1) * Q

and duty_cycle is defined by
 
	PWM_ON(channel) * Q

. (No +1 there?)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
-------------- 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/20220307/45974ece/attachment.sig>