| Message ID | 1492676134-21452-1-git-send-email-kever.yang@rock-chips.com |
|---|---|
| State | Accepted |
| Commit | 602778d3c7ea367bb55f73ec33e8ef8c2c1dcb12 |
| Delegated to: | Simon Glass |
| Headers | show |
> > On 20 Apr 2017, at 10:15, Kever Yang <kever.yang@rock-chips.com> wrote: > > GMAC controller need to init the tx io driver strength to 13mA, > just like the description in dts pinctrl node, or else the controller > may only work in 100MHz Mode, and fail to work at 1000MHz mode. > > Signed-off-by: Kever Yang <kever.yang@rock-chips.com> > --- > > arch/arm/include/asm/arch-rockchip/grf_rk3399.h | 75 +++++++++++++++++++++++-- > drivers/pinctrl/rockchip/pinctrl_rk3399.c | 18 ++++++ > 2 files changed, 89 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/include/asm/arch-rockchip/grf_rk3399.h b/arch/arm/include/asm/arch-rockchip/grf_rk3399.h > index b340b05..e8a6f71 100644 > --- a/arch/arm/include/asm/arch-rockchip/grf_rk3399.h > +++ b/arch/arm/include/asm/arch-rockchip/grf_rk3399.h > @@ -151,10 +151,11 @@ struct rk3399_grf_regs { > u32 gpio2_sr[3][4]; > u32 reserved23[4]; > u32 gpio2_smt[3][4]; > - u32 reserved24[(0xe130 - 0xe0ec)/4 - 1]; > - u32 gpio4b_e01; > - u32 gpio4b_e2; > - u32 reserved24a[(0xe200 - 0xe134)/4 - 1]; > + u32 reserved24[(0xe100 - 0xe0ec)/4 - 1]; > + u32 gpio2_e[4]; > + u32 gpio3_e[7]; > + u32 gpio4_e[5]; > + u32 reserved24a[(0xe200 - 0xe13c)/4 - 1]; > u32 soc_con0; > u32 soc_con1; > u32 soc_con2; > @@ -435,6 +436,72 @@ enum { > GRF_GPIO4C6_SEL_MASK = 3 << GRF_GPIO4C6_SEL_SHIFT, > GRF_PWM_1 = 1, > > + /* GRF_GPIO3A_E01 */ > + GRF_GPIO3A0_E_SHIFT = 0, > + GRF_GPIO3A0_E_MASK = 7 << GRF_GPIO3A0_E_SHIFT, > + GRF_GPIO3A1_E_SHIFT = 3, > + GRF_GPIO3A1_E_MASK = 7 << GRF_GPIO3A1_E_SHIFT, > + GRF_GPIO3A2_E_SHIFT = 6, > + GRF_GPIO3A2_E_MASK = 7 << GRF_GPIO3A2_E_SHIFT, > + GRF_GPIO3A3_E_SHIFT = 9, > + GRF_GPIO3A3_E_MASK = 7 << GRF_GPIO3A3_E_SHIFT, > + GRF_GPIO3A4_E_SHIFT = 12, > + GRF_GPIO3A4_E_MASK = 7 << GRF_GPIO3A4_E_SHIFT, > + GRF_GPIO3A5_E0_SHIFT = 15, > + GRF_GPIO3A5_E0_MASK = 1 << GRF_GPIO3A5_E0_SHIFT, > + > + /* GRF_GPIO3A_E2 */ > + GRF_GPIO3A5_E12_SHIFT = 0, > + GRF_GPIO3A5_E12_MASK = 3 << GRF_GPIO3A5_E12_SHIFT, > + GRF_GPIO3A6_E_SHIFT = 2, > + GRF_GPIO3A6_E_MASK = 7 << GRF_GPIO3A6_E_SHIFT, > + GRF_GPIO3A7_E_SHIFT = 5, > + GRF_GPIO3A7_E_MASK = 7 << GRF_GPIO3A7_E_SHIFT, > + > + /* GRF_GPIO3B_E01 */ > + GRF_GPIO3B0_E_SHIFT = 0, > + GRF_GPIO3B0_E_MASK = 7 << GRF_GPIO3B0_E_SHIFT, > + GRF_GPIO3B1_E_SHIFT = 3, > + GRF_GPIO3B1_E_MASK = 7 << GRF_GPIO3B1_E_SHIFT, > + GRF_GPIO3B2_E_SHIFT = 6, > + GRF_GPIO3B2_E_MASK = 7 << GRF_GPIO3B2_E_SHIFT, > + GRF_GPIO3B3_E_SHIFT = 9, > + GRF_GPIO3B3_E_MASK = 7 << GRF_GPIO3B3_E_SHIFT, > + GRF_GPIO3B4_E_SHIFT = 12, > + GRF_GPIO3B4_E_MASK = 7 << GRF_GPIO3B4_E_SHIFT, > + GRF_GPIO3B5_E0_SHIFT = 15, > + GRF_GPIO3B5_E0_MASK = 1 << GRF_GPIO3B5_E0_SHIFT, > + > + /* GRF_GPIO3A_E2 */ > + GRF_GPIO3B5_E12_SHIFT = 0, > + GRF_GPIO3B5_E12_MASK = 3 << GRF_GPIO3B5_E12_SHIFT, > + GRF_GPIO3B6_E_SHIFT = 2, > + GRF_GPIO3B6_E_MASK = 7 << GRF_GPIO3B6_E_SHIFT, > + GRF_GPIO3B7_E_SHIFT = 5, > + GRF_GPIO3B7_E_MASK = 7 << GRF_GPIO3B7_E_SHIFT, > + > + /* GRF_GPIO3C_E01 */ > + GRF_GPIO3C0_E_SHIFT = 0, > + GRF_GPIO3C0_E_MASK = 7 << GRF_GPIO3C0_E_SHIFT, > + GRF_GPIO3C1_E_SHIFT = 3, > + GRF_GPIO3C1_E_MASK = 7 << GRF_GPIO3C1_E_SHIFT, > + GRF_GPIO3C2_E_SHIFT = 6, > + GRF_GPIO3C2_E_MASK = 7 << GRF_GPIO3C2_E_SHIFT, > + GRF_GPIO3C3_E_SHIFT = 9, > + GRF_GPIO3C3_E_MASK = 7 << GRF_GPIO3C3_E_SHIFT, > + GRF_GPIO3C4_E_SHIFT = 12, > + GRF_GPIO3C4_E_MASK = 7 << GRF_GPIO3C4_E_SHIFT, > + GRF_GPIO3C5_E0_SHIFT = 15, > + GRF_GPIO3C5_E0_MASK = 1 << GRF_GPIO3C5_E0_SHIFT, > + > + /* GRF_GPIO3C_E2 */ > + GRF_GPIO3C5_E12_SHIFT = 0, > + GRF_GPIO3C5_E12_MASK = 3 << GRF_GPIO3C5_E12_SHIFT, > + GRF_GPIO3C6_E_SHIFT = 2, > + GRF_GPIO3C6_E_MASK = 7 << GRF_GPIO3C6_E_SHIFT, > + GRF_GPIO3C7_E_SHIFT = 5, > + GRF_GPIO3C7_E_MASK = 7 << GRF_GPIO3C7_E_SHIFT, > + > /* GRF_SOC_CON7 */ > GRF_UART_DBG_SEL_SHIFT = 10, > GRF_UART_DBG_SEL_MASK = 3 << GRF_UART_DBG_SEL_SHIFT, > diff --git a/drivers/pinctrl/rockchip/pinctrl_rk3399.c b/drivers/pinctrl/rockchip/pinctrl_rk3399.c > index 507bec4..6b62a0c 100644 > --- a/drivers/pinctrl/rockchip/pinctrl_rk3399.c > +++ b/drivers/pinctrl/rockchip/pinctrl_rk3399.c > @@ -232,6 +232,24 @@ static void pinctrl_rk3399_gmac_config(struct rk3399_grf_regs *grf, int mmc_id) > rk_clrsetreg(&grf->gpio3c_iomux, > GRF_GPIO3C1_SEL_MASK, > GRF_MAC_TXCLK << GRF_GPIO3C1_SEL_SHIFT); > + > + /* Set drive strength for GMAC tx io, value 3 means 13mA */ > + rk_clrsetreg(&grf->gpio3_e[0], > + GRF_GPIO3A0_E_MASK | GRF_GPIO3A1_E_MASK | > + GRF_GPIO3A4_E_MASK | GRF_GPIO3A5_E0_MASK, > + 3 << GRF_GPIO3A0_E_SHIFT | > + 3 << GRF_GPIO3A1_E_SHIFT | > + 3 << GRF_GPIO3A4_E_SHIFT | > + 1 << GRF_GPIO3A5_E0_SHIFT); > + rk_clrsetreg(&grf->gpio3_e[1], > + GRF_GPIO3A5_E12_MASK, > + 1 << GRF_GPIO3A5_E12_SHIFT); > + rk_clrsetreg(&grf->gpio3_e[2], > + GRF_GPIO3B4_E_MASK, > + 3 << GRF_GPIO3B4_E_SHIFT); > + rk_clrsetreg(&grf->gpio3_e[4], > + GRF_GPIO3C1_E_MASK, > + 3 << GRF_GPIO3C1_E_SHIFT); > } > #endif > > -- > 1.9.1 > Do you know if this is required for all board designs? We have a total run length of less than 2cm to the KSZ9031 PHY and wondered about this ourselves—our testing has shown that with these small distances (and the PHY we use) the setting doesn’t seem to be required. Instead of hard-coding this: shouldn't we parse the drive-strength setting from the DTS? Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com <mailto:philipp.tomsich@theobroma-systems.com>>
Hi Philipp, On 04/20/2017 04:21 PM, Dr. Philipp Tomsich wrote: >> >> On 20 Apr 2017, at 10:15, Kever Yang <kever.yang@rock-chips.com >> <mailto:kever.yang@rock-chips.com>> wrote: >> >> GMAC controller need to init the tx io driver strength to 13mA, >> just like the description in dts pinctrl node, or else the controller >> may only work in 100MHz Mode, and fail to work at 1000MHz mode. >> >> Signed-off-by: Kever Yang <kever.yang@rock-chips.com >> <mailto:kever.yang@rock-chips.com>> >> --- >> >> arch/arm/include/asm/arch-rockchip/grf_rk3399.h | 75 >> +++++++++++++++++++++++-- >> drivers/pinctrl/rockchip/pinctrl_rk3399.c | 18 ++++++ >> 2 files changed, 89 insertions(+), 4 deletions(-) >> >> diff --git a/arch/arm/include/asm/arch-rockchip/grf_rk3399.h >> b/arch/arm/include/asm/arch-rockchip/grf_rk3399.h >> index b340b05..e8a6f71 100644 >> --- a/arch/arm/include/asm/arch-rockchip/grf_rk3399.h >> +++ b/arch/arm/include/asm/arch-rockchip/grf_rk3399.h >> @@ -151,10 +151,11 @@ struct rk3399_grf_regs { >> u32 gpio2_sr[3][4]; >> u32 reserved23[4]; >> u32 gpio2_smt[3][4]; >> -u32 reserved24[(0xe130 - 0xe0ec)/4 - 1]; >> -u32 gpio4b_e01; >> -u32 gpio4b_e2; >> -u32 reserved24a[(0xe200 - 0xe134)/4 - 1]; >> +u32 reserved24[(0xe100 - 0xe0ec)/4 - 1]; >> +u32 gpio2_e[4]; >> +u32 gpio3_e[7]; >> +u32 gpio4_e[5]; >> +u32 reserved24a[(0xe200 - 0xe13c)/4 - 1]; >> u32 soc_con0; >> u32 soc_con1; >> u32 soc_con2; >> @@ -435,6 +436,72 @@ enum { >> GRF_GPIO4C6_SEL_MASK = 3 << GRF_GPIO4C6_SEL_SHIFT, >> GRF_PWM_1 = 1, >> >> +/* GRF_GPIO3A_E01 */ >> +GRF_GPIO3A0_E_SHIFT = 0, >> +GRF_GPIO3A0_E_MASK = 7 << GRF_GPIO3A0_E_SHIFT, >> +GRF_GPIO3A1_E_SHIFT = 3, >> +GRF_GPIO3A1_E_MASK = 7 << GRF_GPIO3A1_E_SHIFT, >> +GRF_GPIO3A2_E_SHIFT = 6, >> +GRF_GPIO3A2_E_MASK = 7 << GRF_GPIO3A2_E_SHIFT, >> +GRF_GPIO3A3_E_SHIFT = 9, >> +GRF_GPIO3A3_E_MASK = 7 << GRF_GPIO3A3_E_SHIFT, >> +GRF_GPIO3A4_E_SHIFT = 12, >> +GRF_GPIO3A4_E_MASK = 7 << GRF_GPIO3A4_E_SHIFT, >> +GRF_GPIO3A5_E0_SHIFT = 15, >> +GRF_GPIO3A5_E0_MASK = 1 << GRF_GPIO3A5_E0_SHIFT, >> + >> +/* GRF_GPIO3A_E2 */ >> +GRF_GPIO3A5_E12_SHIFT = 0, >> +GRF_GPIO3A5_E12_MASK = 3 << GRF_GPIO3A5_E12_SHIFT, >> +GRF_GPIO3A6_E_SHIFT = 2, >> +GRF_GPIO3A6_E_MASK = 7 << GRF_GPIO3A6_E_SHIFT, >> +GRF_GPIO3A7_E_SHIFT = 5, >> +GRF_GPIO3A7_E_MASK = 7 << GRF_GPIO3A7_E_SHIFT, >> + >> +/* GRF_GPIO3B_E01 */ >> +GRF_GPIO3B0_E_SHIFT = 0, >> +GRF_GPIO3B0_E_MASK = 7 << GRF_GPIO3B0_E_SHIFT, >> +GRF_GPIO3B1_E_SHIFT = 3, >> +GRF_GPIO3B1_E_MASK = 7 << GRF_GPIO3B1_E_SHIFT, >> +GRF_GPIO3B2_E_SHIFT = 6, >> +GRF_GPIO3B2_E_MASK = 7 << GRF_GPIO3B2_E_SHIFT, >> +GRF_GPIO3B3_E_SHIFT = 9, >> +GRF_GPIO3B3_E_MASK = 7 << GRF_GPIO3B3_E_SHIFT, >> +GRF_GPIO3B4_E_SHIFT = 12, >> +GRF_GPIO3B4_E_MASK = 7 << GRF_GPIO3B4_E_SHIFT, >> +GRF_GPIO3B5_E0_SHIFT = 15, >> +GRF_GPIO3B5_E0_MASK = 1 << GRF_GPIO3B5_E0_SHIFT, >> + >> +/* GRF_GPIO3A_E2 */ >> +GRF_GPIO3B5_E12_SHIFT = 0, >> +GRF_GPIO3B5_E12_MASK = 3 << GRF_GPIO3B5_E12_SHIFT, >> +GRF_GPIO3B6_E_SHIFT = 2, >> +GRF_GPIO3B6_E_MASK = 7 << GRF_GPIO3B6_E_SHIFT, >> +GRF_GPIO3B7_E_SHIFT = 5, >> +GRF_GPIO3B7_E_MASK = 7 << GRF_GPIO3B7_E_SHIFT, >> + >> +/* GRF_GPIO3C_E01 */ >> +GRF_GPIO3C0_E_SHIFT = 0, >> +GRF_GPIO3C0_E_MASK = 7 << GRF_GPIO3C0_E_SHIFT, >> +GRF_GPIO3C1_E_SHIFT = 3, >> +GRF_GPIO3C1_E_MASK = 7 << GRF_GPIO3C1_E_SHIFT, >> +GRF_GPIO3C2_E_SHIFT = 6, >> +GRF_GPIO3C2_E_MASK = 7 << GRF_GPIO3C2_E_SHIFT, >> +GRF_GPIO3C3_E_SHIFT = 9, >> +GRF_GPIO3C3_E_MASK = 7 << GRF_GPIO3C3_E_SHIFT, >> +GRF_GPIO3C4_E_SHIFT = 12, >> +GRF_GPIO3C4_E_MASK = 7 << GRF_GPIO3C4_E_SHIFT, >> +GRF_GPIO3C5_E0_SHIFT = 15, >> +GRF_GPIO3C5_E0_MASK = 1 << GRF_GPIO3C5_E0_SHIFT, >> + >> +/* GRF_GPIO3C_E2 */ >> +GRF_GPIO3C5_E12_SHIFT = 0, >> +GRF_GPIO3C5_E12_MASK = 3 << GRF_GPIO3C5_E12_SHIFT, >> +GRF_GPIO3C6_E_SHIFT = 2, >> +GRF_GPIO3C6_E_MASK = 7 << GRF_GPIO3C6_E_SHIFT, >> +GRF_GPIO3C7_E_SHIFT = 5, >> +GRF_GPIO3C7_E_MASK = 7 << GRF_GPIO3C7_E_SHIFT, >> + >> /* GRF_SOC_CON7 */ >> GRF_UART_DBG_SEL_SHIFT= 10, >> GRF_UART_DBG_SEL_MASK= 3 << GRF_UART_DBG_SEL_SHIFT, >> diff --git a/drivers/pinctrl/rockchip/pinctrl_rk3399.c >> b/drivers/pinctrl/rockchip/pinctrl_rk3399.c >> index 507bec4..6b62a0c 100644 >> --- a/drivers/pinctrl/rockchip/pinctrl_rk3399.c >> +++ b/drivers/pinctrl/rockchip/pinctrl_rk3399.c >> @@ -232,6 +232,24 @@ static void pinctrl_rk3399_gmac_config(struct >> rk3399_grf_regs *grf, int mmc_id) >> rk_clrsetreg(&grf->gpio3c_iomux, >> GRF_GPIO3C1_SEL_MASK, >> GRF_MAC_TXCLK << GRF_GPIO3C1_SEL_SHIFT); >> + >> +/* Set drive strength for GMAC tx io, value 3 means 13mA */ >> +rk_clrsetreg(&grf->gpio3_e[0], >> + GRF_GPIO3A0_E_MASK | GRF_GPIO3A1_E_MASK | >> + GRF_GPIO3A4_E_MASK | GRF_GPIO3A5_E0_MASK, >> + 3 << GRF_GPIO3A0_E_SHIFT | >> + 3 << GRF_GPIO3A1_E_SHIFT | >> + 3 << GRF_GPIO3A4_E_SHIFT | >> + 1 << GRF_GPIO3A5_E0_SHIFT); >> +rk_clrsetreg(&grf->gpio3_e[1], >> + GRF_GPIO3A5_E12_MASK, >> + 1 << GRF_GPIO3A5_E12_SHIFT); >> +rk_clrsetreg(&grf->gpio3_e[2], >> + GRF_GPIO3B4_E_MASK, >> + 3 << GRF_GPIO3B4_E_SHIFT); >> +rk_clrsetreg(&grf->gpio3_e[4], >> + GRF_GPIO3C1_E_MASK, >> + 3 << GRF_GPIO3C1_E_SHIFT); >> } >> #endif >> >> -- >> 1.9.1 >> > > Do you know if this is required for all board designs? > We have a total run length of less than 2cm to the KSZ9031 PHY and > wondered about this ourselves—our testing has shown that with these > small distances (and the PHY we use) the setting doesn’t seem to be > required. If your layout is very good, it might work without this patch, did you test with 1000M Ethernet on many boards? With patch, we can keep the setting with kernel and make sure all the hardware able to work at 1000M mode. The firefly-rk3399 and rockchip rk3399-evb can't work at 1000M Ethernet mode without this patch. > > Instead of hard-coding this: shouldn't we parse the drive-strength > setting from the DTS? Sync with the kernel pinctrl-rockchip can parse the drive-strength, but I don't think it's a good idea to sync that more than 2000 line file, this patch should be enough for U-Boot now. Thanks, - Kever > > Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com > <mailto:philipp.tomsich@theobroma-systems.com>> >
> On 20 Apr 2017, at 10:44, Kever Yang <kever.yang@rock-chips.com> wrote: > > Hi Philipp, > > On 04/20/2017 04:21 PM, Dr. Philipp Tomsich wrote: >>> >>> On 20 Apr 2017, at 10:15, Kever Yang <kever.yang@rock-chips.com <mailto:kever.yang@rock-chips.com>> wrote: >>> >>> GMAC controller need to init the tx io driver strength to 13mA, >>> just like the description in dts pinctrl node, or else the controller >>> may only work in 100MHz Mode, and fail to work at 1000MHz mode. >>> >>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com <mailto:kever.yang@rock-chips.com>> >>> --- >>> >>> arch/arm/include/asm/arch-rockchip/grf_rk3399.h | 75 +++++++++++++++++++++++-- >>> drivers/pinctrl/rockchip/pinctrl_rk3399.c | 18 ++++++ >>> 2 files changed, 89 insertions(+), 4 deletions(-) >>> >>> diff --git a/arch/arm/include/asm/arch-rockchip/grf_rk3399.h b/arch/arm/include/asm/arch-rockchip/grf_rk3399.h >>> index b340b05..e8a6f71 100644 >>> --- a/arch/arm/include/asm/arch-rockchip/grf_rk3399.h >>> +++ b/arch/arm/include/asm/arch-rockchip/grf_rk3399.h >>> @@ -151,10 +151,11 @@ struct rk3399_grf_regs { >>> u32 gpio2_sr[3][4]; >>> u32 reserved23[4]; >>> u32 gpio2_smt[3][4]; >>> - u32 reserved24[(0xe130 - 0xe0ec)/4 - 1]; >>> - u32 gpio4b_e01; >>> - u32 gpio4b_e2; >>> - u32 reserved24a[(0xe200 - 0xe134)/4 - 1]; >>> + u32 reserved24[(0xe100 - 0xe0ec)/4 - 1]; >>> + u32 gpio2_e[4]; >>> + u32 gpio3_e[7]; >>> + u32 gpio4_e[5]; >>> + u32 reserved24a[(0xe200 - 0xe13c)/4 - 1]; >>> u32 soc_con0; >>> u32 soc_con1; >>> u32 soc_con2; >>> @@ -435,6 +436,72 @@ enum { >>> GRF_GPIO4C6_SEL_MASK = 3 << GRF_GPIO4C6_SEL_SHIFT, >>> GRF_PWM_1 = 1, >>> >>> + /* GRF_GPIO3A_E01 */ >>> + GRF_GPIO3A0_E_SHIFT = 0, >>> + GRF_GPIO3A0_E_MASK = 7 << GRF_GPIO3A0_E_SHIFT, >>> + GRF_GPIO3A1_E_SHIFT = 3, >>> + GRF_GPIO3A1_E_MASK = 7 << GRF_GPIO3A1_E_SHIFT, >>> + GRF_GPIO3A2_E_SHIFT = 6, >>> + GRF_GPIO3A2_E_MASK = 7 << GRF_GPIO3A2_E_SHIFT, >>> + GRF_GPIO3A3_E_SHIFT = 9, >>> + GRF_GPIO3A3_E_MASK = 7 << GRF_GPIO3A3_E_SHIFT, >>> + GRF_GPIO3A4_E_SHIFT = 12, >>> + GRF_GPIO3A4_E_MASK = 7 << GRF_GPIO3A4_E_SHIFT, >>> + GRF_GPIO3A5_E0_SHIFT = 15, >>> + GRF_GPIO3A5_E0_MASK = 1 << GRF_GPIO3A5_E0_SHIFT, >>> + >>> + /* GRF_GPIO3A_E2 */ >>> + GRF_GPIO3A5_E12_SHIFT = 0, >>> + GRF_GPIO3A5_E12_MASK = 3 << GRF_GPIO3A5_E12_SHIFT, >>> + GRF_GPIO3A6_E_SHIFT = 2, >>> + GRF_GPIO3A6_E_MASK = 7 << GRF_GPIO3A6_E_SHIFT, >>> + GRF_GPIO3A7_E_SHIFT = 5, >>> + GRF_GPIO3A7_E_MASK = 7 << GRF_GPIO3A7_E_SHIFT, >>> + >>> + /* GRF_GPIO3B_E01 */ >>> + GRF_GPIO3B0_E_SHIFT = 0, >>> + GRF_GPIO3B0_E_MASK = 7 << GRF_GPIO3B0_E_SHIFT, >>> + GRF_GPIO3B1_E_SHIFT = 3, >>> + GRF_GPIO3B1_E_MASK = 7 << GRF_GPIO3B1_E_SHIFT, >>> + GRF_GPIO3B2_E_SHIFT = 6, >>> + GRF_GPIO3B2_E_MASK = 7 << GRF_GPIO3B2_E_SHIFT, >>> + GRF_GPIO3B3_E_SHIFT = 9, >>> + GRF_GPIO3B3_E_MASK = 7 << GRF_GPIO3B3_E_SHIFT, >>> + GRF_GPIO3B4_E_SHIFT = 12, >>> + GRF_GPIO3B4_E_MASK = 7 << GRF_GPIO3B4_E_SHIFT, >>> + GRF_GPIO3B5_E0_SHIFT = 15, >>> + GRF_GPIO3B5_E0_MASK = 1 << GRF_GPIO3B5_E0_SHIFT, >>> + >>> + /* GRF_GPIO3A_E2 */ >>> + GRF_GPIO3B5_E12_SHIFT = 0, >>> + GRF_GPIO3B5_E12_MASK = 3 << GRF_GPIO3B5_E12_SHIFT, >>> + GRF_GPIO3B6_E_SHIFT = 2, >>> + GRF_GPIO3B6_E_MASK = 7 << GRF_GPIO3B6_E_SHIFT, >>> + GRF_GPIO3B7_E_SHIFT = 5, >>> + GRF_GPIO3B7_E_MASK = 7 << GRF_GPIO3B7_E_SHIFT, >>> + >>> + /* GRF_GPIO3C_E01 */ >>> + GRF_GPIO3C0_E_SHIFT = 0, >>> + GRF_GPIO3C0_E_MASK = 7 << GRF_GPIO3C0_E_SHIFT, >>> + GRF_GPIO3C1_E_SHIFT = 3, >>> + GRF_GPIO3C1_E_MASK = 7 << GRF_GPIO3C1_E_SHIFT, >>> + GRF_GPIO3C2_E_SHIFT = 6, >>> + GRF_GPIO3C2_E_MASK = 7 << GRF_GPIO3C2_E_SHIFT, >>> + GRF_GPIO3C3_E_SHIFT = 9, >>> + GRF_GPIO3C3_E_MASK = 7 << GRF_GPIO3C3_E_SHIFT, >>> + GRF_GPIO3C4_E_SHIFT = 12, >>> + GRF_GPIO3C4_E_MASK = 7 << GRF_GPIO3C4_E_SHIFT, >>> + GRF_GPIO3C5_E0_SHIFT = 15, >>> + GRF_GPIO3C5_E0_MASK = 1 << GRF_GPIO3C5_E0_SHIFT, >>> + >>> + /* GRF_GPIO3C_E2 */ >>> + GRF_GPIO3C5_E12_SHIFT = 0, >>> + GRF_GPIO3C5_E12_MASK = 3 << GRF_GPIO3C5_E12_SHIFT, >>> + GRF_GPIO3C6_E_SHIFT = 2, >>> + GRF_GPIO3C6_E_MASK = 7 << GRF_GPIO3C6_E_SHIFT, >>> + GRF_GPIO3C7_E_SHIFT = 5, >>> + GRF_GPIO3C7_E_MASK = 7 << GRF_GPIO3C7_E_SHIFT, >>> + >>> /* GRF_SOC_CON7 */ >>> GRF_UART_DBG_SEL_SHIFT = 10, >>> GRF_UART_DBG_SEL_MASK = 3 << GRF_UART_DBG_SEL_SHIFT, >>> diff --git a/drivers/pinctrl/rockchip/pinctrl_rk3399.c b/drivers/pinctrl/rockchip/pinctrl_rk3399.c >>> index 507bec4..6b62a0c 100644 >>> --- a/drivers/pinctrl/rockchip/pinctrl_rk3399.c >>> +++ b/drivers/pinctrl/rockchip/pinctrl_rk3399.c >>> @@ -232,6 +232,24 @@ static void pinctrl_rk3399_gmac_config(struct rk3399_grf_regs *grf, int mmc_id) >>> rk_clrsetreg(&grf->gpio3c_iomux, >>> GRF_GPIO3C1_SEL_MASK, >>> GRF_MAC_TXCLK << GRF_GPIO3C1_SEL_SHIFT); >>> + >>> + /* Set drive strength for GMAC tx io, value 3 means 13mA */ >>> + rk_clrsetreg(&grf->gpio3_e[0], >>> + GRF_GPIO3A0_E_MASK | GRF_GPIO3A1_E_MASK | >>> + GRF_GPIO3A4_E_MASK | GRF_GPIO3A5_E0_MASK, >>> + 3 << GRF_GPIO3A0_E_SHIFT | >>> + 3 << GRF_GPIO3A1_E_SHIFT | >>> + 3 << GRF_GPIO3A4_E_SHIFT | >>> + 1 << GRF_GPIO3A5_E0_SHIFT); >>> + rk_clrsetreg(&grf->gpio3_e[1], >>> + GRF_GPIO3A5_E12_MASK, >>> + 1 << GRF_GPIO3A5_E12_SHIFT); >>> + rk_clrsetreg(&grf->gpio3_e[2], >>> + GRF_GPIO3B4_E_MASK, >>> + 3 << GRF_GPIO3B4_E_SHIFT); >>> + rk_clrsetreg(&grf->gpio3_e[4], >>> + GRF_GPIO3C1_E_MASK, >>> + 3 << GRF_GPIO3C1_E_SHIFT); >>> } >>> #endif >>> >>> -- >>> 1.9.1 >>> >> >> Do you know if this is required for all board designs? >> We have a total run length of less than 2cm to the KSZ9031 PHY and wondered about this ourselves—our testing has shown that with these small distances (and the PHY we use) the setting doesn’t seem to be required. > > If your layout is very good, it might work without this patch, did you test with 1000M Ethernet on many boards? > With patch, we can keep the setting with kernel and make sure all the hardware able to work at 1000M mode. > The firefly-rk3399 and rockchip rk3399-evb can't work at 1000M Ethernet mode without this patch. Yes, we have full GbE in U-Boot (without this change) across the entire board population of our initial batch. This is most likely due to the very short distance between the RK3399 and PHY (there isn’t really an alternative to having it close due to the size constraints of the module). Thanks for the background info on the EVB and Firefly. >> Instead of hard-coding this: shouldn't we parse the drive-strength setting from the DTS? > > Sync with the kernel pinctrl-rockchip can parse the drive-strength, > but I don't think it's a good idea to sync that more than 2000 line file, > this patch should be enough for U-Boot now. Ok. Regards, Philipp.
> On 20 Apr 2017, at 10:48, Dr. Philipp Tomsich <philipp.tomsich@theobroma-systems.com> wrote: > >> On 20 Apr 2017, at 10:44, Kever Yang <kever.yang@rock-chips.com> wrote: >> >> Hi Philipp, >>> >>> >>> Do you know if this is required for all board designs? >>> We have a total run length of less than 2cm to the KSZ9031 PHY and wondered about this ourselves—our testing has shown that with these small distances (and the PHY we use) the setting doesn’t seem to be required. >> >> If your layout is very good, it might work without this patch, did you test with 1000M Ethernet on many boards? >> With patch, we can keep the setting with kernel and make sure all the hardware able to work at 1000M mode. >> The firefly-rk3399 and rockchip rk3399-evb can't work at 1000M Ethernet mode without this patch. > > Yes, we have full GbE in U-Boot (without this change) across the entire board population of our initial batch. > This is most likely due to the very short distance between the RK3399 and PHY (there isn’t really an alternative > to having it close due to the size constraints of the module). Just for completeness, Linux with default drive strength (&pcfg_pull_none) works on puma-rk3399 too. Tested with iperf3 we get about 935Mbit with zero RX/TX errors. Regards, Klaus
On 20 April 2017 at 02:15, Kever Yang <kever.yang@rock-chips.com> wrote: > GMAC controller need to init the tx io driver strength to 13mA, > just like the description in dts pinctrl node, or else the controller > may only work in 100MHz Mode, and fail to work at 1000MHz mode. > > Signed-off-by: Kever Yang <kever.yang@rock-chips.com> > --- > > arch/arm/include/asm/arch-rockchip/grf_rk3399.h | 75 +++++++++++++++++++++++-- > drivers/pinctrl/rockchip/pinctrl_rk3399.c | 18 ++++++ > 2 files changed, 89 insertions(+), 4 deletions(-) Reviewed-by: Simon Glass <sjg@chromium.org>
On 20 April 2017 at 02:15, Kever Yang <kever.yang@rock-chips.com> wrote: > GMAC controller need to init the tx io driver strength to 13mA, > just like the description in dts pinctrl node, or else the controller > may only work in 100MHz Mode, and fail to work at 1000MHz mode. > > Signed-off-by: Kever Yang <kever.yang@rock-chips.com> > --- > > arch/arm/include/asm/arch-rockchip/grf_rk3399.h | 75 +++++++++++++++++++++++-- > drivers/pinctrl/rockchip/pinctrl_rk3399.c | 18 ++++++ > 2 files changed, 89 insertions(+), 4 deletions(-) Reviewed-by: Simon Glass <sjg@chromium.org> Applied to u-boot-rockchip/next, thanks!
diff --git a/arch/arm/include/asm/arch-rockchip/grf_rk3399.h b/arch/arm/include/asm/arch-rockchip/grf_rk3399.h index b340b05..e8a6f71 100644 --- a/arch/arm/include/asm/arch-rockchip/grf_rk3399.h +++ b/arch/arm/include/asm/arch-rockchip/grf_rk3399.h @@ -151,10 +151,11 @@ struct rk3399_grf_regs { u32 gpio2_sr[3][4]; u32 reserved23[4]; u32 gpio2_smt[3][4]; - u32 reserved24[(0xe130 - 0xe0ec)/4 - 1]; - u32 gpio4b_e01; - u32 gpio4b_e2; - u32 reserved24a[(0xe200 - 0xe134)/4 - 1]; + u32 reserved24[(0xe100 - 0xe0ec)/4 - 1]; + u32 gpio2_e[4]; + u32 gpio3_e[7]; + u32 gpio4_e[5]; + u32 reserved24a[(0xe200 - 0xe13c)/4 - 1]; u32 soc_con0; u32 soc_con1; u32 soc_con2; @@ -435,6 +436,72 @@ enum { GRF_GPIO4C6_SEL_MASK = 3 << GRF_GPIO4C6_SEL_SHIFT, GRF_PWM_1 = 1, + /* GRF_GPIO3A_E01 */ + GRF_GPIO3A0_E_SHIFT = 0, + GRF_GPIO3A0_E_MASK = 7 << GRF_GPIO3A0_E_SHIFT, + GRF_GPIO3A1_E_SHIFT = 3, + GRF_GPIO3A1_E_MASK = 7 << GRF_GPIO3A1_E_SHIFT, + GRF_GPIO3A2_E_SHIFT = 6, + GRF_GPIO3A2_E_MASK = 7 << GRF_GPIO3A2_E_SHIFT, + GRF_GPIO3A3_E_SHIFT = 9, + GRF_GPIO3A3_E_MASK = 7 << GRF_GPIO3A3_E_SHIFT, + GRF_GPIO3A4_E_SHIFT = 12, + GRF_GPIO3A4_E_MASK = 7 << GRF_GPIO3A4_E_SHIFT, + GRF_GPIO3A5_E0_SHIFT = 15, + GRF_GPIO3A5_E0_MASK = 1 << GRF_GPIO3A5_E0_SHIFT, + + /* GRF_GPIO3A_E2 */ + GRF_GPIO3A5_E12_SHIFT = 0, + GRF_GPIO3A5_E12_MASK = 3 << GRF_GPIO3A5_E12_SHIFT, + GRF_GPIO3A6_E_SHIFT = 2, + GRF_GPIO3A6_E_MASK = 7 << GRF_GPIO3A6_E_SHIFT, + GRF_GPIO3A7_E_SHIFT = 5, + GRF_GPIO3A7_E_MASK = 7 << GRF_GPIO3A7_E_SHIFT, + + /* GRF_GPIO3B_E01 */ + GRF_GPIO3B0_E_SHIFT = 0, + GRF_GPIO3B0_E_MASK = 7 << GRF_GPIO3B0_E_SHIFT, + GRF_GPIO3B1_E_SHIFT = 3, + GRF_GPIO3B1_E_MASK = 7 << GRF_GPIO3B1_E_SHIFT, + GRF_GPIO3B2_E_SHIFT = 6, + GRF_GPIO3B2_E_MASK = 7 << GRF_GPIO3B2_E_SHIFT, + GRF_GPIO3B3_E_SHIFT = 9, + GRF_GPIO3B3_E_MASK = 7 << GRF_GPIO3B3_E_SHIFT, + GRF_GPIO3B4_E_SHIFT = 12, + GRF_GPIO3B4_E_MASK = 7 << GRF_GPIO3B4_E_SHIFT, + GRF_GPIO3B5_E0_SHIFT = 15, + GRF_GPIO3B5_E0_MASK = 1 << GRF_GPIO3B5_E0_SHIFT, + + /* GRF_GPIO3A_E2 */ + GRF_GPIO3B5_E12_SHIFT = 0, + GRF_GPIO3B5_E12_MASK = 3 << GRF_GPIO3B5_E12_SHIFT, + GRF_GPIO3B6_E_SHIFT = 2, + GRF_GPIO3B6_E_MASK = 7 << GRF_GPIO3B6_E_SHIFT, + GRF_GPIO3B7_E_SHIFT = 5, + GRF_GPIO3B7_E_MASK = 7 << GRF_GPIO3B7_E_SHIFT, + + /* GRF_GPIO3C_E01 */ + GRF_GPIO3C0_E_SHIFT = 0, + GRF_GPIO3C0_E_MASK = 7 << GRF_GPIO3C0_E_SHIFT, + GRF_GPIO3C1_E_SHIFT = 3, + GRF_GPIO3C1_E_MASK = 7 << GRF_GPIO3C1_E_SHIFT, + GRF_GPIO3C2_E_SHIFT = 6, + GRF_GPIO3C2_E_MASK = 7 << GRF_GPIO3C2_E_SHIFT, + GRF_GPIO3C3_E_SHIFT = 9, + GRF_GPIO3C3_E_MASK = 7 << GRF_GPIO3C3_E_SHIFT, + GRF_GPIO3C4_E_SHIFT = 12, + GRF_GPIO3C4_E_MASK = 7 << GRF_GPIO3C4_E_SHIFT, + GRF_GPIO3C5_E0_SHIFT = 15, + GRF_GPIO3C5_E0_MASK = 1 << GRF_GPIO3C5_E0_SHIFT, + + /* GRF_GPIO3C_E2 */ + GRF_GPIO3C5_E12_SHIFT = 0, + GRF_GPIO3C5_E12_MASK = 3 << GRF_GPIO3C5_E12_SHIFT, + GRF_GPIO3C6_E_SHIFT = 2, + GRF_GPIO3C6_E_MASK = 7 << GRF_GPIO3C6_E_SHIFT, + GRF_GPIO3C7_E_SHIFT = 5, + GRF_GPIO3C7_E_MASK = 7 << GRF_GPIO3C7_E_SHIFT, + /* GRF_SOC_CON7 */ GRF_UART_DBG_SEL_SHIFT = 10, GRF_UART_DBG_SEL_MASK = 3 << GRF_UART_DBG_SEL_SHIFT, diff --git a/drivers/pinctrl/rockchip/pinctrl_rk3399.c b/drivers/pinctrl/rockchip/pinctrl_rk3399.c index 507bec4..6b62a0c 100644 --- a/drivers/pinctrl/rockchip/pinctrl_rk3399.c +++ b/drivers/pinctrl/rockchip/pinctrl_rk3399.c @@ -232,6 +232,24 @@ static void pinctrl_rk3399_gmac_config(struct rk3399_grf_regs *grf, int mmc_id) rk_clrsetreg(&grf->gpio3c_iomux, GRF_GPIO3C1_SEL_MASK, GRF_MAC_TXCLK << GRF_GPIO3C1_SEL_SHIFT); + + /* Set drive strength for GMAC tx io, value 3 means 13mA */ + rk_clrsetreg(&grf->gpio3_e[0], + GRF_GPIO3A0_E_MASK | GRF_GPIO3A1_E_MASK | + GRF_GPIO3A4_E_MASK | GRF_GPIO3A5_E0_MASK, + 3 << GRF_GPIO3A0_E_SHIFT | + 3 << GRF_GPIO3A1_E_SHIFT | + 3 << GRF_GPIO3A4_E_SHIFT | + 1 << GRF_GPIO3A5_E0_SHIFT); + rk_clrsetreg(&grf->gpio3_e[1], + GRF_GPIO3A5_E12_MASK, + 1 << GRF_GPIO3A5_E12_SHIFT); + rk_clrsetreg(&grf->gpio3_e[2], + GRF_GPIO3B4_E_MASK, + 3 << GRF_GPIO3B4_E_SHIFT); + rk_clrsetreg(&grf->gpio3_e[4], + GRF_GPIO3C1_E_MASK, + 3 << GRF_GPIO3C1_E_SHIFT); } #endif
GMAC controller need to init the tx io driver strength to 13mA, just like the description in dts pinctrl node, or else the controller may only work in 100MHz Mode, and fail to work at 1000MHz mode. Signed-off-by: Kever Yang <kever.yang@rock-chips.com> --- arch/arm/include/asm/arch-rockchip/grf_rk3399.h | 75 +++++++++++++++++++++++-- drivers/pinctrl/rockchip/pinctrl_rk3399.c | 18 ++++++ 2 files changed, 89 insertions(+), 4 deletions(-)