Re: [RFC PATCH 12/12] platform/x86/of: add support for PC Engines APU v2/3/4 boards

[Enrico Weigelt, metux IT consult]

On 09.02.21 01:06, Rob Herring wrote:

Hi,

> > +/ {
> > +    apu2x {
> > +        compatible = "virtual,dmi-board";
> > +        dmi-sys-vendor = "PC engines";
> > +        dmi-board-name =
> > +          "APU2",
> > +          "apu2",
> > +          "PC engines apu2",
> > +          "APU3",
> > +          "apu3",
> > +          "PC engines apu3",
> > +          "APU4",
> > +          "apu4",
> > +          "PC engines apu4";
>
> I think these DMI properties just need to be the compatible string(s).
> We already have a way to do matching with DT and don't need a
> secondary way. If you can

It's not easy fitting that into one string, because we've got lots of
combinations that need to be matched. In this specific case, I haven't
seen any board where the vendor name isn't an exact match of the given
string (that's why it's only one entry), but in the past seen several
boards where even this changes between bios versions. The board names,
more varying.

Something that's not reflected in this example yet: there're even more
subtle differences between production series (eg. certain pins not
wired, etc). Supporting such things would need adding more matching
rules and possibly runtime DT manipulations.

> > +        unbind {
> > +            acpi = "PNP0076:00", "PNP0B00:00";
> > +            platform = "platform-framebuffer.0", "PNP0103:00";
>
> This node really needs to go. It's clearly Linuxisms. It either has to
> go in the kernel or userspace.

Note that the whole thing here *is* a Linuxism. This kind of DTs is
built into the kernel, not in firmware or anywhere else. This stuff is
only for cases where firmware is not giving, or giving broken
information. And it's for replacing hand-written C code by a machine
readable description.

I had to put that in, since in some cases firmware (-versions) already
enumerates some devices, but does it in a wrong or incomplete way.
So, these devices need to be removed first, before the correct ones
can be initialized. (note that this patch, for now, is just an hacking
example - some details are still broken).

If anybody has a better idea how to do that, let me know.

In general, I'd like to have everything for one board (family) in one
declarative file.

> > +        };
> > +        devices {
> > +            gpio1: gpio1 {
> > +                compatible = "amd,fch-gpio";
>
> This of course will need to be documented.

Yes, but that's a different issue. It's still in RFC stage.
The gpio-amd-fch changes are in this patch queue for a complete example,
but probably will be upstreamed separately.

> > +                gpio-controller;
> > +                status = "okay";
>
> nit: That's the default.

Okay, dropping it.


--mtx

--
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@xxxxxxxxx -- +49-151-27565287