[PATCH v2 0/6] MIPS: cm: Probe GCR address from devicetree

[PATCH v2 0/6] MIPS: cm: Probe GCR address from devicetree

[Jiaxun Yang]

Hi all,

This series enabled mips-cm code to probe GCR address from devicetree.

This feature has been implemented in MIPS's out-of-tree kernel for
a while, and MIPS's u-boot fork on boston will generate required
"mti,mips-cm" node as well.

Please review.
Thanks

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
Changes in v2:
- Fix probe order on malta (Serge)
- dt binding improvements (Conor)
- Build warning fix
- Link to v1: https://lore.kernel.org/r/20240507-cm_probe-v1-0-11dbfd598f3c@flygoat.com

---
Jiaxun Yang (6):
      MIPS: generic: Do __dt_setup_arch in prom_init
      MIPS: malta: Move SMP initialisation to device_tree_init
      MIPS: cm: Prefix probe functions with __init
      MIPS: Move mips_cm_probe after prom_init
      dt-bindings: mips: Document mti,mips-cm
      MIPS: cm: Probe GCR address from DeviceTree

 .../devicetree/bindings/mips/mti,mips-cm.yaml      | 38 ++++++++++++
 arch/mips/generic/init.c                           |  9 ++-
 arch/mips/include/asm/mips-cm.h                    |  4 +-
 arch/mips/kernel/mips-cm.c                         | 69 ++++++++++++++++++----
 arch/mips/kernel/setup.c                           |  2 +-
 arch/mips/mti-malta/malta-init.c                   |  8 ++-
 6 files changed, 111 insertions(+), 19 deletions(-)
---
base-commit: 2b84edefcad14934796fad37b16512b6a2ca467e
change-id: 20240506-cm_probe-0c667c8b63bf

Best regards,
-- 
Jiaxun Yang <jiaxun.yang@flygoat.com>

[PATCH v2 1/6] MIPS: generic: Do __dt_setup_arch in prom_init

[Jiaxun Yang]

We want fdt parse functions to be available as early as possible,
thus do __dt_setup_arch immediately after we get fdt address in
prom_init.

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
 arch/mips/generic/init.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/mips/generic/init.c b/arch/mips/generic/init.c
index 1d712eac1617..9fd09061de78 100644
--- a/arch/mips/generic/init.c
+++ b/arch/mips/generic/init.c
@@ -26,8 +26,12 @@ static __initconst const void *mach_match_data;
 
 void __init prom_init(void)
 {
+	fw_init_cmdline();
 	plat_get_fdt();
 	BUG_ON(!fdt);
+	if (mach && mach->fixup_fdt)
+		fdt = mach->fixup_fdt(fdt, mach_match_data);
+	__dt_setup_arch((void *)fdt);
 }
 
 void __init *plat_get_fdt(void)
@@ -101,11 +105,6 @@ void __init plat_fdt_relocated(void *new_location)
 
 void __init plat_mem_setup(void)
 {
-	if (mach && mach->fixup_fdt)
-		fdt = mach->fixup_fdt(fdt, mach_match_data);
-
-	fw_init_cmdline();
-	__dt_setup_arch((void *)fdt);
 }
 
 void __init device_tree_init(void)

-- 
2.43.0

[PATCH v2 2/6] MIPS: malta: Move SMP initialisation to device_tree_init

[Jiaxun Yang]

Move SMP initialisation to device_tree_init as what generic platform
did, this allows us to move mips_cm_probe to a later point, as CPC
needs to be probed after CM.

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
v2: New patch
---
 arch/mips/mti-malta/malta-init.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/mips/mti-malta/malta-init.c b/arch/mips/mti-malta/malta-init.c
index 000d6d50520a..f67247d7fc69 100644
--- a/arch/mips/mti-malta/malta-init.c
+++ b/arch/mips/mti-malta/malta-init.c
@@ -284,12 +284,18 @@ void __init prom_init(void)
 #ifdef CONFIG_SERIAL_8250_CONSOLE
 	console_config();
 #endif
-	/* Early detection of CMP support */
+}
+
+void __init device_tree_init(void)
+{
+	unflatten_and_copy_device_tree();
 	mips_cpc_probe();
 
 	if (!register_cps_smp_ops())
 		return;
 	if (!register_vsmp_smp_ops())
 		return;
+
 	register_up_smp_ops();
 }
+

-- 
2.43.0

[PATCH v2 3/6] MIPS: cm: Prefix probe functions with __init

[Jiaxun Yang]

Those functions are only used at boot time.
Prefix them with __init so they can be discarded after boot.

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
 arch/mips/include/asm/mips-cm.h | 4 ++--
 arch/mips/kernel/mips-cm.c      | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/mips/include/asm/mips-cm.h b/arch/mips/include/asm/mips-cm.h
index c2930a75b7e4..5292b516d60b 100644
--- a/arch/mips/include/asm/mips-cm.h
+++ b/arch/mips/include/asm/mips-cm.h
@@ -30,7 +30,7 @@ extern void __iomem *mips_cm_l2sync_base;
  * and may be overridden by platforms which determine this address in a
  * different way by defining a function with the same prototype.
  */
-extern phys_addr_t mips_cm_phys_base(void);
+extern phys_addr_t __init mips_cm_phys_base(void);
 
 /**
  * mips_cm_l2sync_phys_base - retrieve the physical base address of the CM
@@ -43,7 +43,7 @@ extern phys_addr_t mips_cm_phys_base(void);
  * determine this address in a different way by defining a function with the
  * same prototype.
  */
-extern phys_addr_t mips_cm_l2sync_phys_base(void);
+extern phys_addr_t  __init mips_cm_l2sync_phys_base(void);
 
 /*
  * mips_cm_is64 - determine CM register width
diff --git a/arch/mips/kernel/mips-cm.c b/arch/mips/kernel/mips-cm.c
index 3a115fab5573..dddc9428fe58 100644
--- a/arch/mips/kernel/mips-cm.c
+++ b/arch/mips/kernel/mips-cm.c
@@ -179,7 +179,7 @@ static char *cm3_causes[32] = {
 static DEFINE_PER_CPU_ALIGNED(spinlock_t, cm_core_lock);
 static DEFINE_PER_CPU_ALIGNED(unsigned long, cm_core_lock_flags);
 
-phys_addr_t __weak mips_cm_phys_base(void)
+phys_addr_t __init __weak mips_cm_phys_base(void)
 {
 	unsigned long cmgcr;
 
@@ -198,7 +198,7 @@ phys_addr_t __weak mips_cm_phys_base(void)
 	return (cmgcr & MIPS_CMGCRF_BASE) << (36 - 32);
 }
 
-phys_addr_t __weak mips_cm_l2sync_phys_base(void)
+phys_addr_t __init __weak mips_cm_l2sync_phys_base(void)
 {
 	u32 base_reg;
 
@@ -214,7 +214,7 @@ phys_addr_t __weak mips_cm_l2sync_phys_base(void)
 	return mips_cm_phys_base() + MIPS_CM_GCR_SIZE;
 }
 
-static void mips_cm_probe_l2sync(void)
+static void __init mips_cm_probe_l2sync(void)
 {
 	unsigned major_rev;
 	phys_addr_t addr;
@@ -237,7 +237,7 @@ static void mips_cm_probe_l2sync(void)
 	mips_cm_l2sync_base = ioremap(addr, MIPS_CM_L2SYNC_SIZE);
 }
 
-int mips_cm_probe(void)
+int __init mips_cm_probe(void)
 {
 	phys_addr_t addr;
 	u32 base_reg;

-- 
2.43.0

[PATCH v2 4/6] MIPS: Move mips_cm_probe after prom_init

[Jiaxun Yang]

Move mips_cm_probe after prom_init so we can use fdt functions
in mips_cm_probe to obtain essential information.

Impat for all systems that may have CM in system:
- geneirc: Adjusted code to accommodate this change
- Lantiq: No impact, CM configuration won't be changed at all
- ralink: Called mips_cm_probe on it's own, in prom_init->prom_soc_init
- malta: Addressed in previous patch

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
 arch/mips/kernel/setup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index 12a1a4ffb602..732579c8f4f8 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -773,8 +773,8 @@ static void __init setup_rng_seed(void)
 void __init setup_arch(char **cmdline_p)
 {
 	cpu_probe();
-	mips_cm_probe();
 	prom_init();
+	mips_cm_probe();
 
 	setup_early_fdc_console();
 #ifdef CONFIG_EARLY_PRINTK

-- 
2.43.0

[PATCH v2 5/6] dt-bindings: mips: Document mti,mips-cm

[Jiaxun Yang]

Add devicetree binding documentation for MIPS Coherence Manager.

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
v2:
 - Better wording for register desc
 - cm -> coherency-manager
 - schema matches compatible
---
 .../devicetree/bindings/mips/mti,mips-cm.yaml      | 38 ++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/Documentation/devicetree/bindings/mips/mti,mips-cm.yaml b/Documentation/devicetree/bindings/mips/mti,mips-cm.yaml
new file mode 100644
index 000000000000..9f500804737d
--- /dev/null
+++ b/Documentation/devicetree/bindings/mips/mti,mips-cm.yaml
@@ -0,0 +1,38 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mips/mti,mips-cm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MIPS Coherence Manager
+
+description: |
+  Defines a location of the MIPS Coherence Manager registers.
+
+maintainers:
+  - Jiaxun Yang <jiaxun.yang@flygoat.com>
+
+properties:
+  compatible:
+    const: mti,mips-cm
+
+  reg:
+    description:
+      Base address and size of an unoccupied region in system's MMIO address
+      space, which will be used to map the MIPS CM global control registers
+      block. It is conventionally decided by the system integrator.
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    coherency-manager@1fbf8000 {
+      compatible = "mti,mips-cm";
+      reg = <0x1bde8000 0x8000>;
+    };
+...

-- 
2.43.0

Re: [PATCH v2 5/6] dt-bindings: mips: Document mti,mips-cm

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1102 bytes --]

On Wed, Jun 12, 2024 at 11:08:57AM +0100, Jiaxun Yang wrote:
> Add devicetree binding documentation for MIPS Coherence Manager.
> 
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
> v2:
>  - Better wording for register desc
>  - cm -> coherency-manager
>  - schema matches compatible
> ---
>  .../devicetree/bindings/mips/mti,mips-cm.yaml      | 38 ++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mips/mti,mips-cm.yaml b/Documentation/devicetree/bindings/mips/mti,mips-cm.yaml
> new file mode 100644
> index 000000000000..9f500804737d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mips/mti,mips-cm.yaml
> @@ -0,0 +1,38 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mips/mti,mips-cm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MIPS Coherence Manager
> +
> +description: |

The | is not needed here.

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 5/6] dt-bindings: mips: Document mti,mips-cm

[Serge Semin]

On Wed, Jun 12, 2024 at 11:08:57AM +0100, Jiaxun Yang wrote:
> Add devicetree binding documentation for MIPS Coherence Manager.
> 
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
> v2:
>  - Better wording for register desc
>  - cm -> coherency-manager
>  - schema matches compatible
> ---
>  .../devicetree/bindings/mips/mti,mips-cm.yaml      | 38 ++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mips/mti,mips-cm.yaml b/Documentation/devicetree/bindings/mips/mti,mips-cm.yaml
> new file mode 100644
> index 000000000000..9f500804737d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mips/mti,mips-cm.yaml
> @@ -0,0 +1,38 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mips/mti,mips-cm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MIPS Coherence Manager
> +
> +description: |
> +  Defines a location of the MIPS Coherence Manager registers.
> +
> +maintainers:
> +  - Jiaxun Yang <jiaxun.yang@flygoat.com>
> +
> +properties:
> +  compatible:
> +    const: mti,mips-cm
> +

> +  reg:
> +    description:
> +      Base address and size of an unoccupied region in system's MMIO address
> +      space, which will be used to map the MIPS CM global control registers
> +      block. It is conventionally decided by the system integrator.
> +    maxItems: 1

Could you please extend the reg array to containing two values: gcr and
l2sync? The later is the L2-cache-only sync region which can be
customized by the CM means.

It's better to define the reg-names property too, so the node would
look like this:

        cm2: cm2@1fbf8000 {
                compatible = "mti,mips-cm";
                reg = <0 0x1fbf8000 0 0x8000>,
                      <0 0x1fbf0000 0 0x1000>;
                reg-names = "gcr", "l2sync";
        };

-Serge(y)

> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    coherency-manager@1fbf8000 {
> +      compatible = "mti,mips-cm";
> +      reg = <0x1bde8000 0x8000>;
> +    };
> +...
> 
> -- 
> 2.43.0
> 

[PATCH v2 6/6] MIPS: cm: Probe GCR address from DeviceTree

[Jiaxun Yang]

Traditionally, CM GCR address can be probed from CP0_CMGCRBase.

However there are chips in wild that do have CM GCR but CP0_CMGCRBase
is not available from CPU point of view. Thus we need to be able to
probe GCR address from DeviceTree.

It is implemented as:
- If only CP0_CMGCRBase present, trust CP0_CMGCRBase
- If only mti,mips-cm node present, trust mti,mips-cm reg prop
- If both present, remap address space to address specified in dt

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
v2: Fix build warning (test bot)
---
 arch/mips/kernel/mips-cm.c | 61 +++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 55 insertions(+), 6 deletions(-)

diff --git a/arch/mips/kernel/mips-cm.c b/arch/mips/kernel/mips-cm.c
index dddc9428fe58..02afc795ba8a 100644
--- a/arch/mips/kernel/mips-cm.c
+++ b/arch/mips/kernel/mips-cm.c
@@ -5,6 +5,8 @@
  */
 
 #include <linux/errno.h>
+#include <linux/of_address.h>
+#include <linux/of_fdt.h>
 #include <linux/percpu.h>
 #include <linux/spinlock.h>
 
@@ -179,23 +181,70 @@ static char *cm3_causes[32] = {
 static DEFINE_PER_CPU_ALIGNED(spinlock_t, cm_core_lock);
 static DEFINE_PER_CPU_ALIGNED(unsigned long, cm_core_lock_flags);
 
+static int __init mips_cm_fdt_scan(unsigned long node, const char *uname,
+		int depth, void *data)
+{
+	u64 addr;
+	unsigned long *cmgcr = data;
+
+	if (!of_flat_dt_is_compatible(node, "mti,mips-cm"))
+		return 0;
+
+	addr = of_flat_dt_translate_address(node);
+	if (addr == OF_BAD_ADDR || addr >= ULONG_MAX)
+		*cmgcr = 0;
+	else
+		*cmgcr = addr;
+
+	return 0;
+}
+
 phys_addr_t __init __weak mips_cm_phys_base(void)
 {
-	unsigned long cmgcr;
+	unsigned long gcr_reg = 0, gcr_dt = 0;
+
+	if (of_have_populated_dt()) {
+		int err;
+		struct resource res;
+		struct device_node *cm_node;
+
+		cm_node = of_find_compatible_node(of_root, NULL, "mti,mips-cm");
+		if (cm_node) {
+			err = of_address_to_resource(cm_node, 0, &res);
+			of_node_put(cm_node);
+			if (!err)
+				gcr_dt = res.start;
+		}
+	} else {
+		of_scan_flat_dt(mips_cm_fdt_scan, &gcr_dt);
+	}
 
 	/* Check the CMGCRBase register is implemented */
 	if (!(read_c0_config() & MIPS_CONF_M))
-		return 0;
+		return gcr_dt;
 
 	if (!(read_c0_config2() & MIPS_CONF_M))
-		return 0;
+		return gcr_dt;
 
 	if (!(read_c0_config3() & MIPS_CONF3_CMGCR))
-		return 0;
+		return gcr_dt;
 
 	/* Read the address from CMGCRBase */
-	cmgcr = read_c0_cmgcrbase();
-	return (cmgcr & MIPS_CMGCRF_BASE) << (36 - 32);
+	gcr_reg = read_c0_cmgcrbase();
+	gcr_reg = (gcr_reg & MIPS_CMGCRF_BASE) << (36 - 32);
+
+	/* If no of node, return straight away */
+	if (!gcr_dt)
+		return gcr_reg;
+
+	/* If the CMGCRBase mismatches with dt, remap it */
+	if (gcr_reg != gcr_dt) {
+		pr_info("Remapping CMGCRBase from 0x%08lx to 0x%08lx\n",
+			gcr_reg, gcr_dt);
+		change_gcr_base(CM_GCR_BASE_GCRBASE, gcr_dt);
+	}
+
+	return gcr_dt;
 }
 
 phys_addr_t __init __weak mips_cm_l2sync_phys_base(void)

-- 
2.43.0

Re: [PATCH v2 6/6] MIPS: cm: Probe GCR address from DeviceTree

[Serge Semin]

Hi Jiaxun, Thomas

On Wed, Jun 12, 2024 at 11:08:58AM +0100, Jiaxun Yang wrote:
> Traditionally, CM GCR address can be probed from CP0_CMGCRBase.
> 
> However there are chips in wild that do have CM GCR but CP0_CMGCRBase
> is not available from CPU point of view. Thus we need to be able to
> probe GCR address from DeviceTree.
> 
> It is implemented as:
> - If only CP0_CMGCRBase present, trust CP0_CMGCRBase
> - If only mti,mips-cm node present, trust mti,mips-cm reg prop
> - If both present, remap address space to address specified in dt
> 
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
> v2: Fix build warning (test bot)
> ---
>  arch/mips/kernel/mips-cm.c | 61 +++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 55 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/mips/kernel/mips-cm.c b/arch/mips/kernel/mips-cm.c
> index dddc9428fe58..02afc795ba8a 100644
> --- a/arch/mips/kernel/mips-cm.c
> +++ b/arch/mips/kernel/mips-cm.c
> @@ -5,6 +5,8 @@
>   */
>  
>  #include <linux/errno.h>
> +#include <linux/of_address.h>
> +#include <linux/of_fdt.h>
>  #include <linux/percpu.h>
>  #include <linux/spinlock.h>
>  
> @@ -179,23 +181,70 @@ static char *cm3_causes[32] = {
>  static DEFINE_PER_CPU_ALIGNED(spinlock_t, cm_core_lock);
>  static DEFINE_PER_CPU_ALIGNED(unsigned long, cm_core_lock_flags);
>  
> +static int __init mips_cm_fdt_scan(unsigned long node, const char *uname,
> +		int depth, void *data)
> +{
> +	u64 addr;
> +	unsigned long *cmgcr = data;
> +
> +	if (!of_flat_dt_is_compatible(node, "mti,mips-cm"))
> +		return 0;
> +
> +	addr = of_flat_dt_translate_address(node);
> +	if (addr == OF_BAD_ADDR || addr >= ULONG_MAX)
> +		*cmgcr = 0;
> +	else
> +		*cmgcr = addr;
> +
> +	return 0;
> +}
> +
>  phys_addr_t __init __weak mips_cm_phys_base(void)
>  {
> -	unsigned long cmgcr;
> +	unsigned long gcr_reg = 0, gcr_dt = 0;
> +
> +	if (of_have_populated_dt()) {
> +		int err;
> +		struct resource res;
> +		struct device_node *cm_node;
> +
> +		cm_node = of_find_compatible_node(of_root, NULL, "mti,mips-cm");
> +		if (cm_node) {
> +			err = of_address_to_resource(cm_node, 0, &res);
> +			of_node_put(cm_node);
> +			if (!err)
> +				gcr_dt = res.start;
> +		}
> +	} else {
> +		of_scan_flat_dt(mips_cm_fdt_scan, &gcr_dt);
> +	}
>  
>  	/* Check the CMGCRBase register is implemented */
>  	if (!(read_c0_config() & MIPS_CONF_M))
> -		return 0;
> +		return gcr_dt;
>  
>  	if (!(read_c0_config2() & MIPS_CONF_M))
> -		return 0;
> +		return gcr_dt;
>  
>  	if (!(read_c0_config3() & MIPS_CONF3_CMGCR))
> -		return 0;
> +		return gcr_dt;
>  
>  	/* Read the address from CMGCRBase */
> -	cmgcr = read_c0_cmgcrbase();
> -	return (cmgcr & MIPS_CMGCRF_BASE) << (36 - 32);
> +	gcr_reg = read_c0_cmgcrbase();
> +	gcr_reg = (gcr_reg & MIPS_CMGCRF_BASE) << (36 - 32);
> +
> +	/* If no of node, return straight away */
> +	if (!gcr_dt)
> +		return gcr_reg;
> +
> +	/* If the CMGCRBase mismatches with dt, remap it */
> +	if (gcr_reg != gcr_dt) {
> +		pr_info("Remapping CMGCRBase from 0x%08lx to 0x%08lx\n",
> +			gcr_reg, gcr_dt);

> +		change_gcr_base(CM_GCR_BASE_GCRBASE, gcr_dt);

This causes the kernel boot-up procedure to crash/hang-up because the
CM GCR base address is supposed to be redefined by means of the
already mapped CM GCR address space by accessing the
CM_GCR_BASE_GCRBASE register:
change_gcr_base()
+-> read_gcr_base()
    +-> addr_gcr_base()
        +-> return mips_gcr_base + CM_GCR_BASE_GCRBASE

By the time of the change_gcr_base() call in mips_cm_phys_base(), the
mips_gcr_base variable hasn't been defined. So the IO operations
performed in the change_gcr_base() method would be accessing the
NULL-based memory space. That's why the kernel crash/hanging-up.

In order to fix this we have to first map the CM GCR block at the
default base-address, then update the CM GCR-base CSR and after that
remap the CM GCR-space.

Please also note, the GCR_BASE field might be RO. It depends on the
IP-core configuration. So it's possible that the CM_GCR_BASE_GCRBASE
field update won't work. Although that will be detected a bit later in
the mips_cm_probe() method by comparing the address returned from
mips_cm_phys_base() and retrieved from the CM GCR-base CSR.

-Serge(y)

> +	}
> +
> +	return gcr_dt;
>  }
>  
>  phys_addr_t __init __weak mips_cm_l2sync_phys_base(void)
> 
> -- 
> 2.43.0
> 

Re: [PATCH v2 6/6] MIPS: cm: Probe GCR address from DeviceTree

[Jiaxun Yang]

在2024年9月10日九月 下午1:36，Serge Semin写道：
[...]
>
> This causes the kernel boot-up procedure to crash/hang-up because the
> CM GCR base address is supposed to be redefined by means of the
> already mapped CM GCR address space by accessing the
> CM_GCR_BASE_GCRBASE register:
> change_gcr_base()
> +-> read_gcr_base()
>     +-> addr_gcr_base()
>         +-> return mips_gcr_base + CM_GCR_BASE_GCRBASE
>
> By the time of the change_gcr_base() call in mips_cm_phys_base(), the
> mips_gcr_base variable hasn't been defined. So the IO operations
> performed in the change_gcr_base() method would be accessing the
> NULL-based memory space. That's why the kernel crash/hanging-up.

Thanks for the analysis!
This path was not taken on my audience hardware, so I didn't catch this,
will fix in next version.

>
> In order to fix this we have to first map the CM GCR block at the
> default base-address, then update the CM GCR-base CSR and after that
> remap the CM GCR-space.
>
> Please also note, the GCR_BASE field might be RO. It depends on the
> IP-core configuration. So it's possible that the CM_GCR_BASE_GCRBASE
> field update won't work. Although that will be detected a bit later in
> the mips_cm_probe() method by comparing the address returned from
> mips_cm_phys_base() and retrieved from the CM GCR-base CSR.

Hmm, I just checked RTL and RDL for CM2 and CM3 and I didn't see it as a
configurable option. It's possible to change hardware reset value but not make it RO.

Maybe it was possible on earlier IP release, in this case it's always
user's responsibility to write correct address in DeviceTree :-)

Thanks

>
> -Serge(y)

-- 
- Jiaxun

Re: [PATCH v2 6/6] MIPS: cm: Probe GCR address from DeviceTree

[Serge Semin]

On Tue, Sep 10, 2024 at 08:23:25PM +0100, Jiaxun Yang wrote:
> 
> 
> 在2024年9月10日九月 下午1:36，Serge Semin写道：
> [...]
> >
> > This causes the kernel boot-up procedure to crash/hang-up because the
> > CM GCR base address is supposed to be redefined by means of the
> > already mapped CM GCR address space by accessing the
> > CM_GCR_BASE_GCRBASE register:
> > change_gcr_base()
> > +-> read_gcr_base()
> >     +-> addr_gcr_base()
> >         +-> return mips_gcr_base + CM_GCR_BASE_GCRBASE
> >
> > By the time of the change_gcr_base() call in mips_cm_phys_base(), the
> > mips_gcr_base variable hasn't been defined. So the IO operations
> > performed in the change_gcr_base() method would be accessing the
> > NULL-based memory space. That's why the kernel crash/hanging-up.
> 
> Thanks for the analysis!
> This path was not taken on my audience hardware, so I didn't catch this,
> will fix in next version.
> 
> >
> > In order to fix this we have to first map the CM GCR block at the
> > default base-address, then update the CM GCR-base CSR and after that
> > remap the CM GCR-space.
> >
> > Please also note, the GCR_BASE field might be RO. It depends on the
> > IP-core configuration. So it's possible that the CM_GCR_BASE_GCRBASE
> > field update won't work. Although that will be detected a bit later in
> > the mips_cm_probe() method by comparing the address returned from
> > mips_cm_phys_base() and retrieved from the CM GCR-base CSR.
> 

> Hmm, I just checked RTL and RDL for CM2 and CM3 and I didn't see it as a
> configurable option. It's possible to change hardware reset value but not make it RO.

Both MIPS P5600 and P6600 databooks define the GCR_BASE field as
optionally R/W:

GCR_BASE 31:15 This field sets the base address of the 32KB          R or R/W
               GCR block of the P5600 MPS.                           (IP Config-
               This register has a fixed value after reset if         uration)
               configured as Read-Only (an IP Configuration Option).

> 
> Maybe it was possible on earlier IP release,

I found the text above in the latest MIPS Warrior P-class software
manuals downloaded from the ImagTech site. Not sure why your RTL code
doesn't have such configs.

> in this case it's always
> user's responsibility to write correct address in DeviceTree :-)

Right. The system just won't work if the CM GCR base address couldn't
be updated.

-Serge(y)

> 
> Thanks
> 
> >
> > -Serge(y)
> 
> -- 
> - Jiaxun

Re: [PATCH v2 6/6] MIPS: cm: Probe GCR address from DeviceTree

[Jiaxun Yang]

在2024年9月10日九月 下午9:07，Serge Semin写道：
[...]
> Both MIPS P5600 and P6600 databooks define the GCR_BASE field as
> optionally R/W:
>
> GCR_BASE 31:15 This field sets the base address of the 32KB          R or R/W
>                GCR block of the P5600 MPS.                           (IP Config-
>                This register has a fixed value after reset if         uration)
>                configured as Read-Only (an IP Configuration Option).
>

Thanks for the pointer, I traced code history and it seems like MIPS decided
to not expose this functionality at some point, but documents were not updated.

Maybe I should add a read back check here.

Thanks
-- 
- Jiaxun

Re: [PATCH v2 6/6] MIPS: cm: Probe GCR address from DeviceTree

[Serge Semin]

On Tue, Sep 10, 2024 at 09:48:10PM +0100, Jiaxun Yang wrote:
> 
> 
> 在2024年9月10日九月 下午9:07，Serge Semin写道：
> [...]
> > Both MIPS P5600 and P6600 databooks define the GCR_BASE field as
> > optionally R/W:
> >
> > GCR_BASE 31:15 This field sets the base address of the 32KB          R or R/W
> >                GCR block of the P5600 MPS.                           (IP Config-
> >                This register has a fixed value after reset if         uration)
> >                configured as Read-Only (an IP Configuration Option).
> >
> 

> Thanks for the pointer, I traced code history and it seems like MIPS decided
> to not expose this functionality at some point, but documents were not updated.

Got it. Thanks for clarification.

> 
> Maybe I should add a read back check here.

The check is already implemented in the mips_cm_probe() method. Just
10 lines below the mips_cm_phys_base() method call.

-Serge(y)

> 
> Thanks
> -- 
> - Jiaxun

Re: [PATCH v2 0/6] MIPS: cm: Probe GCR address from devicetree

[Jiaxun Yang]

在2024年6月12日六月 下午6:08，Jiaxun Yang写道：
> Hi all,
>
> This series enabled mips-cm code to probe GCR address from devicetree.
>
> This feature has been implemented in MIPS's out-of-tree kernel for
> a while, and MIPS's u-boot fork on boston will generate required
> "mti,mips-cm" node as well.

Folks, any comments on this series?

Thanks

>
> Please review.
> Thanks
>
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
> Changes in v2:
> - Fix probe order on malta (Serge)
> - dt binding improvements (Conor)
> - Build warning fix
> - Link to v1: 
> https://lore.kernel.org/r/20240507-cm_probe-v1-0-11dbfd598f3c@flygoat.com
>
> ---
> Jiaxun Yang (6):
>       MIPS: generic: Do __dt_setup_arch in prom_init
>       MIPS: malta: Move SMP initialisation to device_tree_init
>       MIPS: cm: Prefix probe functions with __init
>       MIPS: Move mips_cm_probe after prom_init
>       dt-bindings: mips: Document mti,mips-cm
>       MIPS: cm: Probe GCR address from DeviceTree
>
>  .../devicetree/bindings/mips/mti,mips-cm.yaml      | 38 ++++++++++++
>  arch/mips/generic/init.c                           |  9 ++-
>  arch/mips/include/asm/mips-cm.h                    |  4 +-
>  arch/mips/kernel/mips-cm.c                         | 69 ++++++++++++++++++----
>  arch/mips/kernel/setup.c                           |  2 +-
>  arch/mips/mti-malta/malta-init.c                   |  8 ++-
>  6 files changed, 111 insertions(+), 19 deletions(-)
> ---
> base-commit: 2b84edefcad14934796fad37b16512b6a2ca467e
> change-id: 20240506-cm_probe-0c667c8b63bf
>
> Best regards,
> -- 
> Jiaxun Yang <jiaxun.yang@flygoat.com>

-- 
- Jiaxun

Re: [PATCH v2 0/6] MIPS: cm: Probe GCR address from devicetree

[Jiaxun Yang]

在2024年6月12日六月 下午6:08，Jiaxun Yang写道：
> Hi all,
>
> This series enabled mips-cm code to probe GCR address from devicetree.
>
> This feature has been implemented in MIPS's out-of-tree kernel for
> a while, and MIPS's u-boot fork on boston will generate required
> "mti,mips-cm" node as well.
>
> Please review.
> Thanks


Ping on this?


Thanks

>
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
> Changes in v2:
> - Fix probe order on malta (Serge)
> - dt binding improvements (Conor)
> - Build warning fix
> - Link to v1: 
> https://lore.kernel.org/r/20240507-cm_probe-v1-0-11dbfd598f3c@flygoat.com
>
> ---
> Jiaxun Yang (6):
>       MIPS: generic: Do __dt_setup_arch in prom_init
>       MIPS: malta: Move SMP initialisation to device_tree_init
>       MIPS: cm: Prefix probe functions with __init
>       MIPS: Move mips_cm_probe after prom_init
>       dt-bindings: mips: Document mti,mips-cm
>       MIPS: cm: Probe GCR address from DeviceTree
>
>  .../devicetree/bindings/mips/mti,mips-cm.yaml      | 38 ++++++++++++
>  arch/mips/generic/init.c                           |  9 ++-
>  arch/mips/include/asm/mips-cm.h                    |  4 +-
>  arch/mips/kernel/mips-cm.c                         | 69 ++++++++++++++++++----
>  arch/mips/kernel/setup.c                           |  2 +-
>  arch/mips/mti-malta/malta-init.c                   |  8 ++-
>  6 files changed, 111 insertions(+), 19 deletions(-)
> ---
> base-commit: 2b84edefcad14934796fad37b16512b6a2ca467e
> change-id: 20240506-cm_probe-0c667c8b63bf
>
> Best regards,
> -- 
> Jiaxun Yang <jiaxun.yang@flygoat.com>

-- 
- Jiaxun

Re: [PATCH v2 0/6] MIPS: cm: Probe GCR address from devicetree

[Serge Semin]

Hi Jiaxun

On Wed, Jun 12, 2024 at 11:08:52AM +0100, Jiaxun Yang wrote:
> Hi all,
> 
> This series enabled mips-cm code to probe GCR address from devicetree.
> 
> This feature has been implemented in MIPS's out-of-tree kernel for
> a while, and MIPS's u-boot fork on boston will generate required
> "mti,mips-cm" node as well.
> 
> Please review.
> Thanks

Got this tested on my P5600-based SoC implemented as non-generic
platform. Alas the system hangs up on the early boot-up stage with no
even a single char printed to the console. I'll be able to get back to
the problem debugging on the next week.

-Serge(y)

> 
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
> Changes in v2:
> - Fix probe order on malta (Serge)
> - dt binding improvements (Conor)
> - Build warning fix
> - Link to v1: https://lore.kernel.org/r/20240507-cm_probe-v1-0-11dbfd598f3c@flygoat.com
> 
> ---
> Jiaxun Yang (6):
>       MIPS: generic: Do __dt_setup_arch in prom_init
>       MIPS: malta: Move SMP initialisation to device_tree_init
>       MIPS: cm: Prefix probe functions with __init
>       MIPS: Move mips_cm_probe after prom_init
>       dt-bindings: mips: Document mti,mips-cm
>       MIPS: cm: Probe GCR address from DeviceTree
> 
>  .../devicetree/bindings/mips/mti,mips-cm.yaml      | 38 ++++++++++++
>  arch/mips/generic/init.c                           |  9 ++-
>  arch/mips/include/asm/mips-cm.h                    |  4 +-
>  arch/mips/kernel/mips-cm.c                         | 69 ++++++++++++++++++----
>  arch/mips/kernel/setup.c                           |  2 +-
>  arch/mips/mti-malta/malta-init.c                   |  8 ++-
>  6 files changed, 111 insertions(+), 19 deletions(-)
> ---
> base-commit: 2b84edefcad14934796fad37b16512b6a2ca467e
> change-id: 20240506-cm_probe-0c667c8b63bf
> 
> Best regards,
> -- 
> Jiaxun Yang <jiaxun.yang@flygoat.com>
> 

Re: [PATCH v2 0/6] MIPS: cm: Probe GCR address from devicetree

[Thomas Bogendoerfer]

On Tue, Aug 06, 2024 at 10:49:52PM +0300, Serge Semin wrote:
> Hi Jiaxun
> 
> On Wed, Jun 12, 2024 at 11:08:52AM +0100, Jiaxun Yang wrote:
> > Hi all,
> > 
> > This series enabled mips-cm code to probe GCR address from devicetree.
> > 
> > This feature has been implemented in MIPS's out-of-tree kernel for
> > a while, and MIPS's u-boot fork on boston will generate required
> > "mti,mips-cm" node as well.
> > 
> > Please review.
> > Thanks
> 
> Got this tested on my P5600-based SoC implemented as non-generic
> platform. Alas the system hangs up on the early boot-up stage with no
> even a single char printed to the console. I'll be able to get back to
> the problem debugging on the next week.

any news about that ?

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH v2 0/6] MIPS: cm: Probe GCR address from devicetree

[Serge Semin]

Hi Thomas

On Mon, Sep 09, 2024 at 03:32:21PM +0200, Thomas Bogendoerfer wrote:
> On Tue, Aug 06, 2024 at 10:49:52PM +0300, Serge Semin wrote:
> > Hi Jiaxun
> > 
> > On Wed, Jun 12, 2024 at 11:08:52AM +0100, Jiaxun Yang wrote:
> > > Hi all,
> > > 
> > > This series enabled mips-cm code to probe GCR address from devicetree.
> > > 
> > > This feature has been implemented in MIPS's out-of-tree kernel for
> > > a while, and MIPS's u-boot fork on boston will generate required
> > > "mti,mips-cm" node as well.
> > > 
> > > Please review.
> > > Thanks
> > 
> > Got this tested on my P5600-based SoC implemented as non-generic
> > platform. Alas the system hangs up on the early boot-up stage with no
> > even a single char printed to the console. I'll be able to get back to
> > the problem debugging on the next week.
> 
> any news about that ?

Oops. This patch set has absolutely slipped out of my mind. I am
getting back to it immediately and will submit the debug status
shortly after I dig out the reason of the hanging up. Sorry for the
inconvenience.

-Serge(y)

> 
> Thomas.
> 
> -- 
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH v2 0/6] MIPS: cm: Probe GCR address from devicetree

[Serge Semin]

On Tue, Sep 10, 2024 at 11:06:56AM +0300, Serge Semin wrote:
> Hi Thomas
> 
> On Mon, Sep 09, 2024 at 03:32:21PM +0200, Thomas Bogendoerfer wrote:
> > On Tue, Aug 06, 2024 at 10:49:52PM +0300, Serge Semin wrote:
> > > Hi Jiaxun
> > > 
> > > On Wed, Jun 12, 2024 at 11:08:52AM +0100, Jiaxun Yang wrote:
> > > > Hi all,
> > > > 
> > > > This series enabled mips-cm code to probe GCR address from devicetree.
> > > > 
> > > > This feature has been implemented in MIPS's out-of-tree kernel for
> > > > a while, and MIPS's u-boot fork on boston will generate required
> > > > "mti,mips-cm" node as well.
> > > > 
> > > > Please review.
> > > > Thanks
> > > 
> > > Got this tested on my P5600-based SoC implemented as non-generic
> > > platform. Alas the system hangs up on the early boot-up stage with no
> > > even a single char printed to the console. I'll be able to get back to
> > > the problem debugging on the next week.
> > 
> > any news about that ?
> 
> Oops. This patch set has absolutely slipped out of my mind. I am
> getting back to it immediately and will submit the debug status
> shortly after I dig out the reason of the hanging up. Sorry for the
> inconvenience.

Found the reason of the problem on my platform. It was due to the too
early change_gcs_control() invocation. Since mips_cm_probe() is now
called after the prom_init() method the later function can't access
any CM-register. So for the system to boot up I had to move the GCR
controler register update to the plat_mem_setup() method in my
platform code. After that the system booted up successfully. Double
checked the rest of the platforms in the vanilla kernel repo for having
the similar issue. It seems to me there is no platform left in the
kernel with such potential problem presented.

But then I decided to test out the actual GCR-base address setup
procedure implemented in this patch set, and found another problem
unrelated to my platform. I'll submit the problem summary in reply to
the respective patch in this series.

-Serge(y)

> 
> -Serge(y)
> 
> > 
> > Thomas.
> > 
> > -- 
> > Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> > good idea.                                                [ RFC1925, 2.3 ]