[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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