> From: Simon Glass <s...@chromium.org> > Date: Sat, 19 Feb 2022 15:21:19 -0700 > > Hi Mark, > > On Sat, 19 Feb 2022 at 13:37, Mark Kettenis <mark.kette...@xs4all.nl> wrote: > > > > > From: Simon Glass <s...@chromium.org> > > > Date: Fri, 18 Feb 2022 17:21:13 -0700 > > > > Hi Simon, > > > > > Hi Mark, > > > > > > On Mon, 7 Feb 2022 at 14:20, Mark Kettenis <mark.kette...@xs4all.nl> > > > wrote: > > > > > > > > > From: Simon Glass <s...@chromium.org> > > > > > Date: Mon, 7 Feb 2022 13:22:22 -0700 > > > > > > > > > > Hi Mark, > > > > > > > > > > On Sat, 5 Feb 2022 at 16:10, Mark Kettenis <kette...@openbsd.org> > > > > > wrote: > > > > > > > > > > > > The stdout-path property in the device tree does not necessarily > > > > > > point at a serial device. The code that binds the device if it > > > > > > isn't marked to be bound before relocation does not check whether > > > > > > the device really is a serial device. So check that its uclass is > > > > > > UCLASS_SERIAL before probing it. > > > > > > > > > > > > Signed-off-by: Mark Kettenis <kette...@openbsd.org> > > > > > > --- > > > > > > drivers/serial/serial-uclass.c | 3 ++- > > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > This would be better as an assert() to avoid code bloat. Under what > > > > > circumstances was this wrong? > > > > > > > > I'm passing a debive tree to U-Boot that has stdout-path set to > > > > "/chosen/framebuffer" to indicate that output should go to the > > > > framebuffer instead of the serial port. This is a node with a > > > > "simple-framebuffer" compatible, which means the list_bind_fdt() call > > > > binds the simple_video (simplefb.c) driver and stores a pointer to its > > > > struct udevice into devp. I've verified that the uclass is indeed > > > > UCLASS_VIDEO at that point with my device tree. > > > > > > > > Since the function returns 0, the caller of this function thinks we > > > > returned a valid UCLASS_SERIAL device and when u-boot starts using it > > > > as such it crashes (without printing anything since there is no > > > > console output device yet). > > > > > > > > So an assert won't work. We should simply return an error an run the > > > > fallback code below that looks for the serial port with the specified > > > > index. That way U-Boot continues to work as expected (with output on > > > > both the serial port and the framebuffer). But once my OS is running, > > > > it knows to pick the framebuffer console and all's fine. > > > > > > > > I haven't looked at the binary growth, but it can't be much. But if > > > > you think it is a problem, maybe we should make this code that binds > > > > the console driver optional? > > > > > > OK I see. It likely isn't much, but in SPL it is still growth. Worth > > > checking. > > > > before: > > > > $ size u-boot > > text data bss dec he > > 442903 27700 13448 484051 762d3 > > > > after: > > > > $ size u-boot > > text data bss dec hex > > 442925 27700 13448 484073 762e9 > > > > So that's 22 bytes. > > > > Thew growth in serial-uclass.o is a bit smaller; only 16 byes. > > > > > Can you perhaps create a 'console' alias so that the line above finds > > > the correct one? > > > > Probably. But none of the Linux device trees provide a 'console' > > alias so this seems to be a U-Boot convention. And it doesn't > > actually fix the bug; it just works around it. > > > > > But I'm a bit unsure of how you get the serial console to work if > > > serial_check_stdout() fails? Is the serial driver not marked for > > > pre-reloc use? There is something odd going on. > > > > The serial driver is marked for pre-reloc use. When > > serial_check_stdout() fails the code at the tail end of > > serial_find_console_or_panic() runs and finds the serial device at > > index 0 and uses that. > > > > The "something odd" that's going on here is that I set > > "/chosen/stdout-path" to point to the framebuffer instead of a serial > > port. But nowhere does the device tree specification say that > > "stdout-path" has to point at a serial device. So U-Boot shouldn't > > crash when this isn't the case. > > OK, I think this is the sort of info that should have been in your > commit message. > > However, looking at this, the code you are changing should not be > there now. It was added for backwards-compatibility reasons. > > Let's remove the lists_bind_fdt() call and rely on people doing the > right thing now.
Right. I'm worrying a bit that this will break many boards. I was thinking in particular about boards like the raspberry pi where we use the device tree provided by the firmware. But on that the serial drivers have the DM_FLAG_PRE_RELOC flag set when OF_BOARD is defined, so that shouldn't be a problem. But many other serial drivers don't set DM_FLAG_PRE_RELOC, or only set it when !OF_CONTROL. I checked a few of those and most of them don't seem to have the *-u-boot.dtsi files to add the necessary property to the nodes. I still think we should fix the bug regardless of whether we remove the code somewhere in the near future.