linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Thierry Reding <thierry.reding@avionic-design.de>
To: Stephen Warren <swarren@wwwdotorg.org>
Cc: Grant Likely <grant.likely@secretlab.ca>,
	Rob Herring <rob.herring@calxeda.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Russell King <linux@arm.linux.org.uk>,
	Andrew Murray <andrew.murray@arm.com>,
	Jason Gunthorpe <jgunthorpe@obsidianresearch.com>,
	Arnd Bergmann <arnd@arndb.de>,
	Thomas Petazzoni <thomas.petazzoni@free-electrons.com>,
	devicetree-discuss@lists.ozlabs.org,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-tegra@vger.kernel.org, linux-pci@vger.kernel.org
Subject: Re: [PATCH v3 07/12] PCI: tegra: Move PCIe driver to drivers/pci/host
Date: Fri, 5 Apr 2013 09:37:03 +0200	[thread overview]
Message-ID: <20130405073703.GB15848@avionic-0098.mockup.avionic-design.de> (raw)
In-Reply-To: <515DF096.2000703@wwwdotorg.org>

[-- Attachment #1: Type: text/plain, Size: 7746 bytes --]

On Thu, Apr 04, 2013 at 03:28:54PM -0600, Stephen Warren wrote:
[...]
> > diff --git a/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt b/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt
> 
> > +Required properties:
> 
> > +- interrupts: the interrupt outputs of the controller
> > +- interrupt-names: list of names to identify interrupts
> 
> The specification part of this file should define which interrupt
> outputs must be included in this list, and the order they must appear in
> the list.

Okay, I can add such a description similar to what you propose below for
the clocks and clock-names properties. Something like this:

- interrupts: A list of interrupt outputs of the controller. Must contain an
  entry for each entry in the interrupt-names property.
- interrupt-names: Must include the following entries:
  "intr": The Tegra interrupt that is asserted for controller interrupts
  "msi": The Tegra interrupt that is asserted when an MSI is received

Would that be acceptable?

I should probably update the reg properties in a similar way. Maybe like
so:

- reg: A list of physical base address and length for each set of controller
  registers. Must contain an entry for each entry in the reg-names property.
- reg-names: Must include the following entries:
  "pads": PADS registers
  "afi": AFI registers
  "cs": configuration space region

> I believe that the entries in the interrupts property must
> have a defined order, so I'm not sure whether interrupt-names is useful
> here?

Actually the interrupt-names property is there specifically so that the
order doesn't matter. The driver uses platform_get_irq_byname(), using
"intr" and "msi" respectively so that they don't have to appear in a
specific order. I did this so that it is more in line with properties
such as clocks and reg.

> > +- ranges: describes the translation of addresses for root ports
> 
> Shouldn't there be some discussion re: how the range are expected to be
> set up so that everything works? We shouldn't expect people to just
> blindly copy the example without any way of understanding it.

Possibly. I wouldn't like to go too much into the details, though, since
the ranges property is not only rather complex but also well documented
in other places. But I could add some explanation about which entries
are expected and how they work together. In this case even order is
important. The port register entries need to be listed before the
non-prefetchable memory window entry, otherwise the ranges parser gets
confused. How does the following sound?

- ranges: Describes the translation of addresses for root ports and standard
  PCI regions. The entries must be 6 cells each, where the first three cells
  correspond to the address as described for the #address-cells property
  above, the fourth cell is the physical CPU address to translate to and the
  fifth and six cells are as described for the #size-cells property above.
  - The first two entries are expected to translate the addresses for the root
    port registers, which are referenced by the assigned-addresses property of
    the root port nodes (see below).
  - The remaining entries setup the mapping for the standard I/O, memory and
    prefetchable PCI regions. The first cell determines the type of region
    that is setup:
    - 0x81000000: I/O memory region
    - 0x82000000: non-prefetchable memory region
    - 0xc2000000: prefetchable memory region
  Please refer to the standard PCI bus binding document for a more detailed
  explanation.

> > +- clocks: the clock inputs of the controller
> > +- clock-names: list of names to identify clocks
> 
> The specification part of this file should define which clocks are
> required, and not rely on the example below to do this. I would suggest
> re-writing this as:
> 
> - clocks : Must contain an entry for each entry in clock-names.
> - clock-names : Must include the following entries:
>   "pex" (The Tegra clock of that name)
>   "afi" (The Tegra clock of that name)
>   "pcie_xclk" (The Tegra clock of that name)
>   "pll_e" (The Tegra clock of that name)

Okay, sounds good. Although the Tegra clocks are named somewhat
differently. "pex" is named "pcie" and "pcie_xclk" is "unassigned" (!)
according to the nvidia,tegra20-car.txt binding document. Perhaps I
should update the binding document to replace unassigned with pcie_xclk
for clock 74. And maybe rename pex in the PCIe binding to pcie to match
the CAR binding?

> > +Root ports are defined as subnodes of the PCIe controller node.
> > +
> > +Required properties:
> ...
> > +- ranges: sub-ranges distributed from the PCIe controller node
> 
> Here, I think it's worth mentioning that an empty ranges is all that's
> required.

Yes, that's a good idea.

> > +Board DTS:
> > +
> > +	pcie-controller {
> > +		status = "okay";
> > +
> > +		vdd-supply = <&pci_vdd_reg>;
> > +		pex-clk-supply = <&pci_clk_reg>;
> > +
> > +		/* root port 00:01.0 */
> > +		pci@1,0 {
> > +			status = "okay";
> 
> Is it worth putting a comment here stating that the explicit devices
> included below in this example are entirely optional?

Would it be acceptable to annotate the 01:00.0 bridge with "optional"?
Like so:

	pcie-controller {
		...
		pci@1,0 {
			...
			/* bridge 01:00.0 (optional) */
			pci@0,0 {
				...
			};
		};
	};

Alternatively I could add something like below:

---snip---

Note that devices on the PCI bus are dynamically discovered using PCI's bus
enumeration and therefore don't need corresponding device nodes in DT. However
if a device on the PCI bus provides a non-probeable bus such as I2C or SPI,
device nodes need to be added in order to allow the bus' children to be
instantiated at the proper location in the operating system's device tree (as
illustrated by the optional nodes in the example above).

---snip---

Maybe I'll just do both.

> > diff --git a/arch/arm/mach-tegra/board-harmony-pcie.c b/arch/arm/mach-tegra/board-harmony-pcie.c
> > deleted file mode 100644
> 
> Hmmm. Is this patch meant to include a change to tegra20-harmony.dtsi to
> hook up all the regulators through device tree? Same for TrimSlice?

As discussed above I think it'd be preferrable, in order to keep
dependencies simpler, to separate the changes into different patches.
Aside from the oddity on Harmony I don't think there's anything that
prevents that.

> > diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> > diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> > diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> > diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> 
> I think the creation of those files should be a separate patch, and
> hopefully get into 3.10 to remove any dependencies. Otherwise, 3 or 4
> different patches are all going to try and do the same thing. Didn't the
> Marvell series split out the creation of drivers/pci/host/ into a
> separate patch that's suitable for this, and could go into 3.10?

At the time when I wrote the patch to move the driver to drivers/pci,
there were no other drivers so I didn't think it necessary to split out
those changes. I had also been overly optimistic that the patches could
be merged at that point. Meanwhile there are a few more drivers that
will go into that directory, so yes they should go into a separate patch
like Thomas has done. If that makes it into 3.10, all the better.

> > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> 
> I'll review that file separately later.

Okay, thanks for reviewing.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

  parent reply	other threads:[~2013-04-05  7:37 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-04-03 14:45 [PATCH v3 00/12] Rewrite Tegra PCIe driver Thierry Reding
2013-04-03 14:45 ` [PATCH v3 01/12] of/pci: Provide support for parsing PCI DT ranges property Thierry Reding
2013-04-03 14:45 ` [PATCH v3 02/12] of/pci: Add of_pci_get_devfn() function Thierry Reding
2013-04-03 14:45 ` [PATCH v3 03/12] of/pci: Add of_pci_parse_bus_range() function Thierry Reding
2013-04-03 14:45 ` [PATCH v3 04/12] PCI: Introduce new MSI chip infrastructure Thierry Reding
2013-04-03 14:45 ` [PATCH v3 05/12] ARM: tegra: Move tegra_pcie_xclk_clamp() to PMC Thierry Reding
2013-04-03 14:45 ` [PATCH v3 06/12] ARM: tegra: Move pmc.h to include/linux/tegra-pmc.h Thierry Reding
2013-04-03 14:45 ` [PATCH v3 07/12] PCI: tegra: Move PCIe driver to drivers/pci/host Thierry Reding
2013-04-04 21:28   ` Stephen Warren
2013-04-04 21:30     ` Stephen Warren
2013-04-05  6:03       ` Thierry Reding
2013-04-10 22:46         ` Stephen Warren
2013-04-05  7:37     ` Thierry Reding [this message]
2013-04-05  7:53       ` Thierry Reding
2013-04-10 23:05       ` Stephen Warren
2013-04-15 18:28   ` Stephen Warren
2013-06-12 12:30     ` Thierry Reding
2013-06-12 16:09       ` Stephen Warren
2013-06-12 21:23         ` Thierry Reding
2013-04-03 14:45 ` [PATCH v3 08/12] ARM: tegra: tamonten: Add PCIe support Thierry Reding
2013-04-03 14:45 ` [PATCH v3 09/12] ARM: tegra: tec: " Thierry Reding
2013-04-03 14:45 ` [PATCH v3 10/12] ARM: tegra: harmony: Initialize PCIe from DT Thierry Reding
2013-04-03 14:45 ` [PATCH v3 11/12] ARM: tegra: trimslice: " Thierry Reding
2013-04-03 14:45 ` [PATCH v3 12/12] ARM: tegra: Update default configuration (PCIe) Thierry Reding

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=20130405073703.GB15848@avionic-0098.mockup.avionic-design.de \
    --to=thierry.reding@avionic-design.de \
    --cc=andrew.murray@arm.com \
    --cc=arnd@arndb.de \
    --cc=bhelgaas@google.com \
    --cc=devicetree-discuss@lists.ozlabs.org \
    --cc=grant.likely@secretlab.ca \
    --cc=jgunthorpe@obsidianresearch.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=linux@arm.linux.org.uk \
    --cc=rob.herring@calxeda.com \
    --cc=swarren@wwwdotorg.org \
    --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).