Extending the Marvell MBus DT binding to create remappable windows

Extending the Marvell MBus DT binding to create remappable windows

[Thomas Petazzoni]

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

Extending the Marvell MBus DT binding to create remappable windows

[Arnd Bergmann]

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

Extending the Marvell MBus DT binding to create remappable windows

[Thomas Petazzoni]

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

Extending the Marvell MBus DT binding to create remappable windows

[Arnd Bergmann]

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

Extending the Marvell MBus DT binding to create remappable windows

[Thomas Petazzoni]

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

Extending the Marvell MBus DT binding to create remappable windows

[Arnd Bergmann]

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