[U-Boot] [PATCH V2 3/5] ARM: OMAP5: clocks: Add OPP settings required for OMAP543X ES2.0 soc

[R Sricharan]

On Tuesday 05 February 2013 08:59 PM, Nishanth Menon wrote:
[..]
>>>>
>>     Answering all of your above questions here.
>>     The above data is for OMAP4 and not OMAP5. This file was modified
>>     here just to include dummy dividers. Because we were now using a
>>     common dpll_params structure, there was no functional change in this
>>     file. I think the problem was commit log was missing this info.
>>      I will update to clarify this.
> I dont think updating commit log alone is the right solution -> renaming the
> variables make more sense - making ones meant for OMAP4 as _omap4430, 60
> etc help readability of code.
  omap4/hw_data.c has separate arrays to represent 4430, 4460 dplls data.

> [...]
>>>>   /* ABE M & N values with 32K clock as source */
>>>>   static const struct dpll_params abe_dpll_params_32k_196608khz = {
>>> We do not intend to support 32K ABE source except when doing DPLL
>>> cascading - so this is in effect an configuration which is
>>> un-used in any s/w line.
>>
>>    1) In fact we not locking ABE, IVA, USB DPLLs any more in bootloader
>>       unless some one enable a CLOCKS_ENABLE_ALL config.
>>       But we still have there data, so that we can quickly experiment
>>       when needed.
> it is no longer supported on OMAP5. If you have doubts on the silicon
> usecase specification, I recommend you do read the TI internal
> documentation for the same.
  >>
>>    2) I remember that locking with 32K source was considered better
>>       because, we can cut sys_clk when audio is still running.
>>       Also i think that the jitter was not a real problem for audio
>>       even when running with 32Khz clock. I have to check this point
>>       again.
> please check again.
>>
   I even checked the latest version of ES2.0 TRM.
   M, N specified are 750, 0. So these are ones used to lock at 32KHZ.

>>       The reason we had both the sources supported was just from an
>>       experiment motive. I have seen that during pre silicon, we have
> As a rule, experimentation code should NOT be in mainline. only
> recommended operation code should ever be put in mainline - real
> products are supposed to ramp with this code.
>
   Yes, by default we have set it to OPP_NOM.
   There where many situations where there was a need to boot with
   higher OPPs for performance measurements with mainline.
   So it was better to change just one variable than to add the full
   data for that OPP in those cases. This has helped many times in the
   past.

>>       to switch the clock source from default to get locking. So having
>>       support for both helps in those cases.
> not if the silicon is neither validated NOR expected to be supported at
> this.
> [...]
>>>>   static const struct dpll_params mpu_dpll_params_1_5ghz[NUM_SYS_CLKS] = {
>>> I am not sure we'd like to do this.
>>      This file is common for ES1.0 as well. Also i have added frequency
>>      tables for OPP_HIGH and OPP_LOW as well. We some times find it
>>      nessecary to boot with a different than OPP_NOM to do experiments.
>>      So this is useful from that point.  But what is missing is the VDDs
>>      which I usually add manually while testing. But now to complete
>>     this, i will add that as well.
> Why? Es1.0 will never be publically released, it never was meant to be
> public. ES2.0 is the only one meant to go out. Supporting ES1.0 and
> ES2.0 means a ton of redundant code being maintained in mainline with no
> product lines meant to actively using it - ever! Do we really want to do
> this??
>
  Ok, i will clean up ES1.0 data in a separate patch series after this.
>>
>>>> -	{125, 0, 1, -1, -1, -1, -1, -1, -1, -1},	/* 12 MHz   */
>>>> -	{-1, -1, -1, -1, -1, -1, -1, -1, -1, -1},	/* 13 MHz   */
>>>> -	{625, 6, 1, -1, -1, -1, -1, -1, -1, -1},	/* 16.8 MHz */
>>>> -	{625, 7, 1, -1, -1, -1, -1, -1, -1, -1},	/* 19.2 MHz */
>>>> -	{750, 12, 1, -1, -1, -1, -1, -1, -1, -1},	/* 26 MHz   */
>>>> -	{-1, -1, -1, -1, -1, -1, -1, -1, -1, -1},	/* 27 MHz   */
>>>> -	{625, 15, 1, -1, -1, -1, -1, -1, -1, -1}	/* 38.4 MHz */
>>>> +	{125, 0, 1, -1, -1, -1, -1, -1, -1, -1, -1, -1},	/* 12 MHz   */
>>>> +	{-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1},	/* 13 MHz   */
>>>> +	{625, 6, 1, -1, -1, -1, -1, -1, -1, -1, -1, -1},	/* 16.8 MHz */
>>>> +	{625, 7, 1, -1, -1, -1, -1, -1, -1, -1, -1, -1},	/* 19.2 MHz */
>>>> +	{750, 12, 1, -1, -1, -1, -1, -1, -1, -1, -1, -1},	/* 26 MHz   */
>>>> +	{-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1},	/* 27 MHz   */
>>>> +	{625, 15, 1, -1, -1, -1, -1, -1, -1, -1, -1, -1}	/* 38.4 MHz */
>>>>   };
>>>>
>>>>   static const struct dpll_params mpu_dpll_params_2ghz[NUM_SYS_CLKS] = {
>>> again, frequency is wrong for es2.0?
>>     This file is common for ES1.0 as well.
> ES1.0 DOES NOT run at 2GHz!
>>
  Ok, i will clean up ES1.0 data separately.

>>>> -	{500, 2, 1, -1, -1, -1, -1, -1, -1, -1},	/* 12 MHz   */
>>>> -	{-1, -1, -1, -1, -1, -1, -1, -1, -1, -1},	/* 13 MHz   */
>>>> -	{2024, 16, 1, -1, -1, -1, -1, -1, -1, -1},	/* 16.8 MHz */
>>>> -	{625, 5, 1, -1, -1, -1, -1, -1, -1, -1},	/* 19.2 MHz */
>>>> -	{1000, 12, 1, -1, -1, -1, -1, -1, -1, -1},	/* 26 MHz   */
>>>> -	{-1, -1, -1, -1, -1, -1, -1, -1, -1, -1},	/* 27 MHz   */
>>>> -	{625, 11, 1, -1, -1, -1, -1, -1, -1, -1}	/* 38.4 MHz */
>>>> +	{500, 2, 1, -1, -1, -1, -1, -1, -1, -1, -1, -1},	/* 12 MHz   */
>>>> +	{-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1},	/* 13 MHz   */
>>>> +	{2024, 16, 1, -1, -1, -1, -1, -1, -1, -1, -1, -1},	/* 16.8 MHz */
>>>> +	{625, 5, 1, -1, -1, -1, -1, -1, -1, -1, -1, -1},	/* 19.2 MHz */
>>>> +	{1000, 12, 1, -1, -1, -1, -1, -1, -1, -1, -1, -1},	/* 26 MHz   */
>>>> +	{-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1},	/* 27 MHz   */
>>>> +	{625, 11, 1, -1, -1, -1, -1, -1, -1, -1, -1, -1}	/* 38.4 MHz */
>>>>   };
>>>>
>>>>   static const struct dpll_params mpu_dpll_params_1100mhz[NUM_SYS_CLKS] = {
>>> again, frequency is wrong for es2.0?
>>     MPU runs at 1100 MHZ in OPP_NOM on 2.0
> 1099MHz - I suggest latest spec to be clear on this - i suggest
> reviewing further based on update.
  MPU runs at 1099.9 MHZ

> [...]
>>>> +	.iva = iva_dpll_params_2330mhz_es2,
>>>> +#ifdef CONFIG_SYS_OMAP_ABE_SYSCK
>>>> +	.abe = abe_dpll_params_sysclk_196608khz,
>>>> +#else
>>>> +	.abe = &abe_dpll_params_32k_196608khz,
>>>> +#endif
>>> I strongly suggest dumping ABE sourcing from 32K clock - the code is
>>> just a nuisance waiting to exercise an unsupported silicon feature
>>> waiting to happen.
> [...]
>>>>   	default:
>>>> diff --git a/arch/arm/include/asm/arch-omap5/clocks.h b/arch/arm/include/asm/arch-omap5/clocks.h
>>>> index 15362ae..685aad5 100644
>>>> --- a/arch/arm/include/asm/arch-omap5/clocks.h
>>>> +++ b/arch/arm/include/asm/arch-omap5/clocks.h
>>>> @@ -208,6 +208,10 @@
>>>>   #define VDD_MM_5432	1150
>>>>   #define VDD_CORE_5432	1150
>>>>
>>>> +#define VDD_MPU_ES2	1060
>>>> +#define VDD_MM_ES2	1025
>>>> +#define VDD_CORE_ES2	1040
>>> ^^^ these are meant only for OPP_NOM - considering that you have DPLLs
>>> for OPP_SB added in, do you even expect these devices to bootup at
>>> anything *but* OPP_NOM??
>>    I agree. I add it manually while doing experiments. I will add these
>>    here.
> As a rule we want maintainable s/w in mainline, not experimental code.
> maintainable s/w implies s/w that adheres to SoC's specified operational
> ranges unfortunately :(.
>

Regards,
  Sricharan