From: Mitch Bradley <wmb@firmworks.com>
To: Thierry Reding <thierry.reding@avionic-design.de>
Cc: Stephen Warren <swarren@wwwdotorg.org>,
Russell King <linux@arm.linux.org.uk>,
linux-pci@vger.kernel.org, devicetree-discuss@lists.ozlabs.org,
Rob Herring <rob.herring@calxeda.com>,
Jesse Barnes <jbarnes@virtuousgeek.org>,
Colin Cross <ccross@android.com>,
linux-tegra@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2 07/10] ARM: tegra: pcie: Add device tree support
Date: Tue, 12 Jun 2012 21:28:51 -1000 [thread overview]
Message-ID: <4FD84133.4060401@firmworks.com> (raw)
In-Reply-To: <20120613064519.GD31001@avionic-0098.mockup.avionic-design.de>
On 6/12/2012 8:45 PM, Thierry Reding wrote:
> * Mitch Bradley wrote:
>> On 6/12/2012 10:15 AM, Stephen Warren wrote:
>>> On 06/12/2012 11:20 AM, Thierry Reding wrote:
>>> ...
>>>> I came up with the following alternative:
>>>>
>>>> pci {
>>>> compatible = "nvidia,tegra20-pcie";
>>>> reg =<0x80003000 0x00000800 /* PADS registers */
>>>> 0x80003800 0x00000200 /* AFI registers */
>>>> 0x80004000 0x00100000 /* configuration space */
>>>> 0x80104000 0x00100000 /* extended configuration space */
>>>> 0x80400000 0x00010000 /* downstream I/O */
>>>> 0x90000000 0x10000000 /* non-prefetchable memory */
>>>> 0xa0000000 0x10000000>; /* prefetchable memory */
>>>> interrupts =<0 98 0x04 /* controller interrupt */
>>>> 0 99 0x04>; /* MSI interrupt */
>>>> status = "disabled";
>>>>
>>>> ranges =<0x80000000 0x80000000 0x00002000 /* 2 root ports */
>>>> 0x80004000 0x80004000 0x00100000 /* configuration space */
>>>> 0x80104000 0x80104000 0x00100000 /* extended configuration space */
>>>> 0x80400000 0x80400000 0x00010000 /* downstream I/O */
>>>> 0x90000000 0x90000000 0x10000000 /* non-prefetchable memory */
>>>> 0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */
>>>>
>>>> #address-cells =<1>;
>>>> #size-cells =<1>;
>>>>
>>>> port@80000000 {
>>>> reg =<0x80000000 0x00001000>;
>>>> status = "disabled";
>>>> };
>>>>
>>>> port@80001000 {
>>>> reg =<0x80001000 0x00001000>;
>>>> status = "disabled";
>>>> };
>>>> };
>>>>
>>>> The "ranges" property can probably be cleaned up a bit, but the most
>>>> interesting part is the port@ children, which can simply be enabled in board
>>>> DTS files by setting the status property to "okay". I find that somewhat more
>>>> intuitive to the variant with an "enable-ports" property.
>>>>
>>>> What do you think of this?
>>>
>>> As a general concept, this kind of design seems OK to me.
>>>
>>> The "port" child nodes I think should be named "pci@..." given Mitch's
>>> comments, I think.
>>>
>>> The port nodes probably need two entries in reg, given the following in
>>> our downstream driver:
>>>
>>>> int rp_offset = 0;
>>>> int ctrl_offset = AFI_PEX0_CTRL;
>>> ...
>>>> for (port = 0; port< MAX_PCIE_SUPPORTED_PORTS; port++) {
>>>> ctrl_offset += (port * 8);
>>>> rp_offset = (rp_offset + 0x1000) * port;
>>>> if (tegra_pcie.plat_data->port_status[port])
>>>> tegra_pcie_add_port(port, rp_offset, ctrl_offset);
>>>> }
>>>
>>> (which actually looks likely to be horribly buggy for port>1 and only
>>> accidentally correct for port==1, but anyway...)
>>>
>>> But instead, I'd be tempted to make the top-level node say:
>>>
>>> #address-cells =<1>;
>>> #size-cells =<0>;
>>>
>>> ... so that the child nodes' reg is just the port ID. The parent node
>>> can calculate the addresses/offsets of the per-port registers within the
>>> PCIe controller's register space based on the ID using code roughly like
>>> what I quoted above:
>>>
>>> pci@0 {
>>> reg =<0>;
>>> status = "disabled";
>>> };
>>>
>>> pci@1 {
>>> reg =<0>;
>>> status = "disabled";
>>> };
>> reg =<1> ?
>>
>>>
>>>
>>> That would save having to put 2 entries in the reg, and perhaps remove
>>> the need for any ranges property.
>>
>> ISTM that having two reg entries, specifying the rp and ctrl
>> registers, is preferable to having code to calculate the addresses.
>> That makes the code simpler and the device tree more directly
>> descriptive of the hardware layout. The less "magic" (in this case,
>> the register address calculation), the better.
>
> The problem with this approach is that since the control registers are
> within the AFI register range, both the reg and ranges properties of the
> parent would have to include these holes. Furthermore it means that the
> controller driver would have to remap the AFI registers in chunks, for the
> sole reason of splitting out the controle registers.
>
> Would it be acceptable to make an exception in this case and use the port's
> second reg entry as an offset into the AFI register range instead?
How about this:
pcie-controller {
compatible = "nvidia,tegra20-pcie";
reg =<0x80003000 0x00000800 /* PADS registers */
0x80003800 0x00000200> /* AFI registers */
interrupts =<0 98 0x04 /* controller interrupt */
0 99 0x04>; /* MSI interrupt */
status = "disabled";
ranges =<0x80000000 0x80000000 0x00002000 /* 2 root ports */
0x80004000 0x80004000 0x00100000 /* configuration space */
0x80104000 0x80104000 0x00100000 /* extended
configuration space */
0x80400000 0x80400000 0x00010000 /* downstream I/O */
0x90000000 0x90000000 0x10000000 /* non-prefetchable
memory */
0xa0000000 0xa0000000 0x10000000>; /* prefetchable memory */
#address-cells =<1>;
#size-cells =<1>;
pci@80000000 {
reg =<0x80000000 0x00001000>;
ctrl-offset =<0x0>;
status = "disabled";
#address-cells = <3>; /* Standard for PCI */
#size-cells =<2>; /* Standard for PCI */
ranges =</* Map from standard PCI child address spaces to
linear spaces above */>;
};
pci@80001000 {
reg =<0x80001000 0x00001000>;
ctrl-offset =<0x8>;
status = "disabled";
#address-cells = <3>; /* Standard for PCI */
#size-cells =<2>; /* Standard for PCI */
ranges =</* Map from standard PCI child address spaces to
linear spaces above */>;
};
};
Using ctrl-offset instead of reg avoids any violation of the usual reg
semantics.
Note that the pci subnodes need the full complement of PCI bus node
properties.
>
>
> Thierry
next prev parent reply other threads:[~2012-06-13 7:29 UTC|newest]
Thread overview: 95+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-06-11 15:05 [PATCH v2 00/10] ARM: tegra: Add PCIe device tree support Thierry Reding
2012-06-11 15:05 ` [PATCH v2 01/10] PCI: Keep pci_fixup_irqs() around after init Thierry Reding
2012-06-11 15:05 ` [PATCH v2 02/10] ARM: pci: Keep pci_common_init() " Thierry Reding
2012-06-11 15:05 ` [PATCH v2 03/10] ARM: pci: Allow passing per-controller private data Thierry Reding
2012-06-11 15:05 ` [PATCH v2 04/10] ARM: tegra: Move tegra_pcie_xclk_clamp() to PMC Thierry Reding
2012-06-11 15:05 ` [PATCH v2 05/10] ARM: tegra: Rewrite PCIe support as a driver Thierry Reding
2012-06-11 21:09 ` Stephen Warren
2012-06-12 6:41 ` Thierry Reding
2012-06-12 7:24 ` Thierry Reding
2012-06-12 16:00 ` Stephen Warren
2012-06-13 8:12 ` Thierry Reding
2012-06-11 21:22 ` Stephen Warren
2012-06-12 4:59 ` Thierry Reding
2012-06-11 15:05 ` [PATCH v2 06/10] ARM: tegra: pcie: Add MSI support Thierry Reding
2012-06-11 21:19 ` Stephen Warren
2012-06-12 5:07 ` Thierry Reding
2012-06-12 5:33 ` Stephen Warren
2012-06-12 5:41 ` Thierry Reding
2012-06-12 6:10 ` Thierry Reding
2012-06-12 15:40 ` Stephen Warren
2012-06-12 17:23 ` Thierry Reding
2012-06-11 15:05 ` [PATCH v2 07/10] ARM: tegra: pcie: Add device tree support Thierry Reding
2012-06-11 21:33 ` Stephen Warren
2012-06-12 6:21 ` Thierry Reding
2012-06-12 15:44 ` Stephen Warren
2012-06-12 17:20 ` Thierry Reding
[not found] ` <4FD7943E.60302@firmworks.com>
2012-06-12 19:46 ` Stephen Warren
2012-06-12 19:52 ` Mitch Bradley
2012-06-13 5:54 ` Thierry Reding
2012-06-13 7:04 ` Mitch Bradley
2012-06-12 20:15 ` Stephen Warren
2012-06-12 21:11 ` Mitch Bradley
2012-06-13 6:45 ` Thierry Reding
2012-06-13 7:28 ` Mitch Bradley [this message]
2012-06-13 7:52 ` Thierry Reding
2012-06-13 8:05 ` Mitch Bradley
2012-06-13 8:19 ` Thierry Reding
2012-06-13 8:36 ` Mitch Bradley
2012-06-13 8:42 ` Thierry Reding
2012-06-14 9:19 ` Thierry Reding
2012-06-14 18:30 ` Stephen Warren
2012-06-14 19:29 ` Thierry Reding
2012-06-14 19:50 ` Stephen Warren
2012-06-15 6:12 ` Thierry Reding
2012-06-19 13:30 ` Thierry Reding
2012-06-19 16:40 ` Stephen Warren
2012-06-19 21:31 ` Mitch Bradley
2012-06-20 16:32 ` Stephen Warren
2012-06-20 17:41 ` Mitch Bradley
2012-06-20 17:47 ` Stephen Warren
2012-06-20 19:57 ` Arnd Bergmann
2012-06-20 20:19 ` Mitch Bradley
2012-06-21 6:47 ` Thierry Reding
2012-06-22 10:18 ` Bjorn Helgaas
2012-06-22 11:00 ` Thierry Reding
2012-06-22 11:46 ` Bjorn Helgaas
2012-06-22 12:43 ` Thierry Reding
2012-06-22 13:03 ` Arnd Bergmann
2012-06-22 16:49 ` Bjorn Helgaas
2012-06-22 16:53 ` Arnd Bergmann
2012-06-22 17:13 ` Bjorn Helgaas
2012-06-22 21:08 ` Arnd Bergmann
2012-06-22 17:14 ` Arnd Bergmann
2012-06-22 17:00 ` Stephen Warren
2012-06-22 17:28 ` Stephen Warren
2012-06-23 21:35 ` Bjorn Helgaas
2012-06-25 6:34 ` Thierry Reding
2012-06-26 17:22 ` Stephen Warren
2012-06-27 6:19 ` Thierry Reding
2012-06-22 16:20 ` Stephen Warren
2012-06-22 17:09 ` Mitch Bradley
2012-06-22 11:04 ` Thierry Reding
2012-06-22 13:22 ` Thierry Reding
2012-06-22 13:48 ` Arnd Bergmann
2012-06-22 14:02 ` Thierry Reding
2012-06-22 16:40 ` Arnd Bergmann
2012-06-13 20:21 ` Arnd Bergmann
2012-06-14 8:37 ` Thierry Reding
2012-06-14 10:25 ` Arnd Bergmann
2012-06-14 10:31 ` Thierry Reding
2012-06-14 11:06 ` Arnd Bergmann
2012-06-14 11:58 ` Thierry Reding
2012-06-13 6:34 ` Thierry Reding
2012-06-13 16:20 ` Stephen Warren
2012-06-13 17:03 ` Stephen Warren
2012-06-11 15:05 ` [PATCH v2 08/10] ARM: tegra: harmony: Initialize regulators from DT Thierry Reding
2012-06-11 21:36 ` Stephen Warren
2012-06-12 6:13 ` Thierry Reding
2012-06-21 20:17 ` Stephen Warren
2012-06-22 6:06 ` Thierry Reding
2012-06-11 15:05 ` [PATCH v2 09/10] ARM: tegra: harmony: Initialize PCIe " Thierry Reding
2012-06-11 21:41 ` Stephen Warren
2012-06-12 5:48 ` Thierry Reding
2012-06-12 15:38 ` Stephen Warren
2012-06-11 15:05 ` [PATCH v2 10/10] ARM: tegra: trimslice: " 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=4FD84133.4060401@firmworks.com \
--to=wmb@firmworks.com \
--cc=ccross@android.com \
--cc=devicetree-discuss@lists.ozlabs.org \
--cc=jbarnes@virtuousgeek.org \
--cc=linux-arm-kernel@lists.infradead.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=thierry.reding@avionic-design.de \
/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).