On 4 July 2017 at 02:00, Cyrille Pitchen [off-list ref] wrote:

Hi Matthew,


Le 26/06/2017 à 18:13, matthew.gerlach@linux.intel.com a écrit :

quoted

From: Matthew Gerlach <redacted>

quoted

+static int altera_quadspi_setup_banks(struct device *dev,
+                                   u32 bank, struct device_node *np)
+{
+     struct altera_quadspi *q = dev_get_drvdata(dev);
+     struct altera_quadspi_flash *flash;
+     struct spi_nor *nor;
+     int ret = 0;
+     char modalias[40] = {0};
+     struct spi_nor_hwcaps hwcaps = {
+             .mask = SNOR_HWCAPS_READ |
+                     SNOR_HWCAPS_READ_FAST |
+                     SNOR_HWCAPS_READ_1_1_2 |
+                     SNOR_HWCAPS_READ_1_1_4 |
+                     SNOR_HWCAPS_PP,
+     };

since aletera_quadspi_{read|erase} just don't care about
nor->read_opcode, nor->program_opcode and so on and anyway override all
settings chosen by spi-nor.c, it means they will use Dual or Quad SPI
controllers as they want, whether SNOR_HWCAPS_READ_1_1_{2|4} are set or not.
Then I think it's risky to declare the READ_1_1_2 and READ_1_1_4 hwcaps
because it may trigger additionnal calls of nor->read_reg() /
nor->write_reg() from spi_nor_scan() with op codes not supported by
altera_quadspi_{read|write}_reg().

quoted

+
+     if (bank > q->num_flashes - 1)
+             return -EINVAL;
+
+     altera_quadspi_chip_select(q, bank);
+
+     flash = devm_kzalloc(q->dev, sizeof(*flash), GFP_KERNEL);
+     if (!flash)
+             return -ENOMEM;
+
+     q->flash[bank] = flash;
+     nor = &flash->nor;
+     nor->dev = dev;
+     nor->priv = flash;
+     nor->mtd.priv = nor;
+     flash->q = q;
+     flash->bank = bank;
+     spi_nor_set_flash_node(nor, np);
+
+     /* spi nor framework*/
+     nor->read_reg = altera_quadspi_read_reg;
+     nor->write_reg = altera_quadspi_write_reg;
+     nor->read = altera_quadspi_read;
+     nor->write = altera_quadspi_write;
+     nor->erase = altera_quadspi_erase;
+     nor->flash_lock = altera_quadspi_lock;
+     nor->flash_unlock = altera_quadspi_unlock;

nor->flash_lock and nor->flash_unlock are described as "FLASH SPECIFIC"
in include/linux/mtd/spi-nor.h as opposed to "DRIVER SPECIFIC" functions
like nor->read, nor->read_reg, ...

It means the actual implementations should be provided by the spi-nor
sub-system but not by each SPI controller driver.



For me, it really sounds like a bad idea that this driver tries so much
to mystify the spi-nor sub-system.

I can understand that you have to cope with the hardware design and its
limitations but clearly it looks the spi-nor API is not suited to this
hardware. This driver ignores and by-passes any settings selected by
spi_nor_scan().
Duplicating code is generally a bad idea but in this case, I don't know
if trying to reuse spi_nor_read() / spi_nor_write() and spi_nor_erase()
from spi-nor.c is that helpful.

Why not directly plug your driver into the above mtd layer implementing
you own version of mtd->_read(), mtd->_write() and mtd->_erase() then
registering the mtd device? It may be not the way to go but at least we
should study this alternative.

AFAICT fsl-quadspi does just that preventing the use of the SPI
controller for non-flash devices.

There is at least one accelerated driver that is passed the opcodes to
program in the controller for read acceleration in spi_flash_read so
reusing that should be viable. If the opcodes can be programmed or
match what is hardcoded in the controller use the acceleration and
fallback to plain spi transfer if there is mismatch between what
m25p80_read requests and what the controller can do.

If this works and you can still use the plain SPI trnsfers the
controller will be much morer useful than fsl-quadspi.

Thanks

Michal