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

[Vignesh Raghavendra <vigneshr@ti.com>]

Hi Simon,

On 16/10/19 10:10 PM, Simon Glass wrote:
> Hi Vignesh,
> 
> On Wed, 16 Oct 2019 at 04:28, Vignesh Raghavendra <vigneshr@ti.com> wrote:
>>
>> 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@chromium.org> wrote:
>>>>
>>>> Hi Bin,
>>>>
>>>> On Wed, 9 Oct 2019 at 07:55, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>>
>>>>> Hi Simon,
>>>>>
>>>>> On Wed, Sep 25, 2019 at 10:12 PM Simon Glass <sjg@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?
> 
> This used to be implemented but is not supported anymore. I think we
> should wait until the DM SPI flash migration is complete before trying
> again.
> 
> Also it is not just reading. The data is used in-place in some cases,
> so we do want to know the map region.
> 

I am fine with the idea. Just wanted to know the motivation.

>>
>>>>>> Signed-off-by: Simon Glass <sjg@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.
> 

spi_mem_ops in U-Boot was added by Miquel while introducing SPI NAND
support in U-Boot, but I extended its usage to SPI NOR devices as well.

> The problem is that it is ops within ops so doesn't follow driver model.

Hmm, why is that so?
ops within ops is not unheard of and is common in kernel as well. This
actually allows to grouping of similar operations into simple structs
and thus improves readability.


--
Regards
Vignesh