Re: [PATCH v6 08/14] PCI: cadence: Fix updating Vendor ID and Subsystem Vendor ID register

From: Kishon Vijay Abraham I <kishon@ti.com>
To: Rob Herring <robh@kernel.org>
Cc: Tom Joseph <tjoseph@cadence.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Arnd Bergmann <arnd@arndb.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	<linux-pci@vger.kernel.org>, <devicetree@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <linux-omap@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v6 08/14] PCI: cadence: Fix updating Vendor ID and Subsystem Vendor ID register
Date: Mon, 13 Jul 2020 11:48:21 +0530	[thread overview]
Message-ID: <9e9dd21b-7626-ef06-4b63-f801a297082e@ti.com> (raw)
In-Reply-To: <20200709214555.GA939109@bogus>

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