diff mbox

[v3,03/12] bus: mvebu-mbus: Add static window allocation to the DT binding

Message ID 1371554737-25319-4-git-send-email-ezequiel.garcia@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ezequiel Garcia June 18, 2013, 11:25 a.m. UTC
This patch adds static window allocation to the device tree binding.
Each first-child of the mbus-compatible node, with a suitable 'ranges'
property, declaring an address translation, will trigger an address
decoding window allocation.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 .../devicetree/bindings/bus/mvebu-mbus.txt         | 168 +++++++++++++++++++++
 drivers/bus/mvebu-mbus.c                           | 121 ++++++++++++++-
 2 files changed, 288 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/bus/mvebu-mbus.txt

Comments

Arnd Bergmann June 18, 2013, 4:14 p.m. UTC | #1
On Tuesday 18 June 2013, Ezequiel Garcia wrote:
> +Required properties:
> +
> +- compatible:	 Should be set to one of the following:
> +		 marvell,armada370-mbus
> +		 marvell,armadaxp-mbus
> +
> +- reg:		 Device's register space.
> +		 Two entries are expected, see the examples below.
> +		 The first one controls the devices decoding window and
> +		 the second one controls the SDRAM decoding window.
> +
> +- address-cells: Must be '2'. The first cell for the MBus ID encoding,
> +                 the second cell for the address offset within the window.
> +
> +- size-cells:    Must be '1'.
> +
> +- ranges:        Must be set up to provide a proper translation for each child.
> +	         See the examples below.

You should explain here what the policy is regarding windows set up by the
boot loader. Are the ranges in here required to be the ones that the boot
loader has preconfigured, or are they the ones that the mbus driver is supposed
to set up?

> +Each child device needs at least a 'ranges' property. If the child is avaiable
> +(i.e. status not 'disabled'), then the MBus driver creates a decoding window
> +for it. For instance, in the example below the BootROM child is specified:
> +
> +	soc {
> +		compatible = "marvell,armada370-mbus", "simple-bus";
> +		reg = <0xd0020000 0x100>, <0xd0020180 0x20>;
> +		#address-cells = <2>;
> +		#size-cells = <1>;
> +
> +		ranges = < ... /* other entries */
> +			   0x011d0000 0 0 0xfff00000 0x100000>;
> +
> +		bootrom {
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges = <0 0x011d0000 0 0x100000>;
> +		};
> +
> +		/* other children */
> +		...
> +	};

Do you really want to require the child to provide a "ranges" property?
I think this makes it more complicated to specify devices that belong
into the "internal-regs" category.

Is this mainly for convenience when settup up the windows?

> +
> +			ranges =
> +			       <0x82000000 0 0x40000 0xffff0001 0x40000 0 0x00002000   /* Port 0.0 registers */
> +				0x82000000 0 0x42000 0xffff0001 0x42000 0 0x00002000   /* Port 2.0 registers */
> +				0x82000000 0 0x44000 0xffff0001 0x44000 0 0x00002000   /* Port 0.1 registers */
> +				0x82000000 0 0x48000 0xffff0001 0x48000 0 0x00002000   /* Port 0.2 registers */
> +				0x82000000 0 0x4c000 0xffff0001 0x4c000 0 0x00002000   /* Port 0.3 registers */
> +				0x82000000 0 0x80000 0xffff0001 0x80000 0 0x00002000   /* Port 1.0 registers */
> +				0x82000000 0 0x82000 0xffff0001 0x82000 0 0x00002000   /* Port 3.0 registers */
> +				0x82000000 0 0xe0000000 0xffff0002 0 0 0x08000000   /* non-prefetchable memory */
> +				0x81000000 0 0 0xffff0002 0x8000000 0 0x00100000>; /* downstream I/O */

Using 0xffff0002 as a placeholder for the pcie translation is definitely
better than 0xffff0000 as you had before, but let me ask again in case
you missed it the last time (and sorry if I missed the answer):

Why not just put the actual translation here the way it happens for each
of the PCIe ports? With the definition here, the PCIe driver actually has no
way to figure out what settings the windows need to use!

On a side note, is there a reason why you use 0xffff0001 for the internal-regs
rather than just 0x1? This one isn't important as they both work as well, it's
just more to write your way.

	Arnd
Thomas Petazzoni June 18, 2013, 5:12 p.m. UTC | #2
Dear Arnd Bergmann,

On Tue, 18 Jun 2013 18:14:33 +0200, Arnd Bergmann wrote:

> Using 0xffff0002 as a placeholder for the pcie translation is definitely
> better than 0xffff0000 as you had before, but let me ask again in case
> you missed it the last time (and sorry if I missed the answer):
> 
> Why not just put the actual translation here the way it happens for each
> of the PCIe ports? With the definition here, the PCIe driver actually has no
> way to figure out what settings the windows need to use!

Come on Arnd, this is something we have already discussed *countless*
times with you.

We *cannot* define translations for each PCIe port because we don't
know in advance how much I/O and memory space each PCIe device will
request. This is the reason why we have *one* global range for I/O
space and *one* global space for memory space, that are given to the
Linux PCI core, which then dynamically assigns sub-ranges for each
PCIe device into those two global ranges.

Best regards,

Thomas
Arnd Bergmann June 18, 2013, 5:16 p.m. UTC | #3
On Tuesday 18 June 2013, Thomas Petazzoni wrote:
> Dear Arnd Bergmann,
> 
> On Tue, 18 Jun 2013 18:14:33 +0200, Arnd Bergmann wrote:
> 
> > Using 0xffff0002 as a placeholder for the pcie translation is definitely
> > better than 0xffff0000 as you had before, but let me ask again in case
> > you missed it the last time (and sorry if I missed the answer):
> > 
> > Why not just put the actual translation here the way it happens for each
> > of the PCIe ports? With the definition here, the PCIe driver actually has no
> > way to figure out what settings the windows need to use!
> 
> Come on Arnd, this is something we have already discussed countless
> times with you.
> 
> We cannot define translations for each PCIe port because we don't
> know in advance how much I/O and memory space each PCIe device will
> request. This is the reason why we have one global range for I/O
> space and one global space for memory space, that are given to the
> Linux PCI core, which then dynamically assigns sub-ranges for each
> PCIe device into those two global ranges.

Could you just stop assuming that I'm an idiot and read my suggestion?

The information that this puts into the device-tree is not where things
are mapped by how things are connected in hardware. The mbus mapping
is the part that is variable, while the PCI mapping is actually fixed
in hardware.

	Arnd
Jason Gunthorpe June 18, 2013, 5:46 p.m. UTC | #4
On Tue, Jun 18, 2013 at 08:25:28AM -0300, Ezequiel Garcia wrote:
> +
> +IIAA0000
> +
> +Where:
> +  -- I = Marvell defined target ID for programmable windows
> +  -- A = Marvell defined target attributes for programmable windows

I thought we agreed to something like:

SIAA0000

Where 'S' is the designator for the special items like PCI-E and
internal-regs. 0 = normal target ids, 0xF = special ids.

The target is only 4 bits, the attr is 8, so a little doc update to
clarify this should be enough, no need to change the DTs.

Jason
Sebastian Hesselbarth June 18, 2013, 6:24 p.m. UTC | #5
On 06/18/2013 07:46 PM, Jason Gunthorpe wrote:
> On Tue, Jun 18, 2013 at 08:25:28AM -0300, Ezequiel Garcia wrote:
>> +
>> +IIAA0000
>> +
>> +Where:
>> +  -- I = Marvell defined target ID for programmable windows
>> +  -- A = Marvell defined target attributes for programmable windows
>
> I thought we agreed to something like:
>
> SIAA0000
>
> Where 'S' is the designator for the special items like PCI-E and
> internal-regs. 0 = normal target ids, 0xF = special ids.
>
> The target is only 4 bits, the attr is 8, so a little doc update to
> clarify this should be enough, no need to change the DTs.

+1 for SIAA0000, as it allows to use MBUS_ID also for those fake
windows. It makes it more readable IMHO.

Also allows you to have up to 40b offset, which _might_ be important
with LPAE enabled.

Sebastian
Arnd Bergmann June 18, 2013, 6:39 p.m. UTC | #6
On Tuesday 18 June 2013, Sebastian Hesselbarth wrote:
> On 06/18/2013 07:46 PM, Jason Gunthorpe wrote:
> > On Tue, Jun 18, 2013 at 08:25:28AM -0300, Ezequiel Garcia wrote:
> >> +
> >> +IIAA0000
> >> +
> >> +Where:
> >> +  -- I = Marvell defined target ID for programmable windows
> >> +  -- A = Marvell defined target attributes for programmable windows
> >
> > I thought we agreed to something like:
> >
> > SIAA0000
> >
> > Where 'S' is the designator for the special items like PCI-E and
> > internal-regs. 0 = normal target ids, 0xF = special ids.
> >
> > The target is only 4 bits, the attr is 8, so a little doc update to
> > clarify this should be enough, no need to change the DTs.
> 
> +1 for SIAA0000, as it allows to use MBUS_ID also for those fake
> windows. It makes it more readable IMHO.

+1

> Also allows you to have up to 40b offset, which might be important
> with LPAE enabled.

Not with the current generation I think, since the mbus windows are
32 bit only, but it would avoid having to come up with a new format
for a potential future-generation mbus that has wider address.

	Arnd
Sebastian Hesselbarth June 18, 2013, 6:44 p.m. UTC | #7
On 06/18/2013 08:39 PM, Arnd Bergmann wrote:
> On Tuesday 18 June 2013, Sebastian Hesselbarth wrote:
>> Also allows you to have up to 40b offset, which might be important
>> with LPAE enabled.
>
> Not with the current generation I think, since the mbus windows are
> 32 bit only, but it would avoid having to come up with a new format
> for a potential future-generation mbus that has wider address.

Yeah, I also recall Thomas or Gregory mention a 32b limitation in
remap windows. But we don't need to waste addresses here

And even if SIAA0000 is a concern because there may be target id >15
someday, we can still have IIAASS00 instead of IIAA00SS. I guess
LPAE will not rise above 40b quickly ;)

Sebastian
Jason Gunthorpe June 18, 2013, 6:47 p.m. UTC | #8
On Tue, Jun 18, 2013 at 08:44:30PM +0200, Sebastian Hesselbarth wrote:
> On 06/18/2013 08:39 PM, Arnd Bergmann wrote:
> >On Tuesday 18 June 2013, Sebastian Hesselbarth wrote:
> >>Also allows you to have up to 40b offset, which might be important
> >>with LPAE enabled.
> >
> >Not with the current generation I think, since the mbus windows are
> >32 bit only, but it would avoid having to come up with a new format
> >for a potential future-generation mbus that has wider address.
> 
> Yeah, I also recall Thomas or Gregory mention a 32b limitation in
> remap windows. But we don't need to waste addresses here
> 
> And even if SIAA0000 is a concern because there may be target id >15
> someday, we can still have IIAASS00 instead of IIAA00SS. I guess
> LPAE will not rise above 40b quickly ;)

S = 0 means 4 bit I, 8 bit A
S = F means special
S = 1 could mean 16 bit I, etc , etc

Yes, we could define things as 'SIAAoooo oooooooo'

But remember 'o' is the offset within the window, it is not related to
LPAE.

To need > 32 bits 'o' you need to have windows > 4G in size. The only
thing that could possibly do that is PCI-E, and it is all special
anyhow..

The mbus top level ranges remap already supports >4G locations for
the windows, even though current hardware cannot do that.

Jason
Sebastian Hesselbarth June 18, 2013, 6:59 p.m. UTC | #9
On 06/18/2013 08:47 PM, Jason Gunthorpe wrote:
> On Tue, Jun 18, 2013 at 08:44:30PM +0200, Sebastian Hesselbarth wrote:
>> Yeah, I also recall Thomas or Gregory mention a 32b limitation in
>> remap windows. But we don't need to waste addresses here
>>
>> And even if SIAA0000 is a concern because there may be target id>15
>> someday, we can still have IIAASS00 instead of IIAA00SS. I guess
>> LPAE will not rise above 40b quickly ;)
>
> S = 0 means 4 bit I, 8 bit A
> S = F means special
> S = 1 could mean 16 bit I, etc , etc

S & 0x8 == 0x0 means 7b target
S & 0x8 == 0x8 means 7b special ?

> Yes, we could define things as 'SIAAoooo oooooooo'
>
> But remember 'o' is the offset within the window, it is not related to
> LPAE.
>
> To need>  32 bits 'o' you need to have windows>  4G in size. The only
> thing that could possibly do that is PCI-E, and it is all special
> anyhow..
>
> The mbus top level ranges remap already supports>4G locations for
> the windows, even though current hardware cannot do that.

True. But as Arnd also mentioned, I don't think it will ever be a
problem at all. Possibly there will never be any future SoC with mbus
that will either allow >32b remap base addresses nor >4G size.

But never the less, IIAA00SS (which is used in v3 of the patches) will
limit both to 32b/4G forever.

And we already have +3 for SIAAoooo ;)

Sebastian
Jason Gunthorpe June 18, 2013, 7:10 p.m. UTC | #10
On Tue, Jun 18, 2013 at 08:59:03PM +0200, Sebastian Hesselbarth wrote:

> >S = 0 means 4 bit I, 8 bit A
> >S = F means special
> >S = 1 could mean 16 bit I, etc , etc
> 
> S & 0x8 == 0x0 means 7b target
> S & 0x8 == 0x8 means 7b special ?

No need, special == mbus driver defined. There is no target ID.

The forms could be:

 0IAA0000
 FK000000
   - K=0 -> internal regs
   - K=1 -> PCI-E thingy
    etc
 1IIAA000 (future expansion example)


> >The mbus top level ranges remap already supports>4G locations for
> >the windows, even though current hardware cannot do that.
> 
> True. But as Arnd also mentioned, I don't think it will ever be a
> problem at all. Possibly there will never be any future SoC with mbus
> that will either allow >32b remap base addresses nor >4G size.

Actually, I think the failure to allow > 32b remap is a mistake
on Marvell's part. Linux needs as much low memory as possible, moving
things above 4G gives you more low SDRAM.

So I'd hope to see 40bit addressing for MBUS windows in a future chip.

But again, that already works with what Ezequiel has..

To be very clear, the only issue with the 32 bit offset is if we need
to describe an offset into a target in DT that is larger than 32
bits. The only use of offsets in DT is for internal regs. The
need for a > 32 bit offset in DT does not exist with the current
architecture.

Jason
Sebastian Hesselbarth June 18, 2013, 7:27 p.m. UTC | #11
On 06/18/2013 09:10 PM, Jason Gunthorpe wrote:
> On Tue, Jun 18, 2013 at 08:59:03PM +0200, Sebastian Hesselbarth wrote:
>
>>> S = 0 means 4 bit I, 8 bit A
>>> S = F means special
>>> S = 1 could mean 16 bit I, etc , etc
>>
>> S&  0x8 == 0x0 means 7b target
>> S&  0x8 == 0x8 means 7b special ?
>
> No need, special == mbus driver defined. There is no target ID.
>
> The forms could be:
>
>   0IAA0000
>   FK000000
>     - K=0 ->  internal regs
>     - K=1 ->  PCI-E thingy
>      etc
>   1IIAA000 (future expansion example)

Ok, got it. Any encoding is fine that allows to distinguish real
remap windows and fake ones. I assumed that maybe someday there
will be more than 4b target id so 0x80 as special case indicator
leaves 7b of normal target id in the _current_ mapping.

But yes, we can always invent a new encoding compatible with the
current mapping to allow more bits for either target id or attr
encoding.

I am fine with anything that leaves some room for >32b remap
windows.

>>> The mbus top level ranges remap already supports>4G locations for
>>> the windows, even though current hardware cannot do that.
>>
>> True. But as Arnd also mentioned, I don't think it will ever be a
>> problem at all. Possibly there will never be any future SoC with mbus
>> that will either allow>32b remap base addresses nor>4G size.
>
> Actually, I think the failure to allow>  32b remap is a mistake
> on Marvell's part. Linux needs as much low memory as possible, moving
> things above 4G gives you more low SDRAM.
>
> So I'd hope to see 40bit addressing for MBUS windows in a future chip.
>
> But again, that already works with what Ezequiel has..

Yeah, but currently Ezequiel e.g. uses 0xffff0001 for internal regs
that will not look nice with MBUS_ID(0xff,0xff) | 0x0001.

The whole point in the last mails was to ensure, Ezequiel will update
all remap windows to use MBUS_ID() and the fake ones to
MBUS_ID(0xf0,0x01), MBUS_ID(0xf0,0x02), aso.

Otherwise, I agree on _everything_ you said! :)

> To be very clear, the only issue with the 32 bit offset is if we need
> to describe an offset into a target in DT that is larger than 32
> bits. The only use of offsets in DT is for internal regs. The
> need for a>  32 bit offset in DT does not exist with the current
> architecture.
>
> Jason
Ezequiel Garcia June 18, 2013, 8:49 p.m. UTC | #12
Hi Sebastian,

You loose +1 internet points for dropping me from Cc ;-)

On Tue, Jun 18, 2013 at 09:27:28PM +0200, Sebastian Hesselbarth wrote:
> On 06/18/2013 09:10 PM, Jason Gunthorpe wrote:
> >
> > The forms could be:
> >
> >   0IAA0000
> >   FK000000
> >     - K=0 ->  internal regs
> >     - K=1 ->  PCI-E thingy
> >      etc
> >   1IIAA000 (future expansion example)
> 
> Ok, got it. Any encoding is fine that allows to distinguish real
> remap windows and fake ones. I assumed that maybe someday there
> will be more than 4b target id so 0x80 as special case indicator
> leaves 7b of normal target id in the _current_ mapping.
> 

I'm also wondering about why we not care about target IDs being more
than 4 bits.

Jason: (I'm checking now but perhaps you know better than me):
Is there any MBus-architectural reason for you assuring the
target ID will always be within 4-bits?

Thanks,
Jason Gunthorpe June 18, 2013, 8:55 p.m. UTC | #13
On Tue, Jun 18, 2013 at 05:49:11PM -0300, Ezequiel Garcia wrote:
> Hi Sebastian,
> 
> You loose +1 internet points for dropping me from Cc ;-)
> 
> On Tue, Jun 18, 2013 at 09:27:28PM +0200, Sebastian Hesselbarth wrote:
> > On 06/18/2013 09:10 PM, Jason Gunthorpe wrote:
> > >
> > > The forms could be:
> > >
> > >   0IAA0000
> > >   FK000000
> > >     - K=0 ->  internal regs
> > >     - K=1 ->  PCI-E thingy
> > >      etc
> > >   1IIAA000 (future expansion example)
> > 
> > Ok, got it. Any encoding is fine that allows to distinguish real
> > remap windows and fake ones. I assumed that maybe someday there
> > will be more than 4b target id so 0x80 as special case indicator
> > leaves 7b of normal target id in the _current_ mapping.
> > 
> 
> I'm also wondering about why we not care about target IDs being more
> than 4 bits.
> 
> Jason: (I'm checking now but perhaps you know better than me):
> Is there any MBus-architectural reason for you assuring the
> target ID will always be within 4-bits?

The manuals I have for the register set say 4 bits is allocated for
mbus targets.. Are yours different?

If they change it then the window register layout changes and then the
mbus driver probably need to change as well.

Jason
Ezequiel Garcia June 18, 2013, 9:10 p.m. UTC | #14
On Tue, Jun 18, 2013 at 02:55:22PM -0600, Jason Gunthorpe wrote:
> On Tue, Jun 18, 2013 at 05:49:11PM -0300, Ezequiel Garcia wrote:
> > Hi Sebastian,
> > 
> > You loose +1 internet points for dropping me from Cc ;-)
> > 
> > On Tue, Jun 18, 2013 at 09:27:28PM +0200, Sebastian Hesselbarth wrote:
> > > On 06/18/2013 09:10 PM, Jason Gunthorpe wrote:
> > > >
> > > > The forms could be:
> > > >
> > > >   0IAA0000
> > > >   FK000000
> > > >     - K=0 ->  internal regs
> > > >     - K=1 ->  PCI-E thingy
> > > >      etc
> > > >   1IIAA000 (future expansion example)
> > > 
> > > Ok, got it. Any encoding is fine that allows to distinguish real
> > > remap windows and fake ones. I assumed that maybe someday there
> > > will be more than 4b target id so 0x80 as special case indicator
> > > leaves 7b of normal target id in the _current_ mapping.
> > > 
> > 
> > I'm also wondering about why we not care about target IDs being more
> > than 4 bits.
> > 
> > Jason: (I'm checking now but perhaps you know better than me):
> > Is there any MBus-architectural reason for you assuring the
> > target ID will always be within 4-bits?
> 
> The manuals I have for the register set say 4 bits is allocated for
> mbus targets.. Are yours different?
> 

Nope. 4 bits here as well.

> If they change it then the window register layout changes and then the
> mbus driver probably need to change as well.
> 

Okey, then.
Ezequiel Garcia June 18, 2013, 9:34 p.m. UTC | #15
Arnd,

On Tue, Jun 18, 2013 at 06:14:33PM +0200, Arnd Bergmann wrote:
> On Tuesday 18 June 2013, Ezequiel Garcia wrote:
> > +Required properties:
> > +
> > +- compatible:	 Should be set to one of the following:
> > +		 marvell,armada370-mbus
> > +		 marvell,armadaxp-mbus
> > +
> > +- reg:		 Device's register space.
> > +		 Two entries are expected, see the examples below.
> > +		 The first one controls the devices decoding window and
> > +		 the second one controls the SDRAM decoding window.
> > +
> > +- address-cells: Must be '2'. The first cell for the MBus ID encoding,
> > +                 the second cell for the address offset within the window.
> > +
> > +- size-cells:    Must be '1'.
> > +
> > +- ranges:        Must be set up to provide a proper translation for each child.
> > +	         See the examples below.
> 
> You should explain here what the policy is regarding windows set up by the
> boot loader. Are the ranges in here required to be the ones that the boot
> loader has preconfigured, or are they the ones that the mbus driver is supposed
> to set up?
> 

Actually, I might be confused but the kernel MBus driver is completely
independent from the bootloader's -except in the internal register case,
(because we don't touch that from where the bootloader left it).

What happens is that any decoding window that was setup by the bootloader,
is wiped and completely new windows are allocated using the translations
in the DT, as described by this binding.

This was the case from the start with the old MBus driver. FWIW, I think
it's actually the best choice that can be made: it makes the kernel
independent of the previous setting.

I know you've suggested differently in the past, but I'm not sure I
understand what's the benefit in keeping the bootloaders configuration.

> > +Each child device needs at least a 'ranges' property. If the child is avaiable
> > +(i.e. status not 'disabled'), then the MBus driver creates a decoding window
> > +for it. For instance, in the example below the BootROM child is specified:
> > +
> > +	soc {
> > +		compatible = "marvell,armada370-mbus", "simple-bus";
> > +		reg = <0xd0020000 0x100>, <0xd0020180 0x20>;
> > +		#address-cells = <2>;
> > +		#size-cells = <1>;
> > +
> > +		ranges = < ... /* other entries */
> > +			   0x011d0000 0 0 0xfff00000 0x100000>;
> > +
> > +		bootrom {
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +			ranges = <0 0x011d0000 0 0x100000>;
> > +		};
> > +
> > +		/* other children */
> > +		...
> > +	};
> 
> Do you really want to require the child to provide a "ranges" property?
> I think this makes it more complicated to specify devices that belong
> into the "internal-regs" category.
> 

No, this text is actually a left-over from the previous patchset, in
current v3 patchset MBus children are *not* required to have any ranges.
On the otherside, although you will need one except in the most trivial
cases like for the BootROM.
Arnd Bergmann June 18, 2013, 9:45 p.m. UTC | #16
On Tuesday 18 June 2013, Ezequiel Garcia wrote:
> On Tue, Jun 18, 2013 at 06:14:33PM +0200, Arnd Bergmann wrote:
> > On Tuesday 18 June 2013, Ezequiel Garcia wrote:
> > > +Required properties:
> > > +
> > > +- compatible:	 Should be set to one of the following:
> > > +		 marvell,armada370-mbus
> > > +		 marvell,armadaxp-mbus
> > > +
> > > +- reg:		 Device's register space.
> > > +		 Two entries are expected, see the examples below.
> > > +		 The first one controls the devices decoding window and
> > > +		 the second one controls the SDRAM decoding window.
> > > +
> > > +- address-cells: Must be '2'. The first cell for the MBus ID encoding,
> > > +                 the second cell for the address offset within the window.
> > > +
> > > +- size-cells:    Must be '1'.
> > > +
> > > +- ranges:        Must be set up to provide a proper translation for each child.
> > > +	         See the examples below.
> > 
> > You should explain here what the policy is regarding windows set up by the
> > boot loader. Are the ranges in here required to be the ones that the boot
> > loader has preconfigured, or are they the ones that the mbus driver is supposed
> > to set up?
> > 
> 
> Actually, I might be confused but the kernel MBus driver is completely
> independent from the bootloader's -except in the internal register case,
> (because we don't touch that from where the bootloader left it).

Right, then state that clearly in the binding.

> What happens is that any decoding window that was setup by the bootloader,
> is wiped and completely new windows are allocated using the translations
> in the DT, as described by this binding.
> 
> This was the case from the start with the old MBus driver. FWIW, I think
> it's actually the best choice that can be made: it makes the kernel
> independent of the previous setting.
> 
> I know you've suggested differently in the past, but I'm not sure I
> understand what's the benefit in keeping the bootloaders configuration.

The device tree /normally/ describes things that are either wired up
in hardware or set up by the boot loader. Describing things that the
boot loader may or may not have set up and that the kernel should
set up but may ignore if it wants to is a bit fishy, but it seems
that you have decided to do it that way. You should definitely
document the fact that all ranges except the "internal-regs" are just
suggestions and cannot be relied on to be present at boot time.

A cleaner approach would be to require that all ranges here are exactly
the ones that have been configured by the boot loader, and state the
OS can either assume that they are valid or can reprogram them as
needed. That would imply that you don't actually need an mbus driver
unless you want to dynamically reassign the windows.

Please also add a property that describes the address range in which
the windows might be reassigned, i.e. everything that is not RAM.

> > > +Each child device needs at least a 'ranges' property. If the child is avaiable
> > > +(i.e. status not 'disabled'), then the MBus driver creates a decoding window
> > > +for it. For instance, in the example below the BootROM child is specified:
> > > +
> > > +	soc {
> > > +		compatible = "marvell,armada370-mbus", "simple-bus";
> > > +		reg = <0xd0020000 0x100>, <0xd0020180 0x20>;
> > > +		#address-cells = <2>;
> > > +		#size-cells = <1>;
> > > +
> > > +		ranges = < ... /* other entries */
> > > +			   0x011d0000 0 0 0xfff00000 0x100000>;
> > > +
> > > +		bootrom {
> > > +			#address-cells = <1>;
> > > +			#size-cells = <1>;
> > > +			ranges = <0 0x011d0000 0 0x100000>;
> > > +		};
> > > +
> > > +		/* other children */
> > > +		...
> > > +	};
> > 
> > Do you really want to require the child to provide a "ranges" property?
> > I think this makes it more complicated to specify devices that belong
> > into the "internal-regs" category.
> > 
> 
> No, this text is actually a left-over from the previous patchset, in
> current v3 patchset MBus children are *not* required to have any ranges.
> On the otherside, although you will need one except in the most trivial
> cases like for the BootROM.

Ok.

	Arnd
Ezequiel Garcia June 19, 2013, 6:52 p.m. UTC | #17
Hi Arnd,

On Tue, Jun 18, 2013 at 11:45:26PM +0200, Arnd Bergmann wrote:
> On Tuesday 18 June 2013, Ezequiel Garcia wrote:
> > On Tue, Jun 18, 2013 at 06:14:33PM +0200, Arnd Bergmann wrote:
> > > On Tuesday 18 June 2013, Ezequiel Garcia wrote:
> > > > +Required properties:
> > > > +
> > > > +- compatible:	 Should be set to one of the following:
> > > > +		 marvell,armada370-mbus
> > > > +		 marvell,armadaxp-mbus
> > > > +
> > > > +- reg:		 Device's register space.
> > > > +		 Two entries are expected, see the examples below.
> > > > +		 The first one controls the devices decoding window and
> > > > +		 the second one controls the SDRAM decoding window.
> > > > +
> > > > +- address-cells: Must be '2'. The first cell for the MBus ID encoding,
> > > > +                 the second cell for the address offset within the window.
> > > > +
> > > > +- size-cells:    Must be '1'.
> > > > +
> > > > +- ranges:        Must be set up to provide a proper translation for each child.
> > > > +	         See the examples below.
> > > 
> > > You should explain here what the policy is regarding windows set up by the
> > > boot loader. Are the ranges in here required to be the ones that the boot
> > > loader has preconfigured, or are they the ones that the mbus driver is supposed
> > > to set up?
> > > 
> > 
> > Actually, I might be confused but the kernel MBus driver is completely
> > independent from the bootloader's -except in the internal register case,
> > (because we don't touch that from where the bootloader left it).
> 
> Right, then state that clearly in the binding.
> 
> > What happens is that any decoding window that was setup by the bootloader,
> > is wiped and completely new windows are allocated using the translations
> > in the DT, as described by this binding.
> > 
> > This was the case from the start with the old MBus driver. FWIW, I think
> > it's actually the best choice that can be made: it makes the kernel
> > independent of the previous setting.
> > 
> > I know you've suggested differently in the past, but I'm not sure I
> > understand what's the benefit in keeping the bootloaders configuration.
> 
> The device tree /normally/ describes things that are either wired up
> in hardware or set up by the boot loader. Describing things that the
> boot loader may or may not have set up and that the kernel should
> set up but may ignore if it wants to is a bit fishy, but it seems
> that you have decided to do it that way. You should definitely
> document the fact that all ranges except the "internal-regs" are just
> suggestions and cannot be relied on to be present at boot time.
> 

Hold on! I've just noticed this, and I want to clarify something, just
to avoid mis-interpretations. The binding is not saying "the windows
described through this ranges are present at boot time".

Rather, it is "this binding will guarantee that the windows described 
in it will be present after the mbus allocates them".

Does it sound too fishy?
Arnd Bergmann June 19, 2013, 7:08 p.m. UTC | #18
On Wednesday 19 June 2013, Ezequiel Garcia wrote:
> > > What happens is that any decoding window that was setup by the bootloader,
> > > is wiped and completely new windows are allocated using the translations
> > > in the DT, as described by this binding.
> > > 
> > > This was the case from the start with the old MBus driver. FWIW, I think
> > > it's actually the best choice that can be made: it makes the kernel
> > > independent of the previous setting.
> > > 
> > > I know you've suggested differently in the past, but I'm not sure I
> > > understand what's the benefit in keeping the bootloaders configuration.
> > 
> > The device tree normally describes things that are either wired up
> > in hardware or set up by the boot loader. Describing things that the
> > boot loader may or may not have set up and that the kernel should
> > set up but may ignore if it wants to is a bit fishy, but it seems
> > that you have decided to do it that way. You should definitely
> > document the fact that all ranges except the "internal-regs" are just
> > suggestions and cannot be relied on to be present at boot time.
> > 
> 
> Hold on! I've just noticed this, and I want to clarify something, just
> to avoid mis-interpretations. The binding is not saying "the windows
> described through this ranges are present at boot time".
> 
> Rather, it is "this binding will guarantee that the windows described 
> in it will be present after the mbus allocates them".
> 
> Does it sound too fishy?

I don't think it's a guarantee that the binding can make. The binding
describes the interface between the hardware/firmware and the kernel,
not an interface between one kernel driver and another.

You could instead write:

"The ranges property defines a set of mbus windows that are expected
to be set by the operating system and that are guaranteed to be free
of overlaps with one another or with the system memory ranges.
Each entry in the property refers to exactly one window. If an
operating system choses to use a different set of mbus windows,
it must ensure that any address translations performed from downstream
devices are adapted accordingly. The operating system may insert
additional mbus windows that do not conflict with the ones listed
in the ranges, e.g. for mapping PCIe devices. As a special case,
the internal register window must be set up by the boot loader
at the address listed in the ranges property, since the operating
uses it to set up the other windows."

	Arnd
Ezequiel Garcia June 19, 2013, 7:29 p.m. UTC | #19
On Wed, Jun 19, 2013 at 09:08:30PM +0200, Arnd Bergmann wrote:
> On Wednesday 19 June 2013, Ezequiel Garcia wrote:
> > > > What happens is that any decoding window that was setup by the bootloader,
> > > > is wiped and completely new windows are allocated using the translations
> > > > in the DT, as described by this binding.
> > > > 
> > > > This was the case from the start with the old MBus driver. FWIW, I think
> > > > it's actually the best choice that can be made: it makes the kernel
> > > > independent of the previous setting.
> > > > 
> > > > I know you've suggested differently in the past, but I'm not sure I
> > > > understand what's the benefit in keeping the bootloaders configuration.
> > > 
> > > The device tree normally describes things that are either wired up
> > > in hardware or set up by the boot loader. Describing things that the
> > > boot loader may or may not have set up and that the kernel should
> > > set up but may ignore if it wants to is a bit fishy, but it seems
> > > that you have decided to do it that way. You should definitely
> > > document the fact that all ranges except the "internal-regs" are just
> > > suggestions and cannot be relied on to be present at boot time.
> > > 
> > 
> > Hold on! I've just noticed this, and I want to clarify something, just
> > to avoid mis-interpretations. The binding is not saying "the windows
> > described through this ranges are present at boot time".
> > 
> > Rather, it is "this binding will guarantee that the windows described 
> > in it will be present after the mbus allocates them".
> > 
> > Does it sound too fishy?
> 
> I don't think it's a guarantee that the binding can make. The binding
> describes the interface between the hardware/firmware and the kernel,
> not an interface between one kernel driver and another.
> 
> You could instead write:
> 
> "The ranges property defines a set of mbus windows that are expected
> to be set by the operating system and that are guaranteed to be free
> of overlaps with one another or with the system memory ranges.
> Each entry in the property refers to exactly one window. If an
> operating system choses to use a different set of mbus windows,
> it must ensure that any address translations performed from downstream
> devices are adapted accordingly. The operating system may insert
> additional mbus windows that do not conflict with the ones listed
> in the ranges, e.g. for mapping PCIe devices. As a special case,
> the internal register window must be set up by the boot loader
> at the address listed in the ranges property, since the operating
> uses it to set up the other windows."
> 

Nice!

Shamelessly copy-pasted into the binding documentation.

Thanks,
Jason Cooper June 19, 2013, 7:37 p.m. UTC | #20
On Wed, Jun 19, 2013 at 04:29:16PM -0300, Ezequiel Garcia wrote:
> On Wed, Jun 19, 2013 at 09:08:30PM +0200, Arnd Bergmann wrote:
> > On Wednesday 19 June 2013, Ezequiel Garcia wrote:
> > > > > What happens is that any decoding window that was setup by the bootloader,
> > > > > is wiped and completely new windows are allocated using the translations
> > > > > in the DT, as described by this binding.
> > > > > 
> > > > > This was the case from the start with the old MBus driver. FWIW, I think
> > > > > it's actually the best choice that can be made: it makes the kernel
> > > > > independent of the previous setting.
> > > > > 
> > > > > I know you've suggested differently in the past, but I'm not sure I
> > > > > understand what's the benefit in keeping the bootloaders configuration.
> > > > 
> > > > The device tree normally describes things that are either wired up
> > > > in hardware or set up by the boot loader. Describing things that the
> > > > boot loader may or may not have set up and that the kernel should
> > > > set up but may ignore if it wants to is a bit fishy, but it seems
> > > > that you have decided to do it that way. You should definitely
> > > > document the fact that all ranges except the "internal-regs" are just
> > > > suggestions and cannot be relied on to be present at boot time.
> > > > 
> > > 
> > > Hold on! I've just noticed this, and I want to clarify something, just
> > > to avoid mis-interpretations. The binding is not saying "the windows
> > > described through this ranges are present at boot time".
> > > 
> > > Rather, it is "this binding will guarantee that the windows described 
> > > in it will be present after the mbus allocates them".
> > > 
> > > Does it sound too fishy?
> > 
> > I don't think it's a guarantee that the binding can make. The binding
> > describes the interface between the hardware/firmware and the kernel,
> > not an interface between one kernel driver and another.
> > 
> > You could instead write:
> > 
> > "The ranges property defines a set of mbus windows that are expected
> > to be set by the operating system and that are guaranteed to be free
> > of overlaps with one another or with the system memory ranges.
> > Each entry in the property refers to exactly one window. If an
> > operating system choses to use a different set of mbus windows,
> > it must ensure that any address translations performed from downstream
> > devices are adapted accordingly. The operating system may insert
> > additional mbus windows that do not conflict with the ones listed
> > in the ranges, e.g. for mapping PCIe devices. As a special case,
> > the internal register window must be set up by the boot loader
> > at the address listed in the ranges property, since the operating

...system...

> > uses it to set up the other windows."
> > 
> 
> Nice!
> 
> Shamelessly copy-pasted into the binding documentation.

thx,

Jason.
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/bus/mvebu-mbus.txt b/Documentation/devicetree/bindings/bus/mvebu-mbus.txt
new file mode 100644
index 0000000..e15c280
--- /dev/null
+++ b/Documentation/devicetree/bindings/bus/mvebu-mbus.txt
@@ -0,0 +1,168 @@ 
+
+* Marvell MBus controller
+
+Required properties:
+
+- compatible:	 Should be set to one of the following:
+		 marvell,armada370-mbus
+		 marvell,armadaxp-mbus
+
+- reg:		 Device's register space.
+		 Two entries are expected, see the examples below.
+		 The first one controls the devices decoding window and
+		 the second one controls the SDRAM decoding window.
+
+- address-cells: Must be '2'. The first cell for the MBus ID encoding,
+                 the second cell for the address offset within the window.
+
+- size-cells:    Must be '1'.
+
+- ranges:        Must be set up to provide a proper translation for each child.
+	         See the examples below.
+
+Example:
+
+	soc {
+		compatible = "marvell,armada370-mbus", "simple-bus";
+		reg = <0xd0020000 0x100>, <0xd0020180 0x20>;
+		#address-cells = <2>;
+		#size-cells = <1>;
+	};
+
+** MBus child device specification
+
+Each child device needs at least a 'ranges' property. If the child is avaiable
+(i.e. status not 'disabled'), then the MBus driver creates a decoding window
+for it. For instance, in the example below the BootROM child is specified:
+
+	soc {
+		compatible = "marvell,armada370-mbus", "simple-bus";
+		reg = <0xd0020000 0x100>, <0xd0020180 0x20>;
+		#address-cells = <2>;
+		#size-cells = <1>;
+
+		ranges = < ... /* other entries */
+			   0x011d0000 0 0 0xfff00000 0x100000>;
+
+		bootrom {
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges = <0 0x011d0000 0 0x100000>;
+		};
+
+		/* other children */
+		...
+	};
+
+In the shown example, the MBus child node together with the translation
+entry in the 'ranges' property is what makes the MBus driver creates
+a static decoding window for the given child device.
+
+Since each window is identified by its target ID and attribute ID there's
+a special macro that can be use to simplify the translation entries:
+
+#define MBUS_ID(target,attributes) (((target) << 24) | ((attributes) << 16))
+
+Using this macro, the bootrom child node can be written in a slightly
+more readable fashion:
+
+	bootrom {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0 MBUS_ID(0x01, 0x1d) 0 0x100000>;
+	};
+
+** About the target ID and attribute encodig
+
+As stated above, for each mbus-node first-level child, the MBus driver will
+allocate a decoding window. The window target ID and attribute is created
+using the first cell, which must have the following format:
+
+IIAA0000
+
+Where:
+  -- I = Marvell defined target ID for programmable windows
+  -- A = Marvell defined target attributes for programmable windows
+
+Valid windows are required to define the lower bytes as zero.
+Entries that do not correspond to valid windows, and must be skipped by
+the MBus driver, set a non-zero value in the lower bytes.
+
+** About the window base address
+
+Remember the MBus controller allows a great deal of flexibility for choosing
+the decoding window base address. When planning the device tree layout it's
+possible to choose any address as the base address, provided of course there's
+a region large enough available, and with the required alignment.
+
+Yet in other words: there's nothing preventing us from setting a base address
+of 0xf0000000, or 0xd0000000 for the NOR device shown above, if such region is
+unused.
+
+** Example
+
+See the example below, where a more complete device tree is shown:
+
+	soc {
+		compatible = "marvell,armadaxp-mbus";
+		reg = <0 0xd0020000 0 0x100>, <0 0xd0020180 0 0x20>;
+
+		ranges = <0xffff0001 0 0 0xd0000000 0x100000   /* internal-regs */
+			  0xffff0002 0 0 0xe0000000 0x8100000  /* pcie */
+			  MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000
+			  MBUS_ID(0x01, 0x2f) 0 0 0xf0000000 0x8000000>;
+
+		bootrom {
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges = <0 MBUS_ID(0x01, 0x1d) 0 0x100000>;
+		};
+
+		devbus-bootcs {
+			status = "okay";
+			ranges = <0 MBUS_ID(0x01, 0x2f) 0 0x8000000>;
+
+			/* NOR */
+			nor {
+				compatible = "cfi-flash";
+				reg = <0 0x8000000>;
+				bank-width = <2>;
+			};
+		};
+
+		pcie-controller {
+			compatible = "marvell,armada-xp-pcie";
+			status = "okay";
+			device_type = "pci";
+
+			#address-cells = <3>;
+			#size-cells = <2>;
+
+			ranges =
+			       <0x82000000 0 0x40000 0xffff0001 0x40000 0 0x00002000   /* Port 0.0 registers */
+				0x82000000 0 0x42000 0xffff0001 0x42000 0 0x00002000   /* Port 2.0 registers */
+				0x82000000 0 0x44000 0xffff0001 0x44000 0 0x00002000   /* Port 0.1 registers */
+				0x82000000 0 0x48000 0xffff0001 0x48000 0 0x00002000   /* Port 0.2 registers */
+				0x82000000 0 0x4c000 0xffff0001 0x4c000 0 0x00002000   /* Port 0.3 registers */
+				0x82000000 0 0x80000 0xffff0001 0x80000 0 0x00002000   /* Port 1.0 registers */
+				0x82000000 0 0x82000 0xffff0001 0x82000 0 0x00002000   /* Port 3.0 registers */
+				0x82000000 0 0xe0000000 0xffff0002 0 0 0x08000000   /* non-prefetchable memory */
+				0x81000000 0 0 0xffff0002 0x8000000 0 0x00100000>; /* downstream I/O */
+
+			pcie@1,0 {
+				/* Port 0, Lane 0 */
+				status = "okay";
+			};
+		};
+
+		internal-regs {
+			compatible = "simple-bus";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges = <0 0xffff0001 0 0x100000>;
+
+			interrupt-controller@20000 {
+			      reg = <0x20a00 0x2d0>, <0x21070 0x58>;
+			};
+		};
+	};
diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
index 23f6ae6..4f086c7 100644
--- a/drivers/bus/mvebu-mbus.c
+++ b/drivers/bus/mvebu-mbus.c
@@ -312,6 +312,7 @@  static int mvebu_mbus_setup_window(struct mvebu_mbus_state *mbus,
 		writel(0, addr + WIN_REMAP_HI_OFF);
 	}
 
+	pr_info("window setup: %x:%x, 0x%x@%x\n", target, attr, base, size);
 	return 0;
 }
 
@@ -885,6 +886,120 @@  int __init mvebu_mbus_init(const char *soc, phys_addr_t mbuswins_phys_base,
 }
 
 #ifdef CONFIG_OF
+/*
+ * The window IDs in the ranges DT property have the following format:
+ *  - bits 24 to 31: window target ID
+ *  - bits 16 to 23: window attribute ID
+ *  - bits  8 to 15: unused
+ *  - bits  0 to 7:  custom modifiers
+ */
+#define TARGET(id) (((id) & 0xFF000000) >> 24)
+#define ATTR(id)   (((id) & 0x00FF0000) >> 16)
+#define CUSTOM(id) (((id) & 0x000000FF))
+
+static int __init mbus_dt_setup_win(struct mvebu_mbus_state *mbus,
+				    u32 base, u32 size,
+				    u8 target, u8 attr)
+{
+	const struct mvebu_mbus_mapping *map = mbus->soc->map;
+	const char *name;
+	int i;
+
+	/* Search for a suitable window in the existing mappings */
+	for (i = 0; map[i].name; i++)
+		if (map[i].target == target &&
+		    map[i].attr == (attr & map[i].attrmask))
+			break;
+
+	name = map[i].name;
+	if (!name) {
+		pr_err("window 0x%x:0x%x is unknown, skipping\n",
+		       target, attr);
+		return -EINVAL;
+	}
+
+	if (!mvebu_mbus_window_conflicts(mbus, base, size, target, attr)) {
+		pr_err("cannot add window '%s', conflicts with another window\n",
+		       name);
+		return -EBUSY;
+	}
+
+	if (mvebu_mbus_alloc_window(mbus, base, size, MVEBU_MBUS_NO_REMAP,
+				    target, attr)) {
+		pr_err("cannot add window '%s', too many windows\n",
+		       name);
+		return -ENOMEM;
+	}
+	return 0;
+}
+
+static int __init
+mbus_parse_ranges(struct device_node *node,
+		  int *addr_cells, int *c_addr_cells, int *c_size_cells,
+		  int *cell_count, const __be32 **ranges_start,
+		  const __be32 **ranges_end)
+{
+	const __be32 *prop;
+	int ranges_len, tuple_len;
+
+	*addr_cells = of_n_addr_cells(node);
+
+	prop = of_get_property(node, "#address-cells", NULL);
+	*c_addr_cells = be32_to_cpup(prop);
+
+	prop = of_get_property(node, "#size-cells", NULL);
+	*c_size_cells = be32_to_cpup(prop);
+
+	*cell_count = *addr_cells + *c_addr_cells + *c_size_cells;
+	tuple_len = (*cell_count) * sizeof(__be32);
+
+	*ranges_start = of_get_property(node, "ranges", &ranges_len);
+	*ranges_end = *ranges_start + ranges_len / sizeof(__be32);
+
+	if (*ranges_start == NULL || ranges_len % tuple_len) {
+		pr_warn("malformed ranges entry '%s'\n", node->name);
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int __init mbus_dt_setup(struct mvebu_mbus_state *mbus,
+				struct device_node *np)
+{
+	int addr_cells, c_addr_cells, c_size_cells;
+	int i, ret, cell_count;
+	const __be32 *r, *ranges_start, *ranges_end;
+
+	ret = mbus_parse_ranges(np, &addr_cells, &c_addr_cells,
+				&c_size_cells, &cell_count,
+				&ranges_start, &ranges_end);
+	if (ret < 0)
+		return ret;
+
+	for (i = 0, r = ranges_start; r < ranges_end; r += cell_count, i++) {
+		u32 windowid, base, size;
+		u8 target, attr;
+
+		windowid = of_read_number(r, 1);
+		/*
+		 * An entry with a non-zero custom field do not
+		 * correspond to a static window, skip it.
+		 */
+		if (CUSTOM(windowid))
+			continue;
+		target = TARGET(windowid);
+		attr = ATTR(windowid);
+
+		base = of_read_number(r + c_addr_cells, addr_cells);
+		size = of_read_number(r + c_addr_cells + addr_cells,
+				      c_size_cells);
+		ret = mbus_dt_setup_win(mbus, base, size, target, attr);
+		if (ret < 0)
+			return ret;
+	}
+	return 0;
+}
+
 int __init mvebu_mbus_dt_init(void)
 {
 	struct resource mbuswins_res, sdramwins_res;
@@ -916,6 +1031,10 @@  int __init mvebu_mbus_dt_init(void)
 				     resource_size(&mbuswins_res),
 				     sdramwins_res.start,
 				     resource_size(&sdramwins_res));
-	return ret;
+	if (ret)
+		return ret;
+
+	/* Setup statically declared windows in the DT */
+	return mbus_dt_setup(&mbus_state, np);
 }
 #endif