* [PATCH 0/2] spi: imx: don't loop endlessly @ 2017-08-02 19:39 Uwe Kleine-König 2017-08-02 19:39 ` [PATCH 1/2] spi: imx: add error checking Uwe Kleine-König ` (3 more replies) 0 siblings, 4 replies; 11+ messages in thread From: Uwe Kleine-König @ 2017-08-02 19:39 UTC (permalink / raw) To: barebox Hello, during bringup of an i.MX7 board I am faced with cspi_2_3_xchg_single not returning. I don't know yet why this happens, but with this patch set it at least doesn't block barebox. I didn't test on a working board, maybe the timeout (10 µs) I chose is too tight? Best regards Uwe Uwe Kleine-König (2): spi: imx: add error checking spi: imx: add timeout to xchg_single drivers/spi/imx_spi.c | 105 ++++++++++++++++++++++++++++++++++---------------- 1 file changed, 72 insertions(+), 33 deletions(-) -- 2.11.0 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] spi: imx: add error checking 2017-08-02 19:39 [PATCH 0/2] spi: imx: don't loop endlessly Uwe Kleine-König @ 2017-08-02 19:39 ` Uwe Kleine-König 2017-08-02 19:39 ` [PATCH 2/2] spi: imx: add timeout to xchg_single Uwe Kleine-König ` (2 subsequent siblings) 3 siblings, 0 replies; 11+ messages in thread From: Uwe Kleine-König @ 2017-08-02 19:39 UTC (permalink / raw) To: barebox This makes it possible that the xchg_single callbacks return an error code that is then passed to the caller of spi_transfer. There is no change in behaviour intended as up to now the callbacks always return 0. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/spi/imx_spi.c | 77 ++++++++++++++++++++++++++++++++------------------- 1 file changed, 48 insertions(+), 29 deletions(-) diff --git a/drivers/spi/imx_spi.c b/drivers/spi/imx_spi.c index 876699a02da1..97a2091969dd 100644 --- a/drivers/spi/imx_spi.c +++ b/drivers/spi/imx_spi.c @@ -38,8 +38,8 @@ struct imx_spi { void __iomem *regs; struct clk *clk; - unsigned int (*xchg_single)(struct imx_spi *imx, u32 data); - void (*do_transfer)(struct spi_device *spi); + int (*xchg_single)(struct imx_spi *imx, u32 txdata, u32 *rxdata); + int (*do_transfer)(struct spi_device *spi); void (*chipselect)(struct spi_device *spi, int active); const void *tx_buf; @@ -49,8 +49,8 @@ struct imx_spi { }; struct spi_imx_devtype_data { - unsigned int (*xchg_single)(struct imx_spi *imx, u32 data); - void (*do_transfer)(struct spi_device *spi); + int (*xchg_single)(struct imx_spi *imx, u32 txdata, u32 *rxdata); + int (*do_transfer)(struct spi_device *spi); void (*chipselect)(struct spi_device *spi, int active); void (*init)(struct imx_spi *imx); }; @@ -86,13 +86,13 @@ static unsigned int imx_spi_maybe_reverse_bits(struct spi_device *spi, unsigned return result; } -static unsigned int cspi_0_0_xchg_single(struct imx_spi *imx, unsigned int data) +static int cspi_0_0_xchg_single(struct imx_spi *imx, u32 txdata, u32 *rxdata) { void __iomem *base = imx->regs; unsigned int cfg_reg = readl(base + CSPI_0_0_CTRL); - writel(data, base + CSPI_0_0_TXDATA); + writel(txdata, base + CSPI_0_0_TXDATA); cfg_reg |= CSPI_0_0_CTRL_XCH; @@ -100,7 +100,9 @@ static unsigned int cspi_0_0_xchg_single(struct imx_spi *imx, unsigned int data) while (!(readl(base + CSPI_0_0_INT) & CSPI_0_0_STAT_RR)); - return readl(base + CSPI_0_0_RXDATA); + *rxdata = readl(base + CSPI_0_0_RXDATA); + + return 0; } static void cspi_0_0_chipselect(struct spi_device *spi, int is_active) @@ -152,13 +154,13 @@ static void cspi_0_0_init(struct imx_spi *imx) } while (readl(base + CSPI_0_0_RESET) & CSPI_0_0_RESET_START); } -static unsigned int cspi_0_7_xchg_single(struct imx_spi *imx, unsigned int data) +static int cspi_0_7_xchg_single(struct imx_spi *imx, u32 txdata, u32 *rxdata) { void __iomem *base = imx->regs; unsigned int cfg_reg = readl(base + CSPI_0_7_CTRL); - writel(data, base + CSPI_0_7_TXDATA); + writel(txdata, base + CSPI_0_7_TXDATA); cfg_reg |= CSPI_0_7_CTRL_XCH; @@ -167,7 +169,8 @@ static unsigned int cspi_0_7_xchg_single(struct imx_spi *imx, unsigned int data) while (!(readl(base + CSPI_0_7_STAT) & CSPI_0_7_STAT_RR)) ; - return readl(base + CSPI_0_7_RXDATA); + *rxdata = readl(base + CSPI_0_7_RXDATA); + return 0; } /* MX1, MX31, MX35, MX51 CSPI */ @@ -242,15 +245,17 @@ static void cspi_0_7_init(struct imx_spi *imx) readl(base + CSPI_0_7_RXDATA); } -static unsigned int cspi_2_3_xchg_single(struct imx_spi *imx, unsigned int data) +static int cspi_2_3_xchg_single(struct imx_spi *imx, u32 txdata, u32 *rxdata) { void __iomem *base = imx->regs; - writel(data, base + CSPI_2_3_TXDATA); + writel(txdata, base + CSPI_2_3_TXDATA); while (!(readl(base + CSPI_2_3_STAT) & CSPI_2_3_STAT_RR)); - return readl(base + CSPI_2_3_RXDATA); + *rxdata = readl(base + CSPI_2_3_RXDATA); + + return 0; } static unsigned int cspi_2_3_clkdiv(unsigned int fin, unsigned int fspi) @@ -336,16 +341,20 @@ static void cspi_2_3_chipselect(struct spi_device *spi, int is_active) gpio_set_value(gpio, gpio_cs); } -static u32 imx_xchg_single(struct spi_device *spi, u32 tx_val) +static int imx_xchg_single(struct spi_device *spi, u32 tx_val, u32* rx_val) { - u32 rx_val; struct imx_spi *imx = container_of(spi->master, struct imx_spi, master); - + u32 local_rx_val; + int ret; tx_val = imx_spi_maybe_reverse_bits(spi, tx_val); - rx_val = imx->xchg_single(imx, tx_val); + ret = imx->xchg_single(imx, tx_val, &local_rx_val); + if (ret) + return ret; - return imx_spi_maybe_reverse_bits(spi, rx_val); + *rx_val = imx_spi_maybe_reverse_bits(spi, local_rx_val); + + return 0; } static void cspi_2_3_init(struct imx_spi *imx) @@ -355,18 +364,21 @@ static void cspi_2_3_init(struct imx_spi *imx) writel(0, base + CSPI_2_3_CTRL); } -static void imx_spi_do_transfer(struct spi_device *spi) +static int imx_spi_do_transfer(struct spi_device *spi) { struct imx_spi *imx = container_of(spi->master, struct imx_spi, master); unsigned i; + u32 rx_val; + int ret; if (imx->bits_per_word <= 8) { const u8 *tx_buf = imx->tx_buf; u8 *rx_buf = imx->rx_buf; - u8 rx_val; for (i = 0; i < imx->xfer_len; i++) { - rx_val = imx_xchg_single(spi, tx_buf ? tx_buf[i] : 0); + ret = imx_xchg_single(spi, tx_buf ? tx_buf[i] : 0, &rx_val); + if (ret) + return ret; if (rx_buf) rx_buf[i] = rx_val; @@ -374,10 +386,11 @@ static void imx_spi_do_transfer(struct spi_device *spi) } else if (imx->bits_per_word <= 16) { const u16 *tx_buf = imx->tx_buf; u16 *rx_buf = imx->rx_buf; - u16 rx_val; for (i = 0; i < imx->xfer_len >> 1; i++) { - rx_val = imx_xchg_single(spi, tx_buf ? tx_buf[i] : 0); + ret = imx_xchg_single(spi, tx_buf ? tx_buf[i] : 0, &rx_val); + if (ret) + return ret; if (rx_buf) rx_buf[i] = rx_val; @@ -385,15 +398,18 @@ static void imx_spi_do_transfer(struct spi_device *spi) } else if (imx->bits_per_word <= 32) { const u32 *tx_buf = imx->tx_buf; u32 *rx_buf = imx->rx_buf; - u32 rx_val; for (i = 0; i < imx->xfer_len >> 2; i++) { - rx_val = imx_xchg_single(spi, tx_buf ? tx_buf[i] : 0); + ret = imx_xchg_single(spi, tx_buf ? tx_buf[i] : 0, &rx_val); + if (ret) + return ret; if (rx_buf) rx_buf[i] = rx_val; } } + + return 0; } static int cspi_2_3_xchg_burst(struct spi_device *spi) @@ -448,7 +464,7 @@ static int cspi_2_3_xchg_burst(struct spi_device *spi) return now; } -static void cspi_2_3_do_transfer(struct spi_device *spi) +static int cspi_2_3_do_transfer(struct spi_device *spi) { struct imx_spi *imx = container_of(spi->master, struct imx_spi, master); u32 ctrl; @@ -457,14 +473,14 @@ static void cspi_2_3_do_transfer(struct spi_device *spi) while (cspi_2_3_xchg_burst(spi) > 0); if (!imx->xfer_len) - return; + return 0; ctrl = readl(imx->regs + CSPI_2_3_CTRL); ctrl &= ~(0xfff << CSPI_2_3_CTRL_BL_OFFSET); ctrl |= (spi->bits_per_word - 1) << CSPI_2_3_CTRL_BL_OFFSET; writel(ctrl, imx->regs + CSPI_2_3_CTRL); - imx_spi_do_transfer(spi); + return imx_spi_do_transfer(spi); } static int imx_spi_transfer(struct spi_device *spi, struct spi_message *mesg) @@ -473,6 +489,7 @@ static int imx_spi_transfer(struct spi_device *spi, struct spi_message *mesg) struct spi_transfer *t; unsigned int cs_change; const int nsecs = 50; + int ret; imx->chipselect(spi, 1); @@ -494,7 +511,9 @@ static int imx_spi_transfer(struct spi_device *spi, struct spi_message *mesg) imx->rx_buf = t->rx_buf; imx->xfer_len = t->len; imx->bits_per_word = spi->bits_per_word; - imx->do_transfer(spi); + ret = imx->do_transfer(spi); + if (ret) + return ret; mesg->actual_length += t->len; -- 2.11.0 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] spi: imx: add timeout to xchg_single 2017-08-02 19:39 [PATCH 0/2] spi: imx: don't loop endlessly Uwe Kleine-König 2017-08-02 19:39 ` [PATCH 1/2] spi: imx: add error checking Uwe Kleine-König @ 2017-08-02 19:39 ` Uwe Kleine-König 2017-08-02 19:54 ` [PATCH 0/2] spi: imx: don't loop endlessly Sam Ravnborg 2017-09-29 18:08 ` Alexander Kurz 3 siblings, 0 replies; 11+ messages in thread From: Uwe Kleine-König @ 2017-08-02 19:39 UTC (permalink / raw) To: barebox If there is a problem STAT_RR might never be set which results in an endless loop. Break out after 10 µs with -ETIMEOUT instead. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/spi/imx_spi.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/drivers/spi/imx_spi.c b/drivers/spi/imx_spi.c index 97a2091969dd..78198798a57e 100644 --- a/drivers/spi/imx_spi.c +++ b/drivers/spi/imx_spi.c @@ -32,6 +32,9 @@ #include <linux/err.h> #include <clock.h> +/* time to wait for STAT_RR getting set */ +#define IMX_SPI_RR_TIMEOUT 10000 /* ns */ + struct imx_spi { struct spi_master master; int *cs_array; @@ -89,6 +92,7 @@ static unsigned int imx_spi_maybe_reverse_bits(struct spi_device *spi, unsigned static int cspi_0_0_xchg_single(struct imx_spi *imx, u32 txdata, u32 *rxdata) { void __iomem *base = imx->regs; + int ret; unsigned int cfg_reg = readl(base + CSPI_0_0_CTRL); @@ -98,7 +102,12 @@ static int cspi_0_0_xchg_single(struct imx_spi *imx, u32 txdata, u32 *rxdata) writel(cfg_reg, base + CSPI_0_0_CTRL); - while (!(readl(base + CSPI_0_0_INT) & CSPI_0_0_STAT_RR)); + ret = wait_on_timeout(IMX_SPI_RR_TIMEOUT, + readl(base + CSPI_0_0_INT) & CSPI_0_0_STAT_RR); + if (ret) { + dev_err(imx->master.dev, "Timeout waiting for received data\n"); + return ret; + } *rxdata = readl(base + CSPI_0_0_RXDATA); @@ -157,6 +166,7 @@ static void cspi_0_0_init(struct imx_spi *imx) static int cspi_0_7_xchg_single(struct imx_spi *imx, u32 txdata, u32 *rxdata) { void __iomem *base = imx->regs; + int ret; unsigned int cfg_reg = readl(base + CSPI_0_7_CTRL); @@ -166,8 +176,12 @@ static int cspi_0_7_xchg_single(struct imx_spi *imx, u32 txdata, u32 *rxdata) writel(cfg_reg, base + CSPI_0_7_CTRL); - while (!(readl(base + CSPI_0_7_STAT) & CSPI_0_7_STAT_RR)) - ; + ret = wait_on_timeout(IMX_SPI_RR_TIMEOUT, + readl(base + CSPI_0_7_STAT) & CSPI_0_7_STAT_RR); + if (ret) { + dev_err(imx->master.dev, "Timeout waiting for received data\n"); + return ret; + } *rxdata = readl(base + CSPI_0_7_RXDATA); return 0; @@ -248,10 +262,16 @@ static void cspi_0_7_init(struct imx_spi *imx) static int cspi_2_3_xchg_single(struct imx_spi *imx, u32 txdata, u32 *rxdata) { void __iomem *base = imx->regs; + int ret; writel(txdata, base + CSPI_2_3_TXDATA); - while (!(readl(base + CSPI_2_3_STAT) & CSPI_2_3_STAT_RR)); + ret = wait_on_timeout(IMX_SPI_RR_TIMEOUT, + readl(base + CSPI_2_3_STAT) & CSPI_2_3_STAT_RR); + if (ret) { + dev_err(imx->master.dev, "Timeout waiting for received data\n"); + return ret; + } *rxdata = readl(base + CSPI_2_3_RXDATA); -- 2.11.0 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] spi: imx: don't loop endlessly 2017-08-02 19:39 [PATCH 0/2] spi: imx: don't loop endlessly Uwe Kleine-König 2017-08-02 19:39 ` [PATCH 1/2] spi: imx: add error checking Uwe Kleine-König 2017-08-02 19:39 ` [PATCH 2/2] spi: imx: add timeout to xchg_single Uwe Kleine-König @ 2017-08-02 19:54 ` Sam Ravnborg 2017-08-02 20:19 ` Uwe Kleine-König 2017-09-29 18:08 ` Alexander Kurz 3 siblings, 1 reply; 11+ messages in thread From: Sam Ravnborg @ 2017-08-02 19:54 UTC (permalink / raw) To: Uwe Kleine-König; +Cc: barebox Hi Uwe. On Wed, Aug 02, 2017 at 09:39:05PM +0200, Uwe Kleine-König wrote: > Hello, > > during bringup of an i.MX7 board I am faced with cspi_2_3_xchg_single not > returning. I don't know yet why this happens, but with this patch set it at least > doesn't block barebox. Your description reminded me of a similar issue we hit with SPI transfer. Back then Sascha fixed it but with some open question marks. See 9da7e18573c725eaa6123c401df57d6a4f6a0ea2 ("spi: i.MX: reset controller on init") But as this is the latest patch to the imx_spi.c file you likely have already looked at it. But mentioned just in case.. Sam _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] spi: imx: don't loop endlessly 2017-08-02 19:54 ` [PATCH 0/2] spi: imx: don't loop endlessly Sam Ravnborg @ 2017-08-02 20:19 ` Uwe Kleine-König 2017-08-02 20:32 ` Sam Ravnborg 0 siblings, 1 reply; 11+ messages in thread From: Uwe Kleine-König @ 2017-08-02 20:19 UTC (permalink / raw) To: Sam Ravnborg; +Cc: barebox On Wed, Aug 02, 2017 at 09:54:40PM +0200, Sam Ravnborg wrote: > Hi Uwe. > > On Wed, Aug 02, 2017 at 09:39:05PM +0200, Uwe Kleine-König wrote: > > Hello, > > > > during bringup of an i.MX7 board I am faced with cspi_2_3_xchg_single not > > returning. I don't know yet why this happens, but with this patch set it at least > > doesn't block barebox. > > Your description reminded me of a similar issue we hit with SPI transfer. > Back then Sascha fixed it but with some open question marks. > > See 9da7e18573c725eaa6123c401df57d6a4f6a0ea2 > ("spi: i.MX: reset controller on init") > > But as this is the latest patch to the imx_spi.c file you likely > have already looked at it. I havn't looked at it, but it is included in my barebox as I'm using 2017.08.0 + some patches. Reading through the patch description, it doesn't match my situation. I'm doing USB-Booting because when I made eMMC work all my available remote hands already called it a day. So unless the boot ROM does something strange (impossible!) barebox is the first spi user. What seems to be similar is that TESTREG.TXCNT is != 0. For me this smells clk-related. On i.MX6 I recently identified a problem (but didn't come around yet to mainline a fix) that might match at least your problem. Can you still reproduce? How does your clk-tree look like (clk_dump)? Thanks for your attention Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] spi: imx: don't loop endlessly 2017-08-02 20:19 ` Uwe Kleine-König @ 2017-08-02 20:32 ` Sam Ravnborg 2017-08-03 9:07 ` Uwe Kleine-König 2017-08-03 18:41 ` Uwe Kleine-König 0 siblings, 2 replies; 11+ messages in thread From: Sam Ravnborg @ 2017-08-02 20:32 UTC (permalink / raw) To: Uwe Kleine-König; +Cc: barebox Hi Uwe. > I havn't looked at it, but it is included in my barebox as I'm using > 2017.08.0 + some patches. And I did not expect it to solve your problem, only to give you inspiration what could be the issue. > > Reading through the patch description, it doesn't match my situation. OK > I'm doing USB-Booting because when I made eMMC work all my available > remote hands already called it a day. So unless the boot ROM does > something strange (impossible!) barebox is the first spi user. > > What seems to be similar is that TESTREG.TXCNT is != 0. For me this > smells clk-related. On i.MX6 I recently identified a problem (but didn't > come around yet to mainline a fix) that might match at least your > problem. > > Can you still reproduce? We have used the patch from Sascha for a long time now (from before it hit barebox-next, and on our proprietary IMX6 target we never saw the SPI related error since. Sam > How does your clk-tree look like (clk_dump)? This is with: $ version barebox 2017.03.0-1 #1 Sun May 14 16:17:24 CEST 2017 + some of our own patches (nothing clk related). $ clk_dump lvds2_sel (rate 0, enabled) lvds2_gate (rate 0, disabled) osc (rate 24000000, enabled) pll1_sys (rate 792000000, disabled) pll1_sw (rate 792000000, disabled) arm (rate 792000000, disabled) twd (rate 396000000, disabled) cko2_sel (rate 792000000, disabled) cko2_podf (rate 792000000, disabled) cko2 (rate 792000000, disabled) pll2_bus (rate 528000000, enabled) pll2_pfd0_352m (rate 306580645, disabled) pll2_pfd1_594m (rate 528000000, disabled) pll2_pfd2_396m (rate 396000000, enabled) pll2_198m (rate 198000000, enabled) step (rate 396000000, enabled) periph_pre (rate 396000000, enabled) periph (rate 396000000, enabled) axi_sel (rate 396000000, enabled) axi (rate 198000000, enabled) eim_sel (rate 198000000, enabled) eim_podf (rate 99000000, enabled) eim_slow_sel (rate 198000000, enabled) eim_slow_podf (rate 99000000, enabled) eim_slow (rate 99000000, enabled) vdo_axi_sel (rate 198000000, enabled) pcie_axi_sel (rate 198000000, enabled) pcie_axi (rate 198000000, disabled) mmdc_ch0_axi_podf (rate 396000000, enabled) ipu1_sel (rate 396000000, enabled) ipu1_podf (rate 49500000, enabled) ipu1 (rate 49500000, enabled) 2400000.ipu_di0_pixel (rate 33000000, enabled) 2400000.ipu_di1_pixel (rate 49500000, enabled) ipu2_sel (rate 396000000, enabled) ipu2_podf (rate 198000000, enabled) ipu2 (rate 198000000, enabled) ahb (rate 132000000, enabled) ipg (rate 66000000, enabled) ipg_per (rate 66000000, enabled) gpt_ipg_per (rate 66000000, enabled) i2c1 (rate 66000000, enabled) i2c2 (rate 66000000, enabled) i2c3 (rate 66000000, enabled) pwm1 (rate 66000000, enabled) pwm2 (rate 66000000, enabled) pwm3 (rate 66000000, enabled) pwm4 (rate 66000000, enabled) caam_ipg (rate 66000000, enabled) enet (rate 66000000, enabled) gpt_ipg (rate 66000000, enabled) iim (rate 66000000, enabled) sata (rate 66000000, enabled) uart_ipg (rate 66000000, enabled) usboh3 (rate 66000000, enabled) caam_mem (rate 132000000, enabled) caam_aclk (rate 132000000, enabled) periph2_pre (rate 396000000, enabled) periph2 (rate 396000000, enabled) mmdc_ch1_axi_podf (rate 396000000, enabled) usdhc1_sel (rate 396000000, enabled) usdhc1_podf (rate 198000000, enabled) usdhc1 (rate 198000000, enabled) usdhc2_sel (rate 396000000, enabled) usdhc2_podf (rate 198000000, enabled) usdhc2 (rate 198000000, enabled) usdhc3_sel (rate 396000000, enabled) usdhc3_podf (rate 198000000, enabled) usdhc3 (rate 198000000, enabled) apbh_dma (rate 198000000, enabled) per1_bch (rate 198000000, enabled) gpmi_bch_apb (rate 198000000, enabled) gpmi_apb (rate 198000000, enabled) usdhc4_sel (rate 396000000, enabled) usdhc4_podf (rate 198000000, enabled) usdhc4 (rate 198000000, enabled) gpmi_bch (rate 198000000, enabled) cko1_sel (rate 528000000, enabled) cko1_podf (rate 528000000, enabled) cko1 (rate 528000000, disabled) cko (rate 528000000, disabled) pll3_usb_otg (rate 480000000, enabled) usbphy1 (rate 480000000, disabled) pll3_pfd0_720m (rate 720000000, disabled) pll3_pfd1_540m (rate 540000000, disabled) pll3_pfd2_508m (rate 508235294, disabled) pll3_pfd3_454m (rate 454736842, disabled) pll3_120m (rate 120000000, enabled) pll3_80m (rate 80000000, enabled) uart_serial_podf (rate 80000000, enabled) uart_serial (rate 80000000, enabled) pll3_60m (rate 60000000, enabled) ecspi_root (rate 60000000, enabled) ecspi1 (rate 60000000, enabled) ecspi2 (rate 60000000, enabled) ecspi3 (rate 60000000, enabled) ecspi4 (rate 60000000, enabled) ecspi5 (rate 60000000, enabled) periph2_clk2_sel (rate 480000000, enabled) periph2_clk2 (rate 480000000, enabled) enfc_sel (rate 480000000, enabled) enfc_pred (rate 96000000, enabled) enfc_podf (rate 96000000, enabled) enfc (rate 96000000, enabled) gpmi_io (rate 96000000, enabled) can_root (rate 240000000, enabled) pll4_audio (rate 144000000, disabled) pll5_video (rate 288000000, disabled) pll5_post_div (rate 72000000, disabled) pll5_video_div (rate 72000000, disabled) ldb_di0_sel (rate 72000000, disabled) ldb_di0_div_3_5 (rate 20571428, disabled) ldb_di0_podf (rate 10285714, disabled) ldb_di0 (rate 10285714, disabled) ldb_di1_sel (rate 72000000, disabled) ldb_di1_div_3_5 (rate 20571428, disabled) ldb_di1_podf (rate 10285714, disabled) ldb_di1 (rate 10285714, disabled) ipu1_di0_pre_sel (rate 72000000, disabled) ipu1_di0_pre (rate 24000000, disabled) ipu1_di0_sel (rate 24000000, disabled) ipu1_di0 (rate 24000000, disabled) ipu1_di1_pre_sel (rate 72000000, disabled) ipu1_di1_pre (rate 24000000, disabled) ipu1_di1_sel (rate 24000000, disabled) ipu1_di1 (rate 24000000, disabled) ipu2_di0_pre_sel (rate 72000000, disabled) ipu2_di0_pre (rate 24000000, disabled) ipu2_di0_sel (rate 24000000, disabled) ipu2_di0 (rate 24000000, disabled) ipu2_di1_pre_sel (rate 72000000, disabled) ipu2_di1_pre (rate 36000000, disabled) ipu2_di1_sel (rate 36000000, disabled) ipu2_di1 (rate 36000000, disabled) pll8_mlb (rate 24000000, disabled) pll7_usb_host (rate 480000000, disabled) usbphy2 (rate 480000000, disabled) pll6_enet (rate 500000000, enabled) sata_ref (rate 100000000, enabled) sata_ref_100m (rate 100000000, enabled) lvds1_sel (rate 100000000, enabled) lvds1_gate (rate 100000000, disabled) pcie_ref (rate 125000000, enabled) pcie_ref_125m (rate 125000000, disabled) enet_ref (rate 50000000, enabled) periph_clk2_sel (rate 24000000, enabled) periph_clk2 (rate 24000000, enabled) ckih1 (rate 0, enabled) ckil (rate 32768, enabled) _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] spi: imx: don't loop endlessly 2017-08-02 20:32 ` Sam Ravnborg @ 2017-08-03 9:07 ` Uwe Kleine-König 2017-08-03 10:43 ` Sam Ravnborg 2017-08-03 18:41 ` Uwe Kleine-König 1 sibling, 1 reply; 11+ messages in thread From: Uwe Kleine-König @ 2017-08-03 9:07 UTC (permalink / raw) To: Sam Ravnborg; +Cc: barebox On Wed, Aug 02, 2017 at 10:32:40PM +0200, Sam Ravnborg wrote: > Hi Uwe. > > I havn't looked at it, but it is included in my barebox as I'm using > > 2017.08.0 + some patches. > And I did not expect it to solve your problem, only to give you > inspiration what could be the issue. > > > > > Reading through the patch description, it doesn't match my situation. > OK > > > I'm doing USB-Booting because when I made eMMC work all my available > > remote hands already called it a day. So unless the boot ROM does > > something strange (impossible!) barebox is the first spi user. > > > > What seems to be similar is that TESTREG.TXCNT is != 0. For me this > > smells clk-related. On i.MX6 I recently identified a problem (but didn't > > come around yet to mainline a fix) that might match at least your > > problem. > > > > Can you still reproduce? > We have used the patch from Sascha for a long time now (from before it > hit barebox-next, and on our proprietary IMX6 target we never > saw the SPI related error since. > > Sam > > > How does your clk-tree look like (clk_dump)? > > This is with: > $ version > barebox 2017.03.0-1 #1 Sun May 14 16:17:24 CEST 2017 > + some of our own patches (nothing clk related). Which SoC are you using? Doesn't seem to match my problem. I don't know what Sascha did, I'd route ecspi_root to cko2 and check if there is a clk in the failing situation. (Assuming that pin is accessible on your machine of course.) Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] spi: imx: don't loop endlessly 2017-08-03 9:07 ` Uwe Kleine-König @ 2017-08-03 10:43 ` Sam Ravnborg 0 siblings, 0 replies; 11+ messages in thread From: Sam Ravnborg @ 2017-08-03 10:43 UTC (permalink / raw) To: Uwe Kleine-König; +Cc: barebox Hi Uwe. > Which SoC are you using? Doesn't seem to match my problem. I don't know > what Sascha did, I'd route ecspi_root to cko2 and check if there is a > clk in the failing situation. (Assuming that pin is accessible on your > machine of course.) The CPU is: NXP i.MX 6 Single Core Cortex-A9 Industrial processor MCIMX6S5EVM10AC We also ahev a quadcore variant. Both are a proprietary design, which have roots from a freescale reference design. I can send you the schematics (in private mail) if you think it is usefull for you. But as you already wrote, this is likely a different problem you are fighting with. Sam _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] spi: imx: don't loop endlessly 2017-08-02 20:32 ` Sam Ravnborg 2017-08-03 9:07 ` Uwe Kleine-König @ 2017-08-03 18:41 ` Uwe Kleine-König 1 sibling, 0 replies; 11+ messages in thread From: Uwe Kleine-König @ 2017-08-03 18:41 UTC (permalink / raw) To: Sam Ravnborg; +Cc: barebox On Wed, Aug 02, 2017 at 10:32:40PM +0200, Sam Ravnborg wrote: > Hi Uwe. > > I havn't looked at it, but it is included in my barebox as I'm using > > 2017.08.0 + some patches. > And I did not expect it to solve your problem, only to give you > inspiration what could be the issue. > > > > > Reading through the patch description, it doesn't match my situation. > OK > > > I'm doing USB-Booting because when I made eMMC work all my available > > remote hands already called it a day. So unless the boot ROM does > > something strange (impossible!) barebox is the first spi user. > > > > What seems to be similar is that TESTREG.TXCNT is != 0. For me this > > smells clk-related. On i.MX6 I recently identified a problem (but didn't > > come around yet to mainline a fix) that might match at least your > > problem. > > > > Can you still reproduce? > We have used the patch from Sascha for a long time now (from before it > hit barebox-next, and on our proprietary IMX6 target we never > saw the SPI related error since. > > Sam > > > How does your clk-tree look like (clk_dump)? > > This is with: > $ version > barebox 2017.03.0-1 #1 Sun May 14 16:17:24 CEST 2017 > + some of our own patches (nothing clk related). > > $ clk_dump > [...] > ecspi_root (rate 60000000, enabled) > ecspi1 (rate 60000000, enabled) > ecspi2 (rate 60000000, enabled) > ecspi3 (rate 60000000, enabled) > ecspi4 (rate 60000000, enabled) > ecspi5 (rate 60000000, enabled) ecspi_root is defined as: clks[IMX6QDL_CLK_ECSPI_ROOT] = imx_clk_divider("ecspi_root", "pll3_60m", base + 0x38, 19, 6); The RM (Reference Manual) describes this register as follows: CCM_CSCDR2, bits 24–19: ... NOTE: Divider should be updated when output clock is gated. It seems the BOOTROM in some older revisions of the chips didn't respect this requirement (for a different clk though) which resulted in ERR007117 (described in the ChipErrata document for the i.XM6DQ): For raw NAND boot, ROM switches the source of enfc_clk_root from PLL2_PFD2 to PLL3. The root clock is required to be gated before switching the source clock. If the root clock is not gated, clock glitches might be passed to the divider that follows the clock mux, and the divider might behave unpredictably. This can cause the clock generation to fail and the chip will not boot successfully. This problem can also occur elsewhere in application code if the root clock is not properly gated when the clock configuration is changed. IMHO this fits nicely to your error description. i.MX6UL is affected for sure as it does clk_set_parent(clks[IMX6UL_CLK_ENFC_SEL], clks[IMX6UL_CLK_PLL2_PFD2]); without making sure that some affected clks are off. This resulted in failures to probe the NAND occasionally on a customer machine. For i.MX6SDL or i.MX6DQ I don't see a matching change for the ecspi_root clk, but maybe this is triggered by setting the frequency on one of the child clocks?! Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] spi: imx: don't loop endlessly 2017-08-02 19:39 [PATCH 0/2] spi: imx: don't loop endlessly Uwe Kleine-König ` (2 preceding siblings ...) 2017-08-02 19:54 ` [PATCH 0/2] spi: imx: don't loop endlessly Sam Ravnborg @ 2017-09-29 18:08 ` Alexander Kurz 2017-10-02 5:31 ` Uwe Kleine-König 3 siblings, 1 reply; 11+ messages in thread From: Alexander Kurz @ 2017-09-29 18:08 UTC (permalink / raw) To: Uwe Kleine-König; +Cc: barebox [-- Attachment #1: Type: TEXT/PLAIN, Size: 1211 bytes --] Hi, during xchg_single 32 bits will be sent and received: 2x32 bits / 10 Microseconds = 6.4MHz Clock. Hence, a 10 Microseconds timeout will break SPI communication for boards with SPI frequencies less then 6.4MHz. On some boards spi-max-frequency is limited due to improper communication at higher frequencies, e.g. for the kindle4 it is 1MHz and there also exists one board with 100kHz. Before sending a patch calculating the timeout from spi-max-frequency, is 640 Microseconds (to fit imx28-cfa10049.dts 100kHz) acceptable? Regards, Alexander On Wed, 2 Aug 2017, Uwe Kleine-König wrote: > Hello, > > during bringup of an i.MX7 board I am faced with cspi_2_3_xchg_single not > returning. I don't know yet why this happens, but with this patch set it at least > doesn't block barebox. > > I didn't test on a working board, maybe the timeout (10 ?s) I chose is too tight? > > Best regards > Uwe > > Uwe Kleine-K?nig (2): > spi: imx: add error checking > spi: imx: add timeout to xchg_single > > drivers/spi/imx_spi.c | 105 ++++++++++++++++++++++++++++++++++---------------- > 1 file changed, 72 insertions(+), 33 deletions(-) > > -- > 2.11.0 > > > [-- Attachment #2: Type: text/plain, Size: 149 bytes --] _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] spi: imx: don't loop endlessly 2017-09-29 18:08 ` Alexander Kurz @ 2017-10-02 5:31 ` Uwe Kleine-König 0 siblings, 0 replies; 11+ messages in thread From: Uwe Kleine-König @ 2017-10-02 5:31 UTC (permalink / raw) To: Alexander Kurz; +Cc: barebox On Fri, Sep 29, 2017 at 08:08:28PM +0200, Alexander Kurz wrote: > Hi, > during xchg_single 32 bits will be sent and received: > 2x32 bits / 10 Microseconds = 6.4MHz Clock. why 2x? > Hence, a 10 Microseconds timeout will break SPI communication for > boards with SPI frequencies less then 6.4MHz. > On some boards spi-max-frequency is limited due to improper communication > at higher frequencies, e.g. for the kindle4 it is 1MHz and there > also exists one board with 100kHz. > > Before sending a patch calculating the timeout from spi-max-frequency, > is 640 Microseconds (to fit imx28-cfa10049.dts 100kHz) acceptable? I would expect that the hardware and software add some latency, so maybe round it up to 1 ms? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-10-02 5:31 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-08-02 19:39 [PATCH 0/2] spi: imx: don't loop endlessly Uwe Kleine-König 2017-08-02 19:39 ` [PATCH 1/2] spi: imx: add error checking Uwe Kleine-König 2017-08-02 19:39 ` [PATCH 2/2] spi: imx: add timeout to xchg_single Uwe Kleine-König 2017-08-02 19:54 ` [PATCH 0/2] spi: imx: don't loop endlessly Sam Ravnborg 2017-08-02 20:19 ` Uwe Kleine-König 2017-08-02 20:32 ` Sam Ravnborg 2017-08-03 9:07 ` Uwe Kleine-König 2017-08-03 10:43 ` Sam Ravnborg 2017-08-03 18:41 ` Uwe Kleine-König 2017-09-29 18:08 ` Alexander Kurz 2017-10-02 5:31 ` Uwe Kleine-König
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox