| Message ID | 20220127002711.3632101-3-robert.hancock@calian.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Cadence MACB/GEM support for ZynqMP SGMII | expand |
Hi Robert, On 27.01.2022 02:27, Robert Hancock wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > The GEM controllers on ZynqMP were missing some initialization steps which > are required in some cases when using SGMII mode, which uses the PS-GTR > transceivers managed by the phy-zynqmp driver. > > The GEM core appears to need a hardware-level reset in order to work > properly in SGMII mode in cases where the GT reference clock was not > present at initial power-on. This can be done using a reset mapped to > the zynqmp-reset driver in the device tree. > > Also, when in SGMII mode, the GEM driver needs to ensure the PHY is > initialized and powered on. > > Signed-off-by: Robert Hancock <robert.hancock@calian.com> > --- > drivers/net/ethernet/cadence/macb.h | 5 +++ > drivers/net/ethernet/cadence/macb_main.c | 53 +++++++++++++++++++++++- > 2 files changed, 56 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h > index 9ddbee7de72b..584336b7cdaf 100644 > --- a/drivers/net/ethernet/cadence/macb.h > +++ b/drivers/net/ethernet/cadence/macb.h > @@ -12,6 +12,7 @@ > #include <linux/ptp_clock_kernel.h> > #include <linux/net_tstamp.h> > #include <linux/interrupt.h> > +#include <linux/phy/phy.h> > > #if defined(CONFIG_ARCH_DMA_ADDR_T_64BIT) || defined(CONFIG_MACB_USE_HWSTAMP) > #define MACB_EXT_DESC > @@ -1291,6 +1292,9 @@ struct macb { > u32 wol; > > struct macb_ptp_info *ptp_info; /* macb-ptp interface */ > + > + struct phy *sgmii_phy; /* for ZynqMP SGMII mode */ You have spaces b/w "*sgmii_phy;" and "/* for ZynqMP SGMII mode */" > + > #ifdef MACB_EXT_DESC > uint8_t hw_dma_cap; > #endif > @@ -1315,6 +1319,7 @@ struct macb { > > struct macb_pm_data pm_data; > const struct macb_usrio_config *usrio; > + You have an extra line here. > }; > > #ifdef CONFIG_MACB_USE_HWSTAMP > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > index a363da928e8b..4787196e0980 100644 > --- a/drivers/net/ethernet/cadence/macb_main.c > +++ b/drivers/net/ethernet/cadence/macb_main.c > @@ -34,7 +34,9 @@ > #include <linux/udp.h> > #include <linux/tcp.h> > #include <linux/iopoll.h> > +#include <linux/phy/phy.h> > #include <linux/pm_runtime.h> > +#include <linux/reset.h> > #include "macb.h" > > /* This structure is only used for MACB on SiFive FU540 devices */ > @@ -2739,6 +2741,10 @@ static int macb_open(struct net_device *dev) > > macb_init_hw(bp); > > + err = phy_power_on(bp->sgmii_phy); > + if (err) > + goto reset_hw; > + > err = macb_phylink_connect(bp); > if (err) > goto reset_hw; This one should phy_power_off() before resetting the hardware. > @@ -2775,6 +2781,8 @@ static int macb_close(struct net_device *dev) > phylink_stop(bp->phylink); > phylink_disconnect_phy(bp->phylink); > > + phy_power_off(bp->sgmii_phy); > + > spin_lock_irqsave(&bp->lock, flags); > macb_reset_hw(bp); > netif_carrier_off(dev); > @@ -4544,13 +4552,50 @@ static const struct macb_config np4_config = { > .usrio = &macb_default_usrio, > }; > > +static int zynqmp_init(struct platform_device *pdev) > +{ > + struct net_device *dev = platform_get_drvdata(pdev); > + struct macb *bp = netdev_priv(dev); > + int ret; > + > + if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) { > + /* Ensure PS-GTR PHY device used in SGMII mode is ready */ > + bp->sgmii_phy = devm_phy_get(&pdev->dev, "sgmii-phy"); > + > + if (IS_ERR(bp->sgmii_phy)) { > + ret = PTR_ERR(bp->sgmii_phy); > + dev_err_probe(&pdev->dev, ret, > + "failed to get PS-GTR PHY\n"); > + return ret; > + } > + > + ret = phy_init(bp->sgmii_phy); > + if (ret) { > + dev_err(&pdev->dev, "failed to init PS-GTR PHY: %d\n", > + ret); > + return ret; > + } > + } > + > + /* Fully reset GEM controller at hardware level using zynqmp-reset driver, > + * if mapped in device tree. > + */ > + ret = device_reset_optional(&pdev->dev); > + if (ret) { > + dev_err_probe(&pdev->dev, ret, "failed to reset controller"); Before doing return you should call phy_exit(); > + return ret; > + } > + > + return macb_init(pdev); Same here, check return code and call phy_exit() if needed. In case init() call in macb_probe() fails it will jump to err_out_free_netdev and will miss calling phy_exit(). And is better to free resource in the same function they were allocated. > +} > + > static const struct macb_config zynqmp_config = { > .caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | > MACB_CAPS_JUMBO | > MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH, > .dma_burst_length = 16, > .clk_init = macb_clk_init, > - .init = macb_init, > + .init = zynqmp_init, > .jumbo_max_len = 10240, > .usrio = &macb_default_usrio, > }; > @@ -4767,7 +4812,7 @@ static int macb_probe(struct platform_device *pdev) > > err = macb_mii_init(bp); > if (err) > - goto err_out_free_netdev; > + goto err_out_phy_exit; > > netif_carrier_off(dev); > > @@ -4792,6 +4837,9 @@ static int macb_probe(struct platform_device *pdev) > mdiobus_unregister(bp->mii_bus); > mdiobus_free(bp->mii_bus); > > +err_out_phy_exit: > + phy_exit(bp->sgmii_phy); > + > err_out_free_netdev: > free_netdev(dev); > > @@ -4813,6 +4861,7 @@ static int macb_remove(struct platform_device *pdev) > > if (dev) { > bp = netdev_priv(dev); > + phy_exit(bp->sgmii_phy); > mdiobus_unregister(bp->mii_bus); > mdiobus_free(bp->mii_bus); > > -- > 2.31.1 >
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h index 9ddbee7de72b..584336b7cdaf 100644 --- a/drivers/net/ethernet/cadence/macb.h +++ b/drivers/net/ethernet/cadence/macb.h @@ -12,6 +12,7 @@ #include <linux/ptp_clock_kernel.h> #include <linux/net_tstamp.h> #include <linux/interrupt.h> +#include <linux/phy/phy.h> #if defined(CONFIG_ARCH_DMA_ADDR_T_64BIT) || defined(CONFIG_MACB_USE_HWSTAMP) #define MACB_EXT_DESC @@ -1291,6 +1292,9 @@ struct macb { u32 wol; struct macb_ptp_info *ptp_info; /* macb-ptp interface */ + + struct phy *sgmii_phy; /* for ZynqMP SGMII mode */ + #ifdef MACB_EXT_DESC uint8_t hw_dma_cap; #endif @@ -1315,6 +1319,7 @@ struct macb { struct macb_pm_data pm_data; const struct macb_usrio_config *usrio; + }; #ifdef CONFIG_MACB_USE_HWSTAMP diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index a363da928e8b..4787196e0980 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -34,7 +34,9 @@ #include <linux/udp.h> #include <linux/tcp.h> #include <linux/iopoll.h> +#include <linux/phy/phy.h> #include <linux/pm_runtime.h> +#include <linux/reset.h> #include "macb.h" /* This structure is only used for MACB on SiFive FU540 devices */ @@ -2739,6 +2741,10 @@ static int macb_open(struct net_device *dev) macb_init_hw(bp); + err = phy_power_on(bp->sgmii_phy); + if (err) + goto reset_hw; + err = macb_phylink_connect(bp); if (err) goto reset_hw; @@ -2775,6 +2781,8 @@ static int macb_close(struct net_device *dev) phylink_stop(bp->phylink); phylink_disconnect_phy(bp->phylink); + phy_power_off(bp->sgmii_phy); + spin_lock_irqsave(&bp->lock, flags); macb_reset_hw(bp); netif_carrier_off(dev); @@ -4544,13 +4552,50 @@ static const struct macb_config np4_config = { .usrio = &macb_default_usrio, }; +static int zynqmp_init(struct platform_device *pdev) +{ + struct net_device *dev = platform_get_drvdata(pdev); + struct macb *bp = netdev_priv(dev); + int ret; + + if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) { + /* Ensure PS-GTR PHY device used in SGMII mode is ready */ + bp->sgmii_phy = devm_phy_get(&pdev->dev, "sgmii-phy"); + + if (IS_ERR(bp->sgmii_phy)) { + ret = PTR_ERR(bp->sgmii_phy); + dev_err_probe(&pdev->dev, ret, + "failed to get PS-GTR PHY\n"); + return ret; + } + + ret = phy_init(bp->sgmii_phy); + if (ret) { + dev_err(&pdev->dev, "failed to init PS-GTR PHY: %d\n", + ret); + return ret; + } + } + + /* Fully reset GEM controller at hardware level using zynqmp-reset driver, + * if mapped in device tree. + */ + ret = device_reset_optional(&pdev->dev); + if (ret) { + dev_err_probe(&pdev->dev, ret, "failed to reset controller"); + return ret; + } + + return macb_init(pdev); +} + static const struct macb_config zynqmp_config = { .caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO | MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH, .dma_burst_length = 16, .clk_init = macb_clk_init, - .init = macb_init, + .init = zynqmp_init, .jumbo_max_len = 10240, .usrio = &macb_default_usrio, }; @@ -4767,7 +4812,7 @@ static int macb_probe(struct platform_device *pdev) err = macb_mii_init(bp); if (err) - goto err_out_free_netdev; + goto err_out_phy_exit; netif_carrier_off(dev); @@ -4792,6 +4837,9 @@ static int macb_probe(struct platform_device *pdev) mdiobus_unregister(bp->mii_bus); mdiobus_free(bp->mii_bus); +err_out_phy_exit: + phy_exit(bp->sgmii_phy); + err_out_free_netdev: free_netdev(dev); @@ -4813,6 +4861,7 @@ static int macb_remove(struct platform_device *pdev) if (dev) { bp = netdev_priv(dev); + phy_exit(bp->sgmii_phy); mdiobus_unregister(bp->mii_bus); mdiobus_free(bp->mii_bus);
The GEM controllers on ZynqMP were missing some initialization steps which are required in some cases when using SGMII mode, which uses the PS-GTR transceivers managed by the phy-zynqmp driver. The GEM core appears to need a hardware-level reset in order to work properly in SGMII mode in cases where the GT reference clock was not present at initial power-on. This can be done using a reset mapped to the zynqmp-reset driver in the device tree. Also, when in SGMII mode, the GEM driver needs to ensure the PHY is initialized and powered on. Signed-off-by: Robert Hancock <robert.hancock@calian.com> --- drivers/net/ethernet/cadence/macb.h | 5 +++ drivers/net/ethernet/cadence/macb_main.c | 53 +++++++++++++++++++++++- 2 files changed, 56 insertions(+), 2 deletions(-)