Hi Ley Foon,
> -----Original Message-----
> From: Ley Foon Tan <lftan.li...@gmail.com>
> Sent: Friday, 14 May, 2021 5:13 PM
> To: Lim, Elly Siew Chin <elly.siew.chin....@intel.com>
> Cc: ZY - u-boot <u-boot@lists.denx.de>; Marek Vasut <ma...@denx.de>;
> Tan, Ley Foon <ley.foon....@intel.com>; See, Chin Liang
> <chin.liang....@intel.com>; Simon Goldschmidt
> <simon.k.r.goldschm...@gmail.com>; Chee, Tien Fong
> <tien.fong.c...@intel.com>; Westergreen, Dalon
> <dalon.westergr...@intel.com>; Simon Glass <s...@chromium.org>; Gan,
> Yau Wai <yau.wai....@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....@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....@intel.com>
> > Signed-off-by: Tien Fong Chee <tien.fong.c...@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);
> > - else
> > - debug(" 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);
> > + else
> > + debug(" 0x%08x\n", *table_x32);
> > + }
> > + debug("\n}\n");
> > +
> > return 0;
> > }
>
> Regards
> Ley Foon
