| Message ID | 20230213231931.6546-3-bage@debian.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable hwlock on Allwinner A64 | expand |
On Tue, 14 Feb 2023 00:19:29 +0100 Bastian Germann <bage@debian.org> wrote: > The allwinner,sun6i-a31-hwspinlock.yaml binding needs clock-names > and reset-names set to "ahb" as required by the driver. That should read "Linux driver", and is technically not a good rationale to change a DT binding, but I guess the Linux kernel is the only user so far, so the change should be fine: > Fixes: f9e784dcb63f ("dt-bindings: hwlock: add sun6i_hwspinlock") > Signed-off-by: Bastian Germann <bage@debian.org> Reviewed-by: Andre Przywara <andre.przywara@arm.com> Cheers, Andre > --- > .../hwlock/allwinner,sun6i-a31-hwspinlock.yaml | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/Documentation/devicetree/bindings/hwlock/allwinner,sun6i-a31-hwspinlock.yaml b/Documentation/devicetree/bindings/hwlock/allwinner,sun6i-a31-hwspinlock.yaml > index 01b1bbb3061f..1f11d9580646 100644 > --- a/Documentation/devicetree/bindings/hwlock/allwinner,sun6i-a31-hwspinlock.yaml > +++ b/Documentation/devicetree/bindings/hwlock/allwinner,sun6i-a31-hwspinlock.yaml > @@ -23,9 +23,17 @@ properties: > clocks: > maxItems: 1 > > + clock-names: > + items: > + - const: ahb > + > resets: > maxItems: 1 > > + reset-names: > + items: > + - const: ahb > + > '#hwlock-cells': > const: 1 > > @@ -33,7 +41,9 @@ required: > - compatible > - reg > - clocks > + - clock-names > - resets > + - reset-names > - "#hwlock-cells" > > additionalProperties: false
On 14/02/2023 00:19, Bastian Germann wrote: > The allwinner,sun6i-a31-hwspinlock.yaml binding needs clock-names > and reset-names set to "ahb" as required by the driver. > > Fixes: f9e784dcb63f ("dt-bindings: hwlock: add sun6i_hwspinlock") > Signed-off-by: Bastian Germann <bage@debian.org> > --- With the changes from Andre: Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On Tue, 14 Feb 2023 00:19:29 +0100 Bastian Germann <bage@debian.org> wrote: > The allwinner,sun6i-a31-hwspinlock.yaml binding needs clock-names > and reset-names set to "ahb" as required by the driver. Hmm, this one is a bit odd. If you look into my earlier versions of the patchset, you may notice, that I actually included these bindings and they were refused. I think the argumentation was like "there is only one bus = no need for it". If it gets accepted now, I really like to know why. (It was some trouble back then to get the documentation properly done and accepted.) greetings, Will
On Tue, 14 Feb 2023 09:45:54 +0000 Wilken Gottwalt <wilken.gottwalt@posteo.net> wrote: Hi, > On Tue, 14 Feb 2023 00:19:29 +0100 > Bastian Germann <bage@debian.org> wrote: > > > The allwinner,sun6i-a31-hwspinlock.yaml binding needs clock-names > > and reset-names set to "ahb" as required by the driver. > > Hmm, this one is a bit odd. If you look into my earlier versions of the > patchset, you may notice, that I actually included these bindings and they > were refused. I think the argumentation was like > "there is only one bus = no need for it". That's interesting, because your driver implementation relies on there being a clock name. And if I chased down devm_clk_get() correctly, there must be a named clock in the DT, otherwise it would fail? I haven't tested this, though, but I guess this is the reason for Bastian's patch. Regarding "one bus clock only": while this is true, I think there is (or was?) also the rationale of using names being more future-proof, so adding clocks (for future hardware revisions) can be done more easily, without breaking compatibility. It's not a big problem, since you probably have a new compatible string in this case anyway, but it also doesn't hurt, and allows to use more generic helpers like devm_clk_get(). > If it gets accepted now, I really like to know why. (It was some trouble > back then to get the documentation properly done and accepted.) IIUC, it simply doesn't work without a clock-names property. Cheers, Andre
On 14/02/2023 10:45, Wilken Gottwalt wrote: > On Tue, 14 Feb 2023 00:19:29 +0100 > Bastian Germann <bage@debian.org> wrote: > >> The allwinner,sun6i-a31-hwspinlock.yaml binding needs clock-names >> and reset-names set to "ahb" as required by the driver. > > Hmm, this one is a bit odd. If you look into my earlier versions of the > patchset, you may notice, that I actually included these bindings and they > were refused. I think the argumentation was like > "there is only one bus = no need for it". > > If it gets accepted now, I really like to know why. (It was some trouble > back then to get the documentation properly done and accepted.) The clock names and resent names are not correct. They should have never been added. If you got comments about this and did not update driver, that's not nice. You just shoved incomplete bindings. :( So indeed to avoid precedence - people pushing fake bindings and avoiding review - NAK on this. Best regards, Krzysztof
On 14/02/2023 00:19, Bastian Germann wrote: > The allwinner,sun6i-a31-hwspinlock.yaml binding needs clock-names > and reset-names set to "ahb" as required by the driver. > > Fixes: f9e784dcb63f ("dt-bindings: hwlock: add sun6i_hwspinlock") NAK. Original driver should have been changed, as requested during review. Since the driver was not changed, we must not accept bindings post factum. Best regards, Krzysztof
On 14/02/2023 13:12, Andre Przywara wrote: > On Tue, 14 Feb 2023 09:45:54 +0000 > Wilken Gottwalt <wilken.gottwalt@posteo.net> wrote: > > Hi, > >> On Tue, 14 Feb 2023 00:19:29 +0100 >> Bastian Germann <bage@debian.org> wrote: >> >>> The allwinner,sun6i-a31-hwspinlock.yaml binding needs clock-names >>> and reset-names set to "ahb" as required by the driver. >> >> Hmm, this one is a bit odd. If you look into my earlier versions of the >> patchset, you may notice, that I actually included these bindings and they >> were refused. I think the argumentation was like >> "there is only one bus = no need for it". > > That's interesting, because your driver implementation relies on there > being a clock name. And if I chased down devm_clk_get() correctly, there > must be a named clock in the DT, otherwise it would fail? I haven't tested > this, though, but I guess this is the reason for Bastian's patch. > > Regarding "one bus clock only": while this is true, I think there > is (or was?) also the rationale of using names being more future-proof, so > adding clocks (for future hardware revisions) can be done more easily, > without breaking compatibility. It's not a big problem, since you probably > have a new compatible string in this case anyway, but it also doesn't > hurt, and allows to use more generic helpers like devm_clk_get(). > >> If it gets accepted now, I really like to know why. (It was some trouble >> back then to get the documentation properly done and accepted.) > > IIUC, it simply doesn't work without a clock-names property. Oh, there is clear way. One can ignore review and push mismatched bindings/drivers. Incomplete binding to satisfy DT maintainers and driver doing something entirely else (using undocumented properties). Best regards, Krzysztof
On Tue, 14 Feb 2023 13:37:20 +0100 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: Hi Krzysztof, > On 14/02/2023 10:45, Wilken Gottwalt wrote: > > On Tue, 14 Feb 2023 00:19:29 +0100 > > Bastian Germann <bage@debian.org> wrote: > > > >> The allwinner,sun6i-a31-hwspinlock.yaml binding needs clock-names > >> and reset-names set to "ahb" as required by the driver. > > > > Hmm, this one is a bit odd. If you look into my earlier versions of the > > patchset, you may notice, that I actually included these bindings and they > > were refused. I think the argumentation was like > > "there is only one bus = no need for it". > > > > If it gets accepted now, I really like to know why. (It was some trouble > > back then to get the documentation properly done and accepted.) > > The clock names and resent names are not correct. They should have never > been added. If you got comments about this and did not update driver, > that's not nice. You just shoved incomplete bindings. :( > > So indeed to avoid precedence - people pushing fake bindings and > avoiding review - NAK on this. Maybe it's just me, but I don't think this tone is necessary. Wilken's original submission was correct. Later there was a comment just on the binding patch, to remove the not needed clock-names and reset-names properties. But there was not a word in there that the driver requires changing as well, and I don't think it's fair to blame Wilken on this, or somewhat even implying intention. There were several patch revisions after this was raised, and this just slipped through review. But surely no one wanted this or pushed for that. If anything, it tells us that we should be more careful when merging drivers without users: if there would have been a DT patch, possibly even a consumer, this would have been flagged by dtbs_check. Cheers, Andre
On 14/02/2023 19:11, Andre Przywara wrote: > On Tue, 14 Feb 2023 13:37:20 +0100 > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > Hi Krzysztof, > >> On 14/02/2023 10:45, Wilken Gottwalt wrote: >>> On Tue, 14 Feb 2023 00:19:29 +0100 >>> Bastian Germann <bage@debian.org> wrote: >>> >>>> The allwinner,sun6i-a31-hwspinlock.yaml binding needs clock-names >>>> and reset-names set to "ahb" as required by the driver. >>> >>> Hmm, this one is a bit odd. If you look into my earlier versions of the >>> patchset, you may notice, that I actually included these bindings and they >>> were refused. I think the argumentation was like >>> "there is only one bus = no need for it". >>> >>> If it gets accepted now, I really like to know why. (It was some trouble >>> back then to get the documentation properly done and accepted.) >> >> The clock names and resent names are not correct. They should have never >> been added. If you got comments about this and did not update driver, >> that's not nice. You just shoved incomplete bindings. :( >> >> So indeed to avoid precedence - people pushing fake bindings and >> avoiding review - NAK on this. > > Maybe it's just me, but I don't think this tone is necessary. > > Wilken's original submission was correct. Later there was a comment just > on the binding patch, to remove the not needed clock-names and reset-names > properties. But there was not a word in there that the driver requires > changing as well, and I don't think it's fair to blame Wilken on this, or > somewhat even implying intention. There were several patch revisions after > this was raised, and this just slipped through review. But surely no one > wanted this or pushed for that. I would say it is quite obvious. Otherwise you could remove entire binding and still submit the driver, right? Isn't the entire point of the binding to match what the driver is doing, as it is the description of interface used by driver towards DTS? > > If anything, it tells us that we should be more careful when merging > drivers without users: if there would have been a DT patch, possibly even > a consumer, this would have been flagged by dtbs_check. Sure. To me it tells - this patch is a no-go and driver should be fixed. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/hwlock/allwinner,sun6i-a31-hwspinlock.yaml b/Documentation/devicetree/bindings/hwlock/allwinner,sun6i-a31-hwspinlock.yaml index 01b1bbb3061f..1f11d9580646 100644 --- a/Documentation/devicetree/bindings/hwlock/allwinner,sun6i-a31-hwspinlock.yaml +++ b/Documentation/devicetree/bindings/hwlock/allwinner,sun6i-a31-hwspinlock.yaml @@ -23,9 +23,17 @@ properties: clocks: maxItems: 1 + clock-names: + items: + - const: ahb + resets: maxItems: 1 + reset-names: + items: + - const: ahb + '#hwlock-cells': const: 1 @@ -33,7 +41,9 @@ required: - compatible - reg - clocks + - clock-names - resets + - reset-names - "#hwlock-cells" additionalProperties: false
The allwinner,sun6i-a31-hwspinlock.yaml binding needs clock-names and reset-names set to "ahb" as required by the driver. Fixes: f9e784dcb63f ("dt-bindings: hwlock: add sun6i_hwspinlock") Signed-off-by: Bastian Germann <bage@debian.org> --- .../hwlock/allwinner,sun6i-a31-hwspinlock.yaml | 10 ++++++++++ 1 file changed, 10 insertions(+)