Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1380215 > unrolled thread

Re: [PATCH v2 0/3] ARM: OMAP3: Fix McBSP2/3 hwmod setup for sidetone

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 0/3] ARM: OMAP3: Fix McBSP2/3 hwmod setup for sidetone Peter Ujfalusi <peter.ujfalusi@ti.com> - 2016-04-15 22:00 +0200
    Re: [PATCH v2 0/3] ARM: OMAP3: Fix McBSP2/3 hwmod setup for sidetone Tony Lindgren <tony@atomide.com> - 2016-04-19 02:00 +0200
      Re: [PATCH v2 0/3] ARM: OMAP3: Fix McBSP2/3 hwmod setup for sidetone Peter Ujfalusi <peter.ujfalusi@ti.com> - 2016-04-22 15:20 +0200
        Re: [PATCH v2 0/3] ARM: OMAP3: Fix McBSP2/3 hwmod setup for sidetone Tony Lindgren <tony@atomide.com> - 2016-04-23 00:30 +0200

#1380215 — Re: [PATCH v2 0/3] ARM: OMAP3: Fix McBSP2/3 hwmod setup for sidetone

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

[toc] | [next] | [standalone]

#1382106

* Peter Ujfalusi <peter.ujfalusi@ti.com> [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

[toc] | [prev] | [next] | [standalone]

#1385157

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

[toc] | [prev] | [next] | [standalone]

#1385508

* Peter Ujfalusi <peter.ujfalusi@ti.com> [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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web