[coreboot] PCIe Role-based error reporting

[Zoran Stojsavljevic]

> I tried your changes and it is just as I expected, the register is
read-only. It's always enabled (the spec seemed
> to indicate that all devices need to have that bit set to 1). I even
tried to disable it but it had no effect (read-only).

I knew that you'll do both use cases (if Role-based Error Reporting capability
(bit 15) is by default 0 or 1), especially in the case you discovered that
Capability Register is RO.

Conclusion: INTEL PCIe implementation for this register follows the PCIe
3.0 specification revision/recommendation.

Problem solved. No need to involve anybody else. :-)

The only unsolved (aside) issue remains to be understood: *"I still fail to
see the link between RBER and ASPM".* I need to think about this more...
Later.

Zoran

On Thu, May 4, 2017 at 11:16 PM, Youness Alaoui <
kakaroto at kakaroto.homelinux.net> wrote:

> Hi Zoran,
>
> I tried your changes and it is just as I expected, the register is
> read-only. It's always enabled (the spec seemed to indicate that all
> devices need to have that bit set to 1). I even tried to disable it but it
> had no effect (read-only).
>
> The thing is that there is no RBER flag in the control register, there's
> just various error reporting flags (correctable, fatal, non-fatal and
> unsupported-request errors) and I think those 4 flags are the different
> 'roles' in the role-based error reporting. Since all PCIe devices since
> 1.0a need to support role-based error reporting, that value will always be
> set to 1. Trying to change it is useless since it's a read-only register. I
> still fail to see the link between RBER and ASPM.
>
> Duncan Laurie isn't the one who added that code by the way, if you look at
> the diff in the link you sent (same change as  https://review.coreboot.
> org/#/c/735), then you'll see that code was already there, Duncan simply
> put it inside a "if (apmc != PCIE_ASPM_NONE)" and removed the comment "TODO
> make this dependent on ASPM".
>
> Thanks,
>
> On Thu, May 4, 2017 at 8:42 AM, Zoran Stojsavljevic <
> zoran.stojsavljevic at gmail.com> wrote:
>
>> I thought more about this problem, and how it is easy to miss the
>> complete point?! :-(
>>
>> Revisited C code to be added (in *RED*):
>> /* Enable ASPM role based error reporting. */
>> devcap = pci_read_config32(endp, endp_cap + PCI_EXP_DEVCAP);
>> *// **#define PCI_EXP_DEVCAP_RBER
>> <http://lxr.free-electrons.com/ident?i=PCI_EXP_DEVCAP_RBER> 0x00008000 /*
>> Role-Based Error Reporting */*
>> *if (**devcap & PCI_EXP_DEVCAP_RBER) {*
>> *printk("Role-based Error Reporting capability in the DEVCAP (offset
>> 0x04) is initially ENABLED\n");*
>> *else*
>> *printk("Role-based Error Reporting capability in the DEVCAP (offset
>> 0x04) is initially DISABLED\n");*
>> *}*
>> devcap |= PCI_EXP_DEVCAP_RBER;
>> pci_write_config32(endp, endp_cap + PCI_EXP_DEVCAP, devcap);
>> *devcap = pci_read_config32(endp, endp_cap + PCI_EXP_DEVCAP);*
>> *if (**devcap & PCI_EXP_DEVCAP_RBER) {*
>> *printk("Role-based Error Reporting capability in the DEVCAP (offset
>> 0x04) is now ENABLED\n");*
>> *else*
>> *printk("Role-based Error Reporting capability in the DEVCAP (offset
>> 0x04) stayed DISABLED\n");*
>> *}*
>>
>> This will actually show (solve this mystery) it all! :-)
>>
>> Zoran
>> _______
>>
>> On Thu, May 4, 2017 at 11:26 AM, Zoran Stojsavljevic <
>> zoran.stojsavljevic at gmail.com> wrote:
>>
>>> > Is that an error ?
>>>
>>> I have looked into the document (BTW did some additional investigation)
>>> and tried to find the attributes explanation (found it on page 586):
>>>
>>> [image: Inline image 1]
>>>
>>> The first possibility (my best guess) is: PCH (particular one. please,
>>> note that) has numerous of so called HW straps (IO pins which are either
>>> strapped to level 0 or level 1). Depending upon what some (Very Important)
>>> particular IO is strapped, bit 15 (Role-based error-reporting in the
>>> DEVCAP (offset 0x04)) could be enabled or disabled... So I think this is
>>> what this explanation really does mean.
>>>
>>> I would strongly advise you to add one printk() here and recompile
>>> Coreboot, something like this:
>>> /* Enable ASPM role based error reporting. */
>>> devcap = pci_read_config32(endp, endp_cap + PCI_EXP_DEVCAP);
>>> // #define PCI_EXP_DEVCAP_RBER
>>> <http://lxr.free-electrons.com/ident?i=PCI_EXP_DEVCAP_RBER> 0x00008000 */*
>>> Role-Based Error Reporting */*
>>> *if (**devcap & PCI_EXP_DEVCAP_RBER) printk("Role-based Error Reporting
>>> capability in the DEVCAP (offset 0x04) is ENABLED\n");*
>>> devcap |= PCI_EXP_DEVCAP_RBER;
>>> pci_write_config32(endp, endp_cap + PCI_EXP_DEVCAP, devcap);
>>>
>>> Then, the Truth will reveal itself.
>>>
>>> The second possibility is that you are correct. It should be enabled in
>>> control register, which is RWable. But this is subject of the
>>> implementation by Duncan Laurie <dlaurie at google.com
>>> <http://www.coreboot.org/mailman/listinfo/coreboot>> in the following
>>> patch: https://mail.coreboot.org/pipermail/coreboot/2012-March/0686
>>> 90.html
>>>
>>> I'll include also involved (last web pointer is certainly a smoking gun,
>>> isn't it?) in this matter.
>>>
>>> Zoran
>>> _______
>>>
>>> On Wed, May 3, 2017 at 11:54 PM, Youness Alaoui <
>>> kakaroto at kakaroto.homelinux.net> wrote:
>>>
>>>> Hi,
>>>>
>>>> I'm looking at the src/device/pciexp_device.c file trying to understand
>>>> what it does and I've noticed this in pciexp_enable_aspm :
>>>> /* Enable ASPM role based error reporting. */
>>>> devcap = pci_read_config32(endp, endp_cap + PCI_EXP_DEVCAP);
>>>> devcap |= PCI_EXP_DEVCAP_RBER;
>>>> pci_write_config32(endp, endp_cap + PCI_EXP_DEVCAP, devcap);
>>>>
>>>> This looks like it tries to write a bit to 1 to enable Role-based
>>>> error-reporting in the DEVCAP (offset 0x04) in the PCIe capability, but
>>>> according to the spec (looking at page 612 of the spec version 3.0 that I
>>>> found here  : http://composter.com.ua/documents/PCI_Express_Base_Specif
>>>> ication_Revision_3.0.pdf), that structure is read-only (it's the
>>>> device capabilities register, not the device control register).
>>>> Is that an error ?
>>>> I tracked that code (moved, then moved again) all the way to this
>>>> change from 2011 : https://review.coreboot.org/#/c/735/ but for some
>>>> reason git blame can't find the file in commits before that.
>>>>
>>>> That code is probably harmless since it's a Read-only field anyways,
>>>> but I'm wondering if the code that should be there shouldn't instead be
>>>> enabling the bits 0:3 in the device control register to enable the error
>>>> reporting. Also, I don't see how this actually relates to ASPM (it only
>>>> gets called if ASPM is enabled).
>>>>
>>>> Does anyone know if I'm on the right track in my interpretation or if I
>>>> misunderstood the spec or what that code is meant to do ?
>>>>
>>>> Thanks,
>>>> Youness.
>>>>
>>>>
>>>> --
>>>> coreboot mailing list: coreboot at coreboot.org
>>>> https://mail.coreboot.org/mailman/listinfo/coreboot
>>>>
>>>
>>>
>>
>> --
>> coreboot mailing list: coreboot at coreboot.org
>> https://mail.coreboot.org/mailman/listinfo/coreboot
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot/attachments/20170505/54f91dc5/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: image.png
Type: image/png
Size: 115249 bytes
Desc: not available
URL: <http://mail.coreboot.org/pipermail/coreboot/attachments/20170505/54f91dc5/attachment-0001.png>