[u-boot][master][PATCH 1/3] pico-imx7d: add support for 2GB memory SoMs

[Fabio Estevam]

Hi Benjamin,

On Sat, Jan 7, 2023 at 8:22 AM Szőke Kálmán Benjamin
<egyszeregy at freemail.hu> wrote:
>
> Original code was a nested if statement for checking 1g() and 2g().
> https://github.com/TechNexion/u-boot-tn-imx/blob/tn-imx_v2021.04_5.10.72_2.2.0-stable/board/technexion/pico-imx7d/pico-imx7d_spl.c#L106

Yes, I saw that, but it doesn't look good. We can't blindly take the
vendor code as-is and throw it to mainline.

When I added the is_1g() function there was only the 512MB or 1GB
variants, so the function name was meaningful.

After adding the 2GB variant, the is_1g() function no longer means
that the board has 1GB of RAM.

If you want to keep TechNexion logic, at least rename is_1g() to
is_gpio1_12_low()
and is_2g() to is_gpio1_13_high().

> Do you have any information about the PCB layout of the different i.MX7 pico SoM revisions?Question is, what is the electrical states of GPIO1_12 and GPIO1_13 in real for these variants? My fear is that maybe one of them is not connected to any VDD (high) or GND (low). Question can be, what is the default logical state for it high or low if one of them is not connected neither to GND or VDD?
>
> I totaly do not understand why you need to completly refactoring the code if you have no posibbilites to test it with all revision of SoMs. I rather prefere to keep the original if statement code from Technexion, because that should works 100% and tested. I have no any 2GB varians SoMs yet, also i can not test it yet.

The code below really bothers me:

static void ddr_init(void)
{
       if (is_1g()) {
               if (is_2g()) {

"If the board has 1GB of RAM, then check if it has 2GB of RAM"

If you want to replace it with is_gpio1_12_low() and
is_gpio1_13_high(), as suggested above, I would accept it.

Also, you submitted the patch without even testing it. That's not good.

The pico-imx7d board was not even booting at all. I fixed it recently by:

https://source.denx.de/u-boot/u-boot/-/commit/f8548ce0e09385926574283b17af6c2cd4e32af2

and now you say that you also don't have the 2GB variant.

Please make sure to test your patches against U-Boot mainline.