[v4,6/7] dt-bindings: cache: r9a07g043f-l2-cache: Add DT binding documentation for L2 cache controller

  From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Add DT binding documentation for L2 cache controller found on RZ/Five SoC.

The Renesas RZ/Five microprocessor includes a RISC-V CPU Core (AX45MP
Single) from Andes. The AX45MP core has an L2 cache controller, this patch
describes the L2 cache block.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
RFC v3 -> v4
* Dropped l2 cache configuration parameters
* s/larger/large
* Added minItems/maxItems for andestech,pma-regions
---
 .../cache/andestech,ax45mp-cache.yaml         | 93 +++++++++++++++++++
 .../cache/andestech,ax45mp-cache.h            | 38 ++++++++
 2 files changed, 131 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/cache/andestech,ax45mp-cache.yaml
 create mode 100644 include/dt-bindings/cache/andestech,ax45mp-cache.h
  

On 24/11/2022 18:22, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Add DT binding documentation for L2 cache controller found on RZ/Five SoC.
> 
> The Renesas RZ/Five microprocessor includes a RISC-V CPU Core (AX45MP
> Single) from Andes. The AX45MP core has an L2 cache controller, this patch
> describes the L2 cache block.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> RFC v3 -> v4
> * Dropped l2 cache configuration parameters
> * s/larger/large
> * Added minItems/maxItems for andestech,pma-regions
> ---
>  .../cache/andestech,ax45mp-cache.yaml         | 93 +++++++++++++++++++
>  .../cache/andestech,ax45mp-cache.h            | 38 ++++++++
>  2 files changed, 131 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/cache/andestech,ax45mp-cache.yaml
>  create mode 100644 include/dt-bindings/cache/andestech,ax45mp-cache.h
> 
> diff --git a/Documentation/devicetree/bindings/cache/andestech,ax45mp-cache.yaml b/Documentation/devicetree/bindings/cache/andestech,ax45mp-cache.yaml
> new file mode 100644
> index 000000000000..bf255b177d0a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cache/andestech,ax45mp-cache.yaml
> @@ -0,0 +1,93 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright (C) 2022 Renesas Electronics Corp.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/cache/andestech,ax45mp-cache.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Andestech AX45MP L2 Cache Controller
> +
> +maintainers:
> +  - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> +
> +description:
> +  A level-2 cache (L2C) is used to improve the system performance by providing
> +  a large amount of cache line entries and reasonable access delays. The L2C
> +  is shared between cores, and a non-inclusive non-exclusive policy is used.
> +
> +select:
> +  properties:
> +    compatible:
> +      contains:
> +        enum:
> +          - andestech,ax45mp-cache
> +
> +  required:
> +    - compatible
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: andestech,ax45mp-cache
> +      - const: cache
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  cache-line-size:
> +    const: 64
> +
> +  cache-level:
> +    const: 2
> +
> +  cache-sets:
> +    const: 1024
> +
> +  cache-size:
> +    enum: [131072, 262144, 524288, 1048576, 2097152]
> +
> +  cache-unified: true
> +
> +  next-level-cache: true
> +
> +  andestech,pma-regions:
> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> +    minItems: 1
> +    maxItems: 16
> +    items:
> +      minItems: 3
> +      maxItems: 3

Instead:
items:
  items:
    - description: Explain
    - description: what is
    - description: here

> +    description: Optional array of memory regions to be set in the PMA.
> +
> +additionalProperties: false
> +
> +required:
> +  - compatible
> +  - cache-line-size
> +  - cache-level
> +  - cache-sets
> +  - cache-size
> +  - cache-unified
> +  - interrupts
> +  - reg

Keep the same order as properties appear in the "properties:"

> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/cache/andestech,ax45mp-cache.h>
> +
> +    cache-controller@2010000 {
> +        reg = <0x13400000 0x100000>;
> +        compatible = "andestech,ax45mp-cache", "cache";
> +        interrupts = <508 IRQ_TYPE_LEVEL_HIGH>;
> +        cache-line-size = <64>;
> +        cache-level = <2>;
> +        cache-sets = <1024>;
> +        cache-size = <262144>;
> +        cache-unified;
> +        andestech,pma-regions = <0x58000000 0x08000000
> +                                 (AX45MP_PMACFG_ETYP_NAPOT | AX45MP_PMACFG_MTYP_MEM_NON_CACHE_BUF)>;
> +    };
> diff --git a/include/dt-bindings/cache/andestech,ax45mp-cache.h b/include/dt-bindings/cache/andestech,ax45mp-cache.h
> new file mode 100644
> index 000000000000..aa1cad24075d
> --- /dev/null
> +++ b/include/dt-bindings/cache/andestech,ax45mp-cache.h
> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> +/*
> + * This header provides constants for Andes AX45MP PMA configuration
> + *
> + * Copyright (C) 2022 Renesas Electronics Corp.
> + */
> +
> +#ifndef __DT_BINDINGS_ANDESTECH_AX45MP_CACHE_H
> +#define __DT_BINDINGS_ANDESTECH_AX45MP_CACHE_H
> +
> +/* OFF: PMA entry is disabled */
> +#define AX45MP_PMACFG_ETYP_DISABLED			0
> +/* Naturally aligned power of 2 region */
> +#define AX45MP_PMACFG_ETYP_NAPOT			3
> +
> +/* Device, Non-bufferable */
> +#define AX45MP_PMACFG_MTYP_DEV_NON_BUF			(0 << 2)
> +/* Device, bufferable */
> +#define AX45MP_PMACFG_MTYP_DEV_BUF			(1 << 2)
> +/* Memory, Non-cacheable, Non-bufferable */
> +#define AX45MP_PMACFG_MTYP_MEM_NON_CACHE_NON_BUF	(2 << 2)
> +/* Memory, Non-cacheable, Bufferable */
> +#define AX45MP_PMACFG_MTYP_MEM_NON_CACHE_BUF		(3 << 2)

What are all these? They don't look like flags, because 3 = 1 | 2...
they don't look like constants, because we do not use shifts in
constants. Are these some register values? I also do not see the header
being used in the code, so why having a bindings header if it is not
used (DTS is not usage...)?


> +/* Memory, Write-back, No-allocate */
> +#define AX45MP_PMACFG_MTYP_MEM_WB_NA			(8 << 2)
> +/* Memory, Write-back, Read-allocate */
> +#define AX45MP_PMACFG_MTYP_MEM_WB_RA			(9 << 2)
> +/* Memory, Write-back, Write-allocate */
> +#define AX45MP_PMACFG_MTYP_MEM_WB_WA			(10 << 2)
> +/* Memory, Write-back, Read and Write-allocate */
> +#define AX45MP_PMACFG_MTYP_MEM_WB_R_WA			(11 << 2)
> +
> +/* AMO instructions are supported */
> +#define AX45MP_PMACFG_NAMO_AMO_SUPPORT			(0 << 6)
> +/* AMO instructions are not supported */
> +#define AX45MP_PMACFG_NAMO_AMO_NO_SUPPORT		(1 << 6)
> +
> +#endif /* __DT_BINDINGS_ANDESTECH_AX45MP_CACHE_H */

Best regards,
Krzysztof
  

Hi Krzysztof,

Thank you for the review.

On Fri, Nov 25, 2022 at 8:16 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 24/11/2022 18:22, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Add DT binding documentation for L2 cache controller found on RZ/Five SoC.
> >
> > The Renesas RZ/Five microprocessor includes a RISC-V CPU Core (AX45MP
> > Single) from Andes. The AX45MP core has an L2 cache controller, this patch
> > describes the L2 cache block.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > RFC v3 -> v4
> > * Dropped l2 cache configuration parameters
> > * s/larger/large
> > * Added minItems/maxItems for andestech,pma-regions
> > ---
> >  .../cache/andestech,ax45mp-cache.yaml         | 93 +++++++++++++++++++
> >  .../cache/andestech,ax45mp-cache.h            | 38 ++++++++
> >  2 files changed, 131 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/cache/andestech,ax45mp-cache.yaml
> >  create mode 100644 include/dt-bindings/cache/andestech,ax45mp-cache.h
> >
> > diff --git a/Documentation/devicetree/bindings/cache/andestech,ax45mp-cache.yaml b/Documentation/devicetree/bindings/cache/andestech,ax45mp-cache.yaml
> > new file mode 100644
> > index 000000000000..bf255b177d0a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/cache/andestech,ax45mp-cache.yaml
> > @@ -0,0 +1,93 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +# Copyright (C) 2022 Renesas Electronics Corp.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/cache/andestech,ax45mp-cache.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Andestech AX45MP L2 Cache Controller
> > +
> > +maintainers:
> > +  - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > +
> > +description:
> > +  A level-2 cache (L2C) is used to improve the system performance by providing
> > +  a large amount of cache line entries and reasonable access delays. The L2C
> > +  is shared between cores, and a non-inclusive non-exclusive policy is used.
> > +
> > +select:
> > +  properties:
> > +    compatible:
> > +      contains:
> > +        enum:
> > +          - andestech,ax45mp-cache
> > +
> > +  required:
> > +    - compatible
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - const: andestech,ax45mp-cache
> > +      - const: cache
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  cache-line-size:
> > +    const: 64
> > +
> > +  cache-level:
> > +    const: 2
> > +
> > +  cache-sets:
> > +    const: 1024
> > +
> > +  cache-size:
> > +    enum: [131072, 262144, 524288, 1048576, 2097152]
> > +
> > +  cache-unified: true
> > +
> > +  next-level-cache: true
> > +
> > +  andestech,pma-regions:
> > +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > +    minItems: 1
> > +    maxItems: 16
> > +    items:
> > +      minItems: 3
> > +      maxItems: 3
>
> Instead:
> items:
>   items:
>     - description: Explain
>     - description: what is
>     - description: here
>
Ok, I will do that in the next version.

- description: Memory region offset to be set up in the PMA
- description: Size of the PMA region
- description: Flags indicating how the region should be set up in the
PMA. (ETYP[1:0] | MTYP[5:2]) use the macros
defined in include/dt-bindings/cache/andestech,ax45mp-cache.h.

> > +    description: Optional array of memory regions to be set in the PMA.
> > +
> > +additionalProperties: false
> > +
> > +required:
> > +  - compatible
> > +  - cache-line-size
> > +  - cache-level
> > +  - cache-sets
> > +  - cache-size
> > +  - cache-unified
> > +  - interrupts
> > +  - reg
>
> Keep the same order as properties appear in the "properties:"
>
Agreed, will do.


> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    #include <dt-bindings/cache/andestech,ax45mp-cache.h>
> > +
> > +    cache-controller@2010000 {
> > +        reg = <0x13400000 0x100000>;
> > +        compatible = "andestech,ax45mp-cache", "cache";
> > +        interrupts = <508 IRQ_TYPE_LEVEL_HIGH>;
> > +        cache-line-size = <64>;
> > +        cache-level = <2>;
> > +        cache-sets = <1024>;
> > +        cache-size = <262144>;
> > +        cache-unified;
> > +        andestech,pma-regions = <0x58000000 0x08000000
> > +                                 (AX45MP_PMACFG_ETYP_NAPOT | AX45MP_PMACFG_MTYP_MEM_NON_CACHE_BUF)>;
> > +    };
> > diff --git a/include/dt-bindings/cache/andestech,ax45mp-cache.h b/include/dt-bindings/cache/andestech,ax45mp-cache.h
> > new file mode 100644
> > index 000000000000..aa1cad24075d
> > --- /dev/null
> > +++ b/include/dt-bindings/cache/andestech,ax45mp-cache.h
> > @@ -0,0 +1,38 @@
> > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> > +/*
> > + * This header provides constants for Andes AX45MP PMA configuration
> > + *
> > + * Copyright (C) 2022 Renesas Electronics Corp.
> > + */
> > +
> > +#ifndef __DT_BINDINGS_ANDESTECH_AX45MP_CACHE_H
> > +#define __DT_BINDINGS_ANDESTECH_AX45MP_CACHE_H
> > +
> > +/* OFF: PMA entry is disabled */
> > +#define AX45MP_PMACFG_ETYP_DISABLED                  0
> > +/* Naturally aligned power of 2 region */
> > +#define AX45MP_PMACFG_ETYP_NAPOT                     3
> > +
> > +/* Device, Non-bufferable */
> > +#define AX45MP_PMACFG_MTYP_DEV_NON_BUF                       (0 << 2)
> > +/* Device, bufferable */
> > +#define AX45MP_PMACFG_MTYP_DEV_BUF                   (1 << 2)
> > +/* Memory, Non-cacheable, Non-bufferable */
> > +#define AX45MP_PMACFG_MTYP_MEM_NON_CACHE_NON_BUF     (2 << 2)
> > +/* Memory, Non-cacheable, Bufferable */
> > +#define AX45MP_PMACFG_MTYP_MEM_NON_CACHE_BUF         (3 << 2)
>
> What are all these? They don't look like flags, because 3 = 1 | 2...
> they don't look like constants, because we do not use shifts in
> constants. Are these some register values? I also do not see the header
> being used in the code, so why having a bindings header if it is not
> used (DTS is not usage...)?
>
These are register bit values for the MTYP[5:2] field. The DTS example
in the binding doc (above) uses these macros. I haven't included the
DTS/I patches with this patchset yet do think I should?

Cheers,
Prabhakar
  

, Hi Prabhakar,

On Fri, Nov 25, 2022 at 11:34 AM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> On Fri, Nov 25, 2022 at 8:16 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
> > On 24/11/2022 18:22, Prabhakar wrote:
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >
> > > Add DT binding documentation for L2 cache controller found on RZ/Five SoC.
> > >
> > > The Renesas RZ/Five microprocessor includes a RISC-V CPU Core (AX45MP
> > > Single) from Andes. The AX45MP core has an L2 cache controller, this patch
> > > describes the L2 cache block.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > > RFC v3 -> v4
> > > * Dropped l2 cache configuration parameters
> > > * s/larger/large
> > > * Added minItems/maxItems for andestech,pma-regions

> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/cache/andestech,ax45mp-cache.yaml

> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/interrupt-controller/irq.h>
> > > +    #include <dt-bindings/cache/andestech,ax45mp-cache.h>
> > > +
> > > +    cache-controller@2010000 {
> > > +        reg = <0x13400000 0x100000>;
> > > +        compatible = "andestech,ax45mp-cache", "cache";
> > > +        interrupts = <508 IRQ_TYPE_LEVEL_HIGH>;
> > > +        cache-line-size = <64>;
> > > +        cache-level = <2>;
> > > +        cache-sets = <1024>;
> > > +        cache-size = <262144>;
> > > +        cache-unified;
> > > +        andestech,pma-regions = <0x58000000 0x08000000
> > > +                                 (AX45MP_PMACFG_ETYP_NAPOT | AX45MP_PMACFG_MTYP_MEM_NON_CACHE_BUF)>;
> > > +    };
> > > diff --git a/include/dt-bindings/cache/andestech,ax45mp-cache.h b/include/dt-bindings/cache/andestech,ax45mp-cache.h
> > > new file mode 100644
> > > index 000000000000..aa1cad24075d
> > > --- /dev/null
> > > +++ b/include/dt-bindings/cache/andestech,ax45mp-cache.h
> > > @@ -0,0 +1,38 @@
> > > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> > > +/*
> > > + * This header provides constants for Andes AX45MP PMA configuration
> > > + *
> > > + * Copyright (C) 2022 Renesas Electronics Corp.
> > > + */
> > > +
> > > +#ifndef __DT_BINDINGS_ANDESTECH_AX45MP_CACHE_H
> > > +#define __DT_BINDINGS_ANDESTECH_AX45MP_CACHE_H
> > > +
> > > +/* OFF: PMA entry is disabled */
> > > +#define AX45MP_PMACFG_ETYP_DISABLED                  0
> > > +/* Naturally aligned power of 2 region */
> > > +#define AX45MP_PMACFG_ETYP_NAPOT                     3
> > > +
> > > +/* Device, Non-bufferable */
> > > +#define AX45MP_PMACFG_MTYP_DEV_NON_BUF                       (0 << 2)
> > > +/* Device, bufferable */
> > > +#define AX45MP_PMACFG_MTYP_DEV_BUF                   (1 << 2)
> > > +/* Memory, Non-cacheable, Non-bufferable */
> > > +#define AX45MP_PMACFG_MTYP_MEM_NON_CACHE_NON_BUF     (2 << 2)
> > > +/* Memory, Non-cacheable, Bufferable */
> > > +#define AX45MP_PMACFG_MTYP_MEM_NON_CACHE_BUF         (3 << 2)
> >
> > What are all these? They don't look like flags, because 3 = 1 | 2...
> > they don't look like constants, because we do not use shifts in
> > constants. Are these some register values? I also do not see the header
> > being used in the code, so why having a bindings header if it is not
> > used (DTS is not usage...)?
> >
> These are register bit values for the MTYP[5:2] field. The DTS example
> in the binding doc (above) uses these macros. I haven't included the
> DTS/I patches with this patchset yet do think I should?

I think the main objection from Rob is that these look too much like
raw register values to be written unchanged to registers, which is
frowned upon in DT.

Now, can we make this more generic?

  1. Do you need AX45MP_PMACFG_ETYP_DISABLED, i.e. will it ever be
     specified in DTS, or is this a pure software thing?
  2. Obviously you can let the driver decide if AX45MP_PMACFG_ETYP_NAPOT
     can be set, based on address/size?
  3. If the two above are removed, the shifts can be handled in the
     driver instead,
  4. Are there existing (more generic) definitions that can be used
     instead?

BTW, what's the difference between non-bufferable and non-cacheable?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
  

Hi Geert,

On Fri, Nov 25, 2022 at 11:18 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> , Hi Prabhakar,
>
> On Fri, Nov 25, 2022 at 11:34 AM Lad, Prabhakar
> <prabhakar.csengg@gmail.com> wrote:
> > On Fri, Nov 25, 2022 at 8:16 AM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> > > On 24/11/2022 18:22, Prabhakar wrote:
> > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > >
> > > > Add DT binding documentation for L2 cache controller found on RZ/Five SoC.
> > > >
> > > > The Renesas RZ/Five microprocessor includes a RISC-V CPU Core (AX45MP
> > > > Single) from Andes. The AX45MP core has an L2 cache controller, this patch
> > > > describes the L2 cache block.
> > > >
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > ---
> > > > RFC v3 -> v4
> > > > * Dropped l2 cache configuration parameters
> > > > * s/larger/large
> > > > * Added minItems/maxItems for andestech,pma-regions
>
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/cache/andestech,ax45mp-cache.yaml
>
> > > > +examples:
> > > > +  - |
> > > > +    #include <dt-bindings/interrupt-controller/irq.h>
> > > > +    #include <dt-bindings/cache/andestech,ax45mp-cache.h>
> > > > +
> > > > +    cache-controller@2010000 {
> > > > +        reg = <0x13400000 0x100000>;
> > > > +        compatible = "andestech,ax45mp-cache", "cache";
> > > > +        interrupts = <508 IRQ_TYPE_LEVEL_HIGH>;
> > > > +        cache-line-size = <64>;
> > > > +        cache-level = <2>;
> > > > +        cache-sets = <1024>;
> > > > +        cache-size = <262144>;
> > > > +        cache-unified;
> > > > +        andestech,pma-regions = <0x58000000 0x08000000
> > > > +                                 (AX45MP_PMACFG_ETYP_NAPOT | AX45MP_PMACFG_MTYP_MEM_NON_CACHE_BUF)>;
> > > > +    };
> > > > diff --git a/include/dt-bindings/cache/andestech,ax45mp-cache.h b/include/dt-bindings/cache/andestech,ax45mp-cache.h
> > > > new file mode 100644
> > > > index 000000000000..aa1cad24075d
> > > > --- /dev/null
> > > > +++ b/include/dt-bindings/cache/andestech,ax45mp-cache.h
> > > > @@ -0,0 +1,38 @@
> > > > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> > > > +/*
> > > > + * This header provides constants for Andes AX45MP PMA configuration
> > > > + *
> > > > + * Copyright (C) 2022 Renesas Electronics Corp.
> > > > + */
> > > > +
> > > > +#ifndef __DT_BINDINGS_ANDESTECH_AX45MP_CACHE_H
> > > > +#define __DT_BINDINGS_ANDESTECH_AX45MP_CACHE_H
> > > > +
> > > > +/* OFF: PMA entry is disabled */
> > > > +#define AX45MP_PMACFG_ETYP_DISABLED                  0
> > > > +/* Naturally aligned power of 2 region */
> > > > +#define AX45MP_PMACFG_ETYP_NAPOT                     3
> > > > +
> > > > +/* Device, Non-bufferable */
> > > > +#define AX45MP_PMACFG_MTYP_DEV_NON_BUF                       (0 << 2)
> > > > +/* Device, bufferable */
> > > > +#define AX45MP_PMACFG_MTYP_DEV_BUF                   (1 << 2)
> > > > +/* Memory, Non-cacheable, Non-bufferable */
> > > > +#define AX45MP_PMACFG_MTYP_MEM_NON_CACHE_NON_BUF     (2 << 2)
> > > > +/* Memory, Non-cacheable, Bufferable */
> > > > +#define AX45MP_PMACFG_MTYP_MEM_NON_CACHE_BUF         (3 << 2)
> > >
> > > What are all these? They don't look like flags, because 3 = 1 | 2...
> > > they don't look like constants, because we do not use shifts in
> > > constants. Are these some register values? I also do not see the header
> > > being used in the code, so why having a bindings header if it is not
> > > used (DTS is not usage...)?
> > >
> > These are register bit values for the MTYP[5:2] field. The DTS example
> > in the binding doc (above) uses these macros. I haven't included the
> > DTS/I patches with this patchset yet do think I should?
>
> I think the main objection from Rob is that these look too much like
> raw register values to be written unchanged to registers, which is
> frowned upon in DT.
>
> Now, can we make this more generic?
>
>   1. Do you need AX45MP_PMACFG_ETYP_DISABLED, i.e. will it ever be
>      specified in DTS, or is this a pure software thing?
>   2. Obviously you can let the driver decide if AX45MP_PMACFG_ETYP_NAPOT
>      can be set, based on address/size?
>   3. If the two above are removed, the shifts can be handled in the
>      driver instead,
Yes we can get rid of AX45MP_PMACFG_ETYP_DISABLED and
AX45MP_PMACFG_ETYP_NAPOT. If we are setting up the PMA region it
always has to be a power of 2. So AX45MP_PMACFG_ETYP_NAPOT can be
passed either from the driver or in the OpenSBI just OR it while
setting up the PMA.


>   4. Are there existing (more generic) definitions that can be used
>      instead?
>
You mean for the MTYP flags? I haven't come across any in the kernel.

> BTW, what's the difference between non-bufferable and non-cacheable?
>
non-cacheable, from the Andes manual: Accessing to non-cacheable
memory and device will bypass I-Cache, D-Cache and L2-Cache no matter
the data is cached or not. The cache states are not changed by these
accesses.

TBH I dont have a clear answer for non-bufferable nor do we have in
the Andes HW manual. I'll get the details from Andes.

Cheers,
Prabhakar
  

On 25/11/2022 11:34, Lad, Prabhakar wrote:
>>> +/* Device, Non-bufferable */
>>> +#define AX45MP_PMACFG_MTYP_DEV_NON_BUF                       (0 << 2)
>>> +/* Device, bufferable */
>>> +#define AX45MP_PMACFG_MTYP_DEV_BUF                   (1 << 2)
>>> +/* Memory, Non-cacheable, Non-bufferable */
>>> +#define AX45MP_PMACFG_MTYP_MEM_NON_CACHE_NON_BUF     (2 << 2)
>>> +/* Memory, Non-cacheable, Bufferable */
>>> +#define AX45MP_PMACFG_MTYP_MEM_NON_CACHE_BUF         (3 << 2)
>>
>> What are all these? They don't look like flags, because 3 = 1 | 2...
>> they don't look like constants, because we do not use shifts in
>> constants. Are these some register values? I also do not see the header
>> being used in the code, so why having a bindings header if it is not
>> used (DTS is not usage...)?
>>
> These are register bit values for the MTYP[5:2] field. The DTS example
> in the binding doc (above) uses these macros. I haven't included the
> DTS/I patches with this patchset yet do think I should?

Then why storing it as bindings? Bindings headers describe the interface
implemented by drivers and used by DTS, but this is not implemented by
drivers.

Best regards,
Krzysztof
  

On Fri, Nov 25, 2022 at 01:12:18PM +0100, Krzysztof Kozlowski wrote:
> On 25/11/2022 11:34, Lad, Prabhakar wrote:
> >>> +/* Device, Non-bufferable */
> >>> +#define AX45MP_PMACFG_MTYP_DEV_NON_BUF                       (0 << 2)
> >>> +/* Device, bufferable */
> >>> +#define AX45MP_PMACFG_MTYP_DEV_BUF                   (1 << 2)
> >>> +/* Memory, Non-cacheable, Non-bufferable */
> >>> +#define AX45MP_PMACFG_MTYP_MEM_NON_CACHE_NON_BUF     (2 << 2)
> >>> +/* Memory, Non-cacheable, Bufferable */
> >>> +#define AX45MP_PMACFG_MTYP_MEM_NON_CACHE_BUF         (3 << 2)
> >>
> >> What are all these? They don't look like flags, because 3 = 1 | 2...
> >> they don't look like constants, because we do not use shifts in
> >> constants. Are these some register values? I also do not see the header
> >> being used in the code, so why having a bindings header if it is not
> >> used (DTS is not usage...)?
> >>
> > These are register bit values for the MTYP[5:2] field. The DTS example
> > in the binding doc (above) uses these macros. I haven't included the
> > DTS/I patches with this patchset yet do think I should?
> 
> Then why storing it as bindings? Bindings headers describe the interface
> implemented by drivers and used by DTS, but this is not implemented by
> drivers.

IIUC, some of these properties are non-discoverable attributes of the
cache controller. I see two things that could be done here that are
"better" than #defining bits:
- add an RZ/Five specific compatible and use match data to set the
  attributes which is only possible if the pma-regions are set on a
  per SoC basis
- make pma-regions into a child node, in which andestech,non-cacheable
  andestech,non-bufferable etc are properties of the child node

Prabhakar, does that make sense or am I off with my understanding of the
attributes?

Thanks,
Conor.
  

Hi Conor,

On Fri, Nov 25, 2022 at 12:25 PM Conor Dooley
<conor.dooley@microchip.com> wrote:
>
> On Fri, Nov 25, 2022 at 01:12:18PM +0100, Krzysztof Kozlowski wrote:
> > On 25/11/2022 11:34, Lad, Prabhakar wrote:
> > >>> +/* Device, Non-bufferable */
> > >>> +#define AX45MP_PMACFG_MTYP_DEV_NON_BUF                       (0 << 2)
> > >>> +/* Device, bufferable */
> > >>> +#define AX45MP_PMACFG_MTYP_DEV_BUF                   (1 << 2)
> > >>> +/* Memory, Non-cacheable, Non-bufferable */
> > >>> +#define AX45MP_PMACFG_MTYP_MEM_NON_CACHE_NON_BUF     (2 << 2)
> > >>> +/* Memory, Non-cacheable, Bufferable */
> > >>> +#define AX45MP_PMACFG_MTYP_MEM_NON_CACHE_BUF         (3 << 2)
> > >>
> > >> What are all these? They don't look like flags, because 3 = 1 | 2...
> > >> they don't look like constants, because we do not use shifts in
> > >> constants. Are these some register values? I also do not see the header
> > >> being used in the code, so why having a bindings header if it is not
> > >> used (DTS is not usage...)?
> > >>
> > > These are register bit values for the MTYP[5:2] field. The DTS example
> > > in the binding doc (above) uses these macros. I haven't included the
> > > DTS/I patches with this patchset yet do think I should?
> >
> > Then why storing it as bindings? Bindings headers describe the interface
> > implemented by drivers and used by DTS, but this is not implemented by
> > drivers.
>
> IIUC, some of these properties are non-discoverable attributes of the
> cache controller. I see two things that could be done here that are
> "better" than #defining bits:
> - add an RZ/Five specific compatible and use match data to set the
>   attributes which is only possible if the pma-regions are set on a
>   per SoC basis
> - make pma-regions into a child node, in which andestech,non-cacheable
>   andestech,non-bufferable etc are properties of the child node
>
For now the only way to get DMA working without IOCP is to have
AX45MP_PMACFG_MTYP_MEM_NON_CACHE_BUF. But for future purposes I have
introduced the other available flags.

So maybe for now we could just have this flag
andestech,mem-non-cacheable-bufferable in the binding doc.

    cache-controller@2010000 {
        reg = <0x13400000 0x100000>;
        compatible = "andestech,ax45mp-cache", "cache";
        interrupts = <508 IRQ_TYPE_LEVEL_HIGH>;
        cache-line-size = <64>;
        cache-level = <2>;
        cache-sets = <1024>;
        cache-size = <262144>;
        cache-unified;
        andestech,pma-region@0x58000000 {
            reg = <0x58000000 0x08000000>;
            andestech,mem-non-cacheable-bufferable;
        };
        andestech,pma-region@0xdeadbeef {
            reg = <0xdeadbeef 0x08000000>;
            andestech,mem-non-cacheable-bufferable;
        };
        ....
    };

Did I chime in this time?

Cheers,
Prabhakar
  

On Fri, Nov 25, 2022 at 12:51:34PM +0000, Lad, Prabhakar wrote:
> Hi Conor,
> 
> On Fri, Nov 25, 2022 at 12:25 PM Conor Dooley
> <conor.dooley@microchip.com> wrote:
> >
> > On Fri, Nov 25, 2022 at 01:12:18PM +0100, Krzysztof Kozlowski wrote:
> > > On 25/11/2022 11:34, Lad, Prabhakar wrote:
> > > >>> +/* Device, Non-bufferable */
> > > >>> +#define AX45MP_PMACFG_MTYP_DEV_NON_BUF                       (0 << 2)
> > > >>> +/* Device, bufferable */
> > > >>> +#define AX45MP_PMACFG_MTYP_DEV_BUF                   (1 << 2)
> > > >>> +/* Memory, Non-cacheable, Non-bufferable */
> > > >>> +#define AX45MP_PMACFG_MTYP_MEM_NON_CACHE_NON_BUF     (2 << 2)
> > > >>> +/* Memory, Non-cacheable, Bufferable */
> > > >>> +#define AX45MP_PMACFG_MTYP_MEM_NON_CACHE_BUF         (3 << 2)
> > > >>
> > > >> What are all these? They don't look like flags, because 3 = 1 | 2...
> > > >> they don't look like constants, because we do not use shifts in
> > > >> constants. Are these some register values? I also do not see the header
> > > >> being used in the code, so why having a bindings header if it is not
> > > >> used (DTS is not usage...)?
> > > >>
> > > > These are register bit values for the MTYP[5:2] field. The DTS example
> > > > in the binding doc (above) uses these macros. I haven't included the
> > > > DTS/I patches with this patchset yet do think I should?
> > >
> > > Then why storing it as bindings? Bindings headers describe the interface
> > > implemented by drivers and used by DTS, but this is not implemented by
> > > drivers.
> >
> > IIUC, some of these properties are non-discoverable attributes of the
> > cache controller. I see two things that could be done here that are
> > "better" than #defining bits:
> > - add an RZ/Five specific compatible and use match data to set the
> >   attributes which is only possible if the pma-regions are set on a
> >   per SoC basis
> > - make pma-regions into a child node, in which andestech,non-cacheable
> >   andestech,non-bufferable etc are properties of the child node
> >
> For now the only way to get DMA working without IOCP is to have
> AX45MP_PMACFG_MTYP_MEM_NON_CACHE_BUF. But for future purposes I have
> introduced the other available flags.
> 
> So maybe for now we could just have this flag
> andestech,mem-non-cacheable-bufferable in the binding doc.
> 
>     cache-controller@2010000 {
>         reg = <0x13400000 0x100000>;
>         compatible = "andestech,ax45mp-cache", "cache";
>         interrupts = <508 IRQ_TYPE_LEVEL_HIGH>;
>         cache-line-size = <64>;
>         cache-level = <2>;
>         cache-sets = <1024>;
>         cache-size = <262144>;
>         cache-unified;
>         andestech,pma-region@0x58000000 {
>             reg = <0x58000000 0x08000000>;
>             andestech,mem-non-cacheable-bufferable;

Yah, that's about what I would expect - except splitting the properties
up. I think split up makes more sense from a property description point
of view, rather than needing some sort of
oneOf:
  - non-cacheable-bufferable
  - cacheable-non-bufferable
  - non-cacheable-non-bufferable


>         };
>         andestech,pma-region@0xdeadbeef {
>             reg = <0xdeadbeef 0x08000000>;
>             andestech,mem-non-cacheable-bufferable;
>         };
>         ....
>     };
  

On 25/11/2022 13:25, Conor Dooley wrote:
> On Fri, Nov 25, 2022 at 01:12:18PM +0100, Krzysztof Kozlowski wrote:
>> On 25/11/2022 11:34, Lad, Prabhakar wrote:
>>>>> +/* Device, Non-bufferable */
>>>>> +#define AX45MP_PMACFG_MTYP_DEV_NON_BUF                       (0 << 2)
>>>>> +/* Device, bufferable */
>>>>> +#define AX45MP_PMACFG_MTYP_DEV_BUF                   (1 << 2)
>>>>> +/* Memory, Non-cacheable, Non-bufferable */
>>>>> +#define AX45MP_PMACFG_MTYP_MEM_NON_CACHE_NON_BUF     (2 << 2)
>>>>> +/* Memory, Non-cacheable, Bufferable */
>>>>> +#define AX45MP_PMACFG_MTYP_MEM_NON_CACHE_BUF         (3 << 2)
>>>>
>>>> What are all these? They don't look like flags, because 3 = 1 | 2...
>>>> they don't look like constants, because we do not use shifts in
>>>> constants. Are these some register values? I also do not see the header
>>>> being used in the code, so why having a bindings header if it is not
>>>> used (DTS is not usage...)?
>>>>
>>> These are register bit values for the MTYP[5:2] field. The DTS example
>>> in the binding doc (above) uses these macros. I haven't included the
>>> DTS/I patches with this patchset yet do think I should?
>>
>> Then why storing it as bindings? Bindings headers describe the interface
>> implemented by drivers and used by DTS, but this is not implemented by
>> drivers.
> 
> IIUC, some of these properties are non-discoverable attributes of the
> cache controller. I see two things that could be done here that are
> "better" than #defining bits:

I did not comment about properties. I comment about constants. Why
register values/offsets/addresses are in this particular case suitable
for binding headers?

> - add an RZ/Five specific compatible and use match data to set the
>   attributes which is only possible if the pma-regions are set on a
>   per SoC basis
> - make pma-regions into a child node, in which andestech,non-cacheable
>   andestech,non-bufferable etc are properties of the child node
> 
> Prabhakar, does that make sense or am I off with my understanding of the
> attributes?


Best regards,
Krzysztof
  

On Fri, Nov 25, 2022 at 04:55:11PM +0100, Krzysztof Kozlowski wrote:
> On 25/11/2022 13:25, Conor Dooley wrote:
> > On Fri, Nov 25, 2022 at 01:12:18PM +0100, Krzysztof Kozlowski wrote:
> >> On 25/11/2022 11:34, Lad, Prabhakar wrote:
> >>>>> +/* Device, Non-bufferable */
> >>>>> +#define AX45MP_PMACFG_MTYP_DEV_NON_BUF                       (0 << 2)
> >>>>> +/* Device, bufferable */
> >>>>> +#define AX45MP_PMACFG_MTYP_DEV_BUF                   (1 << 2)
> >>>>> +/* Memory, Non-cacheable, Non-bufferable */
> >>>>> +#define AX45MP_PMACFG_MTYP_MEM_NON_CACHE_NON_BUF     (2 << 2)
> >>>>> +/* Memory, Non-cacheable, Bufferable */
> >>>>> +#define AX45MP_PMACFG_MTYP_MEM_NON_CACHE_BUF         (3 << 2)
> >>>>
> >>>> What are all these? They don't look like flags, because 3 = 1 | 2...
> >>>> they don't look like constants, because we do not use shifts in
> >>>> constants. Are these some register values? I also do not see the header
> >>>> being used in the code, so why having a bindings header if it is not
> >>>> used (DTS is not usage...)?
> >>>>
> >>> These are register bit values for the MTYP[5:2] field. The DTS example
> >>> in the binding doc (above) uses these macros. I haven't included the
> >>> DTS/I patches with this patchset yet do think I should?
> >>
> >> Then why storing it as bindings? Bindings headers describe the interface
> >> implemented by drivers and used by DTS, but this is not implemented by
> >> drivers.
> > 
> > IIUC, some of these properties are non-discoverable attributes of the
> > cache controller. I see two things that could be done here that are
> > "better" than #defining bits:
> 
> I did not comment about properties. I comment about constants. Why
> register values/offsets/addresses are in this particular case suitable
> for binding headers?

I don't think we disagree here. I'm not in favour of the defines either
here. Perhaps I confused you by accidentally not adding Prabhakar to the
to field.

The dt needs to convey his particular cache implementation's bufferable
and/or coherent regions so I was suggesting alternatives for conveying
this information, without resorting to defines.

> > - add an RZ/Five specific compatible and use match data to set the
> >   attributes which is only possible if the pma-regions are set on a
> >   per SoC basis
> > - make pma-regions into a child node, in which andestech,non-cacheable
> >   andestech,non-bufferable etc are properties of the child node
  

Hi Krzysztof,

On Fri, Nov 25, 2022 at 12:12 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 25/11/2022 11:34, Lad, Prabhakar wrote:
> >>> +/* Device, Non-bufferable */
> >>> +#define AX45MP_PMACFG_MTYP_DEV_NON_BUF                       (0 << 2)
> >>> +/* Device, bufferable */
> >>> +#define AX45MP_PMACFG_MTYP_DEV_BUF                   (1 << 2)
> >>> +/* Memory, Non-cacheable, Non-bufferable */
> >>> +#define AX45MP_PMACFG_MTYP_MEM_NON_CACHE_NON_BUF     (2 << 2)
> >>> +/* Memory, Non-cacheable, Bufferable */
> >>> +#define AX45MP_PMACFG_MTYP_MEM_NON_CACHE_BUF         (3 << 2)
> >>
> >> What are all these? They don't look like flags, because 3 = 1 | 2...
> >> they don't look like constants, because we do not use shifts in
> >> constants. Are these some register values? I also do not see the header
> >> being used in the code, so why having a bindings header if it is not
> >> used (DTS is not usage...)?
> >>
> > These are register bit values for the MTYP[5:2] field. The DTS example
> > in the binding doc (above) uses these macros. I haven't included the
> > DTS/I patches with this patchset yet do think I should?
>
> Then why storing it as bindings? Bindings headers describe the interface
> implemented by drivers and used by DTS, but this is not implemented by
> drivers.
>
I got your point. I'll make use of the header in the driver for the
next version and fix your previously pointed comments.

Cheers,
Prabhakar