[PATCH 0/4] Add Allwinner A31 SPI controller support

[PATCH 0/4] Add Allwinner A31 SPI controller support

[Maxime Ripard]

Hi everyone,

This patchset brings support for the SPI controller found in the
Allwinner A31 SoC.

Even though the controller supports DMA, the driver only supports PIO
mode for now. This driver will be used to bring up and test DMA on the
SoC, so support for the DMA will come eventually.

It doesn't support transfer larger than the FIFO size (128 bytes) for
now, so this is kind of a blocker against inclusion, but I expect it
to be fixed by v2.

Thanks!
Maxime

Maxime Ripard (4):
  clk: sunxi: Add support for PLL6 on the A31
  ARM: sun6i: dt: Add PLL6 and SPI module clocks
  spi: sunxi: Add Allwinner A31 SPI controller driver
  ARM: sun6i: dt: Add SPI controllers to the A31 DTSI

 Documentation/devicetree/bindings/clock/sunxi.txt  |   1 +
 .../devicetree/bindings/spi/spi-sun6i.txt          |  23 +
 arch/arm/boot/dts/sun6i-a31.dtsi                   |  88 +++-
 drivers/clk/sunxi/clk-sunxi.c                      |  45 ++
 drivers/spi/Makefile                               |   1 +
 drivers/spi/spi-sun6i.c                            | 463 +++++++++++++++++++++
 6 files changed, 611 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-sun6i.txt
 create mode 100644 drivers/spi/spi-sun6i.c

-- 
1.8.4.2

[PATCH 1/4] clk: sunxi: Add support for PLL6 on the A31

[Maxime Ripard]

The A31 has a slightly different PLL6 clock. Add support for this new clock in
our driver.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 Documentation/devicetree/bindings/clock/sunxi.txt |  1 +
 drivers/clk/sunxi/clk-sunxi.c                     | 45 +++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt
index c2cb762..954845c 100644
--- a/Documentation/devicetree/bindings/clock/sunxi.txt
+++ b/Documentation/devicetree/bindings/clock/sunxi.txt
@@ -11,6 +11,7 @@ Required properties:
 	"allwinner,sun6i-a31-pll1-clk" - for the main PLL clock on A31
 	"allwinner,sun4i-pll5-clk" - for the PLL5 clock
 	"allwinner,sun4i-pll6-clk" - for the PLL6 clock
+	"allwinner,sun6i-a31-pll6-clk" - for the PLL6 clock on A31
 	"allwinner,sun4i-cpu-clk" - for the CPU multiplexer clock
 	"allwinner,sun4i-axi-clk" - for the AXI clock
 	"allwinner,sun4i-axi-gates-clk" - for the AXI gates
diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
index 659e4ea..990ad5d 100644
--- a/drivers/clk/sunxi/clk-sunxi.c
+++ b/drivers/clk/sunxi/clk-sunxi.c
@@ -249,7 +249,38 @@ static void sun4i_get_pll5_factors(u32 *freq, u32 parent_rate,
 	*n = DIV_ROUND_UP(div, (*k+1));
 }
 
+/**
+ * sun6i_a31_get_pll6_factors() - calculates n, k factors for A31 PLL6
+ * PLL6 rate is calculated as follows
+ * rate = parent_rate * n * (k + 1) / 2
+ * parent_rate is always 24Mhz
+ */
+
+static void sun6i_a31_get_pll6_factors(u32 *freq, u32 parent_rate,
+				       u8 *n, u8 *k, u8 *m, u8 *p)
+{
+	u8 div;
+
+	/*
+	 * We always have 24MHz / 2, so we can just say that our
+	 * parent clock is 12MHz.
+	 */
+	parent_rate = parent_rate / 2;
+
+	/* Normalize value to a parent_rate multiple (24M / 2) */
+	div = *freq / parent_rate;
+	*freq = parent_rate * div;
+
+	/* we were called to round the frequency, we can now return */
+	if (n == NULL)
+		return;
+
+	*k = div / 32;
+	if (*k > 3)
+		*k = 3;
 
+	*n = DIV_ROUND_UP(div, (*k+1));
+}
 
 /**
  * sun4i_get_apb1_factors() - calculates m, p factors for APB1
@@ -416,6 +447,13 @@ static struct clk_factors_config sun4i_pll5_config = {
 	.kwidth = 2,
 };
 
+static struct clk_factors_config sun6i_a31_pll6_config = {
+	.nshift	= 8,
+	.nwidth = 5,
+	.kshift = 4,
+	.kwidth = 2,
+};
+
 static struct clk_factors_config sun4i_apb1_config = {
 	.mshift = 0,
 	.mwidth = 5,
@@ -457,6 +495,12 @@ static const struct factors_data sun4i_pll5_data __initconst = {
 	.getter = sun4i_get_pll5_factors,
 };
 
+static const struct factors_data sun6i_a31_pll6_data __initconst = {
+	.enable = 31,
+	.table = &sun6i_a31_pll6_config,
+	.getter = sun6i_a31_get_pll6_factors,
+};
+
 static const struct factors_data sun4i_apb1_data __initconst = {
 	.table = &sun4i_apb1_config,
 	.getter = sun4i_get_apb1_factors,
@@ -972,6 +1016,7 @@ free_clkdata:
 static const struct of_device_id clk_factors_match[] __initconst = {
 	{.compatible = "allwinner,sun4i-pll1-clk", .data = &sun4i_pll1_data,},
 	{.compatible = "allwinner,sun6i-a31-pll1-clk", .data = &sun6i_a31_pll1_data,},
+	{.compatible = "allwinner,sun6i-a31-pll6-clk", .data = &sun6i_a31_pll6_data,},
 	{.compatible = "allwinner,sun4i-apb1-clk", .data = &sun4i_apb1_data,},
 	{.compatible = "allwinner,sun4i-mod0-clk", .data = &sun4i_mod0_data,},
 	{.compatible = "allwinner,sun7i-a20-out-clk", .data = &sun7i_a20_out_data,},
-- 
1.8.4.2

Re: [PATCH 1/4] clk: sunxi: Add support for PLL6 on the A31

[Mike Turquette]

Quoting Maxime Ripard (2014-01-16 09:11:22)
> The A31 has a slightly different PLL6 clock. Add support for this new clock in
> our driver.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

This looks good to me. I guess it will be going in for 3.15 based on the
comments in the coverletter.

Regards,
Mike

> ---
>  Documentation/devicetree/bindings/clock/sunxi.txt |  1 +
>  drivers/clk/sunxi/clk-sunxi.c                     | 45 +++++++++++++++++++++++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt
> index c2cb762..954845c 100644
> --- a/Documentation/devicetree/bindings/clock/sunxi.txt
> +++ b/Documentation/devicetree/bindings/clock/sunxi.txt
> @@ -11,6 +11,7 @@ Required properties:
>         "allwinner,sun6i-a31-pll1-clk" - for the main PLL clock on A31
>         "allwinner,sun4i-pll5-clk" - for the PLL5 clock
>         "allwinner,sun4i-pll6-clk" - for the PLL6 clock
> +       "allwinner,sun6i-a31-pll6-clk" - for the PLL6 clock on A31
>         "allwinner,sun4i-cpu-clk" - for the CPU multiplexer clock
>         "allwinner,sun4i-axi-clk" - for the AXI clock
>         "allwinner,sun4i-axi-gates-clk" - for the AXI gates
> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
> index 659e4ea..990ad5d 100644
> --- a/drivers/clk/sunxi/clk-sunxi.c
> +++ b/drivers/clk/sunxi/clk-sunxi.c
> @@ -249,7 +249,38 @@ static void sun4i_get_pll5_factors(u32 *freq, u32 parent_rate,
>         *n = DIV_ROUND_UP(div, (*k+1));
>  }
>  
> +/**
> + * sun6i_a31_get_pll6_factors() - calculates n, k factors for A31 PLL6
> + * PLL6 rate is calculated as follows
> + * rate = parent_rate * n * (k + 1) / 2
> + * parent_rate is always 24Mhz
> + */
> +
> +static void sun6i_a31_get_pll6_factors(u32 *freq, u32 parent_rate,
> +                                      u8 *n, u8 *k, u8 *m, u8 *p)
> +{
> +       u8 div;
> +
> +       /*
> +        * We always have 24MHz / 2, so we can just say that our
> +        * parent clock is 12MHz.
> +        */
> +       parent_rate = parent_rate / 2;
> +
> +       /* Normalize value to a parent_rate multiple (24M / 2) */
> +       div = *freq / parent_rate;
> +       *freq = parent_rate * div;
> +
> +       /* we were called to round the frequency, we can now return */
> +       if (n == NULL)
> +               return;
> +
> +       *k = div / 32;
> +       if (*k > 3)
> +               *k = 3;
>  
> +       *n = DIV_ROUND_UP(div, (*k+1));
> +}
>  
>  /**
>   * sun4i_get_apb1_factors() - calculates m, p factors for APB1
> @@ -416,6 +447,13 @@ static struct clk_factors_config sun4i_pll5_config = {
>         .kwidth = 2,
>  };
>  
> +static struct clk_factors_config sun6i_a31_pll6_config = {
> +       .nshift = 8,
> +       .nwidth = 5,
> +       .kshift = 4,
> +       .kwidth = 2,
> +};
> +
>  static struct clk_factors_config sun4i_apb1_config = {
>         .mshift = 0,
>         .mwidth = 5,
> @@ -457,6 +495,12 @@ static const struct factors_data sun4i_pll5_data __initconst = {
>         .getter = sun4i_get_pll5_factors,
>  };
>  
> +static const struct factors_data sun6i_a31_pll6_data __initconst = {
> +       .enable = 31,
> +       .table = &sun6i_a31_pll6_config,
> +       .getter = sun6i_a31_get_pll6_factors,
> +};
> +
>  static const struct factors_data sun4i_apb1_data __initconst = {
>         .table = &sun4i_apb1_config,
>         .getter = sun4i_get_apb1_factors,
> @@ -972,6 +1016,7 @@ free_clkdata:
>  static const struct of_device_id clk_factors_match[] __initconst = {
>         {.compatible = "allwinner,sun4i-pll1-clk", .data = &sun4i_pll1_data,},
>         {.compatible = "allwinner,sun6i-a31-pll1-clk", .data = &sun6i_a31_pll1_data,},
> +       {.compatible = "allwinner,sun6i-a31-pll6-clk", .data = &sun6i_a31_pll6_data,},
>         {.compatible = "allwinner,sun4i-apb1-clk", .data = &sun4i_apb1_data,},
>         {.compatible = "allwinner,sun4i-mod0-clk", .data = &sun4i_mod0_data,},
>         {.compatible = "allwinner,sun7i-a20-out-clk", .data = &sun7i_a20_out_data,},
> -- 
> 1.8.4.2
> 

Re: [PATCH 1/4] clk: sunxi: Add support for PLL6 on the A31

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 572 bytes --]

Hi Mike,

On Fri, Jan 17, 2014 at 02:14:02PM -0800, Mike Turquette wrote:
> Quoting Maxime Ripard (2014-01-16 09:11:22)
> > The A31 has a slightly different PLL6 clock. Add support for this new clock in
> > our driver.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> 
> This looks good to me. I guess it will be going in for 3.15 based on the
> comments in the coverletter.

Yes, indeed it is 3.15 materials.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 2/4] ARM: sun6i: dt: Add PLL6 and SPI module clocks

[Maxime Ripard]

The module clocks in the A31 are still compatible with the A10 one. Add the SPI
module clocks and the PLL6 in the device tree to allow their use by the SPI
controllers.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 arch/arm/boot/dts/sun6i-a31.dtsi | 48 +++++++++++++++++++++++++++++++---------
 1 file changed, 38 insertions(+), 10 deletions(-)

diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
index 5256ad9..ae058eb 100644
--- a/arch/arm/boot/dts/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/sun6i-a31.dtsi
@@ -73,16 +73,12 @@
 			clocks = <&osc24M>;
 		};
 
-		/*
-		 * This is a dummy clock, to be used as placeholder on
-		 * other mux clocks when a specific parent clock is not
-		 * yet implemented. It should be dropped when the driver
-		 * is complete.
-		 */
-		pll6: pll6 {
-			#clock-cells = <0>;
-			compatible = "fixed-clock";
-			clock-frequency = <0>;
+		pll6: clk@01c20028 {
+			#clock-cells = <1>;
+			compatible = "allwinner,sun6i-a31-pll6-clk";
+			reg = <0x01c20028 0x4>;
+			clocks = <&osc24M>;
+			clock-output-names = "pll6";
 		};
 
 		cpu: cpu@01c20050 {
@@ -182,6 +178,38 @@
 					"apb2_uart1", "apb2_uart2", "apb2_uart3",
 					"apb2_uart4", "apb2_uart5";
 		};
+
+		spi0_clk: clk@01c200a0 {
+			#clock-cells = <0>;
+			compatible = "allwinner,sun4i-mod0-clk";
+			reg = <0x01c200a0 0x4>;
+			clocks = <&osc24M>, <&pll6>;
+			clock-output-names = "spi0";
+		};
+
+		spi1_clk: clk@01c200a4 {
+			#clock-cells = <0>;
+			compatible = "allwinner,sun4i-mod0-clk";
+			reg = <0x01c200a4 0x4>;
+			clocks = <&osc24M>, <&pll6>;
+			clock-output-names = "spi1";
+		};
+
+		spi2_clk: clk@01c200a8 {
+			#clock-cells = <0>;
+			compatible = "allwinner,sun4i-mod0-clk";
+			reg = <0x01c200a8 0x4>;
+			clocks = <&osc24M>, <&pll6>;
+			clock-output-names = "spi2";
+		};
+
+		spi3_clk: clk@01c200ac {
+			#clock-cells = <0>;
+			compatible = "allwinner,sun4i-mod0-clk";
+			reg = <0x01c200ac 0x4>;
+			clocks = <&osc24M>, <&pll6>;
+			clock-output-names = "spi3";
+		};
 	};
 
 	soc@01c00000 {
-- 
1.8.4.2

Re: [PATCH 2/4] ARM: sun6i: dt: Add PLL6 and SPI module clocks

[Josh Cartwright]

On Thu, Jan 16, 2014 at 06:11:23PM +0100, Maxime Ripard wrote:
> The module clocks in the A31 are still compatible with the A10 one. Add the SPI
> module clocks and the PLL6 in the device tree to allow their use by the SPI
> controllers.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  arch/arm/boot/dts/sun6i-a31.dtsi | 48 +++++++++++++++++++++++++++++++---------
>  1 file changed, 38 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
> index 5256ad9..ae058eb 100644
> --- a/arch/arm/boot/dts/sun6i-a31.dtsi
> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
> @@ -73,16 +73,12 @@
>  			clocks = <&osc24M>;
>  		};
>  
> -		/*
> -		 * This is a dummy clock, to be used as placeholder on
> -		 * other mux clocks when a specific parent clock is not
> -		 * yet implemented. It should be dropped when the driver
> -		 * is complete.
> -		 */
> -		pll6: pll6 {
> -			#clock-cells = <0>;
> -			compatible = "fixed-clock";
> -			clock-frequency = <0>;
> +		pll6: clk@01c20028 {
> +			#clock-cells = <1>;
> +			compatible = "allwinner,sun6i-a31-pll6-clk";
> +			reg = <0x01c20028 0x4>;
> +			clocks = <&osc24M>;
> +			clock-output-names = "pll6";
>  		};
>  
>  		cpu: cpu@01c20050 {
> @@ -182,6 +178,38 @@
>  					"apb2_uart1", "apb2_uart2", "apb2_uart3",
>  					"apb2_uart4", "apb2_uart5";
>  		};
> +
> +		spi0_clk: clk@01c200a0 {
> +			#clock-cells = <0>;
> +			compatible = "allwinner,sun4i-mod0-clk";
> +			reg = <0x01c200a0 0x4>;
> +			clocks = <&osc24M>, <&pll6>;

This looks weird.  You've set the pll6 #clock-cells = <1>, but you
aren't using a specifier here.  Same below, as well.  The binding
documentation indicates that #clock-cells should be 0 for the pll6 node.

  Josh

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 2/4] ARM: sun6i: dt: Add PLL6 and SPI module clocks

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2258 bytes --]

Hi Josh,

On Thu, Jan 16, 2014 at 12:15:28PM -0600, Josh Cartwright wrote:
> On Thu, Jan 16, 2014 at 06:11:23PM +0100, Maxime Ripard wrote:
> > The module clocks in the A31 are still compatible with the A10 one. Add the SPI
> > module clocks and the PLL6 in the device tree to allow their use by the SPI
> > controllers.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > ---
> >  arch/arm/boot/dts/sun6i-a31.dtsi | 48 +++++++++++++++++++++++++++++++---------
> >  1 file changed, 38 insertions(+), 10 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
> > index 5256ad9..ae058eb 100644
> > --- a/arch/arm/boot/dts/sun6i-a31.dtsi
> > +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
> > @@ -73,16 +73,12 @@
> >  			clocks = <&osc24M>;
> >  		};
> >  
> > -		/*
> > -		 * This is a dummy clock, to be used as placeholder on
> > -		 * other mux clocks when a specific parent clock is not
> > -		 * yet implemented. It should be dropped when the driver
> > -		 * is complete.
> > -		 */
> > -		pll6: pll6 {
> > -			#clock-cells = <0>;
> > -			compatible = "fixed-clock";
> > -			clock-frequency = <0>;
> > +		pll6: clk@01c20028 {
> > +			#clock-cells = <1>;
> > +			compatible = "allwinner,sun6i-a31-pll6-clk";
> > +			reg = <0x01c20028 0x4>;
> > +			clocks = <&osc24M>;
> > +			clock-output-names = "pll6";
> >  		};
> >  
> >  		cpu: cpu@01c20050 {
> > @@ -182,6 +178,38 @@
> >  					"apb2_uart1", "apb2_uart2", "apb2_uart3",
> >  					"apb2_uart4", "apb2_uart5";
> >  		};
> > +
> > +		spi0_clk: clk@01c200a0 {
> > +			#clock-cells = <0>;
> > +			compatible = "allwinner,sun4i-mod0-clk";
> > +			reg = <0x01c200a0 0x4>;
> > +			clocks = <&osc24M>, <&pll6>;
> 
> This looks weird.  You've set the pll6 #clock-cells = <1>, but you
> aren't using a specifier here.  Same below, as well.  The binding
> documentation indicates that #clock-cells should be 0 for the pll6 node.

Ah, right, it's a dumb copy/paste mistake.

I'd expect dtc to output a warning/error in such case, but apparently
it doesn't.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 3/4] spi: sunxi: Add Allwinner A31 SPI controller driver

[Maxime Ripard]

The Allwinner A31 has a new SPI controller IP compared to the older Allwinner
SoCs.

It supports DMA, but the driver only does PIO for now, and DMA will be
supported eventually.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 .../devicetree/bindings/spi/spi-sun6i.txt          |  23 +
 drivers/spi/Makefile                               |   1 +
 drivers/spi/spi-sun6i.c                            | 463 +++++++++++++++++++++
 3 files changed, 487 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-sun6i.txt
 create mode 100644 drivers/spi/spi-sun6i.c

diff --git a/Documentation/devicetree/bindings/spi/spi-sun6i.txt b/Documentation/devicetree/bindings/spi/spi-sun6i.txt
new file mode 100644
index 0000000..019a05f
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spi-sun6i.txt
@@ -0,0 +1,23 @@
+Allwinner A31 SPI controller
+
+Required properties:
+- compatible: Should be "allwinner,sun6i-a31-spi".
+- reg: Should contain register location and length.
+- interrupts: Should contain interrupt.
+- clocks: phandle to the clocks feeding the SPI controller. Two are
+          needed:
+  - "ahb": the gated AHB parent clock
+  - "mod": the parent module clock
+- resets: phandle to the reset controller asserting this device in
+          reset
+
+Example:
+
+spi1: spi@01c69000 {
+	compatible = "allwinner,sun6i-a31-spi";
+	reg = <0x01c69000 0x1000>;
+	interrupts = <0 66 4>;
+	clocks = <&ahb1_gates 21>, <&spi1_clk>;
+	clock-names = "ahb", "mod";
+	resets = <&ahb1_rst 21>;
+};
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index ab8d864..b94f43d 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -69,6 +69,7 @@ obj-$(CONFIG_SPI_SH_HSPI)		+= spi-sh-hspi.o
 obj-$(CONFIG_SPI_SH_MSIOF)		+= spi-sh-msiof.o
 obj-$(CONFIG_SPI_SH_SCI)		+= spi-sh-sci.o
 obj-$(CONFIG_SPI_SIRF)		+= spi-sirf.o
+obj-$(CONFIG_ARCH_SUNXI)		+= spi-sun6i.o
 obj-$(CONFIG_SPI_TEGRA114)		+= spi-tegra114.o
 obj-$(CONFIG_SPI_TEGRA20_SFLASH)	+= spi-tegra20-sflash.o
 obj-$(CONFIG_SPI_TEGRA20_SLINK)		+= spi-tegra20-slink.o
diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
new file mode 100644
index 0000000..92f0343
--- /dev/null
+++ b/drivers/spi/spi-sun6i.c
@@ -0,0 +1,463 @@
+/*
+ * Copyright (C) 2012 - 2014 Allwinner Tech
+ * Pan Nan <pannan@allwinnertech.com>
+ *
+ * Copyright (C) 2014 Maxime Ripard
+ * Maxime Ripard <maxime.ripard@free-electrons.com>
+ *
+ * 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
+ * the License, or (at your option) any later version.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+#include <linux/workqueue.h>
+
+#include <linux/spi/spi.h>
+
+#define SUN6I_FIFO_DEPTH		128
+
+#define SUN6I_GBL_CTL_REG		0x04
+#define SUN6I_GBL_CTL_BUS_ENABLE		BIT(0)
+#define SUN6I_GBL_CTL_MASTER			BIT(1)
+#define SUN6I_GBL_CTL_TP			BIT(7)
+#define SUN6I_GBL_CTL_RST			BIT(31)
+
+#define SUN6I_TFR_CTL_REG		0x08
+#define SUN6I_TFR_CTL_CPHA			BIT(0)
+#define SUN6I_TFR_CTL_CPOL			BIT(1)
+#define SUN6I_TFR_CTL_SPOL			BIT(2)
+#define SUN6I_TFR_CTL_CS_MASK			0x3
+#define SUN6I_TFR_CTL_CS(cs)			(((cs) & SUN6I_TFR_CTL_CS_MASK) << 4)
+#define SUN6I_TFR_CTL_FBS			BIT(12)
+#define SUN6I_TFR_CTL_XCH			BIT(31)
+
+#define SUN6I_INT_CTL_REG		0x10
+#define SUN6I_INT_CTL_RF_OVF			BIT(8)
+#define SUN6I_INT_CTL_TC			BIT(12)
+
+#define SUN6I_INT_STA_REG		0x14
+
+#define SUN6I_FIFO_CTL_REG		0x18
+#define SUN6I_FIFO_CTL_RF_RST			BIT(15)
+#define SUN6I_FIFO_CTL_TF_RST			BIT(31)
+
+#define SUN6I_FIFO_STA_REG		0x1c
+#define SUN6I_FIFO_STA_RF_CNT_MASK		0x7f
+#define SUN6I_FIFO_STA_RF_CNT_BITS		0
+#define SUN6I_FIFO_STA_TF_CNT_MASK		0x7f
+#define SUN6I_FIFO_STA_TF_CNT_BITS		16
+
+#define SUN6I_CLK_CTL_REG		0x24
+#define SUN6I_CLK_CTL_CDR2_MASK			0xff
+#define SUN6I_CLK_CTL_CDR2(div)			(((div) & SUN6I_CLK_CTL_CDR2_MASK) << 0)
+#define SUN6I_CLK_CTL_CDR1_MASK			0xf
+#define SUN6I_CLK_CTL_CDR1(div)			(((div) & SUN6I_CLK_CTL_CDR1_MASK) << 8)
+#define SUN6I_CLK_CTL_DRS			BIT(12)
+
+#define SUN6I_BURST_CNT_REG		0x30
+#define SUN6I_BURST_CNT(cnt)			((cnt) & 0xffffff)
+
+#define SUN6I_XMIT_CNT_REG		0x34
+#define SUN6I_XMIT_CNT(cnt)			((cnt) & 0xffffff)
+
+#define SUN6I_BURST_CTL_CNT_REG		0x38
+#define SUN6I_BURST_CTL_CNT_STC(cnt)		((cnt) & 0xffffff)
+
+#define SUN6I_TXDATA_REG		0x200
+#define SUN6I_RXDATA_REG		0x300
+
+struct sun6i_spi {
+	struct spi_master	*master;
+	void __iomem		*base_addr;
+	struct clk		*hclk;
+	struct clk		*mclk;
+	struct reset_control	*rstc;
+
+	struct completion	done;
+
+	const u8		*tx_buf;
+	u8			*rx_buf;
+	int			len;
+};
+
+static inline u32 sun6i_spi_read(struct sun6i_spi *sspi, u32 reg)
+{
+	return readl(sspi->base_addr + reg);
+}
+
+static inline void sun6i_spi_write(struct sun6i_spi *sspi, u32 reg, u32 value)
+{
+	writel(value, sspi->base_addr + reg);
+}
+
+static inline void sun6i_spi_drain_fifo(struct sun6i_spi *sspi, int len)
+{
+	u32 reg, cnt;
+	u8 byte;
+
+	/* See how much data are available */
+	reg = sun6i_spi_read(sspi, SUN6I_FIFO_STA_REG);
+	reg &= SUN6I_FIFO_STA_RF_CNT_MASK;
+	cnt = reg >> SUN6I_FIFO_STA_RF_CNT_BITS;
+
+	if (len > cnt)
+		len = cnt;
+
+	while (len--) {
+		byte = readb(sspi->base_addr + SUN6I_RXDATA_REG);
+		if (sspi->rx_buf)
+			*sspi->rx_buf++ = byte;
+	}
+}
+
+static inline void sun6i_spi_fill_fifo(struct sun6i_spi *sspi, int len)
+{
+	u8 byte;
+
+	if (len > sspi->len)
+		len = sspi->len;
+
+	while (len--) {
+		byte = sspi->tx_buf ? *sspi->tx_buf++ : 0;
+		writeb(byte, sspi->base_addr + SUN6I_TXDATA_REG);
+		sspi->len--;
+	}
+}
+
+static int sun6i_spi_start_transfer(struct spi_device *spi,
+				    struct spi_transfer *tfr)
+{
+	struct sun6i_spi *sspi = spi_master_get_devdata(spi->master);
+	unsigned int bursts = 0, tx_len = 0;
+	unsigned int mclk_rate, div;
+	u32 reg;
+
+	reinit_completion(&sspi->done);
+	sspi->tx_buf = tfr->tx_buf;
+	sspi->rx_buf = tfr->rx_buf;
+	sspi->len = tfr->len;
+
+	/* Clear pending interrupts */
+	sun6i_spi_write(sspi, SUN6I_INT_STA_REG, ~0);
+
+	/* Reset FIFO */
+	sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
+			SUN6I_FIFO_CTL_RF_RST | SUN6I_FIFO_CTL_TF_RST);
+
+	/*
+	 * Setup the transfer control register: Chip Select,
+	 * polarities, etc.
+	 */
+	reg = SUN6I_TFR_CTL_CS(spi->chip_select);
+
+	if (spi->mode & SPI_CPOL)
+		reg |= SUN6I_TFR_CTL_CPOL;
+
+	if (spi->mode & SPI_CPHA)
+		reg |= SUN6I_TFR_CTL_CPHA;
+
+	if (!(spi->mode & SPI_CS_HIGH))
+		reg |= SUN6I_TFR_CTL_SPOL;
+
+	if (spi->mode & SPI_LSB_FIRST)
+		reg |= SUN6I_TFR_CTL_FBS;
+
+	sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg);
+
+	/*
+	 * Setup clock divider.
+	 *
+	 * We have two choices there. Either we can use the clock
+	 * divide rate 1, which is calculated thanks to this formula:
+	 * SPI_CLK = MOD_CLK / (2 ^ cdr)
+	 * Or we can use CDR2, which is calculated with the formula:
+	 * SPI_CLK = MOD_CLK / (2 * (cdr + 1))
+	 * Wether we use the former or the latter is set through the
+	 * DRS bit.
+	 *
+	 * First try CDR2, and if we can't reach the expected
+	 * frequency, fall back to CDR1.
+	 */
+	mclk_rate = clk_get_rate(sspi->mclk);
+	div = mclk_rate / (spi->max_speed_hz << 1);
+
+	if (div <= (SUN6I_CLK_CTL_CDR2_MASK + 1)) {
+		if (div > 0)
+			div--;
+
+		reg = SUN6I_CLK_CTL_CDR2(div) | SUN6I_CLK_CTL_DRS;
+	} else {
+		div = ilog2(mclk_rate) - ilog2(spi->max_speed_hz);
+		reg = SUN6I_CLK_CTL_CDR1(div);
+	}
+
+	sun6i_spi_write(sspi, SUN6I_CLK_CTL_REG, reg);
+
+	/* Setup the transfer now... */
+	if (sspi->rx_buf && sspi->tx_buf) {
+		bursts = tfr->len + 1;
+		tx_len = tfr->len;
+	} else if (sspi->tx_buf) {
+		bursts = tfr->len;
+		tx_len = tfr->len;
+	} else if (sspi->rx_buf) {
+		bursts = tfr->len;
+	}
+
+	/* Setup the counters */
+	sun6i_spi_write(sspi, SUN6I_BURST_CNT_REG, SUN6I_BURST_CNT(bursts));
+	sun6i_spi_write(sspi, SUN6I_XMIT_CNT_REG, SUN6I_XMIT_CNT(tx_len));
+	sun6i_spi_write(sspi, SUN6I_BURST_CTL_CNT_REG,
+			SUN6I_BURST_CTL_CNT_STC(tx_len));
+
+	/* Fill the TX FIFO */
+	sun6i_spi_fill_fifo(sspi, SUN6I_FIFO_DEPTH);
+
+	/* Enable the interrupts */
+	sun6i_spi_write(sspi, SUN6I_INT_CTL_REG,
+			SUN6I_INT_CTL_TC | SUN6I_INT_CTL_RF_OVF);
+
+	/* Start the transfer */
+	reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
+	sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg | SUN6I_TFR_CTL_XCH);
+
+	return 0;
+}
+
+static int sun6i_spi_finish_transfer(struct spi_device *spi,
+				     struct spi_transfer *tfr,
+				     bool cs_change)
+{
+	struct sun6i_spi *sspi = spi_master_get_devdata(spi->master);
+
+	sun6i_spi_drain_fifo(sspi, SUN6I_FIFO_DEPTH);
+
+	if (tfr->delay_usecs)
+		udelay(tfr->delay_usecs);
+
+	return 0;
+}
+
+static int sun6i_spi_transfer_one(struct spi_master *master,
+				  struct spi_message *mesg)
+{
+	struct sun6i_spi *sspi = spi_master_get_devdata(master);
+	struct spi_device *spi = mesg->spi;
+	struct spi_transfer *tfr;
+	unsigned int timeout;
+	int cs_change, err;
+	u32 reg;
+
+	list_for_each_entry(tfr, &mesg->transfers, transfer_list) {
+		err = sun6i_spi_start_transfer(spi, tfr);
+		if (err)
+			goto out;
+
+		timeout = wait_for_completion_timeout(&sspi->done,
+						      msecs_to_jiffies(1000));
+		if (!timeout) {
+			err = -ETIMEDOUT;
+			goto out;
+		}
+
+		cs_change = tfr->cs_change ||
+			list_is_last(&tfr->transfer_list, &mesg->transfers);
+
+		err = sun6i_spi_finish_transfer(spi, tfr, cs_change);
+		if (err)
+			goto out;
+
+		mesg->actual_length += tfr->len;
+	}
+
+out:
+	reg = sun6i_spi_read(sspi, SUN6I_INT_CTL_REG);
+	sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, reg & ~SUN6I_INT_CTL_TC);
+
+	mesg->status = err;
+	spi_finalize_current_message(master);
+	return 0;
+}
+
+/* wake up the sleep thread, and give the result code */
+static irqreturn_t sun6i_spi_handler(int irq, void *dev_id)
+{
+	struct sun6i_spi *sspi = dev_id;
+	u32 status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG);
+
+	/* Transfer complete */
+	if (status & SUN6I_INT_CTL_TC) {
+		sun6i_spi_write(sspi, SUN6I_INT_STA_REG, SUN6I_INT_CTL_TC);
+		complete(&sspi->done);
+		return IRQ_HANDLED;
+	}
+
+	if (status & SUN6I_INT_CTL_RF_OVF) {
+		sun6i_spi_drain_fifo(sspi, SUN6I_FIFO_DEPTH);
+		sun6i_spi_write(sspi, SUN6I_INT_STA_REG, SUN6I_INT_CTL_RF_OVF);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+static int sun6i_spi_probe(struct platform_device *pdev)
+{
+	struct spi_master *master;
+	struct sun6i_spi *sspi;
+	struct resource	*res;
+	int ret = 0, irq;
+
+	master = spi_alloc_master(&pdev->dev, sizeof(struct sun6i_spi));
+	if (!master) {
+		dev_err(&pdev->dev, "Unable to allocate SPI Master\n");
+		return -ENOMEM;
+	}
+
+	platform_set_drvdata(pdev, master);
+	sspi = spi_master_get_devdata(master);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	sspi->base_addr = devm_request_and_ioremap(&pdev->dev, res);
+	if (!sspi->base_addr) {
+		dev_err(&pdev->dev, "Unable to remap IO\n");
+		ret = -ENXIO;
+		goto err;
+	}
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "No spi IRQ specified\n");
+		ret = -ENXIO;
+		goto err;
+	}
+
+	ret = devm_request_irq(&pdev->dev, irq, sun6i_spi_handler,
+			       0, "sun6i-spi", sspi);
+	if (ret) {
+		dev_err(&pdev->dev, "Cannot request IRQ\n");
+		goto err;
+	}
+
+	sspi->master = master;
+	master->bus_num	= -1;
+	master->transfer_one_message = sun6i_spi_transfer_one;
+	master->num_chipselect = 4;
+	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LSB_FIRST;
+	master->dev.of_node = pdev->dev.of_node;
+
+	/* Setup clocks */
+	sspi->hclk = devm_clk_get(&pdev->dev, "ahb");
+	if (IS_ERR(sspi->hclk)) {
+		dev_err(&pdev->dev, "Unable to acquire AHB clock\n");
+		ret = PTR_ERR(sspi->hclk);
+		goto err;
+	}
+
+	ret = clk_prepare_enable(sspi->hclk);
+	if (ret) {
+		dev_err(&pdev->dev, "Couldn't enable clock 'ahb spi'\n");
+		goto err;
+	}
+
+	sspi->mclk = devm_clk_get(&pdev->dev, "mod");
+	if (IS_ERR(sspi->mclk)) {
+		dev_err(&pdev->dev, "Unable to acquire module clock\n");
+		ret = PTR_ERR(sspi->mclk);
+		goto err2;
+	}
+
+	ret = clk_set_rate(sspi->mclk, 100000000);
+	if (ret) {
+		dev_err(&pdev->dev, "Couldn't change module clock rate\n");
+		goto err2;
+	}
+
+	ret = clk_prepare_enable(sspi->mclk);
+	if (ret) {
+		dev_err(&pdev->dev, "Couldn't enable clock 'ahb spi'\n");
+		goto err2;
+	}
+
+	init_completion(&sspi->done);
+
+	sspi->rstc = devm_reset_control_get(&pdev->dev, NULL);
+	if (IS_ERR(sspi->rstc)) {
+		dev_err(&pdev->dev, "Couldn't get reset controller\n");
+		ret = PTR_ERR(sspi->rstc);
+		goto err3;
+	}
+
+	ret = reset_control_deassert(sspi->rstc);
+	if (ret) {
+		dev_err(&pdev->dev, "Couldn't deassert the device from reset\n");
+		goto err3;
+	}
+
+	sun6i_spi_write(sspi, SUN6I_GBL_CTL_REG,
+			SUN6I_GBL_CTL_BUS_ENABLE | SUN6I_GBL_CTL_MASTER | SUN6I_GBL_CTL_TP);
+
+	ret = spi_register_master(master);
+	if (ret) {
+		dev_err(&pdev->dev, "cannot register SPI master\n");
+		goto err4;
+	}
+
+	return 0;
+
+err4:
+	reset_control_assert(sspi->rstc);
+err3:
+	clk_disable_unprepare(sspi->mclk);
+err2:
+	clk_disable_unprepare(sspi->hclk);
+err:
+	spi_master_put(master);
+
+	return ret;
+}
+
+static int sun6i_spi_remove(struct platform_device *pdev)
+{
+	struct spi_master *master = spi_master_get(platform_get_drvdata(pdev));
+	struct sun6i_spi *sspi = spi_master_get_devdata(master);
+
+	spi_unregister_master(master);
+	reset_control_assert(sspi->rstc);
+	clk_disable_unprepare(sspi->mclk);
+	clk_disable_unprepare(sspi->hclk);
+	spi_master_put(master);
+
+	return 0;
+}
+
+static const struct of_device_id sun6i_spi_match[] = {
+	{ .compatible = "allwinner,sun6i-a31-spi", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, sun6i_spi_match);
+
+static struct platform_driver sun6i_spi_driver = {
+	.probe	= sun6i_spi_probe,
+	.remove	= sun6i_spi_remove,
+	.driver	= {
+		.name		= "sun6i-spi",
+		.owner		= THIS_MODULE,
+		.of_match_table	= sun6i_spi_match,
+	},
+};
+module_platform_driver(sun6i_spi_driver);
+
+MODULE_AUTHOR("Pan Nan <pannan@allwinnertech.com>");
+MODULE_AUTHOR("Maxime Ripard <maxime.ripard@free-electrons.com>");
+MODULE_DESCRIPTION("Allwinner A31 SPI controller driver");
+MODULE_LICENSE("GPL");
-- 
1.8.4.2

Re: [PATCH 3/4] spi: sunxi: Add Allwinner A31 SPI controller driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3752 bytes --]

On Thu, Jan 16, 2014 at 06:11:24PM +0100, Maxime Ripard wrote:

Looks pretty clean, a few fairly small things below.

> +- clocks: phandle to the clocks feeding the SPI controller. Two are
> +          needed:
> +  - "ahb": the gated AHB parent clock
> +  - "mod": the parent module clock

I guess you should specify that this needs to be done with clock-names
too then?

> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -69,6 +69,7 @@ obj-$(CONFIG_SPI_SH_HSPI)		+= spi-sh-hspi.o
>  obj-$(CONFIG_SPI_SH_MSIOF)		+= spi-sh-msiof.o
>  obj-$(CONFIG_SPI_SH_SCI)		+= spi-sh-sci.o
>  obj-$(CONFIG_SPI_SIRF)		+= spi-sirf.o
> +obj-$(CONFIG_ARCH_SUNXI)		+= spi-sun6i.o

I would expect a new Kconfig symbol for this.

> +static inline void sun6i_spi_drain_fifo(struct sun6i_spi *sspi, int len)
> +{
> +	u32 reg, cnt;
> +	u8 byte;
> +
> +	/* See how much data are available */

data is available.

> +	while (len--) {
> +		byte = readb(sspi->base_addr + SUN6I_RXDATA_REG);
> +		if (sspi->rx_buf)
> +			*sspi->rx_buf++ = byte;
> +	}

It seems like this hardware is only able to handle bidirectional
operation - this is actually quite common and isn't always as simple as
it is here.  Can I persuade you to put something in the core which
provides dummy data buffers for this case?  I was thinking flags like
must_tx and must_rx or something but didn't get around to this yet.

> +static int sun6i_spi_finish_transfer(struct spi_device *spi,
> +				     struct spi_transfer *tfr,
> +				     bool cs_change)
> +{
> +	struct sun6i_spi *sspi = spi_master_get_devdata(spi->master);
> +
> +	sun6i_spi_drain_fifo(sspi, SUN6I_FIFO_DEPTH);
> +
> +	if (tfr->delay_usecs)
> +		udelay(tfr->delay_usecs);

If you implement this using transfer_one() (as you should) the core will
do this for you.

> +	if (status & SUN6I_INT_CTL_RF_OVF) {
> +		sun6i_spi_drain_fifo(sspi, SUN6I_FIFO_DEPTH);
> +		sun6i_spi_write(sspi, SUN6I_INT_STA_REG, SUN6I_INT_CTL_RF_OVF);
> +		return IRQ_HANDLED;
> +	}

This looks like an overflow - a log message would be helpful for users
and you should possibly be flagging an error on the current transfer.

> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	sspi->base_addr = devm_request_and_ioremap(&pdev->dev, res);
> +	if (!sspi->base_addr) {
> +		dev_err(&pdev->dev, "Unable to remap IO\n");
> +		ret = -ENXIO;
> +		goto err;
> +	}

devm_ioremap_resource() is nicer in that it returns an error and then
you don't need to log either since it's noisy itself.

> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		dev_err(&pdev->dev, "No spi IRQ specified\n");
> +		ret = -ENXIO;

Don't overwrite the error code.

> +	ret = clk_set_rate(sspi->mclk, 100000000);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Couldn't change module clock rate\n");
> +		goto err2;
> +	}

Does this really need to be fatal (or done at all)?  There seems to be
another reasonably flexible divider in the IP and it's more common to
either set this per transfer to something that rounds nicely or just use
the default and rely on the dividers.

> +	ret = clk_prepare_enable(sspi->mclk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Couldn't enable clock 'ahb spi'\n");
> +		goto err2;
> +	}

I would recommend moving these to runtime PM so the clocks are only
active when the device is actually in use, the core will do the runtime
PM management if you set auto_runtime_pm so it's really easy to
implement.

> +	ret = reset_control_deassert(sspi->rstc);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Couldn't deassert the device from reset\n");
> +		goto err3;
> +	}
> +
> +	sun6i_spi_write(sspi, SUN6I_GBL_CTL_REG,
> +			SUN6I_GBL_CTL_BUS_ENABLE | SUN6I_GBL_CTL_MASTER | SUN6I_GBL_CTL_TP);

Similarly here the IP could be kept in reset when not in use.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/4] spi: sunxi: Add Allwinner A31 SPI controller driver

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 5296 bytes --]

Hi Mark,

On Thu, Jan 16, 2014 at 07:40:03PM +0000, Mark Brown wrote:
> On Thu, Jan 16, 2014 at 06:11:24PM +0100, Maxime Ripard wrote:
> 
> Looks pretty clean, a few fairly small things below.
> 
> > +- clocks: phandle to the clocks feeding the SPI controller. Two are
> > +          needed:
> > +  - "ahb": the gated AHB parent clock
> > +  - "mod": the parent module clock
> 
> I guess you should specify that this needs to be done with clock-names
> too then?

Yep, right.

> > --- a/drivers/spi/Makefile
> > +++ b/drivers/spi/Makefile
> > @@ -69,6 +69,7 @@ obj-$(CONFIG_SPI_SH_HSPI)		+= spi-sh-hspi.o
> >  obj-$(CONFIG_SPI_SH_MSIOF)		+= spi-sh-msiof.o
> >  obj-$(CONFIG_SPI_SH_SCI)		+= spi-sh-sci.o
> >  obj-$(CONFIG_SPI_SIRF)		+= spi-sirf.o
> > +obj-$(CONFIG_ARCH_SUNXI)		+= spi-sun6i.o
> 
> I would expect a new Kconfig symbol for this.

Hmmm, yeah, sorry, some hacky leftover.

> > +static inline void sun6i_spi_drain_fifo(struct sun6i_spi *sspi, int len)
> > +{
> > +	u32 reg, cnt;
> > +	u8 byte;
> > +
> > +	/* See how much data are available */
> 
> data is available.
> 
> > +	while (len--) {
> > +		byte = readb(sspi->base_addr + SUN6I_RXDATA_REG);
> > +		if (sspi->rx_buf)
> > +			*sspi->rx_buf++ = byte;
> > +	}
> 
> It seems like this hardware is only able to handle bidirectional
> operation - this is actually quite common and isn't always as simple as
> it is here.  Can I persuade you to put something in the core which
> provides dummy data buffers for this case?  I was thinking flags like
> must_tx and must_rx or something but didn't get around to this yet.

I'm pretty sure It can support unidirectionnal operations as well. I
just didn't found out how yet. There's actually three counters to set
whenever I setup the transfer, two of them seem to be to set the
number of bytes to send, and the last one the overall number of bursts
to set on the clock line, so I guess that we can either set it only in
RX (with the first two to 0, the last one to spi_transfer->len), only
in TX or both (by programming all three to spi_transfer->len).

> > +static int sun6i_spi_finish_transfer(struct spi_device *spi,
> > +				     struct spi_transfer *tfr,
> > +				     bool cs_change)
> > +{
> > +	struct sun6i_spi *sspi = spi_master_get_devdata(spi->master);
> > +
> > +	sun6i_spi_drain_fifo(sspi, SUN6I_FIFO_DEPTH);
> > +
> > +	if (tfr->delay_usecs)
> > +		udelay(tfr->delay_usecs);
> 
> If you implement this using transfer_one() (as you should) the core will
> do this for you.

Oh, nice. I overlooked it.

> > +	if (status & SUN6I_INT_CTL_RF_OVF) {
> > +		sun6i_spi_drain_fifo(sspi, SUN6I_FIFO_DEPTH);
> > +		sun6i_spi_write(sspi, SUN6I_INT_STA_REG, SUN6I_INT_CTL_RF_OVF);
> > +		return IRQ_HANDLED;
> > +	}
> 
> This looks like an overflow - a log message would be helpful for users
> and you should possibly be flagging an error on the current transfer.

Hmmm, that was an attempt at receiving more bytes than the FIFO can
handle, but I guess the FIFO full interrupt would be more appropriate
for this.

> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	sspi->base_addr = devm_request_and_ioremap(&pdev->dev, res);
> > +	if (!sspi->base_addr) {
> > +		dev_err(&pdev->dev, "Unable to remap IO\n");
> > +		ret = -ENXIO;
> > +		goto err;
> > +	}
> 
> devm_ioremap_resource() is nicer in that it returns an error and then
> you don't need to log either since it's noisy itself.

Ack.

> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq < 0) {
> > +		dev_err(&pdev->dev, "No spi IRQ specified\n");
> > +		ret = -ENXIO;
> 
> Don't overwrite the error code.

Ack.

> > +	ret = clk_set_rate(sspi->mclk, 100000000);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Couldn't change module clock rate\n");
> > +		goto err2;
> > +	}
> 
> Does this really need to be fatal (or done at all)?  There seems to be
> another reasonably flexible divider in the IP and it's more common to
> either set this per transfer to something that rounds nicely or just use
> the default and rely on the dividers.

The default parent of the module clock runs at 24MHz, that means that
we won't be able to reach a spi clock higher than 12MHz, which seems
quite low. We can always change the rate in the transfer setup code
though, if needs be.

> > +	ret = clk_prepare_enable(sspi->mclk);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Couldn't enable clock 'ahb spi'\n");
> > +		goto err2;
> > +	}
> 
> I would recommend moving these to runtime PM so the clocks are only
> active when the device is actually in use, the core will do the runtime
> PM management if you set auto_runtime_pm so it's really easy to
> implement.

Ok, nice.


> > +	ret = reset_control_deassert(sspi->rstc);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Couldn't deassert the device from reset\n");
> > +		goto err3;
> > +	}
> > +
> > +	sun6i_spi_write(sspi, SUN6I_GBL_CTL_REG,
> > +			SUN6I_GBL_CTL_BUS_ENABLE | SUN6I_GBL_CTL_MASTER | SUN6I_GBL_CTL_TP);
> 
> Similarly here the IP could be kept in reset when not in use.

Ok.

Thanks a lot!
Maxime


-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/4] spi: sunxi: Add Allwinner A31 SPI controller driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1717 bytes --]

On Thu, Jan 16, 2014 at 10:12:01PM +0100, Maxime Ripard wrote:
> On Thu, Jan 16, 2014 at 07:40:03PM +0000, Mark Brown wrote:

> > > +	if (status & SUN6I_INT_CTL_RF_OVF) {
> > > +		sun6i_spi_drain_fifo(sspi, SUN6I_FIFO_DEPTH);
> > > +		sun6i_spi_write(sspi, SUN6I_INT_STA_REG, SUN6I_INT_CTL_RF_OVF);
> > > +		return IRQ_HANDLED;
> > > +	}

> > This looks like an overflow - a log message would be helpful for users
> > and you should possibly be flagging an error on the current transfer.

> Hmmm, that was an attempt at receiving more bytes than the FIFO can
> handle, but I guess the FIFO full interrupt would be more appropriate
> for this.

If you've got an overflow interrupt that suggests that the data is
already corrupted, assuming the interrupt isn't misnamed.

> > > +	ret = clk_set_rate(sspi->mclk, 100000000);
> > > +	if (ret) {
> > > +		dev_err(&pdev->dev, "Couldn't change module clock rate\n");
> > > +		goto err2;
> > > +	}

> > Does this really need to be fatal (or done at all)?  There seems to be
> > another reasonably flexible divider in the IP and it's more common to
> > either set this per transfer to something that rounds nicely or just use
> > the default and rely on the dividers.

> The default parent of the module clock runs at 24MHz, that means that
> we won't be able to reach a spi clock higher than 12MHz, which seems
> quite low. We can always change the rate in the transfer setup code
> though, if needs be.

12MHz is actually quite a common limit for SPI interfaces (half of a
24MHz master clock like the IP itself has) but yeah, you want to go
higher if you can.  Doing it on transfer setup is going to mean that
you save a little power when you don't need the extra speed too.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 4/4] ARM: sun6i: dt: Add SPI controllers to the A31 DTSI

[Maxime Ripard]

The A31 has 4 SPI controllers. Add them in the DTSI.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 arch/arm/boot/dts/sun6i-a31.dtsi | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
index ae058eb..288cc8e 100644
--- a/arch/arm/boot/dts/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/sun6i-a31.dtsi
@@ -340,6 +340,46 @@
 			status = "disabled";
 		};
 
+		spi0: spi@01c68000 {
+			compatible = "allwinner,sun6i-a31-spi";
+			reg = <0x01c68000 0x1000>;
+			interrupts = <0 65 4>;
+			clocks = <&ahb1_gates 20>, <&spi0_clk>;
+			clock-names = "ahb", "mod";
+			resets = <&ahb1_rst 20>;
+			status = "disabled";
+		};
+
+		spi1: spi@01c69000 {
+			compatible = "allwinner,sun6i-a31-spi";
+			reg = <0x01c69000 0x1000>;
+			interrupts = <0 66 4>;
+			clocks = <&ahb1_gates 21>, <&spi1_clk>;
+			clock-names = "ahb", "mod";
+			resets = <&ahb1_rst 21>;
+			status = "disabled";
+		};
+
+		spi2: spi@01c6a000 {
+			compatible = "allwinner,sun6i-a31-spi";
+			reg = <0x01c6a000 0x1000>;
+			interrupts = <0 67 4>;
+			clocks = <&ahb1_gates 22>, <&spi2_clk>;
+			clock-names = "ahb", "mod";
+			resets = <&ahb1_rst 22>;
+			status = "disabled";
+		};
+
+		spi3: spi@01c6b000 {
+			compatible = "allwinner,sun6i-a31-spi";
+			reg = <0x01c6b000 0x1000>;
+			interrupts = <0 68 4>;
+			clocks = <&ahb1_gates 23>, <&spi3_clk>;
+			clock-names = "ahb", "mod";
+			resets = <&ahb1_rst 23>;
+			status = "disabled";
+		};
+
 		gic: interrupt-controller@01c81000 {
 			compatible = "arm,cortex-a7-gic", "arm,cortex-a15-gic";
 			reg = <0x01c81000 0x1000>,
-- 
1.8.4.2