Bug in Bonito? (mips/fuloong2e)

Bug in Bonito? (mips/fuloong2e)

[BALATON Zoltan via]

Hello,

While continuing with part two of my vt82c686b clean ups I've tried to 
implement SMBus IO base configuration in the vt82c686b-pm part that I've 
already done for vt8231 for pegasos2 and it should be the same for 686B. 
(In short, writing address to pm config 0x90 sets base address of smbus 
regs and bit 0 of 0xd2 enables/disables it.) This is what the firmware 
does first and it would allow removing hard coded 0xeee1 value and the 
property to set it and then I could reuse the same PM part in VT8231.

I have code to implement this and it works with pegasos2 but fails with 
fuloong2e and pmon. I've debugged it that write to 0xd2 comes out as 0xd0 
after some mapping in bonito:

bonito_spciconf_write: bonito_spciconf_write 0000000000000490 size 4 val eee1

bonito_sbridge_pciaddr: cfgaddr 10490 pciaddr 2c90 busno 0 devno 5 funno 4 regno 144

pci_cfg_write vt82c686b-pm 05:4 @0x90 <- 0xeee1

via_pm_write addr 0x90 val 0xeee1 len 0x4


bonito_spciconf_write: bonito_spciconf_write 00000000000004d2 size 2 val 1

bonito_sbridge_pciaddr: cfgaddr 104d2 pciaddr 2cd0 busno 0 devno 5 funno 4 regno 208

pci_cfg_write vt82c686b-pm 05:4 @0xd0 <- 0x1

via_pm_write addr 0xd0 val 0x1 len 0x2

Note the first write to 0x90 is correct but the second ends up at 0xd0 
instead of 0xd2 after bonito_sbridge_pciaddr(). This is somewhere here:

https://git.qemu.org/?p=qemu.git;a=blob;f=hw/pci-host/bonito.c;h=a99eced06574f999f3f1b999576ae09d5f4b06ca;hb=HEAD#l446

Any idea what this is trying to do and how to fix it?

Regards,
BALATON Zoltan

Re: Bug in Bonito? (mips/fuloong2e)

[Jiaxun Yang]

在 2020/12/29 上午11:26, BALATON Zoltan 写道:
> Hello,
>
> While continuing with part two of my vt82c686b clean ups I've tried to 
> implement SMBus IO base configuration in the vt82c686b-pm part that 
> I've already done for vt8231 for pegasos2 and it should be the same 
> for 686B. (In short, writing address to pm config 0x90 sets base 
> address of smbus regs and bit 0 of 0xd2 enables/disables it.) This is 
> what the firmware does first and it would allow removing hard coded 
> 0xeee1 value and the property to set it and then I could reuse the 
> same PM part in VT8231.
>
[...]
>
> Any idea what this is trying to do and how to fix it?

It's trying to translate Bonito style PCI config space r/w to standard PCI
config space R/W.

A quick galance told me change BONITO_PCICONF_REG_MASK to 0xff
may help.

Thanks.

- Jiaxun

>
> Regards,
> BALATON Zoltan

Re: Bug in Bonito? (mips/fuloong2e)

[Philippe Mathieu-Daudé]

On 12/29/20 6:26 AM, Jiaxun Yang wrote:
> 在 2020/12/29 上午11:26, BALATON Zoltan 写道:
>> Hello,
>>
>> While continuing with part two of my vt82c686b clean ups I've tried to
>> implement SMBus IO base configuration in the vt82c686b-pm part that
>> I've already done for vt8231 for pegasos2 and it should be the same
>> for 686B. (In short, writing address to pm config 0x90 sets base
>> address of smbus regs and bit 0 of 0xd2 enables/disables it.) This is
>> what the firmware does first and it would allow removing hard coded
>> 0xeee1 value and the property to set it and then I could reuse the
>> same PM part in VT8231.
>>
> [...]
>>
>> Any idea what this is trying to do and how to fix it?
> 
> It's trying to translate Bonito style PCI config space r/w to standard PCI
> config space R/W.
> 
> A quick galance told me change BONITO_PCICONF_REG_MASK to 0xff
> may help.

Per the datasheet section "5.7.5. Accessing PCI configuration space"
0xfc is the correct value, but the register number starts at the 2nd
bit. So this is not a write access to register 0xd2 but 0x34?

If you can test, this is the snippet I plan to send later:

-- >8 --
diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
index a99eced0657..65953766dd0 100644
--- a/hw/pci-host/bonito.c
+++ b/hw/pci-host/bonito.c
@@ -189,3 +189,3 @@ FIELD(BONGENCFG, PCIQUEUE,      12, 1)
 #define BONITO_PCICONF_REG_MASK        0xFC
-#define BONITO_PCICONF_REG_OFFSET      0
+#define BONITO_PCICONF_REG_OFFSET      2
---

> 
> Thanks.
> 
> - Jiaxun
> 
>>
>> Regards,
>> BALATON Zoltan
> 
> 

Re: Bug in Bonito? (mips/fuloong2e)

[BALATON Zoltan via]

[-- Attachment #1: Type: text/plain, Size: 3174 bytes --]

On Tue, 29 Dec 2020, Philippe Mathieu-Daudé wrote:
> On 12/29/20 6:26 AM, Jiaxun Yang wrote:
>> 在 2020/12/29 上午11:26, BALATON Zoltan 写道:
>>> Hello,
>>>
>>> While continuing with part two of my vt82c686b clean ups I've tried to
>>> implement SMBus IO base configuration in the vt82c686b-pm part that
>>> I've already done for vt8231 for pegasos2 and it should be the same
>>> for 686B. (In short, writing address to pm config 0x90 sets base
>>> address of smbus regs and bit 0 of 0xd2 enables/disables it.) This is
>>> what the firmware does first and it would allow removing hard coded
>>> 0xeee1 value and the property to set it and then I could reuse the
>>> same PM part in VT8231.
>>>
>> [...]
>>>
>>> Any idea what this is trying to do and how to fix it?
>>
>> It's trying to translate Bonito style PCI config space r/w to standard PCI
>> config space R/W.
>>
>> A quick galance told me change BONITO_PCICONF_REG_MASK to 0xff
>> may help.

With Jiaxun's change it works better:

bonito_spciconf_write: bonito_spciconf_write 0000000000000490 size 4 val eee1
bonito_sbridge_pciaddr: cfgaddr 10490 pciaddr 2c90 busno 0 devno 5 funno 4 regno 144
pci_cfg_write vt82c686b-pm 05:4 @0x90 <- 0xeee1
via_pm_write addr 0x90 val 0xeee1 len 0x4

bonito_writel: bonito_writel 0000000000000018 val 1 saddr 6
bonito_readl: bonito_readl 0000000000000018

bonito_spciconf_write: bonito_spciconf_write 00000000000004d2 size 2 val 1
bonito_sbridge_pciaddr: cfgaddr 104d2 pciaddr 2cd2 busno 0 devno 5 funno 4 regno 210
pci_cfg_write vt82c686b-pm 05:4 @0xd2 <- 0x1
via_pm_write addr 0xd2 val 0x1 len 0x2
*** Should update smbus io base to eee1, enable

(last line is my debug message). Not sure if this breaks anyhing else but 
PMON still seems to work (tested with pmon_2e.bin).

> Per the datasheet section "5.7.5. Accessing PCI configuration space"
> 0xfc is the correct value, but the register number starts at the 2nd
> bit. So this is not a write access to register 0xd2 but 0x34?
>
> If you can test, this is the snippet I plan to send later:
>
> -- >8 --
> diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
> index a99eced0657..65953766dd0 100644
> --- a/hw/pci-host/bonito.c
> +++ b/hw/pci-host/bonito.c
> @@ -189,3 +189,3 @@ FIELD(BONGENCFG, PCIQUEUE,      12, 1)
> #define BONITO_PCICONF_REG_MASK        0xFC
> -#define BONITO_PCICONF_REG_OFFSET      0
> +#define BONITO_PCICONF_REG_OFFSET      2
> ---

This does not seem to work, I get all mixed up regs:

bonito_spciconf_write: bonito_spciconf_write 0000000000000490 size 4 val eee1
bonito_sbridge_pciaddr: cfgaddr 10490 pciaddr 2c24 busno 0 devno 5 funno 4 regno 36
pci_cfg_write vt82c686b-pm 05:4 @0x24 <- 0xeee1
via_pm_write addr 0x24 val 0xeee1 len 0x4

bonito_writel: bonito_writel 0000000000000018 val 1 saddr 6
bonito_readl: bonito_readl 0000000000000018

bonito_spciconf_write: bonito_spciconf_write 00000000000004d2 size 2 val 1
bonito_sbridge_pciaddr: cfgaddr 104d2 pciaddr 2c34 busno 0 devno 5 funno 4 regno 52
pci_cfg_write vt82c686b-pm 05:4 @0x34 <- 0x1
via_pm_write addr 0x34 val 0x1 len 0x2

So I'll use the first one for now, that allows me to continue with vt82c686b.

Regards,
BALATON Zoltan

Re: Bug in Bonito? (mips/fuloong2e)

[Jiaxun Yang]

在 2020/12/29 18:47, Philippe Mathieu-Daudé 写道:
> On 12/29/20 6:26 AM, Jiaxun Yang wrote:
>> 在 2020/12/29 上午11:26, BALATON Zoltan 写道:
>>> Hello,
>>>
>>> While continuing with part two of my vt82c686b clean ups I've tried to
>>> implement SMBus IO base configuration in the vt82c686b-pm part that
>>> I've already done for vt8231 for pegasos2 and it should be the same
>>> for 686B. (In short, writing address to pm config 0x90 sets base
>>> address of smbus regs and bit 0 of 0xd2 enables/disables it.) This is
>>> what the firmware does first and it would allow removing hard coded
>>> 0xeee1 value and the property to set it and then I could reuse the
>>> same PM part in VT8231.
>>>
>> [...]
>>> Any idea what this is trying to do and how to fix it?
>> It's trying to translate Bonito style PCI config space r/w to standard PCI
>> config space R/W.
>>
>> A quick galance told me change BONITO_PCICONF_REG_MASK to 0xff
>> may help.
> Per the datasheet section "5.7.5. Accessing PCI configuration space"
> 0xfc is the correct value, but the register number starts at the 2nd
> bit. So this is not a write access to register 0xd2 but 0x34?

It seems like Loongson changed defination of the register?
There is no shifting in kernel[1] as well.

```
/* Type 1 configuration for offboard PCI bus */
addr = (busnum << 16) | (device << 11) | (function << 8) | reg;
```

Thanks.

[1]: 
https://elixir.bootlin.com/linux/latest/source/arch/mips/pci/ops-loongson2.c

- Jiaxun
>
> If you can test, this is the snippet I plan to send later:
>
> -- >8 --
> diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
> index a99eced0657..65953766dd0 100644
> --- a/hw/pci-host/bonito.c
> +++ b/hw/pci-host/bonito.c
> @@ -189,3 +189,3 @@ FIELD(BONGENCFG, PCIQUEUE,      12, 1)
>   #define BONITO_PCICONF_REG_MASK        0xFC
> -#define BONITO_PCICONF_REG_OFFSET      0
> +#define BONITO_PCICONF_REG_OFFSET      2
> ---
>
>> Thanks.
>>
>> - Jiaxun
>>
>>> Regards,
>>> BALATON Zoltan
>>

Re: Bug in Bonito? (mips/fuloong2e)

[Philippe Mathieu-Daudé]

On 12/29/20 2:02 PM, Jiaxun Yang wrote:
> 在 2020/12/29 18:47, Philippe Mathieu-Daudé 写道:
>> On 12/29/20 6:26 AM, Jiaxun Yang wrote:
>>> 在 2020/12/29 上午11:26, BALATON Zoltan 写道:
>>>> Hello,
>>>>
>>>> While continuing with part two of my vt82c686b clean ups I've tried to
>>>> implement SMBus IO base configuration in the vt82c686b-pm part that
>>>> I've already done for vt8231 for pegasos2 and it should be the same
>>>> for 686B. (In short, writing address to pm config 0x90 sets base
>>>> address of smbus regs and bit 0 of 0xd2 enables/disables it.) This is
>>>> what the firmware does first and it would allow removing hard coded
>>>> 0xeee1 value and the property to set it and then I could reuse the
>>>> same PM part in VT8231.
>>>>
>>> [...]
>>>> Any idea what this is trying to do and how to fix it?
>>> It's trying to translate Bonito style PCI config space r/w to
>>> standard PCI
>>> config space R/W.
>>>
>>> A quick galance told me change BONITO_PCICONF_REG_MASK to 0xff
>>> may help.
>> Per the datasheet section "5.7.5. Accessing PCI configuration space"
>> 0xfc is the correct value, but the register number starts at the 2nd
>> bit. So this is not a write access to register 0xd2 but 0x34?
> 
> It seems like Loongson changed defination of the register?

Maybe, I only have the bonito64 specs, not the Loongson2 ones.
I am a bit confused, I thought the Fuloong 2E was based on
bonito64 (which QEMU models).

Do you know if the Loongson2 specs are public?

> There is no shifting in kernel[1] as well.
> 
> ```
> /* Type 1 configuration for offboard PCI bus */
> addr = (busnum << 16) | (device << 11) | (function << 8) | reg;
> ```

OK, this makes sense after looking at Linux kernel commit e2fee5723bbd
("MIPS: Bonito64: Make Loongson independent from Bonito64 code.") [2]

I'm a bit reluctant to modify hw/pci-host/bonito.c to make Loongson2
works without having the specs handy, justifying simply because
"Linux uses it that way".

OTOH it is pointless to maintain a model that has no users (thinking
about not breaking the bonito64 model).

[2]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e2fee5723bbd

> 
> Thanks.
> 
> [1]:
> https://elixir.bootlin.com/linux/latest/source/arch/mips/pci/ops-loongson2.c
> 
> 
> - Jiaxun
>>
>> If you can test, this is the snippet I plan to send later:
>>
>> -- >8 --
>> diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
>> index a99eced0657..65953766dd0 100644
>> --- a/hw/pci-host/bonito.c
>> +++ b/hw/pci-host/bonito.c
>> @@ -189,3 +189,3 @@ FIELD(BONGENCFG, PCIQUEUE,      12, 1)
>>   #define BONITO_PCICONF_REG_MASK        0xFC
>> -#define BONITO_PCICONF_REG_OFFSET      0
>> +#define BONITO_PCICONF_REG_OFFSET      2
>> ---
>>
>>> Thanks.
>>>
>>> - Jiaxun
>>>
>>>> Regards,
>>>> BALATON Zoltan
>>>
> 
>