Linux-OMAP Archive on lore.kernel.org
 help / color / Atom feed
From: Kishon Vijay Abraham I <kishon@ti.com>
To: Rob Herring <robh@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Arnd Bergmann <arnd@arndb.de>, Tom Joseph <tjoseph@cadence.com>,
	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 v4 03/14] PCI: cadence: Add support to use custom read and write accessors
Date: Thu, 21 May 2020 19:03:15 +0530
Message-ID: <37d2d6c3-232d-adb8-4e0b-e0c699ec435a@ti.com> (raw)
In-Reply-To: <20200520220724.GA636352@bogus>

Hi Rob,

On 5/21/2020 3:37 AM, Rob Herring wrote:
> On Wed, May 06, 2020 at 08:44:18PM +0530, Kishon Vijay Abraham I wrote:
>> Add support to use custom read and write accessors. Platforms that
>> don't support half word or byte access or any other constraint
>> 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 of
>> the PCIe host bridge.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  drivers/pci/controller/cadence/pcie-cadence.h | 107 +++++++++++++++---
>>  1 file changed, 94 insertions(+), 13 deletions(-)
> 
> Actually, take back my R-by...
> 
>>
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
>> index df14ad002fe9..70b6b25153e8 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;
>> +	const struct cdns_pcie_ops *ops;
>>  };
>>  
>>  /**
>> @@ -299,69 +304,145 @@ struct cdns_pcie_ep {
>>  /* Register access */
>>  static inline void cdns_pcie_writeb(struct cdns_pcie *pcie, u32 reg, u8 value)
>>  {
>> -	writeb(value, pcie->reg_base + reg);
>> +	void __iomem *addr = pcie->reg_base + reg;
>> +
>> +	if (pcie->ops && pcie->ops->write) {
>> +		pcie->ops->write(addr, 0x1, value);
>> +		return;
>> +	}
>> +
>> +	writeb(value, addr);
>>  }
>>  
>>  static inline void cdns_pcie_writew(struct cdns_pcie *pcie, u32 reg, u16 value)
>>  {
>> -	writew(value, pcie->reg_base + reg);
>> +	void __iomem *addr = pcie->reg_base + reg;
>> +
>> +	if (pcie->ops && pcie->ops->write) {
>> +		pcie->ops->write(addr, 0x2, value);
>> +		return;
>> +	}
>> +
>> +	writew(value, addr);
>>  }
> 
> cdns_pcie_writeb and cdns_pcie_writew are used, so remove them.
> 
>>  
>>  static inline void cdns_pcie_writel(struct cdns_pcie *pcie, u32 reg, u32 value)
>>  {
>> -	writel(value, pcie->reg_base + reg);
>> +	void __iomem *addr = pcie->reg_base + reg;
>> +
>> +	if (pcie->ops && pcie->ops->write) {
>> +		pcie->ops->write(addr, 0x4, value);
>> +		return;
>> +	}
>> +
>> +	writel(value, addr);
> 
> writel isn't broken for you, so you don't need this either.
> 
>>  }
>>  
>>  static inline u32 cdns_pcie_readl(struct cdns_pcie *pcie, u32 reg)
>>  {
>> -	return readl(pcie->reg_base + reg);
>> +	void __iomem *addr = pcie->reg_base + reg;
>> +
>> +	if (pcie->ops && pcie->ops->read)
>> +		return pcie->ops->read(addr, 0x4);
>> +
>> +	return readl(addr);
> 
> And neither is readl.

Sure, I'll remove all the unused functions and avoid using ops for readl and
writel.
> 
>>  }
>>  
>>  /* Root Port register access */
>>  static inline void cdns_pcie_rp_writeb(struct cdns_pcie *pcie,
>>  				       u32 reg, u8 value)
>>  {
>> -	writeb(value, pcie->reg_base + CDNS_PCIE_RP_BASE + reg);
>> +	void __iomem *addr = pcie->reg_base + CDNS_PCIE_RP_BASE + reg;
>> +
>> +	if (pcie->ops && pcie->ops->write) {
>> +		pcie->ops->write(addr, 0x1, value);
>> +		return;
>> +	}
>> +
>> +	writeb(value, addr);
>>  }
>>  
>>  static inline void cdns_pcie_rp_writew(struct cdns_pcie *pcie,
>>  				       u32 reg, u16 value)
>>  {
>> -	writew(value, pcie->reg_base + CDNS_PCIE_RP_BASE + reg);
>> +	void __iomem *addr = pcie->reg_base + CDNS_PCIE_RP_BASE + reg;
>> +
>> +	if (pcie->ops && pcie->ops->write) {
>> +		pcie->ops->write(addr, 0x2, value);
>> +		return;
>> +	}
>> +
>> +	writew(value, addr);
> 
> You removed 2 out of 3 calls to this. I think I'd just make the root 
> port writes always be 32-bit. It is all just one time init stuff 
> anyways.
> 
> Either rework the calls to assemble the data into 32-bits or keep these 
> functions and do the RMW here.

The problem with assembling data into 32-bits is we have to read/write with
different offsets. We'll give PCI_VENDOR_ID offset for modifying deviceID,
PCI_INTERRUPT_LINE for modifying INTERRUPT_PIN which might get non-intuitive.
Similarly in endpoint we read and write to MSI_FLAGS (which is at offset 2) we
have to directly use MSI capability offset.

And doing RMW in the accessors would mean the same RMW op is repeated. So if we
just have cdns_pcie_rp_writeb() and cdns_pcie_rp_writew(), the same code will
be repeated here twice.

IMHO using ops is a lot cleaner for these cases. IMHO except for removing
unused functions and not changing readl/writel, others should use ops.

Kindly let me know what you think.

Thanks
Kishon

  reply index

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-06 15:14 [PATCH v4 00/14] Add PCIe support to TI's J721E SoC Kishon Vijay Abraham I
2020-05-06 15:14 ` [PATCH v4 01/14] PCI: cadence: Fix cdns_pcie_{host|ep}_setup() error path Kishon Vijay Abraham I
2020-05-20 20:59   ` Rob Herring
2020-05-06 15:14 ` [PATCH v4 02/14] linux/kernel.h: Add PTR_ALIGN_DOWN macro Kishon Vijay Abraham I
2020-05-20 21:00   ` Rob Herring
2020-05-06 15:14 ` [PATCH v4 03/14] PCI: cadence: Add support to use custom read and write accessors Kishon Vijay Abraham I
2020-05-20 21:02   ` Rob Herring
2020-05-20 22:07   ` Rob Herring
2020-05-21 13:33     ` Kishon Vijay Abraham I [this message]
2020-05-21 22:17       ` Rob Herring
2020-05-22  3:36         ` Kishon Vijay Abraham I
2020-05-06 15:14 ` [PATCH v4 04/14] PCI: cadence: Add support to start link and verify link status Kishon Vijay Abraham I
2020-05-20 21:06   ` Rob Herring
2020-05-06 15:14 ` [PATCH v4 05/14] PCI: cadence: Add read/write accessors to perform only 32-bit accesses Kishon Vijay Abraham I
2020-05-06 15:14 ` [PATCH v4 06/14] PCI: cadence: Allow pci_host_bridge to have custom pci_ops Kishon Vijay Abraham I
2020-05-06 15:14 ` [PATCH v4 07/14] PCI: cadence: Add new *ops* for CPU addr fixup Kishon Vijay Abraham I
2020-05-20 21:34   ` Rob Herring
2020-05-21 11:34     ` Kishon Vijay Abraham I
2020-05-22 16:45       ` Rob Herring
2020-05-23  1:24         ` Kishon Vijay Abraham I
2020-05-06 15:14 ` [PATCH v4 08/14] PCI: cadence: Fix updating Vendor ID and Subsystem Vendor ID register Kishon Vijay Abraham I
2020-05-20 21:36   ` Rob Herring
2020-05-06 15:14 ` [PATCH v4 09/14] PCI: cadence: Add MSI-X support to Endpoint driver Kishon Vijay Abraham I
2020-05-06 15:14 ` [PATCH v4 10/14] dt-bindings: PCI: Add host mode dt-bindings for TI's J721E SoC Kishon Vijay Abraham I
2020-05-06 15:14 ` [PATCH v4 11/14] dt-bindings: PCI: Add EP " Kishon Vijay Abraham I
2020-05-06 15:14 ` [PATCH v4 12/14] PCI: j721e: Add TI J721E PCIe driver Kishon Vijay Abraham I
2020-05-06 15:14 ` [PATCH v4 13/14] misc: pci_endpoint_test: Add J721E in pci_device_id table Kishon Vijay Abraham I
2020-05-20 22:12   ` Rob Herring
2020-05-06 15:14 ` [PATCH v4 14/14] MAINTAINERS: Add Kishon Vijay Abraham I for TI J721E SoC PCIe Kishon Vijay Abraham I
2020-05-20 22:12   ` Rob Herring
2020-05-18 11:14 ` [PATCH v4 00/14] Add PCIe support to TI's J721E SoC Kishon Vijay Abraham I

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=37d2d6c3-232d-adb8-4e0b-e0c699ec435a@ti.com \
    --to=kishon@ti.com \
    --cc=arnd@arndb.de \
    --cc=bhelgaas@google.com \
    --cc=devicetree@vger.kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-omap@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=robh@kernel.org \
    --cc=tjoseph@cadence.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-OMAP Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-omap/0 linux-omap/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-omap linux-omap/ https://lore.kernel.org/linux-omap \
		linux-omap@vger.kernel.org
	public-inbox-index linux-omap

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-omap


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git