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.
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
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.
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
* For non-boolean properties: Should the default value from FSP be used, or a default value defined in U-Boot?
Feedback on these questions would be appreciated.
+static int read_u8_array(u8 prop[], ofnode node, const char
*propname, int sz,
u8 def)Please add function comment
{ const u8 *ptr;
[..]
ptr = ofnode_read_u8_array_ptr(node, propname, sz);if (ptr) {memcpy(prop, ptr, sz);} else {memset(prop, def, sz);return -EINVAL;} return 0;}
-static void apl_fsp_silicon_init_params_cb(struct apl_config *apl,
struct fsp_s_config*cfg)
+static int read_u16_array(u16 prop[], ofnode node, const char
*propname, int sz,
u16 def){
same here
u8 port;for (port = 0; port < MAX_USB2_PORTS; port++) {if (apl->usb2eye[port].per_port_tx_pe_half)cfg->port_usb20_per_port_tx_pe_half[port] =apl->usb2eye[port].per_port_tx_pe_half;
if (apl->usb2eye[port].per_port_pe_txi_set)cfg->port_usb20_per_port_pe_txi_set[port] =apl->usb2eye[port].per_port_pe_txi_set;
if (apl->usb2eye[port].per_port_txi_set)cfg->port_usb20_per_port_txi_set[port] =apl->usb2eye[port].per_port_txi_set;
u32 *array_buf;int ret;
if (apl->usb2eye[port].hs_skew_sel)cfg->port_usb20_hs_skew_sel[port] =apl->usb2eye[port].hs_skew_sel;
array_buf = malloc(sz * sizeof(u32));Is it possible to use a dynamic array, like
u32 array_buf[siz];
since these arrays are small and free() does not actually do anything in SPL by default.
if (!array_buf)return -ENOMEM;ret = ofnode_read_u32_array(node, propname, array_buf, sz);if (!ret) {for (int i = 0; i < sz; i++)prop[i] = array_buf[i];} else if (ret == -EINVAL) {for (int i = 0; i < sz; i++)prop[i] = def;ret = 0;}free(array_buf);
if (apl->usb2eye[port].usb_tx_emphasis_en)cfg->port_usb20_i_usb_tx_emphasis_en[port]=
apl->usb2eye[port].usb_tx_emphasis_en;
return ret;+}
if (apl->usb2eye[port].per_port_rxi_set)cfg->port_usb20_per_port_rxi_set[port] =apl->usb2eye[port].per_port_rxi_set;
+static int read_u32_array(u32 prop[], ofnode node, const char
*propname, int sz,
u32 def)+{
int ret;
if (apl->usb2eye[port].hs_npre_drv_sel)cfg->port_usb20_hs_npre_drv_sel[port] =apl->usb2eye[port].hs_npre_drv_sel;
ret = ofnode_read_u32_array(node, propname, prop, sz);if (ret == -EINVAL) {for (int i = 0; i < sz; i++)prop[i] = def;ret = 0; }return ret;}
[...]
+#ifdef CONFIG_HAVE_VBT
if (IS_ENABLED(CONFIG_HAVE_VBT)
cfg->graphics_config_ptr = (ulong)vbt_buf;+#else
cfg->graphics_config_ptr = 0;+#endif
[..]
for (int i = 0; i < FSP_I2C_COUNT; i++) {snprintf(buf, sizeof(buf), "fsps,enable-i2c%d", i);*(&cfg->i2c0_enable + i) =ofnode_read_u32_default(node, buf,
I2CX_ENABLE_PCI_MODE);
cfg->i2c0_enable[i]
same below
}for (int i = 0; i < FSP_HSUART_COUNT; i++) {snprintf(buf, sizeof(buf), "fsps,enable-hsuart%d",i);
*(&cfg->hsuart0_enable + i) =ofnode_read_u32_default(node, buf,
HSUARTX_ENABLE_PCI_MODE);
}for (int i = 0; i < FSP_SPI_COUNT; i++) {snprintf(buf, sizeof(buf), "fsps,enable-spi%d", i);*(&cfg->spi0_enable + i) =ofnode_read_u32_default(node, buf,
SPIX_ENABLE_PCI_MODE);
}cfg->os_dbg_enable[..]
Regards, Simon
Regards, Bernhard