2013/10/24 Paul Burton <[email protected]>: >>> diff --git a/drivers/pci/pci_msc01.c b/drivers/pci/pci_msc01.c >>> new file mode 100644 >>> index 0000000..7904378 >>> --- /dev/null >>> +++ b/drivers/pci/pci_msc01.c >>> @@ -0,0 +1,126 @@ >>> +/* >>> + * Copyright (C) 2013 Imagination Technologies >>> + * Author: Paul Burton <[email protected]> >>> + * >>> + * SPDX-License-Identifier: GPL-2.0+ >>> + */ >>> + >>> +#include <common.h> >>> +#include <msc01.h> >>> +#include <pci.h> >>> +#include <pci_msc01.h> >>> +#include <asm/io.h> >>> + >>> +#define PCI_ACCESS_READ 0 >>> +#define PCI_ACCESS_WRITE 1 >>> + >>> +struct msc01_pci_controller { >>> + struct pci_controller hose; >>> + void *base; >>> +}; >>> + >>> +static inline struct msc01_pci_controller * >>> +hose_to_msc01(struct pci_controller *hose) >>> +{ >>> + return container_of(hose, struct msc01_pci_controller, hose); >>> +} >>> + >>> +static int msc01_config_access(struct msc01_pci_controller *msc01, >>> + unsigned char access_type, pci_dev_t bdf, >>> + int where, u32 *data) >>> +{ >>> + const u32 aborts = MSC01_PCI_INTSTAT_MA_MSK | >>> MSC01_PCI_INTSTAT_TA_MSK; >>> + void *intstat = msc01->base + MSC01_PCI_INTSTAT_OFS; >>> + void *cfgdata = msc01->base + MSC01_PCI_CFGDATA_OFS; >>> + unsigned int bus = PCI_BUS(bdf); >>> + unsigned int dev = PCI_DEV(bdf); >>> + unsigned int devfn = PCI_DEV(bdf) << 3 | PCI_FUNC(bdf); >>> + u32 status; >> >> >> gcc-4.8 shows a warning: >> >> pci_msc01.c: In function 'msc01_config_access': >> pci_msc01.c:38:6: warning: unused variable 'status' [-Wunused-variable] >> u32 status; >> ^ > > > Right you are. > > >> >> >>> + >>> + /* clear abort status */ >>> + __raw_writel(aborts, intstat); >>> + >>> + /* setup address */ >>> + __raw_writel((bus << MSC01_PCI_CFGADDR_BNUM_SHF) | >>> + (dev << MSC01_PCI_CFGADDR_DNUM_SHF) | >>> + (devfn << MSC01_PCI_CFGADDR_FNUM_SHF) | >>> + ((where / 4) << MSC01_PCI_CFGADDR_RNUM_SHF), >>> + msc01->base + MSC01_PCI_CFGADDR_OFS); >> >> >> Contrary to the kernel U-Boot code must not use base + offset in IO >> primitives. Registers should be implemented with a struct. >> For example: >> >> struct foobar_regs { >> u32 foo; >> u32 bar; >> }; >> >> struct foobar_regs *regs = (struct foobar_regs *)CKSEG1ADDR(FOOBAR_BASE); >> u32 val = __raw_readl(®s->foo); > > > Could you point me to somewhere stating that? (and why?) I can't find > anything in README and I can see plenty of code using base+offset already in > U-boot. In this case the assembly in lowlevel_init.S has to access various > registers so using the offsets in C means I don't need to duplicate the > information in both struct & offset macro forms. I don't mind too much if > it's a rule but I'd like to see some justification/reasoning before I change > this. > > Thanks for the review, > Paul >
I primarily know it form many review comments by Wolfgang Denk. Some references I've found in ML archives: [1] http://u-boot.10912.n7.nabble.com/U-Boot-structure-usage-for-peripheral-registers-tc37050.html#none [2] http://u-boot.10912.n7.nabble.com/U-Boot-PATCH-v5-0-8-those-series-patches-for-add-ben-nanonote-board-tc17476.html#none @Wolfgang, could you comment please? Also this requirement should be documented in README or Wiki, shouldn't it? -- Best regards, Daniel _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
