Hi Bernhard, On Thu, 30 Apr 2020 at 03:16, Bernhard Messerklinger <[email protected]> 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 <[email protected]> > Signed-off-by: Wolfgang Wallner <[email protected]> > Signed-off-by: Bernhard Messerklinger > <[email protected]> > > --- > > 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 <[email protected]> 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); > + else > + ofnode_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
