[PATCH v2 2/6] clk: actions: Add SD/MMC clocks

[André Przywara]

On 19/12/2020 14:51, Amit Singh Tomar wrote:

Hi,

> From: Amit Singh Tomar <amittomer25 at gmail.com>
> 
> This commit adds SD/MMC clocks, and provides .set/get_rate callbacks
> for SD/MMC device present on Actions OWL S700 SoCs.
> 
> Signed-off-by: Amit Singh Tomar <amittomer25 at gmail.com>
> ---
> Changes since previous version:
> 	* Removed rate *= 2 as this just overclocks.
> 	* Separated the divide by 128 bit from divider value.
> 	* Provided the separate routine to get sd parent rate
> 	  based on bit 9.  	
> 	* Removed unnecessary initialization.
> ---
>  drivers/clk/owl/clk_owl.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/owl/clk_owl.h |  2 ++
>  2 files changed, 74 insertions(+)
> 
> diff --git a/drivers/clk/owl/clk_owl.c b/drivers/clk/owl/clk_owl.c
> index 5be1b3b..cac8e6e 100644
> --- a/drivers/clk/owl/clk_owl.c
> +++ b/drivers/clk/owl/clk_owl.c
> @@ -92,6 +92,9 @@ int owl_clk_enable(struct clk *clk)
>  		setbits_le32(priv->base + CMU_DEVCLKEN1, CMU_DEVCLKEN1_ETH);
>  		setbits_le32(priv->base + CMU_ETHERNETPLL, 5);
>  		break;
> +	case CLK_SD0:
> +		setbits_le32(priv->base + CMU_DEVCLKEN0, CMU_DEVCLKEN0_SD0);
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -121,6 +124,9 @@ int owl_clk_disable(struct clk *clk)
>  	case CLK_ETHERNET:
>  		clrbits_le32(priv->base + CMU_DEVCLKEN1, CMU_DEVCLKEN1_ETH);
>  		break;
> +	case CLK_SD0:
> +		clrbits_le32(priv->base + CMU_DEVCLKEN0, CMU_DEVCLKEN0_SD0);
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -128,11 +134,73 @@ int owl_clk_disable(struct clk *clk)
>  	return 0;
>  }
>  
> +static ulong get_sd_parent_rate(struct owl_clk_priv *priv, u32 dev_index)
> +{
> +	ulong rate;
> +	u32 reg;
> +
> +	reg = readl(priv->base + (CMU_SD0CLK + dev_index * 0x4));
> +	/* Clock output of DEV/NAND_PLL
> +	 * Range: 48M ~ 756M
> +	 * Frequency= PLLCLK * 6
> +	 */
> +	if (reg & 0x200)
> +		rate = readl(priv->base + CMU_NANDPLL) & 0x7f;
> +	else
> +		rate = readl(priv->base + CMU_DEVPLL) & 0x7f;
> +
> +	rate *= 6000000;
> +
> +	return rate;
> +}
> +
> +static ulong owl_get_sd_clk_rate(struct owl_clk_priv *priv, int sd_index)
> +{
> +	uint div;
> +	ulong parent_rate = get_sd_parent_rate(priv, sd_index);
> +
> +	div = readl(priv->base + (CMU_SD0CLK + sd_index * 0x4)) & 0x1f;
> +	div++;

That looks weird. Either add it directly above or below.

> +
> +	return (parent_rate / div);
> +}
> +
> +static ulong owl_set_sd_clk_rate(struct owl_clk_priv *priv, ulong rate,
> +				 int sd_index)
> +{
> +	uint div, val;
> +	ulong parent_rate = get_sd_parent_rate(priv, sd_index);
> +
> +	if (rate <= 0)

rate is unsigned, so the "< 0" part is not needed.

> +		return rate;
> +
> +	div = (parent_rate / rate);
> +
> +	val = readl(priv->base + (CMU_SD0CLK + sd_index * 0x4));
> +	/* Bits 4..0 is used to program div value and bit 8 to enable
> +	 * divide by 128 circuit
> +	 */
> +	val &= ~0x11f;
> +	if (div >= 128) {
> +		div = div / 128;
> +		val |= 0x100; /* enable divide by 128 circuit */
> +	}
> +	div--;

That looks weird. It's a encoding thing, so just subtract the "1"
directly below.

> +	val |= (div & 0x1f);
> +	writel(val, priv->base + (CMU_SD0CLK + sd_index * 0x4));
> +
> +	return owl_get_sd_clk_rate(priv, 0);
> +}
> +
>  static ulong owl_clk_get_rate(struct clk *clk)
>  {
> +	struct owl_clk_priv *priv = dev_get_priv(clk->dev);
>  	ulong rate;
>  
>  	switch (clk->id) {
> +	case CLK_SD0:
> +		rate = owl_get_sd_clk_rate(priv, 0);
> +		break;
>  	default:
>  		return -ENOENT;
>  	}
> @@ -142,9 +210,13 @@ static ulong owl_clk_get_rate(struct clk *clk)
>  
>  static ulong owl_clk_set_rate(struct clk *clk, ulong rate)
>  {
> +	struct owl_clk_priv *priv = dev_get_priv(clk->dev);
>  	ulong new_rate;
>  
>  	switch (clk->id) {
> +	case CLK_SD0:
> +		new_rate = owl_set_sd_clk_rate(priv, rate, 0);
> +		break;
>  	default:
>  		return -ENOENT;
>  	}
> diff --git a/drivers/clk/owl/clk_owl.h b/drivers/clk/owl/clk_owl.h
> index a01f81a..ee5eba4 100644
> --- a/drivers/clk/owl/clk_owl.h
> +++ b/drivers/clk/owl/clk_owl.h
> @@ -62,4 +62,6 @@ struct owl_clk_priv {
>  #define CMU_DEVCLKEN1_UART5	BIT(21)
>  #define CMU_DEVCLKEN1_UART3	BIT(11)
>  
> +#define CMU_DEVCLKEN0_SD0	BIT(22)
> +

Why do we actually have those values in a header file? The driver is
supposed to abstract from them, so there should be no need to have those
values shared in an include file? I guess clk_owl.c is the only user?

Cheers,
Andre