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

Groups > linux.kernel > #1370600 > 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-04 14:50 +0200
    Re: [PATCH v2 0/3] ARM: OMAP3: Fix McBSP2/3 hwmod setup for sidetone Tony Lindgren <tony@atomide.com> - 2016-04-04 17:20 +0200
      Re: [PATCH v2 0/3] ARM: OMAP3: Fix McBSP2/3 hwmod setup for sidetone Peter Ujfalusi <peter.ujfalusi@ti.com> - 2016-04-05 15:20 +0200
        Re: [PATCH v2 0/3] ARM: OMAP3: Fix McBSP2/3 hwmod setup for sidetone Tony Lindgren <tony@atomide.com> - 2016-04-11 23:30 +0200
          Re: [PATCH v2 0/3] ARM: OMAP3: Fix McBSP2/3 hwmod setup for sidetone Peter Ujfalusi <peter.ujfalusi@ti.com> - 2016-04-12 12:00 +0200
            Re: [PATCH v2 0/3] ARM: OMAP3: Fix McBSP2/3 hwmod setup for sidetone Tony Lindgren <tony@atomide.com> - 2016-04-12 18:40 +0200
              Re: [PATCH v2 0/3] ARM: OMAP3: Fix McBSP2/3 hwmod setup for sidetone Peter Ujfalusi <peter.ujfalusi@ti.com> - 2016-04-13 14:00 +0200
                Re: [PATCH v2 0/3] ARM: OMAP3: Fix McBSP2/3 hwmod setup for sidetone Tony Lindgren <tony@atomide.com> - 2016-04-13 17:30 +0200
                  Re: [PATCH v2 0/3] ARM: OMAP3: Fix McBSP2/3 hwmod setup for sidetone Peter Ujfalusi <peter.ujfalusi@ti.com> - 2016-04-14 09:40 +0200
                    Re: [PATCH v2 0/3] ARM: OMAP3: Fix McBSP2/3 hwmod setup for sidetone Tony Lindgren <tony@atomide.com> - 2016-04-14 19:00 +0200
                      Re: [PATCH v2 0/3] ARM: OMAP3: Fix McBSP2/3 hwmod setup for sidetone Peter Ujfalusi <peter.ujfalusi@ti.com> - 2016-04-14 21:40 +0200
                        Re: [PATCH v2 0/3] ARM: OMAP3: Fix McBSP2/3 hwmod setup for sidetone Tony Lindgren <tony@atomide.com> - 2016-04-14 22:40 +0200
                          Re: [PATCH v2 0/3] ARM: OMAP3: Fix McBSP2/3 hwmod setup for sidetone Peter Ujfalusi <peter.ujfalusi@ti.com> - 2016-04-15 12:30 +0200
                            Re: [PATCH v2 0/3] ARM: OMAP3: Fix McBSP2/3 hwmod setup for sidetone Tony Lindgren <tony@atomide.com> - 2016-04-15 17:20 +0200

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

Tony,

On 04/02/16 03:17, Tony Lindgren wrote:
> Hi,
> 
> * Peter Ujfalusi <peter.ujfalusi@ti.com> [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

[toc] | [next] | [standalone]

#1370685

* Peter Ujfalusi <peter.ujfalusi@ti.com> [160404 05:47]:
> On 04/02/16 03:17, Tony Lindgren wrote:
> > * Peter Ujfalusi <peter.ujfalusi@ti.com> [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

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

#1371564

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

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

#1376292

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

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

#1376614

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

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

#1377007

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

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

#1377853

On 04/12/16 19:37, Tony Lindgren wrote:
> * Peter Ujfalusi <peter.ujfalusi@ti.com> [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

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

#1378047

* Peter Ujfalusi <peter.ujfalusi@ti.com> [160413 04:59]:
> On 04/12/16 19:37, Tony Lindgren wrote:
> > * Peter Ujfalusi <peter.ujfalusi@ti.com> [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

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

#1378551

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

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

#1379104

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

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

#1379265

On 04/14/2016 07:55 PM, Tony Lindgren wrote:
> * Peter Ujfalusi <peter.ujfalusi@ti.com> [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

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

#1379296

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

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

#1379671

On 04/14/16 23:34, Tony Lindgren wrote:
> * Peter Ujfalusi <peter.ujfalusi@ti.com> [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

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

#1379907

* Peter Ujfalusi <peter.ujfalusi@ti.com> [160415 03:24]:
> On 04/14/16 23:34, Tony Lindgren wrote:
> > * Peter Ujfalusi <peter.ujfalusi@ti.com> [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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web