| Message ID | 20220525015140.384-2-axe.yang@mediatek.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | mmc: mediatek: add support for SDIO async IRQ | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Wed, 25 May 2022 09:51:38 +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. > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > Signed-off-by: Axe Yang <axe.yang@mediatek.com> > --- > .../devicetree/bindings/mmc/mtk-sd.yaml | 50 ++++++++++++++++++- > 1 file changed, 49 insertions(+), 1 deletion(-) > Reviewed-by: Rob Herring <robh@kernel.org>
On Wed, 25 May 2022 at 03:51, 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. > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > Signed-off-by: Axe Yang <axe.yang@mediatek.com> > --- > .../devicetree/bindings/mmc/mtk-sd.yaml | 50 ++++++++++++++++++- > 1 file changed, 49 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/mmc/mtk-sd.yaml b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml > index 2a2e9fa8c188..e83bf10281d6 100644 > --- a/Documentation/devicetree/bindings/mmc/mtk-sd.yaml > +++ b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml > @@ -72,12 +72,27 @@ 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 > + > + interrupt-names: > + items: > + - const: msdc > + - const: sdio_wakeup > > 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 Don't you need something along the lines of the below instead? I mean the "state_eint" isn't always needed, right? oneOf: - items: - const: default - const: state_uhs - items: - const: default - const: state_uhs - const: state_eint > > pinctrl-0: > description: > @@ -89,6 +104,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. > @@ -208,4 +228,32 @@ examples: > mediatek,hs400-cmd-resp-sel-rising; > }; > > + mmc3: mmc@11260000 { > + compatible = "mediatek,mt8173-mmc"; > + reg = <0x11260000 0x1000>; > + clock-names = "source", "hclk"; > + clocks = <&pericfg CLK_PERI_MSDC30_3>, > + <&topckgen CLK_TOP_MSDC50_2_H_SEL>; > + interrupt-names = "msdc", "sdio_wakeup"; > + interrupts-extended = <&gic GIC_SPI 74 IRQ_TYPE_LEVEL_LOW 0>, > + <&pio 23 IRQ_TYPE_LEVEL_LOW>; > + pinctrl-names = "default", "state_uhs", "state_eint"; > + pinctrl-0 = <&mmc2_pins_default>; > + pinctrl-1 = <&mmc2_pins_uhs>; > + pinctrl-2 = <&mmc2_pins_eint>; > + bus-width = <4>; > + max-frequency = <200000000>; > + cap-sd-highspeed; > + sd-uhs-sdr104; > + keep-power-in-suspend; > + wakeup-source; > + cap-sdio-irq; > + no-mmc; > + no-sd; > + non-removable; > + vmmc-supply = <&sdio_fixed_3v3>; > + vqmmc-supply = <&mt6397_vgp3_reg>; > + mmc-pwrseq = <&wifi_pwrseq>; > + }; > + > ... > -- > 2.25.1 > Kind regards Uffe
On Fri, Jun 03, 2022 at 09:28:37AM +0200, Ulf Hansson wrote: > On Wed, 25 May 2022 at 03:51, 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. > > > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > > Signed-off-by: Axe Yang <axe.yang@mediatek.com> > > --- > > .../devicetree/bindings/mmc/mtk-sd.yaml | 50 ++++++++++++++++++- > > 1 file changed, 49 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/mmc/mtk-sd.yaml b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml > > index 2a2e9fa8c188..e83bf10281d6 100644 > > --- a/Documentation/devicetree/bindings/mmc/mtk-sd.yaml > > +++ b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml > > @@ -72,12 +72,27 @@ 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 > > + > > + interrupt-names: > > + items: > > + - const: msdc > > + - const: sdio_wakeup > > > > 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 > > Don't you need something along the lines of the below instead? I mean > the "state_eint" isn't always needed, right? > > oneOf: > - items: > - const: default > - const: state_uhs > - items: > - const: default > - const: state_uhs > - const: state_eint This is equivalent to what was done. The 'minItems: 2' makes the 3rd item optional. Rob
On Fri, 2022-06-03 at 09:28 +0200, Ulf Hansson wrote: > On Wed, 25 May 2022 at 03:51, 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. > > > > Reviewed-by: AngeloGioacchino Del Regno < > > angelogioacchino.delregno@collabora.com> > > Signed-off-by: Axe Yang <axe.yang@mediatek.com> > > --- > > .../devicetree/bindings/mmc/mtk-sd.yaml | 50 > > ++++++++++++++++++- > > 1 file changed, 49 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/mmc/mtk-sd.yaml > > b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml > > index 2a2e9fa8c188..e83bf10281d6 100644 > > --- a/Documentation/devicetree/bindings/mmc/mtk-sd.yaml > > +++ b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml > > @@ -72,12 +72,27 @@ 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 > > + > > + interrupt-names: > > + items: > > + - const: msdc > > + - const: sdio_wakeup > > > > 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 > > Don't you need something along the lines of the below instead? I mean > the "state_eint" isn't always needed, right? > > oneOf: > - items: > - const: default > - const: state_uhs > - items: > - const: default > - const: state_uhs > - const: state_eint > No, it is not always needed. As Rob said, the 'minItems: 2' makes the 3rd item optional. Combine 'minItems' and 'description' fields, it is easy for others to understand how to set pinctrl related properities. Anyway, If you insist 'oneOf' is the better way, I can update that in next version. What do you think? And thanks to your comment, I found a mistake in 'description', I should remove descriptions related to 'state_dat1', and I will update that in next version. And do you have any comment on patch 2/3 and 3/3? Regards, Axe
On Tue, 7 Jun 2022 at 10:24, Axe Yang <axe.yang@mediatek.com> wrote: > > On Fri, 2022-06-03 at 09:28 +0200, Ulf Hansson wrote: > > On Wed, 25 May 2022 at 03:51, 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. > > > > > > Reviewed-by: AngeloGioacchino Del Regno < > > > angelogioacchino.delregno@collabora.com> > > > Signed-off-by: Axe Yang <axe.yang@mediatek.com> > > > --- > > > .../devicetree/bindings/mmc/mtk-sd.yaml | 50 > > > ++++++++++++++++++- > > > 1 file changed, 49 insertions(+), 1 deletion(-) > > > > > > diff --git a/Documentation/devicetree/bindings/mmc/mtk-sd.yaml > > > b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml > > > index 2a2e9fa8c188..e83bf10281d6 100644 > > > --- a/Documentation/devicetree/bindings/mmc/mtk-sd.yaml > > > +++ b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml > > > @@ -72,12 +72,27 @@ 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 > > > + > > > + interrupt-names: > > > + items: > > > + - const: msdc > > > + - const: sdio_wakeup > > > > > > 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 > > > > Don't you need something along the lines of the below instead? I mean > > the "state_eint" isn't always needed, right? > > > > oneOf: > > - items: > > - const: default > > - const: state_uhs > > - items: > > - const: default > > - const: state_uhs > > - const: state_eint > > > No, it is not always needed. > As Rob said, the 'minItems: 2' makes the 3rd item optional. > Combine 'minItems' and 'description' fields, it is easy for others to > understand how to set pinctrl related properities. Yes, I agree. > > Anyway, If you insist 'oneOf' is the better way, I can update that in > next version. What do you think? I am fine with it as is, sorry for the noise. > > And thanks to your comment, I found a mistake in 'description', I > should remove descriptions related to 'state_dat1', and I will update > that in next version. Okay. > > And do you have any comment on patch 2/3 and 3/3? Sorry for the delay, I will have a look asap. > > > Regards, > Axe > > Kind regards Uffe
diff --git a/Documentation/devicetree/bindings/mmc/mtk-sd.yaml b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml index 2a2e9fa8c188..e83bf10281d6 100644 --- a/Documentation/devicetree/bindings/mmc/mtk-sd.yaml +++ b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml @@ -72,12 +72,27 @@ 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 + + interrupt-names: + items: + - const: msdc + - const: sdio_wakeup 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: @@ -89,6 +104,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. @@ -208,4 +228,32 @@ examples: mediatek,hs400-cmd-resp-sel-rising; }; + mmc3: mmc@11260000 { + compatible = "mediatek,mt8173-mmc"; + reg = <0x11260000 0x1000>; + clock-names = "source", "hclk"; + clocks = <&pericfg CLK_PERI_MSDC30_3>, + <&topckgen CLK_TOP_MSDC50_2_H_SEL>; + interrupt-names = "msdc", "sdio_wakeup"; + interrupts-extended = <&gic GIC_SPI 74 IRQ_TYPE_LEVEL_LOW 0>, + <&pio 23 IRQ_TYPE_LEVEL_LOW>; + pinctrl-names = "default", "state_uhs", "state_eint"; + pinctrl-0 = <&mmc2_pins_default>; + pinctrl-1 = <&mmc2_pins_uhs>; + pinctrl-2 = <&mmc2_pins_eint>; + bus-width = <4>; + max-frequency = <200000000>; + cap-sd-highspeed; + sd-uhs-sdr104; + keep-power-in-suspend; + wakeup-source; + cap-sdio-irq; + no-mmc; + no-sd; + non-removable; + vmmc-supply = <&sdio_fixed_3v3>; + vqmmc-supply = <&mt6397_vgp3_reg>; + mmc-pwrseq = <&wifi_pwrseq>; + }; + ...