[PATCH RFC/RFT] pwm: meson: make full use of common clock framework
Neil Armstrong
neil.armstrong at linaro.org
Thu Apr 6 00:45:42 PDT 2023
On 05/04/2023 22:59, Jerome Brunet wrote:
>
> On Wed 05 Apr 2023 at 22:43, Heiner Kallweit <hkallweit1 at gmail.com> wrote:
>
>> On 03.04.2023 23:01, Martin Blumenstingl wrote:
>>> Hello Heiner,
>>>
>>> On Tue, Mar 28, 2023 at 10:59 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.
>>> That sounds like a good way forward to me
>>>
>>>> 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.
>>> Unfortunately I didn't have much time today so I didn't get to reviewing this.
>>>
>>>> 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.
>>>>
>>>> I'd appreciate testing on the different platforms using the old
>>>> PWM block.
>>> I have tested basic functionality on a X96 Air (SM1 SoC, the version
>>> with Gbit/s PHY) where the VDDCPU regulator is PWM based and the 32kHz
>>> clock for wifi is generated by the PWM controller.
>>> The RTL8822CS SDIO wifi card is still working (firmware download,
>>> basic connectivity and connecting to an AP) and the system survived a
>>> minute of 100% CPU usage without hanging.
>>>
>>> For reference:
>>> # cat /sys/kernel/debug/pwm
>>> platform/ffd19000.pwm, 2 PWM devices
>>> pwm-0 (wifi32k ): requested enabled period: 30518 ns
>>> duty: 15259 ns polarity: normal
>>> pwm-1 ((null) ): period: 0 ns duty: 0 ns polarity: normal
>>>
>>> platform/ff807000.pwm, 2 PWM devices
>>> pwm-0 ((null) ): period: 0 ns duty: 0 ns polarity: normal
>>> pwm-1 ((null) ): period: 0 ns duty: 0 ns polarity: normal
>>>
>>> platform/ff802000.pwm, 2 PWM devices
>>> pwm-0 ((null) ): period: 0 ns duty: 0 ns polarity: normal
>>> pwm-1 (regulator-vddcpu ): requested enabled period: 1500 ns
>>> duty: 1125 ns polarity: normal
>>>
>>> # grep \.pwm /sys/kernel/debug/clk/clk_summary
>>> ffd19000.pwm#mux0 1 1 0 648999985
>>> 0 0 50000 Y
>>> ffd19000.pwm#div0 1 1 0
>>> 648999985 0 0 50000 Y
>>> ffd19000.pwm#gate0 1 1 0
>>> 648999985 0 0 50000 Y
>>> ffd19000.pwm#mux1 0 0 0 24000000
>>> 0 0 50000 Y
>>> ffd19000.pwm#div1 0 0 0 24000000
>>> 0 0 50000 Y
>>> ffd19000.pwm#gate1 0 0 0 24000000
>>> 0 0 50000 N
>>> ff807000.pwm#mux1 0 0 0 24000000
>>> 0 0 50000 Y
>>> ff807000.pwm#div1 0 0 0 24000000
>>> 0 0 50000 Y
>>> ff807000.pwm#gate1 0 0 0 24000000
>>> 0 0 50000 N
>>> ff807000.pwm#mux0 0 0 0 24000000
>>> 0 0 50000 Y
>>> ff807000.pwm#div0 0 0 0 24000000
>>> 0 0 50000 Y
>>> ff807000.pwm#gate0 0 0 0 24000000
>>> 0 0 50000 N
>>> ff802000.pwm#mux1 1 1 0 24000000
>>> 0 0 50000 Y
>>> ff802000.pwm#div1 1 1 0 24000000
>>> 0 0 50000 Y
>>> ff802000.pwm#gate1 1 1 0 24000000
>>> 0 0 50000 Y
>>> ff802000.pwm#mux0 0 0 0 24000000
>>> 0 0 50000 Y
>>> ff802000.pwm#div0 0 0 0 24000000
>>> 0 0 50000 Y
>>> ff802000.pwm#gate0 0 0 0 24000000
>>> 0 0 50000 N
>>>
>>> hdmi_pll is the parent of ffd19000.pwm#mux0 - before it was using the
>>> 24MHz XTAL.
>>> I haven't tested what happens when I change the video mode (that board
>>> is currently not connected to any HDMI screen).
>>>
>>
>> That's a good point. AFAICS drivers/gpu/drm/meson/meson_vclk.c fiddles
>> with the hdmi clock registers. So we may want to avoid using hdmi_pll
>> or vid_pll as pwm parent. Below is a quick (and hopefully not too dirty)
>> follow-up patch disabling the hdmi/video clock parent.
>> Would be great if you can test this patch on top.
>>
>>> Later this week I can also try this e.g. on my Odroid-C1 (with 32-bit
>>> Meson8b SoC) to verify that we don't have any 32-bit compatibility
>>> issues.
>>>
>>>
>>> Best regards,
>>> Martin
>>
>> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
>> index 2b1debda4..81900e03a 100644
>> --- a/drivers/pwm/pwm-meson.c
>> +++ b/drivers/pwm/pwm-meson.c
>> @@ -348,7 +348,7 @@ static const struct pwm_ops meson_pwm_ops = {
>> };
>>
>> static const char * const pwm_meson8b_parent_names[] = {
>> - "xtal", "vid_pll", "fclk_div4", "fclk_div3"
>> + "xtal", "fclk_div4", "fclk_div3"
>> };
>>
>> static const struct meson_pwm_data pwm_meson8b_data = {
>> @@ -357,7 +357,7 @@ static const struct meson_pwm_data pwm_meson8b_data = {
>> };
>>
>> static const char * const pwm_gxbb_parent_names[] = {
>> - "xtal", "hdmi_pll", "fclk_div4", "fclk_div3"
>> + "xtal", "fclk_div4", "fclk_div3"
>> };
>>
>> static const struct meson_pwm_data pwm_gxbb_data = {
>> @@ -415,7 +415,7 @@ static const struct meson_pwm_data pwm_g12a_ao_cd_data = {
>> };
>>
>> static const char * const pwm_g12a_ee_parent_names[] = {
>> - "xtal", "hdmi_pll", "fclk_div4", "fclk_div3"
>> + "xtal", "fclk_div4", "fclk_div3"
>> };
>>
>> static const struct meson_pwm_data pwm_g12a_ee_data = {
>> @@ -470,6 +470,7 @@ static int meson_pwm_init_channels(struct meson_pwm *meson)
>>
>> for (i = 0; i < meson->chip.npwm; i++) {
>> struct meson_pwm_channel *channel = &meson->channels[i];
>> + static const u32 mux_parents_wo_vid[] = {0, 2, 3};
>> const char *clk_parent[1];
>> struct clk *mux_clk, *div_clk;
>>
>> @@ -490,6 +491,10 @@ static int meson_pwm_init_channels(struct meson_pwm *meson)
>> channel->mux.table = NULL;
>> channel->mux.hw.init = &init;
>>
>> + /* 3 parents indicates that video clock parent should be omitted */
>> + if (init.num_parents == 3)
>> + channel->mux.table = mux_parents_wo_vid;
>> +
>
> If you are reworking the pwm driver and its clock usage, I would suggest
> to also stop using global clock names within the driver. The way it is
> used right now is not great. It is essentially the pwm driver directly
> poking the clock tree, without going through DT as it should.
>
> You could have clkin0/1/2/3 from DT, using fw_name for the clock mux.
> This how it should be used. All input being optionnal, you would not any
> dirty trick in the driver to skip an input. You would just not provide
> it through DT.
>
> This approach would be compatible with all the SoCs (compared to our
> current approach which require a new table for each SoC)
>
> Off the course the bindings would be different, so it would probably
> require a new compatible (-v2 ?)
Note the driver should still support the current bindings.
Neil
>
>> mux_clk = devm_clk_register(dev, &channel->mux.hw);
>> if (IS_ERR(mux_clk)) {
>> err = PTR_ERR(mux_clk);
>
>
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic
More information about the linux-amlogic
mailing list