Re: [PATCH 3/3] pwm: imx: Implement get_state() function for hardware readout

[VokÃÄ Michal]

On 13.12.2018 21:14, Uwe Kleine-KÃnig wrote:> On Thu, Dec 13, 2018 at 06:00:00PM +0100, Thierry Reding wrote:
>> On Thu, Dec 13, 2018 at 09:52:13AM +0100, Uwe Kleine-KÃnig wrote:
>>> On Wed, Dec 12, 2018 at 11:54:32AM +0100, Thierry Reding wrote:
>>>> On Mon, Oct 01, 2018 at 04:19:48PM +0200, Michal VokÃÄ wrote:
>>>>> Implement the get_state() function and set the initial state to reflect
>>>>> real state of the hardware. This allows to keep the PWM running if it was
>>>>> enabled in bootloader. It is very similar to the GPIO behavior. GPIO pin
>>>>> set as output in bootloader keep the same setting in Linux unless it is
>>>>> reconfigured.
>>>>>
>>>>> If we find the PWM block enabled we need to prepare and enable its source
>>>>> clock otherwise the clock will be disabled late in the boot as unused.
>>>>> That will leave the PWM in enabled state but with disabled clock. That has
>>>>> a side effect that the PWM output is left at its current level at which
>>>>> the clock was disabled. It is totally non-deterministic and it may be LOW
>>>>> or HIGH.
>>>>>
>>>>> Signed-off-by: Michal VokÃÄ <michal.vokac@xxxxxxxxx>
>>>>> ---
>>>>>   drivers/pwm/pwm-imx.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>   1 file changed, 53 insertions(+)
>>>>
>>>> Applied, thanks.
>>>
>>> Did you miss my feedback for this patch or did you choose to ignore it?
>>
>> Don't have anything in my inbox, but I see that it's there on patchwork,
>> so I suspect it was eaten by the spam filter.
> 
> :-|
> 
>> Let me copy-paste here and go through it.
>>
>>>> @@ -93,6 +96,55 @@ struct imx_chip {
>>>>
>>>>   #define to_imx_chip(chip) container_of(chip, struct imx_chip, chip)
>>>>
>>>> +static void imx_pwm_get_state(struct pwm_chip *chip,
>>>> +  struct pwm_device *pwm, struct pwm_state *state)
>>>
>>> broken alignment.
>>
>> I can fix that up, no need to resend for that.
> 
> fine.
> 
>>>> +{
>>>> + struct imx_chip *imx = to_imx_chip(chip);
>>>> + u32 period, prescaler, pwm_clk, ret, val;
>>>> + u64 tmp;
>>>> +
>>>> + val = readl(imx->mmio_base + MX3_PWMCR);
>>>> +
>>>> + if (val & MX3_PWMCR_EN) {
>>>> +  state->enabled = true;
>>>> +  ret = clk_prepare_enable(imx->clk_per);
>>>> +  if (ret)
>>>> +   return;
>>>> + } else {
>>>> +  state->enabled = false;
>>>> + }
>>>> +
>>>> + switch (FIELD_GET(MX3_PWMCR_POUTC, val)) {
>>>> + case MX3_PWMCR_POUTC_NORMAL:
>>>> +  state->polarity = PWM_POLARITY_NORMAL;
>>>> +  break;
>>>> + case MX3_PWMCR_POUTC_INVERTED:
>>>> +  state->polarity = PWM_POLARITY_INVERSED;
>>>> +  break;
>>>> + default:
>>>> +  dev_warn(chip->dev, "can't set polarity, output disconnected");
>>>
>>> Should we return an error here?
>>
>> We can't return an error here because the function has a void return
>> type. I'm not sure what it means if the POUTC is neither "normal" nor
>> "inverted", but perhaps a good idea would be to default to either of
>> those two in that case, or perhaps forcibly disable the PWM so that
>> we get known-good values in the registers?
> 
> The manual says "PWM output is disconnected". So I think the right thing
> to do is to consider the pwm as disabled. (But checking how the output
> actually behaves would be great.) (Maybe this setting is even the
> solution to our "Make the output high-Z on disable" discussion?)

I checked the behavior. It can be used to disconnect the output even
if the PWM is enabled. It does not solve the problem with the LOW
output level in disabled state. This gave me the same results.
When PWM is enabled and the output is disconnected with POUTC = 2,
the output level is always LOW and the DSE, ODE, and PUS pad control
settings have no effect. I suppose the main purpose of this is to
allow to quickly enable/disable the output without the need to enable/
disable the whole PWM block. So we can not really say the PWM is
enabled/disabled here as there is another (EN) bit in the PWMCR
register that controls that.

>> I'm tempted not to treat this as a blocker because it's not actually
>> a bug or anything. Prior to this patch we also ignore whatever this
>> field was set to.
> 
> Prior to this patch this field was never read as this is only needed in
> .get_state. And in .apply the value is correctly written.
> 
>>>> + }
>>>> +
>>>> + prescaler = MX3_PWMCR_PRESCALER_GET(val);
>>>> + pwm_clk = clk_get_rate(imx->clk_per);
>>>> + pwm_clk = DIV_ROUND_CLOSEST_ULL(pwm_clk, prescaler);
>>>> + val = readl(imx->mmio_base + MX3_PWMPR);
>>>
>>> It would be more cautionous to not rely on the reserved bits to read as
>>> zero. So I suggest to mask the value with 0xffff.
>>
>> If that's what the documentation says I think it's okay to rely on it.
>> But I can add the mask if we want to play it extra safe.
> 
> The PERIOD field only covers bits 0 to 15 and the upper 16 bits of this
> register are reserved. The registers are documented to read as 0, still
> I'd prefer to mask them out for maximal correctness.

OK.

>>>> + period = val >= MX3_PWMPR_MAX ? MX3_PWMPR_MAX : val;
>>>> +
>>>> + /* PWMOUT (Hz) = PWMCLK / (PWMPR + 2) */
>>>> + tmp = NSEC_PER_SEC * (u64)(period + 2);
>>>> + state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);
>>>
>>> Would it make sense to introduce a policy about how to round in this
>>> case? (Similarily for .apply?) This is of course out of scope for this
>>> patch.
>>
>> I'm not sure what you means by policy. The above already does round to
>> closest. Is that not an appropriate policy?
> 
> I mean to document the expectation of the pwm framework how the driver
> is supposed to configure the hardware when it should program the PWM
> with say duty_cycle=100ns/period=300ns and but the hardware can only
> implement both duty_cycle and period in steps of 3ns. So the (IMHO) good
> candidates are:
> 
> 	duty_cycle = 99ns / period = 297ns
> 	duty_cycle = 99ns / period = 300ns
> 	duty_cycle = 102ns / period = 300ns
> 	duty_cycle = 102ns / period = 306ns
> 
> As it's not obvious which is the best here (or is it?) and taking into
> account that the PWM user might care, it would be right to specify
> something like:
> 
>    The driver is supposed to configure the biggest duty_cycle that
>    is not greater than the requested duty_cycle. Similarily it should
>    pick the biggest possible period that is not greater than the
>    requested period.
> 
> To actually allow the user to find the setting that he or she prefers
> something like the clk framework provides with clk_round_rate() would be
> needed of course.
> 
> And for .get_state I'd specify something similar that ensures that in
> the sequence
> 
> 	myops->get_state(chip, pwm, state);
> 	myops->apply(chip, pwm, state);
> 
> the latter is a noop.

Sounds good. Would be great to find something like this in the docs
while I was implementing the .get_state.

If this is out of scope for this patch then there are only two minor
issues to fix - the incorrect indentation and the mask for the period
register, right?

So given the fact that Thierry already applied the series I can send
a separate patch with these fixes, OK? Or you will fix it up all
yourself, Thierry?

Best regards,
Michal