Fix i2c bus hang on A0 version of the Armada XP SoCs

All the mvebu SoCs have information related to their variant and revision that can be read from the PCI control register.

This patch adds support for Armada XP and Armada 370. This reading of the revision and the ID are done before the PCI initialization to avoid any conflicts. Once these data are retrieved, the resources are freed to let the PCI subsystem use it.

Cc stable at vger.kernel.org

Signed-off-by Gregory CLEMENT <[email protected]>

---
 arch/arm/mach-mvebu/Makefile       |   2 +-
 arch/arm/mach-mvebu/mvebu-soc-id.c | 111 +++++++++++++++++++++++++++++++++++++
 include/linux/mvebu-soc-id.h       |  32 +++++++++++
 3 files changed, 144 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/mach-mvebu/mvebu-soc-id.c
 create mode 100644 include/linux/mvebu-soc-id.h

arch/arm/mach-mvebu/Makefile +1 -1

index 2d04f0e21870..878aebe98dcc 100644
@@ -3,7 +3,7 @@ ccflags-$(CONFIG_ARCH_MULTIPLATFORM) := -I$(srctree)/$(src)/include \
 
 AFLAGS_coherency_ll.o		:= -Wa,-march=armv7-a
 
-obj-y				 += system-controller.o
+obj-y				 += system-controller.o mvebu-soc-id.o
 obj-$(CONFIG_MACH_ARMADA_370_XP) += armada-370-xp.o
 obj-$(CONFIG_ARCH_MVEBU)	 += coherency.o coherency_ll.o pmsu.o
 obj-$(CONFIG_SMP)                += platsmp.o headsmp.o

arch/arm/mach-mvebu/mvebu-soc-id.c new file +57 -0

new file mode 100644
index 000000000000..86c901be284c
@@ -0,0 +1,111 @@
+/*
+ * Variant and revision information for mvebu SoCs
+ *
+ * Copyright (C) 2014 Marvell
+ *
+ * Gregory CLEMENT <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ *
+ * All the mvebu SoCs have information related to their variant and
+ * revision that can be read from the PCI control register. This is
+ * done before the PCI initialization to avoid any conflict. Once the
+ * ID and revision are retrieved, the mapping is freed.
+ */
+
+#include <linux/clk.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mvebu-soc-id.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+
+#define PCIE_DEV_ID_OFF		0x0
+#define PCIE_DEV_REV_OFF	0x8
+
+#define SOC_ID_MASK	    0xFFFF0000
+#define SOC_REV_MASK	    0xFF
+
+static u32 soc_dev_id;
+static u32 soc_rev;
+static bool is_id_valid;
+
+static const struct of_device_id mvebu_pcie_of_match_table[] = {
+	{ .compatible = "marvell,armada-xp-pcie", },
+	{ .compatible = "marvell,armada-370-pcie", },
+	{},
+};
+
+int mvebu_get_soc_id(u32 *dev, u32 *rev)
+{
+	if (is_id_valid) {
+		*dev = soc_dev_id;
+		*rev = soc_rev;
+		return 0;
+	} else
+		return -1;
+}
+
+EXPORT_SYMBOL(mvebu_get_soc_id);
+
+static int __init mvebu_soc_id_init(void)
+{
+	struct device_node *np;
+	int ret = 0;

arch/arm/mach-mvebu/mvebu-soc-id.c new file +44 -0

+
+		clk = of_clk_get_by_name(child, NULL);
+		if (IS_ERR(clk)) {
+			pr_err("%s: cannot get clock\n", __func__);
+			ret = -ENOMEM;
+			goto clk_err;
+		}
+
+		ret = clk_prepare_enable(clk);
+		if (ret) {
+			pr_err("%s: cannot enable clock\n", __func__);
+			goto clk_err;
+		}
+
+		pci_base = of_iomap(child, 0);
+		if (IS_ERR(pci_base)) {
+			pr_err("%s: cannot map registers\n",  __func__);
+			ret = -ENOMEM;
+			goto res_ioremap;
+		}
+
+		/* SoC ID */
+		soc_dev_id = __raw_readl(pci_base + PCIE_DEV_ID_OFF) >> 16;
+
+		/* SoC revision */
+		soc_rev = __raw_readl(pci_base + PCIE_DEV_REV_OFF)
+			& SOC_REV_MASK;
+
+		is_id_valid = true;
+
+		iounmap(pci_base);
+
+res_ioremap:
+		clk_disable_unprepare(clk);
+
+clk_err:
+		of_node_put(child);
+		of_node_put(np);
+	}
+
+	return ret;
+}
+arch_initcall(mvebu_soc_id_init);
+

include/linux/mvebu-soc-id.h new file +32 -0

new file mode 100644
index 000000000000..602ce1c50d1d
@@ -0,0 +1,32 @@
+/*
+ * Marvell EBU SoC ID and revision definitions.
+ *
+ * Copyright (C) 2014 Marvell Semiconductor
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#ifndef __LINUX_MVEBU_SOC_ID_H
+#define __LINUX_MVEBU_SOC_ID_H
+
+/* Armada XP ID */
+#define MV78230_DEV_ID	    0x7823
+#define MV78260_DEV_ID	    0x7826
+#define MV78460_DEV_ID	    0x7846
+
+/* Armada XP Revision */
+#define MV78XX0_A0_REV	    0x1
+#define MV78XX0_B0_REV	    0x2
+
+#ifdef CONFIG_ARCH_MVEBU
+int mvebu_get_soc_id(u32 *dev, u32 *rev);
+#else
+int mvebu_get_soc_id(u32 *dev, u32 *rev)
+{
+	return -1;
+}
+#endif
+
+#endif /* __LINUX_MVEBU_SOC_ID_H */

Signature · 1 line

1.8.1.2

Quoted 94 lines · Gregory CLEMENT

All the mvebu SoCs have information related to their variant and revision that can be read from the PCI control register.

This patch adds support for Armada XP and Armada 370. This reading of the revision and the ID are done before the PCI initialization to avoid any conflicts. Once these data are retrieved, the resources are freed to let the PCI subsystem use it.

Cc stable at vger.kernel.org

Signed-off-by Gregory CLEMENT <[email protected]>

---
 arch/arm/mach-mvebu/Makefile       |   2 +-
 arch/arm/mach-mvebu/mvebu-soc-id.c | 111 +++++++++++++++++++++++++++++++++++++
 include/linux/mvebu-soc-id.h       |  32 +++++++++++
 3 files changed, 144 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/mach-mvebu/mvebu-soc-id.c
 create mode 100644 include/linux/mvebu-soc-id.h

arch/arm/mach-mvebu/Makefile +1 -1

index 2d04f0e21870..878aebe98dcc 100644
@@ -3,7 +3,7 @@ ccflags-$(CONFIG_ARCH_MULTIPLATFORM) := -I$(srctree)/$(src)/include \
 
 AFLAGS_coherency_ll.o		:= -Wa,-march=armv7-a
 
-obj-y				 += system-controller.o
+obj-y				 += system-controller.o mvebu-soc-id.o
 obj-$(CONFIG_MACH_ARMADA_370_XP) += armada-370-xp.o
 obj-$(CONFIG_ARCH_MVEBU)	 += coherency.o coherency_ll.o pmsu.o
 obj-$(CONFIG_SMP)                += platsmp.o headsmp.o

arch/arm/mach-mvebu/mvebu-soc-id.c new file +57 -0

new file mode 100644
index 000000000000..86c901be284c
@@ -0,0 +1,111 @@
+/*
+ * Variant and revision information for mvebu SoCs
+ *
+ * Copyright (C) 2014 Marvell
+ *
+ * Gregory CLEMENT <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ *
+ * All the mvebu SoCs have information related to their variant and
+ * revision that can be read from the PCI control register. This is
+ * done before the PCI initialization to avoid any conflict. Once the
+ * ID and revision are retrieved, the mapping is freed.
+ */
+
+#include <linux/clk.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mvebu-soc-id.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+
+#define PCIE_DEV_ID_OFF		0x0
+#define PCIE_DEV_REV_OFF	0x8
+
+#define SOC_ID_MASK	    0xFFFF0000
+#define SOC_REV_MASK	    0xFF
+
+static u32 soc_dev_id;
+static u32 soc_rev;
+static bool is_id_valid;
+
+static const struct of_device_id mvebu_pcie_of_match_table[] = {
+	{ .compatible = "marvell,armada-xp-pcie", },
+	{ .compatible = "marvell,armada-370-pcie", },
+	{},
+};
+
+int mvebu_get_soc_id(u32 *dev, u32 *rev)
+{
+	if (is_id_valid) {
+		*dev = soc_dev_id;
+		*rev = soc_rev;
+		return 0;
+	} else
+		return -1;
+}
+
+EXPORT_SYMBOL(mvebu_get_soc_id);
+
+static int __init mvebu_soc_id_init(void)
+{
+	struct device_node *np;
+	int ret = 0;

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

Quoted 10 lines

arch/arm/mach-mvebu/mvebu-soc-id.c new file +10 -0

+
+	np = of_find_matching_node(NULL, mvebu_pcie_of_match_table);
+	if (np) {
+		void __iomem *pci_base;
+		struct clk *clk;
+		/*
+		 * ID and revision are available from any port, so we
+		 * just pick the first one
+		 */
+		struct device_node *child = of_get_next_child(np, NULL);

Hi Gregory

I'm away from my hardware at the moment.

Does this work when all the PCIe ports have status = "disabled";? We have many kirkwood devices in NAS boxes which don't use PCIe, so all the ports are disabled. But they still exist in the SoC, so we can read the IDs from them. I just don't know if of_get_next_child() will only return enabled children?

Thanks
Andrew

Quoted 85 lines

arch/arm/mach-mvebu/mvebu-soc-id.c new file +44 -0

+
+		clk = of_clk_get_by_name(child, NULL);
+		if (IS_ERR(clk)) {
+			pr_err("%s: cannot get clock\n", __func__);
+			ret = -ENOMEM;
+			goto clk_err;
+		}
+
+		ret = clk_prepare_enable(clk);
+		if (ret) {
+			pr_err("%s: cannot enable clock\n", __func__);
+			goto clk_err;
+		}
+
+		pci_base = of_iomap(child, 0);
+		if (IS_ERR(pci_base)) {
+			pr_err("%s: cannot map registers\n",  __func__);
+			ret = -ENOMEM;
+			goto res_ioremap;
+		}
+
+		/* SoC ID */
+		soc_dev_id = __raw_readl(pci_base + PCIE_DEV_ID_OFF) >> 16;
+
+		/* SoC revision */
+		soc_rev = __raw_readl(pci_base + PCIE_DEV_REV_OFF)
+			& SOC_REV_MASK;
+
+		is_id_valid = true;
+
+		iounmap(pci_base);
+
+res_ioremap:
+		clk_disable_unprepare(clk);
+
+clk_err:
+		of_node_put(child);
+		of_node_put(np);
+	}
+
+	return ret;
+}
+arch_initcall(mvebu_soc_id_init);
+

include/linux/mvebu-soc-id.h new file +32 -0

new file mode 100644
index 000000000000..602ce1c50d1d
@@ -0,0 +1,32 @@
+/*
+ * Marvell EBU SoC ID and revision definitions.
+ *
+ * Copyright (C) 2014 Marvell Semiconductor
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#ifndef __LINUX_MVEBU_SOC_ID_H
+#define __LINUX_MVEBU_SOC_ID_H
+
+/* Armada XP ID */
+#define MV78230_DEV_ID	    0x7823
+#define MV78260_DEV_ID	    0x7826
+#define MV78460_DEV_ID	    0x7846
+
+/* Armada XP Revision */
+#define MV78XX0_A0_REV	    0x1
+#define MV78XX0_B0_REV	    0x2
+
+#ifdef CONFIG_ARCH_MVEBU
+int mvebu_get_soc_id(u32 *dev, u32 *rev);
+#else
+int mvebu_get_soc_id(u32 *dev, u32 *rev)
+{
+	return -1;
+}
+#endif
+
+#endif /* __LINUX_MVEBU_SOC_ID_H */

Signature · 1 line

1.8.1.2

Quoted 108 lines · Andrew Lunn

Quoted 94 lines · Gregory CLEMENT

All the mvebu SoCs have information related to their variant and revision that can be read from the PCI control register.

This patch adds support for Armada XP and Armada 370. This reading of the revision and the ID are done before the PCI initialization to avoid any conflicts. Once these data are retrieved, the resources are freed to let the PCI subsystem use it.

Cc stable at vger.kernel.org

Signed-off-by Gregory CLEMENT <[email protected]>

---
 arch/arm/mach-mvebu/Makefile       |   2 +-
 arch/arm/mach-mvebu/mvebu-soc-id.c | 111 +++++++++++++++++++++++++++++++++++++
 include/linux/mvebu-soc-id.h       |  32 +++++++++++
 3 files changed, 144 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/mach-mvebu/mvebu-soc-id.c
 create mode 100644 include/linux/mvebu-soc-id.h

arch/arm/mach-mvebu/Makefile +1 -1

index 2d04f0e21870..878aebe98dcc 100644
@@ -3,7 +3,7 @@ ccflags-$(CONFIG_ARCH_MULTIPLATFORM) := -I$(srctree)/$(src)/include \
 
 AFLAGS_coherency_ll.o		:= -Wa,-march=armv7-a
 
-obj-y				 += system-controller.o
+obj-y				 += system-controller.o mvebu-soc-id.o
 obj-$(CONFIG_MACH_ARMADA_370_XP) += armada-370-xp.o
 obj-$(CONFIG_ARCH_MVEBU)	 += coherency.o coherency_ll.o pmsu.o
 obj-$(CONFIG_SMP)                += platsmp.o headsmp.o

arch/arm/mach-mvebu/mvebu-soc-id.c new file +57 -0

new file mode 100644
index 000000000000..86c901be284c
@@ -0,0 +1,111 @@
+/*
+ * Variant and revision information for mvebu SoCs
+ *
+ * Copyright (C) 2014 Marvell
+ *
+ * Gregory CLEMENT <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ *
+ * All the mvebu SoCs have information related to their variant and
+ * revision that can be read from the PCI control register. This is
+ * done before the PCI initialization to avoid any conflict. Once the
+ * ID and revision are retrieved, the mapping is freed.
+ */
+
+#include <linux/clk.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mvebu-soc-id.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+
+#define PCIE_DEV_ID_OFF		0x0
+#define PCIE_DEV_REV_OFF	0x8
+
+#define SOC_ID_MASK	    0xFFFF0000
+#define SOC_REV_MASK	    0xFF
+
+static u32 soc_dev_id;
+static u32 soc_rev;
+static bool is_id_valid;
+
+static const struct of_device_id mvebu_pcie_of_match_table[] = {
+	{ .compatible = "marvell,armada-xp-pcie", },
+	{ .compatible = "marvell,armada-370-pcie", },
+	{},
+};
+
+int mvebu_get_soc_id(u32 *dev, u32 *rev)
+{
+	if (is_id_valid) {
+		*dev = soc_dev_id;
+		*rev = soc_rev;
+		return 0;
+	} else
+		return -1;
+}
+
+EXPORT_SYMBOL(mvebu_get_soc_id);
+
+static int __init mvebu_soc_id_init(void)
+{
+	struct device_node *np;
+	int ret = 0;

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

Quoted 10 lines

arch/arm/mach-mvebu/mvebu-soc-id.c new file +10 -0

+
+	np = of_find_matching_node(NULL, mvebu_pcie_of_match_table);
+	if (np) {
+		void __iomem *pci_base;
+		struct clk *clk;
+		/*
+		 * ID and revision are available from any port, so we
+		 * just pick the first one
+		 */
+		struct device_node *child = of_get_next_child(np, NULL);

Hi Gregory

Quoted 7 lines

I'm away from my hardware at the moment.

Does this work when all the PCIe ports have status = "disabled";? We have many kirkwood devices in NAS boxes which don't use PCIe, so all the ports are disabled. But they still exist in the SoC, so we can read the IDs from them. I just don't know if of_get_next_child() will only return enabled children?

There is a function named of_get_next_available_child, so I assumed that of_get_next_child() will return all the children. But I can test it to be sure of it.

Quoted 89 lines

Thanks
Andrew

Quoted 85 lines

arch/arm/mach-mvebu/mvebu-soc-id.c new file +44 -0

+
+		clk = of_clk_get_by_name(child, NULL);
+		if (IS_ERR(clk)) {
+			pr_err("%s: cannot get clock\n", __func__);
+			ret = -ENOMEM;
+			goto clk_err;
+		}
+
+		ret = clk_prepare_enable(clk);
+		if (ret) {
+			pr_err("%s: cannot enable clock\n", __func__);
+			goto clk_err;
+		}
+
+		pci_base = of_iomap(child, 0);
+		if (IS_ERR(pci_base)) {
+			pr_err("%s: cannot map registers\n",  __func__);
+			ret = -ENOMEM;
+			goto res_ioremap;
+		}
+
+		/* SoC ID */
+		soc_dev_id = __raw_readl(pci_base + PCIE_DEV_ID_OFF) >> 16;
+
+		/* SoC revision */
+		soc_rev = __raw_readl(pci_base + PCIE_DEV_REV_OFF)
+			& SOC_REV_MASK;
+
+		is_id_valid = true;
+
+		iounmap(pci_base);
+
+res_ioremap:
+		clk_disable_unprepare(clk);
+
+clk_err:
+		of_node_put(child);
+		of_node_put(np);
+	}
+
+	return ret;
+}
+arch_initcall(mvebu_soc_id_init);
+

include/linux/mvebu-soc-id.h new file +32 -0

new file mode 100644
index 000000000000..602ce1c50d1d
@@ -0,0 +1,32 @@
+/*
+ * Marvell EBU SoC ID and revision definitions.
+ *
+ * Copyright (C) 2014 Marvell Semiconductor
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#ifndef __LINUX_MVEBU_SOC_ID_H
+#define __LINUX_MVEBU_SOC_ID_H
+
+/* Armada XP ID */
+#define MV78230_DEV_ID	    0x7823
+#define MV78260_DEV_ID	    0x7826
+#define MV78460_DEV_ID	    0x7846
+
+/* Armada XP Revision */
+#define MV78XX0_A0_REV	    0x1
+#define MV78XX0_B0_REV	    0x2
+
+#ifdef CONFIG_ARCH_MVEBU
+int mvebu_get_soc_id(u32 *dev, u32 *rev);
+#else
+int mvebu_get_soc_id(u32 *dev, u32 *rev)
+{
+	return -1;
+}
+#endif
+
+#endif /* __LINUX_MVEBU_SOC_ID_H */

Signature · 1 line

1.8.1.2

Quoted 10 lines

Quoted 9 lines

Hi Gregory

I'm away from my hardware at the moment.

Does this work when all the PCIe ports have status = "disabled";? We have many kirkwood devices in NAS boxes which don't use PCIe, so all the ports are disabled. But they still exist in the SoC, so we can read the IDs from them. I just don't know if of_get_next_child() will only return enabled children?

Quoted 4 lines

There is a function named of_get_next_available_child, so I assumed that of_get_next_child() will return all the children. But I can test it to be sure of it.

I have just removed all the PCIe part in the armada-xp-openblocks-ax3-4.dts file (PCie is disable by default in the dtsi file) and it worled as expected! :)

by the way waht do you think of adding this line in at the end of the mvebu_soc_id_init() function:

pr_info("MVEBU SoC ID=0x%X, Rev=0x%X\n", soc_dev_id, soc_rev);

Also keep in mind that currently you can't use it for kirkwood because the build of the file depend on CONFIG_ARCH_MVEBU. But as kirkwood will soon joined the mach-mvebu directory, it won't be a problem then.

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

Hi Gregory

I'm away from my hardware at the moment.

Does this work when all the PCIe ports have status = "disabled";? We have many kirkwood devices in NAS boxes which don't use PCIe, so all the ports are disabled. But they still exist in the SoC, so we can read the IDs from them. I just don't know if of_get_next_child() will only return enabled children?

Quoted 8 lines

Quoted 4 lines

There is a function named of_get_next_available_child, so I assumed that of_get_next_child() will return all the children. But I can test it to be sure of it.

I have just removed all the PCIe part in the armada-xp-openblocks-ax3-4.dts file (PCie is disable by default in the dtsi file) and it worled as expected! :)

Great, thanks for testing.

Quoted 4 lines

by the way waht do you think of adding this line in at the end of the mvebu_soc_id_init() function:

pr_info("MVEBU SoC ID=0x%X, Rev=0x%X\n", soc_dev_id, soc_rev);

Kirkwood prints actual strings, not numbers. More readable.

Quoted 4 lines

Also keep in mind that currently you can't use it for kirkwood because the build of the file depend on CONFIG_ARCH_MVEBU. But as kirkwood will soon joined the mach-mvebu directory, it won't be a problem then.

I think it is possible to do ../mach-mvebu/soc_id.o sort of thing in the Makefile. Ugly, but might work until we move.

Andrew

Dear Gregory CLEMENT,

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

Quoted 7 lines · Gregory CLEMENT

arch/arm/mach-mvebu/mvebu-soc-id.c new file +7 -0

+static int __init mvebu_soc_id_init(void)
+{
+	struct device_node *np;
+	int ret = 0;
+
+	np = of_find_matching_node(NULL, mvebu_pcie_of_match_table);
+	if (np) {

Change this to:

	if (!np)
		return 0;

so that you avoid indenting the entire function code inside the if { ... } block.

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

Quoted 8 lines

arch/arm/mach-mvebu/mvebu-soc-id.c new file +7 -0

+		void __iomem *pci_base;
+		struct clk *clk;
+		/*
+		 * ID and revision are available from any port, so we
+		 * just pick the first one
+		 */
+		struct device_node *child = of_get_next_child(np, NULL);

Maybe check that child is not NULL here?

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

Quoted 5 lines

arch/arm/mach-mvebu/mvebu-soc-id.c new file +4 -0

+
+		clk = of_clk_get_by_name(child, NULL);
+		if (IS_ERR(clk)) {
+			pr_err("%s: cannot get clock\n", __func__);

I think you should rather do:

#define pr_fmt(fmt) "mvebu-soc-id: " fmt

at the beginning of your C file and get rid of the "%s" for the __func__.

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

Quoted 7 lines

arch/arm/mach-mvebu/mvebu-soc-id.c new file +6 -0

+		/* SoC ID */
+		soc_dev_id = __raw_readl(pci_base + PCIE_DEV_ID_OFF) >> 16;
+
+		/* SoC revision */
+		soc_rev = __raw_readl(pci_base + PCIE_DEV_REV_OFF)
+			& SOC_REV_MASK;

Use readl() for both of these reads. __raw_readl() will not work properly when the system is booted big-endian.

Quoted 4 lines

arch/arm/mach-mvebu/mvebu-soc-id.c new file +3 -0

+	return ret;
+}
+arch_initcall(mvebu_soc_id_init);

include/linux/mvebu-soc-id.h line 26

Quoted 4 lines

include/linux/mvebu-soc-id.h new file +4 -0

+#ifdef CONFIG_ARCH_MVEBU
+int mvebu_get_soc_id(u32 *dev, u32 *rev);
+#else
+int mvebu_get_soc_id(u32 *dev, u32 *rev)

Missing "static inline". Without these, if this header file is included more than once, you will have two times the same symbol defined.

Best regards,

Thomas

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

Quoted 6 lines · Gregory CLEMENT

arch/arm/mach-mvebu/mvebu-soc-id.c new file +6 -0

+		/* SoC ID */
+		soc_dev_id = __raw_readl(pci_base + PCIE_DEV_ID_OFF) >> 16;
+
+		/* SoC revision */
+		soc_rev = __raw_readl(pci_base + PCIE_DEV_REV_OFF)
+			& SOC_REV_MASK;

Sort of a minor nit, but I'm not actually sure these registers are read-only :( I know the documentation doesn't describe how to change them, but in PCI-E EndPoint mode they are read by the host so the firmware must be able to change them to reflect the firmware running on the endpoint..

Which is to say, I suppose the bootloader could program them to something else if it wanted to.

That said, in practice obviously they will not be changed, but maybe there is another ID register you can read?

Jason

All the mvebu SoCs have information related to their variant and revision that can be read from the PCI control register.

This patch adds support for Armada XP and Armada 370. This reading of the revision and the ID are done before the PCI initialization to avoid any conflicts. Once these data are retrieved, the resources are freed to let the PCI subsystem use it.

arch/arm/mach-mvebu/mvebu-soc-id.c new file +7 -0

+static const struct of_device_id mvebu_pcie_of_match_table[] = {
+	{ .compatible = "marvell,armada-xp-pcie", },
+	{ .compatible = "marvell,armada-370-pcie", },
+	{},
+};
+
+int mvebu_get_soc_id(u32 *dev, u32 *rev)

arch/arm/mach-mvebu/mvebu-soc-id.c new file +4 -0

+static int __init mvebu_soc_id_init(void)
+{
+	struct device_node *np;
+	int ret = 0;

Quoted 9 lines

arch/arm/mach-mvebu/mvebu-soc-id.c new file +9 -0

+static int __init mvebu_soc_id_init(void)
+{
+	struct device_node *np;
+	int ret = 0;
+
+	np = of_find_matching_node(NULL, mvebu_pcie_of_match_table);
+	if (np) {
+		void __iomem *pci_base;
+		struct clk *clk;

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

Quoted 5 lines

arch/arm/mach-mvebu/mvebu-soc-id.c new file +5 -0

+		/*
+		 * ID and revision are available from any port, so we
+		 * just pick the first one
+		 */
+		struct device_node *child = of_get_next_child(np, NULL);

I guess all this will fail if for some reason the PCIe node is not present on machines that don't use PCIe.

Quoted 9 lines

Quoted 9 lines

arch/arm/mach-mvebu/mvebu-soc-id.c new file +9 -0

+static int __init mvebu_soc_id_init(void)
+{
+	struct device_node *np;
+	int ret = 0;
+
+	np = of_find_matching_node(NULL, mvebu_pcie_of_match_table);
+	if (np) {
+		void __iomem *pci_base;
+		struct clk *clk;

Quoted 8 lines

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

Quoted 5 lines

arch/arm/mach-mvebu/mvebu-soc-id.c new file +5 -0

+		/*
+		 * ID and revision are available from any port, so we
+		 * just pick the first one
+		 */
+		struct device_node *child = of_get_next_child(np, NULL);

I guess all this will fail if for some reason the PCIe node is not present on machines that don't use PCIe.

Hi Arnd

That would be rather odd. These nodes are in the top level SoC dtsi file. When they are not used, they have status = "disabled" and are in the dtb blob with this state.

Quoted 18 lines · Andrew Lunn

Quoted 17 lines

Quoted 4 lines

arch/arm/mach-mvebu/mvebu-soc-id.c new file +4 -0

+static int __init mvebu_soc_id_init(void)
+{
+	struct device_node *np;
+	int ret = 0;

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

Quoted 10 lines

arch/arm/mach-mvebu/mvebu-soc-id.c new file +10 -0

+
+	np = of_find_matching_node(NULL, mvebu_pcie_of_match_table);
+	if (np) {
+		void __iomem *pci_base;
+		struct clk *clk;
+		/*
+		 * ID and revision are available from any port, so we
+		 * just pick the first one
+		 */
+		struct device_node *child = of_get_next_child(np, NULL);

I guess all this will fail if for some reason the PCIe node is not present on machines that don't use PCIe.

Quoted 3 lines

That would be rather odd. These nodes are in the top level SoC dtsi file. When they are not used, they have status = "disabled" and are in the dtb blob with this state.

Hang on, you can't safely read from a disabled PCI node, it could have been powered down by the bootloader..

If you mean clock-gated with "powered down", the code is safe. It enables the clock gate prior reading from the controller. Or is there another way to power down the controller, so you cannot read from the controller registers?

Quoted 4 lines · Sebastian Hesselbarth

If you mean clock-gated with "powered down", the code is safe. It enables the clock gate prior reading from the controller. Or is there another way to power down the controller, so you cannot read from the controller registers?

There is a clock gate and a power down on kirkwood at least, Linux has no code for controlling the powerdown

In any event, I think processing a disabled DT node is not great..

Quoted 7 lines · Jason Gunthorpe

Quoted 4 lines · Sebastian Hesselbarth

If you mean clock-gated with "powered down", the code is safe. It enables the clock gate prior reading from the controller. Or is there another way to power down the controller, so you cannot read from the controller registers?

Quoted 2 lines

There is a clock gate and a power down on kirkwood at least, Linux has no code for controlling the powerdown

Does that power down really disable reading from PCIe controller registers or is it just PHY power down?

Quoted 1 line

In any event, I think processing a disabled DT node is not great..

Yeah, but you see another way to get the PCIe controller registers instead? Or any other common id register to read? IMHO PCIe ids are the best we can find here and Gregory found the first IP that really depends on the SoC revision..

Quoted 8 lines · Sebastian Hesselbarth

Quoted 7 lines · Jason Gunthorpe

Quoted 4 lines · Sebastian Hesselbarth

If you mean clock-gated with "powered down", the code is safe. It enables the clock gate prior reading from the controller. Or is there another way to power down the controller, so you cannot read from the controller registers?

Quoted 5 lines

Quoted 2 lines

There is a clock gate and a power down on kirkwood at least, Linux has no code for controlling the powerdown

Does that power down really disable reading from PCIe controller registers or is it just PHY power down?

I haven't experimented with it, but every block that has a clock gate has a power down, so I doubt it is just a phy power down.

Quoted 6 lines

Quoted 1 line

In any event, I think processing a disabled DT node is not great..

Yeah, but you see another way to get the PCIe controller registers instead? Or any other common id register to read? IMHO PCIe ids are the best we can find here and Gregory found the first IP that really depends on the SoC revision..

I don't know of another option off hand, unless something is encoded in the CPU ID register set.

Encoding the IP block version in the I2C DT compatible string is the next best choice, but it obviously isn't automatic..

Quoted 5 lines

Quoted 2 lines

Does that power down really disable reading from PCIe controller registers or is it just PHY power down?

I haven't experimented with it, but every block that has a clock gate has a power down, so I doubt it is just a phy power down.

Ok, I see. But it isn't documented in the public FS, is it? If there is an extra powerdown register for each ip block, I guess it will also break reading from its registers.

Quoted 9 lines

Quoted 5 lines

Quoted 2 lines

Does that power down really disable reading from PCIe controller registers or is it just PHY power down?

I haven't experimented with it, but every block that has a clock gate has a power down, so I doubt it is just a phy power down.

Ok, I see. But it isn't documented in the public FS, is it? If there is an extra powerdown register for each ip block, I guess it will also break reading from its registers.

Hi Sebastian

The public Kirkwood FS has a memory power management control register, Offset 0x20118. It is unclear what it actually does, and if you can still access registers when it is off. We would have to poke it and see.

Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi Andrew,

Quoted 10 lines · Andrew Lunn

Quoted 9 lines

Quoted 5 lines

Quoted 2 lines

Does that power down really disable reading from PCIe controller registers or is it just PHY power down?

I haven't experimented with it, but every block that has a clock gate has a power down, so I doubt it is just a phy power down.

Ok, I see. But it isn't documented in the public FS, is it? If there is an extra powerdown register for each ip block, I guess it will also break reading from its registers.

Quoted 6 lines

Hi Sebastian

The public Kirkwood FS has a memory power management control register, Offset 0x20118. It is unclear what it actually does, and if you can still access registers when it is off. We would have to poke it and see.

Interesting, this registers is mentioned under the section "Core Clock Power Saving" in the kirkwood datasheet, so maybe we should add this register to the gating clock

I found similar registers for Armada XP/370, I am going to test what happen if the PCxy Memory Power Down are down.

Thanks,

Gregory

Quoted 7 lines

Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

That would be rather odd. These nodes are in the top level SoC dtsi file. When they are not used, they have status = "disabled" and are in the dtb blob with this state.

The only reason i can think of them not being present at all is if somebody adds an optimizer to dtc which removed disabled nodes. What does the device tree spec say about that? Are we relying on undefined dtc behavior?

Quoted 4 lines · Andrew Lunn

That would be rather odd. These nodes are in the top level SoC dtsi file. When they are not used, they have status = "disabled" and are in the dtb blob with this state.

Quoted 4 lines

The only reason i can think of them not being present at all is if somebody adds an optimizer to dtc which removed disabled nodes. What does the device tree spec say about that? Are we relying on undefined dtc behavior?

There is no requirement to use the include files. If someone decides to ship a default dtb file in their boot loader, it wouldn't be a bug to leave the nodes out entirely.

This raises the question, should mainline try to support any random dtb blob, or only those that have ever shipped with mainline?

There are some older mainline DT blobs which won't have PCIe in them, since PCIe support was not there day 1. So returning -ENODEV, and the i2c controller assuming an A0 would make sense.

But what should we do if somebody was to boot linux with a FreeBSD DT blob? It is a valid blob, it describes the hardware, but the FreeBSD nodes have different compatibility strings, don't have clocks, etc. Now that is at the extreme of the range, but where do we put the marker for compatibility in this range from current mainline blobs to FreeBSD blobs?

Are you saying that FreeBSD has a different set of bindings for the same hardware?

That would be rather unfortunate and we should probably try to merge the bindings eventually and make sure that either OS can boot with any conforming DTB

On the example of missing clocks, it should work as long as all relevant clocks are enabled by the boot loader and the clock properties are optional the binding.

Quoted 3 lines

That would be rather unfortunate and we should probably try to merge the bindings eventually and make sure that either OS can boot with any conforming DTB

It probably requires one of the DT maintainers to talk to FreeBSD equivalents to get some coordination going. We have a lot of generic stuff, like gpio keys, gpio leds, cpu nodes, mtd partitions, etc, which could be done at a high level, and then SoC specific nodes sorted out between individual developers.

Quoted 3 lines

On the example of missing clocks, it should work as long as all relevant clocks are enabled by the boot loader and the clock properties are optional the binding.

However, not all clocks are optional. We need the clock in order to know how fast it ticks. So at least the serial ports and i2c will not work, and maybe other devices, i would have to check.

Quoted 7 lines · Gregory CLEMENT

All the mvebu SoCs have information related to their variant and revision that can be read from the PCI control register.

This patch adds support for Armada XP and Armada 370. This reading of the revision and the ID are done before the PCI initialization to avoid any conflicts. Once these data are retrieved, the resources are freed to let the PCI subsystem use it.

I like the idea of identifying the soc version for any number of reasons, but I would hope there was some way of doing this that didn't involve probing the PCI device. I know this is the traditional way on orion/kirkwood/dove/... but it always felt wrong to me.

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

Quoted 4 lines

arch/arm/mach-mvebu/mvebu-soc-id.c new file +3 -0

+static u32 soc_dev_id;
+static u32 soc_rev;
+static bool is_id_valid;

Would it be reasonable to reuse the global "system_rev" variable for this rather than a platform specific exported function?

Quoted 7 lines

arch/arm/mach-mvebu/mvebu-soc-id.c new file +7 -0

+static const struct of_device_id mvebu_pcie_of_match_table[] = {
+	{ .compatible = "marvell,armada-xp-pcie", },
+	{ .compatible = "marvell,armada-370-pcie", },
+	{},
+};
+
+int mvebu_get_soc_id(u32 *dev, u32 *rev)

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

Quoted 10 lines

arch/arm/mach-mvebu/mvebu-soc-id.c new file +10 -0

+{
+	if (is_id_valid) {
+		*dev = soc_dev_id;
+		*rev = soc_rev;
+		return 0;
+	} else
+		return -1;
+}
+
+EXPORT_SYMBOL(mvebu_get_soc_id);

Maybe EXPORT_SYMBOL_GPL, out of principle?

Quoted 9 lines

arch/arm/mach-mvebu/mvebu-soc-id.c new file +9 -0

+static int __init mvebu_soc_id_init(void)
+{
+	struct device_node *np;
+	int ret = 0;
+
+	np = of_find_matching_node(NULL, mvebu_pcie_of_match_table);
+	if (np) {
+		void __iomem *pci_base;
+		struct clk *clk;

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

Quoted 5 lines

arch/arm/mach-mvebu/mvebu-soc-id.c new file +5 -0

+		/*
+		 * ID and revision are available from any port, so we
+		 * just pick the first one
+		 */
+		struct device_node *child = of_get_next_child(np, NULL);

I guess all this will fail if for some reason the PCIe node is not present on machines that don't use PCIe.

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

The first variants of Armada XP SoCs (A0 stepping) have issues related to the i2c controller which prevent to use the offload mechanism and lead to a kernel hang during boot.

The driver now check the revision of the SoC. If the revision is not more recent than the A0 or if the driver can't get the SoC revision then it disables the offload mechanism.

Cc stable at vger.kernel.org

Signed-off-by Gregory CLEMENT <[email protected]>

Acked-by Wolfram Sang <[email protected]>

---
 drivers/i2c/busses/i2c-mv64xxx.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

drivers/i2c/busses/i2c-mv64xxx.c +10 -1

index 8be7e42aa4de..634b04d241fb 100644
@@ -24,6 +24,7 @@
 #include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/delay.h>
+#include <linux/mvebu-soc-id.h>
 
 #define MV64XXX_I2C_ADDR_ADDR(val)			((val & 0x7f) << 1)
 #define MV64XXX_I2C_BAUD_DIV_N(val)			(val & 0x7)

@@ -779,8 +780,16 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
 	 * Transaction Generator support and the errata fix.
 	 */
 	if (of_device_is_compatible(np, "marvell,mv78230-i2c")) {
-		drv_data->offload_enabled = true;
+		u32 dev, rev;
+
 		drv_data->errata_delay = true;
+		/*
+		 * Only revison more recent than A0 support offload
+		 * mechanism. In case we can't get the SoC revision
+		 * weplay safe and we don't enable it
+		 */
+		if (mvebu_get_soc_id(&rev, &dev) == 0 && dev > MV78XX0_A0_REV)
+			drv_data->offload_enabled = true;
 	}
 
 out:

Dear Gregory CLEMENT,

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data, · line 787

Quoted 2 lines · Gregory CLEMENT

drivers/i2c/busses/i2c-mv64xxx.c +2 -0

+		/*
+		 * Only revison more recent than A0 support offload

revison -> revisions

offload mechanism -> the offload mechanism

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data, · line 789

Quoted 2 lines

drivers/i2c/busses/i2c-mv64xxx.c +2 -0

+		 * mechanism. In case we can't get the SoC revision
+		 * weplay safe and we don't enable it

weplay -> we play

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data, · line 795

Quoted 6 lines

drivers/i2c/busses/i2c-mv64xxx.c +3 -0

+		 */
+		if (mvebu_get_soc_id(&rev, &dev) == 0 && dev > MV78XX0_A0_REV)
+			drv_data->offload_enabled = true;
 	}
 
 out:

Thanks!

Thomas

The first variants of Armada XP SoCs (A0 stepping) have issues related to the i2c controller which prevent to use the offload mechanism and lead to a kernel hang during boot.

The driver now check the revision of the SoC. If the revision is not more recent than the A0 or if the driver can't get the SoC revision then it disables the offload mechanism.

Cc stable at vger.kernel.org

Signed-off-by Gregory CLEMENT <[email protected]>

Acked-by Wolfram Sang <[email protected]>

Quoted 8 lines · Gregory CLEMENT

The first variants of Armada XP SoCs (A0 stepping) have issues related to the i2c controller which prevent to use the offload mechanism and lead to a kernel hang during boot.

The driver now check the revision of the SoC. If the revision is not more recent than the A0 or if the driver can't get the SoC revision then it disables the offload mechanism.

Quoted 3 lines

Cc stable at vger.kernel.org

Signed-off-by Gregory CLEMENT <[email protected]>

Acked-by Wolfram Sang <[email protected]>

Relying on the soc id patch for a "stable" bug fix seems a little far-reaching to me. I would be happier to first try to do a local detection based on the i2c bus device node itself. Do you know how

common the A0 revision is? You mention "early release of the OpenBlocks AX3-4 boards". Any others that you suspect? If not,

how about adding either a boolean property in the node or a new "compatible" value to distinguish the working version from the broken one?

If A0 is very common, you might do the same thing in the opposite way and default to "broken" unless it is explicitly known to be the good version. In general, I'm much in favor of keeping "quirks" local to device drivers if possible and not rely on global system state.

Arnd

Quoted 3 lines

Relying on the soc id patch for a "stable" bug fix seems a little far-reaching to me. I would be happier to first try to do a local detection based on the i2c bus device node itself. Do you know how

It was my first proposal in case adding the soc id detection was a too big things. But it turned out that the amount of code is very low so I really think it worth adding it along the fix. Device tree is supposed to be stable so as soon as we add something in it we are supposed support it forever. Moreover using device tree for something we can probe is counter productive.

Quoted 2 lines

common the A0 revision is? You mention "early release of the OpenBlocks AX3-4 boards". Any others that you suspect? If not,

No, from the info I gathered I expected that only OpenBlocks AX3-4 would be the only product shipped with an A0 version and as I said it should be only a limit amount of them.

Quoted 10 lines · Gregory CLEMENT

Quoted 3 lines

Relying on the soc id patch for a "stable" bug fix seems a little far-reaching to me. I would be happier to first try to do a local detection based on the i2c bus device node itself. Do you know how

It was my first proposal in case adding the soc id detection was a too big things. But it turned out that the amount of code is very low so I really think it worth adding it along the fix. Device tree is supposed to be stable so as soon as we add something in it we are supposed support it forever. Moreover using device tree for something we can probe is counter productive.

I would still be happier if we did both and only need to check the SoC version if the device is in the "possibly broken" category but default to the existing behavior.

My main concern is that this patch is adding platform code that we'd otherwise have to carry in the kernel indefinitely. I agree that it's best if we can probe stuff automatically, but that doesn't normally mean looking at an unrelated piece of information. If the i2c controller registers themselves tell us whether this device is broken or not, we should use that information. Looking at a global SoC version register however is more like checking a board ID in the pre-DT days where the board number is the only information we have and everything is derived from that.

Quoted 7 lines

Quoted 3 lines

common the A0 revision is? You mention "early release of the OpenBlocks AX3-4 boards". Any others that you suspect? If not,

No, from the info I gathered I expected that only OpenBlocks AX3-4 would be the only product shipped with an A0 version and as I said it should be only a limit amount of them.

Ok, good. So we really only need to worry about this one board for now and can make all the others default to normal operation without checking the SoC version.

Another idea: Could we have a quirk in the mvebu platform code for the AX3-4 to check the SoC version and then change the property for the i2c controller based on whether we expect it to work or not? This way, we wouldn't even need an interface between the platform code and the driver code.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 10 lines · Arnd Bergmann

On Monday 06 January 2014, Gregory CLEMENT wrote:
My main concern is that this patch is adding platform code that we'd otherwise have to carry in the kernel indefinitely. I agree that it's best if we can probe stuff automatically, but that doesn't normally mean looking at an unrelated piece of information. If the i2c controller registers themselves tell us whether this device is broken or not, we should use that information. Looking at a global SoC version register however is more like checking a board ID in the pre-DT days where the board number is the only information we have and everything is derived from that.

Well the way the hardware is designed is exactly like this: between two revision of a SoC you can have slightly differences between various IP and most of the time this IP don't have a specific register for it. Moreover from my experience a change done in a IP of a given revision of a SoC will be for this revision and not necessary reported in future generation of a SoC. Most of the time the IP are not really under a VCS. That means that the SoC ID is the only reliable information to know the version of most of the IP inside this SoC.

Quoted 5 lines

Another idea: Could we have a quirk in the mvebu platform code for the AX3-4 to check the SoC version and then change the property for the i2c controller based on whether we expect it to work or not? This way, we wouldn't even need an interface between the platform code and the driver code.

I can try this last approach: using the device tree to pass platform parameter from the arch part to the driver.