* [PATCH 0/2] net: phy: broadcom: RGMII delays fixes
@ 2019-10-03 18:43 Florian Fainelli
2019-10-03 18:43 ` [PATCH 1/2] net: phy: broadcom: Fix RGMII delays configuration for BCM54210E Florian Fainelli
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Florian Fainelli @ 2019-10-03 18:43 UTC (permalink / raw)
To: netdev
Cc: Florian Fainelli, Andrew Lunn, David S. Miller, open list,
hkallweit1, bcm-kernel-feedback-list, manasa.mudireddy, ray.jui,
olteanv, rafal
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
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/2] net: phy: broadcom: Fix RGMII delays configuration for BCM54210E 2019-10-03 18:43 [PATCH 0/2] net: phy: broadcom: RGMII delays fixes Florian Fainelli @ 2019-10-03 18:43 ` Florian Fainelli 2019-10-03 18:43 ` [PATCH 2/2] net: phy: broadcom: Use bcm54xx_config_clock_delay() for BCM54612E Florian Fainelli ` (2 subsequent siblings) 3 siblings, 0 replies; 10+ messages in thread From: Florian Fainelli @ 2019-10-03 18:43 UTC (permalink / raw) To: netdev Cc: Florian Fainelli, Andrew Lunn, David S. Miller, open list, hkallweit1, bcm-kernel-feedback-list, manasa.mudireddy, ray.jui, olteanv, rafal 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] net: phy: broadcom: Use bcm54xx_config_clock_delay() for BCM54612E 2019-10-03 18:43 [PATCH 0/2] net: phy: broadcom: RGMII delays fixes Florian Fainelli 2019-10-03 18:43 ` [PATCH 1/2] net: phy: broadcom: Fix RGMII delays configuration for BCM54210E Florian Fainelli @ 2019-10-03 18:43 ` Florian Fainelli 2019-10-03 18:51 ` [PATCH 0/2] net: phy: broadcom: RGMII delays fixes Andrew Lunn 2019-10-04 21:13 ` David Miller 3 siblings, 0 replies; 10+ messages in thread From: Florian Fainelli @ 2019-10-03 18:43 UTC (permalink / raw) To: netdev Cc: Florian Fainelli, Andrew Lunn, David S. Miller, open list, hkallweit1, bcm-kernel-feedback-list, manasa.mudireddy, ray.jui, olteanv, rafal 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] net: phy: broadcom: RGMII delays fixes 2019-10-03 18:43 [PATCH 0/2] net: phy: broadcom: RGMII delays fixes Florian Fainelli 2019-10-03 18:43 ` [PATCH 1/2] net: phy: broadcom: Fix RGMII delays configuration for BCM54210E Florian Fainelli 2019-10-03 18:43 ` [PATCH 2/2] net: phy: broadcom: Use bcm54xx_config_clock_delay() for BCM54612E Florian Fainelli @ 2019-10-03 18:51 ` Andrew Lunn 2019-10-03 18:55 ` Florian Fainelli 2019-10-04 21:13 ` David Miller 3 siblings, 1 reply; 10+ messages in thread From: Andrew Lunn @ 2019-10-03 18:51 UTC (permalink / raw) To: Florian Fainelli Cc: netdev, David S. Miller, open list, hkallweit1, bcm-kernel-feedback-list, manasa.mudireddy, ray.jui, olteanv, rafal 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] net: phy: broadcom: RGMII delays fixes 2019-10-03 18:51 ` [PATCH 0/2] net: phy: broadcom: RGMII delays fixes Andrew Lunn @ 2019-10-03 18:55 ` Florian Fainelli 2019-10-03 19:06 ` Andrew Lunn 0 siblings, 1 reply; 10+ messages in thread From: Florian Fainelli @ 2019-10-03 18:55 UTC (permalink / raw) To: Andrew Lunn, Florian Fainelli Cc: netdev, David S. Miller, open list, hkallweit1, bcm-kernel-feedback-list, manasa.mudireddy, ray.jui, olteanv, rafal 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] net: phy: broadcom: RGMII delays fixes 2019-10-03 18:55 ` Florian Fainelli @ 2019-10-03 19:06 ` Andrew Lunn 2019-10-03 19:54 ` Vladimir Oltean 0 siblings, 1 reply; 10+ messages in thread From: Andrew Lunn @ 2019-10-03 19:06 UTC (permalink / raw) To: Florian Fainelli Cc: netdev, David S. Miller, open list, hkallweit1, bcm-kernel-feedback-list, manasa.mudireddy, ray.jui, olteanv, rafal 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] net: phy: broadcom: RGMII delays fixes 2019-10-03 19:06 ` Andrew Lunn @ 2019-10-03 19:54 ` Vladimir Oltean 2019-10-03 20:10 ` Andrew Lunn 0 siblings, 1 reply; 10+ messages in thread From: Vladimir Oltean @ 2019-10-03 19:54 UTC (permalink / raw) To: Andrew Lunn Cc: Florian Fainelli, netdev, David S. Miller, open list, Heiner Kallweit, bcm-kernel-feedback-list, manasa.mudireddy, ray.jui, rafal 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] net: phy: broadcom: RGMII delays fixes 2019-10-03 19:54 ` Vladimir Oltean @ 2019-10-03 20:10 ` Andrew Lunn 0 siblings, 0 replies; 10+ messages in thread From: Andrew Lunn @ 2019-10-03 20:10 UTC (permalink / raw) To: Vladimir Oltean Cc: Florian Fainelli, netdev, David S. Miller, open list, Heiner Kallweit, bcm-kernel-feedback-list, manasa.mudireddy, ray.jui, rafal 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] net: phy: broadcom: RGMII delays fixes 2019-10-03 18:43 [PATCH 0/2] net: phy: broadcom: RGMII delays fixes Florian Fainelli ` (2 preceding siblings ...) 2019-10-03 18:51 ` [PATCH 0/2] net: phy: broadcom: RGMII delays fixes Andrew Lunn @ 2019-10-04 21:13 ` David Miller 2019-10-04 21:18 ` Florian Fainelli 3 siblings, 1 reply; 10+ messages in thread From: David Miller @ 2019-10-04 21:13 UTC (permalink / raw) To: f.fainelli Cc: netdev, andrew, linux-kernel, hkallweit1, bcm-kernel-feedback-list, manasa.mudireddy, ray.jui, olteanv, rafal 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] net: phy: broadcom: RGMII delays fixes 2019-10-04 21:13 ` David Miller @ 2019-10-04 21:18 ` Florian Fainelli 0 siblings, 0 replies; 10+ messages in thread From: Florian Fainelli @ 2019-10-04 21:18 UTC (permalink / raw) To: David Miller, f.fainelli Cc: netdev, andrew, linux-kernel, hkallweit1, bcm-kernel-feedback-list, manasa.mudireddy, ray.jui, olteanv, rafal 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-10-04 21:18 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-03 18:43 [PATCH 0/2] net: phy: broadcom: RGMII delays fixes Florian Fainelli 2019-10-03 18:43 ` [PATCH 1/2] net: phy: broadcom: Fix RGMII delays configuration for BCM54210E Florian Fainelli 2019-10-03 18:43 ` [PATCH 2/2] net: phy: broadcom: Use bcm54xx_config_clock_delay() for BCM54612E Florian Fainelli 2019-10-03 18:51 ` [PATCH 0/2] net: phy: broadcom: RGMII delays fixes Andrew Lunn 2019-10-03 18:55 ` Florian Fainelli 2019-10-03 19:06 ` Andrew Lunn 2019-10-03 19:54 ` Vladimir Oltean 2019-10-03 20:10 ` Andrew Lunn 2019-10-04 21:13 ` David Miller 2019-10-04 21:18 ` Florian Fainelli
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).