| Message ID | 2757472.iuyUyRzJ3z@wasted.cogentembedded.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Sergei, On Tue, Aug 11, 2015 at 01:44:03AM +0300, Sergei Shtylyov wrote: > Define the SILK board dependent part of the SDHI1 (connected to micro-SD slot) > device nodes along with the necessary voltage regulators. > > Based on the original patch by Vladimir Barinov > <vladimir.barinov@cogentembedded.com>. > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > --- > This patch is against 'renesas-devel-20150810-v4.2-rc6' tag of Simon Horman's > 'renesas.git' repo plus the R8A7794/SILK QSPI patches just re-posted. It needs > the R8A7794 GPIO patches in order to compile. > > Changes in version 2: > - removed not working SDHI0 stuff, renamed the patch; > - replaced SDHI1's "wp-gpios" property with "disable-wp". I am wondering if you could explain the motivation for the "disable-wp" update and weather it is appropriate for other r8a779* dts files. Thanks.
Hello. On 08/11/2015 01:44 AM, Sergei Shtylyov wrote: > Define the SILK board dependent part of the SDHI1 (connected to micro-SD slot) > device nodes along with the necessary voltage regulators. > > Based on the original patch by Vladimir Barinov > <vladimir.barinov@cogentembedded.com>. > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > --- > This patch is against 'renesas-devel-20150810-v4.2-rc6' tag of Simon Horman's > 'renesas.git' repo plus the R8A7794/SILK QSPI patches just re-posted. It needs > the R8A7794 GPIO patches in order to compile. > > Changes in version 2: > - removed not working SDHI0 stuff, renamed the patch; > - replaced SDHI1's "wp-gpios" property with "disable-wp". > > arch/arm/boot/dts/r8a7794-silk.dts | 40 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > Index: renesas/arch/arm/boot/dts/r8a7794-silk.dts > =================================================================== > --- renesas.orig/arch/arm/boot/dts/r8a7794-silk.dts > +++ renesas/arch/arm/boot/dts/r8a7794-silk.dts [...] > @@ -39,6 +40,29 @@ > regulator-boot-on; > regulator-always-on; > }; Oops, need a newline here. I'll repost. > + vcc_sdhi1: regulator@3 { > + compatible = "regulator-fixed"; > + > + regulator-name = "SDHI1 Vcc"; > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + > + gpio = <&gpio4 26 GPIO_ACTIVE_HIGH>; > + enable-active-high; > + }; > + > + vccq_sdhi1: regulator@4 { > + compatible = "regulator-gpio"; > + > + regulator-name = "SDHI1 VccQ"; > + regulator-min-microvolt = <1800000>; > + regulator-max-microvolt = <3300000>; > + > + gpios = <&gpio4 29 GPIO_ACTIVE_HIGH>; > + gpios-states = <1>; > + states = <3300000 1 > + 1800000 0>; > + }; > }; > > &extal_clk { [...] MBR, Sergei
On 08/12/2015 04:26 AM, Simon Horman wrote: >> Define the SILK board dependent part of the SDHI1 (connected to micro-SD slot) >> device nodes along with the necessary voltage regulators. >> >> Based on the original patch by Vladimir Barinov >> <vladimir.barinov@cogentembedded.com>. >> >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >> >> --- >> This patch is against 'renesas-devel-20150810-v4.2-rc6' tag of Simon Horman's >> 'renesas.git' repo plus the R8A7794/SILK QSPI patches just re-posted. It needs >> the R8A7794 GPIO patches in order to compile. >> >> Changes in version 2: >> - removed not working SDHI0 stuff, renamed the patch; >> - replaced SDHI1's "wp-gpios" property with "disable-wp". > > I am wondering if you could explain the motivation for the "disable-wp" > update Please see the comment in mmc_sd_get_ro(). > and weather it is appropriate for other r8a779* dts files. In case of micro-SD slots, yes. > Thanks. MBR, Sergei
On 08/21/2015 11:57 PM, Sergei Shtylyov wrote: >>> Define the SILK board dependent part of the SDHI1 (connected to micro-SD slot) >>> device nodes along with the necessary voltage regulators. >>> >>> Based on the original patch by Vladimir Barinov >>> <vladimir.barinov@cogentembedded.com>. >>> >>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >>> >>> --- >>> This patch is against 'renesas-devel-20150810-v4.2-rc6' tag of Simon Horman's >>> 'renesas.git' repo plus the R8A7794/SILK QSPI patches just re-posted. It needs >>> the R8A7794 GPIO patches in order to compile. >>> >>> Changes in version 2: >>> - removed not working SDHI0 stuff, renamed the patch; >>> - replaced SDHI1's "wp-gpios" property with "disable-wp". >> >> I am wondering if you could explain the motivation for the "disable-wp" >> update > > Please see the comment in mmc_sd_get_ro(). > >> and weather it is appropriate for other r8a779* dts files. > > In case of micro-SD slots, yes. The MMC binding document says it should only be specified if the controller has WP detection logic. We, so far, seem to have been replying on the GPIOs despite this logic is present (according to the R-Car gen2 SDHI manuals I have). The driver will first call mmc_gpio_get_ro() and when that fails due to "wp-gpios" not being specified, it proceeds to reading the register but that is forbidden by TMIO_MMC_WRPROTECT_DISABLE flag set for the R-Car gen1/2 chips, so 0 is always returned from tmio_mmc_get_ro(). There seems to be no point in going that far (and doing the runtime PM dances) -- and MMC_CAP2_NO_WRITE_PROTECT (set when "disable-wp" is specified) prohibits doing that... MBR, Sergei
On Sat, Aug 22, 2015 at 01:18:09AM +0300, Sergei Shtylyov wrote: > On 08/21/2015 11:57 PM, Sergei Shtylyov wrote: > > >>>Define the SILK board dependent part of the SDHI1 (connected to micro-SD slot) > >>>device nodes along with the necessary voltage regulators. > >>> > >>>Based on the original patch by Vladimir Barinov > >>><vladimir.barinov@cogentembedded.com>. > >>> > >>>Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > >>> > >>>--- > >>>This patch is against 'renesas-devel-20150810-v4.2-rc6' tag of Simon Horman's > >>>'renesas.git' repo plus the R8A7794/SILK QSPI patches just re-posted. It needs > >>>the R8A7794 GPIO patches in order to compile. > >>> > >>>Changes in version 2: > >>>- removed not working SDHI0 stuff, renamed the patch; > >>>- replaced SDHI1's "wp-gpios" property with "disable-wp". > >> > >>I am wondering if you could explain the motivation for the "disable-wp" > >>update > > > > Please see the comment in mmc_sd_get_ro(). > > > >>and weather it is appropriate for other r8a779* dts files. > > > > In case of micro-SD slots, yes. > > The MMC binding document says it should only be specified if the > controller has WP detection logic. We, so far, seem to have been replying on > the GPIOs despite this logic is present (according to the R-Car gen2 SDHI > manuals I have). The driver will first call mmc_gpio_get_ro() and when that > fails due to "wp-gpios" not being specified, it proceeds to reading the > register but that is forbidden by TMIO_MMC_WRPROTECT_DISABLE flag set for > the R-Car gen1/2 chips, so 0 is always returned from tmio_mmc_get_ro(). > There seems to be no point in going that far (and doing the runtime PM > dances) -- > and MMC_CAP2_NO_WRITE_PROTECT (set when "disable-wp" is specified) prohibits > doing that... That sounds reasonable to me. Some more questions from my side: * What is the status of this patch * Can you prepare patches for r8a779* dts files for micro-SD slots?
Hello. On 09/02/2015 05:29 AM, Simon Horman wrote: >>>>> Define the SILK board dependent part of the SDHI1 (connected to micro-SD slot) >>>>> device nodes along with the necessary voltage regulators. >>>>> >>>>> Based on the original patch by Vladimir Barinov >>>>> <vladimir.barinov@cogentembedded.com>. >>>>> >>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >>>>> >>>>> --- >>>>> This patch is against 'renesas-devel-20150810-v4.2-rc6' tag of Simon Horman's >>>>> 'renesas.git' repo plus the R8A7794/SILK QSPI patches just re-posted. It needs >>>>> the R8A7794 GPIO patches in order to compile. >>>>> >>>>> Changes in version 2: >>>>> - removed not working SDHI0 stuff, renamed the patch; >>>>> - replaced SDHI1's "wp-gpios" property with "disable-wp". >>>> >>>> I am wondering if you could explain the motivation for the "disable-wp" >>>> update >>> >>> Please see the comment in mmc_sd_get_ro(). >>> >>>> and weather it is appropriate for other r8a779* dts files. >>> >>> In case of micro-SD slots, yes. >> >> The MMC binding document says it should only be specified if the >> controller has WP detection logic. We, so far, seem to have been replying on >> the GPIOs despite this logic is present (according to the R-Car gen2 SDHI >> manuals I have). The driver will first call mmc_gpio_get_ro() and when that >> fails due to "wp-gpios" not being specified, it proceeds to reading the >> register but that is forbidden by TMIO_MMC_WRPROTECT_DISABLE flag set for >> the R-Car gen1/2 chips, so 0 is always returned from tmio_mmc_get_ro(). >> There seems to be no point in going that far (and doing the runtime PM >> dances) -- Alternatively, the driver could be fixed to check the flag before the RPM call unlike what it does now. >> and MMC_CAP2_NO_WRITE_PROTECT (set when "disable-wp" is specified) prohibits >> doing that... > > That sounds reasonable to me. I'm still wondering why TMIO_MMC_WRPROTECT_DISABLE is set for the R-Car SoCs. Morimoto-san, any comments? Your change logs are too terse. :-) > Some more questions from my side: > * What is the status of this patch Tested, working. > * Can you prepare patches for r8a779* dts files for micro-SD slots? Ugh, I probably can but won't promise anything. MBR, Sergei
On Sat, Sep 05, 2015 at 02:47:25AM +0300, Sergei Shtylyov wrote: > Hello. > > On 09/02/2015 05:29 AM, Simon Horman wrote: > > >>>>>Define the SILK board dependent part of the SDHI1 (connected to micro-SD slot) > >>>>>device nodes along with the necessary voltage regulators. > >>>>> > >>>>>Based on the original patch by Vladimir Barinov > >>>>><vladimir.barinov@cogentembedded.com>. > >>>>> > >>>>>Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > >>>>> > >>>>>--- > >>>>>This patch is against 'renesas-devel-20150810-v4.2-rc6' tag of Simon Horman's > >>>>>'renesas.git' repo plus the R8A7794/SILK QSPI patches just re-posted. It needs > >>>>>the R8A7794 GPIO patches in order to compile. > >>>>> > >>>>>Changes in version 2: > >>>>>- removed not working SDHI0 stuff, renamed the patch; > >>>>>- replaced SDHI1's "wp-gpios" property with "disable-wp". > >>>> > >>>>I am wondering if you could explain the motivation for the "disable-wp" > >>>>update > >>> > >>> Please see the comment in mmc_sd_get_ro(). > >>> > >>>>and weather it is appropriate for other r8a779* dts files. > >>> > >>> In case of micro-SD slots, yes. > >> > >> The MMC binding document says it should only be specified if the > >>controller has WP detection logic. We, so far, seem to have been replying on > >>the GPIOs despite this logic is present (according to the R-Car gen2 SDHI > >>manuals I have). The driver will first call mmc_gpio_get_ro() and when that > >>fails due to "wp-gpios" not being specified, it proceeds to reading the > >>register but that is forbidden by TMIO_MMC_WRPROTECT_DISABLE flag set for > >>the R-Car gen1/2 chips, so 0 is always returned from tmio_mmc_get_ro(). > >>There seems to be no point in going that far (and doing the runtime PM > >>dances) -- > > Alternatively, the driver could be fixed to check the flag before the RPM > call unlike what it does now. If the driver can be updated to do the right thing then that seems preferable to me. If so would it be the case that the presence of the "disable-wp" property would not have any run-time effect? > > >>and MMC_CAP2_NO_WRITE_PROTECT (set when "disable-wp" is specified) prohibits > >>doing that... > > > >That sounds reasonable to me. > > I'm still wondering why TMIO_MMC_WRPROTECT_DISABLE is set for the R-Car > SoCs. Morimoto-san, any comments? Your change logs are too terse. :-) I will follow up on this. > >Some more questions from my side: > > >* What is the status of this patch > > Tested, working. > > >* Can you prepare patches for r8a779* dts files for micro-SD slots? > > Ugh, I probably can but won't promise anything. Likewise, ugh.
Hi Simon, On Fri, Sep 18, 2015 at 9:21 AM, Simon Horman <horms@verge.net.au> wrote: > On Sat, Sep 05, 2015 at 02:47:25AM +0300, Sergei Shtylyov wrote: >> Hello. >> >> On 09/02/2015 05:29 AM, Simon Horman wrote: >> >> >>>>>Define the SILK board dependent part of the SDHI1 (connected to micro-SD slot) >> >>>>>device nodes along with the necessary voltage regulators. >> >>>>> >> >>>>>Based on the original patch by Vladimir Barinov >> >>>>><vladimir.barinov@cogentembedded.com>. >> >>>>> >> >>>>>Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >> >>>>> >> >>>>>--- >> >>>>>This patch is against 'renesas-devel-20150810-v4.2-rc6' tag of Simon Horman's >> >>>>>'renesas.git' repo plus the R8A7794/SILK QSPI patches just re-posted. It needs >> >>>>>the R8A7794 GPIO patches in order to compile. >> >>>>> >> >>>>>Changes in version 2: >> >>>>>- removed not working SDHI0 stuff, renamed the patch; >> >>>>>- replaced SDHI1's "wp-gpios" property with "disable-wp". >> >>>> >> >>>>I am wondering if you could explain the motivation for the "disable-wp" >> >>>>update >> >>> >> >>> Please see the comment in mmc_sd_get_ro(). >> >>> >> >>>>and weather it is appropriate for other r8a779* dts files. >> >>> >> >>> In case of micro-SD slots, yes. >> >> >> >> The MMC binding document says it should only be specified if the >> >>controller has WP detection logic. We, so far, seem to have been replying on >> >>the GPIOs despite this logic is present (according to the R-Car gen2 SDHI >> >>manuals I have). The driver will first call mmc_gpio_get_ro() and when that >> >>fails due to "wp-gpios" not being specified, it proceeds to reading the >> >>register but that is forbidden by TMIO_MMC_WRPROTECT_DISABLE flag set for >> >>the R-Car gen1/2 chips, so 0 is always returned from tmio_mmc_get_ro(). >> >>There seems to be no point in going that far (and doing the runtime PM >> >>dances) -- >> >> Alternatively, the driver could be fixed to check the flag before the RPM >> call unlike what it does now. > > If the driver can be updated to do the right thing then that seems > preferable to me. If so would it be the case that the presence of the > "disable-wp" property would not have any run-time effect? >> >> >>and MMC_CAP2_NO_WRITE_PROTECT (set when "disable-wp" is specified) prohibits >> >>doing that... >> > >> >That sounds reasonable to me. >> >> I'm still wondering why TMIO_MMC_WRPROTECT_DISABLE is set for the R-Car >> SoCs. Morimoto-san, any comments? Your change logs are too terse. :-) > > I will follow up on this. I'd like to help you to clarify whatever issues you are having, but I don't have anyhardware access or schematics for the Silk board. So here is some generic information. In general with SD/MMC I can imagine a couple of different types of devices. 1) Hotpluggable SD socket (with write protect) 2) Hotpluggable micro-SD socket (no write protect) 3) On-board SD/MMC/SDIO (no hotplug, no write protect) Which type of device that happens to be used on a particular board is a board specific property. So on one board SDHI1 may be micro-SD but on a different board SDHI1 may be a soldered-on eMMC or a SDIO module (case 3). The same SoC DTSI will contain on-chip information for SDHI1 while the board specific DTS may contain optional information about WP and CD signals. On R-Car Gen2 we default to assume case 3 above which means no WP and CD signals. If a regular SD socket is used then the board specific file needs to point out CD and WP signals. For micro-SD sockets there is no WP signal so in such case only the CD pin is needed. HIstorically with SDHI the availability of CD and WP signals vary wildly depending on SoC and SDHI instance. Also, with power domains and Runtime PM we would like to be able to power off the power domain for the SDHI device and still be able to wake up, so assigning CD and WP as GPIO signals will not only allow board-specific stuff to be cleanly separated from the on-chip SDHI DTSI, it also allows for easier review and better power management. Now what is the issue that you guys are having? >> >Some more questions from my side: >> >> >* What is the status of this patch >> >> Tested, working. >> >> >* Can you prepare patches for r8a779* dts files for micro-SD slots? >> >> Ugh, I probably can but won't promise anything. > > Likewise, ugh. What needs to be updated for R-Car Gen1 again? Thanks, / magnus
Hello. On 09/18/2015 05:56 AM, Magnus Damm wrote: >>>>>>>> Define the SILK board dependent part of the SDHI1 (connected to micro-SD slot) >>>>>>>> device nodes along with the necessary voltage regulators. >>>>>>>> >>>>>>>> Based on the original patch by Vladimir Barinov >>>>>>>> <vladimir.barinov@cogentembedded.com>. >>>>>>>> >>>>>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >>>>>>>> >>>>>>>> --- >>>>>>>> This patch is against 'renesas-devel-20150810-v4.2-rc6' tag of Simon Horman's >>>>>>>> 'renesas.git' repo plus the R8A7794/SILK QSPI patches just re-posted. It needs >>>>>>>> the R8A7794 GPIO patches in order to compile. >>>>>>>> >>>>>>>> Changes in version 2: >>>>>>>> - removed not working SDHI0 stuff, renamed the patch; >>>>>>>> - replaced SDHI1's "wp-gpios" property with "disable-wp". >>>>>>> >>>>>>> I am wondering if you could explain the motivation for the "disable-wp" >>>>>>> update >>>>>> >>>>>> Please see the comment in mmc_sd_get_ro(). >>>>>> >>>>>>> and weather it is appropriate for other r8a779* dts files. >>>>>> >>>>>> In case of micro-SD slots, yes. >>>>> >>>>> The MMC binding document says it should only be specified if the >>>>> controller has WP detection logic. We, so far, seem to have been replying on >>>>> the GPIOs despite this logic is present (according to the R-Car gen2 SDHI >>>>> manuals I have). The driver will first call mmc_gpio_get_ro() and when that >>>>> fails due to "wp-gpios" not being specified, it proceeds to reading the >>>>> register but that is forbidden by TMIO_MMC_WRPROTECT_DISABLE flag set for >>>>> the R-Car gen1/2 chips, so 0 is always returned from tmio_mmc_get_ro(). >>>>> There seems to be no point in going that far (and doing the runtime PM >>>>> dances) -- >>> >>> Alternatively, the driver could be fixed to check the flag before the RPM >>> call unlike what it does now. >> >> If the driver can be updated to do the right thing then that seems >> preferable to me. If so would it be the case that the presence of the >> "disable-wp" property would not have any run-time effect? >>> >>>>> and MMC_CAP2_NO_WRITE_PROTECT (set when "disable-wp" is specified) prohibits >>>>> doing that... >>>> >>>> That sounds reasonable to me. >>> >>> I'm still wondering why TMIO_MMC_WRPROTECT_DISABLE is set for the R-Car >>> SoCs. Morimoto-san, any comments? Your change logs are too terse. :-) >> >> I will follow up on this. [...] > Now what is the issue that you guys are having? My main issue is that I don't understand why checking the write protect bit is always prohibited for the R-Car SoCs (by setting TMIO_MMC_WRPROTECT_DISABLE), i.e. it can only be read via GPIO (though that GPIO is just an alias of the WP signal). >>>> Some more questions from my side: >>> >>>> * What is the status of this patch >>> >>> Tested, working. >>> >>>> * Can you prepare patches for r8a779* dts files for micro-SD slots? >>> >>> Ugh, I probably can but won't promise anything. >> >> Likewise, ugh. > What needs to be updated for R-Car Gen1 again? We haven't reached the agreement about the need for update yet. > Thanks, > / magnus MBR, Sergei
Hello. On 09/18/2015 03:21 AM, Simon Horman wrote: >>>>>>> Define the SILK board dependent part of the SDHI1 (connected to micro-SD slot) >>>>>>> device nodes along with the necessary voltage regulators. >>>>>>> >>>>>>> Based on the original patch by Vladimir Barinov >>>>>>> <vladimir.barinov@cogentembedded.com>. >>>>>>> >>>>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >>>>>>> >>>>>>> --- >>>>>>> This patch is against 'renesas-devel-20150810-v4.2-rc6' tag of Simon Horman's >>>>>>> 'renesas.git' repo plus the R8A7794/SILK QSPI patches just re-posted. It needs >>>>>>> the R8A7794 GPIO patches in order to compile. >>>>>>> >>>>>>> Changes in version 2: >>>>>>> - removed not working SDHI0 stuff, renamed the patch; >>>>>>> - replaced SDHI1's "wp-gpios" property with "disable-wp". >>>>>> >>>>>> I am wondering if you could explain the motivation for the "disable-wp" >>>>>> update >>>>> >>>>> Please see the comment in mmc_sd_get_ro(). >>>>> >>>>>> and weather it is appropriate for other r8a779* dts files. >>>>> >>>>> In case of micro-SD slots, yes. >>>> >>>> The MMC binding document says it should only be specified if the >>>> controller has WP detection logic. We, so far, seem to have been replying on >>>> the GPIOs despite this logic is present (according to the R-Car gen2 SDHI >>>> manuals I have). The driver will first call mmc_gpio_get_ro() and when that >>>> fails due to "wp-gpios" not being specified, it proceeds to reading the >>>> register but that is forbidden by TMIO_MMC_WRPROTECT_DISABLE flag set for >>>> the R-Car gen1/2 chips, so 0 is always returned from tmio_mmc_get_ro(). >>>> There seems to be no point in going that far (and doing the runtime PM >>>> dances) -- >> >> Alternatively, the driver could be fixed to check the flag before the RPM >> call unlike what it does now. > > If the driver can be updated to do the right thing then that seems OK, I'll try going this way... > preferable to me. If so would it be the case that the presence of the > "disable-wp" property would not have any run-time effect? Yes. MBR, Sergei
Hi Sergei, On Wed, Sep 23, 2015 at 8:19 AM, Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote: > Hello. > > > On 09/18/2015 05:56 AM, Magnus Damm wrote: > >>>>>>>>> Define the SILK board dependent part of the SDHI1 (connected to >>>>>>>>> micro-SD slot) >>>>>>>>> device nodes along with the necessary voltage regulators. >>>>>>>>> >>>>>>>>> Based on the original patch by Vladimir Barinov >>>>>>>>> <vladimir.barinov@cogentembedded.com>. >>>>>>>>> >>>>>>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >>>>>>>>> >>>>>>>>> --- >>>>>>>>> This patch is against 'renesas-devel-20150810-v4.2-rc6' tag of >>>>>>>>> Simon Horman's >>>>>>>>> 'renesas.git' repo plus the R8A7794/SILK QSPI patches just >>>>>>>>> re-posted. It needs >>>>>>>>> the R8A7794 GPIO patches in order to compile. >>>>>>>>> >>>>>>>>> Changes in version 2: >>>>>>>>> - removed not working SDHI0 stuff, renamed the patch; >>>>>>>>> - replaced SDHI1's "wp-gpios" property with "disable-wp". >>>>>>>> >>>>>>>> >>>>>>>> I am wondering if you could explain the motivation for the >>>>>>>> "disable-wp" >>>>>>>> update >>>>>>> >>>>>>> >>>>>>> Please see the comment in mmc_sd_get_ro(). >>>>>>> >>>>>>>> and weather it is appropriate for other r8a779* dts files. >>>>>>> >>>>>>> >>>>>>> In case of micro-SD slots, yes. >>>>>> >>>>>> >>>>>> The MMC binding document says it should only be specified if the >>>>>> controller has WP detection logic. We, so far, seem to have been >>>>>> replying on >>>>>> the GPIOs despite this logic is present (according to the R-Car gen2 >>>>>> SDHI >>>>>> manuals I have). The driver will first call mmc_gpio_get_ro() and when >>>>>> that >>>>>> fails due to "wp-gpios" not being specified, it proceeds to reading >>>>>> the >>>>>> register but that is forbidden by TMIO_MMC_WRPROTECT_DISABLE flag set >>>>>> for >>>>>> the R-Car gen1/2 chips, so 0 is always returned from >>>>>> tmio_mmc_get_ro(). >>>>>> There seems to be no point in going that far (and doing the runtime PM >>>>>> dances) -- >>>> >>>> >>>> Alternatively, the driver could be fixed to check the flag before >>>> the RPM >>>> call unlike what it does now. >>> >>> >>> If the driver can be updated to do the right thing then that seems >>> preferable to me. If so would it be the case that the presence of the >>> "disable-wp" property would not have any run-time effect? >>>> >>>> >>>>>> and MMC_CAP2_NO_WRITE_PROTECT (set when "disable-wp" is specified) >>>>>> prohibits >>>>>> doing that... >>>>> >>>>> >>>>> That sounds reasonable to me. >>>> >>>> >>>> I'm still wondering why TMIO_MMC_WRPROTECT_DISABLE is set for the >>>> R-Car >>>> SoCs. Morimoto-san, any comments? Your change logs are too terse. :-) >>> >>> >>> I will follow up on this. > > > [...] >> >> Now what is the issue that you guys are having? > > > My main issue is that I don't understand why checking the write protect > bit is always prohibited for the R-Car SoCs (by setting > TMIO_MMC_WRPROTECT_DISABLE), i.e. it can only be read via GPIO (though that > GPIO is just an alias of the WP signal). I believe the reason is that we decided to keep it simple - so we preferred to use GPIO instead of native SDHI signals. So GPIO WP instead of the not-always-present SDHI WP signal. Historically CD and WP may on some boards be routed on different pins than the SDHI CD and WP lines, and if we support both GPIO and native SDHI signals we need to handle both cases. With GPIO there is only one case to handle. And it is not exactly hot path to handle WP and CD so the overhead must be minimal. Historically there have been were cases with people submitting board support patches and getting the WP signal wrong. It seems difficult for people to understand that the WP signal is missing in case of micro-SD sockets. With the incorrect WP signal the cards would end up being read only. Combine that with lack of board schematics and it all turns into a big mess. The on-chip SoC SDHI devices in DT and the driver on R-Car Gen2 assumes no WP and CD signals by default. It is up to each board to opt-in to add the GPIOs for WP and CD. It is very simple and should make it possible to power down the SDHI instances if no cards are inserted and let the GPIO IRQ wake up things for us. I still don't understand what the real problem is though... Thanks, / magnus
Hello. Sorry for tyhe long delay, I've been busy with other things. Now I'm dealing with SDHI again, this time for the Porter board. On 09/29/2015 11:44 AM, Magnus Damm wrote: >>>>>>>>>> Define the SILK board dependent part of the SDHI1 (connected to >>>>>>>>>> micro-SD slot) >>>>>>>>>> device nodes along with the necessary voltage regulators. >>>>>>>>>> >>>>>>>>>> Based on the original patch by Vladimir Barinov >>>>>>>>>> <vladimir.barinov@cogentembedded.com>. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >>>>>>>>>> >>>>>>>>>> --- >>>>>>>>>> This patch is against 'renesas-devel-20150810-v4.2-rc6' tag of >>>>>>>>>> Simon Horman's >>>>>>>>>> 'renesas.git' repo plus the R8A7794/SILK QSPI patches just >>>>>>>>>> re-posted. It needs >>>>>>>>>> the R8A7794 GPIO patches in order to compile. >>>>>>>>>> >>>>>>>>>> Changes in version 2: >>>>>>>>>> - removed not working SDHI0 stuff, renamed the patch; >>>>>>>>>> - replaced SDHI1's "wp-gpios" property with "disable-wp". >>>>>>>>> >>>>>>>>> >>>>>>>>> I am wondering if you could explain the motivation for the >>>>>>>>> "disable-wp" >>>>>>>>> update >>>>>>>> >>>>>>>> >>>>>>>> Please see the comment in mmc_sd_get_ro(). >>>>>>>> >>>>>>>>> and weather it is appropriate for other r8a779* dts files. >>>>>>>> >>>>>>>> >>>>>>>> In case of micro-SD slots, yes. >>>>>>> >>>>>>> >>>>>>> The MMC binding document says it should only be specified if the >>>>>>> controller has WP detection logic. We, so far, seem to have been >>>>>>> replying on >>>>>>> the GPIOs despite this logic is present (according to the R-Car gen2 >>>>>>> SDHI >>>>>>> manuals I have). The driver will first call mmc_gpio_get_ro() and when >>>>>>> that >>>>>>> fails due to "wp-gpios" not being specified, it proceeds to reading >>>>>>> the >>>>>>> register but that is forbidden by TMIO_MMC_WRPROTECT_DISABLE flag set >>>>>>> for >>>>>>> the R-Car gen1/2 chips, so 0 is always returned from >>>>>>> tmio_mmc_get_ro(). >>>>>>> There seems to be no point in going that far (and doing the runtime PM >>>>>>> dances) -- >>>>> >>>>> >>>>> Alternatively, the driver could be fixed to check the flag before >>>>> the RPM >>>>> call unlike what it does now. >>>> >>>> >>>> If the driver can be updated to do the right thing then that seems >>>> preferable to me. If so would it be the case that the presence of the >>>> "disable-wp" property would not have any run-time effect? >>>>> >>>>> >>>>>>> and MMC_CAP2_NO_WRITE_PROTECT (set when "disable-wp" is specified) >>>>>>> prohibits >>>>>>> doing that... >>>>>> >>>>>> >>>>>> That sounds reasonable to me. >>>>> >>>>> >>>>> I'm still wondering why TMIO_MMC_WRPROTECT_DISABLE is set for the >>>>> R-Car >>>>> SoCs. Morimoto-san, any comments? Your change logs are too terse. :-) >>>> >>>> >>>> I will follow up on this. >> >> >> [...] >>> >>> Now what is the issue that you guys are having? >> >> >> My main issue is that I don't understand why checking the write protect >> bit is always prohibited for the R-Car SoCs (by setting >> TMIO_MMC_WRPROTECT_DISABLE), i.e. it can only be read via GPIO (though that >> GPIO is just an alias of the WP signal). > I believe the reason is that we decided to keep it simple - so we > preferred to use GPIO instead of native SDHI signals. So GPIO WP > instead of the not-always-present SDHI WP signal. Historically CD and > WP may on some boards be routed on different pins than the SDHI CD and > WP lines, and if we support both GPIO and native SDHI signals we need > to handle both cases. If you look at the driver code, it's already capable of handling both cases. > With GPIO there is only one case to handle. And > it is not exactly hot path to handle WP and CD so the overhead must be > minimal. [...] > The on-chip SoC SDHI devices in DT and the driver on R-Car Gen2 > assumes no WP and CD signals by default. It is up to each board to > opt-in to add the GPIOs for WP and CD. It is very simple and should > make it possible to power down the SDHI instances if no cards are > inserted and let the GPIO IRQ wake up things for us. Again, if you look at the driver code, it first powers up the controller, (thru runtime PM) and only then checks the TMIO_MMC_WRPROTECT_DISABLE flag. That's what I tried to change but didn't succeed because the current MMC code will have already powered up the controller by that time. > I still don't understand what the real problem is though... The original issue revolved around the "disable-wp" prop. The common MMC bindings say that this prop should only be used "when the controller has a dedicated write-protect detection logic". This logic is obviously present but its use seems suppressed on the R-Car SoCs by the infamous flag... :-) > Thanks, > > / magnus MBR, Sergei
Index: renesas/arch/arm/boot/dts/r8a7794-silk.dts =================================================================== --- renesas.orig/arch/arm/boot/dts/r8a7794-silk.dts +++ renesas/arch/arm/boot/dts/r8a7794-silk.dts @@ -12,6 +12,7 @@ /dts-v1/; #include "r8a7794.dtsi" +#include <dt-bindings/gpio/gpio.h> / { model = "SILK"; @@ -39,6 +40,29 @@ regulator-boot-on; regulator-always-on; }; + vcc_sdhi1: regulator@3 { + compatible = "regulator-fixed"; + + regulator-name = "SDHI1 Vcc"; + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + + gpio = <&gpio4 26 GPIO_ACTIVE_HIGH>; + enable-active-high; + }; + + vccq_sdhi1: regulator@4 { + compatible = "regulator-gpio"; + + regulator-name = "SDHI1 VccQ"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <3300000>; + + gpios = <&gpio4 29 GPIO_ACTIVE_HIGH>; + gpios-states = <1>; + states = <3300000 1 + 1800000 0>; + }; }; &extal_clk { @@ -66,6 +90,11 @@ renesas,function = "mmc"; }; + sdhi1_pins: sd1 { + renesas,groups = "sdhi1_data4", "sdhi1_ctrl"; + renesas,function = "sdhi1"; + }; + qspi_pins: spi0 { renesas,groups = "qspi_ctrl", "qspi_data4"; renesas,function = "qspi"; @@ -106,6 +135,17 @@ status = "okay"; }; +&sdhi1 { + pinctrl-0 = <&sdhi1_pins>; + pinctrl-names = "default"; + + vmmc-supply = <&vcc_sdhi1>; + vqmmc-supply = <&vccq_sdhi1>; + cd-gpios = <&gpio6 14 GPIO_ACTIVE_LOW>; + disable-wp; + status = "okay"; +}; + &qspi { pinctrl-0 = <&qspi_pins>; pinctrl-names = "default";
Define the SILK board dependent part of the SDHI1 (connected to micro-SD slot) device nodes along with the necessary voltage regulators. Based on the original patch by Vladimir Barinov <vladimir.barinov@cogentembedded.com>. Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> --- This patch is against 'renesas-devel-20150810-v4.2-rc6' tag of Simon Horman's 'renesas.git' repo plus the R8A7794/SILK QSPI patches just re-posted. It needs the R8A7794 GPIO patches in order to compile. Changes in version 2: - removed not working SDHI0 stuff, renamed the patch; - replaced SDHI1's "wp-gpios" property with "disable-wp". arch/arm/boot/dts/r8a7794-silk.dts | 40 +++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)