[PATCH v3 net 0/2] update stmmac fix_mac_speed

[PATCH v3 net 0/2] update stmmac fix_mac_speed

[Shenwei Wang]

Changes in V3:
  - fixed the build errors reported by 'kernel test robot'.
  - Only perform clock pause in RGMII fixed-link usecase.

Changes in V2:
  - Call fix_mac_speed() with new mode parameter added.
  - reorg the function of imx_dwmac_fix_speed_mx93 by using the
    mode parameter.

Shenwei Wang (2):
  net: stmmac: add new mode parameter for fix_mac_speed
  net: stmmac: dwmac-imx: pause the TXC clock in fixed-link

 .../stmicro/stmmac/dwmac-dwc-qos-eth.c        |  2 +-
 .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 44 ++++++++++++++++++-
 .../stmicro/stmmac/dwmac-intel-plat.c         |  4 +-
 .../ethernet/stmicro/stmmac/dwmac-ipq806x.c   |  2 +-
 .../net/ethernet/stmicro/stmmac/dwmac-meson.c |  2 +-
 .../stmicro/stmmac/dwmac-qcom-ethqos.c        |  2 +-
 .../net/ethernet/stmicro/stmmac/dwmac-rk.c    |  2 +-
 .../ethernet/stmicro/stmmac/dwmac-socfpga.c   |  2 +-
 .../ethernet/stmicro/stmmac/dwmac-starfive.c  |  2 +-
 .../net/ethernet/stmicro/stmmac/dwmac-sunxi.c |  2 +-
 .../ethernet/stmicro/stmmac/dwmac-visconti.c  |  2 +-
 .../net/ethernet/stmicro/stmmac/stmmac_main.c |  2 +-
 include/linux/stmmac.h                        |  2 +-
 13 files changed, 56 insertions(+), 14 deletions(-)

--
2.34.1


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

[PATCH v3 net 1/2] net: stmmac: add new mode parameter for fix_mac_speed

[Shenwei Wang]

A mode parameter has been added to the callback function of fix_mac_speed
to indicate the physical layer type.

The mode can be one the following:
	MLO_AN_PHY	- Conventional PHY
	MLO_AN_FIXED	- Fixed-link mode
	MLO_AN_INBAND	- In-band protocol

Also use short version of 'uint' to replace the 'unsigned int' in the
function definitions.

Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c | 2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c         | 2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c  | 4 ++--
 drivers/net/ethernet/stmicro/stmmac/dwmac-ipq806x.c     | 2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-meson.c       | 2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c          | 2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c     | 2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c    | 2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c       | 2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-visconti.c    | 2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c       | 2 +-
 include/linux/stmmac.h                                  | 2 +-
 13 files changed, 14 insertions(+), 14 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 b5efd9c2eac7..7e45cfa9fc2c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
@@ -178,7 +178,7 @@ static void dwc_qos_remove(struct platform_device *pdev)
 #define AUTO_CAL_STATUS 0x880c
 #define  AUTO_CAL_STATUS_ACTIVE BIT(31)
 
-static void tegra_eqos_fix_speed(void *priv, unsigned int speed)
+static void tegra_eqos_fix_speed(void *priv, uint speed, uint mode)
 {
 	struct tegra_eqos *eqos = priv;
 	unsigned long rate = 125000000;
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
index b9378a63f0e8..53ee5a42c071 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
@@ -178,7 +178,7 @@ static void imx_dwmac_exit(struct platform_device *pdev, void *priv)
 	/* nothing to do now */
 }
 
-static void imx_dwmac_fix_speed(void *priv, unsigned int speed)
+static void imx_dwmac_fix_speed(void *priv, uint speed, uint mode)
 {
 	struct plat_stmmacenet_data *plat_dat;
 	struct imx_priv_data *dwmac = priv;
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
index a5e639ab0b9e..1f2eabfe79ca 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
@@ -22,13 +22,13 @@ struct intel_dwmac {
 };
 
 struct intel_dwmac_data {
-	void (*fix_mac_speed)(void *priv, unsigned int speed);
+	void (*fix_mac_speed)(void *priv, uint speed, uint mode);
 	unsigned long ptp_ref_clk_rate;
 	unsigned long tx_clk_rate;
 	bool tx_clk_en;
 };
 
-static void kmb_eth_fix_mac_speed(void *priv, unsigned int speed)
+static void kmb_eth_fix_mac_speed(void *priv, uint speed, uint mode)
 {
 	struct intel_dwmac *dwmac = priv;
 	unsigned long rate;
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-ipq806x.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-ipq806x.c
index e39406df8516..8070352844e3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-ipq806x.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-ipq806x.c
@@ -257,7 +257,7 @@ static int ipq806x_gmac_of_parse(struct ipq806x_gmac *gmac)
 	return PTR_ERR_OR_ZERO(gmac->qsgmii_csr);
 }
 
-static void ipq806x_gmac_fix_mac_speed(void *priv, unsigned int speed)
+static void ipq806x_gmac_fix_mac_speed(void *priv, uint speed, uint mode)
 {
 	struct ipq806x_gmac *gmac = priv;
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson.c
index 7aa5e6bc04eb..612551c09ad9 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson.c
@@ -22,7 +22,7 @@ struct meson_dwmac {
 	void __iomem	*reg;
 };
 
-static void meson6_dwmac_fix_mac_speed(void *priv, unsigned int speed)
+static void meson6_dwmac_fix_mac_speed(void *priv, uint speed, uint mode)
 {
 	struct meson_dwmac *dwmac = priv;
 	unsigned int val;
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
index 735525ba8b93..c32549d2fc5a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
@@ -631,7 +631,7 @@ static int ethqos_configure(struct qcom_ethqos *ethqos)
 	return ethqos->configure_func(ethqos);
 }
 
-static void ethqos_fix_mac_speed(void *priv, unsigned int speed)
+static void ethqos_fix_mac_speed(void *priv, uint speed, uint mode)
 {
 	struct qcom_ethqos *ethqos = priv;
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
index d81591b470a2..2fb24c7e1b44 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
@@ -1785,7 +1785,7 @@ static void rk_gmac_powerdown(struct rk_priv_data *gmac)
 	gmac_clk_enable(gmac, false);
 }
 
-static void rk_fix_speed(void *priv, unsigned int speed)
+static void rk_fix_speed(void *priv, uint speed, uint mode)
 {
 	struct rk_priv_data *bsp_priv = priv;
 	struct device *dev = &bsp_priv->pdev->dev;
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
index 6267bcb60206..ef3be5a3e7b5 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
@@ -61,7 +61,7 @@ struct socfpga_dwmac {
 	struct mdio_device *pcs_mdiodev;
 };
 
-static void socfpga_dwmac_fix_mac_speed(void *priv, unsigned int speed)
+static void socfpga_dwmac_fix_mac_speed(void *priv, uint speed, uint mode)
 {
 	struct socfpga_dwmac *dwmac = (struct socfpga_dwmac *)priv;
 	void __iomem *splitter_base = dwmac->splitter_base;
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
index d3a39d2fb3a9..66e434cd7124 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
@@ -22,7 +22,7 @@ struct starfive_dwmac {
 	struct clk *clk_tx;
 };
 
-static void starfive_dwmac_fix_mac_speed(void *priv, unsigned int speed)
+static void starfive_dwmac_fix_mac_speed(void *priv, uint speed, uint mode)
 {
 	struct starfive_dwmac *dwmac = priv;
 	unsigned long rate;
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c
index 50963e91c347..4bbc9d6888f1 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c
@@ -72,7 +72,7 @@ static void sun7i_gmac_exit(struct platform_device *pdev, void *priv)
 		regulator_disable(gmac->regulator);
 }
 
-static void sun7i_fix_speed(void *priv, unsigned int speed)
+static void sun7i_fix_speed(void *priv, uint speed, uint mode)
 {
 	struct sunxi_priv_data *gmac = priv;
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-visconti.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-visconti.c
index acbb284be174..5c50cebe9a17 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-visconti.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-visconti.c
@@ -53,7 +53,7 @@ struct visconti_eth {
 	spinlock_t lock; /* lock to protect register update */
 };
 
-static void visconti_eth_fix_mac_speed(void *priv, unsigned int speed)
+static void visconti_eth_fix_mac_speed(void *priv, uint speed, uint mode)
 {
 	struct visconti_eth *dwmac = priv;
 	struct net_device *netdev = dev_get_drvdata(dwmac->dev);
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index e1f1c034d325..1c26d60886be 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1060,7 +1060,7 @@ static void stmmac_mac_link_up(struct phylink_config *config,
 	priv->speed = speed;
 
 	if (priv->plat->fix_mac_speed)
-		priv->plat->fix_mac_speed(priv->plat->bsp_priv, speed);
+		priv->plat->fix_mac_speed(priv->plat->bsp_priv, speed, mode);
 
 	if (!duplex)
 		ctrl &= ~priv->hw->link.duplex;
diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index ef67dba775d0..7d5e178574be 100644
--- a/include/linux/stmmac.h
+++ b/include/linux/stmmac.h
@@ -253,7 +253,7 @@ struct plat_stmmacenet_data {
 	u8 tx_sched_algorithm;
 	struct stmmac_rxq_cfg rx_queues_cfg[MTL_MAX_RX_QUEUES];
 	struct stmmac_txq_cfg tx_queues_cfg[MTL_MAX_TX_QUEUES];
-	void (*fix_mac_speed)(void *priv, unsigned int speed);
+	void (*fix_mac_speed)(void *priv, uint speed, uint mode);
 	int (*fix_soc_reset)(void *priv, void __iomem *ioaddr);
 	int (*serdes_powerup)(struct net_device *ndev, void *priv);
 	void (*serdes_powerdown)(struct net_device *ndev, void *priv);
-- 
2.34.1


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

Re: [PATCH v3 net 1/2] net: stmmac: add new mode parameter for fix_mac_speed

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 895 bytes --]

On 31.07.2023 11:19:28, Shenwei Wang wrote:
> A mode parameter has been added to the callback function of fix_mac_speed
> to indicate the physical layer type.
> 
> The mode can be one the following:
> 	MLO_AN_PHY	- Conventional PHY
> 	MLO_AN_FIXED	- Fixed-link mode
> 	MLO_AN_INBAND	- In-band protocol
> 
> Also use short version of 'uint' to replace the 'unsigned int' in the
> function definitions.

There are not many users of 'uint' in the kernel and it's not used in
the stmmac driver so far. From my point of view I would not introduce
it and stick to the standard 'unsigned int'.

Just my 2 cent,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

RE: [EXT] Re: [PATCH v3 net 1/2] net: stmmac: add new mode parameter for fix_mac_speed

[Shenwei Wang]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: Tuesday, August 1, 2023 1:37 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: Russell King <linux@armlinux.org.uk>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Maxime
> Coquelin <mcoquelin.stm32@gmail.com>; Shawn Guo <shawnguo@kernel.org>;
> Sascha Hauer <s.hauer@pengutronix.de>; Neil Armstrong
> <neil.armstrong@linaro.org>; Kevin Hilman <khilman@baylibre.com>; Vinod
> Koul <vkoul@kernel.org>; Chen-Yu Tsai <wens@csie.org>; Jernej Skrabec
> <jernej.skrabec@gmail.com>; Samuel Holland <samuel@sholland.org>; Jose
> Abreu <joabreu@synopsys.com>; imx@lists.linux.dev; Simon Horman
> <simon.horman@corigine.com>; Alexandre Torgue
> <alexandre.torgue@foss.st.com>; Giuseppe Cavallaro
> <peppe.cavallaro@st.com>; Nobuhiro Iwamatsu
> <nobuhiro1.iwamatsu@toshiba.co.jp>; Fabio Estevam <festevam@gmail.com>;
> linux-stm32@st-md-mailman.stormreply.com; Jerome Brunet
> <jbrunet@baylibre.com>; Bartosz Golaszewski
> <bartosz.golaszewski@linaro.org>; Wong Vee Khee <veekhee@apple.com>; dl-
> linux-imx <linux-imx@nxp.com>; Andrew Halaney <ahalaney@redhat.com>;
> Bhupesh Sharma <bhupesh.sharma@linaro.org>; Martin Blumenstingl
> <martin.blumenstingl@googlemail.com>; Revanth Kumar Uppala
> <ruppala@nvidia.com>; Jochen Henneberg <jh@henneberg-systemdesign.com>;
> linux-amlogic@lists.infradead.org; linux-arm-kernel@lists.infradead.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Pengutronix Kernel
> Team <kernel@pengutronix.de>
> Subject: [EXT] Re: [PATCH v3 net 1/2] net: stmmac: add new mode parameter
> for fix_mac_speed
> 
> On 31.07.2023 11:19:28, Shenwei Wang wrote:
> > A mode parameter has been added to the callback function of
> > fix_mac_speed to indicate the physical layer type.
> >
> > The mode can be one the following:
> > 	MLO_AN_PHY	- Conventional PHY
> > 	MLO_AN_FIXED	- Fixed-link mode
> > 	MLO_AN_INBAND	- In-band protocol
> >
> > Also use short version of 'uint' to replace the 'unsigned int' in the
> > function definitions.
> 
> There are not many users of 'uint' in the kernel and it's not used in the stmmac
> driver so far. From my point of view I would not introduce it and stick to the
> standard 'unsigned int'.
> 

Using 'uint' makes the code look cleaner because adding one extra parameter may cause some function 
declarations to span multiple lines.  This change keeps function declarations compact on a single line.

Thanks,
Shenwei

> Just my 2 cent,
> Marc
> 
> --
> Pengutronix e.K.                 | Marc Kleine-Budde          |
> Embedded Linux                   | https://www.pengutronix.de |
> Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [EXT] Re: [PATCH v3 net 1/2] net: stmmac: add new mode parameter for fix_mac_speed

[Jakub Kicinski]

On Tue, 1 Aug 2023 18:43:52 +0000 Shenwei Wang wrote:
> Subject: RE: [EXT] Re: [PATCH v3 net 1/2] net: stmmac: add new mode parameter  for fix_mac_speed

Looks like new platform enablement, the correct tree to target this 
at is net-next (i.e. [PATCH net-next]).

> > -----Original Message-----
> > From: Marc Kleine-Budde <mkl@pengutronix.de>
> > Sent: Tuesday, August 1, 2023 1:37 AM
> > To: Shenwei Wang <shenwei.wang@nxp.com>
> > Cc: Russell King <linux@armlinux.org.uk>; David S. Miller
> > <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> > Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Maxime
> > Coquelin <mcoquelin.stm32@gmail.com>; Shawn Guo <shawnguo@kernel.org>;
> > Sascha Hauer <s.hauer@pengutronix.de>; Neil Armstrong
> > <neil.armstrong@linaro.org>; Kevin Hilman <khilman@baylibre.com>; Vinod
> > Koul <vkoul@kernel.org>; Chen-Yu Tsai <wens@csie.org>; Jernej Skrabec
> > <jernej.skrabec@gmail.com>; Samuel Holland <samuel@sholland.org>; Jose
> > Abreu <joabreu@synopsys.com>; imx@lists.linux.dev; Simon Horman
> > <simon.horman@corigine.com>; Alexandre Torgue
> > <alexandre.torgue@foss.st.com>; Giuseppe Cavallaro
> > <peppe.cavallaro@st.com>; Nobuhiro Iwamatsu
> > <nobuhiro1.iwamatsu@toshiba.co.jp>; Fabio Estevam <festevam@gmail.com>;
> > linux-stm32@st-md-mailman.stormreply.com; Jerome Brunet
> > <jbrunet@baylibre.com>; Bartosz Golaszewski
> > <bartosz.golaszewski@linaro.org>; Wong Vee Khee <veekhee@apple.com>; dl-
> > linux-imx <linux-imx@nxp.com>; Andrew Halaney <ahalaney@redhat.com>;
> > Bhupesh Sharma <bhupesh.sharma@linaro.org>; Martin Blumenstingl
> > <martin.blumenstingl@googlemail.com>; Revanth Kumar Uppala
> > <ruppala@nvidia.com>; Jochen Henneberg <jh@henneberg-systemdesign.com>;
> > linux-amlogic@lists.infradead.org; linux-arm-kernel@lists.infradead.org;
> > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Pengutronix Kernel
> > Team <kernel@pengutronix.de>
> > Subject: [EXT] Re: [PATCH v3 net 1/2] net: stmmac: add new mode parameter
> > for fix_mac_speed

Why is this quote included? Please get a sane email client.

> > On 31.07.2023 11:19:28, Shenwei Wang wrote:  
> > > A mode parameter has been added to the callback function of
> > > fix_mac_speed to indicate the physical layer type.
> > >
> > > The mode can be one the following:
> > > 	MLO_AN_PHY	- Conventional PHY
> > > 	MLO_AN_FIXED	- Fixed-link mode
> > > 	MLO_AN_INBAND	- In-band protocol
> > >
> > > Also use short version of 'uint' to replace the 'unsigned int' in the
> > > function definitions.  
> > 
> > There are not many users of 'uint' in the kernel and it's not used in the stmmac
> > driver so far. From my point of view I would not introduce it and stick to the
> > standard 'unsigned int'.
> 
> Using 'uint' makes the code look cleaner because adding one extra
> parameter may cause some function declarations to span multiple
> lines.  This change keeps function declarations compact on a single
> line.

Marc is right. Just do it.

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

RE: [EXT] Re: [PATCH v3 net 1/2] net: stmmac: add new mode parameter for fix_mac_speed

[Shenwei Wang]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Tuesday, August 1, 2023 2:58 PM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> > > Subject: [EXT] Re: [PATCH v3 net 1/2] net: stmmac: add new mode
> > > parameter for fix_mac_speed
> 
> Why is this quote included? Please get a sane email client.
> 

I have no idea. We are using Office Outlook.

Regards,
Shenwei

> > > On 31.07.2023 11:19:28, Shenwei Wang wrote:
> > > > A mode parameter has been added to the callback function of
> > > > fix_mac_speed to indicate the physical layer type.
> > > >
> > > > The mode can be one the following:
> > > >   MLO_AN_PHY      - Conventional PHY
> > > >   MLO_AN_FIXED    - Fixed-link mode
> > > >   MLO_AN_INBAND   - In-band protocol
> > > >
> > > > Also use short version of 'uint' to replace the 'unsigned int' in
> > > > the function definitions.
> > >
> > > There are not many users of 'uint' in the kernel and it's not used
> > > in the stmmac driver so far. From my point of view I would not
> > > introduce it and stick to the standard 'unsigned int'.
> >
> > Using 'uint' makes the code look cleaner because adding one extra
> > parameter may cause some function declarations to span multiple lines.
> > This change keeps function declarations compact on a single line.
> 
> Marc is right. Just do it.

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

[PATCH v3 net 2/2] net: stmmac: dwmac-imx: pause the TXC clock in fixed-link

[Shenwei Wang]

When using a fixed-link setup, certain devices like the SJA1105 require a
small pause in the TXC clock line to enable their internal tunable
delay line (TDL).

To satisfy this requirement, this patch temporarily disables the TX clock,
and restarts it after a required period. This provides the required
silent interval on the clock line for SJA1105 to complete the frequency
transition and enable the internal TDLs.

So far we have only enabled this feature on the i.MX93 platform.

Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
Reviewed-by: Frank Li <frank.li@nxp.com>
---
 .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 42 +++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
index 53ee5a42c071..2e4173d099f3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
@@ -32,6 +32,7 @@
 #define GPR_ENET_QOS_RGMII_EN		(0x1 << 21)
 
 #define MX93_GPR_ENET_QOS_INTF_MODE_MASK	GENMASK(3, 0)
+#define MX93_GPR_ENET_QOS_INTF_MASK		GENMASK(3, 1)
 #define MX93_GPR_ENET_QOS_INTF_SEL_MII		(0x0 << 1)
 #define MX93_GPR_ENET_QOS_INTF_SEL_RMII		(0x4 << 1)
 #define MX93_GPR_ENET_QOS_INTF_SEL_RGMII	(0x1 << 1)
@@ -40,6 +41,7 @@
 #define DMA_BUS_MODE			0x00001000
 #define DMA_BUS_MODE_SFT_RESET		(0x1 << 0)
 #define RMII_RESET_SPEED		(0x3 << 14)
+#define CTRL_SPEED_MASK			GENMASK(15, 14)
 
 struct imx_dwmac_ops {
 	u32 addr_width;
@@ -56,6 +58,7 @@ struct imx_priv_data {
 	struct regmap *intf_regmap;
 	u32 intf_reg_off;
 	bool rmii_refclk_ext;
+	void __iomem *base_addr;
 
 	const struct imx_dwmac_ops *ops;
 	struct plat_stmmacenet_data *plat_dat;
@@ -212,6 +215,42 @@ static void imx_dwmac_fix_speed(void *priv, uint speed, uint mode)
 		dev_err(dwmac->dev, "failed to set tx rate %lu\n", rate);
 }
 
+static void imx_dwmac_fix_speed_mx93(void *priv, uint speed, uint mode)
+{
+	struct imx_priv_data *dwmac = priv;
+	int ctrl, old_ctrl, iface;
+
+	imx_dwmac_fix_speed(priv, speed, mode);
+
+	if (!dwmac || mode != MLO_AN_FIXED)
+		return;
+
+	if (regmap_read(dwmac->intf_regmap, dwmac->intf_reg_off, &iface))
+		return;
+
+	iface &= MX93_GPR_ENET_QOS_INTF_MASK;
+	if (iface != MX93_GPR_ENET_QOS_INTF_SEL_RGMII)
+		return;
+
+	old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
+	ctrl = old_ctrl & ~CTRL_SPEED_MASK;
+	regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
+			   MX93_GPR_ENET_QOS_INTF_MODE_MASK, 0);
+	writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);
+
+	/* Ensure the settings for CTRL are applied and avoid CPU/Compiler
+	 * reordering.
+	 */
+	wmb();
+
+	usleep_range(10, 20);
+	iface |= MX93_GPR_ENET_QOS_CLK_GEN_EN;
+	regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
+			   MX93_GPR_ENET_QOS_INTF_MODE_MASK, iface);
+
+	writel(old_ctrl, dwmac->base_addr + MAC_CTRL_REG);
+}
+
 static int imx_dwmac_mx93_reset(void *priv, void __iomem *ioaddr)
 {
 	struct plat_stmmacenet_data *plat_dat = priv;
@@ -317,8 +356,11 @@ static int imx_dwmac_probe(struct platform_device *pdev)
 	plat_dat->exit = imx_dwmac_exit;
 	plat_dat->clks_config = imx_dwmac_clks_config;
 	plat_dat->fix_mac_speed = imx_dwmac_fix_speed;
+	if (of_machine_is_compatible("fsl,imx93"))
+		plat_dat->fix_mac_speed = imx_dwmac_fix_speed_mx93;
 	plat_dat->bsp_priv = dwmac;
 	dwmac->plat_dat = plat_dat;
+	dwmac->base_addr = stmmac_res.addr;
 
 	ret = imx_dwmac_clks_config(dwmac, true);
 	if (ret)
-- 
2.34.1


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

Re: [PATCH v3 net 2/2] net: stmmac: dwmac-imx: pause the TXC clock in fixed-link

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 3511 bytes --]

On 31.07.2023 11:19:29, Shenwei Wang wrote:
> When using a fixed-link setup, certain devices like the SJA1105 require a
> small pause in the TXC clock line to enable their internal tunable
> delay line (TDL).
> 
> To satisfy this requirement, this patch temporarily disables the TX clock,
> and restarts it after a required period. This provides the required
> silent interval on the clock line for SJA1105 to complete the frequency
> transition and enable the internal TDLs.
> 
> So far we have only enabled this feature on the i.MX93 platform.
> 
> Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> Reviewed-by: Frank Li <frank.li@nxp.com>
> ---
>  .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 42 +++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> index 53ee5a42c071..2e4173d099f3 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> @@ -32,6 +32,7 @@
>  #define GPR_ENET_QOS_RGMII_EN		(0x1 << 21)
>  
>  #define MX93_GPR_ENET_QOS_INTF_MODE_MASK	GENMASK(3, 0)
> +#define MX93_GPR_ENET_QOS_INTF_MASK		GENMASK(3, 1)
>  #define MX93_GPR_ENET_QOS_INTF_SEL_MII		(0x0 << 1)
>  #define MX93_GPR_ENET_QOS_INTF_SEL_RMII		(0x4 << 1)
>  #define MX93_GPR_ENET_QOS_INTF_SEL_RGMII	(0x1 << 1)
> @@ -40,6 +41,7 @@
>  #define DMA_BUS_MODE			0x00001000
>  #define DMA_BUS_MODE_SFT_RESET		(0x1 << 0)
>  #define RMII_RESET_SPEED		(0x3 << 14)
> +#define CTRL_SPEED_MASK			GENMASK(15, 14)
>  
>  struct imx_dwmac_ops {
>  	u32 addr_width;
> @@ -56,6 +58,7 @@ struct imx_priv_data {
>  	struct regmap *intf_regmap;
>  	u32 intf_reg_off;
>  	bool rmii_refclk_ext;
> +	void __iomem *base_addr;
>  
>  	const struct imx_dwmac_ops *ops;
>  	struct plat_stmmacenet_data *plat_dat;
> @@ -212,6 +215,42 @@ static void imx_dwmac_fix_speed(void *priv, uint speed, uint mode)
>  		dev_err(dwmac->dev, "failed to set tx rate %lu\n", rate);
>  }
>  
> +static void imx_dwmac_fix_speed_mx93(void *priv, uint speed, uint mode)
> +{
> +	struct imx_priv_data *dwmac = priv;
> +	int ctrl, old_ctrl, iface;

regmap_read() wants a pointer to an "unsigned int".

> +
> +	imx_dwmac_fix_speed(priv, speed, mode);
> +
> +	if (!dwmac || mode != MLO_AN_FIXED)
> +		return;
> +
> +	if (regmap_read(dwmac->intf_regmap, dwmac->intf_reg_off, &iface))
> +		return;
> +
> +	iface &= MX93_GPR_ENET_QOS_INTF_MASK;
> +	if (iface != MX93_GPR_ENET_QOS_INTF_SEL_RGMII)
> +		return;
> +
> +	old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
> +	ctrl = old_ctrl & ~CTRL_SPEED_MASK;
> +	regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
> +			   MX93_GPR_ENET_QOS_INTF_MODE_MASK, 0);
> +	writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);
> +
> +	/* Ensure the settings for CTRL are applied and avoid CPU/Compiler
> +	 * reordering.
> +	 */
> +	wmb();
> +
> +	usleep_range(10, 20);
> +	iface |= MX93_GPR_ENET_QOS_CLK_GEN_EN;
> +	regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
> +			   MX93_GPR_ENET_QOS_INTF_MODE_MASK, iface);
> +
> +	writel(old_ctrl, dwmac->base_addr + MAC_CTRL_REG);
> +}

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v3 net 2/2] net: stmmac: dwmac-imx: pause the TXC clock in fixed-link

[Johannes Zink]

Hi Shenwei,

thanks for your patch.

On 7/31/23 18:19, Shenwei Wang wrote:
> When using a fixed-link setup, certain devices like the SJA1105 require a
> small pause in the TXC clock line to enable their internal tunable
> delay line (TDL).

If this is only required for some devices, is it safe to enforce this behaviour 
unconditionally for any kind of fixed link devices connected to the MX93 EQOS 
or could this possibly break for other devices?

Best regards
Johannes

> 
> To satisfy this requirement, this patch temporarily disables the TX clock,
> and restarts it after a required period. This provides the required
> silent interval on the clock line for SJA1105 to complete the frequency
> transition and enable the internal TDLs.
> 
> So far we have only enabled this feature on the i.MX93 platform.
> 
> Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> Reviewed-by: Frank Li <frank.li@nxp.com>
> ---
>   .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 42 +++++++++++++++++++
>   1 file changed, 42 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> index 53ee5a42c071..2e4173d099f3 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> @@ -32,6 +32,7 @@
>   #define GPR_ENET_QOS_RGMII_EN		(0x1 << 21)
>   
>   #define MX93_GPR_ENET_QOS_INTF_MODE_MASK	GENMASK(3, 0)
> +#define MX93_GPR_ENET_QOS_INTF_MASK		GENMASK(3, 1)
>   #define MX93_GPR_ENET_QOS_INTF_SEL_MII		(0x0 << 1)
>   #define MX93_GPR_ENET_QOS_INTF_SEL_RMII		(0x4 << 1)
>   #define MX93_GPR_ENET_QOS_INTF_SEL_RGMII	(0x1 << 1)
> @@ -40,6 +41,7 @@
>   #define DMA_BUS_MODE			0x00001000
>   #define DMA_BUS_MODE_SFT_RESET		(0x1 << 0)
>   #define RMII_RESET_SPEED		(0x3 << 14)
> +#define CTRL_SPEED_MASK			GENMASK(15, 14)
>   
>   struct imx_dwmac_ops {
>   	u32 addr_width;
> @@ -56,6 +58,7 @@ struct imx_priv_data {
>   	struct regmap *intf_regmap;
>   	u32 intf_reg_off;
>   	bool rmii_refclk_ext;
> +	void __iomem *base_addr;
>   
>   	const struct imx_dwmac_ops *ops;
>   	struct plat_stmmacenet_data *plat_dat;
> @@ -212,6 +215,42 @@ static void imx_dwmac_fix_speed(void *priv, uint speed, uint mode)
>   		dev_err(dwmac->dev, "failed to set tx rate %lu\n", rate);
>   }
>   
> +static void imx_dwmac_fix_speed_mx93(void *priv, uint speed, uint mode)
> +{
> +	struct imx_priv_data *dwmac = priv;
> +	int ctrl, old_ctrl, iface;
> +
> +	imx_dwmac_fix_speed(priv, speed, mode);
> +
> +	if (!dwmac || mode != MLO_AN_FIXED)
> +		return;
> +
> +	if (regmap_read(dwmac->intf_regmap, dwmac->intf_reg_off, &iface))
> +		return;
> +
> +	iface &= MX93_GPR_ENET_QOS_INTF_MASK;
> +	if (iface != MX93_GPR_ENET_QOS_INTF_SEL_RGMII)
> +		return;
> +
> +	old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
> +	ctrl = old_ctrl & ~CTRL_SPEED_MASK;
> +	regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
> +			   MX93_GPR_ENET_QOS_INTF_MODE_MASK, 0);
> +	writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);
> +
> +	/* Ensure the settings for CTRL are applied and avoid CPU/Compiler
> +	 * reordering.
> +	 */
> +	wmb();
> +
> +	usleep_range(10, 20);
> +	iface |= MX93_GPR_ENET_QOS_CLK_GEN_EN;
> +	regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
> +			   MX93_GPR_ENET_QOS_INTF_MODE_MASK, iface);
> +
> +	writel(old_ctrl, dwmac->base_addr + MAC_CTRL_REG);
> +}
> +
>   static int imx_dwmac_mx93_reset(void *priv, void __iomem *ioaddr)
>   {
>   	struct plat_stmmacenet_data *plat_dat = priv;
> @@ -317,8 +356,11 @@ static int imx_dwmac_probe(struct platform_device *pdev)
>   	plat_dat->exit = imx_dwmac_exit;
>   	plat_dat->clks_config = imx_dwmac_clks_config;
>   	plat_dat->fix_mac_speed = imx_dwmac_fix_speed;
> +	if (of_machine_is_compatible("fsl,imx93"))
> +		plat_dat->fix_mac_speed = imx_dwmac_fix_speed_mx93;
>   	plat_dat->bsp_priv = dwmac;
>   	dwmac->plat_dat = plat_dat;
> +	dwmac->base_addr = stmmac_res.addr;
>   
>   	ret = imx_dwmac_clks_config(dwmac, true);
>   	if (ret)

-- 
Pengutronix e.K.                | Johannes Zink                  |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |


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

Re: [PATCH v3 net 2/2] net: stmmac: dwmac-imx: pause the TXC clock in fixed-link

[Russell King (Oracle)]

On Tue, Aug 01, 2023 at 02:47:46PM +0200, Johannes Zink wrote:
> Hi Shenwei,
> 
> thanks for your patch.
> 
> On 7/31/23 18:19, Shenwei Wang wrote:
> > When using a fixed-link setup, certain devices like the SJA1105 require a
> > small pause in the TXC clock line to enable their internal tunable
> > delay line (TDL).
> 
> If this is only required for some devices, is it safe to enforce this
> behaviour unconditionally for any kind of fixed link devices connected to
> the MX93 EQOS or could this possibly break for other devices?

This same point has been raised by Andrew Halaney in message-id
 <4govb566nypifbtqp5lcbsjhvoyble5luww3onaa2liinboguf@4kgihys6vhrg>
and Fabio Estevam in message-id
 <CAOMZO5ANQmVbk_jy7qdVtzs3716FisT2c72W+3WZyu7FoAochw@mail.gmail.com>
but we don't seem to have any answer for it.

Also, the patch still uses wmb() between the write and the delay, and as
Will Deacon pointed out in his message, message-id
 <20230728153611.GH21718@willie-the-truck>
this is not safe, yet still a new version was sent.

It seems the author of these patches is pretty resistant to comments,
and has shown that when I was requesting changes - it was an awful
struggle to get changes made. I'm now of the opinion that I really
can't be bothered to review these patches, precisely because feedback
is clearly not welcome or if welcome, apparently acted upon.

-- 
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: [EXT] Re: [PATCH v3 net 2/2] net: stmmac: dwmac-imx: pause the TXC clock in fixed-link

[Shenwei Wang]

> -----Original Message-----
> From: Russell King <linux@armlinux.org.uk>
> Sent: Tuesday, August 1, 2023 7:57 AM
> To: Johannes Zink <j.zink@pengutronix.de>
> Cc: Shenwei Wang <shenwei.wang@nxp.com>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Maxime
> Coquelin <mcoquelin.stm32@gmail.com>; Shawn Guo <shawnguo@kernel.org>;
> Sascha Hauer <s.hauer@pengutronix.de>; Neil Armstrong
> <neil.armstrong@linaro.org>; Kevin Hilman <khilman@baylibre.com>; Vinod
> Koul <vkoul@kernel.org>; Chen-Yu Tsai <wens@csie.org>; Jernej Skrabec
> <jernej.skrabec@gmail.com>; Samuel Holland <samuel@sholland.org>;
> Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; Jerome Brunet
> <jbrunet@baylibre.com>; Martin Blumenstingl
> <martin.blumenstingl@googlemail.com>; Bhupesh Sharma
> <bhupesh.sharma@linaro.org>; Nobuhiro Iwamatsu
> <nobuhiro1.iwamatsu@toshiba.co.jp>; Simon Horman
> <simon.horman@corigine.com>; Andrew Halaney <ahalaney@redhat.com>;
> Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Wong Vee Khee
> <veekhee@apple.com>; Revanth Kumar Uppala <ruppala@nvidia.com>; Jochen
> Henneberg <jh@henneberg-systemdesign.com>; netdev@vger.kernel.org; linux-
> stm32@st-md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; linux-amlogic@lists.infradead.org;
> imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
> Subject: [EXT] Re: [PATCH v3 net 2/2] net: stmmac: dwmac-imx: pause the TXC
> clock in fixed-link
>
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report this
> email' button
>
>
> On Tue, Aug 01, 2023 at 02:47:46PM +0200, Johannes Zink wrote:
> > Hi Shenwei,
> >
> > thanks for your patch.
> >
> > On 7/31/23 18:19, Shenwei Wang wrote:
> > > When using a fixed-link setup, certain devices like the SJA1105
> > > require a small pause in the TXC clock line to enable their internal
> > > tunable delay line (TDL).
> >
> > If this is only required for some devices, is it safe to enforce this
> > behaviour unconditionally for any kind of fixed link devices connected
> > to the MX93 EQOS or could this possibly break for other devices?
>
> This same point has been raised by Andrew Halaney in message-id
> <4govb566nypifbtqp5lcbsjhvoyble5luww3onaa2liinboguf@4kgihys6vhrg>
> and Fabio Estevam in message-id
>
> <CAOMZO5ANQmVbk_jy7qdVtzs3716FisT2c72W+3WZyu7FoAochw@mail.gmail.
> com>
> but we don't seem to have any answer for it.
>
Hi Russell,

I hope you have thoroughly read all of my earlier responses, as I believe I already addressed this question.
I'm happy to clarify further, but kindly avoid unsubstantiated comments.

https://lore.kernel.org/imx/20230727152503.2199550-1-shenwei.wang@nxp.com/T/#m08da3797a056d4d8ea4c1d8956b445ae967e7cfa
" Yes, that's the purpose because it won't hurt even the other side is not SJA1105."

> Also, the patch still uses wmb() between the write and the delay, and as Will
> Deacon pointed out in his message, message-id
> <20230728153611.GH21718@willie-the-truck>
> this is not safe, yet still a new version was sent.
>

Can we conclude that even without the wmb() here, the desired delay time between
operations can still be ensured?

Thanks,
Shenwei

> It seems the author of these patches is pretty resistant to comments, and has
> shown that when I was requesting changes - it was an awful struggle to get
> changes made. I'm now of the opinion that I really can't be bothered to review
> these patches, precisely because feedback is clearly not welcome or if welcome,
> apparently acted upon.
>
> --
> RMK's Patch system:
> https://www.ar/
> mlinux.org.uk%2Fdeveloper%2Fpatches%2F&data=05%7C01%7Cshenwei.wang
> %40nxp.com%7Ce65ab380ff5b4748da5308db928ec751%7C686ea1d3bc2b4c6fa
> 92cd99c5c301635%7C0%7C0%7C638264914150592989%7CUnknown%7CTWFp
> bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> Mn0%3D%7C3000%7C%7C%7C&sdata=%2FzSqRqJFRQljX6ky3XJvfkMH9PwgOstb
> w8HpEppYOIM%3D&reserved=0
> 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: [EXT] Re: [PATCH v3 net 2/2] net: stmmac: dwmac-imx: pause the TXC clock in fixed-link

[Andrew Halaney]

On Tue, Aug 01, 2023 at 05:06:46PM +0000, Shenwei Wang wrote:
> 
> 
> > -----Original Message-----
> > From: Russell King <linux@armlinux.org.uk>
> > Sent: Tuesday, August 1, 2023 7:57 AM
> > To: Johannes Zink <j.zink@pengutronix.de>
> > Cc: Shenwei Wang <shenwei.wang@nxp.com>; David S. Miller
> > <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> > Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Maxime
> > Coquelin <mcoquelin.stm32@gmail.com>; Shawn Guo <shawnguo@kernel.org>;
> > Sascha Hauer <s.hauer@pengutronix.de>; Neil Armstrong
> > <neil.armstrong@linaro.org>; Kevin Hilman <khilman@baylibre.com>; Vinod
> > Koul <vkoul@kernel.org>; Chen-Yu Tsai <wens@csie.org>; Jernej Skrabec
> > <jernej.skrabec@gmail.com>; Samuel Holland <samuel@sholland.org>;
> > Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> > <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> > Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> > <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; Jerome Brunet
> > <jbrunet@baylibre.com>; Martin Blumenstingl
> > <martin.blumenstingl@googlemail.com>; Bhupesh Sharma
> > <bhupesh.sharma@linaro.org>; Nobuhiro Iwamatsu
> > <nobuhiro1.iwamatsu@toshiba.co.jp>; Simon Horman
> > <simon.horman@corigine.com>; Andrew Halaney <ahalaney@redhat.com>;
> > Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Wong Vee Khee
> > <veekhee@apple.com>; Revanth Kumar Uppala <ruppala@nvidia.com>; Jochen
> > Henneberg <jh@henneberg-systemdesign.com>; netdev@vger.kernel.org; linux-
> > stm32@st-md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org;
> > linux-kernel@vger.kernel.org; linux-amlogic@lists.infradead.org;
> > imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
> > Subject: [EXT] Re: [PATCH v3 net 2/2] net: stmmac: dwmac-imx: pause the TXC
> > clock in fixed-link
> >
> > Caution: This is an external email. Please take care when clicking links or
> > opening attachments. When in doubt, report the message using the 'Report this
> > email' button
> >
> >
> > On Tue, Aug 01, 2023 at 02:47:46PM +0200, Johannes Zink wrote:
> > > Hi Shenwei,
> > >
> > > thanks for your patch.
> > >
> > > On 7/31/23 18:19, Shenwei Wang wrote:
> > > > When using a fixed-link setup, certain devices like the SJA1105
> > > > require a small pause in the TXC clock line to enable their internal
> > > > tunable delay line (TDL).
> > >
> > > If this is only required for some devices, is it safe to enforce this
> > > behaviour unconditionally for any kind of fixed link devices connected
> > > to the MX93 EQOS or could this possibly break for other devices?
> >
> > This same point has been raised by Andrew Halaney in message-id
> > <4govb566nypifbtqp5lcbsjhvoyble5luww3onaa2liinboguf@4kgihys6vhrg>
> > and Fabio Estevam in message-id
> >
> > <CAOMZO5ANQmVbk_jy7qdVtzs3716FisT2c72W+3WZyu7FoAochw@mail.gmail.
> > com>
> > but we don't seem to have any answer for it.
> >
> Hi Russell,
> 
> I hope you have thoroughly read all of my earlier responses, as I believe I already addressed this question.
> I'm happy to clarify further, but kindly avoid unsubstantiated comments.
> 
> https://lore.kernel.org/imx/20230727152503.2199550-1-shenwei.wang@nxp.com/T/#m08da3797a056d4d8ea4c1d8956b445ae967e7cfa
> " Yes, that's the purpose because it won't hurt even the other side is not SJA1105."
> 
> > Also, the patch still uses wmb() between the write and the delay, and as Will
> > Deacon pointed out in his message, message-id
> > <20230728153611.GH21718@willie-the-truck>
> > this is not safe, yet still a new version was sent.
> >
> 
> Can we conclude that even without the wmb() here, the desired delay time between
> operations can still be ensured?

Will's talk[0] he linked has the sequence you've done here (writel's
followed by wmb() followed by a udelay), and he states it is wrong if
the goal is for the device to see the writes prior to the udelay. That's
discussed at around 28:00 and followed up by (thankfully, cuz I too
didn't understand it) a question at 34:10 to discuss why mb() isn't
sufficient (it completes the write, but the device *may not* see it
yet, the read forces that).

He mentioned that over at [1] in the review here, and suggested reading
from the device again prior to the udelay() instead to force the writes
to take affect on the device prior to the udelay.

I found a quick example in the ufs-qcom.c driver that I'll copy paste
here too from upstream that follows this advice:

		writel_relaxed(temp, host->dev_ref_clk_ctrl_mmio);

		/*
		 * Make sure the write to ref_clk reaches the destination and
		 * not stored in a Write Buffer (WB).
		 */
		readl(host->dev_ref_clk_ctrl_mmio);

		/*
		 * If we call hibern8 exit after this, we need to make sure that
		 * device ref_clk is stable for at least 1us before the hibern8
		 * exit command.
		 */
		if (enable)
			udelay(1);


[0] https://www.youtube.com/watch?v=i6DayghhA8Q
[1] https://lore.kernel.org/netdev/20230728153611.GH21718@willie-the-truck/

I hope that helps,
Andrew


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

Re: [EXT] Re: [PATCH v3 net 2/2] net: stmmac: dwmac-imx: pause the TXC clock in fixed-link

[Russell King (Oracle)]

On Tue, Aug 01, 2023 at 05:06:46PM +0000, Shenwei Wang wrote:
> > On Tue, Aug 01, 2023 at 02:47:46PM +0200, Johannes Zink wrote:
> > > Hi Shenwei,
> > >
> > > thanks for your patch.
> > >
> > > On 7/31/23 18:19, Shenwei Wang wrote:
> > > > When using a fixed-link setup, certain devices like the SJA1105
> > > > require a small pause in the TXC clock line to enable their internal
> > > > tunable delay line (TDL).
> > >
> > > If this is only required for some devices, is it safe to enforce this
> > > behaviour unconditionally for any kind of fixed link devices connected
> > > to the MX93 EQOS or could this possibly break for other devices?
> >
> > This same point has been raised by Andrew Halaney in message-id
> > <4govb566nypifbtqp5lcbsjhvoyble5luww3onaa2liinboguf@4kgihys6vhrg>
> > and Fabio Estevam in message-id
> >
> > <CAOMZO5ANQmVbk_jy7qdVtzs3716FisT2c72W+3WZyu7FoAochw@mail.gmail.
> > com>
> > but we don't seem to have any answer for it.
> >
> Hi Russell,
> 
> I hope you have thoroughly read all of my earlier responses, as I believe I already addressed this question.
> I'm happy to clarify further, but kindly avoid unsubstantiated comments.
> 
> https://lore.kernel.org/imx/20230727152503.2199550-1-shenwei.wang@nxp.com/T/#m08da3797a056d4d8ea4c1d8956b445ae967e7cfa
> " Yes, that's the purpose because it won't hurt even the other side is not SJA1105."

So, why not include the answer in the commit message given that you've
had to answer it several times already?

> > Also, the patch still uses wmb() between the write and the delay, and as Will
> > Deacon pointed out in his message, message-id
> > <20230728153611.GH21718@willie-the-truck>
> > this is not safe, yet still a new version was sent.
> >
> 
> Can we conclude that even without the wmb() here, the desired delay time between
> operations can still be ensured?

How did you come to that conclusion? I see no further discussion after
I raised this, Will replied, and you suggested a read-back. However,
that isn't what you've implemented on v3, you've gone back to what
looks like the original code in v2 which brought up this question - and
Will indicated it was _unsafe_.

-- 
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: [EXT] Re: [PATCH v3 net 2/2] net: stmmac: dwmac-imx: pause the TXC clock in fixed-link

[Shenwei Wang]

> -----Original Message-----
> From: Johannes Zink <j.zink@pengutronix.de>
> Sent: Tuesday, August 1, 2023 7:48 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>; Russell King
> <linux@armlinux.org.uk>; David S. Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> Abeni <pabeni@redhat.com>; Maxime Coquelin
> <mcoquelin.stm32@gmail.com>; Shawn Guo <shawnguo@kernel.org>; Sascha
> Hauer <s.hauer@pengutronix.de>; Neil Armstrong <neil.armstrong@linaro.org>;
> Kevin Hilman <khilman@baylibre.com>; Vinod Koul <vkoul@kernel.org>; Chen-
> Yu Tsai <wens@csie.org>; Jernej Skrabec <jernej.skrabec@gmail.com>; Samuel
> Holland <samuel@sholland.org>
> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; Jerome Brunet
> <jbrunet@baylibre.com>; Martin Blumenstingl
> <martin.blumenstingl@googlemail.com>; Bhupesh Sharma
> <bhupesh.sharma@linaro.org>; Nobuhiro Iwamatsu
> <nobuhiro1.iwamatsu@toshiba.co.jp>; Simon Horman
> <simon.horman@corigine.com>; Andrew Halaney <ahalaney@redhat.com>;
> Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Wong Vee Khee
> <veekhee@apple.com>; Revanth Kumar Uppala <ruppala@nvidia.com>; Jochen
> Henneberg <jh@henneberg-systemdesign.com>; netdev@vger.kernel.org; linux-
> stm32@st-md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; linux-amlogic@lists.infradead.org;
> imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
> Subject: [EXT] Re: [PATCH v3 net 2/2] net: stmmac: dwmac-imx: pause the TXC
> clock in fixed-link
>
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report this
> email' button
>
>
> Hi Shenwei,
>
> thanks for your patch.
>
> On 7/31/23 18:19, Shenwei Wang wrote:
> > When using a fixed-link setup, certain devices like the SJA1105
> > require a small pause in the TXC clock line to enable their internal
> > tunable delay line (TDL).
>
> If this is only required for some devices, is it safe to enforce this behaviour
> unconditionally for any kind of fixed link devices connected to the MX93 EQOS
> or could this possibly break for other devices?
>

It won't impact normal devices. The link layer hasn't built up yet.

Thanks,
Shenwei

> Best regards
> Johannes
>
> >
> > To satisfy this requirement, this patch temporarily disables the TX
> > clock, and restarts it after a required period. This provides the
> > required silent interval on the clock line for SJA1105 to complete the
> > frequency transition and enable the internal TDLs.
> >
> > So far we have only enabled this feature on the i.MX93 platform.
> >
> > Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> > Reviewed-by: Frank Li <frank.li@nxp.com>
> > ---
> >   .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 42 +++++++++++++++++++
> >   1 file changed, 42 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > index 53ee5a42c071..2e4173d099f3 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > @@ -32,6 +32,7 @@
> >   #define GPR_ENET_QOS_RGMII_EN               (0x1 << 21)
> >
> >   #define MX93_GPR_ENET_QOS_INTF_MODE_MASK    GENMASK(3, 0)
> > +#define MX93_GPR_ENET_QOS_INTF_MASK          GENMASK(3, 1)
> >   #define MX93_GPR_ENET_QOS_INTF_SEL_MII              (0x0 << 1)
> >   #define MX93_GPR_ENET_QOS_INTF_SEL_RMII             (0x4 << 1)
> >   #define MX93_GPR_ENET_QOS_INTF_SEL_RGMII    (0x1 << 1)
> > @@ -40,6 +41,7 @@
> >   #define DMA_BUS_MODE                        0x00001000
> >   #define DMA_BUS_MODE_SFT_RESET              (0x1 << 0)
> >   #define RMII_RESET_SPEED            (0x3 << 14)
> > +#define CTRL_SPEED_MASK                      GENMASK(15, 14)
> >
> >   struct imx_dwmac_ops {
> >       u32 addr_width;
> > @@ -56,6 +58,7 @@ struct imx_priv_data {
> >       struct regmap *intf_regmap;
> >       u32 intf_reg_off;
> >       bool rmii_refclk_ext;
> > +     void __iomem *base_addr;
> >
> >       const struct imx_dwmac_ops *ops;
> >       struct plat_stmmacenet_data *plat_dat; @@ -212,6 +215,42 @@
> > static void imx_dwmac_fix_speed(void *priv, uint speed, uint mode)
> >               dev_err(dwmac->dev, "failed to set tx rate %lu\n", rate);
> >   }
> >
> > +static void imx_dwmac_fix_speed_mx93(void *priv, uint speed, uint
> > +mode) {
> > +     struct imx_priv_data *dwmac = priv;
> > +     int ctrl, old_ctrl, iface;
> > +
> > +     imx_dwmac_fix_speed(priv, speed, mode);
> > +
> > +     if (!dwmac || mode != MLO_AN_FIXED)
> > +             return;
> > +
> > +     if (regmap_read(dwmac->intf_regmap, dwmac->intf_reg_off, &iface))
> > +             return;
> > +
> > +     iface &= MX93_GPR_ENET_QOS_INTF_MASK;
> > +     if (iface != MX93_GPR_ENET_QOS_INTF_SEL_RGMII)
> > +             return;
> > +
> > +     old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
> > +     ctrl = old_ctrl & ~CTRL_SPEED_MASK;
> > +     regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
> > +                        MX93_GPR_ENET_QOS_INTF_MODE_MASK, 0);
> > +     writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);
> > +
> > +     /* Ensure the settings for CTRL are applied and avoid CPU/Compiler
> > +      * reordering.
> > +      */
> > +     wmb();
> > +
> > +     usleep_range(10, 20);
> > +     iface |= MX93_GPR_ENET_QOS_CLK_GEN_EN;
> > +     regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
> > +                        MX93_GPR_ENET_QOS_INTF_MODE_MASK, iface);
> > +
> > +     writel(old_ctrl, dwmac->base_addr + MAC_CTRL_REG); }
> > +
> >   static int imx_dwmac_mx93_reset(void *priv, void __iomem *ioaddr)
> >   {
> >       struct plat_stmmacenet_data *plat_dat = priv; @@ -317,8 +356,11
> > @@ static int imx_dwmac_probe(struct platform_device *pdev)
> >       plat_dat->exit = imx_dwmac_exit;
> >       plat_dat->clks_config = imx_dwmac_clks_config;
> >       plat_dat->fix_mac_speed = imx_dwmac_fix_speed;
> > +     if (of_machine_is_compatible("fsl,imx93"))
> > +             plat_dat->fix_mac_speed = imx_dwmac_fix_speed_mx93;
> >       plat_dat->bsp_priv = dwmac;
> >       dwmac->plat_dat = plat_dat;
> > +     dwmac->base_addr = stmmac_res.addr;
> >
> >       ret = imx_dwmac_clks_config(dwmac, true);
> >       if (ret)
>
> --
> Pengutronix e.K.                | Johannes Zink                  |
> Steuerwalder Str. 21            |
> https://www.pe/
> ngutronix.de%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7C761fbb75c
> 1c24cfe091508db928d8ade%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
> 0%7C638264908852977732%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
> wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%
> 7C&sdata=2l2zNfIaNnRJENmERehNae8g%2F%2BQqlxD2YRx7ksY2X%2BE%3D&r
> eserved=0    |
> 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
> Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |


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

Re: [EXT] Re: [PATCH v3 net 2/2] net: stmmac: dwmac-imx: pause the TXC clock in fixed-link

[Johannes Zink]

Hi Shenwei,

On 8/1/23 19:10, Shenwei Wang wrote:
> 
> 
>> -----Original Message-----
>> From: Johannes Zink <j.zink@pengutronix.de>
>> Sent: Tuesday, August 1, 2023 7:48 AM
>> To: Shenwei Wang <shenwei.wang@nxp.com>; Russell King
>> <linux@armlinux.org.uk>; David S. Miller <davem@davemloft.net>; Eric
>> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
>> Abeni <pabeni@redhat.com>; Maxime Coquelin
>> <mcoquelin.stm32@gmail.com>; Shawn Guo <shawnguo@kernel.org>; Sascha
>> Hauer <s.hauer@pengutronix.de>; Neil Armstrong <neil.armstrong@linaro.org>;
>> Kevin Hilman <khilman@baylibre.com>; Vinod Koul <vkoul@kernel.org>; Chen-
>> Yu Tsai <wens@csie.org>; Jernej Skrabec <jernej.skrabec@gmail.com>; Samuel
>> Holland <samuel@sholland.org>
>> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
>> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
>> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
>> <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; Jerome Brunet
>> <jbrunet@baylibre.com>; Martin Blumenstingl
>> <martin.blumenstingl@googlemail.com>; Bhupesh Sharma
>> <bhupesh.sharma@linaro.org>; Nobuhiro Iwamatsu
>> <nobuhiro1.iwamatsu@toshiba.co.jp>; Simon Horman
>> <simon.horman@corigine.com>; Andrew Halaney <ahalaney@redhat.com>;
>> Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Wong Vee Khee
>> <veekhee@apple.com>; Revanth Kumar Uppala <ruppala@nvidia.com>; Jochen
>> Henneberg <jh@henneberg-systemdesign.com>; netdev@vger.kernel.org; linux-
>> stm32@st-md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org;
>> linux-kernel@vger.kernel.org; linux-amlogic@lists.infradead.org;
>> imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
>> Subject: [EXT] Re: [PATCH v3 net 2/2] net: stmmac: dwmac-imx: pause the TXC
>> clock in fixed-link
>>
>> Caution: This is an external email. Please take care when clicking links or
>> opening attachments. When in doubt, report the message using the 'Report this
>> email' button
>>
>>
>> Hi Shenwei,
>>
>> thanks for your patch.
>>
>> On 7/31/23 18:19, Shenwei Wang wrote:
>>> When using a fixed-link setup, certain devices like the SJA1105
>>> require a small pause in the TXC clock line to enable their internal
>>> tunable delay line (TDL).
>>
>> If this is only required for some devices, is it safe to enforce this behaviour
>> unconditionally for any kind of fixed link devices connected to the MX93 EQOS
>> or could this possibly break for other devices?
>>
> 
> It won't impact normal devices. The link layer hasn't built up yet.
> 

As Russel suggested in [1] - maybe you could rephrase your commit message for 
your v4 to point this out to future reviewers (apparently multiple people have 
had questions about this...)  and have this fact also recorded in the git log 
later on.

Also: does this only apply to i.MX93, or would we have to test and enable it on 
e.g. i.MX8MP as well?

Thanks
Johannes

[1] ZMk/xqRP67zXHNrf@shell.armlinux.org.uk


> Thanks,
> Shenwei
> 
>> Best regards
>> Johannes
>>
>>>
>>> To satisfy this requirement, this patch temporarily disables the TX
>>> clock, and restarts it after a required period. This provides the
>>> required silent interval on the clock line for SJA1105 to complete the
>>> frequency transition and enable the internal TDLs.
>>>
>>> So far we have only enabled this feature on the i.MX93 platform.
>>>
>>> Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
>>> Reviewed-by: Frank Li <frank.li@nxp.com>
>>> ---
>>>    .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 42 +++++++++++++++++++
>>>    1 file changed, 42 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
>>> b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
>>> index 53ee5a42c071..2e4173d099f3 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
>>> @@ -32,6 +32,7 @@
>>>    #define GPR_ENET_QOS_RGMII_EN               (0x1 << 21)
>>>
>>>    #define MX93_GPR_ENET_QOS_INTF_MODE_MASK    GENMASK(3, 0)
>>> +#define MX93_GPR_ENET_QOS_INTF_MASK          GENMASK(3, 1)
>>>    #define MX93_GPR_ENET_QOS_INTF_SEL_MII              (0x0 << 1)
>>>    #define MX93_GPR_ENET_QOS_INTF_SEL_RMII             (0x4 << 1)
>>>    #define MX93_GPR_ENET_QOS_INTF_SEL_RGMII    (0x1 << 1)
>>> @@ -40,6 +41,7 @@
>>>    #define DMA_BUS_MODE                        0x00001000
>>>    #define DMA_BUS_MODE_SFT_RESET              (0x1 << 0)
>>>    #define RMII_RESET_SPEED            (0x3 << 14)
>>> +#define CTRL_SPEED_MASK                      GENMASK(15, 14)
>>>
>>>    struct imx_dwmac_ops {
>>>        u32 addr_width;
>>> @@ -56,6 +58,7 @@ struct imx_priv_data {
>>>        struct regmap *intf_regmap;
>>>        u32 intf_reg_off;
>>>        bool rmii_refclk_ext;
>>> +     void __iomem *base_addr;
>>>
>>>        const struct imx_dwmac_ops *ops;
>>>        struct plat_stmmacenet_data *plat_dat; @@ -212,6 +215,42 @@
>>> static void imx_dwmac_fix_speed(void *priv, uint speed, uint mode)
>>>                dev_err(dwmac->dev, "failed to set tx rate %lu\n", rate);
>>>    }
>>>
>>> +static void imx_dwmac_fix_speed_mx93(void *priv, uint speed, uint
>>> +mode) {
>>> +     struct imx_priv_data *dwmac = priv;
>>> +     int ctrl, old_ctrl, iface;
>>> +
>>> +     imx_dwmac_fix_speed(priv, speed, mode);
>>> +
>>> +     if (!dwmac || mode != MLO_AN_FIXED)
>>> +             return;
>>> +
>>> +     if (regmap_read(dwmac->intf_regmap, dwmac->intf_reg_off, &iface))
>>> +             return;
>>> +
>>> +     iface &= MX93_GPR_ENET_QOS_INTF_MASK;
>>> +     if (iface != MX93_GPR_ENET_QOS_INTF_SEL_RGMII)
>>> +             return;
>>> +
>>> +     old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
>>> +     ctrl = old_ctrl & ~CTRL_SPEED_MASK;
>>> +     regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
>>> +                        MX93_GPR_ENET_QOS_INTF_MODE_MASK, 0);
>>> +     writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);
>>> +
>>> +     /* Ensure the settings for CTRL are applied and avoid CPU/Compiler
>>> +      * reordering.
>>> +      */
>>> +     wmb();
>>> +
>>> +     usleep_range(10, 20);
>>> +     iface |= MX93_GPR_ENET_QOS_CLK_GEN_EN;
>>> +     regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
>>> +                        MX93_GPR_ENET_QOS_INTF_MODE_MASK, iface);
>>> +
>>> +     writel(old_ctrl, dwmac->base_addr + MAC_CTRL_REG); }
>>> +
>>>    static int imx_dwmac_mx93_reset(void *priv, void __iomem *ioaddr)
>>>    {
>>>        struct plat_stmmacenet_data *plat_dat = priv; @@ -317,8 +356,11
>>> @@ static int imx_dwmac_probe(struct platform_device *pdev)
>>>        plat_dat->exit = imx_dwmac_exit;
>>>        plat_dat->clks_config = imx_dwmac_clks_config;
>>>        plat_dat->fix_mac_speed = imx_dwmac_fix_speed;
>>> +     if (of_machine_is_compatible("fsl,imx93"))
>>> +             plat_dat->fix_mac_speed = imx_dwmac_fix_speed_mx93;
>>>        plat_dat->bsp_priv = dwmac;
>>>        dwmac->plat_dat = plat_dat;
>>> +     dwmac->base_addr = stmmac_res.addr;
>>>
>>>        ret = imx_dwmac_clks_config(dwmac, true);
>>>        if (ret)
>>
>> --
>> Pengutronix e.K.                | Johannes Zink                  |
>> Steuerwalder Str. 21            |
>> https://www.pe/
>> ngutronix.de%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7C761fbb75c
>> 1c24cfe091508db928d8ade%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
>> 0%7C638264908852977732%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
>> wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%
>> 7C&sdata=2l2zNfIaNnRJENmERehNae8g%2F%2BQqlxD2YRx7ksY2X%2BE%3D&r
>> eserved=0    |
>> 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
>> Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |
> 
> 

-- 
Pengutronix e.K.                | Johannes Zink                  |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |


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

RE: [EXT] Re: [PATCH v3 net 2/2] net: stmmac: dwmac-imx: pause the TXC clock in fixed-link

[Shenwei Wang]

> -----Original Message-----
> From: Johannes Zink <j.zink@pengutronix.de>
> Sent: Wednesday, August 2, 2023 1:26 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>; Russell King
> <linux@armlinux.org.uk>; David S. Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> Abeni <pabeni@redhat.com>; Maxime Coquelin
> <mcoquelin.stm32@gmail.com>; Shawn Guo <shawnguo@kernel.org>; Sascha
> Hauer <s.hauer@pengutronix.de>; Neil Armstrong <neil.armstrong@linaro.org>;
> Kevin Hilman <khilman@baylibre.com>; Vinod Koul <vkoul@kernel.org>; Chen-
> Yu Tsai <wens@csie.org>; Jernej Skrabec <jernej.skrabec@gmail.com>; Samuel
> Holland <samuel@sholland.org>
> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; Jerome Brunet
> <jbrunet@baylibre.com>; Martin Blumenstingl
> <martin.blumenstingl@googlemail.com>; Bhupesh Sharma
> <bhupesh.sharma@linaro.org>; Nobuhiro Iwamatsu
> <nobuhiro1.iwamatsu@toshiba.co.jp>; Simon Horman
> <simon.horman@corigine.com>; Andrew Halaney <ahalaney@redhat.com>;
> Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Wong Vee Khee
> <veekhee@apple.com>; Revanth Kumar Uppala <ruppala@nvidia.com>; Jochen
> Henneberg <jh@henneberg-systemdesign.com>; netdev@vger.kernel.org; linux-
> stm32@st-md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; linux-amlogic@lists.infradead.org;
> imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
> Subject: Re: [EXT] Re: [PATCH v3 net 2/2] net: stmmac: dwmac-imx: pause the
> TXC clock in fixed-link
>
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report this
> email' button
>
>
> Hi Shenwei,
>
> On 8/1/23 19:10, Shenwei Wang wrote:
> >
> >
> >> -----Original Message-----
> >> From: Johannes Zink <j.zink@pengutronix.de>
> >> Sent: Tuesday, August 1, 2023 7:48 AM
> >> To: Shenwei Wang <shenwei.wang@nxp.com>; Russell King
> >> <linux@armlinux.org.uk>; David S. Miller <davem@davemloft.net>; Eric
> >> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>;
> >> Paolo Abeni <pabeni@redhat.com>; Maxime Coquelin
> >> <mcoquelin.stm32@gmail.com>; Shawn Guo <shawnguo@kernel.org>;
> Sascha
> >> Hauer <s.hauer@pengutronix.de>; Neil Armstrong
> >> <neil.armstrong@linaro.org>; Kevin Hilman <khilman@baylibre.com>;
> >> Vinod Koul <vkoul@kernel.org>; Chen- Yu Tsai <wens@csie.org>; Jernej
> >> Skrabec <jernej.skrabec@gmail.com>; Samuel Holland
> >> <samuel@sholland.org>
> >> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> >> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> >> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> >> <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; Jerome Brunet
> >> <jbrunet@baylibre.com>; Martin Blumenstingl
> >> <martin.blumenstingl@googlemail.com>; Bhupesh Sharma
> >> <bhupesh.sharma@linaro.org>; Nobuhiro Iwamatsu
> >> <nobuhiro1.iwamatsu@toshiba.co.jp>; Simon Horman
> >> <simon.horman@corigine.com>; Andrew Halaney <ahalaney@redhat.com>;
> >> Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Wong Vee Khee
> >> <veekhee@apple.com>; Revanth Kumar Uppala <ruppala@nvidia.com>;
> >> Jochen Henneberg <jh@henneberg-systemdesign.com>;
> >> netdev@vger.kernel.org; linux- stm32@st-md-mailman.stormreply.com;
> >> linux-arm-kernel@lists.infradead.org;
> >> linux-kernel@vger.kernel.org; linux-amlogic@lists.infradead.org;
> >> imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
> >> Subject: [EXT] Re: [PATCH v3 net 2/2] net: stmmac: dwmac-imx: pause
> >> the TXC clock in fixed-link
> >>
> >> Caution: This is an external email. Please take care when clicking
> >> links or opening attachments. When in doubt, report the message using
> >> the 'Report this email' button
> >>
> >>
> >> Hi Shenwei,
> >>
> >> thanks for your patch.
> >>
> >> On 7/31/23 18:19, Shenwei Wang wrote:
> >>> When using a fixed-link setup, certain devices like the SJA1105
> >>> require a small pause in the TXC clock line to enable their internal
> >>> tunable delay line (TDL).
> >>
> >> If this is only required for some devices, is it safe to enforce this
> >> behaviour unconditionally for any kind of fixed link devices
> >> connected to the MX93 EQOS or could this possibly break for other devices?
> >>
> >
> > It won't impact normal devices. The link layer hasn't built up yet.
> >
>
> As Russel suggested in [1] - maybe you could rephrase your commit message for
> your v4 to point this out to future reviewers (apparently multiple people have
> had questions about this...)  and have this fact also recorded in the git log later
> on.
>

Okay.

> Also: does this only apply to i.MX93, or would we have to test and enable it on
> e.g. i.MX8MP as well?
>

Yes, it is required when the EQOS MAC is selected. However, this patch just enables
The feature on i.MX93.

Thanks,
Shenwei

> Thanks
> Johannes
>
> [1] ZMk/xqRP67zXHNrf@shell.armlinux.org.uk
>
>
> > Thanks,
> > Shenwei
> >
> >> Best regards
> >> Johannes
> >>
> >>>
> >>> To satisfy this requirement, this patch temporarily disables the TX
> >>> clock, and restarts it after a required period. This provides the
> >>> required silent interval on the clock line for SJA1105 to complete
> >>> the frequency transition and enable the internal TDLs.
> >>>
> >>> So far we have only enabled this feature on the i.MX93 platform.
> >>>
> >>> Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> >>> Reviewed-by: Frank Li <frank.li@nxp.com>
> >>> ---
> >>>    .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 42
> +++++++++++++++++++
> >>>    1 file changed, 42 insertions(+)
> >>>
> >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> >>> b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> >>> index 53ee5a42c071..2e4173d099f3 100644
> >>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> >>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> >>> @@ -32,6 +32,7 @@
> >>>    #define GPR_ENET_QOS_RGMII_EN               (0x1 << 21)
> >>>
> >>>    #define MX93_GPR_ENET_QOS_INTF_MODE_MASK    GENMASK(3, 0)
> >>> +#define MX93_GPR_ENET_QOS_INTF_MASK          GENMASK(3, 1)
> >>>    #define MX93_GPR_ENET_QOS_INTF_SEL_MII              (0x0 << 1)
> >>>    #define MX93_GPR_ENET_QOS_INTF_SEL_RMII             (0x4 << 1)
> >>>    #define MX93_GPR_ENET_QOS_INTF_SEL_RGMII    (0x1 << 1)
> >>> @@ -40,6 +41,7 @@
> >>>    #define DMA_BUS_MODE                        0x00001000
> >>>    #define DMA_BUS_MODE_SFT_RESET              (0x1 << 0)
> >>>    #define RMII_RESET_SPEED            (0x3 << 14)
> >>> +#define CTRL_SPEED_MASK                      GENMASK(15, 14)
> >>>
> >>>    struct imx_dwmac_ops {
> >>>        u32 addr_width;
> >>> @@ -56,6 +58,7 @@ struct imx_priv_data {
> >>>        struct regmap *intf_regmap;
> >>>        u32 intf_reg_off;
> >>>        bool rmii_refclk_ext;
> >>> +     void __iomem *base_addr;
> >>>
> >>>        const struct imx_dwmac_ops *ops;
> >>>        struct plat_stmmacenet_data *plat_dat; @@ -212,6 +215,42 @@
> >>> static void imx_dwmac_fix_speed(void *priv, uint speed, uint mode)
> >>>                dev_err(dwmac->dev, "failed to set tx rate %lu\n", rate);
> >>>    }
> >>>
> >>> +static void imx_dwmac_fix_speed_mx93(void *priv, uint speed, uint
> >>> +mode) {
> >>> +     struct imx_priv_data *dwmac = priv;
> >>> +     int ctrl, old_ctrl, iface;
> >>> +
> >>> +     imx_dwmac_fix_speed(priv, speed, mode);
> >>> +
> >>> +     if (!dwmac || mode != MLO_AN_FIXED)
> >>> +             return;
> >>> +
> >>> +     if (regmap_read(dwmac->intf_regmap, dwmac->intf_reg_off, &iface))
> >>> +             return;
> >>> +
> >>> +     iface &= MX93_GPR_ENET_QOS_INTF_MASK;
> >>> +     if (iface != MX93_GPR_ENET_QOS_INTF_SEL_RGMII)
> >>> +             return;
> >>> +
> >>> +     old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
> >>> +     ctrl = old_ctrl & ~CTRL_SPEED_MASK;
> >>> +     regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
> >>> +                        MX93_GPR_ENET_QOS_INTF_MODE_MASK, 0);
> >>> +     writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);
> >>> +
> >>> +     /* Ensure the settings for CTRL are applied and avoid CPU/Compiler
> >>> +      * reordering.
> >>> +      */
> >>> +     wmb();
> >>> +
> >>> +     usleep_range(10, 20);
> >>> +     iface |= MX93_GPR_ENET_QOS_CLK_GEN_EN;
> >>> +     regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
> >>> +                        MX93_GPR_ENET_QOS_INTF_MODE_MASK, iface);
> >>> +
> >>> +     writel(old_ctrl, dwmac->base_addr + MAC_CTRL_REG); }
> >>> +
> >>>    static int imx_dwmac_mx93_reset(void *priv, void __iomem *ioaddr)
> >>>    {
> >>>        struct plat_stmmacenet_data *plat_dat = priv; @@ -317,8
> >>> +356,11 @@ static int imx_dwmac_probe(struct platform_device *pdev)
> >>>        plat_dat->exit = imx_dwmac_exit;
> >>>        plat_dat->clks_config = imx_dwmac_clks_config;
> >>>        plat_dat->fix_mac_speed = imx_dwmac_fix_speed;
> >>> +     if (of_machine_is_compatible("fsl,imx93"))
> >>> +             plat_dat->fix_mac_speed = imx_dwmac_fix_speed_mx93;
> >>>        plat_dat->bsp_priv = dwmac;
> >>>        dwmac->plat_dat = plat_dat;
> >>> +     dwmac->base_addr = stmmac_res.addr;
> >>>
> >>>        ret = imx_dwmac_clks_config(dwmac, true);
> >>>        if (ret)
> >>
> >> --
> >> Pengutronix e.K.                | Johannes Zink                  |
> >> Steuerwalder Str. 21            |
> >> https://www/
> >> .pe%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7Ccfd142f0d60a461
> ee01408
> >>
> db9321578d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63826554
> 36335
> >>
> 61986%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
> zIiLCJ
> >>
> BTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=CV10o1M%2BOj
> DPOaH5C
> >> y%2Fka%2B0aOMs0IaVapMH7aa3RnTI%3D&reserved=0
> >>
> ngutronix.de%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7C761fbb75c
> >>
> 1c24cfe091508db928d8ade%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
> >>
> 0%7C638264908852977732%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
> >>
> wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%
> >>
> 7C&sdata=2l2zNfIaNnRJENmERehNae8g%2F%2BQqlxD2YRx7ksY2X%2BE%3D&r
> >> eserved=0    |
> >> 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
> >> Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |
> >
> >
>
> --
> Pengutronix e.K.                | Johannes Zink                  |
> Steuerwalder Str. 21            |
> https://www.pe/
> ngutronix.de%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7Ccfd142f0d
> 60a461ee01408db9321578d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
> C0%7C638265543633561986%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj
> AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C
> %7C&sdata=yKzNPsHqD%2FxU%2FRmzLn4JSQjmuT9tU8SabLxHyGTTmms%3D&r
> eserved=0    |
> 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
> Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |


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

Re: [EXT] Re: [PATCH v3 net 2/2] net: stmmac: dwmac-imx: pause the TXC clock in fixed-link

[Johannes Zink]

Hi Shenwei,

On 8/2/23 16:27, Shenwei Wang wrote:
> 
> 
>> -----Original Message-----
>> From: Johannes Zink <j.zink@pengutronix.de>
>> Sent: Wednesday, August 2, 2023 1:26 AM
>> To: Shenwei Wang <shenwei.wang@nxp.com>; Russell King
>> <linux@armlinux.org.uk>; David S. Miller <davem@davemloft.net>; Eric
>> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
>> Abeni <pabeni@redhat.com>; Maxime Coquelin
>> <mcoquelin.stm32@gmail.com>; Shawn Guo <shawnguo@kernel.org>; Sascha
>> Hauer <s.hauer@pengutronix.de>; Neil Armstrong <neil.armstrong@linaro.org>;
>> Kevin Hilman <khilman@baylibre.com>; Vinod Koul <vkoul@kernel.org>; Chen-
>> Yu Tsai <wens@csie.org>; Jernej Skrabec <jernej.skrabec@gmail.com>; Samuel
>> Holland <samuel@sholland.org>
>> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
>> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
>> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
>> <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; Jerome Brunet
>> <jbrunet@baylibre.com>; Martin Blumenstingl
>> <martin.blumenstingl@googlemail.com>; Bhupesh Sharma
>> <bhupesh.sharma@linaro.org>; Nobuhiro Iwamatsu
>> <nobuhiro1.iwamatsu@toshiba.co.jp>; Simon Horman
>> <simon.horman@corigine.com>; Andrew Halaney <ahalaney@redhat.com>;
>> Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Wong Vee Khee
>> <veekhee@apple.com>; Revanth Kumar Uppala <ruppala@nvidia.com>; Jochen
>> Henneberg <jh@henneberg-systemdesign.com>; netdev@vger.kernel.org; linux-
>> stm32@st-md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org;
>> linux-kernel@vger.kernel.org; linux-amlogic@lists.infradead.org;
>> imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
>> Subject: Re: [EXT] Re: [PATCH v3 net 2/2] net: stmmac: dwmac-imx: pause the
>> TXC clock in fixed-link
>>
>> Caution: This is an external email. Please take care when clicking links or
>> opening attachments. When in doubt, report the message using the 'Report this
>> email' button
>>
>>
>> Hi Shenwei,
>>
>> On 8/1/23 19:10, Shenwei Wang wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Johannes Zink <j.zink@pengutronix.de>
>>>> Sent: Tuesday, August 1, 2023 7:48 AM
>>>> To: Shenwei Wang <shenwei.wang@nxp.com>; Russell King
>>>> <linux@armlinux.org.uk>; David S. Miller <davem@davemloft.net>; Eric
>>>> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>;
>>>> Paolo Abeni <pabeni@redhat.com>; Maxime Coquelin
>>>> <mcoquelin.stm32@gmail.com>; Shawn Guo <shawnguo@kernel.org>;
>> Sascha
>>>> Hauer <s.hauer@pengutronix.de>; Neil Armstrong
>>>> <neil.armstrong@linaro.org>; Kevin Hilman <khilman@baylibre.com>;
>>>> Vinod Koul <vkoul@kernel.org>; Chen- Yu Tsai <wens@csie.org>; Jernej
>>>> Skrabec <jernej.skrabec@gmail.com>; Samuel Holland
>>>> <samuel@sholland.org>
>>>> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
>>>> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
>>>> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
>>>> <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; Jerome Brunet
>>>> <jbrunet@baylibre.com>; Martin Blumenstingl
>>>> <martin.blumenstingl@googlemail.com>; Bhupesh Sharma
>>>> <bhupesh.sharma@linaro.org>; Nobuhiro Iwamatsu
>>>> <nobuhiro1.iwamatsu@toshiba.co.jp>; Simon Horman
>>>> <simon.horman@corigine.com>; Andrew Halaney <ahalaney@redhat.com>;
>>>> Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Wong Vee Khee
>>>> <veekhee@apple.com>; Revanth Kumar Uppala <ruppala@nvidia.com>;
>>>> Jochen Henneberg <jh@henneberg-systemdesign.com>;
>>>> netdev@vger.kernel.org; linux- stm32@st-md-mailman.stormreply.com;
>>>> linux-arm-kernel@lists.infradead.org;
>>>> linux-kernel@vger.kernel.org; linux-amlogic@lists.infradead.org;
>>>> imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
>>>> Subject: [EXT] Re: [PATCH v3 net 2/2] net: stmmac: dwmac-imx: pause
>>>> the TXC clock in fixed-link
>>>>
>>>> Caution: This is an external email. Please take care when clicking
>>>> links or opening attachments. When in doubt, report the message using
>>>> the 'Report this email' button
>>>>
>>>>
>>>> Hi Shenwei,
>>>>
>>>> thanks for your patch.
>>>>
>>>> On 7/31/23 18:19, Shenwei Wang wrote:
>>>>> When using a fixed-link setup, certain devices like the SJA1105
>>>>> require a small pause in the TXC clock line to enable their internal
>>>>> tunable delay line (TDL).
>>>>
>>>> If this is only required for some devices, is it safe to enforce this
>>>> behaviour unconditionally for any kind of fixed link devices
>>>> connected to the MX93 EQOS or could this possibly break for other devices?
>>>>
>>>
>>> It won't impact normal devices. The link layer hasn't built up yet.
>>>
>>
>> As Russel suggested in [1] - maybe you could rephrase your commit message for
>> your v4 to point this out to future reviewers (apparently multiple people have
>> had questions about this...)  and have this fact also recorded in the git log later
>> on.
>>
> 
> Okay.
> 
>> Also: does this only apply to i.MX93, or would we have to test and enable it on
>> e.g. i.MX8MP as well?
>>
> 
> Yes, it is required when the EQOS MAC is selected. However, this patch just enables
> The feature on i.MX93.

If this behaviour is required on all EQOS, I think the name 
imx_dwmac_fix_speed_mx93() is misleading. It should either be 
imx_dwmac_fix_speed() if applicable to all imx implementations, or 
dwmac_fix_speed() (and moved to a non-gluecode file) if applicable for all 
implementations in general.

You can then add a second patch for enabling it for the i.mx93 in the gluecode 
driver.

Johannes


> 
> Thanks,
> Shenwei
> 
>> Thanks
>> Johannes
>>
>> [1] ZMk/xqRP67zXHNrf@shell.armlinux.org.uk
>>
>>
>>> Thanks,
>>> Shenwei
>>>
>>>> Best regards
>>>> Johannes
>>>>
>>>>>
>>>>> To satisfy this requirement, this patch temporarily disables the TX
>>>>> clock, and restarts it after a required period. This provides the
>>>>> required silent interval on the clock line for SJA1105 to complete
>>>>> the frequency transition and enable the internal TDLs.
>>>>>
>>>>> So far we have only enabled this feature on the i.MX93 platform.
>>>>>
>>>>> Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
>>>>> Reviewed-by: Frank Li <frank.li@nxp.com>
>>>>> ---
>>>>>     .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 42
>> +++++++++++++++++++
>>>>>     1 file changed, 42 insertions(+)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
>>>>> b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
>>>>> index 53ee5a42c071..2e4173d099f3 100644
>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
>>>>> @@ -32,6 +32,7 @@
>>>>>     #define GPR_ENET_QOS_RGMII_EN               (0x1 << 21)
>>>>>
>>>>>     #define MX93_GPR_ENET_QOS_INTF_MODE_MASK    GENMASK(3, 0)
>>>>> +#define MX93_GPR_ENET_QOS_INTF_MASK          GENMASK(3, 1)
>>>>>     #define MX93_GPR_ENET_QOS_INTF_SEL_MII              (0x0 << 1)
>>>>>     #define MX93_GPR_ENET_QOS_INTF_SEL_RMII             (0x4 << 1)
>>>>>     #define MX93_GPR_ENET_QOS_INTF_SEL_RGMII    (0x1 << 1)
>>>>> @@ -40,6 +41,7 @@
>>>>>     #define DMA_BUS_MODE                        0x00001000
>>>>>     #define DMA_BUS_MODE_SFT_RESET              (0x1 << 0)
>>>>>     #define RMII_RESET_SPEED            (0x3 << 14)
>>>>> +#define CTRL_SPEED_MASK                      GENMASK(15, 14)
>>>>>
>>>>>     struct imx_dwmac_ops {
>>>>>         u32 addr_width;
>>>>> @@ -56,6 +58,7 @@ struct imx_priv_data {
>>>>>         struct regmap *intf_regmap;
>>>>>         u32 intf_reg_off;
>>>>>         bool rmii_refclk_ext;
>>>>> +     void __iomem *base_addr;
>>>>>
>>>>>         const struct imx_dwmac_ops *ops;
>>>>>         struct plat_stmmacenet_data *plat_dat; @@ -212,6 +215,42 @@
>>>>> static void imx_dwmac_fix_speed(void *priv, uint speed, uint mode)
>>>>>                 dev_err(dwmac->dev, "failed to set tx rate %lu\n", rate);
>>>>>     }
>>>>>
>>>>> +static void imx_dwmac_fix_speed_mx93(void *priv, uint speed, uint
>>>>> +mode) {
>>>>> +     struct imx_priv_data *dwmac = priv;
>>>>> +     int ctrl, old_ctrl, iface;
>>>>> +
>>>>> +     imx_dwmac_fix_speed(priv, speed, mode);
>>>>> +
>>>>> +     if (!dwmac || mode != MLO_AN_FIXED)
>>>>> +             return;
>>>>> +
>>>>> +     if (regmap_read(dwmac->intf_regmap, dwmac->intf_reg_off, &iface))
>>>>> +             return;
>>>>> +
>>>>> +     iface &= MX93_GPR_ENET_QOS_INTF_MASK;
>>>>> +     if (iface != MX93_GPR_ENET_QOS_INTF_SEL_RGMII)
>>>>> +             return;
>>>>> +
>>>>> +     old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
>>>>> +     ctrl = old_ctrl & ~CTRL_SPEED_MASK;
>>>>> +     regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
>>>>> +                        MX93_GPR_ENET_QOS_INTF_MODE_MASK, 0);
>>>>> +     writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);
>>>>> +
>>>>> +     /* Ensure the settings for CTRL are applied and avoid CPU/Compiler
>>>>> +      * reordering.
>>>>> +      */
>>>>> +     wmb();
>>>>> +
>>>>> +     usleep_range(10, 20);
>>>>> +     iface |= MX93_GPR_ENET_QOS_CLK_GEN_EN;
>>>>> +     regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
>>>>> +                        MX93_GPR_ENET_QOS_INTF_MODE_MASK, iface);
>>>>> +
>>>>> +     writel(old_ctrl, dwmac->base_addr + MAC_CTRL_REG); }
>>>>> +
>>>>>     static int imx_dwmac_mx93_reset(void *priv, void __iomem *ioaddr)
>>>>>     {
>>>>>         struct plat_stmmacenet_data *plat_dat = priv; @@ -317,8
>>>>> +356,11 @@ static int imx_dwmac_probe(struct platform_device *pdev)
>>>>>         plat_dat->exit = imx_dwmac_exit;
>>>>>         plat_dat->clks_config = imx_dwmac_clks_config;
>>>>>         plat_dat->fix_mac_speed = imx_dwmac_fix_speed;
>>>>> +     if (of_machine_is_compatible("fsl,imx93"))
>>>>> +             plat_dat->fix_mac_speed = imx_dwmac_fix_speed_mx93;
>>>>>         plat_dat->bsp_priv = dwmac;
>>>>>         dwmac->plat_dat = plat_dat;
>>>>> +     dwmac->base_addr = stmmac_res.addr;
>>>>>
>>>>>         ret = imx_dwmac_clks_config(dwmac, true);
>>>>>         if (ret)
>>>>
>>>> --
>>>> Pengutronix e.K.                | Johannes Zink                  |
>>>> Steuerwalder Str. 21            |
>>>> https://www/
>>>> .pe%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7Ccfd142f0d60a461
>> ee01408
>>>>
>> db9321578d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63826554
>> 36335
>>>>
>> 61986%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
>> zIiLCJ
>>>>
>> BTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=CV10o1M%2BOj
>> DPOaH5C
>>>> y%2Fka%2B0aOMs0IaVapMH7aa3RnTI%3D&reserved=0
>>>>
>> ngutronix.de%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7C761fbb75c
>>>>
>> 1c24cfe091508db928d8ade%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
>>>>
>> 0%7C638264908852977732%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
>>>>
>> wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%
>>>>
>> 7C&sdata=2l2zNfIaNnRJENmERehNae8g%2F%2BQqlxD2YRx7ksY2X%2BE%3D&r
>>>> eserved=0    |
>>>> 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
>>>> Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |
>>>
>>>
>>
>> --
>> Pengutronix e.K.                | Johannes Zink                  |
>> Steuerwalder Str. 21            |
>> https://www.pe/
>> ngutronix.de%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7Ccfd142f0d
>> 60a461ee01408db9321578d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
>> C0%7C638265543633561986%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj
>> AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C
>> %7C&sdata=yKzNPsHqD%2FxU%2FRmzLn4JSQjmuT9tU8SabLxHyGTTmms%3D&r
>> eserved=0    |
>> 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
>> Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |
> 
> 

-- 
Pengutronix e.K.                | Johannes Zink                  |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |


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

RE: [EXT] Re: [PATCH v3 net 2/2] net: stmmac: dwmac-imx: pause the TXC clock in fixed-link

[Shenwei Wang]

> -----Original Message-----
> From: Johannes Zink <j.zink@pengutronix.de>
> Sent: Wednesday, August 2, 2023 9:40 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>; Russell King
> <linux@armlinux.org.uk>; David S. Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> Abeni <pabeni@redhat.com>; Maxime Coquelin
> <mcoquelin.stm32@gmail.com>; Shawn Guo <shawnguo@kernel.org>; Sascha
> Hauer <s.hauer@pengutronix.de>; Neil Armstrong <neil.armstrong@linaro.org>;
> Kevin Hilman <khilman@baylibre.com>; Vinod Koul <vkoul@kernel.org>; Chen-
> Yu Tsai <wens@csie.org>; Jernej Skrabec <jernej.skrabec@gmail.com>; Samuel
> Holland <samuel@sholland.org>
> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; Jerome Brunet
> <jbrunet@baylibre.com>; Martin Blumenstingl
> <martin.blumenstingl@googlemail.com>; Bhupesh Sharma
> <bhupesh.sharma@linaro.org>; Nobuhiro Iwamatsu
> <nobuhiro1.iwamatsu@toshiba.co.jp>; Simon Horman
> <simon.horman@corigine.com>; Andrew Halaney <ahalaney@redhat.com>;
> Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Wong Vee Khee
> <veekhee@apple.com>; Revanth Kumar Uppala <ruppala@nvidia.com>; Jochen
> Henneberg <jh@henneberg-systemdesign.com>; netdev@vger.kernel.org; linux-
> stm32@st-md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; linux-amlogic@lists.infradead.org;
> imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
> Subject: Re: [EXT] Re: [PATCH v3 net 2/2] net: stmmac: dwmac-imx: pause the
> TXC clock in fixed-link
>
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report this
> email' button
>
>
> Hi Shenwei,
>
> On 8/2/23 16:27, Shenwei Wang wrote:
> >
> >
> >> -----Original Message-----
> >> From: Johannes Zink <j.zink@pengutronix.de>
> >> Sent: Wednesday, August 2, 2023 1:26 AM
> >> To: Shenwei Wang <shenwei.wang@nxp.com>; Russell King
> >> <linux@armlinux.org.uk>; David S. Miller <davem@davemloft.net>; Eric
> >> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>;
> >> Paolo Abeni <pabeni@redhat.com>; Maxime Coquelin
> >> <mcoquelin.stm32@gmail.com>; Shawn Guo <shawnguo@kernel.org>;
> Sascha
> >> Hauer <s.hauer@pengutronix.de>; Neil Armstrong
> >> <neil.armstrong@linaro.org>; Kevin Hilman <khilman@baylibre.com>;
> >> Vinod Koul <vkoul@kernel.org>; Chen- Yu Tsai <wens@csie.org>; Jernej
> >> Skrabec <jernej.skrabec@gmail.com>; Samuel Holland
> >> <samuel@sholland.org>
> >> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> >> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> >> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> >> <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; Jerome Brunet
> >> <jbrunet@baylibre.com>; Martin Blumenstingl
> >> <martin.blumenstingl@googlemail.com>; Bhupesh Sharma
> >> <bhupesh.sharma@linaro.org>; Nobuhiro Iwamatsu
> >> <nobuhiro1.iwamatsu@toshiba.co.jp>; Simon Horman
> >> <simon.horman@corigine.com>; Andrew Halaney <ahalaney@redhat.com>;
> >> Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Wong Vee Khee
> >> <veekhee@apple.com>; Revanth Kumar Uppala <ruppala@nvidia.com>;
> >> Jochen Henneberg <jh@henneberg-systemdesign.com>;
> >> netdev@vger.kernel.org; linux- stm32@st-md-mailman.stormreply.com;
> >> linux-arm-kernel@lists.infradead.org;
> >> linux-kernel@vger.kernel.org; linux-amlogic@lists.infradead.org;
> >> imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
> >> Subject: Re: [EXT] Re: [PATCH v3 net 2/2] net: stmmac: dwmac-imx:
> >> pause the TXC clock in fixed-link
> >>
> >> Caution: This is an external email. Please take care when clicking
> >> links or opening attachments. When in doubt, report the message using
> >> the 'Report this email' button
> >>
> >>
> >> Hi Shenwei,
> >>
> >> On 8/1/23 19:10, Shenwei Wang wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Johannes Zink <j.zink@pengutronix.de>
> >>>> Sent: Tuesday, August 1, 2023 7:48 AM
> >>>> To: Shenwei Wang <shenwei.wang@nxp.com>; Russell King
> >>>> <linux@armlinux.org.uk>; David S. Miller <davem@davemloft.net>;
> >>>> Eric Dumazet <edumazet@google.com>; Jakub Kicinski
> >>>> <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Maxime Coquelin
> >>>> <mcoquelin.stm32@gmail.com>; Shawn Guo <shawnguo@kernel.org>;
> >> Sascha
> >>>> Hauer <s.hauer@pengutronix.de>; Neil Armstrong
> >>>> <neil.armstrong@linaro.org>; Kevin Hilman <khilman@baylibre.com>;
> >>>> Vinod Koul <vkoul@kernel.org>; Chen- Yu Tsai <wens@csie.org>;
> >>>> Jernej Skrabec <jernej.skrabec@gmail.com>; Samuel Holland
> >>>> <samuel@sholland.org>
> >>>> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> >>>> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> >>>> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> >>>> <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; Jerome
> >>>> Brunet <jbrunet@baylibre.com>; Martin Blumenstingl
> >>>> <martin.blumenstingl@googlemail.com>; Bhupesh Sharma
> >>>> <bhupesh.sharma@linaro.org>; Nobuhiro Iwamatsu
> >>>> <nobuhiro1.iwamatsu@toshiba.co.jp>; Simon Horman
> >>>> <simon.horman@corigine.com>; Andrew Halaney
> <ahalaney@redhat.com>;
> >>>> Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Wong Vee Khee
> >>>> <veekhee@apple.com>; Revanth Kumar Uppala <ruppala@nvidia.com>;
> >>>> Jochen Henneberg <jh@henneberg-systemdesign.com>;
> >>>> netdev@vger.kernel.org; linux- stm32@st-md-mailman.stormreply.com;
> >>>> linux-arm-kernel@lists.infradead.org;
> >>>> linux-kernel@vger.kernel.org; linux-amlogic@lists.infradead.org;
> >>>> imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
> >>>> Subject: [EXT] Re: [PATCH v3 net 2/2] net: stmmac: dwmac-imx: pause
> >>>> the TXC clock in fixed-link
> >>>>
> >>>> Caution: This is an external email. Please take care when clicking
> >>>> links or opening attachments. When in doubt, report the message
> >>>> using the 'Report this email' button
> >>>>
> >>>>
> >>>> Hi Shenwei,
> >>>>
> >>>> thanks for your patch.
> >>>>
> >>>> On 7/31/23 18:19, Shenwei Wang wrote:
> >>>>> When using a fixed-link setup, certain devices like the SJA1105
> >>>>> require a small pause in the TXC clock line to enable their
> >>>>> internal tunable delay line (TDL).
> >>>>
> >>>> If this is only required for some devices, is it safe to enforce
> >>>> this behaviour unconditionally for any kind of fixed link devices
> >>>> connected to the MX93 EQOS or could this possibly break for other devices?
> >>>>
> >>>
> >>> It won't impact normal devices. The link layer hasn't built up yet.
> >>>
> >>
> >> As Russel suggested in [1] - maybe you could rephrase your commit
> >> message for your v4 to point this out to future reviewers (apparently
> >> multiple people have had questions about this...)  and have this fact
> >> also recorded in the git log later on.
> >>
> >
> > Okay.
> >
> >> Also: does this only apply to i.MX93, or would we have to test and
> >> enable it on e.g. i.MX8MP as well?
> >>
> >
> > Yes, it is required when the EQOS MAC is selected. However, this patch
> > just enables The feature on i.MX93.
>
> If this behaviour is required on all EQOS, I think the name
> imx_dwmac_fix_speed_mx93() is misleading. It should either be
> imx_dwmac_fix_speed() if applicable to all imx implementations, or
> dwmac_fix_speed() (and moved to a non-gluecode file) if applicable for all
> implementations in general.
>

It has the general fix_speed function there named imx_dwmac_fix_speed.
This one is the special for this mx93 fix.

Thanks,
Shenwei


> You can then add a second patch for enabling it for the i.mx93 in the gluecode
> driver.
>
> Johannes
>
>
> >
> > Thanks,
> > Shenwei
> >
> >> Thanks
> >> Johannes
> >>
> >> [1] ZMk/xqRP67zXHNrf@shell.armlinux.org.uk
> >>
> >>
> >>> Thanks,
> >>> Shenwei
> >>>
> >>>> Best regards
> >>>> Johannes
> >>>>
> >>>>>
> >>>>> To satisfy this requirement, this patch temporarily disables the
> >>>>> TX clock, and restarts it after a required period. This provides
> >>>>> the required silent interval on the clock line for SJA1105 to
> >>>>> complete the frequency transition and enable the internal TDLs.
> >>>>>
> >>>>> So far we have only enabled this feature on the i.MX93 platform.
> >>>>>
> >>>>> Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> >>>>> Reviewed-by: Frank Li <frank.li@nxp.com>
> >>>>> ---
> >>>>>     .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 42
> >> +++++++++++++++++++
> >>>>>     1 file changed, 42 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> >>>>> b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> >>>>> index 53ee5a42c071..2e4173d099f3 100644
> >>>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> >>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> >>>>> @@ -32,6 +32,7 @@
> >>>>>     #define GPR_ENET_QOS_RGMII_EN               (0x1 << 21)
> >>>>>
> >>>>>     #define MX93_GPR_ENET_QOS_INTF_MODE_MASK    GENMASK(3, 0)
> >>>>> +#define MX93_GPR_ENET_QOS_INTF_MASK          GENMASK(3, 1)
> >>>>>     #define MX93_GPR_ENET_QOS_INTF_SEL_MII              (0x0 << 1)
> >>>>>     #define MX93_GPR_ENET_QOS_INTF_SEL_RMII             (0x4 << 1)
> >>>>>     #define MX93_GPR_ENET_QOS_INTF_SEL_RGMII    (0x1 << 1)
> >>>>> @@ -40,6 +41,7 @@
> >>>>>     #define DMA_BUS_MODE                        0x00001000
> >>>>>     #define DMA_BUS_MODE_SFT_RESET              (0x1 << 0)
> >>>>>     #define RMII_RESET_SPEED            (0x3 << 14)
> >>>>> +#define CTRL_SPEED_MASK                      GENMASK(15, 14)
> >>>>>
> >>>>>     struct imx_dwmac_ops {
> >>>>>         u32 addr_width;
> >>>>> @@ -56,6 +58,7 @@ struct imx_priv_data {
> >>>>>         struct regmap *intf_regmap;
> >>>>>         u32 intf_reg_off;
> >>>>>         bool rmii_refclk_ext;
> >>>>> +     void __iomem *base_addr;
> >>>>>
> >>>>>         const struct imx_dwmac_ops *ops;
> >>>>>         struct plat_stmmacenet_data *plat_dat; @@ -212,6 +215,42
> >>>>> @@ static void imx_dwmac_fix_speed(void *priv, uint speed, uint mode)
> >>>>>                 dev_err(dwmac->dev, "failed to set tx rate %lu\n", rate);
> >>>>>     }
> >>>>>
> >>>>> +static void imx_dwmac_fix_speed_mx93(void *priv, uint speed, uint
> >>>>> +mode) {
> >>>>> +     struct imx_priv_data *dwmac = priv;
> >>>>> +     int ctrl, old_ctrl, iface;
> >>>>> +
> >>>>> +     imx_dwmac_fix_speed(priv, speed, mode);
> >>>>> +
> >>>>> +     if (!dwmac || mode != MLO_AN_FIXED)
> >>>>> +             return;
> >>>>> +
> >>>>> +     if (regmap_read(dwmac->intf_regmap, dwmac->intf_reg_off, &iface))
> >>>>> +             return;
> >>>>> +
> >>>>> +     iface &= MX93_GPR_ENET_QOS_INTF_MASK;
> >>>>> +     if (iface != MX93_GPR_ENET_QOS_INTF_SEL_RGMII)
> >>>>> +             return;
> >>>>> +
> >>>>> +     old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
> >>>>> +     ctrl = old_ctrl & ~CTRL_SPEED_MASK;
> >>>>> +     regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
> >>>>> +                        MX93_GPR_ENET_QOS_INTF_MODE_MASK, 0);
> >>>>> +     writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);
> >>>>> +
> >>>>> +     /* Ensure the settings for CTRL are applied and avoid CPU/Compiler
> >>>>> +      * reordering.
> >>>>> +      */
> >>>>> +     wmb();
> >>>>> +
> >>>>> +     usleep_range(10, 20);
> >>>>> +     iface |= MX93_GPR_ENET_QOS_CLK_GEN_EN;
> >>>>> +     regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
> >>>>> +                        MX93_GPR_ENET_QOS_INTF_MODE_MASK, iface);
> >>>>> +
> >>>>> +     writel(old_ctrl, dwmac->base_addr + MAC_CTRL_REG); }
> >>>>> +
> >>>>>     static int imx_dwmac_mx93_reset(void *priv, void __iomem *ioaddr)
> >>>>>     {
> >>>>>         struct plat_stmmacenet_data *plat_dat = priv; @@ -317,8
> >>>>> +356,11 @@ static int imx_dwmac_probe(struct platform_device
> >>>>> +*pdev)
> >>>>>         plat_dat->exit = imx_dwmac_exit;
> >>>>>         plat_dat->clks_config = imx_dwmac_clks_config;
> >>>>>         plat_dat->fix_mac_speed = imx_dwmac_fix_speed;
> >>>>> +     if (of_machine_is_compatible("fsl,imx93"))
> >>>>> +             plat_dat->fix_mac_speed = imx_dwmac_fix_speed_mx93;
> >>>>>         plat_dat->bsp_priv = dwmac;
> >>>>>         dwmac->plat_dat = plat_dat;
> >>>>> +     dwmac->base_addr = stmmac_res.addr;
> >>>>>
> >>>>>         ret = imx_dwmac_clks_config(dwmac, true);
> >>>>>         if (ret)
> >>>>
> >>>> --
> >>>> Pengutronix e.K.                | Johannes Zink                  |
> >>>> Steuerwalder Str. 21            |
> >>>> https://www/
> >>>> .pe%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7Ccfd142f0d60a4
> 61
> >> ee01408
> >>>>
> >>
> db9321578d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63826554
> >> 36335
> >>>>
> >>
> 61986%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
> >> zIiLCJ
> >>>>
> >>
> BTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=CV10o1M%2BOj
> >> DPOaH5C
> >>>> y%2Fka%2B0aOMs0IaVapMH7aa3RnTI%3D&reserved=0
> >>>>
> >>
> ngutronix.de%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7C761fbb75c
> >>>>
> >>
> 1c24cfe091508db928d8ade%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
> >>>>
> >>
> 0%7C638264908852977732%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
> >>>>
> >>
> wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%
> >>>>
> >>
> 7C&sdata=2l2zNfIaNnRJENmERehNae8g%2F%2BQqlxD2YRx7ksY2X%2BE%3D&r
> >>>> eserved=0    |
> >>>> 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
> >>>> Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |
> >>>
> >>>
> >>
> >> --
> >> Pengutronix e.K.                | Johannes Zink                  |
> >> Steuerwalder Str. 21            |
> >> https://www/
> >> .pe%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7Cdc64404f8c2c4e
> b87a7808
> >>
> db93666ec9%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63826584
> 03801
> >>
> 74614%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
> zIiLCJ
> >>
> BTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=oxLnb3ppqjhMti
> cQH7P
> >> lfRbIlYJ2R1Z8Tg7Bz2vC%2F%2Bc%3D&reserved=0
> >>
> ngutronix.de%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7Ccfd142f0d
> >>
> 60a461ee01408db9321578d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
> >>
> C0%7C638265543633561986%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj
> >>
> AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C
> >> %7C&sdata=yKzNPsHqD%2FxU%2FRmzLn4JSQjmuT9tU8SabLxHyGTTmms%3
> D&r
> >> eserved=0    |
> >> 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
> >> Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |
> >
> >
>
> --
> Pengutronix e.K.                | Johannes Zink                  |
> Steuerwalder Str. 21            |
> https://www.pe/
> ngutronix.de%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7Cdc64404f8
> c2c4eb87a7808db93666ec9%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
> C0%7C638265840380174614%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj
> AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C
> %7C&sdata=r8tFe0Ts3ev2c7lg3MK0Qc40101d7W%2BEwnpmvMDwjho%3D&res
> erved=0    |
> 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
> Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |


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

Re: [EXT] Re: [PATCH v3 net 2/2] net: stmmac: dwmac-imx: pause the TXC clock in fixed-link

[Johannes Zink]

Hi Shenwei,

[snip]
>>>
>>>> Also: does this only apply to i.MX93, or would we have to test and
>>>> enable it on e.g. i.MX8MP as well?
>>>>
>>>
>>> Yes, it is required when the EQOS MAC is selected. However, this patch
>>> just enables The feature on i.MX93.
>>
>> If this behaviour is required on all EQOS, I think the name
>> imx_dwmac_fix_speed_mx93() is misleading. It should either be
>> imx_dwmac_fix_speed() if applicable to all imx implementations, or
>> dwmac_fix_speed() (and moved to a non-gluecode file) if applicable for all
>> implementations in general.
>>
> 
> It has the general fix_speed function there named imx_dwmac_fix_speed.
> This one is the special for this mx93 fix.

I think I might have misunderstood your last statement or I failed to express 
my point. If you need to replace the dwmac_fix_speed() on mx93, because this 
SoC implementation requires doing so (the usual reason for doing something like 
this is something like reset quirks because of screwed up IP Core integration), 
then your approach is imho valid.

But if I got your last comment right, your changes should apply to EQOS MAC in 
general (but you want to only enable it for mx93 at the moment). In this case 
this quirk will later be as the fix_mac_speed function for other hardware as 
well, in which case the name ..._mx93 is misleading, and imho rather a 
descriptive name should be used (i.e. have the name describe what it does 
rather than for what hardware it is implemented).

Except if the maintainers have a strong opinion that the ..._mx93 suffix 
version is exactly how you should proceed...

Best regards
Johannes

> 
> Thanks,
> Shenwei
> [snip]


-- 
Pengutronix e.K.                | Johannes Zink                  |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |


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

RE: [EXT] Re: [PATCH v3 net 2/2] net: stmmac: dwmac-imx: pause the TXC clock in fixed-link

[Shenwei Wang]

> -----Original Message-----
> From: Johannes Zink <j.zink@pengutronix.de>
> Sent: Thursday, August 3, 2023 1:36 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>; Russell King
> <linux@armlinux.org.uk>; David S. Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> Abeni <pabeni@redhat.com>; Maxime Coquelin
> <mcoquelin.stm32@gmail.com>; Shawn Guo <shawnguo@kernel.org>; Sascha
> Hauer <s.hauer@pengutronix.de>; Neil Armstrong <neil.armstrong@linaro.org>;
> Kevin Hilman <khilman@baylibre.com>; Vinod Koul <vkoul@kernel.org>; Chen-
> Yu Tsai <wens@csie.org>; Jernej Skrabec <jernej.skrabec@gmail.com>; Samuel
> Holland <samuel@sholland.org>
> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; Jerome Brunet
> <jbrunet@baylibre.com>; Martin Blumenstingl
> <martin.blumenstingl@googlemail.com>; Bhupesh Sharma
> <bhupesh.sharma@linaro.org>; Nobuhiro Iwamatsu
> <nobuhiro1.iwamatsu@toshiba.co.jp>; Simon Horman
> > It has the general fix_speed function there named imx_dwmac_fix_speed.
> > This one is the special for this mx93 fix.
>
> I think I might have misunderstood your last statement or I failed to express my
> point. If you need to replace the dwmac_fix_speed() on mx93, because this SoC
> implementation requires doing so (the usual reason for doing something like this
> is something like reset quirks because of screwed up IP Core integration), then
> your approach is imho valid.
>
> But if I got your last comment right, your changes should apply to EQOS MAC in
> general (but you want to only enable it for mx93 at the moment). In this case
> this quirk will later be as the fix_mac_speed function for other hardware as well,
> in which case the name ..._mx93 is misleading, and imho rather a descriptive
> name should be used (i.e. have the name describe what it does rather than for
> what hardware it is implemented).
>

The idea of supporting the SJA1105 is general, but the implementation depends on the
specific SoC. This patch provides the implementation for the MX93 SoC. To support the
MX8x SoCs, the implementation would need to be rewritten based on the current state
of the dwmac-imx driver.

I agree the function name is somewhat ugly. Maybe a better name could be:
mx93_dwmac_fix_speed()

The assumption is that the process of pausing the clock can still be considered part of fixing the speed.

Thanks,
Shenwei

> Except if the maintainers have a strong opinion that the ..._mx93 suffix version
> is exactly how you should proceed...
>
> Best regards
> Johannes
>
> >
> > Thanks,
> > Shenwei
> > [snip]
>
>
> --
> Pengutronix e.K.                | Johannes Zink                  |
> Steuerwalder Str. 21            |
> https://www.pe/
> ngutronix.de%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7Cd328d89d
> 14e847270d5a08db93ebff01%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
> C0%7C638266414027048795%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj
> AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C
> %7C&sdata=dkE9Es7kl3uNYx8zJYZt2r6933dSNtYDpQzCEagAV3M%3D&reserved
> =0    |
> 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
> Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |


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