[U-Boot] [U-Boot, 01/14] net: rockchip: Separate rmii and rgmii speed setup

[David Wu]

Hi Philipp,

在 2018年02月19日 03:36, Philipp Tomsich 写道:
> 
> 
> On Sat, 3 Feb 2018, David Wu wrote:
> 
>> Some Socs both have rgmii and rmii interface, so we need to
>> separate their speed setting.
>>
>> Signed-off-by: David Wu <david.wu at rock-chips.com>
>> Acked-by: Philipp Tomsich <philipp.tomsich at theobroma-systems.com>
>> ---
>>
>> drivers/net/gmac_rockchip.c | 62 
>> +++++++++++++++++++++++++++++++--------------
>> 1 file changed, 43 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/net/gmac_rockchip.c b/drivers/net/gmac_rockchip.c
>> index 683e820..4396ca1 100644
>> --- a/drivers/net/gmac_rockchip.c
>> +++ b/drivers/net/gmac_rockchip.c
>> @@ -40,7 +40,10 @@ struct gmac_rockchip_platdata {
>> };
>>
>> struct rk_gmac_ops {
>> -    int (*fix_mac_speed)(struct dw_eth_dev *priv);
>> +    int (*fix_rmii_speed)(struct gmac_rockchip_platdata *pdata,
>> +                  struct dw_eth_dev *priv);
>> +    int (*fix_rgmii_speed)(struct gmac_rockchip_platdata *pdata,
>> +                   struct dw_eth_dev *priv);
> 
> Why can't this be a single fix_mac_speed function that does the right 
> thing both for RMII and RGMII depending on the platdata?

I think this part is similar to the kernel code, and it's better to 
maintain.

> 
>>     void (*set_to_rmii)(struct gmac_rockchip_platdata *pdata);
>>     void (*set_to_rgmii)(struct gmac_rockchip_platdata *pdata);
>> };
>> @@ -70,7 +73,8 @@ static int gmac_rockchip_ofdata_to_platdata(struct 
>> udevice *dev)
>>     return designware_eth_ofdata_to_platdata(dev);
>> }
>>
>> -static int rk3228_gmac_fix_mac_speed(struct dw_eth_dev *priv)
>> +static int rk3228_gmac_fix_rgmii_speed(struct gmac_rockchip_platdata 
>> *pdata,
>> +                       struct dw_eth_dev *priv)
>> {
>>     struct rk322x_grf *grf;
>>     int clk;
>> @@ -103,7 +107,8 @@ static int rk3228_gmac_fix_mac_speed(struct 
>> dw_eth_dev *priv)
>>     return 0;
>> }
>>
>> -static int rk3288_gmac_fix_mac_speed(struct dw_eth_dev *priv)
>> +static int rk3288_gmac_fix_rgmii_speed(struct gmac_rockchip_platdata 
>> *pdata,
>> +                       struct dw_eth_dev *priv)
>> {
>>     struct rk3288_grf *grf;
>>     int clk;
>> @@ -129,7 +134,8 @@ static int rk3288_gmac_fix_mac_speed(struct 
>> dw_eth_dev *priv)
>>     return 0;
>> }
>>
>> -static int rk3328_gmac_fix_mac_speed(struct dw_eth_dev *priv)
>> +static int rk3328_gmac_fix_rgmii_speed(struct gmac_rockchip_platdata 
>> *pdata,
>> +                       struct dw_eth_dev *priv)
>> {
>>     struct rk3328_grf_regs *grf;
>>     int clk;
>> @@ -162,7 +168,8 @@ static int rk3328_gmac_fix_mac_speed(struct 
>> dw_eth_dev *priv)
>>     return 0;
>> }
>>
>> -static int rk3368_gmac_fix_mac_speed(struct dw_eth_dev *priv)
>> +static int rk3368_gmac_fix_rgmii_speed(struct gmac_rockchip_platdata 
>> *pdata,
>> +                       struct dw_eth_dev *priv)
>> {
>>     struct rk3368_grf *grf;
>>     int clk;
>> @@ -194,7 +201,8 @@ static int rk3368_gmac_fix_mac_speed(struct 
>> dw_eth_dev *priv)
>>     return 0;
>> }
>>
>> -static int rk3399_gmac_fix_mac_speed(struct dw_eth_dev *priv)
>> +static int rk3399_gmac_fix_rgmii_speed(struct gmac_rockchip_platdata 
>> *pdata,
>> +                       struct dw_eth_dev *priv)
>> {
>>     struct rk3399_grf_regs *grf;
>>     int clk;
>> @@ -220,7 +228,8 @@ static int rk3399_gmac_fix_mac_speed(struct 
>> dw_eth_dev *priv)
>>     return 0;
>> }
>>
>> -static int rv1108_set_rmii_speed(struct dw_eth_dev *priv)
>> +static int rv1108_gmac_fix_rmii_speed(struct gmac_rockchip_platdata 
>> *pdata,
>> +                      struct dw_eth_dev *priv)
>> {
>>     struct rv1108_grf *grf;
>>     int clk, speed;
>> @@ -489,7 +498,7 @@ static int gmac_rockchip_probe(struct udevice *dev)
>>
>>         break;
>>     default:
>> -        debug("NO interface defined!\n");
>> +        debug("%s: NO interface defined!\n", __func__);
>>         return -ENXIO;
>>     }
>>
>> @@ -498,18 +507,33 @@ static int gmac_rockchip_probe(struct udevice *dev)
>>
>> static int gmac_rockchip_eth_start(struct udevice *dev)
>> {
>> -    struct eth_pdata *pdata = dev_get_platdata(dev);
>> +    struct eth_pdata *eth_pdata = dev_get_platdata(dev);
>>     struct dw_eth_dev *priv = dev_get_priv(dev);
>>     struct rk_gmac_ops *ops =
>>         (struct rk_gmac_ops *)dev_get_driver_data(dev);
>> +    struct gmac_rockchip_platdata *pdata = dev_get_platdata(dev);
>>     int ret;
>>
>> -    ret = designware_eth_init(priv, pdata->enetaddr);
>> -    if (ret)
>> -        return ret;
>> -    ret = ops->fix_mac_speed(priv);
>> +    ret = designware_eth_init(priv, eth_pdata->enetaddr);
>>     if (ret)
>>         return ret;
>> +
>> +    switch (eth_pdata->phy_interface) {
>> +    case PHY_INTERFACE_MODE_RGMII:
>> +        ret = ops->fix_rgmii_speed(pdata, priv);
>> +        if (ret)
>> +            return ret;
>> +        break;
>> +    case PHY_INTERFACE_MODE_RMII:
>> +        ret = ops->fix_rmii_speed(pdata, priv);
>> +        if (ret)
>> +            return ret;
>> +        break;
> 
> Looking at this, the fix_mac_speed()-function could look into pdata to 
> determine what needs to be done... no need to complicate the common code 
> path with this.
> 
>> +    default:
>> +        debug("%s: NO interface defined!\n", __func__);
>> +        return -ENXIO;
>> +    }
>> +
>>     ret = designware_eth_enable(priv);
>>     if (ret)
>>         return ret;
>> @@ -527,32 +551,32 @@ const struct eth_ops gmac_rockchip_eth_ops = {
>> };
>>
>> const struct rk_gmac_ops rk3228_gmac_ops = {
>> -    .fix_mac_speed = rk3228_gmac_fix_mac_speed,
>> +    .fix_rgmii_speed = rk3228_gmac_fix_rgmii_speed,
>>     .set_to_rgmii = rk3228_gmac_set_to_rgmii,
>> };
>>
>> const struct rk_gmac_ops rk3288_gmac_ops = {
>> -    .fix_mac_speed = rk3288_gmac_fix_mac_speed,
>> +    .fix_rgmii_speed = rk3288_gmac_fix_rgmii_speed,
>>     .set_to_rgmii = rk3288_gmac_set_to_rgmii,
>> };
>>
>> const struct rk_gmac_ops rk3328_gmac_ops = {
>> -    .fix_mac_speed = rk3328_gmac_fix_mac_speed,
>> +    .fix_rgmii_speed = rk3328_gmac_fix_rgmii_speed,
>>     .set_to_rgmii = rk3328_gmac_set_to_rgmii,
>> };
>>
>> const struct rk_gmac_ops rk3368_gmac_ops = {
>> -    .fix_mac_speed = rk3368_gmac_fix_mac_speed,
>> +    .fix_rgmii_speed = rk3368_gmac_fix_rgmii_speed,
>>     .set_to_rgmii = rk3368_gmac_set_to_rgmii,
>> };
>>
>> const struct rk_gmac_ops rk3399_gmac_ops = {
>> -    .fix_mac_speed = rk3399_gmac_fix_mac_speed,
>> +    .fix_rgmii_speed = rk3399_gmac_fix_rgmii_speed,
>>     .set_to_rgmii = rk3399_gmac_set_to_rgmii,
>> };
>>
>> const struct rk_gmac_ops rv1108_gmac_ops = {
>> -    .fix_mac_speed = rv1108_set_rmii_speed,
>> +    .fix_rmii_speed = rv1108_gmac_fix_rmii_speed,
>>     .set_to_rmii = rv1108_gmac_set_to_rmii,
>> };
>>
>>
> 
> 
>