Re: [PATCH v2] mtd: spi-nor: Add support for SPI boot flash access for AMD Family 16h

[Grandbois, Brett]

On 30/10/18 6:26 pm, Boris Brezillon wrote:
> On Mon, 29 Oct 2018 23:15:42 +0000
> "Grandbois, Brett"<brett.grandbois@xxxxxxxxxxxx>  wrote:
>
>> On 28/10/18 1:39 am, Boris Brezillon wrote:
>>> Hi Brett,
>>>
>>> On Tue, 16 Oct 2018 00:57:41 +0000
>>> "Grandbois, Brett"<brett.grandbois@xxxxxxxxxxxx>   wrote:
>>>   
>>>> Add support to expose the SPI boot flash on AMD Family 16h CPUs as
>>>> a standard mtd device to give userspace BIOS updaters greater
>>>> feature support.  The BIOS and Kernel Developer's Guide refers to
>>>> this as the 'SPI ROM' controller and so the driver follows that
>>>> naming convention for consistency.
>>>>   
>>> We're currently trying to convert spi-nor controller drivers to the
>>> spi-mem interface [1]. Can you look at this new interface and tell
>>> me if you'd be able to implement it? If that's not possible, then
>>> I'd prefer to have this driver implement the mtd_info interface
>>> directly.
>> So from going over the spi-mem interface it looks like the intent is
>> for these sorts of devices to be a standard spi_controller with only
>> mem_ops defined and the transfer/_one/_one_message left as NULL?  Is
>> that correct?
> Yes
>
>> That's a bit of a pivot from how it's currently done
>> (it's conceptually similar to the intel-spi-pci driver so I was
>> following that)
> Yes, and that's exactly what I'd like to avoid. intel-spi-pci will
> probably be the trickiest conversion, so I'd like to avoid having
> another one ;-).
>
>> but I should be able to rework it to the new
>> interface.  This then lives under drivers/spi and thus should be
>> submitted to linux-spi?
> Actually, if the controller is only ever connected to the same SPI NOR
> chip (no need for advanced detection scheme) and does not support
> support Octo/Quad/Dual modes (or any other advanced features), you'll
> be better off implementing mtd->_read/_write/_erase() directly (the
> driver would then live in drivers/mtd/devices/).

Hmm, conceptually the device is better suited to the mtd layer than the spi layer.  The HW is designed to only ever access spi flash chips, not as a general spi master controller, so really the end result of it should always only ever be 1 mtd device.  Unfortunately as the device probing and command sequences for this are the same as implemented in spi-nor I'd either be duplicating a lot of existing code, or just wrapping around spi_nor_scan which sounds like we're back to the dedicated spi-nor controller you're trying to move away from.

The spi-mem interface ops do nicely match up with what the controller supports, including the new direct mapping mode which I'd be able to make use of, so as long as there aren't any issues with supporting only mem_ops and not the message transfers then it's probably the way to go.