[PATCH v3 4/4] pwm: meson: make full use of common clock framework

[Heiner Kallweit]

On 12.04.2023 23:05, Martin Blumenstingl wrote:
> Hi Heiner,
> 
> On Wed, Apr 12, 2023 at 9:23 PM Heiner Kallweit <hkallweit1 at gmail.com> wrote:
>>
>> Newer versions of the PWM block use a core clock with external mux,
>> divider, and gate. These components either don't exist any longer in
>> the PWM block, or they are bypassed.
>> To minimize needed changes for supporting the new version, the internal
>> divider and gate should be handled by CCF too.
>>
>> I didn't see a good way to split the patch, therefore it's somewhat
>> bigger. What it does:
>>
>> - The internal mux is handled by CCF already. Register also internal
>>   divider and gate with CCF, so that we have one representation of the
>>   input clock: [mux] parent of [divider] parent of [gate]
>>
>> - Now that CCF selects an appropriate mux parent, we don't need the
>>   DT-provided default parent any longer. Accordingly we can also omit
>>   setting the mux parent directly in the driver.
>>
>> - Instead of manually handling the pre-div divider value, let CCF
>>   set the input clock. Targeted input clock frequency is
>>   0xffff * 1/period for best precision.
>>
>> - For the "inverted pwm disabled" scenario target an input clock
>>   frequency of 1GHz. This ensures that the remaining low pulses
>>   have minimum length.
>>
>> I don't have hw with the old PWM block, therefore I couldn't test this
>> patch. With the not yet included extension for the new PWM block
>> (channel->clock directly coming from get_clk(external_clk)) I didn't
>> notice any problem. My system uses PWM for the CPU voltage regulator
>> and for the SDIO 32kHz clock.
>>
>> Note: The clock gate in the old PWM block is permanently disabled.
>> This seems to indicate that it's not used by the new PWM block.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1 at gmail.com>
> Tested-by: Martin Blumenstingl <martin.blumenstingl at googlemail.com> #
> meson8b-odroidc1, sm1-x96-air
> 
> Generally I'm very happy with this - only a few small questions/comments below.
> 
> [...]
>> +       state->enabled = __clk_is_enabled(channel->clk) && (value & tmp) == tmp;
> I was about to suggest that clk_hw_is_enabled() should be used instead
> of __clk_is_enabled()
> That would be easy for SoCs where the gate is part of the PWM IP. But
> it would not work (at least I don't think that it would) work for the
> newer IP that Heiner's described where the gate is part of the SoC's
> clock controller (and thus outside the PWM controller registers). To
> me this means that we need to keep __clk_is_enabled() here unless
> somebody knows of a better approach.
> 
> The "(value & tmp) == tmp" can now be simplified to !!(value &
> BIT(channel_data->pwm_en_bit)) as we're now only checking a single bit
> (previously we were checking two bits in one statement, so that more
> complex check was needed).
> 
> [...]
>> +               channel->gate.reg = meson->base + REG_MISC_AB;
>> +               channel->gate.bit_idx = meson_pwm_per_channel_data[i].clk_en_bit;
>> +               channel->gate.hw.init = &init;
>> +               channel->gate.flags = 0;
>> +               channel->gate.lock = &meson->lock;
>> +
>> +               channel->clk = devm_clk_register(dev, &channel->gate.hw);
> If I recall correctly Jerome previously suggested that I should use:
> - devm_clk_hw_register() to initially register the clock
> - then use (for example) channel->clk = devm_clk_hw_get_clk(dev,
> &channel->gate.hw, "pwm0")
> 
> It's not the most common pattern (yet) but if I recall correctly this
> is also what the CCF maintainers agreed to be the way forward.
> 
One more remark/question on this pattern. In __clk_register() there's
the following comment:
/*
* Don't call clk_hw_create_clk() here because that would pin the
* provider module to itself and prevent it from ever being removed.
*/
I think we have the same situation here when calling devm_clk_hw_register()
and devm_clk_hw_get_clk() both from pwm-meson. So we may not be able to
remove module pwm-meson. Shouldn't be really relevant for us because we
need pwm for the CPU voltage regulator and therefore have it built-in.
But it wouldn't be nice.

> 
> Best regards,
> Martin