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