Hi Jagan,
On 26 November 2015 at 10:54, Jagan Teki <jt...@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 <s...@chromium.org> wrote:
>> +Fabio
>>
>> Hi Jagan,
>>
>> On 26 November 2015 at 04:03, Jagan Teki <jt...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
