From: Bjorn Helgaas <helgaas@kernel.org>
To: Cyrille Pitchen <cyrille.pitchen@free-electrons.com>
Cc: bhelgaas@google.com, kishon@ti.com, lorenzo.pieralisi@arm.com,
linux-pci@vger.kernel.org, adouglas@cadence.com,
stelford@cadence.com, dgary@cadence.com, kgopi@cadence.com,
eandrews@cadence.com, thomas.petazzoni@free-electrons.com,
sureshp@cadence.com, nsekhar@ti.com,
linux-kernel@vger.kernel.org, robh@kernel.org,
devicetree@vger.kernel.org
Subject: Re: [PATCH 3/5] PCI: cadence: Add host driver for Cadence PCIe controller
Date: Tue, 28 Nov 2017 14:41:14 -0600 [thread overview]
Message-ID: <20171128204114.GE11228@bhelgaas-glaptop.roam.corp.google.com> (raw)
In-Reply-To: <2670f7ddf59e708beb9d32bef1353e15bd4e1ecf.1511439189.git.cyrille.pitchen@free-electrons.com>
On Thu, Nov 23, 2017 at 04:01:48PM +0100, Cyrille Pitchen wrote:
> This patch adds support to the Cadence PCIe controller in host mode.
>
> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@free-electrons.com>
> ---
> drivers/Makefile | 1 +
> drivers/pci/Kconfig | 1 +
> drivers/pci/cadence/Kconfig | 24 ++
> drivers/pci/cadence/Makefile | 2 +
> drivers/pci/cadence/pcie-cadence-host.c | 425 ++++++++++++++++++++++++++++++++
> drivers/pci/cadence/pcie-cadence.c | 110 +++++++++
> drivers/pci/cadence/pcie-cadence.h | 325 ++++++++++++++++++++++++
> 7 files changed, 888 insertions(+)
> create mode 100644 drivers/pci/cadence/Kconfig
> create mode 100644 drivers/pci/cadence/Makefile
> create mode 100644 drivers/pci/cadence/pcie-cadence-host.c
> create mode 100644 drivers/pci/cadence/pcie-cadence.c
> create mode 100644 drivers/pci/cadence/pcie-cadence.h
I prefer a single file per driver. I assume you're anticipating
something like dwc, where the DesignWare core is incorporated into
several devices in slightly different ways. But it doesn't look like
that's here yet, and personally, I'd rather split out the required
things when they actually become required, not ahead of time.
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 1d034b680431..27bdd98784d9 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -18,6 +18,7 @@ obj-y += pwm/
>
> obj-$(CONFIG_PCI) += pci/
> obj-$(CONFIG_PCI_ENDPOINT) += pci/endpoint/
> +obj-$(CONFIG_PCI_CADENCE) += pci/cadence/
I can't remember why we added CONFIG_PCI_ENDPOINT here instead of in
drivers/pci/Makefile. Is there any way to move both CONFIG_PCI_ENDPOINT
and CONFIG_PCI_CADENCE into drivers/pci/Makefile so this is better
encapsulated?
> # PCI dwc controller drivers
> obj-y += pci/dwc/
>...
> + * struct cdns_pcie_rc_data - hardware specific data
"cdns" is a weird abbreviation for "Cadence", since "Cadence" doesn't
contain an "s".
>...
> +static int cdns_pcie_parse_request_of_pci_ranges(struct device *dev,
> + struct list_head *resources,
> + struct resource **bus_range)
> +{
> + int err, res_valid = 0;
> + struct device_node *np = dev->of_node;
> + resource_size_t iobase;
> + struct resource_entry *win, *tmp;
> +
> + err = of_pci_get_host_bridge_resources(np, 0, 0xff, resources, &iobase);
> + if (err)
> + return err;
> +
> + err = devm_request_pci_bus_resources(dev, resources);
> + if (err)
> + return err;
> +
> + resource_list_for_each_entry_safe(win, tmp, resources) {
> + struct resource *res = win->res;
> +
> + switch (resource_type(res)) {
> + case IORESOURCE_IO:
> + err = pci_remap_iospace(res, iobase);
> + if (err) {
> + dev_warn(dev, "error %d: failed to map resource %pR\n",
> + err, res);
> + resource_list_destroy_entry(win);
> + }
> + break;
> + case IORESOURCE_MEM:
> + res_valid |= !(res->flags & IORESOURCE_PREFETCH);
> + break;
> + case IORESOURCE_BUS:
> + *bus_range = res;
> + break;
> + }
> + }
> +
> + if (res_valid)
> + return 0;
> +
> + dev_err(dev, "non-prefetchable memory resource required\n");
> + return -EINVAL;
> +}
The code above is starting to look awfully familiar. I wonder if it's
time to think about some PCI-internal interface that can encapsulate
this. In this case, there's really nothing Cadence-specific here.
There are other callers where there *is* vendor-specific code, but
possibly that could be handled by returning pointers to bus number,
I/O port, and MMIO resources so the caller could do the
vendor-specific stuff?
Bjorn
next prev parent reply other threads:[~2017-11-28 20:41 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-23 15:01 [PATCH 0/5] PCI: Add support to the Cadence PCIe controller Cyrille Pitchen
2017-11-23 15:01 ` [PATCH 2/5] dt-bindings: PCI: cadence: Add DT bindings for Cadence PCIe host controller Cyrille Pitchen
[not found] ` <895cbb8f862c712b78f780a53e05d5429d24cc35.1511439189.git.cyrille.pitchen-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
2017-11-26 19:32 ` Rob Herring
[not found] ` <cover.1511439189.git.cyrille.pitchen-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
2017-11-23 15:01 ` [PATCH 1/5] PCI: Add vendor ID for Cadence Cyrille Pitchen
[not found] ` <a93032cbbe02dc9efef8536a4b2a851d1b08ab6f.1511439189.git.cyrille.pitchen-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
2017-12-06 21:27 ` Bjorn Helgaas
2017-11-23 15:01 ` [PATCH 3/5] PCI: cadence: Add host driver for Cadence PCIe controller Cyrille Pitchen
2017-11-28 20:41 ` Bjorn Helgaas [this message]
2017-11-28 20:46 ` Bjorn Helgaas
2017-11-29 14:14 ` Lorenzo Pieralisi
[not found] ` <20171128204114.GE11228-1RhO1Y9PlrlHTL0Zs8A6p5iNqAH0jzoTYJqu5kTmcBRl57MIdRCFDg@public.gmane.org>
2017-11-29 8:19 ` Thomas Petazzoni
2017-11-29 15:55 ` Bjorn Helgaas
2017-12-01 10:37 ` Cyrille Pitchen
[not found] ` <c6a0d70d-d584-2db8-f862-0c3ddbf6939d-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
2017-12-01 16:20 ` Lorenzo Pieralisi
2017-11-29 17:34 ` Lorenzo Pieralisi
2017-12-03 20:44 ` Cyrille Pitchen
2017-12-04 18:20 ` Lorenzo Pieralisi
2017-12-04 18:49 ` Ard Biesheuvel
[not found] ` <CAKv+Gu_1XZmsKJ_7ay7D74xSAhDW0y++7-CC3YfG7LOUcNZSqA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-12-06 11:32 ` Lorenzo Pieralisi
2017-12-13 16:42 ` Cyrille Pitchen
2017-11-29 18:25 ` Lorenzo Pieralisi
2017-11-30 10:06 ` Lorenzo Pieralisi
2017-11-23 15:01 ` [PATCH 4/5] dt-bindings: PCI: cadence: Add DT bindings for Cadence PCIe endpoint controller Cyrille Pitchen
2017-11-26 19:33 ` Rob Herring
2017-11-23 15:01 ` [PATCH 5/5] PCI: cadence: add EndPoint Controller driver for Cadence PCIe controller Cyrille Pitchen
2017-12-01 12:20 ` Lorenzo Pieralisi
2017-12-04 14:56 ` Cyrille Pitchen
2017-12-05 9:19 ` Kishon Vijay Abraham I
2017-12-07 10:05 ` Philippe Ombredanne
[not found] ` <CAOFm3uGQzAOrAmQLx7uJ0SJGQxMSB+oAniq9wd93tvupKWv=uA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-12-13 16:03 ` Cyrille Pitchen
[not found] ` <ed6d811e-608e-e6aa-7b07-2f2d2d68adbd-l0cyMroinI0@public.gmane.org>
2017-12-13 16:50 ` Cyrille Pitchen
2017-12-14 17:03 ` Cyrille Pitchen
[not found] ` <e43929c8-e283-f774-eca3-3b0ffcdfb49d-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
2017-12-15 5:49 ` Kishon Vijay Abraham I
[not found] ` <4c1e8ba7-8084-7802-df4e-47c6f3ed7816-l0cyMroinI0@public.gmane.org>
2017-12-15 11:49 ` Cyrille Pitchen
2017-11-28 15:50 ` [PATCH 0/5] PCI: Add support to the " Lorenzo Pieralisi
2017-11-30 7:13 ` Kishon Vijay Abraham I
2017-11-30 18:18 ` Lorenzo Pieralisi
2017-11-30 18:45 ` Cyrille Pitchen
[not found] ` <a02af401-2eb8-dc34-6f3c-092f04ec2636-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
2017-11-30 20:05 ` Cyrille Pitchen
2017-11-30 23:05 ` Bjorn Helgaas
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=20171128204114.GE11228@bhelgaas-glaptop.roam.corp.google.com \
--to=helgaas@kernel.org \
--cc=adouglas@cadence.com \
--cc=bhelgaas@google.com \
--cc=cyrille.pitchen@free-electrons.com \
--cc=devicetree@vger.kernel.org \
--cc=dgary@cadence.com \
--cc=eandrews@cadence.com \
--cc=kgopi@cadence.com \
--cc=kishon@ti.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=nsekhar@ti.com \
--cc=robh@kernel.org \
--cc=stelford@cadence.com \
--cc=sureshp@cadence.com \
--cc=thomas.petazzoni@free-electrons.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).