| Message ID | 1517660196-21802-2-git-send-email-david.wu@rock-chips.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Philipp Tomsich |
| Headers | show |
| Series | Add integrated phy support for rk322x and rk3328 | expand |
> Some Socs both have rgmii and rmii interface, so we need to > separate their speed setting. > > Signed-off-by: David Wu <david.wu@rock-chips.com> > --- > > drivers/net/gmac_rockchip.c | 62 +++++++++++++++++++++++++++++++-------------- > 1 file changed, 43 insertions(+), 19 deletions(-) > Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
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@rock-chips.com> > Acked-by: Philipp Tomsich <philipp.tomsich@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? > 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, > }; > >
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@rock-chips.com> >> Acked-by: Philipp Tomsich <philipp.tomsich@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, >> }; >> >> > > >
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); 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; + 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, };
Some Socs both have rgmii and rmii interface, so we need to separate their speed setting. Signed-off-by: David Wu <david.wu@rock-chips.com> --- drivers/net/gmac_rockchip.c | 62 +++++++++++++++++++++++++++++++-------------- 1 file changed, 43 insertions(+), 19 deletions(-)