[U-Boot] FSL eSPI driver is a mess, hack attached.

The fsl_espi.c is wreck w.r.t large TX data.
Below is what I had to hack to load a FPGA over SPI and
what is the malloc hack good for? This does not work
well for TXing several MB data.

The driver needs to be rewritten and I do not have the
time so I post what I got in the hope it can help someone
else or trigger a rewrite.

---
 drivers/spi/fsl_espi.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

Am Freitag, 14. März 2014, 10:46:26 schrieb Joakim Tjernlund:
> The fsl_espi.c is wreck w.r.t large TX data.
> Below is what I had to hack to load a FPGA over SPI and
> what is the malloc hack good for? This does not work
> well for TXing several MB data.
> 
> The driver needs to be rewritten and I do not have the
> time so I post what I got in the hope it can help someone
> else or trigger a rewrite.

although this is a bit harsh, i'm inclined to agree. last time i tried this 
driver, apart from reading from a spi flash, nothing else was working. even a 
spi unlock was broken. if you actually capture the spi pins on a scope you see 
that mostly garbage is transmitted in some cases (eg. using the raw spi 
commands on the cli). fun fact: the spi unlock actually resulted in a spi lock 
;)

i guess freescale haven't put much love into this one ;)


-michael

Michael Walle <michael@walle.cc> wrote on 2014/03/14 22:45:20:
> 
> Am Freitag, 14. März 2014, 10:46:26 schrieb Joakim Tjernlund:
> > The fsl_espi.c is wreck w.r.t large TX data.
> > Below is what I had to hack to load a FPGA over SPI and
> > what is the malloc hack good for? This does not work
> > well for TXing several MB data.
> > 
> > The driver needs to be rewritten and I do not have the
> > time so I post what I got in the hope it can help someone
> > else or trigger a rewrite.
> 
> although this is a bit harsh, i'm inclined to agree. last time i tried 
this 
> driver, apart from reading from a spi flash, nothing else was working. 
even a 
> spi unlock was broken. if you actually capture the spi pins on a scope 
you see 
> that mostly garbage is transmitted in some cases (eg. using the raw spi 
> commands on the cli). fun fact: the spi unlock actually resulted in a 
spi lock 
> ;)
> 
> i guess freescale haven't put much love into this one ;)

Yes, the driver is a quick hack to read small amounts of data from an 
EEPROM/FLASH.

Have you tried the kernel driver for eSPI? I haven't gotten there yet but
I pray it is better.

  Jocke

Am 2014-03-15 12:33, schrieb Joakim Tjernlund:
> Have you tried the kernel driver for eSPI? I haven't gotten there yet 
> but
> I pray it is better.

yeah kernel driver is better.

-michael

Mingkai,

Please take a look at this patch and see if you can verify and polish it.

York


On 03/14/2014 02:46 AM, Joakim Tjernlund wrote:
> The fsl_espi.c is wreck w.r.t large TX data.
> Below is what I had to hack to load a FPGA over SPI and
> what is the malloc hack good for? This does not work
> well for TXing several MB data.
> 
> The driver needs to be rewritten and I do not have the
> time so I post what I got in the hope it can help someone
> else or trigger a rewrite.
> 
> ---
>  drivers/spi/fsl_espi.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/spi/fsl_espi.c b/drivers/spi/fsl_espi.c
> index 7c84582..2843a8e 100644
> --- a/drivers/spi/fsl_espi.c
> +++ b/drivers/spi/fsl_espi.c
> @@ -28,8 +28,9 @@ struct fsl_spi_slave {
>  
>  #define ESPI_MAX_CS_NUM		4
>  
> -#define ESPI_EV_RNE		(1 << 9)
> -#define ESPI_EV_TNF		(1 << 8)
> +#define ESPI_EV_DON		(1 << (31 - 17))
> +#define ESPI_EV_RNE		(1 << (31 - 22))
> +#define ESPI_EV_TNF		(1 << (31 - 23))
>  
>  #define ESPI_MODE_EN		(1 << 31)	/* Enable interface */
>  #define ESPI_MODE_TXTHR(x)	((x) << 8)	/* Tx FIFO threshold */
> @@ -37,6 +38,8 @@ struct fsl_spi_slave {
>  
>  #define ESPI_COM_CS(x)		((x) << 30)
>  #define ESPI_COM_TRANLEN(x)	((x) << 0)
> +#define ESPI_COM_TO             (1 << (31 - 4))
> +
>  
>  #define ESPI_CSMODE_CI_INACTIVEHIGH	(1 << 31)
>  #define ESPI_CSMODE_CP_BEGIN_EDGCLK	(1 << 30)
> @@ -146,8 +149,9 @@ int spi_claim_bus(struct spi_slave *slave)
>  			| ESPI_CSMODE_CI_INACTIVEHIGH);
>  
>  	/* Character bit order: msb first */
> -	out_be32(&espi->csmode[cs], in_be32(&espi->csmode[cs])
> -		| ESPI_CSMODE_REV_MSB_FIRST);
> +	if (!(mode & SPI_LSB_FIRST))
> +		out_be32(&espi->csmode[cs], in_be32(&espi->csmode[cs])
> +			 | ESPI_CSMODE_REV_MSB_FIRST);
>  
>  	/* Character length in bits, between 0x3~0xf, i.e. 4bits~16bits */
>  	out_be32(&espi->csmode[cs], in_be32(&espi->csmode[cs])
> @@ -172,7 +176,7 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *data_out,
>  	int len = 0;
>  	int num_blks, num_chunks, max_tran_len, tran_len;
>  	int num_bytes;
> -	unsigned char *ch;
> +	//unsigned char *ch;
>  	unsigned char *buffer = NULL;
>  	size_t buf_len;
>  	u8 *cmd_buf = fsl->cmd_buf;
> @@ -216,20 +220,21 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *data_out,
>  		cmd_len = 0;
>  		break;
>  	}
> -
> +	dout = data_out;
>  	debug("spi_xfer: slave %u:%u dout %08X(%p) din %08X(%p) len %u\n",
>  	      slave->bus, slave->cs, *(uint *) dout,
>  	      dout, *(uint *) din, din, len);
>  
>  	num_chunks = DIV_ROUND_UP(data_len, max_tran_len);
> +	dout = buffer;
>  	while (num_chunks--) {
>  		if (data_in)
>  			din = buffer + rx_offset;
> -		dout = buffer;
>  		tran_len = min(data_len , max_tran_len);
>  		num_blks = DIV_ROUND_UP(tran_len + cmd_len, 4);
>  		num_bytes = (tran_len + cmd_len) % 4;
>  		fsl->data_len = tran_len + cmd_len;
> +		data_len -= tran_len;
>  		spi_cs_activate(slave);
>  
>  		/* Clear all eSPI events */
> @@ -253,11 +258,12 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *data_out,
>  			}
>  
>  			/* Wait for eSPI transmit to get out */
> -			udelay(80);
> +			udelay(5);
>  
>  			event = in_be32(&espi->event);
>  			if (event & ESPI_EV_RNE) {
>  				tmpdin = in_be32(&espi->rx);
> +#if 0
>  				if (num_blks == 0 && num_bytes != 0) {
>  					ch = (unsigned char *)&tmpdin;
>  					while (num_bytes--)
> @@ -266,7 +272,7 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *data_out,
>  					*(u32 *) din = tmpdin;
>  					din += 4;
>  				}
> -
> +#endif
>  				out_be32(&espi->event, in_be32(&espi->event)
>  						| ESPI_EV_RNE);
>  				debug("***spi_xfer:...%08x readed\n", tmpdin);
> @@ -301,6 +307,7 @@ void spi_cs_activate(struct spi_slave *slave)
>  
>  	com &= ~(ESPI_COM_CS(0x3) | ESPI_COM_TRANLEN(0xFFFF));
>  	com |= ESPI_COM_CS(slave->cs);
> +	com |= ESPI_COM_TO;
>  	com |= ESPI_COM_TRANLEN(data_len - 1);
>  	out_be32(&espi->com, com);
>  }
>

Hi York,
We were also considering to improve the eSPI driver and we will look into this.
Thanks,
Mingkai

-----Original Message-----
From: Sun York-R58495 
Sent: Thursday, April 17, 2014 5:09 AM
Cc: u-boot@lists.denx.de; Hu Mingkai-B21284
Subject: Re: [U-Boot] FSL eSPI driver is a mess, hack attached.

Mingkai,

Please take a look at this patch and see if you can verify and polish it.

York


On 03/14/2014 02:46 AM, Joakim Tjernlund wrote:
> The fsl_espi.c is wreck w.r.t large TX data.
> Below is what I had to hack to load a FPGA over SPI and what is the 
> malloc hack good for? This does not work well for TXing several MB 
> data.
> 
> The driver needs to be rewritten and I do not have the time so I post 
> what I got in the hope it can help someone else or trigger a rewrite.
> 
> ---
>  drivers/spi/fsl_espi.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/spi/fsl_espi.c b/drivers/spi/fsl_espi.c index 
> 7c84582..2843a8e 100644
> --- a/drivers/spi/fsl_espi.c
> +++ b/drivers/spi/fsl_espi.c
> @@ -28,8 +28,9 @@ struct fsl_spi_slave {
>  
>  #define ESPI_MAX_CS_NUM		4
>  
> -#define ESPI_EV_RNE		(1 << 9)
> -#define ESPI_EV_TNF		(1 << 8)
> +#define ESPI_EV_DON		(1 << (31 - 17))
> +#define ESPI_EV_RNE		(1 << (31 - 22))
> +#define ESPI_EV_TNF		(1 << (31 - 23))
>  
>  #define ESPI_MODE_EN		(1 << 31)	/* Enable interface */
>  #define ESPI_MODE_TXTHR(x)	((x) << 8)	/* Tx FIFO threshold */
> @@ -37,6 +38,8 @@ struct fsl_spi_slave {
>  
>  #define ESPI_COM_CS(x)		((x) << 30)
>  #define ESPI_COM_TRANLEN(x)	((x) << 0)
> +#define ESPI_COM_TO             (1 << (31 - 4))
> +
>  
>  #define ESPI_CSMODE_CI_INACTIVEHIGH	(1 << 31)
>  #define ESPI_CSMODE_CP_BEGIN_EDGCLK	(1 << 30)
> @@ -146,8 +149,9 @@ int spi_claim_bus(struct spi_slave *slave)
>  			| ESPI_CSMODE_CI_INACTIVEHIGH);
>  
>  	/* Character bit order: msb first */
> -	out_be32(&espi->csmode[cs], in_be32(&espi->csmode[cs])
> -		| ESPI_CSMODE_REV_MSB_FIRST);
> +	if (!(mode & SPI_LSB_FIRST))
> +		out_be32(&espi->csmode[cs], in_be32(&espi->csmode[cs])
> +			 | ESPI_CSMODE_REV_MSB_FIRST);
>  
>  	/* Character length in bits, between 0x3~0xf, i.e. 4bits~16bits */
>  	out_be32(&espi->csmode[cs], in_be32(&espi->csmode[cs]) @@ -172,7 
> +176,7 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *data_out,
>  	int len = 0;
>  	int num_blks, num_chunks, max_tran_len, tran_len;
>  	int num_bytes;
> -	unsigned char *ch;
> +	//unsigned char *ch;
>  	unsigned char *buffer = NULL;
>  	size_t buf_len;
>  	u8 *cmd_buf = fsl->cmd_buf;
> @@ -216,20 +220,21 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *data_out,
>  		cmd_len = 0;
>  		break;
>  	}
> -
> +	dout = data_out;
>  	debug("spi_xfer: slave %u:%u dout %08X(%p) din %08X(%p) len %u\n",
>  	      slave->bus, slave->cs, *(uint *) dout,
>  	      dout, *(uint *) din, din, len);
>  
>  	num_chunks = DIV_ROUND_UP(data_len, max_tran_len);
> +	dout = buffer;
>  	while (num_chunks--) {
>  		if (data_in)
>  			din = buffer + rx_offset;
> -		dout = buffer;
>  		tran_len = min(data_len , max_tran_len);
>  		num_blks = DIV_ROUND_UP(tran_len + cmd_len, 4);
>  		num_bytes = (tran_len + cmd_len) % 4;
>  		fsl->data_len = tran_len + cmd_len;
> +		data_len -= tran_len;
>  		spi_cs_activate(slave);
>  
>  		/* Clear all eSPI events */
> @@ -253,11 +258,12 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *data_out,
>  			}
>  
>  			/* Wait for eSPI transmit to get out */
> -			udelay(80);
> +			udelay(5);
>  
>  			event = in_be32(&espi->event);
>  			if (event & ESPI_EV_RNE) {
>  				tmpdin = in_be32(&espi->rx);
> +#if 0
>  				if (num_blks == 0 && num_bytes != 0) {
>  					ch = (unsigned char *)&tmpdin;
>  					while (num_bytes--)
> @@ -266,7 +272,7 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *data_out,
>  					*(u32 *) din = tmpdin;
>  					din += 4;
>  				}
> -
> +#endif
>  				out_be32(&espi->event, in_be32(&espi->event)
>  						| ESPI_EV_RNE);
>  				debug("***spi_xfer:...%08x readed\n", tmpdin); @@ -301,6 +307,7 
> @@ void spi_cs_activate(struct spi_slave *slave)
>  
>  	com &= ~(ESPI_COM_CS(0x3) | ESPI_COM_TRANLEN(0xFFFF));
>  	com |= ESPI_COM_CS(slave->cs);
> +	com |= ESPI_COM_TO;
>  	com |= ESPI_COM_TRANLEN(data_len - 1);
>  	out_be32(&espi->com, com);
>  }
>