diff mbox

[v3,09/10] ARM: dts: introduce MPS2 AN385/AN386

Message ID 56C4A46C.50506@arm.com
State New, archived
Headers show

Commit Message

Vladimir Murzin Feb. 17, 2016, 4:48 p.m. UTC
On 16/02/16 16:10, Vladimir Murzin wrote:
> On 16/02/16 11:01, Arnd Bergmann wrote:
>> On Tuesday 16 February 2016 10:08:14 Vladimir Murzin wrote:
>>> +
>>> +       ethernet@40200000 {
>>> +               compatible = "smsc,lan9220", "smsc,lan9115";
>>> +               reg = <0x40200000 0x10000>;
>>> +               interrupts = <13>;
>>> +               interrupt-parent = <&nvic>;
>>> +               smsc,irq-active-high;
>>> +       };
>>> +};
>>> +
>>>
>>
>> This node seems slightly misplaced. Is there some external bus interface
>> that this is connected to? The address suggests that it should be somewhere
>> below the /soc node, and you probably want to list the external bus
>> interface with a "ranges" property that identifies the addresses visibile
>> there, and put the external chip under there.
>>
> 
> I might messed it up since the MAC/PHY connects to the same 16-bit
> interface as the 16MB PSRAM external memory and both connected via AHB.
> 
> Not sure how it should be expressed, so some help form DT camp would be
> appreciated.

Arnd,

Does following fixup address your point on where/how ethernet node
should be placed?


Cheers
Vladimir

> 
> Cheers
> Vladimir
> 
>> 	Arnd
>>
>>
>>
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> 
>

Comments

Arnd Bergmann Feb. 17, 2016, 4:58 p.m. UTC | #1
On Wednesday 17 February 2016 16:48:44 Vladimir Murzin wrote:
> On 16/02/16 16:10, Vladimir Murzin wrote:
> > On 16/02/16 11:01, Arnd Bergmann wrote:
> >> On Tuesday 16 February 2016 10:08:14 Vladimir Murzin wrote:
> >>> +
> >>> +       ethernet@40200000 {
> >>> +               compatible = "smsc,lan9220", "smsc,lan9115";
> >>> +               reg = <0x40200000 0x10000>;
> >>> +               interrupts = <13>;
> >>> +               interrupt-parent = <&nvic>;
> >>> +               smsc,irq-active-high;
> >>> +       };
> >>> +};
> >>> +
> >>>
> >>
> >> This node seems slightly misplaced. Is there some external bus interface
> >> that this is connected to? The address suggests that it should be somewhere
> >> below the /soc node, and you probably want to list the external bus
> >> interface with a "ranges" property that identifies the addresses visibile
> >> there, and put the external chip under there.
> >>
> > 
> > I might messed it up since the MAC/PHY connects to the same 16-bit
> > interface as the 16MB PSRAM external memory and both connected via AHB.
> > 
> > Not sure how it should be expressed, so some help form DT camp would be
> > appreciated.
> 
> Does following fixup address your point on where/how ethernet node
> should be placed?



> diff --git a/arch/arm/boot/dts/mps2-an385.dts b/arch/arm/boot/dts/an385.dts
> index 976f86d..50c8d24 100644
> --- a/arch/arm/boot/dts/mps2-an385.dts
> +++ b/arch/arm/boot/dts/mps2-an385.dts
> @@ -63,14 +63,10 @@
>  		device_type = "memory";
>  		reg = <0x21000000 0x1000000>;
>  	};
> +};
> 
> -	ethernet@40200000 {
> -		compatible = "smsc,lan9220", "smsc,lan9115";
> -		reg = <0x40200000 0x10000>;
> -		interrupts = <13>;
> -		interrupt-parent = <&nvic>;
> -		smsc,irq-active-high;
> -	};
> +&mb {
> +	ranges = <0 0x40200000 0x10000>;
>  };

How is the range being set here? The way I read this is:

"There is an external bus controller whose single CPU physical
address for MMIO is configurable. The chip always connects
a lan9220 device to it (as that is in the dtsi file) and
nothing else is possible, and the bootloader in this
version of the machine has configured the window to be
at address 0x40200000."

Is that what the hardware does?

I would have expected the opposite, with the external bus
interface being hardwired to one or more physical addresses
(more than one if you have multiple chip-selects), and
then allow to connect different devices, which are in the
.dts file, while the bus controller is defined in the
.dtsi file.

	Arnd
Vladimir Murzin Feb. 18, 2016, 10:11 a.m. UTC | #2
On 17/02/16 16:58, Arnd Bergmann wrote:
> On Wednesday 17 February 2016 16:48:44 Vladimir Murzin wrote:
>> On 16/02/16 16:10, Vladimir Murzin wrote:
>>> On 16/02/16 11:01, Arnd Bergmann wrote:
>>>> On Tuesday 16 February 2016 10:08:14 Vladimir Murzin wrote:
>>>>> +
>>>>> +       ethernet@40200000 {
>>>>> +               compatible = "smsc,lan9220", "smsc,lan9115";
>>>>> +               reg = <0x40200000 0x10000>;
>>>>> +               interrupts = <13>;
>>>>> +               interrupt-parent = <&nvic>;
>>>>> +               smsc,irq-active-high;
>>>>> +       };
>>>>> +};
>>>>> +
>>>>>
>>>>
>>>> This node seems slightly misplaced. Is there some external bus interface
>>>> that this is connected to? The address suggests that it should be somewhere
>>>> below the /soc node, and you probably want to list the external bus
>>>> interface with a "ranges" property that identifies the addresses visibile
>>>> there, and put the external chip under there.
>>>>
>>>
>>> I might messed it up since the MAC/PHY connects to the same 16-bit
>>> interface as the 16MB PSRAM external memory and both connected via AHB.
>>>
>>> Not sure how it should be expressed, so some help form DT camp would be
>>> appreciated.
>>
>> Does following fixup address your point on where/how ethernet node
>> should be placed?
> 
> 
> 
>> diff --git a/arch/arm/boot/dts/mps2-an385.dts b/arch/arm/boot/dts/an385.dts
>> index 976f86d..50c8d24 100644
>> --- a/arch/arm/boot/dts/mps2-an385.dts
>> +++ b/arch/arm/boot/dts/mps2-an385.dts
>> @@ -63,14 +63,10 @@
>>  		device_type = "memory";
>>  		reg = <0x21000000 0x1000000>;
>>  	};
>> +};
>>
>> -	ethernet@40200000 {
>> -		compatible = "smsc,lan9220", "smsc,lan9115";
>> -		reg = <0x40200000 0x10000>;
>> -		interrupts = <13>;
>> -		interrupt-parent = <&nvic>;
>> -		smsc,irq-active-high;
>> -	};
>> +&mb {
>> +	ranges = <0 0x40200000 0x10000>;
>>  };
> 
> How is the range being set here? The way I read this is:
> 
> "There is an external bus controller whose single CPU physical
> address for MMIO is configurable. The chip always connects
> a lan9220 device to it (as that is in the dtsi file) and
> nothing else is possible, and the bootloader in this
> version of the machine has configured the window to be
> at address 0x40200000."
> 
> Is that what the hardware does?
> 
> I would have expected the opposite, with the external bus
> interface being hardwired to one or more physical addresses
> (more than one if you have multiple chip-selects), and
> then allow to connect different devices, which are in the
> .dts file, while the bus controller is defined in the
> .dtsi file.
> 

Right, I thought in a wrong way, in opposite it makes more sense now.

.dtsi

/* below the soc/ */
smb {
	compatible = "simple-bus";
	#address-cells = <2>;
	#size-cells = <1>;
	ranges = <0 0 0x40200000 0x10000>,
		 <1 0 0xa0000000 0x10000>;
};

.dts

smb {
	ethernet@0,0 {
		compatible = "smsc,lan9220", "smsc,lan9115";
		reg = <0 0x0 0x10000>;
		interrupts = <13>;
		interrupt-parent = <&nvic>;
		smsc,irq-active-high;
};


and looking again at .dtsi it seems to me that fpgaio should be moved
below the soc/ under separate bus interface which would hosts audio and
spi too or I keep missing things around device-tree?

I appreciate your help on this, thanks!

Vladimir

> 	Arnd
> 
> 
>
Arnd Bergmann Feb. 18, 2016, 10:45 a.m. UTC | #3
On Thursday 18 February 2016 10:11:37 Vladimir Murzin wrote:
> 
> Right, I thought in a wrong way, in opposite it makes more sense now.
> 
> .dtsi
> 
> /* below the soc/ */
> smb {
>         compatible = "simple-bus";
>         #address-cells = <2>;
>         #size-cells = <1>;
>         ranges = <0 0 0x40200000 0x10000>,
>                  <1 0 0xa0000000 0x10000>;
> };

That looks good, yes.

Is 0x10000 the correct maximum addressable size of the external bus
in both cases?

Intuitively, I would guess that the 0xa0000000 range might
be much wider.

> .dts
> 
> smb {
>         ethernet@0,0 {
>                 compatible = "smsc,lan9220", "smsc,lan9115";
>                 reg = <0 0x0 0x10000>;
>                 interrupts = <13>;
>                 interrupt-parent = <&nvic>;
>                 smsc,irq-active-high;
> };
> 
> 
> and looking again at .dtsi it seems to me that fpgaio should be moved
> below the soc/ under separate bus interface which would hosts audio and
> spi too or I keep missing things around device-tree?
> 

I don't see the audio and spi nodes, so I'm not sure where exactly
you would put them.

Ideally those things should be visible from a block diagram in the
datasheet.

	Arnd
Vladimir Murzin Feb. 18, 2016, 11:13 a.m. UTC | #4
On 18/02/16 10:45, Arnd Bergmann wrote:
> On Thursday 18 February 2016 10:11:37 Vladimir Murzin wrote:
>>
>> Right, I thought in a wrong way, in opposite it makes more sense now.
>>
>> .dtsi
>>
>> /* below the soc/ */
>> smb {
>>         compatible = "simple-bus";
>>         #address-cells = <2>;
>>         #size-cells = <1>;
>>         ranges = <0 0 0x40200000 0x10000>,
>>                  <1 0 0xa0000000 0x10000>;
>> };
> 
> That looks good, yes.
> 
> Is 0x10000 the correct maximum addressable size of the external bus
> in both cases?
> 
> Intuitively, I would guess that the 0xa0000000 range might
> be much wider.

There is only Ethernet connected to this bus (apart from PSRAM), so it
might be wider, but there is no indication of this in documentation.

> 
>> .dts
>>
>> smb {
>>         ethernet@0,0 {
>>                 compatible = "smsc,lan9220", "smsc,lan9115";
>>                 reg = <0 0x0 0x10000>;
>>                 interrupts = <13>;
>>                 interrupt-parent = <&nvic>;
>>                 smsc,irq-active-high;
>> };
>>
>>
>> and looking again at .dtsi it seems to me that fpgaio should be moved
>> below the soc/ under separate bus interface which would hosts audio and
>> spi too or I keep missing things around device-tree?
>>
> 
> I don't see the audio and spi nodes, so I'm not sure where exactly
> you would put them.

I just keep things simple ;)

> 
> Ideally those things should be visible from a block diagram in the
> datasheet.

Indeed, block diagram indicates all them as a "FPGA APB subsystem" and
clearly draws a line indicating a bus those devices are connected to.
After your point about lan9220, it looks clearer to me to express that
subsystem outside of soc/ node indicating bus interface, so it would
match to those drawings closely.

Cheers
Vladimir

> 
> 	Arnd
> 
> 
>
Arnd Bergmann Feb. 18, 2016, 12:16 p.m. UTC | #5
On Thursday 18 February 2016 11:13:06 Vladimir Murzin wrote:
> On 18/02/16 10:45, Arnd Bergmann wrote:
> > On Thursday 18 February 2016 10:11:37 Vladimir Murzin wrote:
> >>
> >> Right, I thought in a wrong way, in opposite it makes more sense now.
> >>
> >> .dtsi
> >>
> >> /* below the soc/ */
> >> smb {
> >>         compatible = "simple-bus";
> >>         #address-cells = <2>;
> >>         #size-cells = <1>;
> >>         ranges = <0 0 0x40200000 0x10000>,
> >>                  <1 0 0xa0000000 0x10000>;
> >> };
> > 
> > That looks good, yes.
> > 
> > Is 0x10000 the correct maximum addressable size of the external bus
> > in both cases?
> > 
> > Intuitively, I would guess that the 0xa0000000 range might
> > be much wider.
> 
> There is only Ethernet connected to this bus (apart from PSRAM), so it
> might be wider, but there is no indication of this in documentation.

I see this called "ahb_to_extmem16" in the documentation, which indicates
that it might be use 16 bits of address space, which would match
the 64K you listed.

For SSRAM1 / SSRAM2 / SSRAM3, a 8 MB address space is mentioned
and 16 MB for external PSRAM at 0x21000000.

	Arnd
Vladimir Murzin Feb. 18, 2016, 12:47 p.m. UTC | #6
On 18/02/16 12:16, Arnd Bergmann wrote:
> On Thursday 18 February 2016 11:13:06 Vladimir Murzin wrote:
>> On 18/02/16 10:45, Arnd Bergmann wrote:
>>> On Thursday 18 February 2016 10:11:37 Vladimir Murzin wrote:
>>>>
>>>> Right, I thought in a wrong way, in opposite it makes more sense now.
>>>>
>>>> .dtsi
>>>>
>>>> /* below the soc/ */
>>>> smb {
>>>>         compatible = "simple-bus";
>>>>         #address-cells = <2>;
>>>>         #size-cells = <1>;
>>>>         ranges = <0 0 0x40200000 0x10000>,
>>>>                  <1 0 0xa0000000 0x10000>;
>>>> };
>>>
>>> That looks good, yes.
>>>
>>> Is 0x10000 the correct maximum addressable size of the external bus
>>> in both cases?
>>>
>>> Intuitively, I would guess that the 0xa0000000 range might
>>> be much wider.
>>
>> There is only Ethernet connected to this bus (apart from PSRAM), so it
>> might be wider, but there is no indication of this in documentation.
> 
> I see this called "ahb_to_extmem16" in the documentation, which indicates
> that it might be use 16 bits of address space, which would match
> the 64K you listed.
> 
> For SSRAM1 / SSRAM2 / SSRAM3, a 8 MB address space is mentioned
> and 16 MB for external PSRAM at 0x21000000.
> 

Right. I'll incorporate changes in the next version. Thanks for lessons
given and your time!

Cheers
Vladimir

> 	Arnd
> 
> 
> 
>
diff mbox

Patch

diff --git a/arch/arm/boot/dts/mps2-an385.dts b/arch/arm/boot/dts/an385.dts
index 976f86d..50c8d24 100644
--- a/arch/arm/boot/dts/mps2-an385.dts
+++ b/arch/arm/boot/dts/mps2-an385.dts
@@ -63,14 +63,10 @@ 
 		device_type = "memory";
 		reg = <0x21000000 0x1000000>;
 	};
+};

-	ethernet@40200000 {
-		compatible = "smsc,lan9220", "smsc,lan9115";
-		reg = <0x40200000 0x10000>;
-		interrupts = <13>;
-		interrupt-parent = <&nvic>;
-		smsc,irq-active-high;
-	};
+&mb {
+	ranges = <0 0x40200000 0x10000>;
 };

 &uart0 {
diff --git a/arch/arm/boot/dts/mps2.dtsi b/arch/arm/boot/dts/mps2.dtsi
index 5d2c539..1673472 100644
--- a/arch/arm/boot/dts/mps2.dtsi
+++ b/arch/arm/boot/dts/mps2.dtsi
@@ -223,5 +223,21 @@ 
 				default-state = "off";
 			};
 		};
+
+	};
+
+	mb: smb {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+
+		ethernet@0 {
+			compatible = "smsc,lan9220", "smsc,lan9115";
+			reg = <0x0 0x10000>;
+			interrupts = <13>;
+			interrupt-parent = <&nvic>;
+			smsc,irq-active-high;
+		};
 	};
 };