Hi Michal, On 2020/3/18 下午12:40, quanyang.wang wrote:
Sorry to bother you. I know why this should be changed. Please ignore my questions.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] arerandom.diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.cindex 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?
Thanks, Quanyang
This change avoid triggering the issue. Thanks, QuanyangDriver has changed a little bit based on mainline discussion buthandling 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 (#8520): https://lists.yoctoproject.org/g/linux-yocto/message/8520 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]] -=-=-=-=-=-=-=-=-=-=-=-
