[U-Boot] [PATCH 1/3] aspeed: ast2500: Add AHB clock

[Joel Stanley <joel@jms.id.au>]

On Wed, 12 Sep 2018 at 03:13, Cédric Le Goater <clg@kaod.org> wrote:
>
> On 09/11/2018 06:42 PM, Maxim Sloyko wrote:
> >
> >
> > On Tue, Sep 11, 2018 at 12:35 AM, Joel Stanley <joel at jms.id.au <mailto:joel@jms.id.au>> wrote:
> >
> >     On Mon, 10 Sep 2018 at 23:48, Cédric Le Goater <clg at kaod.org <mailto:clg@kaod.org>> wrote:
> >     >
> >     > The AHB clock is used by the FMC/SPI controllers.
> >     >
> >     > Signed-off-by: Cédric Le Goater <clg at kaod.org <mailto:clg@kaod.org>>
> >     > ---
> >     >  arch/arm/include/asm/arch-aspeed/scu_ast2500.h |  2 ++
> >     >  include/dt-bindings/clock/ast2500-scu.h        |  1 +
> >     >  drivers/clk/aspeed/clk_ast2500.c               | 12 ++++++++++++
> >     >  3 files changed, 15 insertions(+)
> >     >
> >     > diff --git a/arch/arm/include/asm/arch-aspeed/scu_ast2500.h b/arch/arm/include/asm/arch-aspeed/scu_ast2500.h
> >     > index 4988ced7ddcc..6a90ded752ad 100644
> >     > --- a/arch/arm/include/asm/arch-aspeed/scu_ast2500.h
> >     > +++ b/arch/arm/include/asm/arch-aspeed/scu_ast2500.h
> >     > @@ -11,6 +11,8 @@
> >     >  #define SCU_HWSTRAP_VGAMEM_MASK                (3 << SCU_HWSTRAP_VGAMEM_SHIFT)
> >     >  #define SCU_HWSTRAP_MAC1_RGMII         (1 << 6)
> >     >  #define SCU_HWSTRAP_MAC2_RGMII         (1 << 7)
> >     > +#define SCU_HWSTRAP_AXIAHB_DIV_SHIFT   9
> >     > +#define SCU_HWSTRAP_AXIAHB_DIV_MASK    (0x7 << SCU_HWSTRAP_AXIAHB_DIV_SHIFT)
> >     >  #define SCU_HWSTRAP_DDR4               (1 << 24)
> >     >  #define SCU_HWSTRAP_CLKIN_25MHZ                (1 << 23)
> >     >
> >     > diff --git a/include/dt-bindings/clock/ast2500-scu.h b/include/dt-bindings/clock/ast2500-scu.h
> >     > index 4803abe9f628..03e6d16d3de0 100644
> >     > --- a/include/dt-bindings/clock/ast2500-scu.h
> >     > +++ b/include/dt-bindings/clock/ast2500-scu.h
> >     > @@ -17,6 +17,7 @@
> >     >  #define BCLK_MACCLK    103
> >     >  #define BCLK_SDCLK     104
> >     >  #define BCLK_ARMCLK    105
> >     > +#define BCLK_HCLK      106
> >
> >     I like how the clocks are grouped in this file. Are we confident that
> >     HCLK is going in the correct spot?
> >
> >     >  #define MCLK_DDR       201
> >     >
> >     > diff --git a/drivers/clk/aspeed/clk_ast2500.c b/drivers/clk/aspeed/clk_ast2500.c
> >     > index 526470051c5d..c55f8d5ae30d 100644
> >     > --- a/drivers/clk/aspeed/clk_ast2500.c
> >     > +++ b/drivers/clk/aspeed/clk_ast2500.c
> >     > @@ -143,6 +143,18 @@ static ulong ast2500_clk_get_rate(struct clk *clk)
> >     >                         rate = rate / apb_div;
> >     >                 }
> >     >                 break;
> >     > +       case BCLK_HCLK:
> >     > +               {
> >     > +                       ulong ahb_div = 1 + ((readl(&priv->scu->hwstrap)
> >     > +                                             & SCU_HWSTRAP_AXIAHB_DIV_MASK)
> >     > +                                            >> SCU_HWSTRAP_AXIAHB_DIV_SHIFT);
> >     > +                       ulong axi_div = 2;
> >     > +
> >     > +                       rate = ast2500_get_hpll_rate(
> >     > +                               clkin, readl(&priv->scu->h_pll_param));
> >     > +                       rate = rate / axi_div / ahb_div;
> >
> >     In the kernel driver I wrote it as:
> >
> >      rate / (axi_div + ahb_div)
> >
> >
> > These are two different formulae -- just want to make sure that the typo only made it into an email :)
> >
> > In any case, the exact right way to do this computation depends on how this is implemented in the hardware itself. Most likely it's to dividers in sequence, so dividing twice should be more accurate. Of course it would be nice if somebody with hw design experience could comment.
> >
> > If the datasheet has formula, I think the right way is to use it exactly as stated.
>
> it's two dividers in sequence in the datasheet. In the SDK also.
>
> >
> >
> >     I know that the maths works, but do the numbers come out as we expect
> >     when doing integer division?
>
> Yes. 192MHz.

Thanks for clarifying.

Reviewed-by: Joel Stanley <joel@jms.id.au>