[PATCH 0/3] ARM: omap4: embt2ws: Add audio support
Add audio support for Epson Moverio BT-200. In the vendor kernel, the MCBSP side is used as master, so do it here also that way.
Andreas Kemnade (3): ASoC: ti: omap-mcbsp: Ignore errors for getting fck_src ASoC: tlv320aic3x: use BCLK instead of MCLK if not in master mode ARM: dts: omap4: embt2ws: Add audio support
arch/arm/boot/dts/omap4-epson-embt2ws.dts | 21 +++++++++++++++++++++ sound/soc/codecs/tlv320aic3x.c | 4 ++++ sound/soc/ti/omap-mcbsp.c | 4 ++-- 3 files changed, 27 insertions(+), 2 deletions(-)
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(-)
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);
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
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/soun...
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.
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
Required to have audio output on Epson Moverio BT-200. Audio chip there is marked with AC31051. Audio output is silent there without that clock register set.
Signed-off-by: Andreas Kemnade andreas@kemnade.info --- sound/soc/codecs/tlv320aic3x.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c index 56e795a00e22f..87929e210b55e 100644 --- a/sound/soc/codecs/tlv320aic3x.c +++ b/sound/soc/codecs/tlv320aic3x.c @@ -1234,6 +1234,10 @@ static int aic3x_set_dai_sysclk(struct snd_soc_dai *codec_dai, struct snd_soc_component *component = codec_dai->component; struct aic3x_priv *aic3x = snd_soc_component_get_drvdata(component);
+ /* probably no mclk if not master, so rely on bitclk */ + if (!aic3x->master) + clk_id = 2; + /* set clock on MCLK or GPIO2 or BCLK */ snd_soc_component_update_bits(component, AIC3X_CLKGEN_CTRL_REG, PLLCLK_IN_MASK, clk_id << PLLCLK_IN_SHIFT);
On Wed, Jul 05, 2023 at 09:03:23PM +0200, Andreas Kemnade wrote:
- /* probably no mclk if not master, so rely on bitclk */
- if (!aic3x->master)
clk_id = 2;
This is fairly clearly a massive hack, we're just silently ignoring the clock we were asked to configure and choosing another one which is likely at a different rate to that we were expecting and sadly the driver didn't provide an automatic mode due to how old it is. We also appear to try to use the configured clock rate during PLL setup which still happens in hw_params() even with this change which is a bit of a concern here. Are you sure hw_params ends up doing the right thing, and that there are no other systems that get broken by this (perhaps ones sending a lower BCLK for example)?
It would be nicer to set the clock via the DT bindings, ideally with the clock bindings...
On Wed, 5 Jul 2023 20:21:58 +0100 Mark Brown broonie@kernel.org wrote:
On Wed, Jul 05, 2023 at 09:03:23PM +0200, Andreas Kemnade wrote:
- /* probably no mclk if not master, so rely on bitclk */
- if (!aic3x->master)
clk_id = 2;This is fairly clearly a massive hack, we're just silently ignoring the clock we were asked to configure and choosing another one which is likely at a different rate to that we were expecting and sadly the driver didn't provide an automatic mode due to how old it is. We also appear to try to use the configured clock rate during PLL setup which still happens in hw_params() even with this change which is a bit of a concern here. Are you sure hw_params ends up doing the right thing, and that there are no other systems that get broken by this (perhaps ones sending a lower BCLK for example)?
Yes, I am not that happy myself with that one. Possible victim is keystone-k2g-evm.dts. I looked for if (master) things and found the pll enable/disable connected to it. But that is not the whole story...
It would be nicer to set the clock via the DT bindings, ideally with the clock bindings...
I found no path from these simple-audio-card things to provide a clk_id to set_dai_sysclk. I would of course prefer such a thing. Do I have overlooked something?
Regards, Andreas
On Wed, Jul 05, 2023 at 09:56:11PM +0200, Andreas Kemnade wrote:
Mark Brown broonie@kernel.org wrote:
It would be nicer to set the clock via the DT bindings, ideally with the clock bindings...
I found no path from these simple-audio-card things to provide a clk_id to set_dai_sysclk. I would of course prefer such a thing. Do I have overlooked something?
Since we already have clock bindings we should use those to configure the clocks, there's several drivers that have added this support already - look for clock providers.
Hi,
On Thu, 6 Jul 2023 13:02:36 +0100 Mark Brown broonie@kernel.org wrote:
On Wed, Jul 05, 2023 at 09:56:11PM +0200, Andreas Kemnade wrote:
Mark Brown broonie@kernel.org wrote:
It would be nicer to set the clock via the DT bindings, ideally with the clock bindings...
I found no path from these simple-audio-card things to provide a clk_id to set_dai_sysclk. I would of course prefer such a thing. Do I have overlooked something?
Since we already have clock bindings we should use those to configure the clocks, there's several drivers that have added this support already
- look for clock providers.
ok, looking around: Just to make sure I am not running in a bad direction: Do you think tlv320aic32x4{,-clk}.c is a good example? It is ignoring clk_id. I was mentally bound to have to use clk_id there, so I did not found a good solution.
So I guess I have to configure the chip as a master and using mclk and compare register dumps with the state we have now and the state after the changes, additionally to check bclk functionality directly.
Regards, Andreas
On Sat, Jul 08, 2023 at 03:03:19PM +0200, Andreas Kemnade wrote:
Mark Brown broonie@kernel.org wrote:
Since we already have clock bindings we should use those to configure the clocks, there's several drivers that have added this support already
- look for clock providers.
ok, looking around: Just to make sure I am not running in a bad direction: Do you think tlv320aic32x4{,-clk}.c is a good example? It is ignoring clk_id. I was mentally bound to have to use clk_id there, so I did not found a good solution.
Yes, that looks like a good example - the whole thing here is to move away from using clk_id and to using the clock API to specify clocks.
So I guess I have to configure the chip as a master and using mclk and compare register dumps with the state we have now and the state after the changes, additionally to check bclk functionality directly.
You can probably get away with less but it's goot to be thorough.
There is a headset jack on the connection between control unit and display. Wire things up to have audio output with MCBSP2 as a master.
Signed-off-by: Andreas Kemnade andreas@kemnade.info --- arch/arm/boot/dts/omap4-epson-embt2ws.dts | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)
diff --git a/arch/arm/boot/dts/omap4-epson-embt2ws.dts b/arch/arm/boot/dts/omap4-epson-embt2ws.dts index 5e9a997f686b0..ee86981b2e448 100644 --- a/arch/arm/boot/dts/omap4-epson-embt2ws.dts +++ b/arch/arm/boot/dts/omap4-epson-embt2ws.dts @@ -46,6 +46,24 @@ key-lock { }; };
+ sound { + compatible = "simple-audio-card"; + simple-audio-card,name = "BT2 Sound"; + simple-audio-card,format = "left_j"; + simple-audio-card,bitclock-master = <&sound_master>; + simple-audio-card,frame-master = <&sound_master>; + + sound_master: simple-audio-card,cpu { + system-clock-frequency = <24000000>; + sound-dai = <&mcbsp2>; + }; + + simple-audio-card,codec { + sound-dai = <&tlv320aic3x>; + system-clock-frequency = <24000000>; + }; + }; + unknown_supply: unknown-supply { compatible = "regulator-fixed"; regulator-name = "unknown"; @@ -274,6 +292,9 @@ &mcbsp2 { #sound-dai-cells = <0>; pinctrl-names = "default"; pinctrl-0 = <&mcbsp2_pins>; + assigned-clocks = <&abe_clkctrl OMAP4_MCBSP2_CLKCTRL 24>; + assigned-clock-parents = <&abe_clkctrl OMAP4_MCBSP2_CLKCTRL 26>; + status = "okay"; };
On Wed, Jul 05, 2023 at 09:03:24PM +0200, Andreas Kemnade wrote:
- sound {
compatible = "simple-audio-card";
For new usage audio-graph-card2 is preferred, it's a superset of the functionality and much more flexible.
simple-audio-card,codec {sound-dai = <&tlv320aic3x>;system-clock-frequency = <24000000>;};- };
Are you *sure* the BCLK always comes out at this rate?
On Wed, 05 Jul 2023 21:03:21 +0200, Andreas Kemnade wrote:
Add audio support for Epson Moverio BT-200. In the vendor kernel, the MCBSP side is used as master, so do it here also that way.
Andreas Kemnade (3): ASoC: ti: omap-mcbsp: Ignore errors for getting fck_src ASoC: tlv320aic3x: use BCLK instead of MCLK if not in master mode ARM: dts: omap4: embt2ws: Add audio support
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/3] ASoC: ti: omap-mcbsp: Ignore errors for getting fck_src commit: 82e7c8b93a0614b1725e0ea11d0a77b04e058716
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (4)
-
Andreas Kemnade -
Mark Brown -
Péter Ujfalusi -
Tony Lindgren