From: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
To: Gerlando Falauto
<gerlando.falauto-SkAbAL50j+5BDgjK7y7TUQ@public.gmane.org>
Cc: Matthew Garrett
<matthew.garrett-05XSO3Yj/JvQT0dZR+AlfA@public.gmane.org>,
netdev <netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
"devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
<devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
"linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
<linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
Kishon Vijay Abraham I <kishon-l0cyMroinI0@public.gmane.org>
Subject: Re: [PATCH V3] net/dt: Add support for overriding phy configuration from device tree
Date: Tue, 11 Feb 2014 09:43:57 -0800 [thread overview]
Message-ID: <CAGVrzca7CV1SM0PfV2o=02Zd8z-65002kq6e-fhG6gWdkCadGA@mail.gmail.com> (raw)
In-Reply-To: <52F9E8E6.1090006-SkAbAL50j+5BDgjK7y7TUQ@public.gmane.org>
Hi Gerlando,
2014-02-11 1:09 GMT-08:00 Gerlando Falauto <gerlando.falauto-SkAbAL50j+6IwRZHo2/mJg@public.gmane.orgm>:
> Hi Florian,
>
> first of all, thank you for your answer.
>
>
> On 02/10/2014 06:09 PM, Florian Fainelli wrote:
>>
>> Hi Gerlando,
>>
>> Le lundi 10 février 2014, 17:14:59 Gerlando Falauto a écrit :
>>>
>>> Hi,
>>>
>>> I'm currently trying to fix an issue for which this patch provides a
>>> partial solution, so apologies in advance for jumping into the
>>> discussion for my own purposes...
>>>
>>> On 02/04/2014 09:39 PM, Florian Fainelli wrote:> 2014-01-17 Matthew
>>>
>>> Garrett <matthew.garrett-05XSO3Yj/JvQT0dZR+AlfA@public.gmane.org>:
>>> >> Some hardware may be broken in interesting and board-specific ways,
>>> such
>>> >> that various bits of functionality don't work. This patch provides a
>>> >> mechanism for overriding mii registers during init based on the
>>>
>>> contents of
>>>
>>> >> the device tree data, allowing board-specific fixups without having
>>> to
>>> >> pollute generic code.
>>> >
>>> > It would be good to explain exactly how your hardware is broken
>>> > exactly. I really do not think that such a fine-grained setting where
>>> > you could disable, e.g: 100BaseT_Full, but allow 100BaseT_Half to
>>> > remain usable makes that much sense. In general, Gigabit might be
>>> > badly broken, but 100 and 10Mbits/sec should work fine. How about the
>>> > MASTER-SLAVE bit, is overriding it really required?
>>> >
>>> > Is not a PHY fixup registered for a specific OUI the solution you are
>>> > looking for? I am also concerned that this creates PHY
>>> troubleshooting
>>> > issues much harder to debug than before as we may have no idea about
>>> > how much information has been put in Device Tree to override that.
>>> >
>>> > Finally, how about making this more general just like the BCM87xx PHY
>>> > driver, which is supplied value/reg pairs directly? There are 16
>>> > common MII registers, and 16 others for vendor specific registers,
>>> > this is just covering for about 2% of the possible changes.
>>>
>>> Good point. That would easily help me with my current issue, which
>>> requires autoneg to be disabled to begin with (by clearing BMCR_ANENABLE
>>> from register 0).
>>
>>
>> Is there a point in time (e.g: after some specific initial configuration
>> has
>> been made) where BMCR_ANENABLE can be used?
>
>
> What do you mean? In my case, for some HW-related reason (due to the PHY
> counterpart I guess) autoneg needs to be disabled.
> This is currently done by the bootloader code (which clears the bit).
> What I'm looking for is some way for the kernel to either reinforce this
> setting, or just take that into account and skip autoneg.
> On top of that, there's a HW errata about that particular PHY, which
> requires certain operations to be performed on the PHY as a workaround *WHEN
> AUTONEG IS DISABLED*. That I'd implement on a PHY-specif driver.
Ok.
>
>
>>> This would not however fix it entirely (I tried a quick hardwired
>>> implementation), as the whole PHY machinery would not take that into
>>> account and would re-enable autoneg anyway.
>>> I also tried changing the patch so that phydev->support gets updated
>>
>>
>> There are multiple things that you could try doing here:
>>
>> - override the PHY state machine in your read_status callback to make sure
>> that you always set phydev->autoneg set to AUTONEG_ENABLE
>
>
> [you mean AUTONEG_DISABLE, right?]
Right, I fat fingered here.
> Uhm, but I don't want to implement a driver for that PHY that always
> disables autoneg. I only want to disable autoneg for that particular board.
> I figure I might register a fixup for that board, but that kindof makes
> everything more complicated and less clear. Plus, what should be the
> criterion to determine whether we're running on that particular hardware?
of_machine_is_compatible() plus reading the specific PHY OUI should
provide you with with an unique machine + PHY tuple. If your machine
name is too generic.
>
>
>> - clear the SUPPORTED_Autoneg bits from phydev->supported right after PHY
>> registration and before the call to phy_start()
>
>
> I actually tried clearing it by tweaking the patch on this thread, but the
> end result is that it does not produce any effect (see further comments
> below). Only thing that seems to play a role here is explictly setting
> phydev->autoneg = AUTONEG_DISABLE.
>
>
>> - set the PHY_HAS_MAGICANEG bit in your PHY driver flag
>
>
> Again, this seems to play no role whatsoever here:
>
> } else if (0 == phydev->link_timeout--) {
> needs_aneg = 1;
> /* If we have the magic_aneg bit,
> * we try again */
> if (phydev->drv->flags & PHY_HAS_MAGICANEG)
> break;
> }
> break;
> case PHY_NOLINK:
>
> This code might have made sense when it was written in 2006 -- back then,
> the break statement was skipping some fallback code. But now it seems to do
> nothing.
>
>
>>
>>>
>>> (instead of phydev->advertising):
>>> >> + if (!of_property_read_u32(np, override->prop, &tmp))
>>> {
>>> >> + if (tmp) {
>>> >> + *val |= override->value;
>>> >> + phydev->advertising |=
>>>
>>> override->supported;
>>>
>>> >> + } else {
>>> >> + phydev->advertising &=
>>>
>>> ~(override->supported);
>>>
>>> >> + }
>>> >> +
>>> >> + *mask |= override->value;
>>>
>>> What I find weird is that the only way phydev->autoneg could ever be set
>>> to disabled is from here (phy.c):
>>>
>>> static void phy_sanitize_settings(struct phy_device *phydev)
>>> {
>>> u32 features = phydev->supported;
>>> int idx;
>>>
>>> /* Sanitize settings based on PHY capabilities */
>>> if ((features & SUPPORTED_Autoneg) == 0)
>>> phydev->autoneg = AUTONEG_DISABLE;
>>>
>>> which is in turn only called when phydev->autoneg is set to
>>> AUTONEG_DISABLE to begin with:
>>>
>>> int phy_start_aneg(struct phy_device *phydev)
>>> {
>>> int err;
>>>
>>> mutex_lock(&phydev->lock);
>>>
>>> if (AUTONEG_DISABLE == phydev->autoneg)
>>> phy_sanitize_settings(phydev);
>>>
>>> So could someone please help me figure out what I'm missing here?
>>
>>
>> At first glance it looks like the PHY driver should be reading the phydev-
>>>
>>> autoneg value when the PHY driver config_aneg() callback is called to be
>>
>> allowed to set the forced speed and settings.
>>
>> The way phy_sanitize_settings() is coded does not make it return a mask of
>> features, but only the forced supported speed and duplex. Then when the
>> link
>> is forced but we are having some issues getting a link status, libphy
>> tries
>> lower speeds and this function is used again to provide the next
>> speed/duplex
>> pair to try.
>>
>
> What I was trying to say is that phy_sanitize_settings() is only called when
> phydev->autoneg == AUTONEG_DISABLE, and in turn it's the only generic
> function setting phydev->autoneg = AUTONEG_DISABLE.
> So perhaps the condition should read:
>
> - if (AUTONEG_DISABLE == phydev->autoneg)
> + if ((features & SUPPORTED_Autoneg) == 0)
> phy_sanitize_settings(phydev);
>
> Or else, some other parts of the generic code should take care of setting it
> to AUTONEG_DISABLE, depending on whether the feature is supported or not.
> What I found weird is explicitly setting a value (phydev->autoneg =
> AUTONEG_DISABLE), from a static function which is only called when that
> condition is already true.
I do not think that this change is correct either, let me cook a patch
for you to allow disabling autoneg from the start.
>
> BTW, I feel like disabling autoneg from the start has never been a use case
> before, am I right?
Not really no, and that is because most hardware does not need quirks
to work correctly.
>
> Thanks!
> Gerlando
--
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
next prev parent reply other threads:[~2014-02-11 17:43 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-01-15 21:38 [PATCH] net/dt: Add support for overriding phy configuration from device tree Matthew Garrett
2014-01-16 13:59 ` Gerhard Sittig
2014-01-16 14:40 ` Matthew Garrett
[not found] ` <20140116135905.GV20094-kDjWylLy9wD0K7fsECOQyeGNnDKD8DIp@public.gmane.org>
2014-01-16 14:47 ` [PATCH V2] " Matthew Garrett
[not found] ` <1389883631-1480-1-git-send-email-matthew.garrett-05XSO3Yj/JvQT0dZR+AlfA@public.gmane.org>
2014-01-16 15:16 ` Arnd Bergmann
[not found] ` < 1389999459-9483-1-git-send-email-matthew.garrett@nebula.com>
2014-01-17 22:57 ` [PATCH V3] " Matthew Garrett
2014-01-19 15:34 ` Ben Hutchings
[not found] ` <1390145654.16433.102.camel-nDn/Rdv9kqW9Jme8/bJn5UCKIB8iOfG2tUK59QYPAWc@public.gmane.org>
2014-02-04 19:01 ` Florian Fainelli
2014-02-04 17:15 ` Grant Likely
2014-02-04 20:39 ` Florian Fainelli
2014-02-04 21:40 ` Ben Hutchings
2014-02-04 22:48 ` Florian Fainelli
2014-02-05 9:47 ` Grant Likely
2014-02-05 9:51 ` David Laight
[not found] ` <063D6719AE5E284EB5DD2968C1650D6D0F6B8BCA-VkEWCZq2GCInGFn1LkZF6NBPR1lH4CV8@public.gmane.org>
2014-02-07 22:43 ` Florian Fainelli
2014-02-10 16:14 ` Gerlando Falauto
[not found] ` <52F8FB03.6040606-SkAbAL50j+5BDgjK7y7TUQ@public.gmane.org>
2014-02-10 17:09 ` Florian Fainelli
2014-02-11 9:09 ` Gerlando Falauto
[not found] ` <52F9E8E6.1090006-SkAbAL50j+5BDgjK7y7TUQ@public.gmane.org>
2014-02-11 17:43 ` Florian Fainelli [this message]
2014-02-12 8:57 ` Gerlando Falauto
2014-07-10 12:37 ` Gerlando Falauto
[not found] ` <53BE8912.4090804-SkAbAL50j+5BDgjK7y7TUQ@public.gmane.org>
2014-07-22 23:40 ` Florian Fainelli
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='CAGVrzca7CV1SM0PfV2o=02Zd8z-65002kq6e-fhG6gWdkCadGA@mail.gmail.com' \
--to=f.fainelli-re5jqeeqqe8avxtiumwx3w@public.gmane.org \
--cc=devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=gerlando.falauto-SkAbAL50j+5BDgjK7y7TUQ@public.gmane.org \
--cc=kishon-l0cyMroinI0@public.gmane.org \
--cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=matthew.garrett-05XSO3Yj/JvQT0dZR+AlfA@public.gmane.org \
--cc=netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
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;
as well as URLs for NNTP newsgroup(s).