Re: [PATCH 0/7] board: rockchip: add FriendlyElec NanoPi R3S

[Quentin Schulz <quentin.schulz@cherry.de>]

Hi Tianling,

On 1/14/25 5:14 PM, Tianling Shen wrote:
> Hi Quentin,
> 
> On 2025/1/14 23:21, Quentin Schulz wrote:
>> Hi Tianling,
>>
>> On 1/14/25 3:49 PM, Tianling Shen wrote:
>>> Hi Quentin,
>>>
>>> On 2025/1/14 22:39, Quentin Schulz wrote:
>>>> Hi Tianling,
>>>>
>>>> On 12/26/24 10:20 AM, Tianling Shen wrote:
>>>>> The NanoPi R3S(as "R3S") is an open source platform with dual-Gbps
>>>>> Ethernet ports designed and developed by FriendlyElec for IoT
>>>>> applications.
>>>>>
>>>>> Tianling Shen (7):
>>>>>    arm64: dts: rockchip: Add FriendlyARM NanoPi R3S board
>>>>>    arm64: dts: rockchip: fix model name for FriendlyElec NanoPi R3S
>>>>>    arm64: dts: rockchip: replace deprecated snps,reset props for 
>>>>> NanoPi
>>>>>      R3S
>>>>>    arm64: dts: rockchip: sort props in pmu_io_domains node for 
>>>>> NanoPi R3S
>>>>>    arm64: dts: rockchip: enable eMMC HS200 mode for NanoPi R3S
>>>>>    arm64: dts: rockchip: reorder mmc aliases for NanoPi R3S
>>>>
>>>> How did you backport the above patches?
>>>>
>>>> ./tools/update-subtree.sh pick dts <commit>
>>>>
>>>> is the tool to be used, it should have added a
>>>>
>>>
>>> Thank you for the tip! I did not know there's such a script and
>>> I just copy&paste the commit message from linux tree manually.
>>>
>>
>> Mmmm, how did you apply the patch in your tree then? Trying to figure 
>> out how we can avoid this in the future.
> 
> I fetched the patches[1] as-is from kernel and applied them by `git am 
> -3` with path correction, then maually added the line "[ upstream 
> commit: xxxxxx ]" to commit message.
> 
> 1. https://eur02.safelinks.protection.outlook.com/? 
> url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Fpatch%2F%3Fid%3D50decd493c8394c52d04561fe4ede34df27a46ba&data=05%7C02%7Cquentin.schulz%40cherry.de%7C72d5c95645ef4197c18908dd34b69565%7C5e0e1b5221b54e7b83bb514ec460677e%7C0%7C0%7C638724680973346196%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=urDrXWTzIt2NF%2BUrBSO1FnSTxjBO5Q4nYHFU9i%2BRII0%3D&reserved=0
> 
> https://eur02.safelinks.protection.outlook.com/? 
> url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Fpatch%2F%3Fid%3Db5bf84206a5c77528f9dd4cbca4e72caa063c102&data=05%7C02%7Cquentin.schulz%40cherry.de%7C72d5c95645ef4197c18908dd34b69565%7C5e0e1b5221b54e7b83bb514ec460677e%7C0%7C0%7C638724680973368900%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=x4CksM87w51QxzlrqNqjKvpQOJ6KKmSFQ01hoWmiCyg%3D&reserved=0
> 
> https://eur02.safelinks.protection.outlook.com/? 
> url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Fpatch%2F%3Fid%3D82b2868937883b65732da498b26366d34db61510&data=05%7C02%7Cquentin.schulz%40cherry.de%7C72d5c95645ef4197c18908dd34b69565%7C5e0e1b5221b54e7b83bb514ec460677e%7C0%7C0%7C638724680973382545%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=qyjod1yP7k%2F7bhjfHdZqn3LqettTPI41duikY0NRveg%3D&reserved=0
> 
> https://eur02.safelinks.protection.outlook.com/? 
> url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Fpatch%2F%3Fid%3D17e150fdd983c7e59b9240e34a166285f3c3fb39&data=05%7C02%7Cquentin.schulz%40cherry.de%7C72d5c95645ef4197c18908dd34b69565%7C5e0e1b5221b54e7b83bb514ec460677e%7C0%7C0%7C638724680973396320%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=Zv7tjHe0VKKp%2FuIBFu5zrO%2BusEVjm%2F3lJ73jdaTmVuA%3D&reserved=0
> 
> https://eur02.safelinks.protection.outlook.com/? 
> url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Fpatch%2F%3Fid%3D1b5365034410f1ca21adadadd492b99bdf4f2c55&data=05%7C02%7Cquentin.schulz%40cherry.de%7C72d5c95645ef4197c18908dd34b69565%7C5e0e1b5221b54e7b83bb514ec460677e%7C0%7C0%7C638724680973413639%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=%2F3uw74TSfuoUpOFkfJPLWzKjoh331Y5RaPvEa4DrDs4%3D&reserved=0
> 
> https://eur02.safelinks.protection.outlook.com/? 
> url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Fpatch%2F%3Fid%3Db7cd1115456d312f8c5e60c80fdc35fd35ea6eab&data=05%7C02%7Cquentin.schulz%40cherry.de%7C72d5c95645ef4197c18908dd34b69565%7C5e0e1b5221b54e7b83bb514ec460677e%7C0%7C0%7C638724680973431321%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=pFych4CEp7BfolVm1KMGk828MIkCaMRNw8hCf7p2ldI%3D&reserved=0
> 

Thanks for explaining!

>>
>> I'm wondering if we shouldn't have tooling in place to detect when 
>> things aren't done the proper way (for maintainers I mean). We 
>> **really** want to have dts/upstream be upstream + some patches that 
>> were already merged in devicetree-rebasing tree. I don't know enough 
>> about subtree merges that Tom does when updating to a new tagged 
>> release to know if it's actually safe or if the possible mistake made 
>> when applying a commit by hand can persist without us noticing. I 
>> guess a mistake made in a manually applied patch would be caught by 
>> Tom during the merge from the next release with a merge conflict, but 
>> then that's pain for him to debug.
> 
> I'm really sorry for that. All of those commits were already landed on 
> devicetree-rebasing tree so hope everything would be fine.
> 

Nothing to feel sorry for, we should have caught that before merging the 
patches, so the blame is rather on reviewers/maintainers :) If someone 
made the mistake once, it'll happen a second time. I'm glad it was 
caught now, so at least we're aware it's a possibility and we should 
either be more vigilant or write tooling for that.

For what it's worth, the documentation is here on how to cherry pick 
device tree changes: 
https://docs.u-boot.org/en/latest/develop/devicetree/control.html#resyncing-with-devicetree-rebasing

I'm not too sure if there's tooling we could write to check that the 
patches to dts/upstream, were done the proper way. I'm not sure anyone 
wants to touch the checkpatch Perl script and that would still not 
necessarily be run by the maintainer(s) when merging (don't know what's 
the process there). If merge requests are always coming from a GitLab 
tree, maybe there's something we could do from GitLab CI for any change 
made in dts/upstream/ subtree? (like remove the commit, reapply the 
patch on its parent with ./tools/update-subtree.sh pick dts pick <> and 
see if the commit log and content is the same?).

+Cc Tom for at least awareness, hope you don't mind.

Maybe I'm also making this a bigger problem than it is as I am no 
maintainer :)

Cheers,
Quentin