[U-Boot] [PATCH v6 1/4] mtd, spi: add MTD layer driver

[Daniel Schwierzeck]

2015-06-22 8:43 GMT+02:00 Jagan Teki <jteki at openedev.com>:
> On 16 June 2015 at 15:36, Daniel Schwierzeck
> <daniel.schwierzeck at gmail.com> wrote:
>> 2015-06-16 11:36 GMT+02:00 Jagan Teki <jteki at openedev.com>:
>>> On 16 June 2015 at 14:48, Heiko Schocher denx <hs at denx.de> wrote:
>>>> Hello Jagan,
>>>>
>>>>
>>>> Am 16.06.2015 um 10:52 schrieb Jagan Teki:
>>>>>
>>>>> Hi Heiko,
>>>>>
>>>>> On 16 June 2015 at 14:13, Heiko Schocher denx <hs at denx.de> wrote:
>>>>>>
>>>>>> Hello Jagan,
>>>>>>
>>>>>>
>>>>>> Am 16.06.2015 um 10:04 schrieb Jagan Teki:
>>>>>>>
>>>>>>>
>>>>>>> Hi Heiko,
>>>>>>>
>>>>>>> On 20 May 2015 at 12:16, Heiko Schocher <hs at denx.de> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> Hello Jagan,
>>>>>>>>
>>>>>>>> Am 19.05.2015 22:09, schrieb Jagan Teki:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Hi Heiko,
>>>>>>>>>
>>>>>>>>> I have tested this sf-mtd stuff, please see below and enabled prints
>>>>>>>>> in all the func calls.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks for testing!
>>>>>>>>
>>>>>>>>
>>>>>>>>> zynq-uboot> mtdparts add nor0 0x10000 at 0x0 env
>>>>>>>>> mtdparts variable not set, see 'help mtdparts'
>>>>>>>>> zynq-uboot> mtdparts
>>>>>>>>>
>>>>>>>>> device nor0 <zynq-sf.0>, # parts = 1
>>>>>>>>>     #: name                size            offset          mask_flags
>>>>>>>>>     0: env                 0x00010000      0x00000000      0
>>>>>>>>>
>>>>>>>>> active partition: nor0,0 - (env) 0x00010000 @ 0x00000000
>>>>>>>>>
>>>>>>>>> defaults:
>>>>>>>>> mtdids  : nor0=zynq-sf.0
>>>>>>>>> mtdparts: none
>>>>>>>>> zynq-uboot> sf erase env 0x10000
>>>>>>>>> spi_flash_erase
>>>>>>>>> spi_flash_cmd_erase_ops
>>>>>>>>> SF: erase d8  0  0  0 (0)
>>>>>>>>> SF: 65536 bytes @ 0x0 Erased: OK
>>>>>>>>> zynq-uboot> mw.b 0x100 0x44 0x10000
>>>>>>>>> zynq-uboot> sf write 0x100 env
>>>>>>>>> device 0 offset 0x0, size 0x10000
>>>>>>>>> spi_flash_cmd_write_ops
>>>>>>>>> SF: 65536 bytes @ 0x0 Written: OK
>>>>>>>>> zynq-uboot> sf read 0x40000 env
>>>>>>>>> device 0 offset 0x0, size 0x10000
>>>>>>>>> spi_flash_cmd_read_ops
>>>>>>>>> off = 0x10000, len = 0
>>>>>>>>> SF: 65536 bytes @ 0x0 Read: OK
>>>>>>>>> zynq-uboot> cmp.b 0x100 0x40000 0x10000
>>>>>>>>> Total of 65536 byte(s) were the same
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Looks good ...
>>>>>>>>
>>>>>>>>> I wonder none of the sf_mtd_info._* getting called, why?
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Hmm.. good question .. you use the "sf ..." commands, they do not
>>>>>>>> use the mtd interface, right?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> I'm fine with the testing, but mtd code in sf seems used only for in UBI
>>>>>>> only.
>>>>>>
>>>>>>
>>>>>>
>>>>>> a fast "grep mtd_read" in the u-boot source shows, that yaffs2
>>>>>> also uses mtd_read ... I did no yaffs2 test, but I think, yaffs2 can
>>>>>> be used also with spi flashes now ... if this is wise, I don;t know ...
>>>>>>
>>>>>> I tested with UBI on a spi flash, and that works ... in the same
>>>>>> fashion it currently does on nand for example ...
>>>>>>
>>>>>>> I wouldn't see this is a better approach where mtd code is considered as
>>>>>>> to
>>>>>>> be unknown thing for sf.
>>>>>>
>>>>>>
>>>>>>
>>>>>> I do not understand you here complete ...
>>>>>>
>>>>>> drivers/mtd/spi/sf_mtd.c
>>>>>>
>>>>>> adds just spi flash specific functions to integrate spi flashes
>>>>>> into mtd ... and mtd users can then read/write/erase
>>>>>> with the mtd_* functions ...
>>>>>>
>>>>>> Maybe someone has time to convert common/sf.c to use them?
>>>>>>
>>>>>> So, the final question is, can this patches go into mainline?
>>>>>
>>>>>
>>>>> The only point I'm concerned here is If I need to use sf with mtd support
>>>>> without using ubifs or any flash filesystem, the code in sf_mtd.c ops
>>>>> becomes
>>>>> unused.
>>>>
>>>>
>>>> Why you want to enable mtd support for sf, if you not use it?
>>>> do not define CONFIG_SPI_FLASH_MTD in this case?
>>>
>>> Suppose I need sf-mtd and enabled CONFIG_SPI_FLASH_MTD so
>>> when I did it from sf_cmd the code in sf-mtd.c is never been used for
>>> erase/read/write. Is that true right?
>>
>> you obviously do not understand the intention of sf_mtd. It is only an
>> adapter for translating mtd_read/mtd_write commands into
>> spi_flash_read/spi_flash_write commands. It is not intended to use it
>> within sf_cmd or the SPI flash subsystem. Such an adapter is needed
>> for subsystems like UBI which can only operate on top of the MTD
>> layer. Thus if you want to use UBI on SPI flash, you will need sf_mtd.
>> And using UBI on SPI flash is a legitimate use-case for many users.
>
> Say, for example I'm trying to do raw mtd partition without use of any flash
> filesystem say UBI. for this also I must t register the spi flash to mtd right?
>
> So I will enable CONFIG_SPI_FLASH_MTD and add the mtd partitions

If you want to use cmd_mtdparts with SPI flash, you have to enable
CONFIG_SPI_FLASH_MTD to have your SPI flash device registered with the
MTD layer.

>
> zynq-uboot> mtdparts add nor0 0x10000 at 0x0 env
> mtdparts variable not set, see 'help mtdparts'
>
> and then I will able to do partition read/write using sf_cmd.

No. cmd_mtdparts operates on the MTD device list. That does not mean
that sf_cmd have to use mtd_read()/mtd_write() functions. Heiko only
added an option for convenience to use MTD partitions with sf_cmd. But
that option only derive offset and size from a MTD partition to
delegate it to spi_flash_read()/spi_flash_write(). That is a
difference to cmd_nand, where nand_read()/nand_write() always delegate
to mtd_read()/mtd_write().

>
> From your point above "It is not intended to use it within sf_cmd or
> the SPI flash subsystem."
>
> If I didn't enable or register with mtd, how come the device gets register
> with mtd and how could I add the partitions?
>
> zynq-uboot> mtdparts add nor0 0x10000 at 0x0 env
> mtdparts variable not set, see 'help mtdparts'
> Device nor0 not found!
>
> So my point was if I do a raw mtd partition with spi flash I must use sf_mtd
> but there is no usage of sf_mtd_info ops code.
>
> I'm just concentrated on raw mtd usage on spi flash, think on that area and
> let me know your inputs.

yes for cmd_mtdparts you will need sf_mtd to have your SPI flash
device registered with the MTD layer because cmd_mtdparts operates on
the global MTD device list. The callbacks _read()/_write()/_erase() in
the mtd_info struct are unused then. Are you concerned about four
unused callback functions with minimal code footprint in your use
case? In that case we could add a CONFIG_SPI_FLASH_MTD_OPS option
which conditionally compile the callbacks.

Maybe it makes sense in the long term if all flash media is
consistently accessed through the MTD layer especially in conjunction
with driver model and device-tree. If we can replace cmd_nand,
cmd_flash, cmd_sf etc. with a single cmd_mtd respectively env_nand,
env_flash, env_sf with a single env_mtd we could likely reduce the
code footprint. But that discussion is independent from the sf_mtd
patch.

-- 
- Daniel