[PATCH] ARM: Kirkwood: Fix the internal register ranges translation

[PATCH] ARM: Kirkwood: Fix the internal register ranges translation

[Ezequiel Garcia]

Although the internal register window size is 1 MiB, the previous
ranges translation for the internal register space had a size of
0x4000000. This was done to allow the crypto and nand node to access
the corresponding 'sram' and 'nand' decoding windows.

In order to describe the hardware more accurately, we declare the
real 1 MiB internal register space in the ranges, and add a translation
entry for the nand node to access the 'nand' window.

This commit will make future improvements on the MBus DT binding easier.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
Tested on Plathome Openblocks A6 board.

 arch/arm/boot/dts/kirkwood.dtsi | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/kirkwood.dtsi b/arch/arm/boot/dts/kirkwood.dtsi
index 8a1e3bb..910fabc 100644
--- a/arch/arm/boot/dts/kirkwood.dtsi
+++ b/arch/arm/boot/dts/kirkwood.dtsi
@@ -38,7 +38,8 @@
 
 	ocp at f1000000 {
 		compatible = "simple-bus";
-		ranges = <0x00000000 0xf1000000 0x4000000
+		ranges = <0x00000000 0xf1000000 0x0100000
+		          0xf4000000 0xf4000000 0x0000400
 		          0xf5000000 0xf5000000 0x0000400>;
 		#address-cells = <1>;
 		#size-cells = <1>;
@@ -171,7 +172,7 @@
 			ale = <1>;
 			bank-width = <1>;
 			compatible = "marvell,orion-nand";
-			reg = <0x3000000 0x400>;
+			reg = <0xf4000000 0x400>;
 			chip-delay = <25>;
 			/* set partition map and/or chip-delay in board dts */
 			clocks = <&gate_clk 7>;
-- 
1.8.1.5

[PATCH] ARM: Kirkwood: Fix the internal register ranges translation

[Sebastian Hesselbarth]

On 06/18/2013 05:31 PM, Ezequiel Garcia wrote:
> Although the internal register window size is 1 MiB, the previous
> ranges translation for the internal register space had a size of
> 0x4000000. This was done to allow the crypto and nand node to access
> the corresponding 'sram' and 'nand' decoding windows.
>
> In order to describe the hardware more accurately, we declare the
> real 1 MiB internal register space in the ranges, and add a translation
> entry for the nand node to access the 'nand' window.
>
> This commit will make future improvements on the MBus DT binding easier.
>
> Signed-off-by: Ezequiel Garcia<ezequiel.garcia@free-electrons.com>
> ---
> Tested on Plathome Openblocks A6 board.
>
>   arch/arm/boot/dts/kirkwood.dtsi | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/boot/dts/kirkwood.dtsi b/arch/arm/boot/dts/kirkwood.dtsi
> index 8a1e3bb..910fabc 100644
> --- a/arch/arm/boot/dts/kirkwood.dtsi
> +++ b/arch/arm/boot/dts/kirkwood.dtsi
> @@ -38,7 +38,8 @@
>
>   	ocp at f1000000 {
>   		compatible = "simple-bus";
> -		ranges =<0x00000000 0xf1000000 0x4000000
> +		ranges =<0x00000000 0xf1000000 0x0100000
> +		          0xf4000000 0xf4000000 0x0000400
>   		          0xf5000000 0xf5000000 0x0000400>;

Ezequiel,

maybe you should also put a comment at the end of each ranges
line to name the remap it does?

Also, as already mentioned on IRC, it would be great to also
have dove.dtsi updated accordingly.

Sebastian

>   		#address-cells =<1>;
>   		#size-cells =<1>;
> @@ -171,7 +172,7 @@
>   			ale =<1>;
>   			bank-width =<1>;
>   			compatible = "marvell,orion-nand";
> -			reg =<0x3000000 0x400>;
> +			reg =<0xf4000000 0x400>;
>   			chip-delay =<25>;
>   			/* set partition map and/or chip-delay in board dts */
>   			clocks =<&gate_clk 7>;

[PATCH] ARM: Kirkwood: Fix the internal register ranges translation

[Ezequiel Garcia]

On Tue, Jun 18, 2013 at 09:42:48PM +0200, Sebastian Hesselbarth wrote:
> On 06/18/2013 05:31 PM, Ezequiel Garcia wrote:
> > Although the internal register window size is 1 MiB, the previous
> > ranges translation for the internal register space had a size of
> > 0x4000000. This was done to allow the crypto and nand node to access
> > the corresponding 'sram' and 'nand' decoding windows.
> >
> > In order to describe the hardware more accurately, we declare the
> > real 1 MiB internal register space in the ranges, and add a translation
> > entry for the nand node to access the 'nand' window.
> >
> > This commit will make future improvements on the MBus DT binding easier.
> >
> > Signed-off-by: Ezequiel Garcia<ezequiel.garcia@free-electrons.com>
> > ---
> > Tested on Plathome Openblocks A6 board.
> >
> >   arch/arm/boot/dts/kirkwood.dtsi | 5 +++--
> >   1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm/boot/dts/kirkwood.dtsi b/arch/arm/boot/dts/kirkwood.dtsi
> > index 8a1e3bb..910fabc 100644
> > --- a/arch/arm/boot/dts/kirkwood.dtsi
> > +++ b/arch/arm/boot/dts/kirkwood.dtsi
> > @@ -38,7 +38,8 @@
> >
> >   	ocp at f1000000 {
> >   		compatible = "simple-bus";
> > -		ranges =<0x00000000 0xf1000000 0x4000000
> > +		ranges =<0x00000000 0xf1000000 0x0100000
> > +		          0xf4000000 0xf4000000 0x0000400
> >   		          0xf5000000 0xf5000000 0x0000400>;
> 
> Ezequiel,
> 
> maybe you should also put a comment at the end of each ranges
> line to name the remap it does?
> 
> Also, as already mentioned on IRC, it would be great to also
> have dove.dtsi updated accordingly.
> 

Indeed, I will do both.

Thanks!
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH] ARM: Kirkwood: Fix the internal register ranges translation

[Jason Cooper]

On Tue, Jun 18, 2013 at 04:47:57PM -0300, Ezequiel Garcia wrote:
> On Tue, Jun 18, 2013 at 09:42:48PM +0200, Sebastian Hesselbarth wrote:
> > On 06/18/2013 05:31 PM, Ezequiel Garcia wrote:
> > > Although the internal register window size is 1 MiB, the previous
> > > ranges translation for the internal register space had a size of
> > > 0x4000000. This was done to allow the crypto and nand node to access
> > > the corresponding 'sram' and 'nand' decoding windows.
> > >
> > > In order to describe the hardware more accurately, we declare the
> > > real 1 MiB internal register space in the ranges, and add a translation
> > > entry for the nand node to access the 'nand' window.
> > >
> > > This commit will make future improvements on the MBus DT binding easier.
> > >
> > > Signed-off-by: Ezequiel Garcia<ezequiel.garcia@free-electrons.com>
> > > ---
> > > Tested on Plathome Openblocks A6 board.
> > >
> > >   arch/arm/boot/dts/kirkwood.dtsi | 5 +++--
> > >   1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/arm/boot/dts/kirkwood.dtsi b/arch/arm/boot/dts/kirkwood.dtsi
> > > index 8a1e3bb..910fabc 100644
> > > --- a/arch/arm/boot/dts/kirkwood.dtsi
> > > +++ b/arch/arm/boot/dts/kirkwood.dtsi
> > > @@ -38,7 +38,8 @@
> > >
> > >   	ocp at f1000000 {
> > >   		compatible = "simple-bus";
> > > -		ranges =<0x00000000 0xf1000000 0x4000000
> > > +		ranges =<0x00000000 0xf1000000 0x0100000
> > > +		          0xf4000000 0xf4000000 0x0000400
> > >   		          0xf5000000 0xf5000000 0x0000400>;
> > 
> > Ezequiel,
> > 
> > maybe you should also put a comment at the end of each ranges
> > line to name the remap it does?
> > 
> > Also, as already mentioned on IRC, it would be great to also
> > have dove.dtsi updated accordingly.
> > 
> 
> Indeed, I will do both.

The sooner, the better.  This is probably the last patch to get in for
this merge window for mvebu...

thx,

Jason.

[PATCH] ARM: Kirkwood: Fix the internal register ranges translation

[Ezequiel Garcia]

On Wed, Jun 19, 2013 at 3:36 PM, Jason Cooper <jason@lakedaemon.net> wrote:
> On Tue, Jun 18, 2013 at 04:47:57PM -0300, Ezequiel Garcia wrote:
>> On Tue, Jun 18, 2013 at 09:42:48PM +0200, Sebastian Hesselbarth wrote:
>> > On 06/18/2013 05:31 PM, Ezequiel Garcia wrote:
>> > > Although the internal register window size is 1 MiB, the previous
>> > > ranges translation for the internal register space had a size of
>> > > 0x4000000. This was done to allow the crypto and nand node to access
>> > > the corresponding 'sram' and 'nand' decoding windows.
>> > >
>> > > In order to describe the hardware more accurately, we declare the
>> > > real 1 MiB internal register space in the ranges, and add a translation
>> > > entry for the nand node to access the 'nand' window.
>> > >
>> > > This commit will make future improvements on the MBus DT binding easier.
>> > >
>> > > Signed-off-by: Ezequiel Garcia<ezequiel.garcia@free-electrons.com>
>> > > ---
>> > > Tested on Plathome Openblocks A6 board.
>> > >
>> > >   arch/arm/boot/dts/kirkwood.dtsi | 5 +++--
>> > >   1 file changed, 3 insertions(+), 2 deletions(-)
>> > >
>> > > diff --git a/arch/arm/boot/dts/kirkwood.dtsi b/arch/arm/boot/dts/kirkwood.dtsi
>> > > index 8a1e3bb..910fabc 100644
>> > > --- a/arch/arm/boot/dts/kirkwood.dtsi
>> > > +++ b/arch/arm/boot/dts/kirkwood.dtsi
>> > > @@ -38,7 +38,8 @@
>> > >
>> > >           ocp at f1000000 {
>> > >                   compatible = "simple-bus";
>> > > -         ranges =<0x00000000 0xf1000000 0x4000000
>> > > +         ranges =<0x00000000 0xf1000000 0x0100000
>> > > +                   0xf4000000 0xf4000000 0x0000400
>> > >                             0xf5000000 0xf5000000 0x0000400>;
>> >
>> > Ezequiel,
>> >
>> > maybe you should also put a comment at the end of each ranges
>> > line to name the remap it does?
>> >
>> > Also, as already mentioned on IRC, it would be great to also
>> > have dove.dtsi updated accordingly.
>> >
>>
>> Indeed, I will do both.
>
> The sooner, the better.  This is probably the last patch to get in for
> this merge window for mvebu...
>

Right. Please don't wait for this patch if it's not in your mailbox by tomorrow.

Thanks for the notice!
-- 
    Ezequiel

[PATCH] ARM: Kirkwood: Fix the internal register ranges translation

[Jason Cooper]

On Wed, Jun 19, 2013 at 03:42:27PM -0300, Ezequiel Garcia wrote:
> On Wed, Jun 19, 2013 at 3:36 PM, Jason Cooper <jason@lakedaemon.net> wrote:
> > On Tue, Jun 18, 2013 at 04:47:57PM -0300, Ezequiel Garcia wrote:
> >> On Tue, Jun 18, 2013 at 09:42:48PM +0200, Sebastian Hesselbarth wrote:
> >> > On 06/18/2013 05:31 PM, Ezequiel Garcia wrote:
> >> > > Although the internal register window size is 1 MiB, the previous
> >> > > ranges translation for the internal register space had a size of
> >> > > 0x4000000. This was done to allow the crypto and nand node to access
> >> > > the corresponding 'sram' and 'nand' decoding windows.
> >> > >
> >> > > In order to describe the hardware more accurately, we declare the
> >> > > real 1 MiB internal register space in the ranges, and add a translation
> >> > > entry for the nand node to access the 'nand' window.
> >> > >
> >> > > This commit will make future improvements on the MBus DT binding easier.
> >> > >
> >> > > Signed-off-by: Ezequiel Garcia<ezequiel.garcia@free-electrons.com>
> >> > > ---
> >> > > Tested on Plathome Openblocks A6 board.
> >> > >
> >> > >   arch/arm/boot/dts/kirkwood.dtsi | 5 +++--
> >> > >   1 file changed, 3 insertions(+), 2 deletions(-)
> >> > >
> >> > > diff --git a/arch/arm/boot/dts/kirkwood.dtsi b/arch/arm/boot/dts/kirkwood.dtsi
> >> > > index 8a1e3bb..910fabc 100644
> >> > > --- a/arch/arm/boot/dts/kirkwood.dtsi
> >> > > +++ b/arch/arm/boot/dts/kirkwood.dtsi
> >> > > @@ -38,7 +38,8 @@
> >> > >
> >> > >           ocp at f1000000 {
> >> > >                   compatible = "simple-bus";
> >> > > -         ranges =<0x00000000 0xf1000000 0x4000000
> >> > > +         ranges =<0x00000000 0xf1000000 0x0100000
> >> > > +                   0xf4000000 0xf4000000 0x0000400
> >> > >                             0xf5000000 0xf5000000 0x0000400>;
> >> >
> >> > Ezequiel,
> >> >
> >> > maybe you should also put a comment at the end of each ranges
> >> > line to name the remap it does?
> >> >
> >> > Also, as already mentioned on IRC, it would be great to also
> >> > have dove.dtsi updated accordingly.
> >> >
> >>
> >> Indeed, I will do both.
> >
> > The sooner, the better.  This is probably the last patch to get in for
> > this merge window for mvebu...
> >
> 
> Right. Please don't wait for this patch if it's not in your mailbox by tomorrow.

Ok.

thx,

Jason.

[PATCH] ARM: Kirkwood: Fix the internal register ranges translation

[Jason Cooper]

On Tue, Jun 18, 2013 at 12:31:19PM -0300, Ezequiel Garcia wrote:
> Although the internal register window size is 1 MiB, the previous
> ranges translation for the internal register space had a size of
> 0x4000000. This was done to allow the crypto and nand node to access
> the corresponding 'sram' and 'nand' decoding windows.
> 
> In order to describe the hardware more accurately, we declare the
> real 1 MiB internal register space in the ranges, and add a translation
> entry for the nand node to access the 'nand' window.
> 
> This commit will make future improvements on the MBus DT binding easier.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
> Tested on Plathome Openblocks A6 board.
> 
>  arch/arm/boot/dts/kirkwood.dtsi | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Applied to mvebu/dt

thx,

Jason.

[PATCH] ARM: Kirkwood: Fix the internal register ranges translation

[Gerlando Falauto]

Hi Ezequiel,

apologies in advance for commenting on an already-merged patch.

On 06/18/2013 05:31 PM, Ezequiel Garcia wrote:
> Although the internal register window size is 1 MiB, the previous
> ranges translation for the internal register space had a size of
> 0x4000000. This was done to allow the crypto and nand node to access
> the corresponding 'sram' and 'nand' decoding windows.
>
> In order to describe the hardware more accurately, we declare the
> real 1 MiB internal register space in the ranges, and add a translation
> entry for the nand node to access the 'nand' window.
>
> This commit will make future improvements on the MBus DT binding easier.
>
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
> Tested on Plathome Openblocks A6 board.
>
>   arch/arm/boot/dts/kirkwood.dtsi | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/boot/dts/kirkwood.dtsi b/arch/arm/boot/dts/kirkwood.dtsi
> index 8a1e3bb..910fabc 100644
> --- a/arch/arm/boot/dts/kirkwood.dtsi
> +++ b/arch/arm/boot/dts/kirkwood.dtsi
> @@ -38,7 +38,8 @@
>
>   	ocp at f1000000 {
>   		compatible = "simple-bus";
> -		ranges = <0x00000000 0xf1000000 0x4000000
> +		ranges = <0x00000000 0xf1000000 0x0100000
> +		          0xf4000000 0xf4000000 0x0000400
>   		          0xf5000000 0xf5000000 0x0000400>;

Apart from "consistency" with the following range (0xf5000000) used by 
the crypto node, is there any reason why you did not do something like 
this instead (which Valentin suggested, but I will take the blame for):

-		ranges = <0x00000000 0xf1000000 0x4000000
+		ranges = <0x00000000 0xf1000000 0x0100000
+		          0x03000000 0xf4000000 0x0000400
   		          0xf5000000 0xf5000000 0x0000400>;

This would keep a consistent addressing within the child device bus, and 
avoid a later incosistency between the "unit-address" and the first 
"reg" address:

>   		#address-cells = <1>;
>   		#size-cells = <1>;
> @@ -171,7 +172,7 @@
 >   		nand at 3000000 {
		     ^^^^^^^
 >   			#address-cells = <1>;
 >   			#size-cells = <1>;
 >   			cle = <0>;
>   			ale = <1>;
>   			bank-width = <1>;
>   			compatible = "marvell,orion-nand";
> -			reg = <0x3000000 0x400>;
> +			reg = <0xf4000000 0x400>;
				^^^^^^^^^
>   			chip-delay = <25>;
>   			/* set partition map and/or chip-delay in board dts */
>   			clocks = <&gate_clk 7>;
>

I guess the same could be done for the third range and for the crypto 
node as well:

-		ranges = <0x00000000 0xf1000000 0x4000000
-  		          0xf5000000 0xf5000000 0x0000400>;
+		ranges = <0x00000000 0xf1000000 0x0100000
+		          0x03000000 0xf4000000 0x0000400
+		          0x04000000 0xf5000000 0x0000400>;

		crypto at 30000 {
			compatible = "marvell,orion-crypto";
			reg = <0x30000 0x10000>,
-			      <0xf5000000 0x800>;
+			      <0x04000000 0x800>;
			reg-names = "regs", "sram";
			interrupts = <22>;
			clocks = <&gate_clk 17>;
			status = "okay";
		};

Does the above make sense?
Or would that clash with your latest mvebu-mbus device tree binding?
(Which, BTW, I have not quite understood yet).

Thank you,
Gerlando

[PATCH] ARM: Kirkwood: Fix the internal register ranges translation

[Ezequiel Garcia]

Hi Gerlando,

On Tue, Jul 16, 2013 at 11:37:30AM +0200, Gerlando Falauto wrote:
> 
> apologies in advance for commenting on an already-merged patch.

Sure, no problem.

> 
> On 06/18/2013 05:31 PM, Ezequiel Garcia wrote:
> > Although the internal register window size is 1 MiB, the previous
> > ranges translation for the internal register space had a size of
> > 0x4000000. This was done to allow the crypto and nand node to access
> > the corresponding 'sram' and 'nand' decoding windows.
> >
> > In order to describe the hardware more accurately, we declare the
> > real 1 MiB internal register space in the ranges, and add a translation
> > entry for the nand node to access the 'nand' window.
> >
> > This commit will make future improvements on the MBus DT binding easier.
> >
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > ---
> > Tested on Plathome Openblocks A6 board.
> >
> >   arch/arm/boot/dts/kirkwood.dtsi | 5 +++--
> >   1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm/boot/dts/kirkwood.dtsi b/arch/arm/boot/dts/kirkwood.dtsi
> > index 8a1e3bb..910fabc 100644
> > --- a/arch/arm/boot/dts/kirkwood.dtsi
> > +++ b/arch/arm/boot/dts/kirkwood.dtsi
> > @@ -38,7 +38,8 @@
> >
> >   	ocp at f1000000 {
> >   		compatible = "simple-bus";
> > -		ranges = <0x00000000 0xf1000000 0x4000000
> > +		ranges = <0x00000000 0xf1000000 0x0100000
> > +		          0xf4000000 0xf4000000 0x0000400
> >   		          0xf5000000 0xf5000000 0x0000400>;
> 
> Apart from "consistency" with the following range (0xf5000000) used by 
> the crypto node, is there any reason why you did not do something like 
> this instead (which Valentin suggested, but I will take the blame for):
> 

I'm not sure the reason is "consistency with the crypto node".
There's an MBus window at 0xf4000000 for NAND, and that is what is described
in the snippet above; and this is a better reason.

That said, technically speaking, you can have any translation scheme you want,
as long as it ends up in 0xf4000000.

> -		ranges = <0x00000000 0xf1000000 0x4000000
> +		ranges = <0x00000000 0xf1000000 0x0100000
> +		          0x03000000 0xf4000000 0x0000400
>    		          0xf5000000 0xf5000000 0x0000400>;
> 
> This would keep a consistent addressing within the child device bus, and 

Could you explain how this "keeps a consistent addressing"?
Frankly, I don't understand why you choose 0x3000000 ... am I missing something?

Also, speaking of "device bus" this nand node should be behind a devicebus node.

		ranges = <MBUS_ID(0xf0, 0x01) 0 0 0xf1000000 0x100000   /* internal-regs */
			  MBUS_ID(0x01, 0x2f) 0 0 0xf4000000 0x400>;

		devbus {
			status = "okay";
			ranges = <0 MBUS_ID(0x01, 0x2f) 0 0x400>;

			/* nand */
			nand {
				compatible = "marvell,orion-nand";
				reg = <0 0x400>;
			};
		};

(notice this will allow you to relocate the base address of the NAND windows
easily if it conflicts with your PCIe needs).

> avoid a later incosistency between the "unit-address" and the first 
> "reg" address:
> 
> >   		#address-cells = <1>;
> >   		#size-cells = <1>;
> > @@ -171,7 +172,7 @@
>  >   		nand at 3000000 {
> 		     ^^^^^^^

Oh, this should be fixed. I just missed it, and nobody noticed either.

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH] ARM: Kirkwood: Fix the internal register ranges translation

[Gerlando Falauto]

Hi Ezequiel,

On 07/16/2013 02:56 PM, Ezequiel Garcia wrote:
> Hi Gerlando,
>
> On Tue, Jul 16, 2013 at 11:37:30AM +0200, Gerlando Falauto wrote:
>>
>> apologies in advance for commenting on an already-merged patch.
>
> Sure, no problem.
>
>>
>> On 06/18/2013 05:31 PM, Ezequiel Garcia wrote:
>>> Although the internal register window size is 1 MiB, the previous
>>> ranges translation for the internal register space had a size of
>>> 0x4000000. This was done to allow the crypto and nand node to access
>>> the corresponding 'sram' and 'nand' decoding windows.
>>>
>>> In order to describe the hardware more accurately, we declare the
>>> real 1 MiB internal register space in the ranges, and add a translation
>>> entry for the nand node to access the 'nand' window.
>>>
>>> This commit will make future improvements on the MBus DT binding easier.
>>>
>>> Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>>> ---
>>> Tested on Plathome Openblocks A6 board.
>>>
>>>    arch/arm/boot/dts/kirkwood.dtsi | 5 +++--
>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm/boot/dts/kirkwood.dtsi b/arch/arm/boot/dts/kirkwood.dtsi
>>> index 8a1e3bb..910fabc 100644
>>> --- a/arch/arm/boot/dts/kirkwood.dtsi
>>> +++ b/arch/arm/boot/dts/kirkwood.dtsi
>>> @@ -38,7 +38,8 @@
>>>
>>>    	ocp at f1000000 {
>>>    		compatible = "simple-bus";
>>> -		ranges = <0x00000000 0xf1000000 0x4000000
>>> +		ranges = <0x00000000 0xf1000000 0x0100000
>>> +		          0xf4000000 0xf4000000 0x0000400
>>>    		          0xf5000000 0xf5000000 0x0000400>;
>>
>> Apart from "consistency" with the following range (0xf5000000) used by
>> the crypto node, is there any reason why you did not do something like
>> this instead (which Valentin suggested, but I will take the blame for):
>>
>
> I'm not sure the reason is "consistency with the crypto node".
> There's an MBus window at 0xf4000000 for NAND, and that is what is described
> in the snippet above; and this is a better reason.
>
> That said, technically speaking, you can have any translation scheme you want,
> as long as it ends up in 0xf4000000.
>
>> -		ranges = <0x00000000 0xf1000000 0x4000000
>> +		ranges = <0x00000000 0xf1000000 0x0100000
>> +		          0x03000000 0xf4000000 0x0000400
>>     		          0xf5000000 0xf5000000 0x0000400>;
>>
>> This would keep a consistent addressing within the child device bus, and
>
> Could you explain how this "keeps a consistent addressing"?
> Frankly, I don't understand why you choose 0x3000000 ... am I missing something?

Actually, the only one missing something here is myself, as I do not 
quite have the full picture :-)
Mine was a bit of a "reverse allocation" -- we know we must end up at 
0xf4000000. Since all the addresses are shifted by 0xf1000000, here 
comes 0x03000000... But please see my last question further down.

>
> Also, speaking of "device bus" this nand node should be behind a devicebus node.
>
> 		ranges = <MBUS_ID(0xf0, 0x01) 0 0 0xf1000000 0x100000   /* internal-regs */
> 			  MBUS_ID(0x01, 0x2f) 0 0 0xf4000000 0x400>;
>
> 		devbus {
> 			status = "okay";
> 			ranges = <0 MBUS_ID(0x01, 0x2f) 0 0x400>;
>
> 			/* nand */
> 			nand {
> 				compatible = "marvell,orion-nand";
> 				reg = <0 0x400>;
> 			};
> 		};

I believe that makes a lot more sense this way... I guess this feature 
(device bus) requires your latest set of patches, right? (either v7 as 
you posted yesterday or your tree at 
git://github.com/MISL-EBU-System-SW/mainline-public.git/marvell-mvebu-mbus-v7)

> (notice this will allow you to relocate the base address of the NAND windows
> easily if it conflicts with your PCIe needs).

I sort of had the impression I could do already do that somehow, though 
I am not quite sure anymore...

>> avoid a later incosistency between the "unit-address" and the first
>> "reg" address:
>>
>>>    		#address-cells = <1>;
>>>    		#size-cells = <1>;
>>> @@ -171,7 +172,7 @@
>>   >   		nand at 3000000 {
>> 		     ^^^^^^^
>
> Oh, this should be fixed. I just missed it, and nobody noticed either.
>

So, in the end, you think it's OK to have a set of nodes with "relative" 
addresses (gpio at 10140, serial at 12000 etc...) and some with "absolute" 
addresses (nand at 0xf4000000, where the ranges property does a 0-offset 
translation)?
Even though I understand this is just some transitional state, and it 
will all be fixed like your example above, once we get the rest of the 
rework merged (mbus/devbus).

Thanks a lot for your patience!
Gerlando

[PATCH] ARM: Kirkwood: Fix the internal register ranges translation

[Ezequiel Garcia]

Gerlando,

On Tue, Jul 16, 2013 at 08:51:37PM +0200, Gerlando Falauto wrote:
[...]
> >
> > Also, speaking of "device bus" this nand node should be behind a devicebus node.
> >
> > 		ranges = <MBUS_ID(0xf0, 0x01) 0 0 0xf1000000 0x100000   /* internal-regs */
> > 			  MBUS_ID(0x01, 0x2f) 0 0 0xf4000000 0x400>;
> >
> > 		devbus {
> > 			status = "okay";
> > 			ranges = <0 MBUS_ID(0x01, 0x2f) 0 0x400>;
> >
> > 			/* nand */
> > 			nand {
> > 				compatible = "marvell,orion-nand";
> > 				reg = <0 0x400>;
> > 			};
> > 		};
> 
> I believe that makes a lot more sense this way... I guess this feature 
> (device bus) requires your latest set of patches, right? (either v7 as 
> you posted yesterday or your tree at 
> git://github.com/MISL-EBU-System-SW/mainline-public.git/marvell-mvebu-mbus-v7)
> 

No, not really. The device-bus driver is already merged, so we only need
to use it, and maybe extend it a bit to support kirkwood (in case it's
needed). You can see in:

  arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts

for an example.

I would prefer that to be done *after* the MBus is accepted...
but that's just a personal preference.

> > (notice this will allow you to relocate the base address of the NAND windows
> > easily if it conflicts with your PCIe needs).
> 
> I sort of had the impression I could do already do that somehow, though 
> I am not quite sure anymore...
> 

Well, you can do it by patching and re-building the kernel as the
MBus windows are created from C-files.

With the MBus DT binding, all you need is to change the DT.

> >> avoid a later incosistency between the "unit-address" and the first
> >> "reg" address:
> >>
> >>>    		#address-cells = <1>;
> >>>    		#size-cells = <1>;
> >>> @@ -171,7 +172,7 @@
> >>   >   		nand at 3000000 {
> >> 		     ^^^^^^^
> >
> > Oh, this should be fixed. I just missed it, and nobody noticed either.
> >
> 
> So, in the end, you think it's OK to have a set of nodes with "relative" 
> addresses (gpio at 10140, serial at 12000 etc...) and some with "absolute" 
> addresses (nand at 0xf4000000, where the ranges property does a 0-offset 
> translation)?
> Even though I understand this is just some transitional state, and it 
> will all be fixed like your example above, once we get the rest of the 
> rework merged (mbus/devbus).

Mmm.. I see your point. In the end, it's just as you say, the current state
is only transitional. There's no way to have a proper DT without the MBus DT
binding.

The reason for this mess is that the current layout mixes the actual
address space. Feel free to take a look at the latest MBus series and
try to add MBus DT binding for Kirkwood.

If you do, don't hesitate in asking as many questions as you need and/or
posting RFCs for us to look at.

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH] ARM: Kirkwood: Fix the internal register ranges translation

[Gerlando Falauto]

Hi Ezequiel,

On 07/16/2013 02:56 PM, Ezequiel Garcia wrote:
[...]
> Also, speaking of "device bus" this nand node should be behind a devicebus node.
>
> 		ranges = <MBUS_ID(0xf0, 0x01) 0 0 0xf1000000 0x100000   /* internal-regs */
> 			  MBUS_ID(0x01, 0x2f) 0 0 0xf4000000 0x400>;
>
> 		devbus {
> 			status = "okay";
> 			ranges = <0 MBUS_ID(0x01, 0x2f) 0 0x400>;
>
> 			/* nand */
> 			nand {
> 				compatible = "marvell,orion-nand";
> 				reg = <0 0x400>;
> 			};
> 		};
>
> (notice this will allow you to relocate the base address of the NAND windows
> easily if it conflicts with your PCIe needs).

I am MAYBE slowly starting to understand this whole mbus rework.
Just one remark though: don't you think it would make sense to add 
something like:

#define MBUS_ID_INTERNAL_REGS	MBUS_ID(0xf0, 0x01)
#define MBUS_ID_NAND		MBUS_ID(0x01, 0x2f)

I personally have a hard time reading numeric values for 
GPIO_ACTIVE_LOW/GPIO_ACTIVE_HIGH.

Thanks,
Gerlando

>
>> avoid a later incosistency between the "unit-address" and the first
>> "reg" address:
>>
>>>    		#address-cells = <1>;
>>>    		#size-cells = <1>;
>>> @@ -171,7 +172,7 @@
>>   >   		nand at 3000000 {
>> 		     ^^^^^^^
>
> Oh, this should be fixed. I just missed it, and nobody noticed either.
>

[PATCH] ARM: Kirkwood: Fix the internal register ranges translation

[Ezequiel Garcia]

Hi Gerlando,

On Wed, Jul 17, 2013 at 08:35:38AM +0200, Gerlando Falauto wrote:
> On 07/16/2013 02:56 PM, Ezequiel Garcia wrote:
> [...]
> > Also, speaking of "device bus" this nand node should be behind a devicebus node.
> >
> > 		ranges = <MBUS_ID(0xf0, 0x01) 0 0 0xf1000000 0x100000   /* internal-regs */
> > 			  MBUS_ID(0x01, 0x2f) 0 0 0xf4000000 0x400>;
> >
> > 		devbus {
> > 			status = "okay";
> > 			ranges = <0 MBUS_ID(0x01, 0x2f) 0 0x400>;
> >
> > 			/* nand */
> > 			nand {
> > 				compatible = "marvell,orion-nand";
> > 				reg = <0 0x400>;
> > 			};
> > 		};
> >
> > (notice this will allow you to relocate the base address of the NAND windows
> > easily if it conflicts with your PCIe needs).
> 
> I am MAYBE slowly starting to understand this whole mbus rework.
> Just one remark though: don't you think it would make sense to add 
> something like:
> 
> #define MBUS_ID_INTERNAL_REGS	MBUS_ID(0xf0, 0x01)
> #define MBUS_ID_NAND		MBUS_ID(0x01, 0x2f)
> 

Yeah, maybe it would make sense. This has been discussed in the past
and others were against, so that's the reason it's not included in
the series I submitted.

But feel free to send a patch proposing it once the MBus is merged!

Thanks,
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com