[PATCHv2] ARM: socfpga: dts: Add support for SD/MMC

[PATCHv2] ARM: socfpga: dts: Add support for SD/MMC

[dinguyen at altera.com]

From: Dinh Nguyen <dinguyen@altera.com>

Add bindings for SD/MMC for SOCFPGA.
Add "syscon" to the "altr,sys-mgr" binding.

Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
Reviewed-by: Pavel Machek <pavel@denx.de>
Acked-by: Jaehoon Chung <jh80.chung@samsung.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 at vger.kernel.org
Cc: linux-mmc at vger.kernel.org
CC: linux-arm-kernel at lists.infradead.org

v2:
- Remove bus-width and extra line in documentation
- Merge bindings example into a single node in documentation

---
Re-send to updated device tree bindings maintainers.
---
 .../devicetree/bindings/mmc/socfpga-dw-mshc.txt    |   48 ++++++++++++++++++++
 arch/arm/boot/dts/socfpga.dtsi                     |   13 +++++-
 arch/arm/boot/dts/socfpga_cyclone5.dts             |   13 ++++++
 arch/arm/boot/dts/socfpga_vt.dts                   |   12 +++++
 4 files changed, 85 insertions(+), 1 deletion(-)
 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..dc14922
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
@@ -0,0 +1,48 @@
+* Altera 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-ciu-div: Specifies the divider value for the card interface
+  unit (ciu) clock. The value should be (n-1). For Altera's SOCFPGA, the divider
+  value is fixed at 3, which means parent_clock/4.
+
+* 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.
+
+Example:
+	dwmmc0 at ff704000 {
+		compatible = "altr,socfpga-dw-mshc", "snps,dw-mshc";
+		reg = <0xff704000 0x1000>;
+		interrupts = <0 139 4>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		num-slots = <1>;
+		supports-highspeed;
+		broken-cd;
+		fifo-depth = <0x400>;
+		altr,dw-mshc-ciu-div = <3>;
+      		altr,dw-mshc-sdr-timing = <0 3>;
+
+		slot at 0 {
+			reg = <0>;
+			bus-width = <4>;
+		};
+	};
diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index bee62a2..dbf7f22 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -468,6 +468,17 @@
 			cache-level = <2>;
 		};
 
+		mmc: dwmmc0 at 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 at fffec600 {
 			compatible = "arm,cortex-a9-twd-timer";
@@ -521,7 +532,7 @@
 			};
 
 		sysmgr at ffd08000 {
-				compatible = "altr,sys-mgr";
+				compatible = "altr,sys-mgr", "syscon";
 				reg = <0xffd08000 0x4000>;
 			};
 	};
diff --git a/arch/arm/boot/dts/socfpga_cyclone5.dts b/arch/arm/boot/dts/socfpga_cyclone5.dts
index 973999d..1853cb1 100644
--- a/arch/arm/boot/dts/socfpga_cyclone5.dts
+++ b/arch/arm/boot/dts/socfpga_cyclone5.dts
@@ -54,6 +54,19 @@
 			status = "okay";
 		};
 
+		dwmmc0 at ff704000 {
+			num-slots = <1>;
+			supports-highspeed;
+			broken-cd;
+			altr,dw-mshc-ciu-div = <3>;
+			altr,dw-mshc-sdr-timing = <0 3>;
+
+			slot at 0 {
+				reg = <0>;
+				bus-width = <4>;
+			};
+		};
+
 		timer0 at ffc08000 {
 			clock-frequency = <100000000>;
 		};
diff --git a/arch/arm/boot/dts/socfpga_vt.dts b/arch/arm/boot/dts/socfpga_vt.dts
index d1ec0ca..d93deb0 100644
--- a/arch/arm/boot/dts/socfpga_vt.dts
+++ b/arch/arm/boot/dts/socfpga_vt.dts
@@ -46,6 +46,18 @@
 			status = "okay";
 		};
 
+		dwmmc0 at ff704000 {
+			num-slots = <1>;
+			supports-highspeed;
+			broken-cd;
+			altr,dw-mshc-ciu-div = <3>;
+
+			slot at 0 {
+				reg = <0>;
+				bus-width = <4>;
+			};
+		};
+
 		timer0 at ffc08000 {
 			clock-frequency = <7000000>;
 		};
-- 
1.7.9.5

[PATCHv2] ARM: socfpga: dts: Add support for SD/MMC

[Stephen Warren]

On 08/05/2013 02:43 PM, dinguyen at altera.com wrote:
> From: Dinh Nguyen <dinguyen@altera.com>
> 
> Add bindings for SD/MMC for SOCFPGA.
> Add "syscon" to the "altr,sys-mgr" binding.

> 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..dc14922
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
> @@ -0,0 +1,48 @@
> +* Altera 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-ciu-div: Specifies the divider value for the card interface
> +  unit (ciu) clock. The value should be (n-1). For Altera's SOCFPGA, the divider
> +  value is fixed at 3, which means parent_clock/4.

This feels like something that should be represented using the common
clock API; a driver should query the rate of its input clock, and then
calculate the MMC block's internal divider based on that (perhaps also
call clk_set_rate() on the input clock?).

> +Example:
> +	dwmmc0 at ff704000 {
> +		compatible = "altr,socfpga-dw-mshc", "snps,dw-mshc";
> +		reg = <0xff704000 0x1000>;
> +		interrupts = <0 139 4>;

> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		num-slots = <1>;
> +		supports-highspeed;
> +		fifo-depth = <0x400>;

Those properties aren't defined in this document anywhere. I guess this
binding is meant to "inherit" from that described in
"synopsis-dw-mshc.txt"? If so, that should be stated explicitly.

A similar comment applies to the clocks properties in the *.dtsi changes.

> +		altr,dw-mshc-ciu-div = <3>;
> +      		altr,dw-mshc-sdr-timing = <0 3>;

Indentation issue.

[PATCHv2] ARM: socfpga: dts: Add support for SD/MMC

[Dinh Nguyen]

On Thu, 2013-08-08 at 14:14 -0600, Stephen Warren wrote:
> On 08/05/2013 02:43 PM, dinguyen at altera.com wrote:
> > From: Dinh Nguyen <dinguyen@altera.com>
> > 
> > Add bindings for SD/MMC for SOCFPGA.
> > Add "syscon" to the "altr,sys-mgr" binding.
> 
> > 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..dc14922
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
> > @@ -0,0 +1,48 @@
> > +* Altera 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-ciu-div: Specifies the divider value for the card interface
> > +  unit (ciu) clock. The value should be (n-1). For Altera's SOCFPGA, the divider
> > +  value is fixed at 3, which means parent_clock/4.
> 
> This feels like something that should be represented using the common
> clock API; a driver should query the rate of its input clock, and then
> calculate the MMC block's internal divider based on that (perhaps also
> call clk_set_rate() on the input clock?).

This means a change to the dw_mmc driver, which I can look into for the
next round? I have promised Pawel to consolidate the bindings for both
exynos and socfpga in the next round already. I will also look into
using the common clock API for the MMC as well. 

This patch is the only thing that is preventing from SD/MMC working for
SOCFPGA in the mainline, can I get your Ack if I look into doing this
for 3.13 for both the exynos and socfpga driver, and address your latter
comments?

> 
> > +Example:
> > +	dwmmc0 at ff704000 {
> > +		compatible = "altr,socfpga-dw-mshc", "snps,dw-mshc";
> > +		reg = <0xff704000 0x1000>;
> > +		interrupts = <0 139 4>;
> 
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +		num-slots = <1>;
> > +		supports-highspeed;
> > +		fifo-depth = <0x400>;
> 
> Those properties aren't defined in this document anywhere. I guess this
> binding is meant to "inherit" from that described in
> "synopsis-dw-mshc.txt"? If so, that should be stated explicitly.

Yes, will state in v3.
> 
> A similar comment applies to the clocks properties in the *.dtsi changes.
> 
> > +		altr,dw-mshc-ciu-div = <3>;
> > +      		altr,dw-mshc-sdr-timing = <0 3>;
> 
> Indentation issue.

Will fix..

Dinh
> 
> 

[PATCHv2] ARM: socfpga: dts: Add support for SD/MMC

[Stephen Warren]

On 08/08/2013 02:32 PM, Dinh Nguyen wrote:
> On Thu, 2013-08-08 at 14:14 -0600, Stephen Warren wrote:
>> On 08/05/2013 02:43 PM, dinguyen at altera.com wrote:
>>> From: Dinh Nguyen <dinguyen@altera.com>
>>>
>>> Add bindings for SD/MMC for SOCFPGA.
>>> Add "syscon" to the "altr,sys-mgr" binding.
>>
>>> 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..dc14922
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
>>> @@ -0,0 +1,48 @@
>>> +* Altera 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-ciu-div: Specifies the divider value for the card interface
>>> +  unit (ciu) clock. The value should be (n-1). For Altera's SOCFPGA, the divider
>>> +  value is fixed at 3, which means parent_clock/4.
>>
>> This feels like something that should be represented using the common
>> clock API; a driver should query the rate of its input clock, and then
>> calculate the MMC block's internal divider based on that (perhaps also
>> call clk_set_rate() on the input clock?).
> 
> This means a change to the dw_mmc driver, which I can look into for the
> next round? I have promised Pawel to consolidate the bindings for both
> exynos and socfpga in the next round already. I will also look into
> using the common clock API for the MMC as well. 
> 
> This patch is the only thing that is preventing from SD/MMC working for
> SOCFPGA in the mainline, can I get your Ack if I look into doing this
> for 3.13 for both the exynos and socfpga driver, and address your latter
> comments?

The problem is that if the binding supports or requires that property
now, it has to at least support it forever. Given that we're getting
serious about DT ABI now, we should be only introducing DT bindings that
we believe are complete and corrrect, rather than bindings which we
actively expect to be temporary and to change incompatibly later.

[PATCHv2] ARM: socfpga: dts: Add support for SD/MMC

[Dinh Nguyen]

On Thu, 2013-08-08 at 14:37 -0600, Stephen Warren wrote:
> On 08/08/2013 02:32 PM, Dinh Nguyen wrote:
> > On Thu, 2013-08-08 at 14:14 -0600, Stephen Warren wrote:
> >> On 08/05/2013 02:43 PM, dinguyen at altera.com wrote:
> >>> From: Dinh Nguyen <dinguyen@altera.com>
> >>>
> >>> Add bindings for SD/MMC for SOCFPGA.
> >>> Add "syscon" to the "altr,sys-mgr" binding.
> >>
> >>> 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..dc14922
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
> >>> @@ -0,0 +1,48 @@
> >>> +* Altera 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-ciu-div: Specifies the divider value for the card interface
> >>> +  unit (ciu) clock. The value should be (n-1). For Altera's SOCFPGA, the divider
> >>> +  value is fixed at 3, which means parent_clock/4.
> >>
> >> This feels like something that should be represented using the common
> >> clock API; a driver should query the rate of its input clock, and then
> >> calculate the MMC block's internal divider based on that (perhaps also
> >> call clk_set_rate() on the input clock?).
> > 
> > This means a change to the dw_mmc driver, which I can look into for the
> > next round? I have promised Pawel to consolidate the bindings for both
> > exynos and socfpga in the next round already. I will also look into
> > using the common clock API for the MMC as well. 
> > 
> > This patch is the only thing that is preventing from SD/MMC working for
> > SOCFPGA in the mainline, can I get your Ack if I look into doing this
> > for 3.13 for both the exynos and socfpga driver, and address your latter
> > comments?
> 
> The problem is that if the binding supports or requires that property
> now, it has to at least support it forever. Given that we're getting
> serious about DT ABI now, we should be only introducing DT bindings that
> we believe are complete and corrrect, rather than bindings which we
> actively expect to be temporary and to change incompatibly later.
> 
Right ok. Then I guess I will have to look into consolidating the
bindings sooner rather than later.

I went back to look at the driver again, and I think it is doing as you
are suggesting:

host->bus_hz is getting its input value from the common clock API.
"altr,dw-mshc-ciu-div" is specifying the internal divider that is in the
SD/MMC IP itself.

I guess I'm still not clear on how I can represent the SD/MMC divider in
in the context of the common clock API.

Dinh

[PATCHv2] ARM: socfpga: dts: Add support for SD/MMC

[Stephen Warren]

On 08/08/2013 02:54 PM, Dinh Nguyen wrote:
> On Thu, 2013-08-08 at 14:37 -0600, Stephen Warren wrote:
>> On 08/08/2013 02:32 PM, Dinh Nguyen wrote:
>>> On Thu, 2013-08-08 at 14:14 -0600, Stephen Warren wrote:
>>>> On 08/05/2013 02:43 PM, dinguyen at altera.com wrote:
>>>>> From: Dinh Nguyen <dinguyen@altera.com>
>>>>>
>>>>> Add bindings for SD/MMC for SOCFPGA.
>>>>> Add "syscon" to the "altr,sys-mgr" binding.
>>>>
>>>>> 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..dc14922
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
>>>>> @@ -0,0 +1,48 @@
>>>>> +* Altera 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-ciu-div: Specifies the divider value for the card interface
>>>>> +  unit (ciu) clock. The value should be (n-1). For Altera's SOCFPGA, the divider
>>>>> +  value is fixed at 3, which means parent_clock/4.
>>>>
>>>> This feels like something that should be represented using the common
>>>> clock API; a driver should query the rate of its input clock, and then
>>>> calculate the MMC block's internal divider based on that (perhaps also
>>>> call clk_set_rate() on the input clock?).
>>>
>>> This means a change to the dw_mmc driver, which I can look into for the
>>> next round? I have promised Pawel to consolidate the bindings for both
>>> exynos and socfpga in the next round already. I will also look into
>>> using the common clock API for the MMC as well. 
>>>
>>> This patch is the only thing that is preventing from SD/MMC working for
>>> SOCFPGA in the mainline, can I get your Ack if I look into doing this
>>> for 3.13 for both the exynos and socfpga driver, and address your latter
>>> comments?
>>
>> The problem is that if the binding supports or requires that property
>> now, it has to at least support it forever. Given that we're getting
>> serious about DT ABI now, we should be only introducing DT bindings that
>> we believe are complete and corrrect, rather than bindings which we
>> actively expect to be temporary and to change incompatibly later.
>>
> Right ok. Then I guess I will have to look into consolidating the
> bindings sooner rather than later.
> 
> I went back to look at the driver again, and I think it is doing as you
> are suggesting:
> 
> host->bus_hz is getting its input value from the common clock API.
> "altr,dw-mshc-ciu-div" is specifying the internal divider that is in the
> SD/MMC IP itself.
> 
> I guess I'm still not clear on how I can represent the SD/MMC divider in
> in the context of the common clock API.

Why is there a need to directly represent the divider anywhere? The
driver can find the rate of the input clock, and I assume it knows what
rate it wants the clock to run at, so can't it calculate the divider
based on those two pieces of information?

Or, does the driver really not know what rate it wants the clock to be
after the internal divider? If not, then I think that *rate* should be
recorded in DT, not the divider to obtain that rate.

[PATCHv2] ARM: socfpga: dts: Add support for SD/MMC

[Dinh Nguyen]

On Thu, 2013-08-08 at 15:13 -0600, Stephen Warren wrote:
> On 08/08/2013 02:54 PM, Dinh Nguyen wrote:
> > On Thu, 2013-08-08 at 14:37 -0600, Stephen Warren wrote:
> >> On 08/08/2013 02:32 PM, Dinh Nguyen wrote:
> >>> On Thu, 2013-08-08 at 14:14 -0600, Stephen Warren wrote:
> >>>> On 08/05/2013 02:43 PM, dinguyen at altera.com wrote:
> >>>>> From: Dinh Nguyen <dinguyen@altera.com>
> >>>>>
> >>>>> Add bindings for SD/MMC for SOCFPGA.
> >>>>> Add "syscon" to the "altr,sys-mgr" binding.
> >>>>
> >>>>> 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..dc14922
> >>>>> --- /dev/null
> >>>>> +++ b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
> >>>>> @@ -0,0 +1,48 @@
> >>>>> +* Altera 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-ciu-div: Specifies the divider value for the card interface
> >>>>> +  unit (ciu) clock. The value should be (n-1). For Altera's SOCFPGA, the divider
> >>>>> +  value is fixed at 3, which means parent_clock/4.
> >>>>
> >>>> This feels like something that should be represented using the common
> >>>> clock API; a driver should query the rate of its input clock, and then
> >>>> calculate the MMC block's internal divider based on that (perhaps also
> >>>> call clk_set_rate() on the input clock?).
> >>>
> >>> This means a change to the dw_mmc driver, which I can look into for the
> >>> next round? I have promised Pawel to consolidate the bindings for both
> >>> exynos and socfpga in the next round already. I will also look into
> >>> using the common clock API for the MMC as well. 
> >>>
> >>> This patch is the only thing that is preventing from SD/MMC working for
> >>> SOCFPGA in the mainline, can I get your Ack if I look into doing this
> >>> for 3.13 for both the exynos and socfpga driver, and address your latter
> >>> comments?
> >>
> >> The problem is that if the binding supports or requires that property
> >> now, it has to at least support it forever. Given that we're getting
> >> serious about DT ABI now, we should be only introducing DT bindings that
> >> we believe are complete and corrrect, rather than bindings which we
> >> actively expect to be temporary and to change incompatibly later.
> >>
> > Right ok. Then I guess I will have to look into consolidating the
> > bindings sooner rather than later.
> > 
> > I went back to look at the driver again, and I think it is doing as you
> > are suggesting:
> > 
> > host->bus_hz is getting its input value from the common clock API.
> > "altr,dw-mshc-ciu-div" is specifying the internal divider that is in the
> > SD/MMC IP itself.
> > 
> > I guess I'm still not clear on how I can represent the SD/MMC divider in
> > in the context of the common clock API.
> 
> Why is there a need to directly represent the divider anywhere? The
> driver can find the rate of the input clock, and I assume it knows what
> rate it wants the clock to run at, so can't it calculate the divider
> based on those two pieces of information?

CC: Chris Ball
> 
> Or, does the driver really not know what rate it wants the clock to be
> after the internal divider? If not, then I think that *rate* should be
> recorded in DT, not the divider to obtain that rate.
> 

I believe that this is the case, that the driver does not know what rate
it will clock the SD card at. I think internally we did have a "bus_hz"
in DT a while back. I guess I don't really understand why it would be
better to have a *rate* DT entry instead of a fixed-divider entry?

Dinh
> 

[PATCHv2] ARM: socfpga: dts: Add support for SD/MMC

[Stephen Warren]

On 08/08/2013 05:10 PM, Dinh Nguyen wrote:
> On Thu, 2013-08-08 at 15:13 -0600, Stephen Warren wrote:
...
>> Why is there a need to directly represent the divider anywhere? The
>> driver can find the rate of the input clock, and I assume it knows what
>> rate it wants the clock to run at, so can't it calculate the divider
>> based on those two pieces of information?
> 
> CC: Chris Ball
>>
>> Or, does the driver really not know what rate it wants the clock to be
>> after the internal divider? If not, then I think that *rate* should be
>> recorded in DT, not the divider to obtain that rate.
>>
> 
> I believe that this is the case, that the driver does not know what rate
> it will clock the SD card at. I think internally we did have a "bus_hz"
> in DT a while back. I guess I don't really understand why it would be
> better to have a *rate* DT entry instead of a fixed-divider entry?

The value of the divider depends on two things:

1) Input clock rate.
2) Desired rate after applying the internal divider.

The input clock rate may vary, either between SoCs the IP is integrated
into, or even at run-time perhaps base on clk_set_rate() etc.

If the MMC driver knows the clock rate it wants to run at, it can
calculate the divider easily; it can automatically adjust to any input
clock that its environment may provide.

If the MMC driver is simply told "use this divider", that's encoding
assumptions about the rate of the input clock which might not be valid;.
Encoding the desire clock rate within the MMC HW block allows the
divider to be calculated based on the actual environment.

[PATCHv2] ARM: socfpga: dts: Add support for SD/MMC

[Dinh Nguyen]

On Fri, 2013-08-09 at 15:00 -0600, Stephen Warren wrote:
> On 08/08/2013 05:10 PM, Dinh Nguyen wrote:
> > On Thu, 2013-08-08 at 15:13 -0600, Stephen Warren wrote:
> ...
> >> Why is there a need to directly represent the divider anywhere? The
> >> driver can find the rate of the input clock, and I assume it knows what
> >> rate it wants the clock to run at, so can't it calculate the divider
> >> based on those two pieces of information?
> > 
> > CC: Chris Ball
> >>
> >> Or, does the driver really not know what rate it wants the clock to be
> >> after the internal divider? If not, then I think that *rate* should be
> >> recorded in DT, not the divider to obtain that rate.
> >>
> > 
> > I believe that this is the case, that the driver does not know what rate
> > it will clock the SD card at. I think internally we did have a "bus_hz"
> > in DT a while back. I guess I don't really understand why it would be
> > better to have a *rate* DT entry instead of a fixed-divider entry?
> 
> The value of the divider depends on two things:
> 
> 1) Input clock rate.
> 2) Desired rate after applying the internal divider.
> 
> The input clock rate may vary, either between SoCs the IP is integrated
> into, or even at run-time perhaps base on clk_set_rate() etc.
> 
> If the MMC driver knows the clock rate it wants to run at, it can
> calculate the divider easily; it can automatically adjust to any input
> clock that its environment may provide.
> 
> If the MMC driver is simply told "use this divider", that's encoding
> assumptions about the rate of the input clock which might not be valid;.
> Encoding the desire clock rate within the MMC HW block allows the
> divider to be calculated based on the actual environment.

Thanks Stephen. That makes sense.

Chris, Jaehoon, and Seungwon, do you have any inputs? If not I will go
down the path of have the "bus-hz" in the DTS node for the clock rate of
the CIU clock. Then I would also make the same change to dw_mmc-exynos
but would need your help on what the rate would be.

For SOCFPGA, its 12.5 MHZ.

Thanks,

Dinh
> 

[PATCHv2] ARM: socfpga: dts: Add support for SD/MMC

[Stephen Warren]

On 08/09/2013 04:41 PM, Dinh Nguyen wrote:
> On Fri, 2013-08-09 at 15:00 -0600, Stephen Warren wrote:
>> On 08/08/2013 05:10 PM, Dinh Nguyen wrote:
>>> On Thu, 2013-08-08 at 15:13 -0600, Stephen Warren wrote:
>> ...
>>>> Why is there a need to directly represent the divider anywhere? The
>>>> driver can find the rate of the input clock, and I assume it knows what
>>>> rate it wants the clock to run at, so can't it calculate the divider
>>>> based on those two pieces of information?
>>>
>>> CC: Chris Ball
>>>>
>>>> Or, does the driver really not know what rate it wants the clock to be
>>>> after the internal divider? If not, then I think that *rate* should be
>>>> recorded in DT, not the divider to obtain that rate.
>>>>
>>>
>>> I believe that this is the case, that the driver does not know what rate
>>> it will clock the SD card at. I think internally we did have a "bus_hz"
>>> in DT a while back. I guess I don't really understand why it would be
>>> better to have a *rate* DT entry instead of a fixed-divider entry?
>>
>> The value of the divider depends on two things:
>>
>> 1) Input clock rate.
>> 2) Desired rate after applying the internal divider.
>>
>> The input clock rate may vary, either between SoCs the IP is integrated
>> into, or even at run-time perhaps base on clk_set_rate() etc.
>>
>> If the MMC driver knows the clock rate it wants to run at, it can
>> calculate the divider easily; it can automatically adjust to any input
>> clock that its environment may provide.
>>
>> If the MMC driver is simply told "use this divider", that's encoding
>> assumptions about the rate of the input clock which might not be valid;.
>> Encoding the desire clock rate within the MMC HW block allows the
>> divider to be calculated based on the actual environment.
> 
> Thanks Stephen. That makes sense.
> 
> Chris, Jaehoon, and Seungwon, do you have any inputs? If not I will go
> down the path of have the "bus-hz" in the DTS node for the clock rate of
> the CIU clock. Then I would also make the same change to dw_mmc-exynos
> but would need your help on what the rate would be.
> 
> For SOCFPGA, its 12.5 MHZ.

I'm not sure "bus-hz" is the right name; it's not clear from that name
that it refers to an internal clock inside the MMC controller rather
than the SD bus out to the card itself. Perhaps "controller-clock-hz" or
something like that?

[PATCHv2] ARM: socfpga: dts: Add support for SD/MMC

[Dinh Nguyen]

On Fri, 2013-08-09 at 16:58 -0600, Stephen Warren wrote:
> On 08/09/2013 04:41 PM, Dinh Nguyen wrote:
> > On Fri, 2013-08-09 at 15:00 -0600, Stephen Warren wrote:
> >> On 08/08/2013 05:10 PM, Dinh Nguyen wrote:
> >>> On Thu, 2013-08-08 at 15:13 -0600, Stephen Warren wrote:
> >> ...
> >>>> Why is there a need to directly represent the divider anywhere? The
> >>>> driver can find the rate of the input clock, and I assume it knows what
> >>>> rate it wants the clock to run at, so can't it calculate the divider
> >>>> based on those two pieces of information?
> >>>
> >>> CC: Chris Ball
> >>>>
> >>>> Or, does the driver really not know what rate it wants the clock to be
> >>>> after the internal divider? If not, then I think that *rate* should be
> >>>> recorded in DT, not the divider to obtain that rate.
> >>>>
> >>>
> >>> I believe that this is the case, that the driver does not know what rate
> >>> it will clock the SD card at. I think internally we did have a "bus_hz"
> >>> in DT a while back. I guess I don't really understand why it would be
> >>> better to have a *rate* DT entry instead of a fixed-divider entry?
> >>
> >> The value of the divider depends on two things:
> >>
> >> 1) Input clock rate.
> >> 2) Desired rate after applying the internal divider.
> >>
> >> The input clock rate may vary, either between SoCs the IP is integrated
> >> into, or even at run-time perhaps base on clk_set_rate() etc.
> >>
> >> If the MMC driver knows the clock rate it wants to run at, it can
> >> calculate the divider easily; it can automatically adjust to any input
> >> clock that its environment may provide.
> >>
> >> If the MMC driver is simply told "use this divider", that's encoding
> >> assumptions about the rate of the input clock which might not be valid;.
> >> Encoding the desire clock rate within the MMC HW block allows the
> >> divider to be calculated based on the actual environment.
> > 
> > Thanks Stephen. That makes sense.
> > 
> > Chris, Jaehoon, and Seungwon, do you have any inputs? If not I will go
> > down the path of have the "bus-hz" in the DTS node for the clock rate of
> > the CIU clock. Then I would also make the same change to dw_mmc-exynos
> > but would need your help on what the rate would be.
> > 
> > For SOCFPGA, its 12.5 MHZ.
> 
> I'm not sure "bus-hz" is the right name; it's not clear from that name
> that it refers to an internal clock inside the MMC controller rather
> than the SD bus out to the card itself. Perhaps "controller-clock-hz" or
> something like that?

It appears that there is already "clock-frequency" binding defined in
synopsis-dw-mshc.txt design for this purpose.

* clock-frequency: should be the frequency (in Hz) of the ciu clock.  If
this is specified and the ciu clock is specified then we'll try to set
the ciu clock to this at probe time.

Since its already defined, I will go ahead and use this binding.

Thanks again for the review.

Dinh
>