Hi Adrian, > -----Original Message----- > From: Manish Narani > Sent: Wednesday, February 21, 2018 11:39 AM > To: Adrian Hunter <adrian.hunter@xxxxxxxxx>; michal.simek@xxxxxxxxxx; > ulf.hansson@xxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux- > mmc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > devicetree@xxxxxxxxxxxxxxx; mark.rutland@xxxxxxx; robh+dt@xxxxxxxxxx > Cc: Anirudha Sarangi <anirudh@xxxxxxxxxx>; Srinivas Goud > <sgoud@xxxxxxxxxx> > Subject: RE: [RFC PATCH] mmc: sdhci-of-arasan: Add auto tuning support for > ZynqMP Platform > > Hi Adrian, > > > > -----Original Message----- > > From: Adrian Hunter [mailto:adrian.hunter@xxxxxxxxx] > > Sent: Friday, February 16, 2018 7:37 PM > > To: Manish Narani <MNARANI@xxxxxxxxxx>; michal.simek@xxxxxxxxxx; > > ulf.hansson@xxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux- > > mmc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > > devicetree@xxxxxxxxxxxxxxx; mark.rutland@xxxxxxx; robh+dt@xxxxxxxxxx > > Cc: Anirudha Sarangi <anirudh@xxxxxxxxxx>; Srinivas Goud > > <sgoud@xxxxxxxxxx>; Manish Narani <MNARANI@xxxxxxxxxx> > > Subject: Re: [RFC PATCH] mmc: sdhci-of-arasan: Add auto tuning support > > for ZynqMP Platform > > > > On 30/01/18 20:14, Manish Narani wrote: > > > This patch adds support of SD auto tuning for ZynqMP platform. Auto > > > tuning sequence sends tuning block to card when operating in UHS-1 > > > modes. This resets the DLL and sends CMD19/CMD21 as a part of the > > > auto tuning process. Once the auto tuning process gets completed, > > > reset the DLL to load the newly obtained SDHC tuned tap value. > > > > How is this different from: > > 1. reset the dll > > 2. call sdhci_execute_tuning > > 3. reset the dll > > Below is my take on your above comments: - 'Reset the dll' is a platform specific call inside 'arasan_zynqmp_execute_tuning' which is implemented in sdhci-of-arasan.c - 'arasan_zynqmp_execute_tuning' is called from 'sdhci_execute_tuning' as a platform_execute_tuning routine - So to keep 'DLL reset' routine called from sdhci-of-arasan.c, I have implemented the execute_tuning in sdhci-of-arasan.c Alternative way (Please review): - Define a host->quirk2 bit (SDHCI_QUIRK2_DLL_RESET_NEEDED) in sdhci-of-arasan.c indicating DLL reset needed while tuning operation - Call 'dll reset' routine before and after __sdhci_execute_tuning() in sdhci.c when a host->quirk2 bit (SDHCI_QUIRK2_DLL_RESET_NEEDED) is set Thanks, Manish > Thanks for your comments. I am looking into this. I will check and let you > know on the same. > > Thanks, > - Manish > > > > > > > Signed-off-by: Manish Narani <mnarani@xxxxxxxxxx> > > > --- > > > .../devicetree/bindings/mmc/arasan,sdhci.txt | 1 + > > > drivers/mmc/host/sdhci-of-arasan.c | 219 > > ++++++++++++++++++++- > > > 2 files changed, 219 insertions(+), 1 deletion(-) > > > > > > diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt > > > b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt > > > index 60481bf..7d29751 100644 > > > --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt > > > +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt > > > @@ -14,6 +14,7 @@ Required Properties: > > > - "arasan,sdhci-4.9a": generic Arasan SDHCI 4.9a PHY > > > - "arasan,sdhci-5.1": generic Arasan SDHCI 5.1 PHY > > > - "rockchip,rk3399-sdhci-5.1", "arasan,sdhci-5.1": rk3399 eMMC > > > PHY > > > + - "xlnx,zynqmp-8.9a": Xilinx ZynqMP 8.9a PHY > > > For this device it is strongly suggested to include arasan,soc-ctl- > syscon. > > > - reg: From mmc bindings: Register location and length. > > > - clocks: From clock bindings: Handles to clock inputs. > > > diff --git a/drivers/mmc/host/sdhci-of-arasan.c > > > b/drivers/mmc/host/sdhci-of-arasan.c > > > index 0720ea7..7673db4 100644 > > > --- a/drivers/mmc/host/sdhci-of-arasan.c > > > +++ b/drivers/mmc/host/sdhci-of-arasan.c > > > @@ -24,15 +24,18 @@ > > > #include <linux/module.h> > > > #include <linux/of_device.h> > > > #include <linux/phy/phy.h> > > > +#include <linux/mmc/mmc.h> > > > #include <linux/regmap.h> > > > #include "sdhci-pltfm.h" > > > #include <linux/of.h> > > > +#include <linux/firmware/xilinx/zynqmp/firmware.h> > > > > > > #define SDHCI_ARASAN_VENDOR_REGISTER 0x78 > > > > > > #define VENDOR_ENHANCED_STROBE BIT(0) > > > > > > #define PHY_CLK_TOO_SLOW_HZ 400000 > > > +#define MAX_TUNING_LOOP 40 > > > > > > /* > > > * On some SoCs the syscon area has a feature where the upper > > > 16-bits of @@ -88,6 +91,7 @@ struct sdhci_arasan_data { > > > struct sdhci_host *host; > > > struct clk *clk_ahb; > > > struct phy *phy; > > > + u32 device_id; > > > bool is_phy_on; > > > > > > struct clk_hw sdcardclk_hw; > > > @@ -157,6 +161,213 @@ static int sdhci_arasan_syscon_write(struct > > sdhci_host *host, > > > return ret; > > > } > > > > > > +/** > > > + * arasan_zynqmp_dll_reset - Issue the DLL reset. > > > + * @deviceid: Unique Id of device > > > + */ > > > +void zynqmp_dll_reset(u8 deviceid) > > > +{ > > > + const struct zynqmp_eemi_ops *eemi_ops = get_eemi_ops(); > > > + > > > + if (!eemi_ops || !eemi_ops->ioctl) > > > + return; > > > + > > > + /* Issue DLL Reset */ > > > + if (deviceid == 0) > > > + eemi_ops->ioctl(NODE_SD_0, IOCTL_SD_DLL_RESET, > > > + PM_DLL_RESET_PULSE, 0, NULL); > > > + else > > > + eemi_ops->ioctl(NODE_SD_1, IOCTL_SD_DLL_RESET, > > > + PM_DLL_RESET_PULSE, 0, NULL); } > > > + > > > +static void arasan_zynqmp_dll_reset(struct sdhci_host *host, u8 > > > +deviceid) { > > > + u16 clk; > > > + unsigned long timeout; > > > + > > > + clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL); > > > + clk &= ~(SDHCI_CLOCK_CARD_EN | SDHCI_CLOCK_INT_EN); > > > + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); > > > + > > > + /* Issue DLL Reset */ > > > + zynqmp_dll_reset(deviceid); > > > + > > > + clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL); > > > + clk |= SDHCI_CLOCK_INT_EN; > > > + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); > > > + > > > + /* Wait max 20 ms */ > > > + timeout = 20; > > > + 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; > > > + } > > > + timeout--; > > > + mdelay(1); > > > + } > > > + > > > + clk |= SDHCI_CLOCK_CARD_EN; > > > + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); } > > > + > > > +static int arasan_zynqmp_execute_tuning(struct sdhci_host *host, > > > +u32 > > > +opcode) { > > > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > > > + struct sdhci_arasan_data *sdhci_arasan = > > sdhci_pltfm_priv(pltfm_host); > > > + struct mmc_host *mmc = host->mmc; > > > + u16 ctrl; > > > + int tuning_loop_counter = MAX_TUNING_LOOP; > > > + int err = 0; > > > + unsigned long flags; > > > + unsigned int tuning_count = 0; > > > + > > > + spin_lock_irqsave(&host->lock, flags); > > > + > > > + if (host->tuning_mode == SDHCI_TUNING_MODE_1) > > > + tuning_count = host->tuning_count; > > > + > > > + ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2); > > > + ctrl |= SDHCI_CTRL_EXEC_TUNING; > > > + if (host->quirks2 & SDHCI_QUIRK2_TUNING_WORK_AROUND) > > > + ctrl |= SDHCI_CTRL_TUNED_CLK; > > > + sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2); > > > + > > > + mdelay(1); > > > + > > > + arasan_zynqmp_dll_reset(host, sdhci_arasan->device_id); > > > + > > > + /* > > > + * As per the Host Controller spec v3.00, tuning command > > > + * generates Buffer Read Ready interrupt, so enable that. > > > + * > > > + * Note: The spec clearly says that when tuning sequence > > > + * is being performed, the controller does not generate > > > + * interrupts other than Buffer Read Ready interrupt. But > > > + * to make sure we don't hit a controller bug, we _only_ > > > + * enable Buffer Read Ready interrupt here. > > > + */ > > > + sdhci_writel(host, SDHCI_INT_DATA_AVAIL, SDHCI_INT_ENABLE); > > > + sdhci_writel(host, SDHCI_INT_DATA_AVAIL, > > > + SDHCI_SIGNAL_ENABLE); > > > + > > > + /* > > > + * Issue CMD19 repeatedly till Execute Tuning is set to 0 or the > number > > > + * of loops reaches 40 times or a timeout of 150ms occurs. > > > + */ > > > + do { > > > + struct mmc_command cmd = {0}; > > > + struct mmc_request mrq = {NULL}; > > > + > > > + cmd.opcode = opcode; > > > + cmd.arg = 0; > > > + cmd.flags = MMC_RSP_R1 | MMC_CMD_ADTC; > > > + cmd.retries = 0; > > > + cmd.data = NULL; > > > + cmd.mrq = &mrq; > > > + cmd.error = 0; > > > + > > > + if (tuning_loop_counter-- == 0) > > > + break; > > > + > > > + mrq.cmd = &cmd; > > > + > > > + /* > > > + * In response to CMD19, the card sends 64 bytes of tuning > > > + * block to the Host Controller. So we set the block size > > > + * to 64 here. > > > + */ > > > + if (cmd.opcode == MMC_SEND_TUNING_BLOCK_HS200) { > > > + if (mmc->ios.bus_width == MMC_BUS_WIDTH_8) { > > > + sdhci_writew(host, SDHCI_MAKE_BLKSZ(7, 128), > > > + SDHCI_BLOCK_SIZE); > > > + } else if (mmc->ios.bus_width == MMC_BUS_WIDTH_4) { > > > + sdhci_writew(host, SDHCI_MAKE_BLKSZ(7, 64), > > > + SDHCI_BLOCK_SIZE); > > > + } > > > + } else { > > > + sdhci_writew(host, SDHCI_MAKE_BLKSZ(7, 64), > > > + SDHCI_BLOCK_SIZE); > > > + } > > > + > > > + /* > > > + * The tuning block is sent by the card to the host controller. > > > + * So we set the TRNS_READ bit in the Transfer Mode register. > > > + * This also takes care of setting DMA Enable and Multi Block > > > + * Select in the same register to 0. > > > + */ > > > + sdhci_writew(host, SDHCI_TRNS_READ, > > > + SDHCI_TRANSFER_MODE); > > > + > > > + sdhci_send_command(host, &cmd); > > > + > > > + host->cmd = NULL; > > > + > > > + spin_unlock_irqrestore(&host->lock, flags); > > > + /* Wait for Buffer Read Ready interrupt */ > > > + wait_event_interruptible_timeout(host->buf_ready_int, > > > + (host->tuning_done == 1), > > > + msecs_to_jiffies(50)); > > > + spin_lock_irqsave(&host->lock, flags); > > > + > > > + if (!host->tuning_done) { > > > + dev_warn(mmc_dev(host->mmc), > > > + ": Timeout for Buffer Read Ready > > > + interrupt, back to fixed > > sampling clock\n"); > > > + ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2); > > > + ctrl &= ~SDHCI_CTRL_TUNED_CLK; > > > + ctrl &= ~SDHCI_CTRL_EXEC_TUNING; > > > + sdhci_writew(host, ctrl, > > > + SDHCI_HOST_CONTROL2); > > > + > > > + err = -EIO; > > > + goto out; > > > + } > > > + > > > + host->tuning_done = 0; > > > + > > > + ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2); > > > + > > > + /* eMMC spec does not require a delay between tuning cycles > */ > > > + if (opcode == MMC_SEND_TUNING_BLOCK) > > > + mdelay(1); > > > + } while (ctrl & SDHCI_CTRL_EXEC_TUNING); > > > + > > > + /* > > > + * The Host Driver has exhausted the maximum number of loops > > allowed, > > > + * so use fixed sampling frequency. > > > + */ > > > + if (tuning_loop_counter < 0) { > > > + ctrl &= ~SDHCI_CTRL_TUNED_CLK; > > > + sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2); > > > + } > > > + if (!(ctrl & SDHCI_CTRL_TUNED_CLK)) { > > > + dev_warn(mmc_dev(host->mmc), > > > + ": Tuning failed, back to fixed sampling clock\n"); > > > + err = -EIO; > > > + } else { > > > + arasan_zynqmp_dll_reset(host, sdhci_arasan->device_id); > > > + } > > > + > > > +out: > > > + /* > > > + * In case tuning fails, host controllers which support > > > + * re-tuning can try tuning again at a later time, when the > > > + * re-tuning timer expires. So for these controllers, we > > > + * return 0. Since there might be other controllers who do not > > > + * have this capability, we return error for them. > > > + */ > > > + if (tuning_count) > > > + err = 0; > > > + > > > + host->mmc->retune_period = err ? 0 : tuning_count; > > > + > > > + sdhci_writel(host, host->ier, SDHCI_INT_ENABLE); > > > + sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE); > > > + spin_unlock_irqrestore(&host->lock, flags); > > > + > > > + return err; > > > +} > > > + > > > static void sdhci_arasan_set_clock(struct sdhci_host *host, > > > unsigned int clock) { > > > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); @@ > > > -262,7 +473,7 @@ static int sdhci_arasan_voltage_switch(struct > > > mmc_host > > *mmc, > > > return -EINVAL; > > > } > > > > > > -static const struct sdhci_ops sdhci_arasan_ops = { > > > +static struct sdhci_ops sdhci_arasan_ops = { > > > .set_clock = sdhci_arasan_set_clock, > > > .get_max_clock = sdhci_pltfm_clk_get_max_clock, > > > .get_timeout_clock = sdhci_pltfm_clk_get_max_clock, @@ > > > -371,6 > > > +582,7 @@ static const struct of_device_id sdhci_arasan_of_match[] = > > > +{ > > > { .compatible = "arasan,sdhci-8.9a" }, > > > { .compatible = "arasan,sdhci-5.1" }, > > > { .compatible = "arasan,sdhci-4.9a" }, > > > + { .compatible = "xlnx,zynqmp-8.9a" }, > > > > > > { /* sentinel */ } > > > }; > > > @@ -642,6 +854,11 @@ static int sdhci_arasan_probe(struct > > platform_device *pdev) > > > goto unreg_clk; > > > } > > > > > > + if (of_device_is_compatible(pdev->dev.of_node, "xlnx,zynqmp- > > 8.9a")) { > > > + sdhci_arasan_ops.platform_execute_tuning = > > > + arasan_zynqmp_execute_tuning; > > > + } > > > + > > > sdhci_arasan->phy = ERR_PTR(-ENODEV); > > > if (of_device_is_compatible(pdev->dev.of_node, > > > "arasan,sdhci-5.1")) { > > > -- > > > 2.7.4 > > > > > > This email and any attachments are intended for the sole use of the > > > named > > recipient(s) and contain(s) confidential information that may be > > proprietary, privileged or copyrighted under applicable law. If you > > are not the intended recipient, do not read, copy, or forward this > > email message or any attachments. Delete this email message and any > attachments immediately. > > > ��.n��������+%������w��{.n�����{��i��)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
