x86: apl: Acessing SPI flash when booting with Coreboot/U-Boot

Hi Simon,

Since commit 609b90a6a9c0 ("x86: spi: Rewrite logic for obtaining the SPI
memory map") I have trouble accessing the SPI flash on my Apollo Lake board
when booting with Coreboot and having U-Boot as the payload.

Accessing the SPI flash returns -91 (EPROTOTYPE).

My understanding of what happens is the following:

 - ich_spi_ofdata_to_platdata() calls ich_spi_get_basics(), the parameter
   'can_probe' is hardcoded to true
 - There is no PCH device in my devicetree (there is also no PCH driver
   contained in my build)
 - As can_probe is true, ich_spi_get_basics() tries to find a PCH device
   and returns -EPROTOTYPE as it finds none

As far as I see the PCH is not used in the code paths relevant to Apollo Lake.
I think this behavior can not be triggered in a standalone U-Boot on Apollo Lake,
as in this case arch_cpu_init_tpl() requires an LPC, which is below the PCH,
and so a PCH has to be part of the devicetree anyway. In this case a PCH would
be found in ich_spi_get_basics(), but it would not get used.
   
As a quick workaround [1], I have set can_probe hardcoded to false, and with
this change I can access the SPI flash again.

Do you have any advice on how to fix this?
How about making the value for can_probe depend on a check for
ich_version == ICHV_APL?

regards, Wolfgang


[1] Workaround patch:

Hi Wolfgang,

On Fri, 14 Aug 2020 at 08:04, Wolfgang Wallner
<wolfgang.wallner@br-automation.com> wrote:
>
> Hi Simon,
>
> Since commit 609b90a6a9c0 ("x86: spi: Rewrite logic for obtaining the SPI
> memory map") I have trouble accessing the SPI flash on my Apollo Lake board
> when booting with Coreboot and having U-Boot as the payload.
>
> Accessing the SPI flash returns -91 (EPROTOTYPE).
>
> My understanding of what happens is the following:
>
>  - ich_spi_ofdata_to_platdata() calls ich_spi_get_basics(), the parameter
>    'can_probe' is hardcoded to true
>  - There is no PCH device in my devicetree (there is also no PCH driver
>    contained in my build)
>  - As can_probe is true, ich_spi_get_basics() tries to find a PCH device
>    and returns -EPROTOTYPE as it finds none
>
> As far as I see the PCH is not used in the code paths relevant to Apollo Lake.
> I think this behavior can not be triggered in a standalone U-Boot on Apollo Lake,
> as in this case arch_cpu_init_tpl() requires an LPC, which is below the PCH,
> and so a PCH has to be part of the devicetree anyway. In this case a PCH would
> be found in ich_spi_get_basics(), but it would not get used.
>
> As a quick workaround [1], I have set can_probe hardcoded to false, and with
> this change I can access the SPI flash again.
>
> Do you have any advice on how to fix this?
> How about making the value for can_probe depend on a check for
> ich_version == ICHV_APL?

That might be OK I think.

I am quite unhappy with how this has turned out. It seems like we
might be able to refactor it to reduce the number of cases.

>
> regards, Wolfgang
>
>
> [1] Workaround patch:
>
> --- a/drivers/spi/ich.c
> +++ b/drivers/spi/ich.c
> @@ -1016,7 +1016,7 @@ static int ich_spi_ofdata_to_platdata(struct udevice *dev)
>  #if !CONFIG_IS_ENABLED(OF_PLATDATA)
>         struct ich_spi_priv *priv = dev_get_priv(dev);
>
> -       ret = ich_spi_get_basics(dev, true, &priv->pch, &plat->ich_version,
> +       ret = ich_spi_get_basics(dev, false, &priv->pch, &plat->ich_version,
>
>
>

Regards,
Simon

Hi Simon, Wolfgang,

On Sun, Aug 16, 2020 at 11:40 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Wolfgang,
>
> On Fri, 14 Aug 2020 at 08:04, Wolfgang Wallner
> <wolfgang.wallner@br-automation.com> wrote:
> >
> > Hi Simon,
> >
> > Since commit 609b90a6a9c0 ("x86: spi: Rewrite logic for obtaining the SPI
> > memory map") I have trouble accessing the SPI flash on my Apollo Lake board
> > when booting with Coreboot and having U-Boot as the payload.
> >
> > Accessing the SPI flash returns -91 (EPROTOTYPE).
> >
> > My understanding of what happens is the following:
> >
> >  - ich_spi_ofdata_to_platdata() calls ich_spi_get_basics(), the parameter
> >    'can_probe' is hardcoded to true
> >  - There is no PCH device in my devicetree (there is also no PCH driver
> >    contained in my build)
> >  - As can_probe is true, ich_spi_get_basics() tries to find a PCH device
> >    and returns -EPROTOTYPE as it finds none
> >
> > As far as I see the PCH is not used in the code paths relevant to Apollo Lake.
> > I think this behavior can not be triggered in a standalone U-Boot on Apollo Lake,
> > as in this case arch_cpu_init_tpl() requires an LPC, which is below the PCH,
> > and so a PCH has to be part of the devicetree anyway. In this case a PCH would
> > be found in ich_spi_get_basics(), but it would not get used.
> >
> > As a quick workaround [1], I have set can_probe hardcoded to false, and with
> > this change I can access the SPI flash again.
> >
> > Do you have any advice on how to fix this?
> > How about making the value for can_probe depend on a check for
> > ich_version == ICHV_APL?
>
> That might be OK I think.
>
> I am quite unhappy with how this has turned out. It seems like we
> might be able to refactor it to reduce the number of cases.

Since this patch is viewed as only a workaround, would you propose a
fix for this issue?

Regards,
Bin