Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
From: Bjorn Helgaas <helgaas@kernel.org>
To: "Krzysztof Wilczyński" <kw@linux.com>
Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Heiko Stuebner <heiko@sntech.de>,
	linux-pci@vger.kernel.org, Shawn Lin <shawn.lin@rock-chips.com>,
	Michal Simek <michal.simek@xilinx.com>,
	linux-rockchip@lists.infradead.org,
	Zhou Wang <wangzhou1@hisilicon.com>,
	Robert Richter <rrichter@marvell.com>,
	Jonathan Chocron <jonnyc@amazon.com>,
	Will Deacon <will@kernel.org>,
	linux-arm-kernel@lists.infradead.org,
	Rob Herring <robh@kernel.org>
Subject: Re: [PATCH] PCI: Unify ECAM constants in native PCI Express drivers
Date: Tue, 22 Sep 2020 18:27:15 -0500
Message-ID: <20200922232715.GA2238688@bjorn-Precision-5520> (raw)
In-Reply-To: <20200905204416.GA83847@rocinante>

[+cc Rob, who's doing a lot of cleanup in these drivers]

On Sat, Sep 05, 2020 at 10:44:16PM +0200, Krzysztof Wilczyński wrote:
> Hello Jonathan,
> 
> Thank you for the review!  Also, apologies for late reply.
> 
> On 20-08-28 10:08:43, Jonathan Cameron wrote:
> [...]
> > 
> > Might potentially be worth tidying up the masks as well?
> > Or potentially drop them given I suspect that there are no cases
> > in which the mask is actually doing anything...
> 
> Just to confirm - you have the following constants in mind?
> 
> drivers/pci/controller/pcie-rockchip.h:
> 
> #define PCIE_ECAM_BUS(x)	(((x) & 0xff) << 20)
> #define PCIE_ECAM_DEV(x)	(((x) & 0x1f) << 15)
> #define PCIE_ECAM_FUNC(x)	(((x) & 0x7) << 12)
> 
> drivers/pci/controller/dwc/pcie-al.c:
> 
> #define PCIE_ECAM_DEVFN(x)	(((x) & 0xff) << 12)
> 
> I can move PCIE_ECAM_BUS, PCIE_ECAM_DEV and PCIE_ECAM_FUNC (as
> PCIE_ECAM_FUN) to the linux/pci-ecam.h file, as these seem useful, but
> without the masks, and then update other files to use these.  We could
> then leverage these, for example:
> 
>  	pci_base_addr = (void __iomem *)((uintptr_t)pp->va_cfg0_base +
> -					 (busnr_ecam << 20) +
> -					 PCIE_ECAM_DEVFN(devfn));
> +					 PCIE_ECAM_BUS(busnr_ecam) +
> +					 PCIE_ECAM_FUN(devfn));
> 
> What do you think?  Bjorn, would that be acceptable?

It would be nice to use the same style and same macros for all of
the following, which are all really doing the same thing:

  al_pcie_conf_addr_map()
    pci_base_addr = (void __iomem *)((uintptr_t)pp->va_cfg0_base +
				     (busnr_ecam << 20) +
				     PCIE_ECAM_DEVFN(devfn));

  rockchip_pcie_rd_other_conf()
    busdev = PCIE_ECAM_ADDR(bus->number, PCI_SLOT(devfn),
			    PCI_FUNC(devfn), where);

  nwl_pcie_map_bus()
    relbus = (bus->number << ECAM_BUS_LOC_SHIFT) |
		    (devfn << ECAM_DEV_LOC_SHIFT);

    return pcie->ecam_base + relbus + where;

  xilinx_pcie_map_bus()
    relbus = (bus->number << ECAM_BUS_NUM_SHIFT) |
	     (devfn << ECAM_DEV_NUM_SHIFT);

    return port->reg_base + relbus + where;

Maybe that's something like using PCIE_ECAM_ADDR() everywhere?  I'm
not sure there's value in having the caller do the PCI_SLOT() and
PCI_FUNC() decomposition, though, i.e., maybe it's something like
this?

  #define PCIE_ECAM_REG(x)  ((x) & 0xfff)

  #define PCI_ECAM_OFFSET(bus, devfn, where) \
    PCIE_ECAM_BUS(bus->number) | \
    PCIE_ECAM_DEVFN(devfn) | \
    PCIE_ECAM_REG(where)

  reply index

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-27 22:49 Krzysztof Wilczyński
2020-08-28  9:08 ` Jonathan Cameron
2020-09-05 20:44   ` Krzysztof Wilczyński
2020-09-22 23:27     ` Bjorn Helgaas [this message]
2020-09-23 17:23       ` Rob Herring
2020-09-24 20:57         ` Krzysztof Wilczyński

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=20200922232715.GA2238688@bjorn-Precision-5520 \
    --to=helgaas@kernel.org \
    --cc=Jonathan.Cameron@Huawei.com \
    --cc=bhelgaas@google.com \
    --cc=heiko@sntech.de \
    --cc=jonnyc@amazon.com \
    --cc=kw@linux.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=michal.simek@xilinx.com \
    --cc=robh@kernel.org \
    --cc=rrichter@marvell.com \
    --cc=shawn.lin@rock-chips.com \
    --cc=wangzhou1@hisilicon.com \
    --cc=will@kernel.org \
    /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-PCI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/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-pci linux-pci/ https://lore.kernel.org/linux-pci \
		linux-pci@vger.kernel.org
	public-inbox-index linux-pci

Example config snippet for mirrors

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


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