Re: [PATCH 03/13] PCI: cadence: Add support to use custom read and write accessors

[Kishon Vijay Abraham I <kishon@ti.com>]

Hi,

On 16/12/19 7:37 pm, Andrew Murray wrote:
> On Mon, Dec 09, 2019 at 02:51:37PM +0530, Kishon Vijay Abraham I wrote:
>> Add support to use custom read and write accessors. Platforms that
>> doesn't support half word or byte access or any other constraint
> 
> s/doesn't/don't/
> 
>> while accessing registers can use this feature to populate custom
>> read and write accessors. These custom accessors are used for both
>> standard register access and configuration space register access.
> 
> You can put the following sentence underneath a --- as it's not needed
> in the commit message (but may be helpful to reviewers).
> 
>> This is in preparation for adding PCIe support in TI's J721E SoC which
>> uses Cadence PCIe core.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  drivers/pci/controller/cadence/pcie-cadence.h | 99 +++++++++++++++++--
>>  1 file changed, 90 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
>> index a2b28b912ca4..d0d91c69fa1d 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence.h
>> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
>> @@ -223,6 +223,11 @@ enum cdns_pcie_msg_routing {
>>  	MSG_ROUTING_GATHER,
>>  };
>>  
>> +struct cdns_pcie_ops {
>> +	u32	(*read)(void __iomem *addr, int size);
>> +	void	(*write)(void __iomem *addr, int size, u32 value);
>> +};
>> +
>>  /**
>>   * struct cdns_pcie - private data for Cadence PCIe controller drivers
>>   * @reg_base: IO mapped register base
>> @@ -239,7 +244,7 @@ struct cdns_pcie {
>>  	int			phy_count;
>>  	struct phy		**phy;
>>  	struct device_link	**link;
>> -	const struct cdns_pcie_common_ops *ops;
> 
> What was cdns_pcie_common_ops? It's not defined in the current tree is it?

Yeah, it's spurious change that has got merged.
> 
>> +	const struct cdns_pcie_ops *ops;
>>  };
>>  
>>  /**
>> @@ -301,21 +306,47 @@ struct cdns_pcie_ep {
>>  /* Register access */
>>  static inline void cdns_pcie_writeb(struct cdns_pcie *pcie, u32 reg, u8 value)
>>  {
>> +	void __iomem *addr = pcie->reg_base + reg;
>> +
>> +	if (pcie->ops && pcie->ops->write) {
>> +		pcie->ops->write(addr, 0x1, value);
>> +		return;
>> +	}
>> +
>>  	writeb(value, pcie->reg_base + reg);
> 
> Can you use 'addr' here instead of 'pcie->reg_base + reg'? (And similar for the
> rest of them).

Sure.

Thanks
Kishon