| Message ID | 20190818151607.9711-1-dr@jones.dk |
|---|---|
| State | Changes Requested |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
| Series | [U-Boot,1/2] sunxi: fix support board-specific CONFIG_PREBOOT | expand |
On Sun, 18 Aug 2019 17:16:06 +0200 Jonas Smedegaard <dr@jones.dk> wrote: > commit 37304aaf60bf ("Convert CONFIG_USE_PREBOOT and CONFIG_PREBOOT to > Kconfig") intended to support CONFIG_PREBOOT, but > include/configs/sunxi-common.h hardcodes preboot as part of internally > defined CONSOLE_STDIN_SETTINGS, silently ignoring any board-specific > CONFIG_PREBOOT. > > Tested-by: Jonas Smedegaard <dr@jones.dk> > Signed-off-by: Jonas Smedegaard <dr@jones.dk> > Series-Cc: Jagan Teki <jagan@amarulasolutions.com> > --- > > include/configs/sunxi-common.h | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/include/configs/sunxi-common.h > b/include/configs/sunxi-common.h index d7133a73fc..2069884b63 100644 > --- a/include/configs/sunxi-common.h > +++ b/include/configs/sunxi-common.h > @@ -432,8 +432,13 @@ extern int soft_i2c_gpio_scl; > #include <config_distro_bootcmd.h> > > #ifdef CONFIG_USB_KEYBOARD > +#ifdef CONFIG_USE_PREBOOT > +#ifndef CONFIG_PREBOOT > +#define CONFIG_PREBOOT \ > + "usb start" Shouldn't this (CONFIG_PREBOOT) be set in the Kconfig? > +#endif > +#endif > #define CONSOLE_STDIN_SETTINGS \ > - "preboot=usb start\0" \ > "stdin=serial,usbkbd\0" > #else > #define CONSOLE_STDIN_SETTINGS \ Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Quoting Lukasz Majewski (2019-08-23 10:37:28) > On Sun, 18 Aug 2019 17:16:06 +0200 > Jonas Smedegaard <dr@jones.dk> wrote: > > > commit 37304aaf60bf ("Convert CONFIG_USE_PREBOOT and CONFIG_PREBOOT to > > Kconfig") intended to support CONFIG_PREBOOT, but > > include/configs/sunxi-common.h hardcodes preboot as part of internally > > defined CONSOLE_STDIN_SETTINGS, silently ignoring any board-specific > > CONFIG_PREBOOT. > > > > Tested-by: Jonas Smedegaard <dr@jones.dk> > > Signed-off-by: Jonas Smedegaard <dr@jones.dk> > > Series-Cc: Jagan Teki <jagan@amarulasolutions.com> > > --- > > > > include/configs/sunxi-common.h | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/include/configs/sunxi-common.h > > b/include/configs/sunxi-common.h index d7133a73fc..2069884b63 100644 > > --- a/include/configs/sunxi-common.h > > +++ b/include/configs/sunxi-common.h > > @@ -432,8 +432,13 @@ extern int soft_i2c_gpio_scl; > > #include <config_distro_bootcmd.h> > > > > #ifdef CONFIG_USB_KEYBOARD > > +#ifdef CONFIG_USE_PREBOOT > > +#ifndef CONFIG_PREBOOT > > +#define CONFIG_PREBOOT \ > > + "usb start" > > Shouldn't this (CONFIG_PREBOOT) be set in the Kconfig? My changeset ensures that _if_ CONFIG_PREBOOT is set in Kconfig then it is not _also_ set in header file. Reason for my approach was to keep the changeset minimal. If my changeset is not acceptable without first/instead fixing the related but arguably different issue of "usb start" being declared in header file, please tell me and I shall try propose a fix for that. - Jonas
Hi Jonas, > Quoting Lukasz Majewski (2019-08-23 10:37:28) > > On Sun, 18 Aug 2019 17:16:06 +0200 > > Jonas Smedegaard <dr@jones.dk> wrote: > > > > > commit 37304aaf60bf ("Convert CONFIG_USE_PREBOOT and > > > CONFIG_PREBOOT to Kconfig") intended to support CONFIG_PREBOOT, > > > but include/configs/sunxi-common.h hardcodes preboot as part of > > > internally defined CONSOLE_STDIN_SETTINGS, silently ignoring any > > > board-specific CONFIG_PREBOOT. > > > > > > Tested-by: Jonas Smedegaard <dr@jones.dk> > > > Signed-off-by: Jonas Smedegaard <dr@jones.dk> > > > Series-Cc: Jagan Teki <jagan@amarulasolutions.com> > > > --- > > > > > > include/configs/sunxi-common.h | 7 ++++++- > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > diff --git a/include/configs/sunxi-common.h > > > b/include/configs/sunxi-common.h index d7133a73fc..2069884b63 > > > 100644 --- a/include/configs/sunxi-common.h > > > +++ b/include/configs/sunxi-common.h > > > @@ -432,8 +432,13 @@ extern int soft_i2c_gpio_scl; > > > #include <config_distro_bootcmd.h> > > > > > > #ifdef CONFIG_USB_KEYBOARD > > > +#ifdef CONFIG_USE_PREBOOT > > > +#ifndef CONFIG_PREBOOT > > > +#define CONFIG_PREBOOT \ > > > + "usb start" > > > > Shouldn't this (CONFIG_PREBOOT) be set in the Kconfig? > > My changeset ensures that _if_ CONFIG_PREBOOT is set in Kconfig then > it is not _also_ set in header file. > > Reason for my approach was to keep the changeset minimal. I think I have been misunderstood... The patch to which you referred in the commit message: commit 37304aaf60bf ("Convert CONFIG_USE_PREBOOT and CONFIG_PREBOOT to Kconfig") was supposed to move setting of CONFIG_PREBOOT to Kconfig. As it broke your setup - I proposed that you could add your fix not to sunxi-common.h, but to Kconfig. In that way you would benefit from the patch you refer to. > > If my changeset is not acceptable without first/instead fixing the > related but arguably different issue of "usb start" being declared in > header file, please tell me and I shall try propose a fix for that. > > > - Jonas > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi Lukasz, Quoting Lukasz Majewski (2019-08-23 23:05:41) > > Quoting Lukasz Majewski (2019-08-23 10:37:28) > > > On Sun, 18 Aug 2019 17:16:06 +0200 > > > Jonas Smedegaard <dr@jones.dk> wrote: > > > > > > > commit 37304aaf60bf ("Convert CONFIG_USE_PREBOOT and > > > > CONFIG_PREBOOT to Kconfig") intended to support CONFIG_PREBOOT, > > > > but include/configs/sunxi-common.h hardcodes preboot as part of > > > > internally defined CONSOLE_STDIN_SETTINGS, silently ignoring any > > > > board-specific CONFIG_PREBOOT. > > > > > > > > Tested-by: Jonas Smedegaard <dr@jones.dk> > > > > Signed-off-by: Jonas Smedegaard <dr@jones.dk> > > > > Series-Cc: Jagan Teki <jagan@amarulasolutions.com> > > > > --- > > > > > > > > include/configs/sunxi-common.h | 7 ++++++- > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/include/configs/sunxi-common.h > > > > b/include/configs/sunxi-common.h index d7133a73fc..2069884b63 > > > > 100644 --- a/include/configs/sunxi-common.h > > > > +++ b/include/configs/sunxi-common.h > > > > @@ -432,8 +432,13 @@ extern int soft_i2c_gpio_scl; > > > > #include <config_distro_bootcmd.h> > > > > > > > > #ifdef CONFIG_USB_KEYBOARD > > > > +#ifdef CONFIG_USE_PREBOOT > > > > +#ifndef CONFIG_PREBOOT > > > > +#define CONFIG_PREBOOT \ > > > > + "usb start" > > > > > > Shouldn't this (CONFIG_PREBOOT) be set in the Kconfig? > > > > My changeset ensures that _if_ CONFIG_PREBOOT is set in Kconfig then > > it is not _also_ set in header file. > > > > Reason for my approach was to keep the changeset minimal. > > I think I have been misunderstood... Likewise... :-) > The patch to which you referred in the commit message: > > commit 37304aaf60bf ("Convert CONFIG_USE_PREBOOT and CONFIG_PREBOOT to > Kconfig") > > was supposed to move setting of CONFIG_PREBOOT to Kconfig. > > As it broke your setup - I proposed that you could add your fix not to > sunxi-common.h, but to Kconfig. In that way you would benefit from the > patch you refer to. commit 37304aaf60bf did not break my setup. This proposed patch _improves_ commit 37304aaf60bf to make it possible to pass CONFIG_USE_PREBOOT in Kconfig on sunxi devices. In fact, that is the very thing that next patch in this patchset does - which is what fixes my device: My device was broken also before commit 37304aaf60bf. This patchset still applies when unfuzzed. Should I make a non-change v2 of this patch, or do anyone have other comments on it? - Jonas
Hi Jonas, > Hi Lukasz, > > Quoting Lukasz Majewski (2019-08-23 23:05:41) Some time has already passed ... :-) > > > Quoting Lukasz Majewski (2019-08-23 10:37:28) > > > > On Sun, 18 Aug 2019 17:16:06 +0200 > > > > Jonas Smedegaard <dr@jones.dk> wrote: > > > > > > > > > commit 37304aaf60bf ("Convert CONFIG_USE_PREBOOT and > > > > > CONFIG_PREBOOT to Kconfig") intended to support > > > > > CONFIG_PREBOOT, but include/configs/sunxi-common.h hardcodes > > > > > preboot as part of internally defined CONSOLE_STDIN_SETTINGS, > > > > > silently ignoring any board-specific CONFIG_PREBOOT. > > > > > > > > > > Tested-by: Jonas Smedegaard <dr@jones.dk> > > > > > Signed-off-by: Jonas Smedegaard <dr@jones.dk> > > > > > Series-Cc: Jagan Teki <jagan@amarulasolutions.com> > > > > > --- > > > > > > > > > > include/configs/sunxi-common.h | 7 ++++++- > > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/include/configs/sunxi-common.h > > > > > b/include/configs/sunxi-common.h index d7133a73fc..2069884b63 > > > > > 100644 --- a/include/configs/sunxi-common.h > > > > > +++ b/include/configs/sunxi-common.h > > > > > @@ -432,8 +432,13 @@ extern int soft_i2c_gpio_scl; > > > > > #include <config_distro_bootcmd.h> > > > > > > > > > > #ifdef CONFIG_USB_KEYBOARD > > > > > +#ifdef CONFIG_USE_PREBOOT > > > > > +#ifndef CONFIG_PREBOOT > > > > > +#define CONFIG_PREBOOT \ > > > > > + "usb start" > > > > > > > > Shouldn't this (CONFIG_PREBOOT) be set in the Kconfig? > > > > > > My changeset ensures that _if_ CONFIG_PREBOOT is set in Kconfig > > > then it is not _also_ set in header file. > > > > > > Reason for my approach was to keep the changeset minimal. > > > > I think I have been misunderstood... > > Likewise... :-) > > > The patch to which you referred in the commit message: > > > > commit 37304aaf60bf ("Convert CONFIG_USE_PREBOOT and CONFIG_PREBOOT > > to Kconfig") > > > > was supposed to move setting of CONFIG_PREBOOT to Kconfig. > > > > As it broke your setup - I proposed that you could add your fix not > > to sunxi-common.h, but to Kconfig. In that way you would benefit > > from the patch you refer to. > > commit 37304aaf60bf did not break my setup. > > This proposed patch _improves_ commit 37304aaf60bf to make it > possible to pass CONFIG_USE_PREBOOT in Kconfig on sunxi devices. > > In fact, that is the very thing that next patch in this patchset does > - which is what fixes my device: My device was broken also before > commit 37304aaf60bf. > > > This patchset still applies when unfuzzed. Should I make a > non-change v2 of this patch, or do anyone have other comments on it? > If it fixes the issue on your board (and don't break other boards), then prepare v2 on top of newest -master and post the patch (adding sunxi maintainer/custodian to TO). It shall be applied as a fix before next release. > > - Jonas > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h index d7133a73fc..2069884b63 100644 --- a/include/configs/sunxi-common.h +++ b/include/configs/sunxi-common.h @@ -432,8 +432,13 @@ extern int soft_i2c_gpio_scl; #include <config_distro_bootcmd.h> #ifdef CONFIG_USB_KEYBOARD +#ifdef CONFIG_USE_PREBOOT +#ifndef CONFIG_PREBOOT +#define CONFIG_PREBOOT \ + "usb start" +#endif +#endif #define CONSOLE_STDIN_SETTINGS \ - "preboot=usb start\0" \ "stdin=serial,usbkbd\0" #else #define CONSOLE_STDIN_SETTINGS \
commit 37304aaf60bf ("Convert CONFIG_USE_PREBOOT and CONFIG_PREBOOT to Kconfig") intended to support CONFIG_PREBOOT, but include/configs/sunxi-common.h hardcodes preboot as part of internally defined CONSOLE_STDIN_SETTINGS, silently ignoring any board-specific CONFIG_PREBOOT. Tested-by: Jonas Smedegaard <dr@jones.dk> Signed-off-by: Jonas Smedegaard <dr@jones.dk> Series-Cc: Jagan Teki <jagan@amarulasolutions.com> --- include/configs/sunxi-common.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)