[PATCH] ARM: mvebu: use 0xf1000000 as internal registers on Armada 370 DB

[PATCH] ARM: mvebu: use 0xf1000000 as internal registers on Armada 370 DB

[Thomas Petazzoni]

The Armada 370 DB boards are now being used with a more recent version
of U-Boot, which has Device Tree support, and remaps internal
registers at 0xf1000000 like most of the other Marvell EBU platforms.

Consequently, this commit updates the Device Tree describing this
platform to use this new internal register address, so that the
mainline kernel can easily be used on such boards that use a modern
bootloader. A comment is also added to warn users who could have
boards with an older bootloader version.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/boot/dts/armada-370-db.dts | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/armada-370-db.dts b/arch/arm/boot/dts/armada-370-db.dts
index e993c46..dede3e7 100644
--- a/arch/arm/boot/dts/armada-370-db.dts
+++ b/arch/arm/boot/dts/armada-370-db.dts
@@ -45,6 +45,15 @@
  *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
  *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
  *     OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * 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.
  */
 
 /dts-v1/;
@@ -64,7 +73,7 @@
 	};
 
 	soc {
-		ranges = <MBUS_ID(0xf0, 0x01) 0 0xd0000000 0x100000
+		ranges = <MBUS_ID(0xf0, 0x01) 0 0xf1000000 0x100000
 			  MBUS_ID(0x01, 0xe0) 0 0xfff00000 0x100000>;
 
 		internal-regs {
-- 
2.1.0

[PATCH] ARM: mvebu: use 0xf1000000 as internal registers on Armada 370 DB

[Arnd Bergmann]

On Thursday 02 April 2015 11:16:24 Thomas Petazzoni wrote:
> The Armada 370 DB boards are now being used with a more recent version
> of U-Boot, which has Device Tree support, and remaps internal
> registers at 0xf1000000 like most of the other Marvell EBU platforms.
> 
> Consequently, this commit updates the Device Tree describing this
> platform to use this new internal register address, so that the
> mainline kernel can easily be used on such boards that use a modern
> bootloader. A comment is also added to warn users who could have
> boards with an older bootloader version.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

I think it would be better to add a new .dts file that includes the
former, and give it a new name, such as armada-370-db-broken-uboot.dts.

We should not stop supporting the older versions, and it would be better
to revert the bootloader change in their u-boot before it spreads too far.

	Arnd

[PATCH] ARM: mvebu: use 0xf1000000 as internal registers on Armada 370 DB

[Andrew Lunn]

On Thu, Apr 02, 2015 at 03:28:09PM +0200, Arnd Bergmann wrote:
> On Thursday 02 April 2015 11:16:24 Thomas Petazzoni wrote:
> > The Armada 370 DB boards are now being used with a more recent version
> > of U-Boot, which has Device Tree support, and remaps internal
> > registers at 0xf1000000 like most of the other Marvell EBU platforms.
> > 
> > Consequently, this commit updates the Device Tree describing this
> > platform to use this new internal register address, so that the
> > mainline kernel can easily be used on such boards that use a modern
> > bootloader. A comment is also added to warn users who could have
> > boards with an older bootloader version.
> > 
> > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> 
> I think it would be better to add a new .dts file that includes the
> former, and give it a new name, such as armada-370-db-broken-uboot.dts.
> 
> We should not stop supporting the older versions, and it would be better
> to revert the bootloader change in their u-boot before it spreads too far.

Hi Arnd

Changing to 0xf100000 is the revert. Kirkwood and Dove are at
0xf100000 and the regression was introduced with early Armada XP
boards and spread to 370.

       Andrew

[PATCH] ARM: mvebu: use 0xf1000000 as internal registers on Armada 370 DB

[Thomas Petazzoni]

Dear Arnd Bergmann,

On Thu, 02 Apr 2015 15:28:09 +0200, Arnd Bergmann wrote:

> I think it would be better to add a new .dts file that includes the
> former, and give it a new name, such as armada-370-db-broken-uboot.dts.
> 
> We should not stop supporting the older versions, and it would be better
> to revert the bootloader change in their u-boot before it spreads too far.

Sorry, but I don't follow you.

What was wrong was the initial choice in U-Boot made to keep internal
registers at 0xd0000000. Having the registers at 0xf1000000 is what
should have been done from the beginning: it is the right thing to do,
and there is absolutely no reason to do a revert a go back to
0xd0000000.

Actually, having internal registers at 0xd0000000 is a very bad choice,
as it means that more space in the 0 -> 4 GB physical space is devoted
to I/O, which overlaps with RAM when you have 4+ GB of RAM. One of the
reason with moving the registers at 0xf1000000 is to be able to use as
much RAM as possible.

Also, the proposed change is exactly the same we've done on several
Marvell evaluation boards in the past:

  http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/arch/arm/boot/dts/armada-xp-gp.dts?id=91ed32200e6ea1df19df01355c5c7747f9014102
  http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/arch/arm/boot/dts/armada-xp-db.dts?id=82066bdb5a759ec00c18f9667853c4fe8840e83d

Those boards are evaluation boards from Marvell, only available to
selected customers, who are supposed to know what they are doing. And
all of those customers are getting boards with a new version of the
bootloader that remaps registers at 0xf1000000.

So the patch I'm proposing is simply fixing the kernel so that it works
for the customers who have access to this Armada 370 DB board.

If you want a file named armada-370-db-broken-uboot.dts, then it should
be the one using the 0xd0000000 register address. We can do that if you
like it, but this .dts file will just be useless clutter as nobody will
ever use it.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH] ARM: mvebu: use 0xf1000000 as internal registers on Armada 370 DB

[Arnd Bergmann]

On Thursday 02 April 2015 15:36:50 Thomas Petazzoni wrote:
> 
> On Thu, 02 Apr 2015 15:28:09 +0200, Arnd Bergmann wrote:
> 
> > I think it would be better to add a new .dts file that includes the
> > former, and give it a new name, such as armada-370-db-broken-uboot.dts.
> > 
> > We should not stop supporting the older versions, and it would be better
> > to revert the bootloader change in their u-boot before it spreads too far.
> 
> Sorry, but I don't follow you.
> 
> What was wrong was the initial choice in U-Boot made to keep internal
> registers at 0xd0000000. Having the registers at 0xf1000000 is what
> should have been done from the beginning: it is the right thing to do,
> and there is absolutely no reason to do a revert a go back to
> 0xd0000000.
> 
> Actually, having internal registers at 0xd0000000 is a very bad choice,
> as it means that more space in the 0 -> 4 GB physical space is devoted
> to I/O, which overlaps with RAM when you have 4+ GB of RAM. One of the
> reason with moving the registers at 0xf1000000 is to be able to use as
> much RAM as possible.

This device has 1GB, doing an incompatible change to an existing machine
is completely moronic. How hard can it be to special-case this board
in u-boot?

> Also, the proposed change is exactly the same we've done on several
> Marvell evaluation boards in the past:
> 
>   http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/arch/arm/boot/dts/armada-xp-gp.dts?id=91ed32200e6ea1df19df01355c5c7747f9014102
>   http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/arch/arm/boot/dts/armada-xp-db.dts?id=82066bdb5a759ec00c18f9667853c4fe8840e83d

I'm aware of those, I just wish they would learn from their mistakes.

> Those boards are evaluation boards from Marvell, only available to
> selected customers, who are supposed to know what they are doing. And
> all of those customers are getting boards with a new version of the
> bootloader that remaps registers at 0xf1000000.
>
> So the patch I'm proposing is simply fixing the kernel so that it works
> for the customers who have access to this Armada 370 DB board.
> 
> If you want a file named armada-370-db-broken-uboot.dts, then it should
> be the one using the 0xd0000000 register address. We can do that if you
> like it, but this .dts file will just be useless clutter as nobody will
> ever use it.

Find a different name then, but don't change the value in the existing
file. It's been around for too long now. The other ones were changed
shortly after being introduced, but anybody who is using
armada-370-db.dts today obviously has the old boot loader and cannot
update the bootloader without breaking their kernels, but they should
at least be able to update their kernels without changing the
scripts.

	Arnd

[PATCH] ARM: mvebu: use 0xf1000000 as internal registers on Armada 370 DB

[Thomas Petazzoni]

Arnd,

On Thu, 02 Apr 2015 16:09:03 +0200, Arnd Bergmann wrote:

> This device has 1GB, doing an incompatible change to an existing machine
> is completely moronic. How hard can it be to special-case this board
> in u-boot?

The device has a DIMM slot for RAM. 1 GB in the DT is just an example,
and it gets updated through ATAGS, or directly by the bootloader with
the real amount of RAM that you have, which can go up to 4 GB.

> > Also, the proposed change is exactly the same we've done on several
> > Marvell evaluation boards in the past:
> > 
> >   http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/arch/arm/boot/dts/armada-xp-gp.dts?id=91ed32200e6ea1df19df01355c5c7747f9014102
> >   http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/arch/arm/boot/dts/armada-xp-db.dts?id=82066bdb5a759ec00c18f9667853c4fe8840e83d
> 
> I'm aware of those, I just wish they would learn from their mistakes.

There was no mistake at all. You have no idea what the history of this
problem is, and you're giving lessons.

As Andrew Lunn said, on *all* Marvell EBU platforms the internal
registers have always been remapped at 0xf1000000. Look at Kirkwood,
Orion, Dove.

Marvell wanted to do exactly the same for Armada 370 and XP, which is
perfectly sane: keep the same behavior.

Except that the very early steppings of Armada 370 and Armada XP had a
bug, which prevented from changing the internal register base address,
so they had no other choice but to keep it at 0xd0000000. And then,
once those early steppings were replaced by real production steppings,
the bug was fixed, and they migrated to 0xf1000000, and the situation
was back to normal.

> Find a different name then, but don't change the value in the existing
> file. It's been around for too long now. The other ones were changed
> shortly after being introduced, but anybody who is using

Not at all. The others were also changed long after the board was
introduced in the kernel. Not as long as for the Armada 370 DB, because
I only recently put my hand on an eval board which has a production
stepping of the SoC.

> armada-370-db.dts today obviously has the old boot loader and cannot
> update the bootloader without breaking their kernels, but they should
> at least be able to update their kernels without changing the
> scripts.

All customers have evaluation boards with a production stepping of the
silicon, and for all of them the current kernel is *not* working.

The patch I'm proposing is what makes the kernel work for them.

Arnd, please don't get in the way of platform maintainers for such very
platform-specific issues for which you don't have the background about
what happened and why we're doing this change. It's the responsibility
of the platform maintainers to decide how and when they want to keep or
break the compatibility.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH] ARM: mvebu: use 0xf1000000 as internal registers on Armada 370 DB

[Russell King - ARM Linux]

On Thu, Apr 02, 2015 at 04:16:26PM +0200, Thomas Petazzoni wrote:
> All customers have evaluation boards with a production stepping of the
> silicon, and for all of them the current kernel is *not* working.
> 
> The patch I'm proposing is what makes the kernel work for them.
> 
> Arnd, please don't get in the way of platform maintainers for such very
> platform-specific issues for which you don't have the background about
> what happened and why we're doing this change. It's the responsibility
> of the platform maintainers to decide how and when they want to keep or
> break the compatibility.

Wrong.  It's Arnd's responsibility to make sure that platform maintainers
don't do stupid things like change the DT in incompatible ways which
break lots of people's setups.

However, it is _your_ responsibility as the platform maintainer to place
sufficient information into the commit message that explains why the
change is necessary, and justifies the change.  Much of what you've said
in these follow-on emails should have been in the original message to
explain what the situation is, and why the change is appropriate.

I'm willing to bet that if the commit description had explained it
properly, Arnd would have applied it by now.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH] ARM: mvebu: use 0xf1000000 as internal registers on Armada 370 DB

[Thomas Petazzoni]

Dear Russell King - ARM Linux,

On Thu, 2 Apr 2015 15:48:24 +0100, Russell King - ARM Linux wrote:

> Wrong.  It's Arnd's responsibility to make sure that platform maintainers
> don't do stupid things like change the DT in incompatible ways which
> break lots of people's setups.
> 
> However, it is _your_ responsibility as the platform maintainer to place

I am _not_ a platform maintainer. But thanks for supposing so :-)

> sufficient information into the commit message that explains why the
> change is necessary, and justifies the change.  Much of what you've said
> in these follow-on emails should have been in the original message to
> explain what the situation is, and why the change is appropriate.
> 
> I'm willing to bet that if the commit description had explained it
> properly, Arnd would have applied it by now.

Agreed that the commit message could have been better. Though the
commit message is sufficiently clear for the platform maintainers to
understand, since they have all the background about the platform
needed to understand it. I don't think all commit messages in the
kernel are meant to be understood by developers having no knowledge of
the area touched by the patch.

My commit log has way enough details for the platform maintainers
Andrew Lunn, Jason Cooper, Gregory Clement and Sebastian Hesselbarth to
make up their mind about the patch.

See the reaction from both Andrew and Jason: they understood the patch
and basically agree with it.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH] ARM: mvebu: use 0xf1000000 as internal registers on Armada 370 DB

[Russell King - ARM Linux]

On Thu, Apr 02, 2015 at 05:02:53PM +0200, Thomas Petazzoni wrote:
> Dear Russell King - ARM Linux,
> 
> On Thu, 2 Apr 2015 15:48:24 +0100, Russell King - ARM Linux wrote:
> 
> > Wrong.  It's Arnd's responsibility to make sure that platform maintainers
> > don't do stupid things like change the DT in incompatible ways which
> > break lots of people's setups.
> > 
> > However, it is _your_ responsibility as the platform maintainer to place
> 
> I am _not_ a platform maintainer. But thanks for supposing so :-)

Right, that's because your statements in your emails are ambiguous.

> > sufficient information into the commit message that explains why the
> > change is necessary, and justifies the change.  Much of what you've said
> > in these follow-on emails should have been in the original message to
> > explain what the situation is, and why the change is appropriate.
> > 
> > I'm willing to bet that if the commit description had explained it
> > properly, Arnd would have applied it by now.
> 
> Agreed that the commit message could have been better. Though the
> commit message is sufficiently clear for the platform maintainers to
> understand, since they have all the background about the platform
> needed to understand it. I don't think all commit messages in the
> kernel are meant to be understood by developers having no knowledge of
> the area touched by the patch.

Wrong.  The commit message is supposed to be there so that everyone
can understand why the commit is necessary.  For example, you will
often see Andrew Morton regularly complaining about the poor quality
of commit messages on patches he ends up dealing with, and they're
always the same kinds of issues: lack of understandable justification
for the patch.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH] ARM: mvebu: use 0xf1000000 as internal registers on Armada 370 DB

[Jason Cooper]

Thomas,

On Thu, Apr 02, 2015 at 04:16:26PM +0200, Thomas Petazzoni wrote:
> Except that the very early steppings of Armada 370 and Armada XP had a
> bug, which prevented from changing the internal register base address,
> so they had no other choice but to keep it at 0xd0000000. And then,
> once those early steppings were replaced by real production steppings,
> the bug was fixed, and they migrated to 0xf1000000, and the situation
> was back to normal.

fwiw, I agree with the change, and I appreciate this background info.
It's comforting to hear that it was a hardware bug, not just a "well, we
forgot to remap in u-boot, oh well."

> Arnd, please don't get in the way of platform maintainers for such very
> platform-specific issues for which you don't have the background about
> what happened and why we're doing this change. It's the responsibility
> of the platform maintainers to decide how and when they want to keep or
> break the compatibility.

Inhale, exhale, zen. :-)  Arnd is doing exactly what he's supposed to be
doing.  He spotted something that doesn't appear to match the big
picture of what we're trying to do with DT.  He questioned it.  It's our
job to give him a clear, technical argument for why we're making the
change.  To put it bluntly: him, Olof, and others are our sanity check.

If we can't provide a sound technical reasoning for the change(s), then
they shouldn't accept a branch containing them.

thx,

Jason.

[PATCH] ARM: mvebu: use 0xf1000000 as internal registers on Armada 370 DB

[Gregory CLEMENT]

Hi Arnd,

On 02/04/2015 16:16, Thomas Petazzoni wrote:
> Arnd,
> 
> On Thu, 02 Apr 2015 16:09:03 +0200, Arnd Bergmann wrote:
> 
>> This device has 1GB, doing an incompatible change to an existing machine
>> is completely moronic. How hard can it be to special-case this board
>> in u-boot?
> 
> The device has a DIMM slot for RAM. 1 GB in the DT is just an example,
> and it gets updated through ATAGS, or directly by the bootloader with
> the real amount of RAM that you have, which can go up to 4 GB.
> 
>>> Also, the proposed change is exactly the same we've done on several
>>> Marvell evaluation boards in the past:
>>>
>>>   http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/arch/arm/boot/dts/armada-xp-gp.dts?id=91ed32200e6ea1df19df01355c5c7747f9014102
>>>   http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/arch/arm/boot/dts/armada-xp-db.dts?id=82066bdb5a759ec00c18f9667853c4fe8840e83d
>>
>> I'm aware of those, I just wish they would learn from their mistakes.
> 
> There was no mistake at all. You have no idea what the history of this
> problem is, and you're giving lessons.
> 
> As Andrew Lunn said, on *all* Marvell EBU platforms the internal
> registers have always been remapped at 0xf1000000. Look at Kirkwood,
> Orion, Dove.
> 
> Marvell wanted to do exactly the same for Armada 370 and XP, which is
> perfectly sane: keep the same behavior.
> 
> Except that the very early steppings of Armada 370 and Armada XP had a
> bug, which prevented from changing the internal register base address,
> so they had no other choice but to keep it at 0xd0000000. And then,
> once those early steppings were replaced by real production steppings,
> the bug was fixed, and they migrated to 0xf1000000, and the situation
> was back to normal.

As Andrew and Jason seem also agree with this patch, I would like to apply
on it on the mvebu tree. However there is no point to do it if at the end
you won't take it. Given the new information given by Thomas, do you agree
with this patch?

Thanks,

Gregory


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

[PATCH] ARM: mvebu: use 0xf1000000 as internal registers on Armada 370 DB

[Andrew Lunn]

> As Andrew and Jason seem also agree with this patch, I would like to apply
> on it on the mvebu tree. However there is no point to do it if at the end
> you won't take it. Given the new information given by Thomas, do you agree
> with this patch?

It seems like Thomas should maybe include some of this discussion in
the changelog, just to add a bit more justification.

    Andrew

[PATCH] ARM: mvebu: use 0xf1000000 as internal registers on Armada 370 DB

[Thomas Petazzoni]

Dear Andrew Lunn,

On Thu, 2 Apr 2015 17:33:51 +0200, Andrew Lunn wrote:
> > As Andrew and Jason seem also agree with this patch, I would like to apply
> > on it on the mvebu tree. However there is no point to do it if at the end
> > you won't take it. Given the new information given by Thomas, do you agree
> > with this patch?
> 
> It seems like Thomas should maybe include some of this discussion in
> the changelog, just to add a bit more justification.

I will post an updated version of the patch, with more background in
the commit message, as suggested by Arnd and Russell.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com