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

[Gavin Shan <gwshan@linux.vnet.ibm.com>]

On Tue, May 12, 2015 at 04:16:45PM +0800, Wei Yang wrote:
>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.
>

Well, I have to explain a bit more if I didn't make myself clear enough, then
you change the code in another way, which will waste your time.

- From skiboot, each PHB's device node maintains the rules, which *could* be
  described as the data structures I have given in previous replies if you
  can't figure out better data structures.
- Skiboot will reinitialize those devices for the following 3 cases: PCI
  enumeration, PCI config space restore requested from EEH, after PCI hotplug/reset.
  Obviously, the code needs changes to utilize the rules in PHB's device node.
- Kernel will do similiar thing as skiboot will do: Reinitialize VFs according
  to the rules in PHB's device node.

Yes, we have duplicated code, not rules. Hopefully, I make myself clear enough.

>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.
>

Sure.

Thanks,
Gavin