| Message ID | 1414757673-32409-4-git-send-email-human.hwang@samsung.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Minkyu Kang |
| Headers | show |
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. > 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. > #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)
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
Hi Hyungwon, > 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) 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. > > Thanks. > > Best regards, > Hyungwon Hwang >
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@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) > > 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.
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" #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) - /* CLK_SRC_CPU */ #define MUX_APLL_SEL(x) ((x) & 0x1) #define MUX_CORE_SEL(x) (((x) & 0x1) << 16)
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(-)