Re: [Potential Spoof] Re: [PATCH net-next v6 3/3] net: phy: broadcom: add 1000Base-X support for BCM54616S
On 8/9/19 2:59 PM, Heiner Kallweit wrote: > On 09.08.2019 23:13, Tao Ren wrote: >> On 8/9/19 1:54 PM, Tao Ren wrote: >>> Hi Heiner, >>> >>> On 8/9/19 1:21 PM, Heiner Kallweit wrote: On 09.08.2019 07:44, Tao Ren wrote: > The BCM54616S PHY cannot work properly in RGMII->1000Base-KX mode (for > example, on Facebook CMM BMC platform), mainly because genphy functions > are designed for copper links, and 1000Base-X (clause 37) auto negotiation > needs to be handled differently. > > This patch enables 1000Base-X support for BCM54616S by customizing 3 > driver callbacks: > > - probe: probe callback detects PHY's operation mode based on > INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX > Control register. > > - config_aneg: calls genphy_c37_config_aneg when the PHY is running in > 1000Base-X mode; otherwise, genphy_config_aneg will be called. > > - read_status: calls genphy_c37_read_status when the PHY is running in > 1000Base-X mode; otherwise, genphy_read_status will be called. > > Signed-off-by: Tao Ren > --- > Changes in v6: > - nothing changed. > Changes in v5: > - include Heiner's patch "net: phy: add support for clause 37 > auto-negotiation" into the series. > - use genphy_c37_config_aneg and genphy_c37_read_status in BCM54616S > PHY driver's callback when the PHY is running in 1000Base-X mode. > Changes in v4: > - add bcm54616s_config_aneg_1000bx() to deal with auto negotiation in > 1000Base-X mode. > Changes in v3: > - rename bcm5482_read_status to bcm54xx_read_status so the callback can > be shared by BCM5482 and BCM54616S. > Changes in v2: > - Auto-detect PHY operation mode instead of passing DT node. > - move PHY mode auto-detect logic from config_init to probe callback. > - only set speed (not including duplex) in read_status callback. > - update patch description with more background to avoid confusion. > - patch #1 in the series ("net: phy: broadcom: set features explicitly > for BCM54616") is dropped: the fix should go to get_features callback > which may potentially depend on this patch. > > drivers/net/phy/broadcom.c | 54 +++--- > include/linux/brcmphy.h| 10 +-- > 2 files changed, 58 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c > index 937d0059e8ac..fbd76a31c142 100644 > --- a/drivers/net/phy/broadcom.c > +++ b/drivers/net/phy/broadcom.c > @@ -383,9 +383,9 @@ static int bcm5482_config_init(struct phy_device > *phydev) > /* >* Select 1000BASE-X register set (primary SerDes) >*/ > - reg = bcm_phy_read_shadow(phydev, BCM5482_SHD_MODE); > - bcm_phy_write_shadow(phydev, BCM5482_SHD_MODE, > - reg | BCM5482_SHD_MODE_1000BX); > + reg = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE); > + bcm_phy_write_shadow(phydev, BCM54XX_SHD_MODE, > + reg | BCM54XX_SHD_MODE_1000BX); > > /* >* LED1=ACTIVITYLED, LED3=LINKSPD[2] > @@ -451,12 +451,44 @@ static int bcm5481_config_aneg(struct phy_device > *phydev) > return ret; > } > > +static int bcm54616s_probe(struct phy_device *phydev) > +{ > + int val, intf_sel; > + > + val = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE); > + if (val < 0) > + return val; > + > + /* The PHY is strapped in RGMII to fiber mode when INTERF_SEL[1:0] > + * is 01b. > + */ > + intf_sel = (val & BCM54XX_SHD_INTF_SEL_MASK) >> 1; > + if (intf_sel == 1) { > + val = bcm_phy_read_shadow(phydev, BCM54616S_SHD_100FX_CTRL); > + if (val < 0) > + return val; > + > + /* Bit 0 of the SerDes 100-FX Control register, when set > + * to 1, sets the MII/RGMII -> 100BASE-FX configuration. > + * When this bit is set to 0, it sets the GMII/RGMII -> > + * 1000BASE-X configuration. > + */ > + if (!(val & BCM54616S_100FX_MODE)) > + phydev->dev_flags |= PHY_BCM_FLAGS_MODE_1000BX; > + } > + > + return 0; > +} > + > static int bcm54616s_config_aneg(struct phy_device *phydev) > { > int ret; > > /* Aneg firsly. */ > - ret = genphy_config_aneg(phydev); > + if (phydev->dev_flags & PHY_BCM_FLAGS_MODE_1000BX) > + ret = genphy_c37_config_aneg(phydev); > + else > + ret = genphy_config_aneg(phydev); > I'm just wondering whether it needs to be considered that 100base-FX doesn't support auto-negotiation. I suppo
Re: [Potential Spoof] Re: [PATCH net-next v6 3/3] net: phy: broadcom: add 1000Base-X support for BCM54616S
On 09.08.2019 23:13, Tao Ren wrote: > On 8/9/19 1:54 PM, Tao Ren wrote: >> Hi Heiner, >> >> On 8/9/19 1:21 PM, Heiner Kallweit wrote: >>> On 09.08.2019 07:44, Tao Ren wrote: The BCM54616S PHY cannot work properly in RGMII->1000Base-KX mode (for example, on Facebook CMM BMC platform), mainly because genphy functions are designed for copper links, and 1000Base-X (clause 37) auto negotiation needs to be handled differently. This patch enables 1000Base-X support for BCM54616S by customizing 3 driver callbacks: - probe: probe callback detects PHY's operation mode based on INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX Control register. - config_aneg: calls genphy_c37_config_aneg when the PHY is running in 1000Base-X mode; otherwise, genphy_config_aneg will be called. - read_status: calls genphy_c37_read_status when the PHY is running in 1000Base-X mode; otherwise, genphy_read_status will be called. Signed-off-by: Tao Ren --- Changes in v6: - nothing changed. Changes in v5: - include Heiner's patch "net: phy: add support for clause 37 auto-negotiation" into the series. - use genphy_c37_config_aneg and genphy_c37_read_status in BCM54616S PHY driver's callback when the PHY is running in 1000Base-X mode. Changes in v4: - add bcm54616s_config_aneg_1000bx() to deal with auto negotiation in 1000Base-X mode. Changes in v3: - rename bcm5482_read_status to bcm54xx_read_status so the callback can be shared by BCM5482 and BCM54616S. Changes in v2: - Auto-detect PHY operation mode instead of passing DT node. - move PHY mode auto-detect logic from config_init to probe callback. - only set speed (not including duplex) in read_status callback. - update patch description with more background to avoid confusion. - patch #1 in the series ("net: phy: broadcom: set features explicitly for BCM54616") is dropped: the fix should go to get_features callback which may potentially depend on this patch. drivers/net/phy/broadcom.c | 54 +++--- include/linux/brcmphy.h| 10 +-- 2 files changed, 58 insertions(+), 6 deletions(-) diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c index 937d0059e8ac..fbd76a31c142 100644 --- a/drivers/net/phy/broadcom.c +++ b/drivers/net/phy/broadcom.c @@ -383,9 +383,9 @@ static int bcm5482_config_init(struct phy_device *phydev) /* * Select 1000BASE-X register set (primary SerDes) */ - reg = bcm_phy_read_shadow(phydev, BCM5482_SHD_MODE); - bcm_phy_write_shadow(phydev, BCM5482_SHD_MODE, - reg | BCM5482_SHD_MODE_1000BX); + reg = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE); + bcm_phy_write_shadow(phydev, BCM54XX_SHD_MODE, + reg | BCM54XX_SHD_MODE_1000BX); /* * LED1=ACTIVITYLED, LED3=LINKSPD[2] @@ -451,12 +451,44 @@ static int bcm5481_config_aneg(struct phy_device *phydev) return ret; } +static int bcm54616s_probe(struct phy_device *phydev) +{ + int val, intf_sel; + + val = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE); + if (val < 0) + return val; + + /* The PHY is strapped in RGMII to fiber mode when INTERF_SEL[1:0] + * is 01b. + */ + intf_sel = (val & BCM54XX_SHD_INTF_SEL_MASK) >> 1; + if (intf_sel == 1) { + val = bcm_phy_read_shadow(phydev, BCM54616S_SHD_100FX_CTRL); + if (val < 0) + return val; + + /* Bit 0 of the SerDes 100-FX Control register, when set + * to 1, sets the MII/RGMII -> 100BASE-FX configuration. + * When this bit is set to 0, it sets the GMII/RGMII -> + * 1000BASE-X configuration. + */ + if (!(val & BCM54616S_100FX_MODE)) + phydev->dev_flags |= PHY_BCM_FLAGS_MODE_1000BX; + } + + return 0; +} + static int bcm54616s_config_aneg(struct phy_device *phydev) { int ret; /* Aneg firsly. */ - ret = genphy_config_aneg(phydev); + if (phydev->dev_flags & PHY_BCM_FLAGS_MODE_1000BX) + ret = genphy_c37_config_aneg(phydev); + else + ret = genphy_config_aneg(phydev); >>> >>> I'm just wondering whether it needs to be considered that 100base-FX >>> doesn't support auto-negotiation. I suppose BMSR reports aneg as >>> supported, therefore phylib will use aneg per default. >>> Not sure who could set 100Base-FX
Re: [Potential Spoof] Re: [PATCH net-next v6 3/3] net: phy: broadcom: add 1000Base-X support for BCM54616S
On 8/9/19 1:54 PM, Tao Ren wrote: > Hi Heiner, > > On 8/9/19 1:21 PM, Heiner Kallweit wrote: >> On 09.08.2019 07:44, Tao Ren wrote: >>> The BCM54616S PHY cannot work properly in RGMII->1000Base-KX mode (for >>> example, on Facebook CMM BMC platform), mainly because genphy functions >>> are designed for copper links, and 1000Base-X (clause 37) auto negotiation >>> needs to be handled differently. >>> >>> This patch enables 1000Base-X support for BCM54616S by customizing 3 >>> driver callbacks: >>> >>> - probe: probe callback detects PHY's operation mode based on >>> INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX >>> Control register. >>> >>> - config_aneg: calls genphy_c37_config_aneg when the PHY is running in >>> 1000Base-X mode; otherwise, genphy_config_aneg will be called. >>> >>> - read_status: calls genphy_c37_read_status when the PHY is running in >>> 1000Base-X mode; otherwise, genphy_read_status will be called. >>> >>> Signed-off-by: Tao Ren >>> --- >>> Changes in v6: >>> - nothing changed. >>> Changes in v5: >>> - include Heiner's patch "net: phy: add support for clause 37 >>> auto-negotiation" into the series. >>> - use genphy_c37_config_aneg and genphy_c37_read_status in BCM54616S >>> PHY driver's callback when the PHY is running in 1000Base-X mode. >>> Changes in v4: >>> - add bcm54616s_config_aneg_1000bx() to deal with auto negotiation in >>> 1000Base-X mode. >>> Changes in v3: >>> - rename bcm5482_read_status to bcm54xx_read_status so the callback can >>> be shared by BCM5482 and BCM54616S. >>> Changes in v2: >>> - Auto-detect PHY operation mode instead of passing DT node. >>> - move PHY mode auto-detect logic from config_init to probe callback. >>> - only set speed (not including duplex) in read_status callback. >>> - update patch description with more background to avoid confusion. >>> - patch #1 in the series ("net: phy: broadcom: set features explicitly >>> for BCM54616") is dropped: the fix should go to get_features callback >>> which may potentially depend on this patch. >>> >>> drivers/net/phy/broadcom.c | 54 +++--- >>> include/linux/brcmphy.h| 10 +-- >>> 2 files changed, 58 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c >>> index 937d0059e8ac..fbd76a31c142 100644 >>> --- a/drivers/net/phy/broadcom.c >>> +++ b/drivers/net/phy/broadcom.c >>> @@ -383,9 +383,9 @@ static int bcm5482_config_init(struct phy_device >>> *phydev) >>> /* >>> * Select 1000BASE-X register set (primary SerDes) >>> */ >>> - reg = bcm_phy_read_shadow(phydev, BCM5482_SHD_MODE); >>> - bcm_phy_write_shadow(phydev, BCM5482_SHD_MODE, >>> -reg | BCM5482_SHD_MODE_1000BX); >>> + reg = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE); >>> + bcm_phy_write_shadow(phydev, BCM54XX_SHD_MODE, >>> +reg | BCM54XX_SHD_MODE_1000BX); >>> >>> /* >>> * LED1=ACTIVITYLED, LED3=LINKSPD[2] >>> @@ -451,12 +451,44 @@ static int bcm5481_config_aneg(struct phy_device >>> *phydev) >>> return ret; >>> } >>> >>> +static int bcm54616s_probe(struct phy_device *phydev) >>> +{ >>> + int val, intf_sel; >>> + >>> + val = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE); >>> + if (val < 0) >>> + return val; >>> + >>> + /* The PHY is strapped in RGMII to fiber mode when INTERF_SEL[1:0] >>> +* is 01b. >>> +*/ >>> + intf_sel = (val & BCM54XX_SHD_INTF_SEL_MASK) >> 1; >>> + if (intf_sel == 1) { >>> + val = bcm_phy_read_shadow(phydev, BCM54616S_SHD_100FX_CTRL); >>> + if (val < 0) >>> + return val; >>> + >>> + /* Bit 0 of the SerDes 100-FX Control register, when set >>> +* to 1, sets the MII/RGMII -> 100BASE-FX configuration. >>> +* When this bit is set to 0, it sets the GMII/RGMII -> >>> +* 1000BASE-X configuration. >>> +*/ >>> + if (!(val & BCM54616S_100FX_MODE)) >>> + phydev->dev_flags |= PHY_BCM_FLAGS_MODE_1000BX; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> static int bcm54616s_config_aneg(struct phy_device *phydev) >>> { >>> int ret; >>> >>> /* Aneg firsly. */ >>> - ret = genphy_config_aneg(phydev); >>> + if (phydev->dev_flags & PHY_BCM_FLAGS_MODE_1000BX) >>> + ret = genphy_c37_config_aneg(phydev); >>> + else >>> + ret = genphy_config_aneg(phydev); >>> >> >> I'm just wondering whether it needs to be considered that 100base-FX >> doesn't support auto-negotiation. I suppose BMSR reports aneg as >> supported, therefore phylib will use aneg per default. >> Not sure who could set 100Base-FX mode when, but maybe at that place >> also phydev->autoneg needs to be cleared. Did you test 100Base-FX mode? > >