Fix i2c bus hang on A0 version of the Armada XP SoCs
Subsystems (5)
v2 cover Fix i2c bus hang on A0 version of the Armada XP SoCs
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
Signature · 1 line
1.8.1.2
PATCH v2 1/2 ARM: mvebu: Add support to get the ID and the revision of a SoC
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/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
@@ -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 +111 -0
111 lines not on this page · raw message
include/linux/mvebu-soc-id.h new file +32 -0
@@ -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
Andrew Lunn
maintainerPATCH v2 1/2 ARM: mvebu: Add support to get the ID and the revision of a SoC
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.orgSigned-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.harch/arm/mach-mvebu/Makefile +1
-1@@ -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.oarch/arm/mach-mvebu/mvebu-soc-id.c new file +57
-0@@ -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@@ -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
PATCH v2 1/2 ARM: mvebu: Add support to get the ID and the revision of a SoC
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.orgSigned-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.harch/arm/mach-mvebu/Makefile +1
-1@@ -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.oarch/arm/mach-mvebu/mvebu-soc-id.c new file +57
-0@@ -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
AndrewQuoted 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@@ -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
Signature · 4 lines
Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com
PATCH v2 1/2 ARM: mvebu: Add support to get the ID and the revision of a SoC
Hi Andrew,
Quoted 117 lines · Gregory CLEMENT
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.orgSigned-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.harch/arm/mach-mvebu/Makefile +1
-1@@ -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.oarch/arm/mach-mvebu/mvebu-soc-id.c new file +57
-0@@ -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?
Quoted 4 lines
There is a function named
of_get_next_available_child, so I assumed thatof_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
Quoted 91 lines
Quoted 89 lines
Thanks
AndrewQuoted 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@@ -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
Signature · 4 lines
Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com
Andrew Lunn
maintainerPATCH v2 1/2 ARM: mvebu: Add support to get the ID and the revision of a SoC
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 thatof_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.dtsfile (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 3 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
PATCH v2 1/2 ARM: mvebu: Add support to get the ID and the revision of a SoC
Quoted 19 lines · Andrew Lunn
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 thatof_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.dtsfile (PCie is disable by default in the dtsi file) and it worled as expected! :)
Quoted 8 lines
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.
With this number we are sure to have always an information even if the SoC ID or version was unknown by the kernel.
Quoted 7 lines
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.osort of thing in theMakefile. 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.
Quoted 3 lines
Andrew
Signature · 4 lines
Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com
PATCH v2 1/2 ARM: mvebu: Add support to get the ID and the revision of a SoC
In reply to Gregory CLEMENTDear 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 7 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 4 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 6 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.
include/linux/mvebu-soc-id.h line 26
Quoted 8 lines
include/linux/mvebu-soc-id.h new file +7
-0+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
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
PATCH v2 1/2 ARM: mvebu: Add support to get the ID and the revision of a SoC
Quoted 13 lines · Thomas Petazzoni
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:
Quoted 5 lines
if (!np) return 0;so that you avoid indenting the entire function code inside the if { ... } block.
ok
Quoted 10 lines
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?
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.
Quoted 8 lines
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:
Quoted 4 lines
#define
pr_fmt(fmt)"mvebu-soc-id: " fmtat the beginning of your C file and get rid of the "%s" for the
__func__.
ok thanks for the tip
Quoted 10 lines
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.
ok
Quoted 5 lines
Quoted 4 lines
arch/arm/mach-mvebu/mvebu-soc-id.c new file +3
-0+return ret; +} +arch_initcall (mvebu_soc_id_init);
Quoted 8 lines
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.
ok
Quoted 4 lines
Best regards,
Thomas
Signature · 4 lines
Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com
PATCH v2 1/2 ARM: mvebu: Add support to get the ID and the revision of a SoC
In reply to Gregory CLEMENTarch/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
PATCH v2 1/2 ARM: mvebu: Add support to get the ID and the revision of a SoC
Quoted 14 lines · Jason Gunthorpe
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..
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.
Quoted 9 lines
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
Signature · 4 lines
Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com
PATCH v2 1/2 ARM: mvebu: Add support to get the ID and the revision of a SoC
In reply to Gregory CLEMENTQuoted 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 3 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 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.
Andrew Lunn
maintainerPATCH v2 1/2 ARM: mvebu: Add support to get the ID and the revision of a SoC
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.
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
PATCH v2 1/2 ARM: mvebu: Add support to get the ID and the revision of a SoC
Quoted 18 lines · Andrew Lunn
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.
Quoted 5 lines
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
Sebastian Hesselbarth
maintainerPATCH v2 1/2 ARM: mvebu: Add support to get the ID and the revision of a SoC
Quoted 19 lines · Jason Gunthorpe
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 6 lines
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?
Sebastian
Sebastian
PATCH v2 1/2 ARM: mvebu: Add support to get the ID and the revision of a SoC
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..
Jason
Sebastian Hesselbarth
maintainerPATCH v2 1/2 ARM: mvebu: Add support to get the ID and the revision of a SoC
Quoted 9 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?
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..
Sebastian
PATCH v2 1/2 ARM: mvebu: Add support to get the ID and the revision of a SoC
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..
Jason
Sebastian Hesselbarth
maintainerPATCH v2 1/2 ARM: mvebu: Add support to get the ID and the revision of a SoC
Quoted 9 lines · Jason Gunthorpe
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 8 lines
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.
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 7 lines
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..
Quoted 5 lines
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
Andrew Lunn
maintainerPATCH v2 1/2 ARM: mvebu: Add support to get the ID and the revision of a SoC
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
PATCH v2 1/2 ARM: mvebu: Add support to get the ID and the revision of a SoC
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
Signature · 4 lines
Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com
PATCH v2 1/2 ARM: mvebu: Add support to get the ID and the revision of a SoC
Quoted 20 lines · Gregory CLEMENT
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.
Quoted 6 lines
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)
Quoted 17 lines
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
Signature · 4 lines
Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com
PATCH v2 1/2 ARM: mvebu: Add support to get the ID and the revision of a SoC
In reply to Andrew LunnQuoted 8 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.
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
Andrew Lunn
maintainerPATCH v2 1/2 ARM: mvebu: Add support to get the ID and the revision of a SoC
Quoted 5 lines · Arnd Bergmann
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 8 lines
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.
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
PATCH v2 1/2 ARM: mvebu: Add support to get the ID and the revision of a SoC
Quoted 2 lines · Andrew Lunn
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.
Quoted 3 lines
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.
Quoted 6 lines
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
Andrew Lunn
maintainerPATCH v2 1/2 ARM: mvebu: Add support to get the ID and the revision of a SoC
Quoted 2 lines
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
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.
Andrew
PATCH v2 1/2 ARM: mvebu: Add support to get the ID and the revision of a SoC
Quoted 9 lines · Andrew Lunn
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.
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.
Quoted 7 lines
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.
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
PATCH v2 1/2 ARM: mvebu: Add support to get the ID and the revision of a SoC
In reply to Arnd BergmannQuoted 9 lines · Arnd Bergmann
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.
Quoted 4 lines
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
Quoted 37 lines
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;
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.
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.
Quoted 6 lines
--
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
Signature · 4 lines
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
In reply to Gregory CLEMENTThe 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.
--- drivers/i2c/busses/i2c-mv64xxx.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
drivers/i2c/busses/i2c-mv64xxx.c +10 -1
@@ -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:
Signature · 1 line
1.8.1.2
PATCH v2 2/2 i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs
Hi Jason,
Quoted 11 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.
Cc stable at vger.kernel.orgSigned-off-by Gregory CLEMENT <[email protected]>Acked-by Wolfram Sang <[email protected]>
I have just realized that it should be fair to add a
Thanks,
Gregory
Quoted 35 lines
--- drivers/i2c/busses/i2c-mv64xxx.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)drivers/i2c/busses/i2c-mv64xxx.c +10
-1@@ -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:
Signature · 4 lines
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
In reply to Gregory CLEMENTDear 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
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
PATCH v2 2/2 i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs
Quoted 16 lines · Thomas Petazzoni
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
Quoted 7 lines
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:
These typos will be fixed in the next version.
Quoted 4 lines
Thanks!
Thomas
Signature · 4 lines
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
In reply to Gregory CLEMENTQuoted 11 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.
Cc stable at vger.kernel.orgSigned-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
PATCH v2 2/2 i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs
Hi Arnd,
Quoted 9 lines · Arnd Bergmann
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 7 lines
Quoted 3 lines
Cc stable at vger.kernel.orgSigned-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
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 12 lines
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
Signature · 4 lines
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
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 6 lines
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.
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
PATCH v2 2/2 i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs
Quoted 16 lines · Arnd Bergmann
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.
Quoted 9 lines
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 12 lines
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.
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.
Quoted 7 lines
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
Signature · 4 lines
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
Quoted 12 lines · Gregory CLEMENT
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.
Quoted 8 lines
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.
Quoted 8 lines
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.
Ok, thanks!
Arnd