Antwort: Re: [RFC PATCH v2 2/2] arch: x86: apl: Read FSP-S configuration from device-tree
Bernhard Messerklinger
bernhard.messerklinger at br-automation.com
Mon Apr 20 15:11:31 CEST 2020
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.
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
More information about the U-Boot
mailing list