Hi,
On 7/24/21 5:10 PM, Ashok Reddy Soma wrote:
> Currently xilinx sdhci driver is using zynqmp_mmio_write() to set
> tapdelay values. Use xilinx_pm_request() using appropriate arguments
> to set input/output tapdelays for zynqmp. Where tapdelay setting is
> done by firmware. Host driver should explicitly request DLL reset
> before ITAP (assert DLL) and after OTAP (release DLL) to avoid issues
> in some cases. Also handle error return where possible.
>
> Signed-off-by: T Karthik Reddy <t.karthik.re...@xilinx.com>
> Signed-off-by: Ashok Reddy Soma <ashok.reddy.s...@xilinx.com>
> ---
>
> board/xilinx/zynqmp/tap_delays.c | 89 +++-----------------------------
> drivers/mmc/zynq_sdhci.c | 72 +++++++++++++++++++++-----
> include/zynqmp_tap_delay.h | 25 ++++++---
> 3 files changed, 84 insertions(+), 102 deletions(-)
Doen it impossible to separate the board specific patch and mmc patch?
>
> diff --git a/board/xilinx/zynqmp/tap_delays.c
> b/board/xilinx/zynqmp/tap_delays.c
> index d16bbb8eff..4ce2244060 100644
> --- a/board/xilinx/zynqmp/tap_delays.c
> +++ b/board/xilinx/zynqmp/tap_delays.c
> @@ -10,92 +10,17 @@
> #include <asm/arch/sys_proto.h>
> #include <linux/delay.h>
> #include <mmc.h>
> +#include <zynqmp_firmware.h>
>
> -#define SD_DLL_CTRL 0xFF180358
> -#define SD_ITAP_DLY 0xFF180314
> -#define SD_OTAP_DLY 0xFF180318
> -#define SD0_DLL_RST_MASK 0x00000004
> -#define SD0_DLL_RST 0x00000004
> -#define SD1_DLL_RST_MASK 0x00040000
> -#define SD1_DLL_RST 0x00040000
> -#define SD0_ITAPCHGWIN_MASK 0x00000200
> -#define SD0_ITAPCHGWIN 0x00000200
> -#define SD1_ITAPCHGWIN_MASK 0x02000000
> -#define SD1_ITAPCHGWIN 0x02000000
> -#define SD0_ITAPDLYENA_MASK 0x00000100
> -#define SD0_ITAPDLYENA 0x00000100
> -#define SD1_ITAPDLYENA_MASK 0x01000000
> -#define SD1_ITAPDLYENA 0x01000000
> -#define SD0_ITAPDLYSEL_MASK 0x000000FF
> -#define SD1_ITAPDLYSEL_MASK 0x00FF0000
> -#define SD0_OTAPDLYSEL_MASK 0x0000003F
> -#define SD1_OTAPDLYSEL_MASK 0x003F0000
> -
> -void zynqmp_dll_reset(u8 deviceid)
> +int arasan_zynqmp_set_in_tapdelay(u8 deviceid, u32 type, u32 itap_delay)
> {
> - /* Issue DLL Reset */
> - if (deviceid == 0)
> - zynqmp_mmio_write(SD_DLL_CTRL, SD0_DLL_RST_MASK,
> - SD0_DLL_RST);
> - else
> - zynqmp_mmio_write(SD_DLL_CTRL, SD1_DLL_RST_MASK,
> - SD1_DLL_RST);
> -
> - mdelay(1);
>
> - /* Release DLL Reset */
> - if (deviceid == 0)
> - zynqmp_mmio_write(SD_DLL_CTRL, SD0_DLL_RST_MASK, 0x0);
> - else
> - zynqmp_mmio_write(SD_DLL_CTRL, SD1_DLL_RST_MASK, 0x0);
> + return xilinx_pm_request(PM_IOCTL, (u32)deviceid, IOCTL_SET_SD_TAPDELAY,
> + type, itap_delay, NULL);
> }
>
> -void arasan_zynqmp_set_in_tapdelay(u8 deviceid, u32 itap_delay)
> +int arasan_zynqmp_set_out_tapdelay(u8 deviceid, u32 type, u32 otap_delay)
> {
> - if (deviceid == 0) {
> - zynqmp_mmio_write(SD_DLL_CTRL, SD0_DLL_RST_MASK, SD0_DLL_RST);
> -
> - /* Program ITAP delay */
> - zynqmp_mmio_write(SD_ITAP_DLY, SD0_ITAPCHGWIN_MASK,
> - SD0_ITAPCHGWIN);
> - zynqmp_mmio_write(SD_ITAP_DLY, SD0_ITAPDLYENA_MASK,
> - SD0_ITAPDLYENA);
> - zynqmp_mmio_write(SD_ITAP_DLY, SD0_ITAPDLYSEL_MASK, itap_delay);
> - zynqmp_mmio_write(SD_ITAP_DLY, SD0_ITAPCHGWIN_MASK, 0x0);
> -
> - zynqmp_mmio_write(SD_DLL_CTRL, SD0_DLL_RST_MASK, 0x0);
> - } else {
> - zynqmp_mmio_write(SD_DLL_CTRL, SD1_DLL_RST_MASK, SD1_DLL_RST);
> -
> - /* Program ITAP delay */
> - zynqmp_mmio_write(SD_ITAP_DLY, SD1_ITAPCHGWIN_MASK,
> - SD1_ITAPCHGWIN);
> - zynqmp_mmio_write(SD_ITAP_DLY, SD1_ITAPDLYENA_MASK,
> - SD1_ITAPDLYENA);
> - zynqmp_mmio_write(SD_ITAP_DLY, SD1_ITAPDLYSEL_MASK,
> - (itap_delay << 16));
> - zynqmp_mmio_write(SD_ITAP_DLY, SD1_ITAPCHGWIN_MASK, 0x0);
> -
> - zynqmp_mmio_write(SD_DLL_CTRL, SD1_DLL_RST_MASK, 0x0);
> - }
> -}
> -
> -void arasan_zynqmp_set_out_tapdelay(u8 deviceid, u32 otap_delay)
> -{
> - if (deviceid == 0) {
> - zynqmp_mmio_write(SD_DLL_CTRL, SD0_DLL_RST_MASK, SD0_DLL_RST);
> -
> - /* Program OTAP delay */
> - zynqmp_mmio_write(SD_OTAP_DLY, SD0_OTAPDLYSEL_MASK, otap_delay);
> -
> - zynqmp_mmio_write(SD_DLL_CTRL, SD0_DLL_RST_MASK, 0x0);
> - } else {
> - zynqmp_mmio_write(SD_DLL_CTRL, SD1_DLL_RST_MASK, SD1_DLL_RST);
> -
> - /* Program OTAP delay */
> - zynqmp_mmio_write(SD_OTAP_DLY, SD1_OTAPDLYSEL_MASK,
> - (otap_delay << 16));
> -
> - zynqmp_mmio_write(SD_DLL_CTRL, SD1_DLL_RST_MASK, 0x0);
> - }
> + return xilinx_pm_request(PM_IOCTL, (u32)deviceid, IOCTL_SET_SD_TAPDELAY,
> + type, otap_delay, NULL);
> }
> diff --git a/drivers/mmc/zynq_sdhci.c b/drivers/mmc/zynq_sdhci.c
> index 9fb3603c7e..4dafd47b57 100644
> --- a/drivers/mmc/zynq_sdhci.c
> +++ b/drivers/mmc/zynq_sdhci.c
> @@ -18,6 +18,7 @@
> #include <malloc.h>
> #include <sdhci.h>
> #include <zynqmp_tap_delay.h>
> +#include <zynqmp_firmware.h>
>
> #define SDHCI_ARASAN_ITAPDLY_REGISTER 0xF0F8
> #define SDHCI_ARASAN_ITAPDLY_SEL_MASK GENMASK(7, 0)
> @@ -75,26 +76,39 @@ static const u8 mode2timing[] = {
> [MMC_HS_200] = MMC_TIMING_MMC_HS200,
> };
>
> -static void arasan_zynqmp_dll_reset(struct sdhci_host *host, u8 deviceid)
> +static int arasan_zynqmp_dll_reset(struct sdhci_host *host, u8 deviceid)
> {
> - u16 clk;
> + struct mmc *mmc = (struct mmc *)host->mmc;
> + struct udevice *dev = mmc->dev;
> unsigned long timeout;
> + int ret;
> + u16 clk;
>
> clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> clk &= ~(SDHCI_CLOCK_CARD_EN);
> sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>
> /* Issue DLL Reset */
> - zynqmp_dll_reset(deviceid);
> + ret = zynqmp_pm_sd_dll_reset(deviceid, PM_DLL_RESET_ASSERT);
> + if (ret) {
> + dev_err(dev, "dll_reset assert failed with err: %d\n", ret);
> + return ret;
> + }
> +
> + mdelay(1);
If add some comments about this, it's more helpful.
> + ret = zynqmp_pm_sd_dll_reset(deviceid, PM_DLL_RESET_RELEASE);
> + if (ret) {
> + dev_err(dev, "dll_reset release failed with err: %d\n", ret);
> + return ret;
> + }
>
> /* Wait max 20 ms */
> timeout = 100;
> while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
> & SDHCI_CLOCK_INT_STABLE)) {
> if (timeout == 0) {
> - dev_err(mmc_dev(host->mmc),
> - ": Internal clock never stabilised.\n");
> - return;
> + dev_err(dev, ": Internal clock never stabilised.\n");
> + return -EBUSY;
> }
> timeout--;
> udelay(1000);
> @@ -102,6 +116,8 @@ static void arasan_zynqmp_dll_reset(struct sdhci_host
> *host, u8 deviceid)
>
> clk |= SDHCI_CLOCK_CARD_EN;
> sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
> +
> + return 0;
> }
>
> static int arasan_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
> @@ -109,15 +125,14 @@ static int arasan_sdhci_execute_tuning(struct mmc *mmc,
> u8 opcode)
> struct mmc_cmd cmd;
> struct mmc_data data;
> u32 ctrl;
> + u8 node_id = mmc->dev->seq_ ? NODE_SD_0 : NODE_SD_1;
Does it need to get a node_id value from mmc->dev->seq_ at everytime?
And priv->deviceid doesn't use anywhere. Can't it reuse?
Best Regards,
Jaehoon Chung
> struct sdhci_host *host;
> struct arasan_sdhci_priv *priv = dev_get_priv(mmc->dev);
> char tuning_loop_counter = SDHCI_TUNING_LOOP_COUNT;
> - u8 deviceid;
>
> debug("%s\n", __func__);
>
> host = priv->host;
> - deviceid = priv->deviceid;
>
> ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> ctrl |= SDHCI_CTRL_EXEC_TUNING;
> @@ -125,7 +140,7 @@ static int arasan_sdhci_execute_tuning(struct mmc *mmc,
> u8 opcode)
>
> mdelay(1);
>
> - arasan_zynqmp_dll_reset(host, deviceid);
> + arasan_zynqmp_dll_reset(host, node_id);
>
> sdhci_writel(host, SDHCI_INT_DATA_AVAIL, SDHCI_INT_ENABLE);
> sdhci_writel(host, SDHCI_INT_DATA_AVAIL, SDHCI_SIGNAL_ENABLE);
> @@ -171,7 +186,7 @@ static int arasan_sdhci_execute_tuning(struct mmc *mmc,
> u8 opcode)
> }
>
> udelay(1);
> - arasan_zynqmp_dll_reset(host, deviceid);
> + arasan_zynqmp_dll_reset(host, node_id);
>
> /* Enable only interrupts served by the SD controller */
> sdhci_writel(host, SDHCI_INT_DATA_MASK | SDHCI_INT_CMD_MASK,
> @@ -194,10 +209,12 @@ static int arasan_sdhci_execute_tuning(struct mmc *mmc,
> u8 opcode)
> static int sdhci_zynqmp_sdcardclk_set_phase(struct sdhci_host *host,
> int degrees)
> {
> - struct arasan_sdhci_priv *priv = dev_get_priv(host->mmc->dev);
> struct mmc *mmc = (struct mmc *)host->mmc;
> + struct udevice *dev = mmc->dev;
> + u8 node_id = dev->seq_ ? NODE_SD_0 : NODE_SD_1;
> u8 tap_delay, tap_max = 0;
> int timing = mode2timing[mmc->selected_mode];
> + int ret;
>
> /*
> * This is applicable for SDHCI_SPEC_300 and above
> @@ -233,7 +250,20 @@ static int sdhci_zynqmp_sdcardclk_set_phase(struct
> sdhci_host *host,
> /* Limit output tap_delay value to 6 bits */
> tap_delay &= SDHCI_ARASAN_OTAPDLY_SEL_MASK;
>
> - arasan_zynqmp_set_out_tapdelay(priv->deviceid, tap_delay);
> + /* Set the Clock Phase */
> + ret = arasan_zynqmp_set_out_tapdelay(node_id,
> + PM_TAPDELAY_OUTPUT, tap_delay);
> + if (ret) {
> + dev_err(dev, "Error setting output Tap Delay\n");
> + return ret;
> + }
> +
> + /* Release DLL Reset */
> + ret = zynqmp_pm_sd_dll_reset(node_id, PM_DLL_RESET_RELEASE);
> + if (ret) {
> + dev_err(dev, "dll_reset release failed with err: %d\n", ret);
> + return ret;
> + }
>
> return 0;
> }
> @@ -250,10 +280,12 @@ static int sdhci_zynqmp_sdcardclk_set_phase(struct
> sdhci_host *host,
> static int sdhci_zynqmp_sampleclk_set_phase(struct sdhci_host *host,
> int degrees)
> {
> - struct arasan_sdhci_priv *priv = dev_get_priv(host->mmc->dev);
> struct mmc *mmc = (struct mmc *)host->mmc;
> + struct udevice *dev = mmc->dev;
> + u8 node_id = dev->seq_ ? NODE_SD_0 : NODE_SD_1;
> u8 tap_delay, tap_max = 0;
> int timing = mode2timing[mmc->selected_mode];
> + int ret;
>
> /*
> * This is applicable for SDHCI_SPEC_300 and above
> @@ -263,6 +295,13 @@ static int sdhci_zynqmp_sampleclk_set_phase(struct
> sdhci_host *host,
> if (SDHCI_GET_VERSION(host) < SDHCI_SPEC_300)
> return 0;
>
> + /* Assert DLL Reset */
> + ret = zynqmp_pm_sd_dll_reset(node_id, PM_DLL_RESET_ASSERT);
> + if (ret) {
> + dev_err(dev, "dll_reset assert failed with err: %d\n", ret);
> + return ret;
> + }
> +
> switch (timing) {
> case MMC_TIMING_MMC_HS:
> case MMC_TIMING_SD_HS:
> @@ -289,7 +328,12 @@ static int sdhci_zynqmp_sampleclk_set_phase(struct
> sdhci_host *host,
> /* Limit input tap_delay value to 8 bits */
> tap_delay &= SDHCI_ARASAN_ITAPDLY_SEL_MASK;
>
> - arasan_zynqmp_set_in_tapdelay(priv->deviceid, tap_delay);
> + ret = arasan_zynqmp_set_in_tapdelay(node_id,
> + PM_TAPDELAY_INPUT, tap_delay);
> + if (ret) {
> + dev_err(dev, "Error setting Input Tap Delay\n");
> + return ret;
> + }
>
> return 0;
> }
> diff --git a/include/zynqmp_tap_delay.h b/include/zynqmp_tap_delay.h
> index 1c1e3e7dee..7e4d4e4a9a 100644
> --- a/include/zynqmp_tap_delay.h
> +++ b/include/zynqmp_tap_delay.h
> @@ -8,14 +8,27 @@
> #ifndef __ZYNQMP_TAP_DELAY_H__
> #define __ZYNQMP_TAP_DELAY_H__
>
> +#include <zynqmp_firmware.h>
> +
> #ifdef CONFIG_ARCH_ZYNQMP
> -void zynqmp_dll_reset(u8 deviceid);
> -void arasan_zynqmp_set_in_tapdelay(u8 device_id, u32 itap_delay);
> -void arasan_zynqmp_set_out_tapdelay(u8 device_id, u32 otap_delay);
> +int arasan_zynqmp_set_in_tapdelay(u8 device_id, u32 type, u32 itap_delay);
> +int arasan_zynqmp_set_out_tapdelay(u8 device_id, u32 type, u32 otap_delay);
> #else
> -inline void zynqmp_dll_reset(u8 deviceid) {}
> -inline void arasan_zynqmp_set_in_tapdelay(u8 device_id, u32 itap_delay) {}
> -inline void arasan_zynqmp_set_out_tapdelay(u8 device_id, u32 otap_delay) {}
> +inline int arasan_zynqmp_set_in_tapdelay(u8 device_id, u32 type, u32
> itap_delay)
> +{
> + return 0;
> +}
> +
> +int arasan_zynqmp_set_out_tapdelay(u8 device_id, u32 type, u32 otap_delay)
> +{
> + return 0;
> +}
> #endif
>
> +static inline int zynqmp_pm_sd_dll_reset(u8 node_id, u32 type)
> +{
> + return xilinx_pm_request(PM_IOCTL, (u32)node_id, IOCTL_SD_DLL_RESET,
> + type, 0, NULL);
> +}
> +
> #endif
>
