suggest 00b80ac93553 ("spi: imx: mx51-ecspi: Move some initialisation to prepare_message hook.") for stable backports
Hello,
even though the subject sounds harmless it fixes a real bug.
(Yes, I'm aware that commit isn't in Linus' tree yet, but I assume your tracking of patches targetting stable is better than mine, so I didn't wait :-)
For backporting you also need its parent commit (i.e. e697271c4e29 ("spi: imx: add a device specific prepare_message callback")). I have a local backport to v4.14 here which isn't entirely trivial. So just tell me if you need help.
Best regards Uwe
On Thu, Dec 20, 2018 at 03:43:31PM +0100, Uwe Kleine-König wrote:
even though the subject sounds harmless it fixes a real bug.
(Yes, I'm aware that commit isn't in Linus' tree yet, but I assume your tracking of patches targetting stable is better than mine, so I didn't wait :-)
For backporting you also need its parent commit (i.e. e697271c4e29 ("spi: imx: add a device specific prepare_message callback")). I have a local backport to v4.14 here which isn't entirely trivial. So just tell me if you need help.
This makes sense to me as a backport, an oversight on my part.
On Thu, Dec 20, 2018 at 02:55:31PM +0000, Mark Brown wrote:
On Thu, Dec 20, 2018 at 03:43:31PM +0100, Uwe Kleine-König wrote:
even though the subject sounds harmless it fixes a real bug.
(Yes, I'm aware that commit isn't in Linus' tree yet, but I assume your tracking of patches targetting stable is better than mine, so I didn't wait :-)
For backporting you also need its parent commit (i.e. e697271c4e29 ("spi: imx: add a device specific prepare_message callback")). I have a local backport to v4.14 here which isn't entirely trivial. So just tell me if you need help.
This makes sense to me as a backport, an oversight on my part.
I'll happily take your backport here and an ack on it from Mark :)
-- Thanks, Sasha
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 Signed-off-by: Mark Brown broonie@kernel.org [ukleinek: backport to v4.14.x] 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 d51ca243a028..3fdb0652429b 100644 --- a/drivers/spi/spi-imx.c +++ b/drivers/spi/spi-imx.c @@ -72,6 +72,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 *); @@ -439,6 +440,12 @@ static void mx51_ecspi_trigger(struct spi_imx_data *spi_imx) writel(reg, 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); @@ -599,6 +606,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); @@ -695,6 +708,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); @@ -764,6 +783,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); @@ -798,6 +823,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, @@ -810,6 +836,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, @@ -823,6 +850,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, @@ -835,6 +863,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, @@ -848,6 +877,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, @@ -860,6 +890,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, @@ -872,6 +903,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, @@ -1310,7 +1342,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
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 Signed-off-by: Mark Brown broonie@kernel.org [ukleinek: backport to v4.14.x] Signed-off-by: Uwe Kleine-König u.kleine-koenig@pengutronix.de --- drivers/spi/spi-imx.c | 59 ++++++++++++++++++++++++++----------------- 1 file changed, 36 insertions(+), 23 deletions(-)
diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c index 3fdb0652429b..df18d07d544d 100644 --- a/drivers/spi/spi-imx.c +++ b/drivers/spi/spi-imx.c @@ -443,14 +443,9 @@ static void mx51_ecspi_trigger(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);
/* @@ -468,14 +463,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);
- ctrl |= (spi_imx->bits_per_word - 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 + testreg &= ~MX51_ECSPI_TESTREG_LBC; + writel(testreg, spi_imx->base + MX51_ECSPI_TESTREG);
cfg |= MX51_ECSPI_CONFIG_SBBCTRL(spi->chip_select);
@@ -491,26 +493,38 @@ 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);
+ writel(cfg, spi_imx->base + MX51_ECSPI_CONFIG); + + return 0; +} + +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; + ctrl |= (spi_imx->bits_per_word - 1) << MX51_ECSPI_CTRL_BL_OFFSET; + + /* 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;
- /* CTRL register always go first to bring out controller from reset */ writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
- reg = readl(spi_imx->base + MX51_ECSPI_TESTREG); - if (spi->mode & SPI_LOOP) - reg |= MX51_ECSPI_TESTREG_LBC; - else - reg &= ~MX51_ECSPI_TESTREG_LBC; - writel(reg, spi_imx->base + MX51_ECSPI_TESTREG); - - writel(cfg, spi_imx->base + MX51_ECSPI_CONFIG); - /* * Wait until the changes in the configuration register CONFIGREG * propagate into the hardware. It takes exactly one tick of the @@ -532,7 +546,6 @@ static int mx51_ecspi_config(struct spi_device *spi) * Configure the DMA register: setup the watermark * and enable DMA request. */ - writel(MX51_ECSPI_DMA_RX_WML(spi_imx->wml) | MX51_ECSPI_DMA_TX_WML(spi_imx->wml) | MX51_ECSPI_DMA_RXT_WML(spi_imx->wml) |
On Sun, Dec 23, 2018 at 10:21:22PM +0100, Uwe Kleine-König wrote:
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 Signed-off-by: Mark Brown broonie@kernel.org [ukleinek: backport to v4.14.x] Signed-off-by: Uwe Kleine-König u.kleine-koenig@pengutronix.de
Queued both for 4.14, thank you.
-- Thanks, Sasha
linux-stable-mirror@lists.linaro.org
-
Mark Brown -
Sasha Levin -
Uwe Kleine-König