2022-11-30 15:24:01

by Tomer Maimon

[permalink] [raw]
Subject: [PATCH v1 0/2] MMC: add NPCM SDHCI driver support

This patch set adds SDHCI support for the Nuvoton NPCM Baseboard
Management Controller (BMC).

The NPCM SDHCI driver tested on NPCM750 and NPCM845 EVB.

Tomer Maimon (2):
dt-bindings: mmc: npcm,sdhci: Document NPCM SDHCI controller
mmc: sdhci-npcm: Add NPCM SDHCI driver

.../devicetree/bindings/mmc/npcm,sdhci.yaml | 47 +++++++++++
drivers/mmc/host/Kconfig | 8 ++
drivers/mmc/host/Makefile | 1 +
drivers/mmc/host/sdhci-npcm.c | 81 +++++++++++++++++++
4 files changed, 137 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mmc/npcm,sdhci.yaml
create mode 100644 drivers/mmc/host/sdhci-npcm.c

--
2.33.0


2022-11-30 15:48:48

by Tomer Maimon

[permalink] [raw]
Subject: [PATCH v1 2/2] mmc: sdhci-npcm: Add NPCM SDHCI driver

Add Nuvoton NPCM BMC sdhci-pltfm controller driver.

Signed-off-by: Tomer Maimon <[email protected]>
---
drivers/mmc/host/Kconfig | 8 ++++
drivers/mmc/host/Makefile | 1 +
drivers/mmc/host/sdhci-npcm.c | 81 +++++++++++++++++++++++++++++++++++
3 files changed, 90 insertions(+)
create mode 100644 drivers/mmc/host/sdhci-npcm.c

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index fb1062a6394c..4b2d9ce4308c 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -709,6 +709,14 @@ config MMC_TMIO
This provides support for the SD/MMC cell found in TC6393XB,
T7L66XB and also HTC ASIC3

+config MMC_SDHCI_NPCM
+ tristate "Secure Digital Host Controller Interface support for NPCM"
+ depends on ARCH_NPCM || COMPILE_TEST
+ depends on MMC_SDHCI_PLTFM
+ help
+ This provides support for the SD/eMMC controller found in
+ NPCM BMC family SoCs.
+
config MMC_SDHI
tristate "Renesas SDHI SD/SDIO controller support"
depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 4e4ceb32c4b4..801086613d7f 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -37,6 +37,7 @@ obj-$(CONFIG_MMC_SPI) += of_mmc_spi.o
obj-$(CONFIG_MMC_S3C) += s3cmci.o
obj-$(CONFIG_MMC_SDRICOH_CS) += sdricoh_cs.o
obj-$(CONFIG_MMC_TMIO) += tmio_mmc.o
+obj-$(CONFIG_MMC_SDHCI_NPCM) += sdhci-npcm.o
obj-$(CONFIG_MMC_TMIO_CORE) += tmio_mmc_core.o
obj-$(CONFIG_MMC_SDHI) += renesas_sdhi_core.o
obj-$(CONFIG_MMC_SDHI_SYS_DMAC) += renesas_sdhi_sys_dmac.o
diff --git a/drivers/mmc/host/sdhci-npcm.c b/drivers/mmc/host/sdhci-npcm.c
new file mode 100644
index 000000000000..298c5f3e7c2b
--- /dev/null
+++ b/drivers/mmc/host/sdhci-npcm.c
@@ -0,0 +1,81 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * NPCM SDHC MMC host controller driver.
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/mmc/host.h>
+#include <linux/mmc/mmc.h>
+#include <linux/module.h>
+#include <linux/of.h>
+
+#include "sdhci-pltfm.h"
+
+static const struct sdhci_pltfm_data npcm_sdhci_pdata = {
+ .quirks = SDHCI_QUIRK_DELAY_AFTER_POWER,
+ .quirks2 = SDHCI_QUIRK2_STOP_WITH_TC |
+ SDHCI_QUIRK2_NO_1_8_V,
+};
+
+static int npcm_sdhci_probe(struct platform_device *pdev)
+{
+ struct sdhci_pltfm_host *pltfm_host;
+ struct sdhci_host *host;
+ u32 caps;
+ int ret;
+
+ host = sdhci_pltfm_init(pdev, &npcm_sdhci_pdata, 0);
+ if (IS_ERR(host))
+ return PTR_ERR(host);
+
+ pltfm_host = sdhci_priv(host);
+ pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
+
+ if (!IS_ERR(pltfm_host->clk))
+ clk_prepare_enable(pltfm_host->clk);
+
+ caps = sdhci_readl(host, SDHCI_CAPABILITIES);
+ if (caps & SDHCI_CAN_DO_8BIT)
+ host->mmc->caps |= MMC_CAP_8_BIT_DATA;
+
+ ret = mmc_of_parse(host->mmc);
+ if (ret)
+ goto err_sdhci_add;
+
+ ret = sdhci_add_host(host);
+ if (ret)
+ goto err_sdhci_add;
+
+ return 0;
+
+err_sdhci_add:
+ clk_disable_unprepare(pltfm_host->clk);
+ sdhci_pltfm_free(pdev);
+ return ret;
+}
+
+static const struct of_device_id npcm_sdhci_of_match[] = {
+ { .compatible = "nuvoton,npcm750-sdhci" },
+ { .compatible = "nuvoton,npcm845-sdhci" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, npcm_sdhci_of_match);
+
+static struct platform_driver npcm_sdhci_driver = {
+ .driver = {
+ .name = "npcm-sdhci",
+ .of_match_table = npcm_sdhci_of_match,
+ .pm = &sdhci_pltfm_pmops,
+ },
+ .probe = npcm_sdhci_probe,
+ .remove = sdhci_pltfm_unregister,
+};
+
+module_platform_driver(npcm_sdhci_driver);
+
+MODULE_DESCRIPTION("NPCM Secure Digital Host Controller Interface driver");
+MODULE_AUTHOR("Tomer Maimon <[email protected]>");
+MODULE_LICENSE("GPL v2");
--
2.33.0

2022-11-30 15:53:50

by Tomer Maimon

[permalink] [raw]
Subject: [PATCH v1 1/2] dt-bindings: mmc: npcm,sdhci: Document NPCM SDHCI controller

Add binding for Nuvoton NPCM SDHCI controller.

Signed-off-by: Tomer Maimon <[email protected]>
---
.../devicetree/bindings/mmc/npcm,sdhci.yaml | 47 +++++++++++++++++++
1 file changed, 47 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mmc/npcm,sdhci.yaml

diff --git a/Documentation/devicetree/bindings/mmc/npcm,sdhci.yaml b/Documentation/devicetree/bindings/mmc/npcm,sdhci.yaml
new file mode 100644
index 000000000000..38409272807a
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/npcm,sdhci.yaml
@@ -0,0 +1,47 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mmc/npcm,sdhci.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NPCM SDHCI Controller
+
+maintainers:
+ - Tomer Maimon <[email protected]>
+
+properties:
+ compatible:
+ enum:
+ - nuvoton,npcm750-sdhci
+ - nuvoton,npcm845-sdhci
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ maxItems: 1
+
+patternProperties:
+ "^sdhci@[0-9a-f]+$":
+ type: object
+ $ref: mmc-controller.yaml
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ sdhci0: sdhci@f0840000 {
+ compatible = "nuvoton,npcm750-sdhci";
+ reg = <0xf0840000 0x200>;
+ interrupts = <0 27 4>;
+ clocks = <&clk 4>;
+ };
--
2.33.0

2022-11-30 16:57:05

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v1 2/2] mmc: sdhci-npcm: Add NPCM SDHCI driver

On Wed, Nov 30, 2022 at 5:09 PM Tomer Maimon <[email protected]> wrote:
>
> Add Nuvoton NPCM BMC sdhci-pltfm controller driver.

...

> obj-$(CONFIG_MMC_S3C) += s3cmci.o
> obj-$(CONFIG_MMC_SDRICOH_CS) += sdricoh_cs.o
> obj-$(CONFIG_MMC_TMIO) += tmio_mmc.o

> +obj-$(CONFIG_MMC_SDHCI_NPCM) += sdhci-npcm.o

Keep it ordered by module name.

> obj-$(CONFIG_MMC_TMIO_CORE) += tmio_mmc_core.o
> obj-$(CONFIG_MMC_SDHI) += renesas_sdhi_core.o
> obj-$(CONFIG_MMC_SDHI_SYS_DMAC) += renesas_sdhi_sys_dmac.o

...

> +/*
> + * NPCM SDHC MMC host controller driver.
> + *
> + */

Too many lines for seems to be oneliner comment.

...

> +#include <linux/of.h>

I don't see how it's being used.
But it seems the mod_devicetable.h is missing.

...

> +static const struct sdhci_pltfm_data npcm_sdhci_pdata = {
> + .quirks = SDHCI_QUIRK_DELAY_AFTER_POWER,
> + .quirks2 = SDHCI_QUIRK2_STOP_WITH_TC |
> + SDHCI_QUIRK2_NO_1_8_V,
> +};

Why? Can't you use the sdhci as a library?

...

> +static int npcm_sdhci_probe(struct platform_device *pdev)
> +{
> + struct sdhci_pltfm_host *pltfm_host;
> + struct sdhci_host *host;
> + u32 caps;
> + int ret;
> +
> + host = sdhci_pltfm_init(pdev, &npcm_sdhci_pdata, 0);
> + if (IS_ERR(host))
> + return PTR_ERR(host);
> +
> + pltfm_host = sdhci_priv(host);

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

Blank line in a wrong position, should be before devm_clk_get().

> + if (!IS_ERR(pltfm_host->clk))
> + clk_prepare_enable(pltfm_host->clk);

Why not use a specific helper that gets the clock enabled?

> + caps = sdhci_readl(host, SDHCI_CAPABILITIES);
> + if (caps & SDHCI_CAN_DO_8BIT)
> + host->mmc->caps |= MMC_CAP_8_BIT_DATA;
> +
> + ret = mmc_of_parse(host->mmc);
> + if (ret)
> + goto err_sdhci_add;
> +
> + ret = sdhci_add_host(host);
> + if (ret)
> + goto err_sdhci_add;
> +
> + return 0;
> +
> +err_sdhci_add:
> + clk_disable_unprepare(pltfm_host->clk);
> + sdhci_pltfm_free(pdev);
> + return ret;
> +}

...

> +

Redundant blank line.

> +module_platform_driver(npcm_sdhci_driver);

--
With Best Regards,
Andy Shevchenko

2022-11-30 19:04:01

by Tomer Maimon

[permalink] [raw]
Subject: Re: [PATCH v1 2/2] mmc: sdhci-npcm: Add NPCM SDHCI driver

Hi Andy,

Thanks for your comments.

On Wed, 30 Nov 2022 at 17:50, Andy Shevchenko <[email protected]> wrote:
>
> On Wed, Nov 30, 2022 at 5:09 PM Tomer Maimon <[email protected]> wrote:
> >
> > Add Nuvoton NPCM BMC sdhci-pltfm controller driver.
>
> ...
>
> > obj-$(CONFIG_MMC_S3C) += s3cmci.o
> > obj-$(CONFIG_MMC_SDRICOH_CS) += sdricoh_cs.o
> > obj-$(CONFIG_MMC_TMIO) += tmio_mmc.o
>
> > +obj-$(CONFIG_MMC_SDHCI_NPCM) += sdhci-npcm.o
>
> Keep it ordered by module name.
>
> > obj-$(CONFIG_MMC_TMIO_CORE) += tmio_mmc_core.o
> > obj-$(CONFIG_MMC_SDHI) += renesas_sdhi_core.o
> > obj-$(CONFIG_MMC_SDHI_SYS_DMAC) += renesas_sdhi_sys_dmac.o
>
> ...
>
> > +/*
> > + * NPCM SDHC MMC host controller driver.
> > + *
> > + */
>
> Too many lines for seems to be oneliner comment.
>
> ...
>
> > +#include <linux/of.h>
>
> I don't see how it's being used.
> But it seems the mod_devicetable.h is missing.
>
> ...
>
> > +static const struct sdhci_pltfm_data npcm_sdhci_pdata = {
> > + .quirks = SDHCI_QUIRK_DELAY_AFTER_POWER,
> > + .quirks2 = SDHCI_QUIRK2_STOP_WITH_TC |
> > + SDHCI_QUIRK2_NO_1_8_V,
> > +};
>
> Why? Can't you use the sdhci as a library?
Can you explain what you mean by using sdhci as a library?
is it not to use the pltfm_data structure and only to set the quirks
directly through the host?
> ...
>
> > +static int npcm_sdhci_probe(struct platform_device *pdev)
> > +{
> > + struct sdhci_pltfm_host *pltfm_host;
> > + struct sdhci_host *host;
> > + u32 caps;
> > + int ret;
> > +
> > + host = sdhci_pltfm_init(pdev, &npcm_sdhci_pdata, 0);
> > + if (IS_ERR(host))
> > + return PTR_ERR(host);
> > +
> > + pltfm_host = sdhci_priv(host);
>
> > + pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
> > +
>
> Blank line in a wrong position, should be before devm_clk_get().
>
> > + if (!IS_ERR(pltfm_host->clk))
> > + clk_prepare_enable(pltfm_host->clk);
>
> Why not use a specific helper that gets the clock enabled?
which specific helper? can you give me more specific details?
>
> > + caps = sdhci_readl(host, SDHCI_CAPABILITIES);
> > + if (caps & SDHCI_CAN_DO_8BIT)
> > + host->mmc->caps |= MMC_CAP_8_BIT_DATA;
> > +
> > + ret = mmc_of_parse(host->mmc);
> > + if (ret)
> > + goto err_sdhci_add;
> > +
> > + ret = sdhci_add_host(host);
> > + if (ret)
> > + goto err_sdhci_add;
> > +
> > + return 0;
> > +
> > +err_sdhci_add:
> > + clk_disable_unprepare(pltfm_host->clk);
> > + sdhci_pltfm_free(pdev);
> > + return ret;
> > +}
>
> ...
>
> > +
>
> Redundant blank line.
>
> > +module_platform_driver(npcm_sdhci_driver);
>
> --
> With Best Regards,
> Andy Shevchenko

Best regards,

Tomer

2022-12-01 16:44:11

by Adrian Hunter

[permalink] [raw]
Subject: Re: [PATCH v1 2/2] mmc: sdhci-npcm: Add NPCM SDHCI driver

On 30/11/22 17:08, Tomer Maimon wrote:
> Add Nuvoton NPCM BMC sdhci-pltfm controller driver.
>
> Signed-off-by: Tomer Maimon <[email protected]>
> ---
> drivers/mmc/host/Kconfig | 8 ++++
> drivers/mmc/host/Makefile | 1 +
> drivers/mmc/host/sdhci-npcm.c | 81 +++++++++++++++++++++++++++++++++++
> 3 files changed, 90 insertions(+)
> create mode 100644 drivers/mmc/host/sdhci-npcm.c
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index fb1062a6394c..4b2d9ce4308c 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -709,6 +709,14 @@ config MMC_TMIO
> This provides support for the SD/MMC cell found in TC6393XB,
> T7L66XB and also HTC ASIC3
>
> +config MMC_SDHCI_NPCM
> + tristate "Secure Digital Host Controller Interface support for NPCM"
> + depends on ARCH_NPCM || COMPILE_TEST
> + depends on MMC_SDHCI_PLTFM
> + help
> + This provides support for the SD/eMMC controller found in
> + NPCM BMC family SoCs.
> +
> config MMC_SDHI
> tristate "Renesas SDHI SD/SDIO controller support"
> depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> index 4e4ceb32c4b4..801086613d7f 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_MMC_SPI) += of_mmc_spi.o
> obj-$(CONFIG_MMC_S3C) += s3cmci.o
> obj-$(CONFIG_MMC_SDRICOH_CS) += sdricoh_cs.o
> obj-$(CONFIG_MMC_TMIO) += tmio_mmc.o
> +obj-$(CONFIG_MMC_SDHCI_NPCM) += sdhci-npcm.o
> obj-$(CONFIG_MMC_TMIO_CORE) += tmio_mmc_core.o
> obj-$(CONFIG_MMC_SDHI) += renesas_sdhi_core.o
> obj-$(CONFIG_MMC_SDHI_SYS_DMAC) += renesas_sdhi_sys_dmac.o
> diff --git a/drivers/mmc/host/sdhci-npcm.c b/drivers/mmc/host/sdhci-npcm.c
> new file mode 100644
> index 000000000000..298c5f3e7c2b
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-npcm.c
> @@ -0,0 +1,81 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * NPCM SDHC MMC host controller driver.
> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/mmc/host.h>
> +#include <linux/mmc/mmc.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +
> +#include "sdhci-pltfm.h"
> +
> +static const struct sdhci_pltfm_data npcm_sdhci_pdata = {
> + .quirks = SDHCI_QUIRK_DELAY_AFTER_POWER,
> + .quirks2 = SDHCI_QUIRK2_STOP_WITH_TC |
> + SDHCI_QUIRK2_NO_1_8_V,
> +};
> +
> +static int npcm_sdhci_probe(struct platform_device *pdev)
> +{
> + struct sdhci_pltfm_host *pltfm_host;
> + struct sdhci_host *host;
> + u32 caps;
> + int ret;
> +
> + host = sdhci_pltfm_init(pdev, &npcm_sdhci_pdata, 0);
> + if (IS_ERR(host))
> + return PTR_ERR(host);
> +
> + pltfm_host = sdhci_priv(host);
> + pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);

For an optional clock, something like:

pltfm_host->clk = devm_clk_get_optional(&pdev->dev, NULL);
if (IS_ERR(pltfm_host->clk))
return PTR_ERR(pltfm_host->clk);

will handle -EPROBE_DEFER

> +
> + if (!IS_ERR(pltfm_host->clk))
> + clk_prepare_enable(pltfm_host->clk);
> +
> + caps = sdhci_readl(host, SDHCI_CAPABILITIES);
> + if (caps & SDHCI_CAN_DO_8BIT)
> + host->mmc->caps |= MMC_CAP_8_BIT_DATA;
> +
> + ret = mmc_of_parse(host->mmc);
> + if (ret)
> + goto err_sdhci_add;
> +
> + ret = sdhci_add_host(host);
> + if (ret)
> + goto err_sdhci_add;
> +
> + return 0;
> +
> +err_sdhci_add:
> + clk_disable_unprepare(pltfm_host->clk);
> + sdhci_pltfm_free(pdev);
> + return ret;
> +}
> +
> +static const struct of_device_id npcm_sdhci_of_match[] = {
> + { .compatible = "nuvoton,npcm750-sdhci" },
> + { .compatible = "nuvoton,npcm845-sdhci" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, npcm_sdhci_of_match);
> +
> +static struct platform_driver npcm_sdhci_driver = {
> + .driver = {
> + .name = "npcm-sdhci",
> + .of_match_table = npcm_sdhci_of_match,
> + .pm = &sdhci_pltfm_pmops,
> + },
> + .probe = npcm_sdhci_probe,
> + .remove = sdhci_pltfm_unregister,
> +};
> +
> +module_platform_driver(npcm_sdhci_driver);
> +
> +MODULE_DESCRIPTION("NPCM Secure Digital Host Controller Interface driver");
> +MODULE_AUTHOR("Tomer Maimon <[email protected]>");
> +MODULE_LICENSE("GPL v2");

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


2022-12-02 00:05:14

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH v1 1/2] dt-bindings: mmc: npcm,sdhci: Document NPCM SDHCI controller

On Wed, Nov 30, 2022 at 05:08:56PM +0200, Tomer Maimon wrote:
> Add binding for Nuvoton NPCM SDHCI controller.
>
> Signed-off-by: Tomer Maimon <[email protected]>
> ---
> .../devicetree/bindings/mmc/npcm,sdhci.yaml | 47 +++++++++++++++++++
> 1 file changed, 47 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mmc/npcm,sdhci.yaml
>
> diff --git a/Documentation/devicetree/bindings/mmc/npcm,sdhci.yaml b/Documentation/devicetree/bindings/mmc/npcm,sdhci.yaml
> new file mode 100644
> index 000000000000..38409272807a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/npcm,sdhci.yaml
> @@ -0,0 +1,47 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mmc/npcm,sdhci.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NPCM SDHCI Controller
> +
> +maintainers:
> + - Tomer Maimon <[email protected]>
> +
> +properties:
> + compatible:
> + enum:
> + - nuvoton,npcm750-sdhci
> + - nuvoton,npcm845-sdhci
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + clocks:
> + maxItems: 1
> +
> +patternProperties:
> + "^sdhci@[0-9a-f]+$":
> + type: object

sdhci is a child node of the nuvoton,npcm750-sdhci node?

> + $ref: mmc-controller.yaml

I think you want:

allOf:
- $ref: mmc-controller.yaml#

And then you will have some errors in the example to fix.

> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - clocks
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + sdhci0: sdhci@f0840000 {

Drop unused labels.

Node name should be 'mmc'

> + compatible = "nuvoton,npcm750-sdhci";

Indent by 4 spaces.

> + reg = <0xf0840000 0x200>;
> + interrupts = <0 27 4>;
> + clocks = <&clk 4>;
> + };
> --
> 2.33.0
>
>

2022-12-04 11:48:20

by Tomer Maimon

[permalink] [raw]
Subject: Re: [PATCH v1 1/2] dt-bindings: mmc: npcm,sdhci: Document NPCM SDHCI controller

Hi Rob,

Thanks for your comments.

Your comments will be addressed next version.

On Fri, 2 Dec 2022 at 01:49, Rob Herring <[email protected]> wrote:
>
> On Wed, Nov 30, 2022 at 05:08:56PM +0200, Tomer Maimon wrote:
> > Add binding for Nuvoton NPCM SDHCI controller.
> >
> > Signed-off-by: Tomer Maimon <[email protected]>
> > ---
> > .../devicetree/bindings/mmc/npcm,sdhci.yaml | 47 +++++++++++++++++++
> > 1 file changed, 47 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/mmc/npcm,sdhci.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/mmc/npcm,sdhci.yaml b/Documentation/devicetree/bindings/mmc/npcm,sdhci.yaml
> > new file mode 100644
> > index 000000000000..38409272807a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mmc/npcm,sdhci.yaml
> > @@ -0,0 +1,47 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mmc/npcm,sdhci.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: NPCM SDHCI Controller
> > +
> > +maintainers:
> > + - Tomer Maimon <[email protected]>
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - nuvoton,npcm750-sdhci
> > + - nuvoton,npcm845-sdhci
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + interrupts:
> > + maxItems: 1
> > +
> > + clocks:
> > + maxItems: 1
> > +
> > +patternProperties:
> > + "^sdhci@[0-9a-f]+$":
> > + type: object
>
> sdhci is a child node of the nuvoton,npcm750-sdhci node?
No, will be removed
>
> > + $ref: mmc-controller.yaml
>
> I think you want:
>
> allOf:
> - $ref: mmc-controller.yaml#
>
> And then you will have some errors in the example to fix.
>
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - interrupts
> > + - clocks
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > + - |
> > + sdhci0: sdhci@f0840000 {
>
> Drop unused labels.
>
> Node name should be 'mmc'
>
> > + compatible = "nuvoton,npcm750-sdhci";
>
> Indent by 4 spaces.
>
> > + reg = <0xf0840000 0x200>;
> > + interrupts = <0 27 4>;
> > + clocks = <&clk 4>;
> > + };
> > --
> > 2.33.0
> >
> >

Best regards,

Tomer

2022-12-04 12:19:31

by Tomer Maimon

[permalink] [raw]
Subject: Re: [PATCH v1 2/2] mmc: sdhci-npcm: Add NPCM SDHCI driver

Hi Adrian,

Thanks for your comments.

Your comments will be addressed next version.

On Thu, 1 Dec 2022 at 18:28, Adrian Hunter <[email protected]> wrote:
>
> On 30/11/22 17:08, Tomer Maimon wrote:
> > Add Nuvoton NPCM BMC sdhci-pltfm controller driver.
> >
> > Signed-off-by: Tomer Maimon <[email protected]>
> > ---
> > drivers/mmc/host/Kconfig | 8 ++++
> > drivers/mmc/host/Makefile | 1 +
> > drivers/mmc/host/sdhci-npcm.c | 81 +++++++++++++++++++++++++++++++++++
> > 3 files changed, 90 insertions(+)
> > create mode 100644 drivers/mmc/host/sdhci-npcm.c
> >
> > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> > index fb1062a6394c..4b2d9ce4308c 100644
> > --- a/drivers/mmc/host/Kconfig
> > +++ b/drivers/mmc/host/Kconfig
> > @@ -709,6 +709,14 @@ config MMC_TMIO
> > This provides support for the SD/MMC cell found in TC6393XB,
> > T7L66XB and also HTC ASIC3
> >
> > +config MMC_SDHCI_NPCM
> > + tristate "Secure Digital Host Controller Interface support for NPCM"
> > + depends on ARCH_NPCM || COMPILE_TEST
> > + depends on MMC_SDHCI_PLTFM
> > + help
> > + This provides support for the SD/eMMC controller found in
> > + NPCM BMC family SoCs.
> > +
> > config MMC_SDHI
> > tristate "Renesas SDHI SD/SDIO controller support"
> > depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
> > diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> > index 4e4ceb32c4b4..801086613d7f 100644
> > --- a/drivers/mmc/host/Makefile
> > +++ b/drivers/mmc/host/Makefile
> > @@ -37,6 +37,7 @@ obj-$(CONFIG_MMC_SPI) += of_mmc_spi.o
> > obj-$(CONFIG_MMC_S3C) += s3cmci.o
> > obj-$(CONFIG_MMC_SDRICOH_CS) += sdricoh_cs.o
> > obj-$(CONFIG_MMC_TMIO) += tmio_mmc.o
> > +obj-$(CONFIG_MMC_SDHCI_NPCM) += sdhci-npcm.o
> > obj-$(CONFIG_MMC_TMIO_CORE) += tmio_mmc_core.o
> > obj-$(CONFIG_MMC_SDHI) += renesas_sdhi_core.o
> > obj-$(CONFIG_MMC_SDHI_SYS_DMAC) += renesas_sdhi_sys_dmac.o
> > diff --git a/drivers/mmc/host/sdhci-npcm.c b/drivers/mmc/host/sdhci-npcm.c
> > new file mode 100644
> > index 000000000000..298c5f3e7c2b
> > --- /dev/null
> > +++ b/drivers/mmc/host/sdhci-npcm.c
> > @@ -0,0 +1,81 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * NPCM SDHC MMC host controller driver.
> > + *
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/mmc/host.h>
> > +#include <linux/mmc/mmc.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +
> > +#include "sdhci-pltfm.h"
> > +
> > +static const struct sdhci_pltfm_data npcm_sdhci_pdata = {
> > + .quirks = SDHCI_QUIRK_DELAY_AFTER_POWER,
> > + .quirks2 = SDHCI_QUIRK2_STOP_WITH_TC |
> > + SDHCI_QUIRK2_NO_1_8_V,
> > +};
> > +
> > +static int npcm_sdhci_probe(struct platform_device *pdev)
> > +{
> > + struct sdhci_pltfm_host *pltfm_host;
> > + struct sdhci_host *host;
> > + u32 caps;
> > + int ret;
> > +
> > + host = sdhci_pltfm_init(pdev, &npcm_sdhci_pdata, 0);
> > + if (IS_ERR(host))
> > + return PTR_ERR(host);
> > +
> > + pltfm_host = sdhci_priv(host);
> > + pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
>
> For an optional clock, something like:
>
> pltfm_host->clk = devm_clk_get_optional(&pdev->dev, NULL);
> if (IS_ERR(pltfm_host->clk))
> return PTR_ERR(pltfm_host->clk);
>
> will handle -EPROBE_DEFER
>
> > +
> > + if (!IS_ERR(pltfm_host->clk))
> > + clk_prepare_enable(pltfm_host->clk);
> > +
> > + caps = sdhci_readl(host, SDHCI_CAPABILITIES);
> > + if (caps & SDHCI_CAN_DO_8BIT)
> > + host->mmc->caps |= MMC_CAP_8_BIT_DATA;
> > +
> > + ret = mmc_of_parse(host->mmc);
> > + if (ret)
> > + goto err_sdhci_add;
> > +
> > + ret = sdhci_add_host(host);
> > + if (ret)
> > + goto err_sdhci_add;
> > +
> > + return 0;
> > +
> > +err_sdhci_add:
> > + clk_disable_unprepare(pltfm_host->clk);
> > + sdhci_pltfm_free(pdev);
> > + return ret;
> > +}
> > +
> > +static const struct of_device_id npcm_sdhci_of_match[] = {
> > + { .compatible = "nuvoton,npcm750-sdhci" },
> > + { .compatible = "nuvoton,npcm845-sdhci" },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, npcm_sdhci_of_match);
> > +
> > +static struct platform_driver npcm_sdhci_driver = {
> > + .driver = {
> > + .name = "npcm-sdhci",
> > + .of_match_table = npcm_sdhci_of_match,
> > + .pm = &sdhci_pltfm_pmops,
> > + },
> > + .probe = npcm_sdhci_probe,
> > + .remove = sdhci_pltfm_unregister,
> > +};
> > +
> > +module_platform_driver(npcm_sdhci_driver);
> > +
> > +MODULE_DESCRIPTION("NPCM Secure Digital Host Controller Interface driver");
> > +MODULE_AUTHOR("Tomer Maimon <[email protected]>");
> > +MODULE_LICENSE("GPL v2");
>
> WARNING: Prefer "GPL" over "GPL v2" - see commit bf7fbeeae6db ("module: Cure the MODULE_LICENSE "GPL" vs. "GPL v2" bogosity")
> #133: FILE: drivers/mmc/host/sdhci-npcm.c:81:
> +MODULE_LICENSE("GPL v2");
>
>

Best regards,

Tomer