Re: [PATCH v2 4/9] arm: imx: imx8mm: add enable_pwm_clk function

From: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
To: Michael Nazzareno Trimarchi <michael@amarulasolutions.com>
Cc: "Stefano Babic" <sbabic@denx.de>,
	"Fabio Estevam" <festevam@gmail.com>,
	"NXP i.MX U-Boot Team" <uboot-imx@nxp.com>,
	"Peng Fan" <peng.fan@nxp.com>, "Ye Li" <ye.li@nxp.com>,
	"Alice Guo" <alice.guo@nxp.com>,
	"Andrey Zhizhikin" <andrey.zhizhikin@leica-geosystems.com>,
	"Marek Vasut" <marex@denx.de>, "Marek Behún" <marek.behun@nic.cz>,
	u-boot@lists.denx.de
Subject: Re: [PATCH v2 4/9] arm: imx: imx8mm: add enable_pwm_clk function
Date: Thu, 17 Mar 2022 08:40:53 +0100	[thread overview]
Message-ID: <20220317074053.GD29139@tom-ThinkPad-T14s-Gen-2i> (raw)
In-Reply-To: <CAOf5uwm0mB28-tAYK8BNONc=P5_A-=dOZKXfWzgAPadQL6AS9g@mail.gmail.com>

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

-- 
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