diff mbox series

[v2,2/2] PCI: controller: dwc: add UniPhier PCIe host controller support

Message ID 1536226832-5089-3-git-send-email-hayashi.kunihiko@socionext.com
State Changes Requested
Delegated to: Lorenzo Pieralisi
Headers show
Series add new UniPhier PCIe host driver | expand

Commit Message

Kunihiko Hayashi Sept. 6, 2018, 9:40 a.m. UTC 
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.

Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
 drivers/pci/controller/dwc/Kconfig         |   9 +
 drivers/pci/controller/dwc/Makefile        |   1 +
 drivers/pci/controller/dwc/pcie-uniphier.c | 465 +++++++++++++++++++++++++++++
 3 files changed, 475 insertions(+)
 create mode 100644 drivers/pci/controller/dwc/pcie-uniphier.c

Comments

Lorenzo Pieralisi Sept. 25, 2018, 4:14 p.m. UTC | #1 
[+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://marc.info/?l=linux-pci&m=150905742808166&w=2

[...]

> +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 ?

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

> +{
> +	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 ?

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

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

> +	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 ?

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

Lorenzo

> +		if (ret) {
> +			dev_err(dev, "failed to request msi_irq %d\n",
> +				pp->msi_irq);
> +			return ret;
> +		}
> +	}
> +
> +	ret = dw_pcie_host_init(pp);
> +	if (ret) {
> +		dev_err(dev, "Failed to initialize host (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int uniphier_pcie_host_enable(struct uniphier_pcie_priv *priv)
> +{
> +	int ret;
> +
> +	ret = clk_prepare_enable(priv->clk);
> +	if (ret)
> +		return ret;
> +
> +	ret = reset_control_deassert(priv->rst);
> +	if (ret)
> +		goto out_clk_disable;
> +
> +	uniphier_pcie_init_rc(priv);
> +
> +	ret = phy_init(priv->phy);
> +	if (ret)
> +		goto out_rst_assert;
> +
> +	ret = uniphier_pcie_wait_rc(priv);
> +	if (ret)
> +		goto out_phy_exit;
> +
> +	uniphier_pcie_irq_enable(priv);
> +
> +	return 0;
> +
> +out_phy_exit:
> +	phy_exit(priv->phy);
> +out_rst_assert:
> +	reset_control_assert(priv->rst);
> +out_clk_disable:
> +	clk_disable_unprepare(priv->clk);
> +
> +	return ret;
> +}
> +
> +static void uniphier_pcie_host_disable(struct uniphier_pcie_priv *priv)
> +{
> +	uniphier_pcie_irq_disable(priv);
> +	phy_exit(priv->phy);
> +	reset_control_assert(priv->rst);
> +	clk_disable_unprepare(priv->clk);
> +}
> +
> +static const struct dw_pcie_ops dw_pcie_ops = {
> +	.start_link = uniphier_pcie_establish_link,
> +	.stop_link = uniphier_pcie_stop_link,
> +	.link_up = uniphier_pcie_link_up,
> +};
> +
> +static int uniphier_pcie_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct uniphier_pcie_priv *priv;
> +	struct resource *res;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->pci.dev = dev;
> +	priv->pci.ops = &dw_pcie_ops;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
> +	priv->pci.dbi_base = devm_pci_remap_cfg_resource(dev, res);
> +	if (IS_ERR(priv->pci.dbi_base))
> +		return PTR_ERR(priv->pci.dbi_base);
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "link");
> +	priv->base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	priv->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(priv->clk))
> +		return PTR_ERR(priv->clk);
> +
> +	priv->rst = devm_reset_control_get_shared(dev, NULL);
> +	if (IS_ERR(priv->rst))
> +		return PTR_ERR(priv->rst);
> +
> +	priv->phy = devm_phy_optional_get(dev, "pcie-phy");
> +	if (IS_ERR(priv->phy))
> +		return PTR_ERR(priv->phy);
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	ret = uniphier_pcie_host_enable(priv);
> +	if (ret)
> +		return ret;
> +
> +	return uniphier_add_pcie_port(priv, pdev);
> +}
> +
> +static int uniphier_pcie_remove(struct platform_device *pdev)
> +{
> +	struct uniphier_pcie_priv *priv = platform_get_drvdata(pdev);
> +
> +	uniphier_pcie_host_disable(priv);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id uniphier_pcie_match[] = {
> +	{ .compatible = "socionext,uniphier-pcie", },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, uniphier_pcie_match);
> +
> +static struct platform_driver uniphier_pcie_driver = {
> +	.probe  = uniphier_pcie_probe,
> +	.remove = uniphier_pcie_remove,
> +	.driver = {
> +		.name = "uniphier-pcie",
> +		.of_match_table = uniphier_pcie_match,
> +	},
> +};
> +builtin_platform_driver(uniphier_pcie_driver);
> +
> +MODULE_AUTHOR("Kunihiko Hayashi <hayashi.kunihiko@socionext.com>");
> +MODULE_DESCRIPTION("UniPhier PCIe host controller driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.7.4
>
Gustavo Pimentel Sept. 25, 2018, 5:53 p.m. UTC | #2 
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=
> 
> [...]
> 
>> +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 ?
> 
>> +	}
>> +
>> +	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.
> 
>> +{
>> +	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 ?
> 
>> +
>> +	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.
> 
>> +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).
> 
>> +	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?
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.

> 
>> +	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.
> 
> Lorenzo
> 
>> +		if (ret) {
>> +			dev_err(dev, "failed to request msi_irq %d\n",
>> +				pp->msi_irq);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	ret = dw_pcie_host_init(pp);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to initialize host (%d)\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int uniphier_pcie_host_enable(struct uniphier_pcie_priv *priv)
>> +{
>> +	int ret;
>> +
>> +	ret = clk_prepare_enable(priv->clk);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = reset_control_deassert(priv->rst);
>> +	if (ret)
>> +		goto out_clk_disable;
>> +
>> +	uniphier_pcie_init_rc(priv);
>> +
>> +	ret = phy_init(priv->phy);
>> +	if (ret)
>> +		goto out_rst_assert;
>> +
>> +	ret = uniphier_pcie_wait_rc(priv);
>> +	if (ret)
>> +		goto out_phy_exit;
>> +
>> +	uniphier_pcie_irq_enable(priv);
>> +
>> +	return 0;
>> +
>> +out_phy_exit:
>> +	phy_exit(priv->phy);
>> +out_rst_assert:
>> +	reset_control_assert(priv->rst);
>> +out_clk_disable:
>> +	clk_disable_unprepare(priv->clk);
>> +
>> +	return ret;
>> +}
>> +
>> +static void uniphier_pcie_host_disable(struct uniphier_pcie_priv *priv)
>> +{
>> +	uniphier_pcie_irq_disable(priv);
>> +	phy_exit(priv->phy);
>> +	reset_control_assert(priv->rst);
>> +	clk_disable_unprepare(priv->clk);
>> +}
>> +
>> +static const struct dw_pcie_ops dw_pcie_ops = {
>> +	.start_link = uniphier_pcie_establish_link,
>> +	.stop_link = uniphier_pcie_stop_link,
>> +	.link_up = uniphier_pcie_link_up,
>> +};
>> +
>> +static int uniphier_pcie_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct uniphier_pcie_priv *priv;
>> +	struct resource *res;
>> +	int ret;
>> +
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	priv->pci.dev = dev;
>> +	priv->pci.ops = &dw_pcie_ops;
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
>> +	priv->pci.dbi_base = devm_pci_remap_cfg_resource(dev, res);
>> +	if (IS_ERR(priv->pci.dbi_base))
>> +		return PTR_ERR(priv->pci.dbi_base);
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "link");
>> +	priv->base = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(priv->base))
>> +		return PTR_ERR(priv->base);
>> +
>> +	priv->clk = devm_clk_get(dev, NULL);
>> +	if (IS_ERR(priv->clk))
>> +		return PTR_ERR(priv->clk);
>> +
>> +	priv->rst = devm_reset_control_get_shared(dev, NULL);
>> +	if (IS_ERR(priv->rst))
>> +		return PTR_ERR(priv->rst);
>> +
>> +	priv->phy = devm_phy_optional_get(dev, "pcie-phy");
>> +	if (IS_ERR(priv->phy))
>> +		return PTR_ERR(priv->phy);
>> +
>> +	platform_set_drvdata(pdev, priv);
>> +
>> +	ret = uniphier_pcie_host_enable(priv);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return uniphier_add_pcie_port(priv, pdev);
>> +}
>> +
>> +static int uniphier_pcie_remove(struct platform_device *pdev)
>> +{
>> +	struct uniphier_pcie_priv *priv = platform_get_drvdata(pdev);
>> +
>> +	uniphier_pcie_host_disable(priv);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id uniphier_pcie_match[] = {
>> +	{ .compatible = "socionext,uniphier-pcie", },
>> +	{ /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, uniphier_pcie_match);
>> +
>> +static struct platform_driver uniphier_pcie_driver = {
>> +	.probe  = uniphier_pcie_probe,
>> +	.remove = uniphier_pcie_remove,
>> +	.driver = {
>> +		.name = "uniphier-pcie",
>> +		.of_match_table = uniphier_pcie_match,
>> +	},
>> +};
>> +builtin_platform_driver(uniphier_pcie_driver);
>> +
>> +MODULE_AUTHOR("Kunihiko Hayashi <hayashi.kunihiko@socionext.com>");
>> +MODULE_DESCRIPTION("UniPhier PCIe host controller driver");
>> +MODULE_LICENSE("GPL v2");
>> -- 
>> 2.7.4
>>
>
Kunihiko Hayashi Sept. 26, 2018, 12:31 p.m. UTC | #3 
Hi Lorenzo, Gustavo,

Thank you for reviewing.

On Tue, 25 Sep 2018 18:53:01 +0100
Gustavo Pimentel <gustavo.pimentel@synopsys.com> 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
Kunihiko Hayashi Sept. 27, 2018, 7:44 a.m. UTC | #4 
Hi Lorenzo, Gustavo,

On Wed, 26 Sep 2018 21:31:36 +0900 <hayashi.kunihiko@socionext.com> wrote:

> Hi Lorenzo, Gustavo,
> 
> Thank you for reviewing.
> 
> On Tue, 25 Sep 2018 18:53:01 +0100
> Gustavo Pimentel <gustavo.pimentel@synopsys.com> 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.

[snip]

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

I understand that there are 2 types of interrupt and the drivers.

One like pci-keystone-dw.c is:

 - there are 4 interrupts for legacy,
 - invoke handlers for each interrupt, and handle the interrupt,
 - call irq_set_chained_handler_and_data() to make a chain of the interrupts
  when initializing

The other like pci-dra7xx.c is:

 - there is 1 IRQ for legacy as a parent,
 - check an interrupt factor register, and handle the interrupt correspond
   to the factor,
 - call request_irq() for the parent IRQ and irq_domain_add_linear() for
   the factor when initializing

The pcie-uniphier.c is the same type as the latter (like pci-dra7xx.c).

However, in pci-dra7xx.c, MSI and legacy IRQ share the same interrupt number,
so the same handler is called and the handler divides these IRQs.
(found in dra7xx_pcie_msi_irq_handler())

In pcie-uniphier.c, MSI and legacy IRQ are independent.
Therefore it's necessary to prepare the handler for the legacy IRQ.

I think that it's difficult to apply the way of pci-keystone-dw.c,
and uniphier_pcie_irq_handler() and calling devm_request_irq() are still necessary
to handle legacy IRQ.

Thank you,

---
Best Regards,
Kunihiko Hayashi
Lorenzo Pieralisi Sept. 28, 2018, 11:06 a.m. UTC | #5 
[+Murali, Marc]

On Thu, Sep 27, 2018 at 04:44:26PM +0900, Kunihiko Hayashi wrote:
> Hi Lorenzo, Gustavo,
> 
> On Wed, 26 Sep 2018 21:31:36 +0900 <hayashi.kunihiko@socionext.com> wrote:
> 
> > Hi Lorenzo, Gustavo,
> > 
> > Thank you for reviewing.
> > 
> > On Tue, 25 Sep 2018 18:53:01 +0100
> > Gustavo Pimentel <gustavo.pimentel@synopsys.com> 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.
> 
> [snip]
> 
> > > >> +	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.
> 
> I understand that there are 2 types of interrupt and the drivers.
> 
> One like pci-keystone-dw.c is:
> 
>  - there are 4 interrupts for legacy,
>  - invoke handlers for each interrupt, and handle the interrupt,
>  - call irq_set_chained_handler_and_data() to make a chain of the interrupts
>   when initializing
> 
> The other like pci-dra7xx.c is:
> 
>  - there is 1 IRQ for legacy as a parent,
>  - check an interrupt factor register, and handle the interrupt correspond
>    to the factor,
>  - call request_irq() for the parent IRQ and irq_domain_add_linear() for
>    the factor when initializing
> 
> The pcie-uniphier.c is the same type as the latter (like pci-dra7xx.c).
> 
> However, in pci-dra7xx.c, MSI and legacy IRQ share the same interrupt number,
> so the same handler is called and the handler divides these IRQs.
> (found in dra7xx_pcie_msi_irq_handler())
> 
> In pcie-uniphier.c, MSI and legacy IRQ are independent.
> Therefore it's necessary to prepare the handler for the legacy IRQ.
> 
> I think that it's difficult to apply the way of pci-keystone-dw.c, and
> uniphier_pcie_irq_handler() and calling devm_request_irq() are still
> necessary to handle legacy IRQ.

I do not think it is difficult, the difference is that keystone has
1 GIC irq line allocated per legacy IRQ, your set-up has one for
all INTX.

*However*, I would like some clarifications from Murali on this code
in drivers/pci/controller/dwc/pci-keystone.c:

static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
{
	unsigned int irq = irq_desc_get_irq(desc);
	struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc);
	struct dw_pcie *pci = ks_pcie->pci;
	struct device *dev = pci->dev;
	u32 irq_offset = irq - ks_pcie->legacy_host_irqs[0];

Here the IRQ numbers are virtual IRQs, is it correct to consider
the virq numbers as sequential values ? The "offset" is used to
handle the PCI controller interrupt registers, so it must be a value
between 0-3 IIUC.

I would appreciated a detailed explanation of keystone legacy IRQ
handling so that we can eventually try to replicate it and generalize
it.

Lorenzo

	[...]

	chained_irq_enter(chip, desc);
	ks_dw_pcie_handle_legacy_irq(ks_pcie, irq_offset);
	chained_irq_exit(chip, desc);
}
Marc Zyngier Sept. 28, 2018, 1:17 p.m. UTC | #6 
On 28/09/18 12:06, Lorenzo Pieralisi wrote:
> [+Murali, Marc]
> 
> On Thu, Sep 27, 2018 at 04:44:26PM +0900, Kunihiko Hayashi wrote:
>> Hi Lorenzo, Gustavo,
>>
>> On Wed, 26 Sep 2018 21:31:36 +0900 <hayashi.kunihiko@socionext.com> wrote:
>>
>>> Hi Lorenzo, Gustavo,
>>>
>>> Thank you for reviewing.
>>>
>>> On Tue, 25 Sep 2018 18:53:01 +0100
>>> Gustavo Pimentel <gustavo.pimentel@synopsys.com> 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.
>>
>> [snip]
>>
>>>>>> +	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.
>>
>> I understand that there are 2 types of interrupt and the drivers.
>>
>> One like pci-keystone-dw.c is:
>>
>>   - there are 4 interrupts for legacy,
>>   - invoke handlers for each interrupt, and handle the interrupt,
>>   - call irq_set_chained_handler_and_data() to make a chain of the interrupts
>>    when initializing
>>
>> The other like pci-dra7xx.c is:
>>
>>   - there is 1 IRQ for legacy as a parent,
>>   - check an interrupt factor register, and handle the interrupt correspond
>>     to the factor,
>>   - call request_irq() for the parent IRQ and irq_domain_add_linear() for
>>     the factor when initializing
>>
>> The pcie-uniphier.c is the same type as the latter (like pci-dra7xx.c).
>>
>> However, in pci-dra7xx.c, MSI and legacy IRQ share the same interrupt number,
>> so the same handler is called and the handler divides these IRQs.
>> (found in dra7xx_pcie_msi_irq_handler())
>>
>> In pcie-uniphier.c, MSI and legacy IRQ are independent.
>> Therefore it's necessary to prepare the handler for the legacy IRQ.
>>
>> I think that it's difficult to apply the way of pci-keystone-dw.c, and
>> uniphier_pcie_irq_handler() and calling devm_request_irq() are still
>> necessary to handle legacy IRQ.
> 
> I do not think it is difficult, the difference is that keystone has
> 1 GIC irq line allocated per legacy IRQ, your set-up has one for
> all INTX.
> 
> *However*, I would like some clarifications from Murali on this code
> in drivers/pci/controller/dwc/pci-keystone.c:
> 
> static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
> {
> 	unsigned int irq = irq_desc_get_irq(desc);
> 	struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc);
> 	struct dw_pcie *pci = ks_pcie->pci;
> 	struct device *dev = pci->dev;
> 	u32 irq_offset = irq - ks_pcie->legacy_host_irqs[0];
> 
> Here the IRQ numbers are virtual IRQs, is it correct to consider
> the virq numbers as sequential values ? The "offset" is used to
> handle the PCI controller interrupt registers, so it must be a value
> between 0-3 IIUC.

There is absolutely no reason why virtual interrupt numbers should be 
contiguous. Shake the allocator hard enough, and you'll see gaps appearing.

In general, the only thing that makes sense is to compute this offset 
based on the hwirq which is HW-specific.

Thanks,

	M.
Lorenzo Pieralisi Sept. 28, 2018, 3:43 p.m. UTC | #7 
On Fri, Sep 28, 2018 at 02:17:16PM +0100, Marc Zyngier wrote:
> On 28/09/18 12:06, Lorenzo Pieralisi wrote:
> >[+Murali, Marc]
> >
> >On Thu, Sep 27, 2018 at 04:44:26PM +0900, Kunihiko Hayashi wrote:
> >>Hi Lorenzo, Gustavo,
> >>
> >>On Wed, 26 Sep 2018 21:31:36 +0900 <hayashi.kunihiko@socionext.com> wrote:
> >>
> >>>Hi Lorenzo, Gustavo,
> >>>
> >>>Thank you for reviewing.
> >>>
> >>>On Tue, 25 Sep 2018 18:53:01 +0100
> >>>Gustavo Pimentel <gustavo.pimentel@synopsys.com> 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.
> >>
> >>[snip]
> >>
> >>>>>>+	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.
> >>
> >>I understand that there are 2 types of interrupt and the drivers.
> >>
> >>One like pci-keystone-dw.c is:
> >>
> >>  - there are 4 interrupts for legacy,
> >>  - invoke handlers for each interrupt, and handle the interrupt,
> >>  - call irq_set_chained_handler_and_data() to make a chain of the interrupts
> >>   when initializing
> >>
> >>The other like pci-dra7xx.c is:
> >>
> >>  - there is 1 IRQ for legacy as a parent,
> >>  - check an interrupt factor register, and handle the interrupt correspond
> >>    to the factor,
> >>  - call request_irq() for the parent IRQ and irq_domain_add_linear() for
> >>    the factor when initializing
> >>
> >>The pcie-uniphier.c is the same type as the latter (like pci-dra7xx.c).
> >>
> >>However, in pci-dra7xx.c, MSI and legacy IRQ share the same interrupt number,
> >>so the same handler is called and the handler divides these IRQs.
> >>(found in dra7xx_pcie_msi_irq_handler())
> >>
> >>In pcie-uniphier.c, MSI and legacy IRQ are independent.
> >>Therefore it's necessary to prepare the handler for the legacy IRQ.
> >>
> >>I think that it's difficult to apply the way of pci-keystone-dw.c, and
> >>uniphier_pcie_irq_handler() and calling devm_request_irq() are still
> >>necessary to handle legacy IRQ.
> >
> >I do not think it is difficult, the difference is that keystone has
> >1 GIC irq line allocated per legacy IRQ, your set-up has one for
> >all INTX.
> >
> >*However*, I would like some clarifications from Murali on this code
> >in drivers/pci/controller/dwc/pci-keystone.c:
> >
> >static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
> >{
> >	unsigned int irq = irq_desc_get_irq(desc);
> >	struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc);
> >	struct dw_pcie *pci = ks_pcie->pci;
> >	struct device *dev = pci->dev;
> >	u32 irq_offset = irq - ks_pcie->legacy_host_irqs[0];
> >
> >Here the IRQ numbers are virtual IRQs, is it correct to consider
> >the virq numbers as sequential values ? The "offset" is used to
> >handle the PCI controller interrupt registers, so it must be a value
> >between 0-3 IIUC.
> 
> There is absolutely no reason why virtual interrupt numbers should be
> contiguous. Shake the allocator hard enough, and you'll see gaps appearing.
> 
> In general, the only thing that makes sense is to compute this offset based
> on the hwirq which is HW-specific.

That was my understanding and why I asked, which means that keystone
code can break (unless I read it wrong) and Murali will send me a fix as
soon as possible please to get it right (and Kunihiko will base his
code on this discussion).

Lorenzo
Lorenzo Pieralisi Oct. 1, 2018, 10:06 a.m. UTC | #8 
On Fri, Sep 28, 2018 at 02:17:16PM +0100, Marc Zyngier wrote:
> On 28/09/18 12:06, Lorenzo Pieralisi wrote:
> >[+Murali, Marc]
> >
> >On Thu, Sep 27, 2018 at 04:44:26PM +0900, Kunihiko Hayashi wrote:
> >>Hi Lorenzo, Gustavo,
> >>
> >>On Wed, 26 Sep 2018 21:31:36 +0900 <hayashi.kunihiko@socionext.com> wrote:
> >>
> >>>Hi Lorenzo, Gustavo,
> >>>
> >>>Thank you for reviewing.
> >>>
> >>>On Tue, 25 Sep 2018 18:53:01 +0100
> >>>Gustavo Pimentel <gustavo.pimentel@synopsys.com> 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.
> >>
> >>[snip]
> >>
> >>>>>>+	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.
> >>
> >>I understand that there are 2 types of interrupt and the drivers.
> >>
> >>One like pci-keystone-dw.c is:
> >>
> >>  - there are 4 interrupts for legacy,
> >>  - invoke handlers for each interrupt, and handle the interrupt,
> >>  - call irq_set_chained_handler_and_data() to make a chain of the interrupts
> >>   when initializing
> >>
> >>The other like pci-dra7xx.c is:
> >>
> >>  - there is 1 IRQ for legacy as a parent,
> >>  - check an interrupt factor register, and handle the interrupt correspond
> >>    to the factor,
> >>  - call request_irq() for the parent IRQ and irq_domain_add_linear() for
> >>    the factor when initializing
> >>
> >>The pcie-uniphier.c is the same type as the latter (like pci-dra7xx.c).
> >>
> >>However, in pci-dra7xx.c, MSI and legacy IRQ share the same interrupt number,
> >>so the same handler is called and the handler divides these IRQs.
> >>(found in dra7xx_pcie_msi_irq_handler())
> >>
> >>In pcie-uniphier.c, MSI and legacy IRQ are independent.
> >>Therefore it's necessary to prepare the handler for the legacy IRQ.
> >>
> >>I think that it's difficult to apply the way of pci-keystone-dw.c, and
> >>uniphier_pcie_irq_handler() and calling devm_request_irq() are still
> >>necessary to handle legacy IRQ.
> >
> >I do not think it is difficult, the difference is that keystone has
> >1 GIC irq line allocated per legacy IRQ, your set-up has one for
> >all INTX.
> >
> >*However*, I would like some clarifications from Murali on this code
> >in drivers/pci/controller/dwc/pci-keystone.c:
> >
> >static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
> >{
> >	unsigned int irq = irq_desc_get_irq(desc);
> >	struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc);
> >	struct dw_pcie *pci = ks_pcie->pci;
> >	struct device *dev = pci->dev;
> >	u32 irq_offset = irq - ks_pcie->legacy_host_irqs[0];
> >
> >Here the IRQ numbers are virtual IRQs, is it correct to consider
> >the virq numbers as sequential values ? The "offset" is used to
> >handle the PCI controller interrupt registers, so it must be a value
> >between 0-3 IIUC.
> 
> There is absolutely no reason why virtual interrupt numbers should be
> contiguous. Shake the allocator hard enough, and you'll see gaps appearing.
> 
> In general, the only thing that makes sense is to compute this offset based
> on the hwirq which is HW-specific.

More to it, AFAIK the list of HW irqs in the PCI host bridge controller
"interrupts" property is not necessarily a sequential list of numbers
either.

If I understand how these bindings/code works, the PCI INTX legacy IRQs
are mapped to IRQ lines {0,1,2,3} of the PCI host bridge interrupt
controller through the "interrupt-map" property.

Those IRQ lines are cascaded to the GIC through the HW irq in the
PCI host controller "interrupts" property:

IRQ line 0 <-> first cell of "interrupts" property
IRQ line 1 <-> second cell of "interrupts" property

and so forth.

I am not sure there is anything formal defining this behaviour other
than implicit expectactions.

@Murali: can you clarify please ?

Lorenzo
Kunihiko Hayashi Oct. 1, 2018, 11:06 a.m. UTC | #9 
Hi Lorenzo,

On Fri, 28 Sep 2018 12:06:40 +0100 <lorenzo.pieralisi@arm.com> wrote:

> [+Murali, Marc]
> 
> On Thu, Sep 27, 2018 at 04:44:26PM +0900, Kunihiko Hayashi wrote:
> > Hi Lorenzo, Gustavo,
> > 
> > On Wed, 26 Sep 2018 21:31:36 +0900 <hayashi.kunihiko@socionext.com> wrote:
> > 
> > > Hi Lorenzo, Gustavo,
> > > 
> > > Thank you for reviewing.
> > > 
> > > On Tue, 25 Sep 2018 18:53:01 +0100
> > > Gustavo Pimentel <gustavo.pimentel@synopsys.com> 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.
> > 
> > [snip]
> > 
> > > > >> +	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.
> > 
> > I understand that there are 2 types of interrupt and the drivers.
> > 
> > One like pci-keystone-dw.c is:
> > 
> >  - there are 4 interrupts for legacy,
> >  - invoke handlers for each interrupt, and handle the interrupt,
> >  - call irq_set_chained_handler_and_data() to make a chain of the interrupts
> >   when initializing
> > 
> > The other like pci-dra7xx.c is:
> > 
> >  - there is 1 IRQ for legacy as a parent,
> >  - check an interrupt factor register, and handle the interrupt correspond
> >    to the factor,
> >  - call request_irq() for the parent IRQ and irq_domain_add_linear() for
> >    the factor when initializing
> > 
> > The pcie-uniphier.c is the same type as the latter (like pci-dra7xx.c).
> > 
> > However, in pci-dra7xx.c, MSI and legacy IRQ share the same interrupt number,
> > so the same handler is called and the handler divides these IRQs.
> > (found in dra7xx_pcie_msi_irq_handler())
> > 
> > In pcie-uniphier.c, MSI and legacy IRQ are independent.
> > Therefore it's necessary to prepare the handler for the legacy IRQ.
> > 
> > I think that it's difficult to apply the way of pci-keystone-dw.c, and
> > uniphier_pcie_irq_handler() and calling devm_request_irq() are still
> > necessary to handle legacy IRQ.
> 
> I do not think it is difficult, the difference is that keystone has
> 1 GIC irq line allocated per legacy IRQ, your set-up has one for
> all INTX.

I read other drivers to handle INTX like pcie-mediatek.c, pcie-altera.c,
and so forth, and understood the common way to rewrite it with chained IRQ.

Using irq_set_chained_handler_and_data() instead of devm_request_irq()
and adding chained_irq_{enter, exit}() to the handler can allow INTX
to handle chained IRQ.

Now I can rewrite INTX part with the above way in v3, though,
I'm not sure whether there is a way to generalize INTX of all dwc drivers,
because of the followings:

- the handler needs to read SoC-dependent register to check which an IRQ line
  of {0,1,2,3},
- domain_ops functions are different for each driver
  (using handle_simple_irq or handle_level_irq, etc.)
- the way to get child interrupt-controller node might be different.

Anyway I think we need to leave these topics until clarifying keystone's
linear irq topic.

---
Best Regards,
Kunihiko Hayashi
Kishon Vijay Abraham I Oct. 8, 2018, 5:45 a.m. UTC | #10 
Hi Lorenzo,

On Friday 28 September 2018 09:13 PM, Lorenzo Pieralisi wrote:
> On Fri, Sep 28, 2018 at 02:17:16PM +0100, Marc Zyngier wrote:
>> On 28/09/18 12:06, Lorenzo Pieralisi wrote:
>>> [+Murali, Marc]
>>>
>>> On Thu, Sep 27, 2018 at 04:44:26PM +0900, Kunihiko Hayashi wrote:
>>>> Hi Lorenzo, Gustavo,
>>>>
>>>> On Wed, 26 Sep 2018 21:31:36 +0900 <hayashi.kunihiko@socionext.com> wrote:
>>>>
>>>>> Hi Lorenzo, Gustavo,
>>>>>
>>>>> Thank you for reviewing.
>>>>>
>>>>> On Tue, 25 Sep 2018 18:53:01 +0100
>>>>> Gustavo Pimentel <gustavo.pimentel@synopsys.com> 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.
>>>>
>>>> [snip]
>>>>
>>>>>>>> +	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.
>>>>
>>>> I understand that there are 2 types of interrupt and the drivers.
>>>>
>>>> One like pci-keystone-dw.c is:
>>>>
>>>>  - there are 4 interrupts for legacy,
>>>>  - invoke handlers for each interrupt, and handle the interrupt,
>>>>  - call irq_set_chained_handler_and_data() to make a chain of the interrupts
>>>>   when initializing
>>>>
>>>> The other like pci-dra7xx.c is:
>>>>
>>>>  - there is 1 IRQ for legacy as a parent,
>>>>  - check an interrupt factor register, and handle the interrupt correspond
>>>>    to the factor,
>>>>  - call request_irq() for the parent IRQ and irq_domain_add_linear() for
>>>>    the factor when initializing
>>>>
>>>> The pcie-uniphier.c is the same type as the latter (like pci-dra7xx.c).
>>>>
>>>> However, in pci-dra7xx.c, MSI and legacy IRQ share the same interrupt number,
>>>> so the same handler is called and the handler divides these IRQs.
>>>> (found in dra7xx_pcie_msi_irq_handler())
>>>>
>>>> In pcie-uniphier.c, MSI and legacy IRQ are independent.
>>>> Therefore it's necessary to prepare the handler for the legacy IRQ.
>>>>
>>>> I think that it's difficult to apply the way of pci-keystone-dw.c, and
>>>> uniphier_pcie_irq_handler() and calling devm_request_irq() are still
>>>> necessary to handle legacy IRQ.
>>>
>>> I do not think it is difficult, the difference is that keystone has
>>> 1 GIC irq line allocated per legacy IRQ, your set-up has one for
>>> all INTX.
>>>
>>> *However*, I would like some clarifications from Murali on this code
>>> in drivers/pci/controller/dwc/pci-keystone.c:
>>>
>>> static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
>>> {
>>> 	unsigned int irq = irq_desc_get_irq(desc);
>>> 	struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc);
>>> 	struct dw_pcie *pci = ks_pcie->pci;
>>> 	struct device *dev = pci->dev;
>>> 	u32 irq_offset = irq - ks_pcie->legacy_host_irqs[0];
>>>
>>> Here the IRQ numbers are virtual IRQs, is it correct to consider
>>> the virq numbers as sequential values ? The "offset" is used to
>>> handle the PCI controller interrupt registers, so it must be a value
>>> between 0-3 IIUC.
>>
>> There is absolutely no reason why virtual interrupt numbers should be
>> contiguous. Shake the allocator hard enough, and you'll see gaps appearing.
>>
>> In general, the only thing that makes sense is to compute this offset based
>> on the hwirq which is HW-specific.
> 
> That was my understanding and why I asked, which means that keystone
> code can break (unless I read it wrong) and Murali will send me a fix as
> soon as possible please to get it right (and Kunihiko will base his
> code on this discussion).

I had cleaned up legacy interrupt handling in keystone driver [1] which was
also required for TI's AM654 Platform.

But I guess the same issue will occur in MSI interrupt handling. I'll fix that
up in the next version. Btw can you review [2] so that I can fix any other
comments that you may have.

Thanks
Kishon

[1] -> https://lore.kernel.org/patchwork/patch/989541/
[2] => https://lore.kernel.org/patchwork/cover/989487/
> 
> Lorenzo
> 
>
Lorenzo Pieralisi Oct. 8, 2018, 2:32 p.m. UTC | #11 
On Mon, Oct 08, 2018 at 11:15:59AM +0530, Kishon Vijay Abraham I wrote:
> Hi Lorenzo,
> 
> On Friday 28 September 2018 09:13 PM, Lorenzo Pieralisi wrote:
> > On Fri, Sep 28, 2018 at 02:17:16PM +0100, Marc Zyngier wrote:
> >> On 28/09/18 12:06, Lorenzo Pieralisi wrote:
> >>> [+Murali, Marc]
> >>>
> >>> On Thu, Sep 27, 2018 at 04:44:26PM +0900, Kunihiko Hayashi wrote:
> >>>> Hi Lorenzo, Gustavo,
> >>>>
> >>>> On Wed, 26 Sep 2018 21:31:36 +0900 <hayashi.kunihiko@socionext.com> wrote:
> >>>>
> >>>>> Hi Lorenzo, Gustavo,
> >>>>>
> >>>>> Thank you for reviewing.
> >>>>>
> >>>>> On Tue, 25 Sep 2018 18:53:01 +0100
> >>>>> Gustavo Pimentel <gustavo.pimentel@synopsys.com> 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.
> >>>>
> >>>> [snip]
> >>>>
> >>>>>>>> +	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.
> >>>>
> >>>> I understand that there are 2 types of interrupt and the drivers.
> >>>>
> >>>> One like pci-keystone-dw.c is:
> >>>>
> >>>>  - there are 4 interrupts for legacy,
> >>>>  - invoke handlers for each interrupt, and handle the interrupt,
> >>>>  - call irq_set_chained_handler_and_data() to make a chain of the interrupts
> >>>>   when initializing
> >>>>
> >>>> The other like pci-dra7xx.c is:
> >>>>
> >>>>  - there is 1 IRQ for legacy as a parent,
> >>>>  - check an interrupt factor register, and handle the interrupt correspond
> >>>>    to the factor,
> >>>>  - call request_irq() for the parent IRQ and irq_domain_add_linear() for
> >>>>    the factor when initializing
> >>>>
> >>>> The pcie-uniphier.c is the same type as the latter (like pci-dra7xx.c).
> >>>>
> >>>> However, in pci-dra7xx.c, MSI and legacy IRQ share the same interrupt number,
> >>>> so the same handler is called and the handler divides these IRQs.
> >>>> (found in dra7xx_pcie_msi_irq_handler())
> >>>>
> >>>> In pcie-uniphier.c, MSI and legacy IRQ are independent.
> >>>> Therefore it's necessary to prepare the handler for the legacy IRQ.
> >>>>
> >>>> I think that it's difficult to apply the way of pci-keystone-dw.c, and
> >>>> uniphier_pcie_irq_handler() and calling devm_request_irq() are still
> >>>> necessary to handle legacy IRQ.
> >>>
> >>> I do not think it is difficult, the difference is that keystone has
> >>> 1 GIC irq line allocated per legacy IRQ, your set-up has one for
> >>> all INTX.
> >>>
> >>> *However*, I would like some clarifications from Murali on this code
> >>> in drivers/pci/controller/dwc/pci-keystone.c:
> >>>
> >>> static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
> >>> {
> >>> 	unsigned int irq = irq_desc_get_irq(desc);
> >>> 	struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc);
> >>> 	struct dw_pcie *pci = ks_pcie->pci;
> >>> 	struct device *dev = pci->dev;
> >>> 	u32 irq_offset = irq - ks_pcie->legacy_host_irqs[0];
> >>>
> >>> Here the IRQ numbers are virtual IRQs, is it correct to consider
> >>> the virq numbers as sequential values ? The "offset" is used to
> >>> handle the PCI controller interrupt registers, so it must be a value
> >>> between 0-3 IIUC.
> >>
> >> There is absolutely no reason why virtual interrupt numbers should be
> >> contiguous. Shake the allocator hard enough, and you'll see gaps appearing.
> >>
> >> In general, the only thing that makes sense is to compute this offset based
> >> on the hwirq which is HW-specific.
> > 
> > That was my understanding and why I asked, which means that keystone
> > code can break (unless I read it wrong) and Murali will send me a fix as
> > soon as possible please to get it right (and Kunihiko will base his
> > code on this discussion).
> 
> I had cleaned up legacy interrupt handling in keystone driver [1] which was
> also required for TI's AM654 Platform.
> 
> But I guess the same issue will occur in MSI interrupt handling. I'll fix that
> up in the next version. Btw can you review [2] so that I can fix any other
> comments that you may have.

Hi Kishon,

yes I will, I am getting there (sorry for the delay), I don't think we
can make it v4.20 material but let me first have a look, maybe we can
split it up and simplify its merge.

Thanks,
Lorenzo
Kunihiko Hayashi Oct. 12, 2018, 10:50 a.m. UTC | #12 
Hi Lorenzo, Kishon,

On Mon, 8 Oct 2018 15:32:16 +0100 <lorenzo.pieralisi@arm.com> wrote:

> On Mon, Oct 08, 2018 at 11:15:59AM +0530, Kishon Vijay Abraham I wrote:
> > Hi Lorenzo,
> > 
> > On Friday 28 September 2018 09:13 PM, Lorenzo Pieralisi wrote:
> > > On Fri, Sep 28, 2018 at 02:17:16PM +0100, Marc Zyngier wrote:
> > >> On 28/09/18 12:06, Lorenzo Pieralisi wrote:
> > >>> [+Murali, Marc]
> > >>>
> > >>> On Thu, Sep 27, 2018 at 04:44:26PM +0900, Kunihiko Hayashi wrote:
> > >>>> Hi Lorenzo, Gustavo,
> > >>>>
> > >>>> On Wed, 26 Sep 2018 21:31:36 +0900 <hayashi.kunihiko@socionext.com> wrote:
> > >>>>
> > >>>>> Hi Lorenzo, Gustavo,
> > >>>>>
> > >>>>> Thank you for reviewing.
> > >>>>>
> > >>>>> On Tue, 25 Sep 2018 18:53:01 +0100
> > >>>>> Gustavo Pimentel <gustavo.pimentel@synopsys.com> 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.
> > >>>>
> > >>>> [snip]
> > >>>>
> > >>>>>>>> +	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.
> > >>>>
> > >>>> I understand that there are 2 types of interrupt and the drivers.
> > >>>>
> > >>>> One like pci-keystone-dw.c is:
> > >>>>
> > >>>>  - there are 4 interrupts for legacy,
> > >>>>  - invoke handlers for each interrupt, and handle the interrupt,
> > >>>>  - call irq_set_chained_handler_and_data() to make a chain of the interrupts
> > >>>>   when initializing
> > >>>>
> > >>>> The other like pci-dra7xx.c is:
> > >>>>
> > >>>>  - there is 1 IRQ for legacy as a parent,
> > >>>>  - check an interrupt factor register, and handle the interrupt correspond
> > >>>>    to the factor,
> > >>>>  - call request_irq() for the parent IRQ and irq_domain_add_linear() for
> > >>>>    the factor when initializing
> > >>>>
> > >>>> The pcie-uniphier.c is the same type as the latter (like pci-dra7xx.c).
> > >>>>
> > >>>> However, in pci-dra7xx.c, MSI and legacy IRQ share the same interrupt number,
> > >>>> so the same handler is called and the handler divides these IRQs.
> > >>>> (found in dra7xx_pcie_msi_irq_handler())
> > >>>>
> > >>>> In pcie-uniphier.c, MSI and legacy IRQ are independent.
> > >>>> Therefore it's necessary to prepare the handler for the legacy IRQ.
> > >>>>
> > >>>> I think that it's difficult to apply the way of pci-keystone-dw.c, and
> > >>>> uniphier_pcie_irq_handler() and calling devm_request_irq() are still
> > >>>> necessary to handle legacy IRQ.
> > >>>
> > >>> I do not think it is difficult, the difference is that keystone has
> > >>> 1 GIC irq line allocated per legacy IRQ, your set-up has one for
> > >>> all INTX.
> > >>>
> > >>> *However*, I would like some clarifications from Murali on this code
> > >>> in drivers/pci/controller/dwc/pci-keystone.c:
> > >>>
> > >>> static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
> > >>> {
> > >>> 	unsigned int irq = irq_desc_get_irq(desc);
> > >>> 	struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc);
> > >>> 	struct dw_pcie *pci = ks_pcie->pci;
> > >>> 	struct device *dev = pci->dev;
> > >>> 	u32 irq_offset = irq - ks_pcie->legacy_host_irqs[0];
> > >>>
> > >>> Here the IRQ numbers are virtual IRQs, is it correct to consider
> > >>> the virq numbers as sequential values ? The "offset" is used to
> > >>> handle the PCI controller interrupt registers, so it must be a value
> > >>> between 0-3 IIUC.
> > >>
> > >> There is absolutely no reason why virtual interrupt numbers should be
> > >> contiguous. Shake the allocator hard enough, and you'll see gaps appearing.
> > >>
> > >> In general, the only thing that makes sense is to compute this offset based
> > >> on the hwirq which is HW-specific.
> > > 
> > > That was my understanding and why I asked, which means that keystone
> > > code can break (unless I read it wrong) and Murali will send me a fix as
> > > soon as possible please to get it right (and Kunihiko will base his
> > > code on this discussion).
> > 
> > I had cleaned up legacy interrupt handling in keystone driver [1] which was
> > also required for TI's AM654 Platform.
> > 
> > But I guess the same issue will occur in MSI interrupt handling. I'll fix that
> > up in the next version. Btw can you review [2] so that I can fix any other
> > comments that you may have.
> 
> Hi Kishon,
> 
> yes I will, I am getting there (sorry for the delay), I don't think we
> can make it v4.20 material but let me first have a look, maybe we can
> split it up and simplify its merge.

Thank you for introducing RFC patches.
I saw the legacy interrupt part of them and I think that these became easier
to reference. Especially irq_domain_ops.map() will be the same.

Currently candidate pcie-uniphier driver and some ones are assuming to take
an interrupt from "interrupts" property because of using single interrupt.

	interrupt-name = "intx";
	interrupts = <xxx>;

However, keystone has 4 interrupts and Kishon's patch is assuming
to take it from the property in child "legacy-interrupt-controller" node.
Maybe latter is more general and I can apply it.

	legacy-interrupt-controller {
		interrupt-controller;
		#interrupt-cells = <1>;
		interrupt-parent = <&gic>;
		interrupts = <xxx>;
	};

I'll send v3 patch with reference to these RFC patches for now.

Thank you,

---
Best Regards,
Kunihiko Hayashi
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index 91b0194..d8fdb02 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -193,4 +193,13 @@  config PCIE_HISI_STB
 	help
           Say Y here if you want PCIe controller support on HiSilicon STB SoCs
 
+config PCIE_UNIPHIER
+	bool "Socionext UniPhier PCIe controllers"
+	depends on OF && (ARCH_UNIPHIER || COMPILE_TEST)
+	depends on PCI_MSI_IRQ_DOMAIN
+	select PCIE_DW_HOST
+	help
+	  Say Y here if you want PCIe controller support on UniPhier SoCs.
+	  This driver supports LD20 and PXs3 SoCs.
+
 endmenu
diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
index 5d2ce72..cbde733 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -14,6 +14,7 @@  obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
 obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
 obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o
 obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o
+obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
 
 # The following drivers are for devices that use the generic ACPI
 # pci_root.c driver but don't support standard ECAM config access.
diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
new file mode 100644
index 0000000..4090967
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-uniphier.c
@@ -0,0 +1,465 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PCIe host controller driver for UniPhier SoCs
+ * Copyright 2018 Socionext Inc.
+ * Author: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/iopoll.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+
+#include "pcie-designware.h"
+
+#define PCL_PINCTRL0			0x002c
+#define PCL_PERST_PLDN_REGEN		BIT(12)
+#define PCL_PERST_NOE_REGEN		BIT(11)
+#define PCL_PERST_OUT_REGEN		BIT(8)
+#define PCL_PERST_PLDN_REGVAL		BIT(4)
+#define PCL_PERST_NOE_REGVAL		BIT(3)
+#define PCL_PERST_OUT_REGVAL		BIT(0)
+
+#define PCL_PIPEMON			0x0044
+#define PCL_PCLK_ALIVE			BIT(15)
+
+#define PCL_APP_READY_CTRL		0x8008
+#define PCL_APP_LTSSM_ENABLE		BIT(0)
+
+#define PCL_APP_PM0			0x8078
+#define PCL_SYS_AUX_PWR_DET		BIT(8)
+
+#define PCL_RCV_INT			0x8108
+#define PCL_CFG_BW_MGT_ENABLE		BIT(20)
+#define PCL_CFG_LINK_AUTO_BW_ENABLE	BIT(19)
+#define PCL_CFG_AER_RC_ERR_MSI_ENABLE	BIT(18)
+#define PCL_CFG_PME_MSI_ENABLE		BIT(17)
+#define PCL_CFG_BW_MGT_STATUS		BIT(4)
+#define PCL_CFG_LINK_AUTO_BW_STATUS	BIT(3)
+#define PCL_CFG_AER_RC_ERR_MSI_STATUS	BIT(2)
+#define PCL_CFG_PME_MSI_STATUS		BIT(1)
+#define PCL_RCV_INT_ALL_ENABLE			\
+	(PCL_CFG_BW_MGT_ENABLE | PCL_CFG_LINK_AUTO_BW_ENABLE \
+	 | PCL_CFG_AER_RC_ERR_MSI_ENABLE | PCL_CFG_PME_MSI_ENABLE)
+
+#define PCL_RCV_INTX			0x810c
+#define PCL_RADM_INTD_ENABLE		BIT(19)
+#define PCL_RADM_INTC_ENABLE		BIT(18)
+#define PCL_RADM_INTB_ENABLE		BIT(17)
+#define PCL_RADM_INTA_ENABLE		BIT(16)
+#define PCL_RADM_INTD_STATUS		BIT(3)
+#define PCL_RADM_INTC_STATUS		BIT(2)
+#define PCL_RADM_INTB_STATUS		BIT(1)
+#define PCL_RADM_INTA_STATUS		BIT(0)
+#define PCL_RCV_INTX_ALL_ENABLE			\
+	(PCL_RADM_INTD_ENABLE | PCL_RADM_INTC_ENABLE \
+	 | PCL_RADM_INTB_ENABLE	| PCL_RADM_INTA_ENABLE)
+
+#define PCL_STATUS_LINK			0x8140
+#define PCL_RDLH_LINK_UP		BIT(1)
+#define PCL_XMLH_LINK_UP		BIT(0)
+
+struct uniphier_pcie_priv {
+	void __iomem *base;
+	struct dw_pcie pci;
+	struct clk *clk;
+	struct reset_control *rst;
+	struct phy *phy;
+	struct irq_domain *irq_domain;
+};
+
+#define to_uniphier_pcie(x)	dev_get_drvdata((x)->dev)
+
+static void uniphier_pcie_ltssm_enable(struct uniphier_pcie_priv *priv)
+{
+	u32 val;
+
+	val = readl(priv->base + PCL_APP_READY_CTRL);
+	val |= PCL_APP_LTSSM_ENABLE;
+	writel(val, priv->base + PCL_APP_READY_CTRL);
+}
+
+static void uniphier_pcie_ltssm_disable(struct uniphier_pcie_priv *priv)
+{
+	u32 val;
+
+	val = readl(priv->base + PCL_APP_READY_CTRL);
+	val &= ~PCL_APP_LTSSM_ENABLE;
+	writel(val, priv->base + PCL_APP_READY_CTRL);
+}
+
+static void uniphier_pcie_init_rc(struct uniphier_pcie_priv *priv)
+{
+	u32 val;
+
+	/* use auxiliary power detection */
+	val = readl(priv->base + PCL_APP_PM0);
+	val |= PCL_SYS_AUX_PWR_DET;
+	writel(val, priv->base + PCL_APP_PM0);
+
+	/* assert PERST# */
+	val = readl(priv->base + PCL_PINCTRL0);
+	val &= ~(PCL_PERST_NOE_REGVAL | PCL_PERST_OUT_REGVAL
+		 | PCL_PERST_PLDN_REGVAL);
+	val |= PCL_PERST_NOE_REGEN | PCL_PERST_OUT_REGEN
+		| PCL_PERST_PLDN_REGEN;
+	writel(val, priv->base + PCL_PINCTRL0);
+
+	uniphier_pcie_ltssm_disable(priv);
+
+	usleep_range(100000, 200000);
+
+	/* deassert PERST# */
+	val = readl(priv->base + PCL_PINCTRL0);
+	val |= PCL_PERST_OUT_REGVAL | PCL_PERST_OUT_REGEN;
+	writel(val, priv->base + PCL_PINCTRL0);
+}
+
+static int uniphier_pcie_wait_rc(struct uniphier_pcie_priv *priv)
+{
+	u32 status;
+	int ret;
+
+	/* wait PIPE clock */
+	ret = readl_poll_timeout(priv->base + PCL_PIPEMON, status,
+				 status & PCL_PCLK_ALIVE, 100000, 1000000);
+	if (ret) {
+		dev_err(priv->pci.dev,
+			"Failed to initialize controller in RC mode\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+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;
+	}
+
+	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,
+};
+
+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)
+{
+	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));
+
+	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);
+}
+
+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;
+	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);
+	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);
+		if (ret) {
+			dev_err(dev, "failed to request msi_irq %d\n",
+				pp->msi_irq);
+			return ret;
+		}
+	}
+
+	ret = dw_pcie_host_init(pp);
+	if (ret) {
+		dev_err(dev, "Failed to initialize host (%d)\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int uniphier_pcie_host_enable(struct uniphier_pcie_priv *priv)
+{
+	int ret;
+
+	ret = clk_prepare_enable(priv->clk);
+	if (ret)
+		return ret;
+
+	ret = reset_control_deassert(priv->rst);
+	if (ret)
+		goto out_clk_disable;
+
+	uniphier_pcie_init_rc(priv);
+
+	ret = phy_init(priv->phy);
+	if (ret)
+		goto out_rst_assert;
+
+	ret = uniphier_pcie_wait_rc(priv);
+	if (ret)
+		goto out_phy_exit;
+
+	uniphier_pcie_irq_enable(priv);
+
+	return 0;
+
+out_phy_exit:
+	phy_exit(priv->phy);
+out_rst_assert:
+	reset_control_assert(priv->rst);
+out_clk_disable:
+	clk_disable_unprepare(priv->clk);
+
+	return ret;
+}
+
+static void uniphier_pcie_host_disable(struct uniphier_pcie_priv *priv)
+{
+	uniphier_pcie_irq_disable(priv);
+	phy_exit(priv->phy);
+	reset_control_assert(priv->rst);
+	clk_disable_unprepare(priv->clk);
+}
+
+static const struct dw_pcie_ops dw_pcie_ops = {
+	.start_link = uniphier_pcie_establish_link,
+	.stop_link = uniphier_pcie_stop_link,
+	.link_up = uniphier_pcie_link_up,
+};
+
+static int uniphier_pcie_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct uniphier_pcie_priv *priv;
+	struct resource *res;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->pci.dev = dev;
+	priv->pci.ops = &dw_pcie_ops;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
+	priv->pci.dbi_base = devm_pci_remap_cfg_resource(dev, res);
+	if (IS_ERR(priv->pci.dbi_base))
+		return PTR_ERR(priv->pci.dbi_base);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "link");
+	priv->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	priv->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(priv->clk))
+		return PTR_ERR(priv->clk);
+
+	priv->rst = devm_reset_control_get_shared(dev, NULL);
+	if (IS_ERR(priv->rst))
+		return PTR_ERR(priv->rst);
+
+	priv->phy = devm_phy_optional_get(dev, "pcie-phy");
+	if (IS_ERR(priv->phy))
+		return PTR_ERR(priv->phy);
+
+	platform_set_drvdata(pdev, priv);
+
+	ret = uniphier_pcie_host_enable(priv);
+	if (ret)
+		return ret;
+
+	return uniphier_add_pcie_port(priv, pdev);
+}
+
+static int uniphier_pcie_remove(struct platform_device *pdev)
+{
+	struct uniphier_pcie_priv *priv = platform_get_drvdata(pdev);
+
+	uniphier_pcie_host_disable(priv);
+
+	return 0;
+}
+
+static const struct of_device_id uniphier_pcie_match[] = {
+	{ .compatible = "socionext,uniphier-pcie", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, uniphier_pcie_match);
+
+static struct platform_driver uniphier_pcie_driver = {
+	.probe  = uniphier_pcie_probe,
+	.remove = uniphier_pcie_remove,
+	.driver = {
+		.name = "uniphier-pcie",
+		.of_match_table = uniphier_pcie_match,
+	},
+};
+builtin_platform_driver(uniphier_pcie_driver);
+
+MODULE_AUTHOR("Kunihiko Hayashi <hayashi.kunihiko@socionext.com>");
+MODULE_DESCRIPTION("UniPhier PCIe host controller driver");
+MODULE_LICENSE("GPL v2");