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

From: Florian Fainelli
Date: Fri Aug 11 2023 - 18:40:05 EST


On 8/11/23 15:16, Andrew Lunn wrote:
On Fri, Aug 11, 2023 at 11:53:22PM +0200, Giulio Benetti wrote:
This patch adds the BCM5221 PHY support by reusing
brcm_fet_config_intr() and brcm_fet_handle_interrupt() and
implementing config_init()/suspend()/resume().

Sponsored by: Tekvox Inc.

That is a new tag. Maybe you should update
Documentation/process/submitting-patches.rst ?

+static int bcm5221_config_init(struct phy_device *phydev)
+{
+ int reg, err, err2, brcmtest;
+
+ /* Reset the PHY to bring it to a known state. */
+ err = phy_write(phydev, MII_BMCR, BMCR_RESET);
+ if (err < 0)
+ return err;
+
+ /* The datasheet indicates the PHY needs up to 1us to complete a reset,
+ * build some slack here.
+ */
+ usleep_range(1000, 2000);
+
+ /* 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_BRCM_FET_INTREG will error out.
+ */
+ err = phy_read(phydev, MII_BMCR);
+ if (err < 0 && err != -EIO)
+ return err;

It is pretty normal to check the value of MII_BMCR and ensure that
BMCR_RESET has cleared. See phy_poll_reset(). It might not be needed,
if you trust the datasheet, but 802.3 C22 says it should clear.

+ /* Enable auto MDIX */
+ err = phy_clear_bits(phydev, BCM5221_AEGSR, BCM5221_AEGSR_MDIX_DIS);
+ if (err < 0)
+ return err;

It is better to set it based on phydev->mdix_ctrl.

@@ -1288,6 +1431,7 @@ static struct mdio_device_id __maybe_unused broadcom_tbl[] = {
{ PHY_ID_BCM53125, 0xfffffff0 },
{ PHY_ID_BCM53128, 0xfffffff0 },
{ PHY_ID_BCM89610, 0xfffffff0 },
+ { PHY_ID_BCM5221, 0xfffffff0 },

This table has some sort of sorting. I would put this new entry before
PHY_ID_BCM5241.

#define PHY_ID_BCM50610 0x0143bd60
#define PHY_ID_BCM50610M 0x0143bd70
#define PHY_ID_BCM5241 0x0143bc30
+#define PHY_ID_BCM5221 0x004061e0

The value looks odd. Is the OUI correct? Is that a broadcom OUI?

Yes it is correct and yes it is assigned to Broadcom.
--
Florian

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature