Re: [PATCH 08/14] mmc: meson-gx: rework clock init function

[Kevin Hilman]

Jerome Brunet <jbrunet@xxxxxxxxxxxx> writes:

> Perform basic initialisation of the clk register before providing it to
> the CCF.
>
> Thanks to devm, carrying the clock structure around after init is not
> necessary. Rework the function to remove these from the controller host
> data.
>
> Finally, set initial mmc clock rate before enabling it, simplifying the
> exit condition.
>
> Signed-off-by: Jerome Brunet <jbrunet@xxxxxxxxxxxx>
> ---
>  drivers/mmc/host/meson-gx-mmc.c | 101 +++++++++++++++++++---------------------
>  1 file changed, 49 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
> index 8f9ba5190c18..4cc7d6530536 100644
> --- a/drivers/mmc/host/meson-gx-mmc.c
> +++ b/drivers/mmc/host/meson-gx-mmc.c
> @@ -42,10 +42,7 @@
>  
>  #define SD_EMMC_CLOCK 0x0
>  #define   CLK_DIV_MASK GENMASK(5, 0)
> -#define   CLK_DIV_MAX 63
>  #define   CLK_SRC_MASK GENMASK(7, 6)
> -#define   CLK_SRC_XTAL 0   /* external crystal */
> -#define   CLK_SRC_PLL 1    /* FCLK_DIV2 */
>  #define   CLK_CORE_PHASE_MASK GENMASK(9, 8)
>  #define   CLK_TX_PHASE_MASK GENMASK(11, 10)
>  #define   CLK_RX_PHASE_MASK GENMASK(13, 12)
> @@ -137,13 +134,9 @@ struct meson_host {
>  	spinlock_t lock;
>  	void __iomem *regs;
>  	struct clk *core_clk;
> -	struct clk_mux mux;
> -	struct clk *mux_clk;
> +	struct clk *signal_clk;
>  	unsigned long req_rate;
>  
> -	struct clk_divider cfg_div;
> -	struct clk *cfg_div_clk;
> -
>  	unsigned int bounce_buf_size;
>  	void *bounce_buf;
>  	dma_addr_t bounce_dma_addr;
> @@ -291,7 +284,7 @@ static int meson_mmc_clk_set(struct meson_host *host, unsigned long clk_rate)
>  		return 0;
>  	}
>  
> -	ret = clk_set_rate(host->cfg_div_clk, clk_rate);
> +	ret = clk_set_rate(host->signal_clk, clk_rate);

minor nit: where does the name "signal" come from?  I called this
"div_clk" because it's the output of the divider right before the
sd/emmc IP block.   Admittedly, that's not a great name either, and I'm
not too picky about the naming, just curious...

Looking at the diagram we have since I initially wrote the driver, this
is more commonly referred to as device_clk.

Anyways, if you're going to rename...

[...]

>  static void meson_mmc_set_tuning_params(struct mmc_host *mmc)
> @@ -987,7 +984,7 @@ static int meson_mmc_probe(struct platform_device *pdev)
>  	dma_free_coherent(host->dev, host->bounce_buf_size,
>  			  host->bounce_buf, host->bounce_dma_addr);
>  err_div_clk:

... probably should rename this too.

Otherwise,

Reviewed-by: Kevin Hilman <khilman@xxxxxxxxxxxx>

Kevin