| Message ID | 20240619054641.277062-3-shanchun1218@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for Nuvovon MA35D1 SDHCI | expand |
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 |= 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
… > 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
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
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
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 >
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 |= 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
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
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
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
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 |= 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");
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 -- 2.25.1