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

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

[-- Attachment #1.1: Type: text/plain, Size: 5045 bytes --]

On Mon, Mar 07, 2022 at 02:27:22PM -0800, Florian Fainelli wrote:
> On 3/7/22 12:44 PM, Uwe Kleine-König wrote:
> > 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?)
> 
> Yes, I think what you are saying here is correct and matches what is
> being described in the specification:
> 
> In the case where cword < 2^15, a period in the output waveform consists
> of a single 1/27 microsecond HIGH pulse followed by LOW pulse for the
> rest of the period. In the case where cword ≥ 2^15, a period in the
> output waveform consists of a single 1/27 microsecond LOW pulse followed
> by HIGH pulse for the rest of the period. In a sequence of pulse cycles,
> one cycle can have a duty cycle and period that is different from those
> of the other cycles. In order to have every cycle have exactly
> the same duty cycle and period, cword must be chosen such that
> cword=2^i,, i=0..15.

I don't understand the part "a single 1/27 microsecond", does that
assume PWM_ON(channel) == 0 (or 1?). Otherwise bigger cword values seem
to enable inversed polarity, which might be a nice future expansion.

So non-power of two result in variing parameters such that in average
the period is (PWM_PERIOD(channel) + 1) * Q. The relative duty cycle is
right however. So for example if W = 0xc then every second period has
length 2^16 / (8 * 27 MHz) and the others have 2^16 / (16 * 27 MHz),
right? Well, probably they don't alternate, but we see a bunch of W=16
period by (the same ammount of) periods with W=8.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel