diff mbox series

[1/3] ASoC: ti: omap-mcbsp: Ignore errors for getting fck_src

Message ID 20230705190324.355282-2-andreas@kemnade.info
State Accepted
Commit 82e7c8b93a0614b1725e0ea11d0a77b04e058716
Headers show
Series ARM: omap4: embt2ws: Add audio support | expand

Commit Message

Andreas Kemnade July 5, 2023, 7:03 p.m. UTC
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(-)

Comments

Tony Lindgren Oct. 6, 2023, 10:23 a.m. UTC | #1
* 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"),
Andreas Kemnade Oct. 6, 2023, 7:30 p.m. UTC | #2
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
Tony Lindgren Oct. 7, 2023, 6:25 a.m. UTC | #3
* 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);
Andreas Kemnade Oct. 7, 2023, 7:11 a.m. UTC | #4
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
Tony Lindgren Oct. 7, 2023, 7:41 a.m. UTC | #5
* 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
Andreas Kemnade Oct. 7, 2023, 8:34 a.m. UTC | #6
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
Péter Ujfalusi Oct. 12, 2023, 2:41 p.m. UTC | #7
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
Andreas Kemnade Oct. 15, 2023, 9:48 p.m. UTC | #8
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 Oct. 18, 2023, 6:21 a.m. UTC | #9
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 mbox series

Patch

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);