| Message ID | E1qgmkp-007Z4s-GL@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: stmmac: add and use library for setting clock | expand |
On Thu, Sep 14, 2023 at 02:51:35PM +0100, Russell King (Oracle) wrote: > Use stmmac_set_tx_clk_gmii(). > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- > .../net/ethernet/stmicro/stmmac/dwmac-rk.c | 60 +++++-------------- > 1 file changed, 16 insertions(+), 44 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > index d920a50dd16c..5731a73466eb 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > @@ -1081,28 +1081,14 @@ static void rk3568_set_gmac_speed(struct rk_priv_data *bsp_priv, int speed) > { > struct clk *clk_mac_speed = bsp_priv->clks[RK_CLK_MAC_SPEED].clk; > struct device *dev = &bsp_priv->pdev->dev; > - unsigned long rate; > - int ret; > - > - switch (speed) { > - case 10: > - rate = 2500000; > - break; > - case 100: > - rate = 25000000; > - break; > - case 1000: > - rate = 125000000; > - break; > - default: > - dev_err(dev, "unknown speed value for GMAC speed=%d", speed); > - return; > - } > - > - ret = clk_set_rate(clk_mac_speed, rate); > - if (ret) > - dev_err(dev, "%s: set clk_mac_speed rate %ld failed %d\n", > - __func__, rate, ret); > + int err; > + > + err = stmmac_set_tx_clk_gmii(clk_mac_speed, speed); > + if (err == -ENOTSUPP) > + dev_err(dev, "invalid speed %uMbps\n", speed); > + else if (err) > + dev_err(dev, "failed to set tx rate for speed %uMbps: %pe\n", These type specifiers should have been '%d' since the speed variable is of the signed integer type here. -Serge(y) > + speed, ERR_PTR(err)); > } > > static const struct rk_gmac_ops rk3568_ops = { > @@ -1387,28 +1373,14 @@ static void rv1126_set_rgmii_speed(struct rk_priv_data *bsp_priv, int speed) > { > struct clk *clk_mac_speed = bsp_priv->clks[RK_CLK_MAC_SPEED].clk; > struct device *dev = &bsp_priv->pdev->dev; > - unsigned long rate; > - int ret; > - > - switch (speed) { > - case 10: > - rate = 2500000; > - break; > - case 100: > - rate = 25000000; > - break; > - case 1000: > - rate = 125000000; > - break; > - default: > - dev_err(dev, "unknown speed value for RGMII speed=%d", speed); > - return; > - } > - > - ret = clk_set_rate(clk_mac_speed, rate); > - if (ret) > - dev_err(dev, "%s: set clk_mac_speed rate %ld failed %d\n", > - __func__, rate, ret); > + int err; > + > + err = stmmac_set_tx_clk_gmii(clk_mac_speed, speed); > + if (err == -ENOTSUPP) > + dev_err(dev, "invalid speed %dMbps\n", speed); > + else if (err) > + dev_err(dev, "failed to set tx rate for speed %dMbps: %pe\n", > + speed, ERR_PTR(err)); > } > > static void rv1126_set_rmii_speed(struct rk_priv_data *bsp_priv, int speed) > -- > 2.30.2 > >
On Thu, Sep 14, 2023 at 05:37:13PM +0300, Serge Semin wrote: > On Thu, Sep 14, 2023 at 02:51:35PM +0100, Russell King (Oracle) wrote: > > Use stmmac_set_tx_clk_gmii(). > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > --- > > .../net/ethernet/stmicro/stmmac/dwmac-rk.c | 60 +++++-------------- > > 1 file changed, 16 insertions(+), 44 deletions(-) > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > > index d920a50dd16c..5731a73466eb 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > > @@ -1081,28 +1081,14 @@ static void rk3568_set_gmac_speed(struct rk_priv_data *bsp_priv, int speed) > > { > > struct clk *clk_mac_speed = bsp_priv->clks[RK_CLK_MAC_SPEED].clk; > > struct device *dev = &bsp_priv->pdev->dev; > > - unsigned long rate; > > - int ret; > > - > > - switch (speed) { > > - case 10: > > - rate = 2500000; > > - break; > > - case 100: > > - rate = 25000000; > > - break; > > - case 1000: > > - rate = 125000000; > > - break; > > - default: > > - dev_err(dev, "unknown speed value for GMAC speed=%d", speed); > > - return; > > - } > > - > > - ret = clk_set_rate(clk_mac_speed, rate); > > - if (ret) > > - dev_err(dev, "%s: set clk_mac_speed rate %ld failed %d\n", > > - __func__, rate, ret); > > + int err; > > + > > + err = stmmac_set_tx_clk_gmii(clk_mac_speed, speed); > > + if (err == -ENOTSUPP) > > > + dev_err(dev, "invalid speed %uMbps\n", speed); > > + else if (err) > > + dev_err(dev, "failed to set tx rate for speed %uMbps: %pe\n", > > These type specifiers should have been '%d' since the speed variable > is of the signed integer type here. Okay, having re-reviewed the changes, I'm changing them _all_ back to be %d, because that is the _right_ thing. It is *not* unsigned, even if fix_mac_speed() thinks that it is. It isn't. It's signed, and it's stmmac bollocks implicitly casting it to unsigned - and that is _wrong_. So, on that point, my original submission was more correct than this one, and you led me astray.
On Thu, Sep 14, 2023 at 04:03:17PM +0100, Russell King (Oracle) wrote: > On Thu, Sep 14, 2023 at 05:37:13PM +0300, Serge Semin wrote: > > On Thu, Sep 14, 2023 at 02:51:35PM +0100, Russell King (Oracle) wrote: > > > Use stmmac_set_tx_clk_gmii(). > > > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > > --- > > > .../net/ethernet/stmicro/stmmac/dwmac-rk.c | 60 +++++-------------- > > > 1 file changed, 16 insertions(+), 44 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > > > index d920a50dd16c..5731a73466eb 100644 > > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > > > @@ -1081,28 +1081,14 @@ static void rk3568_set_gmac_speed(struct rk_priv_data *bsp_priv, int speed) > > > { > > > struct clk *clk_mac_speed = bsp_priv->clks[RK_CLK_MAC_SPEED].clk; > > > struct device *dev = &bsp_priv->pdev->dev; > > > - unsigned long rate; > > > - int ret; > > > - > > > - switch (speed) { > > > - case 10: > > > - rate = 2500000; > > > - break; > > > - case 100: > > > - rate = 25000000; > > > - break; > > > - case 1000: > > > - rate = 125000000; > > > - break; > > > - default: > > > - dev_err(dev, "unknown speed value for GMAC speed=%d", speed); > > > - return; > > > - } > > > - > > > - ret = clk_set_rate(clk_mac_speed, rate); > > > - if (ret) > > > - dev_err(dev, "%s: set clk_mac_speed rate %ld failed %d\n", > > > - __func__, rate, ret); > > > + int err; > > > + > > > + err = stmmac_set_tx_clk_gmii(clk_mac_speed, speed); > > > + if (err == -ENOTSUPP) > > > > > + dev_err(dev, "invalid speed %uMbps\n", speed); > > > + else if (err) > > > + dev_err(dev, "failed to set tx rate for speed %uMbps: %pe\n", > > > > These type specifiers should have been '%d' since the speed variable > > is of the signed integer type here. > > Okay, having re-reviewed the changes, I'm changing them _all_ back to > be %d, because that is the _right_ thing. It is *not* unsigned, even > if fix_mac_speed() thinks that it is. It isn't. It's signed, and it's > stmmac bollocks implicitly casting it to unsigned - and that is > _wrong_. Yes, stmmac is wrong in casting it to the unsigned type, but even seeing the original type is intended to be signed doesn't mean the qualifier should be fixed separately from the variables type and function prototypes. It will cause even more confusion. IMO the best way would be to fix the plat_stmmacenet_data->fix_mac_speed() prototype and the respective methods in the glue drivers. But it would be too bulky and most likely out of your interest to be done. So I would still have the variables type and the format qualifier type matching here and in the rest of the drivers especially seeing the original code in the imx, starfive, rk, QoS Eth LLDDs sticks to the convention described by me. -Serge(y) > > So, on that point, my original submission was more correct than this > one, and you led me astray. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Thu, Sep 14, 2023 at 06:22:33PM +0300, Serge Semin wrote: > On Thu, Sep 14, 2023 at 04:03:17PM +0100, Russell King (Oracle) wrote: > > On Thu, Sep 14, 2023 at 05:37:13PM +0300, Serge Semin wrote: > > > On Thu, Sep 14, 2023 at 02:51:35PM +0100, Russell King (Oracle) wrote: > > > > Use stmmac_set_tx_clk_gmii(). > > > > > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > > > --- > > > > .../net/ethernet/stmicro/stmmac/dwmac-rk.c | 60 +++++-------------- > > > > 1 file changed, 16 insertions(+), 44 deletions(-) > > > > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > > > > index d920a50dd16c..5731a73466eb 100644 > > > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > > > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > > > > @@ -1081,28 +1081,14 @@ static void rk3568_set_gmac_speed(struct rk_priv_data *bsp_priv, int speed) > > > > { > > > > struct clk *clk_mac_speed = bsp_priv->clks[RK_CLK_MAC_SPEED].clk; > > > > struct device *dev = &bsp_priv->pdev->dev; > > > > - unsigned long rate; > > > > - int ret; > > > > - > > > > - switch (speed) { > > > > - case 10: > > > > - rate = 2500000; > > > > - break; > > > > - case 100: > > > > - rate = 25000000; > > > > - break; > > > > - case 1000: > > > > - rate = 125000000; > > > > - break; > > > > - default: > > > > - dev_err(dev, "unknown speed value for GMAC speed=%d", speed); > > > > - return; > > > > - } > > > > - > > > > - ret = clk_set_rate(clk_mac_speed, rate); > > > > - if (ret) > > > > - dev_err(dev, "%s: set clk_mac_speed rate %ld failed %d\n", > > > > - __func__, rate, ret); > > > > + int err; > > > > + > > > > + err = stmmac_set_tx_clk_gmii(clk_mac_speed, speed); > > > > + if (err == -ENOTSUPP) > > > > > > > + dev_err(dev, "invalid speed %uMbps\n", speed); > > > > + else if (err) > > > > + dev_err(dev, "failed to set tx rate for speed %uMbps: %pe\n", > > > > > > These type specifiers should have been '%d' since the speed variable > > > is of the signed integer type here. > > > > > Okay, having re-reviewed the changes, I'm changing them _all_ back to > > be %d, because that is the _right_ thing. It is *not* unsigned, even > > if fix_mac_speed() thinks that it is. It isn't. It's signed, and it's > > stmmac bollocks implicitly casting it to unsigned - and that is > > _wrong_. > > Yes, stmmac is wrong in casting it to the unsigned type, but even > seeing the original type is intended to be signed doesn't mean the > qualifier should be fixed separately from the variables type and > function prototypes. It will cause even more confusion. IMO the best > way would be to fix the plat_stmmacenet_data->fix_mac_speed() > prototype and the respective methods in the glue drivers. But it would > be too bulky and most likely out of your interest to be done. So I > would still have the variables type and the format qualifier type > matching here and in the rest of the drivers especially seeing the > original code in the imx, starfive, rk, QoS Eth LLDDs sticks to the > convention described by me. I won't be doing that, sorry. If that's not acceptable, then I'm junking this series. What I will be doing is getting rid of as many users of fix_mac_speed() as possible, but that's for a future patch series.
On Thu, Sep 14, 2023 at 04:27:19PM +0100, Russell King (Oracle) wrote: > I won't be doing that, sorry. If that's not acceptable, then I'm > junking this series. In fact, no, I'm making that decision now. I have 42 patches. I'm deleting them all because I just can't be bothered with the hassle of trying to improve this crappy driver. Have a good day.
From: Russell King (Oracle) <linux@armlinux.org.uk> Date: Thu, Sep 14, 2023 at 16:30:11 > On Thu, Sep 14, 2023 at 04:27:19PM +0100, Russell King (Oracle) wrote: > > I won't be doing that, sorry. If that's not acceptable, then I'm > > junking this series. > > In fact, no, I'm making that decision now. I have 42 patches. I'm > deleting them all because I just can't be bothered with the hassle > of trying to improve this crappy driver. Hi Russell, Serge, Jakub, My apologies for not being that active on the review. I totally understand there's a lot of revamps needed on "stmmac", somethings may even need to be totally re-written. I'm also aware that Russell has contributed significantly for this process and was of great help when we first switched "stmmac" to phylink. So, my 5-cents here is that, on this stage, any contribution on "stmmac" is welcomed and we shouldn't try to ask for more but focus instead on small steps. Thanks, Jose
Russel, Jose On Thu, Sep 14, 2023 at 04:01:41PM +0000, Jose Abreu wrote: > From: Russell King (Oracle) <linux@armlinux.org.uk> > Date: Thu, Sep 14, 2023 at 16:30:11 > > > On Thu, Sep 14, 2023 at 04:27:19PM +0100, Russell King (Oracle) wrote: > > > I won't be doing that, sorry. If that's not acceptable, then I'm > > > junking this series. > > > > In fact, no, I'm making that decision now. I have 42 patches. I'm > > deleting them all because I just can't be bothered with the hassle > > of trying to improve this crappy driver. I am sorry to read that. In no means I intended to cause such reaction, but merely to improve the suggested changes as I see it. Speaking about the stmmac driver. I've got over _200_ cleanup, fix and feature patches in my local repo waiting for me having a free time to be properly prepared and finally submitted for review. So I totally understand your initial desire to improve the driver code. > > Hi Russell, Serge, Jakub, > > My apologies for not being that active on the review. I totally understand > there's a lot of revamps needed on "stmmac", somethings may even > need to be totally re-written. > > I'm also aware that Russell has contributed significantly for this process > and was of great help when we first switched "stmmac" to phylink. > > So, my 5-cents here is that, on this stage, any contribution on > "stmmac" is welcomed and we shouldn't try to ask for more > but focus instead on small steps. I actually thought the driver has been long abandoned seeing how many questionable changes have been accepted. That's why I decided to step in with more detailed reviews for now. Anyway It's up to you to decide. You are the driver maintainer after all. -Serge(y) > > Thanks, > Jose
From: Serge Semin <fancer.lancer@gmail.com> Date: Thu, Sep 14, 2023 at 18:05:09 > I actually thought the driver has been long abandoned seeing how many > questionable changes have been accepted. That's why I decided to step > in with more detailed reviews for now. Anyway It's up to you to > decide. You are the driver maintainer after all. It's up to everyone to decide. I understand your comments on the patchset and agree with most of them but on the topic of changing the entire patchset to add the fix on "plat_stmmacenet_data->fix_mac_speed", I don't think it's on the scope of this series. Thanks, Jose
On Fri, Sep 15, 2023 at 08:38:51AM +0000, Jose Abreu wrote: > From: Serge Semin <fancer.lancer@gmail.com> > Date: Thu, Sep 14, 2023 at 18:05:09 > > > I actually thought the driver has been long abandoned seeing how many > > questionable changes have been accepted. That's why I decided to step > > in with more detailed reviews for now. Anyway It's up to you to > > decide. You are the driver maintainer after all. > > It's up to everyone to decide. I understand your comments on the patchset > and agree with most of them Ok. Thanks for clarification. I'll keep reviewing the bits then submitted for the STMMAC driver based on my knowledges of the driver guts and the DW GMAC/XGMAC/Eth QoS IP-cores implementation. > but on the topic of changing the entire > patchset to add the fix on "plat_stmmacenet_data->fix_mac_speed", > I don't think it's on the scope of this series. That's what I meant in my comment. Of course it's out of the series scope. -Serge(y) > > Thanks, > Jose
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c index d920a50dd16c..5731a73466eb 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c @@ -1081,28 +1081,14 @@ static void rk3568_set_gmac_speed(struct rk_priv_data *bsp_priv, int speed) { struct clk *clk_mac_speed = bsp_priv->clks[RK_CLK_MAC_SPEED].clk; struct device *dev = &bsp_priv->pdev->dev; - unsigned long rate; - int ret; - - switch (speed) { - case 10: - rate = 2500000; - break; - case 100: - rate = 25000000; - break; - case 1000: - rate = 125000000; - break; - default: - dev_err(dev, "unknown speed value for GMAC speed=%d", speed); - return; - } - - ret = clk_set_rate(clk_mac_speed, rate); - if (ret) - dev_err(dev, "%s: set clk_mac_speed rate %ld failed %d\n", - __func__, rate, ret); + int err; + + err = stmmac_set_tx_clk_gmii(clk_mac_speed, speed); + if (err == -ENOTSUPP) + dev_err(dev, "invalid speed %uMbps\n", speed); + else if (err) + dev_err(dev, "failed to set tx rate for speed %uMbps: %pe\n", + speed, ERR_PTR(err)); } static const struct rk_gmac_ops rk3568_ops = { @@ -1387,28 +1373,14 @@ static void rv1126_set_rgmii_speed(struct rk_priv_data *bsp_priv, int speed) { struct clk *clk_mac_speed = bsp_priv->clks[RK_CLK_MAC_SPEED].clk; struct device *dev = &bsp_priv->pdev->dev; - unsigned long rate; - int ret; - - switch (speed) { - case 10: - rate = 2500000; - break; - case 100: - rate = 25000000; - break; - case 1000: - rate = 125000000; - break; - default: - dev_err(dev, "unknown speed value for RGMII speed=%d", speed); - return; - } - - ret = clk_set_rate(clk_mac_speed, rate); - if (ret) - dev_err(dev, "%s: set clk_mac_speed rate %ld failed %d\n", - __func__, rate, ret); + int err; + + err = stmmac_set_tx_clk_gmii(clk_mac_speed, speed); + if (err == -ENOTSUPP) + dev_err(dev, "invalid speed %dMbps\n", speed); + else if (err) + dev_err(dev, "failed to set tx rate for speed %dMbps: %pe\n", + speed, ERR_PTR(err)); } static void rv1126_set_rmii_speed(struct rk_priv_data *bsp_priv, int speed)
Use stmmac_set_tx_clk_gmii(). Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- .../net/ethernet/stmicro/stmmac/dwmac-rk.c | 60 +++++-------------- 1 file changed, 16 insertions(+), 44 deletions(-)