[v2,2/3] dt-bindings: timer: Document arm,cortex-a7-timer in arch timer

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.

Signed-off-by: Kuldeep Singh <singh.kuldeep87k@gmail.com>
---
 Documentation/devicetree/bindings/timer/arm,arch_timer.yaml | 1 +
 1 file changed, 1 insertion(+)

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.

> Signed-off-by: Kuldeep Singh <singh.kuldeep87k@gmail.com>
> ---
>   Documentation/devicetree/bindings/timer/arm,arch_timer.yaml | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml b/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml
> index ba2910f0a7b2..ea390e5df71d 100644
> --- a/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml
> +++ b/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml
> @@ -26,6 +26,7 @@ properties:
>             - arm,armv8-timer
>         - items:
>             - enum:
> +              - arm,cortex-a7-timer
>                 - arm,cortex-a15-timer
>             - const: arm,armv7-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