Re: [PATCH net-next 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

Re: [PATCH net-next 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Vladimir Oltean]

Hi Tao,

On Tue, 30 Jul 2019 at 03:31, Tao Ren <taoren@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://elixir.bootlin.com/linux/latest/ident/PHY_BCM_FLAGS_MODE_1000BX
(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.

>
> Signed-off-by: Tao Ren <taoren@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?

> +               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.

>
>
>  /*
> --
> 2.17.1
>

Regards,
-Vladimir

Re: [PATCH net-next 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

On 7/29/19 6:32 PM, Vladimir Oltean wrote:
> Hi Tao,
> 
> On Tue, 30 Jul 2019 at 03:31, Tao Ren <taoren@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?

>> Signed-off-by: Tao Ren <taoren@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.

>> +               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

Re: [PATCH net-next 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Vladimir Oltean]

On Tue, 30 Jul 2019 at 07:52, Tao Ren <taoren@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 <taoren@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.
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?
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 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@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.
>
> >> +               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

Re: [PATCH net-next 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Andrew Lunn]

> 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...

This is something Russell King has PHYLINK patches for, which have not
yet been merged. There are some boards which use a PHY as a media
converter, placed between the MAC and an SFP.

	   Andrew

Re: [PATCH net-next 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

On 7/30/19 3:15 AM, Vladimir Oltean wrote:
> On Tue, 30 Jul 2019 at 07:52, Tao Ren <taoren@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 <taoren@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 <taoren@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
> 

Re: [PATCH net-next 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

On 7/30/19 6:17 AM, Andrew Lunn wrote:
>> 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...
> 
> This is something Russell King has PHYLINK patches for, which have not
> yet been merged. There are some boards which use a PHY as a media
> converter, placed between the MAC and an SFP.

Thank you Andrew. Let me see if the operating mode can be auto-detected on BCM54616S chip, and I will update back soon.

Thanks,

Tao

Re: [PATCH net-next 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Andrew Lunn]

> 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.

Fibre does not perform any sort of auto-negotiation. I assume you have
an SFP connected? When using PHYLINK, the sfp driver will get the
supported baud rate from SFP EEPROM to determine what speed could be
used. However, there is currently no mainline support for having a
chain MAC-PHY-SFP. For that you need Russells out of tree patches.

      Andrew

Re: [PATCH net-next 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

On 7/30/19 6:36 PM, Andrew Lunn wrote:
>> 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.
> 
> Fibre does not perform any sort of auto-negotiation. I assume you have
> an SFP connected? When using PHYLINK, the sfp driver will get the
> supported baud rate from SFP EEPROM to determine what speed could be
> used. However, there is currently no mainline support for having a
> chain MAC-PHY-SFP. For that you need Russells out of tree patches.

Hi Andrew,

The BCM54616S PHY on my machine is connected to a BCM5396 switch chip over backplane (1000Base-KX).


Thanks,

Tao

Re: [PATCH net-next 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Andrew Lunn]

> Hi Andrew,
> 
> The BCM54616S PHY on my machine is connected to a BCM5396 switch chip over backplane (1000Base-KX).

Ah, that is different. So the board is using it for RGMII to 1000Base-KX?

phy-mode is about the MAC-PHY link. So in this case RGMII.

There is no DT way to configure the PHY-Switch link. However, it
sounds like you have the PHY strapped so it is doing 1000BaseX on the
PHY-Switch link. So do you actually need to configure this?

You report you never see link up? So maybe the problem is actually in
read_status? When in 1000BaseX mode, do you need to read the link
status from some other register? The Marvell PHYs use a second page
for 1000BaseX.

    Andrew

Re: [PATCH net-next 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

On 7/30/19 7:34 PM, Andrew Lunn wrote:
>> Hi Andrew,
>>
>> The BCM54616S PHY on my machine is connected to a BCM5396 switch chip over backplane (1000Base-KX).
> 
> Ah, that is different. So the board is using it for RGMII to 1000Base-KX?
> 
> phy-mode is about the MAC-PHY link. So in this case RGMII.

Yes. It's RGMII to 1000Base-KX.

> There is no DT way to configure the PHY-Switch link. However, it
> sounds like you have the PHY strapped so it is doing 1000BaseX on the
> PHY-Switch link. So do you actually need to configure this?

The PHY is strapped in RGMII-Fiber Mode (the term used in datasheet), but besides 1000BaseX, 100Base-FX is also supported in this mode.
The datasheet doesn't say which link type (1000BaseX or 100Base-FX) is active after reset and I cannot find a way to auto-detect the link type, either.

Below are a few more details about 1000Base-X and 100Base-FX in BCM54616S datasheet.

- 1000BaseX: 
  The 1000BaseX register set (MII registers 00-0F) needs to be enabled by setting bit 0 of Mode Control Register.
  Although the register address is the same between 1000BaseX and 1000BaseT/100BaseT/10BaseT registers, some bit fields in 1000BaseX registers are different: for example, speed field is not available in 1000BaseX status register.

- 100Base-FX:
  100Base-FX registers need to be enabled by writing 1 to SerDes 100FX Control Register.
  100Base-FX Control and Status registers are in shadow register 1Ch, instead of MII register 00h and 01h.

> You report you never see link up? So maybe the problem is actually in
> read_status? When in 1000BaseX mode, do you need to read the link
> status from some other register? The Marvell PHYs use a second page
> for 1000BaseX.

read_status callback needs to be updated to report correct link speed in my case. But as I cannot tell which link type (1000BaseX or 100Base-FX) is active, it becomes hard to access registers in read_status method. Any suggestions?

BTW, link-never-up issue seems to be caused by static/dynamic feature detection. I'm still tracing down the issue and it's being tracked in patch #1 of the series.


Thanks,

Tao

Re: [PATCH net-next 2/2] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Tao Ren]

On 7/30/19 10:55 PM, Tao Ren wrote:
> On 7/30/19 7:34 PM, Andrew Lunn wrote:
>>> Hi Andrew,
>>>
>>> The BCM54616S PHY on my machine is connected to a BCM5396 switch chip over backplane (1000Base-KX).
>>
>> Ah, that is different. So the board is using it for RGMII to 1000Base-KX?
>>
>> phy-mode is about the MAC-PHY link. So in this case RGMII.
> 
> Yes. It's RGMII to 1000Base-KX.
> 
>> There is no DT way to configure the PHY-Switch link. However, it
>> sounds like you have the PHY strapped so it is doing 1000BaseX on the
>> PHY-Switch link. So do you actually need to configure this?
> 
> The PHY is strapped in RGMII-Fiber Mode (the term used in datasheet), but besides 1000BaseX, 100Base-FX is also supported in this mode.
> The datasheet doesn't say which link type (1000BaseX or 100Base-FX) is active after reset and I cannot find a way to auto-detect the link type, either.

I found bit 0 of 100-FX control register can be used to detect PHY-switch link type (means DT is not needed). Will run more testing and send out v2 patch soon. Thank you all for the input and help.


Cheers,

Tao