[RFC,1/2] arch: x86: apl: Only load VBT if CONFIG_HAVE_VBT is enabled

Only load VBT if it's present in the u-boot.rom.

Signed-off-by: Bernhard Messerklinger <bernhard.messerklinger at br-automation.com>
---

 arch/x86/cpu/apollolake/fsp_s.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

Hi Bernhard,

On Thu, 30 Apr 2020 at 03:16, Bernhard Messerklinger
<bernhard.messerklinger at br-automation.com> wrote:
>
> Only load VBT if it's present in the u-boot.rom.
>

I think you can drop the RFC from this series. The approach seems good to me.

Also, what APL boards have you tested with?

> Signed-off-by: Bernhard Messerklinger <bernhard.messerklinger at br-automation.com>
> ---
>
>  arch/x86/cpu/apollolake/fsp_s.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/arch/x86/cpu/apollolake/fsp_s.c b/arch/x86/cpu/apollolake/fsp_s.c
> index 17cf1682ad..8f1d6f3008 100644
> --- a/arch/x86/cpu/apollolake/fsp_s.c
> +++ b/arch/x86/cpu/apollolake/fsp_s.c
> @@ -327,16 +327,17 @@ int fsps_update_config(struct udevice *dev, ulong rom_offset,
>  {
>         struct fsp_s_config *cfg = &upd->config;
>         struct apl_config *apl;
> +#ifdef CONFIG_HAVE_VBT

Please use if (IS_ENABLED(CONFIG_HAVE_VBT))

We should try to avoid #ifdef unless needed as it adds different build paths.

>         struct binman_entry vbt;
> -       void *buf;
> +       void *vbt_buf;
>         int ret;
>
>         ret = binman_entry_find("intel-vbt", &vbt);
>         if (ret)
>                 return log_msg_ret("Cannot find VBT", ret);
>         vbt.image_pos += rom_offset;
> -       buf = malloc(vbt.size);
> -       if (!buf)
> +       vbt_buf = malloc(vbt.size);
> +       if (!vbt_buf)
>                 return log_msg_ret("Alloc VBT", -ENOMEM);
>
>         /*
> @@ -344,16 +345,13 @@ int fsps_update_config(struct udevice *dev, ulong rom_offset,
>          * memory-mapped SPI at present.
>          */
>         bootstage_start(BOOTSTAGE_ID_ACCUM_MMAP_SPI, "mmap_spi");
> -       memcpy(buf, (void *)vbt.image_pos, vbt.size);
> +       memcpy(vbt_buf, (void *)vbt.image_pos, vbt.size);
>         bootstage_accum(BOOTSTAGE_ID_ACCUM_MMAP_SPI);
> -       if (*(u32 *)buf != VBT_SIGNATURE)
> +       if (*(u32 *)vbt_buf != VBT_SIGNATURE)
>                 return log_msg_ret("VBT signature", -EINVAL);
> -       cfg->graphics_config_ptr = (ulong)buf;
>
> -       apl = malloc(sizeof(*apl));
> -       if (!apl)
> -               return log_msg_ret("config", -ENOMEM);
> -       get_config(dev, apl);

Should not drop the above code.

> +       cfg->graphics_config_ptr = (ulong)vbt_buf;
> +#endif
>
>         cfg->ish_enable = 0;
>         cfg->enable_sata = 0;
> --
> 2.26.0
>
>

Regards,
Simon

Hi Simon,

>Hi Bernhard,
>
>On Thu, 30 Apr 2020 at 03:16, Bernhard Messerklinger
><bernhard.messerklinger at br-automation.com> wrote:
>>
>> Only load VBT if it's present in the u-boot.rom.
>>
>
>I think you can drop the RFC from this series. The approach seems
>good to me.
>
>Also, what APL boards have you tested with?
I tested our custom APL smarc board.
I also have a Oxbow Hill CRB but can't test it there at the moment
(currently I am working from home).

>
>> Signed-off-by: Bernhard Messerklinger
><bernhard.messerklinger at br-automation.com>
>> ---
>>
>>  arch/x86/cpu/apollolake/fsp_s.c | 18 ++++++++----------
>>  1 file changed, 8 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/x86/cpu/apollolake/fsp_s.c
>b/arch/x86/cpu/apollolake/fsp_s.c
>> index 17cf1682ad..8f1d6f3008 100644
>> --- a/arch/x86/cpu/apollolake/fsp_s.c
>> +++ b/arch/x86/cpu/apollolake/fsp_s.c
>> @@ -327,16 +327,17 @@ int fsps_update_config(struct udevice *dev,
>ulong rom_offset,
>>  {
>>         struct fsp_s_config *cfg = &upd->config;
>>         struct apl_config *apl;
>> +#ifdef CONFIG_HAVE_VBT
>
>Please use if (IS_ENABLED(CONFIG_HAVE_VBT))
>
>We should try to avoid #ifdef unless needed as it adds different
>build paths.
>
>>         struct binman_entry vbt;
>> -       void *buf;
>> +       void *vbt_buf;
>>         int ret;
>>
>>         ret = binman_entry_find("intel-vbt", &vbt);
>>         if (ret)
>>                 return log_msg_ret("Cannot find VBT", ret);
>>         vbt.image_pos += rom_offset;
>> -       buf = malloc(vbt.size);
>> -       if (!buf)
>> +       vbt_buf = malloc(vbt.size);
>> +       if (!vbt_buf)
>>                 return log_msg_ret("Alloc VBT", -ENOMEM);
>>
>>         /*
>> @@ -344,16 +345,13 @@ int fsps_update_config(struct udevice *dev,
>ulong rom_offset,
>>          * memory-mapped SPI at present.
>>          */
>>         bootstage_start(BOOTSTAGE_ID_ACCUM_MMAP_SPI, "mmap_spi");
>> -       memcpy(buf, (void *)vbt.image_pos, vbt.size);
>> +       memcpy(vbt_buf, (void *)vbt.image_pos, vbt.size);
>>         bootstage_accum(BOOTSTAGE_ID_ACCUM_MMAP_SPI);
>> -       if (*(u32 *)buf != VBT_SIGNATURE)
>> +       if (*(u32 *)vbt_buf != VBT_SIGNATURE)
>>                 return log_msg_ret("VBT signature", -EINVAL);
>> -       cfg->graphics_config_ptr = (ulong)buf;
>>
>> -       apl = malloc(sizeof(*apl));
>> -       if (!apl)
>> -               return log_msg_ret("config", -ENOMEM);
>> -       get_config(dev, apl);
>
>Should not drop the above code.
>
>> +       cfg->graphics_config_ptr = (ulong)vbt_buf;
>> +#endif
>>
>>         cfg->ish_enable = 0;
>>         cfg->enable_sata = 0;
>> --
>> 2.26.0
>>
>>
>
>Regards,
>Simon

Regards,
Bernhard