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@benettieng... and nobody pointed me to use sungem_phy.c
Sponsored by: Tekvox Inc. Cc: Jim Reinhart jimr@tekvox.com Cc: James Autry jautry@tekvox.com Cc: Matthew Maron matthewm@tekvox.com Signed-off-by: Giulio Benetti giulio.benetti@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