| Message ID | 20230616063210.19063-4-eric.lin@sifive.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Conor Dooley |
| Headers | show |
| Series | Add SiFive Private L2 cache and PMU driver | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/cover_letter | success | Series has a cover letter |
| conchuod/tree_selection | success | Guessed tree name to be for-next at HEAD d5e45e810e0e |
| conchuod/fixes_present | success | Fixes tag not required for -next series |
| conchuod/maintainers_pattern | success | MAINTAINERS pattern errors before the patch: 6 and now 6 |
| conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
| conchuod/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/build_rv64_clang_allmodconfig | success | Errors and warnings before: 8 this patch: 8 |
| conchuod/module_param | success | Was 0 now: 0 |
| conchuod/build_rv64_gcc_allmodconfig | success | Errors and warnings before: 8 this patch: 8 |
| conchuod/build_rv32_defconfig | success | Build OK |
| conchuod/dtb_warn_rv64 | success | Errors and warnings before: 3 this patch: 3 |
| conchuod/header_inline | success | No static functions without inline keyword in header files |
| conchuod/checkpatch | warning | WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? |
| conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
| conchuod/verify_fixes | success | No Fixes tag |
| conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
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@sifive.com> > Reviewed-by: Zong Li <zong.li@sifive.com> > Reviewed-by: Nick Hu <nick.hu@sifive.com> Firstly, bindings need to come before the driver using them. > --- > .../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@sifive.com> > + - Eric Lin <eric.lin@sifive.com> Drop the alignment here please. > + > +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. > + > +allOf: > + - $ref: /schemas/cache-controller.yaml# > + > +select: > + properties: > + compatible: > + contains: > + enum: > + - sifive,pL2Cache0 > + - sifive,pL2Cache1 Why is this select: required? > + required: > + - compatible > + > +properties: > + compatible: > + items: > + - enum: > + - sifive,pL2Cache0 > + - sifive,pL2Cache1 What is the difference between these? (and drop the caps please) Should this also not fall back to "cache"? > + > + 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@ 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 >
On 16/06/2023 11:10, Conor Dooley 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@sifive.com> >> Reviewed-by: Zong Li <zong.li@sifive.com> >> Reviewed-by: Nick Hu <nick.hu@sifive.com> > > Firstly, bindings need to come before the driver using them. > >> --- >> .../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@sifive.com> >> + - Eric Lin <eric.lin@sifive.com> > > Drop the alignment here please. > >> + >> +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. > >> + >> +allOf: >> + - $ref: /schemas/cache-controller.yaml# >> + >> +select: >> + properties: >> + compatible: >> + contains: >> + enum: >> + - sifive,pL2Cache0 >> + - sifive,pL2Cache1 > > Why is this select: required? > >> + required: >> + - compatible >> + >> +properties: >> + compatible: >> + items: >> + - enum: >> + - sifive,pL2Cache0 >> + - sifive,pL2Cache1 > > What is the difference between these? (and drop the caps please) > > Should this also not fall back to "cache"? I thought cache is required as the last resort. Ben Dooks http://www.codethink.co.uk/ Senior Engineer Codethink - Providing Genius https://www.codethink.co.uk/privacy.html
On 16/06/2023 08:32, Eric Lin wrote: > This add YAML DT binding documentation for SiFive Private L2 > cache controller > > Signed-off-by: Eric Lin <eric.lin@sifive.com> > Reviewed-by: Zong Li <zong.li@sifive.com> > Reviewed-by: Nick Hu <nick.hu@sifive.com> > --- > .../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 > @@ -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@sifive.com> > + - Eric Lin <eric.lin@sifive.com> > + > +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. Drop the last sentence. Why specification would not apply? > + > +allOf: > + - $ref: /schemas/cache-controller.yaml# > + > +select: > + properties: > + compatible: > + contains: > + enum: > + - sifive,pL2Cache0 > + - sifive,pL2Cache > + > + required: > + - compatible > + > +properties: > + compatible: > + items: You have only one item, so no need for items... unless you just missed proper fallback. > + - enum: > + - sifive,pL2Cache0 > + - sifive,pL2Cache1 What is "0" and "1" here? What do these compatibles represent? Why they do not have any SoC related part? > + > + cache-block-size: > + const: 64 > + > + cache-level: > + const: 2 > + > + cache-sets: > + const: 512 > + > + cache-size: > + const: 262144 Are you sure? So all private L2 cache controllers will have fixed size of cache? > + > + cache-unified: true > + > + reg: > + maxItems: 1 > + > + next-level-cache: true > + > +additionalProperties: false > + > +required: required: goes before additionalProperties:. Best regards, Krzysztof
Hi Conor, On Fri, Jun 16, 2023 at 6:12 PM Conor Dooley <conor.dooley@microchip.com> 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@sifive.com> > > Reviewed-by: Zong Li <zong.li@sifive.com> > > Reviewed-by: Nick Hu <nick.hu@sifive.com> > > 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@sifive.com> > > + - Eric Lin <eric.lin@sifive.com> > > 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 > >
Hi Krzysztof, On Fri, Jun 16, 2023 at 6:45 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 16/06/2023 08:32, Eric Lin wrote: > > This add YAML DT binding documentation for SiFive Private L2 > > cache controller > > > > Signed-off-by: Eric Lin <eric.lin@sifive.com> > > Reviewed-by: Zong Li <zong.li@sifive.com> > > Reviewed-by: Nick Hu <nick.hu@sifive.com> > > --- > > .../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 > > @@ -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@sifive.com> > > + - Eric Lin <eric.lin@sifive.com> > > + > > +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. > > Drop the last sentence. Why specification would not apply? > OK, I'll drop it in v2. > > + > > +allOf: > > + - $ref: /schemas/cache-controller.yaml# > > + > > +select: > > + properties: > > + compatible: > > + contains: > > + enum: > > + - sifive,pL2Cache0 > > + - sifive,pL2Cache > > + > > + required: > > + - compatible > > + > > +properties: > > + compatible: > > + items: > > > You have only one item, so no need for items... unless you just missed > proper fallback. OK, I'll fix it in v2. > > > + - enum: > > + - sifive,pL2Cache0 > > + - sifive,pL2Cache1 > > What is "0" and "1" here? What do these compatibles represent? Why they > do not have any SoC related part? The pL2Cache1 has minor changes in hardware, but it can use the same pl2 cache driver. May I ask, what do you mean about the SoC-related part? Thanks. > > > + > > + cache-block-size: > > + const: 64 > > + > > + cache-level: > > + const: 2 > > + > > + cache-sets: > > + const: 512 > > + > > + cache-size: > > + const: 262144 > > Are you sure? So all private L2 cache controllers will have fixed size > of cache? The private L2 cache controllers will have different sizes. OK, I'll fix in v2. > > > + > > + cache-unified: true > > + > > + reg: > > + maxItems: 1 > > + > > + next-level-cache: true > > + > > +additionalProperties: false > > + > > +required: > > required: goes before additionalProperties:. > OK, I'll fix it in v2. Thanks for the review. Best Regards, Eric Lin. > > Best regards, > Krzysztof >
On 26/06/2023 05:26, Eric Lin wrote: > Hi Krzysztof, > > On Fri, Jun 16, 2023 at 6:45 PM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 16/06/2023 08:32, Eric Lin wrote: >>> This add YAML DT binding documentation for SiFive Private L2 >>> cache controller >>> >>> Signed-off-by: Eric Lin <eric.lin@sifive.com> >>> Reviewed-by: Zong Li <zong.li@sifive.com> >>> Reviewed-by: Nick Hu <nick.hu@sifive.com> >>> --- >>> .../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 >>> @@ -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@sifive.com> >>> + - Eric Lin <eric.lin@sifive.com> >>> + >>> +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. >> >> Drop the last sentence. Why specification would not apply? >> > OK, I'll drop it in v2. > >>> + >>> +allOf: >>> + - $ref: /schemas/cache-controller.yaml# >>> + >>> +select: >>> + properties: >>> + compatible: >>> + contains: >>> + enum: >>> + - sifive,pL2Cache0 >>> + - sifive,pL2Cache >>> + >>> + required: >>> + - compatible >>> + >>> +properties: >>> + compatible: >>> + items: >> >> >> You have only one item, so no need for items... unless you just missed >> proper fallback. > > OK, I'll fix it in v2. > >> >>> + - enum: >>> + - sifive,pL2Cache0 >>> + - sifive,pL2Cache1 >> >> What is "0" and "1" here? What do these compatibles represent? Why they >> do not have any SoC related part? > > The pL2Cache1 has minor changes in hardware, but it can use the same > pl2 cache driver. Then why aren't they compatible? > May I ask, what do you mean about the SoC-related part? Thanks. This is part of a SoC, right? We expect SoC blocks to have compatible based on the SoC. Best regards, Krzysztof
Hi Krzysztof, On Mon, Jun 26, 2023 at 2:19 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 26/06/2023 05:26, Eric Lin wrote: > > Hi Krzysztof, > > > > On Fri, Jun 16, 2023 at 6:45 PM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> > >> On 16/06/2023 08:32, Eric Lin wrote: > >>> This add YAML DT binding documentation for SiFive Private L2 > >>> cache controller > >>> > >>> Signed-off-by: Eric Lin <eric.lin@sifive.com> > >>> Reviewed-by: Zong Li <zong.li@sifive.com> > >>> Reviewed-by: Nick Hu <nick.hu@sifive.com> > >>> --- > >>> .../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 > >>> @@ -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@sifive.com> > >>> + - Eric Lin <eric.lin@sifive.com> > >>> + > >>> +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. > >> > >> Drop the last sentence. Why specification would not apply? > >> > > OK, I'll drop it in v2. > > > >>> + > >>> +allOf: > >>> + - $ref: /schemas/cache-controller.yaml# > >>> + > >>> +select: > >>> + properties: > >>> + compatible: > >>> + contains: > >>> + enum: > >>> + - sifive,pL2Cache0 > >>> + - sifive,pL2Cache > >>> + > >>> + required: > >>> + - compatible > >>> + > >>> +properties: > >>> + compatible: > >>> + items: > >> > >> > >> You have only one item, so no need for items... unless you just missed > >> proper fallback. > > > > OK, I'll fix it in v2. > > > >> > >>> + - enum: > >>> + - sifive,pL2Cache0 > >>> + - sifive,pL2Cache1 > >> > >> What is "0" and "1" here? What do these compatibles represent? Why they > >> do not have any SoC related part? > > > > The pL2Cache1 has minor changes in hardware, but it can use the same > > pl2 cache driver. > > Then why aren't they compatible? > The pL2Cache1 has removed some unused bits in the register compared to pl2Cache0. From the hardware perspective, they are not compatible but they can share the same pl2 cache driver in software. Thus, we would like to keep both. It would be great if you can provide some suggestions. Thanks. Best Regards, Eric Lin. > > May I ask, what do you mean about the SoC-related part? Thanks. > > This is part of a SoC, right? We expect SoC blocks to have compatible > based on the SoC. > > > > Best regards, > Krzysztof >
On 28/06/2023 18:31, Eric Lin wrote: >>>> >>>>> + - enum: >>>>> + - sifive,pL2Cache0 >>>>> + - sifive,pL2Cache1 >>>> >>>> What is "0" and "1" here? What do these compatibles represent? Why they >>>> do not have any SoC related part? >>> >>> The pL2Cache1 has minor changes in hardware, but it can use the same >>> pl2 cache driver. >> >> Then why aren't they compatible? >> > > The pL2Cache1 has removed some unused bits in the register compared to > pl2Cache0. > From the hardware perspective, they are not compatible but they can > share the same pl2 cache driver in software. So they are compatible... If they were not compatible, you wouldn't be able to use the same match in the driver. > Thus, we would like to keep both. It would be great if you can provide > some suggestions. Thanks. I propose to make them compatible, like every other piece of SoC. I don't see any benefit of having them separate. Best regards, Krzysztof
On Sat, Jul 01, 2023 at 10:22:25AM +0200, Krzysztof Kozlowski wrote: > On 28/06/2023 18:31, Eric Lin wrote: > > >>>> > >>>>> + - enum: > >>>>> + - sifive,pL2Cache0 > >>>>> + - sifive,pL2Cache1 > >>>> > >>>> What is "0" and "1" here? What do these compatibles represent? Why they > >>>> do not have any SoC related part? > >>> > >>> The pL2Cache1 has minor changes in hardware, but it can use the same > >>> pl2 cache driver. > >> > >> Then why aren't they compatible? > >> > > > > The pL2Cache1 has removed some unused bits in the register compared to > > pl2Cache0. > > From the hardware perspective, they are not compatible but they can > > share the same pl2 cache driver in software. > > So they are compatible... If they were not compatible, you wouldn't be > able to use the same match in the driver. > > > Thus, we would like to keep both. It would be great if you can provide > > some suggestions. Thanks. > > I propose to make them compatible, like every other piece of SoC. I > don't see any benefit of having them separate. > Hi Krzysztof, Sorry for the late reply. The pl2 cache is our internal platform IP and is not part of any SoC. The reason why this driver is compatible with the hardware "pl2cache0" and hardware "pl2cache1" is that it doesn't program the different parts of the config register However, our internal software (e.g., bare-metal software) will program these different parts, so it needs to rely on the different compatible string to identify the hardware. Additionally, we would like the compatible strings to reflect which hardware is being used Thanks. Best regards, Eric Lin > Best regards, > Krzysztof >
On 12/07/2023 13:09, Eric Lin wrote: > On Sat, Jul 01, 2023 at 10:22:25AM +0200, Krzysztof Kozlowski wrote: >> On 28/06/2023 18:31, Eric Lin wrote: >> >>>>>> >>>>>>> + - enum: >>>>>>> + - sifive,pL2Cache0 >>>>>>> + - sifive,pL2Cache1 >>>>>> >>>>>> What is "0" and "1" here? What do these compatibles represent? Why they >>>>>> do not have any SoC related part? >>>>> >>>>> The pL2Cache1 has minor changes in hardware, but it can use the same >>>>> pl2 cache driver. >>>> >>>> Then why aren't they compatible? >>>> >>> >>> The pL2Cache1 has removed some unused bits in the register compared to >>> pl2Cache0. >>> From the hardware perspective, they are not compatible but they can >>> share the same pl2 cache driver in software. >> >> So they are compatible... If they were not compatible, you wouldn't be >> able to use the same match in the driver. >> >>> Thus, we would like to keep both. It would be great if you can provide >>> some suggestions. Thanks. >> >> I propose to make them compatible, like every other piece of SoC. I >> don't see any benefit of having them separate. >> > > Hi Krzysztof, > > Sorry for the late reply. > The pl2 cache is our internal platform IP and is not part of any SoC. > > The reason why this driver is compatible with the hardware "pl2cache0" and hardware "pl2cache1" > is that it doesn't program the different parts of the config register > However, our internal software (e.g., bare-metal software) will program these different parts, > so it needs to rely on the different compatible string to identify the hardware. > > Additionally, we would like the compatible strings to reflect which hardware is being used Thanks. I don't understand how does it contradicts anything I said. So you do agree with me? Or what? Best regards, Krzysztof
On Wed, Jul 12, 2023 at 02:30:06PM +0200, Krzysztof Kozlowski wrote: > On 12/07/2023 13:09, Eric Lin wrote: > > On Sat, Jul 01, 2023 at 10:22:25AM +0200, Krzysztof Kozlowski wrote: > >> On 28/06/2023 18:31, Eric Lin wrote: > >> > >>>>>> > >>>>>>> + - enum: > >>>>>>> + - sifive,pL2Cache0 > >>>>>>> + - sifive,pL2Cache1 > >>>>>> > >>>>>> What is "0" and "1" here? What do these compatibles represent? Why they > >>>>>> do not have any SoC related part? > >>>>> > >>>>> The pL2Cache1 has minor changes in hardware, but it can use the same > >>>>> pl2 cache driver. > >>>> > >>>> Then why aren't they compatible? > >>>> > >>> > >>> The pL2Cache1 has removed some unused bits in the register compared to > >>> pl2Cache0. > >>> From the hardware perspective, they are not compatible but they can > >>> share the same pl2 cache driver in software. > >> > >> So they are compatible... If they were not compatible, you wouldn't be > >> able to use the same match in the driver. > >> > >>> Thus, we would like to keep both. It would be great if you can provide > >>> some suggestions. Thanks. > >> > >> I propose to make them compatible, like every other piece of SoC. I > >> don't see any benefit of having them separate. > >> > > Sorry for the late reply. > > The pl2 cache is our internal platform IP and is not part of any SoC. > > > > The reason why this driver is compatible with the hardware "pl2cache0" and hardware "pl2cache1" > > is that it doesn't program the different parts of the config register > > However, our internal software (e.g., bare-metal software) will program these different parts, > > so it needs to rely on the different compatible string to identify the hardware. > > > > Additionally, we would like the compatible strings to reflect which hardware is being used Thanks. > > I don't understand how does it contradicts anything I said. So you do > agree with me? Or what? I probably should've been keeping a closer eye here, sorry. I assume what Krzysztof means is why do you permit both "sifive,pL2Cache0" and "sifive,pL2Cache1" appearing in isolation. IOW, both of compatible = "sifive,pl2cache0"; and compatible = "sifive,pl2cache1"; are valid in your binding. The hardware for both might be different, and their full featuresets may be incompatible, but they implement a compatible subset of features. I would expect that the following would be the permitted compatible setups: compatible = "sifive,pl2cache0"; and compatible = "sifive,pl2cache1", "sifive,pl2cache0"; A consumer of the DT that does care for the differences should be looking for the specific compatible, and OS code that does not care can always bind to the "0" version. Do the "0" & "1" here refer to the IP version, as in sifive-blocks-ip-versioning.txt? I didn't think the compatibles containing those IP versions were supposed to appear in isolation, without a soc-specific one? Thanks, Conor.
Hi Krzysztof, On Wed, Jul 12, 2023 at 8:30 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 12/07/2023 13:09, Eric Lin wrote: > > On Sat, Jul 01, 2023 at 10:22:25AM +0200, Krzysztof Kozlowski wrote: > >> On 28/06/2023 18:31, Eric Lin wrote: > >> > >>>>>> > >>>>>>> + - enum: > >>>>>>> + - sifive,pL2Cache0 > >>>>>>> + - sifive,pL2Cache1 > >>>>>> > >>>>>> What is "0" and "1" here? What do these compatibles represent? Why they > >>>>>> do not have any SoC related part? > >>>>> > >>>>> The pL2Cache1 has minor changes in hardware, but it can use the same > >>>>> pl2 cache driver. > >>>> > >>>> Then why aren't they compatible? > >>>> > >>> > >>> The pL2Cache1 has removed some unused bits in the register compared to > >>> pl2Cache0. > >>> From the hardware perspective, they are not compatible but they can > >>> share the same pl2 cache driver in software. > >> > >> So they are compatible... If they were not compatible, you wouldn't be > >> able to use the same match in the driver. > >> > >>> Thus, we would like to keep both. It would be great if you can provide > >>> some suggestions. Thanks. > >> > >> I propose to make them compatible, like every other piece of SoC. I > >> don't see any benefit of having them separate. > >> > > > > Hi Krzysztof, > > > > Sorry for the late reply. > > The pl2 cache is our internal platform IP and is not part of any SoC. > > > > The reason why this driver is compatible with the hardware "pl2cache0" and hardware "pl2cache1" > > is that it doesn't program the different parts of the config register > > However, our internal software (e.g., bare-metal software) will program these different parts, > > so it needs to rely on the different compatible string to identify the hardware. > > > > Additionally, we would like the compatible strings to reflect which hardware is being used Thanks. > > I don't understand how does it contradicts anything I said. So you do > agree with me? Or what? > Thanks for your suggestions. OK, I'll fix it in v2. Best regards, Eric Lin > Best regards, > Krzysztof >
Hi Conor, On Wed, Jul 12, 2023 at 8:49 PM Conor Dooley <conor.dooley@microchip.com> wrote: > > On Wed, Jul 12, 2023 at 02:30:06PM +0200, Krzysztof Kozlowski wrote: > > On 12/07/2023 13:09, Eric Lin wrote: > > > On Sat, Jul 01, 2023 at 10:22:25AM +0200, Krzysztof Kozlowski wrote: > > >> On 28/06/2023 18:31, Eric Lin wrote: > > >> > > >>>>>> > > >>>>>>> + - enum: > > >>>>>>> + - sifive,pL2Cache0 > > >>>>>>> + - sifive,pL2Cache1 > > >>>>>> > > >>>>>> What is "0" and "1" here? What do these compatibles represent? Why they > > >>>>>> do not have any SoC related part? > > >>>>> > > >>>>> The pL2Cache1 has minor changes in hardware, but it can use the same > > >>>>> pl2 cache driver. > > >>>> > > >>>> Then why aren't they compatible? > > >>>> > > >>> > > >>> The pL2Cache1 has removed some unused bits in the register compared to > > >>> pl2Cache0. > > >>> From the hardware perspective, they are not compatible but they can > > >>> share the same pl2 cache driver in software. > > >> > > >> So they are compatible... If they were not compatible, you wouldn't be > > >> able to use the same match in the driver. > > >> > > >>> Thus, we would like to keep both. It would be great if you can provide > > >>> some suggestions. Thanks. > > >> > > >> I propose to make them compatible, like every other piece of SoC. I > > >> don't see any benefit of having them separate. > > >> > > > Sorry for the late reply. > > > The pl2 cache is our internal platform IP and is not part of any SoC. > > > > > > The reason why this driver is compatible with the hardware "pl2cache0" and hardware "pl2cache1" > > > is that it doesn't program the different parts of the config register > > > However, our internal software (e.g., bare-metal software) will program these different parts, > > > so it needs to rely on the different compatible string to identify the hardware. > > > > > > Additionally, we would like the compatible strings to reflect which hardware is being used Thanks. > > > > I don't understand how does it contradicts anything I said. So you do > > agree with me? Or what? > > I probably should've been keeping a closer eye here, sorry. > > I assume what Krzysztof means is why do you permit both > "sifive,pL2Cache0" and "sifive,pL2Cache1" appearing in isolation. IOW, > both of > compatible = "sifive,pl2cache0"; > and > compatible = "sifive,pl2cache1"; > are valid in your binding. > > The hardware for both might be different, and their full featuresets may > be incompatible, but they implement a compatible subset of features. I > would expect that the following would be the permitted compatible setups: > compatible = "sifive,pl2cache0"; > and > compatible = "sifive,pl2cache1", "sifive,pl2cache0"; > > A consumer of the DT that does care for the differences should be > looking for the specific compatible, and OS code that does not care can > always bind to the "0" version. > Yes, but I think the proper compatible string for hw pl2cache0 and hw pl2cache1 should be as follows: hw pl2cache0 -> compatible = "sifive,pl2cache0","sifive,pl2cache1"; hw pl2cache1 -> compatible = "sifive,pl2cache1"; Since the hw pl2cache0 implements more features than hw pl2cache1, it can be compatible with the pl2cache1 driver. However, hw pl2cache1 only implements a sub-feature of hw pl2cache0, so it cannot be compatible with the pl2cache0 driver. Thus, I'll keep only the compatible = "sifive,pl2cache1". in the driver and dt-binding. Thanks for the suggestions. > Do the "0" & "1" here refer to the IP version, as in > sifive-blocks-ip-versioning.txt? I didn't think the compatibles > containing those IP versions were supposed to appear in isolation, > without a soc-specific one? > Yes, I think they refer to IP versions. OK, I'll fix it in v2. Thanks for the review. Best regards, Eric Lin > Thanks, > Conor.
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 @@ -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@sifive.com> + - Eric Lin <eric.lin@sifive.com> + +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. + +allOf: + - $ref: /schemas/cache-controller.yaml# + +select: + properties: + compatible: + contains: + enum: + - sifive,pL2Cache0 + - sifive,pL2Cache1 + + required: + - compatible + +properties: + compatible: + items: + - enum: + - sifive,pL2Cache0 + - sifive,pL2Cache1 + + 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 { + 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>; + };