[PATCH net-next 0/6] net: stmmac: add and use library for setting clock

[PATCH net-next 0/6] net: stmmac: add and use library for setting clock

[Russell King (Oracle)]

Hi,

There is a common theme throughout several "bsps" in the stmmac driver
which all code up the same thing: for 10M, 100M and 1G, select the
appropriate 2.5MHz, 25MHz, or 125MHz clock.

Rather than having every BSP implement the same thing but slightly
differently, let's provide a single implementation which is passed
the struct clk and the speed, and have that do the speed to clock
rate decode.

Note: only build tested.

 drivers/net/ethernet/stmicro/stmmac/Makefile       |  2 +-
 .../ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c    | 37 ++++---------
 drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c    | 27 +++-------
 .../net/ethernet/stmicro/stmmac/dwmac-intel-plat.c | 35 ++++---------
 drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c     | 61 ++++++----------------
 .../net/ethernet/stmicro/stmmac/dwmac-starfive.c   | 29 +++-------
 .../net/ethernet/stmicro/stmmac/stmmac_plat_lib.c  | 29 ++++++++++
 .../net/ethernet/stmicro/stmmac/stmmac_plat_lib.h  |  8 +++
 8 files changed, 91 insertions(+), 137 deletions(-)
 create mode 100644 drivers/net/ethernet/stmicro/stmmac/stmmac_plat_lib.c
 create mode 100644 drivers/net/ethernet/stmicro/stmmac/stmmac_plat_lib.h

-- 
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 1/6] net: stmmac: add platform library

[Russell King (Oracle)]

Add a platform library of helper functions for common traits in the
platform drivers. Currently, this is setting the tx clock.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/Makefile  |  2 +-
 .../ethernet/stmicro/stmmac/stmmac_plat_lib.c | 29 +++++++++++++++++++
 .../ethernet/stmicro/stmmac/stmmac_plat_lib.h |  8 +++++
 3 files changed, 38 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/ethernet/stmicro/stmmac/stmmac_plat_lib.c
 create mode 100644 drivers/net/ethernet/stmicro/stmmac/stmmac_plat_lib.h

diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
index 5b57aee19267..ba2cbfa0c9d1 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Makefile
+++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
@@ -6,7 +6,7 @@ stmmac-objs:= stmmac_main.o stmmac_ethtool.o stmmac_mdio.o ring_mode.o	\
 	      mmc_core.o stmmac_hwtstamp.o stmmac_ptp.o dwmac4_descs.o	\
 	      dwmac4_dma.o dwmac4_lib.o dwmac4_core.o dwmac5.o hwif.o \
 	      stmmac_tc.o dwxgmac2_core.o dwxgmac2_dma.o dwxgmac2_descs.o \
-	      stmmac_xdp.o \
+	      stmmac_xdp.o stmmac_plat_lib.o \
 	      $(stmmac-y)
 
 stmmac-$(CONFIG_STMMAC_SELFTESTS) += stmmac_selftests.o
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_plat_lib.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_plat_lib.c
new file mode 100644
index 000000000000..abb9f512bb0e
--- /dev/null
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_plat_lib.c
@@ -0,0 +1,29 @@
+#include <linux/stmmac.h>
+#include <linux/clk.h>
+
+#include "stmmac_plat_lib.h"
+
+int dwmac_set_tx_clk_gmii(struct clk *tx_clk, int speed)
+{
+	unsigned long rate;
+
+	switch (speed) {
+	case SPEED_1000:
+		rate = 125000000;
+		break;
+
+	case SPEED_100:
+		rate = 25000000;
+		break;
+
+	case SPEED_10:
+		rate = 2500000;
+		break;
+
+	default:
+		return -ENOTSUPP;
+	}
+
+	return clk_set_rate(tx_clk, rate);
+}
+EXPORT_SYMBOL_GPL(dwmac_set_tx_clk_gmii);
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_plat_lib.h b/drivers/net/ethernet/stmicro/stmmac/stmmac_plat_lib.h
new file mode 100644
index 000000000000..926fdce379b3
--- /dev/null
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_plat_lib.h
@@ -0,0 +1,8 @@
+#ifndef STMMAC_PLAT_LIB_H
+#define STMMAC_PLAT_LIB_H
+
+struct clk;
+
+int dwmac_set_tx_clk_gmii(struct clk *tx_clk, int speed);
+
+#endif
-- 
2.30.2

[PATCH net-next 2/6] net: stmmac: imx: use dwmac_set_tx_clk_gmii()

[Russell King (Oracle)]

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 27 ++++++-------------
 1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
index df34e34cc14f..d2569faf7cc3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
@@ -21,6 +21,7 @@
 #include <linux/stmmac.h>
 
 #include "stmmac_platform.h"
+#include "stmmac_plat_lib.h"
 
 #define GPR_ENET_QOS_INTF_MODE_MASK	GENMASK(21, 16)
 #define GPR_ENET_QOS_INTF_SEL_MII	(0x0 << 16)
@@ -186,7 +187,6 @@ static void imx_dwmac_fix_speed(void *priv, unsigned int speed, unsigned int mod
 {
 	struct plat_stmmacenet_data *plat_dat;
 	struct imx_priv_data *dwmac = priv;
-	unsigned long rate;
 	int err;
 
 	plat_dat = dwmac->plat_dat;
@@ -196,24 +196,13 @@ static void imx_dwmac_fix_speed(void *priv, unsigned int speed, unsigned int mod
 	    (plat_dat->mac_interface == PHY_INTERFACE_MODE_MII))
 		return;
 
-	switch (speed) {
-	case SPEED_1000:
-		rate = 125000000;
-		break;
-	case SPEED_100:
-		rate = 25000000;
-		break;
-	case SPEED_10:
-		rate = 2500000;
-		break;
-	default:
-		dev_err(dwmac->dev, "invalid speed %u\n", speed);
-		return;
-	}
-
-	err = clk_set_rate(dwmac->clk_tx, rate);
-	if (err < 0)
-		dev_err(dwmac->dev, "failed to set tx rate %lu\n", rate);
+	err = dwmac_set_tx_clk_gmii(dwmac->clk_tx, speed);
+	if (err == -ENOTSUPP)
+		dev_err(dwmac->dev, "invalid speed %dMbps\n", speed);
+	else if (err)
+		dev_err(dwmac->dev,
+			"failed to set tx rate for speed %dMbps: %pe\n",
+			speed, ERR_PTR(err));
 }
 
 static void imx93_dwmac_fix_speed(void *priv, unsigned int speed, unsigned int mode)
-- 
2.30.2

[PATCH net-next 3/6] net: stmmac: intel-plat: use dwmac_set_tx_clk_gmii()

[Russell King (Oracle)]

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 .../stmicro/stmmac/dwmac-intel-plat.c         | 35 +++++--------------
 1 file changed, 9 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
index d352a14f9d48..8cc22f11072e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
@@ -14,6 +14,7 @@
 #include "dwmac4.h"
 #include "stmmac.h"
 #include "stmmac_platform.h"
+#include "stmmac_plat_lib.h"
 
 struct intel_dwmac {
 	struct device *dev;
@@ -31,32 +32,14 @@ struct intel_dwmac_data {
 static void kmb_eth_fix_mac_speed(void *priv, unsigned int speed, unsigned int mode)
 {
 	struct intel_dwmac *dwmac = priv;
-	unsigned long rate;
-	int ret;
-
-	rate = clk_get_rate(dwmac->tx_clk);
-
-	switch (speed) {
-	case SPEED_1000:
-		rate = 125000000;
-		break;
-
-	case SPEED_100:
-		rate = 25000000;
-		break;
-
-	case SPEED_10:
-		rate = 2500000;
-		break;
-
-	default:
-		dev_err(dwmac->dev, "Invalid speed\n");
-		break;
-	}
-
-	ret = clk_set_rate(dwmac->tx_clk, rate);
-	if (ret)
-		dev_err(dwmac->dev, "Failed to configure tx clock rate\n");
+	int err;
+
+	err = dwmac_set_tx_clk_gmii(dwmac->tx_clk, speed);
+	if (err == -ENOTSUPP)
+		dev_err(dwmac->dev, "invalid speed %dMbps\n", speed);
+	else if (err)
+		dev_err(dwmac->dev, "failed to set tx rate for speed %dMbps: %pe\n",
+			speed, ERR_PTR(err));
 }
 
 static const struct intel_dwmac_data kmb_data = {
-- 
2.30.2

[PATCH net-next 4/6] net: stmmac: rk: use dwmac_set_tx_clk_gmii()

[Russell King (Oracle)]

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 .../net/ethernet/stmicro/stmmac/dwmac-rk.c    | 61 ++++++-------------
 1 file changed, 17 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..30172ca3bd06 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
@@ -24,6 +24,7 @@
 #include <linux/pm_runtime.h>
 
 #include "stmmac_platform.h"
+#include "stmmac_plat_lib.h"
 
 struct rk_priv_data;
 struct rk_gmac_ops {
@@ -1081,28 +1082,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 = dwmac_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 const struct rk_gmac_ops rk3568_ops = {
@@ -1387,28 +1374,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 = dwmac_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

[PATCH net-next 5/6] net: stmmac: starfive: use dwmac_set_tx_clk_gmii()

[Russell King (Oracle)]

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 .../ethernet/stmicro/stmmac/dwmac-starfive.c  | 29 +++++--------------
 1 file changed, 8 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
index 9289bb87c3e3..3dc04017e3d3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
@@ -14,6 +14,7 @@
 #include <linux/regmap.h>
 
 #include "stmmac_platform.h"
+#include "stmmac_plat_lib.h"
 
 #define STARFIVE_DWMAC_PHY_INFT_RGMII	0x1
 #define STARFIVE_DWMAC_PHY_INFT_RMII	0x4
@@ -27,29 +28,15 @@ struct starfive_dwmac {
 static void starfive_dwmac_fix_mac_speed(void *priv, unsigned int speed, unsigned int mode)
 {
 	struct starfive_dwmac *dwmac = priv;
-	unsigned long rate;
 	int err;
 
-	rate = clk_get_rate(dwmac->clk_tx);
-
-	switch (speed) {
-	case SPEED_1000:
-		rate = 125000000;
-		break;
-	case SPEED_100:
-		rate = 25000000;
-		break;
-	case SPEED_10:
-		rate = 2500000;
-		break;
-	default:
-		dev_err(dwmac->dev, "invalid speed %u\n", speed);
-		break;
-	}
-
-	err = clk_set_rate(dwmac->clk_tx, rate);
-	if (err)
-		dev_err(dwmac->dev, "failed to set tx rate %lu\n", rate);
+	err = dwmac_set_tx_clk_gmii(dwmac->clk_tx, speed);
+	if (err == -ENOTSUPP)
+		dev_err(dwmac->dev, "invalid speed %dMbps\n", speed);
+	else if (err)
+		dev_err(dwmac->dev,
+			"failed to set tx rate for speed %dMbps: %pe\n",
+			speed, ERR_PTR(err));
 }
 
 static int starfive_dwmac_set_mode(struct plat_stmmacenet_data *plat_dat)
-- 
2.30.2

[PATCH net-next 6/6] net: stmmac: qos-eth: use dwmac_set_tx_clk_gmii()

[Russell King (Oracle)]

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 .../stmicro/stmmac/dwmac-dwc-qos-eth.c        | 37 ++++++-------------
 1 file changed, 11 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
index 61ebf36da13d..a8fae37b9858 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
@@ -22,6 +22,7 @@
 #include <linux/stmmac.h>
 
 #include "stmmac_platform.h"
+#include "stmmac_plat_lib.h"
 #include "dwmac4.h"
 
 struct tegra_eqos {
@@ -181,32 +182,10 @@ static void dwc_qos_remove(struct platform_device *pdev)
 static void tegra_eqos_fix_speed(void *priv, unsigned int speed, unsigned int mode)
 {
 	struct tegra_eqos *eqos = priv;
-	unsigned long rate = 125000000;
-	bool needs_calibration = false;
 	u32 value;
 	int err;
 
-	switch (speed) {
-	case SPEED_1000:
-		needs_calibration = true;
-		rate = 125000000;
-		break;
-
-	case SPEED_100:
-		needs_calibration = true;
-		rate = 25000000;
-		break;
-
-	case SPEED_10:
-		rate = 2500000;
-		break;
-
-	default:
-		dev_err(eqos->dev, "invalid speed %u\n", speed);
-		break;
-	}
-
-	if (needs_calibration) {
+	if (speed == SPEED_1000 || speed == SPEED_100) {
 		/* calibrate */
 		value = readl(eqos->regs + SDMEMCOMPPADCTRL);
 		value |= SDMEMCOMPPADCTRL_PAD_E_INPUT_OR_E_PWRD;
@@ -246,9 +225,15 @@ static void tegra_eqos_fix_speed(void *priv, unsigned int speed, unsigned int mo
 		writel(value, eqos->regs + AUTO_CAL_CONFIG);
 	}
 
-	err = clk_set_rate(eqos->clk_tx, rate);
-	if (err < 0)
-		dev_err(eqos->dev, "failed to set TX rate: %d\n", err);
+	err = dwmac_set_tx_clk_gmii(eqos->clk_tx, speed);
+	if (err == -ENOTSUPP) {
+		dev_err(eqos->dev, "invalid speed %dMbps\n", speed);
+		err = dwmac_set_tx_clk_gmii(eqos->clk_tx, SPEED_1000);
+	} else if (err) {
+		dev_err(eqos->dev,
+			"failed to set tx rate for speed %dMbps: %pe\n",
+			speed, ERR_PTR(err));
+	}
 }
 
 static int tegra_eqos_init(struct platform_device *pdev, void *priv)
-- 
2.30.2

Re: [PATCH net-next 1/6] net: stmmac: add platform library

[Paolo Abeni]

On Mon, 2023-09-11 at 16:29 +0100, Russell King (Oracle) wrote:
> Add a platform library of helper functions for common traits in the
> platform drivers. Currently, this is setting the tx clock.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

FTR, two copies of this series landed on the ML. I'm marking the no-
cover-letter version as Superseded, but please have a look at the
comments there, too.

Thanks,

Paolo

Re: [PATCH net-next 1/6] net: stmmac: add platform library

[Simon Horman]

On Mon, Sep 11, 2023 at 04:29:11PM +0100, Russell King (Oracle) wrote:
> Add a platform library of helper functions for common traits in the
> platform drivers. Currently, this is setting the tx clock.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Hi Russell,

some minor issues raised by checkpatch follow.

> ---
>  drivers/net/ethernet/stmicro/stmmac/Makefile  |  2 +-
>  .../ethernet/stmicro/stmmac/stmmac_plat_lib.c | 29 +++++++++++++++++++
>  .../ethernet/stmicro/stmmac/stmmac_plat_lib.h |  8 +++++
>  3 files changed, 38 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/net/ethernet/stmicro/stmmac/stmmac_plat_lib.c
>  create mode 100644 drivers/net/ethernet/stmicro/stmmac/stmmac_plat_lib.h
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
> index 5b57aee19267..ba2cbfa0c9d1 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/Makefile
> +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
> @@ -6,7 +6,7 @@ stmmac-objs:= stmmac_main.o stmmac_ethtool.o stmmac_mdio.o ring_mode.o	\
>  	      mmc_core.o stmmac_hwtstamp.o stmmac_ptp.o dwmac4_descs.o	\
>  	      dwmac4_dma.o dwmac4_lib.o dwmac4_core.o dwmac5.o hwif.o \
>  	      stmmac_tc.o dwxgmac2_core.o dwxgmac2_dma.o dwxgmac2_descs.o \
> -	      stmmac_xdp.o \
> +	      stmmac_xdp.o stmmac_plat_lib.o \
>  	      $(stmmac-y)
>  
>  stmmac-$(CONFIG_STMMAC_SELFTESTS) += stmmac_selftests.o
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_plat_lib.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_plat_lib.c
> new file mode 100644
> index 000000000000..abb9f512bb0e
> --- /dev/null
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_plat_lib.c
> @@ -0,0 +1,29 @@

Is an SPDX identifier appropriate here?

> +#include <linux/stmmac.h>
> +#include <linux/clk.h>
> +
> +#include "stmmac_plat_lib.h"
> +
> +int dwmac_set_tx_clk_gmii(struct clk *tx_clk, int speed)
> +{
> +	unsigned long rate;
> +
> +	switch (speed) {
> +	case SPEED_1000:
> +		rate = 125000000;
> +		break;
> +
> +	case SPEED_100:
> +		rate = 25000000;
> +		break;
> +
> +	case SPEED_10:
> +		rate = 2500000;
> +		break;
> +
> +	default:
> +		return -ENOTSUPP;

Checkpatch seems to think that EOPNOTSUPP would be more appropriate
as "ENOTSUPP is not a SUSV4 error code".

> +	}
> +
> +	return clk_set_rate(tx_clk, rate);
> +}
> +EXPORT_SYMBOL_GPL(dwmac_set_tx_clk_gmii);

...

Re: [PATCH net-next 1/6] net: stmmac: add platform library

[Russell King (Oracle)]

On Tue, Sep 12, 2023 at 04:52:27PM +0200, Simon Horman wrote:
> On Mon, Sep 11, 2023 at 04:29:11PM +0100, Russell King (Oracle) wrote:
> > +	default:
> > +		return -ENOTSUPP;
> 
> Checkpatch seems to think that EOPNOTSUPP would be more appropriate
> as "ENOTSUPP is not a SUSV4 error code".

It needs to be an error code that clk_set_rate() below isn't going to
return - because if clk_set_rate() does return it, then the users are
going to end up issuing an incorrect error message to the user. I
suspect clk_set_rate() could quite legitimately return -EOPNOTSUPP
or -EINVAL.

Sadly, the CCF implementation of clk_set_rate() doesn't detail what
errors it could return, but it looks like -EBUSY, -EINVAL, or something
from pm_runtime_resume_and_get().

Interestingly, while looking at this, pm_runtime_resume_and_get() can
return '1' if e.g. rpm is disabled and the device is active. It looks
to me like CCF treats that as an error in multiple locations.

> > +	}
> > +
> > +	return clk_set_rate(tx_clk, rate);
> > +}
> > +EXPORT_SYMBOL_GPL(dwmac_set_tx_clk_gmii);

-- 
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/6] net: stmmac: add platform library

[Simon Horman]

On Tue, Sep 12, 2023 at 11:20:55PM +0100, Russell King (Oracle) wrote:
> On Tue, Sep 12, 2023 at 04:52:27PM +0200, Simon Horman wrote:
> > On Mon, Sep 11, 2023 at 04:29:11PM +0100, Russell King (Oracle) wrote:
> > > +	default:
> > > +		return -ENOTSUPP;
> > 
> > Checkpatch seems to think that EOPNOTSUPP would be more appropriate
> > as "ENOTSUPP is not a SUSV4 error code".
> 
> It needs to be an error code that clk_set_rate() below isn't going to
> return - because if clk_set_rate() does return it, then the users are
> going to end up issuing an incorrect error message to the user. I
> suspect clk_set_rate() could quite legitimately return -EOPNOTSUPP
> or -EINVAL.
> 
> Sadly, the CCF implementation of clk_set_rate() doesn't detail what
> errors it could return, but it looks like -EBUSY, -EINVAL, or something
> from pm_runtime_resume_and_get().

Thanks Russell,

Understood.

In that case perhaps ENOTSUPP is not such a bad choice as:
a) it seems rather unlikely CCF would use it; and
b) the scope of usage is well contained - the helper and any direct callers.

No further objections from my side :)

> 
> Interestingly, while looking at this, pm_runtime_resume_and_get() can
> return '1' if e.g. rpm is disabled and the device is active. It looks
> to me like CCF treats that as an error in multiple locations.

The plot thickens...

Re: [PATCH net-next 2/6] net: stmmac: imx: use dwmac_set_tx_clk_gmii()

[Serge Semin]

On Mon, Sep 11, 2023 at 04:29:16PM +0100, Russell King (Oracle) wrote:
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 27 ++++++-------------
>  1 file changed, 8 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> index df34e34cc14f..d2569faf7cc3 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> @@ -21,6 +21,7 @@
>  #include <linux/stmmac.h>
>  
>  #include "stmmac_platform.h"
> +#include "stmmac_plat_lib.h"
>  
>  #define GPR_ENET_QOS_INTF_MODE_MASK	GENMASK(21, 16)
>  #define GPR_ENET_QOS_INTF_SEL_MII	(0x0 << 16)
> @@ -186,7 +187,6 @@ static void imx_dwmac_fix_speed(void *priv, unsigned int speed, unsigned int mod
>  {
>  	struct plat_stmmacenet_data *plat_dat;
>  	struct imx_priv_data *dwmac = priv;
> -	unsigned long rate;
>  	int err;
>  
>  	plat_dat = dwmac->plat_dat;
> @@ -196,24 +196,13 @@ static void imx_dwmac_fix_speed(void *priv, unsigned int speed, unsigned int mod
>  	    (plat_dat->mac_interface == PHY_INTERFACE_MODE_MII))
>  		return;
>  
> -	switch (speed) {
> -	case SPEED_1000:
> -		rate = 125000000;
> -		break;
> -	case SPEED_100:
> -		rate = 25000000;
> -		break;
> -	case SPEED_10:
> -		rate = 2500000;
> -		break;
> -	default:
> -		dev_err(dwmac->dev, "invalid speed %u\n", speed);
> -		return;
> -	}
> -
> -	err = clk_set_rate(dwmac->clk_tx, rate);
> -	if (err < 0)
> -		dev_err(dwmac->dev, "failed to set tx rate %lu\n", rate);
> +	err = dwmac_set_tx_clk_gmii(dwmac->clk_tx, speed);
> +	if (err == -ENOTSUPP)

> +		dev_err(dwmac->dev, "invalid speed %dMbps\n", speed);

The 'speed' variable is of the unsigned int type so the type qualifier is
supposed to be "%u" here.

> +	else if (err)
> +		dev_err(dwmac->dev,

> +			"failed to set tx rate for speed %dMbps: %pe\n",

ditto

-Serge(y)

> +			speed, ERR_PTR(err));
>  }
>  
>  static void imx93_dwmac_fix_speed(void *priv, unsigned int speed, unsigned int mode)
> -- 
> 2.30.2
> 
> 

Re: [PATCH net-next 3/6] net: stmmac: intel-plat: use dwmac_set_tx_clk_gmii()

[Serge Semin]

On Mon, Sep 11, 2023 at 04:29:21PM +0100, Russell King (Oracle) wrote:
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  .../stmicro/stmmac/dwmac-intel-plat.c         | 35 +++++--------------
>  1 file changed, 9 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
> index d352a14f9d48..8cc22f11072e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
> @@ -14,6 +14,7 @@
>  #include "dwmac4.h"
>  #include "stmmac.h"
>  #include "stmmac_platform.h"
> +#include "stmmac_plat_lib.h"
>  
>  struct intel_dwmac {
>  	struct device *dev;
> @@ -31,32 +32,14 @@ struct intel_dwmac_data {
>  static void kmb_eth_fix_mac_speed(void *priv, unsigned int speed, unsigned int mode)
>  {
>  	struct intel_dwmac *dwmac = priv;
> -	unsigned long rate;
> -	int ret;
> -
> -	rate = clk_get_rate(dwmac->tx_clk);
> -
> -	switch (speed) {
> -	case SPEED_1000:
> -		rate = 125000000;
> -		break;
> -
> -	case SPEED_100:
> -		rate = 25000000;
> -		break;
> -
> -	case SPEED_10:
> -		rate = 2500000;
> -		break;
> -
> -	default:
> -		dev_err(dwmac->dev, "Invalid speed\n");
> -		break;
> -	}
> -
> -	ret = clk_set_rate(dwmac->tx_clk, rate);
> -	if (ret)
> -		dev_err(dwmac->dev, "Failed to configure tx clock rate\n");
> +	int err;
> +
> +	err = dwmac_set_tx_clk_gmii(dwmac->tx_clk, speed);
> +	if (err == -ENOTSUPP)

> +		dev_err(dwmac->dev, "invalid speed %dMbps\n", speed);

'%u'?

> +	else if (err)

> +		dev_err(dwmac->dev, "failed to set tx rate for speed %dMbps: %pe\n",

ditto

-Serge(y)

> +			speed, ERR_PTR(err));
>  }
>  
>  static const struct intel_dwmac_data kmb_data = {
> -- 
> 2.30.2
> 
> 

Re: [PATCH net-next 5/6] net: stmmac: starfive: use dwmac_set_tx_clk_gmii()

[Serge Semin]

On Mon, Sep 11, 2023 at 04:29:31PM +0100, Russell King (Oracle) wrote:
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  .../ethernet/stmicro/stmmac/dwmac-starfive.c  | 29 +++++--------------
>  1 file changed, 8 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
> index 9289bb87c3e3..3dc04017e3d3 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
> @@ -14,6 +14,7 @@
>  #include <linux/regmap.h>
>  
>  #include "stmmac_platform.h"
> +#include "stmmac_plat_lib.h"
>  
>  #define STARFIVE_DWMAC_PHY_INFT_RGMII	0x1
>  #define STARFIVE_DWMAC_PHY_INFT_RMII	0x4
> @@ -27,29 +28,15 @@ struct starfive_dwmac {
>  static void starfive_dwmac_fix_mac_speed(void *priv, unsigned int speed, unsigned int mode)
>  {
>  	struct starfive_dwmac *dwmac = priv;
> -	unsigned long rate;
>  	int err;
>  
> -	rate = clk_get_rate(dwmac->clk_tx);
> -
> -	switch (speed) {
> -	case SPEED_1000:
> -		rate = 125000000;
> -		break;
> -	case SPEED_100:
> -		rate = 25000000;
> -		break;
> -	case SPEED_10:
> -		rate = 2500000;
> -		break;
> -	default:
> -		dev_err(dwmac->dev, "invalid speed %u\n", speed);
> -		break;
> -	}
> -
> -	err = clk_set_rate(dwmac->clk_tx, rate);
> -	if (err)
> -		dev_err(dwmac->dev, "failed to set tx rate %lu\n", rate);
> +	err = dwmac_set_tx_clk_gmii(dwmac->clk_tx, speed);
> +	if (err == -ENOTSUPP)

> +		dev_err(dwmac->dev, "invalid speed %dMbps\n", speed);

%u?

> +	else if (err)
> +		dev_err(dwmac->dev,

> +			"failed to set tx rate for speed %dMbps: %pe\n",

ditto

-Serge(y)

> +			speed, ERR_PTR(err));
>  }
>  
>  static int starfive_dwmac_set_mode(struct plat_stmmacenet_data *plat_dat)
> -- 
> 2.30.2
> 
> 

Re: [PATCH net-next 6/6] net: stmmac: qos-eth: use dwmac_set_tx_clk_gmii()

[Serge Semin]

On Mon, Sep 11, 2023 at 04:29:36PM +0100, Russell King (Oracle) wrote:
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  .../stmicro/stmmac/dwmac-dwc-qos-eth.c        | 37 ++++++-------------
>  1 file changed, 11 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
> index 61ebf36da13d..a8fae37b9858 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
> @@ -22,6 +22,7 @@
>  #include <linux/stmmac.h>
>  
>  #include "stmmac_platform.h"
> +#include "stmmac_plat_lib.h"
>  #include "dwmac4.h"
>  
>  struct tegra_eqos {
> @@ -181,32 +182,10 @@ static void dwc_qos_remove(struct platform_device *pdev)
>  static void tegra_eqos_fix_speed(void *priv, unsigned int speed, unsigned int mode)
>  {
>  	struct tegra_eqos *eqos = priv;
> -	unsigned long rate = 125000000;
> -	bool needs_calibration = false;
>  	u32 value;
>  	int err;
>  
> -	switch (speed) {
> -	case SPEED_1000:
> -		needs_calibration = true;
> -		rate = 125000000;
> -		break;
> -
> -	case SPEED_100:
> -		needs_calibration = true;
> -		rate = 25000000;
> -		break;
> -
> -	case SPEED_10:
> -		rate = 2500000;
> -		break;
> -
> -	default:
> -		dev_err(eqos->dev, "invalid speed %u\n", speed);
> -		break;
> -	}
> -
> -	if (needs_calibration) {
> +	if (speed == SPEED_1000 || speed == SPEED_100) {
>  		/* calibrate */
>  		value = readl(eqos->regs + SDMEMCOMPPADCTRL);
>  		value |= SDMEMCOMPPADCTRL_PAD_E_INPUT_OR_E_PWRD;
> @@ -246,9 +225,15 @@ static void tegra_eqos_fix_speed(void *priv, unsigned int speed, unsigned int mo
>  		writel(value, eqos->regs + AUTO_CAL_CONFIG);
>  	}
>  
> -	err = clk_set_rate(eqos->clk_tx, rate);
> -	if (err < 0)
> -		dev_err(eqos->dev, "failed to set TX rate: %d\n", err);
> +	err = dwmac_set_tx_clk_gmii(eqos->clk_tx, speed);
> +	if (err == -ENOTSUPP) {

> +		dev_err(eqos->dev, "invalid speed %dMbps\n", speed);

%u?

> +		err = dwmac_set_tx_clk_gmii(eqos->clk_tx, SPEED_1000);
> +	} else if (err) {
> +		dev_err(eqos->dev,

> +			"failed to set tx rate for speed %dMbps: %pe\n",

ditto

-Serge(y)

> +			speed, ERR_PTR(err));
> +	}
>  }
>  
>  static int tegra_eqos_init(struct platform_device *pdev, void *priv)
> -- 
> 2.30.2
> 
> 

Re: [PATCH net-next 2/6] net: stmmac: imx: use dwmac_set_tx_clk_gmii()

[Serge Semin]

On Mon, Sep 11, 2023 at 04:29:16PM +0100, Russell King (Oracle) wrote:
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

BTW I don't know whether it's ok to have an empty description in the
patches for the networking subsystem, but the kernel maintainers
mainly request to add at least some text with the change justification
especially seeing the submitting-patches.rst doc says the description
is mandatory.

-Serge(y)

> ---
>  .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 27 ++++++-------------
>  1 file changed, 8 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> index df34e34cc14f..d2569faf7cc3 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> @@ -21,6 +21,7 @@
>  #include <linux/stmmac.h>
>  
>  #include "stmmac_platform.h"
> +#include "stmmac_plat_lib.h"
>  
>  #define GPR_ENET_QOS_INTF_MODE_MASK	GENMASK(21, 16)
>  #define GPR_ENET_QOS_INTF_SEL_MII	(0x0 << 16)
> @@ -186,7 +187,6 @@ static void imx_dwmac_fix_speed(void *priv, unsigned int speed, unsigned int mod
>  {
>  	struct plat_stmmacenet_data *plat_dat;
>  	struct imx_priv_data *dwmac = priv;
> -	unsigned long rate;
>  	int err;
>  
>  	plat_dat = dwmac->plat_dat;
> @@ -196,24 +196,13 @@ static void imx_dwmac_fix_speed(void *priv, unsigned int speed, unsigned int mod
>  	    (plat_dat->mac_interface == PHY_INTERFACE_MODE_MII))
>  		return;
>  
> -	switch (speed) {
> -	case SPEED_1000:
> -		rate = 125000000;
> -		break;
> -	case SPEED_100:
> -		rate = 25000000;
> -		break;
> -	case SPEED_10:
> -		rate = 2500000;
> -		break;
> -	default:
> -		dev_err(dwmac->dev, "invalid speed %u\n", speed);
> -		return;
> -	}
> -
> -	err = clk_set_rate(dwmac->clk_tx, rate);
> -	if (err < 0)
> -		dev_err(dwmac->dev, "failed to set tx rate %lu\n", rate);
> +	err = dwmac_set_tx_clk_gmii(dwmac->clk_tx, speed);
> +	if (err == -ENOTSUPP)
> +		dev_err(dwmac->dev, "invalid speed %dMbps\n", speed);
> +	else if (err)
> +		dev_err(dwmac->dev,
> +			"failed to set tx rate for speed %dMbps: %pe\n",
> +			speed, ERR_PTR(err));
>  }
>  
>  static void imx93_dwmac_fix_speed(void *priv, unsigned int speed, unsigned int mode)
> -- 
> 2.30.2
> 
>