[U-Boot] [PATCH] mpc83xx: Add esd VME8349 board support

[Kim Phillips]

On Tue, 21 Jul 2009 11:38:31 +0200
Stefan Roese <sr at denx.de> wrote:

> > might want to add a doc/README.vme8349 at some point.
> 
> Currently not planned. I have to admit that I usually don't add such a readme. 
> It's not an evaluation board after all.

your call - the board will be easier to maintain provided the intended
memory map, etc.

> > > diff --git a/board/esd/vme8349/caddy.h b/board/esd/vme8349/caddy.h
> > >
> > > +typedef enum {
> > > +	CADDY_CMD_IO_READ_8,
> > > +	CADDY_CMD_IO_READ_16,
> > > +	CADDY_CMD_IO_READ_32,
> > > +	CADDY_CMD_IO_WRITE_8,
> > > +	CADDY_CMD_IO_WRITE_16,
> > > +	CADDY_CMD_IO_WRITE_32,
> > > +	CADDY_CMD_CONFIG_READ_8,
> > > +	CADDY_CMD_CONFIG_READ_16,
> > > +	CADDY_CMD_CONFIG_READ_32,
> > > +	CADDY_CMD_CONFIG_WRITE_8,
> > > +	CADDY_CMD_CONFIG_WRITE_16,
> > > +	CADDY_CMD_CONFIG_WRITE_32,
> > > +} CADDY_CMDS;
> > > +
> > > +
> > > +typedef struct {
> > > +	uint32_t cmd;
> > > +	uint32_t issue;
> > > +	uint32_t addr;
> > > +	uint32_t par[5];
> > > +} CADDY_CMD;
> > > +
> > > +typedef struct {
> > > +	uint32_t answer;
> > > +	uint32_t issue;
> > > +	uint32_t status;
> > > +	uint32_t par[5];
> > > +} CADDY_ANSWER;
> > > +
> > > +typedef struct {
> > > +	uint8_t  magic[16];
> > > +	uint32_t cmd_in;
> > > +	uint32_t cmd_out;
> > > +	uint32_t heartbeat;
> > > +	uint32_t reserved1;
> > > +	CADDY_CMD cmd[CMD_SIZE];
> > > +	uint32_t answer_in;
> > > +	uint32_t answer_out;
> > > +	uint32_t reserved2;
> > > +	uint32_t reserved3;
> > > +	CADDY_ANSWER answer[CMD_SIZE];
> > > +} CADDY_INTERFACE;
> >
> > please remove all typedefs (see CodingStyle Ch. 5).  Use 'struct xxx'
> > in each instance instead.
> 
> We really would like to keep these typedefs. The reason for this is that 
> multiple customers already are using this header for their work. And 
> maintaining multiple versions of this file doesn't sound like a good idea.

eh? It's a straight violation of CodingStyle and makes the code
hard to read; STUFF_IN_CAPS to me read as defines, and anyway,
typedefs, assuming CodingStyle liked them, would be appended with _t.
But these need to be defined as 'struct <name>', and used in such a way.

Can't they write a header wrapper for "their work"?  Can you make them
realize they won't need to be wasting time on such effort if they
submit the remainder of their code upstream?

> > > diff --git a/drivers/pci/pci_auto.c b/drivers/pci/pci_auto.c
> > > index c20b981..393e44d 100644
> > > --- a/drivers/pci/pci_auto.c
> > > +++ b/drivers/pci/pci_auto.c
> > > @@ -403,6 +403,7 @@ int pciauto_config_device(struct pci_controller
> > > *hose, pci_dev_t dev) PCI_DEV(dev));
> > >  		break;
> > >  #endif
> > > +#ifndef CONFIG_VME8349
> > >  #ifdef CONFIG_MPC834X
> >
> > #if defined(CONFIG_MPC834x) && !defined(CONFIG_VME8349)
> >
> > I don't know - this might need to be changed to ifdef
> > CONFIG_MPC8349EMDS...
> 
> Should I change this to CONFIG_MPC8349EMDS? Or use
> 
> #if defined(CONFIG_MPC834x) && !defined(CONFIG_VME8349)
> 
> for now?

hmm...based on commit 6902df56a0b493f369153b09d11afcd74a580561 "Add PCI
support for the TQM834x board.", it should really be ifdef
CONFIG_TQM834x...but what does the VME8349 do?  does it want to define
CONFIG_PCIAUTO_SKIP_HOST_BRIDGE instead?

Kim