[PATCH 2/3] aspeed/g5: helper SCU functions for configuring aspeed I2C
Maxim Sloyko
maxims at google.com
Fri Sep 30 04:23:55 AEST 2016
On Wed, Sep 28, 2016 at 6:03 PM, Joel Stanley <joel at jms.id.au> wrote:
> On Wed, Sep 28, 2016 at 3:13 AM, <maxims at google.com> wrote:
>
> You need a commit message.
>
> > From: Maxim Sloyko <maxims at 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 at lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/openbmc
>
--
*M*axim *S*loyko
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20160929/560db889/attachment.html>
More information about the openbmc
mailing list