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.stoid...@phytec.de>
> ---
> Cc: Mathieu Othacehe <m.othac...@gmail.com>
> Cc: Christoph Stoidner <c.stoid...@phytec.de>
> Cc: Stefano Babic <sba...@denx.de>
> Cc: Fabio Estevam <feste...@gmail.com>
> Cc: "NXP i.MX U-Boot Team" <uboot-...@nxp.com>
> Cc: Tom Rini <tr...@konsulko.com>
> Cc: Yannic Moog <y.m...@phytec.de>
> Cc: Primoz Fiser <primoz.fi...@norik.com>
> Cc: Andrej Picej <andrej.pi...@norik.com>
> Cc: Wadim Egorov <w.ego...@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.

>  #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.fi...@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.fi...@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

> [...]
> 


Reply via email to