[PATCH] net: phy: broadcom: add BCM5221 phy support

[Giulio Benetti]

Hello Marek,

thanks for reviewing,

On 12/08/23 08:19, Marek Vasut wrote:
> On 8/11/23 23:42, Giulio Benetti wrote:
>> Add BCM5221 phy support.
> 
> Why not port Linux drivers/net/sungem_phy.c instead ?
> That already supports the PHY .

That was my idea too in the beginning, but sungem_phy.c is a hidden
tristate choice in Kconfig that depends on CONFIG_PCI and is not part of 
the Linux standard drivers/net/phy/ folder.
It's only chosen by Toshiba CONFIG_SPIDER_NET and CONFIG_SUNGEM that
depends on PCI that in order depend on NET_VENDOR_SUN, so it looks like
legacy code and indeed while checking with git last commit that added
some support I've found it was back in 2011.

I've sent a patch for adding BCM5221 to Linux too:
https://lore.kernel.org/lkml/20230811215322.8679-1-giulio.benetti@benettiengineering.com/
and nobody pointed me to use sungem_phy.c

>> Sponsored by: Tekvox Inc.
>> Cc: Jim Reinhart <jimr at tekvox.com>
>> Cc: James Autry <jautry at tekvox.com>
>> Cc: Matthew Maron <matthewm at tekvox.com>
>> Signed-off-by: Giulio Benetti <giulio.benetti at benettiengineering.com>
>> ---
>>   drivers/net/phy/broadcom.c | 99 ++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 99 insertions(+)
>>
>> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
>> index 36c70da181..a1996e6059 100644
>> --- a/drivers/net/phy/broadcom.c
>> +++ b/drivers/net/phy/broadcom.c
>> @@ -34,6 +34,26 @@
>>   #define MIIM_BCM_CHANNEL_WIDTH    0x2000
>> +#define MII_BCM5221_INTREG        0x1a    /* Interrupt register */
>> +#define MII_BCM5221_IR_MASK        0x0100    /* Mask all interrupts */
>> +#define MII_BCM5221_IR_LINK_EN        0x0200    /* Link status change 
>> enable */
>> +#define MII_BCM5221_IR_SPEED_EN    0x0400    /* Link speed change 
>> enable */
>> +#define MII_BCM5221_IR_DUPLEX_EN    0x0800    /* Duplex mode change 
>> enable */
>> +#define MII_BCM5221_IR_ENABLE        0x4000    /* Interrupt enable */
>> +
>> +#define MII_BCM5221_BRCMTEST        0x1f    /* Brcm test register */
>> +#define MII_BCM5221_BT_SRE        0x0080    /* Shadow register enable */
>> +
>> +#define MII_BCM5221_AE_GSR        0x1c    /* BCM5221 Auxiliary Error &
>> +                         * General Status Register
>> +                         */
>> +#define MII_BCM5221_AE_GSR_DIS_MDIX    0x0800    /* BCM5221 Disable 
>> MDIX */
>> +#define MII_BCM5221_SHDW_AM4_FLPM    0x0002    /* BCM5221 Force Low 
>> Power
>> +                         * Mode
>> +                         */
>> +
>> +#define MII_BCM5221_SHDW_AUXMODE4    0x1a    /* Auxiliary mode 4 */
>> +
>>   static void bcm_phy_write_misc(struct phy_device *phydev,
>>                      u16 reg, u16 chl, u16 value)
>>   {
>> @@ -311,6 +331,75 @@ static int bcm5482_startup(struct phy_device 
>> *phydev)
>>       return bcm54xx_parse_status(phydev);
>>   }
>> +static int bcm_bcm5221_config(struct phy_device *phydev)
>> +{
>> +    int reg, err, err2, brcmtest;
>> +
>> +    phy_reset(phydev);
>> +
>> +    /* The datasheet indicates the PHY needs up to 1us to complete a 
>> reset,
>> +     * build some slack here.
>> +     */
>> +    udelay(2000);
> 
> 1 us and 2000 us is a huge difference , why such a long delay ?

I agree with you, only I've found in Linux drivers/net/phy/broadcom.c
usleep_range(1000, 2000) even if the comment above states 1us so for
consistency I've added the worst case 2000us.
But while writing I see that usleep_range(1000, 2000) is an additional
delay added after software reset finished, indeed in Datasheet page 33:
https://docs.broadcom.com/doc/5221-DS07-405-RDS.pdf
they state:
"
...until the reset process is completed, which requires approximately 1 µs.
"
and phy_reset() already waits for RESET bit to be cleared, so that is
really an additional delay. It's not that clear to be honest.

>> +    /* The PHY requires 65 MDC clock cycles to complete a write 
>> operation
>> +     * and turnaround the line properly.
>> +     *
>> +     * We ignore -EIO here as the MDIO controller (e.g.: 
>> mdio-bcm-unimac)
>> +     * may flag the lack of turn-around as a read failure. This is
>> +     * particularly true with this combination since the MDIO controller
>> +     * only used 64 MDC cycles. This is not a critical failure in this
>> +     * specific case and it has no functional impact otherwise, so we 
>> let
>> +     * that one go through. If there is a genuine bus error, the next 
>> read
>> +     * of MII_BCM5221_INTREG will error out.
>> +     */
> 
> Shouldn't this be fixed on the MDIO/MAC driver level?

Yes you're right, but at the moment in Linux they deal with it like
this. I don't have access to such mdio-bcm-unimac so I don't know how
to test, reproduce and fix the problem at MAC level :-/

>> +    err = phy_read(phydev, MDIO_DEVAD_NONE, MII_BMCR);
>> +    if (err < 0 && err != -EIO)
>> +        return err;
>> +
>> +    reg = phy_read(phydev, MDIO_DEVAD_NONE, MII_BCM5221_INTREG);
>> +    if (reg < 0)
>> +        return reg;
>> +
>> +    /* Mask interrupts globally since we don't use interrupt */
>> +    reg = MII_BCM5221_IR_MASK;
>> +
>> +    err = phy_write(phydev, MDIO_DEVAD_NONE, MII_BCM5221_INTREG, reg);
>> +    if (err < 0)
>> +        return err;
>> +
>> +    /* Enable auto MDIX */
>> +    err = phy_modify(phydev, MDIO_DEVAD_NONE, MII_BCM5221_AE_GSR,
>> +             MII_BCM5221_AE_GSR_DIS_MDIX, 0);
>> +    if (err < 0)
>> +        return err;
>> +
>> +    /* Enable shadow register access */
>> +    brcmtest = phy_read(phydev, MDIO_DEVAD_NONE, MII_BCM5221_BRCMTEST);
>> +    if (brcmtest < 0)
>> +        return brcmtest;
>> +
>> +    reg = brcmtest | MII_BCM5221_BT_SRE;
> 
> I think it would be best to port phy_set_bits() wrapper from Linux and 
> use it here. The mmd one is already ported over.

Ok, I will add it on V2, at this point I will add phy_clear_bits() too
for enabling MDIX above. I don't know why I haven't used it neither on
Linux driver. I think I will modify it according to this then, because
I need to preserve all bits of the register and set bit 7 but it is
exactly what phy_set_bits() does.

> 
>> +    err = phy_write(phydev, MDIO_DEVAD_NONE, MII_BCM5221_BRCMTEST, reg);
>> +    if (err < 0)
>> +        return err;
>> +
>> +    /* Exit low power mode */
>> +    err = phy_modify(phydev, MDIO_DEVAD_NONE, MII_BCM5221_SHDW_AUXMODE4,
>> +             MII_BCM5221_SHDW_AM4_FLPM, 0);
>> +    if (err < 0)
>> +        goto done;

Below maybe you mean I have to return instead of 'goto done', right?
Also because it makes no sense since it will get to done: in any case.

>> +
>> +done:
>> +    /* Disable shadow register access */
>> +    err2 = phy_write(phydev, MDIO_DEVAD_NONE, MII_BCM5221_BRCMTEST, 
>> brcmtest);
>> +    if (!err)
>> +        err = err2;
> 
> Just ignore the return value of this write here, you want to return the 
> original error value anyway, and if the write here fails, it means the 
> hardware just failed badly.

And so yes, I can get rid of err2 and use err again and simply return it.

Does all sound good to you?

Thanks again
Best regards
-- 
Giulio Benetti
CEO&CTO at Benetti Engineering sas