Re: [PATCH v2 0/8] powerpc/85xx: p2020: Create one unified machine description
On Tuesday 14 February 2023 05:47:08 Christophe Leroy wrote: > Le 13/02/2023 à 21:11, Pali Rohár a écrit : > > On Monday 13 February 2023 19:58:15 Christophe Leroy wrote: > >> Le 09/02/2023 à 01:15, Pali Rohár a écrit : > > 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. > >> > >> Yes after looking into it in more details, it is exactly that. You > >> copied all helper functions but this is not said in the commit message. > >> I think it should be said, and more important it should be explained why. > >> Because this is exactly what I was not understanding, why I couldn't see > >> all moved functions: just because many of them were not moved but copied. > >> > >> In the two first pages you made some function static, and then you > >> duplicated it. Why ? Why not keep it global and just use it from one > >> place to the other ? > >> > >> Because after patch 3 we have: > >> > >> arch/powerpc/platforms/85xx/mpc85xx_rdb.c:static void __init > >> mpc85xx_rdb_pic_init(void) > >> arch/powerpc/platforms/85xx/p2020.c:static void __init > >> mpc85xx_rdb_pic_init(void) > >> > >> arch/powerpc/platforms/85xx/mpc85xx_ds.c:static void __init > >> mpc85xx_ds_pic_init(void) > >> arch/powerpc/platforms/85xx/p2020.c:static void __init > >> mpc85xx_ds_pic_init(void) > >> > >> Why not just drop patches 1 and 2 and keep those two functions and all > >> the other common functions like mpc85xx_8259_cascade() > >> mpc85xx_ds_uli_init() and a lot more in a separate common file ? > >> > >> Christophe > > > > After applying all patches there is no mpc85xx_rdb_pic_init() / > > mpc85xx_ds_pic_init() function in p2020.c file. There is > > p2020_pic_init() in p2020.c but it is slightly different than previous > > two functions. > > Ok, fair enough, but then please explain in the commit message that you > copy the functions and then they will be re-written in following > patches. That way we know exactly what we are reviewing. But it is already explained in the commit message. Is not it enough? Or should I rephrase some parts of the commit message? > > > > Maybe it could be possible to create one function mpc85xx_pic_init() as > > unification of previous 3 functions, but such change would be needed to > > test on lot of mpc85xx boards, which would be affected by such change. > > And I do not have them for testing. I have only P2020. > > No, if the function are different it's better each platform has its own. > My comment was for functions that are exactly the same. > > > > > So I wrote *_pic_init() function which is p2020 specific, like already > > existing ds and rdb specific functions in their own source files.
Re: [PATCH v2 0/8] powerpc/85xx: p2020: Create one unified machine description
Le 13/02/2023 à 21:11, Pali Rohár a écrit : > On Monday 13 February 2023 19:58:15 Christophe Leroy wrote: >> Le 09/02/2023 à 01:15, Pali Rohár a écrit : 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. >> >> Yes after looking into it in more details, it is exactly that. You >> copied all helper functions but this is not said in the commit message. >> I think it should be said, and more important it should be explained why. >> Because this is exactly what I was not understanding, why I couldn't see >> all moved functions: just because many of them were not moved but copied. >> >> In the two first pages you made some function static, and then you >> duplicated it. Why ? Why not keep it global and just use it from one >> place to the other ? >> >> Because after patch 3 we have: >> >> arch/powerpc/platforms/85xx/mpc85xx_rdb.c:static void __init >> mpc85xx_rdb_pic_init(void) >> arch/powerpc/platforms/85xx/p2020.c:static void __init >> mpc85xx_rdb_pic_init(void) >> >> arch/powerpc/platforms/85xx/mpc85xx_ds.c:static void __init >> mpc85xx_ds_pic_init(void) >> arch/powerpc/platforms/85xx/p2020.c:static void __init >> mpc85xx_ds_pic_init(void) >> >> Why not just drop patches 1 and 2 and keep those two functions and all >> the other common functions like mpc85xx_8259_cascade() >> mpc85xx_ds_uli_init() and a lot more in a separate common file ? >> >> Christophe > > After applying all patches there is no mpc85xx_rdb_pic_init() / > mpc85xx_ds_pic_init() function in p2020.c file. There is > p2020_pic_init() in p2020.c but it is slightly different than previous > two functions. Ok, fair enough, but then please explain in the commit message that you copy the functions and then they will be re-written in following patches. That way we know exactly what we are reviewing. > > Maybe it could be possible to create one function mpc85xx_pic_init() as > unification of previous 3 functions, but such change would be needed to > test on lot of mpc85xx boards, which would be affected by such change. > And I do not have them for testing. I have only P2020. No, if the function are different it's better each platform has its own. My comment was for functions that are exactly the same. > > So I wrote *_pic_init() function which is p2020 specific, like already > existing ds and rdb specific functions in their own source files.
Re: [PATCH v2 0/8] powerpc/85xx: p2020: Create one unified machine description
On Monday 13 February 2023 19:58:15 Christophe Leroy wrote: > Le 09/02/2023 à 01:15, Pali Rohár a écrit : > >> > >> 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. > > Yes after looking into it in more details, it is exactly that. You > copied all helper functions but this is not said in the commit message. > I think it should be said, and more important it should be explained why. > Because this is exactly what I was not understanding, why I couldn't see > all moved functions: just because many of them were not moved but copied. > > In the two first pages you made some function static, and then you > duplicated it. Why ? Why not keep it global and just use it from one > place to the other ? > > Because after patch 3 we have: > > arch/powerpc/platforms/85xx/mpc85xx_rdb.c:static void __init > mpc85xx_rdb_pic_init(void) > arch/powerpc/platforms/85xx/p2020.c:static void __init > mpc85xx_rdb_pic_init(void) > > arch/powerpc/platforms/85xx/mpc85xx_ds.c:static void __init > mpc85xx_ds_pic_init(void) > arch/powerpc/platforms/85xx/p2020.c:static void __init > mpc85xx_ds_pic_init(void) > > Why not just drop patches 1 and 2 and keep those two functions and all > the other common functions like mpc85xx_8259_cascade() > mpc85xx_ds_uli_init() and a lot more in a separate common file ? > > Christophe After applying all patches there is no mpc85xx_rdb_pic_init() / mpc85xx_ds_pic_init() function in p2020.c file. There is p2020_pic_init() in p2020.c but it is slightly different than previous two functions. Maybe it could be possible to create one function mpc85xx_pic_init() as unification of previous 3 functions, but such change would be needed to test on lot of mpc85xx boards, which would be affected by such change. And I do not have them for testing. I have only P2020. So I wrote *_pic_init() function which is p2020 specific, like already existing ds and rdb specific functions in their own source files.
Re: [PATCH v2 0/8] powerpc/85xx: p2020: Create one unified machine description
Le 09/02/2023 à 01:15, Pali Rohár a écrit : >> >> 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. Yes after looking into it in more details, it is exactly that. You copied all helper functions but this is not said in the commit message. I think it should be said, and more important it should be explained why. Because this is exactly what I was not understanding, why I couldn't see all moved functions: just because many of them were not moved but copied. In the two first pages you made some function static, and then you duplicated it. Why ? Why not keep it global and just use it from one place to the other ? Because after patch 3 we have: arch/powerpc/platforms/85xx/mpc85xx_rdb.c:static void __init mpc85xx_rdb_pic_init(void) arch/powerpc/platforms/85xx/p2020.c:static void __init mpc85xx_rdb_pic_init(void) arch/powerpc/platforms/85xx/mpc85xx_ds.c:static void __init mpc85xx_ds_pic_init(void) arch/powerpc/platforms/85xx/p2020.c:static void __init mpc85xx_ds_pic_init(void) Why not just drop patches 1 and 2 and keep those two functions and all the other common functions like mpc85xx_8259_cascade() mpc85xx_ds_uli_init() and a lot more in a separate common file ? Christophe
Re: [PATCH v2 0/8] powerpc/85xx: p2020: Create one unified machine description
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-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.
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.00] 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
> > >>
Re: [PATCH v2 0/8] powerpc/85xx: p2020: Create one unified machine description
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.00] 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
> >>
Re: [PATCH v2 0/8] powerpc/85xx: p2020: Create one unified machine description
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.
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. Then patches 4,
5 and 6 exclusively modify p2020.c which was a completely new file added
by patch 3. Why not making it correct from the beginning, that is merge
patches 4, 5 and 6 in patch 3 ?
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.
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.00] 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
>>
Re: [PATCH v2 0/8] powerpc/85xx: p2020: Create one unified machine description
Hello! Do you have any comments for this patch series? 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.00] 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 >
