Hello Josh,

Thanks for the review. The comments are appreciated. Please see my inline
replies.

Thanks,
Tanmay

On Wed, Nov 23, 2011 at 7:40 PM, Josh Boyer [off-list ref] wrote:

On Wed, Nov 23, 2011 at 4:44 AM, Tanmay Inamdar [off-list ref] wrote:

quoted

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.

Ugh.  What is this for?  More specifically, if the UART requires word
reads and writes, shouldn't it be using reg-shift/reg-offset in the
device tree?  I'm confused why UDBG would need this sort of code, but
the runtime serial driver doesn't?

It seems the name 'UART_16550_WORD_ADDRESSABLE' is confusing here.
The UART has been modeled after the industry-standard 16550. However,
the register address space has been relocated to 32-bit data boundaries.
For each register, only 1st bit is valid and rest 3 bits are just reserved
and read
as zero. Hence we need to pack the structure accordingly.

runtime serial driver also needs changes in register definitions. However
it is not
included in this patch.

In the next version of patch, I will remove UART stuff. I will make
separate patch
for UART.

quoted

diff --git a/arch/powerpc/boot/dts/klondike.dts

b/arch/powerpc/boot/dts/klondike.dts

quoted

+       OCM: ocm@20000000 {
+               compatible = "ibm,ocm";
+               status = "enabled";
+               cell-index = <1>;
+               reg = < 0x20000000 0x1f000   /* 128K - 4K NAND */
+                       0xfffe0000 0x1f000>; /* 128K - 4K I2C  */
+               bootmode = "nand";
+       };

What is this?  There's nothing in the kernel or in this patch that
binds with "ibm,ocm".  Also, that 'bootmode' property doesn't seem
like a hardware value, but a human descriptor of something that
switches it to boot from NAND.

OCM driver is not yet submitted. I will skip the blocks in the dts which

are still
not supported in the next version.

You are right about bootmode. OCM gets mapped to different addresses in
different boot modes. Uboot takes care of updating this parameter.

quoted

+               crypto: crypto@40000000 {
+                       device_type = "crypto";
+                       compatible = "405ex-crypto",

"amcc,ppc405ex-crypto", "amcc,ppc4xx-crypto";

Why is the "405ex-crypto" string there?

quoted

+               EDMA: edma@40080000 {
+                       #address-cells = <1>;
+                       #size-cells = <1>;
+                       compatible = "amcc,edma";
+                       device_type = "dma";
+                       /*complQ-fifo-memory = "ocm";*/
+                       cell-index = <0>;
+                       reg = <0x40080000 0x00010000>;
+                       dcr-reg = <0x060 0x09f>;
+
+                       interrupt-parent = <&UIC0>;
+                       interrupts = </*complQ A */  0x4 4
+                                       /*EDMA Err */  0x6 4 >;
+
+                       dma-channel@0 {
+                               compatible = "amcc,edma-channel";
+                               /*descriptor-memory = "ocm";*/
+                               cell-index = <0>;
+                               dcr-reg = <0x0000006a 0x0000006b>;
+                       };
+               };

What's this?  Again, nothing binds to "amcc,edma" in the kernel or
patch.  At the very least this (and OCM above) need some binding
descriptions added to Documentation/devicetree/bindings/powerpc/4xx/

I will skip this in next patch. I will consider the bindings.

quoted

+
+               MSI: dwc_pcie-msi@40090000 {
+                       compatible = "amcc,dwc_pcie-msi", "dwc_pcie-msi";
+                       status = "ok";

Is the status property something that is set by U-Boot, or will it
always be "ok"?  If the latter, I don't think you need to specify it
at all.

Correct. There is no need of status. MSI is always enabled

quoted

+                       reg = <0x40090000 0x100>;
+                       interrupts =<0x0 0x1 0x2 0x3>;
+                       interrupt-parent = <&MSI>;
+                       #interrupt-cells = <1>;
+                       #address-cells = <0>;
+                       #size-cells = <0>;
+                       interrupt-map = <0x0 &UIC0 0x0C 0x1
+                                        0x1 &UIC0 0x0D 0x1
+                                        0x2 &UIC0 0x0E 0x1
+                                        0x3 &UIC0 0x0F 0x1>;
+               };

Same binding comment here.

quoted

+
+               AHB: ahb {
+                       device_type = "ahb";

I doubt the device_type is needed.

I believe device_type can be skipped in most of the cases.
I will take care of this in next revision of patch.

quoted

+                       compatible = "amcc,405ex-ahb", "ibm,ahb";
+                       #address-cells = <1>;
+                       #size-cells = <1>;
+                       ranges;
+                       clock-frequency = <0>; /* Filled in by U-Boot */

Same binding comment for ahb.

quoted

+
+                       lcd: lcd@58060000 {
+                               device_type = "lcd";

Drop device_type.

quoted

+                               compatible = "apm,apm-lcd","apm,db9000";
+                               version = "apm-1.1";

Why is 'version' there?  Version of what?  The hardware itself, the
driver?  I doubt this is needed.

It is chip version. There are some changes in this block with respect to
chip
revision. The PVR remains the same as old chip revision and there is no
other register to specify revision. Uboot updates this parameter.

I will skip this block from next patch and will reintroduce it when the
actual
driver is added.

quoted

+                               reg = <0x58060000 0x00001000>;
+                               interrupt-parent = <&UIC0>;
+                               /*
+                                * interrupt index 0 for chip 1.0
+                                * interrupt index 1 for chip 1.1
+                                */
+                               interrupts = <0x1c 2 0x1c 4>;
+                       };

Same binding comment for apm,apm-lcd.  I'm just going to assert again
that anything that doesn't have a defined binding and/or driver needs
to be documented when it's introduced.  Repeat that for the rest of
the patch :).

Yes.

quoted

+
+                       sdhc0: sdhc@58050000 {
+                               device_type = "sdhc";

Drop device_type.

quoted

+                               compatible = "amcc,ahb-sdhc";
+                               #interrupt-cells = <1>;
+                               reg = <0x58050000 0x100>;
+                               interrupt-parent = <&UIC0>;
+                               interrupts = <0x18 0x4>;
+                               bootmode = "i2c";
+                       };
+
+                       tdm0: tdm@58010000 {
+                               device_type = "tdm";

Drop device_type.

quoted

+                               status = "disabled";

Is that set by U-Boot?

Yes. The chip is multiplexed. Some IPs get disabled/enabled based on
bootmodes.

quoted

+                               compatible = "apm,ahb-tdm";
+                               #interrupt-cells = <1>;
+                               reg = <0x58010000 0x100>;
+                               interrupt-parent = <&UIC1>;
+                               interrupts = <0x15 0x1>;
+                       };
+
+                       usbotg0: usbotg@58080000 {
+                               device_type = "usb";

Drop device_type.

quoted

+                               compatible = "apm,usb-otg";
+                               reg = <0x58080000 0x10000>;
+                               interrupt-parent = <&UIC0>;
+                               interrupts = <0x1B 4>;
+                               mode = "host";
+                       };
+                       spi0: spi@50000000 {
+                               #address-cells = <1>;
+                               #size-cells = <0>;
+                               device_type = "spi";

Drop device_type.  I think you're starting to get the trend, so repeat
for the rest of the devices.

quoted

+                               compatible = "apm,apm-spi";
+                               reg = <0x50000000 0x100>;
+                               interrupt-parent = <&UIC0>;
+                               interrupts = <0x7 1>;  /* interrupt

number->0x7 Polarity->HIGH Sensitivity->LEVEL */

quoted

+                               half_duplex = <0x1>;   /*0 = rx/tx mode,

1 = eprom read mode */

quoted

+                               sysclk = <100000000>;   /* input clock */
+                               bus_num = <0x0>;       /* SPI = 0 */

quoted

+
+                       PCIE0: pciex@58020000 {
+                               device_type = "pci";
+                               compatible = "ibm,plb-pciex",

"dwc-pciex", "amcc,dwc-pciex";

Why the unprefixed dwc-pciex compatible property?

I will correct it.

quoted

+                               #interrupt-cells = <1>;
+                               #size-cells = <2>;
+                               #address-cells = <3>;
+                               primary;
+                               port = <0>; /* port number */
+                               status = "ok";

quoted

+                       PCIE1: pciex@58030000 {
+                               device_type = "pci";
+                               compatible = "ibm,plb-pciex",

"dwc-pciex", "amcc,dwc-pciex";

quoted

+                               #interrupt-cells = <1>;
+                               #size-cells = <2>;
+                               #address-cells = <3>;
+                               primary;
+                               port = <1>; /* port number */
+                               status = "disabled";

Is this set by U-Boot?

Yes. Same as multiplexed comment.

quoted

+                       sata@58040000 {
+                               compatible = "sata-ahci";

Uh.. what?

It is designware SATA IP compatible with AHCI standard.
I will reintroduce this block along with its driver.

quoted

+                               reg =  <0x58040000 0x2000>;
+                               interrupt-parent = <&UIC0>;
+                               interrupts = <0x1a 1>;

quoted

+                               ufc@0xFE000000 {
+                                       compatible = "ibm,ufc";
+                                       reg = <0xFE000000 0x00010000>;
+                                       #address-cells = <1>;
+                                       #size-cells = <1>;
+                                       bootmode = "nand";

Is UFC some kind of new flash controller that isn't NDFC?  Also,
bootmode seems to be a human and/or driver variable, not a description
of the hardware.

Yes. UFC is new flash controller. You are right about bootmode.

quoted

+                       UART0: serial@50001000 {
+                               device_type = "serial";
+                               compatible = "ns16550";
+                               reg = <0x50001000 0x00000100>;
+                               virtual-reg = <0x50001000>;
+                               clock-frequency = <300000000>; /* Filled

in by U-Boot */

quoted

+                               current-speed = <115200>;
+                               interrupt-parent = <&UIC0>;
+                               interrupts = <0x0 0x4>;
+                               /*reg-shift = <2>;*/

This is commented out, but seems to be needed when you take the
word-addressed UDBG thing into account?

Yes. I will rethink on how it is implemented.

quoted

+                       IEEE1588_0: ieee1588ts0@400a5000 {
+                               status = "ok";
+                               compatible = "ieee1588-ts";

What is that?

This is AMCC IEEE1588 block. I will correct the compatible string.

quoted

diff --git a/arch/powerpc/configs/40x/klondike_defconfig

b/arch/powerpc/configs/40x/klondike_defconfig

quoted

new file mode 100644
index 0000000..840f438

--- /dev/null
+++ b/arch/powerpc/configs/40x/klondike_defconfig

@@ -0,0 +1,1353 @@
+#
+# Automatically generated file; DO NOT EDIT.
+# Linux/powerpc 3.2.0-rc2 Kernel Configuration
+#
+# CONFIG_PPC64 is not set

This is a full defconfig.  We don't need a full config file.  You can
generate one with 'make savedefconfig' that contains only the options
you need to set.

I will do that.

quoted

diff --git a/arch/powerpc/kernel/udbg_16550.c

b/arch/powerpc/kernel/udbg_16550.c

quoted

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);

quoted

 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];

An array of length 3 for something "word-addressable"?  When did words
change to 3 bytes?  Now, I still haven't finished my coffee yet, but
that is really confusing.

Again the name  WORD_ADDRESSABLE is confusing. Hopefully my previous
comment clears the confusion.

quoted

+       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

@@ -52,8 +66,16 @@ static struct NS16550 __iomem *udbg_comport;
 static void udbg_550_flush(void)
 {
       if (udbg_comport) {
+#if defined(CONFIG_APM8018X)
+               int index;
+               for (index = 0; index < 3500; index++) {
+                       if ((in_8(&udbg_comport->lsr) & LSR_THRE) ==

LSR_THRE)

quoted

+                               break;
+               }
+#else

What is index, and why do you read the same register 3500 times?  That
doesn't sound like an index, it sounds like some kind of poor-mans
timeout.

This is hardware bug. Ideally there should not be any change in the code. I
will try to
fix it in better way.

quoted

               while ((in_8(&udbg_comport->lsr) & LSR_THRE) == 0)
                       /* wait for idle */;
+#endif /* CONFIG_APM8018X */
       }
 }

diff --git a/arch/powerpc/platforms/40x/Kconfig

b/arch/powerpc/platforms/40x/Kconfig

quoted

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

default n please.

Yes.

quoted

+       select PPC40x_SIMPLE
+       select UART_16550_WORD_ADDRESSABLE
+       help
+         This option enables support for the AppliedMicro Klondike

board.

quoted

+

diff --git a/arch/powerpc/platforms/40x/ppc40x_simple.c

b/arch/powerpc/platforms/40x/ppc40x_simple.c

quoted

index e8dd5c5..c8576af 100644

--- a/arch/powerpc/platforms/40x/ppc40x_simple.c
+++ b/arch/powerpc/platforms/40x/ppc40x_simple.c

@@ -17,7 +17,7 @@
 #include <asm/pci-bridge.h>
 #include <asm/ppc4xx.h>
 #include <asm/prom.h>
-#include <asm/time.h>
+#include <linux/time.h>

Is this needed for a reason?  If so, it should be submitted as a separate
patch.

checkpatch.pl scripts throws warning. It asks to change <asm/time.h> to
<linux/time.h>

quoted

 #include <asm/udbg.h>
 #include <asm/uic.h>

@@ -29,6 +29,7 @@ static __initdata struct of_device_id ppc40x_of_bus[]

= {

quoted

       { .compatible = "ibm,plb4", },
       { .compatible = "ibm,opb", },
       { .compatible = "ibm,ebc", },
+       { .compatible = "ibm,ahb", },
       { .compatible = "simple-bus", },
       {},
 };

@@ -55,6 +56,7 @@ static const char *board[] __initdata = {
       "amcc,haleakala",
       "amcc,kilauea",
       "amcc,makalu",
+       "amcc,klondike",
       "est,hotfoot"
 };

--

1.6.1.rc3

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.