Hi, I have tested the latest Xilinx kernel based on v5.4 at github.com/Xilinx/linux-xlnx and I built it by toolchain you pointed me to and I can't see any issue. Driver has changed a little bit based on mainline discussion but handling these values is the same.
$ dmesg | grep gcc [ 0.000000] Linux version 5.4.0-09934-ge977da2d1aee-dirty (monstr@monstr-desktop3) (gcc version 9.2.1 20191025 (GNU Toolchain for the A-profile Architecture 9.2-2019.12 (arm-9.10))) #82 SMP Tue Mar 17 10:17:09 CET 2020 $ dmesg | grep arasan [ 15.416035] arasan_dt_parse_clk_phases:0-63-63-0-63 0-0-183-54-0 0 [ 15.422308] arasan_dt_parse_clk_phases:0-72-60-0-60 72-135-48-72-135 0 As I said I have no problem with changing code to look as you have below but I can't see any issue with gcc 9.2 in connection to this. Manish: Can you please take a look at patch below and create a mainline patch to follow this style if you consider this better that what you use now. Thanks, Michal On 10. 03. 20 15:41, Quanyang Wang wrote: > Hi Michal, > > As below is the detail steps to reproduce this issue in SDK kernel: > > 1). the aarch-none-linux-gnu-gcc v9.2 is here > > https://developer.arm.com/-/media/Files/downloads/gnu-a/9.2-2019.12/binrel/gcc-arm-9.2-2019.12-x86_64-aarch64-none-linux-gnu.tar.xz?revision=61c3be5d-5175-4db6-9030-b565aae9f766&la=en&hash=0A37024B42028A9616F56A51C2D20755C5EBBCD > > I use yocto 9.2.0 gcc can also reproduce this issue. > > 2). build xilinx 4.19 kernel with this gcc and boot *zcu102* board. > > The arasan_zynqmp_set_tap_delayj will use random values to initialize > the register of mmc controller. > > And when using the printk to print the values in itapdly, you will found > that the values are not ZYNQMP_ITAP_DELAYS but random values. > > This is the debug patch to print the value: > > $ git diff > diff --git a/drivers/mmc/host/sdhci-of-arasan.c > b/drivers/mmc/host/sdhci-of-arasan.c > index 2d38a4325fff..3c366456f703 100644 > --- a/drivers/mmc/host/sdhci-of-arasan.c > +++ b/drivers/mmc/host/sdhci-of-arasan.c > @@ -971,6 +971,7 @@ static void arasan_dt_parse_tap_delays(struct device > *dev) > otapdly = (u32 [MMC_TIMING_MMC_HS400 + 1]) > VERSAL_OTAP_DELAYS; > } > > + printk("-%x-%x-%x\r\n", itapdly[0], itapdly[1],itapdly[2]); > arasan_dt_read_tap_delay(dev, itapdly, MMC_TIMING_SD_HS, > "xlnx,itap-delay-sd-hsd"); > arasan_dt_read_tap_delay(dev, itapdly, MMC_TIMING_UHS_SDR25, > > > This is the print result before applying this patch: > > [ 5.505387] cdns-wdt fd4d0000.watchdog: Xilinx Watchdog Timer with > timeout 60s > *[ 5.514214] -2f0cdf00-9b4d5b9a-935ba90* > [ 5.549141] mmc0: SDHCI controller on ff170000.mmc [ff170000.mmc] > using ADMA 64-bit > [ 5.565127] input: gpio-keys as /devices/platform/gpio-keys/input/input0 > wg > > This is the print value after applying the patch > > [ 5.484797] cdns-wdt fd4d0000.watchdog: Xilinx Watchdog Timer with > timeout 60s > *[ 5.493622] -0-15-15* > [ 5.525589] mmc0: SDHCI controller on ff170000.mmc [ff170000.mmc] > using ADMA 64-bit > [ 5.541632] input: gpio-keys as /devices/platform/gpio-keys/input/input0 > > I don't know if this is a bug of GCC v9.2.0. But it seems that using > the array of integers is a more reasonable method. > > Thanks, > > Quanyang > > On 3/10/20 8:17 PM, Michal Simek wrote: >> + Manish, >> >> On 10. 03. 20 12:33, [email protected] wrote: >>> From: Quanyang Wang <[email protected]> >>> >>> When assigning value to a pointer as below: >>> >>> itapdly = (u32 [MMC_TIMING_MMC_HS400 + 1]) {0x0, 0x15, 0x15...0x0}; >>> >>> then using gcc 9.2.0 with -O2 to compile, the pointer itapdly will point to >>> the stack of the function, and the values in this area is not initialized. >>> The disassemble code is as below: >>> >>> ffffffc010a389b8: 940089ac bl ffffffc010a5b068 >>> <of_device_is_compatible> >>> ffffffc010a389bc: 34001100 cbz w0, ffffffc010a38bdc >>> <arasan_dt_parse_tap_delays+0x264> >>> itapdly = (u32 [MMC_TIMING_MMC_HS400 + 1]) ZYNQMP_ITAP_DELAYS; >>> otapdly = (u32 [MMC_TIMING_MMC_HS400 + 1]) ZYNQMP_OTAP_DELAYS; >>> ffffffc010a389c0: 9101d3f4 add x20, sp, #0x74 >>> itapdly = (u32 [MMC_TIMING_MMC_HS400 + 1]) ZYNQMP_ITAP_DELAYS; >>> ffffffc010a389c4: 910123f5 add x21, sp, #0x48 >>> int ret = of_property_read_variable_u32_array(np, propname, >>> out_values, >>> >>> This results that the pointer itapdly is not the address of the array >>> ZYNQMP_ITAP_DELAYS but a stack address which contains random values. >>> >>> So use an array of integers to avoid the over optimization of the new >>> compiler. >>> >>> Signed-off-by: Quanyang Wang <[email protected]> >>> -- >>> drivers/mmc/host/sdhci-of-arasan.c | 25 +++++++++++++------------ >>> 1 file changed, 13 insertions(+), 12 deletions(-) >>> --- >>> drivers/mmc/host/sdhci-of-arasan.c | 24 ++++++++++++------------ >>> 1 file changed, 12 insertions(+), 12 deletions(-) >> This look bit weird. > I will fix it in V2. >>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c >>> b/drivers/mmc/host/sdhci-of-arasan.c >>> index bc9ca6ae20ce..34dcded79b78 100644 >>> --- a/drivers/mmc/host/sdhci-of-arasan.c >>> +++ b/drivers/mmc/host/sdhci-of-arasan.c >>> @@ -52,15 +52,15 @@ >>> #define SDHCI_ITAPDLY_ENABLE 0x100 >>> #define SDHCI_OTAPDLY_ENABLE 0x40 >>> >>> -#define ZYNQMP_ITAP_DELAYS {0x0, 0x15, 0x15, 0x0, 0x15, 0x0,\ >>> - 0x0, 0x3D, 0x12, 0x0, 0x0} >>> -#define ZYNQMP_OTAP_DELAYS {0x0, 0x5, 0x6, 0x0, 0x5, 0x3,\ >>> - 0x3, 0x4, 0x6, 0x3, 0x0} >>> +static u32 zynqmp_itap_delays[MMC_TIMING_MMC_HS400+1] = {0x0, 0x15, 0x15, >>> 0x0, >> missing spaces around + > Will fix it in V2. >>> + 0x15, 0x0, 0x0, 0x3D, 0x12, 0x0, 0x0}; >>> +static u32 zynqmp_otap_delays[MMC_TIMING_MMC_HS400+1] = {0x0, 0x5, 0x6, >>> 0x0, >> ditto. > Will fix it in V2. >>> + 0x5, 0x3, 0x3, 0x4, 0x6, 0x3, 0x0}; >>> >>> -#define VERSAL_ITAP_DELAYS {0x0, 0x2C, 0x2C, 0x0, 0x2C, 0x0,\ >>> - 0x0, 0x36, 0x1E, 0x0, 0x0} >>> -#define VERSAL_OTAP_DELAYS {0x0, 0x5, 0x4, 0x0, 0x4, 0x3,\ >>> - 0x2, 0x3, 0x5, 0x2, 0x0} >>> +static u32 versal_itap_delays[MMC_TIMING_MMC_HS400 + 1] = {0x0, 0x2C, 0x2C, >>> + 0x0, 0x2C, 0x0, 0x0, 0x36, 0x1E, 0x0, 0x0}; >>> +static u32 versal_otap_delays[MMC_TIMING_MMC_HS400 + 1] = {0x0, 0x5, 0x4, >>> + 0x0, 0x4, 0x3, 0x2, 0x3, 0x5, 0x2, 0x0}; >>> >>> #define MMC_BANK2 0x2 >>> >>> @@ -995,15 +995,15 @@ static void arasan_dt_parse_tap_delays(struct device >>> *dev) >>> int i; >>> >>> if (of_device_is_compatible(pdev->dev.of_node, "xlnx,zynqmp-8.9a")) { >>> - itapdly = (u32 [MMC_TIMING_MMC_HS400 + 1]) ZYNQMP_ITAP_DELAYS; >>> - otapdly = (u32 [MMC_TIMING_MMC_HS400 + 1]) ZYNQMP_OTAP_DELAYS; >>> + itapdly = zynqmp_itap_delays; >>> + otapdly = zynqmp_otap_delays; >>> if (sdhci_arasan->mio_bank == MMC_BANK2) { >>> otapdly[MMC_TIMING_UHS_SDR104] = 0x2; >>> otapdly[MMC_TIMING_MMC_HS200] = 0x2; >>> } >>> } else { >>> - itapdly = (u32 [MMC_TIMING_MMC_HS400 + 1]) VERSAL_ITAP_DELAYS; >>> - otapdly = (u32 [MMC_TIMING_MMC_HS400 + 1]) VERSAL_OTAP_DELAYS; >>> + itapdly = versal_itap_delays; >>> + otapdly = versal_otap_delays; >>> } >>> >>> arasan_dt_read_tap_delay(dev, itapdly, MMC_TIMING_SD_HS, >>> >> Manish: Can you please take a look? >> >> I can't see any issue with this. >> >> Thanks, >> Michal
-=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#8505): https://lists.yoctoproject.org/g/linux-yocto/message/8505 Mute This Topic: https://lists.yoctoproject.org/mt/71854797/21656 Group Owner: [email protected] Unsubscribe: https://lists.yoctoproject.org/g/linux-yocto/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
