diff mbox series

[v1,1/6] dt-bindings: cache: Document the sifive,perfmon-counters property

Message ID 20240216000837.1868917-2-samuel.holland@sifive.com
State New
Headers
Series SiFive cache controller PMU drivers |

Commit Message

Samuel Holland Feb. 16, 2024, 12:08 a.m. UTC 
  The SiFive Composable Cache controller contains an optional PMU with a
configurable number of event counters. Document a property which
describes the number of available counters.

Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
---

 Documentation/devicetree/bindings/cache/sifive,ccache0.yaml | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Krzysztof Kozlowski Feb. 17, 2024, 9 a.m. UTC | #1 
On 16/02/2024 01:08, Samuel Holland wrote:
> The SiFive Composable Cache controller contains an optional PMU with a
> configurable number of event counters. Document a property which

Configurable in what context? By chip designers or by OS? Why this
cannot be deduced from the compatible?

> describes the number of available counters.
> 
> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> ---
> 
>  Documentation/devicetree/bindings/cache/sifive,ccache0.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
>  

Best regards,
Krzysztof
Samuel Holland Feb. 18, 2024, 3:29 p.m. UTC | #2 
Hi Krzysztof,

On 2024-02-17 3:00 AM, Krzysztof Kozlowski wrote:
> On 16/02/2024 01:08, Samuel Holland wrote:
>> The SiFive Composable Cache controller contains an optional PMU with a
>> configurable number of event counters. Document a property which
> 
> Configurable in what context? By chip designers or by OS? Why this
> cannot be deduced from the compatible?

This parameter is configurable by the chip designers.

The information certainly can be deduced from the SoC-specific compatible
string, but doing so makes the driver only work on that specific list of SoCs.
When provided via a property, the driver can work without changes on any SoC
that uses this IP block. (None of the SoCs currently listed in the binding
contain a PMU, so there is no backward compatibility concern with adding the new
property.)

My understanding of the purpose of the SoC-specific compatible string is to
handle eventualities (silicon bugs, integration quirks, etc.), not to
intentionally limit the driver to a narrow list of hardware.

Regards,
Samuel

>> describes the number of available counters.
>>
>> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
>> ---
>>
>>  Documentation/devicetree/bindings/cache/sifive,ccache0.yaml | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
Krzysztof Kozlowski Feb. 18, 2024, 6:35 p.m. UTC | #3 
On 18/02/2024 16:29, Samuel Holland wrote:
> Hi Krzysztof,
> 
> On 2024-02-17 3:00 AM, Krzysztof Kozlowski wrote:
>> On 16/02/2024 01:08, Samuel Holland wrote:
>>> The SiFive Composable Cache controller contains an optional PMU with a
>>> configurable number of event counters. Document a property which
>>
>> Configurable in what context? By chip designers or by OS? Why this
>> cannot be deduced from the compatible?
> 
> This parameter is configurable by the chip designers.
> 
> The information certainly can be deduced from the SoC-specific compatible
> string, but doing so makes the driver only work on that specific list of SoCs.

Usually that's exactly what's expected, so why here usual approach is wrong?

> When provided via a property, the driver can work without changes on any SoC
> that uses this IP block. (None of the SoCs currently listed in the binding

Sorry, properties are not a work-around for missing compatibles.

> contain a PMU, so there is no backward compatibility concern with adding the new
> property.)
> 
> My understanding of the purpose of the SoC-specific compatible string is to
> handle eventualities (silicon bugs, integration quirks, etc.), not to
> intentionally limit the driver to a narrow list of hardware.

Depends what is the hardware. For most of licensed blocks, the final
design is the hardware so equals to its compatible.

Best regards,
Krzysztof
Rob Herring Feb. 22, 2024, 7:36 p.m. UTC | #4 
On Sun, Feb 18, 2024 at 07:35:35PM +0100, Krzysztof Kozlowski wrote:
> On 18/02/2024 16:29, Samuel Holland wrote:
> > Hi Krzysztof,
> > 
> > On 2024-02-17 3:00 AM, Krzysztof Kozlowski wrote:
> >> On 16/02/2024 01:08, Samuel Holland wrote:
> >>> The SiFive Composable Cache controller contains an optional PMU with a
> >>> configurable number of event counters. Document a property which
> >>
> >> Configurable in what context? By chip designers or by OS? Why this
> >> cannot be deduced from the compatible?
> > 
> > This parameter is configurable by the chip designers.
> > 
> > The information certainly can be deduced from the SoC-specific compatible
> > string, but doing so makes the driver only work on that specific list of SoCs.
> 
> Usually that's exactly what's expected, so why here usual approach is wrong?
> 
> > When provided via a property, the driver can work without changes on any SoC
> > that uses this IP block. (None of the SoCs currently listed in the binding
> 
> Sorry, properties are not a work-around for missing compatibles.
> 
> > contain a PMU, so there is no backward compatibility concern with adding the new
> > property.)
> > 
> > My understanding of the purpose of the SoC-specific compatible string is to
> > handle eventualities (silicon bugs, integration quirks, etc.), not to
> > intentionally limit the driver to a narrow list of hardware.
> 
> Depends what is the hardware. For most of licensed blocks, the final
> design is the hardware so equals to its compatible.

While I generally agree, I think a property is fine here for 2 reasons. 

This is going to vary on just about every design. That's true for any 
PMU. So maybe this shouldn't even be SiFfive specific.

The second is counters available to the OS may not equal the number in 
h/w because counters could be reserved for different priviledge levels 
(secure, hypervisor, guest, etc.). No idea if Risc-V supports this, but 
if not it is a matter of time. That's more likely for a core PMU than an 
uncore PMU.

Rob
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/cache/sifive,ccache0.yaml b/Documentation/devicetree/bindings/cache/sifive,ccache0.yaml
index 7e8cebe21584..100eda4345de 100644
--- a/Documentation/devicetree/bindings/cache/sifive,ccache0.yaml
+++ b/Documentation/devicetree/bindings/cache/sifive,ccache0.yaml
@@ -81,6 +81,11 @@  properties:
       The reference to the reserved-memory for the L2 Loosely Integrated Memory region.
       The reserved memory node should be defined as per the bindings in reserved-memory.txt.
 
+  sifive,perfmon-counters:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    default: 0
+    description: Number of PMU counter registers
+
 allOf:
   - $ref: /schemas/cache-controller.yaml#