Hi,
Chanes since v1:
- removed the ASoC patch as Mark has applied it already
- Added my signed-off to the hwmod patch
- New patch to handle the case when the sidetone hwmod has been removed for
legacy boot.
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.
Regards,
Peter
---
Peter Ujfalusi (3):
ARM: DTS: omap3: Remove mcbsp2/3_sidetone hwmod reference for McBSP2/3
ARM: OMAP2+: mcbsp: Prepare the device build code for sidetone hwmod
removal
ARM: OMAP3: hwmod data: Merge and remove the McBSP sidetone related
data
arch/arm/boot/dts/omap3.dtsi | 4 +-
arch/arm/mach-omap2/mcbsp.c | 28 ++++++-
arch/arm/mach-omap2/omap_hwmod_3xxx_data.c | 120 ++++-------------------------
3 files changed, 43 insertions(+), 109 deletions(-)
--
2.7.3
The McBSP sidetone hwmods will be removed as they should not exist because
the sidetone is a sub-block of McBSP and it is the responsibility of the
McBSP driver to handle it. It has no effect or dependency on PRCM.
When the sidetone hwmod is removed the oh->dev_attr can not be used to
determine if the McBSP port has sidetone or not, but we can use the IRQ
names. If the module has IRQ named as sidetone, the McBSP has it.
Signed-off-by: Peter Ujfalusi <[email protected]>
---
arch/arm/mach-omap2/mcbsp.c | 28 +++++++++++++++++++++++++++-
1 file changed, 27 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-omap2/mcbsp.c b/arch/arm/mach-omap2/mcbsp.c
index b4ac3af1160c..8a3c50fa6a78 100644
--- a/arch/arm/mach-omap2/mcbsp.c
+++ b/arch/arm/mach-omap2/mcbsp.c
@@ -48,6 +48,23 @@ static int omap3_enable_st_clock(unsigned int id, bool enable)
return omap2_clk_allow_idle(mcbsp_iclks[id]);
}
+static bool omap3_mcbsp_has_st(struct omap_hwmod *oh)
+{
+ struct omap_hwmod_irq_info *ohii;
+ int i = 0;
+
+ if (!oh || !oh->mpu_irqs)
+ return false;
+
+ do {
+ ohii = &oh->mpu_irqs[i++];
+ if (!strcmp(ohii->name, "sidetone"))
+ return true;
+ } while (ohii->irq != -1);
+
+ return false;
+}
+
static int __init omap_init_mcbsp(struct omap_hwmod *oh, void *unused)
{
int id, count = 1;
@@ -56,6 +73,7 @@ static int __init omap_init_mcbsp(struct omap_hwmod *oh, void *unused)
struct omap_mcbsp_platform_data *pdata = NULL;
struct platform_device *pdev;
char clk_name[11];
+ bool has_sidetone = false;
sscanf(oh->name, "mcbsp%d", &id);
@@ -96,11 +114,19 @@ static int __init omap_init_mcbsp(struct omap_hwmod *oh, void *unused)
if (oh->dev_attr) {
oh_device[1] = omap_hwmod_lookup((
(struct omap_mcbsp_dev_attr *)(oh->dev_attr))->sidetone);
+ count++;
+
+ has_sidetone = true;
+ } else {
+ has_sidetone = omap3_mcbsp_has_st(oh);
+ }
+
+ if (has_sidetone) {
pdata->enable_st_clock = omap3_enable_st_clock;
sprintf(clk_name, "mcbsp%d_ick", id);
mcbsp_iclks[id] = clk_get(NULL, clk_name);
- count++;
}
+
pdev = omap_device_build_ss(name, id, oh_device, count, pdata,
sizeof(*pdata));
kfree(pdata);
--
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.
Signed-off-by: Peter Ujfalusi <[email protected]>
---
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 Fri, 18 Mar 2016, Peter Ujfalusi wrote:
> Hi,
>
> Chanes since v1:
> - removed the ASoC patch as Mark has applied it already
> - Added my signed-off to the hwmod patch
> - New patch to handle the case when the sidetone hwmod has been removed for
> legacy boot.
>
> 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.
NAK, at least without further discussion - see my comments on the v1 0/3
introduction.
- Paul
Paul,
On 03/19/16 21:38, Paul Walmsley wrote:
> On Fri, 18 Mar 2016, Peter Ujfalusi wrote:
>
>> Hi,
>>
>> Chanes since v1:
>> - removed the ASoC patch as Mark has applied it already
>> - Added my signed-off to the hwmod patch
>> - New patch to handle the case when the sidetone hwmod has been removed for
>> legacy boot.
>>
>> 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.
>
> NAK, at least without further discussion - see my comments on the v1 0/3
> introduction.
Yes, I could have sent the first series as RFC, but I believe(d) that this is
the correct way to fix the McBSP sidetone integration.
--
P?ter
Hi P?ter,
On Mon, 21 Mar 2016, Peter Ujfalusi wrote:
> On 03/19/16 21:38, Paul Walmsley wrote:
> > On Fri, 18 Mar 2016, Peter Ujfalusi wrote:
> >
> >> Hi,
> >>
> >> Chanes since v1:
> >> - removed the ASoC patch as Mark has applied it already
> >> - Added my signed-off to the hwmod patch
> >> - New patch to handle the case when the sidetone hwmod has been removed for
> >> legacy boot.
> >>
> >> 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.
> >
> > NAK, at least without further discussion - see my comments on the v1 0/3
> > introduction.
>
> Yes, I could have sent the first series as RFC, but I believe(d) that this is
> the correct way to fix the McBSP sidetone integration.
Yep not a problem. You're handling the process correctly. There's no
need to send things as an RFC unless you are unsure.
What you and I are doing now is exactly how the discussion and review
process is supposed to work.
- Paul
Hi Paul,
On 03/21/16 19:44, Paul Walmsley wrote:
> Hi P?ter,
>
> On Mon, 21 Mar 2016, Peter Ujfalusi wrote:
>
>> On 03/19/16 21:38, Paul Walmsley wrote:
>>> On Fri, 18 Mar 2016, Peter Ujfalusi wrote:
>>>
>>>> Hi,
>>>>
>>>> Chanes since v1:
>>>> - removed the ASoC patch as Mark has applied it already
>>>> - Added my signed-off to the hwmod patch
>>>> - New patch to handle the case when the sidetone hwmod has been removed for
>>>> legacy boot.
>>>>
>>>> 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.
>>>
>>> NAK, at least without further discussion - see my comments on the v1 0/3
>>> introduction.
>>
>> Yes, I could have sent the first series as RFC, but I believe(d) that this is
>> the correct way to fix the McBSP sidetone integration.
>
> Yep not a problem. You're handling the process correctly. There's no
> need to send things as an RFC unless you are unsure.
>
> What you and I are doing now is exactly how the discussion and review
> process is supposed to work.
So what shall we do with the OMAP3 McBSP2/3 sidetone? It has been broken in DT
boot since the first time we booted OMAP3 with DT... Only in legacy mode we
can have properly working ST.
I have the second level of patches based on this set (I think I need to resend
this series since I might have changed it, can not recall) for both arch/arm
and ASoC to have working ST in legacy and DT boot. We will no longer have
warning regarding to broken hwmod data in DT boot.
But all is based on the assumption that we agree at some point that the ST
block is part of the McBSP module ;)
If I need to write separate driver for the McBSP module's ST block, it would
mean some sort of API between the McBSP and ST driver. This is not straight
forward since there are registers both in McBSP block and ST block that needs
to be configured in specific order -> simple enable_st() would not work
(probably enable_st_stage1(), enable_st_stage2()) and callbacks from McBSP to
ST, ST to McBSP also going to be needed. As far as I can see it is going to be
a huge mess.
Other option would be to deprecate the ST support as such, but that would
leave the n900 guys in trouble as they need ST to be functional...
--
P?ter
Hi,
* Peter Ujfalusi <[email protected]> [160401 02:34]:
> So what shall we do with the OMAP3 McBSP2/3 sidetone? It has been broken in DT
> boot since the first time we booted OMAP3 with DT... Only in legacy mode we
> can have properly working ST.
Grr.
> I have the second level of patches based on this set (I think I need to resend
> this series since I might have changed it, can not recall) for both arch/arm
> and ASoC to have working ST in legacy and DT boot. We will no longer have
> warning regarding to broken hwmod data in DT boot.
> But all is based on the assumption that we agree at some point that the ST
> block is part of the McBSP module ;)
The sidetone module is a separate target from the McBSP on the interconnect
but there are also direct lines between sidetone and McBSP devices :)
Here's what I'm seeing looking at the AP table on dm3730 hardware.
McBSP target module:
0x49022000, ap 5 06.0, McBSP2
0x49024000, ap 7 08.0, McBSP3
Sidetone target modules:
0x49028000, ap 39 0a.0, mcbsp2_sidetone
0x4902a000, ap 41 12.0, mcbsp3_sidetone
And that seems to match TRM "21.6.4 SIDETONE Register Description",
"Table 2-5. L4-Peripheral Memory Space Mapping", and "Table 9-114. Region
Allocation for L4-Per Interconnect".
> If I need to write separate driver for the McBSP module's ST block, it would
> mean some sort of API between the McBSP and ST driver. This is not straight
> forward since there are registers both in McBSP block and ST block that needs
> to be configured in specific order -> simple enable_st() would not work
> (probably enable_st_stage1(), enable_st_stage2()) and callbacks from McBSP to
> ST, ST to McBSP also going to be needed. As far as I can see it is going to be
> a huge mess.
The McBSP and sidetone don't have parent child relationship at the
interconnect level. So I think the best option would be to have the McBSP
driver implement mcbsp_sidetone_register/unregister() etc functions. That
can then set up the necessary callbacks. Then the sidetone driver can call
them on probe/exit and set up the necessary callbacks and whatever might
be needed.
If they are currently handled in a single driver, you you need to
pm_runtime_get both modules. So having two separate drivers might make
things a lot simpler.
If you don't treat the McBSP and sidetone as separate modules, things can
easily fail. For example, doing a read-back to flush of posted write to
sidetone registers flushes nothing for McBSP and the other way around.
> Other option would be to deprecate the ST support as such, but that would
> leave the n900 guys in trouble as they need ST to be functional...
That does not sound like a nice option at all :(
Regards,
Tony
Tony,
On 04/02/16 03:17, Tony Lindgren wrote:
> Hi,
>
> * Peter Ujfalusi <[email protected]> [160401 02:34]:
>> So what shall we do with the OMAP3 McBSP2/3 sidetone? It has been broken in DT
>> boot since the first time we booted OMAP3 with DT... Only in legacy mode we
>> can have properly working ST.
>
> Grr.
Yes :(
The reason for this is that in DT boot we can not provide the
enable_st_clock() callback to the mcbsp driver stack. This is done for legacy
boot in mach-omap2/mcbsp.c
>> I have the second level of patches based on this set (I think I need to resend
>> this series since I might have changed it, can not recall) for both arch/arm
>> and ASoC to have working ST in legacy and DT boot. We will no longer have
>> warning regarding to broken hwmod data in DT boot.
>> But all is based on the assumption that we agree at some point that the ST
>> block is part of the McBSP module ;)
>
> The sidetone module is a separate target from the McBSP on the interconnect
> but there are also direct lines between sidetone and McBSP devices :)
> Here's what I'm seeing looking at the AP table on dm3730 hardware.
>
> McBSP target module:
> 0x49022000, ap 5 06.0, McBSP2
> 0x49024000, ap 7 08.0, McBSP3
>
> Sidetone target modules:
> 0x49028000, ap 39 0a.0, mcbsp2_sidetone
> 0x4902a000, ap 41 12.0, mcbsp3_sidetone
>
> And that seems to match TRM "21.6.4 SIDETONE Register Description",
> "Table 2-5. L4-Peripheral Memory Space Mapping", and "Table 9-114. Region
> Allocation for L4-Per Interconnect".
I'm aware of this, but even today we have one single driver to handle both
McBSP and the sidetone block.
>> If I need to write separate driver for the McBSP module's ST block, it would
>> mean some sort of API between the McBSP and ST driver. This is not straight
>> forward since there are registers both in McBSP block and ST block that needs
>> to be configured in specific order -> simple enable_st() would not work
>> (probably enable_st_stage1(), enable_st_stage2()) and callbacks from McBSP to
>> ST, ST to McBSP also going to be needed. As far as I can see it is going to be
>> a huge mess.
>
> The McBSP and sidetone don't have parent child relationship at the
> interconnect level. So I think the best option would be to have the McBSP
> driver implement mcbsp_sidetone_register/unregister() etc functions. That
> can then set up the necessary callbacks. Then the sidetone driver can call
> them on probe/exit and set up the necessary callbacks and whatever might
> be needed.
>
> If they are currently handled in a single driver, you you need to
> pm_runtime_get both modules.
The ST does not have clocks coming from PRCM level, it only uses the McBSP
iclk when it is enabled (the McBSP block of the McBSP). As far as pm_runtime
goes I think the ST module should not use it. We can not tell hwmod to
enable/disable the McBSP2/3 iclk when we pm_runtime for the ST. It does not
help at all. We can have nop action for the ST when pm_runtime is used, but
then why would we have it?
> So having two separate drivers might make things a lot simpler.
Not really. It will make things way more complicated imho. How to handle
legacy boot as we still have that supported? When the McBSP driver is loaded
we must know if it has sidetone or not so we can create the needed audio
controls, sysfs entries. The sysfs and kcontrol registration could be moved
out to the new ST driver, true.
I actually started with two separate drivers approach first, but decided that
it does not worth the effort (legacy boot support, pm_runtime/hwmod hassle,
platform data, callback API design, etc).
I know, it is not rocket science but it is king of shoot out of cannon into
sparrows.
I'll think about it for a little while ;)
> If you don't treat the McBSP and sidetone as separate modules, things can
> easily fail. For example, doing a read-back to flush of posted write to
> sidetone registers flushes nothing for McBSP and the other way around.
I don't see problem with the need of flushing if we would need it. I don't
think we are doing anything proactively to flush writes in the driver today
and we do have at least one product using the ST (n900).
>> Other option would be to deprecate the ST support as such, but that would
>> leave the n900 guys in trouble as they need ST to be functional...
>
> That does not sound like a nice option at all :(
I know. This has been bugging me for a long time. I want to fix this one
before my beagleboard-xm gives up and won't boot up anymore since after that I
will have no omap3 board to work with :(
--
P?ter
* Peter Ujfalusi <[email protected]> [160404 05:47]:
> On 04/02/16 03:17, Tony Lindgren wrote:
> > * Peter Ujfalusi <[email protected]> [160401 02:34]:
> >> So what shall we do with the OMAP3 McBSP2/3 sidetone? It has been broken in DT
> >> boot since the first time we booted OMAP3 with DT... Only in legacy mode we
> >> can have properly working ST.
> >
> > Grr.
>
> Yes :(
> The reason for this is that in DT boot we can not provide the
> enable_st_clock() callback to the mcbsp driver stack. This is done for legacy
> boot in mach-omap2/mcbsp.c
Seems like the short term fix there is to pass enable_st_clock pointer
in pdata using pdata-quirks.c. Then for the long term solution using
PM runtime to block gating of the clock while sidetone is active is
the way to go it seems.
> The ST does not have clocks coming from PRCM level, it only uses the McBSP
> iclk when it is enabled (the McBSP block of the McBSP). As far as pm_runtime
> goes I think the ST module should not use it. We can not tell hwmod to
> enable/disable the McBSP2/3 iclk when we pm_runtime for the ST. It does not
> help at all. We can have nop action for the ST when pm_runtime is used, but
> then why would we have it?
Using PM runtime in the sidetone driver should just work as long as the
sidetone device driver depends on the McBSP driver before it gets probed.
The clock framework handles things for the mcbsp ick with the usecount.
And doing pm_runtime_get() in the sidetone driver will do what the legacy
enable_st_clock() does currently.
> > So having two separate drivers might make things a lot simpler.
>
> Not really. It will make things way more complicated imho. How to handle
> legacy boot as we still have that supported?
Hey both the legacy driver and DT driver are really just platform devices
and drivers. And passing both dts and platform data can be done just
fine, no?
> When the McBSP driver is loaded we must know if it has sidetone or not so
> we can create the needed audio controls, sysfs entries. The sysfs and
> kcontrol registration could be moved out to the new ST driver, true.
Yeah during the probe, the sidetone driver must register with the McBSP
driver to tell it's there. I guess no need to pass anything in the
dts or platform_data for that.
> I actually started with two separate drivers approach first, but decided that
> it does not worth the effort (legacy boot support, pm_runtime/hwmod hassle,
> platform data, callback API design, etc).
> I know, it is not rocket science but it is king of shoot out of cannon into
> sparrows.
> I'll think about it for a little while ;)
Well what we've seen so far is that any kind of non-standard solution
will always be a pain to maintain in the long run :)
> > If you don't treat the McBSP and sidetone as separate modules, things can
> > easily fail. For example, doing a read-back to flush of posted write to
> > sidetone registers flushes nothing for McBSP and the other way around.
>
> I don't see problem with the need of flushing if we would need it. I don't
> think we are doing anything proactively to flush writes in the driver today
> and we do have at least one product using the ST (n900).
Usually the problem is with an interrupt ack write not reaching the device
in time before something else happens. So I could see mysterious issues
happening with the McBSP and sidetone having separate interrupts. Maybe
not a real problem, but the chance for it is still there for sure.
> >> Other option would be to deprecate the ST support as such, but that would
> >> leave the n900 guys in trouble as they need ST to be functional...
> >
> > That does not sound like a nice option at all :(
>
> I know. This has been bugging me for a long time. I want to fix this one
> before my beagleboard-xm gives up and won't boot up anymore since after that I
> will have no omap3 board to work with :(
There are plenty of cheap omap3 devices available out there though :)
Regards,
Tony
On 04/04/16 18:12, Tony Lindgren wrote:
>> The reason for this is that in DT boot we can not provide the
>> enable_st_clock() callback to the mcbsp driver stack. This is done for legacy
>> boot in mach-omap2/mcbsp.c
>
> Seems like the short term fix there is to pass enable_st_clock pointer
> in pdata using pdata-quirks.c.
I don't think there is a point to spend effort on a workaround via pdata-quirks.
> Then for the long term solution using
> PM runtime to block gating of the clock while sidetone is active is
> the way to go it seems.
Hrm, I think one of the main issue is that with pm_runtime we can not block
the clock gating, this is why legacy code uses enable_st_clock(), which will
call omap2_clk_deny_idle() or omap2_clk_allow_idle().
>> The ST does not have clocks coming from PRCM level, it only uses the McBSP
>> iclk when it is enabled (the McBSP block of the McBSP). As far as pm_runtime
>> goes I think the ST module should not use it. We can not tell hwmod to
>> enable/disable the McBSP2/3 iclk when we pm_runtime for the ST. It does not
>> help at all. We can have nop action for the ST when pm_runtime is used, but
>> then why would we have it?
>
> Using PM runtime in the sidetone driver should just work as long as the
> sidetone device driver depends on the McBSP driver before it gets probed.
> The clock framework handles things for the mcbsp ick with the usecount.
Yes, that is not the problem. The problem is that when McBSP is used w/o
sidetone the iclk can and should be autogated, but if the sidetone is enabled
then the iclk must not autogate and this needs to be prevented in PRCM level.
Note also that while the McBSP is running we must be able to enable/disable
the sidetone any time w/o affecting the McBSP operation.
> And doing pm_runtime_get() in the sidetone driver will do what the legacy
> enable_st_clock() does currently.
it can't do that as we do not have way to deny/enable just the autoidle for a
given clock.
>
>>> So having two separate drivers might make things a lot simpler.
>>
>> Not really. It will make things way more complicated imho. How to handle
>> legacy boot as we still have that supported?
>
> Hey both the legacy driver and DT driver are really just platform devices
> and drivers. And passing both dts and platform data can be done just
> fine, no?
Sure, but McBSP2-ST needs to bind with McBSP2 driver instance and the
McBSP3-ST should bind with McBSP3 instance. In legacy mode we can store the
McBSP pdev pointers in a array and use the devid or get the McBSP id from the
device name. While with DT boot we must have phandle pointing to/from ST
from/to McBSP node to be able to figure out who is who.
>> When the McBSP driver is loaded we must know if it has sidetone or not so
>> we can create the needed audio controls, sysfs entries. The sysfs and
>> kcontrol registration could be moved out to the new ST driver, true.
>
> Yeah during the probe, the sidetone driver must register with the McBSP
> driver to tell it's there.
When McBSP driver probes, it registers itself to ASoC core and it needs to
know at that point if we need to prepare for ST or not. So probably the McBSP
driver needs to register to ST driver?
> I guess no need to pass anything in the
> dts or platform_data for that.
>
>> I actually started with two separate drivers approach first, but decided that
>> it does not worth the effort (legacy boot support, pm_runtime/hwmod hassle,
>> platform data, callback API design, etc).
>> I know, it is not rocket science but it is king of shoot out of cannon into
>> sparrows.
>> I'll think about it for a little while ;)
>
> Well what we've seen so far is that any kind of non-standard solution
> will always be a pain to maintain in the long run :)
The current implementation (one driver to handle McBSP and the ST) is there
ever since OMAP3 was introduced afaik. Changing a working (was working) design
to something which has not been tested will for sure going to open issues we
have not prepared for.
I would avoid the rewrite of a proven driver architecture if it is not broken.
>>> If you don't treat the McBSP and sidetone as separate modules, things can
>>> easily fail. For example, doing a read-back to flush of posted write to
>>> sidetone registers flushes nothing for McBSP and the other way around.
>>
>> I don't see problem with the need of flushing if we would need it. I don't
>> think we are doing anything proactively to flush writes in the driver today
>> and we do have at least one product using the ST (n900).
>
> Usually the problem is with an interrupt ack write not reaching the device
> in time before something else happens. So I could see mysterious issues
> happening with the McBSP and sidetone having separate interrupts. Maybe
> not a real problem, but the chance for it is still there for sure.
The ST interrupt is not in use and the McBSP interrupt is only used for
debugging purposes as McBSP and it's sidetone is not interrupt driven devices.
>
>>>> Other option would be to deprecate the ST support as such, but that would
>>>> leave the n900 guys in trouble as they need ST to be functional...
>>>
>>> That does not sound like a nice option at all :(
>>
>> I know. This has been bugging me for a long time. I want to fix this one
>> before my beagleboard-xm gives up and won't boot up anymore since after that I
>> will have no omap3 board to work with :(
>
> There are plenty of cheap omap3 devices available out there though :)
I might need to look for one or two, preferably a board with support for
legacy and DT boot...
--
Péter
* Peter Ujfalusi <[email protected]> [160405 06:16]:
> On 04/04/16 18:12, Tony Lindgren wrote:
> >> The reason for this is that in DT boot we can not provide the
> >> enable_st_clock() callback to the mcbsp driver stack. This is done for legacy
> >> boot in mach-omap2/mcbsp.c
> >
> > Seems like the short term fix there is to pass enable_st_clock pointer
> > in pdata using pdata-quirks.c.
>
> I don't think there is a point to spend effort on a workaround via pdata-quirks.
>
> > Then for the long term solution using
> > PM runtime to block gating of the clock while sidetone is active is
> > the way to go it seems.
>
> Hrm, I think one of the main issue is that with pm_runtime we can not block
> the clock gating, this is why legacy code uses enable_st_clock(), which will
> call omap2_clk_deny_idle() or omap2_clk_allow_idle().
I see. I think Tero wanted to export omap2_clk_allow_idle() and
omap2_clk_deny_idle() for drivers to use. That should get discussed in
the linux-clk list, probably best to use the pdata callbacks until
the clock idling issue has been discussed.
> >> The ST does not have clocks coming from PRCM level, it only uses the McBSP
> >> iclk when it is enabled (the McBSP block of the McBSP). As far as pm_runtime
> >> goes I think the ST module should not use it. We can not tell hwmod to
> >> enable/disable the McBSP2/3 iclk when we pm_runtime for the ST. It does not
> >> help at all. We can have nop action for the ST when pm_runtime is used, but
> >> then why would we have it?
> >
> > Using PM runtime in the sidetone driver should just work as long as the
> > sidetone device driver depends on the McBSP driver before it gets probed.
> > The clock framework handles things for the mcbsp ick with the usecount.
>
> Yes, that is not the problem. The problem is that when McBSP is used w/o
> sidetone the iclk can and should be autogated, but if the sidetone is enabled
> then the iclk must not autogate and this needs to be prevented in PRCM level.
> Note also that while the McBSP is running we must be able to enable/disable
> the sidetone any time w/o affecting the McBSP operation.
OK I see.
> > And doing pm_runtime_get() in the sidetone driver will do what the legacy
> > enable_st_clock() does currently.
>
> it can't do that as we do not have way to deny/enable just the autoidle for a
> given clock.
Yup. So I suggest the pdata callbacs for the omap2_clk_allow_idle() and
omap2_clk_deny_idle() for now.
> >>> So having two separate drivers might make things a lot simpler.
> >>
> >> Not really. It will make things way more complicated imho. How to handle
> >> legacy boot as we still have that supported?
> >
> > Hey both the legacy driver and DT driver are really just platform devices
> > and drivers. And passing both dts and platform data can be done just
> > fine, no?
>
> Sure, but McBSP2-ST needs to bind with McBSP2 driver instance and the
> McBSP3-ST should bind with McBSP3 instance. In legacy mode we can store the
> McBSP pdev pointers in a array and use the devid or get the McBSP id from the
> device name. While with DT boot we must have phandle pointing to/from ST
> from/to McBSP node to be able to figure out who is who.
>
> >> When the McBSP driver is loaded we must know if it has sidetone or not so
> >> we can create the needed audio controls, sysfs entries. The sysfs and
> >> kcontrol registration could be moved out to the new ST driver, true.
> >
> > Yeah during the probe, the sidetone driver must register with the McBSP
> > driver to tell it's there.
>
> When McBSP driver probes, it registers itself to ASoC core and it needs to
> know at that point if we need to prepare for ST or not. So probably the McBSP
> driver needs to register to ST driver?
OK yes if that makes more sense.
> > I guess no need to pass anything in the
> > dts or platform_data for that.
> >
> >> I actually started with two separate drivers approach first, but decided that
> >> it does not worth the effort (legacy boot support, pm_runtime/hwmod hassle,
> >> platform data, callback API design, etc).
> >> I know, it is not rocket science but it is king of shoot out of cannon into
> >> sparrows.
> >> I'll think about it for a little while ;)
> >
> > Well what we've seen so far is that any kind of non-standard solution
> > will always be a pain to maintain in the long run :)
>
> The current implementation (one driver to handle McBSP and the ST) is there
> ever since OMAP3 was introduced afaik. Changing a working (was working) design
> to something which has not been tested will for sure going to open issues we
> have not prepared for.
> I would avoid the rewrite of a proven driver architecture if it is not broken.
Well probably the best thing to do is the use of platform callback
for now until we know how it can be done incrementally :)
Regards,
Tony
Tony,
On 04/12/16 00:28, Tony Lindgren wrote:
>>> Then for the long term solution using
>>> PM runtime to block gating of the clock while sidetone is active is
>>> the way to go it seems.
>>
>> Hrm, I think one of the main issue is that with pm_runtime we can not block
>> the clock gating, this is why legacy code uses enable_st_clock(), which will
>> call omap2_clk_deny_idle() or omap2_clk_allow_idle().
>
> I see. I think Tero wanted to export omap2_clk_allow_idle() and
> omap2_clk_deny_idle() for drivers to use. That should get discussed in
> the linux-clk list, probably best to use the pdata callbacks until
> the clock idling issue has been discussed.
It is already exported, used by the arch/arm/mach-omap2/mcbsp.c file.
>
>>>> The ST does not have clocks coming from PRCM level, it only uses the McBSP
>>>> iclk when it is enabled (the McBSP block of the McBSP). As far as pm_runtime
>>>> goes I think the ST module should not use it. We can not tell hwmod to
>>>> enable/disable the McBSP2/3 iclk when we pm_runtime for the ST. It does not
>>>> help at all. We can have nop action for the ST when pm_runtime is used, but
>>>> then why would we have it?
>>>
>>> Using PM runtime in the sidetone driver should just work as long as the
>>> sidetone device driver depends on the McBSP driver before it gets probed.
>>> The clock framework handles things for the mcbsp ick with the usecount.
>>
>> Yes, that is not the problem. The problem is that when McBSP is used w/o
>> sidetone the iclk can and should be autogated, but if the sidetone is enabled
>> then the iclk must not autogate and this needs to be prevented in PRCM level.
>> Note also that while the McBSP is running we must be able to enable/disable
>> the sidetone any time w/o affecting the McBSP operation.
>
> OK I see.
>
>>> And doing pm_runtime_get() in the sidetone driver will do what the legacy
>>> enable_st_clock() does currently.
>>
>> it can't do that as we do not have way to deny/enable just the autoidle for a
>> given clock.
>
> Yup. So I suggest the pdata callbacs for the omap2_clk_allow_idle() and
> omap2_clk_deny_idle() for now.
Why not to remove the callback for legacy also and handle it in the driver? It
is less ugly in my opinion.
Going via the pdata callback is just going to cement the current setup.
>>>>> So having two separate drivers might make things a lot simpler.
>>>>
>>>> Not really. It will make things way more complicated imho. How to handle
>>>> legacy boot as we still have that supported?
>>>
>>> Hey both the legacy driver and DT driver are really just platform devices
>>> and drivers. And passing both dts and platform data can be done just
>>> fine, no?
>>
>> Sure, but McBSP2-ST needs to bind with McBSP2 driver instance and the
>> McBSP3-ST should bind with McBSP3 instance. In legacy mode we can store the
>> McBSP pdev pointers in a array and use the devid or get the McBSP id from the
>> device name. While with DT boot we must have phandle pointing to/from ST
>> from/to McBSP node to be able to figure out who is who.
>>
>>>> When the McBSP driver is loaded we must know if it has sidetone or not so
>>>> we can create the needed audio controls, sysfs entries. The sysfs and
>>>> kcontrol registration could be moved out to the new ST driver, true.
>>>
>>> Yeah during the probe, the sidetone driver must register with the McBSP
>>> driver to tell it's there.
>>
>> When McBSP driver probes, it registers itself to ASoC core and it needs to
>> know at that point if we need to prepare for ST or not. So probably the McBSP
>> driver needs to register to ST driver?
>
> OK yes if that makes more sense.
I have drafted out something which would be needed if we separate the McBSP-ST
from the McBSP driver. It is not pretty...
In the new omap3-mcbsp-st.h:
struct omap3_mcbspst;
struct omap_st_to_mcbsp_data {
bool (*is_enabled)(struct omap_st_to_mcbsp_data *st_data);
bool (*enable)(struct omap_st_to_mcbsp_data *st_data);
bool (*disable)(struct omap_st_to_mcbsp_data *st_data);
struct omap3_mcbspst *st_priv;
};
In the current omap-mcbsp.h:
#include <omap3-mcbsp-st.h>
...
struct omap_mcbsp_to_st_data {
bool (*is_enabled)(struct omap_mcbsp_to_st_data *mcbsp_data);
bool (*iclk_idle)(struct omap_mcbsp_to_st_data *mcbsp_data, bool allow);
bool (*enable)(struct omap_mcbsp_to_st_data *mcbsp_data);
bool (*disable)(struct omap_mcbsp_to_st_data *mcbsp_data);
struct omap_mcbsp *mcbsp_priv;
};
#ifdef CONFIG_SND_SOC_OMAP3_MCBSPST
struct omap_mcbsp_to_st_data *omap_mcbsp_st_register(
struct platform_device *pdev, /* McBSP pdev! probably? */
struct omap_st_to_mcbsp_data *st_data);
int omap_mcbsp_st_unregister(struct omap_st_to_mcbsp_data *st_data);
#else
static inline int omap_mcbsp_st_register(struct platform_device *pdev,
struct omap_st_to_mcbsp_data *st_data)
{
return -ENODEV;
}
static inline int omap_mcbsp_st_unregister(struct omap_st_to_mcbsp_data *st_data)
{
return 0;
}
#endif
Since the ST would be separate driver, it should create the needed ALSA
controls as well, probably I need to pass something else here and there.
But, in this setup it would be possible to remove the ST driver while the
McBSP and the sound card is up, which means we must be able to remove
kcontrols runtime, probably there is a way, but not sure about this.
There will be issues like this we have not prepared for I'm sure if we do
dramatic change to the simple implementation we have right now.
>>> I guess no need to pass anything in the
>>> dts or platform_data for that.
>>>
>>>> I actually started with two separate drivers approach first, but decided that
>>>> it does not worth the effort (legacy boot support, pm_runtime/hwmod hassle,
>>>> platform data, callback API design, etc).
>>>> I know, it is not rocket science but it is king of shoot out of cannon into
>>>> sparrows.
>>>> I'll think about it for a little while ;)
>>>
>>> Well what we've seen so far is that any kind of non-standard solution
>>> will always be a pain to maintain in the long run :)
>>
>> The current implementation (one driver to handle McBSP and the ST) is there
>> ever since OMAP3 was introduced afaik. Changing a working (was working) design
>> to something which has not been tested will for sure going to open issues we
>> have not prepared for.
>> I would avoid the rewrite of a proven driver architecture if it is not broken.
>
> Well probably the best thing to do is the use of platform callback
> for now until we know how it can be done incrementally :)
I have reasonably clean patches (6 of them) on top of this three which would
remove the arch code for the iclk handling and implements it in the mcbsp
driver w/o changing the architecture of the McBSP driver itself. Both DT and
legacy boot works. The only part I was not happy about the one where I looked
up the mcbsp2/3_ick, but I think I have found much cleaner way to do it
(meaning that the code will not look hackish at all).
If you want to see, I can make this change and I can send the whole thing as
RFC and continue the discussion around that?
--
P?ter
* Peter Ujfalusi <[email protected]> [160412 02:53]:
> Tony,
>
> On 04/12/16 00:28, Tony Lindgren wrote:
> >>> Then for the long term solution using
> >>> PM runtime to block gating of the clock while sidetone is active is
> >>> the way to go it seems.
> >>
> >> Hrm, I think one of the main issue is that with pm_runtime we can not block
> >> the clock gating, this is why legacy code uses enable_st_clock(), which will
> >> call omap2_clk_deny_idle() or omap2_clk_allow_idle().
> >
> > I see. I think Tero wanted to export omap2_clk_allow_idle() and
> > omap2_clk_deny_idle() for drivers to use. That should get discussed in
> > the linux-clk list, probably best to use the pdata callbacks until
> > the clock idling issue has been discussed.
>
> It is already exported, used by the arch/arm/mach-omap2/mcbsp.c file.
Oh but not with EXPORT_SYMBOL so not usable except for built-in code.
Probably best to keep it that way IMO..
> Why not to remove the callback for legacy also and handle it in the driver? It
> is less ugly in my opinion.
> Going via the pdata callback is just going to cement the current setup.
Sure, maybe you can have a piece of built-in driver code to do that?
> I have drafted out something which would be needed if we separate the McBSP-ST
> from the McBSP driver. It is not pretty...
>
> In the new omap3-mcbsp-st.h:
>
> struct omap3_mcbspst;
>
> struct omap_st_to_mcbsp_data {
> bool (*is_enabled)(struct omap_st_to_mcbsp_data *st_data);
> bool (*enable)(struct omap_st_to_mcbsp_data *st_data);
> bool (*disable)(struct omap_st_to_mcbsp_data *st_data);
> struct omap3_mcbspst *st_priv;
> };
>
> In the current omap-mcbsp.h:
>
> #include <omap3-mcbsp-st.h>
> ...
> struct omap_mcbsp_to_st_data {
> bool (*is_enabled)(struct omap_mcbsp_to_st_data *mcbsp_data);
> bool (*iclk_idle)(struct omap_mcbsp_to_st_data *mcbsp_data, bool allow);
> bool (*enable)(struct omap_mcbsp_to_st_data *mcbsp_data);
> bool (*disable)(struct omap_mcbsp_to_st_data *mcbsp_data);
> struct omap_mcbsp *mcbsp_priv;
> };
>
> #ifdef CONFIG_SND_SOC_OMAP3_MCBSPST
> struct omap_mcbsp_to_st_data *omap_mcbsp_st_register(
> struct platform_device *pdev, /* McBSP pdev! probably? */
> struct omap_st_to_mcbsp_data *st_data);
> int omap_mcbsp_st_unregister(struct omap_st_to_mcbsp_data *st_data);
> #else
> static inline int omap_mcbsp_st_register(struct platform_device *pdev,
> struct omap_st_to_mcbsp_data *st_data)
> {
> return -ENODEV;
> }
> static inline int omap_mcbsp_st_unregister(struct omap_st_to_mcbsp_data *st_data)
> {
> return 0;
> }
> #endif
>
> Since the ST would be separate driver, it should create the needed ALSA
> controls as well, probably I need to pass something else here and there.
> But, in this setup it would be possible to remove the ST driver while the
> McBSP and the sound card is up, which means we must be able to remove
> kcontrols runtime, probably there is a way, but not sure about this.
>
> There will be issues like this we have not prepared for I'm sure if we do
> dramatic change to the simple implementation we have right now.
Best to stick to incremental improvments I think..
> I have reasonably clean patches (6 of them) on top of this three which would
> remove the arch code for the iclk handling and implements it in the mcbsp
> driver w/o changing the architecture of the McBSP driver itself. Both DT and
> legacy boot works. The only part I was not happy about the one where I looked
> up the mcbsp2/3_ick, but I think I have found much cleaner way to do it
> (meaning that the code will not look hackish at all).
> If you want to see, I can make this change and I can send the whole thing as
> RFC and continue the discussion around that?
Sure, especially if that helps with splitting up the modules too.
Regards,
Tony
On 04/12/16 19:37, Tony Lindgren wrote:
> * Peter Ujfalusi <[email protected]> [160412 02:53]:
>> Tony,
>>
>> On 04/12/16 00:28, Tony Lindgren wrote:
>>>>> Then for the long term solution using
>>>>> PM runtime to block gating of the clock while sidetone is active is
>>>>> the way to go it seems.
>>>>
>>>> Hrm, I think one of the main issue is that with pm_runtime we can not block
>>>> the clock gating, this is why legacy code uses enable_st_clock(), which will
>>>> call omap2_clk_deny_idle() or omap2_clk_allow_idle().
>>>
>>> I see. I think Tero wanted to export omap2_clk_allow_idle() and
>>> omap2_clk_deny_idle() for drivers to use. That should get discussed in
>>> the linux-clk list, probably best to use the pdata callbacks until
>>> the clock idling issue has been discussed.
>>
>> It is already exported, used by the arch/arm/mach-omap2/mcbsp.c file.
>
> Oh but not with EXPORT_SYMBOL so not usable except for built-in code.
> Probably best to keep it that way IMO..
It is up to Tero if he want to keep omap2_clk_allow/deny_idle() only be usable
for built in code. It is there just because of OMAP3 McBSP2/3 sidetone support
on the other hand. It is a fair assumption that it could be used by the driver.
>> Why not to remove the callback for legacy also and handle it in the driver? It
>> is less ugly in my opinion.
>> Going via the pdata callback is just going to cement the current setup.
>
> Sure, maybe you can have a piece of built-in driver code to do that?
You mean something like:
int omap3_mcbsp23_ick_for_sidetone_force(struct clk *clk, bool force_on)
{
if (!clk)
return 0;
if (force_on)
return omap2_clk_deny_idle(clk);
else
return omap2_clk_allow_idle(clk);
}
EXPORT_SYMBOL(omap3_mcbsp23_ick_for_sidetone_force);
Looks similarly hackish as with the pdata callback, but if I were to choose,
the pdata callback might be a bit more polite hack if we do not look at how we
will have the pdata crafted for DT boot.
>> I have drafted out something which would be needed if we separate the McBSP-ST
>> from the McBSP driver. It is not pretty...
>>
>> In the new omap3-mcbsp-st.h:
>>
>> struct omap3_mcbspst;
>>
>> struct omap_st_to_mcbsp_data {
>> bool (*is_enabled)(struct omap_st_to_mcbsp_data *st_data);
>> bool (*enable)(struct omap_st_to_mcbsp_data *st_data);
>> bool (*disable)(struct omap_st_to_mcbsp_data *st_data);
>> struct omap3_mcbspst *st_priv;
>> };
>>
>> In the current omap-mcbsp.h:
>>
>> #include <omap3-mcbsp-st.h>
>> ...
>> struct omap_mcbsp_to_st_data {
>> bool (*is_enabled)(struct omap_mcbsp_to_st_data *mcbsp_data);
>> bool (*iclk_idle)(struct omap_mcbsp_to_st_data *mcbsp_data, bool allow);
>> bool (*enable)(struct omap_mcbsp_to_st_data *mcbsp_data);
>> bool (*disable)(struct omap_mcbsp_to_st_data *mcbsp_data);
>> struct omap_mcbsp *mcbsp_priv;
>> };
>>
>> #ifdef CONFIG_SND_SOC_OMAP3_MCBSPST
>> struct omap_mcbsp_to_st_data *omap_mcbsp_st_register(
>> struct platform_device *pdev, /* McBSP pdev! probably? */
>> struct omap_st_to_mcbsp_data *st_data);
>> int omap_mcbsp_st_unregister(struct omap_st_to_mcbsp_data *st_data);
>> #else
>> static inline int omap_mcbsp_st_register(struct platform_device *pdev,
>> struct omap_st_to_mcbsp_data *st_data)
>> {
>> return -ENODEV;
>> }
>> static inline int omap_mcbsp_st_unregister(struct omap_st_to_mcbsp_data *st_data)
>> {
>> return 0;
>> }
>> #endif
>>
>> Since the ST would be separate driver, it should create the needed ALSA
>> controls as well, probably I need to pass something else here and there.
>> But, in this setup it would be possible to remove the ST driver while the
>> McBSP and the sound card is up, which means we must be able to remove
>> kcontrols runtime, probably there is a way, but not sure about this.
>>
>> There will be issues like this we have not prepared for I'm sure if we do
>> dramatic change to the simple implementation we have right now.
>
> Best to stick to incremental improvments I think..
If we were to split the McBSP driver into half - not literally as the ST
support has small amount of code right now, we would consider all possibility
to not introduce regression and keep things working along the way. There will
be a point were the code need to be shuffled..
>> I have reasonably clean patches (6 of them) on top of this three which would
>> remove the arch code for the iclk handling and implements it in the mcbsp
>> driver w/o changing the architecture of the McBSP driver itself. Both DT and
>> legacy boot works. The only part I was not happy about the one where I looked
>> up the mcbsp2/3_ick, but I think I have found much cleaner way to do it
>> (meaning that the code will not look hackish at all).
>> If you want to see, I can make this change and I can send the whole thing as
>> RFC and continue the discussion around that?
>
> Sure, especially if that helps with splitting up the modules too.
To start with the hwmod data is wrong for mcbsp2/3 mcbsp2/3_sidetone:
static struct omap_hwmod omap3xxx_mcbsp2_hwmod = {
.name = "mcbsp2",
.class = &omap3xxx_mcbsp_hwmod_class,
.mpu_irqs = omap3xxx_mcbsp2_irqs,
.sdma_reqs = omap2_mcbsp2_sdma_reqs,
.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,
},
},
.opt_clks = mcbsp234_opt_clks,
.opt_clks_cnt = ARRAY_SIZE(mcbsp234_opt_clks),
.dev_attr = &omap34xx_mcbsp2_dev_attr,
};
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,
},
},
};
The McBSP2_ST main_clk is mcbsp2_ick, not mcbsp2_fck (ch 21.3.2.2.6 in
OMAP36xx TRM).
The sidetone should not have prcm section at all as it does not have control
over it's clocks in that level. Giving the same PRCM registers and bits for
both McBSP and it's sidetone is wrong. What should be expected if McBSP is
enabled and we disable the ST via pm_runtime? Will the hwmod toggle bits in
PRCM? If it does, the McBSP will looses it's clocks...
While the McBSP and ST regions are different, the ST is part of the McBSP from
PRCM point of view so not sure how this could be worked around with separated
drivers.
--
P?ter
* Peter Ujfalusi <[email protected]> [160413 04:59]:
> On 04/12/16 19:37, Tony Lindgren wrote:
> > * Peter Ujfalusi <[email protected]> [160412 02:53]:
> >> Tony,
> >>
> >> On 04/12/16 00:28, Tony Lindgren wrote:
> >>>>> Then for the long term solution using
> >>>>> PM runtime to block gating of the clock while sidetone is active is
> >>>>> the way to go it seems.
> >>>>
> >>>> Hrm, I think one of the main issue is that with pm_runtime we can not block
> >>>> the clock gating, this is why legacy code uses enable_st_clock(), which will
> >>>> call omap2_clk_deny_idle() or omap2_clk_allow_idle().
> >>>
> >>> I see. I think Tero wanted to export omap2_clk_allow_idle() and
> >>> omap2_clk_deny_idle() for drivers to use. That should get discussed in
> >>> the linux-clk list, probably best to use the pdata callbacks until
> >>> the clock idling issue has been discussed.
> >>
> >> It is already exported, used by the arch/arm/mach-omap2/mcbsp.c file.
> >
> > Oh but not with EXPORT_SYMBOL so not usable except for built-in code.
> > Probably best to keep it that way IMO..
>
> It is up to Tero if he want to keep omap2_clk_allow/deny_idle() only be usable
> for built in code. It is there just because of OMAP3 McBSP2/3 sidetone support
> on the other hand. It is a fair assumption that it could be used by the driver.
The problem with EXPORT_SYMBOL is that it tends to then suddenly then get
used all over the drivers. And we've seen what that means..
> >> Why not to remove the callback for legacy also and handle it in the driver? It
> >> is less ugly in my opinion.
> >> Going via the pdata callback is just going to cement the current setup.
> >
> > Sure, maybe you can have a piece of built-in driver code to do that?
>
> You mean something like:
> int omap3_mcbsp23_ick_for_sidetone_force(struct clk *clk, bool force_on)
> {
> if (!clk)
> return 0;
>
> if (force_on)
> return omap2_clk_deny_idle(clk);
> else
> return omap2_clk_allow_idle(clk);
> }
> EXPORT_SYMBOL(omap3_mcbsp23_ick_for_sidetone_force);
Yeah that's something I was thinking.
> Looks similarly hackish as with the pdata callback, but if I were to choose,
> the pdata callback might be a bit more polite hack if we do not look at how we
> will have the pdata crafted for DT boot.
Well the pdata solution avoids exporting custom functions to all the
drivers.
> If we were to split the McBSP driver into half - not literally as the ST
> support has small amount of code right now, we would consider all possibility
> to not introduce regression and keep things working along the way. There will
> be a point were the code need to be shuffled..
You could just create the sidetone child device manually on probe in the
driver as needed. That way you'd have two devices to do the PM runtime
on. I think that was Paul's main concern as they are separate modules.
It still leaves the chance of bugs with flush of posted writes. But might
make things easier to deal with in small steps?
> >> I have reasonably clean patches (6 of them) on top of this three which would
> >> remove the arch code for the iclk handling and implements it in the mcbsp
> >> driver w/o changing the architecture of the McBSP driver itself. Both DT and
> >> legacy boot works. The only part I was not happy about the one where I looked
> >> up the mcbsp2/3_ick, but I think I have found much cleaner way to do it
> >> (meaning that the code will not look hackish at all).
> >> If you want to see, I can make this change and I can send the whole thing as
> >> RFC and continue the discussion around that?
> >
> > Sure, especially if that helps with splitting up the modules too.
>
> To start with the hwmod data is wrong for mcbsp2/3 mcbsp2/3_sidetone:
> static struct omap_hwmod omap3xxx_mcbsp2_hwmod = {
> .name = "mcbsp2",
> .class = &omap3xxx_mcbsp_hwmod_class,
> .mpu_irqs = omap3xxx_mcbsp2_irqs,
> .sdma_reqs = omap2_mcbsp2_sdma_reqs,
> .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,
> },
> },
> .opt_clks = mcbsp234_opt_clks,
> .opt_clks_cnt = ARRAY_SIZE(mcbsp234_opt_clks),
> .dev_attr = &omap34xx_mcbsp2_dev_attr,
> };
>
> 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,
> },
> },
> };
>
> The McBSP2_ST main_clk is mcbsp2_ick, not mcbsp2_fck (ch 21.3.2.2.6 in
> OMAP36xx TRM).
> The sidetone should not have prcm section at all as it does not have control
> over it's clocks in that level. Giving the same PRCM registers and bits for
> both McBSP and it's sidetone is wrong. What should be expected if McBSP is
> enabled and we disable the ST via pm_runtime? Will the hwmod toggle bits in
> PRCM? If it does, the McBSP will looses it's clocks...
I think for omap3 we're just using the clk_get/set. For omap4, the issue
is different as the clkctrl registers are used directly.
> While the McBSP and ST regions are different, the ST is part of the McBSP from
> PRCM point of view so not sure how this could be worked around with separated
> drivers.
Just create the struct device as a child for ST as needed from McBSP?
Regards,
Tony
On 04/13/16 18:28, Tony Lindgren wrote:
>>> Oh but not with EXPORT_SYMBOL so not usable except for built-in code.
>>> Probably best to keep it that way IMO..
>>
>> It is up to Tero if he want to keep omap2_clk_allow/deny_idle() only be usable
>> for built in code. It is there just because of OMAP3 McBSP2/3 sidetone support
>> on the other hand. It is a fair assumption that it could be used by the driver.
>
> The problem with EXPORT_SYMBOL is that it tends to then suddenly then get
> used all over the drivers. And we've seen what that means..
:) Yes, I agree.
>>>> Why not to remove the callback for legacy also and handle it in the driver? It
>>>> is less ugly in my opinion.
>>>> Going via the pdata callback is just going to cement the current setup.
>>>
>>> Sure, maybe you can have a piece of built-in driver code to do that?
>>
>> You mean something like:
>> int omap3_mcbsp23_ick_for_sidetone_force(struct clk *clk, bool force_on)
>> {
>> if (!clk)
>> return 0;
>>
>> if (force_on)
>> return omap2_clk_deny_idle(clk);
>> else
>> return omap2_clk_allow_idle(clk);
>> }
>> EXPORT_SYMBOL(omap3_mcbsp23_ick_for_sidetone_force);
>
> Yeah that's something I was thinking.
>
>> Looks similarly hackish as with the pdata callback, but if I were to choose,
>> the pdata callback might be a bit more polite hack if we do not look at how we
>> will have the pdata crafted for DT boot.
>
> Well the pdata solution avoids exporting custom functions to all the
> drivers.
True. In this light the pdata callback is much sensible thing to do.
I will look at the DT side of crafting out this.
>> If we were to split the McBSP driver into half - not literally as the ST
>> support has small amount of code right now, we would consider all possibility
>> to not introduce regression and keep things working along the way. There will
>> be a point were the code need to be shuffled..
>
> You could just create the sidetone child device manually on probe in the
> driver as needed. That way you'd have two devices to do the PM runtime
> on. I think that was Paul's main concern as they are separate modules.
You mean that not to have separate compatible for the McBSP module's Sidetone
core?
If yes, then it is a valid thing to remove the hwmod data for the sidetone,
like I did in this series.
> It still leaves the chance of bugs with flush of posted writes. But might
> make things easier to deal with in small steps?
The only 'benefit' I see with separated driver for McBSP core and Sidetone
core is that the register writes will happen to the cores in separate drivers.
If the McBSP driver creates the device for the sidetone driver, then passing
the needed callbacks and data to it is going to be cleaner. Registering back
the callbacks to McBSP is what need to be figured out, so it is simple and
clean. Either with a callback to McBSP to set the ST callbacks or have the
callback struct used by ST via pdata to have places for the ST to McBSP
callbacks and when the driver loads it is going to set up those.
>>>> I have reasonably clean patches (6 of them) on top of this three which would
>>>> remove the arch code for the iclk handling and implements it in the mcbsp
>>>> driver w/o changing the architecture of the McBSP driver itself. Both DT and
>>>> legacy boot works. The only part I was not happy about the one where I looked
>>>> up the mcbsp2/3_ick, but I think I have found much cleaner way to do it
>>>> (meaning that the code will not look hackish at all).
>>>> If you want to see, I can make this change and I can send the whole thing as
>>>> RFC and continue the discussion around that?
>>>
>>> Sure, especially if that helps with splitting up the modules too.
>>
>> To start with the hwmod data is wrong for mcbsp2/3 mcbsp2/3_sidetone:
>> static struct omap_hwmod omap3xxx_mcbsp2_hwmod = {
>> .name = "mcbsp2",
>> .class = &omap3xxx_mcbsp_hwmod_class,
>> .mpu_irqs = omap3xxx_mcbsp2_irqs,
>> .sdma_reqs = omap2_mcbsp2_sdma_reqs,
>> .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,
>> },
>> },
>> .opt_clks = mcbsp234_opt_clks,
>> .opt_clks_cnt = ARRAY_SIZE(mcbsp234_opt_clks),
>> .dev_attr = &omap34xx_mcbsp2_dev_attr,
>> };
>>
>> 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,
>> },
>> },
>> };
>>
>> The McBSP2_ST main_clk is mcbsp2_ick, not mcbsp2_fck (ch 21.3.2.2.6 in
>> OMAP36xx TRM).
>> The sidetone should not have prcm section at all as it does not have control
>> over it's clocks in that level. Giving the same PRCM registers and bits for
>> both McBSP and it's sidetone is wrong. What should be expected if McBSP is
>> enabled and we disable the ST via pm_runtime? Will the hwmod toggle bits in
>> PRCM? If it does, the McBSP will looses it's clocks...
>
> I think for omap3 we're just using the clk_get/set. For omap4, the issue
> is different as the clkctrl registers are used directly.
If I remove the prcm section for the ST hwmod:
[ 87.784820] omap_hwmod: mcbsp2_sidetone: _wait_target_ready failed: -22
[ 87.784851] omap-mcbsp 49022000.mcbsp: use pm_runtime_put_sync_suspend() in
driver?
When first try to use the audio.
So the hwmod code at least was checking the idlest bit.
>> While the McBSP and ST regions are different, the ST is part of the McBSP from
>> PRCM point of view so not sure how this could be worked around with separated
>> drivers.
>
> Just create the struct device as a child for ST as needed from McBSP?
OK. I will go with the assumption that the sidetone hwmod can be removed (as
it is not correct) and rework my current series to use pdata callback for the
iclk autogate allow/deny. With this set the ST will be operational in legacy
and DT boot.
>From there I can start to draft out the needed architecture to separate the ST
core handing into a new driver and when it looks good I can make the change.
How this sounds?
--
P?ter
* Peter Ujfalusi <[email protected]> [160414 00:35]:
> On 04/13/16 18:28, Tony Lindgren wrote:
> >
> > You could just create the sidetone child device manually on probe in the
> > driver as needed. That way you'd have two devices to do the PM runtime
> > on. I think that was Paul's main concern as they are separate modules.
>
> You mean that not to have separate compatible for the McBSP module's Sidetone
> core?
> If yes, then it is a valid thing to remove the hwmod data for the sidetone,
> like I did in this series.
No, I meant keep the sidetone hwmod, it really is there in the hardware.
I meant only probe the sidetone in the McBSP probe so you have two
struct dev and two hwmod entries in the McBSP driver. I don't know if
this actually makes things easier or not though.
> > It still leaves the chance of bugs with flush of posted writes. But might
> > make things easier to deal with in small steps?
>
> The only 'benefit' I see with separated driver for McBSP core and Sidetone
> core is that the register writes will happen to the cores in separate drivers.
>
> If the McBSP driver creates the device for the sidetone driver, then passing
> the needed callbacks and data to it is going to be cleaner. Registering back
> the callbacks to McBSP is what need to be figured out, so it is simple and
> clean. Either with a callback to McBSP to set the ST callbacks or have the
> callback struct used by ST via pdata to have places for the ST to McBSP
> callbacks and when the driver loads it is going to set up those.
OK yeah makes sense to me.
> If I remove the prcm section for the ST hwmod:
> [ 87.784820] omap_hwmod: mcbsp2_sidetone: _wait_target_ready failed: -22
> [ 87.784851] omap-mcbsp 49022000.mcbsp: use pm_runtime_put_sync_suspend() in
> driver?
>
> When first try to use the audio.
> So the hwmod code at least was checking the idlest bit.
Yes the module is really there for sidetone, and it really has hardware
registers :)
> OK. I will go with the assumption that the sidetone hwmod can be removed (as
> it is not correct) and rework my current series to use pdata callback for the
> iclk autogate allow/deny. With this set the ST will be operational in legacy
> and DT boot.
Sorry, no I did not want to drop the sidetone hwmod, I was just trying to
come up with ideas on how to make the driver changes easier. It sounds like
you already figured out the driver changes part though with two drivers.
Regards,
Tony
On 04/14/2016 07:55 PM, Tony Lindgren wrote:
> * Peter Ujfalusi <[email protected]> [160414 00:35]:
>> On 04/13/16 18:28, Tony Lindgren wrote:
>>>
>>> You could just create the sidetone child device manually on probe in the
>>> driver as needed. That way you'd have two devices to do the PM runtime
>>> on. I think that was Paul's main concern as they are separate modules.
>>
>> You mean that not to have separate compatible for the McBSP module's Sidetone
>> core?
>> If yes, then it is a valid thing to remove the hwmod data for the sidetone,
>> like I did in this series.
>
> No, I meant keep the sidetone hwmod, it really is there in the hardware.
Hrm, the Sidetone is there, yes. It is part of the McBSP module and the hwmod
for the sidetone is not correct. From hwmod (or PRCM) point of view there is
no sidetone module, there is only McBSP module which consist of McBSP core and
Sidetone core.
> I meant only probe the sidetone in the McBSP probe so you have two
> struct dev and two hwmod entries in the McBSP driver. I don't know if
> this actually makes things easier or not though.
But the hwmod for the sidetone is wrong, there should not have been hwmod for
the sidetone to start with.
>>> It still leaves the chance of bugs with flush of posted writes. But might
>>> make things easier to deal with in small steps?
>>
>> The only 'benefit' I see with separated driver for McBSP core and Sidetone
>> core is that the register writes will happen to the cores in separate drivers.
>>
>> If the McBSP driver creates the device for the sidetone driver, then passing
>> the needed callbacks and data to it is going to be cleaner. Registering back
>> the callbacks to McBSP is what need to be figured out, so it is simple and
>> clean. Either with a callback to McBSP to set the ST callbacks or have the
>> callback struct used by ST via pdata to have places for the ST to McBSP
>> callbacks and when the driver loads it is going to set up those.
>
> OK yeah makes sense to me.
>
>> If I remove the prcm section for the ST hwmod:
>> [ 87.784820] omap_hwmod: mcbsp2_sidetone: _wait_target_ready failed: -22
>> [ 87.784851] omap-mcbsp 49022000.mcbsp: use pm_runtime_put_sync_suspend() in
>> driver?
>>
>> When first try to use the audio.
>> So the hwmod code at least was checking the idlest bit.
>
> Yes the module is really there for sidetone, and it really has hardware
> registers :)
Yes it has registers, but it has no prcm level existence, it is part of McBSP
module. I guess when the OMAP3 was designed the HW people did not wanted to
create new version of the McBSP core for McBSP2/3 so they attached a new core
to the McBSP cores with different targets, etc, but w/o external dependency.
>> OK. I will go with the assumption that the sidetone hwmod can be removed (as
>> it is not correct) and rework my current series to use pdata callback for the
>> iclk autogate allow/deny. With this set the ST will be operational in legacy
>> and DT boot.
>
> Sorry, no I did not want to drop the sidetone hwmod, I was just trying to
> come up with ideas on how to make the driver changes easier. It sounds like
> you already figured out the driver changes part though with two drivers.
If I need to keep the sidetone hwmod around I don't see how it can be done in
a safe and clean way. It is part of McBSP module, it is accessible only if the
McBSP module is enabled, you can not enable the Sidetone alone you need to go
and enable the McBSP module. I don't think it is a good idea to let two
separate hwmods to poke around the same PRCM bits. Have not checked, but I
don't think we have refcounting for the PRCM register bits.
I have things working w/o the two drivers with pdata callback both in legacy
and DT case and it is pretty neat looking, thanks for the suggestion! I'm
still figuring out the needed amount of callbacks from McBSP to ST and from ST
to McBSP. We for sure going to need enable_stage1,2 probably three as well.
But this crossing driver boundaries needs a bit more time to figure out.
--
P?ter
* Peter Ujfalusi <[email protected]> [160414 12:38]:
>
> Yes it has registers, but it has no prcm level existence, it is part of McBSP
> module. I guess when the OMAP3 was designed the HW people did not wanted to
> create new version of the McBSP core for McBSP2/3 so they attached a new core
> to the McBSP cores with different targets, etc, but w/o external dependency.
Yeah well we do have a bunch of modules that don't need any separate
functional clock and are clocked only by the interface clock. So in this
case McBSP and sidetone are both consumers for the clock we just happen
to call McBSP interface clock. They should be able to share that no
problem.
> >> OK. I will go with the assumption that the sidetone hwmod can be removed (as
> >> it is not correct) and rework my current series to use pdata callback for the
> >> iclk autogate allow/deny. With this set the ST will be operational in legacy
> >> and DT boot.
> >
> > Sorry, no I did not want to drop the sidetone hwmod, I was just trying to
> > come up with ideas on how to make the driver changes easier. It sounds like
> > you already figured out the driver changes part though with two drivers.
>
> If I need to keep the sidetone hwmod around I don't see how it can be done in
> a safe and clean way. It is part of McBSP module, it is accessible only if the
> McBSP module is enabled, you can not enable the Sidetone alone you need to go
> and enable the McBSP module. I don't think it is a good idea to let two
> separate hwmods to poke around the same PRCM bits. Have not checked, but I
> don't think we have refcounting for the PRCM register bits.
Yeah there's no refcounting on the PRCM, but the clock framework has it
for the share McBSP interface clock. Then there are two separate sets of
sysconfig registers that PM runtime should manage. And then there's the
clock autogating issue.
Note that we do have an issue with the omap4 and later clkctrl registers
that don't have refcounting. Tero's clkctrl work will sort out that
issue. I don't think we have a similar issue with omap3.
So from that point of view the two separate hwmod modules should work
just fine sharing the clock.
> I have things working w/o the two drivers with pdata callback both in legacy
> and DT case and it is pretty neat looking, thanks for the suggestion! I'm
> still figuring out the needed amount of callbacks from McBSP to ST and from ST
> to McBSP. We for sure going to need enable_stage1,2 probably three as well.
> But this crossing driver boundaries needs a bit more time to figure out.
Yeah I still think we need to struct device instances, one to manage
McBSP and the other to manage sidetone :)
Regards,
Tony
On 04/14/16 23:34, Tony Lindgren wrote:
> * Peter Ujfalusi <[email protected]> [160414 12:38]:
>>
>> Yes it has registers, but it has no prcm level existence, it is part of McBSP
>> module. I guess when the OMAP3 was designed the HW people did not wanted to
>> create new version of the McBSP core for McBSP2/3 so they attached a new core
>> to the McBSP cores with different targets, etc, but w/o external dependency.
>
> Yeah well we do have a bunch of modules that don't need any separate
> functional clock and are clocked only by the interface clock. So in this
> case McBSP and sidetone are both consumers for the clock we just happen
> to call McBSP interface clock. They should be able to share that no
> problem.
>
>>>> OK. I will go with the assumption that the sidetone hwmod can be removed (as
>>>> it is not correct) and rework my current series to use pdata callback for the
>>>> iclk autogate allow/deny. With this set the ST will be operational in legacy
>>>> and DT boot.
>>>
>>> Sorry, no I did not want to drop the sidetone hwmod, I was just trying to
>>> come up with ideas on how to make the driver changes easier. It sounds like
>>> you already figured out the driver changes part though with two drivers.
>>
>> If I need to keep the sidetone hwmod around I don't see how it can be done in
>> a safe and clean way. It is part of McBSP module, it is accessible only if the
>> McBSP module is enabled, you can not enable the Sidetone alone you need to go
>> and enable the McBSP module. I don't think it is a good idea to let two
>> separate hwmods to poke around the same PRCM bits. Have not checked, but I
>> don't think we have refcounting for the PRCM register bits.
>
> Yeah there's no refcounting on the PRCM, but the clock framework has it
> for the share McBSP interface clock.
The hwmod checks the bits described by prcm.omap2. If two hwmods are set up to
manage/monitor the same bits in PRCM, what will happen when the two driver
does pm_runtime?
CM_ICLKEN_PER[0] = 1
McBSP2: runtime_get_sync()
CM_ICLKEN_PER[0] = 0
...
CM_ICLKEN_PER[0] = 0
McBSP2.ST: runtime_get_sync() // hwmod might complain as the idlest was not 1?
CM_ICLKEN_PER[0] = 0
...
CM_ICLKEN_PER[0] = 0
McBSP2.ST: runtime_put_sync()
CM_ICLKEN_PER[0] = 0 // hwmod might warn that the module did not went idle?
...
CM_ICLKEN_PER[0] = 0
McBSP2: runtime_put_sync()
CM_ICLKEN_PER[0] = 1
We can hack this around by adding HWMOD_NO_IDLEST to the sidetone hwmod I
guess. As the sidetone does not have PRCM level control - it is part of McBSP.
> Then there are two separate sets of sysconfig registers that PM runtime should manage.
The sidetone core's sysconfig register is internal to McBSP module. This is
what the TRM has to say about 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 ST_SYSCONFIG_REG is internal to the McBSP module the ST is integrated into.
> And then there's the clock autogating issue.
>
> Note that we do have an issue with the omap4 and later clkctrl registers
> that don't have refcounting. Tero's clkctrl work will sort out that
> issue. I don't think we have a similar issue with omap3.
>
> So from that point of view the two separate hwmod modules should work
> just fine sharing the clock.
>
>> I have things working w/o the two drivers with pdata callback both in legacy
>> and DT case and it is pretty neat looking, thanks for the suggestion! I'm
>> still figuring out the needed amount of callbacks from McBSP to ST and from ST
>> to McBSP. We for sure going to need enable_stage1,2 probably three as well.
>> But this crossing driver boundaries needs a bit more time to figure out.
>
> Yeah I still think we need to struct device instances, one to manage
> McBSP and the other to manage sidetone :)
I'm still not convinced about the benefits of creating separate device for the
ST core of McBSP.
>From my point of view:
McBSP2 module consist of:
- McBSP core
- clock generator subcore
- tx subcore
- rx subcore
- Sidetone core
it is one piece of IP.
I don't know why the hw guys decided to not extend the McBSP IP itself with
the sidetone feature, I'm sure they had their reasons to not touch the McBSP
core, but to add another core to the McBSP module.
The documentation also treats the Sidetone as an additional feature to the
McBSP core functionality.
--
P?ter
* Peter Ujfalusi <[email protected]> [160415 03:24]:
> On 04/14/16 23:34, Tony Lindgren wrote:
> > * Peter Ujfalusi <[email protected]> [160414 12:38]:
> >>
> >> Yes it has registers, but it has no prcm level existence, it is part of McBSP
> >> module. I guess when the OMAP3 was designed the HW people did not wanted to
> >> create new version of the McBSP core for McBSP2/3 so they attached a new core
> >> to the McBSP cores with different targets, etc, but w/o external dependency.
> >
> > Yeah well we do have a bunch of modules that don't need any separate
> > functional clock and are clocked only by the interface clock. So in this
> > case McBSP and sidetone are both consumers for the clock we just happen
> > to call McBSP interface clock. They should be able to share that no
> > problem.
> >
> >>>> OK. I will go with the assumption that the sidetone hwmod can be removed (as
> >>>> it is not correct) and rework my current series to use pdata callback for the
> >>>> iclk autogate allow/deny. With this set the ST will be operational in legacy
> >>>> and DT boot.
> >>>
> >>> Sorry, no I did not want to drop the sidetone hwmod, I was just trying to
> >>> come up with ideas on how to make the driver changes easier. It sounds like
> >>> you already figured out the driver changes part though with two drivers.
> >>
> >> If I need to keep the sidetone hwmod around I don't see how it can be done in
> >> a safe and clean way. It is part of McBSP module, it is accessible only if the
> >> McBSP module is enabled, you can not enable the Sidetone alone you need to go
> >> and enable the McBSP module. I don't think it is a good idea to let two
> >> separate hwmods to poke around the same PRCM bits. Have not checked, but I
> >> don't think we have refcounting for the PRCM register bits.
> >
> > Yeah there's no refcounting on the PRCM, but the clock framework has it
> > for the share McBSP interface clock.
>
> The hwmod checks the bits described by prcm.omap2. If two hwmods are set up to
> manage/monitor the same bits in PRCM, what will happen when the two driver
> does pm_runtime?
>
> CM_ICLKEN_PER[0] = 1
> McBSP2: runtime_get_sync()
> CM_ICLKEN_PER[0] = 0
> ...
> CM_ICLKEN_PER[0] = 0
> McBSP2.ST: runtime_get_sync() // hwmod might complain as the idlest was not 1?
> CM_ICLKEN_PER[0] = 0
> ...
> CM_ICLKEN_PER[0] = 0
> McBSP2.ST: runtime_put_sync()
> CM_ICLKEN_PER[0] = 0 // hwmod might warn that the module did not went idle?
> ...
> CM_ICLKEN_PER[0] = 0
> McBSP2: runtime_put_sync()
> CM_ICLKEN_PER[0] = 1
>
> We can hack this around by adding HWMOD_NO_IDLEST to the sidetone hwmod I
> guess. As the sidetone does not have PRCM level control - it is part of McBSP.
Heh if they are using the same register bits for two separate modules,
then that's a bug for sure :) I think the sidetone module only has the
clock gating bit in the ST_SYSCONFIG.
> > Then there are two separate sets of sysconfig registers that PM runtime should manage.
>
> The sidetone core's sysconfig register is internal to McBSP module. This is
> what the TRM has to say about 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.
Some confusion here.. The McBSPi_ICLK is external, it's just shared
between the McBSP and sidetone modules. So the ST_SYSCONFIG gates
internally separately to the sidetone.
> 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 ST_SYSCONFIG_REG is internal to the McBSP module the ST is integrated into.
The ST_SYSCONFIG is internal to the sidetone module only. Then the
McBSP module has it's own SYSCONFIG register that's internal to the
McBSP module only.
I think the confusion comes from the McBSPi_ICLK naming, that's not
internal to the module(s) in question, it comes from an external
shared source that the SYSCONFIG registers control.
Some SYSCONFIG registers have autoidle features that signal the
source clockdomain too.
> I'm still not convinced about the benefits of creating separate device for the
> ST core of McBSP.
> From my point of view:
> McBSP2 module consist of:
> - McBSP core
> - clock generator subcore
> - tx subcore
> - rx subcore
> - Sidetone core
I think it's more like this for the clocking and
intermodule lines:
clockdomain
clock generarator ---+
subcore |
+- McBSP
| internal gating
| and signaling to
| clockdomain via
| SYSCONFIG register
| | |
| | | intermodule lines
| | | not on the interconnect
| | |
+- sidetone
| internal gating
| (and signaling to
| clockdomain via
| SYSCONFIG register?)
Then all these modules just sit on the L4 interconnet at
separate targets, including the clockdomain.
Regards,
Tony
On 04/15/2016 06:16 PM, Tony Lindgren wrote:
>> The hwmod checks the bits described by prcm.omap2. If two hwmods are set up to
>> manage/monitor the same bits in PRCM, what will happen when the two driver
>> does pm_runtime?
>>
>> CM_ICLKEN_PER[0] = 1
>> McBSP2: runtime_get_sync()
>> CM_ICLKEN_PER[0] = 0
>> ...
>> CM_ICLKEN_PER[0] = 0
>> McBSP2.ST: runtime_get_sync() // hwmod might complain as the idlest was not 1?
>> CM_ICLKEN_PER[0] = 0
>> ...
>> CM_ICLKEN_PER[0] = 0
>> McBSP2.ST: runtime_put_sync()
>> CM_ICLKEN_PER[0] = 0 // hwmod might warn that the module did not went idle?
>> ...
>> CM_ICLKEN_PER[0] = 0
>> McBSP2: runtime_put_sync()
>> CM_ICLKEN_PER[0] = 1
>>
>> We can hack this around by adding HWMOD_NO_IDLEST to the sidetone hwmod I
>> guess. As the sidetone does not have PRCM level control - it is part of McBSP.
>
> Heh if they are using the same register bits for two separate modules,
> then that's a bug for sure :) I think the sidetone module only has the
> clock gating bit in the ST_SYSCONFIG.
Yes, the sidetone only has clock gating bit in ST_SYSCONFIG, but the hwmod has
the prcm section which is identical of the corresponding McBSP hwmod prcm section.
Since we have only one MCBSP2_ICLK and only one bit in PRCM registers for it,
this is a bug in the hwmod data for sure. Only the mcbsp hwmod should have
prcm section and the sidetone hwmod is not needed IMO:
It is a bug to have sidetone enabled when McBSP is not enabled and configured
properly. The sidetone can not work w/o proper McBSP configuration.
If we were to keep both hwmods and add new set of pm_runtime calls for the
mcbsp.sidetone, it will only increase/decrease the mcbsp_iclk enable count. It
must never enable the clock itself since that is a bug in the SW.
>>> Then there are two separate sets of sysconfig registers that PM runtime should manage.
>>
>> The sidetone core's sysconfig register is internal to McBSP module. This is
>> what the TRM has to say about 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.
>
> Some confusion here.. The McBSPi_ICLK is external, it's just shared
> between the McBSP and sidetone modules. So the ST_SYSCONFIG gates
> internally separately to the sidetone.
I think it is called McBSPi_ICLK internally also, but true. The same clock
from PRCM goes to McBSPi core and McBSPi.sidetone core. The only difference is
that the sidetone is only able to gate the clock internally.
>> 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 ST_SYSCONFIG_REG is internal to the McBSP module the ST is integrated into.
>
> The ST_SYSCONFIG is internal to the sidetone module only. Then the
> McBSP module has it's own SYSCONFIG register that's internal to the
> McBSP module only.
And the McBSP core can signal (ack) the PRCM back, but the sidetone core can not.
> I think the confusion comes from the McBSPi_ICLK naming, that's not
> internal to the module(s) in question, it comes from an external
> shared source that the SYSCONFIG registers control.
>
> Some SYSCONFIG registers have autoidle features that signal the
> source clockdomain too.
>
>> I'm still not convinced about the benefits of creating separate device for the
>> ST core of McBSP.
>> From my point of view:
>> McBSP2 module consist of:
>> - McBSP core
>> - clock generator subcore
>> - tx subcore
>> - rx subcore
>> - Sidetone core
>
> I think it's more like this for the clocking and
> intermodule lines:
>
> clockdomain
> clock generarator ---+
> subcore |
> +- McBSP
> | internal gating
> | and signaling to
> | clockdomain via
> | SYSCONFIG register
> | | |
> | | | intermodule lines
> | | | not on the interconnect
> | | |
> +- sidetone
> | internal gating
> | (and signaling to
> | clockdomain via
> | SYSCONFIG register?)
>
> Then all these modules just sit on the L4 interconnet at
> separate targets, including the clockdomain.
The McBSPi core and it's sidetone is in the same clock domain as the sidetone
is using the McBSPi interface clock. It is kind of a leech ;)
It is more like:
clockdomain
clock generator ---+
subcore |
+--+--McBSP2
| |
| |
| McBSP2_sidetone
--
P?ter
* Peter Ujfalusi <[email protected]> [160415 12:52]:
> On 04/15/2016 06:16 PM, Tony Lindgren wrote:
> >> We can hack this around by adding HWMOD_NO_IDLEST to the sidetone hwmod I
> >> guess. As the sidetone does not have PRCM level control - it is part of McBSP.
> >
> > Heh if they are using the same register bits for two separate modules,
> > then that's a bug for sure :) I think the sidetone module only has the
> > clock gating bit in the ST_SYSCONFIG.
>
> Yes, the sidetone only has clock gating bit in ST_SYSCONFIG, but the hwmod has
> the prcm section which is identical of the corresponding McBSP hwmod prcm section.
>
> Since we have only one MCBSP2_ICLK and only one bit in PRCM registers for it,
> this is a bug in the hwmod data for sure. Only the mcbsp hwmod should have
> prcm section and the sidetone hwmod is not needed IMO:
> It is a bug to have sidetone enabled when McBSP is not enabled and configured
> properly. The sidetone can not work w/o proper McBSP configuration.
>
> If we were to keep both hwmods and add new set of pm_runtime calls for the
> mcbsp.sidetone, it will only increase/decrease the mcbsp_iclk enable count. It
> must never enable the clock itself since that is a bug in the SW.
OK makes sense. I'd prefer to keep it to match the hardware for the modules.
> > Then all these modules just sit on the L4 interconnet at
> > separate targets, including the clockdomain.
>
> The McBSPi core and it's sidetone is in the same clock domain as the sidetone
> is using the McBSPi interface clock. It is kind of a leech ;)
Well they still are able to use the McBSP interface clock independently
AFAIK :)
Tony
On 04/19/2016 02:51 AM, Tony Lindgren wrote:
>>> Then all these modules just sit on the L4 interconnet at
>>> separate targets, including the clockdomain.
>>
>> The McBSPi core and it's sidetone is in the same clock domain as the sidetone
>> is using the McBSPi interface clock. It is kind of a leech ;)
>
> Well they still are able to use the McBSP interface clock independently
> AFAIK :)
After trying to hunt down documentation (which I'm still in the process):
Initially the sidetone was designed to be built inside of the MCBSPLP ip found
in OMAP3, but at some point it has been decided to attach the sidetone from
the outside and make wire up the connection like it is ended up in OMAP3.
I have not found reasoning, but if I can guess they wanted to avoid three
different McBSPLP modules in OMAP3:
type1: McBSP1,4,5 - MCBSPLP with 128 FIFO
type2: McBSP2 - MCBSPLP + 1024+256 FIFO + sidetone
type3: McBSP3 - MCBSPLP + 128 FIFO + sidetone
Also the sidetone is never used again in OMAP4/5 so it made the integration
and errata (if any) fixes easier for the upcoming SoCs.
But it is just a guess.
>From the documents it is also clear that McBSPLP.sidetone is using the
McBSPLP's ICLK, but what is not explained in the TRM is that there are
internal clocks going from McBSP to sidetone for the data bus between them.
The iclk is needed so the core can kind of run independently from the clocks
coming from McBSPLP (for data exchange between the two modules).
If McBSP is not configured these clocks are not running which renders the
sidetone non operational.
I can send a cut down series to fix the current sidetone hwmod (main_clk and
prevent it to look at the PRCM bit) plus reworking the pdata callback so we
can support both legacy and DT boot.
--
P?ter
* Peter Ujfalusi <[email protected]> [160422 06:15]:
>
> From the documents it is also clear that McBSPLP.sidetone is using the
> McBSPLP's ICLK, but what is not explained in the TRM is that there are
> internal clocks going from McBSP to sidetone for the data bus between them.
> The iclk is needed so the core can kind of run independently from the clocks
> coming from McBSPLP (for data exchange between the two modules).
> If McBSP is not configured these clocks are not running which renders the
> sidetone non operational.
THe McBSP ick is not coming from McBSP, it's coming from the L4
interconnect. Both McBSP are just consumers for that same clock.
AFAIK there is no clock line going from McBSP to the sidetone.
> I can send a cut down series to fix the current sidetone hwmod (main_clk and
> prevent it to look at the PRCM bit) plus reworking the pdata callback so we
> can support both legacy and DT boot.
OK sounds good to me :)
Tony