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

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Thu, Jul 18, 2019 at 11:43:07AM -0700, Stephen Boyd wrote:
> 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;

Thanks.

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

Nope - it's because there's no point taking a spinlock on something
that is fundamentally only a uniprocessor architecture.  There's never
going to be a SA11x0 compatible SoC that has more than one core.

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

Generally, SA11x0 stuff doesn't interest people, and patches I send out
don't attract comments - so I tend to wait a couple of weeks before
queuing them for merging.

It hasn't yet been merged, but is in the queue - arm-soc has taken it
into their late merges, but those haven't yet been sent.

> > > > +};
> > > > +
> > > > +/*
> > > > + * 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.
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up