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

[Jagan Teki]

Hi Simon,

On 1 December 2015 at 04:47, Simon Glass <sjg at chromium.org> wrote:
> 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:

Please continue your comments on separate on this thread
"[U-Boot] dm: Introduce SPI-NOR framework"

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