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

[Daniel Schwierzeck]

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.

BTW: this is the same approach as for CFI NOR flash. The CFI driver
regularly works without MTD support, but you can enable the optional
cfi_mtd driver if you need it.

>
>>
>>> This seems to be a code size increasing factor which is obviously not a
>>> good
>>> point for bootloader atleast for u-boot, what do you think?
>>>
>>> I agreed your concerned for someone may add support to common/cmd_sf.c
>>> in future, but I'm bit worried to go this.
>>
>>
>> Why? Its the same state as it is in the nand subsystem ...
>
> Agreed but until unless if you have filesystem link to cmd_sf to partitioning.

Again sf_mtd is an optional driver for users who need MTD support on
SPI flash. All others do not need to enable sf_mtd. Thus there is no
increasing of code size.

-- 
- Daniel