[U-Boot] [PATCH v2 3/4] odroid: make some macros common

[Minkyu Kang]

Dear Hyungwon Hwang,

On 04/11/14 17:29, Lukasz Majewski wrote:
> Hi Hyungwon,
> 
>> On Mon, 03 Nov 2014 09:51:25 +0100
>> Lukasz Majewski <l.majewski at samsung.com> wrote:
>>
>>> Hi Hyungwon,
>>>
>>>> Some macros are used commonly for odroid series boards. This patch
>>>> makes a common header file to congregate that kinds of macros.
>>>> Even though there are more macros which can be common, they are
>>>> not become common. Because they are a part of a register, the
>>>> readability is better when they are defined at a place.
>>>>
>>>> Signed-off-by: Hyungwon Hwang <human.hwang at samsung.com>
>>>> ---
>>>>  board/samsung/odroid/odroid.c | 1 +
>>>>  board/samsung/odroid/setup.h  | 8 --------
>>>>  2 files changed, 1 insertion(+), 8 deletions(-)
>>>>
>>>
>>> I suspect that you have not added the new file to git repository -
>>> since you only removed lines from board/samsung/odroid/setup.h
>>>
>>> I also guess that odroid U3 will not build anymore. That is a very
>>> good use case for buildman script.
>>
>> Oh. It is my mistake. I will include the common header in next
>> version.
>>
>>>
>>>> diff --git a/board/samsung/odroid/odroid.c
>>>> b/board/samsung/odroid/odroid.c index 5edb250..ccbb3a0 100644
>>>> --- a/board/samsung/odroid/odroid.c
>>>> +++ b/board/samsung/odroid/odroid.c
>>>> @@ -18,6 +18,7 @@
>>>>  #include <usb.h>
>>>>  #include <usb/s3c_udc.h>
>>>>  #include <samsung/misc.h>
>>>> +#include "../setup.h"
>>>
>>> Relative path is not a good idea IMHO.
>>>
>>> It would be better to place it at ./include/samsung/ with a self
>>> explanatory name (like exynos4-pll.h or/and exynos4-{other excluded
>>> defines for an IP blocks}).
>>>
>>> In this way other boards could use those defines too.
>>
>> I think that your idea is better than mine.
>>
>>>
>>>>  #include "setup.h"
>>>>  
>>>>  DECLARE_GLOBAL_DATA_PTR;
>>>> diff --git a/board/samsung/odroid/setup.h
>>>> b/board/samsung/odroid/setup.h index 3e48dad..35f7af5 100644
>>>> --- a/board/samsung/odroid/setup.h
>>>> +++ b/board/samsung/odroid/setup.h
>>>> @@ -8,14 +8,6 @@
>>>>  #ifndef __ODROIDU3_SETUP__
>>>>  #define __ODROIDU3_SETUP__
>>>>  
>>>> -/* A/M PLL_CON0 */
>>>> -#define SDIV(x)                 ((x) & 0x7)
>>>> -#define PDIV(x)                 (((x) & 0x3f) << 8)
>>>> -#define MDIV(x)                 (((x) & 0x3ff) << 16)
>>>> -#define FSEL(x)                 (((x) & 0x1) << 27)
>>>> -#define PLL_LOCKED_BIT          (0x1 << 29)
>>>> -#define PLL_ENABLE(x)           (((x) & 0x1) << 31)
>>>> -
>>>
>>> The above data is common for Exynos U3 and XU3, but is it the only
>>> one? Aren't there any more defines to be extracted?
>>>
>>>>  /* CLK_SRC_CPU */
>>>>  #define MUX_APLL_SEL(x)         ((x) & 0x1)
>>>>  #define MUX_CORE_SEL(x)         (((x) & 0x1) << 16)
>>>
>>>
>>>
>>
>> You're right. I found some other common macros more now. I will
>> reflect it in next version. But I have a doubt about MUX_APLL_SEL or
>> something else which consist of a register with different macros in
>> different processors. They can be extracted to common file. But is it
>> good to do it? For example, MUX_APLL_SEL exists both in Exynos4 and
>> Exynos5's CLK_SRC_CPU.
>>
>> Exynos 4412
>> /* CLK_SRC_CPU */
>> #define MUX_APLL_SEL(x)		((x) & 0x1)
>> #define MUX_CORE_SEL(x)		(((x) & 0x1) << 16)
>>
>> Exynos 5800
>> /* CLK_SRC_CPU */
>> #define MUX_APLL_SEL(x)         ((x) & 0x1)
>> #define MUX_CORE_SEL(x)         (((x) & 0x1)
>> #define MUX_HPM_SEL(x)          (((x) & 0x1) << 20)
>> #define MUX_MPLL_USER_SEL_C(x)  (((x) & 0x1) << 24)
> 
> It is always a matter of pragmatism.  In the above case you could only
> extract MUX_APLL_SEL(x). But is it worth to add a separate header file
> for only one line? In my opinion not.
> 
>>
>> If MUX_APLL_SEL and MUX_CORE_SEL are extracted to the common file, it
>> will be harder to see what consist of CLK_SRC_CPU at a glance. Isn't
>> it? This situation is worse in the case of APLL_RATIO. (Please see
>> the below.) I want to hear your opinion about it.
>>
>> Exynos 4412
>> /* CLK_DIV_CPU0 */
>> #define ARM_RATIO(x)           ((x) & 0x7)
>> #define CPUD_RATIO(x)         (((x) & 0x7) << 4)
>> #define ATB_RATIO(x)         (((x) & 0x7) << 16)
>> #define PCLK_DBG_RATIO(x)       (((x) & 0x7) << 20)
>> #define APLL_RATIO(x)           (((x) & 0x7) << 24)
>> #define ARM2_RATIO(x)         (((x) & 0x7) << 28)
>>
>> Exynos 5800
>> /* CLK_DIV_CPU0 */
>> #define CORE_RATIO(x)           ((x) & 0x7)
>> #define COREM0_RATIO(x)         (((x) & 0x7) << 4)
>> #define COREM1_RATIO(x)         (((x) & 0x7) << 8)
>> #define PERIPH_RATIO(x)         (((x) & 0x7) << 12)
>> #define ATB_RATIO(x)            (((x) & 0x7) << 16)
>> #define PCLK_DBG_RATIO(x)       (((x) & 0x7) << 20)
>> #define APLL_RATIO(x)           (((x) & 0x7) << 24)
>> #define CORE2_RATIO(x)          (((x) & 0x7) << 28)
> 
> Readability is important. Also please pay a note that ARM2_RATIO() is
> the same as CORE2_RATIO(). Frankly I don't know why those defines have
> different names.
> 
> To sum up:
> 
> When you see a potential to extract a common data and it justifies the
> need for creating a new file, then go for it.
> 
> Is the setup.h the best candidate for data extraction? Hard to say.
> 
> If there aren't many defines to be easily extracted, then we can leave
> things as they are with separate setup.h for XU3.
> 

Actually, such a clock setting is expected to done at IPL or sboot.
So I did not consider detailed clock controls.
For now, I concluded such settings are board specific feature.
So I think, new setup file is better.

Thanks,
Minkyu Kang.