[PATCH] net: stmmac: implement .set_intf_mode() callback for imx8dxl

[PATCH] net: stmmac: implement .set_intf_mode() callback for imx8dxl

[Joakim Zhang]

From: Fugang Duan <fugang.duan@nxp.com>

Implement .set_intf_mode() callback for imx8dxl.

Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 34 ++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
index 223f69da7e95..1d0a4d73add6 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
@@ -6,7 +6,9 @@
  *
  */
 
+#include <dt-bindings/firmware/imx/rsrc.h>
 #include <linux/clk.h>
+#include <linux/firmware/imx/sci.h>
 #include <linux/gpio/consumer.h>
 #include <linux/kernel.h>
 #include <linux/mfd/syscon.h>
@@ -86,7 +88,37 @@ imx8dxl_set_intf_mode(struct plat_stmmacenet_data *plat_dat)
 {
 	int ret = 0;
 
-	/* TBD: depends on imx8dxl scu interfaces to be upstreamed */
+	struct imx_sc_ipc *ipc_handle;
+	int val;
+
+	ret = imx_scu_get_handle(&ipc_handle);
+	if (ret)
+		return ret;
+
+	switch (plat_dat->interface) {
+	case PHY_INTERFACE_MODE_MII:
+		val = GPR_ENET_QOS_INTF_SEL_MII;
+		break;
+	case PHY_INTERFACE_MODE_RMII:
+		val = GPR_ENET_QOS_INTF_SEL_RMII;
+		break;
+	case PHY_INTERFACE_MODE_RGMII:
+	case PHY_INTERFACE_MODE_RGMII_ID:
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+		val = GPR_ENET_QOS_INTF_SEL_RGMII;
+		break;
+	default:
+		pr_debug("imx dwmac doesn't support %d interface\n",
+			 plat_dat->interface);
+		return -EINVAL;
+	}
+
+	ret = imx_sc_misc_set_control(ipc_handle, IMX_SC_R_ENET_1,
+				      IMX_SC_C_INTF_SEL, val >> 16);
+	ret |= imx_sc_misc_set_control(ipc_handle, IMX_SC_R_ENET_1,
+				       IMX_SC_C_CLK_GEN_EN, 0x1);
+
 	return ret;
 }
 
-- 
2.17.1

Re: [PATCH] net: stmmac: implement .set_intf_mode() callback for imx8dxl

[Jakub Kicinski]

On Thu,  3 Dec 2020 12:10:38 +0800 Joakim Zhang wrote:
> From: Fugang Duan <fugang.duan@nxp.com>
> 
> Implement .set_intf_mode() callback for imx8dxl.
> 
> Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>

Couple minor issues.

> @@ -86,7 +88,37 @@ imx8dxl_set_intf_mode(struct plat_stmmacenet_data *plat_dat)
>  {
>  	int ret = 0;
>  
> -	/* TBD: depends on imx8dxl scu interfaces to be upstreamed */
> +	struct imx_sc_ipc *ipc_handle;
> +	int val;

Looks like you're gonna have a empty line in the middle of variable
declarations?

Please remove it and order the variable lines longest to shortest.

> +
> +	ret = imx_scu_get_handle(&ipc_handle);
> +	if (ret)
> +		return ret;
> +
> +	switch (plat_dat->interface) {
> +	case PHY_INTERFACE_MODE_MII:
> +		val = GPR_ENET_QOS_INTF_SEL_MII;
> +		break;
> +	case PHY_INTERFACE_MODE_RMII:
> +		val = GPR_ENET_QOS_INTF_SEL_RMII;
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII:
> +	case PHY_INTERFACE_MODE_RGMII_ID:
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +	case PHY_INTERFACE_MODE_RGMII_TXID:
> +		val = GPR_ENET_QOS_INTF_SEL_RGMII;
> +		break;
> +	default:
> +		pr_debug("imx dwmac doesn't support %d interface\n",
> +			 plat_dat->interface);
> +		return -EINVAL;
> +	}
> +
> +	ret = imx_sc_misc_set_control(ipc_handle, IMX_SC_R_ENET_1,
> +				      IMX_SC_C_INTF_SEL, val >> 16);
> +	ret |= imx_sc_misc_set_control(ipc_handle, IMX_SC_R_ENET_1,
> +				       IMX_SC_C_CLK_GEN_EN, 0x1);
>  	return ret;

These calls may return different errors AFAICT.

You can't just errno values to gether the result will be meaningless.

please use the normal flow, and return the result of the second call
directly:

	ret = func1();
	if (ret)
		return ret;

	return func2();

Please also CC the maintainers of the Ethernet PHY subsystem on v2, 
to make sure there is nothing wrong with the patch from their PoV.

Thanks!

RE: [PATCH] net: stmmac: implement .set_intf_mode() callback for imx8dxl

[Joakim Zhang]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: 2020年12月6日 3:58
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: peppe.cavallaro@st.com; alexandre.torgue@st.com;
> joabreu@synopsys.com; davem@davemloft.net; dl-linux-imx
> <linux-imx@nxp.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH] net: stmmac: implement .set_intf_mode() callback for
> imx8dxl
> 
> On Thu,  3 Dec 2020 12:10:38 +0800 Joakim Zhang wrote:
> > From: Fugang Duan <fugang.duan@nxp.com>
> >
> > Implement .set_intf_mode() callback for imx8dxl.
> >
> > Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> 
> Couple minor issues.
> 
> > @@ -86,7 +88,37 @@ imx8dxl_set_intf_mode(struct
> plat_stmmacenet_data
> > *plat_dat)  {
> >  	int ret = 0;
> >
> > -	/* TBD: depends on imx8dxl scu interfaces to be upstreamed */
> > +	struct imx_sc_ipc *ipc_handle;
> > +	int val;
> 
> Looks like you're gonna have a empty line in the middle of variable
> declarations?
> 
> Please remove it and order the variable lines longest to shortest.
> 
> > +
> > +	ret = imx_scu_get_handle(&ipc_handle);
> > +	if (ret)
> > +		return ret;
> > +
> > +	switch (plat_dat->interface) {
> > +	case PHY_INTERFACE_MODE_MII:
> > +		val = GPR_ENET_QOS_INTF_SEL_MII;
> > +		break;
> > +	case PHY_INTERFACE_MODE_RMII:
> > +		val = GPR_ENET_QOS_INTF_SEL_RMII;
> > +		break;
> > +	case PHY_INTERFACE_MODE_RGMII:
> > +	case PHY_INTERFACE_MODE_RGMII_ID:
> > +	case PHY_INTERFACE_MODE_RGMII_RXID:
> > +	case PHY_INTERFACE_MODE_RGMII_TXID:
> > +		val = GPR_ENET_QOS_INTF_SEL_RGMII;
> > +		break;
> > +	default:
> > +		pr_debug("imx dwmac doesn't support %d interface\n",
> > +			 plat_dat->interface);
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = imx_sc_misc_set_control(ipc_handle, IMX_SC_R_ENET_1,
> > +				      IMX_SC_C_INTF_SEL, val >> 16);
> > +	ret |= imx_sc_misc_set_control(ipc_handle, IMX_SC_R_ENET_1,
> > +				       IMX_SC_C_CLK_GEN_EN, 0x1);
> >  	return ret;
> 
> These calls may return different errors AFAICT.
> 
> You can't just errno values to gether the result will be meaningless.
> 
> please use the normal flow, and return the result of the second call
> directly:
> 
> 	ret = func1();
> 	if (ret)
> 		return ret;
> 
> 	return func2();
> 
> Please also CC the maintainers of the Ethernet PHY subsystem on v2, to make
> sure there is nothing wrong with the patch from their PoV.


Thanks Jakub for your kindly review, I will improve patch following your comments.

Best Regards,
Joakim Zhang
> Thanks!