From: Sergei Shtylyov <sergei.shtylyov@cogentembedded•com>
To: David Miller <davem@davemloft•net>
Cc: nobuhiro.iwamatsu.yj@renesas•com, netdev@vger•kernel.org,
linux-sh@vger•kernel.org,
laurent.pinchart+renesas@ideasonboard•com
Subject: Re: [PATCH] sh_eth: call phy_scan_fixups() after PHY reset
Date: Tue, 05 Nov 2013 23:18:10 +0300 [thread overview]
Message-ID: <52795282.20708@cogentembedded.com> (raw)
In-Reply-To: <5271712A.6020105@cogentembedded.com>
Hello.
On 10/30/2013 11:50 PM, Sergei Shtylyov wrote:
>>>> Sometimes the PHY reset that sh_eth_phy_start() does effects the PHY
>>>> registers
>>>> registers values of which are vital for the correct functioning of the
>>>> driver.
>>>> Unfortunately, the existing PHY platform fixup mechanism doesn't help
>>>> here as
>>>> it only hooks PHY resets done by ioctl() calls. Calling phy_scan_fixups()
>>>> from
>>>> the driver helps here. With a proper platform fixup, this fixes NFS
>>>> timeouts on
>>>> the SH-Mobile Lager board.
> Timeouts happen because of the sideband ETH_LINK signal connected to PHY's
> LED0 pin -- it bounces on/off after each packet in the default LED mode and
> that seems to hinder packet sending and/or reception...
>> "And sets the PHY LED pins to the desired mode", I should have added.
>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded•com>
>>> The PHY layer is designed to naturally already take care of this kind of
>>> thing. I think that part of the problem is that you're fighting the
>>> natural control flow the PHY layer provides.
>>> When the phy_connect() is performed, what we end up doing is calling
>>> phy_attach_direct() which invokes the ->probe() method of the driver
>>> and then afterwards we do phy_init_hw() which takes care of doing
>>> the fixup calls.
>> Yes, I have studied the code paths beforehand.
>>> So if you really need to do a BMCR reset then run the fixups I'd like
>>> you to look into making that happen within the provided control
>>> flow rather than with an exceptional explicit call to run the fixups.
>> That could change the behavior of many Ethernet drivers in sometimes
>> unpredictable ways I think (due to extended registers the PHYs sometimes have,
>> like in this case) if you meant including the PHY reset into phylib control
>> flows. Anyway, that would have required more complex patches only good for
>> merging at the merge window time while I aimed at a quick fix for a problem at
>> hand (which is NFS timeout/slowdown and LED mode mismatch to what was designed
>> for the board).
>> Some other drivers also do reset the PHYs but usually that's accompanied
>> by a loop polling for reset completion, so a naive code like that one on the
>> phylib's ioctl() path couldn't have helped if I wanted to hook reset writes in
>> the same fashion in phy_write(). In my case reset seems just quick enough for
>> the extended PHY register reads/writes to work correctly without polling the
>> reset bit first...
>> That's why I took an easy way and used already exported phy_scan_fixups()
>> to undo what the PHY reset did to some of the PHY's registers. The question is
>> why it was exported in the first place? It doesn't seem to be used by anything
>> but phylib internally...
> Well, how about I create phy_reset() function (that will care about BMCR
> polling and calling PHY driver/fixups) that those drivers that currently do
> reset their PHYs can call (instead of open coding BMCR reset)? That way it
> seems to be less invasive than embedding PHY reset into phylib's control flow...
>>> I'm willing to be convinced that this is a better or necessary approach
>>> but you'll need to explain it to me.
David, will you ever reply to this mail?
WBR, Sergei
next prev parent reply other threads:[~2013-11-05 20:18 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-09-14 0:10 [PATCH] sh_eth: call phy_scan_fixups() after PHY reset Sergei Shtylyov
2013-09-17 19:44 ` David Miller
2013-09-21 0:39 ` Sergei Shtylyov
2013-10-30 20:50 ` Sergei Shtylyov
2013-11-05 20:18 ` Sergei Shtylyov [this message]
2013-11-05 20:01 ` David Miller
2013-11-16 0:46 ` Sergei Shtylyov
2013-11-16 0:04 ` Florian Fainelli
2013-11-16 2:00 ` David Miller
2013-11-21 17:48 ` Sergei Shtylyov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=52795282.20708@cogentembedded.com \
--to=sergei.shtylyov@cogentembedded$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=laurent.pinchart+renesas@ideasonboard$(echo .)com \
--cc=linux-sh@vger$(echo .)kernel.org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=nobuhiro.iwamatsu.yj@renesas$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox