[3/3] riscv: dts: allwinner: d1-t113: Add the hardware spinlock

Message ID 20260927025626.69614-4-tien.nguyenminh@embeddedlinux.blog (mailing list archive)
State New
Headers
Series hwspinlock: sun6i: Allwinner D1 and T113 support |

Commit Message

Nguyen Minh Tien Sept. 27, 2026, 2:56 a.m. UTC
Add the hardware spinlock of the D1 and T113. It goes in
sunxi-d1-t113.dtsi rather than sunxi-d1s-t113.dtsi, as the D1s manual
has no spinlock in its memory map.

Signed-off-by: Nguyen Minh Tien <tien.nguyenminh@embeddedlinux.blog>
---
 arch/riscv/boot/dts/allwinner/sunxi-d1-t113.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)
  

Comments

Wilken Gottwalt Sept. 27, 2026, 11:27 a.m. UTC | #1
On Sun, 27 Sep 2026 09:56:26 +0700
Nguyen Minh Tien <tien.nguyenminh@embeddedlinux.blog> wrote:

> Add the hardware spinlock of the D1 and T113. It goes in
> sunxi-d1-t113.dtsi rather than sunxi-d1s-t113.dtsi, as the D1s manual
> has no spinlock in its memory map.
> 
> Signed-off-by: Nguyen Minh Tien <tien.nguyenminh@embeddedlinux.blog>
> ---
>  arch/riscv/boot/dts/allwinner/sunxi-d1-t113.dtsi | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/arch/riscv/boot/dts/allwinner/sunxi-d1-t113.dtsi
> b/arch/riscv/boot/dts/allwinner/sunxi-d1-t113.dtsi index 3b077dc086..228cc5c074 100644
> --- a/arch/riscv/boot/dts/allwinner/sunxi-d1-t113.dtsi
> +++ b/arch/riscv/boot/dts/allwinner/sunxi-d1-t113.dtsi
> @@ -11,5 +11,14 @@ dsp_wdt: watchdog@1700400 {
>  			clock-names = "hosc", "losc";
>  			status = "reserved";
>  		};
> +
> +		hwlock: hwlock@3005000 {
> +			compatible = "allwinner,sun20i-d1-hwspinlock",
> +				     "allwinner,sun6i-a31-hwspinlock";
> +			reg = <0x3005000 0x1000>;
> +			clocks = <&ccu CLK_BUS_SPINLOCK>;
> +			resets = <&ccu RST_BUS_SPINLOCK>;
> +			#hwlock-cells = <1>;
> +		};
>  	};
>  };

Wouldn't it make more sense to add the "allwinner,sun20i-d1-hwspinlock" line to
the driver in the sun6i_hwspinlock_ids struct, drop
"allwinner,sun6i-a31-hwspinlock" here in the D1 device tree and update the yaml
file accordingly? Hmm, there are actually a lot more devices, which support that
spinlock (H2, H2+, H3, H5, H6...). A31 was the first one introducing that IP
core, but newer reference manuals removed the spinlock section completely.
There it is an unnamed 4k block in the memory map.

Bjorn, what do you think? Just stay with the "allwinner,sun6i-a31-hwspinlock"
string or add all the possible combinations like
"allwinner,sun8i-h2-plus-hwspinlock" or "allwinner,sun8i-a83t-hwspinlock".
I mean, it is just a naming game and there are 10+ SoCs supporting this
spinlock register file.

Oh, and I may be able to test it against the D1, I own a Sipeed Nezha. Though,
setting that one up for kernel + FreeRTOS testing is really, uhm, annoying.

greetings,
Wilken
  
Nguyen Minh Tien Sept. 30, 2026, 1:02 p.m. UTC | #2
Hi Wilken,

> Wouldn't it make more sense to add the "allwinner,sun20i-d1-hwspinlock" line to
> the driver in the sun6i_hwspinlock_ids struct, drop
> "allwinner,sun6i-a31-hwspinlock" here in the D1 device tree and update the yaml
> file accordingly?

Thanks for looking at it. Bjorn hasn't replied yet, so I looked a bit
more at the naming. I'd like to keep the A31 fallback: it's the usual
pattern, other blocks in this dtsi do the same (timer, I2S, LED
controller), and Conor already acked the binding in 2/3. If Bjorn
prefers a driver entry instead, I'm fine to change it.

> Oh, and I may be able to test it against the D1, I own a Sipeed Nezha.

That would be great. You don't need FreeRTOS for it: I tested with a
small Linux module that takes each lock and checks the status
register. I can clean it up for the single-core D1 and send it.

Thanks,
Tien
  
Wilken Gottwalt Sept. 30, 2026, 2:05 p.m. UTC | #3
On Wed, 30 Sep 2026 20:02:21 +0700
Nguyen Minh Tien <tien.nguyenminh@embeddedlinux.blog> wrote:

> Hi Wilken,
> 
> > Wouldn't it make more sense to add the "allwinner,sun20i-d1-hwspinlock" line to
> > the driver in the sun6i_hwspinlock_ids struct, drop
> > "allwinner,sun6i-a31-hwspinlock" here in the D1 device tree and update the yaml
> > file accordingly?
> 
> Thanks for looking at it. Bjorn hasn't replied yet, so I looked a bit
> more at the naming. I'd like to keep the A31 fallback: it's the usual
> pattern, other blocks in this dtsi do the same (timer, I2S, LED
> controller), and Conor already acked the binding in 2/3. If Bjorn
> prefers a driver entry instead, I'm fine to change it.

Yeah, Conor was a bit quick to act here, such things happen often with patchsets
made out of documentation/devicetrees and code. Though, the get clock and resets
patch is fine. The driver could use some modernization.

> > Oh, and I may be able to test it against the D1, I own a Sipeed Nezha.
> 
> That would be great. You don't need FreeRTOS for it: I tested with a
> small Linux module that takes each lock and checks the status
> register. I can clean it up for the single-core D1 and send it.

Uhm, the Linux-only test doesn't work as a hwspinlock test, it misses the entire
point of the primitive. A hwspinlock arbitrates between two independent agents,
in this case, Linux running on the C906 core and FreeRTOS running on the HiFi
DSP, both sharing the same memory bus and other hardware. If Linux is the only
one who ever takes the locks, it is simultaneously writer and reader of the status
register, so the test cannot fail even for a broken (or fake) implementation. That
would basically test nothing at all, well, maybe it would be some kind of bring-up
test, but overall quite useless.

greetings,
Wilken
  
Conor Dooley Oct. 2, 2026, 7:32 a.m. UTC | #4
On Wed, Sep 30, 2026 at 02:05:47PM +0000, Wilken Gottwalt wrote:
> On Wed, 30 Sep 2026 20:02:21 +0700
> Nguyen Minh Tien <tien.nguyenminh@embeddedlinux.blog> wrote:
> 
> > Hi Wilken,
> > 
> > > Wouldn't it make more sense to add the "allwinner,sun20i-d1-hwspinlock" line to
> > > the driver in the sun6i_hwspinlock_ids struct, drop
> > > "allwinner,sun6i-a31-hwspinlock" here in the D1 device tree and update the yaml
> > > file accordingly?
> > 
> > Thanks for looking at it. Bjorn hasn't replied yet, so I looked a bit
> > more at the naming. I'd like to keep the A31 fallback: it's the usual
> > pattern, other blocks in this dtsi do the same (timer, I2S, LED
> > controller), and Conor already acked the binding in 2/3. If Bjorn
> > prefers a driver entry instead, I'm fine to change it.
> 
> Yeah, Conor was a bit quick to act here, such things happen often with patchsets
> made out of documentation/devicetrees and code. Though, the get clock and resets
> patch is fine. The driver could use some modernization.

I dunno, was I too quick to act? The patched looked correct to me, since
it was using a fallback to a device that it appears to be compatible
with. Had the series done what you're suggesting, my review feedback
would have been to tell the Tien to add a fallback.

> > Bjorn, what do you think? Just stay with the "allwinner,sun6i-a31-hwspinlock"
> > string or add all the possible combinations like
> > "allwinner,sun8i-h2-plus-hwspinlock" or "allwinner,sun8i-a83t-hwspinlock".
> > I mean, it is just a naming game and there are 10+ SoCs supporting this
> > spinlock register file

All devices compatible with the a31 should use the a31 as a fallback.

> 
> > > Oh, and I may be able to test it against the D1, I own a Sipeed Nezha.
> > 
> > That would be great. You don't need FreeRTOS for it: I tested with a
> > small Linux module that takes each lock and checks the status
> > register. I can clean it up for the single-core D1 and send it.
> 
> Uhm, the Linux-only test doesn't work as a hwspinlock test, it misses the entire
> point of the primitive. A hwspinlock arbitrates between two independent agents,
> in this case, Linux running on the C906 core and FreeRTOS running on the HiFi
> DSP, both sharing the same memory bus and other hardware. If Linux is the only
> one who ever takes the locks, it is simultaneously writer and reader of the status
> register, so the test cannot fail even for a broken (or fake) implementation. That
> would basically test nothing at all, well, maybe it would be some kind of bring-up
> test, but overall quite useless.
> 
> greetings,
> Wilken
  
Wilken Gottwalt Oct. 2, 2026, 7:48 a.m. UTC | #5
On Fri, 2 Oct 2026 08:32:51 +0100
Conor Dooley <conor.dooley@microchip.com> wrote:

> On Wed, Sep 30, 2026 at 02:05:47PM +0000, Wilken Gottwalt wrote:
> > On Wed, 30 Sep 2026 20:02:21 +0700
> > Nguyen Minh Tien <tien.nguyenminh@embeddedlinux.blog> wrote:
> > 
> > > Hi Wilken,
> > > 
> > > > Wouldn't it make more sense to add the "allwinner,sun20i-d1-hwspinlock" line to
> > > > the driver in the sun6i_hwspinlock_ids struct, drop
> > > > "allwinner,sun6i-a31-hwspinlock" here in the D1 device tree and update the yaml
> > > > file accordingly?
> > > 
> > > Thanks for looking at it. Bjorn hasn't replied yet, so I looked a bit
> > > more at the naming. I'd like to keep the A31 fallback: it's the usual
> > > pattern, other blocks in this dtsi do the same (timer, I2S, LED
> > > controller), and Conor already acked the binding in 2/3. If Bjorn
> > > prefers a driver entry instead, I'm fine to change it.
> > 
> > Yeah, Conor was a bit quick to act here, such things happen often with patchsets
> > made out of documentation/devicetrees and code. Though, the get clock and resets
> > patch is fine. The driver could use some modernization.
> 
> I dunno, was I too quick to act? The patched looked correct to me, since
> it was using a fallback to a device that it appears to be compatible
> with. Had the series done what you're suggesting, my review feedback
> would have been to tell the Tien to add a fallback.

I'm just not sure how to actually do it right, because so many SoCs include that
feature. That is why I asked how you would do it having more insight as a
subsystem maintainer. It just looks incomplete to me. In the past I actually
verified it working with H2, H2+ and H3, none of them being
"allwinner,sun6i-a31-hwspinlock".

> > > Bjorn, what do you think? Just stay with the "allwinner,sun6i-a31-hwspinlock"
> > > string or add all the possible combinations like
> > > "allwinner,sun8i-h2-plus-hwspinlock" or "allwinner,sun8i-a83t-hwspinlock".
> > > I mean, it is just a naming game and there are 10+ SoCs supporting this
> > > spinlock register file
> 
> All devices compatible with the a31 should use the a31 as a fallback.
> 
> > 
> > > > Oh, and I may be able to test it against the D1, I own a Sipeed Nezha.
> > > 
> > > That would be great. You don't need FreeRTOS for it: I tested with a
> > > small Linux module that takes each lock and checks the status
> > > register. I can clean it up for the single-core D1 and send it.
> > 
> > Uhm, the Linux-only test doesn't work as a hwspinlock test, it misses the entire
> > point of the primitive. A hwspinlock arbitrates between two independent agents,
> > in this case, Linux running on the C906 core and FreeRTOS running on the HiFi
> > DSP, both sharing the same memory bus and other hardware. If Linux is the only
> > one who ever takes the locks, it is simultaneously writer and reader of the status
> > register, so the test cannot fail even for a broken (or fake) implementation. That
> > would basically test nothing at all, well, maybe it would be some kind of bring-up
> > test, but overall quite useless.
  
Chen-Yu Tsai Oct. 2, 2026, 7:48 a.m. UTC | #6
On Fri, Oct 2, 2026 at 3:34 PM Conor Dooley <conor.dooley@microchip.com> wrote:
>
> On Wed, Sep 30, 2026 at 02:05:47PM +0000, Wilken Gottwalt wrote:
> > On Wed, 30 Sep 2026 20:02:21 +0700
> > Nguyen Minh Tien <tien.nguyenminh@embeddedlinux.blog> wrote:
> >
> > > Hi Wilken,
> > >
> > > > Wouldn't it make more sense to add the "allwinner,sun20i-d1-hwspinlock" line to
> > > > the driver in the sun6i_hwspinlock_ids struct, drop
> > > > "allwinner,sun6i-a31-hwspinlock" here in the D1 device tree and update the yaml
> > > > file accordingly?
> > >
> > > Thanks for looking at it. Bjorn hasn't replied yet, so I looked a bit
> > > more at the naming. I'd like to keep the A31 fallback: it's the usual
> > > pattern, other blocks in this dtsi do the same (timer, I2S, LED
> > > controller), and Conor already acked the binding in 2/3. If Bjorn
> > > prefers a driver entry instead, I'm fine to change it.
> >
> > Yeah, Conor was a bit quick to act here, such things happen often with patchsets
> > made out of documentation/devicetrees and code. Though, the get clock and resets
> > patch is fine. The driver could use some modernization.
>
> I dunno, was I too quick to act? The patched looked correct to me, since
> it was using a fallback to a device that it appears to be compatible
> with. Had the series done what you're suggesting, my review feedback
> would have been to tell the Tien to add a fallback.
>
> > > Bjorn, what do you think? Just stay with the "allwinner,sun6i-a31-hwspinlock"
> > > string or add all the possible combinations like
> > > "allwinner,sun8i-h2-plus-hwspinlock" or "allwinner,sun8i-a83t-hwspinlock".
> > > I mean, it is just a naming game and there are 10+ SoCs supporting this
> > > spinlock register file
>
> All devices compatible with the a31 should use the a31 as a fallback.

+1.

If the hardware is backward compatible, then with the fallback compatible
everything works.

We ask folks to always add SoC-specific compatibles just in case.


ChenYu

> >
> > > > Oh, and I may be able to test it against the D1, I own a Sipeed Nezha.
> > >
> > > That would be great. You don't need FreeRTOS for it: I tested with a
> > > small Linux module that takes each lock and checks the status
> > > register. I can clean it up for the single-core D1 and send it.
> >
> > Uhm, the Linux-only test doesn't work as a hwspinlock test, it misses the entire
> > point of the primitive. A hwspinlock arbitrates between two independent agents,
> > in this case, Linux running on the C906 core and FreeRTOS running on the HiFi
> > DSP, both sharing the same memory bus and other hardware. If Linux is the only
> > one who ever takes the locks, it is simultaneously writer and reader of the status
> > register, so the test cannot fail even for a broken (or fake) implementation. That
> > would basically test nothing at all, well, maybe it would be some kind of bring-up
> > test, but overall quite useless.
> >
> > greetings,
> > Wilken
  
Chen-Yu Tsai Oct. 2, 2026, 7:50 a.m. UTC | #7
On Fri, Oct 2, 2026 at 3:48 PM Wilken Gottwalt
<wilken.gottwalt@posteo.net> wrote:
>
> On Fri, 2 Oct 2026 08:32:51 +0100
> Conor Dooley <conor.dooley@microchip.com> wrote:
>
> > On Wed, Sep 30, 2026 at 02:05:47PM +0000, Wilken Gottwalt wrote:
> > > On Wed, 30 Sep 2026 20:02:21 +0700
> > > Nguyen Minh Tien <tien.nguyenminh@embeddedlinux.blog> wrote:
> > >
> > > > Hi Wilken,
> > > >
> > > > > Wouldn't it make more sense to add the "allwinner,sun20i-d1-hwspinlock" line to
> > > > > the driver in the sun6i_hwspinlock_ids struct, drop
> > > > > "allwinner,sun6i-a31-hwspinlock" here in the D1 device tree and update the yaml
> > > > > file accordingly?
> > > >
> > > > Thanks for looking at it. Bjorn hasn't replied yet, so I looked a bit
> > > > more at the naming. I'd like to keep the A31 fallback: it's the usual
> > > > pattern, other blocks in this dtsi do the same (timer, I2S, LED
> > > > controller), and Conor already acked the binding in 2/3. If Bjorn
> > > > prefers a driver entry instead, I'm fine to change it.
> > >
> > > Yeah, Conor was a bit quick to act here, such things happen often with patchsets
> > > made out of documentation/devicetrees and code. Though, the get clock and resets
> > > patch is fine. The driver could use some modernization.
> >
> > I dunno, was I too quick to act? The patched looked correct to me, since
> > it was using a fallback to a device that it appears to be compatible
> > with. Had the series done what you're suggesting, my review feedback
> > would have been to tell the Tien to add a fallback.
>
> I'm just not sure how to actually do it right, because so many SoCs include that
> feature. That is why I asked how you would do it having more insight as a
> subsystem maintainer. It just looks incomplete to me. In the past I actually
> verified it working with H2, H2+ and H3, none of them being
> "allwinner,sun6i-a31-hwspinlock".

Even if all the SoCs had them, if no one enabled and tested them, then they
wouldn't have entries in the binding or dtsi files.

> > > > Bjorn, what do you think? Just stay with the "allwinner,sun6i-a31-hwspinlock"
> > > > string or add all the possible combinations like
> > > > "allwinner,sun8i-h2-plus-hwspinlock" or "allwinner,sun8i-a83t-hwspinlock".
> > > > I mean, it is just a naming game and there are 10+ SoCs supporting this
> > > > spinlock register file
> >
> > All devices compatible with the a31 should use the a31 as a fallback.
> >
> > >
> > > > > Oh, and I may be able to test it against the D1, I own a Sipeed Nezha.
> > > >
> > > > That would be great. You don't need FreeRTOS for it: I tested with a
> > > > small Linux module that takes each lock and checks the status
> > > > register. I can clean it up for the single-core D1 and send it.
> > >
> > > Uhm, the Linux-only test doesn't work as a hwspinlock test, it misses the entire
> > > point of the primitive. A hwspinlock arbitrates between two independent agents,
> > > in this case, Linux running on the C906 core and FreeRTOS running on the HiFi
> > > DSP, both sharing the same memory bus and other hardware. If Linux is the only
> > > one who ever takes the locks, it is simultaneously writer and reader of the status
> > > register, so the test cannot fail even for a broken (or fake) implementation. That
> > > would basically test nothing at all, well, maybe it would be some kind of bring-up
> > > test, but overall quite useless.
  
Conor Dooley Oct. 2, 2026, 8:37 a.m. UTC | #8
On Fri, Oct 02, 2026 at 07:48:21AM +0000, Wilken Gottwalt wrote:
> On Fri, 2 Oct 2026 08:32:51 +0100
> Conor Dooley <conor.dooley@microchip.com> wrote:
> 
> > On Wed, Sep 30, 2026 at 02:05:47PM +0000, Wilken Gottwalt wrote:
> > > On Wed, 30 Sep 2026 20:02:21 +0700
> > > Nguyen Minh Tien <tien.nguyenminh@embeddedlinux.blog> wrote:
> > > 
> > > > Hi Wilken,
> > > > 
> > > > > Wouldn't it make more sense to add the "allwinner,sun20i-d1-hwspinlock" line to
> > > > > the driver in the sun6i_hwspinlock_ids struct, drop
> > > > > "allwinner,sun6i-a31-hwspinlock" here in the D1 device tree and update the yaml
> > > > > file accordingly?
> > > > 
> > > > Thanks for looking at it. Bjorn hasn't replied yet, so I looked a bit
> > > > more at the naming. I'd like to keep the A31 fallback: it's the usual
> > > > pattern, other blocks in this dtsi do the same (timer, I2S, LED
> > > > controller), and Conor already acked the binding in 2/3. If Bjorn
> > > > prefers a driver entry instead, I'm fine to change it.
> > > 
> > > Yeah, Conor was a bit quick to act here, such things happen often with patchsets
> > > made out of documentation/devicetrees and code. Though, the get clock and resets
> > > patch is fine. The driver could use some modernization.
> > 
> > I dunno, was I too quick to act? The patched looked correct to me, since
> > it was using a fallback to a device that it appears to be compatible
> > with. Had the series done what you're suggesting, my review feedback
> > would have been to tell the Tien to add a fallback.
> 
> I'm just not sure how to actually do it right, because so many SoCs include that
> feature.

If devices share a programming model with existing devices or have a
programming model that's a functional superset of existing devices (so a
new optional feature etc) they should use the existing device as a
fallback compatible.

> That is why I asked how you would do it having more insight as a
> subsystem maintainer.


> It just looks incomplete to me. In the past I actually
> verified it working with H2, H2+ and H3, none of them being
> "allwinner,sun6i-a31-hwspinlock".

Then probably there should be a patch adding all of these to the enum
alongside the new d1 compatible.

Cheers,
Conor.
  

Patch

diff --git a/arch/riscv/boot/dts/allwinner/sunxi-d1-t113.dtsi b/arch/riscv/boot/dts/allwinner/sunxi-d1-t113.dtsi
index 3b077dc086..228cc5c074 100644
--- a/arch/riscv/boot/dts/allwinner/sunxi-d1-t113.dtsi
+++ b/arch/riscv/boot/dts/allwinner/sunxi-d1-t113.dtsi
@@ -11,5 +11,14 @@  dsp_wdt: watchdog@1700400 {
 			clock-names = "hosc", "losc";
 			status = "reserved";
 		};
+
+		hwlock: hwlock@3005000 {
+			compatible = "allwinner,sun20i-d1-hwspinlock",
+				     "allwinner,sun6i-a31-hwspinlock";
+			reg = <0x3005000 0x1000>;
+			clocks = <&ccu CLK_BUS_SPINLOCK>;
+			resets = <&ccu RST_BUS_SPINLOCK>;
+			#hwlock-cells = <1>;
+		};
 	};
 };