linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 00/20] net: stmmac: Obvious cleanups and several fixes
@ 2021-02-08 14:03 Serge Semin
  2021-02-08 14:03 ` [PATCH 01/20] net: phy: realtek: Fix events detection failure in LPI mode Serge Semin
                   ` (19 more replies)
  0 siblings, 20 replies; 37+ messages in thread
From: Serge Semin @ 2021-02-08 14:03 UTC (permalink / raw)
  To: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S. Miller, Jakub Kicinski, Joao Pinto, Jose Abreu
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Pavel Parkhomenko,
	Vyacheslav Mitrofanov, Maxime Coquelin, Russell King,
	Andrew Lunn, Heiner Kallweit, netdev, linux-stm32,
	linux-arm-kernel, linux-kernel

This series consists of a preparation patches before adding DW MAC GPIOs
and final Baikal-T1 GMAC support. (The later will be done in the framework
of the Generic DW MAC glue-driver though.) It's mainly about cleaning the
code up here and there by removing unused data and macro names, but also
includes several bugs and design fixes.

The patchset starts from fixing the Realtek PHYs driver. In particular it
has been discovered that disabling RXC in LPI (EEE) causes RTL8211E PHY
partial freeze until the next MDIO read operation from the PHY CSRs. We
suggest to fix that problem by dummy reading from the MMD Data register
each time the PC1R.10 bit is intended to be set.

Then the series evolves in a set of bug fixes discovered in the main
STMMAC driver code. First of all the cleanup-on-error path has been
incorrectly implemented in the DMA descriptor allocation procedure due to
which in case Tx DMA resources allocation failures the Rx DMA descriptors
will be left unfreed. Secondly it has been discovered that the MTL IRQs
handling procedure didn't do that quite well, so any MTL RX overflow
errors will be handled for queues with higher order too, which most likely
isn't what the code author originally intended. Thirdly the DW MAC reset
control de-assetion should be performed after the MDIO-bus
de-registration, because the later may need to access PHY registers, which
is supposed to be done via the MAC SMA interface. Fourthly we've found out
that DW MAC v4.x code was using a generic dwmac4_disable_dma_irq() method
to disable DMA IRQs instead of having the dedicated
dwmac410_disable_dma_irq() method utilized. That didn't cause any problem
because the modified bits matches in both IP-core revisions, but for
consistency we suggest to fix that. Fifthly for the same reason of the
naming consistency the GMAC_INT_STATUS_PMT macro constant should be used
instead of GMAC_INT_DISABLE_PMT to check the PMT IRQs status. Finally it's
strange that the problem hasn't been discovered before, but it is most
likely wrong to initialized Tx/Rx DMA descriptors, and then clean them up.
That specifically concerns the Tx DMA descriptors initialization procedure
in the Chain-mode. Please the patch for details.

The patchset then proceed with multiple optimizations and cleanups
performed here and there in the code: fix typo in the XGMAC_L3_ADDR3 macro
name, discard unused mii_irq array from the private data, discard nothing
changing Rx copybreak ethtool setting, discard redundant index variable
usage in the dirty_rx initialization method, discard dwmac1000_dma_ops
declaration from dwmac100.h, move DMA Tx/Rx init methods to the DW MAC lib
since they match for DW MAC and DW GMA IP-cores, discard pointless
STMMAC_RESETING flag, discard conditional service task execution since
it's called from CMWQ anyway (it's also errors prone, since any event
happening during the service task execution will be lost), add 'cause' arg
to the service task executioner to generalize the deferred events handling
interface. Finally in the framework of the code cleanup procedure we
suggest to extend the stmmac_hw_teardown() functionality with all the
necessary hardware cleanups, which for some reason were directly performed
in the network device release callback. That concerns PTP clocks
disabling, DMA channels and MAC Tx/Rx de-activation.

Note the STMMAC driver is having much more weak design patterns and style
problems (like calculating the total number of queues every time it's
needed, or antagonist/cleanup methods absence while having the reversal
code added in the remove/cleanup paths), than what is fixed in the
framework of this series, which make the code hard to read, comprehend,
maintain and extend with new features. Most likely the situation turned to
be like that due to a long history of the driver evolving to support many
different IP-core versions and vendor-specific MAC extensions. Anyway it
would have taken not a single patches series to fix all of the problems.
Since it hasn't been my primary target, here in this series I've
introduced the cleanups and fixes, which prepared the corresponding parts
of the code for easier alterations in the framework of adding the DW MAC
GPIOs and Baikal-T1 GMAC support into the driver.

The series is supposed to be applied on top of the last revision of the
next patchset:
Link: https://lore.kernel.org/netdev/20201214091616.13545-1-Sergey.Semin@baikalelectronics.ru/
otherwise a few patches won't get merged in cleanly.

Fixes: 7bac4e1ec3ca ("net: stmmac: stmmac interrupt treatment prepared for multiple queues")
Fixes: 021bd5e36970 ("net: stmmac: Let TX and RX interrupts be independently enabled/disabled")
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
Cc: Vyacheslav Mitrofanov <Vyacheslav.Mitrofanov@baikalelectronics.ru>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: netdev@vger.kernel.org
Cc: linux-stm32@st-md-mailman.stormreply.com
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org

Serge Semin (20):
  net: phy: realtek: Fix events detection failure in LPI mode
  net: stmmac: Free Rx descs on Tx allocation failure
  net: stmmac: Fix false MTL RX overflow handling for higher queues
  net: stmmac: Assert reset control after MDIO de-registration
  net: stmmac: Use dwmac410_disable_dma_irq for DW MAC v4.10 DMA
  net: stmmac: Use LPI IRQ status-related macro in DW MAC1000 isr
  net: stmmac: Clear descriptors before initializing them
  net: stmmac: Fix typo in the XGMAC_L3_ADDR3 macro name
  net: stmmac: Discard mii_irq array from private data
  net: stmmac: Discard Rx copybreak ethtool setting
  net: stmmac: Discard index usage in the dirty_rx init
  net: stmmac: Discard dwmac1000_dma_ops declaration from dwmac100.h
  net: stmmac: Move DMA Tx/Rx init methods to DW MAC lib
  net: stmmac: Add DW GMAC disable LPI IRQ mask macro
  net: stmmac: Discard STMMAC_RESETING flag
  net: stmmac: Discard conditional service task execution
  net: stmmac: Add 'cause' arg to the service task executioner
  net: stmmac: Move PTP clock enabling to PTP-init method
  net: stmmac: Move DMA stop procedure to HW-setup antagonist
  net: stmmac: Move MAC Tx/Rx disabling to HW-setup antagonist

 .../net/ethernet/stmicro/stmmac/dwmac1000.h   |  2 +-
 .../ethernet/stmicro/stmmac/dwmac1000_core.c  |  2 +-
 .../ethernet/stmicro/stmmac/dwmac1000_dma.c   | 20 +----
 .../ethernet/stmicro/stmmac/dwmac100_dma.c    | 20 +----
 .../net/ethernet/stmicro/stmmac/dwmac4_dma.c  |  2 +-
 .../net/ethernet/stmicro/stmmac/dwmac_dma.h   |  4 +
 .../net/ethernet/stmicro/stmmac/dwmac_lib.c   | 14 ++++
 .../net/ethernet/stmicro/stmmac/dwxgmac2.h    |  2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac.h  |  5 --
 .../ethernet/stmicro/stmmac/stmmac_ethtool.c  | 39 ---------
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 81 +++++++++----------
 drivers/net/phy/realtek.c                     | 37 +++++++++
 12 files changed, 102 insertions(+), 126 deletions(-)

-- 
2.29.2


^ permalink raw reply	[flat|nested] 37+ messages in thread

* [PATCH 01/20] net: phy: realtek: Fix events detection failure in LPI mode
  2021-02-08 14:03 [PATCH 00/20] net: stmmac: Obvious cleanups and several fixes Serge Semin
@ 2021-02-08 14:03 ` Serge Semin
  2021-02-08 15:27   ` Andrew Lunn
  2021-02-08 20:14   ` Heiner Kallweit
  2021-02-08 14:03 ` [PATCH 02/20] net: stmmac: Free Rx descs on Tx allocation failure Serge Semin
                   ` (18 subsequent siblings)
  19 siblings, 2 replies; 37+ messages in thread
From: Serge Semin @ 2021-02-08 14:03 UTC (permalink / raw)
  To: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S. Miller, Jakub Kicinski, Joao Pinto, Jose Abreu,
	Andrew Lunn, Heiner Kallweit, Russell King
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Pavel Parkhomenko,
	Vyacheslav Mitrofanov, Maxime Coquelin, netdev, linux-stm32,
	linux-arm-kernel, linux-kernel

It has been noticed that RTL8211E PHY stops detecting and reporting events
when EEE is successfully advertised and RXC stopping in LPI is enabled.
The freeze happens right after 3.0.10 bit (PC1R "Clock Stop Enable"
register) is set. At the same time LED2 stops blinking as if EEE mode has
been disabled. Notably the network traffic still flows through the PHY
with no obvious problem. Anyway if any MDIO read procedure is performed
after the "RXC stop in LPI" mode is enabled PHY gets to be unfrozen, LED2
starts blinking and PHY interrupts happens again. The problem has been
noticed on RTL8211E PHY working together with DW GMAC 3.73a MAC and
reporting its event via a dedicated IRQ signal. (Obviously the problem has
been unnoticed in the polling mode, since it gets naturally fixed by the
periodic MDIO read procedure from the PHY status register - BMSR.)

In order to fix that problem we suggest to locally re-implement the MMD
write method for RTL8211E PHY and perform a dummy read right after the
PC1R register is accessed to enable the RXC stopping in LPI mode.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/net/phy/realtek.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 99ecd6c4c15a..cbb86c257aae 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -559,6 +559,42 @@ static int rtl822x_write_mmd(struct phy_device *phydev, int devnum, u16 regnum,
 	return ret;
 }
 
+static int rtl8211e_write_mmd(struct phy_device *phydev, int devnum, u16 regnum,
+			      u16 val)
+{
+	int ret;
+
+	/* Write to the MMD registers by using the standard control/data pair.
+	 * The only difference is that we need to perform a dummy read after
+	 * the PC1R.CLKSTOP_EN bit is set. It's required to workaround an issue
+	 * of a partial core freeze so LED2 stops blinking in EEE mode, PHY
+	 * stops detecting the link change and raising IRQs until any read from
+	 * its registers performed. That happens only if and right after the PHY
+	 * is enabled to stop RXC in LPI mode.
+	 */
+	ret = __phy_write(phydev, MII_MMD_CTRL, devnum);
+	if (ret)
+		return ret;
+
+	ret = __phy_write(phydev, MII_MMD_DATA, regnum);
+	if (ret)
+		return ret;
+
+	ret = __phy_write(phydev, MII_MMD_CTRL, devnum | MII_MMD_CTRL_NOINCR);
+	if (ret)
+		return ret;
+
+	ret = __phy_write(phydev, MII_MMD_DATA, val);
+	if (ret)
+		return ret;
+
+	if (devnum == MDIO_MMD_PCS && regnum == MDIO_CTRL1 &&
+	    val & MDIO_PCS_CTRL1_CLKSTOP_EN)
+		ret =  __phy_read(phydev, MII_MMD_DATA);
+
+	return ret < 0 ? ret : 0;
+}
+
 static int rtl822x_get_features(struct phy_device *phydev)
 {
 	int val;
@@ -725,6 +761,7 @@ static struct phy_driver realtek_drvs[] = {
 		.resume		= genphy_resume,
 		.read_page	= rtl821x_read_page,
 		.write_page	= rtl821x_write_page,
+		.write_mmd	= rtl8211e_write_mmd,
 	}, {
 		PHY_ID_MATCH_EXACT(0x001cc916),
 		.name		= "RTL8211F Gigabit Ethernet",
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH 02/20] net: stmmac: Free Rx descs on Tx allocation failure
  2021-02-08 14:03 [PATCH 00/20] net: stmmac: Obvious cleanups and several fixes Serge Semin
  2021-02-08 14:03 ` [PATCH 01/20] net: phy: realtek: Fix events detection failure in LPI mode Serge Semin
@ 2021-02-08 14:03 ` Serge Semin
  2021-02-08 14:03 ` [PATCH 03/20] net: stmmac: Fix false MTL RX overflow handling for higher queues Serge Semin
                   ` (17 subsequent siblings)
  19 siblings, 0 replies; 37+ messages in thread
From: Serge Semin @ 2021-02-08 14:03 UTC (permalink / raw)
  To: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S. Miller, Jakub Kicinski, Joao Pinto, Jose Abreu,
	Maxime Coquelin
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Pavel Parkhomenko,
	Vyacheslav Mitrofanov, Russell King, Andrew Lunn,
	Heiner Kallweit, netdev, linux-stm32, linux-arm-kernel,
	linux-kernel

Indeed in accordance with the alloc_dma_desc_resources() method logic the
Rx descriptors will be left allocated if Tx descriptors allocation fails.
Fix it by calling the free_dma_rx_desc_resources() in case if the
alloc_dma_tx_desc_resources() method returns non-zero value.

While at it refactor the method a bit. Just move the Rx descriptors
allocation method invocation out of the local variables declaration block
and discard a pointless comment from there.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 03acf14d76de..5ee840525824 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1791,13 +1791,15 @@ static int alloc_dma_tx_desc_resources(struct stmmac_priv *priv)
  */
 static int alloc_dma_desc_resources(struct stmmac_priv *priv)
 {
-	/* RX Allocation */
-	int ret = alloc_dma_rx_desc_resources(priv);
+	int ret;
 
+	ret = alloc_dma_rx_desc_resources(priv);
 	if (ret)
 		return ret;
 
 	ret = alloc_dma_tx_desc_resources(priv);
+	if (ret)
+		free_dma_rx_desc_resources(priv);
 
 	return ret;
 }
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH 03/20] net: stmmac: Fix false MTL RX overflow handling for higher queues
  2021-02-08 14:03 [PATCH 00/20] net: stmmac: Obvious cleanups and several fixes Serge Semin
  2021-02-08 14:03 ` [PATCH 01/20] net: phy: realtek: Fix events detection failure in LPI mode Serge Semin
  2021-02-08 14:03 ` [PATCH 02/20] net: stmmac: Free Rx descs on Tx allocation failure Serge Semin
@ 2021-02-08 14:03 ` Serge Semin
  2021-02-08 14:03 ` [PATCH 04/20] net: stmmac: Assert reset control after MDIO de-registration Serge Semin
                   ` (16 subsequent siblings)
  19 siblings, 0 replies; 37+ messages in thread
From: Serge Semin @ 2021-02-08 14:03 UTC (permalink / raw)
  To: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S. Miller, Jakub Kicinski, Joao Pinto, Jose Abreu,
	Maxime Coquelin
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Pavel Parkhomenko,
	Vyacheslav Mitrofanov, Russell King, Andrew Lunn,
	Heiner Kallweit, netdev, linux-stm32, linux-arm-kernel,
	linux-kernel

Judging by the MAC/MTL-related part of the ISR implementation if MTL IRQs
status handler returns MTL Rx overflow bit set, the
stmmac_set_rx_tail_ptr() method will be called for all subsequent queues.
That most likely isn't what we want. Fix it by just overriding the status
variable on each loop iteration. Note we can freely break the loop at the
very beginning if the stmmac_host_mtl_irq_status() method returns -EINVAL,
because that error means the MTL IRQ status handler isn't available for
the detected hardware.

Fixes: 7bac4e1ec3ca ("net: stmmac: stmmac interrupt treatment prepared for multiple queues")
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

---

Folks, I haven't seen an effect of that bug. The patch has been created
purely based on the code visual perception. If you think the handler is
supposed to work like that and I am missing something (though I have much
doubt about that), just drop this patch.
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 5ee840525824..d45af1ea2565 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -4149,7 +4149,6 @@ static irqreturn_t stmmac_interrupt(int irq, void *dev_id)
 	/* To handle GMAC own interrupts */
 	if ((priv->plat->has_gmac) || xmac) {
 		int status = stmmac_host_irq_status(priv, priv->hw, &priv->xstats);
-		int mtl_status;
 
 		if (unlikely(status)) {
 			/* For LPI we need to save the tx status */
@@ -4162,10 +4161,10 @@ static irqreturn_t stmmac_interrupt(int irq, void *dev_id)
 		for (queue = 0; queue < queues_count; queue++) {
 			struct stmmac_rx_queue *rx_q = &priv->rx_queue[queue];
 
-			mtl_status = stmmac_host_mtl_irq_status(priv, priv->hw,
-								queue);
-			if (mtl_status != -EINVAL)
-				status |= mtl_status;
+			status = stmmac_host_mtl_irq_status(priv, priv->hw,
+							    queue);
+			if (status == -EINVAL)
+				break;
 
 			if (status & CORE_IRQ_MTL_RX_OVERFLOW)
 				stmmac_set_rx_tail_ptr(priv, priv->ioaddr,
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH 04/20] net: stmmac: Assert reset control after MDIO de-registration
  2021-02-08 14:03 [PATCH 00/20] net: stmmac: Obvious cleanups and several fixes Serge Semin
                   ` (2 preceding siblings ...)
  2021-02-08 14:03 ` [PATCH 03/20] net: stmmac: Fix false MTL RX overflow handling for higher queues Serge Semin
@ 2021-02-08 14:03 ` Serge Semin
  2021-02-08 14:03 ` [PATCH 05/20] net: stmmac: Use dwmac410_disable_dma_irq for DW MAC v4.10 DMA Serge Semin
                   ` (15 subsequent siblings)
  19 siblings, 0 replies; 37+ messages in thread
From: Serge Semin @ 2021-02-08 14:03 UTC (permalink / raw)
  To: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S. Miller, Jakub Kicinski, Joao Pinto, Jose Abreu,
	Maxime Coquelin
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Pavel Parkhomenko,
	Vyacheslav Mitrofanov, Russell King, Andrew Lunn,
	Heiner Kallweit, netdev, linux-stm32, linux-arm-kernel,
	linux-kernel

Indeed it's unlikely but MDIO de-registration may still require an access
to the core registers, which obviously won't be possible in case if the
interface has been put into the reset state. So move the reset control
assertion to be executed after the MDIO bus is de-registered.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index d45af1ea2565..1c40dc26fbf7 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -5159,10 +5159,10 @@ int stmmac_dvr_remove(struct device *dev)
 	stmmac_exit_fs(ndev);
 #endif
 	phylink_destroy(priv->phylink);
-	reset_control_assert(priv->plat->stmmac_rst);
 	if (priv->hw->pcs != STMMAC_PCS_TBI &&
 	    priv->hw->pcs != STMMAC_PCS_RTBI)
 		stmmac_mdio_unregister(ndev);
+	reset_control_assert(priv->plat->stmmac_rst);
 	destroy_workqueue(priv->wq);
 	mutex_destroy(&priv->lock);
 
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH 05/20] net: stmmac: Use dwmac410_disable_dma_irq for DW MAC v4.10 DMA
  2021-02-08 14:03 [PATCH 00/20] net: stmmac: Obvious cleanups and several fixes Serge Semin
                   ` (3 preceding siblings ...)
  2021-02-08 14:03 ` [PATCH 04/20] net: stmmac: Assert reset control after MDIO de-registration Serge Semin
@ 2021-02-08 14:03 ` Serge Semin
  2021-02-08 14:03 ` [PATCH 06/20] net: stmmac: Use LPI IRQ status-related macro in DW MAC1000 isr Serge Semin
                   ` (14 subsequent siblings)
  19 siblings, 0 replies; 37+ messages in thread
From: Serge Semin @ 2021-02-08 14:03 UTC (permalink / raw)
  To: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S. Miller, Jakub Kicinski, Joao Pinto, Jose Abreu,
	Maxime Coquelin
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Pavel Parkhomenko,
	Vyacheslav Mitrofanov, Russell King, Andrew Lunn,
	Heiner Kallweit, netdev, linux-stm32, linux-arm-kernel,
	linux-kernel

From the very beginning of the DW GMAC v4.10 IP support the driver has used
an invalid DMA IRQ disable method to switch the DMA IRQs off. Since
commit 021bd5e36970 ("net: stmmac: Let TX and RX interrupts be
independently enabled/disabled") a valid method has been added to the
dwmac4_lib.c module, but the commit author forgot to initialize the
corresponding field of the DW MAC DMA operations descriptor with it. That
mistake hasn't caused any problem so far just because the RIE/TIE fields
match in both 4.x and 4.10 IPs. Anyway fix the inconsistency in order to
at least have a coherent driver code.

Fixes: 021bd5e36970 ("net: stmmac: Let TX and RX interrupts be independently enabled/disabled")
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
index bb29bfcd62c3..59da9ff36a43 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
@@ -531,7 +531,7 @@ const struct stmmac_dma_ops dwmac410_dma_ops = {
 	.dma_rx_mode = dwmac4_dma_rx_chan_op_mode,
 	.dma_tx_mode = dwmac4_dma_tx_chan_op_mode,
 	.enable_dma_irq = dwmac410_enable_dma_irq,
-	.disable_dma_irq = dwmac4_disable_dma_irq,
+	.disable_dma_irq = dwmac410_disable_dma_irq,
 	.start_tx = dwmac4_dma_start_tx,
 	.stop_tx = dwmac4_dma_stop_tx,
 	.start_rx = dwmac4_dma_start_rx,
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH 06/20] net: stmmac: Use LPI IRQ status-related macro in DW MAC1000 isr
  2021-02-08 14:03 [PATCH 00/20] net: stmmac: Obvious cleanups and several fixes Serge Semin
                   ` (4 preceding siblings ...)
  2021-02-08 14:03 ` [PATCH 05/20] net: stmmac: Use dwmac410_disable_dma_irq for DW MAC v4.10 DMA Serge Semin
@ 2021-02-08 14:03 ` Serge Semin
  2021-02-08 14:03 ` [PATCH 07/20] net: stmmac: Clear descriptors before initializing them Serge Semin
                   ` (13 subsequent siblings)
  19 siblings, 0 replies; 37+ messages in thread
From: Serge Semin @ 2021-02-08 14:03 UTC (permalink / raw)
  To: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S. Miller, Jakub Kicinski, Joao Pinto, Jose Abreu,
	Maxime Coquelin
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Pavel Parkhomenko,
	Vyacheslav Mitrofanov, Russell King, Andrew Lunn,
	Heiner Kallweit, netdev, linux-stm32, linux-arm-kernel,
	linux-kernel

For some reason the DW MAC1000-specific IRQ status handler has been using
the GMAC_INT_DISABLE_PMT macro to test whether the PMT IRQ is pending in
the MAC status register while there is a dedicated macro
GMAC_INT_STATUS_PMT exists for the corresponding field to test. It didn't
cause any error because the bits position match in both DW MAC IRQ mask
and status registers, but semantically the code still doesn't look
correct. Let's fix that by using the correct macro there.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
index fc8759f146c7..6b9a4f54b93c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
@@ -321,7 +321,7 @@ static int dwmac1000_irq_status(struct mac_device_info *hw,
 		x->mmc_rx_irq_n++;
 	if (unlikely(intr_status & GMAC_INT_STATUS_MMCCSUM))
 		x->mmc_rx_csum_offload_irq_n++;
-	if (unlikely(intr_status & GMAC_INT_DISABLE_PMT)) {
+	if (unlikely(intr_status & GMAC_INT_STATUS_PMT)) {
 		/* clear the PMT bits 5 and 6 by reading the PMT status reg */
 		readl(ioaddr + GMAC_PMT);
 		x->irq_receive_pmt_irq_n++;
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH 07/20] net: stmmac: Clear descriptors before initializing them
  2021-02-08 14:03 [PATCH 00/20] net: stmmac: Obvious cleanups and several fixes Serge Semin
                   ` (5 preceding siblings ...)
  2021-02-08 14:03 ` [PATCH 06/20] net: stmmac: Use LPI IRQ status-related macro in DW MAC1000 isr Serge Semin
@ 2021-02-08 14:03 ` Serge Semin
  2021-02-08 14:03 ` [PATCH 08/20] net: stmmac: Fix typo in the XGMAC_L3_ADDR3 macro name Serge Semin
                   ` (12 subsequent siblings)
  19 siblings, 0 replies; 37+ messages in thread
From: Serge Semin @ 2021-02-08 14:03 UTC (permalink / raw)
  To: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S. Miller, Jakub Kicinski, Joao Pinto, Jose Abreu,
	Maxime Coquelin
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Pavel Parkhomenko,
	Vyacheslav Mitrofanov, Russell King, Andrew Lunn,
	Heiner Kallweit, netdev, linux-stm32, linux-arm-kernel,
	linux-kernel

According to the methods naming and partly based on their semantics the
descriptors need to be cleared first, then they can be properly
initialized. That specifically concerns the Tx descriptors and the chain
mode. Moreover doing the Rx-descriptors clearance twice is redundant. Fix
all of that by discarding the Rx descriptor clearance from the
init_dma_rx_desc_rings() method and move the generic method of all
descriptors clearance to the head of the init_dma_desc_rings() function.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 1c40dc26fbf7..3bc07f5b64e1 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1399,8 +1399,6 @@ static int init_dma_rx_desc_rings(struct net_device *dev, gfp_t flags)
 			  "(%s) dma_rx_phy=0x%08x\n", __func__,
 			  (u32)rx_q->dma_rx_phy);
 
-		stmmac_clear_rx_descriptors(priv, queue);
-
 		for (i = 0; i < priv->dma_rx_size; i++) {
 			struct dma_desc *p;
 
@@ -1522,14 +1520,14 @@ static int init_dma_desc_rings(struct net_device *dev, gfp_t flags)
 	struct stmmac_priv *priv = netdev_priv(dev);
 	int ret;
 
+	stmmac_clear_descriptors(priv);
+
 	ret = init_dma_rx_desc_rings(dev, flags);
 	if (ret)
 		return ret;
 
 	ret = init_dma_tx_desc_rings(dev);
 
-	stmmac_clear_descriptors(priv);
-
 	if (netif_msg_hw(priv))
 		stmmac_display_rings(priv);
 
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH 08/20] net: stmmac: Fix typo in the XGMAC_L3_ADDR3 macro name
  2021-02-08 14:03 [PATCH 00/20] net: stmmac: Obvious cleanups and several fixes Serge Semin
                   ` (6 preceding siblings ...)
  2021-02-08 14:03 ` [PATCH 07/20] net: stmmac: Clear descriptors before initializing them Serge Semin
@ 2021-02-08 14:03 ` Serge Semin
  2021-02-08 14:03 ` [PATCH 09/20] net: stmmac: Discard mii_irq array from private data Serge Semin
                   ` (11 subsequent siblings)
  19 siblings, 0 replies; 37+ messages in thread
From: Serge Semin @ 2021-02-08 14:03 UTC (permalink / raw)
  To: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S. Miller, Jakub Kicinski, Joao Pinto, Jose Abreu,
	Maxime Coquelin
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Pavel Parkhomenko,
	Vyacheslav Mitrofanov, Russell King, Andrew Lunn,
	Heiner Kallweit, netdev, linux-stm32, linux-arm-kernel,
	linux-kernel

The macro has been declared as XMGAC_L3_ADDR3 with obvious second and
third chars confused. Revert them then.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
index 6c3b8a950f58..2e6f60633128 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
@@ -191,7 +191,7 @@
 #define XGMAC_L3_ADDR0			0x4
 #define XGMAC_L3_ADDR1			0x5
 #define XGMAC_L3_ADDR2			0x6
-#define XMGAC_L3_ADDR3			0x7
+#define XGMAC_L3_ADDR3			0x7
 #define XGMAC_ARP_ADDR			0x00000c10
 #define XGMAC_RSS_CTRL			0x00000c80
 #define XGMAC_UDP4TE			BIT(3)
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH 09/20] net: stmmac: Discard mii_irq array from private data
  2021-02-08 14:03 [PATCH 00/20] net: stmmac: Obvious cleanups and several fixes Serge Semin
                   ` (7 preceding siblings ...)
  2021-02-08 14:03 ` [PATCH 08/20] net: stmmac: Fix typo in the XGMAC_L3_ADDR3 macro name Serge Semin
@ 2021-02-08 14:03 ` Serge Semin
  2021-02-08 19:13   ` Andrew Lunn
  2021-02-08 14:03 ` [PATCH 10/20] net: stmmac: Discard Rx copybreak ethtool setting Serge Semin
                   ` (10 subsequent siblings)
  19 siblings, 1 reply; 37+ messages in thread
From: Serge Semin @ 2021-02-08 14:03 UTC (permalink / raw)
  To: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S. Miller, Jakub Kicinski, Joao Pinto, Jose Abreu,
	Maxime Coquelin
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Pavel Parkhomenko,
	Vyacheslav Mitrofanov, Russell King, Andrew Lunn,
	Heiner Kallweit, netdev, linux-stm32, linux-arm-kernel,
	linux-kernel

There has been no user of the denoted array of the device private data
since commit e7f4dc3536a4 ("mdio: Move allocation of interrupts into
core"). Discard it then.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index e553b9a1f785..af02d369e641 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -185,7 +185,6 @@ struct stmmac_priv {
 	unsigned int flow_ctrl;
 	unsigned int pause;
 	struct mii_bus *mii;
-	int mii_irq[PHY_MAX_ADDR];
 
 	struct phylink_config phylink_config;
 	struct phylink *phylink;
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH 10/20] net: stmmac: Discard Rx copybreak ethtool setting
  2021-02-08 14:03 [PATCH 00/20] net: stmmac: Obvious cleanups and several fixes Serge Semin
                   ` (8 preceding siblings ...)
  2021-02-08 14:03 ` [PATCH 09/20] net: stmmac: Discard mii_irq array from private data Serge Semin
@ 2021-02-08 14:03 ` Serge Semin
  2021-02-08 14:03 ` [PATCH 11/20] net: stmmac: Discard index usage in the dirty_rx init Serge Semin
                   ` (9 subsequent siblings)
  19 siblings, 0 replies; 37+ messages in thread
From: Serge Semin @ 2021-02-08 14:03 UTC (permalink / raw)
  To: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S. Miller, Jakub Kicinski, Joao Pinto, Jose Abreu,
	Maxime Coquelin
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Pavel Parkhomenko,
	Vyacheslav Mitrofanov, Russell King, Andrew Lunn,
	Heiner Kallweit, netdev, linux-stm32, linux-arm-kernel,
	linux-kernel

Since commit 2af6106ae949 ("net: stmmac: Introducing support for Page
Pool") the mapping and unmapping has been replaced with Pages Pool usage.
The ethtool-tunable config like Rx copy-break is no longer used for
SK-buffers setup. So the ethtool tunable callback since setting/getting the
ETHTOOL_RX_COPYBREAK id doesn't really change anything. Just discard
the ethtool tunable callback then together with the rx_copybreak private
data field.

The same concerns the "rx_zeroc_thresh" member of the device private data,
but the main user has already been removed in commit 2af6106ae949
("net: stmmac: Introducing support for Page Pool") and in
commit d66e67bd4cc7 ("net: stmmac: Remove unused inline function
stmmac_rx_threshold_count").

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h  |  2 -
 .../ethernet/stmicro/stmmac/stmmac_ethtool.c  | 39 -------------------
 .../net/ethernet/stmicro/stmmac/stmmac_main.c |  4 --
 3 files changed, 45 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index af02d369e641..e444b1b237c0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -79,7 +79,6 @@ struct stmmac_rx_queue {
 	struct dma_desc *dma_rx ____cacheline_aligned_in_smp;
 	unsigned int cur_rx;
 	unsigned int dirty_rx;
-	u32 rx_zeroc_thresh;
 	dma_addr_t dma_rx_phy;
 	u32 rx_tail_addr;
 	unsigned int state_saved;
@@ -159,7 +158,6 @@ struct stmmac_priv {
 	u32 sarc_type;
 
 	unsigned int dma_buf_sz;
-	unsigned int rx_copybreak;
 	u32 rx_riwt;
 	int hwts_rx_en;
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index 9e54f953634b..0ed287edbc2d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -929,43 +929,6 @@ static int stmmac_get_ts_info(struct net_device *dev,
 		return ethtool_op_get_ts_info(dev, info);
 }
 
-static int stmmac_get_tunable(struct net_device *dev,
-			      const struct ethtool_tunable *tuna, void *data)
-{
-	struct stmmac_priv *priv = netdev_priv(dev);
-	int ret = 0;
-
-	switch (tuna->id) {
-	case ETHTOOL_RX_COPYBREAK:
-		*(u32 *)data = priv->rx_copybreak;
-		break;
-	default:
-		ret = -EINVAL;
-		break;
-	}
-
-	return ret;
-}
-
-static int stmmac_set_tunable(struct net_device *dev,
-			      const struct ethtool_tunable *tuna,
-			      const void *data)
-{
-	struct stmmac_priv *priv = netdev_priv(dev);
-	int ret = 0;
-
-	switch (tuna->id) {
-	case ETHTOOL_RX_COPYBREAK:
-		priv->rx_copybreak = *(u32 *)data;
-		break;
-	default:
-		ret = -EINVAL;
-		break;
-	}
-
-	return ret;
-}
-
 static const struct ethtool_ops stmmac_ethtool_ops = {
 	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
 				     ETHTOOL_COALESCE_MAX_FRAMES,
@@ -999,8 +962,6 @@ static const struct ethtool_ops stmmac_ethtool_ops = {
 	.set_coalesce = stmmac_set_coalesce,
 	.get_channels = stmmac_get_channels,
 	.set_channels = stmmac_set_channels,
-	.get_tunable = stmmac_get_tunable,
-	.set_tunable = stmmac_set_tunable,
 	.get_link_ksettings = stmmac_ethtool_get_link_ksettings,
 	.set_link_ksettings = stmmac_ethtool_set_link_ksettings,
 };
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 3bc07f5b64e1..8599ef6df52f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -84,8 +84,6 @@ static int buf_sz = DEFAULT_BUFSIZE;
 module_param(buf_sz, int, 0644);
 MODULE_PARM_DESC(buf_sz, "DMA buffer size");
 
-#define	STMMAC_RX_COPYBREAK	256
-
 static const u32 default_msg_level = (NETIF_MSG_DRV | NETIF_MSG_PROBE |
 				      NETIF_MSG_LINK | NETIF_MSG_IFUP |
 				      NETIF_MSG_IFDOWN | NETIF_MSG_TIMER);
@@ -2831,8 +2829,6 @@ static int stmmac_open(struct net_device *dev)
 	priv->dma_buf_sz = bfsize;
 	buf_sz = bfsize;
 
-	priv->rx_copybreak = STMMAC_RX_COPYBREAK;
-
 	if (!priv->dma_tx_size)
 		priv->dma_tx_size = DMA_DEFAULT_TX_SIZE;
 	if (!priv->dma_rx_size)
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH 11/20] net: stmmac: Discard index usage in the dirty_rx init
  2021-02-08 14:03 [PATCH 00/20] net: stmmac: Obvious cleanups and several fixes Serge Semin
                   ` (9 preceding siblings ...)
  2021-02-08 14:03 ` [PATCH 10/20] net: stmmac: Discard Rx copybreak ethtool setting Serge Semin
@ 2021-02-08 14:03 ` Serge Semin
  2021-02-08 14:03 ` [PATCH 12/20] net: stmmac: Discard dwmac1000_dma_ops declaration from dwmac100.h Serge Semin
                   ` (8 subsequent siblings)
  19 siblings, 0 replies; 37+ messages in thread
From: Serge Semin @ 2021-02-08 14:03 UTC (permalink / raw)
  To: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S. Miller, Jakub Kicinski, Joao Pinto, Jose Abreu,
	Maxime Coquelin
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Pavel Parkhomenko,
	Vyacheslav Mitrofanov, Russell King, Andrew Lunn,
	Heiner Kallweit, netdev, linux-stm32, linux-arm-kernel,
	linux-kernel

Indeed in accordance with the initialization loop logics the statement
"(i - priv->dma_rx_size)" will always equal to zero. Just initialize the
dirty_rx pointer with zero then.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 8599ef6df52f..abe8db9965f4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1412,7 +1412,7 @@ static int init_dma_rx_desc_rings(struct net_device *dev, gfp_t flags)
 		}
 
 		rx_q->cur_rx = 0;
-		rx_q->dirty_rx = (unsigned int)(i - priv->dma_rx_size);
+		rx_q->dirty_rx = 0;
 
 		/* Setup the chained descriptor addresses */
 		if (priv->mode == STMMAC_CHAIN_MODE) {
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH 12/20] net: stmmac: Discard dwmac1000_dma_ops declaration from dwmac100.h
  2021-02-08 14:03 [PATCH 00/20] net: stmmac: Obvious cleanups and several fixes Serge Semin
                   ` (10 preceding siblings ...)
  2021-02-08 14:03 ` [PATCH 11/20] net: stmmac: Discard index usage in the dirty_rx init Serge Semin
@ 2021-02-08 14:03 ` Serge Semin
  2021-02-08 14:03 ` [PATCH 13/20] net: stmmac: Move DMA Tx/Rx init methods to DW MAC lib Serge Semin
                   ` (7 subsequent siblings)
  19 siblings, 0 replies; 37+ messages in thread
From: Serge Semin @ 2021-02-08 14:03 UTC (permalink / raw)
  To: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S. Miller, Jakub Kicinski, Joao Pinto, Jose Abreu,
	Maxime Coquelin
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Pavel Parkhomenko,
	Vyacheslav Mitrofanov, Russell King, Andrew Lunn,
	Heiner Kallweit, netdev, linux-stm32, linux-arm-kernel,
	linux-kernel

Indeed it's redundant to have that variable declaration in the dwmac1000.h
header file since it's used in the hwif.c module only and declared in its
header together with the rest of the ops descriptors.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac1000.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h b/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h
index b70d44ac0990..494e1d2f2971 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h
@@ -329,5 +329,4 @@ enum rtc_control {
 #define GMAC_MMC_RX_CSUM_OFFLOAD   0x208
 #define GMAC_EXTHASH_BASE  0x500
 
-extern const struct stmmac_dma_ops dwmac1000_dma_ops;
 #endif /* __DWMAC1000_H__ */
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH 13/20] net: stmmac: Move DMA Tx/Rx init methods to DW MAC lib
  2021-02-08 14:03 [PATCH 00/20] net: stmmac: Obvious cleanups and several fixes Serge Semin
                   ` (11 preceding siblings ...)
  2021-02-08 14:03 ` [PATCH 12/20] net: stmmac: Discard dwmac1000_dma_ops declaration from dwmac100.h Serge Semin
@ 2021-02-08 14:03 ` Serge Semin
  2021-02-08 14:03 ` [PATCH 14/20] net: stmmac: Add DW GMAC disable LPI IRQ mask macro Serge Semin
                   ` (6 subsequent siblings)
  19 siblings, 0 replies; 37+ messages in thread
From: Serge Semin @ 2021-02-08 14:03 UTC (permalink / raw)
  To: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S. Miller, Jakub Kicinski, Joao Pinto, Jose Abreu,
	Maxime Coquelin
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Pavel Parkhomenko,
	Vyacheslav Mitrofanov, Russell King, Andrew Lunn,
	Heiner Kallweit, netdev, linux-stm32, linux-arm-kernel,
	linux-kernel

These methods are identical for both DW MAC100 and DW MAC1000 cores, so
their implementation can be moved to the common for the core library.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 .../ethernet/stmicro/stmmac/dwmac1000_dma.c   | 20 ++-----------------
 .../ethernet/stmicro/stmmac/dwmac100_dma.c    | 20 ++-----------------
 .../net/ethernet/stmicro/stmmac/dwmac_dma.h   |  4 ++++
 .../net/ethernet/stmicro/stmmac/dwmac_lib.c   | 14 +++++++++++++
 4 files changed, 22 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c
index 2bac49b49f73..2a04d9d45160 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c
@@ -110,22 +110,6 @@ static void dwmac1000_dma_init(void __iomem *ioaddr,
 	writel(DMA_INTR_DEFAULT_MASK, ioaddr + DMA_INTR_ENA);
 }
 
-static void dwmac1000_dma_init_rx(void __iomem *ioaddr,
-				  struct stmmac_dma_cfg *dma_cfg,
-				  dma_addr_t dma_rx_phy, u32 chan)
-{
-	/* RX descriptor base address list must be written into DMA CSR3 */
-	writel(lower_32_bits(dma_rx_phy), ioaddr + DMA_RCV_BASE_ADDR);
-}
-
-static void dwmac1000_dma_init_tx(void __iomem *ioaddr,
-				  struct stmmac_dma_cfg *dma_cfg,
-				  dma_addr_t dma_tx_phy, u32 chan)
-{
-	/* TX descriptor base address list must be written into DMA CSR4 */
-	writel(lower_32_bits(dma_tx_phy), ioaddr + DMA_TX_BASE_ADDR);
-}
-
 static u32 dwmac1000_configure_fc(u32 csr6, int rxfifosz)
 {
 	csr6 &= ~DMA_CONTROL_RFA_MASK;
@@ -263,8 +247,8 @@ static void dwmac1000_rx_watchdog(void __iomem *ioaddr, u32 riwt,
 const struct stmmac_dma_ops dwmac1000_dma_ops = {
 	.reset = dwmac_dma_reset,
 	.init = dwmac1000_dma_init,
-	.init_rx_chan = dwmac1000_dma_init_rx,
-	.init_tx_chan = dwmac1000_dma_init_tx,
+	.init_rx_chan = dwmac_dma_init_rx,
+	.init_tx_chan = dwmac_dma_init_tx,
 	.axi = dwmac1000_dma_axi,
 	.dump_regs = dwmac1000_dump_dma_regs,
 	.dma_rx_mode = dwmac1000_dma_operation_mode_rx,
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac100_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwmac100_dma.c
index 8f0d9bc7cab5..ad51a7949a42 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac100_dma.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac100_dma.c
@@ -29,22 +29,6 @@ static void dwmac100_dma_init(void __iomem *ioaddr,
 	writel(DMA_INTR_DEFAULT_MASK, ioaddr + DMA_INTR_ENA);
 }
 
-static void dwmac100_dma_init_rx(void __iomem *ioaddr,
-				 struct stmmac_dma_cfg *dma_cfg,
-				 dma_addr_t dma_rx_phy, u32 chan)
-{
-	/* RX descriptor base addr lists must be written into DMA CSR3 */
-	writel(lower_32_bits(dma_rx_phy), ioaddr + DMA_RCV_BASE_ADDR);
-}
-
-static void dwmac100_dma_init_tx(void __iomem *ioaddr,
-				 struct stmmac_dma_cfg *dma_cfg,
-				 dma_addr_t dma_tx_phy, u32 chan)
-{
-	/* TX descriptor base addr lists must be written into DMA CSR4 */
-	writel(lower_32_bits(dma_tx_phy), ioaddr + DMA_TX_BASE_ADDR);
-}
-
 /* Store and Forward capability is not used at all.
  *
  * The transmit threshold can be programmed by setting the TTC bits in the DMA
@@ -111,8 +95,8 @@ static void dwmac100_dma_diagnostic_fr(void *data, struct stmmac_extra_stats *x,
 const struct stmmac_dma_ops dwmac100_dma_ops = {
 	.reset = dwmac_dma_reset,
 	.init = dwmac100_dma_init,
-	.init_rx_chan = dwmac100_dma_init_rx,
-	.init_tx_chan = dwmac100_dma_init_tx,
+	.init_rx_chan = dwmac_dma_init_rx,
+	.init_tx_chan = dwmac_dma_init_tx,
 	.dump_regs = dwmac100_dump_dma_regs,
 	.dma_tx_mode = dwmac100_dma_operation_mode_tx,
 	.dma_diagnostic_fr = dwmac100_dma_diagnostic_fr,
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h b/drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h
index e5dbd0bc257e..fa919bf75e19 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac_dma.h
@@ -134,8 +134,12 @@
 void dwmac_enable_dma_transmission(void __iomem *ioaddr);
 void dwmac_enable_dma_irq(void __iomem *ioaddr, u32 chan, bool rx, bool tx);
 void dwmac_disable_dma_irq(void __iomem *ioaddr, u32 chan, bool rx, bool tx);
+void dwmac_dma_init_tx(void __iomem *ioaddr, struct stmmac_dma_cfg *dma_cfg,
+		       dma_addr_t dma_tx_phy, u32 chan);
 void dwmac_dma_start_tx(void __iomem *ioaddr, u32 chan);
 void dwmac_dma_stop_tx(void __iomem *ioaddr, u32 chan);
+void dwmac_dma_init_rx(void __iomem *ioaddr, struct stmmac_dma_cfg *dma_cfg,
+		       dma_addr_t dma_rx_phy, u32 chan);
 void dwmac_dma_start_rx(void __iomem *ioaddr, u32 chan);
 void dwmac_dma_stop_rx(void __iomem *ioaddr, u32 chan);
 int dwmac_dma_interrupt(void __iomem *ioaddr, struct stmmac_extra_stats *x,
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac_lib.c b/drivers/net/ethernet/stmicro/stmmac/dwmac_lib.c
index 57a53a600aa5..6ddfc689e77b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac_lib.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac_lib.c
@@ -56,6 +56,13 @@ void dwmac_disable_dma_irq(void __iomem *ioaddr, u32 chan, bool rx, bool tx)
 	writel(value, ioaddr + DMA_INTR_ENA);
 }
 
+void dwmac_dma_init_tx(void __iomem *ioaddr, struct stmmac_dma_cfg *dma_cfg,
+		       dma_addr_t dma_tx_phy, u32 chan)
+{
+	/* TX descriptor base address list must be written into DMA CSR4 */
+	writel(lower_32_bits(dma_tx_phy), ioaddr + DMA_TX_BASE_ADDR);
+}
+
 void dwmac_dma_start_tx(void __iomem *ioaddr, u32 chan)
 {
 	u32 value = readl(ioaddr + DMA_CONTROL);
@@ -70,6 +77,13 @@ void dwmac_dma_stop_tx(void __iomem *ioaddr, u32 chan)
 	writel(value, ioaddr + DMA_CONTROL);
 }
 
+void dwmac_dma_init_rx(void __iomem *ioaddr, struct stmmac_dma_cfg *dma_cfg,
+		       dma_addr_t dma_rx_phy, u32 chan)
+{
+	/* RX descriptor base address list must be written into DMA CSR3 */
+	writel(lower_32_bits(dma_rx_phy), ioaddr + DMA_RCV_BASE_ADDR);
+}
+
 void dwmac_dma_start_rx(void __iomem *ioaddr, u32 chan)
 {
 	u32 value = readl(ioaddr + DMA_CONTROL);
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH 14/20] net: stmmac: Add DW GMAC disable LPI IRQ mask macro
  2021-02-08 14:03 [PATCH 00/20] net: stmmac: Obvious cleanups and several fixes Serge Semin
                   ` (12 preceding siblings ...)
  2021-02-08 14:03 ` [PATCH 13/20] net: stmmac: Move DMA Tx/Rx init methods to DW MAC lib Serge Semin
@ 2021-02-08 14:03 ` Serge Semin
  2021-02-08 14:03 ` [PATCH 15/20] net: stmmac: Discard STMMAC_RESETING flag Serge Semin
                   ` (5 subsequent siblings)
  19 siblings, 0 replies; 37+ messages in thread
From: Serge Semin @ 2021-02-08 14:03 UTC (permalink / raw)
  To: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S. Miller, Jakub Kicinski, Joao Pinto, Jose Abreu,
	Maxime Coquelin
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Pavel Parkhomenko,
	Vyacheslav Mitrofanov, Russell King, Andrew Lunn,
	Heiner Kallweit, netdev, linux-stm32, linux-arm-kernel,
	linux-kernel

Indeed the DW GMAC Interrupts mask register has got an ability to disable
the LPI interrupts. Add the macro to close up the MAC IRQs mask macros
set.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac1000.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h b/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h
index 494e1d2f2971..919f5b55bc7d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h
@@ -38,6 +38,7 @@
 #define	GMAC_INT_DISABLE_PCSAN		BIT(2)
 #define	GMAC_INT_DISABLE_PMT		BIT(3)
 #define	GMAC_INT_DISABLE_TIMESTAMP	BIT(9)
+#define	GMAC_INT_DISABLE_LPI		BIT(10)
 #define	GMAC_INT_DISABLE_PCS	(GMAC_INT_DISABLE_RGMII | \
 				 GMAC_INT_DISABLE_PCSLINK | \
 				 GMAC_INT_DISABLE_PCSAN)
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH 15/20] net: stmmac: Discard STMMAC_RESETING flag
  2021-02-08 14:03 [PATCH 00/20] net: stmmac: Obvious cleanups and several fixes Serge Semin
                   ` (13 preceding siblings ...)
  2021-02-08 14:03 ` [PATCH 14/20] net: stmmac: Add DW GMAC disable LPI IRQ mask macro Serge Semin
@ 2021-02-08 14:03 ` Serge Semin
  2021-02-08 14:03 ` [PATCH 16/20] net: stmmac: Discard conditional service task execution Serge Semin
                   ` (4 subsequent siblings)
  19 siblings, 0 replies; 37+ messages in thread
From: Serge Semin @ 2021-02-08 14:03 UTC (permalink / raw)
  To: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S. Miller, Jakub Kicinski, Joao Pinto, Jose Abreu,
	Maxime Coquelin
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Pavel Parkhomenko,
	Vyacheslav Mitrofanov, Russell King, Andrew Lunn,
	Heiner Kallweit, netdev, linux-stm32, linux-arm-kernel,
	linux-kernel

That flag is totally useless. It's set inside a non-reentrant
mutex-protected section. For the same reason the test-and-set loop is also
pointless. The flag is also unused anywhere else in the driver. So just
drop it.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h      | 1 -
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 ---
 2 files changed, 4 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index e444b1b237c0..3e2bf7e2dafb 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -248,7 +248,6 @@ struct stmmac_priv {
 enum stmmac_state {
 	STMMAC_DOWN,
 	STMMAC_RESET_REQUESTED,
-	STMMAC_RESETING,
 	STMMAC_SERVICE_SCHED,
 };
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index abe8db9965f4..16e08cfaadf0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -4669,14 +4669,11 @@ static void stmmac_reset_subtask(struct stmmac_priv *priv)
 
 	rtnl_lock();
 	netif_trans_update(priv->dev);
-	while (test_and_set_bit(STMMAC_RESETING, &priv->state))
-		usleep_range(1000, 2000);
 
 	set_bit(STMMAC_DOWN, &priv->state);
 	dev_close(priv->dev);
 	dev_open(priv->dev, NULL);
 	clear_bit(STMMAC_DOWN, &priv->state);
-	clear_bit(STMMAC_RESETING, &priv->state);
 	rtnl_unlock();
 }
 
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH 16/20] net: stmmac: Discard conditional service task execution
  2021-02-08 14:03 [PATCH 00/20] net: stmmac: Obvious cleanups and several fixes Serge Semin
                   ` (14 preceding siblings ...)
  2021-02-08 14:03 ` [PATCH 15/20] net: stmmac: Discard STMMAC_RESETING flag Serge Semin
@ 2021-02-08 14:03 ` Serge Semin
  2021-02-08 14:03 ` [PATCH 17/20] net: stmmac: Add 'cause' arg to the service task executioner Serge Semin
                   ` (3 subsequent siblings)
  19 siblings, 0 replies; 37+ messages in thread
From: Serge Semin @ 2021-02-08 14:03 UTC (permalink / raw)
  To: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S. Miller, Jakub Kicinski, Joao Pinto, Jose Abreu,
	Maxime Coquelin
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Pavel Parkhomenko,
	Vyacheslav Mitrofanov, Russell King, Andrew Lunn,
	Heiner Kallweit, netdev, linux-stm32, linux-arm-kernel,
	linux-kernel

Indeed CMWQ guaranties that each particular work item is non-reenatrant,
while using the atomic bitmask operation statement may cause a requested
event being missed if for instance some event happens while the service
task is being executed (see the STMMAC_SERVICE_SCHED flag semantic).
Similarly the service task can be requested for being executed while the
STMMAC core is in the down state. (Though for now there is no such
sub-task defined in the driver).

So to speak just drop the conditional service task execution and queue the
corresponding work anytime it's requested, while the service sub-tasks
shall determine whether they really need to be performed in particular
situations.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h      | 1 -
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 5 +----
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 3e2bf7e2dafb..d88bc8af8eaa 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -248,7 +248,6 @@ struct stmmac_priv {
 enum stmmac_state {
 	STMMAC_DOWN,
 	STMMAC_RESET_REQUESTED,
-	STMMAC_SERVICE_SCHED,
 };
 
 int stmmac_mdio_unregister(struct net_device *ndev);
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 16e08cfaadf0..08112b6e7afd 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -176,9 +176,7 @@ static void stmmac_enable_all_queues(struct stmmac_priv *priv)
 
 static void stmmac_service_event_schedule(struct stmmac_priv *priv)
 {
-	if (!test_bit(STMMAC_DOWN, &priv->state) &&
-	    !test_and_set_bit(STMMAC_SERVICE_SCHED, &priv->state))
-		queue_work(priv->wq, &priv->service_task);
+	queue_work(priv->wq, &priv->service_task);
 }
 
 static void stmmac_global_err(struct stmmac_priv *priv)
@@ -4683,7 +4681,6 @@ static void stmmac_service_task(struct work_struct *work)
 			service_task);
 
 	stmmac_reset_subtask(priv);
-	clear_bit(STMMAC_SERVICE_SCHED, &priv->state);
 }
 
 /**
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH 17/20] net: stmmac: Add 'cause' arg to the service task executioner
  2021-02-08 14:03 [PATCH 00/20] net: stmmac: Obvious cleanups and several fixes Serge Semin
                   ` (15 preceding siblings ...)
  2021-02-08 14:03 ` [PATCH 16/20] net: stmmac: Discard conditional service task execution Serge Semin
@ 2021-02-08 14:03 ` Serge Semin
  2021-02-08 14:03 ` [PATCH 18/20] net: stmmac: Move PTP clock enabling to PTP-init method Serge Semin
                   ` (2 subsequent siblings)
  19 siblings, 0 replies; 37+ messages in thread
From: Serge Semin @ 2021-02-08 14:03 UTC (permalink / raw)
  To: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S. Miller, Jakub Kicinski, Joao Pinto, Jose Abreu,
	Maxime Coquelin
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Pavel Parkhomenko,
	Vyacheslav Mitrofanov, Russell King, Andrew Lunn,
	Heiner Kallweit, netdev, linux-stm32, linux-arm-kernel,
	linux-kernel

In order to have a more descriptive and coherent service task interface
let's add the cause argument to the stmmac_service_event_schedule()
method. It will be used to test-and-set the corresponding flag in the
private device state variable, and execute the service handler if the flag
hasn't been set. By doing so we'll be able to activate the service
sub-task just by calling the stmmac_service_event_schedule() method.

Note currently there is only a single user of the service tasks interface.
It's used to handle a case of the critical device errors to cause the
interface reset. The changes provided here will also prevent the global
error handler from being called twice if the service task has already
being executed while reset sub-task still isn't started.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 08112b6e7afd..f3ced94b3f4e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -174,16 +174,18 @@ static void stmmac_enable_all_queues(struct stmmac_priv *priv)
 	}
 }
 
-static void stmmac_service_event_schedule(struct stmmac_priv *priv)
+static void stmmac_service_event_schedule(struct stmmac_priv *priv,
+					  unsigned long cause)
 {
-	queue_work(priv->wq, &priv->service_task);
+	if (!test_and_set_bit(cause, &priv->state))
+		queue_work(priv->wq, &priv->service_task);
 }
 
 static void stmmac_global_err(struct stmmac_priv *priv)
 {
 	netif_carrier_off(priv->dev);
-	set_bit(STMMAC_RESET_REQUESTED, &priv->state);
-	stmmac_service_event_schedule(priv);
+
+	stmmac_service_event_schedule(priv, STMMAC_RESET_REQUESTED);
 }
 
 /**
@@ -4658,8 +4660,6 @@ static const struct net_device_ops stmmac_netdev_ops = {
 
 static void stmmac_reset_subtask(struct stmmac_priv *priv)
 {
-	if (!test_and_clear_bit(STMMAC_RESET_REQUESTED, &priv->state))
-		return;
 	if (test_bit(STMMAC_DOWN, &priv->state))
 		return;
 
@@ -4680,7 +4680,8 @@ static void stmmac_service_task(struct work_struct *work)
 	struct stmmac_priv *priv = container_of(work, struct stmmac_priv,
 			service_task);
 
-	stmmac_reset_subtask(priv);
+	if (test_and_clear_bit(STMMAC_RESET_REQUESTED, &priv->state))
+		stmmac_reset_subtask(priv);
 }
 
 /**
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH 18/20] net: stmmac: Move PTP clock enabling to PTP-init method
  2021-02-08 14:03 [PATCH 00/20] net: stmmac: Obvious cleanups and several fixes Serge Semin
                   ` (16 preceding siblings ...)
  2021-02-08 14:03 ` [PATCH 17/20] net: stmmac: Add 'cause' arg to the service task executioner Serge Semin
@ 2021-02-08 14:03 ` Serge Semin
  2021-02-08 14:03 ` [PATCH 19/20] net: stmmac: Move DMA stop procedure to HW-setup antagonist Serge Semin
  2021-02-08 14:03 ` [PATCH 20/20] net: stmmac: Move MAC Tx/Rx disabling " Serge Semin
  19 siblings, 0 replies; 37+ messages in thread
From: Serge Semin @ 2021-02-08 14:03 UTC (permalink / raw)
  To: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S. Miller, Jakub Kicinski, Joao Pinto, Jose Abreu,
	Maxime Coquelin
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Pavel Parkhomenko,
	Vyacheslav Mitrofanov, Russell King, Andrew Lunn,
	Heiner Kallweit, netdev, linux-stm32, linux-arm-kernel,
	linux-kernel

That clock is purely dedicated for the PTP feature of DW *MAC. From the
driver readability and maintainability point of the view it's better to
have it enabled/disable in the PTP interface init/release methods. Let's
do that by moving the clock prepare/enable procedure invocation to the
stmmac_init_ptp() method and adding the clock disable/unprepare function
call to stmmac_release_ptp(). Since the clock is now handled in the
framework of the PTP-interface related initializers/de-initializers we
need to call the stmmac_release_ptp() method in the HW-setup antagonist -
stmmac_hw_teardown(). Thus call the later one when the network device is
closed to clean the PTP-interface too.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 25 +++++++++++++------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index f3ced94b3f4e..d6446aa712e1 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -761,10 +761,18 @@ static int stmmac_hwtstamp_get(struct net_device *dev, struct ifreq *ifr)
 static int stmmac_init_ptp(struct stmmac_priv *priv)
 {
 	bool xmac = priv->plat->has_gmac4 || priv->plat->has_xgmac;
+	int ret;
 
 	if (!(priv->dma_cap.time_stamp || priv->dma_cap.atime_stamp))
 		return -EOPNOTSUPP;
 
+	ret = clk_prepare_enable(priv->plat->clk_ptp_ref);
+	if (ret) {
+		netdev_warn(priv->dev, "failed to enable PTP ref-clock: %d\n",
+			    ret);
+		return ret;
+	}
+
 	priv->adv_ts = 0;
 	/* Check if adv_ts can be enabled for dwmac 4.x / xgmac core */
 	if (xmac && priv->dma_cap.atime_stamp)
@@ -790,8 +798,12 @@ static int stmmac_init_ptp(struct stmmac_priv *priv)
 
 static void stmmac_release_ptp(struct stmmac_priv *priv)
 {
-	clk_disable_unprepare(priv->plat->clk_ptp_ref);
+	if (!(priv->dma_cap.time_stamp || priv->dma_cap.atime_stamp))
+		return;
+
 	stmmac_ptp_unregister(priv);
+
+	clk_disable_unprepare(priv->plat->clk_ptp_ref);
 }
 
 /**
@@ -2716,10 +2728,6 @@ static int stmmac_hw_setup(struct net_device *dev, bool init_ptp)
 	stmmac_mmc_setup(priv);
 
 	if (init_ptp) {
-		ret = clk_prepare_enable(priv->plat->clk_ptp_ref);
-		if (ret < 0)
-			netdev_warn(priv->dev, "failed to enable PTP reference clock: %d\n", ret);
-
 		ret = stmmac_init_ptp(priv);
 		if (ret == -EOPNOTSUPP)
 			netdev_warn(priv->dev, "PTP not supported by HW\n");
@@ -2784,7 +2792,7 @@ static void stmmac_hw_teardown(struct net_device *dev)
 {
 	struct stmmac_priv *priv = netdev_priv(dev);
 
-	clk_disable_unprepare(priv->plat->clk_ptp_ref);
+	stmmac_release_ptp(priv);
 }
 
 /**
@@ -2965,6 +2973,9 @@ static int stmmac_release(struct net_device *dev)
 	/* Stop TX/RX DMA and clear the descriptors */
 	stmmac_stop_all_dma(priv);
 
+	/* Cleanup HW setup */
+	stmmac_hw_teardown(dev);
+
 	/* Release and free the Rx/Tx resources */
 	free_dma_desc_resources(priv);
 
@@ -2973,8 +2984,6 @@ static int stmmac_release(struct net_device *dev)
 
 	netif_carrier_off(dev);
 
-	stmmac_release_ptp(priv);
-
 	return 0;
 }
 
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH 19/20] net: stmmac: Move DMA stop procedure to HW-setup antagonist
  2021-02-08 14:03 [PATCH 00/20] net: stmmac: Obvious cleanups and several fixes Serge Semin
                   ` (17 preceding siblings ...)
  2021-02-08 14:03 ` [PATCH 18/20] net: stmmac: Move PTP clock enabling to PTP-init method Serge Semin
@ 2021-02-08 14:03 ` Serge Semin
  2021-02-08 14:03 ` [PATCH 20/20] net: stmmac: Move MAC Tx/Rx disabling " Serge Semin
  19 siblings, 0 replies; 37+ messages in thread
From: Serge Semin @ 2021-02-08 14:03 UTC (permalink / raw)
  To: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S. Miller, Jakub Kicinski, Joao Pinto, Jose Abreu,
	Maxime Coquelin
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Pavel Parkhomenko,
	Vyacheslav Mitrofanov, Russell King, Andrew Lunn,
	Heiner Kallweit, netdev, linux-stm32, linux-arm-kernel,
	linux-kernel

The DMA-channels enabling procedure is performed in the framework of the
the DW *MAC hardware setup method. For the sake of the driver code
coherency let's move the DMA-channels stop function invocation to the
HW-setup antagonist method - stmmac_hw_teardown(). The latter is called in
the stmmac_hw_setup() error path and in the network device release
callback. So by introducing this alteration we not only improve the code
readability, but also make the stmmac_hw_teardown() doing better the HW
cleanup work.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index d6446aa712e1..3c03b773295a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2792,6 +2792,8 @@ static void stmmac_hw_teardown(struct net_device *dev)
 {
 	struct stmmac_priv *priv = netdev_priv(dev);
 
+	stmmac_stop_all_dma(priv);
+
 	stmmac_release_ptp(priv);
 }
 
@@ -2970,9 +2972,6 @@ static int stmmac_release(struct net_device *dev)
 		del_timer_sync(&priv->eee_ctrl_timer);
 	}
 
-	/* Stop TX/RX DMA and clear the descriptors */
-	stmmac_stop_all_dma(priv);
-
 	/* Cleanup HW setup */
 	stmmac_hw_teardown(dev);
 
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* [PATCH 20/20] net: stmmac: Move MAC Tx/Rx disabling to HW-setup antagonist
  2021-02-08 14:03 [PATCH 00/20] net: stmmac: Obvious cleanups and several fixes Serge Semin
                   ` (18 preceding siblings ...)
  2021-02-08 14:03 ` [PATCH 19/20] net: stmmac: Move DMA stop procedure to HW-setup antagonist Serge Semin
@ 2021-02-08 14:03 ` Serge Semin
  19 siblings, 0 replies; 37+ messages in thread
From: Serge Semin @ 2021-02-08 14:03 UTC (permalink / raw)
  To: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S. Miller, Jakub Kicinski, Joao Pinto, Jose Abreu,
	Maxime Coquelin
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Pavel Parkhomenko,
	Vyacheslav Mitrofanov, Russell King, Andrew Lunn,
	Heiner Kallweit, netdev, linux-stm32, linux-arm-kernel,
	linux-kernel

Since MAC Tx/Rx activity is enabled in the core hardware setup procedure
it would be logically correct to it disabled in the HW-setup antagonist -
stmmac_hw_teardown(). Let's do that to improve the driver code coherency
and thus readability.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 3c03b773295a..f70cab9f46d9 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2795,6 +2795,8 @@ static void stmmac_hw_teardown(struct net_device *dev)
 	stmmac_stop_all_dma(priv);
 
 	stmmac_release_ptp(priv);
+
+	stmmac_mac_set(priv, priv->ioaddr, false);
 }
 
 /**
@@ -2978,9 +2980,6 @@ static int stmmac_release(struct net_device *dev)
 	/* Release and free the Rx/Tx resources */
 	free_dma_desc_resources(priv);
 
-	/* Disable the MAC Rx/Tx */
-	stmmac_mac_set(priv, priv->ioaddr, false);
-
 	netif_carrier_off(dev);
 
 	return 0;
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 37+ messages in thread

* Re: [PATCH 01/20] net: phy: realtek: Fix events detection failure in LPI mode
  2021-02-08 14:03 ` [PATCH 01/20] net: phy: realtek: Fix events detection failure in LPI mode Serge Semin
@ 2021-02-08 15:27   ` Andrew Lunn
  2021-02-08 17:44     ` Serge Semin
  2021-02-08 20:14   ` Heiner Kallweit
  1 sibling, 1 reply; 37+ messages in thread
From: Andrew Lunn @ 2021-02-08 15:27 UTC (permalink / raw)
  To: Serge Semin
  Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S. Miller, Jakub Kicinski, Joao Pinto, Jose Abreu,
	Heiner Kallweit, Russell King, Serge Semin, Alexey Malahov,
	Pavel Parkhomenko, Vyacheslav Mitrofanov, Maxime Coquelin,
	netdev, linux-stm32, linux-arm-kernel, linux-kernel

On Mon, Feb 08, 2021 at 05:03:22PM +0300, Serge Semin wrote:
> It has been noticed that RTL8211E PHY stops detecting and reporting events
> when EEE is successfully advertised and RXC stopping in LPI is enabled.
> The freeze happens right after 3.0.10 bit (PC1R "Clock Stop Enable"
> register) is set. At the same time LED2 stops blinking as if EEE mode has
> been disabled. Notably the network traffic still flows through the PHY
> with no obvious problem. Anyway if any MDIO read procedure is performed
> after the "RXC stop in LPI" mode is enabled PHY gets to be unfrozen, LED2
> starts blinking and PHY interrupts happens again. The problem has been
> noticed on RTL8211E PHY working together with DW GMAC 3.73a MAC and
> reporting its event via a dedicated IRQ signal. (Obviously the problem has
> been unnoticed in the polling mode, since it gets naturally fixed by the
> periodic MDIO read procedure from the PHY status register - BMSR.)
> 
> In order to fix that problem we suggest to locally re-implement the MMD
> write method for RTL8211E PHY and perform a dummy read right after the
> PC1R register is accessed to enable the RXC stopping in LPI mode.

Hi Serge

Is this listed in an Errata from Realtek?

   Andrew

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 01/20] net: phy: realtek: Fix events detection failure in LPI mode
  2021-02-08 15:27   ` Andrew Lunn
@ 2021-02-08 17:44     ` Serge Semin
  2021-02-08 19:03       ` Andrew Lunn
  0 siblings, 1 reply; 37+ messages in thread
From: Serge Semin @ 2021-02-08 17:44 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Serge Semin, Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S. Miller, Jakub Kicinski, Joao Pinto, Jose Abreu,
	Heiner Kallweit, Russell King, Alexey Malahov, Pavel Parkhomenko,
	Vyacheslav Mitrofanov, Maxime Coquelin, netdev, linux-stm32,
	linux-arm-kernel, linux-kernel

On Mon, Feb 08, 2021 at 04:27:36PM +0100, Andrew Lunn wrote:
> On Mon, Feb 08, 2021 at 05:03:22PM +0300, Serge Semin wrote:
> > It has been noticed that RTL8211E PHY stops detecting and reporting events
> > when EEE is successfully advertised and RXC stopping in LPI is enabled.
> > The freeze happens right after 3.0.10 bit (PC1R "Clock Stop Enable"
> > register) is set. At the same time LED2 stops blinking as if EEE mode has
> > been disabled. Notably the network traffic still flows through the PHY
> > with no obvious problem. Anyway if any MDIO read procedure is performed
> > after the "RXC stop in LPI" mode is enabled PHY gets to be unfrozen, LED2
> > starts blinking and PHY interrupts happens again. The problem has been
> > noticed on RTL8211E PHY working together with DW GMAC 3.73a MAC and
> > reporting its event via a dedicated IRQ signal. (Obviously the problem has
> > been unnoticed in the polling mode, since it gets naturally fixed by the
> > periodic MDIO read procedure from the PHY status register - BMSR.)
> > 
> > In order to fix that problem we suggest to locally re-implement the MMD
> > write method for RTL8211E PHY and perform a dummy read right after the
> > PC1R register is accessed to enable the RXC stopping in LPI mode.
> 
> Hi Serge
> 
> Is this listed in an Errata from Realtek?

Hi Andrew,

I honestly tried to find any doc with a glimpse of errata for RTL8211E
PHY, but with no luck. Official datasheet didn't have any info regarding
possible hw bugs too. Thus I had no choice but to find a fix of the
problem myself.

It took me some time to figure out why the events weren't reported after
the very first link setup (turned out only a full HW reset clears the
PC1R.10 bit state). I thought it could have been connected with some
sleep/idle/power-safe mode. So I disabled the EEE initialization in the
STMMAC driver. It worked. Then I left the EEE mode enabled, but called the
phy_init_eee(phy, 0) method with "clk_stop_enable==0", so PHY wouldn't
stop RXC in LPI mode. And it wonderfully worked. Then I started to dig in
from another side. I left "RXC disable in LPI" mode enabled and tried to
figure out what was going on with the PHY when it stopped reporting events
just by reading from its CSR using phytool utility. It was curious to
discover that any attempt to read from any PHY register caused the problem
disappearance (LED2 started blinking, events got to be reported). Since I
did nothing but a mere reading from a random even EEE-unrelated register I
inferred that the problem must be in some HW/PHY bug. That's how I've got
to the patch introduced here. If you have any better idea what could be a
reason of that weird behavior I'd be glad to test it out on my device.

-Sergey

> 
>    Andrew

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 01/20] net: phy: realtek: Fix events detection failure in LPI mode
  2021-02-08 17:44     ` Serge Semin
@ 2021-02-08 19:03       ` Andrew Lunn
  0 siblings, 0 replies; 37+ messages in thread
From: Andrew Lunn @ 2021-02-08 19:03 UTC (permalink / raw)
  To: Serge Semin
  Cc: Serge Semin, Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S. Miller, Jakub Kicinski, Joao Pinto, Jose Abreu,
	Heiner Kallweit, Russell King, Alexey Malahov, Pavel Parkhomenko,
	Vyacheslav Mitrofanov, Maxime Coquelin, netdev, linux-stm32,
	linux-arm-kernel, linux-kernel

> Hi Andrew,
> 
> I honestly tried to find any doc with a glimpse of errata for RTL8211E
> PHY, but with no luck. Official datasheet didn't have any info regarding
> possible hw bugs too. Thus I had no choice but to find a fix of the
> problem myself.
> 
> It took me some time to figure out why the events weren't reported after
> the very first link setup (turned out only a full HW reset clears the
> PC1R.10 bit state). I thought it could have been connected with some
> sleep/idle/power-safe mode. So I disabled the EEE initialization in the
> STMMAC driver. It worked. Then I left the EEE mode enabled, but called the
> phy_init_eee(phy, 0) method with "clk_stop_enable==0", so PHY wouldn't
> stop RXC in LPI mode. And it wonderfully worked. Then I started to dig in
> from another side. I left "RXC disable in LPI" mode enabled and tried to
> figure out what was going on with the PHY when it stopped reporting events
> just by reading from its CSR using phytool utility. It was curious to
> discover that any attempt to read from any PHY register caused the problem
> disappearance (LED2 started blinking, events got to be reported). Since I
> did nothing but a mere reading from a random even EEE-unrelated register I
> inferred that the problem must be in some HW/PHY bug. That's how I've got
> to the patch introduced here. If you have any better idea what could be a
> reason of that weird behavior I'd be glad to test it out on my device.

It is a reasonable explanation, and a read should not do any harm.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
   

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 09/20] net: stmmac: Discard mii_irq array from private data
  2021-02-08 14:03 ` [PATCH 09/20] net: stmmac: Discard mii_irq array from private data Serge Semin
@ 2021-02-08 19:13   ` Andrew Lunn
  0 siblings, 0 replies; 37+ messages in thread
From: Andrew Lunn @ 2021-02-08 19:13 UTC (permalink / raw)
  To: Serge Semin
  Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S. Miller, Jakub Kicinski, Joao Pinto, Jose Abreu,
	Maxime Coquelin, Serge Semin, Alexey Malahov, Pavel Parkhomenko,
	Vyacheslav Mitrofanov, Russell King, Heiner Kallweit, netdev,
	linux-stm32, linux-arm-kernel, linux-kernel

On Mon, Feb 08, 2021 at 05:03:30PM +0300, Serge Semin wrote:
> There has been no user of the denoted array of the device private data
> since commit e7f4dc3536a4 ("mdio: Move allocation of interrupts into
> core"). Discard it then.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 01/20] net: phy: realtek: Fix events detection failure in LPI mode
  2021-02-08 14:03 ` [PATCH 01/20] net: phy: realtek: Fix events detection failure in LPI mode Serge Semin
  2021-02-08 15:27   ` Andrew Lunn
@ 2021-02-08 20:14   ` Heiner Kallweit
  2021-02-09 10:15     ` Serge Semin
  1 sibling, 1 reply; 37+ messages in thread
From: Heiner Kallweit @ 2021-02-08 20:14 UTC (permalink / raw)
  To: Serge Semin, Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S. Miller, Jakub Kicinski, Joao Pinto, Jose Abreu,
	Andrew Lunn, Russell King
  Cc: Serge Semin, Alexey Malahov, Pavel Parkhomenko,
	Vyacheslav Mitrofanov, Maxime Coquelin, netdev, linux-stm32,
	linux-arm-kernel, linux-kernel

On 08.02.2021 15:03, Serge Semin wrote:
> It has been noticed that RTL8211E PHY stops detecting and reporting events
> when EEE is successfully advertised and RXC stopping in LPI is enabled.
> The freeze happens right after 3.0.10 bit (PC1R "Clock Stop Enable"
> register) is set. At the same time LED2 stops blinking as if EEE mode has
> been disabled. Notably the network traffic still flows through the PHY
> with no obvious problem. Anyway if any MDIO read procedure is performed
> after the "RXC stop in LPI" mode is enabled PHY gets to be unfrozen, LED2
> starts blinking and PHY interrupts happens again. The problem has been
> noticed on RTL8211E PHY working together with DW GMAC 3.73a MAC and
> reporting its event via a dedicated IRQ signal. (Obviously the problem has
> been unnoticed in the polling mode, since it gets naturally fixed by the
> periodic MDIO read procedure from the PHY status register - BMSR.)
> 
> In order to fix that problem we suggest to locally re-implement the MMD
> write method for RTL8211E PHY and perform a dummy read right after the
> PC1R register is accessed to enable the RXC stopping in LPI mode.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> ---
>  drivers/net/phy/realtek.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index 99ecd6c4c15a..cbb86c257aae 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -559,6 +559,42 @@ static int rtl822x_write_mmd(struct phy_device *phydev, int devnum, u16 regnum,
>  	return ret;
>  }
>  
> +static int rtl8211e_write_mmd(struct phy_device *phydev, int devnum, u16 regnum,
> +			      u16 val)
> +{
> +	int ret;
> +
> +	/* Write to the MMD registers by using the standard control/data pair.
> +	 * The only difference is that we need to perform a dummy read after
> +	 * the PC1R.CLKSTOP_EN bit is set. It's required to workaround an issue
> +	 * of a partial core freeze so LED2 stops blinking in EEE mode, PHY
> +	 * stops detecting the link change and raising IRQs until any read from
> +	 * its registers performed. That happens only if and right after the PHY
> +	 * is enabled to stop RXC in LPI mode.
> +	 */
> +	ret = __phy_write(phydev, MII_MMD_CTRL, devnum);
> +	if (ret)
> +		return ret;
> +
> +	ret = __phy_write(phydev, MII_MMD_DATA, regnum);
> +	if (ret)
> +		return ret;
> +
> +	ret = __phy_write(phydev, MII_MMD_CTRL, devnum | MII_MMD_CTRL_NOINCR);
> +	if (ret)
> +		return ret;
> +

Nice analysis. Alternatively to duplicating this code piece we could
export mmd_phy_indirect(). But up to you.

> +	ret = __phy_write(phydev, MII_MMD_DATA, val);
> +	if (ret)
> +		return ret;
> +
> +	if (devnum == MDIO_MMD_PCS && regnum == MDIO_CTRL1 &&
> +	    val & MDIO_PCS_CTRL1_CLKSTOP_EN)
> +		ret =  __phy_read(phydev, MII_MMD_DATA);
> +
> +	return ret < 0 ? ret : 0;
> +}
> +
>  static int rtl822x_get_features(struct phy_device *phydev)
>  {
>  	int val;
> @@ -725,6 +761,7 @@ static struct phy_driver realtek_drvs[] = {
>  		.resume		= genphy_resume,
>  		.read_page	= rtl821x_read_page,
>  		.write_page	= rtl821x_write_page,
> +		.write_mmd	= rtl8211e_write_mmd,
>  	}, {
>  		PHY_ID_MATCH_EXACT(0x001cc916),
>  		.name		= "RTL8211F Gigabit Ethernet",
> 


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 01/20] net: phy: realtek: Fix events detection failure in LPI mode
  2021-02-08 20:14   ` Heiner Kallweit
@ 2021-02-09 10:15     ` Serge Semin
  2021-02-09 10:37       ` Heiner Kallweit
  2021-02-09 10:54       ` Russell King - ARM Linux admin
  0 siblings, 2 replies; 37+ messages in thread
From: Serge Semin @ 2021-02-09 10:15 UTC (permalink / raw)
  To: Heiner Kallweit
  Cc: Serge Semin, Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S. Miller, Jakub Kicinski, Joao Pinto, Jose Abreu,
	Andrew Lunn, Russell King, Alexey Malahov, Pavel Parkhomenko,
	Vyacheslav Mitrofanov, Maxime Coquelin, netdev, linux-stm32,
	linux-arm-kernel, linux-kernel

On Mon, Feb 08, 2021 at 09:14:02PM +0100, Heiner Kallweit wrote:
> On 08.02.2021 15:03, Serge Semin wrote:
> > It has been noticed that RTL8211E PHY stops detecting and reporting events
> > when EEE is successfully advertised and RXC stopping in LPI is enabled.
> > The freeze happens right after 3.0.10 bit (PC1R "Clock Stop Enable"
> > register) is set. At the same time LED2 stops blinking as if EEE mode has
> > been disabled. Notably the network traffic still flows through the PHY
> > with no obvious problem. Anyway if any MDIO read procedure is performed
> > after the "RXC stop in LPI" mode is enabled PHY gets to be unfrozen, LED2
> > starts blinking and PHY interrupts happens again. The problem has been
> > noticed on RTL8211E PHY working together with DW GMAC 3.73a MAC and
> > reporting its event via a dedicated IRQ signal. (Obviously the problem has
> > been unnoticed in the polling mode, since it gets naturally fixed by the
> > periodic MDIO read procedure from the PHY status register - BMSR.)
> > 
> > In order to fix that problem we suggest to locally re-implement the MMD
> > write method for RTL8211E PHY and perform a dummy read right after the
> > PC1R register is accessed to enable the RXC stopping in LPI mode.
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > ---
> >  drivers/net/phy/realtek.c | 37 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 37 insertions(+)
> > 
> > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> > index 99ecd6c4c15a..cbb86c257aae 100644
> > --- a/drivers/net/phy/realtek.c
> > +++ b/drivers/net/phy/realtek.c
> > @@ -559,6 +559,42 @@ static int rtl822x_write_mmd(struct phy_device *phydev, int devnum, u16 regnum,
> >  	return ret;
> >  }
> >  
> > +static int rtl8211e_write_mmd(struct phy_device *phydev, int devnum, u16 regnum,
> > +			      u16 val)
> > +{
> > +	int ret;
> > +
> > +	/* Write to the MMD registers by using the standard control/data pair.
> > +	 * The only difference is that we need to perform a dummy read after
> > +	 * the PC1R.CLKSTOP_EN bit is set. It's required to workaround an issue
> > +	 * of a partial core freeze so LED2 stops blinking in EEE mode, PHY
> > +	 * stops detecting the link change and raising IRQs until any read from
> > +	 * its registers performed. That happens only if and right after the PHY
> > +	 * is enabled to stop RXC in LPI mode.
> > +	 */
> > +	ret = __phy_write(phydev, MII_MMD_CTRL, devnum);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = __phy_write(phydev, MII_MMD_DATA, regnum);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = __phy_write(phydev, MII_MMD_CTRL, devnum | MII_MMD_CTRL_NOINCR);
> > +	if (ret)
> > +		return ret;
> > +
> 

> Nice analysis. Alternatively to duplicating this code piece we could
> export mmd_phy_indirect(). But up to you.

I also considered creating a generic method to access the MMD
registers of a generic PHY, something like phy_read()/phy_write(), but
for MMD (alas just exporting mmd_phy_indirect() would not be enough).
But as I see it such methods need to be created only after we get to
have at least several places with duplicating direct MMD-read/write
patterns. Doing that just for a single place seems redundant. Anyway it's
up to maintainers to decide whether they want to see a generic part
of the phy_read_mmd()/phy_write_mmd() methods being detached and
exported as something like genphy_{read,write}_mmd() methods. I can do
that in v2 if you ask me to.

-Sergey

> 
> > +	ret = __phy_write(phydev, MII_MMD_DATA, val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (devnum == MDIO_MMD_PCS && regnum == MDIO_CTRL1 &&
> > +	    val & MDIO_PCS_CTRL1_CLKSTOP_EN)
> > +		ret =  __phy_read(phydev, MII_MMD_DATA);
> > +
> > +	return ret < 0 ? ret : 0;
> > +}
> > +
> >  static int rtl822x_get_features(struct phy_device *phydev)
> >  {
> >  	int val;
> > @@ -725,6 +761,7 @@ static struct phy_driver realtek_drvs[] = {
> >  		.resume		= genphy_resume,
> >  		.read_page	= rtl821x_read_page,
> >  		.write_page	= rtl821x_write_page,
> > +		.write_mmd	= rtl8211e_write_mmd,
> >  	}, {
> >  		PHY_ID_MATCH_EXACT(0x001cc916),
> >  		.name		= "RTL8211F Gigabit Ethernet",
> > 
> 

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 01/20] net: phy: realtek: Fix events detection failure in LPI mode
  2021-02-09 10:15     ` Serge Semin
@ 2021-02-09 10:37       ` Heiner Kallweit
  2021-02-09 10:56         ` Russell King - ARM Linux admin
  2021-02-09 10:54       ` Russell King - ARM Linux admin
  1 sibling, 1 reply; 37+ messages in thread
From: Heiner Kallweit @ 2021-02-09 10:37 UTC (permalink / raw)
  To: Serge Semin
  Cc: Serge Semin, Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	David S. Miller, Jakub Kicinski, Joao Pinto, Jose Abreu,
	Andrew Lunn, Russell King, Alexey Malahov, Pavel Parkhomenko,
	Vyacheslav Mitrofanov, Maxime Coquelin, netdev, linux-stm32,
	linux-arm-kernel, linux-kernel

On 09.02.2021 11:15, Serge Semin wrote:
> On Mon, Feb 08, 2021 at 09:14:02PM +0100, Heiner Kallweit wrote:
>> On 08.02.2021 15:03, Serge Semin wrote:
>>> It has been noticed that RTL8211E PHY stops detecting and reporting events
>>> when EEE is successfully advertised and RXC stopping in LPI is enabled.
>>> The freeze happens right after 3.0.10 bit (PC1R "Clock Stop Enable"
>>> register) is set. At the same time LED2 stops blinking as if EEE mode has
>>> been disabled. Notably the network traffic still flows through the PHY
>>> with no obvious problem. Anyway if any MDIO read procedure is performed
>>> after the "RXC stop in LPI" mode is enabled PHY gets to be unfrozen, LED2
>>> starts blinking and PHY interrupts happens again. The problem has been
>>> noticed on RTL8211E PHY working together with DW GMAC 3.73a MAC and
>>> reporting its event via a dedicated IRQ signal. (Obviously the problem has
>>> been unnoticed in the polling mode, since it gets naturally fixed by the
>>> periodic MDIO read procedure from the PHY status register - BMSR.)
>>>
>>> In order to fix that problem we suggest to locally re-implement the MMD
>>> write method for RTL8211E PHY and perform a dummy read right after the
>>> PC1R register is accessed to enable the RXC stopping in LPI mode.
>>>
>>> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
>>> ---
>>>  drivers/net/phy/realtek.c | 37 +++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 37 insertions(+)
>>>
>>> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
>>> index 99ecd6c4c15a..cbb86c257aae 100644
>>> --- a/drivers/net/phy/realtek.c
>>> +++ b/drivers/net/phy/realtek.c
>>> @@ -559,6 +559,42 @@ static int rtl822x_write_mmd(struct phy_device *phydev, int devnum, u16 regnum,
>>>  	return ret;
>>>  }
>>>  
>>> +static int rtl8211e_write_mmd(struct phy_device *phydev, int devnum, u16 regnum,
>>> +			      u16 val)
>>> +{
>>> +	int ret;
>>> +
>>> +	/* Write to the MMD registers by using the standard control/data pair.
>>> +	 * The only difference is that we need to perform a dummy read after
>>> +	 * the PC1R.CLKSTOP_EN bit is set. It's required to workaround an issue
>>> +	 * of a partial core freeze so LED2 stops blinking in EEE mode, PHY
>>> +	 * stops detecting the link change and raising IRQs until any read from
>>> +	 * its registers performed. That happens only if and right after the PHY
>>> +	 * is enabled to stop RXC in LPI mode.
>>> +	 */
>>> +	ret = __phy_write(phydev, MII_MMD_CTRL, devnum);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	ret = __phy_write(phydev, MII_MMD_DATA, regnum);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	ret = __phy_write(phydev, MII_MMD_CTRL, devnum | MII_MMD_CTRL_NOINCR);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>
> 
>> Nice analysis. Alternatively to duplicating this code piece we could
>> export mmd_phy_indirect(). But up to you.
> 
> I also considered creating a generic method to access the MMD
> registers of a generic PHY, something like phy_read()/phy_write(), but
> for MMD (alas just exporting mmd_phy_indirect() would not be enough).
> But as I see it such methods need to be created only after we get to
> have at least several places with duplicating direct MMD-read/write
> patterns. Doing that just for a single place seems redundant. Anyway it's
> up to maintainers to decide whether they want to see a generic part
> of the phy_read_mmd()/phy_write_mmd() methods being detached and
> exported as something like genphy_{read,write}_mmd() methods. I can do
> that in v2 if you ask me to.
> 
Right, adding something like a genphy_{read,write}_mmd() doesn't make
too much sense for now. What I meant is just exporting mmd_phy_indirect().
Then you don't have to open-code the first three steps of a mmd read/write.
And it requires no additional code in phylib.
But that's not at all a showstopper here.

> -Sergey
> 
>>
>>> +	ret = __phy_write(phydev, MII_MMD_DATA, val);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	if (devnum == MDIO_MMD_PCS && regnum == MDIO_CTRL1 &&
>>> +	    val & MDIO_PCS_CTRL1_CLKSTOP_EN)
>>> +		ret =  __phy_read(phydev, MII_MMD_DATA);
>>> +
>>> +	return ret < 0 ? ret : 0;
>>> +}
>>> +
>>>  static int rtl822x_get_features(struct phy_device *phydev)
>>>  {
>>>  	int val;
>>> @@ -725,6 +761,7 @@ static struct phy_driver realtek_drvs[] = {
>>>  		.resume		= genphy_resume,
>>>  		.read_page	= rtl821x_read_page,
>>>  		.write_page	= rtl821x_write_page,
>>> +		.write_mmd	= rtl8211e_write_mmd,
>>>  	}, {
>>>  		PHY_ID_MATCH_EXACT(0x001cc916),
>>>  		.name		= "RTL8211F Gigabit Ethernet",
>>>
>>


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 01/20] net: phy: realtek: Fix events detection failure in LPI mode
  2021-02-09 10:15     ` Serge Semin
  2021-02-09 10:37       ` Heiner Kallweit
@ 2021-02-09 10:54       ` Russell King - ARM Linux admin
  1 sibling, 0 replies; 37+ messages in thread
From: Russell King - ARM Linux admin @ 2021-02-09 10:54 UTC (permalink / raw)
  To: Serge Semin
  Cc: Heiner Kallweit, Serge Semin, Giuseppe Cavallaro,
	Alexandre Torgue, Jose Abreu, David S. Miller, Jakub Kicinski,
	Joao Pinto, Jose Abreu, Andrew Lunn, Alexey Malahov,
	Pavel Parkhomenko, Vyacheslav Mitrofanov, Maxime Coquelin,
	netdev, linux-stm32, linux-arm-kernel, linux-kernel

On Tue, Feb 09, 2021 at 01:15:28PM +0300, Serge Semin wrote:
> On Mon, Feb 08, 2021 at 09:14:02PM +0100, Heiner Kallweit wrote:
> > Nice analysis. Alternatively to duplicating this code piece we could
> > export mmd_phy_indirect(). But up to you.
> 
> I also considered creating a generic method to access the MMD
> registers of a generic PHY, something like phy_read()/phy_write(), but
> for MMD (alas just exporting mmd_phy_indirect() would not be enough).
> But as I see it such methods need to be created only after we get to
> have at least several places with duplicating direct MMD-read/write
> patterns. Doing that just for a single place seems redundant. Anyway it's
> up to maintainers to decide whether they want to see a generic part
> of the phy_read_mmd()/phy_write_mmd() methods being detached and
> exported as something like genphy_{read,write}_mmd() methods. I can do
> that in v2 if you ask me to.

Please not genphy_* - that namespace is used for up-to-1G PHYs.

I thought about suggesting what you are proposing, but the problem is
this is just making things less and less efficient. Every time we
break a function up and export it, we increase the execution overhead
of the code. That said, the PHY accesses are relatively slow.

My opinion is that as this is just a single location at the moment,
it is not worth the effort - but if we get more of examples of this,
then it makes sense to provide the common accessor.

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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 01/20] net: phy: realtek: Fix events detection failure in LPI mode
  2021-02-09 10:37       ` Heiner Kallweit
@ 2021-02-09 10:56         ` Russell King - ARM Linux admin
  2021-02-10 16:47           ` Serge Semin
  0 siblings, 1 reply; 37+ messages in thread
From: Russell King - ARM Linux admin @ 2021-02-09 10:56 UTC (permalink / raw)
  To: Heiner Kallweit
  Cc: Serge Semin, Jose Abreu, Andrew Lunn, Joao Pinto, linux-kernel,
	Alexandre Torgue, netdev, linux-stm32, Serge Semin,
	Alexey Malahov, Jose Abreu, Pavel Parkhomenko, Maxime Coquelin,
	Jakub Kicinski, Giuseppe Cavallaro, Vyacheslav Mitrofanov,
	David S. Miller, linux-arm-kernel

On Tue, Feb 09, 2021 at 11:37:29AM +0100, Heiner Kallweit wrote:
> Right, adding something like a genphy_{read,write}_mmd() doesn't make
> too much sense for now. What I meant is just exporting mmd_phy_indirect().
> Then you don't have to open-code the first three steps of a mmd read/write.
> And it requires no additional code in phylib.

... but at the cost that the compiler can no longer inline that code,
as I mentioned in my previous reply. (However, the cost of the accesses
will be higher.) On the plus side, less I-cache footprint, and smaller
kernel code.

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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 01/20] net: phy: realtek: Fix events detection failure in LPI mode
  2021-02-09 10:56         ` Russell King - ARM Linux admin
@ 2021-02-10 16:47           ` Serge Semin
  2021-02-11 10:39             ` Russell King - ARM Linux admin
  0 siblings, 1 reply; 37+ messages in thread
From: Serge Semin @ 2021-02-10 16:47 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Serge Semin, Heiner Kallweit, Jose Abreu, Andrew Lunn,
	Joao Pinto, linux-kernel, Alexandre Torgue, netdev, linux-stm32,
	Alexey Malahov, Jose Abreu, Pavel Parkhomenko, Maxime Coquelin,
	Jakub Kicinski, Giuseppe Cavallaro, Vyacheslav Mitrofanov,
	David S. Miller, linux-arm-kernel

On Tue, Feb 09, 2021 at 10:56:46AM +0000, Russell King - ARM Linux admin wrote:
> On Tue, Feb 09, 2021 at 11:37:29AM +0100, Heiner Kallweit wrote:
> > Right, adding something like a genphy_{read,write}_mmd() doesn't make
> > too much sense for now. What I meant is just exporting mmd_phy_indirect().
> > Then you don't have to open-code the first three steps of a mmd read/write.
> > And it requires no additional code in phylib.
> 
> ... but at the cost that the compiler can no longer inline that code,
> as I mentioned in my previous reply. (However, the cost of the accesses
> will be higher.) On the plus side, less I-cache footprint, and smaller
> kernel code.

Just to note mmd_phy_indirect() isn't defined with inline specifier,
but just as static and it's used twice in the
drivers/net/phy/phy-core.c unit. So most likely the compiler won't
inline the function code in there. Anyway it's up to the PHY
library maintainers to decide. Please settle the issue with Heiner and
Andrew then. I am ok with both solutions and will do as you decide.

-Sergey

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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 01/20] net: phy: realtek: Fix events detection failure in LPI mode
  2021-02-10 16:47           ` Serge Semin
@ 2021-02-11 10:39             ` Russell King - ARM Linux admin
  2021-02-11 10:52               ` Serge Semin
  2021-02-20  9:02               ` Serge Semin
  0 siblings, 2 replies; 37+ messages in thread
From: Russell King - ARM Linux admin @ 2021-02-11 10:39 UTC (permalink / raw)
  To: Serge Semin
  Cc: Serge Semin, Heiner Kallweit, Jose Abreu, Andrew Lunn,
	Joao Pinto, linux-kernel, Alexandre Torgue, netdev, linux-stm32,
	Alexey Malahov, Jose Abreu, Pavel Parkhomenko, Maxime Coquelin,
	Jakub Kicinski, Giuseppe Cavallaro, Vyacheslav Mitrofanov,
	David S. Miller, linux-arm-kernel

On Wed, Feb 10, 2021 at 07:47:20PM +0300, Serge Semin wrote:
> On Tue, Feb 09, 2021 at 10:56:46AM +0000, Russell King - ARM Linux admin wrote:
> > On Tue, Feb 09, 2021 at 11:37:29AM +0100, Heiner Kallweit wrote:
> > > Right, adding something like a genphy_{read,write}_mmd() doesn't make
> > > too much sense for now. What I meant is just exporting mmd_phy_indirect().
> > > Then you don't have to open-code the first three steps of a mmd read/write.
> > > And it requires no additional code in phylib.
> > 
> > ... but at the cost that the compiler can no longer inline that code,
> > as I mentioned in my previous reply. (However, the cost of the accesses
> > will be higher.) On the plus side, less I-cache footprint, and smaller
> > kernel code.
> 
> Just to note mmd_phy_indirect() isn't defined with inline specifier,
> but just as static and it's used twice in the
> drivers/net/phy/phy-core.c unit. So most likely the compiler won't
> inline the function code in there.

You can't always tell whether the compiler will inline a static function
or not.

> Anyway it's up to the PHY
> library maintainers to decide. Please settle the issue with Heiner and
> Andrew then. I am ok with both solutions and will do as you decide.

FYI, *I* am one of the phylib maintainers.

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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 01/20] net: phy: realtek: Fix events detection failure in LPI mode
  2021-02-11 10:39             ` Russell King - ARM Linux admin
@ 2021-02-11 10:52               ` Serge Semin
  2021-02-20  9:02               ` Serge Semin
  1 sibling, 0 replies; 37+ messages in thread
From: Serge Semin @ 2021-02-11 10:52 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Serge Semin, Heiner Kallweit, Jose Abreu, Andrew Lunn,
	Joao Pinto, linux-kernel, Alexandre Torgue, netdev, linux-stm32,
	Alexey Malahov, Jose Abreu, Pavel Parkhomenko, Maxime Coquelin,
	Jakub Kicinski, Giuseppe Cavallaro, Vyacheslav Mitrofanov,
	David S. Miller, linux-arm-kernel

On Thu, Feb 11, 2021 at 10:39:41AM +0000, Russell King - ARM Linux admin wrote:
> On Wed, Feb 10, 2021 at 07:47:20PM +0300, Serge Semin wrote:
> > On Tue, Feb 09, 2021 at 10:56:46AM +0000, Russell King - ARM Linux admin wrote:
> > > On Tue, Feb 09, 2021 at 11:37:29AM +0100, Heiner Kallweit wrote:
> > > > Right, adding something like a genphy_{read,write}_mmd() doesn't make
> > > > too much sense for now. What I meant is just exporting mmd_phy_indirect().
> > > > Then you don't have to open-code the first three steps of a mmd read/write.
> > > > And it requires no additional code in phylib.
> > > 
> > > ... but at the cost that the compiler can no longer inline that code,
> > > as I mentioned in my previous reply. (However, the cost of the accesses
> > > will be higher.) On the plus side, less I-cache footprint, and smaller
> > > kernel code.
> > 
> > Just to note mmd_phy_indirect() isn't defined with inline specifier,
> > but just as static and it's used twice in the
> > drivers/net/phy/phy-core.c unit. So most likely the compiler won't
> > inline the function code in there.
> 
> You can't always tell whether the compiler will inline a static function
> or not.
> 
> > Anyway it's up to the PHY
> > library maintainers to decide. Please settle the issue with Heiner and
> > Andrew then. I am ok with both solutions and will do as you decide.
> 

> FYI, *I* am one of the phylib maintainers.

Of course I saw you in the list of maintainers. My message was that
currently two maintainers claims contradicting requests. Thus in order
to go further with this patch first you need to get to some agreement
between yourself. That's why we need to have a response from Hainer
about your arguments against his suggestion.

-Sergey

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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 01/20] net: phy: realtek: Fix events detection failure in LPI mode
  2021-02-11 10:39             ` Russell King - ARM Linux admin
  2021-02-11 10:52               ` Serge Semin
@ 2021-02-20  9:02               ` Serge Semin
  2021-02-20 12:51                 ` Heiner Kallweit
  1 sibling, 1 reply; 37+ messages in thread
From: Serge Semin @ 2021-02-20  9:02 UTC (permalink / raw)
  To: Russell King - ARM Linux admin, Heiner Kallweit, Andrew Lunn
  Cc: Serge Semin, Jose Abreu, Joao Pinto, linux-kernel,
	Alexandre Torgue, netdev, linux-stm32, Alexey Malahov,
	Jose Abreu, Pavel Parkhomenko, Maxime Coquelin, Jakub Kicinski,
	Giuseppe Cavallaro, Vyacheslav Mitrofanov, David S. Miller,
	linux-arm-kernel

On Thu, Feb 11, 2021 at 10:39:41AM +0000, Russell King - ARM Linux admin wrote:
> On Wed, Feb 10, 2021 at 07:47:20PM +0300, Serge Semin wrote:
> > On Tue, Feb 09, 2021 at 10:56:46AM +0000, Russell King - ARM Linux admin wrote:
> > > On Tue, Feb 09, 2021 at 11:37:29AM +0100, Heiner Kallweit wrote:
> > > > Right, adding something like a genphy_{read,write}_mmd() doesn't make
> > > > too much sense for now. What I meant is just exporting mmd_phy_indirect().
> > > > Then you don't have to open-code the first three steps of a mmd read/write.
> > > > And it requires no additional code in phylib.
> > > 
> > > ... but at the cost that the compiler can no longer inline that code,
> > > as I mentioned in my previous reply. (However, the cost of the accesses
> > > will be higher.) On the plus side, less I-cache footprint, and smaller
> > > kernel code.
> > 
> > Just to note mmd_phy_indirect() isn't defined with inline specifier,
> > but just as static and it's used twice in the
> > drivers/net/phy/phy-core.c unit. So most likely the compiler won't
> > inline the function code in there.
> 
> You can't always tell whether the compiler will inline a static function
> or not.

Andrew, Heiner, Russell, what is your final decision about this? Shall
we export the mmd_phy_indirect() method, implement new
genphy_{read,write}_mmd() or just leave the patch as is manually
accessing the MMD register in the driver?

-Sergey

> 
> > Anyway it's up to the PHY
> > library maintainers to decide. Please settle the issue with Heiner and
> > Andrew then. I am ok with both solutions and will do as you decide.
> 
> FYI, *I* am one of the phylib maintainers.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 01/20] net: phy: realtek: Fix events detection failure in LPI mode
  2021-02-20  9:02               ` Serge Semin
@ 2021-02-20 12:51                 ` Heiner Kallweit
  2021-02-20 15:49                   ` Andrew Lunn
  0 siblings, 1 reply; 37+ messages in thread
From: Heiner Kallweit @ 2021-02-20 12:51 UTC (permalink / raw)
  To: Serge Semin, Russell King - ARM Linux admin, Andrew Lunn
  Cc: Serge Semin, Jose Abreu, Joao Pinto, linux-kernel,
	Alexandre Torgue, netdev, linux-stm32, Alexey Malahov,
	Jose Abreu, Pavel Parkhomenko, Maxime Coquelin, Jakub Kicinski,
	Giuseppe Cavallaro, Vyacheslav Mitrofanov, David S. Miller,
	linux-arm-kernel

On 20.02.2021 10:02, Serge Semin wrote:
> On Thu, Feb 11, 2021 at 10:39:41AM +0000, Russell King - ARM Linux admin wrote:
>> On Wed, Feb 10, 2021 at 07:47:20PM +0300, Serge Semin wrote:
>>> On Tue, Feb 09, 2021 at 10:56:46AM +0000, Russell King - ARM Linux admin wrote:
>>>> On Tue, Feb 09, 2021 at 11:37:29AM +0100, Heiner Kallweit wrote:
>>>>> Right, adding something like a genphy_{read,write}_mmd() doesn't make
>>>>> too much sense for now. What I meant is just exporting mmd_phy_indirect().
>>>>> Then you don't have to open-code the first three steps of a mmd read/write.
>>>>> And it requires no additional code in phylib.
>>>>
>>>> ... but at the cost that the compiler can no longer inline that code,
>>>> as I mentioned in my previous reply. (However, the cost of the accesses
>>>> will be higher.) On the plus side, less I-cache footprint, and smaller
>>>> kernel code.
>>>
>>> Just to note mmd_phy_indirect() isn't defined with inline specifier,
>>> but just as static and it's used twice in the
>>> drivers/net/phy/phy-core.c unit. So most likely the compiler won't
>>> inline the function code in there.
>>
>> You can't always tell whether the compiler will inline a static function
>> or not.
> 
> Andrew, Heiner, Russell, what is your final decision about this? Shall
> we export the mmd_phy_indirect() method, implement new
> genphy_{read,write}_mmd() or just leave the patch as is manually
> accessing the MMD register in the driver?
> 

If in doubt, leaving the patch as is would be fine with me.

> -Sergey
> 
>>
>>> Anyway it's up to the PHY
>>> library maintainers to decide. Please settle the issue with Heiner and
>>> Andrew then. I am ok with both solutions and will do as you decide.
>>
>> FYI, *I* am one of the phylib maintainers.
>>
>> -- 
>> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
>> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 01/20] net: phy: realtek: Fix events detection failure in LPI mode
  2021-02-20 12:51                 ` Heiner Kallweit
@ 2021-02-20 15:49                   ` Andrew Lunn
  2021-02-21 13:51                     ` Serge Semin
  0 siblings, 1 reply; 37+ messages in thread
From: Andrew Lunn @ 2021-02-20 15:49 UTC (permalink / raw)
  To: Heiner Kallweit
  Cc: Serge Semin, Russell King - ARM Linux admin, Serge Semin,
	Jose Abreu, Joao Pinto, linux-kernel, Alexandre Torgue, netdev,
	linux-stm32, Alexey Malahov, Jose Abreu, Pavel Parkhomenko,
	Maxime Coquelin, Jakub Kicinski, Giuseppe Cavallaro,
	Vyacheslav Mitrofanov, David S. Miller, linux-arm-kernel

> If in doubt, leaving the patch as is would be fine with me.

The patch is O.K. as is, no need to export something so simple for a
single users. When the next user come along, we can reconsider.

       Andrew

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH 01/20] net: phy: realtek: Fix events detection failure in LPI mode
  2021-02-20 15:49                   ` Andrew Lunn
@ 2021-02-21 13:51                     ` Serge Semin
  0 siblings, 0 replies; 37+ messages in thread
From: Serge Semin @ 2021-02-21 13:51 UTC (permalink / raw)
  To: Andrew Lunn, Heiner Kallweit
  Cc: Serge Semin, Russell King - ARM Linux admin, Jose Abreu,
	Joao Pinto, linux-kernel, Alexandre Torgue, netdev, linux-stm32,
	Alexey Malahov, Jose Abreu, Pavel Parkhomenko, Maxime Coquelin,
	Jakub Kicinski, Giuseppe Cavallaro, Vyacheslav Mitrofanov,
	David S. Miller, linux-arm-kernel

On Sat, Feb 20, 2021 at 04:49:22PM +0100, Andrew Lunn wrote:
> > If in doubt, leaving the patch as is would be fine with me.
> 
> The patch is O.K. as is, no need to export something so simple for a
> single users. When the next user come along, we can reconsider.

Ok. Thanks for clarification. I performed some additional tests to
make sure the bug was on the PHY side. They proved my original
conclusion. It's indeed Realtek PHY to blame for the weird behavior. 
So I've added a few more words into the patch log regarding those
tests. The patch will be resent tomorrow together with the rest of the
STMMAC-driver-related bug-fixes detached from the original series of
the fixes and cleanups (as Andrew asked to do).

-Sergey

> 
>        Andrew

^ permalink raw reply	[flat|nested] 37+ messages in thread

end of thread, other threads:[~2021-02-21 13:52 UTC | newest]

Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-08 14:03 [PATCH 00/20] net: stmmac: Obvious cleanups and several fixes Serge Semin
2021-02-08 14:03 ` [PATCH 01/20] net: phy: realtek: Fix events detection failure in LPI mode Serge Semin
2021-02-08 15:27   ` Andrew Lunn
2021-02-08 17:44     ` Serge Semin
2021-02-08 19:03       ` Andrew Lunn
2021-02-08 20:14   ` Heiner Kallweit
2021-02-09 10:15     ` Serge Semin
2021-02-09 10:37       ` Heiner Kallweit
2021-02-09 10:56         ` Russell King - ARM Linux admin
2021-02-10 16:47           ` Serge Semin
2021-02-11 10:39             ` Russell King - ARM Linux admin
2021-02-11 10:52               ` Serge Semin
2021-02-20  9:02               ` Serge Semin
2021-02-20 12:51                 ` Heiner Kallweit
2021-02-20 15:49                   ` Andrew Lunn
2021-02-21 13:51                     ` Serge Semin
2021-02-09 10:54       ` Russell King - ARM Linux admin
2021-02-08 14:03 ` [PATCH 02/20] net: stmmac: Free Rx descs on Tx allocation failure Serge Semin
2021-02-08 14:03 ` [PATCH 03/20] net: stmmac: Fix false MTL RX overflow handling for higher queues Serge Semin
2021-02-08 14:03 ` [PATCH 04/20] net: stmmac: Assert reset control after MDIO de-registration Serge Semin
2021-02-08 14:03 ` [PATCH 05/20] net: stmmac: Use dwmac410_disable_dma_irq for DW MAC v4.10 DMA Serge Semin
2021-02-08 14:03 ` [PATCH 06/20] net: stmmac: Use LPI IRQ status-related macro in DW MAC1000 isr Serge Semin
2021-02-08 14:03 ` [PATCH 07/20] net: stmmac: Clear descriptors before initializing them Serge Semin
2021-02-08 14:03 ` [PATCH 08/20] net: stmmac: Fix typo in the XGMAC_L3_ADDR3 macro name Serge Semin
2021-02-08 14:03 ` [PATCH 09/20] net: stmmac: Discard mii_irq array from private data Serge Semin
2021-02-08 19:13   ` Andrew Lunn
2021-02-08 14:03 ` [PATCH 10/20] net: stmmac: Discard Rx copybreak ethtool setting Serge Semin
2021-02-08 14:03 ` [PATCH 11/20] net: stmmac: Discard index usage in the dirty_rx init Serge Semin
2021-02-08 14:03 ` [PATCH 12/20] net: stmmac: Discard dwmac1000_dma_ops declaration from dwmac100.h Serge Semin
2021-02-08 14:03 ` [PATCH 13/20] net: stmmac: Move DMA Tx/Rx init methods to DW MAC lib Serge Semin
2021-02-08 14:03 ` [PATCH 14/20] net: stmmac: Add DW GMAC disable LPI IRQ mask macro Serge Semin
2021-02-08 14:03 ` [PATCH 15/20] net: stmmac: Discard STMMAC_RESETING flag Serge Semin
2021-02-08 14:03 ` [PATCH 16/20] net: stmmac: Discard conditional service task execution Serge Semin
2021-02-08 14:03 ` [PATCH 17/20] net: stmmac: Add 'cause' arg to the service task executioner Serge Semin
2021-02-08 14:03 ` [PATCH 18/20] net: stmmac: Move PTP clock enabling to PTP-init method Serge Semin
2021-02-08 14:03 ` [PATCH 19/20] net: stmmac: Move DMA stop procedure to HW-setup antagonist Serge Semin
2021-02-08 14:03 ` [PATCH 20/20] net: stmmac: Move MAC Tx/Rx disabling " Serge Semin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).