Hi Michal,
On 2020/3/17 下午5:24, Michal Simek wrote:
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.
I find that the reason why this issue only exists in SDK v4.19 but not
in SDK v5.4 is because of the souce code section in
arasan_dt_parse_clk_phases:
for (i = 0; i <= MMC_TIMING_MMC_HS400; i++) {
clk_data->clk_phase_in[i] = iclk_phase[i];
clk_data->clk_phase_out[i] = oclk_phase[i];
}
Once I modify the source code as below, which is similar to v4.19, this
issue will occur in xilinx SDK v5.4 again, the values in iclk_phase[i]
and oclk_phase[i] are
random.
diff --git a/drivers/mmc/host/sdhci-of-arasan.c
b/drivers/mmc/host/sdhci-of-arasan.c
index 36d774eb8d0e..357fb05e5ab1 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -1143,21 +1143,16 @@ static void arasan_dt_parse_clk_phases(struct
device *dev,
oclk_phase[MMC_TIMING_UHS_SDR104] = 90;
oclk_phase[MMC_TIMING_MMC_HS200] = 90;
}
-
- for (i = 0; i <= MMC_TIMING_MMC_HS400; i++) {
- clk_data->clk_phase_in[i] = iclk_phase[i];
- clk_data->clk_phase_out[i] = oclk_phase[i];
- }
}
if (of_device_is_compatible(dev->of_node, "xlnx,versal-8.9a")) {
iclk_phase = (int [MMC_TIMING_MMC_HS400 + 1]) VERSAL_ICLK_PHASE;
oclk_phase = (int [MMC_TIMING_MMC_HS400 + 1]) VERSAL_OCLK_PHASE;
+ }
- for (i = 0; i <= MMC_TIMING_MMC_HS400; i++) {
- clk_data->clk_phase_in[i] = iclk_phase[i];
- clk_data->clk_phase_out[i] = oclk_phase[i];
- }
+ for (i = 0; i <= MMC_TIMING_MMC_HS400; i++) {
+ clk_data->clk_phase_in[i] = iclk_phase[i];
+ clk_data->clk_phase_out[i] = oclk_phase[i];
}
arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_LEGACY,
The pointers oclk_phase and iclk_phase are pointed to a stack address
again according to the disassemble code:
b3c: 34000b20 cbz w0, ca0 <arasan_dt_parse_clk_phases+0x1c0>
oclk_phase = (int [MMC_TIMING_MMC_HS400 + 1]) VERSAL_OCLK_PHASE;
b40: 9102f3e5 *add x5, sp, #0xbc*
iclk_phase = (int [MMC_TIMING_MMC_HS400 + 1]) VERSAL_ICLK_PHASE;
b44: 910243e4 *add x4, sp, #0x90*
In v4.19 this code section is out of two "if"s, but in v5.4, it is moved
into every "if".
Is there any special consideration about this change from v4.19 to v5.4?
This change avoid triggering the issue.
Thanks,
Quanyang
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 (#8519):
https://lists.yoctoproject.org/g/linux-yocto/message/8519
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]]
-=-=-=-=-=-=-=-=-=-=-=-
