On 2017-01-03 04:46, Boris Brezillon wrote:
<snip>
>> > Well, regarding the imx_pwm_apply_v2() suggested by Stefan, I think we
>> > both agreed that most of the code was unneeded when all we want to do
>> > is disable the PWM.
>>
>> So for the PATCH 7/11 we fix the issue with recalculating clocks
>> when we want to disable PWM.
>>
>> if (state->enabled) {
>> c = clk_get_rate(imx->clk_per);
>> c *= state->period;
>>
>> do_div(c, 1000000000);
>> period_cycles = c;
>>
>> prescale = period_cycles / 0x10000 + 1;
>>
>> period_cycles /= prescale;
>> c = (unsigned long long)period_cycles *
>> state->duty_cycle;
>> do_div(c, state->period);
>> duty_cycles = c;
>>
>> /*
>> * According to imx pwm RM, the real period value
>> * should be PERIOD value in PWMPR plus 2.
>> */
>> if (period_cycles > 2)
>> period_cycles -= 2;
>> else
>> period_cycles = 0;
>>
>> /*
>> * Enable the clock if the PWM is not already
>> * enabled.
>> */
>> if (!cstate.enabled) {
>> ret = clk_prepare_enable(imx->clk_per);
>> if (ret)
>> return ret;
>> }
>>
>> /*
>> * Wait for a free FIFO slot if the PWM is already
>> * enabled, and flush the FIFO if the PWM was disabled
>> * and is about to be enabled.
>> */
>> if (cstate.enabled)
>> imx_pwm_wait_fifo_slot(chip, pwm);
>> else
>> imx_pwm_sw_reset(chip);
>>
>> writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
>> writel(period_cycles, imx->mmio_base + MX3_PWMPR);
>>
>> writel(MX3_PWMCR_PRESCALER(prescale) |
>> MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
>> MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH |
>> MX3_PWMCR_EN,
>> imx->mmio_base + MX3_PWMCR);
>> } else {
>>
>> writel(0, imx->mmio_base + MX3_PWMCR);
>>
>> /* Disable the clock if the PWM is currently enabled. */
>> if (cstate.enabled)
>> clk_disable_unprepare(imx->clk_per);
>> }
>>
>>
>
> Yep.
>
This looks like a good transformation of the current Patch 7, but once
you merge my patch, it will look slightly different...
>>
>> >
>> > My concern was more about the way PWM changes are applied (->apply()
>> > returns before the change is actually applied), but I agreed that it
>> > could be fixed later on (if other people think it's really needed),
>> > since the existing code already handles it this way.
>>
>> This is the issue with FIFO setting - but for now we do not deal with
>> it.
>
> Exactly.
>
>>
>> >
>> > > No clear decision what to change until today when Stefan prepared
>> > > separate (concise) patch (now I see what is the problem).
>> > >
>> >
>> > The patch proposed by Stefan is addressing a different problem: the
>> > periph clock has to be enabled before accessing registers.
>>
>> So for this reason Stefan's patch [1] always enable the clock no matter
>> if PWM clock is generated or not.
>
> Yes.
>
>>
>> >
>> > >
>> > > >
>> > > > Same goes for the regression introduced in patch 2: I think it's
>> > > > better to keep things bisectable on all platforms (even if it
>> > > > appeared to work by chance on imx7, it did work before this
>> > > > change).
>> > >
>> > > Could you be more specific about your idea to solve this problem?
>> >
>> > Stefan already provided a patch, I just think it should be fixed
>> > before patch 2 to avoid breaking bisectibility.
>>
>> My idea is as follows:
>>
>> I will drop patch v2 (prepared by Sasha) and then squash Stefan's patch
>> [1] to patch 7/11. The "old" ipg enable code will be removed with other
>> not needed code during conversion.
>
> How about keeping patch 2 but enabling/disabling the periph clk
> in imx_pwm_config() instead of completely dropping the enable/disable
> clk sequence.
>
> In patch 7 you just add the logic we talked about earlier:
> unconditionally enable the periph clk when entering the
> imx_pwm_apply_v2() function and disable it before leaving the function.
>
> This way you can preserve bisectibility and still get rid of the ipg
> clk.
>
> Stefan, what's your opinion?
We will get rid of the ipg clocks anyway in patch 8 (which removes those
functions completely).
So I think Lukasz approach should be fine, just drop patch 2 and squash
my patch into patch 7.
--
Stefan
