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

[Marek Vasut]

On 10/18/2016 05:34 AM, Chin Liang See wrote:
> On Sel, 2016-10-18 at 06:00 +0200, Marek Vasut wrote:
>> 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.
> 
> I can hardcode some sensible value for that if good.

I think it's better, yes. Thanks

-- 
Best regards,
Marek Vasut