[U-Boot] [PATCH v3 04/12] arm: socfpga: clkmgr: Separate the Clock Manager for Stratix 10

[Marek Vasut]

On 10/18/2016 05:22 AM, Chin Liang See wrote:
> On Sen, 2016-10-17 at 18:14 +0200, Marek Vasut wrote:
>> On 10/17/2016 05:59 PM, Chin Liang See wrote:
>>>
>>> On Sen, 2016-10-17 at 17:39 +0200, Marek Vasut wrote:
>>>>
>>>> On 10/17/2016 05:28 PM, Chin Liang See wrote:
>>>>>
>>>>>
>>>>> On Sen, 2016-10-17 at 17:20 +0200, Marek Vasut wrote:
>>>>>>
>>>>>>
>>>>>> On 10/17/2016 05:07 PM, Chin Liang See wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Sen, 2016-10-17 at 15:42 +0200, Marek Vasut wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 10/17/2016 03:32 PM, See, Chin Liang wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Min, 2016-10-16 at 17:33 +0200, Marek Vasut wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 10/13/2016 10:33 AM, Chin Liang See wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Separate the Clock Manager to support both GEN5 SoC
>>>>>>>>>>> and
>>>>>>>>>>> Stratix 10 SoC.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Chin Liang See <clsee at altera.com>
>>>>>>>>>>> Cc: Marek Vasut <marex at denx.de>
>>>>>>>>>>> Cc: Dinh Nguyen <dinguyen at opensource.altera.com>
>>>>>>>>>>> Cc: Ley Foon Tan <lftan at altera.com>
>>>>>>>>>>> Cc: Tien Fong Chee <tfchee at altera.com>
>>>>>>>>>>> ---
>>>>>>>>>>>  arch/arm/mach-socfpga/clock_manager.c | 8 ++++++++
>>>>>>>>>>>  1 file changed, 8 insertions(+)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/arch/arm/mach-socfpga/clock_manager.c
>>>>>>>>>>> b/arch/arm/mach-
>>>>>>>>>>> socfpga/clock_manager.c
>>>>>>>>>>> index aa71636..0d67b3c 100644
>>>>>>>>>>> --- a/arch/arm/mach-socfpga/clock_manager.c
>>>>>>>>>>> +++ b/arch/arm/mach-socfpga/clock_manager.c
>>>>>>>>>>> @@ -10,6 +10,7 @@
>>>>>>>>>>>
>>>>>>>>>>>  DECLARE_GLOBAL_DATA_PTR;
>>>>>>>>>>>
>>>>>>>>>>> +#if defined(CONFIG_TARGET_SOCFPGA_GEN5)
>>>>>>>>>>>  static const struct socfpga_clock_manager
>>>>>>>>>>> *clock_manager_base
>>>>>>>>>>> =
>>>>>>>>>>>       (struct socfpga_clock_manager
>>>>>>>>>>> *)SOCFPGA_CLKMGR_ADDRESS;
>>>>>>>>>>>
>>>>>>>>>>> @@ -446,9 +447,11 @@ unsigned int
>>>>>>>>>>> cm_get_l4_sp_clk_hz(void)
>>>>>>>>>>>
>>>>>>>>>>>       return clock;
>>>>>>>>>>>  }
>>>>>>>>>>> +#endif /* CONFIG_TARGET_SOCFPGA_GEN5 */
>>>>>>>>>>>
>>>>>>>>>>>  unsigned int cm_get_mmc_controller_clk_hz(void)
>>>>>>>>>>>  {
>>>>>>>>>>> +#if defined(CONFIG_TARGET_SOCFPGA_GEN5)
>>>>>>>>>>>       uint32_t reg, clock = 0;
>>>>>>>>>>>
>>>>>>>>>>>       /* identify the source of MMC clock */
>>>>>>>>>>> @@ -475,8 +478,12 @@ unsigned int
>>>>>>>>>>> cm_get_mmc_controller_clk_hz(void)
>>>>>>>>>>>       /* further divide by 4 as we have fixed
>>>>>>>>>>> divider
>>>>>>>>>>> at
>>>>>>>>>>> wrapper */
>>>>>>>>>>>       clock /= 4;
>>>>>>>>>>>       return clock;
>>>>>>>>>>> +#elif defined(CONFIG_TARGET_SOCFPGA_STRATIX10)
>>>>>>>>>>> +     return 25000000;
>>>>>>>>>> Is this always gonna be the case or is this S10VP
>>>>>>>>>> specific ?
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> +#endif /* CONFIG_TARGET_SOCFPGA_GEN5 */
>>>>>>>>>>>  }
>>>>>>>>>>>
>>>>>>>>>>> +#if defined(CONFIG_TARGET_SOCFPGA_GEN5)
>>>>>>>>>>>  unsigned int cm_get_qspi_controller_clk_hz(void)
>>>>>>>>>>>  {
>>>>>>>>>>>       uint32_t reg, clock = 0;
>>>>>>>>>>> @@ -556,3 +563,4 @@ U_BOOT_CMD(
>>>>>>>>>>>       "display clocks",
>>>>>>>>>>>       ""
>>>>>>>>>> Why does the clock display not work on S10 ? Are some
>>>>>>>>>> functions
>>>>>>>>>> missing?
>>>>>>>>> Not for SOCVP. But will be added in later stage when
>>>>>>>>> testing
>>>>>>>>> against
>>>>>>>>> emulation
>>>>>>>> How hard would it be to add this missing functionality
>>>>>>>> now ?
>>>>>>>>
>>>>>>> That will take weeks as that need to be validated as whole
>>>>>>> in
>>>>>>> emulation
>>>>>>> platform.
>>>>>> You mean printing a few clock information based on some
>>>>>> values
>>>>>> from
>>>>>> registers would take weeks ? Why ?
>>>>>>
>>>>> Oh actually I am referring all the managers code such as full
>>>>> Clock
>>>>> Manager, Reset Manager ... plus testing. Testing is the part
>>>>> take
>>>>> some
>>>>> significant time especially slow when come to emulation.
>>>> Just use empty functions for the clock init code (since it's not
>>>> needed
>>>> on the socvp) and populate the clock reporting functions. That
>>>> should
>>>> be
>>>> simple, right ?
>>> Can be done but the value won't be meaningful as the register is
>>> uninitialzied. Unless we hardcode to a hard value which might not
>>> sound
>>> right.
>> Ha, I see. Is there some sane default for the SoCVP ?
>>
> 
> It will not meaningful as performance is same even with frequency
> change :) But definitely will address this in coming weeks as I am
> already working on the emulation. 

I understand that, but I believe it is still better to provide some
sensible value rather than just ifdef the whole code out.

-- 
Best regards,
Marek Vasut