[PATCH 06/42] mmc: dw_mmc: Extract clock on/off code into a separate routine

[Quentin Schulz]

Hi Sam,

On 5/23/24 1:30 AM, Sam Protsenko wrote:
> Extract clock control code into a separate routine to avoid code
> duplication in dwmci_setup_bus().
> 
> No functional change.
> 

There are some differences though.

> Signed-off-by: Sam Protsenko <semen.protsenko at linaro.org>
> ---
>   drivers/mmc/dw_mmc.c | 60 ++++++++++++++++++++++++--------------------
>   1 file changed, 33 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c
> index cbfb8d3b8683..40b9b034f793 100644
> --- a/drivers/mmc/dw_mmc.c
> +++ b/drivers/mmc/dw_mmc.c
> @@ -412,11 +412,33 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
>   	return ret;
>   }
>   
> -static int dwmci_setup_bus(struct dwmci_host *host, u32 freq)
> +static int dwmci_control_clken(struct dwmci_host *host, bool on)
>   {
> -	u32 div, status;
> +	const u32 val = on ? DWMCI_CLKEN_ENABLE | DWMCI_CLKEN_LOW_PWR : 0;
> +	const u32 cmd_only_clk = DWMCI_CMD_PRV_DAT_WAIT | DWMCI_CMD_UPD_CLK;
>   	int timeout = 10000;
> +	u32 status;
> +
> +	dwmci_writel(host, DWMCI_CLKENA, val);
> +
> +	/* Inform CIU */
> +	dwmci_writel(host, DWMCI_CMD, DWMCI_CMD_START | cmd_only_clk);
> +	do {
> +		status = dwmci_readl(host, DWMCI_CMD);
> +		if (timeout-- < 0) {
> +			debug("%s: Timeout!\n", __func__);
> +			return -ETIMEDOUT;
> +		}
> +	} while (status & DWMCI_CMD_START);
> +
> +	return 0;
> +}
> +
> +static int dwmci_setup_bus(struct dwmci_host *host, u32 freq)
> +{
> +	u32 div;
>   	unsigned long sclk;
> +	int ret;
>   
>   	if ((freq == host->clock) || (freq == 0))
>   		return 0;
> @@ -439,35 +461,19 @@ static int dwmci_setup_bus(struct dwmci_host *host, u32 freq)
>   	else
>   		div = DIV_ROUND_UP(sclk, 2 * freq);
>   
> -	dwmci_writel(host, DWMCI_CLKENA, 0);
> +	/* Disable clock */
>   	dwmci_writel(host, DWMCI_CLKSRC, 0);
> +	ret = dwmci_control_clken(host, false);

Here, CLKENA and CLKSRC are swapped. The kernel still calls CLKENA 
before CLKSRC, is this really safe? (I have no documentation so cannot 
tell). E.g., we could have CLKSRC register be a way to select the clock 
parent/source, 0 could be 24MHz crystal for example, so switching to 
this new parent before disabling the clock output would likely result in 
glitches?

> +	if (ret)
> +		return ret;
>   
> +	/* Set clock to desired speed */
>   	dwmci_writel(host, DWMCI_CLKDIV, div);

Same here, CLKDIV is set now after the CIU is informed, is this an 
issue? We may want to set the clock speed before we enable the clock 
again. Right now it's setting the desired speed while disabled, inform 
the CIU, enable the clock, inform the CIU. This now does, disable the 
clock, inform the CIU, set the desired speed, enable the clock, inform 
the CIU. We may need to wait for the clock to stabilize before enabling 
it? Again, just making guesses, no documentation for me here :/

> -	dwmci_writel(host, DWMCI_CMD, DWMCI_CMD_PRV_DAT_WAIT |
> -			DWMCI_CMD_UPD_CLK | DWMCI_CMD_START);
>   
> -	do {
> -		status = dwmci_readl(host, DWMCI_CMD);
> -		if (timeout-- < 0) {
> -			debug("%s: Timeout!\n", __func__);
> -			return -ETIMEDOUT;
> -		}
> -	} while (status & DWMCI_CMD_START);
> -
> -	dwmci_writel(host, DWMCI_CLKENA, DWMCI_CLKEN_ENABLE |
> -			DWMCI_CLKEN_LOW_PWR);
> -
> -	dwmci_writel(host, DWMCI_CMD, DWMCI_CMD_PRV_DAT_WAIT |
> -			DWMCI_CMD_UPD_CLK | DWMCI_CMD_START);
> -
> -	timeout = 10000;
> -	do {
> -		status = dwmci_readl(host, DWMCI_CMD);
> -		if (timeout-- < 0) {
> -			debug("%s: Timeout!\n", __func__);
> -			return -ETIMEDOUT;
> -		}
> -	} while (status & DWMCI_CMD_START);
> +	/* Enable clock */
> +	ret = dwmci_control_clken(host, true);
> +	if (ret)
> +		return ret;
>   
>   	host->clock = freq;
>   
Cheers,
Quentin