Re: [PATCH] ARM: sa1100: convert to common clock framework

[Stephen Boyd <sboyd@kernel.org>]

Quoting Russell King - ARM Linux admin (2019-07-18 10:49:01)
> On Thu, Jul 18, 2019 at 09:38:08AM -0700, Stephen Boyd wrote:
> > Quoting Russell King (2019-06-29 03:14:10)
> > > Convert sa1100 to use the common clock framework.
> > > 
> > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > > ---
> > > Please ack; this is part of a larger series.  Thanks.
> > 
> > Just a few minor comments but otherwise looks good to me.
> > 
> > > diff --git a/arch/arm/mach-sa1100/clock.c b/arch/arm/mach-sa1100/clock.c
> > > index 6199e87447ca..523ef25618f7 100644
> > > --- a/arch/arm/mach-sa1100/clock.c
> > > +++ b/arch/arm/mach-sa1100/clock.c
> > > +static const char * const clk_tucr_parents[] = {
> > > +       "clk32768", "clk3686400",
> > >  };
> > 
> > It would be great if you used the new way of specifying clk parents with
> > direct pointers instead of strings. See commit fc0c209c147f ("clk: Allow
> > parents to be specified without string names") for some details.
> 
> I don't see at the moment how this is used with clk-mux.c - can you
> provide some hints?

In this case both the parents are clk_hw pointers I think so an array
where first element is the clk_hw pointer to clk32768 and the second
element is the clk_hw pointer to clk3686400 would be assigned to
clk_init_data's parent_hws member.


	struct clk_hw *clk_tucr_parents[] = {
		&clk32768_hw, 
		&clk3686400_hw,
	};

	clk_tucr_init.parent_hws = clk_tucr_parents;

> 
> > >  
> > > -static void clk_gpio27_enable(struct clk *clk)
> > > -{
> > >         /*
> > >          * First, set up the 3.6864MHz clock on GPIO 27 for the SA-1111:
> > >          * (SA-1110 Developer's Manual, section 9.1.2.1)
> > >          */
> > > +       local_irq_save(flags);
> > >         GAFR |= GPIO_32_768kHz;
> > >         GPDR |= GPIO_32_768kHz;
> > > -       TUCR = TUCR_3_6864MHz;
> > > +       local_irq_restore(flags);
> > > +
> > > +       return 0;
> > >  }
> > >  
> > > -static void clk_gpio27_disable(struct clk *clk)
> > > +static void clk_gpio27_disable(struct clk_hw *hw)
> > >  {
> > > -       TUCR = 0;
> > > +       unsigned long flags;
> > > +
> > > +       local_irq_save(flags);
> > 
> > Why just disable irqs here?
> 
> What do you mean?  Do you mean "why are you only disabling IRQs and not
> taking a spinlock" or do you mean "why are you disabling IRQs here" ?

I mean, why are you disabling irqs and not taking a spinlock? Must be
because there's already a spinlock in the clk framework?

> 
> > 
> > >         GPDR &= ~GPIO_32_768kHz;
> > >         GAFR &= ~GPIO_32_768kHz;
> > > +       local_irq_restore(flags);
> > >  }
> > >  
> > > -static void clk_cpu_enable(struct clk *clk)
> > > -{
> > > -}
> > > +static const struct clk_ops clk_gpio27_ops = {
> > > +       .enable = clk_gpio27_enable,
> > > +       .disable = clk_gpio27_disable,
> > > +};
> > >  
> > > -static void clk_cpu_disable(struct clk *clk)
> > > -{
> > > -}
> > > +static const char * const clk_gpio27_parents[] = {
> > > +       "tucr-mux",
> > > +};
> > >  
> > > -static unsigned long clk_cpu_get_rate(struct clk *clk)
> > > +static const struct clk_init_data clk_gpio27_init_data __initconst = {
> > > +       .name = "gpio27",
> > > +       .ops = &clk_gpio27_ops,
> > > +       .parent_names = clk_gpio27_parents,
> > > +       .num_parents = ARRAY_SIZE(clk_gpio27_parents),
> > > +       .flags = CLK_IS_BASIC,
> > 
> > CLK_IS_BASIC is gone. Please don't use it.
> 
> The patch is against 5.1, and you're right, so that was removed for the
> version that ended up going upstream.

Oh did this get sent to Linus already? I guess I should have reviewed
this earlier.

> 
> > 
> > > +};
> > > +
> > > +/*
> > > + * Derived from the table 8-1 in the SA1110 manual, the MPLL appears to
> > > + * multiply its input rate by 4 x (4 + PPCR).  This calculation gives
> > > + * the exact rate.  The figures given in the table are the rates rounded
> > > + * to 100kHz.  Stick with sa11x0_getspeed() for the time being.
> > [...]
> > > +static const struct clk_init_data clk_mpll_init_data __initconst = {
> > > +       .name = "mpll",
> > > +       .ops = &clk_mpll_ops,
> > > +       .parent_names = clk_mpll_parents,
> > > +       .num_parents = ARRAY_SIZE(clk_mpll_parents),
> > > +       .flags = CLK_IS_BASIC | CLK_GET_RATE_NOCACHE | CLK_IS_CRITICAL,
> > 
> > Please add a comment about these last two flags so we know why the rate
> > can't be cached and the clk is critical.
> 
> Ok, I'll do that with a follow-up patch once the merge window is over.
> 

Ok, thanks.