Re: [PATCH 3/3] dt-bindings: riscv: sifive: Add SiFive Private L2 cache controller

[Eric Lin]

Hi Conor,

On Fri, Jun 16, 2023 at 6:12 PM Conor Dooley <conor.dooley@xxxxxxxxxxxxx> wrote:
>
> Hey Eric,
>
> On Fri, Jun 16, 2023 at 02:32:10PM +0800, Eric Lin wrote:
> > This add YAML DT binding documentation for SiFive Private L2
> > cache controller
> >
> > Signed-off-by: Eric Lin <eric.lin@xxxxxxxxxx>
> > Reviewed-by: Zong Li <zong.li@xxxxxxxxxx>
> > Reviewed-by: Nick Hu <nick.hu@xxxxxxxxxx>
>
> Firstly, bindings need to come before the driver using them.
>

OK, I'll fix it in v2.

> > ---
> >  .../bindings/riscv/sifive,pL2Cache0.yaml      | 81 +++++++++++++++++++
> >  1 file changed, 81 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml b/Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml
> > new file mode 100644
> > index 000000000000..b5d8d4a39dde
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/riscv/sifive,pL2Cache0.yaml
>
> Cache bindings have moved to devicetree/bindings/cache.
>
> > @@ -0,0 +1,81 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +# Copyright (C) 2023 SiFive, Inc.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/riscv/sifive,pL2Cache0.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: SiFive Private L2 Cache Controller
> > +
> > +maintainers:
> > +  - Greentime Hu  <greentime.hu@xxxxxxxxxx>
> > +  - Eric Lin      <eric.lin@xxxxxxxxxx>
>
> Drop the alignment here please.
>

OK, I'll fix it in v2.

> > +
> > +description:
> > +  The SiFive Private L2 Cache Controller is per hart and communicates with both the upstream
> > +  L1 caches and downstream L3 cache or memory, enabling a high-performance cache subsystem.
> > +  All the properties in ePAPR/DeviceTree specification applies for this platform.
>
> Please wrap before 80 characters.
>

OK, I'll fix it in v2.

> > +
> > +allOf:
> > +  - $ref: /schemas/cache-controller.yaml#
> > +
> > +select:
> > +  properties:
> > +    compatible:
> > +      contains:
> > +        enum:
> > +          - sifive,pL2Cache0
> > +          - sifive,pL2Cache1
>
> Why is this select: required?
>
OK, I'll fix it in v2.

> > +  required:
> > +    - compatible
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - sifive,pL2Cache0
> > +          - sifive,pL2Cache1
>
> What is the difference between these? (and drop the caps please)

The pL2Cache1 has minor changes in hardware, but it can use the same
pl2 cache driver.
OK, I'll drop the caps in v2.

>
> Should this also not fall back to "cache"?
>
Yes,  I'll fix it in v2.

> > +
> > +  cache-block-size:
> > +    const: 64
> > +
> > +  cache-level:
> > +    const: 2
> > +
> > +  cache-sets:
> > +    const: 512
> > +
> > +  cache-size:
> > +    const: 262144
> > +
> > +  cache-unified: true
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  next-level-cache: true
> > +
> > +additionalProperties: false
> > +
> > +required:
> > +  - compatible
> > +  - cache-block-size
> > +  - cache-level
> > +  - cache-sets
> > +  - cache-size
> > +  - cache-unified
> > +  - reg
> > +
> > +examples:
> > +  - |
> > +    pl2@10104000 {
>
> cache-controller@
>

OK, I'll fix it in v2.
Thanks for the review.

Best Regards,
Eric Lin.

> Cheers,
> Conor.
>
> > +        compatible = "sifive,pL2Cache0";
> > +        cache-block-size = <64>;
> > +        cache-level = <2>;
> > +        cache-sets = <512>;
> > +        cache-size = <262144>;
> > +        cache-unified;
> > +        reg = <0x10104000 0x4000>;
> > +        next-level-cache = <&L4>;
> > +    };
> > --
> > 2.40.1
> >