| Message ID | E1sD0Ov-00EzBu-BC@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: stmmac: convert stmmac "pcs" to phylink | expand |
On Fri, May 31, 2024 at 12:26:25PM GMT, Russell King (Oracle) wrote: > Convert dwmac1000 sgmii/rgmii "pcs" implementation to use a phylink_pcs > so we can eventually get rid of the exceptional paths that conflict > with phylink. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- > .../ethernet/stmicro/stmmac/dwmac1000_core.c | 113 ++++++++++++------ > 1 file changed, 75 insertions(+), 38 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c > index d413d76a8936..4a0572d5f865 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c > @@ -16,6 +16,7 @@ > #include <linux/slab.h> > #include <linux/ethtool.h> > #include <linux/io.h> > +#include <linux/phylink.h> > #include "stmmac.h" > #include "stmmac_pcs.h" > #include "dwmac1000.h" > @@ -261,39 +262,6 @@ static void dwmac1000_pmt(struct mac_device_info *hw, unsigned long mode) > writel(pmt, ioaddr + GMAC_PMT); > } > > -/* RGMII or SMII interface */ > -static void dwmac1000_rgsmii(void __iomem *ioaddr, struct stmmac_extra_stats *x) > -{ > - u32 status; > - > - status = readl(ioaddr + GMAC_RGSMIIIS); > - x->irq_rgmii_n++; > - > - /* Check the link status */ > - if (status & GMAC_RGSMIIIS_LNKSTS) { > - int speed_value; > - > - x->pcs_link = 1; > - > - speed_value = ((status & GMAC_RGSMIIIS_SPEED) >> > - GMAC_RGSMIIIS_SPEED_SHIFT); > - if (speed_value == GMAC_RGSMIIIS_SPEED_125) > - x->pcs_speed = SPEED_1000; > - else if (speed_value == GMAC_RGSMIIIS_SPEED_25) > - x->pcs_speed = SPEED_100; > - else > - x->pcs_speed = SPEED_10; > - > - x->pcs_duplex = (status & GMAC_RGSMIIIS_LNKMOD_MASK); > - > - pr_info("Link is Up - %d/%s\n", (int)x->pcs_speed, > - x->pcs_duplex ? "Full" : "Half"); > - } else { > - x->pcs_link = 0; > - pr_info("Link is Down\n"); > - } > -} > - > static int dwmac1000_irq_status(struct mac_device_info *hw, > struct stmmac_extra_stats *x) > { > @@ -335,8 +303,12 @@ static int dwmac1000_irq_status(struct mac_device_info *hw, > > dwmac_pcs_isr(ioaddr, GMAC_PCS_BASE, intr_status, x); > > - if (intr_status & PCS_RGSMIIIS_IRQ) > - dwmac1000_rgsmii(ioaddr, x); > + if (intr_status & PCS_RGSMIIIS_IRQ) { > + /* TODO Dummy-read to clear the IRQ status */ > + readl(ioaddr + GMAC_RGSMIIIS); This seems to me that you're doing the TODO here? Maybe I'm misunderstanding... maybe not :) > + phylink_pcs_change(&hw->mac_pcs, false); > + x->irq_rgmii_n++; > + } > > return ret; > } > @@ -404,9 +376,71 @@ static void dwmac1000_ctrl_ane(void __iomem *ioaddr, bool ane, bool srgmi_ral, > dwmac_ctrl_ane(ioaddr, GMAC_PCS_BASE, ane, srgmi_ral, loopback); > } > > -static void dwmac1000_get_adv_lp(void __iomem *ioaddr, struct rgmii_adv *adv) > +static int dwmac1000_mii_pcs_validate(struct phylink_pcs *pcs, > + unsigned long *supported, > + const struct phylink_link_state *state) > +{ > + /* Only support in-band */ > + if (!test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, state->advertising)) > + return -EINVAL; > + > + return 0; > +} > + > +static int dwmac1000_mii_pcs_config(struct phylink_pcs *pcs, > + unsigned int neg_mode, > + phy_interface_t interface, > + const unsigned long *advertising, > + bool permit_pause_to_mac) > { > - dwmac_get_adv_lp(ioaddr, GMAC_PCS_BASE, adv); > + struct mac_device_info *hw = phylink_pcs_to_mac_dev_info(pcs); > + > + return dwmac_pcs_config(hw, neg_mode, interface, advertising, > + GMAC_PCS_BASE); > +} > + > +static void dwmac1000_mii_pcs_get_state(struct phylink_pcs *pcs, > + struct phylink_link_state *state) > +{ > + struct mac_device_info *hw = phylink_pcs_to_mac_dev_info(pcs); > + unsigned int spd_clk; > + u32 status; > + > + status = readl(hw->pcsr + GMAC_RGSMIIIS); > + > + state->link = status & GMAC_RGSMIIIS_LNKSTS; > + if (!state->link) > + return; > + > + spd_clk = FIELD_GET(GMAC_RGSMIIIS_SPEED, status); > + if (spd_clk == GMAC_RGSMIIIS_SPEED_125) > + state->speed = SPEED_1000; > + else if (spd_clk == GMAC_RGSMIIIS_SPEED_25) > + state->speed = SPEED_100; > + else if (spd_clk == GMAC_RGSMIIIS_SPEED_2_5) > + state->speed = SPEED_10; > + > + state->duplex = status & GMAC_RGSMIIIS_LNKMOD_MASK ? > + DUPLEX_FULL : DUPLEX_HALF; > + > + dwmac_pcs_get_state(hw, state, GMAC_PCS_BASE); > +} > + > +static const struct phylink_pcs_ops dwmac1000_mii_pcs_ops = { > + .pcs_validate = dwmac1000_mii_pcs_validate, > + .pcs_config = dwmac1000_mii_pcs_config, > + .pcs_get_state = dwmac1000_mii_pcs_get_state, > +}; > + > +static struct phylink_pcs * > +dwmac1000_phylink_select_pcs(struct stmmac_priv *priv, > + phy_interface_t interface) > +{ > + if (priv->hw->pcs & STMMAC_PCS_RGMII || > + priv->hw->pcs & STMMAC_PCS_SGMII) > + return &priv->hw->mac_pcs; > + > + return NULL; > } > > static void dwmac1000_debug(struct stmmac_priv *priv, void __iomem *ioaddr, > @@ -499,6 +533,7 @@ static void dwmac1000_set_mac_loopback(void __iomem *ioaddr, bool enable) > > const struct stmmac_ops dwmac1000_ops = { > .core_init = dwmac1000_core_init, > + .phylink_select_pcs = dwmac1000_phylink_select_pcs, > .set_mac = stmmac_set_mac, > .rx_ipc = dwmac1000_rx_ipc_enable, > .dump_regs = dwmac1000_dump_regs, > @@ -514,7 +549,6 @@ const struct stmmac_ops dwmac1000_ops = { > .set_eee_pls = dwmac1000_set_eee_pls, > .debug = dwmac1000_debug, > .pcs_ctrl_ane = dwmac1000_ctrl_ane, > - .pcs_get_adv_lp = dwmac1000_get_adv_lp, > .set_mac_loopback = dwmac1000_set_mac_loopback, > }; > > @@ -549,5 +583,8 @@ int dwmac1000_setup(struct stmmac_priv *priv) > mac->mii.clk_csr_shift = 2; > mac->mii.clk_csr_mask = GENMASK(5, 2); > > + mac->mac_pcs.ops = &dwmac1000_mii_pcs_ops; > + mac->mac_pcs.neg_mode = true; > + > return 0; > } > -- > 2.30.2 >
On Wed, Jun 05, 2024 at 03:05:43PM GMT, Andrew Halaney wrote: > On Fri, May 31, 2024 at 12:26:25PM GMT, Russell King (Oracle) wrote: > > Convert dwmac1000 sgmii/rgmii "pcs" implementation to use a phylink_pcs > > so we can eventually get rid of the exceptional paths that conflict > > with phylink. > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > --- > > .../ethernet/stmicro/stmmac/dwmac1000_core.c | 113 ++++++++++++------ > > 1 file changed, 75 insertions(+), 38 deletions(-) > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c > > index d413d76a8936..4a0572d5f865 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c > > @@ -16,6 +16,7 @@ > > #include <linux/slab.h> > > #include <linux/ethtool.h> > > #include <linux/io.h> > > +#include <linux/phylink.h> > > #include "stmmac.h" > > #include "stmmac_pcs.h" > > #include "dwmac1000.h" > > @@ -261,39 +262,6 @@ static void dwmac1000_pmt(struct mac_device_info *hw, unsigned long mode) > > writel(pmt, ioaddr + GMAC_PMT); > > } > > > > -/* RGMII or SMII interface */ > > -static void dwmac1000_rgsmii(void __iomem *ioaddr, struct stmmac_extra_stats *x) > > -{ > > - u32 status; > > - > > - status = readl(ioaddr + GMAC_RGSMIIIS); > > - x->irq_rgmii_n++; > > - > > - /* Check the link status */ > > - if (status & GMAC_RGSMIIIS_LNKSTS) { > > - int speed_value; > > - > > - x->pcs_link = 1; > > - > > - speed_value = ((status & GMAC_RGSMIIIS_SPEED) >> > > - GMAC_RGSMIIIS_SPEED_SHIFT); > > - if (speed_value == GMAC_RGSMIIIS_SPEED_125) > > - x->pcs_speed = SPEED_1000; > > - else if (speed_value == GMAC_RGSMIIIS_SPEED_25) > > - x->pcs_speed = SPEED_100; > > - else > > - x->pcs_speed = SPEED_10; > > - > > - x->pcs_duplex = (status & GMAC_RGSMIIIS_LNKMOD_MASK); > > - > > - pr_info("Link is Up - %d/%s\n", (int)x->pcs_speed, > > - x->pcs_duplex ? "Full" : "Half"); > > - } else { > > - x->pcs_link = 0; > > - pr_info("Link is Down\n"); > > - } > > -} > > - > > static int dwmac1000_irq_status(struct mac_device_info *hw, > > struct stmmac_extra_stats *x) > > { > > @@ -335,8 +303,12 @@ static int dwmac1000_irq_status(struct mac_device_info *hw, > > > > dwmac_pcs_isr(ioaddr, GMAC_PCS_BASE, intr_status, x); > > > > - if (intr_status & PCS_RGSMIIIS_IRQ) > > - dwmac1000_rgsmii(ioaddr, x); > > + if (intr_status & PCS_RGSMIIIS_IRQ) { > > + /* TODO Dummy-read to clear the IRQ status */ > > + readl(ioaddr + GMAC_RGSMIIIS); > > This seems to me that you're doing the TODO here? Maybe I'm > misunderstanding... maybe not :) > > > + phylink_pcs_change(&hw->mac_pcs, false); Continuing to read through this all, sorry for the double reply and possibly dumb question. Should we be passing in false unconditionally here? Prior code had checked GMAC_RGSMIIIS & GMAC_RGSMIIIS_LNKSTS to determine if the link was up/down, which seem logical to pass in here too. Reading the kerneldoc for phylink_pcs_change() I'm not totally positive if we're in the "otherwise pass false" state here or some other detail I'm missing though (it seems that maybe we're just sort of kicking the can to phylink_resolve() which ends up calling into the get_state() callback?) > > + x->irq_rgmii_n++; > > + } > > > > return ret; > > } > > @@ -404,9 +376,71 @@ static void dwmac1000_ctrl_ane(void __iomem *ioaddr, bool ane, bool srgmi_ral, > > dwmac_ctrl_ane(ioaddr, GMAC_PCS_BASE, ane, srgmi_ral, loopback); > > } > > > > -static void dwmac1000_get_adv_lp(void __iomem *ioaddr, struct rgmii_adv *adv) > > +static int dwmac1000_mii_pcs_validate(struct phylink_pcs *pcs, > > + unsigned long *supported, > > + const struct phylink_link_state *state) > > +{ > > + /* Only support in-band */ > > + if (!test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, state->advertising)) > > + return -EINVAL; > > + > > + return 0; > > +} > > + > > +static int dwmac1000_mii_pcs_config(struct phylink_pcs *pcs, > > + unsigned int neg_mode, > > + phy_interface_t interface, > > + const unsigned long *advertising, > > + bool permit_pause_to_mac) > > { > > - dwmac_get_adv_lp(ioaddr, GMAC_PCS_BASE, adv); > > + struct mac_device_info *hw = phylink_pcs_to_mac_dev_info(pcs); > > + > > + return dwmac_pcs_config(hw, neg_mode, interface, advertising, > > + GMAC_PCS_BASE); > > +} > > + > > +static void dwmac1000_mii_pcs_get_state(struct phylink_pcs *pcs, > > + struct phylink_link_state *state) > > +{ > > + struct mac_device_info *hw = phylink_pcs_to_mac_dev_info(pcs); > > + unsigned int spd_clk; > > + u32 status; > > + > > + status = readl(hw->pcsr + GMAC_RGSMIIIS); > > + > > + state->link = status & GMAC_RGSMIIIS_LNKSTS; > > + if (!state->link) > > + return; > > + > > + spd_clk = FIELD_GET(GMAC_RGSMIIIS_SPEED, status); > > + if (spd_clk == GMAC_RGSMIIIS_SPEED_125) > > + state->speed = SPEED_1000; > > + else if (spd_clk == GMAC_RGSMIIIS_SPEED_25) > > + state->speed = SPEED_100; > > + else if (spd_clk == GMAC_RGSMIIIS_SPEED_2_5) > > + state->speed = SPEED_10; > > + > > + state->duplex = status & GMAC_RGSMIIIS_LNKMOD_MASK ? > > + DUPLEX_FULL : DUPLEX_HALF; > > + > > + dwmac_pcs_get_state(hw, state, GMAC_PCS_BASE); > > +} > > + > > +static const struct phylink_pcs_ops dwmac1000_mii_pcs_ops = { > > + .pcs_validate = dwmac1000_mii_pcs_validate, > > + .pcs_config = dwmac1000_mii_pcs_config, > > + .pcs_get_state = dwmac1000_mii_pcs_get_state, > > +}; > > + > > +static struct phylink_pcs * > > +dwmac1000_phylink_select_pcs(struct stmmac_priv *priv, > > + phy_interface_t interface) > > +{ > > + if (priv->hw->pcs & STMMAC_PCS_RGMII || > > + priv->hw->pcs & STMMAC_PCS_SGMII) > > + return &priv->hw->mac_pcs; > > + > > + return NULL; > > } > > > > static void dwmac1000_debug(struct stmmac_priv *priv, void __iomem *ioaddr, > > @@ -499,6 +533,7 @@ static void dwmac1000_set_mac_loopback(void __iomem *ioaddr, bool enable) > > > > const struct stmmac_ops dwmac1000_ops = { > > .core_init = dwmac1000_core_init, > > + .phylink_select_pcs = dwmac1000_phylink_select_pcs, > > .set_mac = stmmac_set_mac, > > .rx_ipc = dwmac1000_rx_ipc_enable, > > .dump_regs = dwmac1000_dump_regs, > > @@ -514,7 +549,6 @@ const struct stmmac_ops dwmac1000_ops = { > > .set_eee_pls = dwmac1000_set_eee_pls, > > .debug = dwmac1000_debug, > > .pcs_ctrl_ane = dwmac1000_ctrl_ane, > > - .pcs_get_adv_lp = dwmac1000_get_adv_lp, > > .set_mac_loopback = dwmac1000_set_mac_loopback, > > }; > > > > @@ -549,5 +583,8 @@ int dwmac1000_setup(struct stmmac_priv *priv) > > mac->mii.clk_csr_shift = 2; > > mac->mii.clk_csr_mask = GENMASK(5, 2); > > > > + mac->mac_pcs.ops = &dwmac1000_mii_pcs_ops; > > + mac->mac_pcs.neg_mode = true; > > + > > return 0; > > } > > -- > > 2.30.2 > >
On Wed, Jun 05, 2024 at 03:05:43PM -0500, Andrew Halaney wrote: > On Fri, May 31, 2024 at 12:26:25PM GMT, Russell King (Oracle) wrote: > > @@ -335,8 +303,12 @@ static int dwmac1000_irq_status(struct mac_device_info *hw, > > > > dwmac_pcs_isr(ioaddr, GMAC_PCS_BASE, intr_status, x); > > > > - if (intr_status & PCS_RGSMIIIS_IRQ) > > - dwmac1000_rgsmii(ioaddr, x); > > + if (intr_status & PCS_RGSMIIIS_IRQ) { > > + /* TODO Dummy-read to clear the IRQ status */ > > + readl(ioaddr + GMAC_RGSMIIIS); > > This seems to me that you're doing the TODO here? Maybe I'm > misunderstanding... maybe not :) Please trim your replies. These two lines come from Serge - please ask him why it's marked as a TODO. I assume he has a reason. Thanks.
On Wed, Jun 05, 2024 at 04:59:14PM -0500, Andrew Halaney wrote: > On Wed, Jun 05, 2024 at 03:05:43PM GMT, Andrew Halaney wrote: > > This seems to me that you're doing the TODO here? Maybe I'm > > misunderstanding... maybe not :) > > > > > + phylink_pcs_change(&hw->mac_pcs, false); > > Continuing to read through this all, sorry for the double reply and > possibly dumb question. Should we be passing in false unconditionally > here? It depends whether there is a way to get the current status of the link without side effects (e.g. where a read clears a latched-low link status.) If that's not possible, then passing "false" is safe provided there aren't any spurious interrupts, since we'll always assume that the link has dropped. If there are spurious interrupts, then the link will go down/up each time there's a spurious interrupt. Even so, that's better than missing a change in the link status which may result in loss of link without manual intervention.
Hi Russell, Andrew On Mon, Jun 10, 2024 at 10:19:39AM +0100, Russell King (Oracle) wrote: > On Wed, Jun 05, 2024 at 03:05:43PM -0500, Andrew Halaney wrote: > > On Fri, May 31, 2024 at 12:26:25PM GMT, Russell King (Oracle) wrote: > > > @@ -335,8 +303,12 @@ static int dwmac1000_irq_status(struct mac_device_info *hw, > > > > > > dwmac_pcs_isr(ioaddr, GMAC_PCS_BASE, intr_status, x); > > > > > > - if (intr_status & PCS_RGSMIIIS_IRQ) > > > - dwmac1000_rgsmii(ioaddr, x); > > > + if (intr_status & PCS_RGSMIIIS_IRQ) { > > > + /* TODO Dummy-read to clear the IRQ status */ > > > + readl(ioaddr + GMAC_RGSMIIIS); > > > > This seems to me that you're doing the TODO here? Maybe I'm > > misunderstanding... maybe not :) > > Please trim your replies. > > These two lines come from Serge - please ask him why it's marked as a > TODO. I assume he has a reason. Thanks. The statement below the "TODO..." comment was supposed to be a quick-fix of the interrupts flood happening due to the uncleared RGSMIIIS IRQ flag. Of course dummy-reading in the IRQ handler with no action required to handle the IRQ wouldn't be the best solution (despite of having the phylink_pcs_change() called), especially seeing there is the dwmac_pcs_isr() method, which name implies the PCS IRQ handling. At least we could have incremented the stmmac_extra_stats::irq_rgmii_n counter in there. So what I meant TODO here was to move the RGSMIIIS IRQ handling in dwmac_pcs_isr(). I know that the dwmac_pcs_isr() method has been created around the cross-IP-cores PCS implementation, but as I mentioned several times the tx_config_reg[15:0] part of the GMAC_RGSMIIIS/MAC_PHYIF_Control_Status registers is the same on both DW GMAC and DW QoS Eth: tx_config_reg[0]: LNKMOD tx_config_reg[1:2]: LNKSPEED tx_config_reg[3]: LNKSTS tx_config_reg[4]: JABTO (Jabber Timeout, specific to SMII) tx_config_reg[5]: FALSCARDET (False Carrier Detected, specific to SMII) Should we have a DW IP-core-specific getter like stmmac_ops::pcs_get_config_reg() which would return the tx_config_reg[15:0] field then we could have cleared the IRQ by just calling it, we could have had the fields generically parsed in the dwmac_pcs_isr() handler and in the phylink_pcs_ops::pcs_get_state(). Thus the entire struct phylink_pcs_ops definition could be moved to the stmmac_pcs.c module simplifying the DW GMAC and DW QoS Eth hardware-dependent code. In this regard there is another change which would be required (and frankly would make the code simpler). Instead of passing the CSRs-base address to the dwmac_pcs_isr()/dwmac_pcs_config()/dwmac_pcs_get_state() methods, we could pre-define the PCS registers base address as it's done for the PTP/MMC/EST implementation in the driver. Here is the brief change description: 1. add stmmac_regs_off::pcs_off field (hwif.h) 2. add stmmac_priv::pcsaddr field (stmmac.h) 3. initialize the stmmac_regs_off::pcs_off field for the DW GMAC and DW QoS Eth IP-cores in the stmmac_hw array (hwif.c) 4. initialize the stmmac_priv::pcsaddr field in the stmmac_hwif_init() method as it's done for stmmac_priv::{ptpaddr,mmcaddr,estaddr}. 5. use the PCS-base address in the stmmac_pcs.c module. As a result (unless I've missed something) we'll be able to move almost the entire internal PCS implementation to the stmmac_pcs.c module (except the tx_config_reg[] data getter). -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 Tue, Jun 11, 2024 at 03:25:14PM +0300, Serge Semin wrote: > Hi Russell, Andrew > > Should we have a DW IP-core-specific getter like > stmmac_ops::pcs_get_config_reg() which would return the > tx_config_reg[15:0] field then we could have cleared the IRQ by just > calling it, we could have had the fields generically > parsed in the dwmac_pcs_isr() handler and in the > phylink_pcs_ops::pcs_get_state(). pcs_get_state() is not supposed to get some cached state, but is supposed to return the real state at the time that it is called. There's a good reason for this - dealing with latched-low link failed indications, it's necessary that pcs_get_state() reports that the link failed if _sometime_ between the last time it was called and the next time the link has failed. So, I'm afraid your idea of simplifying it doesn't sound to me like a good idea.
On Wed, Jun 12, 2024 at 11:04:11PM +0100, Russell King (Oracle) wrote: > On Tue, Jun 11, 2024 at 03:25:14PM +0300, Serge Semin wrote: > > Hi Russell, Andrew > > > > Should we have a DW IP-core-specific getter like > > stmmac_ops::pcs_get_config_reg() which would return the > > tx_config_reg[15:0] field then we could have cleared the IRQ by just > > calling it, we could have had the fields generically > > parsed in the dwmac_pcs_isr() handler and in the > > phylink_pcs_ops::pcs_get_state(). > > pcs_get_state() is not supposed to get some cached state, It won't. > but is > supposed to return the real state at the time that it is called. The idea is to implement the tx_config_reg[15:0] getter for DW GMAC and DW QoS Eth. It will return the current link status retrieved from the GMAC_RGSMIIIS and GMAC_PHYIF_CONTROL_STATUS registers. Like this: GMAC: u16 dwmac1000_pcs_get_config_reg(struct stmmac_priv *priv) { return readl(priv->ioaddr + GMAC_RGSMIIIS); } DW QoS Eth: u16 dwmac1000_pcs_get_config_reg(struct stmmac_priv *priv) { return readl(priv->ioaddr + GMAC_PHYIF_CONTROL_STATUS) >> 16; } Then the dwmac_pcs_isr() can be updated as follows: static inline void dwmac_pcs_isr(void __iomem *ioaddr, u32 reg, unsigned int intr_status, struct stmmac_extra_stats *x) { ... if (intr_status & PCS_RGSMIIIS_IRQ) { x->irq_rgmii_n++; /* The next will clear the PCS_RGSMIIIS_IRQ IRQ * status. (It is done instead of dummy-reading the * GMAC_RGSMIIIS/GMAC_PHYIF_CONTROL_STATUS registers * in the DW GMAC/QoS Eth IRQ handlers.) */ (void)stmmac_pcs_get_config_reg(priv); } } Similarly the dwmac_pcs_get_state() method can now use the stmmac_pcs_get_config_reg() function to retrieve the link state and parse the data in a generic manner. Thus everything what is currently done in dwmac1000_mii_pcs_get_state() and dwmac4_mii_pcs_get_state() could be moved to dwmac_pcs_get_state(). By providing a single STMMAC-driver callback stmmac_ops::pcs_get_config_reg(), we'll be able to define the phylink_pcs_ops::{pcs_validate,pcs_config,pcs_get_state} methods in the stmmac_pcs.c file. > > There's a good reason for this - dealing with latched-low link failed > indications, it's necessary that pcs_get_state() reports that the link > failed if _sometime_ between the last time it was called and the next > time the link has failed. > > So, I'm afraid your idea of simplifying it doesn't sound to me like a > good idea. No caching or latched link state indications. Both the GMAC_RGSMIIIS and GMAC_PHYIF_CONTROL_STATUS registers contain the actual link state retrieved the PHY. stmmac_pcs_get_config_reg() will just return the current link state. Perhaps my suggestion might haven't been well described. Providing the patches with the respective changes shall better describe what was meant. So in a few days I'll submit an incremental patch(es) with the proposed modifications for your series. -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 Fri, Jun 14, 2024 at 12:14:21AM +0300, Serge Semin wrote: > On Wed, Jun 12, 2024 at 11:04:11PM +0100, Russell King (Oracle) wrote: > > On Tue, Jun 11, 2024 at 03:25:14PM +0300, Serge Semin wrote: > > > Hi Russell, Andrew > > > > > > Should we have a DW IP-core-specific getter like > > > stmmac_ops::pcs_get_config_reg() which would return the > > > tx_config_reg[15:0] field then we could have cleared the IRQ by just > > > calling it, we could have had the fields generically > > > parsed in the dwmac_pcs_isr() handler and in the > > > phylink_pcs_ops::pcs_get_state(). > > > > [...] > > > > > There's a good reason for this - dealing with latched-low link failed > > indications, it's necessary that pcs_get_state() reports that the link > > failed if _sometime_ between the last time it was called and the next > > time the link has failed. > > > > So, I'm afraid your idea of simplifying it doesn't sound to me like a > > good idea. > > No caching or latched link state indications. Both the GMAC_RGSMIIIS > and GMAC_PHYIF_CONTROL_STATUS registers contain the actual link state > retrieved the PHY. stmmac_pcs_get_config_reg() will just return the > current link state. > > Perhaps my suggestion might haven't been well described. Providing the > patches with the respective changes shall better describe what was > meant. So in a few days I'll submit an incremental patch(es) with the > proposed modifications for your series The incremental patchset is ready. I need to give it some more tests, then rebase onto the kernel 6.10. It'll be done in one-two days. Thanks -Serge(y) . > > -Serge(y) > > > > > -- > > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Hi Russell On Tue, Jun 18, 2024 at 01:26:52PM +0300, Serge Semin wrote: > On Fri, Jun 14, 2024 at 12:14:21AM +0300, Serge Semin wrote: > > On Wed, Jun 12, 2024 at 11:04:11PM +0100, Russell King (Oracle) wrote: > > > On Tue, Jun 11, 2024 at 03:25:14PM +0300, Serge Semin wrote: > > > > Hi Russell, Andrew > > > > > > > > Should we have a DW IP-core-specific getter like > > > > stmmac_ops::pcs_get_config_reg() which would return the > > > > tx_config_reg[15:0] field then we could have cleared the IRQ by just > > > > calling it, we could have had the fields generically > > > > parsed in the dwmac_pcs_isr() handler and in the > > > > phylink_pcs_ops::pcs_get_state(). > > > > > > > [...] > > > > > > > > There's a good reason for this - dealing with latched-low link failed > > > indications, it's necessary that pcs_get_state() reports that the link > > > failed if _sometime_ between the last time it was called and the next > > > time the link has failed. > > > > > > So, I'm afraid your idea of simplifying it doesn't sound to me like a > > > good idea. > > > > No caching or latched link state indications. Both the GMAC_RGSMIIIS > > and GMAC_PHYIF_CONTROL_STATUS registers contain the actual link state > > retrieved the PHY. stmmac_pcs_get_config_reg() will just return the > > current link state. > > > > Perhaps my suggestion might haven't been well described. Providing the > > patches with the respective changes shall better describe what was > > meant. So in a few days I'll submit an incremental patch(es) with the > > proposed modifications for your series > > The incremental patchset is ready. I need to give it some more > tests, then rebase onto the kernel 6.10. It'll be done in one-two > days. It turned out I has created my series on top of your v1 series. I just finished rebasing it onto v2. The only thing left is to test it out. I'll do that today and then submit the series in-reply to your v2 email thread. Sorry for making you wait once again. -Serge > > Thanks > -Serge(y) > . > > > > -Serge(y) > > > > > > > > -- > > > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > > > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Hi Russel On Mon, Jun 24, 2024 at 04:32:53AM +0300, Serge Semin wrote: > Hi Russell > > On Tue, Jun 18, 2024 at 01:26:52PM +0300, Serge Semin wrote: > > On Fri, Jun 14, 2024 at 12:14:21AM +0300, Serge Semin wrote: > > > On Wed, Jun 12, 2024 at 11:04:11PM +0100, Russell King (Oracle) wrote: > > > > On Tue, Jun 11, 2024 at 03:25:14PM +0300, Serge Semin wrote: > > > > > Hi Russell, Andrew > > > > > > > > > > Should we have a DW IP-core-specific getter like > > > > > stmmac_ops::pcs_get_config_reg() which would return the > > > > > tx_config_reg[15:0] field then we could have cleared the IRQ by just > > > > > calling it, we could have had the fields generically > > > > > parsed in the dwmac_pcs_isr() handler and in the > > > > > phylink_pcs_ops::pcs_get_state(). > > > > > > > > > > [...] > > > > > > > > > > > There's a good reason for this - dealing with latched-low link failed > > > > indications, it's necessary that pcs_get_state() reports that the link > > > > failed if _sometime_ between the last time it was called and the next > > > > time the link has failed. > > > > > > > > So, I'm afraid your idea of simplifying it doesn't sound to me like a > > > > good idea. > > > > > > No caching or latched link state indications. Both the GMAC_RGSMIIIS > > > and GMAC_PHYIF_CONTROL_STATUS registers contain the actual link state > > > retrieved the PHY. stmmac_pcs_get_config_reg() will just return the > > > current link state. > > > > > > Perhaps my suggestion might haven't been well described. Providing the > > > patches with the respective changes shall better describe what was > > > meant. So in a few days I'll submit an incremental patch(es) with the > > > proposed modifications for your series > > > > > The incremental patchset is ready. I need to give it some more > > tests, then rebase onto the kernel 6.10. It'll be done in one-two > > days. > > It turned out I has created my series on top of your v1 series. I just > finished rebasing it onto v2. The only thing left is to test it out. > I'll do that today and then submit the series in-reply to your v2 > email thread. Sorry for making you wait once again. Finally I've done that. https://lore.kernel.org/netdev/20240624132802.14238-1-fancer.lancer@gmail.com The update has turned to be a bit more bulky than I intended, but the resultant code looks neat and small consolidated in the stmmac_pcs.c/stmmac_pcs.h files with just three DW *MAC HW-abstraction callbacks defined in the DW GMAC and DW QoS Eth core modules. I did my best to provide the changes in the incremental manner with no driver breakage. Hopefully I didn't fail that.) As before you can re-shuffle the patches and the change they content whatever you think would be better in the final series. -Serge(y)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c index d413d76a8936..4a0572d5f865 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c @@ -16,6 +16,7 @@ #include <linux/slab.h> #include <linux/ethtool.h> #include <linux/io.h> +#include <linux/phylink.h> #include "stmmac.h" #include "stmmac_pcs.h" #include "dwmac1000.h" @@ -261,39 +262,6 @@ static void dwmac1000_pmt(struct mac_device_info *hw, unsigned long mode) writel(pmt, ioaddr + GMAC_PMT); } -/* RGMII or SMII interface */ -static void dwmac1000_rgsmii(void __iomem *ioaddr, struct stmmac_extra_stats *x) -{ - u32 status; - - status = readl(ioaddr + GMAC_RGSMIIIS); - x->irq_rgmii_n++; - - /* Check the link status */ - if (status & GMAC_RGSMIIIS_LNKSTS) { - int speed_value; - - x->pcs_link = 1; - - speed_value = ((status & GMAC_RGSMIIIS_SPEED) >> - GMAC_RGSMIIIS_SPEED_SHIFT); - if (speed_value == GMAC_RGSMIIIS_SPEED_125) - x->pcs_speed = SPEED_1000; - else if (speed_value == GMAC_RGSMIIIS_SPEED_25) - x->pcs_speed = SPEED_100; - else - x->pcs_speed = SPEED_10; - - x->pcs_duplex = (status & GMAC_RGSMIIIS_LNKMOD_MASK); - - pr_info("Link is Up - %d/%s\n", (int)x->pcs_speed, - x->pcs_duplex ? "Full" : "Half"); - } else { - x->pcs_link = 0; - pr_info("Link is Down\n"); - } -} - static int dwmac1000_irq_status(struct mac_device_info *hw, struct stmmac_extra_stats *x) { @@ -335,8 +303,12 @@ static int dwmac1000_irq_status(struct mac_device_info *hw, dwmac_pcs_isr(ioaddr, GMAC_PCS_BASE, intr_status, x); - if (intr_status & PCS_RGSMIIIS_IRQ) - dwmac1000_rgsmii(ioaddr, x); + if (intr_status & PCS_RGSMIIIS_IRQ) { + /* TODO Dummy-read to clear the IRQ status */ + readl(ioaddr + GMAC_RGSMIIIS); + phylink_pcs_change(&hw->mac_pcs, false); + x->irq_rgmii_n++; + } return ret; } @@ -404,9 +376,71 @@ static void dwmac1000_ctrl_ane(void __iomem *ioaddr, bool ane, bool srgmi_ral, dwmac_ctrl_ane(ioaddr, GMAC_PCS_BASE, ane, srgmi_ral, loopback); } -static void dwmac1000_get_adv_lp(void __iomem *ioaddr, struct rgmii_adv *adv) +static int dwmac1000_mii_pcs_validate(struct phylink_pcs *pcs, + unsigned long *supported, + const struct phylink_link_state *state) +{ + /* Only support in-band */ + if (!test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, state->advertising)) + return -EINVAL; + + return 0; +} + +static int dwmac1000_mii_pcs_config(struct phylink_pcs *pcs, + unsigned int neg_mode, + phy_interface_t interface, + const unsigned long *advertising, + bool permit_pause_to_mac) { - dwmac_get_adv_lp(ioaddr, GMAC_PCS_BASE, adv); + struct mac_device_info *hw = phylink_pcs_to_mac_dev_info(pcs); + + return dwmac_pcs_config(hw, neg_mode, interface, advertising, + GMAC_PCS_BASE); +} + +static void dwmac1000_mii_pcs_get_state(struct phylink_pcs *pcs, + struct phylink_link_state *state) +{ + struct mac_device_info *hw = phylink_pcs_to_mac_dev_info(pcs); + unsigned int spd_clk; + u32 status; + + status = readl(hw->pcsr + GMAC_RGSMIIIS); + + state->link = status & GMAC_RGSMIIIS_LNKSTS; + if (!state->link) + return; + + spd_clk = FIELD_GET(GMAC_RGSMIIIS_SPEED, status); + if (spd_clk == GMAC_RGSMIIIS_SPEED_125) + state->speed = SPEED_1000; + else if (spd_clk == GMAC_RGSMIIIS_SPEED_25) + state->speed = SPEED_100; + else if (spd_clk == GMAC_RGSMIIIS_SPEED_2_5) + state->speed = SPEED_10; + + state->duplex = status & GMAC_RGSMIIIS_LNKMOD_MASK ? + DUPLEX_FULL : DUPLEX_HALF; + + dwmac_pcs_get_state(hw, state, GMAC_PCS_BASE); +} + +static const struct phylink_pcs_ops dwmac1000_mii_pcs_ops = { + .pcs_validate = dwmac1000_mii_pcs_validate, + .pcs_config = dwmac1000_mii_pcs_config, + .pcs_get_state = dwmac1000_mii_pcs_get_state, +}; + +static struct phylink_pcs * +dwmac1000_phylink_select_pcs(struct stmmac_priv *priv, + phy_interface_t interface) +{ + if (priv->hw->pcs & STMMAC_PCS_RGMII || + priv->hw->pcs & STMMAC_PCS_SGMII) + return &priv->hw->mac_pcs; + + return NULL; } static void dwmac1000_debug(struct stmmac_priv *priv, void __iomem *ioaddr, @@ -499,6 +533,7 @@ static void dwmac1000_set_mac_loopback(void __iomem *ioaddr, bool enable) const struct stmmac_ops dwmac1000_ops = { .core_init = dwmac1000_core_init, + .phylink_select_pcs = dwmac1000_phylink_select_pcs, .set_mac = stmmac_set_mac, .rx_ipc = dwmac1000_rx_ipc_enable, .dump_regs = dwmac1000_dump_regs, @@ -514,7 +549,6 @@ const struct stmmac_ops dwmac1000_ops = { .set_eee_pls = dwmac1000_set_eee_pls, .debug = dwmac1000_debug, .pcs_ctrl_ane = dwmac1000_ctrl_ane, - .pcs_get_adv_lp = dwmac1000_get_adv_lp, .set_mac_loopback = dwmac1000_set_mac_loopback, }; @@ -549,5 +583,8 @@ int dwmac1000_setup(struct stmmac_priv *priv) mac->mii.clk_csr_shift = 2; mac->mii.clk_csr_mask = GENMASK(5, 2); + mac->mac_pcs.ops = &dwmac1000_mii_pcs_ops; + mac->mac_pcs.neg_mode = true; + return 0; }
Convert dwmac1000 sgmii/rgmii "pcs" implementation to use a phylink_pcs so we can eventually get rid of the exceptional paths that conflict with phylink. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- .../ethernet/stmicro/stmmac/dwmac1000_core.c | 113 ++++++++++++------ 1 file changed, 75 insertions(+), 38 deletions(-)