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

[Lukasz Majewski]

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.

> 
> Thanks.
> 
> Best regards,
> Hyungwon Hwang
> 



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group