RE: [PATCH net-next V3 3/4] net: lan743x: Migrate phylib to phylink
From: Ronnie.Kunin
Date: Thu Aug 01 2024 - 19:47:36 EST
Hi Russell,
Raju can comment more on it when he comes online tomorrow (IST time), but I recall this was discussed with you last year and my understanding is that the outcome was that as long as the need to use the dynamic fallback from phy to fixed_phy mode is explained in the commit message - which Raju did in the commit description - , it was acceptable to do this in phylink. Unless the "mechanism where a MAC driver can tell phylink to switch to using a fixed-link with certain parameters" has been implemented since then (apologize if it has, I am not a linux expert by any means, but don't seem to see it), I would guess the reasons for doing this are still valid.
Attached is the email thread with that discussion and the relevant comments are copied below.
> The reason this should work is because the fixed-phy support does emulate a real PHY in software, and phylink will treat that exactly the same way as a real PHY (because when in phylink is in PHY mode, it delegates PHY management to phylib.)
>
>Using fixed-phy with phylink will certainly raise some eyebrows, so the reasons for it will need to be set out in the commit message - and as you've dropped Andrew from this thread, I suspect he will still raise some comments about it.
>
>In the longer term, it would probably make sense for phylink to provide a mechanism where a MAC driver can tell phylink to switch to using a fixed-link with certain parameters.
Best regards,
Ronnie
-----Original Message-----
From: Russell King <linux@xxxxxxxxxxxxxxx>
Sent: Thursday, August 1, 2024 12:27 PM
To: Raju Lakkaraju - I30499 <Raju.Lakkaraju@xxxxxxxxxxxxx>
Cc: netdev@xxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx; kuba@xxxxxxxxxx; andrew@xxxxxxx; horms@xxxxxxxxxx; hkallweit1@xxxxxxxxx; richardcochran@xxxxxxxxx; rdunlap@xxxxxxxxxxxxx; Bryan Whitehead - C21958 <Bryan.Whitehead@xxxxxxxxxxxxx>; edumazet@xxxxxxxxxx; pabeni@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; UNGLinuxDriver <UNGLinuxDriver@xxxxxxxxxxxxx>
Subject: Re: [PATCH net-next V3 3/4] net: lan743x: Migrate phylib to phylink
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
On Thu, Aug 01, 2024 at 04:33:13PM +0530, Raju Lakkaraju wrote:
> > > + if (!dn || (ret && !lan743x_phy_handle_exists(dn))) {
> > > + phydev = phy_find_first(adapter->mdiobus);
> > > + if (!phydev) {
> > > + if (((adapter->csr.id_rev & ID_REV_ID_MASK_) ==
> > > + ID_REV_ID_LAN7431_) || adapter->is_pci11x1x) {
> > > + phydev = fixed_phy_register(PHY_POLL,
> > > + &fphy_status,
> > > + NULL);
> >
> > I thought something was going to happen with this?
>
> Our SQA confirmed that it's working ping as expected (i.e Speed at
> 1Gbps with full duplex) with Intel I210 NIC as link partner.
>
> Do you suspect any corner case where it's fail?
Let me restate the review comment from V2:
"Eww. Given that phylink has its own internal fixed-PHY support, can we not find some way to avoid the legacy fixed-PHY usage here?"
Yes, it may work, but fixed-phy is not supposed to be used with phylink.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
--- Begin Message ---
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
On Tue, Sep 26, 2023 at 06:28:18AM +0000, Raju.Lakkaraju@xxxxxxxxxxxxx wrote:
> - As you mentioned in another thread (about phylink bindings) a few
> days ago, our current lan743x driver using phylib has a dynamic
> failover dt -> find_phy -> fixed phy sequence.
You mean this:
/* try devicetree phy, or fixed link */
phydev = of_phy_get_and_connect(netdev, adapter->pdev->dev.of_node,
lan743x_phy_link_status_change);
if (!phydev) {
/* try internal phy */
phydev = phy_find_first(adapter->mdiobus);
if (!phydev) {
if ((adapter->csr.id_rev & ID_REV_ID_MASK_) ==
ID_REV_ID_LAN7431_) {
phydev = fixed_phy_register(PHY_POLL,
&fphy_status, NULL);
...
ret = phy_connect_direct(netdev, phydev,
lan743x_phy_link_status_change,
adapter->phy_interface);
> My understanding is that with the current phylink support you need to
> specify ahead of time whether you will be using PHY or FIXED_PHY
> support and cannot fail over from one to the other, which will break
> the current support we have for some of our customers.
This is not actually correct - the fixed PHY support hasn't been used
with phylink because we have a better solution in phylink itself for
fixed links. You rightly point out that this assumes that we've parsed
DT which tells us whether to use that or not.
However, this is possible:
/* try devicetree phy, or fixed link */
ret = phylink_of_phy_connect(pl, adapter->pdev->dev.of_node, 0);
if (ret == -ENODEV) {
/* try internal phy */
phydev = phy_find_first(adapter->mdiobus);
if (!phydev) {
if ((adapter->csr.id_rev & ID_REV_ID_MASK_) ==
ID_REV_ID_LAN7431_) {
phydev = fixed_phy_register(PHY_POLL,
&fphy_status, NULL);
...
ret = phylink_connect_phy(pl, phydev);
The reason this should work is because the fixed-phy support does
emulate a real PHY in software, and phylink will treat that exactly
the same way as a real PHY (because when in phylink is in PHY mode,
it delegates PHY management to phylib.)
Using fixed-phy with phylink will certainly raise some eyebrows, so
the reasons for it will need to be set out in the commit message -
and as you've dropped Andrew from this thread, I suspect he will
still raise some comments about it.
In the longer term, it would probably make sense for phylink to
provide a mechanism where a MAC driver can tell phylink to switch
to using a fixed-link with certain parameters.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
--- End Message ---