Hi Jagan,
On 11 December 2015 at 09:57, Jagan Teki jteki@openedev.com wrote:
Hi Simon,
On 11 December 2015 at 07:35, Simon Glass sjg@chromium.org wrote:
Hi Jagan,
On 8 December 2015 at 08:36, Jagan Teki jteki@openedev.com wrote:
Hi Simon,
On 8 December 2015 at 08:22, Simon Glass sjg@chromium.org wrote:
Hi Jagan,
On 3 December 2015 at 03:10, Jagan Teki jteki@openedev.com wrote:
Hi Simon,
I re-phrase all the question from previous thread and continue in this for more discussion on spi-nor development.
Is it intended that SPI flash should be a driver for the MTD uclass? Others would be NAND and CFI. From what I can tell MTD has the same operations as SPI flash (erase, read, write) and adds a lot more.
Or is SPI flash really a separate uclass from MTD, with SPI flash being at a higher level?
Based on my "sf: MTD support" series SPI flash is a separate uclass from MTD and it uses mtd_info operations.
> cmd_sf > ======= > MTD > ======= > spi-nor or spi-flash > =============== > "spi-nor to spi drivers" and spi-nor controller driver > ======================================= Your diagram above suggests that MTD calls into SPI NOR or SPI flash, but when I look at (for exampe) spi_flash_erase(), it is calling mtd_erase(), suggesting that it is above MTD in the software stack, the opposite of your diagram above.
Will explain this clearly in below description.
Conceptually this seems problematic.
SPI flash is a uclass and supports driver model. It has operations, etc. Your patches remove the operations in favour of calling MTD. But MTD does not support driver model. This is getting really messy.
Will explain this clearly in below description.
Introducing SPI-NOR:
Some of the spi drivers or spi controllers at drivers/spi/* not a real spi controllers, unlike normal spi controllers those operates varienty of spi devices among spi-nor flash is one of them but instead these were specially designed for to operate spi-nor flash devices - these are spi-nor controllers. example: drivers/spi/fsl_qspi.c
The problem with these were sitting at drivers/spi is entire spi layer becomes spi-nor flash oriented which is absolutely wrong indication where spi layer gets effected more with flash operations - So this SPI-NOR will resolve this issue by separating all spi-nor flash operations from spi layer and by creating a generic layer called SPI-NOR core where it deals with all SPI-NOR flash activities. The basic idea is taken from Linux spi-nor framework.
SPI-NOR Block diagram:
========= cmd_sf.c =========== spi_flash.h =========== mtdcore.c =========== spi-nor.c ======================== m25p80.c fsl_qspi.c ========== =========== spi-uclass spi-nor hw ========== =========== spi_drivers =========== spi-bus hw ==========
Note:
- spi-nor.c is a common core for m25p80.c which is a spi-nor to spi driver
interface layer and for fsl_qspi.c which is a typical spi-nor controller driver.
- Challenging task is about probe.
SPI-NOR Development plan:
a) Adding MTD core support: From command point of view the spi-flash is like a mtd device having erase/write/read ops with offset, addr and size, but from lower layers the spi-flash becomes either spi-nor or spi-nand so they have their own specific features like struct spi_nor {}.
This is the reason for calling MTD from command layer and the lower layer as SPI_NOR uclass.
b) Drop spi_flash uclass: Since spi_flash is a generic term for serial flash devices among these spi-nor and spi-nand are the real device categories so add driver model to these categories.
I sent the series [1] for above a) and b)
It doesn't look like that series drops the SPI flash uclass.
Yes, I have not dropped SPI flash uclass fully on the series only ops.
c) Add spi-nor support (mean Adding SPI-NOR dm drivers) the next step.
I think this is what I am missing. If you are moving to SPI NOR, what is the API? Is it the roughly the same as the current SPI flash API? (read, write, erase)
For now SPI NOR api's are same as SPI flash in terms of structure members but spi-nor hooks, definition and the calling context are different. Usually SPI flash ops are getting called from cmd_sf ==> spi_flash.h ==> sf-uclass.c and then finally goes into sf_ops.c for actual function definitions, but in case of SPI NOR spi-nor ops (defined in spi-nor controller drivers - sf_probe.c, fsl_qspi.c) are getting called from mtd_info ops definitions which are defined at spi-nor core (which is sf_ops.c as of now), and from cmd_sf => spi_flash.h => mtdcore.c and the finally goes into spi-nor for actual mtd ops definitions.
Unlike SPI flash, SPI nor framework will separate the flow from generic spi controller which one of the connected slave as spi nor flash (through sf_probe.c) vs spi-nor controller drivers like fsl_qspi.c
Detailed SPI-NOR with examples:
include/spi_flash.h
struct spi_flash { struct spi_nor *nor; };
include/linux/mtd/spi-nor.h
struct spi_nor { struct udevice *dev; struct mtd_info *info;
// spi-nor hooks int (*read_reg)(struct udevice *dev, u8 cmd, u8 *val, int len); int (*write_reg)(struct udevice *dev, u8 cmd, u8 *data, int len);
int (*read_mmap)(struct udevice *dev, void *data, void *offset, size_t len); int (*read)(struct udevice *dev, const u8 *opcode, size_t cmd_len, void *data, size_t data_len); int (*write)(struct udevice *dev, const u8 *cmd, size_t cmd_len, const void *data, size_t data_len);
But here you show SPI nor with its own operations. So does SPI nor call into MTD? What is your plan to move MTD to driver model and create a MTD uclass? Also, does this mean that each SPI nor device will have an MTD sub-device?
Yes, SPI NOR is different from MTD and MTD operations which are defined at spi-nor will call the respective spi-nor controller driver spi-nor operations. which I explained above.
I think using MTD uclass is another approach where we can remove the udevice from SPI NOR and making all spi-nor controller driver as MTD_UCLASS - do you prefer this?
I think so, so far as I understand it. My initial concern with your approach was that you had a driver model uclass making calls into an API that was not driver-model-compatible. So creating an MTD uclass definitely seems cleaner to me. Also I've found that driver model conversion is best done starting from the bottom of the stack.
MTD uclass looks ok to me as well and with this I may drop spi_flash {} and will use mtd_info since from cmd_sf and below is sample code snippet, just written for understanding things will clear when patches submitted. Let me know your inputs on this.
cmd_sf.c
spi_flash_t *flash;
spi_flash.h
typedef struct mtd_info spi_flash_t;
static inline int spi_flash_read(spi_flash_t *flash, u32 offset, size_t len, void *buf) { // mtd_read
Looks good, but you will need an MTD uclass before doing this.
}
static inline int spi_flash_write(spi_flash_t *flash, u32 offset, size_t len, const void *buf) { // mtd_write }
static inline int spi_flash_erase(spi_flash_t *flash, u32 offset, size_t len) { // mtd_erase }
static inline int spi_flash_protect(spi_flash_t *flash, u32 ofs, u32 len, bool prot) { // mtd_lock // mtd_unlock }
include/linux/mtd/spi-nor.h
struct spi_nor { struct mtd_info *mtd;
// spi-nor hooks int (*read_reg)(struct spi_nor *nor, u8 cmd, u8 *val, int len); int (*write_reg)(struct spi_nor *nor, u8 cmd, u8 *data, int len);
int (*read_mmap)(struct spi_nor *nor, void *data, void *offset, size_t len); int (*read)(struct spi_nor *nor, const u8 *opcode, size_t cmd_len, void *data, size_t data_len); int (*write)(struct spi_nor *nor, const u8 *cmd, size_t cmd_len, const void *data, size_t data_len);
Is this intended to me a new uclass that has a different API from MTD?
};
drivers/mtd/spi-nor/spi-nor.c
static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) { // call to spi-nor erase nor->erase() }
static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, u_char *buf) { // call to spi-nor erase nor->read() }
static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen, const u_char *buf) { // call to spi-nor erase nor->erase() }
int spi_nor_scan(struct spi_nor *nor) { struct mtd_info *mtd = nor->mtd;
// read_jedec // assign mtd hooks // other spi-nor stuff}
drivers/mtd/spi-nor/m25p80.c
struct m25p80 { struct spi_nor nor; };
static int m25p80_probe(struct udevice *dev) { struct spi_slave *spi = dev_get_parentdata(dev); struct mtd_info *mtd = dev_get_uclass_priv(dev); struct m25p80 *flash = dev_get_priv(dev); struct spi_nor *nor;
nor = &flash->nor;
Is this a SPI nor device? See above question. It seems perhaps not, as dev_get_uclass_priv() returns an mtd_info, which suggests that it is an MTD uclass? This needs to be very clear...
nor->mtd = mtd; flash->spi = spi; // spi-nor hooks nor->read = nor->write = nor->read_reg = nor->write_reg = spi_nor_scan(nor); add_mtd_device(mtd);}
static const struct udevice_id m25p80_ids[] = { { .compatible = "jedec,spi-nor" }, { } };
U_BOOT_DRIVER(m25p80) = { .name = "m25p80", .id = UCLASS_MTD, .of_match = m25p80_ids, .probe = m25p80_probe, .priv_auto_alloc_size = sizeof(struct m25p80), };
drivers/mtd/spi-nor/fsl-qspi.c
struct fsl_qspi { struct spi_nor nor; };
static int fsl_probe(struct udevice *dev) { struct mtd_info *mtd = dev_get_uclass_priv(dev); struct fsl_qspi *q = dev_get_priv(dev); struct spi_nor *nor;
nor = &q->nor; nor->mtd = mtd; // spi-nor hooks nor->read = nor->write = nor->read_reg = nor->write_reg = spi_nor_scan(nor); add_mtd_device(&q->mtd);}
static const struct udevice_id fsl_qspi_ids[] = { { .compatible = "fsl,vf610-qspi" }, { } };
U_BOOT_DRIVER(fsl_qspi) = { .name = "fsl_qspi", .id = UCLASS_MTD, .of_match = fsl_qspi_ids, .probe = fsl_qspi_probe, .priv_auto_alloc_size = sizeof(struct fsl_qspi), };
[snip]
Regards, Simon