diff mbox series

[RFC,08/11] net: phy: Allow mdio buses to auto-probe c45 devices

Message ID 20200522213059.1535892-9-jeremy.linton@arm.com
State RFC
Delegated to: David Miller
Headers show
Series Make C45 autoprobe more robust | expand

Commit Message

Jeremy Linton May 22, 2020, 9:30 p.m. UTC
The mdiobus_scan logic is currently hardcoded to only
work with c22 devices. This works fairly well in most
cases, but its possible a c45 device doesn't respond
despite being a standard phy. If the parent hardware
is capable, it makes sense to scan for c45 devices before
falling back to c22.

As we want this to reflect the capabilities of the STA,
lets add a field to the mii_bus structure to represent
the capability. That way devices can opt into the extended
scanning. Existing users should continue to default to c22
only scanning as long as they are zero'ing the structure
before use.

At the moment its a yes/no option, but in the future
it may be useful to extend this to c45 only policy, or
add additional classes and policies.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 drivers/net/phy/mdio_bus.c | 9 +++++++--
 include/linux/phy.h        | 5 +++++
 2 files changed, 12 insertions(+), 2 deletions(-)

Comments

Andrew Lunn May 24, 2020, 2:44 p.m. UTC | #1
> +++ b/include/linux/phy.h
> @@ -275,6 +275,11 @@ struct mii_bus {
>  	int reset_delay_us;
>  	/* RESET GPIO descriptor pointer */
>  	struct gpio_desc *reset_gpiod;
> +	/* bus capabilities, used for probing */
> +	enum {
> +		MDIOBUS_C22_ONLY = 0,
> +		MDIOBUS_C45_FIRST,
> +	} probe_capabilities;
>  };


I'm not so keen on _FIRST. It suggest _LAST would also be valid.  But
that then suggests this is not a bus property, but a PHY property, and
some PHYs might need _FIRST and other phys need _LAST, and then you
have a bus which has both sorts of PHY on it, and you have a problem.

So i think it would be better to have

	enum {
		MDIOBUS_UNKNOWN = 0,
		MDIOBUS_C22,
		MDIOBUS_C45,
		MDIOBUS_C45_C22,
	} bus_capabilities;

Describe just what the bus master can support.

	 Andrew
Jeremy Linton May 25, 2020, 4:28 a.m. UTC | #2
Hi,

On 5/24/20 9:44 AM, Andrew Lunn wrote:
>> +++ b/include/linux/phy.h
>> @@ -275,6 +275,11 @@ struct mii_bus {
>>   	int reset_delay_us;
>>   	/* RESET GPIO descriptor pointer */
>>   	struct gpio_desc *reset_gpiod;
>> +	/* bus capabilities, used for probing */
>> +	enum {
>> +		MDIOBUS_C22_ONLY = 0,
>> +		MDIOBUS_C45_FIRST,
>> +	} probe_capabilities;
>>   };
> 
> 
> I'm not so keen on _FIRST. It suggest _LAST would also be valid.  But
> that then suggests this is not a bus property, but a PHY property, and
> some PHYs might need _FIRST and other phys need _LAST, and then you
> have a bus which has both sorts of PHY on it, and you have a problem.
> 
> So i think it would be better to have
> 
> 	enum {
> 		MDIOBUS_UNKNOWN = 0,
> 		MDIOBUS_C22,
> 		MDIOBUS_C45,
> 		MDIOBUS_C45_C22,
> 	} bus_capabilities;
> 
> Describe just what the bus master can support.

Yes, the naming is reasonable and I will update it in the next patch. I 
went around a bit myself with this naming early on, and the problem I 
saw was that a C45 capable master, can have C45 electrical phy's that 
only respond to c22 requests (AFAIK). So the MDIOBUS_C45 (I think I was 
calling it C45_ONLY) is an invalid selection. Not, that it wouldn't be 
helpful to have a C45_ONLY case, but that the assumption is that you 
wouldn't try and probe c22 registers, which I thought was a mistake.


Thanks,
Russell King (Oracle) May 25, 2020, 8:25 a.m. UTC | #3
On Sun, May 24, 2020 at 11:28:52PM -0500, Jeremy Linton wrote:
> Hi,
> 
> On 5/24/20 9:44 AM, Andrew Lunn wrote:
> > > +++ b/include/linux/phy.h
> > > @@ -275,6 +275,11 @@ struct mii_bus {
> > >   	int reset_delay_us;
> > >   	/* RESET GPIO descriptor pointer */
> > >   	struct gpio_desc *reset_gpiod;
> > > +	/* bus capabilities, used for probing */
> > > +	enum {
> > > +		MDIOBUS_C22_ONLY = 0,
> > > +		MDIOBUS_C45_FIRST,
> > > +	} probe_capabilities;
> > >   };
> > 
> > 
> > I'm not so keen on _FIRST. It suggest _LAST would also be valid.  But
> > that then suggests this is not a bus property, but a PHY property, and
> > some PHYs might need _FIRST and other phys need _LAST, and then you
> > have a bus which has both sorts of PHY on it, and you have a problem.
> > 
> > So i think it would be better to have
> > 
> > 	enum {
> > 		MDIOBUS_UNKNOWN = 0,
> > 		MDIOBUS_C22,
> > 		MDIOBUS_C45,
> > 		MDIOBUS_C45_C22,
> > 	} bus_capabilities;
> > 
> > Describe just what the bus master can support.
> 
> Yes, the naming is reasonable and I will update it in the next patch. I went
> around a bit myself with this naming early on, and the problem I saw was
> that a C45 capable master, can have C45 electrical phy's that only respond
> to c22 requests (AFAIK).

If you have a master that can only generate clause 45 cycles, and
someone is daft enough to connect a clause 22 only PHY to it, the
result is hardware that doesn't work - there's no getting around
that.  The MDIO interface can't generate the appropriate cycles to
access the clause 22 PHY.  So, this is not something we need care
about.

> So the MDIOBUS_C45 (I think I was calling it
> C45_ONLY) is an invalid selection. Not, that it wouldn't be helpful to have
> a C45_ONLY case, but that the assumption is that you wouldn't try and probe
> c22 registers, which I thought was a mistake.

MDIOBUS_C45 means "I can generate clause 45 cycles".
MDIOBUS_C22 means "I can generate clause 22 cycles".
MDIOBUS_C45_C22 means "I can generate both clause 45 and clause 22
cycles."

Notice carefully the values these end up with - MDIOBUS_C22 = BIT(0),
MDIOBUS_C45 = BIT(1), MDIOBUS_C45_C22 = BIT(0) | BIT(1).  I suspect
that was no coincidence in Andrew's suggestion.
Andrew Lunn May 25, 2020, 1:43 p.m. UTC | #4
> > > So i think it would be better to have
> > > 
> > > 	enum {
> > > 		MDIOBUS_UNKNOWN = 0,
> > > 		MDIOBUS_C22,
> > > 		MDIOBUS_C45,
> > > 		MDIOBUS_C45_C22,
> > > 	} bus_capabilities;
> > > 
> > > Describe just what the bus master can support.
> > 
> > Yes, the naming is reasonable and I will update it in the next patch. I went
> > around a bit myself with this naming early on, and the problem I saw was
> > that a C45 capable master, can have C45 electrical phy's that only respond
> > to c22 requests (AFAIK).
> 
> If you have a master that can only generate clause 45 cycles, and
> someone is daft enough to connect a clause 22 only PHY to it, the
> result is hardware that doesn't work - there's no getting around
> that.  The MDIO interface can't generate the appropriate cycles to
> access the clause 22 PHY.  So, this is not something we need care
> about.
> 
> > So the MDIOBUS_C45 (I think I was calling it
> > C45_ONLY) is an invalid selection. Not, that it wouldn't be helpful to have
> > a C45_ONLY case, but that the assumption is that you wouldn't try and probe
> > c22 registers, which I thought was a mistake.
> 
> MDIOBUS_C45 means "I can generate clause 45 cycles".
> MDIOBUS_C22 means "I can generate clause 22 cycles".
> MDIOBUS_C45_C22 means "I can generate both clause 45 and clause 22
> cycles."
> 
> Notice carefully the values these end up with - MDIOBUS_C22 = BIT(0),
> MDIOBUS_C45 = BIT(1), MDIOBUS_C45_C22 = BIT(0) | BIT(1).  I suspect
> that was no coincidence in Andrew's suggestion.

Hi Russell

What was a nice side affect. Since i doubt Jeremy is going to go
through every MDIO driver and set the capabilities correctly, i wanted
0 to have a safe meaning. In the code we should treat MDIOBUS_UNKNOWN
and MDIOBUS_C22 identically. But maybe some time in the distant
future, we can make 0 issue a warning.

  Andrew
Jeremy Linton May 25, 2020, 10:09 p.m. UTC | #5
Hi,

On 5/25/20 3:25 AM, Russell King - ARM Linux admin wrote:
> On Sun, May 24, 2020 at 11:28:52PM -0500, Jeremy Linton wrote:
>> Hi,
>>
>> On 5/24/20 9:44 AM, Andrew Lunn wrote:
>>>> +++ b/include/linux/phy.h
>>>> @@ -275,6 +275,11 @@ struct mii_bus {
>>>>    	int reset_delay_us;
>>>>    	/* RESET GPIO descriptor pointer */
>>>>    	struct gpio_desc *reset_gpiod;
>>>> +	/* bus capabilities, used for probing */
>>>> +	enum {
>>>> +		MDIOBUS_C22_ONLY = 0,
>>>> +		MDIOBUS_C45_FIRST,
>>>> +	} probe_capabilities;
>>>>    };
>>>
>>>
>>> I'm not so keen on _FIRST. It suggest _LAST would also be valid.  But
>>> that then suggests this is not a bus property, but a PHY property, and
>>> some PHYs might need _FIRST and other phys need _LAST, and then you
>>> have a bus which has both sorts of PHY on it, and you have a problem.
>>>
>>> So i think it would be better to have
>>>
>>> 	enum {
>>> 		MDIOBUS_UNKNOWN = 0,
>>> 		MDIOBUS_C22,
>>> 		MDIOBUS_C45,
>>> 		MDIOBUS_C45_C22,
>>> 	} bus_capabilities;
>>>
>>> Describe just what the bus master can support.
>>
>> Yes, the naming is reasonable and I will update it in the next patch. I went
>> around a bit myself with this naming early on, and the problem I saw was
>> that a C45 capable master, can have C45 electrical phy's that only respond
>> to c22 requests (AFAIK).
> 
> If you have a master that can only generate clause 45 cycles, and
> someone is daft enough to connect a clause 22 only PHY to it, the
> result is hardware that doesn't work - there's no getting around
> that.  The MDIO interface can't generate the appropriate cycles to
> access the clause 22 PHY.  So, this is not something we need care
> about.
> 
>> So the MDIOBUS_C45 (I think I was calling it
>> C45_ONLY) is an invalid selection. Not, that it wouldn't be helpful to have
>> a C45_ONLY case, but that the assumption is that you wouldn't try and probe
>> c22 registers, which I thought was a mistake.
> 
> MDIOBUS_C45 means "I can generate clause 45 cycles".
> MDIOBUS_C22 means "I can generate clause 22 cycles".
> MDIOBUS_C45_C22 means "I can generate both clause 45 and clause 22
> cycles."

Hi, to be clear, we are talking about c45 controllers that can access 
the c22 register space via c45, or controllers which are 
electrically/level shifting to be compatible with c22 voltages/etc?

The nxp hardware in question has 1, 10 and 40Gbit phys on the same MDIO, 
the 1gbit we fall back to c22 registers because it doesn't respond 
correctly to c45 registers. Which is AFAIK what the bit0 C22 regs bit is 
for..

The general logic right now for a C45_FIRST is attempt to detect a c45 
phy and if nothing is detected fall back and attempt c22 register 
access. That is whats picking up the 1G phys. If for whatever reason the 
MDIO controller can't do the right thing to access the c22 regs, I guess 
there really isn't anything we can do about it.


> 
> Notice carefully the values these end up with - MDIOBUS_C22 = BIT(0),
> MDIOBUS_C45 = BIT(1), MDIOBUS_C45_C22 = BIT(0) | BIT(1).  I suspect
> that was no coincidence in Andrew's suggestion.
>
Russell King (Oracle) May 25, 2020, 10:41 p.m. UTC | #6
On Mon, May 25, 2020 at 05:09:56PM -0500, Jeremy Linton wrote:
> Hi,
> 
> On 5/25/20 3:25 AM, Russell King - ARM Linux admin wrote:
> > On Sun, May 24, 2020 at 11:28:52PM -0500, Jeremy Linton wrote:
> > > Hi,
> > > 
> > > On 5/24/20 9:44 AM, Andrew Lunn wrote:
> > > > > +++ b/include/linux/phy.h
> > > > > @@ -275,6 +275,11 @@ struct mii_bus {
> > > > >    	int reset_delay_us;
> > > > >    	/* RESET GPIO descriptor pointer */
> > > > >    	struct gpio_desc *reset_gpiod;
> > > > > +	/* bus capabilities, used for probing */
> > > > > +	enum {
> > > > > +		MDIOBUS_C22_ONLY = 0,
> > > > > +		MDIOBUS_C45_FIRST,
> > > > > +	} probe_capabilities;
> > > > >    };
> > > > 
> > > > 
> > > > I'm not so keen on _FIRST. It suggest _LAST would also be valid.  But
> > > > that then suggests this is not a bus property, but a PHY property, and
> > > > some PHYs might need _FIRST and other phys need _LAST, and then you
> > > > have a bus which has both sorts of PHY on it, and you have a problem.
> > > > 
> > > > So i think it would be better to have
> > > > 
> > > > 	enum {
> > > > 		MDIOBUS_UNKNOWN = 0,
> > > > 		MDIOBUS_C22,
> > > > 		MDIOBUS_C45,
> > > > 		MDIOBUS_C45_C22,
> > > > 	} bus_capabilities;
> > > > 
> > > > Describe just what the bus master can support.
> > > 
> > > Yes, the naming is reasonable and I will update it in the next patch. I went
> > > around a bit myself with this naming early on, and the problem I saw was
> > > that a C45 capable master, can have C45 electrical phy's that only respond
> > > to c22 requests (AFAIK).
> > 
> > If you have a master that can only generate clause 45 cycles, and
> > someone is daft enough to connect a clause 22 only PHY to it, the
> > result is hardware that doesn't work - there's no getting around
> > that.  The MDIO interface can't generate the appropriate cycles to
> > access the clause 22 PHY.  So, this is not something we need care
> > about.
> > 
> > > So the MDIOBUS_C45 (I think I was calling it
> > > C45_ONLY) is an invalid selection. Not, that it wouldn't be helpful to have
> > > a C45_ONLY case, but that the assumption is that you wouldn't try and probe
> > > c22 registers, which I thought was a mistake.
> > 
> > MDIOBUS_C45 means "I can generate clause 45 cycles".
> > MDIOBUS_C22 means "I can generate clause 22 cycles".
> > MDIOBUS_C45_C22 means "I can generate both clause 45 and clause 22
> > cycles."
> 
> Hi, to be clear, we are talking about c45 controllers that can access the
> c22 register space via c45, or controllers which are electrically/level
> shifting to be compatible with c22 voltages/etc?

To me, Andrew was quite clear that these flags should describe what
the MDIO controller can support, and what I understand from Andrew's
email is:

If it can't support clause 45 accesses, then it should not advertise
MDIOBUS_C45 nor MDIOBUS_C45_C22.

If it can't support clause 22 accesses, then it should not advertise
MDIOBUS_C22.

And that's all there is to it.

If you want to talk about clause 45 access via the clause 22 register
set, that is a property of the PHY, not of the MDIO controller, and
the MDIO controller has no business attempting to describe that
property in any shape or form since it is a PHY property.

If you have access to clause 22 registers, then you likely have the
clause 22 ID registers, and the way phylib currently works, that will
also match a PHY driver.

Once we have a PHY and a driver bound, then even if it is a C45
driver, accesses using the phy_*_mmd() functions will _automatically_
switch to using C22 cycles to the indirect C45 access registers if
the PHY has not been detected as a C45 PHY (phydev->is_c45 is false.)

So, I'm coming to the conclusion that you're making way more work
here than is necessary, your changes to gratuitously change the way
stuff in phylib work which is not risk-free are completely unnecessary
and really not a risk worth taking when it's likely that we already
have the code mostly in place to be able to support your PHY.

I think there are some questionable uses of phydev->is_c45 that
your point about accessing C45 PHYs through C22 indirect registers
brings up which need to be resolved, but I really don't think that's
a completely separate issue.
diff mbox series

Patch

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 7a4eb3f2cb74..3ab618e15f2c 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -732,10 +732,15 @@  EXPORT_SYMBOL(mdiobus_free);
  */
 struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr)
 {
-	struct phy_device *phydev;
+	struct phy_device *phydev = ERR_PTR(-ENODEV);
 	int err;
 
-	phydev = get_phy_device(bus, addr, false);
+	if (bus->probe_capabilities == MDIOBUS_C45_FIRST)
+		phydev = get_phy_device(bus, addr, true);
+
+	if (IS_ERR(phydev))
+		phydev = get_phy_device(bus, addr, false);
+
 	if (IS_ERR(phydev))
 		return phydev;
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 480a6b153227..d68e1ebab484 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -275,6 +275,11 @@  struct mii_bus {
 	int reset_delay_us;
 	/* RESET GPIO descriptor pointer */
 	struct gpio_desc *reset_gpiod;
+	/* bus capabilities, used for probing */
+	enum {
+		MDIOBUS_C22_ONLY = 0,
+		MDIOBUS_C45_FIRST,
+	} probe_capabilities;
 };
 #define to_mii_bus(d) container_of(d, struct mii_bus, dev)