[PATCH v5 0/3] Amlogic 32-bit Meson SoC SDHC MMC controller driver

[PATCH v5 0/3] Amlogic 32-bit Meson SoC SDHC MMC controller driver

[Martin Blumenstingl]

Hello,

this is the patchset for a driver for the Amlogic "SDHC" MMC controller
found on Meson6, Meson8, Meson8b and Meson8m2 SoCs.

The public S805 (Meson8b) datasheet has some documentation starting on
page 74: [0]

It's performance is still not as good as the driver from Amlogic's 3.10
kernel, but it does not corrupt data anymore (as RFC v1 did).

Special thanks to the people who supported me off-list - you are
amazing and deserve to be mentioned here:
- Xin Yin who helped me fix two more write corruption problems. I am
  hoping that he will reply with Reviewed-by, Tested-by and Bug-fixed-by
- Jianxin Pan for sharing some of the internal workings of this MMC
  controller with me
- Wei Wang for spotting the initial write corruption problem and helping
  test this driver on his board. I have his permission to add his
  Tested-by (off-list, he's Cc'ed so if there's any problem he can speak
  up)


Changes since v4 at [4]:
- move the four clkin clock inputs to the start of the clock-names list
  as suggested by Rob, affects patch #1
- fixed #include statement in dt-bindings example in patch #1

Changes since v3 at [3]:
- split the clock bits into a separate clock controller driver because
  of two reasons: 1) it keeps the MMC controller driver mostly clean of
  the clock bits 2) the pure clock controller can use
  devm_clk_hw_register() (instead of devm_clk_register(), which is
  deprecated) and the MMC controller can act as a pure clock consumer.
  This also affects the dt-bindings which is why I dropped Rob's
  Reviewed-by. Thanks to Ulf for the suggestions

Changes since v2 at [2]:
- rebased on top of v5.5-rc1
- added Rob's and Xin Yin's Reviewed-by and Tested-by (thank you!)
- (note: Kevin had v2 of this series in -next for a few days so the
   build test robots could play with it. I haven't received any negative
   feedback in that time)

Changes since RFC v1 at [1]:
- don't set MESON_SDHC_MISC_MANUAL_STOP to fix one of three write
  corruption problems. the out-of-tree 3.10 "reference" driver doesn't
  set it either
- check against data->flags instead of cmd->flags when testing for
  MMC_DATA_WRITE as spotted by Xin Yin (many thanks!). This fixes
  another write corruption problem
- clear the FIFOs after successfully transferring data as suggested by
  Xin Yin (many thanks!). This is what the 3.10 driver did and fixes yet
  another write corruption problem
- integrate the clock suggestions from Jianxin Pan so the driver is now
  able to set up the clocks correctly for all known cases. documentation
  is also added to the patch description. Thank you Jianxin for the
  help!
- set the correct max_busy_timeout as suggested by Jianxin Pan (thanks!)
- convert the dt-bindings to .yaml (which is why I didn't add Rob's
  Reviewed-by)
- switch to struct clk_parent_data as part of newer common clock
  framework APIs to simplify the clock setup
- dropped CMD23 support because it seems to hurt read and write
  performance by 10-20% in my tests. it's not clear why, but for now we
  can live without this.
- use devm_platform_ioremap_resource instead of open-coding it


[0] https://dn.odroid.com/S805/Datasheet/S805_Datasheet%20V0.8%2020150126.pdf
[1] https://patchwork.kernel.org/cover/11035505/
[2] http://lists.infradead.org/pipermail/linux-amlogic/2019-November/014576.html
[3] https://patchwork.kernel.org/cover/11283179/
[4] https://patchwork.kernel.org/cover/11329017/

Martin Blumenstingl (3):
  dt-bindings: mmc: Document the Amlogic Meson SDHC MMC host controller
  clk: meson: add a driver for the Meson8/8b/8m2 SDHC clock controller
  mmc: host: meson-mx-sdhc: new driver for the Amlogic Meson SDHC host

 .../bindings/mmc/amlogic,meson-mx-sdhc.yaml   |   83 ++
 drivers/clk/meson/Kconfig                     |    9 +
 drivers/clk/meson/Makefile                    |    1 +
 drivers/clk/meson/meson-mx-sdhc.c             |  212 ++++
 drivers/mmc/host/Kconfig                      |   14 +
 drivers/mmc/host/Makefile                     |    1 +
 drivers/mmc/host/meson-mx-sdhc.c              | 1064 +++++++++++++++++
 .../dt-bindings/clock/meson-mx-sdhc-clkc.h    |    8 +
 8 files changed, 1392 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mmc/amlogic,meson-mx-sdhc.yaml
 create mode 100644 drivers/clk/meson/meson-mx-sdhc.c
 create mode 100644 drivers/mmc/host/meson-mx-sdhc.c
 create mode 100644 include/dt-bindings/clock/meson-mx-sdhc-clkc.h

-- 
2.26.0

[PATCH v5 1/3] dt-bindings: mmc: Document the Amlogic Meson SDHC MMC host controller

[Martin Blumenstingl]

This documents the devicetree bindings for the SDHC MMC host controller
found in Meson6, Meson8, Meson8b and Meson8m2 SoCs. It can use a
bus-width of 1/4/8-bit and it supports eMMC spec 4.4x/4.5x including
HS200 mode (up to 100MHz clock). It embeds an internal clock controller
which outputs four clocks (mod_clk, sd_clk, tx_clk and rx_clk) and is
fed by four external input clocks (clkin[0-3]). "pclk" is the module
register clock, it has to be enabled to access the registers.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 .../bindings/mmc/amlogic,meson-mx-sdhc.yaml   | 83 +++++++++++++++++++
 .../dt-bindings/clock/meson-mx-sdhc-clkc.h    |  8 ++
 2 files changed, 91 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mmc/amlogic,meson-mx-sdhc.yaml
 create mode 100644 include/dt-bindings/clock/meson-mx-sdhc-clkc.h

diff --git a/Documentation/devicetree/bindings/mmc/amlogic,meson-mx-sdhc.yaml b/Documentation/devicetree/bindings/mmc/amlogic,meson-mx-sdhc.yaml
new file mode 100644
index 000000000000..1ca44529f622
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/amlogic,meson-mx-sdhc.yaml
@@ -0,0 +1,83 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mmc/amlogic,meson-mx-sdhc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Amlogic Meson SDHC controller Device Tree Bindings
+
+allOf:
+  - $ref: "mmc-controller.yaml"
+
+maintainers:
+  - Martin Blumenstingl <martin.blumenstingl@googlemail.com>
+
+description: |
+  The SDHC MMC host controller on Amlogic SoCs provides an eMMC and MMC
+  card interface with 1/4/8-bit bus width.
+  It supports eMMC spec 4.4x/4.5x including HS200 (up to 100MHz clock).
+
+properties:
+  compatible:
+    items:
+      - enum:
+        - amlogic,meson8-sdhc
+        - amlogic,meson8b-sdhc
+        - amlogic,meson8m2-sdhc
+      - const: amlogic,meson-mx-sdhc
+
+  reg:
+    minItems: 1
+
+  interrupts:
+    minItems: 1
+
+  "#clock-cells":
+    const: 1
+
+  clocks:
+    minItems: 9
+
+  clock-names:
+    items:
+      - const: clkin0
+      - const: clkin1
+      - const: clkin2
+      - const: clkin3
+      - const: pclk
+      - const: mod_clk
+      - const: sd_clk
+      - const: tx_clk
+      - const: rx_clk
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - "#clock-cells"
+  - clocks
+  - clock-names
+
+examples:
+  - |
+    #include <dt-bindings/clock/meson-mx-sdhc-clkc.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    sdhc: mmc@8e00 {
+      compatible = "amlogic,meson8-sdhc", "amlogic,meson-mx-sdhc";
+      reg = <0x8e00 0x42>;
+      interrupts = <GIC_SPI 78 IRQ_TYPE_EDGE_RISING>;
+      #clock-cells = <1>;
+      clocks = <&xtal>,
+               <&fclk_div4>,
+               <&fclk_div3>,
+               <&fclk_div5>,
+               <&sdhc_pclk>,
+               <&sdhc SDHC_CLKID_MOD_CLK>,
+               <&sdhc SDHC_CLKID_SD_CLK>,
+               <&sdhc SDHC_CLKID_TX_CLK>,
+               <&sdhc SDHC_CLKID_RX_CLK>;
+      clock-names = "clkin0", "clkin1", "clkin2", "clkin3", "pclk",
+                    "mod_clk", "sd_clk", "tx_clk", "rx_clk";
+    };
diff --git a/include/dt-bindings/clock/meson-mx-sdhc-clkc.h b/include/dt-bindings/clock/meson-mx-sdhc-clkc.h
new file mode 100644
index 000000000000..ad9f6e4dc426
--- /dev/null
+++ b/include/dt-bindings/clock/meson-mx-sdhc-clkc.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#define SDHC_CLKID_SRC_SEL	0
+#define SDHC_CLKID_DIV		1
+#define SDHC_CLKID_MOD_CLK	2
+#define SDHC_CLKID_SD_CLK	3
+#define SDHC_CLKID_TX_CLK	4
+#define SDHC_CLKID_RX_CLK	5
-- 
2.26.0

[PATCH v5 2/3] clk: meson: add a driver for the Meson8/8b/8m2 SDHC clock controller

[Martin Blumenstingl]

The SDHC (MMC) controller embeds a clock controller inside one of the
SDHC registers. The outputs of thisclock controller are dedicated to the
SDHC controller.

Implement a dedicated clock controller driver so we can keep all the
clock specific logic outside of the MMC controller driver. There is no
dedicated clock controller OF node because the hardware is a big SDHC IP
block with an embedded clock controller (so the .dts doesn't need a
separate clock controller node). Instead this driver re-uses the regmap
as registered by the (platform_device) parent.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/clk/meson/Kconfig         |   9 ++
 drivers/clk/meson/Makefile        |   1 +
 drivers/clk/meson/meson-mx-sdhc.c | 212 ++++++++++++++++++++++++++++++
 3 files changed, 222 insertions(+)
 create mode 100644 drivers/clk/meson/meson-mx-sdhc.c

diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
index dabeb435d067..8769335d2d46 100644
--- a/drivers/clk/meson/Kconfig
+++ b/drivers/clk/meson/Kconfig
@@ -53,6 +53,15 @@ config COMMON_CLK_MESON8B
 	  S805 (Meson8b) and S812 (Meson8m2) devices. Say Y if you
 	  want peripherals and CPU frequency scaling to work.
 
+config COMMON_CLK_MESON_MX_SDHC
+	tristate "Meson MX SDHC MMC Clock Controller Driver"
+	depends on ARCH_MESON
+	select COMMON_CLK_MESON_REGMAP
+	help
+	  Support for the SDHC clock controller on Amlogic Meson8/8b/8m2 SoCs
+	  devices. Say Y or M if you want to use the SDHC MMC controller.
+	  Otherwise say N.
+
 config COMMON_CLK_GXBB
 	bool
 	depends on ARCH_MESON
diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
index 6eca2a406ee3..b71c7ae78dbd 100644
--- a/drivers/clk/meson/Makefile
+++ b/drivers/clk/meson/Makefile
@@ -19,3 +19,4 @@ obj-$(CONFIG_COMMON_CLK_AXG_AUDIO) += axg-audio.o
 obj-$(CONFIG_COMMON_CLK_GXBB) += gxbb.o gxbb-aoclk.o
 obj-$(CONFIG_COMMON_CLK_G12A) += g12a.o g12a-aoclk.o
 obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o meson8-ddr.o
+obj-$(CONFIG_COMMON_CLK_MESON_MX_SDHC) += meson-mx-sdhc.o
diff --git a/drivers/clk/meson/meson-mx-sdhc.c b/drivers/clk/meson/meson-mx-sdhc.c
new file mode 100644
index 000000000000..b98a35d99f65
--- /dev/null
+++ b/drivers/clk/meson/meson-mx-sdhc.c
@@ -0,0 +1,212 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Amlogic Meson SDHC clock controller
+ *
+ * Copyright (C) 2019 Martin Blumenstingl <martin.blumenstingl@googlemail.com>
+ */
+
+#include <dt-bindings/clock/meson-mx-sdhc-clkc.h>
+
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#include "clk-regmap.h"
+#include "clk-pll.h"
+
+#define MESON_SDHC_CLKC			0x10
+
+static const struct clk_regmap meson_mx_sdhc_src_sel = {
+	.data = &(struct clk_regmap_mux_data){
+		.offset = MESON_SDHC_CLKC,
+		.mask = 0x3,
+		.shift = 16,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "sdhc_src_sel",
+		.ops = &clk_regmap_mux_ops,
+		.parent_data = (const struct clk_parent_data[]) {
+			{ .fw_name = "clkin0", .index = -1, },
+			{ .fw_name = "clkin1", .index = -1, },
+			{ .fw_name = "clkin2", .index = -1, },
+			{ .fw_name = "clkin3", .index = -1, },
+		},
+		.num_parents = 4,
+	},
+};
+
+static const struct clk_div_table meson_mx_sdhc_div_table[] = {
+	{ .div = 6, .val = 5, },
+	{ .div = 8, .val = 7, },
+	{ .div = 9, .val = 8, },
+	{ .div = 10, .val = 9, },
+	{ .div = 12, .val = 11, },
+	{ .div = 16, .val = 15, },
+	{ .div = 18, .val = 17, },
+	{ .div = 34, .val = 33, },
+	{ .div = 142, .val = 141, },
+	{ .div = 850, .val = 849, },
+	{ .div = 2126, .val = 2125, },
+	{ .div = 4096, .val = 4095, },
+	{ /* sentinel */ }
+};
+
+static const struct clk_regmap meson_mx_sdhc_div = {
+	.data = &(struct clk_regmap_div_data){
+		.offset = MESON_SDHC_CLKC,
+		.shift = 0,
+		.width = 12,
+		.table = meson_mx_sdhc_div_table,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "sdhc_div",
+		.ops = &clk_regmap_divider_ops,
+		.parent_data = (const struct clk_parent_data[]) {
+			{ .name = "sdhc_src_sel", .index = -1, },
+		},
+		.num_parents = 1,
+		.flags = CLK_SET_RATE_PARENT,
+	},
+};
+
+static const struct clk_regmap meson_mx_sdhc_mod_clk_en = {
+	.data = &(struct clk_regmap_gate_data){
+		.offset = MESON_SDHC_CLKC,
+		.bit_idx = 15,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "sdhc_mod_clk_on",
+		.ops = &clk_regmap_gate_ops,
+		.parent_data = (const struct clk_parent_data[]) {
+			{ .name = "sdhc_div", .index = -1, },
+		},
+		.num_parents = 1,
+		.flags = CLK_SET_RATE_GATE,
+	},
+};
+
+static const struct clk_regmap meson_mx_sdhc_tx_clk_en = {
+	.data = &(struct clk_regmap_gate_data){
+		.offset = MESON_SDHC_CLKC,
+		.bit_idx = 14,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "sdhc_tx_clk_on",
+		.ops = &clk_regmap_gate_ops,
+		.parent_data = (const struct clk_parent_data[]) {
+			{ .name = "sdhc_div", .index = -1, },
+		},
+		.num_parents = 1,
+		.flags = CLK_SET_RATE_GATE,
+	},
+};
+
+static const struct clk_regmap meson_mx_sdhc_rx_clk_en = {
+	.data = &(struct clk_regmap_gate_data){
+		.offset = MESON_SDHC_CLKC,
+		.bit_idx = 13,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "sdhc_rx_clk_on",
+		.ops = &clk_regmap_gate_ops,
+		.parent_data = (const struct clk_parent_data[]) {
+			{ .name = "sdhc_div", .index = -1, },
+		},
+		.num_parents = 1,
+		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
+	},
+};
+
+static const struct clk_regmap meson_mx_sdhc_sd_clk_en = {
+	.data = &(struct clk_regmap_gate_data){
+		.offset = MESON_SDHC_CLKC,
+		.bit_idx = 12,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "sdhc_sd_clk_on",
+		.ops = &clk_regmap_gate_ops,
+		.parent_data = (const struct clk_parent_data[]) {
+			{ .name = "sdhc_div", .index = -1, },
+		},
+		.num_parents = 1,
+		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
+	},
+};
+
+static const struct clk_regmap *meson_mx_sdhc_clk_regmaps[] = {
+	[SDHC_CLKID_SRC_SEL]	= &meson_mx_sdhc_src_sel,
+	[SDHC_CLKID_DIV]	= &meson_mx_sdhc_div,
+	[SDHC_CLKID_MOD_CLK]	= &meson_mx_sdhc_mod_clk_en,
+	[SDHC_CLKID_SD_CLK]	= &meson_mx_sdhc_sd_clk_en,
+	[SDHC_CLKID_TX_CLK]	= &meson_mx_sdhc_tx_clk_en,
+	[SDHC_CLKID_RX_CLK]	= &meson_mx_sdhc_rx_clk_en,
+};
+
+#define MESON_MX_SDHC_NUM_CLKS ARRAY_SIZE(meson_mx_sdhc_clk_regmaps)
+
+static int meson_mx_sdhc_clkc_probe(struct platform_device *pdev)
+{
+	struct device *parent = pdev->dev.parent;
+	struct clk_hw_onecell_data *onecell_data;
+	struct clk_regmap *clk_regmap;
+	struct regmap *regmap;
+	int i, ret;
+
+	regmap = dev_get_regmap(parent, NULL);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	clk_regmap = devm_kcalloc(parent, sizeof(*clk_regmap),
+				  MESON_MX_SDHC_NUM_CLKS, GFP_KERNEL);
+	if (!clk_regmap)
+		return -ENOMEM;
+
+	onecell_data = devm_kzalloc(parent,
+				    struct_size(onecell_data, hws,
+						MESON_MX_SDHC_NUM_CLKS),
+				    GFP_KERNEL);
+	if (!onecell_data)
+		return -ENOMEM;
+
+	for (i = 0; i < MESON_MX_SDHC_NUM_CLKS; i++) {
+		memcpy(&clk_regmap[i], meson_mx_sdhc_clk_regmaps[i],
+		       sizeof(*clk_regmap));
+
+		clk_regmap[i].map = regmap;
+		onecell_data->hws[i] = &clk_regmap[i].hw;
+
+		ret = devm_clk_hw_register(parent, onecell_data->hws[i]);
+		if (ret) {
+			dev_err(parent,
+				"Registration of SDHC clock %d failed\n", i);
+			return ret;
+		}
+	}
+
+	onecell_data->num = MESON_MX_SDHC_NUM_CLKS;
+
+	return devm_of_clk_add_hw_provider(parent, of_clk_hw_onecell_get,
+					   onecell_data);
+}
+
+static const struct platform_device_id meson_mx_sdhc_clkc_ids[] = {
+	{ "meson8-sdhc-clkc" },
+	{ "meson8b-sdhc-clkc" },
+	{ "meson8m2-sdhc-clkc" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(platform, meson_mx_sdhc_clkc_ids);
+
+static struct platform_driver meson_mx_sdhc_clkc_driver = {
+	.id_table	= meson_mx_sdhc_clkc_ids,
+	.probe		= meson_mx_sdhc_clkc_probe,
+	.driver		= {
+		.name	= "meson-mx-sdhc-clkc",
+	},
+};
+module_platform_driver(meson_mx_sdhc_clkc_driver);
+
+MODULE_DESCRIPTION("Amlogic Meson8/8b/8m2 SDHC clock controller driver");
+MODULE_AUTHOR("Martin Blumenstingl <martin.blumenstingl@googlemail.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.26.0

[PATCH v5 3/3] mmc: host: meson-mx-sdhc: new driver for the Amlogic Meson SDHC host

[Martin Blumenstingl]

The SDHC MMC host controller on Amlogic SoCs provides an eMMC and MMC
card interface with 1/4/8-bit bus width.
It supports eMMC spec 4.4x/4.5x including HS200 (up to 100MHz clock).

The public S805 datasheet [0] contains a short documentation about the
registers. Unfortunately it does not describe how to use the registers
to make the hardware work. Thus this driver is based on reading (and
understanding) the Amlogic 3.10 GPL kernel code.

Some hardware details are not easy to see. Jianxin Pan was kind enough
to answer my questions:
The hardware has built-in busy timeout support. The maximum timeout is
30 seconds. This is only documented in Amlogic's internal
documentation.

The controller only works with very specific clock configurations. The
details are not part of the public datasheet. In my own words the
supported configurations are:
- 399.812kHz:	clkin =  850MHz div = 2126 sd_rx_phase = 63
- 1MHz:		clkin =  850MHz div = 850  sd_rx_phase = 55
- 5.986MHz:	clkin =  850MHz div = 142  sd_rx_phase = 24
- 25MHz:	clkin =  850MHz div = 34   sd_rx_phase = 15
- 47.222MHz:	clkin =  850MHz div = 18   sd_rx_phase = 11/15 (SDR50/HS)
- 53.125MHz:	clkin =  850MHz div = 16   sd_rx_phase = (tuning)
- 70.833MHz:	clkin =  850MHz div = 12   sd_rx_phase = (tuning)
- 85MHz:	clkin =  850MHz div = 10   sd_rx_phase = (tuning)
- 94.44MHz:	clkin =  850MHz div = 9    sd_rx_phase = (tuning)
- 106.25MHz:	clkin =  850MHz div = 8    sd_rx_phase = (tuning)
- 127.5MHz:     clkin = 1275MHz div = 10   sd_rx_phase = (tuning)
- 141.667MHz:   clkin =  850MHz div = 6    sd_rx_phase = (tuning)
- 159.375MHz:	clkin = 1275MHz div = 8    sd_rx_phase = (tuning)
- 212.5MHz:	clkin = 1275MHz div = 6    sd_rx_phase = (tuning)
- (sd_tx_phase is always 1, 94.44MHz is not listed in the datasheet
   but this is what the 3.10 BSP kernel on Odroid-C1 actually uses)

NOTE: CMD23 support is disabled for now because it results in command
timeouts and thus decreases read performance.

Tested-by: Wei Wang <lnykww@gmail.com>
Tested-by: Xin Yin <yinxin_1989@aliyun.com>
Reviewed-by: Xin Yin <yinxin_1989@aliyun.com>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/mmc/host/Kconfig         |   14 +
 drivers/mmc/host/Makefile        |    1 +
 drivers/mmc/host/meson-mx-sdhc.c | 1064 ++++++++++++++++++++++++++++++
 3 files changed, 1079 insertions(+)
 create mode 100644 drivers/mmc/host/meson-mx-sdhc.c

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 3a5089f0332c..2c8361aa7eed 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -405,6 +405,20 @@ config MMC_MESON_GX
 
 	  If you have a controller with this interface, say Y here.
 
+config MMC_MESON_MX_SDHC
+	tristate "Amlogic Meson SDHC Host Controller support"
+	depends on (ARM && ARCH_MESON) || COMPILE_TEST
+	depends on OF
+	imply COMMON_CLK_MESON_MX_SDHC
+	help
+	  This selects support for the SDHC Host Controller on
+	  Amlogic Meson6, Meson8, Meson8b and Meson8m2 SoCs.
+	  The controller supports the SD/SDIO Spec 3.x and eMMC Spec 4.5x
+	  with 1, 4, and 8 bit bus widths.
+
+	  If you have a controller with this interface, say Y or M here.
+	  If unsure, say N.
+
 config MMC_MESON_MX_SDIO
 	tristate "Amlogic Meson6/Meson8/Meson8b SD/MMC Host Controller support"
 	depends on ARCH_MESON || COMPILE_TEST
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 21d9089e5eda..9a0c22a8139a 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -68,6 +68,7 @@ obj-$(CONFIG_MMC_VUB300)	+= vub300.o
 obj-$(CONFIG_MMC_USHC)		+= ushc.o
 obj-$(CONFIG_MMC_WMT)		+= wmt-sdmmc.o
 obj-$(CONFIG_MMC_MESON_GX)	+= meson-gx-mmc.o
+obj-$(CONFIG_MMC_MESON_MX_SDHC)	+= meson-mx-sdhc.o
 obj-$(CONFIG_MMC_MESON_MX_SDIO)	+= meson-mx-sdio.o
 obj-$(CONFIG_MMC_MOXART)	+= moxart-mmc.o
 obj-$(CONFIG_MMC_SUNXI)		+= sunxi-mmc.o
diff --git a/drivers/mmc/host/meson-mx-sdhc.c b/drivers/mmc/host/meson-mx-sdhc.c
new file mode 100644
index 000000000000..b92400ee488b
--- /dev/null
+++ b/drivers/mmc/host/meson-mx-sdhc.c
@@ -0,0 +1,1064 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Amlogic Meson6/Meson8/Meson8b/Meson8m2 SDHC MMC host controller driver.
+ *
+ * Copyright (C) 2019 Martin Blumenstingl <martin.blumenstingl@googlemail.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+#include <linux/interrupt.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/types.h>
+
+#include <linux/mmc/host.h>
+#include <linux/mmc/mmc.h>
+#include <linux/mmc/sdio.h>
+#include <linux/mmc/slot-gpio.h>
+
+#define MESON_SDHC_ARGU						0x00
+
+#define MESON_SDHC_SEND						0x04
+	#define MESON_SDHC_SEND_CMD_INDEX			GENMASK(5, 0)
+	#define MESON_SDHC_SEND_CMD_HAS_RESP			BIT(6)
+	#define MESON_SDHC_SEND_CMD_HAS_DATA			BIT(7)
+	#define MESON_SDHC_SEND_RESP_LEN			BIT(8)
+	#define MESON_SDHC_SEND_RESP_NO_CRC			BIT(9)
+	#define MESON_SDHC_SEND_DATA_DIR			BIT(10)
+	#define MESON_SDHC_SEND_DATA_STOP			BIT(11)
+	#define MESON_SDHC_SEND_R1B				BIT(12)
+	#define MESON_SDHC_SEND_TOTAL_PACK			GENMASK(31, 16)
+
+#define MESON_SDHC_CTRL						0x08
+	#define MESON_SDHC_CTRL_DAT_TYPE			GENMASK(1, 0)
+	#define MESON_SDHC_CTRL_DDR_MODE			BIT(2)
+	#define MESON_SDHC_CTRL_TX_CRC_NOCHECK			BIT(3)
+	#define MESON_SDHC_CTRL_PACK_LEN			GENMASK(12, 4)
+	#define MESON_SDHC_CTRL_RX_TIMEOUT			GENMASK(19, 13)
+	#define MESON_SDHC_CTRL_RX_PERIOD			GENMASK(23, 20)
+	#define MESON_SDHC_CTRL_RX_ENDIAN			GENMASK(26, 24)
+	#define MESON_SDHC_CTRL_SDIO_IRQ_MODE			BIT(27)
+	#define MESON_SDHC_CTRL_DAT0_IRQ_SEL			BIT(28)
+	#define MESON_SDHC_CTRL_TX_ENDIAN			GENMASK(31, 29)
+
+#define MESON_SDHC_STAT						0x0c
+	#define MESON_SDHC_STAT_CMD_BUSY			BIT(0)
+	#define MESON_SDHC_STAT_DAT3_0				GENMASK(4, 1)
+	#define MESON_SDHC_STAT_CMD				BIT(5)
+	#define MESON_SDHC_STAT_RXFIFO_CNT			GENMASK(12, 6)
+	#define MESON_SDHC_STAT_TXFIFO_CNT			GENMASK(19, 13)
+	#define MESON_SDHC_STAT_DAT7_4				GENMASK(23, 20)
+
+#define MESON_SDHC_CLKC						0x10
+	#define MESON_SDHC_CLKC_CLK_DIV				GENMASK(11, 0)
+	#define MESON_SDHC_CLKC_CLK_JIC				BIT(24)
+	#define MESON_SDHC_CLKC_MEM_PWR_OFF			GENMASK(26, 25)
+
+#define MESON_SDHC_ADDR						0x14
+
+#define MESON_SDHC_PDMA						0x18
+	#define MESON_SDHC_PDMA_DMA_MODE			BIT(0)
+	#define MESON_SDHC_PDMA_PIO_RDRESP			GENMASK(3, 1)
+	#define MESON_SDHC_PDMA_DMA_URGENT			BIT(4)
+	#define MESON_SDHC_PDMA_WR_BURST			GENMASK(9, 5)
+	#define MESON_SDHC_PDMA_RD_BURST			GENMASK(14, 10)
+	#define MESON_SDHC_PDMA_RXFIFO_TH			GENMASK(21, 15)
+	#define MESON_SDHC_PDMA_TXFIFO_TH			GENMASK(28, 22)
+	#define MESON_SDHC_PDMA_RXFIFO_MANUAL_FLUSH		GENMASK(30, 29)
+	#define MESON_SDHC_PDMA_TXFIFO_FILL			BIT(31)
+
+#define MESON_SDHC_MISC						0x1c
+	#define MESON_SDHC_MISC_WCRC_ERR_PATT			GENMASK(6, 4)
+	#define MESON_SDHC_MISC_WCRC_OK_PATT			GENMASK(9, 7)
+	#define MESON_SDHC_MISC_BURST_NUM			GENMASK(21, 16)
+	#define MESON_SDHC_MISC_THREAD_ID			GENMASK(27, 22)
+	#define MESON_SDHC_MISC_MANUAL_STOP			BIT(28)
+	#define MESON_SDHC_MISC_TXSTART_THRES			GENMASK(31, 29)
+
+#define MESON_SDHC_DATA						0x20
+
+#define MESON_SDHC_ICTL						0x24
+	#define MESON_SDHC_ICTL_RESP_OK				BIT(0)
+	#define MESON_SDHC_ICTL_RESP_TIMEOUT			BIT(1)
+	#define MESON_SDHC_ICTL_RESP_ERR_CRC			BIT(2)
+	#define MESON_SDHC_ICTL_RESP_OK_NOCLEAR			BIT(3)
+	#define MESON_SDHC_ICTL_DATA_1PACK_OK			BIT(4)
+	#define MESON_SDHC_ICTL_DATA_TIMEOUT			BIT(5)
+	#define MESON_SDHC_ICTL_DATA_ERR_CRC			BIT(6)
+	#define MESON_SDHC_ICTL_DATA_XFER_OK			BIT(7)
+	#define MESON_SDHC_ICTL_RX_HIGHER			BIT(8)
+	#define MESON_SDHC_ICTL_RX_LOWER			BIT(9)
+	#define MESON_SDHC_ICTL_DAT1_IRQ			BIT(10)
+	#define MESON_SDHC_ICTL_DMA_DONE			BIT(11)
+	#define MESON_SDHC_ICTL_RXFIFO_FULL			BIT(12)
+	#define MESON_SDHC_ICTL_TXFIFO_EMPTY			BIT(13)
+	#define MESON_SDHC_ICTL_ADDI_DAT1_IRQ			BIT(14)
+	#define MESON_SDHC_ICTL_ALL_IRQS			GENMASK(14, 0)
+	#define MESON_SDHC_ICTL_DAT1_IRQ_DELAY			GENMASK(17, 16)
+
+#define MESON_SDHC_ISTA						0x28
+	#define MESON_SDHC_ISTA_RESP_OK				BIT(0)
+	#define MESON_SDHC_ISTA_RESP_TIMEOUT			BIT(1)
+	#define MESON_SDHC_ISTA_RESP_ERR_CRC			BIT(2)
+	#define MESON_SDHC_ISTA_RESP_OK_NOCLEAR			BIT(3)
+	#define MESON_SDHC_ISTA_DATA_1PACK_OK			BIT(4)
+	#define MESON_SDHC_ISTA_DATA_TIMEOUT			BIT(5)
+	#define MESON_SDHC_ISTA_DATA_ERR_CRC			BIT(6)
+	#define MESON_SDHC_ISTA_DATA_XFER_OK			BIT(7)
+	#define MESON_SDHC_ISTA_RX_HIGHER			BIT(8)
+	#define MESON_SDHC_ISTA_RX_LOWER			BIT(9)
+	#define MESON_SDHC_ISTA_DAT1_IRQ			BIT(10)
+	#define MESON_SDHC_ISTA_DMA_DONE			BIT(11)
+	#define MESON_SDHC_ISTA_RXFIFO_FULL			BIT(12)
+	#define MESON_SDHC_ISTA_TXFIFO_EMPTY			BIT(13)
+	#define MESON_SDHC_ISTA_ADDI_DAT1_IRQ			BIT(14)
+	#define MESON_SDHC_ISTA_ALL_IRQS			GENMASK(14, 0)
+
+#define MESON_SDHC_SRST						0x2c
+	#define MESON_SDHC_SRST_MAIN_CTRL			BIT(0)
+	#define MESON_SDHC_SRST_RXFIFO				BIT(1)
+	#define MESON_SDHC_SRST_TXFIFO				BIT(2)
+	#define MESON_SDHC_SRST_DPHY_RX				BIT(3)
+	#define MESON_SDHC_SRST_DPHY_TX				BIT(4)
+	#define MESON_SDHC_SRST_DMA_IF				BIT(5)
+
+#define MESON_SDHC_ESTA						0x30
+	#define MESON_SDHC_ESTA_11_13				GENMASK(13, 11)
+
+#define MESON_SDHC_ENHC						0x34
+	#define MESON_SDHC_ENHC_MESON8M2_WRRSP_MODE		BIT(0)
+	#define MESON_SDHC_ENHC_MESON8M2_CHK_WRRSP		BIT(1)
+	#define MESON_SDHC_ENHC_MESON8M2_CHK_DMA		BIT(2)
+	#define MESON_SDHC_ENHC_MESON8M2_DEBUG			GENMASK(5, 3)
+	#define MESON_SDHC_ENHC_MESON6_RX_TIMEOUT		GENMASK(7, 0)
+	#define MESON_SDHC_ENHC_MESON6_DMA_RD_RESP		BIT(16)
+	#define MESON_SDHC_ENHC_MESON6_DMA_WR_RESP		BIT(17)
+	#define MESON_SDHC_ENHC_SDIO_IRQ_PERIOD			GENMASK(15, 8)
+	#define MESON_SDHC_ENHC_RXFIFO_TH			GENMASK(24, 18)
+	#define MESON_SDHC_ENHC_TXFIFO_TH			GENMASK(31, 25)
+
+#define MESON_SDHC_CLK2						0x38
+	#define MESON_SDHC_CLK2_RX_CLK_PHASE			GENMASK(11, 0)
+	#define MESON_SDHC_CLK2_SD_CLK_PHASE			GENMASK(23, 12)
+
+#define MESON_SDHC_NUM_BULK_CLKS				4
+#define MESON_SDHC_MAX_BLK_SIZE					512
+#define MESON_SDHC_NUM_TUNING_TRIES				10
+
+struct meson_mx_sdhc_data {
+	void		(*init_hw)(struct mmc_host *mmc);
+	void		(*set_pdma)(struct mmc_host *mmc);
+	void		(*wait_before_send)(struct mmc_host *mmc);
+	bool		hardware_flush_all_cmds;
+	const char	*clkc_name;
+};
+
+struct meson_mx_sdhc_host {
+	struct mmc_host			*mmc;
+
+	struct mmc_request		*mrq;
+	struct mmc_command		*cmd;
+	int				error;
+
+	struct regmap			*regmap;
+
+	struct platform_device		*clkc_pdev;
+	struct clk			*pclk;
+	struct clk			*sd_clk;
+	struct clk_bulk_data		bulk_clks[MESON_SDHC_NUM_BULK_CLKS];
+	bool				bulk_clks_enabled;
+
+	const struct meson_mx_sdhc_data	*platform;
+};
+
+static const struct regmap_config meson_mx_sdhc_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.max_register = MESON_SDHC_CLK2,
+};
+
+static void meson_mx_sdhc_hw_reset(struct mmc_host *mmc)
+{
+	struct meson_mx_sdhc_host *host = mmc_priv(mmc);
+
+	regmap_write(host->regmap, MESON_SDHC_SRST, MESON_SDHC_SRST_MAIN_CTRL |
+		     MESON_SDHC_SRST_RXFIFO | MESON_SDHC_SRST_TXFIFO |
+		     MESON_SDHC_SRST_DPHY_RX | MESON_SDHC_SRST_DPHY_TX |
+		     MESON_SDHC_SRST_DMA_IF);
+	usleep_range(10, 100);
+
+	regmap_write(host->regmap, MESON_SDHC_SRST, 0);
+	usleep_range(10, 100);
+}
+
+static void meson_mx_sdhc_clear_fifo(struct mmc_host *mmc)
+{
+	struct meson_mx_sdhc_host *host = mmc_priv(mmc);
+	u32 stat;
+
+	regmap_read(host->regmap, MESON_SDHC_STAT, &stat);
+	if (!FIELD_GET(MESON_SDHC_STAT_RXFIFO_CNT, stat) &&
+	    !FIELD_GET(MESON_SDHC_STAT_TXFIFO_CNT, stat))
+		return;
+
+	regmap_write(host->regmap, MESON_SDHC_SRST, MESON_SDHC_SRST_RXFIFO |
+		     MESON_SDHC_SRST_TXFIFO | MESON_SDHC_SRST_MAIN_CTRL);
+	udelay(5);
+
+	regmap_read(host->regmap, MESON_SDHC_STAT, &stat);
+	if (FIELD_GET(MESON_SDHC_STAT_RXFIFO_CNT, stat) ||
+	    FIELD_GET(MESON_SDHC_STAT_TXFIFO_CNT, stat))
+		dev_warn(mmc_dev(host->mmc),
+			 "Failed to clear FIFOs, RX: %lu, TX: %lu\n",
+			 FIELD_GET(MESON_SDHC_STAT_RXFIFO_CNT, stat),
+			 FIELD_GET(MESON_SDHC_STAT_TXFIFO_CNT, stat));
+}
+
+static void meson_mx_sdhc_wait_cmd_ready(struct mmc_host *mmc)
+{
+	struct meson_mx_sdhc_host *host = mmc_priv(mmc);
+	u32 stat, esta;
+	int ret;
+
+	ret = regmap_read_poll_timeout(host->regmap, MESON_SDHC_STAT, stat,
+				       !(stat & MESON_SDHC_STAT_CMD_BUSY), 1,
+				       100000);
+	if (ret) {
+		dev_warn(mmc_dev(mmc),
+			 "Failed to poll for CMD_BUSY while processing CMD%d\n",
+			 host->cmd->opcode);
+		meson_mx_sdhc_hw_reset(mmc);
+	}
+
+	ret = regmap_read_poll_timeout(host->regmap, MESON_SDHC_ESTA, esta,
+				       !(esta & MESON_SDHC_ESTA_11_13), 1,
+				       100000);
+	if (ret) {
+		dev_warn(mmc_dev(mmc),
+			 "Failed to poll for ESTA[13:11] while processing CMD%d\n",
+			 host->cmd->opcode);
+		meson_mx_sdhc_hw_reset(mmc);
+	}
+}
+
+static void meson_mx_sdhc_start_cmd(struct mmc_host *mmc,
+				    struct mmc_command *cmd)
+{
+	struct meson_mx_sdhc_host *host = mmc_priv(mmc);
+	u32 ictl, send;
+	int pack_len;
+
+	host->cmd = cmd;
+
+	ictl = MESON_SDHC_ICTL_DATA_TIMEOUT | MESON_SDHC_ICTL_DATA_ERR_CRC |
+	       MESON_SDHC_ICTL_RXFIFO_FULL | MESON_SDHC_ICTL_TXFIFO_EMPTY |
+	       MESON_SDHC_ICTL_RESP_TIMEOUT | MESON_SDHC_ICTL_RESP_ERR_CRC;
+
+	send = FIELD_PREP(MESON_SDHC_SEND_CMD_INDEX, cmd->opcode);
+
+	if (cmd->data) {
+		send |= MESON_SDHC_SEND_CMD_HAS_DATA;
+		send |= FIELD_PREP(MESON_SDHC_SEND_TOTAL_PACK,
+				   cmd->data->blocks - 1);
+
+		if (cmd->data->blksz < MESON_SDHC_MAX_BLK_SIZE)
+			pack_len = cmd->data->blksz;
+		else
+			pack_len = 0;
+
+		if (cmd->data->flags & MMC_DATA_WRITE)
+			send |= MESON_SDHC_SEND_DATA_DIR;
+
+		/*
+		 * If command with no data, just wait response done
+		 * interrupt(int[0]), and if command with data transfer, just
+		 * wait dma done interrupt(int[11]), don't need care about
+		 * dat0 busy or not.
+		 */
+		if (host->platform->hardware_flush_all_cmds ||
+		    cmd->data->flags & MMC_DATA_WRITE)
+			/* hardware flush: */
+			ictl |= MESON_SDHC_ICTL_DMA_DONE;
+		else
+			/* software flush: */
+			ictl |= MESON_SDHC_ICTL_DATA_XFER_OK;
+	} else {
+		pack_len = 0;
+
+		ictl |= MESON_SDHC_ICTL_RESP_OK;
+	}
+
+	if (cmd->opcode == MMC_STOP_TRANSMISSION)
+		send |= MESON_SDHC_SEND_DATA_STOP;
+
+	if (cmd->flags & MMC_RSP_PRESENT)
+		send |= MESON_SDHC_SEND_CMD_HAS_RESP;
+
+	if (cmd->flags & MMC_RSP_136) {
+		send |= MESON_SDHC_SEND_RESP_LEN;
+		send |= MESON_SDHC_SEND_RESP_NO_CRC;
+	}
+
+	if (!(cmd->flags & MMC_RSP_CRC))
+		send |= MESON_SDHC_SEND_RESP_NO_CRC;
+
+	if (cmd->flags & MMC_RSP_BUSY)
+		send |= MESON_SDHC_SEND_R1B;
+
+	/* enable the new IRQs and mask all pending ones */
+	regmap_write(host->regmap, MESON_SDHC_ICTL, ictl);
+	regmap_write(host->regmap, MESON_SDHC_ISTA, MESON_SDHC_ISTA_ALL_IRQS);
+
+	regmap_write(host->regmap, MESON_SDHC_ARGU, cmd->arg);
+
+	regmap_update_bits(host->regmap, MESON_SDHC_CTRL,
+			   MESON_SDHC_CTRL_PACK_LEN,
+			   FIELD_PREP(MESON_SDHC_CTRL_PACK_LEN, pack_len));
+
+	if (cmd->data)
+		regmap_write(host->regmap, MESON_SDHC_ADDR,
+			     sg_dma_address(cmd->data->sg));
+
+	meson_mx_sdhc_wait_cmd_ready(mmc);
+
+	if (cmd->data)
+		host->platform->set_pdma(mmc);
+
+	if (host->platform->wait_before_send)
+		host->platform->wait_before_send(mmc);
+
+	regmap_write(host->regmap, MESON_SDHC_SEND, send);
+}
+
+static void meson_mx_sdhc_disable_clks(struct mmc_host *mmc)
+{
+	struct meson_mx_sdhc_host *host = mmc_priv(mmc);
+
+	if (!host->bulk_clks_enabled)
+		return;
+
+	clk_bulk_disable_unprepare(MESON_SDHC_NUM_BULK_CLKS, host->bulk_clks);
+
+	host->bulk_clks_enabled = false;
+}
+
+static int meson_mx_sdhc_enable_clks(struct mmc_host *mmc)
+{
+	struct meson_mx_sdhc_host *host = mmc_priv(mmc);
+	int ret;
+
+	if (host->bulk_clks_enabled)
+		return 0;
+
+	ret = clk_bulk_prepare_enable(MESON_SDHC_NUM_BULK_CLKS,
+				      host->bulk_clks);
+	if (ret)
+		return ret;
+
+	host->bulk_clks_enabled = true;
+
+	return 0;
+}
+
+static int meson_mx_sdhc_set_clk(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+	struct meson_mx_sdhc_host *host = mmc_priv(mmc);
+	u32 rx_clk_phase;
+	int ret;
+
+	meson_mx_sdhc_disable_clks(mmc);
+
+	if (ios->clock) {
+		ret = clk_set_rate(host->sd_clk, ios->clock);
+		if (ret) {
+			dev_warn(mmc_dev(mmc),
+				 "Failed to set MMC clock to %uHz: %d\n",
+				 ios->clock, host->error);
+			return ret;
+		}
+
+		ret = meson_mx_sdhc_enable_clks(mmc);
+		if (ret)
+			return ret;
+
+		mmc->actual_clock = clk_get_rate(host->sd_clk);
+
+		/*
+		 * according to Amlogic the following latching points are
+		 * selected with empirical values, there is no (known) formula
+		 * to calculate these.
+		 */
+		if (mmc->actual_clock > 100000000) {
+			rx_clk_phase = 1;
+		} else if (mmc->actual_clock > 45000000) {
+			if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330)
+				rx_clk_phase = 15;
+			else
+				rx_clk_phase = 11;
+		} else if (mmc->actual_clock >= 25000000) {
+			rx_clk_phase = 15;
+		} else if (mmc->actual_clock > 5000000) {
+			rx_clk_phase = 23;
+		} else if (mmc->actual_clock > 1000000) {
+			rx_clk_phase = 55;
+		} else {
+			rx_clk_phase = 1061;
+		}
+
+		regmap_update_bits(host->regmap, MESON_SDHC_CLK2,
+				   MESON_SDHC_CLK2_RX_CLK_PHASE,
+				   FIELD_PREP(MESON_SDHC_CLK2_RX_CLK_PHASE,
+					      rx_clk_phase));
+	} else {
+		mmc->actual_clock = 0;
+	}
+
+	return 0;
+}
+
+static void meson_mx_sdhc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+	struct meson_mx_sdhc_host *host = mmc_priv(mmc);
+	unsigned short vdd = ios->vdd;
+
+	switch (ios->power_mode) {
+	case MMC_POWER_OFF:
+		vdd = 0;
+		/* fall through */
+
+	case MMC_POWER_UP:
+		if (!IS_ERR(mmc->supply.vmmc)) {
+			host->error = mmc_regulator_set_ocr(mmc,
+							    mmc->supply.vmmc,
+							    vdd);
+			if (host->error)
+				return;
+		}
+
+		break;
+
+	case MMC_POWER_ON:
+		break;
+	}
+
+	host->error = meson_mx_sdhc_set_clk(mmc, ios);
+	if (host->error)
+		return;
+
+	switch (ios->bus_width) {
+	case MMC_BUS_WIDTH_1:
+		regmap_update_bits(host->regmap, MESON_SDHC_CTRL,
+				   MESON_SDHC_CTRL_DAT_TYPE,
+				   FIELD_PREP(MESON_SDHC_CTRL_DAT_TYPE, 0));
+		break;
+
+	case MMC_BUS_WIDTH_4:
+		regmap_update_bits(host->regmap, MESON_SDHC_CTRL,
+				   MESON_SDHC_CTRL_DAT_TYPE,
+				   FIELD_PREP(MESON_SDHC_CTRL_DAT_TYPE, 1));
+		break;
+
+	case MMC_BUS_WIDTH_8:
+		regmap_update_bits(host->regmap, MESON_SDHC_CTRL,
+				   MESON_SDHC_CTRL_DAT_TYPE,
+				   FIELD_PREP(MESON_SDHC_CTRL_DAT_TYPE, 2));
+		break;
+
+	default:
+		dev_err(mmc_dev(mmc), "unsupported bus width: %d\n",
+			ios->bus_width);
+		host->error = -EINVAL;
+		return;
+	}
+}
+
+static int meson_mx_sdhc_map_dma(struct mmc_host *mmc, struct mmc_request *mrq)
+{
+	struct mmc_data *data = mrq->data;
+	int dma_len;
+
+	if (!data)
+		return 0;
+
+	dma_len = dma_map_sg(mmc_dev(mmc), data->sg, data->sg_len,
+			     mmc_get_dma_dir(data));
+	if (dma_len <= 0) {
+		dev_err(mmc_dev(mmc), "dma_map_sg failed\n");
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static void meson_mx_sdhc_request(struct mmc_host *mmc, struct mmc_request *mrq)
+{
+	struct meson_mx_sdhc_host *host = mmc_priv(mmc);
+	struct mmc_command *cmd = mrq->cmd;
+
+	if (!host->error)
+		host->error = meson_mx_sdhc_map_dma(mmc, mrq);
+
+	if (host->error) {
+		cmd->error = host->error;
+		mmc_request_done(mmc, mrq);
+		return;
+	}
+
+	host->mrq = mrq;
+
+	meson_mx_sdhc_start_cmd(mmc, mrq->cmd);
+}
+
+static int meson_mx_sdhc_card_busy(struct mmc_host *mmc)
+{
+	struct meson_mx_sdhc_host *host = mmc_priv(mmc);
+	u32 stat;
+
+	regmap_read(host->regmap, MESON_SDHC_STAT, &stat);
+	return FIELD_GET(MESON_SDHC_STAT_DAT3_0, stat) == 0;
+}
+
+static bool meson_mx_sdhc_tuning_point_matches(struct mmc_host *mmc,
+					       u32 opcode)
+{
+	unsigned int i, num_matches = 0;
+	int ret;
+
+	for (i = 0; i < MESON_SDHC_NUM_TUNING_TRIES; i++) {
+		ret = mmc_send_tuning(mmc, opcode, NULL);
+		if (!ret)
+			num_matches++;
+	}
+
+	return num_matches == MESON_SDHC_NUM_TUNING_TRIES;
+}
+
+static int meson_mx_sdhc_execute_tuning(struct mmc_host *mmc, u32 opcode)
+{
+	struct meson_mx_sdhc_host *host = mmc_priv(mmc);
+	int div, start, len, best_start, best_len;
+	int curr_phase, old_phase, new_phase;
+	u32 val;
+
+	len = 0;
+	start = 0;
+	best_len = 0;
+
+	regmap_read(host->regmap, MESON_SDHC_CLK2, &val);
+	old_phase = FIELD_GET(MESON_SDHC_CLK2_RX_CLK_PHASE, val);
+
+	regmap_read(host->regmap, MESON_SDHC_CLKC, &val);
+	div = FIELD_GET(MESON_SDHC_CLKC_CLK_DIV, val);
+
+	for (curr_phase = 0; curr_phase <= div; curr_phase++) {
+		regmap_update_bits(host->regmap, MESON_SDHC_CLK2,
+				   MESON_SDHC_CLK2_RX_CLK_PHASE,
+				   FIELD_PREP(MESON_SDHC_CLK2_RX_CLK_PHASE,
+					      curr_phase));
+
+		if (meson_mx_sdhc_tuning_point_matches(mmc, opcode)) {
+			if (!len) {
+				start = curr_phase;
+
+				dev_dbg(mmc_dev(mmc),
+					"New RX phase window starts at %u\n",
+					start);
+			}
+
+			len++;
+		} else {
+			if (len > best_len) {
+				best_start = start;
+				best_len = len;
+
+				dev_dbg(mmc_dev(mmc),
+					"New best RX phase window: %u - %u\n",
+					best_start, best_start + best_len);
+			}
+
+			/* reset the current window */
+			len = 0;
+		}
+	}
+
+	if (len > best_len)
+		/* the last window is the best (or possibly only) window */
+		new_phase = start + (len / 2);
+	else if (best_len)
+		/* there was a better window than the last */
+		new_phase = best_start + (best_len / 2);
+	else
+		/* no window was found at all, reset to the original phase */
+		new_phase = old_phase;
+
+	regmap_update_bits(host->regmap, MESON_SDHC_CLK2,
+			   MESON_SDHC_CLK2_RX_CLK_PHASE,
+			   FIELD_PREP(MESON_SDHC_CLK2_RX_CLK_PHASE,
+				      new_phase));
+
+	if (!len && !best_len)
+		return -EIO;
+
+	dev_dbg(mmc_dev(mmc), "Tuned RX clock phase to %u\n", new_phase);
+
+	return 0;
+}
+
+static const struct mmc_host_ops meson_mx_sdhc_ops = {
+	.hw_reset			= meson_mx_sdhc_hw_reset,
+	.request			= meson_mx_sdhc_request,
+	.set_ios			= meson_mx_sdhc_set_ios,
+	.card_busy			= meson_mx_sdhc_card_busy,
+	.execute_tuning			= meson_mx_sdhc_execute_tuning,
+	.get_cd				= mmc_gpio_get_cd,
+	.get_ro				= mmc_gpio_get_ro,
+};
+
+static void meson_mx_sdhc_request_done(struct meson_mx_sdhc_host *host)
+{
+	struct mmc_request *mrq = host->mrq;
+	struct mmc_host *mmc = host->mmc;
+
+	/* disable interrupts and mask all pending ones */
+	regmap_update_bits(host->regmap, MESON_SDHC_ICTL,
+			   MESON_SDHC_ICTL_ALL_IRQS, 0);
+	regmap_update_bits(host->regmap, MESON_SDHC_ISTA,
+			   MESON_SDHC_ISTA_ALL_IRQS, MESON_SDHC_ISTA_ALL_IRQS);
+
+	host->mrq = NULL;
+	host->cmd = NULL;
+
+	mmc_request_done(mmc, mrq);
+}
+
+static u32 meson_mx_sdhc_read_response(struct meson_mx_sdhc_host *host, u8 idx)
+{
+	u32 val;
+
+	regmap_update_bits(host->regmap, MESON_SDHC_PDMA,
+			   MESON_SDHC_PDMA_DMA_MODE, 0);
+
+	regmap_update_bits(host->regmap, MESON_SDHC_PDMA,
+			   MESON_SDHC_PDMA_PIO_RDRESP,
+			   FIELD_PREP(MESON_SDHC_PDMA_PIO_RDRESP, idx));
+
+	regmap_read(host->regmap, MESON_SDHC_ARGU, &val);
+
+	return val;
+}
+
+static irqreturn_t meson_mx_sdhc_irq(int irq, void *data)
+{
+	struct meson_mx_sdhc_host *host = data;
+	struct mmc_command *cmd = host->cmd;
+	u32 ictl, ista;
+
+	regmap_read(host->regmap, MESON_SDHC_ICTL, &ictl);
+	regmap_read(host->regmap, MESON_SDHC_ISTA, &ista);
+
+	if (!(ictl & ista))
+		return IRQ_NONE;
+
+	if (ista & MESON_SDHC_ISTA_RXFIFO_FULL ||
+	    ista & MESON_SDHC_ISTA_TXFIFO_EMPTY)
+		cmd->error = -EIO;
+	else if (ista & MESON_SDHC_ISTA_RESP_ERR_CRC)
+		cmd->error = -EILSEQ;
+	else if (ista & MESON_SDHC_ISTA_RESP_TIMEOUT)
+		cmd->error = -ETIMEDOUT;
+
+	if (cmd->data) {
+		if (ista & MESON_SDHC_ISTA_DATA_ERR_CRC)
+			cmd->data->error = -EILSEQ;
+		else if (ista & MESON_SDHC_ISTA_DATA_TIMEOUT)
+			cmd->data->error = -ETIMEDOUT;
+	}
+
+	if (cmd->error || (cmd->data && cmd->data->error))
+		dev_dbg(mmc_dev(host->mmc), "CMD%d error, ISTA: 0x%08x\n",
+			cmd->opcode, ista);
+
+	return IRQ_WAKE_THREAD;
+}
+
+static irqreturn_t meson_mx_sdhc_irq_thread(int irq, void *irq_data)
+{
+	struct meson_mx_sdhc_host *host = irq_data;
+	struct mmc_command *cmd;
+	u32 val;
+
+	cmd = host->cmd;
+	if (WARN_ON(!cmd))
+		return IRQ_HANDLED;
+
+	if (cmd->data && !cmd->data->error) {
+		if (!host->platform->hardware_flush_all_cmds &&
+		    cmd->data->flags & MMC_DATA_READ) {
+			meson_mx_sdhc_wait_cmd_ready(host->mmc);
+
+			val = FIELD_PREP(MESON_SDHC_PDMA_RXFIFO_MANUAL_FLUSH,
+					 2);
+			regmap_update_bits(host->regmap, MESON_SDHC_PDMA,
+					   MESON_SDHC_PDMA_RXFIFO_MANUAL_FLUSH,
+					   val);
+		}
+
+		dma_unmap_sg(mmc_dev(host->mmc), cmd->data->sg,
+			     cmd->data->sg_len, mmc_get_dma_dir(cmd->data));
+
+		cmd->data->bytes_xfered = cmd->data->blksz * cmd->data->blocks;
+	}
+
+	meson_mx_sdhc_wait_cmd_ready(host->mmc);
+
+	if (cmd->flags & MMC_RSP_136) {
+		cmd->resp[0] = meson_mx_sdhc_read_response(host, 4);
+		cmd->resp[1] = meson_mx_sdhc_read_response(host, 3);
+		cmd->resp[2] = meson_mx_sdhc_read_response(host, 2);
+		cmd->resp[3] = meson_mx_sdhc_read_response(host, 1);
+	} else {
+		cmd->resp[0] = meson_mx_sdhc_read_response(host, 0);
+	}
+
+	if (cmd->error == -EIO || cmd->error == -ETIMEDOUT)
+		meson_mx_sdhc_hw_reset(host->mmc);
+	else if (cmd->data)
+		/*
+		 * Clear the FIFOs after completing data transfers to prevent
+		 * corrupting data on write access. It's not clear why this is
+		 * needed (for reads and writes), but it mimics what the BSP
+		 * kernel did.
+		 */
+		meson_mx_sdhc_clear_fifo(host->mmc);
+
+	meson_mx_sdhc_request_done(host);
+
+	return IRQ_HANDLED;
+}
+
+static int meson_mx_sdhc_register_clkc(struct meson_mx_sdhc_host *host)
+{
+	struct device *dev = mmc_dev(host->mmc);
+	int ret;
+
+	host->clkc_pdev = platform_device_alloc(host->platform->clkc_name,
+						PLATFORM_DEVID_NONE);
+	if (!host->clkc_pdev) {
+		dev_err(dev, "Failed to allocate %s clock controller device\n",
+			host->platform->clkc_name);
+		return -ENOMEM;
+	}
+
+	host->clkc_pdev->dev.parent = dev;
+
+	ret = platform_device_add(host->clkc_pdev);
+	if (ret) {
+		dev_err(dev, "Failed to register %s clock controller device\n",
+			host->platform->clkc_name);
+		platform_device_put(host->clkc_pdev);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void meson_mx_sdhc_init_hw_meson8(struct mmc_host *mmc)
+{
+	struct meson_mx_sdhc_host *host = mmc_priv(mmc);
+
+	regmap_write(host->regmap, MESON_SDHC_MISC,
+		     FIELD_PREP(MESON_SDHC_MISC_TXSTART_THRES, 7) |
+		     FIELD_PREP(MESON_SDHC_MISC_WCRC_ERR_PATT, 5) |
+		     FIELD_PREP(MESON_SDHC_MISC_WCRC_OK_PATT, 2));
+
+	regmap_write(host->regmap, MESON_SDHC_ENHC,
+		     FIELD_PREP(MESON_SDHC_ENHC_RXFIFO_TH, 63) |
+		     MESON_SDHC_ENHC_MESON6_DMA_WR_RESP |
+		     FIELD_PREP(MESON_SDHC_ENHC_MESON6_RX_TIMEOUT, 255) |
+		     FIELD_PREP(MESON_SDHC_ENHC_SDIO_IRQ_PERIOD, 12));
+};
+
+static void meson_mx_sdhc_set_pdma_meson8(struct mmc_host *mmc)
+{
+	struct meson_mx_sdhc_host *host = mmc_priv(mmc);
+
+	if (host->cmd->data->flags & MMC_DATA_WRITE)
+		regmap_update_bits(host->regmap, MESON_SDHC_PDMA,
+				   MESON_SDHC_PDMA_DMA_MODE |
+				   MESON_SDHC_PDMA_RD_BURST |
+				   MESON_SDHC_PDMA_TXFIFO_FILL,
+				   MESON_SDHC_PDMA_DMA_MODE |
+				   FIELD_PREP(MESON_SDHC_PDMA_RD_BURST, 31) |
+				   MESON_SDHC_PDMA_TXFIFO_FILL);
+	else
+		regmap_update_bits(host->regmap, MESON_SDHC_PDMA,
+				   MESON_SDHC_PDMA_DMA_MODE |
+				   MESON_SDHC_PDMA_RXFIFO_MANUAL_FLUSH,
+				   MESON_SDHC_PDMA_DMA_MODE |
+				   FIELD_PREP(MESON_SDHC_PDMA_RXFIFO_MANUAL_FLUSH,
+					      1));
+
+	if (host->cmd->data->flags & MMC_DATA_WRITE)
+		regmap_update_bits(host->regmap, MESON_SDHC_PDMA,
+				   MESON_SDHC_PDMA_RD_BURST,
+				   FIELD_PREP(MESON_SDHC_PDMA_RD_BURST, 15));
+}
+
+static void meson_mx_sdhc_wait_before_send_meson8(struct mmc_host *mmc)
+{
+	struct meson_mx_sdhc_host *host = mmc_priv(mmc);
+	u32 val;
+	int ret;
+
+	ret = regmap_read_poll_timeout(host->regmap, MESON_SDHC_ESTA, val,
+				       val == 0, 1, 200);
+	if (ret)
+		dev_warn(mmc_dev(mmc),
+			 "Failed to wait for ESTA to clear: 0x%08x\n", val);
+
+	if (host->cmd->data && host->cmd->data->flags & MMC_DATA_WRITE) {
+		ret = regmap_read_poll_timeout(host->regmap, MESON_SDHC_STAT,
+					       val,
+					       val & MESON_SDHC_STAT_TXFIFO_CNT,
+					       1, 200);
+		if (ret)
+			dev_warn(mmc_dev(mmc),
+				 "Failed to wait for TX FIFO to fill\n");
+	}
+}
+
+static void meson_mx_sdhc_init_hw_meson8m2(struct mmc_host *mmc)
+{
+	struct meson_mx_sdhc_host *host = mmc_priv(mmc);
+
+	regmap_write(host->regmap, MESON_SDHC_MISC,
+		     FIELD_PREP(MESON_SDHC_MISC_TXSTART_THRES, 6) |
+		     FIELD_PREP(MESON_SDHC_MISC_WCRC_ERR_PATT, 5) |
+		     FIELD_PREP(MESON_SDHC_MISC_WCRC_OK_PATT, 2));
+
+	regmap_write(host->regmap, MESON_SDHC_ENHC,
+		     FIELD_PREP(MESON_SDHC_ENHC_RXFIFO_TH, 64) |
+		     FIELD_PREP(MESON_SDHC_ENHC_MESON8M2_DEBUG, 1) |
+		     MESON_SDHC_ENHC_MESON8M2_WRRSP_MODE |
+		     FIELD_PREP(MESON_SDHC_ENHC_SDIO_IRQ_PERIOD, 12));
+}
+
+static void meson_mx_sdhc_set_pdma_meson8m2(struct mmc_host *mmc)
+{
+	struct meson_mx_sdhc_host *host = mmc_priv(mmc);
+
+	regmap_update_bits(host->regmap, MESON_SDHC_PDMA,
+			   MESON_SDHC_PDMA_DMA_MODE, MESON_SDHC_PDMA_DMA_MODE);
+}
+
+static void meson_mx_sdhc_init_hw(struct mmc_host *mmc)
+{
+	struct meson_mx_sdhc_host *host = mmc_priv(mmc);
+
+	meson_mx_sdhc_hw_reset(mmc);
+
+	regmap_write(host->regmap, MESON_SDHC_CTRL,
+		     FIELD_PREP(MESON_SDHC_CTRL_RX_PERIOD, 0xf) |
+		     FIELD_PREP(MESON_SDHC_CTRL_RX_TIMEOUT, 0x7f) |
+		     FIELD_PREP(MESON_SDHC_CTRL_RX_ENDIAN, 0x7) |
+		     FIELD_PREP(MESON_SDHC_CTRL_TX_ENDIAN, 0x7));
+
+	/*
+	 * start with a valid divider and enable the memory (un-setting
+	 * MESON_SDHC_CLKC_MEM_PWR_OFF).
+	 */
+	regmap_write(host->regmap, MESON_SDHC_CLKC, MESON_SDHC_CLKC_CLK_DIV);
+
+	regmap_write(host->regmap, MESON_SDHC_CLK2,
+		     FIELD_PREP(MESON_SDHC_CLK2_SD_CLK_PHASE, 1));
+
+	regmap_write(host->regmap, MESON_SDHC_PDMA,
+		     MESON_SDHC_PDMA_DMA_URGENT |
+		     FIELD_PREP(MESON_SDHC_PDMA_WR_BURST, 7) |
+		     FIELD_PREP(MESON_SDHC_PDMA_TXFIFO_TH, 49) |
+		     FIELD_PREP(MESON_SDHC_PDMA_RD_BURST, 15) |
+		     FIELD_PREP(MESON_SDHC_PDMA_RXFIFO_TH, 7));
+
+	/* some initialization bits depend on the SoC: */
+	host->platform->init_hw(mmc);
+
+	/* disable and mask all interrupts: */
+	regmap_write(host->regmap, MESON_SDHC_ICTL, 0);
+	regmap_write(host->regmap, MESON_SDHC_ISTA, MESON_SDHC_ISTA_ALL_IRQS);
+}
+
+static int meson_mx_sdhc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct meson_mx_sdhc_host *host;
+	struct mmc_host *mmc;
+	void __iomem *base;
+	int ret, irq;
+
+	mmc = mmc_alloc_host(sizeof(*host), dev);
+	if (!mmc)
+		return -ENOMEM;
+
+	ret = devm_add_action_or_reset(dev, (void(*)(void *))mmc_free_host,
+				       mmc);
+	if (ret) {
+		dev_err(dev, "Failed to register mmc_free_host action\n");
+		return ret;
+	}
+
+	host = mmc_priv(mmc);
+	host->mmc = mmc;
+
+	platform_set_drvdata(pdev, host);
+
+	host->platform = device_get_match_data(dev);
+	if (!host->platform)
+		return -EINVAL;
+
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	host->regmap = devm_regmap_init_mmio(dev, base,
+					     &meson_mx_sdhc_regmap_config);
+	if (IS_ERR(host->regmap))
+		return PTR_ERR(host->regmap);
+
+	host->pclk = devm_clk_get(dev, "pclk");
+	if (IS_ERR(host->pclk))
+		return PTR_ERR(host->pclk);
+
+	/* accessing any register requires the module clock to be enabled: */
+	ret = clk_prepare_enable(host->pclk);
+	if (ret) {
+		dev_err(dev, "Failed to enable 'pclk' clock\n");
+		return ret;
+	}
+
+	meson_mx_sdhc_init_hw(mmc);
+
+	ret = meson_mx_sdhc_register_clkc(host);
+	if (ret)
+		goto err_disable_pclk;
+
+	host->bulk_clks[0].id = "mod_clk";
+	host->bulk_clks[1].id = "sd_clk";
+	host->bulk_clks[2].id = "tx_clk";
+	host->bulk_clks[3].id = "rx_clk";
+	ret = devm_clk_bulk_get(dev, MESON_SDHC_NUM_BULK_CLKS,
+				host->bulk_clks);
+	if (ret)
+		goto err_clkc_pdev_unregister;
+
+	host->sd_clk = host->bulk_clks[1].clk;
+
+	/* Get regulators and the supported OCR mask */
+	ret = mmc_regulator_get_supply(mmc);
+	if (ret)
+		goto err_clkc_pdev_unregister;
+
+	mmc->max_req_size = SZ_128K;
+	mmc->max_seg_size = mmc->max_req_size;
+	mmc->max_blk_count = FIELD_GET(MESON_SDHC_SEND_TOTAL_PACK, ~0);
+	mmc->max_blk_size = MESON_SDHC_MAX_BLK_SIZE;
+	mmc->max_busy_timeout = 30 * MSEC_PER_SEC;
+	mmc->f_min = clk_round_rate(host->sd_clk, 1);
+	mmc->f_max = clk_round_rate(host->sd_clk, ULONG_MAX);
+	mmc->max_current_180 = 300;
+	mmc->max_current_330 = 300;
+	mmc->caps |= MMC_CAP_ERASE | MMC_CAP_HW_RESET;
+	mmc->ops = &meson_mx_sdhc_ops;
+
+	ret = mmc_of_parse(mmc);
+	if (ret)
+		goto err_clkc_pdev_unregister;
+
+	irq = platform_get_irq(pdev, 0);
+	ret = devm_request_threaded_irq(dev, irq, meson_mx_sdhc_irq,
+					meson_mx_sdhc_irq_thread, IRQF_ONESHOT,
+					NULL, host);
+	if (ret)
+		goto err_clkc_pdev_unregister;
+
+	ret = mmc_add_host(mmc);
+	if (ret)
+		goto err_clkc_pdev_unregister;
+
+	return 0;
+
+err_clkc_pdev_unregister:
+	platform_device_unregister(host->clkc_pdev);
+err_disable_pclk:
+	clk_disable_unprepare(host->pclk);
+	return ret;
+}
+
+static int meson_mx_sdhc_remove(struct platform_device *pdev)
+{
+	struct meson_mx_sdhc_host *host = platform_get_drvdata(pdev);
+
+	mmc_remove_host(host->mmc);
+
+	meson_mx_sdhc_disable_clks(host->mmc);
+
+	platform_device_unregister(host->clkc_pdev);
+
+	clk_disable_unprepare(host->pclk);
+
+	return 0;
+}
+
+static const struct meson_mx_sdhc_data meson_mx_sdhc_data_meson8 = {
+	.init_hw			= meson_mx_sdhc_init_hw_meson8,
+	.set_pdma			= meson_mx_sdhc_set_pdma_meson8,
+	.wait_before_send		= meson_mx_sdhc_wait_before_send_meson8,
+	.hardware_flush_all_cmds	= false,
+	.clkc_name			= "meson8-sdhc-clkc",
+};
+
+static const struct meson_mx_sdhc_data meson_mx_sdhc_data_meson8m2 = {
+	.init_hw			= meson_mx_sdhc_init_hw_meson8m2,
+	.set_pdma			= meson_mx_sdhc_set_pdma_meson8m2,
+	.hardware_flush_all_cmds	= true,
+	.clkc_name			= "meson8m2-sdhc-clkc",
+};
+
+static const struct of_device_id meson_mx_sdhc_of_match[] = {
+	{
+		.compatible = "amlogic,meson8-sdhc",
+		.data = &meson_mx_sdhc_data_meson8
+	},
+	{
+		.compatible = "amlogic,meson8b-sdhc",
+		.data = &meson_mx_sdhc_data_meson8
+	},
+	{
+		.compatible = "amlogic,meson8m2-sdhc",
+		.data = &meson_mx_sdhc_data_meson8m2
+	},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, meson_mx_sdhc_of_match);
+
+static struct platform_driver meson_mx_sdhc_driver = {
+	.probe   = meson_mx_sdhc_probe,
+	.remove  = meson_mx_sdhc_remove,
+	.driver  = {
+		.name = "meson-mx-sdhc",
+		.of_match_table = of_match_ptr(meson_mx_sdhc_of_match),
+	},
+};
+
+module_platform_driver(meson_mx_sdhc_driver);
+
+MODULE_DESCRIPTION("Meson6, Meson8, Meson8b and Meson8m2 SDHC Host Driver");
+MODULE_AUTHOR("Martin Blumenstingl <martin.blumenstingl@googlemail.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.26.0

Re: [PATCH v5 1/3] dt-bindings: mmc: Document the Amlogic Meson SDHC MMC host controller

[Rob Herring]

On Sat, Mar 28, 2020 at 01:32:47AM +0100, Martin Blumenstingl wrote:
> This documents the devicetree bindings for the SDHC MMC host controller
> found in Meson6, Meson8, Meson8b and Meson8m2 SoCs. It can use a
> bus-width of 1/4/8-bit and it supports eMMC spec 4.4x/4.5x including
> HS200 mode (up to 100MHz clock). It embeds an internal clock controller
> which outputs four clocks (mod_clk, sd_clk, tx_clk and rx_clk) and is
> fed by four external input clocks (clkin[0-3]). "pclk" is the module
> register clock, it has to be enabled to access the registers.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  .../bindings/mmc/amlogic,meson-mx-sdhc.yaml   | 83 +++++++++++++++++++
>  .../dt-bindings/clock/meson-mx-sdhc-clkc.h    |  8 ++
>  2 files changed, 91 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mmc/amlogic,meson-mx-sdhc.yaml
>  create mode 100644 include/dt-bindings/clock/meson-mx-sdhc-clkc.h

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

Re: [PATCH v5 3/3] mmc: host: meson-mx-sdhc: new driver for the Amlogic Meson SDHC host

[Anand Moon]

Hi Martin,

On Sat, 28 Mar 2020 at 06:04, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
>
> The SDHC MMC host controller on Amlogic SoCs provides an eMMC and MMC
> card interface with 1/4/8-bit bus width.
> It supports eMMC spec 4.4x/4.5x including HS200 (up to 100MHz clock).
>
> The public S805 datasheet [0] contains a short documentation about the
> registers. Unfortunately it does not describe how to use the registers
> to make the hardware work. Thus this driver is based on reading (and
> understanding) the Amlogic 3.10 GPL kernel code.
>
> Some hardware details are not easy to see. Jianxin Pan was kind enough
> to answer my questions:
> The hardware has built-in busy timeout support. The maximum timeout is
> 30 seconds. This is only documented in Amlogic's internal
> documentation.
>
> The controller only works with very specific clock configurations. The
> details are not part of the public datasheet. In my own words the
> supported configurations are:
> - 399.812kHz:   clkin =  850MHz div = 2126 sd_rx_phase = 63
> - 1MHz:         clkin =  850MHz div = 850  sd_rx_phase = 55
> - 5.986MHz:     clkin =  850MHz div = 142  sd_rx_phase = 24
> - 25MHz:        clkin =  850MHz div = 34   sd_rx_phase = 15
> - 47.222MHz:    clkin =  850MHz div = 18   sd_rx_phase = 11/15 (SDR50/HS)
> - 53.125MHz:    clkin =  850MHz div = 16   sd_rx_phase = (tuning)
> - 70.833MHz:    clkin =  850MHz div = 12   sd_rx_phase = (tuning)
> - 85MHz:        clkin =  850MHz div = 10   sd_rx_phase = (tuning)
> - 94.44MHz:     clkin =  850MHz div = 9    sd_rx_phase = (tuning)
> - 106.25MHz:    clkin =  850MHz div = 8    sd_rx_phase = (tuning)
> - 127.5MHz:     clkin = 1275MHz div = 10   sd_rx_phase = (tuning)
> - 141.667MHz:   clkin =  850MHz div = 6    sd_rx_phase = (tuning)
> - 159.375MHz:   clkin = 1275MHz div = 8    sd_rx_phase = (tuning)
> - 212.5MHz:     clkin = 1275MHz div = 6    sd_rx_phase = (tuning)
> - (sd_tx_phase is always 1, 94.44MHz is not listed in the datasheet
>    but this is what the 3.10 BSP kernel on Odroid-C1 actually uses)
>
> NOTE: CMD23 support is disabled for now because it results in command
> timeouts and thus decreases read performance.
>
> Tested-by: Wei Wang <lnykww@gmail.com>
> Tested-by: Xin Yin <yinxin_1989@aliyun.com>
> Reviewed-by: Xin Yin <yinxin_1989@aliyun.com>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---

Please add my tested on Odroid C1+

Tested-by: Anand Moon <linux.amoon@gmail.com>

Best Regards
-Anand

Re: [PATCH v5 0/3] Amlogic 32-bit Meson SoC SDHC MMC controller driver

[Martin Blumenstingl]

Hi Ulf,

On Sat, Mar 28, 2020 at 1:33 AM Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
[...]
> Martin Blumenstingl (3):
>   dt-bindings: mmc: Document the Amlogic Meson SDHC MMC host controller
>   clk: meson: add a driver for the Meson8/8b/8m2 SDHC clock controller
>   mmc: host: meson-mx-sdhc: new driver for the Amlogic Meson SDHC host
I have Rob's reviewed-by for the dt-bindings patch and three
tested-by's for the MMC driver in patch #3 (which means that patch #2
was implicitly tested as well)
I tried to answer all your previous questions where possible, but for
some of your questions I simply don't have an answer.

is there anything from your side which is holding this driver back
from being merged?

+Cc Jerome, because he is the maintainer of the Amlogic clock
controller drivers - where this series adds another one, so we need to
coordinate where patches go.


Thank you!
Martin

Re: [PATCH v5 0/3] Amlogic 32-bit Meson SoC SDHC MMC controller driver

[Ulf Hansson]

On Sat, 25 Apr 2020 at 22:27, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
>
> Hi Ulf,
>
> On Sat, Mar 28, 2020 at 1:33 AM Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
> [...]
> > Martin Blumenstingl (3):
> >   dt-bindings: mmc: Document the Amlogic Meson SDHC MMC host controller
> >   clk: meson: add a driver for the Meson8/8b/8m2 SDHC clock controller
> >   mmc: host: meson-mx-sdhc: new driver for the Amlogic Meson SDHC host
> I have Rob's reviewed-by for the dt-bindings patch and three
> tested-by's for the MMC driver in patch #3 (which means that patch #2
> was implicitly tested as well)
> I tried to answer all your previous questions where possible, but for
> some of your questions I simply don't have an answer.
>
> is there anything from your side which is holding this driver back
> from being merged?

Apologize for the delay. I will have a look asap.

>
> +Cc Jerome, because he is the maintainer of the Amlogic clock
> controller drivers - where this series adds another one, so we need to
> coordinate where patches go.

It seems like you may need to resend the series so the clock
maintainers (Stephen and Jerome) can get a look as well.

Perhaps it's better if the series is queued together - and can help to
do that, but then I need acks from Stephen/Jerome for the clock patch.

Kind regards
Uffe

Re: [PATCH v5 2/3] clk: meson: add a driver for the Meson8/8b/8m2 SDHC clock controller

[Jerome Brunet]

On Sat 28 Mar 2020 at 01:32, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> The SDHC (MMC) controller embeds a clock controller inside one of the
> SDHC registers. The outputs of thisclock controller are dedicated to the
> SDHC controller.
>
> Implement a dedicated clock controller driver so we can keep all the
> clock specific logic outside of the MMC controller driver. There is no
> dedicated clock controller OF node because the hardware is a big SDHC IP
> block with an embedded clock controller (so the .dts doesn't need a
> separate clock controller node). Instead this driver re-uses the regmap
> as registered by the (platform_device) parent.
>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/clk/meson/Kconfig         |   9 ++
>  drivers/clk/meson/Makefile        |   1 +
>  drivers/clk/meson/meson-mx-sdhc.c | 212 ++++++++++++++++++++++++++++++
>  3 files changed, 222 insertions(+)
>  create mode 100644 drivers/clk/meson/meson-mx-sdhc.c
>
> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> index dabeb435d067..8769335d2d46 100644
> --- a/drivers/clk/meson/Kconfig
> +++ b/drivers/clk/meson/Kconfig
> @@ -53,6 +53,15 @@ config COMMON_CLK_MESON8B
>  	  S805 (Meson8b) and S812 (Meson8m2) devices. Say Y if you
>  	  want peripherals and CPU frequency scaling to work.
>  
> +config COMMON_CLK_MESON_MX_SDHC
> +	tristate "Meson MX SDHC MMC Clock Controller Driver"
> +	depends on ARCH_MESON
> +	select COMMON_CLK_MESON_REGMAP
> +	help
> +	  Support for the SDHC clock controller on Amlogic Meson8/8b/8m2 SoCs
> +	  devices. Say Y or M if you want to use the SDHC MMC controller.
> +	  Otherwise say N.
> +
>  config COMMON_CLK_GXBB
>  	bool
>  	depends on ARCH_MESON
> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
> index 6eca2a406ee3..b71c7ae78dbd 100644
> --- a/drivers/clk/meson/Makefile
> +++ b/drivers/clk/meson/Makefile
> @@ -19,3 +19,4 @@ obj-$(CONFIG_COMMON_CLK_AXG_AUDIO) += axg-audio.o
>  obj-$(CONFIG_COMMON_CLK_GXBB) += gxbb.o gxbb-aoclk.o
>  obj-$(CONFIG_COMMON_CLK_G12A) += g12a.o g12a-aoclk.o
>  obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o meson8-ddr.o
> +obj-$(CONFIG_COMMON_CLK_MESON_MX_SDHC) += meson-mx-sdhc.o
> diff --git a/drivers/clk/meson/meson-mx-sdhc.c b/drivers/clk/meson/meson-mx-sdhc.c
> new file mode 100644
> index 000000000000..b98a35d99f65
> --- /dev/null
> +++ b/drivers/clk/meson/meson-mx-sdhc.c
> @@ -0,0 +1,212 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Amlogic Meson SDHC clock controller
> + *
> + * Copyright (C) 2019 Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> + */
> +
> +#include <dt-bindings/clock/meson-mx-sdhc-clkc.h>
> +
> +#include <linux/clk-provider.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +#include "clk-regmap.h"
> +#include "clk-pll.h"

If you need the pll clocks, it should probably appear in the Kconfig
deps as well

> +
> +#define MESON_SDHC_CLKC			0x10
> +
> +static const struct clk_regmap meson_mx_sdhc_src_sel = {
> +	.data = &(struct clk_regmap_mux_data){
> +		.offset = MESON_SDHC_CLKC,
> +		.mask = 0x3,
> +		.shift = 16,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "sdhc_src_sel",
> +		.ops = &clk_regmap_mux_ops,
> +		.parent_data = (const struct clk_parent_data[]) {
> +			{ .fw_name = "clkin0", .index = -1, },
> +			{ .fw_name = "clkin1", .index = -1, },
> +			{ .fw_name = "clkin2", .index = -1, },
> +			{ .fw_name = "clkin3", .index = -1, },

When fw_name is specified, setting the index is not necessary

> +		},
> +		.num_parents = 4,
> +	},
> +};
> +
> +static const struct clk_div_table meson_mx_sdhc_div_table[] = {
> +	{ .div = 6, .val = 5, },
> +	{ .div = 8, .val = 7, },
> +	{ .div = 9, .val = 8, },
> +	{ .div = 10, .val = 9, },
> +	{ .div = 12, .val = 11, },
> +	{ .div = 16, .val = 15, },
> +	{ .div = 18, .val = 17, },
> +	{ .div = 34, .val = 33, },
> +	{ .div = 142, .val = 141, },
> +	{ .div = 850, .val = 849, },
> +	{ .div = 2126, .val = 2125, },
> +	{ .div = 4096, .val = 4095, },
> +	{ /* sentinel */ }
> +};
> +
> +static const struct clk_regmap meson_mx_sdhc_div = {
> +	.data = &(struct clk_regmap_div_data){
> +		.offset = MESON_SDHC_CLKC,
> +		.shift = 0,
> +		.width = 12,
> +		.table = meson_mx_sdhc_div_table,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "sdhc_div",
> +		.ops = &clk_regmap_divider_ops,
> +		.parent_data = (const struct clk_parent_data[]) {
> +			{ .name = "sdhc_src_sel", .index = -1, },

Any reason for using the lagacy names and not the clk_hw pointers ?
Same comment for the rest of the clocks

> +		},
> +		.num_parents = 1,
> +		.flags = CLK_SET_RATE_PARENT,
> +	},
> +};
> +
> +static const struct clk_regmap meson_mx_sdhc_mod_clk_en = {
> +	.data = &(struct clk_regmap_gate_data){
> +		.offset = MESON_SDHC_CLKC,
> +		.bit_idx = 15,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "sdhc_mod_clk_on",
> +		.ops = &clk_regmap_gate_ops,
> +		.parent_data = (const struct clk_parent_data[]) {
> +			{ .name = "sdhc_div", .index = -1, },
> +		},
> +		.num_parents = 1,
> +		.flags = CLK_SET_RATE_GATE,

Why can't the clock change rate unless gated ? Maybe you prefer to
change the rate in the mmc while clock is gated, but this is the
handling of the clock by the mmc driver, not a constraint of the actual
clock HW, isn't it ?

Also, this is a gate so I suppose the rate propagates through it ?
Can you explain why CLK_SET_RATE_PARENT is not set  ?

> +	},
> +};
> +
> +static const struct clk_regmap meson_mx_sdhc_tx_clk_en = {
> +	.data = &(struct clk_regmap_gate_data){
> +		.offset = MESON_SDHC_CLKC,
> +		.bit_idx = 14,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "sdhc_tx_clk_on",
> +		.ops = &clk_regmap_gate_ops,
> +		.parent_data = (const struct clk_parent_data[]) {
> +			{ .name = "sdhc_div", .index = -1, },
> +		},
> +		.num_parents = 1,
> +		.flags = CLK_SET_RATE_GATE,
> +	},
> +};
> +
> +static const struct clk_regmap meson_mx_sdhc_rx_clk_en = {
> +	.data = &(struct clk_regmap_gate_data){
> +		.offset = MESON_SDHC_CLKC,
> +		.bit_idx = 13,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "sdhc_rx_clk_on",
> +		.ops = &clk_regmap_gate_ops,
> +		.parent_data = (const struct clk_parent_data[]) {
> +			{ .name = "sdhc_div", .index = -1, },
> +		},
> +		.num_parents = 1,
> +		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,

Ok so apparently you only want to set the rate through the RX clock.
You are free to call set_rate() only on this clock in the mmc driver.
However, I don't think this should reflect as clock constraints.

> +	},
> +};
> +
> +static const struct clk_regmap meson_mx_sdhc_sd_clk_en = {
> +	.data = &(struct clk_regmap_gate_data){
> +		.offset = MESON_SDHC_CLKC,
> +		.bit_idx = 12,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "sdhc_sd_clk_on",
> +		.ops = &clk_regmap_gate_ops,
> +		.parent_data = (const struct clk_parent_data[]) {
> +			{ .name = "sdhc_div", .index = -1, },
> +		},
> +		.num_parents = 1,
> +		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,

... now I lost with these flags. I'm sure there is an idea related to
the mmc driver. Clockwise, I don't get it.

> +	},
> +};
> +
> +static const struct clk_regmap *meson_mx_sdhc_clk_regmaps[] = {
> +	[SDHC_CLKID_SRC_SEL]	= &meson_mx_sdhc_src_sel,
> +	[SDHC_CLKID_DIV]	= &meson_mx_sdhc_div,
> +	[SDHC_CLKID_MOD_CLK]	= &meson_mx_sdhc_mod_clk_en,
> +	[SDHC_CLKID_SD_CLK]	= &meson_mx_sdhc_sd_clk_en,
> +	[SDHC_CLKID_TX_CLK]	= &meson_mx_sdhc_tx_clk_en,
> +	[SDHC_CLKID_RX_CLK]	= &meson_mx_sdhc_rx_clk_en,
> +};
> +
> +#define MESON_MX_SDHC_NUM_CLKS ARRAY_SIZE(meson_mx_sdhc_clk_regmaps)
> +
> +static int meson_mx_sdhc_clkc_probe(struct platform_device *pdev)
> +{
> +	struct device *parent = pdev->dev.parent;
> +	struct clk_hw_onecell_data *onecell_data;
> +	struct clk_regmap *clk_regmap;
> +	struct regmap *regmap;
> +	int i, ret;
> +
> +	regmap = dev_get_regmap(parent, NULL);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	clk_regmap = devm_kcalloc(parent, sizeof(*clk_regmap),
> +				  MESON_MX_SDHC_NUM_CLKS, GFP_KERNEL);
> +	if (!clk_regmap)
> +		return -ENOMEM;
> +
> +	onecell_data = devm_kzalloc(parent,
> +				    struct_size(onecell_data, hws,
> +						MESON_MX_SDHC_NUM_CLKS),
> +				    GFP_KERNEL);
> +	if (!onecell_data)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < MESON_MX_SDHC_NUM_CLKS; i++) {
> +		memcpy(&clk_regmap[i], meson_mx_sdhc_clk_regmaps[i],
> +		       sizeof(*clk_regmap));
> +
> +		clk_regmap[i].map = regmap;
> +		onecell_data->hws[i] = &clk_regmap[i].hw;
> +
> +		ret = devm_clk_hw_register(parent, onecell_data->hws[i]);
> +		if (ret) {
> +			dev_err(parent,
> +				"Registration of SDHC clock %d failed\n", i);
> +			return ret;
> +		}
> +	}
> +
> +	onecell_data->num = MESON_MX_SDHC_NUM_CLKS;
> +
> +	return devm_of_clk_add_hw_provider(parent, of_clk_hw_onecell_get,
> +					   onecell_data);
> +}
> +
> +static const struct platform_device_id meson_mx_sdhc_clkc_ids[] = {
> +	{ "meson8-sdhc-clkc" },
> +	{ "meson8b-sdhc-clkc" },
> +	{ "meson8m2-sdhc-clkc" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(platform, meson_mx_sdhc_clkc_ids);
> +
> +static struct platform_driver meson_mx_sdhc_clkc_driver = {
> +	.id_table	= meson_mx_sdhc_clkc_ids,
> +	.probe		= meson_mx_sdhc_clkc_probe,
> +	.driver		= {
> +		.name	= "meson-mx-sdhc-clkc",
> +	},
> +};
> +module_platform_driver(meson_mx_sdhc_clkc_driver);
> +
> +MODULE_DESCRIPTION("Amlogic Meson8/8b/8m2 SDHC clock controller driver");
> +MODULE_AUTHOR("Martin Blumenstingl <martin.blumenstingl@googlemail.com>");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH v5 0/3] Amlogic 32-bit Meson SoC SDHC MMC controller driver

[Jerome Brunet]

On Sat 28 Mar 2020 at 01:32, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> Hello,
>
> this is the patchset for a driver for the Amlogic "SDHC" MMC controller
> found on Meson6, Meson8, Meson8b and Meson8m2 SoCs.
>
> The public S805 (Meson8b) datasheet has some documentation starting on
> page 74: [0]
>
> It's performance is still not as good as the driver from Amlogic's 3.10
> kernel, but it does not corrupt data anymore (as RFC v1 did).
>
> Special thanks to the people who supported me off-list - you are
> amazing and deserve to be mentioned here:
> - Xin Yin who helped me fix two more write corruption problems. I am
>   hoping that he will reply with Reviewed-by, Tested-by and Bug-fixed-by
> - Jianxin Pan for sharing some of the internal workings of this MMC
>   controller with me
> - Wei Wang for spotting the initial write corruption problem and helping
>   test this driver on his board. I have his permission to add his
>   Tested-by (off-list, he's Cc'ed so if there's any problem he can speak
>   up)
>
>
> Changes since v4 at [4]:
> - move the four clkin clock inputs to the start of the clock-names list
>   as suggested by Rob, affects patch #1
> - fixed #include statement in dt-bindings example in patch #1
>
> Changes since v3 at [3]:
> - split the clock bits into a separate clock controller driver because
>   of two reasons: 1) it keeps the MMC controller driver mostly clean of
>   the clock bits

If the register is in the MMC controller register space and the MMC
driver is the driver using these clocks, it is where the clocks belong.
I don't get why it could be an issue ?

Is the clock block is shared with another device, like on the Gx family ?

> 2) the pure clock controller can use
>   devm_clk_hw_register() (instead of devm_clk_register(), which is
>   deprecated) and the MMC controller can act as a pure clock consumer.

Why can't you use devm_clk_hw_register in an MMC driver ?
Unless I missed something, it is provided by clk-provider.h, which can be
included by any driver.

I'm not sure I understand why the support for this device is split in
two drivers. Using CCF clocks out of "drivers/clk" is encouraged.

>   This also affects the dt-bindings which is why I dropped Rob's
>   Reviewed-by. Thanks to Ulf for the suggestions
>
> Changes since v2 at [2]:
> - rebased on top of v5.5-rc1
> - added Rob's and Xin Yin's Reviewed-by and Tested-by (thank you!)
> - (note: Kevin had v2 of this series in -next for a few days so the
>    build test robots could play with it. I haven't received any negative
>    feedback in that time)
>
> Changes since RFC v1 at [1]:
> - don't set MESON_SDHC_MISC_MANUAL_STOP to fix one of three write
>   corruption problems. the out-of-tree 3.10 "reference" driver doesn't
>   set it either
> - check against data->flags instead of cmd->flags when testing for
>   MMC_DATA_WRITE as spotted by Xin Yin (many thanks!). This fixes
>   another write corruption problem
> - clear the FIFOs after successfully transferring data as suggested by
>   Xin Yin (many thanks!). This is what the 3.10 driver did and fixes yet
>   another write corruption problem
> - integrate the clock suggestions from Jianxin Pan so the driver is now
>   able to set up the clocks correctly for all known cases. documentation
>   is also added to the patch description. Thank you Jianxin for the
>   help!
> - set the correct max_busy_timeout as suggested by Jianxin Pan (thanks!)
> - convert the dt-bindings to .yaml (which is why I didn't add Rob's
>   Reviewed-by)
> - switch to struct clk_parent_data as part of newer common clock
>   framework APIs to simplify the clock setup
> - dropped CMD23 support because it seems to hurt read and write
>   performance by 10-20% in my tests. it's not clear why, but for now we
>   can live without this.
> - use devm_platform_ioremap_resource instead of open-coding it
>
>
> [0] https://dn.odroid.com/S805/Datasheet/S805_Datasheet%20V0.8%2020150126.pdf
> [1] https://patchwork.kernel.org/cover/11035505/
> [2] http://lists.infradead.org/pipermail/linux-amlogic/2019-November/014576.html
> [3] https://patchwork.kernel.org/cover/11283179/
> [4] https://patchwork.kernel.org/cover/11329017/
>
> Martin Blumenstingl (3):
>   dt-bindings: mmc: Document the Amlogic Meson SDHC MMC host controller
>   clk: meson: add a driver for the Meson8/8b/8m2 SDHC clock controller
>   mmc: host: meson-mx-sdhc: new driver for the Amlogic Meson SDHC host
>
>  .../bindings/mmc/amlogic,meson-mx-sdhc.yaml   |   83 ++
>  drivers/clk/meson/Kconfig                     |    9 +
>  drivers/clk/meson/Makefile                    |    1 +
>  drivers/clk/meson/meson-mx-sdhc.c             |  212 ++++
>  drivers/mmc/host/Kconfig                      |   14 +
>  drivers/mmc/host/Makefile                     |    1 +
>  drivers/mmc/host/meson-mx-sdhc.c              | 1064 +++++++++++++++++
>  .../dt-bindings/clock/meson-mx-sdhc-clkc.h    |    8 +
>  8 files changed, 1392 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mmc/amlogic,meson-mx-sdhc.yaml
>  create mode 100644 drivers/clk/meson/meson-mx-sdhc.c
>  create mode 100644 drivers/mmc/host/meson-mx-sdhc.c
>  create mode 100644 include/dt-bindings/clock/meson-mx-sdhc-clkc.h

Re: [PATCH v5 0/3] Amlogic 32-bit Meson SoC SDHC MMC controller driver

[Martin Blumenstingl]

Hi Jerome,

On Mon, Apr 27, 2020 at 10:56 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
[...]
> > Changes since v3 at [3]:
> > - split the clock bits into a separate clock controller driver because
> >   of two reasons: 1) it keeps the MMC controller driver mostly clean of
> >   the clock bits
>
> If the register is in the MMC controller register space and the MMC
> driver is the driver using these clocks, it is where the clocks belong.
> I don't get why it could be an issue ?
>
> Is the clock block is shared with another device, like on the Gx family ?
no, it is not shared with another device (to my knowledge).

> > 2) the pure clock controller can use
> >   devm_clk_hw_register() (instead of devm_clk_register(), which is
> >   deprecated) and the MMC controller can act as a pure clock consumer.
>
> Why can't you use devm_clk_hw_register in an MMC driver ?
> Unless I missed something, it is provided by clk-provider.h, which can be
> included by any driver.
indeed, I could use devm_clk_hw_register in the MMC driver.
Ulfs concern was that a lot of code was needed for managing the clocks
and I agree with him. so this is my way of keeping those details away
from the MMC driver and have two separate drivers which are better to
understand overall.


Martin

Re: [PATCH v5 2/3] clk: meson: add a driver for the Meson8/8b/8m2 SDHC clock controller

[Martin Blumenstingl]

Hi Jerome,

thank you for looking into this!

On Mon, Apr 27, 2020 at 10:41 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
[...]
> > +#include "clk-regmap.h"
> > +#include "clk-pll.h"
>
> If you need the pll clocks, it should probably appear in the Kconfig
> deps as well
this driver does not need "clk-pll.h"
good catch - thank you

> > +
> > +#define MESON_SDHC_CLKC                      0x10
> > +
> > +static const struct clk_regmap meson_mx_sdhc_src_sel = {
> > +     .data = &(struct clk_regmap_mux_data){
> > +             .offset = MESON_SDHC_CLKC,
> > +             .mask = 0x3,
> > +             .shift = 16,
> > +     },
> > +     .hw.init = &(struct clk_init_data){
> > +             .name = "sdhc_src_sel",
> > +             .ops = &clk_regmap_mux_ops,
> > +             .parent_data = (const struct clk_parent_data[]) {
> > +                     { .fw_name = "clkin0", .index = -1, },
> > +                     { .fw_name = "clkin1", .index = -1, },
> > +                     { .fw_name = "clkin2", .index = -1, },
> > +                     { .fw_name = "clkin3", .index = -1, },
>
> When fw_name is specified, setting the index is not necessary
noted, will fix this

[...]
> > +     .hw.init = &(struct clk_init_data){
> > +             .name = "sdhc_div",
> > +             .ops = &clk_regmap_divider_ops,
> > +             .parent_data = (const struct clk_parent_data[]) {
> > +                     { .name = "sdhc_src_sel", .index = -1, },
>
> Any reason for using the lagacy names and not the clk_hw pointers ?
> Same comment for the rest of the clocks
indeed, there is a reason and it took me a while to figure out
__clk_register will set hw->init = NULL;
This means: if we unregister the driver and register it again all
hw->init will be lost (as it's now NULL)
This is why I am effectively cloning (devm_kzalloc + memcpy) these
clocks which only serve as a template
Due to this I can't easily use a reference to another clk_hw

We don't have this problem in any of our other clock controller
drivers because these cannot be unloaded

[...]
> > +     .hw.init = &(struct clk_init_data){
> > +             .name = "sdhc_mod_clk_on",
> > +             .ops = &clk_regmap_gate_ops,
> > +             .parent_data = (const struct clk_parent_data[]) {
> > +                     { .name = "sdhc_div", .index = -1, },
> > +             },
> > +             .num_parents = 1,
> > +             .flags = CLK_SET_RATE_GATE,
>
> Why can't the clock change rate unless gated ? Maybe you prefer to
> change the rate in the mmc while clock is gated, but this is the
> handling of the clock by the mmc driver, not a constraint of the actual
> clock HW, isn't it ?
>
> Also, this is a gate so I suppose the rate propagates through it ?
> Can you explain why CLK_SET_RATE_PARENT is not set  ?
[...]
> Ok so apparently you only want to set the rate through the RX clock.
> You are free to call set_rate() only on this clock in the mmc driver.
> However, I don't think this should reflect as clock constraints.
I think these two belong together
looking back at this I believe that you are right:
- CLK_SET_RATE_GATE should be dropped because that's not a constraint
of the clock but of the clock consumer (MMC driver)
- CLK_SET_RATE_PARENT should be added to all clocks because rate
propagation will work for all clocks

> > +     },
> > +};
> > +
> > +static const struct clk_regmap meson_mx_sdhc_sd_clk_en = {
> > +     .data = &(struct clk_regmap_gate_data){
> > +             .offset = MESON_SDHC_CLKC,
> > +             .bit_idx = 12,
> > +     },
> > +     .hw.init = &(struct clk_init_data){
> > +             .name = "sdhc_sd_clk_on",
> > +             .ops = &clk_regmap_gate_ops,
> > +             .parent_data = (const struct clk_parent_data[]) {
> > +                     { .name = "sdhc_div", .index = -1, },
> > +             },
> > +             .num_parents = 1,
> > +             .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
>
> ... now I lost with these flags. I'm sure there is an idea related to
> the mmc driver. Clockwise, I don't get it.
indeed, just like above I'll fix these


Martin

Re: [PATCH v5 0/3] Amlogic 32-bit Meson SoC SDHC MMC controller driver

[Jerome Brunet]

On Mon 27 Apr 2020 at 18:23, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> Hi Jerome,
>
> On Mon, Apr 27, 2020 at 10:56 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
> [...]
>> > Changes since v3 at [3]:
>> > - split the clock bits into a separate clock controller driver because
>> >   of two reasons: 1) it keeps the MMC controller driver mostly clean of
>> >   the clock bits
>>
>> If the register is in the MMC controller register space and the MMC
>> driver is the driver using these clocks, it is where the clocks belong.
>> I don't get why it could be an issue ?
>>
>> Is the clock block is shared with another device, like on the Gx family ?
> no, it is not shared with another device (to my knowledge).
>
>> > 2) the pure clock controller can use
>> >   devm_clk_hw_register() (instead of devm_clk_register(), which is
>> >   deprecated) and the MMC controller can act as a pure clock consumer.
>>
>> Why can't you use devm_clk_hw_register in an MMC driver ?
>> Unless I missed something, it is provided by clk-provider.h, which can be
>> included by any driver.
> indeed, I could use devm_clk_hw_register in the MMC driver.
> Ulfs concern was that a lot of code was needed for managing the clocks
> and I agree with him. so this is my way of keeping those details away
> from the MMC driver and have two separate drivers which are better to
> understand overall.

Martin, Ulf,

I understand that CCF code might seems verbose and I'm happy to help
review it if necessary but I don't think every driver out there should
register some kind of fake clock controller driver everytime they wish
to use CCF API.

Yes the it might make the driver code cleaner but the overall
architecture is harder to follow.

CCF was made so driver from any subsystem *may* use it. Creating a
controller for a single register is overkill. The HW architecture of
this particular device does not justify it.

>
>
> Martin

Re: [PATCH v5 2/3] clk: meson: add a driver for the Meson8/8b/8m2 SDHC clock controller

[Jerome Brunet]

On Mon 27 Apr 2020 at 18:33, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> Hi Jerome,
>
> thank you for looking into this!
>
> On Mon, Apr 27, 2020 at 10:41 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
> [...]
>> > +#include "clk-regmap.h"
>> > +#include "clk-pll.h"
>>
>> If you need the pll clocks, it should probably appear in the Kconfig
>> deps as well
> this driver does not need "clk-pll.h"
> good catch - thank you
>
>> > +
>> > +#define MESON_SDHC_CLKC                      0x10
>> > +
>> > +static const struct clk_regmap meson_mx_sdhc_src_sel = {
>> > +     .data = &(struct clk_regmap_mux_data){
>> > +             .offset = MESON_SDHC_CLKC,
>> > +             .mask = 0x3,
>> > +             .shift = 16,
>> > +     },
>> > +     .hw.init = &(struct clk_init_data){
>> > +             .name = "sdhc_src_sel",
>> > +             .ops = &clk_regmap_mux_ops,
>> > +             .parent_data = (const struct clk_parent_data[]) {
>> > +                     { .fw_name = "clkin0", .index = -1, },
>> > +                     { .fw_name = "clkin1", .index = -1, },
>> > +                     { .fw_name = "clkin2", .index = -1, },
>> > +                     { .fw_name = "clkin3", .index = -1, },
>>
>> When fw_name is specified, setting the index is not necessary
> noted, will fix this
>
> [...]
>> > +     .hw.init = &(struct clk_init_data){
>> > +             .name = "sdhc_div",
>> > +             .ops = &clk_regmap_divider_ops,
>> > +             .parent_data = (const struct clk_parent_data[]) {
>> > +                     { .name = "sdhc_src_sel", .index = -1, },
>>
>> Any reason for using the lagacy names and not the clk_hw pointers ?
>> Same comment for the rest of the clocks
> indeed, there is a reason and it took me a while to figure out
> __clk_register will set hw->init = NULL;
> This means: if we unregister the driver and register it again all
> hw->init will be lost (as it's now NULL)

I think Stephen recently added the reset to NULL and it might be an
unintended consequence. AFAICT, you should be able to use hw pointer
here even if the driver unloads.
Please report it to linux-clk


> This is why I am effectively cloning (devm_kzalloc + memcpy) these
> clocks which only serve as a template
> Due to this I can't easily use a reference to another clk_hw
>
> We don't have this problem in any of our other clock controller
> drivers because these cannot be unloaded
>
> [...]
>> > +     .hw.init = &(struct clk_init_data){
>> > +             .name = "sdhc_mod_clk_on",
>> > +             .ops = &clk_regmap_gate_ops,
>> > +             .parent_data = (const struct clk_parent_data[]) {
>> > +                     { .name = "sdhc_div", .index = -1, },
>> > +             },
>> > +             .num_parents = 1,
>> > +             .flags = CLK_SET_RATE_GATE,
>>
>> Why can't the clock change rate unless gated ? Maybe you prefer to
>> change the rate in the mmc while clock is gated, but this is the
>> handling of the clock by the mmc driver, not a constraint of the actual
>> clock HW, isn't it ?
>>
>> Also, this is a gate so I suppose the rate propagates through it ?
>> Can you explain why CLK_SET_RATE_PARENT is not set  ?
> [...]
>> Ok so apparently you only want to set the rate through the RX clock.
>> You are free to call set_rate() only on this clock in the mmc driver.
>> However, I don't think this should reflect as clock constraints.
> I think these two belong together
> looking back at this I believe that you are right:
> - CLK_SET_RATE_GATE should be dropped because that's not a constraint
> of the clock but of the clock consumer (MMC driver)
> - CLK_SET_RATE_PARENT should be added to all clocks because rate
> propagation will work for all clocks
>
>> > +     },
>> > +};
>> > +
>> > +static const struct clk_regmap meson_mx_sdhc_sd_clk_en = {
>> > +     .data = &(struct clk_regmap_gate_data){
>> > +             .offset = MESON_SDHC_CLKC,
>> > +             .bit_idx = 12,
>> > +     },
>> > +     .hw.init = &(struct clk_init_data){
>> > +             .name = "sdhc_sd_clk_on",
>> > +             .ops = &clk_regmap_gate_ops,
>> > +             .parent_data = (const struct clk_parent_data[]) {
>> > +                     { .name = "sdhc_div", .index = -1, },
>> > +             },
>> > +             .num_parents = 1,
>> > +             .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
>>
>> ... now I lost with these flags. I'm sure there is an idea related to
>> the mmc driver. Clockwise, I don't get it.
> indeed, just like above I'll fix these
>
>
> Martin

Re: [PATCH v5 0/3] Amlogic 32-bit Meson SoC SDHC MMC controller driver

[Ulf Hansson]

Jerome, Martin,

On Mon, 27 Apr 2020 at 18:46, Jerome Brunet <jbrunet@baylibre.com> wrote:
>
>
> On Mon 27 Apr 2020 at 18:23, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:
>
> > Hi Jerome,
> >
> > On Mon, Apr 27, 2020 at 10:56 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
> > [...]
> >> > Changes since v3 at [3]:
> >> > - split the clock bits into a separate clock controller driver because
> >> >   of two reasons: 1) it keeps the MMC controller driver mostly clean of
> >> >   the clock bits
> >>
> >> If the register is in the MMC controller register space and the MMC
> >> driver is the driver using these clocks, it is where the clocks belong.
> >> I don't get why it could be an issue ?
> >>
> >> Is the clock block is shared with another device, like on the Gx family ?
> > no, it is not shared with another device (to my knowledge).
> >
> >> > 2) the pure clock controller can use
> >> >   devm_clk_hw_register() (instead of devm_clk_register(), which is
> >> >   deprecated) and the MMC controller can act as a pure clock consumer.
> >>
> >> Why can't you use devm_clk_hw_register in an MMC driver ?
> >> Unless I missed something, it is provided by clk-provider.h, which can be
> >> included by any driver.
> > indeed, I could use devm_clk_hw_register in the MMC driver.
> > Ulfs concern was that a lot of code was needed for managing the clocks
> > and I agree with him. so this is my way of keeping those details away
> > from the MMC driver and have two separate drivers which are better to
> > understand overall.
>
> Martin, Ulf,
>
> I understand that CCF code might seems verbose and I'm happy to help
> review it if necessary but I don't think every driver out there should
> register some kind of fake clock controller driver everytime they wish
> to use CCF API.
>
> Yes the it might make the driver code cleaner but the overall
> architecture is harder to follow.
>
> CCF was made so driver from any subsystem *may* use it. Creating a
> controller for a single register is overkill. The HW architecture of
> this particular device does not justify it.

I fully understand your point and I agree with it.

If I recall correctly, my point in the earlier review phase was that I
wanted the driver to be nicely split into a clock provider part and
into a mmc host driver part. I also raised the point of using
devm_clk_hw_register() rather than the deprecated devm_clk_register().
I still think this makes sense.

That said, perhaps a reasonable split could be to have two separate
c-files (one for clock provider and one for mmc host), but both in the
mmc subsystem.

Kind regards
Uffe

Re: [PATCH v5 3/3] mmc: host: meson-mx-sdhc: new driver for the Amlogic Meson SDHC host

[Ulf Hansson]

On Sat, 28 Mar 2020 at 01:33, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
>
> The SDHC MMC host controller on Amlogic SoCs provides an eMMC and MMC
> card interface with 1/4/8-bit bus width.
> It supports eMMC spec 4.4x/4.5x including HS200 (up to 100MHz clock).
>
> The public S805 datasheet [0] contains a short documentation about the
> registers. Unfortunately it does not describe how to use the registers
> to make the hardware work. Thus this driver is based on reading (and
> understanding) the Amlogic 3.10 GPL kernel code.
>
> Some hardware details are not easy to see. Jianxin Pan was kind enough
> to answer my questions:
> The hardware has built-in busy timeout support. The maximum timeout is
> 30 seconds. This is only documented in Amlogic's internal
> documentation.
>
> The controller only works with very specific clock configurations. The
> details are not part of the public datasheet. In my own words the
> supported configurations are:
> - 399.812kHz:   clkin =  850MHz div = 2126 sd_rx_phase = 63
> - 1MHz:         clkin =  850MHz div = 850  sd_rx_phase = 55
> - 5.986MHz:     clkin =  850MHz div = 142  sd_rx_phase = 24
> - 25MHz:        clkin =  850MHz div = 34   sd_rx_phase = 15
> - 47.222MHz:    clkin =  850MHz div = 18   sd_rx_phase = 11/15 (SDR50/HS)
> - 53.125MHz:    clkin =  850MHz div = 16   sd_rx_phase = (tuning)
> - 70.833MHz:    clkin =  850MHz div = 12   sd_rx_phase = (tuning)
> - 85MHz:        clkin =  850MHz div = 10   sd_rx_phase = (tuning)
> - 94.44MHz:     clkin =  850MHz div = 9    sd_rx_phase = (tuning)
> - 106.25MHz:    clkin =  850MHz div = 8    sd_rx_phase = (tuning)
> - 127.5MHz:     clkin = 1275MHz div = 10   sd_rx_phase = (tuning)
> - 141.667MHz:   clkin =  850MHz div = 6    sd_rx_phase = (tuning)
> - 159.375MHz:   clkin = 1275MHz div = 8    sd_rx_phase = (tuning)
> - 212.5MHz:     clkin = 1275MHz div = 6    sd_rx_phase = (tuning)
> - (sd_tx_phase is always 1, 94.44MHz is not listed in the datasheet
>    but this is what the 3.10 BSP kernel on Odroid-C1 actually uses)
>
> NOTE: CMD23 support is disabled for now because it results in command
> timeouts and thus decreases read performance.
>
> Tested-by: Wei Wang <lnykww@gmail.com>
> Tested-by: Xin Yin <yinxin_1989@aliyun.com>
> Reviewed-by: Xin Yin <yinxin_1989@aliyun.com>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/mmc/host/Kconfig         |   14 +
>  drivers/mmc/host/Makefile        |    1 +
>  drivers/mmc/host/meson-mx-sdhc.c | 1064 ++++++++++++++++++++++++++++++
>  3 files changed, 1079 insertions(+)
>  create mode 100644 drivers/mmc/host/meson-mx-sdhc.c
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 3a5089f0332c..2c8361aa7eed 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -405,6 +405,20 @@ config MMC_MESON_GX
>
>           If you have a controller with this interface, say Y here.
>
> +config MMC_MESON_MX_SDHC
> +       tristate "Amlogic Meson SDHC Host Controller support"
> +       depends on (ARM && ARCH_MESON) || COMPILE_TEST
> +       depends on OF
> +       imply COMMON_CLK_MESON_MX_SDHC
> +       help
> +         This selects support for the SDHC Host Controller on
> +         Amlogic Meson6, Meson8, Meson8b and Meson8m2 SoCs.
> +         The controller supports the SD/SDIO Spec 3.x and eMMC Spec 4.5x
> +         with 1, 4, and 8 bit bus widths.
> +
> +         If you have a controller with this interface, say Y or M here.
> +         If unsure, say N.
> +
>  config MMC_MESON_MX_SDIO
>         tristate "Amlogic Meson6/Meson8/Meson8b SD/MMC Host Controller support"
>         depends on ARCH_MESON || COMPILE_TEST
> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> index 21d9089e5eda..9a0c22a8139a 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -68,6 +68,7 @@ obj-$(CONFIG_MMC_VUB300)      += vub300.o
>  obj-$(CONFIG_MMC_USHC)         += ushc.o
>  obj-$(CONFIG_MMC_WMT)          += wmt-sdmmc.o
>  obj-$(CONFIG_MMC_MESON_GX)     += meson-gx-mmc.o
> +obj-$(CONFIG_MMC_MESON_MX_SDHC)        += meson-mx-sdhc.o
>  obj-$(CONFIG_MMC_MESON_MX_SDIO)        += meson-mx-sdio.o
>  obj-$(CONFIG_MMC_MOXART)       += moxart-mmc.o
>  obj-$(CONFIG_MMC_SUNXI)                += sunxi-mmc.o
> diff --git a/drivers/mmc/host/meson-mx-sdhc.c b/drivers/mmc/host/meson-mx-sdhc.c
> new file mode 100644
> index 000000000000..b92400ee488b
> --- /dev/null
> +++ b/drivers/mmc/host/meson-mx-sdhc.c

[...]

> +
> +static void meson_mx_sdhc_wait_cmd_ready(struct mmc_host *mmc)
> +{
> +       struct meson_mx_sdhc_host *host = mmc_priv(mmc);
> +       u32 stat, esta;
> +       int ret;
> +
> +       ret = regmap_read_poll_timeout(host->regmap, MESON_SDHC_STAT, stat,
> +                                      !(stat & MESON_SDHC_STAT_CMD_BUSY), 1,
> +                                      100000);

Please use defines for timeout values.

> +       if (ret) {
> +               dev_warn(mmc_dev(mmc),
> +                        "Failed to poll for CMD_BUSY while processing CMD%d\n",
> +                        host->cmd->opcode);
> +               meson_mx_sdhc_hw_reset(mmc);
> +       }
> +
> +       ret = regmap_read_poll_timeout(host->regmap, MESON_SDHC_ESTA, esta,
> +                                      !(esta & MESON_SDHC_ESTA_11_13), 1,
> +                                      100000);

Ditto.

> +       if (ret) {
> +               dev_warn(mmc_dev(mmc),
> +                        "Failed to poll for ESTA[13:11] while processing CMD%d\n",
> +                        host->cmd->opcode);
> +               meson_mx_sdhc_hw_reset(mmc);
> +       }
> +}
> +
> +static void meson_mx_sdhc_start_cmd(struct mmc_host *mmc,
> +                                   struct mmc_command *cmd)
> +{
> +       struct meson_mx_sdhc_host *host = mmc_priv(mmc);
> +       u32 ictl, send;
> +       int pack_len;
> +
> +       host->cmd = cmd;
> +
> +       ictl = MESON_SDHC_ICTL_DATA_TIMEOUT | MESON_SDHC_ICTL_DATA_ERR_CRC |
> +              MESON_SDHC_ICTL_RXFIFO_FULL | MESON_SDHC_ICTL_TXFIFO_EMPTY |
> +              MESON_SDHC_ICTL_RESP_TIMEOUT | MESON_SDHC_ICTL_RESP_ERR_CRC;
> +
> +       send = FIELD_PREP(MESON_SDHC_SEND_CMD_INDEX, cmd->opcode);
> +
> +       if (cmd->data) {
> +               send |= MESON_SDHC_SEND_CMD_HAS_DATA;
> +               send |= FIELD_PREP(MESON_SDHC_SEND_TOTAL_PACK,
> +                                  cmd->data->blocks - 1);
> +
> +               if (cmd->data->blksz < MESON_SDHC_MAX_BLK_SIZE)
> +                       pack_len = cmd->data->blksz;
> +               else
> +                       pack_len = 0;
> +
> +               if (cmd->data->flags & MMC_DATA_WRITE)
> +                       send |= MESON_SDHC_SEND_DATA_DIR;
> +
> +               /*
> +                * If command with no data, just wait response done
> +                * interrupt(int[0]), and if command with data transfer, just
> +                * wait dma done interrupt(int[11]), don't need care about
> +                * dat0 busy or not.
> +                */
> +               if (host->platform->hardware_flush_all_cmds ||
> +                   cmd->data->flags & MMC_DATA_WRITE)
> +                       /* hardware flush: */
> +                       ictl |= MESON_SDHC_ICTL_DMA_DONE;
> +               else
> +                       /* software flush: */
> +                       ictl |= MESON_SDHC_ICTL_DATA_XFER_OK;
> +       } else {
> +               pack_len = 0;
> +
> +               ictl |= MESON_SDHC_ICTL_RESP_OK;
> +       }
> +
> +       if (cmd->opcode == MMC_STOP_TRANSMISSION)
> +               send |= MESON_SDHC_SEND_DATA_STOP;
> +
> +       if (cmd->flags & MMC_RSP_PRESENT)
> +               send |= MESON_SDHC_SEND_CMD_HAS_RESP;
> +
> +       if (cmd->flags & MMC_RSP_136) {
> +               send |= MESON_SDHC_SEND_RESP_LEN;
> +               send |= MESON_SDHC_SEND_RESP_NO_CRC;
> +       }
> +
> +       if (!(cmd->flags & MMC_RSP_CRC))
> +               send |= MESON_SDHC_SEND_RESP_NO_CRC;
> +
> +       if (cmd->flags & MMC_RSP_BUSY)
> +               send |= MESON_SDHC_SEND_R1B;
> +
> +       /* enable the new IRQs and mask all pending ones */
> +       regmap_write(host->regmap, MESON_SDHC_ICTL, ictl);
> +       regmap_write(host->regmap, MESON_SDHC_ISTA, MESON_SDHC_ISTA_ALL_IRQS);
> +
> +       regmap_write(host->regmap, MESON_SDHC_ARGU, cmd->arg);
> +
> +       regmap_update_bits(host->regmap, MESON_SDHC_CTRL,
> +                          MESON_SDHC_CTRL_PACK_LEN,
> +                          FIELD_PREP(MESON_SDHC_CTRL_PACK_LEN, pack_len));
> +
> +       if (cmd->data)
> +               regmap_write(host->regmap, MESON_SDHC_ADDR,
> +                            sg_dma_address(cmd->data->sg));
> +
> +       meson_mx_sdhc_wait_cmd_ready(mmc);
> +
> +       if (cmd->data)
> +               host->platform->set_pdma(mmc);
> +
> +       if (host->platform->wait_before_send)
> +               host->platform->wait_before_send(mmc);
> +
> +       regmap_write(host->regmap, MESON_SDHC_SEND, send);

Isn't there a configurable timeout to set for the command?

I mean the driver sets mmc->max_busy_timeout to 30s in ->probe(), but
can the timeout be configured to a lower value?

[...]

> +static void meson_mx_sdhc_wait_before_send_meson8(struct mmc_host *mmc)
> +{
> +       struct meson_mx_sdhc_host *host = mmc_priv(mmc);
> +       u32 val;
> +       int ret;
> +
> +       ret = regmap_read_poll_timeout(host->regmap, MESON_SDHC_ESTA, val,
> +                                      val == 0, 1, 200);

Please use defines for timeout values.

> +       if (ret)
> +               dev_warn(mmc_dev(mmc),
> +                        "Failed to wait for ESTA to clear: 0x%08x\n", val);
> +
> +       if (host->cmd->data && host->cmd->data->flags & MMC_DATA_WRITE) {
> +               ret = regmap_read_poll_timeout(host->regmap, MESON_SDHC_STAT,
> +                                              val,
> +                                              val & MESON_SDHC_STAT_TXFIFO_CNT,
> +                                              1, 200);

Ditto.

> +               if (ret)
> +                       dev_warn(mmc_dev(mmc),
> +                                "Failed to wait for TX FIFO to fill\n");
> +       }
> +}
> +

[...]

> +
> +static int meson_mx_sdhc_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct meson_mx_sdhc_host *host;
> +       struct mmc_host *mmc;
> +       void __iomem *base;
> +       int ret, irq;
> +
> +       mmc = mmc_alloc_host(sizeof(*host), dev);
> +       if (!mmc)
> +               return -ENOMEM;
> +
> +       ret = devm_add_action_or_reset(dev, (void(*)(void *))mmc_free_host,
> +                                      mmc);
> +       if (ret) {
> +               dev_err(dev, "Failed to register mmc_free_host action\n");
> +               return ret;
> +       }
> +
> +       host = mmc_priv(mmc);
> +       host->mmc = mmc;
> +
> +       platform_set_drvdata(pdev, host);
> +
> +       host->platform = device_get_match_data(dev);
> +       if (!host->platform)
> +               return -EINVAL;
> +
> +       base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(base))
> +               return PTR_ERR(base);
> +
> +       host->regmap = devm_regmap_init_mmio(dev, base,
> +                                            &meson_mx_sdhc_regmap_config);
> +       if (IS_ERR(host->regmap))
> +               return PTR_ERR(host->regmap);
> +
> +       host->pclk = devm_clk_get(dev, "pclk");
> +       if (IS_ERR(host->pclk))
> +               return PTR_ERR(host->pclk);
> +
> +       /* accessing any register requires the module clock to be enabled: */
> +       ret = clk_prepare_enable(host->pclk);
> +       if (ret) {
> +               dev_err(dev, "Failed to enable 'pclk' clock\n");
> +               return ret;
> +       }
> +
> +       meson_mx_sdhc_init_hw(mmc);
> +
> +       ret = meson_mx_sdhc_register_clkc(host);
> +       if (ret)
> +               goto err_disable_pclk;
> +
> +       host->bulk_clks[0].id = "mod_clk";
> +       host->bulk_clks[1].id = "sd_clk";
> +       host->bulk_clks[2].id = "tx_clk";
> +       host->bulk_clks[3].id = "rx_clk";
> +       ret = devm_clk_bulk_get(dev, MESON_SDHC_NUM_BULK_CLKS,
> +                               host->bulk_clks);
> +       if (ret)
> +               goto err_clkc_pdev_unregister;
> +
> +       host->sd_clk = host->bulk_clks[1].clk;
> +
> +       /* Get regulators and the supported OCR mask */
> +       ret = mmc_regulator_get_supply(mmc);
> +       if (ret)
> +               goto err_clkc_pdev_unregister;
> +
> +       mmc->max_req_size = SZ_128K;
> +       mmc->max_seg_size = mmc->max_req_size;
> +       mmc->max_blk_count = FIELD_GET(MESON_SDHC_SEND_TOTAL_PACK, ~0);
> +       mmc->max_blk_size = MESON_SDHC_MAX_BLK_SIZE;
> +       mmc->max_busy_timeout = 30 * MSEC_PER_SEC;
> +       mmc->f_min = clk_round_rate(host->sd_clk, 1);
> +       mmc->f_max = clk_round_rate(host->sd_clk, ULONG_MAX);
> +       mmc->max_current_180 = 300;
> +       mmc->max_current_330 = 300;
> +       mmc->caps |= MMC_CAP_ERASE | MMC_CAP_HW_RESET;

Should you also set MMC_CAP_WAIT_WHILE_BUSY? It sounded like the
driver supported this.

> +       mmc->ops = &meson_mx_sdhc_ops;
> +
> +       ret = mmc_of_parse(mmc);
> +       if (ret)
> +               goto err_clkc_pdev_unregister;
> +
> +       irq = platform_get_irq(pdev, 0);
> +       ret = devm_request_threaded_irq(dev, irq, meson_mx_sdhc_irq,
> +                                       meson_mx_sdhc_irq_thread, IRQF_ONESHOT,
> +                                       NULL, host);
> +       if (ret)
> +               goto err_clkc_pdev_unregister;
> +
> +       ret = mmc_add_host(mmc);
> +       if (ret)
> +               goto err_clkc_pdev_unregister;
> +
> +       return 0;
> +
> +err_clkc_pdev_unregister:
> +       platform_device_unregister(host->clkc_pdev);
> +err_disable_pclk:
> +       clk_disable_unprepare(host->pclk);
> +       return ret;
> +}
> +

[...]

FYI: I left out all comments related to the clock provider
initialization. I think it makes better sense to review that code,
after you have converted to use the devm_clk_hw_register() and avoid
registering a separate driver for it.

Other than the minor comments, this looks good to me.

Kind regards
Uffe

Re: [PATCH v5 0/3] Amlogic 32-bit Meson SoC SDHC MMC controller driver

[Martin Blumenstingl]

Hi Ulf, Jerome,

On Mon, Apr 27, 2020 at 8:36 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> Jerome, Martin,
>
> On Mon, 27 Apr 2020 at 18:46, Jerome Brunet <jbrunet@baylibre.com> wrote:
> >
> >
> > On Mon 27 Apr 2020 at 18:23, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:
> >
> > > Hi Jerome,
> > >
> > > On Mon, Apr 27, 2020 at 10:56 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
> > > [...]
> > >> > Changes since v3 at [3]:
> > >> > - split the clock bits into a separate clock controller driver because
> > >> >   of two reasons: 1) it keeps the MMC controller driver mostly clean of
> > >> >   the clock bits
> > >>
> > >> If the register is in the MMC controller register space and the MMC
> > >> driver is the driver using these clocks, it is where the clocks belong.
> > >> I don't get why it could be an issue ?
> > >>
> > >> Is the clock block is shared with another device, like on the Gx family ?
> > > no, it is not shared with another device (to my knowledge).
> > >
> > >> > 2) the pure clock controller can use
> > >> >   devm_clk_hw_register() (instead of devm_clk_register(), which is
> > >> >   deprecated) and the MMC controller can act as a pure clock consumer.
> > >>
> > >> Why can't you use devm_clk_hw_register in an MMC driver ?
> > >> Unless I missed something, it is provided by clk-provider.h, which can be
> > >> included by any driver.
> > > indeed, I could use devm_clk_hw_register in the MMC driver.
> > > Ulfs concern was that a lot of code was needed for managing the clocks
> > > and I agree with him. so this is my way of keeping those details away
> > > from the MMC driver and have two separate drivers which are better to
> > > understand overall.
> >
> > Martin, Ulf,
> >
> > I understand that CCF code might seems verbose and I'm happy to help
> > review it if necessary but I don't think every driver out there should
> > register some kind of fake clock controller driver everytime they wish
> > to use CCF API.
> >
> > Yes the it might make the driver code cleaner but the overall
> > architecture is harder to follow.
> >
> > CCF was made so driver from any subsystem *may* use it. Creating a
> > controller for a single register is overkill. The HW architecture of
> > this particular device does not justify it.
>
> I fully understand your point and I agree with it.
>
> If I recall correctly, my point in the earlier review phase was that I
> wanted the driver to be nicely split into a clock provider part and
> into a mmc host driver part. I also raised the point of using
> devm_clk_hw_register() rather than the deprecated devm_clk_register().
> I still think this makes sense.
>
> That said, perhaps a reasonable split could be to have two separate
> c-files (one for clock provider and one for mmc host), but both in the
> mmc subsystem.
I'm fine with that - I'll do that in the next patch version
I believe the amount of changes will be small because the clock driver
already uses devm_clk_hw_register()


Martin

Re: [PATCH v5 3/3] mmc: host: meson-mx-sdhc: new driver for the Amlogic Meson SDHC host

[Martin Blumenstingl]

Hi Ulf,

thank you for looking into this!

On Mon, Apr 27, 2020 at 9:20 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
[...]
> > +static void meson_mx_sdhc_wait_cmd_ready(struct mmc_host *mmc)
> > +{
> > +       struct meson_mx_sdhc_host *host = mmc_priv(mmc);
> > +       u32 stat, esta;
> > +       int ret;
> > +
> > +       ret = regmap_read_poll_timeout(host->regmap, MESON_SDHC_STAT, stat,
> > +                                      !(stat & MESON_SDHC_STAT_CMD_BUSY), 1,
> > +                                      100000);
>
> Please use defines for timeout values.
I'll take care of this here and all other places which you have found

[...]
> > +       if (cmd->data)
> > +               host->platform->set_pdma(mmc);
> > +
> > +       if (host->platform->wait_before_send)
> > +               host->platform->wait_before_send(mmc);
> > +
> > +       regmap_write(host->regmap, MESON_SDHC_SEND, send);
>
> Isn't there a configurable timeout to set for the command?
>
> I mean the driver sets mmc->max_busy_timeout to 30s in ->probe(), but
> can the timeout be configured to a lower value?
there's MESON_SDHC_CTRL_RX_TIMEOUT and MESON_SDHC_CTRL_RX_PERIOD
here's what the datasheet has to say about them:
- rx_timeout(cmd or wcrc Receiving Timeout, default 64)
- rc_period(Period between response/cmd and default next cmd,default
8) - I'm not even sure if this is related somehow

if you have a specific test-case for me to provoke these timeouts I
can try playing around with these values
otherwise we have to ask Jianxin and see whether he can get some
information about this from the internal team at Amlogic

[...]
> > +       mmc->caps |= MMC_CAP_ERASE | MMC_CAP_HW_RESET;
>
> Should you also set MMC_CAP_WAIT_WHILE_BUSY? It sounded like the
> driver supported this.
I can try setting it.
From our previous discussion (on the meson-mx-sdio driver) I have
learned that eMMC will be a good test-case for it ;-)

[...]
> FYI: I left out all comments related to the clock provider
> initialization. I think it makes better sense to review that code,
> after you have converted to use the devm_clk_hw_register() and avoid
> registering a separate driver for it.
yes, that makes sense
I expect the code to be easier since it'll be one big driver with the
next version (so no more platform device allocation, etc.)

> Other than the minor comments, this looks good to me.
great - it would be great if this could finally make it into v5.8


Martin