[PATCH 0/5] arm64: sunxi: A64: enable MMC support

[PATCH 0/5] arm64: sunxi: A64: enable MMC support

[Andre Przywara]

So far the Allwinner A64/Pine64 DT was missing MMC support, because we
observed issues with that. Those have now been fixed (patch 1 and 2),
so we can enable the MMC IP block in the SoC .dtsi and the Pine64 .dts.
As this gives access to the SD card (as the only mass storage device on
most boards), this makes the kernel support actually useful.
The A64 MMC controller has more up its sleeves, but for now this level
of support is good enough.
Thanks a lot to Maxime for investigating the eMMC failure and coming up
with a nice fix for that.
Maxime: I picked that patch from some pastebin drop of yours, please holler
if there's something wrong with that (patch 2/5).

I send the BananaPi M64 .dts patch along with that series, as the eMMC on
that board now makes some difference.

Cheers,
Andre.

Andre Przywara (4):
  drivers: mmc: sunxi: fix A64 calibration routine
  arm64: dts: sun50i: add MMC nodes
  arm64: dts: Pine64: add MMC support
  arm64: dts: add BananaPi-M64 support

Maxime Ripard (1):
  drivers: mmc: sunxi: limit A64 MMC2 to 8K DMA buffer

 .../devicetree/bindings/mmc/sunxi-mmc.txt          |   1 +
 arch/arm64/boot/dts/allwinner/Makefile             |   1 +
 .../boot/dts/allwinner/sun50i-a64-bananapi-m64.dts | 125 +++++++++++++++++++++
 .../arm64/boot/dts/allwinner/sun50i-a64-pine64.dts |  18 +++
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi      |  77 +++++++++++++
 drivers/mmc/host/sunxi-mmc.c                       |  37 ++++--
 6 files changed, 247 insertions(+), 12 deletions(-)
 create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts

-- 
2.8.2

[PATCH 1/5] drivers: mmc: sunxi: fix A64 calibration routine

[Andre Przywara]

The calibration facility in the A64 MMC block seems to have been
misunderstood: the result value is not the value to program into the
delay bits, but is the number of delay cells that result in a full clock
cycle delay. So this value has to be scaled by the desired phase, which
we still have to know and program.
Change the calibration routine to take a phase parameter and scale the
calibration value accordingly.
Also introduce sun50i-a64 delay parameters to store the required phase.
Looking at the BSP kernel the sample delay for anything below HS200 is
0, so we go with that value.
Once the driver supports HS200 and faster modes, we can enter confirmed
working values in there.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/mmc/host/sunxi-mmc.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index b1d1303..1e156e8 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -681,15 +681,13 @@ static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32 oclk_en)
 	return 0;
 }
 
-static int sunxi_mmc_calibrate(struct sunxi_mmc_host *host, int reg_off)
+static int sunxi_mmc_calibrate(struct sunxi_mmc_host *host, int reg_off,
+			       int degrees)
 {
 	u32 reg = readl(host->reg_base + reg_off);
 	u32 delay;
 	unsigned long timeout;
 
-	if (!host->cfg->can_calibrate)
-		return 0;
-
 	reg &= ~(SDXC_CAL_DL_MASK << SDXC_CAL_DL_SW_SHIFT);
 	reg &= ~SDXC_CAL_DL_SW_EN;
 
@@ -711,6 +709,7 @@ static int sunxi_mmc_calibrate(struct sunxi_mmc_host *host, int reg_off)
 	}
 
 	delay = (reg >> SDXC_CAL_DL_SHIFT) & SDXC_CAL_DL_MASK;
+	delay = degrees * delay / 360;
 
 	reg &= ~SDXC_CAL_START;
 	reg |= (delay << SDXC_CAL_DL_SW_SHIFT) | SDXC_CAL_DL_SW_EN;
@@ -748,6 +747,11 @@ static int sunxi_mmc_clk_set_phase(struct sunxi_mmc_host *host,
 		return -EINVAL;
 	}
 
+	if (host->cfg->can_calibrate)
+		return sunxi_mmc_calibrate(host, SDXC_REG_SAMP_DL_REG,
+					   host->cfg->clk_delays[index].sample);
+	/* TODO: calibrate data strobe delay once HS-400 is supported. */
+
 	clk_set_phase(host->clk_sample, host->cfg->clk_delays[index].sample);
 	clk_set_phase(host->clk_output, host->cfg->clk_delays[index].output);
 
@@ -802,12 +806,6 @@ static int sunxi_mmc_clk_set_rate(struct sunxi_mmc_host *host,
 	if (ret)
 		return ret;
 
-	ret = sunxi_mmc_calibrate(host, SDXC_REG_SAMP_DL_REG);
-	if (ret)
-		return ret;
-
-	/* TODO: enable calibrate on sdc2 SDXC_REG_DS_DL_REG of A64 */
-
 	return sunxi_mmc_oclk_onoff(host, 1);
 }
 
@@ -1061,6 +1059,14 @@ static const struct sunxi_mmc_clk_delay sun9i_mmc_clk_delays[] = {
 	[SDXC_CLK_50M_DDR_8BIT]	= { .output =  72, .sample =  72 },
 };
 
+static const struct sunxi_mmc_clk_delay sun50i_mmc_clk_delays[] = {
+	[SDXC_CLK_400K]		= { .output = 90, .sample = 0 },
+	[SDXC_CLK_25M]		= { .output = 90, .sample = 0 },
+	[SDXC_CLK_50M]		= { .output = 90, .sample = 0 },
+	[SDXC_CLK_50M_DDR]	= { .output = 90, .sample = 0 },
+	[SDXC_CLK_50M_DDR_8BIT]	= { .output = 90, .sample = 0 },
+};
+
 static const struct sunxi_mmc_cfg sun4i_a10_cfg = {
 	.idma_des_size_bits = 13,
 	.clk_delays = NULL,
@@ -1087,7 +1093,7 @@ static const struct sunxi_mmc_cfg sun9i_a80_cfg = {
 
 static const struct sunxi_mmc_cfg sun50i_a64_cfg = {
 	.idma_des_size_bits = 16,
-	.clk_delays = NULL,
+	.clk_delays = sun50i_mmc_clk_delays,
 	.can_calibrate = true,
 };
 
@@ -1134,7 +1140,7 @@ static int sunxi_mmc_resource_request(struct sunxi_mmc_host *host,
 		return PTR_ERR(host->clk_mmc);
 	}
 
-	if (host->cfg->clk_delays) {
+	if (host->cfg->clk_delays && !host->cfg->can_calibrate) {
 		host->clk_output = devm_clk_get(&pdev->dev, "output");
 		if (IS_ERR(host->clk_output)) {
 			dev_err(&pdev->dev, "Could not get output clock\n");
-- 
2.8.2

[PATCH 2/5] drivers: mmc: sunxi: limit A64 MMC2 to 8K DMA buffer

[Andre Przywara]

From: Maxime Ripard <maxime.ripard@free-electrons.com>

Unlike the A64 user manual reports, the third MMC controller on the
A64 (and the only one capable of 8-bit HS400 eMMC transfers) has a
DMA buffer size limit of 8KB (much like the very old Allwinner SoCs).
This does not affect the other two controllers, so introduce a new
DT compatible string to let the driver use different settings for that
particular device. This will also help to enable the high-speed transfer
modes of that controller later.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 Documentation/devicetree/bindings/mmc/sunxi-mmc.txt | 1 +
 drivers/mmc/host/sunxi-mmc.c                        | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt b/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
index 55cdd80..3d9170f 100644
--- a/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
+++ b/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
@@ -14,6 +14,7 @@ Required properties:
    * "allwinner,sun7i-a20-mmc"
    * "allwinner,sun9i-a80-mmc"
    * "allwinner,sun50i-a64-mmc"
+   * "allwinner,sun50i-a64-emmc"
  - reg : mmc controller base registers
  - clocks : a list with 4 phandle + clock specifier pairs
  - clock-names : must contain "ahb", "mmc", "output" and "sample"
diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index 1e156e8..165486bc 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -1097,12 +1097,19 @@ static const struct sunxi_mmc_cfg sun50i_a64_cfg = {
 	.can_calibrate = true,
 };
 
+static const struct sunxi_mmc_cfg sun50i_a64_emmc_cfg = {
+	.idma_des_size_bits = 13,
+	.clk_delays = sun50i_mmc_clk_delays,
+	.can_calibrate = true,
+};
+
 static const struct of_device_id sunxi_mmc_of_match[] = {
 	{ .compatible = "allwinner,sun4i-a10-mmc", .data = &sun4i_a10_cfg },
 	{ .compatible = "allwinner,sun5i-a13-mmc", .data = &sun5i_a13_cfg },
 	{ .compatible = "allwinner,sun7i-a20-mmc", .data = &sun7i_a20_cfg },
 	{ .compatible = "allwinner,sun9i-a80-mmc", .data = &sun9i_a80_cfg },
 	{ .compatible = "allwinner,sun50i-a64-mmc", .data = &sun50i_a64_cfg },
+	{ .compatible = "allwinner,sun50i-a64-emmc", .data = &sun50i_a64_emmc_cfg },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, sunxi_mmc_of_match);
-- 
2.8.2

[PATCH 3/5] arm64: dts: sun50i: add MMC nodes

[Andre Przywara]

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 67 +++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index e0dcab8..c680566 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -150,6 +150,32 @@
 				pins = "PB8", "PB9";
 				function = "uart0";
 			};
+
+			mmc0_pins: mmc0@0 {
+				pins = "PF0", "PF1", "PF2", "PF3", "PF4", "PF5";
+				function = "mmc0";
+				drive-strength = <30>;
+			};
+
+			mmc0_default_cd_pin: mmc0_cd_pin@0 {
+				pins = "PF6";
+				function = "gpio_in";
+				bias-pull-up;
+			};
+
+			mmc1_pins: mmc1@0 {
+				pins = "PG0", "PG1", "PG2", "PG3", "PG4", "PG5";
+				function = "mmc1";
+				drive-strength = <30>;
+			};
+
+			mmc2_pins: mmc2@0 {
+				pins = "PC1", "PC5", "PC6", "PC8", "PC9",
+				       "PC10", "PC11", "PC12", "PC13", "PC14",
+				       "PC15", "PC16";
+				function = "mmc2";
+				drive-strength = <30>;
+			};
 		};
 
 		uart0: serial@1c28000 {
@@ -240,6 +266,47 @@
 			#size-cells = <0>;
 		};
 
+		mmc0: mmc@1c0f000 {
+			compatible = "allwinner,sun50i-a64-mmc",
+				     "allwinner,sun5i-a13-mmc";
+			reg = <0x01c0f000 0x1000>;
+			clocks = <&ccu 31>, <&ccu 75>;
+			clock-names = "ahb", "mmc";
+			resets = <&ccu 8>;
+			reset-names = "ahb";
+			interrupts = <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>;
+			status = "disabled";
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+
+		mmc1: mmc@1c10000 {
+			compatible = "allwinner,sun50i-a64-mmc",
+				     "allwinner,sun5i-a13-mmc";
+			reg = <0x01c10000 0x1000>;
+			clocks = <&ccu 32>, <&ccu 76>;
+			clock-names = "ahb", "mmc";
+			resets = <&ccu 9>;
+			reset-names = "ahb";
+			interrupts = <GIC_SPI 61 IRQ_TYPE_LEVEL_HIGH>;
+			status = "disabled";
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+
+		mmc2: mmc@1c11000 {
+			compatible = "allwinner,sun50i-a64-emmc";
+			reg = <0x01c11000 0x1000>;
+			clocks = <&ccu 33>, <&ccu 77>;
+			clock-names = "ahb", "mmc";
+			resets = <&ccu 10>;
+			reset-names = "ahb";
+			interrupts = <GIC_SPI 62 IRQ_TYPE_LEVEL_HIGH>;
+			status = "disabled";
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+
 		gic: interrupt-controller@1c81000 {
 			compatible = "arm,gic-400";
 			reg = <0x01c81000 0x1000>,
-- 
2.8.2

[PATCH 4/5] arm64: dts: Pine64: add MMC support

[Andre Przywara]

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
index 4709590..c18ab03 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
@@ -55,6 +55,13 @@
 	chosen {
 		stdout-path = "serial0:115200n8";
 	};
+
+	reg_vcc3v3: vcc3v3 {
+		compatible = "regulator-fixed";
+		regulator-name = "vcc3v3";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+	};
 };
 
 &uart0 {
@@ -72,3 +79,14 @@
 &i2c1_pins {
 	bias-pull-up;
 };
+
+&mmc0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&mmc0_pins>, <&mmc0_default_cd_pin>;
+	vmmc-supply = <&reg_vcc3v3>;
+	cd-gpios = <&pio 5 6 0>;
+	cd-inverted;
+	disable-wp;
+	bus-width = <4>;
+	status = "okay";
+};
-- 
2.8.2

[PATCH 5/5] arm64: dts: add BananaPi-M64 support

[Andre Przywara]

The Banana Pi M64 board is a typical single board computer based on the
Allwinner A64 SoC. Aside from the usual peripherals it features eMMC
storage, which is connected to the 8-bit capable SDHC2 controller.
Also it has a soldered WiFi/Bluetooth chip, so we enable UART1 and SDHC1
as those two interfaces are connected to it.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm64/boot/dts/allwinner/Makefile             |   1 +
 .../boot/dts/allwinner/sun50i-a64-bananapi-m64.dts | 125 +++++++++++++++++++++
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi      |  10 ++
 3 files changed, 136 insertions(+)
 create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts

diff --git a/arch/arm64/boot/dts/allwinner/Makefile b/arch/arm64/boot/dts/allwinner/Makefile
index 1e29a5a..51ecb04 100644
--- a/arch/arm64/boot/dts/allwinner/Makefile
+++ b/arch/arm64/boot/dts/allwinner/Makefile
@@ -1,4 +1,5 @@
 dtb-$(CONFIG_ARCH_SUNXI) += sun50i-a64-pine64-plus.dtb sun50i-a64-pine64.dtb
+dtb-$(CONFIG_ARCH_SUNXI) += sun50i-a64-bananapi-m64.dtb
 
 always		:= $(dtb-y)
 subdir-y	:= $(dts-dirs)
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
new file mode 100644
index 0000000..941ea07
--- /dev/null
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
@@ -0,0 +1,125 @@
+/*
+ * Copyright (c) 2016 ARM Ltd.
+ *
+ * This file is dual-licensed: you can use it either under the terms
+ * of the GPL or the X11 license, at your option. Note that this dual
+ * licensing only applies to this file, and not this project as a
+ * whole.
+ *
+ *  a) This library 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 library 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.
+ *
+ * Or, alternatively,
+ *
+ *  b) Permission is hereby granted, free of charge, to any person
+ *     obtaining a copy of this software and associated documentation
+ *     files (the "Software"), to deal in the Software without
+ *     restriction, including without limitation the rights to use,
+ *     copy, modify, merge, publish, distribute, sublicense, and/or
+ *     sell copies of the Software, and to permit persons to whom the
+ *     Software is furnished to do so, subject to the following
+ *     conditions:
+ *
+ *     The above copyright notice and this permission notice shall be
+ *     included in all copies or substantial portions of the Software.
+ *
+ *     THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ *     EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ *     OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ *     NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ *     HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ *     OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+/dts-v1/;
+
+#include "sun50i-a64.dtsi"
+
+/ {
+	model = "BananaPi-M64";
+	compatible = "sinovoip,bananapi-m64", "allwinner,sun50i-a64";
+
+	aliases {
+		serial0 = &uart0;
+		serial1 = &uart1;
+	};
+
+	chosen {
+		stdout-path = "serial0:115200n8";
+	};
+
+	reg_vcc3v3: vcc3v3 {
+		compatible = "regulator-fixed";
+		regulator-name = "vcc3v3";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+	};
+};
+
+&uart0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&uart0_pins_a>;
+	status = "okay";
+};
+
+&uart1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&uart1_pins_4>;
+	status = "okay";
+};
+
+&i2c1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&i2c1_pins>;
+	status = "okay";
+};
+
+&i2c1_pins {
+	bias-pull-up;
+};
+
+&mmc0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&mmc0_pins>, <&mmc0_default_cd_pin>;
+	vmmc-supply = <&reg_vcc3v3>;
+	cd-gpios = <&pio 5 6 0>;
+	cd-inverted;
+	disable-wp;
+	bus-width = <4>;
+	status = "okay";
+};
+
+&mmc1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&mmc1_pins>;
+	vmmc-supply = <&reg_vcc3v3>;
+	bus-width = <4>;
+	non-removable;
+	status = "okay";
+};
+
+&mmc2 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&mmc2_pins>;
+	vmmc-supply = <&reg_vcc3v3>;
+	bus-width = <8>;
+	non-removable;
+	cap-mmc-hw-reset;
+	status = "okay";
+};
+
+&mmc2_pins {
+	/* Increase drive strength for DDR modes */
+	drive-strength = <40>;
+	/* eMMC is missing pull-ups */
+	bias-pull-up;
+};
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index c680566..9de96ba 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -151,6 +151,16 @@
 				function = "uart0";
 			};
 
+			uart1_pins: uart1@0 {
+				pins = "PG6", "PG7";
+				function = "uart1";
+			};
+
+			uart1_pins_4: uart1_4@0 {
+				pins = "PG6", "PG7", "PG8", "PG9";
+				function = "uart1";
+			};
+
 			mmc0_pins: mmc0@0 {
 				pins = "PF0", "PF1", "PF2", "PF3", "PF4", "PF5";
 				function = "mmc0";
-- 
2.8.2

Re: [PATCH 3/5] arm64: dts: sun50i: add MMC nodes

[Chen-Yu Tsai]

Hi,

On Tue, Jan 3, 2017 at 7:03 AM, Andre Przywara <andre.przywara@arm.com> wrote:

A commit message explaining the mmc controllers would be nice.

> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 67 +++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> index e0dcab8..c680566 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> @@ -150,6 +150,32 @@
>                                 pins = "PB8", "PB9";
>                                 function = "uart0";
>                         };
> +
> +                       mmc0_pins: mmc0@0 {
> +                               pins = "PF0", "PF1", "PF2", "PF3", "PF4", "PF5";
> +                               function = "mmc0";
> +                               drive-strength = <30>;
> +                       };
> +
> +                       mmc0_default_cd_pin: mmc0_cd_pin@0 {
> +                               pins = "PF6";
> +                               function = "gpio_in";
> +                               bias-pull-up;
> +                       };

We are starting to drop pinmux nodes for gpio usage.

> +
> +                       mmc1_pins: mmc1@0 {
> +                               pins = "PG0", "PG1", "PG2", "PG3", "PG4", "PG5";
> +                               function = "mmc1";
> +                               drive-strength = <30>;
> +                       };
> +
> +                       mmc2_pins: mmc2@0 {
> +                               pins = "PC1", "PC5", "PC6", "PC8", "PC9",
> +                                      "PC10", "PC11", "PC12", "PC13", "PC14",
> +                                      "PC15", "PC16";
> +                               function = "mmc2";
> +                               drive-strength = <30>;
> +                       };

Moreover I think you should split out the pinmux nodes to a separate patch.

>                 };
>
>                 uart0: serial@1c28000 {
> @@ -240,6 +266,47 @@
>                         #size-cells = <0>;
>                 };
>
> +               mmc0: mmc@1c0f000 {
> +                       compatible = "allwinner,sun50i-a64-mmc",
> +                                    "allwinner,sun5i-a13-mmc";

Given that sun5i doesn't support mmc delay timings, and the A64 has
calibration and delay timings, I wouldn't call them compatible.

Or are you claiming that for the A64 has a delay of 0 for the
currently supported speeds, so the calibration doesn't really
matter? If so this should be mentioned in the commit message.

> +                       reg = <0x01c0f000 0x1000>;
> +                       clocks = <&ccu 31>, <&ccu 75>;

The clock / reset index macros are in the tree now.
Please switch to them.

ChenYu

> +                       clock-names = "ahb", "mmc";
> +                       resets = <&ccu 8>;
> +                       reset-names = "ahb";
> +                       interrupts = <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>;
> +                       status = "disabled";
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +               };
> +
> +               mmc1: mmc@1c10000 {
> +                       compatible = "allwinner,sun50i-a64-mmc",
> +                                    "allwinner,sun5i-a13-mmc";
> +                       reg = <0x01c10000 0x1000>;
> +                       clocks = <&ccu 32>, <&ccu 76>;
> +                       clock-names = "ahb", "mmc";
> +                       resets = <&ccu 9>;
> +                       reset-names = "ahb";
> +                       interrupts = <GIC_SPI 61 IRQ_TYPE_LEVEL_HIGH>;
> +                       status = "disabled";
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +               };
> +
> +               mmc2: mmc@1c11000 {
> +                       compatible = "allwinner,sun50i-a64-emmc";
> +                       reg = <0x01c11000 0x1000>;
> +                       clocks = <&ccu 33>, <&ccu 77>;
> +                       clock-names = "ahb", "mmc";
> +                       resets = <&ccu 10>;
> +                       reset-names = "ahb";
> +                       interrupts = <GIC_SPI 62 IRQ_TYPE_LEVEL_HIGH>;
> +                       status = "disabled";
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +               };
> +
>                 gic: interrupt-controller@1c81000 {
>                         compatible = "arm,gic-400";
>                         reg = <0x01c81000 0x1000>,
> --
> 2.8.2
>

Re: [PATCH 3/5] arm64: dts: sun50i: add MMC nodes

[André Przywara]

On 03/01/17 02:52, Chen-Yu Tsai wrote:

Hi,

> On Tue, Jan 3, 2017 at 7:03 AM, Andre Przywara <andre.przywara@arm.com> wrote:
> 
> A commit message explaining the mmc controllers would be nice.

OK.

>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 67 +++++++++++++++++++++++++++
>>  1 file changed, 67 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>> index e0dcab8..c680566 100644
>> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>> @@ -150,6 +150,32 @@
>>                                 pins = "PB8", "PB9";
>>                                 function = "uart0";
>>                         };
>> +
>> +                       mmc0_pins: mmc0@0 {
>> +                               pins = "PF0", "PF1", "PF2", "PF3", "PF4", "PF5";
>> +                               function = "mmc0";
>> +                               drive-strength = <30>;
>> +                       };
>> +
>> +                       mmc0_default_cd_pin: mmc0_cd_pin@0 {
>> +                               pins = "PF6";
>> +                               function = "gpio_in";
>> +                               bias-pull-up;
>> +                       };
> 
> We are starting to drop pinmux nodes for gpio usage.

And replacing them with what?
Or do you mean they go in the individual board .dts files?
In this case I believe having a default pin defined here would help to
define it in every .dts.

>> +
>> +                       mmc1_pins: mmc1@0 {
>> +                               pins = "PG0", "PG1", "PG2", "PG3", "PG4", "PG5";
>> +                               function = "mmc1";
>> +                               drive-strength = <30>;
>> +                       };
>> +
>> +                       mmc2_pins: mmc2@0 {
>> +                               pins = "PC1", "PC5", "PC6", "PC8", "PC9",
>> +                                      "PC10", "PC11", "PC12", "PC13", "PC14",
>> +                                      "PC15", "PC16";
>> +                               function = "mmc2";
>> +                               drive-strength = <30>;
>> +                       };
> 
> Moreover I think you should split out the pinmux nodes to a separate patch.

I can surely do, just wondering what's the rationale is behind that?

> 
>>                 };
>>
>>                 uart0: serial@1c28000 {
>> @@ -240,6 +266,47 @@
>>                         #size-cells = <0>;
>>                 };
>>
>> +               mmc0: mmc@1c0f000 {
>> +                       compatible = "allwinner,sun50i-a64-mmc",
>> +                                    "allwinner,sun5i-a13-mmc";
> 
> Given that sun5i doesn't support mmc delay timings, and the A64 has
> calibration and delay timings, I wouldn't call them compatible.
> 
> Or are you claiming that for the A64 has a delay of 0 for the
> currently supported speeds, so the calibration doesn't really
> matter? If so this should be mentioned in the commit message.

Yes, that's my observation: Driving it with sun5-a13-mmc just works.
This sun5i driver version does not (and will never) support higher
transfer modes anyway, so for that subset they are compatible. This
opens up the door to other operating systems not having a particular
driver for the A64, for instance, also older Linux kernels.
I know that sunxi doesn't use this compatible feature much, but IMHO we
should really start thinking about the DT not just being Linux specific
- or even being specific to a certain Linux version. And this case here
is a good example: An A13 MMC driver can drive this device - if there is
no better driver (an A64 one) available.

> 
>> +                       reg = <0x01c0f000 0x1000>;
>> +                       clocks = <&ccu 31>, <&ccu 75>;
> 
> The clock / reset index macros are in the tree now.
> Please switch to them.

The include file is in the tree, but it isn't used in the current HEAD
(as in #included and the actual macros being used in the .dtsi).
So I was wondering if there is a patch pending for that?

Cheers,
Andre

>> +                       clock-names = "ahb", "mmc";
>> +                       resets = <&ccu 8>;
>> +                       reset-names = "ahb";
>> +                       interrupts = <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>;
>> +                       status = "disabled";
>> +                       #address-cells = <1>;
>> +                       #size-cells = <0>;
>> +               };
>> +
>> +               mmc1: mmc@1c10000 {
>> +                       compatible = "allwinner,sun50i-a64-mmc",
>> +                                    "allwinner,sun5i-a13-mmc";
>> +                       reg = <0x01c10000 0x1000>;
>> +                       clocks = <&ccu 32>, <&ccu 76>;
>> +                       clock-names = "ahb", "mmc";
>> +                       resets = <&ccu 9>;
>> +                       reset-names = "ahb";
>> +                       interrupts = <GIC_SPI 61 IRQ_TYPE_LEVEL_HIGH>;
>> +                       status = "disabled";
>> +                       #address-cells = <1>;
>> +                       #size-cells = <0>;
>> +               };
>> +
>> +               mmc2: mmc@1c11000 {
>> +                       compatible = "allwinner,sun50i-a64-emmc";
>> +                       reg = <0x01c11000 0x1000>;
>> +                       clocks = <&ccu 33>, <&ccu 77>;
>> +                       clock-names = "ahb", "mmc";
>> +                       resets = <&ccu 10>;
>> +                       reset-names = "ahb";
>> +                       interrupts = <GIC_SPI 62 IRQ_TYPE_LEVEL_HIGH>;
>> +                       status = "disabled";
>> +                       #address-cells = <1>;
>> +                       #size-cells = <0>;
>> +               };
>> +
>>                 gic: interrupt-controller@1c81000 {
>>                         compatible = "arm,gic-400";
>>                         reg = <0x01c81000 0x1000>,
>> --
>> 2.8.2
>>

Re: [PATCH 3/5] arm64: dts: sun50i: add MMC nodes

[Chen-Yu Tsai]

On Tue, Jan 3, 2017 at 6:48 PM, André Przywara <andre.przywara@arm.com> wrote:
> On 03/01/17 02:52, Chen-Yu Tsai wrote:
>
> Hi,
>
>> On Tue, Jan 3, 2017 at 7:03 AM, Andre Przywara <andre.przywara@arm.com> wrote:
>>
>> A commit message explaining the mmc controllers would be nice.
>
> OK.
>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> ---
>>>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 67 +++++++++++++++++++++++++++
>>>  1 file changed, 67 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>>> index e0dcab8..c680566 100644
>>> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>>> @@ -150,6 +150,32 @@
>>>                                 pins = "PB8", "PB9";
>>>                                 function = "uart0";
>>>                         };
>>> +
>>> +                       mmc0_pins: mmc0@0 {
>>> +                               pins = "PF0", "PF1", "PF2", "PF3", "PF4", "PF5";
>>> +                               function = "mmc0";
>>> +                               drive-strength = <30>;
>>> +                       };
>>> +
>>> +                       mmc0_default_cd_pin: mmc0_cd_pin@0 {
>>> +                               pins = "PF6";
>>> +                               function = "gpio_in";
>>> +                               bias-pull-up;
>>> +                       };
>>
>> We are starting to drop pinmux nodes for gpio usage.
>
> And replacing them with what?
> Or do you mean they go in the individual board .dts files?
> In this case I believe having a default pin defined here would help to
> define it in every .dts.

Nope. I meant dropping them. Pinmux and gpio are orthogonal. One should not
need to specify a gpio pinmux to use it as a gpio. We added them because in
the past nothing was preventing someone from claiming an already muxed pin
as a gpio. On some platforms this is fine. For sunxi, this breaks the system,
as the gpio functions are muxed in.

The idea moving forward is that these cases should be guarded in the driver.
Of course we would have to deal with existing dtbs, but lets not add any more.

>>> +
>>> +                       mmc1_pins: mmc1@0 {
>>> +                               pins = "PG0", "PG1", "PG2", "PG3", "PG4", "PG5";
>>> +                               function = "mmc1";
>>> +                               drive-strength = <30>;
>>> +                       };
>>> +
>>> +                       mmc2_pins: mmc2@0 {
>>> +                               pins = "PC1", "PC5", "PC6", "PC8", "PC9",
>>> +                                      "PC10", "PC11", "PC12", "PC13", "PC14",
>>> +                                      "PC15", "PC16";
>>> +                               function = "mmc2";
>>> +                               drive-strength = <30>;
>>> +                       };
>>
>> Moreover I think you should split out the pinmux nodes to a separate patch.
>
> I can surely do, just wondering what's the rationale is behind that?

More or less the "do one thing in one patch" rationale. Of course you can
claim these are the defaults used in the reference design and pretty much
every board out there. Then it makes sense to do them together. :)

>
>>
>>>                 };
>>>
>>>                 uart0: serial@1c28000 {
>>> @@ -240,6 +266,47 @@
>>>                         #size-cells = <0>;
>>>                 };
>>>
>>> +               mmc0: mmc@1c0f000 {
>>> +                       compatible = "allwinner,sun50i-a64-mmc",
>>> +                                    "allwinner,sun5i-a13-mmc";
>>
>> Given that sun5i doesn't support mmc delay timings, and the A64 has
>> calibration and delay timings, I wouldn't call them compatible.
>>
>> Or are you claiming that for the A64 has a delay of 0 for the
>> currently supported speeds, so the calibration doesn't really
>> matter? If so this should be mentioned in the commit message.
>
> Yes, that's my observation: Driving it with sun5-a13-mmc just works.
> This sun5i driver version does not (and will never) support higher
> transfer modes anyway, so for that subset they are compatible. This
> opens up the door to other operating systems not having a particular
> driver for the A64, for instance, also older Linux kernels.
> I know that sunxi doesn't use this compatible feature much, but IMHO we
> should really start thinking about the DT not just being Linux specific
> - or even being specific to a certain Linux version. And this case here
> is a good example: An A13 MMC driver can drive this device - if there is
> no better driver (an A64 one) available.

Cool. Please put this in the commit log. :)

>
>>
>>> +                       reg = <0x01c0f000 0x1000>;
>>> +                       clocks = <&ccu 31>, <&ccu 75>;
>>
>> The clock / reset index macros are in the tree now.
>> Please switch to them.
>
> The include file is in the tree, but it isn't used in the current HEAD
> (as in #included and the actual macros being used in the .dtsi).
> So I was wondering if there is a patch pending for that?

Not yet I think. Perhaps Maxime will do one once he gets back from vacation?

Regards
ChenYu

>
> Cheers,
> Andre
>
>>> +                       clock-names = "ahb", "mmc";
>>> +                       resets = <&ccu 8>;
>>> +                       reset-names = "ahb";
>>> +                       interrupts = <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>;
>>> +                       status = "disabled";
>>> +                       #address-cells = <1>;
>>> +                       #size-cells = <0>;
>>> +               };
>>> +
>>> +               mmc1: mmc@1c10000 {
>>> +                       compatible = "allwinner,sun50i-a64-mmc",
>>> +                                    "allwinner,sun5i-a13-mmc";
>>> +                       reg = <0x01c10000 0x1000>;
>>> +                       clocks = <&ccu 32>, <&ccu 76>;
>>> +                       clock-names = "ahb", "mmc";
>>> +                       resets = <&ccu 9>;
>>> +                       reset-names = "ahb";
>>> +                       interrupts = <GIC_SPI 61 IRQ_TYPE_LEVEL_HIGH>;
>>> +                       status = "disabled";
>>> +                       #address-cells = <1>;
>>> +                       #size-cells = <0>;
>>> +               };
>>> +
>>> +               mmc2: mmc@1c11000 {
>>> +                       compatible = "allwinner,sun50i-a64-emmc";
>>> +                       reg = <0x01c11000 0x1000>;
>>> +                       clocks = <&ccu 33>, <&ccu 77>;
>>> +                       clock-names = "ahb", "mmc";
>>> +                       resets = <&ccu 10>;
>>> +                       reset-names = "ahb";
>>> +                       interrupts = <GIC_SPI 62 IRQ_TYPE_LEVEL_HIGH>;
>>> +                       status = "disabled";
>>> +                       #address-cells = <1>;
>>> +                       #size-cells = <0>;
>>> +               };
>>> +
>>>                 gic: interrupt-controller@1c81000 {
>>>                         compatible = "arm,gic-400";
>>>                         reg = <0x01c81000 0x1000>,
>>> --
>>> 2.8.2
>>>
>

Re: [PATCH 3/5] arm64: dts: sun50i: add MMC nodes

[André Przywara]

On 03/01/17 13:28, Chen-Yu Tsai wrote:
> On Tue, Jan 3, 2017 at 6:48 PM, André Przywara <andre.przywara@arm.com> wrote:
>> On 03/01/17 02:52, Chen-Yu Tsai wrote:

Hi Chen-Yu,

>>> On Tue, Jan 3, 2017 at 7:03 AM, Andre Przywara <andre.przywara@arm.com> wrote:
>>>
>>> A commit message explaining the mmc controllers would be nice.
>>
>> OK.
>>
>>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>>> ---
>>>>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 67 +++++++++++++++++++++++++++
>>>>  1 file changed, 67 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>>>> index e0dcab8..c680566 100644
>>>> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>>>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>>>> @@ -150,6 +150,32 @@
>>>>                                 pins = "PB8", "PB9";
>>>>                                 function = "uart0";
>>>>                         };
>>>> +
>>>> +                       mmc0_pins: mmc0@0 {
>>>> +                               pins = "PF0", "PF1", "PF2", "PF3", "PF4", "PF5";
>>>> +                               function = "mmc0";
>>>> +                               drive-strength = <30>;
>>>> +                       };
>>>> +
>>>> +                       mmc0_default_cd_pin: mmc0_cd_pin@0 {
>>>> +                               pins = "PF6";
>>>> +                               function = "gpio_in";
>>>> +                               bias-pull-up;
>>>> +                       };
>>>
>>> We are starting to drop pinmux nodes for gpio usage.
>>
>> And replacing them with what?
>> Or do you mean they go in the individual board .dts files?
>> In this case I believe having a default pin defined here would help to
>> define it in every .dts.
> 
> Nope. I meant dropping them. Pinmux and gpio are orthogonal. One should not
> need to specify a gpio pinmux to use it as a gpio. We added them because in
> the past nothing was preventing someone from claiming an already muxed pin
> as a gpio. On some platforms this is fine. For sunxi, this breaks the system,
> as the gpio functions are muxed in.
> 
> The idea moving forward is that these cases should be guarded in the driver.

Ah, OK, you mean just referencing the pin like:
        cd-gpios = <&pio 5 6 GPIO_ACTIVE_HIGH>; /* PF6 */
in the board DT is enough? And the driver claiming it will (eventually?)
prevent users from using them via the sysfs interface then?

> Of course we would have to deal with existing dtbs, but lets not add any more.
> 
>>>> +
>>>> +                       mmc1_pins: mmc1@0 {
>>>> +                               pins = "PG0", "PG1", "PG2", "PG3", "PG4", "PG5";
>>>> +                               function = "mmc1";
>>>> +                               drive-strength = <30>;
>>>> +                       };
>>>> +
>>>> +                       mmc2_pins: mmc2@0 {
>>>> +                               pins = "PC1", "PC5", "PC6", "PC8", "PC9",
>>>> +                                      "PC10", "PC11", "PC12", "PC13", "PC14",
>>>> +                                      "PC15", "PC16";
>>>> +                               function = "mmc2";
>>>> +                               drive-strength = <30>;
>>>> +                       };
>>>
>>> Moreover I think you should split out the pinmux nodes to a separate patch.
>>
>> I can surely do, just wondering what's the rationale is behind that?
> 
> More or less the "do one thing in one patch" rationale. Of course you can
> claim these are the defaults used in the reference design and pretty much
> every board out there. Then it makes sense to do them together. :)

Mmmh, probably a misunderstanding, but:
Those pins are the possible pins that expose the MMC interface. Those
nodes here name them to allow easy and concise reference by a board DT
(as in the following patch).
And in fact in case of the A64 there are _no_ alternative muxes for the
three MMC controllers: SDC0 is only on PF0-5, SDC1 on PG0-PG5 and SDC2
on PC1-PC16.
So it's not a board design question, apart from using a controller or not.

That's why I rather keep them together with the MMC controller nodes.

>>>
>>>>                 };
>>>>
>>>>                 uart0: serial@1c28000 {
>>>> @@ -240,6 +266,47 @@
>>>>                         #size-cells = <0>;
>>>>                 };
>>>>
>>>> +               mmc0: mmc@1c0f000 {
>>>> +                       compatible = "allwinner,sun50i-a64-mmc",
>>>> +                                    "allwinner,sun5i-a13-mmc";
>>>
>>> Given that sun5i doesn't support mmc delay timings, and the A64 has
>>> calibration and delay timings, I wouldn't call them compatible.
>>>
>>> Or are you claiming that for the A64 has a delay of 0 for the
>>> currently supported speeds, so the calibration doesn't really
>>> matter? If so this should be mentioned in the commit message.
>>
>> Yes, that's my observation: Driving it with sun5-a13-mmc just works.
>> This sun5i driver version does not (and will never) support higher
>> transfer modes anyway, so for that subset they are compatible. This
>> opens up the door to other operating systems not having a particular
>> driver for the A64, for instance, also older Linux kernels.
>> I know that sunxi doesn't use this compatible feature much, but IMHO we
>> should really start thinking about the DT not just being Linux specific
>> - or even being specific to a certain Linux version. And this case here
>> is a good example: An A13 MMC driver can drive this device - if there is
>> no better driver (an A64 one) available.
> 
> Cool. Please put this in the commit log. :)

Noted for a repost.

>>
>>>
>>>> +                       reg = <0x01c0f000 0x1000>;
>>>> +                       clocks = <&ccu 31>, <&ccu 75>;
>>>
>>> The clock / reset index macros are in the tree now.
>>> Please switch to them.
>>
>> The include file is in the tree, but it isn't used in the current HEAD
>> (as in #included and the actual macros being used in the .dtsi).
>> So I was wondering if there is a patch pending for that?
> 
> Not yet I think. Perhaps Maxime will do one once he gets back from vacation?

Seems like everybody hopes for it and nobody bothers with this rather
trivial patch ;-)
Frankly I am not overly keen on having all those defines in the DTs,
which rely on a bunch of include files scattered over the Linux include
tree. This makes it unnecessarily complicated to compile this out of
tree or move it over to some other piece of software (other OS, U-Boot).
Also it gives people the impression that it's fine to change the
definition in the include file at will (because it looks like Linux
territory).
Eventually some file has to hold the actual hardware information, I
don't see why this shouldn't be the one .dtsi file. Reading that the
clock for MMC controller 0 is MMC0_CLK isn't very informative.

Cheers,
Andre.

>>>> +                       clock-names = "ahb", "mmc";
>>>> +                       resets = <&ccu 8>;
>>>> +                       reset-names = "ahb";
>>>> +                       interrupts = <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>;
>>>> +                       status = "disabled";
>>>> +                       #address-cells = <1>;
>>>> +                       #size-cells = <0>;
>>>> +               };
>>>> +
>>>> +               mmc1: mmc@1c10000 {
>>>> +                       compatible = "allwinner,sun50i-a64-mmc",
>>>> +                                    "allwinner,sun5i-a13-mmc";
>>>> +                       reg = <0x01c10000 0x1000>;
>>>> +                       clocks = <&ccu 32>, <&ccu 76>;
>>>> +                       clock-names = "ahb", "mmc";
>>>> +                       resets = <&ccu 9>;
>>>> +                       reset-names = "ahb";
>>>> +                       interrupts = <GIC_SPI 61 IRQ_TYPE_LEVEL_HIGH>;
>>>> +                       status = "disabled";
>>>> +                       #address-cells = <1>;
>>>> +                       #size-cells = <0>;
>>>> +               };
>>>> +
>>>> +               mmc2: mmc@1c11000 {
>>>> +                       compatible = "allwinner,sun50i-a64-emmc";
>>>> +                       reg = <0x01c11000 0x1000>;
>>>> +                       clocks = <&ccu 33>, <&ccu 77>;
>>>> +                       clock-names = "ahb", "mmc";
>>>> +                       resets = <&ccu 10>;
>>>> +                       reset-names = "ahb";
>>>> +                       interrupts = <GIC_SPI 62 IRQ_TYPE_LEVEL_HIGH>;
>>>> +                       status = "disabled";
>>>> +                       #address-cells = <1>;
>>>> +                       #size-cells = <0>;
>>>> +               };
>>>> +
>>>>                 gic: interrupt-controller@1c81000 {
>>>>                         compatible = "arm,gic-400";
>>>>                         reg = <0x01c81000 0x1000>,
>>>> --
>>>> 2.8.2
>>>>
>>

Re: [PATCH 3/5] arm64: dts: sun50i: add MMC nodes

[Chen-Yu Tsai]

On Wed, Jan 4, 2017 at 8:22 AM, André Przywara <andre.przywara@arm.com> wrote:
> On 03/01/17 13:28, Chen-Yu Tsai wrote:
>> On Tue, Jan 3, 2017 at 6:48 PM, André Przywara <andre.przywara@arm.com> wrote:
>>> On 03/01/17 02:52, Chen-Yu Tsai wrote:
>
> Hi Chen-Yu,
>
>>>> On Tue, Jan 3, 2017 at 7:03 AM, Andre Przywara <andre.przywara@arm.com> wrote:
>>>>
>>>> A commit message explaining the mmc controllers would be nice.
>>>
>>> OK.
>>>
>>>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>>>> ---
>>>>>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 67 +++++++++++++++++++++++++++
>>>>>  1 file changed, 67 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>>>>> index e0dcab8..c680566 100644
>>>>> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>>>>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>>>>> @@ -150,6 +150,32 @@
>>>>>                                 pins = "PB8", "PB9";
>>>>>                                 function = "uart0";
>>>>>                         };
>>>>> +
>>>>> +                       mmc0_pins: mmc0@0 {
>>>>> +                               pins = "PF0", "PF1", "PF2", "PF3", "PF4", "PF5";
>>>>> +                               function = "mmc0";
>>>>> +                               drive-strength = <30>;
>>>>> +                       };
>>>>> +
>>>>> +                       mmc0_default_cd_pin: mmc0_cd_pin@0 {
>>>>> +                               pins = "PF6";
>>>>> +                               function = "gpio_in";
>>>>> +                               bias-pull-up;
>>>>> +                       };
>>>>
>>>> We are starting to drop pinmux nodes for gpio usage.
>>>
>>> And replacing them with what?
>>> Or do you mean they go in the individual board .dts files?
>>> In this case I believe having a default pin defined here would help to
>>> define it in every .dts.
>>
>> Nope. I meant dropping them. Pinmux and gpio are orthogonal. One should not
>> need to specify a gpio pinmux to use it as a gpio. We added them because in
>> the past nothing was preventing someone from claiming an already muxed pin
>> as a gpio. On some platforms this is fine. For sunxi, this breaks the system,
>> as the gpio functions are muxed in.
>>
>> The idea moving forward is that these cases should be guarded in the driver.
>
> Ah, OK, you mean just referencing the pin like:
>         cd-gpios = <&pio 5 6 GPIO_ACTIVE_HIGH>; /* PF6 */
> in the board DT is enough? And the driver claiming it will (eventually?)
> prevent users from using them via the sysfs interface then?

cd-gpios = ....

should be enough to block other devices from claiming a pinmux (pinctrl-N = ...)
on the same pin, or vice versa, whichever claimed it first in kernel space.

Same goes for a pin already claimed by a device through a pinmux. It should
not be claimable as a gpio through the sysfs interface.

>> Of course we would have to deal with existing dtbs, but lets not add any more.
>>
>>>>> +
>>>>> +                       mmc1_pins: mmc1@0 {
>>>>> +                               pins = "PG0", "PG1", "PG2", "PG3", "PG4", "PG5";
>>>>> +                               function = "mmc1";
>>>>> +                               drive-strength = <30>;
>>>>> +                       };
>>>>> +
>>>>> +                       mmc2_pins: mmc2@0 {
>>>>> +                               pins = "PC1", "PC5", "PC6", "PC8", "PC9",
>>>>> +                                      "PC10", "PC11", "PC12", "PC13", "PC14",
>>>>> +                                      "PC15", "PC16";
>>>>> +                               function = "mmc2";
>>>>> +                               drive-strength = <30>;
>>>>> +                       };
>>>>
>>>> Moreover I think you should split out the pinmux nodes to a separate patch.
>>>
>>> I can surely do, just wondering what's the rationale is behind that?
>>
>> More or less the "do one thing in one patch" rationale. Of course you can
>> claim these are the defaults used in the reference design and pretty much
>> every board out there. Then it makes sense to do them together. :)
>
> Mmmh, probably a misunderstanding, but:
> Those pins are the possible pins that expose the MMC interface. Those
> nodes here name them to allow easy and concise reference by a board DT
> (as in the following patch).
> And in fact in case of the A64 there are _no_ alternative muxes for the
> three MMC controllers: SDC0 is only on PF0-5, SDC1 on PG0-PG5 and SDC2
> on PC1-PC16.
> So it's not a board design question, apart from using a controller or not.
>
> That's why I rather keep them together with the MMC controller nodes.

Cool. Please mention this in the commit log, as in they are the only
possible choice.S ince they are the only choices, please drop the @0
in the node name.

And you might want to rename mmc2_pins to something like
"mmc2_8bit_pins: mmc2_8bit". Who knows, someone might consider using
it for an SD card or SDIO, and the remaining pins for something else.

>>>>
>>>>>                 };
>>>>>
>>>>>                 uart0: serial@1c28000 {
>>>>> @@ -240,6 +266,47 @@
>>>>>                         #size-cells = <0>;
>>>>>                 };
>>>>>
>>>>> +               mmc0: mmc@1c0f000 {
>>>>> +                       compatible = "allwinner,sun50i-a64-mmc",
>>>>> +                                    "allwinner,sun5i-a13-mmc";
>>>>
>>>> Given that sun5i doesn't support mmc delay timings, and the A64 has
>>>> calibration and delay timings, I wouldn't call them compatible.
>>>>
>>>> Or are you claiming that for the A64 has a delay of 0 for the
>>>> currently supported speeds, so the calibration doesn't really
>>>> matter? If so this should be mentioned in the commit message.
>>>
>>> Yes, that's my observation: Driving it with sun5-a13-mmc just works.
>>> This sun5i driver version does not (and will never) support higher
>>> transfer modes anyway, so for that subset they are compatible. This
>>> opens up the door to other operating systems not having a particular
>>> driver for the A64, for instance, also older Linux kernels.
>>> I know that sunxi doesn't use this compatible feature much, but IMHO we
>>> should really start thinking about the DT not just being Linux specific
>>> - or even being specific to a certain Linux version. And this case here
>>> is a good example: An A13 MMC driver can drive this device - if there is
>>> no better driver (an A64 one) available.
>>
>> Cool. Please put this in the commit log. :)
>
> Noted for a repost.
>
>>>
>>>>
>>>>> +                       reg = <0x01c0f000 0x1000>;
>>>>> +                       clocks = <&ccu 31>, <&ccu 75>;
>>>>
>>>> The clock / reset index macros are in the tree now.
>>>> Please switch to them.
>>>
>>> The include file is in the tree, but it isn't used in the current HEAD
>>> (as in #included and the actual macros being used in the .dtsi).
>>> So I was wondering if there is a patch pending for that?
>>
>> Not yet I think. Perhaps Maxime will do one once he gets back from vacation?
>
> Seems like everybody hopes for it and nobody bothers with this rather
> trivial patch ;-)

A bit of context here. This was changed _after_ the A64 patches were
pulled into arm-soc, by the arm-soc maintainers, to avoid ugly cross
tree merges. The idea was to change them back after -rc1. I don't know
if a patch to do that is ready though.

> Frankly I am not overly keen on having all those defines in the DTs,
> which rely on a bunch of include files scattered over the Linux include
> tree. This makes it unnecessarily complicated to compile this out of
> tree or move it over to some other piece of software (other OS, U-Boot).
> Also it gives people the impression that it's fine to change the
> definition in the include file at will (because it looks like Linux
> territory).

The dt-bindings related header files are split into a separate directory.
They are also included in the separate devicetree repository:

    https://git.kernel.org/cgit/linux/kernel/git/devicetree/devicetree-rebasing.git/

And no, I don't think it implies that it's fine to change it. The
include/dt-bindings/ subdirectory is part of the device tree bindings,
just like Documentation/devicetree/bindings. Does the latter give the
impression that it is fine to change it, because it's in the docs?

> Eventually some file has to hold the actual hardware information, I
> don't see why this shouldn't be the one .dtsi file. Reading that the
> clock for MMC controller 0 is MMC0_CLK isn't very informative.

For me, it makes it much easier to review, than say a plain number,
which I would have to dig through some file, probably the clock driver,
to check if it matches.

Regards
ChenYu

> Cheers,
> Andre.
>
>>>>> +                       clock-names = "ahb", "mmc";
>>>>> +                       resets = <&ccu 8>;
>>>>> +                       reset-names = "ahb";
>>>>> +                       interrupts = <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>;
>>>>> +                       status = "disabled";
>>>>> +                       #address-cells = <1>;
>>>>> +                       #size-cells = <0>;
>>>>> +               };
>>>>> +
>>>>> +               mmc1: mmc@1c10000 {
>>>>> +                       compatible = "allwinner,sun50i-a64-mmc",
>>>>> +                                    "allwinner,sun5i-a13-mmc";
>>>>> +                       reg = <0x01c10000 0x1000>;
>>>>> +                       clocks = <&ccu 32>, <&ccu 76>;
>>>>> +                       clock-names = "ahb", "mmc";
>>>>> +                       resets = <&ccu 9>;
>>>>> +                       reset-names = "ahb";
>>>>> +                       interrupts = <GIC_SPI 61 IRQ_TYPE_LEVEL_HIGH>;
>>>>> +                       status = "disabled";
>>>>> +                       #address-cells = <1>;
>>>>> +                       #size-cells = <0>;
>>>>> +               };
>>>>> +
>>>>> +               mmc2: mmc@1c11000 {
>>>>> +                       compatible = "allwinner,sun50i-a64-emmc";
>>>>> +                       reg = <0x01c11000 0x1000>;
>>>>> +                       clocks = <&ccu 33>, <&ccu 77>;
>>>>> +                       clock-names = "ahb", "mmc";
>>>>> +                       resets = <&ccu 10>;
>>>>> +                       reset-names = "ahb";
>>>>> +                       interrupts = <GIC_SPI 62 IRQ_TYPE_LEVEL_HIGH>;
>>>>> +                       status = "disabled";
>>>>> +                       #address-cells = <1>;
>>>>> +                       #size-cells = <0>;
>>>>> +               };
>>>>> +
>>>>>                 gic: interrupt-controller@1c81000 {
>>>>>                         compatible = "arm,gic-400";
>>>>>                         reg = <0x01c81000 0x1000>,
>>>>> --
>>>>> 2.8.2
>>>>>
>>>
>

Re: [PATCH 2/5] drivers: mmc: sunxi: limit A64 MMC2 to 8K DMA buffer

[Rob Herring]

On Mon, Jan 02, 2017 at 11:03:43PM +0000, Andre Przywara wrote:
> From: Maxime Ripard <maxime.ripard@free-electrons.com>
> 
> Unlike the A64 user manual reports, the third MMC controller on the
> A64 (and the only one capable of 8-bit HS400 eMMC transfers) has a
> DMA buffer size limit of 8KB (much like the very old Allwinner SoCs).
> This does not affect the other two controllers, so introduce a new
> DT compatible string to let the driver use different settings for that
> particular device. This will also help to enable the high-speed transfer
> modes of that controller later.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  Documentation/devicetree/bindings/mmc/sunxi-mmc.txt | 1 +
>  drivers/mmc/host/sunxi-mmc.c                        | 7 +++++++
>  2 files changed, 8 insertions(+)

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 5/5] arm64: dts: add BananaPi-M64 support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 6505 bytes --]

On Mon, Jan 02, 2017 at 11:03:46PM +0000, Andre Przywara wrote:
> The Banana Pi M64 board is a typical single board computer based on the
> Allwinner A64 SoC. Aside from the usual peripherals it features eMMC
> storage, which is connected to the 8-bit capable SDHC2 controller.
> Also it has a soldered WiFi/Bluetooth chip, so we enable UART1 and SDHC1
> as those two interfaces are connected to it.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arch/arm64/boot/dts/allwinner/Makefile             |   1 +
>  .../boot/dts/allwinner/sun50i-a64-bananapi-m64.dts | 125 +++++++++++++++++++++
>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi      |  10 ++
>  3 files changed, 136 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
> 
> diff --git a/arch/arm64/boot/dts/allwinner/Makefile b/arch/arm64/boot/dts/allwinner/Makefile
> index 1e29a5a..51ecb04 100644
> --- a/arch/arm64/boot/dts/allwinner/Makefile
> +++ b/arch/arm64/boot/dts/allwinner/Makefile
> @@ -1,4 +1,5 @@
>  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-a64-pine64-plus.dtb sun50i-a64-pine64.dtb
> +dtb-$(CONFIG_ARCH_SUNXI) += sun50i-a64-bananapi-m64.dtb

Please sort it by alphabetical order.

>  
>  always		:= $(dtb-y)
>  subdir-y	:= $(dts-dirs)
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
> new file mode 100644
> index 0000000..941ea07
> --- /dev/null
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
> @@ -0,0 +1,125 @@
> +/*
> + * Copyright (c) 2016 ARM Ltd.
> + *
> + * This file is dual-licensed: you can use it either under the terms
> + * of the GPL or the X11 license, at your option. Note that this dual
> + * licensing only applies to this file, and not this project as a
> + * whole.
> + *
> + *  a) This library 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 library 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.
> + *
> + * Or, alternatively,
> + *
> + *  b) Permission is hereby granted, free of charge, to any person
> + *     obtaining a copy of this software and associated documentation
> + *     files (the "Software"), to deal in the Software without
> + *     restriction, including without limitation the rights to use,
> + *     copy, modify, merge, publish, distribute, sublicense, and/or
> + *     sell copies of the Software, and to permit persons to whom the
> + *     Software is furnished to do so, subject to the following
> + *     conditions:
> + *
> + *     The above copyright notice and this permission notice shall be
> + *     included in all copies or substantial portions of the Software.
> + *
> + *     THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + *     EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
> + *     OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + *     NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
> + *     HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
> + *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + *     OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +/dts-v1/;
> +
> +#include "sun50i-a64.dtsi"
> +
> +/ {
> +	model = "BananaPi-M64";
> +	compatible = "sinovoip,bananapi-m64", "allwinner,sun50i-a64";
> +
> +	aliases {
> +		serial0 = &uart0;
> +		serial1 = &uart1;
> +	};
> +
> +	chosen {
> +		stdout-path = "serial0:115200n8";
> +	};
> +
> +	reg_vcc3v3: vcc3v3 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc3v3";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +	};
> +};
> +
> +&uart0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&uart0_pins_a>;
> +	status = "okay";
> +};
> +
> +&uart1 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&uart1_pins_4>;
> +	status = "okay";
> +};
> +
> +&i2c1 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&i2c1_pins>;
> +	status = "okay";
> +};

And the nodes too.

> +
> +&i2c1_pins {
> +	bias-pull-up;
> +};
> +
> +&mmc0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&mmc0_pins>, <&mmc0_default_cd_pin>;
> +	vmmc-supply = <&reg_vcc3v3>;
> +	cd-gpios = <&pio 5 6 0>;

Please use a define here.

> +	cd-inverted;
> +	disable-wp;
> +	bus-width = <4>;
> +	status = "okay";
> +};
> +
> +&mmc1 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&mmc1_pins>;
> +	vmmc-supply = <&reg_vcc3v3>;
> +	bus-width = <4>;
> +	non-removable;
> +	status = "okay";
> +};
> +
> +&mmc2 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&mmc2_pins>;
> +	vmmc-supply = <&reg_vcc3v3>;
> +	bus-width = <8>;
> +	non-removable;
> +	cap-mmc-hw-reset;
> +	status = "okay";
> +};
> +
> +&mmc2_pins {
> +	/* Increase drive strength for DDR modes */
> +	drive-strength = <40>;

Allwinner recommends to use 30mA, have you tested it?

> +	/* eMMC is missing pull-ups */
> +	bias-pull-up;

This is the default now.

> +};
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> index c680566..9de96ba 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> @@ -151,6 +151,16 @@
>  				function = "uart0";
>  			};
>  
> +			uart1_pins: uart1@0 {
> +				pins = "PG6", "PG7";
> +				function = "uart1";
> +			};
> +
> +			uart1_pins_4: uart1_4@0 {
> +				pins = "PG6", "PG7", "PG8", "PG9";

For other nodes, we've been creating another node for the cts and rts
signals, and reference both nodes in the pinctrl properties. It allows
to avoid duplication.

> +				function = "uart1";
> +			};
> +
>  			mmc0_pins: mmc0@0 {

Please sort it by alphabetical order. And it should be in a separate
patch.

Thanks!
Maxime

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 1/5] drivers: mmc: sunxi: fix A64 calibration routine

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1268 bytes --]

Hi,

On Mon, Jan 02, 2017 at 11:03:42PM +0000, Andre Przywara wrote:
> The calibration facility in the A64 MMC block seems to have been
> misunderstood: the result value is not the value to program into the
> delay bits, but is the number of delay cells that result in a full clock
> cycle delay. So this value has to be scaled by the desired phase, which
> we still have to know and program.
> Change the calibration routine to take a phase parameter and scale the
> calibration value accordingly.
> Also introduce sun50i-a64 delay parameters to store the required phase.
> Looking at the BSP kernel the sample delay for anything below HS200 is
> 0, so we go with that value.
> Once the driver supports HS200 and faster modes, we can enter confirmed
> working values in there.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Exactly how that works hasn't been confirmed, and the only thing that
this patch actually do is... nothing, since the delay is always 0. If
and when we get HS400 to work and we know for a fact how the
calibration works, then we'll be able to use it. Until then, we can
just clear those bits.

Maxime

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 3/5] arm64: dts: sun50i: add MMC nodes

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1954 bytes --]

On Mon, Jan 02, 2017 at 11:03:44PM +0000, Andre Przywara wrote:
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 67 +++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> index e0dcab8..c680566 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> @@ -150,6 +150,32 @@
>  				pins = "PB8", "PB9";
>  				function = "uart0";
>  			};
> +
> +			mmc0_pins: mmc0@0 {
> +				pins = "PF0", "PF1", "PF2", "PF3", "PF4", "PF5";
> +				function = "mmc0";
> +				drive-strength = <30>;
> +			};
> +
> +			mmc0_default_cd_pin: mmc0_cd_pin@0 {
> +				pins = "PF6";
> +				function = "gpio_in";
> +				bias-pull-up;
> +			};
> +
> +			mmc1_pins: mmc1@0 {
> +				pins = "PG0", "PG1", "PG2", "PG3", "PG4", "PG5";
> +				function = "mmc1";
> +				drive-strength = <30>;
> +			};
> +
> +			mmc2_pins: mmc2@0 {
> +				pins = "PC1", "PC5", "PC6", "PC8", "PC9",
> +				       "PC10", "PC11", "PC12", "PC13", "PC14",
> +				       "PC15", "PC16";
> +				function = "mmc2";
> +				drive-strength = <30>;
> +			};
>  		};
>  
>  		uart0: serial@1c28000 {
> @@ -240,6 +266,47 @@
>  			#size-cells = <0>;
>  		};
>  
> +		mmc0: mmc@1c0f000 {
> +			compatible = "allwinner,sun50i-a64-mmc",
> +				     "allwinner,sun5i-a13-mmc";

That's not correct. There's a bunch of features that are broken
without A64-specific (or at least not present on the A13)
quirks. Those features include for example the SDIO aggregation that
are currently broken for all modes supported so far.

I was in holidays and send those patches tomorrow, but you were a bit
too quick :)

Maxime

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 2/5] drivers: mmc: sunxi: limit A64 MMC2 to 8K DMA buffer

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1648 bytes --]

Hi Rob,

On Wed, Jan 04, 2017 at 08:07:50AM -0600, Rob Herring wrote:
> On Mon, Jan 02, 2017 at 11:03:43PM +0000, Andre Przywara wrote:
> > From: Maxime Ripard <maxime.ripard@free-electrons.com>
> > 
> > Unlike the A64 user manual reports, the third MMC controller on the
> > A64 (and the only one capable of 8-bit HS400 eMMC transfers) has a
> > DMA buffer size limit of 8KB (much like the very old Allwinner SoCs).
> > This does not affect the other two controllers, so introduce a new
> > DT compatible string to let the driver use different settings for that
> > particular device. This will also help to enable the high-speed transfer
> > modes of that controller later.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  Documentation/devicetree/bindings/mmc/sunxi-mmc.txt | 1 +
> >  drivers/mmc/host/sunxi-mmc.c                        | 7 +++++++
> >  2 files changed, 8 insertions(+)
> 
> Acked-by: Rob Herring <robh@kernel.org>

Some kind of a digression on this: we have three MMC controllers on
this SoC. Like this patch shows, the third one is clearly different,
and supports both more modes, a wider bus, and specific quirks. We
need a new compatible for this one, everything's perfect.

However, the other two are mostly the same, but seems to need
different tuning parameters to get more performances out of the
controller (but this is unclear yet). How do we usually deal with
that?

Thanks,
Maxime

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 2/5] drivers: mmc: sunxi: limit A64 MMC2 to 8K DMA buffer

[André Przywara]

On 05/01/17 17:57, Maxime Ripard wrote:
> Hi Rob,
> 
> On Wed, Jan 04, 2017 at 08:07:50AM -0600, Rob Herring wrote:
>> On Mon, Jan 02, 2017 at 11:03:43PM +0000, Andre Przywara wrote:
>>> From: Maxime Ripard <maxime.ripard@free-electrons.com>
>>>
>>> Unlike the A64 user manual reports, the third MMC controller on the
>>> A64 (and the only one capable of 8-bit HS400 eMMC transfers) has a
>>> DMA buffer size limit of 8KB (much like the very old Allwinner SoCs).
>>> This does not affect the other two controllers, so introduce a new
>>> DT compatible string to let the driver use different settings for that
>>> particular device. This will also help to enable the high-speed transfer
>>> modes of that controller later.
>>>
>>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> ---
>>>  Documentation/devicetree/bindings/mmc/sunxi-mmc.txt | 1 +
>>>  drivers/mmc/host/sunxi-mmc.c                        | 7 +++++++
>>>  2 files changed, 8 insertions(+)
>>
>> Acked-by: Rob Herring <robh@kernel.org>
> 
> Some kind of a digression on this: we have three MMC controllers on
> this SoC. Like this patch shows, the third one is clearly different,
> and supports both more modes, a wider bus, and specific quirks. We
> need a new compatible for this one, everything's perfect.
> 
> However, the other two are mostly the same, but seems to need
> different tuning parameters to get more performances out of the
> controller (but this is unclear yet). How do we usually deal with
> that?

I guess you wanted to hear Rob's opinion ;-), but "get more performance"
sounds like we add one (or more) properties to tune those values.
If I get this right, it works with default values, but is sub-optimal?

Cheers,
Andre.

Re: [PATCH 1/5] drivers: mmc: sunxi: fix A64 calibration routine

[André Przywara]

On 05/01/17 17:47, Maxime Ripard wrote:

Hi,

> On Mon, Jan 02, 2017 at 11:03:42PM +0000, Andre Przywara wrote:
>> The calibration facility in the A64 MMC block seems to have been
>> misunderstood: the result value is not the value to program into the
>> delay bits, but is the number of delay cells that result in a full clock
>> cycle delay. So this value has to be scaled by the desired phase, which
>> we still have to know and program.
>> Change the calibration routine to take a phase parameter and scale the
>> calibration value accordingly.
>> Also introduce sun50i-a64 delay parameters to store the required phase.
>> Looking at the BSP kernel the sample delay for anything below HS200 is
>> 0, so we go with that value.
>> Once the driver supports HS200 and faster modes, we can enter confirmed
>> working values in there.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> 
> Exactly how that works hasn't been confirmed, and the only thing that
> this patch actually do is... nothing, since the delay is always 0. If
> and when we get HS400 to work and we know for a fact how the
> calibration works, then we'll be able to use it. Until then, we can
> just clear those bits.

Fair enough, though I am a bit puzzled as what to do here now:

Dropping this patch here entirely causes the wrong calibration settings
to degrade the BananaPi eMMC performance by up to 50% (20 MB/s vs.
40MB/s in hdparm, and 25% longer execution time for a "find / -type f |
xargs md5sum > /dev/null" run). So this is not an option.

1) Shall I simply revert Icenowy's original patch that introduced the
calibration function? That should leave the values at their reset value
of 0.  But do we want to make sure that these values are set to 0 by
explicitly zeroing the bits?
Also I guess your HS-400 support will need to write some values in
there, so we will need some code later again?

2) Changing the calibration function to don't do any calibration really
and just write 0 into the delay bits seems like an option, but looks a
bit weird. Also I guess your faster transfer modes support will need to
write _something_, though I don't know what those values are and where
they will come from.

So I am leaning towards 1) for now, unless you send your MMC patches and
we at least merge the patch dealing with the calibration part for the
next release.

Any recommendations? I would love to see the MMC support go into 4.11.

Cheers,
Andre.

Re: [PATCH 2/5] drivers: mmc: sunxi: limit A64 MMC2 to 8K DMA buffer

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2175 bytes --]

On Thu, Jan 05, 2017 at 11:33:28PM +0000, André Przywara wrote:
> On 05/01/17 17:57, Maxime Ripard wrote:
> > Hi Rob,
> > 
> > On Wed, Jan 04, 2017 at 08:07:50AM -0600, Rob Herring wrote:
> >> On Mon, Jan 02, 2017 at 11:03:43PM +0000, Andre Przywara wrote:
> >>> From: Maxime Ripard <maxime.ripard@free-electrons.com>
> >>>
> >>> Unlike the A64 user manual reports, the third MMC controller on the
> >>> A64 (and the only one capable of 8-bit HS400 eMMC transfers) has a
> >>> DMA buffer size limit of 8KB (much like the very old Allwinner SoCs).
> >>> This does not affect the other two controllers, so introduce a new
> >>> DT compatible string to let the driver use different settings for that
> >>> particular device. This will also help to enable the high-speed transfer
> >>> modes of that controller later.
> >>>
> >>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> >>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> >>> ---
> >>>  Documentation/devicetree/bindings/mmc/sunxi-mmc.txt | 1 +
> >>>  drivers/mmc/host/sunxi-mmc.c                        | 7 +++++++
> >>>  2 files changed, 8 insertions(+)
> >>
> >> Acked-by: Rob Herring <robh@kernel.org>
> > 
> > Some kind of a digression on this: we have three MMC controllers on
> > this SoC. Like this patch shows, the third one is clearly different,
> > and supports both more modes, a wider bus, and specific quirks. We
> > need a new compatible for this one, everything's perfect.
> > 
> > However, the other two are mostly the same, but seems to need
> > different tuning parameters to get more performances out of the
> > controller (but this is unclear yet). How do we usually deal with
> > that?
> 
> I guess you wanted to hear Rob's opinion ;-), but "get more performance"
> sounds like we add one (or more) properties to tune those values.
> If I get this right, it works with default values, but is sub-optimal?

That would be my understanding too, at least, it works in a decent way
without fiddling with those parameters.

Maxime

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Dropping device tree pinmux nodes for GPIO usage (Was: [PATCH 3/5] arm64: dts: sun50i: add MMC nodes)

[Rask Ingemann Lambertsen]

On Tue, Jan 03, 2017 at 10:52:12AM +0800, Chen-Yu Tsai wrote:
> On Tue, Jan 3, 2017 at 7:03 AM, Andre Przywara <andre.przywara@arm.com> wrote:
> > +
> > +                       mmc0_default_cd_pin: mmc0_cd_pin@0 {
> > +                               pins = "PF6";
> > +                               function = "gpio_in";
> > +                               bias-pull-up;
> > +                       };
> 
> We are starting to drop pinmux nodes for gpio usage.

How do we get the equivalent of bias-pull-up/down and drive-strength if we
run across a pin that needs it?

-- 
Rask Ingemann Lambertsen

Re: Dropping device tree pinmux nodes for GPIO usage (Was: [PATCH 3/5] arm64: dts: sun50i: add MMC nodes)

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 931 bytes --]

On Sun, Jan 15, 2017 at 04:59:32PM +0100, Rask Ingemann Lambertsen wrote:
> On Tue, Jan 03, 2017 at 10:52:12AM +0800, Chen-Yu Tsai wrote:
> > On Tue, Jan 3, 2017 at 7:03 AM, Andre Przywara <andre.przywara@arm.com> wrote:
> > > +
> > > +                       mmc0_default_cd_pin: mmc0_cd_pin@0 {
> > > +                               pins = "PF6";
> > > +                               function = "gpio_in";
> > > +                               bias-pull-up;
> > > +                       };
> > 
> > We are starting to drop pinmux nodes for gpio usage.
> 
> How do we get the equivalent of bias-pull-up/down and drive-strength if we
> run across a pin that needs it?

Hmm, that's actually a very good question...

For those cases, leave the pinctrl node for now, we'll deal with that
in due time

Maxime

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]