[1/4] dt-bindings: firmware: secvio: Add device tree bindings

Commit Message

Vabhav Sharma May 9, 2024, 12:45 a.m. UTC

Document the secvio device tree bindings.

The tampers are security feature available on i.MX products and
managed by SNVS block.The tamper goal is to detect the variation
of hardware or physical parameters, which can indicate an attack.

The SNVS, which provides secure non-volatile storage, allows to
detect some hardware attacks against the SoC.They are connected
to the security-violation ports, which send an alert when an
out-of-range value is detected.

The "imx-secvio-sc" module is designed to report security violations
and tamper triggering via SCU firmware to the user.

Add the imx-scu secvio sub node and secvio sub node description.

Signed-off-by: Franck LENORMAND <[e-mail]>
Signed-off-by: Vabhav Sharma <[e-mail]>
---
 .../bindings/arm/freescale/fsl,scu-secvio.yaml     | 35 ++++++++++++++++++++++
 .../devicetree/bindings/firmware/fsl,scu.yaml      | 10 +++++++
 2 files changed, 45 insertions(+)

Comments

Frank Li May 9, 2024, 3:06 a.m. UTC | #1

On Thu, May 09, 2024 at 02:45:32AM +0200, Vabhav Sharma wrote:
> Document the secvio device tree bindings.

reduntant sentence. 
> 
> The tampers are security feature available on i.MX products and
> managed by SNVS block.The tamper goal is to detect the variation
                        ^^ space here

> of hardware or physical parameters, which can indicate an attack.
> 
> The SNVS, which provides secure non-volatile storage, allows to
> detect some hardware attacks against the SoC.They are connected
                                               ^^ space here 
> to the security-violation ports, which send an alert when an
> out-of-range value is detected.
> 
> The "imx-secvio-sc" module is designed to report security violations
> and tamper triggering via SCU firmware to the user.
> 
> Add the imx-scu secvio sub node and secvio sub node description.
> 
> Signed-off-by: Franck LENORMAND <[e-mail]>
> Signed-off-by: Vabhav Sharma <[e-mail]>
> ---
>  .../bindings/arm/freescale/fsl,scu-secvio.yaml     | 35 ++++++++++++++++++++++
>  .../devicetree/bindings/firmware/fsl,scu.yaml      | 10 +++++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,scu-secvio.yaml b/Documentation/devicetree/bindings/arm/freescale/fsl,scu-secvio.yaml
> new file mode 100644
> index 000000000000..30dc1e21f903
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu-secvio.yaml
> @@ -0,0 +1,35 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/freescale/fsl,scu-secvio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP i.MX Security Violation driver

Violation detect driver

> +
> +maintainers:
> +  - Franck LENORMAND <[e-mail]>
> +
> +description: |

Needn't "|"

> +  Receive security violation from the SNVS via the SCU firmware. Allow to
> +  register notifier for additional processing
> +
> +properties:
> +  compatible:
> +    enum:
> +      - fsl,imx-sc-secvio
> +
> +  nvmem:
> +    maxItems: 1
> +

any interrupt defined? how do you notify such violation event?

> +required:
> +  - compatible
> +  - nvmem
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    secvio {
> +        compatible = "fsl,imx-sc-secvio";
> +        nvmem = <&ocotp>;
> +    };
> diff --git a/Documentation/devicetree/bindings/firmware/fsl,scu.yaml b/Documentation/devicetree/bindings/firmware/fsl,scu.yaml
> index 557e524786c2..b40e127fdc88 100644
> --- a/Documentation/devicetree/bindings/firmware/fsl,scu.yaml
> +++ b/Documentation/devicetree/bindings/firmware/fsl,scu.yaml
> @@ -129,6 +129,11 @@ properties:
>        RTC controller provided by the SCU
>      $ref: /schemas/rtc/fsl,scu-rtc.yaml
>  
> +  secvio:
> +    description:
> +      Receive security violation from the SNVS via the SCU firmware
> +    $ref: /schemas/arm/freescale/fsl,scu-secvio.yaml
> +
>    thermal-sensor:
>      description:
>        Thermal sensor provided by the SCU
> @@ -197,6 +202,11 @@ examples:
>                  compatible = "fsl,imx8qxp-sc-rtc";
>              };
>  
> +            secvio {
> +                compatible = "fsl,imx-sc-secvio";
> +                nvmem = <&ocotp>;
> +            };
> +
>              keys {
>                  compatible = "fsl,imx8qxp-sc-key", "fsl,imx-sc-key";
>                  linux,keycodes = <KEY_POWER>;
> 
> -- 
> 2.25.1
>

Krzysztof Kozlowski May 9, 2024, 5:53 a.m. UTC | #2

On 09/05/2024 02:45, Vabhav Sharma wrote:
> Document the secvio device tree bindings.
> 
> The tampers are security feature available on i.MX products and
> managed by SNVS block.The tamper goal is to detect the variation
> of hardware or physical parameters, which can indicate an attack.
> 
> The SNVS, which provides secure non-volatile storage, allows to
> detect some hardware attacks against the SoC.They are connected
> to the security-violation ports, which send an alert when an
> out-of-range value is detected.
> 
> The "imx-secvio-sc" module is designed to report security violations
> and tamper triggering via SCU firmware to the user.
> 
> Add the imx-scu secvio sub node and secvio sub node description.
> 
> Signed-off-by: Franck LENORMAND <[e-mail]>
> Signed-off-by: Vabhav Sharma <[e-mail]>
> ---

That's not v1, right? What changed? Why do we have to guess this?

This is thoroughly documented in kernel process so read the
documentation before posting.


>  .../bindings/arm/freescale/fsl,scu-secvio.yaml     | 35 ++++++++++++++++++++++
>  .../devicetree/bindings/firmware/fsl,scu.yaml      | 10 +++++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,scu-secvio.yaml b/Documentation/devicetree/bindings/arm/freescale/fsl,scu-secvio.yaml
> new file mode 100644
> index 000000000000..30dc1e21f903
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu-secvio.yaml
> @@ -0,0 +1,35 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/freescale/fsl,scu-secvio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP i.MX Security Violation driver

Bindings are for hardware, not drivers. Describe hardware.

> +
> +maintainers:
> +  - Franck LENORMAND <[e-mail]>
> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> +  Receive security violation from the SNVS via the SCU firmware. Allow to
> +  register notifier for additional processing

Notifier? That's a Linux thing, how does it relate to the hardware?

> +
> +properties:
> +  compatible:
> +    enum:
> +      - fsl,imx-sc-secvio

Missing SoC compatibles.

So no, that's just abuse of DT to instantiate driver.

NAK. Drop the binding.

Best regards,
Krzysztof

Krzysztof Kozlowski May 9, 2024, 5:54 a.m. UTC | #3

On 09/05/2024 05:06, Frank Li wrote:
> On Thu, May 09, 2024 at 02:45:32AM +0200, Vabhav Sharma wrote:
>> Document the secvio device tree bindings.
> 
> reduntant sentence. 
>>
>> The tampers are security feature available on i.MX products and
>> managed by SNVS block.The tamper goal is to detect the variation
>                         ^^ space here
> 
>> of hardware or physical parameters, which can indicate an attack.
>>
>> The SNVS, which provides secure non-volatile storage, allows to
>> detect some hardware attacks against the SoC.They are connected
>                                                ^^ space here 
>> to the security-violation ports, which send an alert when an
>> out-of-range value is detected.
>>
>> The "imx-secvio-sc" module is designed to report security violations
>> and tamper triggering via SCU firmware to the user.
>>
>> Add the imx-scu secvio sub node and secvio sub node description.
>>
>> Signed-off-by: Franck LENORMAND <[e-mail]>
>> Signed-off-by: Vabhav Sharma <[e-mail]>
>> ---
>>  .../bindings/arm/freescale/fsl,scu-secvio.yaml     | 35 ++++++++++++++++++++++
>>  .../devicetree/bindings/firmware/fsl,scu.yaml      | 10 +++++++
>>  2 files changed, 45 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,scu-secvio.yaml b/Documentation/devicetree/bindings/arm/freescale/fsl,scu-secvio.yaml
>> new file mode 100644
>> index 000000000000..30dc1e21f903
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu-secvio.yaml
>> @@ -0,0 +1,35 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/arm/freescale/fsl,scu-secvio.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: NXP i.MX Security Violation driver
> 
> Violation detect driver

Bindings are not for drivers.

Best regards,
Krzysztof

Vabhav Sharma June 7, 2024, 4:58 a.m. UTC | #4

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: Thursday, May 9, 2024 11:24 AM
> To: Vabhav Sharma <[e-mail]>; Rob Herring
> <[e-mail]>; Krzysztof Kozlowski <[e-mail]>; Conor Dooley
> <[e-mail]>; Franck Lenormand <[e-mail]>;
> Aisheng Dong <[e-mail]>; Shawn Guo
> <[e-mail]>; Sascha Hauer <[e-mail]>;
> Pengutronix Kernel Team <[e-mail]>; Fabio Estevam
> <[e-mail]>; Peng Fan <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Varun Sethi
> <[e-mail]>; Silvano Di Ninno <[e-mail]>; Pankaj
> Gupta <[e-mail]>; Frank Li <[e-mail]>; Daniel Baluta
> <[e-mail]>
> Subject: [EXT] Re: [PATCH 1/4] dt-bindings: firmware: secvio: Add device tree
> bindings
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
> 
> 
> On 09/05/2024 02:45, Vabhav Sharma wrote:
> > Document the secvio device tree bindings.
> >
> > The tampers are security feature available on i.MX products and
> > managed by SNVS block.The tamper goal is to detect the variation of
> > hardware or physical parameters, which can indicate an attack.
> >
> > The SNVS, which provides secure non-volatile storage, allows to detect
> > some hardware attacks against the SoC.They are connected to the
> > security-violation ports, which send an alert when an out-of-range
> > value is detected.
> >
> > The "imx-secvio-sc" module is designed to report security violations
> > and tamper triggering via SCU firmware to the user.
> >
> > Add the imx-scu secvio sub node and secvio sub node description.
> >
> > Signed-off-by: Franck LENORMAND <[e-mail]>
> > Signed-off-by: Vabhav Sharma <[e-mail]>
> > ---
> 
> That's not v1, right? What changed? Why do we have to guess this?
Yes, correct this is v2, I will provide changelog details in v3 for v2 and v1
> 
> This is thoroughly documented in kernel process so read the documentation
> before posting.
> 
> 
> >  .../bindings/arm/freescale/fsl,scu-secvio.yaml     | 35
> ++++++++++++++++++++++
> >  .../devicetree/bindings/firmware/fsl,scu.yaml      | 10 +++++++
> >  2 files changed, 45 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu-secvio.yaml
> > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu-secvio.yaml
> > new file mode 100644
> > index 000000000000..30dc1e21f903
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu-secvio.y
> > +++ aml
> > @@ -0,0 +1,35 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> > +cetree.org%2Fschemas%2Farm%2Ffreescale%2Ffsl%2Cscu-
> secvio.yaml%23&dat
> >
> +a=05%7C02%7Cvabhav.sharma%40nxp.com%7Cdea3ecc999444d8c3f7108dc
> 6fec67e
> >
> +b%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63850830837113
> 8503%7CU
> >
> +nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTi
> I6Ik1h
> >
> +aWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=Nl9F3A9%2BTraZboxg3KXT
> 35pIPAyYZ
> > +51URq1wff7XCmo%3D&reserved=0
> > +$schema:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> > +cetree.org%2Fmeta-
> schemas%2Fcore.yaml%23&data=05%7C02%7Cvabhav.sharma
> >
> +%40nxp.com%7Cdea3ecc999444d8c3f7108dc6fec67eb%7C686ea1d3bc2b4c
> 6fa92cd
> >
> +99c5c301635%7C0%7C0%7C638508308371152796%7CUnknown%7CTWFpb
> GZsb3d8eyJW
> >
> +IjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C
> 0%7C
> >
> +%7C%7C&sdata=dwOG7uVGtO8rccW7vcRwvc2%2B9gyDroIsWnFlXyFxbOM%
> 3D&reserve
> > +d=0
> > +
> > +title: NXP i.MX Security Violation driver
> 
> Bindings are for hardware, not drivers. Describe hardware.
Yes, I will use "security violation detection hardware exported through SCU firmware"
> 
> > +
> > +maintainers:
> > +  - Franck LENORMAND <[e-mail]>
> > +
> > +description: |
> 
> Do not need '|' unless you need to preserve formatting.
Ok
> 
> > +  Receive security violation from the SNVS via the SCU firmware.
> > + Allow to  register notifier for additional processing
> 
> Notifier? That's a Linux thing, how does it relate to the hardware?
Violation detected by HW will call driver API to signify the violation.
> 
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - fsl,imx-sc-secvio
> 
> Missing SoC compatibles.
Ok, I will use fsl,imx8dxl-sc-secvio
> 
> So no, that's just abuse of DT to instantiate driver.
> 
> NAK. Drop the binding.
I will detail the dt binding to describe the real hardware
> 
> Best regards,
> Krzysztof

Vabhav Sharma June 7, 2024, 5 a.m. UTC | #5

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: Thursday, May 9, 2024 11:24 AM
> To: Frank Li <[e-mail]>; Vabhav Sharma <[e-mail]>
> Cc: Rob Herring <[e-mail]>; Krzysztof Kozlowski
> <[e-mail]>; Conor Dooley <[e-mail]>; Franck
> Lenormand <[e-mail]>; Aisheng Dong
> <[e-mail]>; Shawn Guo <[e-mail]>; Sascha
> Hauer <[e-mail]>; Pengutronix Kernel Team
> <[e-mail]>; Fabio Estevam <[e-mail]>; Peng Fan
> <[e-mail]>; [e-mail]; linux-
> [e-mail]; [e-mail]; linux-arm-
> [e-mail]; Varun Sethi <[e-mail]>; Silvano Di Ninno
> <[e-mail]>; Pankaj Gupta <[e-mail]>; Daniel
> Baluta <[e-mail]>
> Subject: [EXT] Re: [PATCH 1/4] dt-bindings: firmware: secvio: Add device tree
> bindings
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
> 
> 
> On 09/05/2024 05:06, Frank Li wrote:
> > On Thu, May 09, 2024 at 02:45:32AM +0200, Vabhav Sharma wrote:
> >> Document the secvio device tree bindings.
> >
> > reduntant sentence.
> >>
> >> The tampers are security feature available on i.MX products and
> >> managed by SNVS block.The tamper goal is to detect the variation
> >                         ^^ space here
> >
> >> of hardware or physical parameters, which can indicate an attack.
> >>
> >> The SNVS, which provides secure non-volatile storage, allows to
> >> detect some hardware attacks against the SoC.They are connected
> >                                                ^^ space here
> >> to the security-violation ports, which send an alert when an
> >> out-of-range value is detected.
> >>
> >> The "imx-secvio-sc" module is designed to report security violations
> >> and tamper triggering via SCU firmware to the user.
> >>
> >> Add the imx-scu secvio sub node and secvio sub node description.
> >>
> >> Signed-off-by: Franck LENORMAND <[e-mail]>
> >> Signed-off-by: Vabhav Sharma <[e-mail]>
> >> ---
> >>  .../bindings/arm/freescale/fsl,scu-secvio.yaml     | 35
> ++++++++++++++++++++++
> >>  .../devicetree/bindings/firmware/fsl,scu.yaml      | 10 +++++++
> >>  2 files changed, 45 insertions(+)
> >>
> >> diff --git
> >> a/Documentation/devicetree/bindings/arm/freescale/fsl,scu-secvio.yaml
> >> b/Documentation/devicetree/bindings/arm/freescale/fsl,scu-secvio.yaml
> >> new file mode 100644
> >> index 000000000000..30dc1e21f903
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu-secvio.
> >> +++ yaml
> >> @@ -0,0 +1,35 @@
> >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> >> +---
> >> +$id:
> >> +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdev
> >> +icetree.org%2Fschemas%2Farm%2Ffreescale%2Ffsl%2Cscu-
> secvio.yaml%23&d
> >>
> +ata=05%7C02%7Cvabhav.sharma%40nxp.com%7C16a07379ee384ddc18f908
> dc6fec
> >>
> +75e7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63850830857
> 3434788
> >>
> +%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIi
> LCJBTiI
> >>
> +6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=MBhqXwhXIQjDb3A
> RdYJ4U5EXM
> >> +ryEy%2F9m5X6jGuNhHxo%3D&reserved=0
> >> +$schema:
> >> +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdev
> >> +icetree.org%2Fmeta-
> schemas%2Fcore.yaml%23&data=05%7C02%7Cvabhav.shar
> >>
> +ma%40nxp.com%7C16a07379ee384ddc18f908dc6fec75e7%7C686ea1d3bc2
> b4c6fa9
> >>
> +2cd99c5c301635%7C0%7C0%7C638508308573446476%7CUnknown%7CTWF
> pbGZsb3d8
> >>
> +eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3
> D%7
> >>
> +C0%7C%7C%7C&sdata=m0RzUoVfr%2F2HkLlSOjhTq%2FQX3EM6ZAW7h5hQ
> Eidnc1g%3D
> >> +&reserved=0
> >> +
> >> +title: NXP i.MX Security Violation driver
> >
> > Violation detect driver
> 
> Bindings are not for drivers.
This is security violation detection hardware exported through SCU firmware. I will detail the HW in the binding
> 
> Best regards,
> Krzysztof

Vabhav Sharma June 7, 2024, 5:08 a.m. UTC | #6

> -----Original Message-----
> From: Frank Li <[e-mail]>
> Sent: Thursday, May 9, 2024 8:37 AM
> To: Vabhav Sharma <[e-mail]>
> Cc: Rob Herring <[e-mail]>; Krzysztof Kozlowski
> <[e-mail]>; Conor Dooley <[e-mail]>; Franck
> Lenormand <[e-mail]>; Aisheng Dong
> <[e-mail]>; Shawn Guo <[e-mail]>; Sascha
> Hauer <[e-mail]>; Pengutronix Kernel Team
> <[e-mail]>; Fabio Estevam <[e-mail]>; Peng Fan
> <[e-mail]>; [e-mail]; linux-
> [e-mail]; [e-mail]; linux-arm-
> [e-mail]; Varun Sethi <[e-mail]>; Silvano Di Ninno
> <[e-mail]>; Pankaj Gupta <[e-mail]>; Daniel
> Baluta <[e-mail]>
> Subject: Re: [PATCH 1/4] dt-bindings: firmware: secvio: Add device tree
> bindings
> 
> On Thu, May 09, 2024 at 02:45:32AM +0200, Vabhav Sharma wrote:
> > Document the secvio device tree bindings.
> 
> reduntant sentence.
Ok, I am removing in v3.
> >
> > The tampers are security feature available on i.MX products and
> > managed by SNVS block.The tamper goal is to detect the variation
>                         ^^ space here
> 
> > of hardware or physical parameters, which can indicate an attack.
> >
> > The SNVS, which provides secure non-volatile storage, allows to detect
> > some hardware attacks against the SoC.They are connected
>                                                ^^ space here
> > to the security-violation ports, which send an alert when an
> > out-of-range value is detected.
> >
> > The "imx-secvio-sc" module is designed to report security violations
> > and tamper triggering via SCU firmware to the user.
> >
> > Add the imx-scu secvio sub node and secvio sub node description.
> >
> > Signed-off-by: Franck LENORMAND <[e-mail]>
> > Signed-off-by: Vabhav Sharma <[e-mail]>
> > ---
> >  .../bindings/arm/freescale/fsl,scu-secvio.yaml     | 35
> ++++++++++++++++++++++
> >  .../devicetree/bindings/firmware/fsl,scu.yaml      | 10 +++++++
> >  2 files changed, 45 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu-secvio.yaml
> > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu-secvio.yaml
> > new file mode 100644
> > index 000000000000..30dc1e21f903
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu-secvio.y
> > +++ aml
> > @@ -0,0 +1,35 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/arm/freescale/fsl,scu-secvio.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: NXP i.MX Security Violation driver
> 
> Violation detect driver
Ok
> 
> > +
> > +maintainers:
> > +  - Franck LENORMAND <[e-mail]>
> > +
> > +description: |
> 
> Needn't "|"
Ok
> 
> > +  Receive security violation from the SNVS via the SCU firmware.
> > + Allow to  register notifier for additional processing
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - fsl,imx-sc-secvio
> > +
> > +  nvmem:
> > +    maxItems: 1
> > +
> 
> any interrupt defined? how do you notify such violation event?
Yes, there is security violation interrupt bit in register map of SECVIO HW block with uses RPC call to notify/enable/disable this bit using RPC API exported through SCU firmware
> 
> > +required:
> > +  - compatible
> > +  - nvmem
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    secvio {
> > +        compatible = "fsl,imx-sc-secvio";
> > +        nvmem = <&ocotp>;
> > +    };
> > diff --git a/Documentation/devicetree/bindings/firmware/fsl,scu.yaml
> > b/Documentation/devicetree/bindings/firmware/fsl,scu.yaml
> > index 557e524786c2..b40e127fdc88 100644
> > --- a/Documentation/devicetree/bindings/firmware/fsl,scu.yaml
> > +++ b/Documentation/devicetree/bindings/firmware/fsl,scu.yaml
> > @@ -129,6 +129,11 @@ properties:
> >        RTC controller provided by the SCU
> >      $ref: /schemas/rtc/fsl,scu-rtc.yaml
> >
> > +  secvio:
> > +    description:
> > +      Receive security violation from the SNVS via the SCU firmware
> > +    $ref: /schemas/arm/freescale/fsl,scu-secvio.yaml
> > +
> >    thermal-sensor:
> >      description:
> >        Thermal sensor provided by the SCU @@ -197,6 +202,11 @@
> > examples:
> >                  compatible = "fsl,imx8qxp-sc-rtc";
> >              };
> >
> > +            secvio {
> > +                compatible = "fsl,imx-sc-secvio";
> > +                nvmem = <&ocotp>;
> > +            };
> > +
> >              keys {
> >                  compatible = "fsl,imx8qxp-sc-key", "fsl,imx-sc-key";
> >                  linux,keycodes = <KEY_POWER>;
> >
> > --
> > 2.25.1
> >

Krzysztof Kozlowski June 7, 2024, 7:07 a.m. UTC | #7

On 07/06/2024 06:58, Vabhav Sharma wrote:
>>
>> Missing SoC compatibles.
> Ok, I will use fsl,imx8dxl-sc-secvio
>>
>> So no, that's just abuse of DT to instantiate driver.
>>
>> NAK. Drop the binding.
> I will detail the dt binding to describe the real hardware

Still looks like way just to instantiate driver. Why it cannot be part
of existing firmware SCU node?

Best regards,
Krzysztof

Aisheng Dong June 12, 2024, 7:20 a.m. UTC | #8

Hi Krzysztof

> From: Krzysztof Kozlowski <[e-mail]>
> Sent: 2024年6月7日 15:08
> 
> On 07/06/2024 06:58, Vabhav Sharma wrote:
> >>
> >> Missing SoC compatibles.
> > Ok, I will use fsl,imx8dxl-sc-secvio
> >>
> >> So no, that's just abuse of DT to instantiate driver.
> >>
> >> NAK. Drop the binding.
> > I will detail the dt binding to describe the real hardware
> 
> Still looks like way just to instantiate driver. Why it cannot be part of existing
> firmware SCU node?
> 

Technically yes. But SCU case is a little bit complicated as there're many
functions and all of them are already added as sub nodes in SCU node
for consistency and handling platform difference.

I guess some of them, e.g. rtc, could be part of SCU node (reuse) while
some couldn't. e.g. pinctrl
Do you want us to only make secvio reuse existing SCU node? 
This might look a bit strange to the existing sub nodes.

BTW, even we can reuse SCU node for secvio function, we still need update
binding doc to add extra property 'nvmem' for secvio.
Please share your suggestions considering above information.

e.g.
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/firmware/fsl,scu.yaml
    firmware {
        system-controller {
            compatible = "fsl,imx-scu";
	    ....

            rtc {
                compatible = "fsl,imx8qxp-sc-rtc";
            };

            keys {
                compatible = "fsl,imx8qxp-sc-key", "fsl,imx-sc-key";
                linux,keycodes = <KEY_POWER>;
            };

            watchdog {
                compatible = "fsl,imx8qxp-sc-wdt", "fsl,imx-sc-wdt";
                timeout-sec = <60>;
            };

            thermal-sensor {
                compatible = "fsl,imx8qxp-sc-thermal", "fsl,imx-sc-thermal";
                #thermal-sensor-cells = <1>;
            };
	.....
    };

Regards
Aisheng

> Best regards,
> Krzysztof

Krzysztof Kozlowski June 13, 2024, 6:13 a.m. UTC | #9

On 12/06/2024 09:20, Aisheng Dong wrote:
> Hi Krzysztof
> 
>> From: Krzysztof Kozlowski <[e-mail]>
>> Sent: 2024年6月7日 15:08
>>
>> On 07/06/2024 06:58, Vabhav Sharma wrote:
>>>>
>>>> Missing SoC compatibles.
>>> Ok, I will use fsl,imx8dxl-sc-secvio
>>>>
>>>> So no, that's just abuse of DT to instantiate driver.
>>>>
>>>> NAK. Drop the binding.
>>> I will detail the dt binding to describe the real hardware
>>
>> Still looks like way just to instantiate driver. Why it cannot be part of existing
>> firmware SCU node?
>>
> 
> Technically yes. But SCU case is a little bit complicated as there're many
> functions and all of them are already added as sub nodes in SCU node
> for consistency and handling platform difference.
> 
> I guess some of them, e.g. rtc, could be part of SCU node (reuse) while
> some couldn't. e.g. pinctrl
> Do you want us to only make secvio reuse existing SCU node? 

Yes

> This might look a bit strange to the existing sub nodes.

Nothing strange/unusual to me.

> 
> BTW, even we can reuse SCU node for secvio function, we still need update
> binding doc to add extra property 'nvmem' for secvio.

Sure.



Best regards,
Krzysztof

Aisheng Dong June 13, 2024, 8:48 a.m. UTC | #10

Hi Krzysztof, 

> From: Krzysztof Kozlowski <[e-mail]>
> Sent: 2024年6月13日 14:14
> 
> On 12/06/2024 09:20, Aisheng Dong wrote:
> > Hi Krzysztof
> >
> >> From: Krzysztof Kozlowski <[e-mail]>
> >> Sent: 2024年6月7日 15:08
> >>
> >> On 07/06/2024 06:58, Vabhav Sharma wrote:
> >>>>
> >>>> Missing SoC compatibles.
> >>> Ok, I will use fsl,imx8dxl-sc-secvio
> >>>>
> >>>> So no, that's just abuse of DT to instantiate driver.
> >>>>
> >>>> NAK. Drop the binding.
> >>> I will detail the dt binding to describe the real hardware
> >>
> >> Still looks like way just to instantiate driver. Why it cannot be
> >> part of existing firmware SCU node?
> >>
> >
> > Technically yes. But SCU case is a little bit complicated as there're
> > many functions and all of them are already added as sub nodes in SCU
> > node for consistency and handling platform difference.
> >
> > I guess some of them, e.g. rtc, could be part of SCU node (reuse)
> > while some couldn't. e.g. pinctrl Do you want us to only make secvio
> > reuse existing SCU node?
> 
> Yes
> 

Digging a bit more on the implementation. It seems there will be a
'parent depends on child' issue when reusing the parent SCU node for secvio.
Considering the defer probe support and ocotop could be modules, I'm still thinking
If any solution. Do you have a good suggestion?

e.g.
system-controller {
        compatible = "fsl,imx-scu";
		nvmem = <&ocotp>;
        ...

        ocotp: ocotp {
                compatible = "fsl,imx8qxp-scu-ocotp";
                #address-cells = <1>;
                #size-cells = <1>;
                read-only;
        };
		...
}

static int imx_scu_probe(struct platform_device *pdev)
{
		...
        ret = imx_secvio_sc_setup(dev);
        if (ret)
                dev_warn(dev, "failed to initialize secvio: %d\n", ret);
		...
        return devm_of_platform_populate(dev);
}

Regards
Aisheng

> > This might look a bit strange to the existing sub nodes.
> 
> Nothing strange/unusual to me.
> 
> >
> > BTW, even we can reuse SCU node for secvio function, we still need
> > update binding doc to add extra property 'nvmem' for secvio.
> 
> Sure.
> 
> 
> 
> Best regards,
> Krzysztof

Krzysztof Kozlowski June 16, 2024, 7:33 a.m. UTC | #11

On 13/06/2024 10:48, Aisheng Dong wrote:
> Hi Krzysztof, 
> 
>> From: Krzysztof Kozlowski <[e-mail]>
>> Sent: 2024年6月13日 14:14
>>
>> On 12/06/2024 09:20, Aisheng Dong wrote:
>>> Hi Krzysztof
>>>
>>>> From: Krzysztof Kozlowski <[e-mail]>
>>>> Sent: 2024年6月7日 15:08
>>>>
>>>> On 07/06/2024 06:58, Vabhav Sharma wrote:
>>>>>>
>>>>>> Missing SoC compatibles.
>>>>> Ok, I will use fsl,imx8dxl-sc-secvio
>>>>>>
>>>>>> So no, that's just abuse of DT to instantiate driver.
>>>>>>
>>>>>> NAK. Drop the binding.
>>>>> I will detail the dt binding to describe the real hardware
>>>>
>>>> Still looks like way just to instantiate driver. Why it cannot be
>>>> part of existing firmware SCU node?
>>>>
>>>
>>> Technically yes. But SCU case is a little bit complicated as there're
>>> many functions and all of them are already added as sub nodes in SCU
>>> node for consistency and handling platform difference.
>>>
>>> I guess some of them, e.g. rtc, could be part of SCU node (reuse)
>>> while some couldn't. e.g. pinctrl Do you want us to only make secvio
>>> reuse existing SCU node?
>>
>> Yes
>>
> 
> Digging a bit more on the implementation. It seems there will be a
> 'parent depends on child' issue when reusing the parent SCU node for secvio.
> Considering the defer probe support and ocotop could be modules, I'm still thinking
> If any solution. Do you have a good suggestion?

I don't see any problem there. You will have even worse if making it
children and using populate - unpredictable order.

Best regards,
Krzysztof

Aisheng Dong June 17, 2024, 10:11 a.m. UTC | #12

Hi Krzystof,

> From: Krzysztof Kozlowski <[e-mail]>
> Sent: 2024年6月16日 15:34
> On 13/06/2024 10:48, Aisheng Dong wrote:
> > Hi Krzysztof,
> >
> >> From: Krzysztof Kozlowski <[e-mail]>
> >> Sent: 2024年6月13日 14:14
> >>
> >> On 12/06/2024 09:20, Aisheng Dong wrote:
> >>> Hi Krzysztof
> >>>
> >>>> From: Krzysztof Kozlowski <[e-mail]>
> >>>> Sent: 2024年6月7日 15:08
> >>>>
> >>>> On 07/06/2024 06:58, Vabhav Sharma wrote:
> >>>>>>
> >>>>>> Missing SoC compatibles.
> >>>>> Ok, I will use fsl,imx8dxl-sc-secvio
> >>>>>>
> >>>>>> So no, that's just abuse of DT to instantiate driver.
> >>>>>>
> >>>>>> NAK. Drop the binding.
> >>>>> I will detail the dt binding to describe the real hardware
> >>>>
> >>>> Still looks like way just to instantiate driver. Why it cannot be
> >>>> part of existing firmware SCU node?
> >>>>
> >>>
> >>> Technically yes. But SCU case is a little bit complicated as
> >>> there're many functions and all of them are already added as sub
> >>> nodes in SCU node for consistency and handling platform difference.
> >>>
> >>> I guess some of them, e.g. rtc, could be part of SCU node (reuse)
> >>> while some couldn't. e.g. pinctrl Do you want us to only make secvio
> >>> reuse existing SCU node?
> >>
> >> Yes
> >>
> >
> > Digging a bit more on the implementation. It seems there will be a
> > 'parent depends on child' issue when reusing the parent SCU node for secvio.
> > Considering the defer probe support and ocotop could be modules, I'm
> > still thinking If any solution. Do you have a good suggestion?
> 
> I don't see any problem there. You will have even worse if making it children
> and using populate - unpredictable order.

Sorry I didn't find a good solution without making OCOTP node to be party of SCU
node too which we can't due to extra required properties of nvmem properties. 
E.g. #addr/size-cells.

I think the key problem is OCOTP is already a child node and there's a mismatch issue
If only making secvio, which depends on OCOTP, re-use their parent SCU node.
Maybe you have a good idea. Would you mind share a bit more?

BTW, I don't understand the order problem if making secvio a children as all
child nodes are Individual function devices and supports defer probe well.

Regards
Aisheng

> 
> Best regards,
> Krzysztof
>

Krzysztof Kozlowski June 17, 2024, 12:11 p.m. UTC | #13

On 17/06/2024 12:11, Aisheng Dong wrote:
> Hi Krzystof,
> 
>> From: Krzysztof Kozlowski <[e-mail]>
>> Sent: 2024年6月16日 15:34
>> On 13/06/2024 10:48, Aisheng Dong wrote:
>>> Hi Krzysztof,
>>>
>>>> From: Krzysztof Kozlowski <[e-mail]>
>>>> Sent: 2024年6月13日 14:14
>>>>
>>>> On 12/06/2024 09:20, Aisheng Dong wrote:
>>>>> Hi Krzysztof
>>>>>
>>>>>> From: Krzysztof Kozlowski <[e-mail]>
>>>>>> Sent: 2024年6月7日 15:08
>>>>>>
>>>>>> On 07/06/2024 06:58, Vabhav Sharma wrote:
>>>>>>>>
>>>>>>>> Missing SoC compatibles.
>>>>>>> Ok, I will use fsl,imx8dxl-sc-secvio
>>>>>>>>
>>>>>>>> So no, that's just abuse of DT to instantiate driver.
>>>>>>>>
>>>>>>>> NAK. Drop the binding.
>>>>>>> I will detail the dt binding to describe the real hardware
>>>>>>
>>>>>> Still looks like way just to instantiate driver. Why it cannot be
>>>>>> part of existing firmware SCU node?
>>>>>>
>>>>>
>>>>> Technically yes. But SCU case is a little bit complicated as
>>>>> there're many functions and all of them are already added as sub
>>>>> nodes in SCU node for consistency and handling platform difference.
>>>>>
>>>>> I guess some of them, e.g. rtc, could be part of SCU node (reuse)
>>>>> while some couldn't. e.g. pinctrl Do you want us to only make secvio
>>>>> reuse existing SCU node?
>>>>
>>>> Yes
>>>>
>>>
>>> Digging a bit more on the implementation. It seems there will be a
>>> 'parent depends on child' issue when reusing the parent SCU node for secvio.
>>> Considering the defer probe support and ocotop could be modules, I'm
>>> still thinking If any solution. Do you have a good suggestion?
>>
>> I don't see any problem there. You will have even worse if making it children
>> and using populate - unpredictable order.
> 
> Sorry I didn't find a good solution without making OCOTP node to be party of SCU
> node too which we can't due to extra required properties of nvmem properties. 
> E.g. #addr/size-cells.
> 
> I think the key problem is OCOTP is already a child node and there's a mismatch issue
> If only making secvio, which depends on OCOTP, re-use their parent SCU node.
> Maybe you have a good idea. Would you mind share a bit more?
> 
> BTW, I don't understand the order problem if making secvio a children as all
> child nodes are Individual function devices and supports defer probe well.

You keep talking about drivers but what does it have to do with bindings
at all?

You can order your probe however you wish regardless of bindings. You
can handle or not handle deferred probe. You can create circular
dependencies or break them. All regardless of the bindings.

Best regards,
Krzysztof

Patch

diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,scu-secvio.yaml b/Documentation/devicetree/bindings/arm/freescale/fsl,scu-secvio.yaml
new file mode 100644
index 000000000000..30dc1e21f903
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu-secvio.yaml
@@ -0,0 +1,35 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/arm/freescale/fsl,scu-secvio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP i.MX Security Violation driver
+
+maintainers:
+  - Franck LENORMAND <[e-mail]>
+
+description: |
+  Receive security violation from the SNVS via the SCU firmware. Allow to
+  register notifier for additional processing
+
+properties:
+  compatible:
+    enum:
+      - fsl,imx-sc-secvio
+
+  nvmem:
+    maxItems: 1
+
+required:
+  - compatible
+  - nvmem
+
+additionalProperties: false
+
+examples:
+  - |
+    secvio {
+        compatible = "fsl,imx-sc-secvio";
+        nvmem = <&ocotp>;
+    };
diff --git a/Documentation/devicetree/bindings/firmware/fsl,scu.yaml b/Documentation/devicetree/bindings/firmware/fsl,scu.yaml
index 557e524786c2..b40e127fdc88 100644
--- a/Documentation/devicetree/bindings/firmware/fsl,scu.yaml
+++ b/Documentation/devicetree/bindings/firmware/fsl,scu.yaml
@@ -129,6 +129,11 @@  properties:
       RTC controller provided by the SCU
     $ref: /schemas/rtc/fsl,scu-rtc.yaml
 
+  secvio:
+    description:
+      Receive security violation from the SNVS via the SCU firmware
+    $ref: /schemas/arm/freescale/fsl,scu-secvio.yaml
+
   thermal-sensor:
     description:
       Thermal sensor provided by the SCU
@@ -197,6 +202,11 @@  examples:
                 compatible = "fsl,imx8qxp-sc-rtc";
             };
 
+            secvio {
+                compatible = "fsl,imx-sc-secvio";
+                nvmem = <&ocotp>;
+            };
+
             keys {
                 compatible = "fsl,imx8qxp-sc-key", "fsl,imx-sc-key";
                 linux,keycodes = <KEY_POWER>;