sdhci-pltfm: Few additions and enhancements

Quoted 7 lines · David Vrabel

Quoted 3 lines · Anton Vorontsov

Some hosts (e.g. as found in CNS3xxx SOCs) report wrong value in CLOCK_BASE capability field, and currently there is no way to force the SDHCI core to use the platform-provided base clock value.

I don't think this needs a new quirk. Change the sdhci driver to check if the platform provides a value before reading the standard register.

Well, Pierre once said that by default we should conform to the SDHCI spec, and any diviations from the spec should be handled by the quirks:

http://linux.derkeiler.com/Mailing-Lists/Kernel/2009-02/msg03385.html

Spec clearly states that base clock == 0 is the case when we should fall back to the platform-provided clocks.

This includes platform ops, quirks and (de)initialization callbacks.

Signed-off-by Anton Vorontsov <[email protected]>

---
 drivers/mmc/host/sdhci-pltfm.c |   24 ++++++++++++++++++++----
 include/linux/sdhci-pltfm.h    |   27 +++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 4 deletions(-)
 create mode 100644 include/linux/sdhci-pltfm.h

drivers/mmc/host/sdhci-pltfm.c +20 -4

index 297f40a..217b911 100644
@@ -29,6 +29,7 @@
 #include <linux/mmc/host.h>
 
 #include <linux/io.h>
+#include <linux/sdhci-pltfm.h>
 
 #include "sdhci.h"
 

@@ -49,12 +50,11 @@ static struct sdhci_ops sdhci_pltfm_ops = {
 
 static int __devinit sdhci_pltfm_probe(struct platform_device *pdev)
 {
+	struct sdhci_pltfm_data *pdata = pdev->dev.platform_data;
 	struct sdhci_host *host;
 	struct resource *iomem;
 	int ret;
 
-	BUG_ON(pdev == NULL);
-
 	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!iomem) {
 		ret = -ENOMEM;

@@ -76,7 +76,12 @@ static int __devinit sdhci_pltfm_probe(struct platform_device *pdev)
 	}
 
 	host->hw_name = "platform";
-	host->ops = &sdhci_pltfm_ops;
+	if (pdata && pdata->ops)
+		host->ops = pdata->ops;
+	else
+		host->ops = &sdhci_pltfm_ops;
+	if (pdata)
+		host->quirks = pdata->quirks;
 	host->irq = platform_get_irq(pdev, 0);
 
 	if (!request_mem_region(iomem->start, resource_size(iomem),

@@ -93,6 +98,12 @@ static int __devinit sdhci_pltfm_probe(struct platform_device *pdev)
 		goto err_remap;
 	}
 
+	if (pdata && pdata->init) {
+		ret = pdata->init(host);
+		if (ret)
+			goto err_plat_init;
+	}
+
 	ret = sdhci_add_host(host);
 	if (ret)
 		goto err_add_host;

@@ -102,6 +113,9 @@ static int __devinit sdhci_pltfm_probe(struct platform_device *pdev)
 	return 0;
 
 err_add_host:
+	if (pdata && pdata->exit)
+		pdata->exit(host);
+err_plat_init:
 	iounmap(host->ioaddr);
 err_remap:
 	release_mem_region(iomem->start, resource_size(iomem));

@@ -114,6 +128,7 @@ err:
 
 static int __devexit sdhci_pltfm_remove(struct platform_device *pdev)
 {
+	struct sdhci_pltfm_data *pdata = pdev->dev.platform_data;
 	struct sdhci_host *host = platform_get_drvdata(pdev);
 	struct resource *iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	int dead;

@@ -125,6 +140,8 @@ static int __devexit sdhci_pltfm_remove(struct platform_device *pdev)
 		dead = 1;
 
 	sdhci_remove_host(host, dead);
+	if (pdata && pdata->exit)
+		pdata->exit(host);
 	iounmap(host->ioaddr);
 	release_mem_region(iomem->start, resource_size(iomem));
 	sdhci_free_host(host);

@@ -165,4 +182,3 @@ MODULE_DESCRIPTION("Secure Digital Host Controller Interface platform driver");
 MODULE_AUTHOR("Mocean Laboratories <[email protected]>");
 MODULE_LICENSE("GPL v2");
 MODULE_ALIAS("platform:sdhci");
-

include/linux/sdhci-pltfm.h new file +15 -0

new file mode 100644
index 0000000..0aaa334
@@ -0,0 +1,27 @@
+/*
+ * Platform data declarations for the sdhci-pltfm driver.
+ *
+ * Copyright (c) 2010 MontaVista Software, LLC.
+ *
+ * Author: Anton Vorontsov <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or (at
+ * your option) any later version.
+ */
+
+#ifndef _SDHCI_PLTFM_H
+#define _SDHCI_PLTFM_H

include/linux/sdhci-pltfm.h new file +2 -0

+
+#endif /* _SDHCI_PLTFM_H */

Quoted 1 line · Anton Vorontsov

This includes platform ops, quirks and (de)initialization callbacks.

Good idea, just a doc comment below.

Review

Quoted 6 lines

(quoted diff) +6 -0

+struct sdhci_pltfm_data {
+	struct sdhci_ops *ops;
+	unsigned int quirks;
+	int (*init)(struct sdhci_host *host);
+	void (*exit)(struct sdhci_host *host);
+};

I would like to see this struct kernel-doc:ed.

Some hosts have an extended SDHCI iomem size, so the driver should only print errors if the iomem size is less than 0x100.

Signed-off-by Anton Vorontsov <[email protected]>

---
 drivers/mmc/host/sdhci-pltfm.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/mmc/host/sdhci-pltfm.c +1 -1

index 217b911..b6ee0d7 100644
@@ -61,7 +61,7 @@ static int __devinit sdhci_pltfm_probe(struct platform_device *pdev)
 		goto err;
 	}
 
-	if (resource_size(iomem) != 0x100)
+	if (resource_size(iomem) < 0x100)
 		dev_err(&pdev->dev, "Invalid iomem size. You may "
 			"experience problems.\n");
 

Quoted 2 lines · Anton Vorontsov

Some hosts have an extended SDHCI iomem size, so the driver should only print errors if the iomem size is less than 0x100.

With this change you allow a bigger resource than we really need. I think you should also change request_mem_region and ioremap to only request and map the actual needed size. (0x100)

Quoted 2 lines · Anton Vorontsov

Some hosts have an extended SDHCI iomem size, so the driver should only print errors if the iomem size is less than 0x100.

With this change you allow a bigger resource than we really need. I think you should also change request_mem_region and ioremap to only request and map the actual needed size. (0x100)

Quoted 18 lines

Signed-off-by Anton Vorontsov <[email protected]>

---
 drivers/mmc/host/sdhci-pltfm.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/mmc/host/sdhci-pltfm.c +1 -1

index 217b911..b6ee0d7 100644
@@ -61,7 +61,7 @@ static int __devinit sdhci_pltfm_probe(struct platform_device *pdev)
 		goto err;
 	}
 
-	if (resource_size(iomem) != 0x100)
+	if (resource_size(iomem) < 0x100)
 		dev_err(&pdev->dev, "Invalid iomem size. You may "
 			"experience problems.\n");
 

Quoted 7 lines · Richard Röjfors

Quoted 2 lines · Anton Vorontsov

Some hosts have an extended SDHCI iomem size, so the driver should only print errors if the iomem size is less than 0x100.

With this change you allow a bigger resource than we really need. I think you should also change request_mem_region and ioremap to only request and map the actual needed size. (0x100)

Sorry for the delayed response.

Why do you think that mapping more than "needed" is a bad thing? Looking into the PCI driver, it just maps the whole PCI BAR (which makes sense, since later SDHCI spec might easily introduce an extended address space, so hardcoding 0x100 isn't very good).

There is another case when mapping the whole SDHCI mem space might be convenient: if the platform code will want to use the ioremapped region inside the platform hooks.

Thanks!

Quoted 19 lines

Quoted 18 lines

Signed-off-by Anton Vorontsov<[email protected]>

---
  drivers/mmc/host/sdhci-pltfm.c |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

drivers/mmc/host/sdhci-pltfm.c +1 -1

index 217b911..b6ee0d7 100644
@@ -61,7 +61,7 @@ static int __devinit sdhci_pltfm_probe(struct platform_device *pdev)
  		goto err;
  	}
 
-	if (resource_size(iomem) != 0x100)
+	if (resource_size(iomem)<  0x100)
  		dev_err(&pdev->dev, "Invalid iomem size. You may "
  			"experience problems.\n");