[coreboot] PCIe Role-based error reporting

[Youness Alaoui]

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/20170504/50a1ec1e/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/20170504/50a1ec1e/attachment-0001.png>