[PATCH v4 1/2] x86: Support APU5 & APU6 in PCEngines platform driver

[PATCH v4 1/2] x86: Support APU5 & APU6 in PCEngines platform driver

[Philip Prindeville]

From: Philip Prindeville <philipp@redfish-solutions.com>

PCEngines make a number of SBC. APU5 has 5 mpcie slots + MSATA
It also has support for 3x LTE modems with 6x SIM slots (pairs with a
SIM switch device). Each mpcie slot for modems has a reset GPIO

To ensure that the naming is sane between APU2-6 the GPIOS are
renamed to be modem1-reset, modem2-reset, etc. This is significant
because the slots that can be reset change between APU2 and APU3/4

GPIO for simswap is moved to the end of the list as it could be dropped
for APU2 boards (but causes no harm to leave it in, hardware could be
added to a future rev of the board).

Structure of the GPIOs for APU5 is extremely similar to APU2-4, but
many lines are moved around and there are simply more
modems/resets/sim-swap lines to breakout.

Also added APU6, which is essentially APU4 with a different ethernet
interface and SFP cage on eth0.

Signed-off-by: Ed Wildgoose <lists@wildgooses.com>
Reviewed-by: Philip Prindeville <philipp@redfish-solutions.com>
Reviewed-by: Andreas Eberlein <foodeas@aeberlein.de>
Reviewed-by: Paul Spooren <paul@spooren.de>
---
 drivers/platform/x86/pcengines-apuv2.c | 113 ++++++++++++++++++++++---
 1 file changed, 99 insertions(+), 14 deletions(-)

diff --git a/drivers/platform/x86/pcengines-apuv2.c b/drivers/platform/x86/pcengines-apuv2.c
index d063d91db9bcbe5ceb2ac641d3105df37651ac4d..8731564bab62c1e47e99adb6ec23b3de81b09069 100644
--- a/drivers/platform/x86/pcengines-apuv2.c
+++ b/drivers/platform/x86/pcengines-apuv2.c
@@ -1,10 +1,11 @@
 // SPDX-License-Identifier: GPL-2.0+
 
 /*
- * PC-Engines APUv2/APUv3 board platform driver
+ * PC-Engines APUv2-6 board platform driver
  * for GPIO buttons and LEDs
  *
  * Copyright (C) 2018 metux IT consult
+ * Copyright (C) 2022 Ed Wildgoose <lists@wildgooses.com>
  * Author: Enrico Weigelt <info@metux.net>
  */
 
@@ -22,38 +23,70 @@
 #include <linux/platform_data/gpio/gpio-amd-fch.h>
 
 /*
- * NOTE: this driver only supports APUv2/3 - not APUv1, as this one
+ * NOTE: this driver only supports APUv2-6 - not APUv1, as this one
  * has completely different register layouts.
  */
 
+/*
+ * There are a number of APU variants, with differing features
+ * APU2 has SIM slots 1/2 mapping to mPCIe sockets 1/2
+ * APU3/4 moved SIM slot 1 to mPCIe socket 3, ie logically reversed
+ * However, most APU3/4 have a SIM switch which we default on to reverse
+ * the order and keep physical SIM order matching physical modem order
+ * APU6 is approximately the same as APU4 with different ethernet layout
+ *
+ * APU5 has 3x SIM sockets, all with a SIM switch
+ * several GPIOs are shuffled (see schematic), including MODESW
+ */
+
 /* Register mappings */
 #define APU2_GPIO_REG_LED1		AMD_FCH_GPIO_REG_GPIO57
 #define APU2_GPIO_REG_LED2		AMD_FCH_GPIO_REG_GPIO58
 #define APU2_GPIO_REG_LED3		AMD_FCH_GPIO_REG_GPIO59_DEVSLP1
 #define APU2_GPIO_REG_MODESW		AMD_FCH_GPIO_REG_GPIO32_GE1
 #define APU2_GPIO_REG_SIMSWAP		AMD_FCH_GPIO_REG_GPIO33_GE2
-#define APU2_GPIO_REG_MPCIE2		AMD_FCH_GPIO_REG_GPIO55_DEVSLP0
-#define APU2_GPIO_REG_MPCIE3		AMD_FCH_GPIO_REG_GPIO51
+#define APU2_GPIO_REG_RESETM1		AMD_FCH_GPIO_REG_GPIO51
+#define APU2_GPIO_REG_RESETM2		AMD_FCH_GPIO_REG_GPIO55_DEVSLP0
+
+#define APU5_GPIO_REG_MODESW		AMT_FCH_GPIO_REG_GEVT22
+#define APU5_GPIO_REG_SIMSWAP1		AMD_FCH_GPIO_REG_GPIO68
+#define APU5_GPIO_REG_SIMSWAP2		AMD_FCH_GPIO_REG_GPIO32_GE1
+#define APU5_GPIO_REG_SIMSWAP3		AMD_FCH_GPIO_REG_GPIO33_GE2
+#define APU5_GPIO_REG_RESETM1		AMD_FCH_GPIO_REG_GPIO51
+#define APU5_GPIO_REG_RESETM2		AMD_FCH_GPIO_REG_GPIO55_DEVSLP0
+#define APU5_GPIO_REG_RESETM3		AMD_FCH_GPIO_REG_GPIO64
 
 /* Order in which the GPIO lines are defined in the register list */
 #define APU2_GPIO_LINE_LED1		0
 #define APU2_GPIO_LINE_LED2		1
 #define APU2_GPIO_LINE_LED3		2
 #define APU2_GPIO_LINE_MODESW		3
-#define APU2_GPIO_LINE_SIMSWAP		4
-#define APU2_GPIO_LINE_MPCIE2		5
-#define APU2_GPIO_LINE_MPCIE3		6
+#define APU2_GPIO_LINE_RESETM1		4
+#define APU2_GPIO_LINE_RESETM2		5
+#define APU2_GPIO_LINE_SIMSWAP		6
+
+#define APU5_GPIO_LINE_LED1		0
+#define APU5_GPIO_LINE_LED2		1
+#define APU5_GPIO_LINE_LED3		2
+#define APU5_GPIO_LINE_MODESW		3
+#define APU5_GPIO_LINE_RESETM1		4
+#define APU5_GPIO_LINE_RESETM2		5
+#define APU5_GPIO_LINE_RESETM3		6
+#define APU5_GPIO_LINE_SIMSWAP1		7
+#define APU5_GPIO_LINE_SIMSWAP2		8
+#define APU5_GPIO_LINE_SIMSWAP3		9
 
-/* GPIO device */
+
+/* GPIO device - APU2/3/4/6 */
 
 static int apu2_gpio_regs[] = {
 	[APU2_GPIO_LINE_LED1]		= APU2_GPIO_REG_LED1,
 	[APU2_GPIO_LINE_LED2]		= APU2_GPIO_REG_LED2,
 	[APU2_GPIO_LINE_LED3]		= APU2_GPIO_REG_LED3,
 	[APU2_GPIO_LINE_MODESW]		= APU2_GPIO_REG_MODESW,
+	[APU2_GPIO_LINE_RESETM1]	= APU2_GPIO_REG_RESETM1,
+	[APU2_GPIO_LINE_RESETM2]	= APU2_GPIO_REG_RESETM2,
 	[APU2_GPIO_LINE_SIMSWAP]	= APU2_GPIO_REG_SIMSWAP,
-	[APU2_GPIO_LINE_MPCIE2]		= APU2_GPIO_REG_MPCIE2,
-	[APU2_GPIO_LINE_MPCIE3]		= APU2_GPIO_REG_MPCIE3,
 };
 
 static const char * const apu2_gpio_names[] = {
@@ -61,9 +94,9 @@ static const char * const apu2_gpio_names[] = {
 	[APU2_GPIO_LINE_LED2]		= "front-led2",
 	[APU2_GPIO_LINE_LED3]		= "front-led3",
 	[APU2_GPIO_LINE_MODESW]		= "front-button",
+	[APU2_GPIO_LINE_RESETM1]	= "modem1-reset",
+	[APU2_GPIO_LINE_RESETM2]	= "modem2-reset",
 	[APU2_GPIO_LINE_SIMSWAP]	= "simswap",
-	[APU2_GPIO_LINE_MPCIE2]		= "mpcie2_reset",
-	[APU2_GPIO_LINE_MPCIE3]		= "mpcie3_reset",
 };
 
 static const struct amd_fch_gpio_pdata board_apu2 = {
@@ -72,6 +105,40 @@ static const struct amd_fch_gpio_pdata board_apu2 = {
 	.gpio_names	= apu2_gpio_names,
 };
 
+/* GPIO device - APU5 */
+
+static int apu5_gpio_regs[] = {
+	[APU5_GPIO_LINE_LED1]		= APU2_GPIO_REG_LED1,
+	[APU5_GPIO_LINE_LED2]		= APU2_GPIO_REG_LED2,
+	[APU5_GPIO_LINE_LED3]		= APU2_GPIO_REG_LED3,
+	[APU5_GPIO_LINE_MODESW]		= APU5_GPIO_REG_MODESW,
+	[APU5_GPIO_LINE_RESETM1]	= APU5_GPIO_REG_RESETM1,
+	[APU5_GPIO_LINE_RESETM2]	= APU5_GPIO_REG_RESETM2,
+	[APU5_GPIO_LINE_RESETM3]	= APU5_GPIO_REG_RESETM3,
+	[APU5_GPIO_LINE_SIMSWAP1]	= APU5_GPIO_REG_SIMSWAP1,
+	[APU5_GPIO_LINE_SIMSWAP2]	= APU5_GPIO_REG_SIMSWAP2,
+	[APU5_GPIO_LINE_SIMSWAP3]	= APU5_GPIO_REG_SIMSWAP3,
+};
+
+static const char * const apu5_gpio_names[] = {
+	[APU5_GPIO_LINE_LED1]		= "front-led1",
+	[APU5_GPIO_LINE_LED2]		= "front-led2",
+	[APU5_GPIO_LINE_LED3]		= "front-led3",
+	[APU5_GPIO_LINE_MODESW]		= "front-button",
+	[APU5_GPIO_LINE_RESETM1]	= "modem1-reset",
+	[APU5_GPIO_LINE_RESETM2]	= "modem2-reset",
+	[APU5_GPIO_LINE_RESETM3]	= "modem3-reset",
+	[APU5_GPIO_LINE_SIMSWAP1]	= "simswap1",
+	[APU5_GPIO_LINE_SIMSWAP2]	= "simswap2",
+	[APU5_GPIO_LINE_SIMSWAP3]	= "simswap3",
+};
+
+static const struct amd_fch_gpio_pdata board_apu5 = {
+	.gpio_num	= ARRAY_SIZE(apu5_gpio_regs),
+	.gpio_reg	= apu5_gpio_regs,
+	.gpio_names	= apu5_gpio_names,
+};
+
 /* GPIO LEDs device */
 
 static const struct gpio_led apu2_leds[] = {
@@ -215,6 +282,24 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
 		},
 		.driver_data = (void *)&board_apu2,
 	},
+	/* APU5 w/ mainline BIOS */
+	{
+		.ident		= "apu5",
+		.matches	= {
+			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
+			DMI_MATCH(DMI_BOARD_NAME, "apu5")
+		},
+		.driver_data	= (void *)&board_apu5,
+	},
+	/* APU6 w/ mainline BIOS */
+	{
+		.ident		= "apu6",
+		.matches	= {
+			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
+			DMI_MATCH(DMI_BOARD_NAME, "apu6")
+		},
+		.driver_data	= (void *)&board_apu2,
+	},
 	{}
 };
 
@@ -249,7 +334,7 @@ static int __init apu_board_init(void)
 
 	id = dmi_first_match(apu_gpio_dmi_table);
 	if (!id) {
-		pr_err("failed to detect APU board via DMI\n");
+		pr_err("No APU board detected via DMI\n");
 		return -ENODEV;
 	}
 
@@ -288,7 +373,7 @@ module_init(apu_board_init);
 module_exit(apu_board_exit);
 
 MODULE_AUTHOR("Enrico Weigelt, metux IT consult <info@metux.net>");
-MODULE_DESCRIPTION("PC Engines APUv2/APUv3 board GPIO/LEDs/keys driver");
+MODULE_DESCRIPTION("PC Engines APUv2-6 board GPIO/LEDs/keys driver");
 MODULE_LICENSE("GPL");
 MODULE_DEVICE_TABLE(dmi, apu_gpio_dmi_table);
 MODULE_ALIAS("platform:pcengines-apuv2");
-- 
2.34.1

Re: [PATCH v4 1/2] x86: Support APU5 & APU6 in PCEngines platform driver

[Hans de Goede]

Hi,

On 1/14/23 00:11, Philip Prindeville wrote:
> From: Philip Prindeville <philipp@redfish-solutions.com>
> 
> PCEngines make a number of SBC. APU5 has 5 mpcie slots + MSATA
> It also has support for 3x LTE modems with 6x SIM slots (pairs with a
> SIM switch device). Each mpcie slot for modems has a reset GPIO
> 
> To ensure that the naming is sane between APU2-6 the GPIOS are
> renamed to be modem1-reset, modem2-reset, etc. This is significant
> because the slots that can be reset change between APU2 and APU3/4
> 
> GPIO for simswap is moved to the end of the list as it could be dropped
> for APU2 boards (but causes no harm to leave it in, hardware could be
> added to a future rev of the board).
> 
> Structure of the GPIOs for APU5 is extremely similar to APU2-4, but
> many lines are moved around and there are simply more
> modems/resets/sim-swap lines to breakout.
> 
> Also added APU6, which is essentially APU4 with a different ethernet
> interface and SFP cage on eth0.
> 
> Signed-off-by: Ed Wildgoose <lists@wildgooses.com>
> Reviewed-by: Philip Prindeville <philipp@redfish-solutions.com>
> Reviewed-by: Andreas Eberlein <foodeas@aeberlein.de>
> Reviewed-by: Paul Spooren <paul@spooren.de>
> ---
>  drivers/platform/x86/pcengines-apuv2.c | 113 ++++++++++++++++++++++---
>  1 file changed, 99 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/platform/x86/pcengines-apuv2.c b/drivers/platform/x86/pcengines-apuv2.c
> index d063d91db9bcbe5ceb2ac641d3105df37651ac4d..8731564bab62c1e47e99adb6ec23b3de81b09069 100644
> --- a/drivers/platform/x86/pcengines-apuv2.c
> +++ b/drivers/platform/x86/pcengines-apuv2.c
> @@ -1,10 +1,11 @@
>  // SPDX-License-Identifier: GPL-2.0+
>  
>  /*
> - * PC-Engines APUv2/APUv3 board platform driver
> + * PC-Engines APUv2-6 board platform driver
>   * for GPIO buttons and LEDs
>   *
>   * Copyright (C) 2018 metux IT consult
> + * Copyright (C) 2022 Ed Wildgoose <lists@wildgooses.com>
>   * Author: Enrico Weigelt <info@metux.net>
>   */
>  
> @@ -22,38 +23,70 @@
>  #include <linux/platform_data/gpio/gpio-amd-fch.h>
>  
>  /*
> - * NOTE: this driver only supports APUv2/3 - not APUv1, as this one
> + * NOTE: this driver only supports APUv2-6 - not APUv1, as this one
>   * has completely different register layouts.
>   */
>  
> +/*
> + * There are a number of APU variants, with differing features
> + * APU2 has SIM slots 1/2 mapping to mPCIe sockets 1/2
> + * APU3/4 moved SIM slot 1 to mPCIe socket 3, ie logically reversed
> + * However, most APU3/4 have a SIM switch which we default on to reverse
> + * the order and keep physical SIM order matching physical modem order
> + * APU6 is approximately the same as APU4 with different ethernet layout
> + *
> + * APU5 has 3x SIM sockets, all with a SIM switch
> + * several GPIOs are shuffled (see schematic), including MODESW
> + */
> +
>  /* Register mappings */
>  #define APU2_GPIO_REG_LED1		AMD_FCH_GPIO_REG_GPIO57
>  #define APU2_GPIO_REG_LED2		AMD_FCH_GPIO_REG_GPIO58
>  #define APU2_GPIO_REG_LED3		AMD_FCH_GPIO_REG_GPIO59_DEVSLP1
>  #define APU2_GPIO_REG_MODESW		AMD_FCH_GPIO_REG_GPIO32_GE1
>  #define APU2_GPIO_REG_SIMSWAP		AMD_FCH_GPIO_REG_GPIO33_GE2
> -#define APU2_GPIO_REG_MPCIE2		AMD_FCH_GPIO_REG_GPIO55_DEVSLP0
> -#define APU2_GPIO_REG_MPCIE3		AMD_FCH_GPIO_REG_GPIO51
> +#define APU2_GPIO_REG_RESETM1		AMD_FCH_GPIO_REG_GPIO51
> +#define APU2_GPIO_REG_RESETM2		AMD_FCH_GPIO_REG_GPIO55_DEVSLP0
> +
> +#define APU5_GPIO_REG_MODESW		AMT_FCH_GPIO_REG_GEVT22
> +#define APU5_GPIO_REG_SIMSWAP1		AMD_FCH_GPIO_REG_GPIO68
> +#define APU5_GPIO_REG_SIMSWAP2		AMD_FCH_GPIO_REG_GPIO32_GE1
> +#define APU5_GPIO_REG_SIMSWAP3		AMD_FCH_GPIO_REG_GPIO33_GE2
> +#define APU5_GPIO_REG_RESETM1		AMD_FCH_GPIO_REG_GPIO51
> +#define APU5_GPIO_REG_RESETM2		AMD_FCH_GPIO_REG_GPIO55_DEVSLP0
> +#define APU5_GPIO_REG_RESETM3		AMD_FCH_GPIO_REG_GPIO64
>  
>  /* Order in which the GPIO lines are defined in the register list */
>  #define APU2_GPIO_LINE_LED1		0
>  #define APU2_GPIO_LINE_LED2		1
>  #define APU2_GPIO_LINE_LED3		2
>  #define APU2_GPIO_LINE_MODESW		3
> -#define APU2_GPIO_LINE_SIMSWAP		4
> -#define APU2_GPIO_LINE_MPCIE2		5
> -#define APU2_GPIO_LINE_MPCIE3		6
> +#define APU2_GPIO_LINE_RESETM1		4
> +#define APU2_GPIO_LINE_RESETM2		5
> +#define APU2_GPIO_LINE_SIMSWAP		6

I don't think this changing of GPIO ordering, or
for that part the changing of the gpio_names from 
"mpcie2_reset" to "modem1-reset" is a good idea.

I'm not entirely sure how these GPIOs are supposed to be
consumed / used by userspace. But since they are not used
directly in this driver I assume userspace is supposed to
use either the (deprecated) sysfs GPIO API or the new ioctl
based GPIO API to toggle say "simswap" if it needs to.

The old sysfs API exclusively uses pin-indexes inside a GPIO
chip to select the pin, so by changing the pin order you
have just broken the userspace API.

And the new ioctl API can use either pin-indexes or GPIO-line-names,
so by changing the names you have also just potentially broken
that.

Please keep the order as is and only use the new names for
the newly added models (so for APU6 I believe).

I guess that means adding a new apu6_gpio_names[]
array for APU6, that is fine.

Note you can still do the "MPCIE2" -> "RESETM1" defines
if you want, to allow reusing the defines and the apu2_gpio_regs[]
array. But in order to not brake uAPI you must:

1. Not change the order of the pins
2. Keep the "mpcie2_reset" and "mpcie3_reset" names for the
   already supported models.

Also can you please split this patch into 2 patches,
1 adding the APU5 support and one adding the APU6 support
(not necessarily in that order) ?

Regards,

Hans


p.s.

About the bounces I'm getting bounces for linux-x86_64@vger.kernel.org
(dropped) but not for Enrico's email.



> +
> +#define APU5_GPIO_LINE_LED1		0
> +#define APU5_GPIO_LINE_LED2		1
> +#define APU5_GPIO_LINE_LED3		2
> +#define APU5_GPIO_LINE_MODESW		3
> +#define APU5_GPIO_LINE_RESETM1		4
> +#define APU5_GPIO_LINE_RESETM2		5
> +#define APU5_GPIO_LINE_RESETM3		6
> +#define APU5_GPIO_LINE_SIMSWAP1		7
> +#define APU5_GPIO_LINE_SIMSWAP2		8
> +#define APU5_GPIO_LINE_SIMSWAP3		9
>  
> -/* GPIO device */
> +
> +/* GPIO device - APU2/3/4/6 */
>  
>  static int apu2_gpio_regs[] = {
>  	[APU2_GPIO_LINE_LED1]		= APU2_GPIO_REG_LED1,
>  	[APU2_GPIO_LINE_LED2]		= APU2_GPIO_REG_LED2,
>  	[APU2_GPIO_LINE_LED3]		= APU2_GPIO_REG_LED3,
>  	[APU2_GPIO_LINE_MODESW]		= APU2_GPIO_REG_MODESW,
> +	[APU2_GPIO_LINE_RESETM1]	= APU2_GPIO_REG_RESETM1,
> +	[APU2_GPIO_LINE_RESETM2]	= APU2_GPIO_REG_RESETM2,
>  	[APU2_GPIO_LINE_SIMSWAP]	= APU2_GPIO_REG_SIMSWAP,
> -	[APU2_GPIO_LINE_MPCIE2]		= APU2_GPIO_REG_MPCIE2,
> -	[APU2_GPIO_LINE_MPCIE3]		= APU2_GPIO_REG_MPCIE3,
>  };
>  
>  static const char * const apu2_gpio_names[] = {
> @@ -61,9 +94,9 @@ static const char * const apu2_gpio_names[] = {
>  	[APU2_GPIO_LINE_LED2]		= "front-led2",
>  	[APU2_GPIO_LINE_LED3]		= "front-led3",
>  	[APU2_GPIO_LINE_MODESW]		= "front-button",
> +	[APU2_GPIO_LINE_RESETM1]	= "modem1-reset",
> +	[APU2_GPIO_LINE_RESETM2]	= "modem2-reset",
>  	[APU2_GPIO_LINE_SIMSWAP]	= "simswap",
> -	[APU2_GPIO_LINE_MPCIE2]		= "mpcie2_reset",
> -	[APU2_GPIO_LINE_MPCIE3]		= "mpcie3_reset",
>  };
>  
>  static const struct amd_fch_gpio_pdata board_apu2 = {
> @@ -72,6 +105,40 @@ static const struct amd_fch_gpio_pdata board_apu2 = {
>  	.gpio_names	= apu2_gpio_names,
>  };
>  
> +/* GPIO device - APU5 */
> +
> +static int apu5_gpio_regs[] = {
> +	[APU5_GPIO_LINE_LED1]		= APU2_GPIO_REG_LED1,
> +	[APU5_GPIO_LINE_LED2]		= APU2_GPIO_REG_LED2,
> +	[APU5_GPIO_LINE_LED3]		= APU2_GPIO_REG_LED3,
> +	[APU5_GPIO_LINE_MODESW]		= APU5_GPIO_REG_MODESW,
> +	[APU5_GPIO_LINE_RESETM1]	= APU5_GPIO_REG_RESETM1,
> +	[APU5_GPIO_LINE_RESETM2]	= APU5_GPIO_REG_RESETM2,
> +	[APU5_GPIO_LINE_RESETM3]	= APU5_GPIO_REG_RESETM3,
> +	[APU5_GPIO_LINE_SIMSWAP1]	= APU5_GPIO_REG_SIMSWAP1,
> +	[APU5_GPIO_LINE_SIMSWAP2]	= APU5_GPIO_REG_SIMSWAP2,
> +	[APU5_GPIO_LINE_SIMSWAP3]	= APU5_GPIO_REG_SIMSWAP3,
> +};
> +
> +static const char * const apu5_gpio_names[] = {
> +	[APU5_GPIO_LINE_LED1]		= "front-led1",
> +	[APU5_GPIO_LINE_LED2]		= "front-led2",
> +	[APU5_GPIO_LINE_LED3]		= "front-led3",
> +	[APU5_GPIO_LINE_MODESW]		= "front-button",
> +	[APU5_GPIO_LINE_RESETM1]	= "modem1-reset",
> +	[APU5_GPIO_LINE_RESETM2]	= "modem2-reset",
> +	[APU5_GPIO_LINE_RESETM3]	= "modem3-reset",
> +	[APU5_GPIO_LINE_SIMSWAP1]	= "simswap1",
> +	[APU5_GPIO_LINE_SIMSWAP2]	= "simswap2",
> +	[APU5_GPIO_LINE_SIMSWAP3]	= "simswap3",
> +};
> +
> +static const struct amd_fch_gpio_pdata board_apu5 = {
> +	.gpio_num	= ARRAY_SIZE(apu5_gpio_regs),
> +	.gpio_reg	= apu5_gpio_regs,
> +	.gpio_names	= apu5_gpio_names,
> +};
> +
>  /* GPIO LEDs device */
>  
>  static const struct gpio_led apu2_leds[] = {
> @@ -215,6 +282,24 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
>  		},
>  		.driver_data = (void *)&board_apu2,
>  	},
> +	/* APU5 w/ mainline BIOS */
> +	{
> +		.ident		= "apu5",
> +		.matches	= {
> +			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
> +			DMI_MATCH(DMI_BOARD_NAME, "apu5")
> +		},
> +		.driver_data	= (void *)&board_apu5,
> +	},
> +	/* APU6 w/ mainline BIOS */
> +	{
> +		.ident		= "apu6",
> +		.matches	= {
> +			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
> +			DMI_MATCH(DMI_BOARD_NAME, "apu6")
> +		},
> +		.driver_data	= (void *)&board_apu2,
> +	},
>  	{}
>  };
>  
> @@ -249,7 +334,7 @@ static int __init apu_board_init(void)
>  
>  	id = dmi_first_match(apu_gpio_dmi_table);
>  	if (!id) {
> -		pr_err("failed to detect APU board via DMI\n");
> +		pr_err("No APU board detected via DMI\n");
>  		return -ENODEV;
>  	}
>  
> @@ -288,7 +373,7 @@ module_init(apu_board_init);
>  module_exit(apu_board_exit);
>  
>  MODULE_AUTHOR("Enrico Weigelt, metux IT consult <info@metux.net>");
> -MODULE_DESCRIPTION("PC Engines APUv2/APUv3 board GPIO/LEDs/keys driver");
> +MODULE_DESCRIPTION("PC Engines APUv2-6 board GPIO/LEDs/keys driver");
>  MODULE_LICENSE("GPL");
>  MODULE_DEVICE_TABLE(dmi, apu_gpio_dmi_table);
>  MODULE_ALIAS("platform:pcengines-apuv2");

Re: [PATCH v4 1/2] x86: Support APU5 & APU6 in PCEngines platform driver

[Hans de Goede]

Hi Philip,

One more remark.

On 1/14/23 00:11, Philip Prindeville wrote:
> From: Philip Prindeville <philipp@redfish-solutions.com>

I believe part of this code (the APU5 or APU6 bits?) are from
Ed Wildgoose right?

Then Ed Wildgoose should be set as author for that part,
you can use:

git commit --amend --author="Ed Wildgoose <lists@wildgooses.com>"

to set the author (of your last commit).

> 
> PCEngines make a number of SBC. APU5 has 5 mpcie slots + MSATA
> It also has support for 3x LTE modems with 6x SIM slots (pairs with a
> SIM switch device). Each mpcie slot for modems has a reset GPIO
> 
> To ensure that the naming is sane between APU2-6 the GPIOS are
> renamed to be modem1-reset, modem2-reset, etc. This is significant
> because the slots that can be reset change between APU2 and APU3/4
> 
> GPIO for simswap is moved to the end of the list as it could be dropped
> for APU2 boards (but causes no harm to leave it in, hardware could be
> added to a future rev of the board).
> 
> Structure of the GPIOs for APU5 is extremely similar to APU2-4, but
> many lines are moved around and there are simply more
> modems/resets/sim-swap lines to breakout.
> 
> Also added APU6, which is essentially APU4 with a different ethernet
> interface and SFP cage on eth0.
> 
> Signed-off-by: Ed Wildgoose <lists@wildgooses.com>
> Reviewed-by: Philip Prindeville <philipp@redfish-solutions.com>
> Reviewed-by: Andreas Eberlein <foodeas@aeberlein.de>
> Reviewed-by: Paul Spooren <paul@spooren.de>

This is missing your Signed-off-by, since this code has passed through
your hands (and might even have been modified by you) this needs both
Ed's S-o-b as well as yours.

And if you have made significant changes, you can / may want to also
add a Co-authored-by. So in essence that would mean adding these
2 tags:

Co-authored-by: Philip Prindeville <philipp@redfish-solutions.com>
Signed-off-by: Philip Prindeville <philipp@redfish-solutions.com>

I see that you have also added a Reviewed-by: Philip Prindeville,
that is fine if you are just forwarding code upstream (with maybe
some small changes but nothing significant). If you have made
significant changes however then adding a Reviewed-by to your own
work is a bit strange.

Regards,

Hans





> ---
>  drivers/platform/x86/pcengines-apuv2.c | 113 ++++++++++++++++++++++---
>  1 file changed, 99 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/platform/x86/pcengines-apuv2.c b/drivers/platform/x86/pcengines-apuv2.c
> index d063d91db9bcbe5ceb2ac641d3105df37651ac4d..8731564bab62c1e47e99adb6ec23b3de81b09069 100644
> --- a/drivers/platform/x86/pcengines-apuv2.c
> +++ b/drivers/platform/x86/pcengines-apuv2.c
> @@ -1,10 +1,11 @@
>  // SPDX-License-Identifier: GPL-2.0+
>  
>  /*
> - * PC-Engines APUv2/APUv3 board platform driver
> + * PC-Engines APUv2-6 board platform driver
>   * for GPIO buttons and LEDs
>   *
>   * Copyright (C) 2018 metux IT consult
> + * Copyright (C) 2022 Ed Wildgoose <lists@wildgooses.com>
>   * Author: Enrico Weigelt <info@metux.net>
>   */
>  
> @@ -22,38 +23,70 @@
>  #include <linux/platform_data/gpio/gpio-amd-fch.h>
>  
>  /*
> - * NOTE: this driver only supports APUv2/3 - not APUv1, as this one
> + * NOTE: this driver only supports APUv2-6 - not APUv1, as this one
>   * has completely different register layouts.
>   */
>  
> +/*
> + * There are a number of APU variants, with differing features
> + * APU2 has SIM slots 1/2 mapping to mPCIe sockets 1/2
> + * APU3/4 moved SIM slot 1 to mPCIe socket 3, ie logically reversed
> + * However, most APU3/4 have a SIM switch which we default on to reverse
> + * the order and keep physical SIM order matching physical modem order
> + * APU6 is approximately the same as APU4 with different ethernet layout
> + *
> + * APU5 has 3x SIM sockets, all with a SIM switch
> + * several GPIOs are shuffled (see schematic), including MODESW
> + */
> +
>  /* Register mappings */
>  #define APU2_GPIO_REG_LED1		AMD_FCH_GPIO_REG_GPIO57
>  #define APU2_GPIO_REG_LED2		AMD_FCH_GPIO_REG_GPIO58
>  #define APU2_GPIO_REG_LED3		AMD_FCH_GPIO_REG_GPIO59_DEVSLP1
>  #define APU2_GPIO_REG_MODESW		AMD_FCH_GPIO_REG_GPIO32_GE1
>  #define APU2_GPIO_REG_SIMSWAP		AMD_FCH_GPIO_REG_GPIO33_GE2
> -#define APU2_GPIO_REG_MPCIE2		AMD_FCH_GPIO_REG_GPIO55_DEVSLP0
> -#define APU2_GPIO_REG_MPCIE3		AMD_FCH_GPIO_REG_GPIO51
> +#define APU2_GPIO_REG_RESETM1		AMD_FCH_GPIO_REG_GPIO51
> +#define APU2_GPIO_REG_RESETM2		AMD_FCH_GPIO_REG_GPIO55_DEVSLP0
> +
> +#define APU5_GPIO_REG_MODESW		AMT_FCH_GPIO_REG_GEVT22
> +#define APU5_GPIO_REG_SIMSWAP1		AMD_FCH_GPIO_REG_GPIO68
> +#define APU5_GPIO_REG_SIMSWAP2		AMD_FCH_GPIO_REG_GPIO32_GE1
> +#define APU5_GPIO_REG_SIMSWAP3		AMD_FCH_GPIO_REG_GPIO33_GE2
> +#define APU5_GPIO_REG_RESETM1		AMD_FCH_GPIO_REG_GPIO51
> +#define APU5_GPIO_REG_RESETM2		AMD_FCH_GPIO_REG_GPIO55_DEVSLP0
> +#define APU5_GPIO_REG_RESETM3		AMD_FCH_GPIO_REG_GPIO64
>  
>  /* Order in which the GPIO lines are defined in the register list */
>  #define APU2_GPIO_LINE_LED1		0
>  #define APU2_GPIO_LINE_LED2		1
>  #define APU2_GPIO_LINE_LED3		2
>  #define APU2_GPIO_LINE_MODESW		3
> -#define APU2_GPIO_LINE_SIMSWAP		4
> -#define APU2_GPIO_LINE_MPCIE2		5
> -#define APU2_GPIO_LINE_MPCIE3		6
> +#define APU2_GPIO_LINE_RESETM1		4
> +#define APU2_GPIO_LINE_RESETM2		5
> +#define APU2_GPIO_LINE_SIMSWAP		6
> +
> +#define APU5_GPIO_LINE_LED1		0
> +#define APU5_GPIO_LINE_LED2		1
> +#define APU5_GPIO_LINE_LED3		2
> +#define APU5_GPIO_LINE_MODESW		3
> +#define APU5_GPIO_LINE_RESETM1		4
> +#define APU5_GPIO_LINE_RESETM2		5
> +#define APU5_GPIO_LINE_RESETM3		6
> +#define APU5_GPIO_LINE_SIMSWAP1		7
> +#define APU5_GPIO_LINE_SIMSWAP2		8
> +#define APU5_GPIO_LINE_SIMSWAP3		9
>  
> -/* GPIO device */
> +
> +/* GPIO device - APU2/3/4/6 */
>  
>  static int apu2_gpio_regs[] = {
>  	[APU2_GPIO_LINE_LED1]		= APU2_GPIO_REG_LED1,
>  	[APU2_GPIO_LINE_LED2]		= APU2_GPIO_REG_LED2,
>  	[APU2_GPIO_LINE_LED3]		= APU2_GPIO_REG_LED3,
>  	[APU2_GPIO_LINE_MODESW]		= APU2_GPIO_REG_MODESW,
> +	[APU2_GPIO_LINE_RESETM1]	= APU2_GPIO_REG_RESETM1,
> +	[APU2_GPIO_LINE_RESETM2]	= APU2_GPIO_REG_RESETM2,
>  	[APU2_GPIO_LINE_SIMSWAP]	= APU2_GPIO_REG_SIMSWAP,
> -	[APU2_GPIO_LINE_MPCIE2]		= APU2_GPIO_REG_MPCIE2,
> -	[APU2_GPIO_LINE_MPCIE3]		= APU2_GPIO_REG_MPCIE3,
>  };
>  
>  static const char * const apu2_gpio_names[] = {
> @@ -61,9 +94,9 @@ static const char * const apu2_gpio_names[] = {
>  	[APU2_GPIO_LINE_LED2]		= "front-led2",
>  	[APU2_GPIO_LINE_LED3]		= "front-led3",
>  	[APU2_GPIO_LINE_MODESW]		= "front-button",
> +	[APU2_GPIO_LINE_RESETM1]	= "modem1-reset",
> +	[APU2_GPIO_LINE_RESETM2]	= "modem2-reset",
>  	[APU2_GPIO_LINE_SIMSWAP]	= "simswap",
> -	[APU2_GPIO_LINE_MPCIE2]		= "mpcie2_reset",
> -	[APU2_GPIO_LINE_MPCIE3]		= "mpcie3_reset",
>  };
>  
>  static const struct amd_fch_gpio_pdata board_apu2 = {
> @@ -72,6 +105,40 @@ static const struct amd_fch_gpio_pdata board_apu2 = {
>  	.gpio_names	= apu2_gpio_names,
>  };
>  
> +/* GPIO device - APU5 */
> +
> +static int apu5_gpio_regs[] = {
> +	[APU5_GPIO_LINE_LED1]		= APU2_GPIO_REG_LED1,
> +	[APU5_GPIO_LINE_LED2]		= APU2_GPIO_REG_LED2,
> +	[APU5_GPIO_LINE_LED3]		= APU2_GPIO_REG_LED3,
> +	[APU5_GPIO_LINE_MODESW]		= APU5_GPIO_REG_MODESW,
> +	[APU5_GPIO_LINE_RESETM1]	= APU5_GPIO_REG_RESETM1,
> +	[APU5_GPIO_LINE_RESETM2]	= APU5_GPIO_REG_RESETM2,
> +	[APU5_GPIO_LINE_RESETM3]	= APU5_GPIO_REG_RESETM3,
> +	[APU5_GPIO_LINE_SIMSWAP1]	= APU5_GPIO_REG_SIMSWAP1,
> +	[APU5_GPIO_LINE_SIMSWAP2]	= APU5_GPIO_REG_SIMSWAP2,
> +	[APU5_GPIO_LINE_SIMSWAP3]	= APU5_GPIO_REG_SIMSWAP3,
> +};
> +
> +static const char * const apu5_gpio_names[] = {
> +	[APU5_GPIO_LINE_LED1]		= "front-led1",
> +	[APU5_GPIO_LINE_LED2]		= "front-led2",
> +	[APU5_GPIO_LINE_LED3]		= "front-led3",
> +	[APU5_GPIO_LINE_MODESW]		= "front-button",
> +	[APU5_GPIO_LINE_RESETM1]	= "modem1-reset",
> +	[APU5_GPIO_LINE_RESETM2]	= "modem2-reset",
> +	[APU5_GPIO_LINE_RESETM3]	= "modem3-reset",
> +	[APU5_GPIO_LINE_SIMSWAP1]	= "simswap1",
> +	[APU5_GPIO_LINE_SIMSWAP2]	= "simswap2",
> +	[APU5_GPIO_LINE_SIMSWAP3]	= "simswap3",
> +};
> +
> +static const struct amd_fch_gpio_pdata board_apu5 = {
> +	.gpio_num	= ARRAY_SIZE(apu5_gpio_regs),
> +	.gpio_reg	= apu5_gpio_regs,
> +	.gpio_names	= apu5_gpio_names,
> +};
> +
>  /* GPIO LEDs device */
>  
>  static const struct gpio_led apu2_leds[] = {
> @@ -215,6 +282,24 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
>  		},
>  		.driver_data = (void *)&board_apu2,
>  	},
> +	/* APU5 w/ mainline BIOS */
> +	{
> +		.ident		= "apu5",
> +		.matches	= {
> +			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
> +			DMI_MATCH(DMI_BOARD_NAME, "apu5")
> +		},
> +		.driver_data	= (void *)&board_apu5,
> +	},
> +	/* APU6 w/ mainline BIOS */
> +	{
> +		.ident		= "apu6",
> +		.matches	= {
> +			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
> +			DMI_MATCH(DMI_BOARD_NAME, "apu6")
> +		},
> +		.driver_data	= (void *)&board_apu2,
> +	},
>  	{}
>  };
>  
> @@ -249,7 +334,7 @@ static int __init apu_board_init(void)
>  
>  	id = dmi_first_match(apu_gpio_dmi_table);
>  	if (!id) {
> -		pr_err("failed to detect APU board via DMI\n");
> +		pr_err("No APU board detected via DMI\n");
>  		return -ENODEV;
>  	}
>  
> @@ -288,7 +373,7 @@ module_init(apu_board_init);
>  module_exit(apu_board_exit);
>  
>  MODULE_AUTHOR("Enrico Weigelt, metux IT consult <info@metux.net>");
> -MODULE_DESCRIPTION("PC Engines APUv2/APUv3 board GPIO/LEDs/keys driver");
> +MODULE_DESCRIPTION("PC Engines APUv2-6 board GPIO/LEDs/keys driver");
>  MODULE_LICENSE("GPL");
>  MODULE_DEVICE_TABLE(dmi, apu_gpio_dmi_table);
>  MODULE_ALIAS("platform:pcengines-apuv2");

Re: [PATCH v4 1/2] x86: Support APU5 & APU6 in PCEngines platform driver

[Philip Prindeville]

> On Jan 19, 2023, at 3:22 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> 
> Hi,
> 
> On 1/14/23 00:11, Philip Prindeville wrote:
>> From: Philip Prindeville <philipp@redfish-solutions.com>
>> 
>> PCEngines make a number of SBC. APU5 has 5 mpcie slots + MSATA
>> It also has support for 3x LTE modems with 6x SIM slots (pairs with a
>> SIM switch device). Each mpcie slot for modems has a reset GPIO
>> 
>> To ensure that the naming is sane between APU2-6 the GPIOS are
>> renamed to be modem1-reset, modem2-reset, etc. This is significant
>> because the slots that can be reset change between APU2 and APU3/4
>> 
>> GPIO for simswap is moved to the end of the list as it could be dropped
>> for APU2 boards (but causes no harm to leave it in, hardware could be
>> added to a future rev of the board).
>> 
>> Structure of the GPIOs for APU5 is extremely similar to APU2-4, but
>> many lines are moved around and there are simply more
>> modems/resets/sim-swap lines to breakout.
>> 
>> Also added APU6, which is essentially APU4 with a different ethernet
>> interface and SFP cage on eth0.
>> 
>> Signed-off-by: Ed Wildgoose <lists@wildgooses.com>
>> Reviewed-by: Philip Prindeville <philipp@redfish-solutions.com>
>> Reviewed-by: Andreas Eberlein <foodeas@aeberlein.de>
>> Reviewed-by: Paul Spooren <paul@spooren.de>
>> ---
>> drivers/platform/x86/pcengines-apuv2.c | 113 ++++++++++++++++++++++---
>> 1 file changed, 99 insertions(+), 14 deletions(-)
>> 
>> diff --git a/drivers/platform/x86/pcengines-apuv2.c b/drivers/platform/x86/pcengines-apuv2.c
>> index d063d91db9bcbe5ceb2ac641d3105df37651ac4d..8731564bab62c1e47e99adb6ec23b3de81b09069 100644
>> --- a/drivers/platform/x86/pcengines-apuv2.c
>> +++ b/drivers/platform/x86/pcengines-apuv2.c
>> @@ -1,10 +1,11 @@
>> // SPDX-License-Identifier: GPL-2.0+
>> 
>> /*
>> - * PC-Engines APUv2/APUv3 board platform driver
>> + * PC-Engines APUv2-6 board platform driver
>>  * for GPIO buttons and LEDs
>>  *
>>  * Copyright (C) 2018 metux IT consult
>> + * Copyright (C) 2022 Ed Wildgoose <lists@wildgooses.com>
>>  * Author: Enrico Weigelt <info@metux.net>
>>  */
>> 
>> @@ -22,38 +23,70 @@
>> #include <linux/platform_data/gpio/gpio-amd-fch.h>
>> 
>> /*
>> - * NOTE: this driver only supports APUv2/3 - not APUv1, as this one
>> + * NOTE: this driver only supports APUv2-6 - not APUv1, as this one
>>  * has completely different register layouts.
>>  */
>> 
>> +/*
>> + * There are a number of APU variants, with differing features
>> + * APU2 has SIM slots 1/2 mapping to mPCIe sockets 1/2
>> + * APU3/4 moved SIM slot 1 to mPCIe socket 3, ie logically reversed
>> + * However, most APU3/4 have a SIM switch which we default on to reverse
>> + * the order and keep physical SIM order matching physical modem order
>> + * APU6 is approximately the same as APU4 with different ethernet layout
>> + *
>> + * APU5 has 3x SIM sockets, all with a SIM switch
>> + * several GPIOs are shuffled (see schematic), including MODESW
>> + */
>> +
>> /* Register mappings */
>> #define APU2_GPIO_REG_LED1 AMD_FCH_GPIO_REG_GPIO57
>> #define APU2_GPIO_REG_LED2 AMD_FCH_GPIO_REG_GPIO58
>> #define APU2_GPIO_REG_LED3 AMD_FCH_GPIO_REG_GPIO59_DEVSLP1
>> #define APU2_GPIO_REG_MODESW AMD_FCH_GPIO_REG_GPIO32_GE1
>> #define APU2_GPIO_REG_SIMSWAP AMD_FCH_GPIO_REG_GPIO33_GE2
>> -#define APU2_GPIO_REG_MPCIE2 AMD_FCH_GPIO_REG_GPIO55_DEVSLP0
>> -#define APU2_GPIO_REG_MPCIE3 AMD_FCH_GPIO_REG_GPIO51
>> +#define APU2_GPIO_REG_RESETM1 AMD_FCH_GPIO_REG_GPIO51
>> +#define APU2_GPIO_REG_RESETM2 AMD_FCH_GPIO_REG_GPIO55_DEVSLP0
>> +
>> +#define APU5_GPIO_REG_MODESW AMT_FCH_GPIO_REG_GEVT22
>> +#define APU5_GPIO_REG_SIMSWAP1 AMD_FCH_GPIO_REG_GPIO68
>> +#define APU5_GPIO_REG_SIMSWAP2 AMD_FCH_GPIO_REG_GPIO32_GE1
>> +#define APU5_GPIO_REG_SIMSWAP3 AMD_FCH_GPIO_REG_GPIO33_GE2
>> +#define APU5_GPIO_REG_RESETM1 AMD_FCH_GPIO_REG_GPIO51
>> +#define APU5_GPIO_REG_RESETM2 AMD_FCH_GPIO_REG_GPIO55_DEVSLP0
>> +#define APU5_GPIO_REG_RESETM3 AMD_FCH_GPIO_REG_GPIO64
>> 
>> /* Order in which the GPIO lines are defined in the register list */
>> #define APU2_GPIO_LINE_LED1 0
>> #define APU2_GPIO_LINE_LED2 1
>> #define APU2_GPIO_LINE_LED3 2
>> #define APU2_GPIO_LINE_MODESW 3
>> -#define APU2_GPIO_LINE_SIMSWAP 4
>> -#define APU2_GPIO_LINE_MPCIE2 5
>> -#define APU2_GPIO_LINE_MPCIE3 6
>> +#define APU2_GPIO_LINE_RESETM1 4
>> +#define APU2_GPIO_LINE_RESETM2 5
>> +#define APU2_GPIO_LINE_SIMSWAP 6
> 
> I don't think this changing of GPIO ordering, or
> for that part the changing of the gpio_names from 
> "mpcie2_reset" to "modem1-reset" is a good idea.
> 
> I'm not entirely sure how these GPIOs are supposed to be
> consumed / used by userspace. But since they are not used
> directly in this driver I assume userspace is supposed to
> use either the (deprecated) sysfs GPIO API or the new ioctl
> based GPIO API to toggle say "simswap" if it needs to.
> 
> The old sysfs API exclusively uses pin-indexes inside a GPIO
> chip to select the pin, so by changing the pin order you
> have just broken the userspace API.
> 
> And the new ioctl API can use either pin-indexes or GPIO-line-names,
> so by changing the names you have also just potentially broken
> that.
> 
> Please keep the order as is and only use the new names for
> the newly added models (so for APU6 I believe).
> 
> I guess that means adding a new apu6_gpio_names[]
> array for APU6, that is fine.
> 
> Note you can still do the "MPCIE2" -> "RESETM1" defines
> if you want, to allow reusing the defines and the apu2_gpio_regs[]
> array. But in order to not brake uAPI you must:
> 
> 1. Not change the order of the pins
> 2. Keep the "mpcie2_reset" and "mpcie3_reset" names for the
>   already supported models.
> 
> Also can you please split this patch into 2 patches,
> 1 adding the APU5 support and one adding the APU6 support
> (not necessarily in that order) ?
> 
> Regards,
> 
> Hans
> 
> 
> p.s.
> 
> About the bounces I'm getting bounces for linux-x86_64@vger.kernel.org
> (dropped) but not for Enrico's email.


I can't speak for Ed, but I've gotten pre-production boards in the past where the pin assignments have changed between the rev 0.1 board that I got to write drivers for and do BSP, and what went to production.

-Philip


> 
> 
>> +
>> +#define APU5_GPIO_LINE_LED1 0
>> +#define APU5_GPIO_LINE_LED2 1
>> +#define APU5_GPIO_LINE_LED3 2
>> +#define APU5_GPIO_LINE_MODESW 3
>> +#define APU5_GPIO_LINE_RESETM1 4
>> +#define APU5_GPIO_LINE_RESETM2 5
>> +#define APU5_GPIO_LINE_RESETM3 6
>> +#define APU5_GPIO_LINE_SIMSWAP1 7
>> +#define APU5_GPIO_LINE_SIMSWAP2 8
>> +#define APU5_GPIO_LINE_SIMSWAP3 9
>> 
>> -/* GPIO device */
>> +
>> +/* GPIO device - APU2/3/4/6 */
>> 
>> static int apu2_gpio_regs[] = {
>> [APU2_GPIO_LINE_LED1] = APU2_GPIO_REG_LED1,
>> [APU2_GPIO_LINE_LED2] = APU2_GPIO_REG_LED2,
>> [APU2_GPIO_LINE_LED3] = APU2_GPIO_REG_LED3,
>> [APU2_GPIO_LINE_MODESW] = APU2_GPIO_REG_MODESW,
>> + [APU2_GPIO_LINE_RESETM1] = APU2_GPIO_REG_RESETM1,
>> + [APU2_GPIO_LINE_RESETM2] = APU2_GPIO_REG_RESETM2,
>> [APU2_GPIO_LINE_SIMSWAP] = APU2_GPIO_REG_SIMSWAP,
>> - [APU2_GPIO_LINE_MPCIE2] = APU2_GPIO_REG_MPCIE2,
>> - [APU2_GPIO_LINE_MPCIE3] = APU2_GPIO_REG_MPCIE3,
>> };
>> 
>> static const char * const apu2_gpio_names[] = {
>> @@ -61,9 +94,9 @@ static const char * const apu2_gpio_names[] = {
>> [APU2_GPIO_LINE_LED2] = "front-led2",
>> [APU2_GPIO_LINE_LED3] = "front-led3",
>> [APU2_GPIO_LINE_MODESW] = "front-button",
>> + [APU2_GPIO_LINE_RESETM1] = "modem1-reset",
>> + [APU2_GPIO_LINE_RESETM2] = "modem2-reset",
>> [APU2_GPIO_LINE_SIMSWAP] = "simswap",
>> - [APU2_GPIO_LINE_MPCIE2] = "mpcie2_reset",
>> - [APU2_GPIO_LINE_MPCIE3] = "mpcie3_reset",
>> };
>> 
>> static const struct amd_fch_gpio_pdata board_apu2 = {
>> @@ -72,6 +105,40 @@ static const struct amd_fch_gpio_pdata board_apu2 = {
>> .gpio_names = apu2_gpio_names,
>> };
>> 
>> +/* GPIO device - APU5 */
>> +
>> +static int apu5_gpio_regs[] = {
>> + [APU5_GPIO_LINE_LED1] = APU2_GPIO_REG_LED1,
>> + [APU5_GPIO_LINE_LED2] = APU2_GPIO_REG_LED2,
>> + [APU5_GPIO_LINE_LED3] = APU2_GPIO_REG_LED3,
>> + [APU5_GPIO_LINE_MODESW] = APU5_GPIO_REG_MODESW,
>> + [APU5_GPIO_LINE_RESETM1] = APU5_GPIO_REG_RESETM1,
>> + [APU5_GPIO_LINE_RESETM2] = APU5_GPIO_REG_RESETM2,
>> + [APU5_GPIO_LINE_RESETM3] = APU5_GPIO_REG_RESETM3,
>> + [APU5_GPIO_LINE_SIMSWAP1] = APU5_GPIO_REG_SIMSWAP1,
>> + [APU5_GPIO_LINE_SIMSWAP2] = APU5_GPIO_REG_SIMSWAP2,
>> + [APU5_GPIO_LINE_SIMSWAP3] = APU5_GPIO_REG_SIMSWAP3,
>> +};
>> +
>> +static const char * const apu5_gpio_names[] = {
>> + [APU5_GPIO_LINE_LED1] = "front-led1",
>> + [APU5_GPIO_LINE_LED2] = "front-led2",
>> + [APU5_GPIO_LINE_LED3] = "front-led3",
>> + [APU5_GPIO_LINE_MODESW] = "front-button",
>> + [APU5_GPIO_LINE_RESETM1] = "modem1-reset",
>> + [APU5_GPIO_LINE_RESETM2] = "modem2-reset",
>> + [APU5_GPIO_LINE_RESETM3] = "modem3-reset",
>> + [APU5_GPIO_LINE_SIMSWAP1] = "simswap1",
>> + [APU5_GPIO_LINE_SIMSWAP2] = "simswap2",
>> + [APU5_GPIO_LINE_SIMSWAP3] = "simswap3",
>> +};
>> +
>> +static const struct amd_fch_gpio_pdata board_apu5 = {
>> + .gpio_num = ARRAY_SIZE(apu5_gpio_regs),
>> + .gpio_reg = apu5_gpio_regs,
>> + .gpio_names = apu5_gpio_names,
>> +};
>> +
>> /* GPIO LEDs device */
>> 
>> static const struct gpio_led apu2_leds[] = {
>> @@ -215,6 +282,24 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
>> },
>> .driver_data = (void *)&board_apu2,
>> },
>> + /* APU5 w/ mainline BIOS */
>> + {
>> + .ident = "apu5",
>> + .matches = {
>> + DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
>> + DMI_MATCH(DMI_BOARD_NAME, "apu5")
>> + },
>> + .driver_data = (void *)&board_apu5,
>> + },
>> + /* APU6 w/ mainline BIOS */
>> + {
>> + .ident = "apu6",
>> + .matches = {
>> + DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
>> + DMI_MATCH(DMI_BOARD_NAME, "apu6")
>> + },
>> + .driver_data = (void *)&board_apu2,
>> + },
>> {}
>> };
>> 
>> @@ -249,7 +334,7 @@ static int __init apu_board_init(void)
>> 
>> id = dmi_first_match(apu_gpio_dmi_table);
>> if (!id) {
>> - pr_err("failed to detect APU board via DMI\n");
>> + pr_err("No APU board detected via DMI\n");
>> return -ENODEV;
>> }
>> 
>> @@ -288,7 +373,7 @@ module_init(apu_board_init);
>> module_exit(apu_board_exit);
>> 
>> MODULE_AUTHOR("Enrico Weigelt, metux IT consult <info@metux.net>");
>> -MODULE_DESCRIPTION("PC Engines APUv2/APUv3 board GPIO/LEDs/keys driver");
>> +MODULE_DESCRIPTION("PC Engines APUv2-6 board GPIO/LEDs/keys driver");
>> MODULE_LICENSE("GPL");
>> MODULE_DEVICE_TABLE(dmi, apu_gpio_dmi_table);
>> MODULE_ALIAS("platform:pcengines-apuv2");
> 

Re: [PATCH v4 1/2] x86: Support APU5 & APU6 in PCEngines platform driver

[Hans de Goede]

Hi,

On 1/20/23 06:34, Philip Prindeville wrote:
> 
> 
>> On Jan 19, 2023, at 3:22 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 1/14/23 00:11, Philip Prindeville wrote:
>>> From: Philip Prindeville <philipp@redfish-solutions.com>
>>>
>>> PCEngines make a number of SBC. APU5 has 5 mpcie slots + MSATA
>>> It also has support for 3x LTE modems with 6x SIM slots (pairs with a
>>> SIM switch device). Each mpcie slot for modems has a reset GPIO
>>>
>>> To ensure that the naming is sane between APU2-6 the GPIOS are
>>> renamed to be modem1-reset, modem2-reset, etc. This is significant
>>> because the slots that can be reset change between APU2 and APU3/4
>>>
>>> GPIO for simswap is moved to the end of the list as it could be dropped
>>> for APU2 boards (but causes no harm to leave it in, hardware could be
>>> added to a future rev of the board).
>>>
>>> Structure of the GPIOs for APU5 is extremely similar to APU2-4, but
>>> many lines are moved around and there are simply more
>>> modems/resets/sim-swap lines to breakout.
>>>
>>> Also added APU6, which is essentially APU4 with a different ethernet
>>> interface and SFP cage on eth0.
>>>
>>> Signed-off-by: Ed Wildgoose <lists@wildgooses.com>
>>> Reviewed-by: Philip Prindeville <philipp@redfish-solutions.com>
>>> Reviewed-by: Andreas Eberlein <foodeas@aeberlein.de>
>>> Reviewed-by: Paul Spooren <paul@spooren.de>
>>> ---
>>> drivers/platform/x86/pcengines-apuv2.c | 113 ++++++++++++++++++++++---
>>> 1 file changed, 99 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/pcengines-apuv2.c b/drivers/platform/x86/pcengines-apuv2.c
>>> index d063d91db9bcbe5ceb2ac641d3105df37651ac4d..8731564bab62c1e47e99adb6ec23b3de81b09069 100644
>>> --- a/drivers/platform/x86/pcengines-apuv2.c
>>> +++ b/drivers/platform/x86/pcengines-apuv2.c
>>> @@ -1,10 +1,11 @@
>>> // SPDX-License-Identifier: GPL-2.0+
>>>
>>> /*
>>> - * PC-Engines APUv2/APUv3 board platform driver
>>> + * PC-Engines APUv2-6 board platform driver
>>>  * for GPIO buttons and LEDs
>>>  *
>>>  * Copyright (C) 2018 metux IT consult
>>> + * Copyright (C) 2022 Ed Wildgoose <lists@wildgooses.com>
>>>  * Author: Enrico Weigelt <info@metux.net>
>>>  */
>>>
>>> @@ -22,38 +23,70 @@
>>> #include <linux/platform_data/gpio/gpio-amd-fch.h>
>>>
>>> /*
>>> - * NOTE: this driver only supports APUv2/3 - not APUv1, as this one
>>> + * NOTE: this driver only supports APUv2-6 - not APUv1, as this one
>>>  * has completely different register layouts.
>>>  */
>>>
>>> +/*
>>> + * There are a number of APU variants, with differing features
>>> + * APU2 has SIM slots 1/2 mapping to mPCIe sockets 1/2
>>> + * APU3/4 moved SIM slot 1 to mPCIe socket 3, ie logically reversed
>>> + * However, most APU3/4 have a SIM switch which we default on to reverse
>>> + * the order and keep physical SIM order matching physical modem order
>>> + * APU6 is approximately the same as APU4 with different ethernet layout
>>> + *
>>> + * APU5 has 3x SIM sockets, all with a SIM switch
>>> + * several GPIOs are shuffled (see schematic), including MODESW
>>> + */
>>> +
>>> /* Register mappings */
>>> #define APU2_GPIO_REG_LED1 AMD_FCH_GPIO_REG_GPIO57
>>> #define APU2_GPIO_REG_LED2 AMD_FCH_GPIO_REG_GPIO58
>>> #define APU2_GPIO_REG_LED3 AMD_FCH_GPIO_REG_GPIO59_DEVSLP1
>>> #define APU2_GPIO_REG_MODESW AMD_FCH_GPIO_REG_GPIO32_GE1
>>> #define APU2_GPIO_REG_SIMSWAP AMD_FCH_GPIO_REG_GPIO33_GE2
>>> -#define APU2_GPIO_REG_MPCIE2 AMD_FCH_GPIO_REG_GPIO55_DEVSLP0
>>> -#define APU2_GPIO_REG_MPCIE3 AMD_FCH_GPIO_REG_GPIO51
>>> +#define APU2_GPIO_REG_RESETM1 AMD_FCH_GPIO_REG_GPIO51
>>> +#define APU2_GPIO_REG_RESETM2 AMD_FCH_GPIO_REG_GPIO55_DEVSLP0
>>> +
>>> +#define APU5_GPIO_REG_MODESW AMT_FCH_GPIO_REG_GEVT22
>>> +#define APU5_GPIO_REG_SIMSWAP1 AMD_FCH_GPIO_REG_GPIO68
>>> +#define APU5_GPIO_REG_SIMSWAP2 AMD_FCH_GPIO_REG_GPIO32_GE1
>>> +#define APU5_GPIO_REG_SIMSWAP3 AMD_FCH_GPIO_REG_GPIO33_GE2
>>> +#define APU5_GPIO_REG_RESETM1 AMD_FCH_GPIO_REG_GPIO51
>>> +#define APU5_GPIO_REG_RESETM2 AMD_FCH_GPIO_REG_GPIO55_DEVSLP0
>>> +#define APU5_GPIO_REG_RESETM3 AMD_FCH_GPIO_REG_GPIO64
>>>
>>> /* Order in which the GPIO lines are defined in the register list */
>>> #define APU2_GPIO_LINE_LED1 0
>>> #define APU2_GPIO_LINE_LED2 1
>>> #define APU2_GPIO_LINE_LED3 2
>>> #define APU2_GPIO_LINE_MODESW 3
>>> -#define APU2_GPIO_LINE_SIMSWAP 4
>>> -#define APU2_GPIO_LINE_MPCIE2 5
>>> -#define APU2_GPIO_LINE_MPCIE3 6
>>> +#define APU2_GPIO_LINE_RESETM1 4
>>> +#define APU2_GPIO_LINE_RESETM2 5
>>> +#define APU2_GPIO_LINE_SIMSWAP 6
>>
>> I don't think this changing of GPIO ordering, or
>> for that part the changing of the gpio_names from 
>> "mpcie2_reset" to "modem1-reset" is a good idea.
>>
>> I'm not entirely sure how these GPIOs are supposed to be
>> consumed / used by userspace. But since they are not used
>> directly in this driver I assume userspace is supposed to
>> use either the (deprecated) sysfs GPIO API or the new ioctl
>> based GPIO API to toggle say "simswap" if it needs to.
>>
>> The old sysfs API exclusively uses pin-indexes inside a GPIO
>> chip to select the pin, so by changing the pin order you
>> have just broken the userspace API.
>>
>> And the new ioctl API can use either pin-indexes or GPIO-line-names,
>> so by changing the names you have also just potentially broken
>> that.
>>
>> Please keep the order as is and only use the new names for
>> the newly added models (so for APU6 I believe).
>>
>> I guess that means adding a new apu6_gpio_names[]
>> array for APU6, that is fine.
>>
>> Note you can still do the "MPCIE2" -> "RESETM1" defines
>> if you want, to allow reusing the defines and the apu2_gpio_regs[]
>> array. But in order to not brake uAPI you must:
>>
>> 1. Not change the order of the pins
>> 2. Keep the "mpcie2_reset" and "mpcie3_reset" names for the
>>   already supported models.
>>
>> Also can you please split this patch into 2 patches,
>> 1 adding the APU5 support and one adding the APU6 support
>> (not necessarily in that order) ?
>>
>> Regards,
>>
>> Hans
>>
>>
>> p.s.
>>
>> About the bounces I'm getting bounces for linux-x86_64@vger.kernel.org
>> (dropped) but not for Enrico's email.
> 
> 
> I can't speak for Ed, but I've gotten pre-production boards in the past where the pin assignments have changed between the rev 0.1 board that I got to write drivers for and do BSP, and what went to production.

Sure hw vendors may do such things, but we are not talking hw here,
in this case the order in which we specify the data passed to
gpio_amd_fch is the pin order.

And normally it would be a bit doubtful if pin-order is part of
the userspace API, since normally the GPIOs are wrapped in
higher-level userspace APIs like e.g. rfkill. Which makes them
unavailable for userspace.

But in this case AFAICT the only API userspace has for the
simswap and modem-reset signals is "raw" GPIO access in
which case the ordering very much matters.

Regards,

Hans




>>> +
>>> +#define APU5_GPIO_LINE_LED1 0
>>> +#define APU5_GPIO_LINE_LED2 1
>>> +#define APU5_GPIO_LINE_LED3 2
>>> +#define APU5_GPIO_LINE_MODESW 3
>>> +#define APU5_GPIO_LINE_RESETM1 4
>>> +#define APU5_GPIO_LINE_RESETM2 5
>>> +#define APU5_GPIO_LINE_RESETM3 6
>>> +#define APU5_GPIO_LINE_SIMSWAP1 7
>>> +#define APU5_GPIO_LINE_SIMSWAP2 8
>>> +#define APU5_GPIO_LINE_SIMSWAP3 9
>>>
>>> -/* GPIO device */
>>> +
>>> +/* GPIO device - APU2/3/4/6 */
>>>
>>> static int apu2_gpio_regs[] = {
>>> [APU2_GPIO_LINE_LED1] = APU2_GPIO_REG_LED1,
>>> [APU2_GPIO_LINE_LED2] = APU2_GPIO_REG_LED2,
>>> [APU2_GPIO_LINE_LED3] = APU2_GPIO_REG_LED3,
>>> [APU2_GPIO_LINE_MODESW] = APU2_GPIO_REG_MODESW,
>>> + [APU2_GPIO_LINE_RESETM1] = APU2_GPIO_REG_RESETM1,
>>> + [APU2_GPIO_LINE_RESETM2] = APU2_GPIO_REG_RESETM2,
>>> [APU2_GPIO_LINE_SIMSWAP] = APU2_GPIO_REG_SIMSWAP,
>>> - [APU2_GPIO_LINE_MPCIE2] = APU2_GPIO_REG_MPCIE2,
>>> - [APU2_GPIO_LINE_MPCIE3] = APU2_GPIO_REG_MPCIE3,
>>> };
>>>
>>> static const char * const apu2_gpio_names[] = {
>>> @@ -61,9 +94,9 @@ static const char * const apu2_gpio_names[] = {
>>> [APU2_GPIO_LINE_LED2] = "front-led2",
>>> [APU2_GPIO_LINE_LED3] = "front-led3",
>>> [APU2_GPIO_LINE_MODESW] = "front-button",
>>> + [APU2_GPIO_LINE_RESETM1] = "modem1-reset",
>>> + [APU2_GPIO_LINE_RESETM2] = "modem2-reset",
>>> [APU2_GPIO_LINE_SIMSWAP] = "simswap",
>>> - [APU2_GPIO_LINE_MPCIE2] = "mpcie2_reset",
>>> - [APU2_GPIO_LINE_MPCIE3] = "mpcie3_reset",
>>> };
>>>
>>> static const struct amd_fch_gpio_pdata board_apu2 = {
>>> @@ -72,6 +105,40 @@ static const struct amd_fch_gpio_pdata board_apu2 = {
>>> .gpio_names = apu2_gpio_names,
>>> };
>>>
>>> +/* GPIO device - APU5 */
>>> +
>>> +static int apu5_gpio_regs[] = {
>>> + [APU5_GPIO_LINE_LED1] = APU2_GPIO_REG_LED1,
>>> + [APU5_GPIO_LINE_LED2] = APU2_GPIO_REG_LED2,
>>> + [APU5_GPIO_LINE_LED3] = APU2_GPIO_REG_LED3,
>>> + [APU5_GPIO_LINE_MODESW] = APU5_GPIO_REG_MODESW,
>>> + [APU5_GPIO_LINE_RESETM1] = APU5_GPIO_REG_RESETM1,
>>> + [APU5_GPIO_LINE_RESETM2] = APU5_GPIO_REG_RESETM2,
>>> + [APU5_GPIO_LINE_RESETM3] = APU5_GPIO_REG_RESETM3,
>>> + [APU5_GPIO_LINE_SIMSWAP1] = APU5_GPIO_REG_SIMSWAP1,
>>> + [APU5_GPIO_LINE_SIMSWAP2] = APU5_GPIO_REG_SIMSWAP2,
>>> + [APU5_GPIO_LINE_SIMSWAP3] = APU5_GPIO_REG_SIMSWAP3,
>>> +};
>>> +
>>> +static const char * const apu5_gpio_names[] = {
>>> + [APU5_GPIO_LINE_LED1] = "front-led1",
>>> + [APU5_GPIO_LINE_LED2] = "front-led2",
>>> + [APU5_GPIO_LINE_LED3] = "front-led3",
>>> + [APU5_GPIO_LINE_MODESW] = "front-button",
>>> + [APU5_GPIO_LINE_RESETM1] = "modem1-reset",
>>> + [APU5_GPIO_LINE_RESETM2] = "modem2-reset",
>>> + [APU5_GPIO_LINE_RESETM3] = "modem3-reset",
>>> + [APU5_GPIO_LINE_SIMSWAP1] = "simswap1",
>>> + [APU5_GPIO_LINE_SIMSWAP2] = "simswap2",
>>> + [APU5_GPIO_LINE_SIMSWAP3] = "simswap3",
>>> +};
>>> +
>>> +static const struct amd_fch_gpio_pdata board_apu5 = {
>>> + .gpio_num = ARRAY_SIZE(apu5_gpio_regs),
>>> + .gpio_reg = apu5_gpio_regs,
>>> + .gpio_names = apu5_gpio_names,
>>> +};
>>> +
>>> /* GPIO LEDs device */
>>>
>>> static const struct gpio_led apu2_leds[] = {
>>> @@ -215,6 +282,24 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
>>> },
>>> .driver_data = (void *)&board_apu2,
>>> },
>>> + /* APU5 w/ mainline BIOS */
>>> + {
>>> + .ident = "apu5",
>>> + .matches = {
>>> + DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
>>> + DMI_MATCH(DMI_BOARD_NAME, "apu5")
>>> + },
>>> + .driver_data = (void *)&board_apu5,
>>> + },
>>> + /* APU6 w/ mainline BIOS */
>>> + {
>>> + .ident = "apu6",
>>> + .matches = {
>>> + DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
>>> + DMI_MATCH(DMI_BOARD_NAME, "apu6")
>>> + },
>>> + .driver_data = (void *)&board_apu2,
>>> + },
>>> {}
>>> };
>>>
>>> @@ -249,7 +334,7 @@ static int __init apu_board_init(void)
>>>
>>> id = dmi_first_match(apu_gpio_dmi_table);
>>> if (!id) {
>>> - pr_err("failed to detect APU board via DMI\n");
>>> + pr_err("No APU board detected via DMI\n");
>>> return -ENODEV;
>>> }
>>>
>>> @@ -288,7 +373,7 @@ module_init(apu_board_init);
>>> module_exit(apu_board_exit);
>>>
>>> MODULE_AUTHOR("Enrico Weigelt, metux IT consult <info@metux.net>");
>>> -MODULE_DESCRIPTION("PC Engines APUv2/APUv3 board GPIO/LEDs/keys driver");
>>> +MODULE_DESCRIPTION("PC Engines APUv2-6 board GPIO/LEDs/keys driver");
>>> MODULE_LICENSE("GPL");
>>> MODULE_DEVICE_TABLE(dmi, apu_gpio_dmi_table);
>>> MODULE_ALIAS("platform:pcengines-apuv2");
>>
> 

Re: [PATCH v4 1/2] x86: Support APU5 & APU6 in PCEngines platform driver

[Ed W]

On 19/01/2023 10:22, Hans de Goede wrote:

>>  /* Order in which the GPIO lines are defined in the register list */
>>  #define APU2_GPIO_LINE_LED1		0
>>  #define APU2_GPIO_LINE_LED2		1
>>  #define APU2_GPIO_LINE_LED3		2
>>  #define APU2_GPIO_LINE_MODESW		3
>> -#define APU2_GPIO_LINE_SIMSWAP		4
>> -#define APU2_GPIO_LINE_MPCIE2		5
>> -#define APU2_GPIO_LINE_MPCIE3		6
>> +#define APU2_GPIO_LINE_RESETM1		4
>> +#define APU2_GPIO_LINE_RESETM2		5
>> +#define APU2_GPIO_LINE_SIMSWAP		6
> I don't think this changing of GPIO ordering, or
> for that part the changing of the gpio_names from 
> "mpcie2_reset" to "modem1-reset" is a good idea.
>
> I'm not entirely sure how these GPIOs are supposed to be
> consumed / used by userspace. But since they are not used
> directly in this driver I assume userspace is supposed to
> use either the (deprecated) sysfs GPIO API or the new ioctl
> based GPIO API to toggle say "simswap" if it needs to.
>
> The old sysfs API exclusively uses pin-indexes inside a GPIO
> chip to select the pin, so by changing the pin order you
> have just broken the userspace API.
>
> And the new ioctl API can use either pin-indexes or GPIO-line-names,
> so by changing the names you have also just potentially broken
> that.
>
> Please keep the order as is and only use the new names for
> the newly added models (so for APU6 I believe).


Hi, I'm not sure what the "correct" thing to do is, but just to add some background to the situation:

There are an increasing number of APU boards, which are *very* similar, and also through time the
pin allocations have muddled around, plus most recently, the BIOS can configure many things and has
started to use naming conventions different to the historic kernel naming

So I don't have a board in front of me to be definitive, but something like the following happened:

- APU2 used something like mpcie sockets 1&2 for USB stuff and hence LTE cards, socket 3 was msata

- Then another version APU3, I think moved these to sockets 2&3

- Then another version APU4, moved the USB to sockets 2&3 and wired up a second SIM slot in most
versions, including a SIM line swapper chip. Now you start to wonder if you should have labelled
things PCIE1, PCIE2, PCIE3, etc, when really they mean modem 1 and modem 2, etc?

- Then came APU5, which has 3x USB sockets, plus 3x mpcie sockets. These are wired to different pcie
numbers, and so the naming modem1, modem2, modem3 starts to make a lot more sense.

- APU6, which is mentioned in the original patch, is really just the same as one of the other
boards, but with different ethernet sockets (SFP vs copper)


- There is also a rare feature, which is likely not known to most users, or even wired up correctly
on many boards. You have a reset/enable line to some of the mpcie slots. This again makes more sense
to label logically vs than per slot. It's really not clear that this feature is properly supported
or functioning on all boards (you can order special order boards wired in various ways). So changes
here are unlikely to be noticed by all but a handful of specialist users.


Overall, if one could start again, the unifying feature would be label slots logically, ie modem1,
modem2, wifi1, wifi2, rather than numbering them based on how they are wired on a specific board rev.

Additionally, users who didn't load the APU driver, likely had ACPI named devices and these all have
different (and to my eye, more logical names). So whatever we decide to do here will cause some
breakage and inconsistency...


Note that I submitted this previous patch "years ago", and I've somewhat given up on ever getting
the APU driver up to date.. I think in 2020, Enrico shot me down because he was working on some
grand unification for modem GPIO handling? (Enrico, please correct me on the details?) Hans, I think
if you search back to 2020 on "APU", you will see that you arbitrated in that thread? For whatever
reason, we seem to be stuck that there are competing voices blocking progress here. Every route
leads to some level of incompatibility. Personally I am a fairly large consumer of these devices,
but I really don't care what we decide, because we ship a custom software, where userspace will
match kernel, so we will update both in lockstep, whatever happens. Changes aren't a problem for me
personally.

My vote would be for a one-of breakage, to at least get everyone using the same
terms/names/whatever. I would suspect OpenWRT is probably the biggest voice here, so suggest we go
with whatever they suggest, and then at least we are all in sync for the future? If its a one off,
then suggest taking into account the ACPI naming as well?

Note, there is a very big risk that I missed the point... Please be gentle. Quite possibly there is
a solution to just reorder some definitions and we land where we want to be? Is it that simple?


Hopefully the above helps understanding the bigger picture?

All the best

Ed W

Re: [PATCH v4 1/2] x86: Support APU5 & APU6 in PCEngines platform driver

[Hans de Goede]

Hi Ed,

On 1/20/23 20:18, Ed W wrote:
> On 19/01/2023 10:22, Hans de Goede wrote:
> 
>>>  /* Order in which the GPIO lines are defined in the register list */
>>>  #define APU2_GPIO_LINE_LED1		0
>>>  #define APU2_GPIO_LINE_LED2		1
>>>  #define APU2_GPIO_LINE_LED3		2
>>>  #define APU2_GPIO_LINE_MODESW		3
>>> -#define APU2_GPIO_LINE_SIMSWAP		4
>>> -#define APU2_GPIO_LINE_MPCIE2		5
>>> -#define APU2_GPIO_LINE_MPCIE3		6
>>> +#define APU2_GPIO_LINE_RESETM1		4
>>> +#define APU2_GPIO_LINE_RESETM2		5
>>> +#define APU2_GPIO_LINE_SIMSWAP		6
>> I don't think this changing of GPIO ordering, or
>> for that part the changing of the gpio_names from 
>> "mpcie2_reset" to "modem1-reset" is a good idea.
>>
>> I'm not entirely sure how these GPIOs are supposed to be
>> consumed / used by userspace. But since they are not used
>> directly in this driver I assume userspace is supposed to
>> use either the (deprecated) sysfs GPIO API or the new ioctl
>> based GPIO API to toggle say "simswap" if it needs to.
>>
>> The old sysfs API exclusively uses pin-indexes inside a GPIO
>> chip to select the pin, so by changing the pin order you
>> have just broken the userspace API.
>>
>> And the new ioctl API can use either pin-indexes or GPIO-line-names,
>> so by changing the names you have also just potentially broken
>> that.
>>
>> Please keep the order as is and only use the new names for
>> the newly added models (so for APU6 I believe).
> 
> 
> Hi, I'm not sure what the "correct" thing to do is, but just to add some background to the situation:
> 
> There are an increasing number of APU boards, which are *very* similar, and also through time the
> pin allocations have muddled around, plus most recently, the BIOS can configure many things and has
> started to use naming conventions different to the historic kernel naming
> 
> So I don't have a board in front of me to be definitive, but something like the following happened:
> 
> - APU2 used something like mpcie sockets 1&2 for USB stuff and hence LTE cards, socket 3 was msata
> 
> - Then another version APU3, I think moved these to sockets 2&3
> 
> - Then another version APU4, moved the USB to sockets 2&3 and wired up a second SIM slot in most
> versions, including a SIM line swapper chip. Now you start to wonder if you should have labelled
> things PCIE1, PCIE2, PCIE3, etc, when really they mean modem 1 and modem 2, etc?
> 
> - Then came APU5, which has 3x USB sockets, plus 3x mpcie sockets. These are wired to different pcie
> numbers, and so the naming modem1, modem2, modem3 starts to make a lot more sense.
> 
> - APU6, which is mentioned in the original patch, is really just the same as one of the other
> boards, but with different ethernet sockets (SFP vs copper)
> 
> 
> - There is also a rare feature, which is likely not known to most users, or even wired up correctly
> on many boards. You have a reset/enable line to some of the mpcie slots. This again makes more sense
> to label logically vs than per slot. It's really not clear that this feature is properly supported
> or functioning on all boards (you can order special order boards wired in various ways). So changes
> here are unlikely to be noticed by all but a handful of specialist users.
> 
> 
> Overall, if one could start again, the unifying feature would be label slots logically, ie modem1,
> modem2, wifi1, wifi2, rather than numbering them based on how they are wired on a specific board rev.


"this" below starts here:

> Additionally, users who didn't load the APU driver, likely had ACPI named devices and these all have
> different (and to my eye, more logical names). So whatever we decide to do here will cause some
> breakage and inconsistency...

Hmm, can you elaborate a bit on this?  Does ACPI somehow expose the LEDs / GPIO to userspace
already and will adding APU5 / APU6 support make those ACPI exposed devices go away ?

If yes then what is the advantage of using the APU driver over the ACPI exported functionality?

Sorry for being a bit pedantic about this patch, but as a maintainer it is my responsibility
to ensure that we don't break existing userspace, e.g. existing use-cases using the exposed
ACPI devices.

Note "fixing" this might be as simple as putting the new APU5 / APU6 support behind an extra Kconfig
option (behind a modparam actually with a Kconfig option to select the default of the modparam).

This way we can get distro's to opt-in to (or opt-out depending on the default) the new behavior,
needing a modparam + Kconfig option for this is not ideal, but if there is a significant behavior
change it is an escape hatch we can use.

> Note that I submitted this previous patch "years ago", and I've somewhat given up on ever getting
> the APU driver up to date.. I think in 2020, Enrico shot me down because he was working on some
> grand unification for modem GPIO handling? (Enrico, please correct me on the details?) Hans, I think
> if you search back to 2020 on "APU", you will see that you arbitrated in that thread?

Yes I vaguely remember back then I was hoping / expecting you and Enrico to sort things
out, but that seems to have not happened. And given Enrico's silence in this thread
I'm not sure if Enrico is still working on this. So I guess now I get to figure out how
to move forward here.

> For whatever
> reason, we seem to be stuck that there are competing voices blocking progress here. Every route
> leads to some level of incompatibility. Personally I am a fairly large consumer of these devices,
> but I really don't care what we decide, because we ship a custom software, where userspace will
> match kernel, so we will update both in lockstep, whatever happens. Changes aren't a problem for me
> personally.
> 
> My vote would be for a one-of breakage, to at least get everyone using the same
> terms/names/whatever. I would suspect OpenWRT is probably the biggest voice here, so suggest we go
> with whatever they suggest, and then at least we are all in sync for the future? If its a one off,
> then suggest taking into account the ACPI naming as well?

I agree that we need to find a way forward here. I would like to get this resolved
and to get something merged into the mainline kernel for this.

I also agree that if there is breakage it would be best to just break things only once.

If there is going to breakage though I think we need some toggle to toggle between
the old and new *mainline* kernel behavior. This could be as easy as making the modparam
for this just abort probe() (return -ENODEV) on the new APU models when it is set to its
"backward compat" value.

> Note, there is a very big risk that I missed the point... Please be gentle. Quite possibly there is
> a solution to just reorder some definitions and we land where we want to be? Is it that simple?

Yes my original compatibility remark was just about reordering some definitions +
keeping the old labels for the already supported APU models.

So talking in code my proposal is to change this (in the new code):

#define APU2_GPIO_LINE_LED1		0
#define APU2_GPIO_LINE_LED2		1
#define APU2_GPIO_LINE_LED3		2
#define APU2_GPIO_LINE_MODESW		3
#define APU2_GPIO_LINE_RESETM1		4
#define APU2_GPIO_LINE_RESETM2		5
#define APU2_GPIO_LINE_SIMSWAP		6

to:

#define APU2_GPIO_LINE_LED1		0
#define APU2_GPIO_LINE_LED2		1
#define APU2_GPIO_LINE_LED3		2
#define APU2_GPIO_LINE_MODESW		3
#define APU2_GPIO_LINE_SIMSWAP		4
#define APU2_GPIO_LINE_RESETM1		5
#define APU2_GPIO_LINE_RESETM2		6

Keeping the simswap signal as GPIO/pin number 4 instead of moving it
to the end.

And also instead of making changes to apu2_gpio_names[] (1)
introduce a new apu5_gpio_names[] / apu6_gpio_names[] so that
the labels don't change on the existing supported models.

I'm less worried about the label change then about the index
change, because typical GPIO use from userspace will use
indexes not labels. So if having different labels on
different APU versions is a big problem you might be able to
convince me to change the labels on the old models too.

Summarizing:

Please change:

1. The GPIO indexing to keep simswap at its old place
2. Use the labels only on new models (open for discussion).

Open questions:
1. Can you elaborate a bit about the ACPI way of accessing these
things. If that is actually a thing, we cannot just break it
(but we could use a module-parameter for still breaking it).

2. You mention this is important to the openwrt community are
there already openwrt people in the Cc here so that we can get
their input? If not can you reach out to them ?

Regards,

Hans



1) other then adjusting the initializers for the MPCIE? -> RESETM? rename

Re: [PATCH v4 1/2] x86: Support APU5 & APU6 in PCEngines platform driver

[Philip Prindeville]

Hi Ed and Hans,

First off, sorry for taking a while to get back.  My wife has been busy with final exams at uni and I've been having to take care of the kids for both of us.



> On Feb 2, 2023, at 4:14 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> 
> Hi Ed,
> 
> On 1/20/23 20:18, Ed W wrote:
>> On 19/01/2023 10:22, Hans de Goede wrote:
>> 
>>>> /* Order in which the GPIO lines are defined in the register list */
>>>> #define APU2_GPIO_LINE_LED1 0
>>>> #define APU2_GPIO_LINE_LED2 1
>>>> #define APU2_GPIO_LINE_LED3 2
>>>> #define APU2_GPIO_LINE_MODESW 3
>>>> -#define APU2_GPIO_LINE_SIMSWAP 4
>>>> -#define APU2_GPIO_LINE_MPCIE2 5
>>>> -#define APU2_GPIO_LINE_MPCIE3 6
>>>> +#define APU2_GPIO_LINE_RESETM1 4
>>>> +#define APU2_GPIO_LINE_RESETM2 5
>>>> +#define APU2_GPIO_LINE_SIMSWAP 6
>>> I don't think this changing of GPIO ordering, or
>>> for that part the changing of the gpio_names from 
>>> "mpcie2_reset" to "modem1-reset" is a good idea.
>>> 
>>> I'm not entirely sure how these GPIOs are supposed to be
>>> consumed / used by userspace. But since they are not used
>>> directly in this driver I assume userspace is supposed to
>>> use either the (deprecated) sysfs GPIO API or the new ioctl
>>> based GPIO API to toggle say "simswap" if it needs to.
>>> 
>>> The old sysfs API exclusively uses pin-indexes inside a GPIO
>>> chip to select the pin, so by changing the pin order you
>>> have just broken the userspace API.
>>> 
>>> And the new ioctl API can use either pin-indexes or GPIO-line-names,
>>> so by changing the names you have also just potentially broken
>>> that.
>>> 
>>> Please keep the order as is and only use the new names for
>>> the newly added models (so for APU6 I believe).
>> 
>> 
>> Hi, I'm not sure what the "correct" thing to do is, but just to add some background to the situation:
>> 
>> There are an increasing number of APU boards, which are *very* similar, and also through time the
>> pin allocations have muddled around, plus most recently, the BIOS can configure many things and has
>> started to use naming conventions different to the historic kernel naming
>> 
>> So I don't have a board in front of me to be definitive, but something like the following happened:
>> 
>> - APU2 used something like mpcie sockets 1&2 for USB stuff and hence LTE cards, socket 3 was msata
>> 
>> - Then another version APU3, I think moved these to sockets 2&3
>> 
>> - Then another version APU4, moved the USB to sockets 2&3 and wired up a second SIM slot in most
>> versions, including a SIM line swapper chip. Now you start to wonder if you should have labelled
>> things PCIE1, PCIE2, PCIE3, etc, when really they mean modem 1 and modem 2, etc?
>> 
>> - Then came APU5, which has 3x USB sockets, plus 3x mpcie sockets. These are wired to different pcie
>> numbers, and so the naming modem1, modem2, modem3 starts to make a lot more sense.
>> 
>> - APU6, which is mentioned in the original patch, is really just the same as one of the other
>> boards, but with different ethernet sockets (SFP vs copper)


Yes, eth0 on the APU6 is an i210 w/ SFP cage, and i211 on all of the other ports.  The APU4 and others all used i211's on all the ports (for 1000baseTX).

I've asked PC Engines for a definitive list of what GPIO lines are used for what on all of the current revs of the boards.  I'll share that as soon as I get it.



>> 
>> - There is also a rare feature, which is likely not known to most users, or even wired up correctly
>> on many boards. You have a reset/enable line to some of the mpcie slots. This again makes more sense
>> to label logically vs than per slot. It's really not clear that this feature is properly supported
>> or functioning on all boards (you can order special order boards wired in various ways). So changes
>> here are unlikely to be noticed by all but a handful of specialist users.
>> 
>> 
>> Overall, if one could start again, the unifying feature would be label slots logically, ie modem1,
>> modem2, wifi1, wifi2, rather than numbering them based on how they are wired on a specific board rev.
> 
> 
> "this" below starts here:
> 
>> Additionally, users who didn't load the APU driver, likely had ACPI named devices and these all have
>> different (and to my eye, more logical names). So whatever we decide to do here will cause some
>> breakage and inconsistency...
> 
> Hmm, can you elaborate a bit on this?  Does ACPI somehow expose the LEDs / GPIO to userspace
> already and will adding APU5 / APU6 support make those ACPI exposed devices go away ?
> 
> If yes then what is the advantage of using the APU driver over the ACPI exported functionality?


Other than ACPI being less than reliable in a lot of cases?


> 
> Sorry for being a bit pedantic about this patch, but as a maintainer it is my responsibility
> to ensure that we don't break existing userspace, e.g. existing use-cases using the exposed
> ACPI devices.
> 
> Note "fixing" this might be as simple as putting the new APU5 / APU6 support behind an extra Kconfig
> option (behind a modparam actually with a Kconfig option to select the default of the modparam).
> 
> This way we can get distro's to opt-in to (or opt-out depending on the default) the new behavior,
> needing a modparam + Kconfig option for this is not ideal, but if there is a significant behavior
> change it is an escape hatch we can use.
> 
>> Note that I submitted this previous patch "years ago", and I've somewhat given up on ever getting
>> the APU driver up to date.. I think in 2020, Enrico shot me down because he was working on some
>> grand unification for modem GPIO handling? (Enrico, please correct me on the details?) Hans, I think
>> if you search back to 2020 on "APU", you will see that you arbitrated in that thread?
> 
> Yes I vaguely remember back then I was hoping / expecting you and Enrico to sort things
> out, but that seems to have not happened. And given Enrico's silence in this thread
> I'm not sure if Enrico is still working on this. So I guess now I get to figure out how
> to move forward here.
> 
>> For whatever
>> reason, we seem to be stuck that there are competing voices blocking progress here. Every route
>> leads to some level of incompatibility. Personally I am a fairly large consumer of these devices,
>> but I really don't care what we decide, because we ship a custom software, where userspace will
>> match kernel, so we will update both in lockstep, whatever happens. Changes aren't a problem for me
>> personally.
>> 
>> My vote would be for a one-of breakage, to at least get everyone using the same
>> terms/names/whatever. I would suspect OpenWRT is probably the biggest voice here, so suggest we go
>> with whatever they suggest, and then at least we are all in sync for the future? If its a one off,
>> then suggest taking into account the ACPI naming as well?
> 
> I agree that we need to find a way forward here. I would like to get this resolved
> and to get something merged into the mainline kernel for this.
> 
> I also agree that if there is breakage it would be best to just break things only once.
> 
> If there is going to breakage though I think we need some toggle to toggle between
> the old and new *mainline* kernel behavior. This could be as easy as making the modparam
> for this just abort probe() (return -ENODEV) on the new APU models when it is set to its
> "backward compat" value.


If people wanted to use ACPI instead of the APU driver, why not just build their kernels without the APU driver linked in?


> 
>> Note, there is a very big risk that I missed the point... Please be gentle. Quite possibly there is
>> a solution to just reorder some definitions and we land where we want to be? Is it that simple?
> 
> Yes my original compatibility remark was just about reordering some definitions +
> keeping the old labels for the already supported APU models.
> 
> So talking in code my proposal is to change this (in the new code):
> 
> #define APU2_GPIO_LINE_LED1 0
> #define APU2_GPIO_LINE_LED2 1
> #define APU2_GPIO_LINE_LED3 2
> #define APU2_GPIO_LINE_MODESW 3
> #define APU2_GPIO_LINE_RESETM1 4
> #define APU2_GPIO_LINE_RESETM2 5
> #define APU2_GPIO_LINE_SIMSWAP 6
> 
> to:
> 
> #define APU2_GPIO_LINE_LED1 0
> #define APU2_GPIO_LINE_LED2 1
> #define APU2_GPIO_LINE_LED3 2
> #define APU2_GPIO_LINE_MODESW 3
> #define APU2_GPIO_LINE_SIMSWAP 4
> #define APU2_GPIO_LINE_RESETM1 5
> #define APU2_GPIO_LINE_RESETM2 6
> 
> Keeping the simswap signal as GPIO/pin number 4 instead of moving it
> to the end.
> 
> And also instead of making changes to apu2_gpio_names[] (1)
> introduce a new apu5_gpio_names[] / apu6_gpio_names[] so that
> the labels don't change on the existing supported models.
> 
> I'm less worried about the label change then about the index
> change, because typical GPIO use from userspace will use
> indexes not labels. So if having different labels on
> different APU versions is a big problem you might be able to
> convince me to change the labels on the old models too.


I've been thinking about this the last few days, and the APU's are all low-power, headless (no video), SBC's.  They're designed for embedded usage.  That is, they don't have generic distros like Ubuntu (et al) installed on them, so the kernel and the bundled applications are all released together, typically in an monolithic image (at least that's the case for OpenWRT).

Changing the kernel and what's visible in user-space typically isn't a problem as long as both happen at the same time.  That's what we've done with OpenWRT, adding the 2 new board models, and the mapping of led triggers to GPIO lines.


> 
> Summarizing:
> 
> Please change:
> 
> 1. The GPIO indexing to keep simswap at its old place
> 2. Use the labels only on new models (open for discussion).
> 
> Open questions:
> 1. Can you elaborate a bit about the ACPI way of accessing these
> things. If that is actually a thing, we cannot just break it
> (but we could use a module-parameter for still breaking it).


What would this look like?  Would it be a boolean that throws the switch from "classic/legacy" to "updated" mapping?  I think that could work...  Since in OpenWRT we control both the drivers, the Kconfig settings, and the default GRUB parameters, that would work in our case.  I can't speak for pfSense, etc.


> 
> 2. You mention this is important to the openwrt community are
> there already openwrt people in the Cc here so that we can get
> their input? If not can you reach out to them ?


Paul and myself are both from the OpenWRT community.

-PHilip



> 
> Regards,
> 
> Hans
> 
> 
> 
> 1) other then adjusting the initializers for the MPCIE? -> RESETM? rename
> 

Re: [PATCH v4 1/2] x86: Support APU5 & APU6 in PCEngines platform driver

[Hans de Goede]

Hi,

On 2/9/23 07:04, Philip Prindeville wrote:
> Hi Ed and Hans,
> 
> First off, sorry for taking a while to get back.  My wife has been busy with final exams at uni and I've been having to take care of the kids for both of us.

No problem.

>> On Feb 2, 2023, at 4:14 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi Ed,
>>
>> On 1/20/23 20:18, Ed W wrote:
>>> On 19/01/2023 10:22, Hans de Goede wrote:
>>>
>>>>> /* Order in which the GPIO lines are defined in the register list */
>>>>> #define APU2_GPIO_LINE_LED1 0
>>>>> #define APU2_GPIO_LINE_LED2 1
>>>>> #define APU2_GPIO_LINE_LED3 2
>>>>> #define APU2_GPIO_LINE_MODESW 3
>>>>> -#define APU2_GPIO_LINE_SIMSWAP 4
>>>>> -#define APU2_GPIO_LINE_MPCIE2 5
>>>>> -#define APU2_GPIO_LINE_MPCIE3 6
>>>>> +#define APU2_GPIO_LINE_RESETM1 4
>>>>> +#define APU2_GPIO_LINE_RESETM2 5
>>>>> +#define APU2_GPIO_LINE_SIMSWAP 6
>>>> I don't think this changing of GPIO ordering, or
>>>> for that part the changing of the gpio_names from 
>>>> "mpcie2_reset" to "modem1-reset" is a good idea.
>>>>
>>>> I'm not entirely sure how these GPIOs are supposed to be
>>>> consumed / used by userspace. But since they are not used
>>>> directly in this driver I assume userspace is supposed to
>>>> use either the (deprecated) sysfs GPIO API or the new ioctl
>>>> based GPIO API to toggle say "simswap" if it needs to.
>>>>
>>>> The old sysfs API exclusively uses pin-indexes inside a GPIO
>>>> chip to select the pin, so by changing the pin order you
>>>> have just broken the userspace API.
>>>>
>>>> And the new ioctl API can use either pin-indexes or GPIO-line-names,
>>>> so by changing the names you have also just potentially broken
>>>> that.
>>>>
>>>> Please keep the order as is and only use the new names for
>>>> the newly added models (so for APU6 I believe).
>>>
>>>
>>> Hi, I'm not sure what the "correct" thing to do is, but just to add some background to the situation:
>>>
>>> There are an increasing number of APU boards, which are *very* similar, and also through time the
>>> pin allocations have muddled around, plus most recently, the BIOS can configure many things and has
>>> started to use naming conventions different to the historic kernel naming
>>>
>>> So I don't have a board in front of me to be definitive, but something like the following happened:
>>>
>>> - APU2 used something like mpcie sockets 1&2 for USB stuff and hence LTE cards, socket 3 was msata
>>>
>>> - Then another version APU3, I think moved these to sockets 2&3
>>>
>>> - Then another version APU4, moved the USB to sockets 2&3 and wired up a second SIM slot in most
>>> versions, including a SIM line swapper chip. Now you start to wonder if you should have labelled
>>> things PCIE1, PCIE2, PCIE3, etc, when really they mean modem 1 and modem 2, etc?
>>>
>>> - Then came APU5, which has 3x USB sockets, plus 3x mpcie sockets. These are wired to different pcie
>>> numbers, and so the naming modem1, modem2, modem3 starts to make a lot more sense.
>>>
>>> - APU6, which is mentioned in the original patch, is really just the same as one of the other
>>> boards, but with different ethernet sockets (SFP vs copper)
> 
> 
> Yes, eth0 on the APU6 is an i210 w/ SFP cage, and i211 on all of the other ports.  The APU4 and others all used i211's on all the ports (for 1000baseTX).
> 
> I've asked PC Engines for a definitive list of what GPIO lines are used for what on all of the current revs of the boards.  I'll share that as soon as I get it.
> 
> 
> 
>>>
>>> - There is also a rare feature, which is likely not known to most users, or even wired up correctly
>>> on many boards. You have a reset/enable line to some of the mpcie slots. This again makes more sense
>>> to label logically vs than per slot. It's really not clear that this feature is properly supported
>>> or functioning on all boards (you can order special order boards wired in various ways). So changes
>>> here are unlikely to be noticed by all but a handful of specialist users.
>>>
>>>
>>> Overall, if one could start again, the unifying feature would be label slots logically, ie modem1,
>>> modem2, wifi1, wifi2, rather than numbering them based on how they are wired on a specific board rev.
>>
>>
>> "this" below starts here:
>>
>>> Additionally, users who didn't load the APU driver, likely had ACPI named devices and these all have
>>> different (and to my eye, more logical names). So whatever we decide to do here will cause some
>>> breakage and inconsistency...
>>
>> Hmm, can you elaborate a bit on this?  Does ACPI somehow expose the LEDs / GPIO to userspace
>> already and will adding APU5 / APU6 support make those ACPI exposed devices go away ?
>>
>> If yes then what is the advantage of using the APU driver over the ACPI exported functionality?
> 
> 
> Other than ACPI being less than reliable in a lot of cases?

ACPI can sometimes be unreliable, but that is just down to it being badly implemented by
board vendors.

If used correctly it is no more or less reliable as any other code, so its reliability is not
really a good argument not to use it unless the ACPI code on PCEngines devices is known to
be unreliable ?

>> Sorry for being a bit pedantic about this patch, but as a maintainer it is my responsibility
>> to ensure that we don't break existing userspace, e.g. existing use-cases using the exposed
>> ACPI devices.
>>
>> Note "fixing" this might be as simple as putting the new APU5 / APU6 support behind an extra Kconfig
>> option (behind a modparam actually with a Kconfig option to select the default of the modparam).
>>
>> This way we can get distro's to opt-in to (or opt-out depending on the default) the new behavior,
>> needing a modparam + Kconfig option for this is not ideal, but if there is a significant behavior
>> change it is an escape hatch we can use.
>>
>>> Note that I submitted this previous patch "years ago", and I've somewhat given up on ever getting
>>> the APU driver up to date.. I think in 2020, Enrico shot me down because he was working on some
>>> grand unification for modem GPIO handling? (Enrico, please correct me on the details?) Hans, I think
>>> if you search back to 2020 on "APU", you will see that you arbitrated in that thread?
>>
>> Yes I vaguely remember back then I was hoping / expecting you and Enrico to sort things
>> out, but that seems to have not happened. And given Enrico's silence in this thread
>> I'm not sure if Enrico is still working on this. So I guess now I get to figure out how
>> to move forward here.
>>
>>> For whatever
>>> reason, we seem to be stuck that there are competing voices blocking progress here. Every route
>>> leads to some level of incompatibility. Personally I am a fairly large consumer of these devices,
>>> but I really don't care what we decide, because we ship a custom software, where userspace will
>>> match kernel, so we will update both in lockstep, whatever happens. Changes aren't a problem for me
>>> personally.
>>>
>>> My vote would be for a one-of breakage, to at least get everyone using the same
>>> terms/names/whatever. I would suspect OpenWRT is probably the biggest voice here, so suggest we go
>>> with whatever they suggest, and then at least we are all in sync for the future? If its a one off,
>>> then suggest taking into account the ACPI naming as well?
>>
>> I agree that we need to find a way forward here. I would like to get this resolved
>> and to get something merged into the mainline kernel for this.
>>
>> I also agree that if there is breakage it would be best to just break things only once.
>>
>> If there is going to breakage though I think we need some toggle to toggle between
>> the old and new *mainline* kernel behavior. This could be as easy as making the modparam
>> for this just abort probe() (return -ENODEV) on the new APU models when it is set to its
>> "backward compat" value.
> 
> 
> If people wanted to use ACPI instead of the APU driver, why not just build their kernels without the APU driver linked in?

Most people do no want to / don't have the skills to build their own kernel, so they are
going to be relying on a distro (including openwrt as a sort of distro) provided kernel.

>>> Note, there is a very big risk that I missed the point... Please be gentle. Quite possibly there is
>>> a solution to just reorder some definitions and we land where we want to be? Is it that simple?
>>
>> Yes my original compatibility remark was just about reordering some definitions +
>> keeping the old labels for the already supported APU models.
>>
>> So talking in code my proposal is to change this (in the new code):
>>
>> #define APU2_GPIO_LINE_LED1 0
>> #define APU2_GPIO_LINE_LED2 1
>> #define APU2_GPIO_LINE_LED3 2
>> #define APU2_GPIO_LINE_MODESW 3
>> #define APU2_GPIO_LINE_RESETM1 4
>> #define APU2_GPIO_LINE_RESETM2 5
>> #define APU2_GPIO_LINE_SIMSWAP 6
>>
>> to:
>>
>> #define APU2_GPIO_LINE_LED1 0
>> #define APU2_GPIO_LINE_LED2 1
>> #define APU2_GPIO_LINE_LED3 2
>> #define APU2_GPIO_LINE_MODESW 3
>> #define APU2_GPIO_LINE_SIMSWAP 4
>> #define APU2_GPIO_LINE_RESETM1 5
>> #define APU2_GPIO_LINE_RESETM2 6
>>
>> Keeping the simswap signal as GPIO/pin number 4 instead of moving it
>> to the end.
>>
>> And also instead of making changes to apu2_gpio_names[] (1)
>> introduce a new apu5_gpio_names[] / apu6_gpio_names[] so that
>> the labels don't change on the existing supported models.
>>
>> I'm less worried about the label change then about the index
>> change, because typical GPIO use from userspace will use
>> indexes not labels. So if having different labels on
>> different APU versions is a big problem you might be able to
>> convince me to change the labels on the old models too.
> 
> 
> I've been thinking about this the last few days, and the APU's are all low-power, headless (no video), SBC's.  They're designed for embedded usage.  That is, they don't have generic distros like Ubuntu (et al) installed on them, so the kernel and the bundled applications are all released together, typically in an monolithic image (at least that's the case for OpenWRT).
> 
> Changing the kernel and what's visible in user-space typically isn't a problem as long as both happen at the same time.  That's what we've done with OpenWRT, adding the 2 new board models, and the mapping of led triggers to GPIO lines.

For the upstream / mainline kernel we have a very clear defined policy of
never breaking userspace (APIs). Even though these are designed for embedded
usage, some people might be running normal distro-s on these.

I have seen people run Fedora on Intel Atom x86 devices designed as
NAS boxes...


> 
> 
>>
>> Summarizing:
>>
>> Please change:
>>
>> 1. The GPIO indexing to keep simswap at its old place
>> 2. Use the labels only on new models (open for discussion).
>>
>> Open questions:
>> 1. Can you elaborate a bit about the ACPI way of accessing these
>> things. If that is actually a thing, we cannot just break it
>> (but we could use a module-parameter for still breaking it).
> 
> 
> What would this look like?  Would it be a boolean that throws the switch from "classic/legacy" to "updated" mapping?  I think that could work...  Since in OpenWRT we control both the drivers, the Kconfig settings, and the default GRUB parameters, that would work in our case.  I can't speak for pfSense, etc.

Yes a boolean module parameter with the default value of the boolean
configurable through Kconfig, so that e.g. openwrt can just pick
default values matching what it wants and won't need to specify
anything on the kernel commandline.

Note this is not just about the mapping though. From what I understand
about this, using the pcengines-apu driver conflicts with the ACPI way
of accessing the LEDs and gpios.

So for the new APU models, there should be a module-option to decide
whether for probe() to continue at all on those models or whether
it should just return -ENODEV (so the driver won't bind), leaving
things just as they were before this changes.  The purpose of this
is to keep the ACPI way of accessing the LEDs, ..., working.

>> 2. You mention this is important to the openwrt community are
>> there already openwrt people in the Cc here so that we can get
>> their input? If not can you reach out to them ?
> 
> 
> Paul and myself are both from the OpenWRT community.

That is good to know.

Regards,

Hans

Re: [PATCH v4 1/2] x86: Support APU5 & APU6 in PCEngines platform driver

[Ed W]

Hi All


> On 2/9/23 07:04, Philip Prindeville wrote:
>> Hi Ed and Hans,
>>
>> First off, sorry for taking a while to get back.  My wife has been busy with final exams at uni and I've been having to take care of the kids for both of us.
> No problem.


Ditto here. I ran out of time to build a current kernel and double check the ACPI status! However, I
can get the details in the next week to confirm. Apologies!



>>>> Hi, I'm not sure what the "correct" thing to do is, but just to add some background to the situation:
>>>>
>>>> There are an increasing number of APU boards, which are *very* similar, and also through time the
>>>> pin allocations have muddled around, plus most recently, the BIOS can configure many things and has
>>>> started to use naming conventions different to the historic kernel naming
>>>>
>>>> So I don't have a board in front of me to be definitive, but something like the following happened:
>>>>
>>>> - APU2 used something like mpcie sockets 1&2 for USB stuff and hence LTE cards, socket 3 was msata
>>>>
>>>> - Then another version APU3, I think moved these to sockets 2&3
>>>>
>>>> - Then another version APU4, moved the USB to sockets 2&3 and wired up a second SIM slot in most
>>>> versions, including a SIM line swapper chip. Now you start to wonder if you should have labelled
>>>> things PCIE1, PCIE2, PCIE3, etc, when really they mean modem 1 and modem 2, etc?
>>>>
>>>> - Then came APU5, which has 3x USB sockets, plus 3x mpcie sockets. These are wired to different pcie
>>>> numbers, and so the naming modem1, modem2, modem3 starts to make a lot more sense.
>>>>
>>>> - APU6, which is mentioned in the original patch, is really just the same as one of the other
>>>> boards, but with different ethernet sockets (SFP vs copper)
>>
>> Yes, eth0 on the APU6 is an i210 w/ SFP cage, and i211 on all of the other ports.  The APU4 and others all used i211's on all the ports (for 1000baseTX).
>>
>> I've asked PC Engines for a definitive list of what GPIO lines are used for what on all of the current revs of the boards.  I'll share that as soon as I get it.


Minor nitpick.  The APU2 has i210 on the original boards. However, recently the shortage of intel
chipsets has led to various compromise options and some boards are coming with i211 on. I'm not sure
if Pascal will standardise on these for the future though?

There is also an APU7 mentioned in the latest firmware notes. I don't own one of these, nor the
schematics, but the release notes say that it's an APU3 with different ethernet ports. I suggest we
assume this to be true and add APU7 to our detection?



>>> Hmm, can you elaborate a bit on this?  Does ACPI somehow expose the LEDs / GPIO to userspace
>>> already and will adding APU5 / APU6 support make those ACPI exposed devices go away ?
>>>
>>> If yes then what is the advantage of using the APU driver over the ACPI exported functionality?
>>
>> Other than ACPI being less than reliable in a lot of cases?
> ACPI can sometimes be unreliable, but that is just down to it being badly implemented by
> board vendors.
>
> If used correctly it is no more or less reliable as any other code, so its reliability is not
> really a good argument not to use it unless the ACPI code on PCEngines devices is known to
> be unreliable ?


So at least with the few firmware's I have tried, the ACPI works ok on APU.

The history (without full details) is something like:

- A long way back in time, we had either a different driver in the kernel, or the ACPI used a
different name for the LEDs and button. I forget the details without looking back at some notes

- As of $lots_of_years_ago, there was a big APU firmware change and since then I believe ACPI has
(reliably) offered auto configuration for the LEDs and I believe also for the switch button (sorry,
hazy now without checking a real board, but I can do that no problem)

- However, I believe Enrico built the original APU kernel driver and that happened to use the old
names, and then once the ACPI change happened, he feels/felt strongly that we should maintain the
current driver, disable the ACPI and ignore the updated naming convention from the ACPI.

- In any case, using only ACPI doesn't setup the GPIOs for things like the SIM swap chip and reset
lines, etc. So I think a platform driver of some sort is the correct way forward

- I think my patch from a year or so back attempted to swap the kernel names to match ACPI (need to
double check that?). This was not unreasonably resisted because it would break userspace relying on
the names used by the LED driver.


- The other change I tried to insert was to rename the GPIOs, more explanation (some repetition from
prev email):

- All boards, except APU5 have the same basic shape and 3x mpcie connectors.

- However, although the connectors are the same, they don;t all have the same things wired to them

- You actually have 2x USB (with up to 2x SIM), 2x pcie, 1x msata

- So based on studying APU1 and APU2, which had a separate msata and the other 2 ports wired up
consistently for wifi/LTE, people named the GPIOs to match the mPCIe port numbers, ie LTE Modem 1
was on mPCIe port 1, so name the gpio something like mpcie1_reset

- Then came APU3-7... These boards flipped around where modem2 was. So you now have a situation
where userspace needs to know that they should toggle pmcie2_reset on APU3 and mpcie3_reset on APU4,
or whatever it really is.

- So my previous attempt to sneak in a change was to rename the GPIOs to be something more like
"modem1_reset" and "modem2_reset" etc. This way the GPIO names stay identical across ALL boards,
even though the slots might jump around board to board. This is clearly a better solution if we were
starting fresh, but it has consequences for a (very?) small number of users (I doubt many others are
even aware of the other GPIOs except for the LEDs/switch? Does even OpenWRT break out the sim_swap
lines and modem reset lines?)



>> If people wanted to use ACPI instead of the APU driver, why not just build their kernels without the APU driver linked in?
> Most people do no want to / don't have the skills to build their own kernel, so they are
> going to be relying on a distro (including openwrt as a sort of distro) provided kernel.


Well, also you don't fix the problem long term. There is this jump in naming if you pick one over
the other. Plus ACPI doesn't setup the sim swap line (and it may not setup the switch as people want
it? Can't recall if this is a problem?)

So whilst I agree, I think it would be desirable to narrow down the number of permutations here



>>>> Note, there is a very big risk that I missed the point... Please be gentle. Quite possibly there is
>>>> a solution to just reorder some definitions and we land where we want to be? Is it that simple?
>>> Yes my original compatibility remark was just about reordering some definitions +
>>> keeping the old labels for the already supported APU models.
>>>
>>> So talking in code my proposal is to change this (in the new code):
>>>
>>> #define APU2_GPIO_LINE_LED1 0
>>> #define APU2_GPIO_LINE_LED2 1
>>> #define APU2_GPIO_LINE_LED3 2
>>> #define APU2_GPIO_LINE_MODESW 3
>>> #define APU2_GPIO_LINE_RESETM1 4
>>> #define APU2_GPIO_LINE_RESETM2 5
>>> #define APU2_GPIO_LINE_SIMSWAP 6
>>>
>>> to:
>>>
>>> #define APU2_GPIO_LINE_LED1 0
>>> #define APU2_GPIO_LINE_LED2 1
>>> #define APU2_GPIO_LINE_LED3 2
>>> #define APU2_GPIO_LINE_MODESW 3
>>> #define APU2_GPIO_LINE_SIMSWAP 4
>>> #define APU2_GPIO_LINE_RESETM1 5
>>> #define APU2_GPIO_LINE_RESETM2 6
>>>
>>> Keeping the simswap signal as GPIO/pin number 4 instead of moving it
>>> to the end.
>>>
>>> And also instead of making changes to apu2_gpio_names[] (1)
>>> introduce a new apu5_gpio_names[] / apu6_gpio_names[] so that
>>> the labels don't change on the existing supported models.
>>>
>>> I'm less worried about the label change then about the index
>>> change, because typical GPIO use from userspace will use
>>> indexes not labels. So if having different labels on
>>> different APU versions is a big problem you might be able to
>>> convince me to change the labels on the old models too.


OK, so I think we see a potential solution here


Note, board summary is:

APU2-4 - vaguely the same, but modem2 jumps around slots (or disappears)

APU5 - oddball, gains an extra modem, so we now have modem1-3 GPIOs. All other GPIOs are maintained
(I think?).

APU6-7 - variants of APU2-4, I believe all the GPIOs stay identical?

Details:

    http://pcengines.github.io/apu2-documentation/APU_mPCIe_capabilities/

    http://pcengines.github.io/apu2-documentation/gpios/

    However, note that the APU5 has 3x SIM swaps, one for each modem (each modem has 2x SIMs). APU4
on the other hand only has a single one, it just inverts how the 2x SIMs connect to the 2x modems


Important Note: Realise that at this point this discussion is heavily theoretical because many
boards don't have the reset wires setup correctly, or their functionality might vary board release
to board release. eg I paid PCEngines to have mine re-enabled, because they ship them disabled by
default. Also in my testing they don't operate as expected. So it's highly unlikely there is a
single user in the world actually using functionality other than "Leds", "mode switch", and "sim
swap" (and even sim swap GPIO is likely to be restricted number of users). Certainly if anyone is
using any of that other functionality they they are super hacker and will cope with what comes next


Conclusion: Hans, would you mind commenting on if you think this is then all done and satisfactory
if we reorder the GPIOs as you propose? ie you are content with the rename?


>> I've been thinking about this the last few days, and the APU's are all low-power, headless (no video), SBC's.  They're designed for embedded usage.  That is, they don't have generic distros like Ubuntu (et al) installed on them, so the kernel and the bundled applications are all released together, typically in an monolithic image (at least that's the case for OpenWRT).
>>
>> Changing the kernel and what's visible in user-space typically isn't a problem as long as both happen at the same time.  That's what we've done with OpenWRT, adding the 2 new board models, and the mapping of led triggers to GPIO lines.
> For the upstream / mainline kernel we have a very clear defined policy of
> never breaking userspace (APIs). Even though these are designed for embedded
> usage, some people might be running normal distro-s on these.


So we don't lose sight of the big picture, I think the remaining detail we are debating is something
like:

- kernel driver names the LEDs something like:

/sys/class/leds/apu4:green:led1
/sys/class/leds/apu4:green:led2
/sys/class/leds/apu4:green:led3

- However, ACPI gives different names (I forget the details)

- Additionally, we already broke this in the (distant) past because there was a previous APU driver
which used different names still...


So I think my rejected change tried to simplify the LED names and drop the apuN bit. However, I'm
really not passionate about any changes here. I have a simple wrapper script for the old Alix and
newer APUs which just lets me set any LEDs by number. OpenWRT is welcome to use this?

    https://github.com/nippynetworks/gpio-utils



>>> Summarizing:
>>>
>>> Please change:
>>>
>>> 1. The GPIO indexing to keep simswap at its old place
>>> 2. Use the labels only on new models (open for discussion).
>>>
>>> Open questions:
>>> 1. Can you elaborate a bit about the ACPI way of accessing these
>>> things. If that is actually a thing, we cannot just break it
>>> (but we could use a module-parameter for still breaking it).
>>
>> What would this look like?  Would it be a boolean that throws the switch from "classic/legacy" to "updated" mapping?  I think that could work...  Since in OpenWRT we control both the drivers, the Kconfig settings, and the default GRUB parameters, that would work in our case.  I can't speak for pfSense, etc.
> Yes a boolean module parameter with the default value of the boolean
> configurable through Kconfig, so that e.g. openwrt can just pick
> default values matching what it wants and won't need to specify
> anything on the kernel commandline.
>
> Note this is not just about the mapping though. From what I understand
> about this, using the pcengines-apu driver conflicts with the ACPI way
> of accessing the LEDs and gpios.
>
> So for the new APU models, there should be a module-option to decide
> whether for probe() to continue at all on those models or whether
> it should just return -ENODEV (so the driver won't bind), leaving
> things just as they were before this changes.  The purpose of this
> is to keep the ACPI way of accessing the LEDs, ..., working.


So keeping this on topic.

1) Have we all agreed and signed off on the GPIO changes to naming, caveat we maintain the current
numbering? If so then lets tick that off

2) There is a debate about whether to change the LED userspace naming. I don't really want to push
this up the hill though? Proposal is to either sync with ACPI and offer a back compatible flag, or
we could just keep the naming as is and allow it to continue to deviate from the ACPI naming? I'm
cool with either? (although I don't like the current naming...)

Quick show of hands from openwrt on point 2? If no one cares enough to update the patch to add a
backward compatible flag then I suggest we stop that piece? (However, I'm happy to do this if I can
get a little support on coding it?)


Sounds like we have made progress?

Philip, are you cool to resubmit your patch with the adjusted GPIO ordering? (and APU7 detection as
an APU3) We just then need to decide whether to drop LED renaming?

Thanks all!

Ed W

Re: [PATCH v4 1/2] x86: Support APU5 & APU6 in PCEngines platform driver

[Hans de Goede]

Hi,

On 2/13/23 15:05, Ed W wrote:
> Hi All
> 
> 
>> On 2/9/23 07:04, Philip Prindeville wrote:
>>> Hi Ed and Hans,
>>>
>>> First off, sorry for taking a while to get back.  My wife has been busy with final exams at uni and I've been having to take care of the kids for both of us.
>> No problem.
> 
> 
> Ditto here. I ran out of time to build a current kernel and double check the ACPI status! However, I
> can get the details in the next week to confirm. Apologies!
> 
> 
> 
>>>>> Hi, I'm not sure what the "correct" thing to do is, but just to add some background to the situation:
>>>>>
>>>>> There are an increasing number of APU boards, which are *very* similar, and also through time the
>>>>> pin allocations have muddled around, plus most recently, the BIOS can configure many things and has
>>>>> started to use naming conventions different to the historic kernel naming
>>>>>
>>>>> So I don't have a board in front of me to be definitive, but something like the following happened:
>>>>>
>>>>> - APU2 used something like mpcie sockets 1&2 for USB stuff and hence LTE cards, socket 3 was msata
>>>>>
>>>>> - Then another version APU3, I think moved these to sockets 2&3
>>>>>
>>>>> - Then another version APU4, moved the USB to sockets 2&3 and wired up a second SIM slot in most
>>>>> versions, including a SIM line swapper chip. Now you start to wonder if you should have labelled
>>>>> things PCIE1, PCIE2, PCIE3, etc, when really they mean modem 1 and modem 2, etc?
>>>>>
>>>>> - Then came APU5, which has 3x USB sockets, plus 3x mpcie sockets. These are wired to different pcie
>>>>> numbers, and so the naming modem1, modem2, modem3 starts to make a lot more sense.
>>>>>
>>>>> - APU6, which is mentioned in the original patch, is really just the same as one of the other
>>>>> boards, but with different ethernet sockets (SFP vs copper)
>>>
>>> Yes, eth0 on the APU6 is an i210 w/ SFP cage, and i211 on all of the other ports.  The APU4 and others all used i211's on all the ports (for 1000baseTX).
>>>
>>> I've asked PC Engines for a definitive list of what GPIO lines are used for what on all of the current revs of the boards.  I'll share that as soon as I get it.
> 
> 
> Minor nitpick.  The APU2 has i210 on the original boards. However, recently the shortage of intel
> chipsets has led to various compromise options and some boards are coming with i211 on. I'm not sure
> if Pascal will standardise on these for the future though?
> 
> There is also an APU7 mentioned in the latest firmware notes. I don't own one of these, nor the
> schematics, but the release notes say that it's an APU3 with different ethernet ports. I suggest we
> assume this to be true and add APU7 to our detection?
> 
> 
> 
>>>> Hmm, can you elaborate a bit on this?  Does ACPI somehow expose the LEDs / GPIO to userspace
>>>> already and will adding APU5 / APU6 support make those ACPI exposed devices go away ?
>>>>
>>>> If yes then what is the advantage of using the APU driver over the ACPI exported functionality?
>>>
>>> Other than ACPI being less than reliable in a lot of cases?
>> ACPI can sometimes be unreliable, but that is just down to it being badly implemented by
>> board vendors.
>>
>> If used correctly it is no more or less reliable as any other code, so its reliability is not
>> really a good argument not to use it unless the ACPI code on PCEngines devices is known to
>> be unreliable ?
> 
> 
> So at least with the few firmware's I have tried, the ACPI works ok on APU.
> 
> The history (without full details) is something like:
> 
> - A long way back in time, we had either a different driver in the kernel, or the ACPI used a
> different name for the LEDs and button. I forget the details without looking back at some notes
> 
> - As of $lots_of_years_ago, there was a big APU firmware change and since then I believe ACPI has
> (reliably) offered auto configuration for the LEDs and I believe also for the switch button (sorry,
> hazy now without checking a real board, but I can do that no problem)
> 
> - However, I believe Enrico built the original APU kernel driver and that happened to use the old
> names, and then once the ACPI change happened, he feels/felt strongly that we should maintain the
> current driver, disable the ACPI and ignore the updated naming convention from the ACPI.
> 
> - In any case, using only ACPI doesn't setup the GPIOs for things like the SIM swap chip and reset
> lines, etc. So I think a platform driver of some sort is the correct way forward
> 
> - I think my patch from a year or so back attempted to swap the kernel names to match ACPI (need to
> double check that?). This was not unreasonably resisted because it would break userspace relying on
> the names used by the LED driver.
> 
> 
> - The other change I tried to insert was to rename the GPIOs, more explanation (some repetition from
> prev email):
> 
> - All boards, except APU5 have the same basic shape and 3x mpcie connectors.
> 
> - However, although the connectors are the same, they don;t all have the same things wired to them
> 
> - You actually have 2x USB (with up to 2x SIM), 2x pcie, 1x msata
> 
> - So based on studying APU1 and APU2, which had a separate msata and the other 2 ports wired up
> consistently for wifi/LTE, people named the GPIOs to match the mPCIe port numbers, ie LTE Modem 1
> was on mPCIe port 1, so name the gpio something like mpcie1_reset
> 
> - Then came APU3-7... These boards flipped around where modem2 was. So you now have a situation
> where userspace needs to know that they should toggle pmcie2_reset on APU3 and mpcie3_reset on APU4,
> or whatever it really is.
> 
> - So my previous attempt to sneak in a change was to rename the GPIOs to be something more like
> "modem1_reset" and "modem2_reset" etc. This way the GPIO names stay identical across ALL boards,
> even though the slots might jump around board to board. This is clearly a better solution if we were
> starting fresh, but it has consequences for a (very?) small number of users (I doubt many others are
> even aware of the other GPIOs except for the LEDs/switch? Does even OpenWRT break out the sim_swap
> lines and modem reset lines?)
> 
> 
> 
>>> If people wanted to use ACPI instead of the APU driver, why not just build their kernels without the APU driver linked in?
>> Most people do no want to / don't have the skills to build their own kernel, so they are
>> going to be relying on a distro (including openwrt as a sort of distro) provided kernel.
> 
> 
> Well, also you don't fix the problem long term. There is this jump in naming if you pick one over
> the other. Plus ACPI doesn't setup the sim swap line (and it may not setup the switch as people want
> it? Can't recall if this is a problem?)
> 
> So whilst I agree, I think it would be desirable to narrow down the number of permutations here

I agree on trying to keep the number of permutations as low as possible.

>>>>> Note, there is a very big risk that I missed the point... Please be gentle. Quite possibly there is
>>>>> a solution to just reorder some definitions and we land where we want to be? Is it that simple?
>>>> Yes my original compatibility remark was just about reordering some definitions +
>>>> keeping the old labels for the already supported APU models.
>>>>
>>>> So talking in code my proposal is to change this (in the new code):
>>>>
>>>> #define APU2_GPIO_LINE_LED1 0
>>>> #define APU2_GPIO_LINE_LED2 1
>>>> #define APU2_GPIO_LINE_LED3 2
>>>> #define APU2_GPIO_LINE_MODESW 3
>>>> #define APU2_GPIO_LINE_RESETM1 4
>>>> #define APU2_GPIO_LINE_RESETM2 5
>>>> #define APU2_GPIO_LINE_SIMSWAP 6
>>>>
>>>> to:
>>>>
>>>> #define APU2_GPIO_LINE_LED1 0
>>>> #define APU2_GPIO_LINE_LED2 1
>>>> #define APU2_GPIO_LINE_LED3 2
>>>> #define APU2_GPIO_LINE_MODESW 3
>>>> #define APU2_GPIO_LINE_SIMSWAP 4
>>>> #define APU2_GPIO_LINE_RESETM1 5
>>>> #define APU2_GPIO_LINE_RESETM2 6
>>>>
>>>> Keeping the simswap signal as GPIO/pin number 4 instead of moving it
>>>> to the end.
>>>>
>>>> And also instead of making changes to apu2_gpio_names[] (1)
>>>> introduce a new apu5_gpio_names[] / apu6_gpio_names[] so that
>>>> the labels don't change on the existing supported models.
>>>>
>>>> I'm less worried about the label change then about the index
>>>> change, because typical GPIO use from userspace will use
>>>> indexes not labels. So if having different labels on
>>>> different APU versions is a big problem you might be able to
>>>> convince me to change the labels on the old models too.
> 
> 
> OK, so I think we see a potential solution here
> 
> 
> Note, board summary is:
> 
> APU2-4 - vaguely the same, but modem2 jumps around slots (or disappears)
> 
> APU5 - oddball, gains an extra modem, so we now have modem1-3 GPIOs. All other GPIOs are maintained
> (I think?).
> 
> APU6-7 - variants of APU2-4, I believe all the GPIOs stay identical?
> 
> Details:
> 
>     http://pcengines.github.io/apu2-documentation/APU_mPCIe_capabilities/
> 
>     http://pcengines.github.io/apu2-documentation/gpios/
> 
>     However, note that the APU5 has 3x SIM swaps, one for each modem (each modem has 2x SIMs). APU4
> on the other hand only has a single one, it just inverts how the 2x SIMs connect to the 2x modems
> 
> 
> Important Note: Realise that at this point this discussion is heavily theoretical because many
> boards don't have the reset wires setup correctly, or their functionality might vary board release
> to board release. eg I paid PCEngines to have mine re-enabled, because they ship them disabled by
> default. Also in my testing they don't operate as expected. So it's highly unlikely there is a
> single user in the world actually using functionality other than "Leds", "mode switch", and "sim
> swap" (and even sim swap GPIO is likely to be restricted number of users). Certainly if anyone is
> using any of that other functionality they they are super hacker and will cope with what comes next
> 
> 
> Conclusion: Hans, would you mind commenting on if you think this is then all done and satisfactory
> if we reorder the GPIOs as you propose? ie you are content with the rename?

As mentioned before I can live with the rename (very small chance of this causing
userspace problems) as long as the pin order stays the same. Most userspace GPIO
APIs work with pin-numbers and changing the order is guaranteed to break that.

>>> I've been thinking about this the last few days, and the APU's are all low-power, headless (no video), SBC's.  They're designed for embedded usage.  That is, they don't have generic distros like Ubuntu (et al) installed on them, so the kernel and the bundled applications are all released together, typically in an monolithic image (at least that's the case for OpenWRT).
>>>
>>> Changing the kernel and what's visible in user-space typically isn't a problem as long as both happen at the same time.  That's what we've done with OpenWRT, adding the 2 new board models, and the mapping of led triggers to GPIO lines.
>> For the upstream / mainline kernel we have a very clear defined policy of
>> never breaking userspace (APIs). Even though these are designed for embedded
>> usage, some people might be running normal distro-s on these.
> 
> 
> So we don't lose sight of the big picture, I think the remaining detail we are debating is something
> like:
> 
> - kernel driver names the LEDs something like:
> 
> /sys/class/leds/apu4:green:led1
> /sys/class/leds/apu4:green:led2
> /sys/class/leds/apu4:green:led3
> 
> - However, ACPI gives different names (I forget the details)

It would be good to know the ACPI names. Also what happens with the ACPI
registered LED devices when the pcengines-apu driver loads, does it
somehow unregister those ?

If yes: Can we then make the pcengines-apu driver use the exact same names for the LEDs on new models ?
If no:  Then I guess that the ACPI registered LEDs keep working in parallel to the new LED devices ?

If the ACPI LEDs are not unregistered and keep working, then I guess there is no userspace
API breakage when using pcengines-apu on newer APU models, "just" duplicate LED devices ?
 
> - Additionally, we already broke this in the (distant) past because there was a previous APU driver
> which used different names still...

Just because we have gotten away with it once, does not mean we should do it again :)

> So I think my rejected change tried to simplify the LED names and drop the apuN bit. However, I'm
> really not passionate about any changes here. I have a simple wrapper script for the old Alix and
> newer APUs which just lets me set any LEDs by number. OpenWRT is welcome to use this?
> 
>     https://github.com/nippynetworks/gpio-utils
> 
> 
> 
>>>> Summarizing:
>>>>
>>>> Please change:
>>>>
>>>> 1. The GPIO indexing to keep simswap at its old place
>>>> 2. Use the labels only on new models (open for discussion).
>>>>
>>>> Open questions:
>>>> 1. Can you elaborate a bit about the ACPI way of accessing these
>>>> things. If that is actually a thing, we cannot just break it
>>>> (but we could use a module-parameter for still breaking it).
>>>
>>> What would this look like?  Would it be a boolean that throws the switch from "classic/legacy" to "updated" mapping?  I think that could work...  Since in OpenWRT we control both the drivers, the Kconfig settings, and the default GRUB parameters, that would work in our case.  I can't speak for pfSense, etc.
>> Yes a boolean module parameter with the default value of the boolean
>> configurable through Kconfig, so that e.g. openwrt can just pick
>> default values matching what it wants and won't need to specify
>> anything on the kernel commandline.
>>
>> Note this is not just about the mapping though. From what I understand
>> about this, using the pcengines-apu driver conflicts with the ACPI way
>> of accessing the LEDs and gpios.
>>
>> So for the new APU models, there should be a module-option to decide
>> whether for probe() to continue at all on those models or whether
>> it should just return -ENODEV (so the driver won't bind), leaving
>> things just as they were before this changes.  The purpose of this
>> is to keep the ACPI way of accessing the LEDs, ..., working.
> 
> 
> So keeping this on topic.
> 
> 1) Have we all agreed and signed off on the GPIO changes to naming, caveat we maintain the current
> numbering? If so then lets tick that off

Yes that is ok, ack.

> 2) There is a debate about whether to change the LED userspace naming. I don't really want to push
> this up the hill though? Proposal is to either sync with ACPI and offer a back compatible flag, or
> we could just keep the naming as is and allow it to continue to deviate from the ACPI naming? I'm
> cool with either? (although I don't like the current naming...)

For the new models I'm fine with whatever LED naming is preferred.

For the old already supported models changing the LED naming would require a default-off
module-parameter, which IMHO is probably not worth it.

For the new models my main worry is what happens to users of the ACPI exposed LED devices
(under /sys/class/leds/)  once the pcengines-apu driver starts binding/loading on the new models ?

> Quick show of hands from openwrt on point 2? If no one cares enough to update the patch to add a
> backward compatible flag then I suggest we stop that piece? (However, I'm happy to do this if I can
> get a little support on coding it?)
> 
> 
> Sounds like we have made progress?
> 
> Philip, are you cool to resubmit your patch with the adjusted GPIO ordering? (and APU7 detection as
> an APU3) We just then need to decide whether to drop LED renaming?

Regards,

Hans

Re: [PATCH v4 1/2] x86: Support APU5 & APU6 in PCEngines platform driver

[Hans de Goede]

Hi,

On 2/17/23 14:50, Enrico Weigelt, metux IT consult wrote:
> On 13.02.23 15:25, Hans de Goede wrote:
> 
>> It would be good to know the ACPI names. Also what happens with the ACPI
>> registered LED devices when the pcengines-apu driver loads, does it
>> somehow unregister those ?
> 
> It doesn't (hasn't any code for that). But the even more interesting
> question is: does the acpi driver lock the IO space so my gpio driver,
> and so the apu driver itself, can't initialize at all ? Or are we in a
> situation where two different drivers meddle with the same chip ?

Since it seems that people are using a patched version of
the pcengines-apu driver with openwrt on newer models I would assume
that the answer here is: "No the ACPI code does not lock the IO space"
although it is unclear to me how the ACPI code exposes leds at all ?

Later in the thread you write that they do some hacks to expose
all io-space of the FCH as gpios and then presumably the embed
devicetree-bits pointing to those new GPIOs into ACPI to make
the leds-gpio.c code handle the LEDs ?

> 
>> If the ACPI LEDs are not unregistered and keep working, then I guess there is no userspace
>> API breakage when using pcengines-apu on newer APU models, "just" duplicate LED devices ?
> 
> Supporting both LED name schemes on the newer boards is an interesting
> thought. Do we have some way for aliasing LED names ?
> 
> OTOH, I wonder whether we need the model specific LED naming at all.
> In the old apuv1 driver, there used to be some (unsupported) LED-only
> support for apuv2, which used model-specific naming. When adding full
> apuv2/v3 support, I specifically chose not to do this, since I don't want userland having to care about the specific model version. And the
> naming is a bit more clear on the actual meaning of these LEDs.
> 
>>> - Additionally, we already broke this in the (distant) past because there was a previous APU driver
>>> which used different names still...
>>
>> Just because we have gotten away with it once, does not mean we should do it again :)
> 
> Back then the situation was different. Haven't even found anybody who's
> was actually using this in the field. This ancient driver (actually made
> for acpuv1, which is totally different HW) was only serving the three
> front LEDs, nothing else, and blocked using the other GPIOs (eg. button)
> Some people out there did weird hacks by directly writing registers from
> userland (obviously w/o loading the ancient driver) - and even worse:
> pcengines publically adviced to so.
> 
>> For the new models I'm fine with whatever LED naming is preferred.
> 
> NAK. The problem here is: userland now has to differenciate between
> various models again. Applications suddenly need to be rewritten in
> order to work with the next higher model, or it fails. That might be not
> a problem for home users, but in the industrial field it is a huge
> problem: you suddenly end up with two product configurations or need
> extra SW complexity to cope with several models at runtime - and this
> even grows with the next one.
> 
> Exactly what I wanted to prevent once and for all.

Note that making applications work OOTB on newer platforms is nice
to have, but is not specifically a blocker from the upstream kernel
review pov.  Userspace needing to adjust to e.g. /dev/sda becoming
/dev/nvme is not considerer userspace API breakage and this is
more or less the same.

Still I agree that preserving the userspace API across different
board models is something which we want to do if possible.

> Since the meaning of these LEDs doesn't change, there's just no need to
> change their naming.

I agree that if the LEDs have the same function as before the name should
be preserved to not needlessly make life harder for userspace consumers
of these LEDs.

Regards,

Hans