Re: Commit-8b63ec18 breaks setting PHY pad-skew settings

Re: Commit-8b63ec18 breaks setting PHY pad-skew settings

[Florian Fainelli]

On 21/12/15 02:40, Roosen Henri wrote:
> Hi David,
> 
> Unfortunately Linux kernel commit-8b63ec1837fa4c1ea604b278d201454eb3b85066 breaks setting pad-skew settings for Micrel PHY's.
> 
> Please see Documentation/devicetree/bindings/net/micrel-ksz90x1.txt: at the examples, it proposes to do PHY pad skew settings at the Ethernet device for autodetected PHY's.
> 
> Multiple boards are using this at the devicetree files: see imx6qdl-nitrogen6x.dtsi, imx6qdl-sabrelite.dtsi, socfpga_arria5_socdk.dts, socfpga_cyclone5_socdk.dts and socfpga_cyclone5_sockit.dts. But of course there might be more users depending on this than the ones which have an in-kernel dts file.
> 
> The micrel.c file searches for the parent of its OpenFirmware node for the skew settings: see ksz9031_config_init(): of_node = dev->parent->of_node; I don't think that was a clean implementation to start with.. But for sure it's incompatible with commit-8b63ec18, because the node of the Ethernet device which has the settings is not found anymore.
> 
> I'm not sure who to put on the copy list to discuss a proper solution, so feel free to get these persons and lists in the loop.

Usually, the best thing is to reply on the mailing-list directive, so
more people can jump in.

This particular issue is fixed with the following commit:

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=651df2183543bc92f5dbcf99cd9e236ead0bc4c5
-- 
Florian

AW: Commit-8b63ec18 breaks setting PHY pad-skew settings

[Roosen Henri]

> -----Ursprüngliche Nachricht-----
> Von: Florian Fainelli [mailto:f.fainelli@gmail.com]
> Gesendet: Montag, 21. Dezember 2015 20:51
> An: Roosen Henri; david.daney@cavium.com
> Cc: buytenh@wantstofly.org; grant.likely@secretlab.ca;
> davem@davemloft.net; netdev@vger.kernel.org; Andrew Lunn
> Betreff: Re: Commit-8b63ec18 breaks setting PHY pad-skew settings
>
> On 21/12/15 02:40, Roosen Henri wrote:
> > Hi David,
> >
> > Unfortunately Linux kernel commit-
> 8b63ec1837fa4c1ea604b278d201454eb3b85066 breaks setting pad-skew
> settings for Micrel PHY's.
> >
> > Please see Documentation/devicetree/bindings/net/micrel-ksz90x1.txt: at
> the examples, it proposes to do PHY pad skew settings at the Ethernet
> device for autodetected PHY's.
> >
> > Multiple boards are using this at the devicetree files: see imx6qdl-
> nitrogen6x.dtsi, imx6qdl-sabrelite.dtsi, socfpga_arria5_socdk.dts,
> socfpga_cyclone5_socdk.dts and socfpga_cyclone5_sockit.dts. But of course
> there might be more users depending on this than the ones which have an
> in-kernel dts file.
> >
> > The micrel.c file searches for the parent of its OpenFirmware node for the
> skew settings: see ksz9031_config_init(): of_node = dev->parent->of_node;
> I don't think that was a clean implementation to start with.. But for sure it's
> incompatible with commit-8b63ec18, because the node of the Ethernet
> device which has the settings is not found anymore.
> >
> > I'm not sure who to put on the copy list to discuss a proper solution, so feel
> free to get these persons and lists in the loop.
>
> Usually, the best thing is to reply on the mailing-list directive, so more people
> can jump in.

Thanks Florian for your reply and getting the mailing-list into the loop!

>
> This particular issue is fixed with the following commit:
>
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=651
> df2183543bc92f5dbcf99cd9e236ead0bc4c5

Unfortunately the patch is incomplete: it only fixes the issue for the KSZ9021. Please have a look at the funcion ksz9031_config_init() which has the the same (broken) search for the parent node.

As both the code for KSZ9021 as well as the code for KSZ9031 parse the same pad skew settings from the OF node (and to prevent incomplete fixes in the future), I think this code is a candidate to be cleaned up. Or is cooking up a similar patch for the KSZ9031 as for the KSZ9021 preferred?

Additionally, if this is a deprecated feature (like the commit states), then the devicetree binding documentation (Documentation/devicetree/bindings/net/micrel-ksz90x1.txt) should be updated. At least the deprecated example for autodetected PHY's should be corrected/removed.

--
Henri
> --
> Florian

________________________________

Ginzinger electronic systems GmbH
Gewerbegebiet Pirath 16
4952 Weng im Innkreis
www.ginzinger.com

Firmenbuchnummer: FN 364958d
Firmenbuchgericht: Ried im Innkreis
UID-Nr.: ATU66521089

________________________________
*** WEIHNACHTSURLAUB VON DONNERSTAG, DEN 24. DEZEMBER 2015 BIS MITTWOCH, DEN 6. JAENNER 2016 ***
*** CHRISTMAS VACATION FROM THURSDAY, THE 24 DECEMBER 2015 TO WEDNESDAY, 6 JANUARY 2016 ***

Re: Commit-8b63ec18 breaks setting PHY pad-skew settings

[Andrew Lunn]

> Unfortunately the patch is incomplete: it only fixes the issue for
> the KSZ9021. Please have a look at the funcion ksz9031_config_init()
> which has the the same (broken) search for the parent node.

Ah, sorry missed that. I was a bit blinkered, the issue was report for
KSZ9021 and i did not look further.

> As both the code for KSZ9021 as well as the code for KSZ9031 parse
> the same pad skew settings from the OF node (and to prevent
> incomplete fixes in the future), I think this code is a candidate to
> be cleaned up. Or is cooking up a similar patch for the KSZ9031 as
> for the KSZ9021 preferred?

Both would be best. We probably need a minimal fix we can get included
fast, and then having a cleanup patch merged during the next merge
window would be nice.

> Additionally, if this is a deprecated feature (like the commit
> states), then the devicetree binding documentation
> (Documentation/devicetree/bindings/net/micrel-ksz90x1.txt) should be
> updated. At least the deprecated example for autodetected PHY's
> should be corrected/removed.

I submitted a patch for that already.
https://www.mail-archive.com/netdev@vger.kernel.org/msg90932.html

It however does not seem to of made its way into net-next. If you
agree with it, please send an reviewed-by, or an Acked-by...

      Andrew