diff mbox

[v2,2/2] PCI: keystone: update to support multiple pci ports

Message ID 1410293835-13050-2-git-send-email-m-karicheri2@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Murali Karicheri Sept. 9, 2014, 8:17 p.m. UTC
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>
---
 - updated based on comment against v1
 .../devicetree/bindings/pci/pci-keystone.txt       |    8 +++++---
 drivers/pci/host/pci-keystone.c                    |   15 ++++++++++-----
 drivers/pci/host/pci-keystone.h                    |    4 ++--
 3 files changed, 17 insertions(+), 10 deletions(-)

Comments

Arnd Bergmann Sept. 9, 2014, 8:28 p.m. UTC | #1
On Tuesday 09 September 2014 16:17:15 Murali Karicheri wrote:
> 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>
> ---
>  - updated based on comment against v1
>  .../devicetree/bindings/pci/pci-keystone.txt       |    8 +++++---
>  drivers/pci/host/pci-keystone.c                    |   15 ++++++++++-----
>  drivers/pci/host/pci-keystone.h                    |    4 ++--
>  3 files changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/pci-keystone.txt b/Documentation/devicetree/bindings/pci/pci-keystone.txt
> index bedacf0..c8f5773 100644
> --- a/Documentation/devicetree/bindings/pci/pci-keystone.txt
> +++ b/Documentation/devicetree/bindings/pci/pci-keystone.txt
> @@ -56,11 +56,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.

Is this still needed? I thought with patch 1 removed, we no longer had
a reason to know the port number.

> -=========================
> -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 f1119eb..893820d 100644
> --- a/drivers/pci/host/pci-keystone.c
> +++ b/drivers/pci/host/pci-keystone.c
> @@ -253,8 +253,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);
> @@ -263,8 +263,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);

This change must have slipped in accidentally, at least it's not
described in the changelog. Should this be another patch?
The change seems useful.

	Arnd
Murali Karicheri Sept. 9, 2014, 8:42 p.m. UTC | #2
On 09/09/2014 04:28 PM, Arnd Bergmann wrote:
> On Tuesday 09 September 2014 16:17:15 Murali Karicheri wrote:
>> 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>
>> ---
>>   - updated based on comment against v1
>>   .../devicetree/bindings/pci/pci-keystone.txt       |    8 +++++---
>>   drivers/pci/host/pci-keystone.c                    |   15 ++++++++++-----
>>   drivers/pci/host/pci-keystone.h                    |    4 ++--
>>   3 files changed, 17 insertions(+), 10 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/pci-keystone.txt b/Documentation/devicetree/bindings/pci/pci-keystone.txt
>> index bedacf0..c8f5773 100644
>> --- a/Documentation/devicetree/bindings/pci/pci-keystone.txt
>> +++ b/Documentation/devicetree/bindings/pci/pci-keystone.txt
>> @@ -56,11 +56,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.
>
> Is this still needed? I thought with patch 1 removed, we no longer had
> a reason to know the port number.
>
You are right. I will remove it and re-send.

>> -=========================
>> -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 f1119eb..893820d 100644
>> --- a/drivers/pci/host/pci-keystone.c
>> +++ b/drivers/pci/host/pci-keystone.c
>> @@ -253,8 +253,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);
>> @@ -263,8 +263,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);
>
> This change must have slipped in accidentally, at least it's not
> described in the changelog. Should this be another patch?
> The change seems useful.
Are you referring to mrrs or update to device id?  device id is in a SoC 
register at index2 and is read and updated by the driver here. MRRS 
update was originally in the code.

Murali
>
> 	Arnd
Arnd Bergmann Sept. 9, 2014, 9:09 p.m. UTC | #3
On Tuesday 09 September 2014 16:42:46 Murali Karicheri wrote:
> >>
> >>      /* 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);
> >
> > This change must have slipped in accidentally, at least it's not
> > described in the changelog. Should this be another patch?
> > The change seems useful.
> Are you referring to mrrs or update to device id?  device id is in a SoC 
> register at index2 and is read and updated by the driver here. MRRS 
> update was originally in the code.
> 

I meant the device id change. Maybe you accidentally did 'git commit
--amend' during a rebase and that replaced the real changelog with
the one of the patch in front of it and merged the two patches?

That happened to me a few times and would explain the strange mix
of two changes.

	Arnd
Murali Karicheri Sept. 9, 2014, 9:49 p.m. UTC | #4
On 09/09/2014 05:09 PM, Arnd Bergmann wrote:
> On Tuesday 09 September 2014 16:42:46 Murali Karicheri wrote:
>>>>
>>>>       /* 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);
>>>
>>> This change must have slipped in accidentally, at least it's not
>>> described in the changelog. Should this be another patch?
>>> The change seems useful.
>> Are you referring to mrrs or update to device id?  device id is in a SoC
>> register at index2 and is read and updated by the driver here. MRRS
>> update was originally in the code.
>>
>
> I meant the device id change. Maybe you accidentally did 'git commit
> --amend' during a rebase and that replaced the real changelog with
> the one of the patch in front of it and merged the two patches?
>
Actually this is an inteded. The vendor ID is in a register indicated by
reg offset and as per the device spec, it needs to be read and updated 
by the software. Now since multiple instances of PCI device needs to be 
read the same register, the reading happens in the probe() and same is 
unmapped after that.

+	ks_pcie->device_id = readl(reg_p) >> 16;
+	devm_iounmap(dev, reg_p);
+	devm_release_mem_region(dev, res->start, resource_size(res));

Afetr that in ks_pcie_host_init(), it update the device_id in the RC's 
config space.

Thanks

Murali
> That happened to me a few times and would explain the strange mix
> of two changes.
>
> 	Arnd
Murali Karicheri Sept. 9, 2014, 9:51 p.m. UTC | #5
On 09/09/2014 05:49 PM, Murali Karicheri wrote:
> On 09/09/2014 05:09 PM, Arnd Bergmann wrote:
>> On Tuesday 09 September 2014 16:42:46 Murali Karicheri wrote:
>>>>>
>>>>> /* 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);
>>>>
>>>> This change must have slipped in accidentally, at least it's not
>>>> described in the changelog. Should this be another patch?
>>>> The change seems useful.
>>> Are you referring to mrrs or update to device id? device id is in a SoC
>>> register at index2 and is read and updated by the driver here. MRRS
>>> update was originally in the code.
>>>
>>
>> I meant the device id change. Maybe you accidentally did 'git commit
>> --amend' during a rebase and that replaced the real changelog with
>> the one of the patch in front of it and merged the two patches?
>>
> Actually this is an inteded. The vendor ID is in a register indicated by
> reg offset and as per the device spec, it needs to be read and updated
> by the software. Now since multiple instances of PCI device needs to be
> read the same register, the reading happens in the probe() and same is
> unmapped after that.
>
> + ks_pcie->device_id = readl(reg_p) >> 16;
> + devm_iounmap(dev, reg_p);
> + devm_release_mem_region(dev, res->start, resource_size(res));
>
> Afetr that in ks_pcie_host_init(), it update the device_id in the RC's
> config space.
>
BTW, I will update the commit log with more description to indicate the 
above and re-send it if this is fine.

> Thanks
>
> Murali
>> That happened to me a few times and would explain the strange mix
>> of two changes.
>>
>> Arnd
>
Arnd Bergmann Sept. 9, 2014, 9:52 p.m. UTC | #6
On Tuesday 09 September 2014 17:49:19 Murali Karicheri wrote:
> Actually this is an inteded. The vendor ID is in a register indicated by
> reg offset and as per the device spec, it needs to be read and updated 
> by the software. Now since multiple instances of PCI device needs to be 
> read the same register, the reading happens in the probe() and same is 
> unmapped after that.
> 
> +       ks_pcie->device_id = readl(reg_p) >> 16;
> +       devm_iounmap(dev, reg_p);
> +       devm_release_mem_region(dev, res->start, resource_size(res));
> 
> Afetr that in ks_pcie_host_init(), it update the device_id in the RC's 
> config space.

I'm not sure I understand the purpose of this. Do you mean you read
the vendor/device ID of whichever device happens to get probed first
and then program the same ID into all other devices as well?

What if the order changes between two boots? Why does the vendor/device
ID of the host bridge even matter at all?

	Arnd
Murali Karicheri Sept. 9, 2014, 10:50 p.m. UTC | #7
On 09/09/2014 05:52 PM, Arnd Bergmann wrote:
> On Tuesday 09 September 2014 17:49:19 Murali Karicheri wrote:
>> Actually this is an inteded. The vendor ID is in a register indicated by
>> reg offset and as per the device spec, it needs to be read and updated
>> by the software. Now since multiple instances of PCI device needs to be
>> read the same register, the reading happens in the probe() and same is
>> unmapped after that.
>>
>> +       ks_pcie->device_id = readl(reg_p)>>  16;
>> +       devm_iounmap(dev, reg_p);
>> +       devm_release_mem_region(dev, res->start, resource_size(res));
>>
>> Afetr that in ks_pcie_host_init(), it update the device_id in the RC's
>> config space.
>
> I'm not sure I understand the purpose of this. Do you mean you read
> the vendor/device ID of whichever device happens to get probed first
> and then program the same ID into all other devices as well?

My mistake. It is the device ID, not vendor ID. The PCI driver supports 
PCI h/w on K2HK, K2E and K2L SoCs for which PCI device IDs are assigned as

+#define PCIE_RC_K2HK		0xb008
+#define PCIE_RC_K2E		0xb009
+#define PCIE_RC_K2L		0xb00a
+

and the same driver code runs on all these h/w. The device ID is not 
filled in by default by the h/w, in the config space of the RC at offset 
1000h "VENDOR_DEVICE_ID Vendor and Device Identification Register". 
Same is available in a seperate SoC register whose offset is specified 
by index 2. This is read by driver and updated in the config space. The 
Vendor ID is however set by default.

There is a mrrs PCI quirk required for Keystone PCI which depends on 
this vendor ID to be filled correctly as it match vendor id/ device id 
of the bridge device to apply the quirk.

Hope this clarify your question.

Murali

>
> What if the order changes between two boots? Why does the vendor/device
> ID of the host bridge even matter at all?
>
> 	Arnd
Arnd Bergmann Sept. 10, 2014, 8:22 a.m. UTC | #8
On Tuesday 09 September 2014 18:50:13 Murali Karicheri wrote:
> My mistake. It is the device ID, not vendor ID. The PCI driver supports 
> PCI h/w on K2HK, K2E and K2L SoCs for which PCI device IDs are assigned as
> 
> +#define PCIE_RC_K2HK           0xb008
> +#define PCIE_RC_K2E            0xb009
> +#define PCIE_RC_K2L            0xb00a
> +
> 
> and the same driver code runs on all these h/w. The device ID is not 
> filled in by default by the h/w, in the config space of the RC at offset 
> 1000h "VENDOR_DEVICE_ID Vendor and Device Identification Register". 
> Same is available in a seperate SoC register whose offset is specified 
> by index 2. This is read by driver and updated in the config space. The 
> Vendor ID is however set by default.
> 
> There is a mrrs PCI quirk required for Keystone PCI which depends on 
> this vendor ID to be filled correctly as it match vendor id/ device id 
> of the bridge device to apply the quirk.
> 
> Hope this clarify your question.

Ok, I understand now. Yes, this makes sense, though I wonder if it would
have been easier to handle the quirk in a different way based on the
driver rather than the PCI ID. It's probably not worth revisiting though,
unless Bjorn wants to see it done differently now.

	Arnd
Murali Karicheri Sept. 10, 2014, 2:28 p.m. UTC | #9
On 09/10/2014 04:22 AM, Arnd Bergmann wrote:
> On Tuesday 09 September 2014 18:50:13 Murali Karicheri wrote:
>> My mistake. It is the device ID, not vendor ID. The PCI driver supports
>> PCI h/w on K2HK, K2E and K2L SoCs for which PCI device IDs are assigned as
>>
>> +#define PCIE_RC_K2HK           0xb008
>> +#define PCIE_RC_K2E            0xb009
>> +#define PCIE_RC_K2L            0xb00a
>> +
>>
>> and the same driver code runs on all these h/w. The device ID is not
>> filled in by default by the h/w, in the config space of the RC at offset
>> 1000h "VENDOR_DEVICE_ID Vendor and Device Identification Register".
>> Same is available in a seperate SoC register whose offset is specified
>> by index 2. This is read by driver and updated in the config space. The
>> Vendor ID is however set by default.
>>
>> There is a mrrs PCI quirk required for Keystone PCI which depends on
>> this vendor ID to be filled correctly as it match vendor id/ device id
>> of the bridge device to apply the quirk.
>>
>> Hope this clarify your question.
>
> Ok, I understand now. Yes, this makes sense, though I wonder if it would
> have been easier to handle the quirk in a different way based on the
> driver rather than the PCI ID. It's probably not worth revisiting though,
> unless Bjorn wants to see it done differently now.

Bjorn has reviewed the quirk patch and wanted to check the device ID and 
vendor ID to avoid applying the quirck when bridge is non TI and same 
device ID.

So support for two PCI port requires only the PCI device id related 
change. So I will modify the subject and commit log to reflect the same 
and re-send the patch.

Thanks again Arnd for your comments.

Regards,

Murali
>
> 	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
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/pci/pci-keystone.txt b/Documentation/devicetree/bindings/pci/pci-keystone.txt
index bedacf0..c8f5773 100644
--- a/Documentation/devicetree/bindings/pci/pci-keystone.txt
+++ b/Documentation/devicetree/bindings/pci/pci-keystone.txt
@@ -56,11 +56,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 f1119eb..893820d 100644
--- a/drivers/pci/host/pci-keystone.c
+++ b/drivers/pci/host/pci-keystone.c
@@ -253,8 +253,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);
@@ -263,8 +263,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);
@@ -345,12 +344,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;
 
 	ks_pcie = devm_kzalloc(&pdev->dev, sizeof(*ks_pcie),
 				GFP_KERNEL);
@@ -360,6 +360,9 @@  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);
+
 	/* initialize SerDes Phy if present */
 	phy = devm_phy_get(dev, "pcie-phy");
 	if (!IS_ERR_OR_NULL(phy)) {
@@ -373,7 +376,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;