Hi, Paul,
I will improve this patch, and could you please read my new comments
on the VDSO patch?
Thanks,
Huacai
On Wed, Sep 19, 2018 at 7:17 AM, Paul Burton <paul.burton@xxxxxxxx> wrote:
> 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
