| Message ID | 20221124172207.153718-7-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | AX45MP: Add support to non-coherent DMA | expand |
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
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 + 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 + +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) +/* 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 */