lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 30 Jul 2019 04:52:37 +0000 From: Tao Ren <taoren@...com> To: Vladimir Oltean <olteanv@...il.com> CC: Andrew Lunn <andrew@...n.ch>, Florian Fainelli <f.fainelli@...il.com>, Heiner Kallweit <hkallweit1@...il.com>, "David S . Miller" <davem@...emloft.net>, Arun Parameswaran <arun.parameswaran@...adcom.com>, Justin Chen <justinpopo6@...il.com>, netdev <netdev@...r.kernel.org>, lkml <linux-kernel@...r.kernel.org>, Andrew Jeffery <andrew@...id.au>, "openbmc@...ts.ozlabs.org" <openbmc@...ts.ozlabs.org> Subject: Re: [PATCH net-next 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S On 7/29/19 6:32 PM, Vladimir Oltean wrote: > Hi Tao, > > On Tue, 30 Jul 2019 at 03:31, Tao Ren <taoren@...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? >> Signed-off-by: Tao Ren <taoren@...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. >> + 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
Powered by blists - more mailing lists