Re: [PATCH] Fix genphy_restart_aneg() for Micrel's ksz9031.

From: Guillermo Rodriguez Garcia <guille.rodriguez@gmail.com>
To: Sascha Hauer <s.hauer@pengutronix.de>
Cc: barebox@lists.infradead.org, Philipp Zabel <pza@pengutronix.de>,
	Trent Piepho <tpiepho@kymetacorp.com>
Subject: Re: [PATCH] Fix genphy_restart_aneg() for Micrel's ksz9031.
Date: Wed, 15 Jun 2016 09:44:13 +0200	[thread overview]
Message-ID: <CABDcavZJDGr7h7CQ2f7qZSiZDL-XYhipDSJUihm5VWv3YEoejA@mail.gmail.com> (raw)
In-Reply-To: <20160615054910.GC9677@pengutronix.de>

2016-06-15 7:49 GMT+02:00 Sascha Hauer <s.hauer@pengutronix.de>:
> On Tue, Jun 14, 2016 at 09:39:23AM +0200, Guillermo Rodriguez Garcia wrote:
>> Hi Sascha,
>>
>> 2016-06-14 8:39 GMT+02:00 Sascha Hauer <s.hauer@pengutronix.de>:
>> > Hi Guillermo,
>> >
>> > On Mon, Jun 13, 2016 at 07:29:15PM +0200, Guillermo Rodriguez Garcia wrote:
>> >> From: grodriguez <guille.rodriguez@gmail.com>
>> >>
>> >> Commit da89ee8f2e04 ("Center FLP timing at 16ms") breaks
>> >> genphy_restart_aneg() for Micrel's ksz9031. According to the
>> >> datasheet, the ksz9031 requires a wait of 1ms after clearing
>> >> the PDOWN bit and before read/write access to any PHY registers.
>> >> ---
>> >>  drivers/net/phy/phy.c |    9 ++++++++-
>> >>  1 file changed, 8 insertions(+), 1 deletion(-)
>> >
>> > I must say that I am not overly happy with this patch as it leaks in phy
>> > specific stuff into a somewhat generic function. Anyway, I see the need
>> > for this patch and so I applied it.
>>
>> Yes, I understand perfectly and I feel the same.
>> The alternative was to create a custom genphy_restart_aneg for micrel
>> PHYs only add the delay there, however I am not sure it is worth the
>> trouble just for a 1ms delay which will be virtually unnoticeable
>> anyway.
>
> There's also the case when Micrel phy support is disabled. In this case
> a Micrel phy is then handled by the generic phy, so we still need the
> delay in the generic code, or we would have to remove the BMCR_PDOWN
> setting from the generic code also.

Ah -- I didn't know this was possible.

>>
>> Also I am not completely sure that this only applies to Micrel. See
>> this for example in the SMSC911x driver from the Linux kernel:
>> http://lxr.free-electrons.com/source/drivers/net/ethernet/smsc/smsc911x.c#L1364
>>
>> Anyway if you would prefer this to be moved to micrel.c I would be
>> happy to prepare a patch for that; just let me know.
>
> Your patch is fine for now. Let's see what unforseen problems it
> generates before changing it again ;)

Sounds like a plan :)

Best,

Guillermo Rodriguez Garcia
guille.rodriguez@gmail.com

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox