Hi Lorenzo, Gustavo, Thank you for reviewing. On Tue, 25 Sep 2018 18:53:01 +0100 Gustavo Pimentel <gustavo.pimentel@xxxxxxxxxxxx> wrote: > On 25/09/2018 17:14, Lorenzo Pieralisi wrote: > > [+Gustavo, please have a look at INTX/MSI management] > > > > On Thu, Sep 06, 2018 at 06:40:32PM +0900, Kunihiko Hayashi wrote: > >> This introduces specific glue layer for UniPhier platform to support > >> PCIe host controller that is based on the DesignWare PCIe core, and > >> this driver supports Root Complex (host) mode. > > > > Please read this thread and apply it to next versions: > > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__marc.info_-3Fl-3Dlinux-2Dpci-26m-3D150905742808166-26w-3D2&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=bkWxpLoW-f-E3EdiDCCa0_h0PicsViasSlvIpzZvPxs&m=H8UNDDUGQnQnqfWr4CBios689dJcjxu4qeTTRGulLmU&s=CgcXc_2LThyOpW-4bCriJNo9H1lzROEdy_cG9p-Y5hU&e= I also found this thread in previous linux-pci, and I think it's helpful for me. I'll check it carefully. > > > > [...] > > > >> +static int uniphier_pcie_link_up(struct dw_pcie *pci) > >> +{ > >> + struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci); > >> + u32 val, mask; > >> + > >> + val = readl(priv->base + PCL_STATUS_LINK); > >> + mask = PCL_RDLH_LINK_UP | PCL_XMLH_LINK_UP; > >> + > >> + return (val & mask) == mask; > >> +} > >> + > >> +static int uniphier_pcie_establish_link(struct dw_pcie *pci) > >> +{ > >> + struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci); > >> + int ret; > >> + > >> + if (dw_pcie_link_up(pci)) > >> + return 0; > >> + > >> + uniphier_pcie_ltssm_enable(priv); > >> + > >> + ret = dw_pcie_wait_for_link(pci); > >> + if (ret == -ETIMEDOUT) { > >> + dev_warn(pci->dev, "Link not up\n"); > >> + ret = 0; > > > > So if the link is not up we warn, return and all is fine ? Although I was not sure about driver's behavior if the link isn't up, since the driver can't do anything after that, so it seems suitable to return -ETIMEOUT and fail to probe. > > > >> + } > >> + > >> + return ret; > >> +} > >> + > >> +static void uniphier_pcie_stop_link(struct dw_pcie *pci) > >> +{ > >> + struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci); > >> + > >> + uniphier_pcie_ltssm_disable(priv); > >> +} > >> + > >> +static int uniphier_pcie_intx_map(struct irq_domain *domain, unsigned int irq, > >> + irq_hw_number_t hwirq) > >> +{ > >> + irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq); > >> + irq_set_chip_data(irq, domain->host_data); > >> + > >> + return 0; > >> +} > >> + > >> +static const struct irq_domain_ops uniphier_intx_domain_ops = { > >> + .map = uniphier_pcie_intx_map, > >> +}; > > > > I looped in Gustavo since this is not how I expect INTX management > > should be done. I do not think there is a DWC INTX generic layer > > but I think drivers/pci/controller/dwc/pci-keystone-dw.c is how > > it has to be done. > > > >> + > >> +static int uniphier_pcie_init_irq_domain(struct pcie_port *pp) > >> +{ > >> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > >> + struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci); > >> + struct device_node *np = pci->dev->of_node; > >> + struct device_node *np_intc = of_get_next_child(np, NULL); > >> + > >> + if (!np_intc) { > >> + dev_err(pci->dev, "Failed to get child node\n"); > >> + return -ENODEV; > >> + } > >> + > >> + priv->irq_domain = irq_domain_add_linear(np_intc, PCI_NUM_INTX, > >> + &uniphier_intx_domain_ops, > >> + pp); > >> + if (!priv->irq_domain) { > >> + dev_err(pci->dev, "Failed to get INTx domain\n"); > >> + return -ENODEV; > >> + } > >> + > >> + return 0; > >> +} > >> + > >> +static void uniphier_pcie_irq_enable(struct uniphier_pcie_priv *priv) > >> +{ > >> + writel(PCL_RCV_INT_ALL_ENABLE, priv->base + PCL_RCV_INT); > >> + writel(PCL_RCV_INTX_ALL_ENABLE, priv->base + PCL_RCV_INTX); > >> +} > >> + > >> +static void uniphier_pcie_irq_disable(struct uniphier_pcie_priv *priv) > >> +{ > >> + writel(0, priv->base + PCL_RCV_INT); > >> + writel(0, priv->base + PCL_RCV_INTX); > >> +} > > > >> + > >> +static irqreturn_t uniphier_pcie_irq_handler(int irq, void *arg) > > > > This should not be an IRQ handler (and we should not use > > devm_request_irq() for the multiplexed IRQ line), it is a chained > > interrupt controller configuration and should be managed by an IRQ > > chain, again the way keystone does it seems reasonable to me. I see. I'll try to refer to keystone driver as an example for chained interrupt management. > > > >> +{ > >> + struct uniphier_pcie_priv *priv = arg; > >> + struct dw_pcie *pci = &priv->pci; > >> + u32 val; > >> + > >> + /* INT for debug */ > >> + val = readl(priv->base + PCL_RCV_INT); > >> + > >> + if (val & PCL_CFG_BW_MGT_STATUS) > >> + dev_dbg(pci->dev, "Link Bandwidth Management Event\n"); > >> + if (val & PCL_CFG_LINK_AUTO_BW_STATUS) > >> + dev_dbg(pci->dev, "Link Autonomous Bandwidth Event\n"); > >> + if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS) > >> + dev_dbg(pci->dev, "Root Error\n"); > >> + if (val & PCL_CFG_PME_MSI_STATUS) > >> + dev_dbg(pci->dev, "PME Interrupt\n"); > >> + > >> + writel(val, priv->base + PCL_RCV_INT); > >> + > >> + /* INTx */ > >> + val = readl(priv->base + PCL_RCV_INTX); > >> + > >> + if (val & PCL_RADM_INTA_STATUS) > >> + generic_handle_irq(irq_find_mapping(priv->irq_domain, 0)); > >> + if (val & PCL_RADM_INTB_STATUS) > >> + generic_handle_irq(irq_find_mapping(priv->irq_domain, 1)); > >> + if (val & PCL_RADM_INTC_STATUS) > >> + generic_handle_irq(irq_find_mapping(priv->irq_domain, 2)); > >> + if (val & PCL_RADM_INTD_STATUS) > >> + generic_handle_irq(irq_find_mapping(priv->irq_domain, 3)); > > > > Nit: Do you really need 4 if statements to handle INTX ? I found an example to use for_each_set_bit() in pci-dra7xx.c. I'll replace it. > > > >> + > >> + writel(val, priv->base + PCL_RCV_INTX); > >> + > >> + return IRQ_HANDLED; > >> +} > >> + > >> +static irqreturn_t uniphier_pcie_msi_irq_handler(int irq, void *arg) > >> +{ > >> + struct pcie_port *pp = arg; > >> + > >> + return dw_handle_msi_irq(pp); > >> +} > > > > This IRQ handler must be removed, the MSI irq is handled by dwc core. I see. I'll remove it. > > > >> +static int uniphier_pcie_host_init(struct pcie_port *pp) > >> +{ > >> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > >> + int ret; > >> + > >> + dw_pcie_setup_rc(pp); > >> + ret = uniphier_pcie_establish_link(pci); > >> + if (ret) > >> + return ret; > >> + > >> + if (IS_ENABLED(CONFIG_PCI_MSI)) > >> + dw_pcie_msi_init(pp); > >> + > >> + return 0; > >> +} > >> + > >> +static const struct dw_pcie_host_ops uniphier_pcie_host_ops = { > >> + .host_init = uniphier_pcie_host_init, > >> +}; > >> + > >> +static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv, > >> + struct platform_device *pdev) > >> +{ > >> + struct dw_pcie *pci = &priv->pci; > >> + struct pcie_port *pp = &pci->pp; > >> + struct device *dev = &pdev->dev; > >> + int ret; > >> + > >> + pp->root_bus_nr = -1; > > > > Useless initialization, remove it. > > > > (ie dw_pcie_host_init() initializes root_bus_nr for you). I found it. This will be removed. > > > >> + pp->ops = &uniphier_pcie_host_ops; > >> + > >> + pp->irq = platform_get_irq_byname(pdev, "intx"); > >> + if (pp->irq < 0) { > >> + dev_err(dev, "Failed to get intx irq\n"); > >> + return pp->irq; > >> + } > >> + > >> + ret = devm_request_irq(dev, pp->irq, uniphier_pcie_irq_handler, > >> + IRQF_SHARED, "pcie", priv); > > > > This is wrong, you should set-up a chained IRQ for INTX. > > > > I *think* that > > > > ks_pcie_setup_interrupts() > > > > is a good example to start with but I wonder whether it is worth > > generalizing the INTX approach to designware as a whole as it was > > done for MSIs. > > > > Thoughts ? > > From what I understood this is for legacy IRQ, right? Yes. For legacy IRQ. > Like you (Lorenzo) said there is 2 drivers (pci-keystone-dw.c and pci-dra7xx.c) > that uses it and can be use as a template for handling this type of interrupts. > > We can try to pass some kind of generic INTX function to the DesignWare host > library to handling this, but this will require some help from keystone and > dra7xx maintainers, since my setup doesn't have legacy IRQ HW support. Now I think it's difficult to make a template for INTX function, and at first, I'll try to re-write this part with reference to pci-keystone-dw.c. > > > > >> + if (ret) { > >> + dev_err(dev, "Failed to request irq %d\n", pp->irq); > >> + return ret; > >> + } > >> + > >> + ret = uniphier_pcie_init_irq_domain(pp); > >> + if (ret) > >> + return ret; > >> + > >> + if (IS_ENABLED(CONFIG_PCI_MSI)) { > >> + pp->msi_irq = platform_get_irq_byname(pdev, "msi"); > >> + if (pp->msi_irq < 0) > >> + return pp->msi_irq; > >> + > >> + ret = devm_request_irq(dev, pp->msi_irq, > >> + uniphier_pcie_msi_irq_handler, > >> + IRQF_SHARED, "pcie-msi", pp); > > > > No. With MSI management in DWC core all you need to do is initializing > > pp->msi_irq. Okay, it isn't necessary to call devm_request_irq() here no longer. Thank you, --- Best Regards, Kunihiko Hayashi
