| Message ID | 20200324074524.1.Ibc9c511db58caa8a1e4c56d7e7824d7690718aeb@changeid |
|---|---|
| State | New |
| Headers | show |
| Series | x86: spi: Only use the fast SPI peripheral when support | expand |
Hi Simon, On Tue, Mar 24, 2020 at 9:45 PM Simon Glass <sjg at 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 at 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
HI Bin, On Wed, 25 Mar 2020 at 01:25, Bin Meng <bmeng.cn at gmail.com> wrote: > > Hi Simon, > > On Tue, Mar 24, 2020 at 9:45 PM Simon Glass <sjg at 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 at 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
Hi Simon, On Fri, Mar 27, 2020 at 12:20 AM Simon Glass <sjg at chromium.org> wrote: > > HI Bin, > > On Wed, 25 Mar 2020 at 01:25, Bin Meng <bmeng.cn at gmail.com> wrote: > > > > Hi Simon, > > > > On Tue, Mar 24, 2020 at 9:45 PM Simon Glass <sjg at 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
Hi Bin, On Thu, 26 Mar 2020 at 10:38, Bin Meng <bmeng.cn at gmail.com> wrote: > > Hi Simon, > > On Fri, Mar 27, 2020 at 12:20 AM Simon Glass <sjg at chromium.org> wrote: > > > > HI Bin, > > > > On Wed, 25 Mar 2020 at 01:25, Bin Meng <bmeng.cn at gmail.com> wrote: > > > > > > Hi Simon, > > > > > > On Tue, Mar 24, 2020 at 9:45 PM Simon Glass <sjg at 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
Hi Bin, On Thu, 26 Mar 2020 at 10:38, Bin Meng <bmeng.cn at gmail.com> wrote: > > Hi Simon, > > On Fri, Mar 27, 2020 at 12:20 AM Simon Glass <sjg at chromium.org> wrote: > > > > HI Bin, > > > > On Wed, 25 Mar 2020 at 01:25, Bin Meng <bmeng.cn at gmail.com> wrote: > > > > > > Hi Simon, > > > > > > On Tue, Mar 24, 2020 at 9:45 PM Simon Glass <sjg at 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
Hi Simon, On Sun, Mar 29, 2020 at 4:58 AM Simon Glass <sjg at chromium.org> wrote: > > Hi Bin, > > On Thu, 26 Mar 2020 at 10:38, Bin Meng <bmeng.cn at gmail.com> wrote: > > > > Hi Simon, > > > > On Fri, Mar 27, 2020 at 12:20 AM Simon Glass <sjg at chromium.org> wrote: > > > > > > HI Bin, > > > > > > On Wed, 25 Mar 2020 at 01:25, Bin Meng <bmeng.cn at gmail.com> wrote: > > > > > > > > Hi Simon, > > > > > > > > On Tue, Mar 24, 2020 at 9:45 PM Simon Glass <sjg at 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
Hi Bin, On Mon, 30 Mar 2020 at 03:42, Bin Meng <bmeng.cn at gmail.com> wrote: > > Hi Simon, > > On Sun, Mar 29, 2020 at 4:58 AM Simon Glass <sjg at chromium.org> wrote: > > > > Hi Bin, > > > > On Thu, 26 Mar 2020 at 10:38, Bin Meng <bmeng.cn at gmail.com> wrote: > > > > > > Hi Simon, > > > > > > On Fri, Mar 27, 2020 at 12:20 AM Simon Glass <sjg at chromium.org> wrote: > > > > > > > > HI Bin, > > > > > > > > On Wed, 25 Mar 2020 at 01:25, Bin Meng <bmeng.cn at gmail.com> wrote: > > > > > > > > > > Hi Simon, > > > > > > > > > > On Tue, Mar 24, 2020 at 9:45 PM Simon Glass <sjg at 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
Hi Simon, On Tue, Mar 31, 2020 at 7:57 AM Simon Glass <sjg at chromium.org> wrote: > > Hi Bin, > > On Mon, 30 Mar 2020 at 03:42, Bin Meng <bmeng.cn at gmail.com> wrote: > > > > Hi Simon, > > > > On Sun, Mar 29, 2020 at 4:58 AM Simon Glass <sjg at chromium.org> wrote: > > > > > > Hi Bin, > > > > > > On Thu, 26 Mar 2020 at 10:38, Bin Meng <bmeng.cn at gmail.com> wrote: > > > > > > > > Hi Simon, > > > > > > > > On Fri, Mar 27, 2020 at 12:20 AM Simon Glass <sjg at chromium.org> wrote: > > > > > > > > > > HI Bin, > > > > > > > > > > On Wed, 25 Mar 2020 at 01:25, Bin Meng <bmeng.cn at gmail.com> wrote: > > > > > > > > > > > > Hi Simon, > > > > > > > > > > > > On Tue, Mar 24, 2020 at 9:45 PM Simon Glass <sjg at 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
Hi Bin, On Tue, 31 Mar 2020 at 03:50, Bin Meng <bmeng.cn at gmail.com> wrote: > > Hi Simon, > > On Tue, Mar 31, 2020 at 7:57 AM Simon Glass <sjg at chromium.org> wrote: > > > > Hi Bin, > > > > On Mon, 30 Mar 2020 at 03:42, Bin Meng <bmeng.cn at gmail.com> wrote: > > > > > > Hi Simon, > > > > > > On Sun, Mar 29, 2020 at 4:58 AM Simon Glass <sjg at chromium.org> wrote: > > > > > > > > Hi Bin, > > > > > > > > On Thu, 26 Mar 2020 at 10:38, Bin Meng <bmeng.cn at gmail.com> wrote: > > > > > > > > > > Hi Simon, > > > > > > > > > > On Fri, Mar 27, 2020 at 12:20 AM Simon Glass <sjg at chromium.org> wrote: > > > > > > > > > > > > HI Bin, > > > > > > > > > > > > On Wed, 25 Mar 2020 at 01:25, Bin Meng <bmeng.cn at gmail.com> wrote: > > > > > > > > > > > > > > Hi Simon, > > > > > > > > > > > > > > On Tue, Mar 24, 2020 at 9:45 PM Simon Glass <sjg at 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
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); }
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 at chromium.org> Fixes: 92842147c31 ("spi: ich: Add support for get_mmap() method") --- drivers/spi/ich.c | 3 +++ 1 file changed, 3 insertions(+)