[V2,1/5] dt-bindings: fsl: scu: add scu power key binding
diff mbox series

Message ID 1567519424-32271-1-git-send-email-Anson.Huang@nxp.com
State New
Headers show
Series
  • [V2,1/5] dt-bindings: fsl: scu: add scu power key binding
Related show

Commit Message

Anson Huang Sept. 3, 2019, 2:03 p.m. UTC
NXP i.MX8QXP is an ARMv8 SoC with a Cortex-M4 core inside as
system controller, the system controller is in charge of system
power, clock and power key event etc. management, Linux kernel
has to communicate with system controller via MU (message unit)
IPC to get power key event, add binding doc for i.MX system
controller power key driver.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
Changes since V1:
	- remove "wakeup-source" property, as it is NOT needed for SCU interrupt;
	- remove "status" in example.
---
 .../devicetree/bindings/arm/freescale/fsl,scu.txt          | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Oleksij Rempel Sept. 3, 2019, 6:32 a.m. UTC | #1
On 03.09.19 16:03, Anson Huang wrote:
> NXP i.MX8QXP is an ARMv8 SoC with a Cortex-M4 core inside as
> system controller, the system controller is in charge of system
> power, clock and power key event etc. management, Linux kernel
> has to communicate with system controller via MU (message unit)
> IPC to get power key event, add binding doc for i.MX system
> controller power key driver.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
> Changes since V1:
> 	- remove "wakeup-source" property, as it is NOT needed for SCU interrupt;
> 	- remove "status" in example.
> ---
>   .../devicetree/bindings/arm/freescale/fsl,scu.txt          | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> index c149fad..f93e2e4 100644
> --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> @@ -157,6 +157,15 @@ Required properties:
>   Optional properties:
>   - timeout-sec: contains the watchdog timeout in seconds.
>   
> +Power key bindings based on SCU Message Protocol
> +------------------------------------------------------------
> +
> +Required properties:
> +- compatible: should be:
> +              "fsl,imx8qxp-sc-pwrkey"
> +              followed by "fsl,imx-sc-pwrkey";
> +- linux,keycodes: See Documentation/devicetree/bindings/input/keys.txt

linux,keycodes is required parameter. So, this kay cab be anything. Why the compatible is 
called pwrkey? Probably it is better to call it "*-sc-key"

> +
>   Example (imx8qxp):
>   -------------
>   aliases {
> @@ -220,6 +229,11 @@ firmware {
>   			compatible = "fsl,imx8qxp-sc-rtc";
>   		};
>   
> +		scu_pwrkey: scu-pwrkey {
> +			compatible = "fsl,imx8qxp-sc-pwrkey", "fsl,imx-sc-pwrkey";
> +			linux,keycode = <KEY_POWER>;
> +		};
> +
>   		watchdog {
>   			compatible = "fsl,imx8qxp-sc-wdt", "fsl,imx-sc-wdt";
>   			timeout-sec = <60>;
> 

Kind regards,
Oleksij Rempel
Anson Huang Sept. 3, 2019, 6:37 a.m. UTC | #2
Hi, Oleksij

> On 03.09.19 16:03, Anson Huang wrote:
> > NXP i.MX8QXP is an ARMv8 SoC with a Cortex-M4 core inside as system
> > controller, the system controller is in charge of system power, clock
> > and power key event etc. management, Linux kernel has to communicate
> > with system controller via MU (message unit) IPC to get power key
> > event, add binding doc for i.MX system controller power key driver.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> > Changes since V1:
> > 	- remove "wakeup-source" property, as it is NOT needed for SCU
> interrupt;
> > 	- remove "status" in example.
> > ---
> >   .../devicetree/bindings/arm/freescale/fsl,scu.txt          | 14
> ++++++++++++++
> >   1 file changed, 14 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > index c149fad..f93e2e4 100644
> > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > @@ -157,6 +157,15 @@ Required properties:
> >   Optional properties:
> >   - timeout-sec: contains the watchdog timeout in seconds.
> >
> > +Power key bindings based on SCU Message Protocol
> > +------------------------------------------------------------
> > +
> > +Required properties:
> > +- compatible: should be:
> > +              "fsl,imx8qxp-sc-pwrkey"
> > +              followed by "fsl,imx-sc-pwrkey";
> > +- linux,keycodes: See
> > +Documentation/devicetree/bindings/input/keys.txt
> 
> linux,keycodes is required parameter. So, this kay cab be anything. Why the
> compatible is called pwrkey? Probably it is better to call it "*-sc-key"

This key is kind of special, it is ON/OFF button which is designed for power key
purpose, it has HW function such as long pressing it would shutdown the system power,
so we always use it as power key, NOT general key, does it make sense?

Thanks,
Anson.
Oleksij Rempel Sept. 3, 2019, 6:47 a.m. UTC | #3
On 03.09.19 08:37, Anson Huang wrote:
> Hi, Oleksij
> 
>> On 03.09.19 16:03, Anson Huang wrote:
>>> NXP i.MX8QXP is an ARMv8 SoC with a Cortex-M4 core inside as system
>>> controller, the system controller is in charge of system power, clock
>>> and power key event etc. management, Linux kernel has to communicate
>>> with system controller via MU (message unit) IPC to get power key
>>> event, add binding doc for i.MX system controller power key driver.
>>>
>>> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
>>> ---
>>> Changes since V1:
>>> 	- remove "wakeup-source" property, as it is NOT needed for SCU
>> interrupt;
>>> 	- remove "status" in example.
>>> ---
>>>    .../devicetree/bindings/arm/freescale/fsl,scu.txt          | 14
>> ++++++++++++++
>>>    1 file changed, 14 insertions(+)
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
>>> b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
>>> index c149fad..f93e2e4 100644
>>> --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
>>> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
>>> @@ -157,6 +157,15 @@ Required properties:
>>>    Optional properties:
>>>    - timeout-sec: contains the watchdog timeout in seconds.
>>>
>>> +Power key bindings based on SCU Message Protocol
>>> +------------------------------------------------------------
>>> +
>>> +Required properties:
>>> +- compatible: should be:
>>> +              "fsl,imx8qxp-sc-pwrkey"
>>> +              followed by "fsl,imx-sc-pwrkey";
>>> +- linux,keycodes: See
>>> +Documentation/devicetree/bindings/input/keys.txt
>>
>> linux,keycodes is required parameter. So, this kay cab be anything. Why the
>> compatible is called pwrkey? Probably it is better to call it "*-sc-key"
> 
> This key is kind of special, it is ON/OFF button which is designed for power key
> purpose, it has HW function such as long pressing it would shutdown the system power,
> so we always use it as power key, NOT general key, does it make sense?

I assume, OF devs will argue: DT is describing hardware not software.
On other hand, software will get notification about assertion/deassertion of this key. So, 
it is just normal key. If, for some reason, it will trigger world destruction, if it is 
pressed too long... probably no body cares.
You can provide fsl,imx-sc-key as additional compatible. In case linux will need some 
quirks, we still can fall back to more precise compatible "fsl,imx8qxp-sc-pwrkey".

Kind regards,
Oleksij Rempel
Anson Huang Sept. 3, 2019, 6:55 a.m. UTC | #4
Hi, Oleksij

> On 03.09.19 08:37, Anson Huang wrote:
> > Hi, Oleksij
> >
> >> On 03.09.19 16:03, Anson Huang wrote:
> >>> NXP i.MX8QXP is an ARMv8 SoC with a Cortex-M4 core inside as system
> >>> controller, the system controller is in charge of system power,
> >>> clock and power key event etc. management, Linux kernel has to
> >>> communicate with system controller via MU (message unit) IPC to get
> >>> power key event, add binding doc for i.MX system controller power key
> driver.
> >>>
> >>> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> >>> ---
> >>> Changes since V1:
> >>> 	- remove "wakeup-source" property, as it is NOT needed for SCU
> >> interrupt;
> >>> 	- remove "status" in example.
> >>> ---
> >>>    .../devicetree/bindings/arm/freescale/fsl,scu.txt          | 14
> >> ++++++++++++++
> >>>    1 file changed, 14 insertions(+)
> >>>
> >>> diff --git
> >>> a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> >>> b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> >>> index c149fad..f93e2e4 100644
> >>> --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> >>> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> >>> @@ -157,6 +157,15 @@ Required properties:
> >>>    Optional properties:
> >>>    - timeout-sec: contains the watchdog timeout in seconds.
> >>>
> >>> +Power key bindings based on SCU Message Protocol
> >>> +------------------------------------------------------------
> >>> +
> >>> +Required properties:
> >>> +- compatible: should be:
> >>> +              "fsl,imx8qxp-sc-pwrkey"
> >>> +              followed by "fsl,imx-sc-pwrkey";
> >>> +- linux,keycodes: See
> >>> +Documentation/devicetree/bindings/input/keys.txt
> >>
> >> linux,keycodes is required parameter. So, this kay cab be anything.
> >> Why the compatible is called pwrkey? Probably it is better to call it "*-sc-
> key"
> >
> > This key is kind of special, it is ON/OFF button which is designed for
> > power key purpose, it has HW function such as long pressing it would
> > shutdown the system power, so we always use it as power key, NOT
> general key, does it make sense?
> 
> I assume, OF devs will argue: DT is describing hardware not software.
> On other hand, software will get notification about assertion/deassertion of
> this key. So, it is just normal key. If, for some reason, it will trigger world
> destruction, if it is pressed too long... probably no body cares.
> You can provide fsl,imx-sc-key as additional compatible. In case linux will
> need some quirks, we still can fall back to more precise compatible
> "fsl,imx8qxp-sc-pwrkey".

I am OK with that, so I will use "fsl,imx-sc-key" as additional compatible and also
present in driver. 

Anson.
Pavel Machek Sept. 22, 2019, 4:13 p.m. UTC | #5
Hi!

> NXP i.MX8QXP is an ARMv8 SoC with a Cortex-M4 core inside as
> system controller, the system controller is in charge of system
> power, clock and power key event etc. management, Linux kernel
> has to communicate with system controller via MU (message unit)
> IPC to get power key event, add binding doc for i.MX system
> controller power key driver.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>

> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> @@ -157,6 +157,15 @@ Required properties:
>  Optional properties:
>  - timeout-sec: contains the watchdog timeout in seconds.
>  
> +Power key bindings based on SCU Message Protocol
> +------------------------------------------------------------
> +
> +Required properties:
> +- compatible: should be:
> +              "fsl,imx8qxp-sc-pwrkey"
> +              followed by "fsl,imx-sc-pwrkey";
> +- linux,keycodes: See Documentation/devicetree/bindings/input/keys.txt

Actually there's no reason for having "linux,keycodes" property when it is always
a power button.

Best regards,
									Pavel
Pavel Machek Sept. 22, 2019, 4:14 p.m. UTC | #6
On Tue 2019-09-03 10:03:40, Anson Huang wrote:
> NXP i.MX8QXP is an ARMv8 SoC with a Cortex-M4 core inside as
> system controller, the system controller is in charge of system
> power, clock and power key event etc. management, Linux kernel
> has to communicate with system controller via MU (message unit)
> IPC to get power key event, add binding doc for i.MX system
> controller power key driver.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
> Changes since V1:
> 	- remove "wakeup-source" property, as it is NOT needed for SCU interrupt;
> 	- remove "status" in example.
> ---
>  .../devicetree/bindings/arm/freescale/fsl,scu.txt          | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> index c149fad..f93e2e4 100644
> --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> @@ -157,6 +157,15 @@ Required properties:
>  Optional properties:
>  - timeout-sec: contains the watchdog timeout in seconds.
>  
> +Power key bindings based on SCU Message Protocol
> +------------------------------------------------------------
> +
> +Required properties:
> +- compatible: should be:
> +              "fsl,imx8qxp-sc-pwrkey"
> +              followed by "fsl,imx-sc-pwrkey";
> +- linux,keycodes: See Documentation/devicetree/bindings/input/keys.txt

Note you have keycode_s_ here, but keycode in the example and in the dts patch.

									Pavel
Anson Huang Sept. 23, 2019, 2:31 a.m. UTC | #7
Hi, Pavel

> Subject: Re: [PATCH V2 1/5] dt-bindings: fsl: scu: add scu power key binding
> 
> Hi!
> 
> > NXP i.MX8QXP is an ARMv8 SoC with a Cortex-M4 core inside as system
> > controller, the system controller is in charge of system power, clock
> > and power key event etc. management, Linux kernel has to communicate
> > with system controller via MU (message unit) IPC to get power key
> > event, add binding doc for i.MX system controller power key driver.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> 
> > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > @@ -157,6 +157,15 @@ Required properties:
> >  Optional properties:
> >  - timeout-sec: contains the watchdog timeout in seconds.
> >
> > +Power key bindings based on SCU Message Protocol
> > +------------------------------------------------------------
> > +
> > +Required properties:
> > +- compatible: should be:
> > +              "fsl,imx8qxp-sc-pwrkey"
> > +              followed by "fsl,imx-sc-pwrkey";
> > +- linux,keycodes: See
> > +Documentation/devicetree/bindings/input/keys.txt
> 
> Actually there's no reason for having "linux,keycodes" property when it is
> always a power button.
The latest version of patch already change the compatible name to *-sc-key which
is more general as key driver, so the "linux,keycodes" is needed now for driver
to define the key function.

Thanks,
Anson
Anson Huang Sept. 23, 2019, 2:34 a.m. UTC | #8
Hi, Pavel

> Subject: Re: [PATCH V2 1/5] dt-bindings: fsl: scu: add scu power key binding
> 
> On Tue 2019-09-03 10:03:40, Anson Huang wrote:
> > NXP i.MX8QXP is an ARMv8 SoC with a Cortex-M4 core inside as system
> > controller, the system controller is in charge of system power, clock
> > and power key event etc. management, Linux kernel has to communicate
> > with system controller via MU (message unit) IPC to get power key
> > event, add binding doc for i.MX system controller power key driver.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> > Changes since V1:
> > 	- remove "wakeup-source" property, as it is NOT needed for SCU
> interrupt;
> > 	- remove "status" in example.
> > ---
> >  .../devicetree/bindings/arm/freescale/fsl,scu.txt          | 14
> ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > index c149fad..f93e2e4 100644
> > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > @@ -157,6 +157,15 @@ Required properties:
> >  Optional properties:
> >  - timeout-sec: contains the watchdog timeout in seconds.
> >
> > +Power key bindings based on SCU Message Protocol
> > +------------------------------------------------------------
> > +
> > +Required properties:
> > +- compatible: should be:
> > +              "fsl,imx8qxp-sc-pwrkey"
> > +              followed by "fsl,imx-sc-pwrkey";
> > +- linux,keycodes: See
> > +Documentation/devicetree/bindings/input/keys.txt
> 
> Note you have keycode_s_ here, but keycode in the example and in the dts
> patch.

NOT quite understand your point, could you please provide more details?

Thanks,
Anson
Shawn Guo Oct. 6, 2019, 12:32 a.m. UTC | #9
On Mon, Sep 23, 2019 at 02:34:07AM +0000, Anson Huang wrote:
> Hi, Pavel
> 
> > Subject: Re: [PATCH V2 1/5] dt-bindings: fsl: scu: add scu power key binding
> > 
> > On Tue 2019-09-03 10:03:40, Anson Huang wrote:
> > > NXP i.MX8QXP is an ARMv8 SoC with a Cortex-M4 core inside as system
> > > controller, the system controller is in charge of system power, clock
> > > and power key event etc. management, Linux kernel has to communicate
> > > with system controller via MU (message unit) IPC to get power key
> > > event, add binding doc for i.MX system controller power key driver.
> > >
> > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > > ---
> > > Changes since V1:
> > > 	- remove "wakeup-source" property, as it is NOT needed for SCU
> > interrupt;
> > > 	- remove "status" in example.
> > > ---
> > >  .../devicetree/bindings/arm/freescale/fsl,scu.txt          | 14
> > ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > index c149fad..f93e2e4 100644
> > > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > @@ -157,6 +157,15 @@ Required properties:
> > >  Optional properties:
> > >  - timeout-sec: contains the watchdog timeout in seconds.
> > >
> > > +Power key bindings based on SCU Message Protocol
> > > +------------------------------------------------------------
> > > +
> > > +Required properties:
> > > +- compatible: should be:
> > > +              "fsl,imx8qxp-sc-pwrkey"
> > > +              followed by "fsl,imx-sc-pwrkey";
> > > +- linux,keycodes: See
> > > +Documentation/devicetree/bindings/input/keys.txt
> > 
> > Note you have keycode_s_ here, but keycode in the example and in the dts
> > patch.
> 
> NOT quite understand your point, could you please provide more details?

The property being used in driver, DTS, and DT example in the
bindings are all 'linux,keycode' rather than 'linux,keycodes'.

Shawn
Anson Huang Oct. 7, 2019, 1:29 a.m. UTC | #10
Hi, Shawn

> On Mon, Sep 23, 2019 at 02:34:07AM +0000, Anson Huang wrote:
> > Hi, Pavel
> >
> > > Subject: Re: [PATCH V2 1/5] dt-bindings: fsl: scu: add scu power key
> > > binding
> > >
> > > On Tue 2019-09-03 10:03:40, Anson Huang wrote:
> > > > NXP i.MX8QXP is an ARMv8 SoC with a Cortex-M4 core inside as
> > > > system controller, the system controller is in charge of system
> > > > power, clock and power key event etc. management, Linux kernel has
> > > > to communicate with system controller via MU (message unit) IPC to
> > > > get power key event, add binding doc for i.MX system controller power
> key driver.
> > > >
> > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > > > ---
> > > > Changes since V1:
> > > > 	- remove "wakeup-source" property, as it is NOT needed for SCU
> > > interrupt;
> > > > 	- remove "status" in example.
> > > > ---
> > > >  .../devicetree/bindings/arm/freescale/fsl,scu.txt          | 14
> > > ++++++++++++++
> > > >  1 file changed, 14 insertions(+)
> > > >
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > index c149fad..f93e2e4 100644
> > > > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > @@ -157,6 +157,15 @@ Required properties:
> > > >  Optional properties:
> > > >  - timeout-sec: contains the watchdog timeout in seconds.
> > > >
> > > > +Power key bindings based on SCU Message Protocol
> > > > +------------------------------------------------------------
> > > > +
> > > > +Required properties:
> > > > +- compatible: should be:
> > > > +              "fsl,imx8qxp-sc-pwrkey"
> > > > +              followed by "fsl,imx-sc-pwrkey";
> > > > +- linux,keycodes: See
> > > > +Documentation/devicetree/bindings/input/keys.txt
> > >
> > > Note you have keycode_s_ here, but keycode in the example and in the
> > > dts patch.
> >
> > NOT quite understand your point, could you please provide more details?
> 
> The property being used in driver, DTS, and DT example in the bindings are
> all 'linux,keycode' rather than 'linux,keycodes'.

I see now, since Documentation/devicetree/bindings/input/keys.txt uses "linux,keycodes",
so I will also use "linux,keycodes" for driver, DTS and DT example in V3.

Thanks,
Anson

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
index c149fad..f93e2e4 100644
--- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
+++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
@@ -157,6 +157,15 @@  Required properties:
 Optional properties:
 - timeout-sec: contains the watchdog timeout in seconds.
 
+Power key bindings based on SCU Message Protocol
+------------------------------------------------------------
+
+Required properties:
+- compatible: should be:
+              "fsl,imx8qxp-sc-pwrkey"
+              followed by "fsl,imx-sc-pwrkey";
+- linux,keycodes: See Documentation/devicetree/bindings/input/keys.txt
+
 Example (imx8qxp):
 -------------
 aliases {
@@ -220,6 +229,11 @@  firmware {
 			compatible = "fsl,imx8qxp-sc-rtc";
 		};
 
+		scu_pwrkey: scu-pwrkey {
+			compatible = "fsl,imx8qxp-sc-pwrkey", "fsl,imx-sc-pwrkey";
+			linux,keycode = <KEY_POWER>;
+		};
+
 		watchdog {
 			compatible = "fsl,imx8qxp-sc-wdt", "fsl,imx-sc-wdt";
 			timeout-sec = <60>;