diff mbox series

[v5,12/12] arm: mvebu: clearfog: Use Pro DT by default

Message ID 20200127200156.15173-13-mrjoel@lixil.net
State Superseded
Headers show
Series ClearFog Base static variant support | expand

Commit Message

Joel Johnson Jan. 27, 2020, 8:01 p.m. UTC
Switch to explicitly using the Pro variant DT, which has been
available since Linux 4.11.

---

v4 changes:
  - new
v5 changes:
  - none

I separated out this change to the end of the series since it drew
questioning on prior review. I'd still advocate for making the change,
since especially with the additions of static variants and runtime
detection, it becomes easier from within a booted kernel to identify the
type and version of U-Boot image installed. Without making this change,
it becomes less direct to determine an actual Pro vs. Base, vs old
U-Boot image maybe supporting some hybrid variant configuration.

Even in the Linux kernel adding of the Pro DTS, it is indicated that it
was meant for backwards compatibility.

Except for cases where checking is done directly against the product
name from userspace, I don't see downsides even from a compatibility
perspective for not making this change. In cases where checking *is*
done from userspace, the broadening of the Clearfog product line would
seem to mean that userspace checking should be flagged as needing to be
udpated as well (or glob/regex matched as needed).

Signed-off-by: Joel Johnson <mrjoel at lixil.net>
---
 board/solidrun/clearfog/clearfog.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Baruch Siach Jan. 28, 2020, 6:17 a.m. UTC | #1
Hi Joel,

On Mon, Jan 27, 2020 at 01:01:56PM -0700, Joel Johnson wrote:
> Switch to explicitly using the Pro variant DT, which has been
> available since Linux 4.11.
> 
> ---
> 
> v4 changes:
>   - new
> v5 changes:
>   - none
> 
> I separated out this change to the end of the series since it drew
> questioning on prior review. I'd still advocate for making the change,
> since especially with the additions of static variants and runtime
> detection, it becomes easier from within a booted kernel to identify the
> type and version of U-Boot image installed. Without making this change,
> it becomes less direct to determine an actual Pro vs. Base, vs old
> U-Boot image maybe supporting some hybrid variant configuration.
> 
> Even in the Linux kernel adding of the Pro DTS, it is indicated that it
> was meant for backwards compatibility.
> 
> Except for cases where checking is done directly against the product
> name from userspace, I don't see downsides even from a compatibility
> perspective for not making this change. In cases where checking *is*
> done from userspace, the broadening of the Clearfog product line would
> seem to mean that userspace checking should be flagged as needing to be
> udpated as well (or glob/regex matched as needed).

One downside I see is that boot of kernels older than 4.11 will fail. But 
maybe since we already assume a newer kernel for armada-388-clearfog-base.dtb 
we can do that for -pro as well.

By the way, does env_set() override the stored environment?

baruch

> 
> Signed-off-by: Joel Johnson <mrjoel at lixil.net>
> ---
>  board/solidrun/clearfog/clearfog.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/board/solidrun/clearfog/clearfog.c b/board/solidrun/clearfog/clearfog.c
> index 4170fd4775..c31cfcb242 100644
> --- a/board/solidrun/clearfog/clearfog.c
> +++ b/board/solidrun/clearfog/clearfog.c
> @@ -238,7 +238,7 @@ int board_late_init(void)
>  	else if (IS_ENABLED(CONFIG_TARGET_CLEARFOG_BASE))
>  		env_set("fdtfile", "armada-388-clearfog-base.dtb");
>  	else
> -		env_set("fdtfile", "armada-388-clearfog.dtb");
> +		env_set("fdtfile", "armada-388-clearfog-pro.dtb");
>  
>  	return 0;
>  }
Joel Johnson Jan. 28, 2020, 6:49 a.m. UTC | #2
On 2020-01-27 23:17, Baruch Siach wrote:
> Hi Joel,
> 
> On Mon, Jan 27, 2020 at 01:01:56PM -0700, Joel Johnson wrote:
>> Switch to explicitly using the Pro variant DT, which has been
>> available since Linux 4.11.
>> 
>> ---
>> 
>> v4 changes:
>>   - new
>> v5 changes:
>>   - none
>> 
>> I separated out this change to the end of the series since it drew
>> questioning on prior review. I'd still advocate for making the change,
>> since especially with the additions of static variants and runtime
>> detection, it becomes easier from within a booted kernel to identify 
>> the
>> type and version of U-Boot image installed. Without making this 
>> change,
>> it becomes less direct to determine an actual Pro vs. Base, vs old
>> U-Boot image maybe supporting some hybrid variant configuration.
>> 
>> Even in the Linux kernel adding of the Pro DTS, it is indicated that 
>> it
>> was meant for backwards compatibility.
>> 
>> Except for cases where checking is done directly against the product
>> name from userspace, I don't see downsides even from a compatibility
>> perspective for not making this change. In cases where checking *is*
>> done from userspace, the broadening of the Clearfog product line would
>> seem to mean that userspace checking should be flagged as needing to 
>> be
>> udpated as well (or glob/regex matched as needed).
> 
> One downside I see is that boot of kernels older than 4.11 will fail. 
> But
> maybe since we already assume a newer kernel for 
> armada-388-clearfog-base.dtb
> we can do that for -pro as well.

Older kernels is one case, and to be fair there may also be the case of 
custom images with a newer kernel that for whatever reason (old file 
glob patterns) don't ship the -pro.dtb file. My testing focused 
primarily on OpenWRT and Debian, and while certainly not pervasive, I've 
also tested with current Armbian and Arch Linux ARM builds.

> By the way, does env_set() override the stored environment?

I'm not entirely sure of the behavior with duplicate entries, however 
patch 7 in the series removes the fdtfile entry from being added via 
CONFIG_EXTRA_ENV_SETTINGS, so it is now only set in one consistent 
location within board_late_init().

Joel
Stefan Roese March 23, 2020, 9:26 a.m. UTC | #3
On 27.01.20 21:01, Joel Johnson wrote:
> Switch to explicitly using the Pro variant DT, which has been
> available since Linux 4.11.
> 
> ---
> 
> v4 changes:
>    - new
> v5 changes:
>    - none
> 
> I separated out this change to the end of the series since it drew
> questioning on prior review. I'd still advocate for making the change,
> since especially with the additions of static variants and runtime
> detection, it becomes easier from within a booted kernel to identify the
> type and version of U-Boot image installed. Without making this change,
> it becomes less direct to determine an actual Pro vs. Base, vs old
> U-Boot image maybe supporting some hybrid variant configuration.
> 
> Even in the Linux kernel adding of the Pro DTS, it is indicated that it
> was meant for backwards compatibility.
> 
> Except for cases where checking is done directly against the product
> name from userspace, I don't see downsides even from a compatibility
> perspective for not making this change. In cases where checking *is*
> done from userspace, the broadening of the Clearfog product line would
> seem to mean that userspace checking should be flagged as needing to be
> udpated as well (or glob/regex matched as needed).
> 
> Signed-off-by: Joel Johnson <mrjoel at lixil.net>

Again, please make sure to put the S-o-b line above the first "---"
line.

Other that that:

Reviewed-by: Stefan Roese <sr at denx.de>

Thanks,
Stefan
diff mbox series

Patch

diff --git a/board/solidrun/clearfog/clearfog.c b/board/solidrun/clearfog/clearfog.c
index 4170fd4775..c31cfcb242 100644
--- a/board/solidrun/clearfog/clearfog.c
+++ b/board/solidrun/clearfog/clearfog.c
@@ -238,7 +238,7 @@  int board_late_init(void)
 	else if (IS_ENABLED(CONFIG_TARGET_CLEARFOG_BASE))
 		env_set("fdtfile", "armada-388-clearfog-base.dtb");
 	else
-		env_set("fdtfile", "armada-388-clearfog.dtb");
+		env_set("fdtfile", "armada-388-clearfog-pro.dtb");
 
 	return 0;
 }