[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <67e5357e.df0a0220.14c972.64ad@mx.google.com>
Date: Thu, 27 Mar 2025 12:24:44 +0100
From: Christian Marangi <ansuelsmth@...il.com>
To: Maxime Chevallier <maxime.chevallier@...tlin.com>
Cc: Andrew Lunn <andrew+netdev@...n.ch>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Heiner Kallweit <hkallweit1@...il.com>,
Russell King <linux@...linux.org.uk>,
Florian Fainelli <florian.fainelli@...adcom.com>,
Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>,
Marek BehĂșn <kabel@...nel.org>,
Eric Woudstra <ericwouds@...il.com>,
Daniel Golle <daniel@...rotopia.org>, netdev@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [net-next RFC PATCH v3 3/4] net: phy: Add support for Aeonsemi
AS21xxx PHYs
On Thu, Mar 27, 2025 at 09:24:18AM +0100, Maxime Chevallier wrote:
> Hi Christian,
>
> On Thu, 27 Mar 2025 00:35:03 +0100
> Christian Marangi <ansuelsmth@...il.com> wrote:
>
> > Add support for Aeonsemi AS21xxx 10G C45 PHYs. These PHYs integrate
> > an IPC to setup some configuration and require special handling to
> > sync with the parity bit. The parity bit is a way the IPC use to
> > follow correct order of command sent.
> >
> > Supported PHYs AS21011JB1, AS21011PB1, AS21010JB1, AS21010PB1,
> > AS21511JB1, AS21511PB1, AS21510JB1, AS21510PB1, AS21210JB1,
> > AS21210PB1 that all register with the PHY ID 0x7500 0x7510
> > before the firmware is loaded.
> >
> > They all support up to 5 LEDs with various HW mode supported.
> >
> > While implementing it was found some strange coincidence with using the
> > same logic for implementing C22 in MMD regs in Broadcom PHYs.
> >
> > For reference here the AS21xxx PHY name logic:
> >
> > AS21x1xxB1
> > ^ ^^
> > | |J: Supports SyncE/PTP
> > | |P: No SyncE/PTP support
> > | 1: Supports 2nd Serdes
> > | 2: Not 2nd Serdes support
> > 0: 10G, 5G, 2.5G
> > 5: 5G, 2.5G
> > 2: 2.5G
> >
> > Signed-off-by: Christian Marangi <ansuelsmth@...il.com>
>
> [...]
>
> I know this is only RFC, but I have some questions
>
> > +static int as21xxx_match_phy_device(struct phy_device *phydev,
> > + const struct phy_driver *phydrv)
> > +{
> > + struct as21xxx_priv *priv;
> > + u32 phy_id;
> > + int ret;
> > +
> > + /* Skip PHY that are not AS21xxx or already have firmware loaded */
> > + if (phydev->c45_ids.device_ids[MDIO_MMD_PCS] != PHY_ID_AS21XXX)
> > + return phydev->phy_id == phydrv->phy_id;
> > +
> > + /* Read PHY ID to handle firmware just loaded */
> > + ret = phy_read_mmd(phydev, MDIO_MMD_PCS, MII_PHYSID1);
> > + if (ret < 0)
> > + return ret;
> > + phy_id = ret << 16;
> > +
> > + ret = phy_read_mmd(phydev, MDIO_MMD_PCS, MII_PHYSID2);
> > + if (ret < 0)
> > + return ret;
> > + phy_id |= ret;
> > +
> > + /* With PHY ID not the generic AS21xxx one assume
> > + * the firmware just loaded
> > + */
> > + if (phy_id != PHY_ID_AS21XXX)
> > + return phy_id == phydrv->phy_id;
> > +
> > + /* Allocate temp priv and load the firmware */
> > + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + mutex_init(&priv->ipc_lock);
> > +
> > + ret = aeon_firmware_load(phydev);
> > + if (ret)
> > + return ret;
>
> Here, and below, you leak priv by returning early.
>
Yes copy paste error with migrating from devm to non-devmem.
> > +
> > + ret = aeon_ipc_sync_parity(phydev, priv);
> > + if (ret)
> > + return ret;
> > +
> > + /* Enable PTP clk if not already Enabled */
> > + ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_PTP_CLK,
> > + VEND1_PTP_CLK_EN);
> > + if (ret)
> > + return ret;
> > +
> > + ret = aeon_dpc_ra_enable(phydev, priv);
> > + if (ret)
> > + return ret;
>
> Does all of the above with sync_parity, PTP clock cfg and so on needs
> to be done in this first pass of the matching process for this PHY ?
>
> From what I got from the discussions, the only important bit is to load
> the FW to get the correct PHY id ?
>
I will do further check on this. Waiting for the sync parity have the
side effect of waiting for the firmware to finish loading.
> > + mutex_destroy(&priv->ipc_lock);
> > + kfree(priv);
> > +
> > + /* Return not maching anyway as PHY ID will change after
> > + * firmware is loaded.
> > + */
> > + return 0;
> > +}
>
> Maxime
--
Ansuel
Powered by blists - more mailing lists