[U-Boot] [PATCH 07/20] arm: socfpga: add clock driver for Arria 10

[Marek Vasut <marex@denx.de>]

On 03/06/2017 08:10 AM, Ley Foon Tan wrote:
> On Sab, 2017-02-25 at 22:35 +0100, Marek Vasut wrote:
>> On 02/22/2017 10:47 AM, Ley Foon Tan wrote:
>>>
>>> Add clock driver support for Arria 10 and update Gen5 clock driver.
>>>
>>> Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
>>> Signed-off-by: Ley Foon Tan <ley.foon.tan@intel.com>
>>> ---

[...]

>>> diff --git a/arch/arm/mach-socfpga/clock_manager_arria10.c
>>> b/arch/arm/mach-socfpga/clock_manager_arria10.c
>>> new file mode 100644
>>> index 0000000..4fa841f
>>> --- /dev/null
>>> +++ b/arch/arm/mach-socfpga/clock_manager_arria10.c
>>> @@ -0,0 +1,1104 @@
>>> +/*
>>> + * Copyright (C) 2016-2017 Intel Corporation
>>> + *
>>> + * SPDX-License-Identifier:    GPL-2.0
>>> + */
>>> +
>>> +#include <common.h>
>>> +#include <asm/io.h>
>>> +#include <asm/arch/clock_manager.h>
>>> +#include <fdtdec.h>
>>> +
>>> +DECLARE_GLOBAL_DATA_PTR;
>>> +
>>> +u32 eosc1_hz;
>>> +u32 cb_intosc_hz;
>>> +u32 f2s_free_hz;
>>> +u32 cm_l4_main_clk_hz;
>>> +u32 cm_l4_sp_clk_hz;
>>> +u32 cm_l4_mp_clk_hz;
>>> +u32 cm_l4_sys_free_clk_hz;
>> Shouldn't these values be static ? (if needed at all) ?
> They are used in clock_manager.c clock info printing. But, I can
> restructure that part and keep all these static.

Yeah, either that or use accessors.

>>
>>>
>>> +struct strtopu32 {
>>> +	const char *str;
>>> +	u32 *p;
>>> +};
>> What is this ^ and is this needed ? Probably not ...
> Will remove.
>> [...]

[...]

>>> -static unsigned int cm_get_main_vco_clk_hz(void)
>>> +unsigned int cm_get_main_vco_clk_hz(void)
>>>  {
>>> -	u32 reg, clock;
>>> +	u32 src_hz, numer, denom, vco;
>>>
>>>  	/* get the main VCO clock */
>>> -	reg = readl(&clock_manager_base->main_pll.vco);
>>> -	clock = cm_get_osc_clk_hz(1);
>>> -	clock /= ((reg & CLKMGR_MAINPLLGRP_VCO_DENOM_MASK) >>
>>> -		  CLKMGR_MAINPLLGRP_VCO_DENOM_OFFSET) + 1;
>>> -	clock *= ((reg & CLKMGR_MAINPLLGRP_VCO_NUMER_MASK) >>
>>> -		  CLKMGR_MAINPLLGRP_VCO_NUMER_OFFSET) + 1;
>>> +	vco = readl(&clock_manager_base->main_pll.vco);
>> How many of the changes here are relevant ?
>>
>> The changes to this file are total chaos and this really does need
>> some
>> splitting, it's unreviewable.
> This file mainly to rename of variables and minor fixes on some clock
> calculation.
> I will separate these changes from this patch and move to after "[PATCH
> 01/20] arm: socfpga: restructure clock manager driver".

Good, thanks.

>>
>>>
>>> -	return clock;
>>> +	numer = ((vco & CLKMGR_MAINPLLGRP_VCO_NUMER_MASK) >>
>>> +		CLKMGR_MAINPLLGRP_VCO_NUMER_OFFSET);
>>> +	denom = ((vco & CLKMGR_MAINPLLGRP_VCO_DENOM_MASK) >>
>>> +		 CLKMGR_MAINPLLGRP_VCO_DENOM_OFFSET);
>>> +
>>> +	src_hz = cm_get_osc_clk_hz(1);
>>> +
>>> +	vco = src_hz;
>>> +	vco /= (1 + denom);
>>> +	vco *= (1 + numer);
>>> +
>>> +	return vco;
>>>  }
>> [...]
>>
> Thanks
>
> Regards
> Ley Foon
>