[U-Boot] [PATCH 3/3] spi: cadence_qspi_apb: Make flash writes 32 bit aligned

[Vignesh R]

On Tuesday 09 January 2018 11:31 AM, Goldschmidt Simon wrote:
> On Mon, 08/01/2018 12:18, Vignesh R wrote:
>> Make flash writes 32 bit aligned by using bounce buffers to deal with non 32 bit
>> aligned buffers. Allocate a 512 byte bounce buffer (max known page size
>> currently) for this case.
> 
> Looking at drivers/mtd/spi/sf_dataflash.c, I see at least one chip
> with 1024 byte page size (at45db642d).
> This should probably be a constant somewhere, or at least be checked
> at runtime, see below.
> 

Oh, I missed the dataflash devices. Since, we don't get the page size
info from framework it would be difficult to do allocate bounce buffer
only once in probe. I think better way would be to allocate and free
bounce buffer as and when required in
cadence_qspi_apb_indirect_write_execute() just like
bounce_buffer_start/stop. Will make changes and post v2 shortly


Regards
Vignesh

>> This is required because as per TI K2G TRM[1], the external master is only
>> permitted to issue 32-bit data interface writes until the last word of an indirect
>> transfer. Otherwise indirect writes is known to fail sometimes.
>>
>> [1] http://www.ti.com/lit/ug/spruhy8g/spruhy8g.pdf
>>
>> Signed-off-by: Vignesh R <vigneshr at ti.com>
>> ---
>>  drivers/spi/cadence_qspi.c     | 13 ++++++++++++-
>>  drivers/spi/cadence_qspi.h     |  1 +
>>  drivers/spi/cadence_qspi_apb.c | 10 +++++-----
>>  3 files changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c index
>> 991a7160bdb8..49c9b477e678 100644
>> --- a/drivers/spi/cadence_qspi.c
>> +++ b/drivers/spi/cadence_qspi.c
>> @@ -158,6 +158,10 @@ static int cadence_spi_probe(struct udevice *bus)
>>
>>  	priv->regbase = plat->regbase;
>>  	priv->ahbbase = plat->ahbbase;
>> +	/* Bounce buf to handle unaligned writes. 512B is max pagesize */
>> +	priv->bounce_buf = malloc(512);
> 
> This should probably be a named constant.
> 
>> +	if (!priv->bounce_buf)
>> +		return -ENOMEM;
>>
>>  	if (!priv->qspi_is_init) {
>>  		cadence_qspi_apb_controller_init(plat);
>> @@ -259,8 +263,15 @@ static int cadence_spi_xfer(struct udevice *dev, unsigned
>> int bitlen,
>>  			err = cadence_qspi_apb_indirect_write_setup
>>  				(plat, priv->cmd_len, cmd_buf);
>>  			if (!err) {
>> +				const u8 *txbuf = dout;
>> +
>> +				if ((uintptr_t)txbuf % 4) {
>> +					memcpy(priv->bounce_buf, dout,
>> +					       data_bytes);
> 
> Without checking the size of the buffer allocated for bounce_buf here,
> you risk a buffer overflow for chips with larger page size.
> 
>> +					txbuf = priv->bounce_buf;
>> +				}
>>  				err = cadence_qspi_apb_indirect_write_execute
>> -				(plat, data_bytes, dout);
>> +				(plat, data_bytes, txbuf);
>>  			}
>>  		break;
>>  		default:
>> diff --git a/drivers/spi/cadence_qspi.h b/drivers/spi/cadence_qspi.h index
>> 83154210bd95..c97419af3de3 100644
>> --- a/drivers/spi/cadence_qspi.h
>> +++ b/drivers/spi/cadence_qspi.h
>> @@ -43,6 +43,7 @@ struct cadence_spi_priv {
>>  	unsigned int	qspi_calibrated_hz;
>>  	unsigned int	qspi_calibrated_cs;
>>  	unsigned int	previous_hz;
>> +	void		*bounce_buf;
>>  };
>>
>>  /* Functions call declaration */
>> diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
>> index fa8af33ae19b..fd3ece4cb129 100644
>> --- a/drivers/spi/cadence_qspi_apb.c
>> +++ b/drivers/spi/cadence_qspi_apb.c
>> @@ -727,11 +727,11 @@ int cadence_qspi_apb_indirect_write_execute(struct
>> cadence_spi_platdata *plat,
>>
>>  	while (remaining > 0) {
>>  		write_bytes = remaining > page_size ? page_size : remaining;
>> -		/* Handle non-4-byte aligned access to avoid data abort. */
>> -		if (((uintptr_t)txbuf % 4) || (write_bytes % 4))
>> -			writesb(plat->ahbbase, txbuf, write_bytes);
>> -		else
>> -			writesl(plat->ahbbase, txbuf, write_bytes >> 2);
>> +		writesl(plat->ahbbase, txbuf, write_bytes >> 2);
>> +		if (write_bytes % 4)
>> +			writesb(plat->ahbbase,
>> +				txbuf + rounddown(write_bytes, 4),
>> +				write_bytes % 4);
>>
>>  		ret = wait_for_bit("QSPI", plat->regbase +
>> CQSPI_REG_SDRAMLEVEL,
>>  				   CQSPI_REG_SDRAMLEVEL_WR_MASK <<
>> --
>> 2.15.1
> 
> Regards,
> Simon
>