diff mbox

[v3,14/17] PCI: dwc: artpec6: Add support for endpoint mode

Message ID 20171031223936.27549-15-niklas.cassel@axis.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Niklas Cassel Oct. 31, 2017, 10:39 p.m. UTC
Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
---
V3:
* Removed ifdefs around match table and match table data.
* Removed ifdefs in probe, use dummy implementations instead.

 drivers/pci/dwc/Kconfig        |  23 ++++--
 drivers/pci/dwc/pcie-artpec6.c | 162 +++++++++++++++++++++++++++++++++++++++--
 2 files changed, 174 insertions(+), 11 deletions(-)

Comments

Arnd Bergmann Nov. 2, 2017, 9:13 a.m. UTC | #1
On Tue, Oct 31, 2017 at 11:39 PM, Niklas Cassel <niklas.cassel@axis.com> wrote:
> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>

It seems like you are missing a changelog text. Please explain what
your work is good for
in any patch you send.

> V3:
> * Removed ifdefs around match table and match table data.
> * Removed ifdefs in probe, use dummy implementations instead.

I think there is room for more of the same ;-)

>
> +#ifdef CONFIG_PCIE_ARTPEC6_HOST
>  static void artpec6_pcie_enable_interrupts(struct artpec6_pcie *artpec6_pcie)
>  {
>         struct dw_pcie *pci = artpec6_pcie->pci;
> @@ -231,11 +257,92 @@ static int artpec6_add_pcie_port(struct artpec6_pcie *artpec6_pcie,
>
>         return 0;
>  }
> +#else
> +static inline int artpec6_add_pcie_port(struct artpec6_pcie *artpec6_pcie,
> +                                       struct platform_device *pdev)
> +{
> +       return -ENODEV;
> +}
> +#endif


Can you try replacing the #ifdef with


        if (!IS_ENABLED(CONFIG_PCIE_ARTPEC6_HOST))
                 return -ENODEV;

at the start of artpec6_pcie_enable_interrupts? I think that would improve
readability here.

> +static int artpec6_add_pcie_ep(struct artpec6_pcie *artpec6_pcie,
> +                              struct platform_device *pdev)
> +{
> +       int ret;
> +       struct dw_pcie_ep *ep;
> +       struct resource *res;
> +       struct device *dev = &pdev->dev;
> +       struct dw_pcie *pci = artpec6_pcie->pci;

The same trick should work here with the other symbol.

        Arnd
Niklas Cassel Nov. 3, 2017, 9:56 a.m. UTC | #2
On 11/02/2017 10:13 AM, Arnd Bergmann wrote:
> On Tue, Oct 31, 2017 at 11:39 PM, Niklas Cassel <niklas.cassel@axis.com> wrote:
>> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
> 
> It seems like you are missing a changelog text. Please explain what
> your work is good for
> in any patch you send.

You are correct, this patch is missing a changelog text.
I will send a V4 of the patch series for this.

> 
>> V3:
>> * Removed ifdefs around match table and match table data.
>> * Removed ifdefs in probe, use dummy implementations instead.
> 
> I think there is room for more of the same ;-)
> 
>>
>> +#ifdef CONFIG_PCIE_ARTPEC6_HOST
>>  static void artpec6_pcie_enable_interrupts(struct artpec6_pcie *artpec6_pcie)
>>  {
>>         struct dw_pcie *pci = artpec6_pcie->pci;
>> @@ -231,11 +257,92 @@ static int artpec6_add_pcie_port(struct artpec6_pcie *artpec6_pcie,
>>
>>         return 0;
>>  }
>> +#else
>> +static inline int artpec6_add_pcie_port(struct artpec6_pcie *artpec6_pcie,
>> +                                       struct platform_device *pdev)
>> +{
>> +       return -ENODEV;
>> +}
>> +#endif
> 
> 
> Can you try replacing the #ifdef with
> 
> 
>         if (!IS_ENABLED(CONFIG_PCIE_ARTPEC6_HOST))
>                  return -ENODEV;
> 
> at the start of artpec6_pcie_enable_interrupts? I think that would improve
> readability here.
> 

artpec6_pcie_enable_interrupts is a void function, so
I guess that you meant at the start of artpec6_add_pcie_port.
That would not really help since artpec6_add_pcie_port
calls artpec6_pcie_msi_handler, and uses artpec6_pcie_host_ops,
which is still inside the CONFIG_PCIE_ARTPEC6_HOST ifdef block.

Please note that there are several functions, as well as
artpec6_pcie_host_ops inside the 
CONFIG_PCIE_ARTPEC6_HOST ifdef block.

The reason for this is because Bjorn was surprised that
this driver at V1 didn't have any ifdefs, even though
it supports two different modes: HOST and EP.
I suspected that his reasoning was that if you compile
the driver with only one of the modes, it is wasteful
to compile and include the functions that belong to the
mode that we are not using in the vmlinux.

>> +static int artpec6_add_pcie_ep(struct artpec6_pcie *artpec6_pcie,
>> +                              struct platform_device *pdev)
>> +{
>> +       int ret;
>> +       struct dw_pcie_ep *ep;
>> +       struct resource *res;
>> +       struct device *dev = &pdev->dev;
>> +       struct dw_pcie *pci = artpec6_pcie->pci;
> 
> The same trick should work here with the other symbol.

While artpec6_add_pcie_ep doesn't call any other
function in this file, it does use pcie_ep_ops,
which does reference other functions in this file
(which are inside the ifdef block).


Regards,
Niklas
Arnd Bergmann Nov. 3, 2017, 10:23 a.m. UTC | #3
On Fri, Nov 3, 2017 at 10:56 AM, Niklas Cassel <niklas.cassel@axis.com> wrote:
> On 11/02/2017 10:13 AM, Arnd Bergmann wrote:

>>
>>
>> Can you try replacing the #ifdef with
>>
>>
>>         if (!IS_ENABLED(CONFIG_PCIE_ARTPEC6_HOST))
>>                  return -ENODEV;
>>
>> at the start of artpec6_pcie_enable_interrupts? I think that would improve
>> readability here.
>>
>
> artpec6_pcie_enable_interrupts is a void function, so
> I guess that you meant at the start of artpec6_add_pcie_port.

Right, sorry about that.

> That would not really help since artpec6_add_pcie_port
> calls artpec6_pcie_msi_handler, and uses artpec6_pcie_host_ops,
> which is still inside the CONFIG_PCIE_ARTPEC6_HOST ifdef block.
>
> Please note that there are several functions, as well as
> artpec6_pcie_host_ops inside the
> CONFIG_PCIE_ARTPEC6_HOST ifdef block.

What I meant is that you can remove the #ifdef entirely if you add

         if (!IS_ENABLED(CONFIG_PCIE_ARTPEC6_HOST))
                  return -ENODEV;

to artpec6_pcie_probe(). Anything after that statement will get
silently dropped by the compiler, including static functions and
structures that are referenced indirectly from there.

      Arnd
Niklas Cassel Nov. 3, 2017, 2:16 p.m. UTC | #4
On 11/03/2017 11:23 AM, Arnd Bergmann wrote:
> On Fri, Nov 3, 2017 at 10:56 AM, Niklas Cassel <niklas.cassel@axis.com> wrote:
>> On 11/02/2017 10:13 AM, Arnd Bergmann wrote:

> What I meant is that you can remove the #ifdef entirely if you add
> 
>          if (!IS_ENABLED(CONFIG_PCIE_ARTPEC6_HOST))
>                   return -ENODEV;
> 
> to artpec6_pcie_probe(). Anything after that statement will get
> silently dropped by the compiler, including static functions and
> structures that are referenced indirectly from there.

Wow, this actually helps gcc with dead code elimination.
It was even possible to add those !IS_ENABLED the two different
case labels in the switch statement.
I get no unwanted symbols when looking at the vmlinux in gdb.

Great suggestion Arnd :)


Regards,
Niklas
diff mbox

Patch

diff --git a/drivers/pci/dwc/Kconfig b/drivers/pci/dwc/Kconfig
index 3954353e3e2e..0fb96c7754de 100644
--- a/drivers/pci/dwc/Kconfig
+++ b/drivers/pci/dwc/Kconfig
@@ -148,15 +148,28 @@  config PCIE_ARMADA_8K
 	  DesignWare core functions to implement the driver.
 
 config PCIE_ARTPEC6
-	bool "Axis ARTPEC-6 PCIe controller"
-	depends on PCI
+	bool
+
+config PCIE_ARTPEC6_HOST
+	bool "Axis ARTPEC-6 PCIe controller Host Mode"
 	depends on MACH_ARTPEC6
-	depends on PCI_MSI_IRQ_DOMAIN
+	depends on PCI && PCI_MSI_IRQ_DOMAIN
 	select PCIEPORTBUS
 	select PCIE_DW_HOST
+	select PCIE_ARTPEC6
+	help
+	  Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
+	  host mode. This uses the DesignWare core.
+
+config PCIE_ARTPEC6_EP
+	bool "Axis ARTPEC-6 PCIe controller Endpoint Mode"
+	depends on MACH_ARTPEC6
+	depends on PCI_ENDPOINT
+	select PCIE_DW_EP
+	select PCIE_ARTPEC6
 	help
-	  Say Y here to enable PCIe controller support on Axis ARTPEC-6
-	  SoCs.  This PCIe controller uses the DesignWare core.
+	  Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
+	  endpoint mode. This uses the DesignWare core.
 
 config PCIE_KIRIN
 	depends on OF && ARM64
diff --git a/drivers/pci/dwc/pcie-artpec6.c b/drivers/pci/dwc/pcie-artpec6.c
index 3b635e745d25..413683d7bd9d 100644
--- a/drivers/pci/dwc/pcie-artpec6.c
+++ b/drivers/pci/dwc/pcie-artpec6.c
@@ -13,6 +13,7 @@ 
 #include <linux/delay.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
+#include <linux/of_device.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
 #include <linux/resource.h>
@@ -30,8 +31,15 @@  struct artpec6_pcie {
 	struct dw_pcie		*pci;
 	struct regmap		*regmap;	/* DT axis,syscon-pcie */
 	void __iomem		*phy_base;	/* DT phy */
+	enum dw_pcie_device_mode mode;
 };
 
+struct artpec_pcie_of_data {
+	enum dw_pcie_device_mode mode;
+};
+
+static const struct of_device_id artpec6_pcie_of_match[];
+
 /* PCIe Port Logic registers (memory-mapped) */
 #define PL_OFFSET			0x700
 #define PCIE_PHY_DEBUG_R0		(PL_OFFSET + 0x28)
@@ -42,6 +50,7 @@  struct artpec6_pcie {
 #define  PCIECFG_DBG_OEN		BIT(24)
 #define  PCIECFG_CORE_RESET_REQ		BIT(21)
 #define  PCIECFG_LTSSM_ENABLE		BIT(20)
+#define  PCIECFG_DEVICE_TYPE_MASK	GENMASK(19, 16)
 #define  PCIECFG_CLKREQ_B		BIT(11)
 #define  PCIECFG_REFCLK_ENABLE		BIT(10)
 #define  PCIECFG_PLL_ENABLE		BIT(9)
@@ -92,6 +101,22 @@  static int artpec6_pcie_establish_link(struct dw_pcie *pci)
 	return 0;
 }
 
+static void artpec6_pcie_stop_link(struct dw_pcie *pci)
+{
+	struct artpec6_pcie *artpec6_pcie = to_artpec6_pcie(pci);
+	u32 val;
+
+	val = artpec6_pcie_readl(artpec6_pcie, PCIECFG);
+	val &= ~PCIECFG_LTSSM_ENABLE;
+	artpec6_pcie_writel(artpec6_pcie, PCIECFG, val);
+}
+
+static const struct dw_pcie_ops dw_pcie_ops = {
+	.cpu_addr_fixup = artpec6_pcie_cpu_addr_fixup,
+	.start_link = artpec6_pcie_establish_link,
+	.stop_link = artpec6_pcie_stop_link,
+};
+
 static void artpec6_pcie_init_phy(struct artpec6_pcie *artpec6_pcie)
 {
 	u32 val;
@@ -157,6 +182,7 @@  static void artpec6_pcie_deassert_core_reset(struct artpec6_pcie *artpec6_pcie)
 	usleep_range(100, 200);
 }
 
+#ifdef CONFIG_PCIE_ARTPEC6_HOST
 static void artpec6_pcie_enable_interrupts(struct artpec6_pcie *artpec6_pcie)
 {
 	struct dw_pcie *pci = artpec6_pcie->pci;
@@ -231,11 +257,92 @@  static int artpec6_add_pcie_port(struct artpec6_pcie *artpec6_pcie,
 
 	return 0;
 }
+#else
+static inline int artpec6_add_pcie_port(struct artpec6_pcie *artpec6_pcie,
+					struct platform_device *pdev)
+{
+	return -ENODEV;
+}
+#endif
 
-static const struct dw_pcie_ops dw_pcie_ops = {
-	.cpu_addr_fixup = artpec6_pcie_cpu_addr_fixup,
+#ifdef CONFIG_PCIE_ARTPEC6_EP
+static void artpec6_pcie_ep_init(struct dw_pcie_ep *ep)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	struct artpec6_pcie *artpec6_pcie = to_artpec6_pcie(pci);
+	enum pci_barno bar;
+
+	artpec6_pcie_assert_core_reset(artpec6_pcie);
+	artpec6_pcie_init_phy(artpec6_pcie);
+	artpec6_pcie_deassert_core_reset(artpec6_pcie);
+
+	for (bar = BAR_0; bar <= BAR_5; bar++)
+		dw_pcie_ep_reset_bar(pci, bar);
+}
+
+static int artpec6_pcie_raise_irq(struct dw_pcie_ep *ep,
+				  enum pci_epc_irq_type type, u8 interrupt_num)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+
+	switch (type) {
+	case PCI_EPC_IRQ_LEGACY:
+		dev_err(pci->dev, "EP cannot trigger legacy IRQs\n");
+		return -EINVAL;
+	case PCI_EPC_IRQ_MSI:
+		return dw_pcie_ep_raise_msi_irq(ep, interrupt_num);
+	default:
+		dev_err(pci->dev, "UNKNOWN IRQ type\n");
+	}
+
+	return 0;
+}
+
+static struct dw_pcie_ep_ops pcie_ep_ops = {
+	.ep_init = artpec6_pcie_ep_init,
+	.raise_irq = artpec6_pcie_raise_irq,
 };
 
+static int artpec6_add_pcie_ep(struct artpec6_pcie *artpec6_pcie,
+			       struct platform_device *pdev)
+{
+	int ret;
+	struct dw_pcie_ep *ep;
+	struct resource *res;
+	struct device *dev = &pdev->dev;
+	struct dw_pcie *pci = artpec6_pcie->pci;
+
+	ep = &pci->ep;
+	ep->ops = &pcie_ep_ops;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi2");
+	pci->dbi_base2 = devm_ioremap(dev, res->start, resource_size(res));
+	if (IS_ERR(pci->dbi_base2))
+		return PTR_ERR(pci->dbi_base2);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
+	if (!res)
+		return -EINVAL;
+
+	ep->phys_base = res->start;
+	ep->addr_size = resource_size(res);
+
+	ret = dw_pcie_ep_init(ep);
+	if (ret) {
+		dev_err(dev, "failed to initialize endpoint\n");
+		return ret;
+	}
+
+	return 0;
+}
+#else
+static inline int artpec6_add_pcie_ep(struct artpec6_pcie *artpec6_pcie,
+				      struct platform_device *pdev)
+{
+	return -ENODEV;
+}
+#endif
+
 static int artpec6_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -244,6 +351,16 @@  static int artpec6_pcie_probe(struct platform_device *pdev)
 	struct resource *dbi_base;
 	struct resource *phy_base;
 	int ret;
+	const struct of_device_id *match;
+	const struct artpec_pcie_of_data *data;
+	enum dw_pcie_device_mode mode;
+
+	match = of_match_device(artpec6_pcie_of_match, dev);
+	if (!match)
+		return -EINVAL;
+
+	data = (struct artpec_pcie_of_data *)match->data;
+	mode = (enum dw_pcie_device_mode)data->mode;
 
 	artpec6_pcie = devm_kzalloc(dev, sizeof(*artpec6_pcie), GFP_KERNEL);
 	if (!artpec6_pcie)
@@ -257,6 +374,7 @@  static int artpec6_pcie_probe(struct platform_device *pdev)
 	pci->ops = &dw_pcie_ops;
 
 	artpec6_pcie->pci = pci;
+	artpec6_pcie->mode = mode;
 
 	dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
 	pci->dbi_base = devm_ioremap_resource(dev, dbi_base);
@@ -276,15 +394,47 @@  static int artpec6_pcie_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, artpec6_pcie);
 
-	ret = artpec6_add_pcie_port(artpec6_pcie, pdev);
-	if (ret < 0)
-		return ret;
+	switch (artpec6_pcie->mode) {
+	case DW_PCIE_RC_TYPE:
+		ret = artpec6_add_pcie_port(artpec6_pcie, pdev);
+		if (ret < 0)
+			return ret;
+		break;
+	case DW_PCIE_EP_TYPE: {
+		u32 val;
+
+		val = artpec6_pcie_readl(artpec6_pcie, PCIECFG);
+		val &= ~PCIECFG_DEVICE_TYPE_MASK;
+		artpec6_pcie_writel(artpec6_pcie, PCIECFG, val);
+		ret = artpec6_add_pcie_ep(artpec6_pcie, pdev);
+		if (ret < 0)
+			return ret;
+		break;
+	}
+	default:
+		dev_err(dev, "INVALID device type %d\n", artpec6_pcie->mode);
+	}
 
 	return 0;
 }
 
+static const struct artpec_pcie_of_data artpec6_pcie_rc_of_data = {
+	.mode = DW_PCIE_RC_TYPE,
+};
+
+static const struct artpec_pcie_of_data artpec6_pcie_ep_of_data = {
+	.mode = DW_PCIE_EP_TYPE,
+};
+
 static const struct of_device_id artpec6_pcie_of_match[] = {
-	{ .compatible = "axis,artpec6-pcie", },
+	{
+		.compatible = "axis,artpec6-pcie",
+		.data = &artpec6_pcie_rc_of_data,
+	},
+	{
+		.compatible = "axis,artpec6-pcie-ep",
+		.data = &artpec6_pcie_ep_of_data,
+	},
 	{},
 };