RE: [RFC PATCH] mmc: sdhci-of-arasan: Add auto tuning support for ZynqMP Platform

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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�����٥




[Index of Archives]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux