| Message ID | 20220125170533.256468-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 |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 25 this patch: 25 |
| netdev/cc_maintainers | success | CCed 5 of 5 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 25 this patch: 25 |
| netdev/checkpatch | warning | WARNING: line length of 82 exceeds 80 columns |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On 1/25/22 18:05, Robert Hancock wrote: > 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 when it is initializing. > > Signed-off-by: Robert Hancock <robert.hancock@calian.com> > --- > drivers/net/ethernet/cadence/macb_main.c | 48 +++++++++++++++++++++++- > 1 file changed, 47 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > index a363da928e8b..80882908a68f 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 */ > @@ -4455,6 +4457,50 @@ static int fu540_c000_init(struct platform_device *pdev) > return macb_init(pdev); > } > > +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 */ > + struct phy *sgmii_phy = devm_phy_get(&pdev->dev, "sgmii-phy"); > + > + if (IS_ERR(sgmii_phy)) { > + ret = PTR_ERR(sgmii_phy); > + dev_err_probe(&pdev->dev, ret, > + "failed to get PS-GTR PHY\n"); > + return ret; > + } > + > + ret = phy_init(sgmii_phy); > + if (ret) { > + dev_err(&pdev->dev, "failed to init PS-GTR PHY: %d\n", > + ret); > + return ret; > + } I think reset below should be here to follow correct startup sequence. Thanks, Michal > + > + ret = phy_power_on(sgmii_phy); > + if (ret) { > + dev_err(&pdev->dev, "failed to power on 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_usrio_config sama7g5_usrio = { > .mii = 0, > .rmii = 1, > @@ -4550,7 +4596,7 @@ static const struct macb_config zynqmp_config = { > 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, > };
On 26.01.2022 09:30, Michal Simek wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > On 1/25/22 18:05, Robert Hancock wrote: >> 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 when it is initializing. >> >> Signed-off-by: Robert Hancock <robert.hancock@calian.com> >> --- >> drivers/net/ethernet/cadence/macb_main.c | 48 +++++++++++++++++++++++- >> 1 file changed, 47 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/cadence/macb_main.c >> b/drivers/net/ethernet/cadence/macb_main.c >> index a363da928e8b..80882908a68f 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 */ >> @@ -4455,6 +4457,50 @@ static int fu540_c000_init(struct platform_device >> *pdev) >> return macb_init(pdev); >> } >> >> +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 */ >> + struct phy *sgmii_phy = devm_phy_get(&pdev->dev, "sgmii-phy"); >> + >> + if (IS_ERR(sgmii_phy)) { >> + ret = PTR_ERR(sgmii_phy); >> + dev_err_probe(&pdev->dev, ret, >> + "failed to get PS-GTR PHY\n"); >> + return ret; >> + } >> + >> + ret = phy_init(sgmii_phy); >> + if (ret) { >> + dev_err(&pdev->dev, "failed to init PS-GTR PHY: %d\n", >> + ret); >> + return ret; >> + } > > I think reset below should be here to follow correct startup sequence. If that's the case is the functionality still kept if moving phy_power_on() in macb_open() and the correspondent phy_power_off() in macb_close() ? Also, Robert, please handle the error path in this function (with calls to phy_exit(), phy_power_off()) and PHY handling in macb_remove(). Thank you, Claudiu Beznea > > Thanks, > Michal > > >> + >> + ret = phy_power_on(sgmii_phy); >> + if (ret) { >> + dev_err(&pdev->dev, "failed to power on 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_usrio_config sama7g5_usrio = { >> .mii = 0, >> .rmii = 1, >> @@ -4550,7 +4596,7 @@ static const struct macb_config zynqmp_config = { >> 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, >> };
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index a363da928e8b..80882908a68f 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 */ @@ -4455,6 +4457,50 @@ static int fu540_c000_init(struct platform_device *pdev) return macb_init(pdev); } +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 */ + struct phy *sgmii_phy = devm_phy_get(&pdev->dev, "sgmii-phy"); + + if (IS_ERR(sgmii_phy)) { + ret = PTR_ERR(sgmii_phy); + dev_err_probe(&pdev->dev, ret, + "failed to get PS-GTR PHY\n"); + return ret; + } + + ret = phy_init(sgmii_phy); + if (ret) { + dev_err(&pdev->dev, "failed to init PS-GTR PHY: %d\n", + ret); + return ret; + } + + ret = phy_power_on(sgmii_phy); + if (ret) { + dev_err(&pdev->dev, "failed to power on 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_usrio_config sama7g5_usrio = { .mii = 0, .rmii = 1, @@ -4550,7 +4596,7 @@ static const struct macb_config zynqmp_config = { 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, };
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 when it is initializing. Signed-off-by: Robert Hancock <robert.hancock@calian.com> --- drivers/net/ethernet/cadence/macb_main.c | 48 +++++++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-)