[PATCH 0/3] mmc: sdhci-s3c: Add device tree support for Samsung's sdhci controller driver

[PATCH 0/3] mmc: sdhci-s3c: Add device tree support for Samsung's sdhci controller driver

[Thomas Abraham]

This patchset adds device tree support for Samsung's sdhci controller driver.

The first patch modifies the sdhcis-s3c driver to mainatain a local copy of the
platform data, which makes it easier to add device tree support for the driver.

The second patch adds support for parsing of mmc host controller capabilities
from a device node. This code would be reusable across other platforms as well.
The third patch device tree based discovery for the sdhci-s3c driver.

This patchset is based on the following tree:
https://github.com/kgene/linux-samsung.git  branch: for-next
and tested on smdkv310 board.

Thomas Abraham (3):
  mmc: sdhci-s3c: Keep a copy of platform data and use it
  mmc: Add OF bindings support for mmc host controller capabilities
  mmc: sdhci-s3c: Add device tree support

 .../devicetree/bindings/mmc/linux-mmc-host.txt     |   11 ++
 .../devicetree/bindings/mmc/samsung-sdhci.txt      |   75 ++++++++++
 drivers/mmc/core/host.c                            |   31 ++++
 drivers/mmc/host/sdhci-s3c.c                       |  155 +++++++++++++++++++-
 include/linux/mmc/host.h                           |    3 +
 5 files changed, 270 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mmc/linux-mmc-host.txt
 create mode 100644 Documentation/devicetree/bindings/mmc/samsung-sdhci.txt

[PATCH 1/3] mmc: sdhci-s3c: Keep a copy of platform data and use it

[Thomas Abraham]

The platform data is copied into driver's private data and the copy is
used for all access to the platform data. This simpifies the addition
of device tree support for the sdhci-s3c driver.

Cc: Ben Dooks <ben-linux@fluff.org>
Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 drivers/mmc/host/sdhci-s3c.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
index 871cf4f..8280af0 100644
--- a/drivers/mmc/host/sdhci-s3c.c
+++ b/drivers/mmc/host/sdhci-s3c.c
@@ -424,7 +424,7 @@ static inline struct sdhci_s3c_drv_data *sdhci_s3c_get_driver_data(
 
 static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
 {
-	struct s3c_sdhci_platdata *pdata = pdev->dev.platform_data;
+	struct s3c_sdhci_platdata *pdata;
 	struct sdhci_s3c_drv_data *drv_data;
 	struct device *dev = &pdev->dev;
 	struct sdhci_host *host;
@@ -432,7 +432,7 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
 	struct resource *res;
 	int ret, irq, ptr, clks;
 
-	if (!pdata) {
+	if (!pdev->dev.platform_data) {
 		dev_err(dev, "no device data specified\n");
 		return -ENOENT;
 	}
@@ -455,6 +455,13 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
 		return PTR_ERR(host);
 	}
 
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata) {
+		ret = -ENOMEM;
+		goto err_io_clk;
+	}
+	memcpy(pdata, pdev->dev.platform_data, sizeof(*pdata));
+
 	drv_data = sdhci_s3c_get_driver_data(pdev);
 	sc = sdhci_priv(host);
 
-- 
1.6.6.rc2

[PATCH 2/3] mmc: Add OF bindings support for mmc host controller capabilities

[Thomas Abraham]

Device nodes representing sd/mmc controllers in a device tree would include
mmc host controller capabilities. Add support for parsing of mmc host
controller capabilities included in device nodes.

Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 .../devicetree/bindings/mmc/linux-mmc-host.txt     |   11 +++++++
 drivers/mmc/core/host.c                            |   31 ++++++++++++++++++++
 include/linux/mmc/host.h                           |    3 ++
 3 files changed, 45 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mmc/linux-mmc-host.txt

diff --git a/Documentation/devicetree/bindings/mmc/linux-mmc-host.txt b/Documentation/devicetree/bindings/mmc/linux-mmc-host.txt
new file mode 100644
index 0000000..cb43905
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/linux-mmc-host.txt
@@ -0,0 +1,11 @@
+* Linux mmc host capabilities
+
+MMC Host Controller capabilities used in linux:
+- linux,mmc_cap_4_bit_data - host can do 4 bit transfers
+- linux,mmc_cap_mmc_highspeed - host can do MMC high-speed timing
+- linux,mmc_cap_sd_highspeed - host can do SD high-speed timing
+- linux,mmc_cap_needs_poll - host needs polling for card detection
+- linux,mmc_cap_8_bit_data - host can do 8 bit transfer
+- linux,mmc_cap_disable - host can be disabled
+- linux,mmc_cap_nonremovable - host is connected to nonremovable media
+- linux,mmc_cap_erase - host allows erase/trim commands
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 793d0a0..4ee2e43 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -19,6 +19,7 @@
 #include <linux/leds.h>
 #include <linux/slab.h>
 #include <linux/suspend.h>
+#include <linux/of.h>
 
 #include <linux/mmc/host.h>
 #include <linux/mmc/card.h>
@@ -385,3 +386,33 @@ void mmc_free_host(struct mmc_host *host)
 }
 
 EXPORT_SYMBOL(mmc_free_host);
+
+#ifdef CONFIG_OF
+/**
+ *	mmc_of_parse_host_caps - parse mmc host capabilities from device node
+ *	@np: pointer to device node in device tree
+ *	@caps: pointer to host caps value to be returned
+ *
+ *	Search the device node in device tree for mmc host capabilities.
+ */
+void mmc_of_parse_host_caps(struct device_node *np, unsigned long *caps)
+{
+	if (of_find_property(np, "linux,mmc_cap_4_bit_data", NULL))
+		*caps |= MMC_CAP_4_BIT_DATA;
+	if (of_find_property(np, "linux,mmc_cap_mmc_highspeed", NULL))
+		*caps |= MMC_CAP_MMC_HIGHSPEED;
+	if (of_find_property(np, "linux,mmc_cap_sd_highspeed", NULL))
+		*caps |= MMC_CAP_SD_HIGHSPEED;
+	if (of_find_property(np, "linux,mmc_cap_needs_poll", NULL))
+		*caps |= MMC_CAP_NEEDS_POLL;
+	if (of_find_property(np, "linux,mmc_cap_8_bit_data", NULL))
+		*caps |= MMC_CAP_8_BIT_DATA;
+	if (of_find_property(np, "linux,mmc_cap_disable", NULL))
+		*caps |= MMC_CAP_DISABLE;
+	if (of_find_property(np, "linux,mmc_cap_nonremovable", NULL))
+		*caps |= MMC_CAP_NONREMOVABLE;
+	if (of_find_property(np, "linux,mmc_cap_erase", NULL))
+		*caps |= MMC_CAP_ERASE;
+}
+EXPORT_SYMBOL(mmc_of_parse_host_caps);
+#endif /* CONFIG_OF */
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 1d09562..72b5df2 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -309,6 +309,9 @@ extern struct mmc_host *mmc_alloc_host(int extra, struct device *);
 extern int mmc_add_host(struct mmc_host *);
 extern void mmc_remove_host(struct mmc_host *);
 extern void mmc_free_host(struct mmc_host *);
+#ifdef CONFIG_OF
+extern void mmc_of_parse_host_caps(struct device_node *np, unsigned long *caps);
+#endif
 
 static inline void *mmc_priv(struct mmc_host *host)
 {
-- 
1.6.6.rc2

[PATCH 2/3] mmc: Add OF bindings support for mmc host controller capabilities

[Rob Herring]

Thomas,

On 10/05/2011 05:13 AM, Thomas Abraham wrote:
> Device nodes representing sd/mmc controllers in a device tree would include
> mmc host controller capabilities. Add support for parsing of mmc host
> controller capabilities included in device nodes.
> 
> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> ---
>  .../devicetree/bindings/mmc/linux-mmc-host.txt     |   11 +++++++
>  drivers/mmc/core/host.c                            |   31 ++++++++++++++++++++
>  include/linux/mmc/host.h                           |    3 ++
>  3 files changed, 45 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/mmc/linux-mmc-host.txt
> 
> diff --git a/Documentation/devicetree/bindings/mmc/linux-mmc-host.txt b/Documentation/devicetree/bindings/mmc/linux-mmc-host.txt
> new file mode 100644
> index 0000000..cb43905
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/linux-mmc-host.txt
> @@ -0,0 +1,11 @@
> +* Linux mmc host capabilities
> +
> +MMC Host Controller capabilities used in linux:

These aren't really linux properties... Is the capabilities register
really this broken that all these are needed? If so, perhaps just a
straight caps register value to override with would be better.

> +- linux,mmc_cap_4_bit_data - host can do 4 bit transfers
> +- linux,mmc_cap_mmc_highspeed - host can do MMC high-speed timing
> +- linux,mmc_cap_sd_highspeed - host can do SD high-speed timing
> +- linux,mmc_cap_needs_poll - host needs polling for card detection
> +- linux,mmc_cap_8_bit_data - host can do 8 bit transfer

"sdhci,1-bit-only" already exists as a binding. Perhaps add
"sdhci,4-bit-only". No property then means can do 8-bit.

> +- linux,mmc_cap_disable - host can be disabled

What?

> +- linux,mmc_cap_nonremovable - host is connected to nonremovable media
> +- linux,mmc_cap_erase - host allows erase/trim commands

Isn't TRIM a card property and transparent to the host controller?

> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 793d0a0..4ee2e43 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -19,6 +19,7 @@
>  #include <linux/leds.h>
>  #include <linux/slab.h>
>  #include <linux/suspend.h>
> +#include <linux/of.h>
>  
>  #include <linux/mmc/host.h>
>  #include <linux/mmc/card.h>
> @@ -385,3 +386,33 @@ void mmc_free_host(struct mmc_host *host)
>  }
>  
>  EXPORT_SYMBOL(mmc_free_host);
> +
> +#ifdef CONFIG_OF
> +/**
> + *	mmc_of_parse_host_caps - parse mmc host capabilities from device node
> + *	@np: pointer to device node in device tree
> + *	@caps: pointer to host caps value to be returned
> + *
> + *	Search the device node in device tree for mmc host capabilities.
> + */
> +void mmc_of_parse_host_caps(struct device_node *np, unsigned long *caps)
> +{
> +	if (of_find_property(np, "linux,mmc_cap_4_bit_data", NULL))
> +		*caps |= MMC_CAP_4_BIT_DATA;
> +	if (of_find_property(np, "linux,mmc_cap_mmc_highspeed", NULL))
> +		*caps |= MMC_CAP_MMC_HIGHSPEED;
> +	if (of_find_property(np, "linux,mmc_cap_sd_highspeed", NULL))
> +		*caps |= MMC_CAP_SD_HIGHSPEED;
> +	if (of_find_property(np, "linux,mmc_cap_needs_poll", NULL))
> +		*caps |= MMC_CAP_NEEDS_POLL;
> +	if (of_find_property(np, "linux,mmc_cap_8_bit_data", NULL))
> +		*caps |= MMC_CAP_8_BIT_DATA;
> +	if (of_find_property(np, "linux,mmc_cap_disable", NULL))
> +		*caps |= MMC_CAP_DISABLE;
> +	if (of_find_property(np, "linux,mmc_cap_nonremovable", NULL))
> +		*caps |= MMC_CAP_NONREMOVABLE;
> +	if (of_find_property(np, "linux,mmc_cap_erase", NULL))
> +		*caps |= MMC_CAP_ERASE;
> +}
> +EXPORT_SYMBOL(mmc_of_parse_host_caps);
> +#endif /* CONFIG_OF */
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 1d09562..72b5df2 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -309,6 +309,9 @@ extern struct mmc_host *mmc_alloc_host(int extra, struct device *);
>  extern int mmc_add_host(struct mmc_host *);
>  extern void mmc_remove_host(struct mmc_host *);
>  extern void mmc_free_host(struct mmc_host *);
> +#ifdef CONFIG_OF
> +extern void mmc_of_parse_host_caps(struct device_node *np, unsigned long *caps);
> +#endif

You don't need a ifdef around this.

Rob

[PATCH 2/3] mmc: Add OF bindings support for mmc host controller capabilities

[Thomas Abraham]

Hi Rob,

On 5 October 2011 18:59, Rob Herring <robherring2@gmail.com> wrote:
> Thomas,
>
> On 10/05/2011 05:13 AM, Thomas Abraham wrote:
>> Device nodes representing sd/mmc controllers in a device tree would include
>> mmc host controller capabilities. Add support for parsing of mmc host
>> controller capabilities included in device nodes.
>>
>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>> ---
>> ?.../devicetree/bindings/mmc/linux-mmc-host.txt ? ? | ? 11 +++++++
>> ?drivers/mmc/core/host.c ? ? ? ? ? ? ? ? ? ? ? ? ? ?| ? 31 ++++++++++++++++++++
>> ?include/linux/mmc/host.h ? ? ? ? ? ? ? ? ? ? ? ? ? | ? ?3 ++
>> ?3 files changed, 45 insertions(+), 0 deletions(-)
>> ?create mode 100644 Documentation/devicetree/bindings/mmc/linux-mmc-host.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/linux-mmc-host.txt b/Documentation/devicetree/bindings/mmc/linux-mmc-host.txt
>> new file mode 100644
>> index 0000000..cb43905
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mmc/linux-mmc-host.txt
>> @@ -0,0 +1,11 @@
>> +* Linux mmc host capabilities
>> +
>> +MMC Host Controller capabilities used in linux:
>
> These aren't really linux properties... Is the capabilities register
> really this broken that all these are needed? If so, perhaps just a
> straight caps register value to override with would be better.


I was trying to provide bindings for the MMC_CAP_XXX defined in
include/linux/mmc/host.h file. I am no expert in mmc subsystem but I
believe that these capabilities are not directly mapped to any
capabilities register in the host controller hardware. All of the
MMC_CAP_XXX defines are flags used by the mmc core to make runtime
decisions. The host controller driver would read its capability
register and report its capabilities to the mmc core layer using these
MMC_CAP_XXX defines.

But not all of the MMC_CAP_XXX defines can be derived from a
capabilities register of a host controller. Some of the MMC_CAP_XXX
capabilities are used as defaults by the host controller driver while
some others which are board dependent are feed to the host controller
driver using the platform data.

While migrating a host controller driver to device tree, and not
relying on the aux data for platform data, there has to be mechanism
to specify the mmc core subsystem capabilities which are board
specific and I though bindings for MMC_CAP_XXX could be an option.


>
>> +- linux,mmc_cap_4_bit_data - host can do 4 bit transfers
>> +- linux,mmc_cap_mmc_highspeed - host can do MMC high-speed timing
>> +- linux,mmc_cap_sd_highspeed - host can do SD high-speed timing
>> +- linux,mmc_cap_needs_poll - host needs polling for card detection
>> +- linux,mmc_cap_8_bit_data - host can do 8 bit transfer
>
> "sdhci,1-bit-only" already exists as a binding. Perhaps add
> "sdhci,4-bit-only". No property then means can do 8-bit.


Ok. But that would remain just as sdhci host controller capability and
will not be applicable to other types of host controllers.


>
>> +- linux,mmc_cap_disable - host can be disabled
>
> What?

Apologies. I will be more verbose next time.

>
>> +- linux,mmc_cap_nonremovable - host is connected to nonremovable media
>> +- linux,mmc_cap_erase - host allows erase/trim commands
>
> Isn't TRIM a card property and transparent to the host controller?


I am not sure on this. Maybe it might have some dependency on the host
controller as well.


>
>> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
>> index 793d0a0..4ee2e43 100644
>> --- a/drivers/mmc/core/host.c
>> +++ b/drivers/mmc/core/host.c
>> @@ -19,6 +19,7 @@
>> ?#include <linux/leds.h>
>> ?#include <linux/slab.h>
>> ?#include <linux/suspend.h>
>> +#include <linux/of.h>
>>
>> ?#include <linux/mmc/host.h>
>> ?#include <linux/mmc/card.h>
>> @@ -385,3 +386,33 @@ void mmc_free_host(struct mmc_host *host)
>> ?}
>>
>> ?EXPORT_SYMBOL(mmc_free_host);
>> +
>> +#ifdef CONFIG_OF
>> +/**
>> + * ? mmc_of_parse_host_caps - parse mmc host capabilities from device node
>> + * ? @np: pointer to device node in device tree
>> + * ? @caps: pointer to host caps value to be returned
>> + *
>> + * ? Search the device node in device tree for mmc host capabilities.
>> + */
>> +void mmc_of_parse_host_caps(struct device_node *np, unsigned long *caps)
>> +{
>> + ? ? if (of_find_property(np, "linux,mmc_cap_4_bit_data", NULL))
>> + ? ? ? ? ? ? *caps |= MMC_CAP_4_BIT_DATA;
>> + ? ? if (of_find_property(np, "linux,mmc_cap_mmc_highspeed", NULL))
>> + ? ? ? ? ? ? *caps |= MMC_CAP_MMC_HIGHSPEED;
>> + ? ? if (of_find_property(np, "linux,mmc_cap_sd_highspeed", NULL))
>> + ? ? ? ? ? ? *caps |= MMC_CAP_SD_HIGHSPEED;
>> + ? ? if (of_find_property(np, "linux,mmc_cap_needs_poll", NULL))
>> + ? ? ? ? ? ? *caps |= MMC_CAP_NEEDS_POLL;
>> + ? ? if (of_find_property(np, "linux,mmc_cap_8_bit_data", NULL))
>> + ? ? ? ? ? ? *caps |= MMC_CAP_8_BIT_DATA;
>> + ? ? if (of_find_property(np, "linux,mmc_cap_disable", NULL))
>> + ? ? ? ? ? ? *caps |= MMC_CAP_DISABLE;
>> + ? ? if (of_find_property(np, "linux,mmc_cap_nonremovable", NULL))
>> + ? ? ? ? ? ? *caps |= MMC_CAP_NONREMOVABLE;
>> + ? ? if (of_find_property(np, "linux,mmc_cap_erase", NULL))
>> + ? ? ? ? ? ? *caps |= MMC_CAP_ERASE;
>> +}
>> +EXPORT_SYMBOL(mmc_of_parse_host_caps);
>> +#endif /* CONFIG_OF */
>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>> index 1d09562..72b5df2 100644
>> --- a/include/linux/mmc/host.h
>> +++ b/include/linux/mmc/host.h
>> @@ -309,6 +309,9 @@ extern struct mmc_host *mmc_alloc_host(int extra, struct device *);
>> ?extern int mmc_add_host(struct mmc_host *);
>> ?extern void mmc_remove_host(struct mmc_host *);
>> ?extern void mmc_free_host(struct mmc_host *);
>> +#ifdef CONFIG_OF
>> +extern void mmc_of_parse_host_caps(struct device_node *np, unsigned long *caps);
>> +#endif
>
> You don't need a ifdef around this.

Ok.

>
> Rob
>

Thanks for your review.

Regards,
Thomas.

[PATCH 2/3] mmc: Add OF bindings support for mmc host controller capabilities

[Stephen Warren]

Thomas Abraham wrote at Wednesday, October 05, 2011 8:28 AM:
> On 5 October 2011 18:59, Rob Herring <robherring2@gmail.com> wrote:
> > On 10/05/2011 05:13 AM, Thomas Abraham wrote:
> >> Device nodes representing sd/mmc controllers in a device tree would include
> >> mmc host controller capabilities. Add support for parsing of mmc host
> >> controller capabilities included in device nodes.
...
> >> +- linux,mmc_cap_4_bit_data - host can do 4 bit transfers
> >> +- linux,mmc_cap_mmc_highspeed - host can do MMC high-speed timing
> >> +- linux,mmc_cap_sd_highspeed - host can do SD high-speed timing
> >> +- linux,mmc_cap_needs_poll - host needs polling for card detection
> >> +- linux,mmc_cap_8_bit_data - host can do 8 bit transfer
> >
> > "sdhci,1-bit-only" already exists as a binding. Perhaps add
> > "sdhci,4-bit-only". No property then means can do 8-bit.
> 
> Ok. But that would remain just as sdhci host controller capability and
> will not be applicable to other types of host controllers.

Just as an FYI, NVIDIA's SDHCI controller bindings use property
"support-8bit" to indicate 8-bit support. A similar property could be
used for 4-bit support too.

-- 
nvpublic

[PATCH 2/3] mmc: Add OF bindings support for mmc host controller capabilities

[Thomas Abraham]

On 5 October 2011 21:25, Stephen Warren <swarren@nvidia.com> wrote:
> Thomas Abraham wrote at Wednesday, October 05, 2011 8:28 AM:
>> On 5 October 2011 18:59, Rob Herring <robherring2@gmail.com> wrote:
>> > On 10/05/2011 05:13 AM, Thomas Abraham wrote:
>> >> Device nodes representing sd/mmc controllers in a device tree would include
>> >> mmc host controller capabilities. Add support for parsing of mmc host
>> >> controller capabilities included in device nodes.
> ...
>> >> +- linux,mmc_cap_4_bit_data - host can do 4 bit transfers
>> >> +- linux,mmc_cap_mmc_highspeed - host can do MMC high-speed timing
>> >> +- linux,mmc_cap_sd_highspeed - host can do SD high-speed timing
>> >> +- linux,mmc_cap_needs_poll - host needs polling for card detection
>> >> +- linux,mmc_cap_8_bit_data - host can do 8 bit transfer
>> >
>> > "sdhci,1-bit-only" already exists as a binding. Perhaps add
>> > "sdhci,4-bit-only". No property then means can do 8-bit.
>>
>> Ok. But that would remain just as sdhci host controller capability and
>> will not be applicable to other types of host controllers.
>
> Just as an FYI, NVIDIA's SDHCI controller bindings use property
> "support-8bit" to indicate 8-bit support. A similar property could be
> used for 4-bit support too.


In this case, the binding would be applicable only to nVidia's SDHCI
controller. And this binding would select MMC_CAP_8_BIT_DATA in the
sdhci-tegra driver. There are sdhci drivers that can accept host
capabilities via platform data.

The MMC_CAP_XXX macros in linux/mmc/host.h file are usuable across
host controllers and some of them are supplied as platform data for
non-device tree platforms.

So, bindings for MMC_CAP_XXX macros can be defined which any host
controller supporting device tree could use. Host controllers could
use the mmc_of_parse_host_caps() helper function (listed in this
patch) to parse all the bindings for MMC_CAP_XXX  available in the
device node.

I would like to retain this patch in this series but if there is a
definite no, then I will drop it. Please let me know your opinion.

Thanks,
Thomas.

>
> --
> nvpublic
>
>

[PATCH 2/3] mmc: Add OF bindings support for mmc host controller capabilities

[Stephen Warren]

Thomas Abraham wrote at Sunday, October 09, 2011 12:58 AM:
> On 5 October 2011 21:25, Stephen Warren <swarren@nvidia.com> wrote:
> > Thomas Abraham wrote at Wednesday, October 05, 2011 8:28 AM:
> >> On 5 October 2011 18:59, Rob Herring <robherring2@gmail.com> wrote:
> >> > On 10/05/2011 05:13 AM, Thomas Abraham wrote:
> >> >> Device nodes representing sd/mmc controllers in a device tree would include
> >> >> mmc host controller capabilities. Add support for parsing of mmc host
> >> >> controller capabilities included in device nodes.
> > ...
> >> >> +- linux,mmc_cap_4_bit_data - host can do 4 bit transfers
> >> >> +- linux,mmc_cap_mmc_highspeed - host can do MMC high-speed timing
> >> >> +- linux,mmc_cap_sd_highspeed - host can do SD high-speed timing
> >> >> +- linux,mmc_cap_needs_poll - host needs polling for card detection
> >> >> +- linux,mmc_cap_8_bit_data - host can do 8 bit transfer
> >> >
> >> > "sdhci,1-bit-only" already exists as a binding. Perhaps add
> >> > "sdhci,4-bit-only". No property then means can do 8-bit.
> >>
> >> Ok. But that would remain just as sdhci host controller capability and
> >> will not be applicable to other types of host controllers.
> >
> > Just as an FYI, NVIDIA's SDHCI controller bindings use property
> > "support-8bit" to indicate 8-bit support. A similar property could be
> > used for 4-bit support too.
> 
> In this case, the binding would be applicable only to nVidia's SDHCI
> controller.

Sure, this is the case right now. And I thought I'd copied this property
name from some other driver, but it looks like I'm mistaken.

> And this binding would select MMC_CAP_8_BIT_DATA in the
> sdhci-tegra driver. There are sdhci drivers that can accept host
> capabilities via platform data.
> 
> The MMC_CAP_XXX macros in linux/mmc/host.h file are usuable across
> host controllers and some of them are supplied as platform data for
> non-device tree platforms.
> 
> So, bindings for MMC_CAP_XXX macros can be defined which any host
> controller supporting device tree could use. Host controllers could
> use the mmc_of_parse_host_caps() helper function (listed in this
> patch) to parse all the bindings for MMC_CAP_XXX  available in the
> device node.
> 
> I would like to retain this patch in this series but if there is a
> definite no, then I will drop it. Please let me know your opinion.

I was simply pointing out that there are already some properties in this
area. It'd be nice if we could unify everything on a single style; either
expanding the existing "sdhci,1-bit-only" style that Rob mentioned, or
expanding the "support-8bit" style that Tegra currently uses, rather than
adding a third way of indicating this. Still, given the early state of
DT on Tegra, I'd be happy migrating Tegra to whatever single unified
approach is chosen, so don't take my comments as nak'ing your patch or
anything like that.

-- 
nvpublic

[PATCH 2/3] mmc: Add OF bindings support for mmc host controller capabilities

[Thomas Abraham]

On 11 October 2011 21:59, Stephen Warren <swarren@nvidia.com> wrote:
> Thomas Abraham wrote at Sunday, October 09, 2011 12:58 AM:
>> On 5 October 2011 21:25, Stephen Warren <swarren@nvidia.com> wrote:
>> > Thomas Abraham wrote at Wednesday, October 05, 2011 8:28 AM:
>> >> On 5 October 2011 18:59, Rob Herring <robherring2@gmail.com> wrote:
>> >> > On 10/05/2011 05:13 AM, Thomas Abraham wrote:
>> >> >> Device nodes representing sd/mmc controllers in a device tree would include
>> >> >> mmc host controller capabilities. Add support for parsing of mmc host
>> >> >> controller capabilities included in device nodes.
>> > ...
>> >> >> +- linux,mmc_cap_4_bit_data - host can do 4 bit transfers
>> >> >> +- linux,mmc_cap_mmc_highspeed - host can do MMC high-speed timing
>> >> >> +- linux,mmc_cap_sd_highspeed - host can do SD high-speed timing
>> >> >> +- linux,mmc_cap_needs_poll - host needs polling for card detection
>> >> >> +- linux,mmc_cap_8_bit_data - host can do 8 bit transfer
>> >> >
>> >> > "sdhci,1-bit-only" already exists as a binding. Perhaps add
>> >> > "sdhci,4-bit-only". No property then means can do 8-bit.
>> >>
>> >> Ok. But that would remain just as sdhci host controller capability and
>> >> will not be applicable to other types of host controllers.
>> >
>> > Just as an FYI, NVIDIA's SDHCI controller bindings use property
>> > "support-8bit" to indicate 8-bit support. A similar property could be
>> > used for 4-bit support too.
>>
>> In this case, the binding would be applicable only to nVidia's SDHCI
>> controller.
>
> Sure, this is the case right now. And I thought I'd copied this property
> name from some other driver, but it looks like I'm mistaken.
>
>> And this binding would select MMC_CAP_8_BIT_DATA in the
>> sdhci-tegra driver. There are sdhci drivers that can accept host
>> capabilities via platform data.
>>
>> The MMC_CAP_XXX macros in linux/mmc/host.h file are usuable across
>> host controllers and some of them are supplied as platform data for
>> non-device tree platforms.
>>
>> So, bindings for MMC_CAP_XXX macros can be defined which any host
>> controller supporting device tree could use. Host controllers could
>> use the mmc_of_parse_host_caps() helper function (listed in this
>> patch) to parse all the bindings for MMC_CAP_XXX ?available in the
>> device node.
>>
>> I would like to retain this patch in this series but if there is a
>> definite no, then I will drop it. Please let me know your opinion.
>
> I was simply pointing out that there are already some properties in this
> area. It'd be nice if we could unify everything on a single style; either
> expanding the existing "sdhci,1-bit-only" style that Rob mentioned, or
> expanding the "support-8bit" style that Tegra currently uses, rather than
> adding a third way of indicating this. Still, given the early state of
> DT on Tegra, I'd be happy migrating Tegra to whatever single unified
> approach is chosen, so don't take my comments as nak'ing your patch or
> anything like that.

Ok.

The sdhci bus width binding is an example that can be extended as you
have suggested. If done this way, the sdhci bus width bindings would
be converted by the driver to either of MMC_CAP_[4/8]_BIT_DATA
capability.

But there are other MMC_CAP_XXX values as well which the host
controller driver needs to report to the upper mmc core layer. Some of
these capabilities are determined by reading caps register of the host
controller and converting them to equivalent MMC_CAP_XXX flags. But
some of them are board dependent such as

MMC_CAP_NEEDS_POLL,
MMC_CAP_DISABLE,
MMC_CAP_NONREMOVABLE,
MMC_CAP_POWER_OFF_CARD and others.

These flags which are tied to the characteristics of the board is
usually supplied using platform data. The driver picks up these from
platform data, adds its own set of CAP's and reports to the upper
layer.

When we do not have platform data (as in the case of dt), there could
be two options to bind these CAP's. Either define custom bindings for
these and then translate them in each host controller driver using
them or provide direct bindings for MMC_CAP_xxx with a common function
to parse them. I tend towards direct bindings for MMC_CAP_XXX because
it can be used by any type of mmc/sd host controller and not just
sdhci controller and provides common binding for all host controllers.

Thanks for your comments.

Regards,
Thomas.

>
> --
> nvpublic
>
>

[PATCH 3/3] mmc: sdhci-s3c: Add device tree support

[Thomas Abraham]

Add device tree based discovery support for Samsung's sdhci controller

Cc: Ben Dooks <ben-linux@fluff.org>
Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 .../devicetree/bindings/mmc/samsung-sdhci.txt      |   75 ++++++++++
 drivers/mmc/host/sdhci-s3c.c                       |  150 +++++++++++++++++++-
 2 files changed, 219 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mmc/samsung-sdhci.txt

diff --git a/Documentation/devicetree/bindings/mmc/samsung-sdhci.txt b/Documentation/devicetree/bindings/mmc/samsung-sdhci.txt
new file mode 100644
index 0000000..dc94d24
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/samsung-sdhci.txt
@@ -0,0 +1,75 @@
+* Samsung's SDHCI Controller device tree bindings
+
+Samsung's SDHCI controller is used as a connectivity interface with external
+MMC, SD and eMMC storage mediums.
+
+Required SoC Specific Properties:
+- compatible: should be one of the following
+  - "samsung,s3c6410-sdhci": For controllers compatible with s3c6410 sdhci
+    controller.
+  - "samsung,exynos4-sdhci": For controller compatible with Exynos4 sdhci
+    controller.
+
+- reg: physical base address of the controller and length of memory mapped
+  region.
+
+- interrupts: The interrupt number to the cpu. The interrupt specifier format
+  depends on the interrupt controller.
+
+
+Required Board Specific Properties:
+- gpios: Should specify the gpios used for clock, command and data lines. The
+  gpio specifier format depends on the gpio controller. Note: There is no
+  particular order in which the gpio's have to be listed.
+
+
+Optional Board Specific Properties:
+- samsung,sdhci-bus-width: Number of data lines connected to the controller.
+  Note: This excludes the clock,command and card detect lines. If this property
+  is not specified, default value is 1.
+
+- samsung,cd-gpio-invert: If 'samsung,sdhci-cd-gpio' card detect method is
+  selected, this property can be optionally specified to invert the value of
+  external card detect gpio line.
+
+- One of the following properties for card detect type.
+  - samsung,sdhci-cd-internal: Card detect line from the card slot  is
+    connected to the card detect pad of the sdhci controller. A gpio is
+    used for this connection (with possible pin function settings).
+  - samsung,sdhci-cd-gpio: A gpio line (with possible pin function settings)
+    is used a card detect line. This gpio line is not connected to card detect
+    pad of the sdhci controller.
+  - samsung,sdhci-cd-none: There is no card detect line. Polling is used to
+    detect the presence of the card. (DEFAULT, if no card detect property
+    is specified).
+  - samsung,sdhci-cd-permanent: There is no card detect line. The card is
+    permanently connected to the sdhci controller.
+
+- gpio-cd: The gpio to be used as card detect line for
+  'samsung,sdhci-cd-internal' or 'samsung,sdhci-cd-gpio' card detection method.
+  The gpio specifier format depends on the gpio controller.
+
+- One or more of the linux specific mmc host bindings.
+  See Documentation/devicetree/bindings/mmc/linux-mmc-host.txt for all the
+  linux mmc host controller specific bindings.
+
+Example:
+	sdhci at 12530000 {
+		compatible = "samsung,exynos4-sdhci";
+		reg = <0x12530000 0x100>;
+		interrupts = <139>;
+		samsung,sdhci-bus-width = <4>;
+		linux,mmc_cap_4_bit_data;
+		samsung,sdhci-cd-internal;
+		gpio-cd = <&gpk2 2 2 3 3>;
+		gpios = <&gpk2 0 2 0 3>,  /* clock line */
+			<&gpk2 1 2 0 3>,  /* command line */
+			<&gpk2 3 2 3 3>,  /* data line 0 */
+			<&gpk2 4 2 3 3>,  /* data line 1 */
+			<&gpk2 5 2 3 3>,  /* data line 2 */
+			<&gpk2 6 2 3 3>;  /* data line 3 */
+	};
+
+	Note: This example shows both SoC specific and board specific properties
+	in a single device node. The properties can be actually be seperated
+	into SoC specific node and board specific node.
diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
index 8280af0..047c404 100644
--- a/drivers/mmc/host/sdhci-s3c.c
+++ b/drivers/mmc/host/sdhci-s3c.c
@@ -20,6 +20,8 @@
 #include <linux/io.h>
 #include <linux/gpio.h>
 #include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
 
 #include <linux/mmc/host.h>
 
@@ -29,6 +31,8 @@
 #include "sdhci.h"
 
 #define MAX_BUS_CLK	(4)
+/* Number of gpio's used is max data bus width + command and clock lines */
+#define NUM_GPIOS(x)	(x + 2)
 
 /**
  * struct sdhci_s3c - S3C SDHCI instance
@@ -48,6 +52,7 @@ struct sdhci_s3c {
 	unsigned int		cur_clk;
 	int			ext_cd_irq;
 	int			ext_cd_gpio;
+	int			*gpios;
 
 	struct clk		*clk_io;
 	struct clk		*clk_bus[MAX_BUS_CLK];
@@ -415,9 +420,110 @@ static void sdhci_s3c_setup_card_detect_gpio(struct sdhci_s3c *sc)
 	}
 }
 
+#ifdef CONFIG_OF
+static int __devinit sdhci_s3c_parse_dt(struct device *dev,
+		struct sdhci_host *host, struct s3c_sdhci_platdata *pdata)
+{
+	struct device_node *node = dev->of_node;
+	struct sdhci_s3c *ourhost = to_s3c(host);
+	u32 max_width;
+	int gpio, cnt, ret;
+
+	/* if the bus-width property is not specified, assume width as 1 */
+	if (of_property_read_u32(node, "samsung,sdhci-bus-width",
+				&max_width))
+		max_width = 1;
+	pdata->max_width = max_width;
+
+	ourhost->gpios = devm_kzalloc(dev, NUM_GPIOS(pdata->max_width) *
+				sizeof(int), GFP_KERNEL);
+	if (!ourhost->gpios)
+		return -ENOMEM;
+
+	mmc_of_parse_host_caps(node, (unsigned long *)&pdata->host_caps);
+
+	/* get the card detection method */
+	if (of_get_property(node, "samsung,sdhci-cd-internal", NULL))
+		pdata->cd_type = S3C_SDHCI_CD_INTERNAL;
+	else if (of_get_property(node, "samsung,sdhci-cd-gpio", NULL))
+		pdata->cd_type = S3C_SDHCI_CD_GPIO;
+	else if (of_get_property(node, "samsung,sdhci-cd-none", NULL))
+		pdata->cd_type = S3C_SDHCI_CD_NONE;
+	else if (of_get_property(node, "samsung,sdhci-cd-permanent", NULL))
+		pdata->cd_type = S3C_SDHCI_CD_PERMANENT;
+	else
+		pdata->cd_type = S3C_SDHCI_CD_NONE;
+
+	/* get the gpio used for card detection */
+	if (pdata->cd_type == S3C_SDHCI_CD_GPIO || S3C_SDHCI_CD_INTERNAL) {
+		gpio = of_get_named_gpio(node, "gpio-cd", 0);
+		if (!gpio_is_valid(gpio)) {
+			dev_err(dev, "invalid card detect gpio specified\n");
+			return -EINVAL;
+		}
+	}
+
+	if (pdata->cd_type == S3C_SDHCI_CD_GPIO) {
+		pdata->ext_cd_gpio = gpio;
+		ourhost->ext_cd_gpio = -1; /* invalid gpio number */
+		if (of_get_property(node, "samsung,cd-gpio-invert", NULL))
+			pdata->ext_cd_gpio_invert = 1;
+	} else if (pdata->cd_type == S3C_SDHCI_CD_INTERNAL) {
+		ret = gpio_request(gpio, "sdhci-cd");
+		if (ret) {
+			dev_err(dev, "card detect gpio request failed\n");
+			return -EINVAL;
+		}
+		ourhost->ext_cd_gpio = gpio;
+	}
+
+	/* get the gpios for command, clock and data lines */
+	for (cnt = 0; cnt < NUM_GPIOS(pdata->max_width); cnt++) {
+		gpio = of_get_gpio(node, cnt);
+		if (!gpio_is_valid(gpio)) {
+			dev_err(dev, "invalid gpio[%d]\n", cnt);
+			goto err_free_dt_cd_gpio;
+		}
+		ourhost->gpios[cnt] = gpio;
+	}
+
+	for (cnt = 0; cnt < NUM_GPIOS(pdata->max_width); cnt++) {
+		ret = gpio_request(ourhost->gpios[cnt], "sdhci-gpio");
+		if (ret) {
+			dev_err(dev, "gpio[%d] request failed\n", cnt);
+			goto err_free_dt_gpios;
+		}
+	}
+
+	return 0;
+
+ err_free_dt_gpios:
+	while (--cnt >= 0)
+		gpio_free(ourhost->gpios[cnt]);
+ err_free_dt_cd_gpio:
+	if (pdata->cd_type == S3C_SDHCI_CD_INTERNAL)
+		gpio_free(ourhost->ext_cd_gpio);
+	return -EINVAL;
+}
+#else
+static int __devinit sdhci_s3c_parse_dt(struct device *dev,
+		struct sdhci_host *host, struct s3c_sdhci_platdata *pdata)
+{
+}
+#endif
+
+static const struct of_device_id sdhci_s3c_dt_match[];
+
 static inline struct sdhci_s3c_drv_data *sdhci_s3c_get_driver_data(
 			struct platform_device *pdev)
 {
+#if CONFIG_OF
+	if (pdev->dev.of_node) {
+		const struct of_device_id *match;
+		match = of_match_node(sdhci_s3c_dt_match, pdev->dev.of_node);
+		return (struct sdhci_s3c_drv_data *)match->data;
+	}
+#endif
 	return (struct sdhci_s3c_drv_data *)
 			platform_get_device_id(pdev)->driver_data;
 }
@@ -432,7 +538,7 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
 	struct resource *res;
 	int ret, irq, ptr, clks;
 
-	if (!pdev->dev.platform_data) {
+	if (!pdev->dev.platform_data && !pdev->dev.of_node) {
 		dev_err(dev, "no device data specified\n");
 		return -ENOENT;
 	}
@@ -454,21 +560,28 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
 		dev_err(dev, "sdhci_alloc_host() failed\n");
 		return PTR_ERR(host);
 	}
+	sc = sdhci_priv(host);
 
 	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
 	if (!pdata) {
 		ret = -ENOMEM;
-		goto err_io_clk;
+		goto err_pdata;
+	}
+
+	if (pdev->dev.of_node) {
+		ret = sdhci_s3c_parse_dt(&pdev->dev, host, pdata);
+		if (ret)
+			goto err_pdata;
+	} else {
+		memcpy(pdata, pdev->dev.platform_data, sizeof(*pdata));
+		sc->ext_cd_gpio = -1; /* invalid gpio number */
 	}
-	memcpy(pdata, pdev->dev.platform_data, sizeof(*pdata));
 
 	drv_data = sdhci_s3c_get_driver_data(pdev);
-	sc = sdhci_priv(host);
 
 	sc->host = host;
 	sc->pdev = pdev;
 	sc->pdata = pdata;
-	sc->ext_cd_gpio = -1; /* invalid gpio number */
 
 	platform_set_drvdata(pdev, host);
 
@@ -625,6 +738,12 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
 	clk_put(sc->clk_io);
 
  err_io_clk:
+	for (ptr = 0; ptr < NUM_GPIOS(sc->pdata->max_width); ptr++)
+		gpio_free(sc->gpios[ptr]);
+	if (pdata->cd_type == S3C_SDHCI_CD_INTERNAL)
+		gpio_free(sc->ext_cd_gpio);
+
+ err_pdata:
 	sdhci_free_host(host);
 
 	return ret;
@@ -632,9 +751,9 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
 
 static int __devexit sdhci_s3c_remove(struct platform_device *pdev)
 {
-	struct s3c_sdhci_platdata *pdata = pdev->dev.platform_data;
 	struct sdhci_host *host =  platform_get_drvdata(pdev);
 	struct sdhci_s3c *sc = sdhci_priv(host);
+	struct s3c_sdhci_platdata *pdata = sc->pdata;
 	int ptr;
 
 	if (pdata->cd_type == S3C_SDHCI_CD_EXTERNAL && pdata->ext_cd_cleanup)
@@ -661,6 +780,11 @@ static int __devexit sdhci_s3c_remove(struct platform_device *pdev)
 	release_resource(sc->ioarea);
 	kfree(sc->ioarea);
 
+	if (pdev->dev.of_node) {
+		for (ptr = 0; ptr < NUM_GPIOS(sc->pdata->max_width); ptr++)
+			gpio_free(sc->gpios[ptr]);
+	}
+
 	sdhci_free_host(host);
 	platform_set_drvdata(pdev, NULL);
 
@@ -706,9 +830,22 @@ static struct platform_device_id sdhci_s3c_driver_ids[] = {
 		.name		= "exynos4-sdhci",
 		.driver_data	= EXYNOS4_SDHCI_DRV_DATA,
 	},
+	{},
 };
 MODULE_DEVICE_TABLE(platform, sdhci_s3c_driver_ids);
 
+#ifdef CONFIG_OF
+static const struct of_device_id sdhci_s3c_dt_match[] = {
+	{ .compatible = "samsung,s3c6410-sdhci", },
+	{ .compatible = "samsung,exynos4-sdhci",
+		.data = &exynos4_sdhci_drv_data },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sdhci_s3c_dt_match);
+#else
+#define sdhci_s3c_dt_match NULL
+#endif
+
 static struct platform_driver sdhci_s3c_driver = {
 	.probe		= sdhci_s3c_probe,
 	.remove		= __devexit_p(sdhci_s3c_remove),
@@ -718,6 +855,7 @@ static struct platform_driver sdhci_s3c_driver = {
 	.driver		= {
 		.owner	= THIS_MODULE,
 		.name	= "s3c-sdhci",
+		.of_match_table = sdhci_s3c_dt_match,
 	},
 };
 
-- 
1.6.6.rc2