Hi Bernhard,
On Thu, 30 Apr 2020 at 03:16, Bernhard Messerklinger bernhard.messerklinger@br-automation.com wrote:
A the moment the FSP configuration is a mix of hard coded values and devicetree properties. This patch makes FSP-M and FSP-S full configurable from devicetree by adding binding properties for all FSP parameters.
Co-developed-by: Wolfgang Wallner wolfgang.wallner@br-automation.com Signed-off-by: Wolfgang Wallner wolfgang.wallner@br-automation.com Signed-off-by: Bernhard Messerklinger bernhard.messerklinger@br-automation.com
arch/x86/cpu/apollolake/Makefile | 1 + arch/x86/cpu/apollolake/fsp_bindings.c | 2096 +++++++++++++++++ arch/x86/cpu/apollolake/fsp_m.c | 164 +- arch/x86/cpu/apollolake/fsp_s.c | 382 +-- arch/x86/dts/chromebook_coral.dts | 72 +- .../asm/arch-apollolake/fsp/fsp_m_upd.h | 168 ++ .../asm/arch-apollolake/fsp/fsp_s_upd.h | 202 ++ .../asm/arch-apollolake/fsp_bindings.h | 74 + .../fsp/fsp2/apollolake/fsp-m.txt | 320 +++ .../fsp/fsp2/apollolake/fsp-s.txt | 483 ++++ 10 files changed, 3422 insertions(+), 540 deletions(-) create mode 100644 arch/x86/cpu/apollolake/fsp_bindings.c create mode 100644 arch/x86/include/asm/arch-apollolake/fsp_bindings.h create mode 100644 doc/device-tree-bindings/fsp/fsp2/apollolake/fsp-m.txt create mode 100644 doc/device-tree-bindings/fsp/fsp2/apollolake/fsp-s.txt
Tested on coral: Tested-by: Simon Glass sjg@chromium.org
This looks good to me. I wonder if one day the binding table could be created from the binding .txt file, or compared with it programmatically?
diff --git a/arch/x86/cpu/apollolake/Makefile b/arch/x86/cpu/apollolake/Makefile index 578e15c4bf..3aa2a55676 100644 --- a/arch/x86/cpu/apollolake/Makefile +++ b/arch/x86/cpu/apollolake/Makefile @@ -10,6 +10,7 @@ obj-y += cpu_common.o ifndef CONFIG_TPL_BUILD obj-y += cpu.o obj-y += punit.o +obj-y += fsp_bindings.o ifdef CONFIG_SPL_BUILD obj-y += fsp_m.o endif diff --git a/arch/x86/cpu/apollolake/fsp_bindings.c b/arch/x86/cpu/apollolake/fsp_bindings.c new file mode 100644 index 0000000000..9c10e7328a --- /dev/null +++ b/arch/x86/cpu/apollolake/fsp_bindings.c @@ -0,0 +1,2096 @@ +// SPDX-License-Identifier: GPL-2.0 +/*
- Copyright 2020 B&R Industrial Automation GmbH - http://www.br-automation.com
- */
+#include <common.h> +#include <dm.h> +#include <asm/arch/fsp_bindings.h>
Please add comments to these functions
+static void read_u8_prop(ofnode node, u8 *dst, char *name, size_t count) +{
u32 tmp;const u8 *buf;int ret;if (count == 0) {ret = ofnode_read_u32(node, name, &tmp);if (ret == 0)*dst = tmp;} else {buf = ofnode_read_u8_array_ptr(node, name, count);if (buf)memcpy(dst, buf, count);}+}
+static void read_u16_prop(ofnode node, u16 *dst, char *name, size_t count) +{
u32 tmp;u32 buf[32];int ret;if (ARRAY_SIZE(buf) < count) {printf("ERROR: %s buffer to small!\n", __func__);
too Can this be debug?
return;
Needs a return value to report the error, perhaps -ENOSPC?
}if (count == 0) {ret = ofnode_read_u32(node, name, &tmp);if (ret == 0)*dst = tmp;} else {ret = ofnode_read_u32_array(node, name, buf, count);if (ret == 0)for (int i = 0; i < count; i++)dst[i] = buf[i];}+}
+static void read_u32_prop(ofnode node, u32 *dst, char *name, size_t count) +{
if (count == 0)ofnode_read_u32(node, name, dst);elseofnode_read_u32_array(node, name, dst, count);+}
+static void read_string_prop(ofnode node, char *dst, char *name, int count) +{
const char *string_buf;if (count > 0) {string_buf = ofnode_read_string(node, name);if (string_buf) {strncpy(dst, string_buf, count);
strlcpy does these two lines
dst[count - 1] = '\0';}}+}
+static void read_swizzle_prop(ofnode node, u8 *dst, char *name, int count) +{
const struct lpddr4_chan_swizzle_cfg *sch;/* Number of bytes to copy per DQS */const size_t sz = DQ_BITS_PER_DQS;const struct lpddr4_swizzle_cfg *swizzle_cfg;swizzle_cfg = (const struct lpddr4_swizzle_cfg *)ofnode_read_u8_array_ptr(node, name, count);if (!swizzle_cfg)return;/** CH0_DQB byte lanes in the bit swizzle configuration field are* not 1:1. The mapping within the swizzling field is:* indices [0:7] - byte lane 1 (DQS1) DQ[8:15]* indices [8:15] - byte lane 0 (DQS0) DQ[0:7]* indices [16:23] - byte lane 3 (DQS3) DQ[24:31]* indices [24:31] - byte lane 2 (DQS2) DQ[16:23]*/sch = &swizzle_cfg->phys[LP4_PHYS_CH0B];memcpy(&dst[0 * DQ_BITS_PER_DQS], &sch->dqs[LP4_DQS1], sz);memcpy(&dst[1 * DQ_BITS_PER_DQS], &sch->dqs[LP4_DQS0], sz);memcpy(&dst[2 * DQ_BITS_PER_DQS], &sch->dqs[LP4_DQS3], sz);memcpy(&dst[3 * DQ_BITS_PER_DQS], &sch->dqs[LP4_DQS2], sz);/** CH0_DQA byte lanes in the bit swizzle configuration field are 1:1.*/sch = &swizzle_cfg->phys[LP4_PHYS_CH0A];memcpy(&dst[4 * DQ_BITS_PER_DQS], &sch->dqs[LP4_DQS0], sz);memcpy(&dst[5 * DQ_BITS_PER_DQS], &sch->dqs[LP4_DQS1], sz);memcpy(&dst[6 * DQ_BITS_PER_DQS], &sch->dqs[LP4_DQS2], sz);memcpy(&dst[7 * DQ_BITS_PER_DQS], &sch->dqs[LP4_DQS3], sz);sch = &swizzle_cfg->phys[LP4_PHYS_CH1B];memcpy(&dst[8 * DQ_BITS_PER_DQS], &sch->dqs[LP4_DQS1], sz);memcpy(&dst[9 * DQ_BITS_PER_DQS], &sch->dqs[LP4_DQS0], sz);memcpy(&dst[10 * DQ_BITS_PER_DQS], &sch->dqs[LP4_DQS3], sz);memcpy(&dst[11 * DQ_BITS_PER_DQS], &sch->dqs[LP4_DQS2], sz);/** CH0_DQA byte lanes in the bit swizzle configuration field are 1:1.*/sch = &swizzle_cfg->phys[LP4_PHYS_CH1A];memcpy(&dst[12 * DQ_BITS_PER_DQS], &sch->dqs[LP4_DQS0], sz);memcpy(&dst[13 * DQ_BITS_PER_DQS], &sch->dqs[LP4_DQS1], sz);memcpy(&dst[14 * DQ_BITS_PER_DQS], &sch->dqs[LP4_DQS2], sz);memcpy(&dst[15 * DQ_BITS_PER_DQS], &sch->dqs[LP4_DQS3], sz);+}
+static void fsp_update_config_from_dtb(ofnode node, u8 *cfg,
const struct fsp_binding *fsp_bindings)+{
Needs a return value as it can fail with read_u16_prop
How about having a local var here, e.g.
for (int i = 0; fsp_bindings[i].propname; i++) {
struct fsp_binding *fspb = fsp_bindings;
so you can use fspb instead of fsp_bindings[i] everywhere in this function
switch (fsp_bindings[i].type) {case FSP_UINT8:read_u8_prop(node, &cfg[fsp_bindings[i].offset],fsp_bindings[i].propname,fsp_bindings[i].count);break;case FSP_UINT16:read_u16_prop(node,(u16 *)&cfg[fsp_bindings[i].offset],fsp_bindings[i].propname,fsp_bindings[i].count);break;case FSP_UINT32:read_u32_prop(node,(u32 *)&cfg[fsp_bindings[i].offset],fsp_bindings[i].propname,fsp_bindings[i].count);break;case FSP_STRING:read_string_prop(node, (char *)&cfg[fsp_bindings[i].offset],fsp_bindings[i].propname,fsp_bindings[i].count);break;case FSP_LPDDR4_SWIZZLE:read_swizzle_prop(node,&cfg[fsp_bindings[i].offset],fsp_bindings[i].propname,fsp_bindings[i].count);break;}}+}
+#if defined(CONFIG_SPL_BUILD)
Do you need these #ifs? I would hope the compiler would only include them if needed.
+const struct fsp_binding fsp_m_bindings[] = {
{.type = FSP_UINT32,.offset = offsetof(struct fsp_m_config, serial_debug_port_address),.propname = "fspm,serial-debug-port-address",},{
Put } on previous line, so }, {
.type = FSP_UINT8,.offset = offsetof(struct fsp_m_config, serial_debug_port_type),.propname = "fspm,serial-debug-port-type",},{.type = FSP_UINT8,
[...]
diff --git a/arch/x86/cpu/apollolake/fsp_m.c b/arch/x86/cpu/apollolake/fsp_m.c index 5308af8ed4..2e63062102 100644 --- a/arch/x86/cpu/apollolake/fsp_m.c +++ b/arch/x86/cpu/apollolake/fsp_m.c @@ -9,180 +9,28 @@ #include <asm/arch/iomap.h> #include <asm/arch/fsp/fsp_configs.h> #include <asm/arch/fsp/fsp_m_upd.h>
Looks like those two headers can dropped?
+#include <asm/arch/fsp_bindings.h> #include <asm/fsp2/fsp_internal.h> #include <dm/uclass-internal.h>
[..]
Drop extra blank line
int fspm_update_config(struct udevice *dev, struct fspm_upd *upd) { struct fsp_m_config *cfg = &upd->config; struct fspm_arch_upd *arch = &upd->arch;
ofnode node; arch->nvs_buffer_ptr = NULL; prepare_mrc_cache(upd); arch->stack_base = (void *)0xfef96000; arch->boot_loader_tolum_size = 0;
[..]
diff --git a/arch/x86/include/asm/arch-apollolake/fsp_bindings.h b/arch/x86/include/asm/arch-apollolake/fsp_bindings.h new file mode 100644 index 0000000000..c4c0028c06 --- /dev/null +++ b/arch/x86/include/asm/arch-apollolake/fsp_bindings.h @@ -0,0 +1,74 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*
- Copyright 2019 Google LLC
- Copyright 2020 B&R Industrial Automation GmbH - http://www.br-automation.com
- */
+#ifndef __ASM_ARCH_FSP_BINDINGS_H +#define __ASM_ARCH_FSP_BINDINGS_H
+#include <asm/arch/fsp/fsp_m_upd.h> +#include <asm/arch/fsp/fsp_s_upd.h>
+#define ARRAY_SIZE_OF_MEMBER(s, m) (ARRAY_SIZE((((s *)0)->m)))
+enum conf_type {
FSP_UINT8,FSP_UINT16,FSP_UINT32,FSP_STRING,FSP_LPDDR4_SWIZZLE,+};
struct comment
+struct fsp_binding {
size_t offset;char *propname;enum conf_type type;size_t count;+};
+/*
- LPDDR4 helper routines for configuring the memory UPD for LPDDR4 operation.
- There are four physical LPDDR4 channels, each 32-bits wide. There are two
- logical channels using two physical channels together to form a 64-bit
- interface to memory for each logical channel.
- */
+enum {
LP4_PHYS_CH0A,LP4_PHYS_CH0B,LP4_PHYS_CH1A,LP4_PHYS_CH1B,LP4_NUM_PHYS_CHANNELS,+};
+/*
- The DQs within a physical channel can be bit-swizzled within each byte.
- Within a channel the bytes can be swapped, but the DQs need to be routed
- with the corresponding DQS (strobe).
- */
+enum {
LP4_DQS0,LP4_DQS1,LP4_DQS2,LP4_DQS3,LP4_NUM_BYTE_LANES,DQ_BITS_PER_DQS = 8,+};
+/* Provide bit swizzling per DQS and byte swapping within a channel */ +struct lpddr4_chan_swizzle_cfg {
u8 dqs[LP4_NUM_BYTE_LANES][DQ_BITS_PER_DQS];+};
+struct lpddr4_swizzle_cfg {
struct lpddr4_chan_swizzle_cfg phys[LP4_NUM_PHYS_CHANNELS];+};
+void fsp_m_update_config_from_dtb(ofnode node, struct fsp_m_config *cfg);
+void fsp_s_update_config_from_dtb(ofnode node, struct fsp_s_config *cfg);
function comments
Regards, Simon