Re: DT vs ARM static mappings

Pawel,

On 09/20/2011 09:02 AM, Pawel Moll wrote:

quoted

quoted

Of course the simplest solution would be to define two different
compatible values, eg. "arm,vexpress-legacy" would execute the current
map_io implementation, while "arm,vexpress-rs1" would use different one,
setting up the other map_desc (the MMIO_P2V macro must die of course,
replaced with a runtime-defined virtual base address for the
peripherals).

If you believe that's what I should do, say it and stop reading :-)

Yes. Different tiles are fundamentally different boards, so they should
have different DTs. Using includes should help minimize duplication though.

You've misunderstood me or (most likely ;-) probably I wasn't clear
enough.

There is no doubt the DTs will be different across the "portfolio".

We already have (patches soon) vexpress-v2p-ca9.dts that includes
vexpress-v2m-legacy.dtsi.

A5 will be vexpress-v2p-ca5p.dts+vexpress-v2m-rs1.dtsi, A15
vexpress-v2p-ca15.dts+vexpress-v2m-rs1.dtsi (notice that the A5/A15 are
sharing the v2m bit, as the motherboard is common).

My point is that we should be able to handle _all_ of them using one
DT_MACHINE_START with a single compat value "arm,vexpress". The only
problem with this (so far) is the mapping.

Yes, you should have 1 DT_MACHINE_START, but arm,vexpress is too
generic. You can and should have a list of compatible strings for each
board/machine.

quoted

Think about it this way. How would you solve this without DT? You
would have a bunch of duplicated data in the kernel for the different
configs. So you're not any worse off in this regard and still have the
other advantages of DT.

Exactly my point :-) I want to have as little duplication as possible.
And the static mapping issue is in the way.

quoted

quoted

To my mind it looked like the whole mechanism was not flexible enough,
so I wanted to explore other options...

The obvious one was to describe the required static mapping in the DTS.
I don't like this idea, though. It can hardly be called "hardware
description". Besides, what node would carry such data? "chosen"?
Hardly...

Would it contain a "regs" property with the physical address and
"virtual-reg" with the virtual one? Again, doesn't sound right to me
(especially the virtual bit, however the virtual address could be common
between different variants and be defined in the board support code, not
the DTS).

I have considered a reference (phandle or an alias?) to the node to be
mapped ("peripherals" in my case), but where to define this reference?
Any ideas?

In "chosen" like the kernel command line would be the place, but I don't
think that is the right approach. Chosen is really for things that
change frequently and this doesn't really fall in that category.

Again, no argument from me here :-)

The question is - where should it be?

Nowhere. It's an OS specific issue, not a h/w issue.

quoted

quoted

There is an additional problem here... The "map_io" is executed before
the tree is un-flattened, so:

1. One can't simply use "of_find_matching_node()" (as in the latest l2x0
patches) to find the interesting nodes - the only way of going through
the tree is raw of_scan_flat_dt() function. Therefore any conditions
more complex then string comparison with the (full) node name are
problematic.

2. The tree mappings (ranges) are not resolved yet, so one can't simply
get the effective address of a node. Only "raw" properties are
available, so all one can get scanning for "peripherals at 7" node is "0 7
0 0x20000" array, instead of the "0x10000000 0x00020000" that is really
important. 

If you add a compatible field to "motherboard" node, then you can read
the ranges.

... and then and then scan for the sysregs, and add the offset and base
together... Sounds to me like duplication of the of_translate_*()?

quoted

quoted

Initially I wanted to find the mentioned devices and create individual
mappings for them, so the MMIO_P2V would be still valid (if slightly
"abused"), but I failed due to the problems mentioned above. And I can't
delay this operation till the tree is un-flattened, as the core tile
must be probed (via sysreg) in map_io (tile's specific code must be able
to create its own mappings):

Do you really need MMIO_P2V? If you have fixed virtual addresses in the
kernel and can pull the phys addresses from DT to populate the iotable,
is that sufficient?

For the third time, 100% agree :-) Well, 90%.

What I need is:

1. Get the phys address from DT. But how? This is getting as back to my
complaints about still-flat tree and ranges, the node to be used to
describe the mapping.

2. The offset inside the mapping will be different (for sysregs it will
be 0 for old mapping, 0x10000 for the new one), so I have to work it out
from the tree as well. And as we are in map_io, the tree is still flat
and... read 1 :-)

So create a mapping per peripheral rather than per chip select. Then the
virtual address can always be the same.

quoted

Generally, the trend is to get rid of static mappings as much as
possible. Doing that first might simplify things.

You can't do ioremap() before kmalloc() is up and running (correct me if
I am wrong), at least you can't do this in map_io. So the static mapping
is a must sometimes. And actually, with the latest Nico's changes:

Correct. You can't do ioremap until init_irq. map_io and init_early are
too early. My point was if you can delay h/w access then you can remove
the static mappings. But yes, we generally can't remove them all. SCU
and LL debug uart are 2 examples.

For the short term, I would just have 2 static iotables and select the
right one based on the board's (or motherboard's) compatible string.

Long term, we should look into implementing a common early DT address
parsing function.

Rob

http://thread.gmane.org/gmane.linux.ports.arm.kernel/132762

it may even be preferred for peripherals (one mapping shared across all
users).

Cheers!

Pawe?

On Tue, 2011-09-20 at 15:37 +0100, Rob Herring wrote

quoted

My point is that we should be able to handle _all_ of them using one
DT_MACHINE_START with a single compat value "arm,vexpress". The only
problem with this (so far) is the mapping.

Yes, you should have 1 DT_MACHINE_START, but arm,vexpress is too
generic. You can and should have a list of compatible strings for each
board/machine.

Our DTS has:

compatible = "arm,vexpress-v2p-ca9", "arm,vexpress";

and v2m.c:

static const char *v2m_dt_match[] __initconst = {
       "arm,vexpress",
       NULL,
};

DT_MACHINE_START(VEXPRESS_DT, "ARM Versatile Express")
       .map_io         = v2m_map_io,
       .init_early     = v2m_init_early,
       .init_irq       = v2m_init_irq,
       .timer          = &v2m_timer,
       .init_machine   = v2m_dt_init,
       .dt_compat      = v2m_dt_match,
MACHINE_END

Isn't it what you meant?

Essentially I see two ways of doing what we are discussing:

1. Two DT_MACHINE_START, one matching "arm,vexpress-legacy" with map_io
= v2m_map_io_legacy and second matching "arm,vexpress-rs1" with map_io =
v2m_map_io_rs1,

2. Single DT_MACHINE_START matching (the most generic) "arm,vexpress"
and doing (rougly) this in v2m_map_io:

of_scan_flat_dt(v2m_dt_iotable_init, NULL);

v2m_dt_iotable_init(...)
{
	if (depth != 0)
		return 0;
	if (of_flat_dt_is_compatible(node, "arm,vexpress-legacy"))
		iotable_init(v2m_io_desc_legacy);
	else (of_flat_dt_is_compatible(node, "arm,vexpress-rs1"))
		iotable_init(v2m_io_desc_rs1);
	else
		panic();
}

Neither of them seem particularly appealing... ;-)

quoted

quoted

In "chosen" like the kernel command line would be the place, but I don't
think that is the right approach. Chosen is really for things that
change frequently and this doesn't really fall in that category.

Again, no argument from me here :-)

The question is - where should it be?

Nowhere. It's an OS specific issue, not a h/w issue.

That's exactly why I didn't like this idea in the first place. This
doesn't change the fact that current infrastructure isn't really helpful
here.

So create a mapping per peripheral rather than per chip select. Then the
virtual address can always be the same.

As I said (see below) this is exactly what I wanted to do, but I was
defeated by the reality :-)

On Tue, 2011-09-20 at 12:51 +0100, Pawel Moll wrote: 

quoted

quoted

Initially I wanted to find the mentioned devices and create individual
mappings for them, so the MMIO_P2V would be still valid (if slightly
"abused"), but I failed due to the problems mentioned above. And I can't
delay this operation till the tree is un-flattened, as the core tile
must be probed (via sysreg) in map_io (tile's specific code must be able
to create its own mappings):

quoted

quoted

Generally, the trend is to get rid of static mappings as much as
possible. Doing that first might simplify things.

You can't do ioremap() before kmalloc() is up and running (correct me if
I am wrong), at least you can't do this in map_io. So the static mapping
is a must sometimes. And actually, with the latest Nico's changes:

Correct. You can't do ioremap until init_irq. map_io and init_early are
too early. My point was if you can delay h/w access then you can remove
the static mappings. But yes, we generally can't remove them all. SCU
and LL debug uart are 2 examples.

In my case it's sysreg and sysctl. There are two more users of static
mappings: timer01 and timer23, but they could at some point do ioremap()
on their own (especially with Nico's changes).

For the short term, I would just have 2 static iotables and select the
right one based on the board's (or motherboard's) compatible string.

Yes, as mentioned above. This doesn't help with the sysreg offset
problem though. I may just scan the flat tree looking for their
particular names and getting raw offset from their regs... Sounds like a
hack, though.

Long term, we should look into implementing a common early DT address
parsing function.

Well, assuming that we want to have them at all. I'm not convinced,
frankly ;-)

Pawe?