Re: [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 <
[e-mail]> 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 <
> [e-mail]> 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.