PIKA Warp: Update platform code to support Rev B boards

Hi Sean,

arch/powerpc/platforms/44x/warp-nand.c line 14

Quoted 7 lines · On Sun, 27 Apr 2008 15:25:46 -0400 Sean MacLennan <[email protected]>…

+++ b/arch/powerpc/platforms/44x/warp-nand.c

arch/powerpc/platforms/44x/warp-nand.c +1 -0

@@ -11,8 +11,10 @@
 #include <linux/mtd/partitions.h>
 #include <linux/mtd/nand.h>
 #include <linux/mtd/ndfc.h>
+#include <linux/of_platform.h>

You really want linux.of.h (unless there was some preexisting reason to require of_platform.h).

arch/powerpc/platforms/44x/warp.c define_machine(warp) { · line 187

Quoted 9 lines

arch/powerpc/platforms/44x/warp.c +9 -0

+static void pika_setup_critical_temp(struct i2c_client *client)
+{
+     struct device_node *np;
+     int irq, rc;
+
+     /* Setup power LEDS for possible critical temp */
+     np = of_find_compatible_node(NULL, NULL, "ibm,gpio-440EP");
+     if (np)
+             np = of_find_compatible_node(np, NULL, "ibm,gpio-440EP");

Did you really mean to test if (!np) above instead?

Actually, it looks like he's trying to find the second gpio node in the tree.

Sean, if that is true, then this is a very fragile way to do it. Really, you should have a phandle somewhere that points to the GPIO node that your LEDs are attached to. Others have been addressing the same problem and the consensus seems to be to add a 'leds' node for each of your leds with a phandle and gpio descriptor to the gpio node.

See the documentation added by this patch (section 't'):
http://patchwork.ozlabs.org/linuxppc/patch?id=18156

Quoted 2 lines · On Sun, 27 Apr 2008 19:51:57 -0600 "Grant Likely" <[email protected]>…

Actually, it looks like he's trying to find the second gpio node in the tree.

Correct.

Quoted 6 lines

Sean, if that is true, then this is a very fragile way to do it. Really, you should have a phandle somewhere that points to the GPIO node that your LEDs are attached to. Others have been addressing the same problem and the consensus seems to be to add a 'leds' node for each of your leds with a phandle and gpio descriptor to the gpio node.

Quoted 2 lines

See the documentation added by this patch (section 't'):
http://patchwork.ozlabs.org/linuxppc/patch?id=18156

I saw that earlier. I thought that that method relied on the gpio_led driver? I want to use the gpio_led driver, but I believe the underlying gpio code for the 440EP is not done yet.

If your LEDs are attached to gpio pins, then you should use the current draft led->gpio bindings as shown in the above patch. Then, let your platform code extract whatever data it needs from the device tree to set up the LEDs.

On Sun, 27 Apr 2008 22:47:43 -0600

Quoted 4 lines · Grant Likely

If your LEDs are attached to gpio pins, then you should use the current draft led->gpio bindings as shown in the above patch. Then, let your platform code extract whatever data it needs from the device tree to set up the LEDs.

I added the following to the dts:

        led@31 {
                compatible = "linux,gpio-led";
                linux,name = "green";
                gpios = <&GPIO1 31>;
        };

        led@30 {
                compatible = "linux,gpio-led";
                linux,name = "red";
                gpios = <&GPIO1 30>;
        };

I then map the gpio base as follows (I removed the if checks just to make things short and sweet):

        np = of_find_compatible_node(NULL, NULL, "linux,gpio-led");

        gpios = of_get_property(np, "gpios", &lenp);
        of_node_put(np);

        np = of_find_node_by_phandle(gpios[0]);


        gpio_base = of_iomap(np, 0);
        of_node_put(np);

This looks appropriate. You'll need to make sure that the values in the linux,name property meet the Linux LED naming guidelines. I think this is covered in Documentation/leds-class.c. You can also as Richard Purdie; the LED subsystem maintainer.

Quoted 4 lines · On Mon, 28 Apr 2008 11:44:19 -0600 "Grant Likely" <[email protected]>…

This looks appropriate. You'll need to make sure that the values in the linux,name property meet the Linux LED naming guidelines. I think this is covered in Documentation/leds-class.c. You can also as Richard Purdie; the LED subsystem maintainer.

The leds name is "devicename:colour:function" where you are allowed to leave sections blank. So I only filled in the colour ;)

I also notice that it is colour, not color.

You can leave sections blank but it pays to leave the separator in so use ":red:" or ":red", not "red".

Quoted 2 lines · On Mon, 28 Apr 2008 21:44:05 +0100 "Richard Purdie" <[email protected]> wrote:

You can leave sections blank but it pays to leave the separator in so use ":red:" or ":red", not "red".

Ok, :red: and :green: it is.

What would be the advantage of pika:red: or warp:red:?

You really want linux.of.h (unless there was some preexisting reason to require of_platform.h).

Ok, here is another version of the patch with Stephen Rothwell's and
Grant Likely's suggestions.

Cheers,
Sean

diff --git a/arch/powerpc/boot/dts/warp.dts b/arch/powerpc/boot/dts/warp.dts index b04a52e..d124497 100644
--- a/arch/powerpc/boot/dts/warp.dts
+++ b/arch/powerpc/boot/dts/warp.dts
@@ -186,6 +179,16 @@

                                reg = <ef600700 14>;
                                interrupt-parent = <&UIC0>;
                                interrupts = <2 4>;
 +                               index = <0>;
 +                               #address-cells = <1>;
 +                               #size-cells = <0>;
 +
 +                               ad7414@4a {
 +                                       compatible = "adi,ad7414";
 +                                       reg = <4a>;
 +                                       interrupts = <19 8>;
 +                                       interrupt-parent = <&UIC0>;
 +                               };
                        };

                        GPIO0: gpio@ef600b00 {
 @@ -196,8 +199,22 @@
                        GPIO1: gpio@ef600c00 {
                                compatible = "ibm,gpio-440ep";
                                reg = <ef600c00 48>;

+
+ };

+
+ led@31 {

+ compatible = "linux,gpio-led";
+ linux,name = "green";
+ gpios = <&GPIO1 31>;
+ };
+
+ led@30 {

 +                               compatible = "linux,gpio-led";
 +                               linux,name = "red";
 +                               gpios = <&GPIO1 30>;
                        };

+

                        ZMII0: emac-zmii@ef600d00 {
                                compatible = "ibm,zmii-440ep", "ibm,zmii-440gp", "ibm,zmii";
                                reg = <ef600d00 c>;

arch/powerpc/platforms/44x/warp-nand.c +0 -1

 index 9150318..d293c70 100644
 
--- a/arch/powerpc/platforms/44x/warp-nand.c
 +++ b/arch/powerpc/platforms/44x/warp-nand.c
 @@ -11,8 +11,10 @@
  #include <linux/mtd/partitions.h>
  #include <linux/mtd/nand.h>
  #include <linux/mtd/ndfc.h>
 +#include <linux/of.h>
 
 
 #include <asm/machdep.h>
 
 +
  #ifdef CONFIG_MTD_NAND_NDFC
 
  #define CS_NAND_0      1       /* use chip select 1 for NAND device 0 */
 @@ -35,13 +37,23 @@ static struct mtd_partition nand_parts[] = {
        {
                .name   = "root",
                .offset = 0x0200000,
 -               .size   = 0x3400000
 +               .size   = 0x3E00000
 +       },
 +       {
 +               .name   = "persistent",
 +               .offset = 0x4000000,
 +               .size   = 0x4000000
        },
        {
 -               .name   = "user",
 -               .offset = 0x3600000,
 -               .size   = 0x0A00000

struct ndfc_controller_settings warp_ndfc_settings = {
@@ -67,19 +79,15 @@ static struct platform_device warp_ndfc_device = {
.resource = &warp_ndfc,
};

-static struct nand_ecclayout nand_oob_16 = {

 -       .eccbytes = 3,
 -       .eccpos = { 0, 1, 2, 3, 6, 7 },
 -       .oobfree = { {.offset = 8, .length = 16} }
 -};
 -
 +/* Do NOT set the ecclayout: let it default so it is correct for both
 + * 64M and 256M flash chips.
 + */
  static struct platform_nand_chip warp_nand_chip0 = {
        .nr_chips = 1,
        .chip_offset = CS_NAND_0,
        .nr_partitions = ARRAY_SIZE(nand_parts),
        .partitions = nand_parts,
 -       .chip_delay = 50,
 -       .ecclayout = &nand_oob_16,
 +       .chip_delay = 20,

.priv = &warp_chip0_settings,
};

@@ -96,6 +104,23 @@ static struct platform_device warp_nand_device = {

static int warp_setup_nand_flash(void)
{
+ struct device_node *np;
+

 +       /* Try to detect a rev A based on NOR size. */
 +       np = of_find_compatible_node(NULL, NULL, "cfi-flash");
 +       if (np) {
 +               struct property *pp;
 +
 +               pp = of_find_property(np, "reg", NULL);
 +               if (pp && (pp->length == 12)) {
 +                       u32 *v = pp->value;
 +                       if (v[2] == 0x4000000)
 +                               /* Rev A = 64M NAND */
 +                               warp_nand_chip0.nr_partitions = 2;
 +               }
 +               of_node_put(np);
 +       }
 +
        platform_device_register(&warp_ndfc_device);
        platform_device_register(&warp_nand_device);

diff --git a/arch/powerpc/platforms/44x/warp.c b/arch/powerpc/platforms/44x/warp.c index 39cf615..8f7d016 100644

--- a/arch/powerpc/platforms/44x/warp.c
+++ b/arch/powerpc/platforms/44x/warp.c
@@ -12,6 +12,10 @@

#include <linux/init.h>
#include <linux/of_platform.h>
#include <linux/kthread.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/pika.h>
+#include <linux/delay.h>

#include <asm/machdep.h>
#include <asm/prom.h>
@@ -27,6 +31,18 @@ static __initdata struct of_device_id warp_of_bus[] = {

        {},

};

+static __initdata struct i2c_board_info warp_i2c_info[] = {
+ { I2C_BOARD_INFO("ad7414", 0x4a) }
+};
+
+static int __init warp_arch_init(void)
+{

 +       /* This should go away once support is moved to the dts. */
 +       i2c_register_board_info(0, warp_i2c_info, ARRAY_SIZE(warp_i2c_info));
 +       return 0;
 +}
 +machine_arch_initcall(warp, warp_arch_init);
 +
  static int __init warp_device_probe(void)
  {
        of_platform_bus_probe(NULL, warp_of_bus, NULL);

@@ -52,61 +68,232 @@ define_machine(warp) {

};

-#define LED_GREEN (0x80000000 >> 0)
-#define LED_RED (0x80000000 >> 1)
+/* I am not sure this is the best place for this... */ +static int __init warp_post_info(void)
+{

 +       struct device_node *np;
 +       void __iomem *fpga;
 +       u32 post1, post2;
 +
 +       /* Sighhhh... POST information is in the sd area. */
 +       np = of_find_compatible_node(NULL, NULL, "pika,fpga-sd");
 +       if (np == NULL)
 +               return -ENOENT;
 +
 +       fpga = of_iomap(np, 0);
 +       of_node_put(np);
 +       if (fpga == NULL)

+ return -ENOENT;
+
+ post1 = in_be32(fpga + 0x40);
+ post2 = in_be32(fpga + 0x44);
+
+ iounmap(fpga);
+
+ if (post1 || post2)
+ printk(KERN_INFO "Warp POST %08x %08x\n", post1, post2);
+ else
+ printk(KERN_INFO "Warp POST OK\n");
+
+ return 0;
+}
+machine_late_initcall(warp, warp_post_info);
+
+
+#ifdef CONFIG_SENSORS_AD7414

+#else /* !CONFIG_SENSORS_AD7414 */

arch/powerpc/platforms/44x/warp-nand.c +2 -0

+
 +int dtm_register_shutdown(void (*func)(void *arg), void *arg)
 +{
 +       return 0;
 +}
 
+
 +int dtm_unregister_shutdown(void (*func)(void *arg), void *arg)

You need to add the gpio-controller and #gpio-cells properties to the GPIO nodes for the LED's gpios property to work correctly. Search for "2) gpio-controller nodes" in Documentation/powerpc/booting-without-of.txt for details. #gpio-cells should probably be '2' for this gpio controller; 1 cell for the gpio pin and 1 cell for flags.

These should not be children of the soc node (they are not part of the SoC internal bus). However, I think it would be perfectly valid to make them children of the gpio node since they don't have any connections to other device on the platform.

Why is this information in the dts *and* the platform file? I haven't been following the flash partition map binding conventions, but having it in both places looks wrong....

oh, wait... the one in the dts is for NOR and this one is for NAND, right? And we don't have a binding yet for NAND partitions yet, correct?

When exporting symbols for platform code you should avoid polluting the global Linux namespace and prefix the functions with your platform name.

Quoted 7 lines

Why is this information in the dts *and* the platform file? I haven't been following the flash partition map binding conventions, but having it in both places looks wrong....

oh, wait... the one in the dts is for NOR and this one is for NAND, right? And we don't have a binding yet for NAND partitions yet, correct?

Correct.

Why can't the existing partition binding be used with NAND? It's what we do with Freescale FCM NAND.

Quoted 2 lines · On Mon, 28 Apr 2008 16:54:24 -0500 Scott Wood <[email protected]> wrote:

Why can't the existing partition binding be used with NAND? It's what we do with Freescale FCM NAND.

I guess I could put the partitions in the dts. But I would have to read them and dynamically create an array to pass to the ndfc driver.

It seems simpler to just statically initialize the array. Once the ndfc is modified to use the dts, I will switch to that method.

On Mon, 28 Apr 2008 13:56:11 -0600

Quoted 8 lines · Grant Likely

You need to add the gpio-controller and #gpio-cells properties to the GPIO nodes for the LED's gpios property to work correctly. Search for "2) gpio-controller nodes" in Documentation/powerpc/booting-without-of.txt for details. #gpio-cells should probably be '2' for this gpio controller; 1 cell for the gpio pin and 1 cell for flags.

I believe these gpio nodes predate that text, but I added the fields anyway.

Quoted 5 lines

These should not be children of the soc node (they are not part of the SoC internal bus). However, I think it would be perfectly valid to make them children of the gpio node since they don't have any connections to other device on the platform.

I put them in gpio. That was where I put them initialy.

Quoted 7 lines

Why is this information in the dts *and* the platform file? I haven't been following the flash partition map binding conventions, but having it in both places looks wrong....

oh, wait... the one in the dts is for NOR and this one is for NAND, right? And we don't have a binding yet for NAND partitions yet, correct?

Correct. Josh originally asked me to split out the warp-nand.c file so that once the NAND is in the dts, we can just delete the file. NAND is much more complicated that NOR to configure.

Quoted 3 lines

When exporting symbols for platform code you should avoid polluting the global Linux namespace and prefix the functions with your platform name.

I was hoping dtm was good enough. I prefixed them with the company name. We are expecting to have a "family" of Asterisk appliances and I am trying to make educated guesses as to what will be family wide (prefixed with pika) and what will be warp specific.

@@ -186,18 +179,45 @@

                        GPIO1: gpio@ef600c00 {
                                compatible = "ibm,gpio-440ep";
                                reg = <ef600c00 48>;

+ #gpio-cells = <2>;
+ gpio-controller;
+
+ led@31 {

+ compatible = "linux,gpio-led";
+ linux,name = ":green:";
+ gpios = <&GPIO1 31>;

+ };
+
+ led@30 {

 +                                       compatible = "linux,gpio-led";
 +                                       linux,name = ":red:";
 +                                       gpios = <&GPIO1 30>;

+ };

                        };

It doesn't help that both of these patches have the same subject line, nor that it starts with "Re:". :(

Also, I find the statement "A change to the dts to get gpios correct broke the led code" a bit opaque. It doesn't tell me in what way it was broken or what this patch does to correct, or even what the change was in enough detail that I could find the change in the git repository.