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

[Hyungwon Hwang <human.hwang@samsung.com>]

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

-- 
Hyungwon Hwang
S/W Platform Team, Software Center
Samsung Electronics
human.hwang@samsung.com