[U-Boot] [PATCH v7 00/34] sf: MTD support

[Simon Glass]

Hi Jagan,

On 26 November 2015 at 10:54, Jagan Teki <jteki at openedev.com> wrote:
> Hi Simon,
>
> Some how I'm unclear about your comments in previous series probably I
> my misunderstanding or something. let me explain about my plan on
> spi-nor development.
>
> The entire spi-flash code is generic for all means there is no
> separate code for any platform or device. So I call it as spi-flash
> core. spi-flash core having functionaries like read_jedec, flash
> read/write/erase all these were calling from cmd_sf using dm and
> non-dm version, here I'm replacing this MTD since it is core interface
> it doesn't handle any specific device or uclass and mtd operations are
> being used directly without any dm except the probe.
>
> So spi-flash shouldn't need dm and the respective probe sf_probe will
> follow dm as it allocates spi_flash and by taking spi_slave{} setup
> spi-uclass. spi-flash operation are common and there is no different
> ops for different drivers or devices or something and mtd_ops are
> manged directly from cmd_sf like nand and spi-nor in Linux.
>
> And once core sits stable spi-flash will implemented as spi-nor core
> and the bellowed drivers becomes spi-nor drivers like spi-nor to spi
> driver interface and spi-nor controller drivers these are dm-driven
> it's having dm ops similar to spi-flash ops and spi-flash no where
> required.
>
> cmd_sf
> =======
> MTD
> =======
> spi-nor or spi-flash
> ===============
> "spi-nor to spi drivers" and spi-nor controller driver
> =======================================
>

I think having read through the series and your emails I am starting
to understand things.

But I'm still not quite there...

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?

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.

>
> On 26 November 2015 at 23:20, Simon Glass <sjg at chromium.org> wrote:
>> +Fabio
>>
>> Hi Jagan,
>>
>> On 26 November 2015 at 04:03, Jagan Teki <jteki at openedev.com> wrote:
>>> This series is combination of mtd and sf tunning stuff in previous
>>> version patches.[1][2]
>>>
>>> This is whole patch series for add mtd support to spi-flash
>>> framework and related stuff.
>>>
>>> The idea is to introduce the spi-nor flash framework which
>>> similar to Linux with driver-model support.
>>>
>>> Detail changes:
>>> - drivers/mtd/spi/sf_probe.c: spi-flash to spi drivers interface(dm and non-dm)
>>> - drivers/mtd/spi/sf_ops.c: Core spi-flash functionalities.
>>> - spi_flash ops and dm_spi_ops are not needed as flash opertaion are
>>>   common for dm and non-dm via MTD
>>>
>>> Changes in v7:
>>> - Rebase to master
>>> - Added MTD core support to dataflash
>>> - Few patch bisectable separations
>>>
>>> Changes in v6, v5, v4, v3, v2:
>>> - One patch bisectable separation
>>> - Rebase to master
>>> - added newly mtd stuff patches.
>>>
>>> Testing:
>>> $ git clone git://git.denx.de/u-boot-spi.git
>>> $ cd u-boot-spi
>>> $ git checkout -b spi-nor-mtd origin/next-spi-nor-mtd
>>>
>>> [1] http://u-boot.10912.n7.nabble.com/PATCH-v6-00-23-sf-MTD-support-td233769.html
>>> [2] http://lists.denx.de/pipermail/u-boot/2015-October/229857.html
>>>
>>> thanks!
>>> Jagan.
>>
>> This seems to build on the patch which adds the flash locking stuff
>> for non-DM SPI flash - commit c3c016cf. When I asked about this a week
>> ago Tom mentioned that Fabio was going to fix this up so that it
>> supports driver model.
>>
>> As I tried to explain last time I reviewed this, the problem with this
>> series is that it adds new functionality to non-DM code. In fact this
>> series takes a backwards step, since it plumbs in DM SPI flash to
>> non-DM SPI MTD, since the latter does not support driver model.
>
> "adds new functionality to non-DM code." means code with #ifndef
> CONFIG_DM_SPI_FLASH sf_probe.c? ie because for mtd should available
> for non-dm drivers as well. Once all move to dm this will drop anyway.

The question in my mind is whether we are getting closer or further
away from that day.

>
>>
>> I am obviously failing to explain what I mean here - let me try again...
>>
>> Function methods should be implemented as 'ops' structs in driver
>> model and use the 'ops' member of the driver to provide the
>> implementation. There should be no other function pointers or structs
>> of function pointers.
>>
>> Drivers should be declared as DM drivers, and devices should be
>> created by driver model when it finds a definition in the device tree,
>> or dynamically if needed for probing buses (e.g. PCI, USB, I2C in some
>> cases). There should not be calls to 'register' and memory
>> allocations. That is a sign that you are bypassing driver model for
>> these things.
>>
>> I do understand that the SPI flash DM conversion is incomplete - there
>> are still many boards which have yet to be converted for SPI or SPI
>> flash. Until that is done I think the efforts should be directed
>> towards that goal, and things like SPI MTD should be introduced for DM
>> only. At present it seems that we have created yet another migration
>> task (SPI MTD) that would be better avoided.
>>
>> I know you have sent detailed emails about the software layers, etc. I
>> do understand that (or at least I think I do), but in the DM world,
>> each layer should be a uclass with its own API and its own devices.
>>
>> How can we move forward on this and get everything moved over to DM in
>> short order?

So the above is the key question here. I'm still not clear on this, as above...

>>
>>>
>>> Jagan Teki (34):
>>>   sf: spi_flash_validate_params => spi_flash_scan
>>>   sf: Move spi_flash_scan code to sf_ops
>>>   sf: Move read_id code to sf_ops
>>>   sf: probe: Code cleanup
>>>   sf: Use static for file-scope functions
>>>   sf: Fix Makefile
>>>   sf: Use simple name for register access functions
>>>   sf: Use flash function pointers in dm_spi_flash_ops
>>>   sf: Flash power up read-only based on idcode0
>>>   sf: Use static for file-scope functions
>>>   sf: Remove unneeded header includes
>>>   sf: probe: Use spi_flash_scan in dm-spi-flash
>>>   sf: Re-factorize spi_flash_probe_tail code
>>>   dm-sf: Re-factorize spi_flash_std_probe code
>>>   zynq: Enable CONFIG_SPL_MTD_SUPPORT
>>>   sf: Add MTD support to spi_flash
>>>   sf: Use mtd_info ops instead of spi_flash ops
>>>   cmd_sf: Use mtd->size instead of flash->size
>>>   sf: Use mtd->erasesize instead of flash->erase_size
>>>   dm-sf: use mtd_ops, drop dm_spi_flash_ops
>>>   sf: Use MTD lock operations
>>>   sf: Add MTD support for non-dm spi_flash interface
>>>   sf: probe: Minor cleanup
>>>   sf: Drop SNOR_F_SST_WR flash->flags
>>>   sf: Remove unneeded SST_BP and SST_WP
>>>   sf: ops: Fix missing break on spansion read_bar
>>>   sf: Drop SPI_FLASH_MTD driver
>>>   configs: Remove CONFIG_SPI_FLASH_MTD
>>>   sf: dataflash: Remove unneeded spi data
>>>   sf: dataflash: Move flash id detection into jedec_probe
>>>   sf: dataflash: Fix add_dataflash return logic
>>>   sf: dataflash: Add MTD core support
>>>   sf: dataflash: Rename sf_dataflash.c to mtd_dataflash.c
>>>   mtd: dataflash: Minor cleanups
>>>
>>>  common/cmd_sf.c                                    |  16 +-
>>>  drivers/mtd/spi/Kconfig                            |  14 +-
>>>  drivers/mtd/spi/Makefile                           |   9 +-
>>>  .../mtd/spi/{sf_dataflash.c => mtd_dataflash.c}    | 402 ++++++-----
>>>  drivers/mtd/spi/sf-uclass.c                        |  39 --
>>>  drivers/mtd/spi/sf_internal.h                      |  74 +-
>>>  drivers/mtd/spi/sf_mtd.c                           | 104 ---
>>>  drivers/mtd/spi/sf_ops.c                           | 769 ++++++++++++++++-----
>>>  drivers/mtd/spi/sf_probe.c                         | 508 ++------------
>>>  include/configs/aristainetos-common.h              |   1 -
>>>  include/configs/gw_ventana.h                       |   1 -
>>>  include/configs/ls1021aqds.h                       |   2 +-
>>>  include/configs/socfpga_common.h                   |   1 -
>>>  include/configs/zynq-common.h                      |   1 +
>>>  include/spi_flash.h                                | 163 ++---
>>>  15 files changed, 926 insertions(+), 1178 deletions(-)
>>>  rename drivers/mtd/spi/{sf_dataflash.c => mtd_dataflash.c} (64%)
>>>  delete mode 100644 drivers/mtd/spi/sf_mtd.c
>>>
>
> thanks!
> --
> Jagan | openedev.

Regards,
Simon