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

Jagan Teki jteki at openedev.com
Mon Jun 22 21:59:34 CEST 2015 

On 22 June 2015 at 17:23, Daniel Schwierzeck
<daniel.schwierzeck at gmail.com> wrote:
> 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.

I just calculate the text size of elf it seems ~290 bytes got increased with
raw mtd partitioning use case, so I'm going without using any macro.

>
> 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.

I will send the same series again with some useful comments and one typo
fix, please find that re-comment so-that I shall pull these stuff on my tree.

thanks!
-- 
Jagan | openedev.

More information about the U-Boot mailing list