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

[André Przywara]

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 at samsung.com>
> Signed-off-by: Jagan Teki <jagan at 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,
>  };
>  
>