Date:   Thu, 24 Mar 2022 00:05:44 +0530
From:   Kuldeep Singh <singh.kuldeep87k@...il.com>
To:     Robin Murphy <robin.murphy@....com>, Rob Herring <robh@...nel.org>
Cc:     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 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.

> 
> 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

- Kuldeep

Powered by blists - more mailing lists