From mboxrd@z Thu Jan  1 00:00:00 1970
From: Paul Burton <paul.burton@imgtec.com>
Date: Thu, 24 Oct 2013 10:47:04 +0100
Subject: [U-Boot] [PATCH 09/15] malta: support for coreFPGA6 boards
In-Reply-To: <CACUy__WUTqns8qGMQS5DyVsEaFagLMi5uf1hQXkE9Dy+cUjn8A@mail.gmail.com>
References: <1382522885-29524-1-git-send-email-paul.burton@imgtec.com>	<1382522885-29524-10-git-send-email-paul.burton@imgtec.com>
	<CACUy__WUTqns8qGMQS5DyVsEaFagLMi5uf1hQXkE9Dy+cUjn8A@mail.gmail.com>
Message-ID: <5268EC98.7070305@imgtec.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

>> 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@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