| Message ID | 20190215153241.6857-11-antoine.tenart@bootlin.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: mvpp2: various fixes | expand |
On Fri, Feb 15, 2019 at 04:32:38PM +0100, Antoine Tenart wrote: > The documentation advises to set the XPCS in reset while reconfiguring > the serdes lanes. This seems to be a good thing to do, but the PPv2 > driver wasn't doing it. This patch fixes it. Hmm. That statment seems to have some ambiguity in it - we do two "reconfigurations" - one may be upon initialisation, where the lane is already configured for 10Gbase-KR, and we're re-initialising it for the same mode. The other case is when we're switching between 10Gbase-KR and SGMII, or as will be the case with 2.5G support for the Alaska PHYs, 2500base-X. Does this apply to reconfiguration of the serdes lane between 10Gbase-KR and slower modes? > > Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com> > --- > drivers/net/ethernet/marvell/mvpp2/mvpp2.h | 1 + > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 8 +++++++- > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h > index 21ddcac1ceea..7380bddc53b8 100644 > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h > @@ -481,6 +481,7 @@ > /* XPCS registers. PPv2.2 only */ > #define MVPP22_XPCS_BASE(port) (0x7400 + (port) * 0x1000) > #define MVPP22_XPCS_CFG0 0x0 > +#define MVPP22_XPCS_CFG0_RESET_DIS BIT(0) > #define MVPP22_XPCS_CFG0_PCS_MODE(n) ((n) << 3) > #define MVPP22_XPCS_CFG0_ACTIVE_LANE(n) ((n) << 5) > > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > index 4a18f8e54c90..5d05306e79a8 100644 > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > @@ -1016,13 +1016,19 @@ static void mvpp22_gop_init_10gkr(struct mvpp2_port *port) > void __iomem *xpcs = priv->iface_base + MVPP22_XPCS_BASE(port->gop_id); > u32 val; > > - /* XPCS */ > + /* XPCS : Reset the XPCS when reconfiguring the lanes */ > + val = readl(xpcs + MVPP22_XPCS_CFG0); > + writel(val & ~MVPP22_XPCS_CFG0_RESET_DIS, xpcs + MVPP22_XPCS_CFG0); > + > val = readl(xpcs + MVPP22_XPCS_CFG0); > val &= ~(MVPP22_XPCS_CFG0_PCS_MODE(0x3) | > MVPP22_XPCS_CFG0_ACTIVE_LANE(0x3)); > val |= MVPP22_XPCS_CFG0_ACTIVE_LANE(2); > writel(val, xpcs + MVPP22_XPCS_CFG0); > > + val = readl(xpcs + MVPP22_XPCS_CFG0); > + writel(val | MVPP22_XPCS_CFG0_RESET_DIS, xpcs + MVPP22_XPCS_CFG0); > + > /* MPCS */ > val = readl(mpcs + MVPP22_MPCS_CTRL); > val &= ~MVPP22_MPCS_CTRL_FWD_ERR_CONN; > -- > 2.20.1 > >
> On Fri, Feb 15, 2019 at 04:32:38PM +0100, Antoine Tenart wrote: > > The documentation advises to set the XPCS in reset while reconfiguring > > the serdes lanes. This seems to be a good thing to do, but the PPv2 > > driver wasn't doing it. This patch fixes it. > > Hmm. That statment seems to have some ambiguity in it - we do two > "reconfigurations" - one may be upon initialisation, where the lane is already > configured for 10Gbase-KR, and we're re-initialising it for the same mode. > The other case is when we're switching between 10Gbase-KR and SGMII, or > as will be the case with 2.5G support for the Alaska PHYs, 2500base-X. Exist one mode that we should add to PPv2&COMPHY 5Gbase-KR(5GBASE-T). Stefan, Best Regards.
Hi Russell, On Fri, Feb 15, 2019 at 05:12:24PM +0000, Russell King - ARM Linux admin wrote: > On Fri, Feb 15, 2019 at 04:32:38PM +0100, Antoine Tenart wrote: > > The documentation advises to set the XPCS in reset while reconfiguring > > the serdes lanes. This seems to be a good thing to do, but the PPv2 > > driver wasn't doing it. This patch fixes it. > > Hmm. That statment seems to have some ambiguity in it - we do two > "reconfigurations" - one may be upon initialisation, where the lane > is already configured for 10Gbase-KR, and we're re-initialising it > for the same mode. The other case is when we're switching between > 10Gbase-KR and SGMII, or as will be the case with 2.5G support for > the Alaska PHYs, 2500base-X. The configuration at the lane at boot time is dependent to the firmware or bootloader configuration. On the mcbin, the lane may be configured in 10Gbase-KR, but it could be configured in SGMII as well. The configuration upon initialization and the re-configuration are quite similar then, as we might change mode as well at boot time. You're right in that we might be re-configuring the lane for the same exact mode at boot time, if it was already configured in the same mode. > Does this apply to reconfiguration of the serdes lane between > 10Gbase-KR and slower modes? This applies only when configuring a line in a 10G mode, mvpp22_gop_init_10gkr isn't called otherwise. When switching to an non-10G mode we might want to put the XPCS in reset though, which is not done today with this patch. Thanks, Antoine
On Mon, Feb 18, 2019 at 11:26:30AM +0100, Antoine Tenart wrote: > Hi Russell, > > On Fri, Feb 15, 2019 at 05:12:24PM +0000, Russell King - ARM Linux admin wrote: > > On Fri, Feb 15, 2019 at 04:32:38PM +0100, Antoine Tenart wrote: > > > The documentation advises to set the XPCS in reset while reconfiguring > > > the serdes lanes. This seems to be a good thing to do, but the PPv2 > > > driver wasn't doing it. This patch fixes it. > > > > Hmm. That statment seems to have some ambiguity in it - we do two > > "reconfigurations" - one may be upon initialisation, where the lane > > is already configured for 10Gbase-KR, and we're re-initialising it > > for the same mode. The other case is when we're switching between > > 10Gbase-KR and SGMII, or as will be the case with 2.5G support for > > the Alaska PHYs, 2500base-X. > > The configuration at the lane at boot time is dependent to the > firmware or bootloader configuration. On the mcbin, the lane may be > configured in 10Gbase-KR, but it could be configured in SGMII as well. > The configuration upon initialization and the re-configuration are quite > similar then, as we might change mode as well at boot time. > > You're right in that we might be re-configuring the lane for the same > exact mode at boot time, if it was already configured in the same mode. > > > Does this apply to reconfiguration of the serdes lane between > > 10Gbase-KR and slower modes? > > This applies only when configuring a line in a 10G mode, > mvpp22_gop_init_10gkr isn't called otherwise. > > When switching to an non-10G mode we might want to put the XPCS in reset > though, which is not done today with this patch. I'm merely pointing out the discrepency between the commit message and what is actually being done. I'm not particularly concerned about what happens at boot. We have four different transitions a port can go through, all of which reconfigure the serdes lanes: 1. 10gkr -> 10gkr 2. 10gkr -> non-10gkr 3. non-10gkr -> non-10gkr 4. non-10gkr -> 10gkr With this patch, XPCS is only placed into reset during the reconfiguration for cases 1 and 4. Case 3 doesn't matter (the XPCS should already be in reset right?) Case 2 isn't covered, and this leaves a rather big hole. It seems to me that if the documentation states that the XPCS needs to be placed in reset while the serdes is reconfigured, then what should be happening is: - at boot, place the XPCS into reset. - when we configure for 10gkr, release the reset once we've finished configuring the serdes. - when we configure away from 10gkr, place the XPCS back into reset before configuring the serdes. Merely placing the XPCS into reset while we configure the serdes for 10gkr doesn't seem to be "fixing" the driver to conform to your commit message opening statement.
On Mon, Feb 18, 2019 at 10:43:02AM +0000, Russell King - ARM Linux admin wrote: > On Mon, Feb 18, 2019 at 11:26:30AM +0100, Antoine Tenart wrote: > > Hi Russell, > > > > On Fri, Feb 15, 2019 at 05:12:24PM +0000, Russell King - ARM Linux admin wrote: > > > On Fri, Feb 15, 2019 at 04:32:38PM +0100, Antoine Tenart wrote: > > > > The documentation advises to set the XPCS in reset while reconfiguring > > > > the serdes lanes. This seems to be a good thing to do, but the PPv2 > > > > driver wasn't doing it. This patch fixes it. > > > > > > Hmm. That statment seems to have some ambiguity in it - we do two > > > "reconfigurations" - one may be upon initialisation, where the lane > > > is already configured for 10Gbase-KR, and we're re-initialising it > > > for the same mode. The other case is when we're switching between > > > 10Gbase-KR and SGMII, or as will be the case with 2.5G support for > > > the Alaska PHYs, 2500base-X. > > > > The configuration at the lane at boot time is dependent to the > > firmware or bootloader configuration. On the mcbin, the lane may be > > configured in 10Gbase-KR, but it could be configured in SGMII as well. > > The configuration upon initialization and the re-configuration are quite > > similar then, as we might change mode as well at boot time. > > > > You're right in that we might be re-configuring the lane for the same > > exact mode at boot time, if it was already configured in the same mode. > > > > > Does this apply to reconfiguration of the serdes lane between > > > 10Gbase-KR and slower modes? > > > > This applies only when configuring a line in a 10G mode, > > mvpp22_gop_init_10gkr isn't called otherwise. > > > > When switching to an non-10G mode we might want to put the XPCS in reset > > though, which is not done today with this patch. > > I'm merely pointing out the discrepency between the commit message and > what is actually being done. I'm not particularly concerned about what > happens at boot. > > We have four different transitions a port can go through, all of which > reconfigure the serdes lanes: > > 1. 10gkr -> 10gkr > 2. 10gkr -> non-10gkr > 3. non-10gkr -> non-10gkr > 4. non-10gkr -> 10gkr > > With this patch, XPCS is only placed into reset during the > reconfiguration for cases 1 and 4. Case 3 doesn't matter (the XPCS > should already be in reset right?) Case 2 isn't covered, and this > leaves a rather big hole. > > It seems to me that if the documentation states that the XPCS needs to > be placed in reset while the serdes is reconfigured, then what should > be happening is: > > - at boot, place the XPCS into reset. > - when we configure for 10gkr, release the reset once we've finished > configuring the serdes. > - when we configure away from 10gkr, place the XPCS back into reset > before configuring the serdes. > > Merely placing the XPCS into reset while we configure the serdes for > 10gkr doesn't seem to be "fixing" the driver to conform to your commit > message opening statement. Another case that needs to be considered: if the XPCS should be placed into reset while reconfiguring the serdes lanes, is the same treatment needed for the GMAC?
Russell, On Mon, Feb 18, 2019 at 10:43:02AM +0000, Russell King - ARM Linux admin wrote: > On Mon, Feb 18, 2019 at 11:26:30AM +0100, Antoine Tenart wrote: > > On Fri, Feb 15, 2019 at 05:12:24PM +0000, Russell King - ARM Linux admin wrote: > > > On Fri, Feb 15, 2019 at 04:32:38PM +0100, Antoine Tenart wrote: > > > > The documentation advises to set the XPCS in reset while reconfiguring > > > > the serdes lanes. This seems to be a good thing to do, but the PPv2 > > > > driver wasn't doing it. This patch fixes it. > > > > > > Hmm. That statment seems to have some ambiguity in it - we do two > > > "reconfigurations" - one may be upon initialisation, where the lane > > > is already configured for 10Gbase-KR, and we're re-initialising it > > > for the same mode. The other case is when we're switching between > > > 10Gbase-KR and SGMII, or as will be the case with 2.5G support for > > > the Alaska PHYs, 2500base-X. > > > > The configuration at the lane at boot time is dependent to the > > firmware or bootloader configuration. On the mcbin, the lane may be > > configured in 10Gbase-KR, but it could be configured in SGMII as well. > > The configuration upon initialization and the re-configuration are quite > > similar then, as we might change mode as well at boot time. > > > > You're right in that we might be re-configuring the lane for the same > > exact mode at boot time, if it was already configured in the same mode. > > > > > Does this apply to reconfiguration of the serdes lane between > > > 10Gbase-KR and slower modes? > > > > This applies only when configuring a line in a 10G mode, > > mvpp22_gop_init_10gkr isn't called otherwise. > > > > When switching to an non-10G mode we might want to put the XPCS in reset > > though, which is not done today with this patch. > > I'm merely pointing out the discrepency between the commit message and > what is actually being done. I'm not particularly concerned about what > happens at boot. > > We have four different transitions a port can go through, all of which > reconfigure the serdes lanes: > > 1. 10gkr -> 10gkr > 2. 10gkr -> non-10gkr > 3. non-10gkr -> non-10gkr > 4. non-10gkr -> 10gkr > > With this patch, XPCS is only placed into reset during the > reconfiguration for cases 1 and 4. Case 3 doesn't matter (the XPCS > should already be in reset right?) Case 2 isn't covered, and this > leaves a rather big hole. > > It seems to me that if the documentation states that the XPCS needs to > be placed in reset while the serdes is reconfigured, then what should > be happening is: > > - at boot, place the XPCS into reset. > - when we configure for 10gkr, release the reset once we've finished > configuring the serdes. > - when we configure away from 10gkr, place the XPCS back into reset > before configuring the serdes. > > Merely placing the XPCS into reset while we configure the serdes for > 10gkr doesn't seem to be "fixing" the driver to conform to your commit > message opening statement. That's right, we should put the XPCS in reset when booting and when switching away from a 10G mode. I'll fix that in v2. Antoine
Russell, On Mon, Feb 18, 2019 at 10:47:57AM +0000, Russell King - ARM Linux admin wrote: > > Another case that needs to be considered: if the XPCS should be placed > into reset while reconfiguring the serdes lanes, is the same treatment > needed for the GMAC? That's something I wanted to check as well. I don't know for the GMAC, while I'm sure the documentation explicitly state to put the XPCS in reset when reconfiguring the lanes. Antoine
> -----Original Message----- > From: Antoine Tenart <antoine.tenart@bootlin.com> > Sent: Monday, February 18, 2019 12:52 PM > To: Russell King - ARM Linux admin <linux@armlinux.org.uk> > Cc: Antoine Tenart <antoine.tenart@bootlin.com>; davem@davemloft.net; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; > thomas.petazzoni@bootlin.com; maxime.chevallier@bootlin.com; > gregory.clement@bootlin.com; miquel.raynal@bootlin.com; Nadav Haklai > <nadavh@marvell.com>; Stefan Chulski <stefanc@marvell.com>; Yan > Markman <ymarkman@marvell.com>; mw@semihalf.com > Subject: [EXT] Re: [PATCH net-next 10/13] net: mvpp2: reset the XPCS while > reconfiguring the serdes lanes > > External Email > > ---------------------------------------------------------------------- > Russell, > > On Mon, Feb 18, 2019 at 10:47:57AM +0000, Russell King - ARM Linux admin > wrote: > > > > Another case that needs to be considered: if the XPCS should be placed > > into reset while reconfiguring the serdes lanes, is the same treatment > > needed for the GMAC? > > That's something I wanted to check as well. I don't know for the GMAC, while > I'm sure the documentation explicitly state to put the XPCS in reset when > reconfiguring the lanes. HW recommendation upon Serdes reconfiguration are the following: 1. Disable port(CTRL0_REG - in XLG/GMAC) 2. Put port in reset (both XLG/GMAC) 3. For KR - put in reset MPCS (MAC control clock, RX SD clock, TX SD clock), XPSC is RXAUI/XAUI clock domain 4. Power down Serdes lane Do reconfiguration of Serdes. 5. Enable Serdes lane 6. Disable MPCS reset for KR 7. Disable port reset (both XLG/GMAC) 8. Enable port (both XLG/GMAC) Stefan, Best Regards.
Hi Stefan, On Mon, Feb 18, 2019 at 11:08:34AM +0000, Stefan Chulski wrote: > HW recommendation upon Serdes reconfiguration are the following: > > 1. Disable port(CTRL0_REG - in XLG/GMAC) > 2. Put port in reset (both XLG/GMAC) > 3. For KR - put in reset MPCS (MAC control clock, RX SD clock, TX SD clock), XPSC is RXAUI/XAUI clock domain > 4. Power down Serdes lane > > Do reconfiguration of Serdes. > > 5. Enable Serdes lane > 6. Disable MPCS reset for KR > 7. Disable port reset (both XLG/GMAC) > 8. Enable port (both XLG/GMAC) For clarity, presumably either the XLG or the GMAC should be released from reset and enabled at any one time depending on the configured mode, but never both together?
> -----Original Message----- > From: Russell King - ARM Linux admin <linux@armlinux.org.uk> > Sent: Monday, February 18, 2019 1:28 PM > To: Stefan Chulski <stefanc@marvell.com> > Cc: Antoine Tenart <antoine.tenart@bootlin.com>; davem@davemloft.net; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; > thomas.petazzoni@bootlin.com; maxime.chevallier@bootlin.com; > gregory.clement@bootlin.com; miquel.raynal@bootlin.com; Nadav Haklai > <nadavh@marvell.com>; Yan Markman <ymarkman@marvell.com>; > mw@semihalf.com > Subject: Re: [EXT] Re: [PATCH net-next 10/13] net: mvpp2: reset the XPCS > while reconfiguring the serdes lanes > > Hi Stefan, > > On Mon, Feb 18, 2019 at 11:08:34AM +0000, Stefan Chulski wrote: > > HW recommendation upon Serdes reconfiguration are the following: > > > > 1. Disable port(CTRL0_REG - in XLG/GMAC) 2. Put port in reset (both > > XLG/GMAC) 3. For KR - put in reset MPCS (MAC control clock, RX SD > > clock, TX SD clock), XPSC is RXAUI/XAUI clock domain 4. Power down > > Serdes lane > > > > Do reconfiguration of Serdes. > > > > 5. Enable Serdes lane > > 6. Disable MPCS reset for KR > > 7. Disable port reset (both XLG/GMAC) > > 8. Enable port (both XLG/GMAC) > > For clarity, presumably either the XLG or the GMAC should be released from > reset and enabled at any one time depending on the configured mode, but > never both together? Yes. Only one of them the XLG or the GMAC ,depending on the configured mode. Best Regards.
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h index 21ddcac1ceea..7380bddc53b8 100644 --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h @@ -481,6 +481,7 @@ /* XPCS registers. PPv2.2 only */ #define MVPP22_XPCS_BASE(port) (0x7400 + (port) * 0x1000) #define MVPP22_XPCS_CFG0 0x0 +#define MVPP22_XPCS_CFG0_RESET_DIS BIT(0) #define MVPP22_XPCS_CFG0_PCS_MODE(n) ((n) << 3) #define MVPP22_XPCS_CFG0_ACTIVE_LANE(n) ((n) << 5) diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c index 4a18f8e54c90..5d05306e79a8 100644 --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c @@ -1016,13 +1016,19 @@ static void mvpp22_gop_init_10gkr(struct mvpp2_port *port) void __iomem *xpcs = priv->iface_base + MVPP22_XPCS_BASE(port->gop_id); u32 val; - /* XPCS */ + /* XPCS : Reset the XPCS when reconfiguring the lanes */ + val = readl(xpcs + MVPP22_XPCS_CFG0); + writel(val & ~MVPP22_XPCS_CFG0_RESET_DIS, xpcs + MVPP22_XPCS_CFG0); + val = readl(xpcs + MVPP22_XPCS_CFG0); val &= ~(MVPP22_XPCS_CFG0_PCS_MODE(0x3) | MVPP22_XPCS_CFG0_ACTIVE_LANE(0x3)); val |= MVPP22_XPCS_CFG0_ACTIVE_LANE(2); writel(val, xpcs + MVPP22_XPCS_CFG0); + val = readl(xpcs + MVPP22_XPCS_CFG0); + writel(val | MVPP22_XPCS_CFG0_RESET_DIS, xpcs + MVPP22_XPCS_CFG0); + /* MPCS */ val = readl(mpcs + MVPP22_MPCS_CTRL); val &= ~MVPP22_MPCS_CTRL_FWD_ERR_CONN;
The documentation advises to set the XPCS in reset while reconfiguring the serdes lanes. This seems to be a good thing to do, but the PPv2 driver wasn't doing it. This patch fixes it. Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com> --- drivers/net/ethernet/marvell/mvpp2/mvpp2.h | 1 + drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 8 +++++++- 2 files changed, 8 insertions(+), 1 deletion(-)