Re: [PATCH v4 5/9] xtensa: Remove arch_setup_bdinfo()
Hi Stefan,
On 13.08.2020 11:15, Stefan Roese wrote:
Hi Ovidiu,
On 13.08.20 10:09, Stefan Roese wrote:
Hi Ovidiu,
On 13.08.20 09:57, Ovidiu Panait wrote:
Hi Stefan,
On 13.08.2020 08:47, Stefan Roese wrote:
arch_setup_bdinfo() only configures the deprecated bi_memstart &
bi_memsize values, which should not be needed any more. Lets remove
this file completely.
Signed-off-by: Stefan Roese
---
Changes in v4:
- New patch
arch/xtensa/lib/Makefile | 2 +-
arch/xtensa/lib/bdinfo.c | 22 --
2 files changed, 1 insertion(+), 23 deletions(-)
delete mode 100644 arch/xtensa/lib/bdinfo.c
diff --git a/arch/xtensa/lib/Makefile b/arch/xtensa/lib/Makefile
index ceee59b9bd..c59df7d372 100644
--- a/arch/xtensa/lib/Makefile
+++ b/arch/xtensa/lib/Makefile
@@ -5,4 +5,4 @@
obj-$(CONFIG_CMD_BOOTM) += bootm.o
-obj-y += cache.o misc.o relocate.o time.o bdinfo.o
+obj-y += cache.o misc.o relocate.o time.o
diff --git a/arch/xtensa/lib/bdinfo.c b/arch/xtensa/lib/bdinfo.c
deleted file mode 100644
index 4ec8529521..00
--- a/arch/xtensa/lib/bdinfo.c
+++ /dev/null
@@ -1,22 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0+
-/*
- * XTENSA-specific information for the 'bd' command
- *
- * (C) Copyright 2003
- * Wolfgang Denk, DENX Software Engineering, w...@denx.de.
- */
-
-#include
-#include
-
-DECLARE_GLOBAL_DATA_PTR;
-
-int arch_setup_bdinfo(void)
-{
- struct bd_info *bd = gd->bd;
-
- bd->bi_memstart = PHYSADDR(CONFIG_SYS_SDRAM_BASE);
- bd->bi_memsize = CONFIG_SYS_SDRAM_SIZE;
-
When I did the setup_bdinfo refactoring, I realized that xtensa was
the only arch handling bi_{memstart,memsize} in a special way:
bd->bi_memstart = PHYSADDR(CONFIG_SYS_SDRAM_BASE);
bd->bi_memsize = CONFIG_SYS_SDRAM_SIZE;
Because xtensa uses PHYSADDR(CONFIG_SYS_SDRAM_BASE) for bi_memstart,
I was not able to replace it with gd->ram_base, as for all the other
arches.
Please note that bi_memstart/size is probably not used at all on
xtensa - only for the bdinfo cmd AFAICT.
Currently, gd->ram_base is defined in common/board_f.c, function
setup_dest_addr as:
#ifdef CONFIG_SYS_SDRAM_BASE
---gd->ram_base = CONFIG_SYS_SDRAM_BASE;
#endif
Yes, this #ifdef is ugly - I also noticed it while working on this
patchset.
So I think the PHYSADDR() logic needs to be preserved so that the
patchset would not change the memory start/end logic in
arch/xtensa/lib/bootm.c:
diff --git a/arch/xtensa/lib/bootm.c b/arch/xtensa/lib/bootm.c
index 458eaf95c0..6fcbfc4292 100644
--- a/arch/xtensa/lib/bootm.c
+++ b/arch/xtensa/lib/bootm.c
@@ -41,15 +41,14 @@ static struct bp_tag *setup_last_tag(struct
bp_tag *params)
static struct bp_tag *setup_memory_tag(struct bp_tag *params)
{
- struct bd_info *bd = gd->bd;
struct meminfo *mem;
params->id = BP_TAG_MEMORY;
params->size = sizeof(struct meminfo);
mem = (struct meminfo *)params->data;
mem->type = MEMORY_TYPE_CONVENTIONAL;
- mem->start = bd->bi_memstart;
- mem->end = bd->bi_memstart + bd->bi_memsize;
+ mem->start = gd->ram_base;
+ mem->end = gd->ram_base + gd->ram_size;
I see. Why not add this instead as well to this new patchset:
diff --git a/arch/xtensa/cpu/cpu.c b/arch/xtensa/cpu/cpu.c
index 85d3464607..a4ba71a301 100644
--- a/arch/xtensa/cpu/cpu.c
+++ b/arch/xtensa/cpu/cpu.c
@@ -45,6 +45,7 @@ int print_cpuinfo(void)
int arch_cpu_init(void)
{
+ gd->ram_base = PHYSADDR(CONFIG_SYS_SDRAM_BASE);
gd->ram_size = CONFIG_SYS_SDRAM_SIZE;
return 0;
}
and keep the bi_memstart -> ram_base conversion above? Looks more
consistant to me.
Thinking a bit more about this, its probably better to use this patch:
diff --git a/arch/xtensa/lib/bootm.c b/arch/xtensa/lib/bootm.c
index 6fcbfc4292..0e564507f9 100644
--- a/arch/xtensa/lib/bootm.c
+++ b/arch/xtensa/lib/bootm.c
@@ -47,8 +47,8 @@ static struct bp_tag *setup_memory_tag(struct bp_tag
*params)
params->size = sizeof(struct meminfo);
mem = (struct meminfo *)params->data;
mem->type = MEMORY_TYPE_CONVENTIONAL;
- mem->start = gd->ram_base;
- mem->end = gd->ram_base + gd->ram_size;
+ mem->start = PHYSADDR(gd->ram_base);
+ mem->end = PHYSADDR(gd->ram_base + gd->ram_size);
printf(" MEMORY: tag:0x%04x, type:0X%lx,
start:0X%lx, end:0X%lx\n",
BP_TAG_MEMORY, mem->type, mem->start, mem->end);
What do you think?
Yes, I think this is better. arch_cpu_init runs earlier than
setup_dest_addr in the boot process, so the gd->ram_base would have been
overwritten anyway if set there.
Ovidiu
Thanks,
Stefan
Re: [PATCH v4 5/9] xtensa: Remove arch_setup_bdinfo()
Hi Ovidiu,
On 13.08.20 10:09, Stefan Roese wrote:
Hi Ovidiu,
On 13.08.20 09:57, Ovidiu Panait wrote:
Hi Stefan,
On 13.08.2020 08:47, Stefan Roese wrote:
arch_setup_bdinfo() only configures the deprecated bi_memstart &
bi_memsize values, which should not be needed any more. Lets remove
this file completely.
Signed-off-by: Stefan Roese
---
Changes in v4:
- New patch
arch/xtensa/lib/Makefile | 2 +-
arch/xtensa/lib/bdinfo.c | 22 --
2 files changed, 1 insertion(+), 23 deletions(-)
delete mode 100644 arch/xtensa/lib/bdinfo.c
diff --git a/arch/xtensa/lib/Makefile b/arch/xtensa/lib/Makefile
index ceee59b9bd..c59df7d372 100644
--- a/arch/xtensa/lib/Makefile
+++ b/arch/xtensa/lib/Makefile
@@ -5,4 +5,4 @@
obj-$(CONFIG_CMD_BOOTM) += bootm.o
-obj-y += cache.o misc.o relocate.o time.o bdinfo.o
+obj-y += cache.o misc.o relocate.o time.o
diff --git a/arch/xtensa/lib/bdinfo.c b/arch/xtensa/lib/bdinfo.c
deleted file mode 100644
index 4ec8529521..00
--- a/arch/xtensa/lib/bdinfo.c
+++ /dev/null
@@ -1,22 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0+
-/*
- * XTENSA-specific information for the 'bd' command
- *
- * (C) Copyright 2003
- * Wolfgang Denk, DENX Software Engineering, w...@denx.de.
- */
-
-#include
-#include
-
-DECLARE_GLOBAL_DATA_PTR;
-
-int arch_setup_bdinfo(void)
-{
- struct bd_info *bd = gd->bd;
-
- bd->bi_memstart = PHYSADDR(CONFIG_SYS_SDRAM_BASE);
- bd->bi_memsize = CONFIG_SYS_SDRAM_SIZE;
-
When I did the setup_bdinfo refactoring, I realized that xtensa was
the only arch handling bi_{memstart,memsize} in a special way:
bd->bi_memstart = PHYSADDR(CONFIG_SYS_SDRAM_BASE);
bd->bi_memsize = CONFIG_SYS_SDRAM_SIZE;
Because xtensa uses PHYSADDR(CONFIG_SYS_SDRAM_BASE) for bi_memstart, I
was not able to replace it with gd->ram_base, as for all the other
arches.
Please note that bi_memstart/size is probably not used at all on
xtensa - only for the bdinfo cmd AFAICT.
Currently, gd->ram_base is defined in common/board_f.c, function
setup_dest_addr as:
#ifdef CONFIG_SYS_SDRAM_BASE
---gd->ram_base = CONFIG_SYS_SDRAM_BASE;
#endif
Yes, this #ifdef is ugly - I also noticed it while working on this
patchset.
So I think the PHYSADDR() logic needs to be preserved so that the
patchset would not change the memory start/end logic in
arch/xtensa/lib/bootm.c:
diff --git a/arch/xtensa/lib/bootm.c b/arch/xtensa/lib/bootm.c
index 458eaf95c0..6fcbfc4292 100644
--- a/arch/xtensa/lib/bootm.c
+++ b/arch/xtensa/lib/bootm.c
@@ -41,15 +41,14 @@ static struct bp_tag *setup_last_tag(struct bp_tag
*params)
static struct bp_tag *setup_memory_tag(struct bp_tag *params)
{
- struct bd_info *bd = gd->bd;
struct meminfo *mem;
params->id = BP_TAG_MEMORY;
params->size = sizeof(struct meminfo);
mem = (struct meminfo *)params->data;
mem->type = MEMORY_TYPE_CONVENTIONAL;
- mem->start = bd->bi_memstart;
- mem->end = bd->bi_memstart + bd->bi_memsize;
+ mem->start = gd->ram_base;
+ mem->end = gd->ram_base + gd->ram_size;
I see. Why not add this instead as well to this new patchset:
diff --git a/arch/xtensa/cpu/cpu.c b/arch/xtensa/cpu/cpu.c
index 85d3464607..a4ba71a301 100644
--- a/arch/xtensa/cpu/cpu.c
+++ b/arch/xtensa/cpu/cpu.c
@@ -45,6 +45,7 @@ int print_cpuinfo(void)
int arch_cpu_init(void)
{
+ gd->ram_base = PHYSADDR(CONFIG_SYS_SDRAM_BASE);
gd->ram_size = CONFIG_SYS_SDRAM_SIZE;
return 0;
}
and keep the bi_memstart -> ram_base conversion above? Looks more
consistant to me.
Thinking a bit more about this, its probably better to use this patch:
diff --git a/arch/xtensa/lib/bootm.c b/arch/xtensa/lib/bootm.c
index 6fcbfc4292..0e564507f9 100644
--- a/arch/xtensa/lib/bootm.c
+++ b/arch/xtensa/lib/bootm.c
@@ -47,8 +47,8 @@ static struct bp_tag *setup_memory_tag(struct bp_tag
*params)
params->size = sizeof(struct meminfo);
mem = (struct meminfo *)params->data;
mem->type = MEMORY_TYPE_CONVENTIONAL;
- mem->start = gd->ram_base;
- mem->end = gd->ram_base + gd->ram_size;
+ mem->start = PHYSADDR(gd->ram_base);
+ mem->end = PHYSADDR(gd->ram_base + gd->ram_size);
printf(" MEMORY: tag:0x%04x, type:0X%lx,
start:0X%lx, end:0X%lx\n",
BP_TAG_MEMORY, mem->type, mem->start, mem->end);
What do you think?
Thanks,
Stefan
Re: [PATCH v4 5/9] xtensa: Remove arch_setup_bdinfo()
Hi Ovidiu,
On 13.08.20 09:57, Ovidiu Panait wrote:
Hi Stefan,
On 13.08.2020 08:47, Stefan Roese wrote:
arch_setup_bdinfo() only configures the deprecated bi_memstart &
bi_memsize values, which should not be needed any more. Lets remove
this file completely.
Signed-off-by: Stefan Roese
---
Changes in v4:
- New patch
arch/xtensa/lib/Makefile | 2 +-
arch/xtensa/lib/bdinfo.c | 22 --
2 files changed, 1 insertion(+), 23 deletions(-)
delete mode 100644 arch/xtensa/lib/bdinfo.c
diff --git a/arch/xtensa/lib/Makefile b/arch/xtensa/lib/Makefile
index ceee59b9bd..c59df7d372 100644
--- a/arch/xtensa/lib/Makefile
+++ b/arch/xtensa/lib/Makefile
@@ -5,4 +5,4 @@
obj-$(CONFIG_CMD_BOOTM) += bootm.o
-obj-y += cache.o misc.o relocate.o time.o bdinfo.o
+obj-y += cache.o misc.o relocate.o time.o
diff --git a/arch/xtensa/lib/bdinfo.c b/arch/xtensa/lib/bdinfo.c
deleted file mode 100644
index 4ec8529521..00
--- a/arch/xtensa/lib/bdinfo.c
+++ /dev/null
@@ -1,22 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0+
-/*
- * XTENSA-specific information for the 'bd' command
- *
- * (C) Copyright 2003
- * Wolfgang Denk, DENX Software Engineering, w...@denx.de.
- */
-
-#include
-#include
-
-DECLARE_GLOBAL_DATA_PTR;
-
-int arch_setup_bdinfo(void)
-{
- struct bd_info *bd = gd->bd;
-
- bd->bi_memstart = PHYSADDR(CONFIG_SYS_SDRAM_BASE);
- bd->bi_memsize = CONFIG_SYS_SDRAM_SIZE;
-
When I did the setup_bdinfo refactoring, I realized that xtensa was the
only arch handling bi_{memstart,memsize} in a special way:
bd->bi_memstart = PHYSADDR(CONFIG_SYS_SDRAM_BASE);
bd->bi_memsize = CONFIG_SYS_SDRAM_SIZE;
Because xtensa uses PHYSADDR(CONFIG_SYS_SDRAM_BASE) for bi_memstart, I
was not able to replace it with gd->ram_base, as for all the other arches.
Please note that bi_memstart/size is probably not used at all on
xtensa - only for the bdinfo cmd AFAICT.
Currently, gd->ram_base is defined in common/board_f.c, function
setup_dest_addr as:
#ifdef CONFIG_SYS_SDRAM_BASE
---gd->ram_base = CONFIG_SYS_SDRAM_BASE;
#endif
Yes, this #ifdef is ugly - I also noticed it while working on this
patchset.
So I think the PHYSADDR() logic needs to be preserved so that the
patchset would not change the memory start/end logic in
arch/xtensa/lib/bootm.c:
diff --git a/arch/xtensa/lib/bootm.c b/arch/xtensa/lib/bootm.c
index 458eaf95c0..6fcbfc4292 100644
--- a/arch/xtensa/lib/bootm.c
+++ b/arch/xtensa/lib/bootm.c
@@ -41,15 +41,14 @@ static struct bp_tag *setup_last_tag(struct bp_tag
*params)
static struct bp_tag *setup_memory_tag(struct bp_tag *params)
{
- struct bd_info *bd = gd->bd;
struct meminfo *mem;
params->id = BP_TAG_MEMORY;
params->size = sizeof(struct meminfo);
mem = (struct meminfo *)params->data;
mem->type = MEMORY_TYPE_CONVENTIONAL;
- mem->start = bd->bi_memstart;
- mem->end = bd->bi_memstart + bd->bi_memsize;
+ mem->start = gd->ram_base;
+ mem->end = gd->ram_base + gd->ram_size;
I see. Why not add this instead as well to this new patchset:
diff --git a/arch/xtensa/cpu/cpu.c b/arch/xtensa/cpu/cpu.c
index 85d3464607..a4ba71a301 100644
--- a/arch/xtensa/cpu/cpu.c
+++ b/arch/xtensa/cpu/cpu.c
@@ -45,6 +45,7 @@ int print_cpuinfo(void)
int arch_cpu_init(void)
{
+ gd->ram_base = PHYSADDR(CONFIG_SYS_SDRAM_BASE);
gd->ram_size = CONFIG_SYS_SDRAM_SIZE;
return 0;
}
and keep the bi_memstart -> ram_base conversion above? Looks more
consistant to me.
printf(" MEMORY: tag:0x%04x, type:0X%lx, start:0X%lx,
end:0X%lx\n",
BP_TAG_MEMORY, mem->type, mem->start, mem->end);
Also, the only xtensa board (board/cadence/xtfpga/xtfpga.c) has an empty
stub for dram_init_banksize, overwriting the weak definition in
common/board_f.c that should populate gd->bd->bi_dram[0].start/size. But
maybe keeping gd->bd->bi_dram[0].start/size undefined does not have
other implications.
I just stumbled over this issue with the "test.py xtfpga" CI test,
which fails, since now bi_dram[].size is zero. I'll remove the local
dram_init_banksize() no-op function in the next patchset version, so
that the weak default will be used instead.
Thanks,
Stefan
Re: [PATCH v4 5/9] xtensa: Remove arch_setup_bdinfo()
Hi Stefan,
On 13.08.2020 08:47, Stefan Roese wrote:
arch_setup_bdinfo() only configures the deprecated bi_memstart &
bi_memsize values, which should not be needed any more. Lets remove
this file completely.
Signed-off-by: Stefan Roese
---
Changes in v4:
- New patch
arch/xtensa/lib/Makefile | 2 +-
arch/xtensa/lib/bdinfo.c | 22 --
2 files changed, 1 insertion(+), 23 deletions(-)
delete mode 100644 arch/xtensa/lib/bdinfo.c
diff --git a/arch/xtensa/lib/Makefile b/arch/xtensa/lib/Makefile
index ceee59b9bd..c59df7d372 100644
--- a/arch/xtensa/lib/Makefile
+++ b/arch/xtensa/lib/Makefile
@@ -5,4 +5,4 @@
obj-$(CONFIG_CMD_BOOTM) += bootm.o
-obj-y += cache.o misc.o relocate.o time.o bdinfo.o
+obj-y += cache.o misc.o relocate.o time.o
diff --git a/arch/xtensa/lib/bdinfo.c b/arch/xtensa/lib/bdinfo.c
deleted file mode 100644
index 4ec8529521..00
--- a/arch/xtensa/lib/bdinfo.c
+++ /dev/null
@@ -1,22 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0+
-/*
- * XTENSA-specific information for the 'bd' command
- *
- * (C) Copyright 2003
- * Wolfgang Denk, DENX Software Engineering, w...@denx.de.
- */
-
-#include
-#include
-
-DECLARE_GLOBAL_DATA_PTR;
-
-int arch_setup_bdinfo(void)
-{
- struct bd_info *bd = gd->bd;
-
- bd->bi_memstart = PHYSADDR(CONFIG_SYS_SDRAM_BASE);
- bd->bi_memsize = CONFIG_SYS_SDRAM_SIZE;
-
When I did the setup_bdinfo refactoring, I realized that xtensa was the
only arch handling bi_{memstart,memsize} in a special way:
bd->bi_memstart = PHYSADDR(CONFIG_SYS_SDRAM_BASE);
bd->bi_memsize = CONFIG_SYS_SDRAM_SIZE;
Because xtensa uses PHYSADDR(CONFIG_SYS_SDRAM_BASE) for bi_memstart, I was not
able to replace it with gd->ram_base, as for all the other arches.
Currently, gd->ram_base is defined in common/board_f.c, function
setup_dest_addr as:
#ifdef CONFIG_SYS_SDRAM_BASE
---gd->ram_base = CONFIG_SYS_SDRAM_BASE;
#endif
So I think the PHYSADDR() logic needs to be preserved so that the
patchset would not change the memory start/end logic in
arch/xtensa/lib/bootm.c:
diff --git a/arch/xtensa/lib/bootm.c b/arch/xtensa/lib/bootm.c
index 458eaf95c0..6fcbfc4292 100644
--- a/arch/xtensa/lib/bootm.c
+++ b/arch/xtensa/lib/bootm.c
@@ -41,15 +41,14 @@ static struct bp_tag *setup_last_tag(struct bp_tag *params)
static struct bp_tag *setup_memory_tag(struct bp_tag *params)
{
- struct bd_info *bd = gd->bd;
struct meminfo *mem;
params->id = BP_TAG_MEMORY;
params->size = sizeof(struct meminfo);
mem = (struct meminfo *)params->data;
mem->type = MEMORY_TYPE_CONVENTIONAL;
- mem->start = bd->bi_memstart;
- mem->end = bd->bi_memstart + bd->bi_memsize;
+ mem->start = gd->ram_base;
+ mem->end = gd->ram_base + gd->ram_size;
printf(" MEMORY: tag:0x%04x, type:0X%lx, start:0X%lx, end:0X%lx\n",
BP_TAG_MEMORY, mem->type, mem->start, mem->end);
Also, the only xtensa board (board/cadence/xtfpga/xtfpga.c) has an empty
stub for dram_init_banksize, overwriting the weak definition in
common/board_f.c that should populate gd->bd->bi_dram[0].start/size. But
maybe keeping gd->bd->bi_dram[0].start/size undefined does not have
other implications.
Ovidiu
- return 0;
-}
[PATCH v4 5/9] xtensa: Remove arch_setup_bdinfo()
arch_setup_bdinfo() only configures the deprecated bi_memstart &
bi_memsize values, which should not be needed any more. Lets remove
this file completely.
Signed-off-by: Stefan Roese
---
Changes in v4:
- New patch
arch/xtensa/lib/Makefile | 2 +-
arch/xtensa/lib/bdinfo.c | 22 --
2 files changed, 1 insertion(+), 23 deletions(-)
delete mode 100644 arch/xtensa/lib/bdinfo.c
diff --git a/arch/xtensa/lib/Makefile b/arch/xtensa/lib/Makefile
index ceee59b9bd..c59df7d372 100644
--- a/arch/xtensa/lib/Makefile
+++ b/arch/xtensa/lib/Makefile
@@ -5,4 +5,4 @@
obj-$(CONFIG_CMD_BOOTM) += bootm.o
-obj-y += cache.o misc.o relocate.o time.o bdinfo.o
+obj-y += cache.o misc.o relocate.o time.o
diff --git a/arch/xtensa/lib/bdinfo.c b/arch/xtensa/lib/bdinfo.c
deleted file mode 100644
index 4ec8529521..00
--- a/arch/xtensa/lib/bdinfo.c
+++ /dev/null
@@ -1,22 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0+
-/*
- * XTENSA-specific information for the 'bd' command
- *
- * (C) Copyright 2003
- * Wolfgang Denk, DENX Software Engineering, w...@denx.de.
- */
-
-#include
-#include
-
-DECLARE_GLOBAL_DATA_PTR;
-
-int arch_setup_bdinfo(void)
-{
- struct bd_info *bd = gd->bd;
-
- bd->bi_memstart = PHYSADDR(CONFIG_SYS_SDRAM_BASE);
- bd->bi_memsize = CONFIG_SYS_SDRAM_SIZE;
-
- return 0;
-}
--
2.28.0
