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