Re: [U-Boot] [PATCH 09/15] malta: support for coreFPGA6 boards

[Paul Burton]

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 <paul.bur...@imgtec.com>
+ *
+ * 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(&regs->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

_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot