[PATCHv2 0/4] ARM: implement workaround for Cortex-A9/PL310/PCIe deadlock

[PATCHv2 0/4] ARM: implement workaround for Cortex-A9/PL310/PCIe deadlock

[Thomas Petazzoni]

Russell, Will, Catalin,

This patch series adresses a problem that affects the newer Marvell
Armada 375 and 38x SOCs, based on Cortex-A9+PL310, combined with the
Marvell PCIe hardware unit. When the hardware I/O coherency is
enabled, the combination of Cortex-A9/PL310/Marvell PCIe hardware unit
will quickly cause a deadlock when the PCIe bus is stressed.

The workaround for this problem has been suggested by ARM, and
consists in two things:

 (1) Map the PCIe regions as strongly-ordered

 (2) Disable the outer cache sync of the PL310 when hardware I/O
     coherency is used, since it is unneeded and causes the deadlock.

The following four patches address the problem in the following way:

 * PATCH 1/4 fixes a minor issue in drivers/of, which prevents from
   using of_update_property() early enough in the boot.

 * PATCH 2/4 adds the necessary infrastructure to allow PCIe I/O
   regions to be mapped as strongly-ordered.

 * PATCH 3/4 extends the l2x0 cache driver with a new compatible
   string "arm,pl310-coherent-cache" which is associated to a
   different set of L2 cache maintenance operations having ->sync set
   to NULL.

 * PATCH 4/4 uses both of the added infrastructures, as well as the
   existing infrastructure to customize the behavior of ioremap() on a
   per-platform basis, to implement the workaround for the Armada 375
   and 38x SOCs.

Changes since v1:

 - Instead of introducing separate l2x0 initialization functions, rely
   on a separate compatible string to identify whether we're coherent
   or not. The compatible string *has* to be modified at runtime,
   because Armada 375 and 38x are only I/O coherent when in SMP
   mode. In non-SMP mode, they are not I/O coherent, so we cannot
   change the DT to 'arm,pl310-coherent-cache'.

 - Addition of the drivers/of fix to be able to use
   of_update_property() early and fix up the PL310 compatible string,
   as explained in the previous item.

Thanks!

Thomas

Thomas Petazzoni (4):
  of: make of_update_property() usable earlier in the boot process
  ARM: mm: allow sub-architectures to override PCI I/O memory type
  ARM: mm: add support for HW coherent systems in PL310
  ARM: mvebu: implement L2/PCIe deadlock workaround

 Documentation/devicetree/bindings/arm/l2cc.txt |  2 +
 arch/arm/include/asm/io.h                      |  6 +++
 arch/arm/mach-mvebu/board-v7.c                 | 55 ++++++++++++++++++++++++++
 arch/arm/mm/cache-l2x0.c                       | 21 ++++++++++
 arch/arm/mm/ioremap.c                          |  9 ++++-
 drivers/of/base.c                              |  5 +++
 6 files changed, 97 insertions(+), 1 deletion(-)

-- 
1.9.2

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

[PATCHv2 1/4] of: make of_update_property() usable earlier in the boot process

[Thomas Petazzoni]

Commit 75b57ecf9d1d1e17d099ab13b8f48e6e038676be ('of: Make device
nodes kobjects so they show up in sysfs') has turned Device Tree nodes
in kobjects and added a sysfs based representation for Device Tree
nodes. Since the sysfs logic is only available after the execution of
a core_initcall(), the patch took precautions in of_add_property() and
of_remove_property() to not do any sysfs related manipulation early in
the boot process.

However, it forgot to do the same for of_update_property(), which if
used early in the boot process (before core_initcalls have been
called), tries to call sysfs_remove_bin_file(), and crashes:

------------[ cut here ]------------
WARNING: CPU: 0 PID: 0 at /home/thomas/projets/linux-2.6/fs/kernfs/dir.c:1216 kernfs_remove_by_name_ns+0x80/0x88()
kernfs: can not remove '(null)', no directory
Modules linked in:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.15.0-rc1-00127-g1d7e7b2-dirty #423
[<c0014910>] (unwind_backtrace) from [<c00110ec>] (show_stack+0x10/0x14)
[<c00110ec>] (show_stack) from [<c04c84b8>] (dump_stack+0x84/0x94)
[<c04c84b8>] (dump_stack) from [<c001d8c0>] (warn_slowpath_common+0x6c/0x88)
[<c001d8c0>] (warn_slowpath_common) from [<c001d90c>] (warn_slowpath_fmt+0x30/0x40)
[<c001d90c>] (warn_slowpath_fmt) from [<c0104468>] (kernfs_remove_by_name_ns+0x80/0x88)
[<c0104468>] (kernfs_remove_by_name_ns) from [<c0394d98>] (of_update_property+0xc0/0xf0)
[<c0394d98>] (of_update_property) from [<c0647248>] (mvebu_timer_and_clk_init+0xfc/0x194)
[<c0647248>] (mvebu_timer_and_clk_init) from [<c0640934>] (start_kernel+0x218/0x350)
[<c0640934>] (start_kernel) from [<00008070>] (0x8070)

To fix this problem, we simply skip the sysfs related calls in
of_update_property(), and rely on of_init() to fix up things when it
will be called, exactly as is done in of_add_property() and
of_remove_property().

Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Fixes: 75b57ecf9d1d1e17d099ab13b8f48e6e038676be ('of: Make device nodes kobjects so they show up in sysfs')
---
 drivers/of/base.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index f72d19b..e981f09 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1831,11 +1831,16 @@ int of_update_property(struct device_node *np, struct property *newprop)
 	if (rc)
 		return rc;
 
+	/* At early boot, bail out and defer setup to of_init() */
+	if (!of_kset)
+		goto skip_sysfs;
+
 	/* Update the sysfs attribute */
 	if (oldprop)
 		sysfs_remove_bin_file(&np->kobj, &oldprop->attr);
 	__of_add_property_sysfs(np, newprop);
 
+ skip_sysfs:
 	if (!found)
 		return -ENODEV;
 
-- 
1.9.2

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

[PATCHv2 2/4] ARM: mm: allow sub-architectures to override PCI I/O memory type

[Thomas Petazzoni]

Due to a design incompatibility between the PCIe Marvell controller
and the Cortex-A9, stressing PCIe devices with a lot of traffic
quickly causes a deadlock.

One part of the workaround for this is to have all PCIe regions mapped
as MT_MEMORY_SO instead of MT_DEVICE. While the arch_ioremap_caller()
mechanism allows sub-architecture code to override ioremap(), used to
map PCIe memory regions, there isn't such a mechanism to override the
behavior of pci_ioremap_io().

This commit adds the arch_pci_ioremap_mem_type variable, initialized
to MT_DEVICE by default, and that sub-architecture code can
override. We have chosen to expose a single variable rather than
offering the possibility of overriding the entire pci_ioremap_io(),
because implementing pci_ioremap_io() requires calling functions
(get_mem_type()) that are private to the arch/arm/mm/ code.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/include/asm/io.h | 6 ++++++
 arch/arm/mm/ioremap.c     | 9 ++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
index 8aa4cca..3d23418 100644
--- a/arch/arm/include/asm/io.h
+++ b/arch/arm/include/asm/io.h
@@ -179,6 +179,12 @@ static inline void __iomem *__typesafe_io(unsigned long addr)
 /* PCI fixed i/o mapping */
 #define PCI_IO_VIRT_BASE	0xfee00000
 
+#if defined(CONFIG_PCI)
+void pci_ioremap_set_mem_type(int mem_type);
+#else
+static inline void pci_ioremap_set_mem_type(int mem_type) {}
+#endif
+
 extern int pci_ioremap_io(unsigned int offset, phys_addr_t phys_addr);
 
 /*
diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
index f9c32ba..d1e5ad7 100644
--- a/arch/arm/mm/ioremap.c
+++ b/arch/arm/mm/ioremap.c
@@ -438,6 +438,13 @@ void __arm_iounmap(volatile void __iomem *io_addr)
 EXPORT_SYMBOL(__arm_iounmap);
 
 #ifdef CONFIG_PCI
+static int pci_ioremap_mem_type = MT_DEVICE;
+
+void pci_ioremap_set_mem_type(int mem_type)
+{
+	pci_ioremap_mem_type = mem_type;
+}
+
 int pci_ioremap_io(unsigned int offset, phys_addr_t phys_addr)
 {
 	BUG_ON(offset + SZ_64K > IO_SPACE_LIMIT);
@@ -445,7 +452,7 @@ int pci_ioremap_io(unsigned int offset, phys_addr_t phys_addr)
 	return ioremap_page_range(PCI_IO_VIRT_BASE + offset,
 				  PCI_IO_VIRT_BASE + offset + SZ_64K,
 				  phys_addr,
-				  __pgprot(get_mem_type(MT_DEVICE)->prot_pte));
+				  __pgprot(get_mem_type(pci_ioremap_mem_type)->prot_pte));
 }
 EXPORT_SYMBOL_GPL(pci_ioremap_io);
 #endif
-- 
1.9.2

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

[PATCHv2 3/4] ARM: mm: add support for HW coherent systems in PL310

[Thomas Petazzoni]

When a PL310 cache is used on a system that provides hardware
coherency, the outer cache sync operation is useless, and can be
skipped. Moreover, on some systems, it is harmful as it causes
deadlocks between the Marvell coherency mechanism, the Marvell PCIe
controller and the Cortex-A9.

To avoid this, this commit introduces a separate
'arm,pl310-coherent-cache' compatible string, which identifies a PL310
cache in an I/O coherent configuration. This compatible string is
associated with a different set of l2x0_of_data, in which the ->sync
operation is NULL.

Note that technically speaking, a fully coherent system wouldn't
require any of the other .outer_cache operations. However, in
practice, when booting secondary CPUs, these are not yet coherent, and
therefore a set of cache maintenance operations are necessary at this
point. This explains why we keep the other .outer_cache operations and
only ->sync is disabled.

While in theory any write to a PL310 register could cause the
deadlock, in practice, disabling ->sync is sufficient to workaround
the deadlock, since the other cache maintenance operations are only
used in very specific situations.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 Documentation/devicetree/bindings/arm/l2cc.txt |  2 ++
 arch/arm/mm/cache-l2x0.c                       | 21 +++++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt
index b513cb8..41953b3 100644
--- a/Documentation/devicetree/bindings/arm/l2cc.txt
+++ b/Documentation/devicetree/bindings/arm/l2cc.txt
@@ -8,6 +8,8 @@ Required properties:
 
 - compatible : should be one of:
   "arm,pl310-cache"
+  "arm,pl310-coherent-cache", used for I/O coherent platforms using
+     the PL310 cache
   "arm,l220-cache"
   "arm,l210-cache"
   "bcm,bcm11351-a2-pl310-cache": DEPRECATED by "brcm,bcm11351-a2-pl310-cache"
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 7abde2ce..ae19540 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -889,6 +889,26 @@ static const struct l2x0_of_data pl310_data = {
 	},
 };
 
+/*
+ * PL310 operations used on I/O coherent systems. Theoretically, no
+ * outer cache operations would be needed, except that for secondary
+ * processors bring up, a few cache maintenance operations are needed
+ * because secondary processors are not directly coherent with the L2
+ * cache when they start up.
+ */
+static const struct l2x0_of_data pl310_coherent_data = {
+	.setup = pl310_of_setup,
+	.save  = pl310_save,
+	.outer_cache = {
+		.resume      = pl310_resume,
+		.inv_range   = l2x0_inv_range,
+		.clean_range = l2x0_clean_range,
+		.flush_range = l2x0_flush_range,
+		.flush_all   = l2x0_flush_all,
+		.inv_all     = l2x0_inv_all,
+	},
+};
+
 static const struct l2x0_of_data l2x0_data = {
 	.setup = l2x0_of_setup,
 	.save  = NULL,
@@ -955,6 +975,7 @@ static const struct of_device_id l2x0_ids[] __initconst = {
 	{ .compatible = "arm,l210-cache", .data = (void *)&l2x0_data },
 	{ .compatible = "arm,l220-cache", .data = (void *)&l2x0_data },
 	{ .compatible = "arm,pl310-cache", .data = (void *)&pl310_data },
+	{ .compatible = "arm,pl310-coherent-cache", .data = (void *)&pl310_coherent_data },
 	{ .compatible = "bcm,bcm11351-a2-pl310-cache", /* deprecated name */
 	  .data = (void *)&bcm_l2x0_data},
 	{ .compatible = "brcm,bcm11351-a2-pl310-cache",
-- 
1.9.2

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

[PATCHv2 4/4] ARM: mvebu: implement L2/PCIe deadlock workaround

[Thomas Petazzoni]

The Marvell Armada 375 and Armada 38x SOCs, which use the Cortex-A9
CPU core, the PL310 cache and the Marvell PCIe hardware block are
affected a L2/PCIe deadlock caused by a system erratum when hardware
I/O coherency is used.

This deadlock can be avoided by mapping the PCIe memory areas as
strongly-ordered, and by removing the outer cache sync done in
software. This is done in this patch, thanks to the new bits of
infrastructure added in 'ARM: mm: allow sub-architectures to override
PCI I/O memory type' and 'ARM: mm: add support for HW coherent systems
in PL310' respectively.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/mach-mvebu/board-v7.c | 55 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/arch/arm/mach-mvebu/board-v7.c b/arch/arm/mach-mvebu/board-v7.c
index 01cfce6..e7e09ce 100644
--- a/arch/arm/mach-mvebu/board-v7.c
+++ b/arch/arm/mach-mvebu/board-v7.c
@@ -71,6 +71,53 @@ static int armada_375_external_abort_wa(unsigned long addr, unsigned int fsr,
 	return 1;
 }
 
+/*
+ * This ioremap hook is used on Armada 375/38x to ensure that PCIe
+ * memory areas are mapped as MT_MEMORY_RW_SO instead of
+ * MT_DEVICE. This is needed as a workaround for a deadlock issue
+ * between the PCIe interface and the cache controller.
+ */
+static void __iomem *
+armada_pcie_wa_ioremap_caller(phys_addr_t phys_addr, size_t size,
+			      unsigned int mtype, void *caller)
+{
+	struct resource pcie_mem;
+
+	mvebu_mbus_get_pcie_mem_aperture(&pcie_mem);
+
+	if (pcie_mem.start <= phys_addr && (phys_addr + size) <= pcie_mem.end)
+		mtype = MT_MEMORY_RW_SO;
+
+	return __arm_ioremap_caller(phys_addr, size, mtype, caller);
+}
+
+/*
+ * When we are I/O coherent and we use the PL310 cache controller, we
+ * switch the PL310 compatible string to
+ * "arm,pl310-coherent-cache". This makes sure the outer sync
+ * operation is not used, which allows to workaround the system
+ * erratum that causes deadlocks when doing PCIe in a SMP situation on
+ * Armada 375 and Armada 38x.
+ */
+static void __init mvebu_l2x0_pl310_coherent(void)
+{
+	struct device_node *np;
+
+	if (!coherency_available())
+		return;
+
+	for_each_compatible_node(np, NULL, "arm,pl310-cache") {
+		struct property *new_compat;
+
+		new_compat = kzalloc(sizeof(*new_compat), GFP_KERNEL);
+		new_compat->name = kstrdup("compatible", GFP_KERNEL);
+		new_compat->value = kstrdup("arm,pl310-coherent-cache",
+					    GFP_KERNEL);
+		new_compat->length = strlen(new_compat->value) + 1;
+		of_update_property(np, new_compat);
+	}
+}
+
 static void __init mvebu_timer_and_clk_init(void)
 {
 	of_clk_init(NULL);
@@ -78,6 +125,14 @@ static void __init mvebu_timer_and_clk_init(void)
 	mvebu_scu_enable();
 	coherency_init();
 	BUG_ON(mvebu_mbus_dt_init(coherency_available()));
+
+	if (of_machine_is_compatible("marvell,armada375") ||
+	    of_machine_is_compatible("marvell,armada38x")) {
+		arch_ioremap_caller = armada_pcie_wa_ioremap_caller;
+		pci_ioremap_set_mem_type(MT_MEMORY_RW_SO);
+		mvebu_l2x0_pl310_coherent();
+	}
+
 	l2x0_of_init(0, ~0UL);
 
 	if (of_machine_is_compatible("marvell,armada375"))
-- 
1.9.2

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

Re: [PATCHv2 4/4] ARM: mvebu: implement L2/PCIe deadlock workaround

[Arnd Bergmann]

On Tuesday 13 May 2014 12:10:39 Thomas Petazzoni wrote:
> +/*
> + * This ioremap hook is used on Armada 375/38x to ensure that PCIe
> + * memory areas are mapped as MT_MEMORY_RW_SO instead of
> + * MT_DEVICE. This is needed as a workaround for a deadlock issue
> + * between the PCIe interface and the cache controller.
> + */
> +static void __iomem *
> +armada_pcie_wa_ioremap_caller(phys_addr_t phys_addr, size_t size,
> +                             unsigned int mtype, void *caller)
> +{
> +       struct resource pcie_mem;
> +
> +       mvebu_mbus_get_pcie_mem_aperture(&pcie_mem);
> +
> +       if (pcie_mem.start <= phys_addr && (phys_addr + size) <= pcie_mem.end)
> +               mtype = MT_MEMORY_RW_SO;
> +
> +       return __arm_ioremap_caller(phys_addr, size, mtype, caller);
> +}

Hmm, I think this needs some more explanation about which flags
you are actually interested in. 

These are the three common mem types for ioremap:

#define PROT_PTE_DEVICE         L_PTE_PRESENT|L_PTE_YOUNG|L_PTE_DIRTY|L_PTE_XN
#define PROT_SECT_DEVICE        PMD_TYPE_SECT|PMD_SECT_AP_WRITE

        [MT_DEVICE] = {           /* Strongly ordered / ARMv6 shared device */
                .prot_pte       = PROT_PTE_DEVICE | L_PTE_MT_DEV_SHARED |
                                  L_PTE_SHARED,
                .prot_pte_s2    = s2_policy(PROT_PTE_S2_DEVICE) |
                                  s2_policy(L_PTE_S2_MT_DEV_SHARED) |
                                  L_PTE_SHARED,
                .prot_sect      = PROT_SECT_DEVICE | PMD_SECT_S,
                .domain         = DOMAIN_IO, 
        },                      
        [MT_DEVICE_CACHED] = {    /* ioremap_cached */
                .prot_pte       = PROT_PTE_DEVICE | L_PTE_MT_DEV_CACHED,
                .prot_sect      = PROT_SECT_DEVICE | PMD_SECT_WB,
                .domain         = DOMAIN_IO,
        },
        [MT_DEVICE_WC] = {      /* ioremap_wc */
                .prot_pte       = PROT_PTE_DEVICE | L_PTE_MT_DEV_WC,
                .prot_sect      = PROT_SECT_DEVICE,
                .domain         = DOMAIN_IO,
        },

and this is the one you enforce here:

        [MT_MEMORY_RW_SO] = {
                .prot_pte  = L_PTE_PRESENT | L_PTE_YOUNG | L_PTE_DIRTY |
                                L_PTE_MT_UNCACHED | L_PTE_XN,
                .prot_l1   = PMD_TYPE_TABLE,
                .prot_sect = PMD_TYPE_SECT | PMD_SECT_AP_WRITE | PMD_SECT_S |
                                PMD_SECT_UNCACHED | PMD_SECT_XN,
                .domain    = DOMAIN_KERNEL,
        },

So you set a different domain, and turn write-combined and cached mappings
into uncached mappings, and for uncached mappings you remove the "shared"
flag. Which of these changes is the one you actually need?

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

Re: [PATCHv2 4/4] ARM: mvebu: implement L2/PCIe deadlock workaround

[Thomas Petazzoni]

Dear Arnd Bergmann,

On Tue, 13 May 2014 13:13:39 +0200, Arnd Bergmann wrote:

> Hmm, I think this needs some more explanation about which flags
> you are actually interested in. 
> 
> These are the three common mem types for ioremap:
> 
> #define PROT_PTE_DEVICE         L_PTE_PRESENT|L_PTE_YOUNG|L_PTE_DIRTY|L_PTE_XN
> #define PROT_SECT_DEVICE        PMD_TYPE_SECT|PMD_SECT_AP_WRITE
> 
>         [MT_DEVICE] = {           /* Strongly ordered / ARMv6 shared device */
>                 .prot_pte       = PROT_PTE_DEVICE | L_PTE_MT_DEV_SHARED |
>                                   L_PTE_SHARED,
>                 .prot_pte_s2    = s2_policy(PROT_PTE_S2_DEVICE) |
>                                   s2_policy(L_PTE_S2_MT_DEV_SHARED) |
>                                   L_PTE_SHARED,
>                 .prot_sect      = PROT_SECT_DEVICE | PMD_SECT_S,
>                 .domain         = DOMAIN_IO, 
>         },                      
>         [MT_DEVICE_CACHED] = {    /* ioremap_cached */
>                 .prot_pte       = PROT_PTE_DEVICE | L_PTE_MT_DEV_CACHED,
>                 .prot_sect      = PROT_SECT_DEVICE | PMD_SECT_WB,
>                 .domain         = DOMAIN_IO,
>         },
>         [MT_DEVICE_WC] = {      /* ioremap_wc */
>                 .prot_pte       = PROT_PTE_DEVICE | L_PTE_MT_DEV_WC,
>                 .prot_sect      = PROT_SECT_DEVICE,
>                 .domain         = DOMAIN_IO,
>         },
> 
> and this is the one you enforce here:
> 
>         [MT_MEMORY_RW_SO] = {
>                 .prot_pte  = L_PTE_PRESENT | L_PTE_YOUNG | L_PTE_DIRTY |
>                                 L_PTE_MT_UNCACHED | L_PTE_XN,
>                 .prot_l1   = PMD_TYPE_TABLE,
>                 .prot_sect = PMD_TYPE_SECT | PMD_SECT_AP_WRITE | PMD_SECT_S |
>                                 PMD_SECT_UNCACHED | PMD_SECT_XN,
>                 .domain    = DOMAIN_KERNEL,
>         },
> 
> So you set a different domain, and turn write-combined and cached mappings
> into uncached mappings, and for uncached mappings you remove the "shared"
> flag. Which of these changes is the one you actually need?

I *believe* the important part is the change from L_PTE_MT_DEV_SHARED
to L_PTE_MT_UNCACHED, because:

#define L_PTE_MT_UNCACHED       (_AT(pteval_t, 0x00) << 2)
#define L_PTE_MT_DEV_SHARED     (_AT(pteval_t, 0x04) << 2)

So the former is "Strongly-Ordered" according to the ARM ARM, while the
latter is "Device Shareable".

The only detail I have access to is that the workaround is "Reads
targeting PCIe End Point must be marked Strongly Ordered", so it's
pretty limited in details.

Do you think I should create a different memory type MT_DEVICE_SO, that
remains in the DOMAIN_IO domain, but uses L_PTE_MT_UNCACHED instead of
L_PTE_MT_DEV_SHARED ?

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 1/4] of: make of_update_property() usable earlier in the boot process

[Rob Herring]

On Tue, May 13, 2014 at 5:10 AM, Thomas Petazzoni
<thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Commit 75b57ecf9d1d1e17d099ab13b8f48e6e038676be ('of: Make device
> nodes kobjects so they show up in sysfs') has turned Device Tree nodes
> in kobjects and added a sysfs based representation for Device Tree
> nodes. Since the sysfs logic is only available after the execution of
> a core_initcall(), the patch took precautions in of_add_property() and
> of_remove_property() to not do any sysfs related manipulation early in
> the boot process.
>
> However, it forgot to do the same for of_update_property(), which if
> used early in the boot process (before core_initcalls have been
> called), tries to call sysfs_remove_bin_file(), and crashes:

It actually crashes or just the warning? It would be good to give the
crash backtrace.

> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 0 at /home/thomas/projets/linux-2.6/fs/kernfs/dir.c:1216 kernfs_remove_by_name_ns+0x80/0x88()
> kernfs: can not remove '(null)', no directory
> Modules linked in:
> CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.15.0-rc1-00127-g1d7e7b2-dirty #423
> [<c0014910>] (unwind_backtrace) from [<c00110ec>] (show_stack+0x10/0x14)
> [<c00110ec>] (show_stack) from [<c04c84b8>] (dump_stack+0x84/0x94)
> [<c04c84b8>] (dump_stack) from [<c001d8c0>] (warn_slowpath_common+0x6c/0x88)
> [<c001d8c0>] (warn_slowpath_common) from [<c001d90c>] (warn_slowpath_fmt+0x30/0x40)
> [<c001d90c>] (warn_slowpath_fmt) from [<c0104468>] (kernfs_remove_by_name_ns+0x80/0x88)
> [<c0104468>] (kernfs_remove_by_name_ns) from [<c0394d98>] (of_update_property+0xc0/0xf0)
> [<c0394d98>] (of_update_property) from [<c0647248>] (mvebu_timer_and_clk_init+0xfc/0x194)
> [<c0647248>] (mvebu_timer_and_clk_init) from [<c0640934>] (start_kernel+0x218/0x350)
> [<c0640934>] (start_kernel) from [<00008070>] (0x8070)
>
> To fix this problem, we simply skip the sysfs related calls in
> of_update_property(), and rely on of_init() to fix up things when it
> will be called, exactly as is done in of_add_property() and
> of_remove_property().
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> Fixes: 75b57ecf9d1d1e17d099ab13b8f48e6e038676be ('of: Make device nodes kobjects so they show up in sysfs')

Humm, I didn't know about this new tag. This doesn't quite match what
SubmittingPatches says. It should be 12 digits of commit hash and
double quotes around the summary.

> ---
>  drivers/of/base.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index f72d19b..e981f09 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1831,11 +1831,16 @@ int of_update_property(struct device_node *np, struct property *newprop)
>         if (rc)
>                 return rc;
>
> +       /* At early boot, bail out and defer setup to of_init() */
> +       if (!of_kset)
> +               goto skip_sysfs;

I prefer to avoid the goto and do: return found ? 0 : -ENODEV;

Also, this code has changed some in 3.15 rc's, you need to rebase.
This change needs to go into 3.15. Other platforms could be broken.

Rob

> +
>         /* Update the sysfs attribute */
>         if (oldprop)
>                 sysfs_remove_bin_file(&np->kobj, &oldprop->attr);
>         __of_add_property_sysfs(np, newprop);
>
> + skip_sysfs:
>         if (!found)
>                 return -ENODEV;
>
> --
> 1.9.2
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 1/4] of: make of_update_property() usable earlier in the boot process

[Thomas Petazzoni]

Dear Rob Herring,

On Tue, 13 May 2014 09:00:02 -0500, Rob Herring wrote:

> > However, it forgot to do the same for of_update_property(), which if
> > used early in the boot process (before core_initcalls have been
> > called), tries to call sysfs_remove_bin_file(), and crashes:
> 
> It actually crashes or just the warning? It would be good to give the
> crash backtrace.

It crashes after the warning. The crash trace is:

Unable to handle kernel NULL pointer dereference at virtual address 0000003c
pgd = c0004000
[0000003c] *pgd=00000000
Internal error: Oops: 5 [#1] SMP ARM
Modules linked in:
CPU: 0 PID: 0 Comm: swapper/0 Tainted: G        W     3.15.0-rc1-00127-g1d7e7b2-dirty #423
task: c10ad4d8 ti: c10a2000 task.ti: c10a2000
PC is at kernfs_find_ns+0x8/0xf0
LR is at kernfs_find_and_get_ns+0x30/0x48
pc : [<c0103834>]    lr : [<c010394c>]    psr: 600001d3
sp : c10a3f34  ip : 00000073  fp : 00000000
r10: 00000000  r9 : cfffc240  r8 : cfdf2980
r7 : cf812c00  r6 : 00000000  r5 : 00000000  r4 : c10b45e0
r3 : c10ad4d8  r2 : 00000000  r1 : cf812c00  r0 : 00000000
Flags: nZCv  IRQs off  FIQs off  Mode SVC_32  ISA ARM  Segment kernel
Control: 10c53c7d  Table: 0000404a  DAC: 00000015
Process swapper/0 (pid: 0, stack limit = 0xc10a2240)
Stack: (0xc10a3f34 to 0xc10a4000)
3f20:                                              c10b45e0 00000000 00000000
3f40: cf812c00 c010394c 00000063 cf812c00 00000001 cf812c00 cfdf29ac c03932cc
3f60: 00000063 cf812bc0 cfdf29ac cf812c00 ffffffff c03943f8 cfdf2980 c0104468
3f80: cfdf2a04 cfdf2980 cf812bc0 c06634b0 c10aa3c0 c0394da4 c10f74dc cfdf2980
3fa0: cf812bc0 c0647248 c10aa3c0 ffffffff c10de940 c10aa3c0 ffffffff c0640934
3fc0: ffffffff ffffffff c06404ec 00000000 00000000 c06634b0 00000000 10c53c7d
3fe0: c10aa434 c06634ac c10ae4c8 0000406a 414fc091 00008070 00000000 00000000
[<c0103834>] (kernfs_find_ns) from [<00000001>] (0x1)
Code: e5c89001 eaffffcf e92d40f0 e1a06002 (e1d023bc) 
---[ end trace 3406ff24bd97382f ]---
Kernel panic - not syncing: Attempted to kill the idle task!
---[ end Kernel panic - not syncing: Attempted to kill the idle task!

> > To fix this problem, we simply skip the sysfs related calls in
> > of_update_property(), and rely on of_init() to fix up things when it
> > will be called, exactly as is done in of_add_property() and
> > of_remove_property().
> >
> > Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > Fixes: 75b57ecf9d1d1e17d099ab13b8f48e6e038676be ('of: Make device nodes kobjects so they show up in sysfs')
> 
> Humm, I didn't know about this new tag. This doesn't quite match what
> SubmittingPatches says. It should be 12 digits of commit hash and
> double quotes around the summary.

Well, I guess it's a per-maintainer choice:

git log | grep "^Fixes:"

    Fixes: 54fe26a900bc528f3df1e4235cb6b9ca5c6d4dc2 ('ARM: mvebu: Add thermal quirk for the Armada 375 DB board')
    Fixes: 54397d85349f ("ARM: kirkwood: Relocate PCIe device tree nodes")
    Fixes: a7d4f81821f7 ('ARM: mvebu: Add support for NOR flash device on Openblocks AX3 board')
    Fixes: b484ff42df47 ('ARM: mvebu: Add support for NOR flash device on Armada XP-DB board')
    Fixes: c971ff185f64 ("leds: leds-pwm: Defer led_pwm_set() if PWM can sleep")
    Fixes: abccd00f8af2 ('btrfs: Fix 32/64-bit problem with BTRFS_SET_RECEIVED_SUBVOL ioctl')
    Fixes: ee1e0994ab1bd (regulator: s5m8767: Use GPIO for controlling Buck9/eMMC)
    Fixes: 652ed95d5fa6 (cpufreq: introduce cpufreq_generic_get() routine)

Somewhat inconsistent :-)

But ok, 12 digits for the commit hash, and double quotes.

> >  drivers/of/base.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index f72d19b..e981f09 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -1831,11 +1831,16 @@ int of_update_property(struct device_node *np, struct property *newprop)
> >         if (rc)
> >                 return rc;
> >
> > +       /* At early boot, bail out and defer setup to of_init() */
> > +       if (!of_kset)
> > +               goto skip_sysfs;
> 
> I prefer to avoid the goto and do: return found ? 0 : -ENODEV;

Ok.

> Also, this code has changed some in 3.15 rc's, you need to rebase.
> This change needs to go into 3.15. Other platforms could be broken.

Ok, will submit a v2 separately.

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 1/4] of: make of_update_property() usable earlier in the boot process

[Grant Likely]

Thanks Thomas, I'll pick up the fix when you post v2. It will be a
week before it hits Linus' tree given that he is currently underwater.

g.

On Tue, May 13, 2014 at 3:30 PM, Thomas Petazzoni
<thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Dear Rob Herring,
>
> On Tue, 13 May 2014 09:00:02 -0500, Rob Herring wrote:
>
>> > However, it forgot to do the same for of_update_property(), which if
>> > used early in the boot process (before core_initcalls have been
>> > called), tries to call sysfs_remove_bin_file(), and crashes:
>>
>> It actually crashes or just the warning? It would be good to give the
>> crash backtrace.
>
> It crashes after the warning. The crash trace is:
>
> Unable to handle kernel NULL pointer dereference at virtual address 0000003c
> pgd = c0004000
> [0000003c] *pgd=00000000
> Internal error: Oops: 5 [#1] SMP ARM
> Modules linked in:
> CPU: 0 PID: 0 Comm: swapper/0 Tainted: G        W     3.15.0-rc1-00127-g1d7e7b2-dirty #423
> task: c10ad4d8 ti: c10a2000 task.ti: c10a2000
> PC is at kernfs_find_ns+0x8/0xf0
> LR is at kernfs_find_and_get_ns+0x30/0x48
> pc : [<c0103834>]    lr : [<c010394c>]    psr: 600001d3
> sp : c10a3f34  ip : 00000073  fp : 00000000
> r10: 00000000  r9 : cfffc240  r8 : cfdf2980
> r7 : cf812c00  r6 : 00000000  r5 : 00000000  r4 : c10b45e0
> r3 : c10ad4d8  r2 : 00000000  r1 : cf812c00  r0 : 00000000
> Flags: nZCv  IRQs off  FIQs off  Mode SVC_32  ISA ARM  Segment kernel
> Control: 10c53c7d  Table: 0000404a  DAC: 00000015
> Process swapper/0 (pid: 0, stack limit = 0xc10a2240)
> Stack: (0xc10a3f34 to 0xc10a4000)
> 3f20:                                              c10b45e0 00000000 00000000
> 3f40: cf812c00 c010394c 00000063 cf812c00 00000001 cf812c00 cfdf29ac c03932cc
> 3f60: 00000063 cf812bc0 cfdf29ac cf812c00 ffffffff c03943f8 cfdf2980 c0104468
> 3f80: cfdf2a04 cfdf2980 cf812bc0 c06634b0 c10aa3c0 c0394da4 c10f74dc cfdf2980
> 3fa0: cf812bc0 c0647248 c10aa3c0 ffffffff c10de940 c10aa3c0 ffffffff c0640934
> 3fc0: ffffffff ffffffff c06404ec 00000000 00000000 c06634b0 00000000 10c53c7d
> 3fe0: c10aa434 c06634ac c10ae4c8 0000406a 414fc091 00008070 00000000 00000000
> [<c0103834>] (kernfs_find_ns) from [<00000001>] (0x1)
> Code: e5c89001 eaffffcf e92d40f0 e1a06002 (e1d023bc)
> ---[ end trace 3406ff24bd97382f ]---
> Kernel panic - not syncing: Attempted to kill the idle task!
> ---[ end Kernel panic - not syncing: Attempted to kill the idle task!
>
>> > To fix this problem, we simply skip the sysfs related calls in
>> > of_update_property(), and rely on of_init() to fix up things when it
>> > will be called, exactly as is done in of_add_property() and
>> > of_remove_property().
>> >
>> > Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>> > Fixes: 75b57ecf9d1d1e17d099ab13b8f48e6e038676be ('of: Make device nodes kobjects so they show up in sysfs')
>>
>> Humm, I didn't know about this new tag. This doesn't quite match what
>> SubmittingPatches says. It should be 12 digits of commit hash and
>> double quotes around the summary.
>
> Well, I guess it's a per-maintainer choice:
>
> git log | grep "^Fixes:"
>
>     Fixes: 54fe26a900bc528f3df1e4235cb6b9ca5c6d4dc2 ('ARM: mvebu: Add thermal quirk for the Armada 375 DB board')
>     Fixes: 54397d85349f ("ARM: kirkwood: Relocate PCIe device tree nodes")
>     Fixes: a7d4f81821f7 ('ARM: mvebu: Add support for NOR flash device on Openblocks AX3 board')
>     Fixes: b484ff42df47 ('ARM: mvebu: Add support for NOR flash device on Armada XP-DB board')
>     Fixes: c971ff185f64 ("leds: leds-pwm: Defer led_pwm_set() if PWM can sleep")
>     Fixes: abccd00f8af2 ('btrfs: Fix 32/64-bit problem with BTRFS_SET_RECEIVED_SUBVOL ioctl')
>     Fixes: ee1e0994ab1bd (regulator: s5m8767: Use GPIO for controlling Buck9/eMMC)
>     Fixes: 652ed95d5fa6 (cpufreq: introduce cpufreq_generic_get() routine)
>
> Somewhat inconsistent :-)
>
> But ok, 12 digits for the commit hash, and double quotes.
>
>> >  drivers/of/base.c | 5 +++++
>> >  1 file changed, 5 insertions(+)
>> >
>> > diff --git a/drivers/of/base.c b/drivers/of/base.c
>> > index f72d19b..e981f09 100644
>> > --- a/drivers/of/base.c
>> > +++ b/drivers/of/base.c
>> > @@ -1831,11 +1831,16 @@ int of_update_property(struct device_node *np, struct property *newprop)
>> >         if (rc)
>> >                 return rc;
>> >
>> > +       /* At early boot, bail out and defer setup to of_init() */
>> > +       if (!of_kset)
>> > +               goto skip_sysfs;
>>
>> I prefer to avoid the goto and do: return found ? 0 : -ENODEV;
>
> Ok.
>
>> Also, this code has changed some in 3.15 rc's, you need to rebase.
>> This change needs to go into 3.15. Other platforms could be broken.
>
> Ok, will submit a v2 separately.
>
> Thanks,
>
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 1/4] of: make of_update_property() usable earlier in the boot process

[Jason Cooper]

On Tue, May 13, 2014 at 04:30:26PM +0200, Thomas Petazzoni wrote:
> On Tue, 13 May 2014 09:00:02 -0500, Rob Herring wrote:
...
> > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > > Fixes: 75b57ecf9d1d1e17d099ab13b8f48e6e038676be ('of: Make device nodes kobjects so they show up in sysfs')
> > 
> > Humm, I didn't know about this new tag. This doesn't quite match what
> > SubmittingPatches says. It should be 12 digits of commit hash and
> > double quotes around the summary.
> 
> Well, I guess it's a per-maintainer choice:
> 
> git log | grep "^Fixes:"
> 
>     Fixes: 54fe26a900bc528f3df1e4235cb6b9ca5c6d4dc2 ('ARM: mvebu: Add thermal quirk for the Armada 375 DB board')

Well, just because the maintainer is an idiot and didn't catch it isn't
an excuse to continue the behavior. ;-)

>     Fixes: 54397d85349f ("ARM: kirkwood: Relocate PCIe device tree nodes")
>     Fixes: a7d4f81821f7 ('ARM: mvebu: Add support for NOR flash device on Openblocks AX3 board')
>     Fixes: b484ff42df47 ('ARM: mvebu: Add support for NOR flash device on Armada XP-DB board')
>     Fixes: c971ff185f64 ("leds: leds-pwm: Defer led_pwm_set() if PWM can sleep")
>     Fixes: abccd00f8af2 ('btrfs: Fix 32/64-bit problem with BTRFS_SET_RECEIVED_SUBVOL ioctl')
>     Fixes: ee1e0994ab1bd (regulator: s5m8767: Use GPIO for controlling Buck9/eMMC)
>     Fixes: 652ed95d5fa6 (cpufreq: introduce cpufreq_generic_get() routine)
> 
> Somewhat inconsistent :-)

Yeah, I can go either way on the single quotes/double quotes.  The
12-character hash definitely increases readability, though.

thx,

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

Re: [PATCHv2 1/4] of: make of_update_property() usable earlier in the boot process

[Thomas Petazzoni]

Dear Jason Cooper,

On Tue, 13 May 2014 11:30:06 -0400, Jason Cooper wrote:

> > Well, I guess it's a per-maintainer choice:
> > 
> > git log | grep "^Fixes:"
> > 
> >     Fixes: 54fe26a900bc528f3df1e4235cb6b9ca5c6d4dc2 ('ARM: mvebu: Add thermal quirk for the Armada 375 DB board')
> 
> Well, just because the maintainer is an idiot and didn't catch it isn't
> an excuse to continue the behavior. ;-)

Yes, no problem :)

> >     Fixes: 54397d85349f ("ARM: kirkwood: Relocate PCIe device tree nodes")
> >     Fixes: a7d4f81821f7 ('ARM: mvebu: Add support for NOR flash device on Openblocks AX3 board')
> >     Fixes: b484ff42df47 ('ARM: mvebu: Add support for NOR flash device on Armada XP-DB board')
> >     Fixes: c971ff185f64 ("leds: leds-pwm: Defer led_pwm_set() if PWM can sleep")
> >     Fixes: abccd00f8af2 ('btrfs: Fix 32/64-bit problem with BTRFS_SET_RECEIVED_SUBVOL ioctl')
> >     Fixes: ee1e0994ab1bd (regulator: s5m8767: Use GPIO for controlling Buck9/eMMC)
> >     Fixes: 652ed95d5fa6 (cpufreq: introduce cpufreq_generic_get() routine)
> > 
> > Somewhat inconsistent :-)
> 
> Yeah, I can go either way on the single quotes/double quotes.  The
> 12-character hash definitely increases readability, though.

I must say I never understood the logic here. We used to use 8 digit
hashes, and then we had collisions. So it means that if we look at the
Git history now, some of these 8 digit hashes no longer uniquely
identify a commit.

To fix this up, we moved to use 12 digit hashes. But that's just
pushing the problem a bit further away, no? There will be some
collision at some point, and therefore in the future 652ed95d5fa6 may
no longer be a unique identifier for the "cpufreq: introduce
cpufreq_generic_get() routine" commit, and therefore people reading the
Git history 3 or 5 years from now will see non-unique identifiers in
'Fixes:' fields.

To me, it would make a lot more sense to use full hashes. I don't
really see how it decreases readability, and it's the most future proof
solution we have (knowing that of course, collisions are still
theoretically possible).

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 1/4] of: make of_update_property() usable earlier in the boot process

[Jason Cooper]

On Tue, May 13, 2014 at 05:54:04PM +0200, Thomas Petazzoni wrote:
> Dear Jason Cooper,
> 
> On Tue, 13 May 2014 11:30:06 -0400, Jason Cooper wrote:
> 
> > > Well, I guess it's a per-maintainer choice:
> > > 
> > > git log | grep "^Fixes:"
> > > 
> > >     Fixes: 54fe26a900bc528f3df1e4235cb6b9ca5c6d4dc2 ('ARM: mvebu: Add thermal quirk for the Armada 375 DB board')
> > 
> > Well, just because the maintainer is an idiot and didn't catch it isn't
> > an excuse to continue the behavior. ;-)
> 
> Yes, no problem :)
> 
> > >     Fixes: 54397d85349f ("ARM: kirkwood: Relocate PCIe device tree nodes")
> > >     Fixes: a7d4f81821f7 ('ARM: mvebu: Add support for NOR flash device on Openblocks AX3 board')
> > >     Fixes: b484ff42df47 ('ARM: mvebu: Add support for NOR flash device on Armada XP-DB board')
> > >     Fixes: c971ff185f64 ("leds: leds-pwm: Defer led_pwm_set() if PWM can sleep")
> > >     Fixes: abccd00f8af2 ('btrfs: Fix 32/64-bit problem with BTRFS_SET_RECEIVED_SUBVOL ioctl')
> > >     Fixes: ee1e0994ab1bd (regulator: s5m8767: Use GPIO for controlling Buck9/eMMC)
> > >     Fixes: 652ed95d5fa6 (cpufreq: introduce cpufreq_generic_get() routine)
> > > 
> > > Somewhat inconsistent :-)
> > 
> > Yeah, I can go either way on the single quotes/double quotes.  The
> > 12-character hash definitely increases readability, though.
> 
> I must say I never understood the logic here. We used to use 8 digit
> hashes, and then we had collisions. So it means that if we look at the
> Git history now, some of these 8 digit hashes no longer uniquely
> identify a commit.

iirc, it was 7, and when git saw a collision, it stepped out to eight.
eg during a git log --oneline.

> To fix this up, we moved to use 12 digit hashes. But that's just
> pushing the problem a bit further away, no? There will be some
> collision at some point, and therefore in the future 652ed95d5fa6 may
> no longer be a unique identifier for the "cpufreq: introduce
> cpufreq_generic_get() routine" commit, and therefore people reading the
> Git history 3 or 5 years from now will see non-unique identifiers in
> 'Fixes:' fields.

This is where the logic is flawed.  Hypothetically, 5 years later,
652ed95d5fa6 is no longer unique, but the patch subject line *is* unique
among the set that matches 652ed95d5fa6.

> To me, it would make a lot more sense to use full hashes. I don't
> really see how it decreases readability, and it's the most future proof
> solution we have (knowing that of course, collisions are still
> theoretically possible).

If there were a full sha1 collision between two different, legitimate
patches, they would still have different subject lines.  Since the Fixes
tag works in the event of either sort of collision (12, full), may as
well use 12.

Not to mention, since there are no collisions yet, you can be reasonably
certain that of the two commits matching 652ed95d5fa6, it's the one
that came before the commit containing the Fixes tag. :)

thx,

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

Re: [PATCHv2 1/4] of: make of_update_property() usable earlier in the boot process

[Rob Herring]

On Tue, May 13, 2014 at 10:54 AM, Thomas Petazzoni
<thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Dear Jason Cooper,
>
> On Tue, 13 May 2014 11:30:06 -0400, Jason Cooper wrote:
>
>> > Well, I guess it's a per-maintainer choice:
>> >
>> > git log | grep "^Fixes:"
>> >
>> >     Fixes: 54fe26a900bc528f3df1e4235cb6b9ca5c6d4dc2 ('ARM: mvebu: Add thermal quirk for the Armada 375 DB board')
>>
>> Well, just because the maintainer is an idiot and didn't catch it isn't
>> an excuse to continue the behavior. ;-)

To be fair, documentation was added last week! I only noticed now
because I just found and read the documentation. In the future, I'll
probably forget and not notice thus becoming an idiot again. ;)

>
> Yes, no problem :)
>
>> >     Fixes: 54397d85349f ("ARM: kirkwood: Relocate PCIe device tree nodes")
>> >     Fixes: a7d4f81821f7 ('ARM: mvebu: Add support for NOR flash device on Openblocks AX3 board')
>> >     Fixes: b484ff42df47 ('ARM: mvebu: Add support for NOR flash device on Armada XP-DB board')
>> >     Fixes: c971ff185f64 ("leds: leds-pwm: Defer led_pwm_set() if PWM can sleep")
>> >     Fixes: abccd00f8af2 ('btrfs: Fix 32/64-bit problem with BTRFS_SET_RECEIVED_SUBVOL ioctl')
>> >     Fixes: ee1e0994ab1bd (regulator: s5m8767: Use GPIO for controlling Buck9/eMMC)
>> >     Fixes: 652ed95d5fa6 (cpufreq: introduce cpufreq_generic_get() routine)
>> >
>> > Somewhat inconsistent :-)
>>
>> Yeah, I can go either way on the single quotes/double quotes.  The
>> 12-character hash definitely increases readability, though.
>
> I must say I never understood the logic here. We used to use 8 digit
> hashes, and then we had collisions. So it means that if we look at the
> Git history now, some of these 8 digit hashes no longer uniquely
> identify a commit.
>
> To fix this up, we moved to use 12 digit hashes. But that's just
> pushing the problem a bit further away, no? There will be some
> collision at some point, and therefore in the future 652ed95d5fa6 may
> no longer be a unique identifier for the "cpufreq: introduce
> cpufreq_generic_get() routine" commit, and therefore people reading the
> Git history 3 or 5 years from now will see non-unique identifiers in
> 'Fixes:' fields.

You could assume that the first match with "git log --online <commit
with fix> | grep <commit with bug>" is the offending commit and will
always work with only a few digits.

If it really becomes a frequent issue, we could add a git rev-parse
option to only look at ancestor commits to resolve hashes.

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

Re: [PATCHv2 1/4] of: make of_update_property() usable earlier in the boot process

[Jason Cooper]

On Tue, May 13, 2014 at 11:58:08AM -0500, Rob Herring wrote:
> On Tue, May 13, 2014 at 10:54 AM, Thomas Petazzoni
> <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > Dear Jason Cooper,
> >
> > On Tue, 13 May 2014 11:30:06 -0400, Jason Cooper wrote:
> >
> >> > Well, I guess it's a per-maintainer choice:
> >> >
> >> > git log | grep "^Fixes:"
> >> >
> >> >     Fixes: 54fe26a900bc528f3df1e4235cb6b9ca5c6d4dc2 ('ARM: mvebu: Add thermal quirk for the Armada 375 DB board')
> >>
> >> Well, just because the maintainer is an idiot and didn't catch it isn't
> >> an excuse to continue the behavior. ;-)
> 
> To be fair, documentation was added last week! I only noticed now
> because I just found and read the documentation. In the future, I'll
> probably forget and not notice thus becoming an idiot again. ;)

um.. I thought that was me.  I usually don't make it a habit of calling
other people idiot.  I reserve that for myself. ;-)

thx,

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

Re: [PATCHv2 3/4] ARM: mm: add support for HW coherent systems in PL310

[Catalin Marinas]

On Tue, May 13, 2014 at 11:10:38AM +0100, Thomas Petazzoni wrote:
> --- a/Documentation/devicetree/bindings/arm/l2cc.txt
> +++ b/Documentation/devicetree/bindings/arm/l2cc.txt
> @@ -8,6 +8,8 @@ Required properties:
>  
>  - compatible : should be one of:
>    "arm,pl310-cache"
> +  "arm,pl310-coherent-cache", used for I/O coherent platforms using
> +     the PL310 cache
>    "arm,l220-cache"
>    "arm,l210-cache"
>    "bcm,bcm11351-a2-pl310-cache": DEPRECATED by "brcm,bcm11351-a2-pl310-cache"

The binding name kind of implies that we have a transparent PL310 cache
(at least to me), which is not the case. I would rather have a specific
dma-coherent property or something similar since it's not another type
of PL310 but rather a different SoC topology.

But I recall you mentioned that you can't make this decision at the DT
level since you don't know before whether the SoC is I/O coherent or
not.

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

Re: [PATCHv2 3/4] ARM: mm: add support for HW coherent systems in PL310

[Thomas Petazzoni]

Dear Catalin Marinas,

On Wed, 14 May 2014 15:34:48 +0100, Catalin Marinas wrote:
> On Tue, May 13, 2014 at 11:10:38AM +0100, Thomas Petazzoni wrote:
> > --- a/Documentation/devicetree/bindings/arm/l2cc.txt
> > +++ b/Documentation/devicetree/bindings/arm/l2cc.txt
> > @@ -8,6 +8,8 @@ Required properties:
> >  
> >  - compatible : should be one of:
> >    "arm,pl310-cache"
> > +  "arm,pl310-coherent-cache", used for I/O coherent platforms using
> > +     the PL310 cache
> >    "arm,l220-cache"
> >    "arm,l210-cache"
> >    "bcm,bcm11351-a2-pl310-cache": DEPRECATED by "brcm,bcm11351-a2-pl310-cache"
> 
> The binding name kind of implies that we have a transparent PL310 cache
> (at least to me), which is not the case. I would rather have a specific
> dma-coherent property or something similar since it's not another type
> of PL310 but rather a different SoC topology.

Fine with me. A separate property or a different compatible string is
the same.

> But I recall you mentioned that you can't make this decision at the DT
> level since you don't know before whether the SoC is I/O coherent or
> not.

As of today, hardware I/O coherency can only be enabled if CONFIG_SMP
is enabled *and* the SoC is actually multi-core, due to limitations in
the ARM core code. So if you look at my PATCH 4/4 in this series, you
will see that I adjust the compatible string of the cache controller at
run time, depending on whether hardware I/O coherency is enabled or not:

+static void __init mvebu_l2x0_pl310_coherent(void)
+{
+	struct device_node *np;
+
+	if (!coherency_available())
+		return;
+
+	for_each_compatible_node(np, NULL, "arm,pl310-cache") {
+		struct property *new_compat;
+
+		new_compat = kzalloc(sizeof(*new_compat), GFP_KERNEL);
+		new_compat->name = kstrdup("compatible", GFP_KERNEL);
+		new_compat->value = kstrdup("arm,pl310-coherent-cache",
+					    GFP_KERNEL);
+		new_compat->length = strlen(new_compat->value) + 1;
+		of_update_property(np, new_compat);
+	}
+}

So, this code can just as well be changed to add a property instead of
adjusting the compatible property. It's the same thing for me.

Also, I will send (hopefully today) a series that allows to enable
hardware I/O coherency even on !SMP configurations. But I believe it
might take a while to get merged.

Best regards,

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

Re: [PATCHv2 4/4] ARM: mvebu: implement L2/PCIe deadlock workaround

[Catalin Marinas]

On Tue, May 13, 2014 at 11:10:39AM +0100, Thomas Petazzoni wrote:
> --- a/arch/arm/mach-mvebu/board-v7.c
> +++ b/arch/arm/mach-mvebu/board-v7.c
[...]
> +static void __init mvebu_l2x0_pl310_coherent(void)
> +{
> +	struct device_node *np;
> +
> +	if (!coherency_available())
> +		return;
> +
> +	for_each_compatible_node(np, NULL, "arm,pl310-cache") {
> +		struct property *new_compat;
> +
> +		new_compat = kzalloc(sizeof(*new_compat), GFP_KERNEL);
> +		new_compat->name = kstrdup("compatible", GFP_KERNEL);
> +		new_compat->value = kstrdup("arm,pl310-coherent-cache",
> +					    GFP_KERNEL);
> +		new_compat->length = strlen(new_compat->value) + 1;
> +		of_update_property(np, new_compat);
> +	}
> +}

I got it know, you update the DT property on the fly.

> +
>  static void __init mvebu_timer_and_clk_init(void)
>  {
>  	of_clk_init(NULL);
> @@ -78,6 +125,14 @@ static void __init mvebu_timer_and_clk_init(void)
>  	mvebu_scu_enable();
>  	coherency_init();
>  	BUG_ON(mvebu_mbus_dt_init(coherency_available()));
> +
> +	if (of_machine_is_compatible("marvell,armada375") ||
> +	    of_machine_is_compatible("marvell,armada38x")) {
> +		arch_ioremap_caller = armada_pcie_wa_ioremap_caller;
> +		pci_ioremap_set_mem_type(MT_MEMORY_RW_SO);
> +		mvebu_l2x0_pl310_coherent();
> +	}
> +
>  	l2x0_of_init(0, ~0UL);
>  
>  	if (of_machine_is_compatible("marvell,armada375"))

A more "hackish" way would be to simply set outer_cache.sync to NULL
after l2x0_of_init, less code ;) but not nice either. With the .fixup in
Russell's L2C patches you could have added an mvebu specific callback to
check for coherency before setting .sync to NULL.

Anyway, your approach is fine by me but I would prefer a specific
property rather than "compatible".

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

Re: [PATCHv2 2/4] ARM: mm: allow sub-architectures to override PCI I/O memory type

[Catalin Marinas]

On Tue, May 13, 2014 at 11:10:37AM +0100, Thomas Petazzoni wrote:
> Due to a design incompatibility between the PCIe Marvell controller
> and the Cortex-A9, stressing PCIe devices with a lot of traffic
> quickly causes a deadlock.
> 
> One part of the workaround for this is to have all PCIe regions mapped
> as MT_MEMORY_SO instead of MT_DEVICE. While the arch_ioremap_caller()
> mechanism allows sub-architecture code to override ioremap(), used to
> map PCIe memory regions, there isn't such a mechanism to override the
> behavior of pci_ioremap_io().
> 
> This commit adds the arch_pci_ioremap_mem_type variable, initialized
> to MT_DEVICE by default, and that sub-architecture code can
> override. We have chosen to expose a single variable rather than
> offering the possibility of overriding the entire pci_ioremap_io(),
> because implementing pci_ioremap_io() requires calling functions
> (get_mem_type()) that are private to the arch/arm/mm/ code.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Acked-by: Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 4/4] ARM: mvebu: implement L2/PCIe deadlock workaround

[Thomas Petazzoni]

Dear Catalin Marinas,

On Wed, 14 May 2014 15:58:54 +0100, Catalin Marinas wrote:

> > +	for_each_compatible_node(np, NULL, "arm,pl310-cache") {
> > +		struct property *new_compat;
> > +
> > +		new_compat = kzalloc(sizeof(*new_compat), GFP_KERNEL);
> > +		new_compat->name = kstrdup("compatible", GFP_KERNEL);
> > +		new_compat->value = kstrdup("arm,pl310-coherent-cache",
> > +					    GFP_KERNEL);
> > +		new_compat->length = strlen(new_compat->value) + 1;
> > +		of_update_property(np, new_compat);
> > +	}
> > +}
> 
> I got it know, you update the DT property on the fly.

Correct.

> >  static void __init mvebu_timer_and_clk_init(void)
> >  {
> >  	of_clk_init(NULL);
> > @@ -78,6 +125,14 @@ static void __init mvebu_timer_and_clk_init(void)
> >  	mvebu_scu_enable();
> >  	coherency_init();
> >  	BUG_ON(mvebu_mbus_dt_init(coherency_available()));
> > +
> > +	if (of_machine_is_compatible("marvell,armada375") ||
> > +	    of_machine_is_compatible("marvell,armada38x")) {
> > +		arch_ioremap_caller = armada_pcie_wa_ioremap_caller;
> > +		pci_ioremap_set_mem_type(MT_MEMORY_RW_SO);
> > +		mvebu_l2x0_pl310_coherent();
> > +	}
> > +
> >  	l2x0_of_init(0, ~0UL);
> >  
> >  	if (of_machine_is_compatible("marvell,armada375"))
> 
> A more "hackish" way would be to simply set outer_cache.sync to NULL
> after l2x0_of_init, less code ;) but not nice either.

How could this be done? All the outer_cache structures are 'static' in
mm/cache-l2x0.c, so the mvebu code cannot hack them after calling
l2x0_of_init().

> With the .fixup in
> Russell's L2C patches you could have added an mvebu specific callback to
> check for coherency before setting .sync to NULL.

I had a look at the ->fixup thing from Russell, but it's only meant to
call fixup operations *inside* the L2C driver. Not external fixup
operations, like operations defined by the platform. Therefore I don't
see how the ->fixup mechanism proposed by Russell would have solved
the particular problem we're discussing.

> Anyway, your approach is fine by me but I would prefer a specific
> property rather than "compatible".

Ok, thanks, I'll repost. For now, I'll repost a series without the
strongly-ordered thing, since we don't have the confirmation it's
absolutely needed for the workaround of the PCIe/SMP/PL310 deadlock
problem.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 4/4] ARM: mvebu: implement L2/PCIe deadlock workaround

[Catalin Marinas]

On Tue, May 13, 2014 at 01:52:05PM +0100, Thomas Petazzoni wrote:
> On Tue, 13 May 2014 13:13:39 +0200, Arnd Bergmann wrote:
> > and this is the one you enforce here:
> > 
> >         [MT_MEMORY_RW_SO] = {
> >                 .prot_pte  = L_PTE_PRESENT | L_PTE_YOUNG | L_PTE_DIRTY |
> >                                 L_PTE_MT_UNCACHED | L_PTE_XN,
> >                 .prot_l1   = PMD_TYPE_TABLE,
> >                 .prot_sect = PMD_TYPE_SECT | PMD_SECT_AP_WRITE | PMD_SECT_S |
> >                                 PMD_SECT_UNCACHED | PMD_SECT_XN,
> >                 .domain    = DOMAIN_KERNEL,
> >         },
> > 
> > So you set a different domain, and turn write-combined and cached mappings
> > into uncached mappings, and for uncached mappings you remove the "shared"
> > flag. Which of these changes is the one you actually need?
> 
> I *believe* the important part is the change from L_PTE_MT_DEV_SHARED
> to L_PTE_MT_UNCACHED, because:
> 
> #define L_PTE_MT_UNCACHED       (_AT(pteval_t, 0x00) << 2)
> #define L_PTE_MT_DEV_SHARED     (_AT(pteval_t, 0x04) << 2)
> 
> So the former is "Strongly-Ordered" according to the ARM ARM, while the
> latter is "Device Shareable".
> 
> The only detail I have access to is that the workaround is "Reads
> targeting PCIe End Point must be marked Strongly Ordered", so it's
> pretty limited in details.
> 
> Do you think I should create a different memory type MT_DEVICE_SO, that
> remains in the DOMAIN_IO domain, but uses L_PTE_MT_UNCACHED instead of
> L_PTE_MT_DEV_SHARED ?

I don't think it's worth, we no longer use domains on ARMv7. You could
even change the domain to IO here (it seems to be used only by omap for
a workaround) or collapse both into MT_UNCACHED which seems to have
similar needs (and used by iop3xx only).

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