Re: [POWERPC 03/15] [POWERPC] TQM5200 board support

Grant Likely wrote:

Both this patch and the CM5200 support patch (#6 in your series) are
pretty much clones of lite5200.c.  I don't think this is the right
approach.  Don't duplicate code in this way.  Determine the common
bits and put them in a common place to be usable by any 5200 board
port.

It might even be better just to add a platform that matches on
compatible='mpc5200-generic' which is usable for mpc5200 boards that
don't need any custom setup by the kernel at platform setup time.
(which will probably be most 5200 boards).

Agree, will try more generic approach.

quoted

+static void __init
+tqm5200_setup_cpu(void)
+{
+       struct mpc52xx_gpio __iomem *gpio;
+       u32 port_config;
+
+       /* Map zones */
+       gpio = mpc52xx_find_and_map("mpc5200-gpio");
+       if (!gpio) {
+               printk(KERN_ERR __FILE__ ": "
+                       "Error while mapping GPIO register for port config. "
+                       "Expect some abnormal behavior\n");
+               goto error;
+       }
+
+       /* Set port config */
+       port_config = in_be32(&gpio->port_config);
+
+       port_config &= ~0x00800000;     /* 48Mhz internal, pin is GPIO  */
+
+       port_config &= ~0x00007000;     /* USB port : Differential mode */
+       port_config |=  0x00001000;     /*            USB 1 only        */
+
+       port_config &= ~0x03000000;     /* ATA CS is on csb_4/5         */
+       port_config |=  0x01000000;

Are you *sure* you want this?  You should only be touching port_config
if firmware fails to set it up correctly.  Don't blindly copy what was
done for the lite5200.

Lite5200 touches it because firmware does *not* do the right thing at
the moment.

Yes, that's needed, but will be moved to U-boot.

quoted

+void tqm5200_show_cpuinfo(struct seq_file *m)
+{
+       struct device_node* np = of_find_all_nodes(NULL);
+       const char *model = NULL;
+
+       if (np)
+               model = of_get_property(np, "model", NULL);
+
+       seq_printf(m, "vendor\t\t:      Freescale Semiconductor\n");

Freescale?  Really?

Well, not really...

Something like

        seq_printf(m, "Vendor\t\t: TQ Components\n");
        seq_printf(m, "Machine\t\t: %s\n", model);

and model set to 'tqc,tqm5200' would be more accurate but going for
compatible='mpc5200-generic' platform we may need to drop Vendor line
anyway.

m.