[PATCH 0/3] Enable hwlock on Allwinner A64

[PATCH 0/3] Enable hwlock on Allwinner A64

[Bastian Germann]

The allwinner,sun6i-a31-hwspinlock compatible driver can be used with
the Allwinner A64 chip. Add the wiring required to enable it.

The device tree schema needs some work to verify everything that is
needed by the sun6i hwlock driver.

The hwlock device was verified to be available with this series applied
on a Pinebook.

Bastian Germann (3):
  dt-bindings: hwlock: sun6i: Add missing #hwlock-cells
  dt-bindings: hwlock: sun6i: Add missing names
  arm64: dts: allwinner: a64: Add hwspinlock node

 .../hwlock/allwinner,sun6i-a31-hwspinlock.yaml     | 14 ++++++++++++++
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi      | 10 ++++++++++
 2 files changed, 24 insertions(+)

-- 
2.39.1

[PATCH 1/3] dt-bindings: hwlock: sun6i: Add missing #hwlock-cells

[Bastian Germann]

The allwinner,sun6i-a31-hwspinlock.yaml binding needs #hwlock-cells
which is required by the hwlock type.

Fixes: f9e784dcb63f ("dt-bindings: hwlock: add sun6i_hwspinlock")
Signed-off-by: Bastian Germann <bage@debian.org>
---
 .../bindings/hwlock/allwinner,sun6i-a31-hwspinlock.yaml       | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwlock/allwinner,sun6i-a31-hwspinlock.yaml b/Documentation/devicetree/bindings/hwlock/allwinner,sun6i-a31-hwspinlock.yaml
index 10e5a53e447b..01b1bbb3061f 100644
--- a/Documentation/devicetree/bindings/hwlock/allwinner,sun6i-a31-hwspinlock.yaml
+++ b/Documentation/devicetree/bindings/hwlock/allwinner,sun6i-a31-hwspinlock.yaml
@@ -26,11 +26,15 @@ properties:
   resets:
     maxItems: 1
 
+  '#hwlock-cells':
+    const: 1
+
 required:
   - compatible
   - reg
   - clocks
   - resets
+  - "#hwlock-cells"
 
 additionalProperties: false
 
-- 
2.39.1

[PATCH 2/3] dt-bindings: hwlock: sun6i: Add missing names

[Bastian Germann]

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(+)

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

[PATCH 3/3] arm64: dts: allwinner: a64: Add hwspinlock node

[Bastian Germann]

Add the hwspinlock to A64 which is already implemented for A31.

Signed-off-by: Bastian Germann <bage@debian.org>
---
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index 77b5349f6087..d4be8be0f2f3 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -851,6 +851,16 @@ wdt0: watchdog@1c20ca0 {
 			clocks = <&osc24M>;
 		};
 
+		hwspinlock: hwlock@1c18000 {
+			compatible = "allwinner,sun6i-a31-hwspinlock";
+			reg = <0x01c18000 0x1000>;
+			clocks = <&ccu CLK_BUS_SPINLOCK>;
+			clock-names = "ahb";
+			resets = <&ccu RST_BUS_SPINLOCK>;
+			reset-names = "ahb";
+			#hwlock-cells = <1>;
+		};
+
 		spdif: spdif@1c21000 {
 			#sound-dai-cells = <0>;
 			compatible = "allwinner,sun50i-a64-spdif",
-- 
2.39.1

Re: [PATCH 1/3] dt-bindings: hwlock: sun6i: Add missing #hwlock-cells

[Andre Przywara]

On Tue, 14 Feb 2023 00:19:28 +0100
Bastian Germann <bage@debian.org> wrote:

> The allwinner,sun6i-a31-hwspinlock.yaml binding needs #hwlock-cells
> which is required by the hwlock type.
> 
> 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

> ---
>  .../bindings/hwlock/allwinner,sun6i-a31-hwspinlock.yaml       | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwlock/allwinner,sun6i-a31-hwspinlock.yaml b/Documentation/devicetree/bindings/hwlock/allwinner,sun6i-a31-hwspinlock.yaml
> index 10e5a53e447b..01b1bbb3061f 100644
> --- a/Documentation/devicetree/bindings/hwlock/allwinner,sun6i-a31-hwspinlock.yaml
> +++ b/Documentation/devicetree/bindings/hwlock/allwinner,sun6i-a31-hwspinlock.yaml
> @@ -26,11 +26,15 @@ properties:
>    resets:
>      maxItems: 1
>  
> +  '#hwlock-cells':
> +    const: 1
> +
>  required:
>    - compatible
>    - reg
>    - clocks
>    - resets
> +  - "#hwlock-cells"
>  
>  additionalProperties: false
>  

Re: [PATCH 2/3] dt-bindings: hwlock: sun6i: Add missing names

[Andre Przywara]

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

Re: [PATCH 3/3] arm64: dts: allwinner: a64: Add hwspinlock node

[Andre Przywara]

On Tue, 14 Feb 2023 00:19:30 +0100
Bastian Germann <bage@debian.org> wrote:

Hi,

> Add the hwspinlock to A64 which is already implemented for A31.
> 
> Signed-off-by: Bastian Germann <bage@debian.org>
> ---
>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> index 77b5349f6087..d4be8be0f2f3 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> @@ -851,6 +851,16 @@ wdt0: watchdog@1c20ca0 {
>  			clocks = <&osc24M>;
>  		};
>  
> +		hwspinlock: hwlock@1c18000 {
> +			compatible = "allwinner,sun6i-a31-hwspinlock";

I wonder if we should add a more specific compatible string here
(sun50i-a64-hwspinlock), with the existing one as a fallback.

Checked the address and the clock/reset against the manual.

I am not entirely sure if it's the best practice to unconditionally
enable those devices, but since it needs explicit consumers to be used,
it's probably fine.

Cheers,
Andre


> +			reg = <0x01c18000 0x1000>;
> +			clocks = <&ccu CLK_BUS_SPINLOCK>;
> +			clock-names = "ahb";
> +			resets = <&ccu RST_BUS_SPINLOCK>;
> +			reset-names = "ahb";
> +			#hwlock-cells = <1>;
> +		};
> +
>  		spdif: spdif@1c21000 {
>  			#sound-dai-cells = <0>;
>  			compatible = "allwinner,sun50i-a64-spdif",

Re: [PATCH 0/3] Enable hwlock on Allwinner A64

[Andre Przywara]

On Tue, 14 Feb 2023 00:19:27 +0100
Bastian Germann <bage@debian.org> wrote:

Hi Bastian,

> The allwinner,sun6i-a31-hwspinlock compatible driver can be used with
> the Allwinner A64 chip. Add the wiring required to enable it.
> 
> The device tree schema needs some work to verify everything that is
> needed by the sun6i hwlock driver.
> 
> The hwlock device was verified to be available with this series applied
> on a Pinebook.

The changes look fine, but are there any actual users of this device?
The missing binding bits you fixed demonstrate that a driver/binding
without actual users can create problems.
So are there Linux/Crust patches out there that use this device?

Cheers,
Andre

> 
> Bastian Germann (3):
>   dt-bindings: hwlock: sun6i: Add missing #hwlock-cells
>   dt-bindings: hwlock: sun6i: Add missing names
>   arm64: dts: allwinner: a64: Add hwspinlock node
> 
>  .../hwlock/allwinner,sun6i-a31-hwspinlock.yaml     | 14 ++++++++++++++
>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi      | 10 ++++++++++
>  2 files changed, 24 insertions(+)
> 

Re: [PATCH 1/3] dt-bindings: hwlock: sun6i: Add missing #hwlock-cells

[Krzysztof Kozlowski]

On 14/02/2023 00:19, Bastian Germann wrote:
> The allwinner,sun6i-a31-hwspinlock.yaml binding needs #hwlock-cells
> which is required by the hwlock type.
> 
> Fixes: f9e784dcb63f ("dt-bindings: hwlock: add sun6i_hwspinlock")
> Signed-off-by: Bastian Germann <bage@debian.org>


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 2/3] dt-bindings: hwlock: sun6i: Add missing names

[Krzysztof Kozlowski]

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

Re: [PATCH 2/3] dt-bindings: hwlock: sun6i: Add missing names

[Wilken Gottwalt]

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

Re: [PATCH 2/3] dt-bindings: hwlock: sun6i: Add missing names

[Andre Przywara]

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

Re: [PATCH 2/3] dt-bindings: hwlock: sun6i: Add missing names

[Krzysztof Kozlowski]

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

Re: [PATCH 2/3] dt-bindings: hwlock: sun6i: Add missing names

[Krzysztof Kozlowski]

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

Re: [PATCH 2/3] dt-bindings: hwlock: sun6i: Add missing names

[Krzysztof Kozlowski]

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

Re: [PATCH 1/3] dt-bindings: hwlock: sun6i: Add missing #hwlock-cells

[Rob Herring]

On Tue, 14 Feb 2023 00:19:28 +0100, Bastian Germann wrote:
> The allwinner,sun6i-a31-hwspinlock.yaml binding needs #hwlock-cells
> which is required by the hwlock type.
> 
> Fixes: f9e784dcb63f ("dt-bindings: hwlock: add sun6i_hwspinlock")
> Signed-off-by: Bastian Germann <bage@debian.org>
> ---
>  .../bindings/hwlock/allwinner,sun6i-a31-hwspinlock.yaml       | 4 ++++
>  1 file changed, 4 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/hwlock/allwinner,sun6i-a31-hwspinlock.example.dtb: hwlock@1c18000: '#hwlock-cells' is a required property
	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/hwlock/allwinner,sun6i-a31-hwspinlock.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230213231931.6546-2-bage@debian.org

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 2/3] dt-bindings: hwlock: sun6i: Add missing names

[Andre Przywara]

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

Re: (subset) [PATCH 0/3] Enable hwlock on Allwinner A64

[Bjorn Andersson]

On Tue, 14 Feb 2023 00:19:27 +0100, Bastian Germann wrote:
> The allwinner,sun6i-a31-hwspinlock compatible driver can be used with
> the Allwinner A64 chip. Add the wiring required to enable it.
> 
> The device tree schema needs some work to verify everything that is
> needed by the sun6i hwlock driver.
> 
> The hwlock device was verified to be available with this series applied
> on a Pinebook.
> 
> [...]

Applied, thanks!

[1/3] dt-bindings: hwlock: sun6i: Add missing #hwlock-cells
      commit: 25d10262ed8aae71cd2f0b3c0a90d06a6b79fba2

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>

Re: [PATCH 0/3] Enable hwlock on Allwinner A64

[Bastian Germann]

Am 14.02.23 um 01:18 schrieb Andre Przywara:
> So are there Linux/Crust patches out there that use this device?

I do not know of any. I was just investigating the chip and checked the parts that I could easily use in Linux.

Re: [PATCH 0/3] Enable hwlock on Allwinner A64

[Wilken Gottwalt]

On Tue, 14 Feb 2023 22:55:45 +0100
Bastian Germann <bage@debian.org> wrote:

> Am 14.02.23 um 01:18 schrieb Andre Przywara:
> > So are there Linux/Crust patches out there that use this device?
> 
> I do not know of any. I was just investigating the chip and checked the parts that I could easily
> use in Linux.

Easily using it is, well, a bit questionable. The only reason to use this is
for syncing access to hardware between the ARM and the embedded OpenRISC core.
Like Andre wrote, it only makes sense if you want to etablish some form of
shared access between Linux and the Crust firmware. Using this in Linux only
instead of the common spinlocks won't be fun, that hwspinlock unit is slower.

greetings,
Will

Re: [PATCH 1/3] dt-bindings: hwlock: sun6i: Add missing #hwlock-cells

[Rob Herring]

On Mon, Feb 13, 2023 at 5:20 PM Bastian Germann <bage@debian.org> wrote:
>
> The allwinner,sun6i-a31-hwspinlock.yaml binding needs #hwlock-cells
> which is required by the hwlock type.
>
> Fixes: f9e784dcb63f ("dt-bindings: hwlock: add sun6i_hwspinlock")
> Signed-off-by: Bastian Germann <bage@debian.org>
> ---
>  .../bindings/hwlock/allwinner,sun6i-a31-hwspinlock.yaml       | 4 ++++
>  1 file changed, 4 insertions(+)

Now failing in linux-next as reported. Please send a fix.

Rob

Re: (subset) [PATCH 0/3] Enable hwlock on Allwinner A64

[Andre Przywara]

On Tue, 14 Feb 2023 12:57:06 -0800
Bjorn Andersson <andersson@kernel.org> wrote:

Hi Bjorn,

> On Tue, 14 Feb 2023 00:19:27 +0100, Bastian Germann wrote:
> > The allwinner,sun6i-a31-hwspinlock compatible driver can be used with
> > the Allwinner A64 chip. Add the wiring required to enable it.
> > 
> > The device tree schema needs some work to verify everything that is
> > needed by the sun6i hwlock driver.
> > 
> > The hwlock device was verified to be available with this series applied
> > on a Pinebook.
> > 
> > [...]  
> 
> Applied, thanks!
> 
> [1/3] dt-bindings: hwlock: sun6i: Add missing #hwlock-cells
>       commit: 25d10262ed8aae71cd2f0b3c0a90d06a6b79fba2

as you might have seen, this was a bit premature, as the example is
missing the newly required property, as pointed out by Rob[1].
Did you send a pull request including this patch already? Can you drop
this patch still from your tree? The fix is rather trivial, but the
patch is also not critical, since there is no DT user in the tree, and
the remaining series won't make it into v6.3 anyway.

Cheers,
Andre

[1]
https://lore.kernel.org/linux-remoteproc/167638944701.3666.5425414440419322569.robh@kernel.org/

Re: [PATCH 2/3] dt-bindings: hwlock: sun6i: Add missing names

[Krzysztof Kozlowski]

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

Re: [PATCH 1/3] dt-bindings: hwlock: sun6i: Add missing #hwlock-cells

[Bastian Germann]

Am 15.02.23 um 15:07 schrieb Rob Herring:
> Now failing in linux-next as reported. Please send a fix.

That is in v2 2/5.