| Message ID | 1411224878-13692-5-git-send-email-hdegoede@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Marek Vasut |
| Headers | show |
Dear Hans de Goede, Typographical error here: > --- a/drivers/serial/serial-uclass.c > +++ b/drivers/serial/serial-uclass.c > <at> <at> -197,7 +197,7 <at> <at> static int serial_pre_remove(struct udevice *dev) > #ifdef CONFIG_SYS_STDIO_DEREGISTER > struct serial_dev_priv *upriv = dev->uclass_priv; > > - if (stdio_deregister_dev(upriv->sdev)) > + if (stdio_deregister_dev(upriv->sdev), 0) Breaks sandbox build, and probably others. All the best, RgC Hans de Goede <hdegoede <at> redhat.com> writes: > > In some cases we really want to move forward with a deregister, add a force > parameter to allow this, and replace the dev with a nulldev in this case. > > Signed-off-by: Hans de Goede <hdegoede <at> redhat.com>
Hi, On 20 September 2014 08:54, Hans de Goede <hdegoede@redhat.com> wrote: > In some cases we really want to move forward with a deregister, add a force > parameter to allow this, and replace the dev with a nulldev in this case. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > common/stdio.c | 13 ++++++++++--- > common/usb_kbd.c | 2 +- > drivers/serial/serial-uclass.c | 2 +- > include/stdio_dev.h | 4 ++-- > 4 files changed, 14 insertions(+), 7 deletions(-) > > diff --git a/common/stdio.c b/common/stdio.c > index c878103..8232815 100644 > --- a/common/stdio.c > +++ b/common/stdio.c > @@ -34,6 +34,9 @@ char *stdio_names[MAX_FILES] = { "stdin", "stdout", "stderr" }; > #define CONFIG_SYS_DEVICE_NULLDEV 1 > #endif > > +#ifdef CONFIG_SYS_STDIO_DEREGISTER > +#define CONFIG_SYS_DEVICE_NULLDEV 1 > +#endif > > #ifdef CONFIG_SYS_DEVICE_NULLDEV > void nulldev_putc(struct stdio_dev *dev, const char c) > @@ -172,7 +175,7 @@ int stdio_register(struct stdio_dev *dev) > * returns 0 if success, -1 if device is assigned and 1 if devname not found > */ > #ifdef CONFIG_SYS_STDIO_DEREGISTER > -int stdio_deregister_dev(struct stdio_dev *dev) > +int stdio_deregister_dev(struct stdio_dev *dev, int force) > { > int l; > struct list_head *pos; > @@ -181,6 +184,10 @@ int stdio_deregister_dev(struct stdio_dev *dev) > /* get stdio devices (ListRemoveItem changes the dev list) */ > for (l=0 ; l< MAX_FILES; l++) { > if (stdio_devices[l] == dev) { > + if (force) { > + strcpy(temp_names[l], "nulldev"); > + continue; > + } > /* Device is assigned -> report error */ > return -1; > } > @@ -202,7 +209,7 @@ int stdio_deregister_dev(struct stdio_dev *dev) > return 0; > } > > -int stdio_deregister(const char *devname) > +int stdio_deregister(const char *devname, int force) > { > struct stdio_dev *dev; > > @@ -211,7 +218,7 @@ int stdio_deregister(const char *devname) > if (!dev) /* device not found */ > return -ENODEV; > > - return stdio_deregister_dev(dev); > + return stdio_deregister_dev(dev, force); > } > #endif /* CONFIG_SYS_STDIO_DEREGISTER */ > > diff --git a/common/usb_kbd.c b/common/usb_kbd.c > index d4d5f48..dcb693d 100644 > --- a/common/usb_kbd.c > +++ b/common/usb_kbd.c > @@ -550,7 +550,7 @@ int drv_usb_kbd_init(void) > int usb_kbd_deregister(void) > { > #ifdef CONFIG_SYS_STDIO_DEREGISTER > - int ret = stdio_deregister(DEVNAME); > + int ret = stdio_deregister(DEVNAME, 0); > if (ret && ret != -ENODEV) > return ret; > > diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c > index d04104e..61cbdc6 100644 > --- a/drivers/serial/serial-uclass.c > +++ b/drivers/serial/serial-uclass.c > @@ -197,7 +197,7 @@ static int serial_pre_remove(struct udevice *dev) > #ifdef CONFIG_SYS_STDIO_DEREGISTER > struct serial_dev_priv *upriv = dev->uclass_priv; > > - if (stdio_deregister_dev(upriv->sdev)) > + if (stdio_deregister_dev(upriv->sdev), 0) That bracket seems to be in a strange place. > return -EPERM; > #endif > > diff --git a/include/stdio_dev.h b/include/stdio_dev.h > index 268de8e..24da23f 100644 > --- a/include/stdio_dev.h > +++ b/include/stdio_dev.h > @@ -103,8 +103,8 @@ int stdio_init(void); > > void stdio_print_current_devices(void); > #ifdef CONFIG_SYS_STDIO_DEREGISTER > -int stdio_deregister(const char *devname); > -int stdio_deregister_dev(struct stdio_dev *dev); > +int stdio_deregister(const char *devname, int force); > +int stdio_deregister_dev(struct stdio_dev *dev, int force); > #endif > struct list_head* stdio_get_list(void); > struct stdio_dev* stdio_get_by_name(const char* name); > -- > 2.1.0 > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot Regards, Simon
On Thursday, October 09, 2014 at 08:18:14 AM, Simon Glass wrote: > Hi, > > On 20 September 2014 08:54, Hans de Goede <hdegoede@redhat.com> wrote: > > In some cases we really want to move forward with a deregister, add a > > force parameter to allow this, and replace the dev with a nulldev in > > this case. > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> [...] > > diff --git a/drivers/serial/serial-uclass.c > > b/drivers/serial/serial-uclass.c index d04104e..61cbdc6 100644 > > --- a/drivers/serial/serial-uclass.c > > +++ b/drivers/serial/serial-uclass.c > > @@ -197,7 +197,7 @@ static int serial_pre_remove(struct udevice *dev) > > > > #ifdef CONFIG_SYS_STDIO_DEREGISTER > > > > struct serial_dev_priv *upriv = dev->uclass_priv; > > > > - if (stdio_deregister_dev(upriv->sdev)) > > + if (stdio_deregister_dev(upriv->sdev), 0) > > That bracket seems to be in a strange place. Good find, thanks! I have two questions: 1) How come I did not notice this and my build didn't spit? 2) Can either of you guys please prepare a patch? Best regards, Marek Vasut
Hi Marek, On 9 October 2014 09:12, Marek Vasut <marex@denx.de> wrote: > > On Thursday, October 09, 2014 at 08:18:14 AM, Simon Glass wrote: > > Hi, > > > > On 20 September 2014 08:54, Hans de Goede <hdegoede@redhat.com> wrote: > > > In some cases we really want to move forward with a deregister, add a > > > force parameter to allow this, and replace the dev with a nulldev in > > > this case. > > > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > [...] > > > > diff --git a/drivers/serial/serial-uclass.c > > > b/drivers/serial/serial-uclass.c index d04104e..61cbdc6 100644 > > > --- a/drivers/serial/serial-uclass.c > > > +++ b/drivers/serial/serial-uclass.c > > > @@ -197,7 +197,7 @@ static int serial_pre_remove(struct udevice *dev) > > > > > > #ifdef CONFIG_SYS_STDIO_DEREGISTER > > > > > > struct serial_dev_priv *upriv = dev->uclass_priv; > > > > > > - if (stdio_deregister_dev(upriv->sdev)) > > > + if (stdio_deregister_dev(upriv->sdev), 0) > > > > That bracket seems to be in a strange place. > > Good find, thanks! I have two questions: > 1) How come I did not notice this and my build didn't spit? If you have CONFIG_SYS_STDIO_DEREGISTER, CONFIG_DM and CONFIG_DM_SERIAL set then I'm not sure. I made sure that sandbox has all of these but it might be the only board. > > 2) Can either of you guys please prepare a patch? > > Best regards, > Marek Vasut Regards, Simon
On Thu, Oct 09, 2014 at 05:12:03PM +0200, Marek Vasut wrote: > On Thursday, October 09, 2014 at 08:18:14 AM, Simon Glass wrote: > > Hi, > > > > On 20 September 2014 08:54, Hans de Goede <hdegoede@redhat.com> wrote: > > > In some cases we really want to move forward with a deregister, add a > > > force parameter to allow this, and replace the dev with a nulldev in > > > this case. > > > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > [...] > > > > diff --git a/drivers/serial/serial-uclass.c > > > b/drivers/serial/serial-uclass.c index d04104e..61cbdc6 100644 > > > --- a/drivers/serial/serial-uclass.c > > > +++ b/drivers/serial/serial-uclass.c > > > @@ -197,7 +197,7 @@ static int serial_pre_remove(struct udevice *dev) > > > > > > #ifdef CONFIG_SYS_STDIO_DEREGISTER > > > > > > struct serial_dev_priv *upriv = dev->uclass_priv; > > > > > > - if (stdio_deregister_dev(upriv->sdev)) > > > + if (stdio_deregister_dev(upriv->sdev), 0) > > > > That bracket seems to be in a strange place. > > Good find, thanks! I have two questions: > 1) How come I did not notice this and my build didn't spit? Because only sandbox uses this right now I think. But I'm unhappy that I can't seem to make repeated runs of: $ ./tools/buildman/buildman -b master -c 1 -ve -T 1 -j 9 \ 'arc|blackfin|microblaze|m68k|nds32|sparc|x86|aarch64|sandbox|mips|avr32|arm|powerpc' $ ./tools/buildman/buildman -b master -c 1 -ve -T 1 -j 9 \ 'arc|blackfin|microblaze|m68k|nds32|sparc|x86|aarch64|sandbox|mips|avr32|arm|powerpc' \ -svel Show me just new problems from the last time I did it. I must be doing something wrong, Simon?
On Thursday, October 09, 2014 at 06:14:11 PM, Simon Glass wrote: > Hi Marek, > > On 9 October 2014 09:12, Marek Vasut <marex@denx.de> wrote: > > On Thursday, October 09, 2014 at 08:18:14 AM, Simon Glass wrote: > > > Hi, > > > > > > On 20 September 2014 08:54, Hans de Goede <hdegoede@redhat.com> wrote: > > > > In some cases we really want to move forward with a deregister, add a > > > > force parameter to allow this, and replace the dev with a nulldev in > > > > this case. > > > > > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > > [...] > > > > > > diff --git a/drivers/serial/serial-uclass.c > > > > b/drivers/serial/serial-uclass.c index d04104e..61cbdc6 100644 > > > > --- a/drivers/serial/serial-uclass.c > > > > +++ b/drivers/serial/serial-uclass.c > > > > @@ -197,7 +197,7 @@ static int serial_pre_remove(struct udevice *dev) > > > > > > > > #ifdef CONFIG_SYS_STDIO_DEREGISTER > > > > > > > > struct serial_dev_priv *upriv = dev->uclass_priv; > > > > > > > > - if (stdio_deregister_dev(upriv->sdev)) > > > > + if (stdio_deregister_dev(upriv->sdev), 0) > > > > > > That bracket seems to be in a strange place. > > > > Good find, thanks! I have two questions: > > 1) How come I did not notice this and my build didn't spit? > > If you have CONFIG_SYS_STDIO_DEREGISTER, CONFIG_DM and > CONFIG_DM_SERIAL set then I'm not sure. I made sure that sandbox has > all of these but it might be the only board. I see, error on my end then. I will start building sandbox for the USB tree. Thank you for pointing this out! This also stresses my point that U-Boot project does need a proper CI (which we could have had thanks to Vadim, but we didn't persudate that, dang again). I think this CI stuff should be added to the agenda of the U-Boot minisummit discussion. Another point to the CI discussion could be that we could prepare a docker image with all the toolchains preinstalled, so one can run buildman easily in a well defined environment on his/her own. Best regards, Marek Vasut
Hi Tom, On 9 October 2014 10:23, Tom Rini <trini@ti.com> wrote: > On Thu, Oct 09, 2014 at 05:12:03PM +0200, Marek Vasut wrote: >> On Thursday, October 09, 2014 at 08:18:14 AM, Simon Glass wrote: >> > Hi, >> > >> > On 20 September 2014 08:54, Hans de Goede <hdegoede@redhat.com> wrote: >> > > In some cases we really want to move forward with a deregister, add a >> > > force parameter to allow this, and replace the dev with a nulldev in >> > > this case. >> > > >> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> >> [...] >> >> > > diff --git a/drivers/serial/serial-uclass.c >> > > b/drivers/serial/serial-uclass.c index d04104e..61cbdc6 100644 >> > > --- a/drivers/serial/serial-uclass.c >> > > +++ b/drivers/serial/serial-uclass.c >> > > @@ -197,7 +197,7 @@ static int serial_pre_remove(struct udevice *dev) >> > > >> > > #ifdef CONFIG_SYS_STDIO_DEREGISTER >> > > >> > > struct serial_dev_priv *upriv = dev->uclass_priv; >> > > >> > > - if (stdio_deregister_dev(upriv->sdev)) >> > > + if (stdio_deregister_dev(upriv->sdev), 0) >> > >> > That bracket seems to be in a strange place. >> >> Good find, thanks! I have two questions: >> 1) How come I did not notice this and my build didn't spit? > > Because only sandbox uses this right now I think. But I'm unhappy that > I can't seem to make repeated runs of: > $ ./tools/buildman/buildman -b master -c 1 -ve -T 1 -j 9 \ > 'arc|blackfin|microblaze|m68k|nds32|sparc|x86|aarch64|sandbox|mips|avr32|arm|powerpc' > $ ./tools/buildman/buildman -b master -c 1 -ve -T 1 -j 9 \ > 'arc|blackfin|microblaze|m68k|nds32|sparc|x86|aarch64|sandbox|mips|avr32|arm|powerpc' \ > -svel > > Show me just new problems from the last time I did it. I must be doing > something wrong, Simon? I don't really see anything obviously wrong. But I'm not sure what you are expecting. This is going to just build the top commit in branch master, and if the commit hash has changed it will remove any previous results for that commit before starting the build. So you will always get a full list of errors, not a delta from last time. If you want that you could add a second commit with your fixes and not adjust the first commit in the branch (and use -c2). If you leave off '-b master -c1' it would have about the same effect, assuming that you have 'master' checked out. In the second line you are specifying -ve twice but that doesn't matter. Why are you changing the thread/jobs defaults? Does that speed it up? Also you don't need the quotes and the | between archs. Also note there is --step to avoid building every commit. For example, this will build the upstream commit and your branch's top commit (only) assuming that master tracks upstream/master. buildman -b master --step 0 Regards, Simon
Hi Marek, On 9 October 2014 10:27, Marek Vasut <marex@denx.de> wrote: > On Thursday, October 09, 2014 at 06:14:11 PM, Simon Glass wrote: >> Hi Marek, >> >> On 9 October 2014 09:12, Marek Vasut <marex@denx.de> wrote: >> > On Thursday, October 09, 2014 at 08:18:14 AM, Simon Glass wrote: >> > > Hi, >> > > >> > > On 20 September 2014 08:54, Hans de Goede <hdegoede@redhat.com> wrote: >> > > > In some cases we really want to move forward with a deregister, add a >> > > > force parameter to allow this, and replace the dev with a nulldev in >> > > > this case. >> > > > >> > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> > >> > [...] >> > >> > > > diff --git a/drivers/serial/serial-uclass.c >> > > > b/drivers/serial/serial-uclass.c index d04104e..61cbdc6 100644 >> > > > --- a/drivers/serial/serial-uclass.c >> > > > +++ b/drivers/serial/serial-uclass.c >> > > > @@ -197,7 +197,7 @@ static int serial_pre_remove(struct udevice *dev) >> > > > >> > > > #ifdef CONFIG_SYS_STDIO_DEREGISTER >> > > > >> > > > struct serial_dev_priv *upriv = dev->uclass_priv; >> > > > >> > > > - if (stdio_deregister_dev(upriv->sdev)) >> > > > + if (stdio_deregister_dev(upriv->sdev), 0) >> > > >> > > That bracket seems to be in a strange place. >> > >> > Good find, thanks! I have two questions: >> > 1) How come I did not notice this and my build didn't spit? >> >> If you have CONFIG_SYS_STDIO_DEREGISTER, CONFIG_DM and >> CONFIG_DM_SERIAL set then I'm not sure. I made sure that sandbox has >> all of these but it might be the only board. > > I see, error on my end then. I will start building sandbox for the USB tree. > Thank you for pointing this out! This also stresses my point that U-Boot project > does need a proper CI (which we could have had thanks to Vadim, but we didn't > persudate that, dang again). What is a Cl? Do you mean his gerrit code review stuff? > > I think this CI stuff should be added to the agenda of the U-Boot minisummit > discussion. > > Another point to the CI discussion could be that we could prepare a docker image > with all the toolchains preinstalled, so one can run buildman easily in a well > defined environment on his/her own. > > Best regards, > Marek Vasut Regards, Simon
On Thu, Oct 09, 2014 at 11:03:02AM -0600, Simon Glass wrote: > Hi Tom, > > On 9 October 2014 10:23, Tom Rini <trini@ti.com> wrote: > > On Thu, Oct 09, 2014 at 05:12:03PM +0200, Marek Vasut wrote: > >> On Thursday, October 09, 2014 at 08:18:14 AM, Simon Glass wrote: > >> > Hi, > >> > > >> > On 20 September 2014 08:54, Hans de Goede <hdegoede@redhat.com> wrote: > >> > > In some cases we really want to move forward with a deregister, add a > >> > > force parameter to allow this, and replace the dev with a nulldev in > >> > > this case. > >> > > > >> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >> > >> [...] > >> > >> > > diff --git a/drivers/serial/serial-uclass.c > >> > > b/drivers/serial/serial-uclass.c index d04104e..61cbdc6 100644 > >> > > --- a/drivers/serial/serial-uclass.c > >> > > +++ b/drivers/serial/serial-uclass.c > >> > > @@ -197,7 +197,7 @@ static int serial_pre_remove(struct udevice *dev) > >> > > > >> > > #ifdef CONFIG_SYS_STDIO_DEREGISTER > >> > > > >> > > struct serial_dev_priv *upriv = dev->uclass_priv; > >> > > > >> > > - if (stdio_deregister_dev(upriv->sdev)) > >> > > + if (stdio_deregister_dev(upriv->sdev), 0) > >> > > >> > That bracket seems to be in a strange place. > >> > >> Good find, thanks! I have two questions: > >> 1) How come I did not notice this and my build didn't spit? > > > > Because only sandbox uses this right now I think. But I'm unhappy that > > I can't seem to make repeated runs of: > > $ ./tools/buildman/buildman -b master -c 1 -ve -T 1 -j 9 \ > > 'arc|blackfin|microblaze|m68k|nds32|sparc|x86|aarch64|sandbox|mips|avr32|arm|powerpc' > > $ ./tools/buildman/buildman -b master -c 1 -ve -T 1 -j 9 \ > > 'arc|blackfin|microblaze|m68k|nds32|sparc|x86|aarch64|sandbox|mips|avr32|arm|powerpc' \ > > -svel > > > > Show me just new problems from the last time I did it. I must be doing > > something wrong, Simon? > > I don't really see anything obviously wrong. But I'm not sure what you > are expecting. This is going to just build the top commit in branch > master, and if the commit hash has changed it will remove any previous > results for that commit before starting the build. So you will always > get a full list of errors, not a delta from last time. If you want > that you could add a second commit with your fixes and not adjust the > first commit in the branch (and use -c2). Ah, OK, this got me going towards what I wanted. I was assuming for some incorrect reason that with -c1 it would just implicitly compare vs the last build it had available. I really want -c2 as I should have a full build from the last go-round (which is to say really, last merge). This is what I wanted (a simplified build): trini@bill-the-cat:~/work/u-boot/u-boot (32d0192...)$ ./tools/buildman/buildman -b HEAD -c 2 -ve -T 1 -j 9 'sandbox|sparc' -svel boards.cfg is up to date. Nothing to do. Summary of 2 commits for 6 boards (1 thread, 9 jobs per thread) 01: usb: kbd: Remove check for already being registered sparc: + grsim_leon2 gr_cpci_ax2000 gr_xc3s_1500 grsim gr_ep2s60 +(grsim_leon2,gr_cpci_ax2000,gr_xc3s_1500,grsim,gr_ep2s60) disk/part.c: In function `get_device': +(grsim_leon2,gr_cpci_ax2000,gr_xc3s_1500,grsim,gr_ep2s60) disk/part.c:454: warning: 'hwpart' might be used uninitialized in this function 02: stdio: Add force parameter to stdio_deregister sandbox: + sandbox +(sandbox) drivers/serial/serial-uclass.c: In function ‘serial_pre_remove’: +(sandbox) drivers/serial/serial-uclass.c:201:2: error: too few arguments to function ‘stdio_deregister_dev’ +(sandbox) include/stdio_dev.h:107:5: note: declared here +(sandbox) make[2]: *** [drivers/serial/serial-uclass.o] Error 1 +(sandbox) make[1]: *** [drivers/serial] Error 2 +(sandbox) make: *** [sub-make] Error 2 w+(sandbox) drivers/serial/serial-uclass.c:201:39: warning: left-hand operand of comma expression has no effect [-Wunused-value] And I don't care about the stuff around 01 (it was like that before), I care about 02 which is new problems. > If you leave off '-b master -c1' it would have about the same effect, > assuming that you have 'master' checked out. > > In the second line you are specifying -ve twice but that doesn't > matter. Why are you changing the thread/jobs defaults? Does that speed > it up? We have some race condition still on very large machines that I haven't been able to track down. Doing it that way was less problematic I think. > Also you don't need the quotes and the | between archs. Oh, interesting. > Also note there is --step to avoid building every commit. For example, > this will build the upstream commit and your branch's top commit > (only) assuming that master tracks upstream/master. > > buildman -b master --step 0 I think this is what I really wanted, yes.
On Thursday, October 09, 2014 at 07:03:42 PM, Simon Glass wrote: > Hi Marek, > > On 9 October 2014 10:27, Marek Vasut <marex@denx.de> wrote: > > On Thursday, October 09, 2014 at 06:14:11 PM, Simon Glass wrote: > >> Hi Marek, > >> > >> On 9 October 2014 09:12, Marek Vasut <marex@denx.de> wrote: > >> > On Thursday, October 09, 2014 at 08:18:14 AM, Simon Glass wrote: > >> > > Hi, > >> > > > >> > > On 20 September 2014 08:54, Hans de Goede <hdegoede@redhat.com> wrote: > >> > > > In some cases we really want to move forward with a deregister, > >> > > > add a force parameter to allow this, and replace the dev with a > >> > > > nulldev in this case. > >> > > > > >> > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >> > > >> > [...] > >> > > >> > > > diff --git a/drivers/serial/serial-uclass.c > >> > > > b/drivers/serial/serial-uclass.c index d04104e..61cbdc6 100644 > >> > > > --- a/drivers/serial/serial-uclass.c > >> > > > +++ b/drivers/serial/serial-uclass.c > >> > > > @@ -197,7 +197,7 @@ static int serial_pre_remove(struct udevice > >> > > > *dev) > >> > > > > >> > > > #ifdef CONFIG_SYS_STDIO_DEREGISTER > >> > > > > >> > > > struct serial_dev_priv *upriv = dev->uclass_priv; > >> > > > > >> > > > - if (stdio_deregister_dev(upriv->sdev)) > >> > > > + if (stdio_deregister_dev(upriv->sdev), 0) > >> > > > >> > > That bracket seems to be in a strange place. > >> > > >> > Good find, thanks! I have two questions: > >> > 1) How come I did not notice this and my build didn't spit? > >> > >> If you have CONFIG_SYS_STDIO_DEREGISTER, CONFIG_DM and > >> CONFIG_DM_SERIAL set then I'm not sure. I made sure that sandbox has > >> all of these but it might be the only board. > > > > I see, error on my end then. I will start building sandbox for the USB > > tree. Thank you for pointing this out! This also stresses my point that > > U-Boot project does need a proper CI (which we could have had thanks to > > Vadim, but we didn't persudate that, dang again). > > What is a Cl? Do you mean his gerrit code review stuff? I mean more continuous integration (build testing) of the code before a PR is submitted to the ML. Right now, we all do our own thing when it comes to testing before PR, but it would be nice to have one easy way of doing the build testing before submitting the PR, don't you think ? This might apply to Linux too. Best regards, Marek Vasut
Hi Marek, On 9 October 2014 11:32, Marek Vasut <marex@denx.de> wrote: > On Thursday, October 09, 2014 at 07:03:42 PM, Simon Glass wrote: > > Hi Marek, > > > > On 9 October 2014 10:27, Marek Vasut <marex@denx.de> wrote: > > > On Thursday, October 09, 2014 at 06:14:11 PM, Simon Glass wrote: > > >> Hi Marek, > > >> > > >> On 9 October 2014 09:12, Marek Vasut <marex@denx.de> wrote: > > >> > On Thursday, October 09, 2014 at 08:18:14 AM, Simon Glass wrote: > > >> > > Hi, > > >> > > > > >> > > On 20 September 2014 08:54, Hans de Goede <hdegoede@redhat.com> > wrote: > > >> > > > In some cases we really want to move forward with a deregister, > > >> > > > add a force parameter to allow this, and replace the dev with a > > >> > > > nulldev in this case. > > >> > > > > > >> > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > >> > > > >> > [...] > > >> > > > >> > > > diff --git a/drivers/serial/serial-uclass.c > > >> > > > b/drivers/serial/serial-uclass.c index d04104e..61cbdc6 100644 > > >> > > > --- a/drivers/serial/serial-uclass.c > > >> > > > +++ b/drivers/serial/serial-uclass.c > > >> > > > @@ -197,7 +197,7 @@ static int serial_pre_remove(struct udevice > > >> > > > *dev) > > >> > > > > > >> > > > #ifdef CONFIG_SYS_STDIO_DEREGISTER > > >> > > > > > >> > > > struct serial_dev_priv *upriv = dev->uclass_priv; > > >> > > > > > >> > > > - if (stdio_deregister_dev(upriv->sdev)) > > >> > > > + if (stdio_deregister_dev(upriv->sdev), 0) > > >> > > > > >> > > That bracket seems to be in a strange place. > > >> > > > >> > Good find, thanks! I have two questions: > > >> > 1) How come I did not notice this and my build didn't spit? > > >> > > >> If you have CONFIG_SYS_STDIO_DEREGISTER, CONFIG_DM and > > >> CONFIG_DM_SERIAL set then I'm not sure. I made sure that sandbox has > > >> all of these but it might be the only board. > > > > > > I see, error on my end then. I will start building sandbox for the USB > > > tree. Thank you for pointing this out! This also stresses my point that > > > U-Boot project does need a proper CI (which we could have had thanks to > > > Vadim, but we didn't persudate that, dang again). > > > > What is a Cl? Do you mean his gerrit code review stuff? > > I mean more continuous integration (build testing) of the code before a PR > is submitted to the ML. Right now, we all do our own thing when it comes to > testing before PR, but it would be nice to have one easy way of doing the > build testing before submitting the PR, don't you think ? This might apply > to Linux too. > Sure it would be useful. Before submitting my pull request I get all the patches in a branch and run: ./tools/buildman/buildman -b x86-push This checks every commit for every board that I build, and gives me good confidence that no patch introduces new breakages. Regards, Simon
On Thursday, October 09, 2014 at 08:04:29 PM, Simon Glass wrote: > Hi Marek, Hi Simon, [..] > > I mean more continuous integration (build testing) of the code before a > > PR is submitted to the ML. Right now, we all do our own thing when it > > comes to testing before PR, but it would be nice to have one easy way of > > doing the build testing before submitting the PR, don't you think ? This > > might apply to Linux too. > > Sure it would be useful. Before submitting my pull request I get all the > patches in a branch and run: > > ./tools/buildman/buildman -b x86-push > > This checks every commit for every board that I build, and gives me good > confidence that no patch introduces new breakages. I agree that buildman solves the CI part nicely, but we also have the part where one has to install the myriad of toolchains for all the architectures to be able to do the compile testing. I wonder if this cannot be made easy in some way -- maybe through a re-usable docker image with all the parts in it already. Best regards, Marek Vasut
Hi Marek, On 9 October 2014 17:59, Marek Vasut <marex@denx.de> wrote: > On Thursday, October 09, 2014 at 08:04:29 PM, Simon Glass wrote: >> Hi Marek, > > Hi Simon, > > [..] > >> > I mean more continuous integration (build testing) of the code before a >> > PR is submitted to the ML. Right now, we all do our own thing when it >> > comes to testing before PR, but it would be nice to have one easy way of >> > doing the build testing before submitting the PR, don't you think ? This >> > might apply to Linux too. >> >> Sure it would be useful. Before submitting my pull request I get all the >> patches in a branch and run: >> >> ./tools/buildman/buildman -b x86-push >> >> This checks every commit for every board that I build, and gives me good >> confidence that no patch introduces new breakages. > > I agree that buildman solves the CI part nicely, but we also have the part > where one has to install the myriad of toolchains for all the architectures > to be able to do the compile testing. I wonder if this cannot be made easy > in some way -- maybe through a re-usable docker image with all the parts in > it already. It would be create if we could download all the toolchains from one place. Maybe we need a toolchain maintainer? Regards, Simon
Hi Marek, On Thu, Oct 9, 2014 at 8:59 PM, Marek Vasut <marex@denx.de> wrote: > I agree that buildman solves the CI part nicely, but we also have the part > where one has to install the myriad of toolchains for all the architectures > to be able to do the compile testing. I wonder if this cannot be made easy > in some way -- maybe through a re-usable docker image with all the parts in > it already. Maybe you could take a look at the make.cross script kbuild robot uses for cross compiling for lots of different architectures: https://lists.01.org/pipermail/kbuild-all/2014-October/006965.html
Hi, On 9 October 2014 20:06, Fabio Estevam <festevam@gmail.com> wrote: > Hi Marek, > > On Thu, Oct 9, 2014 at 8:59 PM, Marek Vasut <marex@denx.de> wrote: > >> I agree that buildman solves the CI part nicely, but we also have the part >> where one has to install the myriad of toolchains for all the architectures >> to be able to do the compile testing. I wonder if this cannot be made easy >> in some way -- maybe through a re-usable docker image with all the parts in >> it already. > > Maybe you could take a look at the make.cross script kbuild robot uses > for cross compiling for lots of different architectures: > https://lists.01.org/pipermail/kbuild-all/2014-October/006965.html It's not really the builder that is the problem - we have one. It's all the different toolchains that no one knows about. I still can't build all the boards successfully. Regards, Simon
On Thu, Oct 9, 2014 at 11:07 PM, Simon Glass <sjg@chromium.org> wrote: > It's not really the builder that is the problem - we have one. It's > all the different toolchains that no one knows about. I still can't > build all the boards successfully. Aren't all the toolchains U-boot need included in the make.cross script? https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross It allows to build for all the archs that the kernel support. Regards, Fabio Estevam
On Friday, October 10, 2014 at 04:00:00 AM, Simon Glass wrote: > Hi Marek, > > On 9 October 2014 17:59, Marek Vasut <marex@denx.de> wrote: > > On Thursday, October 09, 2014 at 08:04:29 PM, Simon Glass wrote: > >> Hi Marek, > > > > Hi Simon, > > > > [..] > > > >> > I mean more continuous integration (build testing) of the code before > >> > a PR is submitted to the ML. Right now, we all do our own thing when > >> > it comes to testing before PR, but it would be nice to have one easy > >> > way of doing the build testing before submitting the PR, don't you > >> > think ? This might apply to Linux too. > >> > >> Sure it would be useful. Before submitting my pull request I get all the > >> patches in a branch and run: > >> > >> ./tools/buildman/buildman -b x86-push > >> > >> This checks every commit for every board that I build, and gives me good > >> confidence that no patch introduces new breakages. > > > > I agree that buildman solves the CI part nicely, but we also have the > > part where one has to install the myriad of toolchains for all the > > architectures to be able to do the compile testing. I wonder if this > > cannot be made easy in some way -- maybe through a re-usable docker > > image with all the parts in it already. > > It would be create if we could download all the toolchains from one > place. Maybe we need a toolchain maintainer? What about [1], this is where we can source the more exotic toolchains from, can we not? I think it was even you who pointed me to this site and it really is a nice one ;-) [1] https://www.kernel.org/pub/tools/crosstool/ Best regards, Marek Vasut
On Thu, Oct 9, 2014 at 11:23 PM, Marek Vasut <marex@denx.de> wrote: > What about [1], this is where we can source the more exotic toolchains from, > can we not? I think it was even you who pointed me to this site and it really > is a nice one ;-) > > [1] https://www.kernel.org/pub/tools/crosstool/ Exactly, the https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross script pulls the toolchains from this same location ;-)
Hi, On 9 October 2014 20:26, Fabio Estevam <festevam@gmail.com> wrote: > On Thu, Oct 9, 2014 at 11:23 PM, Marek Vasut <marex@denx.de> wrote: > >> What about [1], this is where we can source the more exotic toolchains from, >> can we not? I think it was even you who pointed me to this site and it really >> is a nice one ;-) >> >> [1] https://www.kernel.org/pub/tools/crosstool/ > > Exactly, the https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross > script pulls the toolchains from this same location ;-) OK I see. So all we need is someone to add an option to buildman to pull them down and put them somewhere? Regards, Simon
On Friday, October 10, 2014 at 04:35:11 AM, Simon Glass wrote: > Hi, Hi, > On 9 October 2014 20:26, Fabio Estevam <festevam@gmail.com> wrote: > > On Thu, Oct 9, 2014 at 11:23 PM, Marek Vasut <marex@denx.de> wrote: > >> What about [1], this is where we can source the more exotic toolchains > >> from, can we not? I think it was even you who pointed me to this site > >> and it really is a nice one ;-) > >> > >> [1] https://www.kernel.org/pub/tools/crosstool/ > > > > Exactly, the > > https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbi > > n/make.cross script pulls the toolchains from this same location ;-) > > OK I see. So all we need is someone to add an option to buildman to > pull them down and put them somewhere? That might just work. Best regards, Marek Vasut
diff --git a/common/stdio.c b/common/stdio.c index c878103..8232815 100644 --- a/common/stdio.c +++ b/common/stdio.c @@ -34,6 +34,9 @@ char *stdio_names[MAX_FILES] = { "stdin", "stdout", "stderr" }; #define CONFIG_SYS_DEVICE_NULLDEV 1 #endif +#ifdef CONFIG_SYS_STDIO_DEREGISTER +#define CONFIG_SYS_DEVICE_NULLDEV 1 +#endif #ifdef CONFIG_SYS_DEVICE_NULLDEV void nulldev_putc(struct stdio_dev *dev, const char c) @@ -172,7 +175,7 @@ int stdio_register(struct stdio_dev *dev) * returns 0 if success, -1 if device is assigned and 1 if devname not found */ #ifdef CONFIG_SYS_STDIO_DEREGISTER -int stdio_deregister_dev(struct stdio_dev *dev) +int stdio_deregister_dev(struct stdio_dev *dev, int force) { int l; struct list_head *pos; @@ -181,6 +184,10 @@ int stdio_deregister_dev(struct stdio_dev *dev) /* get stdio devices (ListRemoveItem changes the dev list) */ for (l=0 ; l< MAX_FILES; l++) { if (stdio_devices[l] == dev) { + if (force) { + strcpy(temp_names[l], "nulldev"); + continue; + } /* Device is assigned -> report error */ return -1; } @@ -202,7 +209,7 @@ int stdio_deregister_dev(struct stdio_dev *dev) return 0; } -int stdio_deregister(const char *devname) +int stdio_deregister(const char *devname, int force) { struct stdio_dev *dev; @@ -211,7 +218,7 @@ int stdio_deregister(const char *devname) if (!dev) /* device not found */ return -ENODEV; - return stdio_deregister_dev(dev); + return stdio_deregister_dev(dev, force); } #endif /* CONFIG_SYS_STDIO_DEREGISTER */ diff --git a/common/usb_kbd.c b/common/usb_kbd.c index d4d5f48..dcb693d 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -550,7 +550,7 @@ int drv_usb_kbd_init(void) int usb_kbd_deregister(void) { #ifdef CONFIG_SYS_STDIO_DEREGISTER - int ret = stdio_deregister(DEVNAME); + int ret = stdio_deregister(DEVNAME, 0); if (ret && ret != -ENODEV) return ret; diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c index d04104e..61cbdc6 100644 --- a/drivers/serial/serial-uclass.c +++ b/drivers/serial/serial-uclass.c @@ -197,7 +197,7 @@ static int serial_pre_remove(struct udevice *dev) #ifdef CONFIG_SYS_STDIO_DEREGISTER struct serial_dev_priv *upriv = dev->uclass_priv; - if (stdio_deregister_dev(upriv->sdev)) + if (stdio_deregister_dev(upriv->sdev), 0) return -EPERM; #endif diff --git a/include/stdio_dev.h b/include/stdio_dev.h index 268de8e..24da23f 100644 --- a/include/stdio_dev.h +++ b/include/stdio_dev.h @@ -103,8 +103,8 @@ int stdio_init(void); void stdio_print_current_devices(void); #ifdef CONFIG_SYS_STDIO_DEREGISTER -int stdio_deregister(const char *devname); -int stdio_deregister_dev(struct stdio_dev *dev); +int stdio_deregister(const char *devname, int force); +int stdio_deregister_dev(struct stdio_dev *dev, int force); #endif struct list_head* stdio_get_list(void); struct stdio_dev* stdio_get_by_name(const char* name);
In some cases we really want to move forward with a deregister, add a force parameter to allow this, and replace the dev with a nulldev in this case. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- common/stdio.c | 13 ++++++++++--- common/usb_kbd.c | 2 +- drivers/serial/serial-uclass.c | 2 +- include/stdio_dev.h | 4 ++-- 4 files changed, 14 insertions(+), 7 deletions(-)