Hi Ley Foon,
-----Original Message----- From: Ley Foon Tan lftan.linux@gmail.com Sent: Friday, 14 May, 2021 5:13 PM To: Lim, Elly Siew Chin elly.siew.chin.lim@intel.com Cc: ZY - u-boot u-boot@lists.denx.de; Marek Vasut marex@denx.de; Tan, Ley Foon ley.foon.tan@intel.com; See, Chin Liang chin.liang.see@intel.com; Simon Goldschmidt simon.k.r.goldschmidt@gmail.com; Chee, Tien Fong tien.fong.chee@intel.com; Westergreen, Dalon dalon.westergreen@intel.com; Simon Glass sjg@chromium.org; Gan, Yau Wai yau.wai.gan@intel.com Subject: Re: [v2 04/17] arm: socfpga: Add handoff data support for Intel N5X device
On Fri, Apr 30, 2021 at 3:39 PM Siew Chin Lim elly.siew.chin.lim@intel.com wrote:
N5X support both HPS handoff data and DDR handoff data. Existing HPS handoff functions are restructured to support both existing devices and N5X device.
Signed-off-by: Siew Chin Lim elly.siew.chin.lim@intel.com Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com
v2:
- Enabled auto detect the endianness from the magic word
- Merged and simplifying the big and little endian flow
.../mach-socfpga/include/mach/handoff_soc64.h | 38 +++++- arch/arm/mach-socfpga/system_manager_soc64.c | 18 +-- arch/arm/mach-socfpga/wrap_handoff_soc64.c | 126 +++++++++++++--
3 files changed, 136 insertions(+), 46 deletions(-)
[...]
@@ -10,12 +10,54 @@ #include <errno.h> #include "log.h"
-int socfpga_get_handoff_size(void *handoff_address, enum endianness endian) +static enum endianness check_endianness(u32 handoff) {
switch (handoff) {case SOC64_HANDOFF_MAGIC_BOOT:case SOC64_HANDOFF_MAGIC_MUX:case SOC64_HANDOFF_MAGIC_IOCTL:case SOC64_HANDOFF_MAGIC_FPGA:case SOC64_HANDOFF_MAGIC_DELAY:case SOC64_HANDOFF_MAGIC_CLOCK:case SOC64_HANDOFF_MAGIC_MISC:return BIG_ENDIAN;+#if IS_ENABLED(CONFIG_TARGET_SOCFPGA_N5X)
case SOC64_HANDOFF_DDR_UMCTL2_MAGIC:debug("%s: umctl2 handoff data\n", __func__);return LITTLE_ENDIAN;case SOC64_HANDOFF_DDR_PHY_MAGIC:debug("%s: PHY handoff data\n", __func__);return LITTLE_ENDIAN;case SOC64_HANDOFF_DDR_PHY_INIT_ENGINE_MAGIC:debug("%s: PHY engine handoff data\n", __func__);return LITTLE_ENDIAN;Can merge to one 'return' and print the 'handoff' if needed.
I can merge to one 'return' but has to compromise accuracy of DDR handoff type debug print out. So, I don’t see any benefit of doing these. Do you have any suggestion?
+#endif
default:debug("%s: Unknown endianness!!\n", __func__);return UNKNOWN_ENDIANNESS;}+}
+int socfpga_get_handoff_size(void *handoff_address) { u32 size;
enum endianness endian_t;/* Checking handoff data is little endian ? */endian_t = check_endianness(readl(handoff_address));if (endian_t == UNKNOWN_ENDIANNESS) {/* Trying to check handoff data is big endian? */endian_t = check_endianness(swab32(readl(handoff_address)));if (endian_t == UNKNOWN_ENDIANNESS) {debug("%s: Cannot find HANDOFF MAGIC ", __func__);debug("at addr 0x%p\n", (u32 *)handoff_address);return -EPERM;}} size = readl(handoff_address + SOC64_HANDOFF_OFFSET_LENGTH);
if (endian == BIG_ENDIAN)
if (endian_t == BIG_ENDIAN) size = swab32(size); size = (size - SOC64_HANDOFF_OFFSET_DATA) / sizeof(u32); @@-26,41 +68,61 @@ int socfpga_get_handoff_size(void *handoff_address,
enum endianness endian)
return size;}
-int socfpga_handoff_read(void *handoff_address, void *table, u32
table_len,
enum endianness big_endian)+int socfpga_handoff_read(void *handoff_address, void *table, u32 +table_len) {
u32 temp, i;
u32 temp; u32 *table_x32 = table;u32 i = 0;enum endianness endian_t;/* Checking handoff data is little endian ? */endian_t = check_endianness(readl(handoff_address));This code is similar in socfpga_get_handoff_size(). Can have a function to get the endianness.
Okay.
debug("%s: handoff addr = 0x%p ", __func__, (u32*)handoff_address);
if (big_endian) {if (swab32(readl(SOC64_HANDOFF_BASE)) ==SOC64_HANDOFF_MAGIC_BOOT) {
debug("Handoff table address = 0x%p ", table_x32);debug("table length = 0x%x\n", table_len);debug("%s: handoff data =\n{\n", __func__);for (i = 0; i < table_len; i++) {temp = readl(handoff_address +SOC64_HANDOFF_OFFSET_DATA +(i * sizeof(u32)));*table_x32 = swab32(temp);if (!(i % 2))debug(" No.%d Addr 0x%08x: ", i,*table_x32);elsedebug(" 0x%08x\n", *table_x32);table_x32++;}debug("\n}\n");} else {debug("%s: Cannot find SOC64_HANDOFF_MAGIC_BOOT ",__func__);
debug("at addr 0x%p\n", (u32 *)handoff_address);
if (endian_t == UNKNOWN_ENDIANNESS) {/* Trying to check handoff data is big endian? */endian_t = check_endianness(swab32(readl(handoff_address)));if (endian_t == UNKNOWN_ENDIANNESS) {debug("%s: Cannot find HANDOFF MAGIC ", __func__);debug("at addr 0x%p\n", (u32*)handoff_address); return -EPERM; } }
temp = readl(handoff_address + SOC64_HANDOFF_OFFSET_DATA +(i * sizeof(u32)));When this can't be done in the 'for' loop below?
The first iteration is moved out from below loop for supporting correct debug message format and also to avoid reading the same thing twice.
if (endian_t == BIG_ENDIAN) {debug("%s: Handoff addr = 0x%p ", __func__, (u32*)handoff_address);
debug("Handoff table address = 0x%p ", table_x32);debug("table length = 0x%x\n", table_len);debug("%s: handoff data =\n{\n", __func__);*table_x32 = swab32(temp);} else if (endian_t == LITTLE_ENDIAN) {debug(" {\n");*table_x32 = temp;}debug(" No.%d Addr 0x%08x: ", i, *table_x32);for (i = 1; i < table_len; i++) {table_x32++;temp = readl(handoff_address +SOC64_HANDOFF_OFFSET_DATA +(i * sizeof(u32)));if (endian_t == BIG_ENDIAN)*table_x32 = swab32(temp);else if (endian_t == LITTLE_ENDIAN)*table_x32 = temp;if (!(i % 2))debug(" No.%d Addr 0x%08x: ", i,*table_x32);elsedebug(" 0x%08x\n", *table_x32);}debug("\n}\n");return 0;}
Regards Ley Foon