[U-Boot] [PATCH 09/15] malta: support for coreFPGA6 boards
Paul Burton
paul.burton at imgtec.com
Thu Oct 24 11:47:04 CEST 2013
>> 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.burton at 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(®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
More information about the U-Boot
mailing list