From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Roese Date: Fri, 24 Jul 2009 14:50:20 +0200 Subject: [U-Boot] [PATCH] mpc83xx: Add esd VME8349 board support In-Reply-To: <20090721140825.dc2fbd16.kim.phillips@freescale.com> References: <1244653788-699-1-git-send-email-sr@denx.de> <200907211138.31711.sr@denx.de> <20090721140825.dc2fbd16.kim.phillips@freescale.com> Message-ID: <200907241450.20165.sr@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 ', 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 =====================================================================