Re: [PATCH v2 0/8] powerpc/85xx: p2020: Create one unified machine description

["Pali Rohár" <pali@kernel.org>]

On Monday 23 January 2023 21:09:22 Pali Rohár wrote:
> 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-0058ff8b9aa2@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-pali@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.

I sent same patch but generated by git -M and -C options. See if it is
better.

> > 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.

Do you have any other suggestions what should I do?

> > 
> > 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-pali@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
> > >>