Re: [PATCH 3/6] dt-bindings: mmc: fsl-imx-esdhc: add fsl,sdio-async-interrupt-enabled binding

From: Ulf Hansson <ulf.hansson@linaro.org>
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 3/6] dt-bindings: mmc: fsl-imx-esdhc: add fsl,sdio-async-interrupt-enabled binding
Date: Tue, 17 Aug 2021 09:37:00 +0200	[thread overview]
Message-ID: <CAPDyKFqv9R4R8u9vQLkDMFQy3C-+=B-_g4drktq9ncHhzX3pSw@mail.gmail.com> (raw)
In-Reply-To: <VI1PR04MB529432136DB9F5FEB5F5E98090FE9@VI1PR04MB5294.eurprd04.prod.outlook.com>

On Tue, 17 Aug 2021 at 08:41, Bough Chen <haibo.chen@nxp.com> wrote:
>
> > -----Original Message-----
> > From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> > Sent: 2021年8月16日 21:43
> > 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 3/6] dt-bindings: mmc: fsl-imx-esdhc: add
> > fsl,sdio-async-interrupt-enabled binding
> >
> > On Mon, 16 Aug 2021 at 15:00, <haibo.chen@nxp.com> wrote:
> > >
> > > From: Haibo Chen <haibo.chen@nxp.com>
> > >
> > > Add a new fsl,sdio-async-interrupt-enabled binding for sdio devices
> > > which enable the async interrupt function. When get this property,
> > > driver will avoid to use DAT[1] for hardware auto tuning check.
> > >
> > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > > ---
> > >  .../devicetree/bindings/mmc/fsl-imx-esdhc.yaml         | 10
> > ++++++++++
> > >  1 file changed, 10 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
> > > b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
> > > index b5baf439fbac..8a9f1775b0e2 100644
> > > --- a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
> > > +++ b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
> > > @@ -122,6 +122,16 @@ properties:
> > >        - const: state_200mhz
> > >        - const: sleep
> > >
> > > +  fsl,sdio-async-interrupt-enabled:
> > > +    description: |
> > > +      Recommend for SDIO cards that enables SDIO async interrupt for
> > SDR104 and SDR50
> > > +      operating modes. SDIO async interrupt uses DAT[1] to signal the
> > card's interrupt.
> > > +      uSDHC tuning mechanism must use DAT[0] and CMD signals to avoid
> > a possible
> > > +      conflict and incorrect delay line calculated by the uSDHC auto tuning
> > mechanism.
> > > +      Enabling this device tree property is only recommended for layouts
> > that are
> > > +      matching the SD interface length.
> > > +    type: boolean
> >
> > We already have a common mmc property, "cap-sdio-irq", that tells whether
> > the controller supports SDIO irqs (which is delivered on DAT1).
> >
> > Can't you use this instead?
> >
> Hi Ulf,
>
> Thanks for your quick reply!
>
> According to our WiFi team reply, the sdio-irq has two types. Sync interrupt and Async interrupt.
> When WiFi send out the interrupt signal during the interrupt period, if it sync with clock pad(just as
> when send out data), then this is sync interrupt. When this interrupt not sync with clock, it is async
> interrupt. Async interrupt has a better overall performance than sync interrupt.

The async interrupt is what we refer to as SDIO irqs, which is being
delivered on DAT1.

>
> Logically, auto tuning circuit should only take care of the data and cmd line, and ignore interrupt signal.
> But unfortunately current i.mx-usdhc IP do not ignore interrupt signal. So it detect the interrupt signal,
> and take this signal as a data signal, and adjust the delay cell accordingly. For sync interrupt, due to it
> sync with clock, so no affect, but for async interrupt, it will involve wrong delay cell change randomly.

Okay.

>
> I involve a new property here, because in sdhci.c, we default use this "cap-sdio-irq" for all sdio/sd/mmc.

I guess it's the similar variant of the controller for all slots then.

It can be debated whether the proper thing is to set "cap-sdio-irq"
only for the SDIO card slot. I think so, (and it's already being used
like that) if there is an embedded SDIO card attached, because
cap-sdio-irq would not make sense otherwise.

> I need one property which can use only for sdio device, and only when sdio device enable async-interrupt.

If you really need a new DT property (let's discuss that more in patch
4/6), I suggest you add something along the lines of a
"broken-auto-tuning" DT property instead, because that is in principle
what this should be about, isn't it?

Kind regards
Uffe