2014-02-17 10:03:47

by David Lanzendörfer

[permalink] [raw]
Subject: [PATCH v7 0/8] ARM: sunxi: Add driver for SD/MMC hosts found on allwinner sunxi SOCs

Hello
The following patchset adds support for the SD/MMC host found in the Allwinner SoCs.
It contains all the necessary modifications for clock environment and also the device
tree script modification which add it to all the boards using it.
The clock environment function needed for phase offset configuration has
been proposed and implemented by Emilio.
A lot of work and cleanup has been done by Hans de Goede. Special thanks to him!
This patchset is the 4th attempt to send this driver upstream.

Changes since v1:
-Using mmc_of_parse instead of diy dt parsing
-Adding nodes for all mmc controller to the dtsi files,
including sofar unused controllers
-Using generic GPIO slot library for WP/CD
-Adding additional MMC device nodes into DTSI files

Changes since v2:
-Add missing Signed-off-by tags
-Stop using __raw_readl / __raw_writel so that barriers are properly used
-Adding missing new lines
-Adding missing patch for automatic reparenting of clocks

Changes since v3:
-Move clk_enable / disable into host_init / exit (Hans)
-Fix hang on boot caused by irq storm (Hans)

Changes since v4:
-moving sunxi-mci.{c/h} to sunxi-mmc.{c/h}
-removing camel cases from the defines in sunxi-mmc.h
-moving defines out of the struct definition
since this is bad coding style
-adding documentation for the device tree binding

Changes since v5:
-adding host initialization for when the sdio irq is enabled
(just to make sure having a defined state at all time)
-add mmc support fixup: set pullup on cd pins
-fixup: Don't set MMC_CAP_NEEDS_POLL / MMC_CAP_4_BIT_DATA

Changes since v6:
-fixing copyright info in sunxi-mmc.*
-s/__SUNXI_MCI_H__/__SUNXI_MMC_H__/g
-s/SDXC_RESPONSE_/SDXC_RESP_/g
-s/define/definitions <- Comment from Priit Laes

---

David Lanzendörfer (5):
ARM: sunxi: Add driver for SD/MMC hosts found on Allwinner sunxi SoCs
ARM: dts: sun7i: Add support for mmc
ARM: dts: sun4i: Add support for mmc
ARM: dts: sun5i: Add support for mmc
ARM: sunxi: Add documentation for driver for SD/MMC hosts found on Allwinner sunxi SoCs

Emilio López (2):
clk: sunxi: factors: automatic reparenting support
clk: sunxi: Implement MMC phase control

Hans de Goede (1):
ARM: sunxi: clk: export clk_sunxi_mmc_phase_control


.../devicetree/bindings/mmc/sunxi-mmc.txt | 32 +
arch/arm/boot/dts/sun4i-a10-a1000.dts | 8
arch/arm/boot/dts/sun4i-a10-cubieboard.dts | 8
arch/arm/boot/dts/sun4i-a10.dtsi | 54 +
arch/arm/boot/dts/sun5i-a10s-olinuxino-micro.dts | 30 +
arch/arm/boot/dts/sun5i-a10s.dtsi | 44 +
arch/arm/boot/dts/sun5i-a13-olinuxino-micro.dts | 15
arch/arm/boot/dts/sun5i-a13-olinuxino.dts | 15
arch/arm/boot/dts/sun5i-a13.dtsi | 37 +
arch/arm/boot/dts/sun7i-a20-cubieboard2.dts | 8
arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 8
arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts | 23 +
arch/arm/boot/dts/sun7i-a20.dtsi | 61 +
drivers/clk/sunxi/clk-factors.c | 36 +
drivers/clk/sunxi/clk-sunxi.c | 35 +
drivers/mmc/host/Kconfig | 7
drivers/mmc/host/Makefile | 2
drivers/mmc/host/sunxi-mmc.c | 876 ++++++++++++++++++++
drivers/mmc/host/sunxi-mmc.h | 239 +++++
include/linux/clk/sunxi.h | 22 +
20 files changed, 1560 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
create mode 100644 drivers/mmc/host/sunxi-mmc.c
create mode 100644 drivers/mmc/host/sunxi-mmc.h
create mode 100644 include/linux/clk/sunxi.h

--
Signature


2014-02-17 10:03:54

by David Lanzendörfer

[permalink] [raw]
Subject: [PATCH v7 2/8] clk: sunxi: Implement MMC phase control

From: Emilio López <[email protected]>

Signed-off-by: Emilio López <[email protected]>
---
drivers/clk/sunxi/clk-sunxi.c | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)

diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
index abb6c5a..33b9977 100644
--- a/drivers/clk/sunxi/clk-sunxi.c
+++ b/drivers/clk/sunxi/clk-sunxi.c
@@ -377,6 +377,41 @@ static void sun7i_a20_get_out_factors(u32 *freq, u32 parent_rate,


/**
+ * clk_sunxi_mmc_phase_control() - configures MMC clock phase control
+ */
+
+void clk_sunxi_mmc_phase_control(struct clk_hw *hw, u8 sample, u8 output)
+{
+ #define to_clk_composite(_hw) container_of(_hw, struct clk_composite, hw)
+ #define to_clk_factors(_hw) container_of(_hw, struct clk_factors, hw)
+
+ struct clk_composite *composite = to_clk_composite(hw);
+ struct clk_hw *rate_hw = composite->rate_hw;
+ struct clk_factors *factors = to_clk_factors(rate_hw);
+ unsigned long flags = 0;
+ u32 reg;
+
+ if (factors->lock)
+ spin_lock_irqsave(factors->lock, flags);
+
+ reg = readl(factors->reg);
+
+ /* set sample clock phase control */
+ reg &= ~(0x7 << 20);
+ reg |= ((sample & 0x7) << 20);
+
+ /* set output clock phase control */
+ reg &= ~(0x7 << 8);
+ reg |= ((output & 0x7) << 8);
+
+ writel(reg, factors->reg);
+
+ if (factors->lock)
+ spin_unlock_irqrestore(factors->lock, flags);
+}
+
+
+/**
* sunxi_factors_clk_setup() - Setup function for factor clocks
*/

2014-02-17 10:04:03

by David Lanzendörfer

[permalink] [raw]
Subject: [PATCH v7 3/8] ARM: sunxi: clk: export clk_sunxi_mmc_phase_control

From: Hans de Goede <[email protected]>

Signed-off-by: Hans de Goede <[email protected]>
---
include/linux/clk/sunxi.h | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
create mode 100644 include/linux/clk/sunxi.h

diff --git a/include/linux/clk/sunxi.h b/include/linux/clk/sunxi.h
new file mode 100644
index 0000000..1ef5c89
--- /dev/null
+++ b/include/linux/clk/sunxi.h
@@ -0,0 +1,22 @@
+/*
+ * Copyright 2013 - Hans de Goede <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __LINUX_CLK_SUNXI_H_
+#define __LINUX_CLK_SUNXI_H_
+
+#include <linux/clk.h>
+
+void clk_sunxi_mmc_phase_control(struct clk_hw *hw, u8 sample, u8 output);
+
+#endif

2014-02-17 10:04:16

by David Lanzendörfer

[permalink] [raw]
Subject: [PATCH v7 5/8] ARM: dts: sun7i: Add support for mmc

Signed-off-by: David Lanzendörfer <[email protected]>
Signed-off-by: Hans de Goede <[email protected]>
---
arch/arm/boot/dts/sun7i-a20-cubieboard2.dts | 8 +++
arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 8 +++
arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts | 23 +++++++++
arch/arm/boot/dts/sun7i-a20.dtsi | 61 +++++++++++++++++++++++
4 files changed, 100 insertions(+)

diff --git a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
index 5c51cb8..ae800b6 100644
--- a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
+++ b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
@@ -34,6 +34,14 @@
};
};

+ mmc0: [email protected] {
+ pinctrl-names = "default", "default";
+ pinctrl-0 = <&mmc0_pins_a>;
+ pinctrl-1 = <&mmc0_cd_pin_reference_design>;
+ cd-gpios = <&pio 7 1 0>; /* PH1 */
+ status = "okay";
+ };
+
[email protected] {
led_pins_cubieboard2: [email protected] {
allwinner,pins = "PH20", "PH21";
diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
index f9dcb61..370cef84 100644
--- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
+++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
@@ -19,6 +19,14 @@
compatible = "cubietech,cubietruck", "allwinner,sun7i-a20";

[email protected] {
+ mmc0: [email protected] {
+ pinctrl-names = "default", "default";
+ pinctrl-0 = <&mmc0_pins_a>;
+ pinctrl-1 = <&mmc0_cd_pin_reference_design>;
+ cd-gpios = <&pio 7 1 0>; /* PH1 */
+ status = "okay";
+ };
+
[email protected] {
led_pins_cubietruck: [email protected] {
allwinner,pins = "PH7", "PH11", "PH20", "PH21";
diff --git a/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts b/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
index ead3013..685ec06 100644
--- a/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
+++ b/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
@@ -34,7 +34,30 @@
};
};

+ mmc0: [email protected] {
+ pinctrl-names = "default", "default";
+ pinctrl-0 = <&mmc0_pins_a>;
+ pinctrl-1 = <&mmc0_cd_pin_reference_design>;
+ cd-gpios = <&pio 7 1 0>; /* PH1 */
+ status = "okay";
+ };
+
+ mmc3: [email protected] {
+ pinctrl-names = "default", "default";
+ pinctrl-0 = <&mmc3_pins_a>;
+ pinctrl-1 = <&mmc3_cd_pin_olinuxinom>;
+ cd-gpios = <&pio 7 11 0>; /* PH11 */
+ status = "okay";
+ };
+
[email protected] {
+ mmc3_cd_pin_olinuxinom: [email protected] {
+ allwinner,pins = "PH11";
+ allwinner,function = "gpio_in";
+ allwinner,drive = <0>;
+ allwinner,pull = <1>;
+ };
+
led_pins_olinuxino: [email protected] {
allwinner,pins = "PH2";
allwinner,function = "gpio_out";
diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
index 9ff0948..5b55414 100644
--- a/arch/arm/boot/dts/sun7i-a20.dtsi
+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
@@ -355,6 +355,46 @@
#size-cells = <0>;
};

+ mmc0: [email protected] {
+ 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";
+ };
+
+ mmc1: [email protected] {
+ compatible = "allwinner,sun5i-a13-mmc";
+ reg = <0x01c10000 0x1000>;
+ clocks = <&ahb_gates 9>, <&mmc1_clk>;
+ clock-names = "ahb", "mod";
+ interrupts = <0 33 4>;
+ bus-width = <4>;
+ status = "disabled";
+ };
+
+ mmc2: [email protected] {
+ compatible = "allwinner,sun5i-a13-mmc";
+ reg = <0x01c11000 0x1000>;
+ clocks = <&ahb_gates 10>, <&mmc2_clk>;
+ clock-names = "ahb", "mod";
+ interrupts = <0 34 4>;
+ bus-width = <4>;
+ status = "disabled";
+ };
+
+ mmc3: [email protected] {
+ compatible = "allwinner,sun5i-a13-mmc";
+ reg = <0x01c12000 0x1000>;
+ clocks = <&ahb_gates 11>, <&mmc3_clk>;
+ clock-names = "ahb", "mod";
+ interrupts = <0 35 4>;
+ bus-width = <4>;
+ status = "disabled";
+ };
+
pio: [email protected] {
compatible = "allwinner,sun7i-a20-pinctrl";
reg = <0x01c20800 0x400>;
@@ -432,6 +472,27 @@
allwinner,drive = <0>;
allwinner,pull = <0>;
};
+
+ mmc0_pins_a: [email protected] {
+ allwinner,pins = "PF0","PF1","PF2","PF3","PF4","PF5";
+ allwinner,function = "mmc0";
+ allwinner,drive = <3>;
+ allwinner,pull = <0>;
+ };
+
+ mmc0_cd_pin_reference_design: [email protected] {
+ allwinner,pins = "PH1";
+ allwinner,function = "gpio_in";
+ allwinner,drive = <0>;
+ allwinner,pull = <1>;
+ };
+
+ mmc3_pins_a: [email protected] {
+ allwinner,pins = "PI4","PI5","PI6","PI7","PI8","PI9";
+ allwinner,function = "mmc3";
+ allwinner,drive = <3>;
+ allwinner,pull = <0>;
+ };
};

[email protected] {

2014-02-17 10:04:22

by David Lanzendörfer

[permalink] [raw]
Subject: [PATCH v7 6/8] ARM: dts: sun4i: Add support for mmc

Signed-off-by: David Lanzendörfer <[email protected]>
Signed-off-by: Hans de Goede <[email protected]>
---
arch/arm/boot/dts/sun4i-a10-a1000.dts | 8 ++++
arch/arm/boot/dts/sun4i-a10-cubieboard.dts | 8 ++++
arch/arm/boot/dts/sun4i-a10.dtsi | 54 ++++++++++++++++++++++++++++
3 files changed, 70 insertions(+)

diff --git a/arch/arm/boot/dts/sun4i-a10-a1000.dts b/arch/arm/boot/dts/sun4i-a10-a1000.dts
index d4b081d..a879ef3 100644
--- a/arch/arm/boot/dts/sun4i-a10-a1000.dts
+++ b/arch/arm/boot/dts/sun4i-a10-a1000.dts
@@ -35,6 +35,14 @@
};
};

+ mmc0: [email protected] {
+ pinctrl-names = "default", "default";
+ pinctrl-0 = <&mmc0_pins_a>;
+ pinctrl-1 = <&mmc0_cd_pin_reference_design>;
+ cd-gpios = <&pio 7 1 0>; /* PH1 */
+ status = "okay";
+ };
+
[email protected] {
emac_power_pin_a1000: [email protected] {
allwinner,pins = "PH15";
diff --git a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
index b139ee6..20b976a 100644
--- a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
+++ b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
@@ -33,6 +33,14 @@
};
};

+ mmc0: [email protected] {
+ pinctrl-names = "default", "default";
+ pinctrl-0 = <&mmc0_pins_a>;
+ pinctrl-1 = <&mmc0_cd_pin_reference_design>;
+ cd-gpios = <&pio 7 1 0>; /* PH1 */
+ status = "okay";
+ };
+
[email protected] {
led_pins_cubieboard: [email protected] {
allwinner,pins = "PH20", "PH21";
diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
index 9044c53..cd14961 100644
--- a/arch/arm/boot/dts/sun4i-a10.dtsi
+++ b/arch/arm/boot/dts/sun4i-a10.dtsi
@@ -330,6 +330,46 @@
#size-cells = <0>;
};

+ mmc0: [email protected] {
+ compatible = "allwinner,sun5i-a13-mmc";
+ reg = <0x01c0f000 0x1000>;
+ clocks = <&ahb_gates 8>, <&mmc0_clk>;
+ clock-names = "ahb", "mod";
+ interrupts = <32>;
+ bus-width = <4>;
+ status = "disabled";
+ };
+
+ mmc1: [email protected] {
+ compatible = "allwinner,sun5i-a13-mmc";
+ reg = <0x01c10000 0x1000>;
+ clocks = <&ahb_gates 9>, <&mmc1_clk>;
+ clock-names = "ahb", "mod";
+ interrupts = <33>;
+ bus-width = <4>;
+ status = "disabled";
+ };
+
+ mmc2: [email protected] {
+ compatible = "allwinner,sun5i-a13-mmc";
+ reg = <0x01c11000 0x1000>;
+ clocks = <&ahb_gates 10>, <&mmc2_clk>;
+ clock-names = "ahb", "mod";
+ interrupts = <34>;
+ bus-width = <4>;
+ status = "disabled";
+ };
+
+ mmc3: [email protected] {
+ compatible = "allwinner,sun5i-a13-mmc";
+ reg = <0x01c12000 0x1000>;
+ clocks = <&ahb_gates 11>, <&mmc3_clk>;
+ clock-names = "ahb", "mod";
+ interrupts = <35>;
+ bus-width = <4>;
+ status = "disabled";
+ };
+
intc: [email protected] {
compatible = "allwinner,sun4i-ic";
reg = <0x01c20400 0x400>;
@@ -400,6 +440,20 @@
allwinner,drive = <0>;
allwinner,pull = <0>;
};
+
+ mmc0_pins_a: [email protected] {
+ allwinner,pins = "PF0","PF1","PF2","PF3","PF4","PF5";
+ allwinner,function = "mmc0";
+ allwinner,drive = <3>;
+ allwinner,pull = <0>;
+ };
+
+ mmc0_cd_pin_reference_design: [email protected] {
+ allwinner,pins = "PH1";
+ allwinner,function = "gpio_in";
+ allwinner,drive = <0>;
+ allwinner,pull = <1>;
+ };
};

[email protected] {

2014-02-17 10:04:30

by David Lanzendörfer

[permalink] [raw]
Subject: [PATCH v7 7/8] ARM: dts: sun5i: Add support for mmc

Signed-off-by: David Lanzendörfer <[email protected]>
Signed-off-by: Hans de Goede <[email protected]>
---
arch/arm/boot/dts/sun5i-a10s-olinuxino-micro.dts | 30 +++++++++++++++
arch/arm/boot/dts/sun5i-a10s.dtsi | 44 ++++++++++++++++++++++
arch/arm/boot/dts/sun5i-a13-olinuxino-micro.dts | 15 ++++++++
arch/arm/boot/dts/sun5i-a13-olinuxino.dts | 15 ++++++++
arch/arm/boot/dts/sun5i-a13.dtsi | 37 +++++++++++++++++++
5 files changed, 141 insertions(+)

diff --git a/arch/arm/boot/dts/sun5i-a10s-olinuxino-micro.dts b/arch/arm/boot/dts/sun5i-a10s-olinuxino-micro.dts
index 3c9f8b3..5c7b454 100644
--- a/arch/arm/boot/dts/sun5i-a10s-olinuxino-micro.dts
+++ b/arch/arm/boot/dts/sun5i-a10s-olinuxino-micro.dts
@@ -34,7 +34,37 @@
};
};

+ mmc0: [email protected] {
+ pinctrl-names = "default", "default";
+ pinctrl-0 = <&mmc0_pins_a>;
+ pinctrl-1 = <&mmc0_cd_pin_olinuxino_micro>;
+ cd-gpios = <&pio 6 1 0>; /* PG1 */
+ status = "okay";
+ };
+
+ mmc1: [email protected] {
+ pinctrl-names = "default", "default";
+ pinctrl-0 = <&mmc1_pins_a>;
+ pinctrl-1 = <&mmc1_cd_pin_olinuxino_micro>;
+ cd-gpios = <&pio 6 13 0>; /* PG13 */
+ status = "okay";
+ };
+
[email protected] {
+ mmc0_cd_pin_olinuxino_micro: [email protected] {
+ allwinner,pins = "PG1";
+ allwinner,function = "gpio_in";
+ allwinner,drive = <0>;
+ allwinner,pull = <1>;
+ };
+
+ mmc1_cd_pin_olinuxino_micro: [email protected] {
+ allwinner,pins = "PG13";
+ allwinner,function = "gpio_in";
+ allwinner,drive = <0>;
+ allwinner,pull = <1>;
+ };
+
led_pins_olinuxino: [email protected] {
allwinner,pins = "PE3";
allwinner,function = "gpio_out";
diff --git a/arch/arm/boot/dts/sun5i-a10s.dtsi b/arch/arm/boot/dts/sun5i-a10s.dtsi
index 327e87b..b6d1de0 100644
--- a/arch/arm/boot/dts/sun5i-a10s.dtsi
+++ b/arch/arm/boot/dts/sun5i-a10s.dtsi
@@ -293,6 +293,36 @@
#size-cells = <0>;
};

+ mmc0: [email protected] {
+ compatible = "allwinner,sun5i-a13-mmc";
+ reg = <0x01c0f000 0x1000>;
+ clocks = <&ahb_gates 8>, <&mmc0_clk>;
+ clock-names = "ahb", "mod";
+ interrupts = <32>;
+ bus-width = <4>;
+ status = "disabled";
+ };
+
+ mmc1: [email protected] {
+ compatible = "allwinner,sun5i-a13-mmc";
+ reg = <0x01c10000 0x1000>;
+ clocks = <&ahb_gates 9>, <&mmc1_clk>;
+ clock-names = "ahb", "mod";
+ interrupts = <33>;
+ bus-width = <4>;
+ status = "disabled";
+ };
+
+ mmc2: [email protected] {
+ compatible = "allwinner,sun5i-a13-mmc";
+ reg = <0x01c11000 0x1000>;
+ clocks = <&ahb_gates 10>, <&mmc2_clk>;
+ clock-names = "ahb", "mod";
+ interrupts = <34>;
+ bus-width = <4>;
+ status = "disabled";
+ };
+
intc: [email protected] {
compatible = "allwinner,sun4i-ic";
reg = <0x01c20400 0x400>;
@@ -363,6 +393,20 @@
allwinner,drive = <0>;
allwinner,pull = <0>;
};
+
+ mmc0_pins_a: [email protected] {
+ allwinner,pins = "PF0","PF1","PF2","PF3","PF4","PF5";
+ allwinner,function = "mmc0";
+ allwinner,drive = <3>;
+ allwinner,pull = <0>;
+ };
+
+ mmc1_pins_a: [email protected] {
+ allwinner,pins = "PG3","PG4","PG5","PG6","PG7","PG8";
+ allwinner,function = "mmc1";
+ allwinner,drive = <3>;
+ allwinner,pull = <0>;
+ };
};

[email protected] {
diff --git a/arch/arm/boot/dts/sun5i-a13-olinuxino-micro.dts b/arch/arm/boot/dts/sun5i-a13-olinuxino-micro.dts
index fe2ce0a..2f08bb2 100644
--- a/arch/arm/boot/dts/sun5i-a13-olinuxino-micro.dts
+++ b/arch/arm/boot/dts/sun5i-a13-olinuxino-micro.dts
@@ -20,7 +20,22 @@
compatible = "olimex,a13-olinuxino-micro", "allwinner,sun5i-a13";

[email protected] {
+ mmc0: [email protected] {
+ pinctrl-names = "default", "default";
+ pinctrl-0 = <&mmc0_pins_a>;
+ pinctrl-1 = <&mmc0_cd_pin_olinuxinom>;
+ cd-gpios = <&pio 6 0 0>; /* PG0 */
+ status = "okay";
+ };
+
[email protected] {
+ mmc0_cd_pin_olinuxinom: [email protected] {
+ allwinner,pins = "PG0";
+ allwinner,function = "gpio_in";
+ allwinner,drive = <0>;
+ allwinner,pull = <1>;
+ };
+
led_pins_olinuxinom: [email protected] {
allwinner,pins = "PG9";
allwinner,function = "gpio_out";
diff --git a/arch/arm/boot/dts/sun5i-a13-olinuxino.dts b/arch/arm/boot/dts/sun5i-a13-olinuxino.dts
index a4ba5ff..a7280f5 100644
--- a/arch/arm/boot/dts/sun5i-a13-olinuxino.dts
+++ b/arch/arm/boot/dts/sun5i-a13-olinuxino.dts
@@ -19,7 +19,22 @@
compatible = "olimex,a13-olinuxino", "allwinner,sun5i-a13";

[email protected] {
+ mmc0: [email protected] {
+ pinctrl-names = "default", "default";
+ pinctrl-0 = <&mmc0_pins_a>;
+ pinctrl-1 = <&mmc0_cd_pin_olinuxino>;
+ cd-gpios = <&pio 6 0 0>; /* PG0 */
+ status = "okay";
+ };
+
[email protected] {
+ mmc0_cd_pin_olinuxino: [email protected] {
+ allwinner,pins = "PG0";
+ allwinner,function = "gpio_in";
+ allwinner,drive = <0>;
+ allwinner,pull = <1>;
+ };
+
led_pins_olinuxino: [email protected] {
allwinner,pins = "PG9";
allwinner,function = "gpio_out";
diff --git a/arch/arm/boot/dts/sun5i-a13.dtsi b/arch/arm/boot/dts/sun5i-a13.dtsi
index f49eb13..040d304 100644
--- a/arch/arm/boot/dts/sun5i-a13.dtsi
+++ b/arch/arm/boot/dts/sun5i-a13.dtsi
@@ -274,6 +274,36 @@
#size-cells = <1>;
ranges;

+ mmc0: [email protected] {
+ compatible = "allwinner,sun5i-a13-mmc";
+ reg = <0x01c0f000 0x1000>;
+ clocks = <&ahb_gates 8>, <&mmc0_clk>;
+ clock-names = "ahb", "mod";
+ interrupts = <32>;
+ bus-width = <4>;
+ status = "disabled";
+ };
+
+ mmc1: [email protected] {
+ compatible = "allwinner,sun5i-a13-mmc";
+ reg = <0x01c10000 0x1000>;
+ clocks = <&ahb_gates 9>, <&mmc1_clk>;
+ clock-names = "ahb", "mod";
+ interrupts = <33>;
+ bus-width = <4>;
+ status = "disabled";
+ };
+
+ mmc2: [email protected] {
+ compatible = "allwinner,sun5i-a13-mmc";
+ reg = <0x01c11000 0x1000>;
+ clocks = <&ahb_gates 10>, <&mmc2_clk>;
+ clock-names = "ahb", "mod";
+ interrupts = <34>;
+ bus-width = <4>;
+ status = "disabled";
+ };
+
intc: [email protected] {
compatible = "allwinner,sun4i-ic";
reg = <0x01c20400 0x400>;
@@ -326,6 +356,13 @@
allwinner,drive = <0>;
allwinner,pull = <0>;
};
+
+ mmc0_pins_a: [email protected] {
+ allwinner,pins = "PF0","PF1","PF2","PF3","PF4","PF5";
+ allwinner,function = "mmc0";
+ allwinner,drive = <3>;
+ allwinner,pull = <0>;
+ };
};

[email protected] {

2014-02-17 10:04:34

by David Lanzendörfer

[permalink] [raw]
Subject: [PATCH v7 8/8] ARM: sunxi: Add documentation for driver for SD/MMC hosts found on Allwinner sunxi SoCs

Signed-off-by: David Lanzendörfer <[email protected]>
---
.../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.
+- 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: [email protected] {
+ 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";
+};

2014-02-17 10:05:54

by David Lanzendörfer

[permalink] [raw]
Subject: [PATCH v7 4/8] ARM: sunxi: Add driver for SD/MMC hosts found on Allwinner sunxi SoCs

This is based on the driver Allwinner ships in their Android kernel sources.

Initial porting to upstream kernels done by David Lanzendörfer, additional
fixes and cleanups by Hans de Goede.

It uses dma in bus-master mode using a built-in designware idmac controller,
which is identical to the one found in the mmc-dw hosts.
The rest of the host is not identical to mmc-dw.

Signed-off-by: David Lanzendörfer <[email protected]>
Signed-off-by: Hans de Goede <[email protected]>
---
drivers/mmc/host/Kconfig | 7
drivers/mmc/host/Makefile | 2
drivers/mmc/host/sunxi-mmc.c | 876 ++++++++++++++++++++++++++++++++++++++++++
drivers/mmc/host/sunxi-mmc.h | 239 +++++++++++
4 files changed, 1124 insertions(+)
create mode 100644 drivers/mmc/host/sunxi-mmc.c
create mode 100644 drivers/mmc/host/sunxi-mmc.h

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 1384f67..7caf266 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -689,3 +689,10 @@ config MMC_REALTEK_PCI
help
Say Y here to include driver code to support SD/MMC card interface
of Realtek PCI-E card reader
+
+config MMC_SUNXI
+ tristate "Allwinner sunxi SD/MMC Host Controller support"
+ depends on ARCH_SUNXI
+ help
+ This selects support for the SD/MMC Host Controller on
+ Allwinner sunxi SoCs.
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 3483b6b..f3c7c243 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -54,6 +54,8 @@ obj-$(CONFIG_MMC_WMT) += wmt-sdmmc.o

obj-$(CONFIG_MMC_REALTEK_PCI) += rtsx_pci_sdmmc.o

+obj-$(CONFIG_MMC_SUNXI) += sunxi-mmc.o
+
obj-$(CONFIG_MMC_SDHCI_PLTFM) += sdhci-pltfm.o
obj-$(CONFIG_MMC_SDHCI_CNS3XXX) += sdhci-cns3xxx.o
obj-$(CONFIG_MMC_SDHCI_ESDHC_IMX) += sdhci-esdhc-imx.o
diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
new file mode 100644
index 0000000..752e913
--- /dev/null
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -0,0 +1,876 @@
+/*
+ * Driver for sunxi SD/MMC host controllers
+ * (C) Copyright 2007-2011 Reuuimlla Technology Co., Ltd.
+ * (C) Copyright 2007-2011 Aaron Maoye <[email protected]>
+ * (C) Copyright 2013-2014 O2S GmbH <http://www.o2s.ch>
+ * (C) Copyright 2013-2014 David Lanzend?rfer <[email protected]>
+ * (C) Copyright 2013-2014 Hans de Goede <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+
+#include <linux/clk.h>
+#include <linux/clk-private.h>
+#include <linux/clk/sunxi.h>
+
+#include <linux/gpio.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/scatterlist.h>
+#include <linux/dma-mapping.h>
+#include <linux/slab.h>
+#include <linux/regulator/consumer.h>
+
+#include <linux/of_address.h>
+#include <linux/of_gpio.h>
+#include <linux/of_platform.h>
+
+#include <linux/mmc/host.h>
+#include <linux/mmc/sd.h>
+#include <linux/mmc/sdio.h>
+#include <linux/mmc/mmc.h>
+#include <linux/mmc/core.h>
+#include <linux/mmc/card.h>
+#include <linux/mmc/slot-gpio.h>
+
+#include "sunxi-mmc.h"
+
+static int sunxi_mmc_init_host(struct mmc_host *mmc)
+{
+ u32 rval;
+ struct sunxi_mmc_host *smc_host = mmc_priv(mmc);
+ int ret;
+
+ ret = clk_prepare_enable(smc_host->clk_ahb);
+ if (ret) {
+ dev_err(mmc_dev(smc_host->mmc), "AHB clk err %d\n", ret);
+ return ret;
+ }
+ ret = clk_prepare_enable(smc_host->clk_mod);
+ if (ret) {
+ dev_err(mmc_dev(smc_host->mmc), "MOD clk err %d\n", ret);
+ clk_disable_unprepare(smc_host->clk_ahb);
+ return ret;
+ }
+
+ /* reset controller */
+ rval = mci_readl(smc_host, REG_GCTRL) | SDXC_HARDWARE_RESET;
+ mci_writel(smc_host, REG_GCTRL, rval);
+
+ mci_writel(smc_host, REG_FTRGL, 0x20070008);
+ mci_writel(smc_host, REG_TMOUT, 0xffffffff);
+ mci_writel(smc_host, REG_IMASK, smc_host->sdio_imask);
+ mci_writel(smc_host, REG_RINTR, 0xffffffff);
+ mci_writel(smc_host, REG_DBGC, 0xdeb);
+ mci_writel(smc_host, REG_FUNS, 0xceaa0000);
+ mci_writel(smc_host, REG_DLBA, smc_host->sg_dma);
+ rval = mci_readl(smc_host, REG_GCTRL)|SDXC_INTERRUPT_ENABLE_BIT;
+ rval &= ~SDXC_ACCESS_DONE_DIRECT;
+ mci_writel(smc_host, REG_GCTRL, rval);
+
+ return 0;
+}
+
+static void sunxi_mmc_exit_host(struct sunxi_mmc_host *smc_host)
+{
+ mci_writel(smc_host, REG_GCTRL, SDXC_HARDWARE_RESET);
+ clk_disable_unprepare(smc_host->clk_ahb);
+ clk_disable_unprepare(smc_host->clk_mod);
+}
+
+/* /\* UHS-I Operation Modes */
+/* * DS 25MHz 12.5MB/s 3.3V */
+/* * HS 50MHz 25MB/s 3.3V */
+/* * SDR12 25MHz 12.5MB/s 1.8V */
+/* * SDR25 50MHz 25MB/s 1.8V */
+/* * SDR50 100MHz 50MB/s 1.8V */
+/* * SDR104 208MHz 104MB/s 1.8V */
+/* * DDR50 50MHz 50MB/s 1.8V */
+/* * MMC Operation Modes */
+/* * DS 26MHz 26MB/s 3/1.8/1.2V */
+/* * HS 52MHz 52MB/s 3/1.8/1.2V */
+/* * HSDDR 52MHz 104MB/s 3/1.8/1.2V */
+/* * HS200 200MHz 200MB/s 1.8/1.2V */
+/* * */
+/* * Spec. Timing */
+/* * SD3.0 */
+/* * Fcclk Tcclk Fsclk Tsclk Tis Tih odly RTis RTih */
+/* * 400K 2.5us 24M 41ns 5ns 5ns 1 2209ns 41ns */
+/* * 25M 40ns 600M 1.67ns 5ns 5ns 3 14.99ns 5.01ns */
+/* * 50M 20ns 600M 1.67ns 6ns 2ns 3 14.99ns 5.01ns */
+/* * 50MDDR 20ns 600M 1.67ns 6ns 0.8ns 2 6.67ns 3.33ns */
+/* * 104M 9.6ns 600M 1.67ns 3ns 0.8ns 1 7.93ns 1.67ns */
+/* * 208M 4.8ns 600M 1.67ns 1.4ns 0.8ns 1 3.33ns 1.67ns */
+
+/* * 25M 40ns 300M 3.33ns 5ns 5ns 2 13.34ns 6.66ns */
+/* * 50M 20ns 300M 3.33ns 6ns 2ns 2 13.34ns 6.66ns */
+/* * 50MDDR 20ns 300M 3.33ns 6ns 0.8ns 1 6.67ns 3.33ns */
+/* * 104M 9.6ns 300M 3.33ns 3ns 0.8ns 0 7.93ns 1.67ns */
+/* * 208M 4.8ns 300M 3.33ns 1.4ns 0.8ns 0 3.13ns 1.67ns */
+
+/* * eMMC4.5 */
+/* * 400K 2.5us 24M 41ns 3ns 3ns 1 2209ns 41ns */
+/* * 25M 40ns 600M 1.67ns 3ns 3ns 3 14.99ns 5.01ns */
+/* * 50M 20ns 600M 1.67ns 3ns 3ns 3 14.99ns 5.01ns */
+/* * 50MDDR 20ns 600M 1.67ns 2.5ns 2.5ns 2 6.67ns 3.33ns */
+/* * 200M 5ns 600M 1.67ns 1.4ns 0.8ns 1 3.33ns 1.67ns */
+/* *\/ */
+
+static void sunxi_mmc_init_idma_des(struct sunxi_mmc_host *host,
+ struct mmc_data *data)
+{
+ struct sunxi_idma_des *pdes = (struct sunxi_idma_des *)host->sg_cpu;
+ struct sunxi_idma_des *pdes_pa = (struct sunxi_idma_des *)host->sg_dma;
+ int i, max_len = (1 << host->idma_des_size_bits);
+
+ for (i = 0; i < data->sg_len; i++) {
+ pdes[i].config = SDXC_IDMAC_DES0_CH | SDXC_IDMAC_DES0_OWN |
+ SDXC_IDMAC_DES0_DIC;
+
+ if (data->sg[i].length == max_len)
+ pdes[i].buf_size = 0; /* 0 == max_len */
+ else
+ pdes[i].buf_size = data->sg[i].length;
+
+ pdes[i].buf_addr_ptr1 = sg_dma_address(&data->sg[i]);
+ pdes[i].buf_addr_ptr2 = (u32)&pdes_pa[i + 1];
+ }
+
+ pdes[0].config |= SDXC_IDMAC_DES0_FD;
+ pdes[i - 1].config = SDXC_IDMAC_DES0_OWN | SDXC_IDMAC_DES0_LD;
+
+ wmb(); /* Ensure idma_des hit main mem before we start the idmac */
+}
+
+static enum dma_data_direction sunxi_mmc_get_dma_dir(struct mmc_data *data)
+{
+ if (data->flags & MMC_DATA_WRITE)
+ return DMA_TO_DEVICE;
+ else
+ return DMA_FROM_DEVICE;
+}
+
+static int sunxi_mmc_prepare_dma(struct sunxi_mmc_host *smc_host,
+ struct mmc_data *data)
+{
+ u32 dma_len;
+ u32 i;
+ u32 temp;
+ struct scatterlist *sg;
+
+ dma_len = dma_map_sg(mmc_dev(smc_host->mmc), data->sg, data->sg_len,
+ sunxi_mmc_get_dma_dir(data));
+ if (dma_len == 0) {
+ dev_err(mmc_dev(smc_host->mmc), "dma_map_sg failed\n");
+ return -ENOMEM;
+ }
+
+ for_each_sg(data->sg, sg, data->sg_len, i) {
+ if (sg->offset & 3 || sg->length & 3) {
+ dev_err(mmc_dev(smc_host->mmc),
+ "unaligned scatterlist: os %x length %d\n",
+ sg->offset, sg->length);
+ return -EINVAL;
+ }
+ }
+
+ sunxi_mmc_init_idma_des(smc_host, data);
+
+ temp = mci_readl(smc_host, REG_GCTRL);
+ temp |= SDXC_DMA_ENABLE_BIT;
+ mci_writel(smc_host, REG_GCTRL, temp);
+ temp |= SDXC_DMA_RESET;
+ mci_writel(smc_host, REG_GCTRL, temp);
+ mci_writel(smc_host, REG_DMAC, SDXC_IDMAC_SOFT_RESET);
+
+ if (!(data->flags & MMC_DATA_WRITE))
+ mci_writel(smc_host, REG_IDIE, SDXC_IDMAC_RECEIVE_INTERRUPT);
+
+ mci_writel(smc_host, REG_DMAC, SDXC_IDMAC_FIX_BURST | SDXC_IDMAC_IDMA_ON);
+
+ return 0;
+}
+
+static void sunxi_mmc_send_manual_stop(struct sunxi_mmc_host *host,
+ struct mmc_request *req)
+{
+ u32 cmd_val = SDXC_START | SDXC_RESP_EXPIRE | SDXC_STOP_ABORT_CMD
+ | SDXC_CHECK_RESPONSE_CRC | MMC_STOP_TRANSMISSION;
+ u32 ri = 0;
+ unsigned long expire = jiffies + msecs_to_jiffies(1000);
+
+ mci_writel(host, REG_CARG, 0);
+ mci_writel(host, REG_CMDR, cmd_val);
+
+ do {
+ ri = mci_readl(host, REG_RINTR);
+ } while (!(ri & (SDXC_COMMAND_DONE | SDXC_INTERRUPT_ERROR_BIT)) &&
+ time_before(jiffies, expire));
+
+ if (ri & SDXC_INTERRUPT_ERROR_BIT) {
+ dev_err(mmc_dev(host->mmc), "send stop command failed\n");
+ if (req->stop)
+ req->stop->resp[0] = -ETIMEDOUT;
+ } else {
+ if (req->stop)
+ req->stop->resp[0] = mci_readl(host, REG_RESP0);
+ }
+
+ mci_writel(host, REG_RINTR, 0xffff);
+}
+
+static void sunxi_mmc_dump_errinfo(struct sunxi_mmc_host *smc_host)
+{
+ struct mmc_command *cmd = smc_host->mrq->cmd;
+ struct mmc_data *data = smc_host->mrq->data;
+
+ /* For some cmds timeout is normal with sd/mmc cards */
+ if ((smc_host->int_sum & SDXC_INTERRUPT_ERROR_BIT) == SDXC_RESP_TIMEOUT &&
+ (cmd->opcode == SD_IO_SEND_OP_COND || cmd->opcode == SD_IO_RW_DIRECT))
+ return;
+
+ dev_err(mmc_dev(smc_host->mmc),
+ "smc %d err, cmd %d,%s%s%s%s%s%s%s%s%s%s !!\n",
+ smc_host->mmc->index, cmd->opcode,
+ data ? (data->flags & MMC_DATA_WRITE ? " WR" : " RD") : "",
+ smc_host->int_sum & SDXC_RESP_ERROR ? " RE" : "",
+ smc_host->int_sum & SDXC_RESP_CRC_ERROR ? " RCE" : "",
+ smc_host->int_sum & SDXC_DATA_CRC_ERROR ? " DCE" : "",
+ smc_host->int_sum & SDXC_RESP_TIMEOUT ? " RTO" : "",
+ smc_host->int_sum & SDXC_DATA_TIMEOUT ? " DTO" : "",
+ smc_host->int_sum & SDXC_FIFO_RUN_ERROR ? " FE" : "",
+ smc_host->int_sum & SDXC_HARD_WARE_LOCKED ? " HL" : "",
+ smc_host->int_sum & SDXC_START_BIT_ERROR ? " SBE" : "",
+ smc_host->int_sum & SDXC_END_BIT_ERROR ? " EBE" : ""
+ );
+}
+
+static void sunxi_mmc_finalize_request(struct sunxi_mmc_host *host)
+{
+ struct mmc_request *mrq;
+ unsigned long iflags;
+
+ spin_lock_irqsave(&host->lock, iflags);
+
+ mrq = host->mrq;
+ if (!mrq) {
+ spin_unlock_irqrestore(&host->lock, iflags);
+ dev_err(mmc_dev(host->mmc), "no request to finalize\n");
+ return;
+ }
+
+ if (host->int_sum & SDXC_INTERRUPT_ERROR_BIT) {
+ sunxi_mmc_dump_errinfo(host);
+ mrq->cmd->error = -ETIMEDOUT;
+ if (mrq->data)
+ mrq->data->error = -ETIMEDOUT;
+ if (mrq->stop)
+ mrq->stop->error = -ETIMEDOUT;
+ } else {
+ if (mrq->cmd->flags & MMC_RSP_136) {
+ mrq->cmd->resp[0] = mci_readl(host, REG_RESP3);
+ mrq->cmd->resp[1] = mci_readl(host, REG_RESP2);
+ mrq->cmd->resp[2] = mci_readl(host, REG_RESP1);
+ mrq->cmd->resp[3] = mci_readl(host, REG_RESP0);
+ } else {
+ mrq->cmd->resp[0] = mci_readl(host, REG_RESP0);
+ }
+ if (mrq->data)
+ mrq->data->bytes_xfered =
+ mrq->data->blocks * mrq->data->blksz;
+ }
+
+ if (mrq->data) {
+ struct mmc_data *data = mrq->data;
+ u32 temp;
+
+ mci_writel(host, REG_IDST, 0x337);
+ mci_writel(host, REG_DMAC, 0);
+ temp = mci_readl(host, REG_GCTRL);
+ mci_writel(host, REG_GCTRL, temp|SDXC_DMA_RESET);
+ temp &= ~SDXC_DMA_ENABLE_BIT;
+ mci_writel(host, REG_GCTRL, temp);
+ temp |= SDXC_FIFO_RESET;
+ mci_writel(host, REG_GCTRL, temp);
+ dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
+ sunxi_mmc_get_dma_dir(data));
+ }
+
+ mci_writel(host, REG_RINTR, 0xffff);
+
+ dev_dbg(mmc_dev(host->mmc), "req done, resp %08x %08x %08x %08x\n",
+ mrq->cmd->resp[0], mrq->cmd->resp[1],
+ mrq->cmd->resp[2], mrq->cmd->resp[3]);
+
+ host->mrq = NULL;
+ host->int_sum = 0;
+ host->wait_dma = 0;
+
+ spin_unlock_irqrestore(&host->lock, iflags);
+
+ if (mrq->data && mrq->data->error) {
+ dev_err(mmc_dev(host->mmc),
+ "data error, sending stop command\n");
+ sunxi_mmc_send_manual_stop(host, mrq);
+ }
+
+ mmc_request_done(host->mmc, mrq);
+}
+
+static irqreturn_t sunxi_mmc_irq(int irq, void *dev_id)
+{
+ struct sunxi_mmc_host *host = dev_id;
+ u32 finalize = 0;
+ u32 sdio_int = 0;
+ u32 msk_int;
+ u32 idma_int;
+
+ spin_lock(&host->lock);
+
+ idma_int = mci_readl(host, REG_IDST);
+ msk_int = mci_readl(host, REG_MISTA);
+
+ dev_dbg(mmc_dev(host->mmc), "irq: rq %p mi %08x idi %08x\n",
+ host->mrq, msk_int, idma_int);
+
+ if (host->mrq) {
+ if (idma_int & SDXC_IDMAC_RECEIVE_INTERRUPT)
+ host->wait_dma = 0;
+
+ host->int_sum |= msk_int;
+
+ /* Wait for COMMAND_DONE on RESPONSE_TIMEOUT before finishing the req */
+ if ((host->int_sum & SDXC_RESP_TIMEOUT) &&
+ !(host->int_sum & SDXC_COMMAND_DONE))
+ mci_writel(host, REG_IMASK,
+ host->sdio_imask | SDXC_COMMAND_DONE);
+ else if (host->int_sum & SDXC_INTERRUPT_ERROR_BIT)
+ finalize = 1; /* Don't wait for dma on error */
+ else if (host->int_sum & SDXC_INTERRUPT_DONE_BIT && !host->wait_dma)
+ finalize = 1; /* Done */
+
+ if (finalize) {
+ mci_writel(host, REG_IMASK, host->sdio_imask);
+ mci_writel(host, REG_IDIE, 0);
+ }
+ }
+
+ if (msk_int & SDXC_SDIO_INTERRUPT)
+ sdio_int = 1;
+
+ mci_writel(host, REG_RINTR, msk_int);
+ mci_writel(host, REG_IDST, idma_int);
+
+ spin_unlock(&host->lock);
+
+ if (finalize)
+ tasklet_schedule(&host->tasklet);
+
+ if (sdio_int)
+ mmc_signal_sdio_irq(host->mmc);
+
+ return IRQ_HANDLED;
+}
+
+static void sunxi_mmc_tasklet(unsigned long data)
+{
+ struct sunxi_mmc_host *smc_host = (struct sunxi_mmc_host *) data;
+ sunxi_mmc_finalize_request(smc_host);
+}
+
+static void sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32 oclk_en)
+{
+ unsigned long expire = jiffies + msecs_to_jiffies(2000);
+ u32 rval;
+
+ rval = mci_readl(host, REG_CLKCR);
+ rval &= ~(SDXC_CARD_CLOCK_ON | SDXC_LOW_POWER_ON);
+
+ if (oclk_en)
+ rval |= SDXC_CARD_CLOCK_ON;
+
+ if (!host->io_flag)
+ rval |= SDXC_LOW_POWER_ON;
+
+ mci_writel(host, REG_CLKCR, rval);
+
+ rval = SDXC_START | SDXC_UPCLK_ONLY | SDXC_WAIT_PRE_OVER;
+ if (host->voltage_switching)
+ rval |= SDXC_VOLTAGE_SWITCH;
+ mci_writel(host, REG_CMDR, rval);
+
+ do {
+ rval = mci_readl(host, REG_CMDR);
+ } while (time_before(jiffies, expire) && (rval & SDXC_START));
+
+ if (rval & SDXC_START) {
+ dev_err(mmc_dev(host->mmc), "fatal err update clk timeout\n");
+ host->ferror = 1;
+ }
+}
+
+static void sunxi_mmc_set_clk_dly(struct sunxi_mmc_host *smc_host,
+ u32 oclk_dly, u32 sclk_dly)
+{
+ unsigned long iflags;
+ struct clk_hw *hw = __clk_get_hw(smc_host->clk_mod);
+
+ spin_lock_irqsave(&smc_host->lock, iflags);
+ clk_sunxi_mmc_phase_control(hw, sclk_dly, oclk_dly);
+ spin_unlock_irqrestore(&smc_host->lock, iflags);
+}
+
+struct sunxi_mmc_clk_dly mmc_clk_dly[MMC_CLK_MOD_NUM] = {
+ { MMC_CLK_400K, 0, 7 },
+ { MMC_CLK_25M, 0, 5 },
+ { MMC_CLK_50M, 3, 5 },
+ { MMC_CLK_50MDDR, 2, 4 },
+ { MMC_CLK_50MDDR_8BIT, 2, 4 },
+ { MMC_CLK_100M, 1, 4 },
+ { MMC_CLK_200M, 1, 4 },
+};
+
+static void sunxi_mmc_clk_set_rate(struct sunxi_mmc_host *smc_host,
+ unsigned int rate)
+{
+ u32 newrate;
+ u32 src_clk;
+ u32 oclk_dly;
+ u32 sclk_dly;
+ u32 temp;
+ struct sunxi_mmc_clk_dly *dly = NULL;
+
+ newrate = clk_round_rate(smc_host->clk_mod, rate);
+ if (smc_host->clk_mod_rate == newrate) {
+ dev_dbg(mmc_dev(smc_host->mmc), "clk already %d, rounded %d\n",
+ rate, newrate);
+ return;
+ }
+
+ dev_dbg(mmc_dev(smc_host->mmc), "setting clk to %d, rounded %d\n",
+ rate, newrate);
+
+ /* setting clock rate */
+ clk_disable(smc_host->clk_mod);
+ clk_set_rate(smc_host->clk_mod, newrate);
+ clk_enable(smc_host->clk_mod);
+ smc_host->clk_mod_rate = newrate = clk_get_rate(smc_host->clk_mod);
+ dev_dbg(mmc_dev(smc_host->mmc), "clk is now %d\n", newrate);
+
+ sunxi_mmc_oclk_onoff(smc_host, 0);
+ /* clear internal divider */
+ temp = mci_readl(smc_host, REG_CLKCR);
+ temp &= ~0xff;
+ mci_writel(smc_host, REG_CLKCR, temp);
+
+ /* determine delays */
+ if (rate <= 400000) {
+ dly = &mmc_clk_dly[MMC_CLK_400K];
+ } else if (rate <= 25000000) {
+ dly = &mmc_clk_dly[MMC_CLK_25M];
+ } else if (rate <= 50000000) {
+ if (smc_host->ddr) {
+ if (smc_host->bus_width == 8)
+ dly = &mmc_clk_dly[MMC_CLK_50MDDR_8BIT];
+ else
+ dly = &mmc_clk_dly[MMC_CLK_50MDDR];
+ } else {
+ dly = &mmc_clk_dly[MMC_CLK_50M];
+ }
+ } else if (rate <= 104000000) {
+ dly = &mmc_clk_dly[MMC_CLK_100M];
+ } else if (rate <= 208000000) {
+ dly = &mmc_clk_dly[MMC_CLK_200M];
+ } else {
+ dly = &mmc_clk_dly[MMC_CLK_50M];
+ }
+
+ oclk_dly = dly->oclk_dly;
+ sclk_dly = dly->sclk_dly;
+
+ src_clk = clk_get_rate(clk_get_parent(smc_host->clk_mod));
+
+ if (src_clk >= 300000000 && src_clk <= 400000000) {
+ if (oclk_dly)
+ oclk_dly--;
+ if (sclk_dly)
+ sclk_dly--;
+ }
+
+ sunxi_mmc_set_clk_dly(smc_host, oclk_dly, sclk_dly);
+ sunxi_mmc_oclk_onoff(smc_host, 1);
+
+ /* oclk_onoff sets various irq status bits, clear these */
+ mci_writel(smc_host, REG_RINTR,
+ mci_readl(smc_host, REG_RINTR) & ~SDXC_SDIO_INTERRUPT);
+}
+
+static void sunxi_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+ struct sunxi_mmc_host *host = mmc_priv(mmc);
+ u32 temp;
+ s32 err;
+
+ /* Set the power state */
+ switch (ios->power_mode) {
+ case MMC_POWER_ON:
+ break;
+
+ case MMC_POWER_UP:
+ if (!IS_ERR(host->vmmc)) {
+ mmc_regulator_set_ocr(host->mmc, host->vmmc, ios->vdd);
+ udelay(200);
+ }
+
+ err = sunxi_mmc_init_host(mmc);
+ if (err) {
+ host->ferror = 1;
+ return;
+ }
+ enable_irq(host->irq);
+
+ dev_dbg(mmc_dev(host->mmc), "power on!\n");
+ host->ferror = 0;
+ break;
+
+ case MMC_POWER_OFF:
+ dev_dbg(mmc_dev(host->mmc), "power off!\n");
+ disable_irq(host->irq);
+ sunxi_mmc_exit_host(host);
+ if (!IS_ERR(host->vmmc))
+ mmc_regulator_set_ocr(host->mmc, host->vmmc, 0);
+ host->ferror = 0;
+ break;
+ }
+
+ /* set bus width */
+ switch (ios->bus_width) {
+ case MMC_BUS_WIDTH_1:
+ mci_writel(host, REG_WIDTH, SDXC_WIDTH1);
+ host->bus_width = 1;
+ break;
+ case MMC_BUS_WIDTH_4:
+ mci_writel(host, REG_WIDTH, SDXC_WIDTH4);
+ host->bus_width = 4;
+ break;
+ case MMC_BUS_WIDTH_8:
+ mci_writel(host, REG_WIDTH, SDXC_WIDTH8);
+ host->bus_width = 8;
+ break;
+ }
+
+ /* set ddr mode */
+ temp = mci_readl(host, REG_GCTRL);
+ if (ios->timing == MMC_TIMING_UHS_DDR50) {
+ temp |= SDXC_DDR_MODE;
+ host->ddr = 1;
+ } else {
+ temp &= ~SDXC_DDR_MODE;
+ host->ddr = 0;
+ }
+ mci_writel(host, REG_GCTRL, temp);
+
+ /* set up clock */
+ if (ios->clock && ios->power_mode) {
+ dev_dbg(mmc_dev(host->mmc), "ios->clock: %d\n", ios->clock);
+ sunxi_mmc_clk_set_rate(host, ios->clock);
+ usleep_range(50000, 55000);
+ }
+}
+
+static void sunxi_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
+{
+ struct sunxi_mmc_host *smc_host = mmc_priv(mmc);
+ unsigned long flags;
+ int ret;
+ u32 imask;
+
+ spin_lock_irqsave(&smc_host->lock, flags);
+
+ /* Make sure the controller is in a sane state before enabling irqs */
+ ret = sunxi_mmc_init_host(mmc);
+ if (ret) {
+ spin_unlock_irqrestore(&smc_host->lock, flags);
+ return;
+ }
+
+ imask = mci_readl(smc_host, REG_IMASK);
+ if (enable) {
+ smc_host->sdio_imask = SDXC_SDIO_INTERRUPT;
+ imask |= SDXC_SDIO_INTERRUPT;
+ } else {
+ smc_host->sdio_imask = 0;
+ imask &= ~SDXC_SDIO_INTERRUPT;
+ }
+ mci_writel(smc_host, REG_IMASK, imask);
+ spin_unlock_irqrestore(&smc_host->lock, flags);
+}
+
+static void sunxi_mmc_hw_reset(struct mmc_host *mmc)
+{
+ struct sunxi_mmc_host *smc_host = mmc_priv(mmc);
+ mci_writel(smc_host, REG_HWRST, 0);
+ udelay(10);
+ mci_writel(smc_host, REG_HWRST, 1);
+ udelay(300);
+}
+
+static void sunxi_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq)
+{
+ struct sunxi_mmc_host *host = mmc_priv(mmc);
+ struct mmc_command *cmd = mrq->cmd;
+ struct mmc_data *data = mrq->data;
+ unsigned long iflags;
+ u32 imask = SDXC_INTERRUPT_ERROR_BIT;
+ u32 cmd_val = SDXC_START | (cmd->opcode & 0x3f);
+ u32 byte_cnt = 0;
+ int ret;
+
+ if (!mmc_gpio_get_cd(mmc) || host->ferror) {
+ dev_dbg(mmc_dev(host->mmc), "no medium present\n");
+ mrq->cmd->error = -ENOMEDIUM;
+ mmc_request_done(mmc, mrq);
+ return;
+ }
+
+ if (data) {
+ byte_cnt = data->blksz * data->blocks;
+ mci_writel(host, REG_BLKSZ, data->blksz);
+ mci_writel(host, REG_BCNTR, byte_cnt);
+ ret = sunxi_mmc_prepare_dma(host, data);
+ if (ret < 0) {
+ dev_err(mmc_dev(host->mmc), "prepare DMA failed\n");
+ cmd->error = ret;
+ cmd->data->error = ret;
+ mmc_request_done(host->mmc, mrq);
+ return;
+ }
+ }
+
+ if (cmd->opcode == MMC_GO_IDLE_STATE) {
+ cmd_val |= SDXC_SEND_INIT_SEQUENCE;
+ imask |= SDXC_COMMAND_DONE;
+ }
+
+ if (cmd->opcode == SD_SWITCH_VOLTAGE) {
+ cmd_val |= SDXC_VOLTAGE_SWITCH;
+ imask |= SDXC_VOLTAGE_CHANGE_DONE;
+ host->voltage_switching = 1;
+ sunxi_mmc_oclk_onoff(host, 1);
+ }
+
+ if (cmd->flags & MMC_RSP_PRESENT) {
+ cmd_val |= SDXC_RESP_EXPIRE;
+ if (cmd->flags & MMC_RSP_136)
+ cmd_val |= SDXC_LONG_RESPONSE;
+ if (cmd->flags & MMC_RSP_CRC)
+ cmd_val |= SDXC_CHECK_RESPONSE_CRC;
+
+ if ((cmd->flags & MMC_CMD_MASK) == MMC_CMD_ADTC) {
+ cmd_val |= SDXC_DATA_EXPIRE | SDXC_WAIT_PRE_OVER;
+ if (cmd->data->flags & MMC_DATA_STREAM) {
+ imask |= SDXC_AUTO_COMMAND_DONE;
+ cmd_val |= SDXC_SEQUENCE_MODE | SDXC_SEND_AUTO_STOP;
+ }
+ if (cmd->data->stop) {
+ imask |= SDXC_AUTO_COMMAND_DONE;
+ cmd_val |= SDXC_SEND_AUTO_STOP;
+ } else
+ imask |= SDXC_DATA_OVER;
+
+ if (cmd->data->flags & MMC_DATA_WRITE)
+ cmd_val |= SDXC_WRITE;
+ else
+ host->wait_dma = 1;
+ } else
+ imask |= SDXC_COMMAND_DONE;
+ } else
+ imask |= SDXC_COMMAND_DONE;
+
+ dev_dbg(mmc_dev(host->mmc), "cmd %d(%08x) arg %x ie 0x%08x len %d\n",
+ cmd_val & 0x3f, cmd_val, cmd->arg, imask,
+ mrq->data ? mrq->data->blksz * mrq->data->blocks : 0);
+
+ spin_lock_irqsave(&host->lock, iflags);
+ host->mrq = mrq;
+ mci_writel(host, REG_IMASK, host->sdio_imask | imask);
+ spin_unlock_irqrestore(&host->lock, iflags);
+
+ mci_writel(host, REG_CARG, cmd->arg);
+ mci_writel(host, REG_CMDR, cmd_val);
+}
+
+static const struct of_device_id sunxi_mmc_of_match[] = {
+ { .compatible = "allwinner,sun4i-a10-mmc", },
+ { .compatible = "allwinner,sun5i-a13-mmc", },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, sunxi_mmc_of_match);
+
+static struct mmc_host_ops sunxi_mmc_ops = {
+ .request = sunxi_mmc_request,
+ .set_ios = sunxi_mmc_set_ios,
+ .get_ro = mmc_gpio_get_ro,
+ .get_cd = mmc_gpio_get_cd,
+ .enable_sdio_irq = sunxi_mmc_enable_sdio_irq,
+ .hw_reset = sunxi_mmc_hw_reset,
+};
+
+static int sunxi_mmc_resource_request(struct sunxi_mmc_host *host,
+ struct platform_device *pdev)
+{
+ struct device_node *np = pdev->dev.of_node;
+ int ret;
+
+ if (of_device_is_compatible(np, "allwinner,sun4i-a10-mmc"))
+ host->idma_des_size_bits = 13;
+ else
+ host->idma_des_size_bits = 16;
+
+ host->vmmc = devm_regulator_get_optional(&pdev->dev, "vmmc");
+ if (IS_ERR(host->vmmc) && PTR_ERR(host->vmmc) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+
+ host->reg_base = devm_ioremap_resource(&pdev->dev,
+ platform_get_resource(pdev, IORESOURCE_MEM, 0));
+ if (IS_ERR(host->reg_base))
+ return PTR_ERR(host->reg_base);
+
+ host->clk_ahb = devm_clk_get(&pdev->dev, "ahb");
+ if (IS_ERR(host->clk_ahb)) {
+ dev_err(&pdev->dev, "Could not get ahb clock\n");
+ return PTR_ERR(host->clk_ahb);
+ }
+
+ host->clk_mod = devm_clk_get(&pdev->dev, "mod");
+ if (IS_ERR(host->clk_mod)) {
+ dev_err(&pdev->dev, "Could not get mod clock\n");
+ return PTR_ERR(host->clk_mod);
+ }
+
+ /* Make sure the controller is in a sane state before enabling irqs */
+ ret = sunxi_mmc_init_host(host->mmc);
+ if (ret)
+ return ret;
+
+ host->irq = platform_get_irq(pdev, 0);
+ ret = devm_request_irq(&pdev->dev, host->irq, sunxi_mmc_irq, 0,
+ "sunxi-mmc", host);
+ if (ret == 0)
+ disable_irq(host->irq);
+
+ /* And put it back in reset */
+ sunxi_mmc_exit_host(host);
+
+ return ret;
+}
+
+static int sunxi_mmc_probe(struct platform_device *pdev)
+{
+ struct sunxi_mmc_host *host;
+ struct mmc_host *mmc;
+ int ret;
+
+ mmc = mmc_alloc_host(sizeof(struct sunxi_mmc_host), &pdev->dev);
+ if (!mmc) {
+ dev_err(&pdev->dev, "mmc alloc host failed\n");
+ return -ENOMEM;
+ }
+
+ ret = mmc_of_parse(mmc);
+ if (ret)
+ goto error_free_host;
+
+ host = mmc_priv(mmc);
+ host->mmc = mmc;
+ spin_lock_init(&host->lock);
+ tasklet_init(&host->tasklet, sunxi_mmc_tasklet, (unsigned long)host);
+
+ ret = sunxi_mmc_resource_request(host, pdev);
+ if (ret)
+ goto error_free_host;
+
+ host->sg_cpu = dma_alloc_coherent(&pdev->dev, PAGE_SIZE,
+ &host->sg_dma, GFP_KERNEL);
+ if (!host->sg_cpu) {
+ dev_err(&pdev->dev, "Failed to allocate DMA descriptor mem\n");
+ ret = -ENOMEM;
+ goto error_free_host;
+ }
+
+ mmc->ops = &sunxi_mmc_ops;
+ mmc->max_blk_count = 8192;
+ mmc->max_blk_size = 4096;
+ mmc->max_segs = PAGE_SIZE / sizeof(struct sunxi_idma_des);
+ mmc->max_seg_size = (1 << host->idma_des_size_bits);
+ mmc->max_req_size = mmc->max_seg_size * mmc->max_segs;
+ /* 400kHz ~ 50MHz */
+ mmc->f_min = 400000;
+ mmc->f_max = 50000000;
+ /* available voltages */
+ if (!IS_ERR(host->vmmc))
+ mmc->ocr_avail = mmc_regulator_get_ocrmask(host->vmmc);
+ else
+ mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
+
+ mmc->caps = MMC_CAP_MMC_HIGHSPEED | MMC_CAP_SD_HIGHSPEED |
+ MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 | MMC_CAP_UHS_SDR50 |
+ MMC_CAP_UHS_DDR50 | MMC_CAP_SDIO_IRQ | MMC_CAP_DRIVER_TYPE_A;
+ mmc->caps2 = MMC_CAP2_NO_PRESCAN_POWERUP;
+
+ ret = mmc_add_host(mmc);
+
+ if (ret)
+ goto error_free_dma;
+
+ dev_info(&pdev->dev, "base:0x%p irq:%u\n", host->reg_base, host->irq);
+ platform_set_drvdata(pdev, mmc);
+ return 0;
+
+error_free_dma:
+ dma_free_coherent(&pdev->dev, PAGE_SIZE, host->sg_cpu, host->sg_dma);
+error_free_host:
+ mmc_free_host(mmc);
+ return ret;
+}
+
+static int sunxi_mmc_remove(struct platform_device *pdev)
+{
+ struct mmc_host *mmc = platform_get_drvdata(pdev);
+ struct sunxi_mmc_host *host = mmc_priv(mmc);
+
+ mmc_remove_host(mmc);
+ sunxi_mmc_exit_host(host);
+ tasklet_disable(&host->tasklet);
+ dma_free_coherent(&pdev->dev, PAGE_SIZE, host->sg_cpu, host->sg_dma);
+ mmc_free_host(mmc);
+
+ return 0;
+}
+
+static struct platform_driver sunxi_mmc_driver = {
+ .driver = {
+ .name = "sunxi-mmc",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(sunxi_mmc_of_match),
+ },
+ .probe = sunxi_mmc_probe,
+ .remove = sunxi_mmc_remove,
+};
+module_platform_driver(sunxi_mmc_driver);
+
+MODULE_DESCRIPTION("Allwinner's SD/MMC Card Controller Driver");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("David Lanzend?rfer <[email protected]>");
+MODULE_ALIAS("platform:sunxi-mmc");
diff --git a/drivers/mmc/host/sunxi-mmc.h b/drivers/mmc/host/sunxi-mmc.h
new file mode 100644
index 0000000..75eaa02
--- /dev/null
+++ b/drivers/mmc/host/sunxi-mmc.h
@@ -0,0 +1,239 @@
+/*
+ * Driver for sunxi SD/MMC host controllers
+ * (C) Copyright 2007-2011 Reuuimlla Technology Co., Ltd.
+ * (C) Copyright 2007-2011 Aaron Maoye <[email protected]>
+ * (C) Copyright 2013-2014 O2S GmbH <http://www.o2s.ch>
+ * (C) Copyright 2013-2014 David Lanzend?rfer <[email protected]>
+ * (C) Copyright 2013-2014 Hans de Goede <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ */
+
+#ifndef __SUNXI_MMC_H__
+#define __SUNXI_MMC_H__
+
+/* register offset definitions */
+#define SDXC_REG_GCTRL (0x00) /* SMC Global Control Register */
+#define SDXC_REG_CLKCR (0x04) /* SMC Clock Control Register */
+#define SDXC_REG_TMOUT (0x08) /* SMC Time Out Register */
+#define SDXC_REG_WIDTH (0x0C) /* SMC Bus Width Register */
+#define SDXC_REG_BLKSZ (0x10) /* SMC Block Size Register */
+#define SDXC_REG_BCNTR (0x14) /* SMC Byte Count Register */
+#define SDXC_REG_CMDR (0x18) /* SMC Command Register */
+#define SDXC_REG_CARG (0x1C) /* SMC Argument Register */
+#define SDXC_REG_RESP0 (0x20) /* SMC Response Register 0 */
+#define SDXC_REG_RESP1 (0x24) /* SMC Response Register 1 */
+#define SDXC_REG_RESP2 (0x28) /* SMC Response Register 2 */
+#define SDXC_REG_RESP3 (0x2C) /* SMC Response Register 3 */
+#define SDXC_REG_IMASK (0x30) /* SMC Interrupt Mask Register */
+#define SDXC_REG_MISTA (0x34) /* SMC Masked Interrupt Status Register */
+#define SDXC_REG_RINTR (0x38) /* SMC Raw Interrupt Status Register */
+#define SDXC_REG_STAS (0x3C) /* SMC Status Register */
+#define SDXC_REG_FTRGL (0x40) /* SMC FIFO Threshold Watermark Registe */
+#define SDXC_REG_FUNS (0x44) /* SMC Function Select Register */
+#define SDXC_REG_CBCR (0x48) /* SMC CIU Byte Count Register */
+#define SDXC_REG_BBCR (0x4C) /* SMC BIU Byte Count Register */
+#define SDXC_REG_DBGC (0x50) /* SMC Debug Enable Register */
+#define SDXC_REG_HWRST (0x78) /* SMC Card Hardware Reset for Register */
+#define SDXC_REG_DMAC (0x80) /* SMC IDMAC Control Register */
+#define SDXC_REG_DLBA (0x84) /* SMC IDMAC Descriptor List Base Addre */
+#define SDXC_REG_IDST (0x88) /* SMC IDMAC Status Register */
+#define SDXC_REG_IDIE (0x8C) /* SMC IDMAC Interrupt Enable Register */
+#define SDXC_REG_CHDA (0x90)
+#define SDXC_REG_CBDA (0x94)
+
+#define mci_readl(host, reg) \
+ readl((host)->reg_base + SDXC_##reg)
+#define mci_writel(host, reg, value) \
+ writel((value), (host)->reg_base + SDXC_##reg)
+
+/* global control register bits */
+#define SDXC_SOFT_RESET BIT(0)
+#define SDXC_FIFO_RESET BIT(1)
+#define SDXC_DMA_RESET BIT(2)
+#define SDXC_HARDWARE_RESET (SDXC_SOFT_RESET|SDXC_FIFO_RESET|SDXC_DMA_RESET)
+#define SDXC_INTERRUPT_ENABLE_BIT BIT(4)
+#define SDXC_DMA_ENABLE_BIT BIT(5)
+#define SDXC_DEBOUNCE_ENABLE_BIT BIT(8)
+#define SDXC_POSEDGE_LATCH_DATA BIT(9)
+#define SDXC_DDR_MODE BIT(10)
+#define SDXC_MEMORY_ACCESS_DONE BIT(29)
+#define SDXC_ACCESS_DONE_DIRECT BIT(30)
+#define SDXC_ACCESS_BY_AHB BIT(31)
+#define SDXC_ACCESS_BY_DMA (0U << 31)
+/* clock control bits */
+#define SDXC_CARD_CLOCK_ON BIT(16)
+#define SDXC_LOW_POWER_ON BIT(17)
+/* bus width */
+#define SDXC_WIDTH1 (0)
+#define SDXC_WIDTH4 (1)
+#define SDXC_WIDTH8 (2)
+/* smc command bits */
+#define SDXC_RESP_EXPIRE BIT(6)
+#define SDXC_LONG_RESPONSE BIT(7)
+#define SDXC_CHECK_RESPONSE_CRC BIT(8)
+#define SDXC_DATA_EXPIRE BIT(9)
+#define SDXC_WRITE BIT(10)
+#define SDXC_SEQUENCE_MODE BIT(11)
+#define SDXC_SEND_AUTO_STOP BIT(12)
+#define SDXC_WAIT_PRE_OVER BIT(13)
+#define SDXC_STOP_ABORT_CMD BIT(14)
+#define SDXC_SEND_INIT_SEQUENCE BIT(15)
+#define SDXC_UPCLK_ONLY BIT(21)
+#define SDXC_READ_CEATA_DEV BIT(22)
+#define SDXC_CCS_EXPIRE BIT(23)
+#define SDXC_ENABLE_BIT_BOOT BIT(24)
+#define SDXC_ALT_BOOT_OPTIONS BIT(25)
+#define SDXC_BOOT_ACK_EXPIRE BIT(26)
+#define SDXC_BOOT_ABORT BIT(27)
+#define SDXC_VOLTAGE_SWITCH BIT(28)
+#define SDXC_USE_HOLD_REGISTER BIT(29)
+#define SDXC_START BIT(31)
+/* interrupt bits */
+#define SDXC_RESP_ERROR BIT(1)
+#define SDXC_COMMAND_DONE BIT(2)
+#define SDXC_DATA_OVER BIT(3)
+#define SDXC_TX_DATA_REQUEST BIT(4)
+#define SDXC_RX_DATA_REQUEST BIT(5)
+#define SDXC_RESP_CRC_ERROR BIT(6)
+#define SDXC_DATA_CRC_ERROR BIT(7)
+#define SDXC_RESP_TIMEOUT BIT(8)
+#define SDXC_DATA_TIMEOUT BIT(9)
+#define SDXC_VOLTAGE_CHANGE_DONE BIT(10)
+#define SDXC_FIFO_RUN_ERROR BIT(11)
+#define SDXC_HARD_WARE_LOCKED BIT(12)
+#define SDXC_START_BIT_ERROR BIT(13)
+#define SDXC_AUTO_COMMAND_DONE BIT(14)
+#define SDXC_END_BIT_ERROR BIT(15)
+#define SDXC_SDIO_INTERRUPT BIT(16)
+#define SDXC_CARD_INSERT BIT(30)
+#define SDXC_CARD_REMOVE BIT(31)
+#define SDXC_INTERRUPT_ERROR_BIT (SDXC_RESP_ERROR | SDXC_RESP_CRC_ERROR | \
+ SDXC_DATA_CRC_ERROR | SDXC_RESP_TIMEOUT | \
+ SDXC_DATA_TIMEOUT | SDXC_FIFO_RUN_ERROR | \
+ SDXC_HARD_WARE_LOCKED | SDXC_START_BIT_ERROR | \
+ SDXC_END_BIT_ERROR) /* 0xbbc2 */
+#define SDXC_INTERRUPT_DONE_BIT (SDXC_AUTO_COMMAND_DONE | SDXC_DATA_OVER | \
+ SDXC_COMMAND_DONE | SDXC_VOLTAGE_CHANGE_DONE)
+/* status */
+#define SDXC_RXWL_FLAG BIT(0)
+#define SDXC_TXWL_FLAG BIT(1)
+#define SDXC_FIFO_EMPTY BIT(2)
+#define SDXC_FIFO_FULL BIT(3)
+#define SDXC_CARD_PRESENT BIT(8)
+#define SDXC_CARD_DATA_BUSY BIT(9)
+#define SDXC_DATA_FSM_BUSY BIT(10)
+#define SDXC_DMA_REQUEST BIT(31)
+#define SDXC_FIFO_SIZE (16)
+/* Function select */
+#define SDXC_CEATA_ON (0xceaaU << 16)
+#define SDXC_SEND_IRQ_RESPONSE BIT(0)
+#define SDXC_SDIO_READ_WAIT BIT(1)
+#define SDXC_ABORT_READ_DATA BIT(2)
+#define SDXC_SEND_CCSD BIT(8)
+#define SDXC_SEND_AUTO_STOPCCSD BIT(9)
+#define SDXC_CEATA_DEV_INTERRUPT_ENABLE_BIT BIT(10)
+/* IDMA controller bus mod bit field */
+#define SDXC_IDMAC_SOFT_RESET BIT(0)
+#define SDXC_IDMAC_FIX_BURST BIT(1)
+#define SDXC_IDMAC_IDMA_ON BIT(7)
+#define SDXC_IDMAC_REFETCH_DES BIT(31)
+/* IDMA status bit field */
+#define SDXC_IDMAC_TRANSMIT_INTERRUPT BIT(0)
+#define SDXC_IDMAC_RECEIVE_INTERRUPT BIT(1)
+#define SDXC_IDMAC_FATAL_BUS_ERROR BIT(2)
+#define SDXC_IDMAC_DESTINATION_INVALID BIT(4)
+#define SDXC_IDMAC_CARD_ERROR_SUM BIT(5)
+#define SDXC_IDMAC_NORMAL_INTERRUPT_SUM BIT(8)
+#define SDXC_IDMAC_ABNORMAL_INTERRUPT_SUM BIT(9)
+#define SDXC_IDMAC_HOST_ABORT_INTERRUPT_TX BIT(10)
+#define SDXC_IDMAC_HOST_ABORT_INTERRUPT_RX BIT(10)
+#define SDXC_IDMAC_IDLE (0U << 13)
+#define SDXC_IDMAC_SUSPEND (1U << 13)
+#define SDXC_IDMAC_DESC_READ (2U << 13)
+#define SDXC_IDMAC_DESC_CHECK (3U << 13)
+#define SDXC_IDMAC_READ_REQUEST_WAIT (4U << 13)
+#define SDXC_IDMAC_WRITE_REQUEST_WAIT (5U << 13)
+#define SDXC_IDMAC_READ (6U << 13)
+#define SDXC_IDMAC_WRITE (7U << 13)
+#define SDXC_IDMAC_DESC_CLOSE (8U << 13)
+
+/*
+* If the idma-des-size-bits of property is ie 13, bufsize bits are:
+* Bits 0-12: buf1 size
+* Bits 13-25: buf2 size
+* Bits 26-31: not used
+* Since we only ever set buf1 size, we can simply store it directly.
+*/
+#define SDXC_IDMAC_DES0_DIC BIT(1) /* disable interrupt on completion */
+#define SDXC_IDMAC_DES0_LD BIT(2) /* last descriptor */
+#define SDXC_IDMAC_DES0_FD BIT(3) /* first descriptor */
+#define SDXC_IDMAC_DES0_CH BIT(4) /* chain mode */
+#define SDXC_IDMAC_DES0_ER BIT(5) /* end of ring */
+#define SDXC_IDMAC_DES0_CES BIT(30) /* card error summary */
+#define SDXC_IDMAC_DES0_OWN BIT(31) /* 1-idma owns it, 0-host owns it */
+
+struct sunxi_idma_des {
+ u32 config;
+ u32 buf_size;
+ u32 buf_addr_ptr1;
+ u32 buf_addr_ptr2;
+};
+
+struct sunxi_mmc_host {
+ struct mmc_host *mmc;
+ struct regulator *vmmc;
+
+ /* IO mapping base */
+ void __iomem *reg_base;
+
+ spinlock_t lock;
+ struct tasklet_struct tasklet;
+
+ /* clock management */
+ struct clk *clk_ahb;
+ struct clk *clk_mod;
+
+ /* ios information */
+ u32 clk_mod_rate;
+ u32 bus_width;
+ u32 idma_des_size_bits;
+ u32 ddr;
+ u32 voltage_switching;
+
+ /* irq */
+ int irq;
+ u32 int_sum;
+ u32 sdio_imask;
+
+ /* flags */
+ u32 power_on:1;
+ u32 io_flag:1;
+ u32 wait_dma:1;
+
+ dma_addr_t sg_dma;
+ void *sg_cpu;
+
+ struct mmc_request *mrq;
+ u32 ferror;
+};
+
+#define MMC_CLK_400K 0
+#define MMC_CLK_25M 1
+#define MMC_CLK_50M 2
+#define MMC_CLK_50MDDR 3
+#define MMC_CLK_50MDDR_8BIT 4
+#define MMC_CLK_100M 5
+#define MMC_CLK_200M 6
+#define MMC_CLK_MOD_NUM 7
+
+struct sunxi_mmc_clk_dly {
+ u32 mode;
+ u32 oclk_dly;
+ u32 sclk_dly;
+};
+
+#endif

2014-02-17 10:06:57

by David Lanzendörfer

[permalink] [raw]
Subject: [PATCH v7 1/8] clk: sunxi: factors: automatic reparenting support

From: Emilio López <[email protected]>

This commit implements .determine_rate, so that our factor clocks can be
reparented when needed.

Signed-off-by: Emilio López <[email protected]>
---
drivers/clk/sunxi/clk-factors.c | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)

diff --git a/drivers/clk/sunxi/clk-factors.c b/drivers/clk/sunxi/clk-factors.c
index 9e23264..3806d97 100644
--- a/drivers/clk/sunxi/clk-factors.c
+++ b/drivers/clk/sunxi/clk-factors.c
@@ -77,6 +77,41 @@ static long clk_factors_round_rate(struct clk_hw *hw, unsigned long rate,
return rate;
}

+static long clk_factors_determine_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long *best_parent_rate,
+ struct clk **best_parent_p)
+{
+ struct clk *clk = hw->clk, *parent, *best_parent = NULL;
+ int i, num_parents;
+ unsigned long parent_rate, best = 0, child_rate, best_child_rate = 0;
+
+ /* find the parent that can help provide the fastest rate <= rate */
+ num_parents = __clk_get_num_parents(clk);
+ for (i = 0; i < num_parents; i++) {
+ parent = clk_get_parent_by_index(clk, i);
+ if (!parent)
+ continue;
+ if (__clk_get_flags(clk) & CLK_SET_RATE_PARENT)
+ parent_rate = __clk_round_rate(parent, rate);
+ else
+ parent_rate = __clk_get_rate(parent);
+
+ child_rate = clk_factors_round_rate(hw, rate, &parent_rate);
+
+ if (child_rate <= rate && child_rate > best_child_rate) {
+ best_parent = parent;
+ best = parent_rate;
+ best_child_rate = child_rate;
+ }
+ }
+
+ if (best_parent)
+ *best_parent_p = best_parent;
+ *best_parent_rate = best;
+
+ return best_child_rate;
+}
+
static int clk_factors_set_rate(struct clk_hw *hw, unsigned long rate,
unsigned long parent_rate)
{
@@ -113,6 +148,7 @@ static int clk_factors_set_rate(struct clk_hw *hw, unsigned long rate,
}

const struct clk_ops clk_factors_ops = {
+ .determine_rate = clk_factors_determine_rate,
.recalc_rate = clk_factors_recalc_rate,
.round_rate = clk_factors_round_rate,
.set_rate = clk_factors_set_rate,

2014-02-18 14:10:06

by [email protected]

[permalink] [raw]
Subject: Re: [PATCH v7 1/8] clk: sunxi: factors: automatic reparenting support

On Mon, Feb 17, 2014 at 11:02:15AM +0100, David Lanzend?rfer wrote:
> From: Emilio L?pez <[email protected]>
>
> This commit implements .determine_rate, so that our factor clocks can be
> reparented when needed.
>
> Signed-off-by: Emilio L?pez <[email protected]>

Your signed-off-by is missing here.

Once you added it, you can add my Acked-by

Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


Attachments:
(No filename) (469.00 B)
signature.asc (836.00 B)
Digital signature
Download all attachments

2014-02-18 14:20:09

by [email protected]

[permalink] [raw]
Subject: Re: [PATCH v7 3/8] ARM: sunxi: clk: export clk_sunxi_mmc_phase_control

On Mon, Feb 17, 2014 at 11:02:28AM +0100, David Lanzend?rfer wrote:
> From: Hans de Goede <[email protected]>
>
> Signed-off-by: Hans de Goede <[email protected]>

Again, your SoB is missing, and that can be squashed with the previous
patch.

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


Attachments:
(No filename) (355.00 B)
signature.asc (836.00 B)
Digital signature
Download all attachments

2014-02-18 14:20:07

by [email protected]

[permalink] [raw]
Subject: Re: [PATCH v7 2/8] clk: sunxi: Implement MMC phase control

Hi,

On Mon, Feb 17, 2014 at 11:02:21AM +0100, David Lanzend?rfer wrote:
> From: Emilio L?pez <[email protected]>
>
> Signed-off-by: Emilio L?pez <[email protected]z.com.ar>

You're missing your Signed-off-by here too. Remember, for every patch
you send, your Signed-off-by must be there, regardless wether you're
the author or not.

A commit log would be very much welcome too.

Now, down to the patch itself, I remember Mike saying that he would
prefer adding a function in the framework instead of hardcoding
it. Mike, what's your feeling on this? Would merging this seem
reasonnable to you as is, or do you want to take this to the
framework?

> ---
> drivers/clk/sunxi/clk-sunxi.c | 35 +++++++++++++++++++++++++++++++++++
> 1 file changed, 35 insertions(+)
>
> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
> index abb6c5a..33b9977 100644
> --- a/drivers/clk/sunxi/clk-sunxi.c
> +++ b/drivers/clk/sunxi/clk-sunxi.c
> @@ -377,6 +377,41 @@ static void sun7i_a20_get_out_factors(u32 *freq, u32 parent_rate,
>
>
> /**
> + * clk_sunxi_mmc_phase_control() - configures MMC clock phase control
> + */

If you don't go the framework road, some documentation on what are the
arguments it takes and what it's supposed to return would be great.

Thanks!
Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


Attachments:
(No filename) (1.37 kB)
signature.asc (836.00 B)
Digital signature
Download all attachments

2014-02-18 14:25:10

by [email protected]

[permalink] [raw]
Subject: Re: [PATCH v7 5/8] ARM: dts: sun7i: Add support for mmc

On Mon, Feb 17, 2014 at 11:02:41AM +0100, David Lanzend?rfer wrote:
> Signed-off-by: David Lanzend?rfer <[email protected]>
> Signed-off-by: Hans de Goede <[email protected]>
> ---
> arch/arm/boot/dts/sun7i-a20-cubieboard2.dts | 8 +++
> arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 8 +++
> arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts | 23 +++++++++
> arch/arm/boot/dts/sun7i-a20.dtsi | 61 +++++++++++++++++++++++
> 4 files changed, 100 insertions(+)
>

I'd prefer to have three patches here:
- One that add the controllers
- One that add the pin muxing options
- One that enable the controllers on the various boards.

> diff --git a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
> index 5c51cb8..ae800b6 100644
> --- a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
> +++ b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
> @@ -34,6 +34,14 @@
> };
> };
>
> + mmc0: [email protected] {
> + pinctrl-names = "default", "default";
> + pinctrl-0 = <&mmc0_pins_a>;
> + pinctrl-1 = <&mmc0_cd_pin_reference_design>;

This can be made a single pinctrl group, you don't need the pinctrl-1
stuff, it only complicates the node.

> + cd-gpios = <&pio 7 1 0>; /* PH1 */
> + status = "okay";
> + };
> +
> [email protected] {
> led_pins_cubieboard2: [email protected] {
> allwinner,pins = "PH20", "PH21";
> diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
> index f9dcb61..370cef84 100644
> --- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
> +++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
> @@ -19,6 +19,14 @@
> compatible = "cubietech,cubietruck", "allwinner,sun7i-a20";
>
> [email protected] {
> + mmc0: [email protected] {
> + pinctrl-names = "default", "default";
> + pinctrl-0 = <&mmc0_pins_a>;
> + pinctrl-1 = <&mmc0_cd_pin_reference_design>;
> + cd-gpios = <&pio 7 1 0>; /* PH1 */
> + status = "okay";
> + };
> +
> [email protected] {
> led_pins_cubietruck: [email protected] {
> allwinner,pins = "PH7", "PH11", "PH20", "PH21";
> diff --git a/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts b/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
> index ead3013..685ec06 100644
> --- a/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
> +++ b/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
> @@ -34,7 +34,30 @@
> };
> };
>
> + mmc0: [email protected] {
> + pinctrl-names = "default", "default";
> + pinctrl-0 = <&mmc0_pins_a>;
> + pinctrl-1 = <&mmc0_cd_pin_reference_design>;
> + cd-gpios = <&pio 7 1 0>; /* PH1 */
> + status = "okay";
> + };
> +
> + mmc3: [email protected] {
> + pinctrl-names = "default", "default";
> + pinctrl-0 = <&mmc3_pins_a>;
> + pinctrl-1 = <&mmc3_cd_pin_olinuxinom>;
> + cd-gpios = <&pio 7 11 0>; /* PH11 */
> + status = "okay";
> + };
> +
> [email protected] {
> + mmc3_cd_pin_olinuxinom: [email protected] {
> + allwinner,pins = "PH11";
> + allwinner,function = "gpio_in";
> + allwinner,drive = <0>;
> + allwinner,pull = <1>;
> + };
> +
> led_pins_olinuxino: [email protected] {
> allwinner,pins = "PH2";
> allwinner,function = "gpio_out";
> diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
> index 9ff0948..5b55414 100644
> --- a/arch/arm/boot/dts/sun7i-a20.dtsi
> +++ b/arch/arm/boot/dts/sun7i-a20.dtsi
> @@ -355,6 +355,46 @@
> #size-cells = <0>;
> };
>
> + mmc0: [email protected] {
> + 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>;

This belongs to the board, the controller itself is able to handle
several bus width.

> + status = "disabled";
> + };
> +
> + mmc1: [email protected] {
> + compatible = "allwinner,sun5i-a13-mmc";
> + reg = <0x01c10000 0x1000>;
> + clocks = <&ahb_gates 9>, <&mmc1_clk>;
> + clock-names = "ahb", "mod";
> + interrupts = <0 33 4>;
> + bus-width = <4>;
> + status = "disabled";
> + };
> +
> + mmc2: [email protected] {
> + compatible = "allwinner,sun5i-a13-mmc";
> + reg = <0x01c11000 0x1000>;
> + clocks = <&ahb_gates 10>, <&mmc2_clk>;
> + clock-names = "ahb", "mod";
> + interrupts = <0 34 4>;
> + bus-width = <4>;
> + status = "disabled";
> + };
> +
> + mmc3: [email protected] {
> + compatible = "allwinner,sun5i-a13-mmc";
> + reg = <0x01c12000 0x1000>;
> + clocks = <&ahb_gates 11>, <&mmc3_clk>;
> + clock-names = "ahb", "mod";
> + interrupts = <0 35 4>;
> + bus-width = <4>;
> + status = "disabled";
> + };
> +
> pio: [email protected] {
> compatible = "allwinner,sun7i-a20-pinctrl";
> reg = <0x01c20800 0x400>;
> @@ -432,6 +472,27 @@
> allwinner,drive = <0>;
> allwinner,pull = <0>;
> };
> +
> + mmc0_pins_a: [email protected] {
> + allwinner,pins = "PF0","PF1","PF2","PF3","PF4","PF5";
> + allwinner,function = "mmc0";
> + allwinner,drive = <3>;
> + allwinner,pull = <0>;
> + };
> +
> + mmc0_cd_pin_reference_design: [email protected] {
> + allwinner,pins = "PH1";
> + allwinner,function = "gpio_in";
> + allwinner,drive = <0>;
> + allwinner,pull = <1>;
> + };
> +
> + mmc3_pins_a: [email protected] {
> + allwinner,pins = "PI4","PI5","PI6","PI7","PI8","PI9";
> + allwinner,function = "mmc3";
> + allwinner,drive = <3>;
> + allwinner,pull = <0>;
> + };
> };
>
> [email protected] {
>

Looks good otherwise.

Thanks!
Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


Attachments:
(No filename) (5.47 kB)
signature.asc (836.00 B)
Digital signature
Download all attachments

2014-02-18 15:12:02

by Hans de Goede

[permalink] [raw]
Subject: Re: [PATCH v7 5/8] ARM: dts: sun7i: Add support for mmc

Hi,

On 02/18/2014 03:22 PM, Maxime Ripard wrote:
> On Mon, Feb 17, 2014 at 11:02:41AM +0100, David Lanzend?rfer wrote:
>> Signed-off-by: David Lanzend?rfer <[email protected]>
>> Signed-off-by: Hans de Goede <[email protected]>
>> ---
>> arch/arm/boot/dts/sun7i-a20-cubieboard2.dts | 8 +++
>> arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 8 +++
>> arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts | 23 +++++++++
>> arch/arm/boot/dts/sun7i-a20.dtsi | 61 +++++++++++++++++++++++
>> 4 files changed, 100 insertions(+)
>>
>
> I'd prefer to have three patches here:
> - One that add the controllers
> - One that add the pin muxing options
> - One that enable the controllers on the various boards.
>
>> diff --git a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
>> index 5c51cb8..ae800b6 100644
>> --- a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
>> +++ b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
>> @@ -34,6 +34,14 @@
>> };
>> };
>>
>> + mmc0: [email protected] {
>> + pinctrl-names = "default", "default";
>> + pinctrl-0 = <&mmc0_pins_a>;
>> + pinctrl-1 = <&mmc0_cd_pin_reference_design>;
>
> This can be made a single pinctrl group, you don't need the pinctrl-1
> stuff, it only complicates the node.

Then how do we deal with boards which use a different gpio for card-detect ?

In that case we don't want to change the mux setting of the reference
design cd pin. IOW I believe that having 2 separate pinctrl settings for
this is the rigt thing todo. I would prefer using just mmc0_cd_pin_a instead
of _reference_design though.

Oh wait, you're probably talking about using:
pinctrl-0 = <&mmc0_pins_a>, <&mmc0_cd_pin_reference_design>;

Yes that would be better.

>
>> + cd-gpios = <&pio 7 1 0>; /* PH1 */
>> + status = "okay";
>> + };
>> +
>> [email protected] {
>> led_pins_cubieboard2: [email protected] {
>> allwinner,pins = "PH20", "PH21";
>> diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
>> index f9dcb61..370cef84 100644
>> --- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
>> +++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
>> @@ -19,6 +19,14 @@
>> compatible = "cubietech,cubietruck", "allwinner,sun7i-a20";
>>
>> [email protected] {
>> + mmc0: [email protected] {
>> + pinctrl-names = "default", "default";
>> + pinctrl-0 = <&mmc0_pins_a>;
>> + pinctrl-1 = <&mmc0_cd_pin_reference_design>;
>> + cd-gpios = <&pio 7 1 0>; /* PH1 */
>> + status = "okay";
>> + };
>> +
>> [email protected] {
>> led_pins_cubietruck: [email protected] {
>> allwinner,pins = "PH7", "PH11", "PH20", "PH21";
>> diff --git a/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts b/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
>> index ead3013..685ec06 100644
>> --- a/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
>> +++ b/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
>> @@ -34,7 +34,30 @@
>> };
>> };
>>
>> + mmc0: [email protected] {
>> + pinctrl-names = "default", "default";
>> + pinctrl-0 = <&mmc0_pins_a>;
>> + pinctrl-1 = <&mmc0_cd_pin_reference_design>;
>> + cd-gpios = <&pio 7 1 0>; /* PH1 */
>> + status = "okay";
>> + };
>> +
>> + mmc3: [email protected] {
>> + pinctrl-names = "default", "default";
>> + pinctrl-0 = <&mmc3_pins_a>;
>> + pinctrl-1 = <&mmc3_cd_pin_olinuxinom>;
>> + cd-gpios = <&pio 7 11 0>; /* PH11 */
>> + status = "okay";
>> + };
>> +
>> [email protected] {
>> + mmc3_cd_pin_olinuxinom: [email protected] {
>> + allwinner,pins = "PH11";
>> + allwinner,function = "gpio_in";
>> + allwinner,drive = <0>;
>> + allwinner,pull = <1>;
>> + };
>> +
>> led_pins_olinuxino: [email protected] {
>> allwinner,pins = "PH2";
>> allwinner,function = "gpio_out";
>> diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
>> index 9ff0948..5b55414 100644
>> --- a/arch/arm/boot/dts/sun7i-a20.dtsi
>> +++ b/arch/arm/boot/dts/sun7i-a20.dtsi
>> @@ -355,6 +355,46 @@
>> #size-cells = <0>;
>> };
>>
>> + mmc0: [email protected] {
>> + 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>;
>
> This belongs to the board, the controller itself is able to handle
> several bus width.

I believe that providing some form of default in the dtsi makes sense
here and all boards we've seen sofar always use 4 bits, we can always
override this from the dts file itself.

>
>> + status = "disabled";
>> + };
>> +
>> + mmc1: [email protected] {
>> + compatible = "allwinner,sun5i-a13-mmc";
>> + reg = <0x01c10000 0x1000>;
>> + clocks = <&ahb_gates 9>, <&mmc1_clk>;
>> + clock-names = "ahb", "mod";
>> + interrupts = <0 33 4>;
>> + bus-width = <4>;
>> + status = "disabled";
>> + };
>> +
>> + mmc2: [email protected] {
>> + compatible = "allwinner,sun5i-a13-mmc";
>> + reg = <0x01c11000 0x1000>;
>> + clocks = <&ahb_gates 10>, <&mmc2_clk>;
>> + clock-names = "ahb", "mod";
>> + interrupts = <0 34 4>;
>> + bus-width = <4>;
>> + status = "disabled";
>> + };
>> +
>> + mmc3: [email protected] {
>> + compatible = "allwinner,sun5i-a13-mmc";
>> + reg = <0x01c12000 0x1000>;
>> + clocks = <&ahb_gates 11>, <&mmc3_clk>;
>> + clock-names = "ahb", "mod";
>> + interrupts = <0 35 4>;
>> + bus-width = <4>;
>> + status = "disabled";
>> + };
>> +
>> pio: [email protected] {
>> compatible = "allwinner,sun7i-a20-pinctrl";
>> reg = <0x01c20800 0x400>;
>> @@ -432,6 +472,27 @@
>> allwinner,drive = <0>;
>> allwinner,pull = <0>;
>> };
>> +
>> + mmc0_pins_a: [email protected] {
>> + allwinner,pins = "PF0","PF1","PF2","PF3","PF4","PF5";
>> + allwinner,function = "mmc0";
>> + allwinner,drive = <3>;
>> + allwinner,pull = <0>;
>> + };
>> +
>> + mmc0_cd_pin_reference_design: [email protected] {
>> + allwinner,pins = "PH1";
>> + allwinner,function = "gpio_in";
>> + allwinner,drive = <0>;
>> + allwinner,pull = <1>;
>> + };
>> +
>> + mmc3_pins_a: [email protected] {
>> + allwinner,pins = "PI4","PI5","PI6","PI7","PI8","PI9";
>> + allwinner,function = "mmc3";
>> + allwinner,drive = <3>;
>> + allwinner,pull = <0>;
>> + };
>> };
>>
>> [email protected] {
>>
>
> Looks good otherwise.
>
> Thanks!
> Maxime
>


Regards,

Hans

2014-02-18 15:40:12

by [email protected]

[permalink] [raw]
Subject: Re: [PATCH v7 4/8] ARM: sunxi: Add driver for SD/MMC hosts found on Allwinner sunxi SoCs

Hi,

Sorry for taking a bit of time to review this.

On Mon, Feb 17, 2014 at 11:02:34AM +0100, David Lanzend?rfer wrote:
> This is based on the driver Allwinner ships in their Android kernel sources.
>
> Initial porting to upstream kernels done by David Lanzend?rfer, additional
> fixes and cleanups by Hans de Goede.

This belongs to the driver copyright or MODULE_AUTHOR, not really in
the commit log.

What you should describe here is what the driver does, what the device
it drives is capable of, would it be possible to share some common
code with other drivers, why didn't you do it, etc.

>
> It uses dma in bus-master mode using a built-in designware idmac controller,
> which is identical to the one found in the mmc-dw hosts.
> The rest of the host is not identical to mmc-dw.
>
> Signed-off-by: David Lanzend?rfer <[email protected]>
> Signed-off-by: Hans de Goede <[email protected]>
> ---
> drivers/mmc/host/Kconfig | 7
> drivers/mmc/host/Makefile | 2
> drivers/mmc/host/sunxi-mmc.c | 876 ++++++++++++++++++++++++++++++++++++++++++
> drivers/mmc/host/sunxi-mmc.h | 239 +++++++++++
> 4 files changed, 1124 insertions(+)
> create mode 100644 drivers/mmc/host/sunxi-mmc.c
> create mode 100644 drivers/mmc/host/sunxi-mmc.h
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 1384f67..7caf266 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -689,3 +689,10 @@ config MMC_REALTEK_PCI
> help
> Say Y here to include driver code to support SD/MMC card interface
> of Realtek PCI-E card reader
> +
> +config MMC_SUNXI
> + tristate "Allwinner sunxi SD/MMC Host Controller support"
> + depends on ARCH_SUNXI
> + help
> + This selects support for the SD/MMC Host Controller on
> + Allwinner sunxi SoCs.
> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> index 3483b6b..f3c7c243 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -54,6 +54,8 @@ obj-$(CONFIG_MMC_WMT) += wmt-sdmmc.o
>
> obj-$(CONFIG_MMC_REALTEK_PCI) += rtsx_pci_sdmmc.o
>
> +obj-$(CONFIG_MMC_SUNXI) += sunxi-mmc.o
> +
> obj-$(CONFIG_MMC_SDHCI_PLTFM) += sdhci-pltfm.o
> obj-$(CONFIG_MMC_SDHCI_CNS3XXX) += sdhci-cns3xxx.o
> obj-$(CONFIG_MMC_SDHCI_ESDHC_IMX) += sdhci-esdhc-imx.o
> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> new file mode 100644
> index 0000000..752e913
> --- /dev/null
> +++ b/drivers/mmc/host/sunxi-mmc.c
> @@ -0,0 +1,876 @@
> +/*
> + * Driver for sunxi SD/MMC host controllers
> + * (C) Copyright 2007-2011 Reuuimlla Technology Co., Ltd.
> + * (C) Copyright 2007-2011 Aaron Maoye <[email protected]>
> + * (C) Copyright 2013-2014 O2S GmbH <http://www.o2s.ch>
> + * (C) Copyright 2013-2014 David Lanzend?rfer <[email protected]>
> + * (C) Copyright 2013-2014 Hans de Goede <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/device.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +
> +#include <linux/clk.h>
> +#include <linux/clk-private.h>
> +#include <linux/clk/sunxi.h>
> +
> +#include <linux/gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +#include <linux/scatterlist.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/slab.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <linux/of_address.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of_platform.h>
> +
> +#include <linux/mmc/host.h>
> +#include <linux/mmc/sd.h>
> +#include <linux/mmc/sdio.h>
> +#include <linux/mmc/mmc.h>
> +#include <linux/mmc/core.h>
> +#include <linux/mmc/card.h>
> +#include <linux/mmc/slot-gpio.h>
> +
> +#include "sunxi-mmc.h"

Usually, drivers don't have a header of their own.

> +static int sunxi_mmc_init_host(struct mmc_host *mmc)
> +{
> + u32 rval;
> + struct sunxi_mmc_host *smc_host = mmc_priv(mmc);
> + int ret;
> +
> + ret = clk_prepare_enable(smc_host->clk_ahb);
> + if (ret) {
> + dev_err(mmc_dev(smc_host->mmc), "AHB clk err %d\n", ret);
> + return ret;
> + }

Newline.

> + ret = clk_prepare_enable(smc_host->clk_mod);
> + if (ret) {
> + dev_err(mmc_dev(smc_host->mmc), "MOD clk err %d\n", ret);
> + clk_disable_unprepare(smc_host->clk_ahb);
> + return ret;
> + }
> +
> + /* reset controller */
> + rval = mci_readl(smc_host, REG_GCTRL) | SDXC_HARDWARE_RESET;

You seem to be sometimes using either a oneline construct like this
one, or on several lines. It would be great if you were to be using
the same everywhere.

> + mci_writel(smc_host, REG_GCTRL, rval);
> +
> + mci_writel(smc_host, REG_FTRGL, 0x20070008);
> + mci_writel(smc_host, REG_TMOUT, 0xffffffff);
> + mci_writel(smc_host, REG_IMASK, smc_host->sdio_imask);
> + mci_writel(smc_host, REG_RINTR, 0xffffffff);
> + mci_writel(smc_host, REG_DBGC, 0xdeb);
> + mci_writel(smc_host, REG_FUNS, 0xceaa0000);
> + mci_writel(smc_host, REG_DLBA, smc_host->sg_dma);

Newline.

> + rval = mci_readl(smc_host, REG_GCTRL)|SDXC_INTERRUPT_ENABLE_BIT;
> + rval &= ~SDXC_ACCESS_DONE_DIRECT;
> + mci_writel(smc_host, REG_GCTRL, rval);
> +
> + return 0;
> +}
> +
> +static void sunxi_mmc_exit_host(struct sunxi_mmc_host *smc_host)
> +{
> + mci_writel(smc_host, REG_GCTRL, SDXC_HARDWARE_RESET);
> + clk_disable_unprepare(smc_host->clk_ahb);
> + clk_disable_unprepare(smc_host->clk_mod);
> +}
> +
> +/* /\* UHS-I Operation Modes */
> +/* * DS 25MHz 12.5MB/s 3.3V */
> +/* * HS 50MHz 25MB/s 3.3V */
> +/* * SDR12 25MHz 12.5MB/s 1.8V */
> +/* * SDR25 50MHz 25MB/s 1.8V */
> +/* * SDR50 100MHz 50MB/s 1.8V */
> +/* * SDR104 208MHz 104MB/s 1.8V */
> +/* * DDR50 50MHz 50MB/s 1.8V */
> +/* * MMC Operation Modes */
> +/* * DS 26MHz 26MB/s 3/1.8/1.2V */
> +/* * HS 52MHz 52MB/s 3/1.8/1.2V */
> +/* * HSDDR 52MHz 104MB/s 3/1.8/1.2V */
> +/* * HS200 200MHz 200MB/s 1.8/1.2V */
> +/* * */
> +/* * Spec. Timing */
> +/* * SD3.0 */
> +/* * Fcclk Tcclk Fsclk Tsclk Tis Tih odly RTis RTih */
> +/* * 400K 2.5us 24M 41ns 5ns 5ns 1 2209ns 41ns */
> +/* * 25M 40ns 600M 1.67ns 5ns 5ns 3 14.99ns 5.01ns */
> +/* * 50M 20ns 600M 1.67ns 6ns 2ns 3 14.99ns 5.01ns */
> +/* * 50MDDR 20ns 600M 1.67ns 6ns 0.8ns 2 6.67ns 3.33ns */
> +/* * 104M 9.6ns 600M 1.67ns 3ns 0.8ns 1 7.93ns 1.67ns */
> +/* * 208M 4.8ns 600M 1.67ns 1.4ns 0.8ns 1 3.33ns 1.67ns */
> +
> +/* * 25M 40ns 300M 3.33ns 5ns 5ns 2 13.34ns 6.66ns */
> +/* * 50M 20ns 300M 3.33ns 6ns 2ns 2 13.34ns 6.66ns */
> +/* * 50MDDR 20ns 300M 3.33ns 6ns 0.8ns 1 6.67ns 3.33ns */
> +/* * 104M 9.6ns 300M 3.33ns 3ns 0.8ns 0 7.93ns 1.67ns */
> +/* * 208M 4.8ns 300M 3.33ns 1.4ns 0.8ns 0 3.13ns 1.67ns */
> +
> +/* * eMMC4.5 */
> +/* * 400K 2.5us 24M 41ns 3ns 3ns 1 2209ns 41ns */
> +/* * 25M 40ns 600M 1.67ns 3ns 3ns 3 14.99ns 5.01ns */
> +/* * 50M 20ns 600M 1.67ns 3ns 3ns 3 14.99ns 5.01ns */
> +/* * 50MDDR 20ns 600M 1.67ns 2.5ns 2.5ns 2 6.67ns 3.33ns */
> +/* * 200M 5ns 600M 1.67ns 1.4ns 0.8ns 1 3.33ns 1.67ns */
> +/* *\/ */
> +
> +static void sunxi_mmc_init_idma_des(struct sunxi_mmc_host *host,
> + struct mmc_data *data)
> +{
> + struct sunxi_idma_des *pdes = (struct sunxi_idma_des *)host->sg_cpu;
> + struct sunxi_idma_des *pdes_pa = (struct sunxi_idma_des *)host->sg_dma;
> + int i, max_len = (1 << host->idma_des_size_bits);

Please use the BIT() macro here.

> +
> + for (i = 0; i < data->sg_len; i++) {
> + pdes[i].config = SDXC_IDMAC_DES0_CH | SDXC_IDMAC_DES0_OWN |
> + SDXC_IDMAC_DES0_DIC;
> +
> + if (data->sg[i].length == max_len)
> + pdes[i].buf_size = 0; /* 0 == max_len */
> + else
> + pdes[i].buf_size = data->sg[i].length;
> +
> + pdes[i].buf_addr_ptr1 = sg_dma_address(&data->sg[i]);
> + pdes[i].buf_addr_ptr2 = (u32)&pdes_pa[i + 1];
> + }
> +
> + pdes[0].config |= SDXC_IDMAC_DES0_FD;
> + pdes[i - 1].config = SDXC_IDMAC_DES0_OWN | SDXC_IDMAC_DES0_LD;
> +
> + wmb(); /* Ensure idma_des hit main mem before we start the idmac */

wmb ensure the proper ordering of the instructions, not flushing the
caches like what your comment implies.

> +}
> +
> +static enum dma_data_direction sunxi_mmc_get_dma_dir(struct mmc_data *data)
> +{
> + if (data->flags & MMC_DATA_WRITE)
> + return DMA_TO_DEVICE;
> + else
> + return DMA_FROM_DEVICE;
> +}
> +
> +static int sunxi_mmc_prepare_dma(struct sunxi_mmc_host *smc_host,
> + struct mmc_data *data)
> +{
> + u32 dma_len;
> + u32 i;
> + u32 temp;
> + struct scatterlist *sg;
> +
> + dma_len = dma_map_sg(mmc_dev(smc_host->mmc), data->sg, data->sg_len,
> + sunxi_mmc_get_dma_dir(data));
> + if (dma_len == 0) {
> + dev_err(mmc_dev(smc_host->mmc), "dma_map_sg failed\n");
> + return -ENOMEM;
> + }
> +
> + for_each_sg(data->sg, sg, data->sg_len, i) {
> + if (sg->offset & 3 || sg->length & 3) {
> + dev_err(mmc_dev(smc_host->mmc),
> + "unaligned scatterlist: os %x length %d\n",
> + sg->offset, sg->length);
> + return -EINVAL;
> + }
> + }
> +
> + sunxi_mmc_init_idma_des(smc_host, data);
> +
> + temp = mci_readl(smc_host, REG_GCTRL);
> + temp |= SDXC_DMA_ENABLE_BIT;
> + mci_writel(smc_host, REG_GCTRL, temp);
> + temp |= SDXC_DMA_RESET;
> + mci_writel(smc_host, REG_GCTRL, temp);

Does it really need to be done in two steps?

(Newline)

> + mci_writel(smc_host, REG_DMAC, SDXC_IDMAC_SOFT_RESET);
> +
> + if (!(data->flags & MMC_DATA_WRITE))
> + mci_writel(smc_host, REG_IDIE, SDXC_IDMAC_RECEIVE_INTERRUPT);
> +
> + mci_writel(smc_host, REG_DMAC, SDXC_IDMAC_FIX_BURST | SDXC_IDMAC_IDMA_ON);
> +
> + return 0;
> +}
> +
> +static void sunxi_mmc_send_manual_stop(struct sunxi_mmc_host *host,
> + struct mmc_request *req)
> +{
> + u32 cmd_val = SDXC_START | SDXC_RESP_EXPIRE | SDXC_STOP_ABORT_CMD
> + | SDXC_CHECK_RESPONSE_CRC | MMC_STOP_TRANSMISSION;
> + u32 ri = 0;
> + unsigned long expire = jiffies + msecs_to_jiffies(1000);
> +
> + mci_writel(host, REG_CARG, 0);
> + mci_writel(host, REG_CMDR, cmd_val);
> +
> + do {
> + ri = mci_readl(host, REG_RINTR);
> + } while (!(ri & (SDXC_COMMAND_DONE | SDXC_INTERRUPT_ERROR_BIT)) &&
> + time_before(jiffies, expire));
> +
> + if (ri & SDXC_INTERRUPT_ERROR_BIT) {
> + dev_err(mmc_dev(host->mmc), "send stop command failed\n");
> + if (req->stop)
> + req->stop->resp[0] = -ETIMEDOUT;
> + } else {
> + if (req->stop)
> + req->stop->resp[0] = mci_readl(host, REG_RESP0);
> + }
> +
> + mci_writel(host, REG_RINTR, 0xffff);
> +}
> +
> +static void sunxi_mmc_dump_errinfo(struct sunxi_mmc_host *smc_host)
> +{
> + struct mmc_command *cmd = smc_host->mrq->cmd;
> + struct mmc_data *data = smc_host->mrq->data;
> +
> + /* For some cmds timeout is normal with sd/mmc cards */
> + if ((smc_host->int_sum & SDXC_INTERRUPT_ERROR_BIT) == SDXC_RESP_TIMEOUT &&
> + (cmd->opcode == SD_IO_SEND_OP_COND || cmd->opcode == SD_IO_RW_DIRECT))
> + return;
> +
> + dev_err(mmc_dev(smc_host->mmc),

I'd rather put it at a debug loglevel.

> + "smc %d err, cmd %d,%s%s%s%s%s%s%s%s%s%s !!\n",
> + smc_host->mmc->index, cmd->opcode,
> + data ? (data->flags & MMC_DATA_WRITE ? " WR" : " RD") : "",
> + smc_host->int_sum & SDXC_RESP_ERROR ? " RE" : "",
> + smc_host->int_sum & SDXC_RESP_CRC_ERROR ? " RCE" : "",
> + smc_host->int_sum & SDXC_DATA_CRC_ERROR ? " DCE" : "",
> + smc_host->int_sum & SDXC_RESP_TIMEOUT ? " RTO" : "",
> + smc_host->int_sum & SDXC_DATA_TIMEOUT ? " DTO" : "",
> + smc_host->int_sum & SDXC_FIFO_RUN_ERROR ? " FE" : "",
> + smc_host->int_sum & SDXC_HARD_WARE_LOCKED ? " HL" : "",
> + smc_host->int_sum & SDXC_START_BIT_ERROR ? " SBE" : "",
> + smc_host->int_sum & SDXC_END_BIT_ERROR ? " EBE" : ""
> + );
> +}
> +
> +static void sunxi_mmc_finalize_request(struct sunxi_mmc_host *host)
> +{
> + struct mmc_request *mrq;
> + unsigned long iflags;
> +
> + spin_lock_irqsave(&host->lock, iflags);
> +
> + mrq = host->mrq;
> + if (!mrq) {
> + spin_unlock_irqrestore(&host->lock, iflags);
> + dev_err(mmc_dev(host->mmc), "no request to finalize\n");
> + return;
> + }
> +
> + if (host->int_sum & SDXC_INTERRUPT_ERROR_BIT) {
> + sunxi_mmc_dump_errinfo(host);
> + mrq->cmd->error = -ETIMEDOUT;

Newline

> + if (mrq->data)
> + mrq->data->error = -ETIMEDOUT;

Newline

> + if (mrq->stop)
> + mrq->stop->error = -ETIMEDOUT;
> + } else {
> + if (mrq->cmd->flags & MMC_RSP_136) {
> + mrq->cmd->resp[0] = mci_readl(host, REG_RESP3);
> + mrq->cmd->resp[1] = mci_readl(host, REG_RESP2);
> + mrq->cmd->resp[2] = mci_readl(host, REG_RESP1);
> + mrq->cmd->resp[3] = mci_readl(host, REG_RESP0);
> + } else {
> + mrq->cmd->resp[0] = mci_readl(host, REG_RESP0);
> + }
> +

Newline

> + if (mrq->data)
> + mrq->data->bytes_xfered =
> + mrq->data->blocks * mrq->data->blksz;
> + }
> +
> + if (mrq->data) {
> + struct mmc_data *data = mrq->data;
> + u32 temp;
> +
> + mci_writel(host, REG_IDST, 0x337);
> + mci_writel(host, REG_DMAC, 0);
> + temp = mci_readl(host, REG_GCTRL);
> + mci_writel(host, REG_GCTRL, temp|SDXC_DMA_RESET);
> + temp &= ~SDXC_DMA_ENABLE_BIT;
> + mci_writel(host, REG_GCTRL, temp);
> + temp |= SDXC_FIFO_RESET;
> + mci_writel(host, REG_GCTRL, temp);
> + dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
> + sunxi_mmc_get_dma_dir(data));
> + }
> +
> + mci_writel(host, REG_RINTR, 0xffff);
> +
> + dev_dbg(mmc_dev(host->mmc), "req done, resp %08x %08x %08x %08x\n",
> + mrq->cmd->resp[0], mrq->cmd->resp[1],
> + mrq->cmd->resp[2], mrq->cmd->resp[3]);
> +
> + host->mrq = NULL;
> + host->int_sum = 0;
> + host->wait_dma = 0;
> +
> + spin_unlock_irqrestore(&host->lock, iflags);
> +
> + if (mrq->data && mrq->data->error) {
> + dev_err(mmc_dev(host->mmc),
> + "data error, sending stop command\n");
> + sunxi_mmc_send_manual_stop(host, mrq);
> + }
> +
> + mmc_request_done(host->mmc, mrq);
> +}
> +
> +static irqreturn_t sunxi_mmc_irq(int irq, void *dev_id)
> +{
> + struct sunxi_mmc_host *host = dev_id;
> + u32 finalize = 0;
> + u32 sdio_int = 0;
> + u32 msk_int;
> + u32 idma_int;
> +
> + spin_lock(&host->lock);
> +
> + idma_int = mci_readl(host, REG_IDST);
> + msk_int = mci_readl(host, REG_MISTA);
> +
> + dev_dbg(mmc_dev(host->mmc), "irq: rq %p mi %08x idi %08x\n",
> + host->mrq, msk_int, idma_int);
> +
> + if (host->mrq) {
> + if (idma_int & SDXC_IDMAC_RECEIVE_INTERRUPT)
> + host->wait_dma = 0;
> +
> + host->int_sum |= msk_int;
> +
> + /* Wait for COMMAND_DONE on RESPONSE_TIMEOUT before finishing the req */
> + if ((host->int_sum & SDXC_RESP_TIMEOUT) &&
> + !(host->int_sum & SDXC_COMMAND_DONE))
> + mci_writel(host, REG_IMASK,
> + host->sdio_imask | SDXC_COMMAND_DONE);
> + else if (host->int_sum & SDXC_INTERRUPT_ERROR_BIT)
> + finalize = 1; /* Don't wait for dma on error */
> + else if (host->int_sum & SDXC_INTERRUPT_DONE_BIT && !host->wait_dma)
> + finalize = 1; /* Done */

Usually, the comments are not placed on the same line, but on a
separate line just above (like you did a few lines earlier).

> +
> + if (finalize) {
> + mci_writel(host, REG_IMASK, host->sdio_imask);
> + mci_writel(host, REG_IDIE, 0);
> + }
> + }
> +
> + if (msk_int & SDXC_SDIO_INTERRUPT)
> + sdio_int = 1;
> +
> + mci_writel(host, REG_RINTR, msk_int);
> + mci_writel(host, REG_IDST, idma_int);
> +
> + spin_unlock(&host->lock);
> +
> + if (finalize)
> + tasklet_schedule(&host->tasklet);
> +
> + if (sdio_int)
> + mmc_signal_sdio_irq(host->mmc);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void sunxi_mmc_tasklet(unsigned long data)
> +{
> + struct sunxi_mmc_host *smc_host = (struct sunxi_mmc_host *) data;
> + sunxi_mmc_finalize_request(smc_host);
> +}
> +
> +static void sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32 oclk_en)
> +{
> + unsigned long expire = jiffies + msecs_to_jiffies(2000);
> + u32 rval;
> +
> + rval = mci_readl(host, REG_CLKCR);
> + rval &= ~(SDXC_CARD_CLOCK_ON | SDXC_LOW_POWER_ON);
> +
> + if (oclk_en)
> + rval |= SDXC_CARD_CLOCK_ON;
> +
> + if (!host->io_flag)
> + rval |= SDXC_LOW_POWER_ON;
> +
> + mci_writel(host, REG_CLKCR, rval);
> +
> + rval = SDXC_START | SDXC_UPCLK_ONLY | SDXC_WAIT_PRE_OVER;
> + if (host->voltage_switching)
> + rval |= SDXC_VOLTAGE_SWITCH;
> + mci_writel(host, REG_CMDR, rval);
> +
> + do {
> + rval = mci_readl(host, REG_CMDR);
> + } while (time_before(jiffies, expire) && (rval & SDXC_START));
> +
> + if (rval & SDXC_START) {
> + dev_err(mmc_dev(host->mmc), "fatal err update clk timeout\n");
> + host->ferror = 1;
> + }
> +}
> +
> +static void sunxi_mmc_set_clk_dly(struct sunxi_mmc_host *smc_host,
> + u32 oclk_dly, u32 sclk_dly)
> +{
> + unsigned long iflags;
> + struct clk_hw *hw = __clk_get_hw(smc_host->clk_mod);
> +
> + spin_lock_irqsave(&smc_host->lock, iflags);
> + clk_sunxi_mmc_phase_control(hw, sclk_dly, oclk_dly);
> + spin_unlock_irqrestore(&smc_host->lock, iflags);
> +}
> +
> +struct sunxi_mmc_clk_dly mmc_clk_dly[MMC_CLK_MOD_NUM] = {
> + { MMC_CLK_400K, 0, 7 },
> + { MMC_CLK_25M, 0, 5 },
> + { MMC_CLK_50M, 3, 5 },
> + { MMC_CLK_50MDDR, 2, 4 },
> + { MMC_CLK_50MDDR_8BIT, 2, 4 },
> + { MMC_CLK_100M, 1, 4 },
> + { MMC_CLK_200M, 1, 4 },
> +};
> +
> +static void sunxi_mmc_clk_set_rate(struct sunxi_mmc_host *smc_host,
> + unsigned int rate)
> +{
> + u32 newrate;
> + u32 src_clk;
> + u32 oclk_dly;
> + u32 sclk_dly;
> + u32 temp;
> + struct sunxi_mmc_clk_dly *dly = NULL;
> +
> + newrate = clk_round_rate(smc_host->clk_mod, rate);
> + if (smc_host->clk_mod_rate == newrate) {
> + dev_dbg(mmc_dev(smc_host->mmc), "clk already %d, rounded %d\n",
> + rate, newrate);
> + return;
> + }
> +
> + dev_dbg(mmc_dev(smc_host->mmc), "setting clk to %d, rounded %d\n",
> + rate, newrate);
> +
> + /* setting clock rate */
> + clk_disable(smc_host->clk_mod);
> + clk_set_rate(smc_host->clk_mod, newrate);
> + clk_enable(smc_host->clk_mod);

You don't need to disable/enable the clocks here.

> + smc_host->clk_mod_rate = newrate = clk_get_rate(smc_host->clk_mod);
> + dev_dbg(mmc_dev(smc_host->mmc), "clk is now %d\n", newrate);

You don't seem to use this newrate variable somewhere else in the
function, can't you just use the clk_mod_rate one?

And it doesn't seem like you use clk_mod_rate much either.

> +
> + sunxi_mmc_oclk_onoff(smc_host, 0);
> + /* clear internal divider */
> + temp = mci_readl(smc_host, REG_CLKCR);
> + temp &= ~0xff;
> + mci_writel(smc_host, REG_CLKCR, temp);
> +
> + /* determine delays */
> + if (rate <= 400000) {
> + dly = &mmc_clk_dly[MMC_CLK_400K];
> + } else if (rate <= 25000000) {
> + dly = &mmc_clk_dly[MMC_CLK_25M];
> + } else if (rate <= 50000000) {
> + if (smc_host->ddr) {
> + if (smc_host->bus_width == 8)
> + dly = &mmc_clk_dly[MMC_CLK_50MDDR_8BIT];
> + else
> + dly = &mmc_clk_dly[MMC_CLK_50MDDR];
> + } else {
> + dly = &mmc_clk_dly[MMC_CLK_50M];
> + }
> + } else if (rate <= 104000000) {
> + dly = &mmc_clk_dly[MMC_CLK_100M];
> + } else if (rate <= 208000000) {
> + dly = &mmc_clk_dly[MMC_CLK_200M];
> + } else {
> + dly = &mmc_clk_dly[MMC_CLK_50M];
> + }
> +
> + oclk_dly = dly->oclk_dly;
> + sclk_dly = dly->sclk_dly;
> +
> + src_clk = clk_get_rate(clk_get_parent(smc_host->clk_mod));
> +
> + if (src_clk >= 300000000 && src_clk <= 400000000) {
> + if (oclk_dly)
> + oclk_dly--;
> + if (sclk_dly)
> + sclk_dly--;
> + }
> +
> + sunxi_mmc_set_clk_dly(smc_host, oclk_dly, sclk_dly);
> + sunxi_mmc_oclk_onoff(smc_host, 1);
> +
> + /* oclk_onoff sets various irq status bits, clear these */
> + mci_writel(smc_host, REG_RINTR,
> + mci_readl(smc_host, REG_RINTR) & ~SDXC_SDIO_INTERRUPT);
> +}
> +
> +static void sunxi_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> +{
> + struct sunxi_mmc_host *host = mmc_priv(mmc);
> + u32 temp;
> + s32 err;
> +
> + /* Set the power state */
> + switch (ios->power_mode) {
> + case MMC_POWER_ON:
> + break;
> +
> + case MMC_POWER_UP:
> + if (!IS_ERR(host->vmmc)) {
> + mmc_regulator_set_ocr(host->mmc, host->vmmc, ios->vdd);
> + udelay(200);
> + }
> +
> + err = sunxi_mmc_init_host(mmc);
> + if (err) {
> + host->ferror = 1;
> + return;
> + }

Newline

> + enable_irq(host->irq);
> +
> + dev_dbg(mmc_dev(host->mmc), "power on!\n");
> + host->ferror = 0;
> + break;
> +
> + case MMC_POWER_OFF:
> + dev_dbg(mmc_dev(host->mmc), "power off!\n");
> + disable_irq(host->irq);
> + sunxi_mmc_exit_host(host);
> + if (!IS_ERR(host->vmmc))
> + mmc_regulator_set_ocr(host->mmc, host->vmmc, 0);

Newline

> + host->ferror = 0;
> + break;
> + }
> +
> + /* set bus width */
> + switch (ios->bus_width) {
> + case MMC_BUS_WIDTH_1:
> + mci_writel(host, REG_WIDTH, SDXC_WIDTH1);
> + host->bus_width = 1;
> + break;
> + case MMC_BUS_WIDTH_4:
> + mci_writel(host, REG_WIDTH, SDXC_WIDTH4);
> + host->bus_width = 4;
> + break;
> + case MMC_BUS_WIDTH_8:
> + mci_writel(host, REG_WIDTH, SDXC_WIDTH8);
> + host->bus_width = 8;
> + break;
> + }
> +
> + /* set ddr mode */
> + temp = mci_readl(host, REG_GCTRL);
> + if (ios->timing == MMC_TIMING_UHS_DDR50) {
> + temp |= SDXC_DDR_MODE;
> + host->ddr = 1;
> + } else {
> + temp &= ~SDXC_DDR_MODE;
> + host->ddr = 0;
> + }
> + mci_writel(host, REG_GCTRL, temp);
> +
> + /* set up clock */
> + if (ios->clock && ios->power_mode) {
> + dev_dbg(mmc_dev(host->mmc), "ios->clock: %d\n", ios->clock);
> + sunxi_mmc_clk_set_rate(host, ios->clock);
> + usleep_range(50000, 55000);
> + }
> +}
> +
> +static void sunxi_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
> +{
> + struct sunxi_mmc_host *smc_host = mmc_priv(mmc);
> + unsigned long flags;
> + int ret;
> + u32 imask;
> +
> + spin_lock_irqsave(&smc_host->lock, flags);
> +
> + /* Make sure the controller is in a sane state before enabling irqs */
> + ret = sunxi_mmc_init_host(mmc);
> + if (ret) {
> + spin_unlock_irqrestore(&smc_host->lock, flags);
> + return;
> + }
> +
> + imask = mci_readl(smc_host, REG_IMASK);
> + if (enable) {
> + smc_host->sdio_imask = SDXC_SDIO_INTERRUPT;
> + imask |= SDXC_SDIO_INTERRUPT;
> + } else {
> + smc_host->sdio_imask = 0;
> + imask &= ~SDXC_SDIO_INTERRUPT;
> + }
> + mci_writel(smc_host, REG_IMASK, imask);
> + spin_unlock_irqrestore(&smc_host->lock, flags);
> +}
> +
> +static void sunxi_mmc_hw_reset(struct mmc_host *mmc)
> +{
> + struct sunxi_mmc_host *smc_host = mmc_priv(mmc);
> + mci_writel(smc_host, REG_HWRST, 0);
> + udelay(10);
> + mci_writel(smc_host, REG_HWRST, 1);
> + udelay(300);
> +}
> +
> +static void sunxi_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq)
> +{
> + struct sunxi_mmc_host *host = mmc_priv(mmc);
> + struct mmc_command *cmd = mrq->cmd;
> + struct mmc_data *data = mrq->data;
> + unsigned long iflags;
> + u32 imask = SDXC_INTERRUPT_ERROR_BIT;
> + u32 cmd_val = SDXC_START | (cmd->opcode & 0x3f);
> + u32 byte_cnt = 0;
> + int ret;
> +
> + if (!mmc_gpio_get_cd(mmc) || host->ferror) {
> + dev_dbg(mmc_dev(host->mmc), "no medium present\n");
> + mrq->cmd->error = -ENOMEDIUM;
> + mmc_request_done(mmc, mrq);
> + return;
> + }
> +
> + if (data) {
> + byte_cnt = data->blksz * data->blocks;
> + mci_writel(host, REG_BLKSZ, data->blksz);
> + mci_writel(host, REG_BCNTR, byte_cnt);
> + ret = sunxi_mmc_prepare_dma(host, data);
> + if (ret < 0) {
> + dev_err(mmc_dev(host->mmc), "prepare DMA failed\n");
> + cmd->error = ret;
> + cmd->data->error = ret;
> + mmc_request_done(host->mmc, mrq);
> + return;
> + }
> + }
> +
> + if (cmd->opcode == MMC_GO_IDLE_STATE) {
> + cmd_val |= SDXC_SEND_INIT_SEQUENCE;
> + imask |= SDXC_COMMAND_DONE;
> + }
> +
> + if (cmd->opcode == SD_SWITCH_VOLTAGE) {
> + cmd_val |= SDXC_VOLTAGE_SWITCH;
> + imask |= SDXC_VOLTAGE_CHANGE_DONE;
> + host->voltage_switching = 1;
> + sunxi_mmc_oclk_onoff(host, 1);
> + }
> +
> + if (cmd->flags & MMC_RSP_PRESENT) {
> + cmd_val |= SDXC_RESP_EXPIRE;
> + if (cmd->flags & MMC_RSP_136)
> + cmd_val |= SDXC_LONG_RESPONSE;
> + if (cmd->flags & MMC_RSP_CRC)
> + cmd_val |= SDXC_CHECK_RESPONSE_CRC;
> +
> + if ((cmd->flags & MMC_CMD_MASK) == MMC_CMD_ADTC) {
> + cmd_val |= SDXC_DATA_EXPIRE | SDXC_WAIT_PRE_OVER;
> + if (cmd->data->flags & MMC_DATA_STREAM) {
> + imask |= SDXC_AUTO_COMMAND_DONE;
> + cmd_val |= SDXC_SEQUENCE_MODE | SDXC_SEND_AUTO_STOP;
> + }

Newline

> + if (cmd->data->stop) {
> + imask |= SDXC_AUTO_COMMAND_DONE;
> + cmd_val |= SDXC_SEND_AUTO_STOP;
> + } else
> + imask |= SDXC_DATA_OVER;

You should have braces here

> + if (cmd->data->flags & MMC_DATA_WRITE)
> + cmd_val |= SDXC_WRITE;
> + else
> + host->wait_dma = 1;
> + } else
> + imask |= SDXC_COMMAND_DONE;

And here

> + } else
> + imask |= SDXC_COMMAND_DONE;

And here

> + dev_dbg(mmc_dev(host->mmc), "cmd %d(%08x) arg %x ie 0x%08x len %d\n",
> + cmd_val & 0x3f, cmd_val, cmd->arg, imask,
> + mrq->data ? mrq->data->blksz * mrq->data->blocks : 0);
> +
> + spin_lock_irqsave(&host->lock, iflags);
> + host->mrq = mrq;
> + mci_writel(host, REG_IMASK, host->sdio_imask | imask);
> + spin_unlock_irqrestore(&host->lock, iflags);
> +
> + mci_writel(host, REG_CARG, cmd->arg);
> + mci_writel(host, REG_CMDR, cmd_val);
> +}
> +
> +static const struct of_device_id sunxi_mmc_of_match[] = {
> + { .compatible = "allwinner,sun4i-a10-mmc", },
> + { .compatible = "allwinner,sun5i-a13-mmc", },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, sunxi_mmc_of_match);
> +
> +static struct mmc_host_ops sunxi_mmc_ops = {
> + .request = sunxi_mmc_request,
> + .set_ios = sunxi_mmc_set_ios,
> + .get_ro = mmc_gpio_get_ro,
> + .get_cd = mmc_gpio_get_cd,
> + .enable_sdio_irq = sunxi_mmc_enable_sdio_irq,
> + .hw_reset = sunxi_mmc_hw_reset,
> +};
> +
> +static int sunxi_mmc_resource_request(struct sunxi_mmc_host *host,
> + struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + int ret;
> +
> + if (of_device_is_compatible(np, "allwinner,sun4i-a10-mmc"))
> + host->idma_des_size_bits = 13;
> + else
> + host->idma_des_size_bits = 16;
> +
> + host->vmmc = devm_regulator_get_optional(&pdev->dev, "vmmc");
> + if (IS_ERR(host->vmmc) && PTR_ERR(host->vmmc) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> +
> + host->reg_base = devm_ioremap_resource(&pdev->dev,
> + platform_get_resource(pdev, IORESOURCE_MEM, 0));
> + if (IS_ERR(host->reg_base))
> + return PTR_ERR(host->reg_base);
> +
> + host->clk_ahb = devm_clk_get(&pdev->dev, "ahb");
> + if (IS_ERR(host->clk_ahb)) {
> + dev_err(&pdev->dev, "Could not get ahb clock\n");
> + return PTR_ERR(host->clk_ahb);
> + }
> +
> + host->clk_mod = devm_clk_get(&pdev->dev, "mod");
> + if (IS_ERR(host->clk_mod)) {
> + dev_err(&pdev->dev, "Could not get mod clock\n");
> + return PTR_ERR(host->clk_mod);
> + }
> +
> + /* Make sure the controller is in a sane state before enabling irqs */
> + ret = sunxi_mmc_init_host(host->mmc);
> + if (ret)
> + return ret;
> +
> + host->irq = platform_get_irq(pdev, 0);
> + ret = devm_request_irq(&pdev->dev, host->irq, sunxi_mmc_irq, 0,
> + "sunxi-mmc", host);
> + if (ret == 0)
> + disable_irq(host->irq);

The disable_irq is useless here. Just exit.

> +
> + /* And put it back in reset */
> + sunxi_mmc_exit_host(host);

Hu? If it's in reset, how can it generate some IRQs?

> + return ret;
> +}
> +
> +static int sunxi_mmc_probe(struct platform_device *pdev)
> +{
> + struct sunxi_mmc_host *host;
> + struct mmc_host *mmc;
> + int ret;
> +
> + mmc = mmc_alloc_host(sizeof(struct sunxi_mmc_host), &pdev->dev);
> + if (!mmc) {
> + dev_err(&pdev->dev, "mmc alloc host failed\n");
> + return -ENOMEM;
> + }
> +
> + ret = mmc_of_parse(mmc);
> + if (ret)
> + goto error_free_host;
> +
> + host = mmc_priv(mmc);
> + host->mmc = mmc;
> + spin_lock_init(&host->lock);
> + tasklet_init(&host->tasklet, sunxi_mmc_tasklet, (unsigned long)host);
> +
> + ret = sunxi_mmc_resource_request(host, pdev);
> + if (ret)
> + goto error_free_host;
> +
> + host->sg_cpu = dma_alloc_coherent(&pdev->dev, PAGE_SIZE,
> + &host->sg_dma, GFP_KERNEL);
> + if (!host->sg_cpu) {
> + dev_err(&pdev->dev, "Failed to allocate DMA descriptor mem\n");
> + ret = -ENOMEM;
> + goto error_free_host;
> + }
> +
> + mmc->ops = &sunxi_mmc_ops;
> + mmc->max_blk_count = 8192;
> + mmc->max_blk_size = 4096;
> + mmc->max_segs = PAGE_SIZE / sizeof(struct sunxi_idma_des);
> + mmc->max_seg_size = (1 << host->idma_des_size_bits);
> + mmc->max_req_size = mmc->max_seg_size * mmc->max_segs;
> + /* 400kHz ~ 50MHz */
> + mmc->f_min = 400000;
> + mmc->f_max = 50000000;
> + /* available voltages */
> + if (!IS_ERR(host->vmmc))
> + mmc->ocr_avail = mmc_regulator_get_ocrmask(host->vmmc);
> + else
> + mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
> +
> + mmc->caps = MMC_CAP_MMC_HIGHSPEED | MMC_CAP_SD_HIGHSPEED |
> + MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 | MMC_CAP_UHS_SDR50 |
> + MMC_CAP_UHS_DDR50 | MMC_CAP_SDIO_IRQ | MMC_CAP_DRIVER_TYPE_A;
> + mmc->caps2 = MMC_CAP2_NO_PRESCAN_POWERUP;
> +
> + ret = mmc_add_host(mmc);
> +
> + if (ret)
> + goto error_free_dma;
> +
> + dev_info(&pdev->dev, "base:0x%p irq:%u\n", host->reg_base, host->irq);
> + platform_set_drvdata(pdev, mmc);

This should be before the registration. Otherwise, you're racy.

> + return 0;
> +
> +error_free_dma:
> + dma_free_coherent(&pdev->dev, PAGE_SIZE, host->sg_cpu, host->sg_dma);
> +error_free_host:
> + mmc_free_host(mmc);
> + return ret;
> +}
> +
> +static int sunxi_mmc_remove(struct platform_device *pdev)
> +{
> + struct mmc_host *mmc = platform_get_drvdata(pdev);
> + struct sunxi_mmc_host *host = mmc_priv(mmc);
> +
> + mmc_remove_host(mmc);
> + sunxi_mmc_exit_host(host);
> + tasklet_disable(&host->tasklet);
> + dma_free_coherent(&pdev->dev, PAGE_SIZE, host->sg_cpu, host->sg_dma);
> + mmc_free_host(mmc);
> +
> + return 0;
> +}
> +
> +static struct platform_driver sunxi_mmc_driver = {
> + .driver = {
> + .name = "sunxi-mmc",
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(sunxi_mmc_of_match),
> + },
> + .probe = sunxi_mmc_probe,
> + .remove = sunxi_mmc_remove,
> +};
> +module_platform_driver(sunxi_mmc_driver);
> +
> +MODULE_DESCRIPTION("Allwinner's SD/MMC Card Controller Driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("David Lanzend?rfer <[email protected]>");
> +MODULE_ALIAS("platform:sunxi-mmc");
> diff --git a/drivers/mmc/host/sunxi-mmc.h b/drivers/mmc/host/sunxi-mmc.h
> new file mode 100644
> index 0000000..75eaa02
> --- /dev/null
> +++ b/drivers/mmc/host/sunxi-mmc.h
> @@ -0,0 +1,239 @@
> +/*
> + * Driver for sunxi SD/MMC host controllers
> + * (C) Copyright 2007-2011 Reuuimlla Technology Co., Ltd.
> + * (C) Copyright 2007-2011 Aaron Maoye <[email protected]>
> + * (C) Copyright 2013-2014 O2S GmbH <http://www.o2s.ch>
> + * (C) Copyright 2013-2014 David Lanzend?rfer <[email protected]>
> + * (C) Copyright 2013-2014 Hans de Goede <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + */
> +
> +#ifndef __SUNXI_MMC_H__
> +#define __SUNXI_MMC_H__
> +
> +/* register offset definitions */
> +#define SDXC_REG_GCTRL (0x00) /* SMC Global Control Register */
> +#define SDXC_REG_CLKCR (0x04) /* SMC Clock Control Register */
> +#define SDXC_REG_TMOUT (0x08) /* SMC Time Out Register */
> +#define SDXC_REG_WIDTH (0x0C) /* SMC Bus Width Register */
> +#define SDXC_REG_BLKSZ (0x10) /* SMC Block Size Register */
> +#define SDXC_REG_BCNTR (0x14) /* SMC Byte Count Register */
> +#define SDXC_REG_CMDR (0x18) /* SMC Command Register */
> +#define SDXC_REG_CARG (0x1C) /* SMC Argument Register */
> +#define SDXC_REG_RESP0 (0x20) /* SMC Response Register 0 */
> +#define SDXC_REG_RESP1 (0x24) /* SMC Response Register 1 */
> +#define SDXC_REG_RESP2 (0x28) /* SMC Response Register 2 */
> +#define SDXC_REG_RESP3 (0x2C) /* SMC Response Register 3 */
> +#define SDXC_REG_IMASK (0x30) /* SMC Interrupt Mask Register */
> +#define SDXC_REG_MISTA (0x34) /* SMC Masked Interrupt Status Register */
> +#define SDXC_REG_RINTR (0x38) /* SMC Raw Interrupt Status Register */
> +#define SDXC_REG_STAS (0x3C) /* SMC Status Register */
> +#define SDXC_REG_FTRGL (0x40) /* SMC FIFO Threshold Watermark Registe */
> +#define SDXC_REG_FUNS (0x44) /* SMC Function Select Register */
> +#define SDXC_REG_CBCR (0x48) /* SMC CIU Byte Count Register */
> +#define SDXC_REG_BBCR (0x4C) /* SMC BIU Byte Count Register */
> +#define SDXC_REG_DBGC (0x50) /* SMC Debug Enable Register */
> +#define SDXC_REG_HWRST (0x78) /* SMC Card Hardware Reset for Register */
> +#define SDXC_REG_DMAC (0x80) /* SMC IDMAC Control Register */
> +#define SDXC_REG_DLBA (0x84) /* SMC IDMAC Descriptor List Base Addre */
> +#define SDXC_REG_IDST (0x88) /* SMC IDMAC Status Register */
> +#define SDXC_REG_IDIE (0x8C) /* SMC IDMAC Interrupt Enable Register */
> +#define SDXC_REG_CHDA (0x90)
> +#define SDXC_REG_CBDA (0x94)
> +
> +#define mci_readl(host, reg) \
> + readl((host)->reg_base + SDXC_##reg)
> +#define mci_writel(host, reg, value) \
> + writel((value), (host)->reg_base + SDXC_##reg)

Please use some inline functions here.

> +/* global control register bits */
> +#define SDXC_SOFT_RESET BIT(0)
> +#define SDXC_FIFO_RESET BIT(1)
> +#define SDXC_DMA_RESET BIT(2)
> +#define SDXC_HARDWARE_RESET (SDXC_SOFT_RESET|SDXC_FIFO_RESET|SDXC_DMA_RESET)
> +#define SDXC_INTERRUPT_ENABLE_BIT BIT(4)
> +#define SDXC_DMA_ENABLE_BIT BIT(5)
> +#define SDXC_DEBOUNCE_ENABLE_BIT BIT(8)
> +#define SDXC_POSEDGE_LATCH_DATA BIT(9)
> +#define SDXC_DDR_MODE BIT(10)
> +#define SDXC_MEMORY_ACCESS_DONE BIT(29)
> +#define SDXC_ACCESS_DONE_DIRECT BIT(30)
> +#define SDXC_ACCESS_BY_AHB BIT(31)
> +#define SDXC_ACCESS_BY_DMA (0U << 31)

Isn't it 0?

> +/* clock control bits */
> +#define SDXC_CARD_CLOCK_ON BIT(16)
> +#define SDXC_LOW_POWER_ON BIT(17)
> +/* bus width */
> +#define SDXC_WIDTH1 (0)
> +#define SDXC_WIDTH4 (1)
> +#define SDXC_WIDTH8 (2)
> +/* smc command bits */
> +#define SDXC_RESP_EXPIRE BIT(6)
> +#define SDXC_LONG_RESPONSE BIT(7)
> +#define SDXC_CHECK_RESPONSE_CRC BIT(8)
> +#define SDXC_DATA_EXPIRE BIT(9)
> +#define SDXC_WRITE BIT(10)
> +#define SDXC_SEQUENCE_MODE BIT(11)
> +#define SDXC_SEND_AUTO_STOP BIT(12)
> +#define SDXC_WAIT_PRE_OVER BIT(13)
> +#define SDXC_STOP_ABORT_CMD BIT(14)
> +#define SDXC_SEND_INIT_SEQUENCE BIT(15)
> +#define SDXC_UPCLK_ONLY BIT(21)
> +#define SDXC_READ_CEATA_DEV BIT(22)
> +#define SDXC_CCS_EXPIRE BIT(23)
> +#define SDXC_ENABLE_BIT_BOOT BIT(24)
> +#define SDXC_ALT_BOOT_OPTIONS BIT(25)
> +#define SDXC_BOOT_ACK_EXPIRE BIT(26)
> +#define SDXC_BOOT_ABORT BIT(27)
> +#define SDXC_VOLTAGE_SWITCH BIT(28)
> +#define SDXC_USE_HOLD_REGISTER BIT(29)
> +#define SDXC_START BIT(31)
> +/* interrupt bits */
> +#define SDXC_RESP_ERROR BIT(1)
> +#define SDXC_COMMAND_DONE BIT(2)
> +#define SDXC_DATA_OVER BIT(3)
> +#define SDXC_TX_DATA_REQUEST BIT(4)
> +#define SDXC_RX_DATA_REQUEST BIT(5)
> +#define SDXC_RESP_CRC_ERROR BIT(6)
> +#define SDXC_DATA_CRC_ERROR BIT(7)
> +#define SDXC_RESP_TIMEOUT BIT(8)
> +#define SDXC_DATA_TIMEOUT BIT(9)
> +#define SDXC_VOLTAGE_CHANGE_DONE BIT(10)
> +#define SDXC_FIFO_RUN_ERROR BIT(11)
> +#define SDXC_HARD_WARE_LOCKED BIT(12)
> +#define SDXC_START_BIT_ERROR BIT(13)
> +#define SDXC_AUTO_COMMAND_DONE BIT(14)
> +#define SDXC_END_BIT_ERROR BIT(15)
> +#define SDXC_SDIO_INTERRUPT BIT(16)
> +#define SDXC_CARD_INSERT BIT(30)
> +#define SDXC_CARD_REMOVE BIT(31)
> +#define SDXC_INTERRUPT_ERROR_BIT (SDXC_RESP_ERROR | SDXC_RESP_CRC_ERROR | \
> + SDXC_DATA_CRC_ERROR | SDXC_RESP_TIMEOUT | \
> + SDXC_DATA_TIMEOUT | SDXC_FIFO_RUN_ERROR | \
> + SDXC_HARD_WARE_LOCKED | SDXC_START_BIT_ERROR | \
> + SDXC_END_BIT_ERROR) /* 0xbbc2 */
> +#define SDXC_INTERRUPT_DONE_BIT (SDXC_AUTO_COMMAND_DONE | SDXC_DATA_OVER | \
> + SDXC_COMMAND_DONE | SDXC_VOLTAGE_CHANGE_DONE)
> +/* status */
> +#define SDXC_RXWL_FLAG BIT(0)
> +#define SDXC_TXWL_FLAG BIT(1)
> +#define SDXC_FIFO_EMPTY BIT(2)
> +#define SDXC_FIFO_FULL BIT(3)
> +#define SDXC_CARD_PRESENT BIT(8)
> +#define SDXC_CARD_DATA_BUSY BIT(9)
> +#define SDXC_DATA_FSM_BUSY BIT(10)
> +#define SDXC_DMA_REQUEST BIT(31)
> +#define SDXC_FIFO_SIZE (16)
> +/* Function select */
> +#define SDXC_CEATA_ON (0xceaaU << 16)
> +#define SDXC_SEND_IRQ_RESPONSE BIT(0)
> +#define SDXC_SDIO_READ_WAIT BIT(1)
> +#define SDXC_ABORT_READ_DATA BIT(2)
> +#define SDXC_SEND_CCSD BIT(8)
> +#define SDXC_SEND_AUTO_STOPCCSD BIT(9)
> +#define SDXC_CEATA_DEV_INTERRUPT_ENABLE_BIT BIT(10)
> +/* IDMA controller bus mod bit field */
> +#define SDXC_IDMAC_SOFT_RESET BIT(0)
> +#define SDXC_IDMAC_FIX_BURST BIT(1)
> +#define SDXC_IDMAC_IDMA_ON BIT(7)
> +#define SDXC_IDMAC_REFETCH_DES BIT(31)
> +/* IDMA status bit field */
> +#define SDXC_IDMAC_TRANSMIT_INTERRUPT BIT(0)
> +#define SDXC_IDMAC_RECEIVE_INTERRUPT BIT(1)
> +#define SDXC_IDMAC_FATAL_BUS_ERROR BIT(2)
> +#define SDXC_IDMAC_DESTINATION_INVALID BIT(4)
> +#define SDXC_IDMAC_CARD_ERROR_SUM BIT(5)
> +#define SDXC_IDMAC_NORMAL_INTERRUPT_SUM BIT(8)
> +#define SDXC_IDMAC_ABNORMAL_INTERRUPT_SUM BIT(9)
> +#define SDXC_IDMAC_HOST_ABORT_INTERRUPT_TX BIT(10)
> +#define SDXC_IDMAC_HOST_ABORT_INTERRUPT_RX BIT(10)
> +#define SDXC_IDMAC_IDLE (0U << 13)

Ditto

> +#define SDXC_IDMAC_SUSPEND (1U << 13)
> +#define SDXC_IDMAC_DESC_READ (2U << 13)
> +#define SDXC_IDMAC_DESC_CHECK (3U << 13)
> +#define SDXC_IDMAC_READ_REQUEST_WAIT (4U << 13)
> +#define SDXC_IDMAC_WRITE_REQUEST_WAIT (5U << 13)
> +#define SDXC_IDMAC_READ (6U << 13)
> +#define SDXC_IDMAC_WRITE (7U << 13)
> +#define SDXC_IDMAC_DESC_CLOSE (8U << 13)

Please use BIT as much as possible here.


> +
> +/*
> +* If the idma-des-size-bits of property is ie 13, bufsize bits are:
> +* Bits 0-12: buf1 size
> +* Bits 13-25: buf2 size
> +* Bits 26-31: not used
> +* Since we only ever set buf1 size, we can simply store it directly.
> +*/
> +#define SDXC_IDMAC_DES0_DIC BIT(1) /* disable interrupt on completion */
> +#define SDXC_IDMAC_DES0_LD BIT(2) /* last descriptor */
> +#define SDXC_IDMAC_DES0_FD BIT(3) /* first descriptor */
> +#define SDXC_IDMAC_DES0_CH BIT(4) /* chain mode */
> +#define SDXC_IDMAC_DES0_ER BIT(5) /* end of ring */
> +#define SDXC_IDMAC_DES0_CES BIT(30) /* card error summary */
> +#define SDXC_IDMAC_DES0_OWN BIT(31) /* 1-idma owns it, 0-host owns it */

Overall, I prefer to have the registers right beneath the register
declaration. It's easier to spot what bits belong to what registers
that way.

> +struct sunxi_idma_des {
> + u32 config;
> + u32 buf_size;
> + u32 buf_addr_ptr1;
> + u32 buf_addr_ptr2;
> +};

Some comments on top of this structure would be great.

> +struct sunxi_mmc_host {
> + struct mmc_host *mmc;
> + struct regulator *vmmc;
> +
> + /* IO mapping base */
> + void __iomem *reg_base;
> +
> + spinlock_t lock;
> + struct tasklet_struct tasklet;
> +
> + /* clock management */
> + struct clk *clk_ahb;
> + struct clk *clk_mod;
> +
> + /* ios information */
> + u32 clk_mod_rate;
> + u32 bus_width;
> + u32 idma_des_size_bits;
> + u32 ddr;
> + u32 voltage_switching;
> +
> + /* irq */
> + int irq;
> + u32 int_sum;
> + u32 sdio_imask;
> +
> + /* flags */
> + u32 power_on:1;
> + u32 io_flag:1;
> + u32 wait_dma:1;

bool?

> + dma_addr_t sg_dma;
> + void *sg_cpu;
> +
> + struct mmc_request *mrq;
> + u32 ferror;
> +};

Please be consistent in your spacing, either align the variable names,
or don't, but make a decision :)

> +#define MMC_CLK_400K 0
> +#define MMC_CLK_25M 1
> +#define MMC_CLK_50M 2
> +#define MMC_CLK_50MDDR 3
> +#define MMC_CLK_50MDDR_8BIT 4
> +#define MMC_CLK_100M 5
> +#define MMC_CLK_200M 6
> +#define MMC_CLK_MOD_NUM 7

Wouldn't an enum be better here?

> +
> +struct sunxi_mmc_clk_dly {
> + u32 mode;
> + u32 oclk_dly;
> + u32 sclk_dly;
> +};

Comments here would be great too.

Thanks for working on this!
Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


Attachments:
(No filename) (39.69 kB)
signature.asc (836.00 B)
Digital signature
Download all attachments

2014-02-18 15:45:07

by [email protected]

[permalink] [raw]
Subject: Re: [PATCH v7 8/8] ARM: sunxi: Add documentation for driver for SD/MMC hosts found on Allwinner sunxi SoCs

On Mon, Feb 17, 2014 at 11:03:02AM +0100, David Lanzend?rfer wrote:
> Signed-off-by: David Lanzend?rfer <[email protected]>
> ---
> .../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: [email protected] {
> + 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?

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


Attachments:
(No filename) (2.30 kB)
signature.asc (836.00 B)
Digital signature
Download all attachments

2014-02-19 09:50:13

by [email protected]

[permalink] [raw]
Subject: Re: [PATCH v7 4/8] ARM: sunxi: Add driver for SD/MMC hosts found on Allwinner sunxi SoCs

Hi Hans,

On Tue, Feb 18, 2014 at 09:49:21PM +0100, Hans de Goede wrote:
> Hi,
>
> On 02/18/2014 04:37 PM, Maxime Ripard wrote:
>
> <snip>
>
> >>+
> >>+ for (i = 0; i < data->sg_len; i++) {
> >>+ pdes[i].config = SDXC_IDMAC_DES0_CH | SDXC_IDMAC_DES0_OWN |
> >>+ SDXC_IDMAC_DES0_DIC;
> >>+
> >>+ if (data->sg[i].length == max_len)
> >>+ pdes[i].buf_size = 0; /* 0 == max_len */
> >>+ else
> >>+ pdes[i].buf_size = data->sg[i].length;
> >>+
> >>+ pdes[i].buf_addr_ptr1 = sg_dma_address(&data->sg[i]);
> >>+ pdes[i].buf_addr_ptr2 = (u32)&pdes_pa[i + 1];
> >>+ }
> >>+
> >>+ pdes[0].config |= SDXC_IDMAC_DES0_FD;
> >>+ pdes[i - 1].config = SDXC_IDMAC_DES0_OWN | SDXC_IDMAC_DES0_LD;
> >>+
> >>+ wmb(); /* Ensure idma_des hit main mem before we start the idmac */
> >
> >wmb ensure the proper ordering of the instructions, not flushing the
> >caches like what your comment implies.
>
> Since I put that comment there, allow me to explain. A modern ARM
> cpu core has 2 or more units handling stores. One for regular
> memory stores, and one for io-mem stores. Regular mem stores can
> be re-ordered, io stores cannot. Normally there is no "syncing"
> between the 2 store units. Cache flushing is not an issue here
> since the memory holding the descriptors for the idma controller
> is allocated cache coherent, which on arm means it is not cached.
>
> What is an issue here is the io-store starting the idmac hitting
> the io-mem before the descriptors hit the main-mem, the wmb()
> ensures this does not happen.

To expand a bit, my point was not that it was functionnally
wrong. Since you put a barrier in there, and that it resides in a
cache coherent section, we're fine.

My point was that the comment itself was misleading.

> >>+static int sunxi_mmc_prepare_dma(struct sunxi_mmc_host *smc_host,
> >>+ struct mmc_data *data)
> >>+{
> >>+ u32 dma_len;
> >>+ u32 i;
> >>+ u32 temp;
> >>+ struct scatterlist *sg;
> >>+
> >>+ dma_len = dma_map_sg(mmc_dev(smc_host->mmc), data->sg, data->sg_len,
> >>+ sunxi_mmc_get_dma_dir(data));
> >>+ if (dma_len == 0) {
> >>+ dev_err(mmc_dev(smc_host->mmc), "dma_map_sg failed\n");
> >>+ return -ENOMEM;
> >>+ }
> >>+
> >>+ for_each_sg(data->sg, sg, data->sg_len, i) {
> >>+ if (sg->offset & 3 || sg->length & 3) {
> >>+ dev_err(mmc_dev(smc_host->mmc),
> >>+ "unaligned scatterlist: os %x length %d\n",
> >>+ sg->offset, sg->length);
> >>+ return -EINVAL;
> >>+ }
> >>+ }
> >>+
> >>+ sunxi_mmc_init_idma_des(smc_host, data);
> >>+
> >>+ temp = mci_readl(smc_host, REG_GCTRL);
> >>+ temp |= SDXC_DMA_ENABLE_BIT;
> >>+ mci_writel(smc_host, REG_GCTRL, temp);
> >>+ temp |= SDXC_DMA_RESET;
> >>+ mci_writel(smc_host, REG_GCTRL, temp);
> >
> >Does it really need to be done in two steps?
>
> We don't know, so this is probably best left as is.

Ok.

> >
> >(Newline)
> >
> >>+ mci_writel(smc_host, REG_DMAC, SDXC_IDMAC_SOFT_RESET);
> >>+
> >>+ if (!(data->flags & MMC_DATA_WRITE))
> >>+ mci_writel(smc_host, REG_IDIE, SDXC_IDMAC_RECEIVE_INTERRUPT);
> >>+
> >>+ mci_writel(smc_host, REG_DMAC, SDXC_IDMAC_FIX_BURST | SDXC_IDMAC_IDMA_ON);
> >>+
> >>+ return 0;
> >>+}
> >>+
> >>+static void sunxi_mmc_send_manual_stop(struct sunxi_mmc_host *host,
> >>+ struct mmc_request *req)
> >>+{
> >>+ u32 cmd_val = SDXC_START | SDXC_RESP_EXPIRE | SDXC_STOP_ABORT_CMD
> >>+ | SDXC_CHECK_RESPONSE_CRC | MMC_STOP_TRANSMISSION;
> >>+ u32 ri = 0;
> >>+ unsigned long expire = jiffies + msecs_to_jiffies(1000);
> >>+
> >>+ mci_writel(host, REG_CARG, 0);
> >>+ mci_writel(host, REG_CMDR, cmd_val);
> >>+
> >>+ do {
> >>+ ri = mci_readl(host, REG_RINTR);
> >>+ } while (!(ri & (SDXC_COMMAND_DONE | SDXC_INTERRUPT_ERROR_BIT)) &&
> >>+ time_before(jiffies, expire));
> >>+
> >>+ if (ri & SDXC_INTERRUPT_ERROR_BIT) {
> >>+ dev_err(mmc_dev(host->mmc), "send stop command failed\n");
> >>+ if (req->stop)
> >>+ req->stop->resp[0] = -ETIMEDOUT;
> >>+ } else {
> >>+ if (req->stop)
> >>+ req->stop->resp[0] = mci_readl(host, REG_RESP0);
> >>+ }
> >>+
> >>+ mci_writel(host, REG_RINTR, 0xffff);
> >>+}
> >>+
> >>+static void sunxi_mmc_dump_errinfo(struct sunxi_mmc_host *smc_host)
> >>+{
> >>+ struct mmc_command *cmd = smc_host->mrq->cmd;
> >>+ struct mmc_data *data = smc_host->mrq->data;
> >>+
> >>+ /* For some cmds timeout is normal with sd/mmc cards */
> >>+ if ((smc_host->int_sum & SDXC_INTERRUPT_ERROR_BIT) == SDXC_RESP_TIMEOUT &&
> >>+ (cmd->opcode == SD_IO_SEND_OP_COND || cmd->opcode == SD_IO_RW_DIRECT))
> >>+ return;
> >>+
> >>+ dev_err(mmc_dev(smc_host->mmc),
> >
> >I'd rather put it at a debug loglevel.
>
> Erm, this only happens if something is seriously wrong.

Still. Something would be seriously wrong in the MMC
driver/controller. You don't want to bloat the whole kernel logs with
the dump of your registers just because the MMC is failing. This is of
no interest to anyone but someone that would actually try to debug
what's wrong.

> >>+ /* Make sure the controller is in a sane state before enabling irqs */
> >>+ ret = sunxi_mmc_init_host(host->mmc);
> >>+ if (ret)
> >>+ return ret;
> >>+
> >>+ host->irq = platform_get_irq(pdev, 0);
> >>+ ret = devm_request_irq(&pdev->dev, host->irq, sunxi_mmc_irq, 0,
> >>+ "sunxi-mmc", host);
> >>+ if (ret == 0)
> >>+ disable_irq(host->irq);
> >
> >The disable_irq is useless here. Just exit.
>
> No it is not note the ret == 0, this is not an error handling path!

Doh... Right.
My bad.

> This is done under an if because we want to do the sunxi_mmc_exit_host
> regardless of the request_irq succeeding or not.
>
> >
> >>+
> >>+ /* And put it back in reset */
> >>+ sunxi_mmc_exit_host(host);
> >
> >Hu? If it's in reset, how can it generate some IRQs?
>
> Yes, that is why we do the whole dance of init controller, get irq,
> disable irq, drop it back in reset (until the mmc subsys does a power on
> of the mmc card / sdio dev).
>
> Sometime the controller asserts the irq in reset for some reason, so
> without the dance as soon as we do the devm_request_irq we get an irq,
> and worse, not only do we get an irq, we cannot clear it since writing to
> the interrupt status register does not work when the controller is in reset,
> so we get stuck re-entering the irq handler.

Hmmm, I see. It probably deserves some commenting here too then.

> >>+ return ret;
> >>+}
> >>+
> >>+static int sunxi_mmc_probe(struct platform_device *pdev)
> >>+{
> >>+ struct sunxi_mmc_host *host;
> >>+ struct mmc_host *mmc;
> >>+ int ret;
> >>+
> >>+ mmc = mmc_alloc_host(sizeof(struct sunxi_mmc_host), &pdev->dev);
> >>+ if (!mmc) {
> >>+ dev_err(&pdev->dev, "mmc alloc host failed\n");
> >>+ return -ENOMEM;
> >>+ }
> >>+
> >>+ ret = mmc_of_parse(mmc);
> >>+ if (ret)
> >>+ goto error_free_host;
> >>+
> >>+ host = mmc_priv(mmc);
> >>+ host->mmc = mmc;
> >>+ spin_lock_init(&host->lock);
> >>+ tasklet_init(&host->tasklet, sunxi_mmc_tasklet, (unsigned long)host);
> >>+
> >>+ ret = sunxi_mmc_resource_request(host, pdev);
> >>+ if (ret)
> >>+ goto error_free_host;
> >>+
> >>+ host->sg_cpu = dma_alloc_coherent(&pdev->dev, PAGE_SIZE,
> >>+ &host->sg_dma, GFP_KERNEL);
> >>+ if (!host->sg_cpu) {
> >>+ dev_err(&pdev->dev, "Failed to allocate DMA descriptor mem\n");
> >>+ ret = -ENOMEM;
> >>+ goto error_free_host;
> >>+ }
> >>+
> >>+ mmc->ops = &sunxi_mmc_ops;
> >>+ mmc->max_blk_count = 8192;
> >>+ mmc->max_blk_size = 4096;
> >>+ mmc->max_segs = PAGE_SIZE / sizeof(struct sunxi_idma_des);
> >>+ mmc->max_seg_size = (1 << host->idma_des_size_bits);
> >>+ mmc->max_req_size = mmc->max_seg_size * mmc->max_segs;
> >>+ /* 400kHz ~ 50MHz */
> >>+ mmc->f_min = 400000;
> >>+ mmc->f_max = 50000000;
> >>+ /* available voltages */
> >>+ if (!IS_ERR(host->vmmc))
> >>+ mmc->ocr_avail = mmc_regulator_get_ocrmask(host->vmmc);
> >>+ else
> >>+ mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
> >>+
> >>+ mmc->caps = MMC_CAP_MMC_HIGHSPEED | MMC_CAP_SD_HIGHSPEED |
> >>+ MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 | MMC_CAP_UHS_SDR50 |
> >>+ MMC_CAP_UHS_DDR50 | MMC_CAP_SDIO_IRQ | MMC_CAP_DRIVER_TYPE_A;
> >>+ mmc->caps2 = MMC_CAP2_NO_PRESCAN_POWERUP;
> >>+
> >>+ ret = mmc_add_host(mmc);
> >>+
> >>+ if (ret)
> >>+ goto error_free_dma;
> >>+
> >>+ dev_info(&pdev->dev, "base:0x%p irq:%u\n", host->reg_base, host->irq);
> >>+ platform_set_drvdata(pdev, mmc);
> >
> >This should be before the registration. Otherwise, you're racy.
>
> Nope, we only need this to get the data on sunxi_mmc_remove, everywhere
> else the data is found through the mmc-host struct.

Still, if anyone makes a following patch using the platform_device for
some reason, we will have a race condition, without any way to notice
it.

Plus, you're doing all the other bits of initialization of your
structures much earlier, why not be consistent and having all of them
at the same place?

> >
> >>+ return 0;
> >>+
> >>+error_free_dma:
> >>+ dma_free_coherent(&pdev->dev, PAGE_SIZE, host->sg_cpu, host->sg_dma);
> >>+error_free_host:
> >>+ mmc_free_host(mmc);
> >>+ return ret;
> >>+}
> >>+
> >>+static int sunxi_mmc_remove(struct platform_device *pdev)
> >>+{
> >>+ struct mmc_host *mmc = platform_get_drvdata(pdev);
> >>+ struct sunxi_mmc_host *host = mmc_priv(mmc);
> >>+
> >>+ mmc_remove_host(mmc);
> >>+ sunxi_mmc_exit_host(host);
> >>+ tasklet_disable(&host->tasklet);
> >>+ dma_free_coherent(&pdev->dev, PAGE_SIZE, host->sg_cpu, host->sg_dma);
> >>+ mmc_free_host(mmc);
> >>+
> >>+ return 0;
> >>+}
> >>+
> >>+static struct platform_driver sunxi_mmc_driver = {
> >>+ .driver = {
> >>+ .name = "sunxi-mmc",
> >>+ .owner = THIS_MODULE,
> >>+ .of_match_table = of_match_ptr(sunxi_mmc_of_match),
> >>+ },
> >>+ .probe = sunxi_mmc_probe,
> >>+ .remove = sunxi_mmc_remove,
> >>+};
> >>+module_platform_driver(sunxi_mmc_driver);
> >>+
> >>+MODULE_DESCRIPTION("Allwinner's SD/MMC Card Controller Driver");
> >>+MODULE_LICENSE("GPL v2");
> >>+MODULE_AUTHOR("David Lanzend?rfer <[email protected]>");
> >>+MODULE_ALIAS("platform:sunxi-mmc");
> >>diff --git a/drivers/mmc/host/sunxi-mmc.h b/drivers/mmc/host/sunxi-mmc.h
> >>new file mode 100644
> >>index 0000000..75eaa02
> >>--- /dev/null
> >>+++ b/drivers/mmc/host/sunxi-mmc.h
> >>@@ -0,0 +1,239 @@
> >>+/*
> >>+ * Driver for sunxi SD/MMC host controllers
> >>+ * (C) Copyright 2007-2011 Reuuimlla Technology Co., Ltd.
> >>+ * (C) Copyright 2007-2011 Aaron Maoye <[email protected]>
> >>+ * (C) Copyright 2013-2014 O2S GmbH <http://www.o2s.ch>
> >>+ * (C) Copyright 2013-2014 David Lanzend?rfer <[email protected]>
> >>+ * (C) Copyright 2013-2014 Hans de Goede <[email protected]>
> >>+ *
> >>+ * This program is free software; you can redistribute it and/or
> >>+ * modify it under the terms of the GNU General Public License as
> >>+ * published by the Free Software Foundation; either version 2 of
> >>+ * the License, or (at your option) any later version.
> >>+ */
> >>+
> >>+#ifndef __SUNXI_MMC_H__
> >>+#define __SUNXI_MMC_H__
> >>+
> >>+/* register offset definitions */
> >>+#define SDXC_REG_GCTRL (0x00) /* SMC Global Control Register */
> >>+#define SDXC_REG_CLKCR (0x04) /* SMC Clock Control Register */
> >>+#define SDXC_REG_TMOUT (0x08) /* SMC Time Out Register */
> >>+#define SDXC_REG_WIDTH (0x0C) /* SMC Bus Width Register */
> >>+#define SDXC_REG_BLKSZ (0x10) /* SMC Block Size Register */
> >>+#define SDXC_REG_BCNTR (0x14) /* SMC Byte Count Register */
> >>+#define SDXC_REG_CMDR (0x18) /* SMC Command Register */
> >>+#define SDXC_REG_CARG (0x1C) /* SMC Argument Register */
> >>+#define SDXC_REG_RESP0 (0x20) /* SMC Response Register 0 */
> >>+#define SDXC_REG_RESP1 (0x24) /* SMC Response Register 1 */
> >>+#define SDXC_REG_RESP2 (0x28) /* SMC Response Register 2 */
> >>+#define SDXC_REG_RESP3 (0x2C) /* SMC Response Register 3 */
> >>+#define SDXC_REG_IMASK (0x30) /* SMC Interrupt Mask Register */
> >>+#define SDXC_REG_MISTA (0x34) /* SMC Masked Interrupt Status Register */
> >>+#define SDXC_REG_RINTR (0x38) /* SMC Raw Interrupt Status Register */
> >>+#define SDXC_REG_STAS (0x3C) /* SMC Status Register */
> >>+#define SDXC_REG_FTRGL (0x40) /* SMC FIFO Threshold Watermark Registe */
> >>+#define SDXC_REG_FUNS (0x44) /* SMC Function Select Register */
> >>+#define SDXC_REG_CBCR (0x48) /* SMC CIU Byte Count Register */
> >>+#define SDXC_REG_BBCR (0x4C) /* SMC BIU Byte Count Register */
> >>+#define SDXC_REG_DBGC (0x50) /* SMC Debug Enable Register */
> >>+#define SDXC_REG_HWRST (0x78) /* SMC Card Hardware Reset for Register */
> >>+#define SDXC_REG_DMAC (0x80) /* SMC IDMAC Control Register */
> >>+#define SDXC_REG_DLBA (0x84) /* SMC IDMAC Descriptor List Base Addre */
> >>+#define SDXC_REG_IDST (0x88) /* SMC IDMAC Status Register */
> >>+#define SDXC_REG_IDIE (0x8C) /* SMC IDMAC Interrupt Enable Register */
> >>+#define SDXC_REG_CHDA (0x90)
> >>+#define SDXC_REG_CBDA (0x94)
> >>+
> >>+#define mci_readl(host, reg) \
> >>+ readl((host)->reg_base + SDXC_##reg)
> >>+#define mci_writel(host, reg, value) \
> >>+ writel((value), (host)->reg_base + SDXC_##reg)
> >
> >Please use some inline functions here.
> >
> >>+/* global control register bits */
> >>+#define SDXC_SOFT_RESET BIT(0)
> >>+#define SDXC_FIFO_RESET BIT(1)
> >>+#define SDXC_DMA_RESET BIT(2)
> >>+#define SDXC_HARDWARE_RESET (SDXC_SOFT_RESET|SDXC_FIFO_RESET|SDXC_DMA_RESET)
> >>+#define SDXC_INTERRUPT_ENABLE_BIT BIT(4)
> >>+#define SDXC_DMA_ENABLE_BIT BIT(5)
> >>+#define SDXC_DEBOUNCE_ENABLE_BIT BIT(8)
> >>+#define SDXC_POSEDGE_LATCH_DATA BIT(9)
> >>+#define SDXC_DDR_MODE BIT(10)
> >>+#define SDXC_MEMORY_ACCESS_DONE BIT(29)
> >>+#define SDXC_ACCESS_DONE_DIRECT BIT(30)
> >>+#define SDXC_ACCESS_BY_AHB BIT(31)
> >>+#define SDXC_ACCESS_BY_DMA (0U << 31)
> >
> >Isn't it 0?
>
> Yes, but is is the inverse of ACCESS_BY_DMA, which
> this makes much clearer then just 0 does.
>
> >
> >>+/* clock control bits */
> >>+#define SDXC_CARD_CLOCK_ON BIT(16)
> >>+#define SDXC_LOW_POWER_ON BIT(17)
> >>+/* bus width */
> >>+#define SDXC_WIDTH1 (0)
> >>+#define SDXC_WIDTH4 (1)
> >>+#define SDXC_WIDTH8 (2)
> >>+/* smc command bits */
> >>+#define SDXC_RESP_EXPIRE BIT(6)
> >>+#define SDXC_LONG_RESPONSE BIT(7)
> >>+#define SDXC_CHECK_RESPONSE_CRC BIT(8)
> >>+#define SDXC_DATA_EXPIRE BIT(9)
> >>+#define SDXC_WRITE BIT(10)
> >>+#define SDXC_SEQUENCE_MODE BIT(11)
> >>+#define SDXC_SEND_AUTO_STOP BIT(12)
> >>+#define SDXC_WAIT_PRE_OVER BIT(13)
> >>+#define SDXC_STOP_ABORT_CMD BIT(14)
> >>+#define SDXC_SEND_INIT_SEQUENCE BIT(15)
> >>+#define SDXC_UPCLK_ONLY BIT(21)
> >>+#define SDXC_READ_CEATA_DEV BIT(22)
> >>+#define SDXC_CCS_EXPIRE BIT(23)
> >>+#define SDXC_ENABLE_BIT_BOOT BIT(24)
> >>+#define SDXC_ALT_BOOT_OPTIONS BIT(25)
> >>+#define SDXC_BOOT_ACK_EXPIRE BIT(26)
> >>+#define SDXC_BOOT_ABORT BIT(27)
> >>+#define SDXC_VOLTAGE_SWITCH BIT(28)
> >>+#define SDXC_USE_HOLD_REGISTER BIT(29)
> >>+#define SDXC_START BIT(31)
> >>+/* interrupt bits */
> >>+#define SDXC_RESP_ERROR BIT(1)
> >>+#define SDXC_COMMAND_DONE BIT(2)
> >>+#define SDXC_DATA_OVER BIT(3)
> >>+#define SDXC_TX_DATA_REQUEST BIT(4)
> >>+#define SDXC_RX_DATA_REQUEST BIT(5)
> >>+#define SDXC_RESP_CRC_ERROR BIT(6)
> >>+#define SDXC_DATA_CRC_ERROR BIT(7)
> >>+#define SDXC_RESP_TIMEOUT BIT(8)
> >>+#define SDXC_DATA_TIMEOUT BIT(9)
> >>+#define SDXC_VOLTAGE_CHANGE_DONE BIT(10)
> >>+#define SDXC_FIFO_RUN_ERROR BIT(11)
> >>+#define SDXC_HARD_WARE_LOCKED BIT(12)
> >>+#define SDXC_START_BIT_ERROR BIT(13)
> >>+#define SDXC_AUTO_COMMAND_DONE BIT(14)
> >>+#define SDXC_END_BIT_ERROR BIT(15)
> >>+#define SDXC_SDIO_INTERRUPT BIT(16)
> >>+#define SDXC_CARD_INSERT BIT(30)
> >>+#define SDXC_CARD_REMOVE BIT(31)
> >>+#define SDXC_INTERRUPT_ERROR_BIT (SDXC_RESP_ERROR | SDXC_RESP_CRC_ERROR | \
> >>+ SDXC_DATA_CRC_ERROR | SDXC_RESP_TIMEOUT | \
> >>+ SDXC_DATA_TIMEOUT | SDXC_FIFO_RUN_ERROR | \
> >>+ SDXC_HARD_WARE_LOCKED | SDXC_START_BIT_ERROR | \
> >>+ SDXC_END_BIT_ERROR) /* 0xbbc2 */
> >>+#define SDXC_INTERRUPT_DONE_BIT (SDXC_AUTO_COMMAND_DONE | SDXC_DATA_OVER | \
> >>+ SDXC_COMMAND_DONE | SDXC_VOLTAGE_CHANGE_DONE)
> >>+/* status */
> >>+#define SDXC_RXWL_FLAG BIT(0)
> >>+#define SDXC_TXWL_FLAG BIT(1)
> >>+#define SDXC_FIFO_EMPTY BIT(2)
> >>+#define SDXC_FIFO_FULL BIT(3)
> >>+#define SDXC_CARD_PRESENT BIT(8)
> >>+#define SDXC_CARD_DATA_BUSY BIT(9)
> >>+#define SDXC_DATA_FSM_BUSY BIT(10)
> >>+#define SDXC_DMA_REQUEST BIT(31)
> >>+#define SDXC_FIFO_SIZE (16)
> >>+/* Function select */
> >>+#define SDXC_CEATA_ON (0xceaaU << 16)
> >>+#define SDXC_SEND_IRQ_RESPONSE BIT(0)
> >>+#define SDXC_SDIO_READ_WAIT BIT(1)
> >>+#define SDXC_ABORT_READ_DATA BIT(2)
> >>+#define SDXC_SEND_CCSD BIT(8)
> >>+#define SDXC_SEND_AUTO_STOPCCSD BIT(9)
> >>+#define SDXC_CEATA_DEV_INTERRUPT_ENABLE_BIT BIT(10)
> >>+/* IDMA controller bus mod bit field */
> >>+#define SDXC_IDMAC_SOFT_RESET BIT(0)
> >>+#define SDXC_IDMAC_FIX_BURST BIT(1)
> >>+#define SDXC_IDMAC_IDMA_ON BIT(7)
> >>+#define SDXC_IDMAC_REFETCH_DES BIT(31)
> >>+/* IDMA status bit field */
> >>+#define SDXC_IDMAC_TRANSMIT_INTERRUPT BIT(0)
> >>+#define SDXC_IDMAC_RECEIVE_INTERRUPT BIT(1)
> >>+#define SDXC_IDMAC_FATAL_BUS_ERROR BIT(2)
> >>+#define SDXC_IDMAC_DESTINATION_INVALID BIT(4)
> >>+#define SDXC_IDMAC_CARD_ERROR_SUM BIT(5)
> >>+#define SDXC_IDMAC_NORMAL_INTERRUPT_SUM BIT(8)
> >>+#define SDXC_IDMAC_ABNORMAL_INTERRUPT_SUM BIT(9)
> >>+#define SDXC_IDMAC_HOST_ABORT_INTERRUPT_TX BIT(10)
> >>+#define SDXC_IDMAC_HOST_ABORT_INTERRUPT_RX BIT(10)
> >>+#define SDXC_IDMAC_IDLE (0U << 13)
> >
> >Ditto
> >
> >>+#define SDXC_IDMAC_SUSPEND (1U << 13)
> >>+#define SDXC_IDMAC_DESC_READ (2U << 13)
> >>+#define SDXC_IDMAC_DESC_CHECK (3U << 13)
> >>+#define SDXC_IDMAC_READ_REQUEST_WAIT (4U << 13)
> >>+#define SDXC_IDMAC_WRITE_REQUEST_WAIT (5U << 13)
> >>+#define SDXC_IDMAC_READ (6U << 13)
> >>+#define SDXC_IDMAC_WRITE (7U << 13)
> >>+#define SDXC_IDMAC_DESC_CLOSE (8U << 13)
> >
> >Please use BIT as much as possible here.
>
> Erm lets not do that, nor remove the 0 << 13, this are all
> values to store in a multi-bit field which lives in bits 13-xx,
> changing the values which happen to be power of 2 into BIT
> macros is not helpful.

Ok.

Thanks!
Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


Attachments:
(No filename) (17.85 kB)
signature.asc (836.00 B)
Digital signature
Download all attachments

2014-02-19 12:16:13

by Hans de Goede

[permalink] [raw]
Subject: Re: [PATCH v7 4/8] ARM: sunxi: Add driver for SD/MMC hosts found on Allwinner sunxi SoCs

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

On 02/19/2014 10:46 AM, Maxime Ripard wrote:
> Hi Hans,
>
> On Tue, Feb 18, 2014 at 09:49:21PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 02/18/2014 04:37 PM, Maxime Ripard wrote:
>>
>> <snip>
>>
>>>> + + for (i = 0; i < data->sg_len; i++) { + pdes[i].config = SDXC_IDMAC_DES0_CH | SDXC_IDMAC_DES0_OWN | + SDXC_IDMAC_DES0_DIC; + + if (data->sg[i].length == max_len) + pdes[i].buf_size = 0; /* 0 == max_len */ + else + pdes[i].buf_size = data->sg[i].length; + + pdes[i].buf_addr_ptr1 = sg_dma_address(&data->sg[i]); + pdes[i].buf_addr_ptr2 = (u32)&pdes_pa[i + 1]; + } + + pdes[0].config |= SDXC_IDMAC_DES0_FD; + pdes[i - 1].config = SDXC_IDMAC_DES0_OWN | SDXC_IDMAC_DES0_LD; + + wmb(); /* Ensure idma_des hit main mem before we start the idmac */
>>>
>>> wmb ensure the proper ordering of the instructions, not flushing the caches like what your comment implies.
>>
>> Since I put that comment there, allow me to explain. A modern ARM cpu core has 2 or more units handling stores. One for regular memory stores, and one for io-mem stores. Regular mem stores can be re-ordered, io stores cannot. Normally there is no "syncing" between the 2 store units. Cache flushing is not an issue here since the memory holding the descriptors for the idma controller is allocated cache coherent, which on arm means it is not cached.
>>
>> What is an issue here is the io-store starting the idmac hitting the io-mem before the descriptors hit the main-mem, the wmb() ensures this does not happen.
>
> To expand a bit, my point was not that it was functionnally wrong. Since you put a barrier in there, and that it resides in a cache coherent section, we're fine.
>
> My point was that the comment itself was misleading.

Well as explained above, the purpose of the wmb is to ensure that
the descriptors hit main memory, before the following writel (in the
caller of this function) starts the controller. So I don't see exactly
how the comment is wrong.

If you've a better wording for the comment, suggestions are welcome.

<snip>

>>>> +static void sunxi_mmc_dump_errinfo(struct sunxi_mmc_host *smc_host) +{ + struct mmc_command *cmd = smc_host->mrq->cmd; + struct mmc_data *data = smc_host->mrq->data; + + /* For some cmds timeout is normal with sd/mmc cards */ + if ((smc_host->int_sum & SDXC_INTERRUPT_ERROR_BIT) == SDXC_RESP_TIMEOUT && + (cmd->opcode == SD_IO_SEND_OP_COND || cmd->opcode == SD_IO_RW_DIRECT)) + return; + + dev_err(mmc_dev(smc_host->mmc),
>>>
>>> I'd rather put it at a debug loglevel.
>>
>> Erm, this only happens if something is seriously wrong.
>
> Still. Something would be seriously wrong in the MMC driver/controller. You don't want to bloat the whole kernel logs with the dump of your registers just because the MMC is failing. This is of no interest to anyone but someone that would actually try to debug what's wrong.

This is not a complete register dump, this writes a single line to the
kernel log saying that an io error happened, and printing the error flags
set in the status register. We cannot be much shorter then this without
simply not notifying the user that an io error has happened, and not
notifying the user is wrong IMHO.

>
>>>> + /* Make sure the controller is in a sane state before enabling irqs */ + ret = sunxi_mmc_init_host(host->mmc); + if (ret) + return ret; + + host->irq = platform_get_irq(pdev, 0); + ret = devm_request_irq(&pdev->dev, host->irq, sunxi_mmc_irq, 0, + "sunxi-mmc", host); + if (ret == 0) + disable_irq(host->irq);
>>>
>>> The disable_irq is useless here. Just exit.
>>
>> No it is not note the ret == 0, this is not an error handling path!
>
> Doh... Right. My bad.
>
>> This is done under an if because we want to do the sunxi_mmc_exit_host regardless of the request_irq succeeding or not.
>>
>>>
>>>> + + /* And put it back in reset */ + sunxi_mmc_exit_host(host);
>>>
>>> Hu? If it's in reset, how can it generate some IRQs?
>>
>> Yes, that is why we do the whole dance of init controller, get irq, disable irq, drop it back in reset (until the mmc subsys does a power on of the mmc card / sdio dev).
>>
>> Sometime the controller asserts the irq in reset for some reason, so without the dance as soon as we do the devm_request_irq we get an irq, and worse, not only do we get an irq, we cannot clear it since writing to the interrupt status register does not work when the controller is in reset, so we get stuck re-entering the irq handler.
>
> Hmmm, I see. It probably deserves some commenting here too then.

This call is the mirror of the sunxi_mmc_init_host a few lines higher, which
has this comment:

/* Make sure the controller is in a sane state before enabling irqs */

Which attempts to explain why we do the init controller, claim irq,
disable irq, put controller back in reset sequence. Again suggestions
for a better comment are welcome.

<snip>

>>>> + ret = mmc_add_host(mmc); + + if (ret) + goto error_free_dma; + + dev_info(&pdev->dev, "base:0x%p irq:%u\n", host->reg_base, host->irq); + platform_set_drvdata(pdev, mmc);
>>>
>>> This should be before the registration. Otherwise, you're racy.
>>
>> Nope, we only need this to get the data on sunxi_mmc_remove, everywhere else the data is found through the mmc-host struct.
>
> Still, if anyone makes a following patch using the platform_device for some reason, we will have a race condition, without any way to notice it.
>
> Plus, you're doing all the other bits of initialization of your structures much earlier, why not be consistent and having all of them at the same place?

Most platform drivers I've worked on do platform_set_drvdata as late as possible,
so that the drvdata does not get set and never cleared in error paths.

This is just following that pattern.

Regards,

Hans
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlMEoDYACgkQF3VEtJrzE/uAggCdGpdcz1Nli6JREPcUcGww7wXk
r4QAoIZfwy2e5GkHjQvrdpz0aT62x9/W
=y0wO
-----END PGP SIGNATURE-----

2014-02-21 18:55:09

by [email protected]

[permalink] [raw]
Subject: Re: [PATCH v7 5/8] ARM: dts: sun7i: Add support for mmc

On Tue, Feb 18, 2014 at 04:10:38PM +0100, Hans de Goede wrote:
> Hi,
>
> On 02/18/2014 03:22 PM, Maxime Ripard wrote:
> >On Mon, Feb 17, 2014 at 11:02:41AM +0100, David Lanzend?rfer wrote:
> >>Signed-off-by: David Lanzend?rfer <[email protected]>
> >>Signed-off-by: Hans de Goede <[email protected]om>
> >>---
> >> arch/arm/boot/dts/sun7i-a20-cubieboard2.dts | 8 +++
> >> arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 8 +++
> >> arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts | 23 +++++++++
> >> arch/arm/boot/dts/sun7i-a20.dtsi | 61 +++++++++++++++++++++++
> >> 4 files changed, 100 insertions(+)
> >>
> >
> >I'd prefer to have three patches here:
> > - One that add the controllers
> > - One that add the pin muxing options
> > - One that enable the controllers on the various boards.
> >
> >>diff --git a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
> >>index 5c51cb8..ae800b6 100644
> >>--- a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
> >>+++ b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
> >>@@ -34,6 +34,14 @@
> >> };
> >> };
> >>
> >>+ mmc0: [email protected] {
> >>+ pinctrl-names = "default", "default";
> >>+ pinctrl-0 = <&mmc0_pins_a>;
> >>+ pinctrl-1 = <&mmc0_cd_pin_reference_design>;
> >
> >This can be made a single pinctrl group, you don't need the pinctrl-1
> >stuff, it only complicates the node.
>
> Then how do we deal with boards which use a different gpio for card-detect ?
>
> In that case we don't want to change the mux setting of the reference
> design cd pin. IOW I believe that having 2 separate pinctrl settings for
> this is the rigt thing todo. I would prefer using just mmc0_cd_pin_a instead
> of _reference_design though.
>
> Oh wait, you're probably talking about using:
> pinctrl-0 = <&mmc0_pins_a>, <&mmc0_cd_pin_reference_design>;
>
> Yes that would be better.

Yep, exactly :)

> >
> >>+ cd-gpios = <&pio 7 1 0>; /* PH1 */
> >>+ status = "okay";
> >>+ };
> >>+
> >> [email protected] {
> >> led_pins_cubieboard2: [email protected] {
> >> allwinner,pins = "PH20", "PH21";
> >>diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
> >>index f9dcb61..370cef84 100644
> >>--- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
> >>+++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
> >>@@ -19,6 +19,14 @@
> >> compatible = "cubietech,cubietruck", "allwinner,sun7i-a20";
> >>
> >> [email protected] {
> >>+ mmc0: [email protected] {
> >>+ pinctrl-names = "default", "default";
> >>+ pinctrl-0 = <&mmc0_pins_a>;
> >>+ pinctrl-1 = <&mmc0_cd_pin_reference_design>;
> >>+ cd-gpios = <&pio 7 1 0>; /* PH1 */
> >>+ status = "okay";
> >>+ };
> >>+
> >> [email protected] {
> >> led_pins_cubietruck: [email protected] {
> >> allwinner,pins = "PH7", "PH11", "PH20", "PH21";
> >>diff --git a/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts b/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
> >>index ead3013..685ec06 100644
> >>--- a/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
> >>+++ b/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
> >>@@ -34,7 +34,30 @@
> >> };
> >> };
> >>
> >>+ mmc0: [email protected] {
> >>+ pinctrl-names = "default", "default";
> >>+ pinctrl-0 = <&mmc0_pins_a>;
> >>+ pinctrl-1 = <&mmc0_cd_pin_reference_design>;
> >>+ cd-gpios = <&pio 7 1 0>; /* PH1 */
> >>+ status = "okay";
> >>+ };
> >>+
> >>+ mmc3: [email protected] {
> >>+ pinctrl-names = "default", "default";
> >>+ pinctrl-0 = <&mmc3_pins_a>;
> >>+ pinctrl-1 = <&mmc3_cd_pin_olinuxinom>;
> >>+ cd-gpios = <&pio 7 11 0>; /* PH11 */
> >>+ status = "okay";
> >>+ };
> >>+
> >> [email protected] {
> >>+ mmc3_cd_pin_olinuxinom: [email protected] {
> >>+ allwinner,pins = "PH11";
> >>+ allwinner,function = "gpio_in";
> >>+ allwinner,drive = <0>;
> >>+ allwinner,pull = <1>;
> >>+ };
> >>+
> >> led_pins_olinuxino: [email protected] {
> >> allwinner,pins = "PH2";
> >> allwinner,function = "gpio_out";
> >>diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
> >>index 9ff0948..5b55414 100644
> >>--- a/arch/arm/boot/dts/sun7i-a20.dtsi
> >>+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
> >>@@ -355,6 +355,46 @@
> >> #size-cells = <0>;
> >> };
> >>
> >>+ mmc0: [email protected] {
> >>+ 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>;
> >
> >This belongs to the board, the controller itself is able to handle
> >several bus width.
>
> I believe that providing some form of default in the dtsi makes sense
> here and all boards we've seen sofar always use 4 bits, we can always
> override this from the dts file itself.

There's already a documented default that is 1. Plus, I still believe
in a strict separation between what's in the SoC being in the DTSI and
what's in the board being in the DTS.

Apart from being more rigorous, it also has the advantage of being
*much* clearer whenever your read a board DTS, since you don't have to
actually open several DTS(I) to get what's going on.

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


Attachments:
(No filename) (5.20 kB)
signature.asc (836.00 B)
Digital signature
Download all attachments

2014-02-22 07:33:59

by David Lanzendörfer

[permalink] [raw]
Subject: Re: [PATCH v7 8/8] ARM: sunxi: Add documentation for driver for SD/MMC hosts found on Allwinner sunxi SoCs

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


Attachments:
signature.asc (198.00 B)
This is a digitally signed message part.

2014-02-22 08:35:09

by [email protected]

[permalink] [raw]
Subject: Re: [PATCH v7 4/8] ARM: sunxi: Add driver for SD/MMC hosts found on Allwinner sunxi SoCs

Hi Hans,

(As a side note, your mailer just did something nasty with the
wrapping which made the code snippets totally unreadable. I'm going to
drop them.)

On Wed, Feb 19, 2014 at 01:14:58PM +0100, Hans de Goede wrote:
> >>>> + wmb(); /* Ensure idma_des hit main mem before we start the idmac */
> >>>
> >>> wmb ensure the proper ordering of the instructions, not flushing
> >>> the caches like what your comment implies.
> >>
> >> Since I put that comment there, allow me to explain. A modern ARM
> >> cpu core has 2 or more units handling stores. One for regular
> >> memory stores, and one for io-mem stores. Regular mem stores can
> >> be re-ordered, io stores cannot. Normally there is no "syncing"
> >> between the 2 store units. Cache flushing is not an issue here
> >> since the memory holding the descriptors for the idma controller
> >> is allocated cache coherent, which on arm means it is not cached.
> >>
> >> What is an issue here is the io-store starting the idmac hitting
> >> the io-mem before the descriptors hit the main-mem, the wmb()
> >> ensures this does not happen.
> >
> > To expand a bit, my point was not that it was functionnally
> > wrong. Since you put a barrier in there, and that it resides in a
> > cache coherent section, we're fine.
> >
> > My point was that the comment itself was misleading.
>
> Well as explained above, the purpose of the wmb is to ensure that
> the descriptors hit main memory, before the following writel (in the
> caller of this function) starts the controller. So I don't see
> exactly how the comment is wrong.
>
> If you've a better wording for the comment, suggestions are welcome.

Your first reply was great :)

But if you feel like it enough, fine.

[ codeless snip..]

> >>> I'd rather put it at a debug loglevel.
> >>
> >> Erm, this only happens if something is seriously wrong.
> >
> > Still. Something would be seriously wrong in the MMC
> > driver/controller. You don't want to bloat the whole kernel logs
> > with the dump of your registers just because the MMC is
> > failing. This is of no interest to anyone but someone that would
> > actually try to debug what's wrong.
>
> This is not a complete register dump, this writes a single line to
> the kernel log saying that an io error happened, and printing the
> error flags set in the status register. We cannot be much shorter
> then this without simply not notifying the user that an io error has
> happened, and not notifying the user is wrong IMHO.

Ok.

> >>>> + /* And put it back in reset */
> >>>> + sunxi_mmc_exit_host(host);
> >>>
> >>> Hu? If it's in reset, how can it generate some IRQs?
> >>
> >> Yes, that is why we do the whole dance of init controller, get
> >> irq, disable irq, drop it back in reset (until the mmc subsys
> >> does a power on of the mmc card / sdio dev).
> >>
> >> Sometime the controller asserts the irq in reset for some reason,
> >> so without the dance as soon as we do the devm_request_irq we get
> >> an irq, and worse, not only do we get an irq, we cannot clear it
> >> since writing to the interrupt status register does not work when
> >> the controller is in reset, so we get stuck re-entering the irq
> >> handler.
> >
> > Hmmm, I see. It probably deserves some commenting here too then.
>
> This call is the mirror of the sunxi_mmc_init_host a few lines
> higher, which has this comment:
>
> /* Make sure the controller is in a sane state before enabling irqs */
>
> Which attempts to explain why we do the init controller, claim irq,
> disable irq, put controller back in reset sequence. Again suggestions
> for a better comment are welcome.

And again, the second part of your first reply was great :)

> >>>> + ret = mmc_add_host(mmc);
> >>>> + if (ret)
> >>>> + goto error_free_dma;
> >>>> +
> >>>> + dev_info(&pdev->dev, "base:0x%p irq:%u\n", host->reg_base, host->irq);
> >>>> + platform_set_drvdata(pdev, mmc);
> >>>
> >>> This should be before the registration. Otherwise, you're racy.
> >>
> >> Nope, we only need this to get the data on sunxi_mmc_remove,
> >> everywhere else the data is found through the mmc-host struct.
> >
> > Still, if anyone makes a following patch using the platform_device
> > for some reason, we will have a race condition, without any way to
> > notice it.
> >
> > Plus, you're doing all the other bits of initialization of your
> > structures much earlier, why not be consistent and having all of
> > them at the same place?
>
> Most platform drivers I've worked on do platform_set_drvdata as late
> as possible, so that the drvdata does not get set and never cleared
> in error paths.

You don't actually have to clear it, and some frameworks actually
require you to call dev_set_drvdata before registration, so that
statement looks quite odd to me.

Thanks!
Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


Attachments:
(No filename) (4.79 kB)
signature.asc (836.00 B)
Digital signature
Download all attachments

2014-02-22 08:40:06

by [email protected]

[permalink] [raw]
Subject: Re: [PATCH v7 8/8] ARM: sunxi: Add documentation for driver for SD/MMC hosts found on Allwinner sunxi SoCs

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

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


Attachments:
(No filename) (1.14 kB)
signature.asc (836.00 B)
Digital signature
Download all attachments

2014-02-22 11:20:00

by Hans de Goede

[permalink] [raw]
Subject: Re: [PATCH v7 4/8] ARM: sunxi: Add driver for SD/MMC hosts found on Allwinner sunxi SoCs

Hi,

On 02/22/2014 09:31 AM, Maxime Ripard wrote:

<snip>

>>>>> This should be before the registration. Otherwise, you're racy.
>>>>
>>>> Nope, we only need this to get the data on sunxi_mmc_remove,
>>>> everywhere else the data is found through the mmc-host struct.
>>>
>>> Still, if anyone makes a following patch using the platform_device
>>> for some reason, we will have a race condition, without any way to
>>> notice it.
>>>
>>> Plus, you're doing all the other bits of initialization of your
>>> structures much earlier, why not be consistent and having all of
>>> them at the same place?
>>
>> Most platform drivers I've worked on do platform_set_drvdata as late
>> as possible, so that the drvdata does not get set and never cleared
>> in error paths.
>
> You don't actually have to clear it,

Erm, yes and no, you used to have to manually clear it, so as to not
leave a dangling pointer in there, which may confuse things later.

But since a few kernel releases, the driver core will explicitly
clear it on probe failure, since many many drivers got this wrong.
I know this because I wrote the driver core patch doing the clearing :)

So in drivers which used to get it right, you will see the
platform_set_drvdata call quite late and it being so late
in the sunxi-mmc.c driver is old habits dying hard.

But you're right that this is no longer needed, still I see little
reason to move it up, but if you really want it to be moved up,
I'm fine with that.

> and some frameworks actually require you to call dev_set_drvdata before registration, so that
> statement looks quite odd to me.

And the proper thing to do when using those frameworks was to
manually clear drvdata again on probe failure. Anyways this is
all handled in the driver core now, so this whole discussion
is moot anyways. I was merely trying to explain where my
preference for doing the dev_set_drvdata call as late as
possible comes from.

Regards,

Hans