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

[PATCH net-next v2 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.

v2:
- move dwmac_set_tx_clk_gmii() to stmmac_platform, and rename to have
   stmmac_ prefix.
- add comment body to conversion patches
- use %u for printing speed

 .../ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c    | 36 ++++---------
 drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c    | 26 +++-------
 .../net/ethernet/stmicro/stmmac/dwmac-intel-plat.c | 34 +++---------
 drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c     | 60 ++++++----------------
 .../net/ethernet/stmicro/stmmac/dwmac-starfive.c   | 28 +++-------
 .../net/ethernet/stmicro/stmmac/stmmac_platform.c  | 25 +++++++++
 .../net/ethernet/stmicro/stmmac/stmmac_platform.h  |  1 +
 7 files changed, 74 insertions(+), 136 deletions(-)

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH net-next 1/6] net: stmmac: add stmmac_set_tx_clk_gmii()

[Russell King (Oracle)]

Add a helper function for setting the transmit clock for GMII
interfaces. This handles 1G, 100M and 10M using the standard clock
rates of 125MHz, 25MHz and 2.5MHz.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 .../ethernet/stmicro/stmmac/stmmac_platform.c | 25 +++++++++++++++++++
 .../ethernet/stmicro/stmmac/stmmac_platform.h |  1 +
 2 files changed, 26 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index 0f28795e581c..f7635ed2b255 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -700,6 +700,31 @@ EXPORT_SYMBOL_GPL(stmmac_probe_config_dt);
 EXPORT_SYMBOL_GPL(devm_stmmac_probe_config_dt);
 EXPORT_SYMBOL_GPL(stmmac_remove_config_dt);
 
+int stmmac_set_tx_clk_gmii(struct clk *tx_clk, unsigned 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(stmmac_set_tx_clk_gmii);
+
 int stmmac_get_platform_resources(struct platform_device *pdev,
 				  struct stmmac_resources *stmmac_res)
 {
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
index c5565b2a70ac..8dc2287c6724 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
@@ -11,6 +11,7 @@
 
 #include "stmmac.h"
 
+int stmmac_set_tx_clk_gmii(struct clk *tx_clk, unsigned int speed);
 struct plat_stmmacenet_data *
 stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac);
 struct plat_stmmacenet_data *
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Russell King (Oracle)]

Use stmmac_set_tx_clk_gmii().

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 26 +++++--------------
 1 file changed, 7 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..cb56f9523acc 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
@@ -186,7 +186,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 +195,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 = stmmac_set_tx_clk_gmii(dwmac->clk_tx, speed);
+	if (err == -ENOTSUPP)
+		dev_err(dwmac->dev, "invalid speed %uMbps\n", speed);
+	else if (err)
+		dev_err(dwmac->dev,
+			"failed to set tx rate for speed %uMbps: %pe\n",
+			speed, ERR_PTR(err));
 }
 
 static void imx93_dwmac_fix_speed(void *priv, unsigned int speed, unsigned int mode)
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Russell King (Oracle)]

Use stmmac_set_tx_clk_gmii().

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 .../stmicro/stmmac/dwmac-intel-plat.c         | 34 +++++--------------
 1 file changed, 8 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..c6ebd1d91f38 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
@@ -31,32 +31,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 = stmmac_set_tx_clk_gmii(dwmac->tx_clk, speed);
+	if (err == -ENOTSUPP)
+		dev_err(dwmac->dev, "invalid speed %uMbps\n", speed);
+	else if (err)
+		dev_err(dwmac->dev, "failed to set tx rate for speed %uMbps: %pe\n",
+			speed, ERR_PTR(err));
 }
 
 static const struct intel_dwmac_data kmb_data = {
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Russell King (Oracle)]

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",
+			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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Russell King (Oracle)]

Use stmmac_set_tx_clk_gmii().

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 .../ethernet/stmicro/stmmac/dwmac-starfive.c  | 28 +++++--------------
 1 file changed, 7 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..c2931464e977 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
@@ -27,29 +27,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 = stmmac_set_tx_clk_gmii(dwmac->clk_tx, speed);
+	if (err == -ENOTSUPP)
+		dev_err(dwmac->dev, "invalid speed %uMbps\n", speed);
+	else if (err)
+		dev_err(dwmac->dev,
+			"failed to set tx rate for speed %uMbps: %pe\n",
+			speed, ERR_PTR(err));
 }
 
 static int starfive_dwmac_set_mode(struct plat_stmmacenet_data *plat_dat)
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Russell King (Oracle)]

Use stmmac_set_tx_clk_gmii().

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 .../stmicro/stmmac/dwmac-dwc-qos-eth.c        | 36 ++++++-------------
 1 file changed, 10 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..bc9b054e10af 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
@@ -181,32 +181,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 +224,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 = stmmac_set_tx_clk_gmii(eqos->clk_tx, speed);
+	if (err == -ENOTSUPP) {
+		dev_err(eqos->dev, "invalid speed %uMbps\n", speed);
+		err = stmmac_set_tx_clk_gmii(eqos->clk_tx, SPEED_1000);
+	} else if (err) {
+		dev_err(eqos->dev,
+			"failed to set tx rate for speed %uMbps: %pe\n",
+			speed, ERR_PTR(err));
+	}
 }
 
 static int tegra_eqos_init(struct platform_device *pdev, void *priv)
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Serge Semin]

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
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next 1/6] net: stmmac: add stmmac_set_tx_clk_gmii()

[Serge Semin]

On Thu, Sep 14, 2023 at 02:51:20PM +0100, Russell King (Oracle) wrote:
> Add a helper function for setting the transmit clock for GMII
> interfaces. This handles 1G, 100M and 10M using the standard clock
> rates of 125MHz, 25MHz and 2.5MHz.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  .../ethernet/stmicro/stmmac/stmmac_platform.c | 25 +++++++++++++++++++
>  .../ethernet/stmicro/stmmac/stmmac_platform.h |  1 +
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> index 0f28795e581c..f7635ed2b255 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> @@ -700,6 +700,31 @@ EXPORT_SYMBOL_GPL(stmmac_probe_config_dt);
>  EXPORT_SYMBOL_GPL(devm_stmmac_probe_config_dt);
>  EXPORT_SYMBOL_GPL(stmmac_remove_config_dt);
>  

> +int stmmac_set_tx_clk_gmii(struct clk *tx_clk, unsigned 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(stmmac_set_tx_clk_gmii);

As I already noted in v1 normally the switch-case operations are
defined with no additional line separating the cases. I would have
dropped them here too especially seeing the stmmac core driver mainly
follow that implicit convention.

Additionally I suggest to move the method to being defined at the head
of the file. Thus a more natural order normally utilized in the kernel
drivers would be preserved: all functional implementations go first,
the platform-specific things are placed below like probe()/remove()
and their sub-functions, suspend()/resume() and PM descriptors,
(device IDs table, driver descriptor, etc). stmmac_set_tx_clk_gmii()
looks as a functional helper which is normally utilized on the network
device open() stage in the framework of the fix_mac_speed() callback.
Moreover my suggestion gets to be even more justified seeing you
placed the method prototype at the head of the prototypes list in the
stmmac_platform.h file.

Irrespective to the nitpicks above the change looks good:
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>

-Serge(y)

> +
>  int stmmac_get_platform_resources(struct platform_device *pdev,
>  				  struct stmmac_resources *stmmac_res)
>  {
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
> index c5565b2a70ac..8dc2287c6724 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
> @@ -11,6 +11,7 @@
>  
>  #include "stmmac.h"
>  
> +int stmmac_set_tx_clk_gmii(struct clk *tx_clk, unsigned int speed);
>  struct plat_stmmacenet_data *
>  stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac);
>  struct plat_stmmacenet_data *
> -- 
> 2.30.2
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Serge Semin]

On Thu, Sep 14, 2023 at 02:51:25PM +0100, Russell King (Oracle) wrote:
> Use stmmac_set_tx_clk_gmii().
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Serge Semin <fancer.lancer@gmail.com>

-Serge(y)

> ---
>  .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 26 +++++--------------
>  1 file changed, 7 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..cb56f9523acc 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> @@ -186,7 +186,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 +195,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 = stmmac_set_tx_clk_gmii(dwmac->clk_tx, speed);
> +	if (err == -ENOTSUPP)
> +		dev_err(dwmac->dev, "invalid speed %uMbps\n", speed);
> +	else if (err)
> +		dev_err(dwmac->dev,
> +			"failed to set tx rate for speed %uMbps: %pe\n",
> +			speed, ERR_PTR(err));
>  }
>  
>  static void imx93_dwmac_fix_speed(void *priv, unsigned int speed, unsigned int mode)
> -- 
> 2.30.2
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Serge Semin]

On Thu, Sep 14, 2023 at 02:51:30PM +0100, Russell King (Oracle) wrote:
> Use stmmac_set_tx_clk_gmii().
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Serge Semin <fancer.lancer@gmail.com>

-Serge(y)

> ---
>  .../stmicro/stmmac/dwmac-intel-plat.c         | 34 +++++--------------
>  1 file changed, 8 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..c6ebd1d91f38 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
> @@ -31,32 +31,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 = stmmac_set_tx_clk_gmii(dwmac->tx_clk, speed);
> +	if (err == -ENOTSUPP)
> +		dev_err(dwmac->dev, "invalid speed %uMbps\n", speed);
> +	else if (err)
> +		dev_err(dwmac->dev, "failed to set tx rate for speed %uMbps: %pe\n",
> +			speed, ERR_PTR(err));
>  }
>  
>  static const struct intel_dwmac_data kmb_data = {
> -- 
> 2.30.2
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Russell King (Oracle)]

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.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Serge Semin]

On Thu, Sep 14, 2023 at 02:51:40PM +0100, Russell King (Oracle) wrote:
> Use stmmac_set_tx_clk_gmii().
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Serge Semin <fancer.lancer@gmail.com>

-Serge(y)

> ---
>  .../ethernet/stmicro/stmmac/dwmac-starfive.c  | 28 +++++--------------
>  1 file changed, 7 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..c2931464e977 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
> @@ -27,29 +27,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 = stmmac_set_tx_clk_gmii(dwmac->clk_tx, speed);
> +	if (err == -ENOTSUPP)
> +		dev_err(dwmac->dev, "invalid speed %uMbps\n", speed);
> +	else if (err)
> +		dev_err(dwmac->dev,
> +			"failed to set tx rate for speed %uMbps: %pe\n",
> +			speed, ERR_PTR(err));
>  }
>  
>  static int starfive_dwmac_set_mode(struct plat_stmmacenet_data *plat_dat)
> -- 
> 2.30.2
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next 1/6] net: stmmac: add stmmac_set_tx_clk_gmii()

[Russell King (Oracle)]

On Thu, Sep 14, 2023 at 05:54:09PM +0300, Serge Semin wrote:
> On Thu, Sep 14, 2023 at 02:51:20PM +0100, Russell King (Oracle) wrote:
> > Add a helper function for setting the transmit clock for GMII
> > interfaces. This handles 1G, 100M and 10M using the standard clock
> > rates of 125MHz, 25MHz and 2.5MHz.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> >  .../ethernet/stmicro/stmmac/stmmac_platform.c | 25 +++++++++++++++++++
> >  .../ethernet/stmicro/stmmac/stmmac_platform.h |  1 +
> >  2 files changed, 26 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > index 0f28795e581c..f7635ed2b255 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > @@ -700,6 +700,31 @@ EXPORT_SYMBOL_GPL(stmmac_probe_config_dt);
> >  EXPORT_SYMBOL_GPL(devm_stmmac_probe_config_dt);
> >  EXPORT_SYMBOL_GPL(stmmac_remove_config_dt);
> >  
> 
> > +int stmmac_set_tx_clk_gmii(struct clk *tx_clk, unsigned 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(stmmac_set_tx_clk_gmii);
> 
> As I already noted in v1 normally the switch-case operations are
> defined with no additional line separating the cases. I would have
> dropped them here too especially seeing the stmmac core driver mainly
> follow that implicit convention.

It's rather haphazard whether there are blank lines or not between
case statements.

> Additionally I suggest to move the method to being defined at the head
> of the file. Thus a more natural order normally utilized in the kernel
> drivers would be preserved: all functional implementations go first,
> the platform-specific things are placed below like probe()/remove()
> and their sub-functions, suspend()/resume() and PM descriptors,
> (device IDs table, driver descriptor, etc). stmmac_set_tx_clk_gmii()
> looks as a functional helper which is normally utilized on the network
> device open() stage in the framework of the fix_mac_speed() callback.
> Moreover my suggestion gets to be even more justified seeing you
> placed the method prototype at the head of the prototypes list in the
> stmmac_platform.h file.

How is one supposed to know about this? I did my best trying to work
out where they should've gone...

If it's that important, maybe add some /* Comments */ to state that
there are separate sections to the file?

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Serge Semin]

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!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Russell King (Oracle)]

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.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Russell King (Oracle)]

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.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jose Abreu]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next 1/6] net: stmmac: add stmmac_set_tx_clk_gmii()

[Serge Semin]

On Thu, Sep 14, 2023 at 04:12:13PM +0100, Russell King (Oracle) wrote:
> On Thu, Sep 14, 2023 at 05:54:09PM +0300, Serge Semin wrote:
> > On Thu, Sep 14, 2023 at 02:51:20PM +0100, Russell King (Oracle) wrote:
> > > Add a helper function for setting the transmit clock for GMII
> > > interfaces. This handles 1G, 100M and 10M using the standard clock
> > > rates of 125MHz, 25MHz and 2.5MHz.
> > > 
> > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > ---
> > >  .../ethernet/stmicro/stmmac/stmmac_platform.c | 25 +++++++++++++++++++
> > >  .../ethernet/stmicro/stmmac/stmmac_platform.h |  1 +
> > >  2 files changed, 26 insertions(+)
> > > 
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > > index 0f28795e581c..f7635ed2b255 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > > @@ -700,6 +700,31 @@ EXPORT_SYMBOL_GPL(stmmac_probe_config_dt);
> > >  EXPORT_SYMBOL_GPL(devm_stmmac_probe_config_dt);
> > >  EXPORT_SYMBOL_GPL(stmmac_remove_config_dt);
> > >  
> > 
> > > +int stmmac_set_tx_clk_gmii(struct clk *tx_clk, unsigned 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(stmmac_set_tx_clk_gmii);
> > 
> > As I already noted in v1 normally the switch-case operations are
> > defined with no additional line separating the cases. I would have
> > dropped them here too especially seeing the stmmac core driver mainly
> > follow that implicit convention.
> 
> It's rather haphazard whether there are blank lines or not between
> case statements.

Is it haphazard in the STMMAC core driver too? The only exception is
the HWtstamp switch-case statements which just a bit bulky. So having
additional empty lines there rather weakly but is still justified by
that.

In anyway my comment is just a nitpick inferred from the implicit
local convention. It's totally IMO and isn't implied to be considered
as a strong request to be implemented. I repeated my comment just
because you didn't respond to it in v1. It looked as if you just
missed it.

> 
> > Additionally I suggest to move the method to being defined at the head
> > of the file. Thus a more natural order normally utilized in the kernel
> > drivers would be preserved: all functional implementations go first,
> > the platform-specific things are placed below like probe()/remove()
> > and their sub-functions, suspend()/resume() and PM descriptors,
> > (device IDs table, driver descriptor, etc). stmmac_set_tx_clk_gmii()
> > looks as a functional helper which is normally utilized on the network
> > device open() stage in the framework of the fix_mac_speed() callback.
> > Moreover my suggestion gets to be even more justified seeing you
> > placed the method prototype at the head of the prototypes list in the
> > stmmac_platform.h file.
> 

> How is one supposed to know about this? I did my best trying to work
> out where they should've gone...

Well, from my experience submitting the patches to various kernel
subsystems and drivers there are many implicit conventions which
aren't described anywhere, but could be inferred from the code itself.
This one is one of such implicit conventions which isn't mandatory but
a nice-to-have feature for better readability and maintainability (for
instance in order to determine where to put new methods and features
to the already available drivers). In anyway this comment is also a
nitpick, which from my point of view would improve the code
readability. It's normally up to the driver/subsystem maintainers to
define such conventions required.

Regarding the implicit conventions. Some of the subsystem and driver
maintainers imply that such conventions would be preserved (just
recently met that in the PCIe subsystem). When it happens it's so
irritating especially if it concerns a big series.

> 
> If it's that important, maybe add some /* Comments */ to state that
> there are separate sections to the file?

Would be nice to have them indeed. Though I normally just stick to
that convention by default if there is no another one could be
inferred from the code itself.

-Serge(y)

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Serge Semin]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jose Abreu]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Serge Semin]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel