address translation for PCIe-to-localbus bridge

address translation for PCIe-to-localbus bridge

[Gerlando Falauto]

Hi everyone,

I am currently trying to describe an external device within a device 
tree in a Kirkwood design.
Such device is accessed through a local (parallel) bus. Since Kirkwood 
does not provide such an interface, we added a custom FPGA (PCIe device) 
which implements a PCIe-to-localbus bridge.
So essentially BAR0 provides the configuration space for such a bridge, 
and BAR1 provides a remapped area where accesses to the localbus can be 
performed. BAR2 and BAR3 provide other functions.

So with the appropriate reg encoding of the PCI device, the PCI driver 
for the FPGA will automatically get an of_node.

My question is: is there any way I can describe this external device (I 
believe as a child node of the PCI device), so that a call to 
of_address_to_resource() (or equivalent) from its driver will 
automatically translate a localbus address (e.g. 0x0000abcd) to whatever 
address was assigned to BAR1 (in my case 0xe0000000 -> 0xe000abcd)?

I looked up drivers/of/address.c and found of_pci_address_to_resource() 
which seems to do what I want, but it seems that this must be explicitly 
called by the client driver. This means that the driver for the external 
device must be aware it's connected through PCI, which should in 
principle not be necessary.
What I'd like to do instead is describe the device at the appropriate 
location within the device tree (using the appropriate "ranges" 
description) so that its driver can figure out its address space 
automatically (after the PCI device gets probed and enabled of course).

But I found no way to describe which BAR it should refer to, for instance.

Perhaps the "rrrrrrrr" part of phys.hi, using BAR0=0x10, BAR1=0x14, and 
so on?
Or else I should define a new instance of of bus (i.e. 
"pci_lbus_bridge") and invent yet another address encoding syntax?
Any hints?

Thank you su much in advance!
Gerlando

P.S. Here's the skeleton of the this part of the tree (up to the bridge).

pcie-controller {
	compatible = "marvell,kirkwood-pcie";
	status = "okay";
	device_type = "pci";

	#address-cells = <3>;
	#size-cells = <2>;
	bus-range = <0x00 0xff>;

	ranges = <0x82000000 0 0x00040000 0x00040000 0 0x00002000   /* Port 0.0 
registers */
		  0x82000000 0 0xe0000000 0xe0000000 0 0x08000000   /* 
non-prefetchable memory */
	          0x81000000 0 0          0xe8000000 0 0x00100000>; /* 
downstream I/O */

			/* PCI BUS 1 */
			pcie@1,0 {
				device_type = "pci";
				assigned-addresses = <0x82000800 0 0x00040000 0 0x2000>;
				reg = <0x0800 0 0 0 0>;
				#address-cells = <3>;
				#size-cells = <2>;
				...

				/* FPGA, device 0 */
				km_fpga: pcie@0,0 {
					/* Only the first cell matters,
					npt000ss bbbbbbbb dddddfff rrrrrrrr
					where b is 8-bit bus, d is 5-bit device, f is 3-bit function */
					reg = <0x010000 0 0 0 0>;
					compatible = "km_fpga", "pci10ee,0009";
					...
				};
			};
		};

	};
};

And this is what I get at startup:

mvebu-pcie pcie-controller.1: PCIe0.0: link up
mvebu-pcie pcie-controller.1: PCI host bridge to bus 0000:00
pci_bus 0000:00: root bus resource [io  0x1000-0xfffff]
pci_bus 0000:00: root bus resource [mem 0xe0000000-0xebffffff]
pci_bus 0000:00: root bus resource [bus 00-ff]
pci 0000:00:01.0: [11ab:7846] type 01 class 0x060400
PCI: bus0: Fast back to back transfers disabled
pci 0000:00:01.0: bridge configuration invalid ([bus 00-00]), reconfiguring
pci 0000:01:00.0: [10ee:0009] type 00 class 0x050000
pci 0000:01:00.0: reg 10: [mem 0x00000000-0x00000fff]
pci 0000:01:00.0: reg 14: [mem 0x00000000-0x07ffffff]
pci 0000:01:00.0: reg 18: [mem 0x00000000-0x00000fff]
pci 0000:01:00.0: reg 1c: [mem 0x00000000-0x00000fff]
pci 0000:01:00.0: supports D1 D2
pci 0000:01:00.0: PME# supported from D0 D1 D2 D3hot
PCI: bus1: Fast back to back transfers disabled
pci_bus 0000:01: busn_res: [bus 01-ff] end is updated to 01
pci 0000:00:01.0: BAR 8: assigned [mem 0xe0000000-0xebffffff]
pci 0000:01:00.0: BAR 1: assigned [mem 0xe0000000-0xe7ffffff]
pci 0000:01:00.0: BAR 0: assigned [mem 0xe8000000-0xe8000fff]
pci 0000:01:00.0: BAR 2: assigned [mem 0xe8001000-0xe8001fff]
pci 0000:01:00.0: BAR 3: assigned [mem 0xe8002000-0xe8002fff]
pci 0000:00:01.0: PCI bridge to [bus 01]
pci 0000:00:01.0:   bridge window [mem 0xe0000000-0xebffffff]
PCI: enabling device 0000:00:01.0 (0140 -> 0143)

What I'd like to have in the device tree is something like:

&km_fpga {
	...
	device_type = "pci"  /* or pci_lbus_bridge? */
	#address-cells = <2>;/* BAR + local bus address */
	#size-cells = <1>;   /* 32-bit addressing is more than enough */
	ranges = <?????>;    /* ???? */
	slave@0,0 {
		compatible = "keymile,slave";
		reg = <1 0x000 0x200>; /* Use address space 0x0-0x200 from BAR 1 */
	}
}
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: address translation for PCIe-to-localbus bridge

[Thierry Reding]

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

On Wed, Nov 06, 2013 at 11:27:15AM +0100, Gerlando Falauto wrote:
> Hi everyone,
> 
> I am currently trying to describe an external device within a device
> tree in a Kirkwood design.
> Such device is accessed through a local (parallel) bus. Since
> Kirkwood does not provide such an interface, we added a custom FPGA
> (PCIe device) which implements a PCIe-to-localbus bridge.
> So essentially BAR0 provides the configuration space for such a
> bridge, and BAR1 provides a remapped area where accesses to the
> localbus can be performed. BAR2 and BAR3 provide other functions.
> 
> So with the appropriate reg encoding of the PCI device, the PCI
> driver for the FPGA will automatically get an of_node.
> 
> My question is: is there any way I can describe this external device
> (I believe as a child node of the PCI device), so that a call to
> of_address_to_resource() (or equivalent) from its driver will
> automatically translate a localbus address (e.g. 0x0000abcd) to
> whatever address was assigned to BAR1 (in my case 0xe0000000 ->
> 0xe000abcd)?

Perhaps I don't understand properly, but what good is the local bus
address to any driver? I mean you'll have to have a PCI driver to bind
to the PCI device, right? And that PCI driver will only need to obtain
the register addresses from BAR1, then access that region.

Why would you need to have it translated via DT?

Thierry

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

Re: address translation for PCIe-to-localbus bridge

[Gerlando Falauto]

Hi Thierry,

thanks again for your patience!

On 11/06/2013 01:23 PM, Thierry Reding wrote:
> On Wed, Nov 06, 2013 at 11:27:15AM +0100, Gerlando Falauto wrote:
>> Hi everyone,
>>
>> I am currently trying to describe an external device within a device
>> tree in a Kirkwood design.
>> Such device is accessed through a local (parallel) bus. Since
>> Kirkwood does not provide such an interface, we added a custom FPGA
>> (PCIe device) which implements a PCIe-to-localbus bridge.
>> So essentially BAR0 provides the configuration space for such a
>> bridge, and BAR1 provides a remapped area where accesses to the
>> localbus can be performed. BAR2 and BAR3 provide other functions.
>>
>> So with the appropriate reg encoding of the PCI device, the PCI
>> driver for the FPGA will automatically get an of_node.
>>
>> My question is: is there any way I can describe this external device
>> (I believe as a child node of the PCI device), so that a call to
>> of_address_to_resource() (or equivalent) from its driver will
>> automatically translate a localbus address (e.g. 0x0000abcd) to
>> whatever address was assigned to BAR1 (in my case 0xe0000000 ->
>> 0xe000abcd)?
>
> Perhaps I don't understand properly, but what good is the local bus
> address to any driver? I mean you'll have to have a PCI driver to bind
> to the PCI device, right?

Correct, and that's the PCI driver for the FPGA, which is (among other 
things) a PCI-to-localbus bridge.

 > And that PCI driver will only need to obtain
> the register addresses from BAR1,

Correct.

> then access that region.

That's not 100% right. The PCI driver will only _provide_ the region for 
BAR1 (as it is a bridge), not _access_ it.

> Why would you need to have it translated via DT?

Because it would be a *separate* driver, the one handling the /external/ 
"slave" device, to use that region.

In principle, if I wanted to reuse that same external device on a 
completely different architecture (which *does* natively provide a 
localbus) I should be able to move that same device node together with 
its driver. Provided the node is under the right bus, and translation is 
configured properly in the device tree, it should then work out of the 
box. Or am I completely mistaken?

What I did now (and it works) was to put the device node for the 
external device right under the main bus,

/ {
...
	ocp@f1000000 {
		slave@0,0 {
...
			compatible = "keymile,slave";
			reg = <0xe0000000 0x200>;
		};
	};
};

where "0xe0000000" is hardcoded, being the result of the dynamic assignment:

pci 0000:01:00.0: BAR 1: assigned [mem 0xe0000000-0xe7ffffff]

I'd like to avoid that, saying "slave@0,0 is accessible through the 
memory area 0x0-0x200 within BAR1 of device 0 on bus 1".

Thanks again,
Gerlando
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: address translation for PCIe-to-localbus bridge

[Jason Gunthorpe]

On Wed, Nov 06, 2013 at 11:27:15AM +0100, Gerlando Falauto wrote:
> Hi everyone,
> 
> I am currently trying to describe an external device within a device
> tree in a Kirkwood design.

Here is what works for me:

        mbus {
                compatible = "marvell,kirkwood-mbus", "simple-bus";
                ranges = <MBUS_ID(0x04, 0xe8) 0 0xe0000000 0x8000000  /* PEX 0 MEM */
                          MBUS_ID(0xf0, 0x01) 0 0xf1000000 0x100000     /* internal-regs */
                          MBUS_ID(0x01, 0x2f) 0 0xf4000000 0x10000      /* nand flash */
                          MBUS_ID(0x01, 0x1d) 0 0xffe00000 0x10000      /* boot rom */
                          MBUS_ID(0x01, 0x1e) 0 0xfff00000 0x10000      /* spi */
                          >;
                pcie-mem-aperture = <0xe0000000 0x10000000>; /* 256 MiB memory space */
                pex@e0000000 {
                        compatible = "marvell,kirkwood-pcie";
                        device_type = "pci";
                        ranges = <0x82000000 0 0x40000 MBUS_ID(0xf0, 0x01) 0x40000 0 0x00002000 /* Controller regs */
                                  0x82000000 1 0       MBUS_ID(0x04, 0xe8) 0 1 0 /* Port 0.0 MEM */
                                  >;

                        bus-range = <0x0 0xFF>;
                        pcie@1,0 {
                                device_type = "pci";
                                assigned-addresses = <0x82000800 0 0x40000 0 0x2000>;
                                reg = <0x0800 0 0 0 0>;
                                ranges = <0x82000000 0 0 0x82000000 0x1 0 1 0
                                          0x81000000 0 0 0x81000000 0x1 0 1 0>;
                                fpga@0 {
                                        reg = <0x8 0 0  0 0>;
                                        ranges = <0x00000000  0x82000000 0x00000000 0x00000000  0x8000000>;
                                        gpio3: fpga_gpio@8 {
                                                #gpio-cells = <2>;
                                                compatible = "linux,basic-mmio-gpio";
                                                gpio-controller;
                                                reg-names = "dat", "set", "dirin";
                                                reg = <0x8 4>,
                                                      <0xc 4>,
                                                      <0x10 4>;
                                        };

(some hopefully irrelevant items omitted for brevity)

I use code like this in the FPGA PCI driver to load the DT nodes:

        struct of_device_id match_table[2] = {};
        struct device_node *child;
        int rc = 0;

        for_each_child_of_node(root, child) {
                /* Can't just create a single device.. */
                strlcpy(match_table[0].name, child->name,
                        sizeof(match_table[0].name));
                strlcpy(match_table[0].type, child->type,
                        sizeof(match_table[0].type));
                rc = of_platform_bus_probe(child, match_table,
                                           parent);
                if (rc)
                        break;
        }
(root would be the of_node of the FPGA)

My system hot plugs the FPGA in and out, so I use this in the FPGA
remove function to tear it all down:

        struct device_node *child;
        struct platform_device *devs[100];
        unsigned int cur = 0;

        /* We need to remove the platform devices in the reverse of the order
           we created them. The DT is topo sorted in dependency order. */
        for_each_child_of_node(pdev->dev.of_node, child) {
                struct platform_device *dev = of_find_device_by_node(child);
                if (dev)
                        devs[cur++] = dev;
        }

        while (cur != 0) {
                cur--;
                platform_device_unregister(devs[cur]);
        }

Which is a bit tricky and suboptimal.

Thomas: There is one buglet here that I haven't had time to do
anything about. Notice the DT is listing the PEX memory window in its
ranges. I've done this for two reasons
 - The bootloader sets this address range up, so it is correct to
   include in the DT
 - The address translation machinery requires it, otherwise we can't
   translate addreses of the non-PCI sub devices (eg gpio3)

The latter is a kernel issue. As we discussed when mbus was first put
together something needs to make the ranges consistent with the actual
mapping so that address translation works. IIRC people objected to
actually changing the ranges at runtime, so the alternate mechanism of
hooking the address translation seems necessary?

Unfortunately if you add the ranges then the mbus driver throws a
warning that it is trying to overwrite existing windows, but otherwise
things work OK.

> But I found no way to describe which BAR it should refer to, for instance.
> 
> Perhaps the "rrrrrrrr" part of phys.hi, using BAR0=0x10, BAR1=0x14,
> and so on?  Or else I should define a new instance of of bus (i.e.
> "pci_lbus_bridge") and invent yet another address encoding syntax?

I do feel there is some missing dynamic elements in this scheme, eg
the ranges value at the FPGA node should be dynamic based on the BAR
offset, but that is a generality that isn't important if you have only
one PCI device.

Jason
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: address translation for PCIe-to-localbus bridge

[Thomas Petazzoni]

Dear Jason Gunthorpe,

(Adding a bunch of mvebu people in Cc)

On Wed, 6 Nov 2013 10:36:49 -0700, Jason Gunthorpe wrote:

> Thomas: There is one buglet here that I haven't had time to do
> anything about. Notice the DT is listing the PEX memory window in its
> ranges. I've done this for two reasons
>  - The bootloader sets this address range up, so it is correct to
>    include in the DT

The fact that the bootloader sets this MBus window is more-or-less
irrelevant because when the mvebu-mbus driver is initialized, it
completely clears *all* existing MBus windows:

   http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/bus/mvebu-mbus.c#n722

Therefore, when the kernel starts, what the bootloader may have set up
in terms of MBus windows is irrelevant.

>  - The address translation machinery requires it, otherwise we can't
>    translate addreses of the non-PCI sub devices (eg gpio3)

Right.

> The latter is a kernel issue. As we discussed when mbus was first put
> together something needs to make the ranges consistent with the actual
> mapping so that address translation works. IIRC people objected to
> actually changing the ranges at runtime, so the alternate mechanism of
> hooking the address translation seems necessary?

I indeed remember some objections, but I'm not sure what they were
precisely. Maybe we didn't had a precise use case back at the time, to
really make people objecting realize what the problem was?

On the other hand, I think the of_*() API is quite limited when it
comes to updating the DT. If I remember correctly, you can update some
nodes, but you can never reclaim the memory that was used for the
previous value of the node. So any change to the in-memory DT
representation is basically a memory leak for the entire lifetime of
the system (of course, I might be wrong on this, I haven't dived into
all the hardcore details of DT manipulation in the kernel).

I'm not sure what would be the alternate mechanism to hook into the
address translation. of_translate_one(), where all the translation
through ranges takes place is really tied to the DT only, adding
another mechanism to hook some custom address translation in there
seems a bit weird, no?

> Unfortunately if you add the ranges then the mbus driver throws a
> warning that it is trying to overwrite existing windows, but otherwise
> things work OK.

Yes, because at boot time the mvebu-mbus driver will set up windows for
the statically defined ranges (the one you've written explicitly in the
DT), and then later one when the PCIe driver will initialize, it will
enumerate devices, realize that it needs a PCIe memory window, and ask
the mvebu-mbus driver to create, which will fail because an overlapping
window already exists.

However, it just works by pure luck: nothing guarantees you that the
PCIe 0 memory window will start at 0xe0000000. Of course, since you
have only one PCIe interface enabled, you have a guarantee from one
boot to another. But in the general case, if you have several PCIe
interfaces, on which you may plug/unplug different devices, you have no
guarantee a priori as to what will be the base address of the PCIe
memory window for a given PCIe interface.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: address translation for PCIe-to-localbus bridge

[Jason Gunthorpe]

On Wed, Nov 06, 2013 at 07:03:08PM +0100, Thomas Petazzoni wrote:
> Dear Jason Gunthorpe,
> 
> (Adding a bunch of mvebu people in Cc)
> 
> On Wed, 6 Nov 2013 10:36:49 -0700, Jason Gunthorpe wrote:
> 
> > Thomas: There is one buglet here that I haven't had time to do
> > anything about. Notice the DT is listing the PEX memory window in its
> > ranges. I've done this for two reasons
> >  - The bootloader sets this address range up, so it is correct to
> >    include in the DT
> 
> The fact that the bootloader sets this MBus window is more-or-less
> irrelevant because when the mvebu-mbus driver is initialized, it
> completely clears *all* existing MBus windows:

Yes, but it is not irrelevent. In my case this same DT is supporting
3.10 and 3.12 kernels - 3.10 doesn't even have a MBUS driver or
dynamic PEX driver and it relies upon the ranges entry matching the
static bootloader configuration to work properly.

So the 'ranges matching bootloader' does have a use case.

> I indeed remember some objections, but I'm not sure what they were
> precisely. Maybe we didn't had a precise use case back at the time, to
> really make people objecting realize what the problem was?

That is about where I am at too..

> On the other hand, I think the of_*() API is quite limited when it
> comes to updating the DT. If I remember correctly, you can update some
> nodes, but you can never reclaim the memory that was used for the
> previous value of the node. So any change to the in-memory DT
> representation is basically a memory leak for the entire lifetime of
> the system (of course, I might be wrong on this, I haven't dived into
> all the hardcore details of DT manipulation in the kernel).

I'm not clear on that either, but it seems plausible..

> I'm not sure what would be the alternate mechanism to hook into the
> address translation. of_translate_one(), where all the translation
> through ranges takes place is really tied to the DT only, adding
> another mechanism to hook some custom address translation in there
> seems a bit weird, no?

I agree, some kind of callback scheme would really be needed.. Sort of
like the xlate callback we have for IRQs?

So the mbus would register an address xlate for its node that is
called instead of ranges parsing. For the example in my last message
the FPGA driver would register an xlate that made addresses relative
to its own BAR0 address.

> > Unfortunately if you add the ranges then the mbus driver throws a
> > warning that it is trying to overwrite existing windows, but otherwise
> > things work OK.
> 
> Yes, because at boot time the mvebu-mbus driver will set up windows for
> the statically defined ranges (the one you've written explicitly in the
> DT), and then later one when the PCIe driver will initialize, it will
> enumerate devices, realize that it needs a PCIe memory window, and ask
> the mvebu-mbus driver to create, which will fail because an overlapping
> window already exists.

Exactly.

> However, it just works by pure luck: nothing guarantees you that the
> PCIe 0 memory window will start at 0xe0000000. Of course, since you

Right, the general case is totally borked here - the dynamic changes
to the address map by MBUS and PCI are not being reflected to the
of_address machinery, be they from MBUS or from PCI BAR assignment.

However, address assignment is very predicatble, so if you have a
constrained system it can work.

In the normal DT use case (eg on a SPARC or something) the DT itself
would be self-consistent with the addresses assigned from the firmware
and the PCI machinery should try to respect the bootloader addresses
if they are workable during address assignment.

So it isn't as dire as 'pure luck' :)

Jason
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: address translation for PCIe-to-localbus bridge

[Thomas Petazzoni]

Dear Jason Gunthorpe,

On Wed, 6 Nov 2013 11:24:57 -0700, Jason Gunthorpe wrote:

> > The fact that the bootloader sets this MBus window is more-or-less
> > irrelevant because when the mvebu-mbus driver is initialized, it
> > completely clears *all* existing MBus windows:
> 
> Yes, but it is not irrelevent. In my case this same DT is supporting
> 3.10 and 3.12 kernels - 3.10 doesn't even have a MBUS driver or
> dynamic PEX driver and it relies upon the ranges entry matching the
> static bootloader configuration to work properly.

Ok, I understand.

> > I indeed remember some objections, but I'm not sure what they were
> > precisely. Maybe we didn't had a precise use case back at the time, to
> > really make people objecting realize what the problem was?
> 
> That is about where I am at too..

So maybe we should revive this direction then?

> > On the other hand, I think the of_*() API is quite limited when it
> > comes to updating the DT. If I remember correctly, you can update some
> > nodes, but you can never reclaim the memory that was used for the
> > previous value of the node. So any change to the in-memory DT
> > representation is basically a memory leak for the entire lifetime of
> > the system (of course, I might be wrong on this, I haven't dived into
> > all the hardcore details of DT manipulation in the kernel).
> 
> I'm not clear on that either, but it seems plausible..
> 
> > I'm not sure what would be the alternate mechanism to hook into the
> > address translation. of_translate_one(), where all the translation
> > through ranges takes place is really tied to the DT only, adding
> > another mechanism to hook some custom address translation in there
> > seems a bit weird, no?
> 
> I agree, some kind of callback scheme would really be needed.. Sort of
> like the xlate callback we have for IRQs?
> 
> So the mbus would register an address xlate for its node that is
> called instead of ranges parsing. For the example in my last message
> the FPGA driver would register an xlate that made addresses relative
> to its own BAR0 address.

Interesting. I admit I never had a look how the IRQ ->xlate() stuff was
hooked into the OF code, maybe it's a good starting point. This
discussion is on the DT mailing list, so maybe some DT person will
agree to let us know what they think the good approach is?

> > However, it just works by pure luck: nothing guarantees you that the
> > PCIe 0 memory window will start at 0xe0000000. Of course, since you
> 
> Right, the general case is totally borked here - the dynamic changes
> to the address map by MBUS and PCI are not being reflected to the
> of_address machinery, be they from MBUS or from PCI BAR assignment.

Right.

> However, address assignment is very predicatble, so if you have a
> constrained system it can work.

Indeed.

> In the normal DT use case (eg on a SPARC or something) the DT itself
> would be self-consistent with the addresses assigned from the firmware
> and the PCI machinery should try to respect the bootloader addresses
> if they are workable during address assignment.

So ARM isn't a "normal DT use case" ? :-)

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: address translation for PCIe-to-localbus bridge

[Grant Likely]

On Wed, 6 Nov 2013 11:24:57 -0700, Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
> On Wed, Nov 06, 2013 at 07:03:08PM +0100, Thomas Petazzoni wrote:
> > Dear Jason Gunthorpe,
> > 
> > (Adding a bunch of mvebu people in Cc)
> > 
> > On Wed, 6 Nov 2013 10:36:49 -0700, Jason Gunthorpe wrote:
> > 
> > > Thomas: There is one buglet here that I haven't had time to do
> > > anything about. Notice the DT is listing the PEX memory window in its
> > > ranges. I've done this for two reasons
> > >  - The bootloader sets this address range up, so it is correct to
> > >    include in the DT
> > 
> > The fact that the bootloader sets this MBus window is more-or-less
> > irrelevant because when the mvebu-mbus driver is initialized, it
> > completely clears *all* existing MBus windows:
> 
> Yes, but it is not irrelevent. In my case this same DT is supporting
> 3.10 and 3.12 kernels - 3.10 doesn't even have a MBUS driver or
> dynamic PEX driver and it relies upon the ranges entry matching the
> static bootloader configuration to work properly.
> 
> So the 'ranges matching bootloader' does have a use case.
> 
> > I indeed remember some objections, but I'm not sure what they were
> > precisely. Maybe we didn't had a precise use case back at the time, to
> > really make people objecting realize what the problem was?
> 
> That is about where I am at too..
> 
> > On the other hand, I think the of_*() API is quite limited when it
> > comes to updating the DT. If I remember correctly, you can update some
> > nodes, but you can never reclaim the memory that was used for the
> > previous value of the node. So any change to the in-memory DT
> > representation is basically a memory leak for the entire lifetime of
> > the system (of course, I might be wrong on this, I haven't dived into
> > all the hardcore details of DT manipulation in the kernel).
> 
> I'm not clear on that either, but it seems plausible..
> 
> > I'm not sure what would be the alternate mechanism to hook into the
> > address translation. of_translate_one(), where all the translation
> > through ranges takes place is really tied to the DT only, adding
> > another mechanism to hook some custom address translation in there
> > seems a bit weird, no?
> 
> I agree, some kind of callback scheme would really be needed.. Sort of
> like the xlate callback we have for IRQs?
> 
> So the mbus would register an address xlate for its node that is
> called instead of ranges parsing. For the example in my last message
> the FPGA driver would register an xlate that made addresses relative
> to its own BAR0 address.

There are already bus-specific transations available. Take a look at
struct of_bus in drivers/of/address.c

g.

> > However, it just works by pure luck: nothing guarantees you that the
> > PCIe 0 memory window will start at 0xe0000000. Of course, since you
> 
> Right, the general case is totally borked here - the dynamic changes
> to the address map by MBUS and PCI are not being reflected to the
> of_address machinery, be they from MBUS or from PCI BAR assignment.
> 
> However, address assignment is very predicatble, so if you have a
> constrained system it can work.

Yes, the problem is that the DT probably can't have an accurate set of
ranges for the devices under the PCIe bus since they are dynamically
assigned. It would be really nice to wire this up properly so that the
DT states the layout of stuff behind the bar, but leaves it to the OS to
assign PCI regions.

However, if firmware enumarates PCI and wants that to be passed through
to the OS, then the DT really should be updated... even in that case
though Linux should be able to pick up the PCI settings from the BAR
registers and use those for dynamics translation.

g.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: address translation for PCIe-to-localbus bridge

[Thomas Petazzoni]

Dear Grant Likely,

On Mon, 11 Nov 2013 15:50:50 +0000, Grant Likely wrote:

> > So the mbus would register an address xlate for its node that is
> > called instead of ranges parsing. For the example in my last message
> > the FPGA driver would register an xlate that made addresses relative
> > to its own BAR0 address.
> 
> There are already bus-specific transations available. Take a look at
> struct of_bus in drivers/of/address.c

Hum, right, but unless I'm wrong the of_busses[] array of struct of_bus
is fixed in drivers/of/address.c, and as it is, there is no way for a
specific bus driver to provide its own struct of_bus. So that would
need to be extended, right?

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: address translation for PCIe-to-localbus bridge

[Gerlando Falauto]

On 11/12/2013 08:05 AM, Thomas Petazzoni wrote:
> Dear Grant Likely,
>
> On Mon, 11 Nov 2013 15:50:50 +0000, Grant Likely wrote:
>
>>> So the mbus would register an address xlate for its node that is
>>> called instead of ranges parsing. For the example in my last message
>>> the FPGA driver would register an xlate that made addresses relative
>>> to its own BAR0 address.
>>
>> There are already bus-specific transations available. Take a look at
>> struct of_bus in drivers/of/address.c
>
> Hum, right, but unless I'm wrong the of_busses[] array of struct of_bus
> is fixed in drivers/of/address.c, and as it is, there is no way for a
> specific bus driver to provide its own struct of_bus.

That was exactly my understanding as well, and that's where I was 
expecting some trickery to happen.

> So that would need to be extended, right?

The other approach I was foreseeing was to implement a way of 
dynamically updating the DT when the PCI subsystem enumerates the 
devices and assigns memory areas (namely, I would expect the ranges 
property to reflect that). But this would imply a standard way of 
defining the ranges property (I would expect something like <bar# 
start_addr length>, with an arguable number of cells). And I could
not find any such definition in the PCI bus binding document, so I'm 
probably completely off-track here. Aren't I?

After all, we should expect a driver to behave (and expect) the same of 
the DT, regardless of whether enumeration was performed by firmware or
by the OS itself. Or am I wrong on this too?

Thanks guys!
Gerlando
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: address translation for PCIe-to-localbus bridge

[Thomas Petazzoni]

Dear Gerlando Falauto,

On Tue, 12 Nov 2013 09:11:33 +0100, Gerlando Falauto wrote:

> > Hum, right, but unless I'm wrong the of_busses[] array of struct of_bus
> > is fixed in drivers/of/address.c, and as it is, there is no way for a
> > specific bus driver to provide its own struct of_bus.
> 
> That was exactly my understanding as well, and that's where I was 
> expecting some trickery to happen.
> 
> > So that would need to be extended, right?
> 
> The other approach I was foreseeing was to implement a way of 
> dynamically updating the DT when the PCI subsystem enumerates the 
> devices and assigns memory areas (namely, I would expect the ranges 
> property to reflect that). But this would imply a standard way of 
> defining the ranges property (I would expect something like <bar# 
> start_addr length>, with an arguable number of cells). And I could
> not find any such definition in the PCI bus binding document, so I'm 
> probably completely off-track here. Aren't I?
> 
> After all, we should expect a driver to behave (and expect) the same of 
> the DT, regardless of whether enumeration was performed by firmware or
> by the OS itself. Or am I wrong on this too?

Well, in the context of the mvebu platforms (including Kirkwood), the
problem is not so much the ranges in the pcie-controller node, but the
ranges in the main soc { ... } node which encloses the description of
the MBus. It is those ranges that need to be updated when a new window
is created, or a window is removed. So the problem is not PCI related,
but MBus related in this case, no?

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: address translation for PCIe-to-localbus bridge

[Gerlando Falauto]

Hi,
On 11/12/2013 09:16 AM, Thomas Petazzoni wrote:
> Dear Gerlando Falauto,
>
> On Tue, 12 Nov 2013 09:11:33 +0100, Gerlando Falauto wrote:
>
>>> Hum, right, but unless I'm wrong the of_busses[] array of struct of_bus
>>> is fixed in drivers/of/address.c, and as it is, there is no way for a
>>> specific bus driver to provide its own struct of_bus.
>>
>> That was exactly my understanding as well, and that's where I was
>> expecting some trickery to happen.
>>
>>> So that would need to be extended, right?
>>
>> The other approach I was foreseeing was to implement a way of
>> dynamically updating the DT when the PCI subsystem enumerates the
>> devices and assigns memory areas (namely, I would expect the ranges
>> property to reflect that). But this would imply a standard way of
>> defining the ranges property (I would expect something like <bar#
>> start_addr length>, with an arguable number of cells). And I could
>> not find any such definition in the PCI bus binding document, so I'm
>> probably completely off-track here. Aren't I?
>>
>> After all, we should expect a driver to behave (and expect) the same of
>> the DT, regardless of whether enumeration was performed by firmware or
>> by the OS itself. Or am I wrong on this too?
>
> Well, in the context of the mvebu platforms (including Kirkwood), the
> problem is not so much the ranges in the pcie-controller node, but the
> ranges in the main soc { ... } node which encloses the description of
> the MBus. It is those ranges that need to be updated when a new window
> is created, or a window is removed. So the problem is not PCI related,
> but MBus related in this case, no?

I was actually referring to ranges within the PCI *device* (~leaf node).
So not thinking about mvebu, but rather about general PCI devices.

I see drivers/of/address.c implements some:

extern const __be32 *of_get_pci_address(struct device_node *dev, int bar_no,
			       u64 *size, unsigned int *flags);
extern int of_pci_address_to_resource(struct device_node *dev, int bar,
				      struct resource *r);

Which are not hooked to any ranges (of_bus) mechanisms nor any DT-aware 
PCI device driver and that's where I got completely lost.
But I'm probably looking at it from a wrong perspective.

Thanks,
Gerlando
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: address translation for PCIe-to-localbus bridge

[Grant Likely]

On Tue, 12 Nov 2013 09:11:33 +0100, Gerlando Falauto <gerlando.falauto-SkAbAL50j+5BDgjK7y7TUQ@public.gmane.org> wrote:
> On 11/12/2013 08:05 AM, Thomas Petazzoni wrote:
> > Dear Grant Likely,
> >
> > On Mon, 11 Nov 2013 15:50:50 +0000, Grant Likely wrote:
> >
> >>> So the mbus would register an address xlate for its node that is
> >>> called instead of ranges parsing. For the example in my last message
> >>> the FPGA driver would register an xlate that made addresses relative
> >>> to its own BAR0 address.
> >>
> >> There are already bus-specific transations available. Take a look at
> >> struct of_bus in drivers/of/address.c
> >
> > Hum, right, but unless I'm wrong the of_busses[] array of struct of_bus
> > is fixed in drivers/of/address.c, and as it is, there is no way for a
> > specific bus driver to provide its own struct of_bus.
> 
> That was exactly my understanding as well, and that's where I was 
> expecting some trickery to happen.
> 
> > So that would need to be extended, right?
> 
> The other approach I was foreseeing was to implement a way of 
> dynamically updating the DT when the PCI subsystem enumerates the 
> devices and assigns memory areas (namely, I would expect the ranges 
> property to reflect that). But this would imply a standard way of 
> defining the ranges property (I would expect something like <bar# 
> start_addr length>, with an arguable number of cells). And I could
> not find any such definition in the PCI bus binding document, so I'm 
> probably completely off-track here. Aren't I?

The ranges property is pretty well defined already. The format is
<parent-addr-specifier> <child-addr-specifier> <size-specifier>, and
then use assigned-addresses for each BAR to encode the address it is
assigned to in the PCI address space.

Ranges provides the windows into the parent bus, and assigned-address
is used to map the child into the parent. Using it should make the
of_bus translations work without changes.

Grep the source tree for assigned-addresses to get examples.

> After all, we should expect a driver to behave (and expect) the same of 
> the DT, regardless of whether enumeration was performed by firmware or
> by the OS itself. Or am I wrong on this too?

Yes, that is a reasonable expectation and it is a limitation of the
current implementation. We've got two choices here.  Modify the DT at
boot time, or extend the DT translation code to ask the PCI device for
BAR settings. I'm not going to make a call on which is better until I
see some prototype code.

g.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: address translation for PCIe-to-localbus bridge

[Grant Likely]

On Tue, 12 Nov 2013 08:05:12 +0100, Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Dear Grant Likely,
> 
> On Mon, 11 Nov 2013 15:50:50 +0000, Grant Likely wrote:
> 
> > > So the mbus would register an address xlate for its node that is
> > > called instead of ranges parsing. For the example in my last message
> > > the FPGA driver would register an xlate that made addresses relative
> > > to its own BAR0 address.
> > 
> > There are already bus-specific transations available. Take a look at
> > struct of_bus in drivers/of/address.c
> 
> Hum, right, but unless I'm wrong the of_busses[] array of struct of_bus
> is fixed in drivers/of/address.c, and as it is, there is no way for a
> specific bus driver to provide its own struct of_bus. So that would
> need to be extended, right?

Yes... or embed what you need in drivers/of/address.c for the time
being.

g.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: address translation for PCIe-to-localbus bridge

[Pantelis Antoniou]

Hi Thomas,

On Nov 6, 2013, at 8:03 PM, Thomas Petazzoni wrote:

> Dear Jason Gunthorpe,
> 

[snip]

> 
> Thomas
> -- 
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

I don't know if that's really revevant, but this sounds very similar to the way that the GPMC driver 
works (badly) on OMAPs.

In the old days we used to rely on the bootloader setting up things like chip selects, address windows
and what nots, and the kernel being very careful not messing with them.

So the drivers needed nothing more to work than being pointed the address windows that their device
happened to be configured by the bootloader.

Now that with the trend in bootloaders being really minimal, that task falls to the kernel.
Using board files one can get away with it using some per-board specific initialization.

On DT this fall downs completely, and you get abominations like the OMAP GPMC bindings...

Observe:

>                         status = "okay";
> 
>                         #address-cells = <2>;
>                         #size-cells = <1>;
> 
>                         pinctrl-names = "default";
>                         pinctrl-0 = <&gpmc_pins>;
> 
>                         /* chip select ranges */
>                         ranges = <0 0 0x08000000 0x10000000>,        /* bootloader has this enabled */
>                                  <1 0 0x18000000 0x08000000>,
>                                  <2 0 0x20000000 0x08000000>,
>                                  <3 0 0x28000000 0x08000000>,
>                                  <4 0 0x30000000 0x08000000>,
>                                  <5 0 0x38000000 0x04000000>,
>                                  <6 0 0x3c000000 0x04000000>;
> 
>                         /*
>                          * This is complete and utter cr*p
>                          * It doesn't belong here, but we don't
>                          * have a memory controller abstraction.
>                          * So we muddle along...
>                          */
>                         camera {
>                                 compatible = "cssp-camera";
>                                 status = "okay";
>                                 pinctrl-names = "default";
>                                 pinctrl-0 = <&cssp_camera_pins>;
> 
>                                 reg = <1 0 0x01000000>;                /* CS1 */
> 
>                                 bank-width = <2>;                /* GPMC_CONFIG1_DEVICESIZE(1) */
> 
>                                 gpmc,burst-read;                /* GPMC_CONFIG1_READMULTIPLE_SUPP */
>                                 gpmc,sync-read;                        /* GPMC_CONFIG1_READTYPE_SYNC */
>                                 gpmc,sync-write;                /* GPMC_CONFIG1_WRITETYPE_SYNC */
>                                 gpmc,clk-activation-ns = <20>;        /* GPMC_CONFIG1_CLKACTIVATIONTIME(2) */
>                                 gpmc,burst-length = <16>;        /* GPMC_CONFIG1_PAGE_LEN(2) */
>                                 gpmc,mux-add-data = <2>;        /* GPMC_CONFIG1_MUXTYPE(2) */
> 
>                                 gpmc,sync-clk-ps = <20000>;        /* CONFIG2 */
> 
>                                 gpmc,cs-on-ns = <1>;
>                                 gpmc,cs-rd-off-ns = <160>;
>                                 gpmc,cs-wr-off-ns = <310>;
> 
>                                 gpmc,adv-on-ns = <0>;                /* CONFIG3 */
>                                 gpmc,adv-rd-off-ns = <40>;
>                                 gpmc,adv-wr-off-ns = <40>;
> 
>                                 gpmc,we-on-ns = <60>;                /* CONFIG4 */
>                                 gpmc,we-off-ns = <310>;
>                                 gpmc,oe-on-ns = <60>;
>                                 gpmc,oe-off-ns = <160>;
> 
>                                 gpmc,page-burst-access-ns = <20>;        /* CONFIG 5 */
>                                 gpmc,access-ns = <140>;
>                                 gpmc,rd-cycle-ns = <160>;
>                                 gpmc,wr-cycle-ns = <310>;
> 
>                                 gpmc,wr-access-ns = <100>;                /* CONFIG 6 */
>                                 gpmc,wr-data-mux-bus-ns = <60>;
> 
>                                 gpmc,bus-turnaround-ns = <40>;                /* CONFIG6:3:0 = 4 */
>                                 gpmc,cycle2cycle-samecsen;                /* CONFIG6:7 = 1 */
>                                 gpmc,cycle2cycle-delay-ns = <40>;        /* CONFIG6:11:8 = 4 */
> 
>                                 /* not using dma engine yet, but we can get the channel number here */
>                                 dmas = <&edma 20>;
>                                 dma-names = "cssp";
> 
>                                 /* clocks */
>                                 cssp,camera-clk-name = "adjustable_clkout2_ck";
>                                 cssp,camera-clk-rate = <32000000>;
> 
>                                 /* reset */
>                                 reset-gpio = <&gpio1 4 0>;
> 
>                                 /* orientation; -> MT9T112_FLAG_VFLIP */
>                                 orientation-gpio = <&gpio1 30 0>;
> 
>                                 /* reset controller (for the black) */
>                                 reset = <&rstctl 0 0>;
>                                 reset-names = "eMMC_RSTn-CAM3";
> 
>                                 /* camera sensor */
>                                 cssp,sensor {
>                                         i2c-adapter = <&i2c2>;
> 
>                                         /* need it to stop the whinning */
>                                         #address-cells = <1>;
>                                         #size-cells = <0>;
> 
>                                         /* fake i2c device node */
>                                         m59t112 {
>                                                 compatible = "aptina,mt9t112";
>                                                 reg = <0x3c>;
> 
>                                                 /* m, n, p1-7 */
>                                                 flags = <0>;
>                                                 pll-divider = <24 1 0 7 0 10 14 7 0>;
>                                         };
>                                 };
> 
>                         };
> 
>    

This is a fragment from a camera instantiation that happens to be connected to the GPMC interface.

The GPMC driver has to be hacked to support _any_ possible device it might be connected, and
there is a abundance of GPMC configuration properties in an unrelated device driver.

I could be wrong and jumped in the wrong thread, but I believe what we really need is a generic GPMC
framework, similar to the way pinctrl works.

So instead of doing the above we could just add a property that contains a reference to the required GPMC
configuration.

i.e.


	gpmc {
		compatible = "ti,gpmc";

		camera_gpmc_config {

                         bank-width = <2>;                /* GPMC_CONFIG1_DEVICESIZE(1) */

                         gpmc,burst-read;                /* GPMC_CONFIG1_READMULTIPLE_SUPP */
                         gpmc,sync-read;                        /* GPMC_CONFIG1_READTYPE_SYNC */
                         gpmc,sync-write;                /* GPMC_CONFIG1_WRITETYPE_SYNC */
                         gpmc,clk-activation-ns = <20>;        /* GPMC_CONFIG1_CLKACTIVATIONTIME(2) */
                         gpmc,burst-length = <16>;        /* GPMC_CONFIG1_PAGE_LEN(2) */
                         gpmc,mux-add-data = <2>;        /* GPMC_CONFIG1_MUXTYPE(2) */
		};
	}
	
        camera {
                compatible = "cssp-camera";
                reg = <0 0x01000000>;                /* CS1 */
		gpmc-config = <&camera_gpmc_config 1>	/* CS1 */
 		...
	};

Regards

-- Pantelis
 

 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: address translation for PCIe-to-localbus bridge

[Jason Gunthorpe]

On Wed, Nov 06, 2013 at 08:33:39PM +0200, Pantelis Antoniou wrote:

> On DT this fall downs completely, and you get abominations like the
> OMAP GPMC bindings...

This looks similar to Ezequiel's expansion bus bindings for MVEBU.

Maybe you can clarify the issue you see?

The general pattern I expect is something like this:

  some_bus_bridge {
     compatible = 'bridge bus driver';
     
     ranges <[CS#] 0  [CS window base]  [size]>;

     chip-select,1 {
       < All Parameters to configure timing, etc, for this
         chip select>
       ranges = <0  1 0  [size]>;

       my device {
           compatble = 'camera'
           reg = <0 [size]>
       }
     }

The way it works is the DT machinery instantiates a some_bus_bridge.

The bridge driver goes through and parses the ranges, it sets up the
address mappings as necessary to create a window for each CS. If this
address assignment is dynamic then it needs to make the 'ranges'
correct. (MVEBU land does this in the mbus driver)

The bridge driver parses each child and configures the bus CS timing
parameters. (this is done in the external memory driver, IIRC)

The bridge driver constructs a struct device for each child in every
chip-select (the 'my device' node). Normal OF address translation
makes this work fine.

Your camera driver attaches to the 'my device' node, not to the chip
select node. The chip select node is used only by the bus bridge
driver to setup that specific chip select.

The example you have looks close to that except for a few details:
 
> Observe:
> 
> >                         status = "okay";
> > 
> >                         #address-cells = <2>;
> >                         #size-cells = <1>;
> > 
> >                         pinctrl-names = "default";
> >                         pinctrl-0 = <&gpmc_pins>;
> > 
> >                         /* chip select ranges */
> >                         ranges = <0 0 0x08000000 0x10000000>,        /* bootloader has this enabled */
> >                                  <1 0 0x18000000 0x08000000>,
> >                                  <2 0 0x20000000 0x08000000>,
> >                                  <3 0 0x28000000 0x08000000>,
> >                                  <4 0 0x30000000 0x08000000>,
> >                                  <5 0 0x38000000 0x04000000>,
> >                                  <6 0 0x3c000000 0x04000000>;

OK, seems reasonable, setting up chip select windows, giving them base
addresess and sizes.

> >                         camera {
> >                                 compatible = "cssp-camera";
> >                                 status = "okay";
> >                                 pinctrl-names = "default";
> >                                 pinctrl-0 = <&cssp_camera_pins>;
> > 
> >                                 reg = <1 0 0x01000000>;       /* CS1 */

Here we want to hoist bus specific stuff out of 'camera' and into
'cs1@', so:

No reg at this point.

> >                                 bank-width = <2>;                /* GPMC_CONFIG1_DEVICESIZE(1) */
> >                                 gpmc,burst-read;                /* GPMC_CONFIG1_READMULTIPLE_SUPP */
[...]

Timing parameters seem reasonable.

> > 
> >                                 /* not using dma engine yet, but we can get the channel number here */
> >                                 dmas = <&edma 20>;
> >                                 dma-names = "cssp";

DMA's/gpios, etc probably belong in the child. Maybe clocks belong in
the CS? Depends on the bridge driver..

> >                                 /* clocks */
> >                                 cssp,camera-clk-name = "adjustable_clkout2_ck";
> >                                 cssp,camera-clk-rate = <32000000>;
> > 
> >                                 /* reset */
> >                                 reset-gpio = <&gpio1 4 0>;
> > 
> >                                 /* orientation; -> MT9T112_FLAG_VFLIP */
> >                                 orientation-gpio = <&gpio1 30 0>;
> > 
> >                                 /* reset controller (for the black) */
> >                                 reset = <&rstctl 0 0>;
> >                                 reset-names = "eMMC_RSTn-CAM3";

Missing:

ranges = <0  1 0  0x01000000>;

Missing:

 camera@0 {
    reg = <0 0x01000000>;

Put DMA's/etc here.

> >                                 /* camera sensor */
> >                                 cssp,sensor {
> >                                         i2c-adapter = <&i2c2>;

Yikes!

If there is an i2c component then a phandle to the component itself is
way better:

    i2c-component = <&i2c_camera>

i2c2: ...
{ 
         i2c_camera: m59t112 {
             compatible = "aptina,mt9t112";
             reg = <0x3c>;
         }
}

Sprinkle address-cells/size-cells as necessary.

> 	gpmc {
> 		compatible = "ti,gpmc";
> 
> 		camera_gpmc_config {
> 
>                          bank-width = <2>;                /* GPMC_CONFIG1_DEVICESIZE(1) */
> 
>                          gpmc,burst-read;                /* GPMC_CONFIG1_READMULTIPLE_SUPP */
>                          gpmc,sync-read;                        /* GPMC_CONFIG1_READTYPE_SYNC */
>                          gpmc,sync-write;                /* GPMC_CONFIG1_WRITETYPE_SYNC */
>                          gpmc,clk-activation-ns = <20>;        /* GPMC_CONFIG1_CLKACTIVATIONTIME(2) */
>                          gpmc,burst-length = <16>;        /* GPMC_CONFIG1_PAGE_LEN(2) */
>                          gpmc,mux-add-data = <2>;        /* GPMC_CONFIG1_MUXTYPE(2) */
> 		};
> 	}

Similar idea, but you have thrown away the nesting.

DT should reflect the address translation hierarchy of the system, so
'ti,gpmc' does address translation, the things it translates for
should be its children.

Jason
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: address translation for PCIe-to-localbus bridge

[Pantelis Antoniou]

Hi Jason,


On Nov 6, 2013, at 8:56 PM, Jason Gunthorpe wrote:

> On Wed, Nov 06, 2013 at 08:33:39PM +0200, Pantelis Antoniou wrote:
> 
>> On DT this fall downs completely, and you get abominations like the
>> OMAP GPMC bindings...
> 
> This looks similar to Ezequiel's expansion bus bindings for MVEBU.
> 
> Maybe you can clarify the issue you see?
> 
> The general pattern I expect is something like this:
> 
>  some_bus_bridge {
>     compatible = 'bridge bus driver';
> 
>     ranges <[CS#] 0  [CS window base]  [size]>;
> 
>     chip-select,1 {
>       < All Parameters to configure timing, etc, for this
>         chip select>
>       ranges = <0  1 0  [size]>;
> 
>       my device {
>           compatble = 'camera'
>           reg = <0 [size]>
>       }
>     }
> 
> The way it works is the DT machinery instantiates a some_bus_bridge.
> 
> The bridge driver goes through and parses the ranges, it sets up the
> address mappings as necessary to create a window for each CS. If this
> address assignment is dynamic then it needs to make the 'ranges'
> correct. (MVEBU land does this in the mbus driver)
> 
> The bridge driver parses each child and configures the bus CS timing
> parameters. (this is done in the external memory driver, IIRC)
> 
> The bridge driver constructs a struct device for each child in every
> chip-select (the 'my device' node). Normal OF address translation
> makes this work fine.
> 
> Your camera driver attaches to the 'my device' node, not to the chip
> select node. The chip select node is used only by the bus bridge
> driver to setup that specific chip select.
> 

No. I get the point about the bridge driver, but that's not what I want.

I agree that the bridge driver configures the chip selects, address ranges
and timings, but it shouldn't do that at the time the bridge driver is
instantiated. 

That would mean that all configuration is fixed at boot time.


> The example you have looks close to that except for a few details:
> 
>> Observe:
>> 
>>>                        status = "okay";
>>> 
>>>                        #address-cells = <2>;
>>>                        #size-cells = <1>;
>>> 
>>>                        pinctrl-names = "default";
>>>                        pinctrl-0 = <&gpmc_pins>;
>>> 
>>>                        /* chip select ranges */
>>>                        ranges = <0 0 0x08000000 0x10000000>,        /* bootloader has this enabled */
>>>                                 <1 0 0x18000000 0x08000000>,
>>>                                 <2 0 0x20000000 0x08000000>,
>>>                                 <3 0 0x28000000 0x08000000>,
>>>                                 <4 0 0x30000000 0x08000000>,
>>>                                 <5 0 0x38000000 0x04000000>,
>>>                                 <6 0 0x3c000000 0x04000000>;
> 
> OK, seems reasonable, setting up chip select windows, giving them base
> addresess and sizes.
> 
>>>                        camera {
>>>                                compatible = "cssp-camera";
>>>                                status = "okay";
>>>                                pinctrl-names = "default";
>>>                                pinctrl-0 = <&cssp_camera_pins>;
>>> 
>>>                                reg = <1 0 0x01000000>;       /* CS1 */
> 
> Here we want to hoist bus specific stuff out of 'camera' and into
> 'cs1@', so:
> 
> No reg at this point.
> 
>>>                                bank-width = <2>;                /* GPMC_CONFIG1_DEVICESIZE(1) */
>>>                                gpmc,burst-read;                /* GPMC_CONFIG1_READMULTIPLE_SUPP */
> [...]
> 
> Timing parameters seem reasonable.
> 
>>> 
>>>                                /* not using dma engine yet, but we can get the channel number here */
>>>                                dmas = <&edma 20>;
>>>                                dma-names = "cssp";
> 
> DMA's/gpios, etc probably belong in the child. Maybe clocks belong in
> the CS? Depends on the bridge driver..
> 
>>>                                /* clocks */
>>>                                cssp,camera-clk-name = "adjustable_clkout2_ck";
>>>                                cssp,camera-clk-rate = <32000000>;
>>> 
>>>                                /* reset */
>>>                                reset-gpio = <&gpio1 4 0>;
>>> 
>>>                                /* orientation; -> MT9T112_FLAG_VFLIP */
>>>                                orientation-gpio = <&gpio1 30 0>;
>>> 
>>>                                /* reset controller (for the black) */
>>>                                reset = <&rstctl 0 0>;
>>>                                reset-names = "eMMC_RSTn-CAM3";
> 
> Missing:
> 
> ranges = <0  1 0  0x01000000>;
> 
> Missing:
> 
> camera@0 {
>    reg = <0 0x01000000>;
> 
> Put DMA's/etc here.
> 
>>>                                /* camera sensor */
>>>                                cssp,sensor {
>>>                                        i2c-adapter = <&i2c2>;
> 
> Yikes!
> 
> If there is an i2c component then a phandle to the component itself is
> way better:
> 
>    i2c-component = <&i2c_camera>
> 
> i2c2: ...
> { 
>         i2c_camera: m59t112 {
>             compatible = "aptina,mt9t112";
>             reg = <0x3c>;
>         }
> }
> 
> Sprinkle address-cells/size-cells as necessary.
> 
>> 	gpmc {
>> 		compatible = "ti,gpmc";
>> 
>> 		camera_gpmc_config {
>> 
>>                         bank-width = <2>;                /* GPMC_CONFIG1_DEVICESIZE(1) */
>> 
>>                         gpmc,burst-read;                /* GPMC_CONFIG1_READMULTIPLE_SUPP */
>>                         gpmc,sync-read;                        /* GPMC_CONFIG1_READTYPE_SYNC */
>>                         gpmc,sync-write;                /* GPMC_CONFIG1_WRITETYPE_SYNC */
>>                         gpmc,clk-activation-ns = <20>;        /* GPMC_CONFIG1_CLKACTIVATIONTIME(2) */
>>                         gpmc,burst-length = <16>;        /* GPMC_CONFIG1_PAGE_LEN(2) */
>>                         gpmc,mux-add-data = <2>;        /* GPMC_CONFIG1_MUXTYPE(2) */
>> 		};
>> 	}
> 
> Similar idea, but you have thrown away the nesting.
> 

The bindings presented was just for illustration, I just wanted to present the idea :)

> DT should reflect the address translation hierarchy of the system, so
> 'ti,gpmc' does address translation, the things it translates for
> should be its children.
> 

That's where we disagree. I don't want the drivers to be children of the bridge.

All the drivers that are controlled by a chip select and/or bus access method are
written without ever dealing with the bridge; they just expect the configuration to be
valid and need to only know the address range they reside.

As you pointed the i2c component, which was a child of the device node, it is better to become a 
phandle reference.

That's what I'm proposing for the bridge configuration as well, so that the configuration is applied
only when a device requests it. This is important for hot-plugging, and or when using stuff like
device overlays.

> Jason

Regards

-- Pantelis

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: address translation for PCIe-to-localbus bridge

[Jason Gunthorpe]

On Wed, Nov 06, 2013 at 09:16:13PM +0200, Pantelis Antoniou wrote:
> > Your camera driver attaches to the 'my device' node, not to the chip
> > select node. The chip select node is used only by the bus bridge
> > driver to setup that specific chip select.
> 
> No. I get the point about the bridge driver, but that's not what I want.
> 
> I agree that the bridge driver configures the chip selects, address ranges
> and timings, but it shouldn't do that at the time the bridge driver is
> instantiated. 

But that is normal Linux behavior. The bus driver controls the bus,
and it takes control when it is instantiated.

> That would mean that all configuration is fixed at boot time.

No, it means at boot time the configuration data in the DT is
programmed into the hardware at boot time.

If you hotplug the environment later during run time then you have to
notify the bridge driver and have it make whatever changes. This is no
different than PCI which requires notifying the PCI layer to have it
reprogram bridge devices.

This is why the nodes must be under the bridge, only the bridge knows
when it is safe to create struct device's for the children.

> All the drivers that are controlled by a chip select and/or bus
> access method are written without ever dealing with the bridge; they
> just expect the configuration to be valid and need to only know the
> address range they reside.

That is exactly what my example is showing. The driver has no idea
about the bridge. Ezequiel's MVEBU version does this today, IIRC, he
has the standard every day memory mapped NOR driver working as a node
under the bridge, with full configurability, and no changes to the NOR
driver.

> As you pointed the i2c component, which was a child of the device
> node, it is better to become a phandle reference.

Again, the DT parent/child reflects bus ownership/address
translation.

It is incorrect to put an i2c device below any node other than the i2c
bus it is attached to.

> That's what I'm proposing for the bridge configuration as well, so
> that the configuration is applied only when a device requests
> it. This is important for hot-plugging, and or when using stuff like
> device overlays.

If there is no children to a chip select then the bridge can leave it
turned off and keep address space deallocated.

The bridge driver simply has to be involved in the hot plug process,
which is again normal in Linux.

Device tree overlays provide a mechanism to create new chip select
nodes under the bridge, I don't see the problem you do :)

Jason
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: address translation for PCIe-to-localbus bridge

[Gerlando Falauto]

Hi Jason,

thank you for your feeback.

On 11/06/2013 06:36 PM, Jason Gunthorpe wrote:
> On Wed, Nov 06, 2013 at 11:27:15AM +0100, Gerlando Falauto wrote:
>> Hi everyone,
>>
>> I am currently trying to describe an external device within a device
>> tree in a Kirkwood design.
>
> Here is what works for me:
>
>          mbus {
>                  compatible = "marvell,kirkwood-mbus", "simple-bus";
>                  ranges = <MBUS_ID(0x04, 0xe8) 0 0xe0000000 0x8000000  /* PEX 0 MEM */
>                            MBUS_ID(0xf0, 0x01) 0 0xf1000000 0x100000     /* internal-regs */
>                            MBUS_ID(0x01, 0x2f) 0 0xf4000000 0x10000      /* nand flash */
>                            MBUS_ID(0x01, 0x1d) 0 0xffe00000 0x10000      /* boot rom */
>                            MBUS_ID(0x01, 0x1e) 0 0xfff00000 0x10000      /* spi */
>                            >;
>                  pcie-mem-aperture = <0xe0000000 0x10000000>; /* 256 MiB memory space */
>                  pex@e0000000 {
>                          compatible = "marvell,kirkwood-pcie";
>                          device_type = "pci";
>                          ranges = <0x82000000 0 0x40000 MBUS_ID(0xf0, 0x01) 0x40000 0 0x00002000 /* Controller regs */
>                                    0x82000000 1 0       MBUS_ID(0x04, 0xe8) 0 1 0 /* Port 0.0 MEM */
>                                    >;
>
>                          bus-range = <0x0 0xFF>;
>                          pcie@1,0 {
>                                  device_type = "pci";
>                                  assigned-addresses = <0x82000800 0 0x40000 0 0x2000>;
>                                  reg = <0x0800 0 0 0 0>;
>                                  ranges = <0x82000000 0 0 0x82000000 0x1 0 1 0
>                                            0x81000000 0 0 0x81000000 0x1 0 1 0>;
>                                  fpga@0 {
>                                          reg = <0x8 0 0  0 0>;
>                                          ranges = <0x00000000  0x82000000 0x00000000 0x00000000  0x8000000>;
>                                          gpio3: fpga_gpio@8 {
>                                                  #gpio-cells = <2>;
>                                                  compatible = "linux,basic-mmio-gpio";
>                                                  gpio-controller;
>                                                  reg-names = "dat", "set", "dirin";
>                                                  reg = <0x8 4>,
>                                                        <0xc 4>,
>                                                        <0x10 4>;
>                                          };
>
> (some hopefully irrelevant items omitted for brevity)

So let me get this straight.
First of all (though slightly unrelated), I looked at 
drivers/gpio/gpio-generic.c and found no reference whatsoever to the 
of_* infrastructure.
So I realized of_device_alloc() populates the resource table of the 
platform_device automatically -- I wasn't aware of that.

Second of all, if I understand it correctly (guessing the values for 
#size-cells and #address-cells), your translation scheme works as 
follows (let's say for the first register 0x8 of gpio3):

gpio3 (0x8)
-> range 0 of fpga@0       ==> 0x00000000 0x82000000 0x00000000
-> range 0 of pcie@1,0     ==> 0x82000000 0x1 0
-> range 1 of pex@e0000000 ==> MBUS_ID(0x04, 0xe8) 0
-> range 0 of mbus         ==> 0xe0000000

so you end up accessing @0xe0000008, is that right?

Looks like it ends up at the beginning of the memory region for PCIe, 
and that's no wonder since you only have a single device with a single 
BAR... right?

So suppose you also had a bigger BAR1 which would then shift your GPIO 
block at @0xe8000000.
I believe we've established (in other followup mails) it would be nice 
to have a dynamic translation mechanism, which is not in place yet.
Until we get that figured out, where would you hardcode such offset then?
How would you also deal with a second (let's say identical) device on BAR1?
I guess I could live with hardcoded values in the DT, as long as they're 
easy to spot and there's only one per BAR/device.
Then it's easy to do a comparison with whatever gets assigned during 
probing.

(Apologies in advance if I'm talking nonsense, but I only see a bunch of 
numbers in the above tree, I'm having a really hard time figuring out 
how this whole translation works...)

> I use code like this in the FPGA PCI driver to load the DT nodes:
>
>          struct of_device_id match_table[2] = {};
>          struct device_node *child;
>          int rc = 0;
>
>          for_each_child_of_node(root, child) {
>                  /* Can't just create a single device.. */
>                  strlcpy(match_table[0].name, child->name,
>                          sizeof(match_table[0].name));
>                  strlcpy(match_table[0].type, child->type,
>                          sizeof(match_table[0].type));
>                  rc = of_platform_bus_probe(child, match_table,
>                                             parent);
>                  if (rc)
>                          break;
>          }
> (root would be the of_node of the FPGA)

Stupid question... why not the following:

rc = of_platform_populate(root, NULL, NULL, parent);

I've been using this in several places, though I've always wondered 
whether there was indeed something wrong it...

[...]
>> But I found no way to describe which BAR it should refer to, for instance.
>>
>> Perhaps the "rrrrrrrr" part of phys.hi, using BAR0=0x10, BAR1=0x14,
>> and so on?  Or else I should define a new instance of of bus (i.e.
>> "pci_lbus_bridge") and invent yet another address encoding syntax?
>
> I do feel there is some missing dynamic elements in this scheme, eg
> the ranges value at the FPGA node should be dynamic based on the BAR
> offset, but that is a generality that isn't important if you have only
> one PCI device.

Right, which is my case... except, I have several BARs within it, which 
does complicate things quite a bit, I'm afraid.

Thanks again Jason!
Gerlando
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: address translation for PCIe-to-localbus bridge

[Jason Gunthorpe]

On Wed, Nov 06, 2013 at 08:38:33PM +0100, Gerlando Falauto wrote:

> So let me get this straight.
> First of all (though slightly unrelated), I looked at
> drivers/gpio/gpio-generic.c and found no reference whatsoever to the
> of_* infrastructure.

Yes, I have a patch to enable that:
https://github.com/jgunthorpe/linux/commit/da2881f55c094338f3b76617962e6af6c15f9cb9

People didn't like the DT binding, so I'm just sitting on it.

> So I realized of_device_alloc() populates the resource table of the
> platform_device automatically -- I wasn't aware of that.

Yes, in most cases platform drivers should not call of functions to get
resources 

> Second of all, if I understand it correctly (guessing the values for
> #size-cells and #address-cells), your translation scheme works as
> follows (let's say for the first register 0x8 of gpio3):
> 
> gpio3 (0x8)
> -> range 0 of fpga@0       ==> 0x00000000 0x82000000 0x00000000
> -> range 0 of pcie@1,0     ==> 0x82000000 0x1 0
> -> range 1 of pex@e0000000 ==> MBUS_ID(0x04, 0xe8) 0
> -> range 0 of mbus         ==> 0xe0000000
> 
> so you end up accessing @0xe0000008, is that right?

Almost:
 -> reg 0x8 pf gpio3
 -> range 0 of fpga@0       ==> 0x82000000 0x00000000 0x00000008
 -> range 0 of pcie@1,0     ==> 0x82000000 0x1 8
 -> range 1 of pex@e0000000 ==> MBUS_ID(0x04, 0xe8) 8
 -> range 0 of mbus         ==> 0xe0000008

Each translation represents something concrete:
 0x8 -> 0x82000000 0x00000000 0x00000008
    This is the BAR 0 decoder of the PCI device
 0x82000000 0x00000000 0x00000008 -> 0x82000000 0x1 8
    This is the PCI bridge's memory window decoder
  0x82000000 0x1 8 -> MBUS_ID(0x04, 0xe8) 8
    This is the MBUS window associated with the PEX
 MBUS_ID(0x04, 0xe8) 8 -> 0xe0000008
    This is the physical CPU BUS address associated with the MBUS
    window

> Looks like it ends up at the beginning of the memory region for
> PCIe, and that's no wonder since you only have a single device with
> a single BAR... right?

The mapping of the FPGA bus into a BAR is done by a single ranges:

> >                                 fpga@0 {
> >                                         reg = <0x8 0 0  0 0>;
> >                                         ranges = <0x00000000  0x82000000 0x00000000 0x00000000  0x8000000>;
> >                                         gpio3: fpga_gpio@8 {

That ranges says 'put address 0 of the child bus at 0x82000000
0x00000000 0x00000000', which is the BAR 0 address, relative to the
bridge's memory window.

> So suppose you also had a bigger BAR1 which would then shift your
> GPIO block at @0xe8000000.
> Until we get that figured out, where would you hardcode such offset then?

Since your BAR layout of your device is fixed you can adjust the
single ranges:

  ranges = <0x00000000  0x82000000 0x00000000 0x08000000  0x8000000>;

It is possible as well to do this in code in the FPGA driver.

> How would you also deal with a second (let's say identical) device on BAR1?

  ranges = <0x00000000  0x82000000 0x00000000 0x08000000  0x8000000  //  BAR 0
            0x10000000  0x82000000 0x00000000 0x08000000  0x8000000> // BAR 1

Use reg <0x10000abc xxx> to refer to the 2nd BAR. There are other ways
to organize things.

> I guess I could live with hardcoded values in the DT, as long as
> they're easy to spot and there's only one per BAR/device.
> Then it's easy to do a comparison with whatever gets assigned during
> probing.

I'd see it as an interm step, pending on some kind of core support for
this sort of stuff.

> >I use code like this in the FPGA PCI driver to load the DT nodes:
> >
> >         struct of_device_id match_table[2] = {};
> >         struct device_node *child;
> >         int rc = 0;
> >
> >         for_each_child_of_node(root, child) {
> >                 /* Can't just create a single device.. */
> >                 strlcpy(match_table[0].name, child->name,
> >                         sizeof(match_table[0].name));
> >                 strlcpy(match_table[0].type, child->type,
> >                         sizeof(match_table[0].type));
> >                 rc = of_platform_bus_probe(child, match_table,
> >                                            parent);
> >                 if (rc)
> >                         break;
> >         }
> >(root would be the of_node of the FPGA)
> 
> Stupid question... why not the following:
> 
> rc = of_platform_populate(root, NULL, NULL, parent);

Yes, that probably works. My version of the above has an additional
bit of code that I removed which filters the children to
create. Basically FPGA version 1.0 has a different list of devices
than version 1.1, etc.

Jason
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: address translation for PCIe-to-localbus bridge

[Gerlando Falauto]

On 11/06/2013 09:07 PM, Jason Gunthorpe wrote:
> On Wed, Nov 06, 2013 at 08:38:33PM +0100, Gerlando Falauto wrote:
>
[...]
> Each translation represents something concrete:
>   0x8 -> 0x82000000 0x00000000 0x00000008
>      This is the BAR 0 decoder of the PCI device
>   0x82000000 0x00000000 0x00000008 -> 0x82000000 0x1 8
>      This is the PCI bridge's memory window decoder
>    0x82000000 0x1 8 -> MBUS_ID(0x04, 0xe8) 8
>      This is the MBUS window associated with the PEX
>   MBUS_ID(0x04, 0xe8) 8 -> 0xe0000008
>      This is the physical CPU BUS address associated with the MBUS
>      window

Uhm, OK, sounds good. Except you must know the whole mapping in advance. 
Fair enough.
Would that work without the whole mbus driver (I guess Ezequiel's)?
I'm dealing with 3.10.

Where does the 0x82000000 come from?
What about the 0x1 (second cell)?

>> Looks like it ends up at the beginning of the memory region for
>> PCIe, and that's no wonder since you only have a single device with
>> a single BAR... right?
>
> The mapping of the FPGA bus into a BAR is done by a single ranges:
>
>>>                                  fpga@0 {
>>>                                          reg = <0x8 0 0  0 0>;
>>>                                          ranges = <0x00000000  0x82000000 0x00000000 0x00000000  0x8000000>;
>>>                                          gpio3: fpga_gpio@8 {
>
> That ranges says 'put address 0 of the child bus at 0x82000000
> 0x00000000 0x00000000', which is the BAR 0 address, relative to the
> bridge's memory window.
>
>> So suppose you also had a bigger BAR1 which would then shift your
>> GPIO block at @0xe8000000.
>> Until we get that figured out, where would you hardcode such offset then?
>
> Since your BAR layout of your device is fixed you can adjust the
> single ranges:

By "single" you mean that it's just *one* range, right?

>
>    ranges = <0x00000000  0x82000000 0x00000000 0x08000000  0x8000000>;
>
> It is possible as well to do this in code in the FPGA driver.

Uhm... why? And how? You mean dynamically upgrading the ranges window?

>
>> How would you also deal with a second (let's say identical) device on BAR1?
>
>    ranges = <0x00000000  0x82000000 0x00000000 0x08000000  0x8000000  //  BAR 0
>              0x10000000  0x82000000 0x00000000 0x08000000  0x8000000> // BAR 1

Aren't in this case the two regions aliased? Shouldn't the fourth column 
on either row be 0x00000000? Like:

     ranges = <0x00000000  0x82000000 0x00000000 0x00000000  0x8000000 
//  BAR 0
               0x10000000  0x82000000 0x00000000 0x08000000  0x8000000> 
// BAR 1

> Use reg <0x10000abc xxx> to refer to the 2nd BAR. There are other ways
> to organize things.

Now I completely lost you. According to the above description (BTW, I 
like the double space to separate groups -- it makes this whole mess a 
little messy!) the child address should have a single cell.
Why use two then?

>> I guess I could live with hardcoded values in the DT, as long as
>> they're easy to spot and there's only one per BAR/device.
>> Then it's easy to do a comparison with whatever gets assigned during
>> probing.
>
> I'd see it as an interm step, pending on some kind of core support for
> this sort of stuff.

Right.

Sorry for being pedantic, but I'd really like to get this straight once 
and for all...

Thanks again,
Gerlando
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: address translation for PCIe-to-localbus bridge

[Jason Gunthorpe]

On Thu, Nov 07, 2013 at 10:07:01AM +0100, Gerlando Falauto wrote:
> On 11/06/2013 09:07 PM, Jason Gunthorpe wrote:
> >On Wed, Nov 06, 2013 at 08:38:33PM +0100, Gerlando Falauto wrote:
> >
> [...]
> >Each translation represents something concrete:
> >  0x8 -> 0x82000000 0x00000000 0x00000008
> >     This is the BAR 0 decoder of the PCI device
> >  0x82000000 0x00000000 0x00000008 -> 0x82000000 0x1 8
> >     This is the PCI bridge's memory window decoder
> >   0x82000000 0x1 8 -> MBUS_ID(0x04, 0xe8) 8
> >     This is the MBUS window associated with the PEX
> >  MBUS_ID(0x04, 0xe8) 8 -> 0xe0000008
> >     This is the physical CPU BUS address associated with the MBUS
> >     window
> 
> Uhm, OK, sounds good. Except you must know the whole mapping in
> advance. Fair enough.
> Would that work without the whole mbus driver (I guess Ezequiel's)?
> I'm dealing with 3.10.

Yes, I am using this in 3.10. On 3.10 the ranges at the mbus level has
to match the hardwired address mapping in Linux.

> Where does the 0x82000000 come from?
> What about the 0x1 (second cell)?

0x82000000 is part of the OF PCI spec, it indicates the address space
is memory (vs io, vs prefetchable memory, etc)

The 0x1 is an arbitary unique value that matches the next ranges. It
would probably have been clearer if it was MBUS_ID(0x04, 0xe8)
instead...

> >   ranges = <0x00000000  0x82000000 0x00000000 0x08000000  0x8000000>;
> >
> >It is possible as well to do this in code in the FPGA driver.
> 
> Uhm... why? And how? You mean dynamically upgrading the ranges window?

If you can't predict where your BAR will be assigned when you create
the DT then you could update the ranges value at runtime in your pci
driver before probing the children. A bit hacky.

> >
> >>How would you also deal with a second (let's say identical) device on BAR1?
> >
> >   ranges = <0x00000000  0x82000000 0x00000000 0x08000000  0x8000000  //  BAR 0
> >             0x10000000  0x82000000 0x00000000 0x08000000  0x8000000> // BAR 1
> 
> Aren't in this case the two regions aliased? Shouldn't the fourth
> column on either row be 0x00000000? Like:

Yes, you are right!

>     ranges = <0x00000000  0x82000000 0x00000000 0x00000000
> 0x8000000 //  BAR 0
>               0x10000000  0x82000000 0x00000000 0x08000000
> 0x8000000> // BAR 1
> 
> >Use reg <0x10000abc xxx> to refer to the 2nd BAR. There are other ways
> >to organize things.
>
> Now I completely lost you. According to the above description (BTW,
> I like the double space to separate groups -- it makes this whole
> mess a little messy!) the child address should have a single cell.
> Why use two then?

0x10000abc is the address, xxxx is the size, reg always has address +
size

Jason
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html