diff mbox series

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

Message ID 1629117508-4886-3-git-send-email-haibo.chen@nxp.com (mailing list archive)
State New, archived
Headers show
Series [1/6] mmc: sdhci-esdhc-imx: remove redundant code for manual tuning | expand

Commit Message

Bough Chen Aug. 16, 2021, 12:38 p.m. UTC 
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(+)

Comments

Ulf Hansson Aug. 16, 2021, 1:43 p.m. UTC | #1 
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?

> +
>  required:
>    - compatible
>    - reg
> --
> 2.17.1
>

Kind regards
Uffe
Bough Chen Aug. 17, 2021, 6:41 a.m. UTC | #2 
> -----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.

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.

I involve a new property here, because in sdhci.c, we default use this "cap-sdio-irq" for all sdio/sd/mmc.
I need one property which can use only for sdio device, and only when sdio device enable async-interrupt.

Best Regards
Haibo Chen

> > +
> >  required:
> >    - compatible
> >    - reg
> > --
> > 2.17.1
> >
> 
> Kind regards
> Uffe
Ulf Hansson Aug. 17, 2021, 7:37 a.m. UTC | #3 
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
diff mbox series

Patch

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
+
 required:
   - compatible
   - reg