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@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 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@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):

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 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> 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@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_Specification_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@coreboot.org
https://mail.coreboot.org/mailman/listinfo/coreboot

--
coreboot mailing list: coreboot@coreboot.org
https://mail.coreboot.org/mailman/listinfo/coreboot