| Message ID | 20220316152746.47768-5-tommaso.merciai@amarulasolutions.com |
|---|---|
| State | Superseded |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | imx8mm: add pwm-imx backlight support | expand |
Hi Tommaaso On Wed, Mar 16, 2022 at 4:28 PM Tommaso Merciai <tommaso.merciai@amarulasolutions.com> wrote: > > Add function enable_pwm_clk into in clock_imx8mm.c. This > function first configure, then enable pwm clock from clock control > register. The following configuration is used: > > source(0) -> 24 MHz ref clock > div(0) -> no division for this clock > > References: > - iMX8MMRM.pdf p 303 > > Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com> > --- > Changes since v1: > - Fix enable_pwm_clk function implementation. Now is generic for all pwm clks > > arch/arm/mach-imx/imx8m/clock_imx8mm.c | 53 ++++++++++++++++++++++++++ > 1 file changed, 53 insertions(+) > > diff --git a/arch/arm/mach-imx/imx8m/clock_imx8mm.c b/arch/arm/mach-imx/imx8m/clock_imx8mm.c > index 49945faf2c..ffb9456607 100644 > --- a/arch/arm/mach-imx/imx8m/clock_imx8mm.c > +++ b/arch/arm/mach-imx/imx8m/clock_imx8mm.c > @@ -313,6 +313,59 @@ void enable_usboh3_clk(unsigned int enable) > } > } > > +void enable_pwm_clk(u32 index, unsigned char enable) > +{ > + switch (index) { > + case 0: > + if (enable) { > + clock_enable(CCGR_PWM1, false); > + clock_set_target_val(PWM1_CLK_ROOT, CLK_ROOT_ON | > + CLK_ROOT_SOURCE_SEL(0) | > + CLK_ROOT_PRE_DIV(CLK_ROOT_PRE_DIV1)); > + clock_enable(CCGR_PWM1, true); > + } else { > + clock_enable(CCGR_PWM1, false); Pwm is alway before set to false and then enable. Make sense to move out. Then all the code is look quite the same apart minior change Can you clean up in order to have a more compact implementation? Michael > + } > + return; > + case 1: > + if (enable) { > + clock_enable(CCGR_PWM2, false); > + clock_set_target_val(PWM2_CLK_ROOT, CLK_ROOT_ON | > + CLK_ROOT_SOURCE_SEL(0) | > + CLK_ROOT_PRE_DIV(CLK_ROOT_PRE_DIV1)); > + clock_enable(CCGR_PWM2, true); > + } else { > + clock_enable(CCGR_PWM2, false); > + } > + return; > + case 2: > + if (enable) { > + clock_enable(CCGR_PWM3, false); > + clock_set_target_val(PWM3_CLK_ROOT, CLK_ROOT_ON | > + CLK_ROOT_SOURCE_SEL(0) | > + CLK_ROOT_PRE_DIV(CLK_ROOT_PRE_DIV1)); > + clock_enable(CCGR_PWM3, true); > + } else { > + clock_enable(CCGR_PWM3, false); > + } > + return; > + case 3: > + if (enable) { > + clock_enable(CCGR_PWM4, false); > + clock_set_target_val(PWM4_CLK_ROOT, CLK_ROOT_ON | > + CLK_ROOT_SOURCE_SEL(0) | > + CLK_ROOT_PRE_DIV(CLK_ROOT_PRE_DIV1)); > + clock_enable(CCGR_PWM4, true); > + } else { > + clock_enable(CCGR_PWM4, false); > + } > + return; > + default: > + printf("Invalid pwm index\n"); > + return; > + } > +} > + Please factorize things that are always eegual > void init_uart_clk(u32 index) > { > /* > -- > 2.25.1 > -- Michael Nazzareno Trimarchi Co-Founder & Chief Executive Officer M. +39 347 913 2170 michael@amarulasolutions.com
On 3/16/22 16:27, Tommaso Merciai wrote: > Add function enable_pwm_clk into in clock_imx8mm.c. This > function first configure, then enable pwm clock from clock control > register. The following configuration is used: > > source(0) -> 24 MHz ref clock > div(0) -> no division for this clock > > References: > - iMX8MMRM.pdf p 303 > > Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com> > --- > Changes since v1: > - Fix enable_pwm_clk function implementation. Now is generic for all pwm clks > > arch/arm/mach-imx/imx8m/clock_imx8mm.c | 53 ++++++++++++++++++++++++++ > 1 file changed, 53 insertions(+) Why is this not in drivers/clk/imx/ DM driver ?
On Wed, Mar 16, 2022 at 09:54:34PM +0100, Marek Vasut wrote: > On 3/16/22 16:27, Tommaso Merciai wrote: > > Add function enable_pwm_clk into in clock_imx8mm.c. This > > function first configure, then enable pwm clock from clock control > > register. The following configuration is used: > > > > source(0) -> 24 MHz ref clock > > div(0) -> no division for this clock > > > > References: > > - iMX8MMRM.pdf p 303 > > > > Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com> > > --- > > Changes since v1: > > - Fix enable_pwm_clk function implementation. Now is generic for all pwm clks > > > > arch/arm/mach-imx/imx8m/clock_imx8mm.c | 53 ++++++++++++++++++++++++++ > > 1 file changed, 53 insertions(+) > > Why is this not in drivers/clk/imx/ DM driver ? Hi Marek, All function that enable/configure clk from CCGR are in arch/arm/mach-imx/imx8m/clock_imx8mm.c. For that I continue to put here the implementation. After we can port the clk dm part to manipulate clock in drivers/clk/imx/ DM driver. What do you think about? Let me know. Thanks, Tommaso
On Wed, Mar 16, 2022 at 08:07:01PM +0100, Michael Nazzareno Trimarchi wrote: > Hi Tommaaso > > > On Wed, Mar 16, 2022 at 4:28 PM Tommaso Merciai > <tommaso.merciai@amarulasolutions.com> wrote: > > > > Add function enable_pwm_clk into in clock_imx8mm.c. This > > function first configure, then enable pwm clock from clock control > > register. The following configuration is used: > > > > source(0) -> 24 MHz ref clock > > div(0) -> no division for this clock > > > > References: > > - iMX8MMRM.pdf p 303 > > > > Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com> > > --- > > Changes since v1: > > - Fix enable_pwm_clk function implementation. Now is generic for all pwm clks > > > > arch/arm/mach-imx/imx8m/clock_imx8mm.c | 53 ++++++++++++++++++++++++++ > > 1 file changed, 53 insertions(+) > > > > diff --git a/arch/arm/mach-imx/imx8m/clock_imx8mm.c b/arch/arm/mach-imx/imx8m/clock_imx8mm.c > > index 49945faf2c..ffb9456607 100644 > > --- a/arch/arm/mach-imx/imx8m/clock_imx8mm.c > > +++ b/arch/arm/mach-imx/imx8m/clock_imx8mm.c > > @@ -313,6 +313,59 @@ void enable_usboh3_clk(unsigned int enable) > > } > > } > > > > +void enable_pwm_clk(u32 index, unsigned char enable) > > +{ > > + switch (index) { > > + case 0: > > + if (enable) { > > + clock_enable(CCGR_PWM1, false); > > + clock_set_target_val(PWM1_CLK_ROOT, CLK_ROOT_ON | > > + CLK_ROOT_SOURCE_SEL(0) | > > + CLK_ROOT_PRE_DIV(CLK_ROOT_PRE_DIV1)); > > + clock_enable(CCGR_PWM1, true); > > + } else { > > + clock_enable(CCGR_PWM1, false); > > Pwm is alway before set to false and then enable. Make sense to move > out. Then all the code is look quite the same apart > minior change > > Can you clean up in order to have a more compact implementation? Hi Michael, Ok, I remove the else in the implementation in v3. Thanks, Tommaso > > Michael > > > + } > > > > + return; > > + case 1: > > + if (enable) { > > + clock_enable(CCGR_PWM2, false); > > + clock_set_target_val(PWM2_CLK_ROOT, CLK_ROOT_ON | > > + CLK_ROOT_SOURCE_SEL(0) | > > + CLK_ROOT_PRE_DIV(CLK_ROOT_PRE_DIV1)); > > + clock_enable(CCGR_PWM2, true); > > + } else { > > + clock_enable(CCGR_PWM2, false); > > + } > > + return; > > + case 2: > > + if (enable) { > > + clock_enable(CCGR_PWM3, false); > > + clock_set_target_val(PWM3_CLK_ROOT, CLK_ROOT_ON | > > + CLK_ROOT_SOURCE_SEL(0) | > > + CLK_ROOT_PRE_DIV(CLK_ROOT_PRE_DIV1)); > > + clock_enable(CCGR_PWM3, true); > > + } else { > > + clock_enable(CCGR_PWM3, false); > > + } > > + return; > > + case 3: > > + if (enable) { > > + clock_enable(CCGR_PWM4, false); > > + clock_set_target_val(PWM4_CLK_ROOT, CLK_ROOT_ON | > > + CLK_ROOT_SOURCE_SEL(0) | > > + CLK_ROOT_PRE_DIV(CLK_ROOT_PRE_DIV1)); > > + clock_enable(CCGR_PWM4, true); > > + } else { > > + clock_enable(CCGR_PWM4, false); > > + } > > + return; > > + default: > > + printf("Invalid pwm index\n"); > > + return; > > + } > > +} > > + > > Please factorize things that are always eegual > > void init_uart_clk(u32 index) > > { > > /* > > -- > > 2.25.1 > > > > > -- > Michael Nazzareno Trimarchi > Co-Founder & Chief Executive Officer > M. +39 347 913 2170 > michael@amarulasolutions.com > __________________________________ > > Amarula Solutions BV > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL > T. +31 (0)85 111 9172 > info@amarulasolutions.com > www.amarulasolutions.com
On 3/17/22 08:39, Tommaso Merciai wrote: > On Wed, Mar 16, 2022 at 09:54:34PM +0100, Marek Vasut wrote: >> On 3/16/22 16:27, Tommaso Merciai wrote: >>> Add function enable_pwm_clk into in clock_imx8mm.c. This >>> function first configure, then enable pwm clock from clock control >>> register. The following configuration is used: >>> >>> source(0) -> 24 MHz ref clock >>> div(0) -> no division for this clock >>> >>> References: >>> - iMX8MMRM.pdf p 303 >>> >>> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com> >>> --- >>> Changes since v1: >>> - Fix enable_pwm_clk function implementation. Now is generic for all pwm clks >>> >>> arch/arm/mach-imx/imx8m/clock_imx8mm.c | 53 ++++++++++++++++++++++++++ >>> 1 file changed, 53 insertions(+) >> >> Why is this not in drivers/clk/imx/ DM driver ? > > Hi Marek, > All function that enable/configure clk from CCGR are in arch/arm/mach-imx/imx8m/clock_imx8mm.c. These seems to be CCGR: $ grep -C 2 '0x4[0-9a-f]\{3\}' drivers/clk/imx/clk-imx8mm.c | sed "s@^.@@" clk_dm(IMX8MM_CLK_ECSPI1_ROOT, imx_clk_gate4("ecspi1_root_clk", "ecspi1", base + 0x4070, 0)); clk_dm(IMX8MM_CLK_ECSPI2_ROOT, imx_clk_gate4("ecspi2_root_clk", "ecspi2", base + 0x4080, 0)); clk_dm(IMX8MM_CLK_ECSPI3_ROOT, imx_clk_gate4("ecspi3_root_clk", "ecspi3", base + 0x4090, 0)); clk_dm(IMX8MM_CLK_I2C1_ROOT, imx_clk_gate4("i2c1_root_clk", "i2c1", base + 0x4170, 0)); clk_dm(IMX8MM_CLK_I2C2_ROOT, imx_clk_gate4("i2c2_root_clk", "i2c2", base + 0x4180, 0)); clk_dm(IMX8MM_CLK_I2C3_ROOT, imx_clk_gate4("i2c3_root_clk", "i2c3", base + 0x4190, 0)); clk_dm(IMX8MM_CLK_I2C4_ROOT, imx_clk_gate4("i2c4_root_clk", "i2c4", base + 0x41a0, 0)); clk_dm(IMX8MM_CLK_OCOTP_ROOT, imx_clk_gate4("ocotp_root_clk", "ipg_root", base + 0x4220, 0)); clk_dm(IMX8MM_CLK_USDHC1_ROOT, imx_clk_gate4("usdhc1_root_clk", "usdhc1", base + 0x4510, 0)); clk_dm(IMX8MM_CLK_USDHC2_ROOT, imx_clk_gate4("usdhc2_root_clk", "usdhc2", base + 0x4520, 0)); clk_dm(IMX8MM_CLK_WDOG1_ROOT, imx_clk_gate4("wdog1_root_clk", "wdog", base + 0x4530, 0)); clk_dm(IMX8MM_CLK_WDOG2_ROOT, imx_clk_gate4("wdog2_root_clk", "wdog", base + 0x4540, 0)); clk_dm(IMX8MM_CLK_WDOG3_ROOT, imx_clk_gate4("wdog3_root_clk", "wdog", base + 0x4550, 0)); clk_dm(IMX8MM_CLK_USDHC3_ROOT, imx_clk_gate4("usdhc3_root_clk", "usdhc3", base + 0x45e0, 0)); clk_dm(IMX8MM_CLK_QSPI_ROOT, imx_clk_gate4("qspi_root_clk", "qspi", base + 0x42f0, 0)); clk_dm(IMX8MM_CLK_USB1_CTRL_ROOT, imx_clk_gate4("usb1_ctrl_root_clk", "usb_bus", base + 0x44d0, 0)); /* clks not needed in SPL stage */ - clk_dm(IMX8MM_CLK_ENET1_ROOT, imx_clk_gate4("enet1_root_clk", "enet_axi", base + 0x40a0, 0)); endif > For that I continue to put here the implementation. After we can port > the clk dm part to manipulate clock in drivers/clk/imx/ DM driver. > What do you think about? Let me know. Seems like the clk_dm part is already in place and all you have to do is extend it.
On Thu, Mar 17, 2022 at 12:58:31PM +0100, Marek Vasut wrote: > On 3/17/22 08:39, Tommaso Merciai wrote: > > On Wed, Mar 16, 2022 at 09:54:34PM +0100, Marek Vasut wrote: > > > On 3/16/22 16:27, Tommaso Merciai wrote: > > > > Add function enable_pwm_clk into in clock_imx8mm.c. This > > > > function first configure, then enable pwm clock from clock control > > > > register. The following configuration is used: > > > > > > > > source(0) -> 24 MHz ref clock > > > > div(0) -> no division for this clock > > > > > > > > References: > > > > - iMX8MMRM.pdf p 303 > > > > > > > > Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com> > > > > --- > > > > Changes since v1: > > > > - Fix enable_pwm_clk function implementation. Now is generic for all pwm clks > > > > > > > > arch/arm/mach-imx/imx8m/clock_imx8mm.c | 53 ++++++++++++++++++++++++++ > > > > 1 file changed, 53 insertions(+) > > > > > > Why is this not in drivers/clk/imx/ DM driver ? > > > > Hi Marek, > > All function that enable/configure clk from CCGR are in arch/arm/mach-imx/imx8m/clock_imx8mm.c. > > These seems to be CCGR: > > $ grep -C 2 '0x4[0-9a-f]\{3\}' drivers/clk/imx/clk-imx8mm.c | sed "s@^.@@" > > clk_dm(IMX8MM_CLK_ECSPI1_ROOT, > imx_clk_gate4("ecspi1_root_clk", "ecspi1", base + 0x4070, 0)); > clk_dm(IMX8MM_CLK_ECSPI2_ROOT, > imx_clk_gate4("ecspi2_root_clk", "ecspi2", base + 0x4080, 0)); > clk_dm(IMX8MM_CLK_ECSPI3_ROOT, > imx_clk_gate4("ecspi3_root_clk", "ecspi3", base + 0x4090, 0)); > clk_dm(IMX8MM_CLK_I2C1_ROOT, > imx_clk_gate4("i2c1_root_clk", "i2c1", base + 0x4170, 0)); > clk_dm(IMX8MM_CLK_I2C2_ROOT, > imx_clk_gate4("i2c2_root_clk", "i2c2", base + 0x4180, 0)); > clk_dm(IMX8MM_CLK_I2C3_ROOT, > imx_clk_gate4("i2c3_root_clk", "i2c3", base + 0x4190, 0)); > clk_dm(IMX8MM_CLK_I2C4_ROOT, > imx_clk_gate4("i2c4_root_clk", "i2c4", base + 0x41a0, 0)); > clk_dm(IMX8MM_CLK_OCOTP_ROOT, > imx_clk_gate4("ocotp_root_clk", "ipg_root", base + 0x4220, 0)); > clk_dm(IMX8MM_CLK_USDHC1_ROOT, > imx_clk_gate4("usdhc1_root_clk", "usdhc1", base + 0x4510, 0)); > clk_dm(IMX8MM_CLK_USDHC2_ROOT, > imx_clk_gate4("usdhc2_root_clk", "usdhc2", base + 0x4520, 0)); > clk_dm(IMX8MM_CLK_WDOG1_ROOT, > imx_clk_gate4("wdog1_root_clk", "wdog", base + 0x4530, 0)); > clk_dm(IMX8MM_CLK_WDOG2_ROOT, > imx_clk_gate4("wdog2_root_clk", "wdog", base + 0x4540, 0)); > clk_dm(IMX8MM_CLK_WDOG3_ROOT, > imx_clk_gate4("wdog3_root_clk", "wdog", base + 0x4550, 0)); > clk_dm(IMX8MM_CLK_USDHC3_ROOT, > imx_clk_gate4("usdhc3_root_clk", "usdhc3", base + 0x45e0, 0)); > clk_dm(IMX8MM_CLK_QSPI_ROOT, > imx_clk_gate4("qspi_root_clk", "qspi", base + 0x42f0, 0)); > clk_dm(IMX8MM_CLK_USB1_CTRL_ROOT, > imx_clk_gate4("usb1_ctrl_root_clk", "usb_bus", base + 0x44d0, 0)); > > /* clks not needed in SPL stage */ > - > clk_dm(IMX8MM_CLK_ENET1_ROOT, > imx_clk_gate4("enet1_root_clk", "enet_axi", > base + 0x40a0, 0)); > endif > > > For that I continue to put here the implementation. After we can port > > the clk dm part to manipulate clock in drivers/clk/imx/ DM driver. > > What do you think about? Let me know. > > Seems like the clk_dm part is already in place and all you have to do is > extend it. Hi Marek, I'll try also this way and let you know. Thanks, Tommaso
On 3/17/22 13:38, Tommaso Merciai wrote: > On Thu, Mar 17, 2022 at 12:58:31PM +0100, Marek Vasut wrote: >> On 3/17/22 08:39, Tommaso Merciai wrote: >>> On Wed, Mar 16, 2022 at 09:54:34PM +0100, Marek Vasut wrote: >>>> On 3/16/22 16:27, Tommaso Merciai wrote: >>>>> Add function enable_pwm_clk into in clock_imx8mm.c. This >>>>> function first configure, then enable pwm clock from clock control >>>>> register. The following configuration is used: >>>>> >>>>> source(0) -> 24 MHz ref clock >>>>> div(0) -> no division for this clock >>>>> >>>>> References: >>>>> - iMX8MMRM.pdf p 303 >>>>> >>>>> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com> >>>>> --- >>>>> Changes since v1: >>>>> - Fix enable_pwm_clk function implementation. Now is generic for all pwm clks >>>>> >>>>> arch/arm/mach-imx/imx8m/clock_imx8mm.c | 53 ++++++++++++++++++++++++++ >>>>> 1 file changed, 53 insertions(+) >>>> >>>> Why is this not in drivers/clk/imx/ DM driver ? >>> >>> Hi Marek, >>> All function that enable/configure clk from CCGR are in arch/arm/mach-imx/imx8m/clock_imx8mm.c. >> >> These seems to be CCGR: >> >> $ grep -C 2 '0x4[0-9a-f]\{3\}' drivers/clk/imx/clk-imx8mm.c | sed "s@^.@@" >> >> clk_dm(IMX8MM_CLK_ECSPI1_ROOT, >> imx_clk_gate4("ecspi1_root_clk", "ecspi1", base + 0x4070, 0)); >> clk_dm(IMX8MM_CLK_ECSPI2_ROOT, >> imx_clk_gate4("ecspi2_root_clk", "ecspi2", base + 0x4080, 0)); >> clk_dm(IMX8MM_CLK_ECSPI3_ROOT, >> imx_clk_gate4("ecspi3_root_clk", "ecspi3", base + 0x4090, 0)); >> clk_dm(IMX8MM_CLK_I2C1_ROOT, >> imx_clk_gate4("i2c1_root_clk", "i2c1", base + 0x4170, 0)); >> clk_dm(IMX8MM_CLK_I2C2_ROOT, >> imx_clk_gate4("i2c2_root_clk", "i2c2", base + 0x4180, 0)); >> clk_dm(IMX8MM_CLK_I2C3_ROOT, >> imx_clk_gate4("i2c3_root_clk", "i2c3", base + 0x4190, 0)); >> clk_dm(IMX8MM_CLK_I2C4_ROOT, >> imx_clk_gate4("i2c4_root_clk", "i2c4", base + 0x41a0, 0)); >> clk_dm(IMX8MM_CLK_OCOTP_ROOT, >> imx_clk_gate4("ocotp_root_clk", "ipg_root", base + 0x4220, 0)); >> clk_dm(IMX8MM_CLK_USDHC1_ROOT, >> imx_clk_gate4("usdhc1_root_clk", "usdhc1", base + 0x4510, 0)); >> clk_dm(IMX8MM_CLK_USDHC2_ROOT, >> imx_clk_gate4("usdhc2_root_clk", "usdhc2", base + 0x4520, 0)); >> clk_dm(IMX8MM_CLK_WDOG1_ROOT, >> imx_clk_gate4("wdog1_root_clk", "wdog", base + 0x4530, 0)); >> clk_dm(IMX8MM_CLK_WDOG2_ROOT, >> imx_clk_gate4("wdog2_root_clk", "wdog", base + 0x4540, 0)); >> clk_dm(IMX8MM_CLK_WDOG3_ROOT, >> imx_clk_gate4("wdog3_root_clk", "wdog", base + 0x4550, 0)); >> clk_dm(IMX8MM_CLK_USDHC3_ROOT, >> imx_clk_gate4("usdhc3_root_clk", "usdhc3", base + 0x45e0, 0)); >> clk_dm(IMX8MM_CLK_QSPI_ROOT, >> imx_clk_gate4("qspi_root_clk", "qspi", base + 0x42f0, 0)); >> clk_dm(IMX8MM_CLK_USB1_CTRL_ROOT, >> imx_clk_gate4("usb1_ctrl_root_clk", "usb_bus", base + 0x44d0, 0)); >> >> /* clks not needed in SPL stage */ >> - >> clk_dm(IMX8MM_CLK_ENET1_ROOT, >> imx_clk_gate4("enet1_root_clk", "enet_axi", >> base + 0x40a0, 0)); >> endif >> >>> For that I continue to put here the implementation. After we can port >>> the clk dm part to manipulate clock in drivers/clk/imx/ DM driver. >>> What do you think about? Let me know. >> >> Seems like the clk_dm part is already in place and all you have to do is >> extend it. > > Hi Marek, > I'll try also this way and let you know. Thanks
On Thu, Mar 17, 2022 at 01:38:18PM +0100, Tommaso Merciai wrote: > On Thu, Mar 17, 2022 at 12:58:31PM +0100, Marek Vasut wrote: > > On 3/17/22 08:39, Tommaso Merciai wrote: > > > On Wed, Mar 16, 2022 at 09:54:34PM +0100, Marek Vasut wrote: > > > > On 3/16/22 16:27, Tommaso Merciai wrote: > > > > > Add function enable_pwm_clk into in clock_imx8mm.c. This > > > > > function first configure, then enable pwm clock from clock control > > > > > register. The following configuration is used: > > > > > > > > > > source(0) -> 24 MHz ref clock > > > > > div(0) -> no division for this clock > > > > > > > > > > References: > > > > > - iMX8MMRM.pdf p 303 > > > > > > > > > > Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com> > > > > > --- > > > > > Changes since v1: > > > > > - Fix enable_pwm_clk function implementation. Now is generic for all pwm clks > > > > > > > > > > arch/arm/mach-imx/imx8m/clock_imx8mm.c | 53 ++++++++++++++++++++++++++ > > > > > 1 file changed, 53 insertions(+) > > > > > > > > Why is this not in drivers/clk/imx/ DM driver ? > > > > > > Hi Marek, > > > All function that enable/configure clk from CCGR are in arch/arm/mach-imx/imx8m/clock_imx8mm.c. > > > > These seems to be CCGR: > > > > $ grep -C 2 '0x4[0-9a-f]\{3\}' drivers/clk/imx/clk-imx8mm.c | sed "s@^.@@" > > > > clk_dm(IMX8MM_CLK_ECSPI1_ROOT, > > imx_clk_gate4("ecspi1_root_clk", "ecspi1", base + 0x4070, 0)); > > clk_dm(IMX8MM_CLK_ECSPI2_ROOT, > > imx_clk_gate4("ecspi2_root_clk", "ecspi2", base + 0x4080, 0)); > > clk_dm(IMX8MM_CLK_ECSPI3_ROOT, > > imx_clk_gate4("ecspi3_root_clk", "ecspi3", base + 0x4090, 0)); > > clk_dm(IMX8MM_CLK_I2C1_ROOT, > > imx_clk_gate4("i2c1_root_clk", "i2c1", base + 0x4170, 0)); > > clk_dm(IMX8MM_CLK_I2C2_ROOT, > > imx_clk_gate4("i2c2_root_clk", "i2c2", base + 0x4180, 0)); > > clk_dm(IMX8MM_CLK_I2C3_ROOT, > > imx_clk_gate4("i2c3_root_clk", "i2c3", base + 0x4190, 0)); > > clk_dm(IMX8MM_CLK_I2C4_ROOT, > > imx_clk_gate4("i2c4_root_clk", "i2c4", base + 0x41a0, 0)); > > clk_dm(IMX8MM_CLK_OCOTP_ROOT, > > imx_clk_gate4("ocotp_root_clk", "ipg_root", base + 0x4220, 0)); > > clk_dm(IMX8MM_CLK_USDHC1_ROOT, > > imx_clk_gate4("usdhc1_root_clk", "usdhc1", base + 0x4510, 0)); > > clk_dm(IMX8MM_CLK_USDHC2_ROOT, > > imx_clk_gate4("usdhc2_root_clk", "usdhc2", base + 0x4520, 0)); > > clk_dm(IMX8MM_CLK_WDOG1_ROOT, > > imx_clk_gate4("wdog1_root_clk", "wdog", base + 0x4530, 0)); > > clk_dm(IMX8MM_CLK_WDOG2_ROOT, > > imx_clk_gate4("wdog2_root_clk", "wdog", base + 0x4540, 0)); > > clk_dm(IMX8MM_CLK_WDOG3_ROOT, > > imx_clk_gate4("wdog3_root_clk", "wdog", base + 0x4550, 0)); > > clk_dm(IMX8MM_CLK_USDHC3_ROOT, > > imx_clk_gate4("usdhc3_root_clk", "usdhc3", base + 0x45e0, 0)); > > clk_dm(IMX8MM_CLK_QSPI_ROOT, > > imx_clk_gate4("qspi_root_clk", "qspi", base + 0x42f0, 0)); > > clk_dm(IMX8MM_CLK_USB1_CTRL_ROOT, > > imx_clk_gate4("usb1_ctrl_root_clk", "usb_bus", base + 0x44d0, 0)); > > > > /* clks not needed in SPL stage */ > > - > > clk_dm(IMX8MM_CLK_ENET1_ROOT, > > imx_clk_gate4("enet1_root_clk", "enet_axi", > > base + 0x40a0, 0)); > > endif > > > > > For that I continue to put here the implementation. After we can port > > > the clk dm part to manipulate clock in drivers/clk/imx/ DM driver. > > > What do you think about? Let me know. > > > > Seems like the clk_dm part is already in place and all you have to do is > > extend it. Hi Marek, I test the solution using DM model, it work: u-boot=> clk dump 24000000 2 | |-- pwm1 24000000 3 | | `-- pwm1_root_clk 24000000 0 | |-- pwm2 24000000 0 | | `-- pwm2_root_clk 24000000 0 | |-- pwm3 24000000 0 | | `-- pwm3_root_clk 24000000 0 | |-- pwm4 24000000 0 | | `-- pwm4_root_clk I test it using the following call on a dummy driver: ret = uclass_get_device_by_name(UCLASS_PWM, "pwm@30660000", &pwm); if (ret) printk("Failed to get pwm dev\n"); ret = clk_get_by_name(pwm, "per", &per_clk); if (ret) { printf("Failed to get per_clk\n"); return ret; } ret = clk_enable(&per_clk); if (ret) { printf("Failed to enable per_clk\n"); return ret; } ret = clk_get_by_name(pwm, "ipg", &ipg_clk); if (ret) { printf("Failed to get ipg_clk\n"); return ret; } ret = clk_enable(&ipg_clk); if (ret) { printf("Failed to enable ipg_clk\n"); return ret; } It's better to keep both the solutions or only based on DM model? I think we can put this initialization into drivers/pwm/pwm-imx.c imx_pwm_of_to_plat function. What do you think about? Let me know. Thanks, Tommaso > > Hi Marek, > I'll try also this way and let you know. > > Thanks, > Tommaso > > -- > Tommaso Merciai > Embedded Linux Engineer > tommaso.merciai@amarulasolutions.com > __________________________________ > > Amarula Solutions SRL > Via Le Canevare 30, 31100 Treviso, Veneto, IT > T. +39 042 243 5310 > info@amarulasolutions.com > www.amarulasolutions.com
On 3/17/22 16:13, Tommaso Merciai wrote: > On Thu, Mar 17, 2022 at 01:38:18PM +0100, Tommaso Merciai wrote: >> On Thu, Mar 17, 2022 at 12:58:31PM +0100, Marek Vasut wrote: >>> On 3/17/22 08:39, Tommaso Merciai wrote: >>>> On Wed, Mar 16, 2022 at 09:54:34PM +0100, Marek Vasut wrote: >>>>> On 3/16/22 16:27, Tommaso Merciai wrote: >>>>>> Add function enable_pwm_clk into in clock_imx8mm.c. This >>>>>> function first configure, then enable pwm clock from clock control >>>>>> register. The following configuration is used: >>>>>> >>>>>> source(0) -> 24 MHz ref clock >>>>>> div(0) -> no division for this clock >>>>>> >>>>>> References: >>>>>> - iMX8MMRM.pdf p 303 >>>>>> >>>>>> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com> >>>>>> --- >>>>>> Changes since v1: >>>>>> - Fix enable_pwm_clk function implementation. Now is generic for all pwm clks >>>>>> >>>>>> arch/arm/mach-imx/imx8m/clock_imx8mm.c | 53 ++++++++++++++++++++++++++ >>>>>> 1 file changed, 53 insertions(+) >>>>> >>>>> Why is this not in drivers/clk/imx/ DM driver ? >>>> >>>> Hi Marek, >>>> All function that enable/configure clk from CCGR are in arch/arm/mach-imx/imx8m/clock_imx8mm.c. >>> >>> These seems to be CCGR: >>> >>> $ grep -C 2 '0x4[0-9a-f]\{3\}' drivers/clk/imx/clk-imx8mm.c | sed "s@^.@@" >>> >>> clk_dm(IMX8MM_CLK_ECSPI1_ROOT, >>> imx_clk_gate4("ecspi1_root_clk", "ecspi1", base + 0x4070, 0)); >>> clk_dm(IMX8MM_CLK_ECSPI2_ROOT, >>> imx_clk_gate4("ecspi2_root_clk", "ecspi2", base + 0x4080, 0)); >>> clk_dm(IMX8MM_CLK_ECSPI3_ROOT, >>> imx_clk_gate4("ecspi3_root_clk", "ecspi3", base + 0x4090, 0)); >>> clk_dm(IMX8MM_CLK_I2C1_ROOT, >>> imx_clk_gate4("i2c1_root_clk", "i2c1", base + 0x4170, 0)); >>> clk_dm(IMX8MM_CLK_I2C2_ROOT, >>> imx_clk_gate4("i2c2_root_clk", "i2c2", base + 0x4180, 0)); >>> clk_dm(IMX8MM_CLK_I2C3_ROOT, >>> imx_clk_gate4("i2c3_root_clk", "i2c3", base + 0x4190, 0)); >>> clk_dm(IMX8MM_CLK_I2C4_ROOT, >>> imx_clk_gate4("i2c4_root_clk", "i2c4", base + 0x41a0, 0)); >>> clk_dm(IMX8MM_CLK_OCOTP_ROOT, >>> imx_clk_gate4("ocotp_root_clk", "ipg_root", base + 0x4220, 0)); >>> clk_dm(IMX8MM_CLK_USDHC1_ROOT, >>> imx_clk_gate4("usdhc1_root_clk", "usdhc1", base + 0x4510, 0)); >>> clk_dm(IMX8MM_CLK_USDHC2_ROOT, >>> imx_clk_gate4("usdhc2_root_clk", "usdhc2", base + 0x4520, 0)); >>> clk_dm(IMX8MM_CLK_WDOG1_ROOT, >>> imx_clk_gate4("wdog1_root_clk", "wdog", base + 0x4530, 0)); >>> clk_dm(IMX8MM_CLK_WDOG2_ROOT, >>> imx_clk_gate4("wdog2_root_clk", "wdog", base + 0x4540, 0)); >>> clk_dm(IMX8MM_CLK_WDOG3_ROOT, >>> imx_clk_gate4("wdog3_root_clk", "wdog", base + 0x4550, 0)); >>> clk_dm(IMX8MM_CLK_USDHC3_ROOT, >>> imx_clk_gate4("usdhc3_root_clk", "usdhc3", base + 0x45e0, 0)); >>> clk_dm(IMX8MM_CLK_QSPI_ROOT, >>> imx_clk_gate4("qspi_root_clk", "qspi", base + 0x42f0, 0)); >>> clk_dm(IMX8MM_CLK_USB1_CTRL_ROOT, >>> imx_clk_gate4("usb1_ctrl_root_clk", "usb_bus", base + 0x44d0, 0)); >>> >>> /* clks not needed in SPL stage */ >>> - >>> clk_dm(IMX8MM_CLK_ENET1_ROOT, >>> imx_clk_gate4("enet1_root_clk", "enet_axi", >>> base + 0x40a0, 0)); >>> endif >>> >>>> For that I continue to put here the implementation. After we can port >>>> the clk dm part to manipulate clock in drivers/clk/imx/ DM driver. >>>> What do you think about? Let me know. >>> >>> Seems like the clk_dm part is already in place and all you have to do is >>> extend it. > > Hi Marek, > I test the solution using DM model, it work: > > u-boot=> clk dump > > 24000000 2 | |-- pwm1 > 24000000 3 | | `-- pwm1_root_clk > 24000000 0 | |-- pwm2 > 24000000 0 | | `-- pwm2_root_clk > 24000000 0 | |-- pwm3 > 24000000 0 | | `-- pwm3_root_clk > 24000000 0 | |-- pwm4 > 24000000 0 | | `-- pwm4_root_clk > > > I test it using the following call on a dummy driver: > > ret = uclass_get_device_by_name(UCLASS_PWM, "pwm@30660000", &pwm); > if (ret) > printk("Failed to get pwm dev\n"); > > ret = clk_get_by_name(pwm, "per", &per_clk); > if (ret) { > printf("Failed to get per_clk\n"); > return ret; > } > ret = clk_enable(&per_clk); > if (ret) { > printf("Failed to enable per_clk\n"); > return ret; > } > > ret = clk_get_by_name(pwm, "ipg", &ipg_clk); > if (ret) { > printf("Failed to get ipg_clk\n"); > return ret; > } > ret = clk_enable(&ipg_clk); > if (ret) { > printf("Failed to enable ipg_clk\n"); > return ret; > } > > It's better to keep both the solutions or only > based on DM model? I think we can put this initialization into > drivers/pwm/pwm-imx.c imx_pwm_of_to_plat function. > What do you think about? > Let me know. DM is the preferred way, non-DM is fading away.
On Thu, Mar 17, 2022 at 04:31:15PM +0100, Marek Vasut wrote: > On 3/17/22 16:13, Tommaso Merciai wrote: > > On Thu, Mar 17, 2022 at 01:38:18PM +0100, Tommaso Merciai wrote: > > > On Thu, Mar 17, 2022 at 12:58:31PM +0100, Marek Vasut wrote: > > > > On 3/17/22 08:39, Tommaso Merciai wrote: > > > > > On Wed, Mar 16, 2022 at 09:54:34PM +0100, Marek Vasut wrote: > > > > > > On 3/16/22 16:27, Tommaso Merciai wrote: > > > > > > > Add function enable_pwm_clk into in clock_imx8mm.c. This > > > > > > > function first configure, then enable pwm clock from clock control > > > > > > > register. The following configuration is used: > > > > > > > > > > > > > > source(0) -> 24 MHz ref clock > > > > > > > div(0) -> no division for this clock > > > > > > > > > > > > > > References: > > > > > > > - iMX8MMRM.pdf p 303 > > > > > > > > > > > > > > Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com> > > > > > > > --- > > > > > > > Changes since v1: > > > > > > > - Fix enable_pwm_clk function implementation. Now is generic for all pwm clks > > > > > > > > > > > > > > arch/arm/mach-imx/imx8m/clock_imx8mm.c | 53 ++++++++++++++++++++++++++ > > > > > > > 1 file changed, 53 insertions(+) > > > > > > > > > > > > Why is this not in drivers/clk/imx/ DM driver ? > > > > > > > > > > Hi Marek, > > > > > All function that enable/configure clk from CCGR are in arch/arm/mach-imx/imx8m/clock_imx8mm.c. > > > > > > > > These seems to be CCGR: > > > > > > > > $ grep -C 2 '0x4[0-9a-f]\{3\}' drivers/clk/imx/clk-imx8mm.c | sed "s@^.@@" > > > > > > > > clk_dm(IMX8MM_CLK_ECSPI1_ROOT, > > > > imx_clk_gate4("ecspi1_root_clk", "ecspi1", base + 0x4070, 0)); > > > > clk_dm(IMX8MM_CLK_ECSPI2_ROOT, > > > > imx_clk_gate4("ecspi2_root_clk", "ecspi2", base + 0x4080, 0)); > > > > clk_dm(IMX8MM_CLK_ECSPI3_ROOT, > > > > imx_clk_gate4("ecspi3_root_clk", "ecspi3", base + 0x4090, 0)); > > > > clk_dm(IMX8MM_CLK_I2C1_ROOT, > > > > imx_clk_gate4("i2c1_root_clk", "i2c1", base + 0x4170, 0)); > > > > clk_dm(IMX8MM_CLK_I2C2_ROOT, > > > > imx_clk_gate4("i2c2_root_clk", "i2c2", base + 0x4180, 0)); > > > > clk_dm(IMX8MM_CLK_I2C3_ROOT, > > > > imx_clk_gate4("i2c3_root_clk", "i2c3", base + 0x4190, 0)); > > > > clk_dm(IMX8MM_CLK_I2C4_ROOT, > > > > imx_clk_gate4("i2c4_root_clk", "i2c4", base + 0x41a0, 0)); > > > > clk_dm(IMX8MM_CLK_OCOTP_ROOT, > > > > imx_clk_gate4("ocotp_root_clk", "ipg_root", base + 0x4220, 0)); > > > > clk_dm(IMX8MM_CLK_USDHC1_ROOT, > > > > imx_clk_gate4("usdhc1_root_clk", "usdhc1", base + 0x4510, 0)); > > > > clk_dm(IMX8MM_CLK_USDHC2_ROOT, > > > > imx_clk_gate4("usdhc2_root_clk", "usdhc2", base + 0x4520, 0)); > > > > clk_dm(IMX8MM_CLK_WDOG1_ROOT, > > > > imx_clk_gate4("wdog1_root_clk", "wdog", base + 0x4530, 0)); > > > > clk_dm(IMX8MM_CLK_WDOG2_ROOT, > > > > imx_clk_gate4("wdog2_root_clk", "wdog", base + 0x4540, 0)); > > > > clk_dm(IMX8MM_CLK_WDOG3_ROOT, > > > > imx_clk_gate4("wdog3_root_clk", "wdog", base + 0x4550, 0)); > > > > clk_dm(IMX8MM_CLK_USDHC3_ROOT, > > > > imx_clk_gate4("usdhc3_root_clk", "usdhc3", base + 0x45e0, 0)); > > > > clk_dm(IMX8MM_CLK_QSPI_ROOT, > > > > imx_clk_gate4("qspi_root_clk", "qspi", base + 0x42f0, 0)); > > > > clk_dm(IMX8MM_CLK_USB1_CTRL_ROOT, > > > > imx_clk_gate4("usb1_ctrl_root_clk", "usb_bus", base + 0x44d0, 0)); > > > > > > > > /* clks not needed in SPL stage */ > > > > - > > > > clk_dm(IMX8MM_CLK_ENET1_ROOT, > > > > imx_clk_gate4("enet1_root_clk", "enet_axi", > > > > base + 0x40a0, 0)); > > > > endif > > > > > > > > > For that I continue to put here the implementation. After we can port > > > > > the clk dm part to manipulate clock in drivers/clk/imx/ DM driver. > > > > > What do you think about? Let me know. > > > > > > > > Seems like the clk_dm part is already in place and all you have to do is > > > > extend it. > > > > Hi Marek, > > I test the solution using DM model, it work: > > > > u-boot=> clk dump > > > > 24000000 2 | |-- pwm1 > > 24000000 3 | | `-- pwm1_root_clk > > 24000000 0 | |-- pwm2 > > 24000000 0 | | `-- pwm2_root_clk > > 24000000 0 | |-- pwm3 > > 24000000 0 | | `-- pwm3_root_clk > > 24000000 0 | |-- pwm4 > > 24000000 0 | | `-- pwm4_root_clk > > > > > > I test it using the following call on a dummy driver: > > > > ret = uclass_get_device_by_name(UCLASS_PWM, "pwm@30660000", &pwm); > > if (ret) > > printk("Failed to get pwm dev\n"); > > > > ret = clk_get_by_name(pwm, "per", &per_clk); > > if (ret) { > > printf("Failed to get per_clk\n"); > > return ret; > > } > > ret = clk_enable(&per_clk); > > if (ret) { > > printf("Failed to enable per_clk\n"); > > return ret; > > } > > > > ret = clk_get_by_name(pwm, "ipg", &ipg_clk); > > if (ret) { > > printf("Failed to get ipg_clk\n"); > > return ret; > > } > > ret = clk_enable(&ipg_clk); > > if (ret) { > > printf("Failed to enable ipg_clk\n"); > > return ret; > > } > > > > It's better to keep both the solutions or only > > based on DM model? I think we can put this initialization into > > drivers/pwm/pwm-imx.c imx_pwm_of_to_plat function. > > What do you think about? > > Let me know. > > DM is the preferred way, non-DM is fading away. Hi All, I'm working on pwm-imx drv, what do you think about move pwm-imx-utils and put all the 2 function inside pwm-imx.c under an ifndef CONFIG_DM_PWM. Using DM on pwm-imx make pwm-imx-utils.c no sense to exist. Example: #ifndef CONFIG_DM_PWM int pwm_imx_get_parms(int period_ns, int duty_ns, unsigned long *period_c, unsigned long *duty_c, unsigned long *prescale){ .... struct pwm_regs *pwm_id_to_reg(int pwm_id) { ... + all non dm implementation #else struct imx_pwm_priv { struct pwm_regs *regs; bool invert; struct clk *per_clk; struct clk *ipg_clk; }; int pwm_dm_imx_get_parms(struct imx_pwm_priv *priv, int period_ns, int duty_ns, unsigned long *period_c, unsigned long *duty_c, unsigned long *prescale) { .... + all dm implementation #endif What do you think about? Let me know. Thanks Tommaso
diff --git a/arch/arm/mach-imx/imx8m/clock_imx8mm.c b/arch/arm/mach-imx/imx8m/clock_imx8mm.c index 49945faf2c..ffb9456607 100644 --- a/arch/arm/mach-imx/imx8m/clock_imx8mm.c +++ b/arch/arm/mach-imx/imx8m/clock_imx8mm.c @@ -313,6 +313,59 @@ void enable_usboh3_clk(unsigned int enable) } } +void enable_pwm_clk(u32 index, unsigned char enable) +{ + switch (index) { + case 0: + if (enable) { + clock_enable(CCGR_PWM1, false); + clock_set_target_val(PWM1_CLK_ROOT, CLK_ROOT_ON | + CLK_ROOT_SOURCE_SEL(0) | + CLK_ROOT_PRE_DIV(CLK_ROOT_PRE_DIV1)); + clock_enable(CCGR_PWM1, true); + } else { + clock_enable(CCGR_PWM1, false); + } + return; + case 1: + if (enable) { + clock_enable(CCGR_PWM2, false); + clock_set_target_val(PWM2_CLK_ROOT, CLK_ROOT_ON | + CLK_ROOT_SOURCE_SEL(0) | + CLK_ROOT_PRE_DIV(CLK_ROOT_PRE_DIV1)); + clock_enable(CCGR_PWM2, true); + } else { + clock_enable(CCGR_PWM2, false); + } + return; + case 2: + if (enable) { + clock_enable(CCGR_PWM3, false); + clock_set_target_val(PWM3_CLK_ROOT, CLK_ROOT_ON | + CLK_ROOT_SOURCE_SEL(0) | + CLK_ROOT_PRE_DIV(CLK_ROOT_PRE_DIV1)); + clock_enable(CCGR_PWM3, true); + } else { + clock_enable(CCGR_PWM3, false); + } + return; + case 3: + if (enable) { + clock_enable(CCGR_PWM4, false); + clock_set_target_val(PWM4_CLK_ROOT, CLK_ROOT_ON | + CLK_ROOT_SOURCE_SEL(0) | + CLK_ROOT_PRE_DIV(CLK_ROOT_PRE_DIV1)); + clock_enable(CCGR_PWM4, true); + } else { + clock_enable(CCGR_PWM4, false); + } + return; + default: + printf("Invalid pwm index\n"); + return; + } +} + void init_uart_clk(u32 index) { /*
Add function enable_pwm_clk into in clock_imx8mm.c. This function first configure, then enable pwm clock from clock control register. The following configuration is used: source(0) -> 24 MHz ref clock div(0) -> no division for this clock References: - iMX8MMRM.pdf p 303 Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com> --- Changes since v1: - Fix enable_pwm_clk function implementation. Now is generic for all pwm clks arch/arm/mach-imx/imx8m/clock_imx8mm.c | 53 ++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)