diff mbox

[v7,8/8] ARM: sunxi: Add documentation for driver for SD/MMC hosts found on Allwinner sunxi SoCs

Message ID 20140217100302.15040.56273.stgit@pagira.o2s.ch
State New
Headers show

Commit Message

David Lanzendörfer Feb. 17, 2014, 10:03 a.m. UTC
Signed-off-by: David Lanzendörfer <david.lanzendoerfer@o2s.ch>
---
 .../devicetree/bindings/mmc/sunxi-mmc.txt          |   32 ++++++++++++++++++++
 1 file changed, 32 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mmc/sunxi-mmc.txt

Comments

Maxime Ripard Feb. 18, 2014, 3:42 p.m. UTC | #1
On Mon, Feb 17, 2014 at 11:03:02AM +0100, David Lanzendörfer wrote:
> Signed-off-by: David Lanzendörfer <david.lanzendoerfer@o2s.ch>
> ---
>  .../devicetree/bindings/mmc/sunxi-mmc.txt          |   32 ++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
> 
> diff --git a/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt b/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
> new file mode 100644
> index 0000000..5ce8c5e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
> @@ -0,0 +1,32 @@
> +* Allwinner sunxi MMC controller
> +
> +The highspeed MMC host controller on Allwinner SoCs provides an interface
> +for MMC, SD and SDIO types of memory cards.
> +
> +Supported maximum speeds are the ones of the eMMC standard 4.5 as well
> +as the speed of SD standard 3.0.
> +Absolute maximum transfer rate is 200MB/s
> +
> +Required properties:
> +- compatible: Should be "allwinner,<revision>-<chip>-mmc".
> +  The supported chips include a10, a10s, 13, a20 and a31.

Please use the real compatibles here. It's much easier to search
for. Plus, your driver doesn't support all the SoCs you're mentionning here.

> +- base registers are 0x1000 appart, so the base of mmc1
> +  would be 0x01c0f000+0x1000=0x01c10000(see example)
> +  and so on.

Please provide the real property name to use. No need for an example
here, you have a full-fledged one in a few lines.

> +- clocks requires the reference at the ahb clock gate
> +  with the correct index (mmc0 -> 8, mmc1 -> 9, and so on)
> +  as well as the reference to the correct mod0 clock.

Ditto. Plus, this is not a mod0 clock.

> +- interrupts requires the correct IRQ line
> +  (mmc0 -> 32, mmc1 -> 33, and so on)

Ditto.

> +
> +Examples:
> +
> +mmc0: mmc@01c0f000 {
> +	compatible = "allwinner,sun5i-a13-mmc";
> +	reg = <0x01c0f000 0x1000>;
> +	clocks = <&ahb_gates 8>, <&mmc0_clk>;
> +	clock-names = "ahb", "mod";

You never talked about the clock-names property, and which clocks were
supposed to be provided.

> +	interrupts = <0 32 4>;
> +	bus-width = <4>;

And you never talked about bus-width either.

> +	status = "disabled";
> +};
> 

Isn't the cd-gpios property requested too?
David Lanzendörfer Feb. 22, 2014, 7:32 a.m. UTC | #2
Hi

> Please use the real compatibles here. It's much easier to search
> for. Plus, your driver doesn't support all the SoCs you're mentionning here.
> [...]
> Please provide the real property name to use. No need for an example
> here, you have a full-fledged one in a few lines.
Fixed that.
> Ditto. Plus, this is not a mod0 clock.
Yes it is! But maybe the formulation hasn't been clear enough...
> You never talked about the clock-names property, and which clocks were
> supposed to be provided.
Yes I did? But I expanded the text a little bit further...
> > +	interrupts = <0 32 4>;
> > +	bus-width = <4>;
> And you never talked about bus-width either.
I can throw in a line for refering to the mmc slot gpio lib docs.
> Isn't the cd-gpios property requested too?
I can refer to the docs there as well if you like... :-)

cheers
david
Maxime Ripard Feb. 22, 2014, 8:37 a.m. UTC | #3
Hi David,

On Sat, Feb 22, 2014 at 08:32:03AM +0100, David Lanzendörfer wrote:
> > Ditto. Plus, this is not a mod0 clock.
> Yes it is! But maybe the formulation hasn't been clear enough...

Technically, it's not, it has this phase controls features a mod0
clock doesn't have.

> > You never talked about the clock-names property, and which clocks
> > were supposed to be provided.
>
> Yes I did? But I expanded the text a little bit further...

I can't see any reference to the fact that clock-names should be set,
and what values should it hold.

Something like that:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt#n11

> > > +	interrupts = <0 32 4>;
> > > +	bus-width = <4>;
> > And you never talked about bus-width either.
> I can throw in a line for refering to the mmc slot gpio lib docs.

Yes, that would be great :)

> > Isn't the cd-gpios property requested too?
> I can refer to the docs there as well if you like... :-)

That would be great too :)

Thanks!
Maxime
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt b/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
new file mode 100644
index 0000000..5ce8c5e
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
@@ -0,0 +1,32 @@ 
+* Allwinner sunxi MMC controller
+
+The highspeed MMC host controller on Allwinner SoCs provides an interface
+for MMC, SD and SDIO types of memory cards.
+
+Supported maximum speeds are the ones of the eMMC standard 4.5 as well
+as the speed of SD standard 3.0.
+Absolute maximum transfer rate is 200MB/s
+
+Required properties:
+- compatible: Should be "allwinner,<revision>-<chip>-mmc".
+  The supported chips include a10, a10s, 13, a20 and a31.
+- base registers are 0x1000 appart, so the base of mmc1
+  would be 0x01c0f000+0x1000=0x01c10000(see example)
+  and so on.
+- clocks requires the reference at the ahb clock gate
+  with the correct index (mmc0 -> 8, mmc1 -> 9, and so on)
+  as well as the reference to the correct mod0 clock.
+- interrupts requires the correct IRQ line
+  (mmc0 -> 32, mmc1 -> 33, and so on)
+
+Examples:
+
+mmc0: mmc@01c0f000 {
+	compatible = "allwinner,sun5i-a13-mmc";
+	reg = <0x01c0f000 0x1000>;
+	clocks = <&ahb_gates 8>, <&mmc0_clk>;
+	clock-names = "ahb", "mod";
+	interrupts = <0 32 4>;
+	bus-width = <4>;
+	status = "disabled";
+};