diff mbox series

[RESEND] dt-bindings: sifive-ccache: fix cache level for l3 cache

Message ID 20220830125133.1698781-1-ben.dooks@sifive.com (mailing list archive)
State New, archived
Headers show
Series [RESEND] dt-bindings: sifive-ccache: fix cache level for l3 cache | expand

Commit Message

Ben Dooks Aug. 30, 2022, 12:51 p.m. UTC
With newer cores such as the p550, the SiFive composable cache can be
a level 3 cache. Update the cache level to be one of 2 or 3.

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
 Documentation/devicetree/bindings/riscv/sifive-ccache.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Conor Dooley Aug. 30, 2022, 12:56 p.m. UTC | #1
On 30/08/2022 13:51, Ben Dooks wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> With newer cores such as the p550, the SiFive composable cache can be
> a level 3 cache. Update the cache level to be one of 2 or 3.
> 
> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
> ---
>   Documentation/devicetree/bindings/riscv/sifive-ccache.yaml | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/riscv/sifive-ccache.yaml b/Documentation/devicetree/bindings/riscv/sifive-ccache.yaml
> index 1a64a5384e36..6190deb65455 100644
> --- a/Documentation/devicetree/bindings/riscv/sifive-ccache.yaml
> +++ b/Documentation/devicetree/bindings/riscv/sifive-ccache.yaml
> @@ -45,7 +45,7 @@ properties:
>       const: 64
> 
>     cache-level:
> -    const: 2
> +    enum: [2, 3]

Do we want to enforce the cache level like we currently do for
interrupts and cache-sets?
Ben Dooks Aug. 30, 2022, 12:58 p.m. UTC | #2
On 30/08/2022 13:56, Conor.Dooley@microchip.com wrote:
> On 30/08/2022 13:51, Ben Dooks wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> With newer cores such as the p550, the SiFive composable cache can be
>> a level 3 cache. Update the cache level to be one of 2 or 3.
>>
>> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
>> ---
>>    Documentation/devicetree/bindings/riscv/sifive-ccache.yaml | 2 +-
>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/riscv/sifive-ccache.yaml b/Documentation/devicetree/bindings/riscv/sifive-ccache.yaml
>> index 1a64a5384e36..6190deb65455 100644
>> --- a/Documentation/devicetree/bindings/riscv/sifive-ccache.yaml
>> +++ b/Documentation/devicetree/bindings/riscv/sifive-ccache.yaml
>> @@ -45,7 +45,7 @@ properties:
>>        const: 64
>>
>>      cache-level:
>> -    const: 2
>> +    enum: [2, 3]
> 
> Do we want to enforce the cache level like we currently do for
> interrupts and cache-sets?

Not sure on that, for the P550 cores the ccache is going to be level3
and my colleague has said it does confuse some tooling if the level is
not set correctly.
Conor Dooley Aug. 30, 2022, 1:49 p.m. UTC | #3
On 30/08/2022 13:58, Ben Dooks wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 30/08/2022 13:56, Conor.Dooley@microchip.com wrote:
>> On 30/08/2022 13:51, Ben Dooks wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> With newer cores such as the p550, the SiFive composable cache can be
>>> a level 3 cache. Update the cache level to be one of 2 or 3.
>>>
>>> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
>>> ---
>>>    Documentation/devicetree/bindings/riscv/sifive-ccache.yaml | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/riscv/sifive-ccache.yaml b/Documentation/devicetree/bindings/riscv/sifive-ccache.yaml
>>> index 1a64a5384e36..6190deb65455 100644
>>> --- a/Documentation/devicetree/bindings/riscv/sifive-ccache.yaml
>>> +++ b/Documentation/devicetree/bindings/riscv/sifive-ccache.yaml
>>> @@ -45,7 +45,7 @@ properties:
>>>        const: 64
>>>
>>>      cache-level:
>>> -    const: 2
>>> +    enum: [2, 3]
>>
>> Do we want to enforce the cache level like we currently do for
>> interrupts and cache-sets?
> 
> Not sure on that, for the P550 cores the ccache is going to be level3
> and my colleague has said it does confuse some tooling if the level is
> not set correctly.

What I meant was:
Currently we enforce the correct cache-sets & interrupts based on the
compatible string. Adding enum: [2, 3] relaxes the enforcement of the
cache-level for existing compatibles and does not prevent someone from
setting an incorrect cache level for p550 cores.

I think that on top of adding the enum, we should add some enforcement
so that the cache is not incorrectly configured for both existing l2
caches and for the new l3 versions.

@Zong, could you please incorporate Ben's patches into your V2? it
would make it a lot easier to review what's going on here. It may
also make sense to add the compatible for the p550 cache while we are
at it...

FYI, there is also this patch here outstanding against the l2:
https://lore.kernel.org/linux-riscv/20220825180417.1259360-2-mail@conchuod.ie/

I intend taking this into 6.0-rc5 or so as a fix, so if you could
rebase the series on that so it is not lost in the dt-binding rename
that would be great.

Thanks,
Conor.
Ben Dooks Aug. 30, 2022, 4:49 p.m. UTC | #4
On 30/08/2022 14:49, Conor.Dooley@microchip.com wrote:
> On 30/08/2022 13:58, Ben Dooks wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 30/08/2022 13:56, Conor.Dooley@microchip.com wrote:
>>> On 30/08/2022 13:51, Ben Dooks wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> With newer cores such as the p550, the SiFive composable cache can be
>>>> a level 3 cache. Update the cache level to be one of 2 or 3.
>>>>
>>>> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
>>>> ---
>>>>     Documentation/devicetree/bindings/riscv/sifive-ccache.yaml | 2 +-
>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/riscv/sifive-ccache.yaml b/Documentation/devicetree/bindings/riscv/sifive-ccache.yaml
>>>> index 1a64a5384e36..6190deb65455 100644
>>>> --- a/Documentation/devicetree/bindings/riscv/sifive-ccache.yaml
>>>> +++ b/Documentation/devicetree/bindings/riscv/sifive-ccache.yaml
>>>> @@ -45,7 +45,7 @@ properties:
>>>>         const: 64
>>>>
>>>>       cache-level:
>>>> -    const: 2
>>>> +    enum: [2, 3]
>>>
>>> Do we want to enforce the cache level like we currently do for
>>> interrupts and cache-sets?
>>
>> Not sure on that, for the P550 cores the ccache is going to be level3
>> and my colleague has said it does confuse some tooling if the level is
>> not set correctly.
> 
> What I meant was:
> Currently we enforce the correct cache-sets & interrupts based on the
> compatible string. Adding enum: [2, 3] relaxes the enforcement of the
> cache-level for existing compatibles and does not prevent someone from
> setting an incorrect cache level for p550 cores.
> 
> I think that on top of adding the enum, we should add some enforcement
> so that the cache is not incorrectly configured for both existing l2
> caches and for the new l3 versions.

Ok, we can add some enforcement once we add the new bindings, but I'm
not ready for that today and I'd rather get the current queue sorted
out first before we come in with newer silicon which hasn't hit the
market uet.

> @Zong, could you please incorporate Ben's patches into your V2? it
> would make it a lot easier to review what's going on here. It may
> also make sense to add the compatible for the p550 cache while we are
> at it...
> 
> FYI, there is also this patch here outstanding against the l2:
> https://lore.kernel.org/linux-riscv/20220825180417.1259360-2-mail@conchuod.ie/
> 
> I intend taking this into 6.0-rc5 or so as a fix, so if you could
> rebase the series on that so it is not lost in the dt-binding rename
> that would be great.

Do we need someone to take charge of this series?
Conor Dooley Aug. 30, 2022, 5:08 p.m. UTC | #5
On 30/08/2022 17:49, Ben Dooks wrote:
> On 30/08/2022 14:49, Conor.Dooley@microchip.com wrote:
>> On 30/08/2022 13:58, Ben Dooks wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 30/08/2022 13:56, Conor.Dooley@microchip.com wrote:
>>>> On 30/08/2022 13:51, Ben Dooks wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>
>>>>> With newer cores such as the p550, the SiFive composable cache can be
>>>>> a level 3 cache. Update the cache level to be one of 2 or 3.
>>>>>
>>>>> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
>>>>> ---
>>>>>     Documentation/devicetree/bindings/riscv/sifive-ccache.yaml | 2 +-
>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/riscv/sifive-ccache.yaml b/Documentation/devicetree/bindings/riscv/sifive-ccache.yaml
>>>>> index 1a64a5384e36..6190deb65455 100644
>>>>> --- a/Documentation/devicetree/bindings/riscv/sifive-ccache.yaml
>>>>> +++ b/Documentation/devicetree/bindings/riscv/sifive-ccache.yaml
>>>>> @@ -45,7 +45,7 @@ properties:
>>>>>         const: 64
>>>>>
>>>>>       cache-level:
>>>>> -    const: 2
>>>>> +    enum: [2, 3]
>>>>
>>>> Do we want to enforce the cache level like we currently do for
>>>> interrupts and cache-sets?
>>>
>>> Not sure on that, for the P550 cores the ccache is going to be level3
>>> and my colleague has said it does confuse some tooling if the level is
>>> not set correctly.
>>
>> What I meant was:
>> Currently we enforce the correct cache-sets & interrupts based on the
>> compatible string. Adding enum: [2, 3] relaxes the enforcement of the
>> cache-level for existing compatibles and does not prevent someone from
>> setting an incorrect cache level for p550 cores.
>>
>> I think that on top of adding the enum, we should add some enforcement
>> so that the cache is not incorrectly configured for both existing l2
>> caches and for the new l3 versions.
> 
> Ok, we can add some enforcement once we add the new bindings, but I'm
> not ready for that today and I'd rather get the current queue sorted
> out first before we come in with newer silicon which hasn't hit the
> market uet.

SGTM. Maybe this particular patch could come at the start of the
series. And then we could add something so that the end result looks
like the following (white space damaged) diff:

diff --git a/Documentation/devicetree/bindings/riscv/sifive-l2-cache.yaml b/Documentation/devicetree/bindings/riscv/sifive-l2-cache.yaml
index ca3b9be58058..994e4b143485 100644
--- a/Documentation/devicetree/bindings/riscv/sifive-l2-cache.yaml
+++ b/Documentation/devicetree/bindings/riscv/sifive-l2-cache.yaml
@@ -33,6 +33,7 @@ properties:
     oneOf:
       - items:
           - enum:
+              - sifive,ccache0
               - sifive,fu540-c000-ccache
               - sifive,fu740-c000-ccache
           - const: cache
@@ -45,7 +46,7 @@ properties:
     const: 64
 
   cache-level:
-    const: 2
+    enum: [2, 3]
 
   cache-sets:
     enum: [1024, 2048]
@@ -115,6 +116,23 @@ allOf:
         cache-sets:
           const: 1024
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            const:
+              sifive,ccache0
+
+    then:
+      properties:
+        cache-level:
+         enum: [2, 3]
+
+    else:
+      properties:
+        cache-level:
+          const: 2
+
 additionalProperties: false
 
 required:


That would keep the enforcement for existing caches and allow you
the freedome to do w/e you want for the ccache0 compatible.

> 
>> @Zong, could you please incorporate Ben's patches into your V2? it
>> would make it a lot easier to review what's going on here. It may
>> also make sense to add the compatible for the p550 cache while we are
>> at it...
>>
>> FYI, there is also this patch here outstanding against the l2:
>> https://lore.kernel.org/linux-riscv/20220825180417.1259360-2-mail@conchuod.ie/
>>
>> I intend taking this into 6.0-rc5 or so as a fix, so if you could
>> rebase the series on that so it is not lost in the dt-binding rename
>> that would be great.
> 
> Do we need someone to take charge of this series?
> 

Can I volunteer Zong? (since all but two of the patches are theirs)

Thanks,
Conor.
Zong Li Aug. 31, 2022, 5:17 a.m. UTC | #6
<Conor.Dooley@microchip.com> 於 2022年8月31日 週三 凌晨1:09寫道:
>
> On 30/08/2022 17:49, Ben Dooks wrote:
> > On 30/08/2022 14:49, Conor.Dooley@microchip.com wrote:
> >> On 30/08/2022 13:58, Ben Dooks wrote:
> >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >>>
> >>> On 30/08/2022 13:56, Conor.Dooley@microchip.com wrote:
> >>>> On 30/08/2022 13:51, Ben Dooks wrote:
> >>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >>>>>
> >>>>> With newer cores such as the p550, the SiFive composable cache can be
> >>>>> a level 3 cache. Update the cache level to be one of 2 or 3.
> >>>>>
> >>>>> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
> >>>>> ---
> >>>>>     Documentation/devicetree/bindings/riscv/sifive-ccache.yaml | 2 +-
> >>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/riscv/sifive-ccache.yaml b/Documentation/devicetree/bindings/riscv/sifive-ccache.yaml
> >>>>> index 1a64a5384e36..6190deb65455 100644
> >>>>> --- a/Documentation/devicetree/bindings/riscv/sifive-ccache.yaml
> >>>>> +++ b/Documentation/devicetree/bindings/riscv/sifive-ccache.yaml
> >>>>> @@ -45,7 +45,7 @@ properties:
> >>>>>         const: 64
> >>>>>
> >>>>>       cache-level:
> >>>>> -    const: 2
> >>>>> +    enum: [2, 3]
> >>>>
> >>>> Do we want to enforce the cache level like we currently do for
> >>>> interrupts and cache-sets?
> >>>
> >>> Not sure on that, for the P550 cores the ccache is going to be level3
> >>> and my colleague has said it does confuse some tooling if the level is
> >>> not set correctly.
> >>
> >> What I meant was:
> >> Currently we enforce the correct cache-sets & interrupts based on the
> >> compatible string. Adding enum: [2, 3] relaxes the enforcement of the
> >> cache-level for existing compatibles and does not prevent someone from
> >> setting an incorrect cache level for p550 cores.
> >>
> >> I think that on top of adding the enum, we should add some enforcement
> >> so that the cache is not incorrectly configured for both existing l2
> >> caches and for the new l3 versions.
> >
> > Ok, we can add some enforcement once we add the new bindings, but I'm
> > not ready for that today and I'd rather get the current queue sorted
> > out first before we come in with newer silicon which hasn't hit the
> > market uet.
>
> SGTM. Maybe this particular patch could come at the start of the
> series. And then we could add something so that the end result looks
> like the following (white space damaged) diff:
>
> diff --git a/Documentation/devicetree/bindings/riscv/sifive-l2-cache.yaml b/Documentation/devicetree/bindings/riscv/sifive-l2-cache.yaml
> index ca3b9be58058..994e4b143485 100644
> --- a/Documentation/devicetree/bindings/riscv/sifive-l2-cache.yaml
> +++ b/Documentation/devicetree/bindings/riscv/sifive-l2-cache.yaml
> @@ -33,6 +33,7 @@ properties:
>      oneOf:
>        - items:
>            - enum:
> +              - sifive,ccache0
>                - sifive,fu540-c000-ccache
>                - sifive,fu740-c000-ccache
>            - const: cache
> @@ -45,7 +46,7 @@ properties:
>      const: 64
>
>    cache-level:
> -    const: 2
> +    enum: [2, 3]
>
>    cache-sets:
>      enum: [1024, 2048]
> @@ -115,6 +116,23 @@ allOf:
>          cache-sets:
>            const: 1024
>
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const:
> +              sifive,ccache0
> +
> +    then:
> +      properties:
> +        cache-level:
> +         enum: [2, 3]
> +
> +    else:
> +      properties:
> +        cache-level:
> +          const: 2
> +
>  additionalProperties: false
>
>  required:
>
>
> That would keep the enforcement for existing caches and allow you
> the freedome to do w/e you want for the ccache0 compatible.

Thanks you all for bring me here,  we actually have some core series
with 4096 cache set in ccache, should we need to extend the cache set
as follow? or we only need to focus on the DTS which is already in
mainline.

cache-sets:
-    enum: [1024, 2048]
+   enum: [1024, 2048, 4096]

>
> >
> >> @Zong, could you please incorporate Ben's patches into your V2? it
> >> would make it a lot easier to review what's going on here. It may
> >> also make sense to add the compatible for the p550 cache while we are
> >> at it...
> >>
> >> FYI, there is also this patch here outstanding against the l2:
> >> https://lore.kernel.org/linux-riscv/20220825180417.1259360-2-mail@conchuod.ie/
> >>
> >> I intend taking this into 6.0-rc5 or so as a fix, so if you could
> >> rebase the series on that so it is not lost in the dt-binding rename
> >> that would be great.
> >
> > Do we need someone to take charge of this series?
> >
>
> Can I volunteer Zong? (since all but two of the patches are theirs)
>

It is ok to me, but I'm still refining the patchset for V2, and I'm
not sure if we will need the V3. Do you prefer to take V2 patch set
first and replace the dt-binding patch?

> Thanks,
> Conor.
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Conor Dooley Aug. 31, 2022, 6:25 a.m. UTC | #7
On 31/08/2022 06:17, Zong Li wrote:
> [Some people who received this message don't often get email from zongbox@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> <Conor.Dooley@microchip.com> 於 2022年8月31日 週三 凌晨1:09寫道:
>> That would keep the enforcement for existing caches and allow you
>> the freedome to do w/e you want for the ccache0 compatible.
> 
> Thanks you all for bring me here,  we actually have some core series
> with 4096 cache set in ccache, should we need to extend the cache set
> as follow? or we only need to focus on the DTS which is already in
> mainline.
> 
> cache-sets:
> -    enum: [1024, 2048]
> +   enum: [1024, 2048, 4096]

Until a user shows up, I think we are better off not adding 4096.

>>> Do we need someone to take charge of this series?
>>>
>>
>> Can I volunteer Zong? (since all but two of the patches are theirs)
>>
> 
> It is ok to me, but I'm still refining the patchset for V2, and I'm
> not sure if we will need the V3. Do you prefer to take V2 patch set
> first and replace the dt-binding patch?

If you could incorporate it for v2 it would make reviewing easier
I think.

Thanks,
Conor.
Rob Herring (Arm) Sept. 2, 2022, 7:36 p.m. UTC | #8
On Tue, 30 Aug 2022 13:51:33 +0100, Ben Dooks wrote:
> With newer cores such as the p550, the SiFive composable cache can be
> a level 3 cache. Update the cache level to be one of 2 or 3.
> 
> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
> ---
>  Documentation/devicetree/bindings/riscv/sifive-ccache.yaml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Acked-by: Rob Herring <robh@kernel.org>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/riscv/sifive-ccache.yaml b/Documentation/devicetree/bindings/riscv/sifive-ccache.yaml
index 1a64a5384e36..6190deb65455 100644
--- a/Documentation/devicetree/bindings/riscv/sifive-ccache.yaml
+++ b/Documentation/devicetree/bindings/riscv/sifive-ccache.yaml
@@ -45,7 +45,7 @@  properties:
     const: 64
 
   cache-level:
-    const: 2
+    enum: [2, 3]
 
   cache-sets:
     enum: [1024, 2048]