[PATCH 0/3] Update pcengines-apuv2 platform device

[PATCH 0/3] Update pcengines-apuv2 platform device

[Florian Eckert]

This patchset adds the following changes to this pcengines-apuv2
platform device.

* Add mpcie reset gpio export
* Add legacy leds gpio definitions
* Update gpio buttion definitions

Florian Eckert (3):
  platform/x86/pcengines-apuv2: add mpcie reset gpio export
  platform/x86/pcengines-apuv2: add legacy leds gpio definitions
  platform//x86/pcengines-apuv2: update gpio button definition

 drivers/platform/x86/pcengines-apuv2.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

-- 
2.11.0

[PATCH 1/3] platform/x86/pcengines-apuv2: add mpcie reset gpio export

[Florian Eckert]

On APUx we have also mpcie2/mpcie3 reset pins. To make it possible to reset
the ports from the userspace, add the definition to this platform
device. The gpio can then be exported by the legancy gpio subsystem to
toggle the mpcie reset pin.

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
---
 drivers/platform/x86/pcengines-apuv2.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/platform/x86/pcengines-apuv2.c b/drivers/platform/x86/pcengines-apuv2.c
index c1ca931e1fab..f6d8ed100cab 100644
--- a/drivers/platform/x86/pcengines-apuv2.c
+++ b/drivers/platform/x86/pcengines-apuv2.c
@@ -32,6 +32,8 @@
 #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_GPIO59_DEVSLP0
+#define APU2_GPIO_REG_MPCIE3		AMD_FCH_GPIO_REG_GPIO51
 
 /* order in which the gpio lines are defined in the register list */
 #define APU2_GPIO_LINE_LED1		0
@@ -39,6 +41,8 @@
 #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
 
 /* gpio device */
 
@@ -48,6 +52,8 @@ static int apu2_gpio_regs[] = {
 	[APU2_GPIO_LINE_LED3]		= APU2_GPIO_REG_LED3,
 	[APU2_GPIO_LINE_MODESW]		= APU2_GPIO_REG_MODESW,
 	[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[] = {
@@ -56,6 +62,8 @@ static const char * const apu2_gpio_names[] = {
 	[APU2_GPIO_LINE_LED3]		= "front-led3",
 	[APU2_GPIO_LINE_MODESW]		= "front-button",
 	[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 = {
-- 
2.11.0

Re: [PATCH 1/3] platform/x86/pcengines-apuv2: add mpcie reset gpio export

[Enrico Weigelt, metux IT consult]

On 04.07.19 11:02, Florian Eckert wrote:
> On APUx we have also mpcie2/mpcie3 reset pins. To make it possible to reset
> the ports from the userspace, add the definition to this platform
> device. The gpio can then be exported by the legancy gpio subsystem to
> toggle the mpcie reset pin.

Are you sure they're also available on APUv2 (not just v3) ?

--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [PATCH 1/3] platform/x86/pcengines-apuv2: add mpcie reset gpio export

[Florian Eckert]

On 2019-07-08 21:44, Enrico Weigelt, metux IT consult wrote:
> On 04.07.19 11:02, Florian Eckert wrote:
>> On APUx we have also mpcie2/mpcie3 reset pins. To make it possible to 
>> reset
>> the ports from the userspace, add the definition to this platform
>> device. The gpio can then be exported by the legancy gpio subsystem to
>> toggle the mpcie reset pin.
> 
> Are you sure they're also available on APUv2 (not just v3) ?

We have the following models on APU2 family:
The schematic could be downloaded for all APU2 family boards from this 
side.
See https://www.pcengines.ch/apu2.htm

They all use the similar PCB with some minimal changes.

APU2
2 mpcie slot
apu2d0 (2 GB DRAM, 2 i211AT NICs)
apu2d2 (2 GB DRAM, 3 i211AT NICs)
apu2d4 (4 GB DRAM, 3 i210AT NICs)

J14 (USB + SIM1) PE3_RST to GPIO G51
J13 (USB + SIM2) PE4_RST to GPIO G55

APU3
3 mpcie slot
apu3c2 (2 GB DRAM, 3 i211AT NICs, optimized for 3G/LTE modems)
apu3c4 (4 GB DRAM, 3 i211AT NICs, optimized for 3G/LTE modems)

J16 (PCIe + USB no SIM) not connected to a userland GPIO
J15 (USB SIM1) PE4_RST to GPIO G55
J14 (mSATA or USB SIM2) PE3_RST to GPIO G51

APU4
3 mpcie slot
apu4c2 (2 GB DRAM, 4 i211AT NICs)
apu4c4 (4 GB DRAM, 4 i211AT NICs)

J15 (PCIe + USB no SIM) not connected to a userland GPIO
J14 (USB SIM1) PE4_RST to GPIO G55
J13 (mSATA or USB SIM2) PE3_RST to GPIO G51

Please check again so that I have not done any mistake.

So all USB only mpcie slots could be reseted by a GPIO G51 and G55.

Kind regards

Flo

Re: [PATCH 1/3] platform/x86/pcengines-apuv2: add mpcie reset gpio export

[Enrico Weigelt, metux IT consult]

On 04.07.19 11:02, Florian Eckert wrote:
> On APUx we have also mpcie2/mpcie3 reset pins. To make it possible to reset
> the ports from the userspace, add the definition to this platform
> device. The gpio can then be exported by the legancy gpio subsystem to
> toggle the mpcie reset pin.
>

Just tested your patch on an apu3. The driver itself seems to work,
but the pins don't seem to actually do anything.

How exactly did you test it ? Do you have some test case ?


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [PATCH 1/3] platform/x86/pcengines-apuv2: add mpcie reset gpio export

[Florian Eckert]

>> On APUx we have also mpcie2/mpcie3 reset pins. To make it possible to 
>> reset
>> the ports from the userspace, add the definition to this platform
>> device. The gpio can then be exported by the legancy gpio subsystem to
>> toggle the mpcie reset pin.
>> 
> 
> Just tested your patch on an apu3. The driver itself seems to work,
> but the pins don't seem to actually do anything.
> 
> How exactly did you test it ? Do you have some test case ?

I plugged in a mpcie  usb modem.
In my test case it was a EC25 from Quectel in mpcie2 port.
After that I did a reboot and exported the gpio via "/sys/class/gpio"
Then I executed the command "echo 0 > /sys/class/gpio/<name>/value" and 
"echo 1 > /sys/class/gpio/<name>/value".
Then I have seen the log message in the kernel that the device did an 
unregistration/registration
.
-- Florian

Re: [PATCH 1/3] platform/x86/pcengines-apuv2: add mpcie reset gpio export

[Enrico Weigelt, metux IT consult]

On 15.07.19 16:38, Florian Eckert wrote:

> I plugged in a mpcie  usb modem.
> In my test case it was a EC25 from Quectel in mpcie2 port.
> After that I did a reboot and exported the gpio via "/sys/class/gpio"
> Then I executed the command "echo 0 > /sys/class/gpio/<name>/value" and
> "echo 1 > /sys/class/gpio/<name>/value".
> Then I have seen the log message in the kernel that the device did an
> unregistration/registration

hmm, did the same, tried both gpios (mpcie2_reset, mpcie3_reset), but
nothin happened.

But strange: I'm missing some hw, modem doesn't appear ... I'll have to
check again whether i've got unstable hw or missing some kernel config.


--mtx


-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [PATCH 1/3] platform/x86/pcengines-apuv2: add mpcie reset gpio export

[Enrico Weigelt, metux IT consult]

On 04.07.19 11:02, Florian Eckert wrote:
> On APUx we have also mpcie2/mpcie3 reset pins. To make it possible to reset
> the ports from the userspace, add the definition to this platform
> device. The gpio can then be exported by the legancy gpio subsystem to
> toggle the mpcie reset pin.
> 
> Signed-off-by: Florian Eckert <fe@dev.tdt.de>
> ---
>   drivers/platform/x86/pcengines-apuv2.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/platform/x86/pcengines-apuv2.c b/drivers/platform/x86/pcengines-apuv2.c
> index c1ca931e1fab..f6d8ed100cab 100644
> --- a/drivers/platform/x86/pcengines-apuv2.c
> +++ b/drivers/platform/x86/pcengines-apuv2.c
> @@ -32,6 +32,8 @@
>   #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_GPIO59_DEVSLP0
> +#define APU2_GPIO_REG_MPCIE3		AMD_FCH_GPIO_REG_GPIO51
>   
>   /* order in which the gpio lines are defined in the register list */
>   #define APU2_GPIO_LINE_LED1		0
> @@ -39,6 +41,8 @@
>   #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
>   
>   /* gpio device */
>   
> @@ -48,6 +52,8 @@ static int apu2_gpio_regs[] = {
>   	[APU2_GPIO_LINE_LED3]		= APU2_GPIO_REG_LED3,
>   	[APU2_GPIO_LINE_MODESW]		= APU2_GPIO_REG_MODESW,
>   	[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[] = {
> @@ -56,6 +62,8 @@ static const char * const apu2_gpio_names[] = {
>   	[APU2_GPIO_LINE_LED3]		= "front-led3",
>   	[APU2_GPIO_LINE_MODESW]		= "front-button",
>   	[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 = {
> 

Acked-by: Enrico Weigelt <info@metux.net>

But note that these gpio's later will be bound to an appropriate
driver. I've got a patch in the queue that uses LED subsystem, but
haven't sorted out whether it's better to go via reset subsystem
(and add a gpio-based reset driver first).

--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

[PATCH 2/3] platform/x86/pcengines-apuv2: add legacy leds gpio definitions

[Florian Eckert]

Extend the apu2_leds definition to make the leds exportable via the
legacy gpio subsystem. Without this change the leds are not visible
under "/sys/class/leds" and could not be configured.

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
---
 drivers/platform/x86/pcengines-apuv2.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/pcengines-apuv2.c b/drivers/platform/x86/pcengines-apuv2.c
index f6d8ed100cab..d50a50e9d34c 100644
--- a/drivers/platform/x86/pcengines-apuv2.c
+++ b/drivers/platform/x86/pcengines-apuv2.c
@@ -75,9 +75,24 @@ static const struct amd_fch_gpio_pdata board_apu2 = {
 /* gpio leds device */
 
 static const struct gpio_led apu2_leds[] = {
-	{ .name = "apu:green:1" },
-	{ .name = "apu:green:2" },
-	{ .name = "apu:green:3" }
+	{
+		.name            = "apu:green:1",
+		.gpio            = 505,
+		.default_trigger = "default-off",
+		.active_low      = 1,
+	},
+	{
+		.name            = "apu:green:2",
+		.gpio            = 506,
+		.default_trigger = "default-off",
+		.active_low      = 1,
+	},
+	{
+		.name            = "apu:green:3",
+		.gpio            = 507,
+		.default_trigger = "default-off",
+		.active_low      = 1,
+	}
 };
 
 static const struct gpio_led_platform_data apu2_leds_pdata = {
-- 
2.11.0

Re: [PATCH 2/3] platform/x86/pcengines-apuv2: add legacy leds gpio definitions

[Enrico Weigelt, metux IT consult]

On 04.07.19 11:02, Florian Eckert wrote:
> Extend the apu2_leds definition to make the leds exportable via the
> legacy gpio subsystem. 

What for ? The gpios are bound to LED devices as that's exactly what
they are: LEDs.

> Without this change the leds are not visible
> under "/sys/class/leds" and could not be configured.

How so ? (I'm using exactly that all the day ...)


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [PATCH 2/3] platform/x86/pcengines-apuv2: add legacy leds gpio definitions

[Florian Eckert]

On 2019-07-08 21:42, Enrico Weigelt, metux IT consult wrote:
> On 04.07.19 11:02, Florian Eckert wrote:
>> Extend the apu2_leds definition to make the leds exportable via the
>> legacy gpio subsystem.
> 
> What for ? The gpios are bound to LED devices as that's exactly what
> they are: LEDs.

I have back ported your pcengines-apuv2 device and gpio-amd-fch GPIO 
driver to the kernel version 4.19 on OpenWrt.
If I compile and load this without the change no LEDs are visible in 
"/sys/class/leds"!

 From my point of view the connection between the GPIO and the LEDs 
subsystem is missing.
How should the LED subsystem know which GPIO to use?
If I add the change to the pcengines-apuv2 device then the LEDs will be 
visilbe under "/sys/class/leds"
and could be used, by OpenWrt userland.

Mybe I miss something.

Re: [PATCH 2/3] platform/x86/pcengines-apuv2: add legacy leds gpio definitions

[Enrico Weigelt, metux IT consult]

On 10.07.19 14:03, Florian Eckert wrote:

> I have back ported your pcengines-apuv2 device and gpio-amd-fch GPIO
> driver to the kernel version 4.19 on OpenWrt.
> If I compile and load this without the change no LEDs are visible in
> "/sys/class/leds"!

Maybe this old kernel just ignored all entries w/o gpio ID. Note that
these IDs are deprecated for quite a while and shouldn't be used in new
code anymore.

> From my point of view the connection between the GPIO and the LEDs
> subsystem is missing.
> How should the LED subsystem know which GPIO to use?

See gpiod_lookup_table.

> If I add the change to the pcengines-apuv2 device then the LEDs will be
> visilbe under "/sys/class/leds"
> and could be used, by OpenWrt userland.
> 
> Mybe I miss something.

Your patch is only valid for your backport onto this old kernel, not for
recent mainline.


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

[PATCH 3/3] platform//x86/pcengines-apuv2: update gpio button definition

[Florian Eckert]

* Add the gpio number, so the button subsystem can find the right gpio.
* Change also the keycode from KEY_SETUP to KEY_RESTART, because it
  seems more expressive to me and in the Alix-Board, which is the
  predecessor, there isthis keycode defined too. I think this is also
  intended by Pcengines. Also many embedded systems defined in the kernel
  use this key code as well.

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
---
 drivers/platform/x86/pcengines-apuv2.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/pcengines-apuv2.c b/drivers/platform/x86/pcengines-apuv2.c
index d50a50e9d34c..370fd2686d59 100644
--- a/drivers/platform/x86/pcengines-apuv2.c
+++ b/drivers/platform/x86/pcengines-apuv2.c
@@ -116,7 +116,8 @@ struct gpiod_lookup_table gpios_led_table = {
 
 static struct gpio_keys_button apu2_keys_buttons[] = {
 	{
-		.code			= KEY_SETUP,
+		.code			= KEY_RESTART,
+		.gpio			= 508,
 		.active_low		= 1,
 		.desc			= "front button",
 		.type			= EV_KEY,
-- 
2.11.0

Re: [PATCH 3/3] platform//x86/pcengines-apuv2: update gpio button definition

[Enrico Weigelt, metux IT consult]

On 04.07.19 11:02, Florian Eckert wrote:
> * Add the gpio number, so the button subsystem can find the right gpio.
> * Change also the keycode from KEY_SETUP to KEY_RESTART, because it
>   seems more expressive to me and in the Alix-Board, which is the
>   predecessor, there isthis keycode defined too. I think this is also
>   intended by Pcengines. Also many embedded systems defined in the kernel
>   use this key code as well.

In general, I'm not opposed to that, but I'll first have to check trough
various userland code to make sure that it doesn't break
anything in the field, before I can give my ack.


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [PATCH 3/3] platform//x86/pcengines-apuv2: update gpio button definition

[Enrico Weigelt, metux IT consult]

On 04.07.19 11:02, Florian Eckert wrote:
> * Add the gpio number, so the button subsystem can find the right gpio.
> * Change also the keycode from KEY_SETUP to KEY_RESTART, because it
>    seems more expressive to me and in the Alix-Board, which is the
>    predecessor, there isthis keycode defined too. I think this is also
>    intended by Pcengines. Also many embedded systems defined in the kernel
>    use this key code as well.
> 
> Signed-off-by: Florian Eckert <fe@dev.tdt.de>
> ---
>   drivers/platform/x86/pcengines-apuv2.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/pcengines-apuv2.c b/drivers/platform/x86/pcengines-apuv2.c
> index d50a50e9d34c..370fd2686d59 100644
> --- a/drivers/platform/x86/pcengines-apuv2.c
> +++ b/drivers/platform/x86/pcengines-apuv2.c
> @@ -116,7 +116,8 @@ struct gpiod_lookup_table gpios_led_table = {
>   
>   static struct gpio_keys_button apu2_keys_buttons[] = {
>   	{
> -		.code			= KEY_SETUP,
> +		.code			= KEY_RESTART,
> +		.gpio			= 508,
>   		.active_low		= 1,
>   		.desc			= "front button",
>   		.type			= EV_KEY,
> 

I'd like to ack only the keycode change, but not the deprecated .gpio 
field. I'll post a separate patch for the keycode change only.


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [PATCH 3/3] platform//x86/pcengines-apuv2: update gpio button definition

[Florian Eckert]

> I'd like to ack only the keycode change, but not the deprecated .gpio
> field. I'll post a separate patch for the keycode change only.

I am fine if we only change the keycode.
Do I have to send a v2 patch set?

Re: [PATCH 3/3] platform//x86/pcengines-apuv2: update gpio button definition

[Enrico Weigelt, metux IT consult]

On 23.07.19 09:06, Florian Eckert wrote:
>> I'd like to ack only the keycode change, but not the deprecated .gpio
>> field. I'll post a separate patch for the keycode change only.
> 
> I am fine if we only change the keycode.
> Do I have to send a v2 patch set?
> 

already sent one.

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [PATCH 0/3] Update pcengines-apuv2 platform device

[Andy Shevchenko]

On Thu, Jul 4, 2019 at 12:02 PM Florian Eckert <fe@dev.tdt.de> wrote:
>
> This patchset adds the following changes to this pcengines-apuv2
> platform device.
>

Before doing anything to this driver, what is the plan for previously
upstreamed:

drivers/leds/leds-apu.c
arch/x86/platform/geode/alix.c

?

> * Add mpcie reset gpio export
> * Add legacy leds gpio definitions
> * Update gpio buttion definitions
>
> Florian Eckert (3):
>   platform/x86/pcengines-apuv2: add mpcie reset gpio export
>   platform/x86/pcengines-apuv2: add legacy leds gpio definitions
>   platform//x86/pcengines-apuv2: update gpio button definition
>
>  drivers/platform/x86/pcengines-apuv2.c | 32 ++++++++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 4 deletions(-)
>
> --
> 2.11.0
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/3] Update pcengines-apuv2 platform device

[Florian Eckert]

Hello Andy,

thanks for feedback

>> This patchset adds the following changes to this pcengines-apuv2
>> platform device.
>> 
> 
> Before doing anything to this driver, what is the plan for previously
> upstreamed:
> 
> drivers/leds/leds-apu.c

I think we can remove the related APU2/APU3 code stuff from this driver.
The recently added pcengines-apuv2 driver does *not* support the APU1.
So I think we need the related APU1 stuff if we still want to support 
this board.

> arch/x86/platform/geode/alix.c

I think this is not related because this is a different platform driver.
Maybe we should move them to drivers/platform/x86?

Regards

Florian

Re: [PATCH 0/3] Update pcengines-apuv2 platform device

[Andy Shevchenko]

On Thu, Jul 4, 2019 at 5:00 PM Florian Eckert <fe@dev.tdt.de> wrote:
>
> Hello Andy,
>
> thanks for feedback
>
> >> This patchset adds the following changes to this pcengines-apuv2
> >> platform device.
> >>
> >
> > Before doing anything to this driver, what is the plan for previously
> > upstreamed:
> >
> > drivers/leds/leds-apu.c
>
> I think we can remove the related APU2/APU3 code stuff from this driver.
> The recently added pcengines-apuv2 driver does *not* support the APU1.
> So I think we need the related APU1 stuff if we still want to support
> this board.

So, I would like to see some unification (since it's material for v5.4
cycle anyway, we have time).

> > arch/x86/platform/geode/alix.c
>
> I think this is not related because this is a different platform driver.
> Maybe we should move them to drivers/platform/x86?

You mentioned somewhere ALIx, can you elaborate if these are platforms
of the same family (PC engines)?

Looking into the code, I think we may unify all three under umbrella
of one driver if the above is true.


--
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/3] Update pcengines-apuv2 platform device

[Florian Eckert]

Hello Andy

>> >> This patchset adds the following changes to this pcengines-apuv2
>> >> platform device.
>> >>
>> >
>> > Before doing anything to this driver, what is the plan for previously
>> > upstreamed:
>> >
>> > drivers/leds/leds-apu.c
>> 
>> I think we can remove the related APU2/APU3 code stuff from this 
>> driver.
>> The recently added pcengines-apuv2 driver does *not* support the APU1.
>> So I think we need the related APU1 stuff if we still want to support
>> this board.
> 
> So, I would like to see some unification (since it's material for v5.4
> cycle anyway, we have time).

A few thoughts and information about your suggestion to unify this.

APU1 (PC-Engines) CPU "AMD G series T40E APU":
This is also an old design and is not recommend for new design 
(deprecated).
Also not many were produced and are in the field.
See https://pcengines.ch/apu.htm

Platform-Device (LEDs, Button):
I have no platform device description found in the linux sources.
So the GPIO button should not work.

LEDs-Driver:
Only the LEDs should work with this device driver.
This is shared additonal with new APU2/APU3.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/leds/leds-apu.c

I think we should remove the APU2/APU3 stuff. This will now be handled 
by the new gpio-amd-fch.c / pcengines-apuv2.c
kombination.


APU2/APU3/APU4 (PC-Engines) CPU "AMD Embedded G series GX-412TC":
This is the newest design and is recommend for new products.
See https://pcengines.ch/apu2.htm

GPIO-Driver:
The following driver is responsible for the GPIO export and handling
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpio-amd-fch.c

Platform-Device (LEDs, Button):
This Platform description is only valid for APU2/APU3 and not for APU1.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/x86/pcengines-apuv2.c

LEDs-Driver:
We have an additional device only for LEDs this works for 
APU1/APU2/APU3.
I think we should remove the APU2/APU3 LEDs from the leds-apu device as 
mentioned above.
So this device supports only the APU1 LEDs.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/leds/leds-apu.c

We could extend and/or rename the pcengienes-apuv2 device to support 
also APU3 and the newest APU4.
The APU2 does only have LEDs Button and the MPCIE2 reset lines see my 
patch.
The APU3 does have an additional the simswap pin.
So the current pcengines-apuv2 platform is from my point of view wrong.
We should change this to the following layout and add the legacy GPIO 
numbering.

This are the following GPIOs:

APU2:
LED1
LED2
LED3
BUTTON
MPCIE2
MPCIE3

APU3:
LED1
LED2
LED3
BUTTON
MPCIE2
MPCIE3
SIMSWAP

APU4:
TODO


>> > arch/x86/platform/geode/alix.c
>> 
>> I think this is not related because this is a different platform 
>> driver.
>> Maybe we should move them to drivers/platform/x86?
> 
> You mentioned somewhere ALIx, can you elaborate if these are platforms
> of the same family (PC engines)?
> 
> Looking into the code, I think we may unify all three under umbrella
> of one driver if the above is true.

ALIX (PC-Engines) CPU "AMD Geode LX":
This is an old design we have already in use and is not recommend for 
new design (deprecated)
https://pcengines.ch/alix.htm

GPIO-Driver:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpio-cs5535.c

Platform-Device (LEDs, button):
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/platform/geode/alix.c

I think we should leave the driver as it is because this is a different 
design and has nothing to do with the PUs.
The only thing I can imagine is to move the platform device to 
"drivers/platform/x86", but this is cosmetic.
I have only mentioned the alix board to explain why I think that we 
should change the APU key code from the GPIO button to unify this.

With Best Regards,

Florian

Re: [PATCH 0/3] Update pcengines-apuv2 platform device

[Enrico Weigelt, metux IT consult]

On 05.07.19 13:36, Florian Eckert wrote:

Hi,

> APU1 (PC-Engines) CPU "AMD G series T40E APU":> This is also an old design and is not recommend for new design>
(deprecated).> Also not many were produced and are in the field.> See
https://pcengines.ch/apu.htm
Yes, and I haven't been able to get one yet, so I dropped my original
plan of supporting this once. The gpio device is very different.

> Platform-Device (LEDs, Button):> I have no platform device description found in the linux sources.> So
the GPIO button should not work.
Talking about the old driver ? It only supports the three front LEDs
directly and conflicts w/ anything else that wants to support other
GPIOs.

> Platform-Device (LEDs, Button):
> This Platform description is only valid for APU2/APU3 and not for APU1.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/x86/pcengines-apuv2.c

Correct. If we really wanna support APUv1, that most likely become a
completely separate driver, as the chipsets are very different.

> LEDs-Driver:
> We have an additional device only for LEDs this works for APU1/APU2/APU3.
> I think we should remove the APU2/APU3 LEDs from the leds-apu device as
> mentioned above.
> So this device supports only the APU1 LEDs.

ACK.

> We could extend and/or rename the pcengienes-apuv2 device to support
> also APU3 and the newest APU4.

APUv3 already is supported. APUv4 again is pretty different, and I don't
have one yet. (if anybody convices pcengines to give me some boards,
I'll add support for them :))

> The APU3 does have an additional the simswap pin.

Already planned to add it, but not yet sorted into which subsystem it
actually belongs to.

> We should change this to the following layout and add the legacy GPIO
> numbering.

First we'll have to sort out where the new pins actually belong into.
(maybe power management / regulators for the reset pins ?)

> ALIX (PC-Engines) CPU "AMD Geode LX":
> This is an old design we have already in use and is not recommend for
> new design (deprecated)
> https://pcengines.ch/alix.htm

And it's an entirely different chipset, not related at all to apu.

> I think we should leave the driver as it is because this is a different
> design and has nothing to do with the PUs.

Yes.


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [PATCH 0/3] Update pcengines-apuv2 platform device

[Enrico Weigelt, metux IT consult]

On 04.07.19 15:39, Andy Shevchenko wrote:
> On Thu, Jul 4, 2019 at 12:02 PM Florian Eckert <fe@dev.tdt.de> wrote:
>>
>> This patchset adds the following changes to this pcengines-apuv2
>> platform device.
>>
> 
> Before doing anything to this driver, what is the plan for previously
> upstreamed:
> 
> drivers/leds/leds-apu.c

Only supports the three front LEDs, nothing else. (we've got more gpios
that are not LEDs, eg. the front button, simsw, ...)

> arch/x86/platform/geode/alix.c

completely unrelated - very different chipset.


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [PATCH 0/3] Update pcengines-apuv2 platform device

[Florian Eckert]

On 2019-07-08 21:45, Enrico Weigelt, metux IT consult wrote:
> On 04.07.19 15:39, Andy Shevchenko wrote:
>> On Thu, Jul 4, 2019 at 12:02 PM Florian Eckert <fe@dev.tdt.de> wrote:
>>> 
>>> This patchset adds the following changes to this pcengines-apuv2
>>> platform device.
>>> 
>> 
>> Before doing anything to this driver, what is the plan for previously
>> upstreamed:
>> 
>> drivers/leds/leds-apu.c
> 
> Only supports the three front LEDs, nothing else. (we've got more gpios
> that are not LEDs, eg. the front button, simsw, ...)
> 
>> arch/x86/platform/geode/alix.c
> 
> completely unrelated - very different chipset.
> 
> 
> --mtx

I'm going to sum it all what we have

ALIX family boards (https://www.pcengines.ch/alix.htm):
CPU -> AMD Geode LX CPU
Stays as it is different because it has a different CPU

APU family boards (https://www.pcengines.ch/apu.htm):
CPU -> AMD G series T40E APU
Remove the related APU2 family stuff from the LEDs driver
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/leds/leds-apu.c
this will be handled in the future by the platform device
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/x86/pcengines-apuv2.c
The other GPIOs are not supported by this platform. Only LEDs are 
supported for now.

APU2 family boards (https://www.pcengines.ch/apu2.htm):
CPU -> AMD Embedded G series GX-412TC
Add the additional mpcie reset pins and add additional board 
descriptions to
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/x86/pcengines-apuv2.c?h=v5.2#n61
so we can distinguish between the APU2,APU3 and APU4 boards of the APU2 
board family.

My research in the pcengines documentation shows the following GPIO pins 
for the individual boards which we can support.

APU2:
front-led1
front-led2
front-led3
front-button
mpcie2_reset
mpcie3_reset

APU3:
front-led1
front-led2
front-led3
front-button
mpcie2_reset
mpcie3_reset
simswap

APU4:
front-led1
front-led2
front-led3
front-button
mpcie2_reset
mpcie3_reset

Until now we support aAPU2 and APU3 and treat it the same way. But the 
APU2 does not have a simswap.

Kind regards

Florian

Re: [PATCH 0/3] Update pcengines-apuv2 platform device

[Andy Shevchenko]

On Wed, Jul 10, 2019 at 3:54 PM Florian Eckert <fe@dev.tdt.de> wrote:
>
> On 2019-07-08 21:45, Enrico Weigelt, metux IT consult wrote:
> > On 04.07.19 15:39, Andy Shevchenko wrote:
> >> On Thu, Jul 4, 2019 at 12:02 PM Florian Eckert <fe@dev.tdt.de> wrote:
> >>>
> >>> This patchset adds the following changes to this pcengines-apuv2
> >>> platform device.

Guys, I'm lost with this series.
So, for now I dropped them from queue, if needed, please resend a new version.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/3] Update pcengines-apuv2 platform device

[Enrico Weigelt, metux IT consult]

On 25.07.19 19:49, Andy Shevchenko wrote:

Hi,

> On Wed, Jul 10, 2019 at 3:54 PM Florian Eckert <fe@dev.tdt.de> wrote:
>>
>> On 2019-07-08 21:45, Enrico Weigelt, metux IT consult wrote:
>>> On 04.07.19 15:39, Andy Shevchenko wrote:
>>>> On Thu, Jul 4, 2019 at 12:02 PM Florian Eckert <fe@dev.tdt.de> wrote:
>>>>>
>>>>> This patchset adds the following changes to this pcengines-apuv2
>>>>> platform device.
> 
> Guys, I'm lost with this series.
> So, for now I dropped them from queue, if needed, please resend a new version.

@Andy: please take my patch for the keycode change, which I sent
separately. (Florian's version also added raw gpio numnber).

@Florian: could you resend your patch for the reset pin addition ?


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287