[PATCH 0/2] Add support for Nuvovon MA35D1 SDHCI

[PATCH 0/2] Add support for Nuvovon MA35D1 SDHCI

[Shan-Chun Hung]

From: schung <schung@nuvoton.com>

This patch series adds the SDHCI driver for the Nuvoton MA35D1 series
platform. It includes DT binding documentation, the MA35D1 SDHCI driver.

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

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

 .../bindings/mmc/nuvoton,ma35d1-sdhci.yaml    | 106 +++++++
 drivers/mmc/host/Kconfig                      |  13 +
 drivers/mmc/host/Makefile                     |   1 +
 drivers/mmc/host/sdhci-of-ma35d1.c            | 297 ++++++++++++++++++
 4 files changed, 417 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 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: schung <schung@nuvoton.com>
---
 .../bindings/mmc/nuvoton,ma35d1-sdhci.yaml    | 106 ++++++++++++++++++
 1 file changed, 106 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..173449360dea
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/nuvoton,ma35d1-sdhci.yaml
@@ -0,0 +1,106 @@
+# 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>
+
+description: |
+  This controller on Nuvoton MA35 family SoCs provides an interface for MMC, SD, and
+  SDIO types of memory cards.
+
+properties:
+  compatible:
+    oneOf:
+      - enum:
+          - nuvoton,ma35d1-sdhci
+  reg:
+    maxItems: 1
+    description: The SDHCI registers
+
+  interrupts:
+    maxItems: 1
+
+  pinctrl-names:
+    description:
+      Should at least contain default and state_uhs.
+    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:
+    minItems: 1
+    description: The SDHCI bus clock
+
+  resets:
+    maxItems: 1
+    description:
+      Phandle and reset specifier pair to softreset line of sdhci.
+
+  nuvoton,sys:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      Phandle to the syscon that configure sdhci votage stable
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - pinctrl-names
+  - pinctrl-0
+
+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>;
+        sdhci0: sdhci@40180000 {
+            compatible = "nuvoton,ma35d1-sdhci";
+            reg = <0x0 0x40180000 0x0 0x2000>;
+            interrupts = <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>;
+            clocks = <&clk SDH0_GATE>;
+            pinctrl-names = "default";
+            pinctrl-0 = <&pinctrl_sdhci0>;
+            bus-width = <4>;
+            max-frequency = <100000000>;
+            no-1-8-v;
+            status = "disabled";
+        };
+
+        sdhci1: sdhci@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>;
+            bus-width = <8>;
+            max-frequency = <200000000>;
+            status = "disabled";
+        };
+    };
--
2.25.1

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

[Shan-Chun Hung]

This adds the SDHCI driver for the MA35 series SoC. It is based upon the
SDHCI interface, but requires some extra initialization.

Signed-off-by: schung <schung@nuvoton.com>
---
 drivers/mmc/host/Kconfig           |  13 ++
 drivers/mmc/host/Makefile          |   1 +
 drivers/mmc/host/sdhci-of-ma35d1.c | 297 +++++++++++++++++++++++++++++
 3 files changed, 311 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..e993901ebedd 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -252,6 +252,19 @@ 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_A35 || COMPILE_TEST
+	depends on MMC_SDHCI_PLTFM
+	depends on OF && COMMON_CLK
+	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..7714a5ab463d
--- /dev/null
+++ b/drivers/mmc/host/sdhci-of-ma35d1.c
@@ -0,0 +1,297 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2024 Nuvoton Technology Corp.
+ *
+ * Author: Shan-Chun Hung <shanchun1218@gmail.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/dma-mapping.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/mmc/mmc.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+#include <linux/slab.h>
+#include "sdhci-pltfm.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)
+
+#define BOUNDARY_OK(addr, len) \
+	((addr | (SZ_128M - 1)) == ((addr + len - 1) | (SZ_128M - 1)))
+
+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,		32},
+	{ SDHCI_BLOCK_SIZE,		32},
+	{ SDHCI_INT_ENABLE,		32},
+	{ SDHCI_SIGNAL_ENABLE,		32},
+	{ SDHCI_AUTO_CMD_STATUS,	32},
+	{ SDHCI_HOST_CONTROL,		32},
+	{ SDHCI_TIMEOUT_CONTROL,	 8},
+	{ MA35_SDHCI_MSHCCTL,		32},
+	{ MA35_SDHCI_MBIUCTL,		32},
+};
+
+/*
+ * 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 || BOUNDARY_OK(addr, len))) {
+		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 */
+	usleep_range(5000, 5500);
+}
+
+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);
+
+	/* Limitations require a reset SD/eMMC before tuning. */
+	if (!IS_ERR(priv->rst)) {
+		int idx;
+		u32 *val;
+
+		val = kmalloc(ARRAY_SIZE(restore_data), GFP_KERNEL);
+		for (idx = 0; idx < ARRAY_SIZE(restore_data); idx++) {
+			if (restore_data[idx].width == 32)
+				val[idx] = sdhci_readl(host, restore_data[idx].reg);
+			else if (restore_data[idx].width == 8)
+				val[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 == 32)
+				sdhci_writel(host, val[idx], restore_data[idx].reg);
+			else if (restore_data[idx].width == 8)
+				sdhci_writeb(host, val[idx], restore_data[idx].reg);
+		}
+
+		kfree(val);
+	}
+
+	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 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);
+
+	/*
+	 * extra adma table cnt for cross 128M boundary handling.
+	 */
+	extra = DIV_ROUND_UP_ULL(dma_get_required_mask(&pdev->dev), SZ_128M);
+	if (extra > SDHCI_MAX_SEGS)
+		extra = SDHCI_MAX_SEGS;
+	host->adma_table_cnt += extra;
+	pltfm_host = sdhci_priv(host);
+	priv = sdhci_pltfm_priv(pltfm_host);
+
+	if (dev->of_node) {
+		pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
+		if (IS_ERR(pltfm_host->clk)) {
+			err = PTR_ERR(pltfm_host->clk);
+			dev_err(&pdev->dev, "failed to get clk: %d\n", err);
+			goto free_pltfm;
+		}
+		err = clk_prepare_enable(pltfm_host->clk);
+		if (err)
+			goto free_pltfm;
+	}
+
+	err = mmc_of_parse(host->mmc);
+	if (err)
+		goto err_clk;
+
+	priv->rst = devm_reset_control_get(&pdev->dev, NULL);
+
+	sdhci_get_of_property(pdev);
+
+	priv->pinctrl = devm_pinctrl_get(&pdev->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(
+				pdev->dev.of_node, "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)
+		goto err_clk;
+
+	/* 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;
+
+err_clk:
+	clk_disable_unprepare(pltfm_host->clk);
+
+free_pltfm:
+	sdhci_pltfm_free(pdev);
+	return err;
+}
+
+static int ma35_remove(struct platform_device *pdev)
+{
+	struct sdhci_host *host = platform_get_drvdata(pdev);
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+
+	sdhci_remove_host(host, 0);
+	clk_disable_unprepare(pltfm_host->clk);
+	sdhci_pltfm_free(pdev);
+
+	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 = ma35_remove,
+};
+module_platform_driver(sdhci_ma35_driver);
+
+MODULE_DESCRIPTION("SDHCI platform driver for Nuvoton ma35d1");
+MODULE_AUTHOR("shanchun1218@google.com");
+MODULE_LICENSE("GPL v2");
--
2.25.1

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

[Rob Herring (Arm)]

On Wed, 19 Jun 2024 13:46:40 +0800, Shan-Chun Hung wrote:
> Add binding for Nuvoton MA35D1 SDHCI controller.
> 
> Signed-off-by: schung <schung@nuvoton.com>
> ---
>  .../bindings/mmc/nuvoton,ma35d1-sdhci.yaml    | 106 ++++++++++++++++++
>  1 file changed, 106 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mmc/nuvoton,ma35d1-sdhci.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/mmc/nuvoton,ma35d1-sdhci.yaml:77:1: [error] syntax error: found character '\t' that cannot start any token (syntax)

dtschema/dtc warnings/errors:
make[2]: *** Deleting file 'Documentation/devicetree/bindings/mmc/nuvoton,ma35d1-sdhci.example.dts'
Documentation/devicetree/bindings/mmc/nuvoton,ma35d1-sdhci.yaml:77:1: found a tab character where an indentation space is expected
make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/mmc/nuvoton,ma35d1-sdhci.example.dts] Error 1
make[2]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/mmc/nuvoton,ma35d1-sdhci.yaml:77:1: found a tab character where an indentation space is expected
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mmc/nuvoton,ma35d1-sdhci.yaml: ignoring, error parsing file
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1430: dt_binding_check] Error 2
make: *** [Makefile:240: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240619054641.277062-2-shanchun1218@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

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

[Krzysztof Kozlowski]

On 19/06/2024 07:46, Shan-Chun Hung wrote:
> Add binding for Nuvoton MA35D1 SDHCI controller.
> 
> Signed-off-by: schung <schung@nuvoton.com>

Since this was not tested, only limited review follows. Please test your
future patches.

> ---
>  .../bindings/mmc/nuvoton,ma35d1-sdhci.yaml    | 106 ++++++++++++++++++
>  1 file changed, 106 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..173449360dea
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/nuvoton,ma35d1-sdhci.yaml
> @@ -0,0 +1,106 @@
> +# 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>
> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> +  This controller on Nuvoton MA35 family SoCs provides an interface for MMC, SD, and
> +  SDIO types of memory cards.
> +
> +properties:
> +  compatible:
> +    oneOf:

Drop

> +      - enum:
> +          - nuvoton,ma35d1-sdhci

Blank line

> +  reg:
> +    maxItems: 1
> +    description: The SDHCI registers

Drop

> +
> +  interrupts:
> +    maxItems: 1
> +
> +  pinctrl-names:
> +    description:
> +      Should at least contain default and state_uhs.

? Contradicts constraints.

> +    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:
> +    minItems: 1

No, maxItems instead.

> +    description: The SDHCI bus clock

Drop

> +
> +  resets:
> +    maxItems: 1
> +    description:
> +      Phandle and reset specifier pair to softreset line of sdhci.

Drop

> +
> +  nuvoton,sys:

1. too generic, what is sys?

> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Phandle to the syscon that configure sdhci votage stable

2. typo: voltage

3. Which syscon?

4. Why you are not implementing regulators?


> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - pinctrl-names
> +  - pinctrl-0
> +
> +unevaluatedProperties: false

Hm? And where is ref to MMC schema?

> +
> +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>;

Fix your indentation.

Use 4 spaces for example indentation.

> +        sdhci0: sdhci@40180000 {

Drop label

> +            compatible = "nuvoton,ma35d1-sdhci";
> +            reg = <0x0 0x40180000 0x0 0x2000>;
> +            interrupts = <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>;
> +            clocks = <&clk SDH0_GATE>;
> +            pinctrl-names = "default";
> +            pinctrl-0 = <&pinctrl_sdhci0>;
> +            bus-width = <4>;
> +            max-frequency = <100000000>;
> +            no-1-8-v;
> +            status = "disabled";

Drop

> +        };
> +
> +        sdhci1: sdhci@40190000 {

Drop this example. One is enough.



Best regards,
Krzysztof

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

[Dragan Simic]

Hello Shan-Chun,

On 2024-06-19 07:46, Shan-Chun Hung wrote:
> This adds the SDHCI driver for the MA35 series SoC. It is based upon 
> the
> SDHCI interface, but requires some extra initialization.
> 
> Signed-off-by: schung <schung@nuvoton.com>

There's a typo in the patch subject: s/Novoton/Nuvoton/

Also, it would be better to use imperative mode in the patch 
description,
i.e. "Add the SDHCI driver..." instead of "This adds the SDHCI..."

> ---
>  drivers/mmc/host/Kconfig           |  13 ++
>  drivers/mmc/host/Makefile          |   1 +
>  drivers/mmc/host/sdhci-of-ma35d1.c | 297 +++++++++++++++++++++++++++++
>  3 files changed, 311 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..e993901ebedd 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -252,6 +252,19 @@ 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_A35 || COMPILE_TEST
> +	depends on MMC_SDHCI_PLTFM
> +	depends on OF && COMMON_CLK
> +	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..7714a5ab463d
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-of-ma35d1.c
> @@ -0,0 +1,297 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2024 Nuvoton Technology Corp.
> + *
> + * Author: Shan-Chun Hung <shanchun1218@gmail.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/mmc/mmc.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>
> +#include "sdhci-pltfm.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)
> +
> +#define BOUNDARY_OK(addr, len) \
> +	((addr | (SZ_128M - 1)) == ((addr + len - 1) | (SZ_128M - 1)))
> +
> +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,		32},
> +	{ SDHCI_BLOCK_SIZE,		32},
> +	{ SDHCI_INT_ENABLE,		32},
> +	{ SDHCI_SIGNAL_ENABLE,		32},
> +	{ SDHCI_AUTO_CMD_STATUS,	32},
> +	{ SDHCI_HOST_CONTROL,		32},
> +	{ SDHCI_TIMEOUT_CONTROL,	 8},
> +	{ MA35_SDHCI_MSHCCTL,		32},
> +	{ MA35_SDHCI_MBIUCTL,		32},
> +};
> +
> +/*
> + * 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 || BOUNDARY_OK(addr, len))) {
> +		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 */
> +	usleep_range(5000, 5500);
> +}
> +
> +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);
> +
> +	/* Limitations require a reset SD/eMMC before tuning. */
> +	if (!IS_ERR(priv->rst)) {
> +		int idx;
> +		u32 *val;
> +
> +		val = kmalloc(ARRAY_SIZE(restore_data), GFP_KERNEL);
> +		for (idx = 0; idx < ARRAY_SIZE(restore_data); idx++) {
> +			if (restore_data[idx].width == 32)
> +				val[idx] = sdhci_readl(host, restore_data[idx].reg);
> +			else if (restore_data[idx].width == 8)
> +				val[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 == 32)
> +				sdhci_writel(host, val[idx], restore_data[idx].reg);
> +			else if (restore_data[idx].width == 8)
> +				sdhci_writeb(host, val[idx], restore_data[idx].reg);
> +		}
> +
> +		kfree(val);
> +	}
> +
> +	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 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);
> +
> +	/*
> +	 * extra adma table cnt for cross 128M boundary handling.
> +	 */
> +	extra = DIV_ROUND_UP_ULL(dma_get_required_mask(&pdev->dev), SZ_128M);
> +	if (extra > SDHCI_MAX_SEGS)
> +		extra = SDHCI_MAX_SEGS;
> +	host->adma_table_cnt += extra;
> +	pltfm_host = sdhci_priv(host);
> +	priv = sdhci_pltfm_priv(pltfm_host);
> +
> +	if (dev->of_node) {
> +		pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
> +		if (IS_ERR(pltfm_host->clk)) {
> +			err = PTR_ERR(pltfm_host->clk);
> +			dev_err(&pdev->dev, "failed to get clk: %d\n", err);
> +			goto free_pltfm;
> +		}
> +		err = clk_prepare_enable(pltfm_host->clk);
> +		if (err)
> +			goto free_pltfm;
> +	}
> +
> +	err = mmc_of_parse(host->mmc);
> +	if (err)
> +		goto err_clk;
> +
> +	priv->rst = devm_reset_control_get(&pdev->dev, NULL);
> +
> +	sdhci_get_of_property(pdev);
> +
> +	priv->pinctrl = devm_pinctrl_get(&pdev->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(
> +				pdev->dev.of_node, "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)
> +		goto err_clk;
> +
> +	/* 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;
> +
> +err_clk:
> +	clk_disable_unprepare(pltfm_host->clk);
> +
> +free_pltfm:
> +	sdhci_pltfm_free(pdev);
> +	return err;
> +}
> +
> +static int ma35_remove(struct platform_device *pdev)
> +{
> +	struct sdhci_host *host = platform_get_drvdata(pdev);
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +
> +	sdhci_remove_host(host, 0);
> +	clk_disable_unprepare(pltfm_host->clk);
> +	sdhci_pltfm_free(pdev);
> +
> +	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 = ma35_remove,
> +};
> +module_platform_driver(sdhci_ma35_driver);
> +
> +MODULE_DESCRIPTION("SDHCI platform driver for Nuvoton ma35d1");
> +MODULE_AUTHOR("shanchun1218@google.com");
> +MODULE_LICENSE("GPL v2");
> --
> 2.25.1

Re: [PATCH 0/2] Add support for Nuvovon MA35D1 SDHCI

[Markus Elfring]

> This patch series adds the SDHCI driver for the Nuvoton MA35D1 series
> platform. It includes DT binding documentation, the MA35D1 SDHCI driver.
…
>   mmc: sdhci-of-ma35d1: Add Novoton MA35D1 SDHCI driver
…

Would you like to refer to a slightly different company name
at a few message places?

Regards,
Markus

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

[Shan-Chun Hung]

Dear Dragan,

Thanks for your review.  I didn't notice the typo , but I will correct it.

Best Regards,
Shan-Chun.

On 2024/6/19 下午 06:18, Dragan Simic wrote:
> Hello Shan-Chun,
>
> On 2024-06-19 07:46, Shan-Chun Hung wrote:
>> This adds the SDHCI driver for the MA35 series SoC. It is based upon the
>> SDHCI interface, but requires some extra initialization.
>>
>> Signed-off-by: schung <schung@nuvoton.com>
>
> There's a typo in the patch subject: s/Novoton/Nuvoton/
>
> Also, it would be better to use imperative mode in the patch description,
> i.e. "Add the SDHCI driver..." instead of "This adds the SDHCI..."
>
>> ---
>>  drivers/mmc/host/Kconfig           |  13 ++
>>  drivers/mmc/host/Makefile          |   1 +
>>  drivers/mmc/host/sdhci-of-ma35d1.c | 297 +++++++++++++++++++++++++++++
>>  3 files changed, 311 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..e993901ebedd 100644
>> --- a/drivers/mmc/host/Kconfig
>> +++ b/drivers/mmc/host/Kconfig
>> @@ -252,6 +252,19 @@ 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_A35 || COMPILE_TEST
>> +    depends on MMC_SDHCI_PLTFM
>> +    depends on OF && COMMON_CLK
>> +    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..7714a5ab463d
>> --- /dev/null
>> +++ b/drivers/mmc/host/sdhci-of-ma35d1.c
>> @@ -0,0 +1,297 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2024 Nuvoton Technology Corp.
>> + *
>> + * Author: Shan-Chun Hung <shanchun1218@gmail.com>
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/module.h>
>> +#include <linux/mmc/mmc.h>
>> +#include <linux/pinctrl/consumer.h>
>> +#include <linux/regmap.h>
>> +#include <linux/reset.h>
>> +#include <linux/slab.h>
>> +#include "sdhci-pltfm.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)
>> +
>> +#define BOUNDARY_OK(addr, len) \
>> +    ((addr | (SZ_128M - 1)) == ((addr + len - 1) | (SZ_128M - 1)))
>> +
>> +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,        32},
>> +    { SDHCI_BLOCK_SIZE,        32},
>> +    { SDHCI_INT_ENABLE,        32},
>> +    { SDHCI_SIGNAL_ENABLE,        32},
>> +    { SDHCI_AUTO_CMD_STATUS,    32},
>> +    { SDHCI_HOST_CONTROL,        32},
>> +    { SDHCI_TIMEOUT_CONTROL,     8},
>> +    { MA35_SDHCI_MSHCCTL,        32},
>> +    { MA35_SDHCI_MBIUCTL,        32},
>> +};
>> +
>> +/*
>> + * 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 || BOUNDARY_OK(addr, len))) {
>> +        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 */
>> +    usleep_range(5000, 5500);
>> +}
>> +
>> +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);
>> +
>> +    /* Limitations require a reset SD/eMMC before tuning. */
>> +    if (!IS_ERR(priv->rst)) {
>> +        int idx;
>> +        u32 *val;
>> +
>> +        val = kmalloc(ARRAY_SIZE(restore_data), GFP_KERNEL);
>> +        for (idx = 0; idx < ARRAY_SIZE(restore_data); idx++) {
>> +            if (restore_data[idx].width == 32)
>> +                val[idx] = sdhci_readl(host, restore_data[idx].reg);
>> +            else if (restore_data[idx].width == 8)
>> +                val[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 == 32)
>> +                sdhci_writel(host, val[idx], restore_data[idx].reg);
>> +            else if (restore_data[idx].width == 8)
>> +                sdhci_writeb(host, val[idx], restore_data[idx].reg);
>> +        }
>> +
>> +        kfree(val);
>> +    }
>> +
>> +    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 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);
>> +
>> +    /*
>> +     * extra adma table cnt for cross 128M boundary handling.
>> +     */
>> +    extra = DIV_ROUND_UP_ULL(dma_get_required_mask(&pdev->dev), 
>> SZ_128M);
>> +    if (extra > SDHCI_MAX_SEGS)
>> +        extra = SDHCI_MAX_SEGS;
>> +    host->adma_table_cnt += extra;
>> +    pltfm_host = sdhci_priv(host);
>> +    priv = sdhci_pltfm_priv(pltfm_host);
>> +
>> +    if (dev->of_node) {
>> +        pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
>> +        if (IS_ERR(pltfm_host->clk)) {
>> +            err = PTR_ERR(pltfm_host->clk);
>> +            dev_err(&pdev->dev, "failed to get clk: %d\n", err);
>> +            goto free_pltfm;
>> +        }
>> +        err = clk_prepare_enable(pltfm_host->clk);
>> +        if (err)
>> +            goto free_pltfm;
>> +    }
>> +
>> +    err = mmc_of_parse(host->mmc);
>> +    if (err)
>> +        goto err_clk;
>> +
>> +    priv->rst = devm_reset_control_get(&pdev->dev, NULL);
>> +
>> +    sdhci_get_of_property(pdev);
>> +
>> +    priv->pinctrl = devm_pinctrl_get(&pdev->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(
>> +                pdev->dev.of_node, "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)
>> +        goto err_clk;
>> +
>> +    /* 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;
>> +
>> +err_clk:
>> +    clk_disable_unprepare(pltfm_host->clk);
>> +
>> +free_pltfm:
>> +    sdhci_pltfm_free(pdev);
>> +    return err;
>> +}
>> +
>> +static int ma35_remove(struct platform_device *pdev)
>> +{
>> +    struct sdhci_host *host = platform_get_drvdata(pdev);
>> +    struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +
>> +    sdhci_remove_host(host, 0);
>> +    clk_disable_unprepare(pltfm_host->clk);
>> +    sdhci_pltfm_free(pdev);
>> +
>> +    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 = ma35_remove,
>> +};
>> +module_platform_driver(sdhci_ma35_driver);
>> +
>> +MODULE_DESCRIPTION("SDHCI platform driver for Nuvoton ma35d1");
>> +MODULE_AUTHOR("shanchun1218@google.com");
>> +MODULE_LICENSE("GPL v2");
>> -- 
>> 2.25.1

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

[Markus Elfring]

…
> Signed-off-by: schung <schung@nuvoton.com>

Can an other personal name eventually be more appropriate here
(according to the Developer's Certificate of Origin)?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc4#n438


…
> +++ b/drivers/mmc/host/sdhci-of-ma35d1.c
> @@ -0,0 +1,297 @@
…
> +MODULE_AUTHOR("shanchun1218@google.com");
…

How do you think about to improve such information another bit?

Regards,
Markus

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

[Andy Shevchenko]

On Wed, Jun 19, 2024 at 7:47 AM Shan-Chun Hung <shanchun1218@gmail.com> wrote:
>
> This adds the SDHCI driver for the MA35 series SoC. It is based upon the
> SDHCI interface, but requires some extra initialization.
>
> Signed-off-by: schung <schung@nuvoton.com>

Even I agree with Markus' remarks, so please correct your SoB by using
something similar to the From line.

...

> +config MMC_SDHCI_OF_MA35D1
> +       tristate "SDHCI OF support for the MA35D1 SDHCI controller"
> +       depends on ARCH_A35 || COMPILE_TEST
> +       depends on MMC_SDHCI_PLTFM

> +       depends on OF && COMMON_CLK

OF is not compile dependency AFAICS, if you want make it functional, use

  depends on OF || COMPILE_TEST

...

You are missing a lot of header inclusions, please follow IWYU principle.

+ array_size.h
+ bits.h

> +#include <linux/clk.h>

+ device.h

> +#include <linux/dma-mapping.h>

+ err.h

> +#include <linux/mfd/syscon.h>

+ math.h
+ mod_devicetable.h

> +#include <linux/module.h>
> +#include <linux/mmc/mmc.h>
> +#include <linux/pinctrl/consumer.h>

+ platform_device.h

> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>

+ types.h

...

> +#define BOUNDARY_OK(addr, len) \
> +       ((addr | (SZ_128M - 1)) == ((addr + len - 1) | (SZ_128M - 1)))

Besides sizes.h being missed, this can be done with help of ALIGN()
macro (or alike). So, kill this and use the globally defined macro
inline.

...

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

  /*
   * The comment style is wrong and
   * the indentation in the second line.
   * Fix it as in this example.
   */

...

> +static void ma35_voltage_switch(struct sdhci_host *host)
> +{
> +       /* Wait for 5ms after set 1.8V signal enable bit */
> +       usleep_range(5000, 5500);

Use fsleep()

> +}
> +
> +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);
> +
> +       /* Limitations require a reset SD/eMMC before tuning. */
> +       if (!IS_ERR(priv->rst)) {

It's way too big for indentation, moreover it uses an unusual pattern,
usually we check for an error case first. So, invert the conditional
and this all code will become much better.

> +               int idx;
> +               u32 *val;
> +
> +               val = kmalloc(ARRAY_SIZE(restore_data), GFP_KERNEL);
> +               for (idx = 0; idx < ARRAY_SIZE(restore_data); idx++) {
> +                       if (restore_data[idx].width == 32)

sizeof(u32) ?

> +                               val[idx] = sdhci_readl(host, restore_data[idx].reg);
> +                       else if (restore_data[idx].width == 8)

sizeof(u8) ?

> +                               val[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 == 32)
> +                               sdhci_writel(host, val[idx], restore_data[idx].reg);
> +                       else if (restore_data[idx].width == 8)
> +                               sdhci_writeb(host, val[idx], restore_data[idx].reg);

As per above?

> +               }
> +
> +               kfree(val);
> +       }
> +
> +       return sdhci_execute_tuning(mmc, opcode);
> +}

...

> +static int ma35_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;

Since you have it, use it!

> +       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));

One line?

> +       if (IS_ERR(host))
> +               return PTR_ERR(host);
> +
> +       /*
> +        * extra adma table cnt for cross 128M boundary handling.
> +        */

Wrong comment style.

> +       extra = DIV_ROUND_UP_ULL(dma_get_required_mask(&pdev->dev), SZ_128M);
> +       if (extra > SDHCI_MAX_SEGS)
> +               extra = SDHCI_MAX_SEGS;

min() ? clamp() ?

> +       host->adma_table_cnt += extra;
> +       pltfm_host = sdhci_priv(host);
> +       priv = sdhci_pltfm_priv(pltfm_host);

> +       if (dev->of_node) {

Why?

> +               pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
> +               if (IS_ERR(pltfm_host->clk)) {

> +                       err = PTR_ERR(pltfm_host->clk);
> +                       dev_err(&pdev->dev, "failed to get clk: %d\n", err);

Use

  return dev_err_probe(...);

> +                       goto free_pltfm;

This is wrong. You may not call non-devm before devm ones, otherwise
it makes a room for subtle mistakes on remove-probe or unbind-bind
cycles. Have you tested that?

> +               }
> +               err = clk_prepare_enable(pltfm_host->clk);
> +               if (err)
> +                       goto free_pltfm;

Use _enabled variant of devm_clk_get() instead.

> +       }
> +
> +       err = mmc_of_parse(host->mmc);
> +       if (err)
> +               goto err_clk;
> +
> +       priv->rst = devm_reset_control_get(&pdev->dev, NULL);

No error check?!

> +       sdhci_get_of_property(pdev);
> +
> +       priv->pinctrl = devm_pinctrl_get(&pdev->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(
> +                               pdev->dev.of_node, "nuvoton,sys");

dev_of_node(dev)

> +
> +               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)
> +               goto err_clk;
> +
> +       /* 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;

> +err_clk:
> +       clk_disable_unprepare(pltfm_host->clk);

This will go with the _enabled variant being used.

> +free_pltfm:
> +       sdhci_pltfm_free(pdev);

This should go to be correct in ordering.

> +       return err;
> +}
> +
> +static int ma35_remove(struct platform_device *pdev)

Use remove_new callback.

> +{
> +       struct sdhci_host *host = platform_get_drvdata(pdev);
> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +
> +       sdhci_remove_host(host, 0);

> +       clk_disable_unprepare(pltfm_host->clk);
> +       sdhci_pltfm_free(pdev);

At least these two will go away as per probe error path.

> +       return 0;
> +}

...

> +MODULE_AUTHOR("shanchun1218@google.com");

Needs to be fixed as Markus said.


-- 
With Best Regards,
Andy Shevchenko

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

[Shan-Chun Hung]

Dear Markus,

Thanks for your review.

On 2024/6/20 上午 12:18, Markus Elfring wrote:
> …
>> Signed-off-by: schung <schung@nuvoton.com>
> Can an other personal name eventually be more appropriate here
> (according to the Developer's Certificate of Origin)?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc4#n438
I will fix it in the next version.
>
> …
>> +++ b/drivers/mmc/host/sdhci-of-ma35d1.c
>> @@ -0,0 +1,297 @@
> …
>> +MODULE_AUTHOR("shanchun1218@google.com");
> …
>
> How do you think about to improve such information another bit?
>
> Regards,
> Markus
I will add my name to fix it.

Best Regards,

Shan-Chun

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

[Krzysztof Kozlowski]

On 20/06/2024 08:49, Shan-Chun Hung wrote:
> Dear Markus,
> 
> Thanks for your review.
> 
> On 2024/6/20 上午 12:18, Markus Elfring wrote:
>> …
>>> Signed-off-by: schung <schung@nuvoton.com>
>> Can an other personal name eventually be more appropriate here
>> (according to the Developer's Certificate of Origin)?
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc4#n438
> I will fix it in the next version.
>>
>> …
>>> +++ b/drivers/mmc/host/sdhci-of-ma35d1.c
>>> @@ -0,0 +1,297 @@
>> …
>>> +MODULE_AUTHOR("shanchun1218@google.com");
>> …
>>
>> How do you think about to improve such information another bit?
>>
>> Regards,
>> Markus
> I will add my name to fix it.

<form letter>
Feel free to ignore all comments from Markus, regardless whether the
suggestion is reasonable or not. This person is banned from LKML and
several maintainers ignore Markus' feedback, because it is just a waste
of time.
</form letter>

Best regards,
Krzysztof

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

[Shan-Chun Hung]

Dear Krzysztof,

Thank you for your advice.

Best Regards,

Shan-Chun


On 2024/6/20 下午 02:58, Krzysztof Kozlowski wrote:
> On 20/06/2024 08:49, Shan-Chun Hung wrote:
>> Dear Markus,
>>
>> Thanks for your review.
>>
>> On 2024/6/20 上午 12:18, Markus Elfring wrote:
>>> …
>>>> Signed-off-by: schung<schung@nuvoton.com>
>>> Can an other personal name eventually be more appropriate here
>>> (according to the Developer's Certificate of Origin)?
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc4#n438
>> I will fix it in the next version.
>>> …
>>>> +++ b/drivers/mmc/host/sdhci-of-ma35d1.c
>>>> @@ -0,0 +1,297 @@
>>> …
>>>> +MODULE_AUTHOR("shanchun1218@google.com");
>>> …
>>>
>>> How do you think about to improve such information another bit?
>>>
>>> Regards,
>>> Markus
>> I will add my name to fix it.
> <form letter>
> Feel free to ignore all comments from Markus, regardless whether the
> suggestion is reasonable or not. This person is banned from LKML and
> several maintainers ignore Markus' feedback, because it is just a waste
> of time.
> </form letter>
>
> Best regards,
> Krzysztof
>

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

[Shan-Chun Hung]

Dear Krzysztof,

Thanks for you review

On 2024/6/19 下午 03:29, Krzysztof Kozlowski wrote:
> On 19/06/2024 07:46, Shan-Chun Hung wrote:
>> Add binding for Nuvoton MA35D1 SDHCI controller.
>>
>> Signed-off-by: schung<schung@nuvoton.com>
> Since this was not tested, only limited review follows. Please test your
> future patches.
>
>> ---
>>   .../bindings/mmc/nuvoton,ma35d1-sdhci.yaml    | 106 ++++++++++++++++++
>>   1 file changed, 106 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..173449360dea
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mmc/nuvoton,ma35d1-sdhci.yaml
>> @@ -0,0 +1,106 @@
>> +# 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>
>> +
>> +description: |
> Do not need '|' unless you need to preserve formatting.
I will remove '|'
>> +  This controller on Nuvoton MA35 family SoCs provides an interface for MMC, SD, and
>> +  SDIO types of memory cards.
>> +
>> +properties:
>> +  compatible:
>> +    oneOf:
> Drop

I will remove oneof.

>> +      - enum:
>> +          - nuvoton,ma35d1-sdhci
> Blank line

I will fix it.

>> +  reg:
>> +    maxItems: 1
>> +    description: The SDHCI registers
> Drop

I will remove description.

>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  pinctrl-names:
>> +    description:
>> +      Should at least contain default and state_uhs.
> ? Contradicts constraints.
I will modify the description to "Should at least contain default. 
state_uhs is mandatory in this scenario."
>> +    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:
>> +    minItems: 1
> No, maxItems instead.

I will fix it.

>> +    description: The SDHCI bus clock
> Drop

I will remove it.

>> +
>> +  resets:
>> +    maxItems: 1
>> +    description:
>> +      Phandle and reset specifier pair to softreset line of sdhci.
> Drop

I will remove it.

>> +
>> +  nuvoton,sys:
> 1. too generic, what is sys?
>
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description:
>> +      Phandle to the syscon that configure sdhci votage stable

I will modify description as follows:
   nuvoton,sys:
     $ref: /schemas/types.yaml#/definitions/phandle
     description: phandle to access GCR (Global Control Register) registers.

>> 2. typo: voltage
I will fix it.
>> 3. Which syscon?
same as 1.
>> 4. Why you are not implementing regulators?
>>
I will add "vqmmc-supply = <&sdhci1_vqmmc_regulator>;" in the examples.

This requlator is used by regulator-gpio driver.

>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - clocks
>> +  - pinctrl-names
>> +  - pinctrl-0
>> +
>> +unevaluatedProperties: false
> Hm? And where is ref to MMC schema?

I will add as follows:

allOf:
   - $ref: sdhci-common.yaml#

>> +
>> +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>;
> Fix your indentation.
>
> Use 4 spaces for example indentation.
I will fix it.
>> +        sdhci0: sdhci@40180000 {
> Drop label
I will fix it.
>> +            compatible = "nuvoton,ma35d1-sdhci";
>> +            reg = <0x0 0x40180000 0x0 0x2000>;
>> +            interrupts = <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>;
>> +            clocks = <&clk SDH0_GATE>;
>> +            pinctrl-names = "default";
>> +            pinctrl-0 = <&pinctrl_sdhci0>;
>> +            bus-width = <4>;
>> +            max-frequency = <100000000>;
>> +            no-1-8-v;
>> +            status = "disabled";
> Drop
I will fix it.
>> +        };
>> +
>> +        sdhci1: sdhci@40190000 {
> Drop this example. One is enough.
>
>
>
> Best regards,
> Krzysztof
OK , I will remove one.

Best Regards

Shan-Chun

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

[Krzysztof Kozlowski]

On 21/06/2024 01:53, Shan-Chun Hung wrote:
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +
>>> +  pinctrl-names:
>>> +    description:
>>> +      Should at least contain default and state_uhs.
>> ? Contradicts constraints.
> I will modify the description to "Should at least contain default. 
> state_uhs is mandatory in this scenario."

Then just drop it. Don't repeat constraints in free form text.


Best regards,
Krzysztof

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

[Shan-Chun Hung]

Dear Krzysztof,

Thanks for your review.

On 2024/6/21 下午 02:04, Krzysztof Kozlowski wrote:
> On 21/06/2024 01:53, Shan-Chun Hung wrote:
>>>> +
>>>> +  interrupts:
>>>> +    maxItems: 1
>>>> +
>>>> +  pinctrl-names:
>>>> +    description:
>>>> +      Should at least contain default and state_uhs.
>>> ? Contradicts constraints.
>> I will modify the description to "Should at least contain default.
>> state_uhs is mandatory in this scenario."
> Then just drop it. Don't repeat constraints in free form text.
>
>
> Best regards,
> Krzysztof
OK , I will drop the description.

Best Regards,

Shan-Chun

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

[Shan-Chun Hung]

Dear Andy,

Thanks for your review.

On 2024/6/20 上午 03:09, Andy Shevchenko wrote:
> On Wed, Jun 19, 2024 at 7:47 AM Shan-Chun Hung<shanchun1218@gmail.com>  wrote:
>> This adds the SDHCI driver for the MA35 series SoC. It is based upon the
>> SDHCI interface, but requires some extra initialization.
>>
>> Signed-off-by: schung<schung@nuvoton.com>
> Even I agree with Markus' remarks, so please correct your SoB by using
> something similar to the From line.
I will fix it.
>
> ...
>
>> +config MMC_SDHCI_OF_MA35D1
>> +       tristate "SDHCI OF support for the MA35D1 SDHCI controller"
>> +       depends on ARCH_A35 || COMPILE_TEST
>> +       depends on MMC_SDHCI_PLTFM
>> +       depends on OF && COMMON_CLK
> OF is not compile dependency AFAICS, if you want make it functional, use
>
>    depends on OF || COMPILE_TEST
>
> ...
>
> You are missing a lot of header inclusions, please follow IWYU principle.
I am not familiar with IWYU yet, but I will learn it and use it for 
checks later on.

For new, I am adding these header files.

> + array_size.h
> + bits.h
>
>> +#include <linux/clk.h>
> + device.h
>
>> +#include <linux/dma-mapping.h>
> + err.h
>
>> +#include <linux/mfd/syscon.h>
> + math.h
> + mod_devicetable.h
>
>> +#include <linux/module.h>
>> +#include <linux/mmc/mmc.h>
>> +#include <linux/pinctrl/consumer.h>
> + platform_device.h
>
>> +#include <linux/regmap.h>
>> +#include <linux/reset.h>
>> +#include <linux/slab.h>
> + types.h
> ...
>
>> +#define BOUNDARY_OK(addr, len) \
>> +       ((addr | (SZ_128M - 1)) == ((addr + len - 1) | (SZ_128M - 1)))
> Besides sizes.h being missed, this can be done with help of ALIGN()
> macro (or alike). So, kill this and use the globally defined macro
> inline.
I will add sizes.h and directly apply globally defined  ALIGN() macro 
instead
> ...
>
>> +       /* If the clock frequency exceeds MMC_HIGH_52_MAX_DTR,
>> +        *      disable command conflict check.
>> +        */
>    /*
>     * The comment style is wrong and
>     * the indentation in the second line.
>     * Fix it as in this example.
>     */
>
> ...
I will fix it.
>> +static void ma35_voltage_switch(struct sdhci_host *host)
>> +{
>> +       /* Wait for 5ms after set 1.8V signal enable bit */
>> +       usleep_range(5000, 5500);
> Use fsleep()
I will use fsleep() instead of usleep_range().
>> +}
>> +
>> +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);
>> +
>> +       /* Limitations require a reset SD/eMMC before tuning. */
>> +       if (!IS_ERR(priv->rst)) {
> It's way too big for indentation, moreover it uses an unusual pattern,
> usually we check for an error case first. So, invert the conditional
> and this all code will become much better.
I will fix it.
>> +               int idx;
>> +               u32 *val;
>> +
>> +               val = kmalloc(ARRAY_SIZE(restore_data), GFP_KERNEL);
>> +               for (idx = 0; idx < ARRAY_SIZE(restore_data); idx++) {
>> +                       if (restore_data[idx].width == 32)
> sizeof(u32) ?
Your idea is better, I will change it.
>> +                               val[idx] = sdhci_readl(host, restore_data[idx].reg);
>> +                       else if (restore_data[idx].width == 8)
> sizeof(u8) ?
I will fix it.
>> +                               val[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 == 32)
>> +                               sdhci_writel(host, val[idx], restore_data[idx].reg);
>> +                       else if (restore_data[idx].width == 8)
>> +                               sdhci_writeb(host, val[idx], restore_data[idx].reg);
> As per above?
I will fix it.
>> +               }
>> +
>> +               kfree(val);
>> +       }
>> +
>> +       return sdhci_execute_tuning(mmc, opcode);
>> +}
> ...
>
>> +static int ma35_probe(struct platform_device *pdev)
>> +{
>> +       struct device *dev = &pdev->dev;
> Since you have it, use it!
I will use "dev" instead of "&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));
> One line?
I will fix it.
>> +       if (IS_ERR(host))
>> +               return PTR_ERR(host);
>> +
>> +       /*
>> +        * extra adma table cnt for cross 128M boundary handling.
>> +        */
> Wrong comment style.
I will fix it.
>> +       extra = DIV_ROUND_UP_ULL(dma_get_required_mask(&pdev->dev), SZ_128M);
>> +       if (extra > SDHCI_MAX_SEGS)
>> +               extra = SDHCI_MAX_SEGS;
> min() ? clamp() ?
I will use min() macro to fix it
>> +       host->adma_table_cnt += extra;
>> +       pltfm_host = sdhci_priv(host);
>> +       priv = sdhci_pltfm_priv(pltfm_host);
>> +       if (dev->of_node) {
> Why?
I will remove the "if ..."
>> +               pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
>> +               if (IS_ERR(pltfm_host->clk)) {
>> +                       err = PTR_ERR(pltfm_host->clk);
>> +                       dev_err(&pdev->dev, "failed to get clk: %d\n", err);
> Use
>
>    return dev_err_probe(...);
I will use dev_err_probe() instead of dev_err()
>> +                       goto free_pltfm;
> This is wrong. You may not call non-devm before devm ones, otherwise
> it makes a room for subtle mistakes on remove-probe or unbind-bind
> cycles. Have you tested that?
I have tested it, there is no error messages during driver initial process.

My thought is that sdhci_pltfm_init and sdhci_pltfm_free are used together.

If there's any error after the successful execution of sdhci_pltfm_init, 
I'll use sdhci_pltfm_free.

I am not entirely sure if this answers your question.

>> +               }
>> +               err = clk_prepare_enable(pltfm_host->clk);
>> +               if (err)
>> +                       goto free_pltfm;
> Use _enabled variant of devm_clk_get() instead.
I will use devm_clk_get_optional_enabled() instead.
>> +       }
>> +
>> +       err = mmc_of_parse(host->mmc);
>> +       if (err)
>> +               goto err_clk;
>> +
>> +       priv->rst = devm_reset_control_get(&pdev->dev, NULL);
> No error check?!
I will add an error check.
>> +       sdhci_get_of_property(pdev);
>> +
>> +       priv->pinctrl = devm_pinctrl_get(&pdev->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(
>> +                               pdev->dev.of_node, "nuvoton,sys");
> dev_of_node(dev)
I will fix it.
>> +
>> +               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)
>> +               goto err_clk;
>> +
>> +       /* 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;
>> +err_clk:
>> +       clk_disable_unprepare(pltfm_host->clk);
> This will go with the _enabled variant being used.
I will use devm_clk_get_optional_enabled, so I will remove it.
>> +free_pltfm:
>> +       sdhci_pltfm_free(pdev);
> This should go to be correct in ordering.

I am not entirely sure if it is a similar to "goto free_pltfm;" issue.

>> +       return err;
>> +}
>> +
>> +static int ma35_remove(struct platform_device *pdev)
> Use remove_new callback.
I will fix it.
>> +{
>> +       struct sdhci_host *host = platform_get_drvdata(pdev);
>> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +
>> +       sdhci_remove_host(host, 0);
>> +       clk_disable_unprepare(pltfm_host->clk);
>> +       sdhci_pltfm_free(pdev);
> At least these two will go away as per probe error path.
I will use sdhci_pltfm_remove instead of  the ma35_remove.
>> +       return 0;
>> +}
> ...
>
>> +MODULE_AUTHOR("shanchun1218@google.com");
> Needs to be fixed as Markus said.
I will fix it.

Best Regards,

Shan-Chun

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

[Andy Shevchenko]

On Fri, Jun 21, 2024 at 10:06 AM Shan-Chun Hung <shanchun1218@gmail.com> wrote:
> On 2024/6/20 上午 03:09, Andy Shevchenko wrote:
> > On Wed, Jun 19, 2024 at 7:47 AM Shan-Chun Hung<shanchun1218@gmail.com>  wrote:

...

> > You are missing a lot of header inclusions, please follow IWYU principle.
> I am not familiar with IWYU yet, but I will learn it and use it for
> checks later on.

"Include What You Use". But some of the headers may be omitted as they
are guaranteed to be included by others. It's a bit hard because one
should know and follow the kernel development, currently the headers
in the kernel are a bit of a mess.

...

> >> +#define BOUNDARY_OK(addr, len) \
> >> +       ((addr | (SZ_128M - 1)) == ((addr + len - 1) | (SZ_128M - 1)))
> > Besides sizes.h being missed, this can be done with help of ALIGN()
> > macro (or alike). So, kill this and use the globally defined macro
> > inline.
> I will add sizes.h and directly apply globally defined  ALIGN() macro
> instead

Also check what header should be included for that macro, IIRC it's align.h.

...

> >> +               for (idx = 0; idx < ARRAY_SIZE(restore_data); idx++) {
> >> +                       if (restore_data[idx].width == 32)
> > sizeof(u32) ?
> Your idea is better, I will change it.

You might probably want to use the same in the restore_data array initialiser.

> >> +                               val[idx] = sdhci_readl(host, restore_data[idx].reg);
> >> +                       else if (restore_data[idx].width == 8)
> > sizeof(u8) ?
> I will fix it.
> >> +                               val[idx] = sdhci_readb(host, restore_data[idx].reg);
> >> +               }

...

> >> +               pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
> >> +               if (IS_ERR(pltfm_host->clk)) {
> >> +                       err = PTR_ERR(pltfm_host->clk);
> >> +                       dev_err(&pdev->dev, "failed to get clk: %d\n", err);
> > Use
> >
> >    return dev_err_probe(...);
> I will use dev_err_probe() instead of dev_err()
> >> +                       goto free_pltfm;
> > This is wrong. You may not call non-devm before devm ones, otherwise
> > it makes a room for subtle mistakes on remove-probe or unbind-bind
> > cycles. Have you tested that?
> I have tested it, there is no error messages during driver initial process.
>
> My thought is that sdhci_pltfm_init() and sdhci_pltfm_free() are used together.
>
> If there's any error after the successful execution of sdhci_pltfm_init(),
> I'll use sdhci_pltfm_free().
>
> I am not entirely sure if this answers your question.

Yes, they are, the problem is that freeing resources happens in
non-reversed order (for some of the resources). This might lead to
subtle mistakes as I said above. The rule of thumb is to avoid mixing
devm_*() with non-devm_*() calls. If you have both, they have to be
grouped as all devm_*() followed by all non-devm_*().
In some cases you might need to wrap existing calls to become managed.
This may be done with the help of devm_add_action_or_reset(). Check
other drivers which are using that.

> >> +               }
> >> +               err = clk_prepare_enable(pltfm_host->clk);
> >> +               if (err)
> >> +                       goto free_pltfm;
> > Use _enabled variant of devm_clk_get() instead.
> I will use devm_clk_get_optional_enabled() instead.
> >> +       }

...

> >> +free_pltfm:
> >> +       sdhci_pltfm_free(pdev);
> > This should go to be correct in ordering.
>
> I am not entirely sure if it is similar to the "goto free_pltfm;" issue.

Yes. It's part of the same issue.

> >> +       return err;
> >> +}
> >> +
> >> +static int ma35_remove(struct platform_device *pdev)
> > Use remove_new callback.
> I will fix it.
> >> +{
> >> +       struct sdhci_host *host = platform_get_drvdata(pdev);
> >> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >> +
> >> +       sdhci_remove_host(host, 0);
> >> +       clk_disable_unprepare(pltfm_host->clk);
> >> +       sdhci_pltfm_free(pdev);
> > At least these two will go away as per probe error path.
> I will use sdhci_pltfm_remove instead of  the ma35_remove.

After fixing the ordering issues in ->probe() this might need more
modifications.

> >> +       return 0;
> >> +}


--
With Best Regards,
Andy Shevchenko

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

[Philipp Zabel]

On Mi, 2024-06-19 at 13:46 +0800, Shan-Chun Hung wrote:
> This adds the SDHCI driver for the MA35 series SoC. It is based upon the
> SDHCI interface, but requires some extra initialization.
> 
> Signed-off-by: schung <schung@nuvoton.com>
> ---
>  drivers/mmc/host/Kconfig           |  13 ++
>  drivers/mmc/host/Makefile          |   1 +
>  drivers/mmc/host/sdhci-of-ma35d1.c | 297 +++++++++++++++++++++++++++++
>  3 files changed, 311 insertions(+)
>  create mode 100644 drivers/mmc/host/sdhci-of-ma35d1.c
> 
[...]
> diff --git a/drivers/mmc/host/sdhci-of-ma35d1.c b/drivers/mmc/host/sdhci-of-ma35d1.c
> new file mode 100644
> index 000000000000..7714a5ab463d
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-of-ma35d1.c
> @@ -0,0 +1,297 @@
[...]
> +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);
> +
> +	/*
> +	 * extra adma table cnt for cross 128M boundary handling.
> +	 */
> +	extra = DIV_ROUND_UP_ULL(dma_get_required_mask(&pdev->dev), SZ_128M);
> +	if (extra > SDHCI_MAX_SEGS)
> +		extra = SDHCI_MAX_SEGS;
> +	host->adma_table_cnt += extra;
> +	pltfm_host = sdhci_priv(host);
> +	priv = sdhci_pltfm_priv(pltfm_host);
> +
> +	if (dev->of_node) {
> +		pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
> +		if (IS_ERR(pltfm_host->clk)) {
> +			err = PTR_ERR(pltfm_host->clk);
> +			dev_err(&pdev->dev, "failed to get clk: %d\n", err);
> +			goto free_pltfm;
> +		}
> +		err = clk_prepare_enable(pltfm_host->clk);
> +		if (err)
> +			goto free_pltfm;
> +	}
> +
> +	err = mmc_of_parse(host->mmc);
> +	if (err)
> +		goto err_clk;
> +
> +	priv->rst = devm_reset_control_get(&pdev->dev, NULL);

Please use devm_reset_control_get_exclusive() instead.

regards
Philipp

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

[Shan-Chun Hung]

Dear Philipp,

Thanks for your review.

On 2024/6/21 下午 07:45, Philipp Zabel wrote:
> On Mi, 2024-06-19 at 13:46 +0800, Shan-Chun Hung wrote:
>> This adds the SDHCI driver for the MA35 series SoC. It is based upon the
>> SDHCI interface, but requires some extra initialization.
>>
>> Signed-off-by: schung<schung@nuvoton.com>
>> ---
>>   drivers/mmc/host/Kconfig           |  13 ++
>>   drivers/mmc/host/Makefile          |   1 +
>>   drivers/mmc/host/sdhci-of-ma35d1.c | 297 +++++++++++++++++++++++++++++
>>   3 files changed, 311 insertions(+)
>>   create mode 100644 drivers/mmc/host/sdhci-of-ma35d1.c
>>
> [...]
>> diff --git a/drivers/mmc/host/sdhci-of-ma35d1.c b/drivers/mmc/host/sdhci-of-ma35d1.c
>> new file mode 100644
>> index 000000000000..7714a5ab463d
>> --- /dev/null
>> +++ b/drivers/mmc/host/sdhci-of-ma35d1.c
>> @@ -0,0 +1,297 @@
> [...]
>> +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);
>> +
>> +	/*
>> +	 * extra adma table cnt for cross 128M boundary handling.
>> +	 */
>> +	extra = DIV_ROUND_UP_ULL(dma_get_required_mask(&pdev->dev), SZ_128M);
>> +	if (extra > SDHCI_MAX_SEGS)
>> +		extra = SDHCI_MAX_SEGS;
>> +	host->adma_table_cnt += extra;
>> +	pltfm_host = sdhci_priv(host);
>> +	priv = sdhci_pltfm_priv(pltfm_host);
>> +
>> +	if (dev->of_node) {
>> +		pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
>> +		if (IS_ERR(pltfm_host->clk)) {
>> +			err = PTR_ERR(pltfm_host->clk);
>> +			dev_err(&pdev->dev, "failed to get clk: %d\n", err);
>> +			goto free_pltfm;
>> +		}
>> +		err = clk_prepare_enable(pltfm_host->clk);
>> +		if (err)
>> +			goto free_pltfm;
>> +	}
>> +
>> +	err = mmc_of_parse(host->mmc);
>> +	if (err)
>> +		goto err_clk;
>> +
>> +	priv->rst = devm_reset_control_get(&pdev->dev, NULL);
> Please use devm_reset_control_get_exclusive() instead.
>
> regards
> Philipp
OK, I will fix it.

Best Regards

Shan-Chun

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

[Shan-Chun Hung]

Dear Andy,

Thanks for you review.

On 2024/6/21 下午 07:25, Andy Shevchenko wrote:
> On Fri, Jun 21, 2024 at 10:06 AM Shan-Chun Hung<shanchun1218@gmail.com>  wrote:
>> On 2024/6/20 上午 03:09, Andy Shevchenko wrote:
>>> On Wed, Jun 19, 2024 at 7:47 AM Shan-Chun Hung<shanchun1218@gmail.com>   wrote:
> ...
>
>>> You are missing a lot of header inclusions, please follow IWYU principle.
>> I am not familiar with IWYU yet, but I will learn it and use it for
>> checks later on.
> "Include What You Use". But some of the headers may be omitted as they
> are guaranteed to be included by others. It's a bit hard because one
> should know and follow the kernel development, currently the headers
> in the kernel are a bit of a mess.
Absolutely, kernel development needs careful attention to many details, 
like managing header file
> ...
>
>>>> +#define BOUNDARY_OK(addr, len) \
>>>> +       ((addr | (SZ_128M - 1)) == ((addr + len - 1) | (SZ_128M - 1)))
>>> Besides sizes.h being missed, this can be done with help of ALIGN()
>>> macro (or alike). So, kill this and use the globally defined macro
>>> inline.
>> I will add sizes.h and directly apply globally defined  ALIGN() macro
>> instead
> Also check what header should be included for that macro, IIRC it's align.h.
I will add add "#include <linux/align.h>"
> ...
>
>>>> +               for (idx = 0; idx < ARRAY_SIZE(restore_data); idx++) {
>>>> +                       if (restore_data[idx].width == 32)
>>> sizeof(u32) ?
>> Your idea is better, I will change it.
> You might probably want to use the same in the restore_data array initialiser.
I will modify it.
>>>> +                               val[idx] = sdhci_readl(host, restore_data[idx].reg);
>>>> +                       else if (restore_data[idx].width == 8)
>>> sizeof(u8) ?
>> I will fix it.
>>>> +                               val[idx] = sdhci_readb(host, restore_data[idx].reg);
>>>> +               }
> ...
>
>>>> +               pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
>>>> +               if (IS_ERR(pltfm_host->clk)) {
>>>> +                       err = PTR_ERR(pltfm_host->clk);
>>>> +                       dev_err(&pdev->dev, "failed to get clk: %d\n", err);
>>> Use
>>>
>>>     return dev_err_probe(...);
>> I will use dev_err_probe() instead of dev_err()
>>>> +                       goto free_pltfm;
>>> This is wrong. You may not call non-devm before devm ones, otherwise
>>> it makes a room for subtle mistakes on remove-probe or unbind-bind
>>> cycles. Have you tested that?
>> I have tested it, there is no error messages during driver initial process.
>>
>> My thought is that sdhci_pltfm_init() and sdhci_pltfm_free() are used together.
>>
>> If there's any error after the successful execution of sdhci_pltfm_init(),
>> I'll use sdhci_pltfm_free().
>>
>> I am not entirely sure if this answers your question.
> Yes, they are, the problem is that freeing resources happens in
> non-reversed order (for some of the resources). This might lead to
> subtle mistakes as I said above. The rule of thumb is to avoid mixing
> devm_*() with non-devm_*() calls. If you have both, they have to be
> grouped as all devm_*() followed by all non-devm_*().
> In some cases you might need to wrap existing calls to become managed.
> This may be done with the help of devm_add_action_or_reset(). Check
> other drivers which are using that.
I will add devm_add_action_or_reset() to do sdhci_pltfm_free().
>>>> +               }
>>>> +               err = clk_prepare_enable(pltfm_host->clk);
>>>> +               if (err)
>>>> +                       goto free_pltfm;
>>> Use _enabled variant of devm_clk_get() instead.
>> I will use devm_clk_get_optional_enabled() instead.
>>>> +       }
> ...
>
>>>> +free_pltfm:
>>>> +       sdhci_pltfm_free(pdev);
>>> This should go to be correct in ordering.
>> I am not entirely sure if it is similar to the "goto free_pltfm;" issue.
> Yes. It's part of the same issue.
>>>> +       return err;
>>>> +}
>>>> +
>>>> +static int ma35_remove(struct platform_device *pdev)
>>> Use remove_new callback.
>> I will fix it.
>>>> +{
>>>> +       struct sdhci_host *host = platform_get_drvdata(pdev);
>>>> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>> +
>>>> +       sdhci_remove_host(host, 0);
>>>> +       clk_disable_unprepare(pltfm_host->clk);
>>>> +       sdhci_pltfm_free(pdev);
>>> At least these two will go away as per probe error path.
>> I will use sdhci_pltfm_remove instead of  the ma35_remove.
> After fixing the ordering issues in ->probe() this might need more
> modifications.
Understood, I will correct these issues as soon as possible.
>>>> +       return 0;
>>>> +}
> --
> With Best Regards,
> Andy Shevchenko

Best Regards,

Shan-Chun