RE: [PATCH 4/6] mmc: host: sdhci-esdhc-imx.c: correct the auto-tuning setting for sdio device

[Bough Chen <haibo.chen@nxp.com>]

> -----Original Message-----
> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> Sent: 2021年8月17日 16:01
> To: Bough Chen <haibo.chen@nxp.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>; Shawn Guo
> <shawnguo@kernel.org>; Rob Herring <robh+dt@kernel.org>; Sascha Hauer
> <s.hauer@pengutronix.de>; Sascha Hauer <kernel@pengutronix.de>; Fabio
> Estevam <festevam@gmail.com>; linux-mmc <linux-mmc@vger.kernel.org>;
> dl-linux-imx <linux-imx@nxp.com>; DTML <devicetree@vger.kernel.org>; Linux
> ARM <linux-arm-kernel@lists.infradead.org>
> Subject: Re: [PATCH 4/6] mmc: host: sdhci-esdhc-imx.c: correct the auto-tuning
> setting for sdio device
> 
> On Tue, 17 Aug 2021 at 08:57, Bough Chen <haibo.chen@nxp.com> wrote:
> >
> > > -----Original Message-----
> > > From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> > > Sent: 2021年8月16日 21:53
> > > To: Bough Chen <haibo.chen@nxp.com>
> > > Cc: Adrian Hunter <adrian.hunter@intel.com>; Shawn Guo
> > > <shawnguo@kernel.org>; Rob Herring <robh+dt@kernel.org>; Sascha
> > > Hauer <s.hauer@pengutronix.de>; Sascha Hauer
> > > <kernel@pengutronix.de>; Fabio Estevam <festevam@gmail.com>;
> > > linux-mmc <linux-mmc@vger.kernel.org>; dl-linux-imx
> > > <linux-imx@nxp.com>; DTML <devicetree@vger.kernel.org>; Linux ARM
> > > <linux-arm-kernel@lists.infradead.org>
> > > Subject: Re: [PATCH 4/6] mmc: host: sdhci-esdhc-imx.c: correct the
> > > auto-tuning setting for sdio device
> > >
> > > On Mon, 16 Aug 2021 at 15:00, <haibo.chen@nxp.com> wrote:
> > > >
> > > > From: Haibo Chen <haibo.chen@nxp.com>
> > > >
> > > > USDHC contain auto tuning circuit, this circuit will work
> > > > automatically after the tuning procedurae, it can
> > > > increase/decrease the delay cell according to the outside
> > > > environment change (like
> > > temperature).
> > > >
> > > > Unfortunately, this auto tuning circuit can not handle the async
> > > > sdio device interrupt correctly. When sdio device use 4 data line,
> > > > async sdio interrupt will use DAT[1], if we enable auto tuning
> > > > circuit check
> > > > 4 data lines, include the DAT[1], this circuit will detect this
> > > > interrupt, take this as a data on DAT[1], and adjust the delay cell wrongly.
> > > >
> > > > This is the hardware design limitation, to avoid this, when sdio
> > > > device enable async interrupt, auto tuning circuit only check
> > > > DAT[0] and CMD
> > > lines.
> > >
> > > SDIO irqs are being enabled/disabled dynamically in runtime by the
> > > mmc core via the host ops ->enable_sdio_irq().
> > >
> > > Rather than forcing the autotuning circuit to stay unused
> > > statically, perhaps an option would be to disable it when the SDIO
> > > irqs becomes enabled? Or maybe that becomes too complicated?
> >
> > For interrupt in 4-bit mode, there is a definition of interrupt
> > period, only in this period can the interrupt be detect and recognize. The
> interrupt period can exist during data transfer.
> >
> > So to fix this issue, one method is to disable auto tuning circuit.
> > Another is my current method, just detect CMD and DAT0, but at least auto
> tuning still can work (this method need board design keep align all data lines).
> 
> To allow DAT1 being used for SDIO irqs (async or not), the irqs needs to be
> enabled internally in the SDIO card first. This is done by writing to the CCCR
> register, which happens in sdio_claim_irq(). At this point in sdio_claim_irq() the
> core also invokes the
> ->enable_sdio_irq() host ops, to allow the host to prepare itself to
> accept SDIO irqs.
> 
> It sounds to me that you should be able to use the ->enable_sdio_irq ops, as a
> way of understanding that the auto-tuning feature also needs to be turned off,
> because it's not compatible with SDIO irqs.
> 

Yes, for current auto-tuning design, it need to be turned off for sdio interrupt.

I'm just a bit greedy, want to find a method to support auto-tuning for sdio interrupt. 😊
I will use a new property like "fsl,broken-auto-tuning" for this WiFI device in V2 patch.

Best Regards
Haibo Chen.

> Kind regards
> Uffe
> 
> >
> >
> > Best Regards
> > Haibo chen
> >
> > >
> > > >
> > > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > > > ---
> > > >  drivers/mmc/host/sdhci-esdhc-imx.c | 18 ++++++++++++++++++
> > > >  1 file changed, 18 insertions(+)
> > > >
> > > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
> > > > b/drivers/mmc/host/sdhci-esdhc-imx.c
> > > > index f18d169bc8ff..ab84c29777e5 100644
> > > > --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> > > > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> > > > @@ -226,6 +226,7 @@ struct esdhc_platform_data {
> > > >         unsigned int tuning_step;       /* The delay cell steps in
> tuning
> > > procedure */
> > > >         unsigned int tuning_start_tap;  /* The start delay cell
> > > > point in
> > > tuning procedure */
> > > >         unsigned int strobe_dll_delay_target;   /* The delay cell for
> > > strobe pad (read clock) */
> > > > +       bool sdio_async_interrupt_enabled;      /* sdio device enable
> > > the async interrupt */
> > > >  };
> > > >
> > > >  struct esdhc_soc_data {
> > > > @@ -416,6 +417,8 @@ static inline void
> > > > esdhc_wait_for_card_clock_gate_off(struct sdhci_host *host)
> > > >  /* Enable the auto tuning circuit to check the CMD line and BUS
> > > > line */  static inline void usdhc_auto_tuning_mode_sel(struct
> > > > sdhci_host
> > > > *host)  {
> > > > +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > > > +       struct pltfm_imx_data *imx_data =
> > > > + sdhci_pltfm_priv(pltfm_host);
> > > >         u32 buswidth, auto_tune_buswidth;
> > > >
> > > >         buswidth = USDHC_GET_BUSWIDTH(readl(host->ioaddr +
> > > > SDHCI_HOST_CONTROL)); @@ -432,6 +435,18 @@ static inline void
> > > usdhc_auto_tuning_mode_sel(struct sdhci_host *host)
> > > >                 break;
> > > >         }
> > > >
> > > > +       /*
> > > > +        * If sdio device use async interrupt, it will use DAT[1] to signal
> > > > +        * the device's interrupt asynchronous when use 4 data lines.
> > > > +        * Then hardware auto tuning circuit MUST NOT check the
> > > > + DAT[1]
> > > line,
> > > > +        * otherwise auto tuning will be impacted by this async
> interrupt,
> > > > +        * and change the delay cell incorrectly, which then cause
> > > data/cmd
> > > > +        * errors.
> > > > +        * This is the hardware auto tuning circuit limitation.
> > > > +        */
> > > > +       if (imx_data->boarddata.sdio_async_interrupt_enabled)
> > > > +               auto_tune_buswidth =
> > > > + ESDHC_VEND_SPEC2_AUTO_TUNE_1BIT_EN;
> > > > +
> > > >         esdhc_clrset_le(host,
> > > ESDHC_VEND_SPEC2_AUTO_TUNE_MODE_MASK,
> > > >                         auto_tune_buswidth |
> > > ESDHC_VEND_SPEC2_AUTO_TUNE_CMD_EN,
> > > >                         ESDHC_VEND_SPEC2); @@ -1531,6
> +1546,9
> > > @@
> > > > sdhci_esdhc_imx_probe_dt(struct platform_device *pdev,
> > > >         if (of_property_read_u32(np, "fsl,delay-line",
> > > &boarddata->delay_line))
> > > >                 boarddata->delay_line = 0;
> > > >
> > > > +       if (of_property_read_bool(np,
> > > > + "fsl,sdio-async-interrupt-enabled"))
> > >
> > > As stated on the DT patch, I think you can use the "cap-sdio-irq" instead.
> > >
> > > > +               boarddata->sdio_async_interrupt_enabled = true;
> > > > +
> > > >         mmc_of_parse_voltage(host->mmc, &host->ocr_mask);
> > > >
> > > >         if (esdhc_is_usdhc(imx_data) &&
> > > > !IS_ERR(imx_data->pinctrl)) {
> > > > --
> > > > 2.17.1
> > > >
> > >
> > > Kind regards
> > > Uffe
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel