Re: [BISECTED] v4.5-rc1 phylib regression

Re: [BISECTED] v4.5-rc1 phylib regression

[Olof Johansson]

+devicetree@vger.kernel.org instead.

On Tue, Jan 26, 2016 at 10:08 AM, Olof Johansson <olof@lixom.net> wrote:
> On Tue, Jan 26, 2016 at 9:53 AM, Andrew Lunn <andrew@lunn.ch> wrote:
>>> I hate to bikeshed, but I'm not sure if "generic-mdio" is too...
>>> generic? Will someone writing a DT be thinking "well, this is a
>>> generic mdio PHY, I should set it"?  "mdio-device"?
>>> "generic-nonphy-mdio"? Neither of those seem much better.
>>
>> How about 'not-a-phy'?
>
> "mdio,not-a-phy" or "mdio,non-phy" will scope it a bit, especially if
> you expect other generic mdio properties that can do with a namespace.
>
> Probably time to add devicetree-discuss. Doing so.
>
>
> -Olof

Re: [BISECTED] v4.5-rc1 phylib regression

[Florian Fainelli]

On 26/01/16 10:14, Olof Johansson wrote:
> +devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org instead.
> 
> On Tue, Jan 26, 2016 at 10:08 AM, Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org> wrote:
>> On Tue, Jan 26, 2016 at 9:53 AM, Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org> wrote:
>>>> I hate to bikeshed, but I'm not sure if "generic-mdio" is too...
>>>> generic? Will someone writing a DT be thinking "well, this is a
>>>> generic mdio PHY, I should set it"?  "mdio-device"?
>>>> "generic-nonphy-mdio"? Neither of those seem much better.
>>>
>>> How about 'not-a-phy'?
>>
>> "mdio,not-a-phy" or "mdio,non-phy" will scope it a bit, especially if
>> you expect other generic mdio properties that can do with a namespace.

Really not a fan of having to add an additional boolean property to
differentiate an Ethernet PHY from something else, the proper solution
would really be to enforce the use of the c22 or c45 compatible string
as the least compatible string to be used, but I am assuming this is not
necessarily an option here with DTBs out there.

What plays in favor of this boolean property is that the very concept of
MDIO devices has been recently introduced, so presumably, there are not
that many DTBs out there which would be affected...

The only other idea I had was to force the MDIO device creation to be
dependent on finding a matching compatible string provided by a driver
(yikes).
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BISECTED] v4.5-rc1 phylib regression

[Andrew Lunn]

On Tue, Jan 26, 2016 at 12:46:31PM -0800, Florian Fainelli wrote:
> On 26/01/16 10:14, Olof Johansson wrote:
> > +devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org instead.
> > 
> > On Tue, Jan 26, 2016 at 10:08 AM, Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org> wrote:
> >> On Tue, Jan 26, 2016 at 9:53 AM, Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org> wrote:
> >>>> I hate to bikeshed, but I'm not sure if "generic-mdio" is too...
> >>>> generic? Will someone writing a DT be thinking "well, this is a
> >>>> generic mdio PHY, I should set it"?  "mdio-device"?
> >>>> "generic-nonphy-mdio"? Neither of those seem much better.
> >>>
> >>> How about 'not-a-phy'?
> >>
> >> "mdio,not-a-phy" or "mdio,non-phy" will scope it a bit, especially if
> >> you expect other generic mdio properties that can do with a namespace.
> 
> Really not a fan of having to add an additional boolean property to
> differentiate an Ethernet PHY from something else, the proper solution
> would really be to enforce the use of the c22 or c45 compatible string
> as the least compatible string to be used, but I am assuming this is not
> necessarily an option here with DTBs out there.

Nope, not an option. Only a small number of DTB actually use c22 or
c45. The majority of devices have no compatible at all. Why should
they, the binding documentation says it is optional!
 
> What plays in favor of this boolean property is that the very concept of
> MDIO devices has been recently introduced, so presumably, there are not
> that many DTBs out there which would be affected...

In kernel, 0. I have one out of kernel, which i hope to contribute
once we decide on the new binding for DSA.

> The only other idea I had was to force the MDIO device creation to be
> dependent on finding a matching compatible string provided by a driver
> (yikes).

Complex. There are ordering issues, since the driver can be loaded a
long time after of_mdiobus_register() is called, yet it needs to be
of_mdiobus_register() which decides if a device is a PHY or not.

I think the bool is the only practical solution.

  Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BISECTED] v4.5-rc1 phylib regression

[Olof Johansson]

On Tue, Jan 26, 2016 at 1:36 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> On Tue, Jan 26, 2016 at 12:46:31PM -0800, Florian Fainelli wrote:
>> On 26/01/16 10:14, Olof Johansson wrote:
>> > +devicetree@vger.kernel.org instead.
>> >
>> > On Tue, Jan 26, 2016 at 10:08 AM, Olof Johansson <olof@lixom.net> wrote:
>> >> On Tue, Jan 26, 2016 at 9:53 AM, Andrew Lunn <andrew@lunn.ch> wrote:
>> >>>> I hate to bikeshed, but I'm not sure if "generic-mdio" is too...
>> >>>> generic? Will someone writing a DT be thinking "well, this is a
>> >>>> generic mdio PHY, I should set it"?  "mdio-device"?
>> >>>> "generic-nonphy-mdio"? Neither of those seem much better.
>> >>>
>> >>> How about 'not-a-phy'?
>> >>
>> >> "mdio,not-a-phy" or "mdio,non-phy" will scope it a bit, especially if
>> >> you expect other generic mdio properties that can do with a namespace.
>>
>> Really not a fan of having to add an additional boolean property to
>> differentiate an Ethernet PHY from something else, the proper solution
>> would really be to enforce the use of the c22 or c45 compatible string
>> as the least compatible string to be used, but I am assuming this is not
>> necessarily an option here with DTBs out there.
>
> Nope, not an option. Only a small number of DTB actually use c22 or
> c45. The majority of devices have no compatible at all. Why should
> they, the binding documentation says it is optional!

So one thing that can be done is to just have a whitelist in the
driver that we add the known phy compatibles to, with a nice comment
above that this should only be for legacy device trees. I.e. in
addition to the c22 or c45 strings.

You can even fix up existing trees, and do a pr_warn() for these cases
to ask people to update their DT. There's no strict requirement to do
so though, so the kernel still *has* to work with the old ones.

>> What plays in favor of this boolean property is that the very concept of
>> MDIO devices has been recently introduced, so presumably, there are not
>> that many DTBs out there which would be affected...
>
> In kernel, 0. I have one out of kernel, which i hope to contribute
> once we decide on the new binding for DSA.
>
>> The only other idea I had was to force the MDIO device creation to be
>> dependent on finding a matching compatible string provided by a driver
>> (yikes).
>
> Complex. There are ordering issues, since the driver can be loaded a
> long time after of_mdiobus_register() is called, yet it needs to be
> of_mdiobus_register() which decides if a device is a PHY or not.
>
> I think the bool is the only practical solution.


-Olof

Re: [BISECTED] v4.5-rc1 phylib regression

[Andrew Lunn]

> > Nope, not an option. Only a small number of DTB actually use c22 or
> > c45. The majority of devices have no compatible at all. Why should
> > they, the binding documentation says it is optional!
> 
> So one thing that can be done is to just have a whitelist in the
> driver that we add the known phy compatibles to, with a nice comment
> above that this should only be for legacy device trees.

So you mean drivers/of/of_mdio.c:of_mdiobus_child_is_phy()
has a white list like:

"brcm,40nm-ephy"
"marvell,88E1111", 
"marvell,88e1116",
"marvell,88e1118",
"marvell,88e1149r",
"marvell,88e1310",
"marvell,88E1510",
"marvell,88E1514",
"moxa,moxart-rtl8201cp",

Yes, that would work.

We should also update the binding documentation to limit what is legal
in the compatible string.

   Andrew

Re: [BISECTED] v4.5-rc1 phylib regression

[Florian Fainelli]

On 26/01/16 14:09, Andrew Lunn wrote:
>>> Nope, not an option. Only a small number of DTB actually use c22 or
>>> c45. The majority of devices have no compatible at all. Why should
>>> they, the binding documentation says it is optional!
>>
>> So one thing that can be done is to just have a whitelist in the
>> driver that we add the known phy compatibles to, with a nice comment
>> above that this should only be for legacy device trees.
> 
> So you mean drivers/of/of_mdio.c:of_mdiobus_child_is_phy()
> has a white list like:
> 
> "brcm,40nm-ephy"
> "marvell,88E1111", 
> "marvell,88e1116",
> "marvell,88e1118",
> "marvell,88e1149r",
> "marvell,88e1310",
> "marvell,88E1510",
> "marvell,88E1514",
> "moxa,moxart-rtl8201cp",
> 
> Yes, that would work.
> 
> We should also update the binding documentation to limit what is legal
> in the compatible string.

Agreed, and while at it, take the opportunity to make the compatible
string clause 22/45 mandatory properties so we do not multiply the
whitelist.

Thanks!
-- 
Florian

Re: [BISECTED] v4.5-rc1 phylib regression

[Andrew Lunn]

On Tue, Jan 26, 2016 at 03:54:13PM -0800, Florian Fainelli wrote:
> On 26/01/16 14:09, Andrew Lunn wrote:
> >>> Nope, not an option. Only a small number of DTB actually use c22 or
> >>> c45. The majority of devices have no compatible at all. Why should
> >>> they, the binding documentation says it is optional!
> >>
> >> So one thing that can be done is to just have a whitelist in the
> >> driver that we add the known phy compatibles to, with a nice comment
> >> above that this should only be for legacy device trees.
> > 
> > So you mean drivers/of/of_mdio.c:of_mdiobus_child_is_phy()
> > has a white list like:
> > 
> > "brcm,40nm-ephy"
> > "marvell,88E1111", 
> > "marvell,88e1116",
> > "marvell,88e1118",
> > "marvell,88e1149r",
> > "marvell,88e1310",
> > "marvell,88E1510",
> > "marvell,88E1514",
> > "moxa,moxart-rtl8201cp",
> > 
> > Yes, that would work.
> > 
> > We should also update the binding documentation to limit what is legal
> > in the compatible string.
> 
> Agreed, and while at it, take the opportunity to make the compatible
> string clause 22/45 mandatory properties so we do not multiply the
> whitelist.

Hi Florian

I just posted the whitelist code.

Making clause 22/45 mandatory is going further than a fix. So i've
left this out for the moment. We can add it to net-next later.

     Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html