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

[Sean Anderson]

On 11/28/21 6:32 AM, Amit Singh Tomar wrote:
> 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 v3:
> 	* No change.
> Changes since v2:
> 	* Fixed the weird div assignment.
> 	* Moved the clock bit for SD from header file
> 	  to driver file.
> 	* Removed "< 0" part while comparing unsigned.
> 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 | 73 +++++++++++++++++++++++++++++++++++++++
>   1 file changed, 73 insertions(+)
> 
> diff --git a/drivers/clk/owl/clk_owl.c b/drivers/clk/owl/clk_owl.c
> index f78e5fa3f08d..678fdd5a4540 100644
> --- a/drivers/clk/owl/clk_owl.c
> +++ b/drivers/clk/owl/clk_owl.c
> @@ -20,6 +20,8 @@
>   #include <linux/bitops.h>
>   #include <linux/delay.h>
>   
> +#define CMU_DEVCLKEN0_SD0	BIT(22)

Please define this in arch/arm/include/asm/arch-owl/regs_s[79]00.h like CMU_DEVCLKEN1_ETH.

>   void owl_clk_init(struct owl_clk_priv *priv)
>   {
>   	u32 bus_clk = 0, core_pll, dev_pll;
> @@ -92,6 +94,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 +126,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 +136,72 @@ 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)

Please add a define for this bit

> +		rate = readl(priv->base + CMU_NANDPLL) & 0x7f;

ditto for this mask

> +	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, val;
> +	ulong parent_rate = get_sd_parent_rate(priv, sd_index);
> +
> +	val = readl(priv->base + (CMU_SD0CLK + sd_index * 0x4));
> +	div = (val & 0x1f) + 1;

ditto

> +
> +	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)
> +		return rate;
> +
> +	div = (parent_rate / rate);

no parentheses needed here

> +
> +	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
> +	 */

The comment is good, but please use defines. Additionally, we are not
in the net subsystem, so it should be formatted like

	/*
	 * 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 */
> +	}
> +	val |= ((div - 1) & 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 +211,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;
>   	}
>