[PATCH 1/3] Input: add `SW_BOOT_ALT`

[PATCH 1/3] Input: add `SW_BOOT_ALT`

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

This event code represents the firmware source to use at boot.
Value 0 means using "standard" firmware source, value 1 means using
"alternative" firmware source.

For example, some hardware has the ability to force the BOOTROM to load
the bootloader from a secondary firmware source (say SD card) instead of
trying with the standard first and then the secondary. This event allows
the userspace to know which firmware source was requested *in hardware*.

Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---
 include/linux/mod_devicetable.h        | 2 +-
 include/uapi/linux/input-event-codes.h | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 549590e9c644..009e71376a61 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -326,7 +326,7 @@ struct pcmcia_device_id {
 #define INPUT_DEVICE_ID_LED_MAX		0x0f
 #define INPUT_DEVICE_ID_SND_MAX		0x07
 #define INPUT_DEVICE_ID_FF_MAX		0x7f
-#define INPUT_DEVICE_ID_SW_MAX		0x10
+#define INPUT_DEVICE_ID_SW_MAX		0x11
 #define INPUT_DEVICE_ID_PROP_MAX	0x1f
 
 #define INPUT_DEVICE_ID_MATCH_BUS	1
diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
index dff8e7f17074..8cd2b58c81d7 100644
--- a/include/uapi/linux/input-event-codes.h
+++ b/include/uapi/linux/input-event-codes.h
@@ -917,7 +917,8 @@
 #define SW_MUTE_DEVICE		0x0e  /* set = device disabled */
 #define SW_PEN_INSERTED		0x0f  /* set = pen inserted */
 #define SW_MACHINE_COVER	0x10  /* set = cover closed */
-#define SW_MAX			0x10
+#define SW_BOOT_ALT		0x11  /* set = alternative boot firmware source */
+#define SW_MAX			0x11
 #define SW_CNT			(SW_MAX+1)
 
 /*
-- 
2.37.3

Re: [PATCH 1/3] Input: add `SW_BOOT_ALT`

[Jeff LaBundy]

Hi Quentin,

On Thu, Sep 22, 2022 at 12:12:09PM +0200, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> 
> This event code represents the firmware source to use at boot.
> Value 0 means using "standard" firmware source, value 1 means using
> "alternative" firmware source.
> 
> For example, some hardware has the ability to force the BOOTROM to load
> the bootloader from a secondary firmware source (say SD card) instead of
> trying with the standard first and then the secondary. This event allows
> the userspace to know which firmware source was requested *in hardware*.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>

This does not seem like the right approach, especially since the switch
can easily be flipped after the state is already latched.

If the bootloader needs to pass information to the kernel (boot source or
otherwise), a safer and more flexible approach is to share some variables
in eMMC, or pass information using the kernel cmdline.

> ---
>  include/linux/mod_devicetable.h        | 2 +-
>  include/uapi/linux/input-event-codes.h | 3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 549590e9c644..009e71376a61 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -326,7 +326,7 @@ struct pcmcia_device_id {
>  #define INPUT_DEVICE_ID_LED_MAX		0x0f
>  #define INPUT_DEVICE_ID_SND_MAX		0x07
>  #define INPUT_DEVICE_ID_FF_MAX		0x7f
> -#define INPUT_DEVICE_ID_SW_MAX		0x10
> +#define INPUT_DEVICE_ID_SW_MAX		0x11
>  #define INPUT_DEVICE_ID_PROP_MAX	0x1f
>  
>  #define INPUT_DEVICE_ID_MATCH_BUS	1
> diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
> index dff8e7f17074..8cd2b58c81d7 100644
> --- a/include/uapi/linux/input-event-codes.h
> +++ b/include/uapi/linux/input-event-codes.h
> @@ -917,7 +917,8 @@
>  #define SW_MUTE_DEVICE		0x0e  /* set = device disabled */
>  #define SW_PEN_INSERTED		0x0f  /* set = pen inserted */
>  #define SW_MACHINE_COVER	0x10  /* set = cover closed */
> -#define SW_MAX			0x10
> +#define SW_BOOT_ALT		0x11  /* set = alternative boot firmware source */
> +#define SW_MAX			0x11
>  #define SW_CNT			(SW_MAX+1)
>  
>  /*
> -- 
> 2.37.3
> 

Kind regards,
Jeff LaBundy

Re: [PATCH 1/3] Input: add `SW_BOOT_ALT`

[Quentin Schulz]

Hi Jeff,

On 9/22/22 19:20, Jeff LaBundy wrote:
> Hi Quentin,
> 
> On Thu, Sep 22, 2022 at 12:12:09PM +0200, Quentin Schulz wrote:
>> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>>
>> This event code represents the firmware source to use at boot.
>> Value 0 means using "standard" firmware source, value 1 means using
>> "alternative" firmware source.
>>
>> For example, some hardware has the ability to force the BOOTROM to load
>> the bootloader from a secondary firmware source (say SD card) instead of
>> trying with the standard first and then the secondary. This event allows
>> the userspace to know which firmware source was requested *in hardware*.
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> 
> This does not seem like the right approach, especially since the switch
> can easily be flipped after the state is already latched.
> 
> If the bootloader needs to pass information to the kernel (boot source or
> otherwise), a safer and more flexible approach is to share some variables
> in eMMC, or pass information using the kernel cmdline.
> 

I made a terrible job at explaining what this switch is for, sorry.

Obviously, the state of the switch cannot represent which firmware boot 
source was used as only the bootloader will be able to tell (since it 
usually tries storage media in a specific order and the primary boot 
source could get corrupted at one point in time). Anyway, like you 
rightfully stated, this is useless "info" and the important one would be 
passed by the bootloader to the kernel (possibly via Device Tree fixup 
in case of Aarch64). U-Boot does this to set the memory node so this 
could be done again with a different property or something like that. 
Anyways, not something I'm really interested in.

I have a switch on my devkit which implements the 
BOOT_ALT#/BIOS_DISABLE# functionality from the Q7 standard, see section 
3.1.17 Miscellaneous Signals in the specs: 
https://sget.org/wp-content/uploads/2018/09/Qseven-Spec_2.1.pdf

"""
BIOS_DISABLE#
/BOOT_ALT#
Module BIOS disable input signal. Pull low to disable
module's on-board BIOS. Allows off-module BIOS
implementations. This signal can also be used to disable
standard boot firmware flash device and enable an alternative
boot firmware source, for example a boot loader.
"""

This is basically the configuration of the firmware boot source to use 
for *the next boot*. It does not represent which boot source was used, 
nor which one will effectively be used.

In our case, this switch electrically disables eMMC and SPI flashes and 
only allow to boot from SD card (this switch is then electrically 
overridden by another GPIO at runtime by the bootloader/Linux kernel, 
but the state of the switch is still available to the user via another 
GPIO).

I have this switch on the board and I want to expose its state to the 
user, if this new event code is not possible/a good idea what would you 
suggest we could use?

Note that we already support the same switch but in a different way: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399-puma-haikou.dts#n167
We are just configuring the GPIOs into the GPIO mode with a pull-up, and 
then it's up to the user to use gpiod or gpio-sysfs to check the state 
of the GPIO used for this switch. I don't like this, very not 
user-friendly and was looking for something better :)

Hope I explained myself a bit better this time, lemme know if I can 
clarify anything.

Thanks!
Quentin

Re: [PATCH 1/3] Input: add `SW_BOOT_ALT`

[Jeff LaBundy]

Hi Quentin,

On Fri, Sep 23, 2022 at 10:25:58AM +0200, Quentin Schulz wrote:
> Hi Jeff,
> 
> On 9/22/22 19:20, Jeff LaBundy wrote:
> > Hi Quentin,
> > 
> > On Thu, Sep 22, 2022 at 12:12:09PM +0200, Quentin Schulz wrote:
> > > From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > > 
> > > This event code represents the firmware source to use at boot.
> > > Value 0 means using "standard" firmware source, value 1 means using
> > > "alternative" firmware source.
> > > 
> > > For example, some hardware has the ability to force the BOOTROM to load
> > > the bootloader from a secondary firmware source (say SD card) instead of
> > > trying with the standard first and then the secondary. This event allows
> > > the userspace to know which firmware source was requested *in hardware*.
> > > 
> > > Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > 
> > This does not seem like the right approach, especially since the switch
> > can easily be flipped after the state is already latched.
> > 
> > If the bootloader needs to pass information to the kernel (boot source or
> > otherwise), a safer and more flexible approach is to share some variables
> > in eMMC, or pass information using the kernel cmdline.
> > 
> 
> I made a terrible job at explaining what this switch is for, sorry.
> 
> Obviously, the state of the switch cannot represent which firmware boot
> source was used as only the bootloader will be able to tell (since it
> usually tries storage media in a specific order and the primary boot source
> could get corrupted at one point in time). Anyway, like you rightfully
> stated, this is useless "info" and the important one would be passed by the
> bootloader to the kernel (possibly via Device Tree fixup in case of
> Aarch64). U-Boot does this to set the memory node so this could be done
> again with a different property or something like that. Anyways, not
> something I'm really interested in.
> 
> I have a switch on my devkit which implements the BOOT_ALT#/BIOS_DISABLE#
> functionality from the Q7 standard, see section 3.1.17 Miscellaneous Signals
> in the specs:
> https://sget.org/wp-content/uploads/2018/09/Qseven-Spec_2.1.pdf
> 
> """
> BIOS_DISABLE#
> /BOOT_ALT#
> Module BIOS disable input signal. Pull low to disable
> module's on-board BIOS. Allows off-module BIOS
> implementations. This signal can also be used to disable
> standard boot firmware flash device and enable an alternative
> boot firmware source, for example a boot loader.
> """
> 
> This is basically the configuration of the firmware boot source to use for
> *the next boot*. It does not represent which boot source was used, nor which
> one will effectively be used.
> 
> In our case, this switch electrically disables eMMC and SPI flashes and only
> allow to boot from SD card (this switch is then electrically overridden by
> another GPIO at runtime by the bootloader/Linux kernel, but the state of the
> switch is still available to the user via another GPIO).

Thanks for the additional detail and the use-case is quite clear; I just
don't think input is the right home for this. Input makes more sense for
switches that a user may change during runtime with the expectation that
an event handler effects some sort of response.

Such is the case for lid open/close and headphone insertion, but here we
are just interested in the state of a muxed GPIO.

> 
> I have this switch on the board and I want to expose its state to the user,
> if this new event code is not possible/a good idea what would you suggest we
> could use?
> 
> Note that we already support the same switch but in a different way: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399-puma-haikou.dts#n167
> We are just configuring the GPIOs into the GPIO mode with a pull-up, and
> then it's up to the user to use gpiod or gpio-sysfs to check the state of
> the GPIO used for this switch. I don't like this, very not user-friendly and
> was looking for something better :)

Actually, that's exactly what I was going to suggest. What in particular
is not user-friendly about it?

Of course, this is just my opinion as a fellow customer of input and it
is ultimately up to Dmitry.

> 
> Hope I explained myself a bit better this time, lemme know if I can clarify
> anything.
> 
> Thanks!
> Quentin

Kind regards,
Jeff LaBundy

Re: [PATCH 1/3] Input: add `SW_BOOT_ALT`

[Dmitry Torokhov]

On Fri, Sep 23, 2022 at 12:19:11PM -0500, Jeff LaBundy wrote:
> Hi Quentin,
> 
> On Fri, Sep 23, 2022 at 10:25:58AM +0200, Quentin Schulz wrote:
> > Hi Jeff,
> > 
> > On 9/22/22 19:20, Jeff LaBundy wrote:
> > > Hi Quentin,
> > > 
> > > On Thu, Sep 22, 2022 at 12:12:09PM +0200, Quentin Schulz wrote:
> > > > From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > > > 
> > > > This event code represents the firmware source to use at boot.
> > > > Value 0 means using "standard" firmware source, value 1 means using
> > > > "alternative" firmware source.
> > > > 
> > > > For example, some hardware has the ability to force the BOOTROM to load
> > > > the bootloader from a secondary firmware source (say SD card) instead of
> > > > trying with the standard first and then the secondary. This event allows
> > > > the userspace to know which firmware source was requested *in hardware*.
> > > > 
> > > > Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > > 
> > > This does not seem like the right approach, especially since the switch
> > > can easily be flipped after the state is already latched.
> > > 
> > > If the bootloader needs to pass information to the kernel (boot source or
> > > otherwise), a safer and more flexible approach is to share some variables
> > > in eMMC, or pass information using the kernel cmdline.
> > > 
> > 
> > I made a terrible job at explaining what this switch is for, sorry.
> > 
> > Obviously, the state of the switch cannot represent which firmware boot
> > source was used as only the bootloader will be able to tell (since it
> > usually tries storage media in a specific order and the primary boot source
> > could get corrupted at one point in time). Anyway, like you rightfully
> > stated, this is useless "info" and the important one would be passed by the
> > bootloader to the kernel (possibly via Device Tree fixup in case of
> > Aarch64). U-Boot does this to set the memory node so this could be done
> > again with a different property or something like that. Anyways, not
> > something I'm really interested in.
> > 
> > I have a switch on my devkit which implements the BOOT_ALT#/BIOS_DISABLE#
> > functionality from the Q7 standard, see section 3.1.17 Miscellaneous Signals
> > in the specs:
> > https://sget.org/wp-content/uploads/2018/09/Qseven-Spec_2.1.pdf
> > 
> > """
> > BIOS_DISABLE#
> > /BOOT_ALT#
> > Module BIOS disable input signal. Pull low to disable
> > module's on-board BIOS. Allows off-module BIOS
> > implementations. This signal can also be used to disable
> > standard boot firmware flash device and enable an alternative
> > boot firmware source, for example a boot loader.
> > """
> > 
> > This is basically the configuration of the firmware boot source to use for
> > *the next boot*. It does not represent which boot source was used, nor which
> > one will effectively be used.
> > 
> > In our case, this switch electrically disables eMMC and SPI flashes and only
> > allow to boot from SD card (this switch is then electrically overridden by
> > another GPIO at runtime by the bootloader/Linux kernel, but the state of the
> > switch is still available to the user via another GPIO).
> 
> Thanks for the additional detail and the use-case is quite clear; I just
> don't think input is the right home for this. Input makes more sense for
> switches that a user may change during runtime with the expectation that
> an event handler effects some sort of response.
> 
> Such is the case for lid open/close and headphone insertion, but here we
> are just interested in the state of a muxed GPIO.
> 
> > 
> > I have this switch on the board and I want to expose its state to the user,
> > if this new event code is not possible/a good idea what would you suggest we
> > could use?
> > 
> > Note that we already support the same switch but in a different way: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399-puma-haikou.dts#n167
> > We are just configuring the GPIOs into the GPIO mode with a pull-up, and
> > then it's up to the user to use gpiod or gpio-sysfs to check the state of
> > the GPIO used for this switch. I don't like this, very not user-friendly and
> > was looking for something better :)
> 
> Actually, that's exactly what I was going to suggest. What in particular
> is not user-friendly about it?
> 
> Of course, this is just my opinion as a fellow customer of input and it
> is ultimately up to Dmitry.

I agree with Jeff that input subsystem is not the right place for this
one-off platform feature. Just having userspace piece observing some
GPIO state should be fine for this. The layers dealing with it and
layers dealing with typical user interaction will be quite different,
so I do not see the reason for including the boot mode into the list
of input switches.

Thanks.

-- 
Dmitry

Re: [PATCH 1/3] Input: add `SW_BOOT_ALT`

[Quentin Schulz]

Hi all,

On 9/23/22 19:19, Jeff LaBundy wrote:
[...]
>>
>> I have this switch on the board and I want to expose its state to the user,
>> if this new event code is not possible/a good idea what would you suggest we
>> could use?
>>
>> Note that we already support the same switch but in a different way: https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_torvalds_linux.git_tree_arch_arm64_boot_dts_rockchip_rk3399-2Dpuma-2Dhaikou.dts-23n167&d=DwIBAg&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=vObtDja6hUlL_kVvKRILCvaQfklw1gl7TAW7cm5_9H_Yi00zkzGV6-D3yzazR2gU&s=0Xo-7CDmMoriaTNn_hJdMwftLlcY-tgopPcgzXfQtJE&e=
>> We are just configuring the GPIOs into the GPIO mode with a pull-up, and
>> then it's up to the user to use gpiod or gpio-sysfs to check the state of
>> the GPIO used for this switch. I don't like this, very not user-friendly and
>> was looking for something better :)
> 
> Actually, that's exactly what I was going to suggest. What in particular
> is not user-friendly about it?
> 

The implementation we currently have in place for Puma Haikou is just 
muxing the pins in their GPIO function with a pull-up. Nothing else. 
This means the user still needs to know exactly which GPIO number is for 
which signal/event. Not that great. Also does not allow for a consistent 
user "experience" across modules on the same devkit (different GPIOs 
depending on the SoM).

I had hoped gpio-hog in input mode would expose it to userspace but it 
does not seem to?

Anything to recommend so that we can export a GPIO line for a specific 
function to usersapce without needing the user to know which GPIO number 
they should look at?

While I have your attention, I was wondering about the BATTERY event. We 
have a BATLOW button, following the Q7 standard:
"""
Battery low input. This signal may be driven active low by external
circuitry to signal that the system battery is low or may be used to
signal some other external battery management event.
"""

Oddly vague, but the EV_BATTERY event is not really more precise though 
(at least didn't find more precise information and the introducing 
commit is a bit vague too on the usage). Was just wondering if there was 
any specific usage for this event or if I'm using it correctly here?

Cheers,
Quentin

Re: [PATCH 1/3] Input: add `SW_BOOT_ALT`

[Jeff LaBundy]

Hi Quentin,

On Mon, Sep 26, 2022 at 11:37:45AM +0200, Quentin Schulz wrote:
> Hi all,
> 
> On 9/23/22 19:19, Jeff LaBundy wrote:
> [...]
> > > 
> > > I have this switch on the board and I want to expose its state to the user,
> > > if this new event code is not possible/a good idea what would you suggest we
> > > could use?
> > > 
> > > Note that we already support the same switch but in a different way: https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_torvalds_linux.git_tree_arch_arm64_boot_dts_rockchip_rk3399-2Dpuma-2Dhaikou.dts-23n167&d=DwIBAg&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=vObtDja6hUlL_kVvKRILCvaQfklw1gl7TAW7cm5_9H_Yi00zkzGV6-D3yzazR2gU&s=0Xo-7CDmMoriaTNn_hJdMwftLlcY-tgopPcgzXfQtJE&e=
> > > We are just configuring the GPIOs into the GPIO mode with a pull-up, and
> > > then it's up to the user to use gpiod or gpio-sysfs to check the state of
> > > the GPIO used for this switch. I don't like this, very not user-friendly and
> > > was looking for something better :)
> > 
> > Actually, that's exactly what I was going to suggest. What in particular
> > is not user-friendly about it?
> > 
> 
> The implementation we currently have in place for Puma Haikou is just muxing
> the pins in their GPIO function with a pull-up. Nothing else. This means the
> user still needs to know exactly which GPIO number is for which
> signal/event. Not that great. Also does not allow for a consistent user
> "experience" across modules on the same devkit (different GPIOs depending on
> the SoM).
> 
> I had hoped gpio-hog in input mode would expose it to userspace but it does
> not seem to?

No, I don't believe that is the case. That being said, you should be
able to grep /sys/kernel/debug/gpio for the hog node's label and get
the pin state (lo or hi).

> 
> Anything to recommend so that we can export a GPIO line for a specific
> function to usersapce without needing the user to know which GPIO number
> they should look at?

If debugfs won't work for your purposes, another option is to request
the GPIO from a general-purpose platform driver and return one of two
strings via sysfs.

> 
> While I have your attention, I was wondering about the BATTERY event. We
> have a BATLOW button, following the Q7 standard:
> """
> Battery low input. This signal may be driven active low by external
> circuitry to signal that the system battery is low or may be used to
> signal some other external battery management event.
> """
> 
> Oddly vague, but the EV_BATTERY event is not really more precise though (at
> least didn't find more precise information and the introducing commit is a
> bit vague too on the usage). Was just wondering if there was any specific
> usage for this event or if I'm using it correctly here?

Do you mean KEY_BATTERY?

> 
> Cheers,
> Quentin

Kind regards,
Jeff LaBundy

Re: [PATCH 1/3] Input: add `SW_BOOT_ALT`

[Quentin Schulz]

Hi Jeff,

On 9/27/22 03:56, Jeff LaBundy wrote:
> Hi Quentin,
> 
> On Mon, Sep 26, 2022 at 11:37:45AM +0200, Quentin Schulz wrote:
>> Hi all,
>>
>> On 9/23/22 19:19, Jeff LaBundy wrote:
>> [...]
>>>>
>>>> I have this switch on the board and I want to expose its state to the user,
>>>> if this new event code is not possible/a good idea what would you suggest we
>>>> could use?
>>>>
>>>> Note that we already support the same switch but in a different way: https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_torvalds_linux.git_tree_arch_arm64_boot_dts_rockchip_rk3399-2Dpuma-2Dhaikou.dts-23n167&d=DwIBAg&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=vObtDja6hUlL_kVvKRILCvaQfklw1gl7TAW7cm5_9H_Yi00zkzGV6-D3yzazR2gU&s=0Xo-7CDmMoriaTNn_hJdMwftLlcY-tgopPcgzXfQtJE&e=
>>>> We are just configuring the GPIOs into the GPIO mode with a pull-up, and
>>>> then it's up to the user to use gpiod or gpio-sysfs to check the state of
>>>> the GPIO used for this switch. I don't like this, very not user-friendly and
>>>> was looking for something better :)
>>>
>>> Actually, that's exactly what I was going to suggest. What in particular
>>> is not user-friendly about it?
>>>
>>
>> The implementation we currently have in place for Puma Haikou is just muxing
>> the pins in their GPIO function with a pull-up. Nothing else. This means the
>> user still needs to know exactly which GPIO number is for which
>> signal/event. Not that great. Also does not allow for a consistent user
>> "experience" across modules on the same devkit (different GPIOs depending on
>> the SoM).
>>
>> I had hoped gpio-hog in input mode would expose it to userspace but it does
>> not seem to?
> 
> No, I don't believe that is the case. That being said, you should be
> able to grep /sys/kernel/debug/gpio for the hog node's label and get
> the pin state (lo or hi).
> 
>>
>> Anything to recommend so that we can export a GPIO line for a specific
>> function to usersapce without needing the user to know which GPIO number
>> they should look at?
> 
> If debugfs won't work for your purposes, another option is to request
> the GPIO from a general-purpose platform driver and return one of two
> strings via sysfs.
> 

This board will be fully supported upstream and I don't feel like this 
general-purpose platform driver would be accepted, but maybe I'm wrong.

I didn't know about the /sys/kernel/debug/gpio file just yet for some 
reason, so that's one way to do it, thanks for the info.

Good thing is that the kernel requests this pin in GPIO input mode so at 
least that's one less possible misconfiguration from userspace. Not so 
good thing is that you need to grep in this file and it's not that great 
for scripts :) We'll discuss internally what feels the best to us and go 
for it, thanks for the tips!

I'm wondering if allowing sysfs/libgpiod to access the state of GPIOs 
requested by the kernel would make sense on the subsystem level though. 
I'm sure I'm missing obvious security/HW-limitations pitfalls, but I 
think allowing userspace to get the value of a GPIO input pin requested 
by the kernel would be nice? Nothing for the input subsystem folks though :)

>>
>> While I have your attention, I was wondering about the BATTERY event. We
>> have a BATLOW button, following the Q7 standard:
>> """
>> Battery low input. This signal may be driven active low by external
>> circuitry to signal that the system battery is low or may be used to
>> signal some other external battery management event.
>> """
>>
>> Oddly vague, but the EV_BATTERY event is not really more precise though (at
>> least didn't find more precise information and the introducing commit is a
>> bit vague too on the usage). Was just wondering if there was any specific
>> usage for this event or if I'm using it correctly here?
> 
> Do you mean KEY_BATTERY?
> 

Yes sorry.

Cheers,
Quentin

Re: [PATCH 1/3] Input: add `SW_BOOT_ALT`

[Robin Murphy]

On 2022-09-27 09:51, Quentin Schulz wrote:
> Hi Jeff,
> 
> On 9/27/22 03:56, Jeff LaBundy wrote:
>> Hi Quentin,
>>
>> On Mon, Sep 26, 2022 at 11:37:45AM +0200, Quentin Schulz wrote:
>>> Hi all,
>>>
>>> On 9/23/22 19:19, Jeff LaBundy wrote:
>>> [...]
>>>>>
>>>>> I have this switch on the board and I want to expose its state to 
>>>>> the user,
>>>>> if this new event code is not possible/a good idea what would you 
>>>>> suggest we
>>>>> could use?
>>>>>
>>>>> Note that we already support the same switch but in a different 
>>>>> way: 
>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_torvalds_linux.git_tree_arch_arm64_boot_dts_rockchip_rk3399-2Dpuma-2Dhaikou.dts-23n167&d=DwIBAg&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=vObtDja6hUlL_kVvKRILCvaQfklw1gl7TAW7cm5_9H_Yi00zkzGV6-D3yzazR2gU&s=0Xo-7CDmMoriaTNn_hJdMwftLlcY-tgopPcgzXfQtJE&e=
>>>>> We are just configuring the GPIOs into the GPIO mode with a 
>>>>> pull-up, and
>>>>> then it's up to the user to use gpiod or gpio-sysfs to check the 
>>>>> state of
>>>>> the GPIO used for this switch. I don't like this, very not 
>>>>> user-friendly and
>>>>> was looking for something better :)
>>>>
>>>> Actually, that's exactly what I was going to suggest. What in 
>>>> particular
>>>> is not user-friendly about it?
>>>>
>>>
>>> The implementation we currently have in place for Puma Haikou is just 
>>> muxing
>>> the pins in their GPIO function with a pull-up. Nothing else. This 
>>> means the
>>> user still needs to know exactly which GPIO number is for which
>>> signal/event. Not that great. Also does not allow for a consistent user
>>> "experience" across modules on the same devkit (different GPIOs 
>>> depending on
>>> the SoM).
>>>
>>> I had hoped gpio-hog in input mode would expose it to userspace but 
>>> it does
>>> not seem to?
>>
>> No, I don't believe that is the case. That being said, you should be
>> able to grep /sys/kernel/debug/gpio for the hog node's label and get
>> the pin state (lo or hi).
>>
>>>
>>> Anything to recommend so that we can export a GPIO line for a specific
>>> function to usersapce without needing the user to know which GPIO number
>>> they should look at?
>>
>> If debugfs won't work for your purposes, another option is to request
>> the GPIO from a general-purpose platform driver and return one of two
>> strings via sysfs.
>>
> 
> This board will be fully supported upstream and I don't feel like this 
> general-purpose platform driver would be accepted, but maybe I'm wrong.
> 
> I didn't know about the /sys/kernel/debug/gpio file just yet for some 
> reason, so that's one way to do it, thanks for the info.
> 
> Good thing is that the kernel requests this pin in GPIO input mode so at 
> least that's one less possible misconfiguration from userspace. Not so 
> good thing is that you need to grep in this file and it's not that great 
> for scripts :) We'll discuss internally what feels the best to us and go 
> for it, thanks for the tips!
> 
> I'm wondering if allowing sysfs/libgpiod to access the state of GPIOs 
> requested by the kernel would make sense on the subsystem level though. 
> I'm sure I'm missing obvious security/HW-limitations pitfalls, but I 
> think allowing userspace to get the value of a GPIO input pin requested 
> by the kernel would be nice? Nothing for the input subsystem folks 
> though :)

FWIW there was a series a while back proposing to add a boot status 
interface, but I'm not sure if there was any further progress:

https://lore.kernel.org/lkml/20220204072234.304543-1-joel@jms.id.au/

Either way though, these kinds of GPIO (or ADC) configuration inputs for 
things like boot functionality or board ID are just about common enough 
that even if not tied in to any other specific interface, I reckon there 
might be a valid argument to be made for a simple generic driver for 
reporting the state of a named configuration input in a standardised 
manner. Perhaps hwmon might be a better fit?

Robin.

[PATCH 2/3] arm64: dts: rockchip: add PX30-µQ7 (Ringneck) SoM with Haikou baseboard

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

The PX30-uQ7 (Ringneck) SoM is a µQseven-compatible (40mmx70mm, MXM-230
connector) system-on-module from Theobroma Systems[1], featuring the
Rockchip PX30.

It provides the following feature set:
 * up to 4GB DDR4
 * up to 128GB on-module eMMC (with 8-bit 1.8V interface)
 * SD card (on a baseboard) via edge connector
 * Fast Ethernet with on-module TI DP83825I PHY
 * MIPI-DSI/LVDS
 * MIPI-CSI
 * USB
   - 1x USB 2.0 dual-role
   - 3x USB 2.0 host
 * on-module STM32 Cortex-M0 companion controller, implementing:
   - low-power RTC functionality (ISL1208 emulation)
   - fan controller (AMC6821 emulation)
   - USB<->CAN bridge controller
 * on-module Espressif ESP32 for Bluetooth + 2.4GHz WiFi
 * on-module NXP SE05x Secure Element

[1] https://www.theobroma-systems.com/som-product/px30-%C2%B5q7/

Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---
 arch/arm64/boot/dts/rockchip/Makefile         |   1 +
 .../dts/rockchip/px30-ringneck-haikou.dts     | 239 ++++++++++++
 .../boot/dts/rockchip/px30-ringneck.dtsi      | 347 ++++++++++++++++++
 3 files changed, 587 insertions(+)
 create mode 100644 arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts
 create mode 100644 arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi

diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
index ef79a672804a..1e3b9d7c1a8b 100644
--- a/arch/arm64/boot/dts/rockchip/Makefile
+++ b/arch/arm64/boot/dts/rockchip/Makefile
@@ -3,6 +3,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += px30-evb.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += px30-engicam-px30-core-ctouch2.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += px30-engicam-px30-core-ctouch2-of10.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += px30-engicam-px30-core-edimm2.2.dtb
+dtb-$(CONFIG_ARCH_ROCKCHIP) += px30-ringneck-haikou.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3308-evb.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3308-roc-cc.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3308-rock-pi-s.dtb
diff --git a/arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts b/arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts
new file mode 100644
index 000000000000..6304d6f7baa3
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts
@@ -0,0 +1,239 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (c) 2022 Theobroma Systems Design und Consulting GmbH
+ */
+
+/dts-v1/;
+#include "px30-ringneck.dtsi"
+#include <dt-bindings/input/input.h>
+
+/ {
+	model = "Theobroma Systems PX30-µQ7 SoM on Haikou devkit";
+	compatible = "tsd,px30-ringneck-haikou", "rockchip,px30";
+
+	aliases {
+		mmc2 = &sdmmc;
+	};
+
+	chosen {
+		stdout-path = "serial0:115200n8";
+	};
+
+	gpio_keys: haikou-keys {
+		compatible = "gpio-keys";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		pinctrl-0 = <&haikou_keys_pin>;
+		pinctrl-names = "default";
+
+		BATLOWn {
+			label = "BATLOW#";
+			linux,code = <KEY_BATTERY>;
+			gpios = <&gpio3 RK_PA7 GPIO_ACTIVE_LOW>;
+		};
+
+		BIOS_DISABLEn {
+			label = "BIOS_DISABLE#";
+			linux,code = <SW_BOOT_ALT>;
+			linux,input-type = <EV_SW>;
+			gpios = <&gpio2 RK_PC2 GPIO_ACTIVE_HIGH>;
+		};
+
+		LID_BTNn {
+			label = "LID_BTN#";
+			linux,code = <SW_LID>;
+			linux,input-type = <EV_SW>;
+			gpios = <&gpio3 RK_PA6 GPIO_ACTIVE_LOW>;
+		};
+
+		SLP_BTNn {
+			label = "SLP_BTN#";
+			linux,code = <KEY_SLEEP>;
+			gpios = <&gpio3 RK_PA5 GPIO_ACTIVE_LOW>;
+		};
+
+		WAKEn {
+			label = "WAKE#";
+			linux,code = <KEY_WAKEUP>;
+			gpios = <&gpio3 RK_PA4 GPIO_ACTIVE_LOW>;
+			wakeup-source;
+		};
+	};
+
+	leds {
+		pinctrl-0 = <&module_led_pin>, <&sd_card_led_pin>;
+
+		sd_card_led: led-1 {
+			label = "sd_card_led";
+			gpios = <&gpio3 RK_PB3 GPIO_ACTIVE_HIGH>;
+			linux,default-trigger = "mmc2";
+		};
+	};
+
+	i2s0-sound {
+		compatible = "simple-audio-card";
+		simple-audio-card,format = "i2s";
+		simple-audio-card,name = "Haikou,I2S-codec";
+		simple-audio-card,mclk-fs = <512>;
+
+		simple-audio-card,codec {
+			clocks = <&sgtl5000_clk>;
+			sound-dai = <&sgtl5000>;
+		};
+
+		simple-audio-card,cpu {
+			bitclock-master;
+			frame-master;
+			sound-dai = <&i2s0_8ch>;
+		};
+	};
+
+	sgtl5000_clk: sgtl5000-oscillator {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency  = <24576000>;
+	};
+
+	dc_12v: dc-12v {
+		compatible = "regulator-fixed";
+		regulator-name = "dc_12v";
+		regulator-always-on;
+		regulator-boot-on;
+		regulator-min-microvolt = <12000000>;
+		regulator-max-microvolt = <12000000>;
+	};
+
+	vcc3v3_baseboard: vcc3v3-baseboard {
+		compatible = "regulator-fixed";
+		regulator-name = "vcc3v3_baseboard";
+		regulator-always-on;
+		regulator-boot-on;
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		vin-supply = <&dc_12v>;
+	};
+
+	vcc5v0_baseboard: vcc5v0-baseboard {
+		compatible = "regulator-fixed";
+		regulator-name = "vcc5v0_baseboard";
+		regulator-always-on;
+		regulator-boot-on;
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		vin-supply = <&dc_12v>;
+	};
+
+	vdda_codec: vdda-codec {
+		compatible = "regulator-fixed";
+		regulator-name = "vdda_codec";
+		regulator-boot-on;
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		vin-supply = <&vcc5v0_baseboard>;
+	};
+
+	vddd_codec: vddd-codec {
+		compatible = "regulator-fixed";
+		regulator-name = "vddd_codec";
+		regulator-boot-on;
+		regulator-min-microvolt = <1600000>;
+		regulator-max-microvolt = <1600000>;
+		vin-supply = <&vcc5v0_baseboard>;
+	};
+};
+
+&i2c2 {
+	status = "okay";
+	clock-frequency = <400000>;
+
+	sgtl5000: codec@a {
+		compatible = "fsl,sgtl5000";
+		reg = <0x0a>;
+		clocks = <&sgtl5000_clk>;
+		#sound-dai-cells = <0>;
+		VDDA-supply = <&vdda_codec>;
+		VDDIO-supply = <&vcc3v3_baseboard>;
+		VDDD-supply = <&vddd_codec>;
+		status = "okay";
+	};
+};
+
+&i2c3 {
+	status = "okay";
+
+	eeprom@50 {
+		reg = <0x50>;
+		compatible = "atmel,24c01";
+		pagesize = <8>;
+		size = <128>;
+		vcc-supply = <&vcc3v3_baseboard>;
+	};
+};
+
+&i2s0_8ch {
+	status = "okay";
+};
+
+&gmac {
+	status = "okay";
+};
+
+&pinctrl {
+	haikou {
+		haikou_keys_pin: haikou-keys-pin {
+			rockchip,pins =
+			  /* WAKE# */
+			  <3 RK_PA4 RK_FUNC_GPIO &pcfg_pull_up>,
+			  /* SLP_BTN# */
+			  <3 RK_PA5 RK_FUNC_GPIO &pcfg_pull_up>,
+			  /* LID_BTN */
+			  <3 RK_PA6 RK_FUNC_GPIO &pcfg_pull_up>,
+			  /* BATLOW# */
+			  <3 RK_PA7 RK_FUNC_GPIO &pcfg_pull_up>,
+			  /* BIOS_DISABLE# */
+			  <2 RK_PC2 RK_FUNC_GPIO &pcfg_pull_up>;
+		};
+	};
+
+	leds {
+		sd_card_led_pin: sd-card-led-pin {
+			rockchip,pins =
+			  <3 RK_PB3 RK_FUNC_GPIO &pcfg_pull_none>;
+		};
+	};
+};
+
+&pwm0 {
+	status = "okay";
+};
+
+&sdmmc {
+	bus-width = <4>;
+	cap-mmc-highspeed;
+	cap-sd-highspeed;
+	cd-gpios = <&gpio0 RK_PA3 GPIO_ACTIVE_LOW>;
+	disable-wp;
+	vmmc-supply = <&vcc3v3_baseboard>;
+	status = "okay";
+};
+
+&spi1 {
+	status = "okay";
+};
+
+&u2phy_otg {
+	status = "okay";
+};
+
+&uart0 {
+	status = "okay";
+};
+
+&uart5 {
+	pinctrl-0 = <&uart5_xfer>;
+	status = "okay";
+};
+
+&usb20_otg {
+	status = "okay";
+};
diff --git a/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi b/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi
new file mode 100644
index 000000000000..74a7f1182c21
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi
@@ -0,0 +1,347 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (c) 2022 Theobroma Systems Design und Consulting GmbH
+ */
+
+/dts-v1/;
+#include "px30.dtsi"
+
+/ {
+	aliases {
+		mmc0 = &emmc;
+		mmc1 = &sdio;
+		rtc0 = &rtc_twi;
+		rtc1 = &rk809;
+	};
+
+	emmc_pwrseq: emmc-pwrseq {
+		compatible = "mmc-pwrseq-emmc";
+		pinctrl-0 = <&emmc_reset>;
+		pinctrl-names = "default";
+		reset-gpios = <&gpio1 RK_PB3 GPIO_ACTIVE_HIGH>;
+	};
+
+	leds {
+		compatible = "gpio-leds";
+		pinctrl-names = "default";
+		pinctrl-0 = <&module_led_pin>;
+		status = "okay";
+
+		module_led: led-0 {
+			label = "module_led";
+			gpios = <&gpio1 RK_PB0 GPIO_ACTIVE_HIGH>;
+			linux,default-trigger = "heartbeat";
+		};
+	};
+
+	vcc5v0_sys: vccsys {
+		compatible = "regulator-fixed";
+		regulator-name = "vcc5v0_sys";
+		regulator-always-on;
+		regulator-boot-on;
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+	};
+};
+
+&cpu0 {
+	cpu-supply = <&vdd_arm>;
+};
+
+&cpu1 {
+	cpu-supply = <&vdd_arm>;
+};
+
+&cpu2 {
+	cpu-supply = <&vdd_arm>;
+};
+
+&cpu3 {
+	cpu-supply = <&vdd_arm>;
+};
+
+&emmc {
+	bus-width = <8>;
+	cap-mmc-highspeed;
+	mmc-hs200-1_8v;
+	supports-emmc;
+	mmc-pwrseq = <&emmc_pwrseq>;
+	non-removable;
+	vmmc-supply = <&vcc_3v3>;
+	vqmmc-supply = <&vcc_emmc>;
+
+	status = "okay";
+};
+
+/* On-module TI DP83825I PHY but no connector, enable in carrierboard */
+&gmac {
+	snps,reset-gpio = <&gpio3 RK_PB0 GPIO_ACTIVE_LOW>;
+	snps,reset-active-low;
+	snps,reset-delays-us = <0 50000 50000>;
+	phy-supply = <&vcc_3v3>;
+	clock_in_out = "output";
+};
+
+&gpu {
+	status = "okay";
+};
+
+&i2c0 {
+	status = "okay";
+
+	rk809: pmic@20 {
+		compatible = "rockchip,rk809";
+		reg = <0x20>;
+		interrupt-parent = <&gpio0>;
+		interrupts = <7 IRQ_TYPE_LEVEL_LOW>;
+		pinctrl-0 = <&pmic_int>;
+		pinctrl-names = "default";
+		#clock-cells = <0>;
+		clock-output-names = "xin32k";
+		rockchip,system-power-controller;
+		wakeup-source;
+		status = "okay";
+
+		vcc1-supply = <&vcc5v0_sys>;
+		vcc2-supply = <&vcc5v0_sys>;
+		vcc3-supply = <&vcc5v0_sys>;
+		vcc4-supply = <&vcc5v0_sys>;
+		vcc5-supply = <&vcc_3v3>;
+		vcc6-supply = <&vcc_3v3>;
+		vcc7-supply = <&vcc_3v3>;
+		vcc9-supply = <&vcc5v0_sys>;
+
+		regulators {
+			vdd_log: DCDC_REG1 {
+				regulator-name = "vdd_log";
+				regulator-min-microvolt = <950000>;
+				regulator-max-microvolt = <1350000>;
+				regulator-ramp-delay = <6001>;
+				regulator-always-on;
+				regulator-boot-on;
+
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <950000>;
+				};
+			};
+
+			vdd_arm: DCDC_REG2 {
+				regulator-name = "vdd_arm";
+				regulator-min-microvolt = <950000>;
+				regulator-max-microvolt = <1350000>;
+				regulator-ramp-delay = <6001>;
+				regulator-always-on;
+				regulator-boot-on;
+
+				regulator-state-mem {
+					regulator-off-in-suspend;
+					regulator-suspend-microvolt = <950000>;
+				};
+			};
+
+			vcc_ddr: DCDC_REG3 {
+				regulator-name = "vcc_ddr";
+				regulator-always-on;
+				regulator-boot-on;
+
+				regulator-state-mem {
+					regulator-on-in-suspend;
+				};
+			};
+
+			vcc_3v0_1v8: vcc_emmc: DCDC_REG4 {
+				regulator-name = "vcc_3v0_1v8";
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <3000000>;
+				regulator-always-on;
+				regulator-boot-on;
+
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <3000000>;
+				};
+			};
+
+			vcc_3v3: DCDC_REG5 {
+				regulator-name = "vcc_3v3";
+				regulator-min-microvolt = <3300000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-always-on;
+				regulator-boot-on;
+
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <3300000>;
+				};
+			};
+
+			vcc_1v8: LDO_REG2 {
+				regulator-name = "vcc_1v8";
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-always-on;
+				regulator-boot-on;
+
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <1800000>;
+				};
+			};
+
+			vcc_1v0: LDO_REG3 {
+				regulator-name = "vcc_1v0";
+				regulator-min-microvolt = <1000000>;
+				regulator-max-microvolt = <1000000>;
+				regulator-always-on;
+				regulator-boot-on;
+
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <1000000>;
+				};
+			};
+
+			vccio_sd: LDO_REG5 {
+				regulator-name = "vccio_sd";
+				regulator-min-microvolt = <3000000>;
+				regulator-max-microvolt = <3000000>;
+				regulator-always-on;
+				regulator-boot-on;
+
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <3000000>;
+				};
+			};
+
+			vcc_lcd: LDO_REG7 {
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <1000000>;
+				regulator-max-microvolt = <1000000>;
+				regulator-name = "vcc_lcd";
+
+				regulator-state-mem {
+					regulator-off-in-suspend;
+					regulator-suspend-microvolt = <1000000>;
+				};
+			};
+
+			vcc_1v8_lcd: LDO_REG8 {
+				regulator-name = "vcc_1v8_lcd";
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-always-on;
+				regulator-boot-on;
+
+				regulator-state-mem {
+					regulator-on-in-suspend;
+					regulator-suspend-microvolt = <1800000>;
+				};
+			};
+
+			vcca_1v8: LDO_REG9 {
+				regulator-name = "vcca_1v8";
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-always-on;
+				regulator-boot-on;
+
+				regulator-state-mem {
+					regulator-off-in-suspend;
+					regulator-suspend-microvolt = <1800000>;
+				};
+			};
+		};
+	};
+};
+
+&i2c1 {
+	status = "okay";
+
+	/* SE05x is limited to Fast Mode */
+	clock-frequency = <400000>;
+
+	fan: fan@18 {
+		compatible = "ti,amc6821";
+		reg = <0x18>;
+		#cooling-cells = <2>;
+	};
+
+	rtc_twi: rtc@6f {
+		compatible = "isil,isl1208";
+		reg = <0x6f>;
+	};
+};
+
+&i2c3 {
+	status = "okay";
+};
+
+&i2s0_8ch {
+	rockchip,trcm-sync-tx-only;
+
+	pinctrl-0 = <&i2s0_8ch_sclktx &i2s0_8ch_lrcktx
+		     &i2s0_8ch_sdo0 &i2s0_8ch_sdi0>;
+};
+
+&io_domains {
+	vccio1-supply = <&vcc_3v3>;
+	vccio2-supply = <&vccio_sd>;
+	vccio3-supply = <&vcc_3v3>;
+	vccio4-supply = <&vcc_3v3>;
+	vccio5-supply = <&vcc_3v3>;
+	vccio6-supply = <&vcc_emmc>;
+	vccio-oscgpi-supply = <&vcc_3v3>;
+
+	status = "okay";
+};
+
+&pinctrl {
+	emmc {
+		emmc_reset: emmc-reset {
+			rockchip,pins = <1 RK_PB3 RK_FUNC_GPIO &pcfg_pull_none>;
+		};
+	};
+
+	leds {
+		module_led_pin: module-led-pin {
+			rockchip,pins = <1 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
+		};
+	};
+
+	pmic {
+		pmic_int: pmic_int {
+			rockchip,pins =
+				<0 RK_PA7 RK_FUNC_GPIO &pcfg_pull_up>;
+		};
+	};
+
+
+};
+
+&tsadc {
+	status = "okay";
+};
+
+&u2phy {
+	status = "okay";
+};
+
+&u2phy_host {
+	status = "okay";
+};
+
+/* Mule UCAN */
+&usb_host0_ehci {
+	status = "okay";
+};
+
+&usb_host0_ohci {
+	status = "okay";
+};
+
+&wdt {
+	status = "okay";
+};
-- 
2.37.3

Re: [PATCH 2/3] arm64: dts: rockchip: add PX30-µQ7 (Ringneck) SoM with Haikou baseboard

[Quentin Schulz]

Hi all,

On 9/22/22 12:12, Quentin Schulz wrote:
[...]
> diff --git a/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi b/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi
> new file mode 100644
> index 000000000000..74a7f1182c21
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi
> @@ -0,0 +1,347 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (c) 2022 Theobroma Systems Design und Consulting GmbH
> + */
> +
> +/dts-v1/;
> +#include "px30.dtsi"
> +
> +/ {
> +	aliases {
> +		mmc0 = &emmc;
> +		mmc1 = &sdio;
> +		rtc0 = &rtc_twi;
> +		rtc1 = &rk809;
> +	};
> +
> +	emmc_pwrseq: emmc-pwrseq {
> +		compatible = "mmc-pwrseq-emmc";
> +		pinctrl-0 = <&emmc_reset>;
> +		pinctrl-names = "default";
> +		reset-gpios = <&gpio1 RK_PB3 GPIO_ACTIVE_HIGH>;
> +	};
> +
> +	leds {
> +		compatible = "gpio-leds";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&module_led_pin>;
> +		status = "okay";
> +
> +		module_led: led-0 {
> +			label = "module_led";
> +			gpios = <&gpio1 RK_PB0 GPIO_ACTIVE_HIGH>;
> +			linux,default-trigger = "heartbeat";
> +		};
> +	};
> +
> +	vcc5v0_sys: vccsys {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc5v0_sys";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +	};
> +};
> +
> +&cpu0 {
> +	cpu-supply = <&vdd_arm>;
> +};
> +
> +&cpu1 {
> +	cpu-supply = <&vdd_arm>;
> +};
> +
> +&cpu2 {
> +	cpu-supply = <&vdd_arm>;
> +};
> +
> +&cpu3 {
> +	cpu-supply = <&vdd_arm>;
> +};
> +
> +&emmc {
> +	bus-width = <8>;
> +	cap-mmc-highspeed;
> +	mmc-hs200-1_8v;
> +	supports-emmc;
> +	mmc-pwrseq = <&emmc_pwrseq>;
> +	non-removable;
> +	vmmc-supply = <&vcc_3v3>;
> +	vqmmc-supply = <&vcc_emmc>;
> +
> +	status = "okay";
> +};
> +
> +/* On-module TI DP83825I PHY but no connector, enable in carrierboard */
> +&gmac {
> +	snps,reset-gpio = <&gpio3 RK_PB0 GPIO_ACTIVE_LOW>;
> +	snps,reset-active-low;
> +	snps,reset-delays-us = <0 50000 50000>;
> +	phy-supply = <&vcc_3v3>;
> +	clock_in_out = "output";
> +};
> +

I forgot:

&gpio2 {
         /*
          * The Qseven BIOS_DISABLE signal on the PX30-µQ7 keeps the 
on-module
          * eMMC powered-down initially (in fact it keeps the reset signal
          * asserted). BIOS_DISABLE_OVERRIDE pin allows to re-enable eMMC.
          */
         bios_disable_override {
                 gpios = <RK_PB5 GPIO_ACTIVE_LOW>;
                 output-high;
                 line-name = "bios_disable_override";
                 gpio-hog;
         };
};

Since I had this change in the bootloader that was enough during my 
tests, but better not rely on the bootloader whenever/wherever possible :)

I'll wait a bit for reviews before sending a v2 just for that.

Cheers,
Quentin

Re: [PATCH 2/3] arm64: dts: rockchip: add PX30-µQ7 (Ringneck) SoM with Haikou baseboard

[Krzysztof Kozlowski]

On 22/09/2022 12:12, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> 
> The PX30-uQ7 (Ringneck) SoM is a µQseven-compatible (40mmx70mm, MXM-230
> connector) system-on-module from Theobroma Systems[1], featuring the
> Rockchip PX30.
> 
> It provides the following feature set:
>  * up to 4GB DDR4
>  * up to 128GB on-module eMMC (with 8-bit 1.8V interface)
>  * SD card (on a baseboard) via edge connector
>  * Fast Ethernet with on-module TI DP83825I PHY
>  * MIPI-DSI/LVDS
>  * MIPI-CSI
>  * USB
>    - 1x USB 2.0 dual-role
>    - 3x USB 2.0 host
>  * on-module STM32 Cortex-M0 companion controller, implementing:
>    - low-power RTC functionality (ISL1208 emulation)
>    - fan controller (AMC6821 emulation)
>    - USB<->CAN bridge controller
>  * on-module Espressif ESP32 for Bluetooth + 2.4GHz WiFi
>  * on-module NXP SE05x Secure Element
> 
> [1] https://www.theobroma-systems.com/som-product/px30-%C2%B5q7/
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> ---
>  arch/arm64/boot/dts/rockchip/Makefile         |   1 +
>  .../dts/rockchip/px30-ringneck-haikou.dts     | 239 ++++++++++++
>  .../boot/dts/rockchip/px30-ringneck.dtsi      | 347 ++++++++++++++++++
>  3 files changed, 587 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts
>  create mode 100644 arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi
> 
> diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
> index ef79a672804a..1e3b9d7c1a8b 100644
> --- a/arch/arm64/boot/dts/rockchip/Makefile
> +++ b/arch/arm64/boot/dts/rockchip/Makefile
> @@ -3,6 +3,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += px30-evb.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += px30-engicam-px30-core-ctouch2.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += px30-engicam-px30-core-ctouch2-of10.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += px30-engicam-px30-core-edimm2.2.dtb
> +dtb-$(CONFIG_ARCH_ROCKCHIP) += px30-ringneck-haikou.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3308-evb.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3308-roc-cc.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3308-rock-pi-s.dtb
> diff --git a/arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts b/arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts
> new file mode 100644
> index 000000000000..6304d6f7baa3
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts
> @@ -0,0 +1,239 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (c) 2022 Theobroma Systems Design und Consulting GmbH
> + */
> +
> +/dts-v1/;
> +#include "px30-ringneck.dtsi"
> +#include <dt-bindings/input/input.h>
> +
> +/ {
> +	model = "Theobroma Systems PX30-µQ7 SoM on Haikou devkit";
> +	compatible = "tsd,px30-ringneck-haikou", "rockchip,px30";
> +
> +	aliases {
> +		mmc2 = &sdmmc;
> +	};
> +
> +	chosen {
> +		stdout-path = "serial0:115200n8";
> +	};
> +
> +	gpio_keys: haikou-keys {

By convention node name is gpio-keys

> +		compatible = "gpio-keys";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		pinctrl-0 = <&haikou_keys_pin>;
> +		pinctrl-names = "default";
> +
> +		BATLOWn {

Does not look like you tested the DTS against bindings. Please run `make
dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst
for instructions).

Missing generic node name, only lowercase characters.

> +			label = "BATLOW#";
> +			linux,code = <KEY_BATTERY>;
> +			gpios = <&gpio3 RK_PA7 GPIO_ACTIVE_LOW>;
> +		};
> +
> +		BIOS_DISABLEn {
> +			label = "BIOS_DISABLE#";
> +			linux,code = <SW_BOOT_ALT>;
> +			linux,input-type = <EV_SW>;
> +			gpios = <&gpio2 RK_PC2 GPIO_ACTIVE_HIGH>;
> +		};
> +
> +		LID_BTNn {
> +			label = "LID_BTN#";
> +			linux,code = <SW_LID>;
> +			linux,input-type = <EV_SW>;
> +			gpios = <&gpio3 RK_PA6 GPIO_ACTIVE_LOW>;
> +		};
> +
> +		SLP_BTNn {
> +			label = "SLP_BTN#";
> +			linux,code = <KEY_SLEEP>;
> +			gpios = <&gpio3 RK_PA5 GPIO_ACTIVE_LOW>;
> +		};
> +
> +		WAKEn {
> +			label = "WAKE#";
> +			linux,code = <KEY_WAKEUP>;
> +			gpios = <&gpio3 RK_PA4 GPIO_ACTIVE_LOW>;
> +			wakeup-source;
> +		};
> +	};
> +
> +	leds {
> +		pinctrl-0 = <&module_led_pin>, <&sd_card_led_pin>;
> +
> +		sd_card_led: led-1 {
> +			label = "sd_card_led";
> +			gpios = <&gpio3 RK_PB3 GPIO_ACTIVE_HIGH>;
> +			linux,default-trigger = "mmc2";
> +		};
> +	};
> +
> +	i2s0-sound {
> +		compatible = "simple-audio-card";
> +		simple-audio-card,format = "i2s";
> +		simple-audio-card,name = "Haikou,I2S-codec";
> +		simple-audio-card,mclk-fs = <512>;
> +
> +		simple-audio-card,codec {
> +			clocks = <&sgtl5000_clk>;
> +			sound-dai = <&sgtl5000>;
> +		};
> +
> +		simple-audio-card,cpu {
> +			bitclock-master;
> +			frame-master;
> +			sound-dai = <&i2s0_8ch>;
> +		};
> +	};
> +
> +	sgtl5000_clk: sgtl5000-oscillator {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency  = <24576000>;
> +	};
> +
> +	dc_12v: dc-12v {

Node names should be generic.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

So at least generic ("regulator") prefix or suffix.

> +		compatible = "regulator-fixed";
> +		regulator-name = "dc_12v";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <12000000>;
> +		regulator-max-microvolt = <12000000>;
> +	};
> +
> +	vcc3v3_baseboard: vcc3v3-baseboard {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc3v3_baseboard";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		vin-supply = <&dc_12v>;
> +	};
> +
> +	vcc5v0_baseboard: vcc5v0-baseboard {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc5v0_baseboard";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		vin-supply = <&dc_12v>;
> +	};
> +
> +	vdda_codec: vdda-codec {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vdda_codec";
> +		regulator-boot-on;
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		vin-supply = <&vcc5v0_baseboard>;
> +	};
> +
> +	vddd_codec: vddd-codec {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vddd_codec";
> +		regulator-boot-on;
> +		regulator-min-microvolt = <1600000>;
> +		regulator-max-microvolt = <1600000>;
> +		vin-supply = <&vcc5v0_baseboard>;
> +	};
> +};
> +
> +&i2c2 {
> +	status = "okay";
> +	clock-frequency = <400000>;
> +
> +	sgtl5000: codec@a {
> +		compatible = "fsl,sgtl5000";
> +		reg = <0x0a>;
> +		clocks = <&sgtl5000_clk>;
> +		#sound-dai-cells = <0>;
> +		VDDA-supply = <&vdda_codec>;
> +		VDDIO-supply = <&vcc3v3_baseboard>;
> +		VDDD-supply = <&vddd_codec>;
> +		status = "okay";

Okay is by default.

> +	};
> +};
> +
> +&i2c3 {
> +	status = "okay";
> +
> +	eeprom@50 {
> +		reg = <0x50>;
> +		compatible = "atmel,24c01";
> +		pagesize = <8>;
> +		size = <128>;
> +		vcc-supply = <&vcc3v3_baseboard>;
> +	};
> +};
> +
> +&i2s0_8ch {
> +	status = "okay";
> +};
> +
> +&gmac {
> +	status = "okay";
> +};
> +
> +&pinctrl {
> +	haikou {
> +		haikou_keys_pin: haikou-keys-pin {
> +			rockchip,pins =
> +			  /* WAKE# */
> +			  <3 RK_PA4 RK_FUNC_GPIO &pcfg_pull_up>,
> +			  /* SLP_BTN# */
> +			  <3 RK_PA5 RK_FUNC_GPIO &pcfg_pull_up>,
> +			  /* LID_BTN */
> +			  <3 RK_PA6 RK_FUNC_GPIO &pcfg_pull_up>,
> +			  /* BATLOW# */
> +			  <3 RK_PA7 RK_FUNC_GPIO &pcfg_pull_up>,
> +			  /* BIOS_DISABLE# */
> +			  <2 RK_PC2 RK_FUNC_GPIO &pcfg_pull_up>;
> +		};
> +	};
> +
> +	leds {
> +		sd_card_led_pin: sd-card-led-pin {
> +			rockchip,pins =
> +			  <3 RK_PB3 RK_FUNC_GPIO &pcfg_pull_none>;
> +		};
> +	};
> +};
> +
> +&pwm0 {
> +	status = "okay";
> +};
> +
> +&sdmmc {
> +	bus-width = <4>;
> +	cap-mmc-highspeed;
> +	cap-sd-highspeed;
> +	cd-gpios = <&gpio0 RK_PA3 GPIO_ACTIVE_LOW>;
> +	disable-wp;
> +	vmmc-supply = <&vcc3v3_baseboard>;
> +	status = "okay";
> +};
> +
> +&spi1 {
> +	status = "okay";
> +};
> +
> +&u2phy_otg {
> +	status = "okay";
> +};
> +
> +&uart0 {
> +	status = "okay";
> +};
> +
> +&uart5 {
> +	pinctrl-0 = <&uart5_xfer>;
> +	status = "okay";
> +};
> +
> +&usb20_otg {
> +	status = "okay";
> +};
> diff --git a/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi b/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi
> new file mode 100644
> index 000000000000..74a7f1182c21
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi
> @@ -0,0 +1,347 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (c) 2022 Theobroma Systems Design und Consulting GmbH
> + */
> +
> +/dts-v1/;
> +#include "px30.dtsi"
> +
> +/ {
> +	aliases {
> +		mmc0 = &emmc;
> +		mmc1 = &sdio;
> +		rtc0 = &rtc_twi;
> +		rtc1 = &rk809;
> +	};
> +
> +	emmc_pwrseq: emmc-pwrseq {
> +		compatible = "mmc-pwrseq-emmc";
> +		pinctrl-0 = <&emmc_reset>;
> +		pinctrl-names = "default";
> +		reset-gpios = <&gpio1 RK_PB3 GPIO_ACTIVE_HIGH>;
> +	};
> +
> +	leds {
> +		compatible = "gpio-leds";

This belongs to DTSI.

> +		pinctrl-names = "default";
> +		pinctrl-0 = <&module_led_pin>;
> +		status = "okay";

No need.

> +
> +		module_led: led-0 {

Why DTSI - which in logic comes first - starts with 1 and this is 0?\

> +			label = "module_led";
> +			gpios = <&gpio1 RK_PB0 GPIO_ACTIVE_HIGH>;
> +			linux,default-trigger = "heartbeat";
> +		};
> +	};
> +
> +	vcc5v0_sys: vccsys {

Node names should be generic.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc5v0_sys";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +	};
> +};
> +
> +&cpu0 {
> +	cpu-supply = <&vdd_arm>;
> +};
> +
> +&cpu1 {
> +	cpu-supply = <&vdd_arm>;
> +};
> +
> +&cpu2 {
> +	cpu-supply = <&vdd_arm>;
> +};
> +
> +&cpu3 {
> +	cpu-supply = <&vdd_arm>;
> +};
> +
> +&emmc {
> +	bus-width = <8>;
> +	cap-mmc-highspeed;
> +	mmc-hs200-1_8v;
> +	supports-emmc;
> +	mmc-pwrseq = <&emmc_pwrseq>;
> +	non-removable;
> +	vmmc-supply = <&vcc_3v3>;
> +	vqmmc-supply = <&vcc_emmc>;
> +
> +	status = "okay";
> +};
> +
> +/* On-module TI DP83825I PHY but no connector, enable in carrierboard */
> +&gmac {
> +	snps,reset-gpio = <&gpio3 RK_PB0 GPIO_ACTIVE_LOW>;
> +	snps,reset-active-low;
> +	snps,reset-delays-us = <0 50000 50000>;
> +	phy-supply = <&vcc_3v3>;
> +	clock_in_out = "output";
> +};
> +
> +&gpu {
> +	status = "okay";
> +};
> +
> +&i2c0 {
> +	status = "okay";
> +
> +	rk809: pmic@20 {
> +		compatible = "rockchip,rk809";
> +		reg = <0x20>;
> +		interrupt-parent = <&gpio0>;
> +		interrupts = <7 IRQ_TYPE_LEVEL_LOW>;
> +		pinctrl-0 = <&pmic_int>;
> +		pinctrl-names = "default";
> +		#clock-cells = <0>;
> +		clock-output-names = "xin32k";
> +		rockchip,system-power-controller;
> +		wakeup-source;
> +		status = "okay";

No need.

> +
> +		vcc1-supply = <&vcc5v0_sys>;
> +		vcc2-supply = <&vcc5v0_sys>;
> +		vcc3-supply = <&vcc5v0_sys>;
> +		vcc4-supply = <&vcc5v0_sys>;
> +		vcc5-supply = <&vcc_3v3>;
> +		vcc6-supply = <&vcc_3v3>;
> +		vcc7-supply = <&vcc_3v3>;
> +		vcc9-supply = <&vcc5v0_sys>;
> +

(...)

> +&pinctrl {
> +	emmc {
> +		emmc_reset: emmc-reset {
> +			rockchip,pins = <1 RK_PB3 RK_FUNC_GPIO &pcfg_pull_none>;
> +		};
> +	};
> +
> +	leds {
> +		module_led_pin: module-led-pin {
> +			rockchip,pins = <1 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
> +		};
> +	};
> +
> +	pmic {
> +		pmic_int: pmic_int {

No underscores in node names.

> +			rockchip,pins =
> +				<0 RK_PA7 RK_FUNC_GPIO &pcfg_pull_up>;
> +		};
> +	};
> +
> +

Skip unneeded blank lines.

> +};
> +
> +&tsadc {
> +	status = "okay";
> +};
> +
> +&u2phy {
> +	status = "okay";
> +};
> +
> +&u2phy_host {
> +	status = "okay";
> +};
> +
> +/* Mule UCAN */
> +&usb_host0_ehci {
> +	status = "okay";
> +};
> +
> +&usb_host0_ohci {
> +	status = "okay";
> +};
> +
> +&wdt {
> +	status = "okay";
> +};

Best regards,
Krzysztof

Re: [PATCH 2/3] arm64: dts: rockchip: add PX30-µQ7 (Ringneck) SoM with Haikou baseboard

[Quentin Schulz]

Hi Krzysztof,

On 9/22/22 14:45, Krzysztof Kozlowski wrote:
> On 22/09/2022 12:12, Quentin Schulz wrote:
[...]
>> diff --git a/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi b/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi
>> new file mode 100644
>> index 000000000000..74a7f1182c21
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi
>> @@ -0,0 +1,347 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Copyright (c) 2022 Theobroma Systems Design und Consulting GmbH
>> + */
>> +
>> +/dts-v1/;
>> +#include "px30.dtsi"
>> +
>> +/ {
>> +	aliases {
>> +		mmc0 = &emmc;
>> +		mmc1 = &sdio;
>> +		rtc0 = &rtc_twi;
>> +		rtc1 = &rk809;
>> +	};
>> +
>> +	emmc_pwrseq: emmc-pwrseq {
>> +		compatible = "mmc-pwrseq-emmc";
>> +		pinctrl-0 = <&emmc_reset>;
>> +		pinctrl-names = "default";
>> +		reset-gpios = <&gpio1 RK_PB3 GPIO_ACTIVE_HIGH>;
>> +	};
>> +
>> +	leds {
>> +		compatible = "gpio-leds";
> 
> This belongs to DTSI.
> 

It is in the DTSI :)

>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&module_led_pin>;
>> +		status = "okay";
> 
> No need.
> 
>> +
>> +		module_led: led-0 {
> 
> Why DTSI - which in logic comes first - starts with 1 and this is 0?\
> 

The first diff in this patch is for the DTS, this is now the DTSI. So it 
does have led-0 in the DTSI and led-1 in the DTS which I think is what 
you're requesting here? Is this a correct assumption?

Ack for all other review comments, thanks!

Cheers,
Quentin

Re: [PATCH 2/3] arm64: dts: rockchip: add PX30-µQ7 (Ringneck) SoM with Haikou baseboard

[Krzysztof Kozlowski]

On 23/09/2022 11:55, Quentin Schulz wrote:
>>> +
>>> +	leds {
>>> +		compatible = "gpio-leds";
>>
>> This belongs to DTSI.
>>
> 
> It is in the DTSI :)

Ah, right...


> 
>>> +		pinctrl-names = "default";
>>> +		pinctrl-0 = <&module_led_pin>;
>>> +		status = "okay";
>>
>> No need.
>>
>>> +
>>> +		module_led: led-0 {
>>
>> Why DTSI - which in logic comes first - starts with 1 and this is 0?\
>>
> 
> The first diff in this patch is for the DTS, this is now the DTSI. So it 
> does have led-0 in the DTSI and led-1 in the DTS which I think is what 
> you're requesting here? Is this a correct assumption?

Yep.


Best regards,
Krzysztof

[PATCH 3/3] dt-bindings: arm: rockchip: add Theobroma Systems PX30-µQ7 (Ringneck) with Haikou

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

Add entry for the Theobroma Systems PX30-µQ7 (Ringneck) with Haikou
devkit.

Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---
 Documentation/devicetree/bindings/arm/rockchip.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/rockchip.yaml b/Documentation/devicetree/bindings/arm/rockchip.yaml
index 7811ba64149c..f2aa5ddb76d2 100644
--- a/Documentation/devicetree/bindings/arm/rockchip.yaml
+++ b/Documentation/devicetree/bindings/arm/rockchip.yaml
@@ -658,6 +658,11 @@ properties:
           - const: rockchip,rv1108-evb
           - const: rockchip,rv1108
 
+      - description: Theobroma Systems PX30-µQ7 with Haikou baseboard
+        items:
+          - const: tsd,px30-ringneck-haikou
+          - const: rockchip,px30
+
       - description: Theobroma Systems RK3368-uQ7 with Haikou baseboard
         items:
           - const: tsd,rk3368-lion-haikou
-- 
2.37.3

Re: [PATCH 3/3] dt-bindings: arm: rockchip: add Theobroma Systems PX30-µQ7 (Ringneck) with Haikou

[Krzysztof Kozlowski]

On 22/09/2022 12:12, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> 
> Add entry for the Theobroma Systems PX30-µQ7 (Ringneck) with Haikou
> devkit.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> ---
>  Documentation/devicetree/bindings/arm/rockchip.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/rockchip.yaml b/Documentation/devicetree/bindings/arm/rockchip.yaml
> index 7811ba64149c..f2aa5ddb76d2 100644
> --- a/Documentation/devicetree/bindings/arm/rockchip.yaml
> +++ b/Documentation/devicetree/bindings/arm/rockchip.yaml
> @@ -658,6 +658,11 @@ properties:
>            - const: rockchip,rv1108-evb
>            - const: rockchip,rv1108
>  
> +      - description: Theobroma Systems PX30-µQ7 with Haikou baseboard
> +        items:
> +          - const: tsd,px30-ringneck-haikou
> +          - const: rockchip,px30

This should be somewhere around other px30 systems.

Best regards,
Krzysztof

Re: [PATCH 3/3] dt-bindings: arm: rockchip: add Theobroma Systems PX30-µQ7 (Ringneck) with Haikou

[Krzysztof Kozlowski]

On 22/09/2022 14:40, Krzysztof Kozlowski wrote:
> On 22/09/2022 12:12, Quentin Schulz wrote:
>> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>>
>> Add entry for the Theobroma Systems PX30-µQ7 (Ringneck) with Haikou
>> devkit.
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>> ---
>>  Documentation/devicetree/bindings/arm/rockchip.yaml | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/rockchip.yaml b/Documentation/devicetree/bindings/arm/rockchip.yaml
>> index 7811ba64149c..f2aa5ddb76d2 100644
>> --- a/Documentation/devicetree/bindings/arm/rockchip.yaml
>> +++ b/Documentation/devicetree/bindings/arm/rockchip.yaml
>> @@ -658,6 +658,11 @@ properties:
>>            - const: rockchip,rv1108-evb
>>            - const: rockchip,rv1108
>>  
>> +      - description: Theobroma Systems PX30-µQ7 with Haikou baseboard
>> +        items:
>> +          - const: tsd,px30-ringneck-haikou
>> +          - const: rockchip,px30
> 
> This should be somewhere around other px30 systems.

... or not :) if other ordering is used. Anyway - if you keep existing
order, then:

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof