diff mbox series

[v9,1/3] dt-bindings: mmc: mtk-sd: extend interrupts and pinctrls properties

Message ID 20220329032913.8750-2-axe.yang@mediatek.com
State New
Headers show
Series [v9,1/3] dt-bindings: mmc: mtk-sd: extend interrupts and pinctrls properties | expand

Commit Message

Axe Yang March 29, 2022, 3:29 a.m. UTC
Extend interrupts and pinctrls for SDIO wakeup interrupt feature.
This feature allow SDIO devices alarm asynchronous interrupt to host
even when host stop providing clock to SDIO card. An extra wakeup
interrupt and pinctrl states for SDIO DAT1 pin state switching are
required in this scenario.

Signed-off-by: Axe Yang <axe.yang@mediatek.com>
---
 .../devicetree/bindings/mmc/mtk-sd.yaml         | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

Comments

Rob Herring (Arm) March 29, 2022, 11 p.m. UTC | #1
On Tue, 29 Mar 2022 11:29:11 +0800, Axe Yang wrote:
> Extend interrupts and pinctrls for SDIO wakeup interrupt feature.
> This feature allow SDIO devices alarm asynchronous interrupt to host
> even when host stop providing clock to SDIO card. An extra wakeup
> interrupt and pinctrl states for SDIO DAT1 pin state switching are
> required in this scenario.
> 
> Signed-off-by: Axe Yang <axe.yang@mediatek.com>
> ---
>  .../devicetree/bindings/mmc/mtk-sd.yaml         | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>
Ulf Hansson April 1, 2022, 9:22 a.m. UTC | #2
On Tue, 29 Mar 2022 at 05:29, Axe Yang <axe.yang@mediatek.com> wrote:
>
> Extend interrupts and pinctrls for SDIO wakeup interrupt feature.
> This feature allow SDIO devices alarm asynchronous interrupt to host
> even when host stop providing clock to SDIO card. An extra wakeup
> interrupt and pinctrl states for SDIO DAT1 pin state switching are
> required in this scenario.
>
> Signed-off-by: Axe Yang <axe.yang@mediatek.com>
> ---
>  .../devicetree/bindings/mmc/mtk-sd.yaml         | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/mmc/mtk-sd.yaml b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml
> index 297ada03e3de..3872a6ce2867 100644
> --- a/Documentation/devicetree/bindings/mmc/mtk-sd.yaml
> +++ b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml
> @@ -69,12 +69,22 @@ properties:
>        - const: ahb_cg
>
>    interrupts:
> -    maxItems: 1
> +    description:
> +      Should at least contain MSDC GIC interrupt. To support SDIO in-band wakeup, an extended
> +      interrupt is required and be configured as wakeup source irq.

If I understand correctly, the extended interrupt (a GPIO irq) may not
necessarily share the same interrupt parent as the primary device
interrupt.

Perhaps it's then better to extend this with "interrupts-extended"
instead. See Documentation/devicetree/bindings/interrupt-controller/interrupts.txt.

> +    minItems: 1
> +    maxItems: 2
>
>    pinctrl-names:
> +    description:
> +      Should at least contain default and state_uhs. To support SDIO in-band wakeup, dat1 pin
> +      will be switched between GPIO mode and SDIO DAT1 mode, state_eint and state_dat1 are
> +      mandatory in this scenarios.
> +    minItems: 2
>      items:
>        - const: default
>        - const: state_uhs
> +      - const: state_eint
>
>    pinctrl-0:
>      description:
> @@ -86,6 +96,11 @@ properties:
>        should contain uhs mode pin ctrl.
>      maxItems: 1
>
> +  pinctrl-2:
> +    description:
> +      should switch dat1 pin to GPIO mode.
> +    maxItems: 1
> +
>    assigned-clocks:
>      description:
>        PLL of the source clock.
> --
> 2.25.1
>

Kind regards
Uffe
Ulf Hansson April 1, 2022, 9:34 a.m. UTC | #3
On Fri, 1 Apr 2022 at 11:22, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Tue, 29 Mar 2022 at 05:29, Axe Yang <axe.yang@mediatek.com> wrote:
> >
> > Extend interrupts and pinctrls for SDIO wakeup interrupt feature.
> > This feature allow SDIO devices alarm asynchronous interrupt to host
> > even when host stop providing clock to SDIO card. An extra wakeup
> > interrupt and pinctrl states for SDIO DAT1 pin state switching are
> > required in this scenario.
> >
> > Signed-off-by: Axe Yang <axe.yang@mediatek.com>
> > ---
> >  .../devicetree/bindings/mmc/mtk-sd.yaml         | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/mmc/mtk-sd.yaml b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml
> > index 297ada03e3de..3872a6ce2867 100644
> > --- a/Documentation/devicetree/bindings/mmc/mtk-sd.yaml
> > +++ b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml
> > @@ -69,12 +69,22 @@ properties:
> >        - const: ahb_cg
> >
> >    interrupts:
> > -    maxItems: 1
> > +    description:
> > +      Should at least contain MSDC GIC interrupt. To support SDIO in-band wakeup, an extended
> > +      interrupt is required and be configured as wakeup source irq.
>
> If I understand correctly, the extended interrupt (a GPIO irq) may not
> necessarily share the same interrupt parent as the primary device
> interrupt.
>
> Perhaps it's then better to extend this with "interrupts-extended"
> instead. See Documentation/devicetree/bindings/interrupt-controller/interrupts.txt.

One more thing, looks like using the "interrupt-names" property would
be good to use too. To easier distinguish the different irqs.

[...]

Kind regards
Uffe
Rob Herring (Arm) April 1, 2022, 5:43 p.m. UTC | #4
On Fri, Apr 01, 2022 at 11:22:13AM +0200, Ulf Hansson wrote:
> On Tue, 29 Mar 2022 at 05:29, Axe Yang <axe.yang@mediatek.com> wrote:
> >
> > Extend interrupts and pinctrls for SDIO wakeup interrupt feature.
> > This feature allow SDIO devices alarm asynchronous interrupt to host
> > even when host stop providing clock to SDIO card. An extra wakeup
> > interrupt and pinctrl states for SDIO DAT1 pin state switching are
> > required in this scenario.
> >
> > Signed-off-by: Axe Yang <axe.yang@mediatek.com>
> > ---
> >  .../devicetree/bindings/mmc/mtk-sd.yaml         | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/mmc/mtk-sd.yaml b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml
> > index 297ada03e3de..3872a6ce2867 100644
> > --- a/Documentation/devicetree/bindings/mmc/mtk-sd.yaml
> > +++ b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml
> > @@ -69,12 +69,22 @@ properties:
> >        - const: ahb_cg
> >
> >    interrupts:
> > -    maxItems: 1
> > +    description:
> > +      Should at least contain MSDC GIC interrupt. To support SDIO in-band wakeup, an extended
> > +      interrupt is required and be configured as wakeup source irq.
> 
> If I understand correctly, the extended interrupt (a GPIO irq) may not
> necessarily share the same interrupt parent as the primary device
> interrupt.
> 
> Perhaps it's then better to extend this with "interrupts-extended"
> instead. See Documentation/devicetree/bindings/interrupt-controller/interrupts.txt.

'interrupts-extended' is interchangeable with 'interrupts'. For schemas, 
use 'interrupts' and the tools take care of supporting both forms.

Rob
Axe Yang April 6, 2022, 9:38 a.m. UTC | #5
On Fri, 2022-04-01 at 12:43 -0500, Rob Herring wrote:
> On Fri, Apr 01, 2022 at 11:22:13AM +0200, Ulf Hansson wrote:
> > On Tue, 29 Mar 2022 at 05:29, Axe Yang <axe.yang@mediatek.com>
> > wrote:
> > > 
> > > Extend interrupts and pinctrls for SDIO wakeup interrupt feature.
> > > This feature allow SDIO devices alarm asynchronous interrupt to
> > > host
> > > even when host stop providing clock to SDIO card. An extra wakeup
> > > interrupt and pinctrl states for SDIO DAT1 pin state switching
> > > are
> > > required in this scenario.
> > > 
> > > Signed-off-by: Axe Yang <axe.yang@mediatek.com>
> > > ---
> > >  .../devicetree/bindings/mmc/mtk-sd.yaml         | 17
> > > ++++++++++++++++-
> > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/mmc/mtk-sd.yaml
> > > b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml
> > > index 297ada03e3de..3872a6ce2867 100644
> > > --- a/Documentation/devicetree/bindings/mmc/mtk-sd.yaml
> > > +++ b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml
> > > @@ -69,12 +69,22 @@ properties:
> > >        - const: ahb_cg
> > > 
> > >    interrupts:
> > > -    maxItems: 1
> > > +    description:
> > > +      Should at least contain MSDC GIC interrupt. To support
> > > SDIO in-band wakeup, an extended
> > > +      interrupt is required and be configured as wakeup source
> > > irq.
> > 
> > If I understand correctly, the extended interrupt (a GPIO irq) may
> > not
> > necessarily share the same interrupt parent as the primary device
> > interrupt.
> > 
> > Perhaps it's then better to extend this with "interrupts-extended"
> > instead. See Documentation/devicetree/bindings/interrupt-
> > controller/interrupts.txt.
> 
> 'interrupts-extended' is interchangeable with 'interrupts'. For
> schemas, 
> use 'interrupts' and the tools take care of supporting both forms.
> 

hello Ulf, you are right, the wakeup interrupt(parent is &pio) do not
share same parent as primary interrupt(parent is &gic). And as you
said, I am using "interrupts-extended" to declare the wakeup irq, see
commit message in patch 3/3:
         &mmcX {
                 ...
                 interrupts-extended = <...>,
                                       <&pio xxx IRQ_TYPE_LEVEL_LOW>;
                 ...
                 pinctrl-names = "default", "state_uhs", "state_eint";
                 ...
                 pinctrl-2 = <&mmc2_pins_eint>;
                 ...
                 cap-sdio-irq;
                 keep-power-in-suspend;
                 wakeup-source;
                 ...
         };

But the wakup interrupt is for SDIO only, in most instances, MSDC is
been used as eMMC/SD card host, they do not need this interrupt. So as
Rob suggested, I think we'd better keep using 'interrupts'. And I will
update the description for 'interrupts', suggest to use 'interrupts-
extended' to declare SDIO wakeup interrupt.

And 'interrupt-names' is a good idea, I will add this property to
document too. Thank you for the advice.

Regards,
Axe
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/mmc/mtk-sd.yaml b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml
index 297ada03e3de..3872a6ce2867 100644
--- a/Documentation/devicetree/bindings/mmc/mtk-sd.yaml
+++ b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml
@@ -69,12 +69,22 @@  properties:
       - const: ahb_cg
 
   interrupts:
-    maxItems: 1
+    description:
+      Should at least contain MSDC GIC interrupt. To support SDIO in-band wakeup, an extended
+      interrupt is required and be configured as wakeup source irq.
+    minItems: 1
+    maxItems: 2
 
   pinctrl-names:
+    description:
+      Should at least contain default and state_uhs. To support SDIO in-band wakeup, dat1 pin
+      will be switched between GPIO mode and SDIO DAT1 mode, state_eint and state_dat1 are
+      mandatory in this scenarios.
+    minItems: 2
     items:
       - const: default
       - const: state_uhs
+      - const: state_eint
 
   pinctrl-0:
     description:
@@ -86,6 +96,11 @@  properties:
       should contain uhs mode pin ctrl.
     maxItems: 1
 
+  pinctrl-2:
+    description:
+      should switch dat1 pin to GPIO mode.
+    maxItems: 1
+
   assigned-clocks:
     description:
       PLL of the source clock.