Re: [PATCH] mtd: spi-nor: fix options for mx66l51235f

[Marek Vasut <marek.vasut@gmail.com>]

On 08/29/2018 01:00 PM, Alexander Amelkin wrote:
> 
> 28.08.2018 19:03, Marek Vasut wrote:
>> 
>>>>> Probably it would be best not to merge this patch into Linux 
>>>>> kernel. I'll discuss it with colleagues from OpenPOWER
>>>>> community and we'll probably fix it on SBE side.
>>>> Can you fix the firmware ? If so, that'd be amazing.
>>> It turns out that it is impossible to fix that on firmware side 
>>> mainly because the firmware does not send any opcodes over SPI
>>> at all. It relies on the SPI flash controller to do the address
>>> mapping so the firmware accesses the chip as a usual memory
>>> address range. The flash controller is located in our case inside
>>> Aspeed AST2400 BMC SoC and does not support 4B opcodes. It
>>> expects the chip to respond to generic READ and WRITE opcodes
>>> with (configurable) either 3 or 4 address byte cycles. So the
>>> only way to make it work is put the chip in EN4B mode when BMC
>>> boots.
>> And who does that ? Or is the EN4B set by default and U-Boot/Linux 
>> _clears_ it , thus making the board unbootable ?
>> 
>> Doesn't it simply mean your board is missing SPI NOR hardware reset
>> line?
> 
> No, you're probably just missing it. In servers the BMC is always on
> when PSUs are connected to mains. The flash chip in question is the
> one used to boot the host (OpenPOWER P8), not the BMC (ASpeed
> AST2400).

OK

> U-boot on AST2400 has nothing to do with the chip. When BMC
> boots, it loads a driver for the host's chip (because it is connected
> to the BMC SoC for the BMC to be able to upgrade the host's
> firmware), and the driver initializes the chip to the right mode. The
> BMC reads host firmware version info, etc. from the chip. It may also
> update the firmware or parts of it. Then it powers up the host and
> the host reads the chip via the memory window mapped on LPC bus by
> the BMC.

OK

> The BMC hardware (AST2400 in our case) translates memory
> transactions into SPI Flash commands. Unfortunately for us, AST2400
> (unlike newer models) only supports generic 3-byte commands (READ,
> READ FAST, PP, etc.), but has a 'hack' that allows for using 4-byte
> addresses with those commands. Thus if an over-16MB flash chip is
> connected (and the firmware image itself is over 16MB), and thus
> AST2400 is configured in 4-byte address cycle mode, it is crucial
> that the flash chip is in EN4B mode as well. That's what was provided
> by the spi-nor driver before Roman Yeryomin's commit and by this
> patch that reverts that commit. When OpenBMC project upgraded from
> kernel 4.13 to 4.17, it sucked in the commit made in version 4.15 and
> thus broke the mechanism I described above.

Ah, so the whole problem is, the ASpeed 2400 SPI NOR controller does
memory-mapped read from the flash and that has some limitations when the
flash is over 16 MiB in size -- it sends 3 byte standard opcodes with 4
byte address ?

> The flash chip in question does have a hardware reset line, but it
> does not matter in this case at all. Resetting the chip will bring it
> into EX4B (3-byte) mode, and that's not what is needed. Basically,
> that's exactly what is happening now (without this patch).
> 
> By the way, the newer AST2500 BMC chip has support for 4-byte
> commands and most probably does not have this problem.

I see

>>> As this is quite a specific case, I guess you won't be willing to
>>> remove the SPI_NOR_4B_OPCODES option globally. We'll have to keep
>>> the patch local to openbmc project
>> I am more worried about this being actually unfixable in reliable
>> way.
> Well, configuring the chip for EN4B mode before powering on the host
> is quite reliable. There is no-one to interfere with this setup
> in-between and even after the host is booted, as there is no driver
> in Linux for AST2400-based SPI controller on LPC bus (as seen from
> the host). There is only driver for that peripheral as seen from
> BMC.

To simplify the situation, if the ASpeed 2400 SPI NOR controller tried
reading the SPI flash past 16 MiB, it would also require that the flash
is in EN4B mode, yes ?

>>>> But still, there can be a software which does rogue
>>>> transmission on the SPI bus and flips the flash into 3B mode
>>>> before reset, so the system will not boot anyway. Do you have a
>>>> way to deal with that somehow ?
>>> Sure. As I said, there is no way to turn on the host without BMC 
>>> knowing/controlling it. The BMC ensures the chip is in the right 
>>> mode. That's actually the reason why this patch appeared in the
>>> first place. Also, there is no rogue software on BMC.
>> OK, but if the BMC puts the chip in a defined state, how come the
>> system will refuse to (re)boot if Linux interferes with the chip's
>> volatile register settings ?
>
> Because it was BMC's Linux that got broken, not host's. The commit
> fetched into BMC's linux while upgrading 4.13 to 4.17 resulted in
> putting the chip in a wrong state.
> 
> I start thinking about maybe adding an option to "jedec,spi-nor" DTS
> binding like "enable-4byte-mode", so that we could get rid of our
> local patch and just specify that option in the devicetree. The
> option would force EN4B on the chip if it supports that mode. What do
> you think? How do I do it if you approve? Send patches separately in
> here and in the devicetree mailing list or cross-post both patches to
> both lists?

That's an option. But what I think this is really about is a fact that
we completely lack any way to negotiate limitations between the SPI NOR
and the controller.

-- 
Best regards,
Marek Vasut