Hi Jaehoon,
> -----Original Message-----
> From: Jaehoon Chung [mailto:jh80.ch...@samsung.com]
> Sent: 2016年7月21日 12:17
> To: Wenyou Yang <wenyou.y...@atmel.com>; U-Boot Mailing List <u-
> b...@lists.denx.de>
> Cc: Pantelis Antoniou <pa...@antoniou-consulting.com>; Andreas Bießmann
> <andr...@biessmann.org>; Simon Glass <s...@chromium.org>
> Subject: Re: [PATCH v7] mmc: atmel_sdhci: Convert to the driver model support
>
> Hi Wenyuo,
>
> On 07/20/2016 07:04 PM, Wenyou Yang wrote:
> > Convert the driver to the driver model while retaining the existing
> > legacy code. This allows the driver to support boards that have
> > converted to driver model as well as those that have not.
> >
> > Signed-off-by: Wenyou Yang <wenyou.y...@atmel.com>
> > Reviewed-by: Simon Glass <s...@chromium.org>
> > ---
> >
> > Changes in v7:
> > - Add support for using driver model for block devices and MMC operations.
> > - Change clk_client.h -> clk.h to adapt to clk API conversion.
> >
> > Changes in v6:
> > - Remove unnecessary white space.
> > - Use sdhci_read(), instead of readl().
> > - Remove the local variables min_clk.
> >
> > Changes in v5:
> > - Add Reviewed-by tag.
> >
> > Changes in v4:
> > - Update the clk API based on [PATCH] clk: convert API to match
> > reset/mailbox fstyle (http://patchwork.ozlabs.org/patch/625342/).
> > - Remove check on dev_get_parent() return.
> > - Fixed the return value, such as -ENODEV->-EINVAL.
> >
> > Changes in v3:
> > - Remove the redundant log print.
> >
> > Changes in v2:
> > - Add clock support, include enabling peripheral clock and
> > generated clock.
> > - Retain the existing legacy code to support boards which have not
> > converted to driver model.
> >
> > drivers/mmc/Kconfig | 10 ++++
> > drivers/mmc/atmel_sdhci.c | 125
> ++++++++++++++++++++++++++++++++++++++++++++++
> > include/sdhci.h | 2 +
> > 3 files changed, 137 insertions(+)
> >
> > diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig index
> > 79cf18f..49b325e 100644
> > --- a/drivers/mmc/Kconfig
> > +++ b/drivers/mmc/Kconfig
> > @@ -34,6 +34,16 @@ config MSM_SDHCI
> > SD 3.0 specifications. Both SD and eMMC devices are supported.
> > Card-detect gpios are not supported.
> >
> > +config ATMEL_SDHCI
> > + bool "Atmel SDHCI controller support"
> > + depends on DM_MMC && BLK && DM_MMC_OPS && ARCH_AT91
> > + help
> > + This enables support for the Atmel SDHCI controller, which supports
> > + the embedded MultiMedia Card (e.MMC) Specification V4.51, the SD
> > + Memory Card Specification V3.0, and the SDIO V3.0 specification.
> > + It is compliant with the SD Host Controller Standard V3.0
> > + specification.
> > +
> > config ROCKCHIP_DWMMC
> > bool "Rockchip SD/MMC controller support"
> > depends on DM_MMC && OF_CONTROL
> > diff --git a/drivers/mmc/atmel_sdhci.c b/drivers/mmc/atmel_sdhci.c
> > index 24b68b6..977f6ef 100644
> > --- a/drivers/mmc/atmel_sdhci.c
> > +++ b/drivers/mmc/atmel_sdhci.c
> > @@ -6,12 +6,15 @@
> > */
> >
> > #include <common.h>
> > +#include <clk.h>
> > +#include <dm.h>
>
> If it doesn't use DM_MMC, then doesn't it need to include dm.h?
>
> > #include <malloc.h>
> > #include <sdhci.h>
> > #include <asm/arch/clk.h>
> >
> > #define ATMEL_SDHC_MIN_FREQ 400000
> >
> > +#ifndef CONFIG_DM_MMC
> > int atmel_sdhci_init(void *regbase, u32 id) {
> > struct sdhci_host *host;
> > @@ -38,3 +41,125 @@ int atmel_sdhci_init(void *regbase, u32 id)
> >
> > return 0;
> > }
> > +
> > +#else
> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > +struct atmel_sdhci_plat {
> > + struct mmc_config cfg;
> > + struct mmc mmc;
> > +};
> > +
> > +static int atmel_sdhci_probe(struct udevice *dev) {
> > + struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev);
> > + struct atmel_sdhci_plat *plat = dev_get_platdata(dev);
> > + struct sdhci_host *host = dev_get_priv(dev);
> > + u32 max_clk;
> > + u32 caps, caps_1;
> > + u32 clk_base, clk_mul;
> > + u32 gck_rate;
> > + struct udevice *dev_clk;
> > + struct clk clk;
> > + int periph, ret;
> > +
> > + ret = clk_get_by_index(dev, 0, &clk);
> > + if (ret)
> > + return ret;
> > +
> > + periph = fdtdec_get_uint(gd->fdt_blob, clk.dev->of_offset, "reg", -1);
> > + if (periph < 0)
> > + return -EINVAL;
>
> periph is just used to pass to clk.id. it looks meaningless.
> As i already mentioned, you can make more cleaner than now.
There is a litter difference from usual clock structure for Atmel clock driver.
The periph value is got from a child node, then is assigned to its parent clock
node.
>
> > +
> > + dev_clk = dev_get_parent(clk.dev);
> > + ret = clk_request(dev_clk, &clk);
> > + if (ret)
> > + return ret;
> > +
> > + clk.id = periph;
> > + ret = clk_enable(&clk);
> > + if (ret)
> > + return ret;
> > +
> > + host->name = (char *)dev->name;
>
> Needs the type casting?
Accepted.
>
> > + host->ioaddr = (void *)dev_get_addr(dev);
> > +
> > + host->quirks = 0;
> > + host->version = sdhci_readw(host, SDHCI_HOST_VERSION);
> > +
> > + host->bus_width = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
> > + "bus-width", 4);
> > +
> > + caps = sdhci_readl(host, SDHCI_CAPABILITIES);
> > + clk_base = (caps & SDHCI_CLOCK_V3_BASE_MASK) >>
> SDHCI_CLOCK_BASE_SHIFT;
> > + caps_1 = sdhci_readl(host, SDHCI_CAPABILITIES_1);
> > + clk_mul = (caps_1 & SDHCI_CLOCK_MUL_MASK) >>
> SDHCI_CLOCK_MUL_SHIFT;
> > + gck_rate = clk_base * 1000000 * (clk_mul + 1);
>
> gck_rate can be over u32 boundary, doesn't?
Accepted.
>
> > +
> > + ret = clk_get_by_index(dev, 1, &clk);
> > + if (ret)
> > + return ret;
>
> It can reuse the above clock get sequence.
>
> If reuse this sequence, lines can be reduced.
>
> My suggestion is
>
> 1. Make atmel_sdhci_get_clk(..., int index), it should be returned "struct
> clk" type.
>
> 2. Then the below sequence should be included in function,
>
> atmel_sdhci_get_clk(..., int index) {
>
> clk_get_by_index(dev, index, &clk);
> ...
>
> return clk;
> }
>
> How about?
Good ideas, thanks a lot.
>
> Best Regards,
> Jaehoon Chung
>
> > +
> > + periph = fdtdec_get_uint(gd->fdt_blob, clk.dev->of_offset, "reg", -1);
> > + if (periph < 0)
> > + return -EINVAL;
> > +
> > + dev_clk = dev_get_parent(clk.dev);
> > + ret = clk_request(dev_clk, &clk);
> > + if (ret)
> > + return ret;
> > +
> > + clk.id = periph;
> > + ret = clk_set_rate(&clk, gck_rate);
> > + if (ret)
> > + return ret;
> > +
> > + max_clk = clk_get_rate(&clk);
> > + if (!max_clk)
> > + return -EINVAL;
> > +
> > + ret = sdhci_setup_cfg(&plat->cfg, dev->name, host->bus_width,
> > + caps, max_clk, ATMEL_SDHC_MIN_FREQ,
> > + host->version, host->quirks, 0);
> > + if (ret)
> > + return ret;
> > +
> > + host->mmc = &plat->mmc;
> > + host->mmc->dev = dev;
> > + host->mmc->priv = host;
> > + upriv->mmc = host->mmc;
> > +
> > + clk_free(&clk);
> > +
> > + return sdhci_probe(dev);
> > +}
> > +
> > +static int atmel_sdhci_bind(struct udevice *dev) {
> > + struct atmel_sdhci_plat *plat = dev_get_platdata(dev);
> > + int ret;
> > +
> > + ret = sdhci_bind(dev, &plat->mmc, &plat->cfg);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct udevice_id atmel_sdhci_ids[] = {
> > + { .compatible = "atmel,sama5d2-sdhci" },
> > + { }
> > +};
> > +
> > +U_BOOT_DRIVER(atmel_sdhci_drv) = {
> > + .name = "atmel_sdhci",
> > + .id = UCLASS_MMC,
> > + .of_match = atmel_sdhci_ids,
> > + .ops = &sdhci_ops,
> > + .bind = atmel_sdhci_bind,
> > + .probe = atmel_sdhci_probe,
> > + .priv_auto_alloc_size = sizeof(struct sdhci_host),
> > + .platdata_auto_alloc_size = sizeof(struct atmel_sdhci_plat), };
> > +#endif
> > diff --git a/include/sdhci.h b/include/sdhci.h index c4d3b55..fdc0032
> > 100644
> > --- a/include/sdhci.h
> > +++ b/include/sdhci.h
> > @@ -166,6 +166,8 @@
> > #define SDHCI_CAN_64BIT 0x10000000
> >
> > #define SDHCI_CAPABILITIES_1 0x44
> > +#define SDHCI_CLOCK_MUL_MASK 0x00FF0000
> > +#define SDHCI_CLOCK_MUL_SHIFT 16
> >
> > #define SDHCI_MAX_CURRENT 0x48
> >
> >
Best Regards,
Wenyou Yang
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
