Re: [PATCH] phy: aquantia: Configure SERDES mode by default

[Sean Anderson <sean.anderson@seco.com>]

On 11/15/22 18:02, Vladimir Oltean wrote:
> On Tue, Nov 15, 2022 at 05:46:54PM -0500, Sean Anderson wrote:
>> On 11/15/22 17:37, Vladimir Oltean wrote:
>> > Was this patch tested and confirmed to do something sane on any platform
>> > at all?
>> 
>> This was mainly intended for Tim to test and see if it fixed his problem.
> 
> And that is stated where? Does Tim know he should test it?
> If you don't have the certainty that it works, do maintainers know not
> to apply it, as many times unfortunately happens when there is no review
> comment and the change looks innocuous?

Sorry, I should have done a better job communicating this (probably by
marking it RFC).

> 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.

> 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?

> Are you willing to take the risk to break someone's system to find out?

I hope it doesn't come to that. I would much rather get some feedback/testing
so I can be more confident in whatever we end up doing (or not).

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

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 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 
- 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.
- 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).

> 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.

--Sean