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 binding 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.
1/5 was already applied in linux-next.
Changelog:
v2:
Fix the dt-binding example for the added properties.
Add a compatible string for a64.
Wording 3/5: driver -> Linux driver.
Bastian Germann (5):
dt-bindings: hwlock: sun6i: Add missing #hwlock-cells
dt-bindings: hwlock: sun6i: Add #hwlock-cells to example
dt-bindings: hwlock: sun6i: Add missing names
dt-bindings: hwlock: sun6i: Add a64 compatible string
arm64: dts: allwinner: a64: Add hwspinlock node
.../allwinner,sun6i-a31-hwspinlock.yaml | 21 ++++++++++++++++++-
arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 11 ++++++++++
2 files changed, 31 insertions(+), 1 deletion(-)
--
2.39.1
The allwinner,sun6i-a31-hwspinlock.yaml binding needs clock-names
and reset-names set to "ahb" as required by the Linux driver.
Fixes: f9e784dcb63f ("dt-bindings: hwlock: add sun6i_hwspinlock")
Signed-off-by: Bastian Germann <[email protected]>
---
.../hwlock/allwinner,sun6i-a31-hwspinlock.yaml | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/Documentation/devicetree/bindings/hwlock/allwinner,sun6i-a31-hwspinlock.yaml b/Documentation/devicetree/bindings/hwlock/allwinner,sun6i-a31-hwspinlock.yaml
index 38478dad8b25..6cdfe22deb3c 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
@@ -47,7 +57,9 @@ examples:
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>;
};
...
--
2.39.1
Add the hwspinlock to A64 which is already implemented for A31.
Signed-off-by: Bastian Germann <[email protected]>
---
arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index 77b5349f6087..f2ecc21f06ed 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -851,6 +851,17 @@ wdt0: watchdog@1c20ca0 {
clocks = <&osc24M>;
};
+ hwspinlock: hwlock@1c18000 {
+ compatible = "allwinner,sun50i-a64-hwspinlock",
+ "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
On Wed, 15 Feb 2023 21:37:08 +0100
Bastian Germann <[email protected]> wrote:
> The allwinner,sun6i-a31-hwspinlock.yaml binding needs clock-names
> and reset-names set to "ahb" as required by the Linux driver.
Mmmh, but I thought that Krzysztof pretty clearly NAKed this?
So we have to either reach consensus on this or find another solution,
like teaching the driver to comply with the existing binding.
We could for instance get the first clock, should the devm_clk_get()
call fail.
Cheers,
Andre
>
> Fixes: f9e784dcb63f ("dt-bindings: hwlock: add sun6i_hwspinlock")
> Signed-off-by: Bastian Germann <[email protected]>
> ---
> .../hwlock/allwinner,sun6i-a31-hwspinlock.yaml | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/hwlock/allwinner,sun6i-a31-hwspinlock.yaml b/Documentation/devicetree/bindings/hwlock/allwinner,sun6i-a31-hwspinlock.yaml
> index 38478dad8b25..6cdfe22deb3c 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
> @@ -47,7 +57,9 @@ examples:
> 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>;
> };
> ...
On Wed, 15 Feb 2023 21:37:10 +0100
Bastian Germann <[email protected]> wrote:
Hi,
> Add the hwspinlock to A64 which is already implemented for A31.
>
> Signed-off-by: Bastian Germann <[email protected]>
This looks alright to me, but there is still the debate what to do with
the names properties. So holding any tags back until this is resolved.
Cheers,
Andre
> ---
> arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> index 77b5349f6087..f2ecc21f06ed 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> @@ -851,6 +851,17 @@ wdt0: watchdog@1c20ca0 {
> clocks = <&osc24M>;
> };
>
> + hwspinlock: hwlock@1c18000 {
> + compatible = "allwinner,sun50i-a64-hwspinlock",
> + "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",
Am 15.02.23 um 21:45 schrieb Andre Przywara:
> On Wed, 15 Feb 2023 21:37:08 +0100
> Bastian Germann <[email protected]> wrote:
>
>> The allwinner,sun6i-a31-hwspinlock.yaml binding needs clock-names
>> and reset-names set to "ahb" as required by the Linux driver.
>
> Mmmh, but I thought that Krzysztof pretty clearly NAKed this?
> So we have to either reach consensus on this or find another solution,
> like teaching the driver to comply with the existing binding.
> We could for instance get the first clock, should the devm_clk_get()
> call fail.
Either way - I wanted to send a fix for the dt-binding example as Rob requested.
This is not to say that I want to ignore the NAK.
> Cheers,
> Andre
>
>
>>
>> Fixes: f9e784dcb63f ("dt-bindings: hwlock: add sun6i_hwspinlock")
>> Signed-off-by: Bastian Germann <[email protected]>
>> ---
>> .../hwlock/allwinner,sun6i-a31-hwspinlock.yaml | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/hwlock/allwinner,sun6i-a31-hwspinlock.yaml b/Documentation/devicetree/bindings/hwlock/allwinner,sun6i-a31-hwspinlock.yaml
>> index 38478dad8b25..6cdfe22deb3c 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
>> @@ -47,7 +57,9 @@ examples:
>> 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>;
>> };
>> ...
>
On 15/02/2023 21:37, Bastian Germann wrote:
> The allwinner,sun6i-a31-hwspinlock.yaml binding needs clock-names and
> reset-names set to "ahb" as required by the Linux driver.
>
> Fixes: f9e784dcb63f ("dt-bindings: hwlock: add sun6i_hwspinlock")
> Signed-off-by: Bastian Germann <[email protected]>
With new data, I changed my opinion and NAKed this. Still NAK, sorry.
Please drop the clock/reset-names from the driver (use indices) and DTS.
NAK means Not-acknowledge. Usually you should not send the same patch
after getting NAK, because it looks like you ignore the comment.
Best regards,
Krzysztof
On 15/02/2023 21:37, Bastian Germann wrote:
> Add the hwspinlock to A64 which is already implemented for A31.
>
> Signed-off-by: Bastian Germann <[email protected]>
> ---
> arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> index 77b5349f6087..f2ecc21f06ed 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> @@ -851,6 +851,17 @@ wdt0: watchdog@1c20ca0 {
> clocks = <&osc24M>;
> };
>
> + hwspinlock: hwlock@1c18000 {
> + compatible = "allwinner,sun50i-a64-hwspinlock",
> + "allwinner,sun6i-a31-hwspinlock";
> + reg = <0x01c18000 0x1000>;
> + clocks = <&ccu CLK_BUS_SPINLOCK>;
> + clock-names = "ahb";
Please drop.
> + resets = <&ccu RST_BUS_SPINLOCK>;
> + reset-names = "ahb";
Please drop.
Fix the driver instead.
Best regards,
Krzysztof
On Thu, 16 Feb 2023 09:36:08 +0100
Krzysztof Kozlowski <[email protected]> wrote:
> On 15/02/2023 21:37, Bastian Germann wrote:
> > The allwinner,sun6i-a31-hwspinlock.yaml binding needs clock-names and
> > reset-names set to "ahb" as required by the Linux driver.
> >
> > Fixes: f9e784dcb63f ("dt-bindings: hwlock: add sun6i_hwspinlock")
> > Signed-off-by: Bastian Germann <[email protected]>
>
> With new data, I changed my opinion and NAKed this. Still NAK, sorry.
> Please drop the clock/reset-names from the driver (use indices) and DTS.
I won't be able to fix this in the next time. I'm currently in the state of
moving and can't set up my hardware to test the changes. And I'm not willing
to submit changes without testing. And with testing I really mean testing it
against a running Crust firmware which touches the hwspinlock unit.
If someone wants to change that, I'm happy to see it working out, but please
do it properly. Just testing the locks in Linux only is not sufficient. If
some directions are required, I still have my working repo up at Github:
https://github.com/wgottwalt/sunxi_hwspinlock
It may be a bit dated, but should be a good start.
Greetings,
Will