Messages in this thread | | | Date | Fri, 18 Nov 2022 12:11:30 -0500 | Subject | Re: [PATCH] phy: aquantia: Configure SERDES mode by default | From | Sean Anderson <> |
| |
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
| |