[PATCH v2 net 0/2] update stmmac fix_mac_speed

[PATCH v2 net 0/2] update stmmac fix_mac_speed

[Shenwei Wang]

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

 .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 47 ++++++++++++++++++-
 .../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 +-
 .../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 +-
 10 files changed, 55 insertions(+), 10 deletions(-)

--
2.34.1

[PATCH v2 net 0/2] update stmmac fix_mac_speed

[Shenwei Wang]

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

 .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 47 ++++++++++++++++++-
 .../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 +-
 .../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 +-
 10 files changed, 55 insertions(+), 10 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 v2 net 0/2] update stmmac fix_mac_speed

[Shenwei Wang]

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

 .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 47 ++++++++++++++++++-
 .../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 +-
 .../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 +-
 10 files changed, 55 insertions(+), 10 deletions(-)

--
2.34.1


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

[PATCH v2 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-imx.c         | 2 +-
 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-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 +-
 10 files changed, 10 insertions(+), 10 deletions(-)

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

[PATCH v2 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-imx.c         | 2 +-
 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-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 +-
 10 files changed, 10 insertions(+), 10 deletions(-)

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

[PATCH v2 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-imx.c         | 2 +-
 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-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 +-
 10 files changed, 10 insertions(+), 10 deletions(-)

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-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-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-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v2 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   | 45 +++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
index 53ee5a42c071..e7819960128e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
@@ -40,6 +40,9 @@
 #define DMA_BUS_MODE			0x00001000
 #define DMA_BUS_MODE_SFT_RESET		(0x1 << 0)
 #define RMII_RESET_SPEED		(0x3 << 14)
+#define MII_RESET_SPEED			(0x2 << 14)
+#define RGMII_RESET_SPEED		(0x0 << 14)
+#define CTRL_SPEED_MASK			(0x3 << 14)
 
 struct imx_dwmac_ops {
 	u32 addr_width;
@@ -56,6 +59,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 +216,44 @@ 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_MODE_MASK;
+	old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
+	ctrl = old_ctrl & ~CTRL_SPEED_MASK;
+
+	/* by default ctrl will be RGMII */
+	if (iface == MX93_GPR_ENET_QOS_INTF_SEL_RMII)
+		ctrl |= RMII_RESET_SPEED;
+	if (iface == MX93_GPR_ENET_QOS_INTF_SEL_MII)
+		ctrl |= MII_RESET_SPEED;
+
+	writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);
+
+	/* Ensure the settings for CTRL are applied */
+	wmb();
+
+	regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
+			   MX93_GPR_ENET_QOS_INTF_MODE_MASK, 0);
+	usleep_range(50, 100);
+	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 +359,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

[PATCH v2 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   | 45 +++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
index 53ee5a42c071..e7819960128e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
@@ -40,6 +40,9 @@
 #define DMA_BUS_MODE			0x00001000
 #define DMA_BUS_MODE_SFT_RESET		(0x1 << 0)
 #define RMII_RESET_SPEED		(0x3 << 14)
+#define MII_RESET_SPEED			(0x2 << 14)
+#define RGMII_RESET_SPEED		(0x0 << 14)
+#define CTRL_SPEED_MASK			(0x3 << 14)
 
 struct imx_dwmac_ops {
 	u32 addr_width;
@@ -56,6 +59,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 +216,44 @@ 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_MODE_MASK;
+	old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
+	ctrl = old_ctrl & ~CTRL_SPEED_MASK;
+
+	/* by default ctrl will be RGMII */
+	if (iface == MX93_GPR_ENET_QOS_INTF_SEL_RMII)
+		ctrl |= RMII_RESET_SPEED;
+	if (iface == MX93_GPR_ENET_QOS_INTF_SEL_MII)
+		ctrl |= MII_RESET_SPEED;
+
+	writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);
+
+	/* Ensure the settings for CTRL are applied */
+	wmb();
+
+	regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
+			   MX93_GPR_ENET_QOS_INTF_MODE_MASK, 0);
+	usleep_range(50, 100);
+	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 +359,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

[PATCH v2 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   | 45 +++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
index 53ee5a42c071..e7819960128e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
@@ -40,6 +40,9 @@
 #define DMA_BUS_MODE			0x00001000
 #define DMA_BUS_MODE_SFT_RESET		(0x1 << 0)
 #define RMII_RESET_SPEED		(0x3 << 14)
+#define MII_RESET_SPEED			(0x2 << 14)
+#define RGMII_RESET_SPEED		(0x0 << 14)
+#define CTRL_SPEED_MASK			(0x3 << 14)
 
 struct imx_dwmac_ops {
 	u32 addr_width;
@@ -56,6 +59,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 +216,44 @@ 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_MODE_MASK;
+	old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
+	ctrl = old_ctrl & ~CTRL_SPEED_MASK;
+
+	/* by default ctrl will be RGMII */
+	if (iface == MX93_GPR_ENET_QOS_INTF_SEL_RMII)
+		ctrl |= RMII_RESET_SPEED;
+	if (iface == MX93_GPR_ENET_QOS_INTF_SEL_MII)
+		ctrl |= MII_RESET_SPEED;
+
+	writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);
+
+	/* Ensure the settings for CTRL are applied */
+	wmb();
+
+	regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
+			   MX93_GPR_ENET_QOS_INTF_MODE_MASK, 0);
+	usleep_range(50, 100);
+	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 +359,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-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Andrew Halaney]

On Thu, Jul 27, 2023 at 10:25:03AM -0500, 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.
> 

I'd just like to highlight that because of a quirk (I think this is not
standard) in the particular connected switch on a board you're making the
whole "fsl,imx93" platform (compatible) implement said switch quirk.

If you don't think there's any harm in doing that for other fixed-link
scenarios, that's fine I suppose... but just highlighting that.

I have no idea at a higher level how else you'd tackle this. You could
add a dt property for this, but I also don't love that you'd probably
encode it in the MAC (maybe in the fixed-link description it would be
more attractive). At least as a dt property it isn't unconditional.

> Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> Reviewed-by: Frank Li <frank.li@nxp.com>
> ---
>  .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 45 +++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> index 53ee5a42c071..e7819960128e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> @@ -40,6 +40,9 @@
>  #define DMA_BUS_MODE			0x00001000
>  #define DMA_BUS_MODE_SFT_RESET		(0x1 << 0)
>  #define RMII_RESET_SPEED		(0x3 << 14)
> +#define MII_RESET_SPEED			(0x2 << 14)
> +#define RGMII_RESET_SPEED		(0x0 << 14)
> +#define CTRL_SPEED_MASK			(0x3 << 14)

GENMASK() would be cleaner, as well as BIT() usage, but I do see
the driver currently does shifts.. so /me shrugs

>  
>  struct imx_dwmac_ops {
>  	u32 addr_width;
> @@ -56,6 +59,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 +216,44 @@ 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_MODE_MASK;
> +	old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
> +	ctrl = old_ctrl & ~CTRL_SPEED_MASK;
> +
> +	/* by default ctrl will be RGMII */
> +	if (iface == MX93_GPR_ENET_QOS_INTF_SEL_RMII)
> +		ctrl |= RMII_RESET_SPEED;
> +	if (iface == MX93_GPR_ENET_QOS_INTF_SEL_MII)
> +		ctrl |= MII_RESET_SPEED;

I see that ctrl right now would select RGMII, but I think it would
read more clearly if you handled it and made the above an
if/else if/else statement (since they're exclusive of eachother)
vs two independent if's.

> +
> +	writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);
> +
> +	/* Ensure the settings for CTRL are applied */
> +	wmb();

I saw this and recently have been wondering about this sort of pattern
(not an expert on this). From what I can tell it seems reading the
register back is the preferred pattern to force the write out. The above works,
but it feels to me personally akin to how local_lock()
in the kernel is a more fine grained mechanism than using
preempt_disable(). But that's pretty opinionated. See device-io.rst
and io_ordering.rst for how I came to that conclusion.

> +
> +	regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
> +			   MX93_GPR_ENET_QOS_INTF_MODE_MASK, 0);
> +	usleep_range(50, 100);
> +	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);
> +}

I don't have any documentation for the registers here, and as you can
see I'm an amateur with respect to memory ordering based on my prior
comment.

But you:

    1. Read intf_reg_off into variable iface
    2. Write the RESET_SPEED for the appropriate mode to MAC_CTRL_REG
    3. wmb() to ensure that write goes through
    4. Read intf_reg_off (regmap_update_bits())
    5. Write 0 to MX93_GPR_ENET_QOS_INTF_MODE_MASK within intf_reg_off (regmap_update_bits())
    6. Sleep for 50-100 us
    7. Read intf_reg_off (regmap_update_bits())
    8. Write MX93_GPR_ENET_QOS_CLK_GEN_EN | iface (from 1) to
       MX93_GPR_ENET_QOS_INTF_MODE_MASK within intf_reg_off (regmap_update_bits())

I don't know what those bits do, but your description sounds like you
are trying to stop the clock for 50-100 us. In your code, if I
understand the memory ordering correctly, both of the following could
occur:

    1. Write RESET_SPEED
    2. Write 0 to MX93_GPR_ENET_QOS_INTF_MODE_MASK
    3. sleep
    4. Restore MX93_GPR_ENET_QOS_CLK_GEN_EN | iface

    or

    1. Write RESET_SPEED
    2. sleep
    3. Write 0 to MX93_GPR_ENET_QOS_INTF_MODE_MASK
    4. Restore MX93_GPR_ENET_QOS_CLK_GEN_EN | iface

is the latter acceptable to you, or does that wmb() (or alternative)
need to move? It seems to me only the first situation would stop
the clock before sleeping, but that's going off the names in this driver
only.

In either case, shouldn't regmap_update_bits() force a
read of said bits, which would remove the need for that wmb() altogether
to synchronize the two writes?

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

[Andrew Halaney]

On Thu, Jul 27, 2023 at 10:25:03AM -0500, 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.
> 

I'd just like to highlight that because of a quirk (I think this is not
standard) in the particular connected switch on a board you're making the
whole "fsl,imx93" platform (compatible) implement said switch quirk.

If you don't think there's any harm in doing that for other fixed-link
scenarios, that's fine I suppose... but just highlighting that.

I have no idea at a higher level how else you'd tackle this. You could
add a dt property for this, but I also don't love that you'd probably
encode it in the MAC (maybe in the fixed-link description it would be
more attractive). At least as a dt property it isn't unconditional.

> Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> Reviewed-by: Frank Li <frank.li@nxp.com>
> ---
>  .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 45 +++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> index 53ee5a42c071..e7819960128e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> @@ -40,6 +40,9 @@
>  #define DMA_BUS_MODE			0x00001000
>  #define DMA_BUS_MODE_SFT_RESET		(0x1 << 0)
>  #define RMII_RESET_SPEED		(0x3 << 14)
> +#define MII_RESET_SPEED			(0x2 << 14)
> +#define RGMII_RESET_SPEED		(0x0 << 14)
> +#define CTRL_SPEED_MASK			(0x3 << 14)

GENMASK() would be cleaner, as well as BIT() usage, but I do see
the driver currently does shifts.. so /me shrugs

>  
>  struct imx_dwmac_ops {
>  	u32 addr_width;
> @@ -56,6 +59,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 +216,44 @@ 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_MODE_MASK;
> +	old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
> +	ctrl = old_ctrl & ~CTRL_SPEED_MASK;
> +
> +	/* by default ctrl will be RGMII */
> +	if (iface == MX93_GPR_ENET_QOS_INTF_SEL_RMII)
> +		ctrl |= RMII_RESET_SPEED;
> +	if (iface == MX93_GPR_ENET_QOS_INTF_SEL_MII)
> +		ctrl |= MII_RESET_SPEED;

I see that ctrl right now would select RGMII, but I think it would
read more clearly if you handled it and made the above an
if/else if/else statement (since they're exclusive of eachother)
vs two independent if's.

> +
> +	writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);
> +
> +	/* Ensure the settings for CTRL are applied */
> +	wmb();

I saw this and recently have been wondering about this sort of pattern
(not an expert on this). From what I can tell it seems reading the
register back is the preferred pattern to force the write out. The above works,
but it feels to me personally akin to how local_lock()
in the kernel is a more fine grained mechanism than using
preempt_disable(). But that's pretty opinionated. See device-io.rst
and io_ordering.rst for how I came to that conclusion.

> +
> +	regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
> +			   MX93_GPR_ENET_QOS_INTF_MODE_MASK, 0);
> +	usleep_range(50, 100);
> +	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);
> +}

I don't have any documentation for the registers here, and as you can
see I'm an amateur with respect to memory ordering based on my prior
comment.

But you:

    1. Read intf_reg_off into variable iface
    2. Write the RESET_SPEED for the appropriate mode to MAC_CTRL_REG
    3. wmb() to ensure that write goes through
    4. Read intf_reg_off (regmap_update_bits())
    5. Write 0 to MX93_GPR_ENET_QOS_INTF_MODE_MASK within intf_reg_off (regmap_update_bits())
    6. Sleep for 50-100 us
    7. Read intf_reg_off (regmap_update_bits())
    8. Write MX93_GPR_ENET_QOS_CLK_GEN_EN | iface (from 1) to
       MX93_GPR_ENET_QOS_INTF_MODE_MASK within intf_reg_off (regmap_update_bits())

I don't know what those bits do, but your description sounds like you
are trying to stop the clock for 50-100 us. In your code, if I
understand the memory ordering correctly, both of the following could
occur:

    1. Write RESET_SPEED
    2. Write 0 to MX93_GPR_ENET_QOS_INTF_MODE_MASK
    3. sleep
    4. Restore MX93_GPR_ENET_QOS_CLK_GEN_EN | iface

    or

    1. Write RESET_SPEED
    2. sleep
    3. Write 0 to MX93_GPR_ENET_QOS_INTF_MODE_MASK
    4. Restore MX93_GPR_ENET_QOS_CLK_GEN_EN | iface

is the latter acceptable to you, or does that wmb() (or alternative)
need to move? It seems to me only the first situation would stop
the clock before sleeping, but that's going off the names in this driver
only.

In either case, shouldn't regmap_update_bits() force a
read of said bits, which would remove the need for that wmb() altogether
to synchronize the two writes?


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

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

[Andrew Halaney]

On Thu, Jul 27, 2023 at 10:25:03AM -0500, 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.
> 

I'd just like to highlight that because of a quirk (I think this is not
standard) in the particular connected switch on a board you're making the
whole "fsl,imx93" platform (compatible) implement said switch quirk.

If you don't think there's any harm in doing that for other fixed-link
scenarios, that's fine I suppose... but just highlighting that.

I have no idea at a higher level how else you'd tackle this. You could
add a dt property for this, but I also don't love that you'd probably
encode it in the MAC (maybe in the fixed-link description it would be
more attractive). At least as a dt property it isn't unconditional.

> Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> Reviewed-by: Frank Li <frank.li@nxp.com>
> ---
>  .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 45 +++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> index 53ee5a42c071..e7819960128e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> @@ -40,6 +40,9 @@
>  #define DMA_BUS_MODE			0x00001000
>  #define DMA_BUS_MODE_SFT_RESET		(0x1 << 0)
>  #define RMII_RESET_SPEED		(0x3 << 14)
> +#define MII_RESET_SPEED			(0x2 << 14)
> +#define RGMII_RESET_SPEED		(0x0 << 14)
> +#define CTRL_SPEED_MASK			(0x3 << 14)

GENMASK() would be cleaner, as well as BIT() usage, but I do see
the driver currently does shifts.. so /me shrugs

>  
>  struct imx_dwmac_ops {
>  	u32 addr_width;
> @@ -56,6 +59,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 +216,44 @@ 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_MODE_MASK;
> +	old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
> +	ctrl = old_ctrl & ~CTRL_SPEED_MASK;
> +
> +	/* by default ctrl will be RGMII */
> +	if (iface == MX93_GPR_ENET_QOS_INTF_SEL_RMII)
> +		ctrl |= RMII_RESET_SPEED;
> +	if (iface == MX93_GPR_ENET_QOS_INTF_SEL_MII)
> +		ctrl |= MII_RESET_SPEED;

I see that ctrl right now would select RGMII, but I think it would
read more clearly if you handled it and made the above an
if/else if/else statement (since they're exclusive of eachother)
vs two independent if's.

> +
> +	writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);
> +
> +	/* Ensure the settings for CTRL are applied */
> +	wmb();

I saw this and recently have been wondering about this sort of pattern
(not an expert on this). From what I can tell it seems reading the
register back is the preferred pattern to force the write out. The above works,
but it feels to me personally akin to how local_lock()
in the kernel is a more fine grained mechanism than using
preempt_disable(). But that's pretty opinionated. See device-io.rst
and io_ordering.rst for how I came to that conclusion.

> +
> +	regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
> +			   MX93_GPR_ENET_QOS_INTF_MODE_MASK, 0);
> +	usleep_range(50, 100);
> +	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);
> +}

I don't have any documentation for the registers here, and as you can
see I'm an amateur with respect to memory ordering based on my prior
comment.

But you:

    1. Read intf_reg_off into variable iface
    2. Write the RESET_SPEED for the appropriate mode to MAC_CTRL_REG
    3. wmb() to ensure that write goes through
    4. Read intf_reg_off (regmap_update_bits())
    5. Write 0 to MX93_GPR_ENET_QOS_INTF_MODE_MASK within intf_reg_off (regmap_update_bits())
    6. Sleep for 50-100 us
    7. Read intf_reg_off (regmap_update_bits())
    8. Write MX93_GPR_ENET_QOS_CLK_GEN_EN | iface (from 1) to
       MX93_GPR_ENET_QOS_INTF_MODE_MASK within intf_reg_off (regmap_update_bits())

I don't know what those bits do, but your description sounds like you
are trying to stop the clock for 50-100 us. In your code, if I
understand the memory ordering correctly, both of the following could
occur:

    1. Write RESET_SPEED
    2. Write 0 to MX93_GPR_ENET_QOS_INTF_MODE_MASK
    3. sleep
    4. Restore MX93_GPR_ENET_QOS_CLK_GEN_EN | iface

    or

    1. Write RESET_SPEED
    2. sleep
    3. Write 0 to MX93_GPR_ENET_QOS_INTF_MODE_MASK
    4. Restore MX93_GPR_ENET_QOS_CLK_GEN_EN | iface

is the latter acceptable to you, or does that wmb() (or alternative)
need to move? It seems to me only the first situation would stop
the clock before sleeping, but that's going off the names in this driver
only.

In either case, shouldn't regmap_update_bits() force a
read of said bits, which would remove the need for that wmb() altogether
to synchronize the two writes?


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

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

[Fabio Estevam]

On Thu, Jul 27, 2023 at 12:25 PM Shenwei Wang <shenwei.wang@nxp.com> wrote:

>         struct plat_stmmacenet_data *plat_dat = priv;
> @@ -317,8 +359,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;

So you are forcing this on all imx93 boards, even if they don't use a SJA1105.

Andrew Lunn gave the following feedback in v1:

"The SJA1105 has the problem, so i would expect it to be involved in
the solution. Otherwise, how is this going to work for other MAC
drivers?

Maybe you need to expose a common clock framework clock for the TXC
clock line, which the SJA1105 can disable/enable? That then makes it
clear what other MAC drivers need to do."

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

[Fabio Estevam]

On Thu, Jul 27, 2023 at 12:25 PM Shenwei Wang <shenwei.wang@nxp.com> wrote:

>         struct plat_stmmacenet_data *plat_dat = priv;
> @@ -317,8 +359,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;

So you are forcing this on all imx93 boards, even if they don't use a SJA1105.

Andrew Lunn gave the following feedback in v1:

"The SJA1105 has the problem, so i would expect it to be involved in
the solution. Otherwise, how is this going to work for other MAC
drivers?

Maybe you need to expose a common clock framework clock for the TXC
clock line, which the SJA1105 can disable/enable? That then makes it
clear what other MAC drivers need to do."

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

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

[Fabio Estevam]

On Thu, Jul 27, 2023 at 12:25 PM Shenwei Wang <shenwei.wang@nxp.com> wrote:

>         struct plat_stmmacenet_data *plat_dat = priv;
> @@ -317,8 +359,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;

So you are forcing this on all imx93 boards, even if they don't use a SJA1105.

Andrew Lunn gave the following feedback in v1:

"The SJA1105 has the problem, so i would expect it to be involved in
the solution. Otherwise, how is this going to work for other MAC
drivers?

Maybe you need to expose a common clock framework clock for the TXC
clock line, which the SJA1105 can disable/enable? That then makes it
clear what other MAC drivers need to do."

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

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

[Shenwei Wang]

> -----Original Message-----
> From: Andrew Halaney <ahalaney@redhat.com>
> Sent: Thursday, July 27, 2023 1:37 PM
> 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
> > 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.
> >
> 
> I'd just like to highlight that because of a quirk (I think this is not
> standard) in the particular connected switch on a board you're making the whole
> "fsl,imx93" platform (compatible) implement said switch quirk.
> 
> If you don't think there's any harm in doing that for other fixed-link scenarios,
> that's fine I suppose... but just highlighting that.
> 
> I have no idea at a higher level how else you'd tackle this. You could add a dt
> property for this, but I also don't love that you'd probably encode it in the MAC
> (maybe in the fixed-link description it would be more attractive). At least as a dt
> property it isn't unconditional.
> 

This change won't impact the function of any normal cases, introducing a dt property
is not necessary IMO.

> > Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> > Reviewed-by: Frank Li <frank.li@nxp.com>
> > ---
> >  .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 45 +++++++++++++++++++
> >  1 file changed, 45 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > index 53ee5a42c071..e7819960128e 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > @@ -40,6 +40,9 @@
> >  #define DMA_BUS_MODE                 0x00001000
> >  #define DMA_BUS_MODE_SFT_RESET               (0x1 << 0)
> >  #define RMII_RESET_SPEED             (0x3 << 14)
> > +#define MII_RESET_SPEED                      (0x2 << 14)
> > +#define RGMII_RESET_SPEED            (0x0 << 14)
> > +#define CTRL_SPEED_MASK                      (0x3 << 14)
> 
> GENMASK() would be cleaner, as well as BIT() usage, but I do see the driver
> currently does shifts.. so /me shrugs
> 

Okay.

> >
> >  struct imx_dwmac_ops {
> >       u32 addr_width;
> > @@ -56,6 +59,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 +216,44 @@
> > 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_MODE_MASK;
> > +     old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
> > +     ctrl = old_ctrl & ~CTRL_SPEED_MASK;
> > +
> > +     /* by default ctrl will be RGMII */
> > +     if (iface == MX93_GPR_ENET_QOS_INTF_SEL_RMII)
> > +             ctrl |= RMII_RESET_SPEED;
> > +     if (iface == MX93_GPR_ENET_QOS_INTF_SEL_MII)
> > +             ctrl |= MII_RESET_SPEED;
> 
> I see that ctrl right now would select RGMII, but I think it would read more
> clearly if you handled it and made the above an if/else if/else statement (since
> they're exclusive of eachother) vs two independent if's.
> 

I think I did too much here. The other two cases should be removed as only 
RGMII requires to add delays on the clock line.

> > +
> > +     writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);
> > +
> > +     /* Ensure the settings for CTRL are applied */
> > +     wmb();
> 
> I saw this and recently have been wondering about this sort of pattern (not an
> expert on this). From what I can tell it seems reading the register back is the
> preferred pattern to force the write out. The above works, but it feels to me
> personally akin to how local_lock() in the kernel is a more fine grained
> mechanism than using preempt_disable(). But that's pretty opinionated. See
> device-io.rst and io_ordering.rst for how I came to that conclusion.
> 

wmb is necessary here as we want to delay such a period after the registers are
written. But the location should be moved to before the usleep_range() line, so
that it could avoid the scenario #2 that you pointed out below.

Thanks,
Shenwei

> > +
> > +     regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
> > +                        MX93_GPR_ENET_QOS_INTF_MODE_MASK, 0);
> > +     usleep_range(50, 100);
> > +     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); }
> 
> I don't have any documentation for the registers here, and as you can see I'm an
> amateur with respect to memory ordering based on my prior comment.
> 
> But you:
> 
>     1. Read intf_reg_off into variable iface
>     2. Write the RESET_SPEED for the appropriate mode to MAC_CTRL_REG
>     3. wmb() to ensure that write goes through
>     4. Read intf_reg_off (regmap_update_bits())
>     5. Write 0 to MX93_GPR_ENET_QOS_INTF_MODE_MASK within intf_reg_off
> (regmap_update_bits())
>     6. Sleep for 50-100 us
>     7. Read intf_reg_off (regmap_update_bits())
>     8. Write MX93_GPR_ENET_QOS_CLK_GEN_EN | iface (from 1) to
>        MX93_GPR_ENET_QOS_INTF_MODE_MASK within intf_reg_off
> (regmap_update_bits())
> 
> I don't know what those bits do, but your description sounds like you are trying
> to stop the clock for 50-100 us. In your code, if I understand the memory
> ordering correctly, both of the following could
> occur:
> 
>     1. Write RESET_SPEED
>     2. Write 0 to MX93_GPR_ENET_QOS_INTF_MODE_MASK
>     3. sleep
>     4. Restore MX93_GPR_ENET_QOS_CLK_GEN_EN | iface
> 
>     or
> 
>     1. Write RESET_SPEED
>     2. sleep
>     3. Write 0 to MX93_GPR_ENET_QOS_INTF_MODE_MASK
>     4. Restore MX93_GPR_ENET_QOS_CLK_GEN_EN | iface
> 
> is the latter acceptable to you, or does that wmb() (or alternative) need to move?
> It seems to me only the first situation would stop the clock before sleeping, but
> that's going off the names in this driver only.
> 
> In either case, shouldn't regmap_update_bits() force a read of said bits, which
> would remove the need for that wmb() altogether to synchronize the two writes?

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

[Shenwei Wang]

> -----Original Message-----
> From: Andrew Halaney <ahalaney@redhat.com>
> Sent: Thursday, July 27, 2023 1:37 PM
> 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
> > 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.
> >
> 
> I'd just like to highlight that because of a quirk (I think this is not
> standard) in the particular connected switch on a board you're making the whole
> "fsl,imx93" platform (compatible) implement said switch quirk.
> 
> If you don't think there's any harm in doing that for other fixed-link scenarios,
> that's fine I suppose... but just highlighting that.
> 
> I have no idea at a higher level how else you'd tackle this. You could add a dt
> property for this, but I also don't love that you'd probably encode it in the MAC
> (maybe in the fixed-link description it would be more attractive). At least as a dt
> property it isn't unconditional.
> 

This change won't impact the function of any normal cases, introducing a dt property
is not necessary IMO.

> > Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> > Reviewed-by: Frank Li <frank.li@nxp.com>
> > ---
> >  .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 45 +++++++++++++++++++
> >  1 file changed, 45 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > index 53ee5a42c071..e7819960128e 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > @@ -40,6 +40,9 @@
> >  #define DMA_BUS_MODE                 0x00001000
> >  #define DMA_BUS_MODE_SFT_RESET               (0x1 << 0)
> >  #define RMII_RESET_SPEED             (0x3 << 14)
> > +#define MII_RESET_SPEED                      (0x2 << 14)
> > +#define RGMII_RESET_SPEED            (0x0 << 14)
> > +#define CTRL_SPEED_MASK                      (0x3 << 14)
> 
> GENMASK() would be cleaner, as well as BIT() usage, but I do see the driver
> currently does shifts.. so /me shrugs
> 

Okay.

> >
> >  struct imx_dwmac_ops {
> >       u32 addr_width;
> > @@ -56,6 +59,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 +216,44 @@
> > 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_MODE_MASK;
> > +     old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
> > +     ctrl = old_ctrl & ~CTRL_SPEED_MASK;
> > +
> > +     /* by default ctrl will be RGMII */
> > +     if (iface == MX93_GPR_ENET_QOS_INTF_SEL_RMII)
> > +             ctrl |= RMII_RESET_SPEED;
> > +     if (iface == MX93_GPR_ENET_QOS_INTF_SEL_MII)
> > +             ctrl |= MII_RESET_SPEED;
> 
> I see that ctrl right now would select RGMII, but I think it would read more
> clearly if you handled it and made the above an if/else if/else statement (since
> they're exclusive of eachother) vs two independent if's.
> 

I think I did too much here. The other two cases should be removed as only 
RGMII requires to add delays on the clock line.

> > +
> > +     writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);
> > +
> > +     /* Ensure the settings for CTRL are applied */
> > +     wmb();
> 
> I saw this and recently have been wondering about this sort of pattern (not an
> expert on this). From what I can tell it seems reading the register back is the
> preferred pattern to force the write out. The above works, but it feels to me
> personally akin to how local_lock() in the kernel is a more fine grained
> mechanism than using preempt_disable(). But that's pretty opinionated. See
> device-io.rst and io_ordering.rst for how I came to that conclusion.
> 

wmb is necessary here as we want to delay such a period after the registers are
written. But the location should be moved to before the usleep_range() line, so
that it could avoid the scenario #2 that you pointed out below.

Thanks,
Shenwei

> > +
> > +     regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
> > +                        MX93_GPR_ENET_QOS_INTF_MODE_MASK, 0);
> > +     usleep_range(50, 100);
> > +     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); }
> 
> I don't have any documentation for the registers here, and as you can see I'm an
> amateur with respect to memory ordering based on my prior comment.
> 
> But you:
> 
>     1. Read intf_reg_off into variable iface
>     2. Write the RESET_SPEED for the appropriate mode to MAC_CTRL_REG
>     3. wmb() to ensure that write goes through
>     4. Read intf_reg_off (regmap_update_bits())
>     5. Write 0 to MX93_GPR_ENET_QOS_INTF_MODE_MASK within intf_reg_off
> (regmap_update_bits())
>     6. Sleep for 50-100 us
>     7. Read intf_reg_off (regmap_update_bits())
>     8. Write MX93_GPR_ENET_QOS_CLK_GEN_EN | iface (from 1) to
>        MX93_GPR_ENET_QOS_INTF_MODE_MASK within intf_reg_off
> (regmap_update_bits())
> 
> I don't know what those bits do, but your description sounds like you are trying
> to stop the clock for 50-100 us. In your code, if I understand the memory
> ordering correctly, both of the following could
> occur:
> 
>     1. Write RESET_SPEED
>     2. Write 0 to MX93_GPR_ENET_QOS_INTF_MODE_MASK
>     3. sleep
>     4. Restore MX93_GPR_ENET_QOS_CLK_GEN_EN | iface
> 
>     or
> 
>     1. Write RESET_SPEED
>     2. sleep
>     3. Write 0 to MX93_GPR_ENET_QOS_INTF_MODE_MASK
>     4. Restore MX93_GPR_ENET_QOS_CLK_GEN_EN | iface
> 
> is the latter acceptable to you, or does that wmb() (or alternative) need to move?
> It seems to me only the first situation would stop the clock before sleeping, but
> that's going off the names in this driver only.
> 
> In either case, shouldn't regmap_update_bits() force a read of said bits, which
> would remove the need for that wmb() altogether to synchronize the two writes?


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

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

[Shenwei Wang]

> -----Original Message-----
> From: Andrew Halaney <ahalaney@redhat.com>
> Sent: Thursday, July 27, 2023 1:37 PM
> 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
> > 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.
> >
> 
> I'd just like to highlight that because of a quirk (I think this is not
> standard) in the particular connected switch on a board you're making the whole
> "fsl,imx93" platform (compatible) implement said switch quirk.
> 
> If you don't think there's any harm in doing that for other fixed-link scenarios,
> that's fine I suppose... but just highlighting that.
> 
> I have no idea at a higher level how else you'd tackle this. You could add a dt
> property for this, but I also don't love that you'd probably encode it in the MAC
> (maybe in the fixed-link description it would be more attractive). At least as a dt
> property it isn't unconditional.
> 

This change won't impact the function of any normal cases, introducing a dt property
is not necessary IMO.

> > Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> > Reviewed-by: Frank Li <frank.li@nxp.com>
> > ---
> >  .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 45 +++++++++++++++++++
> >  1 file changed, 45 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > index 53ee5a42c071..e7819960128e 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > @@ -40,6 +40,9 @@
> >  #define DMA_BUS_MODE                 0x00001000
> >  #define DMA_BUS_MODE_SFT_RESET               (0x1 << 0)
> >  #define RMII_RESET_SPEED             (0x3 << 14)
> > +#define MII_RESET_SPEED                      (0x2 << 14)
> > +#define RGMII_RESET_SPEED            (0x0 << 14)
> > +#define CTRL_SPEED_MASK                      (0x3 << 14)
> 
> GENMASK() would be cleaner, as well as BIT() usage, but I do see the driver
> currently does shifts.. so /me shrugs
> 

Okay.

> >
> >  struct imx_dwmac_ops {
> >       u32 addr_width;
> > @@ -56,6 +59,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 +216,44 @@
> > 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_MODE_MASK;
> > +     old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
> > +     ctrl = old_ctrl & ~CTRL_SPEED_MASK;
> > +
> > +     /* by default ctrl will be RGMII */
> > +     if (iface == MX93_GPR_ENET_QOS_INTF_SEL_RMII)
> > +             ctrl |= RMII_RESET_SPEED;
> > +     if (iface == MX93_GPR_ENET_QOS_INTF_SEL_MII)
> > +             ctrl |= MII_RESET_SPEED;
> 
> I see that ctrl right now would select RGMII, but I think it would read more
> clearly if you handled it and made the above an if/else if/else statement (since
> they're exclusive of eachother) vs two independent if's.
> 

I think I did too much here. The other two cases should be removed as only 
RGMII requires to add delays on the clock line.

> > +
> > +     writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);
> > +
> > +     /* Ensure the settings for CTRL are applied */
> > +     wmb();
> 
> I saw this and recently have been wondering about this sort of pattern (not an
> expert on this). From what I can tell it seems reading the register back is the
> preferred pattern to force the write out. The above works, but it feels to me
> personally akin to how local_lock() in the kernel is a more fine grained
> mechanism than using preempt_disable(). But that's pretty opinionated. See
> device-io.rst and io_ordering.rst for how I came to that conclusion.
> 

wmb is necessary here as we want to delay such a period after the registers are
written. But the location should be moved to before the usleep_range() line, so
that it could avoid the scenario #2 that you pointed out below.

Thanks,
Shenwei

> > +
> > +     regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
> > +                        MX93_GPR_ENET_QOS_INTF_MODE_MASK, 0);
> > +     usleep_range(50, 100);
> > +     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); }
> 
> I don't have any documentation for the registers here, and as you can see I'm an
> amateur with respect to memory ordering based on my prior comment.
> 
> But you:
> 
>     1. Read intf_reg_off into variable iface
>     2. Write the RESET_SPEED for the appropriate mode to MAC_CTRL_REG
>     3. wmb() to ensure that write goes through
>     4. Read intf_reg_off (regmap_update_bits())
>     5. Write 0 to MX93_GPR_ENET_QOS_INTF_MODE_MASK within intf_reg_off
> (regmap_update_bits())
>     6. Sleep for 50-100 us
>     7. Read intf_reg_off (regmap_update_bits())
>     8. Write MX93_GPR_ENET_QOS_CLK_GEN_EN | iface (from 1) to
>        MX93_GPR_ENET_QOS_INTF_MODE_MASK within intf_reg_off
> (regmap_update_bits())
> 
> I don't know what those bits do, but your description sounds like you are trying
> to stop the clock for 50-100 us. In your code, if I understand the memory
> ordering correctly, both of the following could
> occur:
> 
>     1. Write RESET_SPEED
>     2. Write 0 to MX93_GPR_ENET_QOS_INTF_MODE_MASK
>     3. sleep
>     4. Restore MX93_GPR_ENET_QOS_CLK_GEN_EN | iface
> 
>     or
> 
>     1. Write RESET_SPEED
>     2. sleep
>     3. Write 0 to MX93_GPR_ENET_QOS_INTF_MODE_MASK
>     4. Restore MX93_GPR_ENET_QOS_CLK_GEN_EN | iface
> 
> is the latter acceptable to you, or does that wmb() (or alternative) need to move?
> It seems to me only the first situation would stop the clock before sleeping, but
> that's going off the names in this driver only.
> 
> In either case, shouldn't regmap_update_bits() force a read of said bits, which
> would remove the need for that wmb() altogether to synchronize the two writes?


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

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

[Shenwei Wang]

> -----Original Message-----
> From: Fabio Estevam <festevam@gmail.com>
> Sent: Friday, July 28, 2023 6:01 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>; Andrew Lunn <andrew@lunn.ch>
> 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>;
> Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; dl-linux-imx <linux-
> 
> 
> On Thu, Jul 27, 2023 at 12:25 PM Shenwei Wang <shenwei.wang@nxp.com>
> wrote:
> 
> >         struct plat_stmmacenet_data *plat_dat = priv; @@ -317,8
> > +359,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;
> 
> So you are forcing this on all imx93 boards, even if they don't use a SJA1105.
> 

Yes, that's the purpose because it won't hurt even the other side is not SJA1105.

> Andrew Lunn gave the following feedback in v1:
> 
> "The SJA1105 has the problem, so i would expect it to be involved in the solution.
> Otherwise, how is this going to work for other MAC drivers?
> 
> Maybe you need to expose a common clock framework clock for the TXC clock
> line, which the SJA1105 can disable/enable? That then makes it clear what other
> MAC drivers need to do."

I have been considering this plan for some time. The idea should be implemented 
across all i.mx8/9 platforms. I am going to start to work on it in the following month, 
and it will take some time to implement it.

Thanks,
Shenwei

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

[Shenwei Wang]

> -----Original Message-----
> From: Fabio Estevam <festevam@gmail.com>
> Sent: Friday, July 28, 2023 6:01 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>; Andrew Lunn <andrew@lunn.ch>
> 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>;
> Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; dl-linux-imx <linux-
> 
> 
> On Thu, Jul 27, 2023 at 12:25 PM Shenwei Wang <shenwei.wang@nxp.com>
> wrote:
> 
> >         struct plat_stmmacenet_data *plat_dat = priv; @@ -317,8
> > +359,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;
> 
> So you are forcing this on all imx93 boards, even if they don't use a SJA1105.
> 

Yes, that's the purpose because it won't hurt even the other side is not SJA1105.

> Andrew Lunn gave the following feedback in v1:
> 
> "The SJA1105 has the problem, so i would expect it to be involved in the solution.
> Otherwise, how is this going to work for other MAC drivers?
> 
> Maybe you need to expose a common clock framework clock for the TXC clock
> line, which the SJA1105 can disable/enable? That then makes it clear what other
> MAC drivers need to do."

I have been considering this plan for some time. The idea should be implemented 
across all i.mx8/9 platforms. I am going to start to work on it in the following month, 
and it will take some time to implement it.

Thanks,
Shenwei



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

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

[Shenwei Wang]

> -----Original Message-----
> From: Fabio Estevam <festevam@gmail.com>
> Sent: Friday, July 28, 2023 6:01 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>; Andrew Lunn <andrew@lunn.ch>
> 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>;
> Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; dl-linux-imx <linux-
> 
> 
> On Thu, Jul 27, 2023 at 12:25 PM Shenwei Wang <shenwei.wang@nxp.com>
> wrote:
> 
> >         struct plat_stmmacenet_data *plat_dat = priv; @@ -317,8
> > +359,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;
> 
> So you are forcing this on all imx93 boards, even if they don't use a SJA1105.
> 

Yes, that's the purpose because it won't hurt even the other side is not SJA1105.

> Andrew Lunn gave the following feedback in v1:
> 
> "The SJA1105 has the problem, so i would expect it to be involved in the solution.
> Otherwise, how is this going to work for other MAC drivers?
> 
> Maybe you need to expose a common clock framework clock for the TXC clock
> line, which the SJA1105 can disable/enable? That then makes it clear what other
> MAC drivers need to do."

I have been considering this plan for some time. The idea should be implemented 
across all i.mx8/9 platforms. I am going to start to work on it in the following month, 
and it will take some time to implement it.

Thanks,
Shenwei



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

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

[Russell King (Oracle)]

On Thu, Jul 27, 2023 at 01:36:45PM -0500, Andrew Halaney wrote:
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > index 53ee5a42c071..e7819960128e 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > @@ -40,6 +40,9 @@
> >  #define DMA_BUS_MODE			0x00001000
> >  #define DMA_BUS_MODE_SFT_RESET		(0x1 << 0)
> >  #define RMII_RESET_SPEED		(0x3 << 14)
> > +#define MII_RESET_SPEED			(0x2 << 14)
> > +#define RGMII_RESET_SPEED		(0x0 << 14)
> > +#define CTRL_SPEED_MASK			(0x3 << 14)
> 
> GENMASK() would be cleaner, as well as BIT() usage, but I do see
> the driver currently does shifts.. so /me shrugs

BIT() is only useful for single-bit items, not for use with bitfields,
and their use with bitfields just makes the whole thing perverse.

#define CTRL_SPEED_MASK		GENMASK(15, 14)
#define CTRL_SPEED_RGMII_RESET	0
#define CTRL_SPEED_MII_RESET	2
#define CTRL_SPEED_RMII_RESET	3

and then its use:

	FIELD_PREP(CTRL_SPEED_MASK, CTRL_SPEED_RGMII_RESET)
or
	FIELD_PREP(CTRL_SPEED_MASK, CTRL_SPEED_MII_RESET)
or
	FIELD_PREP(CTRL_SPEED_MASK, CTRL_SPEED_RMII_RESET)

alternatively:

        if (iface == MX93_GPR_ENET_QOS_INTF_SEL_RMII)
                speed = CTRL_SPEED_RMII_RESET;
        else (iface == MX93_GPR_ENET_QOS_INTF_SEL_MII)
                speed = CTRL_SPEED_MII_RESET;
	else
		speed = CTRL_SPEED_RGMII_RESET;

	old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
	ctrl = old_ctrl & ~CTRL_SPEED_MASK;
	ctrl |= FIELD_PREP(CTRL_SPEED_MASK, speed);
	writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);

> I don't have any documentation for the registers here, and as you can
> see I'm an amateur with respect to memory ordering based on my prior
> comment.
> 
> But you:
> 
>     1. Read intf_reg_off into variable iface
>     2. Write the RESET_SPEED for the appropriate mode to MAC_CTRL_REG
>     3. wmb() to ensure that write goes through

I wonder about whether that wmb() is required. If the mapping is
device-like rather than memory-like, the write should be committed
before the read that regmap_update_bits() does according to the ARM
memory model. Maybe a bit of information about where this barrier
has come from would be good, and maybe getting it reviewed by the
arm64 barrier specialist, Will Deacon. :)

wmb() is normally required to be paired with a rmb(), but we're not
talking about system memory here, so I also wonder whether wmb() is
the correct barrier to use.

Adding Will.

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

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

[Russell King (Oracle)]

On Thu, Jul 27, 2023 at 01:36:45PM -0500, Andrew Halaney wrote:
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > index 53ee5a42c071..e7819960128e 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > @@ -40,6 +40,9 @@
> >  #define DMA_BUS_MODE			0x00001000
> >  #define DMA_BUS_MODE_SFT_RESET		(0x1 << 0)
> >  #define RMII_RESET_SPEED		(0x3 << 14)
> > +#define MII_RESET_SPEED			(0x2 << 14)
> > +#define RGMII_RESET_SPEED		(0x0 << 14)
> > +#define CTRL_SPEED_MASK			(0x3 << 14)
> 
> GENMASK() would be cleaner, as well as BIT() usage, but I do see
> the driver currently does shifts.. so /me shrugs

BIT() is only useful for single-bit items, not for use with bitfields,
and their use with bitfields just makes the whole thing perverse.

#define CTRL_SPEED_MASK		GENMASK(15, 14)
#define CTRL_SPEED_RGMII_RESET	0
#define CTRL_SPEED_MII_RESET	2
#define CTRL_SPEED_RMII_RESET	3

and then its use:

	FIELD_PREP(CTRL_SPEED_MASK, CTRL_SPEED_RGMII_RESET)
or
	FIELD_PREP(CTRL_SPEED_MASK, CTRL_SPEED_MII_RESET)
or
	FIELD_PREP(CTRL_SPEED_MASK, CTRL_SPEED_RMII_RESET)

alternatively:

        if (iface == MX93_GPR_ENET_QOS_INTF_SEL_RMII)
                speed = CTRL_SPEED_RMII_RESET;
        else (iface == MX93_GPR_ENET_QOS_INTF_SEL_MII)
                speed = CTRL_SPEED_MII_RESET;
	else
		speed = CTRL_SPEED_RGMII_RESET;

	old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
	ctrl = old_ctrl & ~CTRL_SPEED_MASK;
	ctrl |= FIELD_PREP(CTRL_SPEED_MASK, speed);
	writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);

> I don't have any documentation for the registers here, and as you can
> see I'm an amateur with respect to memory ordering based on my prior
> comment.
> 
> But you:
> 
>     1. Read intf_reg_off into variable iface
>     2. Write the RESET_SPEED for the appropriate mode to MAC_CTRL_REG
>     3. wmb() to ensure that write goes through

I wonder about whether that wmb() is required. If the mapping is
device-like rather than memory-like, the write should be committed
before the read that regmap_update_bits() does according to the ARM
memory model. Maybe a bit of information about where this barrier
has come from would be good, and maybe getting it reviewed by the
arm64 barrier specialist, Will Deacon. :)

wmb() is normally required to be paired with a rmb(), but we're not
talking about system memory here, so I also wonder whether wmb() is
the correct barrier to use.

Adding Will.

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

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

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

[Russell King (Oracle)]

On Thu, Jul 27, 2023 at 01:36:45PM -0500, Andrew Halaney wrote:
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > index 53ee5a42c071..e7819960128e 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > @@ -40,6 +40,9 @@
> >  #define DMA_BUS_MODE			0x00001000
> >  #define DMA_BUS_MODE_SFT_RESET		(0x1 << 0)
> >  #define RMII_RESET_SPEED		(0x3 << 14)
> > +#define MII_RESET_SPEED			(0x2 << 14)
> > +#define RGMII_RESET_SPEED		(0x0 << 14)
> > +#define CTRL_SPEED_MASK			(0x3 << 14)
> 
> GENMASK() would be cleaner, as well as BIT() usage, but I do see
> the driver currently does shifts.. so /me shrugs

BIT() is only useful for single-bit items, not for use with bitfields,
and their use with bitfields just makes the whole thing perverse.

#define CTRL_SPEED_MASK		GENMASK(15, 14)
#define CTRL_SPEED_RGMII_RESET	0
#define CTRL_SPEED_MII_RESET	2
#define CTRL_SPEED_RMII_RESET	3

and then its use:

	FIELD_PREP(CTRL_SPEED_MASK, CTRL_SPEED_RGMII_RESET)
or
	FIELD_PREP(CTRL_SPEED_MASK, CTRL_SPEED_MII_RESET)
or
	FIELD_PREP(CTRL_SPEED_MASK, CTRL_SPEED_RMII_RESET)

alternatively:

        if (iface == MX93_GPR_ENET_QOS_INTF_SEL_RMII)
                speed = CTRL_SPEED_RMII_RESET;
        else (iface == MX93_GPR_ENET_QOS_INTF_SEL_MII)
                speed = CTRL_SPEED_MII_RESET;
	else
		speed = CTRL_SPEED_RGMII_RESET;

	old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
	ctrl = old_ctrl & ~CTRL_SPEED_MASK;
	ctrl |= FIELD_PREP(CTRL_SPEED_MASK, speed);
	writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);

> I don't have any documentation for the registers here, and as you can
> see I'm an amateur with respect to memory ordering based on my prior
> comment.
> 
> But you:
> 
>     1. Read intf_reg_off into variable iface
>     2. Write the RESET_SPEED for the appropriate mode to MAC_CTRL_REG
>     3. wmb() to ensure that write goes through

I wonder about whether that wmb() is required. If the mapping is
device-like rather than memory-like, the write should be committed
before the read that regmap_update_bits() does according to the ARM
memory model. Maybe a bit of information about where this barrier
has come from would be good, and maybe getting it reviewed by the
arm64 barrier specialist, Will Deacon. :)

wmb() is normally required to be paired with a rmb(), but we're not
talking about system memory here, so I also wonder whether wmb() is
the correct barrier to use.

Adding Will.

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

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

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

[Will Deacon]

On Fri, Jul 28, 2023 at 04:22:19PM +0100, Russell King (Oracle) wrote:
> On Thu, Jul 27, 2023 at 01:36:45PM -0500, Andrew Halaney wrote:
> > I don't have any documentation for the registers here, and as you can
> > see I'm an amateur with respect to memory ordering based on my prior
> > comment.
> > 
> > But you:
> > 
> >     1. Read intf_reg_off into variable iface
> >     2. Write the RESET_SPEED for the appropriate mode to MAC_CTRL_REG
> >     3. wmb() to ensure that write goes through
> 
> I wonder about whether that wmb() is required. If the mapping is
> device-like rather than memory-like, the write should be committed
> before the read that regmap_update_bits() does according to the ARM
> memory model. Maybe a bit of information about where this barrier
> has come from would be good, and maybe getting it reviewed by the
> arm64 barrier specialist, Will Deacon. :)
> 
> wmb() is normally required to be paired with a rmb(), but we're not
> talking about system memory here, so I also wonder whether wmb() is
> the correct barrier to use.

Yes, I don't think wmb() is the right thing here. If you need to ensure
that the write to MAC_CTRL_REG has taken effect, then you'll need to go
through some device-specific sequence which probably involves reading
something back. If you just need things to arrive in order eventually,
the memory type already gives you that.

It's also worth pointing out that udelay() isn't necessarily ordered wrt
MMIO writes, so that usleep_range() might need some help as well.
Non-relaxed MMIO reads, however, _are_ ordered against a subsequent
udelay(), so if you add the readback then this might all work out.

I gave a (slightly dated) talk about some of this at ELC a while back:

https://www.youtube.com/watch?v=i6DayghhA8Q

which might help.

Will

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

[Will Deacon]

On Fri, Jul 28, 2023 at 04:22:19PM +0100, Russell King (Oracle) wrote:
> On Thu, Jul 27, 2023 at 01:36:45PM -0500, Andrew Halaney wrote:
> > I don't have any documentation for the registers here, and as you can
> > see I'm an amateur with respect to memory ordering based on my prior
> > comment.
> > 
> > But you:
> > 
> >     1. Read intf_reg_off into variable iface
> >     2. Write the RESET_SPEED for the appropriate mode to MAC_CTRL_REG
> >     3. wmb() to ensure that write goes through
> 
> I wonder about whether that wmb() is required. If the mapping is
> device-like rather than memory-like, the write should be committed
> before the read that regmap_update_bits() does according to the ARM
> memory model. Maybe a bit of information about where this barrier
> has come from would be good, and maybe getting it reviewed by the
> arm64 barrier specialist, Will Deacon. :)
> 
> wmb() is normally required to be paired with a rmb(), but we're not
> talking about system memory here, so I also wonder whether wmb() is
> the correct barrier to use.

Yes, I don't think wmb() is the right thing here. If you need to ensure
that the write to MAC_CTRL_REG has taken effect, then you'll need to go
through some device-specific sequence which probably involves reading
something back. If you just need things to arrive in order eventually,
the memory type already gives you that.

It's also worth pointing out that udelay() isn't necessarily ordered wrt
MMIO writes, so that usleep_range() might need some help as well.
Non-relaxed MMIO reads, however, _are_ ordered against a subsequent
udelay(), so if you add the readback then this might all work out.

I gave a (slightly dated) talk about some of this at ELC a while back:

https://www.youtube.com/watch?v=i6DayghhA8Q

which might help.

Will

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

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

[Will Deacon]

On Fri, Jul 28, 2023 at 04:22:19PM +0100, Russell King (Oracle) wrote:
> On Thu, Jul 27, 2023 at 01:36:45PM -0500, Andrew Halaney wrote:
> > I don't have any documentation for the registers here, and as you can
> > see I'm an amateur with respect to memory ordering based on my prior
> > comment.
> > 
> > But you:
> > 
> >     1. Read intf_reg_off into variable iface
> >     2. Write the RESET_SPEED for the appropriate mode to MAC_CTRL_REG
> >     3. wmb() to ensure that write goes through
> 
> I wonder about whether that wmb() is required. If the mapping is
> device-like rather than memory-like, the write should be committed
> before the read that regmap_update_bits() does according to the ARM
> memory model. Maybe a bit of information about where this barrier
> has come from would be good, and maybe getting it reviewed by the
> arm64 barrier specialist, Will Deacon. :)
> 
> wmb() is normally required to be paired with a rmb(), but we're not
> talking about system memory here, so I also wonder whether wmb() is
> the correct barrier to use.

Yes, I don't think wmb() is the right thing here. If you need to ensure
that the write to MAC_CTRL_REG has taken effect, then you'll need to go
through some device-specific sequence which probably involves reading
something back. If you just need things to arrive in order eventually,
the memory type already gives you that.

It's also worth pointing out that udelay() isn't necessarily ordered wrt
MMIO writes, so that usleep_range() might need some help as well.
Non-relaxed MMIO reads, however, _are_ ordered against a subsequent
udelay(), so if you add the readback then this might all work out.

I gave a (slightly dated) talk about some of this at ELC a while back:

https://www.youtube.com/watch?v=i6DayghhA8Q

which might help.

Will

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

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

[kernel test robot]

Hi Shenwei,

kernel test robot noticed the following build errors:

[auto build test ERROR on net/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Shenwei-Wang/net-stmmac-add-new-mode-parameter-for-fix_mac_speed/20230727-232922
base:   net/main
patch link:    https://lore.kernel.org/r/20230727152503.2199550-2-shenwei.wang%40nxp.com
patch subject: [PATCH v2 net 1/2] net: stmmac: add new mode parameter for fix_mac_speed
config: x86_64-randconfig-x004-20230728 (https://download.01.org/0day-ci/archive/20230728/202307282338.veVKQvK3-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce: (https://download.01.org/0day-ci/archive/20230728/202307282338.veVKQvK3-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202307282338.veVKQvK3-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c:359:22: error: incompatible function pointer types assigning to 'void (*)(void *, uint, uint)' (aka 'void (*)(void *, unsigned int, unsigned int)') from 'void (void *, unsigned int)' [-Wincompatible-function-pointer-types]
           data->fix_mac_speed = tegra_eqos_fix_speed;
                               ^ ~~~~~~~~~~~~~~~~~~~~
   1 error generated.
--
>> drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c:108:28: error: incompatible function pointer types assigning to 'void (*)(void *, uint, uint)' (aka 'void (*)(void *, unsigned int, unsigned int)') from 'void (*const)(void *, unsigned int)' [-Wincompatible-function-pointer-types]
                           plat_dat->fix_mac_speed = dwmac->data->fix_mac_speed;
                                                   ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~
   1 error generated.


vim +359 drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c

e6ea2d16fc615e Thierry Reding 2017-03-10  267  
a884915f4cef94 Jisheng Zhang  2020-11-09  268  static int tegra_eqos_probe(struct platform_device *pdev,
e6ea2d16fc615e Thierry Reding 2017-03-10  269  			    struct plat_stmmacenet_data *data,
e6ea2d16fc615e Thierry Reding 2017-03-10  270  			    struct stmmac_resources *res)
e6ea2d16fc615e Thierry Reding 2017-03-10  271  {
1d4605e0aff9ff Ajay Gupta     2019-12-15  272  	struct device *dev = &pdev->dev;
e6ea2d16fc615e Thierry Reding 2017-03-10  273  	struct tegra_eqos *eqos;
e6ea2d16fc615e Thierry Reding 2017-03-10  274  	int err;
e6ea2d16fc615e Thierry Reding 2017-03-10  275  
e6ea2d16fc615e Thierry Reding 2017-03-10  276  	eqos = devm_kzalloc(&pdev->dev, sizeof(*eqos), GFP_KERNEL);
a884915f4cef94 Jisheng Zhang  2020-11-09  277  	if (!eqos)
a884915f4cef94 Jisheng Zhang  2020-11-09  278  		return -ENOMEM;
e6ea2d16fc615e Thierry Reding 2017-03-10  279  
e6ea2d16fc615e Thierry Reding 2017-03-10  280  	eqos->dev = &pdev->dev;
e6ea2d16fc615e Thierry Reding 2017-03-10  281  	eqos->regs = res->addr;
e6ea2d16fc615e Thierry Reding 2017-03-10  282  
1d4605e0aff9ff Ajay Gupta     2019-12-15  283  	if (!is_of_node(dev->fwnode))
1d4605e0aff9ff Ajay Gupta     2019-12-15  284  		goto bypass_clk_reset_gpio;
1d4605e0aff9ff Ajay Gupta     2019-12-15  285  
e6ea2d16fc615e Thierry Reding 2017-03-10  286  	eqos->clk_master = devm_clk_get(&pdev->dev, "master_bus");
e6ea2d16fc615e Thierry Reding 2017-03-10  287  	if (IS_ERR(eqos->clk_master)) {
e6ea2d16fc615e Thierry Reding 2017-03-10  288  		err = PTR_ERR(eqos->clk_master);
e6ea2d16fc615e Thierry Reding 2017-03-10  289  		goto error;
e6ea2d16fc615e Thierry Reding 2017-03-10  290  	}
e6ea2d16fc615e Thierry Reding 2017-03-10  291  
e6ea2d16fc615e Thierry Reding 2017-03-10  292  	err = clk_prepare_enable(eqos->clk_master);
e6ea2d16fc615e Thierry Reding 2017-03-10  293  	if (err < 0)
e6ea2d16fc615e Thierry Reding 2017-03-10  294  		goto error;
e6ea2d16fc615e Thierry Reding 2017-03-10  295  
e6ea2d16fc615e Thierry Reding 2017-03-10  296  	eqos->clk_slave = devm_clk_get(&pdev->dev, "slave_bus");
e6ea2d16fc615e Thierry Reding 2017-03-10  297  	if (IS_ERR(eqos->clk_slave)) {
e6ea2d16fc615e Thierry Reding 2017-03-10  298  		err = PTR_ERR(eqos->clk_slave);
e6ea2d16fc615e Thierry Reding 2017-03-10  299  		goto disable_master;
e6ea2d16fc615e Thierry Reding 2017-03-10  300  	}
e6ea2d16fc615e Thierry Reding 2017-03-10  301  
e6ea2d16fc615e Thierry Reding 2017-03-10  302  	data->stmmac_clk = eqos->clk_slave;
e6ea2d16fc615e Thierry Reding 2017-03-10  303  
e6ea2d16fc615e Thierry Reding 2017-03-10  304  	err = clk_prepare_enable(eqos->clk_slave);
e6ea2d16fc615e Thierry Reding 2017-03-10  305  	if (err < 0)
e6ea2d16fc615e Thierry Reding 2017-03-10  306  		goto disable_master;
e6ea2d16fc615e Thierry Reding 2017-03-10  307  
e6ea2d16fc615e Thierry Reding 2017-03-10  308  	eqos->clk_rx = devm_clk_get(&pdev->dev, "rx");
e6ea2d16fc615e Thierry Reding 2017-03-10  309  	if (IS_ERR(eqos->clk_rx)) {
e6ea2d16fc615e Thierry Reding 2017-03-10  310  		err = PTR_ERR(eqos->clk_rx);
e6ea2d16fc615e Thierry Reding 2017-03-10  311  		goto disable_slave;
e6ea2d16fc615e Thierry Reding 2017-03-10  312  	}
e6ea2d16fc615e Thierry Reding 2017-03-10  313  
e6ea2d16fc615e Thierry Reding 2017-03-10  314  	err = clk_prepare_enable(eqos->clk_rx);
e6ea2d16fc615e Thierry Reding 2017-03-10  315  	if (err < 0)
e6ea2d16fc615e Thierry Reding 2017-03-10  316  		goto disable_slave;
e6ea2d16fc615e Thierry Reding 2017-03-10  317  
e6ea2d16fc615e Thierry Reding 2017-03-10  318  	eqos->clk_tx = devm_clk_get(&pdev->dev, "tx");
e6ea2d16fc615e Thierry Reding 2017-03-10  319  	if (IS_ERR(eqos->clk_tx)) {
e6ea2d16fc615e Thierry Reding 2017-03-10  320  		err = PTR_ERR(eqos->clk_tx);
e6ea2d16fc615e Thierry Reding 2017-03-10  321  		goto disable_rx;
e6ea2d16fc615e Thierry Reding 2017-03-10  322  	}
e6ea2d16fc615e Thierry Reding 2017-03-10  323  
e6ea2d16fc615e Thierry Reding 2017-03-10  324  	err = clk_prepare_enable(eqos->clk_tx);
e6ea2d16fc615e Thierry Reding 2017-03-10  325  	if (err < 0)
e6ea2d16fc615e Thierry Reding 2017-03-10  326  		goto disable_rx;
e6ea2d16fc615e Thierry Reding 2017-03-10  327  
e6ea2d16fc615e Thierry Reding 2017-03-10  328  	eqos->reset = devm_gpiod_get(&pdev->dev, "phy-reset", GPIOD_OUT_HIGH);
e6ea2d16fc615e Thierry Reding 2017-03-10  329  	if (IS_ERR(eqos->reset)) {
e6ea2d16fc615e Thierry Reding 2017-03-10  330  		err = PTR_ERR(eqos->reset);
e6ea2d16fc615e Thierry Reding 2017-03-10  331  		goto disable_tx;
e6ea2d16fc615e Thierry Reding 2017-03-10  332  	}
e6ea2d16fc615e Thierry Reding 2017-03-10  333  
e6ea2d16fc615e Thierry Reding 2017-03-10  334  	usleep_range(2000, 4000);
e6ea2d16fc615e Thierry Reding 2017-03-10  335  	gpiod_set_value(eqos->reset, 0);
e6ea2d16fc615e Thierry Reding 2017-03-10  336  
1a981c0586c038 Thierry Reding 2019-07-26  337  	/* MDIO bus was already reset just above */
1a981c0586c038 Thierry Reding 2019-07-26  338  	data->mdio_bus_data->needs_reset = false;
1a981c0586c038 Thierry Reding 2019-07-26  339  
e6ea2d16fc615e Thierry Reding 2017-03-10  340  	eqos->rst = devm_reset_control_get(&pdev->dev, "eqos");
e6ea2d16fc615e Thierry Reding 2017-03-10  341  	if (IS_ERR(eqos->rst)) {
e6ea2d16fc615e Thierry Reding 2017-03-10  342  		err = PTR_ERR(eqos->rst);
e6ea2d16fc615e Thierry Reding 2017-03-10  343  		goto reset_phy;
e6ea2d16fc615e Thierry Reding 2017-03-10  344  	}
e6ea2d16fc615e Thierry Reding 2017-03-10  345  
e6ea2d16fc615e Thierry Reding 2017-03-10  346  	err = reset_control_assert(eqos->rst);
e6ea2d16fc615e Thierry Reding 2017-03-10  347  	if (err < 0)
e6ea2d16fc615e Thierry Reding 2017-03-10  348  		goto reset_phy;
e6ea2d16fc615e Thierry Reding 2017-03-10  349  
e6ea2d16fc615e Thierry Reding 2017-03-10  350  	usleep_range(2000, 4000);
e6ea2d16fc615e Thierry Reding 2017-03-10  351  
e6ea2d16fc615e Thierry Reding 2017-03-10  352  	err = reset_control_deassert(eqos->rst);
e6ea2d16fc615e Thierry Reding 2017-03-10  353  	if (err < 0)
e6ea2d16fc615e Thierry Reding 2017-03-10  354  		goto reset_phy;
e6ea2d16fc615e Thierry Reding 2017-03-10  355  
e6ea2d16fc615e Thierry Reding 2017-03-10  356  	usleep_range(2000, 4000);
e6ea2d16fc615e Thierry Reding 2017-03-10  357  
1d4605e0aff9ff Ajay Gupta     2019-12-15  358  bypass_clk_reset_gpio:
e6ea2d16fc615e Thierry Reding 2017-03-10 @359  	data->fix_mac_speed = tegra_eqos_fix_speed;
e6ea2d16fc615e Thierry Reding 2017-03-10  360  	data->init = tegra_eqos_init;
e6ea2d16fc615e Thierry Reding 2017-03-10  361  	data->bsp_priv = eqos;
029c1c2059e9c4 Jon Hunter     2022-07-06  362  	data->sph_disable = 1;
e6ea2d16fc615e Thierry Reding 2017-03-10  363  
e6ea2d16fc615e Thierry Reding 2017-03-10  364  	err = tegra_eqos_init(pdev, eqos);
e6ea2d16fc615e Thierry Reding 2017-03-10  365  	if (err < 0)
e6ea2d16fc615e Thierry Reding 2017-03-10  366  		goto reset;
e6ea2d16fc615e Thierry Reding 2017-03-10  367  
a884915f4cef94 Jisheng Zhang  2020-11-09  368  	return 0;
e6ea2d16fc615e Thierry Reding 2017-03-10  369  reset:
e6ea2d16fc615e Thierry Reding 2017-03-10  370  	reset_control_assert(eqos->rst);
e6ea2d16fc615e Thierry Reding 2017-03-10  371  reset_phy:
e6ea2d16fc615e Thierry Reding 2017-03-10  372  	gpiod_set_value(eqos->reset, 1);
e6ea2d16fc615e Thierry Reding 2017-03-10  373  disable_tx:
e6ea2d16fc615e Thierry Reding 2017-03-10  374  	clk_disable_unprepare(eqos->clk_tx);
e6ea2d16fc615e Thierry Reding 2017-03-10  375  disable_rx:
e6ea2d16fc615e Thierry Reding 2017-03-10  376  	clk_disable_unprepare(eqos->clk_rx);
e6ea2d16fc615e Thierry Reding 2017-03-10  377  disable_slave:
e6ea2d16fc615e Thierry Reding 2017-03-10  378  	clk_disable_unprepare(eqos->clk_slave);
e6ea2d16fc615e Thierry Reding 2017-03-10  379  disable_master:
e6ea2d16fc615e Thierry Reding 2017-03-10  380  	clk_disable_unprepare(eqos->clk_master);
e6ea2d16fc615e Thierry Reding 2017-03-10  381  error:
a884915f4cef94 Jisheng Zhang  2020-11-09  382  	return err;
e6ea2d16fc615e Thierry Reding 2017-03-10  383  }
e6ea2d16fc615e Thierry Reding 2017-03-10  384  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Shenwei Wang]

> -----Original Message-----
> From: Will Deacon <will@kernel.org>
> Sent: Friday, July 28, 2023 10:36 AM
> To: Russell King (Oracle) <linux@armlinux.org.uk>
> Cc: Andrew Halaney <ahalaney@redhat.com>; 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>; 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 v2 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 Fri, Jul 28, 2023 at 04:22:19PM +0100, Russell King (Oracle) wrote:
> > On Thu, Jul 27, 2023 at 01:36:45PM -0500, Andrew Halaney wrote:
> > > I don't have any documentation for the registers here, and as you
> > > can see I'm an amateur with respect to memory ordering based on my
> > > prior comment.
> > >
> > > But you:
> > >
> > >     1. Read intf_reg_off into variable iface
> > >     2. Write the RESET_SPEED for the appropriate mode to MAC_CTRL_REG
> > >     3. wmb() to ensure that write goes through
> >
> > I wonder about whether that wmb() is required. If the mapping is
> > device-like rather than memory-like, the write should be committed
> > before the read that regmap_update_bits() does according to the ARM
> > memory model. Maybe a bit of information about where this barrier has
> > come from would be good, and maybe getting it reviewed by the
> > arm64 barrier specialist, Will Deacon. :)
> >
> > wmb() is normally required to be paired with a rmb(), but we're not
> > talking about system memory here, so I also wonder whether wmb() is
> > the correct barrier to use.
>
> Yes, I don't think wmb() is the right thing here. If you need to ensure that the
> write to MAC_CTRL_REG has taken effect, then you'll need to go through some
> device-specific sequence which probably involves reading something back. If you
> just need things to arrive in order eventually, the memory type already gives you
> that.
>
> It's also worth pointing out that udelay() isn't necessarily ordered wrt MMIO
> writes, so that usleep_range() might need some help as well.
> Non-relaxed MMIO reads, however, _are_ ordered against a subsequent
> udelay(), so if you add the readback then this might all work out.
>

    1. Write RESET_SPEED
    2. Write 0 to MX93_GPR_ENET_QOS_INTF_MODE_MASK
    3. usleep_range()
    4. Restore MX93_GPR_ENET_QOS_CLK_GEN_EN | iface

In the above example, if a readback after step #2 could ensure sufficient time has passed
before step #4, the wmb() here should be abandoned.

Thanks,
Shenwei

> I gave a (slightly dated) talk about some of this at ELC a while back:
>
> https://www.yo/
> utube.com%2Fwatch%3Fv%3Di6DayghhA8Q&data=05%7C01%7Cshenwei.wang
> %40nxp.com%7C32396fd0396e4e46975f08db8f806680%7C686ea1d3bc2b4c6fa
> 92cd99c5c301635%7C0%7C0%7C638261553857503588%7CUnknown%7CTWFp
> bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> Mn0%3D%7C3000%7C%7C%7C&sdata=X5CQrQEVmUjYafYJ%2BzcnGXI9mhDT%
> 2BMzDazGHOcoomas%3D&reserved=0
>
> which might help.
>
> Will

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

[Shenwei Wang]

> -----Original Message-----
> From: Will Deacon <will@kernel.org>
> Sent: Friday, July 28, 2023 10:36 AM
> To: Russell King (Oracle) <linux@armlinux.org.uk>
> Cc: Andrew Halaney <ahalaney@redhat.com>; 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>; 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 v2 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 Fri, Jul 28, 2023 at 04:22:19PM +0100, Russell King (Oracle) wrote:
> > On Thu, Jul 27, 2023 at 01:36:45PM -0500, Andrew Halaney wrote:
> > > I don't have any documentation for the registers here, and as you
> > > can see I'm an amateur with respect to memory ordering based on my
> > > prior comment.
> > >
> > > But you:
> > >
> > >     1. Read intf_reg_off into variable iface
> > >     2. Write the RESET_SPEED for the appropriate mode to MAC_CTRL_REG
> > >     3. wmb() to ensure that write goes through
> >
> > I wonder about whether that wmb() is required. If the mapping is
> > device-like rather than memory-like, the write should be committed
> > before the read that regmap_update_bits() does according to the ARM
> > memory model. Maybe a bit of information about where this barrier has
> > come from would be good, and maybe getting it reviewed by the
> > arm64 barrier specialist, Will Deacon. :)
> >
> > wmb() is normally required to be paired with a rmb(), but we're not
> > talking about system memory here, so I also wonder whether wmb() is
> > the correct barrier to use.
>
> Yes, I don't think wmb() is the right thing here. If you need to ensure that the
> write to MAC_CTRL_REG has taken effect, then you'll need to go through some
> device-specific sequence which probably involves reading something back. If you
> just need things to arrive in order eventually, the memory type already gives you
> that.
>
> It's also worth pointing out that udelay() isn't necessarily ordered wrt MMIO
> writes, so that usleep_range() might need some help as well.
> Non-relaxed MMIO reads, however, _are_ ordered against a subsequent
> udelay(), so if you add the readback then this might all work out.
>

    1. Write RESET_SPEED
    2. Write 0 to MX93_GPR_ENET_QOS_INTF_MODE_MASK
    3. usleep_range()
    4. Restore MX93_GPR_ENET_QOS_CLK_GEN_EN | iface

In the above example, if a readback after step #2 could ensure sufficient time has passed
before step #4, the wmb() here should be abandoned.

Thanks,
Shenwei

> I gave a (slightly dated) talk about some of this at ELC a while back:
>
> https://www.yo/
> utube.com%2Fwatch%3Fv%3Di6DayghhA8Q&data=05%7C01%7Cshenwei.wang
> %40nxp.com%7C32396fd0396e4e46975f08db8f806680%7C686ea1d3bc2b4c6fa
> 92cd99c5c301635%7C0%7C0%7C638261553857503588%7CUnknown%7CTWFp
> bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> Mn0%3D%7C3000%7C%7C%7C&sdata=X5CQrQEVmUjYafYJ%2BzcnGXI9mhDT%
> 2BMzDazGHOcoomas%3D&reserved=0
>
> which might help.
>
> Will

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

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

[Shenwei Wang]

> -----Original Message-----
> From: Will Deacon <will@kernel.org>
> Sent: Friday, July 28, 2023 10:36 AM
> To: Russell King (Oracle) <linux@armlinux.org.uk>
> Cc: Andrew Halaney <ahalaney@redhat.com>; 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>; 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 v2 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 Fri, Jul 28, 2023 at 04:22:19PM +0100, Russell King (Oracle) wrote:
> > On Thu, Jul 27, 2023 at 01:36:45PM -0500, Andrew Halaney wrote:
> > > I don't have any documentation for the registers here, and as you
> > > can see I'm an amateur with respect to memory ordering based on my
> > > prior comment.
> > >
> > > But you:
> > >
> > >     1. Read intf_reg_off into variable iface
> > >     2. Write the RESET_SPEED for the appropriate mode to MAC_CTRL_REG
> > >     3. wmb() to ensure that write goes through
> >
> > I wonder about whether that wmb() is required. If the mapping is
> > device-like rather than memory-like, the write should be committed
> > before the read that regmap_update_bits() does according to the ARM
> > memory model. Maybe a bit of information about where this barrier has
> > come from would be good, and maybe getting it reviewed by the
> > arm64 barrier specialist, Will Deacon. :)
> >
> > wmb() is normally required to be paired with a rmb(), but we're not
> > talking about system memory here, so I also wonder whether wmb() is
> > the correct barrier to use.
>
> Yes, I don't think wmb() is the right thing here. If you need to ensure that the
> write to MAC_CTRL_REG has taken effect, then you'll need to go through some
> device-specific sequence which probably involves reading something back. If you
> just need things to arrive in order eventually, the memory type already gives you
> that.
>
> It's also worth pointing out that udelay() isn't necessarily ordered wrt MMIO
> writes, so that usleep_range() might need some help as well.
> Non-relaxed MMIO reads, however, _are_ ordered against a subsequent
> udelay(), so if you add the readback then this might all work out.
>

    1. Write RESET_SPEED
    2. Write 0 to MX93_GPR_ENET_QOS_INTF_MODE_MASK
    3. usleep_range()
    4. Restore MX93_GPR_ENET_QOS_CLK_GEN_EN | iface

In the above example, if a readback after step #2 could ensure sufficient time has passed
before step #4, the wmb() here should be abandoned.

Thanks,
Shenwei

> I gave a (slightly dated) talk about some of this at ELC a while back:
>
> https://www.yo/
> utube.com%2Fwatch%3Fv%3Di6DayghhA8Q&data=05%7C01%7Cshenwei.wang
> %40nxp.com%7C32396fd0396e4e46975f08db8f806680%7C686ea1d3bc2b4c6fa
> 92cd99c5c301635%7C0%7C0%7C638261553857503588%7CUnknown%7CTWFp
> bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> Mn0%3D%7C3000%7C%7C%7C&sdata=X5CQrQEVmUjYafYJ%2BzcnGXI9mhDT%
> 2BMzDazGHOcoomas%3D&reserved=0
>
> which might help.
>
> Will

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

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

[Frank Li]

On Fri, Jul 28, 2023 at 04:36:12PM +0100, Will Deacon wrote:
> On Fri, Jul 28, 2023 at 04:22:19PM +0100, Russell King (Oracle) wrote:
> > On Thu, Jul 27, 2023 at 01:36:45PM -0500, Andrew Halaney wrote:
> > > I don't have any documentation for the registers here, and as you can
> > > see I'm an amateur with respect to memory ordering based on my prior
> > > comment.
> > > 
> > > But you:
> > > 
> > >     1. Read intf_reg_off into variable iface
> > >     2. Write the RESET_SPEED for the appropriate mode to MAC_CTRL_REG
> > >     3. wmb() to ensure that write goes through
> > 
> > I wonder about whether that wmb() is required. If the mapping is
> > device-like rather than memory-like, the write should be committed
> > before the read that regmap_update_bits() does according to the ARM
> > memory model. Maybe a bit of information about where this barrier
> > has come from would be good, and maybe getting it reviewed by the
> > arm64 barrier specialist, Will Deacon. :)
> > 
> > wmb() is normally required to be paired with a rmb(), but we're not
> > talking about system memory here, so I also wonder whether wmb() is
> > the correct barrier to use.
> 
> Yes, I don't think wmb() is the right thing here. If you need to ensure
> that the write to MAC_CTRL_REG has taken effect, then you'll need to go
> through some device-specific sequence which probably involves reading
> something back. If you just need things to arrive in order eventually,
> the memory type already gives you that.
> 
> It's also worth pointing out that udelay() isn't necessarily ordered wrt
> MMIO writes, so that usleep_range() might need some help as well.

Hi Deacon:

Does it means below pattern will be problem?

1.writel()
2.udelay()
3.writel()

It may not wait enough time between 1 and 3. I think the above pattern
is quite common in driver code.  I am not sure if usleep_range involve
MMIO to get current counter, ARM may use cp15 to get local timer counter.

In our system, readl() is quite slow because cross some bus bridge.
even readl() can work, we don't know it is because delay by readl() itself.
Or it works logically. Suppose readl() and writel() just guarantee memory
access order. 

Frank

> Non-relaxed MMIO reads, however, _are_ ordered against a subsequent
> udelay(), so if you add the readback then this might all work out.
> 
> I gave a (slightly dated) talk about some of this at ELC a while back:
> 
> https://www.youtube.com/watch?v=i6DayghhA8Q
> 
> which might help.
> 
> Will

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

[Frank Li]

On Fri, Jul 28, 2023 at 04:36:12PM +0100, Will Deacon wrote:
> On Fri, Jul 28, 2023 at 04:22:19PM +0100, Russell King (Oracle) wrote:
> > On Thu, Jul 27, 2023 at 01:36:45PM -0500, Andrew Halaney wrote:
> > > I don't have any documentation for the registers here, and as you can
> > > see I'm an amateur with respect to memory ordering based on my prior
> > > comment.
> > > 
> > > But you:
> > > 
> > >     1. Read intf_reg_off into variable iface
> > >     2. Write the RESET_SPEED for the appropriate mode to MAC_CTRL_REG
> > >     3. wmb() to ensure that write goes through
> > 
> > I wonder about whether that wmb() is required. If the mapping is
> > device-like rather than memory-like, the write should be committed
> > before the read that regmap_update_bits() does according to the ARM
> > memory model. Maybe a bit of information about where this barrier
> > has come from would be good, and maybe getting it reviewed by the
> > arm64 barrier specialist, Will Deacon. :)
> > 
> > wmb() is normally required to be paired with a rmb(), but we're not
> > talking about system memory here, so I also wonder whether wmb() is
> > the correct barrier to use.
> 
> Yes, I don't think wmb() is the right thing here. If you need to ensure
> that the write to MAC_CTRL_REG has taken effect, then you'll need to go
> through some device-specific sequence which probably involves reading
> something back. If you just need things to arrive in order eventually,
> the memory type already gives you that.
> 
> It's also worth pointing out that udelay() isn't necessarily ordered wrt
> MMIO writes, so that usleep_range() might need some help as well.

Hi Deacon:

Does it means below pattern will be problem?

1.writel()
2.udelay()
3.writel()

It may not wait enough time between 1 and 3. I think the above pattern
is quite common in driver code.  I am not sure if usleep_range involve
MMIO to get current counter, ARM may use cp15 to get local timer counter.

In our system, readl() is quite slow because cross some bus bridge.
even readl() can work, we don't know it is because delay by readl() itself.
Or it works logically. Suppose readl() and writel() just guarantee memory
access order. 

Frank

> Non-relaxed MMIO reads, however, _are_ ordered against a subsequent
> udelay(), so if you add the readback then this might all work out.
> 
> I gave a (slightly dated) talk about some of this at ELC a while back:
> 
> https://www.youtube.com/watch?v=i6DayghhA8Q
> 
> which might help.
> 
> Will

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

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

[Frank Li]

On Fri, Jul 28, 2023 at 04:36:12PM +0100, Will Deacon wrote:
> On Fri, Jul 28, 2023 at 04:22:19PM +0100, Russell King (Oracle) wrote:
> > On Thu, Jul 27, 2023 at 01:36:45PM -0500, Andrew Halaney wrote:
> > > I don't have any documentation for the registers here, and as you can
> > > see I'm an amateur with respect to memory ordering based on my prior
> > > comment.
> > > 
> > > But you:
> > > 
> > >     1. Read intf_reg_off into variable iface
> > >     2. Write the RESET_SPEED for the appropriate mode to MAC_CTRL_REG
> > >     3. wmb() to ensure that write goes through
> > 
> > I wonder about whether that wmb() is required. If the mapping is
> > device-like rather than memory-like, the write should be committed
> > before the read that regmap_update_bits() does according to the ARM
> > memory model. Maybe a bit of information about where this barrier
> > has come from would be good, and maybe getting it reviewed by the
> > arm64 barrier specialist, Will Deacon. :)
> > 
> > wmb() is normally required to be paired with a rmb(), but we're not
> > talking about system memory here, so I also wonder whether wmb() is
> > the correct barrier to use.
> 
> Yes, I don't think wmb() is the right thing here. If you need to ensure
> that the write to MAC_CTRL_REG has taken effect, then you'll need to go
> through some device-specific sequence which probably involves reading
> something back. If you just need things to arrive in order eventually,
> the memory type already gives you that.
> 
> It's also worth pointing out that udelay() isn't necessarily ordered wrt
> MMIO writes, so that usleep_range() might need some help as well.

Hi Deacon:

Does it means below pattern will be problem?

1.writel()
2.udelay()
3.writel()

It may not wait enough time between 1 and 3. I think the above pattern
is quite common in driver code.  I am not sure if usleep_range involve
MMIO to get current counter, ARM may use cp15 to get local timer counter.

In our system, readl() is quite slow because cross some bus bridge.
even readl() can work, we don't know it is because delay by readl() itself.
Or it works logically. Suppose readl() and writel() just guarantee memory
access order. 

Frank

> Non-relaxed MMIO reads, however, _are_ ordered against a subsequent
> udelay(), so if you add the readback then this might all work out.
> 
> I gave a (slightly dated) talk about some of this at ELC a while back:
> 
> https://www.youtube.com/watch?v=i6DayghhA8Q
> 
> which might help.
> 
> Will

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

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

[Andrew Lunn]

> > Andrew Lunn gave the following feedback in v1:
> > 
> > "The SJA1105 has the problem, so i would expect it to be involved in the solution.
> > Otherwise, how is this going to work for other MAC drivers?
> > 
> > Maybe you need to expose a common clock framework clock for the TXC clock
> > line, which the SJA1105 can disable/enable? That then makes it clear what other
> > MAC drivers need to do."
> 
> I have been considering this plan for some time. The idea should be implemented 
> across all i.mx8/9 platforms. I am going to start to work on it in the following month, 
> and it will take some time to implement it.

So you don't think anybody will use anything else for driving this
switch? Vybrid?

It does not really matter what you implement it for, so long is at is
a clear example for others to follow who might be using the switch
with other SoCs.

	Andrew

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

[Andrew Lunn]

> > Andrew Lunn gave the following feedback in v1:
> > 
> > "The SJA1105 has the problem, so i would expect it to be involved in the solution.
> > Otherwise, how is this going to work for other MAC drivers?
> > 
> > Maybe you need to expose a common clock framework clock for the TXC clock
> > line, which the SJA1105 can disable/enable? That then makes it clear what other
> > MAC drivers need to do."
> 
> I have been considering this plan for some time. The idea should be implemented 
> across all i.mx8/9 platforms. I am going to start to work on it in the following month, 
> and it will take some time to implement it.

So you don't think anybody will use anything else for driving this
switch? Vybrid?

It does not really matter what you implement it for, so long is at is
a clear example for others to follow who might be using the switch
with other SoCs.

	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 v2 net 2/2] net: stmmac: dwmac-imx: pause the TXC clock in fixed-link

[Andrew Lunn]

> > Andrew Lunn gave the following feedback in v1:
> > 
> > "The SJA1105 has the problem, so i would expect it to be involved in the solution.
> > Otherwise, how is this going to work for other MAC drivers?
> > 
> > Maybe you need to expose a common clock framework clock for the TXC clock
> > line, which the SJA1105 can disable/enable? That then makes it clear what other
> > MAC drivers need to do."
> 
> I have been considering this plan for some time. The idea should be implemented 
> across all i.mx8/9 platforms. I am going to start to work on it in the following month, 
> and it will take some time to implement it.

So you don't think anybody will use anything else for driving this
switch? Vybrid?

It does not really matter what you implement it for, so long is at is
a clear example for others to follow who might be using the switch
with other SoCs.

	Andrew

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

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

[Shenwei Wang]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Friday, July 28, 2023 11:56 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: Fabio Estevam <festevam@gmail.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>; Giuseppe Cavallaro
> <peppe.cavallaro@st.com>; Alexandre Torgue <alexandre.torgue@foss.st.com>;
> Jose Abreu <joabreu@synopsys.com>; Pengutronix Kernel Team
> <kernel@pengutronix.de>; 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 v2 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
> 
> 
> > > Andrew Lunn gave the following feedback in v1:
> > >
> > > "The SJA1105 has the problem, so i would expect it to be involved in the
> solution.
> > > Otherwise, how is this going to work for other MAC drivers?
> > >
> > > Maybe you need to expose a common clock framework clock for the TXC
> > > clock line, which the SJA1105 can disable/enable? That then makes it
> > > clear what other MAC drivers need to do."
> >
> > I have been considering this plan for some time. The idea should be
> > implemented across all i.mx8/9 platforms. I am going to start to work
> > on it in the following month, and it will take some time to implement it.
> 
> So you don't think anybody will use anything else for driving this switch? Vybrid?
> 

Vybrid or i.MX6 don't have such kind of problem because they are not using dwmac.
Those are FEC MACs, and the MAC itself can provide delay to the switch.

Thanks,
Shenwei

> It does not really matter what you implement it for, so long is at is a clear
> example for others to follow who might be using the switch with other SoCs.
> 
>         Andrew

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

[Shenwei Wang]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Friday, July 28, 2023 11:56 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: Fabio Estevam <festevam@gmail.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>; Giuseppe Cavallaro
> <peppe.cavallaro@st.com>; Alexandre Torgue <alexandre.torgue@foss.st.com>;
> Jose Abreu <joabreu@synopsys.com>; Pengutronix Kernel Team
> <kernel@pengutronix.de>; 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 v2 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
> 
> 
> > > Andrew Lunn gave the following feedback in v1:
> > >
> > > "The SJA1105 has the problem, so i would expect it to be involved in the
> solution.
> > > Otherwise, how is this going to work for other MAC drivers?
> > >
> > > Maybe you need to expose a common clock framework clock for the TXC
> > > clock line, which the SJA1105 can disable/enable? That then makes it
> > > clear what other MAC drivers need to do."
> >
> > I have been considering this plan for some time. The idea should be
> > implemented across all i.mx8/9 platforms. I am going to start to work
> > on it in the following month, and it will take some time to implement it.
> 
> So you don't think anybody will use anything else for driving this switch? Vybrid?
> 

Vybrid or i.MX6 don't have such kind of problem because they are not using dwmac.
Those are FEC MACs, and the MAC itself can provide delay to the switch.

Thanks,
Shenwei

> It does not really matter what you implement it for, so long is at is a clear
> example for others to follow who might be using the switch with other SoCs.
> 
>         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 v2 net 2/2] net: stmmac: dwmac-imx: pause the TXC clock in fixed-link

[Shenwei Wang]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Friday, July 28, 2023 11:56 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: Fabio Estevam <festevam@gmail.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>; Giuseppe Cavallaro
> <peppe.cavallaro@st.com>; Alexandre Torgue <alexandre.torgue@foss.st.com>;
> Jose Abreu <joabreu@synopsys.com>; Pengutronix Kernel Team
> <kernel@pengutronix.de>; 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 v2 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
> 
> 
> > > Andrew Lunn gave the following feedback in v1:
> > >
> > > "The SJA1105 has the problem, so i would expect it to be involved in the
> solution.
> > > Otherwise, how is this going to work for other MAC drivers?
> > >
> > > Maybe you need to expose a common clock framework clock for the TXC
> > > clock line, which the SJA1105 can disable/enable? That then makes it
> > > clear what other MAC drivers need to do."
> >
> > I have been considering this plan for some time. The idea should be
> > implemented across all i.mx8/9 platforms. I am going to start to work
> > on it in the following month, and it will take some time to implement it.
> 
> So you don't think anybody will use anything else for driving this switch? Vybrid?
> 

Vybrid or i.MX6 don't have such kind of problem because they are not using dwmac.
Those are FEC MACs, and the MAC itself can provide delay to the switch.

Thanks,
Shenwei

> It does not really matter what you implement it for, so long is at is a clear
> example for others to follow who might be using the switch with other SoCs.
> 
>         Andrew

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

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

[Russell King (Oracle)]

On Fri, Jul 28, 2023 at 12:29:46PM -0400, Frank Li wrote:
> On Fri, Jul 28, 2023 at 04:36:12PM +0100, Will Deacon wrote:
> > Yes, I don't think wmb() is the right thing here. If you need to ensure
> > that the write to MAC_CTRL_REG has taken effect, then you'll need to go
> > through some device-specific sequence which probably involves reading
> > something back. If you just need things to arrive in order eventually,
> > the memory type already gives you that.
> > 
> > It's also worth pointing out that udelay() isn't necessarily ordered wrt
> > MMIO writes, so that usleep_range() might need some help as well.
> 
> Hi Deacon:
> 
> Does it means below pattern will be problem?
> 
> 1.writel()
> 2.udelay()
> 3.writel()

Yes, it can be a problem - because the first write may take a while
to hit the hardware. It's been this way ever since PCI became a thing,
even on x86 hardware.

PCI posting rules are that writes can be posted into the various
bridges in the bus structure and forwarded on at some point later.
However, reads are not allowed to bypass writes - which means that if
one reads from a PCI device, the preceeding writes need to be flushed
out of the bridges _in the path to the device being read_.

So, if we take an example and apply it to PCI:

	writel()
	udelay(100)
	writel()
	readl()

The device could well see nothing for a while, and then two consecutive
writes and a read in quick succession.

> It may not wait enough time between 1 and 3. I think the above pattern
> is quite common in driver code.  I am not sure if usleep_range involve
> MMIO to get current counter, ARM may use cp15 to get local timer counter.

There are no guarantees, even on x86, that udelay() offers anything to
space device writes apart.

If this pattern is popular in drivers, and it's critical to the
drivers operation, then it's technically buggy - and it's been that way
for at least a couple of decades! One might get away with it (maybe the
hardware isn't delaying the writes?) but the kernel has never
guaranteed that writel(), udelay(), writel() will space the two writes
apart by the specified delay.

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

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

[Russell King (Oracle)]

On Fri, Jul 28, 2023 at 12:29:46PM -0400, Frank Li wrote:
> On Fri, Jul 28, 2023 at 04:36:12PM +0100, Will Deacon wrote:
> > Yes, I don't think wmb() is the right thing here. If you need to ensure
> > that the write to MAC_CTRL_REG has taken effect, then you'll need to go
> > through some device-specific sequence which probably involves reading
> > something back. If you just need things to arrive in order eventually,
> > the memory type already gives you that.
> > 
> > It's also worth pointing out that udelay() isn't necessarily ordered wrt
> > MMIO writes, so that usleep_range() might need some help as well.
> 
> Hi Deacon:
> 
> Does it means below pattern will be problem?
> 
> 1.writel()
> 2.udelay()
> 3.writel()

Yes, it can be a problem - because the first write may take a while
to hit the hardware. It's been this way ever since PCI became a thing,
even on x86 hardware.

PCI posting rules are that writes can be posted into the various
bridges in the bus structure and forwarded on at some point later.
However, reads are not allowed to bypass writes - which means that if
one reads from a PCI device, the preceeding writes need to be flushed
out of the bridges _in the path to the device being read_.

So, if we take an example and apply it to PCI:

	writel()
	udelay(100)
	writel()
	readl()

The device could well see nothing for a while, and then two consecutive
writes and a read in quick succession.

> It may not wait enough time between 1 and 3. I think the above pattern
> is quite common in driver code.  I am not sure if usleep_range involve
> MMIO to get current counter, ARM may use cp15 to get local timer counter.

There are no guarantees, even on x86, that udelay() offers anything to
space device writes apart.

If this pattern is popular in drivers, and it's critical to the
drivers operation, then it's technically buggy - and it's been that way
for at least a couple of decades! One might get away with it (maybe the
hardware isn't delaying the writes?) but the kernel has never
guaranteed that writel(), udelay(), writel() will space the two writes
apart by the specified delay.

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

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

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

[Russell King (Oracle)]

On Fri, Jul 28, 2023 at 12:29:46PM -0400, Frank Li wrote:
> On Fri, Jul 28, 2023 at 04:36:12PM +0100, Will Deacon wrote:
> > Yes, I don't think wmb() is the right thing here. If you need to ensure
> > that the write to MAC_CTRL_REG has taken effect, then you'll need to go
> > through some device-specific sequence which probably involves reading
> > something back. If you just need things to arrive in order eventually,
> > the memory type already gives you that.
> > 
> > It's also worth pointing out that udelay() isn't necessarily ordered wrt
> > MMIO writes, so that usleep_range() might need some help as well.
> 
> Hi Deacon:
> 
> Does it means below pattern will be problem?
> 
> 1.writel()
> 2.udelay()
> 3.writel()

Yes, it can be a problem - because the first write may take a while
to hit the hardware. It's been this way ever since PCI became a thing,
even on x86 hardware.

PCI posting rules are that writes can be posted into the various
bridges in the bus structure and forwarded on at some point later.
However, reads are not allowed to bypass writes - which means that if
one reads from a PCI device, the preceeding writes need to be flushed
out of the bridges _in the path to the device being read_.

So, if we take an example and apply it to PCI:

	writel()
	udelay(100)
	writel()
	readl()

The device could well see nothing for a while, and then two consecutive
writes and a read in quick succession.

> It may not wait enough time between 1 and 3. I think the above pattern
> is quite common in driver code.  I am not sure if usleep_range involve
> MMIO to get current counter, ARM may use cp15 to get local timer counter.

There are no guarantees, even on x86, that udelay() offers anything to
space device writes apart.

If this pattern is popular in drivers, and it's critical to the
drivers operation, then it's technically buggy - and it's been that way
for at least a couple of decades! One might get away with it (maybe the
hardware isn't delaying the writes?) but the kernel has never
guaranteed that writel(), udelay(), writel() will space the two writes
apart by the specified delay.

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

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

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

[kernel test robot]

Hi Shenwei,

kernel test robot noticed the following build errors:

[auto build test ERROR on net/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Shenwei-Wang/net-stmmac-add-new-mode-parameter-for-fix_mac_speed/20230727-232922
base:   net/main
patch link:    https://lore.kernel.org/r/20230727152503.2199550-2-shenwei.wang%40nxp.com
patch subject: [PATCH v2 net 1/2] net: stmmac: add new mode parameter for fix_mac_speed
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20230729/202307290635.TOWvHdhK-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230729/202307290635.TOWvHdhK-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202307290635.TOWvHdhK-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c: In function 'starfive_dwmac_probe':
>> drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c:132:41: error: assignment to 'void (*)(void *, uint,  uint)' {aka 'void (*)(void *, unsigned int,  unsigned int)'} from incompatible pointer type 'void (*)(void *, unsigned int)' [-Werror=incompatible-pointer-types]
     132 |                 plat_dat->fix_mac_speed = starfive_dwmac_fix_mac_speed;
         |                                         ^
   cc1: some warnings being treated as errors
--
   drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c: In function 'sti_dwmac_probe':
>> drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c:296:33: error: assignment to 'void (*)(void *, uint,  uint)' {aka 'void (*)(void *, unsigned int,  unsigned int)'} from incompatible pointer type 'void (*)(void *, unsigned int)' [-Werror=incompatible-pointer-types]
     296 |         plat_dat->fix_mac_speed = data->fix_retime_src;
         |                                 ^
   cc1: some warnings being treated as errors
--
   drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c: In function 'tegra_eqos_probe':
>> drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c:359:29: error: assignment to 'void (*)(void *, uint,  uint)' {aka 'void (*)(void *, unsigned int,  unsigned int)'} from incompatible pointer type 'void (*)(void *, unsigned int)' [-Werror=incompatible-pointer-types]
     359 |         data->fix_mac_speed = tegra_eqos_fix_speed;
         |                             ^
   cc1: some warnings being treated as errors
--
   drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c: In function 'intel_eth_plat_probe':
>> drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c:108:49: error: assignment to 'void (*)(void *, uint,  uint)' {aka 'void (*)(void *, unsigned int,  unsigned int)'} from incompatible pointer type 'void (*)(void *, unsigned int)' [-Werror=incompatible-pointer-types]
     108 |                         plat_dat->fix_mac_speed = dwmac->data->fix_mac_speed;
         |                                                 ^
   cc1: some warnings being treated as errors


vim +132 drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c

b4a5afa51ceeca Samin Guo 2023-04-17   92  
4bd3bb7b452690 Samin Guo 2023-04-17   93  static int starfive_dwmac_probe(struct platform_device *pdev)
4bd3bb7b452690 Samin Guo 2023-04-17   94  {
4bd3bb7b452690 Samin Guo 2023-04-17   95  	struct plat_stmmacenet_data *plat_dat;
4bd3bb7b452690 Samin Guo 2023-04-17   96  	struct stmmac_resources stmmac_res;
4bd3bb7b452690 Samin Guo 2023-04-17   97  	struct starfive_dwmac *dwmac;
4bd3bb7b452690 Samin Guo 2023-04-17   98  	struct clk *clk_gtx;
4bd3bb7b452690 Samin Guo 2023-04-17   99  	int err;
4bd3bb7b452690 Samin Guo 2023-04-17  100  
4bd3bb7b452690 Samin Guo 2023-04-17  101  	err = stmmac_get_platform_resources(pdev, &stmmac_res);
4bd3bb7b452690 Samin Guo 2023-04-17  102  	if (err)
4bd3bb7b452690 Samin Guo 2023-04-17  103  		return dev_err_probe(&pdev->dev, err,
4bd3bb7b452690 Samin Guo 2023-04-17  104  				     "failed to get resources\n");
4bd3bb7b452690 Samin Guo 2023-04-17  105  
4bd3bb7b452690 Samin Guo 2023-04-17  106  	plat_dat = stmmac_probe_config_dt(pdev, stmmac_res.mac);
4bd3bb7b452690 Samin Guo 2023-04-17  107  	if (IS_ERR(plat_dat))
4bd3bb7b452690 Samin Guo 2023-04-17  108  		return dev_err_probe(&pdev->dev, PTR_ERR(plat_dat),
4bd3bb7b452690 Samin Guo 2023-04-17  109  				     "dt configuration failed\n");
4bd3bb7b452690 Samin Guo 2023-04-17  110  
4bd3bb7b452690 Samin Guo 2023-04-17  111  	dwmac = devm_kzalloc(&pdev->dev, sizeof(*dwmac), GFP_KERNEL);
4bd3bb7b452690 Samin Guo 2023-04-17  112  	if (!dwmac)
4bd3bb7b452690 Samin Guo 2023-04-17  113  		return -ENOMEM;
4bd3bb7b452690 Samin Guo 2023-04-17  114  
4bd3bb7b452690 Samin Guo 2023-04-17  115  	dwmac->clk_tx = devm_clk_get_enabled(&pdev->dev, "tx");
4bd3bb7b452690 Samin Guo 2023-04-17  116  	if (IS_ERR(dwmac->clk_tx))
4bd3bb7b452690 Samin Guo 2023-04-17  117  		return dev_err_probe(&pdev->dev, PTR_ERR(dwmac->clk_tx),
4bd3bb7b452690 Samin Guo 2023-04-17  118  				     "error getting tx clock\n");
4bd3bb7b452690 Samin Guo 2023-04-17  119  
4bd3bb7b452690 Samin Guo 2023-04-17  120  	clk_gtx = devm_clk_get_enabled(&pdev->dev, "gtx");
4bd3bb7b452690 Samin Guo 2023-04-17  121  	if (IS_ERR(clk_gtx))
4bd3bb7b452690 Samin Guo 2023-04-17  122  		return dev_err_probe(&pdev->dev, PTR_ERR(clk_gtx),
4bd3bb7b452690 Samin Guo 2023-04-17  123  				     "error getting gtx clock\n");
4bd3bb7b452690 Samin Guo 2023-04-17  124  
4bd3bb7b452690 Samin Guo 2023-04-17  125  	/* Generally, the rgmii_tx clock is provided by the internal clock,
4bd3bb7b452690 Samin Guo 2023-04-17  126  	 * which needs to match the corresponding clock frequency according
4bd3bb7b452690 Samin Guo 2023-04-17  127  	 * to different speeds. If the rgmii_tx clock is provided by the
4bd3bb7b452690 Samin Guo 2023-04-17  128  	 * external rgmii_rxin, there is no need to configure the clock
4bd3bb7b452690 Samin Guo 2023-04-17  129  	 * internally, because rgmii_rxin will be adaptively adjusted.
4bd3bb7b452690 Samin Guo 2023-04-17  130  	 */
4bd3bb7b452690 Samin Guo 2023-04-17  131  	if (!device_property_read_bool(&pdev->dev, "starfive,tx-use-rgmii-clk"))
4bd3bb7b452690 Samin Guo 2023-04-17 @132  		plat_dat->fix_mac_speed = starfive_dwmac_fix_mac_speed;
4bd3bb7b452690 Samin Guo 2023-04-17  133  
4bd3bb7b452690 Samin Guo 2023-04-17  134  	dwmac->dev = &pdev->dev;
4bd3bb7b452690 Samin Guo 2023-04-17  135  	plat_dat->bsp_priv = dwmac;
4bd3bb7b452690 Samin Guo 2023-04-17  136  	plat_dat->dma_cfg->dche = true;
4bd3bb7b452690 Samin Guo 2023-04-17  137  
b4a5afa51ceeca Samin Guo 2023-04-17  138  	err = starfive_dwmac_set_mode(plat_dat);
b4a5afa51ceeca Samin Guo 2023-04-17  139  	if (err)
b4a5afa51ceeca Samin Guo 2023-04-17  140  		return err;
b4a5afa51ceeca Samin Guo 2023-04-17  141  
4bd3bb7b452690 Samin Guo 2023-04-17  142  	err = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
4bd3bb7b452690 Samin Guo 2023-04-17  143  	if (err) {
4bd3bb7b452690 Samin Guo 2023-04-17  144  		stmmac_remove_config_dt(pdev, plat_dat);
4bd3bb7b452690 Samin Guo 2023-04-17  145  		return err;
4bd3bb7b452690 Samin Guo 2023-04-17  146  	}
4bd3bb7b452690 Samin Guo 2023-04-17  147  
4bd3bb7b452690 Samin Guo 2023-04-17  148  	return 0;
4bd3bb7b452690 Samin Guo 2023-04-17  149  }
4bd3bb7b452690 Samin Guo 2023-04-17  150  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki