Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
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
Re: Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
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.
Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
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:
Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
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(). signature.asc Description: PGP signature
Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
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@ff94 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.
Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
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@ff94 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.
Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
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@ff94 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
Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
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. signature.asc Description: PGP signature
Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
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
Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
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. signature.asc Description: PGP signature
Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module 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
Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
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. signature.asc Description: PGP signature
Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
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
Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
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
Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
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. signature.asc Description: PGP signature
Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
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. signature.asc Description: PGP signature
Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
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. signature.asc Description: PGP signature
Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
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
Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
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/1611944866-29373-4-git-send-email-spu...@nvidia.com/
Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
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/1611944866-29373-4-git-send-email-spu...@nvidia.com/
Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
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/1611944866-29373-4-git-send-email-spu...@nvidia.com/
Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
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
Signed-off-by: Sameer Pujar
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
Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
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.
Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
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
Signed-off-by: Sameer Pujar
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
Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
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
Signed-off-by: Sameer Pujar
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.
Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
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
Signed-off-by: Sameer Pujar
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
Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
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
Signed-off-by: Sameer Pujar
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
[PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
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
> Signed-off-by: Sameer Pujar
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
>
>
Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
Hi Sameer > > /* > > * Parse dai->sysclk come from "clocks = <&xxx>" > > * (if system has common clock) > > * or "system-clock-frequency = " > > * 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
Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
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 = "
* 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"))
Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
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 = "
* 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
[PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
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
Signed-off-by: Sameer Pujar
---
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
