Re: [PATCH 1/2] PCI: generic: Refactor code to enable reuse by other drivers.

[Will Deacon]

On Mon, Dec 21, 2015 at 05:53:41PM -0800, David Daney wrote:
> From: David Daney 
> 
> No change in functionality.
> 
> Move structure definitions into a separate header file.  Split probe
> function in to two parts:
> 
>- a small driver specific probe function (gen_pci_probe)
> 
>- a common probe that can be used by other drivers
>  (gen_pci_common_probe)
> 
> Signed-off-by: David Daney 
> ---
>  drivers/pci/host/pci-host-generic.c | 53 ---
>  drivers/pci/host/pci-host-generic.h | 56 
> +
>  2 files changed, 74 insertions(+), 35 deletions(-)
>  create mode 100644 drivers/pci/host/pci-host-generic.h
> 
> diff --git a/drivers/pci/host/pci-host-generic.c 
> b/drivers/pci/host/pci-host-generic.c
> index 5434c90..e83cec7 100644
> --- a/drivers/pci/host/pci-host-generic.c
> +++ b/drivers/pci/host/pci-host-generic.c
> @@ -25,33 +25,7 @@
>  #include 
>  #include 
>  
> -struct gen_pci_cfg_bus_ops {
> - u32 bus_shift;
> - struct pci_ops ops;
> -};
> -
> -struct gen_pci_cfg_windows {
> - struct resource res;
> - struct resource *bus_range;
> - void __iomem**win;
> -
> - struct gen_pci_cfg_bus_ops  *ops;
> -};
> -
> -/*
> - * ARM pcibios functions expect the ARM struct pci_sys_data as the PCI
> - * sysdata.  Add pci_sys_data as the first element in struct gen_pci so
> - * that when we use a gen_pci pointer as sysdata, it is also a pointer to
> - * a struct pci_sys_data.
> - */
> -struct gen_pci {
> -#ifdef CONFIG_ARM
> - struct pci_sys_data sys;
> -#endif
> - struct pci_host_bridge  host;
> - struct gen_pci_cfg_windows  cfg;
> - struct list_headresources;
> -};
> +#include "pci-host-generic.h"
>  
>  static void __iomem *gen_pci_map_cfg_bus_cam(struct pci_bus *bus,
>unsigned int devfn,
> @@ -208,19 +182,15 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci 
> *pci)
>   return 0;
>  }
>  
> -static int gen_pci_probe(struct platform_device *pdev)
> +int gen_pci_common_probe(struct platform_device *pdev,
> +  struct gen_pci *pci)

Whilst I'm fine with this patch, I don't know how Bjorn will feel about
exposing this function outside of the generic host driver. We could avoid
it by turning things upside-down and having the generic driver probe
the other drivers by matching a compatible string with a probe function
pointer, but I'd be interested to see what others think.

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

Re: [PATCH 2/2] pci, pcie-thunder-pem: Add PCIe host driver for ThunderX processors.

[Will Deacon]

On Mon, Dec 21, 2015 at 05:53:42PM -0800, David Daney wrote:
> From: David Daney 
> 
> Some Cavium ThunderX processors require quirky access methods for the
> config space of the PCIe bridge.  Add a driver to provide these config
> space accessor functions.  The pci-host-generic driver code is used to
> configure the PCI machinery.
> 
> Signed-off-by: David Daney 
> ---
>  .../devicetree/bindings/pci/pcie-thunder-pem.txt   |  43 
>  drivers/pci/host/Kconfig   |   6 +
>  drivers/pci/host/Makefile  |   1 +
>  drivers/pci/host/pcie-thunder-pem.c| 283 
> +
>  4 files changed, 333 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/pcie-thunder-pem.txt
>  create mode 100644 drivers/pci/host/pcie-thunder-pem.c
> 
> diff --git a/Documentation/devicetree/bindings/pci/pcie-thunder-pem.txt 
> b/Documentation/devicetree/bindings/pci/pcie-thunder-pem.txt
> new file mode 100644
> index 000..66824d5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/pcie-thunder-pem.txt
> @@ -0,0 +1,43 @@
> +* ThunderX PEM PCIe host controller
> +
> +Firmware-initialized PCIe host controller found on some Cavium
> +ThunderX processors.
> +
> +The properties and their meanings are identical to those described in
> +host-heneric-pci.txt except as listed below.
> +
> +Properties of the host controller node that differ from
> +host-heneric-pci.txt:

Consistently odd typo (s/heneric/generic/)!

> +
> +- compatible : Must be "cavium,pci-host-thunder-pem"
> +
> +- reg: Two entries: First the configuration space for down
> +   stream devices base address and size, as accessed
> +   from the parent bus. Second, the register bank of
> +   the PEM device PCIe bridge.
> +
> +Example:
> +
> +pem2 {
> + compatible = "cavium,pci-host-thunder-pem";
> + device_type = "pci";
> + msi-parent = <&its>;
> + msi-map = <0 &its 0x1 0x1>;
> + bus-range = <0x8f 0xc7>;
> + #size-cells = <2>;
> + #address-cells = <3>;
> +
> + reg = <0x8880 0x8f00 0x0 0x3900>,  /* Configuration space */
> +   <0x87e0 0xc200 0x0 0x0001>; /* PEM space */
> + ranges = <0x0100 0x00 0x0002 0x88b0 0x0002 0x00 
> 0x0001>, /* I/O */
> +  <0x0300 0x00 0x1000 0x8890 0x1000 0x0f 
> 0xf000>, /* mem64 */
> +  <0x4300 0x10 0x 0x88a0 0x 0x10 
> 0x>, /* mem64-pref */
> +  <0x0300 0x87e0 0xc2f0 0x87e0 0xc200 0x00 
> 0x0010>; /* mem64 PEM BAR4 */
> +
> + #interrupt-cells = <1>;
> + interrupt-map-mask = <0 0 0 7>;
> + interrupt-map = <0 0 0 1 &gic0 0 0 0 24 4>, /* INTA */
> + <0 0 0 2 &gic0 0 0 0 25 4>, /* INTB */
> + <0 0 0 3 &gic0 0 0 0 26 4>, /* INTC */
> + <0 0 0 4 &gic0 0 0 0 27 4>; /* INTD */
> +};
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index f131ba9..16ed9c3 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -172,4 +172,10 @@ config PCI_HISI
>   help
> Say Y here if you want PCIe controller support on HiSilicon HIP05 SoC
>  
> +config PCIE_HOST_THUNDER_PEM
> + bool "Cavium Thunder PCIe controller to off-chip devices"
> + depends on PCI_HOST_GENERIC && ARM64

|| COMPILE_TEST ?

(or does the use of writeq get you? If so, maybe COMPILE_TEST && 64BIT)

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

Re: [PATCH v7 1/4] arm64, numa: adding numa support for arm64 platforms.

[Will Deacon]

On Tue, Dec 22, 2015 at 03:04:48PM +0530, Ganapatrao Kulkarni wrote:
> On Fri, Dec 18, 2015 at 12:00 AM, Ganapatrao Kulkarni
>  wrote:
> > On Thu, Dec 17, 2015 at 10:41 PM, Will Deacon  wrote:
> >> This all looks pretty reasonable, but I'd like to see an Ack from a
> >> devicetree maintainer on the binding before I merge anything (and I see
> >> that there are outstanding comments from Rutland on that).
> > IIUC, there are no open comments for the binding.
> > Mark Rutland: please let me know, if there any open comments.
> > otherwise, can you please Ack the binding.
> >>
> >> On Tue, Nov 17, 2015 at 10:50:40PM +0530, Ganapatrao Kulkarni wrote:
> >>> Adding numa support for arm64 based platforms.
> >>> This patch adds by default the dummy numa node and
> >>> maps all memory and cpus to node 0.
> >>> using this patch, numa can be simulated on single node arm64 platforms.
> >>>
> >>> Reviewed-by: Robert Richter 
> >>> Signed-off-by: Ganapatrao Kulkarni 
> >>> ---
> >>>  arch/arm64/Kconfig  |  25 +++
> >>>  arch/arm64/include/asm/mmzone.h |  17 ++
> >>>  arch/arm64/include/asm/numa.h   |  47 +
> >>>  arch/arm64/kernel/setup.c   |   4 +
> >>>  arch/arm64/kernel/smp.c |   2 +
> >>>  arch/arm64/mm/Makefile  |   1 +
> >>>  arch/arm64/mm/init.c|  30 +++-
> >>>  arch/arm64/mm/numa.c| 384 
> >>> 
> >>>  8 files changed, 506 insertions(+), 4 deletions(-)
> >>>  create mode 100644 arch/arm64/include/asm/mmzone.h
> >>>  create mode 100644 arch/arm64/include/asm/numa.h
> >>>  create mode 100644 arch/arm64/mm/numa.c
> >>>
> >>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> >>> index 9ac16a4..7d8fb42 100644
> >>> --- a/arch/arm64/Kconfig
> >>> +++ b/arch/arm64/Kconfig
> >>> @@ -71,6 +71,7 @@ config ARM64
> >>>   select HAVE_GENERIC_DMA_COHERENT
> >>>   select HAVE_HW_BREAKPOINT if PERF_EVENTS
> >>>   select HAVE_MEMBLOCK
> >>> + select HAVE_MEMBLOCK_NODE_MAP if NUMA
> >>>   select HAVE_PATA_PLATFORM
> >>>   select HAVE_PERF_EVENTS
> >>>   select HAVE_PERF_REGS
> >>> @@ -482,6 +483,30 @@ config HOTPLUG_CPU
> >>> Say Y here to experiment with turning CPUs off and on.  CPUs
> >>> can be controlled through /sys/devices/system/cpu.
> >>>
> >>> +# Common NUMA Features
> >>> +config NUMA
> >>> + bool "Numa Memory Allocation and Scheduler Support"
> >>> + depends on SMP
> >>> + help
> >>> +   Enable NUMA (Non Uniform Memory Access) support.
> >>> +
> >>> +   The kernel will try to allocate memory used by a CPU on the
> >>> +   local memory controller of the CPU and add some more
> >>> +   NUMA awareness to the kernel.
> >>
> >> I appreciate that this is copied from x86, but what exactly do you mean
> >> by "memory controller" here?
> > ok, it is fair enough to say local memory.
> >>
> >>> diff --git a/arch/arm64/include/asm/mmzone.h 
> >>> b/arch/arm64/include/asm/mmzone.h
> >>> new file mode 100644
> >>> index 000..6ddd468
> >>> --- /dev/null
> >>> +++ b/arch/arm64/include/asm/mmzone.h
> >>> @@ -0,0 +1,17 @@
> >>> +#ifndef __ASM_ARM64_MMZONE_H_
> >>> +#define __ASM_ARM64_MMZONE_H_
> >>
> >> Please try to follow the standard naming for header guards under arm64
> >> (yes, it's not perfect, but we've made some effort for consistency).
> > sure, will follow as in other code.
> >>
> >>> +
> >>> +#ifdef CONFIG_NUMA
> >>> +
> >>> +#include 
> >>> +#include 
> >>> +
> >>> +#include 
> >>> +#include 
> >>> +
> >>> +extern struct pglist_data *node_data[];
> >>> +
> >>> +#define NODE_DATA(nid)   (node_data[(nid)])
> >>
> >> This is the same as m32r, metag, parisc, powerpc, s390, sh, sparc, tile
> >> and x86. Can we make this the default in the core code instead and then
> >> replace this header file with asm-generic or something?
> > IIUC, it is same in most but not in all arch.

Yes, so we sho

Re: [RESEND PATCH v7 2/4] Documentation, dt, arm64/arm: dt bindings for numa.

[Will Deacon]

Mark,

On Fri, Dec 18, 2015 at 06:03:47PM +, Mark Rutland wrote:
> On Fri, Dec 18, 2015 at 09:00:18PM +0530, Ganapatrao Kulkarni wrote:
> > Hi Mark,
> > 
> > On Fri, Dec 18, 2015 at 7:48 PM, Mark Rutland  wrote:
> > >> +- distance-matrix
> > >> +  This property defines a matrix to describe the relative distances
> > >> +  between all numa nodes.
> > >> +  It is represented as a list of node pairs and their relative distance.
> > >> +
> > >> +  Note:
> > >> + 1. Each entry represents distance from first node to second node.
> > >> + The distance are equal in either direction.
> > >> + 2. The distance from a node to self(local distance) is represented
> > >> + with value 10 and all inter node distance should be represented 
> > >> with
> > >> + value greater than 10.
> > >> + 3. distance-matrix shold have entries in lexicographical ascending
> > >> + order of nodes.
> > >> + 4. There must be only one Device node distance-map and must reside 
> > >> in the root node.
> > >
> > > I am still concerned that the local distance of 10 is completely
> > > arbitrary.
> > IMHO, i do not see any issue in having defined local distance to
> > arbitrary number(10).
> > inter node numa distance is relative number with respect to local distance
> > we have to fix local distance to some value, having it in dt to make
> > generic will not add
> > any additional value as compared to having the fixed local distance to 10.
> 
> That's not quite true. The figure chosen for the local distance affects
> the granularity with which you can describe all distances.
> 
> By using a local distance of 10 we can only encode distances in 10%
> chunks of that. Using a local distance of 100 we could encode in 1%
> chunks of that.

Whilst I see what you're saying, the local distance of 10 seems to be
part of the ACPI spec, and is the reason why the core code defines it
that way.

Now, we can of course do our own thing for device-tree, but I really
don't think it's worth our while to change this without a compelling
use-case.

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

Re: [PATCH v7 1/4] arm64, numa: adding numa support for arm64 platforms.

[Will Deacon]

Hello,

This all looks pretty reasonable, but I'd like to see an Ack from a
devicetree maintainer on the binding before I merge anything (and I see
that there are outstanding comments from Rutland on that).

On Tue, Nov 17, 2015 at 10:50:40PM +0530, Ganapatrao Kulkarni wrote:
> Adding numa support for arm64 based platforms.
> This patch adds by default the dummy numa node and
> maps all memory and cpus to node 0.
> using this patch, numa can be simulated on single node arm64 platforms.
> 
> Reviewed-by: Robert Richter 
> Signed-off-by: Ganapatrao Kulkarni 
> ---
>  arch/arm64/Kconfig  |  25 +++
>  arch/arm64/include/asm/mmzone.h |  17 ++
>  arch/arm64/include/asm/numa.h   |  47 +
>  arch/arm64/kernel/setup.c   |   4 +
>  arch/arm64/kernel/smp.c |   2 +
>  arch/arm64/mm/Makefile  |   1 +
>  arch/arm64/mm/init.c|  30 +++-
>  arch/arm64/mm/numa.c| 384 
> 
>  8 files changed, 506 insertions(+), 4 deletions(-)
>  create mode 100644 arch/arm64/include/asm/mmzone.h
>  create mode 100644 arch/arm64/include/asm/numa.h
>  create mode 100644 arch/arm64/mm/numa.c
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 9ac16a4..7d8fb42 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -71,6 +71,7 @@ config ARM64
>   select HAVE_GENERIC_DMA_COHERENT
>   select HAVE_HW_BREAKPOINT if PERF_EVENTS
>   select HAVE_MEMBLOCK
> + select HAVE_MEMBLOCK_NODE_MAP if NUMA
>   select HAVE_PATA_PLATFORM
>   select HAVE_PERF_EVENTS
>   select HAVE_PERF_REGS
> @@ -482,6 +483,30 @@ config HOTPLUG_CPU
> Say Y here to experiment with turning CPUs off and on.  CPUs
> can be controlled through /sys/devices/system/cpu.
>  
> +# Common NUMA Features
> +config NUMA
> + bool "Numa Memory Allocation and Scheduler Support"
> + depends on SMP
> + help
> +   Enable NUMA (Non Uniform Memory Access) support.
> +
> +   The kernel will try to allocate memory used by a CPU on the
> +   local memory controller of the CPU and add some more
> +   NUMA awareness to the kernel.

I appreciate that this is copied from x86, but what exactly do you mean
by "memory controller" here?

> diff --git a/arch/arm64/include/asm/mmzone.h b/arch/arm64/include/asm/mmzone.h
> new file mode 100644
> index 000..6ddd468
> --- /dev/null
> +++ b/arch/arm64/include/asm/mmzone.h
> @@ -0,0 +1,17 @@
> +#ifndef __ASM_ARM64_MMZONE_H_
> +#define __ASM_ARM64_MMZONE_H_

Please try to follow the standard naming for header guards under arm64
(yes, it's not perfect, but we've made some effort for consistency).

> +
> +#ifdef CONFIG_NUMA
> +
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +
> +extern struct pglist_data *node_data[];
> +
> +#define NODE_DATA(nid)   (node_data[(nid)])

This is the same as m32r, metag, parisc, powerpc, s390, sh, sparc, tile
and x86. Can we make this the default in the core code instead and then
replace this header file with asm-generic or something?

> +
> +#endif /* CONFIG_NUMA */
> +#endif /* __ASM_ARM64_MMZONE_H_ */
> diff --git a/arch/arm64/include/asm/numa.h b/arch/arm64/include/asm/numa.h
> new file mode 100644
> index 000..c00f3a4
> --- /dev/null
> +++ b/arch/arm64/include/asm/numa.h
> @@ -0,0 +1,47 @@
> +#ifndef _ASM_NUMA_H
> +#define _ASM_NUMA_H

Same comment on the guards.

> +#include 
> +#include 
> +
> +#ifdef CONFIG_NUMA
> +
> +#define NR_NODE_MEMBLKS  (MAX_NUMNODES * 2)

This is only used by the ACPI code afaict, so maybe include it when you
add that?

> +#define ZONE_ALIGN (1UL << (MAX_ORDER + PAGE_SHIFT))

Where is this used?

> +
> +/* currently, arm64 implements flat NUMA topology */
> +#define parent_node(node)(node)
> +
> +extern int __node_distance(int from, int to);
> +#define node_distance(a, b) __node_distance(a, b)
> +
> +/* dummy definitions for pci functions */
> +#define pcibus_to_node(node) 0
> +#define cpumask_of_pcibus(bus)   0

There's a bunch of these dummy definitions already available in
asm-generic/topology.h. Can we use those instead of rolling our own
please?

> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> index 17bf39a..8dc9c5d 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -37,6 +37,7 @@
>  
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -77,6 +78,19 @@ static phys_addr_t max_zone_dma_phys(void)
>   return min(offset + (1ULL << 32), memblock_end_of_DRAM());
>  }
>  
> +#ifdef CONFIG_NUMA
> +static void __init zone_sizes_init(unsigned long min, unsigned long max)
> +{
> + unsigned long max_zone_pfns[MAX_NR_ZONES]  = {0};
> +
> + if (IS_ENABLED(CONFIG_ZONE_DMA))
> + max_zone_pfns[ZONE_DMA] = PFN_DOWN(max_zone_dma_phys());
> + max_zone_pfns[ZONE_NORMAL] = max;
> +
> + free_area_init_nodes(max_zone_pfns);
> +}

This is certainly more readable then the non-numa zone_sizes_init. Is

Re: [PATCH] arm64: perf: Correct Cortex-A53/A57 compatible values

[Will Deacon]

On Tue, Dec 15, 2015 at 10:42:42AM +, Will Deacon wrote:
> On Tue, Dec 15, 2015 at 10:37:00AM +, Mark Rutland wrote:
> > On Tue, Dec 15, 2015 at 09:33:20AM +0100, Geert Uytterhoeven wrote:
> > > Use commas instead of periods.
> > > 
> > > Signed-off-by: Geert Uytterhoeven 
> > 
> > Acked-by: Mark Rutland 
> 
> Cheers, Mark.
> 
> Rob -- do you want to pick this up, or shall I queue it on my perf
> branch?

Since it's so trivial and also has Mark's ack, I've queued this in the
ARM perf tree.

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

Re: [PATCH v2 2/2] Documentation: dt-bindings: Add option to workaround STE.VALID in Broadcom Vulcan

[Will Deacon]

[adding devicetree since I'd like an Ack from them if possible]

On Thu, Dec 17, 2015 at 03:29:36PM +0530, Prem Mallappa wrote:
> Signed-off-by: Prem Mallappa 

Please can you add a commit message for this?

> ---
>  Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt 
> b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
> index 947863a..9c1218e 100644
> --- a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
> @@ -43,6 +43,11 @@ the PCIe specification.
>  - hisilicon,broken-prefetch-cmd
>  : Avoid sending CMD_PREFETCH_* commands to the SMMU.
>  
> +- broadcom,broken-ste-valid-check
> +: Broadcom specific, h/w peeks into more than just 
> +   STE.VALID bit in "Bypass" mode. Enabling this, 
> +   populates needed bits.

This should really describe more about what happens, I think. For example,

"Enable ATS and Stage-2 AArch64 translation in bypass STEs, since some
 hardware requires this in order for an STE to be treated as valid."

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

Re: [PATCH] arm64: perf: Correct Cortex-A53/A57 compatible values

[Will Deacon]

On Tue, Dec 15, 2015 at 10:37:00AM +, Mark Rutland wrote:
> On Tue, Dec 15, 2015 at 09:33:20AM +0100, Geert Uytterhoeven wrote:
> > Use commas instead of periods.
> > 
> > Signed-off-by: Geert Uytterhoeven 
> 
> Acked-by: Mark Rutland 

Cheers, Mark.

Rob -- do you want to pick this up, or shall I queue it on my perf
branch?

Will

> Mark.
> 
> > ---
> >  Documentation/devicetree/bindings/arm/pmu.txt | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/pmu.txt 
> > b/Documentation/devicetree/bindings/arm/pmu.txt
> > index 97ba45af04fc693f..a6cd14888bed9b29 100644
> > --- a/Documentation/devicetree/bindings/arm/pmu.txt
> > +++ b/Documentation/devicetree/bindings/arm/pmu.txt
> > @@ -9,8 +9,8 @@ Required properties:
> >  - compatible : should be one of
> > "apm,potenza-pmu"
> > "arm,armv8-pmuv3"
> > -   "arm.cortex-a57-pmu"
> > -   "arm.cortex-a53-pmu"
> > +   "arm,cortex-a57-pmu"
> > +   "arm,cortex-a53-pmu"
> > "arm,cortex-a17-pmu"
> > "arm,cortex-a15-pmu"
> > "arm,cortex-a12-pmu"
> > -- 
> > 1.9.1
> > 
> 
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ARM: l2x0: make it possible to disable outer sync from DT

[Will Deacon]

On Mon, Dec 14, 2015 at 02:30:53PM +0100, Linus Walleij wrote:
> On Thu, Dec 10, 2015 at 3:32 PM, Mark Rutland  wrote:
> > On Thu, Dec 10, 2015 at 03:14:15PM +0100, Linus Walleij wrote:
> >> diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt 
> >> b/Documentation/devicetree/bindings/arm/l2cc.txt
> >> index d181b7c4c522..aae7387acbdb 100644
> >> --- a/Documentation/devicetree/bindings/arm/l2cc.txt
> >> +++ b/Documentation/devicetree/bindings/arm/l2cc.txt
> >> @@ -75,6 +75,7 @@ Optional properties:
> >>specified to indicate that such transforms are precluded.
> >>  - arm,parity-enable : enable parity checking on the L2 cache (L220 or 
> >> PL310).
> >>  - arm,parity-disable : disable parity checking on the L2 cache (L220 or 
> >> PL310).
> >> +- arm,outer-sync-disable : disable the outer sync operation on the L2 
> >> cache.
> >
> > I'm not sure what this means from a HW perspective. The "outer sync"
> > operation is Linux terminology and doesn't show up in the TRM for L220.
> >
> > What is the (integration? HW?) bug that we're trying to avoid the effect
> > of? It sounds like we should be describing that.
> >
> > We should at least have a better definition of what this means we must
> > avoid.
> 
> So this code is a bit ancient, and we're trying to migrate it to device tree.
> 
> It was inspired by this board file patch from Arnd:
> http://marc.info/?l=linux-arm-kernel&m=144846938616893&w=2
> 
> Then I go back and dig in the code and I find this:
> 
> commit 2503a5ecd86c002506001eba432c524ea009fe7f
> Author: Catalin Marinas 
> Date:   Thu Jul 1 13:21:47 2010 +0100
> 
> ARM: 6201/1: RealView: Do not use outer_sync() on ARM11MPCore
> boards with L220
> 
> RealView boards with certain revisions of the L220 cache controller 
> (ARM11*
> processors only) may have issues (hardware deadlock) with the
> recent changes to
> the mb() barrier implementation (DSB followed by an L2 cache
> sync). The patch
> redefines the RealView ARM11MPCore mandatory barriers without the
> outer_sync()
> call.
> 
> Cc: 
> Tested-by: Linus Walleij 
> Signed-off-by: Catalin Marinas 
> Signed-off-by: Russell King 
> 
> And that is as much as it says.
> 
> It is working around a hardware lockup in some core tiles. Is it
> enough if I add this
> to the explanation or do we need to send Catalin or Will down in the archives
> to read netlists? ;)

/me delegates to Rutland.

Mark -- still want a better definition? ;)

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

Re: [PATCH v5 3/6] iommu: add ARM short descriptor page table allocator

[Will Deacon]

On Fri, Nov 06, 2015 at 04:42:52PM +0800, Yong Wu wrote:
> On Fri, 2015-10-09 at 10:23 +0800, Yong Wu wrote:
> > This patch is for ARM Short Descriptor Format.
> > 
> > Signed-off-by: Yong Wu 
> > ---
> 
> Hi Will, Robin,
>Is there any comment about this patch?
>As our project request, We are going to prepare the next version.

Robin has been looking at the code after he ran into problems using it
in conjunction with arm-smmu. I'd expect him to post something after the
merge window, so you may as well hold-off until then.

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

Re: [PATCH v6 1/6] arm/arm64: add smccc

[Will Deacon]

On Mon, Nov 02, 2015 at 02:03:13PM +, Mark Rutland wrote:
> > > > +/**
> > > > + * struct smccc_res - Result from SMC/HVC call
> > > > + * @a0-a3 result values from registers 0 to 3
> > > > + */
> > > > +struct smccc_res {
> > > > +   unsigned long a0;
> > > > +   unsigned long a1;
> > > > +   unsigned long a2;
> > > > +   unsigned long a3;
> > > > +};
> > > 
> > > Are there any endianness considerations for this structure?
> > 
> > No, I can't find anything in the ARM SMC Calling Convention document
> > indicating that.
> 
> The calling conventions have no bearing on the structure, which is our
> invention.
> 
> We stash register values (which don't have endianness) returned by the
> firmeware into the structure in the sequences above (which appear
> endian-clean to me).

Ah yes, the structure is populated by the kernel not the firmware, so
there's nothing to worry about. Thanks for the explanation.

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

Re: [PATCH v6 2/6] drivers: psci: replace psci firmware calls

[Will Deacon]

On Mon, Nov 02, 2015 at 02:08:26PM +0100, Jens Wiklander wrote:
> On Mon, Nov 02, 2015 at 11:55:39AM +0000, Will Deacon wrote:
> > On Thu, Oct 29, 2015 at 09:21:24AM +0100, Jens Wiklander wrote:
> > > Switch to use a generic interface for issuing SMC/HVC based on ARM SMC
> > > Calling Convention. Removes now the now unused psci-call.S.
> > > 
> > > Signed-off-by: Jens Wiklander 
> > > ---
> > >  arch/arm/kernel/Makefile  |  1 -
> > >  arch/arm/kernel/psci-call.S   | 31 ---
> > >  arch/arm64/kernel/Makefile|  2 +-
> > >  arch/arm64/kernel/psci-call.S | 28 
> > >  drivers/firmware/psci.c   | 21 +++--
> > >  5 files changed, 20 insertions(+), 63 deletions(-)
> > >  delete mode 100644 arch/arm/kernel/psci-call.S
> > >  delete mode 100644 arch/arm64/kernel/psci-call.S
> > 
> > [...]
> > 
> > > diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
> > > index 42700f0..53c9606 100644
> > > --- a/drivers/firmware/psci.c
> > > +++ b/drivers/firmware/psci.c
> > > @@ -19,6 +19,7 @@
> > >  #include 
> > >  #include 
> > >  #include 
> > > +#include 
> > >  #include 
> > >  
> > >  #include 
> > > @@ -56,8 +57,6 @@ struct psci_operations psci_ops;
> > >  
> > >  typedef unsigned long (psci_fn)(unsigned long, unsigned long,
> > >   unsigned long, unsigned long);
> > > -asmlinkage psci_fn __invoke_psci_fn_hvc;
> > > -asmlinkage psci_fn __invoke_psci_fn_smc;
> > >  static psci_fn *invoke_psci_fn;
> > >  
> > >  enum psci_function {
> > > @@ -70,6 +69,24 @@ enum psci_function {
> > >  
> > >  static u32 psci_function_id[PSCI_FN_MAX];
> > >  
> > > +static unsigned long __invoke_psci_fn_hvc(unsigned long a0, unsigned 
> > > long a1,
> > > + unsigned long a2, unsigned long a3)
> > 
> > Minor comment, but could we keep these argument names the same as before
> > please?
> 
> You mean function_id, arg0, arg1... instead? I guess I should update
> arm-smccc.h also then.

No; I think just updating the psci caller to use those, but leave the
SMCCC low-level details as they are. The latter is just pushing data,
whilst the former has some (albeit limited) knowledge of what those
values represent.

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

Re: [PATCH v6 2/6] drivers: psci: replace psci firmware calls

[Will Deacon]

On Thu, Oct 29, 2015 at 09:21:24AM +0100, Jens Wiklander wrote:
> Switch to use a generic interface for issuing SMC/HVC based on ARM SMC
> Calling Convention. Removes now the now unused psci-call.S.
> 
> Signed-off-by: Jens Wiklander 
> ---
>  arch/arm/kernel/Makefile  |  1 -
>  arch/arm/kernel/psci-call.S   | 31 ---
>  arch/arm64/kernel/Makefile|  2 +-
>  arch/arm64/kernel/psci-call.S | 28 
>  drivers/firmware/psci.c   | 21 +++--
>  5 files changed, 20 insertions(+), 63 deletions(-)
>  delete mode 100644 arch/arm/kernel/psci-call.S
>  delete mode 100644 arch/arm64/kernel/psci-call.S

[...]

> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
> index 42700f0..53c9606 100644
> --- a/drivers/firmware/psci.c
> +++ b/drivers/firmware/psci.c
> @@ -19,6 +19,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  #include 
> @@ -56,8 +57,6 @@ struct psci_operations psci_ops;
>  
>  typedef unsigned long (psci_fn)(unsigned long, unsigned long,
>   unsigned long, unsigned long);
> -asmlinkage psci_fn __invoke_psci_fn_hvc;
> -asmlinkage psci_fn __invoke_psci_fn_smc;
>  static psci_fn *invoke_psci_fn;
>  
>  enum psci_function {
> @@ -70,6 +69,24 @@ enum psci_function {
>  
>  static u32 psci_function_id[PSCI_FN_MAX];
>  
> +static unsigned long __invoke_psci_fn_hvc(unsigned long a0, unsigned long a1,
> + unsigned long a2, unsigned long a3)

Minor comment, but could we keep these argument names the same as before
please?

> +{
> + struct smccc_res res;
> +
> + smccc_hvc(a0, a1, a2, a3, 0, 0, 0, 0, &res);

It's slightly tempting to use varargs instead of the '0' argument padding,
but that will probably make the asm code unmanageable.

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

Re: [PATCH v6 1/6] arm/arm64: add smccc

[Will Deacon]

Hi Jens,

On Thu, Oct 29, 2015 at 09:21:23AM +0100, Jens Wiklander wrote:
> Adds helpers to do SMC and HVC based on ARM SMC Calling Convention.
> CONFIG_HAVE_SMCCC is enabled for architectures that may support
> the SMC or HVC instruction. It's the responsibility of the caller
> to know if the SMC instruction is supported by the platform.
> 
> Signed-off-by: Jens Wiklander 
> ---
>  arch/arm/Kconfig   |  4 ++
>  arch/arm/kernel/Makefile   |  2 +
>  arch/arm/kernel/smccc-call.S   | 49 +
>  arch/arm/kernel/smccc.c| 18 
>  arch/arm64/Kconfig |  4 ++
>  arch/arm64/kernel/Makefile |  1 +
>  arch/arm64/kernel/smccc-call.S | 43 ++
>  arch/arm64/kernel/smccc.c  | 18 
>  include/linux/arm-smccc.h  | 98 
> ++

This should probably be split so that the arm and arm64 patches can be
merged separately.

>  9 files changed, 237 insertions(+)
>  create mode 100644 arch/arm/kernel/smccc-call.S
>  create mode 100644 arch/arm/kernel/smccc.c
>  create mode 100644 arch/arm64/kernel/smccc-call.S
>  create mode 100644 arch/arm64/kernel/smccc.c
>  create mode 100644 include/linux/arm-smccc.h

[...]

> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 72ad724..29ab16a 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -226,6 +226,9 @@ config NEED_RET_TO_USER
>  config ARCH_MTD_XIP
>   bool
>  
> +config HAVE_SMCCC
> + bool

If you want this to be selectable by multiple arches, wouldn't it be
better to define it out in a common Kconfig file? Maybe HAVE_ARM_SMCCC
too, for some better namespacing.

An alternative is just making your TEE driver depend on ARM || ARM64
and providing dummy smc wrappers that return an error code for cores
older than ARMv7.

> diff --git a/arch/arm64/kernel/smccc-call.S b/arch/arm64/kernel/smccc-call.S
> new file mode 100644
> index 000..9098bd8
> --- /dev/null
> +++ b/arch/arm64/kernel/smccc-call.S
> @@ -0,0 +1,43 @@
> +/*
> + * Copyright (c) 2015, Linaro Limited
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License Version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +#include 
> +
> +#define SMC_RES_X0_OFFS  0
> +#define SMC_RES_X2_OFFS  16

These should be generates in asm-offsets.c

> +
> +/*
> + * void smccc_smc(unsigned long a0, unsigned long a1, unsigned long a2,
> + * unsigned long a3, unsigned long a4, unsigned long a5,
> + * unsigned long a6, unsigned long a7, struct smccc_res *res)
> + */
> +ENTRY(smccc_smc)
> + smc #0
> + ldr x4, [sp]
> + stp x0, x1, [x4, #SMC_RES_X0_OFFS]
> + stp x2, x3, [x4, #SMC_RES_X2_OFFS]
> + ret
> +ENDPROC(smccc_smc)
> +
> +/*
> + * void smccc_hvc(unsigned long a0, unsigned long a1, unsigned long a2,
> + * unsigned long a3, unsigned long a4, unsigned long a5,
> + * unsigned long a6, unsigned long a7, struct smccc_res *res)
> + */
> +ENTRY(smccc_hvc)
> + hvc #0
> + ldr x4, [sp]
> + stp x0, x1, [x4, #SMC_RES_X0_OFFS]
> + stp x2, x3, [x4, #SMC_RES_X2_OFFS]
> + ret
> +ENDPROC(smccc_hvc)

Maybe you could generate both of these from an asm macro that takes the
first instruction as a parameter?

> diff --git a/arch/arm64/kernel/smccc.c b/arch/arm64/kernel/smccc.c
> new file mode 100644
> index 000..ed13ba3
> --- /dev/null
> +++ b/arch/arm64/kernel/smccc.c
> @@ -0,0 +1,18 @@
> +/*
> + * Copyright (c) 2015, Linaro Limited
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +#include 
> +#include 
> +
> +EXPORT_SYMBOL_GPL(smccc_smc);
> +EXPORT_SYMBOL_GPL(smccc_hvc);

Again, I think you want an arm_ prefix for some better namespacing.

> diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h

[...]

> +#define SMCCC_SMC_32 (0 << 30)
> +#define SMCCC_SMC_64 (1 << 30)
> +#define SMCCC_FAST_CALL  (1 << 31)
> +#define SMCCC_STD_CALL   (0 << 31)
> +
> +#define SMCCC_OWNER_MASK 0x3F
> +#define SMCCC_OWNER_SHIFT24
> +
> +#define SMCCC_FUNC_MASK  0x
> +
> +#

Re: [PATCH v5 3/6] iommu: add ARM short descriptor page table allocator

[Will Deacon]

On Wed, Oct 14, 2015 at 02:54:19PM +0200, Joerg Roedel wrote:
> On Fri, Oct 09, 2015 at 10:23:05AM +0800, Yong Wu wrote:
> > This patch is for ARM Short Descriptor Format.
> > 
> > Signed-off-by: Yong Wu 
> 
> I think it would be good if Will Deacon could have a look on that.
> 
> Will, any comments on this patch?

Unfortunately, I haven't found time to take a good look at this version
(this patch usually takes the best part of a day to review properly)
and I'm away on holiday all next week.

I can review it when I'm back and have got back on top of email, but
that probably doesn't work well with 4.4, unfortunately. Robin and/or
Catalin may be able to review it in my absence, but it's hard work...

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

Re: [PATCH v4 3/6] iommu: add ARM short descriptor page table allocator.

[Will Deacon]

On Fri, Oct 09, 2015 at 06:41:51PM +0100, Robin Murphy wrote:
> On 09/10/15 16:57, Will Deacon wrote:
> >On Tue, Sep 22, 2015 at 03:12:47PM +0100, Yong Wu wrote:
> >>  I would like to show you a problem I met, The recursion here may
> >>lead to stack overflow while we test FHD video decode.
> >>
> >> From the log, I get the internal variable in the error case: the
> >>"size" is 0x10, the "iova" is 0xfea0, but at that time the
> >>"blk_size" is 0x1000 as it was the map of small-page. so it enter the
> >>recursion here.
> >>
> >> After check the unmap flow, there is only a iommu_unmap in
> >>__iommu_dma_unmap, and it won't check the physical address align in
> >>iommu_unmap.
> >
> >That sounds like a bug in __iommu_dma_unmap. Robin?
> 
> Isn't it just cf27ec930be9 again wearing different trousers? All I do is
> call iommu_unmap with the same total size that was mapped originally.

I don't think it's the same as that issue, which was to do with installing
block mappings over the top of an existing table entry. The problem here
seems to be that we don't walk the page table properly on unmap.

The long descriptor code has:

/* If the size matches this level, we're in the right place */
if (size == blk_size) {
__arm_lpae_set_pte(ptep, 0, &data->iop.cfg);

if (!iopte_leaf(pte, lvl)) {
/* Also flush any partial walks */
tlb->tlb_add_flush(iova, size, false, cookie);
tlb->tlb_sync(cookie);
ptep = iopte_deref(pte, data);
__arm_lpae_free_pgtable(data, lvl + 1, ptep);
} else {
tlb->tlb_add_flush(iova, size, true, cookie);
}

return size;
} else if (iopte_leaf(pte, lvl)) {
/*
 * Insert a table at the next level to map the old region,
 * minus the part we want to unmap
 */
return arm_lpae_split_blk_unmap(data, iova, size,
iopte_prot(pte), lvl, ptep,
blk_size);
}

why doesn't something similar work for short descriptors?

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

Re: [PATCH v4 3/6] iommu: add ARM short descriptor page table allocator.

[Will Deacon]

On Tue, Sep 22, 2015 at 03:12:47PM +0100, Yong Wu wrote:
>  I would like to show you a problem I met, The recursion here may
> lead to stack overflow while we test FHD video decode.
> 
> From the log, I get the internal variable in the error case: the
> "size" is 0x10, the "iova" is 0xfea0, but at that time the
> "blk_size" is 0x1000 as it was the map of small-page. so it enter the
> recursion here.
> 
> After check the unmap flow, there is only a iommu_unmap in
> __iommu_dma_unmap, and it won't check the physical address align in
> iommu_unmap.

That sounds like a bug in __iommu_dma_unmap. Robin?

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

Re: [PATCH 3/4] arm64: Juno: Add support for the PCIe host bridge on Juno R1

[Will Deacon]

On Fri, Oct 09, 2015 at 03:11:07PM +0100, Liviu Dudau wrote:
> On Fri, Oct 09, 2015 at 08:54:33AM -0500, Rob Herring wrote:
> > On Fri, Oct 9, 2015 at 8:45 AM, Liviu Dudau  wrote:
> > > Juno R1 board sports a functional PCIe host bridge that is
> > > compliant with the SBSA standard found here[1]. With the right
> > > firmware that initialises the XpressRICH3 controller one can
> > > use the generic Host Bridge driver to use the PCIe hardware.
> > >
> > > Signed-off-by: Liviu Dudau 
> > >
> > > [1] http://infocenter.arm.com/help/topic/com.arm.doc.den0029a/
> > > ---
> > >  arch/arm64/boot/dts/arm/juno-r1.dts | 20 
> > >  1 file changed, 20 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/arm/juno-r1.dts 
> > > b/arch/arm64/boot/dts/arm/juno-r1.dts
> > > index c627511..a702a6b 100644
> > > --- a/arch/arm64/boot/dts/arm/juno-r1.dts
> > > +++ b/arch/arm64/boot/dts/arm/juno-r1.dts
> > > @@ -109,6 +109,26 @@
> > >
> > > #include "juno-base.dtsi"
> > >
> > > +   pcie-controller@4000 {
> > > +   compatible = "pci-host-ecam-generic";
> > 
> > I think this is the first case of real h/w using this. We should have
> > a specific compatible here additionally.
> 
> Or maybe I can claim the use of the string on account on being the first
> on arm64 ;)

This is already being used on arm64 with kvmtool, qemu, the ARM fastmodel
and AMD Seattle.

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

Re: [PATCH v2 4/5] PCI: generic: Correct, and avoid overflow, in bus_max calculation.

[Will Deacon]

On Wed, Sep 23, 2015 at 08:39:27PM +0100, Arnd Bergmann wrote:
> On Wednesday 23 September 2015 20:35:45 Will Deacon wrote:
> > On Wed, Sep 23, 2015 at 08:27:41PM +0100, Arnd Bergmann wrote:
> > > On Wednesday 23 September 2015 11:21:56 David Daney wrote:
> > > > >>
> > > > >>  /* Limit the bus-range to fit within reg */
> > > > >> -bus_max = pci->cfg.bus_range->start +
> > > > >> -  (resource_size(&pci->cfg.res) >> 
> > > > >> pci->cfg.ops.bus_shift) - 1;
> > > > >> +bus_max = (resource_size(&pci->cfg.res) >> 
> > > > >> pci->cfg.ops.bus_shift) - 1;
> > > > >> +if (bus_max > 255)
> > > > >> +bus_max = 255;
> > > > >
> > > > > I still don't understand the need for this part. If the cfg space is 
> > > > > bigger
> > > > > than bus_max, isn't that simply an invalid resource? Given that the 
> > > > > resource
> > > > > could be broken in other ways too, this check feels more like a 
> > > > > specific
> > > > > workaround rather than generally useful code.
> > > > 
> > > > Imagine...
> > > > 
> > > >bus-range [0x80 .. 0xff], this requires a cfg.res that will cover 
> > > > the 
> > > > entire range of 0..0xff.
> > > > 
> > > >according to the calculations above, (resource_size(&pci->cfg.res) 
> > > > >> 
> > > > pci->cfg.ops.bus_shift) - 1 will have a value of 0xff, so...
> > > 
> > > Extending the computation to 32 bit seems fine, but I'd rather warn loudly
> > > if the bus range does not fit within the registers.
> > > 
> > > Also note that the computation is already correct with my interpretation
> > > of the reg property.
> > 
> > From what Lorenzo was saying, ACPI shares the interpretation that David is
> > implementing here and, given that the DT version seems to be subjective,
> > aligning this reg property with MMCFG seems to make sense.
> 
> We should then make it very clear in the binding that the driver
> is not allowed to actually map the registers for the buses outside
> of the bus-range, as that is highly unusual.
> 
> We would also need a special exception for this if we ever get to
> implement the DT source checker that we have been talking about for
> years, as the reg property might then overlap with a property from
> another device.

Completely agreed. Having a base that isn't actually safe to map is horrible
and should be called out.

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

Re: [PATCH v2 4/5] PCI: generic: Correct, and avoid overflow, in bus_max calculation.

[Will Deacon]

On Wed, Sep 23, 2015 at 08:27:41PM +0100, Arnd Bergmann wrote:
> On Wednesday 23 September 2015 11:21:56 David Daney wrote:
> > >>
> > >>  /* Limit the bus-range to fit within reg */
> > >> -bus_max = pci->cfg.bus_range->start +
> > >> -  (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) 
> > >> - 1;
> > >> +bus_max = (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) 
> > >> - 1;
> > >> +if (bus_max > 255)
> > >> +bus_max = 255;
> > >
> > > I still don't understand the need for this part. If the cfg space is 
> > > bigger
> > > than bus_max, isn't that simply an invalid resource? Given that the 
> > > resource
> > > could be broken in other ways too, this check feels more like a specific
> > > workaround rather than generally useful code.
> > 
> > Imagine...
> > 
> >bus-range [0x80 .. 0xff], this requires a cfg.res that will cover the 
> > entire range of 0..0xff.
> > 
> >according to the calculations above, (resource_size(&pci->cfg.res) >> 
> > pci->cfg.ops.bus_shift) - 1 will have a value of 0xff, so...
> 
> Extending the computation to 32 bit seems fine, but I'd rather warn loudly
> if the bus range does not fit within the registers.
> 
> Also note that the computation is already correct with my interpretation
> of the reg property.

>From what Lorenzo was saying, ACPI shares the interpretation that David is
implementing here and, given that the DT version seems to be subjective,
aligning this reg property with MMCFG seems to make sense.

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

Re: [PATCH v2 4/5] PCI: generic: Correct, and avoid overflow, in bus_max calculation.

[Will Deacon]

On Wed, Sep 23, 2015 at 07:21:56PM +0100, David Daney wrote:
> On 09/23/2015 11:01 AM, Will Deacon wrote:
> > On Thu, Sep 17, 2015 at 11:02:11PM +0100, David Daney wrote:
> [...]
> >
> >>   Properties of the /chosen node:
> >> diff --git a/drivers/pci/host/pci-host-generic.c 
> >> b/drivers/pci/host/pci-host-generic.c
> >> index 77cf4bd..0a9c453 100644
> >> --- a/drivers/pci/host/pci-host-generic.c
> >> +++ b/drivers/pci/host/pci-host-generic.c
> >> @@ -164,7 +164,7 @@ out_release_res:
> >>   static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
> >>   {
> >>int err;
> >> -  u8 bus_max;
> >> +  int bus_max;
> >>resource_size_t busn;
> >>struct resource *bus_range;
> >>struct device *dev = pci->host.dev.parent;
> >> @@ -177,8 +177,9 @@ static int gen_pci_parse_map_cfg_windows(struct 
> >> gen_pci *pci)
> >>}
> >>
> >>/* Limit the bus-range to fit within reg */
> >> -  bus_max = pci->cfg.bus_range->start +
> >> -(resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1;
> >> +  bus_max = (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1;
> >> +  if (bus_max > 255)
> >> +  bus_max = 255;
> >
> > I still don't understand the need for this part. If the cfg space is bigger
> > than bus_max, isn't that simply an invalid resource? Given that the resource
> > could be broken in other ways too, this check feels more like a specific
> > workaround rather than generally useful code.
> 
> Imagine...
> 
>bus-range [0x80 .. 0xff], this requires a cfg.res that will cover the 
> entire range of 0..0xff.
> 
>according to the calculations above, (resource_size(&pci->cfg.res) >> 
> pci->cfg.ops.bus_shift) - 1 will have a value of 0xff, so...
> 
>bus_max = 0x80 + 0xff -> OVERFLOW of u8!
> 
> That is not useful.  bus_max should represent the largest bus number 
> that can be covered by cfg.res.  That is what my patch is attempting to 
> accomplish.  Calculate the largest bus number that can be accommodated 
> by cfg.res, and then clamp it to 0xff.

Sorry, I should've been more specific. The only part I don't like is the
'if (bus_max > 255)' check.

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

Re: [PATCH v2 4/5] PCI: generic: Correct, and avoid overflow, in bus_max calculation.

[Will Deacon]

On Thu, Sep 17, 2015 at 11:02:11PM +0100, David Daney wrote:
> From: David Daney 
> 
> There are two problems with the bus_max calculation:
> 
> 1) The u8 data type can overflow for large config space windows.
> 
> 2) The calculation is incorrect for a bus range that doesn't start at
>zero.
> 
> Since the configuration space is relative to bus zero, make bus_max
> just be the size of the config window scaled by bus_shift.  Then clamp
> it to a maximum of 255, per PCI.  Use a data type of int to avoid
> overflow problems.
> 
> Update host-generic-pci.txt to clarify the semantics of the "reg"
> property with respect to non-zero starting bus numbers.
> 
> Signed-off-by: David Daney 
> ---
> Change from v1: Added text to host-generic-pci.txt
> 
>  Documentation/devicetree/bindings/pci/host-generic-pci.txt | 4 +++-
>  drivers/pci/host/pci-host-generic.c| 7 ---
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/host-generic-pci.txt 
> b/Documentation/devicetree/bindings/pci/host-generic-pci.txt
> index cf3e205..105a968 100644
> --- a/Documentation/devicetree/bindings/pci/host-generic-pci.txt
> +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.txt
> @@ -34,7 +34,9 @@ Properties of the host controller node:
>  - #size-cells: Must be 2.
>  
>  - reg: The Configuration Space base address and size, as accessed
> -   from the parent bus.
> +   from the parent bus.  The base address corresponds to
> +   bus zero, even though the "bus-range" property may specify
> +   a different starting bus number.

That's a useful clarification, thanks.

>  Properties of the /chosen node:
> diff --git a/drivers/pci/host/pci-host-generic.c 
> b/drivers/pci/host/pci-host-generic.c
> index 77cf4bd..0a9c453 100644
> --- a/drivers/pci/host/pci-host-generic.c
> +++ b/drivers/pci/host/pci-host-generic.c
> @@ -164,7 +164,7 @@ out_release_res:
>  static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
>  {
>   int err;
> - u8 bus_max;
> + int bus_max;
>   resource_size_t busn;
>   struct resource *bus_range;
>   struct device *dev = pci->host.dev.parent;
> @@ -177,8 +177,9 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci 
> *pci)
>   }
>  
>   /* Limit the bus-range to fit within reg */
> - bus_max = pci->cfg.bus_range->start +
> -   (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1;
> + bus_max = (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1;
> + if (bus_max > 255)
> + bus_max = 255;

I still don't understand the need for this part. If the cfg space is bigger
than bus_max, isn't that simply an invalid resource? Given that the resource
could be broken in other ways too, this check feels more like a specific
workaround rather than generally useful code.

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

Re: [PATCH v2 2/5] PCI: generic: Only fixup irqs for bus we are creating.

[Will Deacon]

On Thu, Sep 17, 2015 at 11:02:09PM +0100, David Daney wrote:
> From: David Daney 
> 
> If we create multiple buses with pci-host-generic, or there are buses
> created by other drivers, we don't want to call pci_fixup_irqs() which
> operates on all devices, not just the devices on the bus being added.
> The consequence is that either the fixups are done more than once, or
> in some cases incorrect fixups could be applied.
> 
> Call pci_bus_fixup_irqs() instead of pci_fixup_irqs().
> 
> Signed-off-by: David Daney 
> ---
> Changes from v1: Moved most of the code to pci_bus_fixup_irqs(),
> making this patch very simple.

Acked-by: Will Deacon 

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

Re: [PATCH v2 3/3] irqchip/gicv3-its: Handle OF device tree "msi-map" properties.

[Will Deacon]

On Wed, Sep 23, 2015 at 06:08:39PM +0100, David Daney wrote:
> On 09/23/2015 10:01 AM, Marc Zyngier wrote:
> > On Tue, 22 Sep 2015 17:00:06 -0700
> > David Daney  wrote:
> >
> >> From: David Daney 
> >>
> >> Call of_msi_map_rid() to handle mapping of the requester id.
> >>
> >> Signed-off-by: David Daney 
> >> ---
> >>   drivers/irqchip/irq-gic-v3-its-pci-msi.c | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c 
> >> b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> >> index cf351c6..8b1c938 100644
> >> --- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> >> +++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> >> @@ -86,7 +86,8 @@ static int its_pci_msi_prepare(struct irq_domain 
> >> *domain, struct device *dev,
> >>pci_for_each_dma_alias(pdev, its_get_pci_alias, &dev_alias);
> >>
> >>/* ITS specific DeviceID, as the core ITS ignores dev. */
> >> -  info->scratchpad[0].ul = dev_alias.dev_id;
> >> +  info->scratchpad[0].ul = of_msi_map_rid(dev, domain->of_node,
> >> +  dev_alias.dev_id);
> >>
> >>return msi_info->ops->msi_prepare(domain->parent,
> >>  dev, dev_alias.count, info);
> >
> > I really wonder if that shouldn't be part of the pci_for_each_dma_alias
> > call. It would make a lot more sense for this functionality to be an
> > integral part of the core code, and would probably make the integration
> > of _IORT (which has the exact same requirements) a bit easier.
> >
> > Thoughts?
> >
> 
> I am a proponent of pushing things like this as far into the core code 
> as possible.  So, from that point of view, I think it would probably be 
> a good idea.
> 
> I can prepare a patch that does that, but it would also be nice hear 
> from other maintainers and get their thoughts on this.

Hmm, we use pci_for_each_dma_alias in the SMMU drivers to get the SID,
so I'm not sure that using the MSI mapping is necessarily the right thing
to do there. Maybe we should instead have dma_alias_to_msi_id helpers or
something?

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

Re: [PATCH 3/3] PCI: generic: Add support for Cavium ThunderX PCIe root complexes.

[Will Deacon]

On Thu, Sep 17, 2015 at 11:41:34PM +0100, David Daney wrote:
> From: David Daney 
> 
> The config space for external PCIe root complexes on some Cavium
> ThunderX SoCs is very similar to CAM and ECAM, but differs in the
> shift values that have to be applied to the bus and devfn numbers to
> compose that address window offset.  These root complexes also have
> the interesting property that there is no root bridge, so the standard
> manner of limiting scanning to only the first device doesn't work.  We
> can use the standard pci-host-generic driver if we make a minor
> addition to handle these differences, so we...
> 
> Add a mapping function for ThunderX PCIe root complexes with a bus
> shift of 24 and devfn shift of 16.  Ignore accesses for devices other
> than the first device on the primary bus.
> 
> Document the whole thing in devicetree/bindings/pci/host-generic-pci.txt
> 
> Signed-off-by: David Daney 
> ---
>  .../devicetree/bindings/pci/host-generic-pci.txt   |  8 +++---
>  drivers/pci/host/pci-host-generic.c| 29 
> ++
>  2 files changed, 34 insertions(+), 3 deletions(-)

Thanks, this looks better now:

  Acked-by: Will Deacon 

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

Re: [PATCH 4/6] PCI: generic: Correct, and avoid overflow, in bus_max calculation.

[Will Deacon]

Hi Lorenzo,

On Wed, Sep 16, 2015 at 12:28:52PM +0100, Lorenzo Pieralisi wrote:
> On Wed, Sep 16, 2015 at 11:41:53AM +0100, Will Deacon wrote:
> > > Here is the current code:
> > > 
> > > >>bus_range = pci->cfg.bus_range;
> > > >>for (busn = bus_range->start; busn <= bus_range->end; ++busn) {
> > > >>u32 idx = busn - bus_range->start;
> > > 
> > > The index is offset by the bus range start...
> > > 
> > > >>u32 sz = 1 << pci->cfg.ops.bus_shift;
> > > >>
> > > >>pci->cfg.win[idx] = devm_ioremap(dev,
> > > >> pci->cfg.res.start + 
> > > >> busn * sz,
> > > >> sz);
> > > 
> > > But, the offset into the "reg" property is the raw bus number.
> > > 
> > > 
> > > >>if (!pci->cfg.win[idx])
> > > >>return -ENOMEM;
> > > >>}
> > > 
> > > 
> > > I hope that makes it more clear.
> > 
> > Got it. So we should be using idx instead of busn in the devm_ioremap
> > call above.
> 
> I think that's not what's specified in the PCI firmware specification,
> at least for the MMCFG regions. For MMCFG regions (quoting the specs)
> the "base address of the memory mapped configuration space always
> corresponds to bus number 0 (regardless of the start bus number decoded
> by the host bridge)..."
> 
> For the x86 implementation have a look at:
> 
> arch/x86/pci/mmconfig_64.c mcfg_ioremap()
> 
> static void __iomem *mcfg_ioremap(struct pci_mmcfg_region *cfg)
> {
>   void __iomem *addr;
>   u64 start, size;
>   int num_buses;
> 
>   start = cfg->address + PCI_MMCFG_BUS_OFFSET(cfg->start_bus);
>   num_buses = cfg->end_bus - cfg->start_bus + 1;
>   size = PCI_MMCFG_BUS_OFFSET(num_buses);
>   addr = ioremap_nocache(start, size);
>   if (addr)
>   addr -= PCI_MMCFG_BUS_OFFSET(cfg->start_bus);
>   return addr;
> }
> 
> The MCFG config accessors add back the PCI_MMCFG_BUS_OFFSET(cfg->start_bus)
> to the virtual address so that the proper virtual address is used when
> issuing the config cycles, that's my understanding.

Ok. I think that whether the config space mapping or the config accessors
do the fixup should remain an implementation detail, but the resource
identifying config space should be dealt with consistently.

So that means the reg property should describe everything from bus 0,
but then we only map the region corresponding to the bus-range.

> So IMO we have to define what "reg" represents for ECAM in DT, we can't
> leave this open to interpretation (and I think makng MCFG and DT config
> work the same way would be ideal).

If we define reg to cover the whole config space from bus 0 onwards,
then I think the driver should work as-is today. It's slightly odd, in
that there may be a prefix of config space that maps to god-knows-where,
but it's consistent with ACPI and doesn't require us to change the driver.

David?

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

Re: [PATCH v5 1/5] arm/arm64: add smccc ARCH32

[Will Deacon]

On Tue, Sep 15, 2015 at 10:05:59PM +0100, Jens Wiklander wrote:
> On Tue, Sep 15, 2015 at 07:26:45PM +0100, Will Deacon wrote:
> > On Mon, Sep 14, 2015 at 09:30:30AM +0100, Jens Wiklander wrote:
> > > On Fri, Aug 21, 2015 at 01:43:31PM +0200, Jens Wiklander wrote:
> > > > On Fri, Aug 21, 2015 at 10:24:30AM +0100, Will Deacon wrote:
> > > > > On Thu, Aug 20, 2015 at 12:37:29PM +0100, Jens Wiklander wrote:
> > > > > > On Wed, Aug 19, 2015 at 05:50:09PM +0100, Will Deacon wrote:
> > > > > > > On Wed, Aug 19, 2015 at 09:40:25AM +0100, Jens Wiklander wrote:
> > > > > > > > +   mov x28, x0
> > > > > > > > +   ldp w0, w1, [x28, #SMC_PARAM_W0_OFFS]
> > > > > > > > +   ldp w2, w3, [x28, #SMC_PARAM_W2_OFFS]
> > > > > > > > +   ldp w4, w5, [x28, #SMC_PARAM_W4_OFFS]
> > > > > > > > +   ldp w6, w7, [x28, #SMC_PARAM_W6_OFFS]
> > > > > > > > +   smc #0
> > > > > > > > +   stp w0, w1, [x28, #SMC_PARAM_W0_OFFS]
> > > > > > > > +   stp w2, w3, [x28, #SMC_PARAM_W2_OFFS]
> > > > > > > > +   ldp x28, x30, [sp], #16
> > > > > > > > +   ret
> > > > > > > > +ENDPROC(smccc_call32)
> > > > > > > 
> > > > > > > Could we deal with this like we do for PSCI instead? (see
> > > > > > > __invoke_psci_fn_smc). We could also then rename psci-call.S to 
> > > > > > > fw-call.S
> > > > > > > and stick this in there too.
> > > > > > 
> > > > > > I assume you're referring to when to use "hvc" and "smc".
> > > > > 
> > > > > No, I mean use a C prototype to avoid marshalling the parameters in 
> > > > > assembly
> > > > > like this. As Rutland pointed out, the return value is a bit messy, 
> > > > > but
> > > > > the arguments align nicely with the PCS afaict.
> > > > 
> > > > If possible I'd like the function to have the same prototype for both
> > > > arm and arm64. For arm it's not possible to supply more than 4
> > > > parameters. To fully support SMC Calling Convention we need to be able
> > > > to pass 8 parameters and have 4 return values. The OP-TEE driver in this
> > > > patch set depends on this. I don't see how we can avoid the marshalling
> > > > here.
> > > > 
> > > > We could have two versions of the SMCCC functions, one simplified which
> > > > only uses registers and one complete like this one with marshalling.
> > > 
> > > Will, what do think about this?
> > 
> > I still think you should make use of a C prototype to avoid explicit
> > parameter marshalling in assembly. If you want to maintain a compatible
> > API between arm and arm64, then you can easily have an intermediate
> > function in arm64 that sits between the API entry point and the assembly.
> 
> Yes, I see how that's convenient for passing argument values in
> registers, but that doesn't help with storing the returned values in
> x0..x3 into something accessible in C. Am I missing something?

Your approach using a struct looked fine to me, just place it after the
parameters and use it only for the return values. Then __invoke_psci*
can wrap this and unpack r0/x0 from the structure.

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

Re: [PATCH v4 3/6] iommu: add ARM short descriptor page table allocator.

[Will Deacon]

On Mon, Aug 03, 2015 at 11:21:16AM +0100, Yong Wu wrote:
> This patch is for ARM Short Descriptor Format.
> 
> Signed-off-by: Yong Wu 
> ---
>  drivers/iommu/Kconfig|  18 +
>  drivers/iommu/Makefile   |   1 +
>  drivers/iommu/io-pgtable-arm-short.c | 813 
> +++
>  drivers/iommu/io-pgtable-arm.c   |   3 -
>  drivers/iommu/io-pgtable.c   |   4 +
>  drivers/iommu/io-pgtable.h   |  14 +
>  6 files changed, 850 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/iommu/io-pgtable-arm-short.c
> 
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index f1fb1d3..3abd066 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -39,6 +39,24 @@ config IOMMU_IO_PGTABLE_LPAE_SELFTEST
> 
>   If unsure, say N here.
> 
> +config IOMMU_IO_PGTABLE_SHORT
> +   bool "ARMv7/v8 Short Descriptor Format"
> +   select IOMMU_IO_PGTABLE
> +   depends on ARM || ARM64 || COMPILE_TEST
> +   help
> + Enable support for the ARM Short-descriptor pagetable format.
> + This allocator supports 2 levels translation tables which supports

Some minor rewording here:

"...2 levels of translation tables, which enables a 32-bit memory map based
 on..."

> + a memory map based on memory sections or pages.
> +
> +config IOMMU_IO_PGTABLE_SHORT_SELFTEST
> +   bool "Short Descriptor selftests"
> +   depends on IOMMU_IO_PGTABLE_SHORT
> +   help
> + Enable self-tests for Short-descriptor page table allocator.
> + This performs a series of page-table consistency checks during boot.
> +
> + If unsure, say N here.
> +
>  endmenu
> 
>  config IOMMU_IOVA

[...]

> +#define ARM_SHORT_PGDIR_SHIFT  20
> +#define ARM_SHORT_PAGE_SHIFT   12
> +#define ARM_SHORT_PTRS_PER_PTE \
> +   (1 << (ARM_SHORT_PGDIR_SHIFT - ARM_SHORT_PAGE_SHIFT))
> +#define ARM_SHORT_BYTES_PER_PTE\
> +   (ARM_SHORT_PTRS_PER_PTE * sizeof(arm_short_iopte))
> +
> +/* level 1 pagetable */
> +#define ARM_SHORT_PGD_TYPE_PGTABLE BIT(0)
> +#define ARM_SHORT_PGD_TYPE_SECTION BIT(1)
> +#define ARM_SHORT_PGD_BBIT(2)
> +#define ARM_SHORT_PGD_CBIT(3)
> +#define ARM_SHORT_PGD_PGTABLE_NS   BIT(3)
> +#define ARM_SHORT_PGD_SECTION_XN   BIT(4)
> +#define ARM_SHORT_PGD_IMPLEBIT(9)
> +#define ARM_SHORT_PGD_RD_WR(3 << 10)
> +#define ARM_SHORT_PGD_RDONLY   BIT(15)
> +#define ARM_SHORT_PGD_SBIT(16)
> +#define ARM_SHORT_PGD_nG   BIT(17)
> +#define ARM_SHORT_PGD_SUPERSECTION BIT(18)
> +#define ARM_SHORT_PGD_SECTION_NS   BIT(19)
> +
> +#define ARM_SHORT_PGD_TYPE_SUPERSECTION\
> +   (ARM_SHORT_PGD_TYPE_SECTION | ARM_SHORT_PGD_SUPERSECTION)
> +#define ARM_SHORT_PGD_SECTION_TYPE_MSK \
> +   (ARM_SHORT_PGD_TYPE_SECTION | ARM_SHORT_PGD_SUPERSECTION)
> +#define ARM_SHORT_PGD_PGTABLE_TYPE_MSK \
> +   (ARM_SHORT_PGD_TYPE_SECTION | ARM_SHORT_PGD_TYPE_PGTABLE)
> +#define ARM_SHORT_PGD_TYPE_IS_PGTABLE(pgd) \
> +   (((pgd) & ARM_SHORT_PGD_PGTABLE_TYPE_MSK) == 
> ARM_SHORT_PGD_TYPE_PGTABLE)
> +#define ARM_SHORT_PGD_TYPE_IS_SECTION(pgd) \
> +   (((pgd) & ARM_SHORT_PGD_SECTION_TYPE_MSK) == 
> ARM_SHORT_PGD_TYPE_SECTION)
> +#define ARM_SHORT_PGD_TYPE_IS_SUPERSECTION(pgd)\
> +   (((pgd) & ARM_SHORT_PGD_SECTION_TYPE_MSK) == \
> +   ARM_SHORT_PGD_TYPE_SUPERSECTION)
> +#define ARM_SHORT_PGD_PGTABLE_MSK  0xfc00

You could use (~(ARM_SHORT_BYTES_PER_PTE - 1)), I think.

> +#define ARM_SHORT_PGD_SECTION_MSK  (~(SZ_1M - 1))
> +#define ARM_SHORT_PGD_SUPERSECTION_MSK (~(SZ_16M - 1))
> +
> +/* level 2 pagetable */
> +#define ARM_SHORT_PTE_TYPE_LARGE   BIT(0)
> +#define ARM_SHORT_PTE_SMALL_XN BIT(0)
> +#define ARM_SHORT_PTE_TYPE_SMALL   BIT(1)
> +#define ARM_SHORT_PTE_BBIT(2)
> +#define ARM_SHORT_PTE_CBIT(3)
> +#define ARM_SHORT_PTE_RD_WR(3 << 4)
> +#define ARM_SHORT_PTE_RDONLY   BIT(9)
> +#define ARM_SHORT_PTE_SBIT(10)
> +#define ARM_SHORT_PTE_nG   BIT(11)
> +#define ARM_SHORT_PTE_LARGE_XN BIT(15)
> +#define ARM_SHORT_PTE_LARGE_MSK(~(SZ_64K - 1))
> +#define ARM_SHORT_PTE_SMALL_MSK(~(SZ_4K - 1))
> +#define ARM_SHORT_PTE_TYPE_MSK \
> +   (ARM_SHORT_PTE_TYPE_LARGE | ARM_SHORT_PTE_TYPE_SMALL)
> +#define ARM_SHORT_PTE_TYPE_IS_SMALLPAGE(pte)   \
> +   (((pte) & ARM_SHORT_PTE_TYPE_SMALL) == ARM_SHORT_PTE_TYPE_SMALL)

Maybe a comment here, because it's conf

Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.

[Will Deacon]

Hello Yong,

On Mon, Sep 14, 2015 at 01:25:00PM +0100, Yong Wu wrote:
> On Tue, 2015-07-21 at 18:11 +0100, Will Deacon wrote:
> > > +   ret = _arm_short_map(data, iova, paddr, pgdprot, pteprot, large);
> > > +
> > > +   tlb->tlb_add_flush(iova, size, true, data->iop.cookie);
> > > +   tlb->tlb_sync(data->iop.cookie);
> > 
> > In _arm_short_map, it looks like you can only go from invalid -> valid,
> > so why do you need to flush the TLB here?
> 
> Hi Will,
>Here is about flush-tlb after map iova, I have deleted it in v4
> following this suggestion. But We meet a problem about it.

Ok.

> Take a example with JPEG. the test steps is:
> a).JPEG HW decode a picture with the source iova,like 0xfd78.
> b).JPEG HW decode done, It will unmap the iova(write 0 in pagetable and
> flush tlb).
> c).JPEG HW decode the second picture, whose source iova is also
> 0xfd78.
>Then our HW maybe fail due to it will auto prefetch, It may prefecth
> between the step b) and c). then the HW may fetch the pagetable content
> which has been unmapped in step b). then the HW will get the iova's
> physical address is 0, It will translation fault!

Oh no! So-called "negative caching" is certainly prohibited by the ARM
architecture, but if you've built it then we can probably work around it
as an additional quirk. I assume the prefetcher stops prefetching when
it sees an invalid descriptor?

> So I think our HW need flush-tlb after map iova. Could we add a
> QUIRK like "IO_PGTABLE_QUIRK_AUTO_PREFETCH_ENABLE" for it?
> If it's not allowed, we will have to add this in our internal function
> mtk_iommu_map of mtk_iommu.c.

Actually, this type of quirk is ringing bells with me (I think another
IOMMU needed something similar in the past), so maybe just add
IO_PGTABLE_QUIRK_TLBI_ON_MAP?

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

Re: [PATCH 4/6] PCI: generic: Correct, and avoid overflow, in bus_max calculation.

[Will Deacon]

On Tue, Sep 15, 2015 at 07:45:56PM +0100, David Daney wrote:
> On 09/15/2015 11:35 AM, Will Deacon wrote:
> > On Tue, Sep 15, 2015 at 07:02:54PM +0100, David Daney wrote:
> >> On 09/15/2015 10:49 AM, Will Deacon wrote:
> >>> On Sat, Sep 12, 2015 at 12:21:57AM +0100, David Daney wrote:
> >>>>  /* Limit the bus-range to fit within reg */
> >>>> -bus_max = pci->cfg.bus_range->start +
> >>>> -  (resource_size(&pci->cfg.res) >> 
> >>>> pci->cfg.ops.bus_shift) - 1;
> >>>> +bus_max = (resource_size(&pci->cfg.res) >> 
> >>>> pci->cfg.ops.bus_shift) - 1;
> >>>> +if (bus_max > 255)
> >>>> +bus_max = 255;
> >>>>  pci->cfg.bus_range->end = min_t(resource_size_t,
> >>>>  pci->cfg.bus_range->end, 
> >>>> bus_max);
> >>>
> >>> Hmm, this is changing the meaning of the bus-range property in the
> >>> device-tree, which really needs to match what IEEE Std 1275-1994 requires.
> >>
> >> I doesn't change the bus-range.
> >
> > Not directly, but pci->cfg.bus_range is a resource populated from the
> > "bus-range" property in the device-tree, so it's changing how the driver
> > uses that property.
> >
> >>> My understanding was that the bus-range could be used to offset the config
> >>> space, which is why it's subtracted from the bus number in
> >>> gen_pci_map_cfg_bus_[e]cam.
> >>
> >> There is an inconsistency in the current code.  The calculation of the
> >> cfg.win[?] pointers is done such that the beginning of the config space
> >> specified in the "reg" property corresponds to bus 0.
> >
> > I don't follow you here. The mapping functions explicitly subtract the
> > start of the bus range when calculating the window offset:
> >
> >resource_size_t idx = bus->number - pci->cfg.bus_range->start;
> >
> > so if I have bus-range = <128 255>; then bus 128 lives at the start of
> > the configuration space described by the reg property, not bus 0.
> >
> > Sorry if I'm being thick; I just can't see the inconsistency.
> >
> 
> Here is the current code:
> 
> >>bus_range = pci->cfg.bus_range;
> >>for (busn = bus_range->start; busn <= bus_range->end; ++busn) {
> >>u32 idx = busn - bus_range->start;
> 
> The index is offset by the bus range start...
> 
> >>u32 sz = 1 << pci->cfg.ops.bus_shift;
> >>
> >>pci->cfg.win[idx] = devm_ioremap(dev,
> >> pci->cfg.res.start + busn * sz,
> >> sz);
> 
> But, the offset into the "reg" property is the raw bus number.
> 
> 
> >>if (!pci->cfg.win[idx])
> >>return -ENOMEM;
> >>}
> 
> 
> I hope that makes it more clear.

Got it. So we should be using idx instead of busn in the devm_ioremap
call above.

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

Re: [PATCH] PCI: generic: Add support for Cavium ThunderX PCIe root complexes.

[Will Deacon]

On Sat, Sep 12, 2015 at 01:07:19AM +0100, David Daney wrote:
> From: David Daney 
> 
> The config space for external PCIe root complexes on some Cavium
> ThunderX SoCs is very similar to CAM and ECAM, but differs in the
> shift values that have to be applied to the bus and devfn numbers to
> compose that address window offset.  These root complexes also have
> the interesting property that there is no root bridge, so the standard
> manner of limiting scanning to only the first device doesn't work.  We
> can use the standard pci-host-generic driver if we make a minor
> addition to handle these differences, so we...
> 
> Add a mapping function for ThunderX PCIe root complexes with a bus
> shift of 24 and devfn shift of 16.  Ignore accesses for devices other
> than the first device on the primary bus.
> 
> Document the whole thing in devicetree/bindings/pci/host-generic-pci.txt
> 
> Signed-off-by: David Daney 
> ---
> 
> This patch depends on the set I recently sent:
> https://lkml.org/lkml/2015/9/11/764
> 
>  .../devicetree/bindings/pci/host-generic-pci.txt   |  5 +++-
>  drivers/pci/host/pci-host-generic.c| 29 
> ++
>  2 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/host-generic-pci.txt 
> b/Documentation/devicetree/bindings/pci/host-generic-pci.txt
> index daa6942..51cc1d1 100644
> --- a/Documentation/devicetree/bindings/pci/host-generic-pci.txt
> +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.txt
> @@ -16,7 +16,10 @@ Properties of the host controller node:
>  
>  - compatible : Must be "pci-host-cam-generic" or "pci-host-ecam-generic"
> depending on the layout of configuration space (CAM vs
> -   ECAM respectively).
> +   ECAM respectively). Also supported is
> +   "cavium,pci-host-thunder-pem" which has bus:devfn:reg in
> +   bits 24:16:0 respectively of the PCI config space address
> +   window.

It's probably easier to read and maintain if we reword this as a list of
property + comment rather than continue the freeform text.

Other than that, I'm fine with special-casing your accessors if need-be.

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

Re: [PATCH 4/6] PCI: generic: Correct, and avoid overflow, in bus_max calculation.

[Will Deacon]

On Tue, Sep 15, 2015 at 07:02:54PM +0100, David Daney wrote:
> On 09/15/2015 10:49 AM, Will Deacon wrote:
> > On Sat, Sep 12, 2015 at 12:21:57AM +0100, David Daney wrote:
> >>/* Limit the bus-range to fit within reg */
> >> -  bus_max = pci->cfg.bus_range->start +
> >> -(resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1;
> >> +  bus_max = (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1;
> >> +  if (bus_max > 255)
> >> +  bus_max = 255;
> >>pci->cfg.bus_range->end = min_t(resource_size_t,
> >>pci->cfg.bus_range->end, bus_max);
> >
> > Hmm, this is changing the meaning of the bus-range property in the
> > device-tree, which really needs to match what IEEE Std 1275-1994 requires.
> 
> I doesn't change the bus-range.

Not directly, but pci->cfg.bus_range is a resource populated from the
"bus-range" property in the device-tree, so it's changing how the driver
uses that property.

> > My understanding was that the bus-range could be used to offset the config
> > space, which is why it's subtracted from the bus number in
> > gen_pci_map_cfg_bus_[e]cam.
> 
> There is an inconsistency in the current code.  The calculation of the 
> cfg.win[?] pointers is done such that the beginning of the config space 
> specified in the "reg" property corresponds to bus 0.

I don't follow you here. The mapping functions explicitly subtract the
start of the bus range when calculating the window offset:

  resource_size_t idx = bus->number - pci->cfg.bus_range->start;

so if I have bus-range = <128 255>; then bus 128 lives at the start of
the configuration space described by the reg property, not bus 0.

Sorry if I'm being thick; I just can't see the inconsistency.

> The calculation that I am changing, was done such that the beginning of 
> the config space specified in the "reg" property corresponds to the 
> first bus of the "bus-range"
> 
> Which is correct?  I assumed that the config space specified in the 
> "reg" property corresponds to bus 0.  Based on this assumption, I made 
> the bus_max calculation match.
> 
> Due to hardware peculiarities, our bus-range starts at a non-zero bus 
> number.  So, something has to be done to make all the code agree on a 
> single interpretation of the meaning "reg" property.

I think you're the first to exercise this code, so it's definitely worth
us fixing whatever's going wrong.

> > Also, why is your config space so large that
> > we end up overflowing bus_max?
> 
> It isn't.  The part of the patch that changes the type from u8 to int 
> was just to add some sanity.  The  code was easily susceptible to 
> overflow failures, it seemed best to change to int.

Can we drop this part for now if it's not actually needed?

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

Re: [PATCH v5 1/5] arm/arm64: add smccc ARCH32

[Will Deacon]

On Mon, Sep 14, 2015 at 09:30:30AM +0100, Jens Wiklander wrote:
> On Fri, Aug 21, 2015 at 01:43:31PM +0200, Jens Wiklander wrote:
> > On Fri, Aug 21, 2015 at 10:24:30AM +0100, Will Deacon wrote:
> > > On Thu, Aug 20, 2015 at 12:37:29PM +0100, Jens Wiklander wrote:
> > > > On Wed, Aug 19, 2015 at 05:50:09PM +0100, Will Deacon wrote:
> > > > > On Wed, Aug 19, 2015 at 09:40:25AM +0100, Jens Wiklander wrote:
> > > > > > +   mov x28, x0
> > > > > > +   ldp w0, w1, [x28, #SMC_PARAM_W0_OFFS]
> > > > > > +   ldp w2, w3, [x28, #SMC_PARAM_W2_OFFS]
> > > > > > +   ldp w4, w5, [x28, #SMC_PARAM_W4_OFFS]
> > > > > > +   ldp w6, w7, [x28, #SMC_PARAM_W6_OFFS]
> > > > > > +   smc #0
> > > > > > +   stp w0, w1, [x28, #SMC_PARAM_W0_OFFS]
> > > > > > +   stp w2, w3, [x28, #SMC_PARAM_W2_OFFS]
> > > > > > +   ldp x28, x30, [sp], #16
> > > > > > +   ret
> > > > > > +ENDPROC(smccc_call32)
> > > > > 
> > > > > Could we deal with this like we do for PSCI instead? (see
> > > > > __invoke_psci_fn_smc). We could also then rename psci-call.S to 
> > > > > fw-call.S
> > > > > and stick this in there too.
> > > > 
> > > > I assume you're referring to when to use "hvc" and "smc".
> > > 
> > > No, I mean use a C prototype to avoid marshalling the parameters in 
> > > assembly
> > > like this. As Rutland pointed out, the return value is a bit messy, but
> > > the arguments align nicely with the PCS afaict.
> > 
> > If possible I'd like the function to have the same prototype for both
> > arm and arm64. For arm it's not possible to supply more than 4
> > parameters. To fully support SMC Calling Convention we need to be able
> > to pass 8 parameters and have 4 return values. The OP-TEE driver in this
> > patch set depends on this. I don't see how we can avoid the marshalling
> > here.
> > 
> > We could have two versions of the SMCCC functions, one simplified which
> > only uses registers and one complete like this one with marshalling.
> 
> Will, what do think about this?

I still think you should make use of a C prototype to avoid explicit
parameter marshalling in assembly. If you want to maintain a compatible
API between arm and arm64, then you can easily have an intermediate
function in arm64 that sits between the API entry point and the assembly.

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

Re: [PATCH 5/6] PCI: generic: Pass proper starting bus number to pci_scan_root_bus().

[Will Deacon]

On Sat, Sep 12, 2015 at 12:21:58AM +0100, David Daney wrote:
> From: David Daney 
> 
> If the bus is being configured with a bus-range that does not start at
> zero, pass that starting bus number to pci_scan_root_bus().  Passing
> the incorrect value of zero causes attempted config accesses outside
> of the supported range, which cascades to an OOPs spew and eventual
> kernel panic.
> 
> Signed-off-by: David Daney 
> ---
>  drivers/pci/host/pci-host-generic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/host/pci-host-generic.c 
> b/drivers/pci/host/pci-host-generic.c
> index fce5bf7..8219c0b 100644
> --- a/drivers/pci/host/pci-host-generic.c
> +++ b/drivers/pci/host/pci-host-generic.c
> @@ -265,7 +265,7 @@ static int gen_pci_probe(struct platform_device *pdev)
>   if (!pci_has_flag(PCI_PROBE_ONLY))
>   pci_add_flags(PCI_REASSIGN_ALL_RSRC | PCI_REASSIGN_ALL_BUS);
>  
> - bus = pci_scan_root_bus(dev, 0,
> + bus = pci_scan_root_bus(dev, pci->cfg.bus_range->start,
>   &pci->cfg.ops.ops, pci, &pci->resources);
>   if (!bus) {
>   dev_err(dev, "Scanning rootbus failed");

Acked-by: Will Deacon 

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

Re: [PATCH 6/6] PCI: generic: Allow bus default MSI controller to be specified.

[Will Deacon]

On Sat, Sep 12, 2015 at 12:21:59AM +0100, David Daney wrote:
> From: David Daney 
> 
> If the device tree node for the bus has a "msi-parent" property, use
> that as the default MSI controller for devices on the bus.  Add device
> tree binding documentation describing the new property.
> 
> This allows the pci-host-generic driver to be used in systems with
> multiple MSI controllers.
> 
> Signed-off-by: David Daney 
> ---
>  .../devicetree/bindings/pci/host-generic-pci.txt  |  5 +
>  drivers/pci/host/pci-host-generic.c   | 15 
> +--
>  2 files changed, 18 insertions(+), 2 deletions(-)

I don't think you need this anymore with 4.3-rc1. The core IRQ subsystem
should take care of parsing and configuring the MSI parents.

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

Re: [PATCH 4/6] PCI: generic: Correct, and avoid overflow, in bus_max calculation.

[Will Deacon]

On Sat, Sep 12, 2015 at 12:21:57AM +0100, David Daney wrote:
> From: David Daney 
> 
> There are two problems with the bus_max calculation:
> 
> 1) The u8 data type can overflow for large config space windows.
> 
> 2) The calculation is incorrect for a bus range that doesn't start at
>zero.
> 
> Since the configuration space is relative to bus zero, make bus_max
> just be the size of the config window scaled by bus_shift.  Then clamp
> it to a maximum of 255, per PCI.  Use a data type of int to avoid
> overflow problems.
> 
> Signed-off-by: David Daney 
> ---
>  drivers/pci/host/pci-host-generic.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-host-generic.c 
> b/drivers/pci/host/pci-host-generic.c
> index cd6f898..fce5bf7 100644
> --- a/drivers/pci/host/pci-host-generic.c
> +++ b/drivers/pci/host/pci-host-generic.c
> @@ -164,7 +164,7 @@ out_release_res:
>  static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
>  {
>   int err;
> - u8 bus_max;
> + int bus_max;
>   resource_size_t busn;
>   struct resource *bus_range;
>   struct device *dev = pci->host.dev.parent;
> @@ -177,8 +177,9 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci 
> *pci)
>   }
>  
>   /* Limit the bus-range to fit within reg */
> - bus_max = pci->cfg.bus_range->start +
> -   (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1;
> + bus_max = (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1;
> + if (bus_max > 255)
> + bus_max = 255;
>   pci->cfg.bus_range->end = min_t(resource_size_t,
>   pci->cfg.bus_range->end, bus_max);

Hmm, this is changing the meaning of the bus-range property in the
device-tree, which really needs to match what IEEE Std 1275-1994 requires.

My understanding was that the bus-range could be used to offset the config
space, which is why it's subtracted from the bus number in
gen_pci_map_cfg_bus_[e]cam. Also, why is your config space so large that
we end up overflowing bus_max?

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

Re: [PATCH 3/6] PCI: generic: Quit clobbering our pci_ops.

[Will Deacon]

On Sat, Sep 12, 2015 at 12:21:56AM +0100, David Daney wrote:
> From: David Daney 
> 
> The pci-host-generic driver keeps a global struct pci_ops which it
> then patches with the .map_bus method appropriate for the bus device.
> A problem arises when the driver is used for two different types of
> bus devices, the .map_bus method for the last device probed clobbers
> the method for all previous devices.  The result, only the last bus
> device probed has the proper .map_bus, and the others fail.
> 
> Move the struct pci_ops into the bus specific structure, and
> initialize it when the bus device is probed.  Keep a copy of the
> gen_pci_cfg_bus_ops structure, instead of a pointer to a global copy,
> to future proof against the addition of bus specific elements to
> struct pci_ops.

This looks ok to me:

  Acked-by: Will Deacon 

Will

> Signed-off-by: David Daney 
> ---
>  drivers/pci/host/pci-host-generic.c | 31 +--
>  1 file changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-host-generic.c 
> b/drivers/pci/host/pci-host-generic.c
> index a0fb241..cd6f898 100644
> --- a/drivers/pci/host/pci-host-generic.c
> +++ b/drivers/pci/host/pci-host-generic.c
> @@ -27,7 +27,7 @@
>  
>  struct gen_pci_cfg_bus_ops {
>   u32 bus_shift;
> - void __iomem *(*map_bus)(struct pci_bus *, unsigned int, int);
> + struct pci_ops ops;
>  };
>  
>  struct gen_pci_cfg_windows {
> @@ -35,7 +35,7 @@ struct gen_pci_cfg_windows {
>   struct resource *bus_range;
>   void __iomem**win;
>  
> - const struct gen_pci_cfg_bus_ops*ops;
> + struct gen_pci_cfg_bus_ops  ops;
>  };
>  
>  /*
> @@ -65,7 +65,11 @@ static void __iomem *gen_pci_map_cfg_bus_cam(struct 
> pci_bus *bus,
>  
>  static struct gen_pci_cfg_bus_ops gen_pci_cfg_cam_bus_ops = {
>   .bus_shift  = 16,
> - .map_bus= gen_pci_map_cfg_bus_cam,
> + .ops= {
> + .map_bus= gen_pci_map_cfg_bus_cam,
> + .read   = pci_generic_config_read,
> + .write  = pci_generic_config_write,
> + }
>  };
>  
>  static void __iomem *gen_pci_map_cfg_bus_ecam(struct pci_bus *bus,
> @@ -80,12 +84,11 @@ static void __iomem *gen_pci_map_cfg_bus_ecam(struct 
> pci_bus *bus,
>  
>  static struct gen_pci_cfg_bus_ops gen_pci_cfg_ecam_bus_ops = {
>   .bus_shift  = 20,
> - .map_bus= gen_pci_map_cfg_bus_ecam,
> -};
> -
> -static struct pci_ops gen_pci_ops = {
> - .read   = pci_generic_config_read,
> - .write  = pci_generic_config_write,
> + .ops= {
> + .map_bus= gen_pci_map_cfg_bus_ecam,
> + .read   = pci_generic_config_read,
> + .write  = pci_generic_config_write,
> + }
>  };
>  
>  static const struct of_device_id gen_pci_of_match[] = {
> @@ -175,7 +178,7 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci 
> *pci)
>  
>   /* Limit the bus-range to fit within reg */
>   bus_max = pci->cfg.bus_range->start +
> -   (resource_size(&pci->cfg.res) >> pci->cfg.ops->bus_shift) - 1;
> +   (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1;
>   pci->cfg.bus_range->end = min_t(resource_size_t,
>   pci->cfg.bus_range->end, bus_max);
>  
> @@ -193,7 +196,7 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci 
> *pci)
>   bus_range = pci->cfg.bus_range;
>   for (busn = bus_range->start; busn <= bus_range->end; ++busn) {
>   u32 idx = busn - bus_range->start;
> - u32 sz = 1 << pci->cfg.ops->bus_shift;
> + u32 sz = 1 << pci->cfg.ops.bus_shift;
>  
>   pci->cfg.win[idx] = devm_ioremap(dev,
>pci->cfg.res.start + busn * sz,
> @@ -240,8 +243,7 @@ static int gen_pci_probe(struct platform_device *pdev)
>   }
>  
>   of_id = of_match_node(gen_pci_of_match, np);
> - pci->cfg.ops = of_id->data;
> - gen_pci_ops.map_bus = pci->cfg.ops->map_bus;
> + pci->cfg.ops = *(struct gen_pci_cfg_bus_ops *)of_id->data;
>   pci->host.dev.parent = dev;
>   INIT_LIST_HEAD(&pci->host.windows);
>   INIT_LIST_HEAD(&pci->resources);
> @@ -262,7 +264,8 @@ static int gen_pci_probe(struct platform_device *pdev)
>   if (!pci_has_flag(PCI_PROBE_ONLY))
>   pci_add_flags(PCI_REASSIGN_ALL_RSRC | PCI_REASSIGN_ALL_BUS);
&

Re: [PATCH 2/6] PCI: generic: Only fixup irqs for bus we are creating.

[Will Deacon]

Hi David,

On Sat, Sep 12, 2015 at 12:21:55AM +0100, David Daney wrote:
> From: David Daney 
> 
> Use pci_walk_bus() to restrict the fixup irq actions to only the bus
> being created.
> 
> If we create multiple buses with pci-host-generic, or there are buses
> created by other drivers, we don't want to call pci_fixup_irqs() which
> operates on all devices, not just the devices on the bus being added.
> The consequence is that either the fixups are done more than once, or
> in some cases incorrect fixups could be applied.
> 
> Signed-off-by: David Daney 
> ---
>  drivers/pci/host/pci-host-generic.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/host/pci-host-generic.c 
> b/drivers/pci/host/pci-host-generic.c
> index 265dd25..a0fb241 100644
> --- a/drivers/pci/host/pci-host-generic.c
> +++ b/drivers/pci/host/pci-host-generic.c
> @@ -205,6 +205,12 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci 
> *pci)
>   return 0;
>  }
>  
> +static int gen_pci_fixup_irq_cb(struct pci_dev *dev, void *arg)
> +{
> + pdev_fixup_irq(dev, pci_common_swizzle, of_irq_parse_and_map_pci);
> + return 0;
> +}
> +
>  static int gen_pci_probe(struct platform_device *pdev)
>  {
>   int err;
> @@ -262,7 +268,7 @@ static int gen_pci_probe(struct platform_device *pdev)
>   return -ENODEV;
>   }
>  
> - pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
> + pci_walk_bus(bus, gen_pci_fixup_irq_cb, NULL);

Any chance we could put something in the core PCI code for this? I think
any host controller wanting to work with arm64 is potentially going to
run into the same problem.

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

Re: [PATCH v4 2/4] PCI: pci-host-generic: Fix lookup of linux,pci-probe-only property

[Will Deacon]

On Fri, Sep 04, 2015 at 05:50:09PM +0100, Marc Zyngier wrote:
> When pci-host-generic looks for the probe-only property, it seems
> to trust the DT to be correctly written, and assumes that there
> is a parameter to the property.
> 
> Unfortunately, this is not always the case, and some firmware expose
> this property naked. The driver ends up making a decision based on
> whatever the property pointer points to, which is likely to be junk.
> 
> Switch to the common of_pci.c implementation that doesn't suffer
> from this problem.
> 
> Signed-off-by: Marc Zyngier 
> ---
>  drivers/pci/host/pci-host-generic.c | 9 +
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-host-generic.c 
> b/drivers/pci/host/pci-host-generic.c
> index 265dd25..224303d 100644
> --- a/drivers/pci/host/pci-host-generic.c
> +++ b/drivers/pci/host/pci-host-generic.c
> @@ -210,7 +210,6 @@ static int gen_pci_probe(struct platform_device *pdev)
>   int err;
>   const char *type;
>   const struct of_device_id *of_id;
> - const int *prop;
>   struct device *dev = &pdev->dev;
>   struct device_node *np = dev->of_node;
>   struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
> @@ -225,13 +224,7 @@ static int gen_pci_probe(struct platform_device *pdev)
>   return -EINVAL;
>   }
>  
> - prop = of_get_property(of_chosen, "linux,pci-probe-only", NULL);
> - if (prop) {
> - if (*prop)
> - pci_add_flags(PCI_PROBE_ONLY);
> - else
> - pci_clear_flags(PCI_PROBE_ONLY);
> - }
> + of_pci_check_probe_only();

Looks good to me:

  Acked-by: Will Deacon 

Thanks, Marc.

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

Re: [PATCH v5 1/4] arm64, numa: adding numa support for arm64 platforms.

[Will Deacon]

On Thu, Sep 03, 2015 at 10:52:02AM +0100, Ganapatrao Kulkarni wrote:
> On Fri, Aug 14, 2015 at 10:09 PM, Ganapatrao Kulkarni
>  wrote:
> > Adding numa support for arm64 based platforms.
> > This patch adds by default the dummy numa node and
> > maps all memory and cpus to node 0.
> > using this patch, numa can be simulated on single node arm64 platforms.
> >
> > Signed-off-by: Ganapatrao Kulkarni 

[trimmed ~850 lines of context]

> can you please review this patch.

It's the middle of the merge window, please be patient.

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

Re: [PATCH v5 1/5] arm/arm64: add smccc ARCH32

[Will Deacon]

On Thu, Aug 20, 2015 at 12:37:29PM +0100, Jens Wiklander wrote:
> On Wed, Aug 19, 2015 at 05:50:09PM +0100, Will Deacon wrote:
> > On Wed, Aug 19, 2015 at 09:40:25AM +0100, Jens Wiklander wrote:
> > > Adds helpers to do SMC based on ARM SMC Calling Convention.
> > > CONFIG_HAVE_SMCCC is enabled for architectures that may support
> > > the SMC instruction. It's the responsibility of the caller to
> > > know if the SMC instruction is supported by the platform.
> > 
> > [...]
> > > diff --git a/arch/arm64/kernel/smccc-call.S 
> > > b/arch/arm64/kernel/smccc-call.S
> > > new file mode 100644
> > > index 000..3ce7fe8
> > > --- /dev/null
> > > +++ b/arch/arm64/kernel/smccc-call.S
> > > @@ -0,0 +1,34 @@
> > > +/*
> > > + * Copyright (c) 2015, Linaro Limited
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License Version 2 as
> > > + * published by the Free Software Foundation.
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > > + * GNU General Public License for more details.
> > > + *
> > > + */
> > > +#include 
> > > +
> > > +#define SMC_PARAM_W0_OFFS  0
> > > +#define SMC_PARAM_W2_OFFS  8
> > > +#define SMC_PARAM_W4_OFFS  16
> > > +#define SMC_PARAM_W6_OFFS  24
> > > +
> > > +/* void smccc_call32(struct smccc_param32 *param) */
> > > +ENTRY(smccc_call32)
> > > +   stp x28, x30, [sp, #-16]!
> > 
> > Why are you saving lr?
> 
> Agree, no point in saving lr, but I still need to decrease sp with 16 to
> maintain correct alignment. I'll do it with an str instruction instead.

That or pad out with xzr

> > 
> > > +   mov x28, x0
> > > +   ldp w0, w1, [x28, #SMC_PARAM_W0_OFFS]
> > > +   ldp w2, w3, [x28, #SMC_PARAM_W2_OFFS]
> > > +   ldp w4, w5, [x28, #SMC_PARAM_W4_OFFS]
> > > +   ldp w6, w7, [x28, #SMC_PARAM_W6_OFFS]
> > > +   smc #0
> > > +   stp w0, w1, [x28, #SMC_PARAM_W0_OFFS]
> > > +   stp w2, w3, [x28, #SMC_PARAM_W2_OFFS]
> > > +   ldp x28, x30, [sp], #16
> > > +   ret
> > > +ENDPROC(smccc_call32)
> > 
> > Could we deal with this like we do for PSCI instead? (see
> > __invoke_psci_fn_smc). We could also then rename psci-call.S to fw-call.S
> > and stick this in there too.
> 
> I assume you're referring to when to use "hvc" and "smc".

No, I mean use a C prototype to avoid marshalling the parameters in assembly
like this. As Rutland pointed out, the return value is a bit messy, but
the arguments align nicely with the PCS afaict.

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

Re: [PATCH v5 1/5] arm/arm64: add smccc ARCH32

[Will Deacon]

On Wed, Aug 19, 2015 at 09:40:25AM +0100, Jens Wiklander wrote:
> Adds helpers to do SMC based on ARM SMC Calling Convention.
> CONFIG_HAVE_SMCCC is enabled for architectures that may support
> the SMC instruction. It's the responsibility of the caller to
> know if the SMC instruction is supported by the platform.

[...]
> diff --git a/arch/arm64/kernel/smccc-call.S b/arch/arm64/kernel/smccc-call.S
> new file mode 100644
> index 000..3ce7fe8
> --- /dev/null
> +++ b/arch/arm64/kernel/smccc-call.S
> @@ -0,0 +1,34 @@
> +/*
> + * Copyright (c) 2015, Linaro Limited
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License Version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +#include 
> +
> +#define SMC_PARAM_W0_OFFS  0
> +#define SMC_PARAM_W2_OFFS  8
> +#define SMC_PARAM_W4_OFFS  16
> +#define SMC_PARAM_W6_OFFS  24
> +
> +/* void smccc_call32(struct smccc_param32 *param) */
> +ENTRY(smccc_call32)
> +   stp x28, x30, [sp, #-16]!

Why are you saving lr?

> +   mov x28, x0
> +   ldp w0, w1, [x28, #SMC_PARAM_W0_OFFS]
> +   ldp w2, w3, [x28, #SMC_PARAM_W2_OFFS]
> +   ldp w4, w5, [x28, #SMC_PARAM_W4_OFFS]
> +   ldp w6, w7, [x28, #SMC_PARAM_W6_OFFS]
> +   smc #0
> +   stp w0, w1, [x28, #SMC_PARAM_W0_OFFS]
> +   stp w2, w3, [x28, #SMC_PARAM_W2_OFFS]
> +   ldp x28, x30, [sp], #16
> +   ret
> +ENDPROC(smccc_call32)

Could we deal with this like we do for PSCI instead? (see
__invoke_psci_fn_smc). We could also then rename psci-call.S to fw-call.S
and stick this in there too.

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

Re: [PATCH v2 2/4] PCI: pci-host-generic: Fix lookup of linux,pci-probe-only property

[Will Deacon]

On Fri, Aug 14, 2015 at 09:26:21PM +0100, Bjorn Helgaas wrote:
> On Fri, Aug 14, 2015 at 11:43 AM, Will Deacon  wrote:
> > On Fri, Aug 14, 2015 at 05:40:51PM +0100, Bjorn Helgaas wrote:
> >> Do we need support for pci-probe-only in pci-host-generic at all?
> >> You're removing the use in amd-overdrive.dts, and there are no other
> >> DTs in the kernel tree that mention it.
> >>
> >> If we can live without it, that would be nice.  It seems like a relic from
> >> days when we couldn't reliably assign resources.  (I'm not saying we can do
> >> that reliably even today, but I'd rather make it reliable than turn it
> >> off.)
> >
> > Kvmtool certainly uses it (and generates its own DT, hence why you don't
> > see it in mainline). Not sure about qemu, though.
> 
> Do you know why kvmtool wants probe-only?  Would something break if we
> didn't support probe-only?  I guess we'd be looking for a case where
> Linux assigns resources and that assignment doesn't work with kvmtool?

It's basically because the BARs aren't writable other than to find the
region size. It could fixed with a bit of pain, but it doesn't help older
kvmtools that do work with mainline today.

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

Re: [PATCH v2 2/4] PCI: pci-host-generic: Fix lookup of linux,pci-probe-only property

[Will Deacon]

On Fri, Aug 14, 2015 at 05:19:17PM +0100, Marc Zyngier wrote:
> When pci-host-generic looks for the probe-only property, it seems
> to trust the DT to be correctly written, and assumes that there
> is a parameter to the property.
> 
> Unfortunately, this is not always the case, and some firmware expose
> this property naked. The driver ends up making a decision based on
> whatever the property pointer points to, which is likely to be junk.
> 
> Switch to the common of_pci.c implementation that doesn't suffer
> from this problem.
> 
> Signed-off-by: Marc Zyngier 
> ---
>  drivers/pci/host/pci-host-generic.c | 9 +
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-host-generic.c 
> b/drivers/pci/host/pci-host-generic.c
> index 265dd25..545ff4e 100644
> --- a/drivers/pci/host/pci-host-generic.c
> +++ b/drivers/pci/host/pci-host-generic.c
> @@ -210,7 +210,6 @@ static int gen_pci_probe(struct platform_device *pdev)
>   int err;
>   const char *type;
>   const struct of_device_id *of_id;
> - const int *prop;
>   struct device *dev = &pdev->dev;
>   struct device_node *np = dev->of_node;
>   struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
> @@ -225,13 +224,7 @@ static int gen_pci_probe(struct platform_device *pdev)
>   return -EINVAL;
>   }
>  
> - prop = of_get_property(of_chosen, "linux,pci-probe-only", NULL);
> - if (prop) {
> - if (*prop)
> - pci_add_flags(PCI_PROBE_ONLY);
> - else
> - pci_clear_flags(PCI_PROBE_ONLY);
> - }
> + of_pci_check_probe_only(of_chosen);

You could probably just make this take void, as the probe-only property
is always in the /chosen node.

Either way:

  Acked-by: Will Deacon 

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

Re: [PATCH v2 2/4] PCI: pci-host-generic: Fix lookup of linux,pci-probe-only property

[Will Deacon]

On Fri, Aug 14, 2015 at 05:40:51PM +0100, Bjorn Helgaas wrote:
> On Fri, Aug 14, 2015 at 05:19:17PM +0100, Marc Zyngier wrote:
> > When pci-host-generic looks for the probe-only property, it seems
> > to trust the DT to be correctly written, and assumes that there
> > is a parameter to the property.
> > 
> > Unfortunately, this is not always the case, and some firmware expose
> > this property naked. The driver ends up making a decision based on
> > whatever the property pointer points to, which is likely to be junk.
> > 
> > Switch to the common of_pci.c implementation that doesn't suffer
> > from this problem.
> > 
> > Signed-off-by: Marc Zyngier 
> > ---
> >  drivers/pci/host/pci-host-generic.c | 9 +
> >  1 file changed, 1 insertion(+), 8 deletions(-)
> > 
> > diff --git a/drivers/pci/host/pci-host-generic.c 
> > b/drivers/pci/host/pci-host-generic.c
> > index 265dd25..545ff4e 100644
> > --- a/drivers/pci/host/pci-host-generic.c
> > +++ b/drivers/pci/host/pci-host-generic.c
> > @@ -210,7 +210,6 @@ static int gen_pci_probe(struct platform_device *pdev)
> > int err;
> > const char *type;
> > const struct of_device_id *of_id;
> > -   const int *prop;
> > struct device *dev = &pdev->dev;
> > struct device_node *np = dev->of_node;
> > struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
> > @@ -225,13 +224,7 @@ static int gen_pci_probe(struct platform_device *pdev)
> > return -EINVAL;
> > }
> >  
> > -   prop = of_get_property(of_chosen, "linux,pci-probe-only", NULL);
> > -   if (prop) {
> > -   if (*prop)
> > -   pci_add_flags(PCI_PROBE_ONLY);
> > -   else
> > -   pci_clear_flags(PCI_PROBE_ONLY);
> > -   }
> > +   of_pci_check_probe_only(of_chosen);
> 
> Do we need support for pci-probe-only in pci-host-generic at all?
> You're removing the use in amd-overdrive.dts, and there are no other
> DTs in the kernel tree that mention it.
> 
> If we can live without it, that would be nice.  It seems like a relic from
> days when we couldn't reliably assign resources.  (I'm not saying we can do
> that reliably even today, but I'd rather make it reliable than turn it
> off.)

Kvmtool certainly uses it (and generates its own DT, hence why you don't
see it in mainline). Not sure about qemu, though.

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

Re: [PATCH 4/5] iommu/msm: Set cacheability attributes without tex remap

[Will Deacon]

On Wed, Aug 12, 2015 at 03:47:48PM +0100, Sricharan R wrote:
> The cacheablity attributes are set when IOMMU_CACHE property
> is true. So cachebility is set as either noncached (normal)
> or cached (normal WBWA) directly and avoid setting using
> tex remap.

Does this IOMMU support the ARMv7 short descriptor format? If so, would
it work with Yong's patch here:

  http://lists.infradead.org/pipermail/linux-arm-kernel/2015-August/361615.html

I've not gotten around to reviewing the latest version yet, but having
other IOMMUs consolidate on one set of page table code would be a good
thing.

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

Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.

[Will Deacon]

On Fri, Jul 31, 2015 at 08:55:37AM +0100, Yong Wu wrote:
> About the AP bits, I may have to add a new quirk for it...
> 
>   Current I add AP in pte like this:
> #define ARM_SHORT_PTE_RD_WR(3 << 4)
> #define ARM_SHORT_PTE_RDONLY   BIT(9)
> 
> pteprot |=  ARM_SHORT_PTE_RD_WR;
> 
> 
>  If(!(prot & IOMMU_WRITE) && (prot & IOMMU_READ))
> 
> 
>   pteprot |= ARM_SHORT_PTE_RDONLY;
> 
> The problem is that the BIT(9) in the level1 and level2 pagetable of our
> HW has been used for PA[32] that is for the dram size over 4G.

Aha, now *thats* a case of page-table abuse!

> so I had to add a quirk to disable bit9 while RDONLY case.
> (If BIT9 isn't disabled, the HW treat it as the PA[32] case then it will
> translation fault..)
> 
> like: IO_PGTABLE_QUIRK_SHORT_MTK ?

Given that you don't have XN either, maybe IO_PGTABLE_QUIRK_NO_PERMS?
When set, IOMMU_READ/WRITE/EXEC are ignored and the mapping will never
generate a permission fault.

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

Re: [PATCH v3 5/6] iommu/mediatek: Add mt8173 IOMMU driver

[Will Deacon]

On Wed, Jul 29, 2015 at 06:41:31AM +0100, Yong Wu wrote:
> On Mon, 2015-07-27 at 16:49 +0100, Robin Murphy wrote:
> > On 27/07/15 16:31, Russell King - ARM Linux wrote:
> > > On Mon, Jul 27, 2015 at 02:23:26PM +0100, Robin Murphy wrote:
> > >> On 16/07/15 10:04, Yong Wu wrote:
> > >>> This patch adds support for mediatek m4u (MultiMedia Memory Management
> > >>> Unit).
> > >>>
> > >>> Signed-off-by: Yong Wu 
> > >> [...]
> > >>> +static void mtk_iommu_flush_pgtable(void *ptr, size_t size, void 
> > >>> *cookie)
> > >>> +{
> > >>> +   struct mtk_iommu_domain *domain = cookie;
> > >>> +   unsigned long offset = (unsigned long)ptr & ~PAGE_MASK;
> > >>> +
> > >>> +   dma_map_page(domain->data->dev, virt_to_page(ptr), offset,
> > >>> +size, DMA_TO_DEVICE);
> > >>
> > >> Nit: this looks like it may as well be dma_map_single.
> > >>
> > >> It would probably be worth following it with a matching unmap too, just 
> > >> to
> > >> avoid any possible leakage bugs (especially if this M4U ever appears in a
> > >> SoC supporting RAM above the 32-bit boundary).
> > >
> > > Why not do the job properly?  Take a look at how I implemented the
> > > streaming DMA API on Tegra SMMU (patch set recently sent out earlier
> > > today).
> > >
> > > There's no need for hacks like dma_map_page() (and discarding it's
> > > return value) or dma_map_page() followed by dma_unmap_page().
> > 
> > Indeed, as it happens I do have a branch where I prototyped that for the 
> > long-descriptor io-pgtable-arm code a while ago; this discussion has 
> > prompted me to dig it up again. Stay tuned, folks...
> 
> Hi Russell, Robin,
> 
>  From I see in arm-smmu-v3.c in v4.2-rc1, 
>  
>  The flush_pgtable seems like this:
> //==
>   dma_addr_t dma_addr;
> 
>   dma_addr = dma_map_page(dev, ptr, size, DMA_TO_DEVICE);
> 
>   if (dma_mapping_error(dev, dma_addr))
>   dev_err(dev, "failed to flush pgtable at %p\n", ptr);
>   else
>   dma_unmap_page(dev, dma_addr, size, DMA_TO_DEVICE);
> //==
>I will change map like this and use dma_map_single instead.
> 
>Is this also seems to be not proper?
> 
>Then how to do it?  add this before unmap? :
>dma_sync_single_for_device(dev, dma_addr, size, DMA_TO_DEVICE);

Robin's proposed a series fixing this in the io-pgtable code:

  http://lists.linuxfoundation.org/pipermail/iommu/2015-July/013821.html

which is currently under review. Please take a look and give comments!
Once merged, the code you cite above will be removed.

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

Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.

[Will Deacon]

On Tue, Jul 28, 2015 at 02:37:43PM +0100, Yong Wu wrote:
> On Tue, 2015-07-28 at 12:00 +0100, Will Deacon wrote:
> > On Tue, Jul 28, 2015 at 06:08:14AM +0100, Yong Wu wrote:
> > > On Mon, 2015-07-27 at 15:11 +0100, Will Deacon wrote:
> > > > On Mon, Jul 27, 2015 at 03:05:38PM +0100, Robin Murphy wrote:
> > > > > On 27/07/15 05:21, Yong Wu wrote:
> > > > > >>>>> +   } else {/* page or largepage */
> > > > > >>>>> +   if (quirk & IO_PGTABLE_QUIRK_SHORT_MTK) {
> > > > > >>>>> +   if (large) { /* special Bit */
> > > > > >>>>
> > > > > >>>> This definitely needs a better comment! What exactly are you 
> > > > > >>>> doing here
> > > > > >>>> and what is that quirk all about?
> > > > > >>>
> > > > > >>> I use this quirk is for MTK Special Bit as we don't have the XN 
> > > > > >>> bit in
> > > > > >>> pagetable.
> > > > > >>
> > > > > >> I'm still not really clear about what this is.
> > > > > >
> > > > > > There is some difference between the standard spec and MTK HW,
> > > > > > Our hw don't implement some bits, like XN and AP.
> > > > > > So I add a quirk for MTK special.
> > > > > 
> > > > > When you say it doesn't implement these bits, do you mean that having 
> > > > > them set will lead to Bad Things happening in the hardware, or that 
> > > > > it 
> > > > > will simply ignore them and not enforce any of the protections they 
> > > > > imply? The former case would definitely want clearly documenting 
> > > > > somewhere, whereas for the latter case I'm not sure it's even worth 
> > > > > the 
> > > > > complication of having a quirk - if the value doesn't matter there 
> > > > > seems 
> > > > > little point in doing a special dance just for the sake of semantic 
> > > > > correctness of the in-memory PTEs, in my opinion.
> > > > 
> > > > Agreed. We should only use quirks if the current (architecturally
> > > > compliant) code causes real issues with the hardware. Then the quirk can
> > > > be used to either avoid the problematic routines or to take extra steps
> > > > to make things work as the architecture intended.
> > > > 
> > > > I've asked how this IOMMU differs from the architecture on a number of
> > > > occasions, but I'm still yet to receive a response other than "it's 
> > > > special".
> > > > 
> > > 
> > > After check further with DE, Our pagetable is refer to ARM-v7's
> > > short-descriptor which is a little different from ARM-v8. like bit0(PXN)
> > > in section and supersection, I didn't read ARM-v7 spec before, so I add
> > > a MTK quirk to disable PXN bit in section and supersection.(if the PXN
> > > bit is wrote in ARM-v7 spec, HW will page fault.)
> > 
> > I've been reviewing this using the ARMv7 ARM (Rev.C of DDI0406C) the whole
> > time. PXN is there as an optional field in non-LPAE implementations. That's
> > fine and doesn't require any quirks.
> 
> Thanks for your confirm.
> Then I delete all the PXN bit in here?
> 
> Take a example, 
> #define ARM_SHORT_PGD_SECTION_XN  (BIT(0) | BIT(4))
> I will change it to "BIT(4)".

Yes. Then the PXN bit can be added later as a quirk when we have an
implementation that supports it.

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

Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.

[Will Deacon]

On Tue, Jul 28, 2015 at 06:08:14AM +0100, Yong Wu wrote:
> On Mon, 2015-07-27 at 15:11 +0100, Will Deacon wrote:
> > On Mon, Jul 27, 2015 at 03:05:38PM +0100, Robin Murphy wrote:
> > > On 27/07/15 05:21, Yong Wu wrote:
> > > >>>>> +   } else {/* page or largepage */
> > > >>>>> +   if (quirk & IO_PGTABLE_QUIRK_SHORT_MTK) {
> > > >>>>> +   if (large) { /* special Bit */
> > > >>>>
> > > >>>> This definitely needs a better comment! What exactly are you doing 
> > > >>>> here
> > > >>>> and what is that quirk all about?
> > > >>>
> > > >>> I use this quirk is for MTK Special Bit as we don't have the XN bit in
> > > >>> pagetable.
> > > >>
> > > >> I'm still not really clear about what this is.
> > > >
> > > > There is some difference between the standard spec and MTK HW,
> > > > Our hw don't implement some bits, like XN and AP.
> > > > So I add a quirk for MTK special.
> > > 
> > > When you say it doesn't implement these bits, do you mean that having 
> > > them set will lead to Bad Things happening in the hardware, or that it 
> > > will simply ignore them and not enforce any of the protections they 
> > > imply? The former case would definitely want clearly documenting 
> > > somewhere, whereas for the latter case I'm not sure it's even worth the 
> > > complication of having a quirk - if the value doesn't matter there seems 
> > > little point in doing a special dance just for the sake of semantic 
> > > correctness of the in-memory PTEs, in my opinion.
> > 
> > Agreed. We should only use quirks if the current (architecturally
> > compliant) code causes real issues with the hardware. Then the quirk can
> > be used to either avoid the problematic routines or to take extra steps
> > to make things work as the architecture intended.
> > 
> > I've asked how this IOMMU differs from the architecture on a number of
> > occasions, but I'm still yet to receive a response other than "it's 
> > special".
> > 
> 
> After check further with DE, Our pagetable is refer to ARM-v7's
> short-descriptor which is a little different from ARM-v8. like bit0(PXN)
> in section and supersection, I didn't read ARM-v7 spec before, so I add
> a MTK quirk to disable PXN bit in section and supersection.(if the PXN
> bit is wrote in ARM-v7 spec, HW will page fault.)

I've been reviewing this using the ARMv7 ARM (Rev.C of DDI0406C) the whole
time. PXN is there as an optional field in non-LPAE implementations. That's
fine and doesn't require any quirks.

> Then I write this code according to ARM-v8 spec defaultly, and add a
> ARM-v7 quirk?

No, I don't think you need this, as the v8 and v7 short-descriptor formats
look compatible to me. You should only need a quirk if architecturally
compliant code cannot work on your hardware.

> And there is a little different between ARM-v7 spec and MTK pagetable.
> It's the XN(bit0) in small page. MTK don't implement XN bit. 
> The bit[1:0] in MTK's small page should be 2'b10, if it's 2'b11, HW will
> page fault.

Aha, thanks! *That* is worthy of a quirk. Something like:

  IO_PGTABLE_QUIRK_ARM_NO_XN

> (MTK don't implement AP bits too, but HW don't use them, it is ok even
> though AP bits is wrote)

Yeah, I think that's fine. The pgtable code will honour the request but
the h/w will ignore it.

> In the end, I will add two quirk like this, is it OK?

I think you only need the one I mentioned above. I don't see the need
for PXN at all (as I said in the last review).

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

Re: [PATCH v3 5/6] iommu/mediatek: Add mt8173 IOMMU driver

[Will Deacon]

On Mon, Jul 27, 2015 at 05:24:31AM +0100, Yong Wu wrote:
> On Fri, 2015-07-24 at 17:55 +0100, Will Deacon wrote:
> > On Fri, Jul 24, 2015 at 06:43:13AM +0100, Yong Wu wrote:
> > > On Tue, 2015-07-21 at 15:59 +0100, Will Deacon wrote:
> > > > On Thu, Jul 16, 2015 at 10:04:34AM +0100, Yong Wu wrote:
> > > > > +static void mtk_iommu_tlb_flush_all(void *cookie)
> > > > > +{
> > > > > +   struct mtk_iommu_domain *domain = cookie;
> > > > > +   void __iomem *base;
> > > > > +
> > > > > +   base = domain->data->base;
> > > > > +   writel(F_INVLD_EN1 | F_INVLD_EN0, base + REG_MMU_INV_SEL);
> > > > > +   writel(F_ALL_INVLD, base + REG_MMU_INVALIDATE);
> > > > 
> > > > This needs to be synchronous, so you probably want to call
> > > > mtk_iommu_tlb_sync at the end.
> > > 
> > > From our spec, we have to wait until HW done after tlb flush range.
> > > But it don't need wait after tlb flush all.
> > > so It isn't necessary to add mtk_iommu_tlb_sync in tlb_flush_all here.
> > 
> > Okey doke, but I'm surprised you don't need a subsequent DSB or read-back.
> > What if the writel is buffered on the way to the IOMMU?
> 
> Then I change to this:
>  //==
> writel_relaxed(F_INVLD_EN1 | F_INVLD_EN0, base + REG_MMU_INV_SEL);
> writel_relaxed(F_ALL_INVLD, base + REG_MMU_INVALIDATE);
> dsb(ishst);
> //===
> dsb or mb(). which one is better here?

I think you should use mb();

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

Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.

[Will Deacon]

On Mon, Jul 27, 2015 at 03:05:38PM +0100, Robin Murphy wrote:
> On 27/07/15 05:21, Yong Wu wrote:
> > +   } else {/* page or largepage */
> > +   if (quirk & IO_PGTABLE_QUIRK_SHORT_MTK) {
> > +   if (large) { /* special Bit */
> 
>  This definitely needs a better comment! What exactly are you doing here
>  and what is that quirk all about?
> >>>
> >>> I use this quirk is for MTK Special Bit as we don't have the XN bit in
> >>> pagetable.
> >>
> >> I'm still not really clear about what this is.
> >
> > There is some difference between the standard spec and MTK HW,
> > Our hw don't implement some bits, like XN and AP.
> > So I add a quirk for MTK special.
> 
> When you say it doesn't implement these bits, do you mean that having 
> them set will lead to Bad Things happening in the hardware, or that it 
> will simply ignore them and not enforce any of the protections they 
> imply? The former case would definitely want clearly documenting 
> somewhere, whereas for the latter case I'm not sure it's even worth the 
> complication of having a quirk - if the value doesn't matter there seems 
> little point in doing a special dance just for the sake of semantic 
> correctness of the in-memory PTEs, in my opinion.

Agreed. We should only use quirks if the current (architecturally
compliant) code causes real issues with the hardware. Then the quirk can
be used to either avoid the problematic routines or to take extra steps
to make things work as the architecture intended.

I've asked how this IOMMU differs from the architecture on a number of
occasions, but I'm still yet to receive a response other than "it's special".

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

Re: [PATCH v3 5/6] iommu/mediatek: Add mt8173 IOMMU driver

[Will Deacon]

On Fri, Jul 24, 2015 at 06:43:13AM +0100, Yong Wu wrote:
> On Tue, 2015-07-21 at 15:59 +0100, Will Deacon wrote:
> > On Thu, Jul 16, 2015 at 10:04:34AM +0100, Yong Wu wrote:
> > > +static void mtk_iommu_tlb_flush_all(void *cookie)
> > > +{
> > > +   struct mtk_iommu_domain *domain = cookie;
> > > +   void __iomem *base;
> > > +
> > > +   base = domain->data->base;
> > > +   writel(F_INVLD_EN1 | F_INVLD_EN0, base + REG_MMU_INV_SEL);
> > > +   writel(F_ALL_INVLD, base + REG_MMU_INVALIDATE);
> > 
> > This needs to be synchronous, so you probably want to call
> > mtk_iommu_tlb_sync at the end.
> 
> From our spec, we have to wait until HW done after tlb flush range.
> But it don't need wait after tlb flush all.
> so It isn't necessary to add mtk_iommu_tlb_sync in tlb_flush_all here.

Okey doke, but I'm surprised you don't need a subsequent DSB or read-back.
What if the writel is buffered on the way to the IOMMU?

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

Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.

[Will Deacon]

On Fri, Jul 24, 2015 at 06:24:26AM +0100, Yong Wu wrote:
> On Tue, 2015-07-21 at 18:11 +0100, Will Deacon wrote:
> > On Thu, Jul 16, 2015 at 10:04:32AM +0100, Yong Wu wrote:
> > > +/* level 2 pagetable */
> > > +#define ARM_SHORT_PTE_TYPE_LARGE   BIT(0)
> > > +#define ARM_SHORT_PTE_SMALL_XN BIT(0)
> > > +#define ARM_SHORT_PTE_TYPE_SMALL   BIT(1)
> > > +#define ARM_SHORT_PTE_BBIT(2)
> > > +#define ARM_SHORT_PTE_CBIT(3)
> > > +#define ARM_SHORT_PTE_SMALL_TEX0   BIT(6)
> > > +#define ARM_SHORT_PTE_IMPLEBIT(9)
> >
> > This is AP[2] for small pages.
> 
> Sorry, In our pagetable bit9 in PGD and PTE is PA[32] that is for  the
> dram size over 4G. I didn't care it is different in PTE of the standard
> spec.
> And I don't use the AP[2] currently, so I only delete this line in next
> time.

Is this related to the "special bit". What would be good is a comment
next to the #define for the quirk describing *exactly* that differs in
your implementation. Without that, it's very difficult to know what is
intentional and what is actually broken.

> > > +static arm_short_iopte
> > > +__arm_short_pte_prot(struct arm_short_io_pgtable *data, int prot, bool 
> > > large)
> > > +{
> > > +   arm_short_iopte pteprot;
> > > +
> > > +   pteprot = ARM_SHORT_PTE_S | ARM_SHORT_PTE_nG;
> > > +   pteprot |= large ? ARM_SHORT_PTE_TYPE_LARGE :
> > > +   ARM_SHORT_PTE_TYPE_SMALL;
> > > +   if (prot & IOMMU_CACHE)
> > > +   pteprot |=  ARM_SHORT_PTE_B | ARM_SHORT_PTE_C;
> > > +   if (prot & IOMMU_WRITE)
> > > +   pteprot |= large ? ARM_SHORT_PTE_LARGE_TEX0 :
> > > +   ARM_SHORT_PTE_SMALL_TEX0;
> >
> > This doesn't make any sense. TEX[2:0] is all about memory attributes, not
> > permissions, so you're making the mapping write-back, write-allocate but
> > that's not what the IOMMU_* values are about.
> 
>  I will delete it.

Well, can you not control mapping permissions with the AP bits? The idea
of the IOMMU flags are:

  IOMMU_CACHE : Install a normal, cacheable mapping (you've got this right)
  IOMMU_READ : Allow read access for the device
  IOMMU_WRITE : Allow write access for the device
  IOMMU_NOEXEC : Disallow execute access for the device

so the caller to iommu_map passes in a bitmap of these, which you need to
encode in the page-table entry.

> > > +static int
> > > +_arm_short_map(struct arm_short_io_pgtable *data,
> > > +  unsigned int iova, phys_addr_t paddr,
> > > +  arm_short_iopte pgdprot, arm_short_iopte pteprot,
> > > +  bool large)
> > > +{
> > > +   const struct iommu_gather_ops *tlb = data->iop.cfg.tlb;
> > > +   arm_short_iopte *pgd = data->pgd, *pte;
> > > +   void *cookie = data->iop.cookie, *pte_va;
> > > +   unsigned int ptenr = large ? 16 : 1;
> > > +   int i, quirk = data->iop.cfg.quirks;
> > > +   bool ptenew = false;
> > > +
> > > +   pgd += ARM_SHORT_PGD_IDX(iova);
> > > +
> > > +   if (!pteprot) { /* section or supersection */
> > > +   if (quirk & IO_PGTABLE_QUIRK_SHORT_MTK)
> > > +   pgdprot &= ~ARM_SHORT_PGD_SECTION_XN;
> > > +   pte = pgd;
> > > +   pteprot = pgdprot;
> > > +   } else {/* page or largepage */
> > > +   if (quirk & IO_PGTABLE_QUIRK_SHORT_MTK) {
> > > +   if (large) { /* special Bit */
> >
> > This definitely needs a better comment! What exactly are you doing here
> > and what is that quirk all about?
> 
> I use this quirk is for MTK Special Bit as we don't have the XN bit in
> pagetable.

I'm still not really clear about what this is.

> > > +   if (!(*pgd)) {
> > > +   pte_va = kmem_cache_zalloc(data->ptekmem, 
> > > GFP_ATOMIC);
> > > +   if (unlikely(!pte_va))
> > > +   return -ENOMEM;
> > > +   ptenew = true;
> > > +   *pgd = virt_to_phys(pte_va) | pgdprot;
> > > +   kmemleak_ignore(pte_va);
> > > +   tlb->flush_pgtable(pgd, sizeof(*pgd), cookie);
> >
> > I think you need to flush this before it becomes visible to the walker.
> 
> I have flushed pgtable here, Do you meaning flush tlb here?

No. afaict, you allocate the pte table using kmem_cache_zalloc but you never
flush it. However, you update the pgd to point at this table, so the walker
can potentially see garbage instead of the zeroed entries.

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

Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.

[Will Deacon]

Hello,

This is looking better, but I still have some concerns.

On Thu, Jul 16, 2015 at 10:04:32AM +0100, Yong Wu wrote:
> This patch is for ARM Short Descriptor Format.
> 
> Signed-off-by: Yong Wu 
> ---
>  drivers/iommu/Kconfig|   18 +
>  drivers/iommu/Makefile   |1 +
>  drivers/iommu/io-pgtable-arm-short.c |  742 
> ++
>  drivers/iommu/io-pgtable-arm.c   |3 -
>  drivers/iommu/io-pgtable.c   |4 +
>  drivers/iommu/io-pgtable.h   |   13 +
>  6 files changed, 778 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/iommu/io-pgtable-arm-short.c

[...]

> +#define ARM_SHORT_PGDIR_SHIFT  20
> +#define ARM_SHORT_PAGE_SHIFT   12
> +#define ARM_SHORT_PTRS_PER_PTE \
> +   (1 << (ARM_SHORT_PGDIR_SHIFT - ARM_SHORT_PAGE_SHIFT))
> +#define ARM_SHORT_BYTES_PER_PTE\
> +   (ARM_SHORT_PTRS_PER_PTE * sizeof(arm_short_iopte))
> +
> +/* level 1 pagetable */
> +#define ARM_SHORT_PGD_TYPE_PGTABLE BIT(0)
> +#define ARM_SHORT_PGD_SECTION_XN   (BIT(0) | BIT(4))
> +#define ARM_SHORT_PGD_TYPE_SECTION BIT(1)
> +#define ARM_SHORT_PGD_PGTABLE_XN   BIT(2)

This should be PXN, but I'm not even sure why we care for a table at
level 1.

> +#define ARM_SHORT_PGD_BBIT(2)
> +#define ARM_SHORT_PGD_CBIT(3)
> +#define ARM_SHORT_PGD_PGTABLE_NS   BIT(3)
> +#define ARM_SHORT_PGD_IMPLEBIT(9)
> +#define ARM_SHORT_PGD_TEX0 BIT(12)
> +#define ARM_SHORT_PGD_SBIT(16)
> +#define ARM_SHORT_PGD_nG   BIT(17)
> +#define ARM_SHORT_PGD_SUPERSECTION BIT(18)
> +#define ARM_SHORT_PGD_SECTION_NS   BIT(19)
> +
> +#define ARM_SHORT_PGD_TYPE_SUPERSECTION\
> +   (ARM_SHORT_PGD_TYPE_SECTION | ARM_SHORT_PGD_SUPERSECTION)
> +#define ARM_SHORT_PGD_SECTION_TYPE_MSK \
> +   (ARM_SHORT_PGD_TYPE_SECTION | ARM_SHORT_PGD_SUPERSECTION)
> +#define ARM_SHORT_PGD_PGTABLE_TYPE_MSK \
> +   (ARM_SHORT_PGD_TYPE_SECTION | ARM_SHORT_PGD_TYPE_PGTABLE)
> +#define ARM_SHORT_PGD_TYPE_IS_PGTABLE(pgd) \
> +   (((pgd) & ARM_SHORT_PGD_PGTABLE_TYPE_MSK) == 
> ARM_SHORT_PGD_TYPE_PGTABLE)
> +#define ARM_SHORT_PGD_TYPE_IS_SECTION(pgd) \
> +   (((pgd) & ARM_SHORT_PGD_SECTION_TYPE_MSK) == 
> ARM_SHORT_PGD_TYPE_SECTION)
> +#define ARM_SHORT_PGD_TYPE_IS_SUPERSECTION(pgd)\
> +   (((pgd) & ARM_SHORT_PGD_SECTION_TYPE_MSK) == \
> +   ARM_SHORT_PGD_TYPE_SUPERSECTION)
> +#define ARM_SHORT_PGD_PGTABLE_MSK  0xfc00
> +#define ARM_SHORT_PGD_SECTION_MSK  (~(SZ_1M - 1))
> +#define ARM_SHORT_PGD_SUPERSECTION_MSK (~(SZ_16M - 1))
> +
> +/* level 2 pagetable */
> +#define ARM_SHORT_PTE_TYPE_LARGE   BIT(0)
> +#define ARM_SHORT_PTE_SMALL_XN BIT(0)
> +#define ARM_SHORT_PTE_TYPE_SMALL   BIT(1)
> +#define ARM_SHORT_PTE_BBIT(2)
> +#define ARM_SHORT_PTE_CBIT(3)
> +#define ARM_SHORT_PTE_SMALL_TEX0   BIT(6)
> +#define ARM_SHORT_PTE_IMPLEBIT(9)

This is AP[2] for small pages.

> +#define ARM_SHORT_PTE_SBIT(10)
> +#define ARM_SHORT_PTE_nG   BIT(11)
> +#define ARM_SHORT_PTE_LARGE_TEX0   BIT(12)
> +#define ARM_SHORT_PTE_LARGE_XN BIT(15)
> +#define ARM_SHORT_PTE_LARGE_MSK(~(SZ_64K - 1))
> +#define ARM_SHORT_PTE_SMALL_MSK(~(SZ_4K - 1))
> +#define ARM_SHORT_PTE_TYPE_MSK \
> +   (ARM_SHORT_PTE_TYPE_LARGE | ARM_SHORT_PTE_TYPE_SMALL)
> +#define ARM_SHORT_PTE_TYPE_IS_SMALLPAGE(pte)   \
> +   (pte) & ARM_SHORT_PTE_TYPE_MSK) >> 1) << 1)\
> +   == ARM_SHORT_PTE_TYPE_SMALL)

I see what you're trying to do here, but the shifting is confusing. I
think it's better doing something like:

((pte) & ARM_SHORT_PTE_TYPE_SMALL) == ARM_SHORT_PTE_TYPE_SMALL

or even just:

(pte) & ARM_SHORT_PTE_TYPE_SMALL

> +#define ARM_SHORT_PTE_TYPE_IS_LARGEPAGE(pte)   \
> +   (((pte) & ARM_SHORT_PTE_TYPE_MSK) == ARM_SHORT_PTE_TYPE_LARGE)
> +
> +#define ARM_SHORT_PGD_IDX(a)   ((a) >> ARM_SHORT_PGDIR_SHIFT)
> +#define ARM_SHORT_PTE_IDX(a)   \
> +   (((a) >> ARM_SHORT_PAGE_SHIFT) & (ARM_SHORT_PTRS_PER_PTE - 1))
> +
> +#define ARM_SHORT_GET_PTE_VA(pgd)  \
> +   (phys_to_virt((unsigned long)pgd & ARM_SHORT_PGD_PGTABLE_MSK))

Maybe better named as ARM_SHORT_GET_PGTABLE_VA ?

> +
> +#define ARM_SHORT_PTE_LARGE_GET_PROT(pte)  \
> +   (((pte) & (~ARM_SHORT_PTE_LARGE_MSK)) & ~ARM_SHORT_PTE_TYPE_MSK)
> +
> +#define ARM_SHORT_PGD_GET_PROT(pgd)\
> +   (((pgd) & (~ARM_SHORT_

Re: [RFC PATCH 1/4] iommu/arm-smmu: Init driver using IOMMU_OF_DECLARE

[Will Deacon]

[adding Robin]

On Fri, Jul 17, 2015 at 05:53:22PM +0100, Sricharan R wrote:
> This patch uses IOMMU_OF_DECLARE to register the driver
> and the iommu_ops. So when master devices of the iommu are
> registered, of_xlate callback can be used to add the master
> configurations to the smmu driver.

I'd really prefer to do this on top of Laurent's series reworking some of
the of_xlate code and I think Robin [CC'd] is working on that. That will
also pave the way for specifying per-master SMR configuration in the
device-tree.

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

Re: [RFC PATCH 3/4] iommu/arm-smmu: Add support for specifying clocks

[Will Deacon]

On Fri, Jul 17, 2015 at 05:53:24PM +0100, Sricharan R wrote:
> From: Mitchel Humpherys 
> 
> On some platforms with tight power constraints it is polite to only
> leave your clocks on for as long as you absolutely need them. Currently
> we assume that all clocks necessary for SMMU register access are always
> on.

You've borrowed this commit message from Mitch's previous version of
this patch, but now you leave the clocks enabled most of the time so it
doesn't make much sense anymore.

Anyway, I'm OK with this kind of clock management in the driver, but I
think that anything more fine-grained needs to be designed into the IOMMU
core.

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

Re: [PATCH v3 5/6] iommu/mediatek: Add mt8173 IOMMU driver

[Will Deacon]

Hi Yong Wu,

On Thu, Jul 16, 2015 at 10:04:34AM +0100, Yong Wu wrote:
> This patch adds support for mediatek m4u (MultiMedia Memory Management
> Unit).

[...]

> +static void mtk_iommu_tlb_flush_all(void *cookie)
> +{
> +   struct mtk_iommu_domain *domain = cookie;
> +   void __iomem *base;
> +
> +   base = domain->data->base;
> +   writel(F_INVLD_EN1 | F_INVLD_EN0, base + REG_MMU_INV_SEL);
> +   writel(F_ALL_INVLD, base + REG_MMU_INVALIDATE);

This needs to be synchronous, so you probably want to call
mtk_iommu_tlb_sync at the end.

> +}
> +
> +static void mtk_iommu_tlb_add_flush(unsigned long iova, size_t size,
> +   bool leaf, void *cookie)
> +{
> +   struct mtk_iommu_domain *domain = cookie;
> +   void __iomem *base = domain->data->base;
> +   unsigned int iova_start = iova, iova_end = iova + size - 1;
> +
> +   writel(F_INVLD_EN1 | F_INVLD_EN0, base + REG_MMU_INV_SEL);
> +
> +   writel(iova_start, base + REG_MMU_INVLD_START_A);
> +   writel(iova_end, base + REG_MMU_INVLD_END_A);
> +   writel(F_MMU_INV_RANGE, base + REG_MMU_INVALIDATE);

Why are you using writel instead of writel_relaxed? I asked this before
but I don't think you replied.

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

Re: Master-aware devices and sideband ID data

[Will Deacon]

On Thu, Jul 16, 2015 at 02:34:41PM +0100, Mark Rutland wrote:
> Hi Will,

Hi Mark,

[adding David, since he's working on PCI/ITS stuff atm]

> The below is an attempt at an MSI binding, derived from my original
> example. It extends msi-parent inoto a phandle+(optional args) style
> property.
> 
> I haven't yet managed to come up with a sane way of describing
> the Bus-ID/BDF -> {iommu,msi}-cells translation, but this should
> hopefully cover the platform device case.
> 
> For the Bus-ID translation case I'm not sure it's sane to attempt to use
> msi-parent, given that the transformation description will necessarily
> have to describe the parents anyway.

We probably want a separate property on the RC node describing the
transformation (i.e. offsetting) in a similar way to ranges or
interrupt-map (potentially replacing msi-parent altogether). Maybe you
could propose something so that David could have a crack at describing
his RequesterID -> DeviceID mapping, which seems to map nicely onto a
per-RC offset IIUC.

David -- does that sound ok to you?

Will

> >8
> From 429dca4bba98732c492e95bdf395aa2ccc634e69 Mon Sep 17 00:00:00 2001
> From: Mark Rutland 
> Date: Thu, 9 Jul 2015 17:53:00 +0100
> Subject: [PATCH] Documentation: dt: add generic MSI bindings
> 
> Currently msi-parent is in use in a couple of drviers despite being
> fairly underspecified. This patch adds a generic binding for MSIs
> (including the existing msi-parent property) enabling the description of
> platform devices capable of using MSIs.
> 
> This binding does not yet cover the general case. Currently the binding
> does not cover the relationship between bus IDs (e.g. PCIe BDF) and
> sideband data.
> 
> Signed-off-by: Mark Rutland 
> ---
>  .../bindings/interrupt-controller/msi.txt  | 135 
> +
>  1 file changed, 135 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/interrupt-controller/msi.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/msi.txt 
> b/Documentation/devicetree/bindings/interrupt-controller/msi.txt
> new file mode 100644
> index 000..c60c034
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/msi.txt
> @@ -0,0 +1,135 @@
> +This document describes the generic device tree binding for MSI controllers 
> and
> +their master(s).
> +
> +Message Signaled Interrupts (MSIs) are a class of interrupts generated by a
> +write to an MMIO address.
> +
> +MSIs were originally specified by PCI (and are used with PCIe), but may also 
> be
> +used with other busses, and hence a mechanism is required to relate devices 
> on
> +those busses to the MSI controllers which they are capable of using,
> +potentially including additional information.
> +
> +MSIs are distinguished by some combination of:
> +
> +- The doorbell (the MMIO address written to).
> +  
> +  Devices may be configured by software to write to arbitrary doorbells which
> +  they can address. An MSI controller may feature a number of doorbells.
> +
> +- The payload (the value written to the doorbell).
> +  
> +  Devices may be configured to write an arbitrary payload chosen by software.
> +  MSI controllers may have restrictions on permitted payloads.
> +
> +- Sideband information accompanying the write.
> +  
> +  Typically this is neither configurable nor probeable, and depends on the 
> path
> +  taken through the memory system (i.e. it is a property of the combination 
> of
> +  MSI controller and device rather than a property of either in isolation).
> +
> +
> +MSI controllers:
> +
> +
> +An MSI controller signals interrupts to a CPU when a write is made to an MMIO
> +address by some master. An MSI controller may feature a number of doorbells.
> +
> +Required properties:
> +
> +
> +- msi-controller: Identifies the node as an MSI controller.
> +
> +Optional properties:
> +
> +
> +- #msi-cells: The number of cells in an msi-specifier, required if not zero.
> +
> +  Typically this will encode information related to sideband data, and will
> +  not encode doorbells or payloads as these can be configured dynamically.
> +
> +  The meaning of the msi-specifier is defined by the device tree binding of
> +  the specific MSI controller. 
> +
> +
> +MSI clients
> +===
> +
> +MSI clients are devices which generate MSIs. For each MSI they wish to
> +generate, the doorbell and payload may be configured, though sideband
> +information may not be configurable.
> +
> +Required properties:
> +
> +
> +- msi-parent: A list of phandle + msi-specifier pairs, one for each MSI
> +  controller which the device is capable of using.
> +
> +  This property is unordered, and MSIs may be allocated from any combination 
> of
> +  MSI controllers listed in the msi-parent property.
> +
> +  If a device has restrictions on the allocation of MSIs, these restrictions
> +  must be described with additional properti

Re: [PATCH 1/2] of: base: Allow more args than MAX_PHANDLE_ARGS if required

[Will Deacon]

On Thu, Jul 16, 2015 at 09:30:43AM +0100, Joerg Roedel wrote:
> From: Joerg Roedel 
> 
> The main use of MAX_PHANDLE_ARGS is to define the number of
> args elements in 'struct of_phandle_args'. This struct is
> often declared on the stack and thus it is impractical to
> increase MAX_PHANDLE_ARGS again and again.
> 
> To handle situations where more than MAX_PHANDLE_ARGS
> elements may appear in a device-tree, introduce functions
> to allocate/free 'struct of_phandle_args' with more than
> MAX_PHANDLE_ARGS elements and provide the new function
> of_parse_phandle_with_var_args(), which can handle those
> variable-size structs.
> 
> This is necessary for the ARM-SMMU driver, where the number
> of mmu-masters can be up to 128.
> 
> Signed-off-by: Joerg Roedel 
> ---
>  drivers/of/base.c  | 43 ---
>  include/linux/of.h |  7 +++
>  2 files changed, 43 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 8b5a187..2b288db 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -54,6 +54,24 @@ DEFINE_MUTEX(of_mutex);
>   */
>  DEFINE_RAW_SPINLOCK(devtree_lock);
>  
> +struct of_phandle_args *of_alloc_phandle_args(int size)
> +{
> + struct of_phandle_args *args;
> + int e = max(0, size - MAX_PHANDLE_ARGS);
> +
> + args =  kzalloc(sizeof(struct of_phandle_args) + e * sizeof(uint32_t),
> + GFP_KERNEL);

Should you also update args->args_count to reflect the extended array?

That said, extending the fixed-size array member like this feels a bit
fragile. Does GCC not complain about out-of-bounds accesses if you
statically address args->args[MAX_PHANDLE_ARGS]? Admittedly, I can't
think *why* this would be break (things like additional padding will be
harmless), but I'm not intimate with the C standard.

I guess the more worrying possibility is if somebody adds a new member to
the end of of_phandle_args.

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

Re: [PATCH 2/2] arm/smmu: Make use of of_parse_phandle_with_var_args

[Will Deacon]

Hi Joerg,

On Thu, Jul 16, 2015 at 09:30:44AM +0100, Joerg Roedel wrote:
> From: Joerg Roedel 
> 
> The function of_parse_phandle_with_args() can only handle 16
> args, but there are systems that require more (25 in my
> case). So use the newly introduced function
> of_parse_phandle_with_var_args() instead.

Looks good to me:

  Acked-by: Will Deacon 

Thanks!

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

Re: Master-aware devices and sideband ID data

[Will Deacon]

On Wed, Jul 08, 2015 at 02:30:50PM +0100, Mark Rutland wrote:
> On Tue, Jun 09, 2015 at 11:17:54AM +0100, Mark Rutland wrote:
> > On Fri, Jun 05, 2015 at 10:05:34AM +0100, Will Deacon wrote:
> > > Mark: how do you see this co-existing/merging with the current bindings?
> > 
> > As I mentioned in my initial mail, it's not clear to me how this can be
> > reconciled with the current bindings. Everything I've been able to come
> > up with so far at best ends up describing the same thing repeatedly.
> > 
> > I'll see what I can come up with. Any sugestions are welcome!
> 
> I can't see a way of keeping the ID transformations explicit with the
> existing bindings, but I think we can simply fold these down into
> properties in the master nodes, given we expect each ID to be derived
> from some initial master ID anyway.
> 
> So, to cater for the ITS we would need to pass master IDs along with the
> MSI parent information, which we could do by extending msi-parent or by
> introducing a new msis property which behaves similarly to the iommus
> property, describing the MSI controllers the device can address (via any
> IOMMUs), along with any controller-specific identification data.
> 
> Which means we'd have DT fragments like the following for an arbitrary
> platform device:
> 
> its0: its {
>   ...
>   msi-controller;
>   #msi-cells = <1>; // DeviceId
> };
> 
> its1: its {
>   ...
>   msi-controller;
>   #msi-cells = <1>; // DeviceId
> };
> 
> smmu: smmu {
>   ...
>   iommu-cells = <1>; // StreamId
> };
> 
> device {
>   ...
>   iommus = <&its 0>;
>   /* Can use either ITS, but has a different ID at each */
>   msis = <&its0 0x0>, <&its1 0x400>;
> };
> 
> That doesn't allow you to describe a device with multiple mater ports
> where each master port might want to generate MSIs, but I'm not sure if
> that's a real case.

In this case, I think we'd need something extra to define precisely how
those master ports relate to the rest of the system anyway. That would
likely be a device-specific property, I reckon.

> For PCIe root complexes, we'd need to describe the BDF -> iommu-cells
> and BDF -> msi-cells translations separately with new properties on the
> node for the root complex itself.
> 
> Is there anything obviously broken with the above approach?

Works for me. Can you write this up as a binding extension to msi-parent,
please?

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

[PATCH] ARM: perf: extend interrupt-affinity property for PPIs

[Will Deacon]

On systems containing multiple, heterogeneous clusters we need a way to
associate a PMU "device" with the CPU(s) on which it exists. For PMUs
that signal overflow with SPIs, this relationship is determined via the
"interrupt-affinity" property, which contains a list of phandles to CPU
nodes for the PMU. For PMUs using PPIs, the per-cpu nature of the
interrupt isn't enough to determine the set of CPUs which actually
contain the device.

This patch allows the interrupt-affinity property to be specified on a
PMU node irrespective of the interrupt type. For PPIs, it identifies
the set of CPUs signalling the PPI in question.

Tested-by: Stephen Boyd  # Krait PMU
Signed-off-by: Will Deacon 
---
 Documentation/devicetree/bindings/arm/pmu.txt | 12 +++--
 arch/arm/kernel/perf_event.c  | 65 ++-
 2 files changed, 53 insertions(+), 24 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/pmu.txt 
b/Documentation/devicetree/bindings/arm/pmu.txt
index 3b5f5d1088c6..435251fa9ce0 100644
--- a/Documentation/devicetree/bindings/arm/pmu.txt
+++ b/Documentation/devicetree/bindings/arm/pmu.txt
@@ -26,13 +26,19 @@ Required properties:
 
 Optional properties:
 
-- interrupt-affinity : Valid only when using SPIs, specifies a list of phandles
-   to CPU nodes corresponding directly to the affinity of
+- interrupt-affinity : When using SPIs, specifies a list of phandles to CPU
+   nodes corresponding directly to the affinity of
   the SPIs listed in the interrupts property.
 
-  This property should be present when there is more than
+   When using a PPI, specifies a list of phandles to CPU
+  nodes corresponding to the set of CPUs which have
+  a PMU of this type signalling the PPI listed in the
+  interrupts property.
+
+   This property should be present when there is more than
   a single SPI.
 
+
 - qcom,no-pc-write : Indicates that this PMU doesn't support the 0xc and 0xd
  events.
 
diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
index 7d5379c1c443..5a8f17bfcc60 100644
--- a/arch/arm/kernel/perf_event.c
+++ b/arch/arm/kernel/perf_event.c
@@ -790,32 +790,39 @@ static int probe_current_pmu(struct arm_pmu *pmu,
 
 static int of_pmu_irq_cfg(struct arm_pmu *pmu)
 {
-   int i, irq, *irqs;
+   int *irqs, i = 0;
+   bool using_spi = false;
struct platform_device *pdev = pmu->plat_device;
 
-   /* Don't bother with PPIs; they're already affine */
-   irq = platform_get_irq(pdev, 0);
-   if (irq >= 0 && irq_is_percpu(irq)) {
-   cpumask_setall(&pmu->supported_cpus);
-   return 0;
-   }
-
irqs = kcalloc(pdev->num_resources, sizeof(*irqs), GFP_KERNEL);
if (!irqs)
return -ENOMEM;
 
-   for (i = 0; i < pdev->num_resources; ++i) {
+   do {
struct device_node *dn;
-   int cpu;
+   int cpu, irq;
 
-   dn = of_parse_phandle(pdev->dev.of_node, "interrupt-affinity",
- i);
-   if (!dn) {
-   pr_warn("Failed to parse %s/interrupt-affinity[%d]\n",
-   of_node_full_name(pdev->dev.of_node), i);
+   /* See if we have an affinity entry */
+   dn = of_parse_phandle(pdev->dev.of_node, "interrupt-affinity", 
i);
+   if (!dn)
break;
+
+   /* Check the IRQ type and prohibit a mix of PPIs and SPIs */
+   irq = platform_get_irq(pdev, i);
+   if (irq >= 0) {
+   bool spi = !irq_is_percpu(irq);
+
+   if (i > 0 && spi != using_spi) {
+   pr_err("PPI/SPI IRQ type mismatch for %s!\n",
+   dn->name);
+   kfree(irqs);
+   return -EINVAL;
+   }
+
+   using_spi = spi;
}
 
+   /* Now look up the logical CPU number */
for_each_possible_cpu(cpu)
if (arch_find_n_match_cpu_physical_id(dn, cpu, NULL))
break;
@@ -824,20 +831,36 @@ static int of_pmu_irq_cfg(struct arm_pmu *pmu)
pr_warn("Failed to find logical CPU for %s\n",
dn->name);
of_node_put(dn);
+   cpumask_setall(&pmu->supported_cpus);
break;
}
of_node_put(dn);
 
-  

Re: [RFC PATCH v4 0/4] arm64:numa: Add numa support for arm64 platforms.

[Will Deacon]

On Mon, Jun 29, 2015 at 11:57:49AM +0100, Ganapatrao Kulkarni wrote:
> Hi Catalin, Will, Mark,
> 
> can you please have a look of numa patches and share your review comments.

Nothing has changed since the last time you poked me about this series:

  - There are open comments from Hanjun
  - The device-tree part hasn't been reviewed by a DT maintainer

so I can't merge anything other than the Kconfig change atm.

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

Re: [PATCH 2/2] arm/smmu: Make use of of_parse_phandle_with_var_args

[Will Deacon]

Hi Joerg,

Thanks for looking at this! I'm fine with the general idea, but obviously
the first patch needs an Ack from a devicetree person.

One comment on the code below...

On Thu, Jun 25, 2015 at 04:52:28PM +0100, Joerg Roedel wrote:
> The function of_parse_phandle_with_args() can only handle 16
> args, but there are systems that require more (25 in my
> case). So use the newly introduced function
> of_parse_phandle_with_var_args() instead.

[...]

> + masterspec = of_alloc_phandle_args(MAX_MASTER_STREAMIDS);
> + if (!masterspec)
> + return -ENOMEM;
> +
>   i = 0;
>   smmu->masters = RB_ROOT;
> - while (!of_parse_phandle_with_args(dev->of_node, "mmu-masters",
> -"#stream-id-cells", i,
> -&masterspec)) {
> - err = register_smmu_master(smmu, dev, &masterspec);
> + while (!of_parse_phandle_with_var_args(dev->of_node, "mmu-masters",
> +"#stream-id-cells", i, masterspec,
> +MAX_MASTER_STREAMIDS)) {
> + err = register_smmu_master(smmu, dev, masterspec);
>   if (err) {
>   dev_err(dev, "failed to add master %s\n",
> - masterspec.np->name);
> + masterspec->np->name);
>   goto out_put_masters;
>   }
>  
> @@ -1811,6 +1815,9 @@ out_free_irqs:
>   free_irq(smmu->irqs[i], smmu);
>  
>  out_put_masters:
> +
> + of_free_phandle_args(masterspec);

Shouldn't we also free the masterspec on success?

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

Re: [PATCH] arm64: dts: symlink cros-ec-keyboard from arm to arm64

[Will Deacon]

On Tue, Jun 16, 2015 at 03:35:41PM +0100, Daniel Kurtz wrote:
> The cros-ec-keyboard.dtsi snippet is useful for both arm and arm64 boards.
> Create a link between the two.
> 
> This may not be the most scalable solution, so consider it temporary until
> we find a more central repository for such shared .dtsi snippets.

I don't have strong opinions either way, but we should be consistent as
to whether we use relative paths:

  http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/350267.html

or symlinks, like below.

Does anybody have some technical arguments one way or the other?

Will

> Signed-off-by: Daniel Kurtz 
> ---
>  arch/arm64/boot/dts/cros-ec-keyboard.dtsi | 1 +
>  1 file changed, 1 insertion(+)
>  create mode 12 arch/arm64/boot/dts/cros-ec-keyboard.dtsi
> 
> diff --git a/arch/arm64/boot/dts/cros-ec-keyboard.dtsi 
> b/arch/arm64/boot/dts/cros-ec-keyboard.dtsi
> new file mode 12
> index 000..42220ac
> --- /dev/null
> +++ b/arch/arm64/boot/dts/cros-ec-keyboard.dtsi
> @@ -0,0 +1 @@
> +../../../arm/boot/dts/cros-ec-keyboard.dtsi
> \ No newline at end of file
> -- 
> 2.2.0.rc0.207.ga3a616c
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Master-aware devices and sideband ID data

[Will Deacon]

On Tue, Jun 09, 2015 at 11:17:54AM +0100, Mark Rutland wrote:
> On Fri, Jun 05, 2015 at 10:05:34AM +0100, Will Deacon wrote:
> > On Thu, Jun 04, 2015 at 11:19:30PM +0100, Chalamarla, Tirumalesh wrote:
> > > > On Jun 1, 2015, at 3:22 AM, Mark Rutland  wrote:
> > > > It's possible to specify that the paths exist. I expect that software
> > > > would select which to use at runtime.
> > > > 
> > > My worry is how to define any priorities/preferences between masters. 
> > > in general the proposal looks reasonable.
> > 
> > I agree that the proposal looks reasonable (in terms of the ability to
> > describe the sort of topologies that we will face) but I still don't
> > understand what I need to do in e.g. my IOMMU driver to support this
> > binding whilst continuing to support the existing iommus binding, which
> > is relied upon to configure dma-mapping.
> > 
> > Mark: how do you see this co-existing/merging with the current bindings?
> 
> As I mentioned in my initial mail, it's not clear to me how this can be
> reconciled with the current bindings. Everything I've been able to come
> up with so far at best ends up describing the same thing repeatedly.
> 
> I'll see what I can come up with. Any sugestions are welcome!

Well, I'd start with the current iommu binding and see if you can figure
out which properties need adding to it in order to describe the sort of
topologies you can describe with your original proposal.

> > I don't think it's practical to throw away what we have and move over to
> > something totally different all in one go, but there clearly *is* benefit
> > in your proposal over the existing scheme.
> 
> I can see that's probably not practical. :(
> 
> Do we know what we're going to do w.r.t. IORT? That's going to require
> the kernel to be able to handle a similar description to this proposal.

IORT is part of ACPI, so it doesn't care about the existing DT binding
and doesn't need to co-exist at runtime.

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

Re: [PATCH v2 5/6] iommu/mediatek: Add mt8173 IOMMU driver

[Will Deacon]

On Fri, May 15, 2015 at 10:43:28AM +0100, Yong Wu wrote:
> This patch adds support for mediatek m4u (MultiMedia Memory Management Unit).

After looking at the page table code, I thought I'd come and check your
TLB invalidate code here.

> +static void mtk_iommu_tlb_flush_all(void *cookie)
> +{
> +   struct mtk_iommu_domain *domain = cookie;
> +   u32 val;
> +
> +   val = F_INVLD_EN1 | F_INVLD_EN0;
> +   writel(val, domain->imuinfo->base + REG_MMU_INV_SEL);
> +   writel(F_ALL_INVLD, domain->imuinfo->base + REG_MMU_INVALIDATE);
> +}
> +
> +static void mtk_iommu_tlb_add_flush(unsigned long iova, size_t size,
> +   bool leaf, void *cookie)
> +{
> +   struct mtk_iommu_domain *domain = cookie;
> +   void __iomem *m4u_base = domain->imuinfo->base;
> +   unsigned int iova_start = iova, iova_end = iova + size - 1;
> +   int ret;
> +   u32 val;
> +
> +   val = F_INVLD_EN1 | F_INVLD_EN0;
> +   writel(val, m4u_base + REG_MMU_INV_SEL);
> +
> +   writel(iova_start, m4u_base + REG_MMU_INVLD_START_A);
> +   writel(iova_end, m4u_base + REG_MMU_INVLD_END_A);
> +   writel(F_MMU_INV_RANGE, m4u_base + REG_MMU_INVALIDATE);
> +
> +   ret = readl_poll_timeout_atomic(m4u_base + REG_MMU_CPE_DONE, val,
> +   val != 0, 10, 100);
> +   if (ret) {
> +   dev_warn(domain->imuinfo->dev, "Invalid tlb don't done\n");
> +   mtk_iommu_tlb_flush_all(cookie);
> +   }
> +   writel(0, m4u_base + REG_MMU_CPE_DONE);
> +}

You don't need to wait for completion here if you can implement a proper
->tlb_sync callback.

> +static void mtk_iommu_flush_pgtable(void *ptr, size_t size, void *cookie)
> +{
> +   /*
> +* After delete arch_setup_dma_ops,
> +* This will be replaced with dma_map_page
> +*/
> +__dma_flush_range(ptr, ptr + size);
> +}

This should give you the necessary barriers to ensure visibility of the
updated page tables, so you can use the _relaxed io accessors for the
other TLB functions.

> +static struct iommu_gather_ops mtk_iommu_gather_ops = {
> +   .tlb_flush_all = mtk_iommu_tlb_flush_all,
> +   .tlb_add_flush = mtk_iommu_tlb_add_flush,
> +   .tlb_sync = mtk_iommu_tlb_flush_all,

sync isn't required to flush anything; it's just supposed to wait for
any outstanding invalidation (i.e. from tlb_add_flush) to complete.

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

Re: [PATCH v2 3/6] iommu: add ARM short descriptor page table allocator.

[Will Deacon]

Hello,

Thanks for the patch, it's good to see another user of the generic
IO page-table code. However, I have quite a lot of comments on the code.

On Fri, May 15, 2015 at 10:43:26AM +0100, Yong Wu wrote:
> This patch is for ARM Short Descriptor Format.It has 2-levels
> pagetable and the allocator supports 4K/64K/1M/16M.
>
> Signed-off-by: Yong Wu 
> ---
>  drivers/iommu/Kconfig|   7 +
>  drivers/iommu/Makefile   |   1 +
>  drivers/iommu/io-pgtable-arm-short.c | 490 
> +++
>  drivers/iommu/io-pgtable.c   |   4 +
>  drivers/iommu/io-pgtable.h   |   6 +
>  5 files changed, 508 insertions(+)
>  create mode 100644 drivers/iommu/io-pgtable-arm-short.c

For some reason, I ended up reviewing this back-to-front (i.e. starting
with the init code), so apologies if the comments feel like they were
written in reverse.

> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index 1ae4e54..3d2eac6 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -39,6 +39,13 @@ config IOMMU_IO_PGTABLE_LPAE_SELFTEST
>
>   If unsure, say N here.
>
> +config IOMMU_IO_PGTABLE_SHORT
> +   bool "ARMv7/v8 Short Descriptor Format"
> +   select IOMMU_IO_PGTABLE

depends on ARM || ARM64 || COMPILE_TEST

> +   help
> + Enable support for the ARM Short descriptor pagetable format.
> + It has 2-levels pagetable and The allocator supports 4K/64K/1M/16M.

The second sentence is worded rather strangely.

> +
>  endmenu
>
>  config IOMMU_IOVA
> diff --git a/drivers/iommu/io-pgtable-arm-short.c 
> b/drivers/iommu/io-pgtable-arm-short.c
> new file mode 100644
> index 000..cc286ce5
> --- /dev/null
> +++ b/drivers/iommu/io-pgtable-arm-short.c
> @@ -0,0 +1,490 @@
> +/*
> + * Copyright (c) 2014-2015 MediaTek Inc.
> + * Author: Yong Wu 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +#define pr_fmt(fmt)"arm-short-desc io-pgtable: "fmt
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "io-pgtable.h"
> +
> +typedef u32 arm_short_iopte;
> +
> +struct arm_short_io_pgtable {
> +   struct io_pgtable   iop;
> +   struct kmem_cache   *ptekmem;
> +   size_t  pgd_size;
> +   void*pgd;
> +};
> +
> +#define io_pgtable_short_to_data(x)\
> +   container_of((x), struct arm_short_io_pgtable, iop)
> +
> +#define io_pgtable_ops_to_pgtable(x)   \
> +   container_of((x), struct io_pgtable, ops)
> +
> +#define io_pgtable_short_ops_to_data(x)\
> +   io_pgtable_short_to_data(io_pgtable_ops_to_pgtable(x))
> +

These are private macros, so I think you can drop the "short" part to,
err, keep them short.

> +#define ARM_SHORT_MAX_ADDR_BITS32
> +
> +#define ARM_SHORT_PGDIR_SHIFT  20
> +#define ARM_SHORT_PAGE_SHIFT   12
> +#define ARM_SHORT_PTRS_PER_PTE 256
> +#define ARM_SHORT_BYTES_PER_PTE1024

Isn't that ARM_SHORT_PTRS_PER_PTE * sizeof(arm_short_iopte)?

> +/* 1 level pagetable */
> +#define ARM_SHORT_F_PGD_TYPE_PAGE  (0x1)

I think you're using PAGE and PGTABLE interchangeably, which is really
confusing to read.

> +#define ARM_SHORT_F_PGD_TYPE_PAGE_MSK  (0x3)

This is the TYPE mask.

> +#define ARM_SHORT_F_PGD_TYPE_SECTION   (0x2)
> +#define ARM_SHORT_F_PGD_TYPE_SUPERSECTION  (0x2 | (1 << 18))

Are you sure this is correct? afaict, bit 0 is PXN, so you should actually
be using bit 18 to distinguihs sections and supersections.

> +#define ARM_SHORT_F_PGD_TYPE_SECTION_MSK   (0x3 | (1 << 18))
> +#define ARM_SHORT_F_PGD_TYPE_IS_PAGE(pgd)  (((pgd) & 0x3) == 1)

Use your TYPE mask here.

> +#define ARM_SHORT_F_PGD_TYPE_IS_SECTION(pgd)   \
> +   (((pgd) & ARM_SHORT_F_PGD_TYPE_SECTION_MSK) \
> +   == ARM_SHORT_F_PGD_TYPE_SECTION)
> +#define ARM_SHORT_F_PGD_TYPE_IS_SUPERSECTION(pgd)  \
> +   (((pgd) & ARM_SHORT_F_PGD_TYPE_SECTION_MSK) \
> +   == ARM_SHORT_F_PGD_TYPE_SUPERSECTION)
> +
> +#define ARM_SHORT_F_PGD_B_BIT  BIT(2)
> +#define ARM_SHORT_F_PGD_C_BIT  BIT(3)
> +#define ARM_SHORT_F_PGD_IMPLE_BIT  BIT(9)
> +#define ARM_SHORT_F_PGD_S_BIT  BIT(16)
> +#define ARM_SHORT_F_PGD_NG_BIT BIT(17)
> +#define ARM_SHORT_F_PGD_NS_BIT_PAGEBIT(3)
> +#define ARM_SHORT_F_PGD_NS_BIT_SECTION   

Re: Master-aware devices and sideband ID data

[Will Deacon]

On Thu, Jun 04, 2015 at 11:19:30PM +0100, Chalamarla, Tirumalesh wrote:
> > On Jun 1, 2015, at 3:22 AM, Mark Rutland  wrote:
> > It's possible to specify that the paths exist. I expect that software
> > would select which to use at runtime.
> > 
> My worry is how to define any priorities/preferences between masters. 
> in general the proposal looks reasonable.

I agree that the proposal looks reasonable (in terms of the ability to
describe the sort of topologies that we will face) but I still don't
understand what I need to do in e.g. my IOMMU driver to support this
binding whilst continuing to support the existing iommus binding, which
is relied upon to configure dma-mapping.

Mark: how do you see this co-existing/merging with the current bindings?
I don't think it's practical to throw away what we have and move over to
something totally different all in one go, but there clearly *is* benefit
in your proposal over the existing scheme.

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

Re: [PATCH 3/6] drivers: firmware: psci: move power_state handling to generic code

[Will Deacon]

On Fri, May 29, 2015 at 01:16:36PM +0100, Lorenzo Pieralisi wrote:
> Functions implemented on arm64 to check if a power_state parameter
> is valid and if the power_state implies context loss are not
> arm64 specific and should be moved to generic code so that they
> can be reused on arm systems too.
> 
> This patch moves the functions handling the power_state parameter
> to generic PSCI firmware layer code.
> 
> Signed-off-by: Lorenzo Pieralisi 
> Acked-by: Sudeep Holla 
> Cc: Will Deacon 
> Cc: Catalin Marinas 
> Cc: Mark Rutland 
> ---
>  arch/arm64/kernel/psci.c | 14 --
>  drivers/firmware/psci.c  | 15 +++
>  include/linux/psci.h |  2 ++
>  3 files changed, 17 insertions(+), 14 deletions(-)

Looks fine to me:

  Acked-by: Will Deacon 

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

Re: [PATCH v3 6/6] ACPI: import watchdog info of GTDT into platform device

[Will Deacon]

On Tue, May 26, 2015 at 05:27:33PM +0100, Fu Wei wrote:
> On 26 May 2015 at 23:36, Guenter Roeck  wrote:
> > On Tue, May 26, 2015 at 04:18:42PM +0100, Will Deacon wrote:
> >> Sure, the device it describes may only ever exist on ARM systems, but by
> >> that logic then we should be moving lots of drivers back under 
> >> arch/arm[64].
> >>
> > It is nt the driver, but its instantiation. The question here would be
> > how and where to instantiate the driver, not where the driver itself
> > is located. The driver itself is ACPI agnostic.
> 
> I really don't mind to refactor the code, If we can make this patch better.
> 
> But for now, I can't see the good reason to move ACPI-relevant code
> into a watchdog driver.

I don't really mind where you move it, just as long as it's outside of
arch/arm64.

> The reasons I put the code here are
> (1)SBSA watchdog only for ARM64
> (2)GTDT only for ARM, design for ARM,
> (3)For ARM Architecture, only ARM64 support ACPI.
> 
> For minimizing arch/arm64/kernel/acpi.c, we can't put the code here,
> and we had better keep these code outside the driver,
> 
> So do you have any suggestion for the better location of the GTDT code?

I don't understand why you can't do the same as
drivers/clocksource/arm_arch_timer.c and parse the table directly in the
driver. If there are objections from the driver/subsystem maintainers then
it sounds like we need a mechanical ACPI table -> platform device
conversion in the core, like we have for device-tree.

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

Re: [PATCH v3 6/6] ACPI: import watchdog info of GTDT into platform device

[Will Deacon]

On Tue, May 26, 2015 at 04:02:56PM +0100, Ashwin Chaugule wrote:
> On 26 May 2015 at 08:28, Will Deacon  wrote:
> > On Mon, May 25, 2015 at 11:03:13AM +0100, fu@linaro.org wrote:
> >> From: Fu Wei 
> >>
> >> Parse SBSA Generic Watchdog Structure in GTDT table of ACPI,
> >> and create a platform device with that information.
> >> This platform device can be used by the ARM SBSA Generic
> >> Watchdog driver.
> >>
> >> Tested-by: Suravee Suthikulpanit 
> >> Tested-by: Timur Tabi 
> >> Signed-off-by: Fu Wei 
> >> ---
> >>  arch/arm64/kernel/acpi.c | 145 
> >> +++
> >>  1 file changed, 145 insertions(+)
> >
> > Why does this all need to be under arch/arm64? The GTDT really isn't
> > architecture-specific, so I'd *much* rather it was parsed in the driver code
> > itself, like we already do for the architected timer. The GIC is an
> > exception because it's in the MADT, which we need to parse in the arch code
> > to configure SMP properly.
> 
> I'm not really against refactoring the code. But the GTDT looks quite
> specific to ARM..
> 
> ---8<
> 5.2.24 Generic Timer Description Table (GTDT)
> This section describes the format of the Generic Timer Description
> Table (GTDT), which provides
> OSPM with information about a system’s Generic Timers configuration.
> The Generic Timer (GT) is
> a standard timer interface implemented on ARM processor-based systems.
> The GT hardware
> specification can be found at Links to ACPI-Related Documents
> (http://uefi.org/acpi) under the
> heading ARM Architecture. The GTDT provides OSPM with information
> about a system's GT
> interrupt configurations, for both per-processor timers, and platform
> (memory-mapped) timers.
> The GT specification defines the following per-processor timers:
> • Secure privilege level 1 (EL1) timer,
> • Non-Secure EL1 timer,
> • Non-Secure privilege level 2 (EL2) timer,
> • Virtual timer,
> and the following Platform (memory-mapped) timers.
> • GT Block
> • Server Base System Architecture (SBSA) Generic Watchdog
> ---8<

Sure, the device it describes may only ever exist on ARM systems, but by
that logic then we should be moving lots of drivers back under arch/arm[64].

The ARM architecture says precisely *nothing* about ACPI, so we should
try to keep arch/arm64/kernel/acpi.c to a minimum and not shovel all sorts
of table conversion code in there for random peripherals.

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

Re: [PATCH v3 6/6] ACPI: import watchdog info of GTDT into platform device

[Will Deacon]

On Mon, May 25, 2015 at 11:03:13AM +0100, fu@linaro.org wrote:
> From: Fu Wei 
> 
> Parse SBSA Generic Watchdog Structure in GTDT table of ACPI,
> and create a platform device with that information.
> This platform device can be used by the ARM SBSA Generic
> Watchdog driver.
> 
> Tested-by: Suravee Suthikulpanit 
> Tested-by: Timur Tabi 
> Signed-off-by: Fu Wei 
> ---
>  arch/arm64/kernel/acpi.c | 145 
> +++
>  1 file changed, 145 insertions(+)

Why does this all need to be under arch/arm64? The GTDT really isn't
architecture-specific, so I'd *much* rather it was parsed in the driver code
itself, like we already do for the architected timer. The GIC is an
exception because it's in the MADT, which we need to parse in the arch code
to configure SMP properly.

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

Re: [PATCH 0/2] Add support for PL172 memory-controller

[Will Deacon]

Hi Joachim,

On Tue, Apr 28, 2015 at 06:24:37PM +0100, Joachim Eastwood wrote:
> This patch set adds support for setting up static memory devices on
> a ARM PrimeCell MultiPort Memory Controller (PL172) from DT. Dynamic
> memory (SDRAM) is not supported in this patch set.
> 
> DT bindings for PL172 is based on the bindings for the TI AEMIF
> memory controller. PL172 can be found on a number of NXP devices
> like the LPC18xx family.
> 
> Tested on Embedded Artists' LPC4357 Developer's Kit with NOR Flash
> (SST39VF320) and 74LCV16374 (gpio-74x164) on MPMC.

Please excuse my ignorance here, but is this memory controller used to
manage the memory in use by the kernel? If so, how is it safe to reconfigure
it dynamically at runtime? If not, then it might be worth describing
some typical use-cases in the binding documentation to motivate the
existence of this driver.

Cheers,

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

Re: [RFC PATCH v4 3/4] arm64:thunder: Add initial dts for Cavium's Thunder SoC in 2 Node topology.

[Will Deacon]

On Wed, Mar 18, 2015 at 10:39:43AM +, Ganapatrao Kulkarni wrote:
> On Wed, Mar 18, 2015 at 3:30 PM, Will Deacon  wrote:
> > On Wed, Mar 18, 2015 at 04:02:53AM +, Ganapatrao Kulkarni wrote:
> >> Please suggest how we go about these patches.
> >> please share your review comments, if any changes needs to be done.
> >
> > I can take the Kconfig patch if you send a version that applies cleanly.
> > As for the rest, you need an ack from a DT maintainer on the bindings
> > before anything else can get merged.
> i will send you Kconfig patch (i.e increase NR_CPUS range to 2-4096)
> separately after rebasing to
> linux-next, is that ok?

Just base against -rc4 and I'll fix any conflicts that arise.

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

Re: [RFC PATCH v4 3/4] arm64:thunder: Add initial dts for Cavium's Thunder SoC in 2 Node topology.

[Will Deacon]

On Wed, Mar 18, 2015 at 04:02:53AM +, Ganapatrao Kulkarni wrote:
> Hi Catalin, Will, Mark, Arnd

[...]

> Please suggest how we go about these patches.
> please share your review comments, if any changes needs to be done.

I can take the Kconfig patch if you send a version that applies cleanly.
As for the rest, you need an ack from a DT maintainer on the bindings
before anything else can get merged.

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

Re: [PATCHv3 0/5] arm-cci400: PMU monitoring support on ARM64

[Will Deacon]

On Tue, Mar 10, 2015 at 03:18:50PM +, Suzuki K. Poulose wrote:
> From: "Suzuki K. Poulose" 
> 
> This series enables the PMU monitoring support for CCI400 on ARM64.
> The existing CCI400 driver code is a mix of PMU driver and the MCPM
> driver code. The MCPM driver is only used on ARM(32) and contains
> arm32 assembly and hence can't be built on ARM64. This patch splits
> the code to
> 
>  - ARM_CCI400_PORT_CTRL driver - depends on ARM && V7
>  - ARM_CCI400_PMU driver

If you repost this with acks added and my feedback addressed, then I'm
happy to put together a branch for arm-soc along with your other CCI PMU
fix for event validation.

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

Re: [PATCH 5/5] arm-cci: Fix CCI PMU event validation

[Will Deacon]

On Tue, Mar 10, 2015 at 03:18:55PM +, Suzuki K. Poulose wrote:
> From: "Suzuki K. Poulose" 
> 
> We mask the event with the CCI_PMU_EVENT_MASK, before passing
> the config to pmu_validate_hw_event(), which causes extra bits
> to be ignored and qualifies an invalid event code as valid.
> 
> e.g,
>  $ perf stat -a -C 0 -e CCI_400/config=0x1ff,name=cycles/ sleep 1
>Performance counter stats for 'system wide':
> 
>  506951142  cycles
> 
>1.013879626 seconds time elapsed
> 
> where, cycles has an event coding of 0xff. This patch also removes
> the unnecessary 'event' mask in pmu_write_register, since the config_base
> is set by the pmu code after the event is validated.
> 
> Changes since V2:
>  - Switch to input unsigned long for pmu_validate_hw_event()
> 
> Signed-off-by: Suzuki K. Poulose 
> ---
>  drivers/bus/arm-cci.c |   10 ++
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> index 581190d..89c86e9 100644
> --- a/drivers/bus/arm-cci.c
> +++ b/drivers/bus/arm-cci.c
> @@ -179,12 +179,15 @@ enum cci400_perf_events {
>  #define CCI_REV_R1_MASTER_PORT_MIN_EV0x00
>  #define CCI_REV_R1_MASTER_PORT_MAX_EV0x11
>  
> -static int pmu_validate_hw_event(u8 hw_event)
> +static int pmu_validate_hw_event(unsigned long hw_event)
>  {
>   u8 ev_source = CCI_PMU_EVENT_SOURCE(hw_event);
>   u8 ev_code = CCI_PMU_EVENT_CODE(hw_event);
>   int if_type;
>  
> + if (hw_event & ~CCI_PMU_EVENT_MASK)
> + return -ENOENT;

Given that you want to build this for arm64, shouldn't CCI_PMU_EVENT_MASK
be an unsigned long too (i.e. 0xffUL)? Otherwise you won't detect set bits
in the upper word here.

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

Re: [PATCH 2/5] arm-cci: Abstract the CCI400 PMU speicific definitions

[Will Deacon]

On Tue, Mar 10, 2015 at 03:18:52PM +, Suzuki K. Poulose wrote:
> From: "Suzuki K. Poulose" 
> 
> CCI400 has different event specifications for PMU, for revsion
> 0 and revision 1. As of now, we check the revision every single
> time before using the parameters for the PMU. This patch abstracts
> the details of the pmu models in a struct (cci_pmu_model) and
> stores the information in cci_pmu at initialisation time, avoiding
> multiple probe operations.
> 
> Changes since V2:
>  - Cleanup event validation(pmu_validate_hw_event). Get rid of
>helper functions:
>   pmu_is_valid_slave_event
>   pmu_is_valid_master_event
> 
> Signed-off-by: Suzuki K. Poulose 
> ---

Looks good to me:

Reviewed-by: Will Deacon 

Will

>  drivers/bus/arm-cci.c |  141 
> -
>  1 file changed, 81 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> index ea39fc2..f88383e 100644
> --- a/drivers/bus/arm-cci.c
> +++ b/drivers/bus/arm-cci.c
> @@ -79,19 +79,38 @@ static const struct of_device_id arm_cci_matches[] = {
>  
>  #define CCI_PMU_MAX_HW_EVENTS 5   /* CCI PMU has 4 counters + 1 cycle 
> counter */
>  
> +/* Types of interfaces that can generate events */
> +enum {
> + CCI_IF_SLAVE,
> + CCI_IF_MASTER,
> + CCI_IF_MAX,
> +};
> +
> +struct event_range {
> + u32 min;
> + u32 max;
> +};
> +
>  struct cci_pmu_hw_events {
>   struct perf_event *events[CCI_PMU_MAX_HW_EVENTS];
>   unsigned long used_mask[BITS_TO_LONGS(CCI_PMU_MAX_HW_EVENTS)];
>   raw_spinlock_t pmu_lock;
>  };
>  
> +struct cci_pmu_model {
> + char *name;
> + struct event_range event_ranges[CCI_IF_MAX];
> +};
> +
> +static struct cci_pmu_model cci_pmu_models[];
> +
>  struct cci_pmu {
>   void __iomem *base;
>   struct pmu pmu;
>   int nr_irqs;
>   int irqs[CCI_PMU_MAX_HW_EVENTS];
>   unsigned long active_irqs;
> - struct pmu_port_event_ranges *port_ranges;
> + const struct cci_pmu_model *model;
>   struct cci_pmu_hw_events hw_events;
>   struct platform_device *plat_device;
>   int num_events;
> @@ -152,53 +171,11 @@ enum cci400_perf_events {
>  #define CCI_REV_R1_MASTER_PORT_MIN_EV0x00
>  #define CCI_REV_R1_MASTER_PORT_MAX_EV0x11
>  
> -struct pmu_port_event_ranges {
> - u8 slave_min;
> - u8 slave_max;
> - u8 master_min;
> - u8 master_max;
> -};
> -
> -static struct pmu_port_event_ranges port_event_range[] = {
> - [CCI_REV_R0] = {
> - .slave_min = CCI_REV_R0_SLAVE_PORT_MIN_EV,
> - .slave_max = CCI_REV_R0_SLAVE_PORT_MAX_EV,
> - .master_min = CCI_REV_R0_MASTER_PORT_MIN_EV,
> - .master_max = CCI_REV_R0_MASTER_PORT_MAX_EV,
> - },
> - [CCI_REV_R1] = {
> - .slave_min = CCI_REV_R1_SLAVE_PORT_MIN_EV,
> - .slave_max = CCI_REV_R1_SLAVE_PORT_MAX_EV,
> - .master_min = CCI_REV_R1_MASTER_PORT_MIN_EV,
> - .master_max = CCI_REV_R1_MASTER_PORT_MAX_EV,
> - },
> -};
> -
> -/*
> - * Export different PMU names for the different revisions so userspace knows
> - * because the event ids are different
> - */
> -static char *const pmu_names[] = {
> - [CCI_REV_R0] = "CCI_400",
> - [CCI_REV_R1] = "CCI_400_r1",
> -};
> -
> -static int pmu_is_valid_slave_event(u8 ev_code)
> -{
> - return pmu->port_ranges->slave_min <= ev_code &&
> - ev_code <= pmu->port_ranges->slave_max;
> -}
> -
> -static int pmu_is_valid_master_event(u8 ev_code)
> -{
> - return pmu->port_ranges->master_min <= ev_code &&
> - ev_code <= pmu->port_ranges->master_max;
> -}
> -
>  static int pmu_validate_hw_event(u8 hw_event)
>  {
>   u8 ev_source = CCI_PMU_EVENT_SOURCE(hw_event);
>   u8 ev_code = CCI_PMU_EVENT_CODE(hw_event);
> + int if_type;
>  
>   switch (ev_source) {
>   case CCI_PORT_S0:
> @@ -207,18 +184,22 @@ static int pmu_validate_hw_event(u8 hw_event)
>   case CCI_PORT_S3:
>   case CCI_PORT_S4:
>   /* Slave Interface */
> - if (pmu_is_valid_slave_event(ev_code))
> - return hw_event;
> + if_type = CCI_IF_SLAVE;
>   break;
>   case CCI_PORT_M0:
>   case CCI_PORT_M1:
>   case CCI_PORT_M2:
>   /* Master Interface */
> - if (pmu_is_valid_master_event(ev_code))
> - return hw_event;
> + if_type = CCI_IF_MASTER;

Re: [PATCH 2/5] iommu/mediatek: Add mt8173 IOMMU driver

[Will Deacon]

On Mon, Mar 09, 2015 at 12:11:43PM +, Yong Wu wrote:
> On Fri, 2015-03-06 at 10:58 +0000, Will Deacon wrote:
> > On Fri, Mar 06, 2015 at 10:48:17AM +, yong...@mediatek.com wrote:
> > > From: Yong Wu 
> > > 
> > > This patch adds support for mediatek m4u (MultiMedia Memory Management 
> > > Unit).
> > > Currently this only supports m4u gen 2 with 2 levels of page table on 
> > > mt8173.
> > 
> > [...]
> > 
> > > +/* 2 level pagetable: pgd -> pte */
> > > +#define F_PTE_TYPE_GET(regval)  (regval & 0x3)
> > > +#define F_PTE_TYPE_LARGE BIT(0)
> > > +#define F_PTE_TYPE_SMALL BIT(1)
> > > +#define F_PTE_B_BIT  BIT(2)
> > > +#define F_PTE_C_BIT  BIT(3)
> > > +#define F_PTE_BIT32_BIT  BIT(9)
> > > +#define F_PTE_S_BIT  BIT(10)
> > > +#define F_PTE_NG_BIT BIT(11)
> > > +#define F_PTE_PA_LARGE_MSK(~0UL << 16)
> > > +#define F_PTE_PA_LARGE_GET(regval)((regval >> 16) & 0x)
> > > +#define F_PTE_PA_SMALL_MSK(~0UL << 12)
> > > +#define F_PTE_PA_SMALL_GET(regval)((regval >> 12) & (~0))
> > > +#define F_PTE_TYPE_IS_LARGE_PAGE(pte) ((imu_pte_val(pte) & 0x3) == \
> > > +   F_PTE_TYPE_LARGE)
> > > +#define F_PTE_TYPE_IS_SMALL_PAGE(pte) ((imu_pte_val(pte) & 0x3) == \
> > > +   F_PTE_TYPE_SMALL)
> > 
> > This looks like the ARM short-descriptor format to me. Could you please
> > add a new page table format to the io-pgtable code, so that other IOMMU
> > drivers can make use of this? I know there was some interest in using
> > short descriptor for the ARM SMMU, for example.
> Currently I not familiar with the io-pgtable,I may need some time
> for it and the ARM short-descriptor. 

Well, you can read the LPAE version I wrote in io-pgtable-arm.c for some
inspiration (it's used by arm-smmu.c and ipmmu-vmsa.c).

> And there are some difference between mediatek's pagetable with the
> standard short-descriptor, like bit 9. we use it for the dram over 4GB.
> Then how should we do if there are some difference. 

That can easily be handled using a quirk (see, for example,
IO_PGTABLE_QUIRK_ARM_NS).

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

Re: [PATCH 2/5] iommu/mediatek: Add mt8173 IOMMU driver

[Will Deacon]

On Fri, Mar 06, 2015 at 10:48:17AM +, yong...@mediatek.com wrote:
> From: Yong Wu 
> 
> This patch adds support for mediatek m4u (MultiMedia Memory Management Unit).
> Currently this only supports m4u gen 2 with 2 levels of page table on mt8173.

[...]

> diff --git a/drivers/iommu/mtk_iommu_pagetable.c 
> b/drivers/iommu/mtk_iommu_pagetable.c
> new file mode 100644
> index 000..5fe9640
> --- /dev/null
> +++ b/drivers/iommu/mtk_iommu_pagetable.c
> @@ -0,0 +1,439 @@
> +/*
> + * Copyright (c) 2014-2015 MediaTek Inc.
> + * Author: Yong Wu 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "asm/cacheflush.h"
> +
> +#include "mtk_iommu.h"
> +#include "mtk_iommu_pagetable.h"
> +
> +/* 2 level pagetable: pgd -> pte */
> +#define F_PTE_TYPE_GET(regval)  (regval & 0x3)
> +#define F_PTE_TYPE_LARGE BIT(0)
> +#define F_PTE_TYPE_SMALL BIT(1)
> +#define F_PTE_B_BIT  BIT(2)
> +#define F_PTE_C_BIT  BIT(3)
> +#define F_PTE_BIT32_BIT  BIT(9)
> +#define F_PTE_S_BIT  BIT(10)
> +#define F_PTE_NG_BIT BIT(11)
> +#define F_PTE_PA_LARGE_MSK(~0UL << 16)
> +#define F_PTE_PA_LARGE_GET(regval)((regval >> 16) & 0x)
> +#define F_PTE_PA_SMALL_MSK(~0UL << 12)
> +#define F_PTE_PA_SMALL_GET(regval)((regval >> 12) & (~0))
> +#define F_PTE_TYPE_IS_LARGE_PAGE(pte) ((imu_pte_val(pte) & 0x3) == \
> +   F_PTE_TYPE_LARGE)
> +#define F_PTE_TYPE_IS_SMALL_PAGE(pte) ((imu_pte_val(pte) & 0x3) == \
> +   F_PTE_TYPE_SMALL)

This looks like the ARM short-descriptor format to me. Could you please
add a new page table format to the io-pgtable code, so that other IOMMU
drivers can make use of this? I know there was some interest in using
short descriptor for the ARM SMMU, for example.

Cheers,

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

Re: [PATCH/RFC 0/4] Probe deferral for IOMMU DT integration

[Will Deacon]

On Tue, Mar 03, 2015 at 11:54:46PM +, Laurent Pinchart wrote:
> Hello,

Hi Laurent,

> I haven't seen any reply to this e-mail. I know that the combination of 
> IOMMU, 
> DMA mapping and DT doesn't exactly sound like fun, but I think we still need 
> to move on :-)

Yup, and thanks for taking the time to write this up!

> Will and Arnd, could you please confirm that my summary below matches your 
> memories of our discussion ?

It certainly matches my recollection, but I suspect the devil is in the
details and we'll have to have some discussions once somebody has a crack at
implementing this.

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

Re: [PATCH v3 0/3] Scorpion PMU support

[Will Deacon]

On Sat, Feb 28, 2015 at 12:11:32AM +, Stephen Boyd wrote:
> These patches add support for the Scorpion PMU found on devices
> such as msm8660, qsd8x50, etc. The first patch is some groundwork
> to make functions more "generic". Even then we end up copying quite
> a bit of code from the Krait part into the Scorpion part with only
> subtle tweaks because two things happen:
> 
>  1. We gain another "region" register, L2LPM
>  2. The way to access the region registers are with different cp15 
> instructions
> 
> I'm not sure how to make this better, suggestions are welcome. The second
> patch is an optimization for an implementation defined register. The 
> final patch is the one that actually adds support for Scorpion.
> 
> Changes from v2:
>  * New patch 2
>  * Macros instead of an inline function for patch 1

Thanks, Stephen. I'll queue this for 4.1.

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

Re: [PATCH v2 2/2] ARM: perf: Add support for Scorpion PMUs

[Will Deacon]

On Fri, Feb 27, 2015 at 07:36:11PM +, Stephen Boyd wrote:
> On 02/25/15 08:58, Ashwin Chaugule wrote:
> > Its a count control register (PMxEVCNTCR). Theres various conditions
> > on which you can select when to start/stop counting. e.g. start when
> > another counter register overflows. Setting it to 0 was the
> > recommended default value on Scorpions and Kraits. Reset value is
> > unpredictable. So, just need to make sure this is set every time a
> > counter is setup. Will that still work if this is moved to the reset
> > path?
> 
> I don't think anything in this register changes unless we reset the CPU
> or software changes it, right? We already have a hotplug notifier to
> handle the case where the CPU is hotplugged out (and may get reset) and
> we need to reset the PMU. The other case is deep idle, which we don't
> have yet but we'll need to do a save/reset/restore across idle when we
> get there. So it seems possible to move it to the PMU reset path.

That would be my preference! Please respin the patches and I'll take another
look.

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

Re: [PATCH v2 2/2] ARM: perf: Add support for Scorpion PMUs

[Will Deacon]

On Fri, Feb 13, 2015 at 06:24:09PM +, Stephen Boyd wrote:
> Scorpion supports a set of local performance monitor event
> selection registers (LPM) sitting behind a cp15 based interface
> that extend the architected PMU events to include Scorpion CPU
> and Venum VFP specific events. To use these events the user is
> expected to program the lpm register with the event code shifted
> into the group they care about and then point the PMNx event at
> that region+group combo by writing a LPMn_GROUPx event. Add
> support for this hardware.
> 
> Note: the raw event number is a pure software construct that
> allows us to map the multi-dimensional number space of regions,
> groups, and event codes into a flat event number space suitable
> for use by the perf framework.
> 
> This is based on code originally written by Ashwin Chaugule and
> Neil Leeder [1] massaged to become similar to the Krait PMU
> support code.
> 
> [1] 
> https://www.codeaurora.org/cgit/quic/la/kernel/msm/tree/arch/arm/kernel/perf_event_msm.c?h=msm-3.4
> 
> Cc: Mark Rutland 
> Cc: Neil Leeder 
> Cc: Ashwin Chaugule 
> Cc: 
> Signed-off-by: Stephen Boyd 
> ---
>  Documentation/devicetree/bindings/arm/pmu.txt |   2 +
>  arch/arm/kernel/perf_event_cpu.c  |   2 +
>  arch/arm/kernel/perf_event_v7.c   | 417 
> ++
>  3 files changed, 421 insertions(+)

[...]

> +static void scorpion_evt_setup(int idx, u32 config_base)
> +{
> +   u32 val;
> +   u32 mask;
> +   u32 vval, fval;
> +   unsigned int region;
> +   unsigned int group;
> +   unsigned int code;
> +   unsigned int group_shift;
> +   bool venum_event;
> +
> +   krait_decode_event(config_base, ®ion, &group, &code, &venum_event,
> +  NULL);
> +
> +   group_shift = group * 8;
> +   mask = 0xff << group_shift;
> +
> +   /* Configure evtsel for the region and group */
> +   if (venum_event)
> +   val = SCORPION_VLPM_GROUP0;
> +   else
> +   val = scorpion_get_pmresrn_event(region);
> +   val += group;
> +   /* Mix in mode-exclusion bits */
> +   val |= config_base & (ARMV7_EXCLUDE_USER | ARMV7_EXCLUDE_PL1);
> +   armv7_pmnc_write_evtsel(idx, val);
> +
> +   asm volatile("mcr p15, 0, %0, c9, c15, 0" : : "r" (0));

What's this guy doing?

> +static int scorpion_pmu_get_event_idx(struct pmu_hw_events *cpuc,
> +  struct perf_event *event)
> +{
> +   int idx;
> +   int bit = -1;
> +   unsigned int region;
> +   unsigned int code;
> +   unsigned int group;
> +   bool venum_event, scorpion_event;
> +   struct hw_perf_event *hwc = &event->hw;
> +
> +   krait_decode_event(hwc->config_base, ®ion, &group, &code,
> +  &venum_event, &scorpion_event);
> +
> +   if (venum_event || scorpion_event) {
> +   /* Ignore invalid events */
> +   if (group > 3 || region > 3)

Where does the 3 come from?

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

Re: [PATCH v6 0/7] PCI: get DMA configuration from parent device

[Will Deacon]

On Thu, Feb 05, 2015 at 09:52:52PM +, Murali Karicheri wrote:
> This patch add an important capability to PCI driver on Keystone. I hope to
> have this merged to the upstream branch so that it is available for v3.20.
> Also would like thank everyone for the contribution.
> 
> PCI devices on Keystone doesn't have correct dma_pfn_offset set. This patch
> add capability to set the dma configuration such as dma-mask, dma_pfn_offset,
> and dma ops etc using the information from DT. The prior RFCs and discussions
> are available at [1] and [2] below.
> 
> [2] : https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg790244.html
> [1] : http://www.gossamer-threads.com/lists/linux/kernel/2024591
> 
> Change history:
>   v6 - Rebased to v3.19-v7
>  - Addressed some minor comments about node name and DT size 
> validation.
>  - Pulled out 8/8 of v5 and plan to send a patch for enhancing
>of_dma_configure() to use size to calculate dma mask.
>  - Added Acks from reviewers.

This series looks fine to me:

  Acked-by: Will Deacon 

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

Re: [PATCH 2/4] arm64: pmu: add support for interrupt-affinity property

[Will Deacon]

On Thu, Feb 05, 2015 at 11:56:01AM +, Mark Rutland wrote:
> On Mon, Jan 26, 2015 at 05:54:16PM +0000, Will Deacon wrote:
> > Historically, the PMU devicetree bindings have expected SPIs to be
> > listed in order of *logical* CPU number. This is problematic for
> > bootloaders, especially when the boot CPU (logical ID 0) isn't listed
> > first in the devicetree.
> > 
> > This patch adds a new optional property, interrupt-affinity, to the
> > PMU node which allows the interrupt affinity to be described using
> > a list of phandled to CPU nodes, with each entry in the list
> > corresponding to the SPI at the same index in the interrupts property.
> > 
> > Cc: Mark Rutland 
> > Signed-off-by: Will Deacon 
> > ---
> >  Documentation/devicetree/bindings/arm/pmu.txt |  6 +++
> >  arch/arm64/include/asm/pmu.h  |  1 +
> >  arch/arm64/kernel/perf_event.c| 57 
> > +--
> >  3 files changed, 60 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/pmu.txt 
> > b/Documentation/devicetree/bindings/arm/pmu.txt
> > index 75ef91d08f3b..a9281fc48743 100644
> > --- a/Documentation/devicetree/bindings/arm/pmu.txt
> > +++ b/Documentation/devicetree/bindings/arm/pmu.txt
> > @@ -24,6 +24,12 @@ Required properties:
> >  
> >  Optional properties:
> >  
> > +- interrupt-affinity : Valid only when using SPIs, specifies a list of 
> > phandles
> > +   to CPU nodes corresponding directly to the affinity 
> > of
> > +  the SPIs listed in the interrupts property. If absent,
> > +  the interrupts are assumed to be listed in logical CPU
> > +  order.
> 
> This covers the case we care about today, but it's problematic in cases
> where the number of interrupts is not equal to the number of CPUs affine
> to that interrupt. For example:
> 
> * PPIs in big.LITTLE systems, where we may need a node per cluster, and
>   will need a way of associating a PMU node with a subset of all CPUs,
>   despite having only one interrupt.
> 
> * Muxed SPIs per-cluster (is this likely to happen?)
> 
> The former can be covered by allowing multiple entries in
> interrupt-affintiy for PPIs.

Yes, that sounds like a sensible extension in the future if we have to
support such a platform.

> I'm not sure if the latter is something we need to cater for. If we do,
> then perhaps we need an interruptN-affinity property per interrupt (though
> that's ugly and painful to deal with).

I'm not keen to handle this, so I'd rather defer it to whoever ends up
building it. Trying to design for every possibility is usually impossible
in my experience and you just end up carrying something that isn't useful.

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

Re: [PATCH 1/4] arm64: dts: fix PMU IRQ ordering for Juno

[Will Deacon]

On Thu, Feb 05, 2015 at 11:59:33AM +, Mark Rutland wrote:
> On Thu, Feb 05, 2015 at 11:54:16AM +0000, Will Deacon wrote:
> > On Thu, Feb 05, 2015 at 11:46:42AM +, Mark Rutland wrote:
> > > On Mon, Jan 26, 2015 at 05:54:15PM +, Will Deacon wrote:
> > > > diff --git a/arch/arm64/boot/dts/arm/juno.dts 
> > > > b/arch/arm64/boot/dts/arm/juno.dts
> > > > index cb3073e4e7a8..4ed9287aaef1 100644
> > > > --- a/arch/arm64/boot/dts/arm/juno.dts
> > > > +++ b/arch/arm64/boot/dts/arm/juno.dts
> > > > @@ -107,11 +107,11 @@
> > > > pmu {
> > > > compatible = "arm,armv8-pmuv3";
> > > > interrupts = ,
> > > > +,
> > > > +,
> > > >  ,
> > > >  ,
> > > > -,
> > > > -,
> > > > -;
> > > > +;
> > > > };
> > > 
> > > I am very much not keen on this. While this may get things working
> > > today, it completely relies on Linux-internal details (the order of CPU
> > > bringup, which in this case is different from the order of entries in
> > > /cpus).
> > > 
> > > In all other dts that I am aware of, the order of entries in /cpus
> > > aligns with the order of interrupts in the PMU node, and the first entry
> > > is the boot CPU.
> > > 
> > > I think that we should ensure that the ordering of CPU nodes matches the
> > > order of interrupts here. That way we can fall back to that ordering (if
> > > not explicitly overridden), and even after an arbitrary logical
> > > renumbering (e.g. after a kexec) the relationship should stay intact.
> > 
> > There are a few problems with reordering the CPU nodes:
> > 
> >   (1) It breaks any existing users of taskset to pin on big/little
> >   clusters.
> 
> This is unfortunate, but this is also the case if the boot CPU is
> different.

Right, so don't change the boot CPU. In that vain, we also shouldn't change
the CPU order in the .dts -- the current .dts is working for taskset and
we shouldn't break people's scripts just because they want to use the PMU.

> >   (2) It's not generally possible if, for example, the bootloader decides
> >   to boot Linux on a different CPU then we have no choice but to
> >   change the PMU interrupt order.
> 
> In that case _this_ patch is broken.

Why? I'm not denying that changing the boot CPU causes problems, I'm saying
that you *can't* fix that by changing the CPU node order. You still have
to change the interrupt order in that case, so why not just localise the
changes there in the first place?

> If we associate the interrupt with a CPU by node order, the relationship
> is preserved regardless of which CPU is the boot CPU (whether it was the
> bootloader's choice, kexec, or whatever).

Sure, and that requires code changes. If we're going to change the code,
then I'd much rather we make the binding explicit, like I did in the
follow-up patches to this one. As I mentioned before, this is a .dts fix
to get things working with the current code. It's really too late to argue
about the existing binding, even if it sucks.

> >   (3) I didn't think that the ordering of CPU nodes was guaranteed to be
> >   preserved by dtc, whereas the order of the interrupts will be.
> 
> The order of nodes is presently preserved.

It's not about the present behaviour; I need a _guarantee_ that dtc/libfdt
will *never* reorder CPU nodes. Today's working .dts file needs to continue
to work with future tools.

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

Re: [PATCH 1/4] arm64: dts: fix PMU IRQ ordering for Juno

[Will Deacon]

On Thu, Feb 05, 2015 at 11:46:42AM +, Mark Rutland wrote:
> On Mon, Jan 26, 2015 at 05:54:15PM +0000, Will Deacon wrote:
> > diff --git a/arch/arm64/boot/dts/arm/juno.dts 
> > b/arch/arm64/boot/dts/arm/juno.dts
> > index cb3073e4e7a8..4ed9287aaef1 100644
> > --- a/arch/arm64/boot/dts/arm/juno.dts
> > +++ b/arch/arm64/boot/dts/arm/juno.dts
> > @@ -107,11 +107,11 @@
> > pmu {
> > compatible = "arm,armv8-pmuv3";
> > interrupts = ,
> > +,
> > +,
> >  ,
> >  ,
> > -,
> > -,
> > -;
> > +;
> > };
> 
> I am very much not keen on this. While this may get things working
> today, it completely relies on Linux-internal details (the order of CPU
> bringup, which in this case is different from the order of entries in
> /cpus).
> 
> In all other dts that I am aware of, the order of entries in /cpus
> aligns with the order of interrupts in the PMU node, and the first entry
> is the boot CPU.
> 
> I think that we should ensure that the ordering of CPU nodes matches the
> order of interrupts here. That way we can fall back to that ordering (if
> not explicitly overridden), and even after an arbitrary logical
> renumbering (e.g. after a kexec) the relationship should stay intact.

There are a few problems with reordering the CPU nodes:

  (1) It breaks any existing users of taskset to pin on big/little
  clusters.

  (2) It's not generally possible if, for example, the bootloader decides
  to boot Linux on a different CPU then we have no choice but to
  change the PMU interrupt order.

  (3) I didn't think that the ordering of CPU nodes was guaranteed to be
  preserved by dtc, whereas the order of the interrupts will be.

> This DT has clearly never worked (nor been tested), and I think having
> this as an intermediary step only adds to the long term support burden
> by having the juno dts arbitrarily different to all other dts files (by
> relying on a logical order that's different to the /cpus order).
> 
> Longer term we must ensure we have a more explicit ordering, as with
> your later patches.

Agreed. This is intended as something simpler for -stable.

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

Re: [PATCH v4 1/6] of: iommu: add ptr to OF node arg to of_iommu_configure()

[Will Deacon]

On Mon, Jan 26, 2015 at 06:49:01PM +, Murali Karicheri wrote:
> On 01/25/2015 08:32 AM, Laurent Pinchart wrote:
> > Hi Murali,
> >
> > Thank you for the patch.
> >
> > On Friday 23 January 2015 17:32:34 Murali Karicheri wrote:
> >> Function of_iommu_configure() is called from of_dma_configure() to
> >> setup iommu ops using DT property. This API is currently used for
> >> platform devices for which DMA configuration (including iommu ops)
> >> may come from device's parent. To extend this functionality for PCI
> >> devices, this API need to take a parent node ptr as an argument
> >> instead of assuming device's parent. This is needed since for PCI, the
> >> dma configuration may be defined in the DT node of the root bus bridge's
> >> parent device. Currently only dma-range is used for PCI and iommu is not
> >> supported. So return error if the device is PCI.
> >>
> >> Cc: Joerg Roedel
> >> Cc: Grant Likely
> >> Cc: Rob Herring
> >> Cc: Bjorn Helgaas
> >> Cc: Will Deacon
> >> Cc: Russell King
> >> Cc: Arnd Bergmann
> >> Cc: Suravee Suthikulpanit
> >>
> >> Signed-off-by: Murali Karicheri
> >> ---
> >>   drivers/iommu/of_iommu.c |   10 --
> >>   drivers/of/platform.c|2 +-
> >>   include/linux/of_iommu.h |6 --
> >>   3 files changed, 13 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> >> index af1dc6a..439235b 100644
> >> --- a/drivers/iommu/of_iommu.c
> >> +++ b/drivers/iommu/of_iommu.c
> >> @@ -133,19 +133,25 @@ struct iommu_ops *of_iommu_get_ops(struct device_node
> >> *np) return ops;
> >>   }
> >>
> >> -struct iommu_ops *of_iommu_configure(struct device *dev)
> >> +struct iommu_ops *of_iommu_configure(struct device *dev,
> >> +   struct device_node *iommu_np)
> >>   {
> >>struct of_phandle_args iommu_spec;
> >>struct device_node *np;
> >>struct iommu_ops *ops = NULL;
> >>int idx = 0;
> >>
> >> +  if (dev_is_pci(dev)) {
> >> +  dev_err(dev, "iommu is currently not supported for PCI\n");
> >> +  return NULL;
> >> +  }
> >> +
> >>/*
> >> * We don't currently walk up the tree looking for a parent IOMMU.
> >> * See the `Notes:' section of
> >> * Documentation/devicetree/bindings/iommu/iommu.txt
> >> */
> > Wouldn't it be better to fix this rather than passing the device node 
> > pointer
> > to the function ? The solution would be more generic.
> Laurent,
> 
> Will Deacon (Copied here) is working on this as we alteady discussed in 
> this thread. So it will be
> addressed by him in another series.

Well, "working on this" equates to "has it somewhere near the bottom of
a very long list" :)

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

Re: [PATCH v4 1/6] of: iommu: add ptr to OF node arg to of_iommu_configure()

[Will Deacon]

On Wed, Jan 28, 2015 at 01:15:10PM +, Laurent Pinchart wrote:
> On Wednesday 28 January 2015 12:29:42 Will Deacon wrote:
> > On Wed, Jan 28, 2015 at 12:23:03PM +, Laurent Pinchart wrote:
> > > On Wednesday 28 January 2015 11:33:00 Will Deacon wrote:
> > >> On Mon, Jan 26, 2015 at 06:49:01PM +, Murali Karicheri wrote:
> > >>> On 01/25/2015 08:32 AM, Laurent Pinchart wrote:
> > >>>> On Friday 23 January 2015 17:32:34 Murali Karicheri wrote:
> > >>>>> Function of_iommu_configure() is called from of_dma_configure() to
> > >>>>> setup iommu ops using DT property. This API is currently used for
> > >>>>> platform devices for which DMA configuration (including iommu ops)
> > >>>>> may come from device's parent. To extend this functionality for PCI
> > >>>>> devices, this API need to take a parent node ptr as an argument
> > >>>>> instead of assuming device's parent. This is needed since for PCI,
> > >>>>> the dma configuration may be defined in the DT node of the root bus
> > >>>>> bridge's parent device. Currently only dma-range is used for PCI and
> > >>>>> iommu is not supported. So return error if the device is PCI.
> > >>>>> 
> > >>>>> Cc: Joerg Roedel
> > >>>>> Cc: Grant Likely
> > >>>>> Cc: Rob Herring
> > >>>>> Cc: Bjorn Helgaas
> > >>>>> Cc: Will Deacon
> > >>>>> Cc: Russell King
> > >>>>> Cc: Arnd Bergmann
> > >>>>> Cc: Suravee Suthikulpanit
> > >>>>> 
> > >>>>> Signed-off-by: Murali Karicheri
> > >>>>> ---
> > >>>>> 
> > >>>>>   drivers/iommu/of_iommu.c |   10 --
> > >>>>>   drivers/of/platform.c|2 +-
> > >>>>>   include/linux/of_iommu.h |6 --
> > >>>>>   3 files changed, 13 insertions(+), 5 deletions(-)
> > >>>>> 
> > >>>>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> > >>>>> index af1dc6a..439235b 100644
> > >>>>> --- a/drivers/iommu/of_iommu.c
> > >>>>> +++ b/drivers/iommu/of_iommu.c
> > >>>>> @@ -133,19 +133,25 @@ struct iommu_ops *of_iommu_get_ops(struct
> > >>>>> device_node *np)
> > >>>>>   return ops;
> > >>>>>  }
> > >>>>> 
> > >>>>> -struct iommu_ops *of_iommu_configure(struct device *dev)
> > >>>>> +struct iommu_ops *of_iommu_configure(struct device *dev,
> > >>>>> +  struct device_node *iommu_np)
> > >>>>>  {
> > >>>>>   struct of_phandle_args iommu_spec;
> > >>>>>   struct device_node *np;
> > >>>>>   struct iommu_ops *ops = NULL;
> > >>>>>   int idx = 0;
> > >>>>> 
> > >>>>> + if (dev_is_pci(dev)) {
> > >>>>> + dev_err(dev, "iommu is currently not supported for 
> > >>>>> PCI\n");
> > >>>>> + return NULL;
> > >>>>> + }
> > >>>>> +
> > >>>>>   /*
> > >>>>>* We don't currently walk up the tree looking for a parent
> > >>>>>IOMMU.
> > >>>>>* See the `Notes:' section of
> > >>>>>* Documentation/devicetree/bindings/iommu/iommu.txt
> > >>>>>*/
> > >>>> 
> > >>>> Wouldn't it be better to fix this rather than passing the device node
> > >>>> pointer to the function ? The solution would be more generic.
> > >>> 
> > >>> Will Deacon (Copied here) is working on this as we alteady discussed
> > >>> in this thread. So it will be addressed by him in another series.
> > >> 
> > >> Well, "working on this" equates to "has it somewhere near the bottom of
> > >> a very long list" :)
> > > 
> > > What's your opinion on this patch then, do you think adding the iommu_np
> > > argument makes sense as a temporary hack, or should we instead walk up the
> > > tree looking for an iommus attribute in parent nodes ? I don't expect that
> > > to be insanely difficult to code.
> > 
> > If walking up the tree is useful, then I think we should do that and update
> > the Documentation to reflect the new behaviour.
> 
> If I understand Murali's patch set right (please correct me if that's not the 
> case) the PCI code walks up the DT nodes hierarchy to the parent node that 
> contains the iommus attribute and passes a pointer to that node to this 
> function. It's thus a PCI-specific solution. As a temporary hack that's OK I 
> suppose, but if implementing it right straight away isn't difficult that 
> would 
> be better.

It looks to me like the code walks the PCI topology to get the DT node for
the host controller, and passes *that* to of_dma_configure. That sounds
like the right thing to do to me, especially since the PCI topology is
likely not encoded in the device-tree. So actually, it is passing the
first parent node afaict.

The part I'm more interested in is the mapping of RequesterID (PCI dma
alias) to IOMMU ID when we transition from the PCI topology to the DT
topology. Currently, it seems to be 1:1 on the platforms I have, but I
doubt this will always be the case.

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

Re: [PATCH v4 1/6] of: iommu: add ptr to OF node arg to of_iommu_configure()

[Will Deacon]

On Wed, Jan 28, 2015 at 12:23:03PM +, Laurent Pinchart wrote:
> On Wednesday 28 January 2015 11:33:00 Will Deacon wrote:
> > On Mon, Jan 26, 2015 at 06:49:01PM +, Murali Karicheri wrote:
> > > On 01/25/2015 08:32 AM, Laurent Pinchart wrote:
> > >> On Friday 23 January 2015 17:32:34 Murali Karicheri wrote:
> > >>> Function of_iommu_configure() is called from of_dma_configure() to
> > >>> setup iommu ops using DT property. This API is currently used for
> > >>> platform devices for which DMA configuration (including iommu ops)
> > >>> may come from device's parent. To extend this functionality for PCI
> > >>> devices, this API need to take a parent node ptr as an argument
> > >>> instead of assuming device's parent. This is needed since for PCI, the
> > >>> dma configuration may be defined in the DT node of the root bus
> > >>> bridge's parent device. Currently only dma-range is used for PCI and
> > >>> iommu is not supported. So return error if the device is PCI.
> > >>> 
> > >>> Cc: Joerg Roedel
> > >>> Cc: Grant Likely
> > >>> Cc: Rob Herring
> > >>> Cc: Bjorn Helgaas
> > >>> Cc: Will Deacon
> > >>> Cc: Russell King
> > >>> Cc: Arnd Bergmann
> > >>> Cc: Suravee Suthikulpanit
> > >>> 
> > >>> Signed-off-by: Murali Karicheri
> > >>> ---
> > >>> 
> > >>>   drivers/iommu/of_iommu.c |   10 --
> > >>>   drivers/of/platform.c|2 +-
> > >>>   include/linux/of_iommu.h |6 --
> > >>>   3 files changed, 13 insertions(+), 5 deletions(-)
> > >>> 
> > >>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> > >>> index af1dc6a..439235b 100644
> > >>> --- a/drivers/iommu/of_iommu.c
> > >>> +++ b/drivers/iommu/of_iommu.c
> > >>> @@ -133,19 +133,25 @@ struct iommu_ops *of_iommu_get_ops(struct
> > >>> device_node *np)
> > >>> return ops;
> > >>>  }
> > >>> 
> > >>> -struct iommu_ops *of_iommu_configure(struct device *dev)
> > >>> +struct iommu_ops *of_iommu_configure(struct device *dev,
> > >>> +struct device_node *iommu_np)
> > >>>  {
> > >>> struct of_phandle_args iommu_spec;
> > >>> struct device_node *np;
> > >>>     struct iommu_ops *ops = NULL;
> > >>> int idx = 0;
> > >>> 
> > >>> +   if (dev_is_pci(dev)) {
> > >>> +   dev_err(dev, "iommu is currently not supported for 
> > >>> PCI\n");
> > >>> +   return NULL;
> > >>> +   }
> > >>> +
> > >>> /*
> > >>>  * We don't currently walk up the tree looking for a parent 
> > >>> IOMMU.
> > >>>  * See the `Notes:' section of
> > >>>  * Documentation/devicetree/bindings/iommu/iommu.txt
> > >>>  */
> > >> 
> > >> Wouldn't it be better to fix this rather than passing the device node
> > >> pointer to the function ? The solution would be more generic.
> > > 
> > > Will Deacon (Copied here) is working on this as we alteady discussed in
> > > this thread. So it will be addressed by him in another series.
> > 
> > Well, "working on this" equates to "has it somewhere near the bottom of
> > a very long list" :)
> 
> What's your opinion on this patch then, do you think adding the iommu_np 
> argument makes sense as a temporary hack, or should we instead walk up the 
> tree looking for an iommus attribute in parent nodes ? I don't expect that to 
> be insanely difficult to code.

If walking up the tree is useful, then I think we should do that and update
the Documentation to reflect the new behaviour. The only reason that I
didn't code that in of_iommu_configure originally is because I didn't want
to go against the binding spec for the initial version, especially as we
didn't have a reason to look at parent nodes.

The only thing to double-check is that we don't break any existing users
of the binding with this change, but I don't think that we do.

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

[PATCH 1/4] arm64: dts: fix PMU IRQ ordering for Juno

[Will Deacon]

For better or worse, perf expects the per-cpu SPI PMU interrupts to be
listed in order of logical CPU. This patch fixes the Juno .dts to
satisfy that requirement.

Without this patch, I see unhandled IRQs in mainline:

  irq 9: nobody cared (try booting with the "irqpoll" option)
  CPU: 3 PID: 2830 Comm: cc1 Not tainted 3.19.0-rc6+ #1
  Hardware name: ARM Juno development board (r0) (DT)

  [...]

  handlers:
  [] armv8pmu_handle_irq
  Disabling IRQ #9

Cc: Mark Rutland 
Signed-off-by: Will Deacon 
---

This is an immediate fix for mainline, with the remaining patches in the
series solving this by extending the binding.

 arch/arm64/boot/dts/arm/juno.dts | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
index cb3073e4e7a8..4ed9287aaef1 100644
--- a/arch/arm64/boot/dts/arm/juno.dts
+++ b/arch/arm64/boot/dts/arm/juno.dts
@@ -107,11 +107,11 @@
pmu {
compatible = "arm,armv8-pmuv3";
interrupts = ,
+,
+,
 ,
 ,
-,
-,
-;
+;
};
 
/include/ "juno-clocks.dtsi"
-- 
2.1.4

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

[PATCH 3/4] ARM: pmu: add support for interrupt-affinity property

[Will Deacon]

Historically, the PMU devicetree bindings have expected SPIs to be
listed in order of *logical* CPU number. This is problematic for
bootloaders, especially when the boot CPU (logical ID 0) isn't listed
first in the devicetree.

This patch adds a new optional property, interrupt-affinity, to the
PMU node which allows the interrupt affinity to be described using
a list of phandled to CPU nodes, with each entry in the list
corresponding to the SPI at the same index in the interrupts property.

Cc: Mark Rutland 
Signed-off-by: Will Deacon 
---
 arch/arm/include/asm/pmu.h   |  1 +
 arch/arm/kernel/perf_event_cpu.c | 69 
 2 files changed, 63 insertions(+), 7 deletions(-)

diff --git a/arch/arm/include/asm/pmu.h b/arch/arm/include/asm/pmu.h
index b1596bd59129..675e4ab79f68 100644
--- a/arch/arm/include/asm/pmu.h
+++ b/arch/arm/include/asm/pmu.h
@@ -92,6 +92,7 @@ struct pmu_hw_events {
 struct arm_pmu {
struct pmu  pmu;
cpumask_t   active_irqs;
+   int *irq_affinity;
char*name;
irqreturn_t (*handle_irq)(int irq_num, void *dev);
void(*enable)(struct perf_event *event);
diff --git a/arch/arm/kernel/perf_event_cpu.c b/arch/arm/kernel/perf_event_cpu.c
index dd9acc95ebc0..488bda0646ec 100644
--- a/arch/arm/kernel/perf_event_cpu.c
+++ b/arch/arm/kernel/perf_event_cpu.c
@@ -92,11 +92,16 @@ static void cpu_pmu_free_irq(struct arm_pmu *cpu_pmu)
free_percpu_irq(irq, &hw_events->percpu_pmu);
} else {
for (i = 0; i < irqs; ++i) {
-   if (!cpumask_test_and_clear_cpu(i, 
&cpu_pmu->active_irqs))
+   int cpu = i;
+
+   if (cpu_pmu->irq_affinity)
+   cpu = cpu_pmu->irq_affinity[i];
+
+   if (!cpumask_test_and_clear_cpu(cpu, 
&cpu_pmu->active_irqs))
continue;
irq = platform_get_irq(pmu_device, i);
if (irq >= 0)
-   free_irq(irq, 
per_cpu_ptr(&hw_events->percpu_pmu, i));
+   free_irq(irq, 
per_cpu_ptr(&hw_events->percpu_pmu, cpu));
}
}
 }
@@ -128,32 +133,37 @@ static int cpu_pmu_request_irq(struct arm_pmu *cpu_pmu, 
irq_handler_t handler)
on_each_cpu(cpu_pmu_enable_percpu_irq, &irq, 1);
} else {
for (i = 0; i < irqs; ++i) {
+   int cpu = i;
+
err = 0;
irq = platform_get_irq(pmu_device, i);
if (irq < 0)
continue;
 
+   if (cpu_pmu->irq_affinity)
+   cpu = cpu_pmu->irq_affinity[i];
+
/*
 * If we have a single PMU interrupt that we can't 
shift,
 * assume that we're running on a uniprocessor machine 
and
 * continue. Otherwise, continue without this interrupt.
 */
-   if (irq_set_affinity(irq, cpumask_of(i)) && irqs > 1) {
+   if (irq_set_affinity(irq, cpumask_of(cpu)) && irqs > 1) 
{
pr_warn("unable to set irq affinity (irq=%d, 
cpu=%u)\n",
-   irq, i);
+   irq, cpu);
continue;
}
 
err = request_irq(irq, handler,
  IRQF_NOBALANCING | IRQF_NO_THREAD, 
"arm-pmu",
- per_cpu_ptr(&hw_events->percpu_pmu, 
i));
+ per_cpu_ptr(&hw_events->percpu_pmu, 
cpu));
if (err) {
pr_err("unable to request IRQ%d for ARM PMU 
counters\n",
irq);
return err;
}
 
-   cpumask_set_cpu(i, &cpu_pmu->active_irqs);
+   cpumask_set_cpu(cpu, &cpu_pmu->active_irqs);
}
}
 
@@ -289,6 +299,48 @@ static int probe_current_pmu(struct arm_pmu *pmu)
return ret;
 }
 
+static int of_pmu_irq_cfg(struct platform_device *pdev)
+{
+   int i;
+   int *irqs = kcalloc(pdev->num_resources, sizeof(*irqs), GFP_KERNEL);
+
+   if (!irqs)
+   return -ENOMEM;
+
+   for (i = 0; i < pdev->num_resources; ++i) {
+   struct device_node *dn;
+   int cpu = -1;
+
+   dn = of_parse_phandle(pdev->dev.of_node, "interrupt-affinity",
+ 

[PATCH 4/4] arm64: dts: add interrupt-affinity property to pmu node for juno

[Will Deacon]

Make the Juno .dts robust against potential reordering of the CPU nodes
by adding an explicit interrupt-affinity property to the PMU node.

Cc: Mark Rutland 
Signed-off-by: Will Deacon 
---
 arch/arm64/boot/dts/arm/juno.dts | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
index 4ed9287aaef1..3542500137f0 100644
--- a/arch/arm64/boot/dts/arm/juno.dts
+++ b/arch/arm64/boot/dts/arm/juno.dts
@@ -112,6 +112,12 @@
 ,
 ,
 ;
+   interrupt-affinity = <&A53_0>,
+<&A57_0>,
+<&A57_1>,
+<&A53_1>,
+<&A53_2>,
+<&A53_3>;
};
 
/include/ "juno-clocks.dtsi"
-- 
2.1.4

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

[PATCH 2/4] arm64: pmu: add support for interrupt-affinity property

[Will Deacon]

Historically, the PMU devicetree bindings have expected SPIs to be
listed in order of *logical* CPU number. This is problematic for
bootloaders, especially when the boot CPU (logical ID 0) isn't listed
first in the devicetree.

This patch adds a new optional property, interrupt-affinity, to the
PMU node which allows the interrupt affinity to be described using
a list of phandled to CPU nodes, with each entry in the list
corresponding to the SPI at the same index in the interrupts property.

Cc: Mark Rutland 
Signed-off-by: Will Deacon 
---
 Documentation/devicetree/bindings/arm/pmu.txt |  6 +++
 arch/arm64/include/asm/pmu.h  |  1 +
 arch/arm64/kernel/perf_event.c| 57 +--
 3 files changed, 60 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/pmu.txt 
b/Documentation/devicetree/bindings/arm/pmu.txt
index 75ef91d08f3b..a9281fc48743 100644
--- a/Documentation/devicetree/bindings/arm/pmu.txt
+++ b/Documentation/devicetree/bindings/arm/pmu.txt
@@ -24,6 +24,12 @@ Required properties:
 
 Optional properties:
 
+- interrupt-affinity : Valid only when using SPIs, specifies a list of phandles
+   to CPU nodes corresponding directly to the affinity of
+  the SPIs listed in the interrupts property. If absent,
+  the interrupts are assumed to be listed in logical CPU
+  order.
+
 - qcom,no-pc-write : Indicates that this PMU doesn't support the 0xc and 0xd
  events.
 
diff --git a/arch/arm64/include/asm/pmu.h b/arch/arm64/include/asm/pmu.h
index e6f087806aaf..b7710a59672c 100644
--- a/arch/arm64/include/asm/pmu.h
+++ b/arch/arm64/include/asm/pmu.h
@@ -44,6 +44,7 @@ struct pmu_hw_events {
 struct arm_pmu {
struct pmu  pmu;
cpumask_t   active_irqs;
+   int *irq_affinity;
const char  *name;
irqreturn_t (*handle_irq)(int irq_num, void *dev);
void(*enable)(struct hw_perf_event *evt, int idx);
diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index 25a5308744b1..19821d936e75 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -25,8 +25,10 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -396,7 +398,12 @@ armpmu_release_hardware(struct arm_pmu *armpmu)
free_percpu_irq(irq, &cpu_hw_events);
} else {
for (i = 0; i < irqs; ++i) {
-   if (!cpumask_test_and_clear_cpu(i, 
&armpmu->active_irqs))
+   int cpu = i;
+
+   if (arm_pmu->irq_affinity)
+   cpu = arm_pmu->irq_affinity[i];
+
+   if (!cpumask_test_and_clear_cpu(cpu, 
&armpmu->active_irqs))
continue;
irq = platform_get_irq(pmu_device, i);
if (irq > 0)
@@ -450,19 +457,24 @@ armpmu_reserve_hardware(struct arm_pmu *armpmu)
on_each_cpu(armpmu_enable_percpu_irq, &irq, 1);
} else {
for (i = 0; i < irqs; ++i) {
+   int cpu = i;
+
err = 0;
irq = platform_get_irq(pmu_device, i);
if (irq <= 0)
continue;
 
+   if (armpmu->irq_affinity)
+   cpu = armpmu->irq_affinity[i];
+
/*
 * If we have a single PMU interrupt that we can't 
shift,
 * assume that we're running on a uniprocessor machine 
and
 * continue. Otherwise, continue without this interrupt.
 */
-   if (irq_set_affinity(irq, cpumask_of(i)) && irqs > 1) {
+   if (irq_set_affinity(irq, cpumask_of(cpu)) && irqs > 1) 
{
pr_warning("unable to set irq affinity (irq=%d, 
cpu=%u)\n",
-   irq, i);
+   irq, cpu);
continue;
}
 
@@ -476,7 +488,7 @@ armpmu_reserve_hardware(struct arm_pmu *armpmu)
return err;
}
 
-   cpumask_set_cpu(i, &armpmu->active_irqs);
+   cpumask_set_cpu(cpu, &armpmu->active_irqs);
}
}
 
@@ -1289,9 +1301,46 @@ static const struct of_device_id armpmu_of_device_ids[] 
= {
 
 static int armpmu_device_probe(struct platform_device *pdev)
 {
+   int i, *irqs;
+
if (!cpu_pmu)
   

Re: [PATCH v3 3/4] of/pci: add of_pci_dma_configure() update dma configuration

[Will Deacon]

On Thu, Jan 08, 2015 at 10:25:15PM +, Arnd Bergmann wrote:
> On Thursday 08 January 2015 14:52:13 Murali Karicheri wrote:
> > 
> > Could you add this as as a follow up patch as I don't have a platformm 
> > that support IOMMU and as such my understanding of the IOMMU is limited?
> > 
> > I can help test the change to make sure it has no side effect on 
> > Keystone that doesn't support IOMMU.
> 
> I think that's fine. Let's get your series done first and add the
> DMA aliases for iommus on top once we have worked out what to do.

Yeah, I'm fine with that too. I think we're going to be working on this for
a while, so doing it bit-by-bit makes a lot of sense.

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

Re: [PATCH v3 3/4] of/pci: add of_pci_dma_configure() update dma configuration

[Will Deacon]

On Wed, Jan 07, 2015 at 06:49:53PM +, Murali Karicheri wrote:
> Add of_pci_dma_configure() to allow updating the dma configuration
> of the pci device using the configuration from DT of the parent of
> the root bridge device.
> 
> Signed-off-by: Murali Karicheri 
> ---
>  drivers/of/of_pci.c|   39 +++
>  include/linux/of_pci.h |   12 
>  2 files changed, 51 insertions(+)
> 
> diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
> index 88471d3..34878c9 100644
> --- a/drivers/of/of_pci.c
> +++ b/drivers/of/of_pci.c
> @@ -2,6 +2,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -229,6 +230,44 @@ parse_failed:
>   return err;
>  }
>  EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources);
> +
> +/**
> + * of_get_pci_root_bridge_parent - get the OF node of the root bridge's 
> parent
> + * @dev: ptr to pci_dev struct of the pci device
> + *
> + * This function will traverse the bus up to the root bus starting with
> + * the child and return the OF node ptr to root bridge device's parent 
> device.
> + */
> +struct device_node *of_get_pci_root_bridge_parent(struct pci_dev *dev)
> +{
> + struct pci_bus *bus = dev->bus;
> + struct device *bridge;
> +
> + while (!pci_is_root_bus(bus))
> + bus = bus->parent;
> + bridge = bus->bridge;
> +
> + return bridge->parent->of_node;
> +}
> +EXPORT_SYMBOL_GPL(of_get_pci_root_bridge_parent);
> +
> +/**
> + * of_pci_dma_configure - Setup DMA configuration
> + * @dev: ptr to pci_dev struct of the pci device
> + *
> + * Function to update PCI devices's DMA configuration using the same
> + * info from the OF node of root host bridge's parent.
> + */
> +void of_pci_dma_configure(struct pci_dev *pci_dev)
> +{
> + struct device *dev = &pci_dev->dev;
> + struct device_node *parent_np;
> +
> + parent_np = of_get_pci_root_bridge_parent(pci_dev);
> + of_dma_configure(dev, parent_np);
> +}
> +EXPORT_SYMBOL_GPL(of_pci_dma_configure);

Whilst I think this is the right overall structure, I think this function
should determine the set of DMA aliases for the device and pass that through
to the IOMMU (as mentioned in my reply to the cover letter). Then we just
need to work out what we're doing for groups.

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

Re: [PATCH v3 0/4] PCI: get DMA configuration from parent device

[Will Deacon]

On Thu, Jan 08, 2015 at 08:56:39AM +, Arnd Bergmann wrote:
> On Wednesday 07 January 2015 18:04:41 Murali Karicheri wrote:
> > On 01/07/2015 04:18 PM, Arnd Bergmann wrote:
> > > On Wednesday 07 January 2015 13:49:50 Murali Karicheri wrote:
> > >> PCI devices on Keystone doesn't have correct dma_pfn_offset set. This 
> > >> patch
> > >> add capability to set the dma configuration such as dma-mask, 
> > >> dma_pfn_offset,
> > >> and dma ops etc using the information from DT. The prior RFCs and 
> > >> discussions
> > >> are available at [1] and [2] below.
> > >>
> > >> [2] : 
> > >> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg790244.html
> > >> [1] : http://www.gossamer-threads.com/lists/linux/kernel/2024591
> > >
> > > Looks all good to me in this version, I'm just unsure about one thing:
> > >
> > >> - Limit the of_iommu_configure to non pci devices
> > >
> > > My last recommendation was to pass the b/d/f number into
> > > of_pci_dma_configure to handle this correctly. What was your
> > > reason for not doing it in the end?
> > Arnd,
> > 
> > I had responded to this already on the list and reproduced below your 
> > remark and my response below from that thread.
> 
> Ah right, sorry for missing a reply on that.
> 
> > ---cut---
> >  > Actually regarding the bit I wrote above, it might be helpful to pass
> >  > the PCI_DEVID() into both of_iommu_configure and of_dma_configure.
> >  >
> >  > While this may or may not be sufficient, I think there is no question
> >  > about it being needed for the ARM SMMU with PCI, so we may as well add
> >  > it at the point when you touch the same lines already. In the platform
> >  > bus case, just pass zero here.
> > 
> > PCI_DEVID() is defined as
> > 
> > #define PCI_DEVID(bus, devfn)  u16)(bus)) << 8) | (devfn))
> > 
> > So PCI_DEVID of 0 is a valid PCI DEVID.
> 
> I think that is ok: the idea would be that we always just list the
> first ID of the range and then let the iommu driver add the offset
> in the appropriate way.
> 
> > How about checking if the device is PCI in of_iommu_configure() using 
> > dev_is_pci(dev) macro and return immediately for PCI? Need to include 
> > pci.h in this file though.
> > 
> > Some of the iommu drivers already include this.
> 
> I guess you are right, but the driver can even handle the PCI
> device correctly without the extra parameter:
> 
>   devid = 0;
>   if (dev_is_pci(dev)) {
>   struct pci_dev *pdev = to_pci_dev(dev);
>   devid = PCI_DEVID(pdev->bus->number, pdev->devfn);
>   }
> 
>   streamid += devid;
> 
> Other IOMMUs won't even care about the devid, so they will just work.

I started looking at this in more depth for the ARM SMMU at the end of last
year and the PCI case ends up being remarkably similar to the DT case.

Although I initially thought we just needed the devid, actually we need to
walk the PCI topology to find the dma alias for the device. The current
code for doing that (iommu_group_get_for_pci_dev) only reports the resulting
IOMMu group, which can actually correspond to *multiple* DMA aliases due
to ACS restrictions.

I knocked up a couple of patches for dealing with this in the ARM SMMU
driver, but I'd really like that to be part of core code if possible :)

https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/commit/?h=iommu/pgtbl&id=41dc7b9e0ff1a4d79d1dd76619056fc0cfa8b84f
https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/commit/?h=iommu/pgtbl&id=5503996103138b41520d5eae90dda62abb65f04f

So, I think of_pci_dma_configure should determine the set of aliases for the
group. What isn't clear to me for the DT *or* the PCI cases is what we
actually do with the group at the dma-mapping level...

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