On Wed, Jun 11, 2025 at 03:25:18PM +0200, Quentin Schulz wrote: > Hi all, > > On 6/11/25 2:08 PM, Sumit Garg wrote: > > On Tue, Jun 10, 2025 at 10:04:54AM -0600, Tom Rini wrote: > > > On Tue, Jun 10, 2025 at 02:22:49PM +0530, Sumit Garg wrote: > > > > On Mon, Jun 09, 2025 at 10:22:39AM -0600, Tom Rini wrote: > > > > > On Mon, Jun 09, 2025 at 05:07:40PM +0100, Sumit Garg wrote: > > > > > > On Mon, Jun 09, 2025 at 09:50:19AM -0600, Tom Rini wrote: > > > > > > > On Mon, Jun 09, 2025 at 04:40:43PM +0100, Sumit Garg wrote: > > > > > > > > On Mon, Jun 09, 2025 at 03:46:27PM +0200, Dario Binacchi wrote: > > > > > > > > > Hi Sumit, > > > > > > > > > > > > > > > > > > On Mon, Jun 9, 2025 at 3:25 PM Sumit Garg > > > > > > > > > <sumit.g...@kernel.org> wrote: > > > > > > > > > > > > > > > > > > > > Hi Patrice, > > > > > > > > > > > > > > > > > > > > On Mon, Jun 09, 2025 at 03:15:14PM +0200, Patrice CHOTARD > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 6/7/25 11:37, Dario Binacchi wrote: > > > > > > > > > > > > The series adds support for stm32h747-discovery board. > > > > > > > > > > > > > > > > > > > > > > > > Detailed information can be found at: > > > > > > > > > > > > https://www.st.com/en/evaluation-tools/stm32h747i-disco.html > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Dario Binacchi (9): > > > > > > > > > > > > ARM: dts: stm32h7-pinctrl: add _a suffix to > > > > > > > > > > > > u[s]art_pins phandles > > > > > > > > > > > > dt-bindings: arm: stm32: add compatible for > > > > > > > > > > > > stm32h747i-disco board > > > > > > > > > > > > dt-bindings: clock: stm32h7: rename USART{7,8}_CK to > > > > > > > > > > > > UART{7,8}_CK > > > > > > > > > > > > ARM: dts: stm32: add uart8 node for stm32h743 MCU > > > > > > > > > > > > ARM: dts: stm32: add pin map for UART8 controller on > > > > > > > > > > > > stm32h743 > > > > > > > > > > > > ARM: dts: stm32: add an extra pin map for USART1 on > > > > > > > > > > > > stm32h743 > > > > > > > > > > > > ARM: dts: stm32: support STM32h747i-disco board > > > > > > > > > > > > ARM: dts: stm32: add stm32h747i-disco-u-boot DTS file > > > > > > > > > > > > board: stm32: add stm32h747-discovery board support > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Dario > > > > > > > > > > > > > > > > > > > > > > For the whole series > > > > > > > > > > > Applied to u-boot-stm32/next > > > > > > > > > > > > > > > > > > > > Please give some time for other maintainers to review this > > > > > > > > > > patch-set. > > > > > > > > > > The dts/upstream patches in this series aren't clean cherry > > > > > > > > > > pick from > > > > > > > > > > upstream. > > > > > > > > > > > > > > > > > > All the commits are already in the mainline Linux kernel, > > > > > > > > > specifically > > > > > > > > > in v6.16-rc1. > > > > > > > > > If you're referring to the fact that the patches can't be > > > > > > > > > applied > > > > > > > > > cleanly, I believe it's > > > > > > > > > because the target path in the Linux kernel doesn't match the > > > > > > > > > one in U-Boot. > > > > > > > > > In fact, the DTS files are located in two different relative > > > > > > > > > paths. > > > > > > > > > > > > > > > > That's exactly why we have (refer here [1]): > > > > > > > > > > > > > > > > ./tools/update-subtree.sh pick dts <commit-id-to-be-picked> > > > > > > > > > > > > > > > > You should have waited v6.16-rc1 tag to be synced into > > > > > > > > devicetree-rebasing [2] for the cherry-picks to work. This way > > > > > > > > of > > > > > > > > manually patching dts/upstream is not allowed since it is going > > > > > > > > to break > > > > > > > > DT syncs in one way or another. > > > > > > > > > > > > > > > > So I would suggest you to wait for v6.16-rc1 to land in DT > > > > > > > > rebasing tree > > > > > > > > and then send v2 with proper cherry picked patches. > > > > > > > > > > > > > > > > [1] > > > > > > > > https://docs.u-boot.org/en/latest/develop/devicetree/control.html#resyncing-with-devicetree-rebasing > > > > > > > > [2] > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/devicetree/devicetree-rebasing.git > > > > > > > > > > > > > > To be honest, I don't think this is a big deal. Git will be > > > > > > > merging > > > > > > > based on content and not specific hashes. And in the case of > > > > > > > conflicts I > > > > > > > just copy the file from the tag to our tree. > > > > > > > > > > > > The essential problem here to me is we are going to allow manual > > > > > > patching of dts/upstream tree given this example? How do we keep > > > > > > track > > > > > > if all that manual patching landed in Linux DT mainline? The cherry > > > > > > picks ensured that we always keep in sync with mainline. > > > > > > > > > > > > Lets take an example what if Git automatically resolved a merge > > > > > > conflict > > > > > > for you with duplicated content or if manually patching a DTS file > > > > > > diverged from upstream and get unnoticed during DT syncs? > > > > > > > > > > > > IMHO, we should try to avoid manual patching of DT subtree > > > > > > otherwise it > > > > > > is hard to set a policy as to what level of manual patching is > > > > > > allowed > > > > > > or not. > > > > > > > > > > Part of the problem here is that from the standpoint of applying > > > > > posted > > > > > patches there's no functional difference between what Dario did here > > > > > and > > > > > what could be done once v6.16-rc1-dts is tagged (if it's not already). > > > > > It's essentially a "manual patch" either way. > > > > > > > > Nope, there is a difference here. The cherry-pick from DT rebasing > > > > allows the custodian to rather just cherry pick corresponding DT patches > > > > rather than applying patches posted on mailing list. I usually do that > > > > when reviewing dts/upstream patches if they can be cherry-picked cleanly > > > > or not. So there won't be manual patching in that process. > > > > > > > > ./tools/update-subtree.sh pick dts <commit-id-to-be-picked> > > > > > > Alright. I hadn't foreseen anyone doing that rather than "b4 {am,shazam} > > > msg-id" to grab the series. > > > > Maybe we need to have some custodian specific docs listing best > > practices. > > > > Or extend > > https://docs.u-boot.org/en/latest/develop/devicetree/control.html#where-do-i-get-a-devicetree-file-for-my-board > https://docs.u-boot.org/en/latest/develop/devicetree/control.html#resyncing-with-devicetree-rebasing > > ? > > > > > > > > > We make it clear that > > > > > dts/upstream/ *only* gets changes that are in Linus' tree. If someone > > > > > tries to be sneaky and push something in that's not quite what's > > > > > upstream, it will get stomped on later and there's not going to be any > > > > > sympathy for the now broken platform. > > > > > > > > For us the upstream sync path is via DT rebasing tree only. It usually > > > > lags behind Linus' tree by maximum 1 week candence what I have noticed. > > > > > > > > > > > > > > Yes, we document saying to use the cherry-pick script, and that's what > > > > > people should do in general. But I don't think there's value in > > > > > adding a > > > > > further delay between "in Linus' tree" and "in devicetree-rebasing". > > > > > In > > > > > the linux kernel, there's thousands of people working on things and so > > > > > strict rules can be understandable (someone will be running a bot to > > > > > look for "(cherry pick from commit $hash)" and fail things where $hash > > > > > doesn't exist, makes sense). Here if the ST custodians are happy just > > > > > verifying the kernel commit, OK, that's fine. Or if they want to wait, > > > > > that's fine too. We can be a little relaxed and let custodians do what > > > > > they see as best. > > > > > > > > The reason we adopted OF_UPSTREAM was just to get rid of the manual DT > > > > patching and the syncs. So is it really that few days lag of DT rebasing > > > > tree which is again pushing us towards manual DT patching? I am just > > > > trying to understand the shortcomings that DT rebasing tree puts in > > > > front of us. > > > > > > It's mainly that I want to be flexible. So long as we don't violate the > > > content rules (Linus' tree *only*) I don't want to hinder the people > > > eager to now upstream U-Boot support for purely process reasons > > > > This flexibility has a cost associated to it which I hopefully was able > > to clarify above. But finally it's your decision which prevails. > > > > BTW, DT rebasing has already got the v6.16-rc1 tag, so it was really > > just 3 days gap. Quentin (CCed) has already done some proper cherry > > picking from there [1]. > > > > Not sure why I was summoned here but I can give my (maybe undesired) 2 cents > :) > > I (a contributor) **really** do not want to have hand-crafted patches in > dts/upstream. Use tools/update-subtree.sh for patches in that tree. Only Tom > is allowed to use pull because it's a mess to send a patch for it and he > usually announces he's doing it and then pushes to the next branch directly, > contributors can use pick instead. If a pick fails, backport everything > needed for it to apply cleanly. Yes, this may be tedious. Make sure you do > not break any other board as well. > > I was already surprised we have U-Boot specific files in dts/upstream (e.g. > Makefiles :) ). > > 1) Doing it manually doesn't enforce the addition of the commit hash used > for the backport/cherry-pick in the commit log, which may be problematic > (how to quickly check that the patch contains what it should contain and not > more, or less?). How to verify it actually was merged? In that state and not > after other changes? Let's imagine an upstream patch that changes the > SoC.dtsi and associated boards dts, if backporting manually one could be > tempted to skip a conflicting board dts (because another commit needs to be > picked before for it to apply cleanly). Also, until it's merged in master > (which can take a long time depending where you are in the release cycle) > there's no guarantee the patch is actually going to make it as is to the > tree (it isn't unheard of to have reverts or even rebases in maintainer > trees before sending a merge request to Linus). > > 2) If there are manual changes made to the patch that aren't upstreamed in > the kernel (or dependencies (git context) are missing), if we diverge too > much from upstream, Tom will have git conflicts when pulling new versions. > How to resolve those conflicts will be interesting. > > 3) Worse, if there are non-upstreamed changes that aren't inducing any git > conflict (via git context for example), then the merge/pull may not say > anything about it and we'll carry non-upstream patches in dts/upstream. > > Maybe we should not even do a subtree pull/merge but erase everything and > reimport everything every time we bump, to make sure 2 or 3) cannot happen? > > For what it's worth, I've caught some (Rockchip) contributors sending > patches to dts/upstream that weren't even sent to the kernel mailing list. > That wasn't done on purpose but there are probably a few patches already > that went through the cracks. I am not sure how we could enforce that (which > needs to be done with maintainer tools as there are already too many ways to > contribute to U-boot (patman, git-send-email, b4, etc...)) or even if we > want to. But making dts/upstream the new arch/arm/dts/ (for ARM) directory > doesn't make much sense to me. > > If you cannot wait for devicetree-rebasing to receive the new tag, do the > changes in -u-boot.dtsi and revert the changes once we update dts/upstream > to a newer tag (or cherry-pick once available)? > > v6.16-rc1 took a bit longer this time to reach devicetree-rebasing I think, > but it landed this morning (UTC) so it isn't THAT long compared to the push > in master. We could ask devicetree-rebasing people to push the master branch > more often to avoid the up-to 2-week delay between v6.15 and v6.16-rc1 for > example. > > I have read nothing in this thread so this is absolutely not a jab at some > contributor or maintainer in this series. > > On a side-note, I think we should add -s to tools/update-subtree.sh's git > cherry-pick call to know who contributed the change to U-Boot (as the commit > author will be the same as in the kernel as far as I remember). > > To be fair, the whole process is a bit constraining, especially for new > boards. You may have to wait up to ~2 months (a full kernel release cycle) > to see a tag with the device tree for your board, and only then can you > support the board in U-Boot with OF_UPSTREAM. In Rockchip we typically force > OF_UPSTREAM for new boards, but we also typically do not accept hand-crafted > git commits to dts/upstream. I don't know if it's deterring contributors but > we are still getting some contributions here and there. Not sure how we > should be handling that :/ > > > [1] https://patchwork.ozlabs.org/project/uboot/list/?series=460450 > > > > > (which > > > happens, E Shattow on IRC was asking how to at least locally point > > > dts/upstream at something else, at least for local testing). > > > > > > > I would love to hear problems from people if that's for downstream > > development too. > > > > We can add things in > https://docs.u-boot.org/en/latest/develop/devicetree/control.html#where-do-i-get-a-devicetree-file-for-my-board > and > https://docs.u-boot.org/en/latest/develop/devicetree/control.html#resyncing-with-devicetree-rebasing > to be clearer on the limitations. Though til we enforce a check, this is > just information.
Thank you for taking the time to provide your perspective on this Quentin. And while I had hoped to be able to look over the history of dts/upstream and see that it was just the Makefiles (which I don't think we can avoid) being the difference between devicetree-rebasing and us, that's not quite the case. So, yes, Sumit's opinion that we must cherry-pick seems best. The documentation needs to be updated a bit, and some more clear examples perhaps provided too. I've added some local scripting that will complain at me about changes that don't have the cherry-pick message or that the commit referenced doesn't exist. I may spend a little time figuring out how to dump each commit and make sure they're the same, but that too much get tricky. -- Tom
signature.asc
Description: PGP signature
