[PATCH] x86: spi: Only use the fast SPI peripheral when support

[PATCH] x86: spi: Only use the fast SPI peripheral when support

[Simon Glass]

At present we query the memory map on boards which don't support it. Fix
this by only doing it on Apollo Lake.

This fixes booting on chromebook_link.

Signed-off-by: Simon Glass <sjg@chromium.org>
Fixes: 92842147c31 ("spi: ich: Add support for get_mmap() method")
---

 drivers/spi/ich.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
index a9d7715a55..9f8af45242 100644
--- a/drivers/spi/ich.c
+++ b/drivers/spi/ich.c
@@ -637,7 +637,10 @@ static int ich_get_mmap(struct udevice *dev, ulong *map_basep, uint *map_sizep,
 			uint *offsetp)
 {
 	struct udevice *bus = dev_get_parent(dev);
+	struct ich_spi_platdata *plat = dev_get_platdata(bus);
 
+	if (plat->ich_version != ICHV_APL)
+		return -ENOENT;
 	return ich_get_mmap_bus(bus, map_basep, map_sizep, offsetp);
 }
 
-- 
2.25.1.696.g5e7596f4ac-goog

[PATCH] x86: spi: Only use the fast SPI peripheral when support

[Bin Meng]

Hi Simon,

On Tue, Mar 24, 2020 at 9:45 PM Simon Glass <sjg@chromium.org> wrote:
>
> At present we query the memory map on boards which don't support it. Fix
> this by only doing it on Apollo Lake.
>

I wonder isn't this check already covered in mrccache_get_region() below:

ret = dm_spi_get_mmap(dev, &map_base, &map_size, &offset);
if (!ret) {
entry->base = map_base;
} else {
ret = dev_read_u32_array(dev, "memory-map", reg, 2);
if (ret)
return log_msg_ret("Cannot find memory map\n", ret);
entry->base = reg[0];
}

> This fixes booting on chromebook_link.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Fixes: 92842147c31 ("spi: ich: Add support for get_mmap() method")
> ---
>
>  drivers/spi/ich.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
> index a9d7715a55..9f8af45242 100644
> --- a/drivers/spi/ich.c
> +++ b/drivers/spi/ich.c
> @@ -637,7 +637,10 @@ static int ich_get_mmap(struct udevice *dev, ulong *map_basep, uint *map_sizep,
>                         uint *offsetp)
>  {
>         struct udevice *bus = dev_get_parent(dev);
> +       struct ich_spi_platdata *plat = dev_get_platdata(bus);
>
> +       if (plat->ich_version != ICHV_APL)
> +               return -ENOENT;
>         return ich_get_mmap_bus(bus, map_basep, map_sizep, offsetp);
>  }

Regards,
Bin

[PATCH] x86: spi: Only use the fast SPI peripheral when support

[Simon Glass]

HI Bin,

On Wed, 25 Mar 2020 at 01:25, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Tue, Mar 24, 2020 at 9:45 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > At present we query the memory map on boards which don't support it. Fix
> > this by only doing it on Apollo Lake.
> >
>
> I wonder isn't this check already covered in mrccache_get_region() below:
>
> ret = dm_spi_get_mmap(dev, &map_base, &map_size, &offset);
> if (!ret) {
> entry->base = map_base;
> } else {
> ret = dev_read_u32_array(dev, "memory-map", reg, 2);
> if (ret)
> return log_msg_ret("Cannot find memory map\n", ret);
> entry->base = reg[0];
> }

Yes it is, so long as dm_spi_get_mmap() returns an error, as it does
with my patch.


>
> > This fixes booting on chromebook_link.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > Fixes: 92842147c31 ("spi: ich: Add support for get_mmap() method")
> > ---
> >
> >  drivers/spi/ich.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
> > index a9d7715a55..9f8af45242 100644
> > --- a/drivers/spi/ich.c
> > +++ b/drivers/spi/ich.c
> > @@ -637,7 +637,10 @@ static int ich_get_mmap(struct udevice *dev, ulong *map_basep, uint *map_sizep,
> >                         uint *offsetp)
> >  {
> >         struct udevice *bus = dev_get_parent(dev);
> > +       struct ich_spi_platdata *plat = dev_get_platdata(bus);
> >
> > +       if (plat->ich_version != ICHV_APL)
> > +               return -ENOENT;
> >         return ich_get_mmap_bus(bus, map_basep, map_sizep, offsetp);
> >  }
Regards,
Simon

[PATCH] x86: spi: Only use the fast SPI peripheral when support

[Bin Meng]

Hi Simon,

On Fri, Mar 27, 2020 at 12:20 AM Simon Glass <sjg@chromium.org> wrote:
>
> HI Bin,
>
> On Wed, 25 Mar 2020 at 01:25, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Tue, Mar 24, 2020 at 9:45 PM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > At present we query the memory map on boards which don't support it. Fix
> > > this by only doing it on Apollo Lake.
> > >
> >
> > I wonder isn't this check already covered in mrccache_get_region() below:
> >
> > ret = dm_spi_get_mmap(dev, &map_base, &map_size, &offset);
> > if (!ret) {
> > entry->base = map_base;
> > } else {
> > ret = dev_read_u32_array(dev, "memory-map", reg, 2);
> > if (ret)
> > return log_msg_ret("Cannot find memory map\n", ret);
> > entry->base = reg[0];
> > }
>
> Yes it is, so long as dm_spi_get_mmap() returns an error, as it does
> with my patch.

So does ich_get_mmap_bus() returns 0 on chromebook_link?

Regards,
Bin

[PATCH] x86: spi: Only use the fast SPI peripheral when support

[Simon Glass]

Hi Bin,



On Thu, 26 Mar 2020 at 10:38, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Fri, Mar 27, 2020 at 12:20 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > HI Bin,
> >
> > On Wed, 25 Mar 2020 at 01:25, Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Tue, Mar 24, 2020 at 9:45 PM Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > At present we query the memory map on boards which don't support it. Fix
> > > > this by only doing it on Apollo Lake.
> > > >
> > >
> > > I wonder isn't this check already covered in mrccache_get_region() below:
> > >
> > > ret = dm_spi_get_mmap(dev, &map_base, &map_size, &offset);
> > > if (!ret) {
> > > entry->base = map_base;
> > > } else {
> > > ret = dev_read_u32_array(dev, "memory-map", reg, 2);
> > > if (ret)
> > > return log_msg_ret("Cannot find memory map\n", ret);
> > > entry->base = reg[0];
> > > }
> >
> > Yes it is, so long as dm_spi_get_mmap() returns an error, as it does
> > with my patch.
>
> So does ich_get_mmap_bus() returns 0 on chromebook_link?

Regards,
Simon

[PATCH] x86: spi: Only use the fast SPI peripheral when support

[Simon Glass]

Hi Bin,

On Thu, 26 Mar 2020 at 10:38, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Fri, Mar 27, 2020 at 12:20 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > HI Bin,
> >
> > On Wed, 25 Mar 2020 at 01:25, Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Tue, Mar 24, 2020 at 9:45 PM Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > At present we query the memory map on boards which don't support it. Fix
> > > > this by only doing it on Apollo Lake.
> > > >
> > >
> > > I wonder isn't this check already covered in mrccache_get_region() below:
> > >
> > > ret = dm_spi_get_mmap(dev, &map_base, &map_size, &offset);
> > > if (!ret) {
> > > entry->base = map_base;
> > > } else {
> > > ret = dev_read_u32_array(dev, "memory-map", reg, 2);
> > > if (ret)
> > > return log_msg_ret("Cannot find memory map\n", ret);
> > > entry->base = reg[0];
> > > }
> >
> > Yes it is, so long as dm_spi_get_mmap() returns an error, as it does
> > with my patch.
>
> So does ich_get_mmap_bus() returns 0 on chromebook_link?

Well on link the SPI peripheral is not a PCI device but a child of the
PCH. It is possible to read the registers but at present this only
works once you have the mmio_base (i.e. the PCH device is probed).
This function needs to work before probing (since FSP-S access needs
to happen without probing PCI).

I suspect it would be possible to read the PCH base without probing
it, but it does add quite a bit of special-case code. What do you
think?

Regards,
Simon

[PATCH] x86: spi: Only use the fast SPI peripheral when support

[Bin Meng]

Hi Simon,

On Sun, Mar 29, 2020 at 4:58 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Bin,
>
> On Thu, 26 Mar 2020 at 10:38, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Fri, Mar 27, 2020 at 12:20 AM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > HI Bin,
> > >
> > > On Wed, 25 Mar 2020 at 01:25, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Tue, Mar 24, 2020 at 9:45 PM Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > At present we query the memory map on boards which don't support it. Fix
> > > > > this by only doing it on Apollo Lake.
> > > > >
> > > >
> > > > I wonder isn't this check already covered in mrccache_get_region() below:
> > > >
> > > > ret = dm_spi_get_mmap(dev, &map_base, &map_size, &offset);
> > > > if (!ret) {
> > > > entry->base = map_base;
> > > > } else {
> > > > ret = dev_read_u32_array(dev, "memory-map", reg, 2);
> > > > if (ret)
> > > > return log_msg_ret("Cannot find memory map\n", ret);
> > > > entry->base = reg[0];
> > > > }
> > >
> > > Yes it is, so long as dm_spi_get_mmap() returns an error, as it does
> > > with my patch.
> >
> > So does ich_get_mmap_bus() returns 0 on chromebook_link?
>
> Well on link the SPI peripheral is not a PCI device but a child of the
> PCH. It is possible to read the registers but at present this only
> works once you have the mmio_base (i.e. the PCH device is probed).
> This function needs to work before probing (since FSP-S access needs
> to happen without probing PCI).
>
> I suspect it would be possible to read the PCH base without probing
> it, but it does add quite a bit of special-case code. What do you
> think?

I've looked at this. So this function mrccache_get_region() is broken
on Minnowmax too. The call to uclass_find_first_device() returns
nothing because SPI flash is not probed hence no SF device is found.

Regards,
Bin

[PATCH] x86: spi: Only use the fast SPI peripheral when support

[Simon Glass]

Hi Bin,

On Mon, 30 Mar 2020 at 03:42, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Sun, Mar 29, 2020 at 4:58 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Bin,
> >
> > On Thu, 26 Mar 2020 at 10:38, Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Fri, Mar 27, 2020 at 12:20 AM Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > HI Bin,
> > > >
> > > > On Wed, 25 Mar 2020 at 01:25, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > On Tue, Mar 24, 2020 at 9:45 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > At present we query the memory map on boards which don't support it. Fix
> > > > > > this by only doing it on Apollo Lake.
> > > > > >
> > > > >
> > > > > I wonder isn't this check already covered in mrccache_get_region() below:
> > > > >
> > > > > ret = dm_spi_get_mmap(dev, &map_base, &map_size, &offset);
> > > > > if (!ret) {
> > > > > entry->base = map_base;
> > > > > } else {
> > > > > ret = dev_read_u32_array(dev, "memory-map", reg, 2);
> > > > > if (ret)
> > > > > return log_msg_ret("Cannot find memory map\n", ret);
> > > > > entry->base = reg[0];
> > > > > }
> > > >
> > > > Yes it is, so long as dm_spi_get_mmap() returns an error, as it does
> > > > with my patch.
> > >
> > > So does ich_get_mmap_bus() returns 0 on chromebook_link?
> >
> > Well on link the SPI peripheral is not a PCI device but a child of the
> > PCH. It is possible to read the registers but at present this only
> > works once you have the mmio_base (i.e. the PCH device is probed).
> > This function needs to work before probing (since FSP-S access needs
> > to happen without probing PCI).
> >
> > I suspect it would be possible to read the PCH base without probing
> > it, but it does add quite a bit of special-case code. What do you
> > think?
>
> I've looked at this. So this function mrccache_get_region() is broken
> on Minnowmax too. The call to uclass_find_first_device() returns
> nothing because SPI flash is not probed hence no SF device is found.

OK, so add to the DT, or do something else?

Regards,
Simon

[PATCH] x86: spi: Only use the fast SPI peripheral when support

[Bin Meng]

Hi Simon,

On Tue, Mar 31, 2020 at 7:57 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Bin,
>
> On Mon, 30 Mar 2020 at 03:42, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Sun, Mar 29, 2020 at 4:58 AM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Bin,
> > >
> > > On Thu, 26 Mar 2020 at 10:38, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Fri, Mar 27, 2020 at 12:20 AM Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > HI Bin,
> > > > >
> > > > > On Wed, 25 Mar 2020 at 01:25, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > >
> > > > > > Hi Simon,
> > > > > >
> > > > > > On Tue, Mar 24, 2020 at 9:45 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > > >
> > > > > > > At present we query the memory map on boards which don't support it. Fix
> > > > > > > this by only doing it on Apollo Lake.
> > > > > > >
> > > > > >
> > > > > > I wonder isn't this check already covered in mrccache_get_region() below:
> > > > > >
> > > > > > ret = dm_spi_get_mmap(dev, &map_base, &map_size, &offset);
> > > > > > if (!ret) {
> > > > > > entry->base = map_base;
> > > > > > } else {
> > > > > > ret = dev_read_u32_array(dev, "memory-map", reg, 2);
> > > > > > if (ret)
> > > > > > return log_msg_ret("Cannot find memory map\n", ret);
> > > > > > entry->base = reg[0];
> > > > > > }
> > > > >
> > > > > Yes it is, so long as dm_spi_get_mmap() returns an error, as it does
> > > > > with my patch.
> > > >
> > > > So does ich_get_mmap_bus() returns 0 on chromebook_link?
> > >
> > > Well on link the SPI peripheral is not a PCI device but a child of the
> > > PCH. It is possible to read the registers but at present this only
> > > works once you have the mmio_base (i.e. the PCH device is probed).
> > > This function needs to work before probing (since FSP-S access needs
> > > to happen without probing PCI).
> > >
> > > I suspect it would be possible to read the PCH base without probing
> > > it, but it does add quite a bit of special-case code. What do you
> > > think?
> >
> > I've looked at this. So this function mrccache_get_region() is broken
> > on Minnowmax too. The call to uclass_find_first_device() returns
> > nothing because SPI flash is not probed hence no SF device is found.
>
> OK, so add to the DT, or do something else?

There is already "memor-map" propert (see below) y in Chromebook DTS
so I am not sure what else needs to be added to the DT, as you
mentioned?

spi: spi {
#address-cells = <1>;
#size-cells = <0>;
compatible = "intel,ich9-spi";
u-boot,dm-pre-reloc;
spi-flash at 0 {
#size-cells = <1>;
#address-cells = <1>;
u-boot,dm-pre-reloc;
reg = <0>;
compatible = "winbond,w25q64",
"jedec,spi-nor";
memory-map = <0xff800000 0x00800000>;
rw-mrc-cache {
label = "rw-mrc-cache";
reg = <0x003e0000 0x00010000>;
u-boot,dm-pre-reloc;
};
};
};

So isn't the failure on Link caused by uclass_find_first_device()? I
think replacing uclass_find_first_device() with uclass_first_device()
will fix failures for at least Minnowmax? The comment says:

/*
* Find the flash chip within the SPI controller node. Avoid probing
* the device here since it may put it into a strange state where the
* memory map cannot be read.
*/

What issue did you see if we probe the SPI controller and flash?

Regards,
Bin

[PATCH] x86: spi: Only use the fast SPI peripheral when support

[Simon Glass]

Hi Bin,

On Tue, 31 Mar 2020 at 03:50, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Tue, Mar 31, 2020 at 7:57 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Bin,
> >
> > On Mon, 30 Mar 2020 at 03:42, Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Sun, Mar 29, 2020 at 4:58 AM Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Bin,
> > > >
> > > > On Thu, 26 Mar 2020 at 10:38, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > On Fri, Mar 27, 2020 at 12:20 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > HI Bin,
> > > > > >
> > > > > > On Wed, 25 Mar 2020 at 01:25, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > >
> > > > > > > Hi Simon,
> > > > > > >
> > > > > > > On Tue, Mar 24, 2020 at 9:45 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > >
> > > > > > > > At present we query the memory map on boards which don't support it. Fix
> > > > > > > > this by only doing it on Apollo Lake.
> > > > > > > >
> > > > > > >
> > > > > > > I wonder isn't this check already covered in mrccache_get_region() below:
> > > > > > >
> > > > > > > ret = dm_spi_get_mmap(dev, &map_base, &map_size, &offset);
> > > > > > > if (!ret) {
> > > > > > > entry->base = map_base;
> > > > > > > } else {
> > > > > > > ret = dev_read_u32_array(dev, "memory-map", reg, 2);
> > > > > > > if (ret)
> > > > > > > return log_msg_ret("Cannot find memory map\n", ret);
> > > > > > > entry->base = reg[0];
> > > > > > > }
> > > > > >
> > > > > > Yes it is, so long as dm_spi_get_mmap() returns an error, as it does
> > > > > > with my patch.
> > > > >
> > > > > So does ich_get_mmap_bus() returns 0 on chromebook_link?
> > > >
> > > > Well on link the SPI peripheral is not a PCI device but a child of the
> > > > PCH. It is possible to read the registers but at present this only
> > > > works once you have the mmio_base (i.e. the PCH device is probed).
> > > > This function needs to work before probing (since FSP-S access needs
> > > > to happen without probing PCI).
> > > >
> > > > I suspect it would be possible to read the PCH base without probing
> > > > it, but it does add quite a bit of special-case code. What do you
> > > > think?
> > >
> > > I've looked at this. So this function mrccache_get_region() is broken
> > > on Minnowmax too. The call to uclass_find_first_device() returns
> > > nothing because SPI flash is not probed hence no SF device is found.
> >
> > OK, so add to the DT, or do something else?
>
> There is already "memor-map" propert (see below) y in Chromebook DTS
> so I am not sure what else needs to be added to the DT, as you
> mentioned?
>
> spi: spi {
> #address-cells = <1>;
> #size-cells = <0>;
> compatible = "intel,ich9-spi";
> u-boot,dm-pre-reloc;
> spi-flash at 0 {
> #size-cells = <1>;
> #address-cells = <1>;
> u-boot,dm-pre-reloc;
> reg = <0>;
> compatible = "winbond,w25q64",
> "jedec,spi-nor";
> memory-map = <0xff800000 0x00800000>;
> rw-mrc-cache {
> label = "rw-mrc-cache";
> reg = <0x003e0000 0x00010000>;
> u-boot,dm-pre-reloc;
> };
> };
> };
>
> So isn't the failure on Link caused by uclass_find_first_device()? I
> think replacing uclass_find_first_device() with uclass_first_device()
> will fix failures for at least Minnowmax? The comment says:
>
> /*
> * Find the flash chip within the SPI controller node. Avoid probing
> * the device here since it may put it into a strange state where the
> * memory map cannot be read.
> */
>
> What issue did you see if we probe the SPI controller and flash?

I think you have the wrong end of the stick and conflating the
bahaviour on several different platforms.

link - uses memory-map, hence this patch to make sure that mmap
returns an error so that link falls back to memory-map
coral - uses mmap
minnowmax - not sure, but I suspect it needs memory-map too

Regards,
Simon

[PATCH] x86: spi: Only use the fast SPI peripheral when support

[Simon Glass]

Hi Bin,

On Tue, 31 Mar 2020 at 07:16, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Bin,
>
> On Tue, 31 Mar 2020 at 03:50, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Tue, Mar 31, 2020 at 7:57 AM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Bin,
> > >
> > > On Mon, 30 Mar 2020 at 03:42, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Sun, Mar 29, 2020 at 4:58 AM Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Bin,
> > > > >
> > > > > On Thu, 26 Mar 2020 at 10:38, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > >
> > > > > > Hi Simon,
> > > > > >
> > > > > > On Fri, Mar 27, 2020 at 12:20 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > > >
> > > > > > > HI Bin,
> > > > > > >
> > > > > > > On Wed, 25 Mar 2020 at 01:25, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > > >
> > > > > > > > Hi Simon,
> > > > > > > >
> > > > > > > > On Tue, Mar 24, 2020 at 9:45 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > >
> > > > > > > > > At present we query the memory map on boards which don't support it. Fix
> > > > > > > > > this by only doing it on Apollo Lake.
> > > > > > > > >
> > > > > > > >
> > > > > > > > I wonder isn't this check already covered in mrccache_get_region() below:
> > > > > > > >
> > > > > > > > ret = dm_spi_get_mmap(dev, &map_base, &map_size, &offset);
> > > > > > > > if (!ret) {
> > > > > > > > entry->base = map_base;
> > > > > > > > } else {
> > > > > > > > ret = dev_read_u32_array(dev, "memory-map", reg, 2);
> > > > > > > > if (ret)
> > > > > > > > return log_msg_ret("Cannot find memory map\n", ret);
> > > > > > > > entry->base = reg[0];
> > > > > > > > }
> > > > > > >
> > > > > > > Yes it is, so long as dm_spi_get_mmap() returns an error, as it does
> > > > > > > with my patch.
> > > > > >
> > > > > > So does ich_get_mmap_bus() returns 0 on chromebook_link?
> > > > >
> > > > > Well on link the SPI peripheral is not a PCI device but a child of the
> > > > > PCH. It is possible to read the registers but at present this only
> > > > > works once you have the mmio_base (i.e. the PCH device is probed).
> > > > > This function needs to work before probing (since FSP-S access needs
> > > > > to happen without probing PCI).
> > > > >
> > > > > I suspect it would be possible to read the PCH base without probing
> > > > > it, but it does add quite a bit of special-case code. What do you
> > > > > think?
> > > >
> > > > I've looked at this. So this function mrccache_get_region() is broken
> > > > on Minnowmax too. The call to uclass_find_first_device() returns
> > > > nothing because SPI flash is not probed hence no SF device is found.
> > >
> > > OK, so add to the DT, or do something else?
> >
> > There is already "memor-map" propert (see below) y in Chromebook DTS
> > so I am not sure what else needs to be added to the DT, as you
> > mentioned?
> >
> > spi: spi {
> > #address-cells = <1>;
> > #size-cells = <0>;
> > compatible = "intel,ich9-spi";
> > u-boot,dm-pre-reloc;
> > spi-flash at 0 {
> > #size-cells = <1>;
> > #address-cells = <1>;
> > u-boot,dm-pre-reloc;
> > reg = <0>;
> > compatible = "winbond,w25q64",
> > "jedec,spi-nor";
> > memory-map = <0xff800000 0x00800000>;
> > rw-mrc-cache {
> > label = "rw-mrc-cache";
> > reg = <0x003e0000 0x00010000>;
> > u-boot,dm-pre-reloc;
> > };
> > };
> > };
> >
> > So isn't the failure on Link caused by uclass_find_first_device()? I
> > think replacing uclass_find_first_device() with uclass_first_device()
> > will fix failures for at least Minnowmax? The comment says:
> >
> > /*
> > * Find the flash chip within the SPI controller node. Avoid probing
> > * the device here since it may put it into a strange state where the
> > * memory map cannot be read.
> > */
> >
> > What issue did you see if we probe the SPI controller and flash?
>
> I think you have the wrong end of the stick and conflating the
> bahaviour on several different platforms.
>
> link - uses memory-map, hence this patch to make sure that mmap
> returns an error so that link falls back to memory-map
> coral - uses mmap
> minnowmax - not sure, but I suspect it needs memory-map too

At present link and samus are broken without this patch.

It just reverts us back to the behaviour before the ICH TPL support.
What can we do to get this applied?

Regards,
Simon

[PATCH] x86: spi: Only use the fast SPI peripheral when support

[Bin Meng]

Hi Simon,

On Mon, Apr 20, 2020 at 3:14 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Bin,
>
> On Tue, 31 Mar 2020 at 07:16, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Bin,
> >
> > On Tue, 31 Mar 2020 at 03:50, Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Tue, Mar 31, 2020 at 7:57 AM Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Bin,
> > > >
> > > > On Mon, 30 Mar 2020 at 03:42, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > On Sun, Mar 29, 2020 at 4:58 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Hi Bin,
> > > > > >
> > > > > > On Thu, 26 Mar 2020 at 10:38, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > >
> > > > > > > Hi Simon,
> > > > > > >
> > > > > > > On Fri, Mar 27, 2020 at 12:20 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > >
> > > > > > > > HI Bin,
> > > > > > > >
> > > > > > > > On Wed, 25 Mar 2020 at 01:25, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > Hi Simon,
> > > > > > > > >
> > > > > > > > > On Tue, Mar 24, 2020 at 9:45 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > >
> > > > > > > > > > At present we query the memory map on boards which don't support it. Fix
> > > > > > > > > > this by only doing it on Apollo Lake.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > I wonder isn't this check already covered in mrccache_get_region() below:
> > > > > > > > >
> > > > > > > > > ret = dm_spi_get_mmap(dev, &map_base, &map_size, &offset);
> > > > > > > > > if (!ret) {
> > > > > > > > > entry->base = map_base;
> > > > > > > > > } else {
> > > > > > > > > ret = dev_read_u32_array(dev, "memory-map", reg, 2);
> > > > > > > > > if (ret)
> > > > > > > > > return log_msg_ret("Cannot find memory map\n", ret);
> > > > > > > > > entry->base = reg[0];
> > > > > > > > > }
> > > > > > > >
> > > > > > > > Yes it is, so long as dm_spi_get_mmap() returns an error, as it does
> > > > > > > > with my patch.
> > > > > > >
> > > > > > > So does ich_get_mmap_bus() returns 0 on chromebook_link?
> > > > > >
> > > > > > Well on link the SPI peripheral is not a PCI device but a child of the
> > > > > > PCH. It is possible to read the registers but at present this only
> > > > > > works once you have the mmio_base (i.e. the PCH device is probed).
> > > > > > This function needs to work before probing (since FSP-S access needs
> > > > > > to happen without probing PCI).
> > > > > >
> > > > > > I suspect it would be possible to read the PCH base without probing
> > > > > > it, but it does add quite a bit of special-case code. What do you
> > > > > > think?
> > > > >
> > > > > I've looked at this. So this function mrccache_get_region() is broken
> > > > > on Minnowmax too. The call to uclass_find_first_device() returns
> > > > > nothing because SPI flash is not probed hence no SF device is found.
> > > >
> > > > OK, so add to the DT, or do something else?
> > >
> > > There is already "memor-map" propert (see below) y in Chromebook DTS
> > > so I am not sure what else needs to be added to the DT, as you
> > > mentioned?
> > >
> > > spi: spi {
> > > #address-cells = <1>;
> > > #size-cells = <0>;
> > > compatible = "intel,ich9-spi";
> > > u-boot,dm-pre-reloc;
> > > spi-flash at 0 {
> > > #size-cells = <1>;
> > > #address-cells = <1>;
> > > u-boot,dm-pre-reloc;
> > > reg = <0>;
> > > compatible = "winbond,w25q64",
> > > "jedec,spi-nor";
> > > memory-map = <0xff800000 0x00800000>;
> > > rw-mrc-cache {
> > > label = "rw-mrc-cache";
> > > reg = <0x003e0000 0x00010000>;
> > > u-boot,dm-pre-reloc;
> > > };
> > > };
> > > };
> > >
> > > So isn't the failure on Link caused by uclass_find_first_device()? I
> > > think replacing uclass_find_first_device() with uclass_first_device()
> > > will fix failures for at least Minnowmax? The comment says:
> > >
> > > /*
> > > * Find the flash chip within the SPI controller node. Avoid probing
> > > * the device here since it may put it into a strange state where the
> > > * memory map cannot be read.
> > > */
> > >
> > > What issue did you see if we probe the SPI controller and flash?
> >
> > I think you have the wrong end of the stick and conflating the
> > bahaviour on several different platforms.
> >
> > link - uses memory-map, hence this patch to make sure that mmap
> > returns an error so that link falls back to memory-map
> > coral - uses mmap
> > minnowmax - not sure, but I suspect it needs memory-map too
>
> At present link and samus are broken without this patch.
>
> It just reverts us back to the behaviour before the ICH TPL support.
> What can we do to get this applied?

Sorry I missed this. I will take a look.

Regards,
Bin

[PATCH] x86: spi: Only use the fast SPI peripheral when support

[Bin Meng]

Hi Simon,

On Tue, Mar 24, 2020 at 9:45 PM Simon Glass <sjg@chromium.org> wrote:
>
> At present we query the memory map on boards which don't support it. Fix
> this by only doing it on Apollo Lake.
>
> This fixes booting on chromebook_link.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Fixes: 92842147c31 ("spi: ich: Add support for get_mmap() method")
> ---
>
>  drivers/spi/ich.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
> index a9d7715a55..9f8af45242 100644
> --- a/drivers/spi/ich.c
> +++ b/drivers/spi/ich.c
> @@ -637,7 +637,10 @@ static int ich_get_mmap(struct udevice *dev, ulong *map_basep, uint *map_sizep,
>                         uint *offsetp)
>  {
>         struct udevice *bus = dev_get_parent(dev);
> +       struct ich_spi_platdata *plat = dev_get_platdata(bus);
>
> +       if (plat->ich_version != ICHV_APL)
> +               return -ENOENT;
>         return ich_get_mmap_bus(bus, map_basep, map_sizep, offsetp);
>  }

Although the check added here is correct, it is still an incomplete fix.

I don't understand why chromebook_link boot failure is caused by this
because as I mentioned before in mrccache_get_region() the call to
uclass_find_first_device(UCLASS_SPI_FLASH, &dev) already returns hence
there is no chance to execute dm_spi_get_mmap() on link.

Regards,
Bin