On Tuesday 20 September 2011, Pawel Moll wrote:

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

quoted

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... ;-)

But I think both ways would be acceptable in the end. It's not a lot
of extra code either way. In the second case, I would probably have
the legacy case as a special variant of the map_io function and have
all others be the default instead of falling back to panic though.

quoted

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.

Agreed, I think that approach would be much worse.

quoted

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).

Well, I think with Nico's cahnges, you /can/ actually do ioremap for
areas that have been mapped through the iotable before kmalloc is up.
IIRC, omap does this for a number of peripherals.

It's a bit of a hack, but I think it's much better than taking hardcoded
addresses.

quoted

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.

With the combination of the points mentioned above, you should be
able to do:

- map the entire I/O area in map_io(), depending on the board
- have an __iomem pointer for the sysreg
- populate that pointer using of_iomap from the device tree address
  before you first access it.

Do you think that would work?

	Arnd