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.bene...@benettiengineering.com/
and nobody pointed me to use sungem_phy.c
Sponsored by: Tekvox Inc.
Cc: Jim Reinhart <j...@tekvox.com>
Cc: James Autry <jau...@tekvox.com>
Cc: Matthew Maron <matth...@tekvox.com>
Signed-off-by: Giulio Benetti <giulio.bene...@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@Benetti Engineering sas
