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

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

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

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

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

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

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

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

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

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
>

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
>

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
>

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.

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

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

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

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

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

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

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

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

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

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

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

> >>Does that power down really disable reading from PCIe controller
> >>registers or is it just PHY power down?
> >
> >I haven't experimented with it, but every block that has a clock gate
> >has a power down, so I doubt it is just a phy power down.
> 
> Ok, I see. But it isn't documented in the public FS, is it? If there is
> an extra powerdown register for each ip block, I guess it will also
> break reading from its registers.

Hi Sebastian

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

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

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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

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
>

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

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

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