From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
To: Cyrille Pitchen <cyrille.pitchen@free-electrons.com>
Cc: bhelgaas@google.com, kishon@ti.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: Thu, 30 Nov 2017 10:06:22 +0000 [thread overview]
Message-ID: <20171130100622.GA10349@red-moon> (raw)
In-Reply-To: <20171129182514.GA1087@red-moon>
On Wed, Nov 29, 2017 at 06:25:15PM +0000, Lorenzo Pieralisi wrote:
[...]
> static void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn,
> int where)
>
> > +cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn, int where)
> > +{
> > + struct pci_host_bridge *bridge = pci_find_host_bridge(bus);
> > + struct cdns_pcie_rc *rc = pci_host_bridge_priv(bridge);
> > + struct cdns_pcie *pcie = &rc->pcie;
> > + unsigned int busn = bus->number;
> > + u32 addr0, desc0;
> > +
> > + if (busn < rc->bus_range->start || busn > rc->bus_range->end)
> > + return NULL;
>
> It does not hurt but I wonder whether you really need this check.
>
> > + if (busn == rc->bus_range->start) {
> > + if (devfn)
>
> I suspect I know why you need this check but I ask you to explain it
> anyway if you do not mind please.
>
> > + return NULL;
> > +
> > + return pcie->reg_base + (where & 0xfff);
> > + }
> > +
> > + /* Update Output registers for AXI region 0. */
> > + addr0 = CDNS_PCIE_AT_OB_REGION_PCI_ADDR0_NBITS(12) |
>
> Ok, so for every config access you reprogram addr0 to reflect the
> correct bus|devfn ID in the PCI bus TLP corresponding to an ECAM address
> in CPU physical address space, is my understanding correct ?
By re-reading it, it looks like this mechanism is there to just
associate a different RID TLP on the PCI bus to a fixed window
in CPU virtual address space.
> > + CDNS_PCIE_AT_OB_REGION_PCI_ADDR0_DEVFN(devfn) |
> > + CDNS_PCIE_AT_OB_REGION_PCI_ADDR0_BUS(busn);
> > + cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_PCI_ADDR0(0), addr0);
> > +
> > + /* Configuration Type 0 or Type 1 access. */
> > + desc0 = CDNS_PCIE_AT_OB_REGION_DESC0_HARDCODED_RID |
> > + CDNS_PCIE_AT_OB_REGION_DESC0_DEVFN(0);
> > + /*
> > + * The bus number was already set once for all in desc1 by
> > + * cdns_pcie_host_init_address_translation().
> > + */
> > + if (busn == rc->bus_range->start + 1)
> > + desc0 |= CDNS_PCIE_AT_OB_REGION_DESC0_TYPE_CONF_TYPE0;
> > + else
> > + desc0 |= CDNS_PCIE_AT_OB_REGION_DESC0_TYPE_CONF_TYPE1;
>
> I would like to ask you why you have to do it here and the root port
> does not figure it out by itself, I do not have the datasheet so I am
> just asking for my own information.
>
> > + cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_DESC0(0), desc0);
> > +
> > + return rc->cfg_base + (where & 0xfff);
> > +}
[...]
> > +static int cdns_pcie_host_init(struct device *dev,
> > + struct list_head *resources,
> > + struct cdns_pcie_rc *rc)
> > +{
> > + struct resource *bus_range = NULL;
> > + int err;
> > +
> > + /* Parse our PCI ranges and request their resources */
> > + err = cdns_pcie_parse_request_of_pci_ranges(dev, resources, &bus_range);
> > + if (err)
> > + goto err_out;
>
> I think that the err_out path should be part of:
>
> cdns_pcie_parse_request_of_pci_ranges()
>
> implementation and here you would just return.
I take it back, what you are doing is cleaner and allows you to
have code freeing the resource list in one single place so you
can leave this as-is.
Thanks,
Lorenzo
next prev parent reply other threads:[~2017-11-30 10:05 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 1/5] PCI: Add vendor ID for Cadence Cyrille Pitchen
2017-12-06 21:27 ` Bjorn Helgaas
2017-11-23 15:01 ` [PATCH 2/5] dt-bindings: PCI: cadence: Add DT bindings for Cadence PCIe host controller Cyrille Pitchen
2017-11-26 19:32 ` Rob Herring
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
2017-11-28 20:46 ` Bjorn Helgaas
2017-11-29 8:19 ` Thomas Petazzoni
2017-11-29 15:55 ` Bjorn Helgaas
2017-11-29 14:14 ` Lorenzo Pieralisi
2017-12-01 10:37 ` Cyrille Pitchen
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
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 [this message]
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
2017-12-13 16:03 ` Cyrille Pitchen
2017-12-13 16:50 ` Cyrille Pitchen
2017-12-14 17:03 ` Cyrille Pitchen
2017-12-15 5:49 ` Kishon Vijay Abraham I
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
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=20171130100622.GA10349@red-moon \
--to=lorenzo.pieralisi@arm.com \
--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=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).