diff mbox series

rockchip: Move Bob specific bits to it's specific u-boot.dtsi

Message ID 20201108230232.1044461-1-pbrobinson@gmail.com
State New
Delegated to: Kever Yang
Headers show
Series rockchip: Move Bob specific bits to it's specific u-boot.dtsi | expand

Commit Message

Peter Robinson Nov. 8, 2020, 11:02 p.m. UTC
Move the bits that are device specific to the -u-boot.dtsi as the
bits may be different on other devices and hence breaks SPI on
those devices such as the Pinebook Pro.

Signed-off-by: Peter Robinson <pbrobinson@gmail.com>
Fixes: c4cea2bbf995 ("rockchip: Enable building a SPI ROM image on bob")
Cc: Simon Glass <sjg@chromium.org>
---
 arch/arm/dts/rk3399-gru-u-boot.dtsi | 30 +++++++++++++++++++++++++++++
 arch/arm/dts/rk3399-u-boot.dtsi     | 25 ------------------------
 2 files changed, 30 insertions(+), 25 deletions(-)

Comments

Kever Yang Nov. 10, 2020, 7:34 a.m. UTC | #1
Hi Peter,

On 2020/11/9 上午7:02, Peter Robinson wrote:
> Move the bits that are device specific to the -u-boot.dtsi as the
> bits may be different on other devices and hence breaks SPI on
> those devices such as the Pinebook Pro.
>
> Signed-off-by: Peter Robinson <pbrobinson@gmail.com>
> Fixes: c4cea2bbf995 ("rockchip: Enable building a SPI ROM image on bob")
> Cc: Simon Glass <sjg@chromium.org>
> ---
>   arch/arm/dts/rk3399-gru-u-boot.dtsi | 30 +++++++++++++++++++++++++++++
>   arch/arm/dts/rk3399-u-boot.dtsi     | 25 ------------------------
>   2 files changed, 30 insertions(+), 25 deletions(-)
>
> diff --git a/arch/arm/dts/rk3399-gru-u-boot.dtsi b/arch/arm/dts/rk3399-gru-u-boot.dtsi
> index 390ac2bb5a..5e95cacfea 100644
> --- a/arch/arm/dts/rk3399-gru-u-boot.dtsi
> +++ b/arch/arm/dts/rk3399-gru-u-boot.dtsi
> @@ -5,6 +5,36 @@
>   
>   #include "rk3399-u-boot.dtsi"
>   
> +/ {
> +	aliases {
> +		spi1 = &spi1;
> +	};
> +};

Does this still need to remove from common code after your another patch 
applied? It look reasonable and

not likely to break others.

https://patchwork.ozlabs.org/project/uboot/patch/20201108140023.32501-1-sigmaris@gmail.com/

> +
> +#ifdef CONFIG_ROCKCHIP_SPI_IMAGE
> +&binman {
> +	rom {
> +		filename = "u-boot.rom";
> +		size = <0x400000>;
> +		pad-byte = <0xff>;
> +
> +		mkimage {
> +			args = "-n rk3399 -T rkspi";
> +			u-boot-spl {
> +			};
> +		};
> +		u-boot-img {
> +			offset = <0x40000>;
> +		};
> +		u-boot {
> +			offset = <0x300000>;
> +		};
> +		fdtmap {
> +		};
> +	};
> +};
> +#endif


What's the image space mapping for Pinebook Pro do you using?

I think there should be another binman config if this is not common .


Thanks,

- Kever

> +
>   &spi_flash {
>   	u-boot,dm-pre-reloc;
>   };
> diff --git a/arch/arm/dts/rk3399-u-boot.dtsi b/arch/arm/dts/rk3399-u-boot.dtsi
> index ecd230c720..26b0a34e64 100644
> --- a/arch/arm/dts/rk3399-u-boot.dtsi
> +++ b/arch/arm/dts/rk3399-u-boot.dtsi
> @@ -11,7 +11,6 @@
>   		mmc0 = &sdhci;
>   		mmc1 = &sdmmc;
>   		pci0 = &pcie0;
> -		spi1 = &spi1;
>   	};
>   
>   	cic: syscon@ff620000 {
> @@ -60,30 +59,6 @@
>   
>   };
>   
> -#ifdef CONFIG_ROCKCHIP_SPI_IMAGE
> -&binman {
> -	rom {
> -		filename = "u-boot.rom";
> -		size = <0x400000>;
> -		pad-byte = <0xff>;
> -
> -		mkimage {
> -			args = "-n rk3399 -T rkspi";
> -			u-boot-spl {
> -			};
> -		};
> -		u-boot-img {
> -			offset = <0x40000>;
> -		};
> -		u-boot {
> -			offset = <0x300000>;
> -		};
> -		fdtmap {
> -		};
> -	};
> -};
> -#endif
> -
>   &cru {
>   	u-boot,dm-pre-reloc;
>   };
Hugh Cole-Baker Nov. 10, 2020, 5:18 p.m. UTC | #2
Hi,

> On 10 Nov 2020, at 07:34, Kever Yang <kever.yang@rock-chips.com> wrote:
> 
> Hi Peter,
> 
> On 2020/11/9 上午7:02, Peter Robinson wrote:
>> Move the bits that are device specific to the -u-boot.dtsi as the
>> bits may be different on other devices and hence breaks SPI on
>> those devices such as the Pinebook Pro.
>> 
>> Signed-off-by: Peter Robinson <pbrobinson@gmail.com>
>> Fixes: c4cea2bbf995 ("rockchip: Enable building a SPI ROM image on bob")
>> Cc: Simon Glass <sjg@chromium.org>
>> ---
>>  arch/arm/dts/rk3399-gru-u-boot.dtsi | 30 +++++++++++++++++++++++++++++
>>  arch/arm/dts/rk3399-u-boot.dtsi     | 25 ------------------------
>>  2 files changed, 30 insertions(+), 25 deletions(-)
>> 
>> diff --git a/arch/arm/dts/rk3399-gru-u-boot.dtsi b/arch/arm/dts/rk3399-gru-u-boot.dtsi
>> index 390ac2bb5a..5e95cacfea 100644
>> --- a/arch/arm/dts/rk3399-gru-u-boot.dtsi
>> +++ b/arch/arm/dts/rk3399-gru-u-boot.dtsi
>> @@ -5,6 +5,36 @@
>>    #include "rk3399-u-boot.dtsi"
>>  +/ {
>> +	aliases {
>> +		spi1 = &spi1;
>> +	};
>> +};
> 
> Does this still need to remove from common code after your another patch applied? It look reasonable and
> 
> not likely to break others.
> 
> https://patchwork.ozlabs.org/project/uboot/patch/20201108140023.32501-1-sigmaris@gmail.com/

My patch linked above was aiming to fix SPI boot on the rockpro64 by
adapting to the spi1 alias that's now in the rk3399-u-boot.dtsi rather
than removing it (in fact, my patch wouldn't work correctly if the spi1
alias was removed). This seemed like one good solution as the RK3399 does
physically have SPI buses 0 to 5, and out of those the SPI flash is on
bus 1, so I thought it'd be better to refer to it as bus 1 instead of
aliasing it to bus 0.

I see now, though, that it's not just the rockpro64 that's affected, but
also Pinebook Pro and it seems rk3399-roc-pc. If the consensus is that
my patch is the right approach, I can send another series with the same
type of fix for the PBP and rk3399-roc-pc. Otherwise this patch should
be used and my patch shouldn't be applied as it relies on the spi1 alias
that this patch removes.

Regards,
Hugh

> 
>> +
>> +#ifdef CONFIG_ROCKCHIP_SPI_IMAGE
>> +&binman {
>> +	rom {
>> +		filename = "u-boot.rom";
>> +		size = <0x400000>;
>> +		pad-byte = <0xff>;
>> +
>> +		mkimage {
>> +			args = "-n rk3399 -T rkspi";
>> +			u-boot-spl {
>> +			};
>> +		};
>> +		u-boot-img {
>> +			offset = <0x40000>;
>> +		};
>> +		u-boot {
>> +			offset = <0x300000>;
>> +		};
>> +		fdtmap {
>> +		};
>> +	};
>> +};
>> +#endif
> 
> 
> What's the image space mapping for Pinebook Pro do you using?
> 
> I think there should be another binman config if this is not common .
> 
> 
> Thanks,
> 
> - Kever
> 
>> +
>>  &spi_flash {
>>  	u-boot,dm-pre-reloc;
>>  };
>> diff --git a/arch/arm/dts/rk3399-u-boot.dtsi b/arch/arm/dts/rk3399-u-boot.dtsi
>> index ecd230c720..26b0a34e64 100644
>> --- a/arch/arm/dts/rk3399-u-boot.dtsi
>> +++ b/arch/arm/dts/rk3399-u-boot.dtsi
>> @@ -11,7 +11,6 @@
>>  		mmc0 = &sdhci;
>>  		mmc1 = &sdmmc;
>>  		pci0 = &pcie0;
>> -		spi1 = &spi1;
>>  	};
>>    	cic: syscon@ff620000 {
>> @@ -60,30 +59,6 @@
>>    };
>>  -#ifdef CONFIG_ROCKCHIP_SPI_IMAGE
>> -&binman {
>> -	rom {
>> -		filename = "u-boot.rom";
>> -		size = <0x400000>;
>> -		pad-byte = <0xff>;
>> -
>> -		mkimage {
>> -			args = "-n rk3399 -T rkspi";
>> -			u-boot-spl {
>> -			};
>> -		};
>> -		u-boot-img {
>> -			offset = <0x40000>;
>> -		};
>> -		u-boot {
>> -			offset = <0x300000>;
>> -		};
>> -		fdtmap {
>> -		};
>> -	};
>> -};
>> -#endif
>> -
>>  &cru {
>>  	u-boot,dm-pre-reloc;
>>  };
Alper Nebi Yasak Nov. 10, 2020, 11:37 p.m. UTC | #3
On 10/11/2020 20:18, Hugh Cole-Baker wrote:
> Hi,
> 
>> On 10 Nov 2020, at 07:34, Kever Yang <kever.yang@rock-chips.com> wrote:
>>
>> Hi Peter,
>>
>> On 2020/11/9 上午7:02, Peter Robinson wrote:
>>> Move the bits that are device specific to the -u-boot.dtsi as the
>>> bits may be different on other devices and hence breaks SPI on
>>> those devices such as the Pinebook Pro.
>>>
>>> Signed-off-by: Peter Robinson <pbrobinson@gmail.com>
>>> Fixes: c4cea2bbf995 ("rockchip: Enable building a SPI ROM image on bob")
>>> Cc: Simon Glass <sjg@chromium.org>
>>> ---
>>>  arch/arm/dts/rk3399-gru-u-boot.dtsi | 30 +++++++++++++++++++++++++++++
>>>  arch/arm/dts/rk3399-u-boot.dtsi     | 25 ------------------------
>>>  2 files changed, 30 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/arch/arm/dts/rk3399-gru-u-boot.dtsi b/arch/arm/dts/rk3399-gru-u-boot.dtsi
>>> index 390ac2bb5a..5e95cacfea 100644
>>> --- a/arch/arm/dts/rk3399-gru-u-boot.dtsi
>>> +++ b/arch/arm/dts/rk3399-gru-u-boot.dtsi
>>> @@ -5,6 +5,36 @@
>>>    #include "rk3399-u-boot.dtsi"
>>>  +/ {
>>> +	aliases {
>>> +		spi1 = &spi1;
>>> +	};
>>> +};
>>
>> Does this still need to remove from common code after your another patch applied? It look reasonable and
>>
>> not likely to break others.
>>
>> https://patchwork.ozlabs.org/project/uboot/patch/20201108140023.32501-1-sigmaris@gmail.com/
> 
> My patch linked above was aiming to fix SPI boot on the rockpro64 by
> adapting to the spi1 alias that's now in the rk3399-u-boot.dtsi rather
> than removing it (in fact, my patch wouldn't work correctly if the spi1
> alias was removed). This seemed like one good solution as the RK3399 does
> physically have SPI buses 0 to 5, and out of those the SPI flash is on
> bus 1, so I thought it'd be better to refer to it as bus 1 instead of
> aliasing it to bus 0.

It looks like something similar to your patch works on the Pinebook Pro,
see this other thread about rk3399 SPI breakage [1] and one recent mail
from it [2].

[1] https://lists.denx.de/pipermail/u-boot/2020-November/432046.html
[2] https://lists.denx.de/pipermail/u-boot/2020-November/432505.html

> I see now, though, that it's not just the rockpro64 that's affected, but
> also Pinebook Pro and it seems rk3399-roc-pc. If the consensus is that
> my patch is the right approach, I can send another series with the same
> type of fix for the PBP and rk3399-roc-pc. Otherwise this patch should
> be used and my patch shouldn't be applied as it relies on the spi1 alias
> that this patch removes.

Looks to me like the full list is (though I don't have those boards):

    pinebook-pro-rk3399         rk3399-pinebook-pro-u-boot.dtsi
    puma-rk3399                 rk3399-puma-haikou-u-boot.dtsi
    roc-pc-rk3399               rk3399-roc-pc-u-boot.dtsi
    roc-pc-mezzanine-rk3399             ``
    rockpro64-rk3399            rk3399-rockpro64-u-boot.dtsi

> Regards,
> Hugh
> 
>>
>>> +
>>> +#ifdef CONFIG_ROCKCHIP_SPI_IMAGE
>>> +&binman {
>>> +	rom {
>>> +		filename = "u-boot.rom";
>>> +		size = <0x400000>;
>>> +		pad-byte = <0xff>;
>>> +
>>> +		mkimage {
>>> +			args = "-n rk3399 -T rkspi";
>>> +			u-boot-spl {
>>> +			};
>>> +		};
>>> +		u-boot-img {
>>> +			offset = <0x40000>;
>>> +		};
>>> +		u-boot {
>>> +			offset = <0x300000>;
>>> +		};
>>> +		fdtmap {
>>> +		};
>>> +	};
>>> +};
>>> +#endif
>>
>>
>> What's the image space mapping for Pinebook Pro do you using?
>>
>> I think there should be another binman config if this is not common .
>>
>>
>> Thanks,
>>
>> - Kever
>>
>>> +
>>>  &spi_flash {
>>>  	u-boot,dm-pre-reloc;
>>>  };
>>> diff --git a/arch/arm/dts/rk3399-u-boot.dtsi b/arch/arm/dts/rk3399-u-boot.dtsi
>>> index ecd230c720..26b0a34e64 100644
>>> --- a/arch/arm/dts/rk3399-u-boot.dtsi
>>> +++ b/arch/arm/dts/rk3399-u-boot.dtsi
>>> @@ -11,7 +11,6 @@
>>>  		mmc0 = &sdhci;
>>>  		mmc1 = &sdmmc;
>>>  		pci0 = &pcie0;
>>> -		spi1 = &spi1;
>>>  	};
>>>    	cic: syscon@ff620000 {
>>> @@ -60,30 +59,6 @@
>>>    };
>>>  -#ifdef CONFIG_ROCKCHIP_SPI_IMAGE
>>> -&binman {
>>> -	rom {
>>> -		filename = "u-boot.rom";
>>> -		size = <0x400000>;
>>> -		pad-byte = <0xff>;
>>> -
>>> -		mkimage {
>>> -			args = "-n rk3399 -T rkspi";
>>> -			u-boot-spl {
>>> -			};
>>> -		};
>>> -		u-boot-img {
>>> -			offset = <0x40000>;
>>> -		};
>>> -		u-boot {
>>> -			offset = <0x300000>;
>>> -		};
>>> -		fdtmap {
>>> -		};
>>> -	};
>>> -};
>>> -#endif
>>> -
>>>  &cru {
>>>  	u-boot,dm-pre-reloc;
>>>  };
>
diff mbox series

Patch

diff --git a/arch/arm/dts/rk3399-gru-u-boot.dtsi b/arch/arm/dts/rk3399-gru-u-boot.dtsi
index 390ac2bb5a..5e95cacfea 100644
--- a/arch/arm/dts/rk3399-gru-u-boot.dtsi
+++ b/arch/arm/dts/rk3399-gru-u-boot.dtsi
@@ -5,6 +5,36 @@ 
 
 #include "rk3399-u-boot.dtsi"
 
+/ {
+	aliases {
+		spi1 = &spi1;
+	};
+};
+
+#ifdef CONFIG_ROCKCHIP_SPI_IMAGE
+&binman {
+	rom {
+		filename = "u-boot.rom";
+		size = <0x400000>;
+		pad-byte = <0xff>;
+
+		mkimage {
+			args = "-n rk3399 -T rkspi";
+			u-boot-spl {
+			};
+		};
+		u-boot-img {
+			offset = <0x40000>;
+		};
+		u-boot {
+			offset = <0x300000>;
+		};
+		fdtmap {
+		};
+	};
+};
+#endif
+
 &spi_flash {
 	u-boot,dm-pre-reloc;
 };
diff --git a/arch/arm/dts/rk3399-u-boot.dtsi b/arch/arm/dts/rk3399-u-boot.dtsi
index ecd230c720..26b0a34e64 100644
--- a/arch/arm/dts/rk3399-u-boot.dtsi
+++ b/arch/arm/dts/rk3399-u-boot.dtsi
@@ -11,7 +11,6 @@ 
 		mmc0 = &sdhci;
 		mmc1 = &sdmmc;
 		pci0 = &pcie0;
-		spi1 = &spi1;
 	};
 
 	cic: syscon@ff620000 {
@@ -60,30 +59,6 @@ 
 
 };
 
-#ifdef CONFIG_ROCKCHIP_SPI_IMAGE
-&binman {
-	rom {
-		filename = "u-boot.rom";
-		size = <0x400000>;
-		pad-byte = <0xff>;
-
-		mkimage {
-			args = "-n rk3399 -T rkspi";
-			u-boot-spl {
-			};
-		};
-		u-boot-img {
-			offset = <0x40000>;
-		};
-		u-boot {
-			offset = <0x300000>;
-		};
-		fdtmap {
-		};
-	};
-};
-#endif
-
 &cru {
 	u-boot,dm-pre-reloc;
 };