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

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

[Pali Rohár]

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

[PATCH v2 1/8] powerpc/85xx: Mark mpc85xx_rdb_pic_init() as static

[Pali Rohár]

Function mpc85xx_rdb_pic_init() is not used out of the mpc85xx_rdb.c file.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 arch/powerpc/platforms/85xx/mpc85xx_rdb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/85xx/mpc85xx_rdb.c b/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
index d99aba158235..b6129c148fea 100644
--- a/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
+++ b/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
@@ -38,7 +38,7 @@
 #endif
 
 
-void __init mpc85xx_rdb_pic_init(void)
+static void __init mpc85xx_rdb_pic_init(void)
 {
 	struct mpic *mpic;
 
-- 
2.20.1

[PATCH v2 2/8] powerpc/85xx: Mark mpc85xx_ds_pic_init() as static

[Pali Rohár]

Function mpc85xx_ds_pic_init() is not used out of the mpc85xx_ds.c file.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 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

[PATCH v2 3/8] powerpc/85xx: p2020: Move all P2020 machine descriptions to p2020.c

[Pali Rohár]

This moves machine descriptions and all related code for all P2020 boards
into new p2020.c source file. This change also copies helper static
functions from other mpc85xx*.c files into p2020.c, which are required for
machine descriptions. This is preparation for code de-duplication and
providing one unified machine description for all P2020 boards. In
follow-up patches would be copied functions refactored and simplified to be
specific just for P2020 boards.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 arch/powerpc/platforms/85xx/Makefile      |   2 +
 arch/powerpc/platforms/85xx/mpc85xx_ds.c  |  23 --
 arch/powerpc/platforms/85xx/mpc85xx_rdb.c |  44 ----
 arch/powerpc/platforms/85xx/p2020.c       | 273 ++++++++++++++++++++++
 4 files changed, 275 insertions(+), 67 deletions(-)
 create mode 100644 arch/powerpc/platforms/85xx/p2020.c

diff --git a/arch/powerpc/platforms/85xx/Makefile b/arch/powerpc/platforms/85xx/Makefile
index 260fbad7967b..1ad261b4eeb6 100644
--- a/arch/powerpc/platforms/85xx/Makefile
+++ b/arch/powerpc/platforms/85xx/Makefile
@@ -23,6 +23,8 @@ obj-$(CONFIG_P1010_RDB)   += p1010rdb.o
 obj-$(CONFIG_P1022_DS)    += p1022_ds.o
 obj-$(CONFIG_P1022_RDK)   += p1022_rdk.o
 obj-$(CONFIG_P1023_RDB)   += p1023_rdb.o
+obj-$(CONFIG_MPC85xx_DS)  += p2020.o
+obj-$(CONFIG_MPC85xx_RDB) += p2020.o
 obj-$(CONFIG_TWR_P102x)   += twr_p102x.o
 obj-$(CONFIG_CORENET_GENERIC)   += corenet_generic.o
 obj-$(CONFIG_FB_FSL_DIU)	+= t1042rdb_diu.o
diff --git a/arch/powerpc/platforms/85xx/mpc85xx_ds.c b/arch/powerpc/platforms/85xx/mpc85xx_ds.c
index 9a6d637ef54a..05aac997b5ed 100644
--- a/arch/powerpc/platforms/85xx/mpc85xx_ds.c
+++ b/arch/powerpc/platforms/85xx/mpc85xx_ds.c
@@ -168,7 +168,6 @@ static int __init mpc8544_ds_probe(void)
 
 machine_arch_initcall(mpc8544_ds, mpc85xx_common_publish_devices);
 machine_arch_initcall(mpc8572_ds, mpc85xx_common_publish_devices);
-machine_arch_initcall(p2020_ds, mpc85xx_common_publish_devices);
 
 /*
  * Called very early, device-tree isn't unflattened
@@ -178,14 +177,6 @@ static int __init mpc8572_ds_probe(void)
 	return !!of_machine_is_compatible("fsl,MPC8572DS");
 }
 
-/*
- * Called very early, device-tree isn't unflattened
- */
-static int __init p2020_ds_probe(void)
-{
-	return !!of_machine_is_compatible("fsl,P2020DS");
-}
-
 define_machine(mpc8544_ds) {
 	.name			= "MPC8544 DS",
 	.probe			= mpc8544_ds_probe,
@@ -213,17 +204,3 @@ define_machine(mpc8572_ds) {
 	.calibrate_decr		= generic_calibrate_decr,
 	.progress		= udbg_progress,
 };
-
-define_machine(p2020_ds) {
-	.name			= "P2020 DS",
-	.probe			= p2020_ds_probe,
-	.setup_arch		= mpc85xx_ds_setup_arch,
-	.init_IRQ		= mpc85xx_ds_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,
-};
diff --git a/arch/powerpc/platforms/85xx/mpc85xx_rdb.c b/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
index b6129c148fea..05f1ed635735 100644
--- a/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
+++ b/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
@@ -108,8 +108,6 @@ static void __init mpc85xx_rdb_setup_arch(void)
 	printk(KERN_INFO "MPC85xx RDB board from Freescale Semiconductor\n");
 }
 
-machine_arch_initcall(p2020_rdb, mpc85xx_common_publish_devices);
-machine_arch_initcall(p2020_rdb_pc, mpc85xx_common_publish_devices);
 machine_arch_initcall(p1020_mbg_pc, mpc85xx_common_publish_devices);
 machine_arch_initcall(p1020_rdb, mpc85xx_common_publish_devices);
 machine_arch_initcall(p1020_rdb_pc, mpc85xx_common_publish_devices);
@@ -122,13 +120,6 @@ machine_arch_initcall(p1024_rdb, mpc85xx_common_publish_devices);
 /*
  * Called very early, device-tree isn't unflattened
  */
-static int __init p2020_rdb_probe(void)
-{
-	if (of_machine_is_compatible("fsl,P2020RDB"))
-		return 1;
-	return 0;
-}
-
 static int __init p1020_rdb_probe(void)
 {
 	if (of_machine_is_compatible("fsl,P1020RDB"))
@@ -153,13 +144,6 @@ static int __init p1021_rdb_pc_probe(void)
 	return 0;
 }
 
-static int __init p2020_rdb_pc_probe(void)
-{
-	if (of_machine_is_compatible("fsl,P2020RDB-PC"))
-		return 1;
-	return 0;
-}
-
 static int __init p1025_rdb_probe(void)
 {
 	return of_machine_is_compatible("fsl,P1025RDB");
@@ -180,20 +164,6 @@ static int __init p1024_rdb_probe(void)
 	return of_machine_is_compatible("fsl,P1024RDB");
 }
 
-define_machine(p2020_rdb) {
-	.name			= "P2020 RDB",
-	.probe			= p2020_rdb_probe,
-	.setup_arch		= mpc85xx_rdb_setup_arch,
-	.init_IRQ		= mpc85xx_rdb_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(p1020_rdb) {
 	.name			= "P1020 RDB",
 	.probe			= p1020_rdb_probe,
@@ -222,20 +192,6 @@ define_machine(p1021_rdb_pc) {
 	.progress		= udbg_progress,
 };
 
-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,
-#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(p1025_rdb) {
 	.name			= "P1025 RDB",
 	.probe			= p1025_rdb_probe,
diff --git a/arch/powerpc/platforms/85xx/p2020.c b/arch/powerpc/platforms/85xx/p2020.c
new file mode 100644
index 000000000000..d65d4c88ac47
--- /dev/null
+++ b/arch/powerpc/platforms/85xx/p2020.c
@@ -0,0 +1,273 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Freescale P2020 board Setup
+ *
+ * Copyright 2007,2009,2012-2013 Freescale Semiconductor Inc.
+ * Copyright 2022 Pali Rohár <pali@kernel.org>
+ */
+
+#include <linux/stddef.h>
+#include <linux/kernel.h>
+#include <linux/pci.h>
+#include <linux/kdev_t.h>
+#include <linux/delay.h>
+#include <linux/seq_file.h>
+#include <linux/interrupt.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/fsl/guts.h>
+
+#include <asm/time.h>
+#include <asm/machdep.h>
+#include <asm/pci-bridge.h>
+#include <mm/mmu_decl.h>
+#include <asm/udbg.h>
+#include <asm/mpic.h>
+#include <asm/i8259.h>
+#include <asm/swiotlb.h>
+
+#include <soc/fsl/qe/qe.h>
+
+#include <sysdev/fsl_soc.h>
+#include <sysdev/fsl_pci.h>
+#include "smp.h"
+
+#include "mpc85xx.h"
+
+#undef DEBUG
+
+#ifdef DEBUG
+#define DBG(fmt, args...) printk(KERN_ERR "%s: " fmt, __func__, ## args)
+#else
+#define DBG(fmt, args...)
+#endif
+
+#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);
+	unsigned int cascade_irq = i8259_irq();
+
+	if (cascade_irq) {
+		generic_handle_irq(cascade_irq);
+	}
+	chip->irq_eoi(&desc->irq_data);
+}
+#endif	/* CONFIG_PPC_I8259 */
+
+static void __init mpc85xx_ds_pic_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;
+		break;
+	}
+
+	if (cascade_node == NULL) {
+		printk(KERN_DEBUG "Could not find i8259 PIC\n");
+		return;
+	}
+
+	cascade_irq = irq_of_parse_and_map(cascade_node, 0);
+	if (!cascade_irq) {
+		printk(KERN_ERR "Failed to map cascade interrupt\n");
+		return;
+	}
+
+	DBG("mpc85xxds: 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 */
+}
+
+#ifdef CONFIG_PCI
+extern int uli_exclude_device(struct pci_controller *hose,
+				u_char bus, u_char devfn);
+
+static struct device_node *pci_with_uli;
+
+static int mpc85xx_exclude_device(struct pci_controller *hose,
+				   u_char bus, u_char devfn)
+{
+	if (hose->dn == pci_with_uli)
+		return uli_exclude_device(hose, bus, devfn);
+
+	return PCIBIOS_SUCCESSFUL;
+}
+#endif	/* CONFIG_PCI */
+
+static void __init mpc85xx_ds_uli_init(void)
+{
+#ifdef CONFIG_PCI
+	struct device_node *node;
+
+	/* See if we have a ULI under the primary */
+
+	node = of_find_node_by_name(NULL, "uli1575");
+	while ((pci_with_uli = of_get_parent(node))) {
+		of_node_put(node);
+		node = pci_with_uli;
+
+		if (pci_with_uli == fsl_pci_primary) {
+			ppc_md.pci_exclude_device = mpc85xx_exclude_device;
+			break;
+		}
+	}
+#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)
+{
+	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 /* CONFIG_MPC85xx_RDB */
+
+#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 */
+
+/*
+ * 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)
+{
+	if (of_machine_is_compatible("fsl,P2020RDB-PC"))
+		return 1;
+	return 0;
+}
+#endif /* CONFIG_MPC85xx_RDB */
+
+#ifdef CONFIG_MPC85xx_DS
+define_machine(p2020_ds) {
+	.name			= "P2020 DS",
+	.probe			= p2020_ds_probe,
+	.setup_arch		= mpc85xx_ds_setup_arch,
+	.init_IRQ		= mpc85xx_ds_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		= mpc85xx_rdb_setup_arch,
+	.init_IRQ		= mpc85xx_rdb_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,
+	.setup_arch		= mpc85xx_rdb_setup_arch,
+	.init_IRQ		= mpc85xx_rdb_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_RDB */
-- 
2.20.1

[PATCH v2 4/8] powerpc/85xx: p2020: Move i8259 code into own function

[Pali Rohár]

Splits mpic and i8259 initialization codes into separate functions.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 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

[PATCH v2 5/8] powerpc/85xx: p2020: Unify .setup_arch and .init_IRQ callbacks

[Pali Rohár]

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 <pali@kernel.org>
---
 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

[PATCH v2 6/8] powerpc/85xx: p2020: Define just one machine description

[Pali Rohár]

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 <pali@kernel.org>
---
 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

[PATCH v2 7/8] powerpc/85xx: p2020: Enable boards by new config option CONFIG_PPC_P2020

[Pali Rohár]

Generic unified P2020 machine description which supports all P2020-based
boards is now in separate file p2020.c. So create a separate config option
CONFIG_PPC_P2020 for it.

Previously machine descriptions for P2020 boards were enabled by
CONFIG_MPC85xx_DS or CONFIG_MPC85xx_RDB option. So set CONFIG_PPC_P2020 to
be enabled by default when one of those option is enabled.

This allows to compile support for P2020 boards without need to have
enabled support for older mpc85xx boards. And to compile kernel for old
mpc85xx boards without having enabled support for new P2020 boards.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 arch/powerpc/platforms/85xx/Kconfig  | 22 ++++++++++++++++++----
 arch/powerpc/platforms/85xx/Makefile |  3 +--
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/platforms/85xx/Kconfig b/arch/powerpc/platforms/85xx/Kconfig
index b92cb2b4d54d..90665882143b 100644
--- a/arch/powerpc/platforms/85xx/Kconfig
+++ b/arch/powerpc/platforms/85xx/Kconfig
@@ -78,16 +78,16 @@ config MPC8536_DS
 	  This option enables support for the MPC8536 DS board
 
 config MPC85xx_DS
-	bool "Freescale MPC8544 DS / MPC8572 DS / P2020 DS"
+	bool "Freescale MPC8544 DS / MPC8572 DS"
 	select PPC_I8259
 	select DEFAULT_UIMAGE
 	select FSL_ULI1575 if PCI
 	select SWIOTLB
 	help
-	  This option enables support for the MPC8544 DS, MPC8572 DS and P2020 DS boards
+	  This option enables support for the MPC8544 DS and MPC8572 DS boards
 
 config MPC85xx_RDB
-	bool "Freescale P102x MBG/UTM/RDB and P2020 RDB"
+	bool "Freescale P102x MBG/UTM/RDB"
 	select PPC_I8259
 	select DEFAULT_UIMAGE
 	select FSL_ULI1575 if PCI
@@ -95,7 +95,21 @@ config MPC85xx_RDB
 	help
 	  This option enables support for the P1020 MBG PC, P1020 UTM PC,
 	  P1020 RDB PC, P1020 RDB PD, P1020 RDB, P1021 RDB PC, P1024 RDB,
-	  P1025 RDB, P2020 RDB and P2020 RDB PC boards
+	  and P1025 RDB boards
+
+config PPC_P2020
+	bool "Freescale P2020"
+	default y if MPC85xx_DS || MPC85xx_RDB
+	select DEFAULT_UIMAGE
+	select SWIOTLB
+	imply PPC_I8259
+	imply FSL_ULI1575 if PCI
+	help
+	  This option enables generic unified support for any board with the
+	  Freescale P2020 processor.
+
+	  For example: P2020 DS board, P2020 RDB board, P2020 RDB PC board or
+	  CZ.NIC Turris 1.x boards.
 
 config P1010_RDB
 	bool "Freescale P1010 RDB"
diff --git a/arch/powerpc/platforms/85xx/Makefile b/arch/powerpc/platforms/85xx/Makefile
index 1ad261b4eeb6..76ee691d29b5 100644
--- a/arch/powerpc/platforms/85xx/Makefile
+++ b/arch/powerpc/platforms/85xx/Makefile
@@ -23,8 +23,7 @@ obj-$(CONFIG_P1010_RDB)   += p1010rdb.o
 obj-$(CONFIG_P1022_DS)    += p1022_ds.o
 obj-$(CONFIG_P1022_RDK)   += p1022_rdk.o
 obj-$(CONFIG_P1023_RDB)   += p1023_rdb.o
-obj-$(CONFIG_MPC85xx_DS)  += p2020.o
-obj-$(CONFIG_MPC85xx_RDB) += p2020.o
+obj-$(CONFIG_PPC_P2020)   += p2020.o
 obj-$(CONFIG_TWR_P102x)   += twr_p102x.o
 obj-$(CONFIG_CORENET_GENERIC)   += corenet_generic.o
 obj-$(CONFIG_FB_FSL_DIU)	+= t1042rdb_diu.o
-- 
2.20.1

[PATCH v2 8/8] powerpc: dts: turris1x.dts: Remove "fsl,P2020RDB-PC" compatible string

[Pali Rohár]

"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 <pali@kernel.org>
---
 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

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

[Pali Rohár]

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

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

[Christophe Leroy]

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

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

> 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@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 3/8] powerpc/85xx: p2020: Move all P2020 machine descriptions to p2020.c

[Pali Rohár]

On Saturday 24 December 2022 22:14:20 Pali Rohár wrote:
> This moves machine descriptions and all related code for all P2020 boards
> into new p2020.c source file. This change also copies helper static
> functions from other mpc85xx*.c files into p2020.c, which are required for
> machine descriptions. This is preparation for code de-duplication and
> providing one unified machine description for all P2020 boards. In
> follow-up patches would be copied functions refactored and simplified to be
> specific just for P2020 boards.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  arch/powerpc/platforms/85xx/Makefile      |   2 +
>  arch/powerpc/platforms/85xx/mpc85xx_ds.c  |  23 --
>  arch/powerpc/platforms/85xx/mpc85xx_rdb.c |  44 ----
>  arch/powerpc/platforms/85xx/p2020.c       | 273 ++++++++++++++++++++++
>  4 files changed, 275 insertions(+), 67 deletions(-)
>  create mode 100644 arch/powerpc/platforms/85xx/p2020.c

Here is same patch, but generated by git -M and -C options. Maybe it is more readable?

 arch/powerpc/platforms/85xx/Makefile               |   2 +
 arch/powerpc/platforms/85xx/mpc85xx_ds.c           |  23 ----
 arch/powerpc/platforms/85xx/mpc85xx_rdb.c          |  44 -------
 .../platforms/85xx/{mpc85xx_ds.c => p2020.c}       | 134 ++++++++++++++-------
 4 files changed, 91 insertions(+), 112 deletions(-)

diff --git a/arch/powerpc/platforms/85xx/Makefile b/arch/powerpc/platforms/85xx/Makefile
index 260fbad7967b..1ad261b4eeb6 100644
--- a/arch/powerpc/platforms/85xx/Makefile
+++ b/arch/powerpc/platforms/85xx/Makefile
@@ -23,6 +23,8 @@ obj-$(CONFIG_P1010_RDB)   += p1010rdb.o
 obj-$(CONFIG_P1022_DS)    += p1022_ds.o
 obj-$(CONFIG_P1022_RDK)   += p1022_rdk.o
 obj-$(CONFIG_P1023_RDB)   += p1023_rdb.o
+obj-$(CONFIG_MPC85xx_DS)  += p2020.o
+obj-$(CONFIG_MPC85xx_RDB) += p2020.o
 obj-$(CONFIG_TWR_P102x)   += twr_p102x.o
 obj-$(CONFIG_CORENET_GENERIC)   += corenet_generic.o
 obj-$(CONFIG_FB_FSL_DIU)	+= t1042rdb_diu.o
diff --git a/arch/powerpc/platforms/85xx/mpc85xx_ds.c b/arch/powerpc/platforms/85xx/mpc85xx_ds.c
index 9a6d637ef54a..05aac997b5ed 100644
--- a/arch/powerpc/platforms/85xx/mpc85xx_ds.c
+++ b/arch/powerpc/platforms/85xx/mpc85xx_ds.c
@@ -168,7 +168,6 @@ static int __init mpc8544_ds_probe(void)
 
 machine_arch_initcall(mpc8544_ds, mpc85xx_common_publish_devices);
 machine_arch_initcall(mpc8572_ds, mpc85xx_common_publish_devices);
-machine_arch_initcall(p2020_ds, mpc85xx_common_publish_devices);
 
 /*
  * Called very early, device-tree isn't unflattened
@@ -178,14 +177,6 @@ static int __init mpc8572_ds_probe(void)
 	return !!of_machine_is_compatible("fsl,MPC8572DS");
 }
 
-/*
- * Called very early, device-tree isn't unflattened
- */
-static int __init p2020_ds_probe(void)
-{
-	return !!of_machine_is_compatible("fsl,P2020DS");
-}
-
 define_machine(mpc8544_ds) {
 	.name			= "MPC8544 DS",
 	.probe			= mpc8544_ds_probe,
@@ -213,17 +204,3 @@ define_machine(mpc8572_ds) {
 	.calibrate_decr		= generic_calibrate_decr,
 	.progress		= udbg_progress,
 };
-
-define_machine(p2020_ds) {
-	.name			= "P2020 DS",
-	.probe			= p2020_ds_probe,
-	.setup_arch		= mpc85xx_ds_setup_arch,
-	.init_IRQ		= mpc85xx_ds_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,
-};
diff --git a/arch/powerpc/platforms/85xx/mpc85xx_rdb.c b/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
index b6129c148fea..05f1ed635735 100644
--- a/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
+++ b/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
@@ -108,8 +108,6 @@ static void __init mpc85xx_rdb_setup_arch(void)
 	printk(KERN_INFO "MPC85xx RDB board from Freescale Semiconductor\n");
 }
 
-machine_arch_initcall(p2020_rdb, mpc85xx_common_publish_devices);
-machine_arch_initcall(p2020_rdb_pc, mpc85xx_common_publish_devices);
 machine_arch_initcall(p1020_mbg_pc, mpc85xx_common_publish_devices);
 machine_arch_initcall(p1020_rdb, mpc85xx_common_publish_devices);
 machine_arch_initcall(p1020_rdb_pc, mpc85xx_common_publish_devices);
@@ -122,13 +120,6 @@ machine_arch_initcall(p1024_rdb, mpc85xx_common_publish_devices);
 /*
  * Called very early, device-tree isn't unflattened
  */
-static int __init p2020_rdb_probe(void)
-{
-	if (of_machine_is_compatible("fsl,P2020RDB"))
-		return 1;
-	return 0;
-}
-
 static int __init p1020_rdb_probe(void)
 {
 	if (of_machine_is_compatible("fsl,P1020RDB"))
@@ -153,13 +144,6 @@ static int __init p1021_rdb_pc_probe(void)
 	return 0;
 }
 
-static int __init p2020_rdb_pc_probe(void)
-{
-	if (of_machine_is_compatible("fsl,P2020RDB-PC"))
-		return 1;
-	return 0;
-}
-
 static int __init p1025_rdb_probe(void)
 {
 	return of_machine_is_compatible("fsl,P1025RDB");
@@ -180,20 +164,6 @@ static int __init p1024_rdb_probe(void)
 	return of_machine_is_compatible("fsl,P1024RDB");
 }
 
-define_machine(p2020_rdb) {
-	.name			= "P2020 RDB",
-	.probe			= p2020_rdb_probe,
-	.setup_arch		= mpc85xx_rdb_setup_arch,
-	.init_IRQ		= mpc85xx_rdb_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(p1020_rdb) {
 	.name			= "P1020 RDB",
 	.probe			= p1020_rdb_probe,
@@ -222,20 +192,6 @@ define_machine(p1021_rdb_pc) {
 	.progress		= udbg_progress,
 };
 
-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,
-#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(p1025_rdb) {
 	.name			= "P1025 RDB",
 	.probe			= p1025_rdb_probe,
diff --git a/arch/powerpc/platforms/85xx/mpc85xx_ds.c b/arch/powerpc/platforms/85xx/p2020.c
similarity index 65%
copy from arch/powerpc/platforms/85xx/mpc85xx_ds.c
copy to arch/powerpc/platforms/85xx/p2020.c
index 9a6d637ef54a..d65d4c88ac47 100644
--- a/arch/powerpc/platforms/85xx/mpc85xx_ds.c
+++ b/arch/powerpc/platforms/85xx/p2020.c
@@ -1,11 +1,9 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 /*
- * MPC85xx DS Board Setup
+ * Freescale P2020 board Setup
  *
- * Author Xianghua Xiao (x.xiao@freescale.com)
- * Roy Zang <tie-fei.zang@freescale.com>
- * 	- Add PCI/PCI Exprees support
- * Copyright 2007 Freescale Semiconductor Inc.
+ * Copyright 2007,2009,2012-2013 Freescale Semiconductor Inc.
+ * Copyright 2022 Pali Rohár <pali@kernel.org>
  */
 
 #include <linux/stddef.h>
@@ -17,6 +15,7 @@
 #include <linux/interrupt.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
+#include <linux/fsl/guts.h>
 
 #include <asm/time.h>
 #include <asm/machdep.h>
@@ -27,6 +26,8 @@
 #include <asm/i8259.h>
 #include <asm/swiotlb.h>
 
+#include <soc/fsl/qe/qe.h>
+
 #include <sysdev/fsl_soc.h>
 #include <sysdev/fsl_pci.h>
 #include "smp.h"
@@ -41,6 +42,8 @@
 #define DBG(fmt, args...)
 #endif
 
+#ifdef CONFIG_MPC85xx_DS
+
 #ifdef CONFIG_PPC_I8259
 static void mpc85xx_8259_cascade(struct irq_desc *desc)
 {
@@ -62,18 +65,11 @@ static void __init mpc85xx_ds_pic_init(void)
 	struct device_node *cascade_node = NULL;
 	int cascade_irq;
 #endif
-	if (of_machine_is_compatible("fsl,MPC8572DS-CAMP")) {
-		mpic = mpic_alloc(NULL, 0,
-			MPIC_NO_RESET |
-			MPIC_BIG_ENDIAN |
-			MPIC_SINGLE_DEST_CPU,
-			0, 256, " OpenPIC  ");
-	} else {
-		mpic = mpic_alloc(NULL, 0,
-			  MPIC_BIG_ENDIAN |
-			  MPIC_SINGLE_DEST_CPU,
-			0, 256, " OpenPIC  ");
-	}
+
+	mpic = mpic_alloc(NULL, 0,
+		  MPIC_BIG_ENDIAN |
+		  MPIC_SINGLE_DEST_CPU,
+		0, 256, " OpenPIC  ");
 
 	BUG_ON(mpic == NULL);
 	mpic_init(mpic);
@@ -142,9 +138,27 @@ 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)
 {
 	if (ppc_md.progress)
@@ -157,38 +171,65 @@ static void __init mpc85xx_ds_setup_arch(void)
 
 	printk("MPC85xx DS board from Freescale Semiconductor\n");
 }
+#endif /* CONFIG_MPC85xx_DS */
 
-/*
- * Called very early, device-tree isn't unflattened
- */
-static int __init mpc8544_ds_probe(void)
+#ifdef CONFIG_MPC85xx_RDB
+static void __init mpc85xx_rdb_setup_arch(void)
 {
-	return !!of_machine_is_compatible("MPC8544DS");
+	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 /* CONFIG_MPC85xx_RDB */
 
-machine_arch_initcall(mpc8544_ds, mpc85xx_common_publish_devices);
-machine_arch_initcall(mpc8572_ds, mpc85xx_common_publish_devices);
+#ifdef CONFIG_MPC85xx_DS
 machine_arch_initcall(p2020_ds, mpc85xx_common_publish_devices);
+#endif /* CONFIG_MPC85xx_DS */
 
-/*
- * Called very early, device-tree isn't unflattened
- */
-static int __init mpc8572_ds_probe(void)
-{
-	return !!of_machine_is_compatible("fsl,MPC8572DS");
-}
+#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 */
 
 /*
  * 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)
+{
+	if (of_machine_is_compatible("fsl,P2020RDB-PC"))
+		return 1;
+	return 0;
+}
+#endif /* CONFIG_MPC85xx_RDB */
 
-define_machine(mpc8544_ds) {
-	.name			= "MPC8544 DS",
-	.probe			= mpc8544_ds_probe,
+#ifdef CONFIG_MPC85xx_DS
+define_machine(p2020_ds) {
+	.name			= "P2020 DS",
+	.probe			= p2020_ds_probe,
 	.setup_arch		= mpc85xx_ds_setup_arch,
 	.init_IRQ		= mpc85xx_ds_pic_init,
 #ifdef CONFIG_PCI
@@ -199,12 +240,14 @@ define_machine(mpc8544_ds) {
 	.calibrate_decr		= generic_calibrate_decr,
 	.progress		= udbg_progress,
 };
-
-define_machine(mpc8572_ds) {
-	.name			= "MPC8572 DS",
-	.probe			= mpc8572_ds_probe,
-	.setup_arch		= mpc85xx_ds_setup_arch,
-	.init_IRQ		= mpc85xx_ds_pic_init,
+#endif /* CONFIG_MPC85xx_DS */
+
+#ifdef CONFIG_MPC85xx_RDB
+define_machine(p2020_rdb) {
+	.name			= "P2020 RDB",
+	.probe			= p2020_rdb_probe,
+	.setup_arch		= mpc85xx_rdb_setup_arch,
+	.init_IRQ		= mpc85xx_rdb_pic_init,
 #ifdef CONFIG_PCI
 	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,
 	.pcibios_fixup_phb      = fsl_pcibios_fixup_phb,
@@ -214,11 +257,11 @@ define_machine(mpc8572_ds) {
 	.progress		= udbg_progress,
 };
 
-define_machine(p2020_ds) {
-	.name			= "P2020 DS",
-	.probe			= p2020_ds_probe,
-	.setup_arch		= mpc85xx_ds_setup_arch,
-	.init_IRQ		= mpc85xx_ds_pic_init,
+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,
 #ifdef CONFIG_PCI
 	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,
 	.pcibios_fixup_phb      = fsl_pcibios_fixup_phb,
@@ -227,3 +270,4 @@ define_machine(p2020_ds) {
 	.calibrate_decr		= generic_calibrate_decr,
 	.progress		= udbg_progress,
 };
+#endif /* CONFIG_MPC85xx_RDB */

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

[Pali Rohár]

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

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

[Christophe Leroy]

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 3/8] powerpc/85xx: p2020: Move all P2020 machine descriptions to p2020.c

[Christophe Leroy]

Le 24/12/2022 à 22:14, Pali Rohár a écrit :
> This moves machine descriptions and all related code for all P2020 boards
> into new p2020.c source file. This change also copies helper static
> functions from other mpc85xx*.c files into p2020.c, which are required for
> machine descriptions. This is preparation for code de-duplication and
> providing one unified machine description for all P2020 boards. In
> follow-up patches would be copied functions refactored and simplified to be
> specific just for P2020 boards.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>   arch/powerpc/platforms/85xx/Makefile      |   2 +
>   arch/powerpc/platforms/85xx/mpc85xx_ds.c  |  23 --
>   arch/powerpc/platforms/85xx/mpc85xx_rdb.c |  44 ----
>   arch/powerpc/platforms/85xx/p2020.c       | 273 ++++++++++++++++++++++
>   4 files changed, 275 insertions(+), 67 deletions(-)
>   create mode 100644 arch/powerpc/platforms/85xx/p2020.c
> 
> diff --git a/arch/powerpc/platforms/85xx/Makefile b/arch/powerpc/platforms/85xx/Makefile
> index 260fbad7967b..1ad261b4eeb6 100644
> --- a/arch/powerpc/platforms/85xx/Makefile
> +++ b/arch/powerpc/platforms/85xx/Makefile
> @@ -23,6 +23,8 @@ obj-$(CONFIG_P1010_RDB)   += p1010rdb.o
>   obj-$(CONFIG_P1022_DS)    += p1022_ds.o
>   obj-$(CONFIG_P1022_RDK)   += p1022_rdk.o
>   obj-$(CONFIG_P1023_RDB)   += p1023_rdb.o
> +obj-$(CONFIG_MPC85xx_DS)  += p2020.o
> +obj-$(CONFIG_MPC85xx_RDB) += p2020.o
>   obj-$(CONFIG_TWR_P102x)   += twr_p102x.o
>   obj-$(CONFIG_CORENET_GENERIC)   += corenet_generic.o
>   obj-$(CONFIG_FB_FSL_DIU)	+= t1042rdb_diu.o
> diff --git a/arch/powerpc/platforms/85xx/mpc85xx_ds.c b/arch/powerpc/platforms/85xx/mpc85xx_ds.c
> index 9a6d637ef54a..05aac997b5ed 100644
> --- a/arch/powerpc/platforms/85xx/mpc85xx_ds.c
> +++ b/arch/powerpc/platforms/85xx/mpc85xx_ds.c
> @@ -168,7 +168,6 @@ static int __init mpc8544_ds_probe(void)
>   
>   machine_arch_initcall(mpc8544_ds, mpc85xx_common_publish_devices);
>   machine_arch_initcall(mpc8572_ds, mpc85xx_common_publish_devices);
> -machine_arch_initcall(p2020_ds, mpc85xx_common_publish_devices);
>   
>   /*
>    * Called very early, device-tree isn't unflattened
> @@ -178,14 +177,6 @@ static int __init mpc8572_ds_probe(void)
>   	return !!of_machine_is_compatible("fsl,MPC8572DS");
>   }
>   
> -/*
> - * Called very early, device-tree isn't unflattened
> - */
> -static int __init p2020_ds_probe(void)
> -{
> -	return !!of_machine_is_compatible("fsl,P2020DS");
> -}
> -
>   define_machine(mpc8544_ds) {
>   	.name			= "MPC8544 DS",
>   	.probe			= mpc8544_ds_probe,
> @@ -213,17 +204,3 @@ define_machine(mpc8572_ds) {
>   	.calibrate_decr		= generic_calibrate_decr,
>   	.progress		= udbg_progress,
>   };
> -
> -define_machine(p2020_ds) {
> -	.name			= "P2020 DS",
> -	.probe			= p2020_ds_probe,
> -	.setup_arch		= mpc85xx_ds_setup_arch,
> -	.init_IRQ		= mpc85xx_ds_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,
> -};
> diff --git a/arch/powerpc/platforms/85xx/mpc85xx_rdb.c b/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
> index b6129c148fea..05f1ed635735 100644
> --- a/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
> +++ b/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
> @@ -108,8 +108,6 @@ static void __init mpc85xx_rdb_setup_arch(void)
>   	printk(KERN_INFO "MPC85xx RDB board from Freescale Semiconductor\n");
>   }
>   
> -machine_arch_initcall(p2020_rdb, mpc85xx_common_publish_devices);
> -machine_arch_initcall(p2020_rdb_pc, mpc85xx_common_publish_devices);
>   machine_arch_initcall(p1020_mbg_pc, mpc85xx_common_publish_devices);
>   machine_arch_initcall(p1020_rdb, mpc85xx_common_publish_devices);
>   machine_arch_initcall(p1020_rdb_pc, mpc85xx_common_publish_devices);
> @@ -122,13 +120,6 @@ machine_arch_initcall(p1024_rdb, mpc85xx_common_publish_devices);
>   /*
>    * Called very early, device-tree isn't unflattened
>    */
> -static int __init p2020_rdb_probe(void)
> -{
> -	if (of_machine_is_compatible("fsl,P2020RDB"))
> -		return 1;
> -	return 0;
> -}
> -
>   static int __init p1020_rdb_probe(void)
>   {
>   	if (of_machine_is_compatible("fsl,P1020RDB"))
> @@ -153,13 +144,6 @@ static int __init p1021_rdb_pc_probe(void)
>   	return 0;
>   }
>   
> -static int __init p2020_rdb_pc_probe(void)
> -{
> -	if (of_machine_is_compatible("fsl,P2020RDB-PC"))
> -		return 1;
> -	return 0;
> -}
> -
>   static int __init p1025_rdb_probe(void)
>   {
>   	return of_machine_is_compatible("fsl,P1025RDB");
> @@ -180,20 +164,6 @@ static int __init p1024_rdb_probe(void)
>   	return of_machine_is_compatible("fsl,P1024RDB");
>   }
>   
> -define_machine(p2020_rdb) {
> -	.name			= "P2020 RDB",
> -	.probe			= p2020_rdb_probe,
> -	.setup_arch		= mpc85xx_rdb_setup_arch,
> -	.init_IRQ		= mpc85xx_rdb_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(p1020_rdb) {
>   	.name			= "P1020 RDB",
>   	.probe			= p1020_rdb_probe,
> @@ -222,20 +192,6 @@ define_machine(p1021_rdb_pc) {
>   	.progress		= udbg_progress,
>   };
>   
> -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,
> -#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(p1025_rdb) {
>   	.name			= "P1025 RDB",
>   	.probe			= p1025_rdb_probe,
> diff --git a/arch/powerpc/platforms/85xx/p2020.c b/arch/powerpc/platforms/85xx/p2020.c
> new file mode 100644
> index 000000000000..d65d4c88ac47
> --- /dev/null
> +++ b/arch/powerpc/platforms/85xx/p2020.c
> @@ -0,0 +1,273 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Freescale P2020 board Setup
> + *
> + * Copyright 2007,2009,2012-2013 Freescale Semiconductor Inc.
> + * Copyright 2022 Pali Rohár <pali@kernel.org>
> + */
> +
> +#include <linux/stddef.h>
> +#include <linux/kernel.h>
> +#include <linux/pci.h>
> +#include <linux/kdev_t.h>
> +#include <linux/delay.h>
> +#include <linux/seq_file.h>
> +#include <linux/interrupt.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/fsl/guts.h>
> +
> +#include <asm/time.h>
> +#include <asm/machdep.h>
> +#include <asm/pci-bridge.h>
> +#include <mm/mmu_decl.h>
> +#include <asm/udbg.h>
> +#include <asm/mpic.h>
> +#include <asm/i8259.h>
> +#include <asm/swiotlb.h>
> +
> +#include <soc/fsl/qe/qe.h>
> +
> +#include <sysdev/fsl_soc.h>
> +#include <sysdev/fsl_pci.h>
> +#include "smp.h"
> +
> +#include "mpc85xx.h"
> +
> +#undef DEBUG
> +
> +#ifdef DEBUG
> +#define DBG(fmt, args...) printk(KERN_ERR "%s: " fmt, __func__, ## args)
> +#else
> +#define DBG(fmt, args...)
> +#endif
> +
> +#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);
> +	unsigned int cascade_irq = i8259_irq();
> +
> +	if (cascade_irq) {
> +		generic_handle_irq(cascade_irq);
> +	}
> +	chip->irq_eoi(&desc->irq_data);
> +}
> +#endif	/* CONFIG_PPC_I8259 */
> +
> +static void __init mpc85xx_ds_pic_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;
> +		break;
> +	}
> +
> +	if (cascade_node == NULL) {
> +		printk(KERN_DEBUG "Could not find i8259 PIC\n");
> +		return;
> +	}
> +
> +	cascade_irq = irq_of_parse_and_map(cascade_node, 0);
> +	if (!cascade_irq) {
> +		printk(KERN_ERR "Failed to map cascade interrupt\n");
> +		return;
> +	}
> +
> +	DBG("mpc85xxds: 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 */
> +}
> +
> +#ifdef CONFIG_PCI
> +extern int uli_exclude_device(struct pci_controller *hose,
> +				u_char bus, u_char devfn);
> +
> +static struct device_node *pci_with_uli;
> +
> +static int mpc85xx_exclude_device(struct pci_controller *hose,
> +				   u_char bus, u_char devfn)
> +{
> +	if (hose->dn == pci_with_uli)
> +		return uli_exclude_device(hose, bus, devfn);
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +#endif	/* CONFIG_PCI */
> +
> +static void __init mpc85xx_ds_uli_init(void)
> +{
> +#ifdef CONFIG_PCI
> +	struct device_node *node;
> +
> +	/* See if we have a ULI under the primary */
> +
> +	node = of_find_node_by_name(NULL, "uli1575");
> +	while ((pci_with_uli = of_get_parent(node))) {
> +		of_node_put(node);
> +		node = pci_with_uli;
> +
> +		if (pci_with_uli == fsl_pci_primary) {
> +			ppc_md.pci_exclude_device = mpc85xx_exclude_device;
> +			break;
> +		}
> +	}
> +#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  ");

Can you make it a single line ? At least no more than two lines ?

> +
> +	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)
> +{
> +	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

Don't need that ifdef, there is a stub for mpc85xx_qe_par_io_init() when 
CONFIG_QUICC_ENGINE is not set.

> +	mpc85xx_qe_par_io_init();
> +#endif	/* CONFIG_QUICC_ENGINE */
> +
> +	printk(KERN_INFO "MPC85xx RDB board from Freescale Semiconductor\n");
> +}
> +#endif /* CONFIG_MPC85xx_RDB */
> +
> +#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 */
> +
> +/*
> + * 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;

could be:

	return !!of_machine_is_compatible("fsl,P2020RDB");

> +}
> +
> +static int __init p2020_rdb_pc_probe(void)
> +{
> +	if (of_machine_is_compatible("fsl,P2020RDB-PC"))
> +		return 1;

Same

> +	return 0;
> +}
> +#endif /* CONFIG_MPC85xx_RDB */
> +
> +#ifdef CONFIG_MPC85xx_DS
> +define_machine(p2020_ds) {
> +	.name			= "P2020 DS",
> +	.probe			= p2020_ds_probe,
> +	.setup_arch		= mpc85xx_ds_setup_arch,
> +	.init_IRQ		= mpc85xx_ds_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		= mpc85xx_rdb_setup_arch,
> +	.init_IRQ		= mpc85xx_rdb_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,
> +	.setup_arch		= mpc85xx_rdb_setup_arch,
> +	.init_IRQ		= mpc85xx_rdb_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_RDB */

Re: [PATCH v2 4/8] powerpc/85xx: p2020: Move i8259 code into own function

[Christophe Leroy]

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 <pali@kernel.org>
> ---
>   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

Re: [PATCH v2 6/8] powerpc/85xx: p2020: Define just one machine description

[Christophe Leroy]

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 <pali@kernel.org>
> ---
>   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 */

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

[Pali Rohár]

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 3/8] powerpc/85xx: p2020: Move all P2020 machine descriptions to p2020.c

[Pali Rohár]

On Monday 13 February 2023 20:05:09 Christophe Leroy wrote:
> Le 24/12/2022 à 22:14, Pali Rohár a écrit :
> > This moves machine descriptions and all related code for all P2020 boards
> > into new p2020.c source file. This change also copies helper static
> > functions from other mpc85xx*.c files into p2020.c, which are required for
> > machine descriptions. This is preparation for code de-duplication and
> > providing one unified machine description for all P2020 boards. In
> > follow-up patches would be copied functions refactored and simplified to be
> > specific just for P2020 boards.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > ---
> >   arch/powerpc/platforms/85xx/Makefile      |   2 +
> >   arch/powerpc/platforms/85xx/mpc85xx_ds.c  |  23 --
> >   arch/powerpc/platforms/85xx/mpc85xx_rdb.c |  44 ----
> >   arch/powerpc/platforms/85xx/p2020.c       | 273 ++++++++++++++++++++++
> >   4 files changed, 275 insertions(+), 67 deletions(-)
> >   create mode 100644 arch/powerpc/platforms/85xx/p2020.c
> > 
> > diff --git a/arch/powerpc/platforms/85xx/Makefile b/arch/powerpc/platforms/85xx/Makefile
> > index 260fbad7967b..1ad261b4eeb6 100644
> > --- a/arch/powerpc/platforms/85xx/Makefile
> > +++ b/arch/powerpc/platforms/85xx/Makefile
> > @@ -23,6 +23,8 @@ obj-$(CONFIG_P1010_RDB)   += p1010rdb.o
> >   obj-$(CONFIG_P1022_DS)    += p1022_ds.o
> >   obj-$(CONFIG_P1022_RDK)   += p1022_rdk.o
> >   obj-$(CONFIG_P1023_RDB)   += p1023_rdb.o
> > +obj-$(CONFIG_MPC85xx_DS)  += p2020.o
> > +obj-$(CONFIG_MPC85xx_RDB) += p2020.o
> >   obj-$(CONFIG_TWR_P102x)   += twr_p102x.o
> >   obj-$(CONFIG_CORENET_GENERIC)   += corenet_generic.o
> >   obj-$(CONFIG_FB_FSL_DIU)	+= t1042rdb_diu.o
> > diff --git a/arch/powerpc/platforms/85xx/mpc85xx_ds.c b/arch/powerpc/platforms/85xx/mpc85xx_ds.c
> > index 9a6d637ef54a..05aac997b5ed 100644
> > --- a/arch/powerpc/platforms/85xx/mpc85xx_ds.c
> > +++ b/arch/powerpc/platforms/85xx/mpc85xx_ds.c
> > @@ -168,7 +168,6 @@ static int __init mpc8544_ds_probe(void)
> >   
> >   machine_arch_initcall(mpc8544_ds, mpc85xx_common_publish_devices);
> >   machine_arch_initcall(mpc8572_ds, mpc85xx_common_publish_devices);
> > -machine_arch_initcall(p2020_ds, mpc85xx_common_publish_devices);
> >   
> >   /*
> >    * Called very early, device-tree isn't unflattened
> > @@ -178,14 +177,6 @@ static int __init mpc8572_ds_probe(void)
> >   	return !!of_machine_is_compatible("fsl,MPC8572DS");
> >   }
> >   
> > -/*
> > - * Called very early, device-tree isn't unflattened
> > - */
> > -static int __init p2020_ds_probe(void)
> > -{
> > -	return !!of_machine_is_compatible("fsl,P2020DS");
> > -}
> > -
> >   define_machine(mpc8544_ds) {
> >   	.name			= "MPC8544 DS",
> >   	.probe			= mpc8544_ds_probe,
> > @@ -213,17 +204,3 @@ define_machine(mpc8572_ds) {
> >   	.calibrate_decr		= generic_calibrate_decr,
> >   	.progress		= udbg_progress,
> >   };
> > -
> > -define_machine(p2020_ds) {
> > -	.name			= "P2020 DS",
> > -	.probe			= p2020_ds_probe,
> > -	.setup_arch		= mpc85xx_ds_setup_arch,
> > -	.init_IRQ		= mpc85xx_ds_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,
> > -};
> > diff --git a/arch/powerpc/platforms/85xx/mpc85xx_rdb.c b/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
> > index b6129c148fea..05f1ed635735 100644
> > --- a/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
> > +++ b/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
> > @@ -108,8 +108,6 @@ static void __init mpc85xx_rdb_setup_arch(void)
> >   	printk(KERN_INFO "MPC85xx RDB board from Freescale Semiconductor\n");
> >   }
> >   
> > -machine_arch_initcall(p2020_rdb, mpc85xx_common_publish_devices);
> > -machine_arch_initcall(p2020_rdb_pc, mpc85xx_common_publish_devices);
> >   machine_arch_initcall(p1020_mbg_pc, mpc85xx_common_publish_devices);
> >   machine_arch_initcall(p1020_rdb, mpc85xx_common_publish_devices);
> >   machine_arch_initcall(p1020_rdb_pc, mpc85xx_common_publish_devices);
> > @@ -122,13 +120,6 @@ machine_arch_initcall(p1024_rdb, mpc85xx_common_publish_devices);
> >   /*
> >    * Called very early, device-tree isn't unflattened
> >    */
> > -static int __init p2020_rdb_probe(void)
> > -{
> > -	if (of_machine_is_compatible("fsl,P2020RDB"))
> > -		return 1;
> > -	return 0;
> > -}
> > -
> >   static int __init p1020_rdb_probe(void)
> >   {
> >   	if (of_machine_is_compatible("fsl,P1020RDB"))
> > @@ -153,13 +144,6 @@ static int __init p1021_rdb_pc_probe(void)
> >   	return 0;
> >   }
> >   
> > -static int __init p2020_rdb_pc_probe(void)
> > -{
> > -	if (of_machine_is_compatible("fsl,P2020RDB-PC"))
> > -		return 1;
> > -	return 0;
> > -}
> > -
> >   static int __init p1025_rdb_probe(void)
> >   {
> >   	return of_machine_is_compatible("fsl,P1025RDB");
> > @@ -180,20 +164,6 @@ static int __init p1024_rdb_probe(void)
> >   	return of_machine_is_compatible("fsl,P1024RDB");
> >   }
> >   
> > -define_machine(p2020_rdb) {
> > -	.name			= "P2020 RDB",
> > -	.probe			= p2020_rdb_probe,
> > -	.setup_arch		= mpc85xx_rdb_setup_arch,
> > -	.init_IRQ		= mpc85xx_rdb_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(p1020_rdb) {
> >   	.name			= "P1020 RDB",
> >   	.probe			= p1020_rdb_probe,
> > @@ -222,20 +192,6 @@ define_machine(p1021_rdb_pc) {
> >   	.progress		= udbg_progress,
> >   };
> >   
> > -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,
> > -#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(p1025_rdb) {
> >   	.name			= "P1025 RDB",
> >   	.probe			= p1025_rdb_probe,
> > diff --git a/arch/powerpc/platforms/85xx/p2020.c b/arch/powerpc/platforms/85xx/p2020.c
> > new file mode 100644
> > index 000000000000..d65d4c88ac47
> > --- /dev/null
> > +++ b/arch/powerpc/platforms/85xx/p2020.c
> > @@ -0,0 +1,273 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Freescale P2020 board Setup
> > + *
> > + * Copyright 2007,2009,2012-2013 Freescale Semiconductor Inc.
> > + * Copyright 2022 Pali Rohár <pali@kernel.org>
> > + */
> > +
> > +#include <linux/stddef.h>
> > +#include <linux/kernel.h>
> > +#include <linux/pci.h>
> > +#include <linux/kdev_t.h>
> > +#include <linux/delay.h>
> > +#include <linux/seq_file.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/fsl/guts.h>
> > +
> > +#include <asm/time.h>
> > +#include <asm/machdep.h>
> > +#include <asm/pci-bridge.h>
> > +#include <mm/mmu_decl.h>
> > +#include <asm/udbg.h>
> > +#include <asm/mpic.h>
> > +#include <asm/i8259.h>
> > +#include <asm/swiotlb.h>
> > +
> > +#include <soc/fsl/qe/qe.h>
> > +
> > +#include <sysdev/fsl_soc.h>
> > +#include <sysdev/fsl_pci.h>
> > +#include "smp.h"
> > +
> > +#include "mpc85xx.h"
> > +
> > +#undef DEBUG
> > +
> > +#ifdef DEBUG
> > +#define DBG(fmt, args...) printk(KERN_ERR "%s: " fmt, __func__, ## args)
> > +#else
> > +#define DBG(fmt, args...)
> > +#endif
> > +
> > +#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);
> > +	unsigned int cascade_irq = i8259_irq();
> > +
> > +	if (cascade_irq) {
> > +		generic_handle_irq(cascade_irq);
> > +	}
> > +	chip->irq_eoi(&desc->irq_data);
> > +}
> > +#endif	/* CONFIG_PPC_I8259 */
> > +
> > +static void __init mpc85xx_ds_pic_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;
> > +		break;
> > +	}
> > +
> > +	if (cascade_node == NULL) {
> > +		printk(KERN_DEBUG "Could not find i8259 PIC\n");
> > +		return;
> > +	}
> > +
> > +	cascade_irq = irq_of_parse_and_map(cascade_node, 0);
> > +	if (!cascade_irq) {
> > +		printk(KERN_ERR "Failed to map cascade interrupt\n");
> > +		return;
> > +	}
> > +
> > +	DBG("mpc85xxds: 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 */
> > +}
> > +
> > +#ifdef CONFIG_PCI
> > +extern int uli_exclude_device(struct pci_controller *hose,
> > +				u_char bus, u_char devfn);
> > +
> > +static struct device_node *pci_with_uli;
> > +
> > +static int mpc85xx_exclude_device(struct pci_controller *hose,
> > +				   u_char bus, u_char devfn)
> > +{
> > +	if (hose->dn == pci_with_uli)
> > +		return uli_exclude_device(hose, bus, devfn);
> > +
> > +	return PCIBIOS_SUCCESSFUL;
> > +}
> > +#endif	/* CONFIG_PCI */
> > +
> > +static void __init mpc85xx_ds_uli_init(void)
> > +{
> > +#ifdef CONFIG_PCI
> > +	struct device_node *node;
> > +
> > +	/* See if we have a ULI under the primary */
> > +
> > +	node = of_find_node_by_name(NULL, "uli1575");
> > +	while ((pci_with_uli = of_get_parent(node))) {
> > +		of_node_put(node);
> > +		node = pci_with_uli;
> > +
> > +		if (pci_with_uli == fsl_pci_primary) {
> > +			ppc_md.pci_exclude_device = mpc85xx_exclude_device;
> > +			break;
> > +		}
> > +	}
> > +#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  ");
> 
> Can you make it a single line ? At least no more than two lines ?

All these lines are already present in kernel tree.
So it could be fixed in separate patch.

If needed I can do it as part of this series in new patch.

> > +
> > +	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)
> > +{
> > +	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
> 
> Don't need that ifdef, there is a stub for mpc85xx_qe_par_io_init() when 
> CONFIG_QUICC_ENGINE is not set.
> 
> > +	mpc85xx_qe_par_io_init();
> > +#endif	/* CONFIG_QUICC_ENGINE */
> > +
> > +	printk(KERN_INFO "MPC85xx RDB board from Freescale Semiconductor\n");
> > +}
> > +#endif /* CONFIG_MPC85xx_RDB */
> > +
> > +#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 */
> > +
> > +/*
> > + * 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;
> 
> could be:
> 
> 	return !!of_machine_is_compatible("fsl,P2020RDB");
> 
> > +}
> > +
> > +static int __init p2020_rdb_pc_probe(void)
> > +{
> > +	if (of_machine_is_compatible("fsl,P2020RDB-PC"))
> > +		return 1;
> 
> Same
> 
> > +	return 0;
> > +}
> > +#endif /* CONFIG_MPC85xx_RDB */
> > +
> > +#ifdef CONFIG_MPC85xx_DS
> > +define_machine(p2020_ds) {
> > +	.name			= "P2020 DS",
> > +	.probe			= p2020_ds_probe,
> > +	.setup_arch		= mpc85xx_ds_setup_arch,
> > +	.init_IRQ		= mpc85xx_ds_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		= mpc85xx_rdb_setup_arch,
> > +	.init_IRQ		= mpc85xx_rdb_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,
> > +	.setup_arch		= mpc85xx_rdb_setup_arch,
> > +	.init_IRQ		= mpc85xx_rdb_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_RDB */

Re: [PATCH v2 4/8] powerpc/85xx: p2020: Move i8259 code into own function

[Pali Rohár]

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 <pali@kernel.org>
> > ---
> >   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

Re: [PATCH v2 6/8] powerpc/85xx: p2020: Define just one machine description

[Pali Rohár]

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 <pali@kernel.org>
> > ---
> >   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 */

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

[Christophe Leroy]

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 3/8] powerpc/85xx: p2020: Move all P2020 machine descriptions to p2020.c

[Christophe Leroy]

Le 13/02/2023 à 21:15, Pali Rohár a écrit :
> On Monday 13 February 2023 20:05:09 Christophe Leroy wrote:
>> Le 24/12/2022 à 22:14, Pali Rohár a écrit :
>>> +#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  ");
>>
>> Can you make it a single line ? At least no more than two lines ?
> 
> All these lines are already present in kernel tree.
> So it could be fixed in separate patch.
> 
> If needed I can do it as part of this series in new patch.
> 

Well, that goes away in patch 5. So it is not really worth it.

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

[Pali Rohár]

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.