[U-Boot] dm: Introduce SPI-NOR framework
Simon Glass
sjg at chromium.org
Mon Dec 14 23:14:31 CET 2015
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.
> }
>
> 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
More information about the U-Boot
mailing list