| Message ID | 20220112181113.875567-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/12/22 19:11, 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 | 47 +++++++++++++++++++++++- > 1 file changed, 46 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > index a363da928e8b..65b0360c487a 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,49 @@ 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; > + > + /* Fully reset GEM controller at hardware level using zynqmp-reset driver, > + * if mapped in device tree. > + */ > + ret = device_reset(&pdev->dev); > + if (ret) { > + dev_err_probe(&pdev->dev, ret, "failed to reset controller"); > + return 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 was playing with it recently on u-boot side and device reset should happen between phy init and phy power on to finish calibration. At least that's I was told and that's I use in u-boot driver. Harini/Piyush: Please correct me if I am wrong. Thanks, Michal
On 12.01.2022 20:11, 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 when it is initializing. > > Signed-off-by: Robert Hancock <robert.hancock@calian.com> > --- > drivers/net/ethernet/cadence/macb_main.c | 47 +++++++++++++++++++++++- > 1 file changed, 46 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > index a363da928e8b..65b0360c487a 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,49 @@ 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; > + > + /* Fully reset GEM controller at hardware level using zynqmp-reset driver, > + * if mapped in device tree. > + */ > + ret = device_reset(&pdev->dev); > + if (ret) { > + dev_err_probe(&pdev->dev, ret, "failed to reset controller"); > + return ret; If using old device trees this will fail, right? If yes, you should take care this code will also work with old device trees. Thank you, Claudiu Beznea > + } > + > + 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; > + } > + } > + return macb_init(pdev); > +} > + > static const struct macb_usrio_config sama7g5_usrio = { > .mii = 0, > .rmii = 1, > @@ -4550,7 +4595,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, > }; > -- > 2.31.1 >
+Radhey Hi Robert, On Thu, Jan 13, 2022 at 2:46 PM Michal Simek <michal.simek@xilinx.com> wrote: > > > > On 1/12/22 19:11, 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 | 47 +++++++++++++++++++++++- > > 1 file changed, 46 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > > index a363da928e8b..65b0360c487a 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,49 @@ 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; > > + > > + /* Fully reset GEM controller at hardware level using zynqmp-reset driver, > > + * if mapped in device tree. > > + */ > > + ret = device_reset(&pdev->dev); > > + if (ret) { > > + dev_err_probe(&pdev->dev, ret, "failed to reset controller"); > > + return 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 was playing with it recently on u-boot side and device reset should happen > between phy init and phy power on to finish calibration. > At least that's I was told and that's I use in u-boot driver. > > Harini/Piyush: Please correct me if I am wrong. Thanks for the patch. GEM should definitely be reset once after the serdes init and power on is done. It can be held in reset and released after serdes init or reset with a 1-0 after serdes init. Either should be fine but a reset before phy init may not work. I've added Radhey who worked on this recently and can add any further info. Regards, Harini
On Thu, 2022-01-13 at 08:04 +0000, Claudiu.Beznea@microchip.com wrote: > On 12.01.2022 20:11, 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 when it is initializing. > > > > Signed-off-by: Robert Hancock <robert.hancock@calian.com> > > --- > > drivers/net/ethernet/cadence/macb_main.c | 47 +++++++++++++++++++++++- > > 1 file changed, 46 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/cadence/macb_main.c > > b/drivers/net/ethernet/cadence/macb_main.c > > index a363da928e8b..65b0360c487a 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,49 @@ 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; > > + > > + /* Fully reset GEM controller at hardware level using zynqmp-reset > > driver, > > + * if mapped in device tree. > > + */ > > + ret = device_reset(&pdev->dev); > > + if (ret) { > > + dev_err_probe(&pdev->dev, ret, "failed to reset > > controller"); > > + return ret; > > If using old device trees this will fail, right? If yes, you should take > care this code will also work with old device trees. I think I had believed device_reset should just return a success without doing anything if the device tree has no reset defined for the device, but it appears to get that behavior we should be using device_reset_optional. > > Thank you, > Claudiu Beznea > > > + } > > + > > + 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; > > + } > > + } > > + return macb_init(pdev); > > +} > > + > > static const struct macb_usrio_config sama7g5_usrio = { > > .mii = 0, > > .rmii = 1, > > @@ -4550,7 +4595,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, > > }; > > -- > > 2.31.1 > >
On Thu, 2022-01-13 at 15:44 +0530, Harini Katakam wrote: > +Radhey > > Hi Robert, > > On Thu, Jan 13, 2022 at 2:46 PM Michal Simek <michal.simek@xilinx.com> wrote: > > > > > > On 1/12/22 19:11, 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 | 47 +++++++++++++++++++++++- > > > 1 file changed, 46 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/ethernet/cadence/macb_main.c > > > b/drivers/net/ethernet/cadence/macb_main.c > > > index a363da928e8b..65b0360c487a 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,49 @@ 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; > > > + > > > + /* Fully reset GEM controller at hardware level using zynqmp-reset > > > driver, > > > + * if mapped in device tree. > > > + */ > > > + ret = device_reset(&pdev->dev); > > > + if (ret) { > > > + dev_err_probe(&pdev->dev, ret, "failed to reset > > > controller"); > > > + return 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 was playing with it recently on u-boot side and device reset should > > happen > > between phy init and phy power on to finish calibration. > > At least that's I was told and that's I use in u-boot driver. > > > > Harini/Piyush: Please correct me if I am wrong. > > Thanks for the patch. > > GEM should definitely be reset once after the serdes init and power on is > done. > It can be held in reset and released after serdes init or reset with a 1-0 > after > serdes init. Either should be fine but a reset before phy init may not work. > I've added Radhey who worked on this recently and can add any further info. Thanks for the feedback on this. I believe I pretty much arrived at the sequence I had via trial and error when trying to get things to work and so it might not quite be ideal. I've done some tests with moving the reset down to after the PHY init and power on and that seems to work well, so if that's the preferred sequence I can switch to that. > > Regards, > Harini
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index a363da928e8b..65b0360c487a 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,49 @@ 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; + + /* Fully reset GEM controller at hardware level using zynqmp-reset driver, + * if mapped in device tree. + */ + ret = device_reset(&pdev->dev); + if (ret) { + dev_err_probe(&pdev->dev, ret, "failed to reset controller"); + return 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; + } + } + return macb_init(pdev); +} + static const struct macb_usrio_config sama7g5_usrio = { .mii = 0, .rmii = 1, @@ -4550,7 +4595,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 | 47 +++++++++++++++++++++++- 1 file changed, 46 insertions(+), 1 deletion(-)