Re: [PATCH v3 03/10] ARM: dts: exynos: Rename SysRAM node to "sram"

Re: [PATCH v3 03/10] ARM: dts: exynos: Rename SysRAM node to "sram"

[Krzysztof Kozlowski]

On Wed, Oct 02, 2019 at 06:43:09PM +0200, Krzysztof Kozlowski wrote:
> The device node name should reflect generic class of a device so rename
> the SysRAM node from "sysram" to "sram".  The child nodes stay as before
> as "smp-sysram" to match their real purpose.  This will be also in sync
> with upcoming DT schema.  No functional change.
> 
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
>  arch/arm/boot/dts/exynos3250.dtsi | 2 +-
>  arch/arm/boot/dts/exynos4210.dtsi | 2 +-
>  arch/arm/boot/dts/exynos4412.dtsi | 2 +-
>  arch/arm/boot/dts/exynos5250.dtsi | 2 +-
>  arch/arm/boot/dts/exynos54xx.dtsi | 2 +-

Applied this one to samsung-soc.

Best regards,
Krzysztof

Re: [PATCH v3 01/10] dt-bindings: sram: Convert SRAM bindings to json-schema

[Rob Herring]

On Wed, Oct 02, 2019 at 06:43:07PM +0200, Krzysztof Kozlowski wrote:
> Convert generic mmio-sram bindings to DT schema format using
> json-schema.  Require the address/size cells to be 1, not equal to root
> node.
> 
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> 
> ---
> 
> Changes since v2:
> 1. Add Rob as maintainer,
> 2. Use "contains" for compatible,
> 3. Fix address and size cells to 1,
> 4. Add maxitems to reg under children,
> 5. Remove unneeded string type from label.
> 
> Changes since v1:
> 1. Indent example with four spaces (more readable).
> ---
>  .../devicetree/bindings/sram/sram.txt         |  80 -----------
>  .../devicetree/bindings/sram/sram.yaml        | 134 ++++++++++++++++++
>  2 files changed, 134 insertions(+), 80 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/sram/sram.txt
>  create mode 100644 Documentation/devicetree/bindings/sram/sram.yaml


> diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
> new file mode 100644
> index 000000000000..a1c1ec2183f2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sram/sram.yaml
> @@ -0,0 +1,134 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sram/sram.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Generic on-chip SRAM
> +
> +maintainers:
> +  - Rob Herring <robh@kernel.org>
> +
> +description: |+
> +  Simple IO memory regions to be managed by the genalloc API.
> +
> +  Each child of the sram node specifies a region of reserved memory. Each
> +  child node should use a 'reg' property to specify a specific range of
> +  reserved memory.
> +
> +  Following the generic-names recommended practice, node names should
> +  reflect the purpose of the node. Unit address (@<address>) should be
> +  appended to the name.
> +
> +properties:
> +  $nodename:
> +    pattern: "^sram(@.*)?"
> +
> +  compatible:
> +    contains:
> +      enum:
> +        - mmio-sram
> +        - atmel,sama5d2-securam
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 1
> +
> +  ranges:
> +    description:
> +      Should translate from local addresses within the sram to bus addresses.
> +
> +  no-memory-wc:
> +    description:
> +      The flag indicating, that SRAM memory region has not to be remapped
> +      as write combining. WC is used by default.
> +    type: boolean
> +
> +patternProperties:
> +  "^([a-z]*-)?sram@[a-f0-9]$":
> +    type: object
> +    description:
> +      Each child of the sram node specifies a region of reserved memory.
> +    properties:
> +      reg:
> +        description:
> +          IO mem address range, relative to the SRAM range.
> +        maxItems: 1
> +
> +      compatible:
> +        $ref: /schemas/types.yaml#/definitions/string

No need to define the type again. We can say 'maxItems: 1' if we really 
want to force it to 1 entry.

> +        description:
> +          Should contain a vendor specific string in the form
> +          <vendor>,[<device>-]<usage>
> +
> +      pool:
> +        description:
> +          Indicates that the particular reserved SRAM area is addressable
> +          and in use by another device or devices.
> +        type: boolean
> +
> +      export:
> +        description:
> +          Indicates that the reserved SRAM area may be accessed outside
> +          of the kernel, e.g. by bootloader or userspace.
> +        type: boolean
> +
> +      protect-exec:
> +        description: |
> +          Same as 'pool' above but with the additional constraint that code
> +          will be run from the region and that the memory is maintained as
> +          read-only, executable during code execution. NOTE: This region must
> +          be page aligned on start and end in order to properly allow
> +          manipulation of the page attributes.
> +        type: boolean
> +
> +      label:
> +        description:
> +          The name for the reserved partition, if omitted, the label is taken
> +          from the node name excluding the unit address.
> +
> +      clocks:

Shouldn't this be up one level? Looks like this is the only case 
(Marvell and i.MX are the only ones I see with clocks).

> +        description:
> +          A list of phandle and clock specifier pair that controls the
> +          single SRAM clock.

maxItems: 1

> +
> +    required:
> +      - reg
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#address-cells"
> +  - "#size-cells"
> +  - ranges

Does 'additionalProperties' work here and/or in the child node? I guess 
not if we keep some node names.

> +
> +examples:
> +  - |
> +    sram: sram@5c000000 {
> +        compatible = "mmio-sram";
> +        reg = <0x5c000000 0x40000>; /* 256 KiB SRAM at address 0x5c000000 */
> +
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +        ranges = <0 0x5c000000 0x40000>;
> +
> +        smp-sram@100 {
> +            compatible = "socvendor,smp-sram";
> +            reg = <0x100 0x50>;
> +        };
> +
> +        device-sram@1000 {
> +            reg = <0x1000 0x1000>;
> +            pool;
> +        };
> +
> +        exported@20000 {

This one doesn't match the pattern. That's fine I guess for dts files, 
but examples should be good examples.

> +            reg = <0x20000 0x20000>;
> +            export;
> +        };
> +    };
> -- 
> 2.17.1
> 

Re: [PATCH v3 01/10] dt-bindings: sram: Convert SRAM bindings to json-schema

[Krzysztof Kozlowski]

On Thu, Oct 10, 2019 at 02:12:40PM -0500, Rob Herring wrote:
> On Wed, Oct 02, 2019 at 06:43:07PM +0200, Krzysztof Kozlowski wrote:
> > Convert generic mmio-sram bindings to DT schema format using
> > json-schema.  Require the address/size cells to be 1, not equal to root
> > node.
> > 
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> > 
> > ---
> > 
> > Changes since v2:
> > 1. Add Rob as maintainer,
> > 2. Use "contains" for compatible,
> > 3. Fix address and size cells to 1,
> > 4. Add maxitems to reg under children,
> > 5. Remove unneeded string type from label.
> > 
> > Changes since v1:
> > 1. Indent example with four spaces (more readable).
> > ---
> >  .../devicetree/bindings/sram/sram.txt         |  80 -----------
> >  .../devicetree/bindings/sram/sram.yaml        | 134 ++++++++++++++++++
> >  2 files changed, 134 insertions(+), 80 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/sram/sram.txt
> >  create mode 100644 Documentation/devicetree/bindings/sram/sram.yaml
> 
> 
> > diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
> > new file mode 100644
> > index 000000000000..a1c1ec2183f2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sram/sram.yaml
> > @@ -0,0 +1,134 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/sram/sram.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Generic on-chip SRAM
> > +
> > +maintainers:
> > +  - Rob Herring <robh@kernel.org>
> > +
> > +description: |+
> > +  Simple IO memory regions to be managed by the genalloc API.
> > +
> > +  Each child of the sram node specifies a region of reserved memory. Each
> > +  child node should use a 'reg' property to specify a specific range of
> > +  reserved memory.
> > +
> > +  Following the generic-names recommended practice, node names should
> > +  reflect the purpose of the node. Unit address (@<address>) should be
> > +  appended to the name.
> > +
> > +properties:
> > +  $nodename:
> > +    pattern: "^sram(@.*)?"
> > +
> > +  compatible:
> > +    contains:
> > +      enum:
> > +        - mmio-sram
> > +        - atmel,sama5d2-securam
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  "#address-cells":
> > +    const: 1
> > +
> > +  "#size-cells":
> > +    const: 1
> > +
> > +  ranges:
> > +    description:
> > +      Should translate from local addresses within the sram to bus addresses.
> > +
> > +  no-memory-wc:
> > +    description:
> > +      The flag indicating, that SRAM memory region has not to be remapped
> > +      as write combining. WC is used by default.
> > +    type: boolean
> > +
> > +patternProperties:
> > +  "^([a-z]*-)?sram@[a-f0-9]$":
> > +    type: object
> > +    description:
> > +      Each child of the sram node specifies a region of reserved memory.
> > +    properties:
> > +      reg:
> > +        description:
> > +          IO mem address range, relative to the SRAM range.
> > +        maxItems: 1
> > +
> > +      compatible:
> > +        $ref: /schemas/types.yaml#/definitions/string
> 
> No need to define the type again. We can say 'maxItems: 1' if we really 
> want to force it to 1 entry.

I'll fix it and integrate Samsung compatibles here.

> 
> > +        description:
> > +          Should contain a vendor specific string in the form
> > +          <vendor>,[<device>-]<usage>
> > +
> > +      pool:
> > +        description:
> > +          Indicates that the particular reserved SRAM area is addressable
> > +          and in use by another device or devices.
> > +        type: boolean
> > +
> > +      export:
> > +        description:
> > +          Indicates that the reserved SRAM area may be accessed outside
> > +          of the kernel, e.g. by bootloader or userspace.
> > +        type: boolean
> > +
> > +      protect-exec:
> > +        description: |
> > +          Same as 'pool' above but with the additional constraint that code
> > +          will be run from the region and that the memory is maintained as
> > +          read-only, executable during code execution. NOTE: This region must
> > +          be page aligned on start and end in order to properly allow
> > +          manipulation of the page attributes.
> > +        type: boolean
> > +
> > +      label:
> > +        description:
> > +          The name for the reserved partition, if omitted, the label is taken
> > +          from the node name excluding the unit address.
> > +
> > +      clocks:
> 
> Shouldn't this be up one level? Looks like this is the only case 
> (Marvell and i.MX are the only ones I see with clocks).

Yes, that's a mistake in original bindings.

> 
> > +        description:
> > +          A list of phandle and clock specifier pair that controls the
> > +          single SRAM clock.
> 
> maxItems: 1

Yes.

> 
> > +
> > +    required:
> > +      - reg
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - "#address-cells"
> > +  - "#size-cells"
> > +  - ranges
> 
> Does 'additionalProperties' work here and/or in the child node? I guess 
> not if we keep some node names.

It seems that it works.

> 
> > +
> > +examples:
> > +  - |
> > +    sram: sram@5c000000 {
> > +        compatible = "mmio-sram";
> > +        reg = <0x5c000000 0x40000>; /* 256 KiB SRAM at address 0x5c000000 */
> > +
> > +        #address-cells = <1>;
> > +        #size-cells = <1>;
> > +        ranges = <0 0x5c000000 0x40000>;
> > +
> > +        smp-sram@100 {
> > +            compatible = "socvendor,smp-sram";
> > +            reg = <0x100 0x50>;
> > +        };
> > +
> > +        device-sram@1000 {
> > +            reg = <0x1000 0x1000>;
> > +            pool;
> > +        };
> > +
> > +        exported@20000 {
> 
> This one doesn't match the pattern. That's fine I guess for dts files, 
> but examples should be good examples.

Sure.

Best regards,
Krzysztof

> 
> > +            reg = <0x20000 0x20000>;
> > +            export;
> > +        };
> > +    };
> > -- 
> > 2.17.1
> > 

Re: [PATCH v3 02/10] dt-bindings: sram: Convert Samsung Exynos SYSRAM bindings to json-schema

[Rob Herring]

On Wed, Oct 02, 2019 at 06:43:08PM +0200, Krzysztof Kozlowski wrote:
> Convert Samsung Exynos SYSRAM bindings to DT schema format using
> json-schema.
> 
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> 
> ---
> 
> Changes since v2:
> 1. Use sram as name of node in example.
> 
> Changes since v1:
> 1. Indent example with four spaces (more readable).
> ---
>  .../devicetree/bindings/sram/samsung-sram.txt | 38 ------------
>  .../bindings/sram/samsung-sram.yaml           | 58 +++++++++++++++++++
>  MAINTAINERS                                   |  2 +-
>  3 files changed, 59 insertions(+), 39 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/sram/samsung-sram.txt
>  create mode 100644 Documentation/devicetree/bindings/sram/samsung-sram.yaml


> diff --git a/Documentation/devicetree/bindings/sram/samsung-sram.yaml b/Documentation/devicetree/bindings/sram/samsung-sram.yaml
> new file mode 100644
> index 000000000000..3e77c434ecca
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sram/samsung-sram.yaml
> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sram/samsung-sram.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Samsung Exynos SoC SYSRAM for SMP bringup
> +
> +maintainers:
> +  - Krzysztof Kozlowski <krzk@kernel.org>
> +
> +description: |+
> +  Samsung SMP-capable Exynos SoCs use part of the SYSRAM for the bringup
> +  of the secondary cores. Once the core gets powered up it executes the
> +  code that is residing at some specific location of the SYSRAM.
> +
> +  Therefore reserved section sub-nodes have to be added to the mmio-sram
> +  declaration. These nodes are of two types depending upon secure or
> +  non-secure execution environment.
> +
> +allOf:
> +  - $ref: "sram.yaml#"
> +
> +properties:
> +  $nodename:
> +    pattern: "^sysram(@.*)?"

As you are renaming all the node names, this will no longer work. If you 
change it to 'sram', that's going to match others, but would still work 
as long as the child node names are unique to Samsung. If you change 
them to '*-sram' then, you'd have to come up with something else. That 
probably means using 'compatible' strings. At that point, it's kind of 
silly to just be validating what your are using to select the schema. It 
may be better to just add the compatible strings into sram.yaml if 
that's the only difference.

> +
> +patternProperties:
> +  "^([a-z]*-)?sysram@[a-f0-9]$":
> +    type: object
> +
> +    properties:
> +      compatible:
> +        description:
> +          Depending upon boot mode
> +        enum:
> +          - samsung,exynos4210-sysram                 # for Secure SYSRAM
> +          - samsung,exynos4210-sysram-ns              # for Non-secure SYSRAM
> +

Re: [PATCH v3 02/10] dt-bindings: sram: Convert Samsung Exynos SYSRAM bindings to json-schema

[Krzysztof Kozlowski]

On Thu, Oct 10, 2019 at 02:33:56PM -0500, Rob Herring wrote:
> On Wed, Oct 02, 2019 at 06:43:08PM +0200, Krzysztof Kozlowski wrote:
> > Convert Samsung Exynos SYSRAM bindings to DT schema format using
> > json-schema.
> > 
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> > 
> > ---
> > 
> > Changes since v2:
> > 1. Use sram as name of node in example.
> > 
> > Changes since v1:
> > 1. Indent example with four spaces (more readable).
> > ---
> >  .../devicetree/bindings/sram/samsung-sram.txt | 38 ------------
> >  .../bindings/sram/samsung-sram.yaml           | 58 +++++++++++++++++++
> >  MAINTAINERS                                   |  2 +-
> >  3 files changed, 59 insertions(+), 39 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/sram/samsung-sram.txt
> >  create mode 100644 Documentation/devicetree/bindings/sram/samsung-sram.yaml
> 
> 
> > diff --git a/Documentation/devicetree/bindings/sram/samsung-sram.yaml b/Documentation/devicetree/bindings/sram/samsung-sram.yaml
> > new file mode 100644
> > index 000000000000..3e77c434ecca
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sram/samsung-sram.yaml
> > @@ -0,0 +1,58 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/sram/samsung-sram.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Samsung Exynos SoC SYSRAM for SMP bringup
> > +
> > +maintainers:
> > +  - Krzysztof Kozlowski <krzk@kernel.org>
> > +
> > +description: |+
> > +  Samsung SMP-capable Exynos SoCs use part of the SYSRAM for the bringup
> > +  of the secondary cores. Once the core gets powered up it executes the
> > +  code that is residing at some specific location of the SYSRAM.
> > +
> > +  Therefore reserved section sub-nodes have to be added to the mmio-sram
> > +  declaration. These nodes are of two types depending upon secure or
> > +  non-secure execution environment.
> > +
> > +allOf:
> > +  - $ref: "sram.yaml#"
> > +
> > +properties:
> > +  $nodename:
> > +    pattern: "^sysram(@.*)?"
> 
> As you are renaming all the node names, this will no longer work. If you 
> change it to 'sram', that's going to match others, but would still work 
> as long as the child node names are unique to Samsung. If you change 
> them to '*-sram' then, you'd have to come up with something else. That 
> probably means using 'compatible' strings. At that point, it's kind of 
> silly to just be validating what your are using to select the schema. It 
> may be better to just add the compatible strings into sram.yaml if 
> that's the only difference.

You're right. I'll move them to sram.yaml and maybe add also example.


Best regards,
Krzysztof

Re: [PATCH v3 06/10] ARM: dts: imx: Rename "iram" node to "sram"

[Shawn Guo]

On Wed, Oct 02, 2019 at 06:43:12PM +0200, Krzysztof Kozlowski wrote:
> The device node name should reflect generic class of a device so rename
> the "iram" node to "sram".  This will be also in sync with upcoming DT
> schema.  No functional change.
> 
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>

Acked-by: Shawn Guo <shawnguo@kernel.org>

Let me know if you want it go through IMX tree.

Shawn

Re: [PATCH v3 06/10] ARM: dts: imx: Rename "iram" node to "sram"

[Krzysztof Kozlowski]

On Mon, 14 Oct 2019 at 14:33, Shawn Guo <shawnguo@kernel.org> wrote:
>
> On Wed, Oct 02, 2019 at 06:43:12PM +0200, Krzysztof Kozlowski wrote:
> > The device node name should reflect generic class of a device so rename
> > the "iram" node to "sram".  This will be also in sync with upcoming DT
> > schema.  No functional change.
> >
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
>
> Acked-by: Shawn Guo <shawnguo@kernel.org>
>
> Let me know if you want it go through IMX tree.

Yes please, I think each of these should go through soc maintainers.

Best regards,
Krzysztof

Re: [PATCH v3 06/10] ARM: dts: imx: Rename "iram" node to "sram"

[Shawn Guo]

On Wed, Oct 02, 2019 at 06:43:12PM +0200, Krzysztof Kozlowski wrote:
> The device node name should reflect generic class of a device so rename
> the "iram" node to "sram".  This will be also in sync with upcoming DT
> schema.  No functional change.
> 
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>

Applied, thanks.

Re: [PATCH v3 10/10] ARM: dts: omap: Rename "ocmcram" node to "sram"

[Tony Lindgren]

* Krzysztof Kozlowski <krzk@kernel.org> [191002 09:45]:
> The device node name should reflect generic class of a device so rename
> the "ocmcram" node to "sram".  This will be also in sync with upcoming DT
> schema.  No functional change.

Applying this into omap-for-v5.5/dt thanks.

Tony