[U-Boot] [PATCH v3 7/9] dm: mmc: sunxi: Add CLK and RESET support

André Przywara
2019-01-22T23:36:58Z

On 21/01/2019 10:31, Jagan Teki wrote:
> Now CLK and RESET driver for Allwinner SoC are available,
> so add the relevant operations on mmc sunxi driver.
>
> Cc: Jaehoon Chung <jh80.chung@samsung.com>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
> Changes for v3:
> - Grab changes for ML
>
> drivers/mmc/sunxi_mmc.c | 52 +++++++++++++++++++++++++++++++++++++----
> 1 file changed, 47 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c
> index 0c443a732d..3c17958c95 100644
> --- a/drivers/mmc/sunxi_mmc.c
> +++ b/drivers/mmc/sunxi_mmc.c
> @@ -8,10 +8,12 @@
> */
>
> #include <common.h>
> +#include <clk.h>
> #include <dm.h>
> #include <errno.h>
> #include <malloc.h>
> #include <mmc.h>
> +#include <reset.h>
> #include <asm/io.h>
> #include <asm/arch/clock.h>
> #include <asm/arch/cpu.h>
> @@ -21,7 +23,6 @@
>
> #ifdef CONFIG_DM_MMC
> struct sunxi_mmc_variant {
> - u16 gate_offset;
> u16 mclk_offset;
> };
> #endif
> @@ -41,6 +42,8 @@ struct sunxi_mmc_priv {
> struct mmc_config cfg;
> #ifdef CONFIG_DM_MMC
> const struct sunxi_mmc_variant *variant;
> + struct clk clk_ahb;
> + struct reset_ctl reset;

Where is that used again outside of the probe() function?

> #endif
> };
>
> @@ -602,6 +605,32 @@ static const struct dm_mmc_ops sunxi_mmc_ops = {
> .get_cd = sunxi_mmc_getcd,
> };
>
> +static int sunxi_mmc_enable(struct udevice *dev)
> +{
> + struct sunxi_mmc_priv *priv = dev_get_priv(dev);
> + int ret;
> +
> + ret = clk_enable(&priv->clk_ahb);
> + if (ret) {
> + dev_err(dev, "failed to enable AHB clock\n");
> + return ret;
> + }
> +
> + if (reset_valid(&priv->reset)) {
> + ret = reset_deassert(&priv->reset);
> + if (ret) {
> + dev_err(dev, "failed to deassert reset\n");
> + goto err_clk_ahb;
> + }
> + }
> +
> + return 0;
> +
> +err_clk_ahb:
> + clk_disable(&priv->clk_ahb);
> + return ret;
> +}
> +

Frankly, that's a lots of lines for very little effect.
Why not just use the much smaller version I had and add an "else
dev_err(...)" to it, if you are keen about error reports?
Saves you the variables, saves you the reset_valid() call, saves you the
extra function. Less code to read, smaller image, less bugs.

Cheers,
Andre.

> static int sunxi_mmc_probe(struct udevice *dev)
> {
> struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev);
> @@ -609,7 +638,7 @@ static int sunxi_mmc_probe(struct udevice *dev)
> struct sunxi_mmc_priv *priv = dev_get_priv(dev);
> struct mmc_config *cfg = &plat->cfg;
> struct ofnode_phandle_args args;
> - u32 *gate_reg, *ccu_reg;
> + u32 *ccu_reg;
> int bus_width, ret;
>
> cfg->name = dev->name;
> @@ -641,8 +670,22 @@ static int sunxi_mmc_probe(struct udevice *dev)
> priv->mmc_no = ((uintptr_t)priv->reg - SUNXI_MMC0_BASE) / 0x1000;
> priv->mclkreg = (void *)ccu_reg +
> (priv->variant->mclk_offset + (priv->mmc_no * 4));
> - gate_reg = (void *)ccu_reg + priv->variant->gate_offset;
> - setbits_le32(gate_reg, BIT(AHB_GATE_OFFSET_MMC(priv->mmc_no)));
> +
> + ret = clk_get_by_name(dev, "ahb", &priv->clk_ahb);
> + if (ret) {
> + dev_err(dev, "failed to get AHB clock\n");
> + return ret;
> + }
> +
> + ret = reset_get_by_name(dev, "ahb", &priv->reset);
> + if (ret && ret != -ENOENT) {
> + dev_err(dev, "failed to get reset\n");
> + return ret;
> + }
> +
> + ret = sunxi_mmc_enable(dev);
> + if (ret)
> + return ret;
>
> ret = mmc_set_mod_clk(priv, 24000000);
> if (ret)
> @@ -676,7 +719,6 @@ static int sunxi_mmc_bind(struct udevice *dev)
> }
>
> static const struct sunxi_mmc_variant sun4i_a10_variant = {
> - .gate_offset = 0x60,
> .mclk_offset = 0x88,
> };
>
>