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

[Stefan Roese]

On Tuesday 21 July 2009 21:08:25 Kim Phillips wrote:
> > > > 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?

OK, "typedefs" removed in next patch version.

> > > > 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?

No. From what I know, this code in question is for some earlier MPC834x chip 
revisions. The comment in the code also states something like this:

		/*
		 * The host/PCI bridge 1 seems broken in 8349 - it presents
		 * itself as 'PCI_CLASS_BRIDGE_OTHER' and appears as an _agent_
		 * device claiming resources io/mem/irq.. we only allow for
		 * the PIMMR window to be allocated (BAR0 - 1MB size)
		 */

You will probably know this better. If this assumption is correct, it would be 
best to check for chip revisions and only enable this code for those "buggy" 
revisions.

Since I can't really tell for sure, and I don't want to change any other 83xx 
systems, I'll keep

#if defined(CONFIG_MPC834x) && !defined(CONFIG_VME8349)

for now. Otherwise our PCI devices won't get enumerated correctly (e.g. PCI 
devices with certain PLX bridges).

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================