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

[Lad, Prabhakar]

Hi Geert,

On Fri, Nov 25, 2022 at 11:18 AM Geert Uytterhoeven
<geert@xxxxxxxxxxxxxx> wrote:
>
> , Hi Prabhakar,
>
> On Fri, Nov 25, 2022 at 11:34 AM Lad, Prabhakar
> <prabhakar.csengg@xxxxxxxxx> wrote:
> > On Fri, Nov 25, 2022 at 8:16 AM Krzysztof Kozlowski
> > <krzysztof.kozlowski@xxxxxxxxxx> wrote:
> > > On 24/11/2022 18:22, Prabhakar wrote:
> > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxxxxxx>
> > > > ---
> > > > 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