by Sumit Garg

[permalink] [raw]

Subject: Re: [PATCH v3 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

On Thu, 21 Mar 2024 at 16:03, Stephan Gerhold <[email protected]> wrote:
>
> On Wed, Mar 20, 2024 at 12:10:32PM +0530, Sumit Garg wrote:
> > On Tue, 19 Mar 2024 at 19:43, Stephan Gerhold <[email protected]> wrote:
> > > On Mon, Mar 18, 2024 at 03:20:46PM +0530, Sumit Garg wrote:
> > > > On Fri, 15 Mar 2024 at 20:43, Stephan Gerhold <[email protected]> wrote:
> > > > > On Fri, Mar 15, 2024 at 11:37:07AM +0530, Sumit Garg wrote:
> > > > > > Add Schneider Electric HMIBSC board DTS. The HMIBSC board is an IIoT Edge
> > > > > > Box Core board based on the Qualcomm APQ8016E SoC.
> > > > > >
> > > > > > Support for Schneider Electric HMIBSC. Features:
> > > > > > - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306)
> > > > > > - 1GiB RAM
> > > > > > - 8GiB eMMC, SD slot
> > > > > > - WiFi and Bluetooth
> > > > > > - 2x Host, 1x Device USB port
> > > > > > - HDMI
> > > > > > - Discrete TPM2 chip over SPI
> > > > > > - USB ethernet adaptors (soldered)
> > > > > >
> > > > > > Co-developed-by: Jagdish Gediya <[email protected]>
> > > > > > Signed-off-by: Jagdish Gediya <[email protected]>
> > > > > > Reviewed-by: Caleb Connolly <[email protected]>
> > > > > > Signed-off-by: Sumit Garg <[email protected]>
> > > > > > ---
> > > > > > arch/arm64/boot/dts/qcom/Makefile | 1 +
> > > > > > .../dts/qcom/apq8016-schneider-hmibsc.dts | 510 ++++++++++++++++++
> > > > > > 2 files changed, 511 insertions(+)
> > > > > > create mode 100644 arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts
> > > > > >
> > > > > > diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> > > > > > index 39889d5f8e12..ad55e52e950b 100644
> > > > > > --- a/arch/arm64/boot/dts/qcom/Makefile
> > > > > > +++ b/arch/arm64/boot/dts/qcom/Makefile
> > > > > > @@ -5,6 +5,7 @@ apq8016-sbc-usb-host-dtbs := apq8016-sbc.dtb apq8016-sbc-usb-host.dtbo
> > > > > >
> > > > > > dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc-usb-host.dtb
> > > > > > dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc-d3-camera-mezzanine.dtb
> > > > > > +dtb-$(CONFIG_ARCH_QCOM) += apq8016-schneider-hmibsc.dtb
> > > > > > dtb-$(CONFIG_ARCH_QCOM) += apq8039-t2.dtb
> > > > > > dtb-$(CONFIG_ARCH_QCOM) += apq8094-sony-xperia-kitakami-karin_windy.dtb
> > > > > > dtb-$(CONFIG_ARCH_QCOM) += apq8096-db820c.dtb
> > > > > > diff --git a/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts
> > > > > > new file mode 100644
> > > > > > index 000000000000..9c79a31a04db
> > > > > > --- /dev/null
> > > > > > +++ b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts
> > > > > > @@ -0,0 +1,510 @@
> > > > > > [...]
> > > > > > +
> > > > > > +&pm8916_resin {
> > > > > > + interrupts = <0x0 0x8 1 IRQ_TYPE_EDGE_FALLING>;
> > > > > > + linux,code = <KEY_POWER>;
> > > > > > + status = "okay";
> > > > > > +};
> > > > >
> > > > > What is the goal of overriding the interrupt here? It looks like you are
> > > > > changing the interrupt type from IRQ_TYPE_EDGE_BOTH to FALLING. This
> > > > > sounds a bit like you want the driver to receive just button release
> > > > > events (or just press events, not sure about the polarity). I'm not sure
> > > > > if the driver will handle this correctly.
> > > >
> > > > The use-case here is to just act upon button release events and the
> > > > driver handles this appropriately. Final use-case of the reset button:
> > > >
> > > > - Short press and release leads to normal Linux reboot.
> > > > - Long press for more than 10 sec or so leads to a hard reset.
> > > >
> > > > With IRQ_TYPE_EDGE_BOTH, that's not achievable because just a simple
> > > > press leads to Linux reboot.
> > > >
> > >
> > > Thanks for explaining your use case. Is the DT really the right place to
> > > describe this? In the hardware, this is just a button that provides both
> > > press and release events. Linux typically forwards these events to user
> > > space, without interpreting them in any way. This means you likely have
> > > some user space component that listens to the events (e.g. systemd
> > > logind). Ideally that component should be reconfigured to trigger the
> > > reboot on release instead of press.
> >
> > I am not sure if that's really the case. I only see power key value to
> > be reported as follows:
> >
> > input_report_key(pwrkey->input, pwrkey->code, 1);
> > or
> > input_report_key(pwrkey->input, pwrkey->code, 0);
> >
> > It's not like a press event being a rising edge (0->1) or a release
> > event being a falling edge (1->0) reported. AFAICS, a reboot is issued
> > whenever the value of power key is reported as "1".
> >
>
> If you look inside the input_report_key() function you can see that the
> input subsystem internally tracks the key state. input_get_disposition()
> returns INPUT_IGNORE_EVENT if the key bit already has the same value.
> Only when the key changes its state, an event is sent to user space.
> This means that all events reported to user space are effectively
> rising/falling edges (an event with value "1" is a rising edge 0->1, an
> event with value "0" is a falling edge 1->0).
>
> The only reason why setting IRQ_TYPE_EDGE_FALLING works here is because
> of the workaround added in commit be8fc023ef64 ("Input: pm8941-pwrkey -
> simulate missed key press events") [1]. No event is reported when you
> start pressing the key. When you release the key, you get a key press
> event (rising edge) immediately followed by a key release (falling
> edge). But the workaround was added to handle potentially missed
> interrupts, not to inhibit reporting key press events.

I rather see it differently being actually allowing the current
use-case to support only the IRQ_TYPE_EDGE_FALLING. As per your
description above, a falling edge can only be an event for user-space
if we have:

input_report_key(pwrkey->input, pwrkey->code, 1);
input_report_key(pwrkey->input, pwrkey->code, 0);

rather than only

input_report_key(pwrkey->input, pwrkey->code, 0);

which won't trigger any event for user-space, right?

>
> In my opinion, if you want to perform an action on key release rather
> than key press then you should adjust the user space program to do so.
> From the point of view of the hardware DT (and even the kernel), the key
> press happens when you actually start pressing the key, and not later
> when you release it.

The reason why we are discussing it back and forth looks like due to
lack of clarity as to how HMIBSC board supports this button. The
common implementation as per db410c DTS is to have two buttons:

- One button is representing pwrkey(KEY_POWER) which is solely
consumed by the operating system to cleanly power off/restart the
db410c.
- Another button is representing pm8916_resin(KEY_VOLUMEDOWN) which
apart from normal volume down is also consumed by PMIC hardware to
perform a hard reset on a long press (if more than 10 sec.).

However, on HMIBSC board we only have a single power/reset button
which has to support the dual role of above two buttons on db410c:

Only one button as pm8916_resin(KEY_POWER), behaviour required:
- The rising edge (or button press) has to be only consumed by PMIC to
perform a hard reset of the HMIBSC board if pressed for more than 10
secs.
- The falling edge has to be consumed by the operating system to
cleanly reboot the HMIBSC board.

So what you are asking here is that the operating system has to be
notified of the rising edge of the button even if it has to just
ignore that without any action. Do you really think that would be
acceptable as a generic solution for systemd logind?

Also, why DT isn't the right place for correctly describing the
intended hardware behaviour? Or am I missing any DT policy which says
hardware has to be described the exact way it is rather than the
expected way for hardware to behave? BTW, If you want the hardware
behaviour to be properly commented then I can do that too.

-Sumit

>
> Thanks,
> Stephan
>
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=be8fc023ef64dcb11042aaa4bb0f29f7e0335d85

2024-03-22 12:04:32

by Caleb Connolly

[permalink] [raw]

Subject: Re: [PATCH v3 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

Hi both,

On 22/03/2024 06:29, Sumit Garg wrote:
> On Thu, 21 Mar 2024 at 16:03, Stephan Gerhold <[email protected]> wrote:
>>
>> On Wed, Mar 20, 2024 at 12:10:32PM +0530, Sumit Garg wrote:
>>> On Tue, 19 Mar 2024 at 19:43, Stephan Gerhold <[email protected]> wrote:
>>>> On Mon, Mar 18, 2024 at 03:20:46PM +0530, Sumit Garg wrote:
>>>>> On Fri, 15 Mar 2024 at 20:43, Stephan Gerhold <[email protected]> wrote:
>>>>>> On Fri, Mar 15, 2024 at 11:37:07AM +0530, Sumit Garg wrote:
>>>>>>> Add Schneider Electric HMIBSC board DTS. The HMIBSC board is an IIoT Edge
>>>>>>> Box Core board based on the Qualcomm APQ8016E SoC.
>>>>>>>
>>>>>>> Support for Schneider Electric HMIBSC. Features:
>>>>>>> - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306)
>>>>>>> - 1GiB RAM
>>>>>>> - 8GiB eMMC, SD slot
>>>>>>> - WiFi and Bluetooth
>>>>>>> - 2x Host, 1x Device USB port
>>>>>>> - HDMI
>>>>>>> - Discrete TPM2 chip over SPI
>>>>>>> - USB ethernet adaptors (soldered)
>>>>>>>
>>>>>>> Co-developed-by: Jagdish Gediya <[email protected]>
>>>>>>> Signed-off-by: Jagdish Gediya <[email protected]>
>>>>>>> Reviewed-by: Caleb Connolly <[email protected]>
>>>>>>> Signed-off-by: Sumit Garg <[email protected]>
>>>>>>> ---
>>>>>>> arch/arm64/boot/dts/qcom/Makefile | 1 +
>>>>>>> .../dts/qcom/apq8016-schneider-hmibsc.dts | 510 ++++++++++++++++++
>>>>>>> 2 files changed, 511 insertions(+)
>>>>>>> create mode 100644 arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts
>>>>>>>
>>>>>>> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
>>>>>>> index 39889d5f8e12..ad55e52e950b 100644
>>>>>>> --- a/arch/arm64/boot/dts/qcom/Makefile
>>>>>>> +++ b/arch/arm64/boot/dts/qcom/Makefile
>>>>>>> @@ -5,6 +5,7 @@ apq8016-sbc-usb-host-dtbs := apq8016-sbc.dtb apq8016-sbc-usb-host.dtbo
>>>>>>>
>>>>>>> dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc-usb-host.dtb
>>>>>>> dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc-d3-camera-mezzanine.dtb
>>>>>>> +dtb-$(CONFIG_ARCH_QCOM) += apq8016-schneider-hmibsc.dtb
>>>>>>> dtb-$(CONFIG_ARCH_QCOM) += apq8039-t2.dtb
>>>>>>> dtb-$(CONFIG_ARCH_QCOM) += apq8094-sony-xperia-kitakami-karin_windy.dtb
>>>>>>> dtb-$(CONFIG_ARCH_QCOM) += apq8096-db820c.dtb
>>>>>>> diff --git a/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..9c79a31a04db
>>>>>>> --- /dev/null
>>>>>>> +++ b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts
>>>>>>> @@ -0,0 +1,510 @@
>>>>>>> [...]
>>>>>>> +
>>>>>>> +&pm8916_resin {
>>>>>>> + interrupts = <0x0 0x8 1 IRQ_TYPE_EDGE_FALLING>;
>>>>>>> + linux,code = <KEY_POWER>;
>>>>>>> + status = "okay";
>>>>>>> +};
>>>>>>
>>>>>> What is the goal of overriding the interrupt here? It looks like you are
>>>>>> changing the interrupt type from IRQ_TYPE_EDGE_BOTH to FALLING. This
>>>>>> sounds a bit like you want the driver to receive just button release
>>>>>> events (or just press events, not sure about the polarity). I'm not sure
>>>>>> if the driver will handle this correctly.
>>>>>
>>>>> The use-case here is to just act upon button release events and the
>>>>> driver handles this appropriately. Final use-case of the reset button:
>>>>>
>>>>> - Short press and release leads to normal Linux reboot.
>>>>> - Long press for more than 10 sec or so leads to a hard reset.
>>>>>
>>>>> With IRQ_TYPE_EDGE_BOTH, that's not achievable because just a simple
>>>>> press leads to Linux reboot.
>>>>>
>>>>
>>>> Thanks for explaining your use case. Is the DT really the right place to
>>>> describe this? In the hardware, this is just a button that provides both
>>>> press and release events. Linux typically forwards these events to user
>>>> space, without interpreting them in any way. This means you likely have
>>>> some user space component that listens to the events (e.g. systemd
>>>> logind). Ideally that component should be reconfigured to trigger the
>>>> reboot on release instead of press.
>>>
>>> I am not sure if that's really the case. I only see power key value to
>>> be reported as follows:
>>>
>>> input_report_key(pwrkey->input, pwrkey->code, 1);
>>> or
>>> input_report_key(pwrkey->input, pwrkey->code, 0);
>>>
>>> It's not like a press event being a rising edge (0->1) or a release
>>> event being a falling edge (1->0) reported. AFAICS, a reboot is issued
>>> whenever the value of power key is reported as "1".
>>>
>>
>> If you look inside the input_report_key() function you can see that the
>> input subsystem internally tracks the key state. input_get_disposition()
>> returns INPUT_IGNORE_EVENT if the key bit already has the same value.
>> Only when the key changes its state, an event is sent to user space.
>> This means that all events reported to user space are effectively
>> rising/falling edges (an event with value "1" is a rising edge 0->1, an
>> event with value "0" is a falling edge 1->0).
>>
>> The only reason why setting IRQ_TYPE_EDGE_FALLING works here is because
>> of the workaround added in commit be8fc023ef64 ("Input: pm8941-pwrkey -
>> simulate missed key press events") [1]. No event is reported when you
>> start pressing the key. When you release the key, you get a key press
>> event (rising edge) immediately followed by a key release (falling
>> edge). But the workaround was added to handle potentially missed
>> interrupts, not to inhibit reporting key press events.
>
> I rather see it differently being actually allowing the current
> use-case to support only the IRQ_TYPE_EDGE_FALLING. As per your
> description above, a falling edge can only be an event for user-space
> if we have:
>
> input_report_key(pwrkey->input, pwrkey->code, 1);
> input_report_key(pwrkey->input, pwrkey->code, 0);
>
> rather than only
>
> input_report_key(pwrkey->input, pwrkey->code, 0);
>
> which won't trigger any event for user-space, right?
>
>>
>> In my opinion, if you want to perform an action on key release rather
>> than key press then you should adjust the user space program to do so.
>> From the point of view of the hardware DT (and even the kernel), the key
>> press happens when you actually start pressing the key, and not later
>> when you release it.
>
> The reason why we are discussing it back and forth looks like due to
> lack of clarity as to how HMIBSC board supports this button. The
> common implementation as per db410c DTS is to have two buttons:
>
> - One button is representing pwrkey(KEY_POWER) which is solely
> consumed by the operating system to cleanly power off/restart the
> db410c.
> - Another button is representing pm8916_resin(KEY_VOLUMEDOWN) which
> apart from normal volume down is also consumed by PMIC hardware to
> perform a hard reset on a long press (if more than 10 sec.).
>
> However, on HMIBSC board we only have a single power/reset button
> which has to support the dual role of above two buttons on db410c:
>
> Only one button as pm8916_resin(KEY_POWER), behaviour required:
> - The rising edge (or button press) has to be only consumed by PMIC to
> perform a hard reset of the HMIBSC board if pressed for more than 10
> secs.
> - The falling edge has to be consumed by the operating system to
> cleanly reboot the HMIBSC board.
>
> So what you are asking here is that the operating system has to be
> notified of the rising edge of the button even if it has to just
> ignore that without any action. Do you really think that would be
> acceptable as a generic solution for systemd logind?

Yes, this does seem to be a bug in systemd-logind. It appears to have
subtly different behaviour depending on if you have
HandlePowerKeyLongPress configured or not. If it is set to any action
then systemd will wait until you release the key to decide whether to
perform HandlePowerKey or HandlePowerKeyLongPress. However if there is
no LongPress action configured then it will immediately perform the
HandlePowerKey action on key down. [2]

As a workaround, try adding HandlePowerKeyLongPress="lock" to
logind.conf and see if that gives you the desired behaviour on short press.
>
> Also, why DT isn't the right place for correctly describing the
> intended hardware behaviour? Or am I missing any DT policy which says
> hardware has to be described the exact way it is rather than the
> expected way for hardware to behave? BTW, If you want the hardware
> behaviour to be properly commented then I can do that too.

DT is for describing the hardware, not for dictating its behaviour. As
Stephen pointed out, your workaround here only works due to a workaround
in the kernel for a bug... DT isn't just consumed by Linux, and some
other OS might not behave correctly as a result of this hack.

[1]:
https://github.com/systemd/systemd/blob/main/src/login/logind-button.c#L215
>
> -Sumit
>
>>
>> Thanks,
>> Stephan
>>
>> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=be8fc023ef64dcb11042aaa4bb0f29f7e0335d85

--
// Caleb (they/them)