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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cdd13dd0-6595-c11c-72b2-6a1d140c2bbc@ti.com>
Date:   Thu, 19 Dec 2019 17:11:00 +0530
From:   Kishon Vijay Abraham I <kishon@...com>
To:     Andrew Murray <andrew.murray@....com>
CC:     Bjorn Helgaas <bhelgaas@...gle.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Rob Herring <robh+dt@...nel.org>,
        Arnd Bergmann <arnd@...db.de>, <linux-pci@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-omap@...r.kernel.org>
Subject: Re: [PATCH 03/13] PCI: cadence: Add support to use custom read and
 write accessors

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@...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