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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
