[U-Boot] [PATCH 03/16] spi: Add non DM version of SPI_MEM

[Vignesh R]

On 13/12/18 2:55 AM, Boris Brezillon wrote:
> On Wed, 12 Dec 2018 22:07:44 +0100
> Jagan Teki <jagan at amarulasolutions.com> wrote:
> 
>> On Wed 12 Dec, 2018, 10:02 PM Boris Brezillon <boris.brezillon at bootlin.com
>> wrote:
>>
>>> On Thu, 13 Dec 2018 02:15:16 +0530
>>> Jagan Teki <jagan at amarulasolutions.com> wrote:
>>>  
>>>> On Thu, Dec 13, 2018 at 2:10 AM Boris Brezillon
>>>> <boris.brezillon at bootlin.com> wrote:  
>>>>>
>>>>> Hi Jagan,
>>>>>
>>>>> On Thu, 13 Dec 2018 01:55:08 +0530
>>>>> Jagan Teki <jagan at amarulasolutions.com> wrote:
>>>>>  
>>>>>> On Wed, Dec 12, 2018 at 11:08 PM Vignesh R <vigneshr at ti.com>  
>>> wrote:  
>>>>>>>
>>>>>>> Add non DM version of SPI_MEM to support easy migration to new SPI  
>>> NOR  
>>>>>>> framework. This can be removed once DM_SPI conversion is  
>>> complete.  
>>>>>>
>>>>>> Our intention to use new driver to follow dm, why we need to support
>>>>>> non-dm? any usecases?  
>>>>>
>>>>> Looks like we're having the same discussion over and over. Vignesh is
>>>>> dropping spi_flash.c which AFAICT was not depending on DM_SPI, so, if
>>>>> we want to keep everyone happy while getting rid of some legacy code,
>>>>> that's the only solution. DM conversion is a nice goal, but it's kind
>>>>> of orthogonal to what Vignesh is working on. If DM_SPI conversion
>>>>> happens before the spi-nor stuff is merged  (which I doubt) then this
>>>>> patch can simply be dropped.  
>>>>
>>>> spi_flash.c is a core code not a specific driver it belongs. spi-mem
>>>> is new feature driver how come new driver will support legacy non-dm
>>>> do we have legacy use for that(ie what I'm asking about usecase)  
>>>
>>> I recommend that you read the spi-mem code carefully. spi-mem is not
>>> driver specific, it's a thin layer on top of spi and driver *can* (but
>>> are not forced to) provide optimized methods to execute spi-mem
>>> operations. When that's not the case, the implementation falls back to
>>> regular spi transfers. AFAIK, both DM and non-DM drivers support
>>> regular spi transfers, right? So why should we depend on DM_SPI? And
>>> more importantly, if we do that, that means we can't get rid of
>>> spi_flash.c since some users might still have non-DM SPI drivers, which
>>> in turn means we keep more legacy code for no good reasons.
>>>  
>>
>> I understand spi-mem is core file, but new code too.
> 
> Sorry, I don't get it.
> 
>>
>>
>>> You want non-DM SPI controller drivers to go away, then remove them,
>>> instead of blocking other changes using this excuse.
>>>  
>>
>> Please understand uboot development flow, legacy driver can be removed if
>> possible once migration expire and NEW drivers or code must be dm driven.
> 
> Sorry, but I think you're the one misunderstanding what we are trying
> to do here. Vignesh changes have simply no impact on the DM SPI
> conversion you're aiming at. All Vignesh does is provide a dummy
> wrapper for non-DM drivers, which would probably have been implemented
> by Miquel if you had not been so insistent on your precious DM_SPI
> conversion. That was not really a problem for spi-nand, as we were
> adding support for a new feature. This is not the case here. SPI NORs
> are already partially supported by the u-boot spi flash layer, and we
> need to keep things in a working state for those that were using it and
> didn't have their SPI controller drivers converted to the DM. This
> leaves us 2 options:
> 
> 1/ keep the sf_flash code as is and add a new spi-nor code base 
> 2/ replace spi_flash code by the spi-nor layer imported from Linux
> 
> Vignesh chose option #2 which has the benefit of avoiding code
> duplication. Given the discussion we're having right now, I'm wondering
> if it wouldn't be easier to go for option #1 in order to avoid those
> endless discussions...
> 

Boris, thanks for chiming in! This is exactly what I had in mind.

To add, I did start with #1 by simply adding support for 4 byte
addressing. But, then released I need spi-mem to communicate this
protocol info to SPI drivers, then found Quad Enable detection logic to
be incomplete and so on.. Finally released I would end up with code
exactly similar to Linux SPI-NOR(with addition of SFDP logic). Therefore
switched to #2 ;)


-- 
Regards
Vignesh