[PATCH v3 0/5] spi: imx: Fix polarity switching for mx51-ecspi

[PATCH v3 0/5] spi: imx: Fix polarity switching for mx51-ecspi

[Uwe Kleine-König]

Hello,

compared to v2 sent with Message-Id:
20181123085158.24753-1-u.kleine-koenig@pengutronix.de I squashed in the
change suggested by Robin (with s/filed/field/). Thanks for testing and
feedback.

Best regards
Uwe

[PATCH v3 1/5] spi: imx: add a device specific prepare_message callback

[Uwe Kleine-König]

This is just preparatory work which allows to move some initialisation
that currently is done in the per transfer hook .config to an earlier
point in time in the next few patches. There is no change in behaviour
introduced by this patch.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/spi/spi-imx.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index dd1ce12aa386..c7db42d6b3bc 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -59,6 +59,7 @@ struct spi_imx_data;
 
 struct spi_imx_devtype_data {
 	void (*intctrl)(struct spi_imx_data *, int);
+	int (*prepare_message)(struct spi_imx_data *, struct spi_message *);
 	int (*config)(struct spi_device *);
 	void (*trigger)(struct spi_imx_data *);
 	int (*rx_available)(struct spi_imx_data *);
@@ -486,6 +487,12 @@ static void mx51_ecspi_disable(struct spi_imx_data *spi_imx)
 	writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
 }
 
+static int mx51_ecspi_prepare_message(struct spi_imx_data *spi_imx,
+				      struct spi_message *msg)
+{
+	return 0;
+}
+
 static int mx51_ecspi_config(struct spi_device *spi)
 {
 	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
@@ -659,6 +666,12 @@ static void mx31_trigger(struct spi_imx_data *spi_imx)
 	writel(reg, spi_imx->base + MXC_CSPICTRL);
 }
 
+static int mx31_prepare_message(struct spi_imx_data *spi_imx,
+				struct spi_message *msg)
+{
+	return 0;
+}
+
 static int mx31_config(struct spi_device *spi)
 {
 	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
@@ -755,6 +768,12 @@ static void mx21_trigger(struct spi_imx_data *spi_imx)
 	writel(reg, spi_imx->base + MXC_CSPICTRL);
 }
 
+static int mx21_prepare_message(struct spi_imx_data *spi_imx,
+				struct spi_message *msg)
+{
+	return 0;
+}
+
 static int mx21_config(struct spi_device *spi)
 {
 	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
@@ -824,6 +843,12 @@ static void mx1_trigger(struct spi_imx_data *spi_imx)
 	writel(reg, spi_imx->base + MXC_CSPICTRL);
 }
 
+static int mx1_prepare_message(struct spi_imx_data *spi_imx,
+			       struct spi_message *msg)
+{
+	return 0;
+}
+
 static int mx1_config(struct spi_device *spi)
 {
 	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
@@ -858,6 +883,7 @@ static void mx1_reset(struct spi_imx_data *spi_imx)
 
 static struct spi_imx_devtype_data imx1_cspi_devtype_data = {
 	.intctrl = mx1_intctrl,
+	.prepare_message = mx1_prepare_message,
 	.config = mx1_config,
 	.trigger = mx1_trigger,
 	.rx_available = mx1_rx_available,
@@ -871,6 +897,7 @@ static struct spi_imx_devtype_data imx1_cspi_devtype_data = {
 
 static struct spi_imx_devtype_data imx21_cspi_devtype_data = {
 	.intctrl = mx21_intctrl,
+	.prepare_message = mx21_prepare_message,
 	.config = mx21_config,
 	.trigger = mx21_trigger,
 	.rx_available = mx21_rx_available,
@@ -885,6 +912,7 @@ static struct spi_imx_devtype_data imx21_cspi_devtype_data = {
 static struct spi_imx_devtype_data imx27_cspi_devtype_data = {
 	/* i.mx27 cspi shares the functions with i.mx21 one */
 	.intctrl = mx21_intctrl,
+	.prepare_message = mx21_prepare_message,
 	.config = mx21_config,
 	.trigger = mx21_trigger,
 	.rx_available = mx21_rx_available,
@@ -898,6 +926,7 @@ static struct spi_imx_devtype_data imx27_cspi_devtype_data = {
 
 static struct spi_imx_devtype_data imx31_cspi_devtype_data = {
 	.intctrl = mx31_intctrl,
+	.prepare_message = mx31_prepare_message,
 	.config = mx31_config,
 	.trigger = mx31_trigger,
 	.rx_available = mx31_rx_available,
@@ -912,6 +941,7 @@ static struct spi_imx_devtype_data imx31_cspi_devtype_data = {
 static struct spi_imx_devtype_data imx35_cspi_devtype_data = {
 	/* i.mx35 and later cspi shares the functions with i.mx31 one */
 	.intctrl = mx31_intctrl,
+	.prepare_message = mx31_prepare_message,
 	.config = mx31_config,
 	.trigger = mx31_trigger,
 	.rx_available = mx31_rx_available,
@@ -925,6 +955,7 @@ static struct spi_imx_devtype_data imx35_cspi_devtype_data = {
 
 static struct spi_imx_devtype_data imx51_ecspi_devtype_data = {
 	.intctrl = mx51_ecspi_intctrl,
+	.prepare_message = mx51_ecspi_prepare_message,
 	.config = mx51_ecspi_config,
 	.trigger = mx51_ecspi_trigger,
 	.rx_available = mx51_ecspi_rx_available,
@@ -940,6 +971,7 @@ static struct spi_imx_devtype_data imx51_ecspi_devtype_data = {
 
 static struct spi_imx_devtype_data imx53_ecspi_devtype_data = {
 	.intctrl = mx51_ecspi_intctrl,
+	.prepare_message = mx51_ecspi_prepare_message,
 	.config = mx51_ecspi_config,
 	.trigger = mx51_ecspi_trigger,
 	.rx_available = mx51_ecspi_rx_available,
@@ -1492,7 +1524,13 @@ spi_imx_prepare_message(struct spi_master *master, struct spi_message *msg)
 		return ret;
 	}
 
-	return 0;
+	ret = spi_imx->devtype_data->prepare_message(spi_imx, msg);
+	if (ret) {
+		clk_disable(spi_imx->clk_ipg);
+		clk_disable(spi_imx->clk_per);
+	}
+
+	return ret;
 }
 
 static int
-- 
2.19.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 2/5] spi: imx: mx51-ecspi: Move some initialisation to prepare_message hook.

[Uwe Kleine-König]

The relevant difference between prepare_message and config is that the
former is run before the CS signal is asserted. So the polarity of the
CLK line must be configured in prepare_message as an edge generated by
config might already result in a latch of the MOSI line.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/spi/spi-imx.c | 67 ++++++++++++++++++++++++++-----------------
 1 file changed, 40 insertions(+), 27 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index c7db42d6b3bc..d3a1e7104556 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -490,14 +490,9 @@ static void mx51_ecspi_disable(struct spi_imx_data *spi_imx)
 static int mx51_ecspi_prepare_message(struct spi_imx_data *spi_imx,
 				      struct spi_message *msg)
 {
-	return 0;
-}
-
-static int mx51_ecspi_config(struct spi_device *spi)
-{
-	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
+	struct spi_device *spi = msg->spi;
 	u32 ctrl = MX51_ECSPI_CTRL_ENABLE;
-	u32 clk = spi_imx->speed_hz, delay, reg;
+	u32 testreg;
 	u32 cfg = readl(spi_imx->base + MX51_ECSPI_CONFIG);
 
 	/* set Master or Slave mode */
@@ -512,19 +507,21 @@ static int mx51_ecspi_config(struct spi_device *spi)
 	if (spi->mode & SPI_READY)
 		ctrl |= MX51_ECSPI_CTRL_DRCTL(spi_imx->spi_drctl);
 
-	/* set clock speed */
-	ctrl |= mx51_ecspi_clkdiv(spi_imx, spi_imx->speed_hz, &clk);
-	spi_imx->spi_bus_clk = clk;
-
 	/* set chip select to use */
 	ctrl |= MX51_ECSPI_CTRL_CS(spi->chip_select);
 
-	if (spi_imx->slave_mode && is_imx53_ecspi(spi_imx))
-		ctrl |= (spi_imx->slave_burst * 8 - 1)
-			<< MX51_ECSPI_CTRL_BL_OFFSET;
+	/*
+	 * The ctrl register must be written first, with the EN bit set other
+	 * registers must not be written to.
+	 */
+	writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
+
+	testreg = readl(spi_imx->base + MX51_ECSPI_TESTREG);
+	if (spi->mode & SPI_LOOP)
+		testreg |= MX51_ECSPI_TESTREG_LBC;
 	else
-		ctrl |= (spi_imx->bits_per_word - 1)
-			<< MX51_ECSPI_CTRL_BL_OFFSET;
+		testreg &= ~MX51_ECSPI_TESTREG_LBC;
+	writel(testreg, spi_imx->base + MX51_ECSPI_TESTREG);
 
 	/*
 	 * eCSPI burst completion by Chip Select signal in Slave mode
@@ -548,25 +545,42 @@ static int mx51_ecspi_config(struct spi_device *spi)
 		cfg &= ~MX51_ECSPI_CONFIG_SCLKPOL(spi->chip_select);
 		cfg &= ~MX51_ECSPI_CONFIG_SCLKCTL(spi->chip_select);
 	}
+
 	if (spi->mode & SPI_CS_HIGH)
 		cfg |= MX51_ECSPI_CONFIG_SSBPOL(spi->chip_select);
 	else
 		cfg &= ~MX51_ECSPI_CONFIG_SSBPOL(spi->chip_select);
 
-	if (spi_imx->usedma)
-		ctrl |= MX51_ECSPI_CTRL_SMC;
+	writel(cfg, spi_imx->base + MX51_ECSPI_CONFIG);
 
-	/* CTRL register always go first to bring out controller from reset */
-	writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
+	return 0;
+}
 
-	reg = readl(spi_imx->base + MX51_ECSPI_TESTREG);
-	if (spi->mode & SPI_LOOP)
-		reg |= MX51_ECSPI_TESTREG_LBC;
+static int mx51_ecspi_config(struct spi_device *spi)
+{
+	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
+	u32 ctrl = readl(spi_imx->base + MX51_ECSPI_CTRL);
+	u32 clk = spi_imx->speed_hz, delay;
+
+	/* Clear BL field and set the right value */
+	ctrl &= ~MX51_ECSPI_CTRL_BL_MASK;
+	if (spi_imx->slave_mode && is_imx53_ecspi(spi_imx))
+		ctrl |= (spi_imx->slave_burst * 8 - 1)
+			<< MX51_ECSPI_CTRL_BL_OFFSET;
 	else
-		reg &= ~MX51_ECSPI_TESTREG_LBC;
-	writel(reg, spi_imx->base + MX51_ECSPI_TESTREG);
+		ctrl |= (spi_imx->bits_per_word - 1)
+			<< MX51_ECSPI_CTRL_BL_OFFSET;
 
-	writel(cfg, spi_imx->base + MX51_ECSPI_CONFIG);
+	/* set clock speed */
+	ctrl &= ~(0xf << MX51_ECSPI_CTRL_POSTDIV_OFFSET |
+		  0xf << MX51_ECSPI_CTRL_PREDIV_OFFSET);
+	ctrl |= mx51_ecspi_clkdiv(spi_imx, spi_imx->speed_hz, &clk);
+	spi_imx->spi_bus_clk = clk;
+
+	if (spi_imx->usedma)
+		ctrl |= MX51_ECSPI_CTRL_SMC;
+
+	writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
 
 	/*
 	 * Wait until the changes in the configuration register CONFIGREG
@@ -594,7 +608,6 @@ static void mx51_setup_wml(struct spi_imx_data *spi_imx)
 	 * Configure the DMA register: setup the watermark
 	 * and enable DMA request.
 	 */
-
 	writel(MX51_ECSPI_DMA_RX_WML(spi_imx->wml - 1) |
 		MX51_ECSPI_DMA_TX_WML(spi_imx->wml) |
 		MX51_ECSPI_DMA_RXT_WML(spi_imx->wml) |
-- 
2.19.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 2/5] spi: imx: mx51-ecspi: Move some initialisation to prepare_message hook.

[Robin Gong]

> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Sent: 2018年11月30日 14:47
> To: Mark Brown <broonie@kernel.org>; Robin Gong <yibin.gong@nxp.com>
> Cc: Marek Vasut <marex@denx.de>; dl-linux-imx <linux-imx@nxp.com>;
> kernel@pengutronix.de; linux-spi@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org
> Subject: [PATCH v3 2/5] spi: imx: mx51-ecspi: Move some initialisation to
> prepare_message hook.
> 
> The relevant difference between prepare_message and config is that the
> former is run before the CS signal is asserted. So the polarity of the CLK line
> must be configured in prepare_message as an edge generated by config might
> already result in a latch of the MOSI line.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/spi/spi-imx.c | 67 ++++++++++++++++++++++++++-----------------
>  1 file changed, 40 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c index
> c7db42d6b3bc..d3a1e7104556 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -490,14 +490,9 @@ static void mx51_ecspi_disable(struct spi_imx_data
> *spi_imx)  static int mx51_ecspi_prepare_message(struct spi_imx_data
> *spi_imx,
>  				      struct spi_message *msg)
>  {
> -	return 0;
> -}
> -
> -static int mx51_ecspi_config(struct spi_device *spi) -{
> -	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
> +	struct spi_device *spi = msg->spi;
>  	u32 ctrl = MX51_ECSPI_CTRL_ENABLE;
> -	u32 clk = spi_imx->speed_hz, delay, reg;
> +	u32 testreg;
>  	u32 cfg = readl(spi_imx->base + MX51_ECSPI_CONFIG);
> 
>  	/* set Master or Slave mode */
> @@ -512,19 +507,21 @@ static int mx51_ecspi_config(struct spi_device *spi)
>  	if (spi->mode & SPI_READY)
>  		ctrl |= MX51_ECSPI_CTRL_DRCTL(spi_imx->spi_drctl);
> 
> -	/* set clock speed */
> -	ctrl |= mx51_ecspi_clkdiv(spi_imx, spi_imx->speed_hz, &clk);
> -	spi_imx->spi_bus_clk = clk;
> -
>  	/* set chip select to use */
>  	ctrl |= MX51_ECSPI_CTRL_CS(spi->chip_select);
> 
> -	if (spi_imx->slave_mode && is_imx53_ecspi(spi_imx))
> -		ctrl |= (spi_imx->slave_burst * 8 - 1)
> -			<< MX51_ECSPI_CTRL_BL_OFFSET;
> +	/*
> +	 * The ctrl register must be written first, with the EN bit set other
> +	 * registers must not be written to.
> +	 */
> +	writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
> +
> +	testreg = readl(spi_imx->base + MX51_ECSPI_TESTREG);
> +	if (spi->mode & SPI_LOOP)
> +		testreg |= MX51_ECSPI_TESTREG_LBC;
>  	else
> -		ctrl |= (spi_imx->bits_per_word - 1)
> -			<< MX51_ECSPI_CTRL_BL_OFFSET;
> +		testreg &= ~MX51_ECSPI_TESTREG_LBC;
> +	writel(testreg, spi_imx->base + MX51_ECSPI_TESTREG);
> 
>  	/*
>  	 * eCSPI burst completion by Chip Select signal in Slave mode @@ -548,25
> +545,42 @@ static int mx51_ecspi_config(struct spi_device *spi)
>  		cfg &= ~MX51_ECSPI_CONFIG_SCLKPOL(spi->chip_select);
>  		cfg &= ~MX51_ECSPI_CONFIG_SCLKCTL(spi->chip_select);
>  	}
> +
>  	if (spi->mode & SPI_CS_HIGH)
>  		cfg |= MX51_ECSPI_CONFIG_SSBPOL(spi->chip_select);
>  	else
>  		cfg &= ~MX51_ECSPI_CONFIG_SSBPOL(spi->chip_select);
> 
> -	if (spi_imx->usedma)
> -		ctrl |= MX51_ECSPI_CTRL_SMC;
> +	writel(cfg, spi_imx->base + MX51_ECSPI_CONFIG);
> 
> -	/* CTRL register always go first to bring out controller from reset */
> -	writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
> +	return 0;
> +}
> 
> -	reg = readl(spi_imx->base + MX51_ECSPI_TESTREG);
> -	if (spi->mode & SPI_LOOP)
> -		reg |= MX51_ECSPI_TESTREG_LBC;
> +static int mx51_ecspi_config(struct spi_device *spi) {
> +	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
> +	u32 ctrl = readl(spi_imx->base + MX51_ECSPI_CTRL);
> +	u32 clk = spi_imx->speed_hz, delay;
> +
> +	/* Clear BL field and set the right value */
> +	ctrl &= ~MX51_ECSPI_CTRL_BL_MASK;
> +	if (spi_imx->slave_mode && is_imx53_ecspi(spi_imx))
> +		ctrl |= (spi_imx->slave_burst * 8 - 1)
> +			<< MX51_ECSPI_CTRL_BL_OFFSET;
>  	else
> -		reg &= ~MX51_ECSPI_TESTREG_LBC;
> -	writel(reg, spi_imx->base + MX51_ECSPI_TESTREG);
> +		ctrl |= (spi_imx->bits_per_word - 1)
> +			<< MX51_ECSPI_CTRL_BL_OFFSET;
> 
> -	writel(cfg, spi_imx->base + MX51_ECSPI_CONFIG);
> +	/* set clock speed */
> +	ctrl &= ~(0xf << MX51_ECSPI_CTRL_POSTDIV_OFFSET |
> +		  0xf << MX51_ECSPI_CTRL_PREDIV_OFFSET);
> +	ctrl |= mx51_ecspi_clkdiv(spi_imx, spi_imx->speed_hz, &clk);
> +	spi_imx->spi_bus_clk = clk;
> +
> +	if (spi_imx->usedma)
> +		ctrl |= MX51_ECSPI_CTRL_SMC;
> +
> +	writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
> 
>  	/*
>  	 * Wait until the changes in the configuration register CONFIGREG @@
Seems we don't need this commit anymore, your patch fix it by the way?

commit 6fd8b8503a0dcf66510314dc054745087ae89f94
Author: Marek Vasut <marex@denx.de>
Date:   Wed Dec 18 18:31:47 2013 +0100

    spi: spi-imx: Fix out-of-order CS/SCLK operation at low speeds

> -594,7 +608,6 @@ static void mx51_setup_wml(struct spi_imx_data *spi_imx)
>  	 * Configure the DMA register: setup the watermark
>  	 * and enable DMA request.
>  	 */
> -
>  	writel(MX51_ECSPI_DMA_RX_WML(spi_imx->wml - 1) |
>  		MX51_ECSPI_DMA_TX_WML(spi_imx->wml) |
>  		MX51_ECSPI_DMA_RXT_WML(spi_imx->wml) |
> --
> 2.19.2

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/5] spi: imx: mx51-ecspi: Move some initialisation to prepare_message hook.

[Uwe Kleine-König]

Hello,

On Mon, Dec 03, 2018 at 08:00:21AM +0000, Robin Gong wrote:
> >  	/*
> >  	 * Wait until the changes in the configuration register CONFIGREG @@
> Seems we don't need this commit anymore, your patch fix it by the way?
> 
> commit 6fd8b8503a0dcf66510314dc054745087ae89f94
> Author: Marek Vasut <marex@denx.de>
> Date:   Wed Dec 18 18:31:47 2013 +0100
> 
>     spi: spi-imx: Fix out-of-order CS/SCLK operation at low speeds

I don't know, that's why I put Marek on Cc and talked to him in irc. I
couldn't lure him into testing on his machine if this indeed fixes his
problem.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH v3 3/5] spi: imx: style fixes

[Uwe Kleine-König]

This change fixes some random style issues that I noticed while debugging
the driver: Remove some double spaces, use tabs for indention instead
of spaces if possible, fix comment style.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/spi/spi-imx.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index d3a1e7104556..d954b6d958c2 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -39,8 +39,8 @@
 #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 a sdma BD can transfer. */
+#define MAX_SDMA_BD_BYTES (1 << 15)
 #define MX51_ECSPI_CTRL_MAX_BURST	512
 /* The maximum bytes that IMX53_ECSPI can transfer in slave mode.*/
 #define MX53_MAX_TRANSFER_BYTES		512
@@ -257,7 +257,7 @@ static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi,
 #define MX51_ECSPI_INT_RREN		(1 <<  3)
 #define MX51_ECSPI_INT_RDREN		(1 <<  4)
 
-#define MX51_ECSPI_DMA      0x14
+#define MX51_ECSPI_DMA		0x14
 #define MX51_ECSPI_DMA_TX_WML(wml)	((wml) & 0x3f)
 #define MX51_ECSPI_DMA_RX_WML(wml)	(((wml) & 0x3f) << 16)
 #define MX51_ECSPI_DMA_RXT_WML(wml)	(((wml) & 0x3f) << 24)
@@ -726,8 +726,10 @@ static int mx31_config(struct spi_device *spi)
 	writel(reg, spi_imx->base + MX31_CSPI_TESTREG);
 
 	if (spi_imx->usedma) {
-		/* configure DMA requests when RXFIFO is half full and
-		   when TXFIFO is half empty */
+		/*
+		 * configure DMA requests when RXFIFO is half full and
+		 * when TXFIFO is half empty
+		 */
 		writel(MX31_DMAREG_RH_DEN | MX31_DMAREG_TH_DEN,
 			spi_imx->base + MX31_CSPI_DMAREG);
 	}
@@ -1093,7 +1095,7 @@ static void spi_imx_push(struct spi_imx_data *spi_imx)
 		if (!spi_imx->count)
 			break;
 		if (spi_imx->dynamic_burst &&
-		    spi_imx->txfifo >=  DIV_ROUND_UP(spi_imx->remainder,
+		    spi_imx->txfifo >= DIV_ROUND_UP(spi_imx->remainder,
 						     fifo_words))
 			break;
 		spi_imx->tx(spi_imx);
@@ -1187,7 +1189,7 @@ static int spi_imx_setupxfer(struct spi_device *spi,
 		return 0;
 
 	spi_imx->bits_per_word = t->bits_per_word;
-	spi_imx->speed_hz  = t->speed_hz;
+	spi_imx->speed_hz = t->speed_hz;
 
 	/*
 	 * Initialize the functions for transfer. To transfer non byte-aligned
-- 
2.19.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 4/5] spi: imx: rename config callback and add useful parameters

[Uwe Kleine-König]

The config callback is called once per transfer while some things can (and
should) be done on a per message manner. To have unambiguous naming in the
end include "transfer" in the callback's name and rename the
implementations accordingly. Also pass the driver struct and transfer
which allows further simplifications in the following patch.

There is no change in behavior intended here.

Reviewed-by: Marek Vasut <marex@denx.de>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/spi/spi-imx.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index d954b6d958c2..72c879226abd 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -60,7 +60,8 @@ struct spi_imx_data;
 struct spi_imx_devtype_data {
 	void (*intctrl)(struct spi_imx_data *, int);
 	int (*prepare_message)(struct spi_imx_data *, struct spi_message *);
-	int (*config)(struct spi_device *);
+	int (*prepare_transfer)(struct spi_imx_data *, struct spi_device *,
+				struct spi_transfer *);
 	void (*trigger)(struct spi_imx_data *);
 	int (*rx_available)(struct spi_imx_data *);
 	void (*reset)(struct spi_imx_data *);
@@ -556,9 +557,10 @@ static int mx51_ecspi_prepare_message(struct spi_imx_data *spi_imx,
 	return 0;
 }
 
-static int mx51_ecspi_config(struct spi_device *spi)
+static int mx51_ecspi_prepare_transfer(struct spi_imx_data *spi_imx,
+				       struct spi_device *spi,
+				       struct spi_transfer *t)
 {
-	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
 	u32 ctrl = readl(spi_imx->base + MX51_ECSPI_CTRL);
 	u32 clk = spi_imx->speed_hz, delay;
 
@@ -685,9 +687,10 @@ static int mx31_prepare_message(struct spi_imx_data *spi_imx,
 	return 0;
 }
 
-static int mx31_config(struct spi_device *spi)
+static int mx31_prepare_transfer(struct spi_imx_data *spi_imx,
+				 struct spi_device *spi,
+				 struct spi_transfer *t)
 {
-	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
 	unsigned int reg = MX31_CSPICTRL_ENABLE | MX31_CSPICTRL_MASTER;
 	unsigned int clk;
 
@@ -789,9 +792,10 @@ static int mx21_prepare_message(struct spi_imx_data *spi_imx,
 	return 0;
 }
 
-static int mx21_config(struct spi_device *spi)
+static int mx21_prepare_transfer(struct spi_imx_data *spi_imx,
+				 struct spi_device *spi,
+				 struct spi_transfer *t)
 {
-	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
 	unsigned int reg = MX21_CSPICTRL_ENABLE | MX21_CSPICTRL_MASTER;
 	unsigned int max = is_imx27_cspi(spi_imx) ? 16 : 18;
 	unsigned int clk;
@@ -864,9 +868,10 @@ static int mx1_prepare_message(struct spi_imx_data *spi_imx,
 	return 0;
 }
 
-static int mx1_config(struct spi_device *spi)
+static int mx1_prepare_transfer(struct spi_imx_data *spi_imx,
+				struct spi_device *spi,
+				struct spi_transfer *t)
 {
-	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
 	unsigned int reg = MX1_CSPICTRL_ENABLE | MX1_CSPICTRL_MASTER;
 	unsigned int clk;
 
@@ -899,7 +904,7 @@ static void mx1_reset(struct spi_imx_data *spi_imx)
 static struct spi_imx_devtype_data imx1_cspi_devtype_data = {
 	.intctrl = mx1_intctrl,
 	.prepare_message = mx1_prepare_message,
-	.config = mx1_config,
+	.prepare_transfer = mx1_prepare_transfer,
 	.trigger = mx1_trigger,
 	.rx_available = mx1_rx_available,
 	.reset = mx1_reset,
@@ -913,7 +918,7 @@ static struct spi_imx_devtype_data imx1_cspi_devtype_data = {
 static struct spi_imx_devtype_data imx21_cspi_devtype_data = {
 	.intctrl = mx21_intctrl,
 	.prepare_message = mx21_prepare_message,
-	.config = mx21_config,
+	.prepare_transfer = mx21_prepare_transfer,
 	.trigger = mx21_trigger,
 	.rx_available = mx21_rx_available,
 	.reset = mx21_reset,
@@ -928,7 +933,7 @@ static struct spi_imx_devtype_data imx27_cspi_devtype_data = {
 	/* i.mx27 cspi shares the functions with i.mx21 one */
 	.intctrl = mx21_intctrl,
 	.prepare_message = mx21_prepare_message,
-	.config = mx21_config,
+	.prepare_transfer = mx21_prepare_transfer,
 	.trigger = mx21_trigger,
 	.rx_available = mx21_rx_available,
 	.reset = mx21_reset,
@@ -942,7 +947,7 @@ static struct spi_imx_devtype_data imx27_cspi_devtype_data = {
 static struct spi_imx_devtype_data imx31_cspi_devtype_data = {
 	.intctrl = mx31_intctrl,
 	.prepare_message = mx31_prepare_message,
-	.config = mx31_config,
+	.prepare_transfer = mx31_prepare_transfer,
 	.trigger = mx31_trigger,
 	.rx_available = mx31_rx_available,
 	.reset = mx31_reset,
@@ -957,7 +962,7 @@ static struct spi_imx_devtype_data imx35_cspi_devtype_data = {
 	/* i.mx35 and later cspi shares the functions with i.mx31 one */
 	.intctrl = mx31_intctrl,
 	.prepare_message = mx31_prepare_message,
-	.config = mx31_config,
+	.prepare_transfer = mx31_prepare_transfer,
 	.trigger = mx31_trigger,
 	.rx_available = mx31_rx_available,
 	.reset = mx31_reset,
@@ -971,7 +976,7 @@ static struct spi_imx_devtype_data imx35_cspi_devtype_data = {
 static struct spi_imx_devtype_data imx51_ecspi_devtype_data = {
 	.intctrl = mx51_ecspi_intctrl,
 	.prepare_message = mx51_ecspi_prepare_message,
-	.config = mx51_ecspi_config,
+	.prepare_transfer = mx51_ecspi_prepare_transfer,
 	.trigger = mx51_ecspi_trigger,
 	.rx_available = mx51_ecspi_rx_available,
 	.reset = mx51_ecspi_reset,
@@ -987,7 +992,7 @@ static struct spi_imx_devtype_data imx51_ecspi_devtype_data = {
 static struct spi_imx_devtype_data imx53_ecspi_devtype_data = {
 	.intctrl = mx51_ecspi_intctrl,
 	.prepare_message = mx51_ecspi_prepare_message,
-	.config = mx51_ecspi_config,
+	.prepare_transfer = mx51_ecspi_prepare_transfer,
 	.trigger = mx51_ecspi_trigger,
 	.rx_available = mx51_ecspi_rx_available,
 	.reset = mx51_ecspi_reset,
@@ -1230,7 +1235,7 @@ static int spi_imx_setupxfer(struct spi_device *spi,
 		spi_imx->slave_burst = t->len;
 	}
 
-	spi_imx->devtype_data->config(spi);
+	spi_imx->devtype_data->prepare_transfer(spi_imx, spi, t);
 
 	return 0;
 }
-- 
2.19.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 4/5] spi: imx: rename config callback and add useful parameters

[Robin Gong]

> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Sent: 2018年11月30日 14:47
> To: Mark Brown <broonie@kernel.org>; Robin Gong <yibin.gong@nxp.com>
> Cc: Marek Vasut <marex@denx.de>; dl-linux-imx <linux-imx@nxp.com>;
> kernel@pengutronix.de; linux-spi@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org
> Subject: [PATCH v3 4/5] spi: imx: rename config callback and add useful
> parameters
> 
> The config callback is called once per transfer while some things can (and
> should) be done on a per message manner. To have unambiguous naming in the
> end include "transfer" in the callback's name and rename the implementations
> accordingly. Also pass the driver struct and transfer which allows further
> simplifications in the following patch.
> 
> There is no change in behavior intended here.
> 
> Reviewed-by: Marek Vasut <marex@denx.de>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/spi/spi-imx.c | 39 ++++++++++++++++++++++-----------------
>  1 file changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c index
> d954b6d958c2..72c879226abd 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -60,7 +60,8 @@ struct spi_imx_data;
>  struct spi_imx_devtype_data {
>  	void (*intctrl)(struct spi_imx_data *, int);
>  	int (*prepare_message)(struct spi_imx_data *, struct spi_message *);
> -	int (*config)(struct spi_device *);
> +	int (*prepare_transfer)(struct spi_imx_data *, struct spi_device *,
> +				struct spi_transfer *);
>  	void (*trigger)(struct spi_imx_data *);
>  	int (*rx_available)(struct spi_imx_data *);
>  	void (*reset)(struct spi_imx_data *);
> @@ -556,9 +557,10 @@ static int mx51_ecspi_prepare_message(struct
> spi_imx_data *spi_imx,
>  	return 0;
>  }
> 
> -static int mx51_ecspi_config(struct spi_device *spi)
> +static int mx51_ecspi_prepare_transfer(struct spi_imx_data *spi_imx,
> +				       struct spi_device *spi,
> +				       struct spi_transfer *t)
>  {
> -	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
Since spi_imx could be get from spi as before, why not remove the parameter *spi_imx for
prepare_transfer() function?
>  	u32 ctrl = readl(spi_imx->base + MX51_ECSPI_CTRL);
>  	u32 clk = spi_imx->speed_hz, delay;
> 
> @@ -685,9 +687,10 @@ static int mx31_prepare_message(struct
> spi_imx_data *spi_imx,
>  	return 0;
>  }
> 
> -static int mx31_config(struct spi_device *spi)
> +static int mx31_prepare_transfer(struct spi_imx_data *spi_imx,
> +				 struct spi_device *spi,
> +				 struct spi_transfer *t)
>  {
> -	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
>  	unsigned int reg = MX31_CSPICTRL_ENABLE | MX31_CSPICTRL_MASTER;
>  	unsigned int clk;
> 
> @@ -789,9 +792,10 @@ static int mx21_prepare_message(struct
> spi_imx_data *spi_imx,
>  	return 0;
>  }
> 
> -static int mx21_config(struct spi_device *spi)
> +static int mx21_prepare_transfer(struct spi_imx_data *spi_imx,
> +				 struct spi_device *spi,
> +				 struct spi_transfer *t)
>  {
> -	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
>  	unsigned int reg = MX21_CSPICTRL_ENABLE | MX21_CSPICTRL_MASTER;
>  	unsigned int max = is_imx27_cspi(spi_imx) ? 16 : 18;
>  	unsigned int clk;
> @@ -864,9 +868,10 @@ static int mx1_prepare_message(struct
> spi_imx_data *spi_imx,
>  	return 0;
>  }
> 
> -static int mx1_config(struct spi_device *spi)
> +static int mx1_prepare_transfer(struct spi_imx_data *spi_imx,
> +				struct spi_device *spi,
> +				struct spi_transfer *t)
>  {
> -	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
>  	unsigned int reg = MX1_CSPICTRL_ENABLE | MX1_CSPICTRL_MASTER;
>  	unsigned int clk;
> 
> @@ -899,7 +904,7 @@ static void mx1_reset(struct spi_imx_data *spi_imx)
> static struct spi_imx_devtype_data imx1_cspi_devtype_data = {
>  	.intctrl = mx1_intctrl,
>  	.prepare_message = mx1_prepare_message,
> -	.config = mx1_config,
> +	.prepare_transfer = mx1_prepare_transfer,
>  	.trigger = mx1_trigger,
>  	.rx_available = mx1_rx_available,
>  	.reset = mx1_reset,
> @@ -913,7 +918,7 @@ static struct spi_imx_devtype_data
> imx1_cspi_devtype_data = {  static struct spi_imx_devtype_data
> imx21_cspi_devtype_data = {
>  	.intctrl = mx21_intctrl,
>  	.prepare_message = mx21_prepare_message,
> -	.config = mx21_config,
> +	.prepare_transfer = mx21_prepare_transfer,
>  	.trigger = mx21_trigger,
>  	.rx_available = mx21_rx_available,
>  	.reset = mx21_reset,
> @@ -928,7 +933,7 @@ static struct spi_imx_devtype_data
> imx27_cspi_devtype_data = {
>  	/* i.mx27 cspi shares the functions with i.mx21 one */
>  	.intctrl = mx21_intctrl,
>  	.prepare_message = mx21_prepare_message,
> -	.config = mx21_config,
> +	.prepare_transfer = mx21_prepare_transfer,
>  	.trigger = mx21_trigger,
>  	.rx_available = mx21_rx_available,
>  	.reset = mx21_reset,
> @@ -942,7 +947,7 @@ static struct spi_imx_devtype_data
> imx27_cspi_devtype_data = {  static struct spi_imx_devtype_data
> imx31_cspi_devtype_data = {
>  	.intctrl = mx31_intctrl,
>  	.prepare_message = mx31_prepare_message,
> -	.config = mx31_config,
> +	.prepare_transfer = mx31_prepare_transfer,
>  	.trigger = mx31_trigger,
>  	.rx_available = mx31_rx_available,
>  	.reset = mx31_reset,
> @@ -957,7 +962,7 @@ static struct spi_imx_devtype_data
> imx35_cspi_devtype_data = {
>  	/* i.mx35 and later cspi shares the functions with i.mx31 one */
>  	.intctrl = mx31_intctrl,
>  	.prepare_message = mx31_prepare_message,
> -	.config = mx31_config,
> +	.prepare_transfer = mx31_prepare_transfer,
>  	.trigger = mx31_trigger,
>  	.rx_available = mx31_rx_available,
>  	.reset = mx31_reset,
> @@ -971,7 +976,7 @@ static struct spi_imx_devtype_data
> imx35_cspi_devtype_data = {  static struct spi_imx_devtype_data
> imx51_ecspi_devtype_data = {
>  	.intctrl = mx51_ecspi_intctrl,
>  	.prepare_message = mx51_ecspi_prepare_message,
> -	.config = mx51_ecspi_config,
> +	.prepare_transfer = mx51_ecspi_prepare_transfer,
>  	.trigger = mx51_ecspi_trigger,
>  	.rx_available = mx51_ecspi_rx_available,
>  	.reset = mx51_ecspi_reset,
> @@ -987,7 +992,7 @@ static struct spi_imx_devtype_data
> imx51_ecspi_devtype_data = {  static struct spi_imx_devtype_data
> imx53_ecspi_devtype_data = {
>  	.intctrl = mx51_ecspi_intctrl,
>  	.prepare_message = mx51_ecspi_prepare_message,
> -	.config = mx51_ecspi_config,
> +	.prepare_transfer = mx51_ecspi_prepare_transfer,
>  	.trigger = mx51_ecspi_trigger,
>  	.rx_available = mx51_ecspi_rx_available,
>  	.reset = mx51_ecspi_reset,
> @@ -1230,7 +1235,7 @@ static int spi_imx_setupxfer(struct spi_device *spi,
>  		spi_imx->slave_burst = t->len;
>  	}
> 
> -	spi_imx->devtype_data->config(spi);
> +	spi_imx->devtype_data->prepare_transfer(spi_imx, spi, t);
> 
>  	return 0;
>  }
> --
> 2.19.2

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 4/5] spi: imx: rename config callback and add useful parameters

[Uwe Kleine-König]

On Mon, Dec 03, 2018 at 08:09:59AM +0000, Robin Gong wrote:
> 
> 
> > -----Original Message-----
> > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Sent: 2018年11月30日 14:47
> > To: Mark Brown <broonie@kernel.org>; Robin Gong <yibin.gong@nxp.com>
> > Cc: Marek Vasut <marex@denx.de>; dl-linux-imx <linux-imx@nxp.com>;
> > kernel@pengutronix.de; linux-spi@vger.kernel.org;
> > linux-arm-kernel@lists.infradead.org
> > Subject: [PATCH v3 4/5] spi: imx: rename config callback and add useful
> > parameters
> > 
> > The config callback is called once per transfer while some things can (and
> > should) be done on a per message manner. To have unambiguous naming in the
> > end include "transfer" in the callback's name and rename the implementations
> > accordingly. Also pass the driver struct and transfer which allows further
> > simplifications in the following patch.
> > 
> > There is no change in behavior intended here.
> > 
> > Reviewed-by: Marek Vasut <marex@denx.de>
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/spi/spi-imx.c | 39 ++++++++++++++++++++++-----------------
> >  1 file changed, 22 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c index
> > d954b6d958c2..72c879226abd 100644
> > --- a/drivers/spi/spi-imx.c
> > +++ b/drivers/spi/spi-imx.c
> > @@ -60,7 +60,8 @@ struct spi_imx_data;
> >  struct spi_imx_devtype_data {
> >  	void (*intctrl)(struct spi_imx_data *, int);
> >  	int (*prepare_message)(struct spi_imx_data *, struct spi_message *);
> > -	int (*config)(struct spi_device *);
> > +	int (*prepare_transfer)(struct spi_imx_data *, struct spi_device *,
> > +				struct spi_transfer *);
> >  	void (*trigger)(struct spi_imx_data *);
> >  	int (*rx_available)(struct spi_imx_data *);
> >  	void (*reset)(struct spi_imx_data *);
> > @@ -556,9 +557,10 @@ static int mx51_ecspi_prepare_message(struct
> > spi_imx_data *spi_imx,
> >  	return 0;
> >  }
> > 
> > -static int mx51_ecspi_config(struct spi_device *spi)
> > +static int mx51_ecspi_prepare_transfer(struct spi_imx_data *spi_imx,
> > +				       struct spi_device *spi,
> > +				       struct spi_transfer *t)
> >  {
> > -	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
> Since spi_imx could be get from spi as before, why not remove the parameter *spi_imx for
> prepare_transfer() function?

My rationale to add spi_imx was that the caller already has it and this
function needs it. So it seems natural for me to pass it as argument.
Also the prepare_message callback gets the driver data so it also looks
consistent to me. Also it might save a few cycles to follow the pointer
chain (spi->master->dev.driver_data) if the compiler doesn't notice it
already has the requested value. But I don't feel like arguing and
didn't check if it makes a difference here in the binary.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 5/5] spi: imx: drop useless member speed_hz from driver data struct

[Uwe Kleine-König]

The driver data's member variable just caches the transfer's speed_hz
member. All users of the former now have access directly to the latter.
So fix them to use the uncached value and remove the cache.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/spi/spi-imx.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 72c879226abd..6ec647bbba77 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -87,7 +87,6 @@ struct spi_imx_data {
 	unsigned long spi_clk;
 	unsigned int spi_bus_clk;
 
-	unsigned int speed_hz;
 	unsigned int bits_per_word;
 	unsigned int spi_drctl;
 
@@ -562,7 +561,7 @@ static int mx51_ecspi_prepare_transfer(struct spi_imx_data *spi_imx,
 				       struct spi_transfer *t)
 {
 	u32 ctrl = readl(spi_imx->base + MX51_ECSPI_CTRL);
-	u32 clk = spi_imx->speed_hz, delay;
+	u32 clk = t->speed_hz, delay;
 
 	/* Clear BL field and set the right value */
 	ctrl &= ~MX51_ECSPI_CTRL_BL_MASK;
@@ -576,7 +575,7 @@ static int mx51_ecspi_prepare_transfer(struct spi_imx_data *spi_imx,
 	/* set clock speed */
 	ctrl &= ~(0xf << MX51_ECSPI_CTRL_POSTDIV_OFFSET |
 		  0xf << MX51_ECSPI_CTRL_PREDIV_OFFSET);
-	ctrl |= mx51_ecspi_clkdiv(spi_imx, spi_imx->speed_hz, &clk);
+	ctrl |= mx51_ecspi_clkdiv(spi_imx, t->speed_hz, &clk);
 	spi_imx->spi_bus_clk = clk;
 
 	if (spi_imx->usedma)
@@ -694,7 +693,7 @@ static int mx31_prepare_transfer(struct spi_imx_data *spi_imx,
 	unsigned int reg = MX31_CSPICTRL_ENABLE | MX31_CSPICTRL_MASTER;
 	unsigned int clk;
 
-	reg |= spi_imx_clkdiv_2(spi_imx->spi_clk, spi_imx->speed_hz, &clk) <<
+	reg |= spi_imx_clkdiv_2(spi_imx->spi_clk, t->speed_hz, &clk) <<
 		MX31_CSPICTRL_DR_SHIFT;
 	spi_imx->spi_bus_clk = clk;
 
@@ -800,7 +799,7 @@ static int mx21_prepare_transfer(struct spi_imx_data *spi_imx,
 	unsigned int max = is_imx27_cspi(spi_imx) ? 16 : 18;
 	unsigned int clk;
 
-	reg |= spi_imx_clkdiv_1(spi_imx->spi_clk, spi_imx->speed_hz, max, &clk)
+	reg |= spi_imx_clkdiv_1(spi_imx->spi_clk, t->speed_hz, max, &clk)
 		<< MX21_CSPICTRL_DR_SHIFT;
 	spi_imx->spi_bus_clk = clk;
 
@@ -875,7 +874,7 @@ static int mx1_prepare_transfer(struct spi_imx_data *spi_imx,
 	unsigned int reg = MX1_CSPICTRL_ENABLE | MX1_CSPICTRL_MASTER;
 	unsigned int clk;
 
-	reg |= spi_imx_clkdiv_2(spi_imx->spi_clk, spi_imx->speed_hz, &clk) <<
+	reg |= spi_imx_clkdiv_2(spi_imx->spi_clk, t->speed_hz, &clk) <<
 		MX1_CSPICTRL_DR_SHIFT;
 	spi_imx->spi_bus_clk = clk;
 
@@ -1194,7 +1193,6 @@ static int spi_imx_setupxfer(struct spi_device *spi,
 		return 0;
 
 	spi_imx->bits_per_word = t->bits_per_word;
-	spi_imx->speed_hz = t->speed_hz;
 
 	/*
 	 * Initialize the functions for transfer. To transfer non byte-aligned
-- 
2.19.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 5/5] spi: imx: drop useless member speed_hz from driver data struct

[Trent Piepho]

On Fri, 2018-11-30 at 07:47 +0100, Uwe Kleine-König wrote:
> The driver data's member variable just caches the transfer's speed_hz
> member. All users of the former now have access directly to the latter.
> So fix them to use the uncached value and remove the cache.

What should have been done with this is to compare the next transfer's
speed_hz to this cached value, and not preprogram the clock if it has
not changed.  Since usually it doesn't change between transfers.

spi_bus_clk caches the actual spi bus clock, which is often not exactly
the same as the transfer speed, so using it compare against the next
transfer doesn't work as well.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 5/5] spi: imx: drop useless member speed_hz from driver data struct

[Uwe Kleine-König]

Hello Trent,

On Fri, Nov 30, 2018 at 05:21:40PM +0000, Trent Piepho wrote:
> On Fri, 2018-11-30 at 07:47 +0100, Uwe Kleine-König wrote:
> > The driver data's member variable just caches the transfer's speed_hz
> > member. All users of the former now have access directly to the latter.
> > So fix them to use the uncached value and remove the cache.
> 
> What should have been done with this is to compare the next transfer's
> speed_hz to this cached value, and not preprogram the clock if it has
> not changed.  Since usually it doesn't change between transfers.

Hmm, yes, this could be done, but I'm not sure the additional effort
brings a performance advantage. If you have the energy to do test that,
feel free to implement it, preferably on top of my cleanup commit.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v3 5/5] spi: imx: drop useless member speed_hz from driver data struct

[Trent Piepho]

On Fri, 2018-11-30 at 18:50 +0100, Uwe Kleine-König wrote:
> Hello Trent,
> 
> On Fri, Nov 30, 2018 at 05:21:40PM +0000, Trent Piepho wrote:
> > On Fri, 2018-11-30 at 07:47 +0100, Uwe Kleine-König wrote:
> > > The driver data's member variable just caches the transfer's speed_hz
> > > member. All users of the former now have access directly to the latter.
> > > So fix them to use the uncached value and remove the cache.
> > 
> > What should have been done with this is to compare the next transfer's
> > speed_hz to this cached value, and not preprogram the clock if it has
> > not changed.  Since usually it doesn't change between transfers.
> 
> Hmm, yes, this could be done, but I'm not sure the additional effort
> brings a performance advantage. If you have the energy to do test that,
> feel free to implement it, preferably on top of my cleanup commit.

I did this for the imx23 spi driver[1], commit a560943ead, and it more
than double the transfer speed.  That's a much slower CPU than
imx6/7/8, but it could still be a pretty good improvement on something
slow like imx27.

[1] Confusingly, imx23 and imx28 are totally different chips that other
imx chips, like imx21 and imx27.  The former are "mxs" designs that
Freescale bought from someone else (I think ST-Micro?).
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 5/5] spi: imx: drop useless member speed_hz from driver data struct

[Uwe Kleine-König]

Hello,

On Fri, Nov 30, 2018 at 06:10:26PM +0000, Trent Piepho wrote:
> [1] Confusingly, imx23 and imx28 are totally different chips that other
> imx chips, like imx21 and imx27.  The former are "mxs" designs that
> Freescale bought from someone else (I think ST-Micro?).

FTR: the mxs family includes parts that were bought by Freescale from
Sigmatel.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v3 0/5] spi: imx: Fix polarity switching for mx51-ecspi

[Uwe Kleine-König]

Hello Mark,

On Fri, Nov 30, 2018 at 07:47:04AM +0100, Uwe Kleine-König wrote:
> compared to v2 sent with Message-Id:
> 20181123085158.24753-1-u.kleine-koenig@pengutronix.de I squashed in the
> change suggested by Robin (with s/filed/field/). Thanks for testing and
> feedback.

In my eyes this series is ready to go in. The feedback I got so far was
only that there could be done still more. And given that patch 2 is a
fix I'd like to see this in 4.21.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v3 0/5] spi: imx: Fix polarity switching for mx51-ecspi

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 350 bytes --]

On Mon, Dec 10, 2018 at 10:52:41PM +0100, Uwe Kleine-König wrote:

> In my eyes this series is ready to go in. The feedback I got so far was
> only that there could be done still more. And given that patch 2 is a
> fix I'd like to see this in 4.21.

It looked like there was an unanswered question about if patch 2 was
needed or not?  Marek?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 0/5] spi: imx: Fix polarity switching for mx51-ecspi

[Uwe Kleine-König]

Hello Mark,

On Tue, Dec 11, 2018 at 01:14:28AM +0000, Mark Brown wrote:
> On Mon, Dec 10, 2018 at 10:52:41PM +0100, Uwe Kleine-König wrote:
> 
> > In my eyes this series is ready to go in. The feedback I got so far was
> > only that there could be done still more. And given that patch 2 is a
> > fix I'd like to see this in 4.21.
> 
> It looked like there was an unanswered question about if patch 2 was
> needed or not?  Marek?

I cannot find an unanswered question. There is a workaround that Marek
introduced in 6fd8b8503a0dcf66510314dc054745087ae89f94 that might not be
needed any more with my patch 2. So it's the other way round: If Marek's
problem is fixed by patch 2, my patch is the better one. I have problems
with SCK changing polarity when CS is already asserted in the presence
of Marek's change. After my change they are gone. Even if this
workaround is superflous now, it doesn't hurt and IMHO the question if
it is needed or not should not delay my fix.

If I miss something, can you please point out the message-id?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v3 0/5] spi: imx: Fix polarity switching for mx51-ecspi

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1228 bytes --]

On Tue, Dec 11, 2018 at 08:13:00AM +0100, Uwe Kleine-König wrote:
> On Tue, Dec 11, 2018 at 01:14:28AM +0000, Mark Brown wrote:

> > It looked like there was an unanswered question about if patch 2 was
> > needed or not?  Marek?

> I cannot find an unanswered question. There is a workaround that Marek
> introduced in 6fd8b8503a0dcf66510314dc054745087ae89f94 that might not be
> needed any more with my patch 2. So it's the other way round: If Marek's
> problem is fixed by patch 2, my patch is the better one. I have problems
> with SCK changing polarity when CS is already asserted in the presence
> of Marek's change. After my change they are gone. Even if this
> workaround is superflous now, it doesn't hurt and IMHO the question if
> it is needed or not should not delay my fix.

That was the unanswered question.

Please include human readable descriptions of things like commits and
issues being discussed in e-mail in your mails, this makes them much
easier for humans to read especially when they have no internet access.
I do frequently catch up on my mail on flights or while otherwise
travelling so this is even more pressing for me than just being about
making things a bit easier to read.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 0/5] spi: imx: Fix polarity switching for mx51-ecspi

[Uwe Kleine-König]

On Tue, Dec 11, 2018 at 11:56:01AM +0000, Mark Brown wrote:
> On Tue, Dec 11, 2018 at 08:13:00AM +0100, Uwe Kleine-König wrote:
> > On Tue, Dec 11, 2018 at 01:14:28AM +0000, Mark Brown wrote:
> 
> > > It looked like there was an unanswered question about if patch 2 was
> > > needed or not?  Marek?
> 
> > I cannot find an unanswered question. There is a workaround that Marek
> > introduced in 6fd8b8503a0dcf66510314dc054745087ae89f94 that might not be
> > needed any more with my patch 2. So it's the other way round: If Marek's
> > problem is fixed by patch 2, my patch is the better one. I have problems
> > with SCK changing polarity when CS is already asserted in the presence
> > of Marek's change. After my change they are gone. Even if this
> > workaround is superflous now, it doesn't hurt and IMHO the question if
> > it is needed or not should not delay my fix.
> 
> That was the unanswered question.

FTR: your reference to an "unanswered question" wasn't optimal either. I
had to search my inbox for a while to determine what you could have
meant :-)

> Please include human readable descriptions of things like commits and
> issues being discussed in e-mail in your mails, this makes them much
> easier for humans to read especially when they have no internet access.

I'm always willing to learn how to make my mails more cooperative.

I'd hope that even when offline you have a linux.git at hand to resolve
what I meant with "6fd8b8503a0dcf66510314dc054745087ae89f94". Apart from
this unannotated commit hash it's not clear to me what else could have
been improved here.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v3 0/5] spi: imx: Fix polarity switching for mx51-ecspi

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1291 bytes --]

On Tue, Dec 11, 2018 at 09:11:26PM +0100, Uwe Kleine-König wrote:
> On Tue, Dec 11, 2018 at 11:56:01AM +0000, Mark Brown wrote:

> > That was the unanswered question.

> FTR: your reference to an "unanswered question" wasn't optimal either. I
> had to search my inbox for a while to determine what you could have
> meant :-)

At the time I wrote that I didn't actually know, I just knew that I'd
remembered the subthread ended with a question to Marek about if this
was needed.

> > Please include human readable descriptions of things like commits and
> > issues being discussed in e-mail in your mails, this makes them much
> > easier for humans to read especially when they have no internet access.

> I'm always willing to learn how to make my mails more cooperative.

> I'd hope that even when offline you have a linux.git at hand to resolve
> what I meant with "6fd8b8503a0dcf66510314dc054745087ae89f94". Apart from
> this unannotated commit hash it's not clear to me what else could have
> been improved here.

Including the title of the commit.  It's not just about offline usage,
it's also about making the mail easier to read - having to stop and use
another tool to understand what's being talked about is a blocker to the
flow of reading the message.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 0/5] spi: imx: Fix polarity switching for mx51-ecspi

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1063 bytes --]

On Tue, Dec 11, 2018 at 08:40:47PM +0000, Mark Brown wrote:
> On Tue, Dec 11, 2018 at 09:11:26PM +0100, Uwe Kleine-König wrote:

> > FTR: your reference to an "unanswered question" wasn't optimal either. I
> > had to search my inbox for a while to determine what you could have
> > meant :-)

> At the time I wrote that I didn't actually know, I just knew that I'd
> remembered the subthread ended with a question to Marek about if this
> was needed.

Just to expand on this a bit: the way I'm working here is that I'm
remembering if there's some reason I'm expecting some additional
replies, either because there's someone likely to do review on the
relevant code, there was some specific question I remember or because
there was a discussion that seemed to peter out without conclusion
between people who are likely to respond.  If something like that is
happening I'll leave a bit longer for replies than I might otherwise
(conversely if it's something where it's very unlikely that there will
be any comment I'll tend to leave less time).

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel