| Message ID | 20210823024952.2047237-2-grandpaul@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | arm: imx8m: imx8mm-cl-iot-gate: Add support for detect memory size | expand |
Hi Paul, Thanks for taking care of this. On Sun, Aug 22, 2021 at 11:50 PM Ying-Chun Liu <grandpaul@gmail.com> wrote: > +int board_phys_sdram_size(phys_size_t *size) > +{ > + struct lpddr4_tcm_desc *lpddr4_tcm_desc = > + (struct lpddr4_tcm_desc *)TCM_DATA_CFG; > + > + switch (lpddr4_tcm_desc->size) { > + case 4096: > + case 2048: > + case 1024: > + *size = (1L << 20) * lpddr4_tcm_desc->size; > + break; > + default: > + printf("%s: DRAM size %uM is not supported\n", > + __func__, > + lpddr4_tcm_desc->size); > + while (1) > + ; Shouldn't the while(1) be replaced by hang() instead? > #define CONFIG_SYS_SDRAM_BASE 0x40000000 > #define PHYS_SDRAM 0x40000000 > -#define PHYS_SDRAM_SIZE 0x80000000 /* 2GB DDR */ > +#define PHYS_SDRAM_SIZE 0x40000000 /* 1GB DDR */ This looks like an unnecessary change for me, as the board size is detected in run-time.
Hi Paul On Mon, 2021-08-23 at 10:49 +0800, Ying-Chun Liu wrote: > From: "Ying-Chun Liu (PaulLiu)" <paulliu@debian.org> > > When purchasing imx8mm-cl-iot-gate it is able to customize the > memory size. It could be 1GB, 2GB and 4GB. We implement > board_phys_sdram_size() to detect the memory size for usage. > > Signed-off-by: Ying-Chun Liu (PaulLiu) <paulliu@debian.org> > Cc: Fabio Estevam <festevam@denx.de> > Cc: Frieder Schrempf <frieder.schrempf@kontron.de> > Cc: uboot-imx <uboot-imx@nxp.com> > --- > .../imx8mm-cl-iot-gate/imx8mm-cl-iot-gate.c | 25 +++++++++++++++++++ > include/configs/imx8mm-cl-iot-gate.h | 2 +- > 2 files changed, 26 insertions(+), 1 deletion(-) > > diff --git a/board/compulab/imx8mm-cl-iot-gate/imx8mm-cl-iot-gate.c b/board/compulab/imx8mm-cl-iot-gate/imx8mm- > cl-iot-gate.c > index eabcc842a4..01c6011b75 100644 > --- a/board/compulab/imx8mm-cl-iot-gate/imx8mm-cl-iot-gate.c > +++ b/board/compulab/imx8mm-cl-iot-gate/imx8mm-cl-iot-gate.c > @@ -14,8 +14,33 @@ > #include <asm/arch/sys_proto.h> > #include <asm/io.h> > > +#include "ddr/ddr.h" > + > DECLARE_GLOBAL_DATA_PTR; > > +int board_phys_sdram_size(phys_size_t *size) > +{ > + struct lpddr4_tcm_desc *lpddr4_tcm_desc = > + (struct lpddr4_tcm_desc *)TCM_DATA_CFG; > + > + switch (lpddr4_tcm_desc->size) { > + case 4096: > + case 2048: > + case 1024: > + *size = (1L << 20) * lpddr4_tcm_desc->size; > + break; > + default: > + printf("%s: DRAM size %uM is not supported\n", > + __func__, > + lpddr4_tcm_desc->size); > + while (1) > + ; > + break; > + }; Why not simply using generic get_ram_size() as we e.g. did here [1]? > + > + return 0; > +} > + > static int setup_fec(void) > { > if (IS_ENABLED(CONFIG_FEC_MXC)) { > diff --git a/include/configs/imx8mm-cl-iot-gate.h b/include/configs/imx8mm-cl-iot-gate.h > index faeee2178c..1e835563d6 100644 > --- a/include/configs/imx8mm-cl-iot-gate.h > +++ b/include/configs/imx8mm-cl-iot-gate.h > @@ -158,7 +158,7 @@ > > #define CONFIG_SYS_SDRAM_BASE 0x40000000 > #define PHYS_SDRAM 0x40000000 > -#define PHYS_SDRAM_SIZE 0x80000000 /* 2GB DDR */ > +#define PHYS_SDRAM_SIZE 0x40000000 /* 1GB DDR */ In our implementation we use PHYS_SDRAM_SIZE as the upper limit which get_ram_size() then actually tests for. > #define CONFIG_MXC_UART_BASE UART3_BASE_ADDR [1] https://marc.info/?l=u-boot&m=160387919927218 Cheers Marcel
Hi Marcel, Thanks for the comment. But isn't it more straightforward to use TCM data? I mean it is already tested when DRAM initializes. Also there's already a commit that avoids U-boot using address more than 4GB on imx8m platform for some reasons I don't know in detail but the commit is e27bddff . Because the base is 0x40000000. So 4GB address means it is about 3G left. But the largest memory we can purchase is 4GB. Which is already beyond the limit. And I don't have Compluab 4GB boards. So I think we should avoid directly testing the address more than 4GB addr? We just report the correct memory size here based on the DRAM init test. And restrict the memory usage by board_get_usable_ram_top() by previous commit. The Compulab board is using various memory chips. They only let you to choose the size, not the company/brand. So the dram init code actually test every brand of the chips at beginning (SPL) so we have the correct brand and size stored in TCM area. I have 5 Compulab boards on my hand. All of them are 2GB ram but different brand. But it is still not covered all the combinations I think. So, can we keep using the TCM data rather than do the actual testing? Yours, Paul On Mon, Aug 23, 2021 at 11:14 PM Marcel Ziswiler < marcel.ziswiler@toradex.com> wrote: > Hi Paul > > On Mon, 2021-08-23 at 10:49 +0800, Ying-Chun Liu wrote: > > From: "Ying-Chun Liu (PaulLiu)" <paulliu@debian.org> > > > > When purchasing imx8mm-cl-iot-gate it is able to customize the > > memory size. It could be 1GB, 2GB and 4GB. We implement > > board_phys_sdram_size() to detect the memory size for usage. > > > > Signed-off-by: Ying-Chun Liu (PaulLiu) <paulliu@debian.org> > > Cc: Fabio Estevam <festevam@denx.de> > > Cc: Frieder Schrempf <frieder.schrempf@kontron.de> > > Cc: uboot-imx <uboot-imx@nxp.com> > > --- > > .../imx8mm-cl-iot-gate/imx8mm-cl-iot-gate.c | 25 +++++++++++++++++++ > > include/configs/imx8mm-cl-iot-gate.h | 2 +- > > 2 files changed, 26 insertions(+), 1 deletion(-) > > > > diff --git a/board/compulab/imx8mm-cl-iot-gate/imx8mm-cl-iot-gate.c > b/board/compulab/imx8mm-cl-iot-gate/imx8mm- > > cl-iot-gate.c > > index eabcc842a4..01c6011b75 100644 > > --- a/board/compulab/imx8mm-cl-iot-gate/imx8mm-cl-iot-gate.c > > +++ b/board/compulab/imx8mm-cl-iot-gate/imx8mm-cl-iot-gate.c > > @@ -14,8 +14,33 @@ > > #include <asm/arch/sys_proto.h> > > #include <asm/io.h> > > > > +#include "ddr/ddr.h" > > + > > DECLARE_GLOBAL_DATA_PTR; > > > > +int board_phys_sdram_size(phys_size_t *size) > > +{ > > + struct lpddr4_tcm_desc *lpddr4_tcm_desc = > > + (struct lpddr4_tcm_desc *)TCM_DATA_CFG; > > + > > + switch (lpddr4_tcm_desc->size) { > > + case 4096: > > + case 2048: > > + case 1024: > > + *size = (1L << 20) * lpddr4_tcm_desc->size; > > + break; > > + default: > > + printf("%s: DRAM size %uM is not supported\n", > > + __func__, > > + lpddr4_tcm_desc->size); > > + while (1) > > + ; > > + break; > > + }; > > Why not simply using generic get_ram_size() as we e.g. did here [1]? > > > + > > + return 0; > > +} > > + > > static int setup_fec(void) > > { > > if (IS_ENABLED(CONFIG_FEC_MXC)) { > > diff --git a/include/configs/imx8mm-cl-iot-gate.h > b/include/configs/imx8mm-cl-iot-gate.h > > index faeee2178c..1e835563d6 100644 > > --- a/include/configs/imx8mm-cl-iot-gate.h > > +++ b/include/configs/imx8mm-cl-iot-gate.h > > @@ -158,7 +158,7 @@ > > > > #define CONFIG_SYS_SDRAM_BASE 0x40000000 > > #define PHYS_SDRAM 0x40000000 > > -#define PHYS_SDRAM_SIZE 0x80000000 /* 2GB DDR */ > > +#define PHYS_SDRAM_SIZE 0x40000000 /* 1GB DDR */ > > In our implementation we use PHYS_SDRAM_SIZE as the upper limit which > get_ram_size() then actually tests for. > > > #define CONFIG_MXC_UART_BASE UART3_BASE_ADDR > > [1] https://marc.info/?l=u-boot&m=160387919927218 > > Cheers > > Marcel >
Hi Paul On Tue, 2021-08-24 at 04:04 +0800, PaulLiu wrote: > Hi Marcel, > > Thanks for the comment. > But isn't it more straightforward to use TCM data? It was not quite clear to me what you were talking about. So I had a quick look at CompuLab's implementation of all this. Let us hope it works fine but I would not exactly call this straightforward. > I mean it is already tested when DRAM initializes. > Also there's already a commit that avoids U-boot using address more than 4GB on imx8m platform for some > reasons I don't know in detail but the commit is e27bddff . I don't think any of this would be needed if everybody would agree on using the generic get_ram_size() routine. The world could be so much simpler! > Because the base is 0x40000000. So 4GB address means it is about 3G left. > But the largest memory we can purchase is 4GB. Which is already beyond the limit. > And I don't have Compluab 4GB boards. Yeah, I remember from the i.MX 8M Plus this actually requires 64-bit addressing to really work. > So I think we should avoid directly testing the address more than 4GB addr? Well, in the 32-bit world there is nothing beyond those 4GB (;-p). > We just report the correct memory size here based on the DRAM init test. > And restrict the memory usage by board_get_usable_ram_top() by previous commit. To be honest, I really think this is done totally the wrong way around, but then as it is not my board I won't stand in anybody's way... > The Compulab board is using various memory chips. They only let you to choose the size, not the > company/brand. So the dram init code actually test every brand of the chips at beginning (SPL) so we have the > correct brand and size stored in TCM area. I have 5 Compulab boards on my hand. All of them are 2GB ram but > different brand. But it is still not covered all the combinations I think. Yes, I saw that. Considering that NXP was unable to provide anything halfway generic this looks OKish. > So, can we keep using the TCM data rather than do the actual testing? Sure, as stated above, I won't stay in your way. Just thought it worth pitching a more generic way of going about RAM sizing. > Yours, > Paul Cheers Marcel > On Mon, Aug 23, 2021 at 11:14 PM Marcel Ziswiler <marcel.ziswiler@toradex.com> wrote: > > Hi Paul > > > > On Mon, 2021-08-23 at 10:49 +0800, Ying-Chun Liu wrote: > > > From: "Ying-Chun Liu (PaulLiu)" <paulliu@debian.org> > > > > > > When purchasing imx8mm-cl-iot-gate it is able to customize the > > > memory size. It could be 1GB, 2GB and 4GB. We implement > > > board_phys_sdram_size() to detect the memory size for usage. > > > > > > Signed-off-by: Ying-Chun Liu (PaulLiu) <paulliu@debian.org> > > > Cc: Fabio Estevam <festevam@denx.de> > > > Cc: Frieder Schrempf <frieder.schrempf@kontron.de> > > > Cc: uboot-imx <uboot-imx@nxp.com> > > > --- > > > .../imx8mm-cl-iot-gate/imx8mm-cl-iot-gate.c | 25 +++++++++++++++++++ > > > include/configs/imx8mm-cl-iot-gate.h | 2 +- > > > 2 files changed, 26 insertions(+), 1 deletion(-) > > > > > > diff --git a/board/compulab/imx8mm-cl-iot-gate/imx8mm-cl-iot-gate.c b/board/compulab/imx8mm-cl-iot- > > gate/imx8mm- > > > cl-iot-gate.c > > > index eabcc842a4..01c6011b75 100644 > > > --- a/board/compulab/imx8mm-cl-iot-gate/imx8mm-cl-iot-gate.c > > > +++ b/board/compulab/imx8mm-cl-iot-gate/imx8mm-cl-iot-gate.c > > > @@ -14,8 +14,33 @@ > > > #include <asm/arch/sys_proto.h> > > > #include <asm/io.h> > > > > > > +#include "ddr/ddr.h" > > > + > > > DECLARE_GLOBAL_DATA_PTR; > > > > > > +int board_phys_sdram_size(phys_size_t *size) > > > +{ > > > + struct lpddr4_tcm_desc *lpddr4_tcm_desc = > > > + (struct lpddr4_tcm_desc *)TCM_DATA_CFG; > > > + > > > + switch (lpddr4_tcm_desc->size) { > > > + case 4096: > > > + case 2048: > > > + case 1024: > > > + *size = (1L << 20) * lpddr4_tcm_desc->size; > > > + break; > > > + default: > > > + printf("%s: DRAM size %uM is not supported\n", > > > + __func__, > > > + lpddr4_tcm_desc->size); > > > + while (1) > > > + ; > > > + break; > > > + }; > > > > Why not simply using generic get_ram_size() as we e.g. did here [1]? > > > > > + > > > + return 0; > > > +} > > > + > > > static int setup_fec(void) > > > { > > > if (IS_ENABLED(CONFIG_FEC_MXC)) { > > > diff --git a/include/configs/imx8mm-cl-iot-gate.h b/include/configs/imx8mm-cl-iot-gate.h > > > index faeee2178c..1e835563d6 100644 > > > --- a/include/configs/imx8mm-cl-iot-gate.h > > > +++ b/include/configs/imx8mm-cl-iot-gate.h > > > @@ -158,7 +158,7 @@ > > > > > > #define CONFIG_SYS_SDRAM_BASE 0x40000000 > > > #define PHYS_SDRAM 0x40000000 > > > -#define PHYS_SDRAM_SIZE 0x80000000 /* 2GB DDR */ > > > +#define PHYS_SDRAM_SIZE 0x40000000 /* 1GB DDR */ > > > > In our implementation we use PHYS_SDRAM_SIZE as the upper limit which get_ram_size() then actually tests > > for. > > > > > #define CONFIG_MXC_UART_BASE UART3_BASE_ADDR > > > > [1] https://marc.info/?l=u-boot&m=160387919927218 > > > > Cheers > > > > Marcel
diff --git a/board/compulab/imx8mm-cl-iot-gate/imx8mm-cl-iot-gate.c b/board/compulab/imx8mm-cl-iot-gate/imx8mm-cl-iot-gate.c index eabcc842a4..01c6011b75 100644 --- a/board/compulab/imx8mm-cl-iot-gate/imx8mm-cl-iot-gate.c +++ b/board/compulab/imx8mm-cl-iot-gate/imx8mm-cl-iot-gate.c @@ -14,8 +14,33 @@ #include <asm/arch/sys_proto.h> #include <asm/io.h> +#include "ddr/ddr.h" + DECLARE_GLOBAL_DATA_PTR; +int board_phys_sdram_size(phys_size_t *size) +{ + struct lpddr4_tcm_desc *lpddr4_tcm_desc = + (struct lpddr4_tcm_desc *)TCM_DATA_CFG; + + switch (lpddr4_tcm_desc->size) { + case 4096: + case 2048: + case 1024: + *size = (1L << 20) * lpddr4_tcm_desc->size; + break; + default: + printf("%s: DRAM size %uM is not supported\n", + __func__, + lpddr4_tcm_desc->size); + while (1) + ; + break; + }; + + return 0; +} + static int setup_fec(void) { if (IS_ENABLED(CONFIG_FEC_MXC)) { diff --git a/include/configs/imx8mm-cl-iot-gate.h b/include/configs/imx8mm-cl-iot-gate.h index faeee2178c..1e835563d6 100644 --- a/include/configs/imx8mm-cl-iot-gate.h +++ b/include/configs/imx8mm-cl-iot-gate.h @@ -158,7 +158,7 @@ #define CONFIG_SYS_SDRAM_BASE 0x40000000 #define PHYS_SDRAM 0x40000000 -#define PHYS_SDRAM_SIZE 0x80000000 /* 2GB DDR */ +#define PHYS_SDRAM_SIZE 0x40000000 /* 1GB DDR */ #define CONFIG_MXC_UART_BASE UART3_BASE_ADDR