[U-Boot] dm: Introduce SPI-NOR framework
Jagan Teki
jteki at openedev.com
Thu Dec 17 18:06:05 CET 2015
Hi Simon,
On 15 December 2015 at 03:44, Simon Glass <sjg at chromium.org> wrote:
> Hi Jagan,
>
> On 11 December 2015 at 09:57, Jagan Teki <jteki at openedev.com> wrote:
>> Hi Simon,
>>
>> On 11 December 2015 at 07:35, Simon Glass <sjg at chromium.org> wrote:
>>> Hi Jagan,
>>>
>>> On 8 December 2015 at 08:36, Jagan Teki <jteki at openedev.com> wrote:
>>>> Hi Simon,
>>>>
>>>> On 8 December 2015 at 08:22, Simon Glass <sjg at chromium.org> wrote:
>>>>> Hi Jagan,
>>>>>
>>>>> On 3 December 2015 at 03:10, Jagan Teki <jteki at 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.
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.
>
>>
>> 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),
>> };
>>
thanks!
--
Jagan.
More information about the U-Boot
mailing list