| Message ID | 20230505074701.1030980-1-bigunclemax@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Conor Dooley |
| Headers | show |
| Series | riscv: dts: allwinner: d1: Add SPI0 controller node | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/cover_letter | success | Single patches do not need cover letters |
| conchuod/tree_selection | success | Guessed tree name to be for-next at HEAD c2d3c8441e3d |
| conchuod/fixes_present | success | Fixes tag not required for -next series |
| conchuod/maintainers_pattern | success | MAINTAINERS pattern errors before the patch: 5 and now 5 |
| conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
| conchuod/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/build_rv64_clang_allmodconfig | success | Errors and warnings before: 8 this patch: 8 |
| conchuod/module_param | success | Was 0 now: 0 |
| conchuod/build_rv64_gcc_allmodconfig | success | Errors and warnings before: 8 this patch: 8 |
| conchuod/build_rv32_defconfig | success | Build OK |
| conchuod/dtb_warn_rv64 | success | Errors and warnings before: 5 this patch: 5 |
| conchuod/header_inline | success | No static functions without inline keyword in header files |
| conchuod/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 32 lines checked |
| conchuod/source_inline | success | Was 0 now: 0 |
| conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
| conchuod/verify_fixes | success | No Fixes tag |
| conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
On Fri, 5 May 2023 10:46:51 +0300 Maksim Kiselev <bigunclemax@gmail.com> wrote: Hi Maksim, thanks for sending a patch! > Some boards form the MangoPi family (MQ\MQ-Dual\MQ-R) may have > an optional SPI flash that connects to the SPI0 controller. > This controller is already supported by sun8i-h3-spi driver. > So let's add its DT node. Interesting, I see SPI mentioned in the D1 platform support cover letter, but indeed there is no DT node. From a quick glance at the manuals, it looks like there are not quite the same, though: the D1/R528/T113s mentions a SPI_SAMP_DL register @0x28, whereas the older IP has a SPI_CCR register @0x24 - which is not mentioned in the newer manuals. The driver relies on that clock control register, so it wouldn't really work reliably, if that register is not there. It *might* work by pure chance because of a particular setup or clock rate, though. Samuel, did you investigate SPI support on the D1/T113s? I see it marked as "WIP" in the Wiki status page, so where there any patches floating around? Regardless of that, one comment that would apply anyway: > Signed-off-by: Maksim Kiselev <bigunclemax@gmail.com> > --- > .../boot/dts/allwinner/sunxi-d1s-t113.dtsi | 20 +++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi b/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi > index 922e8e0e2c09..d2de211d67d7 100644 > --- a/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi > +++ b/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi > @@ -108,6 +108,12 @@ rmii_pe_pins: rmii-pe-pins { > function = "emac"; > }; > > + /omit-if-no-ref/ > + spi0_pins: spi0-pins { > + pins = "PC2", "PC3", "PC4", "PC5"; > + function = "spi0"; > + }; > + > /omit-if-no-ref/ > uart1_pg6_pins: uart1-pg6-pins { > pins = "PG6", "PG7"; > @@ -447,6 +453,20 @@ mmc2: mmc@4022000 { > #size-cells = <0>; > }; > > + spi0: spi@4025000 { > + compatible = "allwinner,sun8i-h3-spi"; Even if it would be compatible, we need to use a more specific compatible first, with the H3 one as a fallback: compatible = "allwinner,sun20i-d1-spi", "allwinner,sun8i-h3-spi"; But that would require that the H3 is a strict subset of the D1 SPI IP. Cheers, Andre > + reg = <0x04025000 0x300>; > + interrupts = <SOC_PERIPHERAL_IRQ(15) IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&ccu CLK_BUS_SPI0>, <&ccu CLK_SPI0>; > + clock-names = "ahb", "mod"; > + dmas = <&dma 22>, <&dma 22>; > + dma-names = "rx", "tx"; > + resets = <&ccu RST_BUS_SPI0>; > + status = "disabled"; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + > usb_otg: usb@4100000 { > compatible = "allwinner,sun20i-d1-musb", > "allwinner,sun8i-a33-musb";
Hi Andre, > From a quick glance at the manuals, it > looks like there are not quite the same, though: the D1/R528/T113s > mentions a SPI_SAMP_DL register @0x28, whereas the older IP has a SPI_CCR > register @0x24 - which is not mentioned in the newer manuals. The driver > relies on that clock control register, so it wouldn't really work > reliably, if that register is not there. Thank you for pointing this out. I missed this difference. I actually have a board with T113 SoC, and it looks like writing to SPI_CCR@0x24 does nothing. And it doesn't affect access to connected SPI NOR flash (read\write operations are fine). But I completely agree with you that this difference should be handled by the spi driver.
On Fri, 5 May 2023 17:27:07 +0400 Maxim Kiselev <bigunclemax@gmail.com> wrote: Hi Maxim, > > From a quick glance at the manuals, it > > looks like there are not quite the same, though: the D1/R528/T113s > > mentions a SPI_SAMP_DL register @0x28, whereas the older IP has a SPI_CCR > > register @0x24 - which is not mentioned in the newer manuals. The driver > > relies on that clock control register, so it wouldn't really work > > reliably, if that register is not there. > > Thank you for pointing this out. I missed this difference. > I actually have a board with T113 SoC, and it looks like writing to > SPI_CCR@0x24 does nothing. According to the manual, the register doesn't exist, so this would support this theory. Most Allwinner IP just implements non-existing registers as RAZ/WI. > And it doesn't affect access to connected SPI NOR flash (read\write > operations are fine). I guess it just works with *some* (default?) clock settings then. Might be faster than configured, but the SPI flash chip might be fine with that. > But I completely agree with you that this difference should be handled > by the spi driver. As Samuel pointed out on IRC, there are patches, for the R329, using the same IP: https://lore.kernel.org/lkml/BYAPR20MB2472E8B10BFEF75E7950BBC0BCF79@BYAPR20MB2472.namprd20.prod.outlook.com/ And he also seemed to have issues, at least with SPI-NAND on one board: https://lore.kernel.org/lkml/0b5b586a-3bc7-384e-103c-e40d0b2fac23@sholland.org/ Can you please try these patches, and use: compatible = "allwinner,sun20i-d1-spi", "allwinner,sun50i-r329-spi"; for the compatible string? If possible, you could revive this series, and add the D1/T113s support on top: just add the compatible string combo to the binding document. Cheers, Andre
> Can you please try these patches, and use: > compatible = "allwinner,sun20i-d1-spi", "allwinner,sun50i-r329-spi"; > for the compatible string? I tested my patch (with compatible prop changed to allwinner,sun50i-r329-spi) on top of these patches and it works fine for me. > If possible, you could revive this series, and add the D1/T113s support on > top: just add the compatible string combo to the binding document. Just one question. Should I do it now, or is it better to wait while Icenowy Zheng finishes work on the next revision of this series?
On Sat, 6 May 2023 01:25:02 +0400 Maxim Kiselev <bigunclemax@gmail.com> wrote: Hi Maxim, > > Can you please try these patches, and use: > > compatible = "allwinner,sun20i-d1-spi", "allwinner,sun50i-r329-spi"; > > for the compatible string? > > I tested my patch (with compatible prop changed to > allwinner,sun50i-r329-spi) on top of these patches and it works fine > for me. > > > If possible, you could revive this series, and add the D1/T113s support on > > top: just add the compatible string combo to the binding document. > > Just one question. Should I do it now, or is it better to wait while > Icenowy Zheng finishes work on the next revision of this series? This series is over a year old, I doubt she will jump on this again any time soon. So it would be great if you could rebase the patches, keeping her authorship and Signed-off-by:, adding your Signed-off-by: and posting them. I might take a deep breath and try to solder some NOR SPI on my MQ-R to test it later on.... Many thanks! Andre
diff --git a/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi b/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi index 922e8e0e2c09..d2de211d67d7 100644 --- a/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi +++ b/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi @@ -108,6 +108,12 @@ rmii_pe_pins: rmii-pe-pins { function = "emac"; }; + /omit-if-no-ref/ + spi0_pins: spi0-pins { + pins = "PC2", "PC3", "PC4", "PC5"; + function = "spi0"; + }; + /omit-if-no-ref/ uart1_pg6_pins: uart1-pg6-pins { pins = "PG6", "PG7"; @@ -447,6 +453,20 @@ mmc2: mmc@4022000 { #size-cells = <0>; }; + spi0: spi@4025000 { + compatible = "allwinner,sun8i-h3-spi"; + reg = <0x04025000 0x300>; + interrupts = <SOC_PERIPHERAL_IRQ(15) IRQ_TYPE_LEVEL_HIGH>; + clocks = <&ccu CLK_BUS_SPI0>, <&ccu CLK_SPI0>; + clock-names = "ahb", "mod"; + dmas = <&dma 22>, <&dma 22>; + dma-names = "rx", "tx"; + resets = <&ccu RST_BUS_SPI0>; + status = "disabled"; + #address-cells = <1>; + #size-cells = <0>; + }; + usb_otg: usb@4100000 { compatible = "allwinner,sun20i-d1-musb", "allwinner,sun8i-a33-musb";
Some boards form the MangoPi family (MQ\MQ-Dual\MQ-R) may have an optional SPI flash that connects to the SPI0 controller. This controller is already supported by sun8i-h3-spi driver. So let's add its DT node. Signed-off-by: Maksim Kiselev <bigunclemax@gmail.com> --- .../boot/dts/allwinner/sunxi-d1s-t113.dtsi | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+)