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

[Pali Rohár]

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@xxxxxxxxxx/

> 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@xxxxxxxxxx/

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