[RFC PATCH 2/2] arch: x86: apl: Use devicetree for FSP configuration
Simon Glass
sjg at chromium.org
Mon May 4 14:53:43 CEST 2020
Hi Bernhard,
On Thu, 30 Apr 2020 at 03:16, Bernhard Messerklinger
<bernhard.messerklinger at 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 at br-automation.com>
> Signed-off-by: Wolfgang Wallner <wolfgang.wallner at br-automation.com>
> Signed-off-by: Bernhard Messerklinger <bernhard.messerklinger at 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 at 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);
> + 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
More information about the U-Boot
mailing list