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

[Florian Fainelli <f.fainelli@gmail.com>]

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