Skip to contentSkip to conversation

Improve mvebu_get_soc_id

PATCH New Gregory CLEMENT 5 participants 10 messages Jun 20, 2014 – Jun 21, 2014

Subsystems (3)
Arm (32-bit) Arm sub-architectures Arm/Marvell Kirkwood and Armada 370, 375, 38x, 39x, XP, 3700, 7K/8K, CN9130 SoC support
lore
Share & tools
lore

Gregory CLEMENT

author
#0 lore raw

v2 cover Improve mvebu_get_soc_id

Hi,

The first use -ENODEV as return value instead of -1 as suggested by Arnd for the second patch.

The second patch allows to get the SoC Id and the revision without using the PCI register on Armada 38x.

Thanks,

Gregory

Changelog:

v1 -> v2:

- Fix typo in mvebu_system_controller_get_soc_id.
- Use -ENODEV as return value instead of -1

Gregory CLEMENT (2):
ARM: mvebu: Use the a standard errno in mvebu_get_soc_id
ARM: mvebu: Use system controller to get the soc id when possible

 arch/arm/mach-mvebu/common.h            |  1 +
 arch/arm/mach-mvebu/mvebu-soc-id.c      | 22 ++++++++++++++++++++--
 arch/arm/mach-mvebu/system-controller.c | 19 +++++++++++++++++++
 3 files changed, 40 insertions(+), 2 deletions(-)
Signature · 1 line
1.8.1.2
author
#1 lore raw

PATCH v2 1/2 ARM: mvebu: Use the a standard errno in mvebu_get_soc_id

Instead of using -1 as error value, use a standard errno.

Signed-off-by Gregory CLEMENT <[email protected]>
---
 arch/arm/mach-mvebu/mvebu-soc-id.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/mach-mvebu/mvebu-soc-id.c +1 -1
index d0f35b4d4a23..12c66cac967d 100644
@@ -51,7 +51,7 @@ int mvebu_get_soc_id(u32 *dev, u32 *rev)
 		*rev = soc_rev;
 		return 0;
 	} else
-		return -1;
+		return -ENODEV;
 }
 
 static int __init mvebu_soc_id_init(void)
Signature · 1 line
1.8.1.2
#2 lore raw

PATCH v2 1/2 ARM: mvebu: Use the a standard errno in mvebu_get_soc_id

Dear Gregory CLEMENT,

Quoted 6 lines · Gregory CLEMENT

Instead of using -1 as error value, use a standard errno.

Signed-off-by Gregory CLEMENT <[email protected]>
---
 arch/arm/mach-mvebu/mvebu-soc-id.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Acked-by Thomas Petazzoni <[email protected]>
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
author
#4 lore raw

PATCH v2 2/2 ARM: mvebu: Use system controller to get the soc id when possible

In reply to Gregory CLEMENT

On Armada 38x it is possible to get the SoC Id and the revision without using the PCI register. Accessing the PCI registers implies enabling its clock and, because of the initialization issue, not keeping them enable. So if possible it is better to avoid it.

Armada 370 and Armada XP provides the SoC ID values from the system controller but not the revision.

Armada 375 provides both but the SoC ID value looks buggy (0x6660 instead of 0x6720).

Signed-off-by Gregory CLEMENT <[email protected]>
---
 arch/arm/mach-mvebu/common.h            |  1 +
 arch/arm/mach-mvebu/mvebu-soc-id.c      | 20 +++++++++++++++++++-
 arch/arm/mach-mvebu/system-controller.c | 19 +++++++++++++++++++
 3 files changed, 39 insertions(+), 1 deletion(-)
arch/arm/mach-mvebu/common.h +1 -0
index b67fb7a10d8b..927d88627a6e 100644
@@ -21,6 +21,7 @@ void mvebu_restart(enum reboot_mode mode, const char *cmd);
 int mvebu_cpu_reset_deassert(int cpu);
 void mvebu_pmsu_set_cpu_boot_addr(int hw_cpu, void *boot_addr);
 void mvebu_system_controller_set_cpu_boot_addr(void *boot_addr);
+int mvebu_system_controller_get_soc_id(u32 *dev, u32 *rev);
 
 void armada_xp_cpu_die(unsigned int cpu);
arch/arm/mach-mvebu/mvebu-soc-id.c +19 -1
index 12c66cac967d..922b1380b0be 100644
@@ -25,8 +25,10 @@
 #include <linux/of_address.h>
 #include <linux/slab.h>
 #include <linux/sys_soc.h>
+#include "common.h"
 #include "mvebu-soc-id.h"
 
+
 #define PCIE_DEV_ID_OFF		0x0
 #define PCIE_DEV_REV_OFF	0x8
 

@@ -54,7 +56,7 @@ int mvebu_get_soc_id(u32 *dev, u32 *rev)
 		return -ENODEV;
 }
 
-static int __init mvebu_soc_id_init(void)
+static int __init get_soc_id_by_pci(void)
 {
 	struct device_node *np;
 	int ret = 0;

@@ -129,6 +131,22 @@ clk_err:
 
 	return ret;
 }
+
+static int __init mvebu_soc_id_init(void)
+{
+
+	/*
+	 * First try to get the ID and the revision by the system
+	 * regsiter and use PCI registers only if it is not possible
+	 */
+	if (!mvebu_system_controller_get_soc_id(&soc_dev_id, &soc_rev)) {
+		is_id_valid = true;
+		pr_info("MVEBU SoC ID=0x%X, Rev=0x%X\n", soc_dev_id, soc_rev);
+		return 0;
+	}
+
+	return get_soc_id_by_pci();
+}
 early_initcall(mvebu_soc_id_init);
 
 static int __init mvebu_soc_device(void)
arch/arm/mach-mvebu/system-controller.c +19 -0
index 0c5524ac75b7..de90821129da 100644
@@ -39,6 +39,9 @@ struct mvebu_system_controller {
 	u32 system_soft_reset;
 
 	u32 resume_boot_addr;
+
+	u32 dev_id;
+	u32 rev_id;
 };
 static struct mvebu_system_controller *mvebu_sc;
 

@@ -47,6 +50,8 @@ static const struct mvebu_system_controller armada_370_xp_system_controller = {
 	.system_soft_reset_offset = 0x64,
 	.rstoutn_mask_reset_out_en = 0x1,
 	.system_soft_reset = 0x1,
+	.dev_id = 0x38,
+	.rev_id = 0x3c,
 };
 
 static const struct mvebu_system_controller armada_375_system_controller = {

@@ -55,6 +60,8 @@ static const struct mvebu_system_controller armada_375_system_controller = {
 	.rstoutn_mask_reset_out_en = 0x1,
 	.system_soft_reset = 0x1,
 	.resume_boot_addr = 0xd4,
+	.dev_id = 0x38,
+	.rev_id = 0x3c,
 };
 
 static const struct mvebu_system_controller orion_system_controller = {

@@ -101,6 +108,18 @@ void mvebu_restart(enum reboot_mode mode, const char *cmd)
 		;
 }
 
+int mvebu_system_controller_get_soc_id(u32 *dev, u32 *rev)
+{
+	if (of_machine_is_compatible("marvell,armada38x") &&
+		system_controller_base) {
+		*dev = readl(system_controller_base + mvebu_sc->dev_id) >> 16;
+		*rev = (readl(system_controller_base + mvebu_sc->rev_id) >> 8)
+			& 0xF;
+		return 0;
+	} else
+		return -ENODEV;
+}
+
 #ifdef CONFIG_SMP
 void mvebu_system_controller_set_cpu_boot_addr(void *boot_addr)
 {
Signature · 1 line
1.8.1.2
#5 lore raw

PATCH v2 2/2 ARM: mvebu: Use system controller to get the soc id when possible

arch/arm/mach-mvebu/mvebu-soc-id.c line 31

Quoted 12 lines · Gregory CLEMENT
arch/arm/mach-mvebu/mvebu-soc-id.c +2 -0
index 12c66cac967d..922b1380b0be 100644
@@ -25,8 +25,10 @@
 #include <linux/of_address.h>
 #include <linux/slab.h>
 #include <linux/sys_soc.h>
+#include "common.h"
 #include "mvebu-soc-id.h"
 
+

Spurious whitespace?

Quoted 3 lines
arch/arm/mach-mvebu/mvebu-soc-id.c +0 -0
 #define PCIE_DEV_ID_OFF		0x0
 #define PCIE_DEV_REV_OFF	0x8
Signature · 3 lines
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
#6 lore raw

PATCH v2 2/2 ARM: mvebu: Use system controller to get the soc id when possible

In reply to Gregory CLEMENT
Quoted 35 lines · Gregory CLEMENT

On Armada 38x it is possible to get the SoC Id and the revision without using the PCI register. Accessing the PCI registers implies enabling its clock and, because of the initialization issue, not keeping them enable. So if possible it is better to avoid it.

Armada 370 and Armada XP provides the SoC ID values from the system controller but not the revision.

Armada 375 provides both but the SoC ID value looks buggy (0x6660 instead of 0x6720).

Signed-off-by Gregory CLEMENT <[email protected]>
---
 arch/arm/mach-mvebu/common.h            |  1 +
 arch/arm/mach-mvebu/mvebu-soc-id.c      | 20 +++++++++++++++++++-
 arch/arm/mach-mvebu/system-controller.c | 19 +++++++++++++++++++
 3 files changed, 39 insertions(+), 1 deletion(-)
arch/arm/mach-mvebu/common.h +1 -0
index b67fb7a10d8b..927d88627a6e 100644
@@ -21,6 +21,7 @@ void mvebu_restart(enum reboot_mode mode, const char *cmd);
 int mvebu_cpu_reset_deassert(int cpu);
 void mvebu_pmsu_set_cpu_boot_addr(int hw_cpu, void *boot_addr);
 void mvebu_system_controller_set_cpu_boot_addr(void *boot_addr);
+int mvebu_system_controller_get_soc_id(u32 *dev, u32 *rev);
 
 void armada_xp_cpu_die(unsigned int cpu);
arch/arm/mach-mvebu/mvebu-soc-id.c +0 -0
index 12c66cac967d..922b1380b0be 100644

arch/arm/mach-mvebu/mvebu-soc-id.c line 32

Quoted 8 lines
arch/arm/mach-mvebu/mvebu-soc-id.c +2 -0
@@ -25,8 +25,10 @@
 #include <linux/of_address.h>
 #include <linux/slab.h>
 #include <linux/sys_soc.h>
+#include "common.h"
 #include "mvebu-soc-id.h"
 
+
 #define PCIE_DEV_ID_OFF		0x0

whitespace?

Quoted 12 lines
arch/arm/mach-mvebu/mvebu-soc-id.c +1 -1
 #define PCIE_DEV_REV_OFF	0x8
 

@@ -54,7 +56,7 @@ int mvebu_get_soc_id(u32 *dev, u32 *rev)
 		return -ENODEV;
 }
 
-static int __init mvebu_soc_id_init(void)
+static int __init get_soc_id_by_pci(void)
 {
 	struct device_node *np;
 	int ret = 0;

arch/arm/mach-mvebu/mvebu-soc-id.c clk_err: · line 140

Quoted 10 lines
arch/arm/mach-mvebu/mvebu-soc-id.c +7 -0
@@ -129,6 +131,22 @@ clk_err:
 
 	return ret;
 }
+
+static int __init mvebu_soc_id_init(void)
+{
+
+	/*
+	 * First try to get the ID and the revision by the system
+	 * regsiter and use PCI registers only if it is not possible

s/regsiter/register/

Quoted 45 lines
arch/arm/mach-mvebu/mvebu-soc-id.c +9 -0
+	 */
+	if (!mvebu_system_controller_get_soc_id(&soc_dev_id, &soc_rev)) {
+		is_id_valid = true;
+		pr_info("MVEBU SoC ID=0x%X, Rev=0x%X\n", soc_dev_id, soc_rev);
+		return 0;
+	}
+
+	return get_soc_id_by_pci();
+}
 early_initcall(mvebu_soc_id_init);
 
 static int __init mvebu_soc_device(void)
arch/arm/mach-mvebu/system-controller.c +7 -0
index 0c5524ac75b7..de90821129da 100644
@@ -39,6 +39,9 @@ struct mvebu_system_controller {
 	u32 system_soft_reset;
 
 	u32 resume_boot_addr;
+
+	u32 dev_id;
+	u32 rev_id;
 };
 static struct mvebu_system_controller *mvebu_sc;
 

@@ -47,6 +50,8 @@ static const struct mvebu_system_controller armada_370_xp_system_controller = {
 	.system_soft_reset_offset = 0x64,
 	.rstoutn_mask_reset_out_en = 0x1,
 	.system_soft_reset = 0x1,
+	.dev_id = 0x38,
+	.rev_id = 0x3c,
 };
 
 static const struct mvebu_system_controller armada_375_system_controller = {

@@ -55,6 +60,8 @@ static const struct mvebu_system_controller armada_375_system_controller = {
 	.rstoutn_mask_reset_out_en = 0x1,
 	.system_soft_reset = 0x1,
 	.resume_boot_addr = 0xd4,
+	.dev_id = 0x38,
+	.rev_id = 0x3c,
 };
 
 static const struct mvebu_system_controller orion_system_controller = {

arch/arm/mach-mvebu/system-controller.c void mvebu_restart(enum reboot_mode mode, const char *cmd) · line 113

Quoted 6 lines
arch/arm/mach-mvebu/system-controller.c +3 -0
@@ -101,6 +108,18 @@ void mvebu_restart(enum reboot_mode mode, const char *cmd)
 		;
 }
 
+int mvebu_system_controller_get_soc_id(u32 *dev, u32 *rev)
+{
+	if (of_machine_is_compatible("marvell,armada38x") &&

As Sergei commented on your documentation patch, we try to avoid wildcards in compatible strings. Perhaps a match list would be more appropriate?

Quoted 15 lines
arch/arm/mach-mvebu/system-controller.c +9 -0
+		system_controller_base) {
+		*dev = readl(system_controller_base + mvebu_sc->dev_id) >> 16;
+		*rev = (readl(system_controller_base + mvebu_sc->rev_id) >> 8)
+			& 0xF;
+		return 0;
+	} else
+		return -ENODEV;
+}
+
 #ifdef CONFIG_SMP
 void mvebu_system_controller_set_cpu_boot_addr(void *boot_addr)
 {
Signature · 1 line
1.8.1.2

thx,

Jason.

maintainer
#7 lore raw

PATCH v2 2/2 ARM: mvebu: Use system controller to get the soc id when possible

Quoted 7 lines

arch/arm/mach-mvebu/system-controller.c void mvebu_restart(enum reboot_mode mode, const char *cmd) · line 113

Quoted 3 lines
arch/arm/mach-mvebu/system-controller.c +3 -0
+int mvebu_system_controller_get_soc_id(u32 *dev, u32 *rev)
+{
+	if (of_machine_is_compatible("marvell,armada38x") &&

As Sergei commented on your documentation patch, we try to avoid wildcards in compatible strings. Perhaps a match list would be more appropriate?

I'm being pedantic, but in this invocation, it is not a wildcard. It will match one and only one compatible string.

However, we have the issue that 38x is not introduced in this patch, it has been there for a while. So would you like another patch series removing it from the DT files? A quick grep suggests nothing is using "marvell,armada38x" except this patch. So it should not be a backwards compatible issue.

Andrew

#8 lore raw

PATCH v2 2/2 ARM: mvebu: Use system controller to get the soc id when possible

Quoted 8 lines · Andrew Lunn
Quoted 7 lines

arch/arm/mach-mvebu/system-controller.c void mvebu_restart(enum reboot_mode mode, const char *cmd) · line 113

Quoted 3 lines
arch/arm/mach-mvebu/system-controller.c +3 -0
+int mvebu_system_controller_get_soc_id(u32 *dev, u32 *rev)
+{
+	if (of_machine_is_compatible("marvell,armada38x") &&

As Sergei commented on your documentation patch, we try to avoid wildcards in compatible strings. Perhaps a match list would be more appropriate?

Quoted 6 lines

I'm being pedantic, but in this invocation, it is not a wildcard. It will match one and only one compatible string.

However, we have the issue that 38x is not introduced in this patch, it has been there for a while. So would you like another patch series removing it from the DT files?

Please do. Sorry I missed it the first time around. Also, may as well put the documentation patch into the same series.

Quoted 2 lines

A quick grep suggests nothing is using "marvell,armada38x" except this patch. So it should not be a backwards compatible issue.

Agreed.

thx,

Jason.

maintainer
#9 lore raw

v2 cover Improve mvebu_get_soc_id

In reply to Gregory CLEMENT
Quoted 7 lines · Gregory CLEMENT

Hi,

The first use -ENODEV as return value instead of -1 as suggested by Arnd for the second patch.

The second patch allows to get the SoC Id and the revision without using the PCI register on Armada 38x.

Hi Gregory

I just tested on Kirkwood. No regression.

Once we get the compatible string worked out, i will post a Tested-by:

Andrew