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

[Goldschmidt Simon]

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.

> 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