Message-ID: <5703BA6B.1080208@ti.com>
Date:	Tue, 5 Apr 2016 16:15:23 +0300
From:	Peter Ujfalusi <peter.ujfalusi@...com>
To:	Tony Lindgren <tony@...mide.com>
CC:	Paul Walmsley <paul@...an.com>, <jarkko.nikula@...mer.com>,
	<t-kristo@...com>, <linux-omap@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>,
	<linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>
Subject: Re: [PATCH v2 0/3] ARM: OMAP3: Fix McBSP2/3 hwmod setup for sidetone

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

Powered by blists - more mailing lists