[PATCHv4 2/3] ARM: socfpga: dts: Add support for SD/MMC

[PATCHv4 2/3] ARM: socfpga: dts: Add support for SD/MMC

[dinguyen]

From: Dinh Nguyen <dinguyen@altera.com>

Add bindings for SD/MMC for SOCFPGA.

Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
Cc: Jaehoon Chung <jh80.chung@samsung.com>
Cc: Seungwon Jeon <tgih.jun@samsung.com>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: devicetree@vger.kernel.org
Cc: linux-mmc@vger.kernel.org
CC: linux-arm-kernel@lists.infradead.org

v4:
- Add a complete binding example in documentations
- Add a phandle entry for "altr,sysmgr" which links the system
  manager to the SD/MMC IP block that controls the SDR timings.
- Split up patches
        1/3 - Add syscon binding to sys-mgr node
        2/3 - DTS bindings and documentation for SD/MMC on SOCFPGA
        3/3 - Driver changes to use the bindings

v3:
- Explicitly reference synopsis-dw-mshc.txt for base bindings
- Remove "dw-mshc-ciu-div" as driver can get clock information dts "ciu" entry
- Fixed indentation issue

v2:
- Remove bus-width and extra line in documentation
- Merge bindings example into a single node in documentation
---
 .../devicetree/bindings/mmc/socfpga-dw-mshc.txt    |   56 ++++++++++++++++++++
 arch/arm/boot/dts/socfpga.dtsi                     |   11 ++++
 arch/arm/boot/dts/socfpga_cyclone5.dts             |   13 +++++
 arch/arm/boot/dts/socfpga_vt.dts                   |   11 ++++
 4 files changed, 91 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt

diff --git a/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
new file mode 100644
index 0000000..eaf98cd
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
@@ -0,0 +1,56 @@
+* Altera SOCFPGA specific extensions to the 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. This file documents
+differences between the core Synopsis dw mshc controller properties described
+by synopsis-dw-mshc.txt and the properties used by the SOCFPGA specific
+extensions to the Synopsis Designware Mobile Storage Host Controller.
+
+Required Properties:
+
+* compatible: should be
+	- "altr,socfpga-dw-mshc": for controllers with Altera SOCFPGA
+	  specific extensions.
+
+* altr,dw-mshc-sdr-timing: Specifies the value of CIU clock phase shift value
+  in transmit mode and CIU clock phase shift value in receive mode for single
+  data rate mode operation. Refer to notes below for the order of the cells and the
+  valid values.
+
+  Notes for the sdr-timing values:
+
+    The order of the cells should be
+      - First Cell: CIU clock phase shift value for RX mode, smplsel bits in
+	the system manager SDMMC control group.
+      - Second Cell: CIU clock phase shift value for TX mode, drvsel bits in
+	the system manager SDMMC control group.
+
+    Valid values for SDR CIU clock timing for SOCFPGA:
+      - valid value for tx phase shift and rx phase shift is 0 to 7.
+
+* altr,sysmgr: Should be the phandle to the system_mgr node. As this is where
+		this where the register that controls the CIU clock phases
+		reside.
+
+Example:
+	dwmmc0@ff704000 {
+		compatible = "altr,socfpga-dw-mshc", "snps,dw-mshc";
+		reg = <0xff704000 0x1000>;
+		interrupts = <0 139 4>;
+		fifo-depth = <0x400>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		clocks = <&l4_mp_clk>, <&sdmmc_clk>;
+		clock-names = "biu", "ciu";
+		num-slots = <1>;
+		supports-highspeed;
+		broken-cd;
+		altr,sysmgr = <&system_mgr>;
+		altr,dw-mshc-sdr-timing = <0 3>;
+
+		slot@0 {
+			reg = <0>;
+			bus-width = <4>;
+		};
+	};
diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index c31c8e9..9706767 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -468,6 +468,17 @@
 			cache-level = <2>;
 		};
 
+		mmc: dwmmc0@ff704000 {
+			compatible = "altr,socfpga-dw-mshc";
+			reg = <0xff704000 0x1000>;
+			interrupts = <0 139 4>;
+			fifo-depth = <0x400>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			clocks = <&l4_mp_clk>, <&sdmmc_clk>;
+			clock-names = "biu", "ciu";
+		};
+
 		/* Local timer */
 		timer@fffec600 {
 			compatible = "arm,cortex-a9-twd-timer";
diff --git a/arch/arm/boot/dts/socfpga_cyclone5.dts b/arch/arm/boot/dts/socfpga_cyclone5.dts
index 973999d..698dde9 100644
--- a/arch/arm/boot/dts/socfpga_cyclone5.dts
+++ b/arch/arm/boot/dts/socfpga_cyclone5.dts
@@ -54,6 +54,19 @@
 			status = "okay";
 		};
 
+		dwmmc0@ff704000 {
+			num-slots = <1>;
+			supports-highspeed;
+			broken-cd;
+			altr,sysmgr = <&system_mgr>;
+			altr,dw-mshc-sdr-timing = <0 3>;
+
+			slot@0 {
+				reg = <0>;
+				bus-width = <4>;
+			};
+		};
+
 		timer0@ffc08000 {
 			clock-frequency = <100000000>;
 		};
diff --git a/arch/arm/boot/dts/socfpga_vt.dts b/arch/arm/boot/dts/socfpga_vt.dts
index d1ec0ca..6f23121 100644
--- a/arch/arm/boot/dts/socfpga_vt.dts
+++ b/arch/arm/boot/dts/socfpga_vt.dts
@@ -46,6 +46,17 @@
 			status = "okay";
 		};
 
+		dwmmc0@ff704000 {
+			num-slots = <1>;
+			supports-highspeed;
+			broken-cd;
+
+			slot@0 {
+				reg = <0>;
+				bus-width = <4>;
+			};
+		};
+
 		timer0@ffc08000 {
 			clock-frequency = <7000000>;
 		};
-- 
1.7.9.5

Re: [PATCHv4 2/3] ARM: socfpga: dts: Add support for SD/MMC

[Stephen Warren]

On 08/14/2013 10:48 AM, dinguyen@altera.com wrote:
> From: Dinh Nguyen <dinguyen@altera.com>
> 
> Add bindings for SD/MMC for SOCFPGA.

> diff --git a/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt

> +* altr,sysmgr: Should be the phandle to the system_mgr node. As this is where
> +		this where the register that controls the CIU clock phases
> +		reside.

On the surface, this binding series seems OK, but I do have a question:
how is the sysmgr phandle used?

I assume there's some register in this syscon device that resets or
enables or otherwise controls this MSHC module. How does the code know
which register it is? The phandle in the altr,sysmgr property would
usually be followed by a/some cell(s) that encode this information, so
that the MSHC driver doesn't have to know anything about the layout of
the syscon registers, and so the sysconf driver doesn't have to know
anything about the identity of the MSHC client device.

That way, the MSHC driver will work fine if a HW designer has dropped
the MSHC IP block into a completely different SoC with a different
syscon register layout.

Re: [PATCHv4 2/3] ARM: socfpga: dts: Add support for SD/MMC

[Dinh Nguyen]

On Fri, 2013-08-16 at 16:36 -0600, Stephen Warren wrote:
> On 08/14/2013 10:48 AM, dinguyen@altera.com wrote:
> > From: Dinh Nguyen <dinguyen@altera.com>
> > 
> > Add bindings for SD/MMC for SOCFPGA.
> 
> > diff --git a/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
> 
> > +* altr,sysmgr: Should be the phandle to the system_mgr node. As this is where
> > +		this where the register that controls the CIU clock phases
> > +		reside.
> 
> On the surface, this binding series seems OK, but I do have a question:
> how is the sysmgr phandle used?
> 
> I assume there's some register in this syscon device that resets or
> enables or otherwise controls this MSHC module. How does the code know
> which register it is? The phandle in the altr,sysmgr property would
> usually be followed by a/some cell(s) that encode this information, so
> that the MSHC driver doesn't have to know anything about the layout of
> the syscon registers, and so the sysconf driver doesn't have to know
> anything about the identity of the MSHC client device.

There is a #define SYSMGR_SDMMCGRP_CTRL_OFFSET that is in
dw_mmc-socfpga.c. This defines the offset from the base address that the
sysmgr phandle will give me.

> 
> That way, the MSHC driver will work fine if a HW designer has dropped
> the MSHC IP block into a completely different SoC with a different
> syscon register layout.
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCHv4 2/3] ARM: socfpga: dts: Add support for SD/MMC

[Stephen Warren]

On 08/21/2013 01:48 PM, Dinh Nguyen wrote:
> On Fri, 2013-08-16 at 16:36 -0600, Stephen Warren wrote:
>> On 08/14/2013 10:48 AM, dinguyen@altera.com wrote:
>>> From: Dinh Nguyen <dinguyen@altera.com>
>>>
>>> Add bindings for SD/MMC for SOCFPGA.
>>
>>> diff --git a/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
>>
>>> +* altr,sysmgr: Should be the phandle to the system_mgr node. As this is where
>>> +		this where the register that controls the CIU clock phases
>>> +		reside.
>>
>> On the surface, this binding series seems OK, but I do have a question:
>> how is the sysmgr phandle used?
>>
>> I assume there's some register in this syscon device that resets or
>> enables or otherwise controls this MSHC module. How does the code know
>> which register it is? The phandle in the altr,sysmgr property would
>> usually be followed by a/some cell(s) that encode this information, so
>> that the MSHC driver doesn't have to know anything about the layout of
>> the syscon registers, and so the sysconf driver doesn't have to know
>> anything about the identity of the MSHC client device.
> 
> There is a #define SYSMGR_SDMMCGRP_CTRL_OFFSET that is in
> dw_mmc-socfpga.c. This defines the offset from the base address that the
> sysmgr phandle will give me.

Hmmm. That doesn't sound good. That means that the SDMMC driver knows
internal details about some other HW module. It'd be better if either:

a) The sysmgr driver was required to provide an API to the SDMMC driver
to set up the CIU register as requested.

or:

b) The CIU register details were represented in DT.

Either of these would allow the SDMMC driver to operate unchanged on an
SoC with a different sysmgr register layout.

Re: [PATCHv4 2/3] ARM: socfpga: dts: Add support for SD/MMC

[Dinh Nguyen]

On Thu, 2013-08-22 at 14:13 -0600, Stephen Warren wrote:
> On 08/21/2013 01:48 PM, Dinh Nguyen wrote:
> > On Fri, 2013-08-16 at 16:36 -0600, Stephen Warren wrote:
> >> On 08/14/2013 10:48 AM, dinguyen@altera.com wrote:
> >>> From: Dinh Nguyen <dinguyen@altera.com>
> >>>
> >>> Add bindings for SD/MMC for SOCFPGA.
> >>
> >>> diff --git a/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
> >>
> >>> +* altr,sysmgr: Should be the phandle to the system_mgr node. As this is where
> >>> +		this where the register that controls the CIU clock phases
> >>> +		reside.
> >>
> >> On the surface, this binding series seems OK, but I do have a question:
> >> how is the sysmgr phandle used?
> >>
> >> I assume there's some register in this syscon device that resets or
> >> enables or otherwise controls this MSHC module. How does the code know
> >> which register it is? The phandle in the altr,sysmgr property would
> >> usually be followed by a/some cell(s) that encode this information, so
> >> that the MSHC driver doesn't have to know anything about the layout of
> >> the syscon registers, and so the sysconf driver doesn't have to know
> >> anything about the identity of the MSHC client device.
> > 
> > There is a #define SYSMGR_SDMMCGRP_CTRL_OFFSET that is in
> > dw_mmc-socfpga.c. This defines the offset from the base address that the
> > sysmgr phandle will give me.
> 
> Hmmm. That doesn't sound good. That means that the SDMMC driver knows
> internal details about some other HW module. It'd be better if either:
> 
> a) The sysmgr driver was required to provide an API to the SDMMC driver
> to set up the CIU register as requested.
> 
> or:
> 
> b) The CIU register details were represented in DT.
> 
> Either of these would allow the SDMMC driver to operate unchanged on an
> SoC with a different sysmgr register layout.

Thanks Stephen...will rework accordingly.

Dinh
> 

[PATCHv4 3/3] mmc: dw_mmc: Use phandle to get SDR timing values from sys-mgr

[dinguyen]

From: Dinh Nguyen <dinguyen@altera.com>

Update the driver to get the system manager node from a phandle. Also, the
driver can get the correct clock value from the common clock API, thus the
"altr,dw-mshc-ciu-div" binding is not needed at all.

Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
Cc: Jaehoon Chung <jh80.chung@samsung.com>
Cc: Seungwon Jeon <tgih.jun@samsung.com>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Chris Ball <cjb@laptop.org>
Cc: devicetree@vger.kernel.org
Cc: linux-mmc@vger.kernel.org
CC: linux-arm-kernel@lists.infradead.org
---
 drivers/mmc/host/dw_mmc-socfpga.c |   14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc-socfpga.c b/drivers/mmc/host/dw_mmc-socfpga.c
index 14b5961..73b4440 100644
--- a/drivers/mmc/host/dw_mmc-socfpga.c
+++ b/drivers/mmc/host/dw_mmc-socfpga.c
@@ -31,7 +31,6 @@
 
 /* SOCFPGA implementation specific driver private data */
 struct dw_mci_socfpga_priv_data {
-	u8	ciu_div; /* card interface unit divisor */
 	u32	hs_timing; /* bitmask for CIU clock phase shift */
 	struct regmap   *sysreg; /* regmap for system manager register */
 };
@@ -39,6 +38,7 @@ struct dw_mci_socfpga_priv_data {
 static int dw_mci_socfpga_priv_init(struct dw_mci *host)
 {
 	struct dw_mci_socfpga_priv_data *priv;
+	struct device_node *np = host->dev->of_node;
 
 	priv = devm_kzalloc(host->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv) {
@@ -46,9 +46,9 @@ static int dw_mci_socfpga_priv_init(struct dw_mci *host)
 		return -ENOMEM;
 	}
 
-	priv->sysreg = syscon_regmap_lookup_by_compatible("altr,sys-mgr");
+	priv->sysreg = syscon_regmap_lookup_by_phandle(np, "altr,sysmgr");
 	if (IS_ERR(priv->sysreg)) {
-		dev_err(host->dev, "regmap for altr,sys-mgr lookup failed.\n");
+		dev_err(host->dev, "No sysmgr phandle specified!\n");
 		return PTR_ERR(priv->sysreg);
 	}
 	host->priv = priv;
@@ -64,8 +64,6 @@ static int dw_mci_socfpga_setup_clock(struct dw_mci *host)
 	regmap_write(priv->sysreg, SYSMGR_SDMMCGRP_CTRL_OFFSET,
 		priv->hs_timing);
 	clk_prepare_enable(host->ciu_clk);
-
-	host->bus_hz /= (priv->ciu_div + 1);
 	return 0;
 }
 
@@ -82,14 +80,8 @@ static int dw_mci_socfpga_parse_dt(struct dw_mci *host)
 	struct dw_mci_socfpga_priv_data *priv = host->priv;
 	struct device_node *np = host->dev->of_node;
 	u32 timing[2];
-	u32 div = 0;
 	int ret;
 
-	ret = of_property_read_u32(np, "altr,dw-mshc-ciu-div", &div);
-	if (ret)
-		dev_info(host->dev, "No dw-mshc-ciu-div specified, assuming 1");
-	priv->ciu_div = div;
-
 	ret = of_property_read_u32_array(np,
 			"altr,dw-mshc-sdr-timing", timing, 2);
 	if (ret)
-- 
1.7.9.5