| Message ID | 20230705190324.355282-2-andreas@kemnade.info |
|---|---|
| State | Accepted |
| Commit | 82e7c8b93a0614b1725e0ea11d0a77b04e058716 |
| Headers | show |
| Series | ARM: omap4: embt2ws: Add audio support | expand |
* 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
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
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
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(-)