| Message ID | 20241224210912.2121-1-naoki@radxa.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | arm64: dts: rockchip: Add Radxa ROCK 5B+ and rebase Radxa ROCK 5B | expand |
Hi Naoki, On Wed, Dec 25, 2024 at 12:22 PM FUKAUMI Naoki <naoki@radxa.com> wrote: > > The Radxa ROCK 5B+ is an upgraded version of the Radxa ROCK 5B. > > This patch series introduces a shared .dtsi that can be used on the > new Radxa ROCK 5B+, the existing Radxa ROCK 5B, and the upcoming Radxa > ROCK 5T. > > This patch series includes "arm64: dts: rockchip: Add USB-C support to > ROCK 5B" by Sebastian Reichel[1]. > > [1] https://patchwork.kernel.org/project/linux-rockchip/patch/20241210163615.120594-1-sebastian.reichel@collabora.com/ > > FUKAUMI Naoki (3): > dt-bindings: arm: rockchip: Add Radxa ROCK 5B+ > arm64: dts: rockchip: Add Radxa ROCK 5B+ > arm64: dts: rockchip: Convert Radxa ROCK 5B to use shared .dtsi This one doesn't look equivalent to the existing ROCK 5B device tree after your changes (I've noticed in particular that fan cooling levels are different - but that's just the part I remember well since I modified it earlier, there might be more differences). Would you mind rearranging the patches so that the split of ROCK 5B dts into a common .dtsi and per-board .dts additions could be made one-to-one equivalent without functional changes, and then add ROCK 5B+ and any other required changes on top of that? See what Dragan did where he prepared parent .dtsi for per-variant OPPs [1] [1] https://lore.kernel.org/linux-rockchip/9ffedc0e2ca7f167d9d795b2a8f43cb9f56a653b.1717923308.git.dsimic@manjaro.org/ Best, Alexey
Hello Alexey and Naoki, On 2024-12-25 09:32, Alexey Charkov wrote: > On Wed, Dec 25, 2024 at 12:22 PM FUKAUMI Naoki <naoki@radxa.com> wrote: >> >> The Radxa ROCK 5B+ is an upgraded version of the Radxa ROCK 5B. >> >> This patch series introduces a shared .dtsi that can be used on the >> new Radxa ROCK 5B+, the existing Radxa ROCK 5B, and the upcoming Radxa >> ROCK 5T. >> >> This patch series includes "arm64: dts: rockchip: Add USB-C support to >> ROCK 5B" by Sebastian Reichel[1]. >> >> [1] >> https://patchwork.kernel.org/project/linux-rockchip/patch/20241210163615.120594-1-sebastian.reichel@collabora.com/ >> >> FUKAUMI Naoki (3): >> dt-bindings: arm: rockchip: Add Radxa ROCK 5B+ >> arm64: dts: rockchip: Add Radxa ROCK 5B+ >> arm64: dts: rockchip: Convert Radxa ROCK 5B to use shared .dtsi > > This one doesn't look equivalent to the existing ROCK 5B device tree > after your changes (I've noticed in particular that fan cooling levels > are different - but that's just the part I remember well since I > modified it earlier, there might be more differences). > > Would you mind rearranging the patches so that the split of ROCK 5B > dts into a common .dtsi and per-board .dts additions could be made > one-to-one equivalent without functional changes, and then add ROCK > 5B+ and any other required changes on top of that? > > See what Dragan did where he prepared parent .dtsi for per-variant OPPs > [1] > > [1] > https://lore.kernel.org/linux-rockchip/9ffedc0e2ca7f167d9d795b2a8f43cb9f56a653b.1717923308.git.dsimic@manjaro.org/ Just a brief note that I agree with Alexey's recommendation above. These patches should be reworked in a way that makes the transition to using the shared dtsi file much more obvious. Besides making these changes easier to review that way, and giving better chances to ensuring there are no issues that managed to slip through the cracks, it will also be much easier later to track down any possible regressions, in case they eventually are discovered.
Hi Alexey, On 12/25/24 17:32, Alexey Charkov wrote: > Hi Naoki, > > On Wed, Dec 25, 2024 at 12:22 PM FUKAUMI Naoki <naoki@radxa.com> wrote: >> >> The Radxa ROCK 5B+ is an upgraded version of the Radxa ROCK 5B. >> >> This patch series introduces a shared .dtsi that can be used on the >> new Radxa ROCK 5B+, the existing Radxa ROCK 5B, and the upcoming Radxa >> ROCK 5T. >> >> This patch series includes "arm64: dts: rockchip: Add USB-C support to >> ROCK 5B" by Sebastian Reichel[1]. >> >> [1] https://patchwork.kernel.org/project/linux-rockchip/patch/20241210163615.120594-1-sebastian.reichel@collabora.com/ >> >> FUKAUMI Naoki (3): >> dt-bindings: arm: rockchip: Add Radxa ROCK 5B+ >> arm64: dts: rockchip: Add Radxa ROCK 5B+ >> arm64: dts: rockchip: Convert Radxa ROCK 5B to use shared .dtsi > > This one doesn't look equivalent to the existing ROCK 5B device tree > after your changes (I've noticed in particular that fan cooling levels > are different - but that's just the part I remember well since I > modified it earlier, there might be more differences). Thanks for pointing! > Would you mind rearranging the patches so that the split of ROCK 5B > dts into a common .dtsi and per-board .dts additions could be made > one-to-one equivalent without functional changes, and then add ROCK > 5B+ and any other required changes on top of that? My priority goal is to add support for ROCK 5B+ and 5T. This time, I will not make any changes to ROCK 5B so as not to break it. Therefore, I will drop PATCH 3/3. (I would like to make changes to ROCK 5B at some point.) Best regards, -- FUKAUMI Naoki Radxa Computer (Shenzhen) Co., Ltd. > See what Dragan did where he prepared parent .dtsi for per-variant OPPs [1] > > [1] https://lore.kernel.org/linux-rockchip/9ffedc0e2ca7f167d9d795b2a8f43cb9f56a653b.1717923308.git.dsimic@manjaro.org/ > > Best, > Alexey >
Hello Naoki, On 2024-12-25 11:09, FUKAUMI Naoki wrote: > On 12/25/24 17:32, Alexey Charkov wrote: >> On Wed, Dec 25, 2024 at 12:22 PM FUKAUMI Naoki <naoki@radxa.com> >> wrote: >>> >>> The Radxa ROCK 5B+ is an upgraded version of the Radxa ROCK 5B. >>> >>> This patch series introduces a shared .dtsi that can be used on the >>> new Radxa ROCK 5B+, the existing Radxa ROCK 5B, and the upcoming >>> Radxa >>> ROCK 5T. >>> >>> This patch series includes "arm64: dts: rockchip: Add USB-C support >>> to >>> ROCK 5B" by Sebastian Reichel[1]. >>> >>> [1] >>> https://patchwork.kernel.org/project/linux-rockchip/patch/20241210163615.120594-1-sebastian.reichel@collabora.com/ >>> >>> FUKAUMI Naoki (3): >>> dt-bindings: arm: rockchip: Add Radxa ROCK 5B+ >>> arm64: dts: rockchip: Add Radxa ROCK 5B+ >>> arm64: dts: rockchip: Convert Radxa ROCK 5B to use shared .dtsi >> >> This one doesn't look equivalent to the existing ROCK 5B device tree >> after your changes (I've noticed in particular that fan cooling levels >> are different - but that's just the part I remember well since I >> modified it earlier, there might be more differences). > > Thanks for pointing! > >> Would you mind rearranging the patches so that the split of ROCK 5B >> dts into a common .dtsi and per-board .dts additions could be made >> one-to-one equivalent without functional changes, and then add ROCK >> 5B+ and any other required changes on top of that? > > My priority goal is to add support for ROCK 5B+ and 5T. This time, I > will not make any changes to ROCK 5B so as not to break it. Therefore, > I will drop PATCH 3/3. > > (I would like to make changes to ROCK 5B at some point.) I see, but I think that just dropping the patch 3/3 wouldn't be the best possible option. That way you'd end up with a "dangling" dtsi file, used by just one board dts file, which is a bit suboptimal. I'd suggest that you actually do it the right way, so to speak, by actually converting the ROCK 5B dts file to use the shared dtsi file in a way that makes the transition more obvious and, as a result, easier to review, as already suggested above. As another option, I'm going to ask Radxa for a sample ROCK 5B+ board soon, for my Rockchip SoC binning project, and if I actually get it, I could take over this series, to help you that way with getting the patch series in better shape.
Hi Dragan, On 12/25/24 19:25, Dragan Simic wrote: > Hello Naoki, > > On 2024-12-25 11:09, FUKAUMI Naoki wrote: >> On 12/25/24 17:32, Alexey Charkov wrote: >>> On Wed, Dec 25, 2024 at 12:22 PM FUKAUMI Naoki <naoki@radxa.com> wrote: >>>> >>>> The Radxa ROCK 5B+ is an upgraded version of the Radxa ROCK 5B. >>>> >>>> This patch series introduces a shared .dtsi that can be used on the >>>> new Radxa ROCK 5B+, the existing Radxa ROCK 5B, and the upcoming Radxa >>>> ROCK 5T. >>>> >>>> This patch series includes "arm64: dts: rockchip: Add USB-C support to >>>> ROCK 5B" by Sebastian Reichel[1]. >>>> >>>> [1] https://patchwork.kernel.org/project/linux-rockchip/ >>>> patch/20241210163615.120594-1-sebastian.reichel@collabora.com/ >>>> >>>> FUKAUMI Naoki (3): >>>> dt-bindings: arm: rockchip: Add Radxa ROCK 5B+ >>>> arm64: dts: rockchip: Add Radxa ROCK 5B+ >>>> arm64: dts: rockchip: Convert Radxa ROCK 5B to use shared .dtsi >>> >>> This one doesn't look equivalent to the existing ROCK 5B device tree >>> after your changes (I've noticed in particular that fan cooling levels >>> are different - but that's just the part I remember well since I >>> modified it earlier, there might be more differences). >> >> Thanks for pointing! >> >>> Would you mind rearranging the patches so that the split of ROCK 5B >>> dts into a common .dtsi and per-board .dts additions could be made >>> one-to-one equivalent without functional changes, and then add ROCK >>> 5B+ and any other required changes on top of that? >> >> My priority goal is to add support for ROCK 5B+ and 5T. This time, I >> will not make any changes to ROCK 5B so as not to break it. Therefore, >> I will drop PATCH 3/3. >> >> (I would like to make changes to ROCK 5B at some point.) > > I see, but I think that just dropping the patch 3/3 wouldn't be the > best possible option. That way you'd end up with a "dangling" dtsi > file, used by just one board dts file, which is a bit suboptimal. So please ignore this patch series until I can add ROCK 5T, which will be available soon. (Maybe...) I'm sending v2 due to a small fix, please ignore it. > I'd suggest that you actually do it the right way, so to speak, by > actually converting the ROCK 5B dts file to use the shared dtsi file > in a way that makes the transition more obvious and, as a result, > easier to review, as already suggested above. If I have enough time to fix the ROCK 5B dts, I will do it. Best regards, -- FUKAUMI Naoki Radxa Computer (Shenzhen) Co., Ltd. > As another option, I'm going to ask Radxa for a sample ROCK 5B+ board > soon, for my Rockchip SoC binning project, and if I actually get it, > I could take over this series, to help you that way with getting the > patch series in better shape.
The Radxa ROCK 5B+ is an upgraded version of the Radxa ROCK 5B. This patch series introduces a shared .dtsi that can be used on the new Radxa ROCK 5B+, the existing Radxa ROCK 5B, and the upcoming Radxa ROCK 5T. This patch series includes "arm64: dts: rockchip: Add USB-C support to ROCK 5B" by Sebastian Reichel[1]. [1] https://patchwork.kernel.org/project/linux-rockchip/patch/20241210163615.120594-1-sebastian.reichel@collabora.com/ FUKAUMI Naoki (3): dt-bindings: arm: rockchip: Add Radxa ROCK 5B+ arm64: dts: rockchip: Add Radxa ROCK 5B+ arm64: dts: rockchip: Convert Radxa ROCK 5B to use shared .dtsi .../devicetree/bindings/arm/rockchip.yaml | 5 + arch/arm64/boot/dts/rockchip/Makefile | 1 + ...{rk3588-rock-5b.dts => rk3588-rock-5.dtsi} | 555 +++++---- .../boot/dts/rockchip/rk3588-rock-5b-plus.dts | 97 ++ .../boot/dts/rockchip/rk3588-rock-5b.dts | 1026 ++--------------- 5 files changed, 549 insertions(+), 1135 deletions(-) copy arch/arm64/boot/dts/rockchip/{rk3588-rock-5b.dts => rk3588-rock-5.dtsi} (71%) create mode 100644 arch/arm64/boot/dts/rockchip/rk3588-rock-5b-plus.dts rewrite arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts (93%)