Hi Simon,

On 02.12.2015 16:50, Simon Glass wrote:

<snip>

I think it would be better to make it depend on whether the bit is
flipped, rather than whether you are in SPL or not.

You simply can't detect if this "bit is flipped". You just have
to know. This is a long lasting ugly thing on some Marvell
patforms. Here the comment from armada-xp-gp.dts:

Can you point me to the place in U-Boot where this bit is flipped?
Something, somewhere has to make the change. So something has to know.
Before it makes the change, the range works one way. Afterwards it
works another way.

Sure. I've mentioned this before. Its here:

arch/arm/mach-mvebu/cpu.c:

int arch_cpu_init(void)
{
        ...

        /* Linux expects the internal registers to be at 0xf1000000 */
        writel(SOC_REGS_PHY_BASE, INTREG_BASE_ADDR_REG);

This is the line that changes the register base address. And
to change it back you need to write to the new address, as the
address holding this base address is also moved. Quite ugly!

So its really right at the start of U-Boot proper.


  ...
  * Note: this Device Tree assumes that the bootloader has remapped the
  * internal registers to 0xf1000000 (instead of the default
  * 0xd0000000). The 0xf1000000 is the default used by the recent,
  * DT-capable, U-Boot bootloaders provided by Marvell. Some earlier
  * boards were delivered with an older version of the bootloader that
  * left internal registers mapped at 0xd0000000. If you are in this
  * situation, you should either update your bootloader (preferred
  * solution) or the below Device Tree should be adjusted.
  ...

It really boils down to a compile-time option for these platforms,
to "detect" where the internal registers are located  (SPL or not).

Yes.


I prefer adding a
new ranges property anyway.

So please bear with me, and explain (again?) how exactly this new
ranges property could solve this problem. Keeping the undetectable
nature of this address change in mind. And without adding some
more ugly #ifdef CONFIG_SPL_BUILD again. (see also below with your
remark to your similar "pit" solution).

We need to get to the bottom of this 'detection' thing first (as
above). But if the only way of detecting is 'am I in SPL or not' then
you can always have some code that selects the range based on that.

Yes, this is what we need to do.



- Add some sort of 'core' driver model address translation setting,
which your board code can set up with a function call, and
dev_get_addr() uses
- Add a uclass and driver for address translation, and call it from
here (ugh...)

All this doesn't sound very "promising". At least not to me. But
I had another idea, which might be a good alternative. Use a new,
different dts for SPL. This has most likely been discussed before,
not sure. The idea here is, to re-use the existing dts and include
it in the new dts. Something like:

U-Boot proper: armada-xp-gp.dts
SPL U-Boot:    armada-xp-gp-spl.dts

And this new SPL dts includes the original dts and only changes
what needs to get changed for SPL. This could include all the
"u-boot,dm-pre-reloc" properties as well and look like this:

---<-----------------------
/*
   * Device Tree file for Marvell Armada XP development board
   * (DB-MV784MP-GP)
   */

#include "armada-xp-gp.dts"

/ {
          soc {
                  /*
                   * Use 0xd0000000 as base address for the internal registers
                   * in SPL U-Boot
                   */
                  ranges = <MBUS_ID(0xf0, 0x01) 0 0 0xd0000000 0x100000
                            MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000
                            MBUS_ID(0x01, 0x2f) 0 0 0xf0000000 0x1000000>;
                  u-boot,dm-pre-reloc;

                  internal-regs {
                          u-boot,dm-pre-reloc;

                          serial@12000 {
                                  u-boot,dm-pre-reloc;
                          };
                  };
          };
};
---<-----------------------

Unfortunately this approach does not work right now. The dtc
seems to not overlay the new values in the resulting dtb.

But has this approach been discussed before? Or if not, what do
you think of it (if and once it really works)?

The dtc should overlay the values - we depend on this bahaviour in
various places.

Right, I know. But it just doesn't work here right now. I've
unsuccessfully tested it.

Hmmm.


But to me this approach seems a bit clumsy.

I don't find this approach clumsy. Just my personal feeling. But
using this approach we could abstract all the U-Boot properties
and nodes into a separate file. And don't have to make any
changes to original dtsi / dts files.

Of course it would be even better, if all of these U-Boot additions
would be accepted to the main DeviceTree source. But this does
not seem to happen, unfortunately.

Right, and I've complained about that, ineffectively. It is really
unfortunate. Still it is worth sending a patch to the list and see
what happens. I'll make time to send a few more also.


I can't help thinking we are missing something here. The DT is
supposed to represent the hardware, and here we have some hardware
which is not being represented correctly. Your complaint that you will
need to change all the files to add a ranges-initial property (or
whatever) doesn't really seem like a problem to me - you only do it
once.

Not really. We need to change this at least for every now SoC
having this problem. But yes, this should not too much trouble.

We can have a setting as to which property to use, and the
platform can change it when needed. It can fall back to 'ranges' if
there is no ranges-initial property. I used a similar approach for
memory on pit - definitions for both SRAM and SDRAM, and allowing
selection of which one to use. There is nothing in DT that prevents us
from handling this sort of thing.

Again, how does this work exactly? Is there some code for "pit"
that I could take as an example?

You can see an upstream version of something similar here:

http://patchwork.ozlabs.org/patch/402714/

I'll try to find some time to take a look at it tomorrow. Thanks.

I can point you to the Chrome OS tree for the downstream part if you like.

Sure. The more the merrier... ;)


Perhaps you could ask about this on the device-tree-discuss list? If
you like I can start a thread.

Also, just to get this off my chest, I would like to expand a bit more
on why I don't think we should call board code from drivers. The
drivers are supposed to be hermetic, and work on any platform. The
information needed by the driver to work should come from data, not
code. Then we can represent it in the device tree, or in platform data
(to which device tree is often converted). If we have the drivers
calling out to code to get their information, then it adds more
coupling between the drivers and the platforms. At present we can drop
a driver in by enabling it in Kconfig and adding a device tree node.
We don't need to modify the board code at all. I think that is worth
protecting. It creates a nice clear wall between the driver and the
board code, conceptually free of hidden interactions, etc. It is much
easier to scale to larger and more complex platforms with this in
place.

For similar reasons I think we should avoid calling drivers from other
drivers - this should always be done through the driver's API (e.g.
the uclass header file). That way we don't have (e.g.) a SPI flash
driver with a compile- or link-time dependency on a particular SPI
driver. It may depend on features that the SPI driver may or may not
provide, but in principle any SPI driver can provide those features,
and if they are publicly declared in the uclass API it is clear what
these features are.

Thanks for the detailed explanation for your reasoning. I understand
your point of view.

OK good. I do understand that this is restrictive and has costs also.
I'm interested to hear other points of view, etc.

Yes, I would also like to hear other opinions on this. If this
2 lines of call-back code are acceptable for others or not?

Thanks,
Stefan

_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to