[PATCH 0/3] ASoC: make simple-card a bit more versatile

[PATCH 0/3] ASoC: make simple-card a bit more versatile

[Daniel Mack]

In the process of porting some boards to DT, I noticed the simple-card
driver is already capable of handling most cases just fine. The missing
bits I've spotted are:

 * The clock that is passed in is only read, never set, which is not
   sufficient for boards that support several sampling rates with
   different base frequencies

 * The sysclk id that is passed to snd_soc_dai_set_sysclk() is
   hard-coded

 * There's currently no support for calling into snd_soc_dai_set_clkdiv()

The following three patches fix all those and make simple-card a good
match for the board I'm working with.


Thanks,
Daniel

Daniel Mack (3):
  ASoC: simple-card: set cpu dai clk in hw_params
  ASoC: simple-card: make sysclk index configurable
  ASoC: simple-card: add support for clock divider setup

 .../devicetree/bindings/sound/simple-card.txt      | 13 ++++++
 include/sound/simple_card_utils.h                  | 19 ++++++++
 sound/soc/generic/simple-card-utils.c              | 50 ++++++++++++++++++++++
 sound/soc/generic/simple-card.c                    | 49 +++++++++++++++++++--
 4 files changed, 127 insertions(+), 4 deletions(-)

-- 
2.14.3

[PATCH 1/3] ASoC: simple-card: set cpu dai clk in hw_params

[Daniel Mack]

The simple-card driver currently accepts a clock node in the cpu dai
sub-node and only uses it as an alternative to the
'system-clock-frequency' property to get the current frequency.

This patch adds another use of the passed clock node. If mclk-fs is
specified, the clock will be set to the calculated rate (stream rate *
mclk_fs) in hw_params. This allows platforms to pass a tuneable clock
as phandle that will automatically be set to the right rates.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 Documentation/devicetree/bindings/sound/simple-card.txt | 5 +++++
 sound/soc/generic/simple-card.c                         | 4 ++++
 2 files changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
index 17c13e74667d..a4c72d09cd45 100644
--- a/Documentation/devicetree/bindings/sound/simple-card.txt
+++ b/Documentation/devicetree/bindings/sound/simple-card.txt
@@ -86,6 +86,11 @@ Optional CPU/CODEC subnodes properties:
 					  in dai startup() and disabled with
 					  clk_disable_unprepare() in dai
 					  shutdown().
+					  If a clock is specified and a
+					  multiplication factor is given with
+					  mclk-fs, the clock will be set to the
+					  calculated mclk frequency when the
+					  stream starts.
 - system-clock-direction-out		: specifies clock direction as 'out' on
 					  initialization. It is useful for some aCPUs with
 					  fixed clocks.
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 6959a74a6f49..ca529a6cab06 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -154,6 +154,10 @@ static int asoc_simple_card_hw_params(struct snd_pcm_substream *substream,
 
 	if (mclk_fs) {
 		mclk = params_rate(params) * mclk_fs;
+
+		if (dai_props->cpu_dai.clk)
+			clk_set_rate(dai_props->cpu_dai.clk, mclk);
+
 		ret = snd_soc_dai_set_sysclk(codec_dai, 0, mclk,
 					     SND_SOC_CLOCK_IN);
 		if (ret && ret != -ENOTSUPP)
-- 
2.14.3

[PATCH 2/3] ASoC: simple-card: make sysclk index configurable

[Daniel Mack]

The simple-card driver currently hard-codes the clk_id parameter in
snd_soc_dai_set_sysclk() to 0. Make this configrable for both CPU and
codec dai sub-nodes.

This still has the limitation that only one clk_id can be configured, but it
should help some more platforms to use simple-card in favor to a more
specific machine driver.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 Documentation/devicetree/bindings/sound/simple-card.txt |  3 +++
 include/sound/simple_card_utils.h                       |  1 +
 sound/soc/generic/simple-card-utils.c                   |  3 +++
 sound/soc/generic/simple-card.c                         | 10 ++++++----
 4 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
index a4c72d09cd45..c8d268285a9e 100644
--- a/Documentation/devicetree/bindings/sound/simple-card.txt
+++ b/Documentation/devicetree/bindings/sound/simple-card.txt
@@ -94,6 +94,9 @@ Optional CPU/CODEC subnodes properties:
 - system-clock-direction-out		: specifies clock direction as 'out' on
 					  initialization. It is useful for some aCPUs with
 					  fixed clocks.
+- system-clock-index			: index of the system clock to use when
+					  the mclk frequency is on the CPU/CODEC
+					  DAI. Defaults to 0.
 
 Example 1 - single DAI link:
 
diff --git a/include/sound/simple_card_utils.h b/include/sound/simple_card_utils.h
index 7e25afce6566..ebdf52c9884f 100644
--- a/include/sound/simple_card_utils.h
+++ b/include/sound/simple_card_utils.h
@@ -21,6 +21,7 @@ struct asoc_simple_dai {
 	unsigned int tx_slot_mask;
 	unsigned int rx_slot_mask;
 	struct clk *clk;
+	int sysclk_id;
 };
 
 struct asoc_simple_card_data {
diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
index 3751a07de6aa..493c9b1f057e 100644
--- a/sound/soc/generic/simple-card-utils.c
+++ b/sound/soc/generic/simple-card-utils.c
@@ -199,6 +199,9 @@ int asoc_simple_card_parse_clk(struct device *dev,
 	if (of_property_read_bool(node, "system-clock-direction-out"))
 		simple_dai->clk_direction = SND_SOC_CLOCK_OUT;
 
+	if (!of_property_read_u32(node, "system-clock-index", &val))
+		simple_dai->sysclk_id = val;
+
 	dev_dbg(dev, "%s : sysclk = %d, direction %d\n", name,
 		simple_dai->sysclk, simple_dai->clk_direction);
 
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index ca529a6cab06..5b4afa624395 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -158,13 +158,15 @@ static int asoc_simple_card_hw_params(struct snd_pcm_substream *substream,
 		if (dai_props->cpu_dai.clk)
 			clk_set_rate(dai_props->cpu_dai.clk, mclk);
 
-		ret = snd_soc_dai_set_sysclk(codec_dai, 0, mclk,
-					     SND_SOC_CLOCK_IN);
+		ret = snd_soc_dai_set_sysclk(codec_dai,
+					     dai_props->codec_dai.sysclk_id,
+					     mclk, SND_SOC_CLOCK_IN);
 		if (ret && ret != -ENOTSUPP)
 			goto err;
 
-		ret = snd_soc_dai_set_sysclk(cpu_dai, 0, mclk,
-					     SND_SOC_CLOCK_OUT);
+		ret = snd_soc_dai_set_sysclk(cpu_dai,
+					     dai_props->cpu_dai.sysclk_id,
+					     mclk, SND_SOC_CLOCK_OUT);
 		if (ret && ret != -ENOTSUPP)
 			goto err;
 	}
-- 
2.14.3

[PATCH 3/3] ASoC: simple-card: add support for clock divider setup

[Daniel Mack]

Add support to call into snd_soc_dai_set_clkdiv() for both CPU and codec
DAIs from simple-card's hw_params().

This allows platforms to set hardware-specific values without providing an
own machine driver.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 .../devicetree/bindings/sound/simple-card.txt      |  5 +++
 include/sound/simple_card_utils.h                  | 18 +++++++++
 sound/soc/generic/simple-card-utils.c              | 47 ++++++++++++++++++++++
 sound/soc/generic/simple-card.c                    | 35 ++++++++++++++++
 4 files changed, 105 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
index c8d268285a9e..d3d83256ab5d 100644
--- a/Documentation/devicetree/bindings/sound/simple-card.txt
+++ b/Documentation/devicetree/bindings/sound/simple-card.txt
@@ -97,6 +97,10 @@ Optional CPU/CODEC subnodes properties:
 - system-clock-index			: index of the system clock to use when
 					  the mclk frequency is on the CPU/CODEC
 					  DAI. Defaults to 0.
+- clock-dividers                        : Array of index/value pairs for CPU/CODEC
+					  specific clock dividers to configure at
+					  stream start time. Optional. No divider
+					  is configured if not specified.
 
 Example 1 - single DAI link:
 
@@ -152,6 +156,7 @@ sound {
 		format = "i2s";
 		cpu {
 			sound-dai = <&audio1 0>;
+			clock-dividers = <0 4>;
 		};
 		codec {
 			sound-dai = <&tda998x 0>;
diff --git a/include/sound/simple_card_utils.h b/include/sound/simple_card_utils.h
index ebdf52c9884f..ed3180f975af 100644
--- a/include/sound/simple_card_utils.h
+++ b/include/sound/simple_card_utils.h
@@ -12,6 +12,11 @@
 
 #include <sound/soc.h>
 
+struct asoc_simple_clkdiv {
+	int div_id;
+	int div;
+};
+
 struct asoc_simple_dai {
 	const char *name;
 	unsigned int sysclk;
@@ -22,6 +27,8 @@ struct asoc_simple_dai {
 	unsigned int rx_slot_mask;
 	struct clk *clk;
 	int sysclk_id;
+	struct asoc_simple_clkdiv *clkdiv;
+	int num_clkdiv;
 };
 
 struct asoc_simple_card_data {
@@ -55,6 +62,17 @@ int asoc_simple_card_parse_clk(struct device *dev,
 int asoc_simple_card_clk_enable(struct asoc_simple_dai *dai);
 void asoc_simple_card_clk_disable(struct asoc_simple_dai *dai);
 
+#define asoc_simple_card_parse_clkdiv_cpu(dev, node, dai_link, simple_dai)	\
+	asoc_simple_card_parse_clkdiv(dev, node, simple_dai,			\
+				      dai_link->cpu_dai_name)
+#define asoc_simple_card_parse_clkdiv_codec(dev, node, dai_link, simple_dai)	\
+	asoc_simple_card_parse_clkdiv(dev, node, simple_dai,			\
+				      dai_link->codec_dai_name)
+int asoc_simple_card_parse_clkdiv(struct device *dev,
+				  struct device_node *node,
+				  struct asoc_simple_dai *simple_dai,
+				  const char *name);
+
 #define asoc_simple_card_parse_cpu(node, dai_link,				\
 				   list_name, cells_name, is_single_link)	\
 	asoc_simple_card_parse_dai(node, &dai_link->cpu_of_node,		\
diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
index 493c9b1f057e..ad7912871240 100644
--- a/sound/soc/generic/simple-card-utils.c
+++ b/sound/soc/generic/simple-card-utils.c
@@ -209,6 +209,53 @@ int asoc_simple_card_parse_clk(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(asoc_simple_card_parse_clk);
 
+int asoc_simple_card_parse_clkdiv(struct device *dev,
+				  struct device_node *node,
+				  struct asoc_simple_dai *simple_dai,
+				  const char *name)
+{
+	const char *pname = "clock-dividers";
+	int i, ret, count;
+	u32 val;
+
+	count = of_property_count_u32_elems(node, pname);
+	if (count <= 0)
+		return 0;
+
+	if (count & 1) {
+		dev_err(dev, "Invalid number of values for %s property", pname);
+		return -EINVAL;
+	}
+
+	simple_dai->num_clkdiv = count / 2;
+	simple_dai->clkdiv = devm_kcalloc(dev, simple_dai->num_clkdiv,
+					  sizeof(struct asoc_simple_clkdiv),
+					  GFP_KERNEL);
+	if (!simple_dai->clkdiv)
+		return -ENOMEM;
+
+	for (i = 0; i < simple_dai->num_clkdiv; i++) {
+		ret = of_property_read_u32_index(node, pname, i * 2, &val);
+		if (ret < 0)
+			return ret;
+
+		simple_dai->clkdiv[i].div_id = val;
+
+		ret = of_property_read_u32_index(node, pname, i * 2 + 1, &val);
+		if (ret < 0)
+			return ret;
+
+		simple_dai->clkdiv[i].div = val;
+
+		dev_dbg(dev, "%s : clkdiv #%d = %d\n", name,
+			simple_dai->clkdiv[i].div_id,
+			simple_dai->clkdiv[i].div);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(asoc_simple_card_parse_clkdiv);
+
 int asoc_simple_card_parse_dai(struct device_node *node,
 				    struct device_node **dai_of_node,
 				    const char **dai_name,
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 5b4afa624395..56d1d774feac 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -135,6 +135,23 @@ static void asoc_simple_card_shutdown(struct snd_pcm_substream *substream)
 	asoc_simple_card_clk_disable(&dai_props->codec_dai);
 }
 
+static int asoc_simple_card_set_clkdiv(struct snd_soc_dai *dai,
+				       unsigned int mclk,
+				       const struct asoc_simple_dai *simple_dai)
+{
+	int ret, i;
+
+	for (i = 0; i < simple_dai->num_clkdiv; i++) {
+		ret = snd_soc_dai_set_clkdiv(dai,
+					     simple_dai->clkdiv[i].div_id,
+					     simple_dai->clkdiv[i].div);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int asoc_simple_card_hw_params(struct snd_pcm_substream *substream,
 				      struct snd_pcm_hw_params *params)
 {
@@ -164,11 +181,21 @@ static int asoc_simple_card_hw_params(struct snd_pcm_substream *substream,
 		if (ret && ret != -ENOTSUPP)
 			goto err;
 
+		ret = asoc_simple_card_set_clkdiv(codec_dai, mclk,
+						  &dai_props->codec_dai);
+		if (ret && ret != -ENOTSUPP)
+			goto err;
+
 		ret = snd_soc_dai_set_sysclk(cpu_dai,
 					     dai_props->cpu_dai.sysclk_id,
 					     mclk, SND_SOC_CLOCK_OUT);
 		if (ret && ret != -ENOTSUPP)
 			goto err;
+
+		ret = asoc_simple_card_set_clkdiv(cpu_dai, mclk,
+						  &dai_props->cpu_dai);
+		if (ret && ret != -ENOTSUPP)
+			goto err;
 	}
 	return 0;
 err:
@@ -287,6 +314,14 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
 	if (ret < 0)
 		goto dai_link_of_err;
 
+	ret = asoc_simple_card_parse_clkdiv_cpu(dev, cpu, dai_link, cpu_dai);
+	if (ret < 0)
+		goto dai_link_of_err;
+
+	ret = asoc_simple_card_parse_clkdiv_codec(dev, codec, dai_link, codec_dai);
+	if (ret < 0)
+		goto dai_link_of_err;
+
 	ret = asoc_simple_card_canonicalize_dailink(dai_link);
 	if (ret < 0)
 		goto dai_link_of_err;
-- 
2.14.3

Re: [PATCH 3/3] ASoC: simple-card: add support for clock divider setup

[Kuninori Morimoto]

Hi

> Add support to call into snd_soc_dai_set_clkdiv() for both CPU and codec
> DAIs from simple-card's hw_params().
> 
> This allows platforms to set hardware-specific values without providing an
> own machine driver.
> 
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> ---

Acked-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Best regards
---
Kuninori Morimoto

Re: [PATCH 2/3] ASoC: simple-card: make sysclk index configurable

[Kuninori Morimoto]

Hi Daniel

> The simple-card driver currently hard-codes the clk_id parameter in
> snd_soc_dai_set_sysclk() to 0. Make this configrable for both CPU and
> codec dai sub-nodes.
> 
> This still has the limitation that only one clk_id can be configured, but it
> should help some more platforms to use simple-card in favor to a more
> specific machine driver.
> 
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> ---
>  Documentation/devicetree/bindings/sound/simple-card.txt |  3 +++
>  include/sound/simple_card_utils.h                       |  1 +
>  sound/soc/generic/simple-card-utils.c                   |  3 +++
>  sound/soc/generic/simple-card.c                         | 10 ++++++----
>  4 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
> index a4c72d09cd45..c8d268285a9e 100644
> --- a/Documentation/devicetree/bindings/sound/simple-card.txt
> +++ b/Documentation/devicetree/bindings/sound/simple-card.txt
> @@ -94,6 +94,9 @@ Optional CPU/CODEC subnodes properties:
>  - system-clock-direction-out		: specifies clock direction as 'out' on
>  					  initialization. It is useful for some aCPUs with
>  					  fixed clocks.
> +- system-clock-index			: index of the system clock to use when
> +					  the mclk frequency is on the CPU/CODEC
> +					  DAI. Defaults to 0.

I'm not a DT guy, but I think DT doesn't want to have index directly ?
I don't know detail, but I guess DT want to have like

	system-mclock = <&xxxx 3>

Best regards
---
Kuninori Morimoto

Re: [PATCH 1/3] ASoC: simple-card: set cpu dai clk in hw_params

[Kuninori Morimoto]

Hi Daniel

> The simple-card driver currently accepts a clock node in the cpu dai
> sub-node and only uses it as an alternative to the
> 'system-clock-frequency' property to get the current frequency.
> 
> This patch adds another use of the passed clock node. If mclk-fs is
> specified, the clock will be set to the calculated rate (stream rate *
> mclk_fs) in hw_params. This allows platforms to pass a tuneable clock
> as phandle that will automatically be set to the right rates.
> 
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> ---
(snip)
>  	if (mclk_fs) {
>  		mclk = params_rate(params) * mclk_fs;
> +
> +		if (dai_props->cpu_dai.clk)
> +			clk_set_rate(dai_props->cpu_dai.clk, mclk);
> +
>  		ret = snd_soc_dai_set_sysclk(codec_dai, 0, mclk,
>  					     SND_SOC_CLOCK_IN);
>  		if (ret && ret != -ENOTSUPP)

Having codec is nice balance ?

Best regards
---
Kuninori Morimoto

Re: [PATCH 1/3] ASoC: simple-card: set cpu dai clk in hw_params

[Daniel Mack]

On Tuesday, May 29, 2018 03:38 AM, Kuninori Morimoto wrote:
>> The simple-card driver currently accepts a clock node in the cpu dai
>> sub-node and only uses it as an alternative to the
>> 'system-clock-frequency' property to get the current frequency.
>>
>> This patch adds another use of the passed clock node. If mclk-fs is
>> specified, the clock will be set to the calculated rate (stream rate *
>> mclk_fs) in hw_params. This allows platforms to pass a tuneable clock
>> as phandle that will automatically be set to the right rates.
>>
>> Signed-off-by: Daniel Mack <daniel@zonque.org>
>> ---
> (snip)
>>   	if (mclk_fs) {
>>   		mclk = params_rate(params) * mclk_fs;
>> +
>> +		if (dai_props->cpu_dai.clk)
>> +			clk_set_rate(dai_props->cpu_dai.clk, mclk);
>> +
>>   		ret = snd_soc_dai_set_sysclk(codec_dai, 0, mclk,
>>   					     SND_SOC_CLOCK_IN);
>>   		if (ret && ret != -ENOTSUPP)
> 
> Having codec is nice balance ?

Ah, yes, why not. Will post a v2.


Thanks,
Daniel

Re: [PATCH 2/3] ASoC: simple-card: make sysclk index configurable

[Daniel Mack]

On Tuesday, May 29, 2018 03:35 AM, Kuninori Morimoto wrote:
>> The simple-card driver currently hard-codes the clk_id parameter in
>> snd_soc_dai_set_sysclk() to 0. Make this configrable for both CPU and
>> codec dai sub-nodes.
>>
>> This still has the limitation that only one clk_id can be configured, but it
>> should help some more platforms to use simple-card in favor to a more
>> specific machine driver.
>>
>> Signed-off-by: Daniel Mack <daniel@zonque.org>
>> ---
>>   Documentation/devicetree/bindings/sound/simple-card.txt |  3 +++
>>   include/sound/simple_card_utils.h                       |  1 +
>>   sound/soc/generic/simple-card-utils.c                   |  3 +++
>>   sound/soc/generic/simple-card.c                         | 10 ++++++----
>>   4 files changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
>> index a4c72d09cd45..c8d268285a9e 100644
>> --- a/Documentation/devicetree/bindings/sound/simple-card.txt
>> +++ b/Documentation/devicetree/bindings/sound/simple-card.txt
>> @@ -94,6 +94,9 @@ Optional CPU/CODEC subnodes properties:
>>   - system-clock-direction-out		: specifies clock direction as 'out' on
>>   					  initialization. It is useful for some aCPUs with
>>   					  fixed clocks.
>> +- system-clock-index			: index of the system clock to use when
>> +					  the mclk frequency is on the CPU/CODEC
>> +					  DAI. Defaults to 0.
> 
> I'm not a DT guy, but I think DT doesn't want to have index directly ?
> I don't know detail, but I guess DT want to have like
> 
> 	system-mclock = <&xxxx 3>

Hmm, no. That index doesn't describe a particular output of a clock 
phandle but an internal detail of the CPU or CODEC DAI on the other end. 
Most DAIs will use 0 here, like your code had it before.

The DAI can be both a producer and a consumer of a clock, depending on 
the audio clocking setup, and these details are not exposed in DT.



Thanks,
Daniel

Re: [PATCH 1/3] ASoC: simple-card: set cpu dai clk in hw_params

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 254 bytes --]

On Mon, May 28, 2018 at 09:35:01PM +0200, Daniel Mack wrote:

>  	if (mclk_fs) {
>  		mclk = params_rate(params) * mclk_fs;
> +
> +		if (dai_props->cpu_dai.clk)
> +			clk_set_rate(dai_props->cpu_dai.clk, mclk);
> +

We're ignoring the return value here.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/3] ASoC: simple-card: set cpu dai clk in hw_params

[Daniel Mack]

On Tuesday, May 29, 2018 01:16 PM, Mark Brown wrote:
> On Mon, May 28, 2018 at 09:35:01PM +0200, Daniel Mack wrote:
> 
>>   	if (mclk_fs) {
>>   		mclk = params_rate(params) * mclk_fs;
>> +
>> +		if (dai_props->cpu_dai.clk)
>> +			clk_set_rate(dai_props->cpu_dai.clk, mclk);
>> +
> 
> We're ignoring the return value here.
> 

On purpose actually. Not all clocks might be settable, and in that case, 
this is a no-op. You think we should bail or warn?

Re: [PATCH 2/3] ASoC: simple-card: make sysclk index configurable

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 817 bytes --]

On Mon, May 28, 2018 at 09:35:02PM +0200, Daniel Mack wrote:
> The simple-card driver currently hard-codes the clk_id parameter in
> snd_soc_dai_set_sysclk() to 0. Make this configrable for both CPU and
> codec dai sub-nodes.

> This still has the limitation that only one clk_id can be configured, but it
> should help some more platforms to use simple-card in favor to a more
> specific machine driver.

If we want to get more complex usage of clocks in the DT we should be
moving the CODECs over to using the standard clock bindings for this
stuff rather than inventing custom ASoC clock bindings for it.  That way
we don't have to deal with the pain of trying to join things up in the
future.

As a practical matter we also don't have any CODECs specifying any
bindings for ASoC level clock IDs right now either.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/3] ASoC: simple-card: set cpu dai clk in hw_params

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 775 bytes --]

On Tue, May 29, 2018 at 01:17:45PM +0200, Daniel Mack wrote:
> On Tuesday, May 29, 2018 01:16 PM, Mark Brown wrote:
> > On Mon, May 28, 2018 at 09:35:01PM +0200, Daniel Mack wrote:

> > > +		if (dai_props->cpu_dai.clk)
> > > +			clk_set_rate(dai_props->cpu_dai.clk, mclk);

> > We're ignoring the return value here.

> On purpose actually. Not all clocks might be settable, and in that case,
> this is a no-op. You think we should bail or warn?

If we need to set the rate and fail to set it then clearly we shouldn't
just carry on ignoring the error.  You might want some more involved
logic there around checking if it's actually a rate change before you
error out, and possibly some logic to carry on with whatever the rate is
and a reduced set of resulting sample rates.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 3/3] ASoC: simple-card: add support for clock divider setup

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 482 bytes --]

On Mon, May 28, 2018 at 09:35:03PM +0200, Daniel Mack wrote:
> Add support to call into snd_soc_dai_set_clkdiv() for both CPU and codec
> DAIs from simple-card's hw_params().

> This allows platforms to set hardware-specific values without providing an
> own machine driver.

Why do we have anything that actually needs this?  The devices with a
set_clkdiv() operation are pretty legacy at this point and most of them
should just be figuring out the dividers for themselves anyway.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 2/3] ASoC: simple-card: make sysclk index configurable

[Daniel Mack]

On Tuesday, May 29, 2018 01:24 PM, Mark Brown wrote:
> On Mon, May 28, 2018 at 09:35:02PM +0200, Daniel Mack wrote:
>> The simple-card driver currently hard-codes the clk_id parameter in
>> snd_soc_dai_set_sysclk() to 0. Make this configrable for both CPU and
>> codec dai sub-nodes.
> 
>> This still has the limitation that only one clk_id can be configured, but it
>> should help some more platforms to use simple-card in favor to a more
>> specific machine driver.
> 
> If we want to get more complex usage of clocks in the DT we should be
> moving the CODECs over to using the standard clock bindings for this
> stuff rather than inventing custom ASoC clock bindings for it.  That way
> we don't have to deal with the pain of trying to join things up in the
> future.

This will get rather complex too though, because most codec and cpu dais 
can act as clock source xor clock consumer, depending on how the 
hardware is built. Would you want to represent everything, bit clocks, 
frame clocks, master clocks etc as clock nodes?

If that's the case, could you depict how the DT bindings should look 
like by example?


Thanks,
Daniel

Re: [PATCH 3/3] ASoC: simple-card: add support for clock divider setup

[Daniel Mack]

On Tuesday, May 29, 2018 01:35 PM, Mark Brown wrote:
> On Mon, May 28, 2018 at 09:35:03PM +0200, Daniel Mack wrote:
>> Add support to call into snd_soc_dai_set_clkdiv() for both CPU and codec
>> DAIs from simple-card's hw_params().
> 
>> This allows platforms to set hardware-specific values without providing an
>> own machine driver.
> 
> Why do we have anything that actually needs this?  The devices with a
> set_clkdiv() operation are pretty legacy at this point and most of them
> should just be figuring out the dividers for themselves anyway.

The PXA platform CPU DAI still has it, and grepping also reveals some 
Freescale and Samsung platforms.

If that's considered legacy, we shouldn't add bindings for it of course. 
I can look into the PXA driver and see if it can auto-detect the 
dividers itself. It won't be straight forward though as the clocking 
setup also depends on configurations such as TDM slots, external vs. 
internal clocks etc. Meh.


Thanks,
Daniel

Re: [PATCH 1/3] ASoC: simple-card: set cpu dai clk in hw_params

[Daniel Mack]

On Tuesday, May 29, 2018 01:32 PM, Mark Brown wrote:
> On Tue, May 29, 2018 at 01:17:45PM +0200, Daniel Mack wrote:
>> On Tuesday, May 29, 2018 01:16 PM, Mark Brown wrote:
>>> On Mon, May 28, 2018 at 09:35:01PM +0200, Daniel Mack wrote:
> 
>>>> +		if (dai_props->cpu_dai.clk)
>>>> +			clk_set_rate(dai_props->cpu_dai.clk, mclk);
> 
>>> We're ignoring the return value here.
> 
>> On purpose actually. Not all clocks might be settable, and in that case,
>> this is a no-op. You think we should bail or warn?
> 
> If we need to set the rate and fail to set it then clearly we shouldn't
> just carry on ignoring the error.  You might want some more involved
> logic there around checking if it's actually a rate change before you
> error out, and possibly some logic to carry on with whatever the rate is
> and a reduced set of resulting sample rates.
> 

Fair enough. I'll add that error checking at least, that makes sense.


Thanks for the feedback!
Daniel

Re: [PATCH 3/3] ASoC: simple-card: add support for clock divider setup

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1030 bytes --]

On Tue, May 29, 2018 at 10:29:10PM +0200, Daniel Mack wrote:
> On Tuesday, May 29, 2018 01:35 PM, Mark Brown wrote:

> > Why do we have anything that actually needs this?  The devices with a
> > set_clkdiv() operation are pretty legacy at this point and most of them
> > should just be figuring out the dividers for themselves anyway.

> The PXA platform CPU DAI still has it, and grepping also reveals some
> Freescale and Samsung platforms.

PXA is definitely very legacy, as are some of the Samsung and Freescale
ones - PXA in particular hasn't had much love in years.

> If that's considered legacy, we shouldn't add bindings for it of course. I
> can look into the PXA driver and see if it can auto-detect the dividers
> itself. It won't be straight forward though as the clocking setup also
> depends on configurations such as TDM slots, external vs. internal clocks
> etc. Meh.

It's usually relatively straightforward - something along the lines of
work out what your BCLK needs to be and everything else flows from that.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 3/3] ASoC: simple-card: add support for clock divider setup

[Daniel Mack]

On Wednesday, May 30, 2018 11:10 AM, Mark Brown wrote:
> On Tue, May 29, 2018 at 10:29:10PM +0200, Daniel Mack wrote:
>> On Tuesday, May 29, 2018 01:35 PM, Mark Brown wrote:
> 
>>> Why do we have anything that actually needs this?  The devices with a
>>> set_clkdiv() operation are pretty legacy at this point and most of them
>>> should just be figuring out the dividers for themselves anyway.
> 
>> The PXA platform CPU DAI still has it, and grepping also reveals some
>> Freescale and Samsung platforms.
> 
> PXA is definitely very legacy, as are some of the Samsung and Freescale
> ones - PXA in particular hasn't had much love in years.

Yes, I'm currently preparing a larger series of cleanups that I'll send 
for 4.18. I hope I can also tackle the clkdiv/pll things with that.


Thanks,
Daniel

Re: [PATCH 2/3] ASoC: simple-card: make sysclk index configurable

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1303 bytes --]

On Tue, May 29, 2018 at 10:23:48PM +0200, Daniel Mack wrote:
> On Tuesday, May 29, 2018 01:24 PM, Mark Brown wrote:

> > If we want to get more complex usage of clocks in the DT we should be
> > moving the CODECs over to using the standard clock bindings for this
> > stuff rather than inventing custom ASoC clock bindings for it.  That way
> > we don't have to deal with the pain of trying to join things up in the
> > future.

> This will get rather complex too though, because most codec and cpu dais can
> act as clock source xor clock consumer, depending on how the hardware is
> built. Would you want to represent everything, bit clocks, frame clocks,
> master clocks etc as clock nodes?

We're probably OK just going down to the master clocks mostly I think.
For the clocks on the actual bus we can probably have helpers in the
core that generate the clocks and the drivers just tell the core the
rates.

> If that's the case, could you depict how the DT bindings should look like by
> example?

I've not been able to dig far enough into the clock bindings yet.
That's not super helpful for you I appreciate, TBH nobody seemed to
particularly need this so it wasn't super urgent - most of the things
using alternative clocks on devices seem to also need custom machine
drivers for other reasons.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 2/3] ASoC: simple-card: make sysclk index configurable

[Rob Herring]

On Tue, May 29, 2018 at 10:23:48PM +0200, Daniel Mack wrote:
> On Tuesday, May 29, 2018 01:24 PM, Mark Brown wrote:
> > On Mon, May 28, 2018 at 09:35:02PM +0200, Daniel Mack wrote:
> > > The simple-card driver currently hard-codes the clk_id parameter in
> > > snd_soc_dai_set_sysclk() to 0. Make this configrable for both CPU and
> > > codec dai sub-nodes.
> > 
> > > This still has the limitation that only one clk_id can be configured, but it
> > > should help some more platforms to use simple-card in favor to a more
> > > specific machine driver.
> > 
> > If we want to get more complex usage of clocks in the DT we should be
> > moving the CODECs over to using the standard clock bindings for this
> > stuff rather than inventing custom ASoC clock bindings for it.  That way
> > we don't have to deal with the pain of trying to join things up in the
> > future.
> 
> This will get rather complex too though, because most codec and cpu dais can
> act as clock source xor clock consumer, depending on how the hardware is
> built. Would you want to represent everything, bit clocks, frame clocks,
> master clocks etc as clock nodes?

I have no idea if you need all the clocks or not, but they certainly 
shouldn't be a node per clock. Rather the codec and/or cpu dai should be 
a clock provider and can provide N clocks.
 
> If that's the case, could you depict how the DT bindings should look like by
> example?

In clock providers, you just add #clock-cells. Then the consumer side 
defines 'clocks'. Which clock(s) comes from the dai is defined by the 
index in the 'clocks' property for that node.

Both ends can be providers, but who is active is determined by defining 
the actual connections with 'clocks'. Or perhaps you have both 
directions shown and there is some other means to select which one is 
active such as solving for who can provide the desired freq.

Hope that helps.

Rob

Re: [PATCH 2/3] ASoC: simple-card: make sysclk index configurable

[Daniel Mack]

On Thursday, May 31, 2018 07:02 PM, Rob Herring wrote:
> On Tue, May 29, 2018 at 10:23:48PM +0200, Daniel Mack wrote:

>> If that's the case, could you depict how the DT bindings should look like by
>> example?
> 
> In clock providers, you just add #clock-cells. Then the consumer side
> defines 'clocks'. Which clock(s) comes from the dai is defined by the
> index in the 'clocks' property for that node.
> 
> Both ends can be providers, but who is active is determined by defining
> the actual connections with 'clocks'. Or perhaps you have both
> directions shown and there is some other means to select which one is
> active such as solving for who can provide the desired freq.

How about entities that are a clock producer and consume their clocks 
themselves? That's a rather typical thing for DAIs. Is that expressible 
in DT?


Thanks,
Daniel

Re: [PATCH 2/3] ASoC: simple-card: make sysclk index configurable

[Rob Herring]

On Thu, May 31, 2018 at 3:03 PM, Daniel Mack <daniel@zonque.org> wrote:
> On Thursday, May 31, 2018 07:02 PM, Rob Herring wrote:
>>
>> On Tue, May 29, 2018 at 10:23:48PM +0200, Daniel Mack wrote:
>
>
>>> If that's the case, could you depict how the DT bindings should look like
>>> by
>>> example?
>>
>>
>> In clock providers, you just add #clock-cells. Then the consumer side
>> defines 'clocks'. Which clock(s) comes from the dai is defined by the
>> index in the 'clocks' property for that node.
>>
>> Both ends can be providers, but who is active is determined by defining
>> the actual connections with 'clocks'. Or perhaps you have both
>> directions shown and there is some other means to select which one is
>> active such as solving for who can provide the desired freq.
>
>
> How about entities that are a clock producer and consume their clocks
> themselves? That's a rather typical thing for DAIs. Is that expressible in
> DT?

And I assume something else consumes those clocks, too? If not, you
can just handle all that within the driver and don't need the clock
binding or clock framework.

But yes, you can do something like this:

myclk: clock-controller {
  #clock-cells = <1>;
  clocks = <&myclk 123>, <&otherclk 1>;
};

You'll have to handle clk registration and clk_get in the right order
to avoid deferring probe on yourself.

Rob

Re: [PATCH 3/3] ASoC: simple-card: add support for clock divider setup

[Daniel Mack]

On Wednesday, May 30, 2018 11:12 AM, Daniel Mack wrote:
> On Wednesday, May 30, 2018 11:10 AM, Mark Brown wrote:
>> On Tue, May 29, 2018 at 10:29:10PM +0200, Daniel Mack wrote:
>>> On Tuesday, May 29, 2018 01:35 PM, Mark Brown wrote:
>>
>>>> Why do we have anything that actually needs this?  The devices with a
>>>> set_clkdiv() operation are pretty legacy at this point and most of them
>>>> should just be figuring out the dividers for themselves anyway.
>>
>>> The PXA platform CPU DAI still has it, and grepping also reveals some
>>> Freescale and Samsung platforms.
>>
>> PXA is definitely very legacy, as are some of the Samsung and Freescale
>> ones - PXA in particular hasn't had much love in years.
> 
> Yes, I'm currently preparing a larger series of cleanups that I'll send
> for 4.18. I hope I can also tackle the clkdiv/pll things with that.

I'd like to get some of this done for 4.19, but for that, I'd need 
Robert's patch that addresses DMA compat issues in your tree.

Not sure if you've followed the thread titled '[PATCH v3 09/14] ASoC: 
pxa: remove the dmaengine compat need' - could you chime in there?


Thanks,
Daniel

Re: [PATCH 3/3] ASoC: simple-card: add support for clock divider setup

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 390 bytes --]

On Sat, Jun 23, 2018 at 08:41:27PM +0200, Daniel Mack wrote:

> I'd like to get some of this done for 4.19, but for that, I'd need Robert's
> patch that addresses DMA compat issues in your tree.

> Not sure if you've followed the thread titled '[PATCH v3 09/14] ASoC: pxa:
> remove the dmaengine compat need' - could you chime in there?

No, I reviewed my patch in there.  What's going on?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 3/3] ASoC: simple-card: add support for clock divider setup

[Daniel Mack]

On Monday, June 25, 2018 01:24 PM, Mark Brown wrote:
> On Sat, Jun 23, 2018 at 08:41:27PM +0200, Daniel Mack wrote:
> 
>> I'd like to get some of this done for 4.19, but for that, I'd need Robert's
>> patch that addresses DMA compat issues in your tree.
> 
>> Not sure if you've followed the thread titled '[PATCH v3 09/14] ASoC: pxa:
>> remove the dmaengine compat need' - could you chime in there?
> 
> No, I reviewed my patch in there.  What's going on?
> 

I'd like this patch to go through your tree, if that's okay. Then I can 
post some more patches on top without causing a conflict in the merge 
window.

Robert is fine with that.


Thanks,
Daniel

Re: [PATCH 3/3] ASoC: simple-card: add support for clock divider setup

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 404 bytes --]

On Mon, Jun 25, 2018 at 02:55:12PM +0200, Daniel Mack wrote:
> On Monday, June 25, 2018 01:24 PM, Mark Brown wrote:

> > No, I reviewed my patch in there.  What's going on?

> I'd like this patch to go through your tree, if that's okay. Then I can post
> some more patches on top without causing a conflict in the merge window.

> Robert is fine with that.

That's fine by me.  Is the latest version v3?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 3/3] ASoC: simple-card: add support for clock divider setup

[Daniel Mack]

On Monday, June 25, 2018 02:57 PM, Mark Brown wrote:
> On Mon, Jun 25, 2018 at 02:55:12PM +0200, Daniel Mack wrote:
>> On Monday, June 25, 2018 01:24 PM, Mark Brown wrote:
> 
>>> No, I reviewed my patch in there.  What's going on?
> 
>> I'd like this patch to go through your tree, if that's okay. Then I can post
>> some more patches on top without causing a conflict in the merge window.
> 
>> Robert is fine with that.
> 
> That's fine by me.  Is the latest version v3?
> 

Yes.


Thanks!