Re: Re: realtek PHY commit bbc4d71d63549 causes regression

[Arnd Bergmann <arnd@kernel.org>]

On Fri, Nov 13, 2020 at 3:45 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > > Sadly, there is one board - Pine64 Plus - where HW settings are wrong and it
> > > actually needs SW override. Until this Realtek PHY driver fix was merged, it
> > > was unclear what magic value provided by Realtek to board manufacturer does.
> > >
> > > Reference:
> > > https://lore.kernel.org/netdev/20191001082912.12905-3-icenowy@aosc.io/
> >
> > I have merged the fixes from the allwinner tree now, but I still think we
> > need something better than this, as the current state breaks any existing
> > dtb file that has the incorrect values, and this really should not have been
> > considered for backporting to stable kernels.
>
> Hi Arnd
>
> This PHY driver bug hiding DT bug is always hard to handle. We have
> been though it once before with the Atheros PHY. All the buggy DT
> files were fixed in about one cycle.

Do you have a link to the problem for the Atheros PHY?

I'm generally skeptical about the idea of being able to fix all DTBs,
some of the problems with that being:

- There is no way to identify which of of the 2019 dts files in the
  kernel actually have this particular phy, because it does not
  have a device node in the dt. Looking only at files that set
  phy-mode="rgmii" limits it to 235 files, but that is still more than
  anyone can test.

- if there was a way to automate identifying the dts files that
  need to be modified, we should also be able to do it at runtime

- I really want to encourage stable dtb files that users can ship
  with their boards in the same way that we treat firmware
  on PCs and classic Open Firmware machines as stable.
  Updating a kernel should never require updating the firmware
  first.

- The model advocated by EBBR [1] actually requires this in
  order to deal with distros updating kernels. When you use a
  distro with grub (or similar) as the bootloader, you can choose
  between multiple kernels through a boot menu, but a single
  DTB is provided by the firmware to the bootloader.
  There is an ongoing discussion on how to deal with picking
  the right DTB for a kernel being picked by the boot loader, while
  also having the firmware modify an arbitrary set of properties
  (memory size, boot arguments, mac address, random seed,
  ...) in there, but it would be best to just not have to rely on that
  kind of mechanism.

- Not every custom board should need to have its dts file
  in the kernel. I'm less concerned about incorrect properties
  in out-of-tree dts files, but would strongly discourage
  other incompatible binding changes.

> Now that we know there is a board which really does want rgmii when it
> says rgmii, we cannot simply ignore it in the PHY driver.
>
> But the whole story is muddy because of the backport to stable.  It
> might make sense to revert the stable change, and just leave HEAD
> broken. That then gives people more time to fix DT blobs. But we have
> to consider Pine64 Plus, are we happy breaking that for a while, when
> it is actually doing everything correct, and is bug free?

I agree this makes the problem harder, but I have still hope that
we can come up with a code solution that can deal with this
one board that needs to have the correct settings applied as well
as the others on which we have traditionally ignored them.

As I understand it so far, the reason this board needs a different
setting is that the strapping pins are wired incorrectly, while all
other boards set them right and work correctly by default. I would
much prefer a way to identify this behavior in dts and have the phy
driver just warn when it finds a mismatch between the internal
delay setting in DT and the strapping pins but keep using the
setting from the strapping pins when there is a conflict.

There are surely other ways to achieve the same state of having
it all working without requiring the fixed dts.

     Arnd

[1] https://arm-software.github.io/ebbr/