[RFC 0/2] A33 SPI support

[Karsten Merker]

Hello everybody,

during the mainlining discussion in the Olimex blog at
https://olimex.wordpress.com/2016/08/30/free-electrons-add-mainline-linux-kernel-support-for-the-a13-allwinner-vpu/#comments
the topic of A33 SPI support came up. Chen-Yu and Siarhei have
pointed out that the chances are good that the A33 can work with
the already existing SPI drivers and that simply nobody has taken
a look at those in the context of the A33 yet - so I have taken
that as an inspiration to take a look :-).

From the description in the docs published by Allwinner, the
A23/A33 SPI controller is clearly derived from the one in the
A31, but it is not completely identical. The biggest difference
is that the RX and TX FIFOs on the A23/A33 are only 64 Bytes deep
while they are 128 Bytes deep on the A31. Documentation source:

Page 463 of the A33 manual:
https://github.com/allwinner-zh/documents/raw/master/A33/A33%20user%20manual%20release%201.1.pdf

Page 889 of the A31 manual:
https://github.com/allwinner-zh/documents/raw/master/A31/A31_User_Manual_v1.3_20150510.pdf

The current spi-sun6i driver hardcodes the FIFO depth to 128
Bytes, so that needs to be changed depending on the SoC type.

Another difference is an additional configuration option
influencing the signal sampling on the A23/A33. This is handled
by bit 13 of the SPI_INTCTL register. The description states:

"Master Sample Data Mode
1-Normal Sample Mode
0-Delay Sample Mode
In Normal Sample Mode,SPI Master samples the data at
the correct edge for each SPI mode.
In Delay Sample Mode,SPI master samples data at the
edge that is half cycle delayed by the correct edge defined
in respective SPI mode."

The manual states that "Delay Sample Mode" is the default, which
sounds a bit strange to me. Can somebody with a deeper knowledge
of SPI comment on that?

There are some additional test modes on the A23/33 compared to the A31,
but those should be irrelevant for normal operations.

Following are two RFC patches based on Maxime Ripard's sunxi/for-next
branch at

https://git.kernel.org/cgit/linux/kernel/git/mripard/linux.git/log/?h=sunxi/for-next

that modify the spi-sun6i driver to handle different FIFO depths and add the
relevant nodes (clock, pinctrl, spi0 controller) to the A23/A33 dtsi files.
Please note that this is largely untested as I currently don't have any
A23/A33 hardware (an A33 board is on the way to me but will probably take
some time to arrive). All I can say at the moment is that the code
compiles. I would apprechiate very much if people could take a look at it
and provide feedback, and if somebody has appropriate hardware, I would
welcome very much if you could subject the code to some practical tests.

Btw, while checking the docs I have stumbled over one thing that strikes me
as rather strange: the A23/A33 has two SPI controllers (SPI0 and SPI1), but
there is no documented pinmux that makes SPI1 available on any pins, so SPI1
appears to be completely useless?

Regards,
Karsten


Karsten Merker (2):
spi: sunxi: Add Allwinner A23/A33 support to spi-sun6i
ARM: dts: sun8i: Add SPI support to the Allwinner A23/A33 dtsi.

.../devicetree/bindings/spi/spi-sun6i.txt | 6 ++-
arch/arm/boot/dts/sun8i-a23-a33.dtsi | 38 +++++++++++++++++++
arch/arm/boot/dts/sun8i-a23.dtsi | 12 ++++++
arch/arm/boot/dts/sun8i-a33.dtsi | 12 ++++++
drivers/spi/spi-sun6i.c | 43 +++++++++++++++++++---
5 files changed, 103 insertions(+), 8 deletions(-)

--
2.1.4

[Karsten Merker]

Define the necessary nodes in the A23/A33 dtsi files to make the
SPI0 controller accessible.

The SoCs also have a secondary SPI controller (SPI1), but it
cannot be muxed to any available pins and therefore doesn't get
included in the dtsi.

Signed-off-by: Karsten Merker <mer...@debian.org>
---
arch/arm/boot/dts/sun8i-a23-a33.dtsi | 38 ++++++++++++++++++++++++++++++++++++
arch/arm/boot/dts/sun8i-a23.dtsi | 12 ++++++++++++
arch/arm/boot/dts/sun8i-a33.dtsi | 12 ++++++++++++
3 files changed, 62 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-a23-a33.dtsi b/arch/arm/boot/dts/sun8i-a23-a33.dtsi
index 6d6509c..a076b5d 100644
--- a/arch/arm/boot/dts/sun8i-a23-a33.dtsi
+++ b/arch/arm/boot/dts/sun8i-a23-a33.dtsi
@@ -3,6 +3,8 @@
*
* Chen-Yu Tsai <we...@csie.org>
*
+ * Copyright 2016 Karsten Merker <mer...@debian.org>
+ *
* This file is dual-licensed: you can use it either under the terms
* of the GPL or the X11 license, at your option. Note that this dual
* licensing only applies to this file, and not this project as a
@@ -247,6 +249,14 @@
clock-output-names = "nand";
};

+ spi0_clk: clk@1c200a0 {
+ #clock-cells = <0>;
+ compatible = "allwinner,sun4i-a10-mod0-clk";
+ reg = <0x01c200a0 0x4>;
+ clocks = <&osc24M>, <&pll6 0>;
+ clock-output-names = "spi0";
+ };
+
usb_clk: clk@01c200cc {
#clock-cells = <1>;
#reset-cells = <1>;
@@ -435,6 +445,34 @@
allwinner,drive = <SUN4I_PINCTRL_10_MA>;
allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
};
+
+ spi0_pins_a: spi0@0 {
+ allwinner,pins = "PH7", "PH8", "PH9";
+ allwinner,function = "spi0";
+ allwinner,drive = <SUN4I_PINCTRL_10_MA>;
+ allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
+ };
+
+ spi0_cs0_pins_a: spi0_cs0@0 {
+ allwinner,pins = "PH6";
+ allwinner,function = "spi0";
+ allwinner,drive = <SUN4I_PINCTRL_10_MA>;
+ allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
+ };
+
+ spi0_pins_b: spi0@1 {
+ allwinner,pins = "PC0", "PC1", "PC2";
+ allwinner,function = "spi0";
+ allwinner,drive = <SUN4I_PINCTRL_10_MA>;
+ allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
+ };
+
+ spi0_cs0_pins_b: spi0_cs0@1 {
+ allwinner,pins = "PC3";
+ allwinner,function = "spi0";
+ allwinner,drive = <SUN4I_PINCTRL_10_MA>;
+ allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
+ };
};

ahb1_rst: reset@01c202c0 {
diff --git a/arch/arm/boot/dts/sun8i-a23.dtsi b/arch/arm/boot/dts/sun8i-a23.dtsi
index 92e6616..f55dc50 100644
--- a/arch/arm/boot/dts/sun8i-a23.dtsi
+++ b/arch/arm/boot/dts/sun8i-a23.dtsi
@@ -3,6 +3,8 @@
*
* Chen-Yu Tsai <we...@csie.org>
*
+ * Copyright 2016 Karsten Merker <mer...@debian.org>
+ *
* This file is dual-licensed: you can use it either under the terms
* of the GPL or the X11 license, at your option. Note that this dual
* licensing only applies to this file, and not this project as a
@@ -85,6 +87,16 @@
};

soc@01c00000 {
+ spi0: spi@01c68000 {
+ compatible = "allwinner,sun8i-a23-spi";
+ reg = <0x01c68000 0x1000>;
+ interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&ahb1_gates 20>, <&spi0_clk>;
+ clock-names = "ahb", "mod";
+ resets = <&ahb1_rst 20>;
+ status = "disabled";
+ };
+
usb_otg: usb@01c19000 {
compatible = "allwinner,sun6i-a31-musb";
reg = <0x01c19000 0x0400>;
diff --git a/arch/arm/boot/dts/sun8i-a33.dtsi b/arch/arm/boot/dts/sun8i-a33.dtsi
index 001d840..e94aea1 100644
--- a/arch/arm/boot/dts/sun8i-a33.dtsi
+++ b/arch/arm/boot/dts/sun8i-a33.dtsi
@@ -3,6 +3,8 @@
*
* Chen-Yu Tsai <we...@csie.org>
*
+ * Copyright 2016 Karsten Merker <mer...@debian.org>
+ *
* This file is dual-licensed: you can use it either under the terms
* of the GPL or the X11 license, at your option. Note that this dual
* licensing only applies to this file, and not this project as a
@@ -127,6 +129,16 @@
reset-names = "ahb";
};

+ spi0: spi@01c68000 {
+ compatible = "allwinner,sun8i-a23-spi";
+ reg = <0x01c68000 0x1000>;
+ interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&ahb1_gates 20>, <&spi0_clk>;
+ clock-names = "ahb", "mod";
+ resets = <&ahb1_rst 20>;
+ status = "disabled";
+ };
+
usb_otg: usb@01c19000 {
compatible = "allwinner,sun8i-a33-musb";
reg = <0x01c19000 0x0400>;
--
2.1.4

[Karsten Merker]

The Allwinner A23/A33 SoCs contain an SPI controller that is
largely identical to the one in the A31. The major differences
from A31 to A23/A33 are:

- The TX and RX FIFOs on the A23/A33 are only 64 Bytes deep
compared to 128 Bytes on the A31.

- On A23/A33, the SPI_INTCTL register provides an additional
"Master Sample Data Mode" configuration option.

Currently the FIFO depth is hardcoded to 128 Bytes in the
driver; this patch moves it into SoC-specific platform data
instead of defining it globally and adds a new compatible
"allwinner,sun8i-a23-spi".

Signed-off-by: Karsten Merker <mer...@debian.org>
---

.../devicetree/bindings/spi/spi-sun6i.txt | 6 ++-
drivers/spi/spi-sun6i.c | 43 +++++++++++++++++++---
2 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/spi-sun6i.txt b/Documentation/devicetree/bindings/spi/spi-sun6i.txt
index 21de73d..4d3eb86 100644
--- a/Documentation/devicetree/bindings/spi/spi-sun6i.txt
+++ b/Documentation/devicetree/bindings/spi/spi-sun6i.txt
@@ -1,7 +1,9 @@
-Allwinner A31 SPI controller
+Allwinner A31/A23 SPI controller

Required properties:
-- compatible: Should be "allwinner,sun6i-a31-spi".
+- compatible: Should be one of
+ - "allwinner,sun6i-a31-spi"
+ - "allwinner,sun8i-a23-spi"
- reg: Should contain register location and length.
- interrupts: Should contain interrupt.
- clocks: phandle to the clocks feeding the SPI controller. Two are
diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
index 9918a57..c6d88c9 100644
--- a/drivers/spi/spi-sun6i.c
+++ b/drivers/spi/spi-sun6i.c
@@ -5,6 +5,8 @@
* Copyright (C) 2014 Maxime Ripard
* Maxime Ripard <maxime...@free-electrons.com>
*
+ * Copyright (C) 2016 Karsten Merker <merker@@debian.org>
+ *
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License as
* published by the Free Software Foundation; either version 2 of
@@ -24,6 +26,7 @@
#include <linux/spi/spi.h>

#define SUN6I_FIFO_DEPTH 128
+#define SUN8I_A23_FIFO_DEPTH 64

#define SUN6I_GBL_CTL_REG 0x04
#define SUN6I_GBL_CTL_BUS_ENABLE BIT(0)
@@ -92,6 +95,18 @@ struct sun6i_spi {
int len;
};

+struct sun6i_spi_platform_data {
+ int fifo_depth;
+};
+
+static struct sun6i_spi_platform_data sun6i_a31_spi_platform_data = {
+ .fifo_depth = SUN6I_FIFO_DEPTH,
+};
+
+static struct sun6i_spi_platform_data sun8i_a23_spi_platform_data = {
+ .fifo_depth = SUN8I_A23_FIFO_DEPTH,
+};
+
static inline u32 sun6i_spi_read(struct sun6i_spi *sspi, u32 reg)
{
return readl(sspi->base_addr + reg);
@@ -155,7 +170,10 @@ static void sun6i_spi_set_cs(struct spi_device *spi, bool enable)

static size_t sun6i_spi_max_transfer_size(struct spi_device *spi)
{
- return SUN6I_FIFO_DEPTH - 1;
+ struct sun6i_spi_platform_data *pdata;
+
+ pdata = spi->dev.platform_data;
+ return (pdata->fifo_depth - 1);
}

static int sun6i_spi_transfer_one(struct spi_master *master,
@@ -168,9 +186,14 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
unsigned int tx_len = 0;
int ret = 0;
u32 reg;
+ int fifo_depth;
+ struct sun6i_spi_platform_data *pdata;

/* We don't support transfer larger than the FIFO */
- if (tfr->len > SUN6I_FIFO_DEPTH)
+ pdata = spi->dev.platform_data;
+ fifo_depth = pdata->fifo_depth;
+
+ if (tfr->len > fifo_depth)
return -EINVAL;

reinit_completion(&sspi->done);
@@ -265,7 +288,7 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
SUN6I_BURST_CTL_CNT_STC(tx_len));

/* Fill the TX FIFO */
- sun6i_spi_fill_fifo(sspi, SUN6I_FIFO_DEPTH);
+ sun6i_spi_fill_fifo(sspi, fifo_depth);

/* Enable the interrupts */
sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, SUN6I_INT_CTL_TC);
@@ -288,7 +311,7 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
goto out;
}

- sun6i_spi_drain_fifo(sspi, SUN6I_FIFO_DEPTH);
+ sun6i_spi_drain_fifo(sspi, fifo_depth);

out:
sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, 0);
@@ -470,7 +493,14 @@ static int sun6i_spi_remove(struct platform_device *pdev)
}

static const struct of_device_id sun6i_spi_match[] = {
- { .compatible = "allwinner,sun6i-a31-spi", },
+ {
+ .compatible = "allwinner,sun6i-a31-spi",
+ .data = &sun6i_a31_spi_platform_data,
+ },
+ {
+ .compatible = "allwinner,sun8i-a23-spi",
+ .data = &sun8i_a23_spi_platform_data,
+ },
{}
};
MODULE_DEVICE_TABLE(of, sun6i_spi_match);
@@ -493,5 +523,6 @@ module_platform_driver(sun6i_spi_driver);

MODULE_AUTHOR("Pan Nan <pan...@allwinnertech.com>");
MODULE_AUTHOR("Maxime Ripard <maxime...@free-electrons.com>");
-MODULE_DESCRIPTION("Allwinner A31 SPI controller driver");
+MODULE_AUTHOR("Karsten Merker <mer...@debian.org>");
+MODULE_DESCRIPTION("Allwinner A31/A23 SPI controller driver");
MODULE_LICENSE("GPL");
--
2.1.4

[Siarhei Siamashka]

Hello Karsten,

On Sun, 4 Sep 2016 18:53:13 +0200
Karsten Merker <mer...@debian.org> wrote:

> Hello everybody,
>
> during the mainlining discussion in the Olimex blog at
> https://olimex.wordpress.com/2016/08/30/free-electrons-add-mainline-linux-kernel-support-for-the-a13-allwinner-vpu/#comments
> the topic of A33 SPI support came up. Chen-Yu and Siarhei have
> pointed out that the chances are good that the A33 can work with
> the already existing SPI drivers and that simply nobody has taken
> a look at those in the context of the A33 yet - so I have taken
> that as an inspiration to take a look :-).

Yes, probably nobody just had an A33 board with SPI pins on an
expansion header. The A33 SoC is primarily used in tablets.

Well, basically it is really difficult to get people interested in
spending their time to look at this code at this point :-)

Please first test it on real hardware when the hardware arrives. And
then submit patches to the appropriate mailing list.

> Btw, while checking the docs I have stumbled over one thing that strikes me
> as rather strange: the A23/A33 has two SPI controllers (SPI0 and SPI1), but
> there is no documented pinmux that makes SPI1 available on any pins, so SPI1
> appears to be completely useless?
>
> Regards,
> Karsten
>
>
> Karsten Merker (2):
> spi: sunxi: Add Allwinner A23/A33 support to spi-sun6i
> ARM: dts: sun8i: Add SPI support to the Allwinner A23/A33 dtsi.
>
> .../devicetree/bindings/spi/spi-sun6i.txt | 6 ++-
> arch/arm/boot/dts/sun8i-a23-a33.dtsi | 38 +++++++++++++++++++
> arch/arm/boot/dts/sun8i-a23.dtsi | 12 ++++++
> arch/arm/boot/dts/sun8i-a33.dtsi | 12 ++++++
> drivers/spi/spi-sun6i.c | 43 +++++++++++++++++++---
> 5 files changed, 103 insertions(+), 8 deletions(-)
>



--

Best regards,
Siarhei Siamashka