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

From: Marian Balakowicz <[email-masked]>
Date: Wed, 17 Oct 2007 13:24:38 +0200

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.

>> +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.

>> +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.