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

[Geert Uytterhoeven]

, 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,
  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@xxxxxxxxxxxxxx

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