Re: [PATCH v1 3/4] ARM: boards: add support for Samsung Galaxy S8 (dreamlte)

[Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>]

On 7/30/25 12:33, Ahmad Fatoum wrote:
> Hi Ivaylo,
>
> On 7/30/25 11:09, Ivaylo Ivanov wrote:
>> On 7/30/25 11:31, Ahmad Fatoum wrote:
>>> Hello Ivaylo,
>>>
>>> Thanks for your patch.
>>>
>>> On 7/29/25 22:36, Ivaylo Ivanov wrote:
>>>> Phones utilizing an exynos SoC boot android with samsung's proprietary
>>>> bootloader, called s-boot (s-lk on newer devices). However, not only is
>>>> it closed source, it also enforces some limitations that prevent us from
>>>> booting mainline linux cleanly on them, such as an applied overlay device
>>>> tree, disabled framebuffer refreshing, misaligned kernel image at boot.
>>> misaligned kernel image might be a bit problematic for barebox too, but
>>> we can fix it for PBL if needed.
>>>
>>> What misalignment are we speaking of?
>> On exynos 8890 up to exynos990, the bootloader takes a random number via
>> hwrng in a specific range after 80000000. If I recall correctly, when I was booting
>> mainline linux without any shim on my galaxy S8 (it was 5 years ago), the kernel
>> was complaining about misaligment, but I'd need to recreate that to be able
>> to provide logs. Anyways, I've tested barebox on 4 exynos devices and so far
>> there haven't been any inconsistencies at boot.
> While barebox is relocatable, we link it as if it would be running from
> address 0. This has the side effect that adrp instructions are emitted
> that assume implicitly that barebox starts at a 4K-aligned address,
> because well, 0, is 4K-aligned.
>
> If this proves problematic, we can try to eliminate the generation of
> adrp instruction. Did you experience kernel/bootloader being loaded to
> an address that's not 4K-aligned?

Nope, I think it's always 4k-aligned at least. We've been booting mainline linux
using our own bootloader shim implementation (github.com/ivoszbg/uniLoader),
and this hasn't been an issue.

>
>>>> Therefore, having another stage bootloader, loaded as a linux kernel
>>>> image by s-boot, is best.
>>>>
>>>> Add support for Samsung Galaxy S8, utilizing the exynos 8895 SoC. Support
>>>> is modelled to be as reusable on other devices as possible, requiring
>>>> only a minimal set of changes to boot - a barebox device tree, which in
>>>> this case is basically imported torvalds tree for dreamlte, that is then
>>>> matched from the downstream device tree, provided by s-boot at x0.
>>>>
>>>> For some reason, on certain devices the stack set up by the previous
>>>> bootloader is not enough. Since the idea of this board support is to be
>>>> as generic as possible, setting a fixed stack top via
>>>> ENTRY_FUNCTION_WITHSTACK does not make sense, due to different exynos
>>>> devices having different memory layouts - exynos8895's dram starts at
>>>> 0x80000000, whereas exynos7870's starts at 0x40000000. Instead, set the
>>>> SP as early as possible in the entry C function by taking the memory base
>>>> from the downstream fdt + (SZ_8M - SZ_64K).
>>> naked functions are not support on ARM64, so setting a stack pointer in
>>> C code is always an unsafe operation that could mess up codegen.
>>>
>>> If you need to do this dynamically, this needs to be done in assembly.
>> hm, so I could just have a C entry that messes with fdt, and then jump to
>> the assembly function for SP relocation, after which we go back to C?
> Yes. See __barebox_arm_entry, which does this generically for all boards.
>
>>>> Currently, only a minimal set of features work. An image can be booted by
>>>> barebox by configuring barebox to jump to the address where ramdisk gets
>>>> loaded by s-boot, and packaging that payload as a ramdisk with mkbootimg.
>>> Nice. Would be good to have this info in Documentation/
>> I was gonna do that in a separate patch after the patchset gets merged.
> Great.
>
>>>> +	compat = fdt_getprop(fdt, node, "model", &len);
>>>> +	if (!compat)
>>>> +		return false;
>>> Why compare the model and not the compatible as the compat variable name
>>> would suggest?
>> Because Samsung are more consistent at coming up with model name
>> props than compatibles. For example:
>>
>> compatible = "samsung,X1S EUR OPEN 05\0samsung,EXYNOS990";
>> compatible = "samsung, DREAMLTE KOR rev01", "samsung,EXYNOS8895";
>> compatible = "samsung, J7VE LTE LTN OPEN 00", "samsung,exynos7870";
>>
>> there are inconsistencies in spacing, capitalizing etc etc. But yeah, I will
>> change the variable name to is_model.
> Sounds good. Can you add a comment that compatibles are inconsistent?
> maybe with an example.

Okay.

>
>>>> +static noinline void exynos_continue(void *downstream_fdt)
>>>> +{
>>>> +	void *fdt;
>>>> +	unsigned long membase, memsize;
>>>> +	char *__dtb_start;
>>>> +
>>>> +	/* select device tree dynamically */
>>>> +	if (is_compat(downstream_fdt, "Samsung DREAMLTE")) {
>>>> +		__dtb_start = __dtb_exynos8895_dreamlte_start;
>>>> +	} else {
>>>> +		/* we didn't match any device */
>>>> +		return;
>>>> +	}
>>>> +	fdt = __dtb_start + get_runtime_offset();
>>>> +	fdt_find_mem(fdt, &membase, &memsize);
>>> Ah, so you do need the FDT in uncompressed form..
>> Mhm. I tried taking mem from downstream DT, but it's so broken it
>> doesn't work.
> That's a shame.
>
>>>> +	/*
>>>> +	 * The previous bootloader has a stack set up, but it seems to not be
>>>> +	 * enough as we can't get past the relocation on some devices. Set up
>>>> +	 * a stack determined by the memory node from the downstream fdt.
>>>> +	 */
>>>> +	fdt_find_mem(downstream_fdt, &mem_base, &mem_size);
>>> It's not a good idea to call this here before having set up the C
>>> environment. This makes regressions after compile updates much more
>>> likely. Can't we instead grow down from the start of the barebox binary?
>>>
>>> That's what start_dt_2nd in board-dt-2nd-aarch64.S is doing.
>> iirc this is not my first attempt at barebox, I had some work a few months ago,
>> and the board-dt-2nd wasn't booting. I suppose it's the SP stuff that was borked.
>> I will test it again though and report back.
> Thanks. Two things come to mind that might be worth exploring:
>
> - increment the stack pointer: We are not going to return from the
> barebox entry point, so we do not care if we overwrite old stack frames.
> Just aligning the stack to the next 4K for example might already be
> enough that you could call relocate_to_current_adr(); setup_c(); right
> away without first parsing the device tree
>
> - enable CONFIG_PBL_FULLY_PIC: This is still a bit experimental, but it
> eliminates a lot of relocation entries that are avoidable. It should be
> the default eventually. If aligning the stack is no option and you need
> to parse the DT before calling relocate_to_current_adr(), PBL_FULLY_PIC
> should help with reducing the chance of problems...

I tested the board-dt-2nd-aarch64.S logic and it works. I'll add an
entry.S with it for v2, but stripped out of the efi header stuff.

>
>>>> diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile
>>>> index 6612a514..a53834f7 100644
>>>> --- a/arch/arm/dts/Makefile
>>>> +++ b/arch/arm/dts/Makefile
>>>> @@ -13,6 +13,7 @@ lwl-$(CONFIG_MACH_BEAGLEPLAY) += k3-am625-beagleplay.dtb.o k3-am625-r5-beaglepla
>>>>  lwl-$(CONFIG_MACH_CLEP7212) += ep7212-clep7212.dtb.o
>>>>  lwl-$(CONFIG_MACH_CM_FX6) += imx6dl-cm-fx6.dtb.o imx6q-cm-fx6.dtb.o imx6q-utilite.dtb.o
>>>>  lwl-$(CONFIG_MACH_DFI_FS700_M60) += imx6q-dfi-fs700-m60-6q.dtb.o imx6dl-dfi-fs700-m60-6s.dtb.o
>>>> +lwl-$(CONFIG_MACH_EXYNOS) += exynos8895-dreamlte.dtb.o
>>> It's on my todo list to add a way to specify device trees externally to
>>> make. Still need to figure out how it will look like though.
>> Yeah, I was thinking that it'd be nice to just compile it directly from
>> dts/src/.
> Ye, we'll have to think about how the X DTs that are built this way and
> bundled into barebox will be selected. The easy way would be just
> including them all into the existing barebox-2nd.fit and have the
> previous stage bootloader select the correct one, but in your case we
> would need custom logic somehow.
>
>>>> +/ {
>>>> +	barebox,disable-deep-probe;
>>> Why not enable it?
>> Honestly, I other devices doing that and didn't dig too deep into what
>> it's meant for. Will test.
> Deep probe means that barebox will recursively probe resources as needed
> instead of returning -EPROBE_DEFER. This can break old board code or
> buggy drivers, so it's disabled for a number of boards in-tree.
>
> I don't assume you'll run into issues with your new subarch here, so you
> can just enable it unconditionally.

I see. Thanks!

Best regards,
Ivaylo

>
> Cheers,
> Ahmad
>