[PATCH v2 0/2] Fix i2c bus hang on A0 version of the Armada XP SoCs

[PATCH v2 0/2] Fix i2c bus hang on A0 version of the Armada XP SoCs

[Gregory CLEMENT]

Hi,

This the 2nd version of the series fixing the i2c bus hang on A0
version of the Armada XP SoCs. It occurred on the early release of the
OpenBlocks AX3-4 boards. Indeed 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 first patch add a mean to detect the SoCs version at run-time and
the second one use this feature in the driver.

These 2 patches should be applied on 3.13-rc and on stable kernel 3.12
as it fixes a regression introduce by the commit 930ab3d403ae "i2c:
mv64xxx: Add I2C Transaction Generator support".

The first patch could be latter be extend to also be used with dove,
kirkwood, orion5x and mv78x00 when there will be merged in mvebu.

Thanks,

Gregory

Changelog:

v1 -> v2:

- Changed the way to test the return of the function mvebu_get_soc_id
  in order to make it clearer.

- Removed the superfluous parentheses

- Added Wolfram's acked-by on the 2nd patch

Gregory CLEMENT (2):
  ARM: mvebu: Add support to get the ID and the revision of a SoC
  i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs

 arch/arm/mach-mvebu/Makefile       |   2 +-
 arch/arm/mach-mvebu/mvebu-soc-id.c | 111 +++++++++++++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-mv64xxx.c   |  11 +++-
 include/linux/mvebu-soc-id.h       |  32 +++++++++++
 4 files changed, 154 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm/mach-mvebu/mvebu-soc-id.c
 create mode 100644 include/linux/mvebu-soc-id.h

-- 
1.8.1.2

[PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC

[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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 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

diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
index 2d04f0e21870..878aebe98dcc 100644
--- a/arch/arm/mach-mvebu/Makefile
+++ b/arch/arm/mach-mvebu/Makefile
@@ -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
diff --git a/arch/arm/mach-mvebu/mvebu-soc-id.c b/arch/arm/mach-mvebu/mvebu-soc-id.c
new file mode 100644
index 000000000000..86c901be284c
--- /dev/null
+++ b/arch/arm/mach-mvebu/mvebu-soc-id.c
@@ -0,0 +1,111 @@
+/*
+ * Variant and revision information for mvebu SoCs
+ *
+ * Copyright (C) 2014 Marvell
+ *
+ * Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
+ *
+ * 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;
+
+	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);
+
+		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);
+
diff --git a/include/linux/mvebu-soc-id.h b/include/linux/mvebu-soc-id.h
new file mode 100644
index 000000000000..602ce1c50d1d
--- /dev/null
+++ b/include/linux/mvebu-soc-id.h
@@ -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 */
-- 
1.8.1.2

Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC

[Andrew Lunn]

On Fri, Jan 03, 2014 at 10:59:44AM +0100, Gregory CLEMENT wrote:
> 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@vger.kernel.org
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>  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
> 
> diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
> index 2d04f0e21870..878aebe98dcc 100644
> --- a/arch/arm/mach-mvebu/Makefile
> +++ b/arch/arm/mach-mvebu/Makefile
> @@ -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
> diff --git a/arch/arm/mach-mvebu/mvebu-soc-id.c b/arch/arm/mach-mvebu/mvebu-soc-id.c
> new file mode 100644
> index 000000000000..86c901be284c
> --- /dev/null
> +++ b/arch/arm/mach-mvebu/mvebu-soc-id.c
> @@ -0,0 +1,111 @@
> +/*
> + * Variant and revision information for mvebu SoCs
> + *
> + * Copyright (C) 2014 Marvell
> + *
> + * Gregory CLEMENT <gregory.clement@free-electrons.com>
> + *
> + * 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;
> +
> +	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

> +
> +		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);
> +
> diff --git a/include/linux/mvebu-soc-id.h b/include/linux/mvebu-soc-id.h
> new file mode 100644
> index 000000000000..602ce1c50d1d
> --- /dev/null
> +++ b/include/linux/mvebu-soc-id.h
> @@ -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 */
> -- 
> 1.8.1.2
> 

Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC

[Gregory CLEMENT]

On 03/01/2014 15:47, Andrew Lunn wrote:
> On Fri, Jan 03, 2014 at 10:59:44AM +0100, Gregory CLEMENT wrote:
>> 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@vger.kernel.org
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> ---
>>  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
>>
>> diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
>> index 2d04f0e21870..878aebe98dcc 100644
>> --- a/arch/arm/mach-mvebu/Makefile
>> +++ b/arch/arm/mach-mvebu/Makefile
>> @@ -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
>> diff --git a/arch/arm/mach-mvebu/mvebu-soc-id.c b/arch/arm/mach-mvebu/mvebu-soc-id.c
>> new file mode 100644
>> index 000000000000..86c901be284c
>> --- /dev/null
>> +++ b/arch/arm/mach-mvebu/mvebu-soc-id.c
>> @@ -0,0 +1,111 @@
>> +/*
>> + * Variant and revision information for mvebu SoCs
>> + *
>> + * Copyright (C) 2014 Marvell
>> + *
>> + * Gregory CLEMENT <gregory.clement@free-electrons.com>
>> + *
>> + * 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;
>> +
>> +	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?

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.

> 
> Thanks
> 	Andrew
> 
>> +
>> +		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);
>> +
>> diff --git a/include/linux/mvebu-soc-id.h b/include/linux/mvebu-soc-id.h
>> new file mode 100644
>> index 000000000000..602ce1c50d1d
>> --- /dev/null
>> +++ b/include/linux/mvebu-soc-id.h
>> @@ -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 */
>> -- 
>> 1.8.1.2
>>


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC

[Gregory CLEMENT]

Hi Andrew,

On 03/01/2014 15:51, Gregory CLEMENT wrote:
> On 03/01/2014 15:47, Andrew Lunn wrote:
>> On Fri, Jan 03, 2014 at 10:59:44AM +0100, Gregory CLEMENT wrote:
>>> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>> Signed-off-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>>> ---
>>>  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
>>>
>>> diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
>>> index 2d04f0e21870..878aebe98dcc 100644
>>> --- a/arch/arm/mach-mvebu/Makefile
>>> +++ b/arch/arm/mach-mvebu/Makefile
>>> @@ -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
>>> diff --git a/arch/arm/mach-mvebu/mvebu-soc-id.c b/arch/arm/mach-mvebu/mvebu-soc-id.c
>>> new file mode 100644
>>> index 000000000000..86c901be284c
>>> --- /dev/null
>>> +++ b/arch/arm/mach-mvebu/mvebu-soc-id.c
>>> @@ -0,0 +1,111 @@
>>> +/*
>>> + * Variant and revision information for mvebu SoCs
>>> + *
>>> + * Copyright (C) 2014 Marvell
>>> + *
>>> + * Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>>> + *
>>> + * 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;
>>> +
>>> +	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?
> 
> 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.

Gregory

>>
>> Thanks
>> 	Andrew
>>
>>> +
>>> +		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);
>>> +
>>> diff --git a/include/linux/mvebu-soc-id.h b/include/linux/mvebu-soc-id.h
>>> new file mode 100644
>>> index 000000000000..602ce1c50d1d
>>> --- /dev/null
>>> +++ b/include/linux/mvebu-soc-id.h
>>> @@ -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 */
>>> -- 
>>> 1.8.1.2
>>>
> 
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC

[Andrew Lunn]

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

Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC

[Gregory CLEMENT]

On 03/01/2014 17:41, Andrew Lunn wrote:
>>>> 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?
>>>
>>> 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.
>  
>> 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.

With this number we are sure to have always an information even if the
SoC ID or version was unknown by the kernel.

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

Yes it is doable, but then we also need to update the mvebu_pcie_of_match_table.
However I would prefer doing this as a separate patch because this one is
for fixing a bug. Later we can improve the kernel.

> 
>     Andrew
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Fri,  3 Jan 2014 10:59:44 +0100, Gregory CLEMENT wrote:
> +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.

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

> +
> +		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__.

> +		/* 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.

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

> +#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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC

[Gregory CLEMENT]

On 03/01/2014 19:48, Thomas Petazzoni wrote:
> Dear Gregory CLEMENT,
> 
> On Fri,  3 Jan 2014 10:59:44 +0100, Gregory CLEMENT wrote:
>> +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.

ok

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

yes I was a little lazy with it: if child is NULL then of_clk_get_by_name
will return an error but then the error message will be a misleading.

> 
>> +
>> +		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__.

ok thanks for the tip
> 
>> +		/* 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.

ok

> 
>> +	return ret;
>> +}
>> +arch_initcall(mvebu_soc_id_init);
> 
>> +#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.
> 

ok
> Best regards,
> 
> Thomas
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC

[Jason Gunthorpe]

On Fri, Jan 03, 2014 at 10:59:44AM +0100, Gregory CLEMENT wrote:

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

Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC

[Gregory CLEMENT]

On 03/01/2014 19:59, Jason Gunthorpe wrote:
> On Fri, Jan 03, 2014 at 10:59:44AM +0100, Gregory CLEMENT wrote:
> 
>> +		/* 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..

According to the datasheet of the Armada XP and the Armada 370 these
register are read-only. Moreover they are already use as is for the
kirkwood, orion5x, dove and mv78x00 in the current kernel and for all
the mvebu Socs in the internal version of Marvell, so even if it was
possible to change these values I believe that it never happened.

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


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC

[Arnd Bergmann]

On Friday 03 January 2014, Gregory CLEMENT wrote:
> 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.

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

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

Maybe EXPORT_SYMBOL_GPL, out of principle?

> +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;
> +		/*
> +		 * 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.

Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC

[Andrew Lunn]

> > +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;
> > +		/*
> > +		 * 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.

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?

    Thanks
	Andrew

Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC

[Jason Gunthorpe]

On Sun, Jan 05, 2014 at 04:40:23PM +0100, Andrew Lunn wrote:
> > > +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;
> > > +		/*
> > > +		 * 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.

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

Jason

Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC

[Sebastian Hesselbarth]

On 01/05/2014 06:27 PM, Jason Gunthorpe wrote:
> On Sun, Jan 05, 2014 at 04:40:23PM +0100, Andrew Lunn wrote:
>>>> +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;
>>>> +		/*
>>>> +		 * 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.
>>
>> 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?

Sebastian

Sebastian

Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC

[Jason Gunthorpe]

On Sun, Jan 05, 2014 at 06:37:21PM +0100, Sebastian Hesselbarth wrote:

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

Jason

Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC

[Sebastian Hesselbarth]

On 01/06/2014 12:07 AM, Jason Gunthorpe wrote:
> On Sun, Jan 05, 2014 at 06:37:21PM +0100, Sebastian Hesselbarth wrote:
>
>> 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

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

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

Sebastian

Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC

[Jason Gunthorpe]

On Mon, Jan 06, 2014 at 12:12:16AM +0100, Sebastian Hesselbarth wrote:
> On 01/06/2014 12:07 AM, Jason Gunthorpe wrote:
> >On Sun, Jan 05, 2014 at 06:37:21PM +0100, Sebastian Hesselbarth wrote:
> >
> >>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
> 
> 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.

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

Jason

Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC

[Sebastian Hesselbarth]

On 01/06/2014 12:40 AM, Jason Gunthorpe wrote:
> On Mon, Jan 06, 2014 at 12:12:16AM +0100, Sebastian Hesselbarth wrote:
>> On 01/06/2014 12:07 AM, Jason Gunthorpe wrote:
>>> On Sun, Jan 05, 2014 at 06:37:21PM +0100, Sebastian Hesselbarth wrote:
>>>
>>>> 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
>>
>> 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.

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

True. But I still wonder how many users will find the correct dtb
without knowing the SoC revision. Having it probed would be best for
users, but I see the difficulties.

We should really work harder on proper u-boot/barebox for all those
Orion SoCs to have at least the option to update it with DT support.

Sebastian

Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC

[Andrew Lunn]

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

Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC

[Gregory CLEMENT]

Hi Andrew,

On 06/01/2014 01:17, Andrew Lunn wrote:
>>>> 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.

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



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


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC

[Gregory CLEMENT]

On 06/01/2014 10:55, Gregory CLEMENT wrote:
> Hi Andrew,
> 
> On 06/01/2014 01:17, Andrew Lunn wrote:
>>>>> 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.
> 
> 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.

So I have just put all the  Memory Power Down for all the PCIe
slot and I still managed to read the ID so it won't be an issue
(at least on Armada XP)

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


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC

[Arnd Bergmann]

On Sunday 05 January 2014, Andrew Lunn wrote:
> 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?

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.

	Arn

Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC

[Andrew Lunn]

On Sun, Jan 05, 2014 at 08:17:10PM +0100, Arnd Bergmann wrote:
> On Sunday 05 January 2014, Andrew Lunn wrote:
> > 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?
> 
> 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.

Hum, yes, interesting.

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?

Andrew

Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC

[Arnd Bergmann]

On Monday 06 January 2014, Andrew Lunn wrote:
> This raises the question, should mainline try to support any random
> dtb blob, or only those that have ever shipped with mainline?

It should support any dtb that conforms to the binding.

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

That seems reasonable, yes.

> 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, which means we would accept both compatible
strings, or that we make an incompatible change to the binding for at
least one of them before we call the binding stable and move the
repository to devicetree.org (or whereever it goes after moving out
of Linux).

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.

	Arnd

Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC

[Andrew Lunn]

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

Yes. I was not even aware FreeBSD was using DT until somebody
mentioned it in Edinburgh. 

As an example:

http://fxr.watson.org/fxr/source/boot/fdt/dts/sheevaplug.dts

compared with

http://lxr.linux.no/linux+v3.12.6/arch/arm/boot/dts/kirkwood-sheevaplug.dts
http://lxr.linux.no/linux+v3.12.6/arch/arm/boot/dts/kirkwood-sheevaplug-common.dtsi
http://lxr.linux.no/linux+v3.12.6/arch/arm/boot/dts/kirkwood-6281.dtsi
http://lxr.linux.no/linux+v3.12.6/arch/arm/boot/dts/kirkwood.dtsi

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

	Andrew

Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC

[Arnd Bergmann]

On Monday 06 January 2014, Andrew Lunn wrote:

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

Right. They seem to have quite a selection of dts files by now, which
are roughly comparable to the ones we have, but slightly different
in some aspects.

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

Right. We used to have "clock-frequency" properties defined in a number
of bindings that predate the generic clock binding, but I guess we wouldn't
do that anymore.

	Arnd

Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC

[Gregory CLEMENT]

On 05/01/2014 15:25, Arnd Bergmann wrote:
> On Friday 03 January 2014, Gregory CLEMENT wrote:
>> 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.

Unfortunately there is no other way to get this ID. It is not a
"traditional" way of doing it, it just the only way to get this
information given the design of the hardware

> 
>> +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?
> 
>> +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);
> 
> Maybe EXPORT_SYMBOL_GPL, out of principle?
> 
>> +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;
>> +		/*
>> +		 * 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.

yes it fails but then this function exits with an error (a more accurate
error in the next version). It will be the responsibility of the code which
need this information to check that this function won't fail. For the i2c
driver, for instance, we will switch on the safe mode and it won(y use
the offload mechanism.

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


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH v2 2/2] i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs

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

Cc: stable@vger.kernel.org
Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
Acked-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 8be7e42aa4de..634b04d241fb 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -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:
-- 
1.8.1.2

Re: [PATCH v2 2/2] i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs

[Gregory CLEMENT]

Hi Jason,

On 03/01/2014 10:59, Gregory CLEMENT wrote:
> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Signed-off-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> Acked-by: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>

I have just realized that it should be fair to add a

Reported-by: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>

Thanks,

Gregory


> ---
>  drivers/i2c/busses/i2c-mv64xxx.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> index 8be7e42aa4de..634b04d241fb 100644
> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> @@ -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:
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v2 2/2] i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Fri,  3 Jan 2014 10:59:45 +0100, Gregory CLEMENT wrote:

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

revison -> revisions

offload mechanism -> the offload mechanism

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

weplay -> we play

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

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH v2 2/2] i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs

[Gregory CLEMENT]

On 03/01/2014 19:49, Thomas Petazzoni wrote:
> Dear Gregory CLEMENT,
> 
> On Fri,  3 Jan 2014 10:59:45 +0100, Gregory CLEMENT wrote:
> 
>> +		/*
>> +		 * Only revison more recent than A0 support offload
> 
> revison -> revisions
> 
> offload mechanism -> the offload mechanism
> 
>> +		 * mechanism. In case we can't get the SoC revision
>> +		 * weplay safe and we don't enable it
> 
> weplay -> we play
> 
>> +		 */
>> +		if (mvebu_get_soc_id(&rev, &dev) == 0 && dev > MV78XX0_A0_REV)
>> +			drv_data->offload_enabled = true;
>>  	}
>>  
>>  out:
> 

These typos will be fixed in the next version.

> Thanks!
> 
> Thomas
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v2 2/2] i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs

[Arnd Bergmann]

On Friday 03 January 2014, Gregory CLEMENT wrote:
> 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@vger.kernel.org
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> Acked-by: Wolfram Sang <wsa@the-dreams.de>

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

Re: [PATCH v2 2/2] i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs

[Gregory CLEMENT]

Hi Arnd,

On 05/01/2014 15:33, Arnd Bergmann wrote:
> On Friday 03 January 2014, Gregory CLEMENT wrote:
>> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Signed-off-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>> Acked-by: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
> 
> 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.

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

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


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v2 2/2] i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs

[Arnd Bergmann]

On Monday 06 January 2014, Gregory CLEMENT wrote:
> > 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.

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

Re: [PATCH v2 2/2] i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs

[Gregory CLEMENT]

On 07/01/2014 10:03, Arnd Bergmann wrote:
> On Monday 06 January 2014, Gregory CLEMENT wrote:
>>> 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.

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.

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

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

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


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v2 2/2] i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs

[Arnd Bergmann]

On Tuesday 07 January 2014, Gregory CLEMENT wrote:
> On 07/01/2014 10:03, Arnd Bergmann wrote:
> > 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.

Right. This is not exactly what I'd call "discoverable" though:
ideally we would have a way to ask the hardware for the availability
of specific features, but that clearly doesn't work here, which
leaves the default way to handle it by using DT properties to describe
it in software. In case of the AX3-4 board, that would unfortunately
imply that we would either need two separate dtb files or fall back
to the workaround for all of them. Neither approach seems particularly
user-friendly, so some form of autodetection seems appropriate.

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

Ok, thanks!

	Arnd