Hi Benjamin,

On Sat, Jan 7, 2023 at 8:22 AM Szőke Kálmán Benjamin
<egyszer...@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.

Reply via email to