[PATCH 0/9] Switch internal registers address to 0xF1 on Armada 370/XP

[Thomas Petazzoni]

Dear Jason Gunthorpe,

On Wed, 22 May 2013 12:19:53 -0600, Jason Gunthorpe wrote:

> Just to chime in a bit here.. We also hit this problem on Kirkwood,
> and fixed it in the bootloader. The power on default for Kirkwood
> chips is 0xd0000000 and Linux has long required the bootloader to move
> it.. Not sure why 370/XP dropped this.

This is a mistake from the original support of Armada 370/XP. What we
want to do now is to fix this mistake by moving everybody to 0xf1 and
align with the other Marvell EBU platforms.

What we're proposing here is only a *temporary* solution to provide a
smooth upgrade path for users of platforms that have an old bootloader.
This temporary solution is meant to go away when we have a good
confidence that new bootloaders for OpenBlocks and Mirabox are
available, and that enough users will have seen the warning that we
plan to add in two release cycles.

> Also, can you talk abit more about how this works to give more low DDR
> memory? The docs say you can't overlap windows, and DDR CS's have a
> power of two size/alignment requirement.
> 
> Does this mean the desire is to place a DDR CS from 3G -> 3.5G?

The MBus windows can overlap the DRAM chip selects. On Armada 370/XP,
there is a tuple of registers, called the MBus Bridge Window Base
Register and the MBus Bridge Window Control Register, that provide a
range of addresses for which the CPU should go look at MBus windows
instead of looking at the DRAM.

So, on a board with 8 GB of RAM, you have one DRAM chip select for the
first 4 GB of RAM, and a second chip select for the last 4 GB of RAM.
This is currently visible with the Armada XP GP board:

# cat /sys/kernel/debug/mvebu-mbus/sdram 
[0] 0000000000000000 - 0000000100000000 : cs0
[1] 0000000100000000 - 0000000200000000 : cs1
[2] disabled
[3] disabled

And the internal registers (be they mapped at 0xd0000000 of 0xf1000000)
and all the other windows (for PCIe and al) are overlapping the DRAM CS.

This is not a problem, because the MBus Bridge Window {Base,Control}
Register tell the CPU that memory accesses between addresses X and Y
are for MBus windows, and the other accesses are for the DRAM.

With the old bootloader, the MBus Bridge Window Base is set to
0xC0000000 (3G), so you cannot benefit from this additional memory
between 3G and 3.5G. But the only two consumers platforms available
that are using the old bootloaders have 3 GB of RAM or less (the
OpenBlocks has an expansion slot, but I believe it's reasonable to ask
the user to upgrade the bootloader if he wants to use a larger RAM on
this specific board).

With the new bootloader, the MBus Bridge Window Base is set to
0xE0000000 (3.5 GB). So any access in [ 0 ; 3.5 GB ] and [ 4 GB; ... ]
will go to the DRAM, and access between [ 3.5 GB ; 4 GB ] will go to
the MBus windows, including the internal registers one.

Does that make sense to explain how we can actually save 512 MB of RAM ?

> > For example, I'm currently booting alternatively with an old and a new
> > bootloader (to test that things work properly), and in both cases I'm
> > booting old style, DTB-appended, with ATAGs.
> 
> IMHO the DTB must match both the hardware *and* the bootloader. The
> bootloader is setting the address map, the DTB contains that address
> map, they must all match together.
> 
> Using a DTB property really is the right way to go.
> 
> Yes, I realize this means you now need to vary the DTB you will use
> depending on boot loader version (see below)
> 
> To use a DTB property, the approach would be to make the internal regs
> base address inside the kernel dynamic instead of hard coded. Do not
> remap the internal regs, just use whatever the bootloader setup
> directly. The kernel can early parse the FDT to find the correct
> address, instead of hardwiring an arbitary base address.
> 
> Also, the blind remapping approach in these patches is sketchy. If the
> bootloader has placed some other mapping at the 0xf1.. target address
> this will blow up. The mbus driver gets it right, to change the window
> layout you have to clear out and disable all window registers except
> for internal, then rebuild the entire mapping from scratch to avoid
> address overlap conflicts. That approach doesn't seem possible from
> early assembly.. :(
> 
> All things considered, I think it is much safer and cleaner to make
> the physical base of internal regs dynamic within the kernel, than to
> try and relocate it during early boot.

We believe it is not safer to make the physical base of internal regs
dynamic within the kernel. Really, the 0xd0 usage should be seen as a
mistake, and the goal is to align everybody to use 0xf1 for all Marvell
platforms.

> So, please consider this instead. The kernel has functions already to
> parse the fdt directly, they can be called before anything attempts to
> map internal regs based stuff.
> 
> You can combine this idea with the CP15 signalling mechanism. Check
> CP15 and use it to alter the DTB, similar to how ATAGs are used to
> fixup the DTB. Don't relocate the registers.

Where would the code doing this would sit? Remember, this is a
temporary solution that we want to get rid of after some time. So I've
been very careful in this proposal to only touch mvebu-specific code.

With what you're proposing, we would have to add something like:

	if (of_machine_is_compatible("marvell,armada-370-xp")) {
		/* do our crap */
	}

in some ARM generic code. That's something we would definitely like to
avoid, and I'm pretty the ARM maintainers would also like to avoid such
SoC-specific code spreading into generic kernel places.

Could you be more specific about where such early code would be
located, and what it would look like? I'm pretty sure once we will see
it, we'll clearly see that it's not acceptable to put such SoC-specific
code in a generic place of the kernel.

Thanks for your feedback,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com