| Message ID | 20160915193405.26943-3-chris.brandt@renesas.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Horman |
| Headers | show |
Hi Chris, On Thu, Sep 15, 2016 at 9:34 PM, Chris Brandt <chris.brandt@renesas.com> wrote: > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> > --- > arch/arm/boot/dts/r7s72100.dtsi | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi > index e18d4e6..4054db3 100644 > --- a/arch/arm/boot/dts/r7s72100.dtsi > +++ b/arch/arm/boot/dts/r7s72100.dtsi > @@ -450,4 +450,15 @@ > #size-cells = <0>; > status = "disabled"; > }; > + > + mmcif: mmc@e804c800 { > + compatible = "renesas,mmcif-r7s72100", "renesas,sh-mmcif"; > + reg = <0xe804c800 0x80>; > + interrupts = <GIC_SPI 268 IRQ_TYPE_LEVEL_HIGH > + GIC_SPI 269 IRQ_TYPE_LEVEL_HIGH>; The bindings do not say anything about interrupts (hence that should be added). The driver handles either 1 combined or 2 separate interrupts. The datasheet says MMC has 3 interrupt requests, though? > + clocks = <&mstp8_clks R7S72100_CLK_MMCIF>; > + reg-io-width = <4>; > + bus-width = <8>; > + status = "disabled"; > + }; > }; The rest looks OK to me. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, On 9/15/2016, Geert Uytterhoeven wrote: > > + interrupts = <GIC_SPI 268 IRQ_TYPE_LEVEL_HIGH > > + GIC_SPI 269 IRQ_TYPE_LEVEL_HIGH>; > > The bindings do not say anything about interrupts (hence that should be > added). I'm sorry, I'm confused... Are you saying: A) I forgot to add something? B) As a general statement, the renesas,mmcif.txt file doesn't say anything about interrupts? > The driver handles either 1 combined or 2 separate interrupts. > The datasheet says MMC has 3 interrupt requests, though? The IP itself has 3 interrupts: #1. MMC0,299: Card detect #2. MMC1,300: Status Change #3. MMC2,301: Error Many of the 'Linux' SoC devices that use this MMC (SH4, R-Car) are only ever going to use eMMC, so the card detection portion of the IP was irrelevant. I agree this is the same case for the RZ/A (who would ever use an MMC card now a days?????) The 'smaller' SoCs kept it in (SH2A, RZ/A1) but the 'bigger' SoCs left it out (SH4A, R-Car). The only way to enable that interrupt is to write to the CE_DETECT register (offset 0x70) which the driver doesn't do. However....if you look in sh_mmcif.h, you'll see that same offset (0x70) is called CE_CLK_CTRL2 #define MMCIF_CE_CLK_CTRL2 0x00000070 Which it only writes to if 'clk_ctrl2_enable' is designated. if (host->clk_ctrl2_enable) sh_mmcif_writel(host->addr, MMCIF_CE_CLK_CTRL2, 0x0F0F0000); And, I see no Renesas SoC that ever sets that or would ever use it so I can't even tell you what SoC that was for (SH4, SH-Mobile, ARM) So after all that ranting...I see no need to support card detect for MMC for the RZ/A (or any future RZ) so I'd like to just leave it as is. Do you agree? Chris
Hi Chris, On Fri, Sep 16, 2016 at 3:08 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote: > On 9/15/2016, Geert Uytterhoeven wrote: >> > + interrupts = <GIC_SPI 268 IRQ_TYPE_LEVEL_HIGH >> > + GIC_SPI 269 IRQ_TYPE_LEVEL_HIGH>; >> >> The bindings do not say anything about interrupts (hence that should be >> added). > > I'm sorry, I'm confused... > Are you saying: > A) I forgot to add something? > B) As a general statement, the renesas,mmcif.txt file doesn't say anything about interrupts? Both ;-) >> The driver handles either 1 combined or 2 separate interrupts. >> The datasheet says MMC has 3 interrupt requests, though? > > The IP itself has 3 interrupts: > #1. MMC0,299: Card detect > #2. MMC1,300: Status Change > #3. MMC2,301: Error > > Many of the 'Linux' SoC devices that use this MMC (SH4, R-Car) are only ever going to use eMMC, so the card detection portion of the IP was irrelevant. I agree this is the same case for the RZ/A (who would ever use an MMC card now a days?????) > The 'smaller' SoCs kept it in (SH2A, RZ/A1) but the 'bigger' SoCs left it out (SH4A, R-Car). > > The only way to enable that interrupt is to write to the CE_DETECT register (offset 0x70) which the driver doesn't do. > > However....if you look in sh_mmcif.h, you'll see that same offset (0x70) is called CE_CLK_CTRL2 > > #define MMCIF_CE_CLK_CTRL2 0x00000070 > > Which it only writes to if 'clk_ctrl2_enable' is designated. > > if (host->clk_ctrl2_enable) > sh_mmcif_writel(host->addr, MMCIF_CE_CLK_CTRL2, 0x0F0F0000); > > And, I see no Renesas SoC that ever sets that or would ever use it so I can't even tell you what SoC that was for (SH4, SH-Mobile, ARM) Google and Patchwork told me this was enabled in the old board code for Lager: commit b77c6bcef2082a7cd96124daf15df8da5b670ebe Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de> Date: Wed Jul 10 21:21:17 2013 +0200 ARM: shmobile: lager: disable MMCIF Command Completion Signal, add CLK_CTRL2 MMCIF on r8a7790 doesn't support Command Completion Signal, but it does implement a CE_CLK_CTRL2 register. Platform parameters have to be added to account for these features on lager. Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com> Signed-off-by: Simon Horman <horms+renesas@verge.net.au> But it was lost during the transition to DT. No idea if someone tried MMC (not SDHI) on Lager, or any other R-Car Gen2 board, recently... > So after all that ranting...I see no need to support card detect for MMC for the RZ/A (or any future RZ) so I'd like to just leave it as is. > > Do you agree? Let's summarize... 1) Documentation/devicetree/bindings/mmc/renesas,mmcif.txt should document the "interrupts" property, for both single and multiple values. 2) If you specify 2 interrupts in DT, the driver names the first one "sh_mmc:error", and the second one "sh_mmc:int", while you specified them in reverse order (status first, error second). I couldn't find which of the three interrupts on R-Mobile A1 is used for what purpose in the datasheet, but the datasheet for SH-Mobile AG5 states the first (140) is MMC_ERR, and the second (141) is MMC_NOR. However, it still worked for you, as the driver uses the same interrupt handler for all interrupts. 3) You may want to add the card detect interrupt to DT anyway, as DT describes the hardware, not limitations of the driver. If you do so, renesas,mmcif.txt should document the third interrupt value. Wolfram, do you have any comments? Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, On 9/19/2016, Geert Uytterhoeven wrote: > >> The bindings do not say anything about interrupts (hence that should > >> be added). > > > > I'm sorry, I'm confused... > > Are you saying: > > A) I forgot to add something? > > B) As a general statement, the renesas,mmcif.txt file doesn't say > anything about interrupts? > > Both ;-) OK. > > The IP itself has 3 interrupts: > > #1. MMC0,299: Card detect > > #2. MMC1,300: Status Change > > #3. MMC2,301: Error Correction! I had the order wrong. From the RZ/AH1 Hardware manual (51.4 Interrupt Requests) #1. MMC0,299: Card detection interrupt #2. MMC1,300: Error/timeout interrupt #3. MMC2,301: Normal operation interrupt So it's the same as all the other SoCs. Sorry for the confusion. > 3) You may want to add the card detect interrupt to DT anyway, as DT > describes the hardware, not limitations of the driver. If you do so, > renesas,mmcif.txt should document the third interrupt value. OK. I have no objections to adding it to DT and renesas,mmcif.txt and not touching sh_mmcif.c (just letting it ignore the 3rd value). If someone in the future wants to code up card detect....they are welcome to it ;) Chris
diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi index e18d4e6..4054db3 100644 --- a/arch/arm/boot/dts/r7s72100.dtsi +++ b/arch/arm/boot/dts/r7s72100.dtsi @@ -450,4 +450,15 @@ #size-cells = <0>; status = "disabled"; }; + + mmcif: mmc@e804c800 { + compatible = "renesas,mmcif-r7s72100", "renesas,sh-mmcif"; + reg = <0xe804c800 0x80>; + interrupts = <GIC_SPI 268 IRQ_TYPE_LEVEL_HIGH + GIC_SPI 269 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&mstp8_clks R7S72100_CLK_MMCIF>; + reg-io-width = <4>; + bus-width = <8>; + status = "disabled"; + }; };
Signed-off-by: Chris Brandt <chris.brandt@renesas.com> --- arch/arm/boot/dts/r7s72100.dtsi | 11 +++++++++++ 1 file changed, 11 insertions(+)