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

[Daniel Schwierzeck]

2013/10/24 Paul Burton <paul.burton at imgtec.com>:
>>> 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(&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
>

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