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 <redacted>
Cc: Alexey Malahov <redacted>
Cc: Pavel Parkhomenko <redacted>
Cc: Vyacheslav Mitrofanov <redacted>
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
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 <redacted>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
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 <redacted>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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 <redacted>
---
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(-)
@@ -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){intstatus=stmmac_host_irq_status(priv,priv->hw,&priv->xstats);-intmtl_status;if(unlikely(status)){/* For LPI we need to save the tx status */
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 <redacted>
---
drivers/net/phy/realtek.c | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
@@ -559,6 +559,42 @@ static int rtl822x_write_mmd(struct phy_device *phydev, int devnum, u16 regnum,returnret;}+staticintrtl8211e_write_mmd(structphy_device*phydev,intdevnum,u16regnum,+u16val)+{+intret;++/* Write to the MMD registers by using the standard control/data pair.+*Theonlydifferenceisthatweneedtoperformadummyreadafter+*thePC1R.CLKSTOP_ENbitisset.It'srequiredtoworkaroundanissue+*ofapartialcorefreezesoLED2stopsblinkinginEEEmode,PHY+*stopsdetectingthelinkchangeandraisingIRQsuntilanyreadfrom+*itsregistersperformed.ThathappensonlyifandrightafterthePHY+*isenabledtostopRXCinLPImode.+*/+ret=__phy_write(phydev,MII_MMD_CTRL,devnum);+if(ret)+returnret;++ret=__phy_write(phydev,MII_MMD_DATA,regnum);+if(ret)+returnret;++ret=__phy_write(phydev,MII_MMD_CTRL,devnum|MII_MMD_CTRL_NOINCR);+if(ret)+returnret;++ret=__phy_write(phydev,MII_MMD_DATA,val);+if(ret)+returnret;++if(devnum==MDIO_MMD_PCS&®num==MDIO_CTRL1&&+val&MDIO_PCS_CTRL1_CLKSTOP_EN)+ret=__phy_read(phydev,MII_MMD_DATA);++returnret<0?ret:0;+}+staticintrtl822x_get_features(structphy_device*phydev){intval;
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 <redacted>
---
drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
@@ -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++;
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 <redacted>
---
drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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 <redacted>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
The macro has been declared as XMGAC_L3_ADDR3 with obvious second and
third chars confused. Revert them then.
Signed-off-by: Serge Semin <redacted>
---
drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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 <redacted>
---
drivers/net/ethernet/stmicro/stmmac/stmmac.h | 1 -
1 file changed, 1 deletion(-)
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 <redacted>
---
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(-)
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 <redacted>
---
drivers/net/ethernet/stmicro/stmmac/dwmac1000.h | 1 -
1 file changed, 1 deletion(-)
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 <redacted>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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 <redacted>
---
.../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(-)
@@ -110,22 +110,6 @@ static void dwmac1000_dma_init(void __iomem *ioaddr,writel(DMA_INTR_DEFAULT_MASK,ioaddr+DMA_INTR_ENA);}-staticvoiddwmac1000_dma_init_rx(void__iomem*ioaddr,-structstmmac_dma_cfg*dma_cfg,-dma_addr_tdma_rx_phy,u32chan)-{-/* RX descriptor base address list must be written into DMA CSR3 */-writel(lower_32_bits(dma_rx_phy),ioaddr+DMA_RCV_BASE_ADDR);-}--staticvoiddwmac1000_dma_init_tx(void__iomem*ioaddr,-structstmmac_dma_cfg*dma_cfg,-dma_addr_tdma_tx_phy,u32chan)-{-/* TX descriptor base address list must be written into DMA CSR4 */-writel(lower_32_bits(dma_tx_phy),ioaddr+DMA_TX_BASE_ADDR);-}-staticu32dwmac1000_configure_fc(u32csr6,intrxfifosz){csr6&=~DMA_CONTROL_RFA_MASK;
@@ -29,22 +29,6 @@ static void dwmac100_dma_init(void __iomem *ioaddr,writel(DMA_INTR_DEFAULT_MASK,ioaddr+DMA_INTR_ENA);}-staticvoiddwmac100_dma_init_rx(void__iomem*ioaddr,-structstmmac_dma_cfg*dma_cfg,-dma_addr_tdma_rx_phy,u32chan)-{-/* RX descriptor base addr lists must be written into DMA CSR3 */-writel(lower_32_bits(dma_rx_phy),ioaddr+DMA_RCV_BASE_ADDR);-}--staticvoiddwmac100_dma_init_tx(void__iomem*ioaddr,-structstmmac_dma_cfg*dma_cfg,-dma_addr_tdma_tx_phy,u32chan)-{-/* 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.**ThetransmitthresholdcanbeprogrammedbysettingtheTTCbitsintheDMA
@@ -56,6 +56,13 @@ void dwmac_disable_dma_irq(void __iomem *ioaddr, u32 chan, bool rx, bool tx)writel(value,ioaddr+DMA_INTR_ENA);}+voiddwmac_dma_init_tx(void__iomem*ioaddr,structstmmac_dma_cfg*dma_cfg,+dma_addr_tdma_tx_phy,u32chan)+{+/* TX descriptor base address list must be written into DMA CSR4 */+writel(lower_32_bits(dma_tx_phy),ioaddr+DMA_TX_BASE_ADDR);+}+voiddwmac_dma_start_tx(void__iomem*ioaddr,u32chan){u32value=readl(ioaddr+DMA_CONTROL);
@@ -70,6 +77,13 @@ void dwmac_dma_stop_tx(void __iomem *ioaddr, u32 chan)writel(value,ioaddr+DMA_CONTROL);}+voiddwmac_dma_init_rx(void__iomem*ioaddr,structstmmac_dma_cfg*dma_cfg,+dma_addr_tdma_rx_phy,u32chan)+{+/* RX descriptor base address list must be written into DMA CSR3 */+writel(lower_32_bits(dma_rx_phy),ioaddr+DMA_RCV_BASE_ADDR);+}+voiddwmac_dma_start_rx(void__iomem*ioaddr,u32chan){u32value=readl(ioaddr+DMA_CONTROL);
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 <redacted>
---
drivers/net/ethernet/stmicro/stmmac/dwmac1000.h | 1 +
1 file changed, 1 insertion(+)
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 <redacted>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
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 <redacted>
---
drivers/net/ethernet/stmicro/stmmac/stmmac.h | 1 -
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 5 +----
2 files changed, 1 insertion(+), 5 deletions(-)
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 <redacted>
---
.../net/ethernet/stmicro/stmmac/stmmac_main.c | 25 +++++++++++++------
1 file changed, 17 insertions(+), 8 deletions(-)
@@ -761,10 +761,18 @@ static int stmmac_hwtstamp_get(struct net_device *dev, struct ifreq *ifr)staticintstmmac_init_ptp(structstmmac_priv*priv){boolxmac=priv->plat->has_gmac4||priv->plat->has_xgmac;+intret;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);+returnret;+}+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)staticvoidstmmac_release_ptp(structstmmac_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");
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 <redacted>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
@@ -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);return0;
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 <redacted>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
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 <redacted>
---
drivers/net/ethernet/stmicro/stmmac/stmmac.h | 1 -
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 ---
2 files changed, 4 deletions(-)
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-02-08 17:29:20
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
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:
quoted
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
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-02-08 20:28:11
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
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-02-08 20:33:50
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 <redacted>
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 <redacted>
---
drivers/net/phy/realtek.c | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
@@ -559,6 +559,42 @@ static int rtl822x_write_mmd(struct phy_device *phydev, int devnum, u16 regnum,returnret;}+staticintrtl8211e_write_mmd(structphy_device*phydev,intdevnum,u16regnum,+u16val)+{+intret;++/* Write to the MMD registers by using the standard control/data pair.+*Theonlydifferenceisthatweneedtoperformadummyreadafter+*thePC1R.CLKSTOP_ENbitisset.It'srequiredtoworkaroundanissue+*ofapartialcorefreezesoLED2stopsblinkinginEEEmode,PHY+*stopsdetectingthelinkchangeandraisingIRQsuntilanyreadfrom+*itsregistersperformed.ThathappensonlyifandrightafterthePHY+*isenabledtostopRXCinLPImode.+*/+ret=__phy_write(phydev,MII_MMD_CTRL,devnum);+if(ret)+returnret;++ret=__phy_write(phydev,MII_MMD_DATA,regnum);+if(ret)+returnret;++ret=__phy_write(phydev,MII_MMD_CTRL,devnum|MII_MMD_CTRL_NOINCR);+if(ret)+returnret;+
Nice analysis. Alternatively to duplicating this code piece we could
export mmd_phy_indirect(). But up to you.
quoted hunk
+ 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;
On Mon, Feb 08, 2021 at 09:14:02PM +0100, Heiner Kallweit wrote:
On 08.02.2021 15:03, Serge Semin wrote:
quoted
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 <redacted>
---
drivers/net/phy/realtek.c | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
@@ -559,6 +559,42 @@ static int rtl822x_write_mmd(struct phy_device *phydev, int devnum, u16 regnum,returnret;}+staticintrtl8211e_write_mmd(structphy_device*phydev,intdevnum,u16regnum,+u16val)+{+intret;++/* Write to the MMD registers by using the standard control/data pair.+*Theonlydifferenceisthatweneedtoperformadummyreadafter+*thePC1R.CLKSTOP_ENbitisset.It'srequiredtoworkaroundanissue+*ofapartialcorefreezesoLED2stopsblinkinginEEEmode,PHY+*stopsdetectingthelinkchangeandraisingIRQsuntilanyreadfrom+*itsregistersperformed.ThathappensonlyifandrightafterthePHY+*isenabledtostopRXCinLPImode.+*/+ret=__phy_write(phydev,MII_MMD_CTRL,devnum);+if(ret)+returnret;++ret=__phy_write(phydev,MII_MMD_DATA,regnum);+if(ret)+returnret;++ret=__phy_write(phydev,MII_MMD_CTRL,devnum|MII_MMD_CTRL_NOINCR);+if(ret)+returnret;+
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
quoted
+ 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;
On Mon, Feb 08, 2021 at 09:14:02PM +0100, Heiner Kallweit wrote:
quoted
On 08.02.2021 15:03, Serge Semin wrote:
quoted
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 <redacted>
---
drivers/net/phy/realtek.c | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
@@ -559,6 +559,42 @@ static int rtl822x_write_mmd(struct phy_device *phydev, int devnum, u16 regnum,returnret;}+staticintrtl8211e_write_mmd(structphy_device*phydev,intdevnum,u16regnum,+u16val)+{+intret;++/* Write to the MMD registers by using the standard control/data pair.+*Theonlydifferenceisthatweneedtoperformadummyreadafter+*thePC1R.CLKSTOP_ENbitisset.It'srequiredtoworkaroundanissue+*ofapartialcorefreezesoLED2stopsblinkinginEEEmode,PHY+*stopsdetectingthelinkchangeandraisingIRQsuntilanyreadfrom+*itsregistersperformed.ThathappensonlyifandrightafterthePHY+*isenabledtostopRXCinLPImode.+*/+ret=__phy_write(phydev,MII_MMD_CTRL,devnum);+if(ret)+returnret;++ret=__phy_write(phydev,MII_MMD_DATA,regnum);+if(ret)+returnret;++ret=__phy_write(phydev,MII_MMD_CTRL,devnum|MII_MMD_CTRL_NOINCR);+if(ret)+returnret;+
quoted
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
quoted
quoted
+ 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;
From: Russell King - ARM Linux admin <linux@armlinux.org.uk> Date: 2021-02-09 10:59:52
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:
quoted
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!
From: Russell King - ARM Linux admin <linux@armlinux.org.uk> Date: 2021-02-09 11:03:16
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!
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:
quoted
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
From: Russell King - ARM Linux admin <linux@armlinux.org.uk> Date: 2021-02-11 10:44:40
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:
quoted
On Tue, Feb 09, 2021 at 11:37:29AM +0100, Heiner Kallweit wrote:
quoted
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.
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:
quoted
On Tue, Feb 09, 2021 at 10:56:46AM +0000, Russell King - ARM Linux admin wrote:
quoted
On Tue, Feb 09, 2021 at 11:37:29AM +0100, Heiner Kallweit wrote:
quoted
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.
quoted
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
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:
quoted
On Tue, Feb 09, 2021 at 10:56:46AM +0000, Russell King - ARM Linux admin wrote:
quoted
On Tue, Feb 09, 2021 at 11:37:29AM +0100, Heiner Kallweit wrote:
quoted
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
quoted
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.
On Thu, Feb 11, 2021 at 10:39:41AM +0000, Russell King - ARM Linux admin wrote:
quoted
On Wed, Feb 10, 2021 at 07:47:20PM +0300, Serge Semin wrote:
quoted
On Tue, Feb 09, 2021 at 10:56:46AM +0000, Russell King - ARM Linux admin wrote:
quoted
On Tue, Feb 09, 2021 at 11:37:29AM +0100, Heiner Kallweit wrote:
quoted
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
quoted
quoted
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.
On Sat, Feb 20, 2021 at 04:49:22PM +0100, Andrew Lunn wrote:
quoted
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