[v6,08/14] PCI: cadence: Fix updating Vendor ID and Subsystem Vendor ID register
diff mbox series

Message ID 20200708093018.28474-9-kishon@ti.com
State Superseded
Delegated to: Lorenzo Pieralisi
Headers show
Series
  • Add PCIe support to TI's J721E SoC
Related show

Commit Message

Kishon Vijay Abraham I July 8, 2020, 9:30 a.m. UTC
Commit 1b79c5284439 ("PCI: cadence: Add host driver for Cadence PCIe
controller") in order to update Vendor ID, directly wrote to
PCI_VENDOR_ID register. However PCI_VENDOR_ID in root port configuration
space is read-only register and writing to it will have no effect.
Use local management register to configure Vendor ID and Subsystem Vendor
ID.

Fixes: 1b79c5284439 ("PCI: cadence: Add host driver for Cadence PCIe controller")
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/cadence/pcie-cadence-host.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Rob Herring July 9, 2020, 9:45 p.m. UTC | #1
On Wed, Jul 08, 2020 at 03:00:12PM +0530, Kishon Vijay Abraham I wrote:
> Commit 1b79c5284439 ("PCI: cadence: Add host driver for Cadence PCIe
> controller") in order to update Vendor ID, directly wrote to
> PCI_VENDOR_ID register. However PCI_VENDOR_ID in root port configuration
> space is read-only register and writing to it will have no effect.
> Use local management register to configure Vendor ID and Subsystem Vendor
> ID.
> 
> Fixes: 1b79c5284439 ("PCI: cadence: Add host driver for Cadence PCIe controller")
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/cadence/pcie-cadence-host.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> index 10127ea71b83..8935f7a37e5a 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> @@ -82,6 +82,7 @@ static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)
>  {
>  	struct cdns_pcie *pcie = &rc->pcie;
>  	u32 value, ctrl;
> +	u32 id;
>  
>  	/*
>  	 * Set the root complex BAR configuration register:
> @@ -101,8 +102,12 @@ static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)
>  	cdns_pcie_writel(pcie, CDNS_PCIE_LM_RC_BAR_CFG, value);
>  
>  	/* Set root port configuration space */
> -	if (rc->vendor_id != 0xffff)
> -		cdns_pcie_rp_writew(pcie, PCI_VENDOR_ID, rc->vendor_id);

If this is read-only, then...

> +	if (rc->vendor_id != 0xffff) {
> +		id = CDNS_PCIE_LM_ID_VENDOR(rc->vendor_id) |
> +			CDNS_PCIE_LM_ID_SUBSYS(rc->vendor_id);
> +		cdns_pcie_writel(pcie, CDNS_PCIE_LM_ID, id);
> +	}
> +
>  	if (rc->device_id != 0xffff)
>  		cdns_pcie_rp_writew(pcie, PCI_DEVICE_ID, rc->device_id);

...isn't this read-only too?

>  
> -- 
> 2.17.1
>
Kishon Vijay Abraham I July 13, 2020, 6:18 a.m. UTC | #2
Hi Rob,

On 7/10/2020 3:15 AM, Rob Herring wrote:
> On Wed, Jul 08, 2020 at 03:00:12PM +0530, Kishon Vijay Abraham I wrote:
>> Commit 1b79c5284439 ("PCI: cadence: Add host driver for Cadence PCIe
>> controller") in order to update Vendor ID, directly wrote to
>> PCI_VENDOR_ID register. However PCI_VENDOR_ID in root port configuration
>> space is read-only register and writing to it will have no effect.
>> Use local management register to configure Vendor ID and Subsystem Vendor
>> ID.
>>
>> Fixes: 1b79c5284439 ("PCI: cadence: Add host driver for Cadence PCIe controller")
>> Reviewed-by: Rob Herring <robh@kernel.org>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  drivers/pci/controller/cadence/pcie-cadence-host.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
>> index 10127ea71b83..8935f7a37e5a 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
>> @@ -82,6 +82,7 @@ static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)
>>  {
>>  	struct cdns_pcie *pcie = &rc->pcie;
>>  	u32 value, ctrl;
>> +	u32 id;
>>  
>>  	/*
>>  	 * Set the root complex BAR configuration register:
>> @@ -101,8 +102,12 @@ static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)
>>  	cdns_pcie_writel(pcie, CDNS_PCIE_LM_RC_BAR_CFG, value);
>>  
>>  	/* Set root port configuration space */
>> -	if (rc->vendor_id != 0xffff)
>> -		cdns_pcie_rp_writew(pcie, PCI_VENDOR_ID, rc->vendor_id);
> 
> If this is read-only, then...
> 
>> +	if (rc->vendor_id != 0xffff) {
>> +		id = CDNS_PCIE_LM_ID_VENDOR(rc->vendor_id) |
>> +			CDNS_PCIE_LM_ID_SUBSYS(rc->vendor_id);
>> +		cdns_pcie_writel(pcie, CDNS_PCIE_LM_ID, id);
>> +	}
>> +
>>  	if (rc->device_id != 0xffff)
>>  		cdns_pcie_rp_writew(pcie, PCI_DEVICE_ID, rc->device_id);
> 
> ...isn't this read-only too?

Apparently no. There is local management register only for vendor ID and
subsystem vendorID. It could be because this same IP is used EP mode and in
case of multi-function device, different deviceID could be required for each
independent function whereas vendor ID should be same, they could have given a
single local management register for vendorID and not for deviceID.

Thanks
Kishon

Patch
diff mbox series

diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
index 10127ea71b83..8935f7a37e5a 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-host.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
@@ -82,6 +82,7 @@  static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)
 {
 	struct cdns_pcie *pcie = &rc->pcie;
 	u32 value, ctrl;
+	u32 id;
 
 	/*
 	 * Set the root complex BAR configuration register:
@@ -101,8 +102,12 @@  static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)
 	cdns_pcie_writel(pcie, CDNS_PCIE_LM_RC_BAR_CFG, value);
 
 	/* Set root port configuration space */
-	if (rc->vendor_id != 0xffff)
-		cdns_pcie_rp_writew(pcie, PCI_VENDOR_ID, rc->vendor_id);
+	if (rc->vendor_id != 0xffff) {
+		id = CDNS_PCIE_LM_ID_VENDOR(rc->vendor_id) |
+			CDNS_PCIE_LM_ID_SUBSYS(rc->vendor_id);
+		cdns_pcie_writel(pcie, CDNS_PCIE_LM_ID, id);
+	}
+
 	if (rc->device_id != 0xffff)
 		cdns_pcie_rp_writew(pcie, PCI_DEVICE_ID, rc->device_id);