| Message ID | 20241009053946.3198805-5-vineeth.karumanchi@amd.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: macb: Add versal2 10GBE device support | expand |
Hello, On Wed, 9 Oct 2024 11:09:45 +0530 Vineeth Karumanchi <vineeth.karumanchi@amd.com> wrote: > HS Mac configuration steps: > - Configure speed and serdes rate bits of USX_CONTROL register from > user specified speed in the device-tree. > - Enable HS Mac for 5G and 10G speeds. > - Reset RX receive path to achieve USX block lock for the > configured serdes rate. > - Wait for USX block lock synchronization. > > Move the initialization instances to macb_usx_pcs_link_up(). > > Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com> [...] > > /* DMA buffer descriptor might be different size > * depends on hardware configuration: > @@ -564,14 +565,59 @@ static void macb_usx_pcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode, > int duplex) > { > struct macb *bp = container_of(pcs, struct macb, phylink_usx_pcs); > - u32 config; > + u32 speed_val, serdes_rate, config; > + bool hs_mac = false; > + > + switch (speed) { > + case SPEED_1000: > + speed_val = HS_SPEED_1000M; > + serdes_rate = MACB_SERDES_RATE_1G; > + break; > + case SPEED_2500: > + speed_val = HS_SPEED_2500M; > + serdes_rate = MACB_SERDES_RATE_2_5G; > + break; > + case SPEED_5000: > + speed_val = HS_SPEED_5000M; > + serdes_rate = MACB_SERDES_RATE_5G; > + hs_mac = true; > + break; You support some new speeds and modes, so you also need to update : - The macb_select_pcs() code, as right now it will return NULL for any mode that isn't 10GBaseR or SGMII, so for 2500/5000 speeds, that probably won't work. And for 1000, the default PCS will be used and not USX - the phylink mac_capabilities, so far 2500 and 5000 speeds aren't reported as supported. - the phylink supported_interfaces, I suppose the IP uses 2500BaseX and 5GBaseT ? or maybe some usxgmii flavors ? > + case SPEED_10000: > + speed_val = HS_SPEED_10000M; > + serdes_rate = MACB_SERDES_RATE_10G; > + hs_mac = true; > + break; > + default: > + netdev_err(bp->dev, "Specified speed not supported\n"); > + return; > + } > + > + /* Enable HS MAC for high speeds */ > + if (hs_mac) { > + config = macb_or_gem_readl(bp, NCR); > + config |= GEM_BIT(ENABLE_HS_MAC); > + macb_or_gem_writel(bp, NCR, config); > + } It looks like you moved the MAC selection between HS MAC and non-HS MAC from the phylink .mac_config to PCS config. This configuration is indeed a MAC-side configuration from what I understand, you shouldn't need to set that in PCS code. Maybe instead, check the interface mode in macb_mac_config, and look if you're in 5GBaseR / 10GBaseR to select the MAC ? Thanks, Maxime
On Wed, Oct 09, 2024 at 11:09:45AM +0530, Vineeth Karumanchi wrote: > HS Mac configuration steps: > - Configure speed and serdes rate bits of USX_CONTROL register from > user specified speed in the device-tree. > - Enable HS Mac for 5G and 10G speeds. > - Reset RX receive path to achieve USX block lock for the > configured serdes rate. > - Wait for USX block lock synchronization. > > Move the initialization instances to macb_usx_pcs_link_up(). It only partly moves stuff there, creating what I can only call a mess which probably doesn't work correctly. Please consider the MAC and PCS as two separate boxes - register settings controlled in one box should not be touched by the other box. For example, macb_mac_config() now does this: old_ncr = ncr = macb_or_gem_readl(bp, NCR); ... } else if (macb_is_gem(bp)) { ... ncr &= ~GEM_BIT(ENABLE_HS_MAC); ... if (old_ncr ^ ncr) macb_or_gem_writel(bp, NCR, ncr); meanwhile: > @@ -564,14 +565,59 @@ static void macb_usx_pcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode, > int duplex) > { > struct macb *bp = container_of(pcs, struct macb, phylink_usx_pcs); ... > + /* Enable HS MAC for high speeds */ > + if (hs_mac) { > + config = macb_or_gem_readl(bp, NCR); > + config |= GEM_BIT(ENABLE_HS_MAC); > + macb_or_gem_writel(bp, NCR, config); > + } Arguably, the time that this would happen is when the interface mode changes which would cause a full reconfiguration and thus both of these functions will be called, but it's not easy to follow that's what is going on here. It also looks like you're messing with MAC registers in the PCS code, setting the MAC speed there. Are the PCS and MAC so integrated together that abstracting the PCS into its own separate code block leads to problems?
Hi Maxime, On 10/9/2024 12:06 PM, Maxime Chevallier wrote: > Hello, > > On Wed, 9 Oct 2024 11:09:45 +0530 > Vineeth Karumanchi <vineeth.karumanchi@amd.com> wrote: > >> HS Mac configuration steps: >> - Configure speed and serdes rate bits of USX_CONTROL register from >> user specified speed in the device-tree. >> - Enable HS Mac for 5G and 10G speeds. >> - Reset RX receive path to achieve USX block lock for the >> configured serdes rate. >> - Wait for USX block lock synchronization. >> >> Move the initialization instances to macb_usx_pcs_link_up(). >> >> Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com> > [...] > >> >> /* DMA buffer descriptor might be different size >> * depends on hardware configuration: >> @@ -564,14 +565,59 @@ static void macb_usx_pcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode, >> int duplex) >> { >> struct macb *bp = container_of(pcs, struct macb, phylink_usx_pcs); >> - u32 config; >> + u32 speed_val, serdes_rate, config; >> + bool hs_mac = false; >> + >> + switch (speed) { >> + case SPEED_1000: >> + speed_val = HS_SPEED_1000M; >> + serdes_rate = MACB_SERDES_RATE_1G; >> + break; >> + case SPEED_2500: >> + speed_val = HS_SPEED_2500M; >> + serdes_rate = MACB_SERDES_RATE_2_5G; >> + break; >> + case SPEED_5000: >> + speed_val = HS_SPEED_5000M; >> + serdes_rate = MACB_SERDES_RATE_5G; >> + hs_mac = true; >> + break; > You support some new speeds and modes, so you also need to update : > > - The macb_select_pcs() code, as right now it will return NULL for any > mode that isn't 10GBaseR or SGMII, so for 2500/5000 speeds, that > probably won't work. And for 1000, the default PCS will be used and not > USX > > - the phylink mac_capabilities, so far 2500 and 5000 speeds aren't > reported as supported. > > - the phylink supported_interfaces, I suppose the IP uses 2500BaseX > and 5GBaseT ? or maybe some usxgmii flavors ? with 10GBaseR, ethtool is showing multiple speed support. ( 1G, 2.5G, 5G and 10G ) The only check I see with 10GbaseR is max speed shouldn't be greater than 10G, which suits the IP requirement. >> + case SPEED_10000: >> + speed_val = HS_SPEED_10000M; >> + serdes_rate = MACB_SERDES_RATE_10G; >> + hs_mac = true; >> + break; >> + default: >> + netdev_err(bp->dev, "Specified speed not supported\n"); >> + return; >> + } >> + >> + /* Enable HS MAC for high speeds */ >> + if (hs_mac) { >> + config = macb_or_gem_readl(bp, NCR); >> + config |= GEM_BIT(ENABLE_HS_MAC); >> + macb_or_gem_writel(bp, NCR, config); >> + } > It looks like you moved the MAC selection between HS MAC and non-HS MAC > from the phylink .mac_config to PCS config. > > This configuration is indeed a MAC-side configuration from what I > understand, you shouldn't need to set that in PCS code. Maybe instead, > check the interface mode in macb_mac_config, and look if you're in > 5GBaseR / 10GBaseR to select the MAC ? Yes, agreed! As our current HW setup doesn't have AN and PHY, to support speed change using ethtool I have moved the above MAC config into PCS. I will explore on setting MAC config based on speed instead of interface in macb_mac_config(). Please let me know your thoughts and comments.
Hi Russel, On 10/9/2024 2:49 PM, Russell King (Oracle) wrote: > On Wed, Oct 09, 2024 at 11:09:45AM +0530, Vineeth Karumanchi wrote: >> HS Mac configuration steps: >> - Configure speed and serdes rate bits of USX_CONTROL register from >> user specified speed in the device-tree. >> - Enable HS Mac for 5G and 10G speeds. >> - Reset RX receive path to achieve USX block lock for the >> configured serdes rate. >> - Wait for USX block lock synchronization. >> >> Move the initialization instances to macb_usx_pcs_link_up(). > It only partly moves stuff there, creating what I can only call a mess > which probably doesn't work correctly. > > Please consider the MAC and PCS as two separate boxes - register > settings controlled in one box should not be touched by the other box. > > For example, macb_mac_config() now does this: > > old_ncr = ncr = macb_or_gem_readl(bp, NCR); > ... > } else if (macb_is_gem(bp)) { > ... > ncr &= ~GEM_BIT(ENABLE_HS_MAC); > ... > if (old_ncr ^ ncr) > macb_or_gem_writel(bp, NCR, ncr); > > meanwhile: > >> @@ -564,14 +565,59 @@ static void macb_usx_pcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode, >> int duplex) >> { >> struct macb *bp = container_of(pcs, struct macb, phylink_usx_pcs); > ... >> + /* Enable HS MAC for high speeds */ >> + if (hs_mac) { >> + config = macb_or_gem_readl(bp, NCR); >> + config |= GEM_BIT(ENABLE_HS_MAC); >> + macb_or_gem_writel(bp, NCR, config); >> + } > Arguably, the time that this would happen is when the interface mode > changes which would cause a full reconfiguration and thus both of > these functions will be called, but it's not easy to follow that's > what is going on here. > > It also looks like you're messing with MAC registers in the PCS code, > setting the MAC speed there. Are the PCS and MAC so integrated together > that abstracting the PCS into its own separate code block leads to > problems? Agreed, Since our current hardware configuration lacks AN and PHY, I've relocated the ENABLE_HS_MAC configuration into PCS to allow speed changes using ethtool. When more hardware with a PHY that supports AN becomes available, the phylink will invoke macb_mac_config() with the communicated speed (phylinkstate->speed). It is possible to make ENABLE_HS_MAC conditional based on speed. Currently, for fixed-link, will keep the earlier implementation. Please let me know your thoughts and comments.
On Thu, Oct 10, 2024 at 07:39:16PM +0530, Karumanchi, Vineeth wrote: > Hi Russel, > > On 10/9/2024 2:49 PM, Russell King (Oracle) wrote: > > It also looks like you're messing with MAC registers in the PCS code, > > setting the MAC speed there. Are the PCS and MAC so integrated together > > that abstracting the PCS into its own separate code block leads to > > problems? > > Agreed, Since our current hardware configuration lacks AN and PHY, I've > relocated the ENABLE_HS_MAC configuration into PCS to > allow speed changes using ethtool. When more hardware with a PHY that > supports AN becomes available, > the phylink will invoke macb_mac_config() with the communicated speed > (phylinkstate->speed). Where are you getting that idea from, because that has not been true for a good number of years - and it's been stated in the phylink documentation for a very long time. I've killed all the code references to ->speed in all mac_config() implementations, and I've even gone to the extent of now ensuring that all mac_config() methods will _always_ be called with state->speed set to SPEED_UNKNOWN, so no one can make any useful determinations from that. If people continue to insist on using this, then I'll have no option but to make a disruptive API change, making mac_config() take an explicit set of arguments for the items that it should have access to. > Currently, for fixed-link, will keep the earlier implementation. I want phylink users to be correct and easy to understand - because I maintain phylink, and that means I need to understand the code that makes use of its facilities. So, want to see phylink methods implemented properly. If they aren't going to be implemented properly, then I will ask that the driver ceases to use phylink quite simply because it makes _my_ maintenance more difficult when drivers don't implement phylink methods correctly. The choice is: implement phylink methods properly or don't use phylink.
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h index 47e80fa72865..ed4edeac3a59 100644 --- a/drivers/net/ethernet/cadence/macb.h +++ b/drivers/net/ethernet/cadence/macb.h @@ -825,6 +825,7 @@ }) #define MACB_READ_NSR(bp) macb_readl(bp, NSR) +#define MACB_READ_USX_STATUS(bp) gem_readl(bp, USX_STATUS) /* struct macb_dma_desc - Hardware DMA descriptor * @addr: DMA address of data buffer diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index 3f9dc0b037c0..7beb775a0bd7 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -94,6 +94,7 @@ struct sifive_fu540_macb_mgmt { #define MACB_PM_TIMEOUT 100 /* ms */ #define MACB_MDIO_TIMEOUT 1000000 /* in usecs */ +#define GEM_SYNC_TIMEOUT 2500000 /* in usecs */ /* DMA buffer descriptor might be different size * depends on hardware configuration: @@ -564,14 +565,59 @@ static void macb_usx_pcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode, int duplex) { struct macb *bp = container_of(pcs, struct macb, phylink_usx_pcs); - u32 config; + u32 speed_val, serdes_rate, config; + bool hs_mac = false; + + switch (speed) { + case SPEED_1000: + speed_val = HS_SPEED_1000M; + serdes_rate = MACB_SERDES_RATE_1G; + break; + case SPEED_2500: + speed_val = HS_SPEED_2500M; + serdes_rate = MACB_SERDES_RATE_2_5G; + break; + case SPEED_5000: + speed_val = HS_SPEED_5000M; + serdes_rate = MACB_SERDES_RATE_5G; + hs_mac = true; + break; + case SPEED_10000: + speed_val = HS_SPEED_10000M; + serdes_rate = MACB_SERDES_RATE_10G; + hs_mac = true; + break; + default: + netdev_err(bp->dev, "Specified speed not supported\n"); + return; + } + + /* Enable HS MAC for high speeds */ + if (hs_mac) { + config = macb_or_gem_readl(bp, NCR); + config |= GEM_BIT(ENABLE_HS_MAC); + macb_or_gem_writel(bp, NCR, config); + } + + /* Configure HS MAC for specified speed */ + config = gem_readl(bp, HS_MAC_CONFIG); + config = GEM_BFINS(HS_MAC_SPEED, speed_val, config); + gem_writel(bp, HS_MAC_CONFIG, config); config = gem_readl(bp, USX_CONTROL); - config = GEM_BFINS(SERDES_RATE, MACB_SERDES_RATE_10G, config); - config = GEM_BFINS(USX_CTRL_SPEED, HS_SPEED_10000M, config); + config = GEM_BFINS(SERDES_RATE, serdes_rate, config); + config = GEM_BFINS(USX_CTRL_SPEED, speed_val, config); config &= ~(GEM_BIT(TX_SCR_BYPASS) | GEM_BIT(RX_SCR_BYPASS)); + config |= GEM_BIT(RX_SYNC_RESET); + gem_writel(bp, USX_CONTROL, config); + mdelay(250); + config &= ~GEM_BIT(RX_SYNC_RESET); config |= GEM_BIT(TX_EN); gem_writel(bp, USX_CONTROL, config); + + if (readx_poll_timeout(MACB_READ_USX_STATUS, bp, config, config & GEM_BIT(USX_BLOCK_LOCK), + 1, GEM_SYNC_TIMEOUT)) + netdev_err(bp->dev, "USX PCS block lock not achieved\n"); } static void macb_usx_pcs_get_state(struct phylink_pcs *pcs, @@ -662,7 +708,6 @@ static void macb_mac_config(struct phylink_config *config, unsigned int mode, ctrl |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL); } else if (state->interface == PHY_INTERFACE_MODE_10GBASER) { ctrl |= GEM_BIT(PCSSEL); - ncr |= GEM_BIT(ENABLE_HS_MAC); } else if (bp->caps & MACB_CAPS_MIIONRGMII && bp->phy_interface == PHY_INTERFACE_MODE_MII) { ncr |= MACB_BIT(MIIONRGMII); @@ -766,10 +811,6 @@ static void macb_mac_link_up(struct phylink_config *config, macb_or_gem_writel(bp, NCFGR, ctrl); - if (bp->phy_interface == PHY_INTERFACE_MODE_10GBASER) - gem_writel(bp, HS_MAC_CONFIG, GEM_BFINS(HS_MAC_SPEED, HS_SPEED_10000M, - gem_readl(bp, HS_MAC_CONFIG))); - spin_unlock_irqrestore(&bp->lock, flags); if (!(bp->caps & MACB_CAPS_MACB_IS_EMAC))
HS Mac configuration steps: - Configure speed and serdes rate bits of USX_CONTROL register from user specified speed in the device-tree. - Enable HS Mac for 5G and 10G speeds. - Reset RX receive path to achieve USX block lock for the configured serdes rate. - Wait for USX block lock synchronization. Move the initialization instances to macb_usx_pcs_link_up(). Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com> --- drivers/net/ethernet/cadence/macb.h | 1 + drivers/net/ethernet/cadence/macb_main.c | 57 ++++++++++++++++++++---- 2 files changed, 50 insertions(+), 8 deletions(-)