Re: [PATCH V3 8/9] powerpc/powernv: Support PCI config restore for VFs

[Wei Yang <weiyang@linux.vnet.ibm.com>]

On Tue, May 12, 2015 at 04:34:03PM +1000, Gavin Shan wrote:
>>
>>>>+	/* Disable Completion Timeout */
>>>>+	if (pcie_cap) {
>>>>+		pnv_pci_cfg_read(pdn, pcie_cap + PCI_EXP_DEVCAP2, 4, &cap2);
>>>>+		if (cap2 & 0x10) {
>>>>+			pnv_pci_cfg_read(pdn, pcie_cap + PCI_EXP_DEVCTL2, 4, &cap2);
>>>>+			cap2 |= 0x10;
>>>>+			pnv_pci_cfg_write(pdn, pcie_cap + PCI_EXP_DEVCTL2, 4, cap2);
>>>>+		}
>>>>+	}
>>>>+
>>>>+	/* Enable SERR and parity checking */
>>>>+	pnv_pci_cfg_read(pdn, PCI_COMMAND, 2, &cmd);
>>>>+	cmd |= (PCI_COMMAND_PARITY | PCI_COMMAND_SERR);
>>>>+	pnv_pci_cfg_write(pdn, PCI_COMMAND, 2, cmd);
>>>>+
>>>>+	/* Enable report various errors */
>>>>+	if (pcie_cap) {
>>>>+		pnv_pci_cfg_read(pdn, pcie_cap + PCI_EXP_DEVCTL, 2, &devctl);
>>>>+		devctl &= ~PCI_EXP_DEVCTL_CERE;
>>>>+		devctl |= (PCI_EXP_DEVCTL_NFERE |
>>>>+			   PCI_EXP_DEVCTL_FERE |
>>>>+			   PCI_EXP_DEVCTL_URRE);
>>>>+		pnv_pci_cfg_write(pdn, pcie_cap + PCI_EXP_DEVCTL, 2, devctl);
>>>>+	}
>>>>+
>>>>+	/* Enable ECRC generation and check */
>>>>+	if (pcie_cap) {
>>>>+		aer_cap = pnv_eeh_find_ecap(pdn, PCI_EXT_CAP_ID_ERR);
>>>>+		pnv_pci_cfg_read(pdn, aer_cap + PCI_ERR_CAP, 4, &aer_capctl);
>>>>+		aer_capctl |= (PCI_ERR_CAP_ECRC_GENE | PCI_ERR_CAP_ECRC_CHKE);
>>>>+		pnv_pci_cfg_write(pdn, aer_cap + PCI_ERR_CAP, 4, aer_capctl);
>>>>+	}
>>>>+
>>>>+	return 0;
>>>>+}
>>>>+#endif /* CONFIG_PCI_IOV */
>>>>+
>>>
>>>The code is copied over from skiboot firmware. I still dislike the fact that
>>>we have to maintain two sets of similar functions in skiboot/kernel. I still
>>>believe the way I suggested can help: the firmware exports the error routing
>>>rules and kernel has support it based on the rules. With it, the skiboot is
>>>the source of the information to avoid mismatching between kernel/firmware.
>>
>>Yes, it looks we have duplicate code in kernel and skiboot.
>>
>>As you suggest, if we export some bit map from device node, we still have the
>>real logic in kernel, until we remove that part in skiboot.
>>
>>By removing that part in skiboot, we may have some compatibility problem. For
>>example, an old kernel may not run on the new version of skiboot.
>>
>
>It's fine to keep two set code which bear with same rule, which is exported
>from skiboot. In that case, the rule is the only thing we have to care. We
>don't need care the code any more to avoid mismatch between kernel/firmware.
>

Ok, duplication is reasonable, then the major point for this is we need to
have a clear rule for restoring configuration for a device.

Than I suggest we could have another patch set to handle this. Define the rule
clearly and restore the configuration in kernel when skiboot firmware export
such rules.

-- 
Richard Yang
Help you, Help me