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@xxxxxxxxxxxxx/ > > > > > > > > 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
