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.majew...@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.hw...@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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
