Hello Josh,<br><br>Thanks for the review. The comments are appreciated. Please see my inline replies.<br><br>Thanks,<br>Tanmay<br><br><div class="gmail_quote">On Wed, Nov 23, 2011 at 7:40 PM, Josh Boyer <span dir="ltr"><<a href="mailto:jwboyer@gmail.com">jwboyer@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div class="im">On Wed, Nov 23, 2011 at 4:44 AM, Tanmay Inamdar <<a href="mailto:tinamdar@apm.com">tinamdar@apm.com</a>> wrote:<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>Ugh. What is this for? More specifically, if the UART requires word<br>
reads and writes, shouldn't it be using reg-shift/reg-offset in the<br>
device tree? I'm confused why UDBG would need this sort of code, but<br>
the runtime serial driver doesn't?<br></blockquote><div> </div><div>It seems the name 'UART_16550_WORD_ADDRESSABLE' is confusing here.<br>The UART
has been modeled after the industry-standard 16550. However,<br>the register
address space has been relocated to 32-bit data boundaries.<br>For each
register, only 1st bit is valid and rest 3 bits are just reserved and read<br>as
zero. Hence we need to pack the structure accordingly.<br><br>runtime serial driver also needs changes in register definitions. However it is not<br>included in this patch.<br><br>In the next version of patch, I will remove UART stuff. I will make separate patch<br>
for UART.<br> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
> diff --git a/arch/powerpc/boot/dts/klondike.dts b/arch/powerpc/boot/dts/klondike.dts<br>
<div class="im"><br>
<br>
> + OCM: ocm@20000000 {<br>
> + compatible = "ibm,ocm";<br>
> + status = "enabled";<br>
> + cell-index = <1>;<br>
> + reg = < 0x20000000 0x1f000 /* 128K - 4K NAND */<br>
> + 0xfffe0000 0x1f000>; /* 128K - 4K I2C */<br>
> + bootmode = "nand";<br>
> + };<br>
<br>
</div>What is this? There's nothing in the kernel or in this patch that<br>
binds with "ibm,ocm". Also, that 'bootmode' property doesn't seem<br>
like a hardware value, but a human descriptor of something that<br>
switches it to boot from NAND.<br>
<div class="im"><br></div></blockquote><div>OCM driver is not yet submitted. I will skip the blocks in the dts which are still<br>not supported in the next version.<br><br>You are right about bootmode. OCM gets mapped to different addresses in<br>
different boot modes. Uboot takes care of updating this parameter.<br> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div class="im">
<br>
> + crypto: crypto@40000000 {<br>
> + device_type = "crypto";<br>
> + compatible = "405ex-crypto", "amcc,ppc405ex-crypto", "amcc,ppc4xx-crypto";<br>
<br>
</div>Why is the "405ex-crypto" string there?<br>
<div class="im"><br>
> + EDMA: edma@40080000 {<br>
> + #address-cells = <1>;<br>
> + #size-cells = <1>;<br>
> + compatible = "amcc,edma";<br>
> + device_type = "dma";<br>
> + /*complQ-fifo-memory = "ocm";*/<br>
> + cell-index = <0>;<br>
> + reg = <0x40080000 0x00010000>;<br>
> + dcr-reg = <0x060 0x09f>;<br>
> +<br>
> + interrupt-parent = <&UIC0>;<br>
> + interrupts = </*complQ A */ 0x4 4<br>
> + /*EDMA Err */ 0x6 4 >;<br>
> +<br>
> + dma-channel@0 {<br>
> + compatible = "amcc,edma-channel";<br>
> + /*descriptor-memory = "ocm";*/<br>
> + cell-index = <0>;<br>
> + dcr-reg = <0x0000006a 0x0000006b>;<br>
> + };<br>
> + };<br>
<br>
</div>What's this? Again, nothing binds to "amcc,edma" in the kernel or<br>
patch. At the very least this (and OCM above) need some binding<br>
descriptions added to Documentation/devicetree/bindings/powerpc/4xx/<br></blockquote><div><br>I will skip this in next patch. I will consider the bindings.<br><br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div class="im"><br>
> +<br>
> + MSI: dwc_pcie-msi@40090000 {<br>
> + compatible = "amcc,dwc_pcie-msi", "dwc_pcie-msi";<br>
> + status = "ok";<br>
<br>
</div>Is the status property something that is set by U-Boot, or will it<br>
always be "ok"? If the latter, I don't think you need to specify it<br>
at all.<br>
<div class="im"><br></div></blockquote><div><br>Correct. There is no need of status. MSI is always enabled<br> <br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div class="im">
> + reg = <0x40090000 0x100>;<br>
> + interrupts =<0x0 0x1 0x2 0x3>;<br>
> + interrupt-parent = <&MSI>;<br>
> + #interrupt-cells = <1>;<br>
> + #address-cells = <0>;<br>
> + #size-cells = <0>;<br>
> + interrupt-map = <0x0 &UIC0 0x0C 0x1<br>
> + 0x1 &UIC0 0x0D 0x1<br>
> + 0x2 &UIC0 0x0E 0x1<br>
> + 0x3 &UIC0 0x0F 0x1>;<br>
> + };<br>
<br>
</div>Same binding comment here.<br>
<div class="im"><br>
> +<br>
> + AHB: ahb {<br>
> + device_type = "ahb";<br>
<br>
</div>I doubt the device_type is needed.<br></blockquote><div><br>I believe device_type can be skipped in most of the cases.<br>I will take care of this in next revision of patch. <br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div class="im"><br>
> + compatible = "amcc,405ex-ahb", "ibm,ahb";<br>
> + #address-cells = <1>;<br>
> + #size-cells = <1>;<br>
> + ranges;<br>
> + clock-frequency = <0>; /* Filled in by U-Boot */<br>
<br>
</div>Same binding comment for ahb.<br>
<div class="im"><br>
> +<br>
> + lcd: lcd@58060000 {<br>
> + device_type = "lcd";<br>
<br>
</div>Drop device_type.<br>
<div class="im"><br>
> + compatible = "apm,apm-lcd","apm,db9000";<br>
> + version = "apm-1.1";<br>
<br>
</div>Why is 'version' there? Version of what? The hardware itself, the<br>
driver? I doubt this is needed.<br></blockquote><div>It is chip version. There are some changes in this block with respect to chip<br>revision. The PVR remains the same as old chip revision and there is no<br>other register to specify revision. Uboot updates this parameter.<br>
<br>I will skip this block from next patch and will reintroduce it when the actual<br>driver is added.<br><br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div class="im"><br>
> + reg = <0x58060000 0x00001000>;<br>
> + interrupt-parent = <&UIC0>;<br>
> + /*<br>
> + * interrupt index 0 for chip 1.0<br>
> + * interrupt index 1 for chip 1.1<br>
> + */<br>
> + interrupts = <0x1c 2 0x1c 4>;<br>
> + };<br>
<br>
</div>Same binding comment for apm,apm-lcd. I'm just going to assert again<br>
that anything that doesn't have a defined binding and/or driver needs<br>
to be documented when it's introduced. Repeat that for the rest of<br>
the patch :).<br></blockquote><div><br>Yes. <br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div class="im"><br>
> +<br>
> + sdhc0: sdhc@58050000 {<br>
> + device_type = "sdhc";<br>
<br>
</div>Drop device_type.<br>
<div class="im"><br>
> + compatible = "amcc,ahb-sdhc";<br>
> + #interrupt-cells = <1>;<br>
> + reg = <0x58050000 0x100>;<br>
> + interrupt-parent = <&UIC0>;<br>
> + interrupts = <0x18 0x4>;<br>
> + bootmode = "i2c";<br>
> + };<br>
> +<br>
> + tdm0: tdm@58010000 {<br>
> + device_type = "tdm";<br>
<br>
</div>Drop device_type.<br>
<br>
> + status = "disabled";<br>
<br>
Is that set by U-Boot?<br></blockquote><div>Yes. The chip is multiplexed. Some IPs get disabled/enabled based on<br>bootmodes. <br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div class="im"><br>
> + compatible = "apm,ahb-tdm";<br>
> + #interrupt-cells = <1>;<br>
> + reg = <0x58010000 0x100>;<br>
> + interrupt-parent = <&UIC1>;<br>
> + interrupts = <0x15 0x1>;<br>
> + };<br>
> +<br>
> + usbotg0: usbotg@58080000 {<br>
> + device_type = "usb";<br>
<br>
</div>Drop device_type.<br>
<div class="im"><br>
> + compatible = "apm,usb-otg";<br>
> + reg = <0x58080000 0x10000>;<br>
> + interrupt-parent = <&UIC0>;<br>
> + interrupts = <0x1B 4>;<br>
> + mode = "host";<br>
> + };<br>
> + spi0: spi@50000000 {<br>
> + #address-cells = <1>;<br>
> + #size-cells = <0>;<br>
> + device_type = "spi";<br>
<br>
</div>Drop device_type. I think you're starting to get the trend, so repeat<br>
for the rest of the devices.<br>
<div class="im"><br>
> + compatible = "apm,apm-spi";<br>
> + reg = <0x50000000 0x100>;<br>
> + interrupt-parent = <&UIC0>;<br>
> + interrupts = <0x7 1>; /* interrupt number->0x7 Polarity->HIGH Sensitivity->LEVEL */<br>
> + half_duplex = <0x1>; /*0 = rx/tx mode, 1 = eprom read mode */<br>
> + sysclk = <100000000>; /* input clock */<br>
> + bus_num = <0x0>; /* SPI = 0 */<br>
<br>
> +<br>
</div>> + PCIE0: pciex@58020000 {<br>
<div class="im">> + device_type = "pci";<br>
> + compatible = "ibm,plb-pciex", "dwc-pciex", "amcc,dwc-pciex";<br>
<br>
</div>Why the unprefixed dwc-pciex compatible property?<br></blockquote><div> </div><div>I will correct it. <br><br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div class="im"><br>
> + #interrupt-cells = <1>;<br>
> + #size-cells = <2>;<br>
> + #address-cells = <3>;<br>
> + primary;<br>
> + port = <0>; /* port number */<br>
> + status = "ok";<br>
<br>
<br>
</div>> + PCIE1: pciex@58030000 {<br>
<div class="im">> + device_type = "pci";<br>
> + compatible = "ibm,plb-pciex", "dwc-pciex", "amcc,dwc-pciex";<br>
> + #interrupt-cells = <1>;<br>
> + #size-cells = <2>;<br>
> + #address-cells = <3>;<br>
> + primary;<br>
> + port = <1>; /* port number */<br>
> + status = "disabled";<br>
<br>
</div>Is this set by U-Boot?<br></blockquote><div>Yes. Same as multiplexed comment. <br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div class="im"><br>
<br>
> + sata@58040000 {<br>
> + compatible = "sata-ahci";<br>
<br>
</div>Uh.. what?<br></blockquote><div>It is designware SATA IP compatible with AHCI standard.<br>I will reintroduce this block along with its driver.<br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div class="im"><br>
> + reg = <0x58040000 0x2000>;<br>
> + interrupt-parent = <&UIC0>;<br>
> + interrupts = <0x1a 1>;<br>
<br>
<br>
</div>> + ufc@0xFE000000 {<br>
<div class="im">> + compatible = "ibm,ufc";<br>
> + reg = <0xFE000000 0x00010000>;<br>
> + #address-cells = <1>;<br>
> + #size-cells = <1>;<br>
> + bootmode = "nand";<br>
<br>
</div>Is UFC some kind of new flash controller that isn't NDFC? Also,<br>
bootmode seems to be a human and/or driver variable, not a description<br>
of the hardware.<br></blockquote><div> </div><div>Yes. UFC is new flash controller. You are right about bootmode. <br><br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div class="im"><br>
> + UART0: serial@50001000 {<br>
> + device_type = "serial";<br>
> + compatible = "ns16550";<br>
> + reg = <0x50001000 0x00000100>;<br>
> + virtual-reg = <0x50001000>;<br>
> + clock-frequency = <300000000>; /* Filled in by U-Boot */<br>
> + current-speed = <115200>;<br>
> + interrupt-parent = <&UIC0>;<br>
> + interrupts = <0x0 0x4>;<br>
> + /*reg-shift = <2>;*/<br>
<br>
</div>This is commented out, but seems to be needed when you take the<br>
word-addressed UDBG thing into account?<br></blockquote><div> </div><div>Yes. I will rethink on how it is implemented. <br><br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div class="im"><br>
<br>
> + IEEE1588_0: ieee1588ts0@400a5000 {<br>
> + status = "ok";<br>
> + compatible = "ieee1588-ts";<br>
<br>
</div>What is that?<br></blockquote><div>This is AMCC IEEE1588 block. I will correct the compatible string.<br><br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div class="im"><br>
> diff --git a/arch/powerpc/configs/40x/klondike_defconfig b/arch/powerpc/configs/40x/klondike_defconfig<br>
> new file mode 100644<br>
> index 0000000..840f438<br>
> --- /dev/null<br>
> +++ b/arch/powerpc/configs/40x/klondike_defconfig<br>
> @@ -0,0 +1,1353 @@<br>
> +#<br>
> +# Automatically generated file; DO NOT EDIT.<br>
> +# Linux/powerpc 3.2.0-rc2 Kernel Configuration<br>
> +#<br>
> +# CONFIG_PPC64 is not set<br>
<br>
</div>This is a full defconfig. We don't need a full config file. You can<br>
generate one with 'make savedefconfig' that contains only the options<br>
you need to set.<br>
<div class="im"><br></div></blockquote><div>I will do that. <br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div class="im">
<br>
<br>
<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>
<br>
</div>An array of length 3 for something "word-addressable"? When did words<br>
change to 3 bytes? Now, I still haven't finished my coffee yet, but<br>
that is really confusing.<br></blockquote><div> </div><div>Again the name WORD_ADDRESSABLE is confusing. Hopefully my previous<br>comment clears the confusion.<br><br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div><div></div><div class="h5"><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>
> @@ -52,8 +66,16 @@ static struct NS16550 __iomem *udbg_comport;<br>
> static void udbg_550_flush(void)<br>
> {<br>
> if (udbg_comport) {<br>
> +#if defined(CONFIG_APM8018X)<br>
> + int index;<br>
> + for (index = 0; index < 3500; index++) {<br>
> + if ((in_8(&udbg_comport->lsr) & LSR_THRE) == LSR_THRE)<br>
> + break;<br>
> + }<br>
> +#else<br>
<br>
</div></div>What is index, and why do you read the same register 3500 times? That<br>
doesn't sound like an index, it sounds like some kind of poor-mans<br>
timeout.<br></blockquote><div>This is hardware bug. Ideally there should not be any change in the code. I will try to<br>fix it in better way. <br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div class="im"><br>
> while ((in_8(&udbg_comport->lsr) & LSR_THRE) == 0)<br>
> /* wait for idle */;<br>
> +#endif /* CONFIG_APM8018X */<br>
> }<br>
> }<br>
><br>
> 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>default n please.<br></blockquote><div><br>Yes. <br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div class="im"><br>
> + select PPC40x_SIMPLE<br>
> + select UART_16550_WORD_ADDRESSABLE<br>
> + help<br>
> + This option enables support for the AppliedMicro Klondike board.<br>
> +<br>
> diff --git a/arch/powerpc/platforms/40x/ppc40x_simple.c b/arch/powerpc/platforms/40x/ppc40x_simple.c<br>
> index e8dd5c5..c8576af 100644<br>
> --- a/arch/powerpc/platforms/40x/ppc40x_simple.c<br>
> +++ b/arch/powerpc/platforms/40x/ppc40x_simple.c<br>
> @@ -17,7 +17,7 @@<br>
> #include <asm/pci-bridge.h><br>
> #include <asm/ppc4xx.h><br>
> #include <asm/prom.h><br>
> -#include <asm/time.h><br>
> +#include <linux/time.h><br>
<br>
</div>Is this needed for a reason? If so, it should be submitted as a separate patch.<br></blockquote><div><br><a href="http://checkpatch.pl">checkpatch.pl</a> scripts throws warning. It asks to change <asm/time.h> to <linux/time.h> <br>
<br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div><div></div><div class="h5"><br>
> #include <asm/udbg.h><br>
> #include <asm/uic.h><br>
><br>
> @@ -29,6 +29,7 @@ static __initdata struct of_device_id ppc40x_of_bus[] = {<br>
> { .compatible = "ibm,plb4", },<br>
> { .compatible = "ibm,opb", },<br>
> { .compatible = "ibm,ebc", },<br>
> + { .compatible = "ibm,ahb", },<br>
> { .compatible = "simple-bus", },<br>
> {},<br>
> };<br>
> @@ -55,6 +56,7 @@ static const char *board[] __initdata = {<br>
> "amcc,haleakala",<br>
> "amcc,kilauea",<br>
> "amcc,makalu",<br>
> + "amcc,klondike",<br>
> "est,hotfoot"<br>
> };<br>
><br>
> --<br>
> 1.6.1.rc3<br>
><br>
><br>
</div></div></blockquote></div><br>
<pre>CONFIDENTIALITY NOTICE: This e-mail message, including any attachments,
is for the sole use of the intended recipient(s) and contains information
that is confidential and proprietary to AppliedMicro Corporation or its subsidiaries.
It is to be used solely for the purpose of furthering the parties' business relationship.
All unauthorized review, use, disclosure or distribution is prohibited.
If you are not the intended recipient, please contact the sender by reply e-mail
and destroy all copies of the original message.