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

[Simon Glass]

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.

>
> Feedback on these questions would be appreciated.
>

[..]

Regards,
Simon