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

[Simon Glass]

Hi Bernhard,

On Tue, 21 Apr 2020 at 01:57, Bernhard Messerklinger
<bernhard.messerklinger at br-automation.com> wrote:
>
> 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?

It sounds good to me.

Bin, do you have any thoughts on this?

Regards,
Simon