| Message ID | 20210326000737.9764-5-tharvey@gateworks.com |
|---|---|
| State | Accepted |
| Commit | 8c64347b7e45f96d82f8c6782c2fcd309f4e45cf |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | octeontx cleanup and fixes | expand |
On 26.03.21 01:07, Tim Harvey wrote: > Revert a change that occured between the Marvell SDK-10.1.1.0 > and SDK-10.3.1.1 which broke QSMII phy support. > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> Thanks. Suneel, do you have a comment on this? Is this revert the "best way" to handle this? Thanks, Stefan > --- > drivers/net/octeontx/bgx.c | 20 +++++++------------- > 1 file changed, 7 insertions(+), 13 deletions(-) > > diff --git a/drivers/net/octeontx/bgx.c b/drivers/net/octeontx/bgx.c > index 2ea54be84d..a5c0c9fe2b 100644 > --- a/drivers/net/octeontx/bgx.c > +++ b/drivers/net/octeontx/bgx.c > @@ -36,7 +36,6 @@ struct lmac { > int dmac; > u8 mac[6]; > bool link_up; > - bool init_pend; > int lmacid; /* ID within BGX */ > int phy_addr; /* ID on board */ > struct udevice *dev; > @@ -849,6 +848,7 @@ static int bgx_lmac_enable(struct bgx *bgx, int8_t lmacid) > u64 cfg; > > lmac = &bgx->lmac[lmacid]; > + lmac->bgx = bgx; > > debug("%s: lmac: %p, lmacid = %d\n", __func__, lmac, lmacid); > > @@ -895,16 +895,6 @@ int bgx_poll_for_link(int node, int bgx_idx, int lmacid) > debug("%s: %d, lmac: %d/%d/%d %p\n", > __FILE__, __LINE__, > node, bgx_idx, lmacid, lmac); > - if (lmac->init_pend) { > - ret = bgx_lmac_enable(lmac->bgx, lmacid); > - if (ret < 0) { > - printf("BGX%d LMAC%d lmac_enable failed\n", bgx_idx, > - lmacid); > - return ret; > - } > - lmac->init_pend = 0; > - mdelay(100); > - } > if (lmac->qlm_mode == QLM_MODE_SGMII || > lmac->qlm_mode == QLM_MODE_RGMII || > lmac->qlm_mode == QLM_MODE_QSGMII) { > @@ -1461,6 +1451,7 @@ int octeontx_bgx_remove(struct udevice *dev) > > int octeontx_bgx_probe(struct udevice *dev) > { > + int err; > struct bgx *bgx = dev_get_priv(dev); > u8 lmac = 0; > int qlm[4] = {-1, -1, -1, -1}; > @@ -1540,8 +1531,11 @@ skip_qlm_config: > struct lmac *tlmac = &bgx->lmac[lmac]; > > tlmac->dev = dev; > - tlmac->init_pend = 1; > - tlmac->bgx = bgx; > + err = bgx_lmac_enable(bgx, lmac); > + if (err) { > + printf("BGX%d failed to enable lmac%d\n", > + bgx->bgx_id, lmac); > + } > } > > return 0; > Viele Grüße, Stefan
This looks like a workaround than the root cause fix. As this patch just moves the bringup of link from probe stage to on-demand. On Thu, Mar 25, 2021 at 11:46 PM Stefan Roese <sr@denx.de> wrote: > > On 26.03.21 01:07, Tim Harvey wrote: > > Revert a change that occured between the Marvell SDK-10.1.1.0 > > and SDK-10.3.1.1 which broke QSMII phy support. > > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > Thanks. > > Suneel, do you have a comment on this? Is this revert the "best way" to > handle this? > > Thanks, > Stefan > > > --- > > drivers/net/octeontx/bgx.c | 20 +++++++------------- > > 1 file changed, 7 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/net/octeontx/bgx.c b/drivers/net/octeontx/bgx.c > > index 2ea54be84d..a5c0c9fe2b 100644 > > --- a/drivers/net/octeontx/bgx.c > > +++ b/drivers/net/octeontx/bgx.c > > @@ -36,7 +36,6 @@ struct lmac { > > int dmac; > > u8 mac[6]; > > bool link_up; > > - bool init_pend; > > int lmacid; /* ID within BGX */ > > int phy_addr; /* ID on board */ > > struct udevice *dev; > > @@ -849,6 +848,7 @@ static int bgx_lmac_enable(struct bgx *bgx, int8_t lmacid) > > u64 cfg; > > > > lmac = &bgx->lmac[lmacid]; > > + lmac->bgx = bgx; > > > > debug("%s: lmac: %p, lmacid = %d\n", __func__, lmac, lmacid); > > > > @@ -895,16 +895,6 @@ int bgx_poll_for_link(int node, int bgx_idx, int lmacid) > > debug("%s: %d, lmac: %d/%d/%d %p\n", > > __FILE__, __LINE__, > > node, bgx_idx, lmacid, lmac); > > - if (lmac->init_pend) { > > - ret = bgx_lmac_enable(lmac->bgx, lmacid); > > - if (ret < 0) { > > - printf("BGX%d LMAC%d lmac_enable failed\n", bgx_idx, > > - lmacid); > > - return ret; > > - } > > - lmac->init_pend = 0; > > - mdelay(100); > > - } > > if (lmac->qlm_mode == QLM_MODE_SGMII || > > lmac->qlm_mode == QLM_MODE_RGMII || > > lmac->qlm_mode == QLM_MODE_QSGMII) { > > @@ -1461,6 +1451,7 @@ int octeontx_bgx_remove(struct udevice *dev) > > > > int octeontx_bgx_probe(struct udevice *dev) > > { > > + int err; > > struct bgx *bgx = dev_get_priv(dev); > > u8 lmac = 0; > > int qlm[4] = {-1, -1, -1, -1}; > > @@ -1540,8 +1531,11 @@ skip_qlm_config: > > struct lmac *tlmac = &bgx->lmac[lmac]; > > > > tlmac->dev = dev; > > - tlmac->init_pend = 1; > > - tlmac->bgx = bgx; > > + err = bgx_lmac_enable(bgx, lmac); > > + if (err) { > > + printf("BGX%d failed to enable lmac%d\n", > > + bgx->bgx_id, lmac); > > + } > > } > > > > return 0; > > > > > Viele Grüße, > Stefan > > -- > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de
On Fri, Mar 26, 2021 at 9:09 AM Suneel Garapati <suneelglinux@gmail.com> wrote: > > This looks like a workaround than the root cause fix. > As this patch just moves the bringup of link from probe stage to on-demand. > > On Thu, Mar 25, 2021 at 11:46 PM Stefan Roese <sr@denx.de> wrote: > > > > On 26.03.21 01:07, Tim Harvey wrote: > > > Revert a change that occured between the Marvell SDK-10.1.1.0 > > > and SDK-10.3.1.1 which broke QSMII phy support. > > > > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > > > Thanks. > > > > Suneel, do you have a comment on this? Is this revert the "best way" to > > handle this? > > > > Thanks, > > Stefan > > > > > --- > > > drivers/net/octeontx/bgx.c | 20 +++++++------------- > > > 1 file changed, 7 insertions(+), 13 deletions(-) > > > > > > diff --git a/drivers/net/octeontx/bgx.c b/drivers/net/octeontx/bgx.c > > > index 2ea54be84d..a5c0c9fe2b 100644 > > > --- a/drivers/net/octeontx/bgx.c > > > +++ b/drivers/net/octeontx/bgx.c > > > @@ -36,7 +36,6 @@ struct lmac { > > > int dmac; > > > u8 mac[6]; > > > bool link_up; > > > - bool init_pend; > > > int lmacid; /* ID within BGX */ > > > int phy_addr; /* ID on board */ > > > struct udevice *dev; > > > @@ -849,6 +848,7 @@ static int bgx_lmac_enable(struct bgx *bgx, int8_t lmacid) > > > u64 cfg; > > > > > > lmac = &bgx->lmac[lmacid]; > > > + lmac->bgx = bgx; > > > > > > debug("%s: lmac: %p, lmacid = %d\n", __func__, lmac, lmacid); > > > > > > @@ -895,16 +895,6 @@ int bgx_poll_for_link(int node, int bgx_idx, int lmacid) > > > debug("%s: %d, lmac: %d/%d/%d %p\n", > > > __FILE__, __LINE__, > > > node, bgx_idx, lmacid, lmac); > > > - if (lmac->init_pend) { > > > - ret = bgx_lmac_enable(lmac->bgx, lmacid); > > > - if (ret < 0) { > > > - printf("BGX%d LMAC%d lmac_enable failed\n", bgx_idx, > > > - lmacid); > > > - return ret; > > > - } > > > - lmac->init_pend = 0; > > > - mdelay(100); > > > - } > > > if (lmac->qlm_mode == QLM_MODE_SGMII || > > > lmac->qlm_mode == QLM_MODE_RGMII || > > > lmac->qlm_mode == QLM_MODE_QSGMII) { > > > @@ -1461,6 +1451,7 @@ int octeontx_bgx_remove(struct udevice *dev) > > > > > > int octeontx_bgx_probe(struct udevice *dev) > > > { > > > + int err; > > > struct bgx *bgx = dev_get_priv(dev); > > > u8 lmac = 0; > > > int qlm[4] = {-1, -1, -1, -1}; > > > @@ -1540,8 +1531,11 @@ skip_qlm_config: > > > struct lmac *tlmac = &bgx->lmac[lmac]; > > > > > > tlmac->dev = dev; > > > - tlmac->init_pend = 1; > > > - tlmac->bgx = bgx; > > > + err = bgx_lmac_enable(bgx, lmac); > > > + if (err) { > > > + printf("BGX%d failed to enable lmac%d\n", > > > + bgx->bgx_id, lmac); > > > + } > > > } > > > > > > return 0; > > > > > Suneel, I agree, it is a workaround and I don't quite understand the reason. Can you look into the code history of the SDK BDK and see what the reason was for introducing this 'pending' state that defers the bgx_lmac_enable call that breaks QSGMII? That specific change was made between SDK-10.1.1.0 and SDK-10.3.1.1. What boards and PHY's have you tested octeontx BGX with? I have the following boards to test with that we designed and support: - GW630x: supports both RGMII and SGMII GbE PHY's - GW640x: supports both RGMII and QSGMII GbE PHy's The only issue I have that is worked around by the above is QSGMII. Best regards, Tim
+ Chandra On Fri, Mar 26, 2021 at 9:38 AM Tim Harvey <tharvey@gateworks.com> wrote: > > On Fri, Mar 26, 2021 at 9:09 AM Suneel Garapati <suneelglinux@gmail.com> wrote: > > > > This looks like a workaround than the root cause fix. > > As this patch just moves the bringup of link from probe stage to on-demand. > > > > On Thu, Mar 25, 2021 at 11:46 PM Stefan Roese <sr@denx.de> wrote: > > > > > > On 26.03.21 01:07, Tim Harvey wrote: > > > > Revert a change that occured between the Marvell SDK-10.1.1.0 > > > > and SDK-10.3.1.1 which broke QSMII phy support. > > > > > > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > > > > > Thanks. > > > > > > Suneel, do you have a comment on this? Is this revert the "best way" to > > > handle this? > > > > > > Thanks, > > > Stefan > > > > > > > --- > > > > drivers/net/octeontx/bgx.c | 20 +++++++------------- > > > > 1 file changed, 7 insertions(+), 13 deletions(-) > > > > > > > > diff --git a/drivers/net/octeontx/bgx.c b/drivers/net/octeontx/bgx.c > > > > index 2ea54be84d..a5c0c9fe2b 100644 > > > > --- a/drivers/net/octeontx/bgx.c > > > > +++ b/drivers/net/octeontx/bgx.c > > > > @@ -36,7 +36,6 @@ struct lmac { > > > > int dmac; > > > > u8 mac[6]; > > > > bool link_up; > > > > - bool init_pend; > > > > int lmacid; /* ID within BGX */ > > > > int phy_addr; /* ID on board */ > > > > struct udevice *dev; > > > > @@ -849,6 +848,7 @@ static int bgx_lmac_enable(struct bgx *bgx, int8_t lmacid) > > > > u64 cfg; > > > > > > > > lmac = &bgx->lmac[lmacid]; > > > > + lmac->bgx = bgx; > > > > > > > > debug("%s: lmac: %p, lmacid = %d\n", __func__, lmac, lmacid); > > > > > > > > @@ -895,16 +895,6 @@ int bgx_poll_for_link(int node, int bgx_idx, int lmacid) > > > > debug("%s: %d, lmac: %d/%d/%d %p\n", > > > > __FILE__, __LINE__, > > > > node, bgx_idx, lmacid, lmac); > > > > - if (lmac->init_pend) { > > > > - ret = bgx_lmac_enable(lmac->bgx, lmacid); > > > > - if (ret < 0) { > > > > - printf("BGX%d LMAC%d lmac_enable failed\n", bgx_idx, > > > > - lmacid); > > > > - return ret; > > > > - } > > > > - lmac->init_pend = 0; > > > > - mdelay(100); > > > > - } > > > > if (lmac->qlm_mode == QLM_MODE_SGMII || > > > > lmac->qlm_mode == QLM_MODE_RGMII || > > > > lmac->qlm_mode == QLM_MODE_QSGMII) { > > > > @@ -1461,6 +1451,7 @@ int octeontx_bgx_remove(struct udevice *dev) > > > > > > > > int octeontx_bgx_probe(struct udevice *dev) > > > > { > > > > + int err; > > > > struct bgx *bgx = dev_get_priv(dev); > > > > u8 lmac = 0; > > > > int qlm[4] = {-1, -1, -1, -1}; > > > > @@ -1540,8 +1531,11 @@ skip_qlm_config: > > > > struct lmac *tlmac = &bgx->lmac[lmac]; > > > > > > > > tlmac->dev = dev; > > > > - tlmac->init_pend = 1; > > > > - tlmac->bgx = bgx; > > > > + err = bgx_lmac_enable(bgx, lmac); > > > > + if (err) { > > > > + printf("BGX%d failed to enable lmac%d\n", > > > > + bgx->bgx_id, lmac); > > > > + } > > > > } > > > > > > > > return 0; > > > > > > > > > Suneel, > > I agree, it is a workaround and I don't quite understand the reason. > > Can you look into the code history of the SDK BDK and see what the > reason was for introducing this 'pending' state that defers the > bgx_lmac_enable call that breaks QSGMII? That specific change was made > between SDK-10.1.1.0 and SDK-10.3.1.1. > > What boards and PHY's have you tested octeontx BGX with? > > I have the following boards to test with that we designed and support: > - GW630x: supports both RGMII and SGMII GbE PHY's > - GW640x: supports both RGMII and QSGMII GbE PHy's > > The only issue I have that is worked around by the above is QSGMII. > > Best regards, > > Tim
On Fri, Mar 26, 2021 at 9:39 AM Suneel Garapati <suneelglinux@gmail.com> wrote: > > + Chandra > > On Fri, Mar 26, 2021 at 9:38 AM Tim Harvey <tharvey@gateworks.com> wrote: > > > > On Fri, Mar 26, 2021 at 9:09 AM Suneel Garapati <suneelglinux@gmail.com> wrote: > > > > > > This looks like a workaround than the root cause fix. > > > As this patch just moves the bringup of link from probe stage to on-demand. > > > > > > On Thu, Mar 25, 2021 at 11:46 PM Stefan Roese <sr@denx.de> wrote: > > > > > > > > On 26.03.21 01:07, Tim Harvey wrote: > > > > > Revert a change that occured between the Marvell SDK-10.1.1.0 > > > > > and SDK-10.3.1.1 which broke QSMII phy support. > > > > > > > > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > > > > > > > Thanks. > > > > > > > > Suneel, do you have a comment on this? Is this revert the "best way" to > > > > handle this? > > > > > > > > Thanks, > > > > Stefan > > > > > > > > > --- > > > > > drivers/net/octeontx/bgx.c | 20 +++++++------------- > > > > > 1 file changed, 7 insertions(+), 13 deletions(-) > > > > > > > > > > diff --git a/drivers/net/octeontx/bgx.c b/drivers/net/octeontx/bgx.c > > > > > index 2ea54be84d..a5c0c9fe2b 100644 > > > > > --- a/drivers/net/octeontx/bgx.c > > > > > +++ b/drivers/net/octeontx/bgx.c > > > > > @@ -36,7 +36,6 @@ struct lmac { > > > > > int dmac; > > > > > u8 mac[6]; > > > > > bool link_up; > > > > > - bool init_pend; > > > > > int lmacid; /* ID within BGX */ > > > > > int phy_addr; /* ID on board */ > > > > > struct udevice *dev; > > > > > @@ -849,6 +848,7 @@ static int bgx_lmac_enable(struct bgx *bgx, int8_t lmacid) > > > > > u64 cfg; > > > > > > > > > > lmac = &bgx->lmac[lmacid]; > > > > > + lmac->bgx = bgx; > > > > > > > > > > debug("%s: lmac: %p, lmacid = %d\n", __func__, lmac, lmacid); > > > > > > > > > > @@ -895,16 +895,6 @@ int bgx_poll_for_link(int node, int bgx_idx, int lmacid) > > > > > debug("%s: %d, lmac: %d/%d/%d %p\n", > > > > > __FILE__, __LINE__, > > > > > node, bgx_idx, lmacid, lmac); > > > > > - if (lmac->init_pend) { > > > > > - ret = bgx_lmac_enable(lmac->bgx, lmacid); > > > > > - if (ret < 0) { > > > > > - printf("BGX%d LMAC%d lmac_enable failed\n", bgx_idx, > > > > > - lmacid); > > > > > - return ret; > > > > > - } > > > > > - lmac->init_pend = 0; > > > > > - mdelay(100); > > > > > - } > > > > > if (lmac->qlm_mode == QLM_MODE_SGMII || > > > > > lmac->qlm_mode == QLM_MODE_RGMII || > > > > > lmac->qlm_mode == QLM_MODE_QSGMII) { > > > > > @@ -1461,6 +1451,7 @@ int octeontx_bgx_remove(struct udevice *dev) > > > > > > > > > > int octeontx_bgx_probe(struct udevice *dev) > > > > > { > > > > > + int err; > > > > > struct bgx *bgx = dev_get_priv(dev); > > > > > u8 lmac = 0; > > > > > int qlm[4] = {-1, -1, -1, -1}; > > > > > @@ -1540,8 +1531,11 @@ skip_qlm_config: > > > > > struct lmac *tlmac = &bgx->lmac[lmac]; > > > > > > > > > > tlmac->dev = dev; > > > > > - tlmac->init_pend = 1; > > > > > - tlmac->bgx = bgx; > > > > > + err = bgx_lmac_enable(bgx, lmac); > > > > > + if (err) { > > > > > + printf("BGX%d failed to enable lmac%d\n", > > > > > + bgx->bgx_id, lmac); > > > > > + } > > > > > } > > > > > > > > > > return 0; > > > > > > > > > > > > > Suneel, > > > > I agree, it is a workaround and I don't quite understand the reason. > > > > Can you look into the code history of the SDK BDK and see what the > > reason was for introducing this 'pending' state that defers the > > bgx_lmac_enable call that breaks QSGMII? That specific change was made > > between SDK-10.1.1.0 and SDK-10.3.1.1. > > > > What boards and PHY's have you tested octeontx BGX with? > > > > I have the following boards to test with that we designed and support: > > - GW630x: supports both RGMII and SGMII GbE PHY's > > - GW640x: supports both RGMII and QSGMII GbE PHy's > > > > The only issue I have that is worked around by the above is QSGMII. > > Sunell / Chandrakala, Any comments here about what PHY's you have tested octeontx BGX with and what the reasoning was for the code change here between Marvell SDK-10.1.1.0 and SDK-10.3.1.1? Tim
On Thu, Mar 25, 2021 at 05:07:35PM -0700, Tim Harvey wrote: > Revert a change that occured between the Marvell SDK-10.1.1.0 > and SDK-10.3.1.1 which broke QSMII phy support. > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> Applied to u-boot/master, thanks!
diff --git a/drivers/net/octeontx/bgx.c b/drivers/net/octeontx/bgx.c index 2ea54be84d..a5c0c9fe2b 100644 --- a/drivers/net/octeontx/bgx.c +++ b/drivers/net/octeontx/bgx.c @@ -36,7 +36,6 @@ struct lmac { int dmac; u8 mac[6]; bool link_up; - bool init_pend; int lmacid; /* ID within BGX */ int phy_addr; /* ID on board */ struct udevice *dev; @@ -849,6 +848,7 @@ static int bgx_lmac_enable(struct bgx *bgx, int8_t lmacid) u64 cfg; lmac = &bgx->lmac[lmacid]; + lmac->bgx = bgx; debug("%s: lmac: %p, lmacid = %d\n", __func__, lmac, lmacid); @@ -895,16 +895,6 @@ int bgx_poll_for_link(int node, int bgx_idx, int lmacid) debug("%s: %d, lmac: %d/%d/%d %p\n", __FILE__, __LINE__, node, bgx_idx, lmacid, lmac); - if (lmac->init_pend) { - ret = bgx_lmac_enable(lmac->bgx, lmacid); - if (ret < 0) { - printf("BGX%d LMAC%d lmac_enable failed\n", bgx_idx, - lmacid); - return ret; - } - lmac->init_pend = 0; - mdelay(100); - } if (lmac->qlm_mode == QLM_MODE_SGMII || lmac->qlm_mode == QLM_MODE_RGMII || lmac->qlm_mode == QLM_MODE_QSGMII) { @@ -1461,6 +1451,7 @@ int octeontx_bgx_remove(struct udevice *dev) int octeontx_bgx_probe(struct udevice *dev) { + int err; struct bgx *bgx = dev_get_priv(dev); u8 lmac = 0; int qlm[4] = {-1, -1, -1, -1}; @@ -1540,8 +1531,11 @@ skip_qlm_config: struct lmac *tlmac = &bgx->lmac[lmac]; tlmac->dev = dev; - tlmac->init_pend = 1; - tlmac->bgx = bgx; + err = bgx_lmac_enable(bgx, lmac); + if (err) { + printf("BGX%d failed to enable lmac%d\n", + bgx->bgx_id, lmac); + } } return 0;
Revert a change that occured between the Marvell SDK-10.1.1.0 and SDK-10.3.1.1 which broke QSMII phy support. Signed-off-by: Tim Harvey <tharvey@gateworks.com> --- drivers/net/octeontx/bgx.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-)