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

[Christoph Stoidner <C.Stoidner@phytec.de>]

Hi Wadmin,

On Mo, 2024-11-04 at 14:53 +0100, Wadim Egorov wrote:
> Hi Christoph,
> 
> Am 04.11.24 um 11:41 schrieb Christoph Stoidner:
> > 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>
> 
> <snip>
> 
> > --- a/board/phytec/phycore_imx93/phycore-imx93.c
> > +++ b/board/phytec/phycore_imx93/phycore-imx93.c
> > @@ -3,6 +3,7 @@
> >    * Copyright (C) 2023 PHYTEC Messtechnik GmbH
> >    * Author: Christoph Stoidner <c.stoidner@phytec.de>
> >    * Copyright (C) 2024 Mathieu Othacehe <m.othacehe@gmail.com>
> > + * Copyright (C) 2024 PHYTEC Messtechnik GmbH
> >    */
> >   
> >   #include <asm/arch-imx9/ccm_regs.h>
> > @@ -12,11 +13,21 @@
> >   #include <asm/global_data.h>
> >   #include <asm/mach-imx/boot_mode.h>
> >   #include <env.h>
> > +#include <fdt_support.h>
> > +
> > +#include "../common/imx93_som_detection.h"
> >   
> >   DECLARE_GLOBAL_DATA_PTR;
> >   
> > +#define EEPROM_ADDR            0x50
> > +
> >   int board_init(void)
> >   {
> > +       int ret = phytec_eeprom_data_setup(NULL, 2, EEPROM_ADDR);
> > +
> > +       if (ret)
> > +               printf("%s: EEPROM data init failed\n", __func__);
> > +
> >         return 0;
> >   }
> >   
> > @@ -40,3 +51,44 @@ int board_late_init(void)
> >   
> >         return 0;
> >   }
> > +
> > +static void emmc_fixup(void *blob, struct phytec_eeprom_data
> > *data)
> > +{
> > +       u8 option = phytec_imx93_get_opt(data,
> > PHYTEC_IMX93_OPT_FEAT);
> 
> I am thinking about future SoMs using similar variants.
> 
> 
> 
> > +       int offset;
> > +
> > +       if (option == PHYTEC_EEPROM_INVAL)
> > +               goto err;
> > +
> > +       /* Check "IO Voltage 1v8" flag is set */
> > +       if (option & 0x01) {
> If you abstract away the option details into an own function, e.g. 
> phytec_get_imx93_io_volt(), you could reduce the amount of code in
> your 
> board files. Also, if SoMs change the option layouts for a specific 
> detail (for whatever reason), you would have an abstraction layer to 
> details with it.
> 

Yes, that might make sense. I will send a v2 for it.

> > +               offset = fdt_node_offset_by_compat_reg(blob,
> > "fsl,imx93-usdhc",
> > +                                                      0x42850000);
> > +               if (offset)
> > +                       fdt_delprop(blob, offset, "no-1-8-v");
> > +               else
> > +                       goto err;
> > +       }
> > +
> > +       return;
> > +err:
> > +       printf("Could not detect eMMC VDD-IO. Fall back to
> > default.\n");
> > +}
> > +
> > +int board_fix_fdt(void *blob)
> > +{
> > +       struct phytec_eeprom_data data;
> > +
> > +       phytec_eeprom_data_setup(&data, 2, EEPROM_ADDR);
> > +
> > +       emmc_fixup(blob, &data);
> > +
> > +       return 0;
> > +}
> > +
> > +int ft_board_setup(void *blob, struct bd_info *bd)
> > +{
> > +       emmc_fixup(blob, NULL);
> > +
> > +       return 0;
> > +}
> > diff --git a/board/phytec/phycore_imx93/spl.c
> > b/board/phytec/phycore_imx93/spl.c
> > index 17a8736c73..cf9c94eb45 100644
> > --- a/board/phytec/phycore_imx93/spl.c
> > +++ b/board/phytec/phycore_imx93/spl.c
> > @@ -3,6 +3,7 @@
> >    * Copyright (C) 2023 PHYTEC Messtechnik GmbH
> >    * Author: Christoph Stoidner <c.stoidner@phytec.de>
> >    * Copyright (C) 2024 Mathieu Othacehe <m.othacehe@gmail.com>
> > + * Copyright (C) 2024 PHYTEC Messtechnik GmbH
> >    */
> >   
> >   #include <asm/arch/clock.h>
> > @@ -20,6 +21,8 @@
> >   #include <power/pca9450.h>
> >   #include <spl.h>
> >   
> > +#include "../common/imx93_som_detection.h"
> > +
> >   DECLARE_GLOBAL_DATA_PTR;
> >   
> >   /*
> > @@ -27,6 +30,13 @@ DECLARE_GLOBAL_DATA_PTR;
> >    * when pca9451a support is added.
> >    */
> >   #define PCA9450_REG_PWRCTRL_TOFF_DEB    BIT(5)
> > +#define EEPROM_ADDR            0x50
> > +
> > +/*
> > + * Prototypes of automatically generated ram config file
> > + */
> > +void set_dram_timings_2gb_lpddr4x(void);
> > +void set_dram_timings_1gb_lpddr4x_900mhz(void);
> >   
> >   int spl_board_boot_device(enum boot_device boot_dev_spl)
> >   {
> > @@ -44,8 +54,56 @@ void spl_board_init(void)
> >         puts("Normal Boot\n");
> >   }
> >   
> > +enum phytec_imx93_ddr_eeprom_code {
> > +       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,
> > +};
> ^ IMO this belongs into imx93_som_detection.h file

That enum is in spl.c, because there is nothing like
phytec_get_imx93_ram_type(), but we use the generic
phytec_imx93_get_opt() instead. So that is very similar to your comment
about phytec_get_imx93_io_volt() above.

I will add a phytec_get_imx93_ram_type() to imx93_som_detection.c
and also move thereby enum phytec_imx93_ddr_eeprom_code to
imx93_som_detection.h .

Thanks,
Christoph

> 
> Regards,
> Wadim
> > +
> >   void spl_dram_init(void)
> >   {
> > +       int ret;
> > +       enum phytec_imx93_ddr_eeprom_code ddr_opt = INVALID;
> > +
> > +       /* NOTE: In SPL lpi2c3 is mapped to bus 0 */
> > +       ret = phytec_eeprom_data_setup(NULL, 0, EEPROM_ADDR);
> > +       if (ret && !IS_ENABLED(CONFIG_PHYCORE_IMX93_RAM_TYPE_FIX))
> > +               goto out;
> > +
> > +       ret = phytec_imx93_detect(NULL);
> > +       if (!ret)
> > +               phytec_print_som_info(NULL);
> > +
> > +       if (IS_ENABLED(CONFIG_PHYCORE_IMX93_RAM_TYPE_FIX)) {
> > +               if
> > (IS_ENABLED(CONFIG_PHYCORE_IMX93_RAM_TYPE_LPDDR4X_1GB))
> > +                       ddr_opt = PHYTEC_IMX93_LPDDR4X_1GB;
> > +               else if
> > (IS_ENABLED(CONFIG_PHYCORE_IMX93_RAM_TYPE_LPDDR4X_2GB))
> > +                       ddr_opt = PHYTEC_IMX93_LPDDR4X_2GB;
> > +       } else {
> > +               ddr_opt = phytec_imx93_get_opt(NULL,
> > PHYTEC_IMX93_OPT_DDR);
> > +       }
> > +
> > +       switch (ddr_opt) {
> > +       case PHYTEC_IMX93_LPDDR4X_1GB:
> > +               if (is_voltage_mode(VOLT_LOW_DRIVE))
> > +                       set_dram_timings_1gb_lpddr4x_900mhz();
> > +               break;
> > +       case PHYTEC_IMX93_LPDDR4X_2GB:
> > +               set_dram_timings_2gb_lpddr4x();
> > +               break;
> > +       default:
> > +               goto out;
> > +       }
> > +       ddr_init(&dram_timing);
> > +       return;
> > +out:
> > +       puts("Could not detect correct RAM type and size. Fall back
> > to default.\n");
> > +       if (is_voltage_mode(VOLT_LOW_DRIVE))
> > +               set_dram_timings_1gb_lpddr4x_900mhz();
> >         ddr_init(&dram_timing);
> >   }
> >   
>