[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b0e5fbe28757d755d814727181c09f32@kernel.org>
Date: Wed, 02 Aug 2023 17:33:20 +0200
From: Michael Walle <mwalle@...nel.org>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Andrew Lunn <andrew@...n.ch>,
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>,
Yisen Zhuang <yisen.zhuang@...wei.com>,
Salil Mehta <salil.mehta@...wei.com>,
Florian Fainelli <florian.fainelli@...adcom.com>,
Broadcom internal kernel review list
<bcm-kernel-feedback-list@...adcom.com>,
Marek BehĂșn <kabel@...nel.org>,
Xu Liang <lxu@...linear.com>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org,
Simon Horman <simon.horman@...igine.com>
Subject: Re: [PATCH net-next v3 02/11] net: phy: introduce
phy_has_c45_registers()
Am 2023-08-01 17:57, schrieb Russell King (Oracle):
> On Tue, Aug 01, 2023 at 05:20:22PM +0200, Michael Walle wrote:
>> > In the case of the above (the code in __phy_read_mmd()), I wouldn't
>> > at least initially change the test there.
>> >
>> > phydev->is_c45 will only be true if we probed the PHY using clause
>> > 45 accesses. Thus, it will be set if "the bus supports clause 45
>> > accesses" _and_ "the PHY responds to those accesses".
>> >
>> > Changing that to only "the bus supports clause 45 accesses" means
>> > that a PHY supporting only clause 22 access with indirect clause
>> > 45 access then fails if it's used with a bus that supports both
>> > clause 22 and clause 45 accesses.
>>
>> Yeah of course. It was more about the naming, but I just realized
>> that with mdiobus_supports_c45() you can't access the original
>> "is_c45" property of the PHY. So maybe this patch needs to be split
>> into two to get rid of .is_c45:
>>
>> First a mechanical one:
>> phy_has_c45_registers() {
>> return phydev->is_c45;
>> }
>
> Andrew's objection was that "phy_has_c45_registers" is a misnomer, and
> suggested "_transfers" instead - because a PHY can have C45 registers
> that are accessible via the indirect registers in C22 space.
I'm confused now. Andrew suggested to split it into four different
functions:
phy_has_c22_registers()
phy_has_c45_registers()
phy_has_c22_transfers()
phy_has_c45_transfers()
Without a functional change. That is, either return phydev->is_c45
or the inverse.
You seem to suggest to use either
phy_supports_c45_transfers() or
phy_has_c22_registers()
I'm not sure how to continue now.
> I'd go one further:
>
> static bool phy_supports_c45_transfers(struct phy_device *phydev)
> {
> return phydev->is_c45;
> }
>
> Since that covers that (a) the bus needs to support C45 transfers and
> (b) the PHY also needs to respond to C45 transfers.
>
> If we want to truly know whether a clause 22 PHY has clause 45
> registers, that's difficult to answer, because then you're into the
> realms of "does this PHY implement the indirect access method" and
> we haven't been keeping track of that for the PHYs we have drivers
> for - many will do, but it's optional in clause 22. The problem is
> that when it's not implemented, the registers could be serving some
> other function.
>
>> phy_has_c22_registers() {
>> return !phydev->is_c45;
>> }
>
> The reverse is not true, as clause 45 PHYs can also support clause 22
> registers - from 802.3:
>
> "For cases where a single entity combines Clause 45 MMDs with Clause
> 22
> registers, then the Clause 22 registers may be accessed using the
> Clause
> 45 electrical interface and the Clause 22 management frame structure."
>
> "Bit 5.0 is used to indicate that Clause 22 functionality has been
> implemented within a Clause 45 electrical interface device."
>
> Therefore, this would more accurately describe when Clause 22 registers
> are present for a PHY:
>
> static bool phy_has_c22_registers(struct phy_device *phydev)
> {
> /* If we probed the PHY without clause 45 accesses, then by
> * definition, clause 22 registers must be present.
> */
> if (!phydev->is_c45)
> return true;
>
> /* If we probed the PHY with clause 45 accesses, clause 22
> * registers may be present if bit 0 in the Devices-in-pacakge
> * register pair is set.
> */
> return phydev->c45_ids.devices_in_package & BIT(0);
> }
>
> Note that this doesn't take account of whether the bus supports clause
> 22 register access - there are a number of MDIO buses that do not
> support such accesses, and they may be coupled with a PHY that does
> support clause 22 registers.
>
> I'm aware of a SFP with a Realtek PHY on that falls into this exact
> case, and getting that working is progressing at the moment.
>
>> For all the places Andrew said it's correct. Leave all the
>> other uses of .is_c45 as is for now and rework them in a
>> later patch to use mdiobus_supports_{c22,c45}().
>
> For the two cases in marvell10g and bcm84881, the test there for
> is_c45 is purely to determine "was this ID found on a PHY supporting
> clause 45 access" - however, in both cases, a check is made for MMDs
> present in devices_in_package which will fail if the PHY wasn't
> discovered in clause 45 mode.
>
> Note that 88x3310 does not support clause 22 access. I forget whether
> bcm84881 does or not.
So a simple "phydev->is_c45" should be enough? Why do you test
for the MMD presence bits?
-michael
Powered by blists - more mailing lists