of: When constructing the bus id consider assigned-addresses as well

'assigned-addresses' is used for certain PCI device type nodes in lieu of 'reg', since this is enforced by of/address.c, have of_device_make_bus_id look there as well.

---
 drivers/of/platform.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

of_can_translate_address and of_translate_address already support using assigned-addresses.

index b80891b..4f0f701 100644
@@ -105,6 +105,8 @@ void of_device_make_bus_id(struct device *dev)
 	 * For MMIO, get the physical address
 	 */
 	reg = of_get_property(node, "reg", NULL);
+	if (!reg)
+		reg = of_get_property(node, "assigned-addresses", NULL);
 	if (reg) {
 		if (of_can_translate_address(node)) {
 			addr = of_translate_address(node, reg);

1.7.5.4

If it is a PCI device, then of_device_make_bus_id() shouldn't come into play. PCI devices already have their own naming scheme. Only platform_bus device creation uses of_device_make_bus_id(). What am I missing?

g.

Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

In my embedded case I have a complex PCI-E connected SOC device.

This is modeled in OF by having a PCI-E bus, a PCI-E device node, and then all of the SOC devices (I2C, GPIO, drivers, etc) placed under the PCI-E device node.

The PCI driver that matches the device just turns it on and calls of_platform_populate(..) with its own node as an argument.

So of_device_make_bus_id isn't called on a PCI-E device node, it is called on the platform_device children of that node, and due to the way the other code works, and what the OF rules seem to be, those childen all use assigned-addresses. Without this patch the code just assigns monotonic ids to those nodes.

Jason

Hmmm. okay that makes sense, but something still isn't quite right. So of_translate_address should take care of drilling down through the bus layers, and when it gets to the PCI node it /should/ use of_bus_pci_translate to handle traversing down to the parent node (which uses the 'assigned-addresses' for the pci node.

However, in your case, of_device_make_bus_id() isn't using that code path and you're getting a generic name instead (with no relation to the device address). Correct?

If that is the case, then the solution is to figure out why of_translate_address() doesn't currently handle your situation and fix it. It is not a good idea to add assigned-addresses specific parsing code to that function since that won't work for any of the other bus types.

g.

The address translation machinery requires PCI format addresses (ie address-cells=3) for all nodes below a PCI bus. Part of this requirement is that 'assigned-addresses' is used for resources, *not* 'reg'.

If you attempt to stick a 'reg' in a block nested below a 'device_type="pci"' the kernel throws lots of error messsages and generates bad address mappings.

So, we are required to use'assigned-addresses' with the 5 word format instead of reg. This seems to be a spec requirement for everything below a PCI bus.

We end up with a DTS where the PCI bus and everything below it must be described in the 5 word format that looks like this:

	pex@e0000000 { // <-- This is the PCI bus/controller node
		device_type = "pci";
		ranges = <0x02000000 0x00000000 0x00000000  0xe0000000  0x0 0x8000000>;
		soc@0 { // <-- This is the actual PCI device
			ranges = <0x02000000 0x00000000 0x00000000  0x02000000 0x00000000 0x00000000  0x0 0x8000000>;
			gpio3: gpio@8 { // <-- This is a platform device
			        #gpio-cells = <2>;
				compatible = "linux,basic-mmio-gpio";
				gpio-controller;
				reg-names = "dat", "set", "dirin";
				assigned-addresses = <0x02000000 0x0 0x8  0x0 4>,
				                     <0x02000000 0x0 0xc  0x0 4>,
				                     <0x02000000 0x0 0x10  0x0 4>;
			};

Which (when combined with the platform_device_add change) builds up an iomem like:

e0000000-e7ffffff : PCIe 0 MEM
e0000000-e000ffff : 0000:00:01.0

    e0000000-e0000fff : /pex@e0000000/soc@0/control@0
      e0000008-e000000b : dat
        e0000008-e000000b : dat
      e000000c-e000000f : set
        e000000c-e000000f : set
      e0000010-e0000013 : dirin
        e0000010-e0000013 : dirin

(I trimmed control@0 node from the dts fragment, see other mails on
the overlapping regions)

And a sysfs like this:

/sys/devices/pci0000:00/0000:00:01.0/e0000000.control /sys/devices/pci0000:00/0000:00:01.0/e0000008.gpio

Without the patch the sysfs names will not have the address (gpio.0 or whatever it is), but all other address calculations work correctly.

Right.

of_translate_address works perfectly - resource records are constructed correctly, for instance. The issue is that of_device_make_bus_id() doesn't call it:

@@ -105,6 +105,8 @@ void of_device_make_bus_id(struct device *dev)

         * For MMIO, get the physical address
         */
        reg = of_get_property(node, "reg", NULL);
+       if (!reg)
+               reg = of_get_property(node, "assigned-addresses", NULL);
        if (reg) {
                if (of_can_translate_address(node)) {
                        addr = of_translate_address(node, reg);

ie what is happening is that of_device_make_bus_id *only* calls *_translate_address if 'reg' is a property of the node. The patch simply extends that to call if 'reg' or 'assigned-addresses' are a property of the node. of_device_make_bus_id doesn't do anything with the 'reg' variable other than test it against NULL.

Jason

Have you added the appropriate #address-cells and #size-cells to the pci device node to go back to a non-pci addressing mode? assigned-addresses only makes sense in the pci-device node itself. reg should work for all nodes below that, and if it doesn't then it is a bug that we need to fix.

g.

Switching away from the 5 dword address format is not ideal because then there is no way to specify the resource region (prefetch, io, mmio) and mmio would have to be assumed.

Okay.. but how should the DTS be constructed?

pcie_bus { // The PCI-E bus
device_type = "pci";
ranges = <5dw ranges>;
#address-cells = <3>;
#size-cells = <2>;
soc_bridge { // The PCI-E device

      device_type = "pci";
      ranges = <5dw ranges>;

      soc_device { // Internal device
          assigned-address = <5dw regs>
      };

};
};

This is what I have now, the soc_bridge PCI-E device is DTS modeled as a PCI bridge - it has a ranges with its memory location, and the children nodes are relative to those ranges. This would not be typical for a non-bridge PCI-E device.

The reason for the 'assigned-address' requirement with the current kernel code is the device_type=pci on soc_bridge. This makes of_match_bus(parent) for soc_device return the PCI structure, which has '.addresses = "assigned-addresses",'

So.. how would you like this to look?

Jason

You don't need to switch away from using 5 cells if that works best for you, but I'd be surprised if it was the ideal representation. I would expect you to use a representation that makes sense for the internal bus architecture of the device. If if exactly matches the PCI address, then go ahead with 5 cells, but if it is one or more 32bit busses, then use 1 or 2 for #address-cells and 1 for #size-cells.

pcie_bus { // The PCI-E bus
device_type = "pci";
ranges = <5dw ranges>;
#address-cells = <3>;
#size-cells = <2>;
soc_bridge { // The PCI-E device

      device_type = "pci";

      // These are important to set up the address format in the child
      // nodes
      #address-cells = <3>;
      #size-cells = <2>;

      // Translation from PCI bus space to local bus space.
      ranges = <5dw ranges>;

      soc_device { // Internal device
          reg = <5dw regs>
      };

};
};

Now, if the children of soc_bridge really are PCI devices (and not just plain-vanilla memory mapped IP cores like I assume above), then they shouldn't be registered in the kernel as platform_devices at all. In that case register them as PCI devices and the existing PCI infrastructure should do the naming correctly.

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.

g.