Re: [PATCH v3 07/12] PCI: tegra: Move PCIe driver to drivers/pci/host
On Wed, Jun 12, 2013 at 10:09:10AM -0600, Stephen Warren wrote: > On 06/12/2013 06:30 AM, Thierry Reding wrote: > > On Mon, Apr 15, 2013 at 12:28:12PM -0600, Stephen Warren wrote: > >> On 04/03/2013 08:45 AM, Thierry Reding wrote: > >>> Move the PCIe driver from arch/arm/mach-tegra into the > >>> drivers/pci/host directory. The motivation is to collect > >>> various host controller drivers in the same location in order > >>> to facilitate refactoring. > >>> > >>> The Tegra PCIe driver has been largely rewritten, both in order > >>> to turn it into a proper platform driver and to add MSI (based > >>> on code by Krishna Kishore ) as well as > >>> device tree support. > ... > >>> +static irqreturn_t tegra_pcie_msi_irq(int irq, void *data) > >> ... > >>> + return IRQ_HANDLED; > >> > >> Shouldn't this function return IRQ_NONE if no MSI status bits > >> were found set? > > > > The IRQ isn't marked IRQF_SHARED, so I don't think this is needed. > > Isn't it still useful to detect unexpected/stuck interrupts? Yes, you're right. I can't think of a nicer way to do it, though, so I'll go with a processed IRQ counter or a flag. > >>> +static int tegra_pcie_probe(struct platform_device *pdev) > >> ... > >>> + pcibios_min_mem = 0; > >> > >> What does that mean/do? I wonder if that should be set to > >> 0x8000 by the Tegra30 patches? > > > > ARM defines PCIBIOS_MIN_MEM to that variable. That macro in turn is > > only used by pci_bus_alloc_resource() AFAICT, which uses it to > > override the start of a resource when allocating if res->start == > > 0. As such it designates a lower-bound of valid PCI memory > > addresses, so 0 on Tegra20 and 0x8000 on Tegra30 don't seem > > like good values. Maybe we need to set them to the lowest of the > > prefetchable and non-prefetchable memory areas as defined in the > > DT? > > > > It doesn't currently seem to matter at all, though, since we never > > pass in a range that's 0, so the start address of resources can > > never be 0 and therefore PCIBIOS_MIN_MEM is never used. > > Hmmm. I guess ignore it then. If the value won't ever be used, 0 is as > good a value as any? Alright, I'll do that then. Thierry pgpvvek3UjHLa.pgp Description: PGP signature
Re: [PATCH v3 07/12] PCI: tegra: Move PCIe driver to drivers/pci/host
On 06/12/2013 06:30 AM, Thierry Reding wrote: > On Mon, Apr 15, 2013 at 12:28:12PM -0600, Stephen Warren wrote: >> On 04/03/2013 08:45 AM, Thierry Reding wrote: >>> Move the PCIe driver from arch/arm/mach-tegra into the >>> drivers/pci/host directory. The motivation is to collect >>> various host controller drivers in the same location in order >>> to facilitate refactoring. >>> >>> The Tegra PCIe driver has been largely rewritten, both in order >>> to turn it into a proper platform driver and to add MSI (based >>> on code by Krishna Kishore ) as well as >>> device tree support. ... >>> +static irqreturn_t tegra_pcie_msi_irq(int irq, void *data) >> ... >>> + return IRQ_HANDLED; >> >> Shouldn't this function return IRQ_NONE if no MSI status bits >> were found set? > > The IRQ isn't marked IRQF_SHARED, so I don't think this is needed. Isn't it still useful to detect unexpected/stuck interrupts? >>> +static int tegra_pcie_enable(struct tegra_pcie *pcie) ... >> >> Why is that needed; when would a port get enabled if it was >> already enabled, and if it was already enabled, wouldn't you want >> this function to be a no-op rather than destroying everything and >> starting again? > > I'm not sure I understand what you're saying. The intent of this > function is to enable a port, check that there's a link and disable > the port otherwise. That way we don't keep ports around where > nothing is connected. Hmm. I have no idea either now:-( The body of tegra_pcie_enable() looks fine. >>> +static int tegra_pcie_probe(struct platform_device *pdev) >> ... >>> + pcibios_min_mem = 0; >> >> What does that mean/do? I wonder if that should be set to >> 0x8000 by the Tegra30 patches? > > ARM defines PCIBIOS_MIN_MEM to that variable. That macro in turn is > only used by pci_bus_alloc_resource() AFAICT, which uses it to > override the start of a resource when allocating if res->start == > 0. As such it designates a lower-bound of valid PCI memory > addresses, so 0 on Tegra20 and 0x8000 on Tegra30 don't seem > like good values. Maybe we need to set them to the lowest of the > prefetchable and non-prefetchable memory areas as defined in the > DT? > > It doesn't currently seem to matter at all, though, since we never > pass in a range that's 0, so the start address of resources can > never be 0 and therefore PCIBIOS_MIN_MEM is never used. Hmmm. I guess ignore it then. If the value won't ever be used, 0 is as good a value as any? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 07/12] PCI: tegra: Move PCIe driver to drivers/pci/host
On Mon, Apr 15, 2013 at 12:28:12PM -0600, Stephen Warren wrote:
> On 04/03/2013 08:45 AM, Thierry Reding wrote:
> > Move the PCIe driver from arch/arm/mach-tegra into the drivers/pci/host
> > directory. The motivation is to collect various host controller drivers
> > in the same location in order to facilitate refactoring.
> >
> > The Tegra PCIe driver has been largely rewritten, both in order to turn
> > it into a proper platform driver and to add MSI (based on code by
> > Krishna Kishore ) as well as device tree support.
>
> > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
>
> > +struct tegra_msi {
> > + DECLARE_BITMAP(used, INT_PCI_MSI_NR);
> > + struct irq_domain *domain;
> > + struct msi_chip chip;
>
> Nit: You could move that to be the first field in the struct, then
> to_tegra_msi() would end up being a no-op, which might save a byte or two.
Done.
> > +static int tegra_pcie_write_conf(struct pci_bus *bus, unsigned int devfn,
> > +int where, int size, u32 value)
> ...
> > + if (bus->number == 0) {
> > + unsigned int slot = PCI_SLOT(devfn);
> > + struct tegra_pcie_port *port;
> > +
> > + list_for_each_entry(port, &pcie->ports, list) {
> > + if (port->index + 1 == slot) {
> > + addr = port->base + (where & ~3);
> > + break;
> > + }
> > + }
> > + } else {
> > + addr = tegra_pcie_bus_map(pcie, bus->number);
> > + if (!addr) {
> > + dev_err(pcie->dev,
> > + "failed to map cfg. space for bus %u\n",
> > + bus->number);
> > + return PCIBIOS_DEVICE_NOT_FOUND;
> > + }
> > +
> > + addr += tegra_pcie_conf_offset(devfn, where);
> > + }
>
> It seems like that chunk of code could be shared between
> tegra_pcie_read_conf() and tegra_pcie_write_conf().
>
> Also, tegra_pcie_read_conf() checks again for addr == NULL and returns
> if so. read and write should be consistent.
Done.
> > +static void tegra_pcie_port_free(struct tegra_pcie_port *port)
> > +{
> > + struct tegra_pcie *pcie = port->pcie;
> > +
> > + devm_iounmap(pcie->dev, port->base);
> > + devm_release_mem_region(pcie->dev, port->regs.start,
> > + resource_size(&port->regs));
> > + list_del(&port->list);
> > + devm_kfree(pcie->dev, port);
> > +}
>
> Do ports get allocated and freed separately from the host controller,
> such that it's actually worth manually calling the
> devm_iounmap/release/free functions?
Yes, they can be freed separately. When a link is down and hence no
device is connected to the port, tegra_pcie_enable() will disable and
free that port.
That's in fact the only place where this is called because we don't
actually support removing the controller. I don't know of any ARM
architecture support to do that (i.e. there is no counterpart to
pci_common_init()).
> > +static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
> ...
> > + /* disable all exceptions */
> > + afi_writel(pcie, 0, AFI_FPCI_ERROR_MASKS);
>
> Is that a good idea?
I have no idea what that register means. It's one of the undocumented
registers. Note that I also didn't change this, it's been there since
the beginning.
It's probably worth finding out about the register's meaning and about
how to catch these exceptions at runtime, though. I'm adding Jay on Cc
maybe he can research internally.
> > +static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
>
> > + err = devm_request_irq(&pdev->dev, pcie->irq, tegra_pcie_isr,
> > + IRQF_SHARED, "PCIE", pcie);
>
> devm_request_irq and IRQF_SHARED sounds like a bad combination; what if
> the IRQ goes off after this driver's remove() is called, but before the
> driver core devm cleanup runs?
You're right. Changed to request_irq() in tegra_pcie_get_resources() and
added free_irq() to tegra_pcie_put_resources().
> > +static irqreturn_t tegra_pcie_msi_irq(int irq, void *data)
> ...
> > + return IRQ_HANDLED;
>
> Shouldn't this function return IRQ_NONE if no MSI status bits were found
> set?
The IRQ isn't marked IRQF_SHARED, so I don't think this is needed.
> > +static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
>
> > + /* setup AFI/FPCI range */
> > + msi->pages = __get_free_pages(GFP_KERNEL, 3);
>
> If tegra_msi_setup_irq() hard-codes the MSI address as msi->pages, then
> I expect you can get away with a single page here. Of course, perhaps
> tegra_msi_setup_irq() is supposed to give a different address to every
> MSI client? Even so, 256 clients * 4 bytes is still less than 1 page.
Yes, I'll try setting the order to 0 and see if that still works. I
don't have a setup with more than one MSI client so test results may not
be very reliable, though.
> > +static u32 tegra_pcie_get_xbar_config(struct tegra_pci
Re: [PATCH v3 07/12] PCI: tegra: Move PCIe driver to drivers/pci/host
On 04/03/2013 08:45 AM, Thierry Reding wrote:
> Move the PCIe driver from arch/arm/mach-tegra into the drivers/pci/host
> directory. The motivation is to collect various host controller drivers
> in the same location in order to facilitate refactoring.
>
> The Tegra PCIe driver has been largely rewritten, both in order to turn
> it into a proper platform driver and to add MSI (based on code by
> Krishna Kishore ) as well as device tree support.
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> +struct tegra_msi {
> + DECLARE_BITMAP(used, INT_PCI_MSI_NR);
> + struct irq_domain *domain;
> + struct msi_chip chip;
Nit: You could move that to be the first field in the struct, then
to_tegra_msi() would end up being a no-op, which might save a byte or two.
> +static int tegra_pcie_write_conf(struct pci_bus *bus, unsigned int devfn,
> + int where, int size, u32 value)
...
> + if (bus->number == 0) {
> + unsigned int slot = PCI_SLOT(devfn);
> + struct tegra_pcie_port *port;
> +
> + list_for_each_entry(port, &pcie->ports, list) {
> + if (port->index + 1 == slot) {
> + addr = port->base + (where & ~3);
> + break;
> + }
> + }
> + } else {
> + addr = tegra_pcie_bus_map(pcie, bus->number);
> + if (!addr) {
> + dev_err(pcie->dev,
> + "failed to map cfg. space for bus %u\n",
> + bus->number);
> + return PCIBIOS_DEVICE_NOT_FOUND;
> + }
> +
> + addr += tegra_pcie_conf_offset(devfn, where);
> + }
It seems like that chunk of code could be shared between
tegra_pcie_read_conf() and tegra_pcie_write_conf().
Also, tegra_pcie_read_conf() checks again for addr == NULL and returns
if so. read and write should be consistent.
> +static void tegra_pcie_port_free(struct tegra_pcie_port *port)
> +{
> + struct tegra_pcie *pcie = port->pcie;
> +
> + devm_iounmap(pcie->dev, port->base);
> + devm_release_mem_region(pcie->dev, port->regs.start,
> + resource_size(&port->regs));
> + list_del(&port->list);
> + devm_kfree(pcie->dev, port);
> +}
Do ports get allocated and freed separately from the host controller,
such that it's actually worth manually calling the
devm_iounmap/release/free functions?
> +static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
...
> + /* disable all exceptions */
> + afi_writel(pcie, 0, AFI_FPCI_ERROR_MASKS);
Is that a good idea?
> +static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
> + err = devm_request_irq(&pdev->dev, pcie->irq, tegra_pcie_isr,
> +IRQF_SHARED, "PCIE", pcie);
devm_request_irq and IRQF_SHARED sounds like a bad combination; what if
the IRQ goes off after this driver's remove() is called, but before the
driver core devm cleanup runs?
> +static irqreturn_t tegra_pcie_msi_irq(int irq, void *data)
...
> + return IRQ_HANDLED;
Shouldn't this function return IRQ_NONE if no MSI status bits were found
set?
> +static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
> + /* setup AFI/FPCI range */
> + msi->pages = __get_free_pages(GFP_KERNEL, 3);
If tegra_msi_setup_irq() hard-codes the MSI address as msi->pages, then
I expect you can get away with a single page here. Of course, perhaps
tegra_msi_setup_irq() is supposed to give a different address to every
MSI client? Even so, 256 clients * 4 bytes is still less than 1 page.
> +static u32 tegra_pcie_get_xbar_config(struct tegra_pcie *pcie, u32 lanes)
> +{
> + struct device_node *np = pcie->dev->of_node;
> +
> + switch (lanes) {
> + case 0x0004:
> + dev_info(pcie->dev, "single-mode configuration\n");
> + return AFI_PCIE_CONFIG_SM2TMS0_XBAR_CONFIG_SINGLE;
> +
> + case 0x0202:
> + dev_info(pcie->dev, "dual-mode configuration\n");
> + return AFI_PCIE_CONFIG_SM2TMS0_XBAR_CONFIG_DUAL;
> + }
> +
> + return 0;
Shouldn't that return an error, and dev_err() about it? If not, then I
think using one of the #defines for the default would make the result a
lot more obvious.
> +static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
> + pcie->xbar_config = tegra_pcie_get_xbar_config(pcie, lanes);
> + if (!pcie->xbar_config) {
> + dev_err(pcie->dev, "invalid lane configuration\n");
> + return -EINVAL;
> + }
Oh, but AFI_PCIE_CONFIG_SM2TMS0_XBAR_CONFIG_SINGLE==0, which is valid...
> +static bool tegra_pcie_port_check_link(struct tegra_pcie_port *port)
...
> + if (value & 0x2000)
> + return true;
Can we use a #define for 0x2000?
> +static int tegra_pcie_enable(struct tegra_pcie *pcie)
> +{
> + struct t
Re: [PATCH v3 07/12] PCI: tegra: Move PCIe driver to drivers/pci/host
On 04/05/2013 01:37 AM, Thierry Reding wrote: > 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. ... >> 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. I believe that reg and interrupts must be possible to interpret without resorting to reg-names and interrupt-names. In other words, one must always define a specific order for those properties, and the -names properties must follow that order. This is different than some newer bindings where the order is arbitrary and based on whatever random order the entries appear in *-names. Given that, I'm not actually convinced that reg-names or interrupt-names actually make any sense... For background, see: http://www.spinics.net/lists/linux-samsung-soc/msg16704.html >>> +- 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. The complexity is exactly why this needs to be fully documented. It's very important to document not only what needs to be there, but also the rationale behind the choice of exactly what and how to represent in ranges. This is necessary so that (a) everyone who participated in the discussion of that ranges definition can ensure that the final result and rationale is what was expected (b) if somebody else extends the binding for a new Tegra SoC in the future, they will have enough information to fully respect the rationale behind the existing binding. Is ranges really well-documented elsewhere? The raw property certainly is (although a link would be useful), but I'm not convinced that the concept of what each cell in the PCIe controller's own internal address space means actually is documented. Otherwise, why would there have been such a discussion on this topic? > 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 s/setup/set up/ > prefetchable PCI regions. The first cell determines the type of > region I assume this definition of "the first cell determines ..." only applies to these remaining entries, and not the first two entries. Perhaps prefix that sentence with "For these entries, "? > that is setup: - 0x8100: I/O memory region - 0x8200: > non-prefetchable memory region - 0xc200: prefetchable memory > region Please refer to the standard PCI bus binding document for a > more detailed explanation. That looks OK, yes. >>> +- 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
Re: [PATCH v3 07/12] PCI: tegra: Move PCIe driver to drivers/pci/host
On 04/05/2013 12:03 AM, Thierry Reding wrote: > On Thu, Apr 04, 2013 at 03:30:01PM -0600, Stephen Warren wrote: >> On 04/04/2013 03:28 PM, Stephen Warren wrote: >>> On 04/03/2013 08:45 AM, Thierry Reding wrote: Move the PCIe driver from arch/arm/mach-tegra into the drivers/pci/host directory. The motivation is to collect various host controller drivers in the same location in order to facilitate refactoring. The Tegra PCIe driver has been largely rewritten, both in order to turn it into a proper platform driver and to add MSI (based on code by Krishna Kishore ) as well as device tree support. >>> arch/arm/boot/dts/tegra20.dtsi | 53 + >>> >>> I guess this has to touch both the driver and the dtsi file so >>> that bisectabilty isn't broken? I guess that's OK. >> >> Actually, can't you move all the *.dts/*.dtsi changes to the >> start of the series, then put the driver conversion patch last, >> to avoid any bisectability issues and still keep code and DT >> changes separate? > > I'm not sure if that'll work. There's this oddity in the Harmony > DTS where the regulator@3 is disabled with a comment that this is a > hack required until board-harmony-pcie.c is removed. If we change > the DTS before the driver rewrite I think that would break > requesting the GPIO in the board file. Hmm. Well I guess that PCIe doesn't really work right now anyway. I mean the enumeration works, but actual instantiation of the Linux devices doesn't due to the resource conflict issues, which your new driver works around. As such, it might be simplest to just submit all the following in a single kernel version: * Delete old Tegra PCI support. * Add new driver (add not move); this would just touch drivers/pci/host. * Add all the required new DT content. That would be basically what you've already posted, except that the DT changes in this patch can be moved later, and all the arch/arm/*.c patches can happen earlier if you want. Does that sound reasonable? I think you can do: * All the DT changes first, except the one-line change to enable the regulator. DT additions for the new PCIe controller would be status=disabled. * Add/move the new PCI. We'd probably still want all this to go through a single tree so (i.e. the Tegra tree, and not submit the drivers/pci/host addition through the PCI tree) to avoid ever having 2 drivers at once for the Tegra PCI controller. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 07/12] PCI: tegra: Move PCIe driver to drivers/pci/host
For reference, here's the new binding document with all your comments
addressed (I think).
Thierry
NVIDIA Tegra PCIe controller
Required properties:
- compatible: "nvidia,tegra20-pcie"
- device_type: Must be "pci"
- 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
- 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
- pex-clk-supply: Supply voltage for internal reference clock
- vdd-supply: Power supply for controller (1.05V)
- bus-range: Range of bus numbers associated with this controller
- #address-cells: Address representation for root ports (must be 3)
- cell 0 specifies the bus and device numbers of the root port:
[23:16]: bus number
[15:11]: device number
- cell 1 denotes the upper 32 address bits and should be 0
- cell 2 contains the lower 32 address bits and is used to translate to the
CPU address space
- #size-cells: Size representation for root ports (must be 2)
- 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:
- 0x8100: I/O memory region
- 0x8200: non-prefetchable memory region
- 0xc200: prefetchable memory region
Please refer to the standard PCI bus binding document for a more detailed
explanation.
- clocks: List of clock inputs of the controller. Must contain an entry for
each entry in the clock-names property.
- 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
Root ports are defined as subnodes of the PCIe controller node.
Required properties:
- device_type: Must be "pci"
- assigned-addresses: Address and size of the port configuration registers
- reg: PCI bus address of the root port
- #address-cells: Must be 3
- #size-cells: Must be 2
- ranges: Sub-ranges distributed from the PCIe controller node. An empty
property is sufficient.
- nvidia,num-lanes: Number of lanes to use for this port. Valid combinations
are:
- Root port 0 uses 4 lanes, root port 1 is unused.
- Both root ports use 2 lanes.
Example:
SoC DTSI:
pcie-controller {
compatible = "nvidia,tegra20-pcie";
device_type = "pci";
reg = <0x80003000 0x0800 /* PADS registers */
0x80003800 0x0200 /* AFI registers */
0x9000 0x1000>; /* configuration space */
reg-names = "pads", "afi", "cs";
interrupts = <0 98 0x04 /* controller interrupt */
0 99 0x04>; /* MSI interrupt */
interrupt-names = "intr", "msi";
bus-range = <0x00 0xff>;
#address-cells = <3>;
#size-cells = <2>;
ranges = <0x8200 0 0x8000 0x8000 0 0x1000 /*
port 0 registers */
0x8200 0 0x80001000 0x80001000 0 0x1000 /*
port 1 registers */
0x8100 0 0 0x8200 0 0x0001 /*
downstream I/O */
0x8200 0 0xa000 0xa000 0 0x1000 /*
non-prefetchable memory */
0xc200 0 0xb000 0xb000 0 0x1000>; /*
prefetchable memory */
clocks = <&tegra_car 70>, <&tegra_car 72>, <&tegra_car 74>,
<&tegra_car 118>;
clock-names = "pex", "afi", "pcie_xclk", "pll_e";
status = "disabled";
pci@1,0 {
device_type = "pci";
assigned-addresses = <0x82000800 0 0x8000 0 0x1000>;
reg = <0x000800 0 0 0 0>;
status = "disabled";
#address-cells = <3>;
Re: [PATCH v3 07/12] PCI: tegra: Move PCIe driver to drivers/pci/host
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:
- 0x8100: I/O memory region
- 0x8200: non-prefetchable memory region
- 0xc200: 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 */
> >
Re: [PATCH v3 07/12] PCI: tegra: Move PCIe driver to drivers/pci/host
On Thu, Apr 04, 2013 at 03:30:01PM -0600, Stephen Warren wrote: > On 04/04/2013 03:28 PM, Stephen Warren wrote: > > On 04/03/2013 08:45 AM, Thierry Reding wrote: > >> Move the PCIe driver from arch/arm/mach-tegra into the drivers/pci/host > >> directory. The motivation is to collect various host controller drivers > >> in the same location in order to facilitate refactoring. > >> > >> The Tegra PCIe driver has been largely rewritten, both in order to turn > >> it into a proper platform driver and to add MSI (based on code by > >> Krishna Kishore ) as well as device tree support. > > > >> arch/arm/boot/dts/tegra20.dtsi | 53 + > > > > I guess this has to touch both the driver and the dtsi file so that > > bisectabilty isn't broken? I guess that's OK. > > Actually, can't you move all the *.dts/*.dtsi changes to the start of > the series, then put the driver conversion patch last, to avoid any > bisectability issues and still keep code and DT changes separate? I'm not sure if that'll work. There's this oddity in the Harmony DTS where the regulator@3 is disabled with a comment that this is a hack required until board-harmony-pcie.c is removed. If we change the DTS before the driver rewrite I think that would break requesting the GPIO in the board file. A possible workaround I can think of is accessing the regulator from board-harmony-pcie.c instead of the GPIO directly. But perhaps such an attempt already failed (deferred probing getting in the way?). Since it looks like this series will not make it into 3.10, maybe we should try and at least get these things ironed out so that the merge can be done more easily in 3.11. One big step in that direction would obviously be if we could get the DTS changes merged already which, as you point out, should be independent of the driver rewrite itself. Thierry pgpCUZrpSZXxh.pgp Description: PGP signature
Re: [PATCH v3 07/12] PCI: tegra: Move PCIe driver to drivers/pci/host
On 04/04/2013 03:28 PM, Stephen Warren wrote: > On 04/03/2013 08:45 AM, Thierry Reding wrote: >> Move the PCIe driver from arch/arm/mach-tegra into the drivers/pci/host >> directory. The motivation is to collect various host controller drivers >> in the same location in order to facilitate refactoring. >> >> The Tegra PCIe driver has been largely rewritten, both in order to turn >> it into a proper platform driver and to add MSI (based on code by >> Krishna Kishore ) as well as device tree support. > >> arch/arm/boot/dts/tegra20.dtsi | 53 + > > I guess this has to touch both the driver and the dtsi file so that > bisectabilty isn't broken? I guess that's OK. Actually, can't you move all the *.dts/*.dtsi changes to the start of the series, then put the driver conversion patch last, to avoid any bisectability issues and still keep code and DT changes separate? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 07/12] PCI: tegra: Move PCIe driver to drivers/pci/host
On 04/03/2013 08:45 AM, Thierry Reding wrote:
> Move the PCIe driver from arch/arm/mach-tegra into the drivers/pci/host
> directory. The motivation is to collect various host controller drivers
> in the same location in order to facilitate refactoring.
>
> The Tegra PCIe driver has been largely rewritten, both in order to turn
> it into a proper platform driver and to add MSI (based on code by
> Krishna Kishore ) as well as device tree support.
> arch/arm/boot/dts/tegra20.dtsi | 53 +
I guess this has to touch both the driver and the dtsi file so that
bisectabilty isn't broken? I guess that's OK.
> 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. 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?
> +- 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.
> +- 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)
> +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.
> +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?
> 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?
> 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?
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
I'll review that file separately later.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
