On Mon, 03 Nov 2014 09:51:25 +0100 Lukasz Majewski l.majewski@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@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)
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)
Thanks.
Best regards, Hyungwon Hwang