On Fri, Nov 13, 2020 at 3:45 PM Andrew Lunn <and...@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-icen...@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/
