* Extending the Marvell MBus DT binding to create remappable windows @ 2015-02-16 17:28 Thomas Petazzoni 2015-02-16 20:05 ` Arnd Bergmann 0 siblings, 1 reply; 6+ messages in thread From: Thomas Petazzoni @ 2015-02-16 17:28 UTC (permalink / raw) To: linux-arm-kernel Arnd, Ezequiel, Both of you were heavily involved in the definition of the MBus Device Tree binding, and I'm now facing a new need, which maybe will require extending this Device Tree binding. Let me summarize a few things: - MBus windows are physical address windows that can be dynamically created. They associate a range of physical addresses with a certain device (a NOR flash, a PCIe memory or I/O area, etc.). - Some windows are statically described in the Device Tree: the ones for a NOR flash, for the BootROM, etc. For these, the Device Tree gives both the MBus target ID and attribute (which identify the target device) and the physical address and size at which the MBus window will be created. Such windows are created by the mvebu-mbus driver during its initialization. - Some windows however have to be dynamically created: the PCIe memory and I/O windows. For these, the Device Tree only specifies the MBus target ID and attribute for each PCIe interface, but the windows are created by the pci-mvebu driver at runtime, using an API provided by the mvebu-mbus driver. Windows, in addition to having a (target ID, target attribute) identifying the device plus a (physical address, size) describing where the window is mapped in the CPU physical address, can describe a "remap address". It's basically an additional register that exists for each configurable MBus window, which allows to say at what address the window is mapped "from the point of view of the device". For now, such remappable windows were only used for PCI I/O windows: even if an I/O window is visible at (say) 0xe8000000 in the CPU physical address space, we still need to make the PCIe device believe the PCIe I/O accesses are made from address (say) 0x10000. This is what the remappable register that exists for each window allows to do. For dynamically created windows, this works all fine: the mvebu-mbus driver provides an API to create windows while specifying a remappable address. So for the PCIe case, no problem. Now, on the newly released Armada 39x SoC, the networking complex requires some MBus windows, and one of them needs to be configured with a certain "remap" value. Those windows are not dynamic like the PCIe ones: they should be defined statically in the Device Tree, just like the MBus window for a NOR flash. Unfortunately, it seems we designed the Device Tree binding for MBus without this use case in mind: there is no way in the MBus DT binding to specify a "remap" address for a certain window. Do you have an idea on how we could extend the MBus Device Tree binding to encode this additional information? If you want to learn more about this remappable attribute, look at the public Armada XP datasheet (http://www.marvell.com/embedded-processors/armada-xp/assets/ARMADA-XP-Functional-SpecDatasheet.pdf), section 3.1.1 for example. This section is only about PCIe remapping, but the same happens for other devices. Also look at the description of register 0x20008 (table 182, page 631), it describes the remap register for window 0. Thanks for your input, Thomas -- Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Extending the Marvell MBus DT binding to create remappable windows 2015-02-16 17:28 Extending the Marvell MBus DT binding to create remappable windows Thomas Petazzoni @ 2015-02-16 20:05 ` Arnd Bergmann 2015-02-16 20:40 ` Thomas Petazzoni 0 siblings, 1 reply; 6+ messages in thread From: Arnd Bergmann @ 2015-02-16 20:05 UTC (permalink / raw) To: linux-arm-kernel On Monday 16 February 2015 18:28:05 Thomas Petazzoni wrote: > Now, on the newly released Armada 39x SoC, the networking complex > requires some MBus windows, and one of them needs to be configured > with a certain "remap" value. Those windows are not dynamic like the > PCIe ones: they should be defined statically in the Device Tree, just > like the MBus window for a NOR flash. Ok > Unfortunately, it seems we designed the Device Tree binding for MBus > without this use case in mind: there is no way in the MBus DT binding > to specify a "remap" address for a certain window. We did? I thought that the binding allowed the case you described, but it's quite possible that I'm missing something. > Do you have an idea on how we could extend the MBus Device Tree > binding to encode this additional information? > > If you want to learn more about this remappable attribute, look at the > public Armada XP datasheet > (http://www.marvell.com/embedded-processors/armada-xp/assets/ARMADA-XP-Functional-SpecDatasheet.pdf), > section 3.1.1 for example. This section is only about PCIe remapping, > but the same happens for other devices. Also look at the description > of register 0x20008 (table 182, page 631), it describes the remap > register for window 0. Looking at for example arch/arm/boot/dts/armada-xp-netgear-rn2120.dts, I find the following mbus ranges ranges = <MBUS_ID(0xf0, 0x01) 0 0 0xd0000000 0x100000 MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000>; ^ ^ ^ ^ ^ a b c c d and here the table should get set up from the fields like a) mbus target (32 bit) b) mbus-local address (32 bit) c) cpu-physical address (64 bit) d) window size with the registers from the spec, this seems to map to a) 0x00020000 b) 0x00020008 c) 0x00020004 d) 0x00020000 so you just need to put the remap-address into the second cell and ensure that it is correctly evaluated by the driver. Did you miss that part of the binding, or did I miss what you are trying to say? Arnd ^ permalink raw reply [flat|nested] 6+ messages in thread
* Extending the Marvell MBus DT binding to create remappable windows 2015-02-16 20:05 ` Arnd Bergmann @ 2015-02-16 20:40 ` Thomas Petazzoni 2015-02-16 20:54 ` Arnd Bergmann 0 siblings, 1 reply; 6+ messages in thread From: Thomas Petazzoni @ 2015-02-16 20:40 UTC (permalink / raw) To: linux-arm-kernel Arnd, Thanks for your quick feedback! On Mon, 16 Feb 2015 21:05:52 +0100, Arnd Bergmann wrote: > Looking at for example arch/arm/boot/dts/armada-xp-netgear-rn2120.dts, > I find the following mbus ranges > > ranges = <MBUS_ID(0xf0, 0x01) 0 0 0xd0000000 0x100000 > MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000>; > ^ ^ ^ ^ ^ > a b c c d > > and here the table should get set up from the fields like > > a) mbus target (32 bit) > b) mbus-local address (32 bit) > c) cpu-physical address (64 bit) > d) window size > > with the registers from the spec, this seems to map to > > a) 0x00020000 > b) 0x00020008 > c) 0x00020004 > d) 0x00020000 > > so you just need to put the remap-address into the second cell and ensure > that it is correctly evaluated by the driver. > > Did you miss that part of the binding, or did I miss what you are > trying to say? Well what you are saying here does not really match exactly what the binding document is saying. From http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/bus/mvebu-mbus.txt#L77: 77 ** MBus address decoding window specification 78 79 The MBus children address space is comprised of two cells: the first one for 80 the window ID and the second one for the offset within the window. 81 In order to allow to describe valid and non-valid window entries, the 82 following encoding is used: 83 84 0xSIAA0000 0x00oooooo I am not sure what this "offset within the window" is supposed to mean. Ezequiel, any idea? In addition, the current implementation of the mvebu-mbus driver considers that all windows described in the DT ranges property are non-remappable windows: static int __init mbus_dt_setup(struct mvebu_mbus_state *mbus, struct device_node *np) { int addr_cells, c_addr_cells, c_size_cells; int i, ret, cell_count; const __be32 *r, *ranges_start, *ranges_end; ret = mbus_parse_ranges(np, &addr_cells, &c_addr_cells, &c_size_cells, &cell_count, &ranges_start, &ranges_end); if (ret < 0) return ret; for (i = 0, r = ranges_start; r < ranges_end; r += cell_count, i++) { u32 windowid, base, size; u8 target, attr; /* * An entry with a non-zero custom field do not * correspond to a static window, so skip it. */ windowid = of_read_number(r, 1); if (CUSTOM(windowid)) continue; target = TARGET(windowid); attr = ATTR(windowid); base = of_read_number(r + c_addr_cells, addr_cells); size = of_read_number(r + c_addr_cells + addr_cells, c_size_cells); ret = mbus_dt_setup_win(mbus, base, size, target, attr); if (ret < 0) return ret; } return 0; } And finally, with a "0" value in 'b' as you suggest, you have no idea whether it means: * That you want a remap address to be set to 0. * Or that you don't want any remap address. This difference is quite important, because out of the 20 windows available, 9 of them are remappable windows, and 11 are not remappable. So you don't want to use windows that have the remap capability for no reason. Or, we should make b == c to indicate that the CPU side address and the MBus local address are the same, and in this case, it indicates that we don't want the remap capability to be enabled for this window. However, if b != c, then we need to enable the remapping capability. Notice also that the remap address can be a 64 bits address, even though I don't really understand in which scenarios this is used. Thanks, Thomas -- Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Extending the Marvell MBus DT binding to create remappable windows 2015-02-16 20:40 ` Thomas Petazzoni @ 2015-02-16 20:54 ` Arnd Bergmann 2015-02-16 21:06 ` Thomas Petazzoni 0 siblings, 1 reply; 6+ messages in thread From: Arnd Bergmann @ 2015-02-16 20:54 UTC (permalink / raw) To: linux-arm-kernel On Monday 16 February 2015 21:40:39 Thomas Petazzoni wrote: > On Mon, 16 Feb 2015 21:05:52 +0100, Arnd Bergmann wrote: > > > Looking at for example arch/arm/boot/dts/armada-xp-netgear-rn2120.dts, > > I find the following mbus ranges > > > > ranges = <MBUS_ID(0xf0, 0x01) 0 0 0xd0000000 0x100000 > > MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000>; > > ^ ^ ^ ^ ^ > > a b c c d > > > > and here the table should get set up from the fields like > > > > a) mbus target (32 bit) > > b) mbus-local address (32 bit) > > c) cpu-physical address (64 bit) > > d) window size > > > > with the registers from the spec, this seems to map to > > > > a) 0x00020000 > > b) 0x00020008 > > c) 0x00020004 > > d) 0x00020000 > > > > so you just need to put the remap-address into the second cell and ensure > > that it is correctly evaluated by the driver. > > > > Did you miss that part of the binding, or did I miss what you are > > trying to say? > > Well what you are saying here does not really match exactly what the > binding document is saying. From > http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/bus/mvebu-mbus.txt#L77: > > 77 ** MBus address decoding window specification > 78 > 79 The MBus children address space is comprised of two cells: the first one for > 80 the window ID and the second one for the offset within the window. > 81 In order to allow to describe valid and non-valid window entries, the > 82 following encoding is used: > 83 > 84 0xSIAA0000 0x00oooooo > > I am not sure what this "offset within the window" is supposed to mean. > Ezequiel, any idea? I think it means exactly what we need. > In addition, the current implementation of the mvebu-mbus driver > considers that all windows described in the DT ranges property are > non-remappable windows: > > static int __init mbus_dt_setup(struct mvebu_mbus_state *mbus, > struct device_node *np) > { > int addr_cells, c_addr_cells, c_size_cells; > int i, ret, cell_count; > const __be32 *r, *ranges_start, *ranges_end; > > ret = mbus_parse_ranges(np, &addr_cells, &c_addr_cells, > &c_size_cells, &cell_count, > &ranges_start, &ranges_end); > if (ret < 0) > return ret; > > for (i = 0, r = ranges_start; r < ranges_end; r += cell_count, i++) { > u32 windowid, base, size; > u8 target, attr; > > /* > * An entry with a non-zero custom field do not > * correspond to a static window, so skip it. > */ > windowid = of_read_number(r, 1); > if (CUSTOM(windowid)) > continue; > > target = TARGET(windowid); > attr = ATTR(windowid); > > base = of_read_number(r + c_addr_cells, addr_cells); > size = of_read_number(r + c_addr_cells + addr_cells, > c_size_cells); > ret = mbus_dt_setup_win(mbus, base, size, target, attr); > if (ret < 0) > return ret; > } > return 0; > } Hmm, I have no idea what that comment and the 'continue' line are about. > And finally, with a "0" value in 'b' as you suggest, you have no idea > whether it means: > > * That you want a remap address to be set to 0. > > * Or that you don't want any remap address. > > This difference is quite important, because out of the 20 windows > available, 9 of them are remappable windows, and 11 are not remappable. > So you don't want to use windows that have the remap capability for no > reason. > > Or, we should make b == c to indicate that the CPU side address and the > MBus local address are the same, and in this case, it indicates that we > don't want the remap capability to be enabled for this window. However, > if b != c, then we need to enable the remapping capability. I suspect we made a mistake with the implementation here if an offset of zero is defined to mean an identity mapping within the window. That would be very unfortunate. Back when the driver was introduced, I only reviewed the binding in detail and it all made sense, but it seems that the driver does not actually implement the binding if I understand you correctly. > Notice also that the remap address can be a 64 bits address, even > though I don't really understand in which scenarios this is used. In order to support that, we'd have to use #address-cells=<3>, but that would be a logical extension if we ever need it. Arnd ^ permalink raw reply [flat|nested] 6+ messages in thread
* Extending the Marvell MBus DT binding to create remappable windows 2015-02-16 20:54 ` Arnd Bergmann @ 2015-02-16 21:06 ` Thomas Petazzoni 2015-02-16 21:39 ` Arnd Bergmann 0 siblings, 1 reply; 6+ messages in thread From: Thomas Petazzoni @ 2015-02-16 21:06 UTC (permalink / raw) To: linux-arm-kernel Dear Arnd Bergmann, On Mon, 16 Feb 2015 21:54:38 +0100, Arnd Bergmann wrote: > > I am not sure what this "offset within the window" is supposed to mean. > > Ezequiel, any idea? > > I think it means exactly what we need. Well, I don't really understand the "remap" mechanism as an "offset within the window". The "remap" thing is apparently to make the target device believe that the memory access is not done at 0xe80000012 (which is offset 0x12 from the window base address 0xe8000000 as visible on the CPU side), but really at address 0x10012 (because this window is configured with a remap address of 0x10000). > > /* > > * An entry with a non-zero custom field do not > > * correspond to a static window, so skip it. > > */ > > windowid = of_read_number(r, 1); > > if (CUSTOM(windowid)) > > continue; > > > > target = TARGET(windowid); > > attr = ATTR(windowid); > > > > base = of_read_number(r + c_addr_cells, addr_cells); > > size = of_read_number(r + c_addr_cells + addr_cells, > > c_size_cells); > > ret = mbus_dt_setup_win(mbus, base, size, target, attr); > > if (ret < 0) > > return ret; > > } > > return 0; > > } > > Hmm, I have no idea what that comment and the 'continue' line are > about. This I know: we have a "fake" window entry, for the internal registers. They are described like a MBus window in the DT, with a fake target ID and attribute, which we handle specially here. > > And finally, with a "0" value in 'b' as you suggest, you have no idea > > whether it means: > > > > * That you want a remap address to be set to 0. > > > > * Or that you don't want any remap address. > > > > This difference is quite important, because out of the 20 windows > > available, 9 of them are remappable windows, and 11 are not remappable. > > So you don't want to use windows that have the remap capability for no > > reason. > > > > Or, we should make b == c to indicate that the CPU side address and the > > MBus local address are the same, and in this case, it indicates that we > > don't want the remap capability to be enabled for this window. However, > > if b != c, then we need to enable the remapping capability. > > I suspect we made a mistake with the implementation here if an offset > of zero is defined to mean an identity mapping within the window. I'm sorry but I don't parse this sentence :-/ > That would be very unfortunate. Back when the driver was introduced, > I only reviewed the binding in detail and it all made sense, but it > seems that the driver does not actually implement the binding if > I understand you correctly. I have no problem adjusting the driver implementation, I just need to understand the binding a bit better. Is my proposal using b == c and b != c to detect if we need the remap capability the right solution? Thanks, Thomas -- Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Extending the Marvell MBus DT binding to create remappable windows 2015-02-16 21:06 ` Thomas Petazzoni @ 2015-02-16 21:39 ` Arnd Bergmann 0 siblings, 0 replies; 6+ messages in thread From: Arnd Bergmann @ 2015-02-16 21:39 UTC (permalink / raw) To: linux-arm-kernel On Monday 16 February 2015 22:06:16 Thomas Petazzoni wrote: > Dear Arnd Bergmann, > > On Mon, 16 Feb 2015 21:54:38 +0100, Arnd Bergmann wrote: > > > > I am not sure what this "offset within the window" is supposed to mean. > > > Ezequiel, any idea? > > > > I think it means exactly what we need. > > Well, I don't really understand the "remap" mechanism as an "offset > within the window". The "remap" thing is apparently to make the target > device believe that the memory access is not done at 0xe80000012 (which > is offset 0x12 from the window base address 0xe8000000 as visible on > the CPU side), but really at address 0x10012 (because this window is > configured with a remap address of 0x10000). Then I think it's just a case of mismatched terminology: The way I read the binding, each entry in the 'ranges' property defines one mbus window that directly corresponds to set of registers in the mbus controller. The way that the 'ranges' property works is to define a window from the parent bus address space to the child address space, using a triplet of <child address> <parent address> <size>. Both child and parent are 64-bit addresses here. The parent address is in the CPU space, and is 64-bit wide because the CPU may use LPAE, while the child address uses the upper bits to identify one of many mbus-internal address spaces and the lower bits to identify the offset within that address space. Doing the access to 0xe80000012 means we look up the address window in the mbus ranges (or in the hardware registers, which are equivalent) and find an entry that defines ranges = <MBUS_ID(0xab, 0xcd) 0x10000 0 0xe8000000 0x10000>; The address 0xe80000012 is within the range 0xe8000000-0xe8010000, so we match that address to this entry. We subtract the start CPU address and add the start mbus address and get 0xe8000012 - 0x00000000.e8000000 + 0xabcd0000.00010000 = 0xabcd0000.00010012 which is offset 0x10012 into the mbus device identified as <MBUS_ID(0xab, 0xcd)> Does that way of looking at it make it clearer? > > > /* > > > * An entry with a non-zero custom field do not > > > * correspond to a static window, so skip it. > > > */ > > > windowid = of_read_number(r, 1); > > > if (CUSTOM(windowid)) > > > continue; > > > > > > target = TARGET(windowid); > > > attr = ATTR(windowid); > > > > > > base = of_read_number(r + c_addr_cells, addr_cells); > > > size = of_read_number(r + c_addr_cells + addr_cells, > > > c_size_cells); > > > ret = mbus_dt_setup_win(mbus, base, size, target, attr); > > > if (ret < 0) > > > return ret; > > > } > > > return 0; > > > } > > > > Hmm, I have no idea what that comment and the 'continue' line are > > about. > > This I know: we have a "fake" window entry, for the internal registers. > They are described like a MBus window in the DT, with a fake target ID > and attribute, which we handle specially here. We should probably change the code to match on the fake window entry instead of the offset then. > > > And finally, with a "0" value in 'b' as you suggest, you have no idea > > > whether it means: > > > > > > * That you want a remap address to be set to 0. > > > > > > * Or that you don't want any remap address. > > > > > > This difference is quite important, because out of the 20 windows > > > available, 9 of them are remappable windows, and 11 are not remappable. > > > So you don't want to use windows that have the remap capability for no > > > reason. > > > > > > Or, we should make b == c to indicate that the CPU side address and the > > > MBus local address are the same, and in this case, it indicates that we > > > don't want the remap capability to be enabled for this window. However, > > > if b != c, then we need to enable the remapping capability. > > > > I suspect we made a mistake with the implementation here if an offset > > of zero is defined to mean an identity mapping within the window. > > I'm sorry but I don't parse this sentence :-/ > > > That would be very unfortunate. Back when the driver was introduced, > > I only reviewed the binding in detail and it all made sense, but it > > seems that the driver does not actually implement the binding if > > I understand you correctly. > > I have no problem adjusting the driver implementation, I just need to > understand the binding a bit better. Is my proposal using b == c and > b != c to detect if we need the remap capability the right solution? It really depends on what address the devices see for a non-remapped address. If the device is always mapped at offset zero within the mbus window for non-remapped addresses, we are fine with the existing implementation and can just check for b==0 to set up a non-remapping window. If however the device actually has its registers at the same address that the CPU sees, then we need to check for b==c but find a way to convert all the existing DT files while providing backwards compatibility. Arnd ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-02-16 21:39 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-02-16 17:28 Extending the Marvell MBus DT binding to create remappable windows Thomas Petazzoni 2015-02-16 20:05 ` Arnd Bergmann 2015-02-16 20:40 ` Thomas Petazzoni 2015-02-16 20:54 ` Arnd Bergmann 2015-02-16 21:06 ` Thomas Petazzoni 2015-02-16 21:39 ` Arnd Bergmann
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.