On 7/30/19 3:15 AM, Vladimir Oltean wrote: > On Tue, 30 Jul 2019 at 07:52, Tao Ren <tao...@fb.com> wrote: >> >> On 7/29/19 6:32 PM, Vladimir Oltean wrote: >>> Hi Tao, >>> >>> On Tue, 30 Jul 2019 at 03:31, Tao Ren <tao...@fb.com> wrote: >>>> >>>> Configure the BCM54616S for 1000Base-X mode when "brcm-phy-mode-1000bx" >>>> is set in device tree. This is needed when the PHY is used for fiber and >>>> backplane connections. >>>> >>>> The patch is inspired by commit cd9af3dac6d1 ("PHYLIB: Add 1000Base-X >>>> support for Broadcom bcm5482"). >>> >>> As far as I can see, for the commit you referenced, >>> PHY_BCM_FLAGS_MODE_1000BX is referenced from nowhere in the entire >>> mainline kernel: >>> https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_linux_latest_ident_PHY-5FBCM-5FFLAGS-5FMODE-5F1000BX&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=iYElT7HC77pRZ3byVvW8ng&m=gy6Y-3Ylme-_GQcGF4fvOX10irgAT4xh253Weo0np38&s=KL__E2bvsmvUL-hBL9hUmOS5vyPQ92EMj6fEfByn8t8&e= >>> (it is supposed to be put by the MAC driver in phydev->dev_flags prior >>> to calling phy_connect). But I don't see the point to this - can't you >>> check for phydev->interface == PHY_INTERFACE_MODE_1000BASEX? >>> This has the advantage that no MAC driver will need to know that it's >>> talking to a Broadcom PHY. Additionally, no custom DT bindings are >>> needed. >>> Also, for backplane connections you probably want 1000Base-KX which >>> has its own AN/LT, not plain 1000Base-X. >> >> Thank you Vladimir for the quick review! >> Perhaps I misunderstood the purpose of phydev->interface, and I thought it >> was usually used to defined the interface between MAC and PHY. For example, >> if I need to pass both "rgmii-id" and "1000base-x" from MAC to PHY driver, >> what would be the preferred way? >> > > Ohhhhhh, now I understand what you're trying to do, sorry, somehow I > was too tired and I thought of something totally unrelated.
Thank you for spending time on the patch, and I really appreciate it. > Let me see if I can explain: you've got the INTF_SEL pin strapping > configured for something else (like RGMII to copper mode) and then > you're changing the operating mode at runtime through MDIO? Is this > intended to be for production code, or is it just some quick hack to > fix a bad board design? The INTF_SEL pins report correct mode (RGMII-Fiber) on my machine, but there are 2 "sub-modes" (1000Base-X and 100Base-FX) and I couldn't find a proper/safe way to auto-detect which "sub-mode" is active. The datasheet just describes instructions to enable a specific mode, but it doesn't say 1000Base-X/100Base-FX mode will be auto-selected. And that's why I came up with the patch to specify 1000Base-X mode. > I think what's supposed to happen (Heiner can comment) is that > genphy_config_init will automatically read the out-of-reset PHY > registers and figure out which link modes are supported. This includes > the 1000Base-X media type, *if* the PHY is strapped correctly. > But you are changing the strapping configuration too late (again, in > .config_init), so phylib doesn't pick up the new Base-X modes. What > happens if you do the switchover from the .probe callback of the > driver, instead of .config_init? I checked the 1000base-x mode control bit and it's already set on my machine when genphy_config_init is executed, means I'm writing the same value to the register. I write the register explicitly because I'm suspecting the mode control bit is set by my boot loader and the value is not changed by software reset. Let me see if I can disable net/phy in boot loader and see what happens. If the bit is still on, maybe we can use the bit to auto-detect sub-mode (although the datasheet doesn't mention it)? Anyways, let me move the logic to .probe callback. Thank you. > I think what got me confused was your "add support for 1000Base-X" > commit message. If I understand correctly, you're not adding support, > you're just forcing it. > Again, I don't think Linux has generic support for overwriting (or > even describing) the operating mode of a PHY, although maybe that's a > direction we would want to push the discussion towards. RGMII to > copper, RGMII to fiber, SGMII to copper, copper to fiber (media > converter), even RGMII to SGMII (RTL8211FS supports this) - lots of > modes, and this is only for gigabit PHYs... > >>>> Signed-off-by: Tao Ren <tao...@fb.com> >>>> --- >>>> drivers/net/phy/broadcom.c | 58 +++++++++++++++++++++++++++++++++++--- >>>> include/linux/brcmphy.h | 4 +-- >>>> 2 files changed, 56 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c >>>> index 2b4e41a9d35a..6c22ac3a844b 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,6 +451,34 @@ static int bcm5481_config_aneg(struct phy_device >>>> *phydev) >>>> return ret; >>>> } >>>> >>>> +static int bcm54616s_config_init(struct phy_device *phydev) >>>> +{ >>>> + int err, reg; >>>> + struct device_node *np = phydev->mdio.dev.of_node; >>>> + >>>> + err = bcm54xx_config_init(phydev); >>>> + >>>> + if (of_property_read_bool(np, "brcm-phy-mode-1000bx")) { >>>> + /* Select 1000BASE-X register set. */ >>>> + reg = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE); >>>> + bcm_phy_write_shadow(phydev, BCM54XX_SHD_MODE, >>>> + reg | BCM54XX_SHD_MODE_1000BX); >>>> + >>>> + /* Auto-negotiation doesn't seem to work quite right >>>> + * in this mode, so we disable it and force it to the >>>> + * right speed/duplex setting. Only 'link status' >>>> + * is important. >>>> + */ >>>> + phydev->autoneg = AUTONEG_DISABLE; >>>> + phydev->speed = SPEED_1000; >>>> + phydev->duplex = DUPLEX_FULL; >>>> + >>> >>> 1000Base-X AN does not include speed negotiation, so hardcoding >>> SPEED_1000 is probably correct. >>> What is wrong with the AN of duplex settings? >> >> FULL_DUPLEX bit is set on my platform by default. Let me enable AN and test >> it out; will share you results tomorrow. Duplex setting is correct when AN is enabled. So I will just hardcode speed settings. >>>> + phydev->dev_flags |= PHY_BCM_FLAGS_MODE_1000BX; >>>> + } >>>> + >>>> + return err; >>>> +} >>>> + >>>> static int bcm54616s_config_aneg(struct phy_device *phydev) >>>> { >>>> int ret; >>>> @@ -464,6 +492,27 @@ static int bcm54616s_config_aneg(struct phy_device >>>> *phydev) >>>> return ret; >>>> } >>>> >>>> +static int bcm54616s_read_status(struct phy_device *phydev) >>>> +{ >>>> + int ret; >>>> + >>>> + ret = genphy_read_status(phydev); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + if (phydev->dev_flags & PHY_BCM_FLAGS_MODE_1000BX) { >>>> + /* Only link status matters for 1000Base-X mode, so force >>>> + * 1000 Mbit/s full-duplex status. >>>> + */ >>>> + if (phydev->link) { >>>> + phydev->speed = SPEED_1000; >>>> + phydev->duplex = DUPLEX_FULL; >>>> + } >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> static int brcm_phy_setbits(struct phy_device *phydev, int reg, int set) >>>> { >>>> int val; >>>> @@ -651,8 +700,9 @@ static struct phy_driver broadcom_drivers[] = { >>>> .phy_id_mask = 0xfffffff0, >>>> .name = "Broadcom BCM54616S", >>>> .features = PHY_GBIT_FEATURES, >>>> - .config_init = bcm54xx_config_init, >>>> + .config_init = bcm54616s_config_init, >>>> .config_aneg = bcm54616s_config_aneg, >>>> + .read_status = bcm54616s_read_status, >>>> .ack_interrupt = bcm_phy_ack_intr, >>>> .config_intr = bcm_phy_config_intr, >>>> }, { >>>> diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h >>>> index 6db2d9a6e503..82030155558c 100644 >>>> --- a/include/linux/brcmphy.h >>>> +++ b/include/linux/brcmphy.h >>>> @@ -200,8 +200,8 @@ >>>> #define BCM5482_SHD_SSD 0x14 /* 10100: Secondary SerDes >>>> control */ >>>> #define BCM5482_SHD_SSD_LEDM 0x0008 /* SSD LED Mode enable */ >>>> #define BCM5482_SHD_SSD_EN 0x0001 /* SSD enable */ >>>> -#define BCM5482_SHD_MODE 0x1f /* 11111: Mode Control Register */ >>>> -#define BCM5482_SHD_MODE_1000BX 0x0001 /* Enable 1000BASE-X >>>> registers */ >>>> +#define BCM54XX_SHD_MODE 0x1f /* 11111: Mode Control Register */ >>>> +#define BCM54XX_SHD_MODE_1000BX 0x0001 /* Enable 1000BASE-X >>>> registers */ >>> >>> These registers are also present on my BCM5464, probably safe to >>> assume they're generic for the entire family. >>> So if you make the registers definitions common, you can probably make >>> the 1000Base-X configuration common as well. >> >> If I understand correctly, your recommendation is to add a common function >> (such as "bcm54xx_config_1000bx") so it can be used by other BCM chips? >> Sure, I will take care of it. >> >> >> Thanks, >> >> Tao > > Regards, > -Vladimir >