[U-Boot] [PATCH v2 2/4] mmc: Add Aspeed SD controller driver

Cédric Le Goater
2019-08-14T15:18:23Z

On 13/08/2019 21:31, Eddie James wrote:
> Add support for the Aspeed SD host controller engine.

It looks correct and simple enough. Some comments below.

>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> ---
> drivers/mmc/Kconfig | 11 +++++++
> drivers/mmc/Makefile | 1 +
> drivers/mmc/aspeed_sdhci.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 90 insertions(+)
> create mode 100644 drivers/mmc/aspeed_sdhci.c
>
> diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
> index c6812f6..536f66a 100644
> --- a/drivers/mmc/Kconfig
> +++ b/drivers/mmc/Kconfig
> @@ -421,6 +421,17 @@ config SPL_MMC_SDHCI_ADMA
> This enables support for the ADMA (Advanced DMA) defined
> in the SD Host Controller Standard Specification Version 3.00 in SPL.
>
> +config MMC_SDHCI_ASPEED
> + bool "Aspeed SDHCI controller"
> + depends on ARCH_ASPEED
> + depends on DM_MMC
> + depends on MMC_SDHCI
> + help
> + Enables support for the Aspeed SDHCI 2.0 controller present on Aspeed
> + SoCs. This device is compatible with SD 3.0 and/or MMC 4.3
> + specifications. On the AST2600, the device is also compatible with
> + MMC 5.1 and eMMC 3.0.
> +
> config MMC_SDHCI_ATMEL
> bool "Atmel SDHCI controller support"
> depends on ARCH_AT91
> diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile
> index 6cc018b..5594195 100644
> --- a/drivers/mmc/Makefile
> +++ b/drivers/mmc/Makefile
> @@ -46,6 +46,7 @@ obj-$(CONFIG_JZ47XX_MMC) += jz_mmc.o
>
> # SDHCI
> obj-$(CONFIG_MMC_SDHCI) += sdhci.o
> +obj-$(CONFIG_MMC_SDHCI_ASPEED) += aspeed_sdhci.o
> obj-$(CONFIG_MMC_SDHCI_ATMEL) += atmel_sdhci.o
> obj-$(CONFIG_MMC_SDHCI_BCM2835) += bcm2835_sdhci.o
> obj-$(CONFIG_MMC_SDHCI_BCMSTB) += bcmstb_sdhci.o
> diff --git a/drivers/mmc/aspeed_sdhci.c b/drivers/mmc/aspeed_sdhci.c
> new file mode 100644
> index 0000000..c292c42
> --- /dev/null
> +++ b/drivers/mmc/aspeed_sdhci.c
> @@ -0,0 +1,78 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2019 IBM Corp.
> + * Eddie James <eajames@linux.ibm.com>
> + */
> +
> +#include <common.h>
> +#include <clk.h>
> +#include <dm.h>
> +#include <malloc.h>
> +#include <sdhci.h>
> +
> +struct aspeed_sdhci_plat {
> + struct mmc_config cfg;
> + struct mmc mmc;
> +};
> +
> +static int aspeed_sdhci_probe(struct udevice *dev)
> +{
> + struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev);
> + struct aspeed_sdhci_plat *plat = dev_get_platdata(dev);
> + struct sdhci_host *host = dev_get_priv(dev);
> + u32 max_clk;
> + struct clk clk;
> + int ret;
> +
> + ret = clk_get_by_index(dev, 0, &clk);
> + if (ret)
> + return ret;
> +
> + ret = clk_enable(&clk);
> + if (ret)
> + return ret;

Don't we need to release the clock below in case of error ?
> + host->name = dev->name;
> + host->ioaddr = (void *)devfdt_get_addr(dev);
> +
> + max_clk = clk_get_rate(&clk);
> + if (!max_clk)

may be use IS_ERR_VALUE(max_clk) and return max_clk

> + return -EINVAL;
> +
> + host->max_clk = max_clk;
> + host->mmc = &plat->mmc;
> + host->mmc->dev = dev;
> + host->mmc->priv = host;
> + upriv->mmc = host->mmc;
> +
> + ret = sdhci_setup_cfg(&plat->cfg, host, 0, 0);
> + if (ret)
> + return ret;
> +
> + return sdhci_probe(dev);
> +}
> +
> +static int aspeed_sdhci_bind(struct udevice *dev)
> +{
> + struct aspeed_sdhci_plat *plat = dev_get_platdata(dev);
> +
> + return sdhci_bind(dev, &plat->mmc, &plat->cfg);
> +}
> +
> +static const struct udevice_id aspeed_sdhci_ids[] = {
> + { .compatible = "aspeed,ast2400-sdhci" },
> + { .compatible = "aspeed,ast2500-sdhci" },
> + { .compatible = "aspeed,ast2600-sdhci" },
> + { }
> +};
> +
> +U_BOOT_DRIVER(aspeed_sdhci_drv) = {
> + .name = "aspeed_sdhci",
> + .id = UCLASS_MMC,
> + .of_match = aspeed_sdhci_ids,
> + .ops = &sdhci_ops,
> + .bind = aspeed_sdhci_bind,
> + .probe = aspeed_sdhci_probe,
> + .priv_auto_alloc_size = sizeof(struct sdhci_host),
> + .platdata_auto_alloc_size = sizeof(struct aspeed_sdhci_plat),
> +};
>