[U-Boot] [PATCH] rockchip: pinctrl: rk3399: add gmac io strength support

[Kever Yang]

Hi Philipp,


On 04/20/2017 04:21 PM, Dr. Philipp Tomsich wrote:
>>
>> On 20 Apr 2017, at 10:15, Kever Yang <kever.yang at rock-chips.com 
>> <mailto:kever.yang at 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 at rock-chips.com 
>> <mailto:kever.yang at 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 at theobroma-systems.com 
> <mailto:philipp.tomsich at theobroma-systems.com>>
>