Message-ID: <Yj4yxjQ2rQ4HW4uR@robh.at.kernel.org>
Date:   Fri, 25 Mar 2022 16:23:18 -0500
From:   Rob Herring <robh@...nel.org>
To:     Kuldeep Singh <singh.kuldeep87k@...il.com>
Cc:     Robin Murphy <robin.murphy@....com>, Marc Zyngier <maz@...nel.org>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Marc Zyngier <marc.zyngier@....com>,
        Mark Rutland <mark.rutland@....com>,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 2/3] dt-bindings: timer: Document arm, cortex-a7-timer
 in arch timer

On Thu, Mar 24, 2022 at 12:05:44AM +0530, Kuldeep Singh wrote:
> On Mon, Mar 21, 2022 at 11:52:27AM +0000, Robin Murphy wrote:
> > On 2022-03-20 18:47, Rob Herring wrote:
> > > On Fri, Mar 18, 2022 at 02:55:08AM +0530, Kuldeep Singh wrote:
> > > > On Thu, Mar 17, 2022 at 08:25:12PM +0000, Robin Murphy wrote:
> > > > > On 2022-03-17 19:15, Kuldeep Singh wrote:
> > > > > > Renesas RZ/N1D platform uses compatible "arm,cortex-a7-timer" in
> > > > > > conjugation with "arm,armv7-timer". Since, initial entry is not
> > > > > > documented, it start raising dtbs_check warnings.
> > > > > > 
> > > > > > ['arm,cortex-a7-timer', 'arm,armv7-timer'] is too long
> > > > > > 'arm,cortex-a7-timer' is not one of ['arm,armv7-timer', 'arm,armv8-timer']
> > > > > > 'arm,cortex-a7-timer' is not one of ['arm,cortex-a15-timer']
> > > > > > 
> > > > > > Document this compatible to address it. The motivation to add this
> > > > > > change is taken from an already existing entry "arm,cortex-a15-timer".
> > > > > > Please note, this will not hurt any arch timer users.
> > > > > 
> > > > > Eh, if it's never been documented or supported, I say just get rid of it.
> > > > > The arch timer interface is by definition part of a CPU, and we can tell
> > > > > what the CPU is by reading its ID registers. Indeed that's how the driver
> > > > > handles the non-zero number of CPU-specific errata that already exist - we
> > > > > don't need compatibles for that.
> > > > > 
> > > > > In some ways it might have been nice to have *SoC-specific* compatibles
> > > > > given the difficulty some integrators seem to have had in wiring up a stable
> > > > > count *to* the interface, but it's not like they could be magically added to
> > > > > already-deployed DTs after a bug is discovered, and nor could we have
> > > > > mandated them from day 1 just in case and subsequently maintained a binding
> > > > > that is just an ever-growing list of every SoC. Oh well.
> > > > 
> > > > Robin, A similar discussion was already done on v1 thread. Please see
> > > > below for details:
> > > > https://lore.kernel.org/linux-devicetree/20220317065925.GA9158@9a2d8922b8f1/
> > > > https://lore.kernel.org/linux-devicetree/726bde76-d792-febf-d364-6eedeb748c3b@canonical.com/
> > > > 
> > > > And final outcome of discussion turns out to add this compatible string.
> > > 
> > > I agree with Robin on dropping. More specific here is not useful. If
> > > we're going to add some cores, then we should add every core
> > > implementation.
> 
> Sure Rob, I will drop A7/15-timer entry from compatibles.
> This means only two entries i.e arm,armv7/8-timer will be there under
> compatibles now.
> 
> I actually added A7-timer because A15-timer was already present in
> binding. Since, it was added by you that's why I added this one.
> I will update compatibles accordingly as you said above.

The A15 compatible is likely there because upstream dts files used it 
and it's a judgement call of supporting in the schema vs. making dts 
changes. Just like the PL022. Maybe there are A7 cases, but fewer to 
fix. I don't remember.

So no real object to removing it, but I can think of better things to 
work on. Here's a list of most occurring compatibles with no schema[1]. 
Or find a platform and work towards getting 0 warnings.


> > Yeah, what I was trying to convey is that a compatible like
> > "arm,cortex-a76-timer" has the problem of being both too specific *and* not
> > specific enough to be genuinely useful *for the particular case of the arch
> > timer*. It's an architectural interface, where the actual functional
> > features are described through the interface itself, so the purpose of the
> > DT entry is really just to indicate that the standard interface is present
> > and describe how its externally-routed interrupts are wired up.
> > 
> > However, it's also true that implementations of standard functionality
> > sometimes have bugs that software needs to know about, but in order for
> > specific DT compatibles to be useful in that respect they really need to
> > identify the *exact* implementation, e.g. to know that
> > "arm,cortex-a76-r0p0-timer" has a bug which needs working around, but
> > "arm,cortex-a76-r4p0-timer" does not. There might be cases where every known
> > version of a CPU is equally affected (e.g. Cortex-A73), but it doesn't hold
> > as a general assumption. Furthermore as mentioned, the other class of bugs
> > which affect this interface are not in the CPU's implementation of the
> > interface at all, but in the external SoC logic that provides the counter
> > value, and therefore it can be identification of the overall SoC that
> > matters regardless of which CPU IP(s) may be present.
> > 
> > If we'd had the benefit of 10 years worth of hindsight 10 years ago, we
> > probably wouldn't have defined "arm,cortex-a15-timer" either. However the
> > fact that we can't erase the legacy of that decision doesn't make an
> > argument for repeating it now.
> > 
> > > If one has a big.LITTLE system with A15/A7 what would be the right
> > > compatible value?
> > > 
> > > > 
> > > > I see people with different set of perspective in regard to whether keep
> > > > compatible string or not. We should have some sort of evidences to
> > > > support claims so that next time when similar situation arises, we'll be
> > > > aware beforehand how to proceed.
> > > 
> > > Every situation tends to be different.
> > 
> > Indeed, I certainly don't have a personal perspective of "delete all the
> > bindings!" in general - only when they're truly redundant (functionally, any
> > driver that can touch the arch timer registers can also read the CPU ID
> > registers, but even in the DT there should already be compatibles for the
> > CPUs themselves).
> 
> Thanks Robin for providing inputs.
> I agree with your opinion of having soc specific compatibles which is
> also mentioned under dos and dont's of bindings and other cases will
> require investigation though.
> https://www.kernel.org/doc/html/latest/devicetree/bindings/writing-bindings.html

There's always exceptions to guidelines. This is one of them.

Rob

[1] https://gitlab.com/robherring/linux-dt/-/jobs/2250856818#L7769

Powered by blists - more mailing lists