Hi Tom! Thanks for the review! Please find my answers below.
> On 7 Oct 2026, at 22:13, Tom Rini <[email protected]> wrote: > > On Mon, Oct 05, 2026 at 08:46:32AM +0000, Vencislav Atanasov wrote: > >> Port U-Boot to the Samsung S5L87xx SoC family, which is used by the >> Apple iPod nano (2nd to 7th generation), iPod classic (6th generation) >> and iPod touch (2nd generation) digital audio players. > > It's very nice to see this. For the next iteration, please reword this > so there's more information, and also the attributions, above the first > "---" so git doesn't omit it. Sorry, this is my first patch using the kernel/u-boot workflow, I was not really sure which part to include in the commit and which part in the description text. > > [snip] > > Globally, there are spacing issues where the changes don't match the > rest of the file, please fix those. Similarly, please run checkpatch.pl > and make sure there's not other fixes like that to do as well. I've ran checkpatch.pl (using b4 prep --check) before submitting and the output is: $ b4 prep --check Checking patches using: /Users/venci/Git/u-boot/scripts/checkpatch.pl -q --terse --no-summary --mailback --showfile --- ● 93571ce45b81: New platform: Samsung S5L87xx ● checkpatch.pl: arch/arm/include/asm/arch-s5l87xx/s5l87xx.h:47: CHECK: Macro argument reuse 'i' - possible side-effects? ● checkpatch.pl: arch/arm/include/asm/arch-s5l87xx/s5l87xx.h:67: CHECK: Macro argument reuse 'rx_pin' - possible side-effects? ● checkpatch.pl: arch/arm/include/asm/arch-s5l87xx/s5l87xx.h:67: CHECK: Macro argument reuse 'tx_pin' - possible side-effects? ● checkpatch.pl: arch/arm/include/asm/arch-s5l87xx/s5l87xx.h:69: CHECK: Macro argument reuse 'fn' - possible side-effects? ● checkpatch.pl: board/apple/nxx-common/nxx.c:15: WARNING: Use the livetree API (dev_read_...) ● checkpatch.pl: board/apple/nxx-common/nxx.c:20: WARNING: Use the livetree API (dev_read_...) ● checkpatch.pl: drivers/serial/serial_s5p.c:16: WARNING: Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible ● checkpatch.pl: drivers/serial/serial_s5p.c:65: WARNING: Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible ● checkpatch.pl: drivers/serial/serial_s5p.c:123: WARNING: Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible ● checkpatch.pl: drivers/serial/serial_s5p.c:140: WARNING: Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible --- Success: 0, Warning: 10, Error: 0 The first CHECK uses a u8 so no side effects, the next CHECKS use a compile time constant. About the livetree API, it looks like a false positive to me, as these fdtdec_* functions look like the standard way to init DRAM and its size. And the serial_s5p code essentially needs to not get compiled at all for this specific platform, so it uses #if IS_ENABLED(x) rathen than if (IS_ENABLED(x)). So there were no spacing issues according to checkpatch.pl. FWIW, the PIE changes in arm920t/start.S and arm1176/start.S were directly copy/pasted from armv7/start.S and they are needed because the previous stage bootloader cannot be configured to load u-boot from a specific address. I'll fix the spaces without relying on checkpatch.pl to point it out for me. > > [snip] > > For all of these dts files, have you submitted them upstream? No, as there isn't a working Linux port for all devices. There is a kernel booting on 2nd, 5th, 6th and 7th gen iPod nanos, but from different branches because individual developers worked on each of the ports. But since getting u-boot to run is the easier part, and a pre-requisite for Linux, I decided to wrap up and submit the u-boot port for review first. > > [snip] > > Can we figure out a way to share includes? It seems like this one for > example is cp'd a few times. Maybe arch-s5l8xxx ? I was relying on the arch/arm/include/asm/arch symlink that points to the corresponding arch-* folder at compile time. But since the files for all s5l87xx devices are essentially the same, I can modify the driver to just include a common on in case of IS_ENABLED(CONFIG_ARCH_S5L87XX). > > [snip] > > We should probably handle this as a "choice" statement I'll fix this. Does this also apply to arch/arm/mach-s5l87xx/s5l8702/Kconfig and eventually arch/arm/mach-s5l87xx/s5l8720/Kconfig, when it gets a second entry i.e. iPod touch (2nd generation)? > > [snip] >> +#obj-$(CONFIG_S5L8730) += s5l87xx-buscon.o > > Commented out lines should be removed unless there's a very good reason > to keep them (and in code, it should probably become some debug > statement that'll be optimized out normally). This code was an attempt to port arch/arm/mach-s5l87xx/relocate.S (Remap SRAM vectors) from ASM to C, but the vector copying needs to be done immediately after the remapping so I was not sure how to do it. I can leave just the ASM code for now and delete the C code. > > [snip] > > Empty functions like this should mean we just need to turn off a Kconfig > symbol instead. I've done it for the other CPU core types, but for ARM1176 there wasn't already a Kconfig symbol, and I saw that other platforms already do something similar e.g.: arch/arm/mach-mvebu/cpu.c:47: void lowlevel_init(void) { /* * Dummy implementation, we only need LOWLEVEL_INIT * on Armada to configure CP15 in start.S / cpu_init_cp15() */ } arch/arm/mach-hpe/gxp/reset.c:15: /* empty to satisfy current lowlevel_init, can be removed any time */ void lowlevel_init(void) { } If the goal is to clean up this pattern, I can modify arch/arm/cpu/arm1176/start.S to include #if !CONFIG_IS_ENABLED(SKIP_LOWLEVEL_INIT_ONLY), similar to the other ARM platforms. > > [snip] > > I can see this (and presumably the rest) weren't done via "make > savedefconfig", so please do that for v2. They weren't as I didn't know this is the way to generate a defconfig. I will redo them using savedefconfig in the next patch. > > -- > Tom Thanks again for taking your time to review the patch. Should I submit the corrections as a new patch which applies on top of this one, or as a second version of this patch? Also I saw you have a GitLab instance, are MRs accepted there, or the email flow is the most recommended one? I'm more used to web-based MR and review, but I've already set up my email and git/b4 tooling, so I can continue submitting the patches via email. Another thing to note is that I'm working on macOS so all building and testing is done on a Mac. I also have a couple of Linux machines, is it a requirement to test if the patches build on Linux before submitting? I'm using arm-none-eabi-gcc version 16.2.0 from Homebrew for the target and Apple clang version 21.0.0 (clang-2100.3.34.2) with target arm64-apple-darwin27.0.0 for the host tools. It seems to work well so far. Regards, Vencislav
