<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Sep 28, 2016 at 6:03 PM, Joel Stanley <span dir="ltr"><<a href="mailto:joel@jms.id.au" target="_blank">joel@jms.id.au</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Wed, Sep 28, 2016 at 3:13 AM, <<a href="mailto:maxims@google.com">maxims@google.com</a>> wrote:<br>
<br>
You need a commit message.<br>
<span class=""><br>
> From: Maxim Sloyko <<a href="mailto:maxims@google.com">maxims@google.com</a>><br>
><br>
> ---<br>
> arch/arm/include/asm/arch-<wbr>aspeed/ast_scu.h | 6 +++<br>
> arch/arm/include/asm/arch-<wbr>aspeed/regs-scu.h | 74 ++++++++++++++++-------------<br>
<br>
</span>Please put the reformatting of header files in a seperate patch.<br></blockquote><div><br></div><div>What do you mean by reformatting?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
> arch/arm/mach-aspeed/ast-scu.c | 30 +++++++++++-<br>
> 3 files changed, 75 insertions(+), 35 deletions(-)<br>
><br>
> diff --git a/arch/arm/include/asm/arch-<wbr>aspeed/ast_scu.h b/arch/arm/include/asm/arch-<wbr>aspeed/ast_scu.h<br>
> index d248416..80ebd6f 100644<br>
> --- a/arch/arm/include/asm/arch-<wbr>aspeed/ast_scu.h<br>
> +++ b/arch/arm/include/asm/arch-<wbr>aspeed/ast_scu.h<br>
> @@ -38,6 +38,7 @@ extern void ast_scu_get_who_init_dram(<wbr>void);<br>
> extern u32 ast_get_clk_source(void);<br>
> extern u32 ast_get_h_pll_clk(void);<br>
> extern u32 ast_get_ahbclk(void);<br>
> +extern u32 ast_get_apbclk(void);<br>
><br>
> extern u32 ast_scu_get_vga_memsize(void);<br>
><br>
> @@ -45,4 +46,9 @@ extern void ast_scu_init_eth(u8 num);<br>
> extern void ast_scu_multi_func_eth(u8 num);<br>
> extern void ast_scu_multi_func_romcs(u8 num);<br>
><br>
> +/* Enable I2C controller and pins for a particular device.<br>
> + * Device numbering starts at 1<br>
> + */<br>
> +extern void ast_scu_enable_i2c(u8 num);<br>
> +<br>
> #endif<br>
> diff --git a/arch/arm/include/asm/arch-<wbr>aspeed/regs-scu.h b/arch/arm/include/asm/arch-<wbr>aspeed/regs-scu.h<br>
> index b89df82..92ce84a 100644<br>
> --- a/arch/arm/include/asm/arch-<wbr>aspeed/regs-scu.h<br>
> +++ b/arch/arm/include/asm/arch-<wbr>aspeed/regs-scu.h<br>
> @@ -10,8 +10,8 @@<br>
> * 1. 2012/12/29 Ryan Chen Create<br>
> *<br>
> ******************************<wbr>******************************<wbr>********************/<br>
> -#ifndef __AST_SCU_H<br>
> -#define __AST_SCU_H 1<br>
> +#ifndef __AST_REGS_SCU_H<br>
> +#define __AST_REGS_SCU_H 1<br>
><br>
> #include <asm/arch/aspeed.h><br>
><br>
> @@ -830,49 +830,50 @@<br>
> /* AST_SCU_FUN_PIN_CTRL5 0x90 - Multi-function Pin Control#5 */<br>
> #define SCU_FUN_PIN_SPICS1 (0x1 << 31)<br>
> #define SCU_FUN_PIN_LPC_PLUS (0x1 << 30)<br>
> -#define SCU_FUC_PIN_USB20_HOST (0x1 << 29)<br>
> -#define SCU_FUC_PIN_USB11_PORT4 (0x1 << 28)<br>
> -#define SCU_FUC_PIN_I2C14 (0x1 << 27)<br>
> -#define SCU_FUC_PIN_I2C13 (0x1 << 26)<br>
> -#define SCU_FUC_PIN_I2C12 (0x1 << 25)<br>
> -#define SCU_FUC_PIN_I2C11 (0x1 << 24)<br>
> -#define SCU_FUC_PIN_I2C10 (0x1 << 23)<br>
> -#define SCU_FUC_PIN_I2C9 (0x1 << 22)<br>
> -#define SCU_FUC_PIN_I2C8 (0x1 << 21)<br>
> -#define SCU_FUC_PIN_I2C7 (0x1 << 20)<br>
> -#define SCU_FUC_PIN_I2C6 (0x1 << 19)<br>
> -#define SCU_FUC_PIN_I2C5 (0x1 << 18)<br>
> -#define SCU_FUC_PIN_I2C4 (0x1 << 17)<br>
> -#define SCU_FUC_PIN_I2C3 (0x1 << 16)<br>
> -#define SCU_FUC_PIN_MII2_RX_DWN_DIS (0x1 << 15)<br>
> -#define SCU_FUC_PIN_MII2_TX_DWN_DIS (0x1 << 14)<br>
> -#define SCU_FUC_PIN_MII1_RX_DWN_DIS (0x1 << 13)<br>
> -#define SCU_FUC_PIN_MII1_TX_DWN_DIS (0x1 << 12)<br>
> -<br>
> -#define SCU_FUC_PIN_MII2_TX_DRIV(x) (x << 10)<br>
> -#define SCU_FUC_PIN_MII2_TX_DRIV_MASK (0x3 << 10)<br>
> -#define SCU_FUC_PIN_MII1_TX_DRIV(x) (x << 8)<br>
> -#define SCU_FUC_PIN_MII1_TX_DRIV_MASK (0x3 << 8)<br>
> +#define SCU_FUN_PIN_USB20_HOST (0x1 << 29)<br>
> +#define SCU_FUN_PIN_USB11_PORT4 (0x1 << 28)<br>
> +#define SCU_FUN_PIN_I2C14 (0x1 << 27)<br>
> +#define SCU_FUN_PIN_I2C13 (0x1 << 26)<br>
> +#define SCU_FUN_PIN_I2C12 (0x1 << 25)<br>
> +#define SCU_FUN_PIN_I2C11 (0x1 << 24)<br>
> +#define SCU_FUN_PIN_I2C10 (0x1 << 23)<br>
> +#define SCU_FUN_PIN_I2C9 (0x1 << 22)<br>
> +#define SCU_FUN_PIN_I2C8 (0x1 << 21)<br>
> +#define SCU_FUN_PIN_I2C7 (0x1 << 20)<br>
> +#define SCU_FUN_PIN_I2C6 (0x1 << 19)<br>
> +#define SCU_FUN_PIN_I2C5 (0x1 << 18)<br>
> +#define SCU_FUN_PIN_I2C4 (0x1 << 17)<br>
> +#define SCU_FUN_PIN_I2C3 (0x1 << 16)<br>
> +#define SCU_FUN_PIN_I2C(n) (0x1 << (16 + (n) - 3))<br>
> +#define SCU_FUN_PIN_MII2_RX_DWN_DIS (0x1 << 15)<br>
> +#define SCU_FUN_PIN_MII2_TX_DWN_DIS (0x1 << 14)<br>
> +#define SCU_FUN_PIN_MII1_RX_DWN_DIS (0x1 << 13)<br>
> +#define SCU_FUN_PIN_MII1_TX_DWN_DIS (0x1 << 12)<br>
<br>
</div></div>It looks like you're just changing the SCU_FUC_ to SCU_FUN. Without a<br>
commit message I'm not sure why you're doing this, can you explain<br>
why?<br>
<br>
I don't think it is required.<br>
<br>
If you were to make a change like this you should perform the renaming<br>
and update the call sites in a separate patch to any other changes.<br>
This makes it easier to review.<br></blockquote><div><br></div><div>Because that's a typo that got out of hand. FUN supposed to stand for FUNCTION,</div><div>FUC ... no guesses.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
<br>
> diff --git a/arch/arm/mach-aspeed/ast-<wbr>scu.c b/arch/arm/mach-aspeed/ast-<wbr>scu.c<br>
> index 0cc0d67..1452b93 100644<br>
> --- a/arch/arm/mach-aspeed/ast-<wbr>scu.c<br>
> +++ b/arch/arm/mach-aspeed/ast-<wbr>scu.c<br>
> @@ -318,6 +318,14 @@ u32 ast_get_ahbclk(void)<br>
><br>
> #endif /* AST_SOC_G5 */<br>
><br>
> +u32 ast_get_apbclk(void)<br>
> +{<br>
> + u32 h_pll = ast_get_h_pll_clk();<br>
> + u32 apb_div = SCU_GET_PCLK_DIV(ast_scu_read(<wbr>AST_SCU_CLK_SEL));<br>
> + return h_pll / apb_div;<br>
> +}<br>
<br>
</span>This will do for now. We suspect need to write a clock driver that<br>
will live in drivers/clk/ to do it properly.<br>
<br>
Can apb_div ever be zero?<br>
<span class=""><br></span></blockquote><div><br></div><div>Oh, this is actually wrong... Fixed. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> +<br>
> +<br>
> void ast_scu_show_system_info(void)<br>
> {<br>
><br>
> @@ -394,7 +402,7 @@ void ast_scu_multi_func_eth(u8 num)<br>
> AST_SCU_FUN_PIN_CTRL1);<br>
><br>
> ast_scu_write(ast_scu_read(<wbr>AST_SCU_FUN_PIN_CTRL5) |<br>
> - SCU_FUC_PIN_MAC1_MDIO,<br>
> + SCU_FUN_PIN_MAC1_MDIO,<br>
> AST_SCU_FUN_PIN_CTRL5);<br>
><br>
> break;<br>
> @@ -496,3 +504,23 @@ void ast_scu_get_who_init_dram(<wbr>void)<br>
> break;<br>
> }<br>
> }<br>
> +<br>
> +void ast_scu_enable_i2c(u8 num)<br>
> +{<br>
<br>
</span>Is num the bus? Perhaps a better name would be "bus_enable" or similar.<br>
<br>
I think you mentioned that num must be > 0. If so, check for zero, log<br>
an error and return.<br>
<span class=""><br>
<br>
> + /* Enable I2C Controllers */<br>
> + clrbits_le32(AST_SCU_BASE + AST_SCU_RESET, SCU_RESET_I2C);<br>
> +<br>
> + if (num < 3) {<br>
<br>
</span>How do we enable buses number 1 and 2 when AST_SOC_G5 is not set?<br></blockquote><div><br></div><div>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.</div><div>Also, this part is basically taken from aspeed's sdk, this is why I assume they are always on.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> +#ifdef AST_SOC_G5<br>
> + if (num == 1) {<br>
> + setbits_le32(AST_SCU_BASE + AST_SCU_FUN_PIN_CTRL8,<br>
> + SCU_FUN_PIN_SDA1 | SCU_FUN_PIN_SCL1);<br>
> + } else {<br>
<br>
</span>Make the else check for num == 2, as that's what you're looking for.<br>
<span class=""><br>
> + setbits_le32(AST_SCU_BASE + AST_SCU_FUN_PIN_CTRL8,<br>
> + SCU_FUN_PIN_SDA2 | SCU_FUN_PIN_SCL2);<br>
> + }<br>
> +#endif<br>
> + } else {<br>
> + setbits_le32(AST_SCU_BASE + AST_SCU_FUN_PIN_CTRL5, SCU_FUN_PIN_I2C(num));<br>
> + }<br>
<br>
<br>
> +}<br>
> --<br>
> 2.8.0.rc3.226.g39d4020<br>
><br>
</span>> ______________________________<wbr>_________________<br>
> openbmc mailing list<br>
> <a href="mailto:openbmc@lists.ozlabs.org">openbmc@lists.ozlabs.org</a><br>
> <a href="https://lists.ozlabs.org/listinfo/openbmc" rel="noreferrer" target="_blank">https://lists.ozlabs.org/<wbr>listinfo/openbmc</a><br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature" data-smartmail="gmail_signature"><div><b>M</b>axim <b>S</b>loyko</div></div>
</div></div>