Date:   Thu, 24 Mar 2022 17:03:00 +0100
From:   Michael Walle <michael@...le.cc>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        "David S . Miller" <davem@...emloft.net>,
        Xu Liang <lxu@...linear.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC net-next 4/5] net: phy: introduce is_c45_over_c22 flag

Am 2022-03-24 01:41, schrieb Andrew Lunn:
>> > Thinking out loud...
>> >
>> > 1) We have a C22 only bus. Easy, C45 over C22 should be used.
>> >
>> > 2) We have a C45 only bus. Easy, C45 should be used, and it will of
>> >    probed that way.
>> >
>> > 3) We have a C22 and C45 bus, but MDIOBUS_NO_CAP. It will probe C22,
>> >    but ideally we want to swap to C45.
>> >
>> > 4) We have a C22 and C45 bus, MDIOBUS_C22_C45. It will probe C22, but
>> >    ideally we want to swap to C45.
>> 
>> I presume you are speaking of
>> https://elixir.bootlin.com/linux/v5.17/source/drivers/net/phy/mdio_bus.c#L700
> 
> Yes.
> 
>> Shouldn't that be the other way around then? How would you tell if
>> you can do C45?
> 
> For a long time, only C22 was probed. To actually make use of a C45
> only PHY, you had to have a compatible string in DT which indicated a
> C45 device is present on the bus. But then none DT systems came along
> which needed to find a C45 only PHY during probe without the hint it
> is was actually there. That is when the probe capabilities we added,
> and the scan extended to look for a C45 device if the capabilities
> indicates the bus actually supported it. But to keep backwards
> compatibility, C22 was scanned first and then C45 afterwards.

Ok, I figured.

> To some extent, we need to separate finding the device on the bus to
> actually using the device. The device might respond to C22, give us
> its ID, get the correct driver loaded based on that ID, and the driver
> then uses the C45 address space to actually configure the PHY.
> 
> Then there is the Marvel 10G PHY. It responds to C22, but returns 0
> for the ID! There is a special case for this in the code, it then
> looks in the C45 space and uses the ID from there, if it finds
> something useful.
> 
> So as i said in my reply to the cover letter, we have two different
> state variables:
> 
> 1) The PHY has the C45 register space.
> 
> 2) We need to either use C45 transfers, or C45 over C22 transfers to
>    access the C45 register space.
> 
> And we potentially have a chicken/egg problem. The PHY driver knows
> 1), but in order to know what driver to load we need the ID registers
> from the PHY, or some external hint like DT. We are also currently
> only probing C22, or C45, but not C45 over C22. And i'm not sure we
> actually can probe C45 over C22 because there are C22 only PHYs which
> use those two register for other things. So we are back to the driver
> again which does know if C45 over C22 will work.

Isn't it safe to assume that if a PHY implements the indirect
registers for c45 in its c22 space that it will also have a valid
PHY ID and then the it's driver will be probed? So if a PHY is
probed as c22 its driver might tell us "wait, it's actually a c45
phy and hey for your convenience it also have the indirect registers
in c22". We can then set has_c45 and maybe c45_over_c22 (also depending
on the bus capabilities).

> So phydev->has_c45 we can provisionally set if we probed the PHY by
> C45. But the driver should also set it if it knows better, or even the
> core can set it the first time the driver uses an _mmd API call.

I'm not sure about the _mmd calls, there are PHYs which have MMDs
(I guess EEE is an example?) but are not capable of C45 accesses.

> phydev->c45_over_c22 we are currently in a bad shape for. We cannot
> reliably say the bus master supports C45. If the bus capabilities say
> C22 only, we can set phydev->c45_over_c22. If the bus capabilities
> list C45, we can set it false. But that only covers a few busses, most
> don't have any capabilities set. We can try a C45 access and see if we
> get an -EOPNOTSUPP, in which case we can set phydev->c45_over_c22. But
> the bus driver could also do the wrong thing, issue a C22 transfer and
> give us back rubbish.

First question, what do you think about keeping the is_c45 property but
with a different meaning and add use_c45_over_c22. That way it will be
less code churn:

  * @is_c45:  Set to true if this PHY has clause 45 address space.
  * @use_c45_over_c22:  Set to true if c45-over-c22 addressing is used.

  1) c45 phy probed as c45 -> is_c45 = 1, like it is at the moment
     use c45 transfers
2a) c45 phy probed as c22 -> is_c45 = 1, use_c45_over_c22 = 0
     use c45 transfers
2b) c45 phy probed as c22 -> is_c45 = 1, use_c45_over_c22 = 1
     use c22 transfers

Did I miss something?

To promote from c22 to c45 we could look at a flag in the PHY
driver. That should be basically that what the gpy driver is trying
to do with the "if (!is_c45) get_c45_ids()" (but fail).

> So i think we do need to review all the bus drivers and any which do
> support C45 need their capabilities set to indicate that. We can then
> set phydev->c45_over_c22.

I could add the probe_capabilites, or at least I could try. But it won't
tell us if the PHY is capable of the indirect addressing. So we aren't
much further in this regard. But IMHO this can be done incrementally
if someone is interested in having that feature. He should also be in
the position to test it properly.

[just saw your latest mail while writing this, so half of it is done
anyway, btw, I did the same today ;)]

-michael

Powered by blists - more mailing lists