On 22/05/2020 13:12, Sandipan Patra wrote:
...
>>>>> /*
>>>>> * Compute the prescaler value for which (1 << PWM_DUTY_WIDTH)
>>>>> * cycles at the PWM clock rate will take period_ns nanoseconds.
>>>>> */
>>>>> - rate = pc->clk_rate >> PWM_DUTY_WIDTH;
>>>>> + if (pc->soc->num_channels == 1) {
>>>>
>>>> Are you using num_channels to determine if Tegra uses the BPMP? If so
>>>> then the above is not really correct, because num_channels is not
>>>> really related to what is being done here. So maybe you need a new SoC
>> attribute in the soc data.
>>>
>>> Here, it tries to find if pwm controller uses multiple channels (like
>>> in Tegra210 or older) or single channel for every pwm instance (i.e.
>>> T186, T194). If found multiple channels on a single controller then it
>>> is not correct to configure separate clock rates to each of the channels. So to
>> distinguish the controller and channel type, num_channels is referred.
>>
>> OK, then that makes sense. Maybe add this detail to the comment about why
>> num_channels is used.
>
> Ok. Will update comment.
>
>>
>>>>
>>>>> + /*
>>>>> + * Rate is multiplied with 2^PWM_DUTY_WIDTH so that it
>>>> matches
>>>>> + * with the hieghest applicable rate that the controller can
>>>>
>>>> s/hieghest/highest/
>>>
>>> Got it.
>>>
>>>>
>>>>> + * provide. Any further lower value can be derived by setting
>>>>> + * PFM bits[0:12].
>>>>> + * Higher mark is taken since BPMP has round-up mechanism
>>>>> + * implemented.
>>>>> + */
>>>>> + required_clk_rate =
>>>>> + (NSEC_PER_SEC / period_ns) << PWM_DUTY_WIDTH;
>>>>> +
>>>>
>>>> Should be we checking the rate against the max rate supported?
>>>
>>> If the request rate is beyond max supported rate, then the
>>> clk_set_rate will be failing and can get caught with error check
>>> followed by. Otherwise it will fail through fitting in the register's frequency
>> divider filed. So I think it is not required to check against max rate.
>>> Please advise if I am not able to follow with what you are suggesting.
>>
>> I think that it would be better to update the cached value so that it is not
>> incorrectly used else where by any future change. Furthermore, this simplifies
>> matters a bit because you can do the following for all devices, but only update
>> the clk_rate for those you wish to ...
>>
>> rate = pc->clk_rate >> PWM_DUTY_WIDTH;
>>
> What I understood from above is, we will always use max rate for any further configurations.
> If this is the suggestion above, then I think its not the right way.
I am not saying that.
> If we consider only max rate then the pwm output can only be ranging from:
> Possible max output rate: rate
> Possible min output rate: rate/2^13 (13 bits frequency divisor)
>
> But if we consider the min rate supported by the source clock then,
> min output rate can go beyond the current min possible and
> that should be considered for finding actual limit of min output rate.
>
> Based on this, in the driver it tries to find a suitable clock rate to achieve
> requested output rate.
> Please suggest if you think we can still improve this further.
What I am suggesting is you ...
if (pc->soc->num_channels == 1) {
required_clk_rate = (NSEC_PER_SEC / period_ns) <<
PWM_DUTY_WIDTH;
err = clk_set_rate(pc->clk, required_clk_rate);
if (err < 0)
return -EINVAL;
pc->clk_rate = clk_get_rate(pc->clk);
}
rate = clk_get_rate(pc->clk) >> PWM_DUTY_WIDTH;
That's all. I think this is simpler.
Jon
--
nvpublic
