On 19/12/2020 14:51, Amit Singh Tomar wrote:
Hi,
> From: Amit Singh Tomar <amittome...@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 <amittome...@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
