[coreboot-gerrit] Change in coreboot[master]: pciexp: retrain PCIe link at lower speed if no link

[Angel Pons (Code Review)]

Attention is currently required from: Jonathan Kollasch.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42683 )

Change subject: pciexp: retrain PCIe link at lower speed if no link
......................................................................

Patch Set 2:

(1 comment)

File src/device/pciexp_device.c:

https://review.coreboot.org/c/coreboot/+/42683/comment/22849b70_52927947 
PS2, Line 115: static void pciexp_limit_speed(struct device *dev, unsigned int cap)
             : {
             : 	u16 x, y;
             : 
             : 	x = pci_read_config16(dev, cap + PCI_EXP_FLAGS);
             : 
             : 	/* is this new enough to support LNKCTL2? */
             : 	if ((x & PCI_EXP_FLAGS_VERS) <= 1)
             : 		return;
             : 
             : 	y = pci_read_config16(dev, cap + PCI_EXP_LNKCTL2);
             : 	y &= PCI_EXP_LNKCTL2_TLS;
             : 
             : 	do {
             : 		/* link up? */
             : 		x = pci_read_config16(dev, cap + PCI_EXP_LNKSTA);
             : 		x = (x & PCI_EXP_LNKSTA_LW) >> 4;
             : 		if (x > 0)
             : 			return;
             : 
             : 		printk(BIOS_INFO, "%s: link down; retraining at target link speed %u\n",
             : 		       dev_path(dev), y);
             : 
             : 		x = pci_read_config16(dev, cap + PCI_EXP_LNKCTL2);
             : 		x &= ~PCI_EXP_LNKCTL2_TLS;
             : 		x |= y & PCI_EXP_LNKCTL2_TLS;
             : 		pci_write_config16(dev, cap + PCI_EXP_LNKCTL2, x);
             : 
             : 		pciexp_retrain_link(dev, cap);
             : 	} while (--y > 0);
             : }
Uh, please do not name things `x` and `y` when they have better names:

static void pciexp_limit_speed(struct device *dev, unsigned int cap)
 {
    /* Is this device new enough to support LNKCTL2? */
    const u16 pciexp_flags = pci_read_config16(dev, cap + PCI_EXP_FLAGS);
    if ((pciexp_flags & PCI_EXP_FLAGS_VERS) <= 1)
    	return;

u16 target_speed = pci_read_config16(dev, cap + PCI_EXP_LNKCTL2);
    target_speed &= PCI_EXP_LNKCTL2_TLS;
    do {
    	/* Link up? */
    	const u16 lnk_sta = pci_read_config16(dev, cap + PCI_EXP_LNKSTA);
    	if ((lnk_sta & PCI_EXP_LNKSTA_LW) >> 4)
    		return;

printk(BIOS_INFO, "%s: link down; retraining at target link speed %u\n",
    	       dev_path(dev), target_speed);

pci_update_config16(dev, cap + PCI_EXP_LNKCTL2, ~PCI_EXP_LNKCTL2_TLS,
    			target_speed & PCI_EXP_LNKCTL2_TLS);
    	pciexp_retrain_link(dev, cap);
    } while (--target_speed > 0);
 }

I would rename the function as well, since the name may suggest that it always limits PCIe speed.