Re: [PATCH] devicetree: Add generic IOMMU device tree bindings

Arnd Bergmann
2014-05-20T11:15:48Z

On Tuesday 20 May 2014 13:05:37 Thierry Reding wrote:
> On Tue, May 20, 2014 at 12:04:54PM +0200, Arnd Bergmann wrote:
> > On Monday 19 May 2014 22:59:46 Thierry Reding wrote:
> > > On Mon, May 19, 2014 at 08:34:07PM +0200, Arnd Bergmann wrote:
> > > > On Monday 19 May 2014 14:53:37 Thierry Reding wrote:
> > > > > On Mon, May 19, 2014 at 12:26:35PM +0200, Arnd Bergmann wrote:
> > > > > > On Friday 16 May 2014 14:23:18 Thierry Reding wrote:
> > > > > > > From: Thierry Reding <treding@nvidia.com>
> [...]
> > > > > > Finally, it makes no sense to use the dma-ranges property of the master's
> > > > > > parent bus, because that bus isn't actually involved in the translation.
> > > > >
> > > > > My understanding here is mostly based on the OpenFirmware working group
> > > > > proposal for the dma-ranges property[0]. I'll give another example to
> > > > > try and clarify how I had imagined this to work:
> > > > >
> > > > > / {
> > > > > #address-cells = <2>;
> > > > > #size-cells = <2>;
> > > > >
> > > > > iommu {
> > > > > /*
> > > > > * This is somewhat unusual (or maybe not) in that we
> > > > > * need 2 cells to represent the size of an address
> > > > > * space that is 32 bits long.
> > > > > */
> > > > > #address-cells = <1>;
> > > > > #size-cells = <2>;
> > > >
> > > > You should never need #size-cells > #address-cells
> > >
> > > That was always my impression as well. But how then do you represent the
> > > full 4 GiB address space in a 32-bit system? It starts at 0 and ends at
> > > 4 GiB - 1, which makes it 4 GiB large. That's:
> > >
> > > <0 1 0>
> > >
> > > With #address-cells = <1> and #size-cells = <1> the best you can do is:
> > >
> > > <0 0xffffffff>
> > >
> > > but that's not accurate.
> >
> > I think we've done both in the past, either extended #size-cells or
> > taken 0xffffffff as a special token. Note that in your example,
> > the iommu actually needs #address-cells = <2> anyway.
>
> But it needs #address-cells = <2> only to encode an ID in addition to
> the address. If this was a single-master IOMMU then there'd be no need
> for the ID.

Right. But for a single-master IOMMU, there is no need to specify
any additional data, it could have #address-cells=<0> if we take the
optimization you suggested.

> This really isn't specific to IOMMUs though. It really applies to all
> cases where #address-cells and #size-cells are parsed. While it's way
> too late to change the semantics of standard properties, perhaps for
> properties that are introduced it would make more sense to encode this
> as a <start limit> pair, both of length #address-cells, to avoid this
> type of corner case.
>
> On the other hand doing so would make it inconsistent with existing
> bindings which may not be desirable either.
>
> But since it seems like we're headed for something completely different
> for IOMMUs, perhaps it would be worth considering to describe the IOMMU
> range as <start limit>. Since it will likely use #iommu-cells rather
> than #address-cells we have an opportunity to change the semantics.

I'd still prefer #address-cells/#size-cells over #iommu-cells.

> > / {
> > #address-cells = <1>;
> > #size-cells = <1>;
> >
> > iommu {
> > #address-cells = <2>; // ID, address
> > #size-cells = <2>;
> > };
> >
> > master@a {
> > iommus = <& {/iommu} 0xa 0x0 0x1 0x0>; // 4GB ID '0xa'
> > }
> >
> > bus1 {
> > #address-cells = <1>;
> > #size-cells = <1>;
> > ranges;
> > iommus = <& {/iommu} 0 0 0x100 0>; // all IDs
> > dma-ranges = <0 0xb 0 1 0>; // child devices use ID '0xb'
> >
> > anothermaster {
> > // no iommus link, implied by dma-ranges above
> > };
> > };
> > };
> >
> > If you set #size-cells=<0>, you can't really do that but instead would
> > require an iommus property in each master, which is not a big concern
> > either.
>
> I'm not sure I understand the need for 0x100 (all IDs) entry above. If
> bus1's iommus property applies to all devices on the bus, why can't the
> ID 0xb be listed in the iommus property?
>
> bus1 {
> #address-cells = <1>;
> #size-cells = <1>;
> ranges;
> iommus = <&{/iommu} 0xb 0 0x1 0x0>; // 4GB ID '0xb'
> dma-ranges = <0 0xb 0 0x1 0x0>;
>
> anothermaster {
> ...
> };
> };

It depends on how the address is interpreted, but we could make this
a valid case too.

> In which case I guess dma-ranges would be redundant.

No, because the iommus property doesn't translate the address range, it
just creates a new address space. bus1 and iommu in the example have
different #address-cells, so you definitely need a non-empty ranges
property.

> > The main advantage I think would be for IOMMUs that use the PCI b/d/f
> > numbers as IDs. These can have #address-cells=<3>, #size-cells=<2>
> > and have an empty dma-ranges property in the PCI host bridge node,
> > and interpret this as using the same encoding as the PCI BARs in
> > the ranges property.
>
> I'm somewhat confused here, since you said earlier:
>
> > After giving the ranges stuff some more thought, I have come to the
> > conclusion that using #iommu-cells should work fine for almost
> > all cases, including windowed iommus, because the window is not
> > actually needed in the device, but only in the iommu, wihch is of course
> > free to interpret the arguments as addresses.
>
> But now you seem to be saying that we should still be using the
> #address-cells and #size-cells properties in the IOMMU node to determine
> the length of the specifier.

I probably wasn't clear. I think we can make it work either way, but
my feeling is that using #address-cells/#size-cells gives us a nicer
syntax for the more complex cases.

Arnd