From: Sean Anderson <sean.anderson@seco.com>
To: Vladimir Oltean <olteanv@gmail.com>
Cc: Andrew Lunn <andrew@lunn.ch>,
Heiner Kallweit <hkallweit1@gmail.com>,
Russell King <linux@armlinux.org.uk>,
netdev@vger.kernel.org, Eric Dumazet <edumazet@google.com>,
Tim Harvey <tharvey@gateworks.com>,
"David S . Miller" <davem@davemloft.net>,
linux-kernel@vger.kernel.org, Paolo Abeni <pabeni@redhat.com>,
Jakub Kicinski <kuba@kernel.org>
Subject: Re: [PATCH] phy: aquantia: Configure SERDES mode by default
Date: Fri, 18 Nov 2022 12:11:30 -0500 [thread overview]
Message-ID: <1015dfec-542d-8222-6c4e-0cf9d5ee7e5a@seco.com> (raw)
In-Reply-To: <20221118164914.6k3gofemf5tu2gfn@skbuf>
On 11/18/22 11:49, Vladimir Oltean wrote:
> On Thu, Nov 17, 2022 at 06:40:02PM -0500, Sean Anderson wrote:
>> > Even if the change works, why would it be a good idea to overwrite some
>> > random registers which are supposed to be configured correctly by the
>> > firmware provided for the board?
>>
>> They're not random registers. They happen to be exactly the same registers
>> we use to determine if rate adaptation is enabled.
>
> As far as I'm concerned, this is just poking in places where there is no
> guarantee that the end result will be a known state.
>
> FWIW, for internal testing of multiple SERDES modes all with the same
> Aquantia firmware, the NXP SDK also has a quick-and-dirty patch to
> change the SERDES protocol on the Aquantia PHY based on device tree:
> https://source.codeaurora.org/external/qoriq/qoriq-components/linux/tree/drivers/net/phy/aquantia_main.c?h=lf-5.15.y#n288
>
> but we decided to not upstream such a thing, specifically because
> it might react in exotic ways with firmware images shipped by Aquantia
> to some of their other customers. I don't work for Aquantia, I am not a
> fan of their model of customizing firmware for everyone, and I don't
> want to have to support the ensuing breakage, I wouldn't have time for
> basically anything else. If you do, I'm not going to stop you. Just be
> prepared to help me too ;)
>
>> > If the Linux fixup works for one board
>> > with one firmware, how do we know it also works for another board with
>> > the same PHY, but different firmware?
>>
>> How do we know if a fix on one board for any hardware works on another board?
>
> If both boards start from the same state X and make the same transition
> T, they end in the same state Y, no? Aquantia PHYs don't all start from
> the same state. Not sure what you'd like me to say.
>
>> Well, part of my goal in sending out this patch is to get some feedback
>> on the right thing to do here. As I see it, there are three ways of
>> configuring this phy:
>>
>> - Always rate adapt to whatever the initial phy interface mode is
>> - Switch phy interfaces depending on the link speed
>> - Do whatever the firmware sets up
>
> "Do whatever the firmware sets up", which means either bullet 1, or
> bullet 2, or a combination of both (unlikely but AFAIU possible).
Happened to Tim.
>>
>> On my system, the last option happens to be the same as the first.
>> However, on Tim's system it's not. I had originally considered doing
>> this kind of configuration in my initial rate adaptation patch. However,
>> I deferred it since nothing needed to be configured for me.
>>
>> The problem here is that if we advertise like we are in the first mode,
>> but we are not actually, then we can end up negotiating a link mode
>> which we don't support.
>
> I didn't quite understand in your patch set why there exists a
> phydev->rate_matching as well as a phy_get_rate_matching() procedure.
> It seems like that's at the root of all issues here? Couldn't
> phy_get_rate_matching() be made to look at the hardware registers for
> the given interface?
This is what I propose below as strategy 2. I didn't do this originally,
because it was more complex and I didn't have evidence that we would need it.
>> I think there are a few ways to address this:
>>
>> - Always enable rate adaptation, since that's what we tell phylink we
>> do. This is what this patch does. It's a bit risky (since it departs
>> from "do whatever the firmware does"). It's also a bit rigid (what if
>
> I think the mistake is that we tell phylink we support rate matching
> when the firmware provisioning doesn't agree.
>
>> - We can check all the registers to ensure we are actually going to rate
>> adapt. If we aren't, we tell phylink we don't support it. This is the
>> least risky, but we can end up not bringing up the link even in
>> circumstances where we could if we configured things properly. And we
>> generally know the right way to configure things.
>
> Like when?
Well, like whenever the phy says "Please do XFI/2" or some other mode we
don't have a phy interface mode for. We will never be able to tell the MAC
"Please do XFI/2" (until we add an interface mode for it), so that's
obviously wrong.
>> - Add a configuration option (devicetree? ethtool?) on which option
>> above to pick. This is probably what we will want to do in the long
>> term, but I feel like we have enough information to determine the
>> right thing to do most of the time (without needing manual
>> intervention).
>
> Not sure I see the need, when long-term there is no volunteer to make
> the Linux driver bring Aquantia PHYs to a known state regardless of
> vendor provisioning. Until then, there is just no reason to even attempt
> this.
I mean a config for option 1 vs 2 above.
>> > As long as the Aquantia PHY driver doesn't contain all the necessary
>> > steps for bringing the PHY up from a clean slate, but works on top of
>> > what the firmware has done, changes like this make me very uncomfortable
>> > to add any PHY ID to the Aquantia driver. I'd rather leave them with the
>> > Generic C45 driver, even if that means I'll lose interrupt support, rate
>> > matching and things like that.
>>
>> I think these registers should be viewed as configuration for the phy as
>> a whole, rather than as guts which should be configure by firmware. At
>> least for the fields we're working with, it seems clear to me what's
>> going on.
>
> Until you look at the procedure in the NXP SDK and see that things are a
> bit more complicated to get right, like put the PHY in low power mode,
> sleep for a while. I think a large part of that was determined experimentally,
> out of laziness to change PHY firmware on some riser cards more than anything.
> We still expect the production boards to have a good firmware, and Linux
> to read what that does and adapt accordingly.
Alas, if only Marvell put stuff like this in a manual... All I have is a spec
sheet and the register reference, and my company has an NDA...
We aren't even using this phy on our board, so I am fine disabling rate adaptation
for funky firmwares.
--Sean
next prev parent reply other threads:[~2022-11-18 17:12 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-14 21:07 [PATCH] phy: aquantia: Configure SERDES mode by default Sean Anderson
2022-11-15 22:37 ` Vladimir Oltean
2022-11-15 22:46 ` Sean Anderson
2022-11-15 23:02 ` Vladimir Oltean
2022-11-17 23:40 ` Sean Anderson
2022-11-18 0:02 ` Andrew Lunn
2022-11-18 17:16 ` Vladimir Oltean
2022-11-18 18:56 ` Andrew Lunn
2022-11-29 0:20 ` Tim Harvey
2022-11-18 16:49 ` Vladimir Oltean
2022-11-18 17:11 ` Sean Anderson [this message]
2022-11-18 17:30 ` Vladimir Oltean
2022-11-18 18:01 ` Sean Anderson
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=1015dfec-542d-8222-6c4e-0cf9d5ee7e5a@seco.com \
--to=sean.anderson@seco.com \
--cc=andrew@lunn.ch \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=hkallweit1@gmail.com \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=netdev@vger.kernel.org \
--cc=olteanv@gmail.com \
--cc=pabeni@redhat.com \
--cc=tharvey@gateworks.com \
/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).