[PATCH 0/2] net: phy: broadcom: RGMII delays fixes

[PATCH 0/2] net: phy: broadcom: RGMII delays fixes

[Florian Fainelli]

Hi all,

This patch series fixes the BCM54210E RGMII delay configuration which
could only have worked in a PHY_INTERFACE_MODE_RGMII configuration.
There is a forward declaration added such that the first patch can be
picked up for -stable and apply fine all the way back to when the bug
was introduced.

The second patch eliminates duplicated code that used a different kind
of logic and did not use existing constants defined.

Thanks

Florian Fainelli (2):
  net: phy: broadcom: Fix RGMII delays configuration for BCM54210E
  net: phy: broadcom: Use bcm54xx_config_clock_delay() for BCM54612E

 drivers/net/phy/broadcom.c | 32 ++++----------------------------
 1 file changed, 4 insertions(+), 28 deletions(-)

-- 
2.17.1

[PATCH 1/2] net: phy: broadcom: Fix RGMII delays configuration for BCM54210E

[Florian Fainelli]

Commit 0fc9ae107669 ("net: phy: broadcom: add support for
BCM54210E") added support for BCM54210E but also unconditionally cleared
the RXC to RXD skew and the TXD to TXC skew, thus only making
PHY_INTERFACE_MODE_RGMII a possible configuration. Use
bcm54xx_config_clock_delay() which correctly sets the registers
depending on the 4 possible PHY interface values that exist for RGMII.

Fixes: 0fc9ae107669 ("net: phy: broadcom: add support for BCM54210E")
Reported-by: Manasa Mudireddy <manasa.mudireddy@broadcom.com>
Reported-by: Ray Jui <ray.jui@broadcom.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/broadcom.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 937d0059e8ac..5e956089bf52 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -26,18 +26,13 @@ MODULE_DESCRIPTION("Broadcom PHY driver");
 MODULE_AUTHOR("Maciej W. Rozycki");
 MODULE_LICENSE("GPL");
 
+static int bcm54xx_config_clock_delay(struct phy_device *phydev);
+
 static int bcm54210e_config_init(struct phy_device *phydev)
 {
 	int val;
 
-	val = bcm54xx_auxctl_read(phydev, MII_BCM54XX_AUXCTL_SHDWSEL_MISC);
-	val &= ~MII_BCM54XX_AUXCTL_SHDWSEL_MISC_RGMII_SKEW_EN;
-	val |= MII_BCM54XX_AUXCTL_MISC_WREN;
-	bcm54xx_auxctl_write(phydev, MII_BCM54XX_AUXCTL_SHDWSEL_MISC, val);
-
-	val = bcm_phy_read_shadow(phydev, BCM54810_SHD_CLK_CTL);
-	val &= ~BCM54810_SHD_CLK_CTL_GTXCLK_EN;
-	bcm_phy_write_shadow(phydev, BCM54810_SHD_CLK_CTL, val);
+	bcm54xx_config_clock_delay(phydev);
 
 	if (phydev->dev_flags & PHY_BRCM_EN_MASTER_MODE) {
 		val = phy_read(phydev, MII_CTRL1000);
-- 
2.17.1

[PATCH 2/2] net: phy: broadcom: Use bcm54xx_config_clock_delay() for BCM54612E

[Florian Fainelli]

bcm54612e_config_init() duplicates what bcm54xx_config_clock_delay()
does with respect to configuring RGMII TX/RX delays appropriately.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/broadcom.c | 21 +--------------------
 1 file changed, 1 insertion(+), 20 deletions(-)

diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 5e956089bf52..4313c74b4fd8 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -47,26 +47,7 @@ static int bcm54612e_config_init(struct phy_device *phydev)
 {
 	int reg;
 
-	/* Clear TX internal delay unless requested. */
-	if ((phydev->interface != PHY_INTERFACE_MODE_RGMII_ID) &&
-	    (phydev->interface != PHY_INTERFACE_MODE_RGMII_TXID)) {
-		/* Disable TXD to GTXCLK clock delay (default set) */
-		/* Bit 9 is the only field in shadow register 00011 */
-		bcm_phy_write_shadow(phydev, 0x03, 0);
-	}
-
-	/* Clear RX internal delay unless requested. */
-	if ((phydev->interface != PHY_INTERFACE_MODE_RGMII_ID) &&
-	    (phydev->interface != PHY_INTERFACE_MODE_RGMII_RXID)) {
-		reg = bcm54xx_auxctl_read(phydev,
-					  MII_BCM54XX_AUXCTL_SHDWSEL_MISC);
-		/* Disable RXD to RXC delay (default set) */
-		reg &= ~MII_BCM54XX_AUXCTL_SHDWSEL_MISC_RGMII_SKEW_EN;
-		/* Clear shadow selector field */
-		reg &= ~MII_BCM54XX_AUXCTL_SHDWSEL_MASK;
-		bcm54xx_auxctl_write(phydev, MII_BCM54XX_AUXCTL_SHDWSEL_MISC,
-				     MII_BCM54XX_AUXCTL_MISC_WREN | reg);
-	}
+	bcm54xx_config_clock_delay(phydev);
 
 	/* Enable CLK125 MUX on LED4 if ref clock is enabled. */
 	if (!(phydev->dev_flags & PHY_BRCM_RX_REFCLK_UNUSED)) {
-- 
2.17.1

Re: [PATCH 0/2] net: phy: broadcom: RGMII delays fixes

[Andrew Lunn]

On Thu, Oct 03, 2019 at 11:43:50AM -0700, Florian Fainelli wrote:
> Hi all,
> 
> This patch series fixes the BCM54210E RGMII delay configuration which
> could only have worked in a PHY_INTERFACE_MODE_RGMII configuration.

Hi Florian

So any DT blob which incorrectly uses one of the other RGMII modes is
now going to break, where as before it was ignored.

Maybe we should let this sit in net-next for a while, before back
porting, so we get an idea of how many platforms we might be about to
break?

   Andrew

Re: [PATCH 0/2] net: phy: broadcom: RGMII delays fixes

[Florian Fainelli]

Hi Andrew,

On 10/3/19 11:51 AM, Andrew Lunn wrote:
> On Thu, Oct 03, 2019 at 11:43:50AM -0700, Florian Fainelli wrote:
>> Hi all,
>>
>> This patch series fixes the BCM54210E RGMII delay configuration which
>> could only have worked in a PHY_INTERFACE_MODE_RGMII configuration.
> 
> Hi Florian
> 
> So any DT blob which incorrectly uses one of the other RGMII modes is
> now going to break, where as before it was ignored.

Potentially yes. There is a precedent with the at803x PHY driver, and
both changes here aim for correcting that mistake. The PHY driver not
looking at phy_interface_t is a bug though. I will have a separate
conversation about solving those problems in a more generic way with you
and Heiner.

Rafal, do you have the platform DTS for which you added BCM54210E
support available somewhere so we can see whether this is going to break?

> 
> Maybe we should let this sit in net-next for a while, before back
> porting, so we get an idea of how many platforms we might be about to
> break?

Fine with me. It solves Ray's and Manasa's customer problem immediately
and they already back ported the first change in the their tree AFAICT.
-- 
Florian

Re: [PATCH 0/2] net: phy: broadcom: RGMII delays fixes

[Andrew Lunn]

On Thu, Oct 03, 2019 at 11:55:40AM -0700, Florian Fainelli wrote:
> Hi Andrew,
> 
> On 10/3/19 11:51 AM, Andrew Lunn wrote:
> > On Thu, Oct 03, 2019 at 11:43:50AM -0700, Florian Fainelli wrote:
> >> Hi all,
> >>
> >> This patch series fixes the BCM54210E RGMII delay configuration which
> >> could only have worked in a PHY_INTERFACE_MODE_RGMII configuration.
> > 
> > Hi Florian
> > 
> > So any DT blob which incorrectly uses one of the other RGMII modes is
> > now going to break, where as before it was ignored.
> 
> Potentially yes. There is a precedent with the at803x PHY driver

Hi Florian

Yes that was an interesting learning experience. I'm not sure we want
to do that again. A lot of devices broken, and a lot of people were
unhappy.

If we are looking at a similar scale of breakage, i think i would
prefer to add a broadcom,bcm54210e-phy-mode property in the DT which
if present would override the phy_interface_t passed to the driver.

   Andrew

Re: [PATCH 0/2] net: phy: broadcom: RGMII delays fixes

[Vladimir Oltean]

Hi Andrew,

On Thu, 3 Oct 2019 at 22:06, Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Thu, Oct 03, 2019 at 11:55:40AM -0700, Florian Fainelli wrote:
> > Hi Andrew,
> >
> > On 10/3/19 11:51 AM, Andrew Lunn wrote:
> > > On Thu, Oct 03, 2019 at 11:43:50AM -0700, Florian Fainelli wrote:
> > >> Hi all,
> > >>
> > >> This patch series fixes the BCM54210E RGMII delay configuration which
> > >> could only have worked in a PHY_INTERFACE_MODE_RGMII configuration.
> > >
> > > Hi Florian
> > >
> > > So any DT blob which incorrectly uses one of the other RGMII modes is
> > > now going to break, where as before it was ignored.
> >
> > Potentially yes. There is a precedent with the at803x PHY driver
>
> Hi Florian
>
> Yes that was an interesting learning experience. I'm not sure we want
> to do that again. A lot of devices broken, and a lot of people were
> unhappy.
>
> If we are looking at a similar scale of breakage, i think i would
> prefer to add a broadcom,bcm54210e-phy-mode property in the DT which
> if present would override the phy_interface_t passed to the driver.
>
>    Andrew

What is the breakage concern here?
The driver was unconditionally clearing the RGMII delays. Therefore,
any board that needed them would have noticed really fast, IMO. That
should include people who configure 'rgmii-id' in the DT in the hope
that it would solve some problems.
The typical RGMII delay breakage is not realizing you need Linux to
enable RGMII delays (perhaps due to strapping) and specifying plain
"rgmii" in the phy-mode, then somebody 'fixing' those and disabling
them.
But in this case, the only breakage would be "hmmm, let's just enable
RGMII delays everywhere. So it works with rgmii-id on both the PHY and
the MAC side of things? Great, time for lunch!". I just hope that did
not happen. And maybe even if it did, AFAIK the BCM54xx skews the data
lines by 1.9 ns (unfortunately not configurable), that leaves an extra
~2.1 ns of timing budget until the next RGMII clock transition,
considering a 50% duty cycle? Maybe that's enough to fit the MAC's
RGMII I/O buffer delays without breaking anything, and then it doesn't
really matter?

Regards,
-Vladimir

Re: [PATCH 0/2] net: phy: broadcom: RGMII delays fixes

[Andrew Lunn]

On Thu, Oct 03, 2019 at 10:54:26PM +0300, Vladimir Oltean wrote:
> Hi Andrew,
> 
> On Thu, 3 Oct 2019 at 22:06, Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Thu, Oct 03, 2019 at 11:55:40AM -0700, Florian Fainelli wrote:
> > > Hi Andrew,
> > >
> > > On 10/3/19 11:51 AM, Andrew Lunn wrote:
> > > > On Thu, Oct 03, 2019 at 11:43:50AM -0700, Florian Fainelli wrote:
> > > >> Hi all,
> > > >>
> > > >> This patch series fixes the BCM54210E RGMII delay configuration which
> > > >> could only have worked in a PHY_INTERFACE_MODE_RGMII configuration.
> > > >
> > > > Hi Florian
> > > >
> > > > So any DT blob which incorrectly uses one of the other RGMII modes is
> > > > now going to break, where as before it was ignored.
> > >
> > > Potentially yes. There is a precedent with the at803x PHY driver
> >
> > Hi Florian
> >
> > Yes that was an interesting learning experience. I'm not sure we want
> > to do that again. A lot of devices broken, and a lot of people were
> > unhappy.
> >
> > If we are looking at a similar scale of breakage, i think i would
> > prefer to add a broadcom,bcm54210e-phy-mode property in the DT which
> > if present would override the phy_interface_t passed to the driver.
> >
> >    Andrew
> 
> What is the breakage concern here?

With the at803x, we had a lot of devices which said rgmii in there DT,
but actually needed rgmii-id. The driver however did not do anything,
and the silicon defaulted to rgmii-id, so things just worked, the two
bugs cancelling each other out.

Then a board come along which really did need rgmii. Adding support to
actually correctly configure the RGMII delays then broke all the
boards with the wrong value in DT.

> But in this case, the only breakage would be "hmmm, let's just enable
> RGMII delays everywhere. So it works with rgmii-id on both the PHY and
> the MAC side of things? Great, time for lunch!". I just hope that did
> not happen.

That is my hope as well. But letting it sit in net-next for a while
might help confirm that hope. As i said, at803x was painful, and i
would like to avoid that again. So i'm being more cautious.

      Andrew

Re: [PATCH 0/2] net: phy: broadcom: RGMII delays fixes

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Thu,  3 Oct 2019 11:43:50 -0700

> This patch series fixes the BCM54210E RGMII delay configuration which
> could only have worked in a PHY_INTERFACE_MODE_RGMII configuration.
> There is a forward declaration added such that the first patch can be
> picked up for -stable and apply fine all the way back to when the bug
> was introduced.
> 
> The second patch eliminates duplicated code that used a different kind
> of logic and did not use existing constants defined.

Based upon the discussion with Andrew, I am applying this to net-next.

Thanks.

Re: [PATCH 0/2] net: phy: broadcom: RGMII delays fixes

[Florian Fainelli]

On 10/4/19 2:13 PM, David Miller wrote:
> From: Florian Fainelli <f.fainelli@gmail.com>
> Date: Thu,  3 Oct 2019 11:43:50 -0700
> 
>> This patch series fixes the BCM54210E RGMII delay configuration which
>> could only have worked in a PHY_INTERFACE_MODE_RGMII configuration.
>> There is a forward declaration added such that the first patch can be
>> picked up for -stable and apply fine all the way back to when the bug
>> was introduced.
>>
>> The second patch eliminates duplicated code that used a different kind
>> of logic and did not use existing constants defined.
> 
> Based upon the discussion with Andrew, I am applying this to net-next.

Sounds good, thanks David.
-- 
Florian