From: Stas Sergeev <stsp@list•ru>
To: Florian Fainelli <f.fainelli@gmail•com>, netdev <netdev@vger•kernel.org>
Cc: Linux kernel <linux-kernel@vger•kernel.org>,
Sebastien Rannou <mxs@sbrk•org>,
Arnaud Ebalard <arno@natisbad•org>,
Stas Sergeev <stsp@users•sourceforge.net>
Subject: Re: [PATCH 1/3] fixed_phy: handle link-down case
Date: Fri, 17 Jul 2015 23:03:15 +0300 [thread overview]
Message-ID: <55A95F83.8010900@list.ru> (raw)
In-Reply-To: <55A94E5A.9010104@gmail.com>
17.07.2015 21:50, Florian Fainelli пишет:
> On 17/07/15 04:26, Stas Sergeev wrote:
>> 17.07.2015 02:25, Florian Fainelli пишет:
>>> On 16/07/15 07:50, Stas Sergeev wrote:
>>>> Currently fixed_phy driver recognizes only the link-up state.
>>>> This simple patch adds an implementation of link-down state.
>>>> It fixes the status registers when link is down, and also allows
>>>> to register the fixed-phy with link down without specifying the speed.
>>> This patch still breaks my setups here, e.g: drivers/net/dsa/bcm_sf2.c,
>>> but I will look into it.
>>>
>>> Do we really need this for now for your two other patches to work
>>> properly, or is it just nicer to have?
>> Yes, absolutely.
>> Otherwise registering fixed phy will return -EINVAL
>> because of the missing link speed (even though the link
>> is down).
> Ok, I see the problem that you have now. Arguably you could say that
> according to the fixed-link binding, speed needs to be specified and the
> code correctly errors out with such an error if you do not specify it. I
Aren't you missing the fact that .link=0?
I think what you say is true only for the link-up case, no?
.speed==0 is valid for link-down IMHO: no link - zero speed.
> So is different is that I use a link_update callback, and so we rely on
> at least one call of this function to initialize the hardware in
> drivers/net/dsa/bcm_sf2.c
Do you mean this?:
core_writel(priv, reg, CORE_STS_OVERRIDE_GMIIP_PORT(port));
Maybe just moving the HW initialization bits to some init func
will be a quick fix?
> for this to work, after that, the hardware
> reflects the fixed link parameters we configured, and we feed the
> fixed_phy_status information from the hardware directly.
>
> >From there I see two different ways to fix this:
>
> - we ignore the fixed_phy_update_regs return value in fixed_phy_add(),
> but that will make us avoid doing verification on the speed, which is
> not so great, but is essentially what your patch does anyway
No, it does not. All it does is to allow no speed _when link is down_,
which is IMHO a very logical fix. The speed checks for the link-up
case are all still there.
> - we update the use of the fixed PHY link_update in drivers using it
IMHO just 2 drivers: bcmii.c and bcm_sf2.c, and the change
is likely trivial, although of course I am not sure in details.
> and
> convert them to use fixed_phy_update_state instead, which can take some
> time and effort to convert
Maybe just move the initialization bits out of the link_update
callback, but still use the callback for now? Should be simple, no?
next prev parent reply other threads:[~2015-07-17 20:03 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-16 14:49 [PATCH v4 0/3] net: enable inband link state negotiation only when explicitly requested Stas Sergeev
2015-07-16 14:50 ` [PATCH 1/3] fixed_phy: handle link-down case Stas Sergeev
2015-07-16 23:25 ` Florian Fainelli
2015-07-17 11:26 ` Stas Sergeev
2015-07-17 18:50 ` Florian Fainelli
2015-07-17 20:03 ` Stas Sergeev [this message]
2015-07-17 22:01 ` Florian Fainelli
2015-07-17 23:24 ` Stas Sergeev
2015-07-17 23:35 ` Florian Fainelli
2015-07-17 23:53 ` Stas Sergeev
2015-07-18 2:29 ` Florian Fainelli
2015-07-18 21:16 ` Stas Sergeev
[not found] ` <55A7C45F.1070501-cmBhpYW9OiY@public.gmane.org>
2015-07-16 14:52 ` [PATCH 2/3] of_mdio: add new DT property 'managed' to specify the PHY management type Stas Sergeev
2015-07-16 14:53 ` [PATCH 3/3] mvneta: use inband status only when explicitly enabled Stas Sergeev
-- strict thread matches above, loose matches on Subject: below --
2015-07-14 17:09 [PATCH v3 0/3] net: enable inband link state negotiation only when explicitly requested Stas Sergeev
2015-07-14 17:11 ` [PATCH 1/3] fixed_phy: handle link-down case Stas Sergeev
2015-07-14 18:28 ` Florian Fainelli
2015-07-10 16:38 [PATCH v2 0/2] net: enable inband link state negotiation only when explicitly requested Stas Sergeev
2015-07-10 16:41 ` [PATCH 1/3] fixed_phy: handle link-down case Stas Sergeev
2015-07-10 20:44 ` Florian Fainelli
2015-07-10 21:14 ` Stas Sergeev
2015-07-11 0:15 ` Florian Fainelli
2015-07-11 8:58 ` Stas Sergeev
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=55A95F83.8010900@list.ru \
--to=stsp@list$(echo .)ru \
--cc=arno@natisbad$(echo .)org \
--cc=f.fainelli@gmail$(echo .)com \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=mxs@sbrk$(echo .)org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=stsp@users$(echo .)sourceforge.net \
/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