| Message ID | 1612939421-19900-2-git-send-email-spujar@nvidia.com |
|---|---|
| State | Accepted |
| Commit | 1e30f642cf2939bbdac82ea0dd3071232670b5ab |
| Headers | show |
| Series | Use clocks property in a device node | expand |
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 <kuninori.morimoto.gx@renesas.com> > Signed-off-by: Sameer Pujar <spujar@nvidia.com> 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 <kuninori.morimoto.gx@renesas.com> >> Signed-off-by: Sameer Pujar <spujar@nvidia.com> > 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 <kuninori.morimoto.gx@renesas.com> >>> Signed-off-by: Sameer Pujar <spujar@nvidia.com> >> >> 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 <kuninori.morimoto.gx@renesas.com> >>>> Signed-off-by: Sameer Pujar <spujar@nvidia.com> >>> >>> 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 <kuninori.morimoto.gx@renesas.com> >>>>> Signed-off-by: Sameer Pujar <spujar@nvidia.com> >>>> >>>> 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 <kuninori.morimoto.gx@renesas.com> >>>>> Signed-off-by: Sameer Pujar <spujar@nvidia.com> >>>> >>>> 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/1611944866-29373-4-git-send-email-spujar@nvidia.com/
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-spujar@nvidia.com/
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-spujar@nvidia.com/ >
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
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
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 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().
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
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"))
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 <kuninori.morimoto.gx@renesas.com> Signed-off-by: Sameer Pujar <spujar@nvidia.com> --- sound/soc/generic/simple-card-utils.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)