[PATCH v3 0/6] Add MMC/SD support for S700/S900

[PATCH v3 0/6] Add MMC/SD support for S700/S900

[Amit Singh Tomar]

From: Amit Singh Tomar <amittomer25@gmail.com>

This series(v3) addresses review comments provided by Andre and Jaehoon. Earlier
in MMC driver, we were *not* checking the command response errors(for instance,
the CRC error) based on STATU register. This is now addressed in patch 5/6.

Also, MMC driver(pacth 5/6) now has replaced the switch/case with if/else to 
correlate MMC_RSP_Rx bitmask values with register bits as suggested by Andre.

Apart from it, there are changes in patch 2/6 to fix weird assignments to div
variable.

----------------------------------------------------------------------------------
Series(v2) has few important updates, while loading large files we found
that MMC framework puts "0x1fffe00" into DMA Frame Length(DMA_FRAME_LEN 0x000C)
but register itself is limited to 24 bits and hence it was failing.
It is due to the wrong Block count(cfg->b_max) used in driver, that should be
just 512. This is now fixed in patch 5/6.

Apart from it, as Andre pointed that we might be just overclocking the MMC/SD
clock, and to confirm this we run following test:

$ md5sum clang 
349eac46cbbe28f8e44da2dce07fa7b7  clang

U-Boot => ext4load mmc 0:2 0x0 clang
503316480 bytes read in 19516 ms (24.6 MiB/s)
U-Boot => md5sum 0x0 0x1e000000
md5 for 00000000 ... 1dffffff ==> d793bb51c4a1cf83c96d1980927461ff

Even though file gets loaded but md5sum doesn't match. This is now fixed in
patch 2/6

U-Boot => ext4load mmc 0:2 0x0 clang
503316480 bytes read in 41524 ms (11.6 MiB/s)
U-Boot => md5sum 0x0 0x1e000000
md5 for 00000000 ... 1dffffff ==> 349eac46cbbe28f8e44da2dce07fa7b7

-----------------------------------------------------------------------------
At the moment on S700 based platforms, only way to load/boot the Kernel
is from Ethernet, and with these patches one can now load/boot the
Kernel from uSD card.

Patches(1/6 and 2/6) adds changes needed for MMC/SD clock. It introduces
set/get callback routine and get/set MMC/SD clock rate based on device id.

Patch 4/6 adds MMC/SD node in U-boot specific dtsi file, which is used by MMC/SD
driver to read controller base address later on.

Patch 5/6 adds driver for MMC/SD controller present on S700 SoC, and its based
on Mainline Linux driver and DMA related bits is picked and simpilified from 
vendor source.

Final patch 6/6 enables the driver support along with MMC commands in
Cubieboard7 config file.

Also, while at it just took the opportunity to synchronize the S700 SoC DT with
Linux in patch 3/6.

This patch-set is tested on Cubieboard7-lite board with following results:

U-Boot 2021.01-rc1-04434-g6589149-dirty (Dec 13 2020 - 13:51:07 +0530)
cubieboard7

DRAM:  1 GiB
PSCI:  v0.2
MMC:   mmc at e0210000: 0
In:    serial at e0126000
Out:   serial at e0126000
Err:   serial at e0126000
Net:   eth0: ethernet at e0220000
Hit any key to stop autoboot:  0 
U-Boot => 
U-Boot => 
U-Boot => 
U-Boot => mmc info
Device: mmc at e0210000
Manufacturer ID: 3
OEM: 5344
Name: SC16G 
Bus Speed: 50000000
Mode: SD High Speed (50MHz)
Rd Block Len: 512
SD version 3.0
High Capacity: Yes
Capacity: 14.8 GiB
Bus Width: 4-bit
Erase Group Size: 512 Bytes
U-Boot => setenv bootargs console=ttyOWL3,115200n8 earlycon=owl,0xe0126000 init=/sbin/init  root=/dev/mmcblk0p2 rw rootwait
U-Boot => setenv kernel_addr_r 0x80000;setenv fdt_addr_r 0x10000000;
U-Boot => fatload mmc 0:1 ${kernel_addr_r} image ;fatload mmc 0:1 ${fdt_addr_r} s700-cubieboard7.dtb
27480576 bytes read in 1041 ms (25.2 MiB/s)
7056 bytes read in 2 ms (3.4 MiB/s)
U-Boot => booti $kernel_addr_r - $fdt_addr_r
## Flattened Device Tree blob at 10000000
   Booting using the fdt blob at 0x10000000
   Loading Device Tree to 000000003df56000, end 000000003df5ab8f ... OK

Starting kernel ...

[    0.000000] Booting Linux on physical CPU 0x0000000000 [0x410fd034]
[    0.000000] Linux version 5.7.0-rc6-00022-g99f1c330923b-dirty (amit at amit-ThinkPad-X230) (gcc version 7.3.1 20180425 [linaro-7.3-2018.05 revision d29120a424ecfbc167ef90065c0eeb7f91977701] (Linaro GCC 7.3-2018.05), GNU ld (GNU Binutils for Ubuntu) 2.26.1) #474 SMP PREEMPT Thu Aug 13 15:27:10 IST 2020
[    0.000000] Machine model: CubieBoard7
[    0.000000] earlycon: owl0 at MMIO 0x00000000e0126000 (options '')
[    0.000000] printk: bootconsole [owl0] enabled
[    0.000000] efi: UEFI not found.
[    0.000000] cma: Reserved 32 MiB at 0x000000003e000000

Amit Singh Tomar (6):
  clk: actions: Introduce dummy get/set_rate callbacks
  clk: actions: Add SD/MMC clocks
  ARM: dts: sync Actions Semi S700 DT from Linux 5.10-rc7
  ARM: dts: s700: add MMC/SD controller node
  mmc: actions: add MMC driver for Actions OWL S700/S900
  configs: Enable mmc support

 arch/arm/dts/s700-u-boot.dtsi                  |  10 +
 arch/arm/dts/s700.dtsi                         |  17 +-
 configs/cubieboard7_defconfig                  |   3 +
 drivers/clk/owl/clk_owl.c                      |  99 ++++++
 drivers/mmc/Kconfig                            |   7 +
 drivers/mmc/Makefile                           |   1 +
 drivers/mmc/owl_mmc.c                          | 401 +++++++++++++++++++++++++
 include/dt-bindings/power/owl-s700-powergate.h |  19 ++
 8 files changed, 556 insertions(+), 1 deletion(-)
 create mode 100644 drivers/mmc/owl_mmc.c
 create mode 100644 include/dt-bindings/power/owl-s700-powergate.h

-- 
2.7.4

[PATCH v3 1/6] clk: actions: Introduce dummy get/set_rate callbacks

[Amit Singh Tomar]

From: Amit Singh Tomar <amittomer25@gmail.com>

This commit introduces get/set_rate callbacks, these are dummy at
the moment, and can be used to get/set clock for various devices
based on the clk id.

Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
---
Changes since v2:
	* No changes.
Changes since previous version:
        * Removed premature initialization to avoid
          compiler warnings.
---
 drivers/clk/owl/clk_owl.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/clk/owl/clk_owl.c b/drivers/clk/owl/clk_owl.c
index 1999c87..5be1b3b 100644
--- a/drivers/clk/owl/clk_owl.c
+++ b/drivers/clk/owl/clk_owl.c
@@ -128,6 +128,30 @@ int owl_clk_disable(struct clk *clk)
 	return 0;
 }
 
+static ulong owl_clk_get_rate(struct clk *clk)
+{
+	ulong rate;
+
+	switch (clk->id) {
+	default:
+		return -ENOENT;
+	}
+
+	return rate;
+}
+
+static ulong owl_clk_set_rate(struct clk *clk, ulong rate)
+{
+	ulong new_rate;
+
+	switch (clk->id) {
+	default:
+		return -ENOENT;
+	}
+
+	return new_rate;
+}
+
 static int owl_clk_probe(struct udevice *dev)
 {
 	struct owl_clk_priv *priv = dev_get_priv(dev);
@@ -145,6 +169,8 @@ static int owl_clk_probe(struct udevice *dev)
 static const struct clk_ops owl_clk_ops = {
 	.enable = owl_clk_enable,
 	.disable = owl_clk_disable,
+	.get_rate = owl_clk_get_rate,
+	.set_rate = owl_clk_set_rate,
 };
 
 static const struct udevice_id owl_clk_ids[] = {
-- 
2.7.4

[PATCH v3 2/6] clk: actions: Add SD/MMC clocks

[Amit Singh Tomar]

From: Amit Singh Tomar <amittomer25@gmail.com>

This commit adds SD/MMC clocks, and provides .set/get_rate callbacks
for SD/MMC device present on Actions OWL S700 SoCs.

Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
---
Changes since v2:
	* Fixed the not weird div assignment.
	* Moved the clock bit for SD from header file 
	  to driver file.
	* Removed "< 0" part while comparing unsigned.
Changes since previous version:
        * Removed rate *= 2 as this just overclocks.
        * Separated the divide by 128 bit from divider value.
        * Provided the separate routine to get sd parent rate
          based on bit 9.
        * Removed unnecessary initialization.
---
 drivers/clk/owl/clk_owl.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/drivers/clk/owl/clk_owl.c b/drivers/clk/owl/clk_owl.c
index 5be1b3b..34cee7d 100644
--- a/drivers/clk/owl/clk_owl.c
+++ b/drivers/clk/owl/clk_owl.c
@@ -20,6 +20,8 @@
 #include <linux/bitops.h>
 #include <linux/delay.h>
 
+#define CMU_DEVCLKEN0_SD0	BIT(22)
+
 void owl_clk_init(struct owl_clk_priv *priv)
 {
 	u32 bus_clk = 0, core_pll, dev_pll;
@@ -92,6 +94,9 @@ int owl_clk_enable(struct clk *clk)
 		setbits_le32(priv->base + CMU_DEVCLKEN1, CMU_DEVCLKEN1_ETH);
 		setbits_le32(priv->base + CMU_ETHERNETPLL, 5);
 		break;
+	case CLK_SD0:
+		setbits_le32(priv->base + CMU_DEVCLKEN0, CMU_DEVCLKEN0_SD0);
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -121,6 +126,9 @@ int owl_clk_disable(struct clk *clk)
 	case CLK_ETHERNET:
 		clrbits_le32(priv->base + CMU_DEVCLKEN1, CMU_DEVCLKEN1_ETH);
 		break;
+	case CLK_SD0:
+		clrbits_le32(priv->base + CMU_DEVCLKEN0, CMU_DEVCLKEN0_SD0);
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -128,11 +136,72 @@ int owl_clk_disable(struct clk *clk)
 	return 0;
 }
 
+static ulong get_sd_parent_rate(struct owl_clk_priv *priv, u32 dev_index)
+{
+	ulong rate;
+	u32 reg;
+
+	reg = readl(priv->base + (CMU_SD0CLK + dev_index * 0x4));
+	/* Clock output of DEV/NAND_PLL
+	 * Range: 48M ~ 756M
+	 * Frequency= PLLCLK * 6
+	 */
+	if (reg & 0x200)
+		rate = readl(priv->base + CMU_NANDPLL) & 0x7f;
+	else
+		rate = readl(priv->base + CMU_DEVPLL) & 0x7f;
+
+	rate *= 6000000;
+
+	return rate;
+}
+
+static ulong owl_get_sd_clk_rate(struct owl_clk_priv *priv, int sd_index)
+{
+	uint div, val;
+	ulong parent_rate = get_sd_parent_rate(priv, sd_index);
+
+	val = readl(priv->base + (CMU_SD0CLK + sd_index * 0x4));
+	div = (val & 0x1f) + 1;
+
+	return (parent_rate / div);
+}
+
+static ulong owl_set_sd_clk_rate(struct owl_clk_priv *priv, ulong rate,
+				 int sd_index)
+{
+	uint div, val;
+	ulong parent_rate = get_sd_parent_rate(priv, sd_index);
+
+	if (rate == 0)
+		return rate;
+
+	div = (parent_rate / rate);
+
+	val = readl(priv->base + (CMU_SD0CLK + sd_index * 0x4));
+	/* Bits 4..0 is used to program div value and bit 8 to enable
+	 * divide by 128 circuit
+	 */
+	val &= ~0x11f;
+	if (div >= 128) {
+		div = div / 128;
+		val |= 0x100; /* enable divide by 128 circuit */
+	}
+	val |= ((div - 1) & 0x1f);
+	writel(val, priv->base + (CMU_SD0CLK + sd_index * 0x4));
+
+	return owl_get_sd_clk_rate(priv, 0);
+}
+
 static ulong owl_clk_get_rate(struct clk *clk)
 {
+	struct owl_clk_priv *priv = dev_get_priv(clk->dev);
 	ulong rate;
 
 	switch (clk->id) {
+	case CLK_SD0:
+		rate = owl_get_sd_clk_rate(priv, 0);
+		break;
 	default:
 		return -ENOENT;
 	}
@@ -142,9 +211,13 @@ static ulong owl_clk_get_rate(struct clk *clk)
 
 static ulong owl_clk_set_rate(struct clk *clk, ulong rate)
 {
+	struct owl_clk_priv *priv = dev_get_priv(clk->dev);
 	ulong new_rate;
 
 	switch (clk->id) {
+	case CLK_SD0:
+		new_rate = owl_set_sd_clk_rate(priv, rate, 0);
+		break;
 	default:
 		return -ENOENT;
 	}
-- 
2.7.4

[PATCH v3 3/6] ARM: dts: sync Actions Semi S700 DT from Linux 5.10-rc7

[Amit Singh Tomar]

From: Amit Singh Tomar <amittomer25@gmail.com>

This Synchronizes the Actions Semi S700 SoC DT changes from
commit "0477e9288185" ("Linux 5.10-rc7").

Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
---
Changes since previous versions
        * No change.
---
 arch/arm/dts/s700.dtsi                         | 17 ++++++++++++++++-
 include/dt-bindings/power/owl-s700-powergate.h | 19 +++++++++++++++++++
 2 files changed, 35 insertions(+), 1 deletion(-)
 create mode 100644 include/dt-bindings/power/owl-s700-powergate.h

diff --git a/arch/arm/dts/s700.dtsi b/arch/arm/dts/s700.dtsi
index 2006ad5..2c78cae 100644
--- a/arch/arm/dts/s700.dtsi
+++ b/arch/arm/dts/s700.dtsi
@@ -5,6 +5,7 @@
 
 #include <dt-bindings/clock/actions,s700-cmu.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/power/owl-s700-powergate.h>
 #include <dt-bindings/reset/actions,s700-reset.h>
 
 / {
@@ -231,7 +232,7 @@
 
 		pinctrl: pinctrl at e01b0000 {
 			compatible = "actions,s700-pinctrl";
-			reg = <0x0 0xe01b0000 0x0 0x1000>;
+			reg = <0x0 0xe01b0000 0x0 0x100>;
 			clocks = <&cmu CLK_GPIO>;
 			gpio-controller;
 			gpio-ranges = <&pinctrl 0 0 136>;
@@ -244,5 +245,19 @@
 				     <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>,
 				     <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
 		};
+
+		dma: dma-controller at e0230000 {
+			compatible = "actions,s700-dma";
+			reg = <0x0 0xe0230000 0x0 0x1000>;
+			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>;
+			#dma-cells = <1>;
+			dma-channels = <10>;
+			dma-requests = <44>;
+			clocks = <&cmu CLK_DMAC>;
+			power-domains = <&sps S700_PD_DMA>;
+		};
 	};
 };
diff --git a/include/dt-bindings/power/owl-s700-powergate.h b/include/dt-bindings/power/owl-s700-powergate.h
new file mode 100644
index 0000000..4cf1aef
--- /dev/null
+++ b/include/dt-bindings/power/owl-s700-powergate.h
@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Actions Semi S700 SPS
+ *
+ * Copyright (c) 2017 Andreas F?rber
+ */
+#ifndef DT_BINDINGS_POWER_OWL_S700_POWERGATE_H
+#define DT_BINDINGS_POWER_OWL_S700_POWERGATE_H
+
+#define S700_PD_VDE	0
+#define S700_PD_VCE_SI	1
+#define S700_PD_USB2_1	2
+#define S700_PD_HDE	3
+#define S700_PD_DMA	4
+#define S700_PD_DS	5
+#define S700_PD_USB3	6
+#define S700_PD_USB2_0	7
+
+#endif
-- 
2.7.4

[PATCH v3 4/6] ARM: dts: s700: add MMC/SD controller node

[Amit Singh Tomar]

From: Amit Singh Tomar <amittomer25@gmail.com>

This patch adds node for mmc/sd controller found on Action Semi OWL
S700 SoC.

Since, upstream Linux binding has not been merged for S700 MMC/SD
controller, Changes are put in u-boot specific dtsi file.

Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
---
Changes since previous versions
        * No change.
---
 arch/arm/dts/s700-u-boot.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/dts/s700-u-boot.dtsi b/arch/arm/dts/s700-u-boot.dtsi
index 1b27682..3c3396b 100644
--- a/arch/arm/dts/s700-u-boot.dtsi
+++ b/arch/arm/dts/s700-u-boot.dtsi
@@ -19,6 +19,16 @@
 			status = "okay";
                 };
 
+		mmc0: mmc at e0210000 {
+			compatible = "actions,s700-mmc", "actions,owl-mmc";
+			reg = <0x0 0xe0210000 0x0 0x4000>;
+			interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cmu CLK_SD0>;
+			dmas = <&dma 2>;
+			dma-names = "mmc";
+			bus-width = <4>;
+			status = "okay";
+		};
 	};
 };
 
-- 
2.7.4

[PATCH v3 5/6] mmc: actions: add MMC driver for Actions OWL S700/S900

[Amit Singh Tomar]

From: Amit Singh Tomar <amittomer25@gmail.com>

This commit adds support for MMC controllers found on Actions OWL
SoC platform(S700/S900).

Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
---
Changes since v2:
        * Progammed read/write delays as suggested by
          Jaehoon, its more readable now.
        * Updated commit message and source file to reflect
          it supports S900 as well.
        * Updated SD_DMA_CHANNEL macro to include channel offset.
        * Checked the command response Error based on SDC STATU register
          bits.
        * Replaced switch/case with if/else to correlate with MMC_RSP_Rx bitmask
	  values, as suggested by Andre.
        * Introduced clk_disable based on mmc->clk_disable flag.
        * Guarded the clk_set_rate under if statment to avoid reprogramming the
          rate.
        * Fetched the DRQ number from using args.args[0].
Changes since previous version
        * Corrected block count to 512.
        * Changed the command timeout value to 30ms.
        * Used readl_poll_timeout.
        * Read DMA parameters from DT instead of hardcoding it.
        * Reduced number of arguments passed to own_dma_cofig.
        * Removed debug leftover.
        * Used mmc_of_parse().
---
 drivers/mmc/Kconfig   |   7 +
 drivers/mmc/Makefile  |   1 +
 drivers/mmc/owl_mmc.c | 401 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 409 insertions(+)
 create mode 100644 drivers/mmc/owl_mmc.c

diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
index 14d7913..3478a4e 100644
--- a/drivers/mmc/Kconfig
+++ b/drivers/mmc/Kconfig
@@ -289,6 +289,13 @@ config MMC_MXC
 
 	  If unsure, say N.
 
+config MMC_OWL
+	bool "Actions OWL Multimedia Card Interface support"
+	depends on ARCH_OWL && DM_MMC && BLK
+	help
+	  This selects the OWL SD/MMC host controller found on board
+	  based on Actions S700/S900 SoC.
+
 config MMC_MXS
 	bool "Freescale MXS Multimedia Card Interface support"
 	depends on MX23 || MX28 || MX6 || MX7
diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile
index 1c849cb..f270f6c 100644
--- a/drivers/mmc/Makefile
+++ b/drivers/mmc/Makefile
@@ -38,6 +38,7 @@ obj-$(CONFIG_MMC_OMAP_HS)		+= omap_hsmmc.o
 obj-$(CONFIG_MMC_MXC)			+= mxcmmc.o
 obj-$(CONFIG_MMC_MXS)			+= mxsmmc.o
 obj-$(CONFIG_MMC_OCTEONTX)		+= octeontx_hsmmc.o
+obj-$(CONFIG_MMC_OWL)			+= owl_mmc.o
 obj-$(CONFIG_MMC_PCI)			+= pci_mmc.o
 obj-$(CONFIG_PXA_MMC_GENERIC) += pxa_mmc_gen.o
 obj-$(CONFIG_$(SPL_TPL_)SUPPORT_EMMC_RPMB) += rpmb.o
diff --git a/drivers/mmc/owl_mmc.c b/drivers/mmc/owl_mmc.c
new file mode 100644
index 0000000..de52583
--- /dev/null
+++ b/drivers/mmc/owl_mmc.c
@@ -0,0 +1,401 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2020 Amit Singh Tomar <amittomer25@gmail.com>
+ *
+ * Driver for SD/MMC controller present on Actions Semi S700/S900 SoC, based
+ * on Linux Driver "drivers/mmc/host/owl-mmc.c".
+ *
+ * Though, there is a bit (BSEL, BUS or DMA Special Channel Selection) that
+ * controls the data transfer from SDx_DAT register either using CPU AHB Bus
+ * or DMA channel, but seems like, it only works correctly using external DMA
+ * channel, and those special bits used in this driver is picked from vendor
+ * source exclusively for MMC/SD.
+ */
+#include <common.h>
+#include <clk.h>
+#include <cpu_func.h>
+#include <dm.h>
+#include <errno.h>
+#include <log.h>
+#include <mmc.h>
+#include <asm/io.h>
+#include <linux/bitops.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/iopoll.h>
+
+/*
+ * SDC registers
+ */
+#define OWL_REG_SD_EN			0x0000
+#define OWL_REG_SD_CTL			0x0004
+#define OWL_REG_SD_STATE		0x0008
+#define OWL_REG_SD_CMD			0x000c
+#define OWL_REG_SD_ARG			0x0010
+#define OWL_REG_SD_RSPBUF0		0x0014
+#define OWL_REG_SD_RSPBUF1		0x0018
+#define OWL_REG_SD_RSPBUF2		0x001c
+#define OWL_REG_SD_RSPBUF3		0x0020
+#define OWL_REG_SD_RSPBUF4		0x0024
+#define OWL_REG_SD_DAT			0x0028
+#define OWL_REG_SD_BLK_SIZE		0x002c
+#define OWL_REG_SD_BLK_NUM		0x0030
+#define OWL_REG_SD_BUF_SIZE		0x0034
+
+/* SD_EN Bits */
+#define OWL_SD_EN_RANE			BIT(31)
+#define OWL_SD_EN_RESE			BIT(10)
+#define OWL_SD_ENABLE			BIT(7)
+#define OWL_SD_EN_BSEL			BIT(6)
+#define OWL_SD_EN_DATAWID(x)		(((x) & 0x3) << 0)
+#define OWL_SD_EN_DATAWID_MASK		0x03
+
+/* SD_CTL Bits */
+#define OWL_SD_CTL_TOUTEN		BIT(31)
+#define OWL_SD_CTL_DELAY_MSK		GENMASK(23, 16)
+#define OWL_SD_CTL_RDELAY(x)		(((x) & 0xf) << 20)
+#define OWL_SD_CTL_WDELAY(x)		(((x) & 0xf) << 16)
+#define OWL_SD_CTL_TS			BIT(7)
+#define OWL_SD_CTL_LBE			BIT(6)
+#define OWL_SD_CTL_TM(x)		(((x) & 0xf) << 0)
+
+#define OWL_SD_DELAY_LOW_CLK		0x0f
+#define OWL_SD_DELAY_MID_CLK		0x0a
+#define OWL_SD_RDELAY_HIGH		0x08
+#define OWL_SD_WDELAY_HIGH		0x09
+
+/* SD_STATE Bits */
+#define OWL_SD_STATE_DAT0S		BIT(7)
+#define OWL_SD_STATE_CLNR		BIT(4)
+#define OWL_SD_STATE_CRC7ER		BIT(0)
+
+#define OWL_MMC_OCR			(MMC_VDD_32_33 | MMC_VDD_33_34 | \
+					 MMC_VDD_165_195)
+
+#define DATA_TRANSFER_TIMEOUT		30000
+
+/*
+ * Simple DMA transfer operations defines for MMC/SD card
+ */
+#define SD_DMA_CHANNEL(base, channel)	((base) + 0x100 + 0x100 * (channel))
+
+#define DMA_MODE			0x0000
+#define DMA_SOURCE			0x0004
+#define DMA_DESTINATION			0x0008
+#define DMA_FRAME_LEN			0x000C
+#define DMA_FRAME_CNT			0x0010
+#define DMA_START			0x0024
+
+/* DMAx_MODE */
+#define DMA_MODE_ST(x)                  (((x) & 0x3) << 8)
+#define DMA_MODE_ST_DEV                 DMA_MODE_ST(0)
+#define DMA_MODE_DT(x)                  (((x) & 0x3) << 10)
+#define DMA_MODE_DT_DCU                 DMA_MODE_DT(2)
+#define DMA_MODE_SAM(x)                 (((x) & 0x3) << 16)
+#define DMA_MODE_SAM_CONST              DMA_MODE_SAM(0)
+#define DMA_MODE_DAM(x)                 (((x) & 0x3) << 18)
+#define DMA_MODE_DAM_INC                DMA_MODE_DAM(1)
+
+struct owl_mmc_plat {
+	struct mmc_config cfg;
+	struct mmc mmc;
+};
+
+struct owl_mmc_priv {
+	void *reg_base;
+	void *dma_channel;
+	struct clk clk;
+	unsigned int clock;	  /* Current clock */
+	unsigned int dma_drq;	  /* Trigger Source */
+};
+
+static void owl_dma_config(struct owl_mmc_priv *priv, unsigned int src,
+			   unsigned int dst, unsigned int len)
+{
+	unsigned int mode = priv->dma_drq;
+
+	/* Set Source and Destination adderess mode */
+	mode |= (DMA_MODE_ST_DEV | DMA_MODE_SAM_CONST | DMA_MODE_DT_DCU |
+			DMA_MODE_DAM_INC);
+
+	writel(mode, SD_DMA_CHANNEL(priv->dma_channel, 0) + DMA_MODE);
+	writel(src, SD_DMA_CHANNEL(priv->dma_channel, 0)  + DMA_SOURCE);
+	writel(dst, SD_DMA_CHANNEL(priv->dma_channel, 0)  + DMA_DESTINATION);
+	writel(len, SD_DMA_CHANNEL(priv->dma_channel, 0)  + DMA_FRAME_LEN);
+	writel(0x1, SD_DMA_CHANNEL(priv->dma_channel, 0)  + DMA_FRAME_CNT);
+}
+
+static void owl_mmc_prepare_data(struct owl_mmc_priv *priv,
+				 struct mmc_data *data)
+{
+	unsigned int reg, total;
+	u32 buf = 0;
+
+	reg = readl(priv->reg_base + OWL_REG_SD_EN);
+	reg |= OWL_SD_EN_BSEL;
+	writel(reg, priv->reg_base + OWL_REG_SD_EN);
+
+	writel(data->blocks, priv->reg_base + OWL_REG_SD_BLK_NUM);
+	writel(data->blocksize, priv->reg_base + OWL_REG_SD_BLK_SIZE);
+	total = data->blocksize * data->blocks;
+
+	if (data->blocksize < 512)
+		writel(total, priv->reg_base + OWL_REG_SD_BUF_SIZE);
+	else
+		writel(512, priv->reg_base + OWL_REG_SD_BUF_SIZE);
+
+	/* DMA STOP */
+	writel(0x0, SD_DMA_CHANNEL(priv->dma_channel, 0) + DMA_START);
+
+	if (data) {
+		if (data->flags == MMC_DATA_READ) {
+			buf = (ulong) (data->dest);
+			owl_dma_config(priv, (ulong) priv->reg_base +
+				       OWL_REG_SD_DAT, buf, total);
+			invalidate_dcache_range(buf, buf + total);
+		} else {
+			buf = (ulong) (data->src);
+			owl_dma_config(priv, buf, (ulong) priv->reg_base +
+				       OWL_REG_SD_DAT, total);
+			flush_dcache_range(buf, buf + total);
+		}
+		/* DMA START */
+		writel(0x1, SD_DMA_CHANNEL(priv->dma_channel, 0) + DMA_START);
+	}
+}
+
+static int owl_mmc_send_cmd(struct udevice *dev, struct mmc_cmd *cmd,
+			    struct mmc_data *data)
+{
+	struct owl_mmc_priv *priv = dev_get_priv(dev);
+	unsigned int cmd_rsp_mask, mode, reg;
+	int ret;
+
+	reg = readl(priv->reg_base + OWL_REG_SD_EN);
+	reg |= OWL_SD_ENABLE;
+	writel(reg, priv->reg_base + OWL_REG_SD_EN);
+
+	/* setup response */
+	if (cmd->resp_type == MMC_RSP_NONE)
+		mode = OWL_SD_CTL_TM(0);
+	else {
+		cmd_rsp_mask = OWL_SD_STATE_CLNR | OWL_SD_STATE_CRC7ER;
+
+		if (cmd->resp_type == MMC_RSP_R1) {
+			if (data) {
+				if (data->flags == MMC_DATA_READ)
+					mode |= OWL_SD_CTL_TM(4);
+				else
+					mode |= OWL_SD_CTL_TM(5);
+			} else
+				mode |= OWL_SD_CTL_TM(1);
+		} else if (cmd->resp_type == MMC_RSP_R2) {
+			mode = OWL_SD_CTL_TM(2);
+		} else if (cmd->resp_type == MMC_RSP_R1b) {
+			mode = OWL_SD_CTL_TM(3);
+		} else if (cmd->resp_type == MMC_RSP_R3) {
+			cmd_rsp_mask = OWL_SD_STATE_CLNR;
+			mode = OWL_SD_CTL_TM(1);
+		}
+	}
+
+	mode |= (readl(priv->reg_base + OWL_REG_SD_CTL) & (0xff << 16));
+
+	/* setup command */
+	writel(cmd->cmdidx, priv->reg_base + OWL_REG_SD_CMD);
+	writel(cmd->cmdarg, priv->reg_base + OWL_REG_SD_ARG);
+
+	/* Set LBE to send clk at the end of last read block */
+	if (data)
+		mode |= (OWL_SD_CTL_TS | OWL_SD_CTL_LBE | 0xE4000000);
+	else
+		mode |= OWL_SD_CTL_TS;
+
+	if (data)
+		owl_mmc_prepare_data(priv, data);
+
+	/* Start transfer */
+	writel(mode, priv->reg_base + OWL_REG_SD_CTL);
+
+	ret = readl_poll_timeout(priv->reg_base + OWL_REG_SD_CTL, reg,
+				 !(reg & OWL_SD_CTL_TS), DATA_TRANSFER_TIMEOUT);
+
+	if (ret == -ETIMEDOUT) {
+		debug("error: transferred data timeout\n");
+		return ret;
+	}
+
+	reg = readl(priv->reg_base + OWL_REG_SD_STATE);
+	if (cmd->resp_type & MMC_RSP_PRESENT) {
+		if (cmd_rsp_mask & reg) {
+			if (reg & OWL_SD_STATE_CLNR) {
+				printf("Error CMD_NO_RSP\n");
+				return -1;
+			}
+
+			if (reg & OWL_SD_STATE_CRC7ER) {
+				printf("Error CMD_RSP_CRC\n");
+				return -1;
+			}
+
+		}
+
+		if (cmd->resp_type & MMC_RSP_136) {
+			cmd->response[3] = readl(priv->reg_base + OWL_REG_SD_RSPBUF0);
+			cmd->response[2] = readl(priv->reg_base + OWL_REG_SD_RSPBUF1);
+			cmd->response[1] = readl(priv->reg_base + OWL_REG_SD_RSPBUF2);
+			cmd->response[0] = readl(priv->reg_base + OWL_REG_SD_RSPBUF3);
+		} else {
+			u32 rsp[2];
+
+			rsp[0] = readl(priv->reg_base + OWL_REG_SD_RSPBUF0);
+			rsp[1] = readl(priv->reg_base + OWL_REG_SD_RSPBUF1);
+			cmd->response[0] = rsp[1] << 24 | rsp[0] >> 8;
+			cmd->response[1] = rsp[1] >> 8;
+		}
+	}
+
+	if (data) {
+		/* DMA STOP */
+		writel(0x0, SD_DMA_CHANNEL(priv->dma_channel, 0) + DMA_START);
+		/* Transmission STOP */
+		while (readl(priv->reg_base + OWL_REG_SD_CTL) & OWL_SD_CTL_TS)
+			clrbits_le32(priv->reg_base + OWL_REG_SD_CTL,
+					OWL_SD_CTL_TS);
+	}
+
+	return 0;
+}
+
+static void owl_mmc_clk_set(struct owl_mmc_priv *priv, int rate)
+{
+	u32 reg, wdelay, rdelay;
+
+	reg = readl(priv->reg_base + OWL_REG_SD_CTL);
+	reg &= ~OWL_SD_CTL_DELAY_MSK;
+
+	/* Set RDELAY and WDELAY based on the clock */
+	if (rate <= 1000000)
+		rdelay = wdelay = OWL_SD_DELAY_LOW_CLK;
+	else if ((rate > 1000000) && (rate <= 26000000))
+		rdelay = wdelay = OWL_SD_DELAY_MID_CLK;
+	else if ((rate > 26000000) && (rate <= 52000000)) {
+		rdelay = OWL_SD_RDELAY_HIGH;
+		wdelay = OWL_SD_WDELAY_HIGH;
+	} else
+		debug("SD clock rate not supported\n");
+
+	writel(reg | OWL_SD_CTL_RDELAY(rdelay) | OWL_SD_CTL_WDELAY(wdelay),
+	       priv->reg_base + OWL_REG_SD_CTL);
+}
+
+static int owl_mmc_set_ios(struct udevice *dev)
+{
+	struct owl_mmc_priv *priv = dev_get_priv(dev);
+	struct owl_mmc_plat *plat = dev_get_platdata(dev);
+	struct mmc *mmc = &plat->mmc;
+	u32 reg, ret;
+
+	if (mmc->clock != priv->clock) {
+		priv->clock = mmc->clock;
+		owl_mmc_clk_set(priv, mmc->clock);
+
+		ret = clk_set_rate(&priv->clk, mmc->clock);
+		if (IS_ERR_VALUE(ret))
+			return ret;
+	}
+
+	if (mmc->clk_disable)
+		ret = clk_disable(&priv->clk);
+	else
+		ret = clk_enable(&priv->clk);
+	if (ret)
+		return ret;
+
+	/* Set the Bus width */
+	reg = readl(priv->reg_base + OWL_REG_SD_EN);
+	reg &= ~OWL_SD_EN_DATAWID_MASK;
+	if (mmc->bus_width == 8)
+		reg |= OWL_SD_EN_DATAWID(2);
+	else if (mmc->bus_width == 4)
+		reg |= OWL_SD_EN_DATAWID(1);
+
+	writel(reg, priv->reg_base + OWL_REG_SD_EN);
+
+	return 0;
+}
+
+static const struct dm_mmc_ops owl_mmc_ops = {
+	.send_cmd       = owl_mmc_send_cmd,
+	.set_ios        = owl_mmc_set_ios,
+};
+
+static int owl_mmc_probe(struct udevice *dev)
+{
+	struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev);
+	struct owl_mmc_plat *plat = dev_get_platdata(dev);
+	struct owl_mmc_priv *priv = dev_get_priv(dev);
+	struct mmc_config *cfg = &plat->cfg;
+	struct ofnode_phandle_args args;
+	int ret;
+	fdt_addr_t addr;
+
+	cfg->name = dev->name;
+	cfg->voltages = OWL_MMC_OCR;
+	cfg->f_min = 400000;
+	cfg->f_max = 52000000;
+	cfg->b_max = 512;
+	cfg->host_caps = MMC_MODE_HS | MMC_MODE_HS_52MHz;
+
+	ret = mmc_of_parse(dev, cfg);
+	if (ret)
+		return ret;
+
+	addr = dev_read_addr(dev);
+	if (addr == FDT_ADDR_T_NONE)
+		return -EINVAL;
+
+	priv->reg_base = (void *)addr;
+
+	ret = dev_read_phandle_with_args(dev, "dmas", "#dma-cells", 0, 0,
+					 &args);
+	if (ret)
+		return ret;
+
+	priv->dma_channel = (void *)ofnode_get_addr(args.node);
+	priv->dma_drq	  = args.args[0];
+
+	ret = clk_get_by_index(dev, 0, &priv->clk);
+	if (ret) {
+		debug("clk_get_by_index() failed: %d\n", ret);
+		return ret;
+	}
+
+	upriv->mmc = &plat->mmc;
+
+	return 0;
+}
+
+static int owl_mmc_bind(struct udevice *dev)
+{
+	struct owl_mmc_plat *plat = dev_get_platdata(dev);
+
+	return mmc_bind(dev, &plat->mmc, &plat->cfg);
+}
+
+static const struct udevice_id owl_mmc_ids[] = {
+	{ .compatible = "actions,s700-mmc" },
+	{ .compatible = "actions,owl-mmc" },
+	{ }
+};
+
+U_BOOT_DRIVER(owl_mmc_drv) = {
+	.name           = "owl_mmc",
+	.id             = UCLASS_MMC,
+	.of_match       = owl_mmc_ids,
+	.bind           = owl_mmc_bind,
+	.probe          = owl_mmc_probe,
+	.ops            = &owl_mmc_ops,
+	.platdata_auto_alloc_size = sizeof(struct owl_mmc_plat),
+	.priv_auto_alloc_size = sizeof(struct owl_mmc_priv),
+};
-- 
2.7.4

[PATCH v3 6/6] configs: Enable mmc support

[Amit Singh Tomar]

From: Amit Singh Tomar <amittomer25@gmail.com>

This commits enables mmc on the Actions Cubieboard7 board.

Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
---
Changes since previous versions
        * No change.
---
 configs/cubieboard7_defconfig | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/configs/cubieboard7_defconfig b/configs/cubieboard7_defconfig
index 64dc593..d1ee862 100644
--- a/configs/cubieboard7_defconfig
+++ b/configs/cubieboard7_defconfig
@@ -14,3 +14,6 @@ CONFIG_PHY_REALTEK=y
 CONFIG_RTL8201F_PHY_S700_RMII_TIMINGS=y
 CONFIG_ETH_DESIGNWARE=y
 CONFIG_ETH_DESIGNWARE_S700=y
+CONFIG_DM_MMC=y
+CONFIG_MMC_OWL=y
+CONFIG_CMD_MMC=y
-- 
2.7.4

[PATCH v3 5/6] mmc: actions: add MMC driver for Actions OWL S700/S900

[André Przywara]

On 16/01/2021 18:32, Amit Singh Tomar wrote:
> From: Amit Singh Tomar <amittomer25@gmail.com>

Hi,

> This commit adds support for MMC controllers found on Actions OWL
> SoC platform(S700/S900).
> 
> Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
> ---
> Changes since v2:
>         * Progammed read/write delays as suggested by
>           Jaehoon, its more readable now.
>         * Updated commit message and source file to reflect
>           it supports S900 as well.
>         * Updated SD_DMA_CHANNEL macro to include channel offset.
>         * Checked the command response Error based on SDC STATU register
>           bits.
>         * Replaced switch/case with if/else to correlate with MMC_RSP_Rx bitmask
> 	  values, as suggested by Andre.
>         * Introduced clk_disable based on mmc->clk_disable flag.
>         * Guarded the clk_set_rate under if statment to avoid reprogramming the
>           rate.
>         * Fetched the DRQ number from using args.args[0].
> Changes since previous version
>         * Corrected block count to 512.
>         * Changed the command timeout value to 30ms.
>         * Used readl_poll_timeout.
>         * Read DMA parameters from DT instead of hardcoding it.
>         * Reduced number of arguments passed to own_dma_cofig.
>         * Removed debug leftover.
>         * Used mmc_of_parse().
> ---
>  drivers/mmc/Kconfig   |   7 +
>  drivers/mmc/Makefile  |   1 +
>  drivers/mmc/owl_mmc.c | 401 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 409 insertions(+)
>  create mode 100644 drivers/mmc/owl_mmc.c
> 
> diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
> index 14d7913..3478a4e 100644
> --- a/drivers/mmc/Kconfig
> +++ b/drivers/mmc/Kconfig
> @@ -289,6 +289,13 @@ config MMC_MXC
>  
>  	  If unsure, say N.
>  
> +config MMC_OWL
> +	bool "Actions OWL Multimedia Card Interface support"
> +	depends on ARCH_OWL && DM_MMC && BLK
> +	help
> +	  This selects the OWL SD/MMC host controller found on board
> +	  based on Actions S700/S900 SoC.
> +
>  config MMC_MXS
>  	bool "Freescale MXS Multimedia Card Interface support"
>  	depends on MX23 || MX28 || MX6 || MX7
> diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile
> index 1c849cb..f270f6c 100644
> --- a/drivers/mmc/Makefile
> +++ b/drivers/mmc/Makefile
> @@ -38,6 +38,7 @@ obj-$(CONFIG_MMC_OMAP_HS)		+= omap_hsmmc.o
>  obj-$(CONFIG_MMC_MXC)			+= mxcmmc.o
>  obj-$(CONFIG_MMC_MXS)			+= mxsmmc.o
>  obj-$(CONFIG_MMC_OCTEONTX)		+= octeontx_hsmmc.o
> +obj-$(CONFIG_MMC_OWL)			+= owl_mmc.o
>  obj-$(CONFIG_MMC_PCI)			+= pci_mmc.o
>  obj-$(CONFIG_PXA_MMC_GENERIC) += pxa_mmc_gen.o
>  obj-$(CONFIG_$(SPL_TPL_)SUPPORT_EMMC_RPMB) += rpmb.o
> diff --git a/drivers/mmc/owl_mmc.c b/drivers/mmc/owl_mmc.c
> new file mode 100644
> index 0000000..de52583
> --- /dev/null
> +++ b/drivers/mmc/owl_mmc.c
> @@ -0,0 +1,401 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2020 Amit Singh Tomar <amittomer25@gmail.com>
> + *
> + * Driver for SD/MMC controller present on Actions Semi S700/S900 SoC, based
> + * on Linux Driver "drivers/mmc/host/owl-mmc.c".
> + *
> + * Though, there is a bit (BSEL, BUS or DMA Special Channel Selection) that
> + * controls the data transfer from SDx_DAT register either using CPU AHB Bus
> + * or DMA channel, but seems like, it only works correctly using external DMA
> + * channel, and those special bits used in this driver is picked from vendor
> + * source exclusively for MMC/SD.
> + */
> +#include <common.h>
> +#include <clk.h>
> +#include <cpu_func.h>
> +#include <dm.h>
> +#include <errno.h>
> +#include <log.h>
> +#include <mmc.h>
> +#include <asm/io.h>
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/iopoll.h>
> +
> +/*
> + * SDC registers
> + */
> +#define OWL_REG_SD_EN			0x0000
> +#define OWL_REG_SD_CTL			0x0004
> +#define OWL_REG_SD_STATE		0x0008
> +#define OWL_REG_SD_CMD			0x000c
> +#define OWL_REG_SD_ARG			0x0010
> +#define OWL_REG_SD_RSPBUF0		0x0014
> +#define OWL_REG_SD_RSPBUF1		0x0018
> +#define OWL_REG_SD_RSPBUF2		0x001c
> +#define OWL_REG_SD_RSPBUF3		0x0020
> +#define OWL_REG_SD_RSPBUF4		0x0024
> +#define OWL_REG_SD_DAT			0x0028
> +#define OWL_REG_SD_BLK_SIZE		0x002c
> +#define OWL_REG_SD_BLK_NUM		0x0030
> +#define OWL_REG_SD_BUF_SIZE		0x0034
> +
> +/* SD_EN Bits */
> +#define OWL_SD_EN_RANE			BIT(31)
> +#define OWL_SD_EN_RESE			BIT(10)
> +#define OWL_SD_ENABLE			BIT(7)
> +#define OWL_SD_EN_BSEL			BIT(6)
> +#define OWL_SD_EN_DATAWID(x)		(((x) & 0x3) << 0)
> +#define OWL_SD_EN_DATAWID_MASK		0x03
> +
> +/* SD_CTL Bits */
> +#define OWL_SD_CTL_TOUTEN		BIT(31)
> +#define OWL_SD_CTL_DELAY_MSK		GENMASK(23, 16)
> +#define OWL_SD_CTL_RDELAY(x)		(((x) & 0xf) << 20)
> +#define OWL_SD_CTL_WDELAY(x)		(((x) & 0xf) << 16)
> +#define OWL_SD_CTL_TS			BIT(7)
> +#define OWL_SD_CTL_LBE			BIT(6)
> +#define OWL_SD_CTL_TM(x)		(((x) & 0xf) << 0)
> +
> +#define OWL_SD_DELAY_LOW_CLK		0x0f
> +#define OWL_SD_DELAY_MID_CLK		0x0a
> +#define OWL_SD_RDELAY_HIGH		0x08
> +#define OWL_SD_WDELAY_HIGH		0x09
> +
> +/* SD_STATE Bits */
> +#define OWL_SD_STATE_DAT0S		BIT(7)
> +#define OWL_SD_STATE_CLNR		BIT(4)
> +#define OWL_SD_STATE_CRC7ER		BIT(0)
> +
> +#define OWL_MMC_OCR			(MMC_VDD_32_33 | MMC_VDD_33_34 | \
> +					 MMC_VDD_165_195)
> +
> +#define DATA_TRANSFER_TIMEOUT		30000
> +
> +/*
> + * Simple DMA transfer operations defines for MMC/SD card
> + */
> +#define SD_DMA_CHANNEL(base, channel)	((base) + 0x100 + 0x100 * (channel))
> +
> +#define DMA_MODE			0x0000
> +#define DMA_SOURCE			0x0004
> +#define DMA_DESTINATION			0x0008
> +#define DMA_FRAME_LEN			0x000C
> +#define DMA_FRAME_CNT			0x0010
> +#define DMA_START			0x0024
> +
> +/* DMAx_MODE */

There are still spaces in the below section.

> +#define DMA_MODE_ST(x)                  (((x) & 0x3) << 8)
> +#define DMA_MODE_ST_DEV                 DMA_MODE_ST(0)
> +#define DMA_MODE_DT(x)                  (((x) & 0x3) << 10)
> +#define DMA_MODE_DT_DCU                 DMA_MODE_DT(2)
> +#define DMA_MODE_SAM(x)                 (((x) & 0x3) << 16)
> +#define DMA_MODE_SAM_CONST              DMA_MODE_SAM(0)
> +#define DMA_MODE_DAM(x)                 (((x) & 0x3) << 18)
> +#define DMA_MODE_DAM_INC                DMA_MODE_DAM(1)
> +
> +struct owl_mmc_plat {
> +	struct mmc_config cfg;
> +	struct mmc mmc;
> +};
> +
> +struct owl_mmc_priv {
> +	void *reg_base;
> +	void *dma_channel;
> +	struct clk clk;
> +	unsigned int clock;	  /* Current clock */
> +	unsigned int dma_drq;	  /* Trigger Source */
> +};
> +
> +static void owl_dma_config(struct owl_mmc_priv *priv, unsigned int src,
> +			   unsigned int dst, unsigned int len)
> +{
> +	unsigned int mode = priv->dma_drq;
> +
> +	/* Set Source and Destination adderess mode */

				      address

> +	mode |= (DMA_MODE_ST_DEV | DMA_MODE_SAM_CONST | DMA_MODE_DT_DCU |
> +			DMA_MODE_DAM_INC);
> +
> +	writel(mode, SD_DMA_CHANNEL(priv->dma_channel, 0) + DMA_MODE);
> +	writel(src, SD_DMA_CHANNEL(priv->dma_channel, 0)  + DMA_SOURCE);
> +	writel(dst, SD_DMA_CHANNEL(priv->dma_channel, 0)  + DMA_DESTINATION);
> +	writel(len, SD_DMA_CHANNEL(priv->dma_channel, 0)  + DMA_FRAME_LEN);
> +	writel(0x1, SD_DMA_CHANNEL(priv->dma_channel, 0)  + DMA_FRAME_CNT);
> +}
> +
> +static void owl_mmc_prepare_data(struct owl_mmc_priv *priv,
> +				 struct mmc_data *data)
> +{
> +	unsigned int reg, total;
> +	u32 buf = 0;
> +
> +	reg = readl(priv->reg_base + OWL_REG_SD_EN);
> +	reg |= OWL_SD_EN_BSEL;
> +	writel(reg, priv->reg_base + OWL_REG_SD_EN);

Can't you use setbits_le32() here? The same to all the other occasions
where you just set or clear some constant bits in a register.

> +
> +	writel(data->blocks, priv->reg_base + OWL_REG_SD_BLK_NUM);
> +	writel(data->blocksize, priv->reg_base + OWL_REG_SD_BLK_SIZE);
> +	total = data->blocksize * data->blocks;
> +
> +	if (data->blocksize < 512)
> +		writel(total, priv->reg_base + OWL_REG_SD_BUF_SIZE);

Is that correct? I would think you either check for total being smaller
than 512, or you write data->blocksize into the register.

> +	else
> +		writel(512, priv->reg_base + OWL_REG_SD_BUF_SIZE);
> +
> +	/* DMA STOP */
> +	writel(0x0, SD_DMA_CHANNEL(priv->dma_channel, 0) + DMA_START);
> +
> +	if (data) {
> +		if (data->flags == MMC_DATA_READ) {
> +			buf = (ulong) (data->dest);
> +			owl_dma_config(priv, (ulong) priv->reg_base +
> +				       OWL_REG_SD_DAT, buf, total);
> +			invalidate_dcache_range(buf, buf + total);
> +		} else {
> +			buf = (ulong) (data->src);
> +			owl_dma_config(priv, buf, (ulong) priv->reg_base +
> +				       OWL_REG_SD_DAT, total);
> +			flush_dcache_range(buf, buf + total);
> +		}
> +		/* DMA START */
> +		writel(0x1, SD_DMA_CHANNEL(priv->dma_channel, 0) + DMA_START);
> +	}
> +}
> +
> +static int owl_mmc_send_cmd(struct udevice *dev, struct mmc_cmd *cmd,
> +			    struct mmc_data *data)
> +{
> +	struct owl_mmc_priv *priv = dev_get_priv(dev);
> +	unsigned int cmd_rsp_mask, mode, reg;
> +	int ret;
> +
> +	reg = readl(priv->reg_base + OWL_REG_SD_EN);
> +	reg |= OWL_SD_ENABLE;
> +	writel(reg, priv->reg_base + OWL_REG_SD_EN);
> +
> +	/* setup response */
> +	if (cmd->resp_type == MMC_RSP_NONE)
> +		mode = OWL_SD_CTL_TM(0);

So this whole mode handling here looks dodgy. Below you mix "assignments
to mode" with "ORing in values", without actually ever initialising mode
explicitly. I wonder why the compiler doesn't warn about this, I can see
paths were you OR into an uninitialised value.

Also, the above two lines are actually: if (resp_type == 0) mode = 0;
So I would suggest:
	mode = 0;
	if (cmd->resp_type != MMC_RSP_NONE)
		cmd_rsp_mask = OWL_SD_STATE_CLNR | OWL_SD_STATE_CRC7ER;

	if (md->resp_type == MMC_RSP_R1) {
		.... (always assign to "mode")


(I haven't checked yet whether the actual combinations are correct.)

> +	else {
> +		cmd_rsp_mask = OWL_SD_STATE_CLNR | OWL_SD_STATE_CRC7ER;
> +
> +		if (cmd->resp_type == MMC_RSP_R1) {
> +			if (data) {
> +				if (data->flags == MMC_DATA_READ)
> +					mode |= OWL_SD_CTL_TM(4);
> +				else
> +					mode |= OWL_SD_CTL_TM(5);
> +			} else
> +				mode |= OWL_SD_CTL_TM(1);
> +		} else if (cmd->resp_type == MMC_RSP_R2) {
> +			mode = OWL_SD_CTL_TM(2);
> +		} else if (cmd->resp_type == MMC_RSP_R1b) {
> +			mode = OWL_SD_CTL_TM(3);
> +		} else if (cmd->resp_type == MMC_RSP_R3) {
> +			cmd_rsp_mask = OWL_SD_STATE_CLNR;
> +			mode = OWL_SD_CTL_TM(1);
> +		}
> +	}
> +
> +	mode |= (readl(priv->reg_base + OWL_REG_SD_CTL) & (0xff << 16));
> +
> +	/* setup command */
> +	writel(cmd->cmdidx, priv->reg_base + OWL_REG_SD_CMD);
> +	writel(cmd->cmdarg, priv->reg_base + OWL_REG_SD_ARG);
> +
> +	/* Set LBE to send clk at the end of last read block */
> +	if (data)
> +		mode |= (OWL_SD_CTL_TS | OWL_SD_CTL_LBE | 0xE4000000);
> +	else
> +		mode |= OWL_SD_CTL_TS;
> +
> +	if (data)
> +		owl_mmc_prepare_data(priv, data);
> +
> +	/* Start transfer */
> +	writel(mode, priv->reg_base + OWL_REG_SD_CTL);
> +
> +	ret = readl_poll_timeout(priv->reg_base + OWL_REG_SD_CTL, reg,
> +				 !(reg & OWL_SD_CTL_TS), DATA_TRANSFER_TIMEOUT);
> +
> +	if (ret == -ETIMEDOUT) {
> +		debug("error: transferred data timeout\n");
> +		return ret;
> +	}
> +
> +	reg = readl(priv->reg_base + OWL_REG_SD_STATE);
> +	if (cmd->resp_type & MMC_RSP_PRESENT) {
> +		if (cmd_rsp_mask & reg) {

Can't you mask the bits in reg, then save the outer if?

> +			if (reg & OWL_SD_STATE_CLNR) {
> +				printf("Error CMD_NO_RSP\n");
> +				return -1;
> +			}
> +
> +			if (reg & OWL_SD_STATE_CRC7ER) {
> +				printf("Error CMD_RSP_CRC\n");
> +				return -1;
> +			}
> +
> +		}
> +
> +		if (cmd->resp_type & MMC_RSP_136) {
> +			cmd->response[3] = readl(priv->reg_base + OWL_REG_SD_RSPBUF0);
> +			cmd->response[2] = readl(priv->reg_base + OWL_REG_SD_RSPBUF1);
> +			cmd->response[1] = readl(priv->reg_base + OWL_REG_SD_RSPBUF2);
> +			cmd->response[0] = readl(priv->reg_base + OWL_REG_SD_RSPBUF3);
> +		} else {
> +			u32 rsp[2];
> +
> +			rsp[0] = readl(priv->reg_base + OWL_REG_SD_RSPBUF0);
> +			rsp[1] = readl(priv->reg_base + OWL_REG_SD_RSPBUF1);
> +			cmd->response[0] = rsp[1] << 24 | rsp[0] >> 8;
> +			cmd->response[1] = rsp[1] >> 8;
> +		}
> +	}
> +
> +	if (data) {
> +		/* DMA STOP */
> +		writel(0x0, SD_DMA_CHANNEL(priv->dma_channel, 0) + DMA_START);
> +		/* Transmission STOP */
> +		while (readl(priv->reg_base + OWL_REG_SD_CTL) & OWL_SD_CTL_TS)
> +			clrbits_le32(priv->reg_base + OWL_REG_SD_CTL,
> +					OWL_SD_CTL_TS);

Should there be a timeout here?

> +	}
> +
> +	return 0;
> +}
> +
> +static void owl_mmc_clk_set(struct owl_mmc_priv *priv, int rate)
> +{
> +	u32 reg, wdelay, rdelay;
> +
> +	reg = readl(priv->reg_base + OWL_REG_SD_CTL);
> +	reg &= ~OWL_SD_CTL_DELAY_MSK;
> +
> +	/* Set RDELAY and WDELAY based on the clock */
> +	if (rate <= 1000000)
> +		rdelay = wdelay = OWL_SD_DELAY_LOW_CLK;
> +	else if ((rate > 1000000) && (rate <= 26000000))
> +		rdelay = wdelay = OWL_SD_DELAY_MID_CLK;
> +	else if ((rate > 26000000) && (rate <= 52000000)) {
> +		rdelay = OWL_SD_RDELAY_HIGH;
> +		wdelay = OWL_SD_WDELAY_HIGH;
> +	} else
> +		debug("SD clock rate not supported\n");

Shouldn't this be more fatal? You still continue to set the register
below. Why not make this function return an int, and check for an error
in the caller?

Cheers,
Andre

> +
> +	writel(reg | OWL_SD_CTL_RDELAY(rdelay) | OWL_SD_CTL_WDELAY(wdelay),
> +	       priv->reg_base + OWL_REG_SD_CTL);
> +}
> +
> +static int owl_mmc_set_ios(struct udevice *dev)
> +{
> +	struct owl_mmc_priv *priv = dev_get_priv(dev);
> +	struct owl_mmc_plat *plat = dev_get_platdata(dev);
> +	struct mmc *mmc = &plat->mmc;
> +	u32 reg, ret;
> +
> +	if (mmc->clock != priv->clock) {
> +		priv->clock = mmc->clock;
> +		owl_mmc_clk_set(priv, mmc->clock);
> +
> +		ret = clk_set_rate(&priv->clk, mmc->clock);
> +		if (IS_ERR_VALUE(ret))
> +			return ret;
> +	}
> +
> +	if (mmc->clk_disable)
> +		ret = clk_disable(&priv->clk);
> +	else
> +		ret = clk_enable(&priv->clk);
> +	if (ret)
> +		return ret;
> +
> +	/* Set the Bus width */
> +	reg = readl(priv->reg_base + OWL_REG_SD_EN);
> +	reg &= ~OWL_SD_EN_DATAWID_MASK;
> +	if (mmc->bus_width == 8)
> +		reg |= OWL_SD_EN_DATAWID(2);
> +	else if (mmc->bus_width == 4)
> +		reg |= OWL_SD_EN_DATAWID(1);
> +
> +	writel(reg, priv->reg_base + OWL_REG_SD_EN);
> +
> +	return 0;
> +}
> +
> +static const struct dm_mmc_ops owl_mmc_ops = {
> +	.send_cmd       = owl_mmc_send_cmd,
> +	.set_ios        = owl_mmc_set_ios,
> +};
> +
> +static int owl_mmc_probe(struct udevice *dev)
> +{
> +	struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev);
> +	struct owl_mmc_plat *plat = dev_get_platdata(dev);
> +	struct owl_mmc_priv *priv = dev_get_priv(dev);
> +	struct mmc_config *cfg = &plat->cfg;
> +	struct ofnode_phandle_args args;
> +	int ret;
> +	fdt_addr_t addr;
> +
> +	cfg->name = dev->name;
> +	cfg->voltages = OWL_MMC_OCR;
> +	cfg->f_min = 400000;
> +	cfg->f_max = 52000000;
> +	cfg->b_max = 512;
> +	cfg->host_caps = MMC_MODE_HS | MMC_MODE_HS_52MHz;
> +
> +	ret = mmc_of_parse(dev, cfg);
> +	if (ret)
> +		return ret;
> +
> +	addr = dev_read_addr(dev);
> +	if (addr == FDT_ADDR_T_NONE)
> +		return -EINVAL;
> +
> +	priv->reg_base = (void *)addr;
> +
> +	ret = dev_read_phandle_with_args(dev, "dmas", "#dma-cells", 0, 0,
> +					 &args);
> +	if (ret)
> +		return ret;
> +
> +	priv->dma_channel = (void *)ofnode_get_addr(args.node);
> +	priv->dma_drq	  = args.args[0];
> +
> +	ret = clk_get_by_index(dev, 0, &priv->clk);
> +	if (ret) {
> +		debug("clk_get_by_index() failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	upriv->mmc = &plat->mmc;
> +
> +	return 0;
> +}
> +
> +static int owl_mmc_bind(struct udevice *dev)
> +{
> +	struct owl_mmc_plat *plat = dev_get_platdata(dev);
> +
> +	return mmc_bind(dev, &plat->mmc, &plat->cfg);
> +}
> +
> +static const struct udevice_id owl_mmc_ids[] = {
> +	{ .compatible = "actions,s700-mmc" },
> +	{ .compatible = "actions,owl-mmc" },
> +	{ }
> +};
> +
> +U_BOOT_DRIVER(owl_mmc_drv) = {
> +	.name           = "owl_mmc",
> +	.id             = UCLASS_MMC,
> +	.of_match       = owl_mmc_ids,
> +	.bind           = owl_mmc_bind,
> +	.probe          = owl_mmc_probe,
> +	.ops            = &owl_mmc_ops,
> +	.platdata_auto_alloc_size = sizeof(struct owl_mmc_plat),
> +	.priv_auto_alloc_size = sizeof(struct owl_mmc_priv),
> +};
> 

[PATCH v3 5/6] mmc: actions: add MMC driver for Actions OWL S700/S900

[Amit Tomar]

Hi,


> So this whole mode handling here looks dodgy. Below you mix "assignments
> to mode" with "ORing in values", without actually ever initialising mode
> explicitly. I wonder why the compiler doesn't warn about this, I can see
> paths were you OR into an uninitialised value.
>
> But the compiler already has initialized mode to 0, that is why there is
no warning.
In order to test , printed out mode value which suggests this variable is
initialized.

Thanks
-Amit

[PATCH v3 5/6] mmc: actions: add MMC driver for Actions OWL S700/S900

[Andre Przywara]

On Mon, 1 Mar 2021 18:47:26 +0530
Amit Tomar <atomar25opensource@gmail.com> wrote:

Hi,

> > So this whole mode handling here looks dodgy. Below you mix "assignments
> > to mode" with "ORing in values", without actually ever initialising mode
> > explicitly. I wonder why the compiler doesn't warn about this, I can see
> > paths were you OR into an uninitialised value.
> >
> > But the compiler already has initialized mode to 0,

Why? Where? I just see a local, non-static definition of mode,
meaning it won't be initialised.

> that is why there is no warning.
> In order to test , printed out mode value which suggests this variable is
> initialized.

To what? Just because you printed 0(?) in your test doesn't mean that
this will always be the case.

Cheers,
Andre

[PATCH v3 5/6] mmc: actions: add MMC driver for Actions OWL S700/S900

[Jaehoon Chung]

On 3/1/21 11:25 PM, Andre Przywara wrote:
> On Mon, 1 Mar 2021 18:47:26 +0530
> Amit Tomar <atomar25opensource@gmail.com> wrote:
> 
> Hi,
> 
>>> So this whole mode handling here looks dodgy. Below you mix "assignments
>>> to mode" with "ORing in values", without actually ever initialising mode
>>> explicitly. I wonder why the compiler doesn't warn about this, I can see
>>> paths were you OR into an uninitialised value.
>>>
>>> But the compiler already has initialized mode to 0,
> 
> Why? Where? I just see a local, non-static definition of mode,
> meaning it won't be initialised.

It seems that it doesn't initialize in this function.
I think that resp_type may be always matched one of condition.
I think that's why the compiler doesn't warn. 

Best Regards,
Jaehoon Chung

> 
>> that is why there is no warning.
>> In order to test , printed out mode value which suggests this variable is
>> initialized.
> 
> To what? Just because you printed 0(?) in your test doesn't mean that
> this will always be the case.
> 
> Cheers,
> Andre
> 

[PATCH v3 5/6] mmc: actions: add MMC driver for Actions OWL S700/S900

[Andre Przywara]

On Tue, 2 Mar 2021 07:11:54 +0900
Jaehoon Chung <jh80.chung@samsung.com> wrote:

> On 3/1/21 11:25 PM, Andre Przywara wrote:
> > On Mon, 1 Mar 2021 18:47:26 +0530
> > Amit Tomar <atomar25opensource@gmail.com> wrote:
> > 
> > Hi,
> >   
> >>> So this whole mode handling here looks dodgy. Below you mix "assignments
> >>> to mode" with "ORing in values", without actually ever initialising mode
> >>> explicitly. I wonder why the compiler doesn't warn about this, I can see
> >>> paths were you OR into an uninitialised value.
> >>>
> >>> But the compiler already has initialized mode to 0,  
> > 
> > Why? Where? I just see a local, non-static definition of mode,
> > meaning it won't be initialised.  
> 
> It seems that it doesn't initialize in this function.
> I think that resp_type may be always matched one of condition.

Possibly, but the problem is that the MMC_RSP_R1 clause also uses
"|=", so it's the same issue here.
One a first glance into the assembly it looks like the compiler
optimises this whole clause away? Undefined behaviour?

Anyway, I am not really sure we need to argue here, when the fix is
dead easy ...

Cheers,
Andre

> I think that's why the compiler doesn't warn. 
> 
> Best Regards,
> Jaehoon Chung
> 
> >   
> >> that is why there is no warning.
> >> In order to test , printed out mode value which suggests this variable is
> >> initialized.  
> > 
> > To what? Just because you printed 0(?) in your test doesn't mean that
> > this will always be the case.
> > 
> > Cheers,
> > Andre
> >   
> 

[PATCH v3 5/6] mmc: actions: add MMC driver for Actions OWL S700/S900

[Jaehoon Chung]

On 3/2/21 8:04 AM, Andre Przywara wrote:
> On Tue, 2 Mar 2021 07:11:54 +0900
> Jaehoon Chung <jh80.chung@samsung.com> wrote:
> 
>> On 3/1/21 11:25 PM, Andre Przywara wrote:
>>> On Mon, 1 Mar 2021 18:47:26 +0530
>>> Amit Tomar <atomar25opensource@gmail.com> wrote:
>>>
>>> Hi,
>>>   
>>>>> So this whole mode handling here looks dodgy. Below you mix "assignments
>>>>> to mode" with "ORing in values", without actually ever initialising mode
>>>>> explicitly. I wonder why the compiler doesn't warn about this, I can see
>>>>> paths were you OR into an uninitialised value.
>>>>>
>>>>> But the compiler already has initialized mode to 0,  
>>>
>>> Why? Where? I just see a local, non-static definition of mode,
>>> meaning it won't be initialised.  
>>
>> It seems that it doesn't initialize in this function.
>> I think that resp_type may be always matched one of condition.
> 
> Possibly, but the problem is that the MMC_RSP_R1 clause also uses
> "|=", so it's the same issue here.
> One a first glance into the assembly it looks like the compiler
> optimises this whole clause away? Undefined behaviour?
> 
> Anyway, I am not really sure we need to argue here, when the fix is
> dead easy ...

Agreed, It doesn't need to discuss about this.
If there is a potential problem, it needs to fix it.

Best Regards,
Jaehoon Chung

> 
> Cheers,
> Andre
> 
>> I think that's why the compiler doesn't warn. 
>>
>> Best Regards,
>> Jaehoon Chung
>>
>>>   
>>>> that is why there is no warning.
>>>> In order to test , printed out mode value which suggests this variable is
>>>> initialized.  
>>>
>>> To what? Just because you printed 0(?) in your test doesn't mean that
>>> this will always be the case.
>>>
>>> Cheers,
>>> Andre
>>>   
>>
> 
>