[LEDE-DEV] [PATCH] brcm63xx: fix internal phy registers

[Jonas Gorski]

Hi,

On 18 December 2016 at 02:19, Daniel Gonzalez Cabanelas
<dgcbueu at gmail.com> wrote:
> The internal phy is using wrong registers for the config interrupt function,
> causing incorrect behavior when detecting the link activity. Fix it.
>
> We cannot use the bcm_phy_config_intr function from the bcm-phy-lib.c
> because it uses different registers from brcm63xx. We need to use
> our own function, which matches with the one used by the
> "Broadcom PHY driver" (brcm_fet_config_intr at broadcom.c).
>
> brcm63xx internal phy uses the same registers as the ones defined in
> brcmphy.h for fast ethernet, use them instead.
>
> Signed-off-by: Daniel Gonzalez Cabanelas <dgcbueu at gmail.com>

Good find.

> diff --git a/target/linux/brcm63xx/patches-4.4/409-bcm63xx_net_phy-fix-registers.patch b/target/linux/brcm63xx/patches-4.4/409-bcm63xx_net_phy-fix-registers.patch
> new file mode 100644
> index 0000000..c53d464
> --- /dev/null
> +++ b/target/linux/brcm63xx/patches-4.4/409-bcm63xx_net_phy-fix-registers.patch
> @@ -0,0 +1,102 @@
> +--- a/drivers/net/phy/bcm63xx.c
> ++++ b/drivers/net/phy/bcm63xx.c
> +@@ -6,44 +6,55 @@
> +  *    as published by the Free Software Foundation; either version
> +  *    2 of the License, or (at your option) any later version.
> +  */
> + #include "bcm-phy-lib.h"
> + #include <linux/module.h>
> + #include <linux/phy.h>
> +-
> +-#define MII_BCM63XX_IR                0x1a    /* interrupt register */
> +-#define MII_BCM63XX_IR_EN     0x4000  /* global interrupt enable */
> +-#define MII_BCM63XX_IR_DUPLEX 0x0800  /* duplex changed */
> +-#define MII_BCM63XX_IR_SPEED  0x0400  /* speed changed */
> +-#define MII_BCM63XX_IR_LINK   0x0200  /* link changed */
> +-#define MII_BCM63XX_IR_GMASK  0x0100  /* global interrupt mask */
> ++#include <linux/brcmphy.h>
> +
> + MODULE_DESCRIPTION("Broadcom 63xx internal PHY driver");
> + MODULE_AUTHOR("Maxime Bizon <mbizon at freebox.fr>");
> + MODULE_LICENSE("GPL");
> +
> + static int bcm63xx_config_init(struct phy_device *phydev)
> + {
> +       int reg, err;
> +
> +-      reg = phy_read(phydev, MII_BCM63XX_IR);
> ++      reg = phy_read(phydev, MII_BRCM_FET_INTREG);
> +       if (reg < 0)
> +               return reg;
> +
> +       /* Mask interrupts globally.  */
> +-      reg |= MII_BCM63XX_IR_GMASK;
> +-      err = phy_write(phydev, MII_BCM63XX_IR, reg);
> ++      reg |= MII_BRCM_FET_IR_MASK;
> ++      err = phy_write(phydev, MII_BRCM_FET_INTREG, reg);
> +       if (err < 0)
> +               return err;
> +
> +       /* Unmask events we are interested in  */
> +-      reg = ~(MII_BCM63XX_IR_DUPLEX |
> +-              MII_BCM63XX_IR_SPEED |
> +-              MII_BCM63XX_IR_LINK) |
> +-              MII_BCM63XX_IR_EN;
> +-      return phy_write(phydev, MII_BCM63XX_IR, reg);
> ++      reg = ~(MII_BRCM_FET_IR_DUPLEX_EN |
> ++              MII_BRCM_FET_IR_SPEED_EN |
> ++              MII_BRCM_FET_IR_LINK_EN) |
> ++              MII_BRCM_FET_IR_ENABLE;
> ++      return phy_write(phydev, MII_BRCM_FET_INTREG, reg);
> ++}

This whole change has nothing to do with the issue and is just
cosmetic, please drop it. It makes finding the actual changes hard to
follow. The relevant part is:

> ++
> ++static int brcm_fet_config_intr(struct phy_device *phydev)

Please call it bcm63xx_config_intr to match the other functions in this driver.

> ++{
> ++      int reg, err;
> ++
> ++      reg = phy_read(phydev, MII_BRCM_FET_INTREG);
> ++      if (reg < 0)
> ++              return reg;
> ++
> ++      if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
> ++              reg &= ~MII_BRCM_FET_IR_MASK;
> ++      else
> ++              reg |= MII_BRCM_FET_IR_MASK;
> ++
> ++      err = phy_write(phydev, MII_BRCM_FET_INTREG, reg);
> ++      return err;

You can just do "return phy_write()" here, and then you can drop the
err variable.

*looks at the breaking commit*

Ah, I see what you did there, that's basically the old function. So it
can probably stay.


Also this change should go upstream. If you don't want to handle it, I
can do as well.


Regards
Jonas