[2/3] dt-bindings: mtd: Add a property to declare secure regions in Qcom NANDc

Message ID 20210222120259.94465-3-manivannan.sadhasivam@linaro.org
State New
Headers show
Series
  • Add support for secure regions in Qcom NANDc driver
Related show

Commit Message

Manivannan Sadhasivam Feb. 22, 2021, 12:02 p.m.
On a typical end product, a vendor may choose to secure some regions in
the NAND memory which are supposed to stay intact between FW upgrades.
The access to those regions will be blocked by a secure element like
Trustzone. So the normal world software like Linux kernel should not
touch these regions (including reading).

So let's add a property for declaring such secure regions so that the
driver can skip touching them.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

---
 Documentation/devicetree/bindings/mtd/qcom,nandc.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Comments

Miquel Raynal Feb. 23, 2021, 4:49 p.m. | #1
Hi Manivannan,

Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote on Mon,
22 Feb 2021 17:32:58 +0530:

> On a typical end product, a vendor may choose to secure some regions in
> the NAND memory which are supposed to stay intact between FW upgrades.
> The access to those regions will be blocked by a secure element like
> Trustzone. So the normal world software like Linux kernel should not
> touch these regions (including reading).
> 
> So let's add a property for declaring such secure regions so that the
> driver can skip touching them.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  Documentation/devicetree/bindings/mtd/qcom,nandc.yaml | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml b/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
> index 84ad7ff30121..7500e20da9c1 100644
> --- a/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
> +++ b/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
> @@ -48,6 +48,13 @@ patternProperties:
>          enum:
>            - 512
>  
> +      qcom,secure-regions:
> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> +        description:
> +          Regions in the NAND memory which are protected using a secure element
> +          like Trustzone. This property contains the start address and size of
> +          the secure regions present (optional).

What does this "(optional)" means? If you mean the property is optional
then it should be described accordingly in the yaml file, or am I
missing something?

I wonder if it wouldn't be better to make this a NAND chip node
property. I don't think a qcom prefix is needed as potentially many
other SoCs might have the same "feature".

I'm fine adding support for it in the qcom driver only though.

> +
>  allOf:
>    - $ref: "nand-controller.yaml#"
>  

Thanks,
Miquèl
Manivannan Sadhasivam Feb. 23, 2021, 5:45 p.m. | #2
Hi Miquel,

On Tue, Feb 23, 2021 at 05:49:22PM +0100, Miquel Raynal wrote:
> Hi Manivannan,

> 

> Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote on Mon,

> 22 Feb 2021 17:32:58 +0530:

> 

> > On a typical end product, a vendor may choose to secure some regions in

> > the NAND memory which are supposed to stay intact between FW upgrades.

> > The access to those regions will be blocked by a secure element like

> > Trustzone. So the normal world software like Linux kernel should not

> > touch these regions (including reading).

> > 

> > So let's add a property for declaring such secure regions so that the

> > driver can skip touching them.

> > 

> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

> > ---

> >  Documentation/devicetree/bindings/mtd/qcom,nandc.yaml | 7 +++++++

> >  1 file changed, 7 insertions(+)

> > 

> > diff --git a/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml b/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml

> > index 84ad7ff30121..7500e20da9c1 100644

> > --- a/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml

> > +++ b/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml

> > @@ -48,6 +48,13 @@ patternProperties:

> >          enum:

> >            - 512

> >  

> > +      qcom,secure-regions:

> > +        $ref: /schemas/types.yaml#/definitions/uint32-array

> > +        description:

> > +          Regions in the NAND memory which are protected using a secure element

> > +          like Trustzone. This property contains the start address and size of

> > +          the secure regions present (optional).

> 

> What does this "(optional)" means? If you mean the property is optional

> then it should be described accordingly in the yaml file, or am I

> missing something?

> 


IIUC, if a property is not listed under "required" section then it is
optional. But I've added the quote here to just make it explicit.

> I wonder if it wouldn't be better to make this a NAND chip node

> property. I don't think a qcom prefix is needed as potentially many

> other SoCs might have the same "feature".

> 

> I'm fine adding support for it in the qcom driver only though.

> 


Hmm, sounds good to me.

Thanks,
Mani

> > +

> >  allOf:

> >    - $ref: "nand-controller.yaml#"

> >  

> 

> Thanks,

> Miquèl


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
Miquel Raynal Feb. 23, 2021, 5:56 p.m. | #3
Hi Manivannan,

Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote on Tue,
23 Feb 2021 23:15:46 +0530:

> Hi Miquel,
> 
> On Tue, Feb 23, 2021 at 05:49:22PM +0100, Miquel Raynal wrote:
> > Hi Manivannan,
> > 
> > Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote on Mon,
> > 22 Feb 2021 17:32:58 +0530:
> >   
> > > On a typical end product, a vendor may choose to secure some regions in
> > > the NAND memory which are supposed to stay intact between FW upgrades.
> > > The access to those regions will be blocked by a secure element like
> > > Trustzone. So the normal world software like Linux kernel should not
> > > touch these regions (including reading).
> > > 
> > > So let's add a property for declaring such secure regions so that the
> > > driver can skip touching them.
> > > 
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > ---
> > >  Documentation/devicetree/bindings/mtd/qcom,nandc.yaml | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml b/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
> > > index 84ad7ff30121..7500e20da9c1 100644
> > > --- a/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
> > > +++ b/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
> > > @@ -48,6 +48,13 @@ patternProperties:
> > >          enum:
> > >            - 512
> > >  
> > > +      qcom,secure-regions:
> > > +        $ref: /schemas/types.yaml#/definitions/uint32-array
> > > +        description:
> > > +          Regions in the NAND memory which are protected using a secure element
> > > +          like Trustzone. This property contains the start address and size of
> > > +          the secure regions present (optional).  
> > 
> > What does this "(optional)" means? If you mean the property is optional
> > then it should be described accordingly in the yaml file, or am I
> > missing something?
> >   
> 
> IIUC, if a property is not listed under "required" section then it is
> optional. But I've added the quote here to just make it explicit.

Absolutely, I was just wondering why you mentioned it here. I don't
think it's useful, you can drop it.

> 
> > I wonder if it wouldn't be better to make this a NAND chip node
> > property. I don't think a qcom prefix is needed as potentially many
> > other SoCs might have the same "feature".
> > 
> > I'm fine adding support for it in the qcom driver only though.
> >   
> 
> Hmm, sounds good to me.
> 
> Thanks,
> Mani
> 
> > > +
> > >  allOf:
> > >    - $ref: "nand-controller.yaml#"
> > >    
> > 
> > Thanks,
> > Miquèl  


Thanks,
Miquèl
Manivannan Sadhasivam March 8, 2021, 5:31 a.m. | #4
On Fri, Mar 05, 2021 at 05:36:57PM -0600, Rob Herring wrote:
> On Mon, Feb 22, 2021 at 05:32:58PM +0530, Manivannan Sadhasivam wrote:

> > On a typical end product, a vendor may choose to secure some regions in

> > the NAND memory which are supposed to stay intact between FW upgrades.

> > The access to those regions will be blocked by a secure element like

> > Trustzone. So the normal world software like Linux kernel should not

> > touch these regions (including reading).

> > 

> > So let's add a property for declaring such secure regions so that the

> > driver can skip touching them.

> > 

> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

> > ---

> >  Documentation/devicetree/bindings/mtd/qcom,nandc.yaml | 7 +++++++

> >  1 file changed, 7 insertions(+)

> > 

> > diff --git a/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml b/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml

> > index 84ad7ff30121..7500e20da9c1 100644

> > --- a/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml

> > +++ b/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml

> > @@ -48,6 +48,13 @@ patternProperties:

> >          enum:

> >            - 512

> >  

> > +      qcom,secure-regions:

> > +        $ref: /schemas/types.yaml#/definitions/uint32-array

> 

> Don't you need 64-bit regions potentially? Though 4GB should be enough 

> for anyone.

> 


Yes, given the size of current NAND based systems around, I thought 32 bit is
enough.

> If more than one addr+size, then you need a matrix.

> 


Okay.

Thanks,
Mani

> > +        description:

> > +          Regions in the NAND memory which are protected using a secure element

> > +          like Trustzone. This property contains the start address and size of

> > +          the secure regions present (optional).

> > +

> >  allOf:

> >    - $ref: "nand-controller.yaml#"

> >  

> > -- 

> > 2.25.1

> > 


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Patch

diff --git a/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml b/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
index 84ad7ff30121..7500e20da9c1 100644
--- a/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
+++ b/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
@@ -48,6 +48,13 @@  patternProperties:
         enum:
           - 512
 
+      qcom,secure-regions:
+        $ref: /schemas/types.yaml#/definitions/uint32-array
+        description:
+          Regions in the NAND memory which are protected using a secure element
+          like Trustzone. This property contains the start address and size of
+          the secure regions present (optional).
+
 allOf:
   - $ref: "nand-controller.yaml#"