| 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
| ||
|
Message-ID: <3987251b-9679-dfbe-6e15-f991c2893bac@fb.com>
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