[PATCH] of: Support a PCI device that is compatible with 'simple-bus'

[PATCH] of: Support a PCI device that is compatible with 'simple-bus'

[Jason Gunthorpe]

The intended use for this feature is to let a PCI device declare
itself a 'simple-bus' and then describe additional devices
(such as GPIOs, I2C, etc) nested below itself.

The devices nested below the PCI device will use 'reg' addressing
with the 5 dw format used by PCI.

This is for embedded cases where the PCI device may be a complex
SOC with no PCI based partitioning of sub-functionality.

Tested on an ARM kirkwood system

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 drivers/of/address.c |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)

Grant:

> If the soc_devices are getting triggered on that and they shouldn't be,
> then we need a mechanism in the soc_bridge node to kick out of that
> behavoir for its children.

Is this what you were thinking?

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 0125524..cf9dac5 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -110,6 +110,25 @@ static int of_bus_pci_match(struct device_node *np)
 	return !strcmp(np->type, "pci") || !strcmp(np->type, "vci");
 }
 
+/*
+ * When simple-bus is nested below a PCI bus then we treat the simple-bus
+ * as having the same 3 address/2 size semantics as the PCI bus, but create
+ * platform devices like normal.
+ */
+static int of_bus_platform_pci_match(struct device_node *np)
+{
+	struct device_node *parent;
+	int parent_is_pci;
+
+	parent = of_get_parent(np);
+	if (parent == NULL)
+		return 0;
+	parent_is_pci = of_bus_pci_match(parent);
+	of_node_put(parent);
+
+	return parent_is_pci && of_device_is_compatible(np, "simple-bus");
+}
+
 static void of_bus_pci_count_cells(struct device_node *np,
 				   int *addrc, int *sizec)
 {
@@ -293,6 +312,16 @@ static unsigned int of_bus_isa_get_flags(const __be32 *addr)
 
 static struct of_bus of_busses[] = {
 #ifdef CONFIG_PCI
+	/* Platform devices beneath a PCI device */
+	{
+		.name = "platform_pci",
+		.addresses = "reg",
+		.match = of_bus_platform_pci_match,
+		.count_cells = of_bus_pci_count_cells,
+		.map = of_bus_pci_map,
+		.translate = of_bus_pci_translate,
+		.get_flags = of_bus_pci_get_flags,
+	},
 	/* PCI */
 	{
 		.name = "pci",
-- 
1.7.5.4

Re: [PATCH] of: Support a PCI device that is compatible with 'simple-bus'

[Grant Likely]

On Mon, 10 Dec 2012 14:51:19 -0700, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:
> The intended use for this feature is to let a PCI device declare
> itself a 'simple-bus' and then describe additional devices
> (such as GPIOs, I2C, etc) nested below itself.
> 
> The devices nested below the PCI device will use 'reg' addressing
> with the 5 dw format used by PCI.
> 
> This is for embedded cases where the PCI device may be a complex
> SOC with no PCI based partitioning of sub-functionality.
> 
> Tested on an ARM kirkwood system
> 
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>  drivers/of/address.c |   29 +++++++++++++++++++++++++++++
>  1 files changed, 29 insertions(+), 0 deletions(-)
> 
> Grant:
> 
> > If the soc_devices are getting triggered on that and they shouldn't be,
> > then we need a mechanism in the soc_bridge node to kick out of that
> > behavoir for its children.
> 
> Is this what you were thinking?

Not really. I see what you're trying to do, but doing it this way forces
all children of PCI nodes to use the PCI addressing space. Others have
had simple children of PCI devices and didn't use the PCI address layout
at all. Those users would break with this approach.

However, if you want to pass a unity mapping from the PCI device to the
a child of it, it should be sufficient to use an empty 'ranges;'
property in the PCI device node instead of listing out the ranges that
you want to translate.

g.

Re: [PATCH] of: Support a PCI device that is compatible with 'simple-bus'

[Jason Gunthorpe]

On Fri, Dec 14, 2012 at 08:26:29PM +0000, Grant Likely wrote:

> > > If the soc_devices are getting triggered on that and they shouldn't be,
> > > then we need a mechanism in the soc_bridge node to kick out of that
> > > behavoir for its children.
> > 
> > Is this what you were thinking?
> 
> Not really. I see what you're trying to do, but doing it this way forces
> all children of PCI nodes to use the PCI addressing space. Others have
> had simple children of PCI devices and didn't use the PCI address layout
> at all. Those users would break with this approach.

Yes, that's right.

If you drop 'device_type=pci' from the PCI device (keep it on the host
bridge), then you can setup a ranges down to a smaller width and
things seem to work OK. That must be what other users are doing.

However, you can't stay at address-cells=3 for the children. That
doesn't work.

So, if you have separate PCI regions, like MMIO and prefetch it looks
like this works OK:

 pci_device@0 {
   ranges =
        // MMIO region, BAR 0
       <0x20000000 0x00000000  0x02000000 0x00000000 0x00000000  0x0 0x8000000
        // Prefetch region, BAR 1
        0x40000000 0x00000000  0x42000000 0x00000000 0x00000000  0x0 0x8000000>;

   #size-cells = <1>;
   #address-cells = <2>;
   sub {
     // MMIO region at BAR 0 offset 0x2000
     reg = <0x20000000 0x00002000 0x1000>;
   }
   sub2 {
     // Prefetch region at BAR 1 offset 0x4000
     reg = <0x40000000 0x00004000 0x1000>;
   }
 }

Which is weird, but OK..

This is good enough for my application.. fixing up address-cells=3 to
work generally seems pretty complicated at first blush?

> However, if you want to pass a unity mapping from the PCI device to the
> a child of it, it should be sufficient to use an empty 'ranges;'
> property in the PCI device node instead of listing out the ranges that
> you want to translate.

It isn't a unity mapping - the children see address 0 as being the
start of a BAR. The DTS has three levels of translation:

- platform device child - 0 is the start of a BAR in the pci device
- pci device - 0 is the start of the host bridge memory window for the
  BAR's type
- pci controller - 0 is the start of physical memory

Thanks,
Jason

Re: [PATCH] of: Support a PCI device that is compatible with 'simple-bus'

[Grant Likely]

On Fri, 14 Dec 2012 14:58:14 -0700, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:
> On Fri, Dec 14, 2012 at 08:26:29PM +0000, Grant Likely wrote:
> 
> > > > If the soc_devices are getting triggered on that and they shouldn't be,
> > > > then we need a mechanism in the soc_bridge node to kick out of that
> > > > behavoir for its children.
> > > 
> > > Is this what you were thinking?
> > 
> > Not really. I see what you're trying to do, but doing it this way forces
> > all children of PCI nodes to use the PCI addressing space. Others have
> > had simple children of PCI devices and didn't use the PCI address layout
> > at all. Those users would break with this approach.
> 
> Yes, that's right.
> 
> If you drop 'device_type=pci' from the PCI device (keep it on the host
> bridge), then you can setup a ranges down to a smaller width and
> things seem to work OK. That must be what other users are doing.
> 
> However, you can't stay at address-cells=3 for the children. That
> doesn't work.
> 
> So, if you have separate PCI regions, like MMIO and prefetch it looks
> like this works OK:
> 
>  pci_device@0 {
>    ranges =
>         // MMIO region, BAR 0
>        <0x20000000 0x00000000  0x02000000 0x00000000 0x00000000  0x0 0x8000000
>         // Prefetch region, BAR 1
>         0x40000000 0x00000000  0x42000000 0x00000000 0x00000000  0x0 0x8000000>;
> 
>    #size-cells = <1>;
>    #address-cells = <2>;
>    sub {
>      // MMIO region at BAR 0 offset 0x2000
>      reg = <0x20000000 0x00002000 0x1000>;
>    }
>    sub2 {
>      // Prefetch region at BAR 1 offset 0x4000
>      reg = <0x40000000 0x00004000 0x1000>;
>    }
>  }
> 
> Which is weird, but OK..
> 
> This is good enough for my application.. fixing up address-cells=3 to
> work generally seems pretty complicated at first blush?
> 
> > However, if you want to pass a unity mapping from the PCI device to the
> > a child of it, it should be sufficient to use an empty 'ranges;'
> > property in the PCI device node instead of listing out the ranges that
> > you want to translate.
> 
> It isn't a unity mapping - the children see address 0 as being the
> start of a BAR. The DTS has three levels of translation:
> 
> - platform device child - 0 is the start of a BAR in the pci device
> - pci device - 0 is the start of the host bridge memory window for the
>   BAR's type
> - pci controller - 0 is the start of physical memory

Then it sounds like you really don't want PCI addressing in the
children. It sounds like the children addresses need to be in the form
[bar#] [offset-from-base-of-bar]. In that case, you need the appropriate
ranges entries to translate from each bar to the PCI address space,
which is exactly what other users are doing. Whether your address format
uses 2 cells or 3, it shouldn't matter. The translation mechanism is the
same:

ranges = <[child-address1] [parent-address2] [child-size]>,
	 <[child-address2] [parent-address2] [child-size]>,
	 ...
	 ;

I may still be missing something though. Here is what I've been
assuming:

- The PCI device has N BARs.
- The PCI bus assigns addresses to the BARs (exposed in assigned-addresses)
- Each BAR may use different address region (IO/mem) or have different
  behaviour (prefetch/non-prefetch)
- either:
  a) The PCI device has a separate local bus behind each BAR. Devices and/or
     memory are attached to those busses and must be accessed through the
     relevant BAR.
  --or--
  b) The PCI device has a single local bus which the BARs map onto.
     Internally, the BARs get mapped onto local bus regions and may
     possibly overlap.

Am I correct so far?

In the a) case, it would make sense for the address format for the
device to be 1 cell for the bar# and 1 or 2 cells for the offset off the
base of the bar. One ranges entry is needed for each bar to translate
from a PCI address range to the BAR region.

In the b) case, the address of the child devices should be the exact
address on the local bus. Again one ranges entry is needed for each BAR,
but this time it encodes the full mapping from PCI address to local
address.

In both cases, the type of transfer is encoded by the BAR address and
does not get exposed to the child device. Exposing the PCI flags into
the child bus(es) really isn't a very good idea because they don't make
sense in that context. It may seem expedient, but it will be fragile.

I apologize if I'm being dense here and missing something about why you
need to use PCI addressing on a non-PCI bus segment. Please let me know
if I'm missing something important.

g.

Re: [PATCH] of: Support a PCI device that is compatible with 'simple-bus'

[Jason Gunthorpe]

On Wed, Dec 19, 2012 at 12:54:51PM +0000, Grant Likely wrote:
 
> Then it sounds like you really don't want PCI addressing in the
> children. It sounds like the children addresses need to be in the form
> [bar#] [offset-from-base-of-bar]. In that case, you need the
> appropriate

They should be interchangeable - in 99% of cases with multiple BARs
each BAR will be a different address type, so tagging by address type
or tagging by bar number is basically the same. I started with the 3
dw format only because that made sense to me and tried to make that
work. As it turns out the 2dw or 1dw format works fine out without any
patching and has the same basic functionality via the ranges
remapping.

> ranges entries to translate from each bar to the PCI address space,
> which is exactly what other users are doing. Whether your address format
> uses 2 cells or 3, it shouldn't matter. The translation mechanism is
> the

It does matter, I could not make 3 cells work, I sent you the various
approaches I tested that fix this, but they were not terribly
general.. I'm still very unclear what the root problem is, though..

> In both cases, the type of transfer is encoded by the BAR address and
> does not get exposed to the child device. Exposing the PCI flags into
> the child bus(es) really isn't a very good idea because they don't make
> sense in that context. It may seem expedient, but it will be fragile.

Well, it makes as much sense as for a PCI driver - each of the three
transfer types has different coding requirements in the driver, so
each of the three type must be kept separate.

I haven't looked super closely at this, but the basic desire is to
have IORESOURCE_PREFETCH tagged on the struct resource that reaches
the platform driver. 'get_flags' for a non-PCI addresses scheme always
returns IORESOURCE_MEM, and translating through a ranges doesn't
appear to fix that. This is where 3dw is desirable because it uses a
get_flags that understands the three resource types.

> I apologize if I'm being dense here and missing something about why you
> need to use PCI addressing on a non-PCI bus segment. Please let me know
> if I'm missing something important.

I think you've got it basically right. I misunderstood some of what
you were originally saying about using ranges from your earlier
mail. What you described is fine for IORESOURCE_MEM style regions, and
I've confirmed that it works OK in my cases.

Regards,
Jason

Re: [PATCH] of: Support a PCI device that is compatible with 'simple-bus'

[Grant Likely]

On Wed, 19 Dec 2012 12:18:00 -0700, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:
> On Wed, Dec 19, 2012 at 12:54:51PM +0000, Grant Likely wrote:
> > In both cases, the type of transfer is encoded by the BAR address and
> > does not get exposed to the child device. Exposing the PCI flags into
> > the child bus(es) really isn't a very good idea because they don't make
> > sense in that context. It may seem expedient, but it will be fragile.
> 
> Well, it makes as much sense as for a PCI driver - each of the three
> transfer types has different coding requirements in the driver, so
> each of the three type must be kept separate.
> 
> I haven't looked super closely at this, but the basic desire is to
> have IORESOURCE_PREFETCH tagged on the struct resource that reaches
> the platform driver. 'get_flags' for a non-PCI addresses scheme always
> returns IORESOURCE_MEM, and translating through a ranges doesn't
> appear to fix that. This is where 3dw is desirable because it uses a
> get_flags that understands the three resource types.

We could possibly change the ranges translation code to pick up
IORESOURCE_PREFETCH when a translation crosses a PREFETCH bar. That
would also ensure that child nodes don't have to keep the flags field
consistent with the ranges mapping.

g.