diff mbox

[U-Boot,v2,3/6] spi: cadence_qspi: remove sram polling from flash write

Message ID 1437013654-29387-4-git-send-email-vikas.manocha@st.com
State Superseded
Delegated to: Jagannadha Sutradharudu Teki
Headers show

Commit Message

Vikas MANOCHA July 16, 2015, 2:27 a.m. UTC 
There is no need to poll sram level before writing to flash, data going to SRAM
till sram is full, after that backpressure will take over.

Signed-off-by: Vikas Manocha <vikas.manocha@st.com>
---

Changes in v2: Rebased to master

 drivers/spi/cadence_qspi_apb.c |   61 ++++++++++------------------------------
 1 file changed, 15 insertions(+), 46 deletions(-)

Comments

Marek Vasut Aug. 13, 2015, 2:11 a.m. UTC | #1 
On Thursday, July 16, 2015 at 04:27:31 AM, Vikas Manocha wrote:
> There is no need to poll sram level before writing to flash, data going to
> SRAM till sram is full, after that backpressure will take over.

Please see the question I posed in 2/6 v2 .

> Signed-off-by: Vikas Manocha <vikas.manocha@st.com>
> ---
> 
> Changes in v2: Rebased to master
> 
>  drivers/spi/cadence_qspi_apb.c |   61
> ++++++++++------------------------------ 1 file changed, 15 insertions(+),
> 46 deletions(-)
> 
> diff --git a/drivers/spi/cadence_qspi_apb.c
> b/drivers/spi/cadence_qspi_apb.c index 487bbef..ad8b79a 100644
> --- a/drivers/spi/cadence_qspi_apb.c
> +++ b/drivers/spi/cadence_qspi_apb.c
> @@ -214,67 +214,36 @@ static int cadence_qspi_apb_read_fifo_data(void
> *dest, return 0;
>  }
> 
> -static void cadence_qspi_apb_write_fifo_data(const void *dest_ahb_addr,
> -	const void *src, unsigned int bytes)
> -{
> -	unsigned int temp = 0;
> -	int i;
> -	int remaining = bytes;
> -	unsigned int *dest_ptr = (unsigned int *)dest_ahb_addr;
> -	unsigned int *src_ptr = (unsigned int *)src;
> -
> -	while (remaining >= CQSPI_FIFO_WIDTH) {
> -		for (i = CQSPI_FIFO_WIDTH/sizeof(src_ptr) - 1; i >= 0; i--)
> -			writel(*(src_ptr+i), dest_ptr+i);
> -		src_ptr += CQSPI_FIFO_WIDTH/sizeof(src_ptr);
> -		remaining -= CQSPI_FIFO_WIDTH;
> -	}
> -	if (remaining) {
> -		/* dangling bytes */
> -		i = remaining/sizeof(dest_ptr);
> -		memcpy(&temp, src_ptr+i, remaining % sizeof(dest_ptr));
> -		writel(temp, dest_ptr+i);
> -		for (--i; i >= 0; i--)
> -			writel(*(src_ptr+i), dest_ptr+i);
> -	}
> -	return;
> -}
> -
>  /* Write to SRAM FIFO with polling SRAM fill level. */
>  static int qpsi_write_sram_fifo_push(struct cadence_spi_platdata *plat,
>  				const void *src_addr, unsigned int num_bytes)
>  {
> -	const void *reg_base = plat->regbase;
> -	void *dest_addr = plat->ahbbase;
> -	unsigned int retry = CQSPI_REG_RETRY;
> -	unsigned int sram_level;
> +	int i = 0;
> +	unsigned int *dest_addr = plat->ahbbase;
>  	unsigned int wr_bytes;
> -	unsigned char *src = (unsigned char *)src_addr;
> +	unsigned int *src_ptr = (unsigned int *)src_addr;
>  	int remaining = num_bytes;
>  	unsigned int page_size = plat->page_size;
> -	unsigned int sram_threshold_words = CQSPI_REG_SRAM_THRESHOLD_WORDS;
> 
>  	while (remaining > 0) {
> -		retry = CQSPI_REG_RETRY;
> -		while (retry--) {
> -			sram_level = CQSPI_GET_WR_SRAM_LEVEL(reg_base);
> -			if (sram_level <= sram_threshold_words)
> -				break;
> -		}
> -		if (!retry) {
> -			printf("QSPI: SRAM fill level (0x%08x) not hit lower 
expected level
> (0x%08x)", -			       sram_level, sram_threshold_words);
> -			return -1;
> -		}
>  		/* Write a page or remaining bytes. */
>  		wr_bytes = (remaining > page_size) ?
>  					page_size : remaining;
> 
> -		cadence_qspi_apb_write_fifo_data(dest_addr, src, wr_bytes);
> -		src += wr_bytes;
>  		remaining -= wr_bytes;
> +		while (wr_bytes >= CQSPI_FIFO_WIDTH) {
> +			for (i = 0; i < CQSPI_FIFO_WIDTH/sizeof(dest_addr); i++)
> +				writel(*(src_ptr+i), dest_addr+i);

Why don't you use memcpy instead , didn't you say you're copying data to/from 
SRAM? Is src_ptr value always aligned ?

> +			src_ptr += CQSPI_FIFO_WIDTH/sizeof(dest_addr);
> +			wr_bytes -= CQSPI_FIFO_WIDTH;
> +		}
> +		if (wr_bytes) {
> +			/* dangling bytes */
> +			i = wr_bytes/sizeof(dest_addr);
> +			for (i = wr_bytes/sizeof(dest_addr); i >= 0; i--)
> +				writel(*(src_ptr+i), dest_addr+i);
> +		}
>  	}
> -
>  	return 0;
>  }
Vikas MANOCHA Aug. 13, 2015, 4:30 p.m. UTC | #2 
Hi Marek,

On 08/12/2015 07:11 PM, Marek Vasut wrote:
> On Thursday, July 16, 2015 at 04:27:31 AM, Vikas Manocha wrote:
>> There is no need to poll sram level before writing to flash, data going to
>> SRAM till sram is full, after that backpressure will take over.
> 
> Please see the question I posed in 2/6 v2 .

replied in 2/6 v2.

> 
>> Signed-off-by: Vikas Manocha <vikas.manocha@st.com>
>> ---
>>
>> Changes in v2: Rebased to master
>>
>>  drivers/spi/cadence_qspi_apb.c |   61
>> ++++++++++------------------------------ 1 file changed, 15 insertions(+),
>> 46 deletions(-)
>>
>> diff --git a/drivers/spi/cadence_qspi_apb.c
>> b/drivers/spi/cadence_qspi_apb.c index 487bbef..ad8b79a 100644
>> --- a/drivers/spi/cadence_qspi_apb.c
>> +++ b/drivers/spi/cadence_qspi_apb.c
>> @@ -214,67 +214,36 @@ static int cadence_qspi_apb_read_fifo_data(void
>> *dest, return 0;
>>  }
>>
>> -static void cadence_qspi_apb_write_fifo_data(const void *dest_ahb_addr,
>> -	const void *src, unsigned int bytes)
>> -{
>> -	unsigned int temp = 0;
>> -	int i;
>> -	int remaining = bytes;
>> -	unsigned int *dest_ptr = (unsigned int *)dest_ahb_addr;
>> -	unsigned int *src_ptr = (unsigned int *)src;
>> -
>> -	while (remaining >= CQSPI_FIFO_WIDTH) {
>> -		for (i = CQSPI_FIFO_WIDTH/sizeof(src_ptr) - 1; i >= 0; i--)
>> -			writel(*(src_ptr+i), dest_ptr+i);
>> -		src_ptr += CQSPI_FIFO_WIDTH/sizeof(src_ptr);
>> -		remaining -= CQSPI_FIFO_WIDTH;
>> -	}
>> -	if (remaining) {
>> -		/* dangling bytes */
>> -		i = remaining/sizeof(dest_ptr);
>> -		memcpy(&temp, src_ptr+i, remaining % sizeof(dest_ptr));
>> -		writel(temp, dest_ptr+i);
>> -		for (--i; i >= 0; i--)
>> -			writel(*(src_ptr+i), dest_ptr+i);
>> -	}
>> -	return;
>> -}
>> -
>>  /* Write to SRAM FIFO with polling SRAM fill level. */
>>  static int qpsi_write_sram_fifo_push(struct cadence_spi_platdata *plat,
>>  				const void *src_addr, unsigned int num_bytes)
>>  {
>> -	const void *reg_base = plat->regbase;
>> -	void *dest_addr = plat->ahbbase;
>> -	unsigned int retry = CQSPI_REG_RETRY;
>> -	unsigned int sram_level;
>> +	int i = 0;
>> +	unsigned int *dest_addr = plat->ahbbase;
>>  	unsigned int wr_bytes;
>> -	unsigned char *src = (unsigned char *)src_addr;
>> +	unsigned int *src_ptr = (unsigned int *)src_addr;
>>  	int remaining = num_bytes;
>>  	unsigned int page_size = plat->page_size;
>> -	unsigned int sram_threshold_words = CQSPI_REG_SRAM_THRESHOLD_WORDS;
>>
>>  	while (remaining > 0) {
>> -		retry = CQSPI_REG_RETRY;
>> -		while (retry--) {
>> -			sram_level = CQSPI_GET_WR_SRAM_LEVEL(reg_base);
>> -			if (sram_level <= sram_threshold_words)
>> -				break;
>> -		}
>> -		if (!retry) {
>> -			printf("QSPI: SRAM fill level (0x%08x) not hit lower 
> expected level
>> (0x%08x)", -			       sram_level, sram_threshold_words);
>> -			return -1;
>> -		}
>>  		/* Write a page or remaining bytes. */
>>  		wr_bytes = (remaining > page_size) ?
>>  					page_size : remaining;
>>
>> -		cadence_qspi_apb_write_fifo_data(dest_addr, src, wr_bytes);
>> -		src += wr_bytes;
>>  		remaining -= wr_bytes;
>> +		while (wr_bytes >= CQSPI_FIFO_WIDTH) {
>> +			for (i = 0; i < CQSPI_FIFO_WIDTH/sizeof(dest_addr); i++)
>> +				writel(*(src_ptr+i), dest_addr+i);
> 
> Why don't you use memcpy instead , didn't you say you're copying data to/from 
> SRAM? Is src_ptr value always aligned ?

i think you are right, i will check it & fix in the next version.

Rgds,
Vikas

> 
>> +			src_ptr += CQSPI_FIFO_WIDTH/sizeof(dest_addr);
>> +			wr_bytes -= CQSPI_FIFO_WIDTH;
>> +		}
>> +		if (wr_bytes) {
>> +			/* dangling bytes */
>> +			i = wr_bytes/sizeof(dest_addr);
>> +			for (i = wr_bytes/sizeof(dest_addr); i >= 0; i--)
>> +				writel(*(src_ptr+i), dest_addr+i);
>> +		}
>>  	}
>> -
>>  	return 0;
>>  }
> .
>
Marek Vasut Aug. 13, 2015, 5:34 p.m. UTC | #3 
On Thursday, August 13, 2015 at 06:30:37 PM, vikasm wrote:
> Hi Marek,
> 
> On 08/12/2015 07:11 PM, Marek Vasut wrote:
> > On Thursday, July 16, 2015 at 04:27:31 AM, Vikas Manocha wrote:
> >> There is no need to poll sram level before writing to flash, data going
> >> to SRAM till sram is full, after that backpressure will take over.
> > 
> > Please see the question I posed in 2/6 v2 .
> 
> replied in 2/6 v2.

[...]

> >>  /* Write to SRAM FIFO with polling SRAM fill level. */
> >>  static int qpsi_write_sram_fifo_push(struct cadence_spi_platdata *plat,
> >>  
> >>  				const void *src_addr, unsigned int num_bytes)
> >>  
> >>  {
> >> 
> >> -	const void *reg_base = plat->regbase;
> >> -	void *dest_addr = plat->ahbbase;
> >> -	unsigned int retry = CQSPI_REG_RETRY;
> >> -	unsigned int sram_level;
> >> +	int i = 0;
> >> +	unsigned int *dest_addr = plat->ahbbase;
> >> 
> >>  	unsigned int wr_bytes;
> >> 
> >> -	unsigned char *src = (unsigned char *)src_addr;
> >> +	unsigned int *src_ptr = (unsigned int *)src_addr;
> >> 
> >>  	int remaining = num_bytes;
> >>  	unsigned int page_size = plat->page_size;
> >> 
> >> -	unsigned int sram_threshold_words = CQSPI_REG_SRAM_THRESHOLD_WORDS;
> >> 
> >>  	while (remaining > 0) {
> >> 
> >> -		retry = CQSPI_REG_RETRY;
> >> -		while (retry--) {
> >> -			sram_level = CQSPI_GET_WR_SRAM_LEVEL(reg_base);
> >> -			if (sram_level <= sram_threshold_words)
> >> -				break;
> >> -		}
> >> -		if (!retry) {
> >> -			printf("QSPI: SRAM fill level (0x%08x) not hit lower
> > 
> > expected level
> > 
> >> (0x%08x)", -			       sram_level, sram_threshold_words);
> >> -			return -1;
> >> -		}
> >> 
> >>  		/* Write a page or remaining bytes. */
> >>  		wr_bytes = (remaining > page_size) ?
> >>  		
> >>  					page_size : remaining;
> >> 
> >> -		cadence_qspi_apb_write_fifo_data(dest_addr, src, wr_bytes);
> >> -		src += wr_bytes;
> >> 
> >>  		remaining -= wr_bytes;
> >> 
> >> +		while (wr_bytes >= CQSPI_FIFO_WIDTH) {
> >> +			for (i = 0; i < CQSPI_FIFO_WIDTH/sizeof(dest_addr); i++)
> >> +				writel(*(src_ptr+i), dest_addr+i);
> > 
> > Why don't you use memcpy instead , didn't you say you're copying data
> > to/from SRAM? Is src_ptr value always aligned ?
> 
> i think you are right, i will check it & fix in the next version.

But is memcpy() really correct ? It does byte access and doesn't enforce 
ordering in any way.
diff mbox

Patch

diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
index 487bbef..ad8b79a 100644
--- a/drivers/spi/cadence_qspi_apb.c
+++ b/drivers/spi/cadence_qspi_apb.c
@@ -214,67 +214,36 @@  static int cadence_qspi_apb_read_fifo_data(void *dest,
 	return 0;
 }
 
-static void cadence_qspi_apb_write_fifo_data(const void *dest_ahb_addr,
-	const void *src, unsigned int bytes)
-{
-	unsigned int temp = 0;
-	int i;
-	int remaining = bytes;
-	unsigned int *dest_ptr = (unsigned int *)dest_ahb_addr;
-	unsigned int *src_ptr = (unsigned int *)src;
-
-	while (remaining >= CQSPI_FIFO_WIDTH) {
-		for (i = CQSPI_FIFO_WIDTH/sizeof(src_ptr) - 1; i >= 0; i--)
-			writel(*(src_ptr+i), dest_ptr+i);
-		src_ptr += CQSPI_FIFO_WIDTH/sizeof(src_ptr);
-		remaining -= CQSPI_FIFO_WIDTH;
-	}
-	if (remaining) {
-		/* dangling bytes */
-		i = remaining/sizeof(dest_ptr);
-		memcpy(&temp, src_ptr+i, remaining % sizeof(dest_ptr));
-		writel(temp, dest_ptr+i);
-		for (--i; i >= 0; i--)
-			writel(*(src_ptr+i), dest_ptr+i);
-	}
-	return;
-}
-
 /* Write to SRAM FIFO with polling SRAM fill level. */
 static int qpsi_write_sram_fifo_push(struct cadence_spi_platdata *plat,
 				const void *src_addr, unsigned int num_bytes)
 {
-	const void *reg_base = plat->regbase;
-	void *dest_addr = plat->ahbbase;
-	unsigned int retry = CQSPI_REG_RETRY;
-	unsigned int sram_level;
+	int i = 0;
+	unsigned int *dest_addr = plat->ahbbase;
 	unsigned int wr_bytes;
-	unsigned char *src = (unsigned char *)src_addr;
+	unsigned int *src_ptr = (unsigned int *)src_addr;
 	int remaining = num_bytes;
 	unsigned int page_size = plat->page_size;
-	unsigned int sram_threshold_words = CQSPI_REG_SRAM_THRESHOLD_WORDS;
 
 	while (remaining > 0) {
-		retry = CQSPI_REG_RETRY;
-		while (retry--) {
-			sram_level = CQSPI_GET_WR_SRAM_LEVEL(reg_base);
-			if (sram_level <= sram_threshold_words)
-				break;
-		}
-		if (!retry) {
-			printf("QSPI: SRAM fill level (0x%08x) not hit lower expected level (0x%08x)",
-			       sram_level, sram_threshold_words);
-			return -1;
-		}
 		/* Write a page or remaining bytes. */
 		wr_bytes = (remaining > page_size) ?
 					page_size : remaining;
 
-		cadence_qspi_apb_write_fifo_data(dest_addr, src, wr_bytes);
-		src += wr_bytes;
 		remaining -= wr_bytes;
+		while (wr_bytes >= CQSPI_FIFO_WIDTH) {
+			for (i = 0; i < CQSPI_FIFO_WIDTH/sizeof(dest_addr); i++)
+				writel(*(src_ptr+i), dest_addr+i);
+			src_ptr += CQSPI_FIFO_WIDTH/sizeof(dest_addr);
+			wr_bytes -= CQSPI_FIFO_WIDTH;
+		}
+		if (wr_bytes) {
+			/* dangling bytes */
+			i = wr_bytes/sizeof(dest_addr);
+			for (i = wr_bytes/sizeof(dest_addr); i >= 0; i--)
+				writel(*(src_ptr+i), dest_addr+i);
+		}
 	}
-
 	return 0;
 }