[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