Hello,
Thanks for the comments. Please see inline.
Regards,
Tanmay
On Wed, Nov 23, 2011 at 3:53 PM, Paul Bolle <pebo...@tiscali.nl> wrote:
> Tanmay,
>
> (Some minor Kconfig related comments follow.)
>
> On Wed, 2011-11-23 at 15:14 +0530, Tanmay Inamdar wrote:
> > The AppliedMicro APM8018X embedded processor targets embedded
> applications that
> > require low power and a small footprint. It features a PowerPC 405
> processor
> > core built in a 65nm low-power CMOS process with a five-stage pipeline
> executing
> > up to one instruction per cycle. The family has 128-kbytes of on-chip
> memory,
> > a 128-bit local bus and on-chip DDR2 SDRAM controller with 16-bit
> interface.
> >
> >[...]
> >
> > Signed-off-by: Tanmay Inamdar <tinam...@apm.com>
> > ---
> > arch/powerpc/Kconfig | 6 +
> > arch/powerpc/boot/dcr.h | 6 +
> > arch/powerpc/boot/dts/klondike.dts | 668 +++++++++++++
> > arch/powerpc/configs/40x/klondike_defconfig | 1353
> +++++++++++++++++++++++++++
> > arch/powerpc/include/asm/dcr-regs.h | 20 +
> > arch/powerpc/kernel/cputable.c | 52 +
> > arch/powerpc/kernel/udbg_16550.c | 22 +
> > arch/powerpc/platforms/40x/Kconfig | 11 +
> > arch/powerpc/platforms/40x/ppc40x_simple.c | 4 +-
> > 9 files changed, 2141 insertions(+), 1 deletions(-)
> > create mode 100644 arch/powerpc/boot/dts/klondike.dts
> > create mode 100644 arch/powerpc/configs/40x/klondike_defconfig
> >
> > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> > index b177caa..3f2cc36 100644
> > --- a/arch/powerpc/Kconfig
> > +++ b/arch/powerpc/Kconfig
> > @@ -978,3 +978,9 @@ config PPC_LIB_RHEAP
> > bool
> >
> > source "arch/powerpc/kvm/Kconfig"
> > +
> > +config UART_16550_WORD_ADDRESSABLE
> > + bool
> > + default n
> > + help
> > + Enable this if your UART 16550 is word addressable.
>
> This is only relevant for this SOC isn't it? If so, it might be better
> to add it to arch/powerpc/platforms/40x/Kconfig.
>
> The help line can be dropped (there's no prompt, so the help won't be
> user visible).
>
> Some people would suggest to use
> def_bool n
>
> here. (I don't really care.)
>
Agreed. As Kumar Gala suggested in his comment, I will move this and make a
separate patch for UART changes.
>
> > [...]
>
> > diff --git a/arch/powerpc/kernel/udbg_16550.c
> b/arch/powerpc/kernel/udbg_16550.c
> > index 6837f83..dd3bce9 100644
> > --- a/arch/powerpc/kernel/udbg_16550.c
> > +++ b/arch/powerpc/kernel/udbg_16550.c
> > @@ -18,6 +18,19 @@ extern void real_writeb(u8 data, volatile u8 __iomem
> *addr);
> > extern u8 real_205_readb(volatile u8 __iomem *addr);
> > extern void real_205_writeb(u8 data, volatile u8 __iomem *addr);
> >
> > +#ifdef CONFIG_UART_16550_WORD_ADDRESSABLE
> > +struct NS16550 {
> > + /* this struct must be packed */
> > + unsigned char rbr; /* 0 */ u8 s0[3];
> > + unsigned char ier; /* 1 */ u8 s1[3];
> > + unsigned char fcr; /* 2 */ u8 s2[3];
> > + unsigned char lcr; /* 3 */ u8 s3[3];
> > + unsigned char mcr; /* 4 */ u8 s4[3];
> > + unsigned char lsr; /* 5 */ u8 s5[3];
> > + unsigned char msr; /* 6 */ u8 s6[3];
> > + unsigned char scr; /* 7 */ u8 s7[3];
> > +};
> > +#else
> > struct NS16550 {
> > /* this struct must be packed */
> > unsigned char rbr; /* 0 */
> > @@ -29,6 +42,7 @@ struct NS16550 {
> > unsigned char msr; /* 6 */
> > unsigned char scr; /* 7 */
> > };
> > +#endif /* CONFIG_UART_16550_WORD_ADDRESSABLE */
> >
> > #define thr rbr
> > #define iir fcr
> > [...]
> > diff --git a/arch/powerpc/platforms/40x/Kconfig
> b/arch/powerpc/platforms/40x/Kconfig
> > index 1530229..3d0d1d9 100644
> > --- a/arch/powerpc/platforms/40x/Kconfig
> > +++ b/arch/powerpc/platforms/40x/Kconfig
> > @@ -186,3 +186,14 @@ config IBM405_ERR51
> > # bool
> > # depends on !STB03xxx && PPC4xx_DMA
> > # default y
> > +#
> > +
> > +config APM8018X
> > + bool "APM8018X"
> > + depends on 40x
> > + default y
>
> Why is this "default y"? All other "selectors" of PPC40x_SIMPLE default
> to n.
>
> Yes. This is a mistake. This should be 'n'.
> > + select PPC40x_SIMPLE
>
> There was recently some powerpc related discussion on using "select" on
> symbols that are themselves user selectable (see
> https://lkml.org/lkml/2011/11/9/426 ). But other symbols already select
> this symbol so this is not specific to this patch.
>
> > + select UART_16550_WORD_ADDRESSABLE
> > + help
> > + This option enables support for the AppliedMicro Klondike board.
> > +
>
> Since you're selecting it here it's good that you made
> UART_16550_WORD_ADDRESSABLE hidden (as it has no prompt). But perhaps
> you could even drop it and in the code just test for CONFIG_APM8018X. Or
> are you expecting more users of UART_16550_WORD_ADDRESSABLE?
>
At this moment, it is only APM8018X SOC expects this. I am not sure about
more users of UART_16550_WORD_ADDRESSABLE. I will try to eliminate it or
make it as a run time selection.
>
> > [...]
>
>
> Paul Bolle
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
