[U-Boot] u-boot, fsl_espi.c driver

[U-Boot] u-boot, fsl_espi.c driver

[York Sun]

Dear Joakim,

Thanks for raising a concern.

It's not fair to blame the last person who submitted a patch. We are all working
to make it better as an opensource comminuty. You have done a good job to hack
it to work. Would it be nicer if you can submit this or improved patch to u-boot
community for further review and testing, after putting informing commit
message? The mailing list address is CC'ed.

York


On 10/09/2014 09:14 AM, Joakim Tjernlund wrote:
> Guys, I noticed you have added to the fsl eSPI driver and you work for 
> Freescale.
> This driver is broken beyond words, just look what I had to hack to load a 
> FPGA over SPI(below).
> I hope you can take the time to fix the driver.
> 
> From e73b8bd36b42e39ee9e22b48deca3a54fbffe4ac Mon Sep 17 00:00:00 2001
> From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> Date: Mon, 3 Mar 2014 16:30:35 +0100
> Subject: [PATCH] Fast fixes to make eSPI driver work.
> 
> The whole driver stinks, needs to be rewritten.
> ---
>  drivers/spi/fsl_espi.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/spi/fsl_espi.c b/drivers/spi/fsl_espi.c
> index ae0fe58..f5a8fcc 100644
> --- a/drivers/spi/fsl_espi.c
> +++ b/drivers/spi/fsl_espi.c
> @@ -45,6 +45,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)
> @@ -165,8 +167,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])
> @@ -302,14 +305,15 @@ int spi_xfer(struct spi_slave *slave, unsigned int 
> bitlen, const void *data_out,
>               *(uint *)data_out, data_out, *(uint *)data_in, data_in, 
> 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 */
> @@ -320,12 +324,11 @@ int spi_xfer(struct spi_slave *slave, unsigned int 
> bitlen, const void *data_out,
>                         if (event & ESPI_EV_TNF) {
>                                 fsl_espi_tx(fsl, dout);
>                                 /* Set up the next iteration */
> -                               if (len > 4) {
> -                                       len -= 4;
> -                                       dout += 4;
> -                               }
> +                               len -= 4;
> +                               dout += 4;
>                         }
>  
> +                       num_blks--;
>                         event = in_be32(&espi->event);
>                         if (event & ESPI_EV_RNE) {
>                                 rf_cnt = ((event & ESPI_EV_RFCNT_MASK)
> @@ -338,7 +341,6 @@ int spi_xfer(struct spi_slave *slave, unsigned int 
> bitlen, const void *data_out,
>                                         continue;
>                                 if (fsl_espi_rx(fsl, din, rx_bytes)
>                                                 == rx_bytes) {
> -                                       num_blks--;
>                                         if (din)
>                                                 din = (unsigned char *)din
>                                                         + rx_bytes;
> @@ -374,6 +376,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);
>  }
> 

[U-Boot] u-boot, fsl_espi.c driver

[Jagan Teki]

On 9 October 2014 21:55, York Sun <yorksun@freescale.com> wrote:
> Dear Joakim,
>
> Thanks for raising a concern.
>
> It's not fair to blame the last person who submitted a patch. We are all working
> to make it better as an opensource comminuty. You have done a good job to hack
> it to work. Would it be nicer if you can submit this or improved patch to u-boot
> community for further review and testing, after putting informing commit
> message? The mailing list address is CC'ed.

Could you please elaborate the issue?

> On 10/09/2014 09:14 AM, Joakim Tjernlund wrote:
>> Guys, I noticed you have added to the fsl eSPI driver and you work for
>> Freescale.
>> This driver is broken beyond words, just look what I had to hack to load a
>> FPGA over SPI(below).
>> I hope you can take the time to fix the driver.
>>
>> From e73b8bd36b42e39ee9e22b48deca3a54fbffe4ac Mon Sep 17 00:00:00 2001
>> From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
>> Date: Mon, 3 Mar 2014 16:30:35 +0100
>> Subject: [PATCH] Fast fixes to make eSPI driver work.
>>
>> The whole driver stinks, needs to be rewritten.
>> ---
>>  drivers/spi/fsl_espi.c | 19 +++++++++++--------
>>  1 file changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/spi/fsl_espi.c b/drivers/spi/fsl_espi.c
>> index ae0fe58..f5a8fcc 100644
>> --- a/drivers/spi/fsl_espi.c
>> +++ b/drivers/spi/fsl_espi.c
>> @@ -45,6 +45,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)
>> @@ -165,8 +167,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])
>> @@ -302,14 +305,15 @@ int spi_xfer(struct spi_slave *slave, unsigned int
>> bitlen, const void *data_out,
>>               *(uint *)data_out, data_out, *(uint *)data_in, data_in,
>> 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 */
>> @@ -320,12 +324,11 @@ int spi_xfer(struct spi_slave *slave, unsigned int
>> bitlen, const void *data_out,
>>                         if (event & ESPI_EV_TNF) {
>>                                 fsl_espi_tx(fsl, dout);
>>                                 /* Set up the next iteration */
>> -                               if (len > 4) {
>> -                                       len -= 4;
>> -                                       dout += 4;
>> -                               }
>> +                               len -= 4;
>> +                               dout += 4;
>>                         }
>>
>> +                       num_blks--;
>>                         event = in_be32(&espi->event);
>>                         if (event & ESPI_EV_RNE) {
>>                                 rf_cnt = ((event & ESPI_EV_RFCNT_MASK)
>> @@ -338,7 +341,6 @@ int spi_xfer(struct spi_slave *slave, unsigned int
>> bitlen, const void *data_out,
>>                                         continue;
>>                                 if (fsl_espi_rx(fsl, din, rx_bytes)
>>                                                 == rx_bytes) {
>> -                                       num_blks--;
>>                                         if (din)
>>                                                 din = (unsigned char *)din
>>                                                         + rx_bytes;
>> @@ -374,6 +376,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);
>>  }
>>

thanks!
-- 
Jagan.

[U-Boot] u-boot, fsl_espi.c driver

[Joakim Tjernlund]

York Sun <yorksun@freescale.com> wrote on 2014/10/09 18:25:40:
> 
> Dear Joakim,
> 
> Thanks for raising a concern.
> 
> It's not fair to blame the last person who submitted a patch. We are all 
working

No of course not, I just noticed you guys had been in there and patched up 
some problem
so I hoped you would be interested to fix the remaining problems. This 
driver should never
have been committed in the first place.

> to make it better as an opensource comminuty. You have done a good job 
to hack
> it to work. Would it be nicer if you can submit this or improved patch 
to u-boot
> community for further review and testing, after putting informing commit
> message? The mailing list address is CC'ed.

Main problem with this driver is that TX does not work for:
 len > max_tran_len (nor does RX)
 does not TX the last odd bytes(len % 4 != 0)
 Does not work with TX only(RX buf == NULL)

Silently ignores SPI_LSB_FIRST 

On top of that it uses malloc all over and copies data back and forth for 
no good
reason, image a big SPI transfer with many MB(like my FPGA load). 

I am not in a good position fix this properly as my FPGA is TX only so I 
cannot test
RX at all(which is broken by my hack)

Finally, just to illustrate the merits of this driver, after transmission 
is done
there is:
 if (*buffer == 0x0b) {
                                data_in += tran_len;
                                data_len -= tran_len;
                                *(int *)buffer += tran_len;
                        }
what is the magic 0x0b test all about?

> 
> York
> 

[U-Boot] u-boot, fsl_espi.c driver

[York Sun]

Dear Joakim,

On 10/09/2014 10:43 AM, Joakim Tjernlund wrote:
> York Sun <yorksun@freescale.com> wrote on 2014/10/09 18:25:40:
>>
>> Dear Joakim,
>>
>> Thanks for raising a concern.
>>
>> It's not fair to blame the last person who submitted a patch. We are all 
> working
> 
> No of course not, I just noticed you guys had been in there and patched up 
> some problem
> so I hoped you would be interested to fix the remaining problems. This 
> driver should never
> have been committed in the first place.
> 
>> to make it better as an opensource comminuty. You have done a good job 
> to hack
>> it to work. Would it be nicer if you can submit this or improved patch 
> to u-boot
>> community for further review and testing, after putting informing commit
>> message? The mailing list address is CC'ed.
> 
> Main problem with this driver is that TX does not work for:
>  len > max_tran_len (nor does RX)
>  does not TX the last odd bytes(len % 4 != 0)
>  Does not work with TX only(RX buf == NULL)
> 
> Silently ignores SPI_LSB_FIRST 
> 
> On top of that it uses malloc all over and copies data back and forth for 
> no good
> reason, image a big SPI transfer with many MB(like my FPGA load). 
> 
> I am not in a good position fix this properly as my FPGA is TX only so I 
> cannot test
> RX at all(which is broken by my hack)
> 
> Finally, just to illustrate the merits of this driver, after transmission 
> is done
> there is:
>  if (*buffer == 0x0b) {
>                                 data_in += tran_len;
>                                 data_len -= tran_len;
>                                 *(int *)buffer += tran_len;
>                         }
> what is the magic 0x0b test all about?
> 

Thanks for the report. Driver maintainer (CC'ed) will take a look.

York

[U-Boot] u-boot, fsl_espi.c driver

[Joakim Tjernlund]

York Sun <yorksun@freescale.com> wrote on 2014/10/09 20:06:31:
> 
> Dear Joakim,
> 
> On 10/09/2014 10:43 AM, Joakim Tjernlund wrote:
> > York Sun <yorksun@freescale.com> wrote on 2014/10/09 18:25:40:
> >>
> >> Dear Joakim,
> >>
> >> Thanks for raising a concern.
> >>
> >> It's not fair to blame the last person who submitted a patch. We are 
all 
> > working
> > 
> > No of course not, I just noticed you guys had been in there and 
patched up 
> > some problem
> > so I hoped you would be interested to fix the remaining problems. This 

> > driver should never
> > have been committed in the first place.
> > 
> >> to make it better as an opensource comminuty. You have done a good 
job 
> > to hack
> >> it to work. Would it be nicer if you can submit this or improved 
patch 
> > to u-boot
> >> community for further review and testing, after putting informing 
commit
> >> message? The mailing list address is CC'ed.
> > 
> > Main problem with this driver is that TX does not work for:
> >  len > max_tran_len (nor does RX)
> >  does not TX the last odd bytes(len % 4 != 0)
> >  Does not work with TX only(RX buf == NULL)
> > 
> > Silently ignores SPI_LSB_FIRST 
> > 
> > On top of that it uses malloc all over and copies data back and forth 
for 
> > no good
> > reason, image a big SPI transfer with many MB(like my FPGA load). 
> > 
> > I am not in a good position fix this properly as my FPGA is TX only so 
I 
> > cannot test
> > RX at all(which is broken by my hack)
> > 
> > Finally, just to illustrate the merits of this driver, after 
transmission 
> > is done
> > there is:
> >  if (*buffer == 0x0b) {
> >                                 data_in += tran_len;
> >                                 data_len -= tran_len;
> >                                 *(int *)buffer += tran_len;
> >                         }
> > what is the magic 0x0b test all about?
> > 
> 
> Thanks for the report. Driver maintainer (CC'ed) will take a look.
> 

Great, I do have some ideas on what to do ishould there be any interest.

 Jocke

[U-Boot] u-boot, fsl_espi.c driver

[Joakim Tjernlund]

York Sun <yorksun@freescale.com> wrote on 2014/10/09 20:06:31:
> 
> Dear Joakim,
> 
> On 10/09/2014 10:43 AM, Joakim Tjernlund wrote:
> > York Sun <yorksun@freescale.com> wrote on 2014/10/09 18:25:40:
> >>
> >> Dear Joakim,
> >>
> >> Thanks for raising a concern.
> >>
> >> It's not fair to blame the last person who submitted a patch. We are 
all 
> > working
> > 
> > No of course not, I just noticed you guys had been in there and 
patched up 
> > some problem
> > so I hoped you would be interested to fix the remaining problems. This 

> > driver should never
> > have been committed in the first place.
> > 
> >> to make it better as an opensource comminuty. You have done a good 
job 
> > to hack
> >> it to work. Would it be nicer if you can submit this or improved 
patch 
> > to u-boot
> >> community for further review and testing, after putting informing 
commit
> >> message? The mailing list address is CC'ed.
> > 
> > Main problem with this driver is that TX does not work for:
> >  len > max_tran_len (nor does RX)
> >  does not TX the last odd bytes(len % 4 != 0)
> >  Does not work with TX only(RX buf == NULL)
> > 
> > Silently ignores SPI_LSB_FIRST 
> > 
> > On top of that it uses malloc all over and copies data back and forth 
for 
> > no good
> > reason, image a big SPI transfer with many MB(like my FPGA load). 
> > 
> > I am not in a good position fix this properly as my FPGA is TX only so 
I 
> > cannot test
> > RX at all(which is broken by my hack)
> > 
> > Finally, just to illustrate the merits of this driver, after 
transmission 
> > is done
> > there is:
> >  if (*buffer == 0x0b) {
> >                                 data_in += tran_len;
> >                                 data_len -= tran_len;
> >                                 *(int *)buffer += tran_len;
> >                         }
> > what is the magic 0x0b test all about?
> > 
> 
> Thanks for the report. Driver maintainer (CC'ed) will take a look.
> 
> York

No reaction from maintainers, I don't think this driver is still 
maintained.

   Jocke

[U-Boot] u-boot, fsl_espi.c driver

[York Sun]

Jagan and Mingkai,

Would you take a look when you have a chance?

York


On 10/22/2014 03:12 PM, Joakim Tjernlund wrote:
> York Sun <yorksun@freescale.com> wrote on 2014/10/09 20:06:31:
>>
>> Dear Joakim,
>>
>> On 10/09/2014 10:43 AM, Joakim Tjernlund wrote:
>>> York Sun <yorksun@freescale.com> wrote on 2014/10/09 18:25:40:
>>>>
>>>> Dear Joakim,
>>>>
>>>> Thanks for raising a concern.
>>>>
>>>> It's not fair to blame the last person who submitted a patch. We are 
> all 
>>> working
>>>
>>> No of course not, I just noticed you guys had been in there and 
> patched up 
>>> some problem
>>> so I hoped you would be interested to fix the remaining problems. This 
> 
>>> driver should never
>>> have been committed in the first place.
>>>
>>>> to make it better as an opensource comminuty. You have done a good 
> job 
>>> to hack
>>>> it to work. Would it be nicer if you can submit this or improved 
> patch 
>>> to u-boot
>>>> community for further review and testing, after putting informing 
> commit
>>>> message? The mailing list address is CC'ed.
>>>
>>> Main problem with this driver is that TX does not work for:
>>>  len > max_tran_len (nor does RX)
>>>  does not TX the last odd bytes(len % 4 != 0)
>>>  Does not work with TX only(RX buf == NULL)
>>>
>>> Silently ignores SPI_LSB_FIRST 
>>>
>>> On top of that it uses malloc all over and copies data back and forth 
> for 
>>> no good
>>> reason, image a big SPI transfer with many MB(like my FPGA load). 
>>>
>>> I am not in a good position fix this properly as my FPGA is TX only so 
> I 
>>> cannot test
>>> RX at all(which is broken by my hack)
>>>
>>> Finally, just to illustrate the merits of this driver, after 
> transmission 
>>> is done
>>> there is:
>>>  if (*buffer == 0x0b) {
>>>                                 data_in += tran_len;
>>>                                 data_len -= tran_len;
>>>                                 *(int *)buffer += tran_len;
>>>                         }
>>> what is the magic 0x0b test all about?
>>>
>>
>> Thanks for the report. Driver maintainer (CC'ed) will take a look.
>>
>> York
> 
> No reaction from maintainers, I don't think this driver is still 
> maintained.
> 
>    Jocke
> 

[U-Boot] u-boot, fsl_espi.c driver

[Mingkai.Hu at freescale.com]

Hi Joakim and York,

I apologize for the delayed response and thanks for your catch up, Joakim
In order to get a higher transfer speed, the eSPI controller was designed to transfer multiple bytes at one transaction which is not comply with the SPI framework and the CS can't be inactive until one transfer finished, so we need to combine the multiple transfers into one transfer, that's the reason why there are more memory copy back and forth.

The driver has been tested on SPI flash and the transfer length larger than max_trans_len should be handled but half-duplex was not handled. Is it possible to provide patch to make it workable for different devices?

0x0b is the FAST READ command. The logic here is to move the pointer of receive buffer to receive new data when reading from NOR flash and the reading length is larger than the max_trans_len.

Thanks,
Mingkai

________________________________________
From: Sun York-R58495
Sent: Thursday, October 23, 2014 6:14 AM
To: jagan Teki; Hu Mingkai-B21284
Cc: Joakim Tjernlund; Hou Zhiqiang-B48286; u-boot at lists.denx.de
Subject: Re: u-boot, fsl_espi.c driver

Jagan and Mingkai,

Would you take a look when you have a chance?

York


On 10/22/2014 03:12 PM, Joakim Tjernlund wrote:
> York Sun <yorksun@freescale.com> wrote on 2014/10/09 20:06:31:
>>
>> Dear Joakim,
>>
>> On 10/09/2014 10:43 AM, Joakim Tjernlund wrote:
>>> York Sun <yorksun@freescale.com> wrote on 2014/10/09 18:25:40:
>>>>
>>>> Dear Joakim,
>>>>
>>>> Thanks for raising a concern.
>>>>
>>>> It's not fair to blame the last person who submitted a patch. We are
> all
>>> working
>>>
>>> No of course not, I just noticed you guys had been in there and
> patched up
>>> some problem
>>> so I hoped you would be interested to fix the remaining problems. This
>
>>> driver should never
>>> have been committed in the first place.
>>>
>>>> to make it better as an opensource comminuty. You have done a good
> job
>>> to hack
>>>> it to work. Would it be nicer if you can submit this or improved
> patch
>>> to u-boot
>>>> community for further review and testing, after putting informing
> commit
>>>> message? The mailing list address is CC'ed.
>>>
>>> Main problem with this driver is that TX does not work for:
>>>  len > max_tran_len (nor does RX)
>>>  does not TX the last odd bytes(len % 4 != 0)
>>>  Does not work with TX only(RX buf == NULL)
>>>
>>> Silently ignores SPI_LSB_FIRST
>>>
>>> On top of that it uses malloc all over and copies data back and forth
> for
>>> no good
>>> reason, image a big SPI transfer with many MB(like my FPGA load).
>>>
>>> I am not in a good position fix this properly as my FPGA is TX only so
> I
>>> cannot test
>>> RX at all(which is broken by my hack)
>>>
>>> Finally, just to illustrate the merits of this driver, after
> transmission
>>> is done
>>> there is:
>>>  if (*buffer == 0x0b) {
>>>                                 data_in += tran_len;
>>>                                 data_len -= tran_len;
>>>                                 *(int *)buffer += tran_len;
>>>                         }
>>> what is the magic 0x0b test all about?
>>>
>>
>> Thanks for the report. Driver maintainer (CC'ed) will take a look.
>>
>> York
>
> No reaction from maintainers, I don't think this driver is still
> maintained.
>
>    Jocke
>

[U-Boot] u-boot, fsl_espi.c driver

[Joakim Tjernlund]

"Mingkai.Hu at freescale.com" <Mingkai.Hu@freescale.com> wrote on 2014/10/28 
12:17:24:
> 
> Hi Joakim and York,

Hi yourself, been travelling or a few days.

> 
> I apologize for the delayed response and thanks for your catch up, 
Joakim
> In order to get a higher transfer speed, the eSPI controller was 
designed to transfer multiple bytes at one transaction which is not comply 
with the SPI framework and the CS can't be inactive until one transfer 
finished, so we need to combine the multiple transfers into one transfer, 
that's the reason why there are more memory copy back and forth.

hmm, can't make sense of this. If you can memcpy from/to memory you can 
also write/read the SPI FIFO
Also writing 4 bytes to the SPI FIFO at a time will not but you much if 
anything at all. It just
makes it impossible to use NF flag etc.
Speed will come from keeping the SPI FIFO non empty and not copying memory 
back and fourth.
CS can be controlled within the SPI framework already.

> 
> The driver has been tested on SPI flash and the transfer length larger 
than max_trans_len should be handled but half-duplex 
was not handled. Is it possible to provide patch to make it workable for 
different devices?

Not as the driver is written today.

> 
> 0x0b is the FAST READ command. The logic here is to move the pointer of 
receive buffer to receive new data when reading from NOR flash and the 
reading length is larger than the max_trans_len.

What FAST READ command? Sounds like it is connected to the FLASH rather 
than SPI? if so that
need to go from the SPI driver and moved into some board specific addon.

> 
> Thanks,
> Mingkai
> 
> ________________________________________
> From: Sun York-R58495
> Sent: Thursday, October 23, 2014 6:14 AM
> To: jagan Teki; Hu Mingkai-B21284
> Cc: Joakim Tjernlund; Hou Zhiqiang-B48286; u-boot at lists.denx.de
> Subject: Re: u-boot, fsl_espi.c driver
> 
> Jagan and Mingkai,
> 
> Would you take a look when you have a chance?
> 
> York
> 
> 
> On 10/22/2014 03:12 PM, Joakim Tjernlund wrote:
> > York Sun <yorksun@freescale.com> wrote on 2014/10/09 20:06:31:
> >>
> >> Dear Joakim,
> >>
> >> On 10/09/2014 10:43 AM, Joakim Tjernlund wrote:
> >>> York Sun <yorksun@freescale.com> wrote on 2014/10/09 18:25:40:
> >>>>
> >>>> Dear Joakim,
> >>>>
> >>>> Thanks for raising a concern.
> >>>>
> >>>> It's not fair to blame the last person who submitted a patch. We 
are
> > all
> >>> working
> >>>
> >>> No of course not, I just noticed you guys had been in there and
> > patched up
> >>> some problem
> >>> so I hoped you would be interested to fix the remaining problems. 
This
> >
> >>> driver should never
> >>> have been committed in the first place.
> >>>
> >>>> to make it better as an opensource comminuty. You have done a good
> > job
> >>> to hack
> >>>> it to work. Would it be nicer if you can submit this or improved
> > patch
> >>> to u-boot
> >>>> community for further review and testing, after putting informing
> > commit
> >>>> message? The mailing list address is CC'ed.
> >>>
> >>> Main problem with this driver is that TX does not work for:
> >>>  len > max_tran_len (nor does RX)
> >>>  does not TX the last odd bytes(len % 4 != 0)
> >>>  Does not work with TX only(RX buf == NULL)
> >>>
> >>> Silently ignores SPI_LSB_FIRST
> >>>
> >>> On top of that it uses malloc all over and copies data back and 
forth
> > for
> >>> no good
> >>> reason, image a big SPI transfer with many MB(like my FPGA load).
> >>>
> >>> I am not in a good position fix this properly as my FPGA is TX only 
so
> > I
> >>> cannot test
> >>> RX at all(which is broken by my hack)
> >>>
> >>> Finally, just to illustrate the merits of this driver, after
> > transmission
> >>> is done
> >>> there is:
> >>>  if (*buffer == 0x0b) {
> >>>                                 data_in += tran_len;
> >>>                                 data_len -= tran_len;
> >>>                                 *(int *)buffer += tran_len;
> >>>                         }
> >>> what is the magic 0x0b test all about?
> >>>
> >>
> >> Thanks for the report. Driver maintainer (CC'ed) will take a look.
> >>
> >> York
> >
> > No reaction from maintainers, I don't think this driver is still
> > maintained.
> >
> >    Jocke
> >

[U-Boot] u-boot, fsl_espi.c driver

[Joakim Tjernlund]

Ping?

Joakim Tjernlund/Transmode wrote on 2014/10/29 19:43:14:
> 
> "Mingkai.Hu at freescale.com" <Mingkai.Hu@freescale.com> wrote on 
2014/10/28 12:17:24:
> > 
> > Hi Joakim and York,
> 
> Hi yourself, been travelling or a few days.
> 
> > 
> > I apologize for the delayed response and thanks for your catch up, 
Joakim
> > In order to get a higher transfer speed, the eSPI controller was 
designed to transfer multiple bytes at one transaction which is not comply 
with the SPI framework and the CS can't be inactive until one transfer 
finished, so we need to combine the multiple transfers into one transfer, 
that's the reason why there are more memory copy back and forth.
> 
> hmm, can't make sense of this. If you can memcpy from/to memory you can 
also write/read the SPI FIFO
> Also writing 4 bytes to the SPI FIFO at a time will not but you much if 
anything at all. It just
> makes it impossible to use NF flag etc.
> Speed will come from keeping the SPI FIFO non empty and not copying 
memory back and fourth.
> CS can be controlled within the SPI framework already.
> 
> > 
> > The driver has been tested on SPI flash and the transfer length larger 
than max_trans_len should be handled but half-duplex 
> was not handled. Is it possible to provide patch to make it workable for 
different devices?
> 
> Not as the driver is written today.
> 
> > 
> > 0x0b is the FAST READ command. The logic here is to move the pointer 
of receive buffer to receive new data when reading from NOR flash and the 
reading length is larger than the max_trans_len.
> 
> What FAST READ command? Sounds like it is connected to the FLASH rather 
than SPI? if so that
> need to go from the SPI driver and moved into some board specific addon.
> 
> > 
> > Thanks,
> > Mingkai
> > 
> > ________________________________________
> > From: Sun York-R58495
> > Sent: Thursday, October 23, 2014 6:14 AM
> > To: jagan Teki; Hu Mingkai-B21284
> > Cc: Joakim Tjernlund; Hou Zhiqiang-B48286; u-boot at lists.denx.de
> > Subject: Re: u-boot, fsl_espi.c driver
> > 
> > Jagan and Mingkai,
> > 
> > Would you take a look when you have a chance?
> > 
> > York
> > 
> > 
> > On 10/22/2014 03:12 PM, Joakim Tjernlund wrote:
> > > York Sun <yorksun@freescale.com> wrote on 2014/10/09 20:06:31:
> > >>
> > >> Dear Joakim,
> > >>
> > >> On 10/09/2014 10:43 AM, Joakim Tjernlund wrote:
> > >>> York Sun <yorksun@freescale.com> wrote on 2014/10/09 18:25:40:
> > >>>>
> > >>>> Dear Joakim,
> > >>>>
> > >>>> Thanks for raising a concern.
> > >>>>
> > >>>> It's not fair to blame the last person who submitted a patch. We 
are
> > > all
> > >>> working
> > >>>
> > >>> No of course not, I just noticed you guys had been in there and
> > > patched up
> > >>> some problem
> > >>> so I hoped you would be interested to fix the remaining problems. 
This
> > >
> > >>> driver should never
> > >>> have been committed in the first place.
> > >>>
> > >>>> to make it better as an opensource comminuty. You have done a 
good
> > > job
> > >>> to hack
> > >>>> it to work. Would it be nicer if you can submit this or improved
> > > patch
> > >>> to u-boot
> > >>>> community for further review and testing, after putting informing
> > > commit
> > >>>> message? The mailing list address is CC'ed.
> > >>>
> > >>> Main problem with this driver is that TX does not work for:
> > >>>  len > max_tran_len (nor does RX)
> > >>>  does not TX the last odd bytes(len % 4 != 0)
> > >>>  Does not work with TX only(RX buf == NULL)
> > >>>
> > >>> Silently ignores SPI_LSB_FIRST
> > >>>
> > >>> On top of that it uses malloc all over and copies data back and 
forth
> > > for
> > >>> no good
> > >>> reason, image a big SPI transfer with many MB(like my FPGA load).
> > >>>
> > >>> I am not in a good position fix this properly as my FPGA is TX 
only so
> > > I
> > >>> cannot test
> > >>> RX at all(which is broken by my hack)
> > >>>
> > >>> Finally, just to illustrate the merits of this driver, after
> > > transmission
> > >>> is done
> > >>> there is:
> > >>>  if (*buffer == 0x0b) {
> > >>>                                 data_in += tran_len;
> > >>>                                 data_len -= tran_len;
> > >>>                                 *(int *)buffer += tran_len;
> > >>>                         }
> > >>> what is the magic 0x0b test all about?
> > >>>
> > >>
> > >> Thanks for the report. Driver maintainer (CC'ed) will take a look.
> > >>
> > >> York
> > >
> > > No reaction from maintainers, I don't think this driver is still
> > > maintained.
> > >
> > >    Jocke
> > >

[U-Boot] u-boot, fsl_espi.c driver

[Jagan Teki]

On 18 November 2014 14:42, Joakim Tjernlund
<joakim.tjernlund@transmode.se> wrote:
> Ping?
>
> Joakim Tjernlund/Transmode wrote on 2014/10/29 19:43:14:

Couldn't understand what discussion is going, does some issue in
driver or any plan to
write new code, please let me know - we can plan accordingly and kill
the thread.

>>
>> "Mingkai.Hu at freescale.com" <Mingkai.Hu@freescale.com> wrote on
> 2014/10/28 12:17:24:
>> >
>> > Hi Joakim and York,
>>
>> Hi yourself, been travelling or a few days.
>>
>> >
>> > I apologize for the delayed response and thanks for your catch up,
> Joakim
>> > In order to get a higher transfer speed, the eSPI controller was
> designed to transfer multiple bytes at one transaction which is not comply
> with the SPI framework and the CS can't be inactive until one transfer
> finished, so we need to combine the multiple transfers into one transfer,
> that's the reason why there are more memory copy back and forth.
>>
>> hmm, can't make sense of this. If you can memcpy from/to memory you can
> also write/read the SPI FIFO
>> Also writing 4 bytes to the SPI FIFO at a time will not but you much if
> anything at all. It just
>> makes it impossible to use NF flag etc.
>> Speed will come from keeping the SPI FIFO non empty and not copying
> memory back and fourth.
>> CS can be controlled within the SPI framework already.
>>
>> >
>> > The driver has been tested on SPI flash and the transfer length larger
> than max_trans_len should be handled but half-duplex
>> was not handled. Is it possible to provide patch to make it workable for
> different devices?
>>
>> Not as the driver is written today.
>>
>> >
>> > 0x0b is the FAST READ command. The logic here is to move the pointer
> of receive buffer to receive new data when reading from NOR flash and the
> reading length is larger than the max_trans_len.
>>
>> What FAST READ command? Sounds like it is connected to the FLASH rather
> than SPI? if so that
>> need to go from the SPI driver and moved into some board specific addon.
>>
>> >
>> > Thanks,
>> > Mingkai
>> >
>> > ________________________________________
>> > From: Sun York-R58495
>> > Sent: Thursday, October 23, 2014 6:14 AM
>> > To: jagan Teki; Hu Mingkai-B21284
>> > Cc: Joakim Tjernlund; Hou Zhiqiang-B48286; u-boot at lists.denx.de
>> > Subject: Re: u-boot, fsl_espi.c driver
>> >
>> > Jagan and Mingkai,
>> >
>> > Would you take a look when you have a chance?
>> >
>> > York
>> >
>> >
>> > On 10/22/2014 03:12 PM, Joakim Tjernlund wrote:
>> > > York Sun <yorksun@freescale.com> wrote on 2014/10/09 20:06:31:
>> > >>
>> > >> Dear Joakim,
>> > >>
>> > >> On 10/09/2014 10:43 AM, Joakim Tjernlund wrote:
>> > >>> York Sun <yorksun@freescale.com> wrote on 2014/10/09 18:25:40:
>> > >>>>
>> > >>>> Dear Joakim,
>> > >>>>
>> > >>>> Thanks for raising a concern.
>> > >>>>
>> > >>>> It's not fair to blame the last person who submitted a patch. We
> are
>> > > all
>> > >>> working
>> > >>>
>> > >>> No of course not, I just noticed you guys had been in there and
>> > > patched up
>> > >>> some problem
>> > >>> so I hoped you would be interested to fix the remaining problems.
> This
>> > >
>> > >>> driver should never
>> > >>> have been committed in the first place.
>> > >>>
>> > >>>> to make it better as an opensource comminuty. You have done a
> good
>> > > job
>> > >>> to hack
>> > >>>> it to work. Would it be nicer if you can submit this or improved
>> > > patch
>> > >>> to u-boot
>> > >>>> community for further review and testing, after putting informing
>> > > commit
>> > >>>> message? The mailing list address is CC'ed.
>> > >>>
>> > >>> Main problem with this driver is that TX does not work for:
>> > >>>  len > max_tran_len (nor does RX)
>> > >>>  does not TX the last odd bytes(len % 4 != 0)
>> > >>>  Does not work with TX only(RX buf == NULL)
>> > >>>
>> > >>> Silently ignores SPI_LSB_FIRST
>> > >>>
>> > >>> On top of that it uses malloc all over and copies data back and
> forth
>> > > for
>> > >>> no good
>> > >>> reason, image a big SPI transfer with many MB(like my FPGA load).
>> > >>>
>> > >>> I am not in a good position fix this properly as my FPGA is TX
> only so
>> > > I
>> > >>> cannot test
>> > >>> RX at all(which is broken by my hack)
>> > >>>
>> > >>> Finally, just to illustrate the merits of this driver, after
>> > > transmission
>> > >>> is done
>> > >>> there is:
>> > >>>  if (*buffer == 0x0b) {
>> > >>>                                 data_in += tran_len;
>> > >>>                                 data_len -= tran_len;
>> > >>>                                 *(int *)buffer += tran_len;
>> > >>>                         }
>> > >>> what is the magic 0x0b test all about?
>> > >>>
>> > >>
>> > >> Thanks for the report. Driver maintainer (CC'ed) will take a look.
>> > >>
>> > >> York
>> > >
>> > > No reaction from maintainers, I don't think this driver is still
>> > > maintained.
>> > >
>> > >    Jocke
>> > >
>

thanks!
-- 
Jagan.

[U-Boot] u-boot, fsl_espi.c driver

[Joakim Tjernlund]

Jagan Teki <jagannadh.teki@gmail.com> wrote on 2014/11/18 22:31:53:
> 
> On 18 November 2014 14:42, Joakim Tjernlund
> <joakim.tjernlund@transmode.se> wrote:
> > Ping?
> >
> > Joakim Tjernlund/Transmode wrote on 2014/10/29 19:43:14:
> 
> Couldn't understand what discussion is going, does some issue in
> driver or any plan to
> write new code, please let me know - we can plan accordingly and kill
> the thread.

Basically the driver is broken for other uses than reading EEPROM:
  malloc/free should not be used.
  has a builtin FAST READ command(0xb) which I guess I related to EEPROM.
It needs to be cleaned up w.r.t above.
Then one can start fixing the other problems:
  len > max_tran_len (nor does RX) does not work
  does not TX the last odd bytes(len % 4 != 0)
  Does not work with TX only(RX buf == NULL)
  Silently ignores SPI_LSB_FIRST

 Jocke

> >>
> >> "Mingkai.Hu at freescale.com" <Mingkai.Hu@freescale.com> wrote on
> > 2014/10/28 12:17:24:
> >> >
> >> > Hi Joakim and York,
> >>
> >> Hi yourself, been travelling or a few days.
> >>
> >> >
> >> > I apologize for the delayed response and thanks for your catch up,
> > Joakim
> >> > In order to get a higher transfer speed, the eSPI controller was
> > designed to transfer multiple bytes at one transaction which is not 
comply
> > with the SPI framework and the CS can't be inactive until one transfer
> > finished, so we need to combine the multiple transfers into one 
transfer,
> > that's the reason why there are more memory copy back and forth.
> >>
> >> hmm, can't make sense of this. If you can memcpy from/to memory you 
can
> > also write/read the SPI FIFO
> >> Also writing 4 bytes to the SPI FIFO at a time will not but you much 
if
> > anything at all. It just
> >> makes it impossible to use NF flag etc.
> >> Speed will come from keeping the SPI FIFO non empty and not copying
> > memory back and fourth.
> >> CS can be controlled within the SPI framework already.
> >>
> >> >
> >> > The driver has been tested on SPI flash and the transfer length 
larger
> > than max_trans_len should be handled but half-duplex
> >> was not handled. Is it possible to provide patch to make it workable 
for
> > different devices?
> >>
> >> Not as the driver is written today.
> >>
> >> >
> >> > 0x0b is the FAST READ command. The logic here is to move the 
pointer
> > of receive buffer to receive new data when reading from NOR flash and 
the
> > reading length is larger than the max_trans_len.
> >>
> >> What FAST READ command? Sounds like it is connected to the FLASH 
rather
> > than SPI? if so that
> >> need to go from the SPI driver and moved into some board specific 
addon.
> >>
> >> >
> >> > Thanks,
> >> > Mingkai
> >> >
> >> > ________________________________________
> >> > From: Sun York-R58495
> >> > Sent: Thursday, October 23, 2014 6:14 AM
> >> > To: jagan Teki; Hu Mingkai-B21284
> >> > Cc: Joakim Tjernlund; Hou Zhiqiang-B48286; u-boot at lists.denx.de
> >> > Subject: Re: u-boot, fsl_espi.c driver
> >> >
> >> > Jagan and Mingkai,
> >> >
> >> > Would you take a look when you have a chance?
> >> >
> >> > York
> >> >
> >> >
> >> > On 10/22/2014 03:12 PM, Joakim Tjernlund wrote:
> >> > > York Sun <yorksun@freescale.com> wrote on 2014/10/09 20:06:31:
> >> > >>
> >> > >> Dear Joakim,
> >> > >>
> >> > >> On 10/09/2014 10:43 AM, Joakim Tjernlund wrote:
> >> > >>> York Sun <yorksun@freescale.com> wrote on 2014/10/09 18:25:40:
> >> > >>>>
> >> > >>>> Dear Joakim,
> >> > >>>>
> >> > >>>> Thanks for raising a concern.
> >> > >>>>
> >> > >>>> It's not fair to blame the last person who submitted a patch. 
We
> > are
> >> > > all
> >> > >>> working
> >> > >>>
> >> > >>> No of course not, I just noticed you guys had been in there and
> >> > > patched up
> >> > >>> some problem
> >> > >>> so I hoped you would be interested to fix the remaining 
problems.
> > This
> >> > >
> >> > >>> driver should never
> >> > >>> have been committed in the first place.
> >> > >>>
> >> > >>>> to make it better as an opensource comminuty. You have done a
> > good
> >> > > job
> >> > >>> to hack
> >> > >>>> it to work. Would it be nicer if you can submit this or 
improved
> >> > > patch
> >> > >>> to u-boot
> >> > >>>> community for further review and testing, after putting 
informing
> >> > > commit
> >> > >>>> message? The mailing list address is CC'ed.
> >> > >>>
> >> > >>> Main problem with this driver is that TX does not work for:
> >> > >>>  len > max_tran_len (nor does RX)
> >> > >>>  does not TX the last odd bytes(len % 4 != 0)
> >> > >>>  Does not work with TX only(RX buf == NULL)
> >> > >>>
> >> > >>> Silently ignores SPI_LSB_FIRST
> >> > >>>
> >> > >>> On top of that it uses malloc all over and copies data back and
> > forth
> >> > > for
> >> > >>> no good
> >> > >>> reason, image a big SPI transfer with many MB(like my FPGA 
load).
> >> > >>>
> >> > >>> I am not in a good position fix this properly as my FPGA is TX
> > only so
> >> > > I
> >> > >>> cannot test
> >> > >>> RX at all(which is broken by my hack)
> >> > >>>
> >> > >>> Finally, just to illustrate the merits of this driver, after
> >> > > transmission
> >> > >>> is done
> >> > >>> there is:
> >> > >>>  if (*buffer == 0x0b) {
> >> > >>>                                 data_in += tran_len;
> >> > >>>                                 data_len -= tran_len;
> >> > >>>                                 *(int *)buffer += tran_len;
> >> > >>>                         }
> >> > >>> what is the magic 0x0b test all about?
> >> > >>>
> >> > >>
> >> > >> Thanks for the report. Driver maintainer (CC'ed) will take a 
look.
> >> > >>
> >> > >> York
> >> > >
> >> > > No reaction from maintainers, I don't think this driver is still
> >> > > maintained.
> >> > >
> >> > >    Jocke
> >> > >
> >
> 
> thanks!
> -- 
> Jagan.