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

[Daniel]

Ok, patch v2 sent, only with the interrupt bug fix, and without
renaming all registers.

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

Yes, please make the upstream work.

Regards

2016-12-18 12:54 GMT+01:00 Jonas Gorski <jonas.gorski at gmail.com>:
> 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.
>
>
>
>
> Regards
> Jonas



-- 
Daniel