[RFC PATCH v2 2/2] arch: x86: apl: Read FSP-S configuration from device-tree

[Bernhard Messerklinger]

Hi Simon,
>
>Hi Bernhard,
>
>On Mon, 20 Apr 2020 at 07:11, Bernhard Messerklinger
><bernhard.messerklinger at br-automation.com> wrote:
>>
>> Hi Simon,
>>
>> >Hi Bernhard,
>> >
>> >On Tue, 14 Apr 2020 at 03:26, Bernhard Messerklinger
>> ><bernhard.messerklinger at br-automation.com> wrote:
>> >>
>> >> Move FSP-S configuration to the device-tree like it's already
>done
>> >for
>> >> other SoCs (Baytrail).
>> >>
>> >> Signed-off-by: Bernhard Messerklinger
>> ><bernhard.messerklinger at br-automation.com>
>> >> ---
>> >>
>> >> Changes in v2:
>> >> Remove FSP-M binding file
>> >>
>> >>  arch/x86/cpu/apollolake/fsp_s.c               | 1070
>> >+++++++++++------
>> >>  arch/x86/dts/chromebook_coral.dts             |   35 +-
>> >>  .../asm/arch-apollolake/fsp/fsp_s_upd.h       |  268 +++++
>> >>  .../fsp/fsp2/apollolake/fsp-s.txt             |  485 ++++++++
>> >>  4 files changed, 1489 insertions(+), 369 deletions(-)
>> >>  create mode 100644
>> >doc/device-tree-bindings/fsp/fsp2/apollolake/fsp-s.txt
>> >
>> >Tested on chromebook-coral:
>> >Tested-by: Simon Glass <sjg at chromium.org>
>> >
>> >>
>> >> diff --git a/arch/x86/cpu/apollolake/fsp_s.c
>> >b/arch/x86/cpu/apollolake/fsp_s.c
>> >> index 458825bc49..7d516adc92 100644
>> >> --- a/arch/x86/cpu/apollolake/fsp_s.c
>> >> +++ b/arch/x86/cpu/apollolake/fsp_s.c
>> >[..]
>> >
>> >> +
>> >> +const u8 pcie_rp_clk_req_number_def[6] = { 0x4, 0x5, 0x0, 0x1,
>> >0x2, 0x3 };
>> >> +const u8 physical_slot_number_def[6] = { 0x0, 0x1, 0x2, 0x3,
>0x4,
>> >0x5 };
>> >> +const u8 ipc_def[16] = { 0xf8, 0xef, 0xff, 0xff, 0xff, 0xff,
>0xff,
>> >0xff, 0xff,
>> >> +                       0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff
>};
>> >> +const u8 port_usb20_per_port_pe_txi_set_def[8] = { 0x07, 0x06,
>> >0x06, 0x06, 0x07,
>> >> +                                                 0x07, 0x07,
>0x01
>> >};
>> >> +const u8 port_usb20_per_port_txi_set_def[8] = { 0x00, 0x00,
>0x00,
>> >0x00, 0x00,
>> >> +                                              0x00, 0x00,
>0x03};
>> >> +const u8 port_usb20_hs_skew_sel_def[8] = { 0x00, 0x00, 0x00,
>0x00,
>> >0x00, 0x00,
>> >> +                                         0x00, 0x01 };
>> >> +const u8 port_usb20_i_usb_tx_emphasis_en_def[8] = { 0x03, 0x03,
>> >0x03, 0x03,
>> >> +                                                  0x03, 0x03,
>> >0x03, 0x01 };
>> >> +const u8 port_usb20_hs_npre_drv_sel_def[8] = { 0x00, 0x00,
>0x00,
>> >0x00, 0x00,
>> >> +                                             0x00, 0x00, 0x03
>};
>> >
>> >Do we actually need these, or does the FSP have these defaults in
>the
>> >right place anyway?
>>
>> The FSP would already have these default values included.
>> Whether we use them or not is a design decision.
>>
>> My current approach tries the following:
>>  * only non-default values should require a devicetree entry
>>  * boolean FSP parameters are implemented with boolean
>>    devicetree properties
>>
>> A limitation for boolean properties in devicetree is that they
>> are true when they are present, and false when they are not
>> present. But it is not possible to leave them out and use some
>> default value in this case.
>>
>> --> For boolean properties, this patch uses the devicetree value
>>     (either the entry is present or it is not present)
>>     unconditionally and overwrites any default values included
>>     in the FSP.
>
>I think it would be better to use int properties for the booleans.
>Perhaps we can add a new dev_read_opt_bool() to handle it.
>
>>
>> Non-boolean devicetree properties on the other hand support
>> default values. But it needs to be decided where these default
>> values should come from:
>>
>>    a) from the default values within FSP
>>    b) from default values within U-Boot
>
>OK. I had assumed that U-Boot itself wouldn't have any particular
>defaults.
>
>>
>> I have implemented option b) in this patch, as for this option
>> we don't rely on other tools to configure FSP default values
>> and IMHO it feels slightly more consistent with how boolean
>> properties are handled.
>> This is why the variables above are defined.
>>
>> But I have no strong opinion on this topic, and could implement
>> it differently depending on the feedback I receive.
>
>Neither do I, so let's keep it with the defaults in U-Boot as you
>have it.
>
>>
>> Open questions:
>>    * Should boolean properties of the FSP be implemented by
>>      boolean devicetree properties? The alternative would be
>>      to use u32 for everything.
>>      advantage: support for default values
>>      drawback:  devicetree gets bigger
>
>Yes we should support u32. It only increases the size by 4 bytes for
>each one.
>
>>
>>    * For non-boolean properties: Should the default value from
>>      FSP be used, or a default value defined in U-Boot?
>
>Let's go with what you have then as I don't have any info to suggest
>we should change it.

Thanks for your fast feedback.

The main reason why I added default values within U-Boot is
to avoid inconsistency between boolean and u32 properties.

I agree that we should use u32 for all parameters, but in this
case we can drop the default values in U-Boot and directly use
the default values in the FSP.

This would mean:
1.) All devicetree bool parameters are changed to u32.
2.) If a parameter is not specified in the devicetree, the
    default value configured in the FSP is used.

Advantages:
1.) U-boot can boot without any devicetree FPS parameters
if the FSP was already configured with the Intel bct tool.

2.) There is no need to maintain default FSP values in U-Boot.
New FSP releases can be used without checking for changes in
their default settings.

What do you think about this?

>> Feedback on these questions would be appreciated.
>>
>
>[..]
>
>Regards,
>Simon
>
Regards,
Bernhard