lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <05fbb2b3-17cb-3d4d-5787-50825c090451@benettiengineering.com> Date: Sat, 12 Aug 2023 19:12:19 +0200 From: Giulio Benetti <giulio.benetti@...ettiengineering.com> To: Andrew Lunn <andrew@...n.ch> Cc: Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>, Heiner Kallweit <hkallweit1@...il.com>, "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Florian Fainelli <florian.fainelli@...adcom.com>, Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, Jim Reinhart <jimr@...vox.com>, James Autry <jautry@...vox.com>, Matthew Maron <matthewm@...vox.com> Subject: Re: [PATCH] net: phy: broadcom: add support for BCM5221 phy Hello Andrew, thank you for reviewing, On 12/08/23 00: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 ? I've picked this tag from commit: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=73c289bac05919286f8c7e1660fcaf6ec0468363 it's not "Sponsored-by:", do you think it's better using this last form? I can send a patch along with V2 of this one for that. >> +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. oh ok, I'll do that on V2 > >> + /* 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. ok, > >> @@ -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. right, this and even the struct phy_driver driver entry too then, > >> #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, you can check it in the pdf page 37: https://docs.broadcom.com/doc/5221-DS07-405-RDS.pdf and yes, it's odd. Thanks again for the review, I'll do the changes here and the others suggested by the other and send V2 patch. Best regards -- Giulio Benetti CEO&CTO@...etti Engineering sas
Powered by blists - more mailing lists