It is recommended to not specifiy clocks property in an endpoint subnode.
This series moves clocks to device node.
However after moving the clocks to device node, the audio playback or
capture fails. The specified clock is not actually getting enabled and
hence the failure is seen. There seems to be a bug in simple-card-utils.c
where clock handle is not assigned when parsing clocks from device node.
Fix the same and revert original change which actually added clocks
property in endpoint subnode. Also update Jetson AGX Xavier DT where the
usage is found.
Sameer Pujar (3):
ASoC: simple-card-utils: Fix device module clock
Revert "ASoC: audio-graph-card: Add clocks property to endpoint node"
arm64: tegra: Move clocks from RT5658 endpoint to device node
.../devicetree/bindings/sound/audio-graph-port.yaml | 3 ---
arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts | 2 +-
sound/soc/generic/simple-card-utils.c | 13 ++++++-------
3 files changed, 7 insertions(+), 11 deletions(-)
--
2.7.4
If "clocks = <&xxx>" is specified from the CPU or Codec component
device node, the clock is not getting enabled. Thus audio playback
or capture fails.
Fix this by populating "simple_dai->clk" field when clocks property
is specified from device node as well. Also tidy up by re-organising
conditional statements of parsing logic.
Fixes: bb6fc620c2ed ("ASoC: simple-card-utils: add asoc_simple_card_parse_clk()")
Cc: Kuninori Morimoto <[email protected]>
Signed-off-by: Sameer Pujar <[email protected]>
---
sound/soc/generic/simple-card-utils.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
index bc0b62e..0754d70 100644
--- a/sound/soc/generic/simple-card-utils.c
+++ b/sound/soc/generic/simple-card-utils.c
@@ -173,16 +173,15 @@ int asoc_simple_parse_clk(struct device *dev,
* or device's module clock.
*/
clk = devm_get_clk_from_child(dev, node, NULL);
- if (!IS_ERR(clk)) {
- simple_dai->sysclk = clk_get_rate(clk);
+ if (IS_ERR(clk))
+ clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
+ if (!IS_ERR(clk)) {
simple_dai->clk = clk;
- } else if (!of_property_read_u32(node, "system-clock-frequency", &val)) {
+ simple_dai->sysclk = clk_get_rate(clk);
+ } else if (!of_property_read_u32(node, "system-clock-frequency",
+ &val)) {
simple_dai->sysclk = val;
- } else {
- clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
- if (!IS_ERR(clk))
- simple_dai->sysclk = clk_get_rate(clk);
}
if (of_property_read_bool(node, "system-clock-direction-out"))
--
2.7.4
An endpoint is not a device and it is recommended to use clocks property
in the device node. Hence reverting the original change.
Fixes: 531e5b7abbde ("ASoC: audio-graph-card: Add clocks property to endpoint node")
Suggested-by: Rob Herring <[email protected]>
Cc: Kuninori Morimoto <[email protected]>
Signed-off-by: Sameer Pujar <[email protected]>
---
Documentation/devicetree/bindings/sound/audio-graph-port.yaml | 3 ---
1 file changed, 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/sound/audio-graph-port.yaml b/Documentation/devicetree/bindings/sound/audio-graph-port.yaml
index 08ed8f5..766e910 100644
--- a/Documentation/devicetree/bindings/sound/audio-graph-port.yaml
+++ b/Documentation/devicetree/bindings/sound/audio-graph-port.yaml
@@ -33,9 +33,6 @@ properties:
properties:
remote-endpoint:
maxItems: 1
- clocks:
- maxItems: 1
- description: Describes the clock used by audio component.
mclk-fs:
description: |
Multiplication factor between stream rate and codec mclk.
--
2.7.4
An endpoint is not a device and it is recommended to use clocks property
in device node. RT5658 Codec binding already specifies the usage of
clocks property. Thus move the clocks from endpoint to device node.
Fixes: 5b4f6323096a ("arm64: tegra: Audio graph sound card for Jetson AGX Xavier")
Suggested-by: Rob Herring <[email protected]>
Signed-off-by: Sameer Pujar <[email protected]>
---
arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts b/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
index b079420..ef41349e 100644
--- a/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
+++ b/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
@@ -651,6 +651,7 @@
reg = <0x1a>;
interrupt-parent = <&gpio>;
interrupts = <TEGRA194_MAIN_GPIO(S, 5) GPIO_ACTIVE_HIGH>;
+ clocks = <&bpmp TEGRA194_CLK_AUD_MCLK>;
realtek,jd-src = <2>;
sound-name-prefix = "CVB-RT";
@@ -658,7 +659,6 @@
rt5658_ep: endpoint {
remote-endpoint = <&i2s1_dap_ep>;
mclk-fs = <256>;
- clocks = <&bpmp TEGRA194_CLK_AUD_MCLK>;
};
};
};
--
2.7.4
On Wed, Feb 10, 2021 at 12:13:40PM +0530, Sameer Pujar wrote:
> An endpoint is not a device and it is recommended to use clocks property
> in the device node. Hence reverting the original change.
Please submit patches using subject lines reflecting the style for the
subsystem, this makes it easier for people to identify relevant patches.
Look at what existing commits in the area you're changing are doing and
make sure your subject lines visually resemble what they're doing.
There's no need to resubmit to fix this alone.
On Wed, 10 Feb 2021 12:13:38 +0530, Sameer Pujar wrote:
> It is recommended to not specifiy clocks property in an endpoint subnode.
> This series moves clocks to device node.
>
> However after moving the clocks to device node, the audio playback or
> capture fails. The specified clock is not actually getting enabled and
> hence the failure is seen. There seems to be a bug in simple-card-utils.c
> where clock handle is not assigned when parsing clocks from device node.
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/3] ASoC: simple-card-utils: Fix device module clock
commit: 1e30f642cf2939bbdac82ea0dd3071232670b5ab
[2/3] Revert "ASoC: audio-graph-card: Add clocks property to endpoint node"
commit: 0be0f142b8323378df6358c36dd15494134f5b94
[3/3] arm64: tegra: Move clocks from RT5658 endpoint to device node
(no commit info)
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
Hi Sameer
> diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
> index bc0b62e..0754d70 100644
> --- a/sound/soc/generic/simple-card-utils.c
> +++ b/sound/soc/generic/simple-card-utils.c
> @@ -173,16 +173,15 @@ int asoc_simple_parse_clk(struct device *dev,
> * or device's module clock.
> */
> clk = devm_get_clk_from_child(dev, node, NULL);
> - if (!IS_ERR(clk)) {
> - simple_dai->sysclk = clk_get_rate(clk);
> + if (IS_ERR(clk))
> + clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
>
> + if (!IS_ERR(clk)) {
> simple_dai->clk = clk;
> - } else if (!of_property_read_u32(node, "system-clock-frequency", &val)) {
> + simple_dai->sysclk = clk_get_rate(clk);
> + } else if (!of_property_read_u32(node, "system-clock-frequency",
> + &val)) {
> simple_dai->sysclk = val;
> - } else {
> - clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
> - if (!IS_ERR(clk))
> - simple_dai->sysclk = clk_get_rate(clk);
> }
The comment is indicating that that the clock parsing order,
but this patch exchanges it.
This comment also should be updated, I think.
/*
* Parse dai->sysclk come from "clocks = <&xxx>"
* (if system has common clock)
* or "system-clock-frequency = <xxx>"
* or device's module clock.
*/
asoc_simple_set_clk_rate() will be called if it has simple_dai->clk.
CPU or Codec component clock rate will be exchanged by this patch, I think.
I'm not sure the effect of this patch to existing boards.
And also, this patch has too many unneeded exchange,
thus it was difficult to read for me.
I think it can be simply like this ?
It is understandable what it want to do.
diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
index 8c423afb9d2e..d441890de4dc 100644
--- a/sound/soc/generic/simple-card-utils.c
+++ b/sound/soc/generic/simple-card-utils.c
@@ -168,16 +168,14 @@ int asoc_simple_parse_clk(struct device *dev,
* or device's module clock.
*/
clk = devm_get_clk_from_child(dev, node, NULL);
+ if (IS_ERR(clk))
+ clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
+
if (!IS_ERR(clk)) {
simple_dai->sysclk = clk_get_rate(clk);
-
simple_dai->clk = clk;
} else if (!of_property_read_u32(node, "system-clock-frequency", &val)) {
simple_dai->sysclk = val;
- } else {
- clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
- if (!IS_ERR(clk))
- simple_dai->sysclk = clk_get_rate(clk);
}
if (of_property_read_bool(node, "system-clock-direction-out"))
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Hi Morimoto-san,
On 2/12/2021 5:14 AM, Kuninori Morimoto wrote:
>> diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
>> index bc0b62e..0754d70 100644
>> --- a/sound/soc/generic/simple-card-utils.c
>> +++ b/sound/soc/generic/simple-card-utils.c
>> @@ -173,16 +173,15 @@ int asoc_simple_parse_clk(struct device *dev,
>> * or device's module clock.
>> */
>> clk = devm_get_clk_from_child(dev, node, NULL);
>> - if (!IS_ERR(clk)) {
>> - simple_dai->sysclk = clk_get_rate(clk);
>> + if (IS_ERR(clk))
>> + clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
>>
>> + if (!IS_ERR(clk)) {
>> simple_dai->clk = clk;
>> - } else if (!of_property_read_u32(node, "system-clock-frequency", &val)) {
>> + simple_dai->sysclk = clk_get_rate(clk);
>> + } else if (!of_property_read_u32(node, "system-clock-frequency",
>> + &val)) {
>> simple_dai->sysclk = val;
>> - } else {
>> - clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
>> - if (!IS_ERR(clk))
>> - simple_dai->sysclk = clk_get_rate(clk);
>> }
> The comment is indicating that that the clock parsing order,
> but this patch exchanges it.
> This comment also should be updated, I think.
>
> /*
> * Parse dai->sysclk come from "clocks = <&xxx>"
> * (if system has common clock)
> * or "system-clock-frequency = <xxx>"
> * or device's module clock.
> */
Yes, this can be rephrased now.
> asoc_simple_set_clk_rate() will be called if it has simple_dai->clk.
> CPU or Codec component clock rate will be exchanged by this patch, I think.
> I'm not sure the effect of this patch to existing boards.
If CPU or Codec node does not specifiy "mclk-fs" factor,
asoc_simple_set_clk_rate() won't be called. So I don't think there would
be any effect w.r.t clock rate. With this patch clocks would get
enabled/disabled.
>
> And also, this patch has too many unneeded exchange,
> thus it was difficult to read for me.
> I think it can be simply like this ?
> It is understandable what it want to do.
I think the patch does exactly the same thing as what you are suggesting
below. Am I missing anything?
>
> diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
> index 8c423afb9d2e..d441890de4dc 100644
> --- a/sound/soc/generic/simple-card-utils.c
> +++ b/sound/soc/generic/simple-card-utils.c
> @@ -168,16 +168,14 @@ int asoc_simple_parse_clk(struct device *dev,
> * or device's module clock.
> */
> clk = devm_get_clk_from_child(dev, node, NULL);
> + if (IS_ERR(clk))
> + clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
> +
> if (!IS_ERR(clk)) {
> simple_dai->sysclk = clk_get_rate(clk);
> -
> simple_dai->clk = clk;
> } else if (!of_property_read_u32(node, "system-clock-frequency", &val)) {
> simple_dai->sysclk = val;
> - } else {
> - clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
> - if (!IS_ERR(clk))
> - simple_dai->sysclk = clk_get_rate(clk);
> }
>
> if (of_property_read_bool(node, "system-clock-direction-out"))
Hi Sameer
> > /*
> > * Parse dai->sysclk come from "clocks = <&xxx>"
> > * (if system has common clock)
> > * or "system-clock-frequency = <xxx>"
> > * or device's module clock.
> > */
>
> Yes, this can be rephrased now.
Thanks.
It is not a big-deal. no streass :)
> > And also, this patch has too many unneeded exchange,
> > thus it was difficult to read for me.
> > I think it can be simply like this ?
> > It is understandable what it want to do.
>
> I think the patch does exactly the same thing as what you are
> suggesting below. Am I missing anything?
Yes, it is 100% same, but is simple patch.
I wanted to tell was it is easy to read/understand.
Your patch is already applied, so nothing we can do now ;)
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Hi,
> If "clocks = <&xxx>" is specified from the CPU or Codec component
> device node, the clock is not getting enabled. Thus audio playback
> or capture fails.
>
> Fix this by populating "simple_dai->clk" field when clocks property
> is specified from device node as well. Also tidy up by re-organising
> conditional statements of parsing logic.
>
> Fixes: bb6fc620c2ed ("ASoC: simple-card-utils: add asoc_simple_card_parse_clk()")
> Cc: Kuninori Morimoto <[email protected]>
> Signed-off-by: Sameer Pujar <[email protected]>
This actually breaks sound on my board
(arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts).
The codec on this board (wm8904) has a fixed clock input (only distinct
frequencies are supported) and uses the FLL of the codec to generate the
desired sample rate.
It seems that after this patch the clock rate of the codecs clock (rather
than the FLL) is tried to be changed. Which fails, because it doesn't
support arbitrary frequencies.
-michael
> ---
> sound/soc/generic/simple-card-utils.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
> index bc0b62e..0754d70 100644
> --- a/sound/soc/generic/simple-card-utils.c
> +++ b/sound/soc/generic/simple-card-utils.c
> @@ -173,16 +173,15 @@ int asoc_simple_parse_clk(struct device *dev,
> * or device's module clock.
> */
> clk = devm_get_clk_from_child(dev, node, NULL);
> - if (!IS_ERR(clk)) {
> - simple_dai->sysclk = clk_get_rate(clk);
> + if (IS_ERR(clk))
> + clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
>
> + if (!IS_ERR(clk)) {
> simple_dai->clk = clk;
> - } else if (!of_property_read_u32(node, "system-clock-frequency", &val)) {
> + simple_dai->sysclk = clk_get_rate(clk);
> + } else if (!of_property_read_u32(node, "system-clock-frequency",
> + &val)) {
> simple_dai->sysclk = val;
> - } else {
> - clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
> - if (!IS_ERR(clk))
> - simple_dai->sysclk = clk_get_rate(clk);
> }
>
> if (of_property_read_bool(node, "system-clock-direction-out"))
> --
> 2.7.4
>
>
Hi Michael,
On 3/9/2021 8:11 PM, Michael Walle wrote:
> External email: Use caution opening links or attachments
>
>
> Hi,
>
>> If "clocks = <&xxx>" is specified from the CPU or Codec component
>> device node, the clock is not getting enabled. Thus audio playback
>> or capture fails.
>>
>> Fix this by populating "simple_dai->clk" field when clocks property
>> is specified from device node as well. Also tidy up by re-organising
>> conditional statements of parsing logic.
>>
>> Fixes: bb6fc620c2ed ("ASoC: simple-card-utils: add asoc_simple_card_parse_clk()")
>> Cc: Kuninori Morimoto <[email protected]>
>> Signed-off-by: Sameer Pujar <[email protected]>
> This actually breaks sound on my board
> (arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts).
> The codec on this board (wm8904) has a fixed clock input (only distinct
> frequencies are supported) and uses the FLL of the codec to generate the
> desired sample rate.
>
> It seems that after this patch the clock rate of the codecs clock (rather
> than the FLL) is tried to be changed. Which fails, because it doesn't
> support arbitrary frequencies.
Yes, after the given change the clock will be updated if "*mclk-fs"
property is specified.
DT you mentioned has property "simple-audio-card,mclk-fs = <256>", which
means you need a clock that is a function of sample rate. But as per
above you want a fixed clock for MCLK. I think if you drop this
property, the clock updates won't happen. Earlier for your case, this
property was not used at all because the clock handle was not populated.
>
> -michael
>
>> ---
>> sound/soc/generic/simple-card-utils.c | 13 ++++++-------
>> 1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
>> index bc0b62e..0754d70 100644
>> --- a/sound/soc/generic/simple-card-utils.c
>> +++ b/sound/soc/generic/simple-card-utils.c
>> @@ -173,16 +173,15 @@ int asoc_simple_parse_clk(struct device *dev,
>> * or device's module clock.
>> */
>> clk = devm_get_clk_from_child(dev, node, NULL);
>> - if (!IS_ERR(clk)) {
>> - simple_dai->sysclk = clk_get_rate(clk);
>> + if (IS_ERR(clk))
>> + clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
>>
>> + if (!IS_ERR(clk)) {
>> simple_dai->clk = clk;
>> - } else if (!of_property_read_u32(node, "system-clock-frequency", &val)) {
>> + simple_dai->sysclk = clk_get_rate(clk);
>> + } else if (!of_property_read_u32(node, "system-clock-frequency",
>> + &val)) {
>> simple_dai->sysclk = val;
>> - } else {
>> - clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
>> - if (!IS_ERR(clk))
>> - simple_dai->sysclk = clk_get_rate(clk);
>> }
>>
>> if (of_property_read_bool(node, "system-clock-direction-out"))
>> --
>> 2.7.4
>>
>>
Hi Sameer,
Am 2021-03-09 17:27, schrieb Sameer Pujar:
> On 3/9/2021 8:11 PM, Michael Walle wrote:
>>> If "clocks = <&xxx>" is specified from the CPU or Codec component
>>> device node, the clock is not getting enabled. Thus audio playback
>>> or capture fails.
>>>
>>> Fix this by populating "simple_dai->clk" field when clocks property
>>> is specified from device node as well. Also tidy up by re-organising
>>> conditional statements of parsing logic.
>>>
>>> Fixes: bb6fc620c2ed ("ASoC: simple-card-utils: add
>>> asoc_simple_card_parse_clk()")
>>> Cc: Kuninori Morimoto <[email protected]>
>>> Signed-off-by: Sameer Pujar <[email protected]>
>>
>> This actually breaks sound on my board
>> (arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts).
>> The codec on this board (wm8904) has a fixed clock input (only
>> distinct
>> frequencies are supported) and uses the FLL of the codec to generate
>> the
>> desired sample rate.
>>
>> It seems that after this patch the clock rate of the codecs clock
>> (rather
>> than the FLL) is tried to be changed. Which fails, because it doesn't
>> support arbitrary frequencies.
>
> Yes, after the given change the clock will be updated if "*mclk-fs"
> property is specified.
>
> DT you mentioned has property "simple-audio-card,mclk-fs = <256>",
> which means you need a clock that is a function of sample rate. But as
> per above you want a fixed clock for MCLK. I think if you drop this
> property, the clock updates won't happen. Earlier for your case, this
> property was not used at all because the clock handle was not
> populated.
You mean to drop the mclk-fs property? I can't do that because I
actually need a frequency of 256 * sample rate. But that doesn't
necessarily need to be the MCLK, because the codec itself has a
FLL/PLL which can be used to generate any frequency for a given
MCLK. So that is a valid scenario. See also commit 13409d27cb39
("ASoC: wm8904: configure sysclk/FLL automatically").
-michael
On 3/10/2021 4:00 AM, Michael Walle wrote:
> Am 2021-03-09 17:27, schrieb Sameer Pujar:
>> On 3/9/2021 8:11 PM, Michael Walle wrote:
>>>> If "clocks = <&xxx>" is specified from the CPU or Codec component
>>>> device node, the clock is not getting enabled. Thus audio playback
>>>> or capture fails.
>>>>
>>>> Fix this by populating "simple_dai->clk" field when clocks property
>>>> is specified from device node as well. Also tidy up by re-organising
>>>> conditional statements of parsing logic.
>>>>
>>>> Fixes: bb6fc620c2ed ("ASoC: simple-card-utils: add
>>>> asoc_simple_card_parse_clk()")
>>>> Cc: Kuninori Morimoto <[email protected]>
>>>> Signed-off-by: Sameer Pujar <[email protected]>
>>>
>>> This actually breaks sound on my board
>>> (arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts).
>>> The codec on this board (wm8904) has a fixed clock input (only
>>> distinct
>>> frequencies are supported) and uses the FLL of the codec to generate
>>> the
>>> desired sample rate.
>>>
>>> It seems that after this patch the clock rate of the codecs clock
>>> (rather
>>> than the FLL) is tried to be changed. Which fails, because it doesn't
>>> support arbitrary frequencies.
>>
>> Yes, after the given change the clock will be updated if "*mclk-fs"
>> property is specified.
>>
>> DT you mentioned has property "simple-audio-card,mclk-fs = <256>",
>> which means you need a clock that is a function of sample rate. But as
>> per above you want a fixed clock for MCLK. I think if you drop this
>> property, the clock updates won't happen. Earlier for your case, this
>> property was not used at all because the clock handle was not
>> populated.
>
> You mean to drop the mclk-fs property? I can't do that because I
> actually need a frequency of 256 * sample rate. But that doesn't
> necessarily need to be the MCLK, because the codec itself has a
> FLL/PLL which can be used to generate any frequency for a given
> MCLK. So that is a valid scenario. See also commit 13409d27cb39
> ("ASoC: wm8904: configure sysclk/FLL automatically").
>
If I read this correctly below is the configuration you need,
SoC -> MCLK(fixed rate) -> PLL(wm8904) -> PLL output (256 * fs) -> sysclk
From the doc simple-card.txt, "simple-audio-card,mclk-fs" is a scaling
factor for MCLK and hence I am not sure if it is correct to have
"*mclk-fs" property when MCLK is fixed. In simple cases, codec sysclk
direclty depends on MCLK and set_sysclk() callback helps. Your case
requires PLL configuration and set_pll() may be a better alternative.
However simple-card does not offer this yet. But even if this is added,
there should be a way to suggest PLL output requirement as a function of
sample rate.
Hi Sameer, Hi Mark,
Am 2021-03-10 15:50, schrieb Sameer Pujar:
> On 3/10/2021 4:00 AM, Michael Walle wrote:
>> Am 2021-03-09 17:27, schrieb Sameer Pujar:
>>> On 3/9/2021 8:11 PM, Michael Walle wrote:
>>>>> If "clocks = <&xxx>" is specified from the CPU or Codec component
>>>>> device node, the clock is not getting enabled. Thus audio playback
>>>>> or capture fails.
>>>>>
>>>>> Fix this by populating "simple_dai->clk" field when clocks property
>>>>> is specified from device node as well. Also tidy up by
>>>>> re-organising
>>>>> conditional statements of parsing logic.
>>>>>
>>>>> Fixes: bb6fc620c2ed ("ASoC: simple-card-utils: add
>>>>> asoc_simple_card_parse_clk()")
>>>>> Cc: Kuninori Morimoto <[email protected]>
>>>>> Signed-off-by: Sameer Pujar <[email protected]>
>>>>
>>>> This actually breaks sound on my board
>>>> (arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts).
>>>> The codec on this board (wm8904) has a fixed clock input (only
>>>> distinct
>>>> frequencies are supported) and uses the FLL of the codec to generate
>>>> the
>>>> desired sample rate.
>>>>
>>>> It seems that after this patch the clock rate of the codecs clock
>>>> (rather
>>>> than the FLL) is tried to be changed. Which fails, because it
>>>> doesn't
>>>> support arbitrary frequencies.
>>>
>>> Yes, after the given change the clock will be updated if "*mclk-fs"
>>> property is specified.
>>>
>>> DT you mentioned has property "simple-audio-card,mclk-fs = <256>",
>>> which means you need a clock that is a function of sample rate. But
>>> as
>>> per above you want a fixed clock for MCLK. I think if you drop this
>>> property, the clock updates won't happen. Earlier for your case, this
>>> property was not used at all because the clock handle was not
>>> populated.
>>
>> You mean to drop the mclk-fs property? I can't do that because I
>> actually need a frequency of 256 * sample rate. But that doesn't
>> necessarily need to be the MCLK, because the codec itself has a
>> FLL/PLL which can be used to generate any frequency for a given
>> MCLK. So that is a valid scenario. See also commit 13409d27cb39
>> ("ASoC: wm8904: configure sysclk/FLL automatically").
>>
>
> If I read this correctly below is the configuration you need,
> SoC -> MCLK(fixed rate) -> PLL(wm8904) -> PLL output (256 * fs) ->
> sysclk
>
> From the doc simple-card.txt, "simple-audio-card,mclk-fs" is a scaling
> factor for MCLK and hence I am not sure if it is correct to have
> "*mclk-fs" property when MCLK is fixed. In simple cases, codec sysclk
> direclty depends on MCLK and set_sysclk() callback helps. Your case
> requires PLL configuration and set_pll() may be a better alternative.
> However simple-card does not offer this yet. But even if this is
> added, there should be a way to suggest PLL output requirement as a
> function of sample rate.
But its also a scaling factor for the sysclk, then maybe the property
has a wrong name. And you might be right with your suggestion, but
as I said, this breaks sound on my board and whats even worse, it
breaks it for older kernel too, because of the Fixes tag.
Btw I'm pretty sure, the MCLK was enabled and disabled depending on
whether there was an audio stream, the last time I've measured the
clock.
Mark, can this patch please be reverted (with a Fixes tag) until
a proper fix is found which satisfies both needs?
-michael
On 3/10/2021 11:44 PM, Michael Walle wrote:
> Btw I'm pretty sure, the MCLK was enabled and disabled depending on
> whether there was an audio stream, the last time I've measured the
> clock.
This may be true in your case because wm8904 driver does an explicit
clock enable/disable and does not rely on simple-card-utils for this. In
my case it depends on simple-card-utils.
Hi Sameer,
Am 2021-03-10 15:50, schrieb Sameer Pujar:
> On 3/10/2021 4:00 AM, Michael Walle wrote:
>> Am 2021-03-09 17:27, schrieb Sameer Pujar:
>>> On 3/9/2021 8:11 PM, Michael Walle wrote:
>>>>> If "clocks = <&xxx>" is specified from the CPU or Codec component
>>>>> device node, the clock is not getting enabled. Thus audio playback
>>>>> or capture fails.
>>>>>
>>>>> Fix this by populating "simple_dai->clk" field when clocks property
>>>>> is specified from device node as well. Also tidy up by
>>>>> re-organising
>>>>> conditional statements of parsing logic.
>>>>>
>>>>> Fixes: bb6fc620c2ed ("ASoC: simple-card-utils: add
>>>>> asoc_simple_card_parse_clk()")
>>>>> Cc: Kuninori Morimoto <[email protected]>
>>>>> Signed-off-by: Sameer Pujar <[email protected]>
>>>>
>>>> This actually breaks sound on my board
>>>> (arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts).
>>>> The codec on this board (wm8904) has a fixed clock input (only
>>>> distinct
>>>> frequencies are supported) and uses the FLL of the codec to generate
>>>> the
>>>> desired sample rate.
>>>>
>>>> It seems that after this patch the clock rate of the codecs clock
>>>> (rather
>>>> than the FLL) is tried to be changed. Which fails, because it
>>>> doesn't
>>>> support arbitrary frequencies.
>>>
>>> Yes, after the given change the clock will be updated if "*mclk-fs"
>>> property is specified.
>>>
>>> DT you mentioned has property "simple-audio-card,mclk-fs = <256>",
>>> which means you need a clock that is a function of sample rate. But
>>> as
>>> per above you want a fixed clock for MCLK. I think if you drop this
>>> property, the clock updates won't happen. Earlier for your case, this
>>> property was not used at all because the clock handle was not
>>> populated.
>>
>> You mean to drop the mclk-fs property? I can't do that because I
>> actually need a frequency of 256 * sample rate. But that doesn't
>> necessarily need to be the MCLK, because the codec itself has a
>> FLL/PLL which can be used to generate any frequency for a given
>> MCLK. So that is a valid scenario. See also commit 13409d27cb39
>> ("ASoC: wm8904: configure sysclk/FLL automatically").
>>
I've had a closer look at this and it seems you're messing around
with the clock of the codec's node (which is _not_ a subnode of
the simple-audio-card). I don't think this is correct.
I guess you should rather set the clock property in the codec
subnode of the simple-audio-card, which is then picked up by the
simple-audio-card driver and changed accordingly.
For example:
simple-audio-card,dai-link@0 {
reg = <0>;
bitclock-master = <&dailink0_master>;
frame-master = <&dailink0_master>;
format = "i2s";
cpu {
sound-dai = <&sai6>;
};
dailink0_master: codec {
sound-dai = <&wm8904>;
clocks = <&mclk>;
};
};
In this case mclk will be enabled and disabled accordingly.
Could you test this?
-michael
On 3/11/2021 3:57 PM, Michael Walle wrote:
> I've had a closer look at this and it seems you're messing around
> with the clock of the codec's node (which is _not_ a subnode of
> the simple-audio-card). I don't think this is correct.
>
> I guess you should rather set the clock property in the codec
> subnode of the simple-audio-card, which is then picked up by the
> simple-audio-card driver and changed accordingly.
>
> For example:
> simple-audio-card,dai-link@0 {
> reg = <0>;
> bitclock-master = <&dailink0_master>;
> frame-master = <&dailink0_master>;
> format = "i2s";
>
> cpu {
> sound-dai = <&sai6>;
> };
>
> dailink0_master: codec {
> sound-dai = <&wm8904>;
> clocks = <&mclk>;
> };
> };
>
> In this case mclk will be enabled and disabled accordingly.
>
> Could you test this?
>
It would work and initially I had similar patch, see [0] and related
series. Suggestion is to always use "clocks" property with devices only.
[0]
https://patchwork.kernel.org/project/alsa-devel/patch/[email protected]/
Am 2021-03-11 12:05, schrieb Sameer Pujar:
> It would work and initially I had similar patch, see [0] and related
> series. Suggestion is to always use "clocks" property with devices
> only.
I see. But again, I don't think it is correct to change the clock of
the codec by default. What happens if this is for example a
compatible = "fixed-clock"?
As you pointed out in the referred thread [0]. simple-audio-card has
that clock and judging from the code it is exactly for this reason:
to either change/enable it or not.
With this patch you'll switch that to "always change it". Therefore,
shouldn't there be a dt flag to indicate wheter simple-audio-card/graph
should be in charge of the codecs clock input?
And its fetching just the first clock, doesn't it? What happens if a
codec has two clock inputs?
-michael
[0]
https://patchwork.kernel.org/project/alsa-devel/patch/[email protected]/
On 3/11/2021 4:46 PM, Michael Walle wrote:
> Am 2021-03-11 12:05, schrieb Sameer Pujar:
>
>> It would work and initially I had similar patch, see [0] and related
>> series. Suggestion is to always use "clocks" property with devices
>> only.
>
> I see. But again, I don't think it is correct to change the clock of
> the codec by default. What happens if this is for example a
> compatible = "fixed-clock"?
The codec rate won't be changed unless a corresponding "*mclk-fs" is
provided.
>
> As you pointed out in the referred thread [0]. simple-audio-card has
> that clock and judging from the code it is exactly for this reason:
> to either change/enable it or not.
>
> With this patch you'll switch that to "always change it". Therefore,
> shouldn't there be a dt flag to indicate wheter simple-audio-card/graph
> should be in charge of the codecs clock input?
As mentioned above, it does not change always. Requires "*mclk-fs" to do so.
May be below could be a possible alternative?
- Re-order if-else of clock parsing.
if (!of_property_read_u32(node, "system-clock-frequency", &val)) {
// Since you are fixing rate already via "assigned-clocks" this
may be a duplication. OR
// "assigned-clocks" can be parsed to understand if a fixed rate
is expected.
simple_dai->sysclk = val;
} else {
// fetch MCLK clock from device and setup sysclk
// a. If "*mclk-fs" is given and "clocks" is found, the rate
would be updated.
// b. If "*mclk-fs" is not mentioned and "clocks" is found, then
simple-card utils won't touch rate. It will just do clock enable/disable.
}
>
> And its fetching just the first clock, doesn't it? What happens if a
> codec has two clock inputs?
Yes, it would have been more descriptive if it were specifically looking
for clock "mclk". I think the original assumption was codec takes one
input clock (MCLK) and uses it for sysclk.
>
> -michael
>
> [0]
> https://patchwork.kernel.org/project/alsa-devel/patch/[email protected]/
>
Am 2021-03-11 15:29, schrieb Sameer Pujar:
> On 3/11/2021 4:46 PM, Michael Walle wrote:
>> Am 2021-03-11 12:05, schrieb Sameer Pujar:
>>
>>> It would work and initially I had similar patch, see [0] and related
>>> series. Suggestion is to always use "clocks" property with devices
>>> only.
>>
>> I see. But again, I don't think it is correct to change the clock of
>> the codec by default. What happens if this is for example a
>> compatible = "fixed-clock"?
>
> The codec rate won't be changed unless a corresponding "*mclk-fs" is
> provided.
>
>>
>> As you pointed out in the referred thread [0]. simple-audio-card has
>> that clock and judging from the code it is exactly for this reason:
>> to either change/enable it or not.
>>
>
>
>> With this patch you'll switch that to "always change it". Therefore,
>> shouldn't there be a dt flag to indicate wheter
>> simple-audio-card/graph
>> should be in charge of the codecs clock input?
>
> As mentioned above, it does not change always. Requires "*mclk-fs" to
> do so.
As mentioned earlier, this is changing the sysclk, too. And I'd guess
most codecs need a fixed factor for the sysclk, thus if the codec
supports
generating its own sysclk by a PLL it will need this factor, too.
Which is also defined in the binding:
system-clock-frequency:
description: |
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.
> May be below could be a possible alternative?
> - Re-order if-else of clock parsing.
>
> if (!of_property_read_u32(node, "system-clock-frequency", &val)) {
> // Since you are fixing rate already via "assigned-clocks" this
> may be a duplication. OR
exactly. and also need a device tree change (also for older kernels)
on my side.
This could be a last resort, yes. But I'd rather see a flag which
indicates whether the simple-audio-card should control the (first)
clock of the codec or not. Because until now, this wasn't the case.
And I don't know if this was an oversight or on purpose. Kuninori would
need to comment on that. And with the "we change mclk by default", we
break codecs with automatic sysclk generation.
> // "assigned-clocks" can be parsed to understand if a fixed
> rate is expected.
Sounds like a hack to me. Esp. its doing the same as its already
doing right now. That is a "sysfreq = clk_get_rate(codec)".
> simple_dai->sysclk = val;
> } else {
> // fetch MCLK clock from device and setup sysclk
> // a. If "*mclk-fs" is given and "clocks" is found, the rate
> would be updated.
> // b. If "*mclk-fs" is not mentioned and "clocks" is found,
> then simple-card utils won't touch rate. It will just do clock
> enable/disable.
mclk-fs is also a factor for the sysclk, thus it is also needed
if there is no mclk (or a fixed mclk).
I don't think you can deduce whether you can change the codecs
first clock with the current information :(
> }
>
>>
>> And its fetching just the first clock, doesn't it? What happens if a
>> codec has two clock inputs?
>
> Yes, it would have been more descriptive if it were specifically
> looking for clock "mclk". I think the original assumption was codec
> takes one input clock (MCLK) and uses it for sysclk.
Yeah, I've just noticed that the clk_get_rate() also only works
for the first clock of the codec.
-michael
On Tue, Mar 09, 2021 at 03:41:56PM +0100, Michael Walle wrote:
> Hi,
>
> > If "clocks = <&xxx>" is specified from the CPU or Codec component
> > device node, the clock is not getting enabled. Thus audio playback
> > or capture fails.
> This actually breaks sound on my board
> (arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts).
Please, when sending replies format the subject line like normal replies
with a "Re: " at the start so people can tell it's a reply to an
existing discussion and not a new patch.
On Wed, Mar 10, 2021 at 08:20:28PM +0530, Sameer Pujar wrote:
> If I read this correctly below is the configuration you need,
> SoC -> MCLK(fixed rate) -> PLL(wm8904) -> PLL output (256 * fs) -> sysclk
For this device for integration with something like simple-audio-card
since there's limited flexibility within the device the simplest thing
would be to not make the internal clocking of the device visible and
just have it figure out how to use the input clock, using the MCLK
directly if possible otherwise using the FLL to generate a suitable
clock. The trick is figuring out if it's best to vary the input clock
or to use the FLL to adapt a fixed input clock, and of course adapting
any existing users if things get changed.
On Thu, Mar 11, 2021 at 04:43:20PM +0100, Michael Walle wrote:
> This could be a last resort, yes. But I'd rather see a flag which
> indicates whether the simple-audio-card should control the (first)
> clock of the codec or not. Because until now, this wasn't the case.
> And I don't know if this was an oversight or on purpose. Kuninori would
> need to comment on that. And with the "we change mclk by default", we
> break codecs with automatic sysclk generation.
It shouldn't break anything so long as the clock ends up correct via
some path. Where there's multiple options we can also try going through
them in some order, preferring the clock in the CODEC would probably
make sense from both a compatibility and quality point of view.
> > > And its fetching just the first clock, doesn't it? What happens if a
> > > codec has two clock inputs?
> > Yes, it would have been more descriptive if it were specifically
> > looking for clock "mclk". I think the original assumption was codec
> > takes one input clock (MCLK) and uses it for sysclk.
> Yeah, I've just noticed that the clk_get_rate() also only works
> for the first clock of the codec.
simple-audio-card isn't really intended to work with complex devices,
it's very much only for the simplest of use cases.
Am 2021-03-11 17:00, schrieb Mark Brown:
> On Tue, Mar 09, 2021 at 03:41:56PM +0100, Michael Walle wrote:
>> Hi,
>>
>> > If "clocks = <&xxx>" is specified from the CPU or Codec component
>> > device node, the clock is not getting enabled. Thus audio playback
>> > or capture fails.
>
>> This actually breaks sound on my board
>> (arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts).
>
> Please, when sending replies format the subject line like normal
> replies
> with a "Re: " at the start so people can tell it's a reply to an
> existing discussion and not a new patch.
Whoops, must have missed that. I need to figure out a new mail setup,
rather than saving the mbox from lore.kernel.org, editing it and
sending it with git-send-email.
-michael
Am 2021-03-11 17:15, schrieb Mark Brown:
> On Wed, Mar 10, 2021 at 08:20:28PM +0530, Sameer Pujar wrote:
>
>> If I read this correctly below is the configuration you need,
>> SoC -> MCLK(fixed rate) -> PLL(wm8904) -> PLL output (256 * fs) ->
>> sysclk
>
> For this device for integration with something like simple-audio-card
> since there's limited flexibility within the device the simplest thing
> would be to not make the internal clocking of the device visible and
> just have it figure out how to use the input clock, using the MCLK
> directly if possible otherwise using the FLL to generate a suitable
> clock.
Before this patch, part of this was already happening. That is,
simple-audio-card called set_sysclk(samplerate * mclk-fs), then
the codec figured out that its mclk was different than the
requested sample rate and enabled its FLL to generate the requested
sample rate automatically.
With this patch applied, simple-audio-card already tries to change
mclk, which isn't working in my case (the MCLK isn't generated by
a PLL and just supports fixed frequencies) and thus breaks audio. And
this patch also propagate to the stable kernels and breaks my
board there, too.
> The trick is figuring out if it's best to vary the input clock
> or to use the FLL to adapt a fixed input clock,
For simple-audio-card you can set the "clock" property if you want
that clock to be changed/enabled/disabled. But that doesn't seem to
be the way to go, at least it was NAKed by Rob for the audio-graph-card.
I don't see a way to figure out if MCLK should be controlled by
simple-*-card without adding further properties to the device tree.
> and of course adapting any existing users if things get changed.
To be frank, I don't see that happening.
-michael
On Thu, Mar 11, 2021 at 11:11:15PM +0100, Michael Walle wrote:
> Am 2021-03-11 17:15, schrieb Mark Brown:
> > The trick is figuring out if it's best to vary the input clock
> > or to use the FLL to adapt a fixed input clock,
> For simple-audio-card you can set the "clock" property if you want
> that clock to be changed/enabled/disabled. But that doesn't seem to
> be the way to go, at least it was NAKed by Rob for the audio-graph-card.
> I don't see a way to figure out if MCLK should be controlled by
> simple-*-card without adding further properties to the device tree.
If the card has a clock API clock as sysclk then set_sysclk(() should
be configuring that clock.
Am 2021-03-12 12:35, schrieb Mark Brown:
> On Thu, Mar 11, 2021 at 11:11:15PM +0100, Michael Walle wrote:
>> Am 2021-03-11 17:15, schrieb Mark Brown:
>
>> > The trick is figuring out if it's best to vary the input clock
>> > or to use the FLL to adapt a fixed input clock,
>
>> For simple-audio-card you can set the "clock" property if you want
>> that clock to be changed/enabled/disabled. But that doesn't seem to
>> be the way to go, at least it was NAKed by Rob for the
>> audio-graph-card.
>> I don't see a way to figure out if MCLK should be controlled by
>> simple-*-card without adding further properties to the device tree.
>
> If the card has a clock API clock as sysclk then set_sysclk(() should
> be configuring that clock.
What do you mean by "the card". The simple-audio-card itself?
Take a look at:
https://elixir.bootlin.com/linux/v5.12-rc2/source/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts#L29
Does the card has a clock? IMHO the WM8904 codec has a clock, but not
the audio card.
-michael
On Fri, Mar 12, 2021 at 01:01:41PM +0100, Michael Walle wrote:
> Am 2021-03-12 12:35, schrieb Mark Brown:
> > If the card has a clock API clock as sysclk then set_sysclk(() should
> > be configuring that clock.
> What do you mean by "the card". The simple-audio-card itself?
> Take a look at:
> https://elixir.bootlin.com/linux/v5.12-rc2/source/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts#L29
> Does the card has a clock? IMHO the WM8904 codec has a clock, but not
> the audio card.
The clock on the CODEC, which the card configures. The CODEC should be
passing on the configuration to the clock API.
Am 2021-03-12 13:04, schrieb Mark Brown:
> On Fri, Mar 12, 2021 at 01:01:41PM +0100, Michael Walle wrote:
>> Am 2021-03-12 12:35, schrieb Mark Brown:
>
>> > If the card has a clock API clock as sysclk then set_sysclk(() should
>> > be configuring that clock.
>
>> What do you mean by "the card". The simple-audio-card itself?
>
>> Take a look at:
>> https://elixir.bootlin.com/linux/v5.12-rc2/source/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts#L29
>
>> Does the card has a clock? IMHO the WM8904 codec has a clock, but not
>> the audio card.
>
> The clock on the CODEC, which the card configures. The CODEC should be
> passing on the configuration to the clock API.
Sorry, I don't understand.
The card calls set_sysclk(), which eventually ends up in the codec.
The codec therefore, could figure out if it needs to configure the
clock or if it can use its internal FLL.
Is that what you mean?
But the set_sysclk() of the codec isn't even called, because the
card itself already tries to call clk_set_rate() on the Codec's MCLK,
which returns with an error [0].
[0]
https://elixir.bootlin.com/linux/v5.12-rc2/source/sound/soc/generic/simple-card-utils.c#L265
-michael
On Fri, Mar 12, 2021 at 01:30:02PM +0100, Michael Walle wrote:
> The card calls set_sysclk(), which eventually ends up in the codec.
> The codec therefore, could figure out if it needs to configure the
> clock or if it can use its internal FLL.
> Is that what you mean?
Yes.
> But the set_sysclk() of the codec isn't even called, because the
> card itself already tries to call clk_set_rate() on the Codec's MCLK,
> which returns with an error [0].
OK, so I think we need to push this down a level so that the clock
setting is implemented by the core/CODEC rather than by simple-card,
with the helpers being something the CODEC can opt out of.
Am 2021-03-12 14:46, schrieb Mark Brown:
> On Fri, Mar 12, 2021 at 01:30:02PM +0100, Michael Walle wrote:
>
>> The card calls set_sysclk(), which eventually ends up in the codec.
>> The codec therefore, could figure out if it needs to configure the
>> clock or if it can use its internal FLL.
>> Is that what you mean?
>
> Yes.
>
>> But the set_sysclk() of the codec isn't even called, because the
>> card itself already tries to call clk_set_rate() on the Codec's MCLK,
>> which returns with an error [0].
>
> OK, so I think we need to push this down a level so that the clock
> setting is implemented by the core/CODEC rather than by simple-card,
> with the helpers being something the CODEC can opt out of.
Sameer, it looks like the proper fix should be to add the clock
support to your codec.
I've also looked at other users of "simple-audio-card" and
it looks like they will break too. For example,
- arch/arm64/boot/dts/rockchip/rk3399.dtsi
If I'm not mistaken, this will try to set the first clock
of hdmi@ff940000 there, which is "iahb".
- arch/arm/boot/dts/sun8i-a33.dtsi
There "&ccu CLK_BUS_CODEC" of codec@1c22e00 will be changed
And it doesn't stop there, it also sets the first clock
of the CPU endpoint, which I guess just works because .set_rate
is a noop for the most clocks which are used there.
-michael
On 3/15/2021 5:35 PM, Michael Walle wrote:
> External email: Use caution opening links or attachments
>
>
> Am 2021-03-12 14:46, schrieb Mark Brown:
>> On Fri, Mar 12, 2021 at 01:30:02PM +0100, Michael Walle wrote:
>>
>>> The card calls set_sysclk(), which eventually ends up in the codec.
>>> The codec therefore, could figure out if it needs to configure the
>>> clock or if it can use its internal FLL.
>>> Is that what you mean?
>>
>> Yes.
>>
>>> But the set_sysclk() of the codec isn't even called, because the
>>> card itself already tries to call clk_set_rate() on the Codec's MCLK,
>>> which returns with an error [0].
>>
>> OK, so I think we need to push this down a level so that the clock
>> setting is implemented by the core/CODEC rather than by simple-card,
>> with the helpers being something the CODEC can opt out of.
>
> Sameer, it looks like the proper fix should be to add the clock
> support to your codec.
I agree that complicated clock relationships should be handled within
the codec itself, however MCLK rate setting depends on "mclk-fs" factor
and this property is specified as part of simple-card/audio-graph-card
codec subnode. Right now codec, in general, does not have a way to know
this. The set_sysclk() callback takes rate argument and not the factor.
Moreover the same codec is used by other platform vendors too and unless
a new DT property is added for codec, runtime MCLK update based on the
scaling factor cannot be supported. This would mean that we will be
having two methods to specify "mclk-fs" factor, one from
simple-card/audio-graph-card and one from respective codec nodes, which
does not seem ideal.
Also it does not seem consistent with the way we handle MCLK clock based
on where it is specified.
a) If specified in simple-card/audio-graph-card, MCLK clock
rate/enable/disable updates are allowed.
b) If specified in codec device node, it is not expected to touch the
MCLK clock. This patch tried to treat it the same way as (a) does.
Advantage of this is, all codec drivers need not explicitly handle MCLK,
instead it is done at a central place. The platforms which use specific
machine drivers do the same and that is why probably the codec driver
patch was never required. It is about just setting MCLK clock as a
factor of sample rate, whenever the factor is available. I understand
that it is breaking your use case, but I am not sure if the usage of
set_sysclk() is consistent. I mean, it takes the "freq" argument. Does
it refer to MCLK rate or system-clock (sysclk) rate? MCLK and sysclk are
not really the same when codec PLL is involved. So I would like to
understand clearly about what "freq" argument means. Also when "mclk-fs"
factor is specified, is it related to MCLK or sysclk? My understanding
is that it should be strictly viewed as related to MCLK.
Does it help if a separate helper is used for audio-graph-card with
current change and reverting the simple-card to its previous state?
Morimoto-san, does it affect any other users of audio-graph-card?
>
> I've also looked at other users of "simple-audio-card" and
> it looks like they will break too. For example,
> - arch/arm64/boot/dts/rockchip/rk3399.dtsi
> If I'm not mistaken, this will try to set the first clock
> of hdmi@ff940000 there, which is "iahb".
> - arch/arm/boot/dts/sun8i-a33.dtsi
> There "&ccu CLK_BUS_CODEC" of codec@1c22e00 will be changed
>
> And it doesn't stop there, it also sets the first clock
> of the CPU endpoint, which I guess just works because .set_rate
> is a noop for the most clocks which are used there.
Yes this is a problem, unfortunately I missed checking some of the
simple-card examples. I wonder we should be specifically looking for
"mclk" clock here.
Am 2021-03-15 16:19, schrieb Sameer Pujar:
> On 3/15/2021 5:35 PM, Michael Walle wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> Am 2021-03-12 14:46, schrieb Mark Brown:
>>> On Fri, Mar 12, 2021 at 01:30:02PM +0100, Michael Walle wrote:
>>>
>>>> The card calls set_sysclk(), which eventually ends up in the codec.
>>>> The codec therefore, could figure out if it needs to configure the
>>>> clock or if it can use its internal FLL.
>>>> Is that what you mean?
>>>
>>> Yes.
>>>
>>>> But the set_sysclk() of the codec isn't even called, because the
>>>> card itself already tries to call clk_set_rate() on the Codec's
>>>> MCLK,
>>>> which returns with an error [0].
>>>
>>> OK, so I think we need to push this down a level so that the clock
>>> setting is implemented by the core/CODEC rather than by simple-card,
>>> with the helpers being something the CODEC can opt out of.
>>
>> Sameer, it looks like the proper fix should be to add the clock
>> support to your codec.
>
> I agree that complicated clock relationships should be handled within
> the codec itself, however MCLK rate setting depends on "mclk-fs"
> factor and this property is specified as part of
> simple-card/audio-graph-card codec subnode. Right now codec, in
> general, does not have a way to know this. The set_sysclk() callback
> takes rate argument and not the factor.
Why would you need the factor?
> Moreover the same codec is
> used by other platform vendors too and unless a new DT property is
> added for codec, runtime MCLK update based on the scaling factor
> cannot be supported.
Could you test the following:
diff --git a/sound/soc/codecs/rt5659.c b/sound/soc/codecs/rt5659.c
index 67f0ab817135..7fd41f51f856 100644
--- a/sound/soc/codecs/rt5659.c
+++ b/sound/soc/codecs/rt5659.c
@@ -3426,12 +3426,18 @@ static int rt5659_set_component_sysclk(struct
snd_soc_component *component, int
{
struct rt5659_priv *rt5659 =
snd_soc_component_get_drvdata(component);
unsigned int reg_val = 0;
+ int ret;
if (freq == rt5659->sysclk && clk_id == rt5659->sysclk_src)
return 0;
switch (clk_id) {
case RT5659_SCLK_S_MCLK:
+ ret = clk_set_rate(rt5659->mclk, freq);
+ if (ret)
+ return ret;
reg_val |= RT5659_SCLK_SRC_MCLK;
break;
case RT5659_SCLK_S_PLL1:
-michael
> This would mean that we will be having two
> methods to specify "mclk-fs" factor, one from
> simple-card/audio-graph-card and one from respective codec nodes,
> which does not seem ideal.
>
> Also it does not seem consistent with the way we handle MCLK clock
> based on where it is specified.
>
> a) If specified in simple-card/audio-graph-card, MCLK clock
> rate/enable/disable updates are allowed.
>
> b) If specified in codec device node, it is not expected to touch
> the MCLK clock. This patch tried to treat it the same way as (a) does.
> Advantage of this is, all codec drivers need not explicitly handle
> MCLK, instead it is done at a central place. The platforms which use
> specific machine drivers do the same and that is why probably the
> codec driver patch was never required. It is about just setting MCLK
> clock as a factor of sample rate, whenever the factor is available. I
> understand that it is breaking your use case, but I am not sure if the
> usage of set_sysclk() is consistent. I mean, it takes the "freq"
> argument. Does it refer to MCLK rate or system-clock (sysclk) rate?
> MCLK and sysclk are not really the same when codec PLL is involved. So
> I would like to understand clearly about what "freq" argument means.
> Also when "mclk-fs" factor is specified, is it related to MCLK or
> sysclk? My understanding is that it should be strictly viewed as
> related to MCLK.
>
>
> Does it help if a separate helper is used for audio-graph-card with
> current change and reverting the simple-card to its previous state?
> Morimoto-san, does it affect any other users of audio-graph-card?
>
>>
>> I've also looked at other users of "simple-audio-card" and
>> it looks like they will break too. For example,
>> - arch/arm64/boot/dts/rockchip/rk3399.dtsi
>> If I'm not mistaken, this will try to set the first clock
>> of hdmi@ff940000 there, which is "iahb".
>> - arch/arm/boot/dts/sun8i-a33.dtsi
>> There "&ccu CLK_BUS_CODEC" of codec@1c22e00 will be changed
>>
>> And it doesn't stop there, it also sets the first clock
>> of the CPU endpoint, which I guess just works because .set_rate
>> is a noop for the most clocks which are used there.
>
> Yes this is a problem, unfortunately I missed checking some of the
> simple-card examples. I wonder we should be specifically looking for
> "mclk" clock here.
On Mon, Mar 15, 2021 at 08:49:00PM +0530, Sameer Pujar wrote:
> On 3/15/2021 5:35 PM, Michael Walle wrote:
> > Am 2021-03-12 14:46, schrieb Mark Brown:
> > Sameer, it looks like the proper fix should be to add the clock
> > support to your codec.
> I agree that complicated clock relationships should be handled within the
> codec itself, however MCLK rate setting depends on "mclk-fs" factor and this
> property is specified as part of simple-card/audio-graph-card codec subnode.
> Right now codec, in general, does not have a way to know this. The
> set_sysclk() callback takes rate argument and not the factor. Moreover the
I just don't understand what you're saying here at all. At the point
where the card is setting the clock API clock rate it can just as well
set a sysclk, these are equivalent operations.
> same codec is used by other platform vendors too and unless a new DT
> property is added for codec, runtime MCLK update based on the scaling factor
> cannot be supported. This would mean that we will be having two methods to
> specify "mclk-fs" factor, one from simple-card/audio-graph-card and one from
> respective codec nodes, which does not seem ideal.
Again I just can't follow what you're saying here at all. Again, if the
card is able to set a clock API clock rate it can just as well set a
clock rate via sysclk.
> Yes this is a problem, unfortunately I missed checking some of the
> simple-card examples. I wonder we should be specifically looking for "mclk"
> clock here.
That would definitely help mitigate the problem but I really think it's
cleaner and safer to just push this down to set_sysclk().
On 3/15/2021 9:03 PM, Michael Walle wrote:
> Am 2021-03-15 16:19, schrieb Sameer Pujar:
>> On 3/15/2021 5:35 PM, Michael Walle wrote:
>>> Am 2021-03-12 14:46, schrieb Mark Brown:
>>>> On Fri, Mar 12, 2021 at 01:30:02PM +0100, Michael Walle wrote:
>>>>
>>>>> The card calls set_sysclk(), which eventually ends up in the codec.
>>>>> The codec therefore, could figure out if it needs to configure the
>>>>> clock or if it can use its internal FLL.
>>>>> Is that what you mean?
>>>>
>>>> Yes.
>>>>
>>>>> But the set_sysclk() of the codec isn't even called, because the
>>>>> card itself already tries to call clk_set_rate() on the Codec's
>>>>> MCLK,
>>>>> which returns with an error [0].
>>>>
>>>> OK, so I think we need to push this down a level so that the clock
>>>> setting is implemented by the core/CODEC rather than by simple-card,
>>>> with the helpers being something the CODEC can opt out of.
>>>
>>> Sameer, it looks like the proper fix should be to add the clock
>>> support to your codec.
>>
>> I agree that complicated clock relationships should be handled within
>> the codec itself, however MCLK rate setting depends on "mclk-fs"
>> factor and this property is specified as part of
>> simple-card/audio-graph-card codec subnode. Right now codec, in
>> general, does not have a way to know this. The set_sysclk() callback
>> takes rate argument and not the factor.
>
> Why would you need the factor?
Sorry, you are right. I misread the flow. Fixed rates can use
"assigned-clocks" binding and runtime updates can depend on "mclk-fs".
No additional property is required for codec. Something like below
should help.
>
> diff --git a/sound/soc/codecs/rt5659.c b/sound/soc/codecs/rt5659.c
> index 67f0ab817135..7fd41f51f856 100644
> --- a/sound/soc/codecs/rt5659.c
> +++ b/sound/soc/codecs/rt5659.c
> @@ -3426,12 +3426,18 @@ static int rt5659_set_component_sysclk(struct
> snd_soc_component *component, int
> {
> struct rt5659_priv *rt5659 =
> snd_soc_component_get_drvdata(component);
> unsigned int reg_val = 0;
> + int ret;
>
> if (freq == rt5659->sysclk && clk_id == rt5659->sysclk_src)
> return 0;
>
> switch (clk_id) {
> case RT5659_SCLK_S_MCLK:
> + ret = clk_set_rate(rt5659->mclk, freq);
> + if (ret)
> + return ret;
> reg_val |= RT5659_SCLK_SRC_MCLK;
> break;
> case RT5659_SCLK_S_PLL1:
>> Yes this is a problem, unfortunately I missed checking some of the
>> simple-card examples. I wonder we should be specifically looking for "mclk"
>> clock here.
> That would definitely help mitigate the problem but I really think it's
> cleaner and safer to just push this down to set_sysclk().
Understand now. I will push patches based on this. Thanks for the details.
Am 2021-03-15 18:10, schrieb Sameer Pujar:
>>> Yes this is a problem, unfortunately I missed checking some of the
>>> simple-card examples. I wonder we should be specifically looking for
>>> "mclk"
>>> clock here.
>> That would definitely help mitigate the problem but I really think
>> it's
>> cleaner and safer to just push this down to set_sysclk().
>
> Understand now. I will push patches based on this. Thanks for the
> details.
Please keep me on CC, I'm not subscribed to the sound/alsa mailinglist.
Thanks,
-michael