Re: [PATCH 07/15] powerpc/powerpc: Add new PCIe functions for allocating cxl interrupts

Michael Neuling
2014-09-22T05:01:54Z

<snip>
> >+struct device_node *pnv_pci_to_phb_node(struct pci_dev *dev)
> >+{
> >+ struct device_node *np;
> >+ struct property *prop = NULL;
> >+
> >+ np = of_node_get(pci_device_to_OF_node(dev));
> >+
> >+ /* Scan up the tree looking for the PHB node */
> >+ while (np) {
> >+ if ((prop = of_find_property(np, "ibm,opal-phbid", NULL)))
> >+ break;
> >+ np = of_get_next_parent(np);
> >+ }
> >+
> >+ if (!prop) {
> >+ of_node_put(np);
> >+ return NULL;
> >+ }
> >+
> >+ return np;
> >+}
> >+EXPORT_SYMBOL(pnv_pci_to_phb_node);
>
> Nitpick: I'm not sure it's better way. "struct pci_controller::dn" should
> always have valid "ibm,opal-phbid", so I guess the code could be like this
> way:
>
> struct pci_controller *hose = pci_bus_to_host(dev->bus);
>
> return hose->dn;

Nice.. that makes it much simpler. I'll update.

<snip>
> >+
> >+#ifdef CONFIG_CXL_BASE
> >+int pnv_phb_to_cxl(struct pci_dev *dev)
> >+{
> >+ struct device_node *np;
> >+ struct pnv_ioda_pe *pe;
> >+ const u64 *prop64;
> >+ u64 phb_id;
> >+ int rc;
> >+
> >+ dev_info(&dev->dev, "switch PHB to CXL\n");
> >+
> >+ if (!(np = pnv_pci_to_phb_node(dev)))
> >+ return -ENODEV;
> >+
> >+ prop64 = of_get_property(np, "ibm,opal-phbid", NULL);
> >+
> >+ phb_id = be64_to_cpup(prop64);
> >+ dev_info(&dev->dev, "PHB-ID : 0x%016llx\n", phb_id);
> >+
>
> The PHB ID would have been there: struct pnv_phb::opal_id. So
> I guess we needn't grab it from device-tree again :)

Nice, I'll update.

> >+ if (!(pe = pnv_ioda_get_pe(dev))) {
> >+ rc = -ENODEV;
> >+ goto out;
> >+ }
> >+ dev_info(&dev->dev, " pe : %i\n", pe->pe_number);
>
> Perhaps you can reuse pe_info() here.

Yep, will do.

<snip>
> >+#ifdef CONFIG_CXL_BASE
> >+int pnv_cxl_ioda_msi_setup(struct pci_dev *dev, unsigned int hwirq,
> >+ unsigned int virq)
> >+{
> >+ struct pci_controller *hose = pci_bus_to_host(dev->bus);
> >+ struct pnv_phb *phb = hose->private_data;
> >+ unsigned int xive_num = hwirq - phb->msi_base;
> >+ struct pnv_ioda_pe *pe;
> >+ int rc;
> >+
> >+ if (!(pe = pnv_ioda_get_pe(dev)))
> >+ return -ENODEV;
> >+
> >+ /* Assign XIVE to PE */
> >+ rc = opal_pci_set_xive_pe(phb->opal_id, pe->pe_number, xive_num);
> >+ if (rc) {
> >+ pr_warn("%s: OPAL error %d setting msi_base 0x%x hwirq 0x%x XIVE 0x%x PE\n",
> >+ pci_name(dev), rc, phb->msi_base, hwirq, xive_num);
> >+ return -EIO;
> >+ }
>
> It seems current firmware doesn't support the OPAL API for PHB3.

The current public version of skiboot seems to be doing something here
in hw/phb3.c in phb3_set_ive_pe():

https://github.com/open-power/skiboot/blob/c34c4ef8c660e3e439365c8f5c06143ff00bc6bc/hw/phb3.c#L1096

I think we still need this.

Thanks again!
Mikey