| Message ID | ZrCoQZKo74zvKMhT@shell.armlinux.org.uk (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: stmmac: convert stmmac "pcs" to phylink | expand |
Hi Russell On Mon, Aug 05, 2024 at 11:24:01AM +0100, Russell King (Oracle) wrote: > Hi, > > Changes since version 3: > - added Andrew's reviewed-bys > - fixed kernel-doc for dwmac_pcs_isr() > - updated patch 11 commit message > - fixed build error reported by Jakub > - add Sneh Shah to Cc list (for testing 2.5G modes) > > Bartosz - I know you've given your tested-by this morning, I will be > adding that after posting this series, so please don't think it's been > lost! Got this series tested on my DW GMAC v3.73a + Micrel KSZ9031RNX PHY with the in-band link status management enabled. The same positive result as before, on v1-v2: [ 294.651324] stmmaceth 1f060000.ethernet eth1: configuring for inband/rgmii-rxid link mode [ 294.582498] stmmaceth 1f060000.ethernet eth1: Register MEM_TYPE_PAGE_POOL RxQ-0 [ 294.594308] stmmaceth 1f060000.ethernet eth1: PHY [stmmac-1:03] driver [RTL8211E Gigabit Ethernet] (irq=POLL) [ 294.605453] dwmac1000: Master AXI performs any burst length [ 294.611899] stmmaceth 1f060000.ethernet: invalid port speed [ 294.618229] stmmaceth 1f060000.ethernet eth1: No Safety Features support found [ 294.626412] stmmaceth 1f060000.ethernet eth1: No MAC Management Counters available [ 294.634912] stmmaceth 1f060000.ethernet eth1: IEEE 1588-2008 Advanced Timestamp supported [ 294.644380] stmmaceth 1f060000.ethernet eth1: registered PTP clock [ 294.651324] stmmaceth 1f060000.ethernet eth1: configuring for inband/rgmii-rxid link mode ... [ 298.772917] stmmaceth 1f060000.ethernet eth1: Link is Up - 1Gbps/Full - flow control rx/tx So feel free to add: Tested-by: Serge Semin <fancer.lancer@gmail.com> Please note the warning: "stmmaceth 1f060000.ethernet: invalid port speed" in the log above. This is a false negative warning since my network devices isn't of MAC2MAC-type and there is no snps,ps-speed property in my dts. So having the priv->hw.ps set to zero should be fine. That said I guess we need to add the warning fix to the 14/14 patch which would permit the plat_stmmacenet_data::mac_port_sel_speed field being zero. > > Previous cover messages from earlier posts below: > > This is version 3 of the series switching stmmac to use phylink PCS > isntead of going behind phylink's back. > > Changes since version 2: > - Adopted some of Serge's feedback. > - New patch: adding ethqos_pcs_set_inband() for qcom-ethqos so we > have one place to modify for AN control rather than many. > - New patch: pass the stmmac_priv structure into the pcs_set_ane() > method. > - New patch: remove pcs_get_adv_lp() early, as this is only for TBI > and RTBI, support for which we dropped in an already merged patch. > - Provide stmmac_pcs structure to encapsulate the pointer to > stmmac_priv, PCS MMIO address pointer and phylink_pcs structure. > - Restructure dwmac_pcs_config() so we can eventually share code > with dwmac_ctrl_ane(). > - New patch: move dwmac_ctrl_ane() into stmmac_pcs.c, and share code. > - New patch: pass the stmmac_pcs structure into dwmac_pcs_isr(). > - New patch: similar to Serge's patch, rename the PCS registers, but > use STMMAC_PCS_ as the prefix rather than just PCS_ which is too > generic. > - New patch: incorporate "net: stmmac: Activate Inband/PCS flag > based on the selected iface" from Serge. > > On the subject of whether we should have two PCS instances, I > experimented with that and have now decided against it. Instead, > dwmac_pcs_config() now tests whether we need to fiddle with the > PCS control register or not. > > Note that I prefer not to have multiple layers of indirection, but > instead prefer a library-style approach, which is why I haven't > turned the PCS support into something that's self contained with > a method in the MAC driver to grab the RGSMII status. I understand the reason of your choice in this case. As a result a some part of my changes haven't been merged in into your series. But I deliberately selected the approach with having the simple PCS HW-interface callbacks utilized for a self-contained internal PCS implementation. Here is why: 1. Signify that the DW GMAC and DW QoS Eth internal PCSs are the same. 2. Reduce the amount of code. 3. Collects the entire PCS implementation in a single place which improves the code readability. 4. The PCS ops initialization is implemented in the same way as the PTP, MMC and EST (and likely FPE in some time in future), in the hwif.c and the interface/core callbacks in the dedicated files (stmmac_ptp.c, mmc_core.c, stmmac_est.c, etc). So the PCS implementation would be in general unified with what has been done for PTP/MMC/EST/etc. 5. ... Taking that into account I am still convinced that my approach worth to be implemented. Hope you won't mind, if after your series is merged in I'll submit another patch set which would introduce some of my PCS-changes not included into your patch set. Like this: 1. Move the mac_device_info instance to being defined in the stmmac_priv structure (new patch, so to drop the stmmac_priv pointer from stmmac_pcs). 2. Introduce stmmac_priv::pcsaddr (to have the PCS CSR base address defined in the same way as for PTP/MMC/EST/etc). 3. Provide the HWIF ops: stmmac_pcs_ops { pcs_get_config_reg; pcs_enable_irq; pcs_disable_irq; } for DW GMAC and DW QoS Eth. 4. Move PCS implementation to stmmac_pcs.c 5. Direct using the plat_stmmacenet_data::mac_port_sel_speed field instead of the mac_device_info::ps. 6. Some more cleanups like converting the struct stmmac_hwif_entry field from void-pointers to the typed-pointers, ... -Serge(y) > > ----- > > This is version 2 of the series switching stmmac to use phylink PCS > instead of going behind phylink's back. > > Changes since version 1: > - Addition of patches from Serge Semin to allow RGMII to use the > "PCS" code even if priv->dma_cap.pcs is not set (including tweaks > by me.) > - Restructuring of the patch set to be a more logical split. > - Leave the pcs_ctrl_ane methods until we've worked out what to do > with the qcom-ethqos driver (this series may still end up breaking > it, but at least we will now successfully compile.) > > A reminder that what I want to hear from this patch set are the results > of testing - and thanks to Serge, the RGMII paths were exercised, but > I have not had any results for the SGMII side of this. > > There are still a bunch of outstanding questions: > > - whether we should be using two separate PCS instances, one for > RGMII and another for SGMII. If the PCS hardware is not present, > but are using RGMII mode, then we probably don't want to be > accessing the registers that would've been there for SGMII. > - what the three interrupts associated with the PCS code actually > mean when they fire. > - which block's status we're reading in the pcs_get_state() method, > and whether we should be reading that for both RGMII and SGMII. > - whether we need to activate phylink's inband mode in more cases > (so that the PCS/MAC status gets read and used for the link.) > > There's probably more questions to be asked... but really the critical > thing is to shake out any breakage from making this conversion. Bear > in mind that I have little knowledge of this hardware, so this > conversion has been done somewhat blind using only what I can observe > from the current driver. > > ------ > > As I noted recently in a thread (and was ignored) stmmac sucks. (I > won't hide my distain for drivers that make my life as phylink > maintainer more difficult!) > > One of the contract conditions for using phylink is that the driver > will _not_ mess with the netif carrier. stmmac developers/maintainers > clearly didn't read that, because stmmac messes with the netif > carrier, which destroys phylink's guarantee that it'll make certain > calls in a particular order (e.g. it won't call mac_link_up() twice > in a row without an intervening mac_link_down().) This is clearly > stated in the phylink documentation. > > Thus, this patch set attempts to fix this. Why does it mess with the > netif carrier? It has its own independent PCS implementation that > completely bypasses phylink _while_ phylink is still being used. > This is not acceptable. Either the driver uses phylink, or it doesn't > use phylink. There is no half-way house about this. Therefore, this > driver needs to either be fixed, or needs to stop using phylink. > > Since I was ignored when I brought this up, I've hacked together the > following patch set - and it is hacky at the moment. It's also broken > because of recentl changes involving dwmac-qcom-ethqos.c - but there > isn't sufficient information in the driver for me to fix this. The > driver appears to use SGMII at 2500Mbps, which simply does not exist. > What interface mode (and neg_mode) does phylink pass to pcs_config() > in each of the speeds that dwmac-qcom-ethqos.c is interested in. > Without this information, I can't do that conversion. So for the > purposes of this, I've just ignored dwmac-qcom-ethqos.c (which means > it will fail to build.) > > The patch splitup is not ideal, but that's not what I'm interested in > here. What I want to hear is the results of testing - does this switch > of the RGMII/SGMII "pcs" stuff to a phylink_pcs work for this driver? > > Please don't review the patches, but you are welcome to send fixes to > them. Once we know that the overall implementation works, then I'll > look at how best to split the patches. In the mean time, the present > form is more convenient for making changes and fixing things. > > There is still more improvement that's needed here. > > Thanks. > > drivers/net/ethernet/stmicro/stmmac/Makefile | 2 +- > drivers/net/ethernet/stmicro/stmmac/common.h | 25 ++-- > .../ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 13 +- > drivers/net/ethernet/stmicro/stmmac/dwmac1000.h | 13 +- > .../net/ethernet/stmicro/stmmac/dwmac1000_core.c | 110 +++++++------- > drivers/net/ethernet/stmicro/stmmac/dwmac4.h | 13 +- > drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c | 99 +++++++------ > drivers/net/ethernet/stmicro/stmmac/hwif.h | 24 ++-- > .../net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 111 +------------- > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 30 +--- > drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c | 63 ++++++++ > drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.h | 160 ++++++++++----------- > 12 files changed, 306 insertions(+), 357 deletions(-) > create mode 100644 drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Tue, Aug 06, 2024 at 09:56:04PM +0300, Serge Semin wrote: > Hi Russell > > Got this series tested on my DW GMAC v3.73a + Micrel KSZ9031RNX PHY > with the in-band link status management enabled. The same positive result > as before, on v1-v2: > [ 294.651324] stmmaceth 1f060000.ethernet eth1: configuring for inband/rgmii-rxid link mode > [ 294.582498] stmmaceth 1f060000.ethernet eth1: Register MEM_TYPE_PAGE_POOL RxQ-0 > [ 294.594308] stmmaceth 1f060000.ethernet eth1: PHY [stmmac-1:03] driver [RTL8211E Gigabit Ethernet] (irq=POLL) > [ 294.605453] dwmac1000: Master AXI performs any burst length > [ 294.611899] stmmaceth 1f060000.ethernet: invalid port speed > [ 294.618229] stmmaceth 1f060000.ethernet eth1: No Safety Features support found > [ 294.626412] stmmaceth 1f060000.ethernet eth1: No MAC Management Counters available > [ 294.634912] stmmaceth 1f060000.ethernet eth1: IEEE 1588-2008 Advanced Timestamp supported > [ 294.644380] stmmaceth 1f060000.ethernet eth1: registered PTP clock > [ 294.651324] stmmaceth 1f060000.ethernet eth1: configuring for inband/rgmii-rxid link mode > ... > [ 298.772917] stmmaceth 1f060000.ethernet eth1: Link is Up - 1Gbps/Full - flow control rx/tx > > So feel free to add: > Tested-by: Serge Semin <fancer.lancer@gmail.com> Thanks. > Please note the warning: "stmmaceth 1f060000.ethernet: invalid port > speed" in the log above. This is a false negative warning since my > network devices isn't of MAC2MAC-type and there is no snps,ps-speed > property in my dts. So having the priv->hw.ps set to zero should be > fine. That said I guess we need to add the warning fix to the 14/14 > patch which would permit the plat_stmmacenet_data::mac_port_sel_speed > field being zero. I think this is a separate issue - one which exists even today with the stmmac driver as this code hasn't changed. Maybe it should be a separate patch targetting the net tree? > > Previous cover messages from earlier posts below: > > > > This is version 3 of the series switching stmmac to use phylink PCS > > isntead of going behind phylink's back. > > > > Changes since version 2: > > - Adopted some of Serge's feedback. > > - New patch: adding ethqos_pcs_set_inband() for qcom-ethqos so we > > have one place to modify for AN control rather than many. > > - New patch: pass the stmmac_priv structure into the pcs_set_ane() > > method. > > - New patch: remove pcs_get_adv_lp() early, as this is only for TBI > > and RTBI, support for which we dropped in an already merged patch. > > - Provide stmmac_pcs structure to encapsulate the pointer to > > stmmac_priv, PCS MMIO address pointer and phylink_pcs structure. > > - Restructure dwmac_pcs_config() so we can eventually share code > > with dwmac_ctrl_ane(). > > - New patch: move dwmac_ctrl_ane() into stmmac_pcs.c, and share code. > > - New patch: pass the stmmac_pcs structure into dwmac_pcs_isr(). > > - New patch: similar to Serge's patch, rename the PCS registers, but > > use STMMAC_PCS_ as the prefix rather than just PCS_ which is too > > generic. > > - New patch: incorporate "net: stmmac: Activate Inband/PCS flag > > based on the selected iface" from Serge. > > > > On the subject of whether we should have two PCS instances, I > > experimented with that and have now decided against it. Instead, > > dwmac_pcs_config() now tests whether we need to fiddle with the > > PCS control register or not. > > > > > Note that I prefer not to have multiple layers of indirection, but > > instead prefer a library-style approach, which is why I haven't > > turned the PCS support into something that's self contained with > > a method in the MAC driver to grab the RGSMII status. > > I understand the reason of your choice in this case. As a result a > some part of my changes haven't been merged in into your series. But I > deliberately selected the approach with having the simple PCS > HW-interface callbacks utilized for a self-contained internal PCS > implementation. Here is why: > 1. Signify that the DW GMAC and DW QoS Eth internal PCSs are the > same. > 2. Reduce the amount of code. > 3. Collects the entire PCS implementation in a single place which > improves the code readability. > 4. The PCS ops initialization is implemented in the same way as the > PTP, MMC and EST (and likely FPE in some time in future), in the > hwif.c and the interface/core callbacks in the dedicated files > (stmmac_ptp.c, mmc_core.c, stmmac_est.c, etc). So the PCS > implementation would be in general unified with what has been done for > PTP/MMC/EST/etc. > 5. ... > > Taking that into account I am still convinced that my approach worth > to be implemented. Hope you won't mind, if after your series is merged > in I'll submit another patch set which would introduce some of my > PCS-changes not included into your patch set. Like this: > 1. Move the mac_device_info instance to being defined in the > stmmac_priv structure (new patch, so to drop the stmmac_priv pointer > from stmmac_pcs). > 2. Introduce stmmac_priv::pcsaddr (to have the PCS CSR base address > defined in the same way as for PTP/MMC/EST/etc). > 3. Provide the HWIF ops: > stmmac_pcs_ops { > pcs_get_config_reg; > pcs_enable_irq; > pcs_disable_irq; > } for DW GMAC and DW QoS Eth. > 4. Move PCS implementation to stmmac_pcs.c > 5. Direct using the plat_stmmacenet_data::mac_port_sel_speed field > instead of the mac_device_info::ps. > 6. Some more cleanups like converting the struct stmmac_hwif_entry > field from void-pointers to the typed-pointers, ... I guessed that you would dig your heals in over this, and want to do it your own way despite all the points I raised against your patch series on my previous posting arguing against much of this. So, at this point I give up with this patch series - clearly there is no room for discussion about the way forward, and you want to do it your way no matter what.
On Wed, Aug 07, 2024 at 10:21:07AM +0100, Russell King (Oracle) wrote: > On Tue, Aug 06, 2024 at 09:56:04PM +0300, Serge Semin wrote: > > Hi Russell > > > > Got this series tested on my DW GMAC v3.73a + Micrel KSZ9031RNX PHY > > with the in-band link status management enabled. The same positive result > > as before, on v1-v2: > > [ 294.651324] stmmaceth 1f060000.ethernet eth1: configuring for inband/rgmii-rxid link mode > > [ 294.582498] stmmaceth 1f060000.ethernet eth1: Register MEM_TYPE_PAGE_POOL RxQ-0 > > [ 294.594308] stmmaceth 1f060000.ethernet eth1: PHY [stmmac-1:03] driver [RTL8211E Gigabit Ethernet] (irq=POLL) > > [ 294.605453] dwmac1000: Master AXI performs any burst length > > [ 294.611899] stmmaceth 1f060000.ethernet: invalid port speed > > [ 294.618229] stmmaceth 1f060000.ethernet eth1: No Safety Features support found > > [ 294.626412] stmmaceth 1f060000.ethernet eth1: No MAC Management Counters available > > [ 294.634912] stmmaceth 1f060000.ethernet eth1: IEEE 1588-2008 Advanced Timestamp supported > > [ 294.644380] stmmaceth 1f060000.ethernet eth1: registered PTP clock > > [ 294.651324] stmmaceth 1f060000.ethernet eth1: configuring for inband/rgmii-rxid link mode > > ... > > [ 298.772917] stmmaceth 1f060000.ethernet eth1: Link is Up - 1Gbps/Full - flow control rx/tx > > > > So feel free to add: > > Tested-by: Serge Semin <fancer.lancer@gmail.com> > > Thanks. > > > Please note the warning: "stmmaceth 1f060000.ethernet: invalid port > > speed" in the log above. This is a false negative warning since my > > network devices isn't of MAC2MAC-type and there is no snps,ps-speed > > property in my dts. So having the priv->hw.ps set to zero should be > > fine. That said I guess we need to add the warning fix to the 14/14 > > patch which would permit the plat_stmmacenet_data::mac_port_sel_speed > > field being zero. > > I think this is a separate issue - one which exists even today with > the stmmac driver as this code hasn't changed. Maybe it should be a > separate patch targetting the net tree? Ok. Tomorrow I'll submit the patch fixing that case. > > > > Previous cover messages from earlier posts below: > > > > > > This is version 3 of the series switching stmmac to use phylink PCS > > > isntead of going behind phylink's back. > > > > > > Changes since version 2: > > > - Adopted some of Serge's feedback. > > > - New patch: adding ethqos_pcs_set_inband() for qcom-ethqos so we > > > have one place to modify for AN control rather than many. > > > - New patch: pass the stmmac_priv structure into the pcs_set_ane() > > > method. > > > - New patch: remove pcs_get_adv_lp() early, as this is only for TBI > > > and RTBI, support for which we dropped in an already merged patch. > > > - Provide stmmac_pcs structure to encapsulate the pointer to > > > stmmac_priv, PCS MMIO address pointer and phylink_pcs structure. > > > - Restructure dwmac_pcs_config() so we can eventually share code > > > with dwmac_ctrl_ane(). > > > - New patch: move dwmac_ctrl_ane() into stmmac_pcs.c, and share code. > > > - New patch: pass the stmmac_pcs structure into dwmac_pcs_isr(). > > > - New patch: similar to Serge's patch, rename the PCS registers, but > > > use STMMAC_PCS_ as the prefix rather than just PCS_ which is too > > > generic. > > > - New patch: incorporate "net: stmmac: Activate Inband/PCS flag > > > based on the selected iface" from Serge. > > > > > > On the subject of whether we should have two PCS instances, I > > > experimented with that and have now decided against it. Instead, > > > dwmac_pcs_config() now tests whether we need to fiddle with the > > > PCS control register or not. > > > > > > > > Note that I prefer not to have multiple layers of indirection, but > > > instead prefer a library-style approach, which is why I haven't > > > turned the PCS support into something that's self contained with > > > a method in the MAC driver to grab the RGSMII status. > > > > I understand the reason of your choice in this case. As a result a > > some part of my changes haven't been merged in into your series. But I > > deliberately selected the approach with having the simple PCS > > HW-interface callbacks utilized for a self-contained internal PCS > > implementation. Here is why: > > 1. Signify that the DW GMAC and DW QoS Eth internal PCSs are the > > same. > > 2. Reduce the amount of code. > > 3. Collects the entire PCS implementation in a single place which > > improves the code readability. > > 4. The PCS ops initialization is implemented in the same way as the > > PTP, MMC and EST (and likely FPE in some time in future), in the > > hwif.c and the interface/core callbacks in the dedicated files > > (stmmac_ptp.c, mmc_core.c, stmmac_est.c, etc). So the PCS > > implementation would be in general unified with what has been done for > > PTP/MMC/EST/etc. > > 5. ... > > > > Taking that into account I am still convinced that my approach worth > > to be implemented. Hope you won't mind, if after your series is merged > > in I'll submit another patch set which would introduce some of my > > PCS-changes not included into your patch set. Like this: > > 1. Move the mac_device_info instance to being defined in the > > stmmac_priv structure (new patch, so to drop the stmmac_priv pointer > > from stmmac_pcs). > > 2. Introduce stmmac_priv::pcsaddr (to have the PCS CSR base address > > defined in the same way as for PTP/MMC/EST/etc). > > 3. Provide the HWIF ops: > > stmmac_pcs_ops { > > pcs_get_config_reg; > > pcs_enable_irq; > > pcs_disable_irq; > > } for DW GMAC and DW QoS Eth. > > 4. Move PCS implementation to stmmac_pcs.c > > 5. Direct using the plat_stmmacenet_data::mac_port_sel_speed field > > instead of the mac_device_info::ps. > > 6. Some more cleanups like converting the struct stmmac_hwif_entry > > field from void-pointers to the typed-pointers, ... > > I guessed that you would dig your heals in over this, and want to do > it your own way despite all the points I raised against your patch > series on my previous posting arguing against much of this. > > So, at this point I give up with this patch series - clearly there is > no room for discussion about the way forward, and you want to do it > your way no matter what. I actually thought that in general the approach implemented in my patches didn't meet much dislikes from your side. Just several notes which could be easily fixed in the next revisions. Anyway thanks for understanding. I'll wait for your series to be merged in. Then I'll submit my patch set based on top of it (of course taking into account all the notes raised by you back then). -Serge(y) > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Thu, Aug 08, 2024 at 11:42:53PM GMT, Serge Semin wrote: > On Wed, Aug 07, 2024 at 10:21:07AM +0100, Russell King (Oracle) wrote: > > On Tue, Aug 06, 2024 at 09:56:04PM +0300, Serge Semin wrote: > > > Hi Russell > > > ... > > > I guessed that you would dig your heals in over this, and want to do > > it your own way despite all the points I raised against your patch > > series on my previous posting arguing against much of this. > > > > So, at this point I give up with this patch series - clearly there is > > no room for discussion about the way forward, and you want to do it > > your way no matter what. > > I actually thought that in general the approach implemented in my > patches didn't meet much dislikes from your side. Just several notes > which could be easily fixed in the next revisions. > > Anyway thanks for understanding. I'll wait for your series to be > merged in. Then I'll submit my patch set based on top of it (of course > taking into account all the notes raised by you back then). > Hmmm, I'll poke the bears :) Any chance this series will be rebased and sent out again? I really liked the direction of this and it seems a waste to end it at a stalemate here despite some differing opinions on the design and possible future changes. I think we're all in agreement that stmmac's current PCS usage behind phylink's back is not good, and this is a massive improvement. Thanks, Andrew
> Hmmm, I'll poke the bears :)
Russell is away on 'medical leave', cataract surgery. It probably
makes sense to wait until he is back.
Andrew
On Thu, Sep 05, 2024 at 11:00:31PM GMT, Andrew Lunn wrote: > > Hmmm, I'll poke the bears :) > > Russell is away on 'medical leave', cataract surgery. It probably > makes sense to wait until he is back. > Ahh yes, I forgot about that! Thanks for the reminder. I'll be patient then and hope is surgery and recovery is smooth :) Thanks, Andrew