[PATCH net-next 0/2] net: stmmac: dwmac-starfive: some improvements

[PATCH net-next 0/2] net: stmmac: dwmac-starfive: some improvements

[Jisheng Zhang]

This series is to improve the dwmac-starfive driver by correcting
error handling and removing unnecessary clk_get_rate().

Jisheng Zhang (2):
  net: stmmac: dwmac-starfive: improve error handling during probe
  net: stmmac: dwmac-starfive: remove unnecessary clk_get_rate()

 .../ethernet/stmicro/stmmac/dwmac-starfive.c  | 38 +++++++++++--------
 1 file changed, 22 insertions(+), 16 deletions(-)

-- 
2.40.1

[PATCH net-next 1/2] net: stmmac: dwmac-starfive: improve error handling during probe

[Jisheng Zhang]

After stmmac_probe_config_dt() succeeds, when error happens later,
stmmac_remove_config_dt() needs to be called for proper error handling.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 .../ethernet/stmicro/stmmac/dwmac-starfive.c  | 34 ++++++++++++-------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
index 892612564694..b68f42795eaa 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
@@ -111,18 +111,24 @@ static int starfive_dwmac_probe(struct platform_device *pdev)
 				     "dt configuration failed\n");
 
 	dwmac = devm_kzalloc(&pdev->dev, sizeof(*dwmac), GFP_KERNEL);
-	if (!dwmac)
-		return -ENOMEM;
+	if (!dwmac) {
+		err = -ENOMEM;
+		goto err_remove_config_dt;
+	}
 
 	dwmac->clk_tx = devm_clk_get_enabled(&pdev->dev, "tx");
-	if (IS_ERR(dwmac->clk_tx))
-		return dev_err_probe(&pdev->dev, PTR_ERR(dwmac->clk_tx),
-				     "error getting tx clock\n");
+	if (IS_ERR(dwmac->clk_tx)) {
+		err = dev_err_probe(&pdev->dev, PTR_ERR(dwmac->clk_tx),
+				    "error getting tx clock\n");
+		goto err_remove_config_dt;
+	}
 
 	clk_gtx = devm_clk_get_enabled(&pdev->dev, "gtx");
-	if (IS_ERR(clk_gtx))
-		return dev_err_probe(&pdev->dev, PTR_ERR(clk_gtx),
-				     "error getting gtx clock\n");
+	if (IS_ERR(clk_gtx)) {
+		err = dev_err_probe(&pdev->dev, PTR_ERR(clk_gtx),
+				    "error getting gtx clock\n");
+		goto err_remove_config_dt;
+	}
 
 	/* Generally, the rgmii_tx clock is provided by the internal clock,
 	 * which needs to match the corresponding clock frequency according
@@ -139,15 +145,17 @@ static int starfive_dwmac_probe(struct platform_device *pdev)
 
 	err = starfive_dwmac_set_mode(plat_dat);
 	if (err)
-		return err;
+		goto err_remove_config_dt;
 
 	err = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
-	if (err) {
-		stmmac_remove_config_dt(pdev, plat_dat);
-		return err;
-	}
+	if (err)
+		goto err_remove_config_dt;
 
 	return 0;
+
+err_remove_config_dt:
+	stmmac_remove_config_dt(pdev, plat_dat);
+	return err;
 }
 
 static const struct of_device_id starfive_dwmac_match[] = {
-- 
2.40.1

Re: [PATCH net-next 1/2] net: stmmac: dwmac-starfive: improve error handling during probe

[Russell King (Oracle)]

On Sun, Aug 27, 2023 at 09:41:49PM +0800, Jisheng Zhang wrote:
> After stmmac_probe_config_dt() succeeds, when error happens later,
> stmmac_remove_config_dt() needs to be called for proper error handling.

Have you thought about converting to use devm_stmmac_probe_config_dt()
which will call stmmac_remove_config_dt() if the probe fails or when
the device is unbound?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 1/2] net: stmmac: dwmac-starfive: improve error handling during probe

[Emil Renner Berthing]

On Sun, 27 Aug 2023 at 15:59, Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
> On Sun, Aug 27, 2023 at 09:41:49PM +0800, Jisheng Zhang wrote:
> > After stmmac_probe_config_dt() succeeds, when error happens later,
> > stmmac_remove_config_dt() needs to be called for proper error handling.
>
> Have you thought about converting to use devm_stmmac_probe_config_dt()
> which will call stmmac_remove_config_dt() if the probe fails or when
> the device is unbound?

+1. Using devm_stmmac_probe_config_dt() seems like a better solution.

> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

[PATCH net-next 2/2] net: stmmac: dwmac-starfive: remove unnecessary clk_get_rate()

[Jisheng Zhang]

In starfive_dwmac_fix_mac_speed(), the rate gotten by clk_get_rate()
is not necessary, remove the clk_get_rate() calling.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
index b68f42795eaa..422138ef565e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
@@ -30,8 +30,6 @@ static void starfive_dwmac_fix_mac_speed(void *priv, unsigned int speed, unsigne
 	unsigned long rate;
 	int err;
 
-	rate = clk_get_rate(dwmac->clk_tx);
-
 	switch (speed) {
 	case SPEED_1000:
 		rate = 125000000;
@@ -44,7 +42,7 @@ static void starfive_dwmac_fix_mac_speed(void *priv, unsigned int speed, unsigne
 		break;
 	default:
 		dev_err(dwmac->dev, "invalid speed %u\n", speed);
-		break;
+		return;
 	}
 
 	err = clk_set_rate(dwmac->clk_tx, rate);
-- 
2.40.1

Re: [PATCH net-next 2/2] net: stmmac: dwmac-starfive: remove unnecessary clk_get_rate()

[Emil Renner Berthing]

On Sun, 27 Aug 2023 at 15:53, Jisheng Zhang <jszhang@kernel.org> wrote:
>
> In starfive_dwmac_fix_mac_speed(), the rate gotten by clk_get_rate()
> is not necessary, remove the clk_get_rate() calling.

Thanks. I suggested this change during the initial review, but someone
wanted the code as it is. I must admit I don't understand why, so
Reviewed-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>

> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
>  drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
> index b68f42795eaa..422138ef565e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
> @@ -30,8 +30,6 @@ static void starfive_dwmac_fix_mac_speed(void *priv, unsigned int speed, unsigne
>         unsigned long rate;
>         int err;
>
> -       rate = clk_get_rate(dwmac->clk_tx);
> -
>         switch (speed) {
>         case SPEED_1000:
>                 rate = 125000000;
> @@ -44,7 +42,7 @@ static void starfive_dwmac_fix_mac_speed(void *priv, unsigned int speed, unsigne
>                 break;
>         default:
>                 dev_err(dwmac->dev, "invalid speed %u\n", speed);
> -               break;
> +               return;
>         }
>
>         err = clk_set_rate(dwmac->clk_tx, rate);
> --
> 2.40.1
>

Re: [PATCH net-next 2/2] net: stmmac: dwmac-starfive: remove unnecessary clk_get_rate()

[Russell King (Oracle)]

On Sun, Aug 27, 2023 at 07:33:10PM +0200, Emil Renner Berthing wrote:
> On Sun, 27 Aug 2023 at 15:53, Jisheng Zhang <jszhang@kernel.org> wrote:
> >
> > In starfive_dwmac_fix_mac_speed(), the rate gotten by clk_get_rate()
> > is not necessary, remove the clk_get_rate() calling.
> 
> Thanks. I suggested this change during the initial review, but someone
> wanted the code as it is. I must admit I don't understand why, so
> Reviewed-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>

The code in starfive_dwmac_fix_mac_speed() is a repeated pattern amongst
many drivers, and having each platform driver re-implement this is not
sane. Those who know me will know that I have a patch that cleans this
all up - moving basically the guts of this to a library function which
platform drivers can make use of.

It's not like the clock rates are somehow special - they're standard for
10M/100M/1G speeds on a GMII-family interface, and the 10M/100M also
share the clock rates with MII.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!