[U-Boot] [PATCH v2 14/38] spi: Add support for memory-mapped flash

[Vignesh Raghavendra]

Hi Simon,

On 12/10/19 10:03 AM, Bin Meng wrote:
> Hi Simon,
> 
> On Sat, Oct 12, 2019 at 11:08 AM Simon Glass <sjg at chromium.org> wrote:
>>
>> Hi Bin,
>>
>> On Wed, 9 Oct 2019 at 07:55, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>
>>> Hi Simon,
>>>
>>> On Wed, Sep 25, 2019 at 10:12 PM Simon Glass <sjg at chromium.org> wrote:
>>>>
>>>> On x86 platforms the SPI flash can be mapped into memory so that the
>>>> contents can be read with normal memory accesses.
>>>>
>>>> Add a new SPI flash method to find the location of the SPI flash in
>>>> memory. This differs from the existing device-tree "memory-map" mechanism
>>>> in that the location can be discovered at run-time.
>>>>

Whats is the usecase? Why can't spi_flash_read() be used instead?
Flash + Controller driver can underneath take care of using memory
mapped mode to read data from flash right while making sure that access
is within valid window?

>>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>>> ---
>>>>
>>>> Changes in v2: None
>>>>
>>>>  drivers/mtd/spi/sandbox_direct.c | 11 +++++++++++
>>>>  drivers/mtd/spi/sf-uclass.c      | 11 +++++++++++
>>>>  include/spi_flash.h              | 27 +++++++++++++++++++++++++++
>>>>  test/dm/sf.c                     |  9 +++++++++
>>>>  4 files changed, 58 insertions(+)
>>>>
>>>> diff --git a/drivers/mtd/spi/sandbox_direct.c b/drivers/mtd/spi/sandbox_direct.c
>>>> index 43d8907710c..fb515edcb7c 100644
>>>> --- a/drivers/mtd/spi/sandbox_direct.c
>>>> +++ b/drivers/mtd/spi/sandbox_direct.c
>>>> @@ -68,6 +68,16 @@ static int sandbox_direct_get_sw_write_prot(struct udevice *dev)
>>>>         return priv->write_prot++ ? 1 : 0;
>>>>  }
>>>>
>>>> +static int sandbox_direct_get_mmap(struct udevice *dev, ulong *map_basep,
>>>> +                                  size_t *map_sizep, u32 *offsetp)
>>>> +{
>>>> +       *map_basep = 0x1000;
>>>> +       *map_sizep = 0x2000;
>>>> +       *offsetp = 0x100;
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>>  static int sandbox_direct_probe(struct udevice *dev)
>>>>  {
>>>>         struct sandbox_direct_priv *priv = dev_get_priv(dev);
>>>> @@ -82,6 +92,7 @@ static struct dm_spi_flash_ops sandbox_direct_ops = {
>>>>         .write = sandbox_direct_write,
>>>>         .erase = sandbox_direct_erase,
>>>>         .get_sw_write_prot = sandbox_direct_get_sw_write_prot,
>>>> +       .get_mmap = sandbox_direct_get_mmap,
>>>>  };
>>>>
>>>>  static const struct udevice_id sandbox_direct_ids[] = {
>>>> diff --git a/drivers/mtd/spi/sf-uclass.c b/drivers/mtd/spi/sf-uclass.c
>>>> index 719a2fd23ae..127ec7e7aa6 100644
>>>> --- a/drivers/mtd/spi/sf-uclass.c
>>>> +++ b/drivers/mtd/spi/sf-uclass.c
>>>> @@ -28,6 +28,17 @@ int spi_flash_erase_dm(struct udevice *dev, u32 offset, size_t len)
>>>>         return log_ret(sf_get_ops(dev)->erase(dev, offset, len));
>>>>  }
>>>>
>>>> +int spi_flash_get_mmap(struct udevice *dev, ulong *map_basep, size_t *map_sizep,
>>>> +                      u32 *offsetp)
>>>> +{
>>>> +       struct dm_spi_flash_ops *ops = sf_get_ops(dev);
>>>> +
>>>> +       if (!ops->get_mmap)
>>>> +               return -ENOSYS;
>>>> +
>>>> +       return ops->get_mmap(dev, map_basep, map_sizep, offsetp);
>>>> +}
>>>> +
>>>>  int spl_flash_get_sw_write_prot(struct udevice *dev)
>>>>  {
>>>>         struct dm_spi_flash_ops *ops = sf_get_ops(dev);
>>>> diff --git a/include/spi_flash.h b/include/spi_flash.h
>>>> index 55b4721813a..840189e22c7 100644
>>>> --- a/include/spi_flash.h
>>>> +++ b/include/spi_flash.h
>>>> @@ -47,6 +47,19 @@ struct dm_spi_flash_ops {
>>>>          *      other -ve value on error
>>>>          */
>>>>         int (*get_sw_write_prot)(struct udevice *dev);
>>>> +
>>>> +       /**
>>>> +        * get_mmap() - Get memory-mapped SPI
>>>> +        *
>>>> +        * @dev:        SPI flash device
>>>> +        * @map_basep:  Returns base memory address for mapped SPI
>>>> +        * @map_sizep:  Returns size of mapped SPI
>>>> +        * @offsetp:    Returns start offset of SPI flash where the map works
>>>> +        *      correctly (offsets before this are not visible)
>>>> +        * @return 0 if OK, -EFAULT if memory mapping is not available
>>>> +        */
>>>
>>> I feel odd to add such an op to the flash op, as memory address is not
>>> determined by the flash itself, but the SPI flash controller. We
>>> probably should add the op to the SPI flash controller instead.
>>
>> So do you think this should be added to UCLASS_SPI?
> 
> Yes, I think so. Jagan, what's your recommendation?
> 
>>
>> As it stands we don't actually use that uclass with this SPI flash
>> driver - it implements the SPI_FLASH interface directly.
>>
>> But given that I'm going to try to use the same ich.c driver this
>> should be easy enough.
>>
>> I've just found the weird mem_ops argument within struct dm_spi_ops...oh dear.
>>
> 
> The mem_ops was added by Vignesh. I believe that was derived from the
> Linux kernel.
> 

Some SPI controllers provide interfaces to work with any type of SPI
flashes like SPI NOR, SPI NAND, SPI SRAM etc. They may also provide
specialized accelerated interface to work with them. spi_mem_ops
abstracts these details and provides flash agnostic interface b/w flash
driver and SPI controller.
This means single SPI controller driver can support different variety of
SPI flashes such as SPI NOR (QSPI/OSPI), SPI NAND, etc
Most drivers implementing spi_mem_ops provide memory mapped access to
flash as well. (I see ich.c does too).

If this HW is a generic SPI controller that can talk to any or subset of
SPI NOR flashes (such as the ones listed in
drivers/mtd/spi/spi-nor-ids.c) and provides a way to configure commands
for SPI Flash operations, then this needs to be modeled as SPI
controller implementing spi_mem_ops. spi-nor-core.c will take care flash
details.

> Regards,
> Bin
> 

-- 
Regards
Vignesh