[PATCH v2 1/2] mmc: sdhci-pltfm: add calling mmc_of_parse in sdhci_pltfm_register

[PATCH v2 1/2] mmc: sdhci-pltfm: add calling mmc_of_parse in sdhci_pltfm_register

[Kevin Liu]

commit 6c56e7a0 provide a function mmc_of_parse for standard MMC
device-tree binding parser centrally. So just call it with
sdhci_get_of_property together in sdhci_pltfm_register.

Signed-off-by: Kevin Liu <kliu5@marvell.com>
---
 drivers/mmc/host/sdhci-pltfm.c |   56 +++++++++++++++++++++-------------------
 1 file changed, 29 insertions(+), 27 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
index e605509..0c5eb6a 100644
--- a/drivers/mmc/host/sdhci-pltfm.c
+++ b/drivers/mmc/host/sdhci-pltfm.c
@@ -71,42 +71,43 @@ void sdhci_get_of_property(struct platform_device *pdev)
 	u32 bus_width;
 	int size;
 
-	if (of_device_is_available(np)) {
-		if (of_get_property(np, "sdhci,auto-cmd12", NULL))
-			host->quirks |= SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12;
+	if (!of_device_is_available(np))
+		return;
 
-		if (of_get_property(np, "sdhci,1-bit-only", NULL) ||
-		    (of_property_read_u32(np, "bus-width", &bus_width) == 0 &&
-		    bus_width == 1))
-			host->quirks |= SDHCI_QUIRK_FORCE_1_BIT_DATA;
+	if (of_get_property(np, "sdhci,auto-cmd12", NULL))
+		host->quirks |= SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12;
 
-		if (sdhci_of_wp_inverted(np))
-			host->quirks |= SDHCI_QUIRK_INVERTED_WRITE_PROTECT;
+	if (of_get_property(np, "sdhci,1-bit-only", NULL) ||
+		(of_property_read_u32(np, "bus-width", &bus_width) == 0 &&
+		bus_width == 1))
+		host->quirks |= SDHCI_QUIRK_FORCE_1_BIT_DATA;
 
-		if (of_get_property(np, "broken-cd", NULL))
-			host->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION;
+	if (sdhci_of_wp_inverted(np))
+		host->quirks |= SDHCI_QUIRK_INVERTED_WRITE_PROTECT;
 
-		if (of_get_property(np, "no-1-8-v", NULL))
-			host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
+	if (of_get_property(np, "broken-cd", NULL))
+		host->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION;
 
-		if (of_device_is_compatible(np, "fsl,p2020-rev1-esdhc"))
-			host->quirks |= SDHCI_QUIRK_BROKEN_DMA;
+	if (of_get_property(np, "no-1-8-v", NULL))
+		host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
 
-		if (of_device_is_compatible(np, "fsl,p2020-esdhc") ||
-		    of_device_is_compatible(np, "fsl,p1010-esdhc") ||
-		    of_device_is_compatible(np, "fsl,mpc8536-esdhc"))
-			host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
+	if (of_device_is_compatible(np, "fsl,p2020-rev1-esdhc"))
+		host->quirks |= SDHCI_QUIRK_BROKEN_DMA;
 
-		clk = of_get_property(np, "clock-frequency", &size);
-		if (clk && size == sizeof(*clk) && *clk)
-			pltfm_host->clock = be32_to_cpup(clk);
+	if (of_device_is_compatible(np, "fsl,p2020-esdhc") ||
+			of_device_is_compatible(np, "fsl,p1010-esdhc") ||
+			of_device_is_compatible(np, "fsl,mpc8536-esdhc"))
+		host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
 
-		if (of_find_property(np, "keep-power-in-suspend", NULL))
-			host->mmc->pm_caps |= MMC_PM_KEEP_POWER;
+	clk = of_get_property(np, "clock-frequency", &size);
+	if (clk && size == sizeof(*clk) && *clk)
+		pltfm_host->clock = be32_to_cpup(clk);
 
-		if (of_find_property(np, "enable-sdio-wakeup", NULL))
-			host->mmc->pm_caps |= MMC_PM_WAKE_SDIO_IRQ;
-	}
+	if (of_find_property(np, "keep-power-in-suspend", NULL))
+		host->mmc->pm_caps |= MMC_PM_KEEP_POWER;
+
+	if (of_find_property(np, "enable-sdio-wakeup", NULL))
+		host->mmc->pm_caps |= MMC_PM_WAKE_SDIO_IRQ;
 }
 #else
 void sdhci_get_of_property(struct platform_device *pdev) {}
@@ -212,6 +213,7 @@ int sdhci_pltfm_register(struct platform_device *pdev,
 	if (IS_ERR(host))
 		return PTR_ERR(host);
 
+	mmc_of_parse(host->mmc);
 	sdhci_get_of_property(pdev);
 
 	ret = sdhci_add_host(host);
-- 
1.7.9.5

[PATCH 2/2] mmc: sdhci-pltfm: add function in sdhci_ops to parse dt property

[Kevin Liu]

For sdhci based drivers, it's better to parse their host specific
device tree properties in their host's driver (sdhci-xxx.c) rather
than in the shared sdhci platform driver (sdhci-pltfm.c).
Otherwise function sdhci_get_of_property need to parse every
host's private dt property and will be bigger and bigger.

This patch add a function of_get_property in sdhci_ops to parse
host specific device tree property.

This patch also move Freescale eSDHC specific dt property parse code
from sdhci_get_of_property to of_get_property in its driver.

Signed-off-by: Kevin Liu <kliu5@marvell.com>
---
 drivers/mmc/host/sdhci-of-esdhc.c |   19 +++++++++++++++++++
 drivers/mmc/host/sdhci-pltfm.c    |   11 +++--------
 drivers/mmc/host/sdhci.h          |    1 +
 3 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
index f32526d..7df98b0 100644
--- a/drivers/mmc/host/sdhci-of-esdhc.c
+++ b/drivers/mmc/host/sdhci-of-esdhc.c
@@ -230,6 +230,24 @@ static void esdhc_of_platform_init(struct sdhci_host *host)
 		host->quirks &= ~SDHCI_QUIRK_NO_BUSY_IRQ;
 }
 
+static void esdhc_of_get_property(struct sdhci_host *host)
+{
+	struct device_node *np;
+
+	if (!host->mmc->parent || !host->mmc->parent->of_node)
+		return;
+
+	np = host->mmc->parent->of_node;
+
+	if (of_device_is_compatible(np, "fsl,p2020-rev1-esdhc"))
+		host->quirks |= SDHCI_QUIRK_BROKEN_DMA;
+
+	if (of_device_is_compatible(np, "fsl,p2020-esdhc") ||
+			of_device_is_compatible(np, "fsl,p1010-esdhc") ||
+			of_device_is_compatible(np, "fsl,mpc8536-esdhc"))
+		host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
+}
+
 static struct sdhci_ops sdhci_esdhc_ops = {
 	.read_l = esdhc_readl,
 	.read_w = esdhc_readw,
@@ -247,6 +265,7 @@ static struct sdhci_ops sdhci_esdhc_ops = {
 	.platform_resume = esdhc_of_resume,
 #endif
 	.adma_workaround = esdhci_of_adma_workaround,
+	.get_of_property = esdhc_of_get_property,
 };
 
 static struct sdhci_pltfm_data sdhci_esdhc_pdata = {
diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
index 0c5eb6a..c17a03c 100644
--- a/drivers/mmc/host/sdhci-pltfm.c
+++ b/drivers/mmc/host/sdhci-pltfm.c
@@ -91,14 +91,6 @@ void sdhci_get_of_property(struct platform_device *pdev)
 	if (of_get_property(np, "no-1-8-v", NULL))
 		host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
 
-	if (of_device_is_compatible(np, "fsl,p2020-rev1-esdhc"))
-		host->quirks |= SDHCI_QUIRK_BROKEN_DMA;
-
-	if (of_device_is_compatible(np, "fsl,p2020-esdhc") ||
-			of_device_is_compatible(np, "fsl,p1010-esdhc") ||
-			of_device_is_compatible(np, "fsl,mpc8536-esdhc"))
-		host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
-
 	clk = of_get_property(np, "clock-frequency", &size);
 	if (clk && size == sizeof(*clk) && *clk)
 		pltfm_host->clock = be32_to_cpup(clk);
@@ -108,6 +100,9 @@ void sdhci_get_of_property(struct platform_device *pdev)
 
 	if (of_find_property(np, "enable-sdio-wakeup", NULL))
 		host->mmc->pm_caps |= MMC_PM_WAKE_SDIO_IRQ;
+
+	if (host->ops->get_of_property)
+		host->ops->get_of_property(host);
 }
 #else
 void sdhci_get_of_property(struct platform_device *pdev) {}
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 379e09d..0e5a0a1 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -294,6 +294,7 @@ struct sdhci_ops {
 	void	(*platform_resume)(struct sdhci_host *host);
 	void    (*adma_workaround)(struct sdhci_host *host, u32 intmask);
 	void	(*platform_init)(struct sdhci_host *host);
+	void	(*get_of_property)(struct sdhci_host *host);
 };
 
 #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
-- 
1.7.9.5

Re: [PATCH v2 1/2] mmc: sdhci-pltfm: add calling mmc_of_parse in sdhci_pltfm_register

[Stephen Warren]

On 03/08/2013 08:07 AM, Kevin Liu wrote:
> commit 6c56e7a0 provide a function mmc_of_parse for standard MMC
> device-tree binding parser centrally. So just call it with
> sdhci_get_of_property together in sdhci_pltfm_register.

> diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c

> @@ -212,6 +213,7 @@ int sdhci_pltfm_register(struct platform_device *pdev,
>  	if (IS_ERR(host))
>  		return PTR_ERR(host);
>  
> +	mmc_of_parse(host->mmc);

A few drivers already call mmc_of_parse() themselves. This change will
make that call happen twice. Mostly this won't be an issue, but there
are a couple gpio_request() calls in there, the error-handling for which
in mmc_of_parse() will spew error messages if attempted twice. I also
have a patch in the Tegra tree that adds a call to mmc_of_parse() into
the Tegra driver, and that relies on fixing some bugs in the device
tree; the CD GPIO polarity was previously specified incorrectly in the DT...

I guess to resolve this, what I could do is as follows:

a) Create a topic branch in the Tegra tree that contains just the DT
fixes that mmc_of_parse() relies on for Tegra.

b) Have Chris merge that into the MMC tree.

c) Then, it's safe to move the Tegra driver patch out of the Tegra tree
into the MMC tree, so this issue can be addressed there. Equally, if the
merge in (b) above happens before this current patch is applied, this
current patch won't cause any breakage on Tegra.

Chris, do you want to do this?

Re: [PATCH v2 1/2] mmc: sdhci-pltfm: add calling mmc_of_parse in sdhci_pltfm_register

[Chris Ball]

Hi,

On Fri, Mar 08 2013, Stephen Warren wrote:
> On 03/08/2013 08:07 AM, Kevin Liu wrote:
>> commit 6c56e7a0 provide a function mmc_of_parse for standard MMC
>> device-tree binding parser centrally. So just call it with
>> sdhci_get_of_property together in sdhci_pltfm_register.
>
>> diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
>
>> @@ -212,6 +213,7 @@ int sdhci_pltfm_register(struct platform_device *pdev,
>>  	if (IS_ERR(host))
>>  		return PTR_ERR(host);
>>  
>> +	mmc_of_parse(host->mmc);
>
> A few drivers already call mmc_of_parse() themselves. This change will
> make that call happen twice. Mostly this won't be an issue, but there
> are a couple gpio_request() calls in there, the error-handling for which
> in mmc_of_parse() will spew error messages if attempted twice.

Thanks for noticing that.  We'll have to remove those mmc_of_parse()
calls at the same time, if we want to do this.

> have a patch in the Tegra tree that adds a call to mmc_of_parse() into
> the Tegra driver, and that relies on fixing some bugs in the device
> tree; the CD GPIO polarity was previously specified incorrectly in the DT...
>
> I guess to resolve this, what I could do is as follows:
>
> a) Create a topic branch in the Tegra tree that contains just the DT
> fixes that mmc_of_parse() relies on for Tegra.
>
> b) Have Chris merge that into the MMC tree.
>
> c) Then, it's safe to move the Tegra driver patch out of the Tegra tree
> into the MMC tree, so this issue can be addressed there. Equally, if the
> merge in (b) above happens before this current patch is applied, this
> current patch won't cause any breakage on Tegra.
>
> Chris, do you want to do this?

Sure, that sounds fine.

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH v2 1/2] mmc: sdhci-pltfm: add calling mmc_of_parse in sdhci_pltfm_register

[Kevin Liu]

2013/3/9 Stephen Warren <swarren@wwwdotorg.org>:
> On 03/08/2013 08:07 AM, Kevin Liu wrote:
>> commit 6c56e7a0 provide a function mmc_of_parse for standard MMC
>> device-tree binding parser centrally. So just call it with
>> sdhci_get_of_property together in sdhci_pltfm_register.
>
>> diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
>
>> @@ -212,6 +213,7 @@ int sdhci_pltfm_register(struct platform_device *pdev,
>>       if (IS_ERR(host))
>>               return PTR_ERR(host);
>>
>> +     mmc_of_parse(host->mmc);
>
> A few drivers already call mmc_of_parse() themselves. This change will
> make that call happen twice. Mostly this won't be an issue, but there
> are a couple gpio_request() calls in there, the error-handling for which
> in mmc_of_parse() will spew error messages if attempted twice. I also
> have a patch in the Tegra tree that adds a call to mmc_of_parse() into
> the Tegra driver, and that relies on fixing some bugs in the device
> tree; the CD GPIO polarity was previously specified incorrectly in the DT...
>

Stephen,

I don't think so. I add calling mmc_of_parse in sdhci_pltfm_register
rather than sdhci_pltfm_init.

In latest code, only sh_mmcif.c and tmio_mmc_pio.c called mmc_of_parse
explicitly and they are not sdhci based driver and won't call any
sdhci-pltfm functions. So this change won't make that call
mmc_of_parse twice.

For the gpio related issue. That's true and that's why I call
mmc_of_parse in sdhci_pltfm_register rather than in sdhci_pltfm_init.
The sdhci based drivers which call sdhci_pltfm_register means want
sdhci_pltfm do everything for them which include sdhci_pltfm_init,
parse dt property and sdhci_add_host.
Meanwhile some other sdhci based drivers just call sdhci_pltfm_init
and then do some specific tasks such as gpio handling. If these
drivers call mmc_of_parse with their current code will lead to the
gpio issue you mentioned above.


> I guess to resolve this, what I could do is as follows:
>
> a) Create a topic branch in the Tegra tree that contains just the DT
> fixes that mmc_of_parse() relies on for Tegra.
>
> b) Have Chris merge that into the MMC tree.
>
> c) Then, it's safe to move the Tegra driver patch out of the Tegra tree
> into the MMC tree, so this issue can be addressed there. Equally, if the
> merge in (b) above happens before this current patch is applied, this
> current patch won't cause any breakage on Tegra.
>

But I'm afraid we still can't call mmc_of_parse directly in
sdhci_pltfm_init after this issue fixed.
1. some host drivers want to do specific gpio handling and use their
gpio irq handler.
2. some host drivers want to request gpio irq after sdhci_add_host
like sdhci-dove.c
So I think it's better to seperate gpio handling from mmc_of_parse.
How do you think?

> Chris, do you want to do this?

Re: [PATCH v2 1/2] mmc: sdhci-pltfm: add calling mmc_of_parse in sdhci_pltfm_register

[Stephen Warren]

n 03/08/2013 09:24 PM, Kevin Liu wrote:
> 2013/3/9 Stephen Warren <swarren@wwwdotorg.org>:
>> On 03/08/2013 08:07 AM, Kevin Liu wrote:
>>> commit 6c56e7a0 provide a function mmc_of_parse for standard MMC
>>> device-tree binding parser centrally. So just call it with
>>> sdhci_get_of_property together in sdhci_pltfm_register.
>>
>>> diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
>>
>>> @@ -212,6 +213,7 @@ int sdhci_pltfm_register(struct platform_device *pdev,
>>>       if (IS_ERR(host))
>>>               return PTR_ERR(host);
>>>
>>> +     mmc_of_parse(host->mmc);
>>
>> A few drivers already call mmc_of_parse() themselves. This change will
>> make that call happen twice. Mostly this won't be an issue, but there
>> are a couple gpio_request() calls in there, the error-handling for which
>> in mmc_of_parse() will spew error messages if attempted twice. I also
>> have a patch in the Tegra tree that adds a call to mmc_of_parse() into
>> the Tegra driver, and that relies on fixing some bugs in the device
>> tree; the CD GPIO polarity was previously specified incorrectly in the DT...
>>
> 
> Stephen,
> 
> I don't think so. I add calling mmc_of_parse in sdhci_pltfm_register
> rather than sdhci_pltfm_init.

Ah yes, you're right. So, there's no issue.

Chris, I do think I'll still send you a pull request to move the Tegra
SDHCI change that's currently in the Tegra tree into the MMC tree, just
in case there do turn out to be any MMC core cleanup/... issues this
kernel cycle. Best to be safe! I suspect that Tegra might be able to be
converted to using sdhci_pltfm_register() rather than sdhci_pltfm_init()
after Kevin's change, but I'll have to investigate more to be certain.