[BUG] Cadence QSPI broken since barebox-2019.06.0?

[Ian Abbott <abbotti@mev.co.uk>]

Hi all,

I'm using Barebox on a Cyclone5 SoCFPGA system, booting from QSPI NOR 
flash (name "s25fl256s1"), using the Cadence QSPI driver.

All is fine with barebox 2019.05.0, but with barebox 2019.06.0 I get the 
following errors from xload when it tries to probe the QSPI flash device:

cadence_qspi cadence_qspi0: Spansion Quad bit not set
cadence_qspi cadence_qspi0: Spansion quad-read not enabled
cadence_qspi cadence_qspi0: quad mode not supported
cadence_qspi cadence_qspi0: probing for flashchip failed
cadence_qspi cadence_qspi0: Cadence QSPI NOR probe failed
cadence_qspi cadence_qspi0: probe failed: error 22

I think there was a nasty bug introduced in commit 5085d2ef3fbf ("mtd: 
spi-nor: cadence: add cqspi_set_protocol") leading to corruption of the 
`struct cqspi_st` private data structure in 
"drivers/mtd/spi-nor/cadence-quadspi.c".  The bug is in the handling of 
the `current_cs` member of `struct cqspi_st`:

1. `cqspi_probe()` initializes the `current_cs` member to -1.

2. `cqspi_probe()` calls `cqspi_setup_flash()` for each chip select.

3. `cqspi_setup_flash()` calls `spi_nor_scan()` to look probe for the 
SPI-NOR flash chip.

4. `spi_nor_scan()` calls `spi_nor_read_id()` to read the JEDEC ID.

5. `spi_nor_read_id()` calls `nor->read_reg()`, which is 
`cqspi_read_reg()`, to read the JEDEC ID registers.

6. `cqspi_read_reg()` calls new function `cqspi_set_protocol()`.

7. `cqspi_set_protocol()` starts with the following code:

	struct cqspi_st *cqspi = nor->priv;
	struct cqspi_flash_pdata *f_pdata;

	f_pdata = &cqspi->f_pdata[cqspi->current_cs];

	f_pdata->inst_width = CQSPI_INST_TYPE_SINGLE;
	f_pdata->addr_width = CQSPI_INST_TYPE_SINGLE;
	f_pdata->data_width = CQSPI_INST_TYPE_SINGLE;

   However, `cqspi->current_cs` still has the initial value of -1, so 
this will corrupt the parts of `struct cqspi_st` that precedes the 
`f_pdata[]` member.  (It is probably the `fifo_depth` member that will 
be corrupted by the above code.)

Even if `cqspi->current_cs` is not -1, it will be the previous chip 
select, not the current chip select, so is wrong anyway.

To fix the bug, probably the first thing that `cqspi_set_protocol()` 
should do is call `cqspi_configure()`, which will update the 
`current_cs` member to the correct value, select the correct chip select 
and update the baudrate divisor and delays.  (It doesn't look like 
`cqspi_configure()` is affected by any changes to the `inst_width`, 
`addr_width` or `data_width` values.) The bug fix _could_ do some error 
checking first, but what it definitely should not do is update the 
members of `cqspi->f_pdata[cqspi->current_cs]` until `current_cs` before 
`cqspi_current_cs` has been updated.

There may be other problems with the commit too.  `cqspi_set_protocol()` 
modifies `f_pdata->data_width` according to the current read protocol 
(single, dual or quad mode), but it doesn't modify `f_pdata->addr_width` 
according to the current addressing mode.  This might make 
`cqspi_calc_rdreg()` return the wrong value.  (`cqspi_calc_rdreg()` is 
the only function that uses the `f_pdata->addr_width` value.  Other 
functions use `nor->addr_width` instead.)

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268.  Registered address:    )=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox