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

[Bernhard Messerklinger]

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