Hi,
The series addresses a long standing issue with McBSP2/3 regarding to hwmod
setup. When booting with DT a warning is printed that mcbsp2/3 is using two
hwmod.
The root of the issue is the way how the hwmod data was constructed in the first
place for OMAP3 McBSP2/3.
After re-reading the TRM it is clear that the sidetone should not have it's
own hwmod data as it is not a separate IP, it is part of the McBSP module. It
can not affect PRCM either since it's SYSCONFIG register's AUTOIDLE bit is only
sets the autoidle from the internal McBSP_iclk clock to the sidetone block of
the same McBSP.
The first two patch will remove the omap2/3_sidetone hwmod and it's use from DT.
The ASoC patch is clear the autoidle bit in McBSP sidetone as it is adviced in
the TRM.
Mark: The ASoC patch can be safely merged via ASoC tree.
Regards,
Peter
---
Peter Ujfalusi (3):
ARM: DTS: omap3: Remove mcbsp2/3_sidetone hwmod reference for McBSP2/3
ARM: OMAP3: hwmod data: Merge and remove the McBSP sidetone related
data
ASoC: omap-mcbsp: Enable/disable sidetone block auto clock gating for
omap3
arch/arm/boot/dts/omap3.dtsi | 4 +-
arch/arm/mach-omap2/omap_hwmod_3xxx_data.c | 120 ++++-------------------------
sound/soc/omap/mcbsp.c | 8 ++
3 files changed, 24 insertions(+), 108 deletions(-)
--
2.7.3
OMAP3's McBSP2 and McBSP3 module have integrated sidetone block with
dedicated SYSCONFIG register. The sidetone is operating from the maain
McBSP module's ICLK. For normal operation the sidetone clock auto idle
support needs to be disabled when it is activated.
Note: This is not enough to avoid choppy sidetone because this AUTOIDLE
bit is controlling only the clock auto idle from the McBSP to the sidetone
block. If the McBSP_ICLK is idling, the sidetone clock is going to do the
same.
Signed-off-by: Peter Ujfalusi <[email protected]>
---
sound/soc/omap/mcbsp.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/sound/soc/omap/mcbsp.c b/sound/soc/omap/mcbsp.c
index c7563e230c7d..4a16e778966b 100644
--- a/sound/soc/omap/mcbsp.c
+++ b/sound/soc/omap/mcbsp.c
@@ -260,6 +260,10 @@ static void omap_st_on(struct omap_mcbsp *mcbsp)
if (mcbsp->pdata->enable_st_clock)
mcbsp->pdata->enable_st_clock(mcbsp->id, 1);
+ /* Disable Sidetone clock auto-gating for normal operation */
+ w = MCBSP_ST_READ(mcbsp, SYSCONFIG);
+ MCBSP_ST_WRITE(mcbsp, SYSCONFIG, w & ~(ST_AUTOIDLE));
+
/* Enable McBSP Sidetone */
w = MCBSP_READ(mcbsp, SSELCR);
MCBSP_WRITE(mcbsp, SSELCR, w | SIDETONEEN);
@@ -279,6 +283,10 @@ static void omap_st_off(struct omap_mcbsp *mcbsp)
w = MCBSP_READ(mcbsp, SSELCR);
MCBSP_WRITE(mcbsp, SSELCR, w & ~(SIDETONEEN));
+ /* Enable Sidetone clock auto-gating to reduce power consumption */
+ w = MCBSP_ST_READ(mcbsp, SYSCONFIG);
+ MCBSP_ST_WRITE(mcbsp, SYSCONFIG, w | ST_AUTOIDLE);
+
if (mcbsp->pdata->enable_st_clock)
mcbsp->pdata->enable_st_clock(mcbsp->id, 0);
}
--
2.7.3
McBSP2 and 3 have integrated sidetone block. The sidetone alone can not
operate, can not be enabled separately from the McBSP it is attached to.
The sidetone is enabled via McBSP register(s) and it is using the McBSP
module's iclk as clock. While the sidetone block of McBSP does have it's
SYSCONFIG registers to enable/disable AUTOIDLE (of McBSP_ICLK) this alone
does not mean that we should have dedicated hwmod for McBSP internal
blocks. Note: The AUTOIDLE bit is set after reset and hwmod is not changing
it in runtime.
The sidetone can not be enabled separately from McBSP, it can only enabled
when McBSP is in use.
Furthermore the sidetone data is specifying the exact same prcm registers
and bits as the McBSP hwmod it is attached to.
As one of the main function of the sidetone hwmod data is to convey the
address and irq number to the main mcbsp hwmod, move them directly to the
corresponding McBSP's hwmod data.
---
arch/arm/mach-omap2/omap_hwmod_3xxx_data.c | 120 ++++-------------------------
1 file changed, 14 insertions(+), 106 deletions(-)
diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
index 9869a75c5d96..317e9b816a02 100644
--- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
@@ -1175,13 +1175,10 @@ static struct omap_hwmod_irq_info omap3xxx_mcbsp2_irqs[] = {
{ .name = "common", .irq = 17 + OMAP_INTC_START, },
{ .name = "tx", .irq = 62 + OMAP_INTC_START, },
{ .name = "rx", .irq = 63 + OMAP_INTC_START, },
+ { .name = "sidetone", .irq = 4 + OMAP_INTC_START, },
{ .irq = -1 },
};
-static struct omap_mcbsp_dev_attr omap34xx_mcbsp2_dev_attr = {
- .sidetone = "mcbsp2_sidetone",
-};
-
static struct omap_hwmod omap3xxx_mcbsp2_hwmod = {
.name = "mcbsp2",
.class = &omap3xxx_mcbsp_hwmod_class,
@@ -1199,7 +1196,6 @@ static struct omap_hwmod omap3xxx_mcbsp2_hwmod = {
},
.opt_clks = mcbsp234_opt_clks,
.opt_clks_cnt = ARRAY_SIZE(mcbsp234_opt_clks),
- .dev_attr = &omap34xx_mcbsp2_dev_attr,
};
/* mcbsp3 */
@@ -1207,13 +1203,10 @@ static struct omap_hwmod_irq_info omap3xxx_mcbsp3_irqs[] = {
{ .name = "common", .irq = 22 + OMAP_INTC_START, },
{ .name = "tx", .irq = 89 + OMAP_INTC_START, },
{ .name = "rx", .irq = 90 + OMAP_INTC_START, },
+ { .name = "sidetone", .irq = 5 + OMAP_INTC_START, },
{ .irq = -1 },
};
-static struct omap_mcbsp_dev_attr omap34xx_mcbsp3_dev_attr = {
- .sidetone = "mcbsp3_sidetone",
-};
-
static struct omap_hwmod omap3xxx_mcbsp3_hwmod = {
.name = "mcbsp3",
.class = &omap3xxx_mcbsp_hwmod_class,
@@ -1231,7 +1224,6 @@ static struct omap_hwmod omap3xxx_mcbsp3_hwmod = {
},
.opt_clks = mcbsp234_opt_clks,
.opt_clks_cnt = ARRAY_SIZE(mcbsp234_opt_clks),
- .dev_attr = &omap34xx_mcbsp3_dev_attr,
};
/* mcbsp4 */
@@ -1300,62 +1292,6 @@ static struct omap_hwmod omap3xxx_mcbsp5_hwmod = {
.opt_clks_cnt = ARRAY_SIZE(mcbsp15_opt_clks),
};
-/* 'mcbsp sidetone' class */
-static struct omap_hwmod_class_sysconfig omap3xxx_mcbsp_sidetone_sysc = {
- .sysc_offs = 0x0010,
- .sysc_flags = SYSC_HAS_AUTOIDLE,
- .sysc_fields = &omap_hwmod_sysc_type1,
-};
-
-static struct omap_hwmod_class omap3xxx_mcbsp_sidetone_hwmod_class = {
- .name = "mcbsp_sidetone",
- .sysc = &omap3xxx_mcbsp_sidetone_sysc,
-};
-
-/* mcbsp2_sidetone */
-static struct omap_hwmod_irq_info omap3xxx_mcbsp2_sidetone_irqs[] = {
- { .name = "irq", .irq = 4 + OMAP_INTC_START, },
- { .irq = -1 },
-};
-
-static struct omap_hwmod omap3xxx_mcbsp2_sidetone_hwmod = {
- .name = "mcbsp2_sidetone",
- .class = &omap3xxx_mcbsp_sidetone_hwmod_class,
- .mpu_irqs = omap3xxx_mcbsp2_sidetone_irqs,
- .main_clk = "mcbsp2_fck",
- .prcm = {
- .omap2 = {
- .prcm_reg_id = 1,
- .module_bit = OMAP3430_EN_MCBSP2_SHIFT,
- .module_offs = OMAP3430_PER_MOD,
- .idlest_reg_id = 1,
- .idlest_idle_bit = OMAP3430_ST_MCBSP2_SHIFT,
- },
- },
-};
-
-/* mcbsp3_sidetone */
-static struct omap_hwmod_irq_info omap3xxx_mcbsp3_sidetone_irqs[] = {
- { .name = "irq", .irq = 5 + OMAP_INTC_START, },
- { .irq = -1 },
-};
-
-static struct omap_hwmod omap3xxx_mcbsp3_sidetone_hwmod = {
- .name = "mcbsp3_sidetone",
- .class = &omap3xxx_mcbsp_sidetone_hwmod_class,
- .mpu_irqs = omap3xxx_mcbsp3_sidetone_irqs,
- .main_clk = "mcbsp3_fck",
- .prcm = {
- .omap2 = {
- .prcm_reg_id = 1,
- .module_bit = OMAP3430_EN_MCBSP3_SHIFT,
- .module_offs = OMAP3430_PER_MOD,
- .idlest_reg_id = 1,
- .idlest_idle_bit = OMAP3430_ST_MCBSP3_SHIFT,
- },
- },
-};
-
/* SR common */
static struct omap_hwmod_sysc_fields omap34xx_sr_sysc_fields = {
.clkact_shift = 20,
@@ -3108,6 +3044,12 @@ static struct omap_hwmod_addr_space omap3xxx_mcbsp2_addrs[] = {
.pa_end = 0x490220ff,
.flags = ADDR_TYPE_RT
},
+ {
+ .name = "sidetone",
+ .pa_start = 0x49028000,
+ .pa_end = 0x490280ff,
+ .flags = ADDR_TYPE_RT
+ },
{ }
};
@@ -3127,6 +3069,12 @@ static struct omap_hwmod_addr_space omap3xxx_mcbsp3_addrs[] = {
.pa_end = 0x490240ff,
.flags = ADDR_TYPE_RT
},
+ {
+ .name = "sidetone",
+ .pa_start = 0x4902A000,
+ .pa_end = 0x4902A0ff,
+ .flags = ADDR_TYPE_RT
+ },
{ }
};
@@ -3177,44 +3125,6 @@ static struct omap_hwmod_ocp_if omap3xxx_l4_core__mcbsp5 = {
.user = OCP_USER_MPU | OCP_USER_SDMA,
};
-static struct omap_hwmod_addr_space omap3xxx_mcbsp2_sidetone_addrs[] = {
- {
- .name = "sidetone",
- .pa_start = 0x49028000,
- .pa_end = 0x490280ff,
- .flags = ADDR_TYPE_RT
- },
- { }
-};
-
-/* l4_per -> mcbsp2_sidetone */
-static struct omap_hwmod_ocp_if omap3xxx_l4_per__mcbsp2_sidetone = {
- .master = &omap3xxx_l4_per_hwmod,
- .slave = &omap3xxx_mcbsp2_sidetone_hwmod,
- .clk = "mcbsp2_ick",
- .addr = omap3xxx_mcbsp2_sidetone_addrs,
- .user = OCP_USER_MPU,
-};
-
-static struct omap_hwmod_addr_space omap3xxx_mcbsp3_sidetone_addrs[] = {
- {
- .name = "sidetone",
- .pa_start = 0x4902A000,
- .pa_end = 0x4902A0ff,
- .flags = ADDR_TYPE_RT
- },
- { }
-};
-
-/* l4_per -> mcbsp3_sidetone */
-static struct omap_hwmod_ocp_if omap3xxx_l4_per__mcbsp3_sidetone = {
- .master = &omap3xxx_l4_per_hwmod,
- .slave = &omap3xxx_mcbsp3_sidetone_hwmod,
- .clk = "mcbsp3_ick",
- .addr = omap3xxx_mcbsp3_sidetone_addrs,
- .user = OCP_USER_MPU,
-};
-
/* l4_core -> mailbox */
static struct omap_hwmod_ocp_if omap3xxx_l4_core__mailbox = {
.master = &omap3xxx_l4_core_hwmod,
@@ -3651,8 +3561,6 @@ static struct omap_hwmod_ocp_if *omap3xxx_hwmod_ocp_ifs[] __initdata = {
&omap3xxx_l4_per__mcbsp3,
&omap3xxx_l4_per__mcbsp4,
&omap3xxx_l4_core__mcbsp5,
- &omap3xxx_l4_per__mcbsp2_sidetone,
- &omap3xxx_l4_per__mcbsp3_sidetone,
&omap34xx_l4_core__mcspi1,
&omap34xx_l4_core__mcspi2,
&omap34xx_l4_core__mcspi3,
--
2.7.3
The mcbsp2/3_sidetone hwmod is a dummy hwmod and servers as a placeholder
to attach the register address and irq number to the main mcbsp2/3 hwmod.
The hwmod for the McBSP sidetone module is going to be removed from the
kernel.
Signed-off-by: Peter Ujfalusi <[email protected]>
---
arch/arm/boot/dts/omap3.dtsi | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/omap3.dtsi b/arch/arm/boot/dts/omap3.dtsi
index b41d07e8e765..d6b7abce7a55 100644
--- a/arch/arm/boot/dts/omap3.dtsi
+++ b/arch/arm/boot/dts/omap3.dtsi
@@ -507,7 +507,7 @@
<4>; /* Sidetone */
interrupt-names = "common", "tx", "rx", "sidetone";
ti,buffer-size = <1280>;
- ti,hwmods = "mcbsp2", "mcbsp2_sidetone";
+ ti,hwmods = "mcbsp2";
dmas = <&sdma 33>,
<&sdma 34>;
dma-names = "tx", "rx";
@@ -525,7 +525,7 @@
<5>; /* Sidetone */
interrupt-names = "common", "tx", "rx", "sidetone";
ti,buffer-size = <128>;
- ti,hwmods = "mcbsp3", "mcbsp3_sidetone";
+ ti,hwmods = "mcbsp3";
dmas = <&sdma 17>,
<&sdma 18>;
dma-names = "tx", "rx";
--
2.7.3
On 03/18/2016 12:28 PM, Peter Ujfalusi wrote:
> McBSP2 and 3 have integrated sidetone block. The sidetone alone can not
> operate, can not be enabled separately from the McBSP it is attached to.
> The sidetone is enabled via McBSP register(s) and it is using the McBSP
> module's iclk as clock. While the sidetone block of McBSP does have it's
> SYSCONFIG registers to enable/disable AUTOIDLE (of McBSP_ICLK) this alone
> does not mean that we should have dedicated hwmod for McBSP internal
> blocks. Note: The AUTOIDLE bit is set after reset and hwmod is not changing
> it in runtime.
> The sidetone can not be enabled separately from McBSP, it can only enabled
> when McBSP is in use.
> Furthermore the sidetone data is specifying the exact same prcm registers
> and bits as the McBSP hwmod it is attached to.
> As one of the main function of the sidetone hwmod data is to convey the
> address and irq number to the main mcbsp hwmod, move them directly to the
> corresponding McBSP's hwmod data.
My signed-off is missing :(
Also I have noticed one thing with this patch: it will cause regression in
legacy boot on OMPA3.
The code in mach-omap2/mcbsp.c is checking:
if (oh->dev_attr) {
...
}
To decide if the mcbsp port has sidetone or not, and if it does it will fill
in the hooks to be able to disable and enable the McBSP idle modes in PRCM level.
I will resend the DTS and OMAP patches with a fix included.
BTW: I'm working on a solution which will work with DT boot as well. Currently
in DT booted kernel we can not get stable sidetone.
Sorry,
P?ter
> ---
> arch/arm/mach-omap2/omap_hwmod_3xxx_data.c | 120 ++++-------------------------
> 1 file changed, 14 insertions(+), 106 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> index 9869a75c5d96..317e9b816a02 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> @@ -1175,13 +1175,10 @@ static struct omap_hwmod_irq_info omap3xxx_mcbsp2_irqs[] = {
> { .name = "common", .irq = 17 + OMAP_INTC_START, },
> { .name = "tx", .irq = 62 + OMAP_INTC_START, },
> { .name = "rx", .irq = 63 + OMAP_INTC_START, },
> + { .name = "sidetone", .irq = 4 + OMAP_INTC_START, },
> { .irq = -1 },
> };
>
> -static struct omap_mcbsp_dev_attr omap34xx_mcbsp2_dev_attr = {
> - .sidetone = "mcbsp2_sidetone",
> -};
> -
> static struct omap_hwmod omap3xxx_mcbsp2_hwmod = {
> .name = "mcbsp2",
> .class = &omap3xxx_mcbsp_hwmod_class,
> @@ -1199,7 +1196,6 @@ static struct omap_hwmod omap3xxx_mcbsp2_hwmod = {
> },
> .opt_clks = mcbsp234_opt_clks,
> .opt_clks_cnt = ARRAY_SIZE(mcbsp234_opt_clks),
> - .dev_attr = &omap34xx_mcbsp2_dev_attr,
> };
>
> /* mcbsp3 */
> @@ -1207,13 +1203,10 @@ static struct omap_hwmod_irq_info omap3xxx_mcbsp3_irqs[] = {
> { .name = "common", .irq = 22 + OMAP_INTC_START, },
> { .name = "tx", .irq = 89 + OMAP_INTC_START, },
> { .name = "rx", .irq = 90 + OMAP_INTC_START, },
> + { .name = "sidetone", .irq = 5 + OMAP_INTC_START, },
> { .irq = -1 },
> };
>
> -static struct omap_mcbsp_dev_attr omap34xx_mcbsp3_dev_attr = {
> - .sidetone = "mcbsp3_sidetone",
> -};
> -
> static struct omap_hwmod omap3xxx_mcbsp3_hwmod = {
> .name = "mcbsp3",
> .class = &omap3xxx_mcbsp_hwmod_class,
> @@ -1231,7 +1224,6 @@ static struct omap_hwmod omap3xxx_mcbsp3_hwmod = {
> },
> .opt_clks = mcbsp234_opt_clks,
> .opt_clks_cnt = ARRAY_SIZE(mcbsp234_opt_clks),
> - .dev_attr = &omap34xx_mcbsp3_dev_attr,
> };
>
> /* mcbsp4 */
> @@ -1300,62 +1292,6 @@ static struct omap_hwmod omap3xxx_mcbsp5_hwmod = {
> .opt_clks_cnt = ARRAY_SIZE(mcbsp15_opt_clks),
> };
>
> -/* 'mcbsp sidetone' class */
> -static struct omap_hwmod_class_sysconfig omap3xxx_mcbsp_sidetone_sysc = {
> - .sysc_offs = 0x0010,
> - .sysc_flags = SYSC_HAS_AUTOIDLE,
> - .sysc_fields = &omap_hwmod_sysc_type1,
> -};
> -
> -static struct omap_hwmod_class omap3xxx_mcbsp_sidetone_hwmod_class = {
> - .name = "mcbsp_sidetone",
> - .sysc = &omap3xxx_mcbsp_sidetone_sysc,
> -};
> -
> -/* mcbsp2_sidetone */
> -static struct omap_hwmod_irq_info omap3xxx_mcbsp2_sidetone_irqs[] = {
> - { .name = "irq", .irq = 4 + OMAP_INTC_START, },
> - { .irq = -1 },
> -};
> -
> -static struct omap_hwmod omap3xxx_mcbsp2_sidetone_hwmod = {
> - .name = "mcbsp2_sidetone",
> - .class = &omap3xxx_mcbsp_sidetone_hwmod_class,
> - .mpu_irqs = omap3xxx_mcbsp2_sidetone_irqs,
> - .main_clk = "mcbsp2_fck",
> - .prcm = {
> - .omap2 = {
> - .prcm_reg_id = 1,
> - .module_bit = OMAP3430_EN_MCBSP2_SHIFT,
> - .module_offs = OMAP3430_PER_MOD,
> - .idlest_reg_id = 1,
> - .idlest_idle_bit = OMAP3430_ST_MCBSP2_SHIFT,
> - },
> - },
> -};
> -
> -/* mcbsp3_sidetone */
> -static struct omap_hwmod_irq_info omap3xxx_mcbsp3_sidetone_irqs[] = {
> - { .name = "irq", .irq = 5 + OMAP_INTC_START, },
> - { .irq = -1 },
> -};
> -
> -static struct omap_hwmod omap3xxx_mcbsp3_sidetone_hwmod = {
> - .name = "mcbsp3_sidetone",
> - .class = &omap3xxx_mcbsp_sidetone_hwmod_class,
> - .mpu_irqs = omap3xxx_mcbsp3_sidetone_irqs,
> - .main_clk = "mcbsp3_fck",
> - .prcm = {
> - .omap2 = {
> - .prcm_reg_id = 1,
> - .module_bit = OMAP3430_EN_MCBSP3_SHIFT,
> - .module_offs = OMAP3430_PER_MOD,
> - .idlest_reg_id = 1,
> - .idlest_idle_bit = OMAP3430_ST_MCBSP3_SHIFT,
> - },
> - },
> -};
> -
> /* SR common */
> static struct omap_hwmod_sysc_fields omap34xx_sr_sysc_fields = {
> .clkact_shift = 20,
> @@ -3108,6 +3044,12 @@ static struct omap_hwmod_addr_space omap3xxx_mcbsp2_addrs[] = {
> .pa_end = 0x490220ff,
> .flags = ADDR_TYPE_RT
> },
> + {
> + .name = "sidetone",
> + .pa_start = 0x49028000,
> + .pa_end = 0x490280ff,
> + .flags = ADDR_TYPE_RT
> + },
> { }
> };
>
> @@ -3127,6 +3069,12 @@ static struct omap_hwmod_addr_space omap3xxx_mcbsp3_addrs[] = {
> .pa_end = 0x490240ff,
> .flags = ADDR_TYPE_RT
> },
> + {
> + .name = "sidetone",
> + .pa_start = 0x4902A000,
> + .pa_end = 0x4902A0ff,
> + .flags = ADDR_TYPE_RT
> + },
> { }
> };
>
> @@ -3177,44 +3125,6 @@ static struct omap_hwmod_ocp_if omap3xxx_l4_core__mcbsp5 = {
> .user = OCP_USER_MPU | OCP_USER_SDMA,
> };
>
> -static struct omap_hwmod_addr_space omap3xxx_mcbsp2_sidetone_addrs[] = {
> - {
> - .name = "sidetone",
> - .pa_start = 0x49028000,
> - .pa_end = 0x490280ff,
> - .flags = ADDR_TYPE_RT
> - },
> - { }
> -};
> -
> -/* l4_per -> mcbsp2_sidetone */
> -static struct omap_hwmod_ocp_if omap3xxx_l4_per__mcbsp2_sidetone = {
> - .master = &omap3xxx_l4_per_hwmod,
> - .slave = &omap3xxx_mcbsp2_sidetone_hwmod,
> - .clk = "mcbsp2_ick",
> - .addr = omap3xxx_mcbsp2_sidetone_addrs,
> - .user = OCP_USER_MPU,
> -};
> -
> -static struct omap_hwmod_addr_space omap3xxx_mcbsp3_sidetone_addrs[] = {
> - {
> - .name = "sidetone",
> - .pa_start = 0x4902A000,
> - .pa_end = 0x4902A0ff,
> - .flags = ADDR_TYPE_RT
> - },
> - { }
> -};
> -
> -/* l4_per -> mcbsp3_sidetone */
> -static struct omap_hwmod_ocp_if omap3xxx_l4_per__mcbsp3_sidetone = {
> - .master = &omap3xxx_l4_per_hwmod,
> - .slave = &omap3xxx_mcbsp3_sidetone_hwmod,
> - .clk = "mcbsp3_ick",
> - .addr = omap3xxx_mcbsp3_sidetone_addrs,
> - .user = OCP_USER_MPU,
> -};
> -
> /* l4_core -> mailbox */
> static struct omap_hwmod_ocp_if omap3xxx_l4_core__mailbox = {
> .master = &omap3xxx_l4_core_hwmod,
> @@ -3651,8 +3561,6 @@ static struct omap_hwmod_ocp_if *omap3xxx_hwmod_ocp_ifs[] __initdata = {
> &omap3xxx_l4_per__mcbsp3,
> &omap3xxx_l4_per__mcbsp4,
> &omap3xxx_l4_core__mcbsp5,
> - &omap3xxx_l4_per__mcbsp2_sidetone,
> - &omap3xxx_l4_per__mcbsp3_sidetone,
> &omap34xx_l4_core__mcspi1,
> &omap34xx_l4_core__mcspi2,
> &omap34xx_l4_core__mcspi3,
>
On Fri, 18 Mar 2016, Peter Ujfalusi wrote:
> The series addresses a long standing issue with McBSP2/3 regarding to hwmod
> setup. When booting with DT a warning is printed that mcbsp2/3 is using two
> hwmod.
> The root of the issue is the way how the hwmod data was constructed in the first
> place for OMAP3 McBSP2/3.
> After re-reading the TRM it is clear that the sidetone should not have it's
> own hwmod data as it is not a separate IP, it is part of the McBSP module.
Odd. I come to exactly the opposite conclusion from reading the TRM. In
fact the SIDETONE blocks clearly should be hwmods, according to the
documentation.
Consider:
1. The SIDETONE blocks have their own L4 ports - as documented in the
OMAP36xx public TRM rev Z, Table 2-5 "L4-Peripheral Memory Space".
2. The SIDETONE blocks have different register access width restrictions
from the McBSP. Ibid., Table 2-7 "Register Access Restrictions".
3. The SIDETONE blocks have distinct L4-Per firewall region IDs from their
corresponding McBSP IP blocks. Ibid., Table 9-114 "Region Allocation for
L4-Per Interconnect".
4. The SIDETONE blocks have their own L4 target interconnect agents.
Table 9-128 "L4-Per Instance Summary"
5. The SIDETONE blocks have their own MPU IRQ lines, distinct from the
McBSP block IRQ lines. Table 12-4 "Interrupt Mapping to the MPU
Subsystem"
6. The SIDETONE IP block register target space is distinct from the
corresponding McBSP address ranges. Table 21-36 "McBSP Instance Summary"
7. The SIDETONE IP blocks have their own "TI OCP" integration registers.
Table 21-134 "ST_REV_REG", Table 21-136 "ST_SYSCONFIG_REG".
A better solution to the warnings you mention at the top of the message is
to provide a separate low-level McBSP SIDETONE IP block device driver,
distinct from the existing McBSP low-level IP block driver.
> It can not affect PRCM either since it's SYSCONFIG register's AUTOIDLE
> bit is only sets the autoidle from the internal McBSP_iclk clock to the
> sidetone block of the same McBSP.
Can't parse this - could you try again? Are you referring to the erratum
where someone forgot to hook up the SIDETONE idle signals to the PRCM, and
the MCBSP_ICLK has to be manually kept active when the SIDETONE block is
active?
- Paul
On Fri, 18 Mar 2016, Peter Ujfalusi wrote:
> OMAP3's McBSP2 and McBSP3 module have integrated sidetone block with
> dedicated SYSCONFIG register. The sidetone is operating from the maain
> McBSP module's ICLK. For normal operation the sidetone clock auto idle
> support needs to be disabled when it is activated.
> Note: This is not enough to avoid choppy sidetone because this AUTOIDLE
> bit is controlling only the clock auto idle from the McBSP to the sidetone
> block. If the McBSP_ICLK is idling, the sidetone clock is going to do the
> same.
>
> Signed-off-by: Peter Ujfalusi <[email protected]>
Mark, please drop this patch for the time being, until the SoC integration
issues can be sorted out first. It's best to wait a little while before
applying patches like these so folks have a chance to comment on their
correctness first.
We used to handle this problem in the OMAP hwmod SoC integration layer
with a flag that forced the interface clock to stay active as long as the
underlying IP blocks were active. However I can't find that flag right
now in the current data, so maybe it got accidentally or inadvertently
removed at some point in time in the past. The right way to fix this
would be to add that flag back in, rather than messing with the SoC
integration registers from the McBSP drivers.
thanks
- Paul
Paul,
On 03/19/16 21:31, Paul Walmsley wrote:
> On Fri, 18 Mar 2016, Peter Ujfalusi wrote:
>
>> The series addresses a long standing issue with McBSP2/3 regarding to hwmod
>> setup. When booting with DT a warning is printed that mcbsp2/3 is using two
>> hwmod.
>> The root of the issue is the way how the hwmod data was constructed in the first
>> place for OMAP3 McBSP2/3.
>> After re-reading the TRM it is clear that the sidetone should not have it's
>> own hwmod data as it is not a separate IP, it is part of the McBSP module.
>
> Odd. I come to exactly the opposite conclusion from reading the TRM.
I was looking at the Figure 21-2 in chapter 21.1.2 SIDETONE core:
McBSP2/3 module consist of McBSP core + SIDETONE core.
In fact the SIDETONE blocks clearly should be hwmods, according to the
> documentation.
>
> Consider:
>
> 1. The SIDETONE blocks have their own L4 ports - as documented in the
> OMAP36xx public TRM rev Z, Table 2-5 "L4-Peripheral Memory Space".
The SIDETONE feature is an addition on top of 'standard' McBSP
> 2. The SIDETONE blocks have different register access width restrictions
> from the McBSP. Ibid., Table 2-7 "Register Access Restrictions".
Yes, this is odd. McBSP is 32bits and the McBSP sidetone is 8/16/32bits
> 3. The SIDETONE blocks have distinct L4-Per firewall region IDs from their
> corresponding McBSP IP blocks. Ibid., Table 9-114 "Region Allocation for
> L4-Per Interconnect".
This is also interesting:
McBSP2 sidetone is in region 39 and 40 (module and L4 interconnect) which is
unique in case of OMAP34xx and OMAP35xx, but it is overlapping with GPIO6 on
OMAP36xx. Not sure what are the implications.
> 4. The SIDETONE blocks have their own L4 target interconnect agents.
> Table 9-128 "L4-Per Instance Summary"
As they are different in the memory map.
> 5. The SIDETONE blocks have their own MPU IRQ lines, distinct from the
> McBSP block IRQ lines. Table 12-4 "Interrupt Mapping to the MPU
> Subsystem"
Well, all McBSP have 3 distinct MPU IRQ lines: combined, RX, TX (McBSP2:
combined: 17, TX: 62, RX: 63 for example). This does not make the McBSP RX and
TX different IP block.
> 6. The SIDETONE IP block register target space is distinct from the
> corresponding McBSP address ranges. Table 21-36 "McBSP Instance Summary"
Yes, they are as it is an additional feature integrated into only McBSP2/3 module.
> 7. The SIDETONE IP blocks have their own "TI OCP" integration registers.
> Table 21-134 "ST_REV_REG", Table 21-136 "ST_SYSCONFIG_REG".
Which does not work in a way it is supposed to work.
As per the TRM (SWPU177T - OMAP36xx) 21.3.2.2.6:
McBSPi.ST_SYSCONFIG_REG[0] AUTOIDLE bit:
- When this bit is asserted (set to 1), the McBSPi_ICLK clock auto-gating is
enabled and this clock is disabled internally to the SIDETONE feature, thus
reducing power consumption, but not to the McBSP module that contains this
feature.
After reset, the automatic clock gating is enabled; thus, this bit must be
disabled by software for activated SIDETONE feature.
- When this bit is set to 0, the McBSPi_ICLK clock auto-gating is disabled and
this clock is enabled. The SIDETONE feature can be used normally.
The important part being: "When this bit is asserted (set to 1), the
McBSPi_ICLK clock auto-gating is enabled and this clock is disabled internally
to the SIDETONE feature"
In my reading the McBSPi.ST_SYSCONFIG_REG[0] AUTOIDLE is internal to McBSP
module and it will not affect the PRCM level of gating the McBSP2/3_ICLK. Most
likely the actual plan was that by setting this bit the McBSP core should
prevent the ICLK auto gating, but it is - as you know - not working that way.
> A better solution to the warnings you mention at the top of the message is
> to provide a separate low-level McBSP SIDETONE IP block device driver,
> distinct from the existing McBSP low-level IP block driver.
If we treat the McBSP sidetone as a distinct module (which is not) of OMAP3:
we will have two hwmods and two drivers poking with the same PRCM registers as
we need this for the sidetone: if it is enabled we need to disable the
autoidle for the McBSP_ICLK for the given McBSP module the sidetone is part of.
The sidetone can be enabled/disabled any time when McBSP is active so if we
have McBSP running and we enable the sidetone the McBSP_ICLK should not idle,
when the sidetone is disabled and McBSP is still in use we can not disable the
McBSP in PRCM level, but we can enable the ICLK autoidle.
>> It can not affect PRCM either since it's SYSCONFIG register's AUTOIDLE
>> bit is only sets the autoidle from the internal McBSP_iclk clock to the
>> sidetone block of the same McBSP.
>
> Can't parse this - could you try again? Are you referring to the erratum
> where someone forgot to hook up the SIDETONE idle signals to the PRCM, and
> the MCBSP_ICLK has to be manually kept active when the SIDETONE block is
> active?
Probably there is an errata for this, but 21.3.2.2.6 tells that the
McBSPi.ST_SYSCONFIG[0] AUTOIDLE is working internally to the SIDETONE feature.
The intention must have been that in case the SIDETONE is in use, the McBSP
module itself should prevent the autoidle, but it is not. We all know that
this is not going to be fixed by new revision.
--
P?ter
Paul,
On 03/19/16 21:37, Paul Walmsley wrote:
> On Fri, 18 Mar 2016, Peter Ujfalusi wrote:
>
>> OMAP3's McBSP2 and McBSP3 module have integrated sidetone block with
>> dedicated SYSCONFIG register. The sidetone is operating from the maain
>> McBSP module's ICLK. For normal operation the sidetone clock auto idle
>> support needs to be disabled when it is activated.
>> Note: This is not enough to avoid choppy sidetone because this AUTOIDLE
>> bit is controlling only the clock auto idle from the McBSP to the sidetone
>> block. If the McBSP_ICLK is idling, the sidetone clock is going to do the
>> same.
>>
>> Signed-off-by: Peter Ujfalusi <[email protected]>
>
> Mark, please drop this patch for the time being, until the SoC integration
> issues can be sorted out first. It's best to wait a little while before
> applying patches like these so folks have a chance to comment on their
> correctness first.
>
> We used to handle this problem in the OMAP hwmod SoC integration layer
> with a flag that forced the interface clock to stay active as long as the
> underlying IP blocks were active. However I can't find that flag right
> now in the current data, so maybe it got accidentally or inadvertently
> removed at some point in time in the past. The right way to fix this
> would be to add that flag back in, rather than messing with the SoC
> integration registers from the McBSP drivers.
I can not recall such a flag. We had both hwmods attached to the given McBSP
mkodule via dev_attr and we dealt with the McBSP_ICLK autoidle enable/disable
via callbacks provided to the driver via platform data.
arch/arm/mach-omap2/mcbsp.c: omap3_enable_st_clock() In there we use
omap2_clk_deny_idle()/omap2_clk_allow_idle() to make sure that the ICLK is not
gated when the ST is enabled in the given McBSP module.
But this only works when we boot in legacy mode. The DT boot is broken in this
regards as long we have first booted OMAP3 with DT.
--
P?ter
On Sat, Mar 19, 2016 at 07:37:33PM +0000, Paul Walmsley wrote:
> Mark, please drop this patch for the time being, until the SoC integration
> issues can be sorted out first. It's best to wait a little while before
> applying patches like these so folks have a chance to comment on their
> correctness first.
Nobody except Peter and Jarkko ever shows any interest in the OMAP code
- the amount of time I wait for review depends on who's sending the
patches and if I expect anyone else is going to respond.
Hi,
* Peter Ujfalusi <[email protected]> [160321 01:39]:
>
> This is also interesting:
> McBSP2 sidetone is in region 39 and 40 (module and L4 interconnect) which is
> unique in case of OMAP34xx and OMAP35xx, but it is overlapping with GPIO6 on
> OMAP36xx. Not sure what are the implications.
Hmm GPIO6 is in a different L4 segment though? Maybe
you did not account for the segments?
0x49000000 + 0x20000 + 0x2000/0x4000/0x6000 for McBSP
0x49000000 + 0x50000 + 0x8000 for GPIO6
Or maybe I don't understand at which physical address the
overlap is? :)
Let me know if you need me to dump out the IO ranges for
you from the AP table for 34xx and 36xx.
Regards,
Tony
Tony,
On 03/21/16 22:21, Tony Lindgren wrote:
> Hi,
>
> * Peter Ujfalusi <[email protected]> [160321 01:39]:
>>
>> This is also interesting:
>> McBSP2 sidetone is in region 39 and 40 (module and L4 interconnect) which is
>> unique in case of OMAP34xx and OMAP35xx, but it is overlapping with GPIO6 on
>> OMAP36xx. Not sure what are the implications.
>
> Hmm GPIO6 is in a different L4 segment though? Maybe
> you did not account for the segments?
>
> 0x49000000 + 0x20000 + 0x2000/0x4000/0x6000 for McBSP
> 0x49000000 + 0x50000 + 0x8000 for GPIO6
>
> Or maybe I don't understand at which physical address the
> overlap is? :)
The addresses are not overlapping, but the Region Numbers for GPIO6 and McBSP2
sidetone in Table 9-114 "Region Allocation for
L4-Per Interconnect". But only in case of OMAP36xx
--
P?ter
* Peter Ujfalusi <[email protected]> [160322 01:54]:
> Tony,
>
> On 03/21/16 22:21, Tony Lindgren wrote:
> > Hi,
> >
> > * Peter Ujfalusi <[email protected]> [160321 01:39]:
> >>
> >> This is also interesting:
> >> McBSP2 sidetone is in region 39 and 40 (module and L4 interconnect) which is
> >> unique in case of OMAP34xx and OMAP35xx, but it is overlapping with GPIO6 on
> >> OMAP36xx. Not sure what are the implications.
> >
> > Hmm GPIO6 is in a different L4 segment though? Maybe
> > you did not account for the segments?
> >
> > 0x49000000 + 0x20000 + 0x2000/0x4000/0x6000 for McBSP
> > 0x49000000 + 0x50000 + 0x8000 for GPIO6
> >
> > Or maybe I don't understand at which physical address the
> > overlap is? :)
>
> The addresses are not overlapping, but the Region Numbers for GPIO6 and McBSP2
> sidetone in Table 9-114 "Region Allocation for
> L4-Per Interconnect". But only in case of OMAP36xx
Oh OK, yeah the TRM region numbers are wrong, they skip unused
entries compared to the hardware AP table :) Basically the
TRM AP region numbering is useless and wrong.
Regards,
Tony