| Message ID | 20190925141147.191166-15-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Bin Meng |
| Headers | show |
| Series | x86: Various modifications to prepare for FSP2 | expand |
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. > > 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. > + int (*get_mmap)(struct udevice *dev, ulong *map_basep, > + size_t *map_sizep, u32 *offsetp); > }; > > /* Access the serial operations for a device */ > @@ -88,6 +101,20 @@ int spi_flash_write_dm(struct udevice *dev, u32 offset, size_t len, > */ > int spi_flash_erase_dm(struct udevice *dev, u32 offset, size_t len); > > +/** > + * spi_flash_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, -ENOSYS if no operation, -EFAULT if memory mapping is not > + * available > + */ > +int spi_flash_get_mmap(struct udevice *dev, ulong *map_basep, size_t *map_sizep, > + u32 *offsetp); > + > /** > * spl_flash_get_sw_write_prot() - Check state of software write-protect feature > * > diff --git a/test/dm/sf.c b/test/dm/sf.c > index 56277954c23..f8f9e717720 100644 > --- a/test/dm/sf.c > +++ b/test/dm/sf.c > @@ -102,6 +102,9 @@ static int dm_test_spi_flash_direct(struct unit_test_state *uts) > { > struct udevice *dev; > char buf[BUF_SIZE]; > + size_t map_size; > + ulong map_base; > + u32 offset; > int i; > > ut_assertok(uclass_get_device(UCLASS_SPI_FLASH, 1, &dev)); > @@ -130,6 +133,12 @@ static int dm_test_spi_flash_direct(struct unit_test_state *uts) > ut_asserteq(0, spl_flash_get_sw_write_prot(dev)); > ut_asserteq(1, spl_flash_get_sw_write_prot(dev)); > > + /* Check mapping */ > + ut_assertok(spi_flash_get_mmap(dev, &map_base, &map_size, &offset)); > + ut_asserteq(0x1000, map_base); > + ut_asserteq(0x2000, map_size); > + ut_asserteq(0x100, offset); > + > return 0; > } > DM_TEST(dm_test_spi_flash_direct, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT); > -- Regards, Bin
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. > > > > 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? 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. - Simon
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. > > > > > > 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. Regards, Bin
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? >>>> 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. > 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 >
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. > > >>>> 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. 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
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? I can see spi_flash_read_dm() but it does not support returning a pointer to the data, only reading it. Also I cannot find any documentation about any of this. I've been looking in the doc/ directory. I found the spi_mem.h file but it doesn't mention the meaning of the in and out buffer pointers so I don't know how to use them. Is there an API missing or just comments/documentation? Regards, Simon
Hi, On Thu, 17 Oct 2019 at 08:28, Simon Glass <sjg@chromium.org> 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? > > I can see spi_flash_read_dm() but it does not support returning a > pointer to the data, only reading it. > > Also I cannot find any documentation about any of this. I've been > looking in the doc/ directory. > > I found the spi_mem.h file but it doesn't mention the meaning of the > in and out buffer pointers so I don't know how to use them. > > Is there an API missing or just comments/documentation? Apart from this I have found that the ich_spi driver does not work for APL since it apparently only supports 'hardware sequencing'. I did try getting software sequencing to work, but no dice. In addition, I found that enabling SPI flash, etc. added about 6KB of code! So I think it might be best to have two SPI drivers for x86 - one for software sequencing and one for hardware? Regards, Simon
Hi Simon, On Fri, Oct 18, 2019 at 10:22 AM Simon Glass <sjg@chromium.org> wrote: > > Hi, > > On Thu, 17 Oct 2019 at 08:28, Simon Glass <sjg@chromium.org> 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? > > > > I can see spi_flash_read_dm() but it does not support returning a > > pointer to the data, only reading it. > > > > Also I cannot find any documentation about any of this. I've been > > looking in the doc/ directory. > > > > I found the spi_mem.h file but it doesn't mention the meaning of the > > in and out buffer pointers so I don't know how to use them. > > > > Is there an API missing or just comments/documentation? > > Apart from this I have found that the ich_spi driver does not work for > APL since it apparently only supports 'hardware sequencing'. I did try > getting software sequencing to work, but no dice. > > In addition, I found that enabling SPI flash, etc. added about 6KB of code! > > So I think it might be best to have two SPI drivers for x86 - one for > software sequencing and one for hardware? So if this is true, I think we can create Kconfig options (HW_SEQ and SW_SEQ) for ICH SPI driver, and select either one in the SoC Kconfig file. But I see from the Linux kernel driver, it has: 300static int intel_spi_init(struct intel_spi *ispi) 301{ 302 u32 opmenu0, opmenu1, lvscc, uvscc, val; 303 int i; 304 305 switch (ispi->info->type) { 306 case INTEL_SPI_BYT: 307 ispi->sregs = ispi->base + BYT_SSFSTS_CTL; 308 ispi->pregs = ispi->base + BYT_PR; 309 ispi->nregions = BYT_FREG_NUM; 310 ispi->pr_num = BYT_PR_NUM; 311 ispi->swseq_reg = true; 312 313 if (writeable) { 314 /* Disable write protection */ 315 val = readl(ispi->base + BYT_BCR); 316 if (!(val & BYT_BCR_WPD)) { 317 val |= BYT_BCR_WPD; 318 writel(val, ispi->base + BYT_BCR); 319 val = readl(ispi->base + BYT_BCR); 320 } 321 322 ispi->writeable = !!(val & BYT_BCR_WPD); 323 } 324 325 break; 326 327 case INTEL_SPI_LPT: 328 ispi->sregs = ispi->base + LPT_SSFSTS_CTL; 329 ispi->pregs = ispi->base + LPT_PR; 330 ispi->nregions = LPT_FREG_NUM; 331 ispi->pr_num = LPT_PR_NUM; 332 ispi->swseq_reg = true; 333 break; 334 335 case INTEL_SPI_BXT: 336 ispi->sregs = ispi->base + BXT_SSFSTS_CTL; 337 ispi->pregs = ispi->base + BXT_PR; 338 ispi->nregions = BXT_FREG_NUM; 339 ispi->pr_num = BXT_PR_NUM; 340 ispi->erase_64k = true; 341 break; So for INTEL_SPI_BXT (which is for ApolloLake I believe) ispi->swseq_reg is not set to true which means it uses hardware sequencer, which seems to contradict with what you found. Regards, Bin
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
On 18/10/19 7:52 AM, Simon Glass wrote: > Hi, > > On Thu, 17 Oct 2019 at 08:28, Simon Glass <sjg@chromium.org> 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? >> >> I can see spi_flash_read_dm() but it does not support returning a >> pointer to the data, only reading it. >> No, I was merely suggesting to use spi_flash_read_dm() (which underneath can be implemented by spi_mem_ops) to get data from flash instead of using get_mmap() to get MMIO pointer and then reading the data using that potiner. >> Also I cannot find any documentation about any of this. I've been >> looking in the doc/ directory. >> >> I found the spi_mem.h file but it doesn't mention the meaning of the >> in and out buffer pointers so I don't know how to use them. >> >> Is there an API missing or just comments/documentation? > spi-mem.h has struct and APIs documented as kerneldoc: https://elixir.bootlin.com/u-boot/latest/source/include/spi-mem.h#L72 https://elixir.bootlin.com/u-boot/latest/source/include/spi-mem.h#L168 Note that clients should still use spi_flash_*() APIs to interact with flash. spi_mem_ops are not an alternative to spi_flash_*() APIs which are used by other drivers to read/write data from/to flash. This continues to be the same. > Apart from this I have found that the ich_spi driver does not work for > APL since it apparently only supports 'hardware sequencing'. I did try > getting software sequencing to work, but no dice. > > In addition, I found that enabling SPI flash, etc. added about 6KB of code! > > So I think it might be best to have two SPI drivers for x86 - one for > software sequencing and one for hardware? > > Regards, > Simon >
Hi Vignesh, On Fri, 18 Oct 2019 at 02:50, Vignesh Raghavendra <vigneshr@ti.com> wrote: > [..] > >>>> 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. It is not used elsewhere in driver model though. Also there are no tests. How can we get some tests in there? Regards, Simon
Hi Vignesh, On Fri, 18 Oct 2019 at 03:48, Vignesh Raghavendra <vigneshr@ti.com> wrote: > > > > On 18/10/19 7:52 AM, Simon Glass wrote: > > Hi, > > > > On Thu, 17 Oct 2019 at 08:28, Simon Glass <sjg@chromium.org> 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? > >> > >> I can see spi_flash_read_dm() but it does not support returning a > >> pointer to the data, only reading it. > >> > > No, I was merely suggesting to use spi_flash_read_dm() (which underneath > can be implemented by spi_mem_ops) to get data from flash instead of > using get_mmap() to get MMIO pointer and then reading the data using > that potiner. OK I see. > > >> Also I cannot find any documentation about any of this. I've been > >> looking in the doc/ directory. > >> > >> I found the spi_mem.h file but it doesn't mention the meaning of the > >> in and out buffer pointers so I don't know how to use them. > >> > >> Is there an API missing or just comments/documentation? > > > > spi-mem.h has struct and APIs documented as kerneldoc: > https://elixir.bootlin.com/u-boot/latest/source/include/spi-mem.h#L72 > https://elixir.bootlin.com/u-boot/latest/source/include/spi-mem.h#L168 Yes I see that, but these appear to be undocumented, or not documented enough to understand their purpose/use: opcode in out Could you please update that? > > Note that clients should still use spi_flash_*() APIs to interact with > flash. spi_mem_ops are not an alternative to spi_flash_*() APIs which > are used by other drivers to read/write data from/to flash. This > continues to be the same. OK. Regards, Simon
Hi Bin, On Thu, 17 Oct 2019 at 20:32, Bin Meng <bmeng.cn@gmail.com> wrote: > > Hi Simon, > > On Fri, Oct 18, 2019 at 10:22 AM Simon Glass <sjg@chromium.org> wrote: > > > > Hi, > > > > On Thu, 17 Oct 2019 at 08:28, Simon Glass <sjg@chromium.org> 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? > > > > > > I can see spi_flash_read_dm() but it does not support returning a > > > pointer to the data, only reading it. > > > > > > Also I cannot find any documentation about any of this. I've been > > > looking in the doc/ directory. > > > > > > I found the spi_mem.h file but it doesn't mention the meaning of the > > > in and out buffer pointers so I don't know how to use them. > > > > > > Is there an API missing or just comments/documentation? > > > > Apart from this I have found that the ich_spi driver does not work for > > APL since it apparently only supports 'hardware sequencing'. I did try > > getting software sequencing to work, but no dice. > > > > In addition, I found that enabling SPI flash, etc. added about 6KB of code! > > > > So I think it might be best to have two SPI drivers for x86 - one for > > software sequencing and one for hardware? > > So if this is true, I think we can create Kconfig options (HW_SEQ and > SW_SEQ) for ICH SPI driver, and select either one in the SoC Kconfig > file. I think at least for TPL I'm going to need to bypass all the code as it is just too big for TPL. What do you think? Should we have a separate HW SEQ driver? If you look at the HW SEQ driver I have, it is very little code. Or are you saying we should have a combined driver with Kconfig options to enable either or both of SW SEQ and HW SEQ? One other wrinkle is that I have to have a separate driver anyway, since of-platdata requires it. It is not possible to use of-platdata in a generic driver since the struct definitions rely on the compatible string. > > But I see from the Linux kernel driver, it has: > > 300static int intel_spi_init(struct intel_spi *ispi) > 301{ > 302 u32 opmenu0, opmenu1, lvscc, uvscc, val; > 303 int i; > 304 > 305 switch (ispi->info->type) { > 306 case INTEL_SPI_BYT: > 307 ispi->sregs = ispi->base + BYT_SSFSTS_CTL; > 308 ispi->pregs = ispi->base + BYT_PR; > 309 ispi->nregions = BYT_FREG_NUM; > 310 ispi->pr_num = BYT_PR_NUM; > 311 ispi->swseq_reg = true; > 312 > 313 if (writeable) { > 314 /* Disable write protection */ > 315 val = readl(ispi->base + BYT_BCR); > 316 if (!(val & BYT_BCR_WPD)) { > 317 val |= BYT_BCR_WPD; > 318 writel(val, ispi->base + BYT_BCR); > 319 val = readl(ispi->base + BYT_BCR); > 320 } > 321 > 322 ispi->writeable = !!(val & BYT_BCR_WPD); > 323 } > 324 > 325 break; > 326 > 327 case INTEL_SPI_LPT: > 328 ispi->sregs = ispi->base + LPT_SSFSTS_CTL; > 329 ispi->pregs = ispi->base + LPT_PR; > 330 ispi->nregions = LPT_FREG_NUM; > 331 ispi->pr_num = LPT_PR_NUM; > 332 ispi->swseq_reg = true; > 333 break; > 334 > 335 case INTEL_SPI_BXT: > 336 ispi->sregs = ispi->base + BXT_SSFSTS_CTL; > 337 ispi->pregs = ispi->base + BXT_PR; > 338 ispi->nregions = BXT_FREG_NUM; > 339 ispi->pr_num = BXT_PR_NUM; > 340 ispi->erase_64k = true; > 341 break; > > So for INTEL_SPI_BXT (which is for ApolloLake I believe) > ispi->swseq_reg is not set to true which means it uses hardware > sequencer, which seems to contradict with what you found. I found that it only supports hardware sequencing, so this matches. Regards, Simon
Hi Simon, On Fri, Oct 18, 2019 at 10:14 PM Simon Glass <sjg@chromium.org> wrote: > > Hi Bin, > > On Thu, 17 Oct 2019 at 20:32, Bin Meng <bmeng.cn@gmail.com> wrote: > > > > Hi Simon, > > > > On Fri, Oct 18, 2019 at 10:22 AM Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi, > > > > > > On Thu, 17 Oct 2019 at 08:28, Simon Glass <sjg@chromium.org> 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? > > > > > > > > I can see spi_flash_read_dm() but it does not support returning a > > > > pointer to the data, only reading it. > > > > > > > > Also I cannot find any documentation about any of this. I've been > > > > looking in the doc/ directory. > > > > > > > > I found the spi_mem.h file but it doesn't mention the meaning of the > > > > in and out buffer pointers so I don't know how to use them. > > > > > > > > Is there an API missing or just comments/documentation? > > > > > > Apart from this I have found that the ich_spi driver does not work for > > > APL since it apparently only supports 'hardware sequencing'. I did try > > > getting software sequencing to work, but no dice. > > > > > > In addition, I found that enabling SPI flash, etc. added about 6KB of code! > > > > > > So I think it might be best to have two SPI drivers for x86 - one for > > > software sequencing and one for hardware? > > > > So if this is true, I think we can create Kconfig options (HW_SEQ and > > SW_SEQ) for ICH SPI driver, and select either one in the SoC Kconfig > > file. > > I think at least for TPL I'm going to need to bypass all the code as > it is just too big for TPL. What do you think? So what about other boards/drivers? Say we have drv_foo.c, is the best practice to create a separate drv_foo_tpl.c for use with TPL? > > Should we have a separate HW SEQ driver? If you look at the HW SEQ > driver I have, it is very little code. > > Or are you saying we should have a combined driver with Kconfig > options to enable either or both of SW SEQ and HW SEQ? > Yes I was suggesting enable either or both SW and HW SEQ in the same driver. > One other wrinkle is that I have to have a separate driver anyway, > since of-platdata requires it. It is not possible to use of-platdata > in a generic driver since the struct definitions rely on the > compatible string. > > > > > But I see from the Linux kernel driver, it has: > > > > 300static int intel_spi_init(struct intel_spi *ispi) > > 301{ > > 302 u32 opmenu0, opmenu1, lvscc, uvscc, val; > > 303 int i; > > 304 > > 305 switch (ispi->info->type) { > > 306 case INTEL_SPI_BYT: > > 307 ispi->sregs = ispi->base + BYT_SSFSTS_CTL; > > 308 ispi->pregs = ispi->base + BYT_PR; > > 309 ispi->nregions = BYT_FREG_NUM; > > 310 ispi->pr_num = BYT_PR_NUM; > > 311 ispi->swseq_reg = true; > > 312 > > 313 if (writeable) { > > 314 /* Disable write protection */ > > 315 val = readl(ispi->base + BYT_BCR); > > 316 if (!(val & BYT_BCR_WPD)) { > > 317 val |= BYT_BCR_WPD; > > 318 writel(val, ispi->base + BYT_BCR); > > 319 val = readl(ispi->base + BYT_BCR); > > 320 } > > 321 > > 322 ispi->writeable = !!(val & BYT_BCR_WPD); > > 323 } > > 324 > > 325 break; > > 326 > > 327 case INTEL_SPI_LPT: > > 328 ispi->sregs = ispi->base + LPT_SSFSTS_CTL; > > 329 ispi->pregs = ispi->base + LPT_PR; > > 330 ispi->nregions = LPT_FREG_NUM; > > 331 ispi->pr_num = LPT_PR_NUM; > > 332 ispi->swseq_reg = true; > > 333 break; > > 334 > > 335 case INTEL_SPI_BXT: > > 336 ispi->sregs = ispi->base + BXT_SSFSTS_CTL; > > 337 ispi->pregs = ispi->base + BXT_PR; > > 338 ispi->nregions = BXT_FREG_NUM; > > 339 ispi->pr_num = BXT_PR_NUM; > > 340 ispi->erase_64k = true; > > 341 break; > > > > So for INTEL_SPI_BXT (which is for ApolloLake I believe) > > ispi->swseq_reg is not set to true which means it uses hardware > > sequencer, which seems to contradict with what you found. > > I found that it only supports hardware sequencing, so this matches. OK, I see. So the BXT only supports HW SEQ while LPT and BYT support both SW and HW SEQ. Regards, Bin
Hi Bin, On Fri, 18 Oct 2019 at 09:38, Bin Meng <bmeng.cn@gmail.com> wrote: > > Hi Simon, > > On Fri, Oct 18, 2019 at 10:14 PM Simon Glass <sjg@chromium.org> wrote: > > > > Hi Bin, > > > > On Thu, 17 Oct 2019 at 20:32, Bin Meng <bmeng.cn@gmail.com> wrote: > > > > > > Hi Simon, > > > > > > On Fri, Oct 18, 2019 at 10:22 AM Simon Glass <sjg@chromium.org> wrote: > > > > > > > > Hi, > > > > > > > > On Thu, 17 Oct 2019 at 08:28, Simon Glass <sjg@chromium.org> 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? > > > > > > > > > > I can see spi_flash_read_dm() but it does not support returning a > > > > > pointer to the data, only reading it. > > > > > > > > > > Also I cannot find any documentation about any of this. I've been > > > > > looking in the doc/ directory. > > > > > > > > > > I found the spi_mem.h file but it doesn't mention the meaning of the > > > > > in and out buffer pointers so I don't know how to use them. > > > > > > > > > > Is there an API missing or just comments/documentation? > > > > > > > > Apart from this I have found that the ich_spi driver does not work for > > > > APL since it apparently only supports 'hardware sequencing'. I did try > > > > getting software sequencing to work, but no dice. > > > > > > > > In addition, I found that enabling SPI flash, etc. added about 6KB of code! > > > > > > > > So I think it might be best to have two SPI drivers for x86 - one for > > > > software sequencing and one for hardware? > > > > > > So if this is true, I think we can create Kconfig options (HW_SEQ and > > > SW_SEQ) for ICH SPI driver, and select either one in the SoC Kconfig > > > file. > > > > I think at least for TPL I'm going to need to bypass all the code as > > it is just too big for TPL. What do you think? > > So what about other boards/drivers? Say we have drv_foo.c, is the best > practice to create a separate drv_foo_tpl.c for use with TPL? Yes, although in general we can export the functions from the core driver, so the TPL one becomes pretty bare-bones - e.g. it handles platdata but uses the operations of the core drivers. > > > > > Should we have a separate HW SEQ driver? If you look at the HW SEQ > > driver I have, it is very little code. > > > > Or are you saying we should have a combined driver with Kconfig > > options to enable either or both of SW SEQ and HW SEQ? > > > > Yes I was suggesting enable either or both SW and HW SEQ in the same driver. OK I will take a look at that. My code-size concern still stands though, but let's see how it looks. > > > One other wrinkle is that I have to have a separate driver anyway, > > since of-platdata requires it. It is not possible to use of-platdata > > in a generic driver since the struct definitions rely on the > > compatible string. > > > > > > > > But I see from the Linux kernel driver, it has: > > > > > > 300static int intel_spi_init(struct intel_spi *ispi) > > > 301{ > > > 302 u32 opmenu0, opmenu1, lvscc, uvscc, val; > > > 303 int i; > > > 304 > > > 305 switch (ispi->info->type) { > > > 306 case INTEL_SPI_BYT: > > > 307 ispi->sregs = ispi->base + BYT_SSFSTS_CTL; > > > 308 ispi->pregs = ispi->base + BYT_PR; > > > 309 ispi->nregions = BYT_FREG_NUM; > > > 310 ispi->pr_num = BYT_PR_NUM; > > > 311 ispi->swseq_reg = true; > > > 312 > > > 313 if (writeable) { > > > 314 /* Disable write protection */ > > > 315 val = readl(ispi->base + BYT_BCR); > > > 316 if (!(val & BYT_BCR_WPD)) { > > > 317 val |= BYT_BCR_WPD; > > > 318 writel(val, ispi->base + BYT_BCR); > > > 319 val = readl(ispi->base + BYT_BCR); > > > 320 } > > > 321 > > > 322 ispi->writeable = !!(val & BYT_BCR_WPD); > > > 323 } > > > 324 > > > 325 break; > > > 326 > > > 327 case INTEL_SPI_LPT: > > > 328 ispi->sregs = ispi->base + LPT_SSFSTS_CTL; > > > 329 ispi->pregs = ispi->base + LPT_PR; > > > 330 ispi->nregions = LPT_FREG_NUM; > > > 331 ispi->pr_num = LPT_PR_NUM; > > > 332 ispi->swseq_reg = true; > > > 333 break; > > > 334 > > > 335 case INTEL_SPI_BXT: > > > 336 ispi->sregs = ispi->base + BXT_SSFSTS_CTL; > > > 337 ispi->pregs = ispi->base + BXT_PR; > > > 338 ispi->nregions = BXT_FREG_NUM; > > > 339 ispi->pr_num = BXT_PR_NUM; > > > 340 ispi->erase_64k = true; > > > 341 break; > > > > > > So for INTEL_SPI_BXT (which is for ApolloLake I believe) > > > ispi->swseq_reg is not set to true which means it uses hardware > > > sequencer, which seems to contradict with what you found. > > > > I found that it only supports hardware sequencing, so this matches. > > OK, I see. So the BXT only supports HW SEQ while LPT and BYT support > both SW and HW SEQ. OK, ta. How do I look up what LPT and BYT are? Regards, Simon
Hi Simon, On Sat, Oct 19, 2019 at 4:37 AM Simon Glass <sjg@chromium.org> wrote: > > Hi Bin, > > On Fri, 18 Oct 2019 at 09:38, Bin Meng <bmeng.cn@gmail.com> wrote: > > > > Hi Simon, > > > > On Fri, Oct 18, 2019 at 10:14 PM Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Bin, > > > > > > On Thu, 17 Oct 2019 at 20:32, Bin Meng <bmeng.cn@gmail.com> wrote: > > > > > > > > Hi Simon, > > > > > > > > On Fri, Oct 18, 2019 at 10:22 AM Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > Hi, > > > > > > > > > > On Thu, 17 Oct 2019 at 08:28, Simon Glass <sjg@chromium.org> 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? > > > > > > > > > > > > I can see spi_flash_read_dm() but it does not support returning a > > > > > > pointer to the data, only reading it. > > > > > > > > > > > > Also I cannot find any documentation about any of this. I've been > > > > > > looking in the doc/ directory. > > > > > > > > > > > > I found the spi_mem.h file but it doesn't mention the meaning of the > > > > > > in and out buffer pointers so I don't know how to use them. > > > > > > > > > > > > Is there an API missing or just comments/documentation? > > > > > > > > > > Apart from this I have found that the ich_spi driver does not work for > > > > > APL since it apparently only supports 'hardware sequencing'. I did try > > > > > getting software sequencing to work, but no dice. > > > > > > > > > > In addition, I found that enabling SPI flash, etc. added about 6KB of code! > > > > > > > > > > So I think it might be best to have two SPI drivers for x86 - one for > > > > > software sequencing and one for hardware? > > > > > > > > So if this is true, I think we can create Kconfig options (HW_SEQ and > > > > SW_SEQ) for ICH SPI driver, and select either one in the SoC Kconfig > > > > file. > > > > > > I think at least for TPL I'm going to need to bypass all the code as > > > it is just too big for TPL. What do you think? > > > > So what about other boards/drivers? Say we have drv_foo.c, is the best > > practice to create a separate drv_foo_tpl.c for use with TPL? > > Yes, although in general we can export the functions from the core > driver, so the TPL one becomes pretty bare-bones - e.g. it handles > platdata but uses the operations of the core drivers. > > > > > > > > > Should we have a separate HW SEQ driver? If you look at the HW SEQ > > > driver I have, it is very little code. > > > > > > Or are you saying we should have a combined driver with Kconfig > > > options to enable either or both of SW SEQ and HW SEQ? > > > > > > > Yes I was suggesting enable either or both SW and HW SEQ in the same driver. > > OK I will take a look at that. > > My code-size concern still stands though, but let's see how it looks. > > > > > > One other wrinkle is that I have to have a separate driver anyway, > > > since of-platdata requires it. It is not possible to use of-platdata > > > in a generic driver since the struct definitions rely on the > > > compatible string. > > > > > > > > > > > But I see from the Linux kernel driver, it has: > > > > > > > > 300static int intel_spi_init(struct intel_spi *ispi) > > > > 301{ > > > > 302 u32 opmenu0, opmenu1, lvscc, uvscc, val; > > > > 303 int i; > > > > 304 > > > > 305 switch (ispi->info->type) { > > > > 306 case INTEL_SPI_BYT: > > > > 307 ispi->sregs = ispi->base + BYT_SSFSTS_CTL; > > > > 308 ispi->pregs = ispi->base + BYT_PR; > > > > 309 ispi->nregions = BYT_FREG_NUM; > > > > 310 ispi->pr_num = BYT_PR_NUM; > > > > 311 ispi->swseq_reg = true; > > > > 312 > > > > 313 if (writeable) { > > > > 314 /* Disable write protection */ > > > > 315 val = readl(ispi->base + BYT_BCR); > > > > 316 if (!(val & BYT_BCR_WPD)) { > > > > 317 val |= BYT_BCR_WPD; > > > > 318 writel(val, ispi->base + BYT_BCR); > > > > 319 val = readl(ispi->base + BYT_BCR); > > > > 320 } > > > > 321 > > > > 322 ispi->writeable = !!(val & BYT_BCR_WPD); > > > > 323 } > > > > 324 > > > > 325 break; > > > > 326 > > > > 327 case INTEL_SPI_LPT: > > > > 328 ispi->sregs = ispi->base + LPT_SSFSTS_CTL; > > > > 329 ispi->pregs = ispi->base + LPT_PR; > > > > 330 ispi->nregions = LPT_FREG_NUM; > > > > 331 ispi->pr_num = LPT_PR_NUM; > > > > 332 ispi->swseq_reg = true; > > > > 333 break; > > > > 334 > > > > 335 case INTEL_SPI_BXT: > > > > 336 ispi->sregs = ispi->base + BXT_SSFSTS_CTL; > > > > 337 ispi->pregs = ispi->base + BXT_PR; > > > > 338 ispi->nregions = BXT_FREG_NUM; > > > > 339 ispi->pr_num = BXT_PR_NUM; > > > > 340 ispi->erase_64k = true; > > > > 341 break; > > > > > > > > So for INTEL_SPI_BXT (which is for ApolloLake I believe) > > > > ispi->swseq_reg is not set to true which means it uses hardware > > > > sequencer, which seems to contradict with what you found. > > > > > > I found that it only supports hardware sequencing, so this matches. > > > > OK, I see. So the BXT only supports HW SEQ while LPT and BYT support > > both SW and HW SEQ. > > OK, ta. How do I look up what LPT and BYT are? > I think LPT stands Linx Point (the PCH chipset that desktop processors use) and BYT stands BayTrail which U-Boot already supports. Regards, Bin
Hi Bin, On Sun, 20 Oct 2019 at 20:29, Bin Meng <bmeng.cn@gmail.com> wrote: > > Hi Simon, > > On Sat, Oct 19, 2019 at 4:37 AM Simon Glass <sjg@chromium.org> wrote: > > > > Hi Bin, > > > > On Fri, 18 Oct 2019 at 09:38, Bin Meng <bmeng.cn@gmail.com> wrote: > > > > > > Hi Simon, > > > > > > On Fri, Oct 18, 2019 at 10:14 PM Simon Glass <sjg@chromium.org> wrote: > > > > > > > > Hi Bin, > > > > > > > > On Thu, 17 Oct 2019 at 20:32, Bin Meng <bmeng.cn@gmail.com> wrote: > > > > > > > > > > Hi Simon, > > > > > > > > > > On Fri, Oct 18, 2019 at 10:22 AM Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > > > Hi, > > > > > > > > > > > > On Thu, 17 Oct 2019 at 08:28, Simon Glass <sjg@chromium.org> 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? > > > > > > > > > > > > > > I can see spi_flash_read_dm() but it does not support returning a > > > > > > > pointer to the data, only reading it. > > > > > > > > > > > > > > Also I cannot find any documentation about any of this. I've been > > > > > > > looking in the doc/ directory. > > > > > > > > > > > > > > I found the spi_mem.h file but it doesn't mention the meaning of the > > > > > > > in and out buffer pointers so I don't know how to use them. > > > > > > > > > > > > > > Is there an API missing or just comments/documentation? > > > > > > > > > > > > Apart from this I have found that the ich_spi driver does not work for > > > > > > APL since it apparently only supports 'hardware sequencing'. I did try > > > > > > getting software sequencing to work, but no dice. > > > > > > > > > > > > In addition, I found that enabling SPI flash, etc. added about 6KB of code! > > > > > > > > > > > > So I think it might be best to have two SPI drivers for x86 - one for > > > > > > software sequencing and one for hardware? > > > > > > > > > > So if this is true, I think we can create Kconfig options (HW_SEQ and > > > > > SW_SEQ) for ICH SPI driver, and select either one in the SoC Kconfig > > > > > file. > > > > > > > > I think at least for TPL I'm going to need to bypass all the code as > > > > it is just too big for TPL. What do you think? > > > > > > So what about other boards/drivers? Say we have drv_foo.c, is the best > > > practice to create a separate drv_foo_tpl.c for use with TPL? > > > > Yes, although in general we can export the functions from the core > > driver, so the TPL one becomes pretty bare-bones - e.g. it handles > > platdata but uses the operations of the core drivers. > > > > > > > > > > > > > Should we have a separate HW SEQ driver? If you look at the HW SEQ > > > > driver I have, it is very little code. > > > > > > > > Or are you saying we should have a combined driver with Kconfig > > > > options to enable either or both of SW SEQ and HW SEQ? > > > > > > > > > > Yes I was suggesting enable either or both SW and HW SEQ in the same driver. > > > > OK I will take a look at that. > > > > My code-size concern still stands though, but let's see how it looks. > > > > > > > > > One other wrinkle is that I have to have a separate driver anyway, > > > > since of-platdata requires it. It is not possible to use of-platdata > > > > in a generic driver since the struct definitions rely on the > > > > compatible string. > > > > > > > > > > > > > > But I see from the Linux kernel driver, it has: > > > > > > > > > > 300static int intel_spi_init(struct intel_spi *ispi) > > > > > 301{ > > > > > 302 u32 opmenu0, opmenu1, lvscc, uvscc, val; > > > > > 303 int i; > > > > > 304 > > > > > 305 switch (ispi->info->type) { > > > > > 306 case INTEL_SPI_BYT: > > > > > 307 ispi->sregs = ispi->base + BYT_SSFSTS_CTL; > > > > > 308 ispi->pregs = ispi->base + BYT_PR; > > > > > 309 ispi->nregions = BYT_FREG_NUM; > > > > > 310 ispi->pr_num = BYT_PR_NUM; > > > > > 311 ispi->swseq_reg = true; > > > > > 312 > > > > > 313 if (writeable) { > > > > > 314 /* Disable write protection */ > > > > > 315 val = readl(ispi->base + BYT_BCR); > > > > > 316 if (!(val & BYT_BCR_WPD)) { > > > > > 317 val |= BYT_BCR_WPD; > > > > > 318 writel(val, ispi->base + BYT_BCR); > > > > > 319 val = readl(ispi->base + BYT_BCR); > > > > > 320 } > > > > > 321 > > > > > 322 ispi->writeable = !!(val & BYT_BCR_WPD); > > > > > 323 } > > > > > 324 > > > > > 325 break; > > > > > 326 > > > > > 327 case INTEL_SPI_LPT: > > > > > 328 ispi->sregs = ispi->base + LPT_SSFSTS_CTL; > > > > > 329 ispi->pregs = ispi->base + LPT_PR; > > > > > 330 ispi->nregions = LPT_FREG_NUM; > > > > > 331 ispi->pr_num = LPT_PR_NUM; > > > > > 332 ispi->swseq_reg = true; > > > > > 333 break; > > > > > 334 > > > > > 335 case INTEL_SPI_BXT: > > > > > 336 ispi->sregs = ispi->base + BXT_SSFSTS_CTL; > > > > > 337 ispi->pregs = ispi->base + BXT_PR; > > > > > 338 ispi->nregions = BXT_FREG_NUM; > > > > > 339 ispi->pr_num = BXT_PR_NUM; > > > > > 340 ispi->erase_64k = true; > > > > > 341 break; > > > > > > > > > > So for INTEL_SPI_BXT (which is for ApolloLake I believe) > > > > > ispi->swseq_reg is not set to true which means it uses hardware > > > > > sequencer, which seems to contradict with what you found. > > > > > > > > I found that it only supports hardware sequencing, so this matches. > > > > > > OK, I see. So the BXT only supports HW SEQ while LPT and BYT support > > > both SW and HW SEQ. > > > > OK, ta. How do I look up what LPT and BYT are? > > > > I think LPT stands Linx Point (the PCH chipset that desktop processors > use) and BYT stands BayTrail which U-Boot already supports. OK ta. BTW I'm just preparing the new series. It will be v3 as I have brought in some patches from the other series which was v2. Regards, Simon
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 + */ + int (*get_mmap)(struct udevice *dev, ulong *map_basep, + size_t *map_sizep, u32 *offsetp); }; /* Access the serial operations for a device */ @@ -88,6 +101,20 @@ int spi_flash_write_dm(struct udevice *dev, u32 offset, size_t len, */ int spi_flash_erase_dm(struct udevice *dev, u32 offset, size_t len); +/** + * spi_flash_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, -ENOSYS if no operation, -EFAULT if memory mapping is not + * available + */ +int spi_flash_get_mmap(struct udevice *dev, ulong *map_basep, size_t *map_sizep, + u32 *offsetp); + /** * spl_flash_get_sw_write_prot() - Check state of software write-protect feature * diff --git a/test/dm/sf.c b/test/dm/sf.c index 56277954c23..f8f9e717720 100644 --- a/test/dm/sf.c +++ b/test/dm/sf.c @@ -102,6 +102,9 @@ static int dm_test_spi_flash_direct(struct unit_test_state *uts) { struct udevice *dev; char buf[BUF_SIZE]; + size_t map_size; + ulong map_base; + u32 offset; int i; ut_assertok(uclass_get_device(UCLASS_SPI_FLASH, 1, &dev)); @@ -130,6 +133,12 @@ static int dm_test_spi_flash_direct(struct unit_test_state *uts) ut_asserteq(0, spl_flash_get_sw_write_prot(dev)); ut_asserteq(1, spl_flash_get_sw_write_prot(dev)); + /* Check mapping */ + ut_assertok(spi_flash_get_mmap(dev, &map_base, &map_size, &offset)); + ut_asserteq(0x1000, map_base); + ut_asserteq(0x2000, map_size); + ut_asserteq(0x100, offset); + return 0; } DM_TEST(dm_test_spi_flash_direct, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
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. 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(+)