bcm53xx: replace SPI revert with a fix sent upstream
Instead of reverting whole commit it's enough to just revert a single line change. It seems the real problem with the regressing commit was a bump of read chunk size. Switching back to 256 B chunks is enough to fix the problem/regression. Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
This commit is contained in:
		| @@ -0,0 +1,42 @@ | |||||||
|  | From: =?UTF-8?q?Rafa=C5=82=20Mi=C5=82ecki?= <rafal@milecki.pl> | ||||||
|  | Date: Thu, 11 Oct 2018 09:07:31 +0200 | ||||||
|  | Subject: [PATCH FIX] spi: bcm-qspi: switch back to reading flash using smaller | ||||||
|  |  chunks | ||||||
|  | MIME-Version: 1.0 | ||||||
|  | Content-Type: text/plain; charset=UTF-8 | ||||||
|  | Content-Transfer-Encoding: 8bit | ||||||
|  |  | ||||||
|  | Fixing/optimizing bcm_qspi_bspi_read() performance introduced two | ||||||
|  | changes: | ||||||
|  | 1) It added a loop to read all requested data using multiple BSPI ops. | ||||||
|  | 2) It bumped max size of a single BSPI block request from 256 to 512 B. | ||||||
|  |  | ||||||
|  | The later change resulted in occasional BSPI timeouts causing a | ||||||
|  | regression. | ||||||
|  |  | ||||||
|  | For some unknown reason hardware doesn't always handle reads as expected | ||||||
|  | when using 512 B chunks. In such cases it may happen that BSPI returns | ||||||
|  | amount of requested bytes without the last 1-3 ones. It provides the | ||||||
|  | remaining bytes later but doesn't raise an interrupt until another LR | ||||||
|  | start. | ||||||
|  |  | ||||||
|  | Switching back to 256 B reads fixes that problem and regression. | ||||||
|  |  | ||||||
|  | Fixes: 345309fa7c0c ("spi: bcm-qspi: Fix bcm_qspi_bspi_read() performance") | ||||||
|  | Signed-off-by: Rafał Miłecki <rafal@milecki.pl> | ||||||
|  | Cc: stable@vger.kernel.org # 4.11+ | ||||||
|  | --- | ||||||
|  |  drivers/spi/spi-bcm-qspi.c | 2 +- | ||||||
|  |  1 file changed, 1 insertion(+), 1 deletion(-) | ||||||
|  |  | ||||||
|  | --- a/drivers/spi/spi-bcm-qspi.c | ||||||
|  | +++ b/drivers/spi/spi-bcm-qspi.c | ||||||
|  | @@ -88,7 +88,7 @@ | ||||||
|  |  #define BSPI_BPP_MODE_SELECT_MASK		BIT(8) | ||||||
|  |  #define BSPI_BPP_ADDR_SELECT_MASK		BIT(16) | ||||||
|  |   | ||||||
|  | -#define BSPI_READ_LENGTH			512 | ||||||
|  | +#define BSPI_READ_LENGTH			256 | ||||||
|  |   | ||||||
|  |  /* MSPI register offsets */ | ||||||
|  |  #define MSPI_SPCR0_LSB				0x000 | ||||||
| @@ -1,146 +0,0 @@ | |||||||
| From: =?UTF-8?q?Rafa=C5=82=20Mi=C5=82ecki?= <rafal@milecki.pl> |  | ||||||
| Subject: [PATCH] Revert "spi: bcm-qspi: Fix bcm_qspi_bspi_read() performance" |  | ||||||
| MIME-Version: 1.0 |  | ||||||
| Content-Type: text/plain; charset=UTF-8 |  | ||||||
| Content-Transfer-Encoding: 8bit |  | ||||||
|  |  | ||||||
| This reverts commit 345309fa7c0c9206a5344d379b174499952d79d9. |  | ||||||
|  |  | ||||||
| BSPI reads became unstable starting with above commit. There are BSPI |  | ||||||
| timeouts like this: |  | ||||||
| [   15.637809] bcm_iproc 18029200.spi: timeout waiting for BSPI |  | ||||||
| (...) |  | ||||||
| [   15.997809] bcm_iproc 18029200.spi: timeout waiting for BSPI |  | ||||||
| which cause filesystem stability problems. |  | ||||||
|  |  | ||||||
| Before above commit every time that bcm_qspi_bspi_lr_l2_isr() called |  | ||||||
| bcm_qspi_bspi_lr_l2_isr() it was resulting in bspi_rf_msg_len becoming |  | ||||||
| 0. |  | ||||||
| With that change it's not the case anymore which suggests there may be |  | ||||||
| some bug around that code. |  | ||||||
|  |  | ||||||
| It has changed and the new behavior seems to be causing problems. |  | ||||||
|  |  | ||||||
| Signed-off-by: Rafał Miłecki <rafal@milecki.pl> |  | ||||||
| --- |  | ||||||
|  |  | ||||||
| --- a/drivers/spi/spi-bcm-qspi.c |  | ||||||
| +++ b/drivers/spi/spi-bcm-qspi.c |  | ||||||
| @@ -88,7 +88,7 @@ |  | ||||||
|  #define BSPI_BPP_MODE_SELECT_MASK		BIT(8) |  | ||||||
|  #define BSPI_BPP_ADDR_SELECT_MASK		BIT(16) |  | ||||||
|   |  | ||||||
| -#define BSPI_READ_LENGTH			512 |  | ||||||
| +#define BSPI_READ_LENGTH			256 |  | ||||||
|   |  | ||||||
|  /* MSPI register offsets */ |  | ||||||
|  #define MSPI_SPCR0_LSB				0x000 |  | ||||||
| @@ -806,7 +806,7 @@ static int bcm_qspi_bspi_flash_read(stru |  | ||||||
|  				    struct spi_flash_read_message *msg) |  | ||||||
|  { |  | ||||||
|  	struct bcm_qspi *qspi = spi_master_get_devdata(spi->master); |  | ||||||
| -	u32 addr = 0, len, rdlen, len_words; |  | ||||||
| +	u32 addr = 0, len, len_words; |  | ||||||
|  	int ret = 0; |  | ||||||
|  	unsigned long timeo = msecs_to_jiffies(100); |  | ||||||
|  	struct bcm_qspi_soc_intc *soc_intc = qspi->soc_intc; |  | ||||||
| @@ -819,7 +819,7 @@ static int bcm_qspi_bspi_flash_read(stru |  | ||||||
|  	bcm_qspi_write(qspi, MSPI, MSPI_WRITE_LOCK, 0); |  | ||||||
|   |  | ||||||
|  	/* |  | ||||||
| -	 * when using flex mode we need to send |  | ||||||
| +	 * when using flex mode mode we need to send |  | ||||||
|  	 * the upper address byte to bspi |  | ||||||
|  	 */ |  | ||||||
|  	if (bcm_qspi_bspi_ver_three(qspi) == false) { |  | ||||||
| @@ -833,56 +833,47 @@ static int bcm_qspi_bspi_flash_read(stru |  | ||||||
|  	else |  | ||||||
|  		addr = msg->from & 0x00ffffff; |  | ||||||
|   |  | ||||||
| +	/* set BSPI RAF buffer max read length */ |  | ||||||
| +	len = msg->len; |  | ||||||
| +	if (len > BSPI_READ_LENGTH) |  | ||||||
| +		len = BSPI_READ_LENGTH; |  | ||||||
| + |  | ||||||
|  	if (bcm_qspi_bspi_ver_three(qspi) == true) |  | ||||||
|  		addr = (addr + 0xc00000) & 0xffffff; |  | ||||||
|   |  | ||||||
| -	/* |  | ||||||
| -	 * read into the entire buffer by breaking the reads |  | ||||||
| -	 * into RAF buffer read lengths |  | ||||||
| -	 */ |  | ||||||
| -	len = msg->len; |  | ||||||
| +	reinit_completion(&qspi->bspi_done); |  | ||||||
| +	bcm_qspi_enable_bspi(qspi); |  | ||||||
| +	len_words = (len + 3) >> 2; |  | ||||||
| +	qspi->bspi_rf_msg = msg; |  | ||||||
| +	qspi->bspi_rf_msg_status = 0; |  | ||||||
|  	qspi->bspi_rf_msg_idx = 0; |  | ||||||
| +	qspi->bspi_rf_msg_len = len; |  | ||||||
| +	dev_dbg(&qspi->pdev->dev, "bspi xfr addr 0x%x len 0x%x", addr, len); |  | ||||||
|   |  | ||||||
| -	do { |  | ||||||
| -		if (len > BSPI_READ_LENGTH) |  | ||||||
| -			rdlen = BSPI_READ_LENGTH; |  | ||||||
| -		else |  | ||||||
| -			rdlen = len; |  | ||||||
| - |  | ||||||
| -		reinit_completion(&qspi->bspi_done); |  | ||||||
| -		bcm_qspi_enable_bspi(qspi); |  | ||||||
| -		len_words = (rdlen + 3) >> 2; |  | ||||||
| -		qspi->bspi_rf_msg = msg; |  | ||||||
| -		qspi->bspi_rf_msg_status = 0; |  | ||||||
| -		qspi->bspi_rf_msg_len = rdlen; |  | ||||||
| -		dev_dbg(&qspi->pdev->dev, |  | ||||||
| -			"bspi xfr addr 0x%x len 0x%x", addr, rdlen); |  | ||||||
| -		bcm_qspi_write(qspi, BSPI, BSPI_RAF_START_ADDR, addr); |  | ||||||
| -		bcm_qspi_write(qspi, BSPI, BSPI_RAF_NUM_WORDS, len_words); |  | ||||||
| -		bcm_qspi_write(qspi, BSPI, BSPI_RAF_WATERMARK, 0); |  | ||||||
| -		if (qspi->soc_intc) { |  | ||||||
| -			/* |  | ||||||
| -			 * clear soc MSPI and BSPI interrupts and enable |  | ||||||
| -			 * BSPI interrupts. |  | ||||||
| -			 */ |  | ||||||
| -			soc_intc->bcm_qspi_int_ack(soc_intc, MSPI_BSPI_DONE); |  | ||||||
| -			soc_intc->bcm_qspi_int_set(soc_intc, BSPI_DONE, true); |  | ||||||
| -		} |  | ||||||
| +	bcm_qspi_write(qspi, BSPI, BSPI_RAF_START_ADDR, addr); |  | ||||||
| +	bcm_qspi_write(qspi, BSPI, BSPI_RAF_NUM_WORDS, len_words); |  | ||||||
| +	bcm_qspi_write(qspi, BSPI, BSPI_RAF_WATERMARK, 0); |  | ||||||
| + |  | ||||||
| +	if (qspi->soc_intc) { |  | ||||||
| +		/* |  | ||||||
| +		 * clear soc MSPI and BSPI interrupts and enable |  | ||||||
| +		 * BSPI interrupts. |  | ||||||
| +		 */ |  | ||||||
| +		soc_intc->bcm_qspi_int_ack(soc_intc, MSPI_BSPI_DONE); |  | ||||||
| +		soc_intc->bcm_qspi_int_set(soc_intc, BSPI_DONE, true); |  | ||||||
| +	} |  | ||||||
|   |  | ||||||
| -		/* Must flush previous writes before starting BSPI operation */ |  | ||||||
| -		mb(); |  | ||||||
| -		bcm_qspi_bspi_lr_start(qspi); |  | ||||||
| -		if (!wait_for_completion_timeout(&qspi->bspi_done, timeo)) { |  | ||||||
| -			dev_err(&qspi->pdev->dev, "timeout waiting for BSPI\n"); |  | ||||||
| -			ret = -ETIMEDOUT; |  | ||||||
| -			break; |  | ||||||
| -		} |  | ||||||
| +	/* Must flush previous writes before starting BSPI operation */ |  | ||||||
| +	mb(); |  | ||||||
|   |  | ||||||
| -		/* set msg return length */ |  | ||||||
| -		msg->retlen += rdlen; |  | ||||||
| -		addr += rdlen; |  | ||||||
| -		len -= rdlen; |  | ||||||
| -	} while (len); |  | ||||||
| +	bcm_qspi_bspi_lr_start(qspi); |  | ||||||
| +	if (!wait_for_completion_timeout(&qspi->bspi_done, timeo)) { |  | ||||||
| +		dev_err(&qspi->pdev->dev, "timeout waiting for BSPI\n"); |  | ||||||
| +		ret = -ETIMEDOUT; |  | ||||||
| +	} else { |  | ||||||
| +		/* set the return length for the caller */ |  | ||||||
| +		msg->retlen = len; |  | ||||||
| +	} |  | ||||||
|   |  | ||||||
|  	return ret; |  | ||||||
|  } |  | ||||||
		Reference in New Issue
	
	Block a user
	 Rafał Miłecki
					Rafał Miłecki