On Fri, Aug 06, 2021 at 02:40:10AM +0530, srikanth.thokala@xxxxxxxxx wrote: [...] > +static int keembay_pcie_add_pcie_port(struct keembay_pcie *pcie, > + struct platform_device *pdev) > +{ > + struct dw_pcie *pci = &pcie->pci; > + struct pcie_port *pp = &pci->pp; > + struct device *dev = &pdev->dev; > + u32 val; > + int ret; > + > + pp->ops = &keembay_pcie_host_ops; > + pp->msi_irq = -ENODEV; > + > + ret = keembay_pcie_setup_msi_irq(pcie); > + if (ret) > + return ret; May I ask you (and DWC maintainers) please why we need to resort to setting pp->msi_irq = -ENODEV; while we *could* (?) just use pp->ops->msi_host_init() (ie which I believe can be keembay_pcie_setup_msi_irq() revisited ?) I would like to understand how this choice can be made by a DWC developer that has to add an incantation specific MSI logic handling glue. Other than that we can merge this code but I would like to understand the rationale behind the question above - it is not obvious to me. Thanks, Lorenzo > + > + pcie->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); > + if (IS_ERR(pcie->reset)) > + return PTR_ERR(pcie->reset); > + > + ret = keembay_pcie_probe_clocks(pcie); > + if (ret) > + return ret; > + > + val = readl(pcie->apb_base + PCIE_REGS_PCIE_PHY_CNTL); > + val |= PHY0_SRAM_BYPASS; > + writel(val, pcie->apb_base + PCIE_REGS_PCIE_PHY_CNTL); > + > + writel(PCIE_DEVICE_TYPE, pcie->apb_base + PCIE_REGS_PCIE_CFG); > + > + ret = keembay_pcie_pll_init(pcie); > + if (ret) > + return ret; > + > + val = readl(pcie->apb_base + PCIE_REGS_PCIE_CFG); > + writel(val | PCIE_RSTN, pcie->apb_base + PCIE_REGS_PCIE_CFG); > + keembay_ep_reset_deassert(pcie); > + > + ret = dw_pcie_host_init(pp); > + if (ret) { > + keembay_ep_reset_assert(pcie); > + dev_err(dev, "Failed to initialize host: %d\n", ret); > + return ret; > + } > + > + val = readl(pcie->apb_base + PCIE_REGS_INTERRUPT_ENABLE); > + if (IS_ENABLED(CONFIG_PCI_MSI)) > + val |= MSI_CTRL_INT_EN; > + writel(val, pcie->apb_base + PCIE_REGS_INTERRUPT_ENABLE); > + > + return 0; > +} > + > +static int keembay_pcie_probe(struct platform_device *pdev) > +{ > + const struct keembay_pcie_of_data *data; > + struct device *dev = &pdev->dev; > + struct keembay_pcie *pcie; > + struct dw_pcie *pci; > + enum dw_pcie_device_mode mode; > + > + data = device_get_match_data(dev); > + if (!data) > + return -ENODEV; > + > + mode = (enum dw_pcie_device_mode)data->mode; > + > + pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL); > + if (!pcie) > + return -ENOMEM; > + > + pci = &pcie->pci; > + pci->dev = dev; > + pci->ops = &keembay_pcie_ops; > + > + pcie->mode = mode; > + > + pcie->apb_base = devm_platform_ioremap_resource_byname(pdev, "apb"); > + if (IS_ERR(pcie->apb_base)) > + return PTR_ERR(pcie->apb_base); > + > + platform_set_drvdata(pdev, pcie); > + > + switch (pcie->mode) { > + case DW_PCIE_RC_TYPE: > + if (!IS_ENABLED(CONFIG_PCIE_KEEMBAY_HOST)) > + return -ENODEV; > + > + return keembay_pcie_add_pcie_port(pcie, pdev); > + case DW_PCIE_EP_TYPE: > + if (!IS_ENABLED(CONFIG_PCIE_KEEMBAY_EP)) > + return -ENODEV; > + > + pci->ep.ops = &keembay_pcie_ep_ops; > + return dw_pcie_ep_init(&pci->ep); > + default: > + dev_err(dev, "Invalid device type %d\n", pcie->mode); > + return -ENODEV; > + } > +} > + > +static const struct keembay_pcie_of_data keembay_pcie_rc_of_data = { > + .mode = DW_PCIE_RC_TYPE, > +}; > + > +static const struct keembay_pcie_of_data keembay_pcie_ep_of_data = { > + .mode = DW_PCIE_EP_TYPE, > +}; > + > +static const struct of_device_id keembay_pcie_of_match[] = { > + { > + .compatible = "intel,keembay-pcie", > + .data = &keembay_pcie_rc_of_data, > + }, > + { > + .compatible = "intel,keembay-pcie-ep", > + .data = &keembay_pcie_ep_of_data, > + }, > + {} > +}; > + > +static struct platform_driver keembay_pcie_driver = { > + .driver = { > + .name = "keembay-pcie", > + .of_match_table = keembay_pcie_of_match, > + .suppress_bind_attrs = true, > + }, > + .probe = keembay_pcie_probe, > +}; > +builtin_platform_driver(keembay_pcie_driver); > -- > 2.17.1 >