2012-05-02 05:12:16

by Thomas Abraham

[permalink] [raw]
Subject: [PATCH 0/7] mmc: dw_mmc: add support for device tree based instantiation

This patch series adds device tree support for Synopsis Designware Mobile
Storage Host Controller.

The first patch adds clock lookup in the driver and this is optional. Platforms
that do not need any clock gating and control for the dw_mmc controllers will
not be affected with this change. The second patch adds a quirk to notify the
controller about the absence of the write protect line.

The third patch adds device tree support and fourth patch adds Samsung
Exynos5250 specific controlller extentions to the driver. The rest of the
patches in the series enable support for dw_mmc support on the Samsing
Exynos5250 based platforms.

This patchset has been derived from Samsung internal patches prepared by
Abhilash Kesavan <[email protected]>.

Thomas Abraham (7):
mmc: dw_mmc: lookup for optional biu and ciu clocks
mmc: dw_mmc: add quirk to indicate missing write protect line
mmc: dw_mmc: add device tree support
mmc: dw_mmc: add samsung exynos5250 specific extentions
ARM: Samsung: Add support for MSHC controller clocks
ARM: Exynos5: Add AUXDATA support for MSHC controllers
ARM: dts: Add nodes for dw_mmc controllers for Samsung Exynos5250 platforms

.../devicetree/bindings/mmc/synposis-dw-mshc.txt | 116 +++++++++
arch/arm/boot/dts/exynos5250-smdk5250.dts | 50 ++++-
arch/arm/boot/dts/exynos5250.dtsi | 24 ++
arch/arm/mach-exynos/clock-exynos5.c | 45 ++---
arch/arm/mach-exynos/mach-exynos5-dt.c | 8 +
drivers/mmc/host/dw_mmc-pltfm.c | 32 +++
drivers/mmc/host/dw_mmc.c | 252 +++++++++++++++++++-
drivers/mmc/host/dw_mmc.h | 23 ++
include/linux/mmc/dw_mmc.h | 15 +-
9 files changed, 521 insertions(+), 44 deletions(-)
create mode 100644 Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt

--
1.7.5.4


2012-05-02 05:12:18

by Thomas Abraham

[permalink] [raw]
Subject: [PATCH 1/7] mmc: dw_mmc: lookup for optional biu and ciu clocks

Some platforms allow for clock gating and control of bus interface unit clock
and card interface unit clock. Add support for clock lookup of optional biu
and ciu clocks for clock gating and clock speed determination.

Signed-off-by: Abhilash Kesavan <[email protected]>
Signed-off-by: Thomas Abraham <[email protected]>
---
drivers/mmc/host/dw_mmc.c | 35 +++++++++++++++++++++++++++++++----
include/linux/mmc/dw_mmc.h | 4 ++++
2 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 1532357..036846f 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1938,19 +1938,35 @@ int dw_mci_probe(struct dw_mci *host)
return -ENODEV;
}

- if (!host->pdata->bus_hz) {
+ host->biu_clk = clk_get(&host->dev, "biu");
+ if (IS_ERR(host->biu_clk))
+ dev_info(&host->dev, "biu clock not available\n");
+ else
+ clk_enable(host->biu_clk);
+
+ host->ciu_clk = clk_get(&host->dev, "ciu");
+ if (IS_ERR(host->ciu_clk))
+ dev_info(&host->dev, "ciu clock not available\n");
+ else
+ clk_enable(host->ciu_clk);
+
+ if (IS_ERR(host->ciu_clk))
+ host->bus_hz = host->pdata->bus_hz;
+ else
+ host->bus_hz = clk_get_rate(host->ciu_clk);
+
+ if (!host->bus_hz) {
dev_err(&host->dev,
"Platform data must supply bus speed\n");
- return -ENODEV;
+ ret = -ENODEV;
+ goto err_clk;
}

- host->bus_hz = host->pdata->bus_hz;
host->quirks = host->pdata->quirks;

spin_lock_init(&host->lock);
INIT_LIST_HEAD(&host->queue);

-
host->dma_ops = host->pdata->dma_ops;
dw_mci_init_dma(host);

@@ -2095,6 +2111,13 @@ err_dmaunmap:
regulator_disable(host->vmmc);
regulator_put(host->vmmc);
}
+ kfree(host);
+
+err_clk:
+ clk_disable(host->ciu_clk);
+ clk_disable(host->biu_clk);
+ clk_put(host->ciu_clk);
+ clk_put(host->biu_clk);
return ret;
}
EXPORT_SYMBOL(dw_mci_probe);
@@ -2128,6 +2151,10 @@ void dw_mci_remove(struct dw_mci *host)
regulator_put(host->vmmc);
}

+ clk_disable(host->ciu_clk);
+ clk_disable(host->biu_clk);
+ clk_put(host->ciu_clk);
+ clk_put(host->biu_clk);
}
EXPORT_SYMBOL(dw_mci_remove);

diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
index 7a7ebd3..fa9a139 100644
--- a/include/linux/mmc/dw_mmc.h
+++ b/include/linux/mmc/dw_mmc.h
@@ -78,6 +78,8 @@ struct mmc_data;
* @data_offset: Set the offset of DATA register according to VERID.
* @dev: Device associated with the MMC controller.
* @pdata: Platform data associated with the MMC controller.
+ * @biu_clk: Pointer to bus interface unit clock instance.
+ * @ciu_clk: Pointer to card interface unit clock instance.
* @slot: Slots sharing this MMC controller.
* @fifo_depth: depth of FIFO.
* @data_shift: log2 of FIFO item size.
@@ -158,6 +160,8 @@ struct dw_mci {
u16 data_offset;
struct device dev;
struct dw_mci_board *pdata;
+ struct clk *biu_clk;
+ struct clk *ciu_clk;
struct dw_mci_slot *slot[MAX_MCI_SLOTS];

/* FIFO push and pull */
--
1.7.5.4

2012-05-02 05:12:27

by Thomas Abraham

[permalink] [raw]
Subject: [PATCH 6/7] ARM: Exynos5: Add AUXDATA support for MSHC controllers

Add entries if MSHC controllers in AUXDATA table for correct device name
initialization.

Signed-off-by: Abhilash Kesavan <[email protected]>
Signed-off-by: Thomas Abraham <[email protected]>
---
arch/arm/mach-exynos/mach-exynos5-dt.c | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-exynos/mach-exynos5-dt.c b/arch/arm/mach-exynos/mach-exynos5-dt.c
index 00e25fc..4a060f1 100644
--- a/arch/arm/mach-exynos/mach-exynos5-dt.c
+++ b/arch/arm/mach-exynos/mach-exynos5-dt.c
@@ -72,6 +72,14 @@ static const struct of_dev_auxdata exynos5250_auxdata_lookup[] __initconst = {
"s3c2440-i2c.0", NULL),
OF_DEV_AUXDATA("samsung,s3c2440-i2c", EXYNOS5_PA_IIC(1),
"s3c2440-i2c.1", NULL),
+ OF_DEV_AUXDATA("synopsis,dw-mshc-exynos5250", 0x12200000,
+ "dw_mmc.0", NULL),
+ OF_DEV_AUXDATA("synopsis,dw-mshc-exynos5250", 0x12210000,
+ "dw_mmc.1", NULL),
+ OF_DEV_AUXDATA("synopsis,dw-mshc-exynos5250", 0x12220000,
+ "dw_mmc.2", NULL),
+ OF_DEV_AUXDATA("synopsis,dw-mshc-exynos5250", 0x12230000,
+ "dw_mmc.3", NULL),
OF_DEV_AUXDATA("arm,pl330", EXYNOS5_PA_PDMA0, "dma-pl330.0", NULL),
OF_DEV_AUXDATA("arm,pl330", EXYNOS5_PA_PDMA1, "dma-pl330.1", NULL),
OF_DEV_AUXDATA("arm,pl330", EXYNOS5_PA_MDMA1, "dma-pl330.2", NULL),
--
1.7.5.4

2012-05-02 05:12:40

by Thomas Abraham

[permalink] [raw]
Subject: [PATCH 7/7] ARM: dts: Add nodes for dw_mmc controllers for Samsung Exynos5250 platforms

Add device nodes for the four instances of dw_mmc controllers in Exynos5250
and enable instance 0 and 2 for the smdk5250 board.

Signed-off-by: Thomas Abraham <[email protected]>
---
arch/arm/boot/dts/exynos5250-smdk5250.dts | 50 ++++++++++++++++++++++++++++-
arch/arm/boot/dts/exynos5250.dtsi | 24 ++++++++++++++
2 files changed, 73 insertions(+), 1 deletions(-)

diff --git a/arch/arm/boot/dts/exynos5250-smdk5250.dts b/arch/arm/boot/dts/exynos5250-smdk5250.dts
index dbc4bdb..ac9df2f 100644
--- a/arch/arm/boot/dts/exynos5250-smdk5250.dts
+++ b/arch/arm/boot/dts/exynos5250-smdk5250.dts
@@ -86,4 +86,52 @@
i2c@12CD0000 {
status = "disabled";
};
+
+ dwmmc0@12200000 {
+ supports-highspeed;
+ card-detection-broken;
+ no-write-protect;
+ fifo-depth = <0x80>;
+ card-detect-delay = <200>;
+ samsung,dw-mshc-sdr-timing = <2 3 3>;
+ samsung,dw-mshc-ddr-timing = <1 2 3>;
+
+ slot0 {
+ bus-width = <8>;
+ cd-gpios = <&gpc0 2 2 3 3>;
+ gpios = <&gpc0 0 2 0 3>, <&gpc0 1 2 0 3>,
+ <&gpc1 0 2 3 3>, <&gpc1 1 2 3 3>,
+ <&gpc1 2 2 3 3>, <&gpc1 3 2 3 3>,
+ <&gpc0 3 2 3 3>, <&gpc0 4 2 3 3>,
+ <&gpc0 5 2 3 3>, <&gpc0 6 2 3 3>;
+ };
+ };
+
+ dwmmc1@12210000 {
+ status = "disabled";
+ };
+
+ dwmmc2@12220000 {
+ supports-highspeed;
+ card-detection-broken;
+ no-write-protect;
+ fifo-depth = <0x80>;
+ card-detect-delay = <200>;
+ samsung,dw-mshc-sdr-timing = <2 3 3>;
+ samsung,dw-mshc-ddr-timing = <1 2 3>;
+
+ slot0 {
+ bus-width = <4>;
+ cd-gpios = <&gpc3 2 2 3 3>;
+ gpios = <&gpc3 0 2 0 3>, <&gpc3 1 2 0 3>,
+ <&gpc3 3 2 3 3>, <&gpc3 4 2 3 3>,
+ <&gpc3 5 2 3 3>, <&gpc3 6 2 3 3>,
+ <&gpc4 3 3 3 3>, <&gpc4 3 3 3 3>,
+ <&gpc4 5 3 3 3>, <&gpc4 6 3 3 3>;
+ };
+ };
+
+ dwmmc3@12230000 {
+ status = "disabled";
+ };
};
diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
index af124917..345e0bb 100644
--- a/arch/arm/boot/dts/exynos5250.dtsi
+++ b/arch/arm/boot/dts/exynos5250.dtsi
@@ -169,6 +169,30 @@
interrupts = <0 63 0>;
};

+ dwmmc0@12200000 {
+ compatible = "synopsis,dw-mshc-exynos5250";
+ reg = <0x12200000 0x1000>;
+ interrupts = <0 75 0>;
+ };
+
+ dwmmc1@12210000 {
+ compatible = "synopsis,dw-mshc-exynos5250";
+ reg = <0x12210000 0x1000>;
+ interrupts = <0 76 0>;
+ };
+
+ dwmmc2@12220000 {
+ compatible = "synopsis,dw-mshc-exynos5250";
+ reg = <0x12220000 0x1000>;
+ interrupts = <0 77 0>;
+ };
+
+ dwmmc3@12230000 {
+ compatible = "synopsis,dw-mshc-exynos5250";
+ reg = <0x12230000 0x1000>;
+ interrupts = <0 78 0>;
+ };
+
amba {
#address-cells = <1>;
#size-cells = <1>;
--
1.7.5.4

2012-05-02 05:13:26

by Thomas Abraham

[permalink] [raw]
Subject: [PATCH 5/7] ARM: Samsung: Add support for MSHC controller clocks

Add clock instances for bus interface unit clock and card interface unit
clock of the all four MSHC controller instances.

Signed-off-by: Abhilash Kesavan <[email protected]>
Signed-off-by: Thomas Abraham <[email protected]>
---
arch/arm/mach-exynos/clock-exynos5.c | 45 ++++++++++++----------------------
1 files changed, 16 insertions(+), 29 deletions(-)

diff --git a/arch/arm/mach-exynos/clock-exynos5.c b/arch/arm/mach-exynos/clock-exynos5.c
index 7c0f810..4e17131 100644
--- a/arch/arm/mach-exynos/clock-exynos5.c
+++ b/arch/arm/mach-exynos/clock-exynos5.c
@@ -524,35 +524,30 @@ static struct clk exynos5_init_clocks_off[] = {
.enable = exynos5_clk_ip_peris_ctrl,
.ctrlbit = (1 << 19),
}, {
- .name = "hsmmc",
- .devname = "exynos4-sdhci.0",
+ .name = "biu",
+ .devname = "dw_mmc.0",
.parent = &exynos5_clk_aclk_200.clk,
.enable = exynos5_clk_ip_fsys_ctrl,
.ctrlbit = (1 << 12),
}, {
- .name = "hsmmc",
- .devname = "exynos4-sdhci.1",
+ .name = "biu",
+ .devname = "dw_mmc.1",
.parent = &exynos5_clk_aclk_200.clk,
.enable = exynos5_clk_ip_fsys_ctrl,
.ctrlbit = (1 << 13),
}, {
- .name = "hsmmc",
- .devname = "exynos4-sdhci.2",
+ .name = "biu",
+ .devname = "dw_mmc.2",
.parent = &exynos5_clk_aclk_200.clk,
.enable = exynos5_clk_ip_fsys_ctrl,
.ctrlbit = (1 << 14),
}, {
- .name = "hsmmc",
- .devname = "exynos4-sdhci.3",
+ .name = "biu",
+ .devname = "dw_mmc.3",
.parent = &exynos5_clk_aclk_200.clk,
.enable = exynos5_clk_ip_fsys_ctrl,
.ctrlbit = (1 << 15),
}, {
- .name = "dwmci",
- .parent = &exynos5_clk_aclk_200.clk,
- .enable = exynos5_clk_ip_fsys_ctrl,
- .ctrlbit = (1 << 16),
- }, {
.name = "sata",
.devname = "ahci",
.enable = exynos5_clk_ip_fsys_ctrl,
@@ -882,8 +877,8 @@ static struct clksrc_clk exynos5_clk_sclk_uart3 = {

static struct clksrc_clk exynos5_clk_sclk_mmc0 = {
.clk = {
- .name = "sclk_mmc",
- .devname = "exynos4-sdhci.0",
+ .name = "ciu",
+ .devname = "dw_mmc.0",
.parent = &exynos5_clk_dout_mmc0.clk,
.enable = exynos5_clksrc_mask_fsys_ctrl,
.ctrlbit = (1 << 0),
@@ -893,8 +888,8 @@ static struct clksrc_clk exynos5_clk_sclk_mmc0 = {

static struct clksrc_clk exynos5_clk_sclk_mmc1 = {
.clk = {
- .name = "sclk_mmc",
- .devname = "exynos4-sdhci.1",
+ .name = "ciu",
+ .devname = "dw_mmc.1",
.parent = &exynos5_clk_dout_mmc1.clk,
.enable = exynos5_clksrc_mask_fsys_ctrl,
.ctrlbit = (1 << 4),
@@ -904,8 +899,8 @@ static struct clksrc_clk exynos5_clk_sclk_mmc1 = {

static struct clksrc_clk exynos5_clk_sclk_mmc2 = {
.clk = {
- .name = "sclk_mmc",
- .devname = "exynos4-sdhci.2",
+ .name = "ciu",
+ .devname = "dw_mmc.2",
.parent = &exynos5_clk_dout_mmc2.clk,
.enable = exynos5_clksrc_mask_fsys_ctrl,
.ctrlbit = (1 << 8),
@@ -915,8 +910,8 @@ static struct clksrc_clk exynos5_clk_sclk_mmc2 = {

static struct clksrc_clk exynos5_clk_sclk_mmc3 = {
.clk = {
- .name = "sclk_mmc",
- .devname = "exynos4-sdhci.3",
+ .name = "ciu",
+ .devname = "dw_mmc.3",
.parent = &exynos5_clk_dout_mmc3.clk,
.enable = exynos5_clksrc_mask_fsys_ctrl,
.ctrlbit = (1 << 12),
@@ -927,14 +922,6 @@ static struct clksrc_clk exynos5_clk_sclk_mmc3 = {
static struct clksrc_clk exynos5_clksrcs[] = {
{
.clk = {
- .name = "sclk_dwmci",
- .parent = &exynos5_clk_dout_mmc4.clk,
- .enable = exynos5_clksrc_mask_fsys_ctrl,
- .ctrlbit = (1 << 16),
- },
- .reg_div = { .reg = EXYNOS5_CLKDIV_FSYS3, .shift = 8, .size = 8 },
- }, {
- .clk = {
.name = "sclk_fimd",
.devname = "s3cfb.1",
.enable = exynos5_clksrc_mask_disp1_0_ctrl,
--
1.7.5.4

2012-05-02 05:12:23

by Thomas Abraham

[permalink] [raw]
Subject: [PATCH 3/7] mmc: dw_mmc: add device tree support

Add device tree based discovery support.

Signed-off-by: Thomas Abraham <[email protected]>
---
.../devicetree/bindings/mmc/synposis-dw-mshc.txt | 85 +++++++++
drivers/mmc/host/dw_mmc-pltfm.c | 24 +++
drivers/mmc/host/dw_mmc.c | 181 +++++++++++++++++++-
drivers/mmc/host/dw_mmc.h | 10 +
include/linux/mmc/dw_mmc.h | 2 +
5 files changed, 296 insertions(+), 6 deletions(-)
create mode 100644 Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt

diff --git a/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
new file mode 100644
index 0000000..c1ed70e
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
@@ -0,0 +1,85 @@
+* Synopsis Designware Mobile Storage Host Controller
+
+The Synopsis designware mobile storage host controller is used to interface
+a SoC with storage medium such as eMMC or SD/MMC cards.
+
+Required Properties:
+
+* compatible: should be one of the following
+ - synopsis,dw-mshc: for controllers compliant with synopsis dw-mshc.
+
+* reg: physical base address of the dw-mshc controller and size of its memory
+ region.
+
+* interrupts: interrupt specifier for the controller. The format and value of
+ the interrupt specifier depends on the interrupt parent for the controller.
+
+# Slots: The slot specific information are contained within child-nodes with
+ each child-node representing a supported slot. There should be atleast one
+ child node representing a card slot. The name of the slot child node should
+ be 'slot{n}' where n is the unique number of the slot connnected to the
+ controller. The following are optional properties which can be included in
+ the slot child node.
+
+ * bus-width: specifies the width of the data bus connected from the
+ controller to the card slot. The value should be 1, 4 or 8. In case
+ this property is not specified, a default value of 1 is assumed for
+ this property.
+
+ * cd-gpios: specifies the card detect gpio line. The format of the
+ gpio specifier depends on the gpio controller.
+
+ * wp-gpios: specifies the write protect gpio line. The format of the
+ gpio specifier depends on the gpio controller.
+
+ * gpios: specifies a list of gpios used for command, clock and data
+ bus. The first gpio is the command line and the second gpio is the
+ clock line. The rest of the gpios (depending on the bus-width
+ property) are the data lines in no particular order. The format of
+ the gpio specifier depends on the gpio controller.
+
+Optional properties:
+
+* fifo-depth: The maximum size of the tx/rx fifo's. If this property is not
+ specified, the default value of the fifo size is determined from the
+ controller registers.
+
+* card-detect-delay: Delay in milli-seconds before detecting card after card
+ insert event. The default value is 0.
+
+* supports-highspeed: Enables support for high speed cards (upto 50MHz)
+
+* card-detection-broken: The card detection functionality is not available on
+ any of the slots.
+
+* no-write-protect: The write protect pad of the controller is not connected
+ to the write protect pin on the slot.
+
+Example:
+
+ The MSHC controller node can be split into two portions, SoC specific and
+ board specific portions as listed below.
+
+ dwmmc0@12200000 {
+ compatible = "synopsis,dw-mshc";
+ reg = <0x12200000 0x1000>;
+ interrupts = <0 75 0>;
+ };
+
+ dwmmc0@12200000 {
+ supports-highspeed;
+ card-detection-broken;
+ no-write-protect;
+ fifo-depth = <0x80>;
+ card-detect-delay = <200>;
+
+ slot0 {
+ bus-width = <8>;
+ cd-gpios = <&gpc0 2 2 3 3>;
+ gpios = <&gpc0 0 2 0 3>, <&gpc0 1 2 0 3>,
+ <&gpc1 0 2 3 3>, <&gpc1 1 2 3 3>,
+ <&gpc1 2 2 3 3>, <&gpc1 3 2 3 3>,
+ <&gpc0 3 2 3 3>, <&gpc0 4 2 3 3>,
+ <&gpc0 5 2 3 3>, <&gpc0 6 2 3 3>;
+ };
+ };
diff --git a/drivers/mmc/host/dw_mmc-pltfm.c b/drivers/mmc/host/dw_mmc-pltfm.c
index 92ec3eb..2b2c9bd 100644
--- a/drivers/mmc/host/dw_mmc-pltfm.c
+++ b/drivers/mmc/host/dw_mmc-pltfm.c
@@ -19,8 +19,24 @@
#include <linux/mmc/host.h>
#include <linux/mmc/mmc.h>
#include <linux/mmc/dw_mmc.h>
+#include <linux/of.h>
#include "dw_mmc.h"

+#ifdef CONFIG_OF
+static struct dw_mci_drv_data synopsis_drv_data = {
+ .ctrl_type = DW_MCI_TYPE_SYNOPSIS,
+};
+
+static const struct of_device_id dw_mci_pltfm_match[] = {
+ { .compatible = "synopsis,dw-mshc",
+ .data = (void *)&synopsis_drv_data, },
+ {},
+};
+MODULE_DEVICE_TABLE(of, dw_mci_pltfm_match);
+#else
+static const struct of_device_id dw_mci_pltfm_match[];
+#endif
+
static int dw_mci_pltfm_probe(struct platform_device *pdev)
{
struct dw_mci *host;
@@ -51,6 +67,13 @@ static int dw_mci_pltfm_probe(struct platform_device *pdev)
if (!host->regs)
goto err_free;
platform_set_drvdata(pdev, host);
+
+ if (pdev->dev.of_node) {
+ const struct of_device_id *match;
+ match = of_match_node(dw_mci_pltfm_match, pdev->dev.of_node);
+ host->drv_data = match->data;
+ }
+
ret = dw_mci_probe(host);
if (ret)
goto err_out;
@@ -111,6 +134,7 @@ static struct platform_driver dw_mci_pltfm_driver = {
.remove = __exit_p(dw_mci_pltfm_remove),
.driver = {
.name = "dw_mmc",
+ .of_match_table = of_match_ptr(dw_mci_pltfm_match),
.pm = &dw_mci_pltfm_pmops,
},
};
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 38cb7f8..bcf66d7 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -33,9 +33,13 @@
#include <linux/bitops.h>
#include <linux/regulator/consumer.h>
#include <linux/workqueue.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>

#include "dw_mmc.h"

+#define NUM_PINS(x) (x + 2)
+
/* Common flag combinations */
#define DW_MCI_DATA_ERROR_FLAGS (SDMMC_INT_DTO | SDMMC_INT_DCRC | \
SDMMC_INT_HTO | SDMMC_INT_SBE | \
@@ -86,6 +90,8 @@ struct idmac_desc {
struct dw_mci_slot {
struct mmc_host *mmc;
struct dw_mci *host;
+ int wp_gpio;
+ int cd_gpio;

u32 ctype;

@@ -816,6 +822,8 @@ static int dw_mci_get_ro(struct mmc_host *mmc)
read_only = 0;
else if (brd->get_ro)
read_only = brd->get_ro(slot->id);
+ else if (gpio_is_valid(slot->wp_gpio))
+ read_only = gpio_get_value(slot->wp_gpio);
else
read_only =
mci_readl(slot->host, WRTPRT) & (1 << slot->id) ? 1 : 0;
@@ -837,6 +845,8 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
present = 1;
else if (brd->get_cd)
present = !brd->get_cd(slot->id);
+ else if (gpio_is_valid(slot->cd_gpio))
+ present = gpio_get_value(slot->cd_gpio);
else
present = (mci_readl(slot->host, CDETECT) & (1 << slot->id))
== 0 ? 1 : 0;
@@ -1747,6 +1757,96 @@ static void dw_mci_work_routine_card(struct work_struct *work)
}
}

+#ifdef CONFIG_OF
+static struct device_node *dw_mci_of_find_slot_node(struct device *dev, u8 slot)
+{
+ struct device_node *np;
+ char name[7];
+
+ if (!dev || !dev->of_node)
+ return NULL;
+
+ for_each_child_of_node(dev->of_node, np) {
+ sprintf(name, "slot%d", slot);
+ if (!strcmp(name, np->name))
+ return np;
+ }
+ return NULL;
+}
+
+static u32 dw_mci_of_get_bus_wd(struct device *dev, u8 slot)
+{
+ struct device_node *np = dw_mci_of_find_slot_node(dev, slot);
+ u32 bus_wd = 1;
+
+ if (!np)
+ return 1;
+
+ if (of_property_read_u32(np, "bus-width", &bus_wd))
+ dev_err(dev, "bus-width property not found, assuming width"
+ " as 1\n");
+ return bus_wd;
+}
+
+static int dw_mci_of_setup_bus(struct dw_mci *host, u8 slot, u32 bus_wd)
+{
+ struct device_node *np = dw_mci_of_find_slot_node(&host->dev, slot);
+ int idx, gpio, ret;
+
+ for (idx = 0; idx < NUM_PINS(bus_wd); idx++) {
+ gpio = of_get_gpio(np, idx);
+ if (!gpio_is_valid(gpio)) {
+ dev_err(&host->dev, "invalid gpio: %d\n", gpio);
+ return -EINVAL;
+ }
+
+ ret = devm_gpio_request(&host->dev, gpio, "dw-mci-bus");
+ if (ret) {
+ dev_err(&host->dev, "gpio [%d] request failed\n", gpio);
+ return -EBUSY;
+ }
+ }
+
+ host->slot[slot]->wp_gpio = -1;
+ gpio = of_get_named_gpio(np, "wp_gpios", 0);
+ if (!gpio_is_valid(gpio)) {
+ dev_info(&host->dev, "wp gpio not available");
+ } else {
+ ret = devm_gpio_request(&host->dev, gpio, "dw-mci-wp");
+ if (ret)
+ dev_info(&host->dev, "gpio [%d] request failed\n",
+ gpio);
+ else
+ host->slot[slot]->wp_gpio = gpio;
+ }
+
+ host->slot[slot]->cd_gpio = -1;
+ gpio = of_get_named_gpio(np, "cd-gpios", 0);
+ if (!gpio_is_valid(gpio)) {
+ dev_info(&host->dev, "cd gpio not available");
+ } else {
+ ret = devm_gpio_request(&host->dev, gpio, "dw-mci-cd");
+ if (ret)
+ dev_err(&host->dev, "gpio [%d] request failed\n", gpio);
+ else
+ host->slot[slot]->cd_gpio = gpio;
+ }
+
+ return 0;
+}
+
+#else /* CONFIG_OF */
+static u32 dw_mci_of_get_bus_wd(struct device *dev, u8 slot)
+{
+ return 1;
+}
+
+static int dw_mci_of_setup_bus(struct dw_mci *host, u8 slot, u32 bus_wd)
+{
+ return -EINVAL;
+}
+#endif /* CONFIG_OF */
+
static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id)
{
struct mmc_host *mmc;
@@ -1760,6 +1860,7 @@ static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id)
slot->id = id;
slot->mmc = mmc;
slot->host = host;
+ host->slot[id] = slot;

mmc->ops = &dw_mci_ops;
mmc->f_min = DIV_ROUND_UP(host->bus_hz, 510);
@@ -1780,12 +1881,21 @@ static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id)
if (host->pdata->caps)
mmc->caps = host->pdata->caps;

+ mmc->caps |= host->drv_data->caps;
+
if (host->pdata->caps2)
mmc->caps2 = host->pdata->caps2;

- if (host->pdata->get_bus_wd)
+ if (host->pdata->get_bus_wd) {
if (host->pdata->get_bus_wd(slot->id) >= 4)
mmc->caps |= MMC_CAP_4_BIT_DATA;
+ } else if (host->dev.of_node) {
+ unsigned int bus_width;
+ bus_width = dw_mci_of_get_bus_wd(&host->dev, slot->id);
+ if (bus_width >= 4)
+ mmc->caps |= MMC_CAP_4_BIT_DATA;
+ dw_mci_of_setup_bus(host, slot->id, bus_width);
+ }

if (host->pdata->quirks & DW_MCI_QUIRK_HIGHSPEED)
mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
@@ -1830,7 +1940,6 @@ static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id)
else
clear_bit(DW_MMC_CARD_PRESENT, &slot->flags);

- host->slot[id] = slot;
mmc_add_host(mmc);

#if defined(CONFIG_DEBUG_FS)
@@ -1923,15 +2032,75 @@ static bool mci_wait_reset(struct device *dev, struct dw_mci *host)
return false;
}

+#ifdef CONFIG_OF
+static struct dw_mci_of_quirks {
+ char *quirk;
+ int id;
+} of_quriks[] = {
+ {
+ .quirk = "supports-highspeed",
+ .id = DW_MCI_QUIRK_HIGHSPEED,
+ }, {
+ .quirk = "card-detection-broken",
+ .id = DW_MCI_QUIRK_BROKEN_CARD_DETECTION,
+ }, {
+ .quirk = "no-write-protect",
+ .id = DW_MCI_QUIRK_NO_WRITE_PROTECT,
+ },
+ { }
+};
+
+static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
+{
+ struct dw_mci_board *pdata;
+ struct device *dev = &host->dev;
+ struct device_node *np = dev->of_node, *slot;
+ u32 timing[3];
+ int idx, cnt;
+
+ pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata) {
+ dev_err(dev, "could not allocate memory for pdata\n");
+ return ERR_PTR(-ENOMEM);
+ }
+
+ /* find out number of slots supported */
+ for_each_child_of_node(np, slot)
+ pdata->num_slots++;
+
+ /* get quirks */
+ cnt = sizeof(of_quriks) / sizeof(struct dw_mci_of_quirks);
+ for (idx = 0; idx < cnt; idx++)
+ if (of_get_property(np, of_quriks[idx].quirk, NULL))
+ pdata->quirks |= of_quriks[idx].id;
+
+ if (of_property_read_u32(np, "fifo-depth", &pdata->fifo_depth))
+ dev_info(dev, "fifo-depth property not found, using "
+ "value of FIFOTH register as default\n");
+
+ of_property_read_u32(np, "card-detect-delay", &pdata->detect_delay_ms);
+
+ return pdata;
+}
+
+#else /* CONFIG_OF */
+static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
+{
+ return ERR_PTR(-EINVAL);
+}
+#endif /* CONFIG_OF */
+
int dw_mci_probe(struct dw_mci *host)
{
int width, i, ret = 0;
u32 fifo_size;

- if (!host->pdata || !host->pdata->init) {
- dev_err(&host->dev,
- "Platform data must supply init function\n");
- return -ENODEV;
+ if (!host->pdata) {
+ host->pdata = dw_mci_parse_dt(host);
+ if (IS_ERR(host->pdata)) {
+ dev_err(&host->dev, "platform data not available\n");
+ return -EINVAL;
+ }
}

if (!host->pdata->select_slot && host->pdata->num_slots > 1) {
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index 15c27e1..8b8862b 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -182,4 +182,14 @@ extern int dw_mci_suspend(struct dw_mci *host);
extern int dw_mci_resume(struct dw_mci *host);
#endif

+/* Variations in the dw_mci controller */
+#define DW_MCI_TYPE_SYNOPSIS 0
+#define DW_MCI_TYPE_EXYNOS5250 1 /* Samsung Exynos5250 Extensions */
+
+/* dw_mci platform driver data */
+struct dw_mci_drv_data {
+ unsigned long ctrl_type;
+ unsigned long caps;
+};
+
#endif /* _DW_MMC_H_ */
diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
index fb51a5f..71d2b56 100644
--- a/include/linux/mmc/dw_mmc.h
+++ b/include/linux/mmc/dw_mmc.h
@@ -78,6 +78,7 @@ struct mmc_data;
* @data_offset: Set the offset of DATA register according to VERID.
* @dev: Device associated with the MMC controller.
* @pdata: Platform data associated with the MMC controller.
+ * @drv_data: Driver specific data for identified variant of the controller
* @biu_clk: Pointer to bus interface unit clock instance.
* @ciu_clk: Pointer to card interface unit clock instance.
* @slot: Slots sharing this MMC controller.
@@ -160,6 +161,7 @@ struct dw_mci {
u16 data_offset;
struct device dev;
struct dw_mci_board *pdata;
+ struct dw_mci_drv_data *drv_data;
struct clk *biu_clk;
struct clk *ciu_clk;
struct dw_mci_slot *slot[MAX_MCI_SLOTS];
--
1.7.5.4

2012-05-02 05:13:55

by Thomas Abraham

[permalink] [raw]
Subject: [PATCH 4/7] mmc: dw_mmc: add samsung exynos5250 specific extentions

The instantiation of the Synopsis Designware controller on Exynos5250
include extension for SDR and DDR specific tx/rx phase shift timing
and CIU internal divider. In addition to that, the option to skip the
command hold stage is also introduced. Add support for these Exynos5250
specfic extenstions.

Signed-off-by: Abhilash Kesavan <[email protected]>
Signed-off-by: Thomas Abraham <[email protected]>
---
.../devicetree/bindings/mmc/synposis-dw-mshc.txt | 33 +++++++++++++++++++-
drivers/mmc/host/dw_mmc-pltfm.c | 8 +++++
drivers/mmc/host/dw_mmc.c | 32 ++++++++++++++++++-
drivers/mmc/host/dw_mmc.h | 13 ++++++++
include/linux/mmc/dw_mmc.h | 6 +++
5 files changed, 89 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
index c1ed70e..465fc31 100644
--- a/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
+++ b/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
@@ -7,6 +7,8 @@ Required Properties:

* compatible: should be one of the following
- synopsis,dw-mshc: for controllers compliant with synopsis dw-mshc.
+ - synopsis,dw-mshc-exynos5250: for controllers with Samsung
+ Exynos5250 specific extentions.

* reg: physical base address of the dw-mshc controller and size of its memory
region.
@@ -55,13 +57,40 @@ Optional properties:
* no-write-protect: The write protect pad of the controller is not connected
to the write protect pin on the slot.

+Samsung Exynos5250 specific properties:
+
+* samsung,dw-mshc-sdr-timing: Specifies the value of CUI clock divider, CIU
+ clock phase shift value in transmit mode and CIU clock phase shift value in
+ receive mode for single data rate mode operation. Refer notes of the valid
+ values below.
+
+* samsung,dw-mshc-ddr-timing: Specifies the value of CUI clock divider, CIU
+ clock phase shift value in transmit mode and CIU clock phase shift value in
+ receive mode for double data rate mode operation. Refer notes of the valid
+ values below. The order of the cells should be
+
+ - First Cell: CIU clock divider value.
+ - Second Cell: CIU clock phase shift value for tx mode.
+ - Third Cell: CIU clock phase shift value for rx mode.
+
+ Valid values for SDR and DDR CIU clock timing:
+
+ - valid values for CIU clock divider, tx phase shift and rx phase shift
+ is 0 to 7.
+
+ - When CIU clock divider value is set to 3, all possible 8 phase shift
+ values can be used.
+
+ - If CIU clock divider value is 0 (that is divide by 1), both tx and rx
+ phase shift clocks should be 0.
+
Example:

The MSHC controller node can be split into two portions, SoC specific and
board specific portions as listed below.

dwmmc0@12200000 {
- compatible = "synopsis,dw-mshc";
+ compatible = "synopsis,dw-mshc-exynos5250";
reg = <0x12200000 0x1000>;
interrupts = <0 75 0>;
};
@@ -72,6 +101,8 @@ Example:
no-write-protect;
fifo-depth = <0x80>;
card-detect-delay = <200>;
+ samsung,dw-mshc-sdr-timing = <2 3 3>;
+ samsung,dw-mshc-ddr-timing = <1 2 3>;

slot0 {
bus-width = <8>;
diff --git a/drivers/mmc/host/dw_mmc-pltfm.c b/drivers/mmc/host/dw_mmc-pltfm.c
index 2b2c9bd..35056fd 100644
--- a/drivers/mmc/host/dw_mmc-pltfm.c
+++ b/drivers/mmc/host/dw_mmc-pltfm.c
@@ -27,9 +27,17 @@ static struct dw_mci_drv_data synopsis_drv_data = {
.ctrl_type = DW_MCI_TYPE_SYNOPSIS,
};

+static struct dw_mci_drv_data exynos5250_drv_data = {
+ .ctrl_type = DW_MCI_TYPE_EXYNOS5250,
+ .caps = MMC_CAP_UHS_DDR50 | MMC_CAP_1_8V_DDR |
+ MMC_CAP_8_BIT_DATA | MMC_CAP_CMD23,
+};
+
static const struct of_device_id dw_mci_pltfm_match[] = {
{ .compatible = "synopsis,dw-mshc",
.data = (void *)&synopsis_drv_data, },
+ { .compatible = "synopsis,dw-mshc-exynos5250",
+ .data = (void *)&exynos5250_drv_data, },
{},
};
MODULE_DEVICE_TABLE(of, dw_mci_pltfm_match);
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index bcf66d7..9174a69 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -236,6 +236,7 @@ static void dw_mci_set_timeout(struct dw_mci *host)
static u32 dw_mci_prepare_command(struct mmc_host *mmc, struct mmc_command *cmd)
{
struct mmc_data *data;
+ struct dw_mci_slot *slot = mmc_priv(mmc);
u32 cmdr;
cmd->error = -EINPROGRESS;

@@ -265,6 +266,10 @@ static u32 dw_mci_prepare_command(struct mmc_host *mmc, struct mmc_command *cmd)
cmdr |= SDMMC_CMD_DAT_WR;
}

+ if (slot->host->drv_data->ctrl_type == DW_MCI_TYPE_EXYNOS5250)
+ if (SDMMC_CLKSEL_GET_SELCLK_DRV(mci_readl(slot->host, CLKSEL)))
+ cmdr |= SDMMC_USE_HOLD_REG;
+
return cmdr;
}

@@ -787,10 +792,19 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
regs = mci_readl(slot->host, UHS_REG);

/* DDR mode set */
- if (ios->timing == MMC_TIMING_UHS_DDR50)
+ if (ios->timing == MMC_TIMING_UHS_DDR50) {
regs |= (0x1 << slot->id) << 16;
- else
+ mci_writel(slot->host, CLKSEL, slot->host->ddr_timing);
+ } else {
regs &= ~(0x1 << slot->id) << 16;
+ mci_writel(slot->host, CLKSEL, slot->host->sdr_timing);
+ }
+
+ if (slot->host->drv_data->ctrl_type == DW_MCI_TYPE_EXYNOS5250) {
+ slot->host->bus_hz = clk_get_rate(slot->host->ciu_clk);
+ slot->host->bus_hz /= SDMMC_CLKSEL_GET_DIVRATIO(
+ mci_readl(slot->host, CLKSEL));
+ }

mci_writel(slot->host, UHS_REG, regs);

@@ -2074,6 +2088,20 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
if (of_get_property(np, of_quriks[idx].quirk, NULL))
pdata->quirks |= of_quriks[idx].id;

+ if (of_property_read_u32_array(dev->of_node,
+ "samsung,dw-mshc-sdr-timing", timing, 3))
+ host->sdr_timing = DW_MCI_DEF_SDR_TIMING;
+ else
+ host->sdr_timing = SDMMC_CLKSEL_TIMING(timing[0],
+ timing[1], timing[2]);
+
+ if (of_property_read_u32_array(dev->of_node,
+ "samsung,dw-mshc-ddr-timing", timing, 3))
+ host->ddr_timing = DW_MCI_DEF_DDR_TIMING;
+ else
+ host->ddr_timing = SDMMC_CLKSEL_TIMING(timing[0],
+ timing[1], timing[2]);
+
if (of_property_read_u32(np, "fifo-depth", &pdata->fifo_depth))
dev_info(dev, "fifo-depth property not found, using "
"value of FIFOTH register as default\n");
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index 8b8862b..4b7e42b 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -53,6 +53,7 @@
#define SDMMC_IDINTEN 0x090
#define SDMMC_DSCADDR 0x094
#define SDMMC_BUFADDR 0x098
+#define SDMMC_CLKSEL 0x09C /* specific to Samsung Exynos5250 */
#define SDMMC_DATA(x) (x)

/*
@@ -111,6 +112,7 @@
#define SDMMC_INT_ERROR 0xbfc2
/* Command register defines */
#define SDMMC_CMD_START BIT(31)
+#define SDMMC_USE_HOLD_REG BIT(29)
#define SDMMC_CMD_CCS_EXP BIT(23)
#define SDMMC_CMD_CEATA_RD BIT(22)
#define SDMMC_CMD_UPD_CLK BIT(21)
@@ -142,6 +144,17 @@
/* Version ID register define */
#define SDMMC_GET_VERID(x) ((x) & 0xFFFF)

+#define DW_MCI_DEF_SDR_TIMING 0x03030002
+#define DW_MCI_DEF_DDR_TIMING 0x03020001
+#define SDMMC_CLKSEL_CCLK_SAMPLE(x) (((x) & 3) << 0)
+#define SDMMC_CLKSEL_CCLK_DRIVE(x) (((x) & 3) << 16)
+#define SDMMC_CLKSEL_CCLK_DIVIDER(x) (((x) & 3) << 24)
+#define SDMMC_CLKSEL_TIMING(x, y, z) (SDMMC_CLKSEL_CCLK_SAMPLE(x) | \
+ SDMMC_CLKSEL_CCLK_DRIVE(y) | \
+ SDMMC_CLKSEL_CCLK_DIVIDER(z))
+#define SDMMC_CLKSEL_GET_DIVRATIO(x) ((((x) >> 24) & 0x7) + 1)
+#define SDMMC_CLKSEL_GET_SELCLK_DRV(x) (((x) >> 16) & 0x7)
+
/* Register access macros */
#define mci_readl(dev, reg) \
__raw_readl((dev)->regs + SDMMC_##reg)
diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
index 71d2b56..6e6d036 100644
--- a/include/linux/mmc/dw_mmc.h
+++ b/include/linux/mmc/dw_mmc.h
@@ -82,6 +82,8 @@ struct mmc_data;
* @biu_clk: Pointer to bus interface unit clock instance.
* @ciu_clk: Pointer to card interface unit clock instance.
* @slot: Slots sharing this MMC controller.
+ * @sdr_timing: Clock phase shifting for driving and sampling in sdr mode
+ * @ddr_timing: Clock phase shifting for driving and sampling in ddr mode
* @fifo_depth: depth of FIFO.
* @data_shift: log2 of FIFO item size.
* @part_buf_start: Start index in part_buf.
@@ -166,6 +168,10 @@ struct dw_mci {
struct clk *ciu_clk;
struct dw_mci_slot *slot[MAX_MCI_SLOTS];

+ /* Phase Shift Value (for exynos5250 variant) */
+ u32 sdr_timing;
+ u32 ddr_timing;
+
/* FIFO push and pull */
int fifo_depth;
int data_shift;
--
1.7.5.4

2012-05-02 05:14:18

by Thomas Abraham

[permalink] [raw]
Subject: [PATCH 2/7] mmc: dw_mmc: add quirk to indicate missing write protect line

If the write protect pad of the controller is not connected to the write
protect pin of the slot, the driver should be notified of this condition
so that incorrect check for write protection by reading the WRTORT
register can avoided. The get_ro platform callback can be used for in
such cases, but with device tree support enabled, such platform callbacks
cannot be supported.

Add a new quirk for notifying the driver about the missing write protect
line so the driver can assume that the card write protection is disabled.

Signed-off-by: Thomas Abraham <[email protected]>
---
drivers/mmc/host/dw_mmc.c | 4 +++-
include/linux/mmc/dw_mmc.h | 3 ++-
2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 036846f..38cb7f8 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -812,7 +812,9 @@ static int dw_mci_get_ro(struct mmc_host *mmc)
struct dw_mci_board *brd = slot->host->pdata;

/* Use platform get_ro function, else try on board write protect */
- if (brd->get_ro)
+ if (brd->quirks & DW_MCI_QUIRK_NO_WRITE_PROTECT)
+ read_only = 0;
+ else if (brd->get_ro)
read_only = brd->get_ro(slot->id);
else
read_only =
diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
index fa9a139..fb51a5f 100644
--- a/include/linux/mmc/dw_mmc.h
+++ b/include/linux/mmc/dw_mmc.h
@@ -205,7 +205,8 @@ struct dw_mci_dma_ops {
#define DW_MCI_QUIRK_HIGHSPEED BIT(2)
/* Unreliable card detection */
#define DW_MCI_QUIRK_BROKEN_CARD_DETECTION BIT(3)
-
+/* Write Protect detection not available */
+#define DW_MCI_QUIRK_NO_WRITE_PROTECT BIT(4)

struct dma_pdata;

--
1.7.5.4

2012-05-02 06:55:56

by Kyungmin Park

[permalink] [raw]
Subject: Re: [PATCH 3/7] mmc: dw_mmc: add device tree support

Hi,

On 5/2/12, Thomas Abraham <[email protected]> wrote:
> Add device tree based discovery support.
>
> Signed-off-by: Thomas Abraham <[email protected]>
> ---
> .../devicetree/bindings/mmc/synposis-dw-mshc.txt | 85 +++++++++
> drivers/mmc/host/dw_mmc-pltfm.c | 24 +++
> drivers/mmc/host/dw_mmc.c | 181
> +++++++++++++++++++-
> drivers/mmc/host/dw_mmc.h | 10 +
> include/linux/mmc/dw_mmc.h | 2 +
> 5 files changed, 296 insertions(+), 6 deletions(-)
> create mode 100644
> Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
>
> diff --git a/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
> b/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
> new file mode 100644
> index 0000000..c1ed70e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
> @@ -0,0 +1,85 @@
> +* Synopsis Designware Mobile Storage Host Controller
> +
> +The Synopsis designware mobile storage host controller is used to
> interface
> +a SoC with storage medium such as eMMC or SD/MMC cards.
> +
> +Required Properties:
> +
> +* compatible: should be one of the following
> + - synopsis,dw-mshc: for controllers compliant with synopsis dw-mshc.
I googled the "Synopsis Designware Mobile Storage Host Controller" and
Synopsis released it as this name. but still I like the 'dw-mmc'
instead of'dw-mshc'.
> +
> +* reg: physical base address of the dw-mshc controller and size of its
> memory
> + region.
> +
> +* interrupts: interrupt specifier for the controller. The format and value
> of
> + the interrupt specifier depends on the interrupt parent for the
> controller.
> +
> +# Slots: The slot specific information are contained within child-nodes
> with
> + each child-node representing a supported slot. There should be atleast
> one
> + child node representing a card slot. The name of the slot child node
> should
> + be 'slot{n}' where n is the unique number of the slot connnected to the
> + controller. The following are optional properties which can be included
> in
> + the slot child node.
> +
> + * bus-width: specifies the width of the data bus connected from the
> + controller to the card slot. The value should be 1, 4 or 8. In case
> + this property is not specified, a default value of 1 is assumed for
> + this property.
> +
> + * cd-gpios: specifies the card detect gpio line. The format of the
> + gpio specifier depends on the gpio controller.
> +
> + * wp-gpios: specifies the write protect gpio line. The format of the
> + gpio specifier depends on the gpio controller.
> +
> + * gpios: specifies a list of gpios used for command, clock and data
> + bus. The first gpio is the command line and the second gpio is the
> + clock line. The rest of the gpios (depending on the bus-width
> + property) are the data lines in no particular order. The format of
> + the gpio specifier depends on the gpio controller.
> +
> +Optional properties:
> +
> +* fifo-depth: The maximum size of the tx/rx fifo's. If this property is
> not
> + specified, the default value of the fifo size is determined from the
> + controller registers.
> +
> +* card-detect-delay: Delay in milli-seconds before detecting card after
> card
> + insert event. The default value is 0.
> +
> +* supports-highspeed: Enables support for high speed cards (upto 50MHz)
> +
> +* card-detection-broken: The card detection functionality is not available
> on
> + any of the slots.
> +
> +* no-write-protect: The write protect pad of the controller is not
> connected
> + to the write protect pin on the slot.
> +
> +Example:
> +
> + The MSHC controller node can be split into two portions, SoC specific
> and
> + board specific portions as listed below.
> +
> + dwmmc0@12200000 {
> + compatible = "synopsis,dw-mshc";
> + reg = <0x12200000 0x1000>;
> + interrupts = <0 75 0>;
> + };
> +
> + dwmmc0@12200000 {
> + supports-highspeed;
> + card-detection-broken;
> + no-write-protect;
> + fifo-depth = <0x80>;
> + card-detect-delay = <200>;
> +
> + slot0 {
> + bus-width = <8>;
> + cd-gpios = <&gpc0 2 2 3 3>;
> + gpios = <&gpc0 0 2 0 3>, <&gpc0 1 2 0 3>,
> + <&gpc1 0 2 3 3>, <&gpc1 1 2 3 3>,
> + <&gpc1 2 2 3 3>, <&gpc1 3 2 3 3>,
> + <&gpc0 3 2 3 3>, <&gpc0 4 2 3 3>,
> + <&gpc0 5 2 3 3>, <&gpc0 6 2 3 3>;
> + };
> + };
> diff --git a/drivers/mmc/host/dw_mmc-pltfm.c
> b/drivers/mmc/host/dw_mmc-pltfm.c
> index 92ec3eb..2b2c9bd 100644
> --- a/drivers/mmc/host/dw_mmc-pltfm.c
> +++ b/drivers/mmc/host/dw_mmc-pltfm.c
> @@ -19,8 +19,24 @@
> #include <linux/mmc/host.h>
> #include <linux/mmc/mmc.h>
> #include <linux/mmc/dw_mmc.h>
> +#include <linux/of.h>
> #include "dw_mmc.h"
>
> +#ifdef CONFIG_OF
> +static struct dw_mci_drv_data synopsis_drv_data = {
> + .ctrl_type = DW_MCI_TYPE_SYNOPSIS,
> +};
> +
> +static const struct of_device_id dw_mci_pltfm_match[] = {
> + { .compatible = "synopsis,dw-mshc",
> + .data = (void *)&synopsis_drv_data, },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, dw_mci_pltfm_match);
> +#else
> +static const struct of_device_id dw_mci_pltfm_match[];
> +#endif
> +
> static int dw_mci_pltfm_probe(struct platform_device *pdev)
> {
> struct dw_mci *host;
> @@ -51,6 +67,13 @@ static int dw_mci_pltfm_probe(struct platform_device
> *pdev)
> if (!host->regs)
> goto err_free;
> platform_set_drvdata(pdev, host);
> +
> + if (pdev->dev.of_node) {
> + const struct of_device_id *match;
> + match = of_match_node(dw_mci_pltfm_match, pdev->dev.of_node);
> + host->drv_data = match->data;
> + }
> +
> ret = dw_mci_probe(host);
> if (ret)
> goto err_out;
> @@ -111,6 +134,7 @@ static struct platform_driver dw_mci_pltfm_driver = {
> .remove = __exit_p(dw_mci_pltfm_remove),
> .driver = {
> .name = "dw_mmc",
> + .of_match_table = of_match_ptr(dw_mci_pltfm_match),
> .pm = &dw_mci_pltfm_pmops,
> },
> };
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 38cb7f8..bcf66d7 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -33,9 +33,13 @@
> #include <linux/bitops.h>
> #include <linux/regulator/consumer.h>
> #include <linux/workqueue.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
>
> #include "dw_mmc.h"
>
> +#define NUM_PINS(x) (x + 2)
> +
> /* Common flag combinations */
> #define DW_MCI_DATA_ERROR_FLAGS (SDMMC_INT_DTO | SDMMC_INT_DCRC | \
> SDMMC_INT_HTO | SDMMC_INT_SBE | \
> @@ -86,6 +90,8 @@ struct idmac_desc {
> struct dw_mci_slot {
> struct mmc_host *mmc;
> struct dw_mci *host;
> + int wp_gpio;
> + int cd_gpio;
>
> u32 ctype;
>
> @@ -816,6 +822,8 @@ static int dw_mci_get_ro(struct mmc_host *mmc)
> read_only = 0;
> else if (brd->get_ro)
> read_only = brd->get_ro(slot->id);
> + else if (gpio_is_valid(slot->wp_gpio))
> + read_only = gpio_get_value(slot->wp_gpio);
> else
> read_only =
> mci_readl(slot->host, WRTPRT) & (1 << slot->id) ? 1 : 0;
> @@ -837,6 +845,8 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
> present = 1;
> else if (brd->get_cd)
> present = !brd->get_cd(slot->id);
> + else if (gpio_is_valid(slot->cd_gpio))
> + present = gpio_get_value(slot->cd_gpio);
> else
> present = (mci_readl(slot->host, CDETECT) & (1 << slot->id))
> == 0 ? 1 : 0;
> @@ -1747,6 +1757,96 @@ static void dw_mci_work_routine_card(struct
> work_struct *work)
> }
> }
>
> +#ifdef CONFIG_OF
> +static struct device_node *dw_mci_of_find_slot_node(struct device *dev, u8
> slot)
> +{
> + struct device_node *np;
> + char name[7];
> +
> + if (!dev || !dev->of_node)
> + return NULL;
> +
> + for_each_child_of_node(dev->of_node, np) {
> + sprintf(name, "slot%d", slot);
> + if (!strcmp(name, np->name))
> + return np;
> + }
> + return NULL;
> +}
> +
> +static u32 dw_mci_of_get_bus_wd(struct device *dev, u8 slot)
> +{
> + struct device_node *np = dw_mci_of_find_slot_node(dev, slot);
> + u32 bus_wd = 1;
> +
> + if (!np)
> + return 1;
> +
> + if (of_property_read_u32(np, "bus-width", &bus_wd))
> + dev_err(dev, "bus-width property not found, assuming width"
> + " as 1\n");
> + return bus_wd;
> +}
> +
> +static int dw_mci_of_setup_bus(struct dw_mci *host, u8 slot, u32 bus_wd)
> +{
> + struct device_node *np = dw_mci_of_find_slot_node(&host->dev, slot);
> + int idx, gpio, ret;
> +
> + for (idx = 0; idx < NUM_PINS(bus_wd); idx++) {
> + gpio = of_get_gpio(np, idx);
> + if (!gpio_is_valid(gpio)) {
> + dev_err(&host->dev, "invalid gpio: %d\n", gpio);
> + return -EINVAL;
> + }
> +
> + ret = devm_gpio_request(&host->dev, gpio, "dw-mci-bus");
> + if (ret) {
> + dev_err(&host->dev, "gpio [%d] request failed\n", gpio);
> + return -EBUSY;
> + }
> + }
> +
> + host->slot[slot]->wp_gpio = -1;
> + gpio = of_get_named_gpio(np, "wp_gpios", 0);
> + if (!gpio_is_valid(gpio)) {
> + dev_info(&host->dev, "wp gpio not available");
> + } else {
> + ret = devm_gpio_request(&host->dev, gpio, "dw-mci-wp");
> + if (ret)
> + dev_info(&host->dev, "gpio [%d] request failed\n",
> + gpio);
> + else
> + host->slot[slot]->wp_gpio = gpio;
> + }
> +
> + host->slot[slot]->cd_gpio = -1;
> + gpio = of_get_named_gpio(np, "cd-gpios", 0);
> + if (!gpio_is_valid(gpio)) {
> + dev_info(&host->dev, "cd gpio not available");
> + } else {
> + ret = devm_gpio_request(&host->dev, gpio, "dw-mci-cd");
> + if (ret)
> + dev_err(&host->dev, "gpio [%d] request failed\n", gpio);
> + else
> + host->slot[slot]->cd_gpio = gpio;
> + }
> +
> + return 0;
> +}
> +
> +#else /* CONFIG_OF */
> +static u32 dw_mci_of_get_bus_wd(struct device *dev, u8 slot)
> +{
> + return 1;
> +}
> +
> +static int dw_mci_of_setup_bus(struct dw_mci *host, u8 slot, u32 bus_wd)
> +{
> + return -EINVAL;
> +}
> +#endif /* CONFIG_OF */
> +
> static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id)
> {
> struct mmc_host *mmc;
> @@ -1760,6 +1860,7 @@ static int __init dw_mci_init_slot(struct dw_mci
> *host, unsigned int id)
> slot->id = id;
> slot->mmc = mmc;
> slot->host = host;
> + host->slot[id] = slot;
>
> mmc->ops = &dw_mci_ops;
> mmc->f_min = DIV_ROUND_UP(host->bus_hz, 510);
> @@ -1780,12 +1881,21 @@ static int __init dw_mci_init_slot(struct dw_mci
> *host, unsigned int id)
> if (host->pdata->caps)
> mmc->caps = host->pdata->caps;
>
> + mmc->caps |= host->drv_data->caps;
> +
> if (host->pdata->caps2)
> mmc->caps2 = host->pdata->caps2;
>
> - if (host->pdata->get_bus_wd)
> + if (host->pdata->get_bus_wd) {
> if (host->pdata->get_bus_wd(slot->id) >= 4)
> mmc->caps |= MMC_CAP_4_BIT_DATA;
> + } else if (host->dev.of_node) {
> + unsigned int bus_width;
> + bus_width = dw_mci_of_get_bus_wd(&host->dev, slot->id);
> + if (bus_width >= 4)
> + mmc->caps |= MMC_CAP_4_BIT_DATA;
In case of bus_width has 8; then how to handle it? maybe it's missing one.
> + dw_mci_of_setup_bus(host, slot->id, bus_width);
> + }
>
> if (host->pdata->quirks & DW_MCI_QUIRK_HIGHSPEED)
> mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
> @@ -1830,7 +1940,6 @@ static int __init dw_mci_init_slot(struct dw_mci
> *host, unsigned int id)
> else
> clear_bit(DW_MMC_CARD_PRESENT, &slot->flags);
>
> - host->slot[id] = slot;
> mmc_add_host(mmc);
>
> #if defined(CONFIG_DEBUG_FS)
> @@ -1923,15 +2032,75 @@ static bool mci_wait_reset(struct device *dev,
> struct dw_mci *host)
> return false;
> }
>
> +#ifdef CONFIG_OF
> +static struct dw_mci_of_quirks {
> + char *quirk;
> + int id;
> +} of_quriks[] = {
> + {
> + .quirk = "supports-highspeed",
> + .id = DW_MCI_QUIRK_HIGHSPEED,
> + }, {
> + .quirk = "card-detection-broken",
> + .id = DW_MCI_QUIRK_BROKEN_CARD_DETECTION,
> + }, {
> + .quirk = "no-write-protect",
> + .id = DW_MCI_QUIRK_NO_WRITE_PROTECT,
> + },
> + { }
> +};
> +
> +static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
> +{
> + struct dw_mci_board *pdata;
> + struct device *dev = &host->dev;
> + struct device_node *np = dev->of_node, *slot;
> + u32 timing[3];
> + int idx, cnt;
> +
> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata) {
> + dev_err(dev, "could not allocate memory for pdata\n");
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + /* find out number of slots supported */
> + for_each_child_of_node(np, slot)
> + pdata->num_slots++;
> +
> + /* get quirks */
> + cnt = sizeof(of_quriks) / sizeof(struct dw_mci_of_quirks);
> + for (idx = 0; idx < cnt; idx++)
> + if (of_get_property(np, of_quriks[idx].quirk, NULL))
> + pdata->quirks |= of_quriks[idx].id;
> +
> + if (of_property_read_u32(np, "fifo-depth", &pdata->fifo_depth))
> + dev_info(dev, "fifo-depth property not found, using "
> + "value of FIFOTH register as default\n");
> +
> + of_property_read_u32(np, "card-detect-delay", &pdata->detect_delay_ms);
> +
> + return pdata;
> +}
> +
> +#else /* CONFIG_OF */
> +static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
> +{
> + return ERR_PTR(-EINVAL);
> +}
> +#endif /* CONFIG_OF */
> +
> int dw_mci_probe(struct dw_mci *host)
> {
> int width, i, ret = 0;
> u32 fifo_size;
>
> - if (!host->pdata || !host->pdata->init) {
> - dev_err(&host->dev,
> - "Platform data must supply init function\n");
> - return -ENODEV;
> + if (!host->pdata) {
> + host->pdata = dw_mci_parse_dt(host);
> + if (IS_ERR(host->pdata)) {
> + dev_err(&host->dev, "platform data not available\n");
> + return -EINVAL;
> + }
> }
>
> if (!host->pdata->select_slot && host->pdata->num_slots > 1) {
> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
> index 15c27e1..8b8862b 100644
> --- a/drivers/mmc/host/dw_mmc.h
> +++ b/drivers/mmc/host/dw_mmc.h
> @@ -182,4 +182,14 @@ extern int dw_mci_suspend(struct dw_mci *host);
> extern int dw_mci_resume(struct dw_mci *host);
> #endif
>
> +/* Variations in the dw_mci controller */
> +#define DW_MCI_TYPE_SYNOPSIS 0
> +#define DW_MCI_TYPE_EXYNOS5250 1 /* Samsung Exynos5250 Extensions */
Um. it's not good idea to add specific SOC version. And as you know,
exynos4 series has this controller.
> +
> +/* dw_mci platform driver data */
> +struct dw_mci_drv_data {
> + unsigned long ctrl_type;
> + unsigned long caps;
> +};
> +
> #endif /* _DW_MMC_H_ */
> diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
> index fb51a5f..71d2b56 100644
> --- a/include/linux/mmc/dw_mmc.h
> +++ b/include/linux/mmc/dw_mmc.h
> @@ -78,6 +78,7 @@ struct mmc_data;
> * @data_offset: Set the offset of DATA register according to VERID.
> * @dev: Device associated with the MMC controller.
> * @pdata: Platform data associated with the MMC controller.
> + * @drv_data: Driver specific data for identified variant of the
> controller
> * @biu_clk: Pointer to bus interface unit clock instance.
> * @ciu_clk: Pointer to card interface unit clock instance.
> * @slot: Slots sharing this MMC controller.
> @@ -160,6 +161,7 @@ struct dw_mci {
> u16 data_offset;
> struct device dev;
> struct dw_mci_board *pdata;
> + struct dw_mci_drv_data *drv_data;
> struct clk *biu_clk;
> struct clk *ciu_clk;
> struct dw_mci_slot *slot[MAX_MCI_SLOTS];
> --
> 1.7.5.4
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

2012-05-02 07:01:50

by Kyungmin Park

[permalink] [raw]
Subject: Re: [PATCH 4/7] mmc: dw_mmc: add samsung exynos5250 specific extentions

Hi,

On 5/2/12, Thomas Abraham <[email protected]> wrote:
> The instantiation of the Synopsis Designware controller on Exynos5250
> include extension for SDR and DDR specific tx/rx phase shift timing
> and CIU internal divider. In addition to that, the option to skip the
> command hold stage is also introduced. Add support for these Exynos5250
> specfic extenstions.
>
> Signed-off-by: Abhilash Kesavan <[email protected]>
> Signed-off-by: Thomas Abraham <[email protected]>
> ---
> .../devicetree/bindings/mmc/synposis-dw-mshc.txt | 33
> +++++++++++++++++++-
> drivers/mmc/host/dw_mmc-pltfm.c | 8 +++++
> drivers/mmc/host/dw_mmc.c | 32
> ++++++++++++++++++-
> drivers/mmc/host/dw_mmc.h | 13 ++++++++
> include/linux/mmc/dw_mmc.h | 6 +++
> 5 files changed, 89 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
> b/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
> index c1ed70e..465fc31 100644
> --- a/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
> +++ b/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
> @@ -7,6 +7,8 @@ Required Properties:
>
> * compatible: should be one of the following
> - synopsis,dw-mshc: for controllers compliant with synopsis dw-mshc.
> + - synopsis,dw-mshc-exynos5250: for controllers with Samsung
> + Exynos5250 specific extentions.
>
> * reg: physical base address of the dw-mshc controller and size of its
> memory
> region.
> @@ -55,13 +57,40 @@ Optional properties:
> * no-write-protect: The write protect pad of the controller is not
> connected
> to the write protect pin on the slot.
>
> +Samsung Exynos5250 specific properties:
> +
> +* samsung,dw-mshc-sdr-timing: Specifies the value of CUI clock divider,
> CIU
> + clock phase shift value in transmit mode and CIU clock phase shift value
> in
> + receive mode for single data rate mode operation. Refer notes of the
> valid
> + values below.
> +
> +* samsung,dw-mshc-ddr-timing: Specifies the value of CUI clock divider,
> CIU
> + clock phase shift value in transmit mode and CIU clock phase shift value
> in
> + receive mode for double data rate mode operation. Refer notes of the
> valid
> + values below. The order of the cells should be
> +
> + - First Cell: CIU clock divider value.
> + - Second Cell: CIU clock phase shift value for tx mode.
> + - Third Cell: CIU clock phase shift value for rx mode.
> +
> + Valid values for SDR and DDR CIU clock timing:
> +
> + - valid values for CIU clock divider, tx phase shift and rx phase
> shift
> + is 0 to 7.
> +
> + - When CIU clock divider value is set to 3, all possible 8 phase shift
> + values can be used.
> +
> + - If CIU clock divider value is 0 (that is divide by 1), both tx and
> rx
> + phase shift clocks should be 0.
> +
> Example:
>
> The MSHC controller node can be split into two portions, SoC specific
> and
> board specific portions as listed below.
>
> dwmmc0@12200000 {
> - compatible = "synopsis,dw-mshc";
> + compatible = "synopsis,dw-mshc-exynos5250";
> reg = <0x12200000 0x1000>;
> interrupts = <0 75 0>;
> };
> @@ -72,6 +101,8 @@ Example:
> no-write-protect;
> fifo-depth = <0x80>;
> card-detect-delay = <200>;
> + samsung,dw-mshc-sdr-timing = <2 3 3>;
> + samsung,dw-mshc-ddr-timing = <1 2 3>;
>
> slot0 {
> bus-width = <8>;
> diff --git a/drivers/mmc/host/dw_mmc-pltfm.c
> b/drivers/mmc/host/dw_mmc-pltfm.c
> index 2b2c9bd..35056fd 100644
> --- a/drivers/mmc/host/dw_mmc-pltfm.c
> +++ b/drivers/mmc/host/dw_mmc-pltfm.c
> @@ -27,9 +27,17 @@ static struct dw_mci_drv_data synopsis_drv_data = {
> .ctrl_type = DW_MCI_TYPE_SYNOPSIS,
> };
>
> +static struct dw_mci_drv_data exynos5250_drv_data = {
> + .ctrl_type = DW_MCI_TYPE_EXYNOS5250,
> + .caps = MMC_CAP_UHS_DDR50 | MMC_CAP_1_8V_DDR |
> + MMC_CAP_8_BIT_DATA | MMC_CAP_CMD23,
These caps should be board specific. So it's not proper place. Esp.,
MMC_CAP_8_BIT_DATA.
> +};
> +
> static const struct of_device_id dw_mci_pltfm_match[] = {
> { .compatible = "synopsis,dw-mshc",
> .data = (void *)&synopsis_drv_data, },
> + { .compatible = "synopsis,dw-mshc-exynos5250",
> + .data = (void *)&exynos5250_drv_data, },
> {},
> };
> MODULE_DEVICE_TABLE(of, dw_mci_pltfm_match);
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index bcf66d7..9174a69 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -236,6 +236,7 @@ static void dw_mci_set_timeout(struct dw_mci *host)
> static u32 dw_mci_prepare_command(struct mmc_host *mmc, struct mmc_command
> *cmd)
> {
> struct mmc_data *data;
> + struct dw_mci_slot *slot = mmc_priv(mmc);
> u32 cmdr;
> cmd->error = -EINPROGRESS;
>
> @@ -265,6 +266,10 @@ static u32 dw_mci_prepare_command(struct mmc_host *mmc,
> struct mmc_command *cmd)
> cmdr |= SDMMC_CMD_DAT_WR;
> }
>
> + if (slot->host->drv_data->ctrl_type == DW_MCI_TYPE_EXYNOS5250)
> + if (SDMMC_CLKSEL_GET_SELCLK_DRV(mci_readl(slot->host, CLKSEL)))
> + cmdr |= SDMMC_USE_HOLD_REG;
Some other board, custom SOC also can use this HOLD register. So it's
not EXYNOS5250 specific one. I think we introduce the more generic
quirks for this instead of SOC specific.
> +
> return cmdr;
> }
>
> @@ -787,10 +792,19 @@ static void dw_mci_set_ios(struct mmc_host *mmc,
> struct mmc_ios *ios)
> regs = mci_readl(slot->host, UHS_REG);
>
> /* DDR mode set */
> - if (ios->timing == MMC_TIMING_UHS_DDR50)
> + if (ios->timing == MMC_TIMING_UHS_DDR50) {
> regs |= (0x1 << slot->id) << 16;
> - else
> + mci_writel(slot->host, CLKSEL, slot->host->ddr_timing);
As you wrote, does CLKSEL is some SOC specific registers?
> + } else {
> regs &= ~(0x1 << slot->id) << 16;
> + mci_writel(slot->host, CLKSEL, slot->host->sdr_timing);
> + }
> +
> + if (slot->host->drv_data->ctrl_type == DW_MCI_TYPE_EXYNOS5250) {
> + slot->host->bus_hz = clk_get_rate(slot->host->ciu_clk);
> + slot->host->bus_hz /= SDMMC_CLKSEL_GET_DIVRATIO(
> + mci_readl(slot->host, CLKSEL));
> + }
>
> mci_writel(slot->host, UHS_REG, regs);
>
> @@ -2074,6 +2088,20 @@ static struct dw_mci_board *dw_mci_parse_dt(struct
> dw_mci *host)
> if (of_get_property(np, of_quriks[idx].quirk, NULL))
> pdata->quirks |= of_quriks[idx].id;
>
> + if (of_property_read_u32_array(dev->of_node,
> + "samsung,dw-mshc-sdr-timing", timing, 3))
> + host->sdr_timing = DW_MCI_DEF_SDR_TIMING;
> + else
> + host->sdr_timing = SDMMC_CLKSEL_TIMING(timing[0],
> + timing[1], timing[2]);
> +
> + if (of_property_read_u32_array(dev->of_node,
> + "samsung,dw-mshc-ddr-timing", timing, 3))
> + host->ddr_timing = DW_MCI_DEF_DDR_TIMING;
> + else
> + host->ddr_timing = SDMMC_CLKSEL_TIMING(timing[0],
> + timing[1], timing[2]);
> +
> if (of_property_read_u32(np, "fifo-depth", &pdata->fifo_depth))
> dev_info(dev, "fifo-depth property not found, using "
> "value of FIFOTH register as default\n");
> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
> index 8b8862b..4b7e42b 100644
> --- a/drivers/mmc/host/dw_mmc.h
> +++ b/drivers/mmc/host/dw_mmc.h
> @@ -53,6 +53,7 @@
> #define SDMMC_IDINTEN 0x090
> #define SDMMC_DSCADDR 0x094
> #define SDMMC_BUFADDR 0x098
> +#define SDMMC_CLKSEL 0x09C /* specific to Samsung Exynos5250 */
Another Samsung Custom SOC also used it.
> #define SDMMC_DATA(x) (x)
>
> /*
> @@ -111,6 +112,7 @@
> #define SDMMC_INT_ERROR 0xbfc2
> /* Command register defines */
> #define SDMMC_CMD_START BIT(31)
> +#define SDMMC_USE_HOLD_REG BIT(29)
> #define SDMMC_CMD_CCS_EXP BIT(23)
> #define SDMMC_CMD_CEATA_RD BIT(22)
> #define SDMMC_CMD_UPD_CLK BIT(21)
> @@ -142,6 +144,17 @@
> /* Version ID register define */
> #define SDMMC_GET_VERID(x) ((x) & 0xFFFF)
>
> +#define DW_MCI_DEF_SDR_TIMING 0x03030002
> +#define DW_MCI_DEF_DDR_TIMING 0x03020001
> +#define SDMMC_CLKSEL_CCLK_SAMPLE(x) (((x) & 3) << 0)
> +#define SDMMC_CLKSEL_CCLK_DRIVE(x) (((x) & 3) << 16)
> +#define SDMMC_CLKSEL_CCLK_DIVIDER(x) (((x) & 3) << 24)
> +#define SDMMC_CLKSEL_TIMING(x, y, z) (SDMMC_CLKSEL_CCLK_SAMPLE(x) | \
> + SDMMC_CLKSEL_CCLK_DRIVE(y) | \
> + SDMMC_CLKSEL_CCLK_DIVIDER(z))
> +#define SDMMC_CLKSEL_GET_DIVRATIO(x) ((((x) >> 24) & 0x7) + 1)
> +#define SDMMC_CLKSEL_GET_SELCLK_DRV(x) (((x) >> 16) & 0x7)
> +
> /* Register access macros */
> #define mci_readl(dev, reg) \
> __raw_readl((dev)->regs + SDMMC_##reg)
> diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
> index 71d2b56..6e6d036 100644
> --- a/include/linux/mmc/dw_mmc.h
> +++ b/include/linux/mmc/dw_mmc.h
> @@ -82,6 +82,8 @@ struct mmc_data;
> * @biu_clk: Pointer to bus interface unit clock instance.
> * @ciu_clk: Pointer to card interface unit clock instance.
> * @slot: Slots sharing this MMC controller.
> + * @sdr_timing: Clock phase shifting for driving and sampling in sdr mode
> + * @ddr_timing: Clock phase shifting for driving and sampling in ddr mode
> * @fifo_depth: depth of FIFO.
> * @data_shift: log2 of FIFO item size.
> * @part_buf_start: Start index in part_buf.
> @@ -166,6 +168,10 @@ struct dw_mci {
> struct clk *ciu_clk;
> struct dw_mci_slot *slot[MAX_MCI_SLOTS];
>
> + /* Phase Shift Value (for exynos5250 variant) */
> + u32 sdr_timing;
> + u32 ddr_timing;
> +
> /* FIFO push and pull */
> int fifo_depth;
> int data_shift;
> --
> 1.7.5.4
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

2012-05-02 07:04:59

by Kyungmin Park

[permalink] [raw]
Subject: Re: [PATCH 5/7] ARM: Samsung: Add support for MSHC controller clocks

Hi Thomas,

I suggest to split the patches into mmc part and samsung specific
part. As you know previous time there's mismatch between mmc and
samsung. So split the patches and send it separately to avoid merge
conflict and mismatch.

I think regardless mmc changes, it can be merged into samsung tree directly.

Thank you,
Kyungmin Park

On 5/2/12, Thomas Abraham <[email protected]> wrote:
> Add clock instances for bus interface unit clock and card interface unit
> clock of the all four MSHC controller instances.
>
> Signed-off-by: Abhilash Kesavan <[email protected]>
> Signed-off-by: Thomas Abraham <[email protected]>
> ---
> arch/arm/mach-exynos/clock-exynos5.c | 45
> ++++++++++++----------------------
> 1 files changed, 16 insertions(+), 29 deletions(-)
>
> diff --git a/arch/arm/mach-exynos/clock-exynos5.c
> b/arch/arm/mach-exynos/clock-exynos5.c
> index 7c0f810..4e17131 100644
> --- a/arch/arm/mach-exynos/clock-exynos5.c
> +++ b/arch/arm/mach-exynos/clock-exynos5.c
> @@ -524,35 +524,30 @@ static struct clk exynos5_init_clocks_off[] = {
> .enable = exynos5_clk_ip_peris_ctrl,
> .ctrlbit = (1 << 19),
> }, {
> - .name = "hsmmc",
> - .devname = "exynos4-sdhci.0",
> + .name = "biu",
> + .devname = "dw_mmc.0",
> .parent = &exynos5_clk_aclk_200.clk,
> .enable = exynos5_clk_ip_fsys_ctrl,
> .ctrlbit = (1 << 12),
> }, {
> - .name = "hsmmc",
> - .devname = "exynos4-sdhci.1",
> + .name = "biu",
> + .devname = "dw_mmc.1",
> .parent = &exynos5_clk_aclk_200.clk,
> .enable = exynos5_clk_ip_fsys_ctrl,
> .ctrlbit = (1 << 13),
> }, {
> - .name = "hsmmc",
> - .devname = "exynos4-sdhci.2",
> + .name = "biu",
> + .devname = "dw_mmc.2",
> .parent = &exynos5_clk_aclk_200.clk,
> .enable = exynos5_clk_ip_fsys_ctrl,
> .ctrlbit = (1 << 14),
> }, {
> - .name = "hsmmc",
> - .devname = "exynos4-sdhci.3",
> + .name = "biu",
> + .devname = "dw_mmc.3",
> .parent = &exynos5_clk_aclk_200.clk,
> .enable = exynos5_clk_ip_fsys_ctrl,
> .ctrlbit = (1 << 15),
> }, {
> - .name = "dwmci",
> - .parent = &exynos5_clk_aclk_200.clk,
> - .enable = exynos5_clk_ip_fsys_ctrl,
> - .ctrlbit = (1 << 16),
> - }, {
> .name = "sata",
> .devname = "ahci",
> .enable = exynos5_clk_ip_fsys_ctrl,
> @@ -882,8 +877,8 @@ static struct clksrc_clk exynos5_clk_sclk_uart3 = {
>
> static struct clksrc_clk exynos5_clk_sclk_mmc0 = {
> .clk = {
> - .name = "sclk_mmc",
> - .devname = "exynos4-sdhci.0",
> + .name = "ciu",
> + .devname = "dw_mmc.0",
> .parent = &exynos5_clk_dout_mmc0.clk,
> .enable = exynos5_clksrc_mask_fsys_ctrl,
> .ctrlbit = (1 << 0),
> @@ -893,8 +888,8 @@ static struct clksrc_clk exynos5_clk_sclk_mmc0 = {
>
> static struct clksrc_clk exynos5_clk_sclk_mmc1 = {
> .clk = {
> - .name = "sclk_mmc",
> - .devname = "exynos4-sdhci.1",
> + .name = "ciu",
> + .devname = "dw_mmc.1",
> .parent = &exynos5_clk_dout_mmc1.clk,
> .enable = exynos5_clksrc_mask_fsys_ctrl,
> .ctrlbit = (1 << 4),
> @@ -904,8 +899,8 @@ static struct clksrc_clk exynos5_clk_sclk_mmc1 = {
>
> static struct clksrc_clk exynos5_clk_sclk_mmc2 = {
> .clk = {
> - .name = "sclk_mmc",
> - .devname = "exynos4-sdhci.2",
> + .name = "ciu",
> + .devname = "dw_mmc.2",
> .parent = &exynos5_clk_dout_mmc2.clk,
> .enable = exynos5_clksrc_mask_fsys_ctrl,
> .ctrlbit = (1 << 8),
> @@ -915,8 +910,8 @@ static struct clksrc_clk exynos5_clk_sclk_mmc2 = {
>
> static struct clksrc_clk exynos5_clk_sclk_mmc3 = {
> .clk = {
> - .name = "sclk_mmc",
> - .devname = "exynos4-sdhci.3",
> + .name = "ciu",
> + .devname = "dw_mmc.3",
> .parent = &exynos5_clk_dout_mmc3.clk,
> .enable = exynos5_clksrc_mask_fsys_ctrl,
> .ctrlbit = (1 << 12),
> @@ -927,14 +922,6 @@ static struct clksrc_clk exynos5_clk_sclk_mmc3 = {
> static struct clksrc_clk exynos5_clksrcs[] = {
> {
> .clk = {
> - .name = "sclk_dwmci",
> - .parent = &exynos5_clk_dout_mmc4.clk,
> - .enable = exynos5_clksrc_mask_fsys_ctrl,
> - .ctrlbit = (1 << 16),
> - },
> - .reg_div = { .reg = EXYNOS5_CLKDIV_FSYS3, .shift = 8, .size = 8 },
> - }, {
> - .clk = {
> .name = "sclk_fimd",
> .devname = "s3cfb.1",
> .enable = exynos5_clksrc_mask_disp1_0_ctrl,
> --
> 1.7.5.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc"
> in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

2012-05-02 07:49:36

by Jaehoon Chung

[permalink] [raw]
Subject: Re: [PATCH 4/7] mmc: dw_mmc: add samsung exynos5250 specific extentions

On 05/02/2012 04:01 PM, Kyungmin Park wrote:

> Hi,
>
> On 5/2/12, Thomas Abraham <[email protected]> wrote:
>> The instantiation of the Synopsis Designware controller on Exynos5250
>> include extension for SDR and DDR specific tx/rx phase shift timing
>> and CIU internal divider. In addition to that, the option to skip the
>> command hold stage is also introduced. Add support for these Exynos5250
>> specfic extenstions.
>>
>> Signed-off-by: Abhilash Kesavan <[email protected]>
>> Signed-off-by: Thomas Abraham <[email protected]>
>> ---
>> .../devicetree/bindings/mmc/synposis-dw-mshc.txt | 33
>> +++++++++++++++++++-
>> drivers/mmc/host/dw_mmc-pltfm.c | 8 +++++
>> drivers/mmc/host/dw_mmc.c | 32
>> ++++++++++++++++++-
>> drivers/mmc/host/dw_mmc.h | 13 ++++++++
>> include/linux/mmc/dw_mmc.h | 6 +++
>> 5 files changed, 89 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
>> b/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
>> index c1ed70e..465fc31 100644
>> --- a/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
>> +++ b/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
>> @@ -7,6 +7,8 @@ Required Properties:
>>
>> * compatible: should be one of the following
>> - synopsis,dw-mshc: for controllers compliant with synopsis dw-mshc.
>> + - synopsis,dw-mshc-exynos5250: for controllers with Samsung
>> + Exynos5250 specific extentions.
>>
>> * reg: physical base address of the dw-mshc controller and size of its
>> memory
>> region.
>> @@ -55,13 +57,40 @@ Optional properties:
>> * no-write-protect: The write protect pad of the controller is not
>> connected
>> to the write protect pin on the slot.
>>
>> +Samsung Exynos5250 specific properties:
>> +
>> +* samsung,dw-mshc-sdr-timing: Specifies the value of CUI clock divider,
>> CIU
>> + clock phase shift value in transmit mode and CIU clock phase shift value
>> in
>> + receive mode for single data rate mode operation. Refer notes of the
>> valid
>> + values below.
>> +
>> +* samsung,dw-mshc-ddr-timing: Specifies the value of CUI clock divider,
>> CIU
>> + clock phase shift value in transmit mode and CIU clock phase shift value
>> in
>> + receive mode for double data rate mode operation. Refer notes of the
>> valid
>> + values below. The order of the cells should be
>> +
>> + - First Cell: CIU clock divider value.
>> + - Second Cell: CIU clock phase shift value for tx mode.
>> + - Third Cell: CIU clock phase shift value for rx mode.
>> +
>> + Valid values for SDR and DDR CIU clock timing:
>> +
>> + - valid values for CIU clock divider, tx phase shift and rx phase
>> shift
>> + is 0 to 7.
>> +
>> + - When CIU clock divider value is set to 3, all possible 8 phase shift
>> + values can be used.
>> +
>> + - If CIU clock divider value is 0 (that is divide by 1), both tx and
>> rx
>> + phase shift clocks should be 0.
>> +
>> Example:
>>
>> The MSHC controller node can be split into two portions, SoC specific
>> and
>> board specific portions as listed below.
>>
>> dwmmc0@12200000 {
>> - compatible = "synopsis,dw-mshc";
>> + compatible = "synopsis,dw-mshc-exynos5250";
>> reg = <0x12200000 0x1000>;
>> interrupts = <0 75 0>;
>> };
>> @@ -72,6 +101,8 @@ Example:
>> no-write-protect;
>> fifo-depth = <0x80>;
>> card-detect-delay = <200>;
>> + samsung,dw-mshc-sdr-timing = <2 3 3>;
>> + samsung,dw-mshc-ddr-timing = <1 2 3>;
>>
>> slot0 {
>> bus-width = <8>;
>> diff --git a/drivers/mmc/host/dw_mmc-pltfm.c
>> b/drivers/mmc/host/dw_mmc-pltfm.c
>> index 2b2c9bd..35056fd 100644
>> --- a/drivers/mmc/host/dw_mmc-pltfm.c
>> +++ b/drivers/mmc/host/dw_mmc-pltfm.c
>> @@ -27,9 +27,17 @@ static struct dw_mci_drv_data synopsis_drv_data = {
>> .ctrl_type = DW_MCI_TYPE_SYNOPSIS,
>> };
>>
>> +static struct dw_mci_drv_data exynos5250_drv_data = {
>> + .ctrl_type = DW_MCI_TYPE_EXYNOS5250,
>> + .caps = MMC_CAP_UHS_DDR50 | MMC_CAP_1_8V_DDR |
>> + MMC_CAP_8_BIT_DATA | MMC_CAP_CMD23,
> These caps should be board specific. So it's not proper place. Esp.,
> MMC_CAP_8_BIT_DATA.
>> +};
>> +
>> static const struct of_device_id dw_mci_pltfm_match[] = {
>> { .compatible = "synopsis,dw-mshc",
>> .data = (void *)&synopsis_drv_data, },
>> + { .compatible = "synopsis,dw-mshc-exynos5250",
>> + .data = (void *)&exynos5250_drv_data, },
>> {},
>> };
>> MODULE_DEVICE_TABLE(of, dw_mci_pltfm_match);
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index bcf66d7..9174a69 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -236,6 +236,7 @@ static void dw_mci_set_timeout(struct dw_mci *host)
>> static u32 dw_mci_prepare_command(struct mmc_host *mmc, struct mmc_command
>> *cmd)
>> {
>> struct mmc_data *data;
>> + struct dw_mci_slot *slot = mmc_priv(mmc);
>> u32 cmdr;
>> cmd->error = -EINPROGRESS;
>>
>> @@ -265,6 +266,10 @@ static u32 dw_mci_prepare_command(struct mmc_host *mmc,
>> struct mmc_command *cmd)
>> cmdr |= SDMMC_CMD_DAT_WR;
>> }
>>
>> + if (slot->host->drv_data->ctrl_type == DW_MCI_TYPE_EXYNOS5250)
>> + if (SDMMC_CLKSEL_GET_SELCLK_DRV(mci_readl(slot->host, CLKSEL)))
>> + cmdr |= SDMMC_USE_HOLD_REG;
> Some other board, custom SOC also can use this HOLD register. So it's
> not EXYNOS5250 specific one. I think we introduce the more generic
> quirks for this instead of SOC specific.

One more, I think that also need to check the IMPLEMENT_HOLD_REG bit in HCON register.
It has dependency with that.
As Mr.Park is mentioned, this register is clock phasing.
In spec, card is enumerated in SDR12 or SDR25 mode, the application must program the use_hold_reg.

Best Regards,
Jaehoon Chung

>> +
>> return cmdr;
>> }
>>
>> @@ -787,10 +792,19 @@ static void dw_mci_set_ios(struct mmc_host *mmc,
>> struct mmc_ios *ios)
>> regs = mci_readl(slot->host, UHS_REG);
>>
>> /* DDR mode set */
>> - if (ios->timing == MMC_TIMING_UHS_DDR50)
>> + if (ios->timing == MMC_TIMING_UHS_DDR50) {
>> regs |= (0x1 << slot->id) << 16;
>> - else
>> + mci_writel(slot->host, CLKSEL, slot->host->ddr_timing);
> As you wrote, does CLKSEL is some SOC specific registers?
>> + } else {
>> regs &= ~(0x1 << slot->id) << 16;
>> + mci_writel(slot->host, CLKSEL, slot->host->sdr_timing);
>> + }
>> +
>> + if (slot->host->drv_data->ctrl_type == DW_MCI_TYPE_EXYNOS5250) {
>> + slot->host->bus_hz = clk_get_rate(slot->host->ciu_clk);
>> + slot->host->bus_hz /= SDMMC_CLKSEL_GET_DIVRATIO(
>> + mci_readl(slot->host, CLKSEL));
>> + }
>>
>> mci_writel(slot->host, UHS_REG, regs);
>>
>> @@ -2074,6 +2088,20 @@ static struct dw_mci_board *dw_mci_parse_dt(struct
>> dw_mci *host)
>> if (of_get_property(np, of_quriks[idx].quirk, NULL))
>> pdata->quirks |= of_quriks[idx].id;
>>
>> + if (of_property_read_u32_array(dev->of_node,
>> + "samsung,dw-mshc-sdr-timing", timing, 3))
>> + host->sdr_timing = DW_MCI_DEF_SDR_TIMING;
>> + else
>> + host->sdr_timing = SDMMC_CLKSEL_TIMING(timing[0],
>> + timing[1], timing[2]);
>> +
>> + if (of_property_read_u32_array(dev->of_node,
>> + "samsung,dw-mshc-ddr-timing", timing, 3))
>> + host->ddr_timing = DW_MCI_DEF_DDR_TIMING;
>> + else
>> + host->ddr_timing = SDMMC_CLKSEL_TIMING(timing[0],
>> + timing[1], timing[2]);
>> +
>> if (of_property_read_u32(np, "fifo-depth", &pdata->fifo_depth))
>> dev_info(dev, "fifo-depth property not found, using "
>> "value of FIFOTH register as default\n");
>> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
>> index 8b8862b..4b7e42b 100644
>> --- a/drivers/mmc/host/dw_mmc.h
>> +++ b/drivers/mmc/host/dw_mmc.h
>> @@ -53,6 +53,7 @@
>> #define SDMMC_IDINTEN 0x090
>> #define SDMMC_DSCADDR 0x094
>> #define SDMMC_BUFADDR 0x098
>> +#define SDMMC_CLKSEL 0x09C /* specific to Samsung Exynos5250 */
> Another Samsung Custom SOC also used it.
>> #define SDMMC_DATA(x) (x)
>>
>> /*
>> @@ -111,6 +112,7 @@
>> #define SDMMC_INT_ERROR 0xbfc2
>> /* Command register defines */
>> #define SDMMC_CMD_START BIT(31)
>> +#define SDMMC_USE_HOLD_REG BIT(29)
>> #define SDMMC_CMD_CCS_EXP BIT(23)
>> #define SDMMC_CMD_CEATA_RD BIT(22)
>> #define SDMMC_CMD_UPD_CLK BIT(21)
>> @@ -142,6 +144,17 @@
>> /* Version ID register define */
>> #define SDMMC_GET_VERID(x) ((x) & 0xFFFF)
>>
>> +#define DW_MCI_DEF_SDR_TIMING 0x03030002
>> +#define DW_MCI_DEF_DDR_TIMING 0x03020001
>> +#define SDMMC_CLKSEL_CCLK_SAMPLE(x) (((x) & 3) << 0)
>> +#define SDMMC_CLKSEL_CCLK_DRIVE(x) (((x) & 3) << 16)
>> +#define SDMMC_CLKSEL_CCLK_DIVIDER(x) (((x) & 3) << 24)
>> +#define SDMMC_CLKSEL_TIMING(x, y, z) (SDMMC_CLKSEL_CCLK_SAMPLE(x) | \
>> + SDMMC_CLKSEL_CCLK_DRIVE(y) | \
>> + SDMMC_CLKSEL_CCLK_DIVIDER(z))
>> +#define SDMMC_CLKSEL_GET_DIVRATIO(x) ((((x) >> 24) & 0x7) + 1)
>> +#define SDMMC_CLKSEL_GET_SELCLK_DRV(x) (((x) >> 16) & 0x7)
>> +
>> /* Register access macros */
>> #define mci_readl(dev, reg) \
>> __raw_readl((dev)->regs + SDMMC_##reg)
>> diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
>> index 71d2b56..6e6d036 100644
>> --- a/include/linux/mmc/dw_mmc.h
>> +++ b/include/linux/mmc/dw_mmc.h
>> @@ -82,6 +82,8 @@ struct mmc_data;
>> * @biu_clk: Pointer to bus interface unit clock instance.
>> * @ciu_clk: Pointer to card interface unit clock instance.
>> * @slot: Slots sharing this MMC controller.
>> + * @sdr_timing: Clock phase shifting for driving and sampling in sdr mode
>> + * @ddr_timing: Clock phase shifting for driving and sampling in ddr mode
>> * @fifo_depth: depth of FIFO.
>> * @data_shift: log2 of FIFO item size.
>> * @part_buf_start: Start index in part_buf.
>> @@ -166,6 +168,10 @@ struct dw_mci {
>> struct clk *ciu_clk;
>> struct dw_mci_slot *slot[MAX_MCI_SLOTS];
>>
>> + /* Phase Shift Value (for exynos5250 variant) */
>> + u32 sdr_timing;
>> + u32 ddr_timing;
>> +
>> /* FIFO push and pull */
>> int fifo_depth;
>> int data_shift;
>> --
>> 1.7.5.4
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> [email protected]
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

2012-05-02 09:10:27

by Will Newton

[permalink] [raw]
Subject: Re: [PATCH 2/7] mmc: dw_mmc: add quirk to indicate missing write protect line

On Wed, May 2, 2012 at 6:07 AM, Thomas Abraham
<[email protected]> wrote:
> If the write protect pad of the controller is not connected to the write
> protect pin of the slot, the driver should be notified of this condition
> so that incorrect check for write protection by reading the WRTORT
> register can avoided. The get_ro platform callback can be used for in
> such cases, but with device tree support enabled, such platform callbacks
> cannot be supported.
>
> Add a new quirk for notifying the driver about the missing write protect
> line so the driver can assume that the card write protection is disabled.
>
> Signed-off-by: Thomas Abraham <[email protected]>
> ---
> ?drivers/mmc/host/dw_mmc.c ?| ? ?4 +++-
> ?include/linux/mmc/dw_mmc.h | ? ?3 ++-
> ?2 files changed, 5 insertions(+), 2 deletions(-)

Acked-by: Will Newton <[email protected]>

2012-05-02 09:17:29

by Will Newton

[permalink] [raw]
Subject: Re: [PATCH 1/7] mmc: dw_mmc: lookup for optional biu and ciu clocks

On Wed, May 2, 2012 at 6:07 AM, Thomas Abraham
<[email protected]> wrote:
> Some platforms allow for clock gating and control of bus interface unit clock
> and card interface unit clock. Add support for clock lookup of optional biu
> and ciu clocks for clock gating and clock speed determination.
>
> Signed-off-by: Abhilash Kesavan <[email protected]>
> Signed-off-by: Thomas Abraham <[email protected]>
> ---
> ?drivers/mmc/host/dw_mmc.c ?| ? 35 +++++++++++++++++++++++++++++++----
> ?include/linux/mmc/dw_mmc.h | ? ?4 ++++
> ?2 files changed, 35 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 1532357..036846f 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1938,19 +1938,35 @@ int dw_mci_probe(struct dw_mci *host)
> ? ? ? ? ? ? ? ?return -ENODEV;
> ? ? ? ?}
>
> - ? ? ? if (!host->pdata->bus_hz) {
> + ? ? ? host->biu_clk = clk_get(&host->dev, "biu");
> + ? ? ? if (IS_ERR(host->biu_clk))
> + ? ? ? ? ? ? ? dev_info(&host->dev, "biu clock not available\n");
> + ? ? ? else
> + ? ? ? ? ? ? ? clk_enable(host->biu_clk);
> +
> + ? ? ? host->ciu_clk = clk_get(&host->dev, "ciu");
> + ? ? ? if (IS_ERR(host->ciu_clk))
> + ? ? ? ? ? ? ? dev_info(&host->dev, "ciu clock not available\n");

Should these printks be at debug level? I'm not 100% sure either way
but it could be a little noisy on systems that do not use these
clocks.

> + ? ? ? else
> + ? ? ? ? ? ? ? clk_enable(host->ciu_clk);
> +
> + ? ? ? if (IS_ERR(host->ciu_clk))
> + ? ? ? ? ? ? ? host->bus_hz = host->pdata->bus_hz;
> + ? ? ? else
> + ? ? ? ? ? ? ? host->bus_hz = clk_get_rate(host->ciu_clk);
> +
> + ? ? ? if (!host->bus_hz) {
> ? ? ? ? ? ? ? ?dev_err(&host->dev,
> ? ? ? ? ? ? ? ? ? ? ? ?"Platform data must supply bus speed\n");
> - ? ? ? ? ? ? ? return -ENODEV;
> + ? ? ? ? ? ? ? ret = -ENODEV;
> + ? ? ? ? ? ? ? goto err_clk;
> ? ? ? ?}
>
> - ? ? ? host->bus_hz = host->pdata->bus_hz;
> ? ? ? ?host->quirks = host->pdata->quirks;
>
> ? ? ? ?spin_lock_init(&host->lock);
> ? ? ? ?INIT_LIST_HEAD(&host->queue);
>
> -
> ? ? ? ?host->dma_ops = host->pdata->dma_ops;
> ? ? ? ?dw_mci_init_dma(host);
>
> @@ -2095,6 +2111,13 @@ err_dmaunmap:
> ? ? ? ? ? ? ? ?regulator_disable(host->vmmc);
> ? ? ? ? ? ? ? ?regulator_put(host->vmmc);
> ? ? ? ?}
> + ? ? ? kfree(host);
> +
> +err_clk:
> + ? ? ? clk_disable(host->ciu_clk);
> + ? ? ? clk_disable(host->biu_clk);
> + ? ? ? clk_put(host->ciu_clk);
> + ? ? ? clk_put(host->biu_clk);

Are these calls safe, or do we need to check IS_ERR as above?

> ? ? ? ?return ret;
> ?}
> ?EXPORT_SYMBOL(dw_mci_probe);
> @@ -2128,6 +2151,10 @@ void dw_mci_remove(struct dw_mci *host)
> ? ? ? ? ? ? ? ?regulator_put(host->vmmc);
> ? ? ? ?}
>
> + ? ? ? clk_disable(host->ciu_clk);
> + ? ? ? clk_disable(host->biu_clk);
> + ? ? ? clk_put(host->ciu_clk);
> + ? ? ? clk_put(host->biu_clk);

Likewise.

> ?}
> ?EXPORT_SYMBOL(dw_mci_remove);
>
> diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
> index 7a7ebd3..fa9a139 100644
> --- a/include/linux/mmc/dw_mmc.h
> +++ b/include/linux/mmc/dw_mmc.h
> @@ -78,6 +78,8 @@ struct mmc_data;
> ?* @data_offset: Set the offset of DATA register according to VERID.
> ?* @dev: Device associated with the MMC controller.
> ?* @pdata: Platform data associated with the MMC controller.
> + * @biu_clk: Pointer to bus interface unit clock instance.
> + * @ciu_clk: Pointer to card interface unit clock instance.
> ?* @slot: Slots sharing this MMC controller.
> ?* @fifo_depth: depth of FIFO.
> ?* @data_shift: log2 of FIFO item size.
> @@ -158,6 +160,8 @@ struct dw_mci {
> ? ? ? ?u16 ? ? ? ? ? ? ? ? ? ? data_offset;
> ? ? ? ?struct device ? ? ? ? ? dev;
> ? ? ? ?struct dw_mci_board ? ? *pdata;
> + ? ? ? struct clk ? ? ? ? ? ? ?*biu_clk;
> + ? ? ? struct clk ? ? ? ? ? ? ?*ciu_clk;
> ? ? ? ?struct dw_mci_slot ? ? ?*slot[MAX_MCI_SLOTS];
>
> ? ? ? ?/* FIFO push and pull */
> --
> 1.7.5.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/

2012-05-02 09:53:41

by Russell King - ARM Linux

[permalink] [raw]
Subject: Re: [PATCH 1/7] mmc: dw_mmc: lookup for optional biu and ciu clocks

On Tue, May 01, 2012 at 10:07:40PM -0700, Thomas Abraham wrote:
> Some platforms allow for clock gating and control of bus interface unit clock
> and card interface unit clock. Add support for clock lookup of optional biu
> and ciu clocks for clock gating and clock speed determination.

As we're moving progressively towards the common clk API, which requires
the new clk_prepare/clk_unprepare calls, new code should be using the
dated API. Please update this to do so (eg, by using clk_prepare_enable()
instead of clk_enable() for the simple solution.) Better solutions
involve decoupling the two and only using clk_enable()/clk_disable() where
you really need the clock to be on.

2012-05-02 14:53:57

by James Hogan

[permalink] [raw]
Subject: Re: [PATCH 1/7] mmc: dw_mmc: lookup for optional biu and ciu clocks

Hi

On 2 May 2012 06:07, Thomas Abraham <[email protected]> wrote:
> Some platforms allow for clock gating and control of bus interface unit clock
> and card interface unit clock. Add support for clock lookup of optional biu
> and ciu clocks for clock gating and clock speed determination.
>
> Signed-off-by: Abhilash Kesavan <[email protected]>
> Signed-off-by: Thomas Abraham <[email protected]>
> ---
> ?drivers/mmc/host/dw_mmc.c ?| ? 35 +++++++++++++++++++++++++++++++----
> ?include/linux/mmc/dw_mmc.h | ? ?4 ++++
> ?2 files changed, 35 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 1532357..036846f 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1938,19 +1938,35 @@ int dw_mci_probe(struct dw_mci *host)
> ? ? ? ? ? ? ? ?return -ENODEV;
> ? ? ? ?}
>
> - ? ? ? if (!host->pdata->bus_hz) {
> + ? ? ? host->biu_clk = clk_get(&host->dev, "biu");

These clock names sound quite platform specific (what if they're
called something else on another platform, or another platform has
separate ones for different instantiations of the block?). Perhaps the
clk names should get passed in through platform data. I haven't looked
how other drivers handle that though.

> + ? ? ? if (IS_ERR(host->biu_clk))
> + ? ? ? ? ? ? ? dev_info(&host->dev, "biu clock not available\n");

In this case, should it set host->biu_clk to NULL or are clk_disable
and clk_put guaranteed to handle an IS_ERR value?

> + ? ? ? else
> + ? ? ? ? ? ? ? clk_enable(host->biu_clk);
> +
> + ? ? ? host->ciu_clk = clk_get(&host->dev, "ciu");
> + ? ? ? if (IS_ERR(host->ciu_clk))
> + ? ? ? ? ? ? ? dev_info(&host->dev, "ciu clock not available\n");

same here

> + ? ? ? else
> + ? ? ? ? ? ? ? clk_enable(host->ciu_clk);
> +
> + ? ? ? if (IS_ERR(host->ciu_clk))
> + ? ? ? ? ? ? ? host->bus_hz = host->pdata->bus_hz;
> + ? ? ? else
> + ? ? ? ? ? ? ? host->bus_hz = clk_get_rate(host->ciu_clk);
> +
> + ? ? ? if (!host->bus_hz) {
> ? ? ? ? ? ? ? ?dev_err(&host->dev,
> ? ? ? ? ? ? ? ? ? ? ? ?"Platform data must supply bus speed\n");
> - ? ? ? ? ? ? ? return -ENODEV;
> + ? ? ? ? ? ? ? ret = -ENODEV;
> + ? ? ? ? ? ? ? goto err_clk;
> ? ? ? ?}
>
> - ? ? ? host->bus_hz = host->pdata->bus_hz;
> ? ? ? ?host->quirks = host->pdata->quirks;
>
> ? ? ? ?spin_lock_init(&host->lock);
> ? ? ? ?INIT_LIST_HEAD(&host->queue);
>
> -
> ? ? ? ?host->dma_ops = host->pdata->dma_ops;
> ? ? ? ?dw_mci_init_dma(host);
>
> @@ -2095,6 +2111,13 @@ err_dmaunmap:
> ? ? ? ? ? ? ? ?regulator_disable(host->vmmc);
> ? ? ? ? ? ? ? ?regulator_put(host->vmmc);
> ? ? ? ?}
> + ? ? ? kfree(host);

what's this about?

> +
> +err_clk:
> + ? ? ? clk_disable(host->ciu_clk);
> + ? ? ? clk_disable(host->biu_clk);
> + ? ? ? clk_put(host->ciu_clk);
> + ? ? ? clk_put(host->biu_clk);
> ? ? ? ?return ret;
> ?}
> ?EXPORT_SYMBOL(dw_mci_probe);
> @@ -2128,6 +2151,10 @@ void dw_mci_remove(struct dw_mci *host)
> ? ? ? ? ? ? ? ?regulator_put(host->vmmc);
> ? ? ? ?}
>
> + ? ? ? clk_disable(host->ciu_clk);
> + ? ? ? clk_disable(host->biu_clk);
> + ? ? ? clk_put(host->ciu_clk);
> + ? ? ? clk_put(host->biu_clk);
> ?}
> ?EXPORT_SYMBOL(dw_mci_remove);
>
> diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
> index 7a7ebd3..fa9a139 100644
> --- a/include/linux/mmc/dw_mmc.h
> +++ b/include/linux/mmc/dw_mmc.h
> @@ -78,6 +78,8 @@ struct mmc_data;
> ?* @data_offset: Set the offset of DATA register according to VERID.
> ?* @dev: Device associated with the MMC controller.
> ?* @pdata: Platform data associated with the MMC controller.
> + * @biu_clk: Pointer to bus interface unit clock instance.
> + * @ciu_clk: Pointer to card interface unit clock instance.
> ?* @slot: Slots sharing this MMC controller.
> ?* @fifo_depth: depth of FIFO.
> ?* @data_shift: log2 of FIFO item size.
> @@ -158,6 +160,8 @@ struct dw_mci {
> ? ? ? ?u16 ? ? ? ? ? ? ? ? ? ? data_offset;
> ? ? ? ?struct device ? ? ? ? ? dev;
> ? ? ? ?struct dw_mci_board ? ? *pdata;
> + ? ? ? struct clk ? ? ? ? ? ? ?*biu_clk;
> + ? ? ? struct clk ? ? ? ? ? ? ?*ciu_clk;
> ? ? ? ?struct dw_mci_slot ? ? ?*slot[MAX_MCI_SLOTS];
>
> ? ? ? ?/* FIFO push and pull */
> --
> 1.7.5.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/



--
James Hogan

2012-05-02 18:07:48

by Olof Johansson

[permalink] [raw]
Subject: Re: [PATCH 3/7] mmc: dw_mmc: add device tree support

Hi,

On Tue, May 1, 2012 at 10:07 PM, Thomas Abraham
<[email protected]> wrote:
> Add device tree based discovery support.
>
> Signed-off-by: Thomas Abraham <[email protected]>
> ---
> ?.../devicetree/bindings/mmc/synposis-dw-mshc.txt ? | ? 85 +++++++++
> ?drivers/mmc/host/dw_mmc-pltfm.c ? ? ? ? ? ? ? ? ? ?| ? 24 +++
> ?drivers/mmc/host/dw_mmc.c ? ? ? ? ? ? ? ? ? ? ? ? ?| ?181 +++++++++++++++++++-
> ?drivers/mmc/host/dw_mmc.h ? ? ? ? ? ? ? ? ? ? ? ? ?| ? 10 +
> ?include/linux/mmc/dw_mmc.h ? ? ? ? ? ? ? ? ? ? ? ? | ? ?2 +
> ?5 files changed, 296 insertions(+), 6 deletions(-)
> ?create mode 100644 Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
>
> diff --git a/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
> new file mode 100644
> index 0000000..c1ed70e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
> @@ -0,0 +1,85 @@
> +* Synopsis Designware Mobile Storage Host Controller
> +
> +The Synopsis designware mobile storage host controller is used to interface
> +a SoC with storage medium such as eMMC or SD/MMC cards.
> +
> +Required Properties:
> +
> +* compatible: should be one of the following
> + ? ? ? - synopsis,dw-mshc: for controllers compliant with synopsis dw-mshc.
> +
> +* reg: physical base address of the dw-mshc controller and size of its memory
> + ?region.
> +
> +* interrupts: interrupt specifier for the controller. The format and value of
> + ?the interrupt specifier depends on the interrupt parent for the controller.
> +
> +# Slots: The slot specific information are contained within child-nodes with
> + ?each child-node representing a supported slot. There should be atleast one
> + ?child node representing a card slot. The name of the slot child node should
> + ?be 'slot{n}' where n is the unique number of the slot connnected to the
> + ?controller. The following are optional properties which can be included in
> + ?the slot child node.

Since we're talking slots / cards on a bus, I think the addressing
model would be useful here. So in the main controller node:
#address-cells = <1>;
#size-cells = <0>;

And then each slot would need a reg property and possibly unit address:

slot {
reg = <0>;
...
};

(unit addresses on the slots are only needed if they can't be
disambiguated by name, so not needed if you only have one slot).


-Olof

2012-05-02 18:10:47

by Olof Johansson

[permalink] [raw]
Subject: Re: [PATCH 4/7] mmc: dw_mmc: add samsung exynos5250 specific extentions

Hi,

On Tue, May 1, 2012 at 10:07 PM, Thomas Abraham
<[email protected]> wrote:
> The instantiation of the Synopsis Designware controller on Exynos5250
> include extension for SDR and DDR specific tx/rx phase shift timing
> and CIU internal divider. In addition to that, the option to skip the
> command hold stage is also introduced. Add support for these Exynos5250
> specfic extenstions.
>
> Signed-off-by: Abhilash Kesavan <[email protected]>
> Signed-off-by: Thomas Abraham <[email protected]>
> ---
> ?.../devicetree/bindings/mmc/synposis-dw-mshc.txt ? | ? 33 +++++++++++++++++++-
> ?drivers/mmc/host/dw_mmc-pltfm.c ? ? ? ? ? ? ? ? ? ?| ? ?8 +++++
> ?drivers/mmc/host/dw_mmc.c ? ? ? ? ? ? ? ? ? ? ? ? ?| ? 32 ++++++++++++++++++-
> ?drivers/mmc/host/dw_mmc.h ? ? ? ? ? ? ? ? ? ? ? ? ?| ? 13 ++++++++
> ?include/linux/mmc/dw_mmc.h ? ? ? ? ? ? ? ? ? ? ? ? | ? ?6 +++
> ?5 files changed, 89 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
> index c1ed70e..465fc31 100644
> --- a/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
> +++ b/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
> @@ -7,6 +7,8 @@ Required Properties:
>
> ?* compatible: should be one of the following
> ? ? ? ?- synopsis,dw-mshc: for controllers compliant with synopsis dw-mshc.
> + ? ? ? - synopsis,dw-mshc-exynos5250: for controllers with Samsung
> + ? ? ? ? Exynos5250 specific extentions.

It makes more sense to use your own manufacturer prefix here:

samsung,exynos5250-dw-mshc


-Olof

2012-05-03 22:49:09

by Guennadi Liakhovetski

[permalink] [raw]
Subject: Re: [PATCH 3/7] mmc: dw_mmc: add device tree support

On Tue, 1 May 2012, Thomas Abraham wrote:

> Add device tree based discovery support.
>
> Signed-off-by: Thomas Abraham <[email protected]>
> ---
> .../devicetree/bindings/mmc/synposis-dw-mshc.txt | 85 +++++++++
> drivers/mmc/host/dw_mmc-pltfm.c | 24 +++
> drivers/mmc/host/dw_mmc.c | 181 +++++++++++++++++++-
> drivers/mmc/host/dw_mmc.h | 10 +
> include/linux/mmc/dw_mmc.h | 2 +
> 5 files changed, 296 insertions(+), 6 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
>
> diff --git a/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
> new file mode 100644
> index 0000000..c1ed70e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
> @@ -0,0 +1,85 @@
> +* Synopsis Designware Mobile Storage Host Controller
> +
> +The Synopsis designware mobile storage host controller is used to interface
> +a SoC with storage medium such as eMMC or SD/MMC cards.
> +
> +Required Properties:
> +
> +* compatible: should be one of the following
> + - synopsis,dw-mshc: for controllers compliant with synopsis dw-mshc.
> +
> +* reg: physical base address of the dw-mshc controller and size of its memory
> + region.
> +
> +* interrupts: interrupt specifier for the controller. The format and value of
> + the interrupt specifier depends on the interrupt parent for the controller.
> +
> +# Slots: The slot specific information are contained within child-nodes with
> + each child-node representing a supported slot. There should be atleast one
> + child node representing a card slot. The name of the slot child node should
> + be 'slot{n}' where n is the unique number of the slot connnected to the
> + controller. The following are optional properties which can be included in
> + the slot child node.
> +
> + * bus-width: specifies the width of the data bus connected from the
> + controller to the card slot. The value should be 1, 4 or 8. In case
> + this property is not specified, a default value of 1 is assumed for
> + this property.
> +
> + * cd-gpios: specifies the card detect gpio line. The format of the
> + gpio specifier depends on the gpio controller.
> +
> + * wp-gpios: specifies the write protect gpio line. The format of the
> + gpio specifier depends on the gpio controller.
> +
> + * gpios: specifies a list of gpios used for command, clock and data
> + bus. The first gpio is the command line and the second gpio is the
> + clock line. The rest of the gpios (depending on the bus-width
> + property) are the data lines in no particular order. The format of
> + the gpio specifier depends on the gpio controller.
> +
> +Optional properties:
> +
> +* fifo-depth: The maximum size of the tx/rx fifo's. If this property is not
> + specified, the default value of the fifo size is determined from the
> + controller registers.
> +
> +* card-detect-delay: Delay in milli-seconds before detecting card after card
> + insert event. The default value is 0.
> +
> +* supports-highspeed: Enables support for high speed cards (upto 50MHz)
> +
> +* card-detection-broken: The card detection functionality is not available on
> + any of the slots.
> +
> +* no-write-protect: The write protect pad of the controller is not connected
> + to the write protect pin on the slot.

What do you think about this patch

http://www.spinics.net/lists/linux-sh/msg11259.html

and about using mmc-generic OF properties instead of creating yet another
copy of proprietary ones?

Thanks
Guennadi

> +
> +Example:
> +
> + The MSHC controller node can be split into two portions, SoC specific and
> + board specific portions as listed below.
> +
> + dwmmc0@12200000 {
> + compatible = "synopsis,dw-mshc";
> + reg = <0x12200000 0x1000>;
> + interrupts = <0 75 0>;
> + };
> +
> + dwmmc0@12200000 {
> + supports-highspeed;
> + card-detection-broken;
> + no-write-protect;
> + fifo-depth = <0x80>;
> + card-detect-delay = <200>;
> +
> + slot0 {
> + bus-width = <8>;
> + cd-gpios = <&gpc0 2 2 3 3>;
> + gpios = <&gpc0 0 2 0 3>, <&gpc0 1 2 0 3>,
> + <&gpc1 0 2 3 3>, <&gpc1 1 2 3 3>,
> + <&gpc1 2 2 3 3>, <&gpc1 3 2 3 3>,
> + <&gpc0 3 2 3 3>, <&gpc0 4 2 3 3>,
> + <&gpc0 5 2 3 3>, <&gpc0 6 2 3 3>;
> + };
> + };
> diff --git a/drivers/mmc/host/dw_mmc-pltfm.c b/drivers/mmc/host/dw_mmc-pltfm.c
> index 92ec3eb..2b2c9bd 100644
> --- a/drivers/mmc/host/dw_mmc-pltfm.c
> +++ b/drivers/mmc/host/dw_mmc-pltfm.c
> @@ -19,8 +19,24 @@
> #include <linux/mmc/host.h>
> #include <linux/mmc/mmc.h>
> #include <linux/mmc/dw_mmc.h>
> +#include <linux/of.h>
> #include "dw_mmc.h"
>
> +#ifdef CONFIG_OF
> +static struct dw_mci_drv_data synopsis_drv_data = {
> + .ctrl_type = DW_MCI_TYPE_SYNOPSIS,
> +};
> +
> +static const struct of_device_id dw_mci_pltfm_match[] = {
> + { .compatible = "synopsis,dw-mshc",
> + .data = (void *)&synopsis_drv_data, },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, dw_mci_pltfm_match);
> +#else
> +static const struct of_device_id dw_mci_pltfm_match[];
> +#endif
> +
> static int dw_mci_pltfm_probe(struct platform_device *pdev)
> {
> struct dw_mci *host;
> @@ -51,6 +67,13 @@ static int dw_mci_pltfm_probe(struct platform_device *pdev)
> if (!host->regs)
> goto err_free;
> platform_set_drvdata(pdev, host);
> +
> + if (pdev->dev.of_node) {
> + const struct of_device_id *match;
> + match = of_match_node(dw_mci_pltfm_match, pdev->dev.of_node);
> + host->drv_data = match->data;
> + }
> +
> ret = dw_mci_probe(host);
> if (ret)
> goto err_out;
> @@ -111,6 +134,7 @@ static struct platform_driver dw_mci_pltfm_driver = {
> .remove = __exit_p(dw_mci_pltfm_remove),
> .driver = {
> .name = "dw_mmc",
> + .of_match_table = of_match_ptr(dw_mci_pltfm_match),
> .pm = &dw_mci_pltfm_pmops,
> },
> };
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 38cb7f8..bcf66d7 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -33,9 +33,13 @@
> #include <linux/bitops.h>
> #include <linux/regulator/consumer.h>
> #include <linux/workqueue.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
>
> #include "dw_mmc.h"
>
> +#define NUM_PINS(x) (x + 2)
> +
> /* Common flag combinations */
> #define DW_MCI_DATA_ERROR_FLAGS (SDMMC_INT_DTO | SDMMC_INT_DCRC | \
> SDMMC_INT_HTO | SDMMC_INT_SBE | \
> @@ -86,6 +90,8 @@ struct idmac_desc {
> struct dw_mci_slot {
> struct mmc_host *mmc;
> struct dw_mci *host;
> + int wp_gpio;
> + int cd_gpio;
>
> u32 ctype;
>
> @@ -816,6 +822,8 @@ static int dw_mci_get_ro(struct mmc_host *mmc)
> read_only = 0;
> else if (brd->get_ro)
> read_only = brd->get_ro(slot->id);
> + else if (gpio_is_valid(slot->wp_gpio))
> + read_only = gpio_get_value(slot->wp_gpio);
> else
> read_only =
> mci_readl(slot->host, WRTPRT) & (1 << slot->id) ? 1 : 0;
> @@ -837,6 +845,8 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
> present = 1;
> else if (brd->get_cd)
> present = !brd->get_cd(slot->id);
> + else if (gpio_is_valid(slot->cd_gpio))
> + present = gpio_get_value(slot->cd_gpio);
> else
> present = (mci_readl(slot->host, CDETECT) & (1 << slot->id))
> == 0 ? 1 : 0;
> @@ -1747,6 +1757,96 @@ static void dw_mci_work_routine_card(struct work_struct *work)
> }
> }
>
> +#ifdef CONFIG_OF
> +static struct device_node *dw_mci_of_find_slot_node(struct device *dev, u8 slot)
> +{
> + struct device_node *np;
> + char name[7];
> +
> + if (!dev || !dev->of_node)
> + return NULL;
> +
> + for_each_child_of_node(dev->of_node, np) {
> + sprintf(name, "slot%d", slot);
> + if (!strcmp(name, np->name))
> + return np;
> + }
> + return NULL;
> +}
> +
> +static u32 dw_mci_of_get_bus_wd(struct device *dev, u8 slot)
> +{
> + struct device_node *np = dw_mci_of_find_slot_node(dev, slot);
> + u32 bus_wd = 1;
> +
> + if (!np)
> + return 1;
> +
> + if (of_property_read_u32(np, "bus-width", &bus_wd))
> + dev_err(dev, "bus-width property not found, assuming width"
> + " as 1\n");
> + return bus_wd;
> +}
> +
> +static int dw_mci_of_setup_bus(struct dw_mci *host, u8 slot, u32 bus_wd)
> +{
> + struct device_node *np = dw_mci_of_find_slot_node(&host->dev, slot);
> + int idx, gpio, ret;
> +
> + for (idx = 0; idx < NUM_PINS(bus_wd); idx++) {
> + gpio = of_get_gpio(np, idx);
> + if (!gpio_is_valid(gpio)) {
> + dev_err(&host->dev, "invalid gpio: %d\n", gpio);
> + return -EINVAL;
> + }
> +
> + ret = devm_gpio_request(&host->dev, gpio, "dw-mci-bus");
> + if (ret) {
> + dev_err(&host->dev, "gpio [%d] request failed\n", gpio);
> + return -EBUSY;
> + }
> + }
> +
> + host->slot[slot]->wp_gpio = -1;
> + gpio = of_get_named_gpio(np, "wp_gpios", 0);
> + if (!gpio_is_valid(gpio)) {
> + dev_info(&host->dev, "wp gpio not available");
> + } else {
> + ret = devm_gpio_request(&host->dev, gpio, "dw-mci-wp");
> + if (ret)
> + dev_info(&host->dev, "gpio [%d] request failed\n",
> + gpio);
> + else
> + host->slot[slot]->wp_gpio = gpio;
> + }
> +
> + host->slot[slot]->cd_gpio = -1;
> + gpio = of_get_named_gpio(np, "cd-gpios", 0);
> + if (!gpio_is_valid(gpio)) {
> + dev_info(&host->dev, "cd gpio not available");
> + } else {
> + ret = devm_gpio_request(&host->dev, gpio, "dw-mci-cd");
> + if (ret)
> + dev_err(&host->dev, "gpio [%d] request failed\n", gpio);
> + else
> + host->slot[slot]->cd_gpio = gpio;
> + }
> +
> + return 0;
> +}
> +
> +#else /* CONFIG_OF */
> +static u32 dw_mci_of_get_bus_wd(struct device *dev, u8 slot)
> +{
> + return 1;
> +}
> +
> +static int dw_mci_of_setup_bus(struct dw_mci *host, u8 slot, u32 bus_wd)
> +{
> + return -EINVAL;
> +}
> +#endif /* CONFIG_OF */
> +
> static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id)
> {
> struct mmc_host *mmc;
> @@ -1760,6 +1860,7 @@ static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id)
> slot->id = id;
> slot->mmc = mmc;
> slot->host = host;
> + host->slot[id] = slot;
>
> mmc->ops = &dw_mci_ops;
> mmc->f_min = DIV_ROUND_UP(host->bus_hz, 510);
> @@ -1780,12 +1881,21 @@ static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id)
> if (host->pdata->caps)
> mmc->caps = host->pdata->caps;
>
> + mmc->caps |= host->drv_data->caps;
> +
> if (host->pdata->caps2)
> mmc->caps2 = host->pdata->caps2;
>
> - if (host->pdata->get_bus_wd)
> + if (host->pdata->get_bus_wd) {
> if (host->pdata->get_bus_wd(slot->id) >= 4)
> mmc->caps |= MMC_CAP_4_BIT_DATA;
> + } else if (host->dev.of_node) {
> + unsigned int bus_width;
> + bus_width = dw_mci_of_get_bus_wd(&host->dev, slot->id);
> + if (bus_width >= 4)
> + mmc->caps |= MMC_CAP_4_BIT_DATA;
> + dw_mci_of_setup_bus(host, slot->id, bus_width);
> + }
>
> if (host->pdata->quirks & DW_MCI_QUIRK_HIGHSPEED)
> mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
> @@ -1830,7 +1940,6 @@ static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id)
> else
> clear_bit(DW_MMC_CARD_PRESENT, &slot->flags);
>
> - host->slot[id] = slot;
> mmc_add_host(mmc);
>
> #if defined(CONFIG_DEBUG_FS)
> @@ -1923,15 +2032,75 @@ static bool mci_wait_reset(struct device *dev, struct dw_mci *host)
> return false;
> }
>
> +#ifdef CONFIG_OF
> +static struct dw_mci_of_quirks {
> + char *quirk;
> + int id;
> +} of_quriks[] = {
> + {
> + .quirk = "supports-highspeed",
> + .id = DW_MCI_QUIRK_HIGHSPEED,
> + }, {
> + .quirk = "card-detection-broken",
> + .id = DW_MCI_QUIRK_BROKEN_CARD_DETECTION,
> + }, {
> + .quirk = "no-write-protect",
> + .id = DW_MCI_QUIRK_NO_WRITE_PROTECT,
> + },
> + { }
> +};
> +
> +static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
> +{
> + struct dw_mci_board *pdata;
> + struct device *dev = &host->dev;
> + struct device_node *np = dev->of_node, *slot;
> + u32 timing[3];
> + int idx, cnt;
> +
> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata) {
> + dev_err(dev, "could not allocate memory for pdata\n");
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + /* find out number of slots supported */
> + for_each_child_of_node(np, slot)
> + pdata->num_slots++;
> +
> + /* get quirks */
> + cnt = sizeof(of_quriks) / sizeof(struct dw_mci_of_quirks);
> + for (idx = 0; idx < cnt; idx++)
> + if (of_get_property(np, of_quriks[idx].quirk, NULL))
> + pdata->quirks |= of_quriks[idx].id;
> +
> + if (of_property_read_u32(np, "fifo-depth", &pdata->fifo_depth))
> + dev_info(dev, "fifo-depth property not found, using "
> + "value of FIFOTH register as default\n");
> +
> + of_property_read_u32(np, "card-detect-delay", &pdata->detect_delay_ms);
> +
> + return pdata;
> +}
> +
> +#else /* CONFIG_OF */
> +static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
> +{
> + return ERR_PTR(-EINVAL);
> +}
> +#endif /* CONFIG_OF */
> +
> int dw_mci_probe(struct dw_mci *host)
> {
> int width, i, ret = 0;
> u32 fifo_size;
>
> - if (!host->pdata || !host->pdata->init) {
> - dev_err(&host->dev,
> - "Platform data must supply init function\n");
> - return -ENODEV;
> + if (!host->pdata) {
> + host->pdata = dw_mci_parse_dt(host);
> + if (IS_ERR(host->pdata)) {
> + dev_err(&host->dev, "platform data not available\n");
> + return -EINVAL;
> + }
> }
>
> if (!host->pdata->select_slot && host->pdata->num_slots > 1) {
> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
> index 15c27e1..8b8862b 100644
> --- a/drivers/mmc/host/dw_mmc.h
> +++ b/drivers/mmc/host/dw_mmc.h
> @@ -182,4 +182,14 @@ extern int dw_mci_suspend(struct dw_mci *host);
> extern int dw_mci_resume(struct dw_mci *host);
> #endif
>
> +/* Variations in the dw_mci controller */
> +#define DW_MCI_TYPE_SYNOPSIS 0
> +#define DW_MCI_TYPE_EXYNOS5250 1 /* Samsung Exynos5250 Extensions */
> +
> +/* dw_mci platform driver data */
> +struct dw_mci_drv_data {
> + unsigned long ctrl_type;
> + unsigned long caps;
> +};
> +
> #endif /* _DW_MMC_H_ */
> diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
> index fb51a5f..71d2b56 100644
> --- a/include/linux/mmc/dw_mmc.h
> +++ b/include/linux/mmc/dw_mmc.h
> @@ -78,6 +78,7 @@ struct mmc_data;
> * @data_offset: Set the offset of DATA register according to VERID.
> * @dev: Device associated with the MMC controller.
> * @pdata: Platform data associated with the MMC controller.
> + * @drv_data: Driver specific data for identified variant of the controller
> * @biu_clk: Pointer to bus interface unit clock instance.
> * @ciu_clk: Pointer to card interface unit clock instance.
> * @slot: Slots sharing this MMC controller.
> @@ -160,6 +161,7 @@ struct dw_mci {
> u16 data_offset;
> struct device dev;
> struct dw_mci_board *pdata;
> + struct dw_mci_drv_data *drv_data;
> struct clk *biu_clk;
> struct clk *ciu_clk;
> struct dw_mci_slot *slot[MAX_MCI_SLOTS];
> --
> 1.7.5.4
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

2012-05-10 09:11:14

by Thomas Abraham

[permalink] [raw]
Subject: Re: [PATCH 1/7] mmc: dw_mmc: lookup for optional biu and ciu clocks

On 2 May 2012 14:47, Will Newton <[email protected]> wrote:
> On Wed, May 2, 2012 at 6:07 AM, Thomas Abraham
> <[email protected]> wrote:
>> Some platforms allow for clock gating and control of bus interface unit clock
>> and card interface unit clock. Add support for clock lookup of optional biu
>> and ciu clocks for clock gating and clock speed determination.
>>
>> Signed-off-by: Abhilash Kesavan <[email protected]>
>> Signed-off-by: Thomas Abraham <[email protected]>
>> ---
>> ?drivers/mmc/host/dw_mmc.c ?| ? 35 +++++++++++++++++++++++++++++++----
>> ?include/linux/mmc/dw_mmc.h | ? ?4 ++++
>> ?2 files changed, 35 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 1532357..036846f 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -1938,19 +1938,35 @@ int dw_mci_probe(struct dw_mci *host)
>> ? ? ? ? ? ? ? ?return -ENODEV;
>> ? ? ? ?}
>>
>> - ? ? ? if (!host->pdata->bus_hz) {
>> + ? ? ? host->biu_clk = clk_get(&host->dev, "biu");
>> + ? ? ? if (IS_ERR(host->biu_clk))
>> + ? ? ? ? ? ? ? dev_info(&host->dev, "biu clock not available\n");
>> + ? ? ? else
>> + ? ? ? ? ? ? ? clk_enable(host->biu_clk);
>> +
>> + ? ? ? host->ciu_clk = clk_get(&host->dev, "ciu");
>> + ? ? ? if (IS_ERR(host->ciu_clk))
>> + ? ? ? ? ? ? ? dev_info(&host->dev, "ciu clock not available\n");
>
> Should these printks be at debug level? I'm not 100% sure either way
> but it could be a little noisy on systems that do not use these
> clocks.

Right. I will change dev_info to dev_dbg.

>
>> + ? ? ? else
>> + ? ? ? ? ? ? ? clk_enable(host->ciu_clk);
>> +
>> + ? ? ? if (IS_ERR(host->ciu_clk))
>> + ? ? ? ? ? ? ? host->bus_hz = host->pdata->bus_hz;
>> + ? ? ? else
>> + ? ? ? ? ? ? ? host->bus_hz = clk_get_rate(host->ciu_clk);
>> +
>> + ? ? ? if (!host->bus_hz) {
>> ? ? ? ? ? ? ? ?dev_err(&host->dev,
>> ? ? ? ? ? ? ? ? ? ? ? ?"Platform data must supply bus speed\n");
>> - ? ? ? ? ? ? ? return -ENODEV;
>> + ? ? ? ? ? ? ? ret = -ENODEV;
>> + ? ? ? ? ? ? ? goto err_clk;
>> ? ? ? ?}
>>
>> - ? ? ? host->bus_hz = host->pdata->bus_hz;
>> ? ? ? ?host->quirks = host->pdata->quirks;
>>
>> ? ? ? ?spin_lock_init(&host->lock);
>> ? ? ? ?INIT_LIST_HEAD(&host->queue);
>>
>> -
>> ? ? ? ?host->dma_ops = host->pdata->dma_ops;
>> ? ? ? ?dw_mci_init_dma(host);
>>
>> @@ -2095,6 +2111,13 @@ err_dmaunmap:
>> ? ? ? ? ? ? ? ?regulator_disable(host->vmmc);
>> ? ? ? ? ? ? ? ?regulator_put(host->vmmc);
>> ? ? ? ?}
>> + ? ? ? kfree(host);
>> +
>> +err_clk:
>> + ? ? ? clk_disable(host->ciu_clk);
>> + ? ? ? clk_disable(host->biu_clk);
>> + ? ? ? clk_put(host->ciu_clk);
>> + ? ? ? clk_put(host->biu_clk);
>
> Are these calls safe, or do we need to check IS_ERR as above?

The clk_disable is safe on Samsung platforms but it cannot be assumed
for other platforms. So, the IS_ERR check will be added before
clk_disable. clk_put will not need the IS_ERR check. Thanks for
pointing this out.

>
>> ? ? ? ?return ret;
>> ?}
>> ?EXPORT_SYMBOL(dw_mci_probe);
>> @@ -2128,6 +2151,10 @@ void dw_mci_remove(struct dw_mci *host)
>> ? ? ? ? ? ? ? ?regulator_put(host->vmmc);
>> ? ? ? ?}
>>
>> + ? ? ? clk_disable(host->ciu_clk);
>> + ? ? ? clk_disable(host->biu_clk);
>> + ? ? ? clk_put(host->ciu_clk);
>> + ? ? ? clk_put(host->biu_clk);
>
> Likewise.

Yes, will be fixed.

Thanks,
Thomas.

[...]

2012-05-10 09:12:32

by Thomas Abraham

[permalink] [raw]
Subject: Re: [PATCH 1/7] mmc: dw_mmc: lookup for optional biu and ciu clocks

On 2 May 2012 15:23, Russell King - ARM Linux <[email protected]> wrote:
> On Tue, May 01, 2012 at 10:07:40PM -0700, Thomas Abraham wrote:
>> Some platforms allow for clock gating and control of bus interface unit clock
>> and card interface unit clock. Add support for clock lookup of optional biu
>> and ciu clocks for clock gating and clock speed determination.
>
> As we're moving progressively towards the common clk API, which requires
> the new clk_prepare/clk_unprepare calls, new code should be using the
> dated API. ?Please update this to do so (eg, by using clk_prepare_enable()
> instead of clk_enable() for the simple solution.) ?Better solutions
> involve decoupling the two and only using clk_enable()/clk_disable() where
> you really need the clock to be on.

Thanks for your suggestion. I will modify this to use the
clk_prepare_enable and clk_disable_unprepare calls for now.

Thanks,
Thomas.

2012-05-10 09:20:08

by Thomas Abraham

[permalink] [raw]
Subject: Re: [PATCH 1/7] mmc: dw_mmc: lookup for optional biu and ciu clocks

On 2 May 2012 20:23, James Hogan <[email protected]> wrote:
> Hi
>
> On 2 May 2012 06:07, Thomas Abraham <[email protected]> wrote:
>> Some platforms allow for clock gating and control of bus interface unit clock
>> and card interface unit clock. Add support for clock lookup of optional biu
>> and ciu clocks for clock gating and clock speed determination.
>>
>> Signed-off-by: Abhilash Kesavan <[email protected]>
>> Signed-off-by: Thomas Abraham <[email protected]>
>> ---
>> ?drivers/mmc/host/dw_mmc.c ?| ? 35 +++++++++++++++++++++++++++++++----
>> ?include/linux/mmc/dw_mmc.h | ? ?4 ++++
>> ?2 files changed, 35 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 1532357..036846f 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -1938,19 +1938,35 @@ int dw_mci_probe(struct dw_mci *host)
>> ? ? ? ? ? ? ? ?return -ENODEV;
>> ? ? ? ?}
>>
>> - ? ? ? if (!host->pdata->bus_hz) {
>> + ? ? ? host->biu_clk = clk_get(&host->dev, "biu");
>
> These clock names sound quite platform specific (what if they're
> called something else on another platform, or another platform has
> separate ones for different instantiations of the block?). Perhaps the
> clk names should get passed in through platform data. I haven't looked
> how other drivers handle that though.

The clock names 'biu' and 'ciu' are derived from the terminology used
by the data sheet of the mshc controller. The 'biu' clock is the
source clock for the bus interface unit and 'ciu' clock is the clock
source for card interface unit of the controller. So these names are
generic and not specific to a platform. Passing clock names from
platform data is generally frowned upon.

>
>> + ? ? ? if (IS_ERR(host->biu_clk))
>> + ? ? ? ? ? ? ? dev_info(&host->dev, "biu clock not available\n");
>
> In this case, should it set host->biu_clk to NULL or are clk_disable
> and clk_put guaranteed to handle an IS_ERR value?

Yes, the clk_disable will have to be preceded with a IS_ERR check. I
will fix this.

>
>> + ? ? ? else
>> + ? ? ? ? ? ? ? clk_enable(host->biu_clk);
>> +
>> + ? ? ? host->ciu_clk = clk_get(&host->dev, "ciu");
>> + ? ? ? if (IS_ERR(host->ciu_clk))
>> + ? ? ? ? ? ? ? dev_info(&host->dev, "ciu clock not available\n");
>
> same here

Ok, this also will be fixed.

>
>> + ? ? ? else
>> + ? ? ? ? ? ? ? clk_enable(host->ciu_clk);
>> +
>> + ? ? ? if (IS_ERR(host->ciu_clk))
>> + ? ? ? ? ? ? ? host->bus_hz = host->pdata->bus_hz;
>> + ? ? ? else
>> + ? ? ? ? ? ? ? host->bus_hz = clk_get_rate(host->ciu_clk);
>> +
>> + ? ? ? if (!host->bus_hz) {
>> ? ? ? ? ? ? ? ?dev_err(&host->dev,
>> ? ? ? ? ? ? ? ? ? ? ? ?"Platform data must supply bus speed\n");
>> - ? ? ? ? ? ? ? return -ENODEV;
>> + ? ? ? ? ? ? ? ret = -ENODEV;
>> + ? ? ? ? ? ? ? goto err_clk;
>> ? ? ? ?}
>>
>> - ? ? ? host->bus_hz = host->pdata->bus_hz;
>> ? ? ? ?host->quirks = host->pdata->quirks;
>>
>> ? ? ? ?spin_lock_init(&host->lock);
>> ? ? ? ?INIT_LIST_HEAD(&host->queue);
>>
>> -
>> ? ? ? ?host->dma_ops = host->pdata->dma_ops;
>> ? ? ? ?dw_mci_init_dma(host);
>>
>> @@ -2095,6 +2111,13 @@ err_dmaunmap:
>> ? ? ? ? ? ? ? ?regulator_disable(host->vmmc);
>> ? ? ? ? ? ? ? ?regulator_put(host->vmmc);
>> ? ? ? ?}
>> + ? ? ? kfree(host);
>
> what's this about?

This is wrong. It should not have been here. I will fix this. Thanks
for pointing this out.

Thanks,
Thomas.

2012-05-10 09:42:36

by Thomas Abraham

[permalink] [raw]
Subject: Re: [PATCH 3/7] mmc: dw_mmc: add device tree support

Dear Mr. Park,

On 2 May 2012 12:25, Kyungmin Park <[email protected]> wrote:
> Hi,
>
> On 5/2/12, Thomas Abraham <[email protected]> wrote:
>> Add device tree based discovery support.
>>
>> Signed-off-by: Thomas Abraham <[email protected]>
>> ---
>> ?.../devicetree/bindings/mmc/synposis-dw-mshc.txt ? | ? 85 +++++++++
>> ?drivers/mmc/host/dw_mmc-pltfm.c ? ? ? ? ? ? ? ? ? ?| ? 24 +++
>> ?drivers/mmc/host/dw_mmc.c ? ? ? ? ? ? ? ? ? ? ? ? ?| ?181
>> +++++++++++++++++++-
>> ?drivers/mmc/host/dw_mmc.h ? ? ? ? ? ? ? ? ? ? ? ? ?| ? 10 +
>> ?include/linux/mmc/dw_mmc.h ? ? ? ? ? ? ? ? ? ? ? ? | ? ?2 +
>> ?5 files changed, 296 insertions(+), 6 deletions(-)
>> ?create mode 100644
>> Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
>> b/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
>> new file mode 100644
>> index 0000000..c1ed70e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
>> @@ -0,0 +1,85 @@
>> +* Synopsis Designware Mobile Storage Host Controller
>> +
>> +The Synopsis designware mobile storage host controller is used to
>> interface
>> +a SoC with storage medium such as eMMC or SD/MMC cards.
>> +
>> +Required Properties:
>> +
>> +* compatible: should be one of the following
>> + ? ? - synopsis,dw-mshc: for controllers compliant with synopsis dw-mshc.
> I googled the "Synopsis Designware Mobile Storage Host Controller" and
> Synopsis released it as this name. but still I like the 'dw-mmc'
> instead of'dw-mshc'.

Ok. Synopsis abbreviates this controller as MSHC in their datasheets
available online. Since device tree is more about describing the
hardware, using MSHC as the abbreviation will help with correlating
hardware specs with the dts file. So I would prefer to continue using
mshc as the abbreviation.

>> +
>> +* reg: physical base address of the dw-mshc controller and size of its
>> memory
>> + ?region.
>> +
>> +* interrupts: interrupt specifier for the controller. The format and value
>> of
>> + ?the interrupt specifier depends on the interrupt parent for the
>> controller.
>> +
>> +# Slots: The slot specific information are contained within child-nodes
>> with
>> + ?each child-node representing a supported slot. There should be atleast
>> one
>> + ?child node representing a card slot. The name of the slot child node
>> should
>> + ?be 'slot{n}' where n is the unique number of the slot connnected to the
>> + ?controller. The following are optional properties which can be included
>> in
>> + ?the slot child node.
>> +
>> + ? ? * bus-width: specifies the width of the data bus connected from the
>> + ? ? ? controller to the card slot. The value should be 1, 4 or 8. In case
>> + ? ? ? this property is not specified, a default value of 1 is assumed for
>> + ? ? ? this property.
>> +
>> + ? ? * cd-gpios: specifies the card detect gpio line. The format of the
>> + ? ? ? gpio specifier depends on the gpio controller.
>> +
>> + ? ? * wp-gpios: specifies the write protect gpio line. The format of the
>> + ? ? ? gpio specifier depends on the gpio controller.
>> +
>> + ? ? * gpios: specifies a list of gpios used for command, clock and data
>> + ? ? ? bus. The first gpio is the command line and the second gpio is the
>> + ? ? ? clock line. The rest of the gpios (depending on the bus-width
>> + ? ? ? property) are the data lines in no particular order. The format of
>> + ? ? ? the gpio specifier depends on the gpio controller.
>> +
>> +Optional properties:
>> +
>> +* fifo-depth: The maximum size of the tx/rx fifo's. If this property is
>> not
>> + ?specified, the default value of the fifo size is determined from the
>> + ?controller registers.
>> +
>> +* ?card-detect-delay: Delay in milli-seconds before detecting card after
>> card
>> + ? insert event. The default value is 0.
>> +
>> +* supports-highspeed: Enables support for high speed cards (upto 50MHz)
>> +
>> +* card-detection-broken: The card detection functionality is not available
>> on
>> + ?any of the slots.
>> +
>> +* no-write-protect: The write protect pad of the controller is not
>> connected
>> + ?to the write protect pin on the slot.
>> +
>> +Example:
>> +
>> + ?The MSHC controller node can be split into two portions, SoC specific
>> and
>> + ?board specific portions as listed below.
>> +
>> + ? ? dwmmc0@12200000 {
>> + ? ? ? ? ? ? compatible = "synopsis,dw-mshc";
>> + ? ? ? ? ? ? reg = <0x12200000 0x1000>;
>> + ? ? ? ? ? ? interrupts = <0 75 0>;
>> + ? ? };
>> +
>> + ? ? dwmmc0@12200000 {
>> + ? ? ? ? ? ? supports-highspeed;
>> + ? ? ? ? ? ? card-detection-broken;
>> + ? ? ? ? ? ? no-write-protect;
>> + ? ? ? ? ? ? fifo-depth = <0x80>;
>> + ? ? ? ? ? ? card-detect-delay = <200>;
>> +
>> + ? ? ? ? ? ? slot0 {
>> + ? ? ? ? ? ? ? ? ? ? bus-width = <8>;
>> + ? ? ? ? ? ? ? ? ? ? cd-gpios = <&gpc0 2 2 3 3>;
>> + ? ? ? ? ? ? ? ? ? ? gpios = <&gpc0 0 2 0 3>, <&gpc0 1 2 0 3>,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? <&gpc1 0 2 3 3>, <&gpc1 1 2 3 3>,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? <&gpc1 2 2 3 3>, <&gpc1 3 2 3 3>,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? <&gpc0 3 2 3 3>, <&gpc0 4 2 3 3>,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? <&gpc0 5 2 3 3>, <&gpc0 6 2 3 3>;
>> + ? ? ? ? ? ? };
>> + ? ? };
>> diff --git a/drivers/mmc/host/dw_mmc-pltfm.c
>> b/drivers/mmc/host/dw_mmc-pltfm.c
>> index 92ec3eb..2b2c9bd 100644
>> --- a/drivers/mmc/host/dw_mmc-pltfm.c
>> +++ b/drivers/mmc/host/dw_mmc-pltfm.c
>> @@ -19,8 +19,24 @@
>> ?#include <linux/mmc/host.h>
>> ?#include <linux/mmc/mmc.h>
>> ?#include <linux/mmc/dw_mmc.h>
>> +#include <linux/of.h>
>> ?#include "dw_mmc.h"
>>
>> +#ifdef CONFIG_OF
>> +static struct dw_mci_drv_data synopsis_drv_data = {
>> + ? ? .ctrl_type ? ? ?= DW_MCI_TYPE_SYNOPSIS,
>> +};
>> +
>> +static const struct of_device_id dw_mci_pltfm_match[] = {
>> + ? ? { .compatible = "synopsis,dw-mshc",
>> + ? ? ? ? ? ? ? ? ? ? .data = (void *)&synopsis_drv_data, },
>> + ? ? {},
>> +};
>> +MODULE_DEVICE_TABLE(of, dw_mci_pltfm_match);
>> +#else
>> +static const struct of_device_id dw_mci_pltfm_match[];
>> +#endif
>> +
>> ?static int dw_mci_pltfm_probe(struct platform_device *pdev)
>> ?{
>> ? ? ? struct dw_mci *host;
>> @@ -51,6 +67,13 @@ static int dw_mci_pltfm_probe(struct platform_device
>> *pdev)
>> ? ? ? if (!host->regs)
>> ? ? ? ? ? ? ? goto err_free;
>> ? ? ? platform_set_drvdata(pdev, host);
>> +
>> + ? ? if (pdev->dev.of_node) {
>> + ? ? ? ? ? ? const struct of_device_id *match;
>> + ? ? ? ? ? ? match = of_match_node(dw_mci_pltfm_match, pdev->dev.of_node);
>> + ? ? ? ? ? ? host->drv_data = match->data;
>> + ? ? }
>> +
>> ? ? ? ret = dw_mci_probe(host);
>> ? ? ? if (ret)
>> ? ? ? ? ? ? ? goto err_out;
>> @@ -111,6 +134,7 @@ static struct platform_driver dw_mci_pltfm_driver = {
>> ? ? ? .remove ? ? ? ? = __exit_p(dw_mci_pltfm_remove),
>> ? ? ? .driver ? ? ? ? = {
>> ? ? ? ? ? ? ? .name ? ? ? ? ? = "dw_mmc",
>> + ? ? ? ? ? ? .of_match_table = of_match_ptr(dw_mci_pltfm_match),
>> ? ? ? ? ? ? ? .pm ? ? ? ? ? ? = &dw_mci_pltfm_pmops,
>> ? ? ? },
>> ?};
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 38cb7f8..bcf66d7 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -33,9 +33,13 @@
>> ?#include <linux/bitops.h>
>> ?#include <linux/regulator/consumer.h>
>> ?#include <linux/workqueue.h>
>> +#include <linux/of.h>
>> +#include <linux/of_gpio.h>
>>
>> ?#include "dw_mmc.h"
>>
>> +#define NUM_PINS(x) (x + 2)
>> +
>> ?/* Common flag combinations */
>> ?#define DW_MCI_DATA_ERROR_FLAGS ? ? ?(SDMMC_INT_DTO | SDMMC_INT_DCRC | \
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?SDMMC_INT_HTO | SDMMC_INT_SBE ?| \
>> @@ -86,6 +90,8 @@ struct idmac_desc {
>> ?struct dw_mci_slot {
>> ? ? ? struct mmc_host ? ? ? ? *mmc;
>> ? ? ? struct dw_mci ? ? ? ? ? *host;
>> + ? ? int ? ? ? ? ? ? ? ? ? ? wp_gpio;
>> + ? ? int ? ? ? ? ? ? ? ? ? ? cd_gpio;
>>
>> ? ? ? u32 ? ? ? ? ? ? ? ? ? ? ctype;
>>
>> @@ -816,6 +822,8 @@ static int dw_mci_get_ro(struct mmc_host *mmc)
>> ? ? ? ? ? ? ? read_only = 0;
>> ? ? ? else if (brd->get_ro)
>> ? ? ? ? ? ? ? read_only = brd->get_ro(slot->id);
>> + ? ? else if (gpio_is_valid(slot->wp_gpio))
>> + ? ? ? ? ? ? read_only = gpio_get_value(slot->wp_gpio);
>> ? ? ? else
>> ? ? ? ? ? ? ? read_only =
>> ? ? ? ? ? ? ? ? ? ? ? mci_readl(slot->host, WRTPRT) & (1 << slot->id) ? 1 : 0;
>> @@ -837,6 +845,8 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
>> ? ? ? ? ? ? ? present = 1;
>> ? ? ? else if (brd->get_cd)
>> ? ? ? ? ? ? ? present = !brd->get_cd(slot->id);
>> + ? ? else if (gpio_is_valid(slot->cd_gpio))
>> + ? ? ? ? ? ? present = gpio_get_value(slot->cd_gpio);
>> ? ? ? else
>> ? ? ? ? ? ? ? present = (mci_readl(slot->host, CDETECT) & (1 << slot->id))
>> ? ? ? ? ? ? ? ? ? ? ? == 0 ? 1 : 0;
>> @@ -1747,6 +1757,96 @@ static void dw_mci_work_routine_card(struct
>> work_struct *work)
>> ? ? ? }
>> ?}
>>
>> +#ifdef CONFIG_OF
>> +static struct device_node *dw_mci_of_find_slot_node(struct device *dev, u8
>> slot)
>> +{
>> + ? ? struct device_node *np;
>> + ? ? char name[7];
>> +
>> + ? ? if (!dev || !dev->of_node)
>> + ? ? ? ? ? ? return NULL;
>> +
>> + ? ? for_each_child_of_node(dev->of_node, np) {
>> + ? ? ? ? ? ? sprintf(name, "slot%d", slot);
>> + ? ? ? ? ? ? if (!strcmp(name, np->name))
>> + ? ? ? ? ? ? ? ? ? ? return np;
>> + ? ? }
>> + ? ? return NULL;
>> +}
>> +
>> +static u32 dw_mci_of_get_bus_wd(struct device *dev, u8 slot)
>> +{
>> + ? ? struct device_node *np = dw_mci_of_find_slot_node(dev, slot);
>> + ? ? u32 bus_wd = 1;
>> +
>> + ? ? if (!np)
>> + ? ? ? ? ? ? return 1;
>> +
>> + ? ? if (of_property_read_u32(np, "bus-width", &bus_wd))
>> + ? ? ? ? ? ? dev_err(dev, "bus-width property not found, assuming width"
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? " as 1\n");
>> + ? ? return bus_wd;
>> +}
>> +
>> +static int dw_mci_of_setup_bus(struct dw_mci *host, u8 slot, u32 bus_wd)
>> +{
>> + ? ? struct device_node *np = dw_mci_of_find_slot_node(&host->dev, slot);
>> + ? ? int idx, gpio, ret;
>> +
>> + ? ? for (idx = 0; idx < NUM_PINS(bus_wd); idx++) {
>> + ? ? ? ? ? ? gpio = of_get_gpio(np, idx);
>> + ? ? ? ? ? ? if (!gpio_is_valid(gpio)) {
>> + ? ? ? ? ? ? ? ? ? ? dev_err(&host->dev, "invalid gpio: %d\n", gpio);
>> + ? ? ? ? ? ? ? ? ? ? return -EINVAL;
>> + ? ? ? ? ? ? }
>> +
>> + ? ? ? ? ? ? ret = devm_gpio_request(&host->dev, gpio, "dw-mci-bus");
>> + ? ? ? ? ? ? if (ret) {
>> + ? ? ? ? ? ? ? ? ? ? dev_err(&host->dev, "gpio [%d] request failed\n", gpio);
>> + ? ? ? ? ? ? ? ? ? ? return -EBUSY;
>> + ? ? ? ? ? ? }
>> + ? ? }
>> +
>> + ? ? host->slot[slot]->wp_gpio = -1;
>> + ? ? gpio = of_get_named_gpio(np, "wp_gpios", 0);
>> + ? ? if (!gpio_is_valid(gpio)) {
>> + ? ? ? ? ? ? dev_info(&host->dev, "wp gpio not available");
>> + ? ? } else {
>> + ? ? ? ? ? ? ret = devm_gpio_request(&host->dev, gpio, "dw-mci-wp");
>> + ? ? ? ? ? ? if (ret)
>> + ? ? ? ? ? ? ? ? ? ? dev_info(&host->dev, "gpio [%d] request failed\n",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? gpio);
>> + ? ? ? ? ? ? else
>> + ? ? ? ? ? ? ? ? ? ? host->slot[slot]->wp_gpio = gpio;
>> + ? ? }
>> +
>> + ? ? host->slot[slot]->cd_gpio = -1;
>> + ? ? gpio = of_get_named_gpio(np, "cd-gpios", 0);
>> + ? ? if (!gpio_is_valid(gpio)) {
>> + ? ? ? ? ? ? dev_info(&host->dev, "cd gpio not available");
>> + ? ? } else {
>> + ? ? ? ? ? ? ret = devm_gpio_request(&host->dev, gpio, "dw-mci-cd");
>> + ? ? ? ? ? ? if (ret)
>> + ? ? ? ? ? ? ? ? ? ? dev_err(&host->dev, "gpio [%d] request failed\n", gpio);
>> + ? ? ? ? ? ? else
>> + ? ? ? ? ? ? ? ? ? ? host->slot[slot]->cd_gpio = gpio;
>> + ? ? }
>> +
>> + ? ? return 0;
>> +}
>> +
>> +#else /* CONFIG_OF */
>> +static u32 dw_mci_of_get_bus_wd(struct device *dev, u8 slot)
>> +{
>> + ? ? return 1;
>> +}
>> +
>> +static int dw_mci_of_setup_bus(struct dw_mci *host, u8 slot, u32 bus_wd)
>> +{
>> + ? ? return -EINVAL;
>> +}
>> +#endif /* CONFIG_OF */
>> +
>> ?static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id)
>> ?{
>> ? ? ? struct mmc_host *mmc;
>> @@ -1760,6 +1860,7 @@ static int __init dw_mci_init_slot(struct dw_mci
>> *host, unsigned int id)
>> ? ? ? slot->id = id;
>> ? ? ? slot->mmc = mmc;
>> ? ? ? slot->host = host;
>> + ? ? host->slot[id] = slot;
>>
>> ? ? ? mmc->ops = &dw_mci_ops;
>> ? ? ? mmc->f_min = DIV_ROUND_UP(host->bus_hz, 510);
>> @@ -1780,12 +1881,21 @@ static int __init dw_mci_init_slot(struct dw_mci
>> *host, unsigned int id)
>> ? ? ? if (host->pdata->caps)
>> ? ? ? ? ? ? ? mmc->caps = host->pdata->caps;
>>
>> + ? ? mmc->caps |= host->drv_data->caps;
>> +
>> ? ? ? if (host->pdata->caps2)
>> ? ? ? ? ? ? ? mmc->caps2 = host->pdata->caps2;
>>
>> - ? ? if (host->pdata->get_bus_wd)
>> + ? ? if (host->pdata->get_bus_wd) {
>> ? ? ? ? ? ? ? if (host->pdata->get_bus_wd(slot->id) >= 4)
>> ? ? ? ? ? ? ? ? ? ? ? mmc->caps |= MMC_CAP_4_BIT_DATA;
>> + ? ? } else if (host->dev.of_node) {
>> + ? ? ? ? ? ? unsigned int bus_width;
>> + ? ? ? ? ? ? bus_width = dw_mci_of_get_bus_wd(&host->dev, slot->id);
>> + ? ? ? ? ? ? if (bus_width >= 4)
>> + ? ? ? ? ? ? ? ? ? ? mmc->caps |= MMC_CAP_4_BIT_DATA;
> In case of bus_width has 8; then how to handle it? maybe it's missing one.


Yes, this was not correct. I will fix this. Thanks for pointing this out.


>> + ? ? ? ? ? ? dw_mci_of_setup_bus(host, slot->id, bus_width);
>> + ? ? }
>>
>> ? ? ? if (host->pdata->quirks & DW_MCI_QUIRK_HIGHSPEED)
>> ? ? ? ? ? ? ? mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
>> @@ -1830,7 +1940,6 @@ static int __init dw_mci_init_slot(struct dw_mci
>> *host, unsigned int id)
>> ? ? ? else
>> ? ? ? ? ? ? ? clear_bit(DW_MMC_CARD_PRESENT, &slot->flags);
>>
>> - ? ? host->slot[id] = slot;
>> ? ? ? mmc_add_host(mmc);
>>
>> ?#if defined(CONFIG_DEBUG_FS)
>> @@ -1923,15 +2032,75 @@ static bool mci_wait_reset(struct device *dev,
>> struct dw_mci *host)
>> ? ? ? return false;
>> ?}
>>
>> +#ifdef CONFIG_OF
>> +static struct dw_mci_of_quirks {
>> + ? ? char *quirk;
>> + ? ? int id;
>> +} of_quriks[] = {
>> + ? ? {
>> + ? ? ? ? ? ? .quirk ?= "supports-highspeed",
>> + ? ? ? ? ? ? .id ? ? = DW_MCI_QUIRK_HIGHSPEED,
>> + ? ? }, {
>> + ? ? ? ? ? ? .quirk ?= "card-detection-broken",
>> + ? ? ? ? ? ? .id ? ? = DW_MCI_QUIRK_BROKEN_CARD_DETECTION,
>> + ? ? }, {
>> + ? ? ? ? ? ? .quirk ?= "no-write-protect",
>> + ? ? ? ? ? ? .id ? ? = DW_MCI_QUIRK_NO_WRITE_PROTECT,
>> + ? ? },
>> + ? ? { }
>> +};
>> +
>> +static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
>> +{
>> + ? ? struct dw_mci_board *pdata;
>> + ? ? struct device *dev = &host->dev;
>> + ? ? struct device_node *np = dev->of_node, *slot;
>> + ? ? u32 timing[3];
>> + ? ? int idx, cnt;
>> +
>> + ? ? pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> + ? ? if (!pdata) {
>> + ? ? ? ? ? ? dev_err(dev, "could not allocate memory for pdata\n");
>> + ? ? ? ? ? ? return ERR_PTR(-ENOMEM);
>> + ? ? }
>> +
>> + ? ? /* find out number of slots supported */
>> + ? ? for_each_child_of_node(np, slot)
>> + ? ? ? ? ? ? pdata->num_slots++;
>> +
>> + ? ? /* get quirks */
>> + ? ? cnt = sizeof(of_quriks) / sizeof(struct dw_mci_of_quirks);
>> + ? ? for (idx = 0; idx < cnt; idx++)
>> + ? ? ? ? ? ? if (of_get_property(np, of_quriks[idx].quirk, NULL))
>> + ? ? ? ? ? ? ? ? ? ? pdata->quirks |= of_quriks[idx].id;
>> +
>> + ? ? if (of_property_read_u32(np, "fifo-depth", &pdata->fifo_depth))
>> + ? ? ? ? ? ? dev_info(dev, "fifo-depth property not found, using "
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? "value of FIFOTH register as default\n");
>> +
>> + ? ? of_property_read_u32(np, "card-detect-delay", &pdata->detect_delay_ms);
>> +
>> + ? ? return pdata;
>> +}
>> +
>> +#else /* CONFIG_OF */
>> +static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
>> +{
>> + ? ? return ERR_PTR(-EINVAL);
>> +}
>> +#endif /* CONFIG_OF */
>> +
>> ?int dw_mci_probe(struct dw_mci *host)
>> ?{
>> ? ? ? int width, i, ret = 0;
>> ? ? ? u32 fifo_size;
>>
>> - ? ? if (!host->pdata || !host->pdata->init) {
>> - ? ? ? ? ? ? dev_err(&host->dev,
>> - ? ? ? ? ? ? ? ? ? ? "Platform data must supply init function\n");
>> - ? ? ? ? ? ? return -ENODEV;
>> + ? ? if (!host->pdata) {
>> + ? ? ? ? ? ? host->pdata = dw_mci_parse_dt(host);
>> + ? ? ? ? ? ? if (IS_ERR(host->pdata)) {
>> + ? ? ? ? ? ? ? ? ? ? dev_err(&host->dev, "platform data not available\n");
>> + ? ? ? ? ? ? ? ? ? ? return -EINVAL;
>> + ? ? ? ? ? ? }
>> ? ? ? }
>>
>> ? ? ? if (!host->pdata->select_slot && host->pdata->num_slots > 1) {
>> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
>> index 15c27e1..8b8862b 100644
>> --- a/drivers/mmc/host/dw_mmc.h
>> +++ b/drivers/mmc/host/dw_mmc.h
>> @@ -182,4 +182,14 @@ extern int dw_mci_suspend(struct dw_mci *host);
>> ?extern int dw_mci_resume(struct dw_mci *host);
>> ?#endif
>>
>> +/* Variations in the dw_mci controller */
>> +#define DW_MCI_TYPE_SYNOPSIS ? ? ? ? 0
>> +#define DW_MCI_TYPE_EXYNOS5250 ? ? ? ? ? ? ? 1 /* Samsung Exynos5250 Extensions */
> Um. it's not good idea to add specific SOC version. And as you know,
> exynos4 series has this controller.

There are some differences between the Exynos4 and Exynos5 mshc
controllers. For instance, the DIVRATIO field in the CLKSEL register
is available only in Exynos5 and there are 8 phase shift values in
Exynos5 when compared to 4 phase shift values in Exynos4. Likewise,
there are some other differences. So to handle these specific
implementations, we need to define types (or variants) of the
controller. Using SoC names for the type would help in readability of
the different types of implementations that are defined. So I would
prefer to continue using SoC names for this.

Thanks,
Thomas.

2012-05-10 09:59:55

by Kyungmin Park

[permalink] [raw]
Subject: Re: [PATCH 3/7] mmc: dw_mmc: add device tree support

On 5/10/12, Thomas Abraham <[email protected]> wrote:
> Dear Mr. Park,
>
> On 2 May 2012 12:25, Kyungmin Park <[email protected]> wrote:
>> Hi,
>>
>> On 5/2/12, Thomas Abraham <[email protected]> wrote:
>>> Add device tree based discovery support.
>>>
>>> Signed-off-by: Thomas Abraham <[email protected]>
>>> ---
>>> .../devicetree/bindings/mmc/synposis-dw-mshc.txt | 85 +++++++++
>>> drivers/mmc/host/dw_mmc-pltfm.c | 24 +++
>>> drivers/mmc/host/dw_mmc.c | 181
>>> +++++++++++++++++++-
>>> drivers/mmc/host/dw_mmc.h | 10 +
>>> include/linux/mmc/dw_mmc.h | 2 +
>>> 5 files changed, 296 insertions(+), 6 deletions(-)
>>> create mode 100644
>>> Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
>>> b/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
>>> new file mode 100644
>>> index 0000000..c1ed70e
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
>>> @@ -0,0 +1,85 @@
>>> +* Synopsis Designware Mobile Storage Host Controller
>>> +
>>> +The Synopsis designware mobile storage host controller is used to
>>> interface
>>> +a SoC with storage medium such as eMMC or SD/MMC cards.
>>> +
>>> +Required Properties:
>>> +
>>> +* compatible: should be one of the following
>>> + - synopsis,dw-mshc: for controllers compliant with synopsis
>>> dw-mshc.
>> I googled the "Synopsis Designware Mobile Storage Host Controller" and
>> Synopsis released it as this name. but still I like the 'dw-mmc'
>> instead of'dw-mshc'.
>
> Ok. Synopsis abbreviates this controller as MSHC in their datasheets
> available online. Since device tree is more about describing the
> hardware, using MSHC as the abbreviation will help with correlating
> hardware specs with the dts file. So I would prefer to continue using
> mshc as the abbreviation.

Then why author of this file uses "dw-mmc" instead of "dw-mshc"? and
it uses 'dw_mci' prefix at functions. I just worried and don't want to
give confusion to users who uses this device.

>
>>> +
>>> +* reg: physical base address of the dw-mshc controller and size of its
>>> memory
>>> + region.
>>> +
>>> +* interrupts: interrupt specifier for the controller. The format and
>>> value
>>> of
>>> + the interrupt specifier depends on the interrupt parent for the
>>> controller.
>>> +
>>> +# Slots: The slot specific information are contained within child-nodes
>>> with
>>> + each child-node representing a supported slot. There should be
>>> atleast
>>> one
>>> + child node representing a card slot. The name of the slot child node
>>> should
>>> + be 'slot{n}' where n is the unique number of the slot connnected to
>>> the
>>> + controller. The following are optional properties which can be
>>> included
>>> in
>>> + the slot child node.
>>> +
>>> + * bus-width: specifies the width of the data bus connected from
>>> the
>>> + controller to the card slot. The value should be 1, 4 or 8. In
>>> case
>>> + this property is not specified, a default value of 1 is assumed
>>> for
>>> + this property.
>>> +
>>> + * cd-gpios: specifies the card detect gpio line. The format of the
>>> + gpio specifier depends on the gpio controller.
>>> +
>>> + * wp-gpios: specifies the write protect gpio line. The format of
>>> the
>>> + gpio specifier depends on the gpio controller.
>>> +
>>> + * gpios: specifies a list of gpios used for command, clock and
>>> data
>>> + bus. The first gpio is the command line and the second gpio is
>>> the
>>> + clock line. The rest of the gpios (depending on the bus-width
>>> + property) are the data lines in no particular order. The format
>>> of
>>> + the gpio specifier depends on the gpio controller.
>>> +
>>> +Optional properties:
>>> +
>>> +* fifo-depth: The maximum size of the tx/rx fifo's. If this property is
>>> not
>>> + specified, the default value of the fifo size is determined from the
>>> + controller registers.
>>> +
>>> +* card-detect-delay: Delay in milli-seconds before detecting card
>>> after
>>> card
>>> + insert event. The default value is 0.
>>> +
>>> +* supports-highspeed: Enables support for high speed cards (upto 50MHz)
>>> +
>>> +* card-detection-broken: The card detection functionality is not
>>> available
>>> on
>>> + any of the slots.
>>> +
>>> +* no-write-protect: The write protect pad of the controller is not
>>> connected
>>> + to the write protect pin on the slot.
>>> +
>>> +Example:
>>> +
>>> + The MSHC controller node can be split into two portions, SoC specific
>>> and
>>> + board specific portions as listed below.
>>> +
>>> + dwmmc0@12200000 {
>>> + compatible = "synopsis,dw-mshc";
>>> + reg = <0x12200000 0x1000>;
>>> + interrupts = <0 75 0>;
>>> + };
>>> +
>>> + dwmmc0@12200000 {
>>> + supports-highspeed;
>>> + card-detection-broken;
>>> + no-write-protect;
>>> + fifo-depth = <0x80>;
>>> + card-detect-delay = <200>;
>>> +
>>> + slot0 {
>>> + bus-width = <8>;
>>> + cd-gpios = <&gpc0 2 2 3 3>;
>>> + gpios = <&gpc0 0 2 0 3>, <&gpc0 1 2 0 3>,
>>> + <&gpc1 0 2 3 3>, <&gpc1 1 2 3 3>,
>>> + <&gpc1 2 2 3 3>, <&gpc1 3 2 3 3>,
>>> + <&gpc0 3 2 3 3>, <&gpc0 4 2 3 3>,
>>> + <&gpc0 5 2 3 3>, <&gpc0 6 2 3 3>;
>>> + };
>>> + };
>>> diff --git a/drivers/mmc/host/dw_mmc-pltfm.c
>>> b/drivers/mmc/host/dw_mmc-pltfm.c
>>> index 92ec3eb..2b2c9bd 100644
>>> --- a/drivers/mmc/host/dw_mmc-pltfm.c
>>> +++ b/drivers/mmc/host/dw_mmc-pltfm.c
>>> @@ -19,8 +19,24 @@
>>> #include <linux/mmc/host.h>
>>> #include <linux/mmc/mmc.h>
>>> #include <linux/mmc/dw_mmc.h>
>>> +#include <linux/of.h>
>>> #include "dw_mmc.h"
>>>
>>> +#ifdef CONFIG_OF
>>> +static struct dw_mci_drv_data synopsis_drv_data = {
>>> + .ctrl_type = DW_MCI_TYPE_SYNOPSIS,
>>> +};
>>> +
>>> +static const struct of_device_id dw_mci_pltfm_match[] = {
>>> + { .compatible = "synopsis,dw-mshc",
>>> + .data = (void *)&synopsis_drv_data, },
>>> + {},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, dw_mci_pltfm_match);
>>> +#else
>>> +static const struct of_device_id dw_mci_pltfm_match[];
>>> +#endif
>>> +
>>> static int dw_mci_pltfm_probe(struct platform_device *pdev)
>>> {
>>> struct dw_mci *host;
>>> @@ -51,6 +67,13 @@ static int dw_mci_pltfm_probe(struct platform_device
>>> *pdev)
>>> if (!host->regs)
>>> goto err_free;
>>> platform_set_drvdata(pdev, host);
>>> +
>>> + if (pdev->dev.of_node) {
>>> + const struct of_device_id *match;
>>> + match = of_match_node(dw_mci_pltfm_match,
>>> pdev->dev.of_node);
>>> + host->drv_data = match->data;
>>> + }
>>> +
>>> ret = dw_mci_probe(host);
>>> if (ret)
>>> goto err_out;
>>> @@ -111,6 +134,7 @@ static struct platform_driver dw_mci_pltfm_driver =
>>> {
>>> .remove = __exit_p(dw_mci_pltfm_remove),
>>> .driver = {
>>> .name = "dw_mmc",
>>> + .of_match_table = of_match_ptr(dw_mci_pltfm_match),
>>> .pm = &dw_mci_pltfm_pmops,
>>> },
>>> };
>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>> index 38cb7f8..bcf66d7 100644
>>> --- a/drivers/mmc/host/dw_mmc.c
>>> +++ b/drivers/mmc/host/dw_mmc.c
>>> @@ -33,9 +33,13 @@
>>> #include <linux/bitops.h>
>>> #include <linux/regulator/consumer.h>
>>> #include <linux/workqueue.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_gpio.h>
>>>
>>> #include "dw_mmc.h"
>>>
>>> +#define NUM_PINS(x) (x + 2)
>>> +
>>> /* Common flag combinations */
>>> #define DW_MCI_DATA_ERROR_FLAGS (SDMMC_INT_DTO | SDMMC_INT_DCRC |
>>> \
>>> SDMMC_INT_HTO | SDMMC_INT_SBE | \
>>> @@ -86,6 +90,8 @@ struct idmac_desc {
>>> struct dw_mci_slot {
>>> struct mmc_host *mmc;
>>> struct dw_mci *host;
>>> + int wp_gpio;
>>> + int cd_gpio;
>>>
>>> u32 ctype;
>>>
>>> @@ -816,6 +822,8 @@ static int dw_mci_get_ro(struct mmc_host *mmc)
>>> read_only = 0;
>>> else if (brd->get_ro)
>>> read_only = brd->get_ro(slot->id);
>>> + else if (gpio_is_valid(slot->wp_gpio))
>>> + read_only = gpio_get_value(slot->wp_gpio);
>>> else
>>> read_only =
>>> mci_readl(slot->host, WRTPRT) & (1 << slot->id) ? 1
>>> : 0;
>>> @@ -837,6 +845,8 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
>>> present = 1;
>>> else if (brd->get_cd)
>>> present = !brd->get_cd(slot->id);
>>> + else if (gpio_is_valid(slot->cd_gpio))
>>> + present = gpio_get_value(slot->cd_gpio);
>>> else
>>> present = (mci_readl(slot->host, CDETECT) & (1 <<
>>> slot->id))
>>> == 0 ? 1 : 0;
>>> @@ -1747,6 +1757,96 @@ static void dw_mci_work_routine_card(struct
>>> work_struct *work)
>>> }
>>> }
>>>
>>> +#ifdef CONFIG_OF
>>> +static struct device_node *dw_mci_of_find_slot_node(struct device *dev,
>>> u8
>>> slot)
>>> +{
>>> + struct device_node *np;
>>> + char name[7];
>>> +
>>> + if (!dev || !dev->of_node)
>>> + return NULL;
>>> +
>>> + for_each_child_of_node(dev->of_node, np) {
>>> + sprintf(name, "slot%d", slot);
>>> + if (!strcmp(name, np->name))
>>> + return np;
>>> + }
>>> + return NULL;
>>> +}
>>> +
>>> +static u32 dw_mci_of_get_bus_wd(struct device *dev, u8 slot)
>>> +{
>>> + struct device_node *np = dw_mci_of_find_slot_node(dev, slot);
>>> + u32 bus_wd = 1;
>>> +
>>> + if (!np)
>>> + return 1;
>>> +
>>> + if (of_property_read_u32(np, "bus-width", &bus_wd))
>>> + dev_err(dev, "bus-width property not found, assuming
>>> width"
>>> + " as 1\n");
>>> + return bus_wd;
>>> +}
>>> +
>>> +static int dw_mci_of_setup_bus(struct dw_mci *host, u8 slot, u32
>>> bus_wd)
>>> +{
>>> + struct device_node *np = dw_mci_of_find_slot_node(&host->dev,
>>> slot);
>>> + int idx, gpio, ret;
>>> +
>>> + for (idx = 0; idx < NUM_PINS(bus_wd); idx++) {
>>> + gpio = of_get_gpio(np, idx);
>>> + if (!gpio_is_valid(gpio)) {
>>> + dev_err(&host->dev, "invalid gpio: %d\n", gpio);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + ret = devm_gpio_request(&host->dev, gpio, "dw-mci-bus");
>>> + if (ret) {
>>> + dev_err(&host->dev, "gpio [%d] request failed\n",
>>> gpio);
>>> + return -EBUSY;
>>> + }
>>> + }
>>> +
>>> + host->slot[slot]->wp_gpio = -1;
>>> + gpio = of_get_named_gpio(np, "wp_gpios", 0);
>>> + if (!gpio_is_valid(gpio)) {
>>> + dev_info(&host->dev, "wp gpio not available");
>>> + } else {
>>> + ret = devm_gpio_request(&host->dev, gpio, "dw-mci-wp");
>>> + if (ret)
>>> + dev_info(&host->dev, "gpio [%d] request failed\n",
>>> + gpio);
>>> + else
>>> + host->slot[slot]->wp_gpio = gpio;
>>> + }
>>> +
>>> + host->slot[slot]->cd_gpio = -1;
>>> + gpio = of_get_named_gpio(np, "cd-gpios", 0);
>>> + if (!gpio_is_valid(gpio)) {
>>> + dev_info(&host->dev, "cd gpio not available");
>>> + } else {
>>> + ret = devm_gpio_request(&host->dev, gpio, "dw-mci-cd");
>>> + if (ret)
>>> + dev_err(&host->dev, "gpio [%d] request failed\n",
>>> gpio);
>>> + else
>>> + host->slot[slot]->cd_gpio = gpio;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +#else /* CONFIG_OF */
>>> +static u32 dw_mci_of_get_bus_wd(struct device *dev, u8 slot)
>>> +{
>>> + return 1;
>>> +}
>>> +
>>> +static int dw_mci_of_setup_bus(struct dw_mci *host, u8 slot, u32
>>> bus_wd)
>>> +{
>>> + return -EINVAL;
>>> +}
>>> +#endif /* CONFIG_OF */
>>> +
>>> static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int
>>> id)
>>> {
>>> struct mmc_host *mmc;
>>> @@ -1760,6 +1860,7 @@ static int __init dw_mci_init_slot(struct dw_mci
>>> *host, unsigned int id)
>>> slot->id = id;
>>> slot->mmc = mmc;
>>> slot->host = host;
>>> + host->slot[id] = slot;
>>>
>>> mmc->ops = &dw_mci_ops;
>>> mmc->f_min = DIV_ROUND_UP(host->bus_hz, 510);
>>> @@ -1780,12 +1881,21 @@ static int __init dw_mci_init_slot(struct dw_mci
>>> *host, unsigned int id)
>>> if (host->pdata->caps)
>>> mmc->caps = host->pdata->caps;
>>>
>>> + mmc->caps |= host->drv_data->caps;
>>> +
>>> if (host->pdata->caps2)
>>> mmc->caps2 = host->pdata->caps2;
>>>
>>> - if (host->pdata->get_bus_wd)
>>> + if (host->pdata->get_bus_wd) {
>>> if (host->pdata->get_bus_wd(slot->id) >= 4)
>>> mmc->caps |= MMC_CAP_4_BIT_DATA;
>>> + } else if (host->dev.of_node) {
>>> + unsigned int bus_width;
>>> + bus_width = dw_mci_of_get_bus_wd(&host->dev, slot->id);
>>> + if (bus_width >= 4)
>>> + mmc->caps |= MMC_CAP_4_BIT_DATA;
>> In case of bus_width has 8; then how to handle it? maybe it's missing
>> one.
>
>
> Yes, this was not correct. I will fix this. Thanks for pointing this out.
>
>
>>> + dw_mci_of_setup_bus(host, slot->id, bus_width);
>>> + }
>>>
>>> if (host->pdata->quirks & DW_MCI_QUIRK_HIGHSPEED)
>>> mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
>>> @@ -1830,7 +1940,6 @@ static int __init dw_mci_init_slot(struct dw_mci
>>> *host, unsigned int id)
>>> else
>>> clear_bit(DW_MMC_CARD_PRESENT, &slot->flags);
>>>
>>> - host->slot[id] = slot;
>>> mmc_add_host(mmc);
>>>
>>> #if defined(CONFIG_DEBUG_FS)
>>> @@ -1923,15 +2032,75 @@ static bool mci_wait_reset(struct device *dev,
>>> struct dw_mci *host)
>>> return false;
>>> }
>>>
>>> +#ifdef CONFIG_OF
>>> +static struct dw_mci_of_quirks {
>>> + char *quirk;
>>> + int id;
>>> +} of_quriks[] = {
>>> + {
>>> + .quirk = "supports-highspeed",
>>> + .id = DW_MCI_QUIRK_HIGHSPEED,
>>> + }, {
>>> + .quirk = "card-detection-broken",
>>> + .id = DW_MCI_QUIRK_BROKEN_CARD_DETECTION,
>>> + }, {
>>> + .quirk = "no-write-protect",
>>> + .id = DW_MCI_QUIRK_NO_WRITE_PROTECT,
>>> + },
>>> + { }
>>> +};
>>> +
>>> +static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
>>> +{
>>> + struct dw_mci_board *pdata;
>>> + struct device *dev = &host->dev;
>>> + struct device_node *np = dev->of_node, *slot;
>>> + u32 timing[3];
>>> + int idx, cnt;
>>> +
>>> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>>> + if (!pdata) {
>>> + dev_err(dev, "could not allocate memory for pdata\n");
>>> + return ERR_PTR(-ENOMEM);
>>> + }
>>> +
>>> + /* find out number of slots supported */
>>> + for_each_child_of_node(np, slot)
>>> + pdata->num_slots++;
>>> +
>>> + /* get quirks */
>>> + cnt = sizeof(of_quriks) / sizeof(struct dw_mci_of_quirks);
>>> + for (idx = 0; idx < cnt; idx++)
>>> + if (of_get_property(np, of_quriks[idx].quirk, NULL))
>>> + pdata->quirks |= of_quriks[idx].id;
>>> +
>>> + if (of_property_read_u32(np, "fifo-depth", &pdata->fifo_depth))
>>> + dev_info(dev, "fifo-depth property not found, using "
>>> + "value of FIFOTH register as default\n");
>>> +
>>> + of_property_read_u32(np, "card-detect-delay",
>>> &pdata->detect_delay_ms);
>>> +
>>> + return pdata;
>>> +}
>>> +
>>> +#else /* CONFIG_OF */
>>> +static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
>>> +{
>>> + return ERR_PTR(-EINVAL);
>>> +}
>>> +#endif /* CONFIG_OF */
>>> +
>>> int dw_mci_probe(struct dw_mci *host)
>>> {
>>> int width, i, ret = 0;
>>> u32 fifo_size;
>>>
>>> - if (!host->pdata || !host->pdata->init) {
>>> - dev_err(&host->dev,
>>> - "Platform data must supply init function\n");
>>> - return -ENODEV;
>>> + if (!host->pdata) {
>>> + host->pdata = dw_mci_parse_dt(host);
>>> + if (IS_ERR(host->pdata)) {
>>> + dev_err(&host->dev, "platform data not
>>> available\n");
>>> + return -EINVAL;
>>> + }
>>> }
>>>
>>> if (!host->pdata->select_slot && host->pdata->num_slots > 1) {
>>> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
>>> index 15c27e1..8b8862b 100644
>>> --- a/drivers/mmc/host/dw_mmc.h
>>> +++ b/drivers/mmc/host/dw_mmc.h
>>> @@ -182,4 +182,14 @@ extern int dw_mci_suspend(struct dw_mci *host);
>>> extern int dw_mci_resume(struct dw_mci *host);
>>> #endif
>>>
>>> +/* Variations in the dw_mci controller */
>>> +#define DW_MCI_TYPE_SYNOPSIS 0
>>> +#define DW_MCI_TYPE_EXYNOS5250 1 /* Samsung Exynos5250
>>> Extensions */
>> Um. it's not good idea to add specific SOC version. And as you know,
>> exynos4 series has this controller.
>
> There are some differences between the Exynos4 and Exynos5 mshc
> controllers. For instance, the DIVRATIO field in the CLKSEL register
> is available only in Exynos5 and there are 8 phase shift values in
> Exynos5 when compared to 4 phase shift values in Exynos4. Likewise,
> there are some other differences. So to handle these specific
> implementations, we need to define types (or variants) of the
> controller. Using SoC names for the type would help in readability of
> the different types of implementations that are defined. So I would
> prefer to continue using SoC names for this.

You mean if it supports the exynos4, then it should be add exynos4 type?

Thank you,
Kyungmin Park
>
> Thanks,
> Thomas.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc"
> in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

2012-05-10 10:12:51

by Thomas Abraham

[permalink] [raw]
Subject: Re: [PATCH 3/7] mmc: dw_mmc: add device tree support

On 10 May 2012 15:29, Kyungmin Park <[email protected]> wrote:
> On 5/10/12, Thomas Abraham <[email protected]> wrote:
>> Dear Mr. Park,
>>
>> On 2 May 2012 12:25, Kyungmin Park <[email protected]> wrote:
>>> Hi,

[...]

>>> I googled the "Synopsis Designware Mobile Storage Host Controller" and
>>> Synopsis released it as this name. but still I like the 'dw-mmc'
>>> instead of'dw-mshc'.
>>
>> Ok. Synopsis abbreviates this controller as MSHC in their datasheets
>> available online. Since device tree is more about describing the
>> hardware, using MSHC as the abbreviation will help with correlating
>> hardware specs with the dts file. So I would prefer to continue using
>> mshc as the abbreviation.
>
> Then why author of this file uses "dw-mmc" instead of "dw-mshc"? and
> it uses 'dw_mci' prefix at functions. I just worried and don't want to
> give confusion to users who uses this device.

The device tree source files are independent of any OS specific
implementations. The linux driver for this controller uses dw-mmc and
dw_mci but drivers of this controller for other operating systems
could choose use other names. Since Synopsis calls this controller as
mshc, the dts file would be closer to hardware specs if 'mshc' is used
to describe the controller.

[...]

>>>> +/* Variations in the dw_mci controller */
>>>> +#define DW_MCI_TYPE_SYNOPSIS ? ? ? ? 0
>>>> +#define DW_MCI_TYPE_EXYNOS5250 ? ? ? ? ? ? ? 1 /* Samsung Exynos5250
>>>> Extensions */
>>> Um. it's not good idea to add specific SOC version. And as you know,
>>> exynos4 series has this controller.
>>
>> There are some differences between the Exynos4 and Exynos5 mshc
>> controllers. For instance, the DIVRATIO field in the CLKSEL register
>> is available only in Exynos5 and there are 8 phase shift values in
>> Exynos5 when compared to 4 phase shift values in Exynos4. Likewise,
>> there are some other differences. So to handle these specific
>> implementations, we need to define types (or variants) of the
>> controller. Using SoC names for the type would help in readability of
>> the different types of implementations that are defined. So I would
>> prefer to continue using SoC names for this.
>
> You mean if it supports the exynos4, then it should be add exynos4 type?
>

If the existing driver requires any Exynos4 specific extensions to
operate the controller correctly on Exynos4, then yes we should add a
Exynos4 type. The Exynos4 and Exynos5 implementations of this
controller vary in certain aspects and so we might need a Exynos4 type
as well.

Thanks,
Thomas.

[...]

2012-05-10 10:15:46

by Thomas Abraham

[permalink] [raw]
Subject: Re: [PATCH 3/7] mmc: dw_mmc: add device tree support

Hi Olof,

On 2 May 2012 23:37, Olof Johansson <[email protected]> wrote:
> Hi,

[...]

>> +# Slots: The slot specific information are contained within child-nodes with
>> + ?each child-node representing a supported slot. There should be atleast one
>> + ?child node representing a card slot. The name of the slot child node should
>> + ?be 'slot{n}' where n is the unique number of the slot connnected to the
>> + ?controller. The following are optional properties which can be included in
>> + ?the slot child node.
>
> Since we're talking slots / cards on a bus, I think the addressing
> model would be useful here. So in the main controller node:
> ? ?#address-cells = <1>;
> ? ?#size-cells = <0>;
>
> And then each slot would need a reg property and possibly unit address:
>
> ? slot {
> ? ? ? ?reg = <0>;
> ? ? ? ?...
> ? };
>
> (unit addresses on the slots are only needed if they can't be
> disambiguated by name, so not needed if you only have one slot).
>

Is the addressing model as described above needed in this case? The
address for a slot is not used by the controller driver code and is
just a virtual number. It would be sufficient to represent the nodes
representing the slots with a unique name.

Thanks,
Thomas.

2012-05-10 10:32:32

by James Hogan

[permalink] [raw]
Subject: Re: [PATCH 3/7] mmc: dw_mmc: add device tree support

Hi

On 2 May 2012 06:07, Thomas Abraham <[email protected]> wrote:
> Add device tree based discovery support.
>
> Signed-off-by: Thomas Abraham <[email protected]>
> ---
> ?.../devicetree/bindings/mmc/synposis-dw-mshc.txt ? | ? 85 +++++++++
> ?drivers/mmc/host/dw_mmc-pltfm.c ? ? ? ? ? ? ? ? ? ?| ? 24 +++
> ?drivers/mmc/host/dw_mmc.c ? ? ? ? ? ? ? ? ? ? ? ? ?| ?181 +++++++++++++++++++-
> ?drivers/mmc/host/dw_mmc.h ? ? ? ? ? ? ? ? ? ? ? ? ?| ? 10 +
> ?include/linux/mmc/dw_mmc.h ? ? ? ? ? ? ? ? ? ? ? ? | ? ?2 +
> ?5 files changed, 296 insertions(+), 6 deletions(-)
> ?create mode 100644 Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
>
> diff --git a/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
> new file mode 100644
> index 0000000..c1ed70e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
> @@ -0,0 +1,85 @@
> +* Synopsis Designware Mobile Storage Host Controller
> +
> +The Synopsis designware mobile storage host controller is used to interface
> +a SoC with storage medium such as eMMC or SD/MMC cards.
> +
> +Required Properties:
> +
> +* compatible: should be one of the following
> + ? ? ? - synopsis,dw-mshc: for controllers compliant with synopsis dw-mshc.

Note that although undocumented in
Documentation/devicetree/bindings/vendor-prefixes.txt (which it
probably should be), the designware uart already uses a different
prefix:
{ .compatible = "snps,dw-apb-uart" },
http://lxr.linux.no/#linux+v3.3.5/drivers/tty/serial/8250/8250_dw.c#L165

Cheers
James

> +
> +* reg: physical base address of the dw-mshc controller and size of its memory
> + ?region.
> +
> +* interrupts: interrupt specifier for the controller. The format and value of
> + ?the interrupt specifier depends on the interrupt parent for the controller.
> +
> +# Slots: The slot specific information are contained within child-nodes with
> + ?each child-node representing a supported slot. There should be atleast one
> + ?child node representing a card slot. The name of the slot child node should
> + ?be 'slot{n}' where n is the unique number of the slot connnected to the
> + ?controller. The following are optional properties which can be included in
> + ?the slot child node.
> +
> + ? ? ? * bus-width: specifies the width of the data bus connected from the
> + ? ? ? ? controller to the card slot. The value should be 1, 4 or 8. In case
> + ? ? ? ? this property is not specified, a default value of 1 is assumed for
> + ? ? ? ? this property.
> +
> + ? ? ? * cd-gpios: specifies the card detect gpio line. The format of the
> + ? ? ? ? gpio specifier depends on the gpio controller.
> +
> + ? ? ? * wp-gpios: specifies the write protect gpio line. The format of the
> + ? ? ? ? gpio specifier depends on the gpio controller.
> +
> + ? ? ? * gpios: specifies a list of gpios used for command, clock and data
> + ? ? ? ? bus. The first gpio is the command line and the second gpio is the
> + ? ? ? ? clock line. The rest of the gpios (depending on the bus-width
> + ? ? ? ? property) are the data lines in no particular order. The format of
> + ? ? ? ? the gpio specifier depends on the gpio controller.
> +
> +Optional properties:
> +
> +* fifo-depth: The maximum size of the tx/rx fifo's. If this property is not
> + ?specified, the default value of the fifo size is determined from the
> + ?controller registers.
> +
> +* ?card-detect-delay: Delay in milli-seconds before detecting card after card
> + ? insert event. The default value is 0.
> +
> +* supports-highspeed: Enables support for high speed cards (upto 50MHz)
> +
> +* card-detection-broken: The card detection functionality is not available on
> + ?any of the slots.
> +
> +* no-write-protect: The write protect pad of the controller is not connected
> + ?to the write protect pin on the slot.
> +
> +Example:
> +
> + ?The MSHC controller node can be split into two portions, SoC specific and
> + ?board specific portions as listed below.
> +
> + ? ? ? dwmmc0@12200000 {
> + ? ? ? ? ? ? ? compatible = "synopsis,dw-mshc";
> + ? ? ? ? ? ? ? reg = <0x12200000 0x1000>;
> + ? ? ? ? ? ? ? interrupts = <0 75 0>;
> + ? ? ? };
> +
> + ? ? ? dwmmc0@12200000 {
> + ? ? ? ? ? ? ? supports-highspeed;
> + ? ? ? ? ? ? ? card-detection-broken;
> + ? ? ? ? ? ? ? no-write-protect;
> + ? ? ? ? ? ? ? fifo-depth = <0x80>;
> + ? ? ? ? ? ? ? card-detect-delay = <200>;
> +
> + ? ? ? ? ? ? ? slot0 {
> + ? ? ? ? ? ? ? ? ? ? ? bus-width = <8>;
> + ? ? ? ? ? ? ? ? ? ? ? cd-gpios = <&gpc0 2 2 3 3>;
> + ? ? ? ? ? ? ? ? ? ? ? gpios = <&gpc0 0 2 0 3>, <&gpc0 1 2 0 3>,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? <&gpc1 0 2 3 3>, <&gpc1 1 2 3 3>,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? <&gpc1 2 2 3 3>, <&gpc1 3 2 3 3>,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? <&gpc0 3 2 3 3>, <&gpc0 4 2 3 3>,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? <&gpc0 5 2 3 3>, <&gpc0 6 2 3 3>;
> + ? ? ? ? ? ? ? };
> + ? ? ? };
> diff --git a/drivers/mmc/host/dw_mmc-pltfm.c b/drivers/mmc/host/dw_mmc-pltfm.c
> index 92ec3eb..2b2c9bd 100644
> --- a/drivers/mmc/host/dw_mmc-pltfm.c
> +++ b/drivers/mmc/host/dw_mmc-pltfm.c
> @@ -19,8 +19,24 @@
> ?#include <linux/mmc/host.h>
> ?#include <linux/mmc/mmc.h>
> ?#include <linux/mmc/dw_mmc.h>
> +#include <linux/of.h>
> ?#include "dw_mmc.h"
>
> +#ifdef CONFIG_OF
> +static struct dw_mci_drv_data synopsis_drv_data = {
> + ? ? ? .ctrl_type ? ? ?= DW_MCI_TYPE_SYNOPSIS,
> +};
> +
> +static const struct of_device_id dw_mci_pltfm_match[] = {
> + ? ? ? { .compatible = "synopsis,dw-mshc",
> + ? ? ? ? ? ? ? ? ? ? ? .data = (void *)&synopsis_drv_data, },
> + ? ? ? {},
> +};
> +MODULE_DEVICE_TABLE(of, dw_mci_pltfm_match);
> +#else
> +static const struct of_device_id dw_mci_pltfm_match[];
> +#endif
> +
> ?static int dw_mci_pltfm_probe(struct platform_device *pdev)
> ?{
> ? ? ? ?struct dw_mci *host;
> @@ -51,6 +67,13 @@ static int dw_mci_pltfm_probe(struct platform_device *pdev)
> ? ? ? ?if (!host->regs)
> ? ? ? ? ? ? ? ?goto err_free;
> ? ? ? ?platform_set_drvdata(pdev, host);
> +
> + ? ? ? if (pdev->dev.of_node) {
> + ? ? ? ? ? ? ? const struct of_device_id *match;
> + ? ? ? ? ? ? ? match = of_match_node(dw_mci_pltfm_match, pdev->dev.of_node);
> + ? ? ? ? ? ? ? host->drv_data = match->data;
> + ? ? ? }
> +
> ? ? ? ?ret = dw_mci_probe(host);
> ? ? ? ?if (ret)
> ? ? ? ? ? ? ? ?goto err_out;
> @@ -111,6 +134,7 @@ static struct platform_driver dw_mci_pltfm_driver = {
> ? ? ? ?.remove ? ? ? ? = __exit_p(dw_mci_pltfm_remove),
> ? ? ? ?.driver ? ? ? ? = {
> ? ? ? ? ? ? ? ?.name ? ? ? ? ? = "dw_mmc",
> + ? ? ? ? ? ? ? .of_match_table = of_match_ptr(dw_mci_pltfm_match),
> ? ? ? ? ? ? ? ?.pm ? ? ? ? ? ? = &dw_mci_pltfm_pmops,
> ? ? ? ?},
> ?};
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 38cb7f8..bcf66d7 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -33,9 +33,13 @@
> ?#include <linux/bitops.h>
> ?#include <linux/regulator/consumer.h>
> ?#include <linux/workqueue.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
>
> ?#include "dw_mmc.h"
>
> +#define NUM_PINS(x) (x + 2)
> +
> ?/* Common flag combinations */
> ?#define DW_MCI_DATA_ERROR_FLAGS ? ? ? ?(SDMMC_INT_DTO | SDMMC_INT_DCRC | \
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? SDMMC_INT_HTO | SDMMC_INT_SBE ?| \
> @@ -86,6 +90,8 @@ struct idmac_desc {
> ?struct dw_mci_slot {
> ? ? ? ?struct mmc_host ? ? ? ? *mmc;
> ? ? ? ?struct dw_mci ? ? ? ? ? *host;
> + ? ? ? int ? ? ? ? ? ? ? ? ? ? wp_gpio;
> + ? ? ? int ? ? ? ? ? ? ? ? ? ? cd_gpio;
>
> ? ? ? ?u32 ? ? ? ? ? ? ? ? ? ? ctype;
>
> @@ -816,6 +822,8 @@ static int dw_mci_get_ro(struct mmc_host *mmc)
> ? ? ? ? ? ? ? ?read_only = 0;
> ? ? ? ?else if (brd->get_ro)
> ? ? ? ? ? ? ? ?read_only = brd->get_ro(slot->id);
> + ? ? ? else if (gpio_is_valid(slot->wp_gpio))
> + ? ? ? ? ? ? ? read_only = gpio_get_value(slot->wp_gpio);
> ? ? ? ?else
> ? ? ? ? ? ? ? ?read_only =
> ? ? ? ? ? ? ? ? ? ? ? ?mci_readl(slot->host, WRTPRT) & (1 << slot->id) ? 1 : 0;
> @@ -837,6 +845,8 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
> ? ? ? ? ? ? ? ?present = 1;
> ? ? ? ?else if (brd->get_cd)
> ? ? ? ? ? ? ? ?present = !brd->get_cd(slot->id);
> + ? ? ? else if (gpio_is_valid(slot->cd_gpio))
> + ? ? ? ? ? ? ? present = gpio_get_value(slot->cd_gpio);
> ? ? ? ?else
> ? ? ? ? ? ? ? ?present = (mci_readl(slot->host, CDETECT) & (1 << slot->id))
> ? ? ? ? ? ? ? ? ? ? ? ?== 0 ? 1 : 0;
> @@ -1747,6 +1757,96 @@ static void dw_mci_work_routine_card(struct work_struct *work)
> ? ? ? ?}
> ?}
>
> +#ifdef CONFIG_OF
> +static struct device_node *dw_mci_of_find_slot_node(struct device *dev, u8 slot)
> +{
> + ? ? ? struct device_node *np;
> + ? ? ? char name[7];
> +
> + ? ? ? if (!dev || !dev->of_node)
> + ? ? ? ? ? ? ? return NULL;
> +
> + ? ? ? for_each_child_of_node(dev->of_node, np) {
> + ? ? ? ? ? ? ? sprintf(name, "slot%d", slot);
> + ? ? ? ? ? ? ? if (!strcmp(name, np->name))
> + ? ? ? ? ? ? ? ? ? ? ? return np;
> + ? ? ? }
> + ? ? ? return NULL;
> +}
> +
> +static u32 dw_mci_of_get_bus_wd(struct device *dev, u8 slot)
> +{
> + ? ? ? struct device_node *np = dw_mci_of_find_slot_node(dev, slot);
> + ? ? ? u32 bus_wd = 1;
> +
> + ? ? ? if (!np)
> + ? ? ? ? ? ? ? return 1;
> +
> + ? ? ? if (of_property_read_u32(np, "bus-width", &bus_wd))
> + ? ? ? ? ? ? ? dev_err(dev, "bus-width property not found, assuming width"
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? " as 1\n");
> + ? ? ? return bus_wd;
> +}
> +
> +static int dw_mci_of_setup_bus(struct dw_mci *host, u8 slot, u32 bus_wd)
> +{
> + ? ? ? struct device_node *np = dw_mci_of_find_slot_node(&host->dev, slot);
> + ? ? ? int idx, gpio, ret;
> +
> + ? ? ? for (idx = 0; idx < NUM_PINS(bus_wd); idx++) {
> + ? ? ? ? ? ? ? gpio = of_get_gpio(np, idx);
> + ? ? ? ? ? ? ? if (!gpio_is_valid(gpio)) {
> + ? ? ? ? ? ? ? ? ? ? ? dev_err(&host->dev, "invalid gpio: %d\n", gpio);
> + ? ? ? ? ? ? ? ? ? ? ? return -EINVAL;
> + ? ? ? ? ? ? ? }
> +
> + ? ? ? ? ? ? ? ret = devm_gpio_request(&host->dev, gpio, "dw-mci-bus");
> + ? ? ? ? ? ? ? if (ret) {
> + ? ? ? ? ? ? ? ? ? ? ? dev_err(&host->dev, "gpio [%d] request failed\n", gpio);
> + ? ? ? ? ? ? ? ? ? ? ? return -EBUSY;
> + ? ? ? ? ? ? ? }
> + ? ? ? }
> +
> + ? ? ? host->slot[slot]->wp_gpio = -1;
> + ? ? ? gpio = of_get_named_gpio(np, "wp_gpios", 0);
> + ? ? ? if (!gpio_is_valid(gpio)) {
> + ? ? ? ? ? ? ? dev_info(&host->dev, "wp gpio not available");
> + ? ? ? } else {
> + ? ? ? ? ? ? ? ret = devm_gpio_request(&host->dev, gpio, "dw-mci-wp");
> + ? ? ? ? ? ? ? if (ret)
> + ? ? ? ? ? ? ? ? ? ? ? dev_info(&host->dev, "gpio [%d] request failed\n",
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? gpio);
> + ? ? ? ? ? ? ? else
> + ? ? ? ? ? ? ? ? ? ? ? host->slot[slot]->wp_gpio = gpio;
> + ? ? ? }
> +
> + ? ? ? host->slot[slot]->cd_gpio = -1;
> + ? ? ? gpio = of_get_named_gpio(np, "cd-gpios", 0);
> + ? ? ? if (!gpio_is_valid(gpio)) {
> + ? ? ? ? ? ? ? dev_info(&host->dev, "cd gpio not available");
> + ? ? ? } else {
> + ? ? ? ? ? ? ? ret = devm_gpio_request(&host->dev, gpio, "dw-mci-cd");
> + ? ? ? ? ? ? ? if (ret)
> + ? ? ? ? ? ? ? ? ? ? ? dev_err(&host->dev, "gpio [%d] request failed\n", gpio);
> + ? ? ? ? ? ? ? else
> + ? ? ? ? ? ? ? ? ? ? ? host->slot[slot]->cd_gpio = gpio;
> + ? ? ? }
> +
> + ? ? ? return 0;
> +}
> +
> +#else /* CONFIG_OF */
> +static u32 dw_mci_of_get_bus_wd(struct device *dev, u8 slot)
> +{
> + ? ? ? return 1;
> +}
> +
> +static int dw_mci_of_setup_bus(struct dw_mci *host, u8 slot, u32 bus_wd)
> +{
> + ? ? ? return -EINVAL;
> +}
> +#endif /* CONFIG_OF */
> +
> ?static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id)
> ?{
> ? ? ? ?struct mmc_host *mmc;
> @@ -1760,6 +1860,7 @@ static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id)
> ? ? ? ?slot->id = id;
> ? ? ? ?slot->mmc = mmc;
> ? ? ? ?slot->host = host;
> + ? ? ? host->slot[id] = slot;
>
> ? ? ? ?mmc->ops = &dw_mci_ops;
> ? ? ? ?mmc->f_min = DIV_ROUND_UP(host->bus_hz, 510);
> @@ -1780,12 +1881,21 @@ static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id)
> ? ? ? ?if (host->pdata->caps)
> ? ? ? ? ? ? ? ?mmc->caps = host->pdata->caps;
>
> + ? ? ? mmc->caps |= host->drv_data->caps;
> +
> ? ? ? ?if (host->pdata->caps2)
> ? ? ? ? ? ? ? ?mmc->caps2 = host->pdata->caps2;
>
> - ? ? ? if (host->pdata->get_bus_wd)
> + ? ? ? if (host->pdata->get_bus_wd) {
> ? ? ? ? ? ? ? ?if (host->pdata->get_bus_wd(slot->id) >= 4)
> ? ? ? ? ? ? ? ? ? ? ? ?mmc->caps |= MMC_CAP_4_BIT_DATA;
> + ? ? ? } else if (host->dev.of_node) {
> + ? ? ? ? ? ? ? unsigned int bus_width;
> + ? ? ? ? ? ? ? bus_width = dw_mci_of_get_bus_wd(&host->dev, slot->id);
> + ? ? ? ? ? ? ? if (bus_width >= 4)
> + ? ? ? ? ? ? ? ? ? ? ? mmc->caps |= MMC_CAP_4_BIT_DATA;
> + ? ? ? ? ? ? ? dw_mci_of_setup_bus(host, slot->id, bus_width);
> + ? ? ? }
>
> ? ? ? ?if (host->pdata->quirks & DW_MCI_QUIRK_HIGHSPEED)
> ? ? ? ? ? ? ? ?mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
> @@ -1830,7 +1940,6 @@ static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id)
> ? ? ? ?else
> ? ? ? ? ? ? ? ?clear_bit(DW_MMC_CARD_PRESENT, &slot->flags);
>
> - ? ? ? host->slot[id] = slot;
> ? ? ? ?mmc_add_host(mmc);
>
> ?#if defined(CONFIG_DEBUG_FS)
> @@ -1923,15 +2032,75 @@ static bool mci_wait_reset(struct device *dev, struct dw_mci *host)
> ? ? ? ?return false;
> ?}
>
> +#ifdef CONFIG_OF
> +static struct dw_mci_of_quirks {
> + ? ? ? char *quirk;
> + ? ? ? int id;
> +} of_quriks[] = {
> + ? ? ? {
> + ? ? ? ? ? ? ? .quirk ?= "supports-highspeed",
> + ? ? ? ? ? ? ? .id ? ? = DW_MCI_QUIRK_HIGHSPEED,
> + ? ? ? }, {
> + ? ? ? ? ? ? ? .quirk ?= "card-detection-broken",
> + ? ? ? ? ? ? ? .id ? ? = DW_MCI_QUIRK_BROKEN_CARD_DETECTION,
> + ? ? ? }, {
> + ? ? ? ? ? ? ? .quirk ?= "no-write-protect",
> + ? ? ? ? ? ? ? .id ? ? = DW_MCI_QUIRK_NO_WRITE_PROTECT,
> + ? ? ? },
> + ? ? ? { }
> +};
> +
> +static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
> +{
> + ? ? ? struct dw_mci_board *pdata;
> + ? ? ? struct device *dev = &host->dev;
> + ? ? ? struct device_node *np = dev->of_node, *slot;
> + ? ? ? u32 timing[3];
> + ? ? ? int idx, cnt;
> +
> + ? ? ? pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> + ? ? ? if (!pdata) {
> + ? ? ? ? ? ? ? dev_err(dev, "could not allocate memory for pdata\n");
> + ? ? ? ? ? ? ? return ERR_PTR(-ENOMEM);
> + ? ? ? }
> +
> + ? ? ? /* find out number of slots supported */
> + ? ? ? for_each_child_of_node(np, slot)
> + ? ? ? ? ? ? ? pdata->num_slots++;
> +
> + ? ? ? /* get quirks */
> + ? ? ? cnt = sizeof(of_quriks) / sizeof(struct dw_mci_of_quirks);
> + ? ? ? for (idx = 0; idx < cnt; idx++)
> + ? ? ? ? ? ? ? if (of_get_property(np, of_quriks[idx].quirk, NULL))
> + ? ? ? ? ? ? ? ? ? ? ? pdata->quirks |= of_quriks[idx].id;
> +
> + ? ? ? if (of_property_read_u32(np, "fifo-depth", &pdata->fifo_depth))
> + ? ? ? ? ? ? ? dev_info(dev, "fifo-depth property not found, using "
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "value of FIFOTH register as default\n");
> +
> + ? ? ? of_property_read_u32(np, "card-detect-delay", &pdata->detect_delay_ms);
> +
> + ? ? ? return pdata;
> +}
> +
> +#else /* CONFIG_OF */
> +static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
> +{
> + ? ? ? return ERR_PTR(-EINVAL);
> +}
> +#endif /* CONFIG_OF */
> +
> ?int dw_mci_probe(struct dw_mci *host)
> ?{
> ? ? ? ?int width, i, ret = 0;
> ? ? ? ?u32 fifo_size;
>
> - ? ? ? if (!host->pdata || !host->pdata->init) {
> - ? ? ? ? ? ? ? dev_err(&host->dev,
> - ? ? ? ? ? ? ? ? ? ? ? "Platform data must supply init function\n");
> - ? ? ? ? ? ? ? return -ENODEV;
> + ? ? ? if (!host->pdata) {
> + ? ? ? ? ? ? ? host->pdata = dw_mci_parse_dt(host);
> + ? ? ? ? ? ? ? if (IS_ERR(host->pdata)) {
> + ? ? ? ? ? ? ? ? ? ? ? dev_err(&host->dev, "platform data not available\n");
> + ? ? ? ? ? ? ? ? ? ? ? return -EINVAL;
> + ? ? ? ? ? ? ? }
> ? ? ? ?}
>
> ? ? ? ?if (!host->pdata->select_slot && host->pdata->num_slots > 1) {
> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
> index 15c27e1..8b8862b 100644
> --- a/drivers/mmc/host/dw_mmc.h
> +++ b/drivers/mmc/host/dw_mmc.h
> @@ -182,4 +182,14 @@ extern int dw_mci_suspend(struct dw_mci *host);
> ?extern int dw_mci_resume(struct dw_mci *host);
> ?#endif
>
> +/* Variations in the dw_mci controller */
> +#define DW_MCI_TYPE_SYNOPSIS ? ? ? ? ? 0
> +#define DW_MCI_TYPE_EXYNOS5250 ? ? ? ? 1 /* Samsung Exynos5250 Extensions */
> +
> +/* dw_mci platform driver data */
> +struct dw_mci_drv_data {
> + ? ? ? unsigned long ? ? ? ? ? ctrl_type;
> + ? ? ? unsigned long ? ? ? ? ? caps;
> +};
> +
> ?#endif /* _DW_MMC_H_ */
> diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
> index fb51a5f..71d2b56 100644
> --- a/include/linux/mmc/dw_mmc.h
> +++ b/include/linux/mmc/dw_mmc.h
> @@ -78,6 +78,7 @@ struct mmc_data;
> ?* @data_offset: Set the offset of DATA register according to VERID.
> ?* @dev: Device associated with the MMC controller.
> ?* @pdata: Platform data associated with the MMC controller.
> + * @drv_data: Driver specific data for identified variant of the controller
> ?* @biu_clk: Pointer to bus interface unit clock instance.
> ?* @ciu_clk: Pointer to card interface unit clock instance.
> ?* @slot: Slots sharing this MMC controller.
> @@ -160,6 +161,7 @@ struct dw_mci {
> ? ? ? ?u16 ? ? ? ? ? ? ? ? ? ? data_offset;
> ? ? ? ?struct device ? ? ? ? ? dev;
> ? ? ? ?struct dw_mci_board ? ? *pdata;
> + ? ? ? struct dw_mci_drv_data ?*drv_data;
> ? ? ? ?struct clk ? ? ? ? ? ? ?*biu_clk;
> ? ? ? ?struct clk ? ? ? ? ? ? ?*ciu_clk;
> ? ? ? ?struct dw_mci_slot ? ? ?*slot[MAX_MCI_SLOTS];
> --
> 1.7.5.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html



--
James Hogan

2012-05-10 10:38:32

by Thomas Abraham

[permalink] [raw]
Subject: Re: [PATCH 4/7] mmc: dw_mmc: add samsung exynos5250 specific extentions

Dear Mr. Park,

On 2 May 2012 12:31, Kyungmin Park <[email protected]> wrote:
> Hi,
>
> On 5/2/12, Thomas Abraham <[email protected]> wrote:
>> The instantiation of the Synopsis Designware controller on Exynos5250
>> include extension for SDR and DDR specific tx/rx phase shift timing
>> and CIU internal divider. In addition to that, the option to skip the
>> command hold stage is also introduced. Add support for these Exynos5250
>> specfic extenstions.
>>

[...]

>> +static struct dw_mci_drv_data exynos5250_drv_data = {
>> + ? ? .ctrl_type ? ? ?= DW_MCI_TYPE_EXYNOS5250,
>> + ? ? .caps ? ? ? ? ? = MMC_CAP_UHS_DDR50 | MMC_CAP_1_8V_DDR |
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? MMC_CAP_8_BIT_DATA | MMC_CAP_CMD23,
> These caps should be board specific. So it's not proper place. Esp.,
> MMC_CAP_8_BIT_DATA.

Yes, this is incorrect. I will fix this. Thanks for pointing this out.

>> +};
>> +
>> ?static const struct of_device_id dw_mci_pltfm_match[] = {
>> ? ? ? { .compatible = "synopsis,dw-mshc",
>> ? ? ? ? ? ? ? ? ? ? ? .data = (void *)&synopsis_drv_data, },
>> + ? ? { .compatible = "synopsis,dw-mshc-exynos5250",
>> + ? ? ? ? ? ? ? ? ? ? .data = (void *)&exynos5250_drv_data, },
>> ? ? ? {},
>> ?};
>> ?MODULE_DEVICE_TABLE(of, dw_mci_pltfm_match);
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index bcf66d7..9174a69 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -236,6 +236,7 @@ static void dw_mci_set_timeout(struct dw_mci *host)
>> ?static u32 dw_mci_prepare_command(struct mmc_host *mmc, struct mmc_command
>> *cmd)
>> ?{
>> ? ? ? struct mmc_data *data;
>> + ? ? struct dw_mci_slot *slot = mmc_priv(mmc);
>> ? ? ? u32 cmdr;
>> ? ? ? cmd->error = -EINPROGRESS;
>>
>> @@ -265,6 +266,10 @@ static u32 dw_mci_prepare_command(struct mmc_host *mmc,
>> struct mmc_command *cmd)
>> ? ? ? ? ? ? ? ? ? ? ? cmdr |= SDMMC_CMD_DAT_WR;
>> ? ? ? }
>>
>> + ? ? if (slot->host->drv_data->ctrl_type == DW_MCI_TYPE_EXYNOS5250)
>> + ? ? ? ? ? ? if (SDMMC_CLKSEL_GET_SELCLK_DRV(mci_readl(slot->host, CLKSEL)))
>> + ? ? ? ? ? ? ? ? ? ? cmdr |= SDMMC_USE_HOLD_REG;
> Some other board, custom SOC also can use this HOLD register. So it's
> not EXYNOS5250 specific one. I think we introduce the more generic
> quirks for this instead of SOC specific.

The above code is Exynos5250 specific. The Exynos5250 hardware manual
specifies additional restrictions on when the hold register can be
used. These restrictions are specific to Exynos5250 and hence there is
check for type of the controller.


>> +
>> ? ? ? return cmdr;
>> ?}
>>
>> @@ -787,10 +792,19 @@ static void dw_mci_set_ios(struct mmc_host *mmc,
>> struct mmc_ios *ios)
>> ? ? ? regs = mci_readl(slot->host, UHS_REG);
>>
>> ? ? ? /* DDR mode set */
>> - ? ? if (ios->timing == MMC_TIMING_UHS_DDR50)
>> + ? ? if (ios->timing == MMC_TIMING_UHS_DDR50) {
>> ? ? ? ? ? ? ? regs |= (0x1 << slot->id) << 16;
>> - ? ? else
>> + ? ? ? ? ? ? mci_writel(slot->host, CLKSEL, slot->host->ddr_timing);
> As you wrote, does CLKSEL is some SOC specific registers?

Yes, the CLKSEL is a Exynos specific register.


>> + ? ? } else {
>> ? ? ? ? ? ? ? regs &= ~(0x1 << slot->id) << 16;
>> + ? ? ? ? ? ? mci_writel(slot->host, CLKSEL, slot->host->sdr_timing);
>> + ? ? }
>> +
>> + ? ? if (slot->host->drv_data->ctrl_type == DW_MCI_TYPE_EXYNOS5250) {
>> + ? ? ? ? ? ? slot->host->bus_hz = clk_get_rate(slot->host->ciu_clk);
>> + ? ? ? ? ? ? slot->host->bus_hz /= SDMMC_CLKSEL_GET_DIVRATIO(
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? mci_readl(slot->host, CLKSEL));
>> + ? ? }
>>
>> ? ? ? mci_writel(slot->host, UHS_REG, regs);
>>
>> @@ -2074,6 +2088,20 @@ static struct dw_mci_board *dw_mci_parse_dt(struct
>> dw_mci *host)
>> ? ? ? ? ? ? ? if (of_get_property(np, of_quriks[idx].quirk, NULL))
>> ? ? ? ? ? ? ? ? ? ? ? pdata->quirks |= of_quriks[idx].id;
>>
>> + ? ? if (of_property_read_u32_array(dev->of_node,
>> + ? ? ? ? ? ? ? ? ? ? "samsung,dw-mshc-sdr-timing", timing, 3))
>> + ? ? ? ? ? ? host->sdr_timing = DW_MCI_DEF_SDR_TIMING;
>> + ? ? else
>> + ? ? ? ? ? ? host->sdr_timing = SDMMC_CLKSEL_TIMING(timing[0],
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? timing[1], timing[2]);
>> +
>> + ? ? if (of_property_read_u32_array(dev->of_node,
>> + ? ? ? ? ? ? ? ? ? ? "samsung,dw-mshc-ddr-timing", timing, 3))
>> + ? ? ? ? ? ? host->ddr_timing = DW_MCI_DEF_DDR_TIMING;
>> + ? ? else
>> + ? ? ? ? ? ? host->ddr_timing = SDMMC_CLKSEL_TIMING(timing[0],
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? timing[1], timing[2]);
>> +
>> ? ? ? if (of_property_read_u32(np, "fifo-depth", &pdata->fifo_depth))
>> ? ? ? ? ? ? ? dev_info(dev, "fifo-depth property not found, using "
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "value of FIFOTH register as default\n");
>> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
>> index 8b8862b..4b7e42b 100644
>> --- a/drivers/mmc/host/dw_mmc.h
>> +++ b/drivers/mmc/host/dw_mmc.h
>> @@ -53,6 +53,7 @@
>> ?#define SDMMC_IDINTEN ? ? ? ? ? ? ? ?0x090
>> ?#define SDMMC_DSCADDR ? ? ? ? ? ? ? ?0x094
>> ?#define SDMMC_BUFADDR ? ? ? ? ? ? ? ?0x098
>> +#define SDMMC_CLKSEL ? ? ? ? 0x09C /* specific to Samsung Exynos5250 */
> Another Samsung Custom SOC also used it.

Right, it is used in Exynos4 as well. So I will fix the comment accordingly.

Thanks,
Thomas.

2012-05-10 10:44:17

by Thomas Abraham

[permalink] [raw]
Subject: Re: [PATCH 3/7] mmc: dw_mmc: add device tree support

On 10 May 2012 16:02, James Hogan <[email protected]> wrote:
> Hi
>
> On 2 May 2012 06:07, Thomas Abraham <[email protected]> wrote:
>> Add device tree based discovery support.
>>
>> Signed-off-by: Thomas Abraham <[email protected]>
>> ---
>> ?.../devicetree/bindings/mmc/synposis-dw-mshc.txt ? | ? 85 +++++++++
>> ?drivers/mmc/host/dw_mmc-pltfm.c ? ? ? ? ? ? ? ? ? ?| ? 24 +++
>> ?drivers/mmc/host/dw_mmc.c ? ? ? ? ? ? ? ? ? ? ? ? ?| ?181 +++++++++++++++++++-
>> ?drivers/mmc/host/dw_mmc.h ? ? ? ? ? ? ? ? ? ? ? ? ?| ? 10 +
>> ?include/linux/mmc/dw_mmc.h ? ? ? ? ? ? ? ? ? ? ? ? | ? ?2 +
>> ?5 files changed, 296 insertions(+), 6 deletions(-)
>> ?create mode 100644 Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
>> new file mode 100644
>> index 0000000..c1ed70e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
>> @@ -0,0 +1,85 @@
>> +* Synopsis Designware Mobile Storage Host Controller
>> +
>> +The Synopsis designware mobile storage host controller is used to interface
>> +a SoC with storage medium such as eMMC or SD/MMC cards.
>> +
>> +Required Properties:
>> +
>> +* compatible: should be one of the following
>> + ? ? ? - synopsis,dw-mshc: for controllers compliant with synopsis dw-mshc.
>
> Note that although undocumented in
> Documentation/devicetree/bindings/vendor-prefixes.txt (which it
> probably should be), the designware uart already uses a different
> prefix:
> { .compatible = "snps,dw-apb-uart" },
> http://lxr.linux.no/#linux+v3.3.5/drivers/tty/serial/8250/8250_dw.c#L165

Thanks James for pointing this out. I will change the prefix for mshc
controller bindings to be consistent with that of the uart controller.

Thanks,
Thomas.

>
> Cheers
> James

2012-05-10 10:55:20

by Thomas Abraham

[permalink] [raw]
Subject: Re: [PATCH 4/7] mmc: dw_mmc: add samsung exynos5250 specific extentions

On 2 May 2012 13:19, Jaehoon Chung <[email protected]> wrote:
> On 05/02/2012 04:01 PM, Kyungmin Park wrote:
>
>> Hi,
>>
>> On 5/2/12, Thomas Abraham <[email protected]> wrote:
>>> The instantiation of the Synopsis Designware controller on Exynos5250
>>> include extension for SDR and DDR specific tx/rx phase shift timing
>>> and CIU internal divider. In addition to that, the option to skip the
>>> command hold stage is also introduced. Add support for these Exynos5250
>>> specfic extenstions.

[...]

>>> @@ -265,6 +266,10 @@ static u32 dw_mci_prepare_command(struct mmc_host *mmc,
>>> struct mmc_command *cmd)
>>> ? ? ? ? ? ? ? ? ? ? ?cmdr |= SDMMC_CMD_DAT_WR;
>>> ? ? ?}
>>>
>>> + ? ?if (slot->host->drv_data->ctrl_type == DW_MCI_TYPE_EXYNOS5250)
>>> + ? ? ? ? ? ?if (SDMMC_CLKSEL_GET_SELCLK_DRV(mci_readl(slot->host, CLKSEL)))
>>> + ? ? ? ? ? ? ? ? ? ?cmdr |= SDMMC_USE_HOLD_REG;
>> Some other board, custom SOC also can use this HOLD register. So it's
>> not EXYNOS5250 specific one. I think we introduce the more generic
>> quirks for this instead of SOC specific.
>
> One more, I think that also need to check the IMPLEMENT_HOLD_REG bit in HCON register.
> It has dependency with that.

The above code is specific to Exynos5250 and hence it is not required
to check the IMPLEMENT_HOLD_REG bit in HCON register. On Exynos5250,
the hold register is implemented and available.


> As Mr.Park is mentioned, this register is clock phasing.
> In spec, card is enumerated in SDR12 or SDR25 mode, the application must program the use_hold_reg.

Exynos5250 hardware manual specifies additional restrictions on the
use of hold register. The above code checks for those restrictions and
programs the USE_HOLD_REG accordingly. Please let me know if there is
any condition that is not handled by the above code.

Thanks,
Thomas.

>
> Best Regards,
> Jaehoon Chung
>

2012-05-10 10:56:05

by Thomas Abraham

[permalink] [raw]
Subject: Re: [PATCH 4/7] mmc: dw_mmc: add samsung exynos5250 specific extentions

On 2 May 2012 23:40, Olof Johansson <[email protected]> wrote:
> Hi,
>
> On Tue, May 1, 2012 at 10:07 PM, Thomas Abraham
> <[email protected]> wrote:
>> The instantiation of the Synopsis Designware controller on Exynos5250
>> include extension for SDR and DDR specific tx/rx phase shift timing
>> and CIU internal divider. In addition to that, the option to skip the
>> command hold stage is also introduced. Add support for these Exynos5250
>> specfic extenstions.
>>
>> Signed-off-by: Abhilash Kesavan <[email protected]>
>> Signed-off-by: Thomas Abraham <[email protected]>
>> ---
>> ?.../devicetree/bindings/mmc/synposis-dw-mshc.txt ? | ? 33 +++++++++++++++++++-
>> ?drivers/mmc/host/dw_mmc-pltfm.c ? ? ? ? ? ? ? ? ? ?| ? ?8 +++++
>> ?drivers/mmc/host/dw_mmc.c ? ? ? ? ? ? ? ? ? ? ? ? ?| ? 32 ++++++++++++++++++-
>> ?drivers/mmc/host/dw_mmc.h ? ? ? ? ? ? ? ? ? ? ? ? ?| ? 13 ++++++++
>> ?include/linux/mmc/dw_mmc.h ? ? ? ? ? ? ? ? ? ? ? ? | ? ?6 +++
>> ?5 files changed, 89 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
>> index c1ed70e..465fc31 100644
>> --- a/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
>> +++ b/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
>> @@ -7,6 +7,8 @@ Required Properties:
>>
>> ?* compatible: should be one of the following
>> ? ? ? ?- synopsis,dw-mshc: for controllers compliant with synopsis dw-mshc.
>> + ? ? ? - synopsis,dw-mshc-exynos5250: for controllers with Samsung
>> + ? ? ? ? Exynos5250 specific extentions.
>
> It makes more sense to use your own manufacturer prefix here:
>
> samsung,exynos5250-dw-mshc
>

Ok. I will modify the compatible value as you have suggested.

Thanks,
Thomas.

>
> -Olof

2012-05-10 11:17:51

by Jaehoon Chung

[permalink] [raw]
Subject: Re: [PATCH 4/7] mmc: dw_mmc: add samsung exynos5250 specific extentions

On 05/10/2012 07:55 PM, Thomas Abraham wrote:

> On 2 May 2012 13:19, Jaehoon Chung <[email protected]> wrote:
>> On 05/02/2012 04:01 PM, Kyungmin Park wrote:
>>
>>> Hi,
>>>
>>> On 5/2/12, Thomas Abraham <[email protected]> wrote:
>>>> The instantiation of the Synopsis Designware controller on Exynos5250
>>>> include extension for SDR and DDR specific tx/rx phase shift timing
>>>> and CIU internal divider. In addition to that, the option to skip the
>>>> command hold stage is also introduced. Add support for these Exynos5250
>>>> specfic extenstions.
>
> [...]
>
>>>> @@ -265,6 +266,10 @@ static u32 dw_mci_prepare_command(struct mmc_host *mmc,
>>>> struct mmc_command *cmd)
>>>> cmdr |= SDMMC_CMD_DAT_WR;
>>>> }
>>>>
>>>> + if (slot->host->drv_data->ctrl_type == DW_MCI_TYPE_EXYNOS5250)
>>>> + if (SDMMC_CLKSEL_GET_SELCLK_DRV(mci_readl(slot->host, CLKSEL)))
>>>> + cmdr |= SDMMC_USE_HOLD_REG;
>>> Some other board, custom SOC also can use this HOLD register. So it's
>>> not EXYNOS5250 specific one. I think we introduce the more generic
>>> quirks for this instead of SOC specific.
>>
>> One more, I think that also need to check the IMPLEMENT_HOLD_REG bit in HCON register.
>> It has dependency with that.
>
> The above code is specific to Exynos5250 and hence it is not required
> to check the IMPLEMENT_HOLD_REG bit in HCON register. On Exynos5250,
> the hold register is implemented and available.

Right, the above code is specific for Exynos5250.
But HOLD_REG should be used in other SoC. it's not only Exynos5250 specific.
I want more generic code than specific code for Exynos5250.

Best Regards,
Jaehoon Chung

>
>
>> As Mr.Park is mentioned, this register is clock phasing.
>> In spec, card is enumerated in SDR12 or SDR25 mode, the application must program the use_hold_reg.
>
> Exynos5250 hardware manual specifies additional restrictions on the
> use of hold register. The above code checks for those restrictions and
> programs the USE_HOLD_REG accordingly. Please let me know if there is
> any condition that is not handled by the above code.
>
> Thanks,
> Thomas.
>
>>
>> Best Regards,
>> Jaehoon Chung
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

2012-05-10 13:44:13

by Russell King - ARM Linux

[permalink] [raw]
Subject: Re: [PATCH 1/7] mmc: dw_mmc: lookup for optional biu and ciu clocks

On Wed, May 02, 2012 at 03:53:53PM +0100, James Hogan wrote:
> Hi
>
> On 2 May 2012 06:07, Thomas Abraham <[email protected]> wrote:
> > + ? ? ? if (IS_ERR(host->biu_clk))
> > + ? ? ? ? ? ? ? dev_info(&host->dev, "biu clock not available\n");
>
> In this case, should it set host->biu_clk to NULL or are clk_disable
> and clk_put guaranteed to handle an IS_ERR value?

I keep saying this. NULL must be treated as a valid reutrn value from
clk_get() and must not use this as a sentinel. The clk namespace is
that errors are indicated with IS_ERR(clk) returning true. Everything
else must be assumed to be valid by drivers.

2012-05-12 07:01:31

by Thomas Abraham

[permalink] [raw]
Subject: Re: [PATCH 3/7] mmc: dw_mmc: add device tree support

On 4 May 2012 04:18, Guennadi Liakhovetski <[email protected]> wrote:
>
> What do you think about this patch
>
> http://www.spinics.net/lists/linux-sh/msg11259.html
>
> and about using mmc-generic OF properties instead of creating yet another
> copy of proprietary ones?

Hi Guennadi,

This patch does not intend to add any custom mmc properties. I checked
your patch (in the link mentioned above) and most of the bindings are
similar to what you have come up with except for the "ro-gpios" for
which I have used "wp-gpios". But this is following what Arnd had
proposed here: http://www.spinics.net/lists/linux-mmc/msg13564.html.

Regarding the MMC_CAP_4_BIT_DATA and MMC_CAP_8_BIT_DATA in your patch,
these can be derived from the bus width information that the driver
receives.

Thanks,
Thomas.

2012-05-12 07:07:33

by Olof Johansson

[permalink] [raw]
Subject: Re: [PATCH 3/7] mmc: dw_mmc: add device tree support

Hi,

On Thu, May 10, 2012 at 3:15 AM, Thomas Abraham
<[email protected]> wrote:
> Hi Olof,
>
> On 2 May 2012 23:37, Olof Johansson <[email protected]> wrote:
>> Hi,
>
> [...]
>
>>> +# Slots: The slot specific information are contained within child-nodes with
>>> + ?each child-node representing a supported slot. There should be atleast one
>>> + ?child node representing a card slot. The name of the slot child node should
>>> + ?be 'slot{n}' where n is the unique number of the slot connnected to the
>>> + ?controller. The following are optional properties which can be included in
>>> + ?the slot child node.
>>
>> Since we're talking slots / cards on a bus, I think the addressing
>> model would be useful here. So in the main controller node:
>> ? ?#address-cells = <1>;
>> ? ?#size-cells = <0>;
>>
>> And then each slot would need a reg property and possibly unit address:
>>
>> ? slot {
>> ? ? ? ?reg = <0>;
>> ? ? ? ?...
>> ? };
>>
>> (unit addresses on the slots are only needed if they can't be
>> disambiguated by name, so not needed if you only have one slot).
>>
>
> Is the addressing model as described above needed in this case? The
> address for a slot is not used by the controller driver code and is
> just a virtual number. It would be sufficient to represent the nodes
> representing the slots with a unique name.

The driver has the concept of slot IDs (slot->id struct member), and
the hardware definitely enumerates them.

So, I think it makes sense to give a chance to enumerate the slots in
the device tree. Otherwise, how do you know which one is which on
hardware? It also opens up the flexibility to have the same name for
both slots if it makes sense to describe a board that way.


-Olof

2012-05-12 08:43:14

by Thomas Abraham

[permalink] [raw]
Subject: Re: [PATCH 3/7] mmc: dw_mmc: add device tree support

On 12 May 2012 12:37, Olof Johansson <[email protected]> wrote:
> Hi,
>
> On Thu, May 10, 2012 at 3:15 AM, Thomas Abraham
> <[email protected]> wrote:
>> Hi Olof,
>>
>> On 2 May 2012 23:37, Olof Johansson <[email protected]> wrote:
>>> Hi,
>>
>> [...]
>>
>>>> +# Slots: The slot specific information are contained within child-nodes with
>>>> + ?each child-node representing a supported slot. There should be atleast one
>>>> + ?child node representing a card slot. The name of the slot child node should
>>>> + ?be 'slot{n}' where n is the unique number of the slot connnected to the
>>>> + ?controller. The following are optional properties which can be included in
>>>> + ?the slot child node.
>>>
>>> Since we're talking slots / cards on a bus, I think the addressing
>>> model would be useful here. So in the main controller node:
>>> ? ?#address-cells = <1>;
>>> ? ?#size-cells = <0>;
>>>
>>> And then each slot would need a reg property and possibly unit address:
>>>
>>> ? slot {
>>> ? ? ? ?reg = <0>;
>>> ? ? ? ?...
>>> ? };
>>>
>>> (unit addresses on the slots are only needed if they can't be
>>> disambiguated by name, so not needed if you only have one slot).
>>>
>>
>> Is the addressing model as described above needed in this case? The
>> address for a slot is not used by the controller driver code and is
>> just a virtual number. It would be sufficient to represent the nodes
>> representing the slots with a unique name.
>
> The driver has the concept of slot IDs (slot->id struct member), and
> the hardware definitely enumerates them.
>
> So, I think it makes sense to give a chance to enumerate the slots in
> the device tree. Otherwise, how do you know which one is which on
> hardware? It also opens up the flexibility to have the same name for
> both slots if it makes sense to describe a board that way.

Thanks Olof. Yes, I missed the usage of the id number in the driver. I
will add the slot addressing as you have suggested and repost.

Thanks,
Thomas.

>
>
> -Olof

2012-05-12 19:31:55

by Guennadi Liakhovetski

[permalink] [raw]
Subject: Re: [PATCH 3/7] mmc: dw_mmc: add device tree support

Hi Thomas

On Sat, 12 May 2012, Thomas Abraham wrote:

> On 4 May 2012 04:18, Guennadi Liakhovetski <[email protected]> wrote:
> >
> > What do you think about this patch
> >
> > http://www.spinics.net/lists/linux-sh/msg11259.html
> >
> > and about using mmc-generic OF properties instead of creating yet another
> > copy of proprietary ones?
>
> Hi Guennadi,
>
> This patch does not intend to add any custom mmc properties. I checked
> your patch (in the link mentioned above) and most of the bindings are
> similar to what you have come up with except for the "ro-gpios" for
> which I have used "wp-gpios". But this is following what Arnd had
> proposed here: http://www.spinics.net/lists/linux-mmc/msg13564.html.

Thanks for the link! I didn't know about that patch.

> Regarding the MMC_CAP_4_BIT_DATA and MMC_CAP_8_BIT_DATA in your patch,
> these can be derived from the bus width information that the driver
> receives.

Sure. I think, my patch, that I mentioned above, shall be dropped, at
least in its present form. But if at least in principle we do want to have
a common MMC OF parser for common bindings, some code from your patch
would become redundant, I think. BTW, isn't this

+ gpio = of_get_named_gpio(np, "wp_gpios", 0);

a typo? Shouldn't it be "wp-gpios" instead?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

2012-05-17 15:39:24

by Thomas Abraham

[permalink] [raw]
Subject: Re: [PATCH 3/7] mmc: dw_mmc: add device tree support

On 13 May 2012 01:01, Guennadi Liakhovetski <[email protected]> wrote:
> Sure. I think, my patch, that I mentioned above, shall be dropped, at
> least in its present form. But if at least in principle we do want to have
> a common MMC OF parser for common bindings, some code from your patch
> would become redundant, I think. BTW, isn't this
>
> + ? ? ? gpio = of_get_named_gpio(np, "wp_gpios", 0);
>
> a typo? Shouldn't it be "wp-gpios" instead?

Yes, it is a typo. Thanks for your review. This is fixed now.

Regards,
Thomas.