| 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: <e8f85ef3-1216-8efb-a54d-84426234fe82@fb.com>
Date: Tue, 30 Jul 2019 23:44:14 +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/30/19 3:15 AM, Vladimir Oltean wrote:
> On Tue, 30 Jul 2019 at 07:52, Tao Ren <taoren@...com> wrote:
>>
>> 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?
>>
>
> 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 <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.
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
>
Powered by blists - more mailing lists