diff mbox

[2/3] aspeed/g5: helper SCU functions for configuring aspeed I2C

Message ID 1474998214-12720-2-git-send-email-maxims@google.com
State Changes Requested, archived
Headers show

Commit Message

Maxim Sloyko Sept. 27, 2016, 5:43 p.m. UTC
From: Maxim Sloyko <maxims@google.com>

---
 arch/arm/include/asm/arch-aspeed/ast_scu.h  |  6 +++
 arch/arm/include/asm/arch-aspeed/regs-scu.h | 74 ++++++++++++++++-------------
 arch/arm/mach-aspeed/ast-scu.c              | 30 +++++++++++-
 3 files changed, 75 insertions(+), 35 deletions(-)

Comments

Joel Stanley Sept. 29, 2016, 1:03 a.m. UTC | #1
On Wed, Sep 28, 2016 at 3:13 AM,  <maxims@google.com> wrote:

You need a commit message.

> From: Maxim Sloyko <maxims@google.com>
>
> ---
>  arch/arm/include/asm/arch-aspeed/ast_scu.h  |  6 +++
>  arch/arm/include/asm/arch-aspeed/regs-scu.h | 74 ++++++++++++++++-------------

Please put the reformatting of header files in a seperate patch.

>  arch/arm/mach-aspeed/ast-scu.c              | 30 +++++++++++-
>  3 files changed, 75 insertions(+), 35 deletions(-)
>
> diff --git a/arch/arm/include/asm/arch-aspeed/ast_scu.h b/arch/arm/include/asm/arch-aspeed/ast_scu.h
> index d248416..80ebd6f 100644
> --- a/arch/arm/include/asm/arch-aspeed/ast_scu.h
> +++ b/arch/arm/include/asm/arch-aspeed/ast_scu.h
> @@ -38,6 +38,7 @@ extern void ast_scu_get_who_init_dram(void);
>  extern u32 ast_get_clk_source(void);
>  extern u32 ast_get_h_pll_clk(void);
>  extern u32 ast_get_ahbclk(void);
> +extern u32 ast_get_apbclk(void);
>
>  extern u32 ast_scu_get_vga_memsize(void);
>
> @@ -45,4 +46,9 @@ extern void ast_scu_init_eth(u8 num);
>  extern void ast_scu_multi_func_eth(u8 num);
>  extern void ast_scu_multi_func_romcs(u8 num);
>
> +/* Enable I2C controller and pins for a particular device.
> + * Device numbering starts at 1
> + */
> +extern void ast_scu_enable_i2c(u8 num);
> +
>  #endif
> diff --git a/arch/arm/include/asm/arch-aspeed/regs-scu.h b/arch/arm/include/asm/arch-aspeed/regs-scu.h
> index b89df82..92ce84a 100644
> --- a/arch/arm/include/asm/arch-aspeed/regs-scu.h
> +++ b/arch/arm/include/asm/arch-aspeed/regs-scu.h
> @@ -10,8 +10,8 @@
>   *    1. 2012/12/29 Ryan Chen Create
>   *
>  ********************************************************************************/
> -#ifndef __AST_SCU_H
> -#define __AST_SCU_H                     1
> +#ifndef __AST_REGS_SCU_H
> +#define __AST_REGS_SCU_H                     1
>
>  #include <asm/arch/aspeed.h>
>
> @@ -830,49 +830,50 @@
>  /* AST_SCU_FUN_PIN_CTRL5               0x90 - Multi-function Pin Control#5 */
>  #define SCU_FUN_PIN_SPICS1             (0x1 << 31)
>  #define SCU_FUN_PIN_LPC_PLUS           (0x1 << 30)
> -#define SCU_FUC_PIN_USB20_HOST         (0x1 << 29)
> -#define SCU_FUC_PIN_USB11_PORT4                (0x1 << 28)
> -#define SCU_FUC_PIN_I2C14              (0x1 << 27)
> -#define SCU_FUC_PIN_I2C13              (0x1 << 26)
> -#define SCU_FUC_PIN_I2C12              (0x1 << 25)
> -#define SCU_FUC_PIN_I2C11              (0x1 << 24)
> -#define SCU_FUC_PIN_I2C10              (0x1 << 23)
> -#define SCU_FUC_PIN_I2C9               (0x1 << 22)
> -#define SCU_FUC_PIN_I2C8               (0x1 << 21)
> -#define SCU_FUC_PIN_I2C7               (0x1 << 20)
> -#define SCU_FUC_PIN_I2C6               (0x1 << 19)
> -#define SCU_FUC_PIN_I2C5               (0x1 << 18)
> -#define SCU_FUC_PIN_I2C4               (0x1 << 17)
> -#define SCU_FUC_PIN_I2C3               (0x1 << 16)
> -#define SCU_FUC_PIN_MII2_RX_DWN_DIS    (0x1 << 15)
> -#define SCU_FUC_PIN_MII2_TX_DWN_DIS    (0x1 << 14)
> -#define SCU_FUC_PIN_MII1_RX_DWN_DIS    (0x1 << 13)
> -#define SCU_FUC_PIN_MII1_TX_DWN_DIS    (0x1 << 12)
> -
> -#define SCU_FUC_PIN_MII2_TX_DRIV(x)    (x << 10)
> -#define SCU_FUC_PIN_MII2_TX_DRIV_MASK  (0x3 << 10)
> -#define SCU_FUC_PIN_MII1_TX_DRIV(x)    (x << 8)
> -#define SCU_FUC_PIN_MII1_TX_DRIV_MASK  (0x3 << 8)
> +#define SCU_FUN_PIN_USB20_HOST         (0x1 << 29)
> +#define SCU_FUN_PIN_USB11_PORT4                (0x1 << 28)
> +#define SCU_FUN_PIN_I2C14              (0x1 << 27)
> +#define SCU_FUN_PIN_I2C13              (0x1 << 26)
> +#define SCU_FUN_PIN_I2C12              (0x1 << 25)
> +#define SCU_FUN_PIN_I2C11              (0x1 << 24)
> +#define SCU_FUN_PIN_I2C10              (0x1 << 23)
> +#define SCU_FUN_PIN_I2C9               (0x1 << 22)
> +#define SCU_FUN_PIN_I2C8               (0x1 << 21)
> +#define SCU_FUN_PIN_I2C7               (0x1 << 20)
> +#define SCU_FUN_PIN_I2C6               (0x1 << 19)
> +#define SCU_FUN_PIN_I2C5               (0x1 << 18)
> +#define SCU_FUN_PIN_I2C4               (0x1 << 17)
> +#define SCU_FUN_PIN_I2C3               (0x1 << 16)
> +#define SCU_FUN_PIN_I2C(n)             (0x1 << (16 + (n) - 3))
> +#define SCU_FUN_PIN_MII2_RX_DWN_DIS    (0x1 << 15)
> +#define SCU_FUN_PIN_MII2_TX_DWN_DIS    (0x1 << 14)
> +#define SCU_FUN_PIN_MII1_RX_DWN_DIS    (0x1 << 13)
> +#define SCU_FUN_PIN_MII1_TX_DWN_DIS    (0x1 << 12)

It looks like you're just changing the SCU_FUC_ to SCU_FUN. Without a
commit message I'm not sure why you're doing this, can you explain
why?

I don't think it is required.

If you were to make a change like this you should perform the renaming
and update the call sites in a separate patch to any other changes.
This makes it easier to review.


> diff --git a/arch/arm/mach-aspeed/ast-scu.c b/arch/arm/mach-aspeed/ast-scu.c
> index 0cc0d67..1452b93 100644
> --- a/arch/arm/mach-aspeed/ast-scu.c
> +++ b/arch/arm/mach-aspeed/ast-scu.c
> @@ -318,6 +318,14 @@ u32 ast_get_ahbclk(void)
>
>  #endif /* AST_SOC_G5 */
>
> +u32 ast_get_apbclk(void)
> +{
> +       u32 h_pll = ast_get_h_pll_clk();
> +       u32 apb_div = SCU_GET_PCLK_DIV(ast_scu_read(AST_SCU_CLK_SEL));
> +       return h_pll / apb_div;
> +}

This will do for now. We suspect need to write a clock driver that
will live in drivers/clk/ to do it properly.

Can apb_div ever be zero?

> +
> +
>  void ast_scu_show_system_info(void)
>  {
>
> @@ -394,7 +402,7 @@ void ast_scu_multi_func_eth(u8 num)
>                               AST_SCU_FUN_PIN_CTRL1);
>
>                 ast_scu_write(ast_scu_read(AST_SCU_FUN_PIN_CTRL5) |
> -                             SCU_FUC_PIN_MAC1_MDIO,
> +                             SCU_FUN_PIN_MAC1_MDIO,
>                               AST_SCU_FUN_PIN_CTRL5);
>
>                 break;
> @@ -496,3 +504,23 @@ void ast_scu_get_who_init_dram(void)
>                 break;
>         }
>  }
> +
> +void ast_scu_enable_i2c(u8 num)
> +{

Is num the bus? Perhaps a better name would be "bus_enable" or similar.

I think you mentioned that num must be > 0. If so, check for zero, log
an error and return.


> +       /* Enable I2C Controllers */
> +       clrbits_le32(AST_SCU_BASE + AST_SCU_RESET, SCU_RESET_I2C);
> +
> +       if (num < 3) {

How do we enable buses number 1 and 2 when AST_SOC_G5 is not set?

> +#ifdef AST_SOC_G5
> +               if (num == 1) {
> +                       setbits_le32(AST_SCU_BASE + AST_SCU_FUN_PIN_CTRL8,
> +                                                SCU_FUN_PIN_SDA1 | SCU_FUN_PIN_SCL1);
> +               } else {

Make the else check for num == 2, as that's what you're looking for.

> +                       setbits_le32(AST_SCU_BASE + AST_SCU_FUN_PIN_CTRL8,
> +                                                SCU_FUN_PIN_SDA2 | SCU_FUN_PIN_SCL2);
> +               }
> +#endif
> +       } else {
> +               setbits_le32(AST_SCU_BASE + AST_SCU_FUN_PIN_CTRL5, SCU_FUN_PIN_I2C(num));
> +       }


> +}
> --
> 2.8.0.rc3.226.g39d4020
>
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc
Maxim Sloyko Sept. 29, 2016, 6:23 p.m. UTC | #2
On Wed, Sep 28, 2016 at 6:03 PM, Joel Stanley <joel@jms.id.au> wrote:

> On Wed, Sep 28, 2016 at 3:13 AM,  <maxims@google.com> wrote:
>
> You need a commit message.
>
> > From: Maxim Sloyko <maxims@google.com>
> >
> > ---
> >  arch/arm/include/asm/arch-aspeed/ast_scu.h  |  6 +++
> >  arch/arm/include/asm/arch-aspeed/regs-scu.h | 74
> ++++++++++++++++-------------
>
> Please put the reformatting of header files in a seperate patch.
>

What do you mean by reformatting?


>
> >  arch/arm/mach-aspeed/ast-scu.c              | 30 +++++++++++-
> >  3 files changed, 75 insertions(+), 35 deletions(-)
> >
> > diff --git a/arch/arm/include/asm/arch-aspeed/ast_scu.h
> b/arch/arm/include/asm/arch-aspeed/ast_scu.h
> > index d248416..80ebd6f 100644
> > --- a/arch/arm/include/asm/arch-aspeed/ast_scu.h
> > +++ b/arch/arm/include/asm/arch-aspeed/ast_scu.h
> > @@ -38,6 +38,7 @@ extern void ast_scu_get_who_init_dram(void);
> >  extern u32 ast_get_clk_source(void);
> >  extern u32 ast_get_h_pll_clk(void);
> >  extern u32 ast_get_ahbclk(void);
> > +extern u32 ast_get_apbclk(void);
> >
> >  extern u32 ast_scu_get_vga_memsize(void);
> >
> > @@ -45,4 +46,9 @@ extern void ast_scu_init_eth(u8 num);
> >  extern void ast_scu_multi_func_eth(u8 num);
> >  extern void ast_scu_multi_func_romcs(u8 num);
> >
> > +/* Enable I2C controller and pins for a particular device.
> > + * Device numbering starts at 1
> > + */
> > +extern void ast_scu_enable_i2c(u8 num);
> > +
> >  #endif
> > diff --git a/arch/arm/include/asm/arch-aspeed/regs-scu.h
> b/arch/arm/include/asm/arch-aspeed/regs-scu.h
> > index b89df82..92ce84a 100644
> > --- a/arch/arm/include/asm/arch-aspeed/regs-scu.h
> > +++ b/arch/arm/include/asm/arch-aspeed/regs-scu.h
> > @@ -10,8 +10,8 @@
> >   *    1. 2012/12/29 Ryan Chen Create
> >   *
> >  ************************************************************
> ********************/
> > -#ifndef __AST_SCU_H
> > -#define __AST_SCU_H                     1
> > +#ifndef __AST_REGS_SCU_H
> > +#define __AST_REGS_SCU_H                     1
> >
> >  #include <asm/arch/aspeed.h>
> >
> > @@ -830,49 +830,50 @@
> >  /* AST_SCU_FUN_PIN_CTRL5               0x90 - Multi-function Pin
> Control#5 */
> >  #define SCU_FUN_PIN_SPICS1             (0x1 << 31)
> >  #define SCU_FUN_PIN_LPC_PLUS           (0x1 << 30)
> > -#define SCU_FUC_PIN_USB20_HOST         (0x1 << 29)
> > -#define SCU_FUC_PIN_USB11_PORT4                (0x1 << 28)
> > -#define SCU_FUC_PIN_I2C14              (0x1 << 27)
> > -#define SCU_FUC_PIN_I2C13              (0x1 << 26)
> > -#define SCU_FUC_PIN_I2C12              (0x1 << 25)
> > -#define SCU_FUC_PIN_I2C11              (0x1 << 24)
> > -#define SCU_FUC_PIN_I2C10              (0x1 << 23)
> > -#define SCU_FUC_PIN_I2C9               (0x1 << 22)
> > -#define SCU_FUC_PIN_I2C8               (0x1 << 21)
> > -#define SCU_FUC_PIN_I2C7               (0x1 << 20)
> > -#define SCU_FUC_PIN_I2C6               (0x1 << 19)
> > -#define SCU_FUC_PIN_I2C5               (0x1 << 18)
> > -#define SCU_FUC_PIN_I2C4               (0x1 << 17)
> > -#define SCU_FUC_PIN_I2C3               (0x1 << 16)
> > -#define SCU_FUC_PIN_MII2_RX_DWN_DIS    (0x1 << 15)
> > -#define SCU_FUC_PIN_MII2_TX_DWN_DIS    (0x1 << 14)
> > -#define SCU_FUC_PIN_MII1_RX_DWN_DIS    (0x1 << 13)
> > -#define SCU_FUC_PIN_MII1_TX_DWN_DIS    (0x1 << 12)
> > -
> > -#define SCU_FUC_PIN_MII2_TX_DRIV(x)    (x << 10)
> > -#define SCU_FUC_PIN_MII2_TX_DRIV_MASK  (0x3 << 10)
> > -#define SCU_FUC_PIN_MII1_TX_DRIV(x)    (x << 8)
> > -#define SCU_FUC_PIN_MII1_TX_DRIV_MASK  (0x3 << 8)
> > +#define SCU_FUN_PIN_USB20_HOST         (0x1 << 29)
> > +#define SCU_FUN_PIN_USB11_PORT4                (0x1 << 28)
> > +#define SCU_FUN_PIN_I2C14              (0x1 << 27)
> > +#define SCU_FUN_PIN_I2C13              (0x1 << 26)
> > +#define SCU_FUN_PIN_I2C12              (0x1 << 25)
> > +#define SCU_FUN_PIN_I2C11              (0x1 << 24)
> > +#define SCU_FUN_PIN_I2C10              (0x1 << 23)
> > +#define SCU_FUN_PIN_I2C9               (0x1 << 22)
> > +#define SCU_FUN_PIN_I2C8               (0x1 << 21)
> > +#define SCU_FUN_PIN_I2C7               (0x1 << 20)
> > +#define SCU_FUN_PIN_I2C6               (0x1 << 19)
> > +#define SCU_FUN_PIN_I2C5               (0x1 << 18)
> > +#define SCU_FUN_PIN_I2C4               (0x1 << 17)
> > +#define SCU_FUN_PIN_I2C3               (0x1 << 16)
> > +#define SCU_FUN_PIN_I2C(n)             (0x1 << (16 + (n) - 3))
> > +#define SCU_FUN_PIN_MII2_RX_DWN_DIS    (0x1 << 15)
> > +#define SCU_FUN_PIN_MII2_TX_DWN_DIS    (0x1 << 14)
> > +#define SCU_FUN_PIN_MII1_RX_DWN_DIS    (0x1 << 13)
> > +#define SCU_FUN_PIN_MII1_TX_DWN_DIS    (0x1 << 12)
>
> It looks like you're just changing the SCU_FUC_ to SCU_FUN. Without a
> commit message I'm not sure why you're doing this, can you explain
> why?
>
> I don't think it is required.
>
> If you were to make a change like this you should perform the renaming
> and update the call sites in a separate patch to any other changes.
> This makes it easier to review.
>

Because that's a typo that got out of hand. FUN supposed to stand for
FUNCTION,
FUC ... no guesses.


>
>
> > diff --git a/arch/arm/mach-aspeed/ast-scu.c b/arch/arm/mach-aspeed/ast-
> scu.c
> > index 0cc0d67..1452b93 100644
> > --- a/arch/arm/mach-aspeed/ast-scu.c
> > +++ b/arch/arm/mach-aspeed/ast-scu.c
> > @@ -318,6 +318,14 @@ u32 ast_get_ahbclk(void)
> >
> >  #endif /* AST_SOC_G5 */
> >
> > +u32 ast_get_apbclk(void)
> > +{
> > +       u32 h_pll = ast_get_h_pll_clk();
> > +       u32 apb_div = SCU_GET_PCLK_DIV(ast_scu_read(AST_SCU_CLK_SEL));
> > +       return h_pll / apb_div;
> > +}
>
> This will do for now. We suspect need to write a clock driver that
> will live in drivers/clk/ to do it properly.
>
> Can apb_div ever be zero?
>
>
Oh, this is actually wrong... Fixed.


> > +
> > +
> >  void ast_scu_show_system_info(void)
> >  {
> >
> > @@ -394,7 +402,7 @@ void ast_scu_multi_func_eth(u8 num)
> >                               AST_SCU_FUN_PIN_CTRL1);
> >
> >                 ast_scu_write(ast_scu_read(AST_SCU_FUN_PIN_CTRL5) |
> > -                             SCU_FUC_PIN_MAC1_MDIO,
> > +                             SCU_FUN_PIN_MAC1_MDIO,
> >                               AST_SCU_FUN_PIN_CTRL5);
> >
> >                 break;
> > @@ -496,3 +504,23 @@ void ast_scu_get_who_init_dram(void)
> >                 break;
> >         }
> >  }
> > +
> > +void ast_scu_enable_i2c(u8 num)
> > +{
>
> Is num the bus? Perhaps a better name would be "bus_enable" or similar.
>
> I think you mentioned that num must be > 0. If so, check for zero, log
> an error and return.
>
>
> > +       /* Enable I2C Controllers */
> > +       clrbits_le32(AST_SCU_BASE + AST_SCU_RESET, SCU_RESET_I2C);
> > +
> > +       if (num < 3) {
>
> How do we enable buses number 1 and 2 when AST_SOC_G5 is not set?
>

I don't know. My guess is we don't, as in, the pins are always assigned
these functions. In datasheet these bits in PIN CTRL8 register are marked
as "new in ast2500" and there is no mention of sda{1,2},scl{1,2} in ast2500
datasheet.
Also, this part is basically taken from aspeed's sdk, this is why I assume
they are always on.


>
> > +#ifdef AST_SOC_G5
> > +               if (num == 1) {
> > +                       setbits_le32(AST_SCU_BASE +
> AST_SCU_FUN_PIN_CTRL8,
> > +                                                SCU_FUN_PIN_SDA1 |
> SCU_FUN_PIN_SCL1);
> > +               } else {
>
> Make the else check for num == 2, as that's what you're looking for.
>
> > +                       setbits_le32(AST_SCU_BASE +
> AST_SCU_FUN_PIN_CTRL8,
> > +                                                SCU_FUN_PIN_SDA2 |
> SCU_FUN_PIN_SCL2);
> > +               }
> > +#endif
> > +       } else {
> > +               setbits_le32(AST_SCU_BASE + AST_SCU_FUN_PIN_CTRL5,
> SCU_FUN_PIN_I2C(num));
> > +       }
>
>
> > +}
> > --
> > 2.8.0.rc3.226.g39d4020
> >
> > _______________________________________________
> > openbmc mailing list
> > openbmc@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/openbmc
>
Andrew Jeffery Sept. 30, 2016, 1:49 a.m. UTC | #3
On Thu, 2016-09-29 at 11:23 -0700, Maxim Sloyko wrote:
> > > +       /* Enable I2C Controllers */
> > > +       clrbits_le32(AST_SCU_BASE + AST_SCU_RESET, SCU_RESET_I2C);
> > > +
> > > +       if (num < 3) {
> > 
> > How do we enable buses number 1 and 2 when AST_SOC_G5 is not set?
> I don't know. My guess is we don't, as in, the pins are always
> assigned these functions. In datasheet these bits in PIN CTRL8
> register are marked as "new in ast2500" and there is no mention of
> sda{1,2},scl{1,2} in ast2500 datasheet.
> Also, this part is basically taken from aspeed's sdk, this is why I
> assume they are always on.

So to clarify, the AST2400 has (SCL1, SDA1) and (SCL2, SDA2) on fixed
function pins: (K21, K22) and (J19, J18) respectively. I don't have the
datasheets for earlier releases of the AST SoCs, so can't speak for
them. The AST2500 has (SCL1, SDA1) and (SCL2, SDA2) on multi-function
pins: (M18, M19) and (M20, P20) respectively.

As an aside: In AST2500 case I2Cs 1 and 2 are only multiplexed with
GPIO and are at a higher priority, so we don't need to flip any other
bits to enable I2C.

Maybe there should be some comments along these given the code looks
odd at first glance?

Cheers,

Andrew

Separately: This code makes me glad I took a data-driven approach for
the Linux pinmux driver (not to say that result is beautiful either,
but I don't want even begin to imagine the maze of conditionals
protecting corner cases).
Maxim Sloyko Oct. 3, 2016, 6:57 p.m. UTC | #4
On Thu, Sep 29, 2016 at 6:49 PM, Andrew Jeffery <andrew@aj.id.au> wrote:

> On Thu, 2016-09-29 at 11:23 -0700, Maxim Sloyko wrote:
> > > > +       /* Enable I2C Controllers */
> > > > +       clrbits_le32(AST_SCU_BASE + AST_SCU_RESET, SCU_RESET_I2C);
> > > > +
> > > > +       if (num < 3) {
> > >
> > > How do we enable buses number 1 and 2 when AST_SOC_G5 is not set?
> > I don't know. My guess is we don't, as in, the pins are always
> > assigned these functions. In datasheet these bits in PIN CTRL8
> > register are marked as "new in ast2500" and there is no mention of
> > sda{1,2},scl{1,2} in ast2500 datasheet.
> > Also, this part is basically taken from aspeed's sdk, this is why I
> > assume they are always on.
>
> So to clarify, the AST2400 has (SCL1, SDA1) and (SCL2, SDA2) on fixed
> function pins: (K21, K22) and (J19, J18) respectively. I don't have the
> datasheets for earlier releases of the AST SoCs, so can't speak for
> them. The AST2500 has (SCL1, SDA1) and (SCL2, SDA2) on multi-function
> pins: (M18, M19) and (M20, P20) respectively.
>
> As an aside: In AST2500 case I2Cs 1 and 2 are only multiplexed with
> GPIO and are at a higher priority, so we don't need to flip any other
> bits to enable I2C.
>
> Maybe there should be some comments along these given the code looks
> odd at first glance?
>

Added comment.


>
> Cheers,
>
> Andrew
>
> Separately: This code makes me glad I took a data-driven approach for
> the Linux pinmux driver (not to say that result is beautiful either,
> but I don't want even begin to imagine the maze of conditionals
> protecting corner cases).
diff mbox

Patch

diff --git a/arch/arm/include/asm/arch-aspeed/ast_scu.h b/arch/arm/include/asm/arch-aspeed/ast_scu.h
index d248416..80ebd6f 100644
--- a/arch/arm/include/asm/arch-aspeed/ast_scu.h
+++ b/arch/arm/include/asm/arch-aspeed/ast_scu.h
@@ -38,6 +38,7 @@  extern void ast_scu_get_who_init_dram(void);
 extern u32 ast_get_clk_source(void);
 extern u32 ast_get_h_pll_clk(void);
 extern u32 ast_get_ahbclk(void);
+extern u32 ast_get_apbclk(void);
 
 extern u32 ast_scu_get_vga_memsize(void);
 
@@ -45,4 +46,9 @@  extern void ast_scu_init_eth(u8 num);
 extern void ast_scu_multi_func_eth(u8 num);
 extern void ast_scu_multi_func_romcs(u8 num);
 
+/* Enable I2C controller and pins for a particular device.
+ * Device numbering starts at 1
+ */
+extern void ast_scu_enable_i2c(u8 num);
+
 #endif
diff --git a/arch/arm/include/asm/arch-aspeed/regs-scu.h b/arch/arm/include/asm/arch-aspeed/regs-scu.h
index b89df82..92ce84a 100644
--- a/arch/arm/include/asm/arch-aspeed/regs-scu.h
+++ b/arch/arm/include/asm/arch-aspeed/regs-scu.h
@@ -10,8 +10,8 @@ 
  *    1. 2012/12/29 Ryan Chen Create
  *
 ********************************************************************************/
-#ifndef __AST_SCU_H
-#define __AST_SCU_H                     1
+#ifndef __AST_REGS_SCU_H
+#define __AST_REGS_SCU_H                     1
 
 #include <asm/arch/aspeed.h>
 
@@ -830,49 +830,50 @@ 
 /* AST_SCU_FUN_PIN_CTRL5		0x90 - Multi-function Pin Control#5 */
 #define SCU_FUN_PIN_SPICS1		(0x1 << 31)
 #define SCU_FUN_PIN_LPC_PLUS		(0x1 << 30)
-#define SCU_FUC_PIN_USB20_HOST		(0x1 << 29)
-#define SCU_FUC_PIN_USB11_PORT4		(0x1 << 28)
-#define SCU_FUC_PIN_I2C14		(0x1 << 27)
-#define SCU_FUC_PIN_I2C13		(0x1 << 26)
-#define SCU_FUC_PIN_I2C12		(0x1 << 25)
-#define SCU_FUC_PIN_I2C11		(0x1 << 24)
-#define SCU_FUC_PIN_I2C10		(0x1 << 23)
-#define SCU_FUC_PIN_I2C9		(0x1 << 22)
-#define SCU_FUC_PIN_I2C8		(0x1 << 21)
-#define SCU_FUC_PIN_I2C7		(0x1 << 20)
-#define SCU_FUC_PIN_I2C6		(0x1 << 19)
-#define SCU_FUC_PIN_I2C5		(0x1 << 18)
-#define SCU_FUC_PIN_I2C4		(0x1 << 17)
-#define SCU_FUC_PIN_I2C3		(0x1 << 16)
-#define SCU_FUC_PIN_MII2_RX_DWN_DIS	(0x1 << 15)
-#define SCU_FUC_PIN_MII2_TX_DWN_DIS	(0x1 << 14)
-#define SCU_FUC_PIN_MII1_RX_DWN_DIS	(0x1 << 13)
-#define SCU_FUC_PIN_MII1_TX_DWN_DIS	(0x1 << 12)
-
-#define SCU_FUC_PIN_MII2_TX_DRIV(x)	(x << 10)
-#define SCU_FUC_PIN_MII2_TX_DRIV_MASK	(0x3 << 10)
-#define SCU_FUC_PIN_MII1_TX_DRIV(x)	(x << 8)
-#define SCU_FUC_PIN_MII1_TX_DRIV_MASK	(0x3 << 8)
+#define SCU_FUN_PIN_USB20_HOST		(0x1 << 29)
+#define SCU_FUN_PIN_USB11_PORT4		(0x1 << 28)
+#define SCU_FUN_PIN_I2C14		(0x1 << 27)
+#define SCU_FUN_PIN_I2C13		(0x1 << 26)
+#define SCU_FUN_PIN_I2C12		(0x1 << 25)
+#define SCU_FUN_PIN_I2C11		(0x1 << 24)
+#define SCU_FUN_PIN_I2C10		(0x1 << 23)
+#define SCU_FUN_PIN_I2C9		(0x1 << 22)
+#define SCU_FUN_PIN_I2C8		(0x1 << 21)
+#define SCU_FUN_PIN_I2C7		(0x1 << 20)
+#define SCU_FUN_PIN_I2C6		(0x1 << 19)
+#define SCU_FUN_PIN_I2C5		(0x1 << 18)
+#define SCU_FUN_PIN_I2C4		(0x1 << 17)
+#define SCU_FUN_PIN_I2C3		(0x1 << 16)
+#define SCU_FUN_PIN_I2C(n)		(0x1 << (16 + (n) - 3))
+#define SCU_FUN_PIN_MII2_RX_DWN_DIS	(0x1 << 15)
+#define SCU_FUN_PIN_MII2_TX_DWN_DIS	(0x1 << 14)
+#define SCU_FUN_PIN_MII1_RX_DWN_DIS	(0x1 << 13)
+#define SCU_FUN_PIN_MII1_TX_DWN_DIS	(0x1 << 12)
+
+#define SCU_FUN_PIN_MII2_TX_DRIV(x)	(x << 10)
+#define SCU_FUN_PIN_MII2_TX_DRIV_MASK	(0x3 << 10)
+#define SCU_FUN_PIN_MII1_TX_DRIV(x)	(x << 8)
+#define SCU_FUN_PIN_MII1_TX_DRIV_MASK	(0x3 << 8)
 
 #define MII_NORMAL_DRIV			0x0
 #define MII_HIGH_DRIV			0x2
 
-#define SCU_FUC_PIN_UART6		(0x1 << 7)
-#define SCU_FUC_PIN_ROM_16BIT		(0x1 << 6)
-#define SCU_FUC_PIN_DIGI_V_OUT(x)	(x)
-#define SCU_FUC_PIN_DIGI_V_OUT_MASK	(0x3)
+#define SCU_FUN_PIN_UART6		(0x1 << 7)
+#define SCU_FUN_PIN_ROM_16BIT		(0x1 << 6)
+#define SCU_FUN_PIN_DIGI_V_OUT(x)	(x)
+#define SCU_FUN_PIN_DIGI_V_OUT_MASK	(0x3)
 
 #define VIDEO_DISABLE			0x0
 #define VIDEO_12BITS			0x1
 #define VIDEO_24BITS			0x2
 //#define VIDEO_DISABLE			0x3
 
-#define SCU_FUC_PIN_USB11_PORT2		(0x1 << 3)
-#define SCU_FUC_PIN_SD1_8BIT		(0x1 << 3)
+#define SCU_FUN_PIN_USB11_PORT2		(0x1 << 3)
+#define SCU_FUN_PIN_SD1_8BIT		(0x1 << 3)
 
-#define SCU_FUC_PIN_MAC1_MDIO		(0x1 << 2)
-#define SCU_FUC_PIN_SD2			(0x1 << 1)
-#define SCU_FUC_PIN_SD1			(0x1 << 0)
+#define SCU_FUN_PIN_MAC1_MDIO		(0x1 << 2)
+#define SCU_FUN_PIN_SD2			(0x1 << 1)
+#define SCU_FUN_PIN_SD1			(0x1 << 0)
 
 
 /* AST_SCU_FUN_PIN_CTRL6		0x94 - Multi-function Pin Control#6*/
@@ -914,6 +915,11 @@ 
 #define SCU_FUN_PIN_ROMA4		(0x1 << 18)
 #define SCU_FUN_PIN_ROMA3		(0x1 << 17)
 #define SCU_FUN_PIN_ROMA2		(0x1 << 16)
+/* AST2500 only */
+#define SCU_FUN_PIN_SDA2		(0x1 << 15)
+#define SCU_FUN_PIN_SCL2		(0x1 << 14)
+#define SCU_FUN_PIN_SDA1		(0x1 << 13)
+#define SCU_FUN_PIN_SCL1		(0x1 << 12)
 
 /* AST_SCU_FUN_PIN_CTRL9		0xA8 - Multi-function Pin Control#9 */
 #define SCU_FUN_PIN_ROMA21		(0x1 << 3)
diff --git a/arch/arm/mach-aspeed/ast-scu.c b/arch/arm/mach-aspeed/ast-scu.c
index 0cc0d67..1452b93 100644
--- a/arch/arm/mach-aspeed/ast-scu.c
+++ b/arch/arm/mach-aspeed/ast-scu.c
@@ -318,6 +318,14 @@  u32 ast_get_ahbclk(void)
 
 #endif /* AST_SOC_G5 */
 
+u32 ast_get_apbclk(void)
+{
+	u32 h_pll = ast_get_h_pll_clk();
+	u32 apb_div = SCU_GET_PCLK_DIV(ast_scu_read(AST_SCU_CLK_SEL));
+	return h_pll / apb_div;
+}
+
+
 void ast_scu_show_system_info(void)
 {
 
@@ -394,7 +402,7 @@  void ast_scu_multi_func_eth(u8 num)
 			      AST_SCU_FUN_PIN_CTRL1);
 
 		ast_scu_write(ast_scu_read(AST_SCU_FUN_PIN_CTRL5) |
-			      SCU_FUC_PIN_MAC1_MDIO,
+			      SCU_FUN_PIN_MAC1_MDIO,
 			      AST_SCU_FUN_PIN_CTRL5);
 
 		break;
@@ -496,3 +504,23 @@  void ast_scu_get_who_init_dram(void)
 		break;
 	}
 }
+
+void ast_scu_enable_i2c(u8 num)
+{
+	/* Enable I2C Controllers */
+	clrbits_le32(AST_SCU_BASE + AST_SCU_RESET, SCU_RESET_I2C);
+
+	if (num < 3) {
+#ifdef AST_SOC_G5
+		if (num == 1) {
+			setbits_le32(AST_SCU_BASE + AST_SCU_FUN_PIN_CTRL8,
+						 SCU_FUN_PIN_SDA1 | SCU_FUN_PIN_SCL1);
+		} else {
+			setbits_le32(AST_SCU_BASE + AST_SCU_FUN_PIN_CTRL8,
+						 SCU_FUN_PIN_SDA2 | SCU_FUN_PIN_SCL2);
+		}
+#endif
+	} else {
+		setbits_le32(AST_SCU_BASE + AST_SCU_FUN_PIN_CTRL5, SCU_FUN_PIN_I2C(num));
+	}
+}