Re: [PATCH 1/6] OMAP4: Add missing clock divider for OCP_ABE_ICLK

[Jon Hunter <jon-hunter@ti.com>]

Hi Paul,

On 7/15/2011 3:21, Paul Walmsley wrote:
> cc'ing Benoît
>
> Hi Jon
>
> On Thu, 14 Jul 2011, Jon Hunter wrote:
>
>> From: Jon Hunter<jon-hunter@ti.com>
>>
>> The parent clock of the OCP_ABE_ICLK is the AESS_FCLK and the
>> parent clock of the AESS_FCLK is the ABE_FCLK...
>>
>> ABE_FCLK -->  AESS_FCLK -->  OCP_ABE_ICLK
>>
>> The AESS_FCLK and OCP_ABE_ICLK clocks both have dividers which
>> determine their operational frequency. However, the dividers for
>> the AESS_FCLK and OCP_ABE_ICLK are controlled via a single bit,
>> which is the CM1_ABE_AESS_CLKCTRL[24] bit.>  When this bit is set to
>> 0, the AESS_FCLK divider is 1 and the OCP_ABE_ICLK divider is 2.
>> Similarly, when this bit is set to 1, the AESS_FCLK divider is 2
>> and the OCP_ABE_ICLK is 1.
>
> Sigh.  This type of hardware design makes software design difficult :-(

Hopefully, because this is a interface clock the impact is really 
minimal...more below...

>> The above relationship between the AESS_FCLK and OCP_ABE_ICLK
>> dividers ensure that the OCP_ABE_ICLK clock is always half the
>> frequency of the ABE_CLK...
>>
>> OCP_ABE_ICLK = ABE_FCLK/2
>>
>> The divider for the OCP_ABE_ICLK is currently missing so add a
>> divider that will ensure the OCP_ABE_ICLK frequency is always half
>> the ABE_FCLK frequency.
>>
>> Signed-off-by: Jon Hunter<jon-hunter@ti.com>
>> ---
>>   arch/arm/mach-omap2/clock44xx_data.c |   16 +++++++++++++++-
>>   1 files changed, 15 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/clock44xx_data.c b/arch/arm/mach-omap2/clock44xx_data.c
>> index 8c96567..6e158ce 100644
>> --- a/arch/arm/mach-omap2/clock44xx_data.c
>> +++ b/arch/arm/mach-omap2/clock44xx_data.c
>> @@ -1301,11 +1301,25 @@ static struct clk mcasp3_fclk = {
>>   	.recalc		=&followparent_recalc,
>>   };
>>
>> +static const struct clksel_rate div2_2to1_rates[] = {
>> +	{ .div = 1, .val = 1, .flags = RATE_IN_4430 },
>> +	{ .div = 2, .val = 0, .flags = RATE_IN_4430 },
>> +	{ .div = 0 },
>> +};
>> +
>> +static const struct clksel ocp_abe_iclk_div[] = {
>> +	{ .parent =&aess_fclk, .rates = div2_2to1_rates },
>> +	{ .parent = NULL },
>> +};
>> +
>>   static struct clk ocp_abe_iclk = {
>>   	.name		= "ocp_abe_iclk",
>>   	.parent		=&aess_fclk,
>> +	.clksel		= ocp_abe_iclk_div,
>> +	.clksel_reg	= OMAP4430_CM1_ABE_AESS_CLKCTRL,
>> +	.clksel_mask	= OMAP4430_CLKSEL_AESS_FCLK_MASK,
>>   	.ops		=&clkops_null,
>> -	.recalc		=&followparent_recalc,
>> +	.recalc		=&omap2_clksel_recalc,
>>   };
>>
>>   static struct clk per_abe_24m_fclk = {
>
> I guess the reason that this patch can get away with this is because it
> doesn't allow software to change the rate of ocp_abe_iclk, and the
> ocp_abe_iclk is a child of aess_fclk, so when aess_fclk is changed, it
> will recalc ocp_abe_iclk.
>
> Benoît, what do you think?  Is this a reasonable approach for the script?
> Or do we need to deal with some kind of 'linked clock' implementation...

If you want my two cents on this matter, I would say that...

1). People should not need to configure the "ocp_abe_iclk" clock 
directly, because regardless of the divider setting it is always 1/2 the 
"abe_fclk". In other words, only the "aess_fclk" frequency is really 
changing because of the divider and the above relationship ensures that 
the "ocp_abe_iclk" is always the same frequency. So a user only cares 
about the "aess_fclk" frequency and the "ocp_abe_iclk" frequency is 
handled for them.

2). The "ocp_abe_iclk" is an interface clock and is not a parent to any 
other functional clock and therefore, is not driving any internal logic 
in a peripheral which would have a direct impact of the speed of that 
peripheral. However, there does appear to be another bug here in the 
clock44xx_data.c as it shows that the "ocp_abe_iclk" is parent to the 
"slimbus1_fck" which I believe is incorrect. According to the TRM, the 
the ocp_abe_iclk is parent to the slimbus1_iclk. I can send another 
patch for this. However, I will let Benoit chime in first.

Cheers
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html