On Monday 23 January 2023 14:32:36 Christophe Leroy wrote: > Le 22/01/2023 à 12:16, Pali Rohár a écrit : > > Hello! Do you have any comments for this patch series? > > > I think patches 1 and 2 could be a single patch.
Well, if you want to have them in single patch, it could be easily squashed during applying. I thought that it is better to have them separated because of different boards, files, etc...: https://lore.kernel.org/linuxppc-dev/5bf1f2fc-a1de-d873-7d1b-0058ff8b9...@csgroup.eu/ > I'm having hard time understanding how things are built. Patch 3 > introduces 273 lines of new code in a file named p2020.c while only > removing 23 lines and 44 lines from mpc85xx_{ds/rdb}.c. In v1 I generated that patch with git -M, -C and other options which detects copy and renames. But I had an impression that it is less readable: https://lore.kernel.org/linuxppc-dev/20220819191557.28116-4-p...@kernel.org/ So I tried to describe all changes in commit message and generated that patch without copy options (so it is plain patch with add lines). This patch moves all p2020 boards from mpc85xx_rdb.c and mpc85xx_ds.c files into new p2020.c file, and plus it copies all helper functions which p2020 boards requires. This patch does not introduce any new code or functional change. It should be really plain copy/move. > Then patches 4, > 5 and 6 exclusively modify p2020.c which was a completely new file added > by patch 3. In later patches is then that moved/copied code improved and cleaned. > Why not making it correct from the beginning, that is merge > patches 4, 5 and 6 in patch 3 ? I wanted to separate logical changes into separate commits. So first just moves/copy code (which should be noop) and then do functional changes in followup patches. I like this progress because for me it is easier for reviewing. Important parts are functional changes, which are in separated commits and it is visually separated from boring move/copy code changes. > Or maybe p2020.c is not really new but is a copy of some previously > existing code ? In that case it would be better to make it explicit, for > history. Yes. Do you have any suggestion how to make it _more_ explicit? I tried to explain it in commit message (but I'm not sure if it is enough). And when viewing via git show, it is needed to call it with additional -M and -C options to see this. git does not do it automatically. > > Christophe > > > > > > On Saturday 24 December 2022 22:14:17 Pali Rohár wrote: > >> This patch series unifies all P2020 boards and machine descriptions into > >> one generic unified P2020 machine description. With this generic machine > >> description, kernel can boot on any P2020-based board with correct DTS > >> file. > >> > >> Tested on CZ.NIC Turris 1.1 board with has Freescale P2020 processor. > >> Kernel during booting correctly detects P2020 and prints: > >> [ 0.000000] Using Freescale P2020 machine description > >> > >> Changes in v2: > >> * Added patch "p2020: Move i8259 code into own function" (separated from > >> the next one) > >> * Renamed CONFIG_P2020 to CONFIG_PPC_P2020 > >> * Fixed descriptions > >> > >> Link to v1: > >> https://lore.kernel.org/linuxppc-dev/20220819191557.28116-1-p...@kernel.org/ > >> > >> Pali Rohár (8): > >> powerpc/85xx: Mark mpc85xx_rdb_pic_init() as static > >> powerpc/85xx: Mark mpc85xx_ds_pic_init() as static > >> powerpc/85xx: p2020: Move all P2020 machine descriptions to p2020.c > >> powerpc/85xx: p2020: Move i8259 code into own function > >> powerpc/85xx: p2020: Unify .setup_arch and .init_IRQ callbacks > >> powerpc/85xx: p2020: Define just one machine description > >> powerpc/85xx: p2020: Enable boards by new config option > >> CONFIG_PPC_P2020 > >> powerpc: dts: turris1x.dts: Remove "fsl,P2020RDB-PC" compatible string > >> > >> arch/powerpc/boot/dts/turris1x.dts | 2 +- > >> arch/powerpc/platforms/85xx/Kconfig | 22 ++- > >> arch/powerpc/platforms/85xx/Makefile | 1 + > >> arch/powerpc/platforms/85xx/mpc85xx_ds.c | 25 +-- > >> arch/powerpc/platforms/85xx/mpc85xx_rdb.c | 46 +----- > >> arch/powerpc/platforms/85xx/p2020.c | 193 ++++++++++++++++++++++ > >> 6 files changed, 215 insertions(+), 74 deletions(-) > >> create mode 100644 arch/powerpc/platforms/85xx/p2020.c > >> > >> -- > >> 2.20.1 > >>