Re: [PATCH v3 2/2] dt-bindings: perf: starfive: Add StarLink PMU

[Ji Sheng Teoh]

On Thu, 16 Nov 2023 14:34:00 +0000
Conor Dooley <conor@xxxxxxxxxx> wrote:

> On Thu, Nov 16, 2023 at 10:10:35AM +0800, Ji Sheng Teoh wrote:
> > On Wed, 15 Nov 2023 20:03:53 +0000
> > Conor Dooley <conor@xxxxxxxxxx> wrote:  
> > > On Wed, Nov 15, 2023 at 11:36:08AM +0800, Ji Sheng Teoh wrote:  
> > > > Add device tree binding for StarFive's StarLink PMU (Performance
> > > > Monitor Unit).
> > > > 
> > > > Signed-off-by: Ji Sheng Teoh <jisheng.teoh@xxxxxxxxxxxxxxxx>
> > > > ---
> > > >  .../bindings/perf/starfive,starlink-pmu.yaml  | 46
> > > > +++++++++++++++++++ 1 file changed, 46 insertions(+)
> > > >  create mode 100644
> > > > Documentation/devicetree/bindings/perf/starfive,starlink-pmu.yaml
> > > > 
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/perf/starfive,starlink-pmu.yaml
> > > > b/Documentation/devicetree/bindings/perf/starfive,starlink-pmu.yaml
> > > > new file mode 100644 index 000000000000..a9426a7faeae ---
> > > > /dev/null +++
> > > > b/Documentation/devicetree/bindings/perf/starfive,starlink-pmu.yaml
> > > >    
> > > 
> > > btw, since you changed the compatible, the filename should have
> > > been changed to match it.  
> > 
> > The intention to keep the filename generic is to allow addition of
> > new version of StarLink PMU in future if any, similar to what
> > arm,cmn.yaml is doing. Hope that makes sense.  
> 
> No, please keep the filename matching the compatible. Even if the
> filename contains "500", there's nothing stopping you from then adding
> other pmu variants. There are many many examples of this in the tree.
> 

Sure, will do that in v4. 

> > > > @@ -0,0 +1,46 @@
> > > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > > +%YAML 1.2
> > > > +---
> > > > +$id:
> > > > http://devicetree.org/schemas/perf/starfive,starlink-pmu.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# +
> > > > +title: StarFive StarLink PMU
> > > > +
> > > > +maintainers:
> > > > +  - Ji Sheng Teoh <jisheng.teoh@xxxxxxxxxxxxxxxx>
> > > > +
> > > > +description:
> > > > +  StarFive's StarLink PMU integrates one or more CPU cores
> > > > with a shared L3
> > > > +  memory system. The PMU support overflow interrupt, up to 16
> > > > programmable
> > > > +  64bit event counters, and an independent 64bit cycle counter.
> > > > +  StarLink PMU is accessed via MMIO.
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    const: starfive,starlink-500-pmu    
> > > 
> > > So this is not what I had in mind by a "device". I was looking
> > > for a compatible representing an soc in which this IP had been
> > > integrated. A soc-specific compatible, rather than something
> > > generic, is requirement for devicetree - we don't want various
> > > integrations of this IP to all be using a generic compatible when
> > > there may be subtle (or less subtle) differences between
> > > integrations.
> > > 
> > > I'm trying to come up with the syntax for enforcing having two
> > > compatibles with your current one as the fallback, but I have yet
> > > to come up with the correct syntax for that that works correctly.
> > > 
> > > Hopefully by the time you get some feedback on the driver side of
> > > this submission I will have a concrete suggestion for what to do
> > > here.  
> > 
> > Thanks Conor for the enlightenment. In the meantime, to fit the
> > requirement I would suggest going for
> > "starfive,jh8100-starlink-pmu", making it JH8100 SOC specific if
> > that makes sense.  
> 
> Okay, you could definitely do that!
> 
> Cheers,
> Conor.
> 

Ok, will use that in v4. Thanks!