[PATCH v2 1/1] PCI: layerscape: Add the endpoint linkup notifier support

[PATCH v2 1/1] PCI: layerscape: Add the endpoint linkup notifier support

[Frank Li]

Layerscape has PME interrupt, which can be used as linkup notifier.
Set CFG_READY bit when linkup detected.

Signed-off-by: Xiaowei Bao <xiaowei.bao@nxp.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change from v1 to v2
- pme -> PME
- irq -> IRQ
- update dev_info message according to Bjorn's suggestion
- remove '.' at error message
	
 .../pci/controller/dwc/pci-layerscape-ep.c    | 104 +++++++++++++++++-
 1 file changed, 103 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c b/drivers/pci/controller/dwc/pci-layerscape-ep.c
index c640db60edc6..e974fbe3b6d8 100644
--- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
+++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
@@ -18,6 +18,20 @@
 
 #include "pcie-designware.h"
 
+#define PEX_PF0_CONFIG			0xC0014
+#define PEX_PF0_CFG_READY		BIT(0)
+
+/* PEX PFa PCIE PME and message interrupt registers*/
+#define PEX_PF0_PME_MES_DR		0xC0020
+#define PEX_PF0_PME_MES_DR_LUD		BIT(7)
+#define PEX_PF0_PME_MES_DR_LDD		BIT(9)
+#define PEX_PF0_PME_MES_DR_HRD		BIT(10)
+
+#define PEX_PF0_PME_MES_IER		0xC0028
+#define PEX_PF0_PME_MES_IER_LUDIE	BIT(7)
+#define PEX_PF0_PME_MES_IER_LDDIE	BIT(9)
+#define PEX_PF0_PME_MES_IER_HRDIE	BIT(10)
+
 #define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
 
 struct ls_pcie_ep_drvdata {
@@ -30,8 +44,88 @@ struct ls_pcie_ep {
 	struct dw_pcie			*pci;
 	struct pci_epc_features		*ls_epc;
 	const struct ls_pcie_ep_drvdata *drvdata;
+	bool				big_endian;
+	int				irq;
 };
 
+static u32 ls_lut_readl(struct ls_pcie_ep *pcie, u32 offset)
+{
+	struct dw_pcie *pci = pcie->pci;
+
+	if (pcie->big_endian)
+		return ioread32be(pci->dbi_base + offset);
+	else
+		return ioread32(pci->dbi_base + offset);
+}
+
+static void ls_lut_writel(struct ls_pcie_ep *pcie, u32 offset,
+			  u32 value)
+{
+	struct dw_pcie *pci = pcie->pci;
+
+	if (pcie->big_endian)
+		iowrite32be(value, pci->dbi_base + offset);
+	else
+		iowrite32(value, pci->dbi_base + offset);
+}
+
+static irqreturn_t ls_pcie_ep_event_handler(int irq, void *dev_id)
+{
+	struct ls_pcie_ep *pcie = (struct ls_pcie_ep *)dev_id;
+	struct dw_pcie *pci = pcie->pci;
+	u32 val, cfg;
+
+	val = ls_lut_readl(pcie, PEX_PF0_PME_MES_DR);
+	if (!val)
+		return IRQ_NONE;
+
+	if (val & PEX_PF0_PME_MES_DR_LUD) {
+		cfg = ls_lut_readl(pcie, PEX_PF0_CONFIG);
+		cfg |= PEX_PF0_CFG_READY;
+		ls_lut_writel(pcie, PEX_PF0_CONFIG, cfg);
+		dw_pcie_ep_linkup(&pci->ep);
+
+		dev_info(pci->dev, "Link up\n");
+	} else if (val & PEX_PF0_PME_MES_DR_LDD) {
+		dev_info(pci->dev, "Link down\n");
+	} else if (val & PEX_PF0_PME_MES_DR_HRD) {
+		dev_info(pci->dev, "Hot reset\n");
+	}
+
+	ls_lut_writel(pcie, PEX_PF0_PME_MES_DR, val);
+
+	return IRQ_HANDLED;
+}
+
+static int ls_pcie_ep_interrupt_init(struct ls_pcie_ep *pcie,
+				     struct platform_device *pdev)
+{
+	u32 val;
+	int ret;
+
+	pcie->irq = platform_get_irq_byname(pdev, "pme");
+	if (pcie->irq < 0) {
+		dev_err(&pdev->dev, "Can't get 'pme' IRQ\n");
+		return pcie->irq;
+	}
+
+	ret = devm_request_irq(&pdev->dev, pcie->irq,
+			       ls_pcie_ep_event_handler, IRQF_SHARED,
+			       pdev->name, pcie);
+	if (ret) {
+		dev_err(&pdev->dev, "Can't register PCIe IRQ\n");
+		return ret;
+	}
+
+	/* Enable interrupts */
+	val = ls_lut_readl(pcie, PEX_PF0_PME_MES_IER);
+	val |=  PEX_PF0_PME_MES_IER_LDDIE | PEX_PF0_PME_MES_IER_HRDIE |
+		PEX_PF0_PME_MES_IER_LUDIE;
+	ls_lut_writel(pcie, PEX_PF0_PME_MES_IER, val);
+
+	return 0;
+}
+
 static const struct pci_epc_features*
 ls_pcie_ep_get_features(struct dw_pcie_ep *ep)
 {
@@ -125,6 +219,7 @@ static int __init ls_pcie_ep_probe(struct platform_device *pdev)
 	struct ls_pcie_ep *pcie;
 	struct pci_epc_features *ls_epc;
 	struct resource *dbi_base;
+	int ret;
 
 	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
 	if (!pcie)
@@ -144,6 +239,7 @@ static int __init ls_pcie_ep_probe(struct platform_device *pdev)
 	pci->ops = pcie->drvdata->dw_pcie_ops;
 
 	ls_epc->bar_fixed_64bit = (1 << BAR_2) | (1 << BAR_4);
+	ls_epc->linkup_notifier = true;
 
 	pcie->pci = pci;
 	pcie->ls_epc = ls_epc;
@@ -155,9 +251,15 @@ static int __init ls_pcie_ep_probe(struct platform_device *pdev)
 
 	pci->ep.ops = &ls_pcie_ep_ops;
 
+	pcie->big_endian = of_property_read_bool(dev->of_node, "big-endian");
+
 	platform_set_drvdata(pdev, pcie);
 
-	return dw_pcie_ep_init(&pci->ep);
+	ret = dw_pcie_ep_init(&pci->ep);
+	if (ret)
+		return  ret;
+
+	return  ls_pcie_ep_interrupt_init(pcie, pdev);
 }
 
 static struct platform_driver ls_pcie_ep_driver = {
-- 
2.34.1

[PATCH v2 1/1] PCI: layerscape: Add the endpoint linkup notifier support

[Frank Li]

Layerscape has PME interrupt, which can be used as linkup notifier.
Set CFG_READY bit when linkup detected.

Signed-off-by: Xiaowei Bao <xiaowei.bao@nxp.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change from v1 to v2
- pme -> PME
- irq -> IRQ
- update dev_info message according to Bjorn's suggestion
- remove '.' at error message
	
 .../pci/controller/dwc/pci-layerscape-ep.c    | 104 +++++++++++++++++-
 1 file changed, 103 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c b/drivers/pci/controller/dwc/pci-layerscape-ep.c
index c640db60edc6..e974fbe3b6d8 100644
--- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
+++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
@@ -18,6 +18,20 @@
 
 #include "pcie-designware.h"
 
+#define PEX_PF0_CONFIG			0xC0014
+#define PEX_PF0_CFG_READY		BIT(0)
+
+/* PEX PFa PCIE PME and message interrupt registers*/
+#define PEX_PF0_PME_MES_DR		0xC0020
+#define PEX_PF0_PME_MES_DR_LUD		BIT(7)
+#define PEX_PF0_PME_MES_DR_LDD		BIT(9)
+#define PEX_PF0_PME_MES_DR_HRD		BIT(10)
+
+#define PEX_PF0_PME_MES_IER		0xC0028
+#define PEX_PF0_PME_MES_IER_LUDIE	BIT(7)
+#define PEX_PF0_PME_MES_IER_LDDIE	BIT(9)
+#define PEX_PF0_PME_MES_IER_HRDIE	BIT(10)
+
 #define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
 
 struct ls_pcie_ep_drvdata {
@@ -30,8 +44,88 @@ struct ls_pcie_ep {
 	struct dw_pcie			*pci;
 	struct pci_epc_features		*ls_epc;
 	const struct ls_pcie_ep_drvdata *drvdata;
+	bool				big_endian;
+	int				irq;
 };
 
+static u32 ls_lut_readl(struct ls_pcie_ep *pcie, u32 offset)
+{
+	struct dw_pcie *pci = pcie->pci;
+
+	if (pcie->big_endian)
+		return ioread32be(pci->dbi_base + offset);
+	else
+		return ioread32(pci->dbi_base + offset);
+}
+
+static void ls_lut_writel(struct ls_pcie_ep *pcie, u32 offset,
+			  u32 value)
+{
+	struct dw_pcie *pci = pcie->pci;
+
+	if (pcie->big_endian)
+		iowrite32be(value, pci->dbi_base + offset);
+	else
+		iowrite32(value, pci->dbi_base + offset);
+}
+
+static irqreturn_t ls_pcie_ep_event_handler(int irq, void *dev_id)
+{
+	struct ls_pcie_ep *pcie = (struct ls_pcie_ep *)dev_id;
+	struct dw_pcie *pci = pcie->pci;
+	u32 val, cfg;
+
+	val = ls_lut_readl(pcie, PEX_PF0_PME_MES_DR);
+	if (!val)
+		return IRQ_NONE;
+
+	if (val & PEX_PF0_PME_MES_DR_LUD) {
+		cfg = ls_lut_readl(pcie, PEX_PF0_CONFIG);
+		cfg |= PEX_PF0_CFG_READY;
+		ls_lut_writel(pcie, PEX_PF0_CONFIG, cfg);
+		dw_pcie_ep_linkup(&pci->ep);
+
+		dev_info(pci->dev, "Link up\n");
+	} else if (val & PEX_PF0_PME_MES_DR_LDD) {
+		dev_info(pci->dev, "Link down\n");
+	} else if (val & PEX_PF0_PME_MES_DR_HRD) {
+		dev_info(pci->dev, "Hot reset\n");
+	}
+
+	ls_lut_writel(pcie, PEX_PF0_PME_MES_DR, val);
+
+	return IRQ_HANDLED;
+}
+
+static int ls_pcie_ep_interrupt_init(struct ls_pcie_ep *pcie,
+				     struct platform_device *pdev)
+{
+	u32 val;
+	int ret;
+
+	pcie->irq = platform_get_irq_byname(pdev, "pme");
+	if (pcie->irq < 0) {
+		dev_err(&pdev->dev, "Can't get 'pme' IRQ\n");
+		return pcie->irq;
+	}
+
+	ret = devm_request_irq(&pdev->dev, pcie->irq,
+			       ls_pcie_ep_event_handler, IRQF_SHARED,
+			       pdev->name, pcie);
+	if (ret) {
+		dev_err(&pdev->dev, "Can't register PCIe IRQ\n");
+		return ret;
+	}
+
+	/* Enable interrupts */
+	val = ls_lut_readl(pcie, PEX_PF0_PME_MES_IER);
+	val |=  PEX_PF0_PME_MES_IER_LDDIE | PEX_PF0_PME_MES_IER_HRDIE |
+		PEX_PF0_PME_MES_IER_LUDIE;
+	ls_lut_writel(pcie, PEX_PF0_PME_MES_IER, val);
+
+	return 0;
+}
+
 static const struct pci_epc_features*
 ls_pcie_ep_get_features(struct dw_pcie_ep *ep)
 {
@@ -125,6 +219,7 @@ static int __init ls_pcie_ep_probe(struct platform_device *pdev)
 	struct ls_pcie_ep *pcie;
 	struct pci_epc_features *ls_epc;
 	struct resource *dbi_base;
+	int ret;
 
 	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
 	if (!pcie)
@@ -144,6 +239,7 @@ static int __init ls_pcie_ep_probe(struct platform_device *pdev)
 	pci->ops = pcie->drvdata->dw_pcie_ops;
 
 	ls_epc->bar_fixed_64bit = (1 << BAR_2) | (1 << BAR_4);
+	ls_epc->linkup_notifier = true;
 
 	pcie->pci = pci;
 	pcie->ls_epc = ls_epc;
@@ -155,9 +251,15 @@ static int __init ls_pcie_ep_probe(struct platform_device *pdev)
 
 	pci->ep.ops = &ls_pcie_ep_ops;
 
+	pcie->big_endian = of_property_read_bool(dev->of_node, "big-endian");
+
 	platform_set_drvdata(pdev, pcie);
 
-	return dw_pcie_ep_init(&pci->ep);
+	ret = dw_pcie_ep_init(&pci->ep);
+	if (ret)
+		return  ret;
+
+	return  ls_pcie_ep_interrupt_init(pcie, pdev);
 }
 
 static struct platform_driver ls_pcie_ep_driver = {
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/1] PCI: layerscape: Add the endpoint linkup notifier support

[Manivannan Sadhasivam]

On Mon, May 01, 2023 at 10:48:06AM -0400, Frank Li wrote:
> Layerscape has PME interrupt, which can be used as linkup notifier.
> Set CFG_READY bit when linkup detected.

Where are you setting this bit?

> 
> Signed-off-by: Xiaowei Bao <xiaowei.bao@nxp.com>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> Change from v1 to v2
> - pme -> PME
> - irq -> IRQ
> - update dev_info message according to Bjorn's suggestion
> - remove '.' at error message
> 	
>  .../pci/controller/dwc/pci-layerscape-ep.c    | 104 +++++++++++++++++-
>  1 file changed, 103 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> index c640db60edc6..e974fbe3b6d8 100644
> --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> @@ -18,6 +18,20 @@
>  
>  #include "pcie-designware.h"
>  
> +#define PEX_PF0_CONFIG			0xC0014
> +#define PEX_PF0_CFG_READY		BIT(0)
> +
> +/* PEX PFa PCIE PME and message interrupt registers*/
> +#define PEX_PF0_PME_MES_DR		0xC0020
> +#define PEX_PF0_PME_MES_DR_LUD		BIT(7)
> +#define PEX_PF0_PME_MES_DR_LDD		BIT(9)
> +#define PEX_PF0_PME_MES_DR_HRD		BIT(10)
> +
> +#define PEX_PF0_PME_MES_IER		0xC0028
> +#define PEX_PF0_PME_MES_IER_LUDIE	BIT(7)
> +#define PEX_PF0_PME_MES_IER_LDDIE	BIT(9)
> +#define PEX_PF0_PME_MES_IER_HRDIE	BIT(10)
> +
>  #define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
>  
>  struct ls_pcie_ep_drvdata {
> @@ -30,8 +44,88 @@ struct ls_pcie_ep {
>  	struct dw_pcie			*pci;
>  	struct pci_epc_features		*ls_epc;
>  	const struct ls_pcie_ep_drvdata *drvdata;
> +	bool				big_endian;
> +	int				irq;
>  };
>  
> +static u32 ls_lut_readl(struct ls_pcie_ep *pcie, u32 offset)
> +{
> +	struct dw_pcie *pci = pcie->pci;
> +
> +	if (pcie->big_endian)
> +		return ioread32be(pci->dbi_base + offset);
> +	else
> +		return ioread32(pci->dbi_base + offset);
> +}
> +
> +static void ls_lut_writel(struct ls_pcie_ep *pcie, u32 offset,
> +			  u32 value)

Above function argument could be wrapped within 80 columns.

> +{
> +	struct dw_pcie *pci = pcie->pci;
> +
> +	if (pcie->big_endian)
> +		iowrite32be(value, pci->dbi_base + offset);
> +	else
> +		iowrite32(value, pci->dbi_base + offset);
> +}
> +
> +static irqreturn_t ls_pcie_ep_event_handler(int irq, void *dev_id)
> +{
> +	struct ls_pcie_ep *pcie = (struct ls_pcie_ep *)dev_id;

No need to do explicit typecase for void pointer.

> +	struct dw_pcie *pci = pcie->pci;
> +	u32 val, cfg;
> +
> +	val = ls_lut_readl(pcie, PEX_PF0_PME_MES_DR);
> +	if (!val)
> +		return IRQ_NONE;
> +
> +	if (val & PEX_PF0_PME_MES_DR_LUD) {
> +		cfg = ls_lut_readl(pcie, PEX_PF0_CONFIG);
> +		cfg |= PEX_PF0_CFG_READY;
> +		ls_lut_writel(pcie, PEX_PF0_CONFIG, cfg);
> +		dw_pcie_ep_linkup(&pci->ep);
> +
> +		dev_info(pci->dev, "Link up\n");

These messages could be demoted to dev_dbg() logs.

> +	} else if (val & PEX_PF0_PME_MES_DR_LDD) {
> +		dev_info(pci->dev, "Link down\n");
> +	} else if (val & PEX_PF0_PME_MES_DR_HRD) {
> +		dev_info(pci->dev, "Hot reset\n");
> +	}
> +
> +	ls_lut_writel(pcie, PEX_PF0_PME_MES_DR, val);

You should clear the interrupts before processing.

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ls_pcie_ep_interrupt_init(struct ls_pcie_ep *pcie,
> +				     struct platform_device *pdev)
> +{
> +	u32 val;
> +	int ret;
> +
> +	pcie->irq = platform_get_irq_byname(pdev, "pme");
> +	if (pcie->irq < 0) {
> +		dev_err(&pdev->dev, "Can't get 'pme' IRQ\n");

PME

> +		return pcie->irq;
> +	}
> +
> +	ret = devm_request_irq(&pdev->dev, pcie->irq,
> +			       ls_pcie_ep_event_handler, IRQF_SHARED,
> +			       pdev->name, pcie);

Again, please wrap to fit the 80 column width.

> +	if (ret) {
> +		dev_err(&pdev->dev, "Can't register PCIe IRQ\n");
> +		return ret;
> +	}
> +
> +	/* Enable interrupts */
> +	val = ls_lut_readl(pcie, PEX_PF0_PME_MES_IER);
> +	val |=  PEX_PF0_PME_MES_IER_LDDIE | PEX_PF0_PME_MES_IER_HRDIE |
> +		PEX_PF0_PME_MES_IER_LUDIE;
> +	ls_lut_writel(pcie, PEX_PF0_PME_MES_IER, val);
> +
> +	return 0;
> +}
> +
>  static const struct pci_epc_features*
>  ls_pcie_ep_get_features(struct dw_pcie_ep *ep)
>  {
> @@ -125,6 +219,7 @@ static int __init ls_pcie_ep_probe(struct platform_device *pdev)
>  	struct ls_pcie_ep *pcie;
>  	struct pci_epc_features *ls_epc;
>  	struct resource *dbi_base;
> +	int ret;
>  
>  	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
>  	if (!pcie)
> @@ -144,6 +239,7 @@ static int __init ls_pcie_ep_probe(struct platform_device *pdev)
>  	pci->ops = pcie->drvdata->dw_pcie_ops;
>  
>  	ls_epc->bar_fixed_64bit = (1 << BAR_2) | (1 << BAR_4);
> +	ls_epc->linkup_notifier = true;
>  
>  	pcie->pci = pci;
>  	pcie->ls_epc = ls_epc;
> @@ -155,9 +251,15 @@ static int __init ls_pcie_ep_probe(struct platform_device *pdev)
>  
>  	pci->ep.ops = &ls_pcie_ep_ops;
>  
> +	pcie->big_endian = of_property_read_bool(dev->of_node, "big-endian");
> +
>  	platform_set_drvdata(pdev, pcie);
>  
> -	return dw_pcie_ep_init(&pci->ep);
> +	ret = dw_pcie_ep_init(&pci->ep);
> +	if (ret)
> +		return  ret;

Double space after return.

> +
> +	return  ls_pcie_ep_interrupt_init(pcie, pdev);

Double space after return.

- Mani

>  }
>  
>  static struct platform_driver ls_pcie_ep_driver = {
> -- 
> 2.34.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 1/1] PCI: layerscape: Add the endpoint linkup notifier support

[Manivannan Sadhasivam]

On Mon, May 01, 2023 at 10:48:06AM -0400, Frank Li wrote:
> Layerscape has PME interrupt, which can be used as linkup notifier.
> Set CFG_READY bit when linkup detected.

Where are you setting this bit?

> 
> Signed-off-by: Xiaowei Bao <xiaowei.bao@nxp.com>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> Change from v1 to v2
> - pme -> PME
> - irq -> IRQ
> - update dev_info message according to Bjorn's suggestion
> - remove '.' at error message
> 	
>  .../pci/controller/dwc/pci-layerscape-ep.c    | 104 +++++++++++++++++-
>  1 file changed, 103 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> index c640db60edc6..e974fbe3b6d8 100644
> --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> @@ -18,6 +18,20 @@
>  
>  #include "pcie-designware.h"
>  
> +#define PEX_PF0_CONFIG			0xC0014
> +#define PEX_PF0_CFG_READY		BIT(0)
> +
> +/* PEX PFa PCIE PME and message interrupt registers*/
> +#define PEX_PF0_PME_MES_DR		0xC0020
> +#define PEX_PF0_PME_MES_DR_LUD		BIT(7)
> +#define PEX_PF0_PME_MES_DR_LDD		BIT(9)
> +#define PEX_PF0_PME_MES_DR_HRD		BIT(10)
> +
> +#define PEX_PF0_PME_MES_IER		0xC0028
> +#define PEX_PF0_PME_MES_IER_LUDIE	BIT(7)
> +#define PEX_PF0_PME_MES_IER_LDDIE	BIT(9)
> +#define PEX_PF0_PME_MES_IER_HRDIE	BIT(10)
> +
>  #define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
>  
>  struct ls_pcie_ep_drvdata {
> @@ -30,8 +44,88 @@ struct ls_pcie_ep {
>  	struct dw_pcie			*pci;
>  	struct pci_epc_features		*ls_epc;
>  	const struct ls_pcie_ep_drvdata *drvdata;
> +	bool				big_endian;
> +	int				irq;
>  };
>  
> +static u32 ls_lut_readl(struct ls_pcie_ep *pcie, u32 offset)
> +{
> +	struct dw_pcie *pci = pcie->pci;
> +
> +	if (pcie->big_endian)
> +		return ioread32be(pci->dbi_base + offset);
> +	else
> +		return ioread32(pci->dbi_base + offset);
> +}
> +
> +static void ls_lut_writel(struct ls_pcie_ep *pcie, u32 offset,
> +			  u32 value)

Above function argument could be wrapped within 80 columns.

> +{
> +	struct dw_pcie *pci = pcie->pci;
> +
> +	if (pcie->big_endian)
> +		iowrite32be(value, pci->dbi_base + offset);
> +	else
> +		iowrite32(value, pci->dbi_base + offset);
> +}
> +
> +static irqreturn_t ls_pcie_ep_event_handler(int irq, void *dev_id)
> +{
> +	struct ls_pcie_ep *pcie = (struct ls_pcie_ep *)dev_id;

No need to do explicit typecase for void pointer.

> +	struct dw_pcie *pci = pcie->pci;
> +	u32 val, cfg;
> +
> +	val = ls_lut_readl(pcie, PEX_PF0_PME_MES_DR);
> +	if (!val)
> +		return IRQ_NONE;
> +
> +	if (val & PEX_PF0_PME_MES_DR_LUD) {
> +		cfg = ls_lut_readl(pcie, PEX_PF0_CONFIG);
> +		cfg |= PEX_PF0_CFG_READY;
> +		ls_lut_writel(pcie, PEX_PF0_CONFIG, cfg);
> +		dw_pcie_ep_linkup(&pci->ep);
> +
> +		dev_info(pci->dev, "Link up\n");

These messages could be demoted to dev_dbg() logs.

> +	} else if (val & PEX_PF0_PME_MES_DR_LDD) {
> +		dev_info(pci->dev, "Link down\n");
> +	} else if (val & PEX_PF0_PME_MES_DR_HRD) {
> +		dev_info(pci->dev, "Hot reset\n");
> +	}
> +
> +	ls_lut_writel(pcie, PEX_PF0_PME_MES_DR, val);

You should clear the interrupts before processing.

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ls_pcie_ep_interrupt_init(struct ls_pcie_ep *pcie,
> +				     struct platform_device *pdev)
> +{
> +	u32 val;
> +	int ret;
> +
> +	pcie->irq = platform_get_irq_byname(pdev, "pme");
> +	if (pcie->irq < 0) {
> +		dev_err(&pdev->dev, "Can't get 'pme' IRQ\n");

PME

> +		return pcie->irq;
> +	}
> +
> +	ret = devm_request_irq(&pdev->dev, pcie->irq,
> +			       ls_pcie_ep_event_handler, IRQF_SHARED,
> +			       pdev->name, pcie);

Again, please wrap to fit the 80 column width.

> +	if (ret) {
> +		dev_err(&pdev->dev, "Can't register PCIe IRQ\n");
> +		return ret;
> +	}
> +
> +	/* Enable interrupts */
> +	val = ls_lut_readl(pcie, PEX_PF0_PME_MES_IER);
> +	val |=  PEX_PF0_PME_MES_IER_LDDIE | PEX_PF0_PME_MES_IER_HRDIE |
> +		PEX_PF0_PME_MES_IER_LUDIE;
> +	ls_lut_writel(pcie, PEX_PF0_PME_MES_IER, val);
> +
> +	return 0;
> +}
> +
>  static const struct pci_epc_features*
>  ls_pcie_ep_get_features(struct dw_pcie_ep *ep)
>  {
> @@ -125,6 +219,7 @@ static int __init ls_pcie_ep_probe(struct platform_device *pdev)
>  	struct ls_pcie_ep *pcie;
>  	struct pci_epc_features *ls_epc;
>  	struct resource *dbi_base;
> +	int ret;
>  
>  	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
>  	if (!pcie)
> @@ -144,6 +239,7 @@ static int __init ls_pcie_ep_probe(struct platform_device *pdev)
>  	pci->ops = pcie->drvdata->dw_pcie_ops;
>  
>  	ls_epc->bar_fixed_64bit = (1 << BAR_2) | (1 << BAR_4);
> +	ls_epc->linkup_notifier = true;
>  
>  	pcie->pci = pci;
>  	pcie->ls_epc = ls_epc;
> @@ -155,9 +251,15 @@ static int __init ls_pcie_ep_probe(struct platform_device *pdev)
>  
>  	pci->ep.ops = &ls_pcie_ep_ops;
>  
> +	pcie->big_endian = of_property_read_bool(dev->of_node, "big-endian");
> +
>  	platform_set_drvdata(pdev, pcie);
>  
> -	return dw_pcie_ep_init(&pci->ep);
> +	ret = dw_pcie_ep_init(&pci->ep);
> +	if (ret)
> +		return  ret;

Double space after return.

> +
> +	return  ls_pcie_ep_interrupt_init(pcie, pdev);

Double space after return.

- Mani

>  }
>  
>  static struct platform_driver ls_pcie_ep_driver = {
> -- 
> 2.34.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 1/1] PCI: layerscape: Add the endpoint linkup notifier support

[Manivannan Sadhasivam]

On Mon, May 01, 2023 at 10:48:06AM -0400, Frank Li wrote:
> Layerscape has PME interrupt, which can be used as linkup notifier.
> Set CFG_READY bit when linkup detected.

Where are you setting this bit?

> 
> Signed-off-by: Xiaowei Bao <xiaowei.bao@nxp.com>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> Change from v1 to v2
> - pme -> PME
> - irq -> IRQ
> - update dev_info message according to Bjorn's suggestion
> - remove '.' at error message
> 	
>  .../pci/controller/dwc/pci-layerscape-ep.c    | 104 +++++++++++++++++-
>  1 file changed, 103 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> index c640db60edc6..e974fbe3b6d8 100644
> --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> @@ -18,6 +18,20 @@
>  
>  #include "pcie-designware.h"
>  
> +#define PEX_PF0_CONFIG			0xC0014
> +#define PEX_PF0_CFG_READY		BIT(0)
> +
> +/* PEX PFa PCIE PME and message interrupt registers*/
> +#define PEX_PF0_PME_MES_DR		0xC0020
> +#define PEX_PF0_PME_MES_DR_LUD		BIT(7)
> +#define PEX_PF0_PME_MES_DR_LDD		BIT(9)
> +#define PEX_PF0_PME_MES_DR_HRD		BIT(10)
> +
> +#define PEX_PF0_PME_MES_IER		0xC0028
> +#define PEX_PF0_PME_MES_IER_LUDIE	BIT(7)
> +#define PEX_PF0_PME_MES_IER_LDDIE	BIT(9)
> +#define PEX_PF0_PME_MES_IER_HRDIE	BIT(10)
> +
>  #define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
>  
>  struct ls_pcie_ep_drvdata {
> @@ -30,8 +44,88 @@ struct ls_pcie_ep {
>  	struct dw_pcie			*pci;
>  	struct pci_epc_features		*ls_epc;
>  	const struct ls_pcie_ep_drvdata *drvdata;
> +	bool				big_endian;
> +	int				irq;
>  };
>  
> +static u32 ls_lut_readl(struct ls_pcie_ep *pcie, u32 offset)
> +{
> +	struct dw_pcie *pci = pcie->pci;
> +
> +	if (pcie->big_endian)
> +		return ioread32be(pci->dbi_base + offset);
> +	else
> +		return ioread32(pci->dbi_base + offset);
> +}
> +
> +static void ls_lut_writel(struct ls_pcie_ep *pcie, u32 offset,
> +			  u32 value)

Above function argument could be wrapped within 80 columns.

> +{
> +	struct dw_pcie *pci = pcie->pci;
> +
> +	if (pcie->big_endian)
> +		iowrite32be(value, pci->dbi_base + offset);
> +	else
> +		iowrite32(value, pci->dbi_base + offset);
> +}
> +
> +static irqreturn_t ls_pcie_ep_event_handler(int irq, void *dev_id)
> +{
> +	struct ls_pcie_ep *pcie = (struct ls_pcie_ep *)dev_id;

No need to do explicit typecase for void pointer.

> +	struct dw_pcie *pci = pcie->pci;
> +	u32 val, cfg;
> +
> +	val = ls_lut_readl(pcie, PEX_PF0_PME_MES_DR);
> +	if (!val)
> +		return IRQ_NONE;
> +
> +	if (val & PEX_PF0_PME_MES_DR_LUD) {
> +		cfg = ls_lut_readl(pcie, PEX_PF0_CONFIG);
> +		cfg |= PEX_PF0_CFG_READY;
> +		ls_lut_writel(pcie, PEX_PF0_CONFIG, cfg);
> +		dw_pcie_ep_linkup(&pci->ep);
> +
> +		dev_info(pci->dev, "Link up\n");

These messages could be demoted to dev_dbg() logs.

> +	} else if (val & PEX_PF0_PME_MES_DR_LDD) {
> +		dev_info(pci->dev, "Link down\n");
> +	} else if (val & PEX_PF0_PME_MES_DR_HRD) {
> +		dev_info(pci->dev, "Hot reset\n");
> +	}
> +
> +	ls_lut_writel(pcie, PEX_PF0_PME_MES_DR, val);

You should clear the interrupts before processing.

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ls_pcie_ep_interrupt_init(struct ls_pcie_ep *pcie,
> +				     struct platform_device *pdev)
> +{
> +	u32 val;
> +	int ret;
> +
> +	pcie->irq = platform_get_irq_byname(pdev, "pme");
> +	if (pcie->irq < 0) {
> +		dev_err(&pdev->dev, "Can't get 'pme' IRQ\n");

PME

> +		return pcie->irq;
> +	}
> +
> +	ret = devm_request_irq(&pdev->dev, pcie->irq,
> +			       ls_pcie_ep_event_handler, IRQF_SHARED,
> +			       pdev->name, pcie);

Again, please wrap to fit the 80 column width.

> +	if (ret) {
> +		dev_err(&pdev->dev, "Can't register PCIe IRQ\n");
> +		return ret;
> +	}
> +
> +	/* Enable interrupts */
> +	val = ls_lut_readl(pcie, PEX_PF0_PME_MES_IER);
> +	val |=  PEX_PF0_PME_MES_IER_LDDIE | PEX_PF0_PME_MES_IER_HRDIE |
> +		PEX_PF0_PME_MES_IER_LUDIE;
> +	ls_lut_writel(pcie, PEX_PF0_PME_MES_IER, val);
> +
> +	return 0;
> +}
> +
>  static const struct pci_epc_features*
>  ls_pcie_ep_get_features(struct dw_pcie_ep *ep)
>  {
> @@ -125,6 +219,7 @@ static int __init ls_pcie_ep_probe(struct platform_device *pdev)
>  	struct ls_pcie_ep *pcie;
>  	struct pci_epc_features *ls_epc;
>  	struct resource *dbi_base;
> +	int ret;
>  
>  	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
>  	if (!pcie)
> @@ -144,6 +239,7 @@ static int __init ls_pcie_ep_probe(struct platform_device *pdev)
>  	pci->ops = pcie->drvdata->dw_pcie_ops;
>  
>  	ls_epc->bar_fixed_64bit = (1 << BAR_2) | (1 << BAR_4);
> +	ls_epc->linkup_notifier = true;
>  
>  	pcie->pci = pci;
>  	pcie->ls_epc = ls_epc;
> @@ -155,9 +251,15 @@ static int __init ls_pcie_ep_probe(struct platform_device *pdev)
>  
>  	pci->ep.ops = &ls_pcie_ep_ops;
>  
> +	pcie->big_endian = of_property_read_bool(dev->of_node, "big-endian");
> +
>  	platform_set_drvdata(pdev, pcie);
>  
> -	return dw_pcie_ep_init(&pci->ep);
> +	ret = dw_pcie_ep_init(&pci->ep);
> +	if (ret)
> +		return  ret;

Double space after return.

> +
> +	return  ls_pcie_ep_interrupt_init(pcie, pdev);

Double space after return.

- Mani

>  }
>  
>  static struct platform_driver ls_pcie_ep_driver = {
> -- 
> 2.34.1
> 

-- 
மணிவண்ணன் சதாசிவம்

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add the endpoint linkup notifier support

[Frank Li]

> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Sent: Saturday, May 6, 2023 2:59 AM
> To: Frank Li <frank.li@nxp.com>
> Cc: M.H. Lian <minghuan.lian@nxp.com>; Mingkai Hu
> <mingkai.hu@nxp.com>; Roy Zang <roy.zang@nxp.com>; Lorenzo Pieralisi
> <lpieralisi@kernel.org>; Rob Herring <robh@kernel.org>; Krzysztof
> Wilczyński <kw@linux.com>; Bjorn Helgaas <bhelgaas@google.com>; open
> list:PCI DRIVER FOR FREESCALE LAYERSCAPE <linuxppc-dev@lists.ozlabs.org>;
> open list:PCI DRIVER FOR FREESCALE LAYERSCAPE <linux-
> pci@vger.kernel.org>; moderated list:PCI DRIVER FOR FREESCALE
> LAYERSCAPE <linux-arm-kernel@lists.infradead.org>; open list <linux-
> kernel@vger.kernel.org>; imx@lists.linux.dev
> Subject: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add the endpoint linkup
> notifier support
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
> 
> 
> On Mon, May 01, 2023 at 10:48:06AM -0400, Frank Li wrote:
> > Layerscape has PME interrupt, which can be used as linkup notifier.
> > Set CFG_READY bit when linkup detected.
> 
> Where are you setting this bit?
> 
> >
> > Signed-off-by: Xiaowei Bao <xiaowei.bao@nxp.com>
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > Change from v1 to v2
> > - pme -> PME
> > - irq -> IRQ
> > - update dev_info message according to Bjorn's suggestion
> > - remove '.' at error message
> >
> >  .../pci/controller/dwc/pci-layerscape-ep.c    | 104 +++++++++++++++++-
> >  1 file changed, 103 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > index c640db60edc6..e974fbe3b6d8 100644
> > --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > @@ -18,6 +18,20 @@
> >
> >  #include "pcie-designware.h"
> >
> > +#define PEX_PF0_CONFIG                       0xC0014
> > +#define PEX_PF0_CFG_READY            BIT(0)
> > +
> > +/* PEX PFa PCIE PME and message interrupt registers*/
> > +#define PEX_PF0_PME_MES_DR           0xC0020
> > +#define PEX_PF0_PME_MES_DR_LUD               BIT(7)
> > +#define PEX_PF0_PME_MES_DR_LDD               BIT(9)
> > +#define PEX_PF0_PME_MES_DR_HRD               BIT(10)
> > +
> > +#define PEX_PF0_PME_MES_IER          0xC0028
> > +#define PEX_PF0_PME_MES_IER_LUDIE    BIT(7)
> > +#define PEX_PF0_PME_MES_IER_LDDIE    BIT(9)
> > +#define PEX_PF0_PME_MES_IER_HRDIE    BIT(10)
> > +
> >  #define to_ls_pcie_ep(x)     dev_get_drvdata((x)->dev)
> >
> >  struct ls_pcie_ep_drvdata {
> > @@ -30,8 +44,88 @@ struct ls_pcie_ep {
> >       struct dw_pcie                  *pci;
> >       struct pci_epc_features         *ls_epc;
> >       const struct ls_pcie_ep_drvdata *drvdata;
> > +     bool                            big_endian;
> > +     int                             irq;
> >  };
> >
> > +static u32 ls_lut_readl(struct ls_pcie_ep *pcie, u32 offset)
> > +{
> > +     struct dw_pcie *pci = pcie->pci;
> > +
> > +     if (pcie->big_endian)
> > +             return ioread32be(pci->dbi_base + offset);
> > +     else
> > +             return ioread32(pci->dbi_base + offset);
> > +}
> > +
> > +static void ls_lut_writel(struct ls_pcie_ep *pcie, u32 offset,
> > +                       u32 value)
> 
> Above function argument could be wrapped within 80 columns.
> 
> > +{
> > +     struct dw_pcie *pci = pcie->pci;
> > +
> > +     if (pcie->big_endian)
> > +             iowrite32be(value, pci->dbi_base + offset);
> > +     else
> > +             iowrite32(value, pci->dbi_base + offset);
> > +}
> > +
> > +static irqreturn_t ls_pcie_ep_event_handler(int irq, void *dev_id)
> > +{
> > +     struct ls_pcie_ep *pcie = (struct ls_pcie_ep *)dev_id;
> 
> No need to do explicit typecase for void pointer.
> 
> > +     struct dw_pcie *pci = pcie->pci;
> > +     u32 val, cfg;
> > +
> > +     val = ls_lut_readl(pcie, PEX_PF0_PME_MES_DR);
> > +     if (!val)
> > +             return IRQ_NONE;
> > +
> > +     if (val & PEX_PF0_PME_MES_DR_LUD) {
> > +             cfg = ls_lut_readl(pcie, PEX_PF0_CONFIG);
> > +             cfg |= PEX_PF0_CFG_READY;
> > +             ls_lut_writel(pcie, PEX_PF0_CONFIG, cfg);
> > +             dw_pcie_ep_linkup(&pci->ep);
> > +
> > +             dev_info(pci->dev, "Link up\n");
> 
> These messages could be demoted to dev_dbg() logs.
> 
> > +     } else if (val & PEX_PF0_PME_MES_DR_LDD) {
> > +             dev_info(pci->dev, "Link down\n");
> > +     } else if (val & PEX_PF0_PME_MES_DR_HRD) {
> > +             dev_info(pci->dev, "Hot reset\n");
> > +     }
> > +
> > +     ls_lut_writel(pcie, PEX_PF0_PME_MES_DR, val);
> 
> You should clear the interrupts before processing.
> 
> > +
> > +     return IRQ_HANDLED;
> > +}
> > +
> > +static int ls_pcie_ep_interrupt_init(struct ls_pcie_ep *pcie,
> > +                                  struct platform_device *pdev)
> > +{
> > +     u32 val;
> > +     int ret;
> > +
> > +     pcie->irq = platform_get_irq_byname(pdev, "pme");
> > +     if (pcie->irq < 0) {
> > +             dev_err(&pdev->dev, "Can't get 'pme' IRQ\n");
> 
> PME

Here should be dts property `pme`, suppose should match
platform_get_irq_byname(pdev, "pme");

> 
> > +             return pcie->irq;
> > +     }
> > +
> > +     ret = devm_request_irq(&pdev->dev, pcie->irq,
> > +                            ls_pcie_ep_event_handler, IRQF_SHARED,
> > +                            pdev->name, pcie);
> 
> Again, please wrap to fit the 80 column width.
> 
> > +     if (ret) {
> > +             dev_err(&pdev->dev, "Can't register PCIe IRQ\n");
> > +             return ret;
> > +     }
> > +
> > +     /* Enable interrupts */
> > +     val = ls_lut_readl(pcie, PEX_PF0_PME_MES_IER);
> > +     val |=  PEX_PF0_PME_MES_IER_LDDIE |
> PEX_PF0_PME_MES_IER_HRDIE |
> > +             PEX_PF0_PME_MES_IER_LUDIE;
> > +     ls_lut_writel(pcie, PEX_PF0_PME_MES_IER, val);
> > +
> > +     return 0;
> > +}
> > +
> >  static const struct pci_epc_features*
> >  ls_pcie_ep_get_features(struct dw_pcie_ep *ep)
> >  {
> > @@ -125,6 +219,7 @@ static int __init ls_pcie_ep_probe(struct
> platform_device *pdev)
> >       struct ls_pcie_ep *pcie;
> >       struct pci_epc_features *ls_epc;
> >       struct resource *dbi_base;
> > +     int ret;
> >
> >       pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
> >       if (!pcie)
> > @@ -144,6 +239,7 @@ static int __init ls_pcie_ep_probe(struct
> platform_device *pdev)
> >       pci->ops = pcie->drvdata->dw_pcie_ops;
> >
> >       ls_epc->bar_fixed_64bit = (1 << BAR_2) | (1 << BAR_4);
> > +     ls_epc->linkup_notifier = true;
> >
> >       pcie->pci = pci;
> >       pcie->ls_epc = ls_epc;
> > @@ -155,9 +251,15 @@ static int __init ls_pcie_ep_probe(struct
> platform_device *pdev)
> >
> >       pci->ep.ops = &ls_pcie_ep_ops;
> >
> > +     pcie->big_endian = of_property_read_bool(dev->of_node, "big-
> endian");
> > +
> >       platform_set_drvdata(pdev, pcie);
> >
> > -     return dw_pcie_ep_init(&pci->ep);
> > +     ret = dw_pcie_ep_init(&pci->ep);
> > +     if (ret)
> > +             return  ret;
> 
> Double space after return.
> 
> > +
> > +     return  ls_pcie_ep_interrupt_init(pcie, pdev);
> 
> Double space after return.
> 
> - Mani
> 
> >  }
> >
> >  static struct platform_driver ls_pcie_ep_driver = {
> > --
> > 2.34.1
> >
> 
> --
> மணிவண்ணன் சதாசிவம்

RE: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add the endpoint linkup notifier support

[Frank Li]

> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Sent: Saturday, May 6, 2023 2:59 AM
> To: Frank Li <frank.li@nxp.com>
> Cc: M.H. Lian <minghuan.lian@nxp.com>; Mingkai Hu
> <mingkai.hu@nxp.com>; Roy Zang <roy.zang@nxp.com>; Lorenzo Pieralisi
> <lpieralisi@kernel.org>; Rob Herring <robh@kernel.org>; Krzysztof
> Wilczyński <kw@linux.com>; Bjorn Helgaas <bhelgaas@google.com>; open
> list:PCI DRIVER FOR FREESCALE LAYERSCAPE <linuxppc-dev@lists.ozlabs.org>;
> open list:PCI DRIVER FOR FREESCALE LAYERSCAPE <linux-
> pci@vger.kernel.org>; moderated list:PCI DRIVER FOR FREESCALE
> LAYERSCAPE <linux-arm-kernel@lists.infradead.org>; open list <linux-
> kernel@vger.kernel.org>; imx@lists.linux.dev
> Subject: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add the endpoint linkup
> notifier support
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
> 
> 
> On Mon, May 01, 2023 at 10:48:06AM -0400, Frank Li wrote:
> > Layerscape has PME interrupt, which can be used as linkup notifier.
> > Set CFG_READY bit when linkup detected.
> 
> Where are you setting this bit?
> 
> >
> > Signed-off-by: Xiaowei Bao <xiaowei.bao@nxp.com>
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > Change from v1 to v2
> > - pme -> PME
> > - irq -> IRQ
> > - update dev_info message according to Bjorn's suggestion
> > - remove '.' at error message
> >
> >  .../pci/controller/dwc/pci-layerscape-ep.c    | 104 +++++++++++++++++-
> >  1 file changed, 103 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > index c640db60edc6..e974fbe3b6d8 100644
> > --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > @@ -18,6 +18,20 @@
> >
> >  #include "pcie-designware.h"
> >
> > +#define PEX_PF0_CONFIG                       0xC0014
> > +#define PEX_PF0_CFG_READY            BIT(0)
> > +
> > +/* PEX PFa PCIE PME and message interrupt registers*/
> > +#define PEX_PF0_PME_MES_DR           0xC0020
> > +#define PEX_PF0_PME_MES_DR_LUD               BIT(7)
> > +#define PEX_PF0_PME_MES_DR_LDD               BIT(9)
> > +#define PEX_PF0_PME_MES_DR_HRD               BIT(10)
> > +
> > +#define PEX_PF0_PME_MES_IER          0xC0028
> > +#define PEX_PF0_PME_MES_IER_LUDIE    BIT(7)
> > +#define PEX_PF0_PME_MES_IER_LDDIE    BIT(9)
> > +#define PEX_PF0_PME_MES_IER_HRDIE    BIT(10)
> > +
> >  #define to_ls_pcie_ep(x)     dev_get_drvdata((x)->dev)
> >
> >  struct ls_pcie_ep_drvdata {
> > @@ -30,8 +44,88 @@ struct ls_pcie_ep {
> >       struct dw_pcie                  *pci;
> >       struct pci_epc_features         *ls_epc;
> >       const struct ls_pcie_ep_drvdata *drvdata;
> > +     bool                            big_endian;
> > +     int                             irq;
> >  };
> >
> > +static u32 ls_lut_readl(struct ls_pcie_ep *pcie, u32 offset)
> > +{
> > +     struct dw_pcie *pci = pcie->pci;
> > +
> > +     if (pcie->big_endian)
> > +             return ioread32be(pci->dbi_base + offset);
> > +     else
> > +             return ioread32(pci->dbi_base + offset);
> > +}
> > +
> > +static void ls_lut_writel(struct ls_pcie_ep *pcie, u32 offset,
> > +                       u32 value)
> 
> Above function argument could be wrapped within 80 columns.
> 
> > +{
> > +     struct dw_pcie *pci = pcie->pci;
> > +
> > +     if (pcie->big_endian)
> > +             iowrite32be(value, pci->dbi_base + offset);
> > +     else
> > +             iowrite32(value, pci->dbi_base + offset);
> > +}
> > +
> > +static irqreturn_t ls_pcie_ep_event_handler(int irq, void *dev_id)
> > +{
> > +     struct ls_pcie_ep *pcie = (struct ls_pcie_ep *)dev_id;
> 
> No need to do explicit typecase for void pointer.
> 
> > +     struct dw_pcie *pci = pcie->pci;
> > +     u32 val, cfg;
> > +
> > +     val = ls_lut_readl(pcie, PEX_PF0_PME_MES_DR);
> > +     if (!val)
> > +             return IRQ_NONE;
> > +
> > +     if (val & PEX_PF0_PME_MES_DR_LUD) {
> > +             cfg = ls_lut_readl(pcie, PEX_PF0_CONFIG);
> > +             cfg |= PEX_PF0_CFG_READY;
> > +             ls_lut_writel(pcie, PEX_PF0_CONFIG, cfg);
> > +             dw_pcie_ep_linkup(&pci->ep);
> > +
> > +             dev_info(pci->dev, "Link up\n");
> 
> These messages could be demoted to dev_dbg() logs.
> 
> > +     } else if (val & PEX_PF0_PME_MES_DR_LDD) {
> > +             dev_info(pci->dev, "Link down\n");
> > +     } else if (val & PEX_PF0_PME_MES_DR_HRD) {
> > +             dev_info(pci->dev, "Hot reset\n");
> > +     }
> > +
> > +     ls_lut_writel(pcie, PEX_PF0_PME_MES_DR, val);
> 
> You should clear the interrupts before processing.
> 
> > +
> > +     return IRQ_HANDLED;
> > +}
> > +
> > +static int ls_pcie_ep_interrupt_init(struct ls_pcie_ep *pcie,
> > +                                  struct platform_device *pdev)
> > +{
> > +     u32 val;
> > +     int ret;
> > +
> > +     pcie->irq = platform_get_irq_byname(pdev, "pme");
> > +     if (pcie->irq < 0) {
> > +             dev_err(&pdev->dev, "Can't get 'pme' IRQ\n");
> 
> PME

Here should be dts property `pme`, suppose should match
platform_get_irq_byname(pdev, "pme");

> 
> > +             return pcie->irq;
> > +     }
> > +
> > +     ret = devm_request_irq(&pdev->dev, pcie->irq,
> > +                            ls_pcie_ep_event_handler, IRQF_SHARED,
> > +                            pdev->name, pcie);
> 
> Again, please wrap to fit the 80 column width.
> 
> > +     if (ret) {
> > +             dev_err(&pdev->dev, "Can't register PCIe IRQ\n");
> > +             return ret;
> > +     }
> > +
> > +     /* Enable interrupts */
> > +     val = ls_lut_readl(pcie, PEX_PF0_PME_MES_IER);
> > +     val |=  PEX_PF0_PME_MES_IER_LDDIE |
> PEX_PF0_PME_MES_IER_HRDIE |
> > +             PEX_PF0_PME_MES_IER_LUDIE;
> > +     ls_lut_writel(pcie, PEX_PF0_PME_MES_IER, val);
> > +
> > +     return 0;
> > +}
> > +
> >  static const struct pci_epc_features*
> >  ls_pcie_ep_get_features(struct dw_pcie_ep *ep)
> >  {
> > @@ -125,6 +219,7 @@ static int __init ls_pcie_ep_probe(struct
> platform_device *pdev)
> >       struct ls_pcie_ep *pcie;
> >       struct pci_epc_features *ls_epc;
> >       struct resource *dbi_base;
> > +     int ret;
> >
> >       pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
> >       if (!pcie)
> > @@ -144,6 +239,7 @@ static int __init ls_pcie_ep_probe(struct
> platform_device *pdev)
> >       pci->ops = pcie->drvdata->dw_pcie_ops;
> >
> >       ls_epc->bar_fixed_64bit = (1 << BAR_2) | (1 << BAR_4);
> > +     ls_epc->linkup_notifier = true;
> >
> >       pcie->pci = pci;
> >       pcie->ls_epc = ls_epc;
> > @@ -155,9 +251,15 @@ static int __init ls_pcie_ep_probe(struct
> platform_device *pdev)
> >
> >       pci->ep.ops = &ls_pcie_ep_ops;
> >
> > +     pcie->big_endian = of_property_read_bool(dev->of_node, "big-
> endian");
> > +
> >       platform_set_drvdata(pdev, pcie);
> >
> > -     return dw_pcie_ep_init(&pci->ep);
> > +     ret = dw_pcie_ep_init(&pci->ep);
> > +     if (ret)
> > +             return  ret;
> 
> Double space after return.
> 
> > +
> > +     return  ls_pcie_ep_interrupt_init(pcie, pdev);
> 
> Double space after return.
> 
> - Mani
> 
> >  }
> >
> >  static struct platform_driver ls_pcie_ep_driver = {
> > --
> > 2.34.1
> >
> 
> --
> மணிவண்ணன் சதாசிவம்

Re: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add the endpoint linkup notifier support

[Bjorn Helgaas]

On Mon, May 08, 2023 at 01:31:26PM +0000, Frank Li wrote:
> > -----Original Message-----
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Sent: Saturday, May 6, 2023 2:59 AM
> > To: Frank Li <frank.li@nxp.com>
> > Cc: M.H. Lian <minghuan.lian@nxp.com>; Mingkai Hu
> > <mingkai.hu@nxp.com>; Roy Zang <roy.zang@nxp.com>; Lorenzo Pieralisi
> > <lpieralisi@kernel.org>; Rob Herring <robh@kernel.org>; Krzysztof
> > Wilczyński <kw@linux.com>; Bjorn Helgaas <bhelgaas@google.com>; open
> > list:PCI DRIVER FOR FREESCALE LAYERSCAPE <linuxppc-dev@lists.ozlabs.org>;
> > open list:PCI DRIVER FOR FREESCALE LAYERSCAPE <linux-
> > pci@vger.kernel.org>; moderated list:PCI DRIVER FOR FREESCALE
> > LAYERSCAPE <linux-arm-kernel@lists.infradead.org>; open list <linux-
> > kernel@vger.kernel.org>; imx@lists.linux.dev
> > Subject: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add the endpoint linkup
> > notifier support

All these quoted headers are redundant clutter since we've already
seen them when Manivannan sent his comments.  It would be nice if your
mailer could be configured to omit them.

> > > +static int ls_pcie_ep_interrupt_init(struct ls_pcie_ep *pcie,
> > > +                                  struct platform_device *pdev)
> > > +{
> > > +     u32 val;
> > > +     int ret;
> > > +
> > > +     pcie->irq = platform_get_irq_byname(pdev, "pme");
> > > +     if (pcie->irq < 0) {
> > > +             dev_err(&pdev->dev, "Can't get 'pme' IRQ\n");
> > 
> > PME
> 
> Here should be dts property `pme`, suppose should match
> platform_get_irq_byname(pdev, "pme");

You can also edit out all the other context and questions if you're
not responding to them.

There were a lot of other comments that were useful but are not
relevant to this reply.

Bjorn

Re: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add the endpoint linkup notifier support

[Bjorn Helgaas]

On Mon, May 08, 2023 at 01:31:26PM +0000, Frank Li wrote:
> > -----Original Message-----
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Sent: Saturday, May 6, 2023 2:59 AM
> > To: Frank Li <frank.li@nxp.com>
> > Cc: M.H. Lian <minghuan.lian@nxp.com>; Mingkai Hu
> > <mingkai.hu@nxp.com>; Roy Zang <roy.zang@nxp.com>; Lorenzo Pieralisi
> > <lpieralisi@kernel.org>; Rob Herring <robh@kernel.org>; Krzysztof
> > Wilczyński <kw@linux.com>; Bjorn Helgaas <bhelgaas@google.com>; open
> > list:PCI DRIVER FOR FREESCALE LAYERSCAPE <linuxppc-dev@lists.ozlabs.org>;
> > open list:PCI DRIVER FOR FREESCALE LAYERSCAPE <linux-
> > pci@vger.kernel.org>; moderated list:PCI DRIVER FOR FREESCALE
> > LAYERSCAPE <linux-arm-kernel@lists.infradead.org>; open list <linux-
> > kernel@vger.kernel.org>; imx@lists.linux.dev
> > Subject: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add the endpoint linkup
> > notifier support

All these quoted headers are redundant clutter since we've already
seen them when Manivannan sent his comments.  It would be nice if your
mailer could be configured to omit them.

> > > +static int ls_pcie_ep_interrupt_init(struct ls_pcie_ep *pcie,
> > > +                                  struct platform_device *pdev)
> > > +{
> > > +     u32 val;
> > > +     int ret;
> > > +
> > > +     pcie->irq = platform_get_irq_byname(pdev, "pme");
> > > +     if (pcie->irq < 0) {
> > > +             dev_err(&pdev->dev, "Can't get 'pme' IRQ\n");
> > 
> > PME
> 
> Here should be dts property `pme`, suppose should match
> platform_get_irq_byname(pdev, "pme");

You can also edit out all the other context and questions if you're
not responding to them.

There were a lot of other comments that were useful but are not
relevant to this reply.

Bjorn

RE: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add the endpoint linkup notifier support

[Frank Li]

> > > Subject: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add the endpoint
> linkup
> > > notifier support
> 
> All these quoted headers are redundant clutter since we've already
> seen them when Manivannan sent his comments.  It would be nice if your
> mailer could be configured to omit them.

Our email client quite stupid. 

> 
> > > > +static int ls_pcie_ep_interrupt_init(struct ls_pcie_ep *pcie,
> > > > +                                  struct platform_device *pdev)
> > > > +{
> > > > +     u32 val;
> > > > +     int ret;
> > > > +
> > > > +     pcie->irq = platform_get_irq_byname(pdev, "pme");
> > > > +     if (pcie->irq < 0) {
> > > > +             dev_err(&pdev->dev, "Can't get 'pme' IRQ\n");
> > >
> > > PME
> >
> > Here should be dts property `pme`, suppose should match
> > platform_get_irq_byname(pdev, "pme");
> 
> You can also edit out all the other context and questions if you're
> not responding to them.
> 
> There were a lot of other comments that were useful but are not
> relevant to this reply.

Okay, I found I mess up patch version number.
 
> 
> Bjorn

RE: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add the endpoint linkup notifier support

[Frank Li]

> > > Subject: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add the endpoint
> linkup
> > > notifier support
> 
> All these quoted headers are redundant clutter since we've already
> seen them when Manivannan sent his comments.  It would be nice if your
> mailer could be configured to omit them.

Our email client quite stupid. 

> 
> > > > +static int ls_pcie_ep_interrupt_init(struct ls_pcie_ep *pcie,
> > > > +                                  struct platform_device *pdev)
> > > > +{
> > > > +     u32 val;
> > > > +     int ret;
> > > > +
> > > > +     pcie->irq = platform_get_irq_byname(pdev, "pme");
> > > > +     if (pcie->irq < 0) {
> > > > +             dev_err(&pdev->dev, "Can't get 'pme' IRQ\n");
> > >
> > > PME
> >
> > Here should be dts property `pme`, suppose should match
> > platform_get_irq_byname(pdev, "pme");
> 
> You can also edit out all the other context and questions if you're
> not responding to them.
> 
> There were a lot of other comments that were useful but are not
> relevant to this reply.

Okay, I found I mess up patch version number.
 
> 
> Bjorn

Re: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add the endpoint linkup notifier support

[Bjorn Helgaas]

On Mon, May 08, 2023 at 09:45:59PM +0000, Frank Li wrote:
> > > > Subject: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add the endpoint
> > linkup
> > > > notifier support
> > 
> > All these quoted headers are redundant clutter since we've already
> > seen them when Manivannan sent his comments.  It would be nice if your
> > mailer could be configured to omit them.
> 
> Our email client quite stupid. 

Yeah, sometimes those are really hard to work around.

Bjorn

Re: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add the endpoint linkup notifier support

[Bjorn Helgaas]

On Mon, May 08, 2023 at 09:45:59PM +0000, Frank Li wrote:
> > > > Subject: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add the endpoint
> > linkup
> > > > notifier support
> > 
> > All these quoted headers are redundant clutter since we've already
> > seen them when Manivannan sent his comments.  It would be nice if your
> > mailer could be configured to omit them.
> 
> Our email client quite stupid. 

Yeah, sometimes those are really hard to work around.

Bjorn

[PATCH v2 1/1] PCI: layerscape: Add the endpoint linkup notifier support

[Frank Li]

Layerscape has PME interrupt, which can be used as linkup notifier.
Set CFG_READY bit of PEX_PF0_CONFIG to enable accesses from root complex
when linkup detected.

Signed-off-by: Xiaowei Bao <xiaowei.bao@nxp.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 .../pci/controller/dwc/pci-layerscape-ep.c    | 102 +++++++++++++++++-
 1 file changed, 101 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c b/drivers/pci/controller/dwc/pci-layerscape-ep.c
index c640db60edc6..1a431a9be91e 100644
--- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
+++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
@@ -18,6 +18,20 @@
 
 #include "pcie-designware.h"
 
+#define PEX_PF0_CONFIG			0xC0014
+#define PEX_PF0_CFG_READY		BIT(0)
+
+/* PEX PFa PCIE PME and message interrupt registers*/
+#define PEX_PF0_PME_MES_DR		0xC0020
+#define PEX_PF0_PME_MES_DR_LUD		BIT(7)
+#define PEX_PF0_PME_MES_DR_LDD		BIT(9)
+#define PEX_PF0_PME_MES_DR_HRD		BIT(10)
+
+#define PEX_PF0_PME_MES_IER		0xC0028
+#define PEX_PF0_PME_MES_IER_LUDIE	BIT(7)
+#define PEX_PF0_PME_MES_IER_LDDIE	BIT(9)
+#define PEX_PF0_PME_MES_IER_HRDIE	BIT(10)
+
 #define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
 
 struct ls_pcie_ep_drvdata {
@@ -30,8 +44,86 @@ struct ls_pcie_ep {
 	struct dw_pcie			*pci;
 	struct pci_epc_features		*ls_epc;
 	const struct ls_pcie_ep_drvdata *drvdata;
+	bool				big_endian;
+	int				irq;
 };
 
+static u32 ls_lut_readl(struct ls_pcie_ep *pcie, u32 offset)
+{
+	struct dw_pcie *pci = pcie->pci;
+
+	if (pcie->big_endian)
+		return ioread32be(pci->dbi_base + offset);
+	else
+		return ioread32(pci->dbi_base + offset);
+}
+
+static void ls_lut_writel(struct ls_pcie_ep *pcie, u32 offset, u32 value)
+{
+	struct dw_pcie *pci = pcie->pci;
+
+	if (pcie->big_endian)
+		iowrite32be(value, pci->dbi_base + offset);
+	else
+		iowrite32(value, pci->dbi_base + offset);
+}
+
+static irqreturn_t ls_pcie_ep_event_handler(int irq, void *dev_id)
+{
+	struct ls_pcie_ep *pcie = dev_id;
+	struct dw_pcie *pci = pcie->pci;
+	u32 val, cfg;
+
+	val = ls_lut_readl(pcie, PEX_PF0_PME_MES_DR);
+	ls_lut_writel(pcie, PEX_PF0_PME_MES_DR, val);
+
+	if (!val)
+		return IRQ_NONE;
+
+	if (val & PEX_PF0_PME_MES_DR_LUD) {
+		cfg = ls_lut_readl(pcie, PEX_PF0_CONFIG);
+		cfg |= PEX_PF0_CFG_READY;
+		ls_lut_writel(pcie, PEX_PF0_CONFIG, cfg);
+		dw_pcie_ep_linkup(&pci->ep);
+
+		dev_dbg(pci->dev, "Link up\n");
+	} else if (val & PEX_PF0_PME_MES_DR_LDD) {
+		dev_dbg(pci->dev, "Link down\n");
+	} else if (val & PEX_PF0_PME_MES_DR_HRD) {
+		dev_dbg(pci->dev, "Hot reset\n");
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int ls_pcie_ep_interrupt_init(struct ls_pcie_ep *pcie,
+				     struct platform_device *pdev)
+{
+	u32 val;
+	int ret;
+
+	pcie->irq = platform_get_irq_byname(pdev, "pme");
+	if (pcie->irq < 0) {
+		dev_err(&pdev->dev, "Can't get 'pme' IRQ\n");
+		return pcie->irq;
+	}
+
+	ret = devm_request_irq(&pdev->dev, pcie->irq, ls_pcie_ep_event_handler,
+			       IRQF_SHARED, pdev->name, pcie);
+	if (ret) {
+		dev_err(&pdev->dev, "Can't register PCIe IRQ\n");
+		return ret;
+	}
+
+	/* Enable interrupts */
+	val = ls_lut_readl(pcie, PEX_PF0_PME_MES_IER);
+	val |=  PEX_PF0_PME_MES_IER_LDDIE | PEX_PF0_PME_MES_IER_HRDIE |
+		PEX_PF0_PME_MES_IER_LUDIE;
+	ls_lut_writel(pcie, PEX_PF0_PME_MES_IER, val);
+
+	return 0;
+}
+
 static const struct pci_epc_features*
 ls_pcie_ep_get_features(struct dw_pcie_ep *ep)
 {
@@ -125,6 +217,7 @@ static int __init ls_pcie_ep_probe(struct platform_device *pdev)
 	struct ls_pcie_ep *pcie;
 	struct pci_epc_features *ls_epc;
 	struct resource *dbi_base;
+	int ret;
 
 	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
 	if (!pcie)
@@ -144,6 +237,7 @@ static int __init ls_pcie_ep_probe(struct platform_device *pdev)
 	pci->ops = pcie->drvdata->dw_pcie_ops;
 
 	ls_epc->bar_fixed_64bit = (1 << BAR_2) | (1 << BAR_4);
+	ls_epc->linkup_notifier = true;
 
 	pcie->pci = pci;
 	pcie->ls_epc = ls_epc;
@@ -155,9 +249,15 @@ static int __init ls_pcie_ep_probe(struct platform_device *pdev)
 
 	pci->ep.ops = &ls_pcie_ep_ops;
 
+	pcie->big_endian = of_property_read_bool(dev->of_node, "big-endian");
+
 	platform_set_drvdata(pdev, pcie);
 
-	return dw_pcie_ep_init(&pci->ep);
+	ret = dw_pcie_ep_init(&pci->ep);
+	if (ret)
+		return ret;
+
+	return ls_pcie_ep_interrupt_init(pcie, pdev);
 }
 
 static struct platform_driver ls_pcie_ep_driver = {
-- 
2.34.1