[U-Boot] [BUG] SPI NOR for SST25V hangs for TI AM335x.

[U-Boot] [BUG] SPI NOR for SST25V hangs for TI AM335x.

[Gautam Bhat]

Hi,

There is a bug in the SST25V SPI NOR for the TI AM335x. On a write the
SPI NOR hangs. This is because of the spi_release_bus(..) in the
spi_flash_read_common call done by spi_flash_sr_ready(..) called in
the spi_flash_wait_till_ready(..) in sst_write_wp(..). This call
clears all the registers and the setting associated with it and then
calls the spi_flash_cmd_write_disable in sst_write_wp(..) with the
wrong SPI settings.

Also there are spi_reset(..) calls happening in the sst_write_wp(..)
which seems to be unnecessary when the data sheet does not mention any
need for resets. Calling unnecessary resets simply increases the
latency in the driver and causes more bugs.

Currently commenting out the spi_release_bus(..) makes a successful write.

Thanks,
Gautam.

[U-Boot] [BUG] SPI NOR for SST25V hangs for TI AM335x.

[Faiz Abbas]

Hi,

Adding Vignesh. He should be able to help.

On Monday 21 May 2018 11:53 PM, Gautam Bhat wrote:
> Hi,
> 
> There is a bug in the SST25V SPI NOR for the TI AM335x. On a write the
> SPI NOR hangs. This is because of the spi_release_bus(..) in the
> spi_flash_read_common call done by spi_flash_sr_ready(..) called in
> the spi_flash_wait_till_ready(..) in sst_write_wp(..). This call
> clears all the registers and the setting associated with it and then
> calls the spi_flash_cmd_write_disable in sst_write_wp(..) with the
> wrong SPI settings.
> 
> Also there are spi_reset(..) calls happening in the sst_write_wp(..)
> which seems to be unnecessary when the data sheet does not mention any
> need for resets. Calling unnecessary resets simply increases the
> latency in the driver and causes more bugs.
> 
> Currently commenting out the spi_release_bus(..) makes a successful write.
> 
> Thanks,
> Gautam.
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
> 

Thanks,
Faiz

[U-Boot] [BUG] SPI NOR for SST25V hangs for TI AM335x.

[Gautam Bhat]

On Tue, May 22, 2018 at 4:17 PM, Faiz Abbas <faiz_abbas@ti.com> wrote:
> Hi,
>
> Adding Vignesh. He should be able to help.
>
> On Monday 21 May 2018 11:53 PM, Gautam Bhat wrote:
>> Hi,
>>
>> There is a bug in the SST25V SPI NOR for the TI AM335x. On a write the
>> SPI NOR hangs. This is because of the spi_release_bus(..) in the
>> spi_flash_read_common call done by spi_flash_sr_ready(..) called in
>> the spi_flash_wait_till_ready(..) in sst_write_wp(..). This call
>> clears all the registers and the setting associated with it and then
>> calls the spi_flash_cmd_write_disable in sst_write_wp(..) with the
>> wrong SPI settings.
>>
>> Also there are spi_reset(..) calls happening in the sst_write_wp(..)
>> which seems to be unnecessary when the data sheet does not mention any
>> need for resets. Calling unnecessary resets simply increases the
>> latency in the driver and causes more bugs.
>>
>> Currently commenting out the spi_release_bus(..) makes a successful write.
>>
>> Thanks,
>> Gautam.
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> https://lists.denx.de/listinfo/u-boot
>>
>
> Thanks,
> Faiz


To make things easier the following is my analysis:

In spi_flash.c:sst_write_wp(...) we have the
spi_flash_wait_till_ready(...). In this function there is a check for
spi_flash_ready(...) which calls the spi_flash_sr_ready(...). This
function checks the "status" register which finally calls the
read_sr(...). In the read_sr(...) the checking of the status register
is done by the function spi_flash_read_common(...). The problem with
this is that there is claim and especially a release call which clears
all the registers and resets the SPI block. In the subsequent call for
write disable WRDI in sst_write_wp(...) i.e.
spi_flash_cmd_write_disable(...) there is no effort to do a claim and
a release because it uses the simple spi_flash_cmd(...) call which
expects the registers to be configured and thereby goes into an
undefined state when a SPI transaction is made.

There are two problems here:

1) There is no need to claim and release just to check the status
register. The claim and release has to be done at the start and the
end of the     transaction and not in between a Auto Address Increment
word program. For this the check of the Status Register(SR) should be
a simple spi_flash_cmd call.
2) The design of the read_sr wrapper should also have a simple
spi_flash_cmd call for conditions such as this.

I have fixed this problem temporarily by writing a wrapper as in 2)
and doing a simple check of whether there is a write.

There is another problem of the writes not actually happening which I
am trying to fix now.

This problem was also raised previously in the mailing list here:
https://lists.denx.de/pipermail/u-boot/2016-June/257447.html

Thanks,
Gautam.

[U-Boot] [BUG] SPI NOR for SST25V hangs for TI AM335x.

[Gautam Bhat]

On Wed, May 23, 2018 at 11:29 AM, Gautam Bhat <mindentropy@gmail.com> wrote:
> On Tue, May 22, 2018 at 4:17 PM, Faiz Abbas <faiz_abbas@ti.com> wrote:
>> Hi,
>>
>> Adding Vignesh. He should be able to help.
>>
>> On Monday 21 May 2018 11:53 PM, Gautam Bhat wrote:
>>> Hi,
>>>
>>> There is a bug in the SST25V SPI NOR for the TI AM335x. On a write the
>>> SPI NOR hangs. This is because of the spi_release_bus(..) in the
>>> spi_flash_read_common call done by spi_flash_sr_ready(..) called in
>>> the spi_flash_wait_till_ready(..) in sst_write_wp(..). This call
>>> clears all the registers and the setting associated with it and then
>>> calls the spi_flash_cmd_write_disable in sst_write_wp(..) with the
>>> wrong SPI settings.
>>>
>>> Also there are spi_reset(..) calls happening in the sst_write_wp(..)
>>> which seems to be unnecessary when the data sheet does not mention any
>>> need for resets. Calling unnecessary resets simply increases the
>>> latency in the driver and causes more bugs.
>>>
>>> Currently commenting out the spi_release_bus(..) makes a successful write.
>>>
>>> Thanks,
>>> Gautam.
>>> _______________________________________________
>>> U-Boot mailing list
>>> U-Boot at lists.denx.de
>>> https://lists.denx.de/listinfo/u-boot
>>>
>>
>> Thanks,
>> Faiz
>
>
> To make things easier the following is my analysis:
>
> In spi_flash.c:sst_write_wp(...) we have the
> spi_flash_wait_till_ready(...). In this function there is a check for
> spi_flash_ready(...) which calls the spi_flash_sr_ready(...). This
> function checks the "status" register which finally calls the
> read_sr(...). In the read_sr(...) the checking of the status register
> is done by the function spi_flash_read_common(...). The problem with
> this is that there is claim and especially a release call which clears
> all the registers and resets the SPI block. In the subsequent call for
> write disable WRDI in sst_write_wp(...) i.e.
> spi_flash_cmd_write_disable(...) there is no effort to do a claim and
> a release because it uses the simple spi_flash_cmd(...) call which
> expects the registers to be configured and thereby goes into an
> undefined state when a SPI transaction is made.
>
> There are two problems here:
>
> 1) There is no need to claim and release just to check the status
> register. The claim and release has to be done at the start and the
> end of the     transaction and not in between a Auto Address Increment
> word program. For this the check of the Status Register(SR) should be
> a simple spi_flash_cmd call.
> 2) The design of the read_sr wrapper should also have a simple
> spi_flash_cmd call for conditions such as this.
>
> I have fixed this problem temporarily by writing a wrapper as in 2)
> and doing a simple check of whether there is a write.
>
> There is another problem of the writes not actually happening which I
> am trying to fix now.
>
> This problem was also raised previously in the mailing list here:
> https://lists.denx.de/pipermail/u-boot/2016-June/257447.html
>
> Thanks,
> Gautam.

An update. My writes are happening! Forgot to erase before write.

-Gautam.