[PATCH] PCI: keystone: update to support multiple pci ports

[PATCH] PCI: keystone: update to support multiple pci ports

[Murali Karicheri]

K2E SoC has two PCI ports. The SATA controller is connected to second
PCI port (port 1). This patch enhances the driver to support multiple
ports.

Update the DT Documentation for the new attribute, ti,pcie-port and
remove the note for bootargs as this is no longer needed.

Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>

CC: Rob Herring <robh+dt@kernel.org>
CC: Pawel Moll <pawel.moll@arm.com>
CC: Mark Rutland <mark.rutland@arm.com>
CC: Ian Campbell <ijc+devicetree@hellion.org.uk>
CC: Kumar Gala <galak@codeaurora.org>
CC: Bjorn Helgaas <bhelgaas@google.com>
CC: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 .../devicetree/bindings/pci/pci-keystone.txt       |    8 ++++---
 drivers/pci/host/pci-keystone.c                    |   23 ++++++++++++++------
 drivers/pci/host/pci-keystone.h                    |    4 ++--
 3 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/pci-keystone.txt b/Documentation/devicetree/bindings/pci/pci-keystone.txt
index ceb3e24..e279143 100644
--- a/Documentation/devicetree/bindings/pci/pci-keystone.txt
+++ b/Documentation/devicetree/bindings/pci/pci-keystone.txt
@@ -58,11 +58,13 @@ Optional properties:-
 	phy-names: name of the Generic Keystine SerDes phy for PCI
 	  - If boot loader already does PCI link establishment, then phys and
 	    phy-names shouldn't be present.
+	ti,pcie-port: PCI port number. This is used to configure the PCI port
+	    number. For example K2E SoC supports 2 PCI ports and PCI bindings
+	    for the second port adds ti,pcie-port = <1> to identify second port
+	    and driver uses this to configure the PCI mode register for second
+	    port. If not present, port number 0 is assumed.
 
 Designware DT Properties not applicable for Keystone PCI
 
 1. pcie_bus clock-names not used.  Instead, a phandle to phys is used.
 
-Note for PCI driver usage
-=========================
-Driver requires pci=pcie_bus_perf in the bootargs for proper functioning.
diff --git a/drivers/pci/host/pci-keystone.c b/drivers/pci/host/pci-keystone.c
index a132622..b4831e8 100644
--- a/drivers/pci/host/pci-keystone.c
+++ b/drivers/pci/host/pci-keystone.c
@@ -252,8 +252,8 @@ static int keystone_pcie_fault(unsigned long addr, unsigned int fsr,
 
 static void __init ks_pcie_host_init(struct pcie_port *pp)
 {
-	u32 vendor_device_id, val;
 	struct keystone_pcie *ks_pcie = to_keystone_pcie(pp);
+	u32 val;
 
 	ks_pcie_establish_link(ks_pcie);
 	ks_dw_pcie_setup_rc_app_regs(ks_pcie);
@@ -262,8 +262,7 @@ static void __init ks_pcie_host_init(struct pcie_port *pp)
 			pp->dbi_base + PCI_IO_BASE);
 
 	/* update the Vendor ID */
-	vendor_device_id = readl(ks_pcie->va_reg_pciid);
-	writew((vendor_device_id >> 16), pp->dbi_base + PCI_DEVICE_ID);
+	writew(ks_pcie->device_id, pp->dbi_base + PCI_DEVICE_ID);
 
 	/* update the DEV_STAT_CTRL to publish right mrrs */
 	val = readl(pp->dbi_base + PCIE_CAP_BASE + PCI_EXP_DEVCTL);
@@ -344,12 +343,13 @@ static int __exit ks_pcie_remove(struct platform_device *pdev)
 static int __init ks_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
 	struct keystone_pcie *ks_pcie;
+	int ret = 0, port_id = 0;
 	struct pcie_port *pp;
 	struct resource *res;
 	void __iomem *reg_p;
 	struct phy *phy;
-	int ret = 0;
 	u32 val;
 
 	ks_pcie = devm_kzalloc(&pdev->dev, sizeof(*ks_pcie),
@@ -360,17 +360,24 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
 	}
 	pp = &ks_pcie->pp;
 
+	/* read the PCI port id */
+	of_property_read_u32(np, "ti,pcie-port", &port_id);
+
 	/* index 2 is the devcfg register for RC mode settings */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
 	reg_p = devm_ioremap_resource(dev, res);
 	if (IS_ERR(reg_p))
 		return PTR_ERR(reg_p);
 
+
 	/* enable RC mode in devcfg */
 	val = readl(reg_p);
-	val &= ~PCIE_MODE_MASK;
-	val |= PCIE_RC_MODE;
+	port_id <<= 1;
+	val &= ~(PCIE_MODE_MASK << port_id);
+	val |= (PCIE_RC_MODE << port_id);
 	writel(val, reg_p);
+	devm_iounmap(dev, reg_p);
+	devm_release_mem_region(dev, res->start, resource_size(res));
 
 	/* initialize SerDes Phy if present */
 	phy = devm_phy_get(dev, "pcie-phy");
@@ -385,7 +392,9 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
 	reg_p = devm_ioremap_resource(dev, res);
 	if (IS_ERR(reg_p))
 		return PTR_ERR(reg_p);
-	ks_pcie->va_reg_pciid = reg_p;
+	ks_pcie->device_id = readl(reg_p) >> 16;
+	devm_iounmap(dev, reg_p);
+	devm_release_mem_region(dev, res->start, resource_size(res));
 
 	pp->dev = dev;
 	platform_set_drvdata(pdev, ks_pcie);
diff --git a/drivers/pci/host/pci-keystone.h b/drivers/pci/host/pci-keystone.h
index 729ea7d..80cfa8e 100644
--- a/drivers/pci/host/pci-keystone.h
+++ b/drivers/pci/host/pci-keystone.h
@@ -19,8 +19,8 @@
 struct keystone_pcie {
 	struct	clk		*clk;
 	struct	pcie_port	pp;
-	void __iomem		*va_reg_pciid;
-
+	/* PCI Device ID */
+	u32			device_id;
 	int			num_legacy_host_irqs;
 	int			legacy_host_irqs[MAX_LEGACY_HOST_IRQS];
 	struct			device_node *legacy_intc_np;
-- 
1.7.9.5

Re: [PATCH] PCI: keystone: update to support multiple pci ports

[Arnd Bergmann]

On Friday 05 September 2014 13:39:42 Murali Karicheri wrote:
> +
>         /* enable RC mode in devcfg */
>         val = readl(reg_p);
> -       val &= ~PCIE_MODE_MASK;
> -       val |= PCIE_RC_MODE;
> +       port_id <<= 1;
> +       val &= ~(PCIE_MODE_MASK << port_id);
> +       val |= (PCIE_RC_MODE << port_id);
>         writel(val, reg_p);
> +       devm_iounmap(dev, reg_p);
> +       devm_release_mem_region(dev, res->start, resource_size(res));

This looks like it's a shared register of some sort that doesn't
really belong into the registers of a particular port. Could it
be that it's actually for the PHY?

	Arnd

Re: [PATCH] PCI: keystone: update to support multiple pci ports

[Murali Karicheri]

On 09/05/2014 01:54 PM, Arnd Bergmann wrote:
> On Friday 05 September 2014 13:39:42 Murali Karicheri wrote:
>> +
>>          /* enable RC mode in devcfg */
>>          val = readl(reg_p);
>> -       val&= ~PCIE_MODE_MASK;
>> -       val |= PCIE_RC_MODE;
>> +       port_id<<= 1;
>> +       val&= ~(PCIE_MODE_MASK<<  port_id);
>> +       val |= (PCIE_RC_MODE<<  port_id);
>>          writel(val, reg_p);
>> +       devm_iounmap(dev, reg_p);
>> +       devm_release_mem_region(dev, res->start, resource_size(res));
>
> This looks like it's a shared register of some sort that doesn't
> really belong into the registers of a particular port. Could it
> be that it's actually for the PHY?
>
> 	Arnd
Arnd,

This a shared device configuration register between the two ports the 
desciption states it is bootstrap configuration of the PCIe module as 
Endpoint or Root complex and Not Phy. Hope below text will help.

Table 3-23 Device Configuration Register (DEVCFG)


PCIESSMODE[1:0] 00b PCIESSMODE is used to control the
functionality of PCIESS module out of
reset. This MMR output is connected to
DEVTYPE input of PCIESS
(Changes from
Nysh) : Note that in Nysh this value came
from a bootstrap pin.
00 : Endpoint
01 : Legacy Endpoint
10 : Rootcomplex
11 : Reserved
PCIESS_1_MODE[1:0
]
00b PCIESSMODE is used to control the
functionality of PCIE_1 module out of
reset. This MMR output is connected to
DEVTYPE input of PCIE_1
00 : Endpoint
01 : Legacy Endpoint
10 : Rootcomplex
11 : Reserv

Re: [PATCH] PCI: keystone: update to support multiple pci ports

[Arnd Bergmann]

On Friday 05 September 2014 14:33:54 Murali Karicheri wrote:
> > This looks like it's a shared register of some sort that doesn't
> > really belong into the registers of a particular port. Could it
> > be that it's actually for the PHY?
> >
> This a shared device configuration register between the two ports the 
> desciption states it is bootstrap configuration of the PCIe module as 
> Endpoint or Root complex and Not Phy. Hope below text will help.

Ok. Why do you want to have this user-selectable though? Can't it
just be set by the boot loader before starting Linux?

	Arnd

> Table 3-23 Device Configuration Register (DEVCFG)
> 
> 
> PCIESSMODE[1:0] 00b PCIESSMODE is used to control the
> functionality of PCIESS module out of
> reset. This MMR output is connected to
> DEVTYPE input of PCIESS
> (Changes from
> Nysh) : Note that in Nysh this value came
> from a bootstrap pin.
> 00 : Endpoint
> 01 : Legacy Endpoint
> 10 : Rootcomplex
> 11 : Reserved
> PCIESS_1_MODE[1:0
> ]
> 00b PCIESSMODE is used to control the
> functionality of PCIE_1 module out of
> reset. This MMR output is connected to
> DEVTYPE input of PCIE_1
> 00 : Endpoint
> 01 : Legacy Endpoint
> 10 : Rootcomplex
> 11 : Reserv
> 

Re: [PATCH] PCI: keystone: update to support multiple pci ports

[Murali Karicheri]

On 09/05/2014 03:00 PM, Arnd Bergmann wrote:
> On Friday 05 September 2014 14:33:54 Murali Karicheri wrote:
>>> This looks like it's a shared register of some sort that doesn't
>>> really belong into the registers of a particular port. Could it
>>> be that it's actually for the PHY?
>>>
>> This a shared device configuration register between the two ports the
>> desciption states it is bootstrap configuration of the PCIe module as
>> Endpoint or Root complex and Not Phy. Hope below text will help.
>
> Ok. Why do you want to have this user-selectable though? Can't it
> just be set by the boot loader before starting Linux?

Arnd,

As the driver is responsible for configuring the device to support the 
device functionality, it make sense to do this in the device driver. The 
driver enables clock to the IP and this is an addition thing to be 
configured so that when the device is powered up, it should function as 
RC. The IP can be configured to work as Root Complex or Endpoint. So not 
sure why you want to me to move this functionality to boot loader.

Murali

>
> 	Arnd
>
>> Table 3-23 Device Configuration Register (DEVCFG)
>>
>>
>> PCIESSMODE[1:0] 00b PCIESSMODE is used to control the
>> functionality of PCIESS module out of
>> reset. This MMR output is connected to
>> DEVTYPE input of PCIESS
>> (Changes from
>> Nysh) : Note that in Nysh this value came
>> from a bootstrap pin.
>> 00 : Endpoint
>> 01 : Legacy Endpoint
>> 10 : Rootcomplex
>> 11 : Reserved
>> PCIESS_1_MODE[1:0
>> ]
>> 00b PCIESSMODE is used to control the
>> functionality of PCIE_1 module out of
>> reset. This MMR output is connected to
>> DEVTYPE input of PCIE_1
>> 00 : Endpoint
>> 01 : Legacy Endpoint
>> 10 : Rootcomplex
>> 11 : Reserv
>>
>

Re: [PATCH] PCI: keystone: update to support multiple pci ports

[Arnd Bergmann]

On Friday 05 September 2014 16:37:25 Murali Karicheri wrote:
> On 09/05/2014 03:00 PM, Arnd Bergmann wrote:
> > On Friday 05 September 2014 14:33:54 Murali Karicheri wrote:
> >>> This looks like it's a shared register of some sort that doesn't
> >>> really belong into the registers of a particular port. Could it
> >>> be that it's actually for the PHY?
> >>>
> >> This a shared device configuration register between the two ports the
> >> desciption states it is bootstrap configuration of the PCIe module as
> >> Endpoint or Root complex and Not Phy. Hope below text will help.
> >
> > Ok. Why do you want to have this user-selectable though? Can't it
> > just be set by the boot loader before starting Linux?
> 
> Arnd,
> 
> As the driver is responsible for configuring the device to support the 
> device functionality, it make sense to do this in the device driver. The 
> driver enables clock to the IP and this is an addition thing to be 
> configured so that when the device is powered up, it should function as 
> RC. The IP can be configured to work as Root Complex or Endpoint. So not 
> sure why you want to me to move this functionality to boot loader.

But the driver can only do root complex mode, and we would probably
want a completely different driver if we were to start supporting
endpoint mode.

This also implies that the firmware has to pass a different DT for
endpoint mode, so it should be responsible for setting up the hardware
to match the DT.

	Arnd

Re: [PATCH] PCI: keystone: update to support multiple pci ports

[Murali Karicheri]

On 09/05/2014 05:11 PM, Arnd Bergmann wrote:
> On Friday 05 September 2014 16:37:25 Murali Karicheri wrote:
>> On 09/05/2014 03:00 PM, Arnd Bergmann wrote:
>>> On Friday 05 September 2014 14:33:54 Murali Karicheri wrote:
>>>>> This looks like it's a shared register of some sort that doesn't
>>>>> really belong into the registers of a particular port. Could it
>>>>> be that it's actually for the PHY?
>>>>>
>>>> This a shared device configuration register between the two ports the
>>>> desciption states it is bootstrap configuration of the PCIe module as
>>>> Endpoint or Root complex and Not Phy. Hope below text will help.
>>>
>>> Ok. Why do you want to have this user-selectable though? Can't it
>>> just be set by the boot loader before starting Linux?
>>
>> Arnd,
>>
>> As the driver is responsible for configuring the device to support the
>> device functionality, it make sense to do this in the device driver. The
>> driver enables clock to the IP and this is an addition thing to be
>> configured so that when the device is powered up, it should function as
>> RC. The IP can be configured to work as Root Complex or Endpoint. So not
>> sure why you want to me to move this functionality to boot loader.
>
> But the driver can only do root complex mode, and we would probably
> want a completely different driver if we were to start supporting
> endpoint mode.
>
Arnd,

Good point! I will drop index#2 handling in the driver code and will 
handle the same in boot loader. But I have a question though. The 
original driver which is queued up for merge to v3.18 has index #2 for 
this reg offset and is documented in the DT documentation as

	index 2 is the base address and length of PCI mode configuration
	register.
	index 3 is the base address and length of PCI device ID register.


Will this create any issue in terms of backward compatibility if I 
remove it and move index3 to index2 and update the code for the same? I 
assume since this patch also will likely be on the next branch soon, and 
gets merged together with original driver to v3.18, this should be fine. 
But for some reason, if this patch doesn't make to v3.18, then won't 
this break the backward compatibility?

I think the other option is document index2 as obsolete and update the 
document and remove the code for handling it. Any suggestion?

Thanks

Murali

> This also implies that the firmware has to pass a different DT for
> endpoint mode, so it should be responsible for setting up the hardware
> to match the DT.
>
> 	Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] PCI: keystone: update to support multiple pci ports

[Arnd Bergmann]

On Monday 08 September 2014 11:52:47 Murali Karicheri wrote:
> On 09/05/2014 05:11 PM, Arnd Bergmann wrote:
> > But the driver can only do root complex mode, and we would probably
> > want a completely different driver if we were to start supporting
> > endpoint mode.
> 
> Good point! I will drop index#2 handling in the driver code and will 
> handle the same in boot loader. But I have a question though. The 
> original driver which is queued up for merge to v3.18 has index #2 for 
> this reg offset and is documented in the DT documentation as
> 
> 	index 2 is the base address and length of PCI mode configuration
> 	register.
> 	index 3 is the base address and length of PCI device ID register.
> 
> 
> Will this create any issue in terms of backward compatibility if I 
> remove it and move index3 to index2 and update the code for the same? I 
> assume since this patch also will likely be on the next branch soon, and 
> gets merged together with original driver to v3.18, this should be fine. 
> But for some reason, if this patch doesn't make to v3.18, then won't 
> this break the backward compatibility?
> 
> I think the other option is document index2 as obsolete and update the 
> document and remove the code for handling it. Any suggestion?

Since the driver is not merged into Linus' tree yet and (more importantly)
has not been in a release yet, we can still fix it. Please just send a
patch on top to remove it now, it's not too late yet.

	Arnd