Re: [PATCH 01/11 RESEND] ARM: OMAP: DRA7: hwmod: Add data for McASP3

[Peter Ujfalusi <peter.ujfalusi@ti.com>]

On 10/02/2015 02:55 PM, Peter Ujfalusi wrote:
> On 10/02/2015 02:22 PM, Peter Ujfalusi wrote:
>> Paul,
>>
>> On 10/02/2015 12:07 PM, Paul Walmsley wrote:
>>> Hello Péter,
>>>
>>> On Wed, 30 Sep 2015, Peter Ujfalusi wrote:
>>>
>>>> On 09/27/2015 10:02 AM, Paul Walmsley wrote:
>>>>>>  /*
>>>>>> + * 'mcasp' class
>>>>>> + *
>>>>>> + */
>>>>>> +static struct omap_hwmod_class_sysconfig dra7xx_mcasp_sysc = {
>>>>>> +	.sysc_offs	= 0x0004,
>>>>>> +	.sysc_flags	= SYSC_HAS_SIDLEMODE,
>>>>>> +	.idlemodes	= (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART),
>>>>>> +	.sysc_fields	= &omap_hwmod_sysc_type3,
>>>>>> +};
>>>>>> +
>>>>>> +static struct omap_hwmod_class dra7xx_mcasp_hwmod_class = {
>>>>>> +	.name	= "mcasp",
>>>>>> +	.sysc	= &dra7xx_mcasp_sysc,
>>>>>> +};
>>>>>> +
>>>>>> +/* mcasp3 */
>>>>>> +static struct omap_hwmod dra7xx_mcasp3_hwmod = {
>>>>>> +	.name		= "mcasp3",
>>>>>> +	.class		= &dra7xx_mcasp_hwmod_class,
>>>>>> +	.clkdm_name	= "l4per2_clkdm",
>>>>>> +	.main_clk	= "mcasp3_ahclkx_mux",
>>>>>
>>>>> I'd expect this clock to be something derived from mcasp3_aux_gfclk, 
>>>>> according to Table 24-408 "Clocks and Resets" of SPRUHZ6.  Could you 
>>>>> please doublecheck this?
>>>>
>>>> I can not explain this. If I change the main_clk to "mcasp3_aux_gfclk_mux"
>>>> then I can not access to McASP3 register at all.
>>>> I don't see anything popping out in the clock data, nor in other places.
>>>
>>> OK thank you for testing that.  Maybe just add a brief comment along those 
>>> lines in the hwmod data, above the structure record declaration?
>>
>> Now I more or less figured out the root of the issue. According to the
>> documentations the McASP in dra7xx family has following clocks:
>> ICLK - interface clock
>> GFCLK - functional clock
>> AHCLKX - Transmit high-frequency master clock
>> AHCLKR - Receive high-frequency master clock (on selected instances)
>>
>> In order to access registers all of these clock lines must have valid clock
>> signal. No other SoC with McASP has this setup.
>> As for why things seams to work when mcasp3_ahclkx_mux is set as main_clk and
>> mcasp3_aux_gfclk_mux is not handled at all?
>> The mcasp3_aux_gfclk_mux by default is from PER_ABE_X1GFCLK we never change
>> this. On dra7/72 evm the AHCLKX is reparented to ATL2 clock.
>> When with pm_runtime we enable the device, the mcasp3_ahclkx_mux will be
>> enabled by the SW (and ATL as well) _and_ the mcasp3_aux_gfclk_mux path will
>> be enabled by HW w/o SW.
>> The reason why I have had crash when I switched the main_clk to
>> mcasp3_aux_gfclk_mux is that even the path for the mcasp3_ahclkx_mux is
>> enabled by the HW, the ATL itself was not enabled, so it was not generating
>> the needed clocks. Same goes backwards for the gfclk: if I reparent it to
>> let's say HDMI clock - which is not present, and handle the ahclkx clock I
>> have similar crash.
>> All in all: the McASP3 needs ICLK and both FCLK and AHCLKX to be running in
>> order to be able to access registers.
>>
>> This current setup works fine, the only issue I see is that the refcounts for
>> the mcasp3_aux_gfclk_mux path is not reflecting the reality.
>>
>> With Tero we looked at different angles of this and how to solve it w/o
>> considering it as a hack.
>> Either we go with the hwmod data with main_clk set to mcasp3_ahclkx_mux and
>> document it in a comment, or:
>> Add new flag HWMOD_OPT_CLKS_NEEDED to hwmods to use to tell that the optional
>> clocks are not really optional as they are needed to be enabled in order to
>> access to the IP. In omap_hwmod.c's _enable_clocks() and _disable_clocks() we
>> call _enable_optional_clocks()/_disable_optional_clocks() if the flag is set
>> for the hwmod and add the ahclkx_mux as optional clock for McASP3.
> 
> This might be awkward to say that the optional clocks are not optional,
> probably would be better to add:
> struct omap_hwmod {
> 	...
> 	struct omap_hwmod_opt_clk	*needed_clks;
> 	...
> 	u8				needed_clks_cnt;
> 	...
> };
> 
> and use the needed_clks in _init_main_clk()/_enable_clocks()/_disable_clocks() ?

Arrgh, but how to deal with this via DT bindings? It will be better to mark
the optional clocks as needed.

-- 
Péter