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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 11 Aug 2023 15:39:48 -0700
From: Florian Fainelli <florian.fainelli@...adcom.com>
To: Andrew Lunn <andrew@...n.ch>,
 Giulio Benetti <giulio.benetti@...ettiengineering.com>
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>,
 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

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


Download attachment "smime.p7s" of type "application/pkcs7-signature" (4221 bytes)