Hi Vignesh, On Wed, 16 Oct 2019 at 04:28, Vignesh Raghavendra <vigne...@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 <s...@chromium.org> wrote: > >> > >> Hi Bin, > >> > >> On Wed, 9 Oct 2019 at 07:55, Bin Meng <bmeng...@gmail.com> wrote: > >>> > >>> Hi Simon, > >>> > >>> On Wed, Sep 25, 2019 at 10:12 PM Simon Glass <s...@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. > > >>>> Signed-off-by: Simon Glass <s...@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. The problem is that it is ops within ops so doesn't follow driver model. > > 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. Yes I'm going to try to make use of the existing ish driver as Bin suggested. That might help avoid 'working around' the driver. Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot