Re: [u-boot][master][PATCH 1/3] pico-imx7d: add support for 2GB memory SoMs
On Sat, Jan 07, 2023 at 04:53:16PM +, Szőke Kálmán Benjamin wrote: [snip] > @Tom Rini Is there any plan to improve the patches, pull-requests, > technical review/threads processes to use a better web-technnolgies > instead of to use infinity long-long ping-pong mailing in a legacy > mailing lists? > In 2023, It's time to use something better, for example to use a > normal pull request features on GitHub/GitLab website for U-boot as > every modern open-source project use it. It could be more traceabla > and faster in ask a question for a developer, moreover the code > reviewing is much much better in this modern > way. If you have specific plans and resources you're going to be able to dedicate to such a project, I'd be happy to have your plan discussed on the custodians and board maintainer mailing lists. Otherwise, we should probably try and leverage some of the more recent posts / videos that try and explain and making emailing patches for the linux kernel be linked in our documentation as they apply here too. -- Tom signature.asc Description: PGP signature
Re: [u-boot][master][PATCH 1/3] pico-imx7d: add support for 2GB memory SoMs
My last three patches were tested in my company's custom i.MX7D pico SoMs
carrier board, it was workd well with 512MB and 1 GB SoM variants in U-boot
v2022.10. I have no any 2GB variants but i can trust in Technexion that it was
tested in their house, bedore.But it can be better if you take contact with
them and request a confirmation about your swtich case logic, is it will be
fine or not for
them.https://github.com/richard-huhttps://github.com/JoeZhang-tn@Tom RiniIs
there any plan to improve the patches, pull-requests, technical review/threads
processes to use a better web-technnolgies instead of to use infinity long-long
ping-pong mailing in a legacy mailing lists? In 2023, It's time to use
something better, for example to use a normal pull request features on
GitHub/GitLab website for U-boot as every modern open-source project use it. It
could be more traceabla and faster in ask a question for a developer, moreover
the code reviewing is much much better in this modern
way.https://github.com/u-boot/u-boot/pulls Eredeti levél
Feladó: Fabio Estevam Dátum: 2023 január 7
14:37:47Tárgy: Re: [u-boot][master][PATCH 1/3] pico-imx7d: add support for 2GB
memory SoMsCímzett: Szőke Kálmán Benjamin Hi Benjamin,
On Sat, Jan 7, 2023 at 8:22 AM Szőke Kálmán Benjamin
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.
Re: [u-boot][master][PATCH 1/3] pico-imx7d: add support for 2GB memory SoMs
Hi Benjamin,
Please don't top post.
On Sat, Jan 7, 2023 at 2:15 PM Szőke Kálmán Benjamin
wrote:
>
> My last three patches were tested in my company's custom i.MX7D pico SoMs
> carrier board, it was workd well with 512MB and 1 GB SoM variants in U-boot
> v2022.10. I have no any 2GB variants but i can trust in Technexion that it
> was tested in their house, before.
pico-imx7d_defconfig does not boot in v2022.10 either.
2f96d4dd95f8 ("imx7s/d: synchronise device trees with linux") landed
in v2022.10-rc4.
Anyway, please always generate and test the patches against top of tree U-Boot.
Re: [u-boot][master][PATCH 1/3] pico-imx7d: add support for 2GB memory SoMs
Hi Benjamin,
On Sat, Jan 7, 2023 at 8:22 AM Szőke Kálmán Benjamin
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.
Re: [u-boot][master][PATCH 1/3] pico-imx7d: add support for 2GB memory SoMs
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#L106Do
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.So i am prefering more to apply my last (copy pasted from Technexion)
patch, to support 2GB SoMs in u-boot
mainline.https://lists.denx.de/pipermail/u-boot/2022-December/502465.html
Eredeti levél Feladó: Fabio Estevam Dátum: 2022
december 31 17:31:01Tárgy: Re: [u-boot][master][PATCH 1/3] pico-imx7d: add
support for 2GB memory SoMsCímzett: egyszer...@freemail.huhi Benjamin, On Sun,
Dec 18, 2022 at 9:52 AM wrote: > > From: Benjamin
Szőke > > Take over codes from Techenxion to support
SoMs with 2GB DDR3. > > Signed-off-by: Benjamin Szőke
> --- > board/technexion/pico-imx7d/Makefile | 2 +- >
.../pico-imx7d/{spl.c => pico-imx7d_spl.c}| 30 +-- > 2
files changed, 28 insertions(+), 4 deletions(-) > rename
board/technexion/pico-imx7d/{spl.c => pico-imx7d_spl.c} (83%) Could you please
test whether the attached patch works for you? I don't have a 2GB board variant
here to test it myself. You also need to apply the following patch to fix the
boot against U-Boot top of tree:
https://lore.kernel.org/u-boot/20221231162514.334303-1-feste...@denx.de/T/#u
Re: [u-boot][master][PATCH 1/3] pico-imx7d: add support for 2GB memory SoMs
Hi Benjamin,
On Sun, Dec 18, 2022 at 9:52 AM wrote:
>
> From: Benjamin Szőke
>
> Take over codes from Techenxion to support SoMs with 2GB DDR3.
>
> Signed-off-by: Benjamin Szőke
> ---
> board/technexion/pico-imx7d/Makefile | 2 +-
> .../pico-imx7d/{spl.c => pico-imx7d_spl.c}| 30 +--
> 2 files changed, 28 insertions(+), 4 deletions(-)
> rename board/technexion/pico-imx7d/{spl.c => pico-imx7d_spl.c} (83%)
Could you please test whether the attached patch works for you?
I don't have a 2GB board variant here to test it myself.
You also need to apply the following patch to fix the boot against
U-Boot top of tree:
https://lore.kernel.org/u-boot/20221231162514.334303-1-feste...@denx.de/T/#u
From 1777fa36f9b1e93284969b49037daa3871e99a77 Mon Sep 17 00:00:00 2001
From: Fabio Estevam
Date: Sat, 31 Dec 2022 13:14:02 -0300
Subject: [PATCH] pico-imx7d: Add support for the 2GB variant
Add the board detection mechanism to be able to support
the 2GB variant.
Based on the code from TechNexion U-Boot downstream tree.
Signed-off-by: Fabio Estevam
---
board/technexion/pico-imx7d/spl.c | 30 +++---
1 file changed, 27 insertions(+), 3 deletions(-)
diff --git a/board/technexion/pico-imx7d/spl.c b/board/technexion/pico-imx7d/spl.c
index df5f058577..f86fee9c88 100644
--- a/board/technexion/pico-imx7d/spl.c
+++ b/board/technexion/pico-imx7d/spl.c
@@ -61,6 +61,8 @@ static struct ddrc ddrc_regs_val = {
.dramtmg0 = 0x09081109,
.addrmap0 = 0x001f,
.addrmap1 = 0x00080808,
+ .addrmap2 = 0x,
+ .addrmap3 = 0x,
.addrmap4 = 0x0f0f,
.addrmap5 = 0x07070707,
.addrmap6 = 0x0f0f0707,
@@ -100,16 +102,38 @@ static void gpr_init(void)
writel(0x4F45, &gpr_regs->gpr[1]);
}
-static bool is_1g(void)
+/*
+ * Revision Detection
+ *
+ * GPIO1_12 GPIO1_13
+ * 00 1GB DDR3
+ * 01 2GB DDR3
+ * 10 512MB DDR3
+ */
+
+static int imx7d_pico_detect_board(void)
{
gpio_direction_input(IMX_GPIO_NR(1, 12));
- return !gpio_get_value(IMX_GPIO_NR(1, 12));
+ gpio_direction_input(IMX_GPIO_NR(1, 13));
+
+ return gpio_get_value(IMX_GPIO_NR(1, 12)) << 1 |
+ gpio_get_value(IMX_GPIO_NR(1, 13));
}
static void ddr_init(void)
{
- if (is_1g())
+ switch (imx7d_pico_detect_board()) {
+ case 0:
ddrc_regs_val.addrmap6 = 0x0f070707;
+ break;
+ case 1:
+ ddrc_regs_val.addrmap0 = 0x001f;
+ ddrc_regs_val.addrmap1 = 0x00181818;
+ ddrc_regs_val.addrmap4 = 0x0f0f;
+ ddrc_regs_val.addrmap5 = 0x04040404;
+ ddrc_regs_val.addrmap6 = 0x04040404;
+ break;
+ }
mx7_dram_cfg(&ddrc_regs_val, &ddrc_mp_val, &ddr_phy_regs_val,
&calib_param);
--
2.25.1
Re: [u-boot][master][PATCH 1/3] pico-imx7d: add support for 2GB memory SoMs
I will not keep, most of Technexion SoM codes are broken or limited in functionalities (like 2GB RAM support missing, dual boot missing), because there is no any active maintenancing and as i see nobody cares about it from freescale community.What is the point of renaming and rewriting these codes, which they have already made for themselves once and work well, now? My opinion is, need to migrate only these codes, and do not need to change.
Re: [u-boot][master][PATCH 1/3] pico-imx7d: add support for 2GB memory SoMs
Technexion did that renaming, it is better to keep in synchronized.https://github.com/TechNexion/u-boot-tn-imx/tree/tn-imx_v2021.04_5.10.72_2.2.0-stable/board/technexion/pico-imx7d
Re: [u-boot][master][PATCH 1/3] pico-imx7d: add support for 2GB memory SoMs
All codes come from official u-boot codes from Technexion may they knows better why it is optimal in this form.https://github.com/TechNexion/u-boot-tn-imx/tree/tn-imx_v2021.04_5.10.72_2.2.0-stable/board/technexion/pico-imx7d
Re: [u-boot][master][PATCH 1/3] pico-imx7d: add support for 2GB memory SoMs
On Mon, Dec 19, 2022 at 9:12 PM Szőke Kálmán Benjamin wrote: > > Technexion did that renaming, it is better to keep in synchronized. > https://github.com/TechNexion/u-boot-tn-imx/tree/tn-imx_v2021.04_5.10.72_2.2.0-stable/board/technexion/pico-imx7d Sorry, that's not a good justification. Just keep the original file name.
Re: [u-boot][master][PATCH 1/3] pico-imx7d: add support for 2GB memory SoMs
On Sun, Dec 18, 2022 at 9:52 AM wrote:
>
> From: Benjamin Szőke
>
> Take over codes from Techenxion to support SoMs with 2GB DDR3.
>
> Signed-off-by: Benjamin Szőke
> ---
> board/technexion/pico-imx7d/Makefile | 2 +-
> .../pico-imx7d/{spl.c => pico-imx7d_spl.c}| 30 +--
Also, what is the purpose of this file rename?
I don't think it is needed.
Re: [u-boot][master][PATCH 1/3] pico-imx7d: add support for 2GB memory SoMs
On Sun, Dec 18, 2022 at 9:52 AM wrote:
> +static void ddr_init(void)
> +{
> + if (is_1g()) {
> + if (is_2g()) {
This logic is confusing.
It would be better to read the two GPIOs and then do a switch case to handle
the possible values.
[u-boot][master][PATCH 1/3] pico-imx7d: add support for 2GB memory SoMs
From: Benjamin Szőke
Take over codes from Techenxion to support SoMs with 2GB DDR3.
Signed-off-by: Benjamin Szőke
---
board/technexion/pico-imx7d/Makefile | 2 +-
.../pico-imx7d/{spl.c => pico-imx7d_spl.c}| 30 +--
2 files changed, 28 insertions(+), 4 deletions(-)
rename board/technexion/pico-imx7d/{spl.c => pico-imx7d_spl.c} (83%)
diff --git a/board/technexion/pico-imx7d/Makefile
b/board/technexion/pico-imx7d/Makefile
index 4ae3d606b5..b45b127884 100644
--- a/board/technexion/pico-imx7d/Makefile
+++ b/board/technexion/pico-imx7d/Makefile
@@ -1,4 +1,4 @@
# SPDX-License-Identifier: GPL-2.0+
# (C) Copyright 2017 NXP Semiconductors
-obj-y := pico-imx7d.o spl.o
+obj-y := pico-imx7d.o pico-imx7d_spl.o
diff --git a/board/technexion/pico-imx7d/spl.c
b/board/technexion/pico-imx7d/pico-imx7d_spl.c
similarity index 83%
rename from board/technexion/pico-imx7d/spl.c
rename to board/technexion/pico-imx7d/pico-imx7d_spl.c
index df5f058577..0009c55022 100644
--- a/board/technexion/pico-imx7d/spl.c
+++ b/board/technexion/pico-imx7d/pico-imx7d_spl.c
@@ -61,6 +61,8 @@ static struct ddrc ddrc_regs_val = {
.dramtmg0 = 0x09081109,
.addrmap0 = 0x001f,
.addrmap1 = 0x00080808,
+ .addrmap2 = 0x,
+ .addrmap3 = 0x,
.addrmap4 = 0x0f0f,
.addrmap5 = 0x07070707,
.addrmap6 = 0x0f0f0707,
@@ -100,17 +102,39 @@ static void gpr_init(void)
writel(0x4F45, &gpr_regs->gpr[1]);
}
+/**
+* Revision Detection
+*
+* DDR_TYPE_DET_1 DDR_TYPE_DET_2
+* GPIO_1 GPIO_2
+* 01 2GB DDR3
+* 00 1GB DDR3
+* 10 512MB DDR3
+***/
static bool is_1g(void)
{
gpio_direction_input(IMX_GPIO_NR(1, 12));
return !gpio_get_value(IMX_GPIO_NR(1, 12));
}
-static void ddr_init(void)
+static bool is_2g(void)
{
- if (is_1g())
- ddrc_regs_val.addrmap6 = 0x0f070707;
+ gpio_direction_input(IMX_GPIO_NR(1, 13));
+ return gpio_get_value(IMX_GPIO_NR(1, 13));
+}
+static void ddr_init(void)
+{
+ if (is_1g()) {
+ if (is_2g()) {
+ ddrc_regs_val.addrmap0 = 0x001f;
+ ddrc_regs_val.addrmap1 = 0x00181818;
+ ddrc_regs_val.addrmap4 = 0x0f0f;
+ ddrc_regs_val.addrmap5 = 0x04040404;
+ ddrc_regs_val.addrmap6 = 0x04040404;
+ } else
+ ddrc_regs_val.addrmap6 = 0x0f070707;
+ }
mx7_dram_cfg(&ddrc_regs_val, &ddrc_mp_val, &ddr_phy_regs_val,
&calib_param);
}
--
2.38.1.windows.1
