On Sat, Oct 10, 2026 at 10:51:08AM +0000, Vencislav Atanasov wrote: > 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.
No worries.
> > [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)).
OK, thanks.
> So there were no spacing issues according to checkpatch.pl.
Yes, it's not really a good tool for that. Changes should be consistent
with the rest of a given file.
> 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.
Thanks.
> > [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.
OK. They should be able to be upstreamed in some manner, at this point
in time.
> > [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).
Thanks.
> > [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)?
I suspect so, yes.
> > [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.
OK. And yes, please remove stuff like that in general.
> > [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.
Yes, there's a lot of places where the code can be cleaned up,
especially for these older cores, now that we've moved to Kconfig,
really. There generally speaking shouldn't be a need to add a dummy
function today, it just means some symbol should be off, or needs to be
able to be turned off (as part of migrating to Kconfig we did a lot of
"everyone seems to set CONFIG_X, so keep setting it, without seeing if
we could just turn off CONFIG_X and then drop some dummy functions).
> > [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.
OK.
> > --
> > 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.
It should be a new iteration of the patch. And really, if you can split
things up more (say, a patch per generation) that might also make review
easier. We do have Gitlab, but email is how code is reviewed and
eventually picked up, rather than merge requests.
> 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.
It should be fine to keep working as you work. As documented at
https://docs.u-boot-project.org/en/latest/develop/ci_testing.html it is
possible to get a CI run done, when you're ready to try yourself, and
that can be helpful.
--
Tom
signature.asc
Description: PGP signature
