[U-Boot] [PATCH V7 1/3] MX5: clock: Add clock config interface

[Stefano Babic <sbabic@denx.de>]

On 05/17/2011 08:25 AM, Jason Liu wrote:
> Hi, Stefano,

Hi Jason,

> I use array here just for the case we will have another element in the array
> (currently we only have one) in the future. Currently, we use 24MHz as ref.

Understood, thanks.

>>>  /*
>>> + * Clock config code start here
>>> + */
>>> +
>>> +/* precondition: m>0 and n>0.  Let g=gcd(m,n). */
>>> +static int gcd(int m, int n)
>>> +{
>>> +     int t;
>>> +     while (m > 0) {
>>> +             if (n > m) {
>>> +                     t = m;
>>> +                     m = n;
>>> +                     n = t;
>>> +             } /* swap */
>>> +             m -= n;
>>> +     }
>>> +     return n;
>>> +}
>>
>> This function has nothing to do with MX5 code. This is a general
>> function, and should be add to lib/. I think you have to remove it from
>> here and put it in a separate patch.
> 
> I don't think it should be put to lib since only the mx5 clock code use it here.

This function computes the GCD of two numbers, and has nothing to do
with MX5 and clock. It is a general function, that others can use.

> OK, I will check linux implementation. Do you know does u-boot also has such
> kind of function already, I did not find it?

No, this function is not yet implemented in u-boot. However, it could be
that there is in some drivers an analog function doing the same. For
this reason and to avoid multiple instances of "quite" same code doing
"quite" the same thing, I suggest to move this function from here from
strictly related IMX code to a general lib. Let's see what Wolfgang
whinks about it.

>>> + *      t_clk = 2*ref_freq*(mfi + mfn/(mfd+1))/(pd+1)
>>
>> Where does this formula come from ?
> 
> The comments is not correct, I will fix it, it should be 4*ref_freq
> not 2 * ref_req.
> The reference manual: 12.2.2.3 DPLL Operation Register, elvis user guide,
> Registers DP_OP, DP_MFN, and DP_MFD calculate the output frequency by
> the formula:
> 
> The chapter number will not the same as you, but you still can find it
> in the reference manual.

Ok. I think it is better to add a comment with the chapter title, as to
set a reference number, because this is changing.

I will check in the manual, thanks for hint.

>> Use defines to set registers. Or read-modify-write, as you changed only
>> PERIPH_APM_SEL from the reset value. However, you reset to 0
>> DDR_CLK_SEL.  I see you switch values back at the end of the function,
>> but my doubt is if it is correct to set in this transition DDR_CLK_SEL
>> always to zero. Can it work with all boards or there are configurations
>> where it does not work ?
> 
> I think it should work since DDR_CLK_SEL:00 derive clock from axi a
> is the default value when boot up.

Then I think it is enough you add as comment your explanation, and
mention that the code does not support if the clock is not derived from
axi-a. If someone adds a custom board with this set-up (and I do not
know if it will be a use case), he is at least warned that must adapt
this function.


>>> + *
>>> + * @param ref   pll input reference clock (24MHz)
>>
>> Reference clock is defined as CONFIG_SYS_MX5_HCLK. Is it really possible
>> to pass a parameter to this function different as CONFIG_SYS_MX5_HCLK ?
>> What happens if CONFIG_SYS_MX5_HCLK and the ref parameter in this
>> function have two different values ? It seems to me this is not a use
>> case, and then we must avoid it. If I am right, you should drop the ref
>> parameter and use CONFIG_SYS_MX5_HCLK.
> 
> As the reference manual tell, the pll can have 4 different clock
> source. The most
> common use case is using 24Mhz OSC as the input.

Understood, thanks.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================