| Message ID | 20230705190324.355282-2-andreas@kemnade.info (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: omap4: embt2ws: Add audio support | expand |
On 7/5/23 22:03, Andreas Kemnade wrote: > Commit 349355ce3a05 ("ARM: OMAP2+: Drop legacy platform data for omap4 mcbsp") > dropped prcm_fck for omap4, it also dropped the pad_fck for that matter. > so the clk_src might not be >available making the > clk_get(src) fail. Wow, so OMAP4 audio is pretty broken if would ever need to select FCLK? By default we don't on OMAP4, but this is astonishing. > In such cases, rely on the devicetree to assign > the correct parent. You cannot rely on DT to dynamically select the FCLK parent for different use cases. The dai_set_dai_sysclk() cannot select between internal or external source of the reference clock and DT cannot handle this. If one sampling frequency is available with pad_fck while other is only possible with internal clock then this is no longer possible. > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > --- > sound/soc/ti/omap-mcbsp.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/sound/soc/ti/omap-mcbsp.c b/sound/soc/ti/omap-mcbsp.c > index 21fa7b9787997..f9fe96b61852b 100644 > --- a/sound/soc/ti/omap-mcbsp.c > +++ b/sound/soc/ti/omap-mcbsp.c > @@ -70,8 +70,8 @@ static int omap2_mcbsp_set_clks_src(struct omap_mcbsp *mcbsp, u8 fck_src_id) > > fck_src = clk_get(mcbsp->dev, src); > if (IS_ERR(fck_src)) { > - dev_err(mcbsp->dev, "CLKS: could not clk_get() %s\n", src); > - return -EINVAL; > + dev_info(mcbsp->dev, "CLKS: could not clk_get() %s\n", src); > + return 0; I would rather have some clock alias for OMAP4/5 to provide the clocks that we need for the fclk. If we did not got the clock we needed to select we cannot say that all is good, carry on. Normally the machine driver does this and it thinks that we switched clocks while we did not and the clocking is all wrong now. > } > > pm_runtime_put_sync(mcbsp->dev);
Hi, * Péter Ujfalusi <peter.ujfalusi@gmail.com> [230919 18:25]: > > > On 7/5/23 22:03, Andreas Kemnade wrote: > > Commit 349355ce3a05 ("ARM: OMAP2+: Drop legacy platform data for omap4 mcbsp") > > dropped prcm_fck for omap4, The prcm_fck should be there in the dts for each mcbsp interconnect targe module as "fck" alias and get's enabled/disabled when the mcbsp driver calls runtime PM. So maybe the description should explain that things broke as the aliases for prcm_fck and pad_ck no longer get added. > it also dropped the pad_fck for that matter. OK so an alias is needed for that too. That's the MCPDM_CLKCTRL pad_clks_ck alias, right? Seems like the pad_clks_ck should be claimed by the mcpdm and mcbsp drivers if they are used? I guess it would be for some external device use case? > > so the clk_src might not be >available making the > > clk_get(src) fail. > > Wow, so OMAP4 audio is pretty broken if would ever need to select FCLK? > By default we don't on OMAP4, but this is astonishing. So sounds like we just got lucky because of -ENOSUCHUSERS? The mcbsp works for me, not sure how come I'm not seeing this issue, does it now only work for the default clock source? Do we have some other examples for devices using other type of clocking? > > In such cases, rely on the devicetree to assign > > the correct parent. > > You cannot rely on DT to dynamically select the FCLK parent for > different use cases. > The dai_set_dai_sysclk() cannot select between internal or external > source of the reference clock and DT cannot handle this. > If one sampling frequency is available with pad_fck while other is only > possible with internal clock then this is no longer possible. If the functional clock source needs to be changed, seems things should work. The parent interconnect target module driver does not care about the fck rate. Not sure if the clock usage count causes issues though with reparenting, if so, sounds like reparenting needs to be done in runtime PM suspended state to do the switch. > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > > --- > > sound/soc/ti/omap-mcbsp.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/sound/soc/ti/omap-mcbsp.c b/sound/soc/ti/omap-mcbsp.c > > index 21fa7b9787997..f9fe96b61852b 100644 > > --- a/sound/soc/ti/omap-mcbsp.c > > +++ b/sound/soc/ti/omap-mcbsp.c > > @@ -70,8 +70,8 @@ static int omap2_mcbsp_set_clks_src(struct omap_mcbsp *mcbsp, u8 fck_src_id) > > > > fck_src = clk_get(mcbsp->dev, src); > > if (IS_ERR(fck_src)) { > > - dev_err(mcbsp->dev, "CLKS: could not clk_get() %s\n", src); > > - return -EINVAL; > > + dev_info(mcbsp->dev, "CLKS: could not clk_get() %s\n", src); > > + return 0; > > I would rather have some clock alias for OMAP4/5 to provide the clocks > that we need for the fclk. > If we did not got the clock we needed to select we cannot say that all > is good, carry on. > Normally the machine driver does this and it thinks that we switched > clocks while we did not and the clocking is all wrong now. How about someting like below to deal with getting the fck in a bit more future proof way: fck_src = clk_get(mcbsp->dev, src); if (IS_ERR(fck_src)) { fck_src = clk_get(mcbsp->dev->parent, "fck): ... } Also sounds like we should also add the missing the aliases too like Peter pointed out if mcbsp driver needs to reparent. But just adding the alias will possibly cause trouble for anybody adding mcbsp support for some other SoC variant. To me it seems in the long run the mcbsp configurations using pad_fck should configure it in the dts for the interconnect target module node instead as the default clock. Then mcbsp can still look up the fck of the parent device, and change it dynamically as needed. Regards, Tony
Hi, On Wed, 20 Sep 2023 09:33:53 +0300 Tony Lindgren <tony@atomide.com> wrote: > Hi, > > * Péter Ujfalusi <peter.ujfalusi@gmail.com> [230919 18:25]: > > > > > > On 7/5/23 22:03, Andreas Kemnade wrote: > > > Commit 349355ce3a05 ("ARM: OMAP2+: Drop legacy platform data for omap4 mcbsp") > > > dropped prcm_fck for omap4, > > The prcm_fck should be there in the dts for each mcbsp interconnect targe > module as "fck" alias and get's enabled/disabled when the mcbsp driver > calls runtime PM. > > So maybe the description should explain that things broke as the aliases > for prcm_fck and pad_ck no longer get added. > > > it also dropped the pad_fck for that matter. > > OK so an alias is needed for that too. > > That's the MCPDM_CLKCTRL pad_clks_ck alias, right? Seems like the > pad_clks_ck should be claimed by the mcpdm and mcbsp drivers if they are > used? I guess it would be for some external device use case? > > > > so the clk_src might not be >available making the > > > clk_get(src) fail. > > > > Wow, so OMAP4 audio is pretty broken if would ever need to select FCLK? > > By default we don't on OMAP4, but this is astonishing. > > So sounds like we just got lucky because of -ENOSUCHUSERS? The mcbsp works > for me, not sure how come I'm not seeing this issue, does it now only work > for the default clock source? > Well, I did not run into any problems (besides of no sound output) as long as I tried to use the codec side as bitclock/frameclock-master and that is what droid4 does... Regards, Andreas
Hi, On 9/20/23 17:52, Andreas Kemnade wrote: > Hi, > > On Wed, 20 Sep 2023 09:33:53 +0300 > Tony Lindgren <tony@atomide.com> wrote: > >> Hi, >> >> * Péter Ujfalusi <peter.ujfalusi@gmail.com> [230919 18:25]: >>> >>> >>> On 7/5/23 22:03, Andreas Kemnade wrote: >>>> Commit 349355ce3a05 ("ARM: OMAP2+: Drop legacy platform data for omap4 mcbsp") >>>> dropped prcm_fck for omap4, >> >> The prcm_fck should be there in the dts for each mcbsp interconnect targe >> module as "fck" alias and get's enabled/disabled when the mcbsp driver >> calls runtime PM. >> >> So maybe the description should explain that things broke as the aliases >> for prcm_fck and pad_ck no longer get added. >> >>> it also dropped the pad_fck for that matter. >> >> OK so an alias is needed for that too. >> >> That's the MCPDM_CLKCTRL pad_clks_ck alias, right? Seems like the >> pad_clks_ck should be claimed by the mcpdm and mcbsp drivers if they are >> used? I guess it would be for some external device use case? >> >>>> so the clk_src might not be >available making the >>>> clk_get(src) fail. >>> >>> Wow, so OMAP4 audio is pretty broken if would ever need to select FCLK? >>> By default we don't on OMAP4, but this is astonishing. >> >> So sounds like we just got lucky because of -ENOSUCHUSERS? The mcbsp works >> for me, not sure how come I'm not seeing this issue, does it now only work >> for the default clock source? >> > Well, I did not run into any problems (besides of no sound output) > as long as I tried to use the codec side as bitclock/frameclock-master and > that is what droid4 does... Looks like only omap3 pandora is using external clock source for McBSP, most OMAP4/5 devices tend to use McPDM and in most cases the McBSP is not the clock provider, so the clocking does not matter. It is going to cause issues on new boards, but those are unlikely to surface.
On 9/20/23 09:33, Tony Lindgren wrote: > Hi, > > * Péter Ujfalusi <peter.ujfalusi@gmail.com> [230919 18:25]: >> >> >> On 7/5/23 22:03, Andreas Kemnade wrote: >>> Commit 349355ce3a05 ("ARM: OMAP2+: Drop legacy platform data for omap4 mcbsp") >>> dropped prcm_fck for omap4, > > The prcm_fck should be there in the dts for each mcbsp interconnect targe > module as "fck" alias and get's enabled/disabled when the mcbsp driver > calls runtime PM. > > So maybe the description should explain that things broke as the aliases > for prcm_fck and pad_ck no longer get added. > >> it also dropped the pad_fck for that matter. > > OK so an alias is needed for that too. Something like that, yes. > That's the MCPDM_CLKCTRL pad_clks_ck alias, right? Seems like the > pad_clks_ck should be claimed by the mcpdm and mcbsp drivers if they are > used? I guess it would be for some external device use case? > >>> so the clk_src might not be >available making the >>> clk_get(src) fail. >> >> Wow, so OMAP4 audio is pretty broken if would ever need to select FCLK? >> By default we don't on OMAP4, but this is astonishing. > > So sounds like we just got lucky because of -ENOSUCHUSERS? The mcbsp works > for me, not sure how come I'm not seeing this issue, does it now only work > for the default clock source? It will be only a problem if McBSP is the clock provider and we need to change to external reference clock for the BCLK/FSYNC generation. In most board designs the codec provides the clocks - they tend to be more flexible when it comes to clock source. > Do we have some other examples for devices using other type of clocking? > >>> In such cases, rely on the devicetree to assign >>> the correct parent. >> >> You cannot rely on DT to dynamically select the FCLK parent for >> different use cases. >> The dai_set_dai_sysclk() cannot select between internal or external >> source of the reference clock and DT cannot handle this. >> If one sampling frequency is available with pad_fck while other is only >> possible with internal clock then this is no longer possible. > > If the functional clock source needs to be changed, seems things > should work. The parent interconnect target module driver does not care > about the fck rate. Not sure if the clock usage count causes issues > though with reparenting, if so, sounds like reparenting needs to be > done in runtime PM suspended state to do the switch. > >>> Signed-off-by: Andreas Kemnade <andreas@kemnade.info> >>> --- >>> sound/soc/ti/omap-mcbsp.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/sound/soc/ti/omap-mcbsp.c b/sound/soc/ti/omap-mcbsp.c >>> index 21fa7b9787997..f9fe96b61852b 100644 >>> --- a/sound/soc/ti/omap-mcbsp.c >>> +++ b/sound/soc/ti/omap-mcbsp.c >>> @@ -70,8 +70,8 @@ static int omap2_mcbsp_set_clks_src(struct omap_mcbsp *mcbsp, u8 fck_src_id) >>> >>> fck_src = clk_get(mcbsp->dev, src); >>> if (IS_ERR(fck_src)) { >>> - dev_err(mcbsp->dev, "CLKS: could not clk_get() %s\n", src); >>> - return -EINVAL; >>> + dev_info(mcbsp->dev, "CLKS: could not clk_get() %s\n", src); >>> + return 0; >> >> I would rather have some clock alias for OMAP4/5 to provide the clocks >> that we need for the fclk. >> If we did not got the clock we needed to select we cannot say that all >> is good, carry on. >> Normally the machine driver does this and it thinks that we switched >> clocks while we did not and the clocking is all wrong now. > > How about someting like below to deal with getting the fck in a bit more > future proof way: > > fck_src = clk_get(mcbsp->dev, src); > if (IS_ERR(fck_src)) { > fck_src = clk_get(mcbsp->dev->parent, "fck): > ... > } It is not the parent's fck, it is the PRCM clock which is selected as the sourcee of the clock generator (CLKS) for BCLK/FSYNC. That is the functional clock as well for the McBSP instance. > Also sounds like we should also add the missing the aliases too like Peter > pointed out if mcbsp driver needs to reparent. But just adding the alias > will possibly cause trouble for anybody adding mcbsp support for some other > SoC variant. > > To me it seems in the long run the mcbsp configurations using pad_fck > should configure it in the dts for the interconnect target module node > instead as the default clock. Then mcbsp can still look up the fck > of the parent device, and change it dynamically as needed. Out of reset it is using the PRCM source which is fine in all current users. I would do this fix or workaround in a different way: instead of ignoring the error, avoid it in the first place. Do nothing if the already selected clock is requested. That would remove the error and will fail in case the reparenting is not working -> boards will know this and might be able to do something about it in a reasonable way. How that sounds? > Regards, > > Tony >
* Péter Ujfalusi <peter.ujfalusi@gmail.com> [230920 17:40]: > It is not the parent's fck, it is the PRCM clock which is selected as > the sourcee of the clock generator (CLKS) for BCLK/FSYNC. That is the > functional clock as well for the McBSP instance. Oh OK > Out of reset it is using the PRCM source which is fine in all current users. > I would do this fix or workaround in a different way: instead of > ignoring the error, avoid it in the first place. Do nothing if the > already selected clock is requested. > That would remove the error and will fail in case the reparenting is not > working -> boards will know this and might be able to do something about > it in a reasonable way. > > How that sounds? Sounds good to me :) Tony
* Tony Lindgren <tony@atomide.com> [230921 20:34]: > * Péter Ujfalusi <peter.ujfalusi@gmail.com> [230920 17:40]: > > It is not the parent's fck, it is the PRCM clock which is selected as > > the sourcee of the clock generator (CLKS) for BCLK/FSYNC. That is the > > functional clock as well for the McBSP instance. > > Oh OK > > > Out of reset it is using the PRCM source which is fine in all current users. > > I would do this fix or workaround in a different way: instead of > > ignoring the error, avoid it in the first place. Do nothing if the > > already selected clock is requested. > > That would remove the error and will fail in case the reparenting is not > > working -> boards will know this and might be able to do something about > > it in a reasonable way. Here's what I think the regression fix for omap4 clocks would be, the old main_clk is not the same as the module clock that we get by default. If this looks OK I'll do a similar fix also for omap5. Or is something else also needed? Regards, Tony 8< ----------------------------- diff --git a/arch/arm/boot/dts/ti/omap/omap4-l4-abe.dtsi b/arch/arm/boot/dts/ti/omap/omap4-l4-abe.dtsi --- a/arch/arm/boot/dts/ti/omap/omap4-l4-abe.dtsi +++ b/arch/arm/boot/dts/ti/omap/omap4-l4-abe.dtsi @@ -109,6 +109,8 @@ mcbsp1: mcbsp@0 { reg = <0x0 0xff>, /* MPU private access */ <0x49022000 0xff>; /* L3 Interconnect */ reg-names = "mpu", "dma"; + clocks = <&abe_clkctrl OMAP4_MCBSP1_CLKCTRL 24>; + clock-names = "fck"; interrupts = <GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>; interrupt-names = "common"; ti,buffer-size = <128>; @@ -142,6 +144,8 @@ mcbsp2: mcbsp@0 { reg = <0x0 0xff>, /* MPU private access */ <0x49024000 0xff>; /* L3 Interconnect */ reg-names = "mpu", "dma"; + clocks = <&abe_clkctrl OMAP4_MCBSP2_CLKCTRL 24>; + clock-names = "fck"; interrupts = <GIC_SPI 22 IRQ_TYPE_LEVEL_HIGH>; interrupt-names = "common"; ti,buffer-size = <128>; @@ -175,6 +179,8 @@ mcbsp3: mcbsp@0 { reg = <0x0 0xff>, /* MPU private access */ <0x49026000 0xff>; /* L3 Interconnect */ reg-names = "mpu", "dma"; + clocks = <&abe_clkctrl OMAP4_MCBSP3_CLKCTRL 24>; + clock-names = "fck"; interrupts = <GIC_SPI 23 IRQ_TYPE_LEVEL_HIGH>; interrupt-names = "common"; ti,buffer-size = <128>; diff --git a/arch/arm/boot/dts/ti/omap/omap4-l4.dtsi b/arch/arm/boot/dts/ti/omap/omap4-l4.dtsi --- a/arch/arm/boot/dts/ti/omap/omap4-l4.dtsi +++ b/arch/arm/boot/dts/ti/omap/omap4-l4.dtsi @@ -2043,6 +2043,8 @@ mcbsp4: mcbsp@0 { compatible = "ti,omap4-mcbsp"; reg = <0x0 0xff>; /* L4 Interconnect */ reg-names = "mpu"; + clocks = <&l4_per_clkctrl OMAP4_MCBSP4_CLKCTRL 24>; + clock-names = "fck"; interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>; interrupt-names = "common"; ti,buffer-size = <128>; diff --git a/drivers/clk/ti/clk-44xx.c b/drivers/clk/ti/clk-44xx.c --- a/drivers/clk/ti/clk-44xx.c +++ b/drivers/clk/ti/clk-44xx.c @@ -749,9 +749,14 @@ static struct ti_dt_clk omap44xx_clks[] = { DT_CLK(NULL, "mcbsp1_sync_mux_ck", "abe-clkctrl:0028:26"), DT_CLK(NULL, "mcbsp2_sync_mux_ck", "abe-clkctrl:0030:26"), DT_CLK(NULL, "mcbsp3_sync_mux_ck", "abe-clkctrl:0038:26"), + DT_CLK("40122000.mcbsp", "prcm_fck", "abe-clkctrl:0028:26"), + DT_CLK("40124000.mcbsp", "prcm_fck", "abe-clkctrl:0030:26"), + DT_CLK("40126000.mcbsp", "prcm_fck", "abe-clkctrl:0038:26"), DT_CLK(NULL, "mcbsp4_sync_mux_ck", "l4-per-clkctrl:00c0:26"), + DT_CLK("48096000.mcbsp", "prcm_fck", "l4-per-clkctrl:00c0:26"), DT_CLK(NULL, "ocp2scp_usb_phy_phy_48m", "l3-init-clkctrl:00c0:8"), DT_CLK(NULL, "otg_60m_gfclk", "l3-init-clkctrl:0040:24"), + DT_CLK(NULL, "pad_fck", "pad_clks_ck"), DT_CLK(NULL, "per_mcbsp4_gfclk", "l4-per-clkctrl:00c0:24"), DT_CLK(NULL, "pmd_stm_clock_mux_ck", "emu-sys-clkctrl:0000:20"), DT_CLK(NULL, "pmd_trace_clk_mux_ck", "emu-sys-clkctrl:0000:22"),
On Fri, 6 Oct 2023 13:23:48 +0300 Tony Lindgren <tony@atomide.com> wrote: > * Tony Lindgren <tony@atomide.com> [230921 20:34]: > > * Péter Ujfalusi <peter.ujfalusi@gmail.com> [230920 17:40]: > > > It is not the parent's fck, it is the PRCM clock which is selected as > > > the sourcee of the clock generator (CLKS) for BCLK/FSYNC. That is the > > > functional clock as well for the McBSP instance. > > > > Oh OK > > > > > Out of reset it is using the PRCM source which is fine in all current users. > > > I would do this fix or workaround in a different way: instead of > > > ignoring the error, avoid it in the first place. Do nothing if the > > > already selected clock is requested. > > > That would remove the error and will fail in case the reparenting is not > > > working -> boards will know this and might be able to do something about > > > it in a reasonable way. > > Here's what I think the regression fix for omap4 clocks would be, the > old main_clk is not the same as the module clock that we get by default. > If this looks OK I'll do a similar fix also for omap5. > > Or is something else also needed? > hmm, audio output works, the waring is away, but something new is here: omap-mcbsp 40124000.mcbsp: Runtime PM usage count underflow! # cat /sys/bus/platform/devices/40124000.mcbsp/power/runtime_status active even with no sound. Regards, Andreas
* Andreas Kemnade <andreas@kemnade.info> [231006 19:30]: > On Fri, 6 Oct 2023 13:23:48 +0300 > Tony Lindgren <tony@atomide.com> wrote: > > Here's what I think the regression fix for omap4 clocks would be, the > > old main_clk is not the same as the module clock that we get by default. > > If this looks OK I'll do a similar fix also for omap5. > > > > Or is something else also needed? > > > > hmm, > audio output works, the waring is away, but something new is here: OK good to hear it works, I'll send out fixes for omap4 and 5, seems the runtime PM warning is something different. > omap-mcbsp 40124000.mcbsp: Runtime PM usage count underflow! > # cat /sys/bus/platform/devices/40124000.mcbsp/power/runtime_status > active > > even with no sound. I guess if the mcbsp instance is not used, runtime PM is enabled but pm_runtime_resume_and_get() is never called by ASoC? If so then the following might be a fix, not familiar with runtime PM done by ASoC though and not sure if some kind of locking would be needed here. Regards, Tony 8< ------------------------ diff --git a/sound/soc/ti/omap-mcbsp.c b/sound/soc/ti/omap-mcbsp.c index fdabed5133e83..b399d86f22777 100644 --- a/sound/soc/ti/omap-mcbsp.c +++ b/sound/soc/ti/omap-mcbsp.c @@ -74,14 +74,16 @@ static int omap2_mcbsp_set_clks_src(struct omap_mcbsp *mcbsp, u8 fck_src_id) return 0; } - pm_runtime_put_sync(mcbsp->dev); + if (mcbsp->active) + pm_runtime_put_sync(mcbsp->dev); r = clk_set_parent(mcbsp->fclk, fck_src); if (r) dev_err(mcbsp->dev, "CLKS: could not clk_set_parent() to %s\n", src); - pm_runtime_get_sync(mcbsp->dev); + if (mcbsp->active) + pm_runtime_get_sync(mcbsp->dev); clk_put(fck_src);
On Sat, 7 Oct 2023 09:25:18 +0300 Tony Lindgren <tony@atomide.com> wrote: > * Andreas Kemnade <andreas@kemnade.info> [231006 19:30]: > > On Fri, 6 Oct 2023 13:23:48 +0300 > > Tony Lindgren <tony@atomide.com> wrote: > > > Here's what I think the regression fix for omap4 clocks would be, the > > > old main_clk is not the same as the module clock that we get by default. > > > If this looks OK I'll do a similar fix also for omap5. > > > > > > Or is something else also needed? > > > > > > > hmm, > > audio output works, the waring is away, but something new is here: > > OK good to hear it works, I'll send out fixes for omap4 and 5, seems > the runtime PM warning is something different. > > > omap-mcbsp 40124000.mcbsp: Runtime PM usage count underflow! > > # cat /sys/bus/platform/devices/40124000.mcbsp/power/runtime_status > > active > > > > even with no sound. > Well, it is a regression caused by your fix. Without it (and not reverting the already applied ignore patch), runtime is properly suspended. Don't know why yet. Regards, Andreas
* Andreas Kemnade <andreas@kemnade.info> [231007 07:12]: > Well, it is a regression caused by your fix. Without it (and not reverting > the already applied ignore patch), runtime is properly suspended. Don't know > why yet. We return early from omap2_mcbsp_set_clks_src() with IS_ERR(fck_src) and the runtime PM functions never get called? Regards, Tony
On Sat, 7 Oct 2023 10:41:59 +0300 Tony Lindgren <tony@atomide.com> wrote: > * Andreas Kemnade <andreas@kemnade.info> [231007 07:12]: > > Well, it is a regression caused by your fix. Without it (and not reverting > > the already applied ignore patch), runtime is properly suspended. Don't know > > why yet. > > We return early from omap2_mcbsp_set_clks_src() with IS_ERR(fck_src) and > the runtime PM functions never get called? > no, we do not. This patch we are talking about to do it in a better way made its way into mainline v6.6-rc1. The other pieces of sound support did not, they need rework. Regards, Andreas
On 07/10/2023 10:11, Andreas Kemnade wrote: >> OK good to hear it works, I'll send out fixes for omap4 and 5, seems >> the runtime PM warning is something different. >> >>> omap-mcbsp 40124000.mcbsp: Runtime PM usage count underflow! >>> # cat /sys/bus/platform/devices/40124000.mcbsp/power/runtime_status >>> active >>> >>> even with no sound. >> > Well, it is a regression caused by your fix. Without it (and not reverting > the already applied ignore patch), runtime is properly suspended. Don't know > why yet. I guess it is because of the pm_runtime_put_sync() in the omap2_mcbsp_set_clks_src() around the fclk re-parenting. That is a bit dubious thing for sure. We need to disable the device to be able to re-parent the fclk but if we disable the device it is going to be powered down, right? I think we have appropriate context handling, so it might work, but it is certainly not a rock solid code... If you have a stream running already, you don't really want to kill the McBSP. The problem is that this mux is outside of the McBSP IP, so we need a system level (iow, clk API) way to change it runtime. What is the machine driver where this happens? If you set the sysclk in hw_params of the machine driver, it will be OK, but if you do that in probe time then it is likely going to fail as you experienced
On Thu, 12 Oct 2023 17:41:34 +0300 Péter Ujfalusi <peter.ujfalusi@gmail.com> wrote: > On 07/10/2023 10:11, Andreas Kemnade wrote: > >> OK good to hear it works, I'll send out fixes for omap4 and 5, seems > >> the runtime PM warning is something different. > >> > >>> omap-mcbsp 40124000.mcbsp: Runtime PM usage count underflow! > >>> # cat /sys/bus/platform/devices/40124000.mcbsp/power/runtime_status > >>> active > >>> > >>> even with no sound. > >> > > Well, it is a regression caused by your fix. Without it (and not reverting > > the already applied ignore patch), runtime is properly suspended. Don't know > > why yet. > > I guess it is because of the pm_runtime_put_sync() in the > omap2_mcbsp_set_clks_src() around the fclk re-parenting. > That is a bit dubious thing for sure. We need to disable the device to > be able to re-parent the fclk but if we disable the device it is going > to be powered down, right? I think we have appropriate context handling, > so it might work, but it is certainly not a rock solid code... If you > have a stream running already, you don't really want to kill the McBSP. > Ok, so if the device is powered of at omap2_mcbsp_set_clks_src() we get the usage count underflow, and the counter is incremented immediately again in the runtime put function. So things get out of balance... I'll check Tony's fix here. > The problem is that this mux is outside of the McBSP IP, so we need a > system level (iow, clk API) way to change it runtime. > > What is the machine driver where this happens? If you set the sysclk in > hw_params of the machine driver, it will be OK, but if you do that in > probe time then it is likely going to fail as you experienced > As you see in the other patches of this series, it is a simple-audio-card with a tlv320aic3x codec in combination with the mcbsp. Regards, Andreas
Hi Tony, On Sat, 7 Oct 2023 09:25:18 +0300 Tony Lindgren <tony@atomide.com> wrote: > * Andreas Kemnade <andreas@kemnade.info> [231006 19:30]: > > On Fri, 6 Oct 2023 13:23:48 +0300 > > Tony Lindgren <tony@atomide.com> wrote: > > > Here's what I think the regression fix for omap4 clocks would be, the > > > old main_clk is not the same as the module clock that we get by default. > > > If this looks OK I'll do a similar fix also for omap5. > > > > > > Or is something else also needed? > > > > > > > hmm, > > audio output works, the waring is away, but something new is here: > > OK good to hear it works, I'll send out fixes for omap4 and 5, seems > the runtime PM warning is something different. > > > omap-mcbsp 40124000.mcbsp: Runtime PM usage count underflow! > > # cat /sys/bus/platform/devices/40124000.mcbsp/power/runtime_status > > active > > > > even with no sound. > > I guess if the mcbsp instance is not used, runtime PM is enabled but > pm_runtime_resume_and_get() is never called by ASoC? > > If so then the following might be a fix, not familiar with runtime PM > done by ASoC though and not sure if some kind of locking would be > needed here. > just checked: that one fixes the regression. runtime suspends again. Regards, Andreas
* Andreas Kemnade <andreas@kemnade.info> [231015 21:48]: > On Sat, 7 Oct 2023 09:25:18 +0300 > Tony Lindgren <tony@atomide.com> wrote: > > If so then the following might be a fix, not familiar with runtime PM > > done by ASoC though and not sure if some kind of locking would be > > needed here. > > > just checked: that one fixes the regression. runtime suspends again. OK good to hear. So is there some fixes tag for this one or where did this start happening? I'm not quite following how the dropping of platform data could have affected this, maybe it just hid the problem because of returning early? Regards, Tony
On Wed, 18 Oct 2023 08:23:45 +0300 Tony Lindgren <tony@atomide.com> wrote: > * Andreas Kemnade <andreas@kemnade.info> [231015 21:48]: > > On Sat, 7 Oct 2023 09:25:18 +0300 > > Tony Lindgren <tony@atomide.com> wrote: > > > If so then the following might be a fix, not familiar with runtime PM > > > done by ASoC though and not sure if some kind of locking would be > > > needed here. > > > > > just checked: that one fixes the regression. runtime suspends again. > > OK good to hear. So is there some fixes tag for this one or where did this > start happening? I'm not quite following how the dropping of platform data > could have affected this, maybe it just hid the problem because of > returning early? > yes I think so. From a perspective of OMAP[45] mith McBSP in master mode, applying "clk: ti: Fix missing omap4 mcbsp functional clock and aliases" on top of "ASoC: ti: omap-mcbsp: Ignore errors for getting fck_src" is a regression. For everyone else not. So "clk: ti: Fix missing omap4 mcbsp functional clock and aliases" did uncover a hidden problem, but of course it is the right step forward. Regards Andreas
On 13/10/2023 14:25, Andreas Kemnade wrote: >> I guess it is because of the pm_runtime_put_sync() in the >> omap2_mcbsp_set_clks_src() around the fclk re-parenting. >> That is a bit dubious thing for sure. We need to disable the device to >> be able to re-parent the fclk but if we disable the device it is going >> to be powered down, right? I think we have appropriate context handling, >> so it might work, but it is certainly not a rock solid code... If you >> have a stream running already, you don't really want to kill the McBSP. >> > Ok, so if the device is powered of at omap2_mcbsp_set_clks_src() > we get the usage count underflow, and the counter is incremented > immediately again in the runtime put function. So things get out of balance... > I'll check Tony's fix here. > >> The problem is that this mux is outside of the McBSP IP, so we need a >> system level (iow, clk API) way to change it runtime. >> >> What is the machine driver where this happens? If you set the sysclk in >> hw_params of the machine driver, it will be OK, but if you do that in >> probe time then it is likely going to fail as you experienced >> > As you see in the other patches of this series, > it is a simple-audio-card with a tlv320aic3x codec > in combination with the mcbsp. To be honest I would be happier if we can just remove the whole omap2_mcbsp_set_clks_src() and leave the CLKS source selection outside of the driver. But omap3pandora is selecting external clock as parent (OMAP_MCBSP_SYSCLK_CLKS_EXT - in hw_params, so it actually works) and I don't know what happens if this functionality is removed. Likely going to break Pandora. That is fixable, but what worries me is this comment and code: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/ti/omap-mcbsp.c#n388 Which is added by me a long time ago: e386615c01d37 ("ASoC: omap-mcbsp: When closing the port select PRCM source for CLKS signal") I'm not sure if this is possible to do in any other way.
diff --git a/sound/soc/ti/omap-mcbsp.c b/sound/soc/ti/omap-mcbsp.c index 21fa7b9787997..f9fe96b61852b 100644 --- a/sound/soc/ti/omap-mcbsp.c +++ b/sound/soc/ti/omap-mcbsp.c @@ -70,8 +70,8 @@ static int omap2_mcbsp_set_clks_src(struct omap_mcbsp *mcbsp, u8 fck_src_id) fck_src = clk_get(mcbsp->dev, src); if (IS_ERR(fck_src)) { - dev_err(mcbsp->dev, "CLKS: could not clk_get() %s\n", src); - return -EINVAL; + dev_info(mcbsp->dev, "CLKS: could not clk_get() %s\n", src); + return 0; } pm_runtime_put_sync(mcbsp->dev);
Commit 349355ce3a05 ("ARM: OMAP2+: Drop legacy platform data for omap4 mcbsp") dropped prcm_fck for omap4, so the clk_src might not be available making the clk_get(src) fail. In such cases, rely on the devicetree to assign the correct parent. Signed-off-by: Andreas Kemnade <andreas@kemnade.info> --- sound/soc/ti/omap-mcbsp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)