Hi Simon,
On 6 January 2016 at 05:54, Simon Glass <s...@chromium.org> wrote:
> Hi Jagan,
>
> On 17 December 2015 at 10:06, Jagan Teki <jt...@openedev.com> wrote:
>> Hi Simon,
>>
>> On 15 December 2015 at 03:44, Simon Glass <s...@chromium.org> wrote:
>>> Hi Jagan,
>>>
>>> On 11 December 2015 at 09:57, Jagan Teki <jt...@openedev.com> wrote:
>>>> Hi Simon,
>>>>
>>>> On 11 December 2015 at 07:35, Simon Glass <s...@chromium.org> wrote:
>>>>> Hi Jagan,
>>>>>
>>>>> On 8 December 2015 at 08:36, Jagan Teki <jt...@openedev.com> wrote:
>>>>>> Hi Simon,
>>>>>>
>>>>>> On 8 December 2015 at 08:22, Simon Glass <s...@chromium.org> wrote:
>>>>>>> Hi Jagan,
>>>>>>>
>>>>>>> On 3 December 2015 at 03:10, Jagan Teki <jt...@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.
>
> OK, so are you working on a uclass for MTD?
Yes.
thanks!
--
Jagan.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
