Hi Jagan, On 17 December 2015 at 10:06, Jagan Teki <[email protected]> wrote: > Hi Simon, > > On 15 December 2015 at 03:44, Simon Glass <[email protected]> wrote: >> Hi Jagan, >> >> On 11 December 2015 at 09:57, Jagan Teki <[email protected]> wrote: >>> Hi Simon, >>> >>> On 11 December 2015 at 07:35, Simon Glass <[email protected]> wrote: >>>> Hi Jagan, >>>> >>>> On 8 December 2015 at 08:36, Jagan Teki <[email protected]> wrote: >>>>> Hi Simon, >>>>> >>>>> On 8 December 2015 at 08:22, Simon Glass <[email protected]> wrote: >>>>>> Hi Jagan, >>>>>> >>>>>> On 3 December 2015 at 03:10, Jagan Teki <[email protected]> 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. > > Yes. > >> >>> } >>> >>> 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... > > struct spi_nor is a low level device for mtd other wards from user > point-of-view, user doing an mtd operation on mtd flash device which > is SPI nor in this case. Since the driver is MTD uclass, driver probe > must assign hooks to spi_nor ops but struct spi_nor is not a uclass.
OK, so are you working on a uclass for MTD? Regards, Simon _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
