Re: [PATCH v2 2/3] board: phytec: imx93: Add eeprom-based hardware introspection

[Yannic Moog <Y.Moog@phytec.de>]

On Tue, 2024-11-19 at 17:45 +0100, Christoph Stoidner wrote:
> 
> 
> On Do, 2024-11-14 at 07:30 +0100, Yannic Moog wrote:
> > Hi Christoph,
> > 
> > On Wed, 2024-11-13 at 17:00 +0100, Christoph Stoidner wrote:
> > > The phyCORE-i.MX 93 is available in various variants. Relevant
> > > variant
> > > options for the spl/u-boot are:
> > > - with or without HS400 support for the eMMC
> > > - with 1GB ram chip, or 2GB ram chip
> > > 
> > > The phyCORE's eeprom contains all information about the existing
> > > variant
> > > options. Add evaluation of the eeprom data to the spl/u-boot to
> > > enable/disable HS400 and to select the appropriate ram
> > > configuration at
> > > startup.
> > > 
> > > Signed-off-by: Christoph Stoidner <c.stoidner@phytec.de>
> > > ---
> > > Cc: Mathieu Othacehe <m.othacehe@gmail.com>
> > > Cc: Christoph Stoidner <c.stoidner@phytec.de>
> > > Cc: Stefano Babic <sbabic@denx.de>
> > > Cc: Fabio Estevam <festevam@gmail.com>
> > > Cc: "NXP i.MX U-Boot Team" <uboot-imx@nxp.com>
> > > Cc: Tom Rini <trini@konsulko.com>
> > > Cc: Yannic Moog <y.moog@phytec.de>
> > > Cc: Primoz Fiser <primoz.fiser@norik.com>
> > > Cc: Andrej Picej <andrej.picej@norik.com>
> > > Cc: Wadim Egorov <w.egorov@phytec.de>
> > > ---
> > > Changes in v2:
> > > - encapsulate handling of feature flag VOLTAGE into own function
> > > - move definition of enum phytec_imx93_ddr_eeprom_code into header
> > > file
> > > 
> > >  arch/arm/dts/imx93-phyboard-segin-u-boot.dtsi |  19 +++
> > >  arch/arm/mach-imx/imx9/Kconfig                |   2 +
> > >  arch/arm/mach-imx/imx9/soc.c                  |   2 +-
> > >  board/phytec/common/Kconfig                   |   8 ++
> > >  board/phytec/common/Makefile                  |   1 +
> > >  board/phytec/common/imx93_som_detection.c     | 111
> > > ++++++++++++++++++
> > >  board/phytec/common/imx93_som_detection.h     |  51 ++++++++
> > >  board/phytec/phycore_imx93/Kconfig            |  28 +++++
> > >  board/phytec/phycore_imx93/MAINTAINERS        |   5 +-
> > >  board/phytec/phycore_imx93/phycore-imx93.c    |  51 ++++++++
> > >  board/phytec/phycore_imx93/spl.c              |  48 ++++++++
> > >  11 files changed, 324 insertions(+), 2 deletions(-)
> > >  create mode 100644 board/phytec/common/imx93_som_detection.c
> > >  create mode 100644 board/phytec/common/imx93_som_detection.h
> > > 
> > > diff --git a/arch/arm/dts/imx93-phyboard-segin-u-boot.dtsi
> > > b/arch/arm/dts/imx93-phyboard-segin-u-
> > > boot.dtsi
> > > index 6897c91f4d..25c778bb07 100644
> > > --- a/arch/arm/dts/imx93-phyboard-segin-u-boot.dtsi
> > > +++ b/arch/arm/dts/imx93-phyboard-segin-u-boot.dtsi
> > > @@ -305,4 +305,23 @@
> > >                         };
> > >                 };
> > >         };
> > > +
> > > +       eeprom@50 {
> > > +               bootph-pre-ram;
> > > +               bootph-some-ram;
> > > +               compatible = "atmel,24c32";
> > > +               reg = <0x50>;
> > > +               pagesize = <32>;
> > > +               vcc-supply = <&buck4>;
> > > +       };
> > > +
> > > +       eepromid@58 {
> > > +               bootph-pre-ram;
> > > +               bootph-some-ram;
> > > +               compatible = "atmel,24c32";
> > > +               pagesize = <32>;
> > > +               reg = <0x58>;
> > > +               size = <32>;
> > > +               vcc-supply = <&buck4>;
> > > +       };
> > >  };
> > > diff --git a/arch/arm/mach-imx/imx9/Kconfig b/arch/arm/mach-
> > > imx/imx9/Kconfig
> > > index 5c1054138f..2465e31d73 100644
> > > --- a/arch/arm/mach-imx/imx9/Kconfig
> > > +++ b/arch/arm/mach-imx/imx9/Kconfig
> > > @@ -45,6 +45,8 @@ config TARGET_PHYCORE_IMX93
> > >         bool "phycore_imx93"
> > >         select IMX93
> > >         select IMX9_LPDDR4X
> > > +       select OF_BOARD_FIXUP
> > > +       select OF_BOARD_SETUP
> > >  
> > >  endchoice
> > >  
> > > diff --git a/arch/arm/mach-imx/imx9/soc.c b/arch/arm/mach-
> > > imx/imx9/soc.c
> > > index 7c28fa39e1..237354f507 100644
> > > --- a/arch/arm/mach-imx/imx9/soc.c
> > > +++ b/arch/arm/mach-imx/imx9/soc.c
> > > @@ -628,7 +628,7 @@ static int low_drive_freq_update(void *blob)
> > >         return 0;
> > >  }
> > >  
> > > -#ifdef CONFIG_OF_BOARD_FIXUP
> > > +#if defined(CONFIG_OF_BOARD_FIXUP) &&
> > > !defined(CONFIG_TARGET_PHYCORE_IMX93)
> > 
> > Why do you do this?
> > 
> > It doesn't look right to me. When you have to modify soc.c to add
> > your own board config, then either
> > there is a problem with the file and it should be fixed properly here
> > or there is a problem with
> > your board and it should be fixed in your board code.
> 
> The function board_fix_fdt() is an u-boot callback, that can be
> implemented by the board-code. It enables the board-code to modify
> the Device Tree. The modified device tree is then subsequently used
> by U-Boot.
> 
> We need that in our board-code to enable or disable HS400 for the
> eMMC, depending of what module variant we detected from the
> information in the eeprom. 
> 
> Unfortunately, NXP has already implemented that function in their
> SOC-code, so it is no more available for the board-code. From my
> opinion, the best solution would be to move board_fix_fdt()
> from soc.c to all imx9 board codes that need it. Then that
> function is again available for the board code.

Understood, thanks.

> 
> My proposal: To avoid loosig the focus of that patchset we accept
> it now as it is here. After that, I will then take care to send
> another patchset to move board_fix_fdt() for all boards accordingly.
> 
> Do you agree with that? 

Yes, sounds good.

Yannic
> 
> > 
> > >  #ifndef CONFIG_XPL_BUILD
> > >  int board_fix_fdt(void *fdt)
> > >  {
> > > diff --git a/board/phytec/common/Kconfig
> > > b/board/phytec/common/Kconfig
> > > index f394ace786..bc5511707a 100644
> > > --- a/board/phytec/common/Kconfig
> > > +++ b/board/phytec/common/Kconfig
> > > @@ -19,6 +19,14 @@ config PHYTEC_IMX8M_SOM_DETECTION
> > >           Support of I2C EEPROM based SoM detection. Supported
> > >           for PHYTEC i.MX8MM/i.MX8MP boards
> > >  
> > > +config PHYTEC_IMX93_SOM_DETECTION
> > > +       bool "Support SoM detection for i.MX93 PHYTEC platforms"
> > > +       depends on ARCH_IMX9 && PHYTEC_SOM_DETECTION
> > > +       default y
> > > +       help
> > > +         Support of I2C EEPROM based SoM detection. Supported
> > > +         for PHYTEC i.MX93 based boards
> > > +
> > >  config PHYTEC_AM62_SOM_DETECTION
> > >         bool "Support SoM detection for AM62x PHYTEC platforms"
> > >         depends on (TARGET_PHYCORE_AM62X_A53 ||
> > > TARGET_PHYCORE_AM62X_R5) && \
> > > diff --git a/board/phytec/common/Makefile
> > > b/board/phytec/common/Makefile
> > > index cd78f7686f..8126f7356e 100644
> > > --- a/board/phytec/common/Makefile
> > > +++ b/board/phytec/common/Makefile
> > > @@ -10,3 +10,4 @@ endif
> > >  obj-y += phytec_som_detection.o phytec_som_detection_blocks.o
> > >  obj-$(CONFIG_ARCH_K3) += am6_som_detection.o k3/
> > >  obj-$(CONFIG_ARCH_IMX8M) += imx8m_som_detection.o
> > > +obj-$(CONFIG_ARCH_IMX9) += imx93_som_detection.o
> > > diff --git a/board/phytec/common/imx93_som_detection.c
> > > b/board/phytec/common/imx93_som_detection.c
> > > new file mode 100644
> > > index 0000000000..a28cdf6b51
> > > --- /dev/null
> > > +++ b/board/phytec/common/imx93_som_detection.c
> > > @@ -0,0 +1,111 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/*
> > > + * Copyright (C) 2024 PHYTEC Messtechnik GmbH
> > > + * Author: Primoz Fiser <primoz.fiser@norik.com>
> > > + */
> > > +
> > > +#include <asm/arch/sys_proto.h>
> > > +#include <dm/device.h>
> > > +#include <dm/uclass.h>
> > > +#include <i2c.h>
> > > +#include <u-boot/crc.h>
> > > +
> > > +#include "imx93_som_detection.h"
> > > +
> > > +extern struct phytec_eeprom_data eeprom_data;
> > > +
> > > +#if IS_ENABLED(CONFIG_PHYTEC_IMX93_SOM_DETECTION)
> > > +
> > > +/* Check if the SoM is actually one of the following products:
> > > + * - i.MX93
> > > + *
> > > + * Returns 0 in case it's a known SoM. Otherwise, returns 1.
> > > + */
> > > +u8 __maybe_unused phytec_imx93_detect(struct phytec_eeprom_data
> > > *data)
> > > +{
> > > +       u8 som;
> > > +
> > > +       if (!data)
> > > +               data = &eeprom_data;
> > > +
> > > +       /* Early API revisions are not supported */
> > > +       if (!data->valid || data->payload.api_rev <
> > > PHYTEC_API_REV2)
> > > +               return 1;
> > > +
> > > +       som = data->payload.data.data_api2.som_no;
> > > +       debug("%s: som id: %u\n", __func__, som);
> > > +
> > > +       if (som == PHYTEC_IMX93_SOM && is_imx93())
> > > +               return 0;
> > > +
> > > +       pr_err("%s: SoM ID does not match. Wrong EEPROM data?\n",
> > > __func__);
> > > +       return 1;
> > > +}
> > > +
> > > +/*
> > > + * Filter PHYTEC i.MX93 SoM options by option index
> > > + *
> > > + * Returns:
> > > + *  - option value
> > > + *  - PHYTEC_EEPROM_INVAL when the data is invalid
> > > + *
> > > + */
> > > +u8 __maybe_unused phytec_imx93_get_opt(struct phytec_eeprom_data
> > > *data,
> > > +                                      enum
> > > phytec_imx93_option_index idx)
> > > +{
> > > +       char *opt;
> > > +       u8 opt_id;
> > > +
> > > +       if (!data)
> > > +               data = &eeprom_data;
> > > +
> > > +       if (!data->valid || data->payload.api_rev <
> > > PHYTEC_API_REV2)
> > > +               return PHYTEC_EEPROM_INVAL;
> > > +
> > > +       opt = phytec_get_opt(data);
> > > +       if (opt)
> > > +               opt_id = PHYTEC_GET_OPTION(opt[idx]);
> > > +       else
> > > +               opt_id = PHYTEC_EEPROM_INVAL;
> > > +
> > > +       debug("%s: opt[%d] id: %u\n", __func__, idx, opt_id);
> > > +       return opt_id;
> > > +}
> > > +
> > > +/*
> > > + * Filter PHYTEC i.MX93 SoM voltage
> > > + *
> > > + * Returns:
> > > + *  - PHYTEC_IMX93_VOLTAGE_1V8 or PHYTEC_IMX93_VOLTAGE_3V8
> > > + *  - PHYTEC_EEPROM_INVAL when the data is invalid
> > > + *
> > > + */
> > > +enum phytec_imx93_voltage __maybe_unused
> > > phytec_imx93_get_voltage(struct phytec_eeprom_data
> > > *data)
> > > +{
> > > +       u8 option = phytec_imx93_get_opt(data,
> > > PHYTEC_IMX93_OPT_FEAT);
> > > +
> > > +       if (option == PHYTEC_EEPROM_INVAL)
> > > +               return PHYTEC_IMX93_VOLTAGE_INVALID;
> > > +       return (option & 0x01) ? PHYTEC_IMX93_VOLTAGE_1V8 :
> > > PHYTEC_IMX93_VOLTAGE_3V3;
> > > +}
> > > +
> > > +#else
> > > +
> > > +inline u8 __maybe_unused phytec_imx93_detect(struct
> > > phytec_eeprom_data *data)
> > > +{
> > > +       return 1;
> > > +}
> > > +
> > > +inline u8 __maybe_unused phytec_imx93_get_opt(struct
> > > phytec_eeprom_data *data,
> > > +                                             enum
> > > phytec_imx93_option_index idx)
> > > +{
> > > +       return PHYTEC_EEPROM_INVAL;
> > > +}
> > > +
> > > +inline enum phytec_imx93_voltage __maybe_unused
> > > phytec_imx93_get_voltage
> > > +       (struct phytec_eeprom_data *data)
> > > +{
> > > +       return PHYTEC_EEPROM_INVAL;
> > > +}
> > > +
> > > +#endif /* IS_ENABLED(CONFIG_PHYTEC_IMX93_SOM_DETECTION) */
> > > diff --git a/board/phytec/common/imx93_som_detection.h
> > > b/board/phytec/common/imx93_som_detection.h
> > > new file mode 100644
> > > index 0000000000..6d3d9283d8
> > > --- /dev/null
> > > +++ b/board/phytec/common/imx93_som_detection.h
> > > @@ -0,0 +1,51 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > +/*
> > > + * Copyright (C) 2024 PHYTEC Messtechnik GmbH
> > > + * Author: Primoz Fiser <primoz.fiser@norik.com>
> > > + */
> > > +
> > > +#ifndef _PHYTEC_IMX93_SOM_DETECTION_H
> > > +#define _PHYTEC_IMX93_SOM_DETECTION_H
> > > +
> > > +#include "phytec_som_detection.h"
> > > +
> > > +#define PHYTEC_IMX93_SOM       77
> > > +
> > > +enum phytec_imx93_option_index {
> > > +       PHYTEC_IMX93_OPT_DDR = 0,
> > > +       PHYTEC_IMX93_OPT_EMMC,
> > > +       PHYTEC_IMX93_OPT_CPU,
> > > +       PHYTEC_IMX93_OPT_FREQ,
> > > +       PHYTEC_IMX93_OPT_NPU,
> > > +       PHYTEC_IMX93_OPT_DISP,
> > > +       PHYTEC_IMX93_OPT_ETH,
> > > +       PHYTEC_IMX93_OPT_FEAT,
> > > +       PHYTEC_IMX93_OPT_TEMP,
> > > +       PHYTEC_IMX93_OPT_BOOT,
> > > +       PHYTEC_IMX93_OPT_LED,
> > > +       PHYTEC_IMX93_OPT_EEPROM,
> > > +};
> > > +
> > > +enum phytec_imx93_voltage {
> > > +       PHYTEC_IMX93_VOLTAGE_INVALID = PHYTEC_EEPROM_INVAL,
> > > +       PHYTEC_IMX93_VOLTAGE_3V3 = 0,
> > > +       PHYTEC_IMX93_VOLTAGE_1V8,
> > > +};
> > > +
> > > +enum phytec_imx93_ddr_eeprom_code {
> > > +       PHYTEC_IMX93_DDR_INVALID = PHYTEC_EEPROM_INVAL,
> > > +       PHYTEC_IMX93_LPDDR4X_512MB = 0,
> > > +       PHYTEC_IMX93_LPDDR4X_1GB = 1,
> > > +       PHYTEC_IMX93_LPDDR4X_2GB = 2,
> > > +       PHYTEC_IMX93_LPDDR4_512MB = 3,
> > > +       PHYTEC_IMX93_LPDDR4_1GB = 4,
> > > +       PHYTEC_IMX93_LPDDR4_2GB = 5,
> > > +};
> > 
> > This is inconsistent within enums. For the ddr, you set each value
> > explicitly, but for the others
> > you don't. Please choose one style.
> > 
> > Yannic
> > 
> > > [...]
> > > 
> > 
> >