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