Re: [RFC PATCH 2/2] board: ti: am65x: Move to using Extension framework
On Wed, 4 Oct 2023 15:39:23 +0300 Roger Quadros wrote: Hello, Thanks for adding me in cc. Also it seems I forgot to add myself to MAINTAINERS for the extension_board.c file. > >>> Before this goes too far I think this should move to using a linker > >>> list to declare the driver (or a driver-model driver if you prefer, > >>> but that might be overkill). > > So I've been working on this on the side and got linker list way working > with custom script booting but as soon as I move to standard boot flow > it no longer works. This is because there is no code in place to > apply the overlay and pass it to next stage e.g. EFI. > > I see the following note at > https://elixir.bootlin.com/u-boot/latest/source/boot/bootmeth_efi.c#L304 > > " > /* > * TODO: Apply extension overlay > * > * Here we need to load and apply the extension overlay. This > is > * not implemented. See do_extension_apply(). The extension > * stuff needs an implementation in boot/extension.c so it is > * separate from the command code. Really the extension stuff > * should use the device tree and a uclass / driver interface > * rather than implementing its own list > */ > " I agreed that the extension implementation should move in boot/extension.c or common for general use. I am wondering what the advantage of creating an uclass interface? I am not an uclass expert but there is no per driver ops and usage. What do you dislike about using its own list? > Another note at > https://elixir.bootlin.com/u-boot/latest/source/cmd/extension_board.c#L198 > > "/* extensions should have a uclass - for now we use UCLASS_SIMPLE_BUS uclass > */" > > So are we better off implementing a class driver for extension stuff? I think the first point should be to move it in common or boot and makes it generic for using the extension function everywhere. I will let Simon answer about the uclass part. Regards,
Re: [PATCH 1/1] cmd: undefined return value of do_extension_apply()
Hello Heinrich,
On Mon, 11 Jul 2022 20:01:12 +0200
Heinrich Schuchardt wrote:
> If 'extension apply all' is executed and no extension is found, the return
> value of do_extension_apply() is undefined. Return CMD_RET_FAILURE in this
> case.
>
> Fixes: 2f84e9cf06d3 ("cmd: add support for a new "extension" command")
> Signed-off-by: Heinrich Schuchardt
Reviewed-by: Kory Maincent
> ---
> cmd/extension_board.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/cmd/extension_board.c b/cmd/extension_board.c
> index bbb4812ff8..f94abd612d 100644
> --- a/cmd/extension_board.c
> +++ b/cmd/extension_board.c
> @@ -111,6 +111,7 @@ static int do_extension_apply(struct cmd_tbl *cmdtp, int
> flag, return CMD_RET_USAGE;
>
> if (strcmp(argv[1], "all") == 0) {
> + ret = CMD_RET_FAILURE;
> list_for_each_entry(extension, _list, list) {
> ret = extension_apply(extension);
> if (ret != CMD_RET_SUCCESS)
Thanks,
Köry
Re: [PATCH] usb: kbd: allow probing even if usbkbd not in stdin
On Mon, 27 Jun 2022 13:39:03 +0200 Marek Vasut wrote: > >> Can you document the usecase in a bit more detail ? > > > > My usecase is to get a key press from the USB keyboard and do some thing > > about it in board_init. I do not want to multiplex the keyboard input to > > stdin but I still want to read character from it. > > All right, understood, both. Thanks for clarification. > > I applied this to usb/next , I hope that's OK ? Great! Thanks! Köry
Re: [PATCH] usb: kbd: allow probing even if usbkbd not in stdin
Hello Marek,
On Fri, 24 Jun 2022 20:12:31 +0200
Marek Vasut wrote:
> On 6/22/22 10:59, kory.mainc...@bootlin.com wrote:
> > From: Kory Maincent
> >
> > For now the driver does not probe if usbkbd was not present in stdin.
> > This presents two issues, we can not probe the driver before setting stdin
>
> Environment should be up and running before USB, so this is likely not a
> problem.
Having a driver probing only in this specific state is weird?
It should probe whatever the state of stdin.
>
> > and we can not use this driver in other manner than stdin console.
> >
> > This patch fixes this by adding an else statement. It simply probes the
> > driver without console management in the case "usbkbd" is not in stdin.
>
> Can you document the usecase in a bit more detail ?
My usecase is to get a key press from the USB keyboard and do some thing about
it in board_init. I do not want to multiplex the keyboard input to stdin but I
still want to read character from it.
> What exactly is the problem you are solving here ?
> > stdinname = env_get("stdin");
> > #if CONFIG_IS_ENABLED(CONSOLE_MUX)
> > - error = iomux_doenv(stdin, stdinname);
> > - if (error)
> > - return error;
> > + if (strstr(stdinname, DEVNAME) != NULL) {
> > + error = iomux_doenv(stdin, stdinname);
> > + if (error)
> > + return error;
> > + }
> > #else
> > /* Check if this is the standard input device. */
> > - if (strcmp(stdinname, DEVNAME))
> > - return 1;
> > -
> > - /* Reassign the console */
> > - if (overwrite_console())
> > - return 1;
> > + if (!strcmp(stdinname, DEVNAME)) {
>
> Maybe use strcmp() == NULL to be consistent with the != NULL check above ?
Yes, you are right.
Regards,
Köry
Re: [PATCH v5 08/10] configs: CHIP: add support for DIP detect functionality
Hello Andre, On Thu, 13 May 2021 12:03:05 +0100 Andre Przywara wrote: > On Tue, 4 May 2021 19:31:28 +0200 > Kory Maincent wrote: > > > This commit enables using the extension board detection mechanism on > > CHIP boards > > > > Signed-off-by: Kory Maincent > > Acked-by: Maxime Ripard > > Acked-by: Andre Przywara > > Cheers, > Andre > > P.S. Out of curiosity: what is the story for the CHIP Pro here? Does it > not work there or is it just not tested? The CHIP PRO design is not compatible with the DIPs in the market. I do not have heard about DIPs for CHIP PRO pinout, maybe Maxime has more information about it. Regards, Köry
Re: [PATCH v3 07/10] arm: sunxi: add support for DIP detection to CHIP board
On Thu, 15 Apr 2021 11:32:48 +0200 Maxime Ripard wrote: > Hi, > > On Thu, Apr 15, 2021 at 10:38:43AM +0200, Köry Maincent wrote: > > > > arch/arm/mach-sunxi/Kconfig | 9 > > > > board/sunxi/Makefile| 1 + > > > > board/sunxi/chip.c | 104 > > > > 3 files changed, 114 insertions(+) > > > > create mode 100644 board/sunxi/chip.c > > > > > > > > diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig > > > > index 49ef217f08..2b6ba873ff 100644 > > > > --- a/arch/arm/mach-sunxi/Kconfig > > > > +++ b/arch/arm/mach-sunxi/Kconfig > > > > @@ -1017,3 +1017,12 @@ config PINEPHONE_DT_SELECTION > > > > correct PinePhone hardware revision during boot. > > > > > > > > endif > > > > + > > > > +config CHIP_DIP_SCAN > > > > + bool "Enable DIPs detection for CHIP board" > > > > + select SUPPORT_EXTENSION_SCAN > > > > + select W1 > > > > + select W1_GPIO > > > > + select W1_EEPROM > > > > + select W1_EEPROM_DS24XXX > > > > + imply CMD_EXTENSION > > > > diff --git a/board/sunxi/Makefile b/board/sunxi/Makefile > > > > index c4e13f8c38..d96b7897b6 100644 > > > > --- a/board/sunxi/Makefile > > > > +++ b/board/sunxi/Makefile > > > > @@ -11,3 +11,4 @@ obj-$(CONFIG_SUN7I_GMAC) += gmac.o > > > > obj-$(CONFIG_MACH_SUN4I) += dram_sun4i_auto.o > > > > obj-$(CONFIG_MACH_SUN5I) += dram_sun5i_auto.o > > > > obj-$(CONFIG_MACH_SUN7I) += dram_sun5i_auto.o > > > > +obj-$(CONFIG_CHIP_DIP_SCAN)+= chip.o > > > > diff --git a/board/sunxi/chip.c b/board/sunxi/chip.c > > > > new file mode 100644 > > > > index 00..9be2ede57b > > > > --- /dev/null > > > > +++ b/board/sunxi/chip.c > > > > @@ -0,0 +1,104 @@ > > > > +// SPDX-License-Identifier: GPL-2.0+ > > > > +/* > > > > + * (C) Copyright 2021 > > > > + * Köry Maincent, Bootlin, > > > > + * Based on initial code from Maxime Ripard > > > > + */ > > > > + > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > + > > > > +#include > > > > + > > > > +#include > > > > + > > > > +#ifdef CONFIG_CMD_EXTENSION > > > > > > You depend on CMD_EXTENSION already, so that code won't even be compiled > > > if that configuration option is false > > > > It is wrong, the build of this code depend on CHIP_DIP_SCAN and this config > > *imply* CMD_EXTENSION. > > Indeed :) > > > Therefore it can be build even if CMD_EXTENSION is false. > > It's not really clear to me what would be the point though? Nothing in > that file will be compiled if CMD_EXTENSION, so it looks like (from a > functional point of view) CMD_EXTENSION doesn't bring anything over > SUPPORT_EXTENSION_SCAN > > Why do we need both? My point was to separate this part of code in chip.c in case of future additional code in this file. Your right, from a functional point it may be better to replace the imply by a select and remove "#ifdef CONFIG_CMD_EXTENSION". Regards, Köry
Re: [PATCH v3 07/10] arm: sunxi: add support for DIP detection to CHIP board
Hello Maxime, Thanks for your review. On Wed, 24 Mar 2021 15:32:59 +0100 Maxime Ripard wrote: > > arch/arm/mach-sunxi/Kconfig | 9 > > board/sunxi/Makefile| 1 + > > board/sunxi/chip.c | 104 > > 3 files changed, 114 insertions(+) > > create mode 100644 board/sunxi/chip.c > > > > diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig > > index 49ef217f08..2b6ba873ff 100644 > > --- a/arch/arm/mach-sunxi/Kconfig > > +++ b/arch/arm/mach-sunxi/Kconfig > > @@ -1017,3 +1017,12 @@ config PINEPHONE_DT_SELECTION > > correct PinePhone hardware revision during boot. > > > > endif > > + > > +config CHIP_DIP_SCAN > > + bool "Enable DIPs detection for CHIP board" > > + select SUPPORT_EXTENSION_SCAN > > + select W1 > > + select W1_GPIO > > + select W1_EEPROM > > + select W1_EEPROM_DS24XXX > > + imply CMD_EXTENSION > > diff --git a/board/sunxi/Makefile b/board/sunxi/Makefile > > index c4e13f8c38..d96b7897b6 100644 > > --- a/board/sunxi/Makefile > > +++ b/board/sunxi/Makefile > > @@ -11,3 +11,4 @@ obj-$(CONFIG_SUN7I_GMAC) += gmac.o > > obj-$(CONFIG_MACH_SUN4I) += dram_sun4i_auto.o > > obj-$(CONFIG_MACH_SUN5I) += dram_sun5i_auto.o > > obj-$(CONFIG_MACH_SUN7I) += dram_sun5i_auto.o > > +obj-$(CONFIG_CHIP_DIP_SCAN)+= chip.o > > diff --git a/board/sunxi/chip.c b/board/sunxi/chip.c > > new file mode 100644 > > index 00..9be2ede57b > > --- /dev/null > > +++ b/board/sunxi/chip.c > > @@ -0,0 +1,104 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * (C) Copyright 2021 > > + * Köry Maincent, Bootlin, > > + * Based on initial code from Maxime Ripard > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include > > + > > +#include > > + > > +#ifdef CONFIG_CMD_EXTENSION > > You depend on CMD_EXTENSION already, so that code won't even be compiled > if that configuration option is false It is wrong, the build of this code depend on CHIP_DIP_SCAN and this config *imply* CMD_EXTENSION. Therefore it can be build even if CMD_EXTENSION is false. Regards, Köry
Re: [PATCH v2 10/12] sun5i: add support for DIP detection to CHIP board
Hello Maxime, Thanks for your review. On Tue, 2 Mar 2021 17:05:38 +0100 Maxime Ripard wrote: > The split of that patch and the previous one is weird: you're adding a > Kconfig symbol doing close to nothing in patch 9, and you add the logic > behind it here > > It would be more natural to have the Kconfig option and the code here, > and Ok I will merge them into one commit. > > > diff --git a/board/sunxi/chip.c b/board/sunxi/chip.c > > new file mode 100644 > > index 00..fc3d14e497 > > --- /dev/null > > +++ b/board/sunxi/chip.c > > @@ -0,0 +1,104 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * (C) Copyright 2021 > > + * Köry Maincent, Bootlin, > > + * Based on initial code from Maxime Ripard > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include > > + > > +#include > > + > > +#ifdef CONFIG_CMD_EXTENSION > > + > > +#define for_each_1w_device(b, d) \ > > + for (device_find_first_child(b, d); *d; > > device_find_next_child(d)) > > The name is inconsistent with the rest of the 1-wire acronyms in the > file (1w vs w1) Yes, I didn't notice this. > > Also, reusing macro arguments is fairly dangerous but since it's used > only once we don't really need that macro? It was to increase the readability of the loop, but if you judge it not pertinent I can remove it.
Re: [PATCH 08/10] sun5i: add support for DIP detection to CHIP board
Hello Andre,
On Tue, 23 Feb 2021 13:30:38 +
Andre Przywara wrote:
y extension_board_scan board specific function, would you prefer if I
> > move to callback like below instead of Kconfig?
> >
> > if (of_machine_is_compatible("nextthing,chip"))
> > extension_board_register_callback(chip_extension_board_scan);
>
> Well, the CHIP Pro uses a different compatible string, so you would
> need to check for that too.
> I am not fully decided if checking for the machine compatible is the
> right approach. The more traditional U-Boot way would be to define a
> config symbol (as Tom already pointed out), that would also keep the
> code out of other board builds.
> We do this already with CONFIG_PINE64_DT_SELECTION and
> CONFIG_PINEPHONE_DT_SELECTION.
Okay, I will move on to the traditional config symbol.
Thanks.
Köry
Re: [PATCH 08/10] sun5i: add support for DIP detection to CHIP board
Hi Tom, Andre
Thanks for your reviews.
On Fri, 19 Feb 2021 17:29:26 +
Andre Przywara wrote:
> >
> > And then based on my comment in the previous patch, similar to
> > PINEPHONE_DT_SELECTION and other board-specific options, add a
> > CHIP_DIP_SCAN option or similar.
>
> Yes, indeed, thanks Tom.
>
> The idea of making this extension scheme generic is great, it's just
> that sunxi is using a different approach to board specific code here.
> Normally we expect U-Boot-proper board specific code to be DT driven,
> and where this does not really work, use those symbols that Tom pointed
> to.
For my extension_board_scan board specific function, would you prefer if I move
to callback like below instead of Kconfig?
if (of_machine_is_compatible("nextthing,chip"))
extension_board_register_callback(chip_extension_board_scan);
Regards,
Köry
Re: [PATCH v4 10/16] dm: gpio: Add a way to update flags
On Thu, 4 Feb 2021 21:22:03 -0700 Simon Glass wrote: > It is convenient to be able to adjust some of the flags for a GPIO while > leaving others alone. Add a function for this. > > Update dm_gpio_set_dir_flags() to make use of this. > > Also update dm_gpio_set_value() to use this also, since this allows the > open-drain / open-source features to be implemented directly in the > driver, rather than using the uclass workaround. > > Update the sandbox tests accordingly. This involves a lot of changes to > dm_test_gpio_opendrain_opensource() since we no-longer have the direciion > being reported differently depending on the open drain/open source flags. > > Also update the STM32 drivers to let the uclass handle the active low/high > logic. > > Drop the GPIOD_FLAGS_OUTPUT() macro which is no-longer used. > > Signed-off-by: Simon Glass > --- > > Changes in v4: > - Update dm_gpio_set_dir_flags() to clear direction flags first > > Changes in v3: > - Incorporate GPIOD_FLAGS_OUTPUT() changes from Patrick Delaunay > > drivers/gpio/gpio-uclass.c | 75 ++ > drivers/gpio/stm32_gpio.c | 3 +- > drivers/pinctrl/pinctrl-stmfx.c | 5 +- > include/asm-generic/gpio.h | 31 ++-- > test/dm/gpio.c | 132 > 5 files changed, 203 insertions(+), 43 deletions(-) > Tested-by: Kory Maincent
Re: [PATCH] gpio-uclass: fix gpio flags save condition
Hi Simon, On Thu, 4 Feb 2021 19:45:22 -0700 Simon Glass wrote: > Thank you for that. I think that is because dm_gpio_set_dir_flags() > does not clear the flags first. But we now do actually update > desc->flags, where as before it was just supposed to happen. So I > think this function needs to update to clear the direction flags, then > add in the ones passed. > > I have pushed this to u-boot-dm/gpio-working if you can test it again. I have tested your v4 series of patches and all works well for my onewire on gpio. Thanks for your update. Regards, Köry
Re: [PATCH v3] sysboot: add zboot support to boot x86 Linux kernel image
Hi Simon, Thanks for the review. On Mon, 1 Feb 2021 13:44:46 -0700 Simon Glass wrote: > Hi Kory, > > On Mon, 1 Feb 2021 at 08:31, Kory Maincent wrote: > > > > Add "zboot" command to the list of supported boot in the label_boot > > function. > > > > Signed-off-by: Kory Maincent > > --- > > > > Change since v1: > > - Modify comment > > > > Change since v2: > > - Update do_zboot to do_zboot_parent function to follow the patch: > >5588e776b0 > > > > cmd/pxe_utils.c | 4 > > include/command.h | 3 +++ > > 2 files changed, 7 insertions(+) > > > > diff --git a/cmd/pxe_utils.c b/cmd/pxe_utils.c > > index 3526a651d7..06611262c1 100644 > > --- a/cmd/pxe_utils.c > > +++ b/cmd/pxe_utils.c > > @@ -657,6 +657,10 @@ static int label_boot(struct cmd_tbl *cmdtp, struct > > pxe_label *label) /* Try booting a Image */ > > else > > do_bootz(cmdtp, 0, bootm_argc, bootm_argv); > > +#elif defined(CONFIG_CMD_ZBOOT) > > Can this use IS_ENABLED() ? Yes it can use IS_ENABLED indeed. > > > + /* Try booting an x86_64 Linux kernel image */ > > + else > > + do_zboot_parent(cmdtp, 0, bootm_argc, bootm_argv, NULL); > > #endif > > unmap_sysmem(buf); > > > > diff --git a/include/command.h b/include/command.h > > index e229bf2825..cb91ba81b5 100644 > > --- a/include/command.h > > +++ b/include/command.h > > @@ -165,6 +165,9 @@ extern int do_bootz(struct cmd_tbl *cmdtp, int flag, > > int argc, extern int do_booti(struct cmd_tbl *cmdtp, int flag, int argc, > > char *const argv[]); > > > > +extern int do_zboot_parent(struct cmd_tbl *cmdtp, int flag, int argc, > > + char *const argv[], int *repeatable); > > We don't normally use extern for function decls in header files. Ok, why the other boot commands (do_booti, do_bootz) use extern? Regards,
Re: [PATCH] gpio-uclass: fix gpio flags save condition
Hello Simon, On Sun, 24 Jan 2021 13:24:14 -0700 Simon Glass wrote: > Hi Kory, > > On Fri, 22 Jan 2021 at 08:23, Kory Maincent wrote: > > > > The commit cd2faeba1a moves the location where we save the flags to the gpio > > descriptor but it adds a "!" character. > > This breaks the condition we expect to save the flags. > > > > Signed-off-by: Kory Maincent > > --- > > drivers/gpio/gpio-uclass.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > This does indeed seem to be a bug, and the tests do not cover it. But > this happens to be fixed by my GPIO series. I'm about to send out v3 > so will copy you on it. Thanks. I have tested your series of patch and it seems to have also an issue with my use case. I am working with w1-gpio. I got this issue: => w1 bus check_dir_flags: flags 0x16 has GPIOD_IS_OUT and GPIOD_IS_IN gpio_sunxi PD: PD error: set_dir_flags for gpio PD2 has invalid dir flags 0x16 Bus 0: onewire (active) w1_eeprom@0 (0) uclass w1_eeprom : family 0x0 Regards, Köry
Run U-Boot as Legacy Bios payload?
Hello, Does U-Boot can be built and run as a Legacy BIOS payload, like GRUB or SeaBios? Indeed GRUB or SeaBios have a stage 1 binary to be merge in GPT/MBR partition table. I did not find something like that with U-Boot. I do not want to use U-Boot with UEFI, I have already done it. I saw U-boot can be used as Coreboot payload, but I can not find how to use with BIOS. Regards, Köry
