| Message ID | 20210722154943.127775-7-oleksandr.suvorov@toradex.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | Patches for Toradex boards | expand |
On 7/22/21 5:49 PM, Oleksandr Suvorov wrote: [...] > diff --git a/include/configs/apalis-imx8.h b/include/configs/apalis-imx8.h > index b04a03f76d..5a90e03edb 100644 > --- a/include/configs/apalis-imx8.h > +++ b/include/configs/apalis-imx8.h > @@ -24,6 +24,7 @@ > #define CONFIG_IPADDR 192.168.10.2 > #define CONFIG_NETMASK 255.255.255.0 > #define CONFIG_SERVERIP 192.168.10.1 > +#define CONFIG_ROOTPATH "/srv/nfs" Rather, all this should be removed, since all these IP addresses and root path settings are invalid for most places where the system will be used. The user should define their own. It is not recommended to hard code this in board config.
Hello Marek, On Thu, Jul 22, 2021 at 7:08 PM Marek Vasut <marex@denx.de> wrote: > > On 7/22/21 5:49 PM, Oleksandr Suvorov wrote: > > [...] > > > diff --git a/include/configs/apalis-imx8.h b/include/configs/apalis-imx8.h > > index b04a03f76d..5a90e03edb 100644 > > --- a/include/configs/apalis-imx8.h > > +++ b/include/configs/apalis-imx8.h > > @@ -24,6 +24,7 @@ > > #define CONFIG_IPADDR 192.168.10.2 > > #define CONFIG_NETMASK 255.255.255.0 > > #define CONFIG_SERVERIP 192.168.10.1 > > +#define CONFIG_ROOTPATH "/srv/nfs" > > Rather, all this should be removed, since all these IP addresses and > root path settings are invalid for most places where the system will be > used. The user should define their own. It is not recommended to hard > code this in board config. Is it acceptable to move this stuff to Toradex Kconfigs as default values? Best regards Oleksandr Suvorov Toradex AG Ebenaustrasse 10 | 6048 Horw | Switzerland | T: +41 41 500 48 00
On 7/22/21 6:32 PM, Oleksandr Suvorov wrote: > Hello Marek, > > On Thu, Jul 22, 2021 at 7:08 PM Marek Vasut <marex@denx.de> wrote: >> >> On 7/22/21 5:49 PM, Oleksandr Suvorov wrote: >> >> [...] >> >>> diff --git a/include/configs/apalis-imx8.h b/include/configs/apalis-imx8.h >>> index b04a03f76d..5a90e03edb 100644 >>> --- a/include/configs/apalis-imx8.h >>> +++ b/include/configs/apalis-imx8.h >>> @@ -24,6 +24,7 @@ >>> #define CONFIG_IPADDR 192.168.10.2 >>> #define CONFIG_NETMASK 255.255.255.0 >>> #define CONFIG_SERVERIP 192.168.10.1 >>> +#define CONFIG_ROOTPATH "/srv/nfs" >> >> Rather, all this should be removed, since all these IP addresses and >> root path settings are invalid for most places where the system will be >> used. The user should define their own. It is not recommended to hard >> code this in board config. > > Is it acceptable to move this stuff to Toradex Kconfigs as default values? The general recommendation is to not set such default values at all, just let the user configure what they need.
On Thu, Jul 22, 2021 at 9:06 PM Marek Vasut <marex@denx.de> wrote: > > On 7/22/21 6:32 PM, Oleksandr Suvorov wrote: > > Hello Marek, > > > > On Thu, Jul 22, 2021 at 7:08 PM Marek Vasut <marex@denx.de> wrote: > >> > >> On 7/22/21 5:49 PM, Oleksandr Suvorov wrote: > >> > >> [...] > >> > >>> diff --git a/include/configs/apalis-imx8.h b/include/configs/apalis-imx8.h > >>> index b04a03f76d..5a90e03edb 100644 > >>> --- a/include/configs/apalis-imx8.h > >>> +++ b/include/configs/apalis-imx8.h > >>> @@ -24,6 +24,7 @@ > >>> #define CONFIG_IPADDR 192.168.10.2 > >>> #define CONFIG_NETMASK 255.255.255.0 > >>> #define CONFIG_SERVERIP 192.168.10.1 > >>> +#define CONFIG_ROOTPATH "/srv/nfs" > >> > >> Rather, all this should be removed, since all these IP addresses and > >> root path settings are invalid for most places where the system will be > >> used. The user should define their own. It is not recommended to hard > >> code this in board config. > > > > Is it acceptable to move this stuff to Toradex Kconfigs as default values? > > The general recommendation is to not set such default values at all, > just let the user configure what they need. Thanks for your feedback! I'll remove this patch from the next version of the patchset.
On 7/22/21 8:10 PM, Oleksandr Suvorov wrote: > On Thu, Jul 22, 2021 at 9:06 PM Marek Vasut <marex@denx.de> wrote: >> >> On 7/22/21 6:32 PM, Oleksandr Suvorov wrote: >>> Hello Marek, >>> >>> On Thu, Jul 22, 2021 at 7:08 PM Marek Vasut <marex@denx.de> wrote: >>>> >>>> On 7/22/21 5:49 PM, Oleksandr Suvorov wrote: >>>> >>>> [...] >>>> >>>>> diff --git a/include/configs/apalis-imx8.h b/include/configs/apalis-imx8.h >>>>> index b04a03f76d..5a90e03edb 100644 >>>>> --- a/include/configs/apalis-imx8.h >>>>> +++ b/include/configs/apalis-imx8.h >>>>> @@ -24,6 +24,7 @@ >>>>> #define CONFIG_IPADDR 192.168.10.2 >>>>> #define CONFIG_NETMASK 255.255.255.0 >>>>> #define CONFIG_SERVERIP 192.168.10.1 >>>>> +#define CONFIG_ROOTPATH "/srv/nfs" >>>> >>>> Rather, all this should be removed, since all these IP addresses and >>>> root path settings are invalid for most places where the system will be >>>> used. The user should define their own. It is not recommended to hard >>>> code this in board config. >>> >>> Is it acceptable to move this stuff to Toradex Kconfigs as default values? >> >> The general recommendation is to not set such default values at all, >> just let the user configure what they need. > > Thanks for your feedback! I'll remove this patch from the next version > of the patchset. You likely also want to remove all those CONFIG_IPADDR and co.
On Thu, Jul 22, 2021 at 9:18 PM Marek Vasut <marex@denx.de> wrote: > > On 7/22/21 8:10 PM, Oleksandr Suvorov wrote: > > On Thu, Jul 22, 2021 at 9:06 PM Marek Vasut <marex@denx.de> wrote: > >> > >> On 7/22/21 6:32 PM, Oleksandr Suvorov wrote: > >>> Hello Marek, > >>> > >>> On Thu, Jul 22, 2021 at 7:08 PM Marek Vasut <marex@denx.de> wrote: > >>>> > >>>> On 7/22/21 5:49 PM, Oleksandr Suvorov wrote: > >>>> > >>>> [...] > >>>> > >>>>> diff --git a/include/configs/apalis-imx8.h b/include/configs/apalis-imx8.h > >>>>> index b04a03f76d..5a90e03edb 100644 > >>>>> --- a/include/configs/apalis-imx8.h > >>>>> +++ b/include/configs/apalis-imx8.h > >>>>> @@ -24,6 +24,7 @@ > >>>>> #define CONFIG_IPADDR 192.168.10.2 > >>>>> #define CONFIG_NETMASK 255.255.255.0 > >>>>> #define CONFIG_SERVERIP 192.168.10.1 > >>>>> +#define CONFIG_ROOTPATH "/srv/nfs" > >>>> > >>>> Rather, all this should be removed, since all these IP addresses and > >>>> root path settings are invalid for most places where the system will be > >>>> used. The user should define their own. It is not recommended to hard > >>>> code this in board config. > >>> > >>> Is it acceptable to move this stuff to Toradex Kconfigs as default values? > >> > >> The general recommendation is to not set such default values at all, > >> just let the user configure what they need. > > > > Thanks for your feedback! I'll remove this patch from the next version > > of the patchset. > > You likely also want to remove all those CONFIG_IPADDR and co. Yes, thanks, probably by the next release :)
diff --git a/include/configs/apalis-imx8.h b/include/configs/apalis-imx8.h index b04a03f76d..5a90e03edb 100644 --- a/include/configs/apalis-imx8.h +++ b/include/configs/apalis-imx8.h @@ -24,6 +24,7 @@ #define CONFIG_IPADDR 192.168.10.2 #define CONFIG_NETMASK 255.255.255.0 #define CONFIG_SERVERIP 192.168.10.1 +#define CONFIG_ROOTPATH "/srv/nfs" #define MEM_LAYOUT_ENV_SETTINGS \ "fdt_addr_r=0x84000000\0" \ diff --git a/include/configs/apalis-imx8x.h b/include/configs/apalis-imx8x.h index 2ad4ca3418..ad3b77853c 100644 --- a/include/configs/apalis-imx8x.h +++ b/include/configs/apalis-imx8x.h @@ -22,6 +22,7 @@ #define CONFIG_IPADDR 192.168.10.2 #define CONFIG_NETMASK 255.255.255.0 #define CONFIG_SERVERIP 192.168.10.1 +#define CONFIG_ROOTPATH "/srv/nfs" #define MEM_LAYOUT_ENV_SETTINGS \ "kernel_addr_r=0x80280000\0" \ diff --git a/include/configs/apalis-tk1.h b/include/configs/apalis-tk1.h index 57192649ec..9c5c89a6e3 100644 --- a/include/configs/apalis-tk1.h +++ b/include/configs/apalis-tk1.h @@ -44,6 +44,7 @@ #define CONFIG_NETMASK 255.255.255.0 #undef CONFIG_SERVERIP #define CONFIG_SERVERIP 192.168.10.1 +#define CONFIG_ROOTPATH "/srv/nfs" #define DFU_ALT_EMMC_INFO "apalis-tk1.img raw 0x0 0x500 mmcpart 1; " \ "boot part 0 1 mmcpart 0; " \ diff --git a/include/configs/apalis_imx6.h b/include/configs/apalis_imx6.h index 12de0105c6..9bd3d7a848 100644 --- a/include/configs/apalis_imx6.h +++ b/include/configs/apalis_imx6.h @@ -79,6 +79,7 @@ #define CONFIG_NETMASK 255.255.255.0 #undef CONFIG_SERVERIP #define CONFIG_SERVERIP 192.168.10.1 +#define CONFIG_ROOTPATH "/srv/nfs" #define CONFIG_LOADADDR 0x12000000 diff --git a/include/configs/colibri-imx8x.h b/include/configs/colibri-imx8x.h index cb22b3c75a..acd3694a44 100644 --- a/include/configs/colibri-imx8x.h +++ b/include/configs/colibri-imx8x.h @@ -24,6 +24,7 @@ #define CONFIG_IPADDR 192.168.10.2 #define CONFIG_NETMASK 255.255.255.0 #define CONFIG_SERVERIP 192.168.10.1 +#define CONFIG_ROOTPATH "/srv/nfs" #define MEM_LAYOUT_ENV_SETTINGS \ "fdt_addr_r=0x83000000\0" \ diff --git a/include/configs/colibri_imx6.h b/include/configs/colibri_imx6.h index 804a144a03..02f3bf41b8 100644 --- a/include/configs/colibri_imx6.h +++ b/include/configs/colibri_imx6.h @@ -67,6 +67,7 @@ #define CONFIG_NETMASK 255.255.255.0 #undef CONFIG_SERVERIP #define CONFIG_SERVERIP 192.168.10.1 +#define CONFIG_ROOTPATH "/srv/nfs" #define CONFIG_LOADADDR 0x12000000 diff --git a/include/configs/colibri_imx7.h b/include/configs/colibri_imx7.h index 2fffaa39c0..008d8749c0 100644 --- a/include/configs/colibri_imx7.h +++ b/include/configs/colibri_imx7.h @@ -31,6 +31,7 @@ #define CONFIG_IPADDR 192.168.10.2 #define CONFIG_NETMASK 255.255.255.0 #define CONFIG_SERVERIP 192.168.10.1 +#define CONFIG_ROOTPATH "/srv/nfs" #if defined(CONFIG_TARGET_COLIBRI_IMX7_EMMC) #define UBOOT_UPDATE \
The rootpath is used by distroboot script as a default path to a rootfs on an NFS server. Set this properly for all Toradex apalis and colibri modules, that support booting with Distro Boot script. Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com> --- include/configs/apalis-imx8.h | 1 + include/configs/apalis-imx8x.h | 1 + include/configs/apalis-tk1.h | 1 + include/configs/apalis_imx6.h | 1 + include/configs/colibri-imx8x.h | 1 + include/configs/colibri_imx6.h | 1 + include/configs/colibri_imx7.h | 1 + 7 files changed, 7 insertions(+)