MMC: add NPCM SDHCI driver support

Add binding for Nuvoton NPCM SDHCI controller.

Signed-off-by Tomer Maimon <[email protected]>

---
 .../devicetree/bindings/mmc/npcm,sdhci.yaml   | 45 +++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mmc/npcm,sdhci.yaml

Add Nuvoton NPCM BMC sdhci-pltfm controller driver.

+config MMC_SDHCI_NPCM

config MMC_SDHCI_IPROC

drivers/mmc/host/Makefile +0 -0

 obj-$(CONFIG_MMC_CQHCI)                        += cqhci.o

drivers/mmc/host/sdhci-npcm.c new file +4 -0

+static int npcm_sdhci_probe(struct platform_device *pdev)
+{
+       struct sdhci_pltfm_host *pltfm_host;
+       struct sdhci_host *host;

drivers/mmc/host/sdhci-npcm.c new file +8 -0

+       if (IS_ERR(pltfm_host->clk))
+               return PTR_ERR(pltfm_host->clk);
+
+       ret = clk_prepare_enable(pltfm_host->clk);
+       if (ret)
+               return ret;
+
+       caps = sdhci_readl(host, SDHCI_CAPABILITIES);

Quoted 2 lines · Tomer Maimon

Add Nuvoton NPCM BMC sdhci-pltfm controller driver.

Thank you for an update, my comments below.

...

Quoted 3 lines

+config MMC_SDHCI_NPCM

config MMC_SDHCI_IPROC

Perhaps after IPROC?

...

drivers/mmc/host/Makefile obj-$(CONFIG_MMC_SDHCI_MICROCHIP_PIC32) += sdhci-pic32.o · line 100

Quoted 5 lines

drivers/mmc/host/Makefile +1 -0

@@ -97,6 +97,7 @@ obj-$(CONFIG_MMC_SDHCI_MICROCHIP_PIC32)       += sdhci-pic32.o
 obj-$(CONFIG_MMC_SDHCI_BRCMSTB)                += sdhci-brcmstb.o
 obj-$(CONFIG_MMC_SDHCI_OMAP)           += sdhci-omap.o
 obj-$(CONFIG_MMC_SDHCI_SPRD)           += sdhci-sprd.o
+obj-$(CONFIG_MMC_SDHCI_NPCM)           += sdhci-npcm.o

Perhaps after IPROC? (There is a group of platform drivers slightly below than here)

Quoted 2 lines

drivers/mmc/host/Makefile +0 -0

 obj-$(CONFIG_MMC_CQHCI)                        += cqhci.o

...

drivers/mmc/host/sdhci-npcm.c line 13

Quoted 6 lines

drivers/mmc/host/sdhci-npcm.c new file +6 -0

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

I guess platform_device.h is missing here.

...

Quoted 12 lines

drivers/mmc/host/sdhci-npcm.c new file +12 -0

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

Quoted 1 line

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

You can't mix devm with non-devm in this way.

Quoted 13 lines

drivers/mmc/host/sdhci-npcm.c new file +12 -0

+       if (IS_ERR(pltfm_host->clk))
+               return PTR_ERR(pltfm_host->clk);
+
+       ret = clk_prepare_enable(pltfm_host->clk);
+       if (ret)
+               return ret;
+
+       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);

drivers/mmc/host/sdhci-npcm.c line 54

Quoted 6 lines

drivers/mmc/host/sdhci-npcm.c new file +6 -0

+       if (ret)
+               goto err_sdhci_add;
+
+       ret = sdhci_add_host(host);
+       if (ret)
+               goto err_sdhci_add;

Why can't you use sdhci_pltfm_register()?

drivers/mmc/host/sdhci-npcm.c line 62

Quoted 8 lines

drivers/mmc/host/sdhci-npcm.c new file +7 -0

+       return 0;
+
+err_sdhci_add:
+       clk_disable_unprepare(pltfm_host->clk);
+       sdhci_pltfm_free(pdev);
+       return ret;
+}

Missing ->remove() due to above.

Have you tried to compile as a module and then remove and insert it several times?

--
With Best Regards,
Andy Shevchenko

Quoted 1 line · Andy Shevchenko

On Mon, Dec 5, 2022 at 10:54 AM Tomer Maimon <[email protected]> wrote:

Quoted 8 lines

drivers/mmc/host/sdhci-npcm.c line 13

Quoted 6 lines

drivers/mmc/host/sdhci-npcm.c new file +6 -0

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

I guess platform_device.h is missing here.

Build and work without platform_device.h, do I need it for module use?

Quoted 13 lines

Quoted 12 lines

drivers/mmc/host/sdhci-npcm.c new file +12 -0

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

Quoted 3 lines

Quoted 1 line

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

You can't mix devm with non-devm in this way.

Can you explain what you mean You can't mix devm with non-devm in this way, where is the mix?
In version 1 used devm_clk_get, is it problematic?

Quoted 12 lines

Quoted 12 lines

drivers/mmc/host/sdhci-npcm.c new file +12 -0

+       if (IS_ERR(pltfm_host->clk))
+               return PTR_ERR(pltfm_host->clk);
+
+       ret = clk_prepare_enable(pltfm_host->clk);
+       if (ret)
+               return ret;
+
+       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);

Quoted 8 lines

drivers/mmc/host/sdhci-npcm.c line 54

Quoted 6 lines

drivers/mmc/host/sdhci-npcm.c new file +6 -0

+       if (ret)
+               goto err_sdhci_add;
+
+       ret = sdhci_add_host(host);
+       if (ret)
+               goto err_sdhci_add;

Why can't you use sdhci_pltfm_register()?

two things are missing in sdhci_pltfm_register
1. clock.

2. Adding SDHCI_CAN_DO_8BIT capability according the eMMC capabilities.

Quoted 7 lines

Quoted 7 lines

drivers/mmc/host/sdhci-npcm.c new file +7 -0

+       return 0;
+
+err_sdhci_add:
+       clk_disable_unprepare(pltfm_host->clk);
+       sdhci_pltfm_free(pdev);
+       return ret;
+}

Quoted 2 lines · Tomer Maimon

Quoted 1 line · Andy Shevchenko

On Mon, Dec 5, 2022 at 10:54 AM Tomer Maimon <[email protected]> wrote:

...

Quoted 9 lines

Quoted 8 lines

drivers/mmc/host/sdhci-npcm.c line 13

Quoted 6 lines

drivers/mmc/host/sdhci-npcm.c new file +6 -0

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

I guess platform_device.h is missing here.

Build and work without platform_device.h, do I need it for module use?

The rule of thumb is to include headers we are the direct user of. I believe you have a data type and API that are defined in that header.

...

Quoted 15 lines

Quoted 15 lines

Quoted 14 lines

drivers/mmc/host/sdhci-npcm.c new file +13 -0

+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_optional(&pdev->dev, NULL);

Quoted 4 lines

Quoted 1 line

You can't mix devm with non-devm in this way.

Can you explain what you mean You can't mix devm with non-devm in this way, where is the mix?
In version 1 used devm_clk_get, is it problematic?

devm_ is problematic in your case.
TL;DR: you need to use clk_get_optional() and clk_put().

Your ->remove() callback doesn't free resources in the reversed order which may or, by luck, may not be the case of all possible crashes, UAFs, races, etc during removal stage. All the same for error path in ->probe().

Quoted 20 lines

Quoted 12 lines

Quoted 12 lines

drivers/mmc/host/sdhci-npcm.c new file +12 -0

+       if (IS_ERR(pltfm_host->clk))
+               return PTR_ERR(pltfm_host->clk);
+
+       ret = clk_prepare_enable(pltfm_host->clk);
+       if (ret)
+               return ret;
+
+       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);

Quoted 8 lines

drivers/mmc/host/sdhci-npcm.c line 54

Quoted 6 lines

drivers/mmc/host/sdhci-npcm.c new file +6 -0

+       if (ret)
+               goto err_sdhci_add;
+
+       ret = sdhci_add_host(host);
+       if (ret)
+               goto err_sdhci_add;

Why can't you use sdhci_pltfm_register()?

Quoted 2 lines

two things are missing in sdhci_pltfm_register
1. clock.

Taking into account the implementation of the corresponding _unregister() I would add the clock handling to the _register() one. Perhaps via a new member of the platform data that supplies the name and index of the clock and hence all clk_get_optional() / clk_put will be moved there.

Quoted 1 line

2. Adding SDHCI_CAN_DO_8BIT capability according the eMMC capabilities.

All the same, why can't platform data be utilised for this?

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

drivers/mmc/host/sdhci-npcm.c new file +7 -0

+       return 0;
+
+err_sdhci_add:
+       clk_disable_unprepare(pltfm_host->clk);
+       sdhci_pltfm_free(pdev);
+       return ret;
+}

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Tomer Maimon

Quoted 1 line · Andy Shevchenko

On Mon, Dec 5, 2022 at 10:54 AM Tomer Maimon <[email protected]> wrote:

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

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

You can't mix devm with non-devm in this way.

Can you explain what you mean You can't mix devm with non-devm in this way, where is the mix?
In version 1 used devm_clk_get, is it problematic?

devm_ is problematic in your case.
TL;DR: you need to use clk_get_optional() and clk_put().

devm_ calls exactly those, so what is the issue?

Quoted 4 lines

Your ->remove() callback doesn't free resources in the reversed order which may or, by luck, may not be the case of all possible crashes, UAFs, races, etc during removal stage. All the same for error path in ->probe().

Quoted 40 lines

Quoted 20 lines

Quoted 12 lines

Quoted 12 lines

drivers/mmc/host/sdhci-npcm.c new file +12 -0

+       if (IS_ERR(pltfm_host->clk))
+               return PTR_ERR(pltfm_host->clk);
+
+       ret = clk_prepare_enable(pltfm_host->clk);
+       if (ret)
+               return ret;
+
+       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);

Quoted 8 lines

drivers/mmc/host/sdhci-npcm.c line 54

Quoted 6 lines

drivers/mmc/host/sdhci-npcm.c new file +6 -0

+       if (ret)
+               goto err_sdhci_add;
+
+       ret = sdhci_add_host(host);
+       if (ret)
+               goto err_sdhci_add;

Why can't you use sdhci_pltfm_register()?

Quoted 2 lines

two things are missing in sdhci_pltfm_register
1. clock.

Taking into account the implementation of the corresponding _unregister() I would add the clock handling to the _register() one. Perhaps via a new member of the platform data that supplies the name and index of the clock and hence all clk_get_optional() / clk_put will be moved there.

Quoted 1 line

2. Adding SDHCI_CAN_DO_8BIT capability according the eMMC capabilities.

All the same, why can't platform data be utilised for this?

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

drivers/mmc/host/sdhci-npcm.c new file +7 -0

+       return 0;
+
+err_sdhci_add:
+       clk_disable_unprepare(pltfm_host->clk);
+       sdhci_pltfm_free(pdev);
+       return ret;
+}

Quoted 4 lines · Adrian Hunter

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Tomer Maimon

Quoted 1 line · Andy Shevchenko

On Mon, Dec 5, 2022 at 10:54 AM Tomer Maimon <[email protected]> wrote:

...

Quoted 11 lines

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

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

You can't mix devm with non-devm in this way.

Can you explain what you mean You can't mix devm with non-devm in this way, where is the mix?
In version 1 used devm_clk_get, is it problematic?

devm_ is problematic in your case.
TL;DR: you need to use clk_get_optional() and clk_put().

devm_ calls exactly those, so what is the issue?

The issue is the error path or removal stage where it may or may be not problematic. To be on the safe side, the best approach is to make sure that allocated resources are being deallocated in the reversed order. That said, the

1. call non-devm_func()
2. call devm_func()

is wrong strictly speaking.

Quoted 4 lines

Quoted 4 lines

Your ->remove() callback doesn't free resources in the reversed order which may or, by luck, may not be the case of all possible crashes, UAFs, races, etc during removal stage. All the same for error path in ->probe().

I also pointed out above what would be the outcome of neglecting this rule.

Quoted 33 lines

Quoted 33 lines

Quoted 20 lines

Quoted 12 lines

Quoted 12 lines

drivers/mmc/host/sdhci-npcm.c new file +12 -0

+       if (IS_ERR(pltfm_host->clk))
+               return PTR_ERR(pltfm_host->clk);
+
+       ret = clk_prepare_enable(pltfm_host->clk);
+       if (ret)
+               return ret;
+
+       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);

Quoted 8 lines

drivers/mmc/host/sdhci-npcm.c line 54

Quoted 6 lines

drivers/mmc/host/sdhci-npcm.c new file +6 -0

+       if (ret)
+               goto err_sdhci_add;
+
+       ret = sdhci_add_host(host);
+       if (ret)
+               goto err_sdhci_add;

Why can't you use sdhci_pltfm_register()?

Quoted 2 lines

two things are missing in sdhci_pltfm_register
1. clock.

Taking into account the implementation of the corresponding _unregister() I would add the clock handling to the _register() one. Perhaps via a new member of the platform data that supplies the name and index of the clock and hence all clk_get_optional() / clk_put will be moved there.

Quoted 1 line

2. Adding SDHCI_CAN_DO_8BIT capability according the eMMC capabilities.

All the same, why can't platform data be utilised for this?

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

drivers/mmc/host/sdhci-npcm.c line 62

Quoted 7 lines

drivers/mmc/host/sdhci-npcm.c new file +7 -0

+       return 0;
+
+err_sdhci_add:
+       clk_disable_unprepare(pltfm_host->clk);
+       sdhci_pltfm_free(pdev);
+       return ret;
+}

--
With Best Regards,
Andy Shevchenko

Quoted 29 lines · Andy Shevchenko

Quoted 4 lines · Adrian Hunter

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Tomer Maimon

Quoted 1 line · Andy Shevchenko

On Mon, Dec 5, 2022 at 10:54 AM Tomer Maimon <[email protected]> wrote:

...

Quoted 11 lines

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

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

You can't mix devm with non-devm in this way.

Can you explain what you mean You can't mix devm with non-devm in this way, where is the mix?
In version 1 used devm_clk_get, is it problematic?

devm_ is problematic in your case.
TL;DR: you need to use clk_get_optional() and clk_put().

devm_ calls exactly those, so what is the issue?

The issue is the error path or removal stage where it may or may be not problematic. To be on the safe side, the best approach is to make sure that allocated resources are being deallocated in the reversed order. That said, the

1. call non-devm_func()
2. call devm_func()

Quoted 1 line

is wrong strictly speaking.

To elaborate more, the

1. call all devm_func()
2. call only non-devm_func()

is the correct order.

Hence in this case the driver can be worked around easily (by shuffling the order in ->probe() to call devm_ first), but as I said looking into implementation of the _unregister() I'm pretty sure that clock management should be in sdhci-pltfm, rather than in all callers who won't need the full customization.

Hope this helps to understand my point.

Quoted 52 lines

Quoted 4 lines

Quoted 4 lines

Your ->remove() callback doesn't free resources in the reversed order which may or, by luck, may not be the case of all possible crashes, UAFs, races, etc during removal stage. All the same for error path in ->probe().

I also pointed out above what would be the outcome of neglecting this rule.

Quoted 33 lines

Quoted 33 lines

Quoted 20 lines

Quoted 12 lines

Quoted 12 lines

(quoted diff) +12 -0

+       if (IS_ERR(pltfm_host->clk))
+               return PTR_ERR(pltfm_host->clk);
+
+       ret = clk_prepare_enable(pltfm_host->clk);
+       if (ret)
+               return ret;
+
+       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);

Quoted 8 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+       if (ret)
+               goto err_sdhci_add;
+
+       ret = sdhci_add_host(host);
+       if (ret)
+               goto err_sdhci_add;

Why can't you use sdhci_pltfm_register()?

Quoted 2 lines

two things are missing in sdhci_pltfm_register
1. clock.

Taking into account the implementation of the corresponding _unregister() I would add the clock handling to the _register() one. Perhaps via a new member of the platform data that supplies the name and index of the clock and hence all clk_get_optional() / clk_put will be moved there.

Quoted 1 line

2. Adding SDHCI_CAN_DO_8BIT capability according the eMMC capabilities.

All the same, why can't platform data be utilised for this?

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Review

Quoted 7 lines

(quoted diff) +7 -0

+       return 0;
+
+err_sdhci_add:
+       clk_disable_unprepare(pltfm_host->clk);
+       sdhci_pltfm_free(pdev);
+       return ret;
+}

--
With Best Regards,
Andy Shevchenko

Quoted 35 lines · Andy Shevchenko

Quoted 26 lines · Andy Shevchenko

Quoted 4 lines · Adrian Hunter

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Tomer Maimon

Quoted 1 line · Andy Shevchenko

On Mon, Dec 5, 2022 at 10:54 AM Tomer Maimon <[email protected]> wrote:

...

Quoted 11 lines

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

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

You can't mix devm with non-devm in this way.

Can you explain what you mean You can't mix devm with non-devm in this way, where is the mix?
In version 1 used devm_clk_get, is it problematic?

devm_ is problematic in your case.
TL;DR: you need to use clk_get_optional() and clk_put().

devm_ calls exactly those, so what is the issue?

The issue is the error path or removal stage where it may or may be not problematic. To be on the safe side, the best approach is to make sure that allocated resources are being deallocated in the reversed order. That said, the

Quoted 4 lines

1. call non-devm_func()
2. call devm_func()

is wrong strictly speaking.

To elaborate more, the

Quoted 4 lines

1. call all devm_func()
2. call only non-devm_func()

is the correct order.

1. WRT pltfm_host->clk, that is what is happening
2. WRT other resources that is simply not always possible because not every resource is wrapped by devm_ e.g. mmc_alloc_host() / mmc_free_host()

Quoted 37 lines

Hence in this case the driver can be worked around easily (by shuffling the order in ->probe() to call devm_ first), but as I said looking into implementation of the _unregister() I'm pretty sure that clock management should be in sdhci-pltfm, rather than in all callers who won't need the full customization.

Hope this helps to understand my point.

Quoted 27 lines

Quoted 4 lines

Quoted 4 lines

Your ->remove() callback doesn't free resources in the reversed order which may or, by luck, may not be the case of all possible crashes, UAFs, races, etc during removal stage. All the same for error path in ->probe().

I also pointed out above what would be the outcome of neglecting this rule.

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

Quoted 8 lines

(quoted diff) +8 -0

+       if (IS_ERR(pltfm_host->clk))
+               return PTR_ERR(pltfm_host->clk);
+
+       ret = clk_prepare_enable(pltfm_host->clk);
+       if (ret)
+               return ret;
+
+       caps = sdhci_readl(host, SDHCI_CAPABILITIES);

Review

Quoted 10 lines

(quoted diff) +10 -0

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

Why can't you use sdhci_pltfm_register()?

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

two things are missing in sdhci_pltfm_register
1. clock.

Taking into account the implementation of the corresponding _unregister() I would add the clock handling to the _register() one. Perhaps via a new member of the platform data that supplies the name and index of the clock and hence all clk_get_optional() / clk_put will be moved there.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

2. Adding SDHCI_CAN_DO_8BIT capability according the eMMC capabilities.

All the same, why can't platform data be utilised for this?

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

(quoted diff) +4 -0

+       return 0;
+
+err_sdhci_add:
+       clk_disable_unprepare(pltfm_host->clk);

Quoted 11 lines

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+       sdhci_pltfm_free(pdev);
+       return ret;
+}

--
With Best Regards,
Andy Shevchenko

Quoted 6 lines · Adrian Hunter

Quoted 5 lines · Andy Shevchenko

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Adrian Hunter

Quoted 1 line · Andy Shevchenko

On Mon, Dec 5, 2022 at 1:20 PM Tomer Maimon <[email protected]> wrote:

Quoted 20 lines

Quoted 20 lines

Quoted 10 lines

Quoted 4 lines

Quoted 2 lines

devm_ is problematic in your case.
TL;DR: you need to use clk_get_optional() and clk_put().

devm_ calls exactly those, so what is the issue?

The issue is the error path or removal stage where it may or may be not problematic. To be on the safe side, the best approach is to make sure that allocated resources are being deallocated in the reversed order. That said, the

Quoted 4 lines

1. call non-devm_func()
2. call devm_func()

is wrong strictly speaking.

To elaborate more, the

1. call all devm_func()
2. call only non-devm_func()

Quoted 5 lines

Quoted 1 line

is the correct order.

1. WRT pltfm_host->clk, that is what is happening
2. WRT other resources that is simply not always possible because not every resource is wrapped by devm_ e.g. mmc_alloc_host() / mmc_free_host()

I little confused about what to decide, should I use only non-devm_func because mmc_alloc_host() / mmc_free_host() is not warrped with devm_?

Quoted 14 lines

Quoted 14 lines

Hence in this case the driver can be worked around easily (by shuffling the order in ->probe() to call devm_ first), but as I said looking into implementation of the _unregister() I'm pretty sure that clock management should be in sdhci-pltfm, rather than in all callers who won't need the full customization.

Hope this helps to understand my point.

Quoted 6 lines

Quoted 4 lines

Quoted 4 lines

Your ->remove() callback doesn't free resources in the reversed order which may or, by luck, may not be the case of all possible crashes, UAFs, races, etc during removal stage. All the same for error path in ->probe().

I also pointed out above what would be the outcome of neglecting this rule.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

Quoted 1 line

Why can't you use sdhci_pltfm_register()?

two things are missing in sdhci_pltfm_register
1. clock.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Taking into account the implementation of the corresponding _unregister() I would add the clock handling to the _register() one. Perhaps via a new member of the platform data that supplies the name and index of the clock and hence all clk_get_optional() / clk_put will be moved there.

Do you mean to add it to sdhci_pltfm_register function? if yes I believe it will take some time to modify sdhci_pltfm_register I prefer not to use sdhci_pltfm_register.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

2. Adding SDHCI_CAN_DO_8BIT capability according the eMMC capabilities.

All the same, why can't platform data be utilised for this?

Quoted 7 lines · Tomer Maimon

Quoted 6 lines · Adrian Hunter

Quoted 5 lines · Andy Shevchenko

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Adrian Hunter

Quoted 1 line · Andy Shevchenko

On Mon, Dec 5, 2022 at 1:20 PM Tomer Maimon <[email protected]> wrote:

...

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

Quoted 10 lines

Quoted 4 lines

Quoted 2 lines

devm_ is problematic in your case.
TL;DR: you need to use clk_get_optional() and clk_put().

devm_ calls exactly those, so what is the issue?

The issue is the error path or removal stage where it may or may be not problematic. To be on the safe side, the best approach is to make sure that allocated resources are being deallocated in the reversed order. That said, the

Quoted 4 lines

1. call non-devm_func()
2. call devm_func()

is wrong strictly speaking.

To elaborate more, the

1. call all devm_func()
2. call only non-devm_func()

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

is the correct order.

1. WRT pltfm_host->clk, that is what is happening
2. WRT other resources that is simply not always possible because not every resource is wrapped by devm_ e.g. mmc_alloc_host() / mmc_free_host()

I little confused about what to decide, should I use only non-devm_func because mmc_alloc_host() / mmc_free_host() is not warrped with devm_?

It is up to you how to proceed. I pointed out the problem with your code which may or may not be fatal.

If you want to solve it, there are several approaches:
1) get rid of devm_ completely;
2) properly shuffle the ordering in ->probe(), so all devm_ calls are followed by non-devm_;
3) wrap non-devm_ cals to become managed (see devm_add_action_or_reset() approach);
4) fix SDHCI / MMC layer by providing necessary devm_ calls and/or fix sdhci_pltfm_register() to handle the clock.

Personally, the list order is from the least, what I prefer, to the most (i.o.w. I would like to see rather 4) than 1) to be implemented).

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Hence in this case the driver can be worked around easily (by shuffling the order in ->probe() to call devm_ first), but as I said looking into implementation of the _unregister() I'm pretty sure that clock management should be in sdhci-pltfm, rather than in all callers who won't need the full customization.

Hope this helps to understand my point.

Quoted 6 lines

Quoted 4 lines

Quoted 4 lines

Your ->remove() callback doesn't free resources in the reversed order which may or, by luck, may not be the case of all possible crashes, UAFs, races, etc during removal stage. All the same for error path in ->probe().

I also pointed out above what would be the outcome of neglecting this rule.

...

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

Quoted 1 line

Why can't you use sdhci_pltfm_register()?

two things are missing in sdhci_pltfm_register
1. clock.

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Taking into account the implementation of the corresponding _unregister() I would add the clock handling to the _register() one. Perhaps via a new member of the platform data that supplies the name and index of the clock and hence all clk_get_optional() / clk_put will be moved there.

Do you mean to add it to sdhci_pltfm_register function? if yes I believe it will take some time to modify sdhci_pltfm_register I prefer not to use sdhci_pltfm_register.

In the Linux kernel we are trying hard to avoid code duplication. Why do you need it to be open coded? (Yes, I heard you, but somebody should fix the issues with that funcion at some point, right?)

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

2. Adding SDHCI_CAN_DO_8BIT capability according the eMMC capabilities.

All the same, why can't platform data be utilised for this?

Quoted 8 lines · Andy Shevchenko

Quoted 7 lines · Tomer Maimon

Quoted 6 lines · Adrian Hunter

Quoted 5 lines · Andy Shevchenko

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Adrian Hunter

Quoted 1 line · Andy Shevchenko

On Mon, Dec 5, 2022 at 1:20 PM Tomer Maimon <[email protected]> wrote:

Quoted 32 lines

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

Quoted 10 lines

Quoted 4 lines

Quoted 2 lines

devm_ is problematic in your case.
TL;DR: you need to use clk_get_optional() and clk_put().

devm_ calls exactly those, so what is the issue?

The issue is the error path or removal stage where it may or may be not problematic. To be on the safe side, the best approach is to make sure that allocated resources are being deallocated in the reversed order. That said, the

Quoted 4 lines

1. call non-devm_func()
2. call devm_func()

is wrong strictly speaking.

To elaborate more, the

1. call all devm_func()
2. call only non-devm_func()

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

is the correct order.

1. WRT pltfm_host->clk, that is what is happening
2. WRT other resources that is simply not always possible because not every resource is wrapped by devm_ e.g. mmc_alloc_host() / mmc_free_host()

I little confused about what to decide, should I use only non-devm_func because mmc_alloc_host() / mmc_free_host() is not warrped with devm_?

It is up to you how to proceed. I pointed out the problem with your code which may or may not be fatal.

Quoted 8 lines

If you want to solve it, there are several approaches:
1) get rid of devm_ completely;
2) properly shuffle the ordering in ->probe(), so all devm_ calls are followed by non-devm_;
3) wrap non-devm_ cals to become managed (see devm_add_action_or_reset() approach);
4) fix SDHCI / MMC layer by providing necessary devm_ calls and/or fix sdhci_pltfm_register() to handle the clock.

I can take care of sdhci_pltfm when I next have some time. Otherwise it looks OK to me, so I am acking it.

Quoted 17 lines

Personally, the list order is from the least, what I prefer, to the most (i.o.w. I would like to see rather 4) than 1) to be implemented).

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Hence in this case the driver can be worked around easily (by shuffling the order in ->probe() to call devm_ first), but as I said looking into implementation of the _unregister() I'm pretty sure that clock management should be in sdhci-pltfm, rather than in all callers who won't need the full customization.

Hope this helps to understand my point.

Quoted 6 lines

Quoted 4 lines

Quoted 4 lines

Your ->remove() callback doesn't free resources in the reversed order which may or, by luck, may not be the case of all possible crashes, UAFs, races, etc during removal stage. All the same for error path in ->probe().

I also pointed out above what would be the outcome of neglecting this rule.

Quoted 20 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

Quoted 1 line

Why can't you use sdhci_pltfm_register()?

two things are missing in sdhci_pltfm_register
1. clock.

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Taking into account the implementation of the corresponding _unregister() I would add the clock handling to the _register() one. Perhaps via a new member of the platform data that supplies the name and index of the clock and hence all clk_get_optional() / clk_put will be moved there.

Do you mean to add it to sdhci_pltfm_register function? if yes I believe it will take some time to modify sdhci_pltfm_register I prefer not to use sdhci_pltfm_register.

In the Linux kernel we are trying hard to avoid code duplication. Why do you need it to be open coded? (Yes, I heard you, but somebody should fix the issues with that funcion at some point, right?)

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

2. Adding SDHCI_CAN_DO_8BIT capability according the eMMC capabilities.

All the same, why can't platform data be utilised for this?

Quoted 9 lines · Adrian Hunter

Quoted 8 lines · Andy Shevchenko

Quoted 7 lines · Tomer Maimon

Quoted 6 lines · Adrian Hunter

Quoted 5 lines · Andy Shevchenko

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Adrian Hunter

Quoted 1 line · Andy Shevchenko

On Mon, Dec 5, 2022 at 1:20 PM Tomer Maimon <[email protected]> wrote:

...

Quoted 41 lines

Quoted 32 lines

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

Quoted 10 lines

Quoted 4 lines

Quoted 2 lines

devm_ is problematic in your case.
TL;DR: you need to use clk_get_optional() and clk_put().

devm_ calls exactly those, so what is the issue?

The issue is the error path or removal stage where it may or may be not problematic. To be on the safe side, the best approach is to make sure that allocated resources are being deallocated in the reversed order. That said, the

Quoted 4 lines

1. call non-devm_func()
2. call devm_func()

is wrong strictly speaking.

To elaborate more, the

1. call all devm_func()
2. call only non-devm_func()

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

is the correct order.

1. WRT pltfm_host->clk, that is what is happening
2. WRT other resources that is simply not always possible because not every resource is wrapped by devm_ e.g. mmc_alloc_host() / mmc_free_host()

I little confused about what to decide, should I use only non-devm_func because mmc_alloc_host() / mmc_free_host() is not warrped with devm_?

It is up to you how to proceed. I pointed out the problem with your code which may or may not be fatal.

Quoted 8 lines

If you want to solve it, there are several approaches:
1) get rid of devm_ completely;
2) properly shuffle the ordering in ->probe(), so all devm_ calls are followed by non-devm_;
3) wrap non-devm_ cals to become managed (see devm_add_action_or_reset() approach);
4) fix SDHCI / MMC layer by providing necessary devm_ calls and/or fix sdhci_pltfm_register() to handle the clock.

Quoted 2 lines

I can take care of sdhci_pltfm when I next have some time. Otherwise it looks OK to me, so I am acking it.

Thank you!

Quoted 17 lines

Quoted 17 lines

Personally, the list order is from the least, what I prefer, to the most (i.o.w. I would like to see rather 4) than 1) to be implemented).

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Hence in this case the driver can be worked around easily (by shuffling the order in ->probe() to call devm_ first), but as I said looking into implementation of the _unregister() I'm pretty sure that clock management should be in sdhci-pltfm, rather than in all callers who won't need the full customization.

Hope this helps to understand my point.

Quoted 6 lines

Quoted 4 lines

Quoted 4 lines

Your ->remove() callback doesn't free resources in the reversed order which may or, by luck, may not be the case of all possible crashes, UAFs, races, etc during removal stage. All the same for error path in ->probe().

I also pointed out above what would be the outcome of neglecting this rule.

...

Quoted 13 lines

Quoted 13 lines

Quoted 12 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 3 lines

Quoted 1 line

Why can't you use sdhci_pltfm_register()?

two things are missing in sdhci_pltfm_register
1. clock.

Taking into account the implementation of the corresponding _unregister() I would add the clock handling to the _register() one. Perhaps via a new member of the platform data that supplies the name and index of the clock and hence all clk_get_optional() / clk_put will be moved there.

Do you mean to add it to sdhci_pltfm_register function? if yes I believe it will take some time to modify sdhci_pltfm_register I prefer not to use sdhci_pltfm_register.

Quoted 7 lines

Quoted 7 lines

In the Linux kernel we are trying hard to avoid code duplication. Why do you need it to be open coded? (Yes, I heard you, but somebody should fix the issues with that funcion at some point, right?)

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

2. Adding SDHCI_CAN_DO_8BIT capability according the eMMC capabilities.

All the same, why can't platform data be utilised for this?

--
With Best Regards,
Andy Shevchenko

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 | 84 +++++++++++++++++++++++++++++++++++
 3 files changed, 93 insertions(+)
 create mode 100644 drivers/mmc/host/sdhci-npcm.c

drivers/mmc/host/Kconfig +8 -0

index fb1062a6394c..82ab6fc25dca 100644
@@ -415,6 +415,14 @@ config MMC_SDHCI_MILBEAUT
 
 	  If unsure, say N.
 
+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_SDHCI_IPROC
 	tristate "SDHCI support for the BCM2835 & iProc SD/MMC Controller"
 	depends on ARCH_BCM2835 || ARCH_BCM_IPROC || ARCH_BRCMSTB || COMPILE_TEST

drivers/mmc/host/Makefile +1 -0

index 4e4ceb32c4b4..a101f87a5f19 100644
@@ -97,6 +97,7 @@ obj-$(CONFIG_MMC_SDHCI_MICROCHIP_PIC32)	+= sdhci-pic32.o
 obj-$(CONFIG_MMC_SDHCI_BRCMSTB)		+= sdhci-brcmstb.o
 obj-$(CONFIG_MMC_SDHCI_OMAP)		+= sdhci-omap.o
 obj-$(CONFIG_MMC_SDHCI_SPRD)		+= sdhci-sprd.o
+obj-$(CONFIG_MMC_SDHCI_NPCM)		+= sdhci-npcm.o
 obj-$(CONFIG_MMC_CQHCI)			+= cqhci.o
 cqhci-y					+= cqhci-core.o
 cqhci-$(CONFIG_MMC_CRYPTO)		+= cqhci-crypto.o

drivers/mmc/host/sdhci-npcm.c new file +84 -0

new file mode 100644
index 000000000000..beace15b6c00
@@ -0,0 +1,84 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * NPCM SDHC MMC host controller driver.
+ *
+ * Copyright (c) 2020 Nuvoton Technology corporation.
+ */
+
+#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 "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_optional(&pdev->dev, NULL);
+	if (IS_ERR(pltfm_host->clk))
+		return PTR_ERR(pltfm_host->clk);
+
+	ret = clk_prepare_enable(pltfm_host->clk);
+	if (ret)
+		return ret;
+
+	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");

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 | 84 +++++++++++++++++++++++++++++++++++
 3 files changed, 93 insertions(+)
 create mode 100644 drivers/mmc/host/sdhci-npcm.c

drivers/mmc/host/Kconfig +8 -0

index fb1062a6394c..82ab6fc25dca 100644
@@ -415,6 +415,14 @@ config MMC_SDHCI_MILBEAUT
 
 	  If unsure, say N.
 
+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_SDHCI_IPROC
 	tristate "SDHCI support for the BCM2835 & iProc SD/MMC Controller"
 	depends on ARCH_BCM2835 || ARCH_BCM_IPROC || ARCH_BRCMSTB || COMPILE_TEST

drivers/mmc/host/Makefile +1 -0

index 4e4ceb32c4b4..a101f87a5f19 100644
@@ -97,6 +97,7 @@ obj-$(CONFIG_MMC_SDHCI_MICROCHIP_PIC32)	+= sdhci-pic32.o
 obj-$(CONFIG_MMC_SDHCI_BRCMSTB)		+= sdhci-brcmstb.o
 obj-$(CONFIG_MMC_SDHCI_OMAP)		+= sdhci-omap.o
 obj-$(CONFIG_MMC_SDHCI_SPRD)		+= sdhci-sprd.o
+obj-$(CONFIG_MMC_SDHCI_NPCM)		+= sdhci-npcm.o
 obj-$(CONFIG_MMC_CQHCI)			+= cqhci.o
 cqhci-y					+= cqhci-core.o
 cqhci-$(CONFIG_MMC_CRYPTO)		+= cqhci-crypto.o

drivers/mmc/host/sdhci-npcm.c new file +33 -0

new file mode 100644
index 000000000000..beace15b6c00
@@ -0,0 +1,84 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * NPCM SDHC MMC host controller driver.
+ *
+ * Copyright (c) 2020 Nuvoton Technology corporation.
+ */
+
+#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 "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);
+

drivers/mmc/host/sdhci-npcm.c new file +41 -0

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

Signature · 1 line

2.33.0

Quoted 3 lines · Tomer Maimon

Add Nuvoton NPCM BMC sdhci-pltfm controller driver.

Signed-off-by Tomer Maimon <[email protected]>

I still don't see this driver in the upstream kernel, or in linux-next.

Couple of comments:

- devm ordering does not really matter here. The devm resource is the clock, it does not depend on local data, and it will be released last, so that is ok.

Quoted 10 lines · Guenter Roeck

Quoted 3 lines · Tomer Maimon

Add Nuvoton NPCM BMC sdhci-pltfm controller driver.

Signed-off-by Tomer Maimon <[email protected]>

I still don't see this driver in the upstream kernel, or in linux-next.

Couple of comments:

Quoted 3 lines

- devm ordering does not really matter here. The devm resource is the clock, it does not depend on local data, and it will be released last, so that is ok.

Not sure. Strictly speaking this is the problem. If you leave a clock going on in a wrong period of time it (theoretically) might break your hardware once and forever. Similar discussion about power, clock and reset signals has been held for camera sensors.

Quoted 3 lines · Tomer Maimon

Add Nuvoton NPCM BMC sdhci-pltfm controller driver.

Signed-off-by Tomer Maimon <[email protected]>

I still don't see this driver in the upstream kernel, or in linux-next.

Couple of comments:

- devm ordering does not really matter here. The devm resource is the clock, it does not depend on local data, and it will be released last, so that is ok.
- sdhci_pltfm_unregister() calls clk_disable_unprepare(), so there is no enabled clock floating around on driver removal. Unfortunately, that also means that the more convenient devm_clk_get_optional_enabled() can not be used.

Real problem inline below.

Guenter

Quoted 77 lines

---
 drivers/mmc/host/Kconfig      |  8 ++++
 drivers/mmc/host/Makefile     |  1 +
 drivers/mmc/host/sdhci-npcm.c | 84 +++++++++++++++++++++++++++++++++++
 3 files changed, 93 insertions(+)
 create mode 100644 drivers/mmc/host/sdhci-npcm.c

drivers/mmc/host/Kconfig +8 -0

index fb1062a6394c..82ab6fc25dca 100644
@@ -415,6 +415,14 @@ config MMC_SDHCI_MILBEAUT
 
        If unsure, say N.
 
+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_SDHCI_IPROC
      tristate "SDHCI support for the BCM2835 & iProc SD/MMC Controller"
      depends on ARCH_BCM2835 || ARCH_BCM_IPROC || ARCH_BRCMSTB || COMPILE_TEST

drivers/mmc/host/Makefile +1 -0

index 4e4ceb32c4b4..a101f87a5f19 100644
@@ -97,6 +97,7 @@ obj-$(CONFIG_MMC_SDHCI_MICROCHIP_PIC32)     += sdhci-pic32.o
 obj-$(CONFIG_MMC_SDHCI_BRCMSTB)              += sdhci-brcmstb.o
 obj-$(CONFIG_MMC_SDHCI_OMAP)         += sdhci-omap.o
 obj-$(CONFIG_MMC_SDHCI_SPRD)         += sdhci-sprd.o
+obj-$(CONFIG_MMC_SDHCI_NPCM)         += sdhci-npcm.o
 obj-$(CONFIG_MMC_CQHCI)                      += cqhci.o
 cqhci-y                                      += cqhci-core.o
 cqhci-$(CONFIG_MMC_CRYPTO)           += cqhci-crypto.o

drivers/mmc/host/sdhci-npcm.c new file +33 -0

new file mode 100644
index 000000000000..beace15b6c00
@@ -0,0 +1,84 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * NPCM SDHC MMC host controller driver.
+ *
+ * Copyright (c) 2020 Nuvoton Technology corporation.
+ */
+
+#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 "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);
+

drivers/mmc/host/sdhci-npcm.c line 43

Quoted 10 lines

drivers/mmc/host/sdhci-npcm.c new file +10 -0

+     pltfm_host = sdhci_priv(host);
+
+     pltfm_host->clk = devm_clk_get_optional(&pdev->dev, NULL);
+     if (IS_ERR(pltfm_host->clk))
+             return PTR_ERR(pltfm_host->clk);
+
+     ret = clk_prepare_enable(pltfm_host->clk);
+     if (ret)
+             return ret;
+

The two functions above should not return but goto the call to sdhci_pltfm_free() to avoid a memory leak on error.

Quoted 44 lines

drivers/mmc/host/sdhci-npcm.c new file +41 -1

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

2.33.0