The GPY215 driver supports indirect accesses to c45 over the c22
registers. In its probe function phy_get_c45_ids() is called and the
author descibed their use case as follows:
The problem comes from condition "phydev->c45_ids.mmds_present &
MDIO_DEVS_AN".
Our product supports both C22 and C45.
In the real system, we found C22 was used by customers (with indirect
access to C45 registers when necessary).
So it is pretty clear that the intention was to have a method to use the
c45 features over a c22-only MDIO bus. The purpose of calling
phy_get_c45_ids() is to populate the .c45_ids for a PHY which wasn't
probed as a c45 one. Thus, first rename the phy_get_c45_ids() function
to reflect its actual meaning and second, add a new flag which indicates
that this is actually a c45 PHY but behind a c22 bus. The latter is
important for phylink because phylink will treat c45 in a special way by
checking the .is_c45 property. But in our case this isn't set.
Signed-off-by: Michael Walle <[email protected]>
---
drivers/net/phy/mxl-gpy.c | 2 +-
drivers/net/phy/phy_device.c | 20 +++++++++++++++-----
include/linux/phy.h | 4 +++-
3 files changed, 19 insertions(+), 7 deletions(-)
diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
index 5ce1bf03bbd7..0c825ec20eaa 100644
--- a/drivers/net/phy/mxl-gpy.c
+++ b/drivers/net/phy/mxl-gpy.c
@@ -99,7 +99,7 @@ static int gpy_probe(struct phy_device *phydev)
int ret;
if (!phydev->is_c45) {
- ret = phy_get_c45_ids(phydev);
+ ret = phy_get_c45_ids_by_c22(phydev);
if (ret < 0)
return ret;
}
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index c766f5bb421a..43354b261bd5 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1005,18 +1005,28 @@ void phy_device_remove(struct phy_device *phydev)
EXPORT_SYMBOL(phy_device_remove);
/**
- * phy_get_c45_ids - Read 802.3-c45 IDs for phy device.
+ * phy_get_c45_ids - Read 802.3-c45 IDs for phy device by using indirect
+ * c22 accesses.
* @phydev: phy_device structure to read 802.3-c45 IDs
*
* Returns zero on success, %-EIO on bus access error, or %-ENODEV if
* the "devices in package" is invalid.
*/
-int phy_get_c45_ids(struct phy_device *phydev)
+int phy_get_c45_ids_by_c22(struct phy_device *phydev)
{
- return get_phy_c45_ids(phydev->mdio.bus, phydev->mdio.addr,
- &phydev->c45_ids);
+ int ret;
+
+ if (WARN(phydev->is_c45, "PHY is already clause 45\n"))
+ return -EINVAL;
+
+ ret = get_phy_c45_ids(phydev->mdio.bus, phydev->mdio.addr,
+ &phydev->c45_ids);
+ if (!ret)
+ phydev->is_c45_over_c22 = true;
+
+ return ret;
}
-EXPORT_SYMBOL(phy_get_c45_ids);
+EXPORT_SYMBOL(phy_get_c45_ids_by_c22);
/**
* phy_find_first - finds the first PHY device on the bus
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 36ca2b5c2253..eb436d603feb 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -525,6 +525,7 @@ struct macsec_ops;
* @phy_id: UID for this device found during discovery
* @c45_ids: 802.3-c45 Device Identifiers if is_c45.
* @is_c45: Set to true if this PHY uses clause 45 addressing.
+ * @is_c45_over_c22: Set to true if this PHY uses c45-over-c22 addressing.
* @is_internal: Set to true if this PHY is internal to a MAC.
* @is_pseudo_fixed_link: Set to true if this PHY is an Ethernet switch, etc.
* @is_gigabit_capable: Set to true if PHY supports 1000Mbps
@@ -606,6 +607,7 @@ struct phy_device {
struct phy_c45_device_ids c45_ids;
unsigned is_c45:1;
+ unsigned is_c45_over_c22:1;
unsigned is_internal:1;
unsigned is_pseudo_fixed_link:1;
unsigned is_gigabit_capable:1;
@@ -1466,7 +1468,7 @@ static inline int phy_device_register(struct phy_device *phy)
static inline void phy_device_free(struct phy_device *phydev) { }
#endif /* CONFIG_PHYLIB */
void phy_device_remove(struct phy_device *phydev);
-int phy_get_c45_ids(struct phy_device *phydev);
+int phy_get_c45_ids_by_c22(struct phy_device *phydev);
int phy_init_hw(struct phy_device *phydev);
int phy_suspend(struct phy_device *phydev);
int phy_resume(struct phy_device *phydev);
--
2.30.2
On Wed, Mar 23, 2022 at 07:34:18PM +0100, Michael Walle wrote:
> The GPY215 driver supports indirect accesses to c45 over the c22
> registers. In its probe function phy_get_c45_ids() is called and the
> author descibed their use case as follows:
>
> The problem comes from condition "phydev->c45_ids.mmds_present &
> MDIO_DEVS_AN".
>
> Our product supports both C22 and C45.
>
> In the real system, we found C22 was used by customers (with indirect
> access to C45 registers when necessary).
>
> So it is pretty clear that the intention was to have a method to use the
> c45 features over a c22-only MDIO bus. The purpose of calling
> phy_get_c45_ids() is to populate the .c45_ids for a PHY which wasn't
> probed as a c45 one. Thus, first rename the phy_get_c45_ids() function
> to reflect its actual meaning and second, add a new flag which indicates
> that this is actually a c45 PHY but behind a c22 bus. The latter is
> important for phylink because phylink will treat c45 in a special way by
> checking the .is_c45 property. But in our case this isn't set.
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.
> @@ -99,7 +99,7 @@ static int gpy_probe(struct phy_device *phydev)
> int ret;
>
> if (!phydev->is_c45) {
> - ret = phy_get_c45_ids(phydev);
> + ret = phy_get_c45_ids_by_c22(phydev);
> if (ret < 0)
> return ret;
> }
If we are inside the if, we know we probed C22. We have to achieve two
things:
1) Get the c45 ids,
2) Figure out if C45 works, or if C45 over C22 is needed.
I don't see how we are getting this second bit of information, if we
are explicitly using c45 over c22.
This _by_c22 is also making me think of the previous patch, where we
look at the bus capabilities. We are explicitly saying here was want
c45 over c22, and the PHY driver should know the PHY is capable of
it. So we don't need to look at the capabilities, just do it.
Andrew
> > 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.
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.
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.
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.
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.
Andrew
> > 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?
See: https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy_device.c#L895
No valid ID in C22 space.
> 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).
In general, if the core can do something, it is better than the driver
doing it. If the core cannot reliably figure it out, then we have to
leave it to the drivers. It could well be we need the drivers to set
has_c45. I would prefer that drivers don't touch c45_over_c22 because
they don't have the knowledge of what the bus is capable of doing. The
only valid case i can think of is for a very oddball PHY which has C45
register space, but cannot actually do C45 transfers, and so C45 over
C22 is the only option.
> > 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.
Ah, yes, i forgot about EEE. That was a bad idea.
> > 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.
I prefer to change is_c45. We then get the compiler to help us with
code review. The build bots will tell us about any code we fail to
check and change. It will also help anybody with out of tree code
making use of is_c45.
Andrew
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
> The
> > only valid case i can think of is for a very oddball PHY which has C45
> > register space, but cannot actually do C45 transfers, and so C45 over
> > C22 is the only option.
>
> And how would you know that the PHY has the needed registers in c22
> space? Or do we assume that every C45 PHY has these registers?
I think it is a reasonable assumption at the moment. We have around
170 MDIO bus masters in Linux. All but one can do C22. There are
around 15 which can do both C22 and C45, and only one that i know of
which is C45 only. So anybody making a C45 transfer only PHY is making
their device hard to sell.
I suppose a PHY integrated with the MAC could by C45 only, and not
support C22, but i've not seen such a device yet. My guess such a
device would be an Ethernet switch, or a USB device, or maybe an
automotive device with a T1 PHY.
Andrew
Am 2022-03-23 21:07, schrieb Andrew Lunn:
> On Wed, Mar 23, 2022 at 07:34:18PM +0100, Michael Walle wrote:
>> The GPY215 driver supports indirect accesses to c45 over the c22
>> registers. In its probe function phy_get_c45_ids() is called and the
>> author descibed their use case as follows:
>>
>> The problem comes from condition "phydev->c45_ids.mmds_present &
>> MDIO_DEVS_AN".
>>
>> Our product supports both C22 and C45.
>>
>> In the real system, we found C22 was used by customers (with
>> indirect
>> access to C45 registers when necessary).
>>
>> So it is pretty clear that the intention was to have a method to use
>> the
>> c45 features over a c22-only MDIO bus. The purpose of calling
>> phy_get_c45_ids() is to populate the .c45_ids for a PHY which wasn't
>> probed as a c45 one. Thus, first rename the phy_get_c45_ids() function
>> to reflect its actual meaning and second, add a new flag which
>> indicates
>> that this is actually a c45 PHY but behind a c22 bus. The latter is
>> important for phylink because phylink will treat c45 in a special way
>> by
>> checking the .is_c45 property. But in our case this isn't set.
>
> 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
Shouldn't that be the other way around then? How would you tell if
you can do C45?
>> @@ -99,7 +99,7 @@ static int gpy_probe(struct phy_device *phydev)
>> int ret;
>>
>> if (!phydev->is_c45) {
>> - ret = phy_get_c45_ids(phydev);
>> + ret = phy_get_c45_ids_by_c22(phydev);
>> if (ret < 0)
>> return ret;
>> }
>
> If we are inside the if, we know we probed C22. We have to achieve two
> things:
>
> 1) Get the c45 ids,
> 2) Figure out if C45 works, or if C45 over C22 is needed.
>
> I don't see how we are getting this second bit of information, if we
> are explicitly using c45 over c22.
That is related to how C45 capable PHYs are probed (your 4) above),
right? If the PHY would be probed correctly as C45 we wouldn't have
to worry about it. TBH I didn't consider that a valid case because
I thought there were other means to tell "treat this PHY as C45",
that is by the device tree compatible, for example.
Btw. all of this made me question if this is actually the correct
place, or if if shouldn't be handled in the core. With a flag
in the phy driver which might indicate its capable of doing
c45 over c22.
> This _by_c22 is also making me think of the previous patch, where we
> look at the bus capabilities. We are explicitly saying here was want
> c45 over c22, and the PHY driver should know the PHY is capable of
> it. So we don't need to look at the capabilities, just do it.
Mh? I can't follow you here. Are you talking about the
probe_capabilites? These are for the bus probing, i.e. if you can
call mdiobus_c45_read().
-michael
Am 2022-03-24 17:23, schrieb Andrew Lunn:
>> > 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?
>
> See:
> https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy_device.c#L895
>
> No valid ID in C22 space.
I actually looked at the datasheet and yes, it implements the
registers 13 and 14 in c22 to access the c45 space. I couldn't
find any descriptions of other c22 registers though.
>> 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).
>
> In general, if the core can do something, it is better than the driver
> doing it. If the core cannot reliably figure it out, then we have to
> leave it to the drivers. It could well be we need the drivers to set
> has_c45. I would prefer that drivers don't touch c45_over_c22 because
> they don't have the knowledge of what the bus is capable of doing. The
> only valid case i can think of is for a very oddball PHY which has C45
> register space, but cannot actually do C45 transfers, and so C45 over
> C22 is the only option.
And how would you know that the PHY has the needed registers in c22
space? Or do we assume that every C45 PHY has these registers?
..
>> > 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.
>
> I prefer to change is_c45. We then get the compiler to help us with
> code review. The build bots will tell us about any code we fail to
> check and change. It will also help anybody with out of tree code
> making use of is_c45.
Ok. Fair enough.
-michael
On Thu, Mar 24, 2022 at 01:41:44AM +0100, Andrew Lunn wrote:
ydev->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.
Unfortunately, trying a C45 access will be very hit and miss - we
need to fix all the MDIO drivers before we do that to check the
access type. Many don't, and worse, many assume a C22 formatted
access request, and just try throwing the PHY address and register
address into the register fields without any masking. The result is
that a C45 access will set random bits in the register.
For example:
drivers/net/mdio/mdio-bcm-iproc.c (no bus capability):
/* Prepare the read operation */
cmd = (MII_DATA_TA_VAL << MII_DATA_TA_SHIFT) |
(reg << MII_DATA_RA_SHIFT) |
(phy_id << MII_DATA_PA_SHIFT) |
BIT(MII_DATA_SB_SHIFT) |
(MII_DATA_OP_READ << MII_DATA_OP_SHIFT);
writel(cmd, priv->base + MII_DATA_OFFSET);
Similar is true for:
drivers/net/mdio/mdio-bcm-unimac.c (no bus capability)
drivers/net/mdio/mdio-hisi-femac.c (no bus capability)
drivers/net/mdio/mdio-moxart.c (no bus capability)
drivers/net/mdio/mdio-mscc-miim.c (no bus capability)
drivers/net/mdio/mdio-mux-bcm6368.c (no bus capability)
drivers/net/mdio/mdio-mux-bcm-iproc.c (no bus capability)
drivers/net/mdio/mdio-sun4i.c (no bus capability)
These truncate the fields, and fwics they don't set the bus type:
drivers/net/mdio/mdio-xgene.c (for the "rgmii" only bus and no bus capability)
So all of the above need at the very least code added to reject a C45
"dev" or "phy_id" address, or they need to set the bus capability
correctly.
My feeling is that the introduction of the bus capability hasn't done
much to improve this situation; it was introduced to hint at whether a
bus is safe for clause 45 accesses, but it hasn't actually solved this
problem because we patently still have this same issue today.
I think we just need to bite the bullet and audit all the MDIO drivers
we currently have, checking what the results would be if they were
passed a C45 access request, and make them reject such a request if
the register address or phy address obviously overflows into different
register fields and also mark them as C22-only.
I can't see any other reasonable option.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Thu, Mar 31, 2022 at 12:50:42PM +0100, Russell King (Oracle) wrote:
> On Thu, Mar 24, 2022 at 07:55:28PM +0100, Andrew Lunn wrote:
> > > The
> > > > only valid case i can think of is for a very oddball PHY which has C45
> > > > register space, but cannot actually do C45 transfers, and so C45 over
> > > > C22 is the only option.
> > >
> > > And how would you know that the PHY has the needed registers in c22
> > > space? Or do we assume that every C45 PHY has these registers?
> >
> > I think it is a reasonable assumption at the moment. We have around
> > 170 MDIO bus masters in Linux. All but one can do C22.
>
> I don't think that is correct. I'm aware of the Marvell XMDIO driver
> that is C45 only, and also xgene's non-rgmii "xfi" variant which is
> also C45 only. Note that the xfi variant doesn't reject C22 and makes
> no distinction between a C22 and C45 access (so a C22 access to
> phy_id = 0 reg = 0 hits C45 phy_id = 0 mmd 0 reg 0.
>
> MDIO drivers are IMHO an utter mess and are in dire need of fixing...
> and I'm coming to the conclusion that the bodge of passing both C22
> and C45 accesses through the same read/write functions is a huge
> mistake, one that is crying out for fixing to prevent more prolification
> of this kind of mess.
>
> Yes, it's a lot of work, but I think it needs to be done. Retrofitting
> the MDIO drivers with checks etc sounds nice, but if we assume that
> patches will continue to be applied to net-next with little review,
> we have a losing battle - it would be better to have interfaces designed
> to make this kind of mistake impossible.
Hi Russell
So what i think you are saying is change the mii_bus structure:
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 36ca2b5c2253..26322ee23867 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -353,10 +353,15 @@ struct mii_bus {
const char *name;
char id[MII_BUS_ID_SIZE];
void *priv;
- /** @read: Perform a read transfer on the bus */
- int (*read)(struct mii_bus *bus, int addr, int regnum);
- /** @write: Perform a write transfer on the bus */
- int (*write)(struct mii_bus *bus, int addr, int regnum, u16 val);
+ /** @read: Perform a C22 read transfer on the bus */
+ int (*read_c22)(struct mii_bus *bus, int addr, int regnum);
+ /** @write: Perform a C22 write transfer on the bus */
+ int (*write_c22)(struct mii_bus *bus, int addr, int regnum, u16 val);
+ /** @read: Perform a C45 read transfer on the bus */
+ int (*read_c45)(struct mii_bus *bus, int addr, int devnum, int regnum);
+ /** @write: Perform a C45 write transfer on the bus */
+ int (*write_c45)(struct mii_bus *bus, int addr, int devnum,
+ int regnum, u16 val);
/** @reset: Perform a reset of the bus */
int (*reset)(struct mii_bus *bus);
This way we get a cleaner interface, and the compiler helping us
finding drivers we miss during conversion?
Andrew
On Thu, Mar 24, 2022 at 07:55:28PM +0100, Andrew Lunn wrote:
> > The
> > > only valid case i can think of is for a very oddball PHY which has C45
> > > register space, but cannot actually do C45 transfers, and so C45 over
> > > C22 is the only option.
> >
> > And how would you know that the PHY has the needed registers in c22
> > space? Or do we assume that every C45 PHY has these registers?
>
> I think it is a reasonable assumption at the moment. We have around
> 170 MDIO bus masters in Linux. All but one can do C22.
I don't think that is correct. I'm aware of the Marvell XMDIO driver
that is C45 only, and also xgene's non-rgmii "xfi" variant which is
also C45 only. Note that the xfi variant doesn't reject C22 and makes
no distinction between a C22 and C45 access (so a C22 access to
phy_id = 0 reg = 0 hits C45 phy_id = 0 mmd 0 reg 0.
MDIO drivers are IMHO an utter mess and are in dire need of fixing...
and I'm coming to the conclusion that the bodge of passing both C22
and C45 accesses through the same read/write functions is a huge
mistake, one that is crying out for fixing to prevent more prolification
of this kind of mess.
Yes, it's a lot of work, but I think it needs to be done. Retrofitting
the MDIO drivers with checks etc sounds nice, but if we assume that
patches will continue to be applied to net-next with little review,
we have a losing battle - it would be better to have interfaces designed
to make this kind of mistake impossible.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Thu, Mar 31, 2022 at 02:06:16PM +0200, Andrew Lunn wrote:
> On Thu, Mar 31, 2022 at 12:50:42PM +0100, Russell King (Oracle) wrote:
> > On Thu, Mar 24, 2022 at 07:55:28PM +0100, Andrew Lunn wrote:
> > > > The
> > > > > only valid case i can think of is for a very oddball PHY which has C45
> > > > > register space, but cannot actually do C45 transfers, and so C45 over
> > > > > C22 is the only option.
> > > >
> > > > And how would you know that the PHY has the needed registers in c22
> > > > space? Or do we assume that every C45 PHY has these registers?
> > >
> > > I think it is a reasonable assumption at the moment. We have around
> > > 170 MDIO bus masters in Linux. All but one can do C22.
> >
> > I don't think that is correct. I'm aware of the Marvell XMDIO driver
> > that is C45 only, and also xgene's non-rgmii "xfi" variant which is
> > also C45 only. Note that the xfi variant doesn't reject C22 and makes
> > no distinction between a C22 and C45 access (so a C22 access to
> > phy_id = 0 reg = 0 hits C45 phy_id = 0 mmd 0 reg 0.
> >
> > MDIO drivers are IMHO an utter mess and are in dire need of fixing...
> > and I'm coming to the conclusion that the bodge of passing both C22
> > and C45 accesses through the same read/write functions is a huge
> > mistake, one that is crying out for fixing to prevent more prolification
> > of this kind of mess.
> >
> > Yes, it's a lot of work, but I think it needs to be done. Retrofitting
> > the MDIO drivers with checks etc sounds nice, but if we assume that
> > patches will continue to be applied to net-next with little review,
> > we have a losing battle - it would be better to have interfaces designed
> > to make this kind of mistake impossible.
>
> Hi Russell
>
> So what i think you are saying is change the mii_bus structure:
>
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 36ca2b5c2253..26322ee23867 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -353,10 +353,15 @@ struct mii_bus {
> const char *name;
> char id[MII_BUS_ID_SIZE];
> void *priv;
> - /** @read: Perform a read transfer on the bus */
> - int (*read)(struct mii_bus *bus, int addr, int regnum);
> - /** @write: Perform a write transfer on the bus */
> - int (*write)(struct mii_bus *bus, int addr, int regnum, u16 val);
> + /** @read: Perform a C22 read transfer on the bus */
> + int (*read_c22)(struct mii_bus *bus, int addr, int regnum);
> + /** @write: Perform a C22 write transfer on the bus */
> + int (*write_c22)(struct mii_bus *bus, int addr, int regnum, u16 val);
> + /** @read: Perform a C45 read transfer on the bus */
> + int (*read_c45)(struct mii_bus *bus, int addr, int devnum, int regnum);
> + /** @write: Perform a C45 write transfer on the bus */
> + int (*write_c45)(struct mii_bus *bus, int addr, int devnum,
> + int regnum, u16 val);
> /** @reset: Perform a reset of the bus */
> int (*reset)(struct mii_bus *bus);
>
> This way we get a cleaner interface, and the compiler helping us
> finding drivers we miss during conversion?
Yes, and not only does it mean the compiler helps us find unconverted
drivers, it will also stop new drivers that use the old interfaces being
merged and most importantly it puts an end to the ongoing question about
which clause accesses are actually supported by a MDIO driver.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Thu, Mar 24, 2022 at 06:18:14PM +0100, Michael Walle wrote:
> Am 2022-03-24 17:23, schrieb Andrew Lunn:
> > > 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?
> >
> > See:
> > https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy_device.c#L895
> >
> > No valid ID in C22 space.
>
> I actually looked at the datasheet and yes, it implements the
> registers 13 and 14 in c22 to access the c45 space. I couldn't
> find any descriptions of other c22 registers though.
I'm not sure which PHY you're referring to here, but iirc, the later
hardware revisions of the 88x3310 implement the indirect access, but
earlier revisions do not.
> > In general, if the core can do something, it is better than the driver
> > doing it. If the core cannot reliably figure it out, then we have to
> > leave it to the drivers. It could well be we need the drivers to set
> > has_c45. I would prefer that drivers don't touch c45_over_c22 because
> > they don't have the knowledge of what the bus is capable of doing. The
> > only valid case i can think of is for a very oddball PHY which has C45
> > register space, but cannot actually do C45 transfers, and so C45 over
> > C22 is the only option.
>
> And how would you know that the PHY has the needed registers in c22
> space? Or do we assume that every C45 PHY has these registers?
That's the problem. Currently C22 PHY drivers that do not support the
C45 register space have to set the .read_mmd and .write_mmd methods to
genphy_read_mmd_unsupported/genphy_write_mmd_unsupported which
effectively disables access to the C45 register space. In order for
that to happen, we must have read the C22 PHY ID and bound the driver.
That doesn't help with reading the PHY ID though.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!