diff mbox series

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

Message ID 20210208222203.22335-13-info@metux.net
State New
Headers show
Series [RFC,01/12] of: base: improve error message in of_phandle_iterator_next() | expand

Commit Message

Enrico Weigelt, metux IT consult Feb. 8, 2021, 10:22 p.m. UTC 
Add oftree based support for PC Engines APUv2/3/4 board family.
This is entirely separate from the existing pcengines-apuv2 driver.

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
---
 drivers/platform/of/Kconfig   | 15 ++++++++
 drivers/platform/of/Makefile  |  2 +
 drivers/platform/of/apu2x.dts | 86 +++++++++++++++++++++++++++++++++++++++++++
 drivers/platform/of/init.c    |  7 ++++
 4 files changed, 110 insertions(+)
 create mode 100644 drivers/platform/of/apu2x.dts

Comments

Rob Herring Feb. 9, 2021, 12:06 a.m. UTC | #1 
On Mon, Feb 8, 2021 at 4:22 PM Enrico Weigelt, metux IT consult
<info@metux.net> wrote:
>
> Add oftree based support for PC Engines APUv2/3/4 board family.
> This is entirely separate from the existing pcengines-apuv2 driver.
>
> Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
> ---
>  drivers/platform/of/Kconfig   | 15 ++++++++
>  drivers/platform/of/Makefile  |  2 +
>  drivers/platform/of/apu2x.dts | 86 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/platform/of/init.c    |  7 ++++
>  4 files changed, 110 insertions(+)
>  create mode 100644 drivers/platform/of/apu2x.dts
>
> diff --git a/drivers/platform/of/Kconfig b/drivers/platform/of/Kconfig
> index a0b5a641a7c6..e13b6ee8c316 100644
> --- a/drivers/platform/of/Kconfig
> +++ b/drivers/platform/of/Kconfig
> @@ -38,4 +38,19 @@ config PLATFORM_OF_DRV_SYSFS_DT
>           Say Y here to enable exposing device tree nodes at
>           /sys/firmware/devicetree.
>
> +config PLATFORM_OF_DRV_PCENGINES_APU2
> +       bool "Support PC Engines APU2/3/4 mainboards"
> +       depends on INPUT
> +       depends on GPIOLIB
> +       depends on X86
> +       select GPIO_AMD_FCH
> +       select KEYBOARD_GPIO_POLLED
> +       select LEDS_GPIO
> +       select INPUT_KEYBOARD
> +       help
> +         Say Y to enable devicetree based support for PC Engines APU2/3/4
> +         mainboards. This supersedes the older pcengines-apu2 driver.
> +
> +         Supports Gpios, front panel LEDs and front button.
> +
>  endif # PLATFORM_OF_DRV
> diff --git a/drivers/platform/of/Makefile b/drivers/platform/of/Makefile
> index 84cf3003c500..dd4a13c18f16 100644
> --- a/drivers/platform/of/Makefile
> +++ b/drivers/platform/of/Makefile
> @@ -2,4 +2,6 @@
>
>  ofboard-y := init.o drv.o
>
> +ofboard-$(CONFIG_PLATFORM_OF_DRV_PCENGINES_APU2) += apu2x.dtb.o
> +
>  obj-$(CONFIG_PLATFORM_OF_DRV) += ofboard.o
> diff --git a/drivers/platform/of/apu2x.dts b/drivers/platform/of/apu2x.dts
> new file mode 100644
> index 000000000000..c16a59eb2a0e
> --- /dev/null
> +++ b/drivers/platform/of/apu2x.dts
> @@ -0,0 +1,86 @@
> +/dts-v1/;
> +
> +#include <dt-bindings/leds/common.h>
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/gpio/amd-fch-gpio.h>
> +
> +/ {
> +    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

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

> +        };
> +        devices {
> +            gpio1: gpio1 {
> +                compatible = "amd,fch-gpio";

This of course will need to be documented.

> +                gpio-controller;
> +                status = "okay";

nit: That's the default.

> +                ngpios=<7>;
> +                #gpio-cells=<2>;
> +                gpio-regs = <
> +                    AMD_FCH_GPIO_REG_GPIO57 // led1
> +                    AMD_FCH_GPIO_REG_GPIO58 // led2
> +                    AMD_FCH_GPIO_REG_GPIO59_DEVSLP1 // led3
> +                    AMD_FCH_GPIO_REG_GPIO32_GE1 // modesw
> +                    AMD_FCH_GPIO_REG_GPIO33_GE2 // simawap
> +                    AMD_FCH_GPIO_REG_GPIO55_DEVSLP0 // mpcie2
> +                    AMD_FCH_GPIO_REG_GPIO51 // mpcie3
> +                >;
> +                gpio-line-names =
> +                    "front-led1",
> +                    "front-led2",
> +                    "front-led3",
> +                    "front-button",
> +                    "simswap",
> +                    "mpcie2_reset",
> +                    "mpcie3_reset";
> +            };
> +            front-leds {
> +                compatible = "gpio-leds";
> +                led@0 {
> +                    gpios = <&gpio1 0 GPIO_ACTIVE_HIGH>;
> +                    color = <LED_COLOR_ID_GREEN>;
> +                    default-state = "keep";
> +                    label = "apu:green:1";
> +                };
> +                led@1 {
> +                    gpios = <&gpio1 1 GPIO_ACTIVE_HIGH>;
> +                    color = <LED_COLOR_ID_GREEN>;
> +                    default-state = "keep";
> +                    label = "apu:green:2";
> +                };
> +                led@2 {
> +                    gpios = <&gpio1 2 GPIO_ACTIVE_HIGH>;
> +                    color = <LED_COLOR_ID_GREEN>;
> +                    default-state = "keep";
> +                    label = "apu:green:3";
> +                };
> +            };
> +            front-keys {
> +                compatible = "gpio-keys-polled";
> +                address-cells = <1>;
> +                size-cells = <0>;
> +                poll-interval = <100>;
> +                button@1 {
> +                    linux,code = <KEY_RESTART>;
> +                    label = "front button";
> +                    debounce-interval = <10>;
> +                    gpios = <&gpio1 3 GPIO_ACTIVE_HIGH>;
> +                };
> +            };
> +        };
> +    };
> +};
> diff --git a/drivers/platform/of/init.c b/drivers/platform/of/init.c
> index 3b8373cda77a..195120dad26d 100644
> --- a/drivers/platform/of/init.c
> +++ b/drivers/platform/of/init.c
> @@ -47,7 +47,14 @@ static ssize_t fdt_image_raw_read(struct file *filep, struct kobject *kobj,
>         return count;
>  }
>
> +#ifdef CONFIG_PLATFORM_OF_DRV_PCENGINES_APU2
> +DECLARE_FDT_EXTERN(apu2x);
> +#endif
> +
>  static struct fdt_image fdt[] = {
> +#ifdef CONFIG_PLATFORM_OF_DRV_PCENGINES_APU2
> +       FDT_IMAGE_ENT(apu2x)
> +#endif
>  };
>
>  static int __init ofdrv_init_sysfs(struct fdt_image *image)
> --
> 2.11.0
>
Enrico Weigelt, metux IT consult Feb. 11, 2021, 1:15 p.m. UTC | #2 
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
Linus Walleij March 1, 2021, 2:55 p.m. UTC | #3 
On Mon, Feb 8, 2021 at 11:22 PM Enrico Weigelt, metux IT consult
<info@metux.net> wrote:

> +                gpio-regs = <
> +                    AMD_FCH_GPIO_REG_GPIO57 // led1
> +                    AMD_FCH_GPIO_REG_GPIO58 // led2
> +                    AMD_FCH_GPIO_REG_GPIO59_DEVSLP1 // led3
> +                    AMD_FCH_GPIO_REG_GPIO32_GE1 // modesw
> +                    AMD_FCH_GPIO_REG_GPIO33_GE2 // simawap
> +                    AMD_FCH_GPIO_REG_GPIO55_DEVSLP0 // mpcie2
> +                    AMD_FCH_GPIO_REG_GPIO51 // mpcie3
> +                >;

Please don't define registers in the DTS files. Determine the set of registers
from the compatible string and put them in the driver. If that is not possible,
the compatible string is not precise enough and needs to indicate properly
which hardware this is.

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/platform/of/Kconfig b/drivers/platform/of/Kconfig
index a0b5a641a7c6..e13b6ee8c316 100644
--- a/drivers/platform/of/Kconfig
+++ b/drivers/platform/of/Kconfig
@@ -38,4 +38,19 @@  config PLATFORM_OF_DRV_SYSFS_DT
 	  Say Y here to enable exposing device tree nodes at
 	  /sys/firmware/devicetree.
 
+config PLATFORM_OF_DRV_PCENGINES_APU2
+	bool "Support PC Engines APU2/3/4 mainboards"
+	depends on INPUT
+	depends on GPIOLIB
+	depends on X86
+	select GPIO_AMD_FCH
+	select KEYBOARD_GPIO_POLLED
+	select LEDS_GPIO
+	select INPUT_KEYBOARD
+	help
+	  Say Y to enable devicetree based support for PC Engines APU2/3/4
+	  mainboards. This supersedes the older pcengines-apu2 driver.
+
+	  Supports Gpios, front panel LEDs and front button.
+
 endif # PLATFORM_OF_DRV
diff --git a/drivers/platform/of/Makefile b/drivers/platform/of/Makefile
index 84cf3003c500..dd4a13c18f16 100644
--- a/drivers/platform/of/Makefile
+++ b/drivers/platform/of/Makefile
@@ -2,4 +2,6 @@ 
 
 ofboard-y := init.o drv.o
 
+ofboard-$(CONFIG_PLATFORM_OF_DRV_PCENGINES_APU2) += apu2x.dtb.o
+
 obj-$(CONFIG_PLATFORM_OF_DRV) += ofboard.o
diff --git a/drivers/platform/of/apu2x.dts b/drivers/platform/of/apu2x.dts
new file mode 100644
index 000000000000..c16a59eb2a0e
--- /dev/null
+++ b/drivers/platform/of/apu2x.dts
@@ -0,0 +1,86 @@ 
+/dts-v1/;
+
+#include <dt-bindings/leds/common.h>
+#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/input/input.h>
+#include <dt-bindings/gpio/amd-fch-gpio.h>
+
+/ {
+    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";
+        unbind {
+            acpi = "PNP0076:00", "PNP0B00:00";
+            platform = "platform-framebuffer.0", "PNP0103:00";
+        };
+        devices {
+            gpio1: gpio1 {
+                compatible = "amd,fch-gpio";
+                gpio-controller;
+                status = "okay";
+                ngpios=<7>;
+                #gpio-cells=<2>;
+                gpio-regs = <
+                    AMD_FCH_GPIO_REG_GPIO57 // led1
+                    AMD_FCH_GPIO_REG_GPIO58 // led2
+                    AMD_FCH_GPIO_REG_GPIO59_DEVSLP1 // led3
+                    AMD_FCH_GPIO_REG_GPIO32_GE1 // modesw
+                    AMD_FCH_GPIO_REG_GPIO33_GE2 // simawap
+                    AMD_FCH_GPIO_REG_GPIO55_DEVSLP0 // mpcie2
+                    AMD_FCH_GPIO_REG_GPIO51 // mpcie3
+                >;
+                gpio-line-names =
+                    "front-led1",
+                    "front-led2",
+                    "front-led3",
+                    "front-button",
+                    "simswap",
+                    "mpcie2_reset",
+                    "mpcie3_reset";
+            };
+            front-leds {
+                compatible = "gpio-leds";
+                led@0 {
+                    gpios = <&gpio1 0 GPIO_ACTIVE_HIGH>;
+                    color = <LED_COLOR_ID_GREEN>;
+                    default-state = "keep";
+                    label = "apu:green:1";
+                };
+                led@1 {
+                    gpios = <&gpio1 1 GPIO_ACTIVE_HIGH>;
+                    color = <LED_COLOR_ID_GREEN>;
+                    default-state = "keep";
+                    label = "apu:green:2";
+                };
+                led@2 {
+                    gpios = <&gpio1 2 GPIO_ACTIVE_HIGH>;
+                    color = <LED_COLOR_ID_GREEN>;
+                    default-state = "keep";
+                    label = "apu:green:3";
+                };
+            };
+            front-keys {
+                compatible = "gpio-keys-polled";
+                address-cells = <1>;
+                size-cells = <0>;
+                poll-interval = <100>;
+                button@1 {
+                    linux,code = <KEY_RESTART>;
+                    label = "front button";
+                    debounce-interval = <10>;
+                    gpios = <&gpio1 3 GPIO_ACTIVE_HIGH>;
+                };
+            };
+        };
+    };
+};
diff --git a/drivers/platform/of/init.c b/drivers/platform/of/init.c
index 3b8373cda77a..195120dad26d 100644
--- a/drivers/platform/of/init.c
+++ b/drivers/platform/of/init.c
@@ -47,7 +47,14 @@  static ssize_t fdt_image_raw_read(struct file *filep, struct kobject *kobj,
 	return count;
 }
 
+#ifdef CONFIG_PLATFORM_OF_DRV_PCENGINES_APU2
+DECLARE_FDT_EXTERN(apu2x);
+#endif
+
 static struct fdt_image fdt[] = {
+#ifdef CONFIG_PLATFORM_OF_DRV_PCENGINES_APU2
+	FDT_IMAGE_ENT(apu2x)
+#endif
 };
 
 static int __init ofdrv_init_sysfs(struct fdt_image *image)