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

20 Apr 2020

      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?
...

+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