[4/5,RESENT] mmc: SDHI: update sh_mobile_sdhi_of_data for r8a7790

Commit Message

Kuninori Morimoto Feb. 21, 2014, 12:55 a.m. UTC

From: Kuninori Morimoto <[e-mail]>

This patch updates r8a7790 DT data to have SoC specific settings.

Acked-by: Simon Horman <[e-mail]>
Signed-off-by: Kuninori Morimoto <[e-mail]>
---
 drivers/mmc/host/sh_mobile_sdhi.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Sergei Shtylyov May 2, 2014, 11:55 p.m. UTC | #1

Hello.

On 02/21/2014 03:55 AM, Kuninori Morimoto wrote:

> From: Kuninori Morimoto <[e-mail]>

> This patch updates r8a7790 DT data to have SoC specific settings.

> Acked-by: Simon Horman <[e-mail]>
> Signed-off-by: Kuninori Morimoto <[e-mail]>
> ---
>   drivers/mmc/host/sh_mobile_sdhi.c |   10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)

> diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
> index 38acf26..cfc37ec 100644
> --- a/drivers/mmc/host/sh_mobile_sdhi.c
> +++ b/drivers/mmc/host/sh_mobile_sdhi.c
[...]
> @@ -51,6 +52,12 @@ static const struct sh_mobile_sdhi_of_data of_rcar_gen1_compatible = {
>   	.capabilities	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ,
>   };
>
> +static const struct sh_mobile_sdhi_of_data of_rcar_gen2_compatible = {
> +	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_WRPROTECT_DISABLE,

    Hm, I think SDHI0/1 still have the WP signal, only SDHI2 on Koelsch lacks 
it due to using micro-SD slot?..

WBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Magnus Damm May 3, 2014, 7:23 p.m. UTC | #2

Hi Sergei,

On Fri, May 2, 2014 at 11:55 PM, Sergei Shtylyov
<[e-mail]> wrote:
> Hello.
>
>
> On 02/21/2014 03:55 AM, Kuninori Morimoto wrote:
>
>> From: Kuninori Morimoto <[e-mail]>
>
>
>> This patch updates r8a7790 DT data to have SoC specific settings.
>
>
>> Acked-by: Simon Horman <[e-mail]>
>> Signed-off-by: Kuninori Morimoto <[e-mail]>
>> ---
>>   drivers/mmc/host/sh_mobile_sdhi.c |   10 +++++++++-
>>   1 file changed, 9 insertions(+), 1 deletion(-)
>
>
>> diff --git a/drivers/mmc/host/sh_mobile_sdhi.c
>> b/drivers/mmc/host/sh_mobile_sdhi.c
>> index 38acf26..cfc37ec 100644
>> --- a/drivers/mmc/host/sh_mobile_sdhi.c
>> +++ b/drivers/mmc/host/sh_mobile_sdhi.c
>
> [...]
>
>> @@ -51,6 +52,12 @@ static const struct sh_mobile_sdhi_of_data
>> of_rcar_gen1_compatible = {
>>         .capabilities   = MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ,
>>   };
>>
>> +static const struct sh_mobile_sdhi_of_data of_rcar_gen2_compatible = {
>> +       .tmio_flags     = TMIO_MMC_HAS_IDLE_WAIT |
>> TMIO_MMC_WRPROTECT_DISABLE,
>
>
>    Hm, I think SDHI0/1 still have the WP signal, only SDHI2 on Koelsch lacks
> it due to using micro-SD slot?..

This has been design this way intentionally. Basically, we don't want
any board specific code in any driver and the WP signal is a very
board specific property. So we default to not using WP, but let the
user hook in GPIO via DT in case it is used on a particular instance
on a particular board. If we do it the other way around and enable by
default then we would have to implement disabling of WP in DT which is
just strange. If done wrong then the micro-SD cards may end up with
incorrect WP signal state (read only?). So letting board code hook in
board specific bits via DT seems like the only sane way forward in my
mind.

Is there anything related to WP and CD that does not work for you?

Thanks,

/ magnus
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sergei Shtylyov May 3, 2014, 9:30 p.m. UTC | #3

Hello.

On 05/03/2014 11:23 PM, Magnus Damm wrote:

>>> From: Kuninori Morimoto <[e-mail]>

>>> This patch updates r8a7790 DT data to have SoC specific settings.

>>> Acked-by: Simon Horman <[e-mail]>
>>> Signed-off-by: Kuninori Morimoto <[e-mail]>
>>> ---
>>>    drivers/mmc/host/sh_mobile_sdhi.c |   10 +++++++++-
>>>    1 file changed, 9 insertions(+), 1 deletion(-)

>>> diff --git a/drivers/mmc/host/sh_mobile_sdhi.c
>>> b/drivers/mmc/host/sh_mobile_sdhi.c
>>> index 38acf26..cfc37ec 100644
>>> --- a/drivers/mmc/host/sh_mobile_sdhi.c
>>> +++ b/drivers/mmc/host/sh_mobile_sdhi.c

>> [...]

>>> @@ -51,6 +52,12 @@ static const struct sh_mobile_sdhi_of_data
>>> of_rcar_gen1_compatible = {
>>>          .capabilities   = MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ,
>>>    };

>>> +static const struct sh_mobile_sdhi_of_data of_rcar_gen2_compatible = {
>>> +       .tmio_flags     = TMIO_MMC_HAS_IDLE_WAIT |
>>> TMIO_MMC_WRPROTECT_DISABLE,

>>     Hm, I think SDHI0/1 still have the WP signal, only SDHI2 on Koelsch lacks
>> it due to using micro-SD slot?..

> This has been design this way intentionally. Basically, we don't want
> any board specific code in any driver and the WP signal is a very
> board specific property. So we default to not using WP, but let the
> user hook in GPIO via DT in case it is used on a particular instance
> on a particular board. If we do it the other way around and enable by
> default then we would have to implement disabling of WP in DT which is
> just strange.

    I don't quite understand what's so strange about that. IMO, the device 
node properties are designed to carry the board specific info.

> If done wrong then the micro-SD cards may end up with
> incorrect WP signal state (read only?).

    Hm, isn't WP a read-only signal by design?

> So letting board code hook in
> board specific bits via DT seems like the only sane way forward in my
> mind.

    Yes, I'd agree to that but I can't agree that preferring GPIO to the 
native WP signal is the only sane way.

> Is there anything related to WP and CD that does not work for you?

    I don't think the legacy code handles CD correctly. The driver doesn't 
ever read the native CD signal, it expects CD to be always implemented via 
GPIO while the legacy code doesn't pass the GPIO to the driver.

> Thanks,

> / magnus

WBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Magnus Damm May 4, 2014, 8:01 a.m. UTC | #4

On Sun, May 4, 2014 at 6:30 AM, Sergei Shtylyov
<[e-mail]> wrote:
> Hello.
>
>
> On 05/03/2014 11:23 PM, Magnus Damm wrote:
>
>>>> From: Kuninori Morimoto <[e-mail]>
>
>
>>>> This patch updates r8a7790 DT data to have SoC specific settings.
>
>
>>>> Acked-by: Simon Horman <[e-mail]>
>>>> Signed-off-by: Kuninori Morimoto <[e-mail]>
>>>> ---
>>>>    drivers/mmc/host/sh_mobile_sdhi.c |   10 +++++++++-
>>>>    1 file changed, 9 insertions(+), 1 deletion(-)
>
>
>>>> diff --git a/drivers/mmc/host/sh_mobile_sdhi.c
>>>> b/drivers/mmc/host/sh_mobile_sdhi.c
>>>> index 38acf26..cfc37ec 100644
>>>> --- a/drivers/mmc/host/sh_mobile_sdhi.c
>>>> +++ b/drivers/mmc/host/sh_mobile_sdhi.c
>
>
>>> [...]
>
>
>>>> @@ -51,6 +52,12 @@ static const struct sh_mobile_sdhi_of_data
>>>> of_rcar_gen1_compatible = {
>>>>          .capabilities   = MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ,
>>>>    };
>
>
>>>> +static const struct sh_mobile_sdhi_of_data of_rcar_gen2_compatible = {
>>>> +       .tmio_flags     = TMIO_MMC_HAS_IDLE_WAIT |
>>>> TMIO_MMC_WRPROTECT_DISABLE,
>
>
>>>     Hm, I think SDHI0/1 still have the WP signal, only SDHI2 on Koelsch
>>> lacks
>>> it due to using micro-SD slot?..
>
>
>> This has been design this way intentionally. Basically, we don't want
>> any board specific code in any driver and the WP signal is a very
>> board specific property. So we default to not using WP, but let the
>> user hook in GPIO via DT in case it is used on a particular instance
>> on a particular board. If we do it the other way around and enable by
>> default then we would have to implement disabling of WP in DT which is
>> just strange.
>
>
>    I don't quite understand what's so strange about that. IMO, the device
> node properties are designed to carry the board specific info.

This is all about wanting sane defaults and keeping the code simple.
Using GPIOs covers 100% of all use cases on R-Car Gen2 and it allows
us to use generic DT properties in the board DTS to enable WP and CD.
Any other native CD or WP handling is just a corner case.

You may be confused by the difference between the R-Car Gen1 case and
the R-Car Gen2 case when it comes to CD and WP handling in the SDHI
driver. The reason why we don't rely 100% on GPIO for Gen1 is that the
GPIO hardware does not support IRQ trigger with both edges. In R-Car
Gen2 case the GPIO should cover all pins on the system and allows for
both edge trigger.

Using the native CD (and maybe WP) functionality of the SDHI hardware
may conflict with Runtime PM. Ideally we want to be able to disable
the clock and power domain when no card is inserted or the card is
in-use but idle. Using GPIO helps us with that.

>> If done wrong then the micro-SD cards may end up with
>> incorrect WP signal state (read only?).
>
>
>    Hm, isn't WP a read-only signal by design?

The _card_ may end up read-only if WP is used incorrectly.

>> So letting board code hook in
>> board specific bits via DT seems like the only sane way forward in my
>> mind.
>
>
>    Yes, I'd agree to that but I can't agree that preferring GPIO to the
> native WP signal is the only sane way.

So why would we want to have multiple overlapping ways of handling the
WP signal?

>> Is there anything related to WP and CD that does not work for you?
>
>
>    I don't think the legacy code handles CD correctly. The driver doesn't
> ever read the native CD signal, it expects CD to be always implemented via
> GPIO while the legacy code doesn't pass the GPIO to the driver.

Are you sure about this? If you find a bug then please report it,
otherwise I can think of more interesting things to spend time on.

Cheers,

/ magnus
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index 38acf26..cfc37ec 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -38,6 +38,7 @@ 
 struct sh_mobile_sdhi_of_data {
 	unsigned long tmio_flags;
 	unsigned long capabilities;
+	unsigned long capabilities2;
 };
 
 static const struct sh_mobile_sdhi_of_data sh_mobile_sdhi_of_cfg[] = {
@@ -51,6 +52,12 @@  static const struct sh_mobile_sdhi_of_data of_rcar_gen1_compatible = {
 	.capabilities	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ,
 };
 
+static const struct sh_mobile_sdhi_of_data of_rcar_gen2_compatible = {
+	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_WRPROTECT_DISABLE,
+	.capabilities	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ,
+	.capabilities2	= MMC_CAP2_NO_MULTI_READ,
+};
+
 static const struct of_device_id sh_mobile_sdhi_of_match[] = {
 	{ .compatible = "renesas,sdhi-shmobile" },
 	{ .compatible = "renesas,sdhi-sh7372" },
@@ -59,7 +66,7 @@  static const struct of_device_id sh_mobile_sdhi_of_match[] = {
 	{ .compatible = "renesas,sdhi-r8a7740", .data = &sh_mobile_sdhi_of_cfg[0], },
 	{ .compatible = "renesas,sdhi-r8a7778", .data = &of_rcar_gen1_compatible, },
 	{ .compatible = "renesas,sdhi-r8a7779", .data = &of_rcar_gen1_compatible, },
-	{ .compatible = "renesas,sdhi-r8a7790", .data = &sh_mobile_sdhi_of_cfg[0], },
+	{ .compatible = "renesas,sdhi-r8a7790", .data = &of_rcar_gen2_compatible, },
 	{},
 };
 MODULE_DEVICE_TABLE(of, sh_mobile_sdhi_of_match);
@@ -219,6 +226,7 @@  static int sh_mobile_sdhi_probe(struct platform_device *pdev)
 		const struct sh_mobile_sdhi_of_data *of_data = of_id->data;
 		mmc_data->flags |= of_data->tmio_flags;
 		mmc_data->capabilities |= of_data->capabilities;
+		mmc_data->capabilities2 |= of_data->capabilities2;
 	}
 
 	/* SD control register space size is 0x100, 0x200 for bus_shift=1 */