Re: Re: [RFC PATCH 2/2] arch: x86: apl: Use devicetree for FSP configuration
Hi Bernhard, On Thu, 7 May 2020 at 01:53, Bernhard Messerklinger wrote: > > Hi Simon, > > >Hi Bernhard, > > > >On Thu, 30 Apr 2020 at 03:16, Bernhard Messerklinger > > 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 > > > >> Signed-off-by: Wolfgang Wallner > > > >> Signed-off-by: Bernhard Messerklinger > > > >> > >> --- > >> > >> 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 > > > >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? > Yes that's true. But at the moment its just copy paste. > I generated the binding table from the fsp_s and fsp_m config struct > with a python script. But this script is also far from being finished. OK thanks. > > ... > >> +#if defined(CONFIG_SPL_BUILD) > > > >Do you need these #ifs? I would hope the compiler would only include > >them if needed. > Without the #ifs the SPL size stays the same but the u-boot proper size > increases by about 2 kb. OK, well then we need the #ifs. BTW next time you send this you could take off the RFC as I think we should apply this. Regards, Simon
Antwort: Re: [RFC PATCH 2/2] arch: x86: apl: Use devicetree for FSP configuration
Hi Simon, >Hi Bernhard, > >On Thu, 30 Apr 2020 at 03:16, Bernhard Messerklinger > 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 > >> Signed-off-by: Wolfgang Wallner > >> Signed-off-by: Bernhard Messerklinger > >> >> --- >> >> 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 > >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? Yes that's true. But at the moment its just copy paste. I generated the binding table from the fsp_s and fsp_m config struct with a python script. But this script is also far from being finished. > ... >> +#if defined(CONFIG_SPL_BUILD) > >Do you need these #ifs? I would hope the compiler would only include >them if needed. Without the #ifs the SPL size stays the same but the u-boot proper size increases by about 2 kb. > ... >> +void fsp_s_update_config_from_dtb(ofnode node, struct fsp_s_config >*cfg); > >function comments > >Regards, >Simon Regards, Bernhard
Re: [RFC PATCH 2/2] arch: x86: apl: Use devicetree for FSP configuration
Hi Bernhard,
On Thu, 30 Apr 2020 at 03:16, Bernhard Messerklinger
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
> Signed-off-by: Wolfgang Wallner
> Signed-off-by: Bernhard Messerklinger
>
>
> ---
>
> 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
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 00..9c10e7328a
> --- /dev/null
> +++ b/arch/x86/cpu/apollolake/fsp_bindings.c
> @@ -0,0 +1,2096 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2020 B Industrial Automation GmbH -
> http://www.br-automation.com
> + */
> +
> +#include
> +#include
> +#include
> +
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, );
> + 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, );
> + 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
