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/[email protected]/
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
"fsl,P2020RDB-PC" compatible string was present in Turris 1.x DTS file just
because Linux kernel required it for proper detection of P2020 processor
during boot.
This was quite a hack as CZ.NIC Turris 1.x is not compatible with
Freescale P2020-RDB-PC board.
Now when kernel has generic unified support for boards with P2020
processors, there is no need to have this "hack" in turris1x.dts file.
So remove incorrect "fsl,P2020RDB-PC" compatible string from turris1x.dts.
Signed-off-by: Pali Rohár <[email protected]>
---
arch/powerpc/boot/dts/turris1x.dts | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/boot/dts/turris1x.dts b/arch/powerpc/boot/dts/turris1x.dts
index e9cda34a140e..a95857de6858 100644
--- a/arch/powerpc/boot/dts/turris1x.dts
+++ b/arch/powerpc/boot/dts/turris1x.dts
@@ -15,7 +15,7 @@
/ {
model = "Turris 1.x";
- compatible = "cznic,turris1x", "fsl,P2020RDB-PC"; /* fsl,P2020RDB-PC is required for booting Linux */
+ compatible = "cznic,turris1x";
aliases {
ethernet0 = &enet0;
--
2.20.1
Make just one .setup_arch and one .init_IRQ callback implementation for all
P2020 board code. This deduplicate repeated and same code.
Signed-off-by: Pali Rohár <[email protected]>
---
arch/powerpc/platforms/85xx/p2020.c | 58 +++++------------------------
1 file changed, 9 insertions(+), 49 deletions(-)
diff --git a/arch/powerpc/platforms/85xx/p2020.c b/arch/powerpc/platforms/85xx/p2020.c
index b8584bf307b0..adf3750abef9 100644
--- a/arch/powerpc/platforms/85xx/p2020.c
+++ b/arch/powerpc/platforms/85xx/p2020.c
@@ -42,8 +42,6 @@
#define DBG(fmt, args...)
#endif
-#ifdef CONFIG_MPC85xx_DS
-
#ifdef CONFIG_PPC_I8259
static void mpc85xx_8259_cascade(struct irq_desc *desc)
@@ -90,7 +88,7 @@ static void __init mpc85xx_8259_init(void)
#endif /* CONFIG_PPC_I8259 */
-static void __init mpc85xx_ds_pic_init(void)
+static void __init p2020_pic_init(void)
{
struct mpic *mpic;
@@ -143,58 +141,20 @@ static void __init mpc85xx_ds_uli_init(void)
#endif
}
-#endif /* CONFIG_MPC85xx_DS */
-
-#ifdef CONFIG_MPC85xx_RDB
-static void __init mpc85xx_rdb_pic_init(void)
-{
- struct mpic *mpic;
-
- mpic = mpic_alloc(NULL, 0,
- MPIC_BIG_ENDIAN |
- MPIC_SINGLE_DEST_CPU,
- 0, 256, " OpenPIC ");
-
- BUG_ON(mpic == NULL);
- mpic_init(mpic);
-}
-#endif /* CONFIG_MPC85xx_RDB */
-
/*
* Setup the architecture
*/
-#ifdef CONFIG_MPC85xx_DS
-static void __init mpc85xx_ds_setup_arch(void)
+static void __init p2020_setup_arch(void)
{
- if (ppc_md.progress)
- ppc_md.progress("mpc85xx_ds_setup_arch()", 0);
-
swiotlb_detect_4g();
fsl_pci_assign_primary();
mpc85xx_ds_uli_init();
mpc85xx_smp_init();
- printk("MPC85xx DS board from Freescale Semiconductor\n");
-}
-#endif /* CONFIG_MPC85xx_DS */
-
-#ifdef CONFIG_MPC85xx_RDB
-static void __init mpc85xx_rdb_setup_arch(void)
-{
- if (ppc_md.progress)
- ppc_md.progress("mpc85xx_rdb_setup_arch()", 0);
-
- mpc85xx_smp_init();
-
- fsl_pci_assign_primary();
-
#ifdef CONFIG_QUICC_ENGINE
mpc85xx_qe_par_io_init();
-#endif /* CONFIG_QUICC_ENGINE */
-
- printk(KERN_INFO "MPC85xx RDB board from Freescale Semiconductor\n");
+#endif
}
-#endif /* CONFIG_MPC85xx_RDB */
#ifdef CONFIG_MPC85xx_DS
machine_arch_initcall(p2020_ds, mpc85xx_common_publish_devices);
@@ -235,8 +195,8 @@ static int __init p2020_rdb_pc_probe(void)
define_machine(p2020_ds) {
.name = "P2020 DS",
.probe = p2020_ds_probe,
- .setup_arch = mpc85xx_ds_setup_arch,
- .init_IRQ = mpc85xx_ds_pic_init,
+ .setup_arch = p2020_setup_arch,
+ .init_IRQ = p2020_pic_init,
#ifdef CONFIG_PCI
.pcibios_fixup_bus = fsl_pcibios_fixup_bus,
.pcibios_fixup_phb = fsl_pcibios_fixup_phb,
@@ -251,8 +211,8 @@ define_machine(p2020_ds) {
define_machine(p2020_rdb) {
.name = "P2020 RDB",
.probe = p2020_rdb_probe,
- .setup_arch = mpc85xx_rdb_setup_arch,
- .init_IRQ = mpc85xx_rdb_pic_init,
+ .setup_arch = p2020_setup_arch,
+ .init_IRQ = p2020_pic_init,
#ifdef CONFIG_PCI
.pcibios_fixup_bus = fsl_pcibios_fixup_bus,
.pcibios_fixup_phb = fsl_pcibios_fixup_phb,
@@ -265,8 +225,8 @@ define_machine(p2020_rdb) {
define_machine(p2020_rdb_pc) {
.name = "P2020RDB-PC",
.probe = p2020_rdb_pc_probe,
- .setup_arch = mpc85xx_rdb_setup_arch,
- .init_IRQ = mpc85xx_rdb_pic_init,
+ .setup_arch = p2020_setup_arch,
+ .init_IRQ = p2020_pic_init,
#ifdef CONFIG_PCI
.pcibios_fixup_bus = fsl_pcibios_fixup_bus,
.pcibios_fixup_phb = fsl_pcibios_fixup_phb,
--
2.20.1
Function mpc85xx_ds_pic_init() is not used out of the mpc85xx_ds.c file.
Signed-off-by: Pali Rohár <[email protected]>
---
arch/powerpc/platforms/85xx/mpc85xx_ds.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/85xx/mpc85xx_ds.c b/arch/powerpc/platforms/85xx/mpc85xx_ds.c
index f8d2c97f39bd..9a6d637ef54a 100644
--- a/arch/powerpc/platforms/85xx/mpc85xx_ds.c
+++ b/arch/powerpc/platforms/85xx/mpc85xx_ds.c
@@ -54,7 +54,7 @@ static void mpc85xx_8259_cascade(struct irq_desc *desc)
}
#endif /* CONFIG_PPC_I8259 */
-void __init mpc85xx_ds_pic_init(void)
+static void __init mpc85xx_ds_pic_init(void)
{
struct mpic *mpic;
#ifdef CONFIG_PPC_I8259
--
2.20.1
Splits mpic and i8259 initialization codes into separate functions.
Signed-off-by: Pali Rohár <[email protected]>
---
arch/powerpc/platforms/85xx/p2020.c | 37 ++++++++++++++++-------------
1 file changed, 21 insertions(+), 16 deletions(-)
diff --git a/arch/powerpc/platforms/85xx/p2020.c b/arch/powerpc/platforms/85xx/p2020.c
index d65d4c88ac47..b8584bf307b0 100644
--- a/arch/powerpc/platforms/85xx/p2020.c
+++ b/arch/powerpc/platforms/85xx/p2020.c
@@ -45,6 +45,7 @@
#ifdef CONFIG_MPC85xx_DS
#ifdef CONFIG_PPC_I8259
+
static void mpc85xx_8259_cascade(struct irq_desc *desc)
{
struct irq_chip *chip = irq_desc_get_chip(desc);
@@ -55,27 +56,13 @@ static void mpc85xx_8259_cascade(struct irq_desc *desc)
}
chip->irq_eoi(&desc->irq_data);
}
-#endif /* CONFIG_PPC_I8259 */
-static void __init mpc85xx_ds_pic_init(void)
+static void __init mpc85xx_8259_init(void)
{
- struct mpic *mpic;
-#ifdef CONFIG_PPC_I8259
struct device_node *np;
struct device_node *cascade_node = NULL;
int cascade_irq;
-#endif
-
- mpic = mpic_alloc(NULL, 0,
- MPIC_BIG_ENDIAN |
- MPIC_SINGLE_DEST_CPU,
- 0, 256, " OpenPIC ");
-
- BUG_ON(mpic == NULL);
- mpic_init(mpic);
-#ifdef CONFIG_PPC_I8259
- /* Initialize the i8259 controller */
for_each_node_by_type(np, "interrupt-controller")
if (of_device_is_compatible(np, "chrp,iic")) {
cascade_node = np;
@@ -93,13 +80,31 @@ static void __init mpc85xx_ds_pic_init(void)
return;
}
- DBG("mpc85xxds: cascade mapped to irq %d\n", cascade_irq);
+ DBG("i8259: cascade mapped to irq %d\n", cascade_irq);
i8259_init(cascade_node, 0);
of_node_put(cascade_node);
irq_set_chained_handler(cascade_irq, mpc85xx_8259_cascade);
+}
+
#endif /* CONFIG_PPC_I8259 */
+
+static void __init mpc85xx_ds_pic_init(void)
+{
+ struct mpic *mpic;
+
+ mpic = mpic_alloc(NULL, 0,
+ MPIC_BIG_ENDIAN |
+ MPIC_SINGLE_DEST_CPU,
+ 0, 256, " OpenPIC ");
+
+ BUG_ON(mpic == NULL);
+ mpic_init(mpic);
+
+#ifdef CONFIG_PPC_I8259
+ mpc85xx_8259_init();
+#endif
}
#ifdef CONFIG_PCI
--
2.20.1
Combine machine descriptions and code of all P2020 boards into just one
generic unified P2020 machine description. This allows kernel to boot on
any P2020-based board with P2020 DTS file without need to patch kernel and
define a new machine description in 85xx powerpc platform directory.
Signed-off-by: Pali Rohár <[email protected]>
---
arch/powerpc/platforms/85xx/p2020.c | 83 +++++++----------------------
1 file changed, 19 insertions(+), 64 deletions(-)
diff --git a/arch/powerpc/platforms/85xx/p2020.c b/arch/powerpc/platforms/85xx/p2020.c
index adf3750abef9..b3fb600e1d83 100644
--- a/arch/powerpc/platforms/85xx/p2020.c
+++ b/arch/powerpc/platforms/85xx/p2020.c
@@ -156,83 +156,38 @@ static void __init p2020_setup_arch(void)
#endif
}
-#ifdef CONFIG_MPC85xx_DS
-machine_arch_initcall(p2020_ds, mpc85xx_common_publish_devices);
-#endif /* CONFIG_MPC85xx_DS */
-
-#ifdef CONFIG_MPC85xx_RDB
-machine_arch_initcall(p2020_rdb, mpc85xx_common_publish_devices);
-machine_arch_initcall(p2020_rdb_pc, mpc85xx_common_publish_devices);
-#endif /* CONFIG_MPC85xx_RDB */
+machine_arch_initcall(p2020, mpc85xx_common_publish_devices);
/*
* Called very early, device-tree isn't unflattened
*/
-#ifdef CONFIG_MPC85xx_DS
-static int __init p2020_ds_probe(void)
-{
- return !!of_machine_is_compatible("fsl,P2020DS");
-}
-#endif /* CONFIG_MPC85xx_DS */
-
-#ifdef CONFIG_MPC85xx_RDB
-static int __init p2020_rdb_probe(void)
-{
- if (of_machine_is_compatible("fsl,P2020RDB"))
- return 1;
- return 0;
-}
-
-static int __init p2020_rdb_pc_probe(void)
+static int __init p2020_probe(void)
{
- if (of_machine_is_compatible("fsl,P2020RDB-PC"))
- return 1;
- return 0;
+ struct device_node *p2020_cpu;
+
+ /*
+ * There is no common compatible string for all P2020 boards.
+ * The only common thing is "PowerPC,P2020@0" cpu node.
+ * So check for P2020 board via this cpu node.
+ */
+ p2020_cpu = of_find_node_by_path("/cpus/PowerPC,P2020@0");
+ if (!p2020_cpu)
+ return 0;
+
+ of_node_put(p2020_cpu);
+ return 1;
}
-#endif /* CONFIG_MPC85xx_RDB */
-
-#ifdef CONFIG_MPC85xx_DS
-define_machine(p2020_ds) {
- .name = "P2020 DS",
- .probe = p2020_ds_probe,
- .setup_arch = p2020_setup_arch,
- .init_IRQ = p2020_pic_init,
-#ifdef CONFIG_PCI
- .pcibios_fixup_bus = fsl_pcibios_fixup_bus,
- .pcibios_fixup_phb = fsl_pcibios_fixup_phb,
-#endif
- .get_irq = mpic_get_irq,
- .calibrate_decr = generic_calibrate_decr,
- .progress = udbg_progress,
-};
-#endif /* CONFIG_MPC85xx_DS */
-
-#ifdef CONFIG_MPC85xx_RDB
-define_machine(p2020_rdb) {
- .name = "P2020 RDB",
- .probe = p2020_rdb_probe,
- .setup_arch = p2020_setup_arch,
- .init_IRQ = p2020_pic_init,
-#ifdef CONFIG_PCI
- .pcibios_fixup_bus = fsl_pcibios_fixup_bus,
- .pcibios_fixup_phb = fsl_pcibios_fixup_phb,
-#endif
- .get_irq = mpic_get_irq,
- .calibrate_decr = generic_calibrate_decr,
- .progress = udbg_progress,
-};
-define_machine(p2020_rdb_pc) {
- .name = "P2020RDB-PC",
- .probe = p2020_rdb_pc_probe,
+define_machine(p2020) {
+ .name = "Freescale P2020",
+ .probe = p2020_probe,
.setup_arch = p2020_setup_arch,
.init_IRQ = p2020_pic_init,
#ifdef CONFIG_PCI
.pcibios_fixup_bus = fsl_pcibios_fixup_bus,
- .pcibios_fixup_phb = fsl_pcibios_fixup_phb,
+ .pcibios_fixup_phb = fsl_pcibios_fixup_phb,
#endif
.get_irq = mpic_get_irq,
.calibrate_decr = generic_calibrate_decr,
.progress = udbg_progress,
};
-#endif /* CONFIG_MPC85xx_RDB */
--
2.20.1
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.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/[email protected]/
>
> 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
>
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.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/[email protected]/
>>
>> 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
>>
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/[email protected]/
> 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/[email protected]/
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/[email protected]/
> >>
> >> 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
> >>
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/[email protected]/
>
> > 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/[email protected]/
>
> 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/[email protected]/
> > >>
> > >> 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
> > >>
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
Le 24/12/2022 à 22:14, Pali Rohár a écrit :
> Splits mpic and i8259 initialization codes into separate functions.
>
> Signed-off-by: Pali Rohár <[email protected]>
> ---
> arch/powerpc/platforms/85xx/p2020.c | 37 ++++++++++++++++-------------
> 1 file changed, 21 insertions(+), 16 deletions(-)
>
> diff --git a/arch/powerpc/platforms/85xx/p2020.c b/arch/powerpc/platforms/85xx/p2020.c
> index d65d4c88ac47..b8584bf307b0 100644
> --- a/arch/powerpc/platforms/85xx/p2020.c
> +++ b/arch/powerpc/platforms/85xx/p2020.c
> @@ -45,6 +45,7 @@
> #ifdef CONFIG_MPC85xx_DS
>
> #ifdef CONFIG_PPC_I8259
> +
> static void mpc85xx_8259_cascade(struct irq_desc *desc)
> {
> struct irq_chip *chip = irq_desc_get_chip(desc);
> @@ -55,27 +56,13 @@ static void mpc85xx_8259_cascade(struct irq_desc *desc)
> }
> chip->irq_eoi(&desc->irq_data);
> }
> -#endif /* CONFIG_PPC_I8259 */
>
> -static void __init mpc85xx_ds_pic_init(void)
> +static void __init mpc85xx_8259_init(void)
> {
> - struct mpic *mpic;
> -#ifdef CONFIG_PPC_I8259
> struct device_node *np;
> struct device_node *cascade_node = NULL;
> int cascade_irq;
> -#endif
> -
> - mpic = mpic_alloc(NULL, 0,
> - MPIC_BIG_ENDIAN |
> - MPIC_SINGLE_DEST_CPU,
> - 0, 256, " OpenPIC ");
> -
> - BUG_ON(mpic == NULL);
> - mpic_init(mpic);
>
> -#ifdef CONFIG_PPC_I8259
> - /* Initialize the i8259 controller */
> for_each_node_by_type(np, "interrupt-controller")
> if (of_device_is_compatible(np, "chrp,iic")) {
> cascade_node = np;
> @@ -93,13 +80,31 @@ static void __init mpc85xx_ds_pic_init(void)
> return;
> }
>
> - DBG("mpc85xxds: cascade mapped to irq %d\n", cascade_irq);
> + DBG("i8259: cascade mapped to irq %d\n", cascade_irq);
>
> i8259_init(cascade_node, 0);
> of_node_put(cascade_node);
>
> irq_set_chained_handler(cascade_irq, mpc85xx_8259_cascade);
> +}
> +
> #endif /* CONFIG_PPC_I8259 */
> +
> +static void __init mpc85xx_ds_pic_init(void)
> +{
> + struct mpic *mpic;
> +
> + mpic = mpic_alloc(NULL, 0,
> + MPIC_BIG_ENDIAN |
> + MPIC_SINGLE_DEST_CPU,
> + 0, 256, " OpenPIC ");
> +
> + BUG_ON(mpic == NULL);
> + mpic_init(mpic);
> +
> +#ifdef CONFIG_PPC_I8259
Ca we minimise number of #ifdef CONFIG_PPC_I8259 by using
IS_ENABLED(CONFIG_PPC_I8259) inside if/else ?
> + mpc85xx_8259_init();
> +#endif
> }
>
> #ifdef CONFIG_PCI
Le 24/12/2022 à 22:14, Pali Rohár a écrit :
> Combine machine descriptions and code of all P2020 boards into just one
> generic unified P2020 machine description. This allows kernel to boot on
> any P2020-based board with P2020 DTS file without need to patch kernel and
> define a new machine description in 85xx powerpc platform directory.
>
> Signed-off-by: Pali Rohár <[email protected]>
> ---
> arch/powerpc/platforms/85xx/p2020.c | 83 +++++++----------------------
> 1 file changed, 19 insertions(+), 64 deletions(-)
>
> diff --git a/arch/powerpc/platforms/85xx/p2020.c b/arch/powerpc/platforms/85xx/p2020.c
> index adf3750abef9..b3fb600e1d83 100644
> --- a/arch/powerpc/platforms/85xx/p2020.c
> +++ b/arch/powerpc/platforms/85xx/p2020.c
> @@ -156,83 +156,38 @@ static void __init p2020_setup_arch(void)
> #endif
> }
>
> -#ifdef CONFIG_MPC85xx_DS
> -machine_arch_initcall(p2020_ds, mpc85xx_common_publish_devices);
> -#endif /* CONFIG_MPC85xx_DS */
> -
> -#ifdef CONFIG_MPC85xx_RDB
> -machine_arch_initcall(p2020_rdb, mpc85xx_common_publish_devices);
> -machine_arch_initcall(p2020_rdb_pc, mpc85xx_common_publish_devices);
> -#endif /* CONFIG_MPC85xx_RDB */
> +machine_arch_initcall(p2020, mpc85xx_common_publish_devices);
>
> /*
> * Called very early, device-tree isn't unflattened
> */
> -#ifdef CONFIG_MPC85xx_DS
> -static int __init p2020_ds_probe(void)
> -{
> - return !!of_machine_is_compatible("fsl,P2020DS");
> -}
> -#endif /* CONFIG_MPC85xx_DS */
> -
> -#ifdef CONFIG_MPC85xx_RDB
> -static int __init p2020_rdb_probe(void)
> -{
> - if (of_machine_is_compatible("fsl,P2020RDB"))
> - return 1;
> - return 0;
> -}
> -
> -static int __init p2020_rdb_pc_probe(void)
> +static int __init p2020_probe(void)
> {
> - if (of_machine_is_compatible("fsl,P2020RDB-PC"))
> - return 1;
> - return 0;
> + struct device_node *p2020_cpu;
> +
> + /*
> + * There is no common compatible string for all P2020 boards.
> + * The only common thing is "PowerPC,P2020@0" cpu node.
> + * So check for P2020 board via this cpu node.
> + */
> + p2020_cpu = of_find_node_by_path("/cpus/PowerPC,P2020@0");
> + if (!p2020_cpu)
> + return 0;
> +
> + of_node_put(p2020_cpu);
of_node_put() already checks for nullity of its parameter, so you can
simplify stuff here, something like
p2020_cpu = of_find_node_by_path("/cpus/PowerPC,P2020@0");
of_node_put(p2020_cpu);
return !!p2020_cpu;
> + return 1;
> }
> -#endif /* CONFIG_MPC85xx_RDB */
> -
> -#ifdef CONFIG_MPC85xx_DS
> -define_machine(p2020_ds) {
> - .name = "P2020 DS",
> - .probe = p2020_ds_probe,
> - .setup_arch = p2020_setup_arch,
> - .init_IRQ = p2020_pic_init,
> -#ifdef CONFIG_PCI
> - .pcibios_fixup_bus = fsl_pcibios_fixup_bus,
> - .pcibios_fixup_phb = fsl_pcibios_fixup_phb,
> -#endif
> - .get_irq = mpic_get_irq,
> - .calibrate_decr = generic_calibrate_decr,
> - .progress = udbg_progress,
> -};
> -#endif /* CONFIG_MPC85xx_DS */
> -
> -#ifdef CONFIG_MPC85xx_RDB
> -define_machine(p2020_rdb) {
> - .name = "P2020 RDB",
> - .probe = p2020_rdb_probe,
> - .setup_arch = p2020_setup_arch,
> - .init_IRQ = p2020_pic_init,
> -#ifdef CONFIG_PCI
> - .pcibios_fixup_bus = fsl_pcibios_fixup_bus,
> - .pcibios_fixup_phb = fsl_pcibios_fixup_phb,
> -#endif
> - .get_irq = mpic_get_irq,
> - .calibrate_decr = generic_calibrate_decr,
> - .progress = udbg_progress,
> -};
>
> -define_machine(p2020_rdb_pc) {
> - .name = "P2020RDB-PC",
> - .probe = p2020_rdb_pc_probe,
> +define_machine(p2020) {
> + .name = "Freescale P2020",
> + .probe = p2020_probe,
> .setup_arch = p2020_setup_arch,
> .init_IRQ = p2020_pic_init,
> #ifdef CONFIG_PCI
> .pcibios_fixup_bus = fsl_pcibios_fixup_bus,
> - .pcibios_fixup_phb = fsl_pcibios_fixup_phb,
> + .pcibios_fixup_phb = fsl_pcibios_fixup_phb,
> #endif
> .get_irq = mpic_get_irq,
> .calibrate_decr = generic_calibrate_decr,
> .progress = udbg_progress,
> };
> -#endif /* CONFIG_MPC85xx_RDB */
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.
On Monday 13 February 2023 20:06:27 Christophe Leroy wrote:
>
>
> Le 24/12/2022 à 22:14, Pali Rohár a écrit :
> > Splits mpic and i8259 initialization codes into separate functions.
> >
> > Signed-off-by: Pali Rohár <[email protected]>
> > ---
> > arch/powerpc/platforms/85xx/p2020.c | 37 ++++++++++++++++-------------
> > 1 file changed, 21 insertions(+), 16 deletions(-)
> >
> > diff --git a/arch/powerpc/platforms/85xx/p2020.c b/arch/powerpc/platforms/85xx/p2020.c
> > index d65d4c88ac47..b8584bf307b0 100644
> > --- a/arch/powerpc/platforms/85xx/p2020.c
> > +++ b/arch/powerpc/platforms/85xx/p2020.c
> > @@ -45,6 +45,7 @@
> > #ifdef CONFIG_MPC85xx_DS
> >
> > #ifdef CONFIG_PPC_I8259
> > +
> > static void mpc85xx_8259_cascade(struct irq_desc *desc)
> > {
> > struct irq_chip *chip = irq_desc_get_chip(desc);
> > @@ -55,27 +56,13 @@ static void mpc85xx_8259_cascade(struct irq_desc *desc)
> > }
> > chip->irq_eoi(&desc->irq_data);
> > }
> > -#endif /* CONFIG_PPC_I8259 */
> >
> > -static void __init mpc85xx_ds_pic_init(void)
> > +static void __init mpc85xx_8259_init(void)
> > {
> > - struct mpic *mpic;
> > -#ifdef CONFIG_PPC_I8259
> > struct device_node *np;
> > struct device_node *cascade_node = NULL;
> > int cascade_irq;
> > -#endif
> > -
> > - mpic = mpic_alloc(NULL, 0,
> > - MPIC_BIG_ENDIAN |
> > - MPIC_SINGLE_DEST_CPU,
> > - 0, 256, " OpenPIC ");
> > -
> > - BUG_ON(mpic == NULL);
> > - mpic_init(mpic);
> >
> > -#ifdef CONFIG_PPC_I8259
> > - /* Initialize the i8259 controller */
> > for_each_node_by_type(np, "interrupt-controller")
> > if (of_device_is_compatible(np, "chrp,iic")) {
> > cascade_node = np;
> > @@ -93,13 +80,31 @@ static void __init mpc85xx_ds_pic_init(void)
> > return;
> > }
> >
> > - DBG("mpc85xxds: cascade mapped to irq %d\n", cascade_irq);
> > + DBG("i8259: cascade mapped to irq %d\n", cascade_irq);
> >
> > i8259_init(cascade_node, 0);
> > of_node_put(cascade_node);
> >
> > irq_set_chained_handler(cascade_irq, mpc85xx_8259_cascade);
> > +}
> > +
> > #endif /* CONFIG_PPC_I8259 */
> > +
> > +static void __init mpc85xx_ds_pic_init(void)
> > +{
> > + struct mpic *mpic;
> > +
> > + mpic = mpic_alloc(NULL, 0,
> > + MPIC_BIG_ENDIAN |
> > + MPIC_SINGLE_DEST_CPU,
> > + 0, 256, " OpenPIC ");
> > +
> > + BUG_ON(mpic == NULL);
> > + mpic_init(mpic);
> > +
> > +#ifdef CONFIG_PPC_I8259
>
> Ca we minimise number of #ifdef CONFIG_PPC_I8259 by using
> IS_ENABLED(CONFIG_PPC_I8259) inside if/else ?
>
> > + mpc85xx_8259_init();
> > +#endif
Ok, I can change code to:
+if (IS_ENABLED(CONFIG_PPC_I8259))
+ mpc85xx_8259_init();
I guess it should be equivalent.
> > }
> >
> > #ifdef CONFIG_PCI
On Monday 13 February 2023 20:08:52 Christophe Leroy wrote:
> Le 24/12/2022 à 22:14, Pali Rohár a écrit :
> > Combine machine descriptions and code of all P2020 boards into just one
> > generic unified P2020 machine description. This allows kernel to boot on
> > any P2020-based board with P2020 DTS file without need to patch kernel and
> > define a new machine description in 85xx powerpc platform directory.
> >
> > Signed-off-by: Pali Rohár <[email protected]>
> > ---
> > arch/powerpc/platforms/85xx/p2020.c | 83 +++++++----------------------
> > 1 file changed, 19 insertions(+), 64 deletions(-)
> >
> > diff --git a/arch/powerpc/platforms/85xx/p2020.c b/arch/powerpc/platforms/85xx/p2020.c
> > index adf3750abef9..b3fb600e1d83 100644
> > --- a/arch/powerpc/platforms/85xx/p2020.c
> > +++ b/arch/powerpc/platforms/85xx/p2020.c
> > @@ -156,83 +156,38 @@ static void __init p2020_setup_arch(void)
> > #endif
> > }
> >
> > -#ifdef CONFIG_MPC85xx_DS
> > -machine_arch_initcall(p2020_ds, mpc85xx_common_publish_devices);
> > -#endif /* CONFIG_MPC85xx_DS */
> > -
> > -#ifdef CONFIG_MPC85xx_RDB
> > -machine_arch_initcall(p2020_rdb, mpc85xx_common_publish_devices);
> > -machine_arch_initcall(p2020_rdb_pc, mpc85xx_common_publish_devices);
> > -#endif /* CONFIG_MPC85xx_RDB */
> > +machine_arch_initcall(p2020, mpc85xx_common_publish_devices);
> >
> > /*
> > * Called very early, device-tree isn't unflattened
> > */
> > -#ifdef CONFIG_MPC85xx_DS
> > -static int __init p2020_ds_probe(void)
> > -{
> > - return !!of_machine_is_compatible("fsl,P2020DS");
> > -}
> > -#endif /* CONFIG_MPC85xx_DS */
> > -
> > -#ifdef CONFIG_MPC85xx_RDB
> > -static int __init p2020_rdb_probe(void)
> > -{
> > - if (of_machine_is_compatible("fsl,P2020RDB"))
> > - return 1;
> > - return 0;
> > -}
> > -
> > -static int __init p2020_rdb_pc_probe(void)
> > +static int __init p2020_probe(void)
> > {
> > - if (of_machine_is_compatible("fsl,P2020RDB-PC"))
> > - return 1;
> > - return 0;
> > + struct device_node *p2020_cpu;
> > +
> > + /*
> > + * There is no common compatible string for all P2020 boards.
> > + * The only common thing is "PowerPC,P2020@0" cpu node.
> > + * So check for P2020 board via this cpu node.
> > + */
> > + p2020_cpu = of_find_node_by_path("/cpus/PowerPC,P2020@0");
> > + if (!p2020_cpu)
> > + return 0;
> > +
> > + of_node_put(p2020_cpu);
>
> of_node_put() already checks for nullity of its parameter, so you can
> simplify stuff here, something like
>
> p2020_cpu = of_find_node_by_path("/cpus/PowerPC,P2020@0");
> of_node_put(p2020_cpu);
>
> return !!p2020_cpu;
Ok!
> > + return 1;
> > }
> > -#endif /* CONFIG_MPC85xx_RDB */
> > -
> > -#ifdef CONFIG_MPC85xx_DS
> > -define_machine(p2020_ds) {
> > - .name = "P2020 DS",
> > - .probe = p2020_ds_probe,
> > - .setup_arch = p2020_setup_arch,
> > - .init_IRQ = p2020_pic_init,
> > -#ifdef CONFIG_PCI
> > - .pcibios_fixup_bus = fsl_pcibios_fixup_bus,
> > - .pcibios_fixup_phb = fsl_pcibios_fixup_phb,
> > -#endif
> > - .get_irq = mpic_get_irq,
> > - .calibrate_decr = generic_calibrate_decr,
> > - .progress = udbg_progress,
> > -};
> > -#endif /* CONFIG_MPC85xx_DS */
> > -
> > -#ifdef CONFIG_MPC85xx_RDB
> > -define_machine(p2020_rdb) {
> > - .name = "P2020 RDB",
> > - .probe = p2020_rdb_probe,
> > - .setup_arch = p2020_setup_arch,
> > - .init_IRQ = p2020_pic_init,
> > -#ifdef CONFIG_PCI
> > - .pcibios_fixup_bus = fsl_pcibios_fixup_bus,
> > - .pcibios_fixup_phb = fsl_pcibios_fixup_phb,
> > -#endif
> > - .get_irq = mpic_get_irq,
> > - .calibrate_decr = generic_calibrate_decr,
> > - .progress = udbg_progress,
> > -};
> >
> > -define_machine(p2020_rdb_pc) {
> > - .name = "P2020RDB-PC",
> > - .probe = p2020_rdb_pc_probe,
> > +define_machine(p2020) {
> > + .name = "Freescale P2020",
> > + .probe = p2020_probe,
> > .setup_arch = p2020_setup_arch,
> > .init_IRQ = p2020_pic_init,
> > #ifdef CONFIG_PCI
> > .pcibios_fixup_bus = fsl_pcibios_fixup_bus,
> > - .pcibios_fixup_phb = fsl_pcibios_fixup_phb,
> > + .pcibios_fixup_phb = fsl_pcibios_fixup_phb,
> > #endif
> > .get_irq = mpic_get_irq,
> > .calibrate_decr = generic_calibrate_decr,
> > .progress = udbg_progress,
> > };
> > -#endif /* CONFIG_MPC85xx_RDB */
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.
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.