On Monday 23 January 2023 21:09:22 Pali Rohár wrote:

On Monday 23 January 2023 14:32:36 Christophe Leroy wrote:

quoted

Le 22/01/2023 à 12:16, Pali Rohár a écrit :

quoted

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/ (local)

quoted

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/ (local)

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.

quoted

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.

quoted

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.

quoted

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?

quoted

Christophe

quoted

On Saturday 24 December 2022 22:14:17 Pali Rohár wrote:

quoted

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/ (local)

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