Message-ID: <87efjnvpjl.fsf@notabene.neil.brown.name>
Date:   Tue, 10 Apr 2018 11:05:50 +1000
From:   NeilBrown <neil@...wn.name>
To:     Marek Vasut <marek.vasut@...il.com>,
        Cyrille Pitchen <cyrille.pitchen@...ev4u.fr>
Cc:     David Woodhouse <dwmw2@...radead.org>,
        Brian Norris <computersforpeace@...il.com>,
        Boris Brezillon <boris.brezillon@...e-electrons.com>,
        Richard Weinberger <richard@....at>,
        linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mtd: spi-nor: clear Extended Address Reg on switch to 3-byte addressing.

On Mon, Apr 09 2018, Marek Vasut wrote:

> On 04/08/2018 11:56 PM, NeilBrown wrote:
>> On Sun, Apr 08 2018, Marek Vasut wrote:
>> 
>>> On 04/08/2018 09:04 AM, NeilBrown wrote:
>>>>
>>>> According to section
>>>>    8.2.7 Write Extended Address Register (C5h)
>>>>
>>>> of the Winbond W25Q256FV data sheet (256M-BIT SPI flash)
>>>>
>>>>    The Extended Address Register is only effective when the device is
>>>>    in the 3-Byte Address Mode.  When the device operates in the 4-Byte
>>>>    Address Mode (ADS=1), any command with address input of A31-A24
>>>>    will replace the Extended Address Register values. It is
>>>>    recommended to check and update the Extended Address Register if
>>>>    necessary when the device is switched from 4-Byte to 3-Byte Address
>>>>    Mode.
>>>>
>>>> This patch adds code to implement that recommendation.  Without this,
>>>> my GNUBEE-PC1 will not successfully reboot, as the Extended Address
>>>> Register is left with a value of '1'. When the SOC attempts to read
>>>> (in 3-byte address mode) the boot loader, it reads from the wrong
>>>> location.
>>>
>>> Your board is broken by design and does not implement proper reset logic
>>> for the SPI NOR chip, right ? That is, when the CPU resets, the SPI NOR
>>> is left in some random undefined state instead of being reset too, yes?
>> 
>> Thanks for the reply.
>
> Sorry for the delay.
>
>> "Broken" is an emotive word :-)  Certain the board *doesn't* reset the NOR
>> chip on reset.
>
> It's not emotive, it is descriptive of what it really is. Such boards
> which do not correctly reset the SPI NOR can, during ie. reset, get into
> a state where the system is unbootable or corrupts the content of the
> SPI NOR. This stuff came up over and over on the ML, it seems HW
> designers never learn.

Ok, the board is broken.
Still, Linux has a history of even supporting broken hardware -  Spectre
and Meltdown for example.
I wouldn't propose a fix that harms the kernel in any way (hurts
maintainability or negatively affect other hardware), but I don't
think my proposed patch does.

>
>> However the NOR chip doesn't have a dedicated RESET pin. It has a pin
>> that defaults to "HOLD" and can be programmed to act as "RESET".  This
>> pin is tied to 3V3 on my board. If it were tied to a reset line, it
>> would still need code changes to work (or switch from the default).
>
> I'm afraid this needs HW changes. The solution for SPI NORs without a
> dedicated reset line is to just hard cut the power to them for a while
> in case the CPU core reset out is asserted.
>
>> A few month ago:
>> Commit: 8dee1d971af9 ("mtd: spi-nor: add an API to restore the status of SPI flash chip")
>> Commit: 59b356ffd0b0 ("mtd: m25p80: restore the status of SPI flash when exiting")
>
> This works when reloading the driver, but not when restarting the system.

I don't understand what you are saying.
The second patch provides a ->shutdown function for the spi_driver.
This is called by spi_drv_shutdown which is called by the driver
->shutdown function, which is called by device_shutdown(), which
is only called by
  kernel_shutdown_prepare() and
  kernel_restart_prepare().

So it works exactly when restarting the system, and not when reloading
the driver.

>
>> were added to Linux.  They appear to be designed to address a very
>> similar situation to mine.  Unfortunately they aren't complete as the
>> code to disable 4-byte addressing doesn't follow documented requirements
>> (at least for winbond) and doesn't work as intended (at least in one
>> case - mine). This code should either be fixed (e.g. with my patch), or removed.
>> 
>>>
>>> Doesn't this chip support 4-byte addressing opcodes ? If so, we should
>>> use those and keep the chip in 3-byte addressing mode. Would that work?
>> 
>> Yes and no.
>> If I
>> -	{ "w25q256", INFO(0xef4019, 0, 64 * 1024, 512, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
>> +	{ "w25q256", INFO(0xef4019, 0, 64 * 1024, 512,
>> +			  SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>> +			  SPI_NOR_4B_OPCODES) },
>> 
>> then I can still read all the flash and it never gets switched to
>> 4-byte mode.
>
> Yes, dedicated opcodes are the way to go ... although, it doesn't solve
> the reset problem completely. Imagine your CPU ie. restarts during a
> page program operation and then the BootROM sends data over the SPI.
> Those data might get written into the SPI NOR, thus corrupting the
> content.

What, apart from power loss, would restart the CPU??
I guess the watchdog could do that - so I would not enable the watchdog
timer on this board unless I could disable it during a NOR write cycle.

Even if I we cannot, in software, fix all the down-sides of not having a
reset line, I think there is still a case for fixing anything that can
easily be fixed.

>
>> However, if the last address read from the flash is beyond 16M, the
>> extended address register gets implicitly set to 1, and reboot doesn't
>> work.
>> i.e. the problem isn't 4-byte mode exactly.  The problem is the Extended
>> Address Register being set implicitly, and not being zero at reboot.
>
> Uh oh, I seems to remember something like this with the winbond flash.
> I think this was also the reason why zynqmp couldn't boot from those,
> because it was somehow weirdly configured by default.

I've since discovered another aspect of the weirdness.
Switching from 4-byte mode to 3-byte mode sets the extended address
register to 1.  I'm happy to call that "broken hardware".
So for this chip, the correct sequence for switching to 3-byte mode is:
  - send the "switch to 3-byte mote" command
  - write 0 to the extended address register.
 

>
>> It looks like we need to clear the extended address register before
>> reboot, either by:
>>  - software-reset the flash at shutdown
>
> Doesn't work if CPU resets without executing this hook.
>
>>  - write '0' in the shutdown handler
>
> See above
>
>>  - write '0' after every transfer (or every transfer beyond 16M).
>
> What happens if CPU resets during the transfer ? System fails to boot.
>
>> Which would you prefer, or is there another option?
> Neither, and I am really sorry I don't have a suggestion for you here.
> But I think there might be something eluding me regarding this winbond
> extended something register. How is the behavior of the chip different
> exactly from a convention > 16 MiB SPI NOR (ie. Spansion one) ?

I'm sorry but I cannot answer that.  I don't have a Spansion chip and
I've never worked with nor flash before.
What I know is that in the Winbond W25Q256FV

- There is an "Extended address register" (EAR) which is used for the
  top byte when 3-byte addressing is used.
- The EAR is set to 1 when switching from 4-byte to 3-byte mode, and
  set to zero at power-on.
- When using a 4-byte-address opcode, the high byte is copied into
  the EAR on each access.

(I examined http://www.cypress.com/file/177966/download which seems to be
 a data sheet for the spansion S25FL256S and it describes a single bit
 in the BAR (Bank Address Register) which corresponds to the Winbond
 Extended address register.  Presumably is does not get updated
 implicitly in the same way that the Winbond EAR does)

Also on my board
- the SOC boots from the NOR flash in 3-byte mode
- the hold/reset line for the NOR flash is wired to 3v3.

I cannot defend any of this, nor can I change it.  I just want to make
it work as best I can.  The patch I provided does fix the one
problematic symptom that I can easily reproduce.  Do you have any
objection to the actual code?

Thanks,
NeilBrown

Download attachment "signature.asc" of type "application/pgp-signature" (833 bytes)

Powered by blists - more mailing lists