[PATCH linux-next v3 6/7] ASoC: rsnd: add avb clocks

[PATCH linux-next v3 6/7] ASoC: rsnd: add avb clocks

[Jiada Wang]

There are AVB Counter Clocks in ADG, each clock has 12bits integral
and 8 bits fractional dividers which operates with S0D1ϕ clock.

This patch registers 8 AVB Counter Clocks when clock-cells of
rcar_sound node is 2,

Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
---
 sound/soc/sh/rcar/adg.c  | 316 +++++++++++++++++++++++++++++++++++++--
 sound/soc/sh/rcar/gen.c  |   9 ++
 sound/soc/sh/rcar/rsnd.h |   9 ++
 3 files changed, 325 insertions(+), 9 deletions(-)

diff --git a/sound/soc/sh/rcar/adg.c b/sound/soc/sh/rcar/adg.c
index 6768a66588eb..0708be1f38d9 100644
--- a/sound/soc/sh/rcar/adg.c
+++ b/sound/soc/sh/rcar/adg.c
@@ -5,6 +5,7 @@
 //  Copyright (C) 2013  Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
 
 #include <linux/clk-provider.h>
+#include <linux/of_address.h>
 #include "rsnd.h"
 
 #define CLKA	0
@@ -21,13 +22,30 @@
 
 #define BRRx_MASK(x) (0x3FF & x)
 
+#define AVB_CLK_NUM		8
+#define AVB_MAX_RATE		25000000
+#define AVB_DIV_EN_COM		BIT(31)
+#define AVB_MAX_DIV		0x3ffc0
+
 static struct rsnd_mod_ops adg_ops = {
 	.name = "adg",
 };
 
+struct clk_avb {
+	struct clk_hw hw;
+	unsigned int idx;
+	struct rsnd_adg *adg;
+	/* lock reg access */
+	spinlock_t *lock;
+};
+
+#define hw_to_avb(_hw) container_of(_hw, struct clk_avb, hw)
+
 struct rsnd_adg {
 	struct clk *clk[CLKMAX];
 	struct clk *clkout[CLKOUTMAX];
+	struct clk *clkavb[AVB_CLK_NUM];
+	struct clk *clkadg;
 	struct clk_onecell_data onecell;
 	struct rsnd_mod mod;
 	u32 flags;
@@ -37,6 +55,7 @@ struct rsnd_adg {
 
 	int rbga_rate_for_441khz; /* RBGA */
 	int rbgb_rate_for_48khz;  /* RBGB */
+	spinlock_t avb_lock;
 };
 
 #define LRCLK_ASYNC	(1 << 0)
@@ -408,6 +427,248 @@ void rsnd_adg_clk_control(struct rsnd_priv *priv, int enable)
 	}
 }
 
+static struct clk *rsnd_adg_clk_src_twocell_get(struct of_phandle_args *clkspec,
+						void *data)
+{
+	unsigned int clktype = clkspec->args[0];
+	unsigned int clkidx = clkspec->args[1];
+	struct rsnd_adg *adg = data;
+	struct rsnd_mod *adg_mod = rsnd_mod_get(adg);
+	struct rsnd_priv *priv = rsnd_mod_to_priv(adg_mod);
+	struct device *dev = rsnd_priv_to_dev(priv);
+	struct clk *clk;
+
+	switch (clktype) {
+	case 0:
+		if (clkidx >= CLKOUTMAX) {
+			dev_err(dev, "Invalid fixed clock index %u\n", clkidx);
+			return ERR_PTR(-EINVAL);
+		}
+		clk = adg->clkout[clkidx];
+		break;
+	case 1:
+		if (clkidx >= AVB_CLK_NUM) {
+			dev_err(dev, "Invalid avb clock index %u\n", clkidx);
+			return ERR_PTR(-EINVAL);
+		}
+		clk = adg->clkavb[clkidx];
+		break;
+	default:
+		dev_err(dev, "Invalid ADG clock type %u\n", clktype);
+		return ERR_PTR(-EINVAL);
+	}
+
+	return clk;
+}
+
+static void clk_avb_div_write(struct rsnd_adg *adg, u32 data, unsigned int idx)
+{
+	struct rsnd_mod *mod = rsnd_mod_get(adg);
+
+	switch (idx) {
+	case 0:
+		rsnd_mod_write(mod, AVB_CLK_DIV0, data);
+		break;
+	case 1:
+		rsnd_mod_write(mod, AVB_CLK_DIV1, data);
+		break;
+	case 2:
+		rsnd_mod_write(mod, AVB_CLK_DIV2, data);
+		break;
+	case 3:
+		rsnd_mod_write(mod, AVB_CLK_DIV3, data);
+		break;
+	case 4:
+		rsnd_mod_write(mod, AVB_CLK_DIV4, data);
+		break;
+	case 5:
+		rsnd_mod_write(mod, AVB_CLK_DIV5, data);
+		break;
+	case 6:
+		rsnd_mod_write(mod, AVB_CLK_DIV6, data);
+		break;
+	case 7:
+		rsnd_mod_write(mod, AVB_CLK_DIV7, data);
+		break;
+	}
+}
+
+static u32 clk_avb_div_read(struct rsnd_adg *adg, unsigned int idx)
+{
+	struct rsnd_mod *mod = rsnd_mod_get(adg);
+	u32 val = 0;
+
+	switch (idx) {
+	case 0:
+		val = rsnd_mod_read(mod, AVB_CLK_DIV0);
+		break;
+	case 1:
+		val = rsnd_mod_read(mod, AVB_CLK_DIV1);
+		break;
+	case 2:
+		val = rsnd_mod_read(mod, AVB_CLK_DIV2);
+		break;
+	case 3:
+		val = rsnd_mod_read(mod, AVB_CLK_DIV3);
+		break;
+	case 4:
+		val = rsnd_mod_read(mod, AVB_CLK_DIV4);
+		break;
+	case 5:
+		val = rsnd_mod_read(mod, AVB_CLK_DIV5);
+		break;
+	case 6:
+		val = rsnd_mod_read(mod, AVB_CLK_DIV6);
+		break;
+	case 7:
+		val = rsnd_mod_read(mod, AVB_CLK_DIV7);
+		break;
+	}
+
+	return val;
+}
+
+static int clk_avb_is_enabled(struct clk_hw *hw)
+{
+	struct clk_avb *avb = hw_to_avb(hw);
+	struct rsnd_mod *mod = rsnd_mod_get(avb->adg);
+
+	return rsnd_mod_read(mod, AVB_CLK_CONFIG) & BIT(avb->idx);
+}
+
+static int clk_avb_enabledisable(struct clk_hw *hw, int enable)
+{
+	struct clk_avb *avb = hw_to_avb(hw);
+	u32 val;
+	struct rsnd_mod *mod = rsnd_mod_get(avb->adg);
+
+	spin_lock(avb->lock);
+
+	val = rsnd_mod_read(mod, AVB_CLK_CONFIG);
+	if (enable)
+		val |= BIT(avb->idx);
+	else
+		val &= ~BIT(avb->idx);
+	rsnd_mod_write(mod, AVB_CLK_CONFIG,  val);
+
+	spin_unlock(avb->lock);
+
+	return 0;
+}
+
+static int clk_avb_enable(struct clk_hw *hw)
+{
+	return clk_avb_enabledisable(hw, 1);
+}
+
+static void clk_avb_disable(struct clk_hw *hw)
+{
+	clk_avb_enabledisable(hw, 0);
+}
+
+static unsigned long clk_avb_recalc_rate(struct clk_hw *hw,
+					 unsigned long parent_rate)
+{
+	struct clk_avb *avb = hw_to_avb(hw);
+	u32 div;
+
+	div = clk_avb_div_read(avb->adg, avb->idx);
+	if (!div)
+		return parent_rate;
+
+	return parent_rate * 32 / div;
+}
+
+static unsigned int clk_avb_calc_div(unsigned long rate,
+				     unsigned long parent_rate)
+{
+	unsigned int div;
+
+	if (!rate)
+		rate = 1;
+
+	if (rate > AVB_MAX_RATE)
+		rate = AVB_MAX_RATE;
+
+	div = DIV_ROUND_CLOSEST(parent_rate * 32, rate);
+
+	if (div > AVB_MAX_DIV)
+		div = AVB_MAX_DIV;
+
+	return div;
+}
+
+static long clk_avb_round_rate(struct clk_hw *hw, unsigned long rate,
+			       unsigned long *parent_rate)
+{
+	unsigned int div = clk_avb_calc_div(rate, *parent_rate);
+
+	return (*parent_rate * 32) / div;
+}
+
+static int clk_avb_set_rate(struct clk_hw *hw, unsigned long rate,
+			    unsigned long parent_rate)
+{
+	struct clk_avb *avb = hw_to_avb(hw);
+	unsigned int div = clk_avb_calc_div(rate, parent_rate);
+	u32 val;
+
+	val = clk_avb_div_read(avb->adg, avb->idx);
+	clk_avb_div_write(avb->adg, val | div, avb->idx);
+
+	return 0;
+}
+
+static const struct clk_ops clk_avb_ops = {
+	.enable =	clk_avb_enable,
+	.disable =	clk_avb_disable,
+	.is_enabled =	clk_avb_is_enabled,
+	.recalc_rate =	clk_avb_recalc_rate,
+	.round_rate =	clk_avb_round_rate,
+	.set_rate =	clk_avb_set_rate,
+};
+
+static struct clk *clk_register_avb(struct device *dev, struct rsnd_adg *adg,
+				    unsigned int id, spinlock_t *lock)
+{
+	struct clk_init_data init;
+	struct clk_avb *avb;
+	struct clk *clk;
+	char name[8];
+	const char *parent_name;
+
+	if (IS_ERR(adg->clkadg))
+		return ERR_PTR(-ENODEV);
+
+	avb = devm_kzalloc(dev, sizeof(*avb), GFP_KERNEL);
+	if (!avb)
+		return ERR_PTR(-ENOMEM);
+
+	parent_name = __clk_get_name(adg->clkadg);
+
+	snprintf(name, sizeof(name), "%s%u", "avb", id);
+
+	avb->idx = id;
+	avb->lock = lock;
+	avb->adg = adg;
+
+	/* Register the clock. */
+	init.name = name;
+	init.ops = &clk_avb_ops;
+	init.flags = CLK_IS_BASIC;
+	init.parent_names = &parent_name;
+	init.num_parents = 1;
+
+	avb->hw.init = &init;
+
+	/* init DIV to a valid state */
+	clk_avb_div_write(avb->adg, AVB_MAX_DIV, avb->idx);
+
+	clk = devm_clk_register(dev, &avb->hw);
+
+	return clk;
+}
+
 static void rsnd_adg_get_clkin(struct rsnd_priv *priv,
 			       struct rsnd_adg *adg)
 {
@@ -436,6 +697,7 @@ static void rsnd_adg_get_clkout(struct rsnd_priv *priv,
 	unsigned long req_48kHz_rate, req_441kHz_rate;
 	int i, req_size;
 	const char *parent_clk_name = NULL;
+	struct rsnd_mod *mod = rsnd_mod_get(adg);
 	static const char * const clkout_name[] = {
 		[CLKOUT]  = "audio_clkout",
 		[CLKOUT1] = "audio_clkout1",
@@ -540,21 +802,23 @@ static void rsnd_adg_get_clkout(struct rsnd_priv *priv,
 	 */
 
 	of_property_read_u32(np, "#clock-cells", &count);
-	/*
-	 * for clkout
-	 */
-	if (!count) {
+
+	switch (count) {
+	case 0:
+		/*
+		 * for clkout
+		 */
 		clk = clk_register_fixed_rate(dev, clkout_name[CLKOUT],
 					      parent_clk_name, 0, req_rate[0]);
 		if (!IS_ERR(clk)) {
 			adg->clkout[CLKOUT] = clk;
 			of_clk_add_provider(np, of_clk_src_simple_get, clk);
 		}
-	}
-	/*
-	 * for clkout0/1/2/3
-	 */
-	else {
+		break;
+	case 1:
+		/*
+		 * for clkout0/1/2/3
+		 */
 		for (i = 0; i < CLKOUTMAX; i++) {
 			clk = clk_register_fixed_rate(dev, clkout_name[i],
 						      parent_clk_name, 0,
@@ -566,6 +830,33 @@ static void rsnd_adg_get_clkout(struct rsnd_priv *priv,
 		adg->onecell.clk_num	= CLKOUTMAX;
 		of_clk_add_provider(np, of_clk_src_onecell_get,
 				    &adg->onecell);
+		break;
+	case 2:
+		/*
+		 * for clkout0/1/2/3 and avb clocks
+		 */
+		for (i = 0; i < CLKOUTMAX; i++) {
+			clk = clk_register_fixed_rate(dev, clkout_name[i],
+						      parent_clk_name, 0,
+						      req_rate[0]);
+			if (!IS_ERR(clk))
+				adg->clkout[i] = clk;
+		}
+
+		for (i = 0; i < AVB_CLK_NUM; i++) {
+			clk = clk_register_avb(dev, adg, i, &adg->avb_lock);
+			if (!IS_ERR(clk))
+				adg->clkavb[i] = clk;
+		}
+
+		of_clk_add_provider(np, rsnd_adg_clk_src_twocell_get, adg);
+
+		rsnd_mod_write(mod, AVB_CLK_CONFIG, AVB_DIV_EN_COM);
+
+		break;
+	default:
+		dev_err(dev, "Invalid clock-cell %d\n", count);
+		break;
 	}
 
 rsnd_adg_get_clkout_end:
@@ -606,17 +897,24 @@ int rsnd_adg_probe(struct rsnd_priv *priv)
 {
 	struct rsnd_adg *adg;
 	struct device *dev = rsnd_priv_to_dev(priv);
+	struct clk *clk;
 	int ret;
 
 	adg = devm_kzalloc(dev, sizeof(*adg), GFP_KERNEL);
 	if (!adg)
 		return -ENOMEM;
 
+	spin_lock_init(&adg->avb_lock);
+
 	ret = rsnd_mod_init(priv, &adg->mod, &adg_ops,
 		      NULL, 0, 0);
 	if (ret)
 		return ret;
 
+	clk = devm_clk_get(dev, "adg");
+	if (!IS_ERR(clk))
+		adg->clkadg = clk;
+
 	rsnd_adg_get_clkin(priv, adg);
 	rsnd_adg_get_clkout(priv, adg);
 	rsnd_adg_clk_dbg_info(priv, adg);
diff --git a/sound/soc/sh/rcar/gen.c b/sound/soc/sh/rcar/gen.c
index ca639404f2cd..1b000d03b76e 100644
--- a/sound/soc/sh/rcar/gen.c
+++ b/sound/soc/sh/rcar/gen.c
@@ -355,6 +355,15 @@ static int rsnd_gen2_probe(struct rsnd_priv *priv)
 		RSND_GEN_S_REG(SRCOUT_TIMSEL3,	0x54),
 		RSND_GEN_S_REG(SRCOUT_TIMSEL4,	0x58),
 		RSND_GEN_S_REG(CMDOUT_TIMSEL,	0x5c),
+		RSND_GEN_S_REG(AVB_CLK_DIV0,	0x11c),
+		RSND_GEN_S_REG(AVB_CLK_DIV1,	0x120),
+		RSND_GEN_S_REG(AVB_CLK_DIV2,	0x124),
+		RSND_GEN_S_REG(AVB_CLK_DIV3,	0x128),
+		RSND_GEN_S_REG(AVB_CLK_DIV4,	0x12c),
+		RSND_GEN_S_REG(AVB_CLK_DIV5,	0x130),
+		RSND_GEN_S_REG(AVB_CLK_DIV6,	0x134),
+		RSND_GEN_S_REG(AVB_CLK_DIV7,	0x138),
+		RSND_GEN_S_REG(AVB_CLK_CONFIG,	0x13c),
 	};
 	static const struct rsnd_regmap_field_conf conf_ssi[] = {
 		RSND_GEN_M_REG(SSICR,		0x00,	0x40),
diff --git a/sound/soc/sh/rcar/rsnd.h b/sound/soc/sh/rcar/rsnd.h
index 3c57129af6d1..d31b8a65985f 100644
--- a/sound/soc/sh/rcar/rsnd.h
+++ b/sound/soc/sh/rcar/rsnd.h
@@ -148,6 +148,15 @@ enum rsnd_reg {
 	RSND_REG_AUDIO_CLK_SEL0,
 	RSND_REG_AUDIO_CLK_SEL1,
 	RSND_REG_AUDIO_CLK_SEL2,
+	RSND_REG_AVB_CLK_DIV0,
+	RSND_REG_AVB_CLK_DIV1,
+	RSND_REG_AVB_CLK_DIV2,
+	RSND_REG_AVB_CLK_DIV3,
+	RSND_REG_AVB_CLK_DIV4,
+	RSND_REG_AVB_CLK_DIV5,
+	RSND_REG_AVB_CLK_DIV6,
+	RSND_REG_AVB_CLK_DIV7,
+	RSND_REG_AVB_CLK_CONFIG,
 
 	/* SSIU */
 	RSND_REG_SSI_MODE,
-- 
2.19.2

Re: [PATCH linux-next v3 6/7] ASoC: rsnd: add avb clocks

[Kuninori Morimoto]

Hi Jiada

> There are AVB Counter Clocks in ADG, each clock has 12bits integral
> and 8 bits fractional dividers which operates with S0D1ϕ clock.
> 
> This patch registers 8 AVB Counter Clocks when clock-cells of
> rcar_sound node is 2,
> 
> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
> ---


> +static void clk_avb_div_write(struct rsnd_adg *adg, u32 data, unsigned int idx)
(snip)
> +static u32 clk_avb_div_read(struct rsnd_adg *adg, unsigned int idx)

As I mentioned before, I think we can avoid confusable parameter by

	static void clk_avb_div_write(struct clk_avb *avb, u32 data)
	static u32 clk_avb_div_read(struct clk_avb)	

	div = clk_avb_div_read(avb);
	clk_avb_div_write(avb, val | div);

> +static struct clk *clk_register_avb(struct device *dev, struct rsnd_adg *adg,
> +				    unsigned int id, spinlock_t *lock)
> +{
> +	struct clk_init_data init;
> +	struct clk_avb *avb;
> +	struct clk *clk;
> +	char name[8];
> +	const char *parent_name;
> +
> +	if (IS_ERR(adg->clkadg))
> +		return ERR_PTR(-ENODEV);

I think adg->clkadg will never hit to IS_ERR().
It will have NULL or correct pointer.

	clk = devm_clk_get(dev, "adg");
	if (!IS_ERR(clk))
		adg->clkadg = clk;

And this "adg" clock is strange.
see below

> +	avb = devm_kzalloc(dev, sizeof(*avb), GFP_KERNEL);
> +	if (!avb)
> +		return ERR_PTR(-ENOMEM);
> +
> +	parent_name = __clk_get_name(adg->clkadg);

This parent_name is very strange to me.
AVB parent clk is "AUDIO_CLK_A/B/C/I" (= clk_a/b/c/i in this driver)
or "AUDIO_CLK_OUT_A/B/C/D" (= audio_clkout/1/2/3 in this driver).
And we don't have "adg" clock.
Please double check it.

Best regards
---
Kuninori Morimoto

Re: [PATCH linux-next v3 6/7] ASoC: rsnd: add avb clocks

[Jiada Wang]

Hi Morimoto-san

Thanks for your comments

On 2018/12/05 17:58, Kuninori Morimoto wrote:
> Hi Jiada
>
>> There are AVB Counter Clocks in ADG, each clock has 12bits integral
>> and 8 bits fractional dividers which operates with S0D1ϕ clock.
>>
>> This patch registers 8 AVB Counter Clocks when clock-cells of
>> rcar_sound node is 2,
>>
>> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
>> ---
>
>> +static void clk_avb_div_write(struct rsnd_adg *adg, u32 data, unsigned int idx)
> (snip)
>> +static u32 clk_avb_div_read(struct rsnd_adg *adg, unsigned int idx)
> As I mentioned before, I think we can avoid confusable parameter by
>
> 	static void clk_avb_div_write(struct clk_avb *avb, u32 data)
> 	static u32 clk_avb_div_read(struct clk_avb)	
>
> 	div = clk_avb_div_read(avb);
> 	clk_avb_div_write(avb, val | div);
right, I forgot to update this, will address it in next version
>> +static struct clk *clk_register_avb(struct device *dev, struct rsnd_adg *adg,
>> +				    unsigned int id, spinlock_t *lock)
>> +{
>> +	struct clk_init_data init;
>> +	struct clk_avb *avb;
>> +	struct clk *clk;
>> +	char name[8];
>> +	const char *parent_name;
>> +
>> +	if (IS_ERR(adg->clkadg))
>> +		return ERR_PTR(-ENODEV);
> I think adg->clkadg will never hit to IS_ERR().
> It will have NULL or correct pointer.
right, will update this in next version
> 	clk = devm_clk_get(dev, "adg");
> 	if (!IS_ERR(clk))
> 		adg->clkadg = clk;
>
> And this "adg" clock is strange.
> see below
>
>> +	avb = devm_kzalloc(dev, sizeof(*avb), GFP_KERNEL);
>> +	if (!avb)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	parent_name = __clk_get_name(adg->clkadg);
> This parent_name is very strange to me.
> AVB parent clk is "AUDIO_CLK_A/B/C/I" (= clk_a/b/c/i in this driver)
> or "AUDIO_CLK_OUT_A/B/C/D" (= audio_clkout/1/2/3 in this driver).
> And we don't have "adg" clock.
> Please double check it.
I have some local device-tree change, which expends 'adg' register range 
and add
"adg" clock to rcar_sound node which refer to newly added 'adg' clock 
(S0D1ϕ) in this patch-set

the change looks like the following

                         compatible =  "renesas,rcar_sound-r8a7795", 
"renesas,rcar_sound-gen3";
                         reg =   <0 0xec500000 0 0x1000>, /* SCU */
-                               <0 0xec5a0000 0 0x100>,  /* ADG */
+                               <0 0xec5a0000 0 0x140>,  /* ADG */
                                 <0 0xec540000 0 0x1000>, /* SSIU */
                                 <0 0xec541000 0 0x280>,  /* SSI */
                                 <0 0xec740000 0 0x200>;  /* Audio DMAC 
peri peri*/
                         reg-names = "scu", "adg", "ssiu", "ssi", "audmapp";

-                       clocks = <&cpg CPG_MOD 1005>,
+                       clocks = <&cpg CPG_MOD 922>, <&cpg CPG_MOD 1005>,
                                  <&cpg CPG_MOD 1006>, <&cpg CPG_MOD 1007>,
                                  <&cpg CPG_MOD 1008>, <&cpg CPG_MOD 1009>,
                                  <&cpg CPG_MOD 1010>, <&cpg CPG_MOD 1011>,
@@ -1856,7 +1856,7 @@
                                  <&audio_clk_a>, <&audio_clk_b>,
                                  <&audio_clk_c>,
                                  <&cpg CPG_CORE R8A7795_CLK_S0D4>;
-                       clock-names = "ssi-all",
+                       clock-names = "adg", "ssi-all",
                                       "ssi.9", "ssi.8", "ssi.7", "ssi.6",
                                       "ssi.5", "ssi.4", "ssi.3", "ssi.2",
                                       "ssi.1", "ssi.0",

Thanks,
Jiada

> Best regards
> ---
> Kuninori Morimoto

Re: [PATCH linux-next v3 6/7] ASoC: rsnd: add avb clocks

[Kuninori Morimoto]

Hi Jiada

> >> +	avb = devm_kzalloc(dev, sizeof(*avb), GFP_KERNEL);
> >> +	if (!avb)
> >> +		return ERR_PTR(-ENOMEM);
> >> +
> >> +	parent_name = __clk_get_name(adg->clkadg);
> > This parent_name is very strange to me.
> > AVB parent clk is "AUDIO_CLK_A/B/C/I" (= clk_a/b/c/i in this driver)
> > or "AUDIO_CLK_OUT_A/B/C/D" (= audio_clkout/1/2/3 in this driver).
> > And we don't have "adg" clock.
> > Please double check it.
> I have some local device-tree change, which expends 'adg' register
> range and add
> "adg" clock to rcar_sound node which refer to newly added 'adg' clock
> (S0D1ϕ) in this patch-set
(snip)
> -                       clocks = <&cpg CPG_MOD 1005>,
> +                       clocks = <&cpg CPG_MOD 922>, <&cpg CPG_MOD 1005>,
>                                  <&cpg CPG_MOD 1006>, <&cpg CPG_MOD 1007>,
>                                  <&cpg CPG_MOD 1008>, <&cpg CPG_MOD 1009>,
>                                  <&cpg CPG_MOD 1010>, <&cpg CPG_MOD 1011>,
> @@ -1856,7 +1856,7 @@
>                                  <&audio_clk_a>, <&audio_clk_b>,
>                                  <&audio_clk_c>,
>                                  <&cpg CPG_CORE R8A7795_CLK_S0D4>;
> -                       clock-names = "ssi-all",
> +                       clock-names = "adg", "ssi-all",
>                                       "ssi.9", "ssi.8", "ssi.7", "ssi.6",
>                                       "ssi.5", "ssi.4", "ssi.3", "ssi.2",
>                                       "ssi.1", "ssi.0",

Noooo !!
It breaks sound clock / module stop controling !!

OK, now I checked more detail of ADG for EAVB/IF.

1st, I don't think we need to add "adg" clock.
It is not exist on Gen2, and default ON on Gen3.
If you want to add it, please add it to "last" of clocks, not "first".
"first" clock is handling whole SSI power.

2nd, it is "Module stop clock", not "S0D1ϕ".
We already handling it as "clk_i".

3rd, we need to create new clock/handler for
avb_counter8 / audio_clk_div3 / avb_div8 for "internal" purpose,
and need to create avb_adg_syn[] clock for "external" purpose for EAVB/IF.
Your code is creating / registering adg->clkavb[],
but it is for avb_counter8 in my understanding.
We don't need to register it as formal clock IMO.

4th, EAVB driver need to get clock from AVB via DT
as "avb_adg_syn[]" clock, not "avb_counter8".

I'm not sure EAVB side driver, but please double check these.


Best regards
---
Kuninori Morimoto

Re: [PATCH linux-next v3 6/7] ASoC: rsnd: add avb clocks

[Jiada Wang]

Hi Morimoto-san

Thanks for your comments

On 2018/12/06 9:59, Kuninori Morimoto wrote:
> Hi Jiada
>
>>>> +	avb = devm_kzalloc(dev, sizeof(*avb), GFP_KERNEL);
>>>> +	if (!avb)
>>>> +		return ERR_PTR(-ENOMEM);
>>>> +
>>>> +	parent_name = __clk_get_name(adg->clkadg);
>>> This parent_name is very strange to me.
>>> AVB parent clk is "AUDIO_CLK_A/B/C/I" (= clk_a/b/c/i in this driver)
>>> or "AUDIO_CLK_OUT_A/B/C/D" (= audio_clkout/1/2/3 in this driver).
>>> And we don't have "adg" clock.
>>> Please double check it.
>> I have some local device-tree change, which expends 'adg' register
>> range and add
>> "adg" clock to rcar_sound node which refer to newly added 'adg' clock
>> (S0D1ϕ) in this patch-set
> (snip)
>> -                       clocks = <&cpg CPG_MOD 1005>,
>> +                       clocks = <&cpg CPG_MOD 922>, <&cpg CPG_MOD 1005>,
>>                                   <&cpg CPG_MOD 1006>, <&cpg CPG_MOD 1007>,
>>                                   <&cpg CPG_MOD 1008>, <&cpg CPG_MOD 1009>,
>>                                   <&cpg CPG_MOD 1010>, <&cpg CPG_MOD 1011>,
>> @@ -1856,7 +1856,7 @@
>>                                   <&audio_clk_a>, <&audio_clk_b>,
>>                                   <&audio_clk_c>,
>>                                   <&cpg CPG_CORE R8A7795_CLK_S0D4>;
>> -                       clock-names = "ssi-all",
>> +                       clock-names = "adg", "ssi-all",
>>                                        "ssi.9", "ssi.8", "ssi.7", "ssi.6",
>>                                        "ssi.5", "ssi.4", "ssi.3", "ssi.2",
>>                                        "ssi.1", "ssi.0",
> Noooo !!
> It breaks sound clock / module stop controling !!
Can you let me know how it breaks sound clock / module stop
so that I can come up with a fix
> OK, now I checked more detail of ADG for EAVB/IF.
>
> 1st, I don't think we need to add "adg" clock.
> It is not exist on Gen2, and default ON on Gen3.
> If you want to add it, please add it to "last" of clocks, not "first".
> "first" clock is handling whole SSI power.
as this device-tree change is only local, I will move 'adg' clock to 
'last' of clocks,
when I submit it.
> 2nd, it is "Module stop clock", not "S0D1ϕ".
> We already handling it as "clk_i".
SMSTPCR922 controls input of two clocks "S0D1ϕ" and "S0D4ϕ",
"S0D4ϕ" is input to BRGA,
"S0D1ϕ" is input to avb_counter8
>
> 3rd, we need to create new clock/handler for
> avb_counter8 / audio_clk_div3 / avb_div8 for "internal" purpose,
> and need to create avb_adg_syn[] clock for "external" purpose for EAVB/IF.
> Your code is creating / registering adg->clkavb[],
> but it is for avb_counter8 in my understanding.
> We don't need to register it as formal clock IMO.
I agree, besides avb_counter8 there are other clocks which need to be 
added as you have mentioned,
in this patch-set, I only added avb_counter8, because I want to keep the 
patch-set simple,
other clocks can be added later.

avb_counter8 is registered as formal clock is because,
there is use case that EAVB-IF may use avb_div8, and avb_counter8 is 
input to avb_div8.
if keeps avb_counter8 'internal', I am not sure how EAVB-IF can set 
clock rate for avb_counter8
> 4th, EAVB driver need to get clock from AVB via DT
> as "avb_adg_syn[]" clock, not "avb_counter8".
>
> I'm not sure EAVB side driver, but please double check these.
yes, EAVB driver only needs to get 'avb_adg_sync[]' clock,
and avb_adg_sync clock is child clock of audio_clk_div3 / avb_div8


Thanks,
Jiada
>
>
> Best regards
> ---
> Kuninori Morimoto

Re: [PATCH linux-next v3 6/7] ASoC: rsnd: add avb clocks

[Kuninori Morimoto]

Hi Jiada

> SMSTPCR922 controls input of two clocks "S0D1ϕ" and "S0D4ϕ",

Ahh, OK I could check it via Block diagram.
But it is "Module stop" for these, anyway.
"Module stop" and "S0D1ϕ/S0D4ϕ" are completely different clocks.

> "S0D4ϕ" is input to BRGA,
> "S0D1ϕ" is input to avb_counter8

It seems we need to update "clk_i" portion first for this purpose.
Then, we need to consider about Gen2 vs Gen3, etc, etc...
It seems we can't keep patch simple...

> > 3rd, we need to create new clock/handler for
> > avb_counter8 / audio_clk_div3 / avb_div8 for "internal" purpose,
> > and need to create avb_adg_syn[] clock for "external" purpose for EAVB/IF.
> > Your code is creating / registering adg->clkavb[],
> > but it is for avb_counter8 in my understanding.
> > We don't need to register it as formal clock IMO.
> I agree, besides avb_counter8 there are other clocks which need to be
> added as you have mentioned,
> in this patch-set, I only added avb_counter8, because I want to keep
> the patch-set simple,
> other clocks can be added later.

No, No, No.
avb_counter8 is just "internal" clock, and one of
parent clock for avb_div8 which is needed for avb_adg_syn[].
We need to control/register is avb_adg_syn[], not avb_counter8.

Your approach on this patch is very "Quick-Hack".
I have no objection for out-of-tree patches.
But, "Quick-Hack approach with changing formal DT on upstream"
is nightmare.

> avb_counter8 is registered as formal clock is because,
> there is use case that EAVB-IF may use avb_div8, and avb_counter8 is
> input to avb_div8.
> if keeps avb_counter8 'internal', I am not sure how EAVB-IF can set
> clock rate for avb_counter8

EAVB/IF driver setups avb_adg_syn[].
Then, ADG considers/searchs avb_counter8/avb_div8/audio_clk_div3
settings internally.
Don't control avb_counter8 directly from EAVB.

Best regards
---
Kuninori Morimoto

Re: [PATCH linux-next v3 6/7] ASoC: rsnd: add avb clocks

[Jiada Wang]

Hi Morimoto-san


On 2018/12/06 14:38, Kuninori Morimoto wrote:
> Hi Jiada
>
>> SMSTPCR922 controls input of two clocks "S0D1ϕ" and "S0D4ϕ",
> Ahh, OK I could check it via Block diagram.
> But it is "Module stop" for these, anyway.
> "Module stop" and "S0D1ϕ/S0D4ϕ" are completely different clocks.
Yes, "module stop" is different from "S0D1ϕ/S0D4ϕ",
both clk_i and avb_counter8's parent clock is same "module stop" clock,
which is <&cpg CPG_MOD 922>, it is a shared gate clock between
avb_counter8 and clk_i


>
>> "S0D4ϕ" is input to BRGA,
>> "S0D1ϕ" is input to avb_counter8
> It seems we need to update "clk_i" portion first for this purpose.
> Then, we need to consider about Gen2 vs Gen3, etc, etc...
> It seems we can't keep patch simple...
Yes, I know it's not simple, so I want to discuss with you,
how we can proceed on this topic,
by adding avb_counter8, rcar audio can support arbitrary clock rate,
which is a desired feature, IMO

>>> 3rd, we need to create new clock/handler for
>>> avb_counter8 / audio_clk_div3 / avb_div8 for "internal" purpose,
>>> and need to create avb_adg_syn[] clock for "external" purpose for EAVB/IF.
>>> Your code is creating / registering adg->clkavb[],
>>> but it is for avb_counter8 in my understanding.
>>> We don't need to register it as formal clock IMO.
>> I agree, besides avb_counter8 there are other clocks which need to be
>> added as you have mentioned,
>> in this patch-set, I only added avb_counter8, because I want to keep
>> the patch-set simple,
>> other clocks can be added later.
> No, No, No.
> avb_counter8 is just "internal" clock, and one of
> parent clock for avb_div8 which is needed for avb_adg_syn[].
> We need to control/register is avb_adg_syn[], not avb_counter8.
>
> Your approach on this patch is very "Quick-Hack".
> I have no objection for out-of-tree patches.
> But, "Quick-Hack approach with changing formal DT on upstream"
> is nightmare.
>
>> avb_counter8 is registered as formal clock is because,
>> there is use case that EAVB-IF may use avb_div8, and avb_counter8 is
>> input to avb_div8.
>> if keeps avb_counter8 'internal', I am not sure how EAVB-IF can set
>> clock rate for avb_counter8
> EAVB/IF driver setups avb_adg_syn[].
> Then, ADG considers/searchs avb_counter8/avb_div8/audio_clk_div3
> settings internally.
> Don't control avb_counter8 directly from EAVB.
OK, I think I understand your point,
which avb_counter8 / avb_div8/audio_clk_div3 need to be used,
only need to be considered by adg internally.

Thanks,
Jiada

> Best regards
> ---
> Kuninori Morimoto