Hi Huacai,
On Sat, Sep 15, 2018 at 02:01:13PM +0800, Huacai Chen wrote:
> Signed-off-by: Huacai Chen <chenhc@xxxxxxxxxx>
> ---
> arch/mips/pci/ops-loongson3.c | 29 +++++++++++++++++++++--------
> 1 file changed, 21 insertions(+), 8 deletions(-)
Please can you add a commit message, even if it's brief.
> diff --git a/arch/mips/pci/ops-loongson3.c b/arch/mips/pci/ops-loongson3.c
> index 9e11843..3100117 100644
> --- a/arch/mips/pci/ops-loongson3.c
> +++ b/arch/mips/pci/ops-loongson3.c
> @@ -24,16 +24,29 @@ static int loongson3_pci_config_access(unsigned char access_type,
> int function = PCI_FUNC(devfn);
> int reg = where & ~3;
>
> - addr = (busnum << 16) | (device << 11) | (function << 8) | reg;
> - if (busnum == 0) {
> - if (device > 31)
> + if (where < 256) { /* standard config */
This can be PCI_CFG_SPACE_SIZE instead of the magic 256 number, to make
it clearer what's going on.
> + addr = (busnum << 16) | (device << 11) | (function << 8) | reg;
> + if (busnum == 0) {
> + if (device > 31)
> + return PCIBIOS_DEVICE_NOT_FOUND;
> + addrp = (void *)(TO_UNCAC(HT1LO_PCICFG_BASE) | (addr & 0xffff));
I know this is existing code, but we could clean up some unnecessary
brackets whilst we're here:
addrp = (void *)TO_UNCAC(HT1LO_PCICFG_BASE | (addr & 0xffff));
Actually we can go further than that - the only thing in bits 16 & above
of addr is busnum & we already know it's zero, so we don't need to mask
addr at all & this line can be simplified to:
addrp = (void *)TO_UNCAC(HT1LO_PCICFG_BASE | addr);
> + type = 0;
Is the type variable ever used? It looks unused both in the existing
code & after this patch - can we just remove it?
> + } else {
> + addrp = (void *)(TO_UNCAC(HT1LO_PCICFG_BASE_TP1) | (addr));
> + type = 0x10000;
Similar comments to above - we could drop some brackets & perhaps drop
the type variable.
> + }
> + } else { /* extended config */
Should this be "} else if (where < PCI_CFG_SPACE_EXP_SIZE) {"?
> + struct pci_dev *rootdev;
> +
> + rootdev = pci_get_domain_bus_and_slot(0, 0, 0);
> + if (!rootdev)
> + return PCIBIOS_DEVICE_NOT_FOUND;
> +
> + addr = pci_resource_start(rootdev, 3);
> + if (!addr)
> return PCIBIOS_DEVICE_NOT_FOUND;
> - addrp = (void *)(TO_UNCAC(HT1LO_PCICFG_BASE) | (addr & 0xffff));
> - type = 0;
>
> - } else {
> - addrp = (void *)(TO_UNCAC(HT1LO_PCICFG_BASE_TP1) | (addr));
> - type = 0x10000;
> + addrp = (void *)TO_UNCAC(addr | busnum << 20 | device << 15 | function << 12 | reg);
> }
>
> if (access_type == PCI_ACCESS_WRITE)
> --
> 2.7.0
Thanks,
Paul
