Re: [platforms: PATCH 10/11] Drivers/Spi/Devices/MvSpiFlash: Enable dynamic SPI Flash detection

[Leif Lindholm <leif.lindholm@linaro.org>]

On Fri, Sep 01, 2017 at 07:20:06PM +0200, Marcin Wojtas wrote:
> 2017-09-01 17:33 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> > On Fri, Sep 01, 2017 at 03:08:22PM +0200, Marcin Wojtas wrote:
> >> Hitherto mechanism of fixing SPI flash model in the PCDs,
> >> occured to be very inefficient and problematic. Enable
> >> dynamic detection by reworking MvSpiFlashReadId() command,
> >> which now reads the Id and goes through newly added table
> >> with JEDEC compliant devices and their description.
> >>
> >> On the occasion fix the ReadId process by using master's
> >> ReadWrite routine instead of pure Transfer - no longer
> >> swapping and byte shifting is needed. Simplify code by
> >> using local array instead of dynamic allocation. Also
> >> reduced number of ReadId arguments allowed for cleaning
> >> Fupdate and Sf shell commands probe routines.
> >>
> >> Additionally, use SpiFlashInfo fields instead of PCDs,
> >> and configure needed settings in MvSpiFlashInit.
> >>
> >> Update PortingGuide documentation accordingly.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> >
> > So, this looks _really_ good, but I'm seeing three separate patches in
> > here really:
> > - Style fixups (yay). I love them, but squashing them in make the
> >   functional changes impossible to spot in the noise.
> > - SPI Flash Id table. Love it!
> >   But this is a completely generic feature. Medium-term, this should
> >   be living in EDK2. For now, can you break it out into a separate
> >   library that can be used by other platforms (and will be easier to
> >   move)?
> > - Functional improvements to existing driver - great! (but can't
> >   really review before the above two have been broken out).
> >
> 
> I'll check and do the style fixup/functional improvement split.

Thanks!

> About flash ids in a separate library - we already have
> SpiFlashProtocol->ReadId callback. In fact despite Mv prefix the
> MvSpiFlash.c driver and its associated SPI_FLASH_PROTOCOL are
> completely generic for now and SoC controller agnostic. It works in
> very similar way as U-Boot driver (drivers/mtd/spi) and Linux one
> (drivers/mtd/spi-nor/spi-nor.c). Due to lack of similar solution, how
> about moving it to EDK2? Do you think there's chance to promote such
> solution?

Oh, certainly.
The only question would be whether you would be willing/able to make
that happen for this series. Having the identification portion as a
separate library may not be a bad idea regardless ... and it's a
decent halfway house.

/
    Leif