Patchwork [09/15] SPEAr13xx: dts: Fix PCIe core address ranges

login
register
mail settings
Submitter Pratyush ANAND
Date Oct. 29, 2012, 7:01 a.m.
Message ID <6ed9683e3149a640b4333cf3039ae31063b23e3b.1351492562.git.pratyush.anand@st.com>
Download mbox | patch
Permalink /patch/194853/
State Not Applicable
Headers show

Comments

Pratyush ANAND - Oct. 29, 2012, 7:01 a.m.
Each PCIe controller has 256 MB of address space whcih is accessible by
AHB and used to communicate with PCIe devices connected with the host
controller.

Signed-off-by: Pratyush Anand <pratyush.anand@st.com>
---
 arch/arm/boot/dts/spear13xx.dtsi |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)
viresh kumar - Oct. 29, 2012, 1:23 p.m.
On Mon, Oct 29, 2012 at 12:31 PM, Pratyush Anand <pratyush.anand@st.com> wrote:
> Each PCIe controller has 256 MB of address space whcih is accessible by
> AHB and used to communicate with PCIe devices connected with the host
> controller.

you haven't said what you have done here in this patch. i.e. extend AHB range
to include pcie space.

> Signed-off-by: Pratyush Anand <pratyush.anand@st.com>
> ---
>  arch/arm/boot/dts/spear13xx.dtsi |    9 +++++----
>  1 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/boot/dts/spear13xx.dtsi b/arch/arm/boot/dts/spear13xx.dtsi
> index 9ff4f5f..b7990754 100644
> --- a/arch/arm/boot/dts/spear13xx.dtsi
> +++ b/arch/arm/boot/dts/spear13xx.dtsi
> @@ -76,7 +76,8 @@
>                 #size-cells = <1>;
>                 compatible = "simple-bus";
>                 ranges = <0x50000000 0x50000000 0x10000000
> -                         0xb0000000 0xb0000000 0x10000000
> +                         0x80000000 0x80000000 0x20000000
> +                         0xb0000000 0xb0000000 0x20000000
>                           0xd0000000 0xd0000000 0x02000000
>                           0xd8000000 0xd8000000 0x01000000
>                           0xe0000000 0xe0000000 0x10000000>;
> @@ -194,7 +195,7 @@
>                 pcie0@b1000000 {
>                         compatible = "st,pcie-gadget", "st,pcie-host" ;
>                         reg = < 0xb1000000 0x4000
> -                               0x80000000 0x2000
> +                               0x80000000 0x10000000
>                                 0xeb800000 0x1000 >;
>                         interrupts = <0 68 0x4>;
>                         status = "disabled";

That's a blunder. I am damn sure, you haven't rebased it
on any rc. And you have rebased it on your local next.

There is no pci support in upstream kernel.

--
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pratyush ANAND - Oct. 30, 2012, 3:23 a.m.
On 10/29/2012 6:53 PM, viresh kumar wrote:
> On Mon, Oct 29, 2012 at 12:31 PM, Pratyush Anand <pratyush.anand@st.com> wrote:
>> Each PCIe controller has 256 MB of address space whcih is accessible by
>> AHB and used to communicate with PCIe devices connected with the host
>> controller.
>
> you haven't said what you have done here in this patch. i.e. extend AHB range
> to include pcie space.
>
>> Signed-off-by: Pratyush Anand <pratyush.anand@st.com>
>> ---
>>   arch/arm/boot/dts/spear13xx.dtsi |    9 +++++----
>>   1 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/spear13xx.dtsi b/arch/arm/boot/dts/spear13xx.dtsi
>> index 9ff4f5f..b7990754 100644
>> --- a/arch/arm/boot/dts/spear13xx.dtsi
>> +++ b/arch/arm/boot/dts/spear13xx.dtsi
>> @@ -76,7 +76,8 @@
>>                  #size-cells = <1>;
>>                  compatible = "simple-bus";
>>                  ranges = <0x50000000 0x50000000 0x10000000
>> -                         0xb0000000 0xb0000000 0x10000000
>> +                         0x80000000 0x80000000 0x20000000
>> +                         0xb0000000 0xb0000000 0x20000000
>>                            0xd0000000 0xd0000000 0x02000000
>>                            0xd8000000 0xd8000000 0x01000000
>>                            0xe0000000 0xe0000000 0x10000000>;
>> @@ -194,7 +195,7 @@
>>                  pcie0@b1000000 {
>>                          compatible = "st,pcie-gadget", "st,pcie-host" ;
>>                          reg = < 0xb1000000 0x4000
>> -                               0x80000000 0x2000
>> +                               0x80000000 0x10000000
>>                                  0xeb800000 0x1000 >;
>>                          interrupts = <0 68 0x4>;
>>                          status = "disabled";
>
> That's a blunder. I am damn sure, you haven't rebased it
> on any rc. And you have rebased it on your local next.
>

Yes, I re-based it with next of ST's repo.  :(

> There is no pci support in upstream kernel.

I will send a v2 after rebasing with mainline content.

>
> --
> viresh
> .
>


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann - Oct. 30, 2012, 9:55 p.m.
On Monday 29 October 2012, Pratyush Anand wrote:
> sible by
> AHB and used to communicate with PCIe devices connected with the host
> controller.
> 
> Signed-off-by: Pratyush Anand <pratyush.anand@st.com>
> ---
>  arch/arm/boot/dts/spear13xx.dtsi |    9 +++++----
>  1 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/spear13xx.dtsi b/arch/arm/boot/dts/spear13xx.dtsi
> index 9ff4f5f..b7990754 100644
> --- a/arch/arm/boot/dts/spear13xx.dtsi
> +++ b/arch/arm/boot/dts/spear13xx.dtsi
> @@ -76,7 +76,8 @@
>                 #size-cells = <1>;
>                 compatible = "simple-bus";
>                 ranges = <0x50000000 0x50000000 0x10000000
> -                         0xb0000000 0xb0000000 0x10000000
> +                         0x80000000 0x80000000 0x20000000
> +                         0xb0000000 0xb0000000 0x20000000
>                           0xd0000000 0xd0000000 0x02000000
>                           0xd8000000 0xd8000000 0x01000000
>                           0xe0000000 0xe0000000 0x10000000>;
> @@ -194,7 +195,7 @@
>                 pcie0@b1000000 {
>                         compatible = "st,pcie-gadget", "st,pcie-host" ;
>                         reg = < 0xb1000000 0x4000
> -                               0x80000000 0x2000
> +                               0x80000000 0x10000000
>                                 0xeb800000 0x1000 >;
>                         interrupts = <0 68 0x4>;
>                         status = "disabled";

The code that I'm looking at in the upstream kernel looks very different, so I don't
see how this patch would apply. Also, the version that you are patching as well
as the changes you do don't actually follow the generic PCI bindings at all
and should be done quite differently.

Please have a look at how to configure the PCI ranges in the IEEE1275 PCI
bindings and in examples you find in arch/powerpc/.

Most importantly, the compatible string used here can't really refer to both
the host and the endpoint mode, you really need separate bindings for those
and have only one of the two used in a particular system. What you can do is
to put both into the dtsi file and mark them as "disabled" and then enable
just one of the two on a given board file.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pratyush ANAND - Oct. 31, 2012, 11:24 a.m.
On 10/31/2012 3:25 AM, Arnd Bergmann wrote:
> On Monday 29 October 2012, Pratyush Anand wrote:
>> sible by
>> AHB and used to communicate with PCIe devices connected with the host
>> controller.
>>
>> Signed-off-by: Pratyush Anand <pratyush.anand@st.com>
>> ---
>>   arch/arm/boot/dts/spear13xx.dtsi |    9 +++++----
>>   1 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/spear13xx.dtsi b/arch/arm/boot/dts/spear13xx.dtsi
>> index 9ff4f5f..b7990754 100644
>> --- a/arch/arm/boot/dts/spear13xx.dtsi
>> +++ b/arch/arm/boot/dts/spear13xx.dtsi
>> @@ -76,7 +76,8 @@
>>                  #size-cells = <1>;
>>                  compatible = "simple-bus";
>>                  ranges = <0x50000000 0x50000000 0x10000000
>> -                         0xb0000000 0xb0000000 0x10000000
>> +                         0x80000000 0x80000000 0x20000000
>> +                         0xb0000000 0xb0000000 0x20000000
>>                            0xd0000000 0xd0000000 0x02000000
>>                            0xd8000000 0xd8000000 0x01000000
>>                            0xe0000000 0xe0000000 0x10000000>;
>> @@ -194,7 +195,7 @@
>>                  pcie0@b1000000 {
>>                          compatible = "st,pcie-gadget", "st,pcie-host" ;
>>                          reg = < 0xb1000000 0x4000
>> -                               0x80000000 0x2000
>> +                               0x80000000 0x10000000
>>                                  0xeb800000 0x1000 >;
>>                          interrupts = <0 68 0x4>;
>>                          status = "disabled";
>
> The code that I'm looking at in the upstream kernel looks very different, so I don't
> see how this patch would apply. Also, the version that you are patching as well
> as the changes you do don't actually follow the generic PCI bindings at all
> and should be done quite differently.
>

Yes, this was my mistake. I rebased with ST repo. Will send V2 with 
correct rebase.


> Please have a look at how to configure the PCI ranges in the IEEE1275 PCI
> bindings and in examples you find in arch/powerpc/.
>

Will look into.

> Most importantly, the compatible string used here can't really refer to both
> the host and the endpoint mode, you really need separate bindings for those
> and have only one of the two used in a particular system. What you can do is
> to put both into the dtsi file and mark them as "disabled" and then enable
> just one of the two on a given board file.

Yes, we had though of this limitation and had decided to do exactly the 
same way you have suggested.

thanks for your review comments.
Regards
Pratyush

>
> 	Arnd
> .
>


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/arm/boot/dts/spear13xx.dtsi b/arch/arm/boot/dts/spear13xx.dtsi
index 9ff4f5f..b7990754 100644
--- a/arch/arm/boot/dts/spear13xx.dtsi
+++ b/arch/arm/boot/dts/spear13xx.dtsi
@@ -76,7 +76,8 @@ 
 		#size-cells = <1>;
 		compatible = "simple-bus";
 		ranges = <0x50000000 0x50000000 0x10000000
-			  0xb0000000 0xb0000000 0x10000000
+			  0x80000000 0x80000000 0x20000000
+			  0xb0000000 0xb0000000 0x20000000
 			  0xd0000000 0xd0000000 0x02000000
 			  0xd8000000 0xd8000000 0x01000000
 			  0xe0000000 0xe0000000 0x10000000>;
@@ -194,7 +195,7 @@ 
 		pcie0@b1000000 {
 			compatible = "st,pcie-gadget", "st,pcie-host" ;
 			reg = < 0xb1000000 0x4000
-				0x80000000 0x2000
+				0x80000000 0x10000000
 				0xeb800000 0x1000 >;
 			interrupts = <0 68 0x4>;
 			status = "disabled";
@@ -203,7 +204,7 @@ 
 		pcie1@b1800000 {
 			compatible = "st,pcie-gadget", "st,pcie-host" ;
 			reg = < 0xb1800000 0x4000
-				0x90000000 0x2000
+				0x90000000 0x10000000
 				0xeb804000 0x1000 >;
 			interrupts = <0 69 0x4>;
 			status = "disabled";
@@ -212,7 +213,7 @@ 
 		pcie2@b4000000 {
 			compatible = "st,pcie-gadget", "st,pcie-host" ;
 			reg = < 0xb4000000 0x4000
-				0xc0000000 0x2000
+				0xc0000000 0x10000000
 				0xeb808000 0x1000 >;
 			interrupts = <0 70 0x4>;
 			status = "disabled";