Re: [PATCH RFC/RFT] pwm: meson: make full use of common clock framework

[Heiner Kallweit <hkallweit1@gmail.com>]

On 03.04.2023 23:01, Martin Blumenstingl wrote:
> Hello Heiner,
> 
> On Tue, Mar 28, 2023 at 10:59 PM Heiner Kallweit <hkallweit1@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;
+
 		mux_clk = devm_clk_register(dev, &channel->mux.hw);
 		if (IS_ERR(mux_clk)) {
 			err = PTR_ERR(mux_clk);
-- 
2.40.0



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel