[PATCH 1/4] microblaze: drop CONFIG_SYS_INIT_RAM_ADDR and CONFIG_SYS_INIT_RAM_SIZE

[PATCH 1/4] microblaze: drop CONFIG_SYS_INIT_RAM_ADDR and CONFIG_SYS_INIT_RAM_SIZE

[Ovidiu Panait]

These macros don't seem to be used by microblaze code anymore, so remove
them.

Signed-off-by: Ovidiu Panait <ovpanait@gmail.com>
---

 include/configs/microblaze-generic.h | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/include/configs/microblaze-generic.h b/include/configs/microblaze-generic.h
index 8eaac4f8bc..dfae8cea7b 100644
--- a/include/configs/microblaze-generic.h
+++ b/include/configs/microblaze-generic.h
@@ -97,10 +97,4 @@
 
 #define CONFIG_SYS_UBOOT_BASE		CONFIG_SYS_TEXT_BASE
 
-/* SP location before relocation, must use scratch RAM */
-/* BRAM start */
-#define CONFIG_SYS_INIT_RAM_ADDR	0x0
-/* BRAM size - will be generated */
-#define CONFIG_SYS_INIT_RAM_SIZE	0x100000
-
 #endif	/* __CONFIG_H */
-- 
2.25.1

[PATCH 2/4] cpu: microblaze: remove unused ret variable

[Ovidiu Panait]

Drop the unused ret variable from microblaze_cpu_get_desc().

Signed-off-by: Ovidiu Panait <ovpanait@gmail.com>
---

 drivers/cpu/microblaze_cpu.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/cpu/microblaze_cpu.c b/drivers/cpu/microblaze_cpu.c
index 969a1047e5..4eae06a8a6 100644
--- a/drivers/cpu/microblaze_cpu.c
+++ b/drivers/cpu/microblaze_cpu.c
@@ -88,15 +88,14 @@ static int microblaze_cpu_get_desc(const struct udevice *dev, char *buf,
 	struct microblaze_cpuinfo *ci = gd_cpuinfo();
 	const char *cpu_ver, *fpga_family;
 	u32 cpu_freq_mhz;
-	int ret;
 
 	cpu_freq_mhz = ci->cpu_freq / 1000000;
 	cpu_ver = microblaze_lookup_cpu_version_string(ci->ver_code);
 	fpga_family = microblaze_lookup_fpga_family_string(ci->fpga_code);
 
-	ret = snprintf(buf, size,
-		       "MicroBlaze @ %uMHz, Rev: %s, FPGA family: %s",
-		       cpu_freq_mhz, cpu_ver, fpga_family);
+	snprintf(buf, size,
+		 "MicroBlaze @ %uMHz, Rev: %s, FPGA family: %s",
+		 cpu_freq_mhz, cpu_ver, fpga_family);
 
 	return 0;
 }
-- 
2.25.1

Re: [PATCH 2/4] cpu: microblaze: remove unused ret variable

[Michal Simek]

On 8/25/22 08:41, Ovidiu Panait wrote:
> Drop the unused ret variable from microblaze_cpu_get_desc().
> 
> Signed-off-by: Ovidiu Panait <ovpanait@gmail.com>
> ---
> 
>   drivers/cpu/microblaze_cpu.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cpu/microblaze_cpu.c b/drivers/cpu/microblaze_cpu.c
> index 969a1047e5..4eae06a8a6 100644
> --- a/drivers/cpu/microblaze_cpu.c
> +++ b/drivers/cpu/microblaze_cpu.c
> @@ -88,15 +88,14 @@ static int microblaze_cpu_get_desc(const struct udevice *dev, char *buf,
>   	struct microblaze_cpuinfo *ci = gd_cpuinfo();
>   	const char *cpu_ver, *fpga_family;
>   	u32 cpu_freq_mhz;
> -	int ret;
>   
>   	cpu_freq_mhz = ci->cpu_freq / 1000000;
>   	cpu_ver = microblaze_lookup_cpu_version_string(ci->ver_code);
>   	fpga_family = microblaze_lookup_fpga_family_string(ci->fpga_code);
>   
> -	ret = snprintf(buf, size,
> -		       "MicroBlaze @ %uMHz, Rev: %s, FPGA family: %s",
> -		       cpu_freq_mhz, cpu_ver, fpga_family);
> +	snprintf(buf, size,
> +		 "MicroBlaze @ %uMHz, Rev: %s, FPGA family: %s",
> +		 cpu_freq_mhz, cpu_ver, fpga_family);
>   
>   	return 0;
>   }

First of all I think you can remove DECLARE_GLOBAL_DATA_PTR from 
drivers/cpu/microblaze_cpu.c

I looked at the code and I think that there are some things what should happen.
First of all I would memset by 0 that buf which is passed and I think this 
should be done in uclass to make sure that you won't show anything what it is on 
the stack where buf is allocated in cmd/cpu.c for example.

The second if snprintf fails we shouldn't ignore that error because if you do 
that that means that buffer is valid and you can print content.

For cpu_freq_mhz I think that make sense to allocate some space in string. For 
example %4u gives you exact size GHz should be fine for now. cpu_ver has max 
size 6 now and family string has 18 chars now.


It means change string to this with some chars on the top should be fine for me.
	ret = snprintf(buf, size,
		 "MicroBlaze @ %5uMHz, Rev: %7s, FPGA family: %20s",
		 cpu_freq_mhz, cpu_ver, fpga_family);


And then check that ret is equal XX size would be easy for checking and 
returning 0 if they match.

Thanks,
Michal

Re: [PATCH 2/4] cpu: microblaze: remove unused ret variable

[Ovidiu Panait]

Hi Michal,

On 8/25/22 11:59, Michal Simek wrote:
>
>
> On 8/25/22 08:41, Ovidiu Panait wrote:
>> Drop the unused ret variable from microblaze_cpu_get_desc().
>>
>> Signed-off-by: Ovidiu Panait <ovpanait@gmail.com>
>> ---
>>
>>   drivers/cpu/microblaze_cpu.c | 7 +++----
>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/cpu/microblaze_cpu.c b/drivers/cpu/microblaze_cpu.c
>> index 969a1047e5..4eae06a8a6 100644
>> --- a/drivers/cpu/microblaze_cpu.c
>> +++ b/drivers/cpu/microblaze_cpu.c
>> @@ -88,15 +88,14 @@ static int microblaze_cpu_get_desc(const struct 
>> udevice *dev, char *buf,
>>       struct microblaze_cpuinfo *ci = gd_cpuinfo();
>>       const char *cpu_ver, *fpga_family;
>>       u32 cpu_freq_mhz;
>> -    int ret;
>>         cpu_freq_mhz = ci->cpu_freq / 1000000;
>>       cpu_ver = microblaze_lookup_cpu_version_string(ci->ver_code);
>>       fpga_family = microblaze_lookup_fpga_family_string(ci->fpga_code);
>>   -    ret = snprintf(buf, size,
>> -               "MicroBlaze @ %uMHz, Rev: %s, FPGA family: %s",
>> -               cpu_freq_mhz, cpu_ver, fpga_family);
>> +    snprintf(buf, size,
>> +         "MicroBlaze @ %uMHz, Rev: %s, FPGA family: %s",
>> +         cpu_freq_mhz, cpu_ver, fpga_family);
>>         return 0;
>>   }
>
> First of all I think you can remove DECLARE_GLOBAL_DATA_PTR from 
> drivers/cpu/microblaze_cpu.c
>
gd_cpuinfo() macro expands to "(struct microblaze_cpuinfo 
*)&gd->arch.cpuinfo", so we need the declaration for gd.


> I looked at the code and I think that there are some things what 
> should happen.
> First of all I would memset by 0 that buf which is passed and I think 
> this should be done in uclass to make sure that you won't show 
> anything what it is on the stack where buf is allocated in cmd/cpu.c 
> for example.
>
I don't think that the memset is necessary, snprintf will overwrite any 
previous contents and append the terminating null character after the 
last byte that was written to the buffer. So no garbage previously 
present on the stack should be printed when buf is used afterwards, as 
the string is null-terminated.


> The second if snprintf fails we shouldn't ignore that error because if 
> you do that that means that buffer is valid and you can print content.
>
> For cpu_freq_mhz I think that make sense to allocate some space in 
> string. For example %4u gives you exact size GHz should be fine for 
> now. cpu_ver has max size 6 now and family string has 18 chars now.
>
>
> It means change string to this with some chars on the top should be 
> fine for me.
>     ret = snprintf(buf, size,
>          "MicroBlaze @ %5uMHz, Rev: %7s, FPGA family: %20s",
>          cpu_freq_mhz, cpu_ver, fpga_family);
>
>
> And then check that ret is equal XX size would be easy for checking 
> and returning 0 if they match.

I agree, the snprintf errors should be handled properly here.


Thanks,

Ovidiu

>
> Thanks,
> Michal
>
>

[PATCH 3/4] cmd: bdinfo: introduce bdinfo_print_size() helper

[Ovidiu Panait]

Add bdinfo_print_size() helper to display size variables (such as cache
sizes) in bdinfo format. The size is printed as "xxx Bytes", "xxx KiB",
"xxx MiB", "xxx GiB", etc as needed;

Signed-off-by: Ovidiu Panait <ovpanait@gmail.com>
---

 cmd/bdinfo.c   |  7 +++++++
 include/init.h | 13 +++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/cmd/bdinfo.c b/cmd/bdinfo.c
index 37cd8a57eb..9e23c4dd8f 100644
--- a/cmd/bdinfo.c
+++ b/cmd/bdinfo.c
@@ -16,9 +16,16 @@
 #include <vsprintf.h>
 #include <asm/cache.h>
 #include <asm/global_data.h>
+#include <display_options.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
+void bdinfo_print_size(const char *name, uint64_t size)
+{
+	printf("%-12s= ", name);
+	print_size(size, "\n");
+}
+
 void bdinfo_print_num_l(const char *name, ulong value)
 {
 	printf("%-12s= 0x%0*lx\n", name, 2 * (int)sizeof(value), value);
diff --git a/include/init.h b/include/init.h
index 7b8f62c121..02bb4ce13e 100644
--- a/include/init.h
+++ b/include/init.h
@@ -343,6 +343,19 @@ void bdinfo_print_num_ll(const char *name, unsigned long long value);
 /* Print a clock speed in MHz */
 void bdinfo_print_mhz(const char *name, unsigned long hz);
 
+/**
+ * bdinfo_print_size - print size variables in bdinfo format
+ * @name:	string to print before the size
+ * @size:	size to print
+ *
+ * Helper function for displaying size variables as properly formatted bdinfo
+ * entries. The size is printed as "xxx Bytes", "xxx KiB", "xxx MiB",
+ * "xxx GiB", etc. as needed;
+ *
+ * For use in arch_print_bdinfo().
+ */
+void bdinfo_print_size(const char *name, uint64_t size);
+
 /* Show arch-specific information for the 'bd' command */
 void arch_print_bdinfo(void);
 
-- 
2.25.1

Re: [PATCH 3/4] cmd: bdinfo: introduce bdinfo_print_size() helper

[Michal Simek]

čt 25. 8. 2022 v 8:42 odesílatel Ovidiu Panait <ovpanait@gmail.com> napsal:
>
> Add bdinfo_print_size() helper to display size variables (such as cache
> sizes) in bdinfo format. The size is printed as "xxx Bytes", "xxx KiB",
> "xxx MiB", "xxx GiB", etc as needed;
>
> Signed-off-by: Ovidiu Panait <ovpanait@gmail.com>
> ---
>
>  cmd/bdinfo.c   |  7 +++++++
>  include/init.h | 13 +++++++++++++
>  2 files changed, 20 insertions(+)
>
> diff --git a/cmd/bdinfo.c b/cmd/bdinfo.c
> index 37cd8a57eb..9e23c4dd8f 100644
> --- a/cmd/bdinfo.c
> +++ b/cmd/bdinfo.c
> @@ -16,9 +16,16 @@
>  #include <vsprintf.h>
>  #include <asm/cache.h>
>  #include <asm/global_data.h>
> +#include <display_options.h>
>
>  DECLARE_GLOBAL_DATA_PTR;
>
> +void bdinfo_print_size(const char *name, uint64_t size)
> +{
> +       printf("%-12s= ", name);
> +       print_size(size, "\n");
> +}
> +
>  void bdinfo_print_num_l(const char *name, ulong value)
>  {
>         printf("%-12s= 0x%0*lx\n", name, 2 * (int)sizeof(value), value);
> diff --git a/include/init.h b/include/init.h
> index 7b8f62c121..02bb4ce13e 100644
> --- a/include/init.h
> +++ b/include/init.h
> @@ -343,6 +343,19 @@ void bdinfo_print_num_ll(const char *name, unsigned long long value);
>  /* Print a clock speed in MHz */
>  void bdinfo_print_mhz(const char *name, unsigned long hz);
>
> +/**
> + * bdinfo_print_size - print size variables in bdinfo format
> + * @name:      string to print before the size
> + * @size:      size to print
> + *
> + * Helper function for displaying size variables as properly formatted bdinfo
> + * entries. The size is printed as "xxx Bytes", "xxx KiB", "xxx MiB",
> + * "xxx GiB", etc. as needed;
> + *
> + * For use in arch_print_bdinfo().
> + */
> +void bdinfo_print_size(const char *name, uint64_t size);
> +
>  /* Show arch-specific information for the 'bd' command */
>  void arch_print_bdinfo(void);
>
> --
> 2.25.1
>

No problem with this code. If there is any problem you can include it
directly to mb implementation but
make sense to have it in generic location.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs

Re: [PATCH 3/4] cmd: bdinfo: introduce bdinfo_print_size() helper

[Simon Glass]

On Thu, 25 Aug 2022 at 00:41, Ovidiu Panait <ovpanait@gmail.com> wrote:
>
> Add bdinfo_print_size() helper to display size variables (such as cache
> sizes) in bdinfo format. The size is printed as "xxx Bytes", "xxx KiB",
> "xxx MiB", "xxx GiB", etc as needed;
>
> Signed-off-by: Ovidiu Panait <ovpanait@gmail.com>
> ---
>
>  cmd/bdinfo.c   |  7 +++++++
>  include/init.h | 13 +++++++++++++
>  2 files changed, 20 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 4/4] microblaze: add arch_print_bdinfo() implementation

[Ovidiu Panait]

Allow bdinfo command to print icache/dcache information:
U-Boot-mONStR> bdinfo
boot_params = 0x00000000
DRAM bank   = 0x00000000
-> start    = 0x04000000
-> size     = 0x04000000
flashstart  = 0x00000000
flashsize   = 0x00000000
flashoffset = 0x00000000
baudrate    = 9600 bps
relocaddr   = 0x07f76000
reloc off   = 0x02f76000
Build       = 32-bit
current eth = unknown
ethaddr     = (not set)
IP addr     = <NULL>
fdt_blob    = 0x07fec7e0
new_fdt     = 0x00000000
fdt_size    = 0x00000000
lmb_dump_all:
 memory.cnt  = 0x1
 memory[0]      [0x4000000-0x7ffffff], 0x04000000 bytes flags: 0
 reserved.cnt  = 0x1
 reserved[0]    [0x7e94b8c-0x7ffffff], 0x0016b474 bytes flags: 0
devicetree  = embed
icache      = 32 KiB
icache line = 4 Bytes
dcache      = 32 KiB
dcache line = 4 Bytes

Signed-off-by: Ovidiu Panait <ovpanait@gmail.com>
---

 arch/microblaze/lib/Makefile |  1 +
 arch/microblaze/lib/bdinfo.c | 24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+)
 create mode 100644 arch/microblaze/lib/bdinfo.c

diff --git a/arch/microblaze/lib/Makefile b/arch/microblaze/lib/Makefile
index 05f447abba..dfd8135f4f 100644
--- a/arch/microblaze/lib/Makefile
+++ b/arch/microblaze/lib/Makefile
@@ -4,4 +4,5 @@
 # Wolfgang Denk, DENX Software Engineering, wd@denx.de.
 
 obj-$(CONFIG_CMD_BOOTM) += bootm.o
+obj-$(CONFIG_CMD_BDI) += bdinfo.o
 obj-y	+= muldi3.o
diff --git a/arch/microblaze/lib/bdinfo.c b/arch/microblaze/lib/bdinfo.c
new file mode 100644
index 0000000000..41b7a216a4
--- /dev/null
+++ b/arch/microblaze/lib/bdinfo.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2022, Ovidiu Panait <ovpanait@gmail.com>
+ */
+#include <init.h>
+#include <asm/cpuinfo.h>
+#include <asm/global_data.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+void arch_print_bdinfo(void)
+{
+	struct microblaze_cpuinfo *ci = gd_cpuinfo();
+
+	if (ci->icache_size) {
+		bdinfo_print_size("icache", ci->icache_size);
+		bdinfo_print_size("icache line", ci->icache_line_length);
+	}
+
+	if (ci->dcache_size) {
+		bdinfo_print_size("dcache", ci->dcache_size);
+		bdinfo_print_size("dcache line", ci->dcache_line_length);
+	}
+}
-- 
2.25.1

Re: [PATCH 4/4] microblaze: add arch_print_bdinfo() implementation

[Michal Simek]

On 8/25/22 08:41, Ovidiu Panait wrote:
> Allow bdinfo command to print icache/dcache information:
> U-Boot-mONStR> bdinfo
> boot_params = 0x00000000
> DRAM bank   = 0x00000000
> -> start    = 0x04000000
> -> size     = 0x04000000
> flashstart  = 0x00000000
> flashsize   = 0x00000000
> flashoffset = 0x00000000
> baudrate    = 9600 bps
> relocaddr   = 0x07f76000
> reloc off   = 0x02f76000
> Build       = 32-bit
> current eth = unknown
> ethaddr     = (not set)
> IP addr     = <NULL>
> fdt_blob    = 0x07fec7e0
> new_fdt     = 0x00000000
> fdt_size    = 0x00000000
> lmb_dump_all:
>   memory.cnt  = 0x1
>   memory[0]      [0x4000000-0x7ffffff], 0x04000000 bytes flags: 0
>   reserved.cnt  = 0x1
>   reserved[0]    [0x7e94b8c-0x7ffffff], 0x0016b474 bytes flags: 0
> devicetree  = embed
> icache      = 32 KiB
> icache line = 4 Bytes
> dcache      = 32 KiB
> dcache line = 4 Bytes
> 
> Signed-off-by: Ovidiu Panait <ovpanait@gmail.com>
> ---
> 
>   arch/microblaze/lib/Makefile |  1 +
>   arch/microblaze/lib/bdinfo.c | 24 ++++++++++++++++++++++++
>   2 files changed, 25 insertions(+)
>   create mode 100644 arch/microblaze/lib/bdinfo.c
> 
> diff --git a/arch/microblaze/lib/Makefile b/arch/microblaze/lib/Makefile
> index 05f447abba..dfd8135f4f 100644
> --- a/arch/microblaze/lib/Makefile
> +++ b/arch/microblaze/lib/Makefile
> @@ -4,4 +4,5 @@
>   # Wolfgang Denk, DENX Software Engineering, wd@denx.de.
>   
>   obj-$(CONFIG_CMD_BOOTM) += bootm.o
> +obj-$(CONFIG_CMD_BDI) += bdinfo.o
>   obj-y	+= muldi3.o
> diff --git a/arch/microblaze/lib/bdinfo.c b/arch/microblaze/lib/bdinfo.c
> new file mode 100644
> index 0000000000..41b7a216a4
> --- /dev/null
> +++ b/arch/microblaze/lib/bdinfo.c
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2022, Ovidiu Panait <ovpanait@gmail.com>
> + */
> +#include <init.h>
> +#include <asm/cpuinfo.h>
> +#include <asm/global_data.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;

remove this line. You are not using gd in this function.

> +
> +void arch_print_bdinfo(void)
> +{
> +	struct microblaze_cpuinfo *ci = gd_cpuinfo();
> +
> +	if (ci->icache_size) {
> +		bdinfo_print_size("icache", ci->icache_size);
> +		bdinfo_print_size("icache line", ci->icache_line_length);
> +	}
> +
> +	if (ci->dcache_size) {
> +		bdinfo_print_size("dcache", ci->dcache_size);
> +		bdinfo_print_size("dcache line", ci->dcache_line_length);
> +	}
> +}

The rest looks good to me.

M

Re: [PATCH 1/4] microblaze: drop CONFIG_SYS_INIT_RAM_ADDR and CONFIG_SYS_INIT_RAM_SIZE

[Michal Simek]

Hi,

On 8/25/22 08:41, Ovidiu Panait wrote:
> These macros don't seem to be used by microblaze code anymore, so remove
> them.

the patch itself is correct but I would prefer if you can provide more details 
in commit message.

I tracked it down and that macros should be removed as the part of this commit.

Fixes: 9b5f9aeb3b48 ("Convert CONFIG_SPL_BSS_MAX_SIZE et al to Kconfig")


Thanks,
Michal

> 
> Signed-off-by: Ovidiu Panait <ovpanait@gmail.com>
> ---
> 
>   include/configs/microblaze-generic.h | 6 ------
>   1 file changed, 6 deletions(-)
> 
> diff --git a/include/configs/microblaze-generic.h b/include/configs/microblaze-generic.h
> index 8eaac4f8bc..dfae8cea7b 100644
> --- a/include/configs/microblaze-generic.h
> +++ b/include/configs/microblaze-generic.h
> @@ -97,10 +97,4 @@
>   
>   #define CONFIG_SYS_UBOOT_BASE		CONFIG_SYS_TEXT_BASE
>   
> -/* SP location before relocation, must use scratch RAM */
> -/* BRAM start */
> -#define CONFIG_SYS_INIT_RAM_ADDR	0x0
> -/* BRAM size - will be generated */
> -#define CONFIG_SYS_INIT_RAM_SIZE	0x100000
> -
>   #endif	/* __CONFIG_H */