[PATCH v2 0/2] Add support for Nuvoton MA35D1 SDHCI

[PATCH v2 0/2] Add support for Nuvoton MA35D1 SDHCI

[Shan-Chun Hung]

This patch adds the SDHCI driver and DT binding documentation
for the Nuvoton MA35D1 platform.

This MA35D1 SDHCI driver has been tested on the MA35D1 SOM board with
Linux 6.10

v2:
  - Update to nuvoton,ma35d1-sdhci.yaml
    - Remove some redundant descriptions.
    - Replace 'minitem' with 'maxitem' in the clock settings.
    - Make corrections to nuvoton,sys description.
    - Add sdhci-common.yaml.
    - Remove '|' except where neccessary to be preserved.
    - Keeping one example is sufficient.
    - Add regulators in the example.
  - Update ma35d1 sdhci driver
    - Refer to 'include what you use' to modify included header files.
    - Replace the number 8 with sizeof(u8), and similarly for others.
    - Use "dev" instead of "&pdev->dev".
    - Use the min() macro to improve the code.
    - Use dev_err_probe() instead of dev_err().
    - Implement an error reset check mechanism.
    - Add devm_add_action_or_reset() to help with sdhci_pltfm_free().
    - Use devm_reset_control_get_exclusive() instead of devm_reset_control_get().

Shan-Chun Hung (2):
  dt-bindings: mmc: nuvoton,ma35d1-sdhci: Document MA35D1 SDHCI
    controller
  mmc: sdhci-of-ma35d1: Add Nuvoton MA35D1 SDHCI driver

 .../bindings/mmc/nuvoton,ma35d1-sdhci.yaml    |  88 ++++++
 drivers/mmc/host/Kconfig                      |  14 +
 drivers/mmc/host/Makefile                     |   1 +
 drivers/mmc/host/sdhci-of-ma35d1.c            | 291 ++++++++++++++++++
 4 files changed, 394 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mmc/nuvoton,ma35d1-sdhci.yaml
 create mode 100644 drivers/mmc/host/sdhci-of-ma35d1.c

--
2.25.1

[PATCH v2 1/2] dt-bindings: mmc: nuvoton,ma35d1-sdhci: Document MA35D1 SDHCI controller

[Shan-Chun Hung]

Add binding for Nuvoton MA35D1 SDHCI controller.

Signed-off-by: Shan-Chun Hung <shanchun1218@gmail.com>
---
 .../bindings/mmc/nuvoton,ma35d1-sdhci.yaml    | 88 +++++++++++++++++++
 1 file changed, 88 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mmc/nuvoton,ma35d1-sdhci.yaml

diff --git a/Documentation/devicetree/bindings/mmc/nuvoton,ma35d1-sdhci.yaml b/Documentation/devicetree/bindings/mmc/nuvoton,ma35d1-sdhci.yaml
new file mode 100644
index 000000000000..cf08cdcb58ed
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/nuvoton,ma35d1-sdhci.yaml
@@ -0,0 +1,88 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mmc/nuvoton,ma35d1-sdhci.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Nuvoton MA35D1 SD/SDIO/MMC Controller
+
+maintainers:
+  - Shan-Chun Hung <shanchun1218@gmail.com>
+
+allOf:
+  - $ref: sdhci-common.yaml#
+
+properties:
+  compatible:
+    enum:
+      - nuvoton,ma35d1-sdhci
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  pinctrl-names:
+    minItems: 1
+    items:
+      - const: default
+      - const: state_uhs
+
+  pinctrl-0:
+    description:
+      Should contain default/high speed pin ctrl.
+    maxItems: 1
+
+  pinctrl-1:
+    description:
+      Should contain uhs mode pin ctrl.
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  nuvoton,sys:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: phandle to access GCR (Global Control Register) registers.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - pinctrl-names
+  - pinctrl-0
+  - resets
+  - nuvoton,sys
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/nuvoton,ma35d1-clk.h>
+    #include <dt-bindings/reset/nuvoton,ma35d1-reset.h>
+
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+        mmc@40190000 {
+            compatible = "nuvoton,ma35d1-sdhci";
+            reg = <0x0 0x40190000 0x0 0x2000>;
+            interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
+            clocks = <&clk SDH1_GATE>;
+            pinctrl-names = "default", "state_uhs";
+            pinctrl-0 = <&pinctrl_sdhci1>;
+            pinctrl-1 = <&pinctrl_sdhci1_uhs>;
+            resets = <&sys MA35D1_RESET_SDH1>;
+            nuvoton,sys = <&sys>;
+            vqmmc-supply = <&sdhci1_vqmmc_regulator>;
+            bus-width = <8>;
+            max-frequency = <200000000>;
+            status = "disabled";
+        };
+    };
--
2.25.1

Re: [PATCH v2 1/2] dt-bindings: mmc: nuvoton,ma35d1-sdhci: Document MA35D1 SDHCI controller

[Krzysztof Kozlowski]

On 26/06/2024 11:48, Shan-Chun Hung wrote:

> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +        mmc@40190000 {
> +            compatible = "nuvoton,ma35d1-sdhci";
> +            reg = <0x0 0x40190000 0x0 0x2000>;
> +            interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
> +            clocks = <&clk SDH1_GATE>;
> +            pinctrl-names = "default", "state_uhs";
> +            pinctrl-0 = <&pinctrl_sdhci1>;
> +            pinctrl-1 = <&pinctrl_sdhci1_uhs>;
> +            resets = <&sys MA35D1_RESET_SDH1>;
> +            nuvoton,sys = <&sys>;
> +            vqmmc-supply = <&sdhci1_vqmmc_regulator>;
> +            bus-width = <8>;
> +            max-frequency = <200000000>;
> +            status = "disabled";

Again: Drop

<form letter>
This is a friendly reminder during the review process.

It seems my or other reviewer's previous comments were not fully
addressed. Maybe the feedback got lost between the quotes, maybe you
just forgot to apply it. Please go back to the previous discussion and
either implement all requested changes or keep discussing them.

Thank you.
</form letter>


Best regards,
Krzysztof

[PATCH v2 2/2] mmc: sdhci-of-ma35d1: Add Nuvoton MA35D1 SDHCI driver

[Shan-Chun Hung]

Add the SDHCI driver for the MA35D1 platform. It is based upon the
SDHCI interface, but requires some extra initialization.

Signed-off-by: Shan-Chun Hung <shanchun1218@gmail.com>
---
 drivers/mmc/host/Kconfig           |  14 ++
 drivers/mmc/host/Makefile          |   1 +
 drivers/mmc/host/sdhci-of-ma35d1.c | 291 +++++++++++++++++++++++++++++
 3 files changed, 306 insertions(+)
 create mode 100644 drivers/mmc/host/sdhci-of-ma35d1.c

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index bb0d4fb0892a..31cd076e1c53 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -252,6 +252,20 @@ config MMC_SDHCI_OF_SPARX5

	  If unsure, say N.

+config MMC_SDHCI_OF_MA35D1
+	tristate "SDHCI OF support for the MA35D1 SDHCI controller"
+	depends on ARCH_MA35
+	depends on MMC_SDHCI_PLTFM
+	depends on COMMON_CLK
+	depends on OF || COMPILE_TEST
+	help
+	  This selects the MA35D1 Secure Digital Host Controller Interface.
+
+	  If you have a controller with this interface, say Y or M here. You
+	  also need to enable an appropriate bus interface.
+
+	  If unsure, say N.
+
 config MMC_SDHCI_CADENCE
	tristate "SDHCI support for the Cadence SD/SDIO/eMMC controller"
	depends on MMC_SDHCI_PLTFM
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index f53f86d200ac..3ccffebbe59b 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -88,6 +88,7 @@ obj-$(CONFIG_MMC_SDHCI_OF_ESDHC)	+= sdhci-of-esdhc.o
 obj-$(CONFIG_MMC_SDHCI_OF_HLWD)		+= sdhci-of-hlwd.o
 obj-$(CONFIG_MMC_SDHCI_OF_DWCMSHC)	+= sdhci-of-dwcmshc.o
 obj-$(CONFIG_MMC_SDHCI_OF_SPARX5)	+= sdhci-of-sparx5.o
+obj-$(CONFIG_MMC_SDHCI_OF_MA35D1)	+= sdhci-of-ma35d1.o
 obj-$(CONFIG_MMC_SDHCI_BCM_KONA)	+= sdhci-bcm-kona.o
 obj-$(CONFIG_MMC_SDHCI_IPROC)		+= sdhci-iproc.o
 obj-$(CONFIG_MMC_SDHCI_NPCM)		+= sdhci-npcm.o
diff --git a/drivers/mmc/host/sdhci-of-ma35d1.c b/drivers/mmc/host/sdhci-of-ma35d1.c
new file mode 100644
index 000000000000..e260aeb12d7f
--- /dev/null
+++ b/drivers/mmc/host/sdhci-of-ma35d1.c
@@ -0,0 +1,291 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2024 Nuvoton Technology Corp.
+ *
+ * Author: Shan-Chun Hung <shanchun1218@gmail.com>
+ */
+
+#include <linux/align.h>
+#include <linux/array_size.h>
+#include <linux/bits.h>
+#include <linux/build_bug.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/dev_printk.h>
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+#include <linux/err.h>
+#include <linux/math.h>
+#include <linux/mfd/syscon.h>
+#include <linux/minmax.h>
+#include <linux/mmc/card.h>
+#include <linux/mmc/host.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+#include <linux/sizes.h>
+#include <linux/types.h>
+
+#include "sdhci-pltfm.h"
+#include "sdhci.h"
+
+#define MA35_SYS_MISCFCR0	0x070
+#define MA35_SDHCI_MSHCCTL	0x508
+#define MA35_SDHCI_MBIUCTL	0x510
+
+#define MA35_SDHCI_CMD_CONFLICT_CHK	BIT(0)
+#define MA35_SDHCI_INCR_MSK		GENMASK(3, 0)
+#define MA35_SDHCI_INCR16		BIT(3)
+#define MA35_SDHCI_INCR8		BIT(2)
+
+struct ma35_priv {
+	struct regmap		*regmap;
+	struct reset_control	*rst;
+	struct pinctrl		*pinctrl;
+	struct pinctrl_state	*pins_uhs;
+	struct pinctrl_state	*pins_default;
+};
+
+struct ma35_restore_data {
+	u32	reg;
+	u32	width;
+};
+
+static const struct ma35_restore_data restore_data[] = {
+	{ SDHCI_CLOCK_CONTROL,		sizeof(u32)},
+	{ SDHCI_BLOCK_SIZE,		sizeof(u32)},
+	{ SDHCI_INT_ENABLE,		sizeof(u32)},
+	{ SDHCI_SIGNAL_ENABLE,		sizeof(u32)},
+	{ SDHCI_AUTO_CMD_STATUS,	sizeof(u32)},
+	{ SDHCI_HOST_CONTROL,		sizeof(u32)},
+	{ SDHCI_TIMEOUT_CONTROL,	sizeof(u8) },
+	{ MA35_SDHCI_MSHCCTL,		sizeof(u16)},
+	{ MA35_SDHCI_MBIUCTL,		sizeof(u16)},
+};
+
+/*
+ * If DMA addr spans 128MB boundary, we split the DMA transfer into two
+ * so that each DMA transfer doesn't exceed the boundary.
+ */
+static void ma35_adma_write_desc(struct sdhci_host *host, void **desc,
+				  dma_addr_t addr, int len, unsigned int cmd)
+{
+	int tmplen, offset;
+
+	if (likely(!len || (ALIGN(addr, SZ_128M) == ALIGN(addr+len-1, SZ_128M)))) {
+		sdhci_adma_write_desc(host, desc, addr, len, cmd);
+		return;
+	}
+
+	offset = addr & (SZ_128M - 1);
+	tmplen = SZ_128M - offset;
+	sdhci_adma_write_desc(host, desc, addr, tmplen, cmd);
+
+	addr += tmplen;
+	len -= tmplen;
+	sdhci_adma_write_desc(host, desc, addr, len, cmd);
+}
+
+static void ma35_set_clock(struct sdhci_host *host, unsigned int clock)
+{
+	u32 ctl;
+
+	/* If the clock frequency exceeds MMC_HIGH_52_MAX_DTR,
+	 * disable command conflict check.
+	 */
+	ctl = sdhci_readw(host, MA35_SDHCI_MSHCCTL);
+	if (clock > MMC_HIGH_52_MAX_DTR)
+		ctl &= ~MA35_SDHCI_CMD_CONFLICT_CHK;
+	else
+		ctl |= MA35_SDHCI_CMD_CONFLICT_CHK;
+	sdhci_writew(host, ctl, MA35_SDHCI_MSHCCTL);
+
+	sdhci_set_clock(host, clock);
+}
+
+static int ma35_start_signal_voltage_switch(struct mmc_host *mmc,
+					      struct mmc_ios *ios)
+{
+	struct sdhci_host *host = mmc_priv(mmc);
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct ma35_priv *priv = sdhci_pltfm_priv(pltfm_host);
+
+	switch (ios->signal_voltage) {
+	case MMC_SIGNAL_VOLTAGE_180:
+		if (!IS_ERR(priv->pinctrl) && !IS_ERR(priv->pins_uhs))
+			pinctrl_select_state(priv->pinctrl, priv->pins_uhs);
+		break;
+	case MMC_SIGNAL_VOLTAGE_330:
+		if (!IS_ERR(priv->pinctrl) && !IS_ERR(priv->pins_default))
+			pinctrl_select_state(priv->pinctrl, priv->pins_default);
+		break;
+	default:
+		dev_err(mmc_dev(host->mmc), "Unsupported signal voltage!\n");
+		return -EINVAL;
+	}
+
+	return sdhci_start_signal_voltage_switch(mmc, ios);
+}
+
+static void ma35_voltage_switch(struct sdhci_host *host)
+{
+	/* Wait for 5ms after set 1.8V signal enable bit */
+	fsleep(5000);
+}
+
+static int ma35_execute_tuning(struct mmc_host *mmc, u32 opcode)
+{
+	struct sdhci_host *host = mmc_priv(mmc);
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct ma35_priv *priv = sdhci_pltfm_priv(pltfm_host);
+	int idx;
+	u32 regs[ARRAY_SIZE(restore_data)] = { };
+
+	/* Limitations require a reset of SD/eMMC before tuning and
+	 * saving the registers before resetting, then restoring
+	 * after the reset.
+	 */
+	for (idx = 0; idx < ARRAY_SIZE(restore_data); idx++) {
+		if (restore_data[idx].width == sizeof(u32))
+			regs[idx] = sdhci_readl(host, restore_data[idx].reg);
+		else if (restore_data[idx].width == sizeof(u16))
+			regs[idx] = sdhci_readw(host, restore_data[idx].reg);
+		else if (restore_data[idx].width == sizeof(u8))
+			regs[idx] = sdhci_readb(host, restore_data[idx].reg);
+	}
+
+	reset_control_assert(priv->rst);
+	reset_control_deassert(priv->rst);
+
+	for (idx = 0; idx < ARRAY_SIZE(restore_data); idx++) {
+		if (restore_data[idx].width == sizeof(u32))
+			sdhci_writel(host, regs[idx], restore_data[idx].reg);
+		else if (restore_data[idx].width == sizeof(u16))
+			sdhci_writew(host, regs[idx], restore_data[idx].reg);
+		else if (restore_data[idx].width == sizeof(u8))
+			sdhci_writeb(host, regs[idx], restore_data[idx].reg);
+	}
+
+	return sdhci_execute_tuning(mmc, opcode);
+}
+
+static const struct sdhci_ops sdhci_ma35_ops = {
+	.set_clock		= ma35_set_clock,
+	.set_bus_width		= sdhci_set_bus_width,
+	.set_uhs_signaling	= sdhci_set_uhs_signaling,
+	.get_max_clock		= sdhci_pltfm_clk_get_max_clock,
+	.reset			= sdhci_reset,
+	.adma_write_desc	= ma35_adma_write_desc,
+	.voltage_switch	= ma35_voltage_switch,
+};
+
+static const struct sdhci_pltfm_data sdhci_ma35_pdata = {
+	.ops = &sdhci_ma35_ops,
+	.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
+	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | SDHCI_QUIRK2_BROKEN_DDR50 |
+		   SDHCI_QUIRK2_ACMD23_BROKEN,
+};
+
+static void ma35_sdhci_pltfm_free(void *data)
+{
+	sdhci_pltfm_free(data);
+}
+
+static int ma35_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct sdhci_pltfm_host *pltfm_host;
+	struct sdhci_host *host;
+	struct ma35_priv *priv;
+	int err;
+	u32 extra, ctl;
+
+	host = sdhci_pltfm_init(pdev, &sdhci_ma35_pdata, sizeof(struct ma35_priv));
+	if (IS_ERR(host))
+		return PTR_ERR(host);
+
+	err = devm_add_action_or_reset(dev, ma35_sdhci_pltfm_free, pdev);
+	if (err)
+		return dev_err_probe(dev, err, "Failed to register sdhci_pltfm_free action\n");
+
+	/* Extra adma table cnt for cross 128M boundary handling. */
+	extra = DIV_ROUND_UP_ULL(dma_get_required_mask(dev), SZ_128M);
+	extra = min(extra, SDHCI_MAX_SEGS);
+
+	host->adma_table_cnt += extra;
+	pltfm_host = sdhci_priv(host);
+	priv = sdhci_pltfm_priv(pltfm_host);
+
+	pltfm_host->clk = devm_clk_get_optional_enabled(dev, NULL);
+	if (IS_ERR(pltfm_host->clk))
+		return dev_err_probe(dev, IS_ERR(pltfm_host->clk), "failed to get clk\n");
+
+	err = mmc_of_parse(host->mmc);
+	if (err)
+		return err;
+
+	priv->rst = devm_reset_control_get_exclusive(dev, NULL);
+	if (IS_ERR(priv->rst))
+		return dev_err_probe(dev, PTR_ERR(priv->rst), "failed to get reset control\n");
+
+	sdhci_get_of_property(pdev);
+
+	priv->pinctrl = devm_pinctrl_get(dev);
+	if (!IS_ERR(priv->pinctrl)) {
+		priv->pins_default = pinctrl_lookup_state(priv->pinctrl, "default");
+		priv->pins_uhs = pinctrl_lookup_state(priv->pinctrl, "state_uhs");
+		pinctrl_select_state(priv->pinctrl, priv->pins_default);
+	}
+
+	if (!(host->quirks2 & SDHCI_QUIRK2_NO_1_8_V)) {
+		u32 reg;
+
+		priv->regmap = syscon_regmap_lookup_by_phandle(dev_of_node(dev), "nuvoton,sys");
+
+		if (!IS_ERR(priv->regmap)) {
+			/* Enable SDHCI voltage stable for 1.8V */
+			regmap_read(priv->regmap, MA35_SYS_MISCFCR0, &reg);
+			reg |= BIT(17);
+			regmap_write(priv->regmap, MA35_SYS_MISCFCR0, reg);
+		}
+
+		host->mmc_host_ops.start_signal_voltage_switch =
+					ma35_start_signal_voltage_switch;
+	}
+
+	host->mmc_host_ops.execute_tuning = ma35_execute_tuning;
+
+	err = sdhci_add_host(host);
+	if (err)
+		return err;
+
+	/* Enable INCR16 and INCR8 */
+	ctl = sdhci_readw(host, MA35_SDHCI_MBIUCTL);
+	ctl &= ~MA35_SDHCI_INCR_MSK;
+	ctl |= MA35_SDHCI_INCR16|MA35_SDHCI_INCR8;
+	sdhci_writew(host, ctl, MA35_SDHCI_MBIUCTL);
+
+	return 0;
+}
+
+static const struct of_device_id sdhci_ma35_dt_ids[] = {
+	{ .compatible = "nuvoton,ma35d1-sdhci" },
+	{}
+};
+
+static struct platform_driver sdhci_ma35_driver = {
+	.driver	= {
+		.name	= "sdhci-ma35",
+		.of_match_table = sdhci_ma35_dt_ids,
+	},
+	.probe	= ma35_probe,
+	.remove_new = sdhci_pltfm_remove,
+};
+module_platform_driver(sdhci_ma35_driver);
+
+MODULE_DESCRIPTION("SDHCI platform driver for Nuvoton MA35");
+MODULE_AUTHOR("Shan-Chun Hung <shanchun1218@google.com>");
+MODULE_LICENSE("GPL v2");
--
2.25.1

Re: [PATCH v2 2/2] mmc: sdhci-of-ma35d1: Add Nuvoton MA35D1 SDHCI driver

[Adrian Hunter]

On 26/06/24 12:49, Shan-Chun Hung wrote:
> Add the SDHCI driver for the MA35D1 platform. It is based upon the
> SDHCI interface, but requires some extra initialization.
> 
> Signed-off-by: Shan-Chun Hung <shanchun1218@gmail.com>
> ---
>  drivers/mmc/host/Kconfig           |  14 ++
>  drivers/mmc/host/Makefile          |   1 +
>  drivers/mmc/host/sdhci-of-ma35d1.c | 291 +++++++++++++++++++++++++++++
>  3 files changed, 306 insertions(+)
>  create mode 100644 drivers/mmc/host/sdhci-of-ma35d1.c
> 
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index bb0d4fb0892a..31cd076e1c53 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -252,6 +252,20 @@ config MMC_SDHCI_OF_SPARX5
> 

Patch did not apply cleanly:

Applying: mmc: sdhci-of-ma35d1: Add Nuvoton MA35D1 SDHCI driver
error: corrupt patch at line 14
Patch failed at 0001 mmc: sdhci-of-ma35d1: Add Nuvoton MA35D1 SDHCI driver

> 	  If unsure, say N.
> 
> +config MMC_SDHCI_OF_MA35D1
> +	tristate "SDHCI OF support for the MA35D1 SDHCI controller"
> +	depends on ARCH_MA35

Does this dependency serve a purpose?  At least add COMPILE_TEST

	depends on ARCH_MA35 || COMPILE_TEST

> +	depends on MMC_SDHCI_PLTFM
> +	depends on COMMON_CLK
> +	depends on OF || COMPILE_TEST
> +	help
> +	  This selects the MA35D1 Secure Digital Host Controller Interface.
> +
> +	  If you have a controller with this interface, say Y or M here. You
> +	  also need to enable an appropriate bus interface.
> +
> +	  If unsure, say N.
> +
>  config MMC_SDHCI_CADENCE
> 	tristate "SDHCI support for the Cadence SD/SDIO/eMMC controller"
> 	depends on MMC_SDHCI_PLTFM
> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> index f53f86d200ac..3ccffebbe59b 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -88,6 +88,7 @@ obj-$(CONFIG_MMC_SDHCI_OF_ESDHC)	+= sdhci-of-esdhc.o
>  obj-$(CONFIG_MMC_SDHCI_OF_HLWD)		+= sdhci-of-hlwd.o
>  obj-$(CONFIG_MMC_SDHCI_OF_DWCMSHC)	+= sdhci-of-dwcmshc.o
>  obj-$(CONFIG_MMC_SDHCI_OF_SPARX5)	+= sdhci-of-sparx5.o
> +obj-$(CONFIG_MMC_SDHCI_OF_MA35D1)	+= sdhci-of-ma35d1.o
>  obj-$(CONFIG_MMC_SDHCI_BCM_KONA)	+= sdhci-bcm-kona.o
>  obj-$(CONFIG_MMC_SDHCI_IPROC)		+= sdhci-iproc.o
>  obj-$(CONFIG_MMC_SDHCI_NPCM)		+= sdhci-npcm.o
> diff --git a/drivers/mmc/host/sdhci-of-ma35d1.c b/drivers/mmc/host/sdhci-of-ma35d1.c
> new file mode 100644
> index 000000000000..e260aeb12d7f
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-of-ma35d1.c
> @@ -0,0 +1,291 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2024 Nuvoton Technology Corp.
> + *
> + * Author: Shan-Chun Hung <shanchun1218@gmail.com>
> + */
> +
> +#include <linux/align.h>
> +#include <linux/array_size.h>
> +#include <linux/bits.h>
> +#include <linux/build_bug.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/dev_printk.h>
> +#include <linux/device.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/err.h>
> +#include <linux/math.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/minmax.h>
> +#include <linux/mmc/card.h>
> +#include <linux/mmc/host.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include <linux/sizes.h>
> +#include <linux/types.h>
> +
> +#include "sdhci-pltfm.h"
> +#include "sdhci.h"
> +
> +#define MA35_SYS_MISCFCR0	0x070
> +#define MA35_SDHCI_MSHCCTL	0x508
> +#define MA35_SDHCI_MBIUCTL	0x510
> +
> +#define MA35_SDHCI_CMD_CONFLICT_CHK	BIT(0)
> +#define MA35_SDHCI_INCR_MSK		GENMASK(3, 0)
> +#define MA35_SDHCI_INCR16		BIT(3)
> +#define MA35_SDHCI_INCR8		BIT(2)
> +
> +struct ma35_priv {
> +	struct regmap		*regmap;
> +	struct reset_control	*rst;
> +	struct pinctrl		*pinctrl;
> +	struct pinctrl_state	*pins_uhs;
> +	struct pinctrl_state	*pins_default;
> +};
> +
> +struct ma35_restore_data {
> +	u32	reg;
> +	u32	width;
> +};
> +
> +static const struct ma35_restore_data restore_data[] = {
> +	{ SDHCI_CLOCK_CONTROL,		sizeof(u32)},
> +	{ SDHCI_BLOCK_SIZE,		sizeof(u32)},
> +	{ SDHCI_INT_ENABLE,		sizeof(u32)},
> +	{ SDHCI_SIGNAL_ENABLE,		sizeof(u32)},
> +	{ SDHCI_AUTO_CMD_STATUS,	sizeof(u32)},
> +	{ SDHCI_HOST_CONTROL,		sizeof(u32)},
> +	{ SDHCI_TIMEOUT_CONTROL,	sizeof(u8) },
> +	{ MA35_SDHCI_MSHCCTL,		sizeof(u16)},
> +	{ MA35_SDHCI_MBIUCTL,		sizeof(u16)},
> +};
> +
> +/*
> + * If DMA addr spans 128MB boundary, we split the DMA transfer into two
> + * so that each DMA transfer doesn't exceed the boundary.
> + */
> +static void ma35_adma_write_desc(struct sdhci_host *host, void **desc,
> +				  dma_addr_t addr, int len, unsigned int cmd)

Please take a look at checkpatch --strict output.  Fixing "Alignment" and
"spaces preferred around" CHECKs tends to make the code look better.

> +{
> +	int tmplen, offset;
> +
> +	if (likely(!len || (ALIGN(addr, SZ_128M) == ALIGN(addr+len-1, SZ_128M)))) {
> +		sdhci_adma_write_desc(host, desc, addr, len, cmd);
> +		return;
> +	}
> +
> +	offset = addr & (SZ_128M - 1);
> +	tmplen = SZ_128M - offset;
> +	sdhci_adma_write_desc(host, desc, addr, tmplen, cmd);
> +
> +	addr += tmplen;
> +	len -= tmplen;
> +	sdhci_adma_write_desc(host, desc, addr, len, cmd);
> +}
> +
> +static void ma35_set_clock(struct sdhci_host *host, unsigned int clock)
> +{
> +	u32 ctl;
> +
> +	/* If the clock frequency exceeds MMC_HIGH_52_MAX_DTR,
> +	 * disable command conflict check.
> +	 */

Please use the following style for multi-line comments:

	/*
	 * If the clock frequency exceeds MMC_HIGH_52_MAX_DTR,
	 * disable command conflict check.
	 */

> +	ctl = sdhci_readw(host, MA35_SDHCI_MSHCCTL);
> +	if (clock > MMC_HIGH_52_MAX_DTR)
> +		ctl &= ~MA35_SDHCI_CMD_CONFLICT_CHK;
> +	else
> +		ctl |= MA35_SDHCI_CMD_CONFLICT_CHK;
> +	sdhci_writew(host, ctl, MA35_SDHCI_MSHCCTL);
> +
> +	sdhci_set_clock(host, clock);
> +}
> +
> +static int ma35_start_signal_voltage_switch(struct mmc_host *mmc,
> +					      struct mmc_ios *ios)
> +{
> +	struct sdhci_host *host = mmc_priv(mmc);
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct ma35_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +
> +	switch (ios->signal_voltage) {
> +	case MMC_SIGNAL_VOLTAGE_180:
> +		if (!IS_ERR(priv->pinctrl) && !IS_ERR(priv->pins_uhs))
> +			pinctrl_select_state(priv->pinctrl, priv->pins_uhs);
> +		break;
> +	case MMC_SIGNAL_VOLTAGE_330:
> +		if (!IS_ERR(priv->pinctrl) && !IS_ERR(priv->pins_default))
> +			pinctrl_select_state(priv->pinctrl, priv->pins_default);
> +		break;
> +	default:
> +		dev_err(mmc_dev(host->mmc), "Unsupported signal voltage!\n");
> +		return -EINVAL;
> +	}
> +
> +	return sdhci_start_signal_voltage_switch(mmc, ios);
> +}
> +
> +static void ma35_voltage_switch(struct sdhci_host *host)
> +{
> +	/* Wait for 5ms after set 1.8V signal enable bit */
> +	fsleep(5000);
> +}
> +
> +static int ma35_execute_tuning(struct mmc_host *mmc, u32 opcode)
> +{
> +	struct sdhci_host *host = mmc_priv(mmc);
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct ma35_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +	int idx;
> +	u32 regs[ARRAY_SIZE(restore_data)] = { };
> +
> +	/* Limitations require a reset of SD/eMMC before tuning and
> +	 * saving the registers before resetting, then restoring
> +	 * after the reset.
> +	 */

Please use the following style for multi-line comments:

	/*
	 * Limitations require a reset of SD/eMMC before tuning and
	 * saving the registers before resetting, then restoring
	 * after the reset.
	 */

> +	for (idx = 0; idx < ARRAY_SIZE(restore_data); idx++) {
> +		if (restore_data[idx].width == sizeof(u32))
> +			regs[idx] = sdhci_readl(host, restore_data[idx].reg);
> +		else if (restore_data[idx].width == sizeof(u16))
> +			regs[idx] = sdhci_readw(host, restore_data[idx].reg);
> +		else if (restore_data[idx].width == sizeof(u8))
> +			regs[idx] = sdhci_readb(host, restore_data[idx].reg);
> +	}
> +
> +	reset_control_assert(priv->rst);
> +	reset_control_deassert(priv->rst);
> +
> +	for (idx = 0; idx < ARRAY_SIZE(restore_data); idx++) {
> +		if (restore_data[idx].width == sizeof(u32))
> +			sdhci_writel(host, regs[idx], restore_data[idx].reg);
> +		else if (restore_data[idx].width == sizeof(u16))
> +			sdhci_writew(host, regs[idx], restore_data[idx].reg);
> +		else if (restore_data[idx].width == sizeof(u8))
> +			sdhci_writeb(host, regs[idx], restore_data[idx].reg);
> +	}
> +
> +	return sdhci_execute_tuning(mmc, opcode);
> +}
> +
> +static const struct sdhci_ops sdhci_ma35_ops = {
> +	.set_clock		= ma35_set_clock,
> +	.set_bus_width		= sdhci_set_bus_width,
> +	.set_uhs_signaling	= sdhci_set_uhs_signaling,
> +	.get_max_clock		= sdhci_pltfm_clk_get_max_clock,
> +	.reset			= sdhci_reset,
> +	.adma_write_desc	= ma35_adma_write_desc,
> +	.voltage_switch	= ma35_voltage_switch,

Last one does not line up

> +};
> +
> +static const struct sdhci_pltfm_data sdhci_ma35_pdata = {
> +	.ops = &sdhci_ma35_ops,
> +	.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
> +	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | SDHCI_QUIRK2_BROKEN_DDR50 |
> +		   SDHCI_QUIRK2_ACMD23_BROKEN,
> +};
> +
> +static void ma35_sdhci_pltfm_free(void *data)
> +{
> +	sdhci_pltfm_free(data);
> +}
> +
> +static int ma35_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct sdhci_pltfm_host *pltfm_host;
> +	struct sdhci_host *host;
> +	struct ma35_priv *priv;
> +	int err;
> +	u32 extra, ctl;
> +
> +	host = sdhci_pltfm_init(pdev, &sdhci_ma35_pdata, sizeof(struct ma35_priv));
> +	if (IS_ERR(host))
> +		return PTR_ERR(host);
> +
> +	err = devm_add_action_or_reset(dev, ma35_sdhci_pltfm_free, pdev);

This just looks like it is going to call sdhci_pltfm_free()
for a second time after it is called by sdhci_pltfm_remove()

> +	if (err)
> +		return dev_err_probe(dev, err, "Failed to register sdhci_pltfm_free action\n");
> +
> +	/* Extra adma table cnt for cross 128M boundary handling. */
> +	extra = DIV_ROUND_UP_ULL(dma_get_required_mask(dev), SZ_128M);
> +	extra = min(extra, SDHCI_MAX_SEGS);
> +
> +	host->adma_table_cnt += extra;
> +	pltfm_host = sdhci_priv(host);
> +	priv = sdhci_pltfm_priv(pltfm_host);
> +
> +	pltfm_host->clk = devm_clk_get_optional_enabled(dev, NULL);
> +	if (IS_ERR(pltfm_host->clk))
> +		return dev_err_probe(dev, IS_ERR(pltfm_host->clk), "failed to get clk\n");
> +
> +	err = mmc_of_parse(host->mmc);
> +	if (err)
> +		return err;
> +
> +	priv->rst = devm_reset_control_get_exclusive(dev, NULL);
> +	if (IS_ERR(priv->rst))
> +		return dev_err_probe(dev, PTR_ERR(priv->rst), "failed to get reset control\n");
> +
> +	sdhci_get_of_property(pdev);
> +
> +	priv->pinctrl = devm_pinctrl_get(dev);
> +	if (!IS_ERR(priv->pinctrl)) {
> +		priv->pins_default = pinctrl_lookup_state(priv->pinctrl, "default");
> +		priv->pins_uhs = pinctrl_lookup_state(priv->pinctrl, "state_uhs");
> +		pinctrl_select_state(priv->pinctrl, priv->pins_default);
> +	}
> +
> +	if (!(host->quirks2 & SDHCI_QUIRK2_NO_1_8_V)) {
> +		u32 reg;
> +
> +		priv->regmap = syscon_regmap_lookup_by_phandle(dev_of_node(dev), "nuvoton,sys");
> +
> +		if (!IS_ERR(priv->regmap)) {
> +			/* Enable SDHCI voltage stable for 1.8V */
> +			regmap_read(priv->regmap, MA35_SYS_MISCFCR0, &reg);
> +			reg |= BIT(17);
> +			regmap_write(priv->regmap, MA35_SYS_MISCFCR0, reg);
> +		}
> +
> +		host->mmc_host_ops.start_signal_voltage_switch =
> +					ma35_start_signal_voltage_switch;
> +	}
> +
> +	host->mmc_host_ops.execute_tuning = ma35_execute_tuning;
> +
> +	err = sdhci_add_host(host);
> +	if (err)
> +		return err;
> +
> +	/* Enable INCR16 and INCR8 */

Comment would be more useful if it said what INCR16 and
INCR8 are

> +	ctl = sdhci_readw(host, MA35_SDHCI_MBIUCTL);
> +	ctl &= ~MA35_SDHCI_INCR_MSK;
> +	ctl |= MA35_SDHCI_INCR16|MA35_SDHCI_INCR8;
> +	sdhci_writew(host, ctl, MA35_SDHCI_MBIUCTL);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id sdhci_ma35_dt_ids[] = {
> +	{ .compatible = "nuvoton,ma35d1-sdhci" },
> +	{}
> +};
> +
> +static struct platform_driver sdhci_ma35_driver = {
> +	.driver	= {
> +		.name	= "sdhci-ma35",
> +		.of_match_table = sdhci_ma35_dt_ids,
> +	},
> +	.probe	= ma35_probe,
> +	.remove_new = sdhci_pltfm_remove,
> +};
> +module_platform_driver(sdhci_ma35_driver);
> +
> +MODULE_DESCRIPTION("SDHCI platform driver for Nuvoton MA35");
> +MODULE_AUTHOR("Shan-Chun Hung <shanchun1218@google.com>");

"From:" is shanchun1218@gmail.com
"Signed-off-by" is shanchun1218@gmail.com

but here it is shanchun1218@google.com

Please be consistent

> +MODULE_LICENSE("GPL v2");

checkpatch says:

WARNING: Prefer "GPL" over "GPL v2" - see commit bf7fbeeae6db ("module: Cure the MODULE_LICENSE "GPL" vs. "GPL v2" bogosity")
#351: FILE: drivers/mmc/host/sdhci-of-ma35d1.c:291:
+MODULE_LICENSE("GPL v2");

> --
> 2.25.1

Re: [PATCH v2 2/2] mmc: sdhci-of-ma35d1: Add Nuvoton MA35D1 SDHCI driver

[Shan-Chun Hung]

Dear Adrian,

Thanks for your review.

On 2024/6/27 下午 05:40, Adrian Hunter wrote:
> On 26/06/24 12:49, Shan-Chun Hung wrote:
>> Add the SDHCI driver for the MA35D1 platform. It is based upon the
>> SDHCI interface, but requires some extra initialization.
>>
>> Signed-off-by: Shan-Chun Hung <shanchun1218@gmail.com>
>> ---
>>   drivers/mmc/host/Kconfig           |  14 ++
>>   drivers/mmc/host/Makefile          |   1 +
>>   drivers/mmc/host/sdhci-of-ma35d1.c | 291 +++++++++++++++++++++++++++++
>>   3 files changed, 306 insertions(+)
>>   create mode 100644 drivers/mmc/host/sdhci-of-ma35d1.c
>>
>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>> index bb0d4fb0892a..31cd076e1c53 100644
>> --- a/drivers/mmc/host/Kconfig
>> +++ b/drivers/mmc/host/Kconfig
>> @@ -252,6 +252,20 @@ config MMC_SDHCI_OF_SPARX5
>>
> Patch did not apply cleanly:
>
> Applying: mmc: sdhci-of-ma35d1: Add Nuvoton MA35D1 SDHCI driver
> error: corrupt patch at line 14
> Patch failed at 0001 mmc: sdhci-of-ma35d1: Add Nuvoton MA35D1 SDHCI driver
I will fix it.
>> 	  If unsure, say N.
>>
>> +config MMC_SDHCI_OF_MA35D1
>> +	tristate "SDHCI OF support for the MA35D1 SDHCI controller"
>> +	depends on ARCH_MA35
> Does this dependency serve a purpose?  At least add COMPILE_TEST
>
> 	depends on ARCH_MA35 || COMPILE_TEST
I will modify "depends on ARCH_MA35 || COMPILE_TEST"
>> +	depends on MMC_SDHCI_PLTFM
>> +	depends on COMMON_CLK
>> +	depends on OF || COMPILE_TEST
>> +	help
>> +	  This selects the MA35D1 Secure Digital Host Controller Interface.
>> +
>> +	  If you have a controller with this interface, say Y or M here. You
>> +	  also need to enable an appropriate bus interface.
>> +
>> +	  If unsure, say N.
>> +
>>   config MMC_SDHCI_CADENCE
>> 	tristate "SDHCI support for the Cadence SD/SDIO/eMMC controller"
>> 	depends on MMC_SDHCI_PLTFM
>> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
>> index f53f86d200ac..3ccffebbe59b 100644
>> --- a/drivers/mmc/host/Makefile
>> +++ b/drivers/mmc/host/Makefile
>> @@ -88,6 +88,7 @@ obj-$(CONFIG_MMC_SDHCI_OF_ESDHC)	+= sdhci-of-esdhc.o
>>   obj-$(CONFIG_MMC_SDHCI_OF_HLWD)		+= sdhci-of-hlwd.o
>>   obj-$(CONFIG_MMC_SDHCI_OF_DWCMSHC)	+= sdhci-of-dwcmshc.o
>>   obj-$(CONFIG_MMC_SDHCI_OF_SPARX5)	+= sdhci-of-sparx5.o
>> +obj-$(CONFIG_MMC_SDHCI_OF_MA35D1)	+= sdhci-of-ma35d1.o
>>   obj-$(CONFIG_MMC_SDHCI_BCM_KONA)	+= sdhci-bcm-kona.o
>>   obj-$(CONFIG_MMC_SDHCI_IPROC)		+= sdhci-iproc.o
>>   obj-$(CONFIG_MMC_SDHCI_NPCM)		+= sdhci-npcm.o
>> diff --git a/drivers/mmc/host/sdhci-of-ma35d1.c b/drivers/mmc/host/sdhci-of-ma35d1.c
>> new file mode 100644
>> index 000000000000..e260aeb12d7f
>> --- /dev/null
>> +++ b/drivers/mmc/host/sdhci-of-ma35d1.c
>> @@ -0,0 +1,291 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2024 Nuvoton Technology Corp.
>> + *
>> + * Author: Shan-Chun Hung <shanchun1218@gmail.com>
>> + */
>> +
>> +#include <linux/align.h>
>> +#include <linux/array_size.h>
>> +#include <linux/bits.h>
>> +#include <linux/build_bug.h>
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/dev_printk.h>
>> +#include <linux/device.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/err.h>
>> +#include <linux/math.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/minmax.h>
>> +#include <linux/mmc/card.h>
>> +#include <linux/mmc/host.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/module.h>
>> +#include <linux/pinctrl/consumer.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/reset.h>
>> +#include <linux/sizes.h>
>> +#include <linux/types.h>
>> +
>> +#include "sdhci-pltfm.h"
>> +#include "sdhci.h"
>> +
>> +#define MA35_SYS_MISCFCR0	0x070
>> +#define MA35_SDHCI_MSHCCTL	0x508
>> +#define MA35_SDHCI_MBIUCTL	0x510
>> +
>> +#define MA35_SDHCI_CMD_CONFLICT_CHK	BIT(0)
>> +#define MA35_SDHCI_INCR_MSK		GENMASK(3, 0)
>> +#define MA35_SDHCI_INCR16		BIT(3)
>> +#define MA35_SDHCI_INCR8		BIT(2)
>> +
>> +struct ma35_priv {
>> +	struct regmap		*regmap;
>> +	struct reset_control	*rst;
>> +	struct pinctrl		*pinctrl;
>> +	struct pinctrl_state	*pins_uhs;
>> +	struct pinctrl_state	*pins_default;
>> +};
>> +
>> +struct ma35_restore_data {
>> +	u32	reg;
>> +	u32	width;
>> +};
>> +
>> +static const struct ma35_restore_data restore_data[] = {
>> +	{ SDHCI_CLOCK_CONTROL,		sizeof(u32)},
>> +	{ SDHCI_BLOCK_SIZE,		sizeof(u32)},
>> +	{ SDHCI_INT_ENABLE,		sizeof(u32)},
>> +	{ SDHCI_SIGNAL_ENABLE,		sizeof(u32)},
>> +	{ SDHCI_AUTO_CMD_STATUS,	sizeof(u32)},
>> +	{ SDHCI_HOST_CONTROL,		sizeof(u32)},
>> +	{ SDHCI_TIMEOUT_CONTROL,	sizeof(u8) },
>> +	{ MA35_SDHCI_MSHCCTL,		sizeof(u16)},
>> +	{ MA35_SDHCI_MBIUCTL,		sizeof(u16)},
>> +};
>> +
>> +/*
>> + * If DMA addr spans 128MB boundary, we split the DMA transfer into two
>> + * so that each DMA transfer doesn't exceed the boundary.
>> + */
>> +static void ma35_adma_write_desc(struct sdhci_host *host, void **desc,
>> +				  dma_addr_t addr, int len, unsigned int cmd)
> Please take a look at checkpatch --strict output.  Fixing "Alignment" and
> "spaces preferred around" CHECKs tends to make the code look better.
I will fix it.
>> +{
>> +	int tmplen, offset;
>> +
>> +	if (likely(!len || (ALIGN(addr, SZ_128M) == ALIGN(addr+len-1, SZ_128M)))) {
>> +		sdhci_adma_write_desc(host, desc, addr, len, cmd);
>> +		return;
>> +	}
>> +
>> +	offset = addr & (SZ_128M - 1);
>> +	tmplen = SZ_128M - offset;
>> +	sdhci_adma_write_desc(host, desc, addr, tmplen, cmd);
>> +
>> +	addr += tmplen;
>> +	len -= tmplen;
>> +	sdhci_adma_write_desc(host, desc, addr, len, cmd);
>> +}
>> +
>> +static void ma35_set_clock(struct sdhci_host *host, unsigned int clock)
>> +{
>> +	u32 ctl;
>> +
>> +	/* If the clock frequency exceeds MMC_HIGH_52_MAX_DTR,
>> +	 * disable command conflict check.
>> +	 */
> Please use the following style for multi-line comments:
>
> 	/*
> 	 * If the clock frequency exceeds MMC_HIGH_52_MAX_DTR,
> 	 * disable command conflict check.
> 	 */
I will fix it.
>> +	ctl = sdhci_readw(host, MA35_SDHCI_MSHCCTL);
>> +	if (clock > MMC_HIGH_52_MAX_DTR)
>> +		ctl &= ~MA35_SDHCI_CMD_CONFLICT_CHK;
>> +	else
>> +		ctl |= MA35_SDHCI_CMD_CONFLICT_CHK;
>> +	sdhci_writew(host, ctl, MA35_SDHCI_MSHCCTL);
>> +
>> +	sdhci_set_clock(host, clock);
>> +}
>> +
>> +static int ma35_start_signal_voltage_switch(struct mmc_host *mmc,
>> +					      struct mmc_ios *ios)
>> +{
>> +	struct sdhci_host *host = mmc_priv(mmc);
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct ma35_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> +
>> +	switch (ios->signal_voltage) {
>> +	case MMC_SIGNAL_VOLTAGE_180:
>> +		if (!IS_ERR(priv->pinctrl) && !IS_ERR(priv->pins_uhs))
>> +			pinctrl_select_state(priv->pinctrl, priv->pins_uhs);
>> +		break;
>> +	case MMC_SIGNAL_VOLTAGE_330:
>> +		if (!IS_ERR(priv->pinctrl) && !IS_ERR(priv->pins_default))
>> +			pinctrl_select_state(priv->pinctrl, priv->pins_default);
>> +		break;
>> +	default:
>> +		dev_err(mmc_dev(host->mmc), "Unsupported signal voltage!\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	return sdhci_start_signal_voltage_switch(mmc, ios);
>> +}
>> +
>> +static void ma35_voltage_switch(struct sdhci_host *host)
>> +{
>> +	/* Wait for 5ms after set 1.8V signal enable bit */
>> +	fsleep(5000);
>> +}
>> +
>> +static int ma35_execute_tuning(struct mmc_host *mmc, u32 opcode)
>> +{
>> +	struct sdhci_host *host = mmc_priv(mmc);
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct ma35_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> +	int idx;
>> +	u32 regs[ARRAY_SIZE(restore_data)] = { };
>> +
>> +	/* Limitations require a reset of SD/eMMC before tuning and
>> +	 * saving the registers before resetting, then restoring
>> +	 * after the reset.
>> +	 */
> Please use the following style for multi-line comments:
>
> 	/*
> 	 * Limitations require a reset of SD/eMMC before tuning and
> 	 * saving the registers before resetting, then restoring
> 	 * after the reset.
> 	 */
I will fix it.
>> +	for (idx = 0; idx < ARRAY_SIZE(restore_data); idx++) {
>> +		if (restore_data[idx].width == sizeof(u32))
>> +			regs[idx] = sdhci_readl(host, restore_data[idx].reg);
>> +		else if (restore_data[idx].width == sizeof(u16))
>> +			regs[idx] = sdhci_readw(host, restore_data[idx].reg);
>> +		else if (restore_data[idx].width == sizeof(u8))
>> +			regs[idx] = sdhci_readb(host, restore_data[idx].reg);
>> +	}
>> +
>> +	reset_control_assert(priv->rst);
>> +	reset_control_deassert(priv->rst);
>> +
>> +	for (idx = 0; idx < ARRAY_SIZE(restore_data); idx++) {
>> +		if (restore_data[idx].width == sizeof(u32))
>> +			sdhci_writel(host, regs[idx], restore_data[idx].reg);
>> +		else if (restore_data[idx].width == sizeof(u16))
>> +			sdhci_writew(host, regs[idx], restore_data[idx].reg);
>> +		else if (restore_data[idx].width == sizeof(u8))
>> +			sdhci_writeb(host, regs[idx], restore_data[idx].reg);
>> +	}
>> +
>> +	return sdhci_execute_tuning(mmc, opcode);
>> +}
>> +
>> +static const struct sdhci_ops sdhci_ma35_ops = {
>> +	.set_clock		= ma35_set_clock,
>> +	.set_bus_width		= sdhci_set_bus_width,
>> +	.set_uhs_signaling	= sdhci_set_uhs_signaling,
>> +	.get_max_clock		= sdhci_pltfm_clk_get_max_clock,
>> +	.reset			= sdhci_reset,
>> +	.adma_write_desc	= ma35_adma_write_desc,
>> +	.voltage_switch	= ma35_voltage_switch,
> Last one does not line up
I will fix it
>> +};
>> +
>> +static const struct sdhci_pltfm_data sdhci_ma35_pdata = {
>> +	.ops = &sdhci_ma35_ops,
>> +	.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
>> +	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | SDHCI_QUIRK2_BROKEN_DDR50 |
>> +		   SDHCI_QUIRK2_ACMD23_BROKEN,
>> +};
>> +
>> +static void ma35_sdhci_pltfm_free(void *data)
>> +{
>> +	sdhci_pltfm_free(data);
>> +}
>> +
>> +static int ma35_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct sdhci_pltfm_host *pltfm_host;
>> +	struct sdhci_host *host;
>> +	struct ma35_priv *priv;
>> +	int err;
>> +	u32 extra, ctl;
>> +
>> +	host = sdhci_pltfm_init(pdev, &sdhci_ma35_pdata, sizeof(struct ma35_priv));
>> +	if (IS_ERR(host))
>> +		return PTR_ERR(host);
>> +
>> +	err = devm_add_action_or_reset(dev, ma35_sdhci_pltfm_free, pdev);
> This just looks like it is going to call sdhci_pltfm_free()
> for a second time after it is called by sdhci_pltfm_remove()

During the testing process, we only simulated the scenario where 
ma35_probe() returned an error.

We overlooked the fact that sdhci_pltfm_remove would also be called when 
ma35_probe() succeeds.

This issue was confirmed through testing using the module method. I will 
fix it.

>> +	if (err)
>> +		return dev_err_probe(dev, err, "Failed to register sdhci_pltfm_free action\n");
>> +
>> +	/* Extra adma table cnt for cross 128M boundary handling. */
>> +	extra = DIV_ROUND_UP_ULL(dma_get_required_mask(dev), SZ_128M);
>> +	extra = min(extra, SDHCI_MAX_SEGS);
>> +
>> +	host->adma_table_cnt += extra;
>> +	pltfm_host = sdhci_priv(host);
>> +	priv = sdhci_pltfm_priv(pltfm_host);
>> +
>> +	pltfm_host->clk = devm_clk_get_optional_enabled(dev, NULL);
>> +	if (IS_ERR(pltfm_host->clk))
>> +		return dev_err_probe(dev, IS_ERR(pltfm_host->clk), "failed to get clk\n");
>> +
>> +	err = mmc_of_parse(host->mmc);
>> +	if (err)
>> +		return err;
>> +
>> +	priv->rst = devm_reset_control_get_exclusive(dev, NULL);
>> +	if (IS_ERR(priv->rst))
>> +		return dev_err_probe(dev, PTR_ERR(priv->rst), "failed to get reset control\n");
>> +
>> +	sdhci_get_of_property(pdev);
>> +
>> +	priv->pinctrl = devm_pinctrl_get(dev);
>> +	if (!IS_ERR(priv->pinctrl)) {
>> +		priv->pins_default = pinctrl_lookup_state(priv->pinctrl, "default");
>> +		priv->pins_uhs = pinctrl_lookup_state(priv->pinctrl, "state_uhs");
>> +		pinctrl_select_state(priv->pinctrl, priv->pins_default);
>> +	}
>> +
>> +	if (!(host->quirks2 & SDHCI_QUIRK2_NO_1_8_V)) {
>> +		u32 reg;
>> +
>> +		priv->regmap = syscon_regmap_lookup_by_phandle(dev_of_node(dev), "nuvoton,sys");
>> +
>> +		if (!IS_ERR(priv->regmap)) {
>> +			/* Enable SDHCI voltage stable for 1.8V */
>> +			regmap_read(priv->regmap, MA35_SYS_MISCFCR0, &reg);
>> +			reg |= BIT(17);
>> +			regmap_write(priv->regmap, MA35_SYS_MISCFCR0, reg);
>> +		}
>> +
>> +		host->mmc_host_ops.start_signal_voltage_switch =
>> +					ma35_start_signal_voltage_switch;
>> +	}
>> +
>> +	host->mmc_host_ops.execute_tuning = ma35_execute_tuning;
>> +
>> +	err = sdhci_add_host(host);
>> +	if (err)
>> +		return err;
>> +
>> +	/* Enable INCR16 and INCR8 */
> Comment would be more useful if it said what INCR16 and
> INCR8 are

I will modify as follows:

/*
  * Split data into chunks of 16 or 8 bytes for sending.
  * Each chunk transfer is guaranteed to be uninterrupted on the bus.
  */

>> +	ctl = sdhci_readw(host, MA35_SDHCI_MBIUCTL);
>> +	ctl &= ~MA35_SDHCI_INCR_MSK;
>> +	ctl |= MA35_SDHCI_INCR16|MA35_SDHCI_INCR8;
>> +	sdhci_writew(host, ctl, MA35_SDHCI_MBIUCTL);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id sdhci_ma35_dt_ids[] = {
>> +	{ .compatible = "nuvoton,ma35d1-sdhci" },
>> +	{}
>> +};
>> +
>> +static struct platform_driver sdhci_ma35_driver = {
>> +	.driver	= {
>> +		.name	= "sdhci-ma35",
>> +		.of_match_table = sdhci_ma35_dt_ids,
>> +	},
>> +	.probe	= ma35_probe,
>> +	.remove_new = sdhci_pltfm_remove,
>> +};
>> +module_platform_driver(sdhci_ma35_driver);
>> +
>> +MODULE_DESCRIPTION("SDHCI platform driver for Nuvoton MA35");
>> +MODULE_AUTHOR("Shan-Chun Hung <shanchun1218@google.com>");
> "From:" is shanchun1218@gmail.com
> "Signed-off-by" is shanchun1218@gmail.com
>
> but here it is shanchun1218@google.com
>
> Please be consistent
I will fix it.
>
>> +MODULE_LICENSE("GPL v2");
> checkpatch says:
>
> WARNING: Prefer "GPL" over "GPL v2" - see commit bf7fbeeae6db ("module: Cure the MODULE_LICENSE "GPL" vs. "GPL v2" bogosity")
> #351: FILE: drivers/mmc/host/sdhci-of-ma35d1.c:291:
> +MODULE_LICENSE("GPL v2");
I will fix it.
>> --
>> 2.25.1

Best Regards,

Shan-Chun

Re: [PATCH v2 2/2] mmc: sdhci-of-ma35d1: Add Nuvoton MA35D1 SDHCI driver

[Adrian Hunter]

On 28/06/24 09:30, Shan-Chun Hung wrote:
> On 2024/6/27 下午 05:40, Adrian Hunter wrote:
>> On 26/06/24 12:49, Shan-Chun Hung wrote:
>>> +    /* Enable INCR16 and INCR8 */
>> Comment would be more useful if it said what INCR16 and
>> INCR8 are
> 
> I will modify as follows:
> 
> /*
>  * Split data into chunks of 16 or 8 bytes for sending.
>  * Each chunk transfer is guaranteed to be uninterrupted on the bus.
>  */
> 

AFAICT, it relates to AHB bus DMA burst size, so maybe add
that if it is correct.

Re: [PATCH v2 2/2] mmc: sdhci-of-ma35d1: Add Nuvoton MA35D1 SDHCI driver

[Shan-Chun Hung]

Dear Adrian,

Yes, that is correct. It relates to the AHB bus DMA burst size.

     /*
      * Split data into chunks of 16 or 8 bytes for sending.
      * Each chunk transfer is guaranteed to be uninterrupted on the bus.
      * This likely corresponds to the AHB bus DMA burst size.
      */

Best Regards,

Shan-Chun

On 2024/6/28 下午 02:59, Adrian Hunter wrote:
> On 28/06/24 09:30, Shan-Chun Hung wrote:
>> On 2024/6/27 下午 05:40, Adrian Hunter wrote:
>>> On 26/06/24 12:49, Shan-Chun Hung wrote:
>>>> +    /* Enable INCR16 and INCR8 */
>>> Comment would be more useful if it said what INCR16 and
>>> INCR8 are
>> I will modify as follows:
>>
>> /*
>>   * Split data into chunks of 16 or 8 bytes for sending.
>>   * Each chunk transfer is guaranteed to be uninterrupted on the bus.
>>   */
>>
> AFAICT, it relates to AHB bus DMA burst size, so maybe add
> that if it is correct.
>

Re: [PATCH v2 2/2] mmc: sdhci-of-ma35d1: Add Nuvoton MA35D1 SDHCI driver

[Krzysztof Kozlowski]

On 26/06/2024 11:49, Shan-Chun Hung wrote:
> Add the SDHCI driver for the MA35D1 platform. It is based upon the
> SDHCI interface, but requires some extra initialization.
> 


> +static int ma35_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct sdhci_pltfm_host *pltfm_host;
> +	struct sdhci_host *host;
> +	struct ma35_priv *priv;
> +	int err;
> +	u32 extra, ctl;
> +
> +	host = sdhci_pltfm_init(pdev, &sdhci_ma35_pdata, sizeof(struct ma35_priv));
> +	if (IS_ERR(host))
> +		return PTR_ERR(host);
> +
> +	err = devm_add_action_or_reset(dev, ma35_sdhci_pltfm_free, pdev);
> +	if (err)
> +		return dev_err_probe(dev, err, "Failed to register sdhci_pltfm_free action\n");
> +
> +	/* Extra adma table cnt for cross 128M boundary handling. */
> +	extra = DIV_ROUND_UP_ULL(dma_get_required_mask(dev), SZ_128M);
> +	extra = min(extra, SDHCI_MAX_SEGS);
> +
> +	host->adma_table_cnt += extra;
> +	pltfm_host = sdhci_priv(host);
> +	priv = sdhci_pltfm_priv(pltfm_host);
> +
> +	pltfm_host->clk = devm_clk_get_optional_enabled(dev, NULL);
> +	if (IS_ERR(pltfm_host->clk))
> +		return dev_err_probe(dev, IS_ERR(pltfm_host->clk), "failed to get clk\n");

Ykes, you cannot return IS_ERR.

> +
> +	err = mmc_of_parse(host->mmc);
> +	if (err)
> +		return err;
> +
> +	priv->rst = devm_reset_control_get_exclusive(dev, NULL);
> +	if (IS_ERR(priv->rst))
> +		return dev_err_probe(dev, PTR_ERR(priv->rst), "failed to get reset control\n");
> +


Best regards,
Krzysztof

Re: [PATCH v2 2/2] mmc: sdhci-of-ma35d1: Add Nuvoton MA35D1 SDHCI driver

[Shan-Chun Hung]

Dear Krzysztof,

Thanks for your review.

On 2024/6/28 下午 03:31, Krzysztof Kozlowski wrote:
> On 26/06/2024 11:49, Shan-Chun Hung wrote:
>> Add the SDHCI driver for the MA35D1 platform. It is based upon the
>> SDHCI interface, but requires some extra initialization.
>>
>
>> +static int ma35_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct sdhci_pltfm_host *pltfm_host;
>> +	struct sdhci_host *host;
>> +	struct ma35_priv *priv;
>> +	int err;
>> +	u32 extra, ctl;
>> +
>> +	host = sdhci_pltfm_init(pdev, &sdhci_ma35_pdata, sizeof(struct ma35_priv));
>> +	if (IS_ERR(host))
>> +		return PTR_ERR(host);
>> +
>> +	err = devm_add_action_or_reset(dev, ma35_sdhci_pltfm_free, pdev);
>> +	if (err)
>> +		return dev_err_probe(dev, err, "Failed to register sdhci_pltfm_free action\n");
>> +
>> +	/* Extra adma table cnt for cross 128M boundary handling. */
>> +	extra = DIV_ROUND_UP_ULL(dma_get_required_mask(dev), SZ_128M);
>> +	extra = min(extra, SDHCI_MAX_SEGS);
>> +
>> +	host->adma_table_cnt += extra;
>> +	pltfm_host = sdhci_priv(host);
>> +	priv = sdhci_pltfm_priv(pltfm_host);
>> +
>> +	pltfm_host->clk = devm_clk_get_optional_enabled(dev, NULL);
>> +	if (IS_ERR(pltfm_host->clk))
>> +		return dev_err_probe(dev, IS_ERR(pltfm_host->clk), "failed to get clk\n");
> Ykes, you cannot return IS_ERR.
I will fix it to PTR_ERR.
>> +
>> +	err = mmc_of_parse(host->mmc);
>> +	if (err)
>> +		return err;
>> +
>> +	priv->rst = devm_reset_control_get_exclusive(dev, NULL);
>> +	if (IS_ERR(priv->rst))
>> +		return dev_err_probe(dev, PTR_ERR(priv->rst), "failed to get reset control\n");
>> +
>
> Best regards,
> Krzysztof

Best Regards,

Shan-Chun