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

[Simon Glass]

Hi Jagan,

On 27 November 2015 at 02:21, Jagan Teki <jteki at openedev.com> wrote:
> Hi Bin,
>
> On 27 November 2015 at 07:55, Bin Meng <bmeng.cn at gmail.com> wrote:
>> Hi Jagan,
>>
>> On Fri, Nov 27, 2015 at 2:54 AM, 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.
>>
>> My understanding is that when we introduce a new driver feature, it
>> should target driver model by default. This is to encourage boards to
>> take advantage of driver model, and all these *new* features. This is
>> what Simon asked for. As Simon mentioned, probably we should make the
>> SPI flash DM conversion complete now instead of adding any new feature
>> on top of it.
>
> I'm not introduced any new driver feature here, instead I re-used the
> existing mtd core features and functionalities.
>
> The current code looks as below:
> --------------------------------------------
> sf_probe.c:
> 1) having both non-dm and dm spi-flash and calls to
> 2) spi_flash_probe_slave for spi_flash detection and setting up hooks
> (spi_flash ops)
>
> sf_ops.c:
> 1) having all spi_flash ops definitions.
>
> These are the changes I did
> a) Moved spi_flash probing like definition of  spi_flash_probe_slave
> from sf_probe.c to sf_ops.c so sf_ops.c having all core spi-flash code
> like flash detection, setting up hooks (spi_flash ops) and spi_flash
> ops definitions.
> b) from above point a) sf_probe.c having both dm and non-dm interface
> with a common call to sf_ops.c for spi-flash core functionalities
> using spi_flash_scan.
> c) Replaced existing spi_flash_mtd_register (sf_mtd.c) registration
> with generic mtd core using add_mtd_device in sf_probe.c for both dm
> and non-dm interfaces.
> d) In sf_ops.c Updated all mtd_info structure filling and replaced
> spi_flash ops hooks with mtd_info hooks - where dm_spi_flash_ops got
> dropped.
> e) Called mtd core operations from spi_flash.h like
> mtd_erase|write|read instead of direct calls to sf_ops.c with
> spi_flash operation as they are not needed, so mtd core calls will in
> turn call mtd hooks on spi-flash like nand, cfi and spi-nor in Linux.
>
> The code after these changes:
> ----------------------------------------
> sf_probe.c:
> 1) having both non-dm and dm spi-flash and calls to
> 2) spi_flash_scan for spi-flash core functionalities and register with core mtd.
>
> sf_ops.c:
> 1) core spi-flash operations => spi_flash detection + setting up hooks
> (mtd ops) + having all mtd_info ops definitions.
>
> I couldn't understand what's wrong with this approach, because I have
> not added any new feature instead I reused it existing MTD core. I
> agree that the dm_spi_flash_ops are removed as mtd_info ops are used
> instead and sf_probe.c is still with dm.
>
> Please let me know your inputs.

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.

Before going any further we need to at least figure out the end goal.

To repeat my question from the previous email:

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?

>
>>
>>>
>>> cmd_sf
>>> =======
>>> MTD
>>> =======
>>> spi-nor or spi-flash
>>> ===============
>>> "spi-nor to spi drivers" and spi-nor controller driver
>>> =======================================
>
> thanks!
> --
> Jagan | openedev.
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

Regards,
Simon