Hi Bernhard, On Tue, 21 Apr 2020 at 01:57, Bernhard Messerklinger <bernhard.messerklin...@br-automation.com> wrote: > > Hi Simon, > > > >Hi Bernhard, > > > >On Mon, 20 Apr 2020 at 07:11, Bernhard Messerklinger > ><bernhard.messerklin...@br-automation.com> wrote: > >> > >> Hi Simon, > >> > >> >Hi Bernhard, > >> > > >> >On Tue, 14 Apr 2020 at 03:26, Bernhard Messerklinger > >> ><bernhard.messerklin...@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.messerklin...@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 <s...@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