[PATCH v2 linux-next 0/3] i.MX ECSPI controller slave mode support

[PATCH v2 linux-next 0/3] i.MX ECSPI controller slave mode support

[jiada_wang-nmGgyN9QBj3QT0dZR+AlfA]

From: Jiada Wang <jiada_wang-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>


Changes in v2:
re-workd i.MX ECSPI controller slave mode support based on Geert's work

Jiada Wang (3):
  spi: imx: add selection for iMX53 and iMX6 controller
  ARM: dts: imx: change compatiblity for SPI controllers on imx53 later
    soc
  spi: imx: Add support for SPI Slave mode

 .../devicetree/bindings/spi/fsl-imx-cspi.txt       |   1 +
 arch/arm/boot/dts/imx53.dtsi                       |   4 +-
 arch/arm/boot/dts/imx6q.dtsi                       |   2 +-
 arch/arm/boot/dts/imx6qdl.dtsi                     |   8 +-
 arch/arm/boot/dts/imx6sl.dtsi                      |   8 +-
 arch/arm/boot/dts/imx6sx.dtsi                      |   8 +-
 arch/arm/boot/dts/imx6ul.dtsi                      |   8 +-
 drivers/spi/spi-imx.c                              | 273 ++++++++++++++++++---
 8 files changed, 257 insertions(+), 55 deletions(-)

-- 
2.7.4


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 linux-next 1/3] spi: imx: add selection for iMX53 and iMX6 controller

[jiada_wang]

From: Jiada Wang <jiada_wang@mentor.com>

ECSPI contorller for iMX53 and iMX6 has few hardware issues
comparing to iMX51.
The change add possibility to detect which controller is used
to apply possible workaround and limitations.

Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
---
 .../devicetree/bindings/spi/fsl-imx-cspi.txt       |  1 +
 drivers/spi/spi-imx.c                              | 46 ++++++++++++++++++++--
 2 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt b/Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt
index 31b5b21..5bf1396 100644
--- a/Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt
+++ b/Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt
@@ -9,6 +9,7 @@ Required properties:
   - "fsl,imx31-cspi" for SPI compatible with the one integrated on i.MX31
   - "fsl,imx35-cspi" for SPI compatible with the one integrated on i.MX35
   - "fsl,imx51-ecspi" for SPI compatible with the one integrated on i.MX51
+  - "fsl,imx53-ecspi" for SPI compatible with the one integrated on i.MX53 and later Soc
 - reg : Offset and length of the register set for the device
 - interrupts : Should contain CSPI/eCSPI interrupt
 - cs-gpios : Specifies the gpio pins to be used for chipselects.
diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index b402530..765856c 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -67,7 +67,8 @@ enum spi_imx_devtype {
 	IMX27_CSPI,
 	IMX31_CSPI,
 	IMX35_CSPI,	/* CSPI on all i.mx except above */
-	IMX51_ECSPI,	/* ECSPI on i.mx51 and later */
+	IMX51_ECSPI,	/* ECSPI on i.mx51 */
+	IMX53_ECSPI,	/* ECSPI on i.mx53 and later */
 };
 
 struct spi_imx_data;
@@ -128,9 +129,32 @@ static inline int is_imx51_ecspi(struct spi_imx_data *d)
 	return d->devtype_data->devtype == IMX51_ECSPI;
 }
 
+static inline int is_imx53_ecspi(struct spi_imx_data *d)
+{
+	return d->devtype_data->devtype == IMX53_ECSPI;
+}
+
 static inline unsigned spi_imx_get_fifosize(struct spi_imx_data *d)
 {
-	return is_imx51_ecspi(d) ? 64 : 8;
+	switch (d->devtype_data->devtype) {
+	case IMX51_ECSPI:
+	case IMX53_ECSPI:
+		return 64;
+	default:
+		return 8;
+	}
+}
+
+static inline bool spi_imx_has_dmamode(struct spi_imx_data *d)
+{
+	switch (d->devtype_data->devtype) {
+	case IMX35_CSPI:
+	case IMX51_ECSPI:
+	case IMX53_ECSPI:
+		return true;
+	default:
+		return false;
+	}
 }
 
 #define MXC_SPI_BUF_RX(type)						\
@@ -754,6 +778,15 @@ static struct spi_imx_devtype_data imx51_ecspi_devtype_data = {
 	.devtype = IMX51_ECSPI,
 };
 
+static struct spi_imx_devtype_data imx53_ecspi_devtype_data = {
+	.intctrl = mx51_ecspi_intctrl,
+	.config = mx51_ecspi_config,
+	.trigger = mx51_ecspi_trigger,
+	.rx_available = mx51_ecspi_rx_available,
+	.reset = mx51_ecspi_reset,
+	.devtype = IMX53_ECSPI,
+};
+
 static const struct platform_device_id spi_imx_devtype[] = {
 	{
 		.name = "imx1-cspi",
@@ -774,6 +807,9 @@ static const struct platform_device_id spi_imx_devtype[] = {
 		.name = "imx51-ecspi",
 		.driver_data = (kernel_ulong_t) &imx51_ecspi_devtype_data,
 	}, {
+		.name = "imx53-ecspi",
+		.driver_data = (kernel_ulong_t) &imx53_ecspi_devtype_data,
+	}, {
 		/* sentinel */
 	}
 };
@@ -785,6 +821,7 @@ static const struct of_device_id spi_imx_dt_ids[] = {
 	{ .compatible = "fsl,imx31-cspi", .data = &imx31_cspi_devtype_data, },
 	{ .compatible = "fsl,imx35-cspi", .data = &imx35_cspi_devtype_data, },
 	{ .compatible = "fsl,imx51-ecspi", .data = &imx51_ecspi_devtype_data, },
+	{ .compatible = "fsl,imx53-ecspi", .data = &imx53_ecspi_devtype_data, },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, spi_imx_dt_ids);
@@ -1229,7 +1266,8 @@ static int spi_imx_probe(struct platform_device *pdev)
 	spi_imx->bitbang.master->prepare_message = spi_imx_prepare_message;
 	spi_imx->bitbang.master->unprepare_message = spi_imx_unprepare_message;
 	spi_imx->bitbang.master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH;
-	if (is_imx35_cspi(spi_imx) || is_imx51_ecspi(spi_imx))
+	if (is_imx35_cspi(spi_imx) || is_imx51_ecspi(spi_imx) ||
+	    is_imx53_ecspi(spi_imx))
 		spi_imx->bitbang.master->mode_bits |= SPI_LOOP | SPI_READY;
 
 	spi_imx->spi_drctl = spi_drctl;
@@ -1282,7 +1320,7 @@ static int spi_imx_probe(struct platform_device *pdev)
 	 * Only validated on i.mx35 and i.mx6 now, can remove the constraint
 	 * if validated on other chips.
 	 */
-	if (is_imx35_cspi(spi_imx) || is_imx51_ecspi(spi_imx)) {
+	if (spi_imx_has_dmamode(spi_imx)) {
 		ret = spi_imx_sdma_init(&pdev->dev, spi_imx, master);
 		if (ret == -EPROBE_DEFER)
 			goto out_clk_put;
-- 
2.7.4

Re: [PATCH v2 linux-next 1/3] spi: imx: add selection for iMX53 and iMX6 controller

[Sascha Hauer]

On Wed, May 31, 2017 at 02:19:56AM -0700, jiada_wang-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org wrote:
> From: Jiada Wang <jiada_wang-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
> 
> ECSPI contorller for iMX53 and iMX6 has few hardware issues
> comparing to iMX51.
> The change add possibility to detect which controller is used
> to apply possible workaround and limitations.
> 
> Signed-off-by: Jiada Wang <jiada_wang-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
> ---
>  .../devicetree/bindings/spi/fsl-imx-cspi.txt       |  1 +
>  drivers/spi/spi-imx.c                              | 46 ++++++++++++++++++++--

[...]

> +
>  static inline unsigned spi_imx_get_fifosize(struct spi_imx_data *d)
>  {
> -	return is_imx51_ecspi(d) ? 64 : 8;
> +	switch (d->devtype_data->devtype) {
> +	case IMX51_ECSPI:
> +	case IMX53_ECSPI:
> +		return 64;
> +	default:
> +		return 8;
> +	}
> +}
> +
> +static inline bool spi_imx_has_dmamode(struct spi_imx_data *d)
> +{
> +	switch (d->devtype_data->devtype) {
> +	case IMX35_CSPI:
> +	case IMX51_ECSPI:
> +	case IMX53_ECSPI:
> +		return true;
> +	default:
> +		return false;
> +	}
>  }

Please add the fifosize and DMA capability to struct spi_imx_devtype_data, this
struct was introduced to hold SoC specific informations.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 linux-next 2/3] ARM: dts: imx: change compatiblity for SPI controllers on imx53 later soc

[jiada_wang-nmGgyN9QBj3QT0dZR+AlfA]

From: Jiada Wang <jiada_wang-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>

for SPI controllers on imx53 and later SoCs, there is HW issue when
work in slave mode, as new device type 'IMX53_ECSPI' has been added
for these SPI controllers which is compatible with 'fsl,imx53-ecspi'.

This patch updates DTS to make imx53 later SPI controller only be
compatibile with 'fsl,imx53-ecspi'.

Signed-off-by: Jiada Wang <jiada_wang-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/imx53.dtsi   | 4 ++--
 arch/arm/boot/dts/imx6q.dtsi   | 2 +-
 arch/arm/boot/dts/imx6qdl.dtsi | 8 ++++----
 arch/arm/boot/dts/imx6sl.dtsi  | 8 ++++----
 arch/arm/boot/dts/imx6sx.dtsi  | 8 ++++----
 arch/arm/boot/dts/imx6ul.dtsi  | 8 ++++----
 6 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/arch/arm/boot/dts/imx53.dtsi b/arch/arm/boot/dts/imx53.dtsi
index 2e516f4..9eeafb9 100644
--- a/arch/arm/boot/dts/imx53.dtsi
+++ b/arch/arm/boot/dts/imx53.dtsi
@@ -243,7 +243,7 @@
 				ecspi1: ecspi@50010000 {
 					#address-cells = <1>;
 					#size-cells = <0>;
-					compatible = "fsl,imx53-ecspi", "fsl,imx51-ecspi";
+					compatible = "fsl,imx53-ecspi";
 					reg = <0x50010000 0x4000>;
 					interrupts = <36>;
 					clocks = <&clks IMX5_CLK_ECSPI1_IPG_GATE>,
@@ -662,7 +662,7 @@
 			ecspi2: ecspi@63fac000 {
 				#address-cells = <1>;
 				#size-cells = <0>;
-				compatible = "fsl,imx53-ecspi", "fsl,imx51-ecspi";
+				compatible = "fsl,imx53-ecspi";
 				reg = <0x63fac000 0x4000>;
 				interrupts = <37>;
 				clocks = <&clks IMX5_CLK_ECSPI2_IPG_GATE>,
diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index dd33849..b214442 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -90,7 +90,7 @@
 				ecspi5: ecspi@02018000 {
 					#address-cells = <1>;
 					#size-cells = <0>;
-					compatible = "fsl,imx6q-ecspi", "fsl,imx51-ecspi";
+					compatible = "fsl,imx6q-ecspi", "fsl,imx53-ecspi";
 					reg = <0x02018000 0x4000>;
 					interrupts = <0 35 IRQ_TYPE_LEVEL_HIGH>;
 					clocks = <&clks IMX6Q_CLK_ECSPI5>,
diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
index f325411..ac19c58 100644
--- a/arch/arm/boot/dts/imx6qdl.dtsi
+++ b/arch/arm/boot/dts/imx6qdl.dtsi
@@ -266,7 +266,7 @@
 				ecspi1: ecspi@02008000 {
 					#address-cells = <1>;
 					#size-cells = <0>;
-					compatible = "fsl,imx6q-ecspi", "fsl,imx51-ecspi";
+					compatible = "fsl,imx6q-ecspi", "fsl,imx53-ecspi";
 					reg = <0x02008000 0x4000>;
 					interrupts = <0 31 IRQ_TYPE_LEVEL_HIGH>;
 					clocks = <&clks IMX6QDL_CLK_ECSPI1>,
@@ -280,7 +280,7 @@
 				ecspi2: ecspi@0200c000 {
 					#address-cells = <1>;
 					#size-cells = <0>;
-					compatible = "fsl,imx6q-ecspi", "fsl,imx51-ecspi";
+					compatible = "fsl,imx6q-ecspi", "fsl,imx53-ecspi";
 					reg = <0x0200c000 0x4000>;
 					interrupts = <0 32 IRQ_TYPE_LEVEL_HIGH>;
 					clocks = <&clks IMX6QDL_CLK_ECSPI2>,
@@ -294,7 +294,7 @@
 				ecspi3: ecspi@02010000 {
 					#address-cells = <1>;
 					#size-cells = <0>;
-					compatible = "fsl,imx6q-ecspi", "fsl,imx51-ecspi";
+					compatible = "fsl,imx6q-ecspi", "fsl,imx53-ecspi";
 					reg = <0x02010000 0x4000>;
 					interrupts = <0 33 IRQ_TYPE_LEVEL_HIGH>;
 					clocks = <&clks IMX6QDL_CLK_ECSPI3>,
@@ -308,7 +308,7 @@
 				ecspi4: ecspi@02014000 {
 					#address-cells = <1>;
 					#size-cells = <0>;
-					compatible = "fsl,imx6q-ecspi", "fsl,imx51-ecspi";
+					compatible = "fsl,imx6q-ecspi", "fsl,imx53-ecspi";
 					reg = <0x02014000 0x4000>;
 					interrupts = <0 34 IRQ_TYPE_LEVEL_HIGH>;
 					clocks = <&clks IMX6QDL_CLK_ECSPI4>,
diff --git a/arch/arm/boot/dts/imx6sl.dtsi b/arch/arm/boot/dts/imx6sl.dtsi
index 3243af4..d9b9053 100644
--- a/arch/arm/boot/dts/imx6sl.dtsi
+++ b/arch/arm/boot/dts/imx6sl.dtsi
@@ -168,7 +168,7 @@
 				ecspi1: ecspi@02008000 {
 					#address-cells = <1>;
 					#size-cells = <0>;
-					compatible = "fsl,imx6sl-ecspi", "fsl,imx51-ecspi";
+					compatible = "fsl,imx6sl-ecspi", "fsl,imx53-ecspi";
 					reg = <0x02008000 0x4000>;
 					interrupts = <0 31 IRQ_TYPE_LEVEL_HIGH>;
 					clocks = <&clks IMX6SL_CLK_ECSPI1>,
@@ -180,7 +180,7 @@
 				ecspi2: ecspi@0200c000 {
 					#address-cells = <1>;
 					#size-cells = <0>;
-					compatible = "fsl,imx6sl-ecspi", "fsl,imx51-ecspi";
+					compatible = "fsl,imx6sl-ecspi", "fsl,imx53-ecspi";
 					reg = <0x0200c000 0x4000>;
 					interrupts = <0 32 IRQ_TYPE_LEVEL_HIGH>;
 					clocks = <&clks IMX6SL_CLK_ECSPI2>,
@@ -192,7 +192,7 @@
 				ecspi3: ecspi@02010000 {
 					#address-cells = <1>;
 					#size-cells = <0>;
-					compatible = "fsl,imx6sl-ecspi", "fsl,imx51-ecspi";
+					compatible = "fsl,imx6sl-ecspi", "fsl,imx53-ecspi";
 					reg = <0x02010000 0x4000>;
 					interrupts = <0 33 IRQ_TYPE_LEVEL_HIGH>;
 					clocks = <&clks IMX6SL_CLK_ECSPI3>,
@@ -204,7 +204,7 @@
 				ecspi4: ecspi@02014000 {
 					#address-cells = <1>;
 					#size-cells = <0>;
-					compatible = "fsl,imx6sl-ecspi", "fsl,imx51-ecspi";
+					compatible = "fsl,imx6sl-ecspi", "fsl,imx53-ecspi";
 					reg = <0x02014000 0x4000>;
 					interrupts = <0 34 IRQ_TYPE_LEVEL_HIGH>;
 					clocks = <&clks IMX6SL_CLK_ECSPI4>,
diff --git a/arch/arm/boot/dts/imx6sx.dtsi b/arch/arm/boot/dts/imx6sx.dtsi
index f16b9df..149ef79 100644
--- a/arch/arm/boot/dts/imx6sx.dtsi
+++ b/arch/arm/boot/dts/imx6sx.dtsi
@@ -251,7 +251,7 @@
 				ecspi1: ecspi@02008000 {
 					#address-cells = <1>;
 					#size-cells = <0>;
-					compatible = "fsl,imx6sx-ecspi", "fsl,imx51-ecspi";
+					compatible = "fsl,imx6sx-ecspi", "fsl,imx53-ecspi";
 					reg = <0x02008000 0x4000>;
 					interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
 					clocks = <&clks IMX6SX_CLK_ECSPI1>,
@@ -263,7 +263,7 @@
 				ecspi2: ecspi@0200c000 {
 					#address-cells = <1>;
 					#size-cells = <0>;
-					compatible = "fsl,imx6sx-ecspi", "fsl,imx51-ecspi";
+					compatible = "fsl,imx6sx-ecspi", "fsl,imx53-ecspi";
 					reg = <0x0200c000 0x4000>;
 					interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
 					clocks = <&clks IMX6SX_CLK_ECSPI2>,
@@ -275,7 +275,7 @@
 				ecspi3: ecspi@02010000 {
 					#address-cells = <1>;
 					#size-cells = <0>;
-					compatible = "fsl,imx6sx-ecspi", "fsl,imx51-ecspi";
+					compatible = "fsl,imx6sx-ecspi", "fsl,imx53-ecspi";
 					reg = <0x02010000 0x4000>;
 					interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>;
 					clocks = <&clks IMX6SX_CLK_ECSPI3>,
@@ -287,7 +287,7 @@
 				ecspi4: ecspi@02014000 {
 					#address-cells = <1>;
 					#size-cells = <0>;
-					compatible = "fsl,imx6sx-ecspi", "fsl,imx51-ecspi";
+					compatible = "fsl,imx6sx-ecspi", "fsl,imx53-ecspi";
 					reg = <0x02014000 0x4000>;
 					interrupts = <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>;
 					clocks = <&clks IMX6SX_CLK_ECSPI4>,
diff --git a/arch/arm/boot/dts/imx6ul.dtsi b/arch/arm/boot/dts/imx6ul.dtsi
index 6da2b77..7226061 100644
--- a/arch/arm/boot/dts/imx6ul.dtsi
+++ b/arch/arm/boot/dts/imx6ul.dtsi
@@ -204,7 +204,7 @@
 				ecspi1: ecspi@02008000 {
 					#address-cells = <1>;
 					#size-cells = <0>;
-					compatible = "fsl,imx6ul-ecspi", "fsl,imx51-ecspi";
+					compatible = "fsl,imx6ul-ecspi", "fsl,imx53-ecspi";
 					reg = <0x02008000 0x4000>;
 					interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
 					clocks = <&clks IMX6UL_CLK_ECSPI1>,
@@ -216,7 +216,7 @@
 				ecspi2: ecspi@0200c000 {
 					#address-cells = <1>;
 					#size-cells = <0>;
-					compatible = "fsl,imx6ul-ecspi", "fsl,imx51-ecspi";
+					compatible = "fsl,imx6ul-ecspi", "fsl,imx53-ecspi";
 					reg = <0x0200c000 0x4000>;
 					interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
 					clocks = <&clks IMX6UL_CLK_ECSPI2>,
@@ -228,7 +228,7 @@
 				ecspi3: ecspi@02010000 {
 					#address-cells = <1>;
 					#size-cells = <0>;
-					compatible = "fsl,imx6ul-ecspi", "fsl,imx51-ecspi";
+					compatible = "fsl,imx6ul-ecspi", "fsl,imx53-ecspi";
 					reg = <0x02010000 0x4000>;
 					interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>;
 					clocks = <&clks IMX6UL_CLK_ECSPI3>,
@@ -240,7 +240,7 @@
 				ecspi4: ecspi@02014000 {
 					#address-cells = <1>;
 					#size-cells = <0>;
-					compatible = "fsl,imx6ul-ecspi", "fsl,imx51-ecspi";
+					compatible = "fsl,imx6ul-ecspi", "fsl,imx53-ecspi";
 					reg = <0x02014000 0x4000>;
 					interrupts = <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>;
 					clocks = <&clks IMX6UL_CLK_ECSPI4>,
-- 
2.7.4


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 linux-next 3/3] spi: imx: Add support for SPI Slave mode

[jiada_wang]

From: Jiada Wang <jiada_wang@mentor.com>

Previously i.MX SPI controller only works in Master mode.
This patch adds support to i.MX51, i.MX53 and i.MX6 ECSPI
controller to work also in Slave mode.

Currently SPI Slave mode support patch has the following limitations:
1. The stale data in RXFIFO will be dropped when the Slave does any new
   transfer.
2. One transfer can be finished only after all transfer->len data been
   transferred to master device
3. Slave device only accepts transfer->len data. Any data longer than this
   from master device will be dropped. Any data shorter than this from
   master will cause SPI to stuck due to mentioned HW limitation 2.
4. Only PIO transfer is supported in Slave mode.

Following HW limitation applies:
1.  ECSPI has a HW issue when works in Slave mode, after 64
    words written to TXFIFO, even TXFIFO becomes empty,
    ECSPI_TXDATA keeps shift out the last word data,
    so we have to disable ECSPI when in slave mode after the
    transfer completes
2.  Due to Freescale errata ERR003775 "eCSPI: Burst completion by Chip
    Select (SS) signal in Slave mode is not functional" burst size must
    be set exactly to the size of the transfer. This limit SPI transaction
    with maximum 2^12 bits. This errata affects i.MX53 and i.MX6 ECSPI
    controllers.

Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
---
 drivers/spi/spi-imx.c | 227 +++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 195 insertions(+), 32 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 765856c..a227a30 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -53,9 +53,13 @@
 /* generic defines to abstract from the different register layouts */
 #define MXC_INT_RR	(1 << 0) /* Receive data ready interrupt */
 #define MXC_INT_TE	(1 << 1) /* Transmit FIFO empty interrupt */
+#define MXC_INT_RDR	BIT(4) /* Receive date threshold interrupt */
 
 /* The maximum  bytes that a sdma BD can transfer.*/
 #define MAX_SDMA_BD_BYTES  (1 << 15)
+/* The maximum bytes that IMX53_ECSPI can transfer in slave mode.*/
+#define MX53_MAX_TRANSFER_BYTES		512
+
 struct spi_imx_config {
 	unsigned int speed_hz;
 	unsigned int bpw;
@@ -79,6 +83,7 @@ struct spi_imx_devtype_data {
 	void (*trigger)(struct spi_imx_data *);
 	int (*rx_available)(struct spi_imx_data *);
 	void (*reset)(struct spi_imx_data *);
+	void (*disable)(struct spi_imx_data *);
 	enum spi_imx_devtype devtype;
 };
 
@@ -105,6 +110,11 @@ struct spi_imx_data {
 	const void *tx_buf;
 	unsigned int txfifo; /* number of words pushed in tx FIFO */
 
+	/* Slave mode */
+	bool slave_mode;
+	bool slave_aborted;
+	unsigned int slave_burst;
+
 	/* DMA */
 	bool usedma;
 	u32 wml;
@@ -157,6 +167,17 @@ static inline bool spi_imx_has_dmamode(struct spi_imx_data *d)
 	}
 }
 
+static inline bool spi_imx_has_slavemode(const struct spi_imx_devtype_data *d)
+{
+	switch (d->devtype) {
+	case IMX51_ECSPI:
+	case IMX53_ECSPI:
+		return true;
+	default:
+		return false;
+	}
+}
+
 #define MXC_SPI_BUF_RX(type)						\
 static void spi_imx_buf_rx_##type(struct spi_imx_data *spi_imx)		\
 {									\
@@ -287,6 +308,7 @@ static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi,
 #define MX51_ECSPI_INT		0x10
 #define MX51_ECSPI_INT_TEEN		(1 <<  0)
 #define MX51_ECSPI_INT_RREN		(1 <<  3)
+#define MX51_ECSPI_INT_RDREN		(1 <<  4)
 
 #define MX51_ECSPI_DMA      0x14
 #define MX51_ECSPI_DMA_TX_WML(wml)	((wml) & 0x3f)
@@ -303,6 +325,51 @@ static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi,
 #define MX51_ECSPI_TESTREG	0x20
 #define MX51_ECSPI_TESTREG_LBC	BIT(31)
 
+static void mx53_ecspi_rx_slave(struct spi_imx_data *spi_imx)
+{
+	u32 val = be32_to_cpu(readl(spi_imx->base + MXC_CSPIRXDATA));
+
+	if (spi_imx->rx_buf) {
+		int shift = spi_imx->slave_burst % sizeof(val);
+
+		if (shift) {
+			memcpy(spi_imx->rx_buf,
+			       ((u8 *)&val) + sizeof(val) - shift, shift);
+		} else {
+			*((u32 *)spi_imx->rx_buf) = val;
+			shift = sizeof(val);
+		}
+
+		spi_imx->rx_buf += shift;
+		spi_imx->slave_burst -= shift;
+	}
+}
+
+static void mx53_ecspi_tx_slave(struct spi_imx_data *spi_imx)
+{
+	u32 val = 0;
+	int shift = spi_imx->count % sizeof(val);
+
+	if (spi_imx->tx_buf) {
+		if (shift) {
+			memcpy(((u8 *)&val) + sizeof(val) - shift,
+			       spi_imx->tx_buf, shift);
+		} else {
+			val = *((u32 *)spi_imx->tx_buf);
+			shift = sizeof(val);
+		}
+		val = cpu_to_be32(val);
+		spi_imx->tx_buf += shift;
+	}
+
+	if (!shift)
+		shift = sizeof(val);
+
+	spi_imx->count -= shift;
+
+	writel(val, spi_imx->base + MXC_CSPITXDATA);
+}
+
 /* MX51 eCSPI */
 static unsigned int mx51_ecspi_clkdiv(struct spi_imx_data *spi_imx,
 				      unsigned int fspi, unsigned int *fres)
@@ -352,6 +419,9 @@ static void mx51_ecspi_intctrl(struct spi_imx_data *spi_imx, int enable)
 	if (enable & MXC_INT_RR)
 		val |= MX51_ECSPI_INT_RREN;
 
+	if (enable & MXC_INT_RDR)
+		val |= MX51_ECSPI_INT_RDREN;
+
 	writel(val, spi_imx->base + MX51_ECSPI_INT);
 }
 
@@ -364,6 +434,15 @@ static void mx51_ecspi_trigger(struct spi_imx_data *spi_imx)
 	writel(reg, spi_imx->base + MX51_ECSPI_CTRL);
 }
 
+static void __maybe_unused mx51_ecspi_disable(struct spi_imx_data *spi_imx)
+{
+	u32 ctrl;
+
+	ctrl = readl(spi_imx->base + MX51_ECSPI_CTRL);
+	ctrl &= ~MX51_ECSPI_CTRL_ENABLE;
+	writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
+}
+
 static int mx51_ecspi_config(struct spi_device *spi,
 			     struct spi_imx_config *config)
 {
@@ -372,14 +451,11 @@ static int mx51_ecspi_config(struct spi_device *spi,
 	u32 clk = config->speed_hz, delay, reg;
 	u32 cfg = readl(spi_imx->base + MX51_ECSPI_CONFIG);
 
-	/*
-	 * The hardware seems to have a race condition when changing modes. The
-	 * current assumption is that the selection of the channel arrives
-	 * earlier in the hardware than the mode bits when they are written at
-	 * the same time.
-	 * So set master mode for all channels as we do not support slave mode.
-	 */
-	ctrl |= MX51_ECSPI_CTRL_MODE_MASK;
+	/* set Master or Slave mode */
+	if (spi_imx->slave_mode)
+		ctrl &= ~MX51_ECSPI_CTRL_MODE_MASK;
+	else
+		ctrl |= MX51_ECSPI_CTRL_MODE_MASK;
 
 	/*
 	 * Enable SPI_RDY handling (falling edge/level triggered).
@@ -394,9 +470,21 @@ static int mx51_ecspi_config(struct spi_device *spi,
 	/* set chip select to use */
 	ctrl |= MX51_ECSPI_CTRL_CS(spi->chip_select);
 
-	ctrl |= (config->bpw - 1) << MX51_ECSPI_CTRL_BL_OFFSET;
+	if (spi_imx->slave_mode && is_imx53_ecspi(spi_imx))
+		ctrl |= (spi_imx->slave_burst * 8 - 1)
+			<< MX51_ECSPI_CTRL_BL_OFFSET;
+	else
+		ctrl |= (config->bpw - 1) << MX51_ECSPI_CTRL_BL_OFFSET;
 
-	cfg |= MX51_ECSPI_CONFIG_SBBCTRL(spi->chip_select);
+	/*
+	 * eCSPI burst completion by Chip Select signal in Slave mode
+	 * is not functional for imx53 Soc, config SPI burst completed when
+	 * BURST_LENGTH + 1 bits are received
+	 */
+	if (spi_imx->slave_mode && is_imx53_ecspi(spi_imx))
+		cfg &= ~MX51_ECSPI_CONFIG_SBBCTRL(spi->chip_select);
+	else
+		cfg |= MX51_ECSPI_CONFIG_SBBCTRL(spi->chip_select);
 
 	if (spi->mode & SPI_CPHA)
 		cfg |= MX51_ECSPI_CONFIG_SCLKPHA(spi->chip_select);
@@ -775,6 +863,7 @@ static struct spi_imx_devtype_data imx51_ecspi_devtype_data = {
 	.trigger = mx51_ecspi_trigger,
 	.rx_available = mx51_ecspi_rx_available,
 	.reset = mx51_ecspi_reset,
+	.disable = mx51_ecspi_disable,
 	.devtype = IMX51_ECSPI,
 };
 
@@ -784,6 +873,7 @@ static struct spi_imx_devtype_data imx53_ecspi_devtype_data = {
 	.trigger = mx51_ecspi_trigger,
 	.rx_available = mx51_ecspi_rx_available,
 	.reset = mx51_ecspi_reset,
+	.disable = mx51_ecspi_disable,
 	.devtype = IMX53_ECSPI,
 };
 
@@ -846,14 +936,16 @@ static void spi_imx_push(struct spi_imx_data *spi_imx)
 		spi_imx->txfifo++;
 	}
 
-	spi_imx->devtype_data->trigger(spi_imx);
+	if (!spi_imx->slave_mode)
+		spi_imx->devtype_data->trigger(spi_imx);
 }
 
 static irqreturn_t spi_imx_isr(int irq, void *dev_id)
 {
 	struct spi_imx_data *spi_imx = dev_id;
 
-	while (spi_imx->devtype_data->rx_available(spi_imx)) {
+	while (spi_imx->txfifo &&
+	       spi_imx->devtype_data->rx_available(spi_imx)) {
 		spi_imx->rx(spi_imx);
 		spi_imx->txfifo--;
 	}
@@ -952,7 +1044,8 @@ static int spi_imx_setupxfer(struct spi_device *spi,
 		spi_imx->tx = spi_imx_buf_tx_u32;
 	}
 
-	if (spi_imx_can_dma(spi_imx->bitbang.master, spi, t))
+	if (!spi_imx->slave_mode &&
+	    spi_imx_can_dma(spi_imx->bitbang.master, spi, t))
 		spi_imx->usedma = 1;
 	else
 		spi_imx->usedma = 0;
@@ -964,6 +1057,12 @@ static int spi_imx_setupxfer(struct spi_device *spi,
 			return ret;
 	}
 
+	if (is_imx53_ecspi(spi_imx) && spi_imx->slave_mode) {
+		spi_imx->rx = mx53_ecspi_rx_slave;
+		spi_imx->tx = mx53_ecspi_tx_slave;
+		spi_imx->slave_burst = t->len;
+	}
+
 	spi_imx->devtype_data->config(spi, &config);
 
 	return 0;
@@ -1125,16 +1224,50 @@ static int spi_imx_pio_transfer(struct spi_device *spi,
 	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
 	unsigned long transfer_timeout;
 	unsigned long timeout;
+	int ret = transfer->len;
+
+	if (is_imx53_ecspi(spi_imx) && spi_imx->slave_mode &&
+	    transfer->len > MX53_MAX_TRANSFER_BYTES) {
+		pr_err("Transaction too big, max size is %d bytes\n",
+		       MX53_MAX_TRANSFER_BYTES);
+		return -EMSGSIZE;
+	}
 
 	spi_imx->tx_buf = transfer->tx_buf;
 	spi_imx->rx_buf = transfer->rx_buf;
 	spi_imx->count = transfer->len;
 	spi_imx->txfifo = 0;
 
+	if (spi_imx->slave_mode)
+		spi_imx->slave_burst = spi_imx->count;
+
 	reinit_completion(&spi_imx->xfer_done);
+	spi_imx->slave_aborted = false;
 
 	spi_imx_push(spi_imx);
 
+	if (spi_imx->slave_mode) {
+		spi_imx->devtype_data->intctrl(spi_imx, MXC_INT_TE |
+							MXC_INT_RDR);
+
+		if (wait_for_completion_interruptible(&spi_imx->xfer_done) ||
+		    spi_imx->slave_aborted) {
+			dev_dbg(&spi->dev, "interrupted\n");
+			ret = -EINTR;
+		}
+
+		/* ecspi has a HW issue when works in Slave mode,
+		 * after 64 words writtern to TXFIFO, even TXFIFO becomes empty,
+		 * ECSPI_TXDATA keeps shift out the last word data,
+		 * so we have to disable ECSPI when in slave mode after the
+		 * transfer completes
+		 */
+		if (spi_imx->devtype_data->disable)
+			spi_imx->devtype_data->disable(spi_imx);
+
+		goto out;
+	}
+
 	spi_imx->devtype_data->intctrl(spi_imx, MXC_INT_TE);
 
 	transfer_timeout = spi_imx_calculate_timeout(spi_imx, transfer->len);
@@ -1147,7 +1280,8 @@ static int spi_imx_pio_transfer(struct spi_device *spi,
 		return -ETIMEDOUT;
 	}
 
-	return transfer->len;
+out:
+	return ret;
 }
 
 static int spi_imx_transfer(struct spi_device *spi,
@@ -1155,6 +1289,10 @@ static int spi_imx_transfer(struct spi_device *spi,
 {
 	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
 
+	/* flush rxfifo before transfer */
+	while (spi_imx->devtype_data->rx_available(spi_imx))
+		spi_imx->rx(spi_imx);
+
 	if (spi_imx->usedma)
 		return spi_imx_dma_transfer(spi_imx, transfer);
 	else
@@ -1208,6 +1346,16 @@ spi_imx_unprepare_message(struct spi_master *master, struct spi_message *msg)
 	return 0;
 }
 
+static int spi_imx_slave_abort(struct spi_master *master)
+{
+	struct spi_imx_data *spi_imx = spi_master_get_devdata(master);
+
+	spi_imx->slave_aborted = true;
+	complete(&spi_imx->xfer_done);
+
+	return 0;
+}
+
 static int spi_imx_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -1219,13 +1367,24 @@ static int spi_imx_probe(struct platform_device *pdev)
 	struct spi_imx_data *spi_imx;
 	struct resource *res;
 	int i, ret, irq, spi_drctl;
+	const struct spi_imx_devtype_data *devtype_data = of_id ? of_id->data :
+		(struct spi_imx_devtype_data *)pdev->id_entry->driver_data;
+	bool slave_mode;
 
 	if (!np && !mxc_platform_info) {
 		dev_err(&pdev->dev, "can't get the platform data\n");
 		return -EINVAL;
 	}
 
-	master = spi_alloc_master(&pdev->dev, sizeof(struct spi_imx_data));
+	slave_mode = spi_imx_has_slavemode(devtype_data) &&
+			of_property_read_bool(np, "spi-slave");
+	if (slave_mode)
+		master = spi_alloc_slave(&pdev->dev,
+					 sizeof(struct spi_imx_data));
+	else
+		master = spi_alloc_master(&pdev->dev,
+					  sizeof(struct spi_imx_data));
+
 	ret = of_property_read_u32(np, "fsl,spi-rdy-drctl", &spi_drctl);
 	if ((ret < 0) || (spi_drctl >= 0x3)) {
 		/* '11' is reserved */
@@ -1243,9 +1402,9 @@ static int spi_imx_probe(struct platform_device *pdev)
 	spi_imx = spi_master_get_devdata(master);
 	spi_imx->bitbang.master = master;
 	spi_imx->dev = &pdev->dev;
+	spi_imx->slave_mode = slave_mode;
 
-	spi_imx->devtype_data = of_id ? of_id->data :
-		(struct spi_imx_devtype_data *)pdev->id_entry->driver_data;
+	spi_imx->devtype_data = devtype_data;
 
 	if (mxc_platform_info) {
 		master->num_chipselect = mxc_platform_info->num_chipselect;
@@ -1265,6 +1424,7 @@ static int spi_imx_probe(struct platform_device *pdev)
 	spi_imx->bitbang.master->cleanup = spi_imx_cleanup;
 	spi_imx->bitbang.master->prepare_message = spi_imx_prepare_message;
 	spi_imx->bitbang.master->unprepare_message = spi_imx_unprepare_message;
+	spi_imx->bitbang.master->slave_abort = spi_imx_slave_abort;
 	spi_imx->bitbang.master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH;
 	if (is_imx35_cspi(spi_imx) || is_imx51_ecspi(spi_imx) ||
 	    is_imx53_ecspi(spi_imx))
@@ -1341,23 +1501,26 @@ static int spi_imx_probe(struct platform_device *pdev)
 		goto out_clk_put;
 	}
 
-	if (!master->cs_gpios) {
-		dev_err(&pdev->dev, "No CS GPIOs available\n");
-		ret = -EINVAL;
-		goto out_clk_put;
-	}
-
-	for (i = 0; i < master->num_chipselect; i++) {
-		if (!gpio_is_valid(master->cs_gpios[i]))
-			continue;
-
-		ret = devm_gpio_request(&pdev->dev, master->cs_gpios[i],
-					DRIVER_NAME);
-		if (ret) {
-			dev_err(&pdev->dev, "Can't get CS GPIO %i\n",
-				master->cs_gpios[i]);
+	if (!spi_imx->slave_mode) {
+		if (!master->cs_gpios) {
+			dev_err(&pdev->dev, "No CS GPIOs available\n");
+			ret = -EINVAL;
 			goto out_clk_put;
 		}
+
+		for (i = 0; i < master->num_chipselect; i++) {
+			if (!gpio_is_valid(master->cs_gpios[i]))
+				continue;
+
+			ret = devm_gpio_request(&pdev->dev,
+						master->cs_gpios[i],
+						DRIVER_NAME);
+			if (ret) {
+				dev_err(&pdev->dev, "Can't get CS GPIO %i\n",
+					master->cs_gpios[i]);
+				goto out_clk_put;
+			}
+		}
 	}
 
 	dev_info(&pdev->dev, "probed\n");
-- 
2.7.4

Re: [PATCH v2 linux-next 3/3] spi: imx: Add support for SPI Slave mode

[Sascha Hauer]

On Wed, May 31, 2017 at 02:19:58AM -0700, jiada_wang-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org wrote:
> From: Jiada Wang <jiada_wang-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
> 
> Previously i.MX SPI controller only works in Master mode.
> This patch adds support to i.MX51, i.MX53 and i.MX6 ECSPI
> controller to work also in Slave mode.
> 
> Currently SPI Slave mode support patch has the following limitations:
> 1. The stale data in RXFIFO will be dropped when the Slave does any new
>    transfer.
> 2. One transfer can be finished only after all transfer->len data been
>    transferred to master device
> 3. Slave device only accepts transfer->len data. Any data longer than this
>    from master device will be dropped. Any data shorter than this from
>    master will cause SPI to stuck due to mentioned HW limitation 2.
> 4. Only PIO transfer is supported in Slave mode.
> 
> Following HW limitation applies:
> 1.  ECSPI has a HW issue when works in Slave mode, after 64
>     words written to TXFIFO, even TXFIFO becomes empty,
>     ECSPI_TXDATA keeps shift out the last word data,
>     so we have to disable ECSPI when in slave mode after the
>     transfer completes
> 2.  Due to Freescale errata ERR003775 "eCSPI: Burst completion by Chip
>     Select (SS) signal in Slave mode is not functional" burst size must
>     be set exactly to the size of the transfer. This limit SPI transaction
>     with maximum 2^12 bits. This errata affects i.MX53 and i.MX6 ECSPI
>     controllers.
> 
> Signed-off-by: Jiada Wang <jiada_wang-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/spi/spi-imx.c | 227 +++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 195 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index 765856c..a227a30 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -53,9 +53,13 @@
>  /* generic defines to abstract from the different register layouts */
>  #define MXC_INT_RR	(1 << 0) /* Receive data ready interrupt */
>  #define MXC_INT_TE	(1 << 1) /* Transmit FIFO empty interrupt */
> +#define MXC_INT_RDR	BIT(4) /* Receive date threshold interrupt */
>  
>  /* The maximum  bytes that a sdma BD can transfer.*/
>  #define MAX_SDMA_BD_BYTES  (1 << 15)
> +/* The maximum bytes that IMX53_ECSPI can transfer in slave mode.*/
> +#define MX53_MAX_TRANSFER_BYTES		512
> +
>  struct spi_imx_config {
>  	unsigned int speed_hz;
>  	unsigned int bpw;
> @@ -79,6 +83,7 @@ struct spi_imx_devtype_data {
>  	void (*trigger)(struct spi_imx_data *);
>  	int (*rx_available)(struct spi_imx_data *);
>  	void (*reset)(struct spi_imx_data *);
> +	void (*disable)(struct spi_imx_data *);
>  	enum spi_imx_devtype devtype;
>  };
>  
> @@ -105,6 +110,11 @@ struct spi_imx_data {
>  	const void *tx_buf;
>  	unsigned int txfifo; /* number of words pushed in tx FIFO */
>  
> +	/* Slave mode */
> +	bool slave_mode;
> +	bool slave_aborted;
> +	unsigned int slave_burst;
> +
>  	/* DMA */
>  	bool usedma;
>  	u32 wml;
> @@ -157,6 +167,17 @@ static inline bool spi_imx_has_dmamode(struct spi_imx_data *d)
>  	}
>  }
>  
> +static inline bool spi_imx_has_slavemode(const struct spi_imx_devtype_data *d)
> +{
> +	switch (d->devtype) {
> +	case IMX51_ECSPI:
> +	case IMX53_ECSPI:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}

Add a new boolean flag to struct spi_imx_devtype_data instead.


> +
>  #define MXC_SPI_BUF_RX(type)						\
>  static void spi_imx_buf_rx_##type(struct spi_imx_data *spi_imx)		\
>  {									\
> @@ -287,6 +308,7 @@ static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi,
>  #define MX51_ECSPI_INT		0x10
>  #define MX51_ECSPI_INT_TEEN		(1 <<  0)
>  #define MX51_ECSPI_INT_RREN		(1 <<  3)
> +#define MX51_ECSPI_INT_RDREN		(1 <<  4)
>  
>  #define MX51_ECSPI_DMA      0x14
>  #define MX51_ECSPI_DMA_TX_WML(wml)	((wml) & 0x3f)
> @@ -303,6 +325,51 @@ static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi,
>  #define MX51_ECSPI_TESTREG	0x20
>  #define MX51_ECSPI_TESTREG_LBC	BIT(31)
>  
> +static void mx53_ecspi_rx_slave(struct spi_imx_data *spi_imx)
> +{
> +	u32 val = be32_to_cpu(readl(spi_imx->base + MXC_CSPIRXDATA));
> +
> +	if (spi_imx->rx_buf) {
> +		int shift = spi_imx->slave_burst % sizeof(val);
> +
> +		if (shift) {
> +			memcpy(spi_imx->rx_buf,
> +			       ((u8 *)&val) + sizeof(val) - shift, shift);
> +		} else {
> +			*((u32 *)spi_imx->rx_buf) = val;
> +			shift = sizeof(val);
> +		}
> +
> +		spi_imx->rx_buf += shift;
> +		spi_imx->slave_burst -= shift;
> +	}
> +}
> +
> +static void mx53_ecspi_tx_slave(struct spi_imx_data *spi_imx)
> +{
> +	u32 val = 0;
> +	int shift = spi_imx->count % sizeof(val);
> +
> +	if (spi_imx->tx_buf) {
> +		if (shift) {
> +			memcpy(((u8 *)&val) + sizeof(val) - shift,
> +			       spi_imx->tx_buf, shift);
> +		} else {
> +			val = *((u32 *)spi_imx->tx_buf);
> +			shift = sizeof(val);
> +		}
> +		val = cpu_to_be32(val);
> +		spi_imx->tx_buf += shift;
> +	}
> +
> +	if (!shift)
> +		shift = sizeof(val);

A better name for 'shift' is n_bytes. Its value should be calculated
before the if (spi_imx->tx_buf) block. Then you can use memcpy for the
four byte case aswell.


But anyway, have you tested this function for anything with
spi_imx->count % sizeof(val) != 0? In this case you have to put the fill
the FIFO only partly somewhere. I would assume you have to do this at
the end of the transfer, but you do this at the beginning. Can this
work?

> +
> +	spi_imx->count -= shift;
> +
> +	writel(val, spi_imx->base + MXC_CSPITXDATA);
> +}
> +
>  /* MX51 eCSPI */
>  static unsigned int mx51_ecspi_clkdiv(struct spi_imx_data *spi_imx,
>  				      unsigned int fspi, unsigned int *fres)
> @@ -352,6 +419,9 @@ static void mx51_ecspi_intctrl(struct spi_imx_data *spi_imx, int enable)
>  	if (enable & MXC_INT_RR)
>  		val |= MX51_ECSPI_INT_RREN;
>  
> +	if (enable & MXC_INT_RDR)
> +		val |= MX51_ECSPI_INT_RDREN;
> +
>  	writel(val, spi_imx->base + MX51_ECSPI_INT);
>  }
>  
> @@ -364,6 +434,15 @@ static void mx51_ecspi_trigger(struct spi_imx_data *spi_imx)
>  	writel(reg, spi_imx->base + MX51_ECSPI_CTRL);
>  }
>  
> +static void __maybe_unused mx51_ecspi_disable(struct spi_imx_data *spi_imx)

I don't see how this function may end up unused.

> +{
> +	u32 ctrl;
> +
> +	ctrl = readl(spi_imx->base + MX51_ECSPI_CTRL);
> +	ctrl &= ~MX51_ECSPI_CTRL_ENABLE;
> +	writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
> +}
> +
>  static int mx51_ecspi_config(struct spi_device *spi,
>  			     struct spi_imx_config *config)
>  {
> @@ -372,14 +451,11 @@ static int mx51_ecspi_config(struct spi_device *spi,
>  	u32 clk = config->speed_hz, delay, reg;
>  	u32 cfg = readl(spi_imx->base + MX51_ECSPI_CONFIG);
>  
> -	/*
> -	 * The hardware seems to have a race condition when changing modes. The
> -	 * current assumption is that the selection of the channel arrives
> -	 * earlier in the hardware than the mode bits when they are written at
> -	 * the same time.
> -	 * So set master mode for all channels as we do not support slave mode.
> -	 */
> -	ctrl |= MX51_ECSPI_CTRL_MODE_MASK;
> +	/* set Master or Slave mode */
> +	if (spi_imx->slave_mode)
> +		ctrl &= ~MX51_ECSPI_CTRL_MODE_MASK;
> +	else
> +		ctrl |= MX51_ECSPI_CTRL_MODE_MASK;
>  
>  	/*
>  	 * Enable SPI_RDY handling (falling edge/level triggered).
> @@ -394,9 +470,21 @@ static int mx51_ecspi_config(struct spi_device *spi,
>  	/* set chip select to use */
>  	ctrl |= MX51_ECSPI_CTRL_CS(spi->chip_select);
>  
> -	ctrl |= (config->bpw - 1) << MX51_ECSPI_CTRL_BL_OFFSET;
> +	if (spi_imx->slave_mode && is_imx53_ecspi(spi_imx))
> +		ctrl |= (spi_imx->slave_burst * 8 - 1)
> +			<< MX51_ECSPI_CTRL_BL_OFFSET;
> +	else
> +		ctrl |= (config->bpw - 1) << MX51_ECSPI_CTRL_BL_OFFSET;
>  
> -	cfg |= MX51_ECSPI_CONFIG_SBBCTRL(spi->chip_select);
> +	/*
> +	 * eCSPI burst completion by Chip Select signal in Slave mode
> +	 * is not functional for imx53 Soc, config SPI burst completed when
> +	 * BURST_LENGTH + 1 bits are received
> +	 */
> +	if (spi_imx->slave_mode && is_imx53_ecspi(spi_imx))
> +		cfg &= ~MX51_ECSPI_CONFIG_SBBCTRL(spi->chip_select);
> +	else
> +		cfg |= MX51_ECSPI_CONFIG_SBBCTRL(spi->chip_select);
>  
>  	if (spi->mode & SPI_CPHA)
>  		cfg |= MX51_ECSPI_CONFIG_SCLKPHA(spi->chip_select);
> @@ -775,6 +863,7 @@ static struct spi_imx_devtype_data imx51_ecspi_devtype_data = {
>  	.trigger = mx51_ecspi_trigger,
>  	.rx_available = mx51_ecspi_rx_available,
>  	.reset = mx51_ecspi_reset,
> +	.disable = mx51_ecspi_disable,
>  	.devtype = IMX51_ECSPI,
>  };
>  
> @@ -784,6 +873,7 @@ static struct spi_imx_devtype_data imx53_ecspi_devtype_data = {
>  	.trigger = mx51_ecspi_trigger,
>  	.rx_available = mx51_ecspi_rx_available,
>  	.reset = mx51_ecspi_reset,
> +	.disable = mx51_ecspi_disable,
>  	.devtype = IMX53_ECSPI,
>  };
>  
> @@ -846,14 +936,16 @@ static void spi_imx_push(struct spi_imx_data *spi_imx)
>  		spi_imx->txfifo++;
>  	}
>  
> -	spi_imx->devtype_data->trigger(spi_imx);
> +	if (!spi_imx->slave_mode)
> +		spi_imx->devtype_data->trigger(spi_imx);
>  }
>  
>  static irqreturn_t spi_imx_isr(int irq, void *dev_id)
>  {
>  	struct spi_imx_data *spi_imx = dev_id;
>  
> -	while (spi_imx->devtype_data->rx_available(spi_imx)) {
> +	while (spi_imx->txfifo &&
> +	       spi_imx->devtype_data->rx_available(spi_imx)) {
>  		spi_imx->rx(spi_imx);
>  		spi_imx->txfifo--;
>  	}
> @@ -952,7 +1044,8 @@ static int spi_imx_setupxfer(struct spi_device *spi,
>  		spi_imx->tx = spi_imx_buf_tx_u32;
>  	}
>  
> -	if (spi_imx_can_dma(spi_imx->bitbang.master, spi, t))
> +	if (!spi_imx->slave_mode &&
> +	    spi_imx_can_dma(spi_imx->bitbang.master, spi, t))
>  		spi_imx->usedma = 1;

So in spi_imx_can_dma() you may return true for a slave mode transfer
and here we end up doing PIO for the same transfer. That doesn't look
correct. You should move the test for slave mode into spi_imx_can_dma().

>  	else
>  		spi_imx->usedma = 0;
> @@ -964,6 +1057,12 @@ static int spi_imx_setupxfer(struct spi_device *spi,
>  			return ret;
>  	}
>  
> +	if (is_imx53_ecspi(spi_imx) && spi_imx->slave_mode) {
> +		spi_imx->rx = mx53_ecspi_rx_slave;
> +		spi_imx->tx = mx53_ecspi_tx_slave;
> +		spi_imx->slave_burst = t->len;
> +	}
> +
>  	spi_imx->devtype_data->config(spi, &config);
>  
>  	return 0;
> @@ -1125,16 +1224,50 @@ static int spi_imx_pio_transfer(struct spi_device *spi,
>  	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
>  	unsigned long transfer_timeout;
>  	unsigned long timeout;
> +	int ret = transfer->len;
> +
> +	if (is_imx53_ecspi(spi_imx) && spi_imx->slave_mode &&
> +	    transfer->len > MX53_MAX_TRANSFER_BYTES) {
> +		pr_err("Transaction too big, max size is %d bytes\n",
> +		       MX53_MAX_TRANSFER_BYTES);

Don't use pr_* functions when you have a struct device * available.

> +		return -EMSGSIZE;
> +	}
>  
>  	spi_imx->tx_buf = transfer->tx_buf;
>  	spi_imx->rx_buf = transfer->rx_buf;
>  	spi_imx->count = transfer->len;
>  	spi_imx->txfifo = 0;
>  
> +	if (spi_imx->slave_mode)
> +		spi_imx->slave_burst = spi_imx->count;

You have set this in spi_imx_setupxfer() already. Why here again?

> +
>  	reinit_completion(&spi_imx->xfer_done);
> +	spi_imx->slave_aborted = false;
>  
>  	spi_imx_push(spi_imx);
>  
> +	if (spi_imx->slave_mode) {
> +		spi_imx->devtype_data->intctrl(spi_imx, MXC_INT_TE |
> +							MXC_INT_RDR);
> +
> +		if (wait_for_completion_interruptible(&spi_imx->xfer_done) ||
> +		    spi_imx->slave_aborted) {
> +			dev_dbg(&spi->dev, "interrupted\n");
> +			ret = -EINTR;
> +		}
> +
> +		/* ecspi has a HW issue when works in Slave mode,
> +		 * after 64 words writtern to TXFIFO, even TXFIFO becomes empty,
> +		 * ECSPI_TXDATA keeps shift out the last word data,
> +		 * so we have to disable ECSPI when in slave mode after the
> +		 * transfer completes
> +		 */
> +		if (spi_imx->devtype_data->disable)
> +			spi_imx->devtype_data->disable(spi_imx);
> +
> +		goto out;
> +	}
> +
>  	spi_imx->devtype_data->intctrl(spi_imx, MXC_INT_TE);
>  
>  	transfer_timeout = spi_imx_calculate_timeout(spi_imx, transfer->len);

You are adding more lines to spi_imx_pio_transfer() than the original
function had, most of them inside a if(spi_imx->slave_mode).
It would probably more readable if you introduced a separate
spi_imx_pio_transfer_slave().

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 linux-next 3/3] spi: imx: Add support for SPI Slave mode

[Jiada Wang]

Hi Sascha

On 05/31/2017 05:10 AM, Sascha Hauer wrote:
> On Wed, May 31, 2017 at 02:19:58AM -0700, jiada_wang-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org wrote:
>> From: Jiada Wang <jiada_wang-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
>>
>> Previously i.MX SPI controller only works in Master mode.
>> This patch adds support to i.MX51, i.MX53 and i.MX6 ECSPI
>> controller to work also in Slave mode.
>>
>> Currently SPI Slave mode support patch has the following limitations:
>> 1. The stale data in RXFIFO will be dropped when the Slave does any new
>>    transfer.
>> 2. One transfer can be finished only after all transfer->len data been
>>    transferred to master device
>> 3. Slave device only accepts transfer->len data. Any data longer than this
>>    from master device will be dropped. Any data shorter than this from
>>    master will cause SPI to stuck due to mentioned HW limitation 2.
>> 4. Only PIO transfer is supported in Slave mode.
>>
>> Following HW limitation applies:
>> 1.  ECSPI has a HW issue when works in Slave mode, after 64
>>     words written to TXFIFO, even TXFIFO becomes empty,
>>     ECSPI_TXDATA keeps shift out the last word data,
>>     so we have to disable ECSPI when in slave mode after the
>>     transfer completes
>> 2.  Due to Freescale errata ERR003775 "eCSPI: Burst completion by Chip
>>     Select (SS) signal in Slave mode is not functional" burst size must
>>     be set exactly to the size of the transfer. This limit SPI transaction
>>     with maximum 2^12 bits. This errata affects i.MX53 and i.MX6 ECSPI
>>     controllers.
>>
>> Signed-off-by: Jiada Wang <jiada_wang-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/spi/spi-imx.c | 227 +++++++++++++++++++++++++++++++++++++++++++-------
>>  1 file changed, 195 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
>> index 765856c..a227a30 100644
>> --- a/drivers/spi/spi-imx.c
>> +++ b/drivers/spi/spi-imx.c
>> @@ -53,9 +53,13 @@
>>  /* generic defines to abstract from the different register layouts */
>>  #define MXC_INT_RR	(1 << 0) /* Receive data ready interrupt */
>>  #define MXC_INT_TE	(1 << 1) /* Transmit FIFO empty interrupt */
>> +#define MXC_INT_RDR	BIT(4) /* Receive date threshold interrupt */
>>  
>>  /* The maximum  bytes that a sdma BD can transfer.*/
>>  #define MAX_SDMA_BD_BYTES  (1 << 15)
>> +/* The maximum bytes that IMX53_ECSPI can transfer in slave mode.*/
>> +#define MX53_MAX_TRANSFER_BYTES		512
>> +
>>  struct spi_imx_config {
>>  	unsigned int speed_hz;
>>  	unsigned int bpw;
>> @@ -79,6 +83,7 @@ struct spi_imx_devtype_data {
>>  	void (*trigger)(struct spi_imx_data *);
>>  	int (*rx_available)(struct spi_imx_data *);
>>  	void (*reset)(struct spi_imx_data *);
>> +	void (*disable)(struct spi_imx_data *);
>>  	enum spi_imx_devtype devtype;
>>  };
>>  
>> @@ -105,6 +110,11 @@ struct spi_imx_data {
>>  	const void *tx_buf;
>>  	unsigned int txfifo; /* number of words pushed in tx FIFO */
>>  
>> +	/* Slave mode */
>> +	bool slave_mode;
>> +	bool slave_aborted;
>> +	unsigned int slave_burst;
>> +
>>  	/* DMA */
>>  	bool usedma;
>>  	u32 wml;
>> @@ -157,6 +167,17 @@ static inline bool spi_imx_has_dmamode(struct spi_imx_data *d)
>>  	}
>>  }
>>  
>> +static inline bool spi_imx_has_slavemode(const struct spi_imx_devtype_data *d)
>> +{
>> +	switch (d->devtype) {
>> +	case IMX51_ECSPI:
>> +	case IMX53_ECSPI:
>> +		return true;
>> +	default:
>> +		return false;
>> +	}
>> +}
> Add a new boolean flag to struct spi_imx_devtype_data instead.
>
>
>> +
>>  #define MXC_SPI_BUF_RX(type)						\
>>  static void spi_imx_buf_rx_##type(struct spi_imx_data *spi_imx)		\
>>  {									\
>> @@ -287,6 +308,7 @@ static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi,
>>  #define MX51_ECSPI_INT		0x10
>>  #define MX51_ECSPI_INT_TEEN		(1 <<  0)
>>  #define MX51_ECSPI_INT_RREN		(1 <<  3)
>> +#define MX51_ECSPI_INT_RDREN		(1 <<  4)
>>  
>>  #define MX51_ECSPI_DMA      0x14
>>  #define MX51_ECSPI_DMA_TX_WML(wml)	((wml) & 0x3f)
>> @@ -303,6 +325,51 @@ static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi,
>>  #define MX51_ECSPI_TESTREG	0x20
>>  #define MX51_ECSPI_TESTREG_LBC	BIT(31)
>>  
>> +static void mx53_ecspi_rx_slave(struct spi_imx_data *spi_imx)
>> +{
>> +	u32 val = be32_to_cpu(readl(spi_imx->base + MXC_CSPIRXDATA));
>> +
>> +	if (spi_imx->rx_buf) {
>> +		int shift = spi_imx->slave_burst % sizeof(val);
>> +
>> +		if (shift) {
>> +			memcpy(spi_imx->rx_buf,
>> +			       ((u8 *)&val) + sizeof(val) - shift, shift);
>> +		} else {
>> +			*((u32 *)spi_imx->rx_buf) = val;
>> +			shift = sizeof(val);
>> +		}
>> +
>> +		spi_imx->rx_buf += shift;
>> +		spi_imx->slave_burst -= shift;
>> +	}
>> +}
>> +
>> +static void mx53_ecspi_tx_slave(struct spi_imx_data *spi_imx)
>> +{
>> +	u32 val = 0;
>> +	int shift = spi_imx->count % sizeof(val);
>> +
>> +	if (spi_imx->tx_buf) {
>> +		if (shift) {
>> +			memcpy(((u8 *)&val) + sizeof(val) - shift,
>> +			       spi_imx->tx_buf, shift);
>> +		} else {
>> +			val = *((u32 *)spi_imx->tx_buf);
>> +			shift = sizeof(val);
>> +		}
>> +		val = cpu_to_be32(val);
>> +		spi_imx->tx_buf += shift;
>> +	}
>> +
>> +	if (!shift)
>> +		shift = sizeof(val);
> A better name for 'shift' is n_bytes. Its value should be calculated
> before the if (spi_imx->tx_buf) block. Then you can use memcpy for the
> four byte case aswell.
>
>
> But anyway, have you tested this function for anything with
> spi_imx->count % sizeof(val) != 0? In this case you have to put the fill
> the FIFO only partly somewhere. I would assume you have to do this at
> the end of the transfer, but you do this at the beginning. Can this
> work?
Yes, I have tested the case when "spi_imx->count % sizeof(val) != 0", it
works.
based on what I observed,
when the data bits written to FIFO exceeds burst len, then the 'old'
bits will be dropped.

I will update this patch set to address your review comments in v3

Thanks,
Jiada

>> +
>> +	spi_imx->count -= shift;
>> +
>> +	writel(val, spi_imx->base + MXC_CSPITXDATA);
>> +}
>> +
>>  /* MX51 eCSPI */
>>  static unsigned int mx51_ecspi_clkdiv(struct spi_imx_data *spi_imx,
>>  				      unsigned int fspi, unsigned int *fres)
>> @@ -352,6 +419,9 @@ static void mx51_ecspi_intctrl(struct spi_imx_data *spi_imx, int enable)
>>  	if (enable & MXC_INT_RR)
>>  		val |= MX51_ECSPI_INT_RREN;
>>  
>> +	if (enable & MXC_INT_RDR)
>> +		val |= MX51_ECSPI_INT_RDREN;
>> +
>>  	writel(val, spi_imx->base + MX51_ECSPI_INT);
>>  }
>>  
>> @@ -364,6 +434,15 @@ static void mx51_ecspi_trigger(struct spi_imx_data *spi_imx)
>>  	writel(reg, spi_imx->base + MX51_ECSPI_CTRL);
>>  }
>>  
>> +static void __maybe_unused mx51_ecspi_disable(struct spi_imx_data *spi_imx)
> I don't see how this function may end up unused.
>
>> +{
>> +	u32 ctrl;
>> +
>> +	ctrl = readl(spi_imx->base + MX51_ECSPI_CTRL);
>> +	ctrl &= ~MX51_ECSPI_CTRL_ENABLE;
>> +	writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
>> +}
>> +
>>  static int mx51_ecspi_config(struct spi_device *spi,
>>  			     struct spi_imx_config *config)
>>  {
>> @@ -372,14 +451,11 @@ static int mx51_ecspi_config(struct spi_device *spi,
>>  	u32 clk = config->speed_hz, delay, reg;
>>  	u32 cfg = readl(spi_imx->base + MX51_ECSPI_CONFIG);
>>  
>> -	/*
>> -	 * The hardware seems to have a race condition when changing modes. The
>> -	 * current assumption is that the selection of the channel arrives
>> -	 * earlier in the hardware than the mode bits when they are written at
>> -	 * the same time.
>> -	 * So set master mode for all channels as we do not support slave mode.
>> -	 */
>> -	ctrl |= MX51_ECSPI_CTRL_MODE_MASK;
>> +	/* set Master or Slave mode */
>> +	if (spi_imx->slave_mode)
>> +		ctrl &= ~MX51_ECSPI_CTRL_MODE_MASK;
>> +	else
>> +		ctrl |= MX51_ECSPI_CTRL_MODE_MASK;
>>  
>>  	/*
>>  	 * Enable SPI_RDY handling (falling edge/level triggered).
>> @@ -394,9 +470,21 @@ static int mx51_ecspi_config(struct spi_device *spi,
>>  	/* set chip select to use */
>>  	ctrl |= MX51_ECSPI_CTRL_CS(spi->chip_select);
>>  
>> -	ctrl |= (config->bpw - 1) << MX51_ECSPI_CTRL_BL_OFFSET;
>> +	if (spi_imx->slave_mode && is_imx53_ecspi(spi_imx))
>> +		ctrl |= (spi_imx->slave_burst * 8 - 1)
>> +			<< MX51_ECSPI_CTRL_BL_OFFSET;
>> +	else
>> +		ctrl |= (config->bpw - 1) << MX51_ECSPI_CTRL_BL_OFFSET;
>>  
>> -	cfg |= MX51_ECSPI_CONFIG_SBBCTRL(spi->chip_select);
>> +	/*
>> +	 * eCSPI burst completion by Chip Select signal in Slave mode
>> +	 * is not functional for imx53 Soc, config SPI burst completed when
>> +	 * BURST_LENGTH + 1 bits are received
>> +	 */
>> +	if (spi_imx->slave_mode && is_imx53_ecspi(spi_imx))
>> +		cfg &= ~MX51_ECSPI_CONFIG_SBBCTRL(spi->chip_select);
>> +	else
>> +		cfg |= MX51_ECSPI_CONFIG_SBBCTRL(spi->chip_select);
>>  
>>  	if (spi->mode & SPI_CPHA)
>>  		cfg |= MX51_ECSPI_CONFIG_SCLKPHA(spi->chip_select);
>> @@ -775,6 +863,7 @@ static struct spi_imx_devtype_data imx51_ecspi_devtype_data = {
>>  	.trigger = mx51_ecspi_trigger,
>>  	.rx_available = mx51_ecspi_rx_available,
>>  	.reset = mx51_ecspi_reset,
>> +	.disable = mx51_ecspi_disable,
>>  	.devtype = IMX51_ECSPI,
>>  };
>>  
>> @@ -784,6 +873,7 @@ static struct spi_imx_devtype_data imx53_ecspi_devtype_data = {
>>  	.trigger = mx51_ecspi_trigger,
>>  	.rx_available = mx51_ecspi_rx_available,
>>  	.reset = mx51_ecspi_reset,
>> +	.disable = mx51_ecspi_disable,
>>  	.devtype = IMX53_ECSPI,
>>  };
>>  
>> @@ -846,14 +936,16 @@ static void spi_imx_push(struct spi_imx_data *spi_imx)
>>  		spi_imx->txfifo++;
>>  	}
>>  
>> -	spi_imx->devtype_data->trigger(spi_imx);
>> +	if (!spi_imx->slave_mode)
>> +		spi_imx->devtype_data->trigger(spi_imx);
>>  }
>>  
>>  static irqreturn_t spi_imx_isr(int irq, void *dev_id)
>>  {
>>  	struct spi_imx_data *spi_imx = dev_id;
>>  
>> -	while (spi_imx->devtype_data->rx_available(spi_imx)) {
>> +	while (spi_imx->txfifo &&
>> +	       spi_imx->devtype_data->rx_available(spi_imx)) {
>>  		spi_imx->rx(spi_imx);
>>  		spi_imx->txfifo--;
>>  	}
>> @@ -952,7 +1044,8 @@ static int spi_imx_setupxfer(struct spi_device *spi,
>>  		spi_imx->tx = spi_imx_buf_tx_u32;
>>  	}
>>  
>> -	if (spi_imx_can_dma(spi_imx->bitbang.master, spi, t))
>> +	if (!spi_imx->slave_mode &&
>> +	    spi_imx_can_dma(spi_imx->bitbang.master, spi, t))
>>  		spi_imx->usedma = 1;
> So in spi_imx_can_dma() you may return true for a slave mode transfer
> and here we end up doing PIO for the same transfer. That doesn't look
> correct. You should move the test for slave mode into spi_imx_can_dma().
>
>>  	else
>>  		spi_imx->usedma = 0;
>> @@ -964,6 +1057,12 @@ static int spi_imx_setupxfer(struct spi_device *spi,
>>  			return ret;
>>  	}
>>  
>> +	if (is_imx53_ecspi(spi_imx) && spi_imx->slave_mode) {
>> +		spi_imx->rx = mx53_ecspi_rx_slave;
>> +		spi_imx->tx = mx53_ecspi_tx_slave;
>> +		spi_imx->slave_burst = t->len;
>> +	}
>> +
>>  	spi_imx->devtype_data->config(spi, &config);
>>  
>>  	return 0;
>> @@ -1125,16 +1224,50 @@ static int spi_imx_pio_transfer(struct spi_device *spi,
>>  	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
>>  	unsigned long transfer_timeout;
>>  	unsigned long timeout;
>> +	int ret = transfer->len;
>> +
>> +	if (is_imx53_ecspi(spi_imx) && spi_imx->slave_mode &&
>> +	    transfer->len > MX53_MAX_TRANSFER_BYTES) {
>> +		pr_err("Transaction too big, max size is %d bytes\n",
>> +		       MX53_MAX_TRANSFER_BYTES);
> Don't use pr_* functions when you have a struct device * available.
>
>> +		return -EMSGSIZE;
>> +	}
>>  
>>  	spi_imx->tx_buf = transfer->tx_buf;
>>  	spi_imx->rx_buf = transfer->rx_buf;
>>  	spi_imx->count = transfer->len;
>>  	spi_imx->txfifo = 0;
>>  
>> +	if (spi_imx->slave_mode)
>> +		spi_imx->slave_burst = spi_imx->count;
> You have set this in spi_imx_setupxfer() already. Why here again?
>
>> +
>>  	reinit_completion(&spi_imx->xfer_done);
>> +	spi_imx->slave_aborted = false;
>>  
>>  	spi_imx_push(spi_imx);
>>  
>> +	if (spi_imx->slave_mode) {
>> +		spi_imx->devtype_data->intctrl(spi_imx, MXC_INT_TE |
>> +							MXC_INT_RDR);
>> +
>> +		if (wait_for_completion_interruptible(&spi_imx->xfer_done) ||
>> +		    spi_imx->slave_aborted) {
>> +			dev_dbg(&spi->dev, "interrupted\n");
>> +			ret = -EINTR;
>> +		}
>> +
>> +		/* ecspi has a HW issue when works in Slave mode,
>> +		 * after 64 words writtern to TXFIFO, even TXFIFO becomes empty,
>> +		 * ECSPI_TXDATA keeps shift out the last word data,
>> +		 * so we have to disable ECSPI when in slave mode after the
>> +		 * transfer completes
>> +		 */
>> +		if (spi_imx->devtype_data->disable)
>> +			spi_imx->devtype_data->disable(spi_imx);
>> +
>> +		goto out;
>> +	}
>> +
>>  	spi_imx->devtype_data->intctrl(spi_imx, MXC_INT_TE);
>>  
>>  	transfer_timeout = spi_imx_calculate_timeout(spi_imx, transfer->len);
> You are adding more lines to spi_imx_pio_transfer() than the original
> function had, most of them inside a if(spi_imx->slave_mode).
> It would probably more readable if you introduced a separate
> spi_imx_pio_transfer_slave().
>
> Sascha
>
>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 linux-next 3/3] spi: imx: Add support for SPI Slave mode

[Sascha Hauer]

On Fri, Jun 02, 2017 at 06:13:07AM -0700, Jiada Wang wrote:
> Hi Sascha
> 
> On 05/31/2017 05:10 AM, Sascha Hauer wrote:
> >> +
> >> +static void mx53_ecspi_tx_slave(struct spi_imx_data *spi_imx)
> >> +{
> >> +	u32 val = 0;
> >> +	int shift = spi_imx->count % sizeof(val);
> >> +
> >> +	if (spi_imx->tx_buf) {
> >> +		if (shift) {
> >> +			memcpy(((u8 *)&val) + sizeof(val) - shift,
> >> +			       spi_imx->tx_buf, shift);
> >> +		} else {
> >> +			val = *((u32 *)spi_imx->tx_buf);
> >> +			shift = sizeof(val);
> >> +		}
> >> +		val = cpu_to_be32(val);
> >> +		spi_imx->tx_buf += shift;
> >> +	}
> >> +
> >> +	if (!shift)
> >> +		shift = sizeof(val);
> > A better name for 'shift' is n_bytes. Its value should be calculated
> > before the if (spi_imx->tx_buf) block. Then you can use memcpy for the
> > four byte case aswell.
> >
> >
> > But anyway, have you tested this function for anything with
> > spi_imx->count % sizeof(val) != 0? In this case you have to put the fill
> > the FIFO only partly somewhere. I would assume you have to do this at
> > the end of the transfer, but you do this at the beginning. Can this
> > work?
> Yes, I have tested the case when "spi_imx->count % sizeof(val) != 0", it
> works.

For the record: Yes, this seems to be right. I misread the datasheet.
When the controller is programmed to transfer 5 bytes, then it seemed
fairly logical to me that it first expects four bytes in its 32bit FIFO
and then the remaining single byte. In fact it's the other way round,
how the example of the BURST_LENGTH register description says:

0x021 A SPI burst contains the 2 LSB in first word and all 32 bits in
second word.

So the part that doesn't fill a whole FIFO width goes out first, not
last.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html