[coreboot] PCIe Role-based error reporting

[Zoran Stojsavljevic]

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/068690.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
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot/attachments/20170504/b9022d38/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/b9022d38/attachment-0001.png>