Re: [PATCH 3/5] hw/pci-host/bonito: Allow PCI config accesses smaller than 32-bit

[Philippe Mathieu-Daudé]

On 6/24/21 10:49 PM, BALATON Zoltan wrote:
> On Thu, 24 Jun 2021, Philippe Mathieu-Daudé wrote:
>> When running the official PMON firmware for the Fuloong 2E, we see
>> 8-bit and 16-bit accesses to PCI config space:
>>
>>  $ qemu-system-mips64el -M fuloong2e -bios pmon_2e.bin \
>>    -trace -trace bonito\* -trace pci_cfg\*
>>
>>  pci_cfg_write vt82c686b-pm 05:4 @0x90 <- 0xeee1
>>  bonito_spciconf_small_access PCI config address is smaller then
>> 32-bit, addr: 0x4d2, size: 2
>>  pci_cfg_write vt82c686b-pm 05:4 @0xd2 <- 0x1
>>  pci_cfg_write vt82c686b-pm 05:4 @0x4 <- 0x1
>>  pci_cfg_write vt82c686b-isa 05:0 @0x4 <- 0x7
>>  bonito_spciconf_small_access PCI config address is smaller then
>> 32-bit, addr: 0x81, size: 1
>>  pci_cfg_read vt82c686b-isa 05:0 @0x81 -> 0x0
>>  bonito_spciconf_small_access PCI config address is smaller then
>> 32-bit, addr: 0x81, size: 1
>>  pci_cfg_write vt82c686b-isa 05:0 @0x81 <- 0x80
>>  bonito_spciconf_small_access PCI config address is smaller then
>> 32-bit, addr: 0x83, size: 1
>>  pci_cfg_write vt82c686b-isa 05:0 @0x83 <- 0x89
>>  bonito_spciconf_small_access PCI config address is smaller then
>> 32-bit, addr: 0x85, size: 1
>>  pci_cfg_write vt82c686b-isa 05:0 @0x85 <- 0x3
>>  bonito_spciconf_small_access PCI config address is smaller then
>> 32-bit, addr: 0x5a, size: 1
>>  pci_cfg_write vt82c686b-isa 05:0 @0x5a <- 0x7
>>  bonito_spciconf_small_access PCI config address is smaller then
>> 32-bit, addr: 0x85, size: 1
>>  pci_cfg_write vt82c686b-isa 05:0 @0x85 <- 0x1
>>
>> Also this is what the Linux kernel does since it supports the Bonito
>> north bridge:
>> https://elixir.bootlin.com/linux/v2.6.15/source/arch/mips/pci/ops-bonito64.c#L85
>>
>>
>> So it seems safe to assume the datasheet is incomplete or outdated
>> regarding the address constraints.
>>
>> This problem was exposed by commit 911629e6d3773a8adeab48b
>> ("vt82c686: Fix SMBus IO base and configuration registers").
>>
>> Reported-by: BALATON Zoltan <balaton@eik.bme.hu>
>> Suggested-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> hw/pci-host/bonito.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
>> index 751fdcec689..3c10608c9a2 100644
>> --- a/hw/pci-host/bonito.c
>> +++ b/hw/pci-host/bonito.c
>> @@ -187,7 +187,7 @@ FIELD(BONGENCFG, PCIQUEUE,      12, 1)
>> #define BONITO_PCICONF_FUN_MASK        0x700    /* [10:8] */
>> #define BONITO_PCICONF_FUN_OFFSET      8
>> #define BONITO_PCICONF_REG_MASK_DS     (~3)         /* Per datasheet */
>> -#define BONITO_PCICONF_REG_MASK        0xFC
>> +#define BONITO_PCICONF_REG_MASK_HW     0xff         /* As seen on
>> hardware */
> 
> I think we didn't really see it on hardware just inferred this from what
> the firmware does. That's a slight difference but may worth noting so
> people later don't think this was really tested with real hardware.
> Maybe "As seen with PMON"?

OK.

> Also if this is a loongson thing as was
> thought in the thread in December then maybe the #define could be named
> that instead of _HW so if somebody wants to reuse this model later ad
> Bonito then know that it implements the Loongson version.

Bonito64 is what is modelled. This is what I checked from the Linux
kernel:
https://elixir.bootlin.com/linux/v2.6.15/source/arch/mips/pci/ops-bonito64.c#L85