| Message ID | 20220316095433.20225-4-singh.kuldeep87k@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix dtbs warnings for arch timer | expand |
Hi Kuldeep, On Wed, 16 Mar 2022 09:54:32 +0000, Kuldeep Singh <singh.kuldeep87k@gmail.com> wrote: > > Few platforms such as Renesas RZ/N1D, Calxeda, Alpine etc. are using > arm,cortex-a15-timer and arm,cortex-a7-timer entries in conjugation with > arm,armv7-timer which are not currently defined in driver file. Add > these entries in arch_timer_of_match list to bring them in use. "arm,armv7-timer" already acts as a catch all (and is mandatory anyway), and there is no difference in handling as a result of this. So what is this fixing? Thanks, M.
On 16/03/2022 10:54, Kuldeep Singh wrote: > Few platforms such as Renesas RZ/N1D, Calxeda, Alpine etc. are using > arm,cortex-a15-timer and arm,cortex-a7-timer entries in conjugation with > arm,armv7-timer which are not currently defined in driver file. Add > these entries in arch_timer_of_match list to bring them in use. > This looks wrong (also Marc pointed this out) and rationale is not sufficient. Why do you need these compatibles in the driver? Best regards, Krzysztof
On Wed, Mar 16, 2022 at 05:30:26PM +0100, Krzysztof Kozlowski wrote: > On 16/03/2022 10:54, Kuldeep Singh wrote: > > Few platforms such as Renesas RZ/N1D, Calxeda, Alpine etc. are using > > arm,cortex-a15-timer and arm,cortex-a7-timer entries in conjugation with > > arm,armv7-timer which are not currently defined in driver file. Add > > these entries in arch_timer_of_match list to bring them in use. > > > > This looks wrong (also Marc pointed this out) and rationale is not > sufficient. Why do you need these compatibles in the driver? Hi Krzysztof and Marc, I find myself in trouble whenever dealing with compatible entries and had 2 options when I stumble this issue. 1. Remove unused compatible 2. Add required compatible to binding and driver My past experience and advise from other developer says not to remove an existing compatible. And also I found "arm,cortex-a15-timer" in binding which was again not documented and was present in DT. This prompted me to go for second option and make necessary additions in binding and driver following current entries. As per your perspective, current configuration isn't apt which means "arm,cortex-a15-timer" is a stub and is wrongly present in binding. I also observed many other DTs have compatibles which are not present in driver. What is an ideal idealogy behind such cases? - Kuldeep
On Wed, 16 Mar 2022 17:41:08 +0000, Kuldeep Singh <singh.kuldeep87k@gmail.com> wrote: > > On Wed, Mar 16, 2022 at 05:30:26PM +0100, Krzysztof Kozlowski wrote: > > On 16/03/2022 10:54, Kuldeep Singh wrote: > > > Few platforms such as Renesas RZ/N1D, Calxeda, Alpine etc. are using > > > arm,cortex-a15-timer and arm,cortex-a7-timer entries in conjugation with > > > arm,armv7-timer which are not currently defined in driver file. Add > > > these entries in arch_timer_of_match list to bring them in use. > > > > > > > This looks wrong (also Marc pointed this out) and rationale is not > > sufficient. Why do you need these compatibles in the driver? > > Hi Krzysztof and Marc, > > I find myself in trouble whenever dealing with compatible entries and > had 2 options when I stumble this issue. > 1. Remove unused compatible That'd be silly. > 2. Add required compatible to binding and driver To the binding, yes. But to the driver? > My past experience and advise from other developer says not to remove an > existing compatible. And also I found "arm,cortex-a15-timer" in binding > which was again not documented and was present in DT. This prompted me > to go for second option and make necessary additions in binding and > driver following current entries. The "arm,cortex-a15-timer" compatible is documentation, and only that. If, one day, we find a bug in this implementation, we could work around it in the driver thanks to the separate compatible (although in this case, we'd have much better way of doing that). > As per your perspective, current configuration isn't apt which means > "arm,cortex-a15-timer" is a stub and is wrongly present in binding. That's not what I said. This compatible string is perfectly fine, and accurately describe the HW. The driver doesn't need to know about the fine details of the implementation, and is perfectly happy with the current state of things. Think of it as an instance of a class. The driver doesn't need to know the instance, only that it is a certain class. > I also observed many other DTs have compatibles which are not present in > driver. What is an ideal idealogy behind such cases? I think I've made myself clear above. Thanks, M.
On Wed, Mar 16, 2022 at 06:43:15PM +0000, Marc Zyngier wrote: > On Wed, 16 Mar 2022 17:41:08 +0000, > Kuldeep Singh <singh.kuldeep87k@gmail.com> wrote: > > > > On Wed, Mar 16, 2022 at 05:30:26PM +0100, Krzysztof Kozlowski wrote: > > > On 16/03/2022 10:54, Kuldeep Singh wrote: > > > > Few platforms such as Renesas RZ/N1D, Calxeda, Alpine etc. are using > > > > arm,cortex-a15-timer and arm,cortex-a7-timer entries in conjugation with > > > > arm,armv7-timer which are not currently defined in driver file. Add > > > > these entries in arch_timer_of_match list to bring them in use. > > > > > > > > > > This looks wrong (also Marc pointed this out) and rationale is not > > > sufficient. Why do you need these compatibles in the driver? > > > > Hi Krzysztof and Marc, > > > > I find myself in trouble whenever dealing with compatible entries and > > had 2 options when I stumble this issue. > > 1. Remove unused compatible > > That'd be silly. > > > 2. Add required compatible to binding and driver > > To the binding, yes. But to the driver? > > > My past experience and advise from other developer says not to remove an > > existing compatible. And also I found "arm,cortex-a15-timer" in binding > > which was again not documented and was present in DT. This prompted me > > to go for second option and make necessary additions in binding and > > driver following current entries. > > The "arm,cortex-a15-timer" compatible is documentation, and only > that. If, one day, we find a bug in this implementation, we could work > around it in the driver thanks to the separate compatible (although in > this case, we'd have much better way of doing that). > > > As per your perspective, current configuration isn't apt which means > > "arm,cortex-a15-timer" is a stub and is wrongly present in binding. > > That's not what I said. This compatible string is perfectly fine, and > accurately describe the HW. The driver doesn't need to know about the > fine details of the implementation, and is perfectly happy with the > current state of things. > > Think of it as an instance of a class. The driver doesn't need to know > the instance, only that it is a certain class. > Thanks Marc for sharing knowledge. This was indeed helpful. To sum up from what I understood, bindings and DTs should always be in sync and driver file may not need to define all compatible entries as long as purpose is served. This means no driver change will be required to address "arm,cortex-a7-timer". To which I have a question to Krzysztof. Hi Krzysztof, As per your comments on 2/3 patch, that it's DT which is not aligned with binding w.r.t arm,cortex-a7-timer. What makes "arm,cortex-a7-timer" an invalid entry from binding perspective when we have a similar entry "arm,cortex-a15-timer" already present? I think we should share some common grounds here and keep both of them in bindings or remove them altogether. I prefer first option, What's your say? Or please let me know in case there's better way to address this. - Kuldeep > > I also observed many other DTs have compatibles which are not present in > > driver. What is an ideal idealogy behind such cases? > > I think I've made myself clear above. > > Thanks, > > M. > > -- > Without deviation from the norm, progress is not possible.
On 17/03/2022 07:59, Kuldeep Singh wrote: > On Wed, Mar 16, 2022 at 06:43:15PM +0000, Marc Zyngier wrote: >> On Wed, 16 Mar 2022 17:41:08 +0000, >> Kuldeep Singh <singh.kuldeep87k@gmail.com> wrote: >>> >>> On Wed, Mar 16, 2022 at 05:30:26PM +0100, Krzysztof Kozlowski wrote: >>>> On 16/03/2022 10:54, Kuldeep Singh wrote: >>>>> Few platforms such as Renesas RZ/N1D, Calxeda, Alpine etc. are using >>>>> arm,cortex-a15-timer and arm,cortex-a7-timer entries in conjugation with >>>>> arm,armv7-timer which are not currently defined in driver file. Add >>>>> these entries in arch_timer_of_match list to bring them in use. >>>>> >>>> >>>> This looks wrong (also Marc pointed this out) and rationale is not >>>> sufficient. Why do you need these compatibles in the driver? >>> >>> Hi Krzysztof and Marc, >>> >>> I find myself in trouble whenever dealing with compatible entries and >>> had 2 options when I stumble this issue. >>> 1. Remove unused compatible >> >> That'd be silly. >> >>> 2. Add required compatible to binding and driver >> >> To the binding, yes. But to the driver? >> >>> My past experience and advise from other developer says not to remove an >>> existing compatible. And also I found "arm,cortex-a15-timer" in binding >>> which was again not documented and was present in DT. This prompted me >>> to go for second option and make necessary additions in binding and >>> driver following current entries. >> >> The "arm,cortex-a15-timer" compatible is documentation, and only >> that. If, one day, we find a bug in this implementation, we could work >> around it in the driver thanks to the separate compatible (although in >> this case, we'd have much better way of doing that). >> >>> As per your perspective, current configuration isn't apt which means >>> "arm,cortex-a15-timer" is a stub and is wrongly present in binding. >> >> That's not what I said. This compatible string is perfectly fine, and >> accurately describe the HW. The driver doesn't need to know about the >> fine details of the implementation, and is perfectly happy with the >> current state of things. >> >> Think of it as an instance of a class. The driver doesn't need to know >> the instance, only that it is a certain class. >> > > Thanks Marc for sharing knowledge. This was indeed helpful. > To sum up from what I understood, bindings and DTs should always be in > sync and driver file may not need to define all compatible entries as > long as purpose is served. > > This means no driver change will be required to address > "arm,cortex-a7-timer". To which I have a question to Krzysztof. > > Hi Krzysztof, > > As per your comments on 2/3 patch, that it's DT which is not aligned > with binding w.r.t arm,cortex-a7-timer. > > What makes "arm,cortex-a7-timer" an invalid entry from binding > perspective when we have a similar entry "arm,cortex-a15-timer" already > present? > > I think we should share some common grounds here and keep both of them > in bindings or remove them altogether. I prefer first option, What's > your say? In this case the compatible should be added, just please explain it in the message. Your previous commit msg was saying about disastrous backward compatibility issue which so far does not exist here. It's simply more detailed compatible. There were few other cases where more detailed compatible was actually unwanted, so that's why each case should be analyzed. Best regards, Krzysztof
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index 1ecd52f903b8..805e60c09916 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -1266,6 +1266,8 @@ static int __init arch_timer_mem_register(void __iomem *base, unsigned int irq) static const struct of_device_id arch_timer_of_match[] __initconst = { { .compatible = "arm,armv7-timer", }, { .compatible = "arm,armv8-timer", }, + { .compatible = "arm,cortex-a7-timer", }, + { .compatible = "arm,cortex-a15-timer", }, {}, };
Few platforms such as Renesas RZ/N1D, Calxeda, Alpine etc. are using arm,cortex-a15-timer and arm,cortex-a7-timer entries in conjugation with arm,armv7-timer which are not currently defined in driver file. Add these entries in arch_timer_of_match list to bring them in use. Signed-off-by: Kuldeep Singh <singh.kuldeep87k@gmail.com> --- drivers/clocksource/arm_arch_timer.c | 2 ++ 1 file changed, 2 insertions(+)