Re: [PATCH v5 3/8] arm64: hyperv: Add memory alloc/free functions for Hyper-V size pages

[Marc Zyngier]

On 2019-10-03 20:12, Michael Kelley wrote:

Add ARM64-specific code to allocate memory with HV_HYP_PAGE_SIZE
size and alignment. These are for use when pages need to be shared
with Hyper-V. Separate functions are needed as the page size used
by Hyper-V may not be the same as the guest page size.  Free
operations are rarely done, so no attempt is made to combine
freed pages into larger chunks.

This code is built only when CONFIG_HYPERV is enabled.

Signed-off-by: Michael Kelley 
---
 arch/arm64/hyperv/hv_init.c| 68
++
 include/asm-generic/mshyperv.h |  5 
 2 files changed, 73 insertions(+)

diff --git a/arch/arm64/hyperv/hv_init.c 
b/arch/arm64/hyperv/hv_init.c

index 6808bc8..9c294f6 100644
--- a/arch/arm64/hyperv/hv_init.c
+++ b/arch/arm64/hyperv/hv_init.c
@@ -15,10 +15,78 @@
 #include 
 #include 
 #include 
+#include 
+#include 
+#include 
 #include 
 #include 
 #include 

+
+/*
+ * Functions for allocating and freeing memory with size and
+ * alignment HV_HYP_PAGE_SIZE. These functions are needed because
+ * the guest page size may not be the same as the Hyper-V page
+ * size. And while kalloc() could allocate the memory, it does not
+ * guarantee the required alignment. So a separate small memory
+ * allocator is needed.  The free function is rarely used, so it
+ * does not try to combine freed pages into larger chunks.


Is this still needed now that kmalloc has alignment guarantees
(see 59bb47985c1d)?

M.
--
Jazz is not dead. It just smells funny...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH v5 2/8] arm64: hyperv: Add hypercall and register access functions

[Marc Zyngier]

On 2019-11-06 19:08, Michael Kelley wrote:

From: Marc Zyngier   Sent: Wednesday, November 6,
2019 2:20 AM


On 2019-10-03 20:12, Michael Kelley wrote:
> Add ARM64-specific code to make Hyper-V hypercalls and to
> access virtual processor synthetic registers via hypercalls.
> Hypercalls use a Hyper-V specific calling sequence with a non-zero
> immediate value per Section 2.9 of the SMC Calling Convention
> spec.

I find this "following the spec by actively sidestepping it" counter
productive. You (or rather the Hyper-V people) are reinventing the
wheel (of the slightly square variety) instead of using the standard
that the whole of the ARM ecosystem seems happy to take advantage
of.

I wonder what is the rational for this. If something doesn't quite
work for Hyper-V, I think we'd all like to know.



I'll go another round internally with the Hyper-V people on this
topic and impress upon them the desire of the Linux community to
have Hyper-V adopt the true spirit of the spec.  But I know they are
fairly set in their approach at this point, regardless of the 
technical
merits or lack thereof.  Hyper-V is shipping and in use as a 
commercial

product on ARM64 hardware, which makes it harder to change.  I
hope we can find a way to avoid a complete impasse 


Hyper-V shipping with their own calling convention is fine by me. Linux
having to implement multiple calling conventions because the Hyper-V
folks refuse (for undisclosed reason) to adopt the standard isn't fine 
at

all.

HV can perfectly retain its interface for Windows or other things, but
please *at least* implement the standard interface on which all 
existing

operating systems rely.

Thanks,

M.
--
Jazz is not dead. It just smells funny...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 2/8] arm64: hyperv: Add hypercall and register access functions

[Marc Zyngier]

On 2019-10-03 20:12, Michael Kelley wrote:

Add ARM64-specific code to make Hyper-V hypercalls and to
access virtual processor synthetic registers via hypercalls.
Hypercalls use a Hyper-V specific calling sequence with a non-zero
immediate value per Section 2.9 of the SMC Calling Convention
spec.


I find this "following the spec by actively sidestepping it" counter
productive. You (or rather the Hyper-V people) are reinventing the
wheel (of the slightly square variety) instead of using the standard
that the whole of the ARM ecosystem seems happy to take advantage
of.

I wonder what is the rational for this. If something doesn't quite
work for Hyper-V, I think we'd all like to know.

Thanks,

M.
--
Jazz is not dead. It just smells funny...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/4] arm64: hyperv: Add support for Hyper-V as a hypervisor

[Marc Zyngier]

Hi Michael,

On 12/12/2018 05:00, Michael Kelley wrote:
> From: Marc Zyngier   Sent: Friday, December 7, 2018 
> 6:43 AM
> 
>>> Add ARM64-specific code to enable Hyper-V. This code includes:
>>> * Detecting Hyper-V and initializing the guest/Hyper-V interface
>>> * Setting up Hyper-V's synthetic clocks
>>> * Making hypercalls using the HVC instruction
>>> * Setting up VMbus and stimer0 interrupts
>>> * Setting up kexec and crash handlers
>>
>> This commit message is a clear indication that this should be split in
>> at least 5 different patches.
> 
> OK, I'll work on separating into multiple layered patches in the next
> version.

Thanks. This will definitely help the review process.

>>> +/*
>>> + * This variant of HVC invocation is for hv_get_vpreg and
>>> + * hv_get_vpreg_128. The input parameters are passed in registers
>>> + * along with a pointer in x4 to where the output result should
>>> + * be stored. The output is returned in x15 and x16.  x18 is used as
>>> + * scratch space to avoid buildng a stack frame, as Hyper-V does
>>> + * not preserve registers x0-x17.
>>> + */
>>> +ENTRY(hv_do_hvc_fast_get)
>>> +   mov x18, x4
>>> +   hvc #1
>>> +   str x15,[x18]
>>> +   str x16,[x18,#8]
>>> +   ret
>>> +ENDPROC(hv_do_hvc_fast_get)
>>
>> As Will said, this isn't a viable option. Please follow SMCCC 1.1.
> 
> I'll have to start a conversation with the Hyper-V team about this.
> I don't know why they chose to use HVC #1 or this register scheme
> for output values.  It may be tough to change at this point because
> there are Windows guests on Hyper-V for ARM64 that are already
> using this approach.

I appreciate you already have stuff in the wild, but there is definitely
a case to be made for supporting architecturally specified mechanisms in
a hypervisor, and SMCCC is definitely part of it (I'm certainly curious
of how you support the Spectre mitigation otherwise).

> 
>>
>>> diff --git a/arch/arm64/hyperv/hv_init.c b/arch/arm64/hyperv/hv_init.c
>>> new file mode 100644
>>> index ..aa1a8c09d989
>>> --- /dev/null
>>> +++ b/arch/arm64/hyperv/hv_init.c
>>> @@ -0,0 +1,441 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>>> +/*
>>> + * Initialization of the interface with Microsoft's Hyper-V hypervisor,
>>> + * and various low level utility routines for interacting with Hyper-V.
>>> + *
>>> + * Copyright (C) 2018, Microsoft, Inc.
>>> + *
>>> + * Author : Michael Kelley 
>>> + *
>>> + * 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, GOOD TITLE or
>>> + * NON INFRINGEMENT.  See the GNU General Public License for more
>>> + * details.
>>> + */
>>> +
>>> +
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +static boolhyperv_initialized;
>>> +struct ms_hyperv_info ms_hyperv;
>>> +EXPORT_SYMBOL_GPL(ms_hyperv);
>>
>> Who are the users of this structure? Should they go via accessors instead?
> 
> The structure is an aggregation of several flags fields that describe a myriad
> of features and hints that may or may not be present on any particular version
> of Hyper-V, plus the max virtual processor ID values.  Everything is read-only
> after initialization.   

nit: please consider using a __ro_after_init annotation in that case.

> Most of the references are to test one of the flags.  It's
> a judgment call, but there are a lot of different flags with long names, and
> writing accessors for each one doesn't seem to me to add any clarity.

Looking at that structure, it doesn't seem so bad, and you could easily
have generic accessors that take a flag as a parameter. Your call.

[...]

>>> +static struct clocksourc

Re: [PATCH 2/4] arm64: hyperv: Add support for Hyper-V as a hypervisor

[Marc Zyngier]

On 22/11/2018 03:10, k...@linuxonhyperv.com wrote:
> From: Michael Kelley 
> 
> Add ARM64-specific code to enable Hyper-V. This code includes:
> * Detecting Hyper-V and initializing the guest/Hyper-V interface
> * Setting up Hyper-V's synthetic clocks
> * Making hypercalls using the HVC instruction
> * Setting up VMbus and stimer0 interrupts
> * Setting up kexec and crash handlers

This commit message is a clear indication that this should be split in
at least 5 different patches.

> This code is architecture dependent code and is mostly driven by
> architecture independent code in the VMbus driver in drivers/hv/hv.c
> and drivers/hv/vmbus_drv.c.
> 
> This code is built only when CONFIG_HYPERV is enabled.
> 
> Signed-off-by: Michael Kelley 
> Signed-off-by: K. Y. Srinivasan 
> ---
>  MAINTAINERS  |   1 +
>  arch/arm64/Makefile  |   1 +
>  arch/arm64/hyperv/Makefile   |   2 +
>  arch/arm64/hyperv/hv_hvc.S   |  54 +
>  arch/arm64/hyperv/hv_init.c  | 441 +++
>  arch/arm64/hyperv/mshyperv.c | 178 ++
>  6 files changed, 677 insertions(+)
>  create mode 100644 arch/arm64/hyperv/Makefile
>  create mode 100644 arch/arm64/hyperv/hv_hvc.S
>  create mode 100644 arch/arm64/hyperv/hv_init.c
>  create mode 100644 arch/arm64/hyperv/mshyperv.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 72f19cef4c48..326eeb32a0cd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6837,6 +6837,7 @@ F:  arch/x86/kernel/cpu/mshyperv.c
>  F:   arch/x86/hyperv
>  F:   arch/arm64/include/asm/hyperv-tlfs.h
>  F:   arch/arm64/include/asm/mshyperv.h
> +F:   arch/arm64/hyperv
>  F:   drivers/hid/hid-hyperv.c
>  F:   drivers/hv/
>  F:   drivers/input/serio/hyperv-keyboard.c
> diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
> index 6cb9fc7e9382..ad9ec0579553 100644
> --- a/arch/arm64/Makefile
> +++ b/arch/arm64/Makefile
> @@ -106,6 +106,7 @@ core-y+= arch/arm64/kernel/ arch/arm64/mm/
>  core-$(CONFIG_NET) += arch/arm64/net/
>  core-$(CONFIG_KVM) += arch/arm64/kvm/
>  core-$(CONFIG_XEN) += arch/arm64/xen/
> +core-$(CONFIG_HYPERV) += arch/arm64/hyperv/
>  core-$(CONFIG_CRYPTO) += arch/arm64/crypto/
>  libs-y   := arch/arm64/lib/ $(libs-y)
>  core-$(CONFIG_EFI_STUB) += $(objtree)/drivers/firmware/efi/libstub/lib.a
> diff --git a/arch/arm64/hyperv/Makefile b/arch/arm64/hyperv/Makefile
> new file mode 100644
> index ..988eda55330c
> --- /dev/null
> +++ b/arch/arm64/hyperv/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0
> +obj-y:= hv_init.o hv_hvc.o mshyperv.o
> diff --git a/arch/arm64/hyperv/hv_hvc.S b/arch/arm64/hyperv/hv_hvc.S
> new file mode 100644
> index ..82636969b4f2
> --- /dev/null
> +++ b/arch/arm64/hyperv/hv_hvc.S
> @@ -0,0 +1,54 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +/*
> + * Microsoft Hyper-V hypervisor invocation routines
> + *
> + * Copyright (C) 2018, Microsoft, Inc.
> + *
> + * Author : Michael Kelley 
> + *
> + * 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, GOOD TITLE or
> + * NON INFRINGEMENT.  See the GNU General Public License for more
> + * details.
> + */
> +
> +#include 
> +
> + .text
> +/*
> + * Do the HVC instruction.  For Hyper-V the argument is always 1.
> + * x0 contains the hypercall control value, while additional registers
> + * vary depending on the hypercall, and whether the hypercall arguments
> + * are in memory or in registers (a "fast" hypercall per the Hyper-V
> + * TLFS).  When the arguments are in memory x1 is the guest physical
> + * address of the input arguments, and x2 is the guest physical
> + * address of the output arguments.  When the arguments are in
> + * registers, the register values depends on the hypercall.  Note
> + * that this version cannot return any values in registers.
> + */
> +ENTRY(hv_do_hvc)
> + hvc #1
> + ret
> +ENDPROC(hv_do_hvc)
> +
> +/*
> + * This variant of HVC invocation is for hv_get_vpreg and
> + * hv_get_vpreg_128. The input parameters are passed in registers
> + * along with a pointer in x4 to where the output result should
> + * be stored. The output is returned in x15 and x16.  x18 is used as
> + * scratch space to avoid buildng a stack frame, as Hyper-V does
> + * not preserve registers x0-x17.
> + */
> +ENTRY(hv_do_hvc_fast_get)
> + mov x18, x4
> + hvc #1
> + str x15,[x18]
> + str x16,[x18,#8]
> + ret
> +ENDPROC(hv_do_hvc_fast_get)

As Will said, this isn't a viable option. Please follow SMCCC 1.1.

> diff --git a/arch/arm64/hyperv/hv_init.c b/arch/arm64/hyperv/hv_init.c
> new file mode 100644
> index 

Re: [PATCH v5 2/2] staging: fsl-mc: Move irqchip code out of staging

[Marc Zyngier]

On 26/01/18 12:51, Bogdan Purcareata wrote:
> Now that the fsl-mc bus core infrastructure is out of staging, the
> remaining irqchip glue code used (irq-gic-v3-its-fsl-mc-msi.c) goes
> to drivers/irqchip.
> 
> Signed-off-by: Stuart Yoder <stuyo...@gmail.com>
> [rebased, add dpaa2_eth and dpio #include updates]
> Signed-off-by: Laurentiu Tudor <laurentiu.tu...@nxp.com>
> [rebased, split irqchip to separate patch]
> Signed-off-by: Bogdan Purcareata <bogdan.purcare...@nxp.com>
> Cc: Thomas Gleixner <t...@linutronix.de>
> Cc: Jason Cooper <ja...@lakedaemon.net>
> Cc: Marc Zyngier <marc.zyng...@arm.com>
> ---
> Notes:
> -v5:
>   - split irqchip glue code to separate patch (GregKH)
> -v4 - v1:
>   - no change
> 
>  drivers/irqchip/Makefile   |   1 +
>  drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c| 100 
> +
>  drivers/staging/fsl-mc/bus/Makefile|   3 +-
>  .../staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c | 100 
> -
>  4 files changed, 102 insertions(+), 102 deletions(-)
>  create mode 100644 drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c
>  delete mode 100644 drivers/staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c
> 
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index d2df34a..641d8a4 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -32,6 +32,7 @@ obj-$(CONFIG_ARM_GIC_V2M)   += irq-gic-v2m.o
>  obj-$(CONFIG_ARM_GIC_V3) += irq-gic-v3.o irq-gic-common.o
>  obj-$(CONFIG_ARM_GIC_V3_ITS) += irq-gic-v3-its.o 
> irq-gic-v3-its-platform-msi.o irq-gic-v4.o
>  obj-$(CONFIG_ARM_GIC_V3_ITS_PCI) += irq-gic-v3-its-pci-msi.o
> +obj-$(CONFIG_FSL_MC_BUS) += irq-gic-v3-its-fsl-mc-msi.o
>  obj-$(CONFIG_PARTITION_PERCPU)   += irq-partition-percpu.o
>  obj-$(CONFIG_HISILICON_IRQ_MBIGEN)   += irq-mbigen.o
>  obj-$(CONFIG_ARM_NVIC)   += irq-nvic.o
> diff --git a/drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c 
> b/drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c
> new file mode 100644
> index 000..b365fbb
> --- /dev/null
> +++ b/drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c
> @@ -0,0 +1,100 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Freescale Management Complex (MC) bus driver MSI support
> + *
> + * Copyright (C) 2015-2016 Freescale Semiconductor, Inc.
> + * Author: German Rivera <german.riv...@freescale.com>
> + *
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +static struct irq_chip its_msi_irq_chip = {
> + .name = "ITS-fMSI",
> + .irq_mask = irq_chip_mask_parent,
> + .irq_unmask = irq_chip_unmask_parent,
> + .irq_eoi = irq_chip_eoi_parent,
> + .irq_set_affinity = msi_domain_set_affinity
> +};
> +
> +static int its_fsl_mc_msi_prepare(struct irq_domain *msi_domain,
> +   struct device *dev,
> +   int nvec, msi_alloc_info_t *info)
> +{
> + struct fsl_mc_device *mc_bus_dev;
> + struct msi_domain_info *msi_info;
> +
> + if (!dev_is_fsl_mc(dev))
> + return -EINVAL;
> +
> + mc_bus_dev = to_fsl_mc_device(dev);
> + if (!(mc_bus_dev->flags & FSL_MC_IS_DPRC))
> + return -EINVAL;
> +
> + /*
> +  * Set the device Id to be passed to the GIC-ITS:
> +  *
> +  * NOTE: This device id corresponds to the IOMMU stream ID
> +  * associated with the DPRC object (ICID).
> +  */
> +#ifdef GENERIC_MSI_DOMAIN_OPS
> + info->scratchpad[0].ul = mc_bus_dev->icid;
> +#endif

I'd really like to avoid this kind of condition in irqchip drivers.
Either the architecture you're targeting this at can deal with it, and
you can compile this driver, or it doesn't, and you really shouldn't
offer it. And given that this thing is 100% specific to the ARM GICv3
ITS, you should really have a dependency on it.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: fsl-mc: move bus driver out of staging

[Marc Zyngier]

On 19/08/17 18:18, laurentiu.tu...@nxp.com wrote:
> From: Stuart Yoder <stuart.yo...@nxp.com>
> 
> Move the source files out of staging into their final locations:
>   -include files in drivers/staging/fsl-mc/include go to include/linux/fsl
>   -irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip
>   -source in drivers/staging/fsl-mc/bus goes to drivers/bus/fsl-mc
>   -README.txt, providing and overview of DPAA goes to
>Documentation/dpaa2/overview.txt
> 
> Update or delete other remaining staging files-- Makefile, Kconfig, TODO.
> Update dpaa2_eth and dpio staging drivers.
> 
> Signed-off-by: Stuart Yoder <stuyo...@gmail.com>
> Signed-off-by: Laurentiu Tudor <laurentiu.tu...@nxp.com>
> [Laurentiu: rebased, add dpaa2_eth and dpio #include updates]
> Cc: Thomas Gleixner <t...@linutronix.de>
> Cc: Jason Cooper <ja...@lakedaemon.net>
> Cc: Marc Zyngier <marc.zyng...@arm.com>

[...]

> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index e88d856..4839165 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -78,3 +78,4 @@ obj-$(CONFIG_EZNPS_GIC) += irq-eznps.o
>  obj-$(CONFIG_ARCH_ASPEED)+= irq-aspeed-vic.o irq-aspeed-i2c-ic.o
>  obj-$(CONFIG_STM32_EXTI) += irq-stm32-exti.o
>  obj-$(CONFIG_QCOM_IRQ_COMBINER)  += qcom-irq-combiner.o
> +obj-$(CONFIG_FSL_MC_BUS) += irq-gic-v3-its-fsl-mc-msi.o

Please keep the ITS bus glue next to the rest of the ITS code.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 10/10] staging: fsl-mc: move bus driver out of staging

[Marc Zyngier]

On 31/05/17 11:58, laurentiu.tu...@nxp.com wrote:
> From: Stuart Yoder <stuart.yo...@nxp.com>
> 
> Move the source files out of staging into their final locations:
>   -include files in drivers/staging/fsl-mc/include go to include/linux/fsl
>   -irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip
>   -source in drivers/staging/fsl-mc/bus goes to drivers/bus/fsl-mc
>   -README.txt, providing and overview of DPAA goes to
>Documentation/dpaa2/overview.txt
> 
> Update or delete other remaining staging files-- Makefile, Kconfig, TODO.
> Update dpaa2_eth and dpio staging drivers.
> 
> Signed-off-by: Stuart Yoder <stuyo...@gmail.com>
> Signed-off-by: Laurentiu Tudor <laurentiu.tu...@nxp.com>
> [Laurentiu: rebased, add dpaa2_eth and dpio #include updates]
> Cc: Thomas Gleixner <t...@linutronix.de>
> Cc: Jason Cooper <ja...@lakedaemon.net>
> Cc: Marc Zyngier <marc.zyng...@arm.com>
> ---
> 
> Notes:
> -no changes since v4
> -v4
>   -rebased
>   -update existing dpaa2 drivers to work with the bus out of staging
> -v3
>   -no changes
> -v2
>   -updated MAINTAINERS with new location
> 
>  .../README.txt => Documentation/dpaa2/overview.txt|  0

I thought you had agreed to drop all references to the userspace
interface (restool and co) which is completely absent from this code?
I'm certainly not going to ack this until this is done.

Also, please CC me on the whole series, as I'd really like to see things
in context, and not a patch that just moves things around.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/3][v4] staging: fsl-mc: move bus driver out of staging

[Marc Zyngier]

On 22/05/17 09:42, Laurentiu Tudor wrote:
> Hi Marc,
> 
>> -Original Message-----
>> From: Marc Zyngier [mailto:marc.zyng...@arm.com]
>> Sent: Monday, May 22, 2017 10:41 AM
>>
>> On Mon, May 22 2017 at  7:12:39 am GMT, Laurentiu Tudor
>> <laurentiu.tu...@nxp.com> wrote:
>>
>> Hi Laurentiu,
>>
>>> Hi Marc,
>>>
>>>> -Original Message-
>>>> From: Marc Zyngier [mailto:marc.zyng...@arm.com]
>>>> Sent: Saturday, May 20, 2017 9:43 AM
>>>> To: Matthias Brugger <matthias@gmail.com>
>>>>
>>>> On Fri, May 19 2017 at 02:41:43 PM, Matthias Brugger
>>>> <matthias@gmail.com> wrote:
>>>>> On 19/05/17 15:13, laurentiu.tu...@nxp.com wrote:
>>>>>> From: Stuart Yoder <stuart.yo...@nxp.com>
>>>>>>
>>>>>> Move the source files out of staging into their final locations:
>>>>>>-include files in drivers/staging/fsl-mc/include go to 
>>>>>> include/linux/fsl
>>>>>>-irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip
>>>>>
>>>>> This driver has as compatible "arm,gic-v3-its". I wonder if this is
>>>>> correct and if it should be moved like this out of staging.
>>>>
>>>> This is no different from the way we handle *any* bus that uses the
>>>> GICv3 ITS as an MSI controller. Each bus provides its glue code that
>>>> latches onto the ITS node, and calls into the generic code.
>>>>
>>>> Now, when it comes to moving this out of staging, here is my concern:
>>>> There is mention of a userspace tool (restool) used to control the
>>>> HW. Where is this tool? Where is the user ABI documented?
>>>
>>> The tool is published here:
>>>
>>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit
>>> hub.com%2Fqoriq-open-
>> source%2Frestool=01%7C01%7Claurentiu.tudor%4
>>>
>> 0nxp.com%7Cd3c05908969d499cd4a008d4a0e5eaae%7C686ea1d3bc2b4c6fa92
>> cd99c
>>>
>> 5c301635%7C0=2sEXCZ%2BAFlTtle8N3yWJPsGRve8cXMRPzyumlwqOhbg
>> %3D
>>> served=0
>>>
>>> There are two ways of configuring the mc-bus:
>>>  - a static one, through a FDT based configuration file (we call it
>>> DPL), documented in the refman linked below, chapter 22.
>>>  - a dynamic one, using this restool utility.
>>> Please note the usage of restool is optional.
>>
>> Optional or not, it still is a userspace ABI, and while I can see restool 
>> issuing ioctl
>> system calls to configure the HW, I cannot see the corresponding code in the
>> kernel tree. So how does it work?
>> If the syscall interface is not present in the mainline kernel, drop the 
>> reference
>> to it in the documentation. If it is there (and I obviously missed it), 
>> document it,
>> and get it reviewed. 
> 
> Our original plan was to first get the bus out of staging and after that 
> submit the restool support ASAP (patches are done - so I'm thinking at few 
> days timeframe).
> if this is not acceptable, I can drop the restool reference from the README 
> and resubmit the patch series. We'll re-add the reference together with the 
> restool support patches.

I think it would make a lot more sense to drop anything that is not
implemented by the current code. Once you have patches that implement
this userspace interface, they can be reviewed together with the
corresponding documentation.

>> If there are associated DT bindings to the kernel code, they
>> must be documented (and reviewed) as part of the device-tree documentation,
>> and not in some obscure, hard to access document.
> 
> There's only one binding involved and it's already accepted [1].

Ah, I missed it. It would be good to mention it in the documentation as
well.

> [snip]
> 
>>>
>>> We're also working on publishing the docs on github so that they're
>>> more accessible.
>>
>> That'd be great, because the way the registration process is presented, I'd 
>> have
>> to agree to the Access Agreement *before* having a chance to read it. Not
>> going to happen...
> 
> Sorry about that. Not much I can do. :-( 

I understand this is not your job ;-). But maybe making people inside
NXP aware of the issue would help... Oh well.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/3][v4] staging: fsl-mc: move bus driver out of staging

[Marc Zyngier]

On Mon, May 22 2017 at  7:12:39 am GMT, Laurentiu Tudor 
<laurentiu.tu...@nxp.com> wrote:

Hi Laurentiu,

> Hi Marc,
>
>> -Original Message-----
>> From: Marc Zyngier [mailto:marc.zyng...@arm.com]
>> Sent: Saturday, May 20, 2017 9:43 AM
>> To: Matthias Brugger <matthias@gmail.com>
>> Cc: Laurentiu Tudor <laurentiu.tu...@nxp.com>; gre...@linuxfoundation.org;
>> stuyo...@gmail.com; de...@driverdev.osuosl.org; a...@arndb.de; Ruxandra
>> Ioana Radulescu <ruxandra.radule...@nxp.com>; Stuart Yoder
>> <stuart.yo...@nxp.com>; Roy Pledge <roy.ple...@nxp.com>; linux-
>> ker...@vger.kernel.org; ag...@suse.de; Catalin Horghidan
>> <catalin.horghi...@nxp.com>; Ioana Ciornei <ioana.cior...@nxp.com>;
>> Thomas Gleixner <t...@linutronix.de>; Leo Li <leoyang...@nxp.com>; Bharat
>> Bhushan <bharat.bhus...@nxp.com>; Jason Cooper <ja...@lakedaemon.net>;
>> linux-arm-ker...@lists.infradead.org; Rob Herring <robh...@kernel.org>
>> Subject: Re: [PATCH 3/3][v4] staging: fsl-mc: move bus driver out of staging
>> Importance: High
>> 
>> On Fri, May 19 2017 at 02:41:43 PM, Matthias Brugger
>> <matthias@gmail.com> wrote:
>> > On 19/05/17 15:13, laurentiu.tu...@nxp.com wrote:
>> >> From: Stuart Yoder <stuart.yo...@nxp.com>
>> >>
>> >> Move the source files out of staging into their final locations:
>> >>-include files in drivers/staging/fsl-mc/include go to 
>> >> include/linux/fsl
>> >>-irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip
>> >
>> > This driver has as compatible "arm,gic-v3-its". I wonder if this is
>> > correct and if it should be moved like this out of staging.
>> 
>> This is no different from the way we handle *any* bus that uses the
>> GICv3 ITS as an MSI controller. Each bus provides its glue code that
>> latches onto
>> the ITS node, and calls into the generic code.
>> 
>> Now, when it comes to moving this out of staging, here is my concern:
>> There is mention of a userspace tool (restool) used to control the
>> HW. Where is
>> this tool? Where is the user ABI documented?
>
> The tool is published here:
>
> https://github.com/qoriq-open-source/restool
>
> There are two ways of configuring the mc-bus:
>  - a static one, through a FDT based configuration file (we call it
> DPL), documented in the refman linked below, chapter 22.
>  - a dynamic one, using this restool utility.
> Please note the usage of restool is optional.

Optional or not, it still is a userspace ABI, and while I can see
restool issuing ioctl system calls to configure the HW, I cannot see the
corresponding code in the kernel tree. So how does it work?

If the syscall interface is not present in the mainline kernel, drop the
reference to it in the documentation. If it is there (and I obviously
missed it), document it, and get it reviewed. If there are associated DT
bindings to the kernel code, they must be documented (and reviewed) as
part of the device-tree documentation, and not in some obscure, hard to
access document.

>
> The reference manual documenting the ABI can be found here
> (registration required):
>
> https://freescale.sdlproducts.com/LiveContent/content/en-US/QorIQ_SDK/GUID-53BEBDD8-1A5E-4DD0-8354-A9647AD35755
>
> Click on the DPAA2 user manual link.
>
> We're also working on publishing the docs on github so that they're
> more accessible.

That'd be great, because the way the registration process is presented,
I'd have to agree to the Access Agreement *before* having a chance to
read it. Not going to happen...

Thanks,

M.
-- 
Jazz is not dead, it just smell funny.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/3][v4] staging: fsl-mc: move bus driver out of staging

[Marc Zyngier]

On Fri, May 19 2017 at 02:41:43 PM, Matthias Brugger  
wrote:
> On 19/05/17 15:13, laurentiu.tu...@nxp.com wrote:
>> From: Stuart Yoder 
>>
>> Move the source files out of staging into their final locations:
>>-include files in drivers/staging/fsl-mc/include go to include/linux/fsl
>>-irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip
>
> This driver has as compatible "arm,gic-v3-its". I wonder if this is
> correct and if it should be moved like this out of staging.

This is no different from the way we handle *any* bus that uses the
GICv3 ITS as an MSI controller. Each bus provides its glue code that
latches onto the ITS node, and calls into the generic code.

Now, when it comes to moving this out of staging, here is my concern:
There is mention of a userspace tool (restool) used to control the
HW. Where is this tool? Where is the user ABI documented?

Thanks,

M.
-- 
Jazz is not dead. It just smells funny.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4/6] staging: fsl-mc: Use platform_msi_* infrastructure

[Marc Zyngier]

On 13/04/16 12:23, Matthias Brugger wrote:
> 
> 
> On 13/04/16 12:56, Marc Zyngier wrote:
>> On 13/04/16 11:30, Matthias Brugger wrote:
>>> From: Matthias Brugger <matthias@gmail.com>
>>>
>>> The fsl-mc driver can't be build as a module because it uses msi_*
>>> functions directly. Port the driver to use the platform_msi_*
>>> infrastructure instead, to allow it to be build as a module.
>>>
>>> Signed-off-by: Matthias Brugger <mbrug...@suse.com>
>>> ---
>>>   .../staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c |   5 +-
>>>   drivers/staging/fsl-mc/bus/mc-allocator.c  |   9 +-
>>>   drivers/staging/fsl-mc/bus/mc-msi.c| 169 
>>> +
>>>   drivers/staging/fsl-mc/include/mc-sys.h|   3 +
>>>   4 files changed, 14 insertions(+), 172 deletions(-)
>>>
>>> diff --git a/drivers/staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c 
>>> b/drivers/staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c
>>> index 720e2b0..0eecb7e 100644
>>> --- a/drivers/staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c
>>> +++ b/drivers/staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c
>>> @@ -25,7 +25,6 @@ static struct irq_chip its_msi_irq_chip = {
>>> .irq_mask = irq_chip_mask_parent,
>>> .irq_unmask = irq_chip_unmask_parent,
>>> .irq_eoi = irq_chip_eoi_parent,
>>> -   .irq_set_affinity = msi_domain_set_affinity
>>>   };
>>>
>>>   static int its_fsl_mc_msi_prepare(struct irq_domain *msi_domain,
>>> @@ -86,7 +85,7 @@ int __init its_fsl_mc_msi_init(void)
>>> continue;
>>> }
>>>
>>> -   mc_msi_domain = fsl_mc_msi_create_irq_domain(
>>> +   mc_msi_domain = platform_msi_create_irq_domain(
>>>  of_node_to_fwnode(np),
>>>  _fsl_mc_msi_domain_info,
>>>  parent);
>>
>> What? We are already creating a platform MSI domain for the ITS. How is
>> that going to work? If you want to convert this set of drivers to
>> platform ITS, fine. But you can't randomly hack in the ITS code and pray
>> for things not to fall apart.
>>
> 
>  From what I see, the difference between irq-gic-v3-its-fsl-mc-msi and 
> the irq-gic-v3-its-platform-msi is the way ITS specific DeviceID is 
> created in msi_prepare.

It is not "created". It is extracted from the HW, either by looking at
the RequesterID (PCI), at the DT (platform MSI), or a bus-specific method.

> German, is there a reason why you use the ICID read from the DPRC as dev_id?

Because that's what is presented to the ITS as a DevID. This is a HW
constraint, and you can't just change it.

If you want to use platform MSI for that, you also need to describe the
DevID in the DT (breaking the existing platforms in the process).

M.
-- 
Jazz is not dead. It just smells funny...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4/6] staging: fsl-mc: Use platform_msi_* infrastructure

[Marc Zyngier]

On 13/04/16 11:30, Matthias Brugger wrote:
> From: Matthias Brugger 
> 
> The fsl-mc driver can't be build as a module because it uses msi_*
> functions directly. Port the driver to use the platform_msi_*
> infrastructure instead, to allow it to be build as a module.
> 
> Signed-off-by: Matthias Brugger 
> ---
>  .../staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c |   5 +-
>  drivers/staging/fsl-mc/bus/mc-allocator.c  |   9 +-
>  drivers/staging/fsl-mc/bus/mc-msi.c| 169 
> +
>  drivers/staging/fsl-mc/include/mc-sys.h|   3 +
>  4 files changed, 14 insertions(+), 172 deletions(-)
> 
> diff --git a/drivers/staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c 
> b/drivers/staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c
> index 720e2b0..0eecb7e 100644
> --- a/drivers/staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c
> +++ b/drivers/staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c
> @@ -25,7 +25,6 @@ static struct irq_chip its_msi_irq_chip = {
>   .irq_mask = irq_chip_mask_parent,
>   .irq_unmask = irq_chip_unmask_parent,
>   .irq_eoi = irq_chip_eoi_parent,
> - .irq_set_affinity = msi_domain_set_affinity
>  };
>  
>  static int its_fsl_mc_msi_prepare(struct irq_domain *msi_domain,
> @@ -86,7 +85,7 @@ int __init its_fsl_mc_msi_init(void)
>   continue;
>   }
>  
> - mc_msi_domain = fsl_mc_msi_create_irq_domain(
> + mc_msi_domain = platform_msi_create_irq_domain(
>of_node_to_fwnode(np),
>_fsl_mc_msi_domain_info,
>parent);

What? We are already creating a platform MSI domain for the ITS. How is
that going to work? If you want to convert this set of drivers to
platform ITS, fine. But you can't randomly hack in the ITS code and pray
for things not to fall apart.

M.
-- 
Jazz is not dead. It just smells funny...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RESEND 1/3] PCI: Add fwnode_handle to pci_sysdata

[Marc Zyngier]

On 03/02/16 18:51, Bjorn Helgaas wrote:
> On Wed, Feb 03, 2016 at 06:32:20PM +, Jake Oshins wrote:
>>> -Original Message-
>>> From: Bjorn Helgaas [mailto:helg...@kernel.org]
>>> Sent: Wednesday, February 3, 2016 10:25 AM
>>> To: Jake Oshins 
>>> Cc: gre...@linuxfoundation.org; KY Srinivasan ; linux-
>>> ker...@vger.kernel.org; de...@linuxdriverproject.org; Haiyang Zhang
>>> ; marc.zyng...@arm.com;
>>> bhelg...@google.com; linux-...@vger.kernel.org
>>> Subject: Re: [PATCH RESEND 1/3] PCI: Add fwnode_handle to pci_sysdata
>>>
>>> Hi Jake,
>>>
>>> On Tue, Feb 02, 2016 at 05:41:41PM +, ja...@microsoft.com wrote:
> 
 diff --git a/include/linux/pci.h b/include/linux/pci.h index
 27df4a6..cd05a8e 100644
 --- a/include/linux/pci.h
 +++ b/include/linux/pci.h
 @@ -1515,6 +1515,10 @@ static inline int pci_get_new_domain_nr(void) {
 return -ENOSYS; }

  #include 

 +#ifndef pci_root_bus_fwnode
 +#define pci_root_bus_fwnode(bus)  ((void)(bus), NULL)
>>>
>>> Huh, interesting.  This is new for me; I guess the idea is that we at least
>>> evaluate "bus" even when pci_root_bus_fwnode isn't defined, so the
>>> compiler can catch egregious errors?
>>>
>>
>> This was a suggestion by Mark Zyngier.  It made the non-x86 architectures 
>> build benignly.  If you'd like it done differently, I'm open to suggestion.

I don't remember suggesting the use of the comma operator, but just to
check that pci_root_bus_fwnode wasn't previously defined.

> Something like "#define pci_root_bus_fwnode(bus) NULL" would be
> typical.  What I'm curious about is the use of the comma operator.
> I'm not opposed to it; I'm just trying to understand why it makes a
> difference.

I guess it flags the variable as used, and prevents an overly sensitive
compiler from being loud and obnoxious... Just a guess though.

M.
-- 
Jazz is not dead. It just smells funny...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v10 3/7] PCI: Make it possible to implement a PCI MSI IRQ Domain in a module.

[Marc Zyngier]

On 10/12/15 17:52, ja...@microsoft.com wrote:
> From: Jake Oshins <ja...@microsoft.com>
> 
> The Linux kernel already has the concpet of IRQ domain, wherein a
> component can expose a set of IRQs which are managed by a particular
> interrupt controller chip or other subsystem. The PCI driver exposes
> the notion of an IRQ domain for Message-Signaled Interrupts (MSI) from
> PCI Express devices. This patch exposes the functions which are
> necessary for making an MSI IRQ domain within a module.
> 
> Signed-off-by: Jake Oshins <ja...@microsoft.com>

Reviewed-by: Marc Zyngier <marc.zyng...@arm.com>

M.
-- 
Jazz is not dead. It just smells funny...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v10 5/7] PCI: irqdomain: Look up IRQ domain by fwnode_handle

[Marc Zyngier]

On 10/12/15 17:53, ja...@microsoft.com wrote:
> From: Jake Oshins <ja...@microsoft.com>
> 
> This patch adds a second way of finding an IRQ domain associated with
> a root PCI bus.  After looking to see if one can be found through
> the OF tree, it attempts to look up the IRQ domain through an
> fwnode_handle stored in the pci_sysdata struct.
> 
> Signed-off-by: Jake Oshins <ja...@microsoft.com>
> ---
>  drivers/pci/probe.c | 14 ++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 750f907..c6369dd 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -674,6 +674,20 @@ static struct irq_domain 
> *pci_host_bridge_msi_domain(struct pci_bus *bus)
>*/
>   d = pci_host_bridge_of_msi_domain(bus);
>  
> +#ifdef CONFIG_PCI_MSI_IRQ_DOMAIN
> + /*
> +  * If no IRQ domain was found via the OF tree, try looking it up
> +  * directly through the fwnode_handle.
> +  */
> + if (!d) {
> + struct fwnode_handle *fwnode = pci_root_bus_fwnode(bus);
> +
> + if (fwnode)
> + d = irq_find_matching_fwnode(fwnode,
> +          DOMAIN_BUS_PCI_MSI);
> + }
> +#endif
> +
>   return d;
>  }
>  
> 

Reviewed-by: Marc Zyngier <marc.zyng...@arm.com>

M.
-- 
Jazz is not dead. It just smells funny...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v10 4/7] PCI: Add fwnode_handle to pci_sysdata

[Marc Zyngier]

On 10/12/15 17:53, ja...@microsoft.com wrote:
> From: Jake Oshins <ja...@microsoft.com>
> 
> This patch adds an fwnode_handle to struct pci_sysdata, which is
> used by the next patch in the series when trying to locate an
> IRQ domain associated with a root PCI bus.
> 
> Signed-off-by: Jake Oshins <ja...@microsoft.com>
> ---
>  arch/x86/include/asm/pci.h | 15 +++
>  drivers/pci/probe.c|  1 +
>  include/linux/pci.h|  4 
>  3 files changed, 20 insertions(+)
> 
> diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h
> index 4625943..6fc3c7c 100644
> --- a/arch/x86/include/asm/pci.h
> +++ b/arch/x86/include/asm/pci.h
> @@ -20,6 +20,9 @@ struct pci_sysdata {
>  #ifdef CONFIG_X86_64
>   void*iommu; /* IOMMU private data */
>  #endif
> +#ifdef CONFIG_PCI_MSI_IRQ_DOMAIN
> + void*fwnode;/* IRQ domain for MSI assignment */
> +#endif
>  };
>  
>  extern int pci_routeirq;
> @@ -32,6 +35,7 @@ extern int noioapicreroute;
>  static inline int pci_domain_nr(struct pci_bus *bus)
>  {
>   struct pci_sysdata *sd = bus->sysdata;
> +
>   return sd->domain;
>  }
>  
> @@ -41,6 +45,17 @@ static inline int pci_proc_domain(struct pci_bus *bus)
>  }
>  #endif
>  
> +#ifdef CONFIG_PCI_MSI_IRQ_DOMAIN
> +static inline void *_pci_root_bus_fwnode(struct pci_bus *bus)
> +{
> + struct pci_sysdata *sd = bus->sysdata;
> +
> + return sd->fwnode;
> +}
> +
> +#define pci_root_bus_fwnode  _pci_root_bus_fwnode
> +#endif
> +
>  /* Can be used to override the logic in pci_scan_bus for skipping
> already-configured bus numbers - to be used for buggy BIOSes
> or architectures with incomplete PCI setup by the loader */
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index edb1984..750f907 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -15,6 +15,7 @@
>  #include 
>  #include 
>  #include 
> +#include 

Nit: this should be with patch 5, but that doesn't hurt. I don't think
you need to respin the series for this.

>  #include 
>  #include "pci.h"
>  
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 6ae25aa..b414422 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1517,6 +1517,10 @@ static inline int pci_get_new_domain_nr(void) { return 
> -ENOSYS; }
>  
>  #include 
>  
> +#ifndef pci_root_bus_fwnode
> +#define pci_root_bus_fwnode(bus) ((void)(bus), NULL)
> +#endif
> +
>  /* these helpers provide future and backwards compatibility
>   * for accessing popular PCI BAR info */
>  #define pci_resource_start(dev, bar) ((dev)->resource[(bar)].start)
> 

Reviewed-by: Marc Zyngier <marc.zyng...@arm.com>

M.
-- 
Jazz is not dead. It just smells funny...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RESEND v3 02/11] fsl-mc: msi: Added FSL-MC-specific member to the msi_desc's union

[Marc Zyngier]

On 24/11/15 22:31, J. German Rivera wrote:
> FSL-MC is a bus type different from PCI and platform, so it needs
> its own member in the msi_desc's union.
> 
> Signed-off-by: J. German Rivera <german.riv...@freescale.com>

Acked-by: Marc Zyngier <marc.zyng...@arm.com>

M.
-- 
Jazz is not dead. It just smells funny...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RESEND v3 04/11] staging: fsl-mc: Added GICv3-ITS support for FSL-MC MSIs

[Marc Zyngier]

On 24/11/15 22:31, J. German Rivera wrote:
> Added platform-specific MSI support layer for FSL-MC devices.
> 
> Signed-off-by: J. German Rivera 
> ---
> CHANGE HISTORY
> 
> Changes in v3: none
> 
> Changes in v2: none
> 
>  drivers/staging/fsl-mc/bus/Makefile|   1 +
>  .../staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c | 127 
> +
>  drivers/staging/fsl-mc/include/mc-private.h|   4 +
>  3 files changed, 132 insertions(+)
>  create mode 100644 drivers/staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c
> 
> diff --git a/drivers/staging/fsl-mc/bus/Makefile 
> b/drivers/staging/fsl-mc/bus/Makefile
> index a5f2ba4..e731517 100644
> --- a/drivers/staging/fsl-mc/bus/Makefile
> +++ b/drivers/staging/fsl-mc/bus/Makefile
> @@ -14,5 +14,6 @@ mc-bus-driver-objs := mc-bus.o \
> dprc-driver.o \
> mc-allocator.o \
> mc-msi.o \
> +   irq-gic-v3-its-fsl-mc-msi.o \
> dpmcp.o \
> dpbp.o
> diff --git a/drivers/staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c 
> b/drivers/staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c
> new file mode 100644
> index 000..5319afa
> --- /dev/null
> +++ b/drivers/staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c
> @@ -0,0 +1,127 @@
> +/*
> + * Freescale Management Complex (MC) bus driver MSI support
> + *
> + * Copyright (C) 2015 Freescale Semiconductor, Inc.
> + * Author: German Rivera 
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include "../include/mc-private.h"
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "../include/mc-sys.h"
> +#include "dprc-cmd.h"
> +
> +static struct irq_chip its_msi_irq_chip = {
> + .name = "fsl-mc-bus-msi",
> + .irq_mask = irq_chip_mask_parent,
> + .irq_unmask = irq_chip_unmask_parent,
> + .irq_eoi = irq_chip_eoi_parent,
> + .irq_set_affinity = msi_domain_set_affinity
> +};
> +
> +static int its_fsl_mc_msi_prepare(struct irq_domain *msi_domain,
> +   struct device *dev,
> +   int nvec, msi_alloc_info_t *info)
> +{
> + u32 its_dev_id;
> + struct fsl_mc_device *mc_bus_dev = to_fsl_mc_device(dev);
> + struct msi_domain_info *msi_info;
> +
> + if (WARN_ON(dev->bus != _mc_bus_type))
> + return -EINVAL;

Isn't that a bit late? You've already called to_fsl_mc_device()...

> +
> + if (WARN_ON(!(mc_bus_dev->flags & FSL_MC_IS_DPRC)))
> + return -EINVAL;
> +
> + /*
> +  * Set the device Id to be passed to the GIC-ITS:
> +  *
> +  * NOTE: This device id corresponds to the IOMMU stream ID
> +  * associated with the DPRC object (ICID).
> +  */
> + its_dev_id = mc_bus_dev->icid;
> + info->scratchpad[0].ul = its_dev_id;

You can probably get rid of the its_dev_id variable.

> + msi_info = msi_get_domain_info(msi_domain->parent);
> + return msi_info->ops->msi_prepare(msi_domain->parent, dev, nvec, info);
> +}
> +
> +static struct msi_domain_ops its_fsl_mc_msi_ops = {
> + .msi_prepare = its_fsl_mc_msi_prepare,
> +};
> +
> +static struct msi_domain_info its_fsl_mc_msi_domain_info = {
> + .flags  = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS),
> + .ops= _fsl_mc_msi_ops,
> + .chip   = _msi_irq_chip,
> +};
> +
> +static const struct of_device_id its_device_id[] = {
> + {   .compatible = "arm,gic-v3-its", },
> + {},
> +};
> +
> +int __init its_fsl_mc_msi_init(void)
> +{
> + struct device_node *np;
> + struct irq_domain *parent;
> + struct irq_domain *mc_msi_domain;
> +
> + for (np = of_find_matching_node(NULL, its_device_id); np;
> +  np = of_find_matching_node(np, its_device_id)) {
> + if (!of_property_read_bool(np, "msi-controller"))
> + continue;
> +
> + parent = irq_find_matching_host(np, DOMAIN_BUS_NEXUS);
> + if (!parent || !msi_get_domain_info(parent)) {
> + pr_err("%s: unable to locate ITS domain\n",
> +np->full_name);
> + continue;
> + }
> +
> + mc_msi_domain =
> + fsl_mc_msi_create_irq_domain(of_node_to_fwnode(np),
> +  _fsl_mc_msi_domain_info,
> +  parent);

Please keep both sides of the assignment on the same line.

> + if (!mc_msi_domain) {
> + pr_err("%s: unable to create fsl-mc domain\n",
> +np->full_name);
> + continue;
> + }
> +
> + 

Re: [PATCH RESEND v3 01/11] irqdomain: Added domain bus token DOMAIN_BUS_FSL_MC_MSI

[Marc Zyngier]

On 24/11/15 22:31, J. German Rivera wrote:
> Since an FSL-MC bus is a new bus type that is neither PCI nor
> PLATFORM, we need a new domain bus token to disambiguate the
> IRQ domain for FSL-MC MSIs.
> 
> Signed-off-by: J. German Rivera <german.riv...@freescale.com>

Acked-by: Marc Zyngier <marc.zyng...@arm.com>

M.
-- 
Jazz is not dead. It just smells funny...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RESEND v3 03/11] staging: fsl-mc: Added generic MSI support for FSL-MC devices

[Marc Zyngier]

On 24/11/15 22:31, J. German Rivera wrote:
> Created an MSI domain for the fsl-mc bus-- including functions
> to create a domain, find a domain, alloc/free domain irqs, and
> bus specific overrides for domain and irq_chip ops.
> 
> Signed-off-by: J. German Rivera <german.riv...@freescale.com>
> ---
> CHANGE HISTORY
> 
> Changes in v3:
> - Addressed comments from Marc Zyngier:
>   * Added WARN_ON in fsl_mc_msi_set_desc to check that caller does
> not set set_desc
>   * Changed type of paddr in irq_cfg to be phys_addr_t
>   * Added WARN_ON in fsl_mc_msi_update_chip_op() to check that caller
> does not set irq_write_msi_msg
> 
> Changes in v2: none
> 
>  drivers/staging/fsl-mc/bus/Kconfig  |   1 +
>  drivers/staging/fsl-mc/bus/Makefile |   1 +
>  drivers/staging/fsl-mc/bus/mc-msi.c | 285 
> 
>  drivers/staging/fsl-mc/include/dprc.h   |   2 +-
>  drivers/staging/fsl-mc/include/mc-private.h |  17 ++
>  drivers/staging/fsl-mc/include/mc.h |  17 ++
>  6 files changed, 322 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/staging/fsl-mc/bus/mc-msi.c
> 

[...]

> diff --git a/drivers/staging/fsl-mc/bus/mc-msi.c 
> b/drivers/staging/fsl-mc/bus/mc-msi.c
> new file mode 100644
> index 000..d6ac465
> --- /dev/null
> +++ b/drivers/staging/fsl-mc/bus/mc-msi.c

[...]

> +int fsl_mc_find_msi_domain(struct device_node *mc_of_node,
> +struct irq_domain **mc_msi_domain)
> +{
> + struct device_node *msi_parent_node;
> + struct irq_domain *msi_domain;
> +
> + msi_parent_node = of_parse_phandle(mc_of_node, "msi-parent", 0);
> + if (!msi_parent_node) {
> + pr_err("msi-parent phandle missing for %s\n",
> +mc_of_node->full_name);
> + return -ENOENT;
> + }
> +
> + /*
> +  * Look up the fsl-mc MSI domain:
> +  */
> + msi_domain = irq_find_matching_host(msi_parent_node,
> + DOMAIN_BUS_FSL_MC_MSI);
> + if (!msi_domain) {
> + pr_err("Unable to find fsl-mc MSI domain for %s\n",
> +mc_of_node->full_name);
> +
> + return -ENOENT;
> + }
> +
> + *mc_msi_domain = msi_domain;
> + return 0;
> +}

We now have of_msi_get_domain() which does deal with this (and more). It
is probably worth investigating switching to it.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7 4/7] PCI: Add fwnode_handle to pci_sysdata

[Marc Zyngier]

On 05/12/15 00:36, ja...@microsoft.com wrote:
> From: Jake Oshins 
> 
> This patch adds an fwnode_handle to struct pci_sysdata, which is
> used by the next patch in the series when trying to locate an
> IRQ domain associated with a root PCI bus.
> 
> Signed-off-by: Jake Oshins 
> ---
>  arch/x86/include/asm/pci.h | 15 +++
>  include/asm-generic/pci.h  |  4 
>  2 files changed, 19 insertions(+)
> 

[...]

> diff --git a/include/asm-generic/pci.h b/include/asm-generic/pci.h
> index f24bc51..4092886 100644
> --- a/include/asm-generic/pci.h
> +++ b/include/asm-generic/pci.h
> @@ -21,4 +21,8 @@ static inline int pci_get_legacy_ide_irq(struct pci_dev 
> *dev, int channel)
>  #define PCI_DMA_BUS_IS_PHYS  (1)
>  #endif
>  
> +#ifndef pci_root_bus_fwnode
> +#define pci_root_bus_fwnode(bus) ((void)(bus), NULL)
> +#endif
> +
>  #endif /* _ASM_GENERIC_PCI_H */
> 

This breaks at least arm64 (as you can see from the reply to patch #5, 
because it does have its own asm/pci.h. Instead, how about moving this 
to linux/pci.h, just after the include of asm/pci.h? I just gave it a 
go, and it seems to work nicely (the first hunk fixes the rest of the
arm64 compile issue):

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 8f3d056..c6369dd 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -15,6 +15,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include "pci.h"
 
diff --git a/include/asm-generic/pci.h b/include/asm-generic/pci.h
index 4092886..f24bc51 100644
--- a/include/asm-generic/pci.h
+++ b/include/asm-generic/pci.h
@@ -21,8 +21,4 @@ static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, 
int channel)
 #define PCI_DMA_BUS_IS_PHYS(1)
 #endif
 
-#ifndef pci_root_bus_fwnode
-#define pci_root_bus_fwnode(bus)   ((void)(bus), NULL)
-#endif
-
 #endif /* _ASM_GENERIC_PCI_H */
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 6ae25aa..b4144228 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1517,6 +1517,10 @@ static inline int pci_get_new_domain_nr(void) { return 
-ENOSYS; }
 
 #include 
 
+#ifndef pci_root_bus_fwnode
+#define pci_root_bus_fwnode(bus)   ((void)(bus), NULL)
+#endif
+
 /* these helpers provide future and backwards compatibility
  * for accessing popular PCI BAR info */
 #define pci_resource_start(dev, bar)   ((dev)->resource[(bar)].start)

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7 4/7] PCI: Add fwnode_handle to pci_sysdata

[Marc Zyngier]

On 09/12/15 16:54, Jake Oshins wrote:
>> -Original Message-
>> From: Marc Zyngier [mailto:marc.zyng...@arm.com]
>> Sent: Wednesday, December 9, 2015 8:51 AM
>> To: Jake Oshins <ja...@microsoft.com>; gre...@linuxfoundation.org; KY
>> Srinivasan <k...@microsoft.com>; linux-ker...@vger.kernel.org;
>> de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com;
>> vkuzn...@redhat.com; t...@linutronix.de; Haiyang Zhang
>> <haiya...@microsoft.com>; bhelg...@google.com; linux-
>> p...@vger.kernel.org
>> Subject: Re: [PATCH v7 4/7] PCI: Add fwnode_handle to pci_sysdata
>>
>> On 05/12/15 00:36, ja...@microsoft.com wrote:
>>> From: Jake Oshins <ja...@microsoft.com>
>>>
>>> This patch adds an fwnode_handle to struct pci_sysdata, which is
>>> used by the next patch in the series when trying to locate an
>>> IRQ domain associated with a root PCI bus.
>>>
>>> Signed-off-by: Jake Oshins <ja...@microsoft.com>
>>> ---
>>>  arch/x86/include/asm/pci.h | 15 +++
>>>  include/asm-generic/pci.h  |  4 
>>>  2 files changed, 19 insertions(+)
>>>
>>
>> [...]
>>
>>> diff --git a/include/asm-generic/pci.h b/include/asm-generic/pci.h
>>> index f24bc51..4092886 100644
>>> --- a/include/asm-generic/pci.h
>>> +++ b/include/asm-generic/pci.h
>>> @@ -21,4 +21,8 @@ static inline int pci_get_legacy_ide_irq(struct pci_dev
>> *dev, int channel)
>>>  #define PCI_DMA_BUS_IS_PHYS(1)
>>>  #endif
>>>
>>> +#ifndef pci_root_bus_fwnode
>>> +#define pci_root_bus_fwnode(bus)   ((void)(bus), NULL)
>>> +#endif
>>> +
>>>  #endif /* _ASM_GENERIC_PCI_H */
>>>
>>
>> This breaks at least arm64 (as you can see from the reply to patch #5,
>> because it does have its own asm/pci.h. Instead, how about moving this
>> to linux/pci.h, just after the include of asm/pci.h? I just gave it a
>> go, and it seems to work nicely (the first hunk fixes the rest of the
>> arm64 compile issue):
> 
> Thank you.  I was just working through how to do that.  I lost a
> couple of days trying to figure out how to cross-compile for arm64 to
> check that any fix that I made actually worked.  (In the process,
> I've completely messed up my development machine.  Any pointers to
> the right strategy would be appreciated.)

My own strategy is pretty simple. It involves:
- downloading a binary toolchain ([1] for example)
- untar this somewhere, putting the
gcc-linaro-5.2-2015.11-x86_64_aarch64-linux-gnu/bin directory in your PATH
- and then the usual "make ARCH=arm64 defconfig && make ARCH=arm64
CROSS_COMPILE=aarch64-linux-gnu- all"

That's of course assuming you're using a Linux x86-64 box for
cross-compiling. I also cross-compile from 32bit ARM, but I doubt you'd
be interested... ;-)

Cheers,

M.

[1]
https://releases.linaro.org/components/toolchain/binaries/5.2-2015.11/aarch64-linux-gnu/gcc-linaro-5.2-2015.11-x86_64_aarch64-linux-gnu.tar.xz
-- 
Jazz is not dead. It just smells funny...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 03/11] staging: fsl-mc: Added generic MSI support for FSL-MC devices

[Marc Zyngier]

On 06/11/15 23:20, Jose Rivera wrote:

[...]

>>> +static void __fsl_mc_msi_write_msg(struct fsl_mc_device *mc_bus_dev,
>>> +  struct fsl_mc_device_irq *mc_dev_irq) {
>>> +   int error;
>>> +   struct fsl_mc_device *owner_mc_dev = mc_dev_irq->mc_dev;
>>> +   struct msi_desc *msi_desc = mc_dev_irq->msi_desc;
>>> +   struct dprc_irq_cfg irq_cfg;

[...]

>>> +   /*
>>> +* DPRCs and other objects use different commands to set up IRQs,
>>> +* so we have to differentiate them here.
>>> +*/
>>> +   if (owner_mc_dev == mc_bus_dev) {
>>> +   /*
>>> +* IRQ is for the mc_bus_dev's DPRC itself
>>> +*/
>>> +   error = dprc_set_irq(mc_bus_dev->mc_io,
>>> +MC_CMD_FLAG_INTR_DIS | MC_CMD_FLAG_PRI,
>>> +mc_bus_dev->mc_handle,
>>> +mc_dev_irq->dev_irq_index,
>>> +_cfg);
>>> +   if (error < 0) {
>>> +   dev_err(_mc_dev->dev,
>>> +   "dprc_set_irq() failed: %d\n", error);
>>> +   }
>>> +   } else {
>>> +   error = dprc_set_obj_irq(mc_bus_dev->mc_io,
>>> +MC_CMD_FLAG_INTR_DIS | MC_CMD_FLAG_PRI,
>>> +mc_bus_dev->mc_handle,
>>> +owner_mc_dev->obj_desc.type,
>>> +owner_mc_dev->obj_desc.id,
>>> +mc_dev_irq->dev_irq_index,
>>> +_cfg);
>>> +   if (error < 0) {
>>> +   dev_err(_mc_dev->dev,
>>> +   "dprc_obj_set_irq() failed: %d\n", error);
>>> +   }
>>> +   }
>>
>> It feels a bit odd that you have all of this under a single MSI umbrella,
>> and yet have to differentiate between them. Do they have a different
>> programming interface? Or is that because these dprc_set_*_irq functions
>> do some other stuff behind the scene (I'm too lazy to check...)?
>>
> Due to MC firmware API limitations, dprc_set_obj_irq can only be used
> to set IRQs for the DPRC's children not for the DPRC itself.

Right. So this makes me wonder whether or not you have the right
approach here. The logic behind the bus type was that devices with a
common programming interface would share a bus type (the odd duck being
platform which is used to implement anything else).

Your description seems to suggest that only devices behind the DPRC
share that programming interface, and that the DPRC itself is the local
weirdo.  Should it be using the platform-msi subsystem instead? Or is it
just a matter of firmware oddity?

This is probably not a big deal, but it is worth keeping it in mind,
specially if that has visible consequences (in your device tree, for
example).

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 03/11] staging: fsl-mc: Added generic MSI support for FSL-MC devices

[Marc Zyngier]

On 30/10/15 19:43, J. German Rivera wrote:
> Created an MSI domain for the fsl-mc bus-- including functions
> to create a domain, find a domain, alloc/free domain irqs, and
> bus specific overrides for domain and irq_chip ops.
> 
> Signed-off-by: J. German Rivera 
> ---
> Changes in v2: none
> 
>  drivers/staging/fsl-mc/bus/Kconfig  |   1 +
>  drivers/staging/fsl-mc/bus/Makefile |   1 +
>  drivers/staging/fsl-mc/bus/mc-msi.c | 276 
> 
>  drivers/staging/fsl-mc/include/mc-private.h |  17 ++
>  drivers/staging/fsl-mc/include/mc.h |  17 ++
>  5 files changed, 312 insertions(+)
>  create mode 100644 drivers/staging/fsl-mc/bus/mc-msi.c
> 
> diff --git a/drivers/staging/fsl-mc/bus/Kconfig 
> b/drivers/staging/fsl-mc/bus/Kconfig
> index 0d779d9..c498ac6 100644
> --- a/drivers/staging/fsl-mc/bus/Kconfig
> +++ b/drivers/staging/fsl-mc/bus/Kconfig
> @@ -9,6 +9,7 @@
>  config FSL_MC_BUS
>   tristate "Freescale Management Complex (MC) bus driver"
>   depends on OF && ARM64
> + select GENERIC_MSI_IRQ_DOMAIN
>   help
> Driver to enable the bus infrastructure for the Freescale
>QorIQ Management Complex (fsl-mc). The fsl-mc is a hardware
> diff --git a/drivers/staging/fsl-mc/bus/Makefile 
> b/drivers/staging/fsl-mc/bus/Makefile
> index 25433a9..a5f2ba4 100644
> --- a/drivers/staging/fsl-mc/bus/Makefile
> +++ b/drivers/staging/fsl-mc/bus/Makefile
> @@ -13,5 +13,6 @@ mc-bus-driver-objs := mc-bus.o \
> dpmng.o \
> dprc-driver.o \
> mc-allocator.o \
> +   mc-msi.o \
> dpmcp.o \
> dpbp.o
> diff --git a/drivers/staging/fsl-mc/bus/mc-msi.c 
> b/drivers/staging/fsl-mc/bus/mc-msi.c
> new file mode 100644
> index 000..81b53e3
> --- /dev/null
> +++ b/drivers/staging/fsl-mc/bus/mc-msi.c
> @@ -0,0 +1,276 @@
> +/*
> + * Freescale Management Complex (MC) bus driver MSI support
> + *
> + * Copyright (C) 2015 Freescale Semiconductor, Inc.
> + * Author: German Rivera 
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include "../include/mc-private.h"
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "../include/mc-sys.h"
> +#include "dprc-cmd.h"
> +
> +static void fsl_mc_msi_set_desc(msi_alloc_info_t *arg,
> + struct msi_desc *desc)
> +{
> + arg->desc = desc;
> + arg->hwirq = (irq_hw_number_t)desc->fsl_mc.msi_index;
> +}
> +
> +static void fsl_mc_msi_update_dom_ops(struct msi_domain_info *info)
> +{
> + struct msi_domain_ops *ops = info->ops;
> +
> + if (WARN_ON(!ops))
> + return;
> +
> + if (!ops->set_desc)
> + ops->set_desc = fsl_mc_msi_set_desc;

When would that be overridden by the MSI controller?

> +}
> +
> +static void __fsl_mc_msi_write_msg(struct fsl_mc_device *mc_bus_dev,
> +struct fsl_mc_device_irq *mc_dev_irq)
> +{
> + int error;
> + struct fsl_mc_device *owner_mc_dev = mc_dev_irq->mc_dev;
> + struct msi_desc *msi_desc = mc_dev_irq->msi_desc;
> + struct dprc_irq_cfg irq_cfg;
> +
> + /*
> +  * msi_desc->msg.address is 0x0 when this function is invoked in
> +  * the free_irq() code path. In this case, for the MC, we don't
> +  * really need to "unprogram" the MSI, so we just return.
> +  */
> + if (msi_desc->msg.address_lo == 0x0 && msi_desc->msg.address_hi == 0x0)
> + return;
> +
> + if (WARN_ON(!owner_mc_dev))
> + return;
> +
> + irq_cfg.paddr = ((u64)msi_desc->msg.address_hi << 32) |
> + msi_desc->msg.address_lo;

This should really be a phys_addr_t.

> + irq_cfg.val = msi_desc->msg.data;
> + irq_cfg.user_irq_id = msi_desc->irq;
> +
> + /*
> +  * DPRCs and other objects use different commands to set up IRQs,
> +  * so we have to differentiate them here.
> +  */
> + if (owner_mc_dev == mc_bus_dev) {
> + /*
> +  * IRQ is for the mc_bus_dev's DPRC itself
> +  */
> + error = dprc_set_irq(mc_bus_dev->mc_io,
> +  MC_CMD_FLAG_INTR_DIS | MC_CMD_FLAG_PRI,
> +  mc_bus_dev->mc_handle,
> +  mc_dev_irq->dev_irq_index,
> +  _cfg);
> + if (error < 0) {
> + dev_err(_mc_dev->dev,
> + "dprc_set_irq() failed: %d\n", error);
> + }
> + } else {
> + error = dprc_set_obj_irq(mc_bus_dev->mc_io,
> +  MC_CMD_FLAG_INTR_DIS | MC_CMD_FLAG_PRI,
> +   

Re: [PATCH v2 00/12] New paravirtual PCI front-end for Hyper-V VMs

[Marc Zyngier]

On 14/09/15 18:59, Jake Oshins wrote:
>> -Original Message-
>> From: Marc Zyngier [mailto:marc.zyng...@arm.com]
>> Sent: Monday, September 14, 2015 8:01 AM
>> To: Jake Oshins <ja...@microsoft.com>; gre...@linuxfoundation.org; KY
>> Srinivasan <k...@microsoft.com>; linux-ker...@vger.kernel.org;
>> de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com;
>> vkuzn...@redhat.com; linux-...@vger.kernel.org; bhelg...@google.com;
>> t...@linutronix.de; Jiang Liu <jiang@linux.intel.com>
>> Subject: Re: [PATCH v2 00/12] New paravirtual PCI front-end for Hyper-V
>> VMs
>>
>> Hi Jake,
>>
>> In the future, please CC me on anything that touches irqdomains, along
>> with Jiang Liu as we both co-maintain this piece of code.
>>
> 
> Absolutely.  Sorry for that omission.
> 
>> On 11/09/15 01:00, ja...@microsoft.com wrote:
>>> From: Jake Oshins <ja...@microsoft.com>
>>>
>>> The patch series updates the one sent about a month ago in three ways.  It
>>> integrated with other IRQ domain work done in linux-next in that time, it
>>> distributes interrupts to multiple virtual processors in the guest VM, and 
>>> it
>>> incorporates feedback from Thomas Gleixner and others.
>>>
>>> These patches change the IRQ domain code so that an IRQ domain can
>> match on both
>>> bus type and on the PCI domain.  The IRQ domain match code is modified
>> so that
>>> IRQ domains can have a "rank," allowing for a default one which matches
>> every
>>> x86 PC and more specific ones that replace the default.
>>
>> I'm not really fond of this approach. We already have a way to match an
>> IRQ domain, and that's the device node. It looks to me that you're going
>> through a lot of pain inventing a new infrastructure to avoid divorcing
>> the two. If you could lookup your PCI IRQ domain directly based some
>> (non-DT) identifier, and then possibly fallback to the default one,
>> would that help?
>>
>> If so, here's the deal: I have been working on a patch series that
>> addresses the above for unrelated reasons (ACPI support on arm64). It
>> has been posted twice already:
>>
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/358768.html
>>
>> and the latest version is there:
>>
>> https://git.kernel.org/cgit/linux/kernel/git/maz/arm-
>> platforms.git/log/?h=irq/gsi-irq-domain-v3
>>
>> I have the feeling that you could replace a lot of your patches with
>> this infrastructure.
>>
>> Thoughts?
>>
>>  M.
>> --
> 
> First, thank you so much for reviewing this.  I've read the patch
> series above, but I'm sure that I might have misinterpreted it.  It
> seems to merge the DT and ACPI GSI infrastructure, which I think is a
> great idea.  I'm not sure, however, that it would, as it stands,
> provide what I need here.  Please do tell me if I'm wrong.
> 
> The series above allows you to supply different IRQ domains for
> separate parts of the ACPI GSI space, which is fine for IRQs which
> are actually defined by ACPI.  Message-signaled interrupts (MSI),
> however, aren't defined by ACPI.  ACPI only talks about the routing
> of interrupts with pins and traces (or ones which have equivalent
> mechanisms like the INTx# protocol in PCI Express.)
> 
> What the older DT layer code allowed was for the PCI driver to look
> up an IRQ domain by walking up the device tree looking for a node
> that claimed to be an IRQ domain.  The match() function on the IRQ
> domain allowed it to say that it supported interrupts on PCI buses.
> 
> What's not clear to me is how I would create an IRQ domain that
> matches not on ACPI GSI ranges (because ACPI doesn't talk about MSI)
> and not just on generic PCI buses.  I need to be able to ask for an
> IRQ domain "from my parent" which doesn't really exist without the OF
> device tree or "for a specific PCI bus domain."  That second one is
> what I was trying to enable.
> 
> Is there a way to do that with the infrastructure that you're
> introducing?

The ACPI/GSI stuff is a red herring, and is completely unrelated to the
problem you're trying to solve. What I think is of interest to you is
contained in the first three patches.

In your 4th patch, you have the following code:

+   pci_domain = pci_domain_nr(bus);
+   d = irq_find_matching_host(NULL, DOMAIN_BUS_PCI_MSI, _domain);

which really feels like you're trying to create a namespace that is
parallel to the one defined by the device_node parameter. What I'm
trying to do is to be able to replace the device_node by

Re: [PATCH v2 00/12] New paravirtual PCI front-end for Hyper-V VMs

[Marc Zyngier]

Hi Jake,

In the future, please CC me on anything that touches irqdomains, along
with Jiang Liu as we both co-maintain this piece of code.

On 11/09/15 01:00, ja...@microsoft.com wrote:
> From: Jake Oshins 
> 
> The patch series updates the one sent about a month ago in three ways.  It
> integrated with other IRQ domain work done in linux-next in that time, it
> distributes interrupts to multiple virtual processors in the guest VM, and it
> incorporates feedback from Thomas Gleixner and others.
> 
> These patches change the IRQ domain code so that an IRQ domain can match on 
> both
> bus type and on the PCI domain.  The IRQ domain match code is modified so that
> IRQ domains can have a "rank," allowing for a default one which matches every
> x86 PC and more specific ones that replace the default.

I'm not really fond of this approach. We already have a way to match an
IRQ domain, and that's the device node. It looks to me that you're going
through a lot of pain inventing a new infrastructure to avoid divorcing
the two. If you could lookup your PCI IRQ domain directly based some
(non-DT) identifier, and then possibly fallback to the default one,
would that help?

If so, here's the deal: I have been working on a patch series that
addresses the above for unrelated reasons (ACPI support on arm64). It
has been posted twice already:

http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/358768.html

and the latest version is there:

https://git.kernel.org/cgit/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/gsi-irq-domain-v3

I have the feeling that you could replace a lot of your patches with
this infrastructure.

Thoughts?

M.
-- 
Jazz is not dead. It just smells funny...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH/RFC 0/6] staging: board: armadillo800eva: Board staging for sh_mobile_lcdc_fb

[Marc Zyngier]

Hi Geert,

On 2015-04-03 13:41, Geert Uytterhoeven wrote:

Hi all,

This RFC patch series adds board staging support for 
r8a7740/armadillo.
For now this supports only the frame buffer device for the on-board 
LCD.

The goal is to complete the move to ARM multiplatform kernels for all
shmobile platforms, and drop the existing board files
(arch/arm/mach-shmobile/board-*).

The board staging area was introduced last year to allow continuous
upstream in-tree development and integration of platform devices. It
helps developers integrate devices as platform devices for device
drivers that only provide platform device bindings.  This in turn 
allows

for incremental development of both hardware feature support and DT
binding work in parallel.

This series consists of 4 parts:
  - Patch 1 re-enables compilation of the board staging area, which 
was

disabled after a compile breakage, but has been fixed in the mean
time,
  - Path 2 moves initialization of staging board code to an earlier
moment, as currently it happens after unused PM domains are 
powered

down,
  - Patches 3 and 4 (hopefully) fix the existing kzm9d board staging
code, which was presumably broken by commit 9a1091ef0017c40a
(irqchip: gic: Support hierarchy irq domain.),


Please allow me a semantic correction here: this commit never broke 
anything. It merely revealed how platform code (OMAP, iMX6, shmobile) 
abused the irq_domain subsystem, hoping to get away with only a partial 
conversion to DT. The platform code itself was broken from the moment it 
used DT to discover its interrupt controller.


The truth is, it is not possible to sanely convert bits of a platform 
to DT. The changes creep into all the subsystems, and doing it in stages 
requires tons of ugly hacks (and I've written my share of them).


That being said, I'm off reviewing these two matches.

Thanks,

M.
--
Fast, cheap, reliable. Pick two.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH/RFC 3/6] staging: board: Add support for translating hwirq to virq numbers

[Marc Zyngier]

On 2015-04-03 13:42, Geert Uytterhoeven wrote:

As of commit 9a1091ef0017c40a (irqchip: gic: Support hierarchy irq
domain.), GIC IRQ numbers are virtual, breaking hardcoded hardware 
IRQ

numbers in platform device resources.

Add support for translating hardware IRQ numbers to virtual IRQ 
numbers,

and fixing up platform device resources with hardcoded IRQ numbers.

Add a copyright header, including the original author.

Signed-off-by: Geert Uytterhoeven geert+rene...@glider.be
---
 drivers/staging/board/board.c | 66
+++
 drivers/staging/board/board.h |  5 
 2 files changed, 71 insertions(+)

diff --git a/drivers/staging/board/board.c 
b/drivers/staging/board/board.c

index d5a6abc845191c93..b84ac2837a20bf06 100644
--- a/drivers/staging/board/board.c
+++ b/drivers/staging/board/board.c
@@ -1,10 +1,27 @@
+/*
+ * Copyright (C) 2014 Magnus Damm
+ * Copyright (C) 2015 Glider bvba
+ *
+ * This file is subject to the terms and conditions of the GNU
General Public
+ * License.  See the file COPYING in the main directory of this 
archive

+ * for more details.
+ */
+
+#define pr_fmt(fmt)board_staging:   fmt
+
 #include linux/init.h
+#include linux/irq.h
 #include linux/device.h
 #include linux/kernel.h
 #include linux/of.h
 #include linux/of_address.h
+#include linux/of_irq.h
+
 #include board.h

+static struct device_node *irqc_node __initdata;
+static unsigned int irqc_base __initdata;
+
 static bool find_by_address(u64 base_address)
 {
struct device_node *dn = of_find_all_nodes(NULL);
@@ -38,3 +55,52 @@ bool __init board_staging_dt_node_available(const
struct resource *resource,

return false; /* Nothing found */
 }
+
+int __init board_staging_setup_hwirq_xlate(const char *irqc_match,
+  unsigned int base)
+{
+   irqc_node = of_find_compatible_node(NULL, NULL, irqc_match);
+
+   WARN_ON(!irqc_node);
+   if (!irqc_node)
+   return -ENOENT;
+
+   irqc_base = base;
+   return 0;
+}


And what happens when you have multiple (cascaded) interrupt 
controllers, each wanting to register with this translation service? You 
should at least check that nobody has been here before.



+static unsigned int __init xlate_hwirq(unsigned int hwirq)
+{
+   struct of_phandle_args irq_data;
+   unsigned int irq;
+
+   if (!irqc_node)
+   return hwirq;
+
+   irq_data.np = irqc_node;
+   irq_data.args_count = 3;
+   irq_data.args[0] = 0;
+   irq_data.args[1] = hwirq - irqc_base;
+   irq_data.args[2] = IRQ_TYPE_LEVEL_HIGH;


And what happens for edge interrupts? Or LEVEL_LOW? This is very much 
GIC specific (3 args...). How does it work with non-GIC interrupt 
controllers?



+
+   irq = irq_create_of_mapping(irq_data);
+   if (WARN_ON(!irq))
+   irq = hwirq;
+
+   return irq;
+}
+
+void __init board_staging_fixup_irq_resources(struct resource *res,
+ unsigned int nres)
+{
+   unsigned int i;
+
+   for (i = 0; i  nres; i++)
+   if (res[i].flags == IORESOURCE_IRQ) {
+   unsigned int hwirq = res[i].start;
+   unsigned int virq = xlate_hwirq(hwirq);
+
+   pr_debug(hwirq %u - virq %u\n, hwirq, virq);
+   res[i].start = virq;
+   }
+}
diff --git a/drivers/staging/board/board.h 
b/drivers/staging/board/board.h

index e9c914985d4acb36..4cedc3c46e287eb7 100644
--- a/drivers/staging/board/board.h
+++ b/drivers/staging/board/board.h
@@ -1,10 +1,15 @@
 #ifndef __BOARD_H__
 #define __BOARD_H__
+
 #include linux/init.h
 #include linux/of.h

+struct resource;
+
 bool board_staging_dt_node_available(const struct resource 
*resource,

 unsigned int num_resources);
+int board_staging_setup_hwirq_xlate(const char *irqc_match, unsigned
int base);
+void board_staging_fixup_irq_resources(struct resource *res,
unsigned int nres);

 #define board_staging(str, fn) \
 static int __init runtime_board_check(void)\


It won't surprise you that I don't really like this approach. It is 
controller-specific, restrictive, and allows platform code to continue 
doing something that is essentially wrong. Should this code ever move 
out of staging, it should depend on CONFIG_BROKEN, because this is 
essentially what it is - support code for broken systems. I'd also 
welcome moving parts of the OMAP4/5 code to such CONFIG_BROKEN 
dependency.


Thanks,

M.
--
Fast, cheap, reliable. Pick two.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel