[U-Boot] [RFC] [PATCH 0/4] Add support for Freescale's 85xx and P1/P2xxx eSPI controller

[Can Aydin <can.aydin@locatacorp.com>]

  Dear Reinhard,

On 28/09/2010 9:02 PM, Reinhard Meyer wrote:
> Dear Can Aydin,
>> The reason this is an RFC is that unfortunately the hardware on these
>> chips does not permit indefinite SPI transactions on a given chip
>> select. A chip select is asserted only when a 'transaction length' has
>> been passed to the controller. Once the number of characters specified
>> in the transaction length have been transmitted, the controller decides
>> that the 'frame' has ended and de-asserts the chip select after a
>> defined delay. It is not possible to initiate a second transfer without
>> re-initialising the command register, and hence clearing and
>> re-asserting a chip select signal.
>>
>> This patch set addresses the issue by defining a read/write function in
>> the spi_flash_internal API. Subsequent patches add the freescale eSPI
>> driver and add support for it in the spansion driver and the P1/P2 board
>> configuration header.
>>
>> I'm pretty sure that there are better ways of doing this, especially if
>> a driver model with support for driver quirks was implemented for
>> instance. Until then however, I assume having some sort of ability to
>> use the SPI controller on these boards would be better than not being
>> able to do anything at all. If anyone has a better solution please feel
>> free to comment.
> Can the Chip Select Pins be used as GPIO? It might be simpler to do 
> that than
> adding a speciality to common code.
True, a GPIO pin could be re-purposed to serve as chip select, but that 
would involve a soldering iron and a steady hand as the P1/P2xxRDB 
boards are reference design boards sold by a lot of third party vendors 
(including Denx if I'm not mistaken) this would detract from the ability 
to just plug u-boot in and run with it.

People could also choose to bitbang the SPI on their custom hardware but 
again I feel that might defeat the purpose and spirit of u-boot. (Then 
again, I'm new so don't quote me on that).
>
>>    mode change 100644 =>  100755 drivers/mtd/spi/spansion.c
>>    mode change 100644 =>  100755 drivers/mtd/spi/spi_flash.c
>>    mode change 100644 =>  100755 drivers/mtd/spi/spi_flash_internal.h
>>    create mode 100755 drivers/spi/fsl_espi.c
>>    create mode 100755 include/fsl_espi.h
> Having edited from W****** using Samba?
> Make sure the file modes are and stay 644!
Dammit, I was hoping to not let on about that. Yes, I am unfortunately 
confined to a certain OS due to certain factors. Will fix those.
>
> Without looking at the functionality I saw several coding style issues in
> the patch series:
> +    if ( cmd_len&&  cmd )
> +        memcpy(buffer, cmd, cmd_len);
> should be like "if (cmd_len && cmd)"
>
> Similar space issues seem to happen all over your patches,
> examples:
>
> +    for ( i = 0; i<  size; i++)
>
> +    return bus == 0&&  cs<  ESPI_MAX_CS_NUM;
>
> +#define ESPI_COM_CS(x)        ((x)<<  30)
>
> You need to fix that globally...
Again I plead newbie. Will fix also.
>
> Best Regards,
>
> Reinhard
>
> PS: you might want to wait and see if that patch in common code to
> fix a special hardware issue is welcome at all ;)
>
Well, even if the common code patch is rejected (and breaking the 
spi_flash abstraction was definitely not the high point of my day), at 
least the cmd_spi module will work with these boards if only the driver 
patch gets in. Even that might help those trying to get their own hw off 
the ground at least somewhat.