* [PATCH v4 1/7] pinctrl: bcm2835: add pull defines to dt bindings
From: Linus Walleij @ 2016-09-23 8:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <87zin49p9m.fsf@eliezer.anholt.net>
On Mon, Sep 19, 2016 at 12:07 PM, Eric Anholt <eric@anholt.net> wrote:
> Gerd Hoffmann <kraxel@redhat.com> writes:
>
>> Also delete (unused) private enum from driver.
>> The pull defines can be used instead if needed.
>>
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>
> Acked-by: Eric Anholt <eric@anholt.net>
>
> gpio maintainers, could I pull this through my dt tree? Or does this
> need to be split in two?
You can take it. The bcm2835 driver is not seeing any conflicting
changes in this merge window.
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH v2] arm: dts: zynq: Add MicroZed board support
From: Michal Simek @ 2016-09-23 8:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAD6G_RRn0pAF=TsgONQ4yah2VhZXQeBQmeBQ5p46u6R5F8pSqw@mail.gmail.com>
On 23.9.2016 10:42, Jagan Teki wrote:
> On Fri, Sep 23, 2016 at 2:07 PM, Michal Simek <michal.simek@xilinx.com> wrote:
>> On 23.9.2016 09:39, Jagan Teki wrote:
>>> On Fri, Sep 23, 2016 at 11:33 AM, Michal Simek <michal.simek@xilinx.com> wrote:
>>>> On 22.9.2016 22:59, S?ren Brinkmann wrote:
>>>>> On Thu, 2016-09-22 at 18:51:29 +0530, Jagan Teki wrote:
>>>>>> From: Jagan Teki <jteki@openedev.com>
>>>>>>
>>>>>> Added basic dts support for MicroZed board.
>>>>>>
>>>>>> - UART
>>>>>> - SDHCI
>>>>>> - Ethernet
>>>>>>
>>>>>> Cc: Soren Brinkmann <soren.brinkmann@xilinx.com>
>>>>>> Cc: Michal Simek <michal.simek@xilinx.com>
>>>>>> Signed-off-by: Jagan Teki <jteki@openedev.com>
>>>>>> ---
>>>>>> Changes for v2:
>>>>>> - Add SDHCI
>>>>>> - Add Ethernet
>>>>>>
>>>>>> arch/arm/boot/dts/Makefile | 1 +
>>>>>> arch/arm/boot/dts/zynq-microzed.dts | 95 +++++++++++++++++++++++++++++++++++++
>>>>>> 2 files changed, 96 insertions(+)
>>>>>> create mode 100644 arch/arm/boot/dts/zynq-microzed.dts
>>>>>>
>>>>>> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
>>>>>> index faacd52..4d7b858 100644
>>>>>> --- a/arch/arm/boot/dts/Makefile
>>>>>> +++ b/arch/arm/boot/dts/Makefile
>>>>>> @@ -862,6 +862,7 @@ dtb-$(CONFIG_ARCH_VT8500) += \
>>>>>> wm8750-apc8750.dtb \
>>>>>> wm8850-w70v2.dtb
>>>>>> dtb-$(CONFIG_ARCH_ZYNQ) += \
>>>>>> + zynq-microzed.dtb \
>>>>>> zynq-parallella.dtb \
>>>>>> zynq-zc702.dtb \
>>>>>> zynq-zc706.dtb \
>>>>>> diff --git a/arch/arm/boot/dts/zynq-microzed.dts b/arch/arm/boot/dts/zynq-microzed.dts
>>>>>> new file mode 100644
>>>>>> index 0000000..9e64496
>>>>>> --- /dev/null
>>>>>> +++ b/arch/arm/boot/dts/zynq-microzed.dts
>>>>>> @@ -0,0 +1,95 @@
>>>>>> +/*
>>>>>> + * Copyright (C) 2015 Jagan Teki <jteki@openedev.com>
>>>>
>>>>
>>>> Did you created only yourself without copying part of this from others?
>>>> There is dts in u-boot which I believe you are aware of.
>>>
>>> I sent the initial one few months back so at that time, I was created
>>> and now I just added USB and Ethernet with the reference of zc702
>>> board.
>>
>> Can you send me a link to that post? I didn't find it.
>
> Here is the link [1]
>
>> Anyway you should keep Xilinx copyright there.
>
> OK then will append Xilinx as well.
>
> [1] http://lkml.iu.edu/hypermail/linux/kernel/1601.1/03236.html
ok. It was lkml and devicetree not arm mailing list that's why I didn't
find it.
Thanks,
Michal
^ permalink raw reply
* [PATCH 1/3] dt-bindings: Add a binding for the RPi firmware GPIO driver.
From: Linus Walleij @ 2016-09-23 8:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160919161314.25858-1-eric@anholt.net>
On Mon, Sep 19, 2016 at 6:13 PM, Eric Anholt <eric@anholt.net> wrote:
> The RPi firmware exposes all of the board's GPIO lines through
> property calls. Linux chooses to control most lines directly through
> the pinctrl driver, but for the FXL6408 GPIO expander on the Pi3, we
> need to access them through the firmware.
>
> Signed-off-by: Eric Anholt <eric@anholt.net>
Aha
> +++ b/Documentation/devicetree/bindings/gpio/gpio-raspberrypi-firmware.txt
> @@ -0,0 +1,22 @@
> +Raspberry Pi power domain driver
Really? :)
> +Required properties:
> +
> +- compatible: Should be "raspberrypi,firmware-gpio"
Usually this is vendor,compat, is the vendors name "raspberrypi"?
> +- gpio-controller: Marks the device node as a gpio controller
> +- #gpio-cells: Should be <2> for GPIO number and flags
> +- ngpios: Number of GPIO lines to control. See gpio.txt
Is this ever anything else than 8? Else omit it and hardcode
8 in the driver instead.
> +- firmware: Reference to the RPi firmware device node
Reference the DT binding for this.
> +- raspberrypi,firmware-gpio-offset:
> + Number the firmware uses for the first GPIO line
> + controlled by this driver
Does this differ between different instances of this hardware or
can it just be open coded in the driver instead?
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH v5] KVM: arm/arm64: Route vtimer events to user space
From: Paolo Bonzini @ 2016-09-23 8:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <d90f04d4-b29f-f08e-df39-ec307af44225@suse.de>
On 23/09/2016 09:14, Alexander Graf wrote:
>>> +/*
>>> + * Synchronize the timer IRQ state with the interrupt controller.
>>> + */
>>> static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level)
>>> {
>>> int ret;
>>> struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
>>>
>>> timer->active_cleared_last = false;
>>> timer->irq.level = new_level;
>>> - trace_kvm_timer_update_irq(vcpu->vcpu_id, timer->irq.irq,
>>> + trace_kvm_timer_update_irq(vcpu->vcpu_id, host_vtimer_irq,
>>> timer->irq.level);
>>> + [...]
>>> + struct kvm_sync_regs *regs = &vcpu->run->s.regs;
>>> +
>>> + /* Populate the timer bitmap for user space */
>>> + regs->kernel_timer_pending &= ~KVM_ARM_TIMER_VTIMER;
>>> + if (new_level)
>>> + regs->kernel_timer_pending |= KVM_ARM_TIMER_VTIMER;
>>
>> I think if you got here, it means you have to exit to userspace to
>> update it of the new state. If you don't want to propagate a return
>
> Yes, but we can't exit straight away with our own exit reason because we
> might be inside an MMIO exit path here which already occupies the
> exit_reason.
So the idea is that whenever you're here you have one of the following
cases:
- are coming from kvm_timer_flush_hwstate, and then you exit immediately
with KVM_EXIT_INTR if needed
- you are coming from the kvm_timer_sync_hwstate just before
handle_exit. Then if there's a vmexit you have already set
regs->kernel_timer_pending, if not you'll do a kvm_timer_flush_hwstate soon.
- you are coming from the kvm_timer_sync_hwstate in the middle of
kvm_arch_vcpu_ioctl_run, and then "continue" will either exit the loop
immediately (if ret <= 0) or go to kvm_timer_flush_hwstate as in the
previous case
Right?
>> Maybe I'm misunderstanding and user_timer_pending is just a cached
>> verison of what you said last, but as I said above, I think you can just
>> compare timer->irq.level with the last value the kvm_run struct, and if
>> something changed, you have to exit.
>
> So how would user space know whether the line went up or down? Or didn't
> change at all (if we coalesce with an MMIO exit)?
It would save the status of the line somewhere in its own variable,
without introducing a relatively delicate API between kernel and user.
I agree that you cannot update kernel_timer_pending only in
flush_hwstate; otherwise you miss on case (2) when there is a vmexit.
That has to stay in kvm_timer_sync_hwstate (or it could become a new
function called at the very end of kvm_arch_vcpu_ioctl_run).
However, I'm not sure why user_timer_pending is necessary. If it is
just the value you assigned to regs->kernel_timer_pending at the time of
the previous vmexit, can kvm_timer_flush_hwstate just do
if (timer->prev_kernel_timer_pending != regs->kernel_timer_pending) {
timer->prev_kernel_timer_pending = regs->kernel_timer_pending;
return 1;
}
? Or even
if (timer->prev_irq_level != timer->irq.level) {
timer->prev_irq_level = regs->irq.level;
return 1;
}
so that regs->kernel_timer_pending remains exclusively
kvm_timer_sync_hwstate's business?
Thanks,
Paolo
>>
>>> +
>>> + /*
>>> + * As long as user space is aware that the timer is pending,
>>> + * we do not need to get new host timer events.
>>> + */
>>
>> yes, correct, but I don't think this concept was clearly reflected in
>> your API text above.
>>
>>> + if (timer->irq.level)
>>> + disable_percpu_irq(host_vtimer_irq);
>>> + else
>>> + enable_percpu_irq(host_vtimer_irq, 0);
>>> + }
>>
>> could we move these two blocks into their own functions instead? That
>> would also give nice names to the huge chunk of complicated
>> functionality, e.g. flush_timer_state_to_user() and
>> flush_timer_state_to_vgic().
>
> That's probably a very useful cleanup, yes :).
>
>
> Alex
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* [PATCH v3 0/4] regulator: axp20x: support AXP803/AXP813 variants
From: Jean-Francois Moine @ 2016-09-23 8:58 UTC (permalink / raw)
To: linux-arm-kernel
This patch series adds support for the X-Powers AXP803 and AXP813 PMICs.
It is based on the previous patch series
regulator: axp20x: Simplify various code
v3:
- put the code of the new devices in new files instead of in the common
axp20x file.
- fix errors about the regulators and interrupts
v2:
- fix lack of support of dcdc frequency
- notice that the AXP803 is also handled
- send the patch to the DT maintainers
Jean-Francois Moine (4):
regulator: axp20x: move device independant parts to new files
regulator: axp20x: duplicate the MFD axp20x-rsb code
regulator: axp20x: add the AXP803
regulator: axp20x: add the AXP813
Documentation/devicetree/bindings/mfd/axp20x.txt | 29 ++-
drivers/mfd/axp20x.c | 15 +
drivers/regulator/Makefile | 3 +-
drivers/regulator/axp-regulator.c | 347 +++++++++++++++++++
drivers/regulator/axp-regulator.h | 133 ++++++++
drivers/regulator/axp20x-regulator.c | 415 ++---------------------
drivers/regulator/axp803.c | 225 ++++++++++++
drivers/regulator/axp813.c | 229 +++++++++++++
include/linux/mfd/axp20x.h | 2 +
9 files changed, 1012 insertions(+), 388 deletions(-)
create mode 100644 drivers/regulator/axp-regulator.c
create mode 100644 drivers/regulator/axp-regulator.h
create mode 100644 drivers/regulator/axp803.c
create mode 100644 drivers/regulator/axp813.c
--
2.10.0
^ permalink raw reply
* [PATCH v6 0/3] arm64 kexec-tools patches
From: Pratyush Anand @ 2016-09-23 8:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <cover.1474481332.git.geoff@infradead.org>
Hi Geoff,
On 21/09/2016:06:14:25 PM, Geoff Levand wrote:
> This series adds the core support for kexec re-boot on ARM64.
I tested this series with mustang and seattle and they work.
So you can use my "Tested-By: Pratyush Anand <panand@redhat.com>".
Just one Nitpick: In patch 2/3 arm64_header_check_msb(),
arm64_header_page_size() and arm64_header_placement() have been defined but they
have not been used anywhere. So, may be they can be removed.
~Pratyush
>
> Linux kernel support for ARM64 kexec reboot has been merged in v4.8-rc1 with the
> expectation that it will be included in the v4.8 stable kernel release.
>
> For ARM64 kdump support see Takahiro's latest kdump patches [1].
>
> [1] http://lists.infradead.org/pipermail/kexec
>
> Changes for v6 (Sep , 2016, 2m):
>
> o Remove the get_memory_ranges_dt() routine.
> o Rename dtb_[12] to dtb_{proc,sys,user}.
>
> Changes for v5 (Sep 1, 2016, 2m):
>
> o Add common routine arm64_locate_kernel_segment().
>
> Changes for v4 (Aug 18, 2016, 1m):
>
> o Fix some error return values and error messages.
> o Add enum arm64_header_page_size.
> o Rename page_offset to vp_offset.
>
> Changes for v3 (Aug 10, 2016, 1m):
>
> o Rebase to 2.0.13.
> o Add support for flag bits 1-3 to arm64 image-header.h.
> o Fix OPT_ARCH_MAX value.
> o Use fdt_pack() in dtb_set_property().
> o Remove patch ("kexec: (bugfix) calc correct end address of memory ranges in device tree").
>
> Changes for v2 (July 26, 2016, 0m):
>
> o Inline some small routines.
> o Reformat some dbgprintf messages.
> o Remove debug_brk from entry.S
> o Change arm64_image_header.flags to uint64_t.
> o Look in iomem then dt for mem info.
> o Remove check_cpu_nodes.
> o Remove purgatory printing.
>
> First submission v1 (July 20, 2016).
>
> -Geoff
>
> The following changes since commit 67488beb0a6ee8ad2c0b05f721a9e00041fab93a:
>
> kexec-tools 2.0.13 (2016-08-08 12:26:44 +0200)
>
> are available in the git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/geoff/kexec-tools.git for-merge-arm64-v6
>
> for you to fetch changes up to a9f1fafd7c248d33768177438742b90bc4338202:
>
> arm64: Add support for binary image files (2016-09-21 11:04:03 -0700)
>
> ----------------------------------------------------------------
> Geoff Levand (2):
> kexec: Add common device tree routines
> arm64: Add arm64 kexec support
>
> Pratyush Anand (1):
> arm64: Add support for binary image files
>
> configure.ac | 3 +
> kexec/Makefile | 5 +
> kexec/arch/arm64/Makefile | 40 +++
> kexec/arch/arm64/crashdump-arm64.c | 21 ++
> kexec/arch/arm64/crashdump-arm64.h | 12 +
> kexec/arch/arm64/image-header.h | 146 ++++++++
> kexec/arch/arm64/include/arch/options.h | 39 ++
> kexec/arch/arm64/kexec-arm64.c | 615 ++++++++++++++++++++++++++++++++
> kexec/arch/arm64/kexec-arm64.h | 71 ++++
> kexec/arch/arm64/kexec-elf-arm64.c | 146 ++++++++
> kexec/arch/arm64/kexec-image-arm64.c | 80 +++++
> kexec/dt-ops.c | 145 ++++++++
> kexec/dt-ops.h | 13 +
> kexec/kexec-syscall.h | 8 +-
> purgatory/Makefile | 1 +
> purgatory/arch/arm64/Makefile | 18 +
> purgatory/arch/arm64/entry.S | 51 +++
> purgatory/arch/arm64/purgatory-arm64.c | 19 +
> 18 files changed, 1431 insertions(+), 2 deletions(-)
> create mode 100644 kexec/arch/arm64/Makefile
> create mode 100644 kexec/arch/arm64/crashdump-arm64.c
> create mode 100644 kexec/arch/arm64/crashdump-arm64.h
> create mode 100644 kexec/arch/arm64/image-header.h
> create mode 100644 kexec/arch/arm64/include/arch/options.h
> create mode 100644 kexec/arch/arm64/kexec-arm64.c
> create mode 100644 kexec/arch/arm64/kexec-arm64.h
> create mode 100644 kexec/arch/arm64/kexec-elf-arm64.c
> create mode 100644 kexec/arch/arm64/kexec-image-arm64.c
> create mode 100644 kexec/dt-ops.c
> create mode 100644 kexec/dt-ops.h
> create mode 100644 purgatory/arch/arm64/Makefile
> create mode 100644 purgatory/arch/arm64/entry.S
> create mode 100644 purgatory/arch/arm64/purgatory-arm64.c
>
> --
> 2.7.4
^ permalink raw reply
* [PATCH v5] KVM: arm/arm64: Route vtimer events to user space
From: Christoffer Dall @ 2016-09-23 9:08 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <d90f04d4-b29f-f08e-df39-ec307af44225@suse.de>
On Fri, Sep 23, 2016 at 09:14:13AM +0200, Alexander Graf wrote:
>
>
> On 22.09.16 23:28, Christoffer Dall wrote:
> > On Thu, Sep 22, 2016 at 02:52:49PM +0200, Alexander Graf wrote:
> >> We have 2 modes for dealing with interrupts in the ARM world. We can either
> >> handle them all using hardware acceleration through the vgic or we can emulate
> >> a gic in user space and only drive CPU IRQ pins from there.
> >>
> >> Unfortunately, when driving IRQs from user space, we never tell user space
> >> about timer events that may result in interrupt line state changes, so we
> >> lose out on timer events if we run with user space gic emulation.
> >>
> >> This patch fixes that by syncing user space's view of the vtimer irq line
> >> with the kvm view of that same line.
> >>
> >> With this patch I can successfully run edk2 and Linux with user space gic
> >> emulation.
> >>
> >> Signed-off-by: Alexander Graf <agraf@suse.de>
> >>
> >> ---
> >>
> >> v1 -> v2:
> >>
> >> - Add back curly brace that got lost
> >>
> >> v2 -> v3:
> >>
> >> - Split into patch set
> >>
> >> v3 -> v4:
> >>
> >> - Improve documentation
> >>
> >> v4 -> v5:
> >>
> >> - Rewrite to use pending state sync in sregs (marc)
> >> - Remove redundant checks of vgic_initialized()
> >> - qemu tree to try this out: https://github.com/agraf/u-boot.git no-kvm-irqchip-for-v5
> >
> > huh, qemu=u-boot?
>
> Bleks, qemu.git of course.
>
> >
> >> ---
> >> Documentation/virtual/kvm/api.txt | 26 ++++++++
> >> arch/arm/include/uapi/asm/kvm.h | 3 +
> >> arch/arm/kvm/arm.c | 14 ++---
> >> arch/arm64/include/uapi/asm/kvm.h | 3 +
> >> include/kvm/arm_arch_timer.h | 2 +-
> >> include/uapi/linux/kvm.h | 6 ++
> >> virt/kvm/arm/arch_timer.c | 129 ++++++++++++++++++++++++++------------
> >> 7 files changed, 134 insertions(+), 49 deletions(-)
> >>
> >> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> >> index 739db9a..8049327 100644
> >> --- a/Documentation/virtual/kvm/api.txt
> >> +++ b/Documentation/virtual/kvm/api.txt
> >> @@ -3928,3 +3928,29 @@ In order to use SynIC, it has to be activated by setting this
> >> capability via KVM_ENABLE_CAP ioctl on the vcpu fd. Note that this
> >> will disable the use of APIC hardware virtualization even if supported
> >> by the CPU, as it's incompatible with SynIC auto-EOI behavior.
> >> +
> >> +8.3 KVM_CAP_ARM_TIMER
> >> +
> >> +Architectures: arm, arm64
> >> +This capability, if KVM_CHECK_EXTENSION indicates that it is available and no
> >> +in-kernel interrupt controller is in use, means that that the kernel populates
> >> +the vcpu's run->s.regs.kernel_timer_pending field with timers that are currently
> >> +considered pending by kvm.
> >
> > Be careful with the word 'pending' here. I think this could be
> > misleading, because pending is a state in the GIC, but not really
> > something I can find specific to the timer. It would be more
> > descriptive to say that the kernel maintained generic timer's output
> > signal is asserted.
>
> Sure, asserted works for me. Or maybe istatus?
>
I think the concept that most closely describes the generic timer
architecture talks about 'asserting the timer output signal', so I'd
rather we stick to something as close to that as possible.
> >
> >> +
> >> +If active, it also allows user space to propagate its own pending state of timer
> >> +interrupt lines using run->s.regs.user_timer_pending. If those two fields
> >> +mismatch during CPU execution, kvm will exit to user space to give it a chance
> >
> > I don't quite understand the semantics here. The only entity that knows
> > what the level state of the output of the timer is, is the kernel, which
> > emulates the timer. Userspace knows interrupt controller state, but if
> > it has a different view of the timer state than the kernel, it's because
> > the kernel failed to notify userspace of a change or userspace failed to
> > listen?
>
> Right, and the reason we have 2 fields is to get us exit-less (and
> easier) updates whenever we can. In most cases, taking the assertion
> down will coincide with an MMIO exit to user space for an EOI for example.
The assertion of the timer output signal is a completely separate
concept from an EOI. Most often, the signal will be deasserted *before*
the EOI, but only noticed when you trap for the EOI. Just to make that
distinction clear.
>
> Somewhere in between in the development I had a version that explicitly
> triggered a KVM_EXIT for every state change of the timer. But that gets
> messy very quickly. You need to update the state change before an MMIO
> for example, otherwise a sequence like
>
> * set cval to future (which in turns sets istatus=0)
> * read gic pending state
>
> will give you bogus results, as the mmio read to user space did not yet
> have the timer status updated. And I really didn't want to wrap my head
> around restarting MMIO exits ;).
>
> So having this side channel where user space potentially expects a timer
> status change on every exit is much cleaner.
>
Eh, I'm confused. I'm not arguing against the side channel, I
completely agree that this is a reasonable approach: Always just tell
userspace what the timer output level is, no matter why you exit.
What I don't understand is why userspace have to tell the kernel
anything back. The kernel already knows what the state is, and if
userspace forgot what it was told, it did something wrong.
> >
> >> +to update its own interrupt pending status. This usually involves triggering
> >> +an interrupt line on a user space emulated interrupt controller.
> >
> > To me it feels like the semantics should be that userspace can always
> > derive the status of the timer and the level of the output signal from
> > the timer by simply looking at kvm_run structure.
>
> Yes, it can, and it does. That's what the "kernel_timer_pending" field is.
>
> The kernel however also needs to know whether user space's view is in
> sync, because we don't know whether there was an exit between our
> internal state change and the guest entry.
Yes we do, whenever we notice that the computed state is changing (this
is exactly when we call kvm_timer_update_irq), then we will update
timer->irq.level and we will notice that there's now a deviation between
what we last told userspace (kernel_timer_pending) and the current
state, so we update kernel_timer_pending and force an exit to userspace.
Why should we care if userspace has derived something in the mean time
(which I don't even see how it can)?
> That's what the
> "user_timer_pending" field is for.
>
I don't understand the semantics of this field.
Perhaps you can explain me which hardware state (line etc.) this field
represents?
> >
> > The remaining two problems are:
> >
> > (1) when should the kernel trigger exits to userspace? Presumably on
> > any change in the timer's output level, because this change has to be
> > propagated to the userspace interrupt controller.
>
> Yes, but if we can we want to piggy-back on an existing exit.
>
never argued against that.
> > (2) the kernel needs to somehow mask the underlying hardware timer
> > interrupt signal when it's active, because otherwise the guest won't
> > proceed. If we simply mask the hardware signal after telling userspace
> > the output signal is asserted and until the output signal ever becomes
> > deasserted, why do we need to listen to anything userspace has to say?
>
> Because we need to make sure that the cpu IRQ line is updated before we
> enter the guest. Otherwise we might get spurious interrupts.
>
This doesn't mean we need to listen to what userspace says, we just have
to exit to userspace with the updated value before re-entering the
guest.
You shoudl have the exact same semantics between the arch timer and the
gic and between the arch timer and the userspace irqchip: Before
entering the guest, the kernel arch timer code needs to notify the
irqchip (whereever that lives) of a state change.
We've had all sorts of other schemes in the past and they just kept on
breaking for weirdo corner cases. The only thing that works here is to
maintain the semantics of the architecture.
> >> +
> >> +The fields run->s.regs.kernel_timer_pending and run->s.regs.user_timer_pending
> >> +are available independent of run->kvm_valid_regs or run->kvm_dirty_regs bits.
> >> +If no in-kernel interrupt controller is used and the capability exists, they
> >> +will always be available and used.
> >> +
> >> +Currently the following bits are defined for both bitmaps:
> >> +
> >> + KVM_ARM_TIMER_VTIMER - virtual timer
> >> +
> >> +Future versions of kvm may implement additional timer events. These will get
> >> +indicated by additional KVM_CAP extensions.
> >> diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
> >> index a2b3eb3..caad81d 100644
> >> --- a/arch/arm/include/uapi/asm/kvm.h
> >> +++ b/arch/arm/include/uapi/asm/kvm.h
> >> @@ -105,6 +105,9 @@ struct kvm_debug_exit_arch {
> >> };
> >>
> >> struct kvm_sync_regs {
> >> + /* Used with KVM_CAP_ARM_TIMER */
> >> + u8 kernel_timer_pending;
> >> + u8 user_timer_pending;
> >> };
> >>
> >> struct kvm_arch_memory_slot {
> >> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> >> index 75f130e..dc19221 100644
> >> --- a/arch/arm/kvm/arm.c
> >> +++ b/arch/arm/kvm/arm.c
> >> @@ -187,6 +187,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> >> case KVM_CAP_ARM_PSCI_0_2:
> >> case KVM_CAP_READONLY_MEM:
> >> case KVM_CAP_MP_STATE:
> >> + case KVM_CAP_ARM_TIMER:
> >> r = 1;
> >> break;
> >> case KVM_CAP_COALESCED_MMIO:
> >> @@ -474,13 +475,7 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
> >> return ret;
> >> }
> >>
> >> - /*
> >> - * Enable the arch timers only if we have an in-kernel VGIC
> >> - * and it has been properly initialized, since we cannot handle
> >> - * interrupts from the virtual timer with a userspace gic.
> >> - */
> >> - if (irqchip_in_kernel(kvm) && vgic_initialized(kvm))
> >> - ret = kvm_timer_enable(vcpu);
> >> + ret = kvm_timer_enable(vcpu);
> >>
> >> return ret;
> >> }
> >> @@ -588,7 +583,10 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
> >> */
> >> preempt_disable();
> >> kvm_pmu_flush_hwstate(vcpu);
> >> - kvm_timer_flush_hwstate(vcpu);
> >> + if (kvm_timer_flush_hwstate(vcpu)) {
> >> + ret = -EINTR;
> >> + run->exit_reason = KVM_EXIT_INTR;
> >> + }
> >> kvm_vgic_flush_hwstate(vcpu);
> >>
> >> local_irq_disable();
> >> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> >> index 3051f86..9aac860 100644
> >> --- a/arch/arm64/include/uapi/asm/kvm.h
> >> +++ b/arch/arm64/include/uapi/asm/kvm.h
> >> @@ -143,6 +143,9 @@ struct kvm_debug_exit_arch {
> >> #define KVM_GUESTDBG_USE_HW (1 << 17)
> >>
> >> struct kvm_sync_regs {
> >> + /* Used with KVM_CAP_ARM_TIMER */
> >> + u8 kernel_timer_pending;
> >> + u8 user_timer_pending;
> >> };
> >>
> >> struct kvm_arch_memory_slot {
> >> diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> >> index dda39d8..8cd7240 100644
> >> --- a/include/kvm/arm_arch_timer.h
> >> +++ b/include/kvm/arm_arch_timer.h
> >> @@ -63,7 +63,7 @@ void kvm_timer_init(struct kvm *kvm);
> >> int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
> >> const struct kvm_irq_level *irq);
> >> void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu);
> >> -void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu);
> >> +int kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu);
> >> void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu);
> >> void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu);
> >>
> >> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> >> index 300ef25..1fc02d7 100644
> >> --- a/include/uapi/linux/kvm.h
> >> +++ b/include/uapi/linux/kvm.h
> >> @@ -870,6 +870,7 @@ struct kvm_ppc_smmu_info {
> >> #define KVM_CAP_S390_USER_INSTR0 130
> >> #define KVM_CAP_MSI_DEVID 131
> >> #define KVM_CAP_PPC_HTM 132
> >> +#define KVM_CAP_ARM_TIMER 133
> >>
> >> #ifdef KVM_CAP_IRQ_ROUTING
> >>
> >> @@ -1327,4 +1328,9 @@ struct kvm_assigned_msix_entry {
> >> #define KVM_X2APIC_API_USE_32BIT_IDS (1ULL << 0)
> >> #define KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK (1ULL << 1)
> >>
> >> +/* Available with KVM_CAP_ARM_TIMER */
> >> +
> >> +/* Bits for run->s.regs.{user,kernel}_timer_pending */
> >> +#define KVM_ARM_TIMER_VTIMER (1 << 0)
> >> +
> >> #endif /* __LINUX_KVM_H */
> >> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> >> index 4309b60..0c6fc38 100644
> >> --- a/virt/kvm/arm/arch_timer.c
> >> +++ b/virt/kvm/arm/arch_timer.c
> >> @@ -166,21 +166,36 @@ bool kvm_timer_should_fire(struct kvm_vcpu *vcpu)
> >> return cval <= now;
> >> }
> >>
> >> +/*
> >> + * Synchronize the timer IRQ state with the interrupt controller.
> >> + */
> >> static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level)
> >> {
> >> int ret;
> >> struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> >>
> >> - BUG_ON(!vgic_initialized(vcpu->kvm));
> >> -
> >> timer->active_cleared_last = false;
> >> timer->irq.level = new_level;
> >> - trace_kvm_timer_update_irq(vcpu->vcpu_id, timer->irq.irq,
> >> + trace_kvm_timer_update_irq(vcpu->vcpu_id, host_vtimer_irq,
> >> timer->irq.level);
> >> - ret = kvm_vgic_inject_mapped_irq(vcpu->kvm, vcpu->vcpu_id,
> >> - timer->irq.irq,
> >> - timer->irq.level);
> >> - WARN_ON(ret);
> >> +
> >> + if (irqchip_in_kernel(vcpu->kvm)) {
> >> + BUG_ON(!vgic_initialized(vcpu->kvm));
> >> +
> >> + /* Fire the timer in the VGIC */
> >> + ret = kvm_vgic_inject_mapped_irq(vcpu->kvm, vcpu->vcpu_id,
> >> + timer->irq.irq,
> >> + timer->irq.level);
> >> +
> >> + WARN_ON(ret);
> >> + } else {
> >> + struct kvm_sync_regs *regs = &vcpu->run->s.regs;
> >> +
> >> + /* Populate the timer bitmap for user space */
> >> + regs->kernel_timer_pending &= ~KVM_ARM_TIMER_VTIMER;
> >> + if (new_level)
> >> + regs->kernel_timer_pending |= KVM_ARM_TIMER_VTIMER;
> >
> > I think if you got here, it means you have to exit to userspace to
> > update it of the new state. If you don't want to propagate a return
>
> Yes, but we can't exit straight away with our own exit reason because we
> might be inside an MMIO exit path here which already occupies the
> exit_reason.
>
> > value from here, I think you should just not do anything an then later
> > compare timer->irq.level with whatever was last written to
> > run->kernel_timer_pending (which should be named something else than
> > pending).
>
> Hm, so you're saying we should just update kernel_timer_pending in
> flush_hwstate()? That way we miss out on the piggy backing, no?
>
I don't see why. What I had in mind was something like this (untested,
incomplete, pseudo-code'ish):
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 85a3f90..a78ce7d 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -549,7 +549,7 @@ static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
*/
int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
{
- int ret;
+ int ret, timer_ret;
sigset_t sigsaved;
if (unlikely(!kvm_vcpu_initialized(vcpu)))
@@ -588,7 +588,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
*/
preempt_disable();
kvm_pmu_flush_hwstate(vcpu);
- kvm_timer_flush_hwstate(vcpu);
+ timer_ret = kvm_timer_flush_hwstate(vcpu);
kvm_vgic_flush_hwstate(vcpu);
local_irq_disable();
@@ -596,7 +596,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
/*
* Re-check atomic conditions
*/
- if (signal_pending(current)) {
+ if (timer_ret || signal_pending(current)) {
ret = -EINTR;
run->exit_reason = KVM_EXIT_INTR;
}
@@ -659,13 +659,17 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
* interrupt line.
*/
kvm_pmu_sync_hwstate(vcpu);
- kvm_timer_sync_hwstate(vcpu);
+ timer_ret = kvm_timer_sync_hwstate(vcpu);
kvm_vgic_sync_hwstate(vcpu);
preempt_enable();
ret = handle_exit(vcpu, run, ret);
+ if (!ret & timer_ret) {
+ ret = -EINTR;
+ run->exit_reason = KVM_EXIT_INTR;
+ }
}
if (vcpu->sigset_active)
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 27a1f63..0f7c23d 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -242,6 +242,19 @@ void kvm_timer_unschedule(struct kvm_vcpu *vcpu)
timer_disarm(timer);
}
+static int kvm_timer_update_user_irqchip(struct kvm_vcpu *vcpu)
+{
+ struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+ struct kvm_sync_regs *regs = &vcpu->run->s.regs;
+
+ if (irqchip_in_kernel(vcpu->kvm))
+ return 0;
+
+ if (timer->irq.level != (regs->kernel_timer_pending & KVM_ARM_TIMER_VTIMER))
+ return 1;
+ return 0;
+}
+
/**
* kvm_timer_flush_hwstate - prepare to move the virt timer to the cpu
* @vcpu: The vcpu pointer
@@ -258,6 +271,11 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
if (kvm_timer_update_state(vcpu))
return;
+ if (!irqchip_in_kernel(vcpu->kvm)) {
+ /* do your masking here */
+ return kvm_timer_update_user_irqchip(vcpu);
+ }
+
/*
* If we enter the guest with the virtual input level to the VGIC
* asserted, then we have already told the VGIC what we need to, and
@@ -315,8 +333,10 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
*
* Check if the virtual timer has expired while we were running in the guest,
* and inject an interrupt if that was the case.
+ *
+ * Return 1 to force exit to userspace, 0 otherwise.
*/
-void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu)
+int kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu)
{
struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
@@ -327,6 +347,8 @@ void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu)
* could have expired, update the timer state.
*/
kvm_timer_update_state(vcpu);
+
+ return kvm_timer_update_user_irqchip(vcpu);
}
int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
> >
> >> + }
> >> }
> >>
> >> /*
> >> @@ -197,7 +212,8 @@ static int kvm_timer_update_state(struct kvm_vcpu *vcpu)
> >> * because the guest would never see the interrupt. Instead wait
> >> * until we call this function from kvm_timer_flush_hwstate.
> >> */
> >> - if (!vgic_initialized(vcpu->kvm) || !timer->enabled)
> >> + if ((irqchip_in_kernel(vcpu->kvm) && !vgic_initialized(vcpu->kvm)) ||
> >> + !timer->enabled)
> >> return -ENODEV;
> >>
> >> if (kvm_timer_should_fire(vcpu) != timer->irq.level)
> >> @@ -248,15 +264,20 @@ void kvm_timer_unschedule(struct kvm_vcpu *vcpu)
> >> *
> >> * Check if the virtual timer has expired while we were running in the host,
> >> * and inject an interrupt if that was the case.
> >> + *
> >> + * Returns:
> >> + *
> >> + * 0 - success
> >> + * 1 - need exit to user space
> >
> > this is opposite to all other exit-related APIs we have. Why not just
> > return -EINTR?
>
> All functions in the arch timer file are in normal C API notion
> (0=success). I actually had it all in exit-related API notion at first
> and it became very awkward and hard to read to convert exit codes in
> between. Doing it just once in kvm.c felt much cleaner.
>
ok, that's fair.
> >
> >> */
> >> -void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
> >> +int kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
> >> {
> >> struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> >> bool phys_active;
> >> int ret;
> >>
> >> if (kvm_timer_update_state(vcpu))
> >> - return;
> >> + return 0;
> >>
> >> /*
> >> * If we enter the guest with the virtual input level to the VGIC
> >> @@ -275,38 +296,61 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
> >> * to ensure that hardware interrupts from the timer triggers a guest
> >> * exit.
> >> */
> >> - phys_active = timer->irq.level ||
> >> - kvm_vgic_map_is_active(vcpu, timer->irq.irq);
> >> -
> >> - /*
> >> - * We want to avoid hitting the (re)distributor as much as
> >> - * possible, as this is a potentially expensive MMIO access
> >> - * (not to mention locks in the irq layer), and a solution for
> >> - * this is to cache the "active" state in memory.
> >> - *
> >> - * Things to consider: we cannot cache an "active set" state,
> >> - * because the HW can change this behind our back (it becomes
> >> - * "clear" in the HW). We must then restrict the caching to
> >> - * the "clear" state.
> >> - *
> >> - * The cache is invalidated on:
> >> - * - vcpu put, indicating that the HW cannot be trusted to be
> >> - * in a sane state on the next vcpu load,
> >> - * - any change in the interrupt state
> >> - *
> >> - * Usage conditions:
> >> - * - cached value is "active clear"
> >> - * - value to be programmed is "active clear"
> >> - */
> >> - if (timer->active_cleared_last && !phys_active)
> >> - return;
> >> -
> >> - ret = irq_set_irqchip_state(host_vtimer_irq,
> >> - IRQCHIP_STATE_ACTIVE,
> >> - phys_active);
> >> - WARN_ON(ret);
> >> + if (irqchip_in_kernel(vcpu->kvm)) {
> >> + phys_active = timer->irq.level ||
> >> + kvm_vgic_map_is_active(vcpu, timer->irq.irq);
> >> +
> >> + /*
> >> + * We want to avoid hitting the (re)distributor as much as
> >> + * possible, as this is a potentially expensive MMIO access
> >> + * (not to mention locks in the irq layer), and a solution for
> >> + * this is to cache the "active" state in memory.
> >> + *
> >> + * Things to consider: we cannot cache an "active set" state,
> >> + * because the HW can change this behind our back (it becomes
> >> + * "clear" in the HW). We must then restrict the caching to
> >> + * the "clear" state.
> >> + *
> >> + * The cache is invalidated on:
> >> + * - vcpu put, indicating that the HW cannot be trusted to be
> >> + * in a sane state on the next vcpu load,
> >> + * - any change in the interrupt state
> >> + *
> >> + * Usage conditions:
> >> + * - cached value is "active clear"
> >> + * - value to be programmed is "active clear"
> >> + */
> >> + if (timer->active_cleared_last && !phys_active)
> >> + return 0;
> >> +
> >> + ret = irq_set_irqchip_state(host_vtimer_irq,
> >> + IRQCHIP_STATE_ACTIVE,
> >> + phys_active);
> >> + WARN_ON(ret);
> >> + } else {
> >> + struct kvm_sync_regs *regs = &vcpu->run->s.regs;
> >> +
> >> + /*
> >> + * User space handles timer events, so we need to check whether
> >> + * its view of the world is in sync with ours.
> >> + */
> >> + if (regs->kernel_timer_pending != regs->user_timer_pending) {
> >> + /* Return to user space */
> >> + return 1;
> >> + }
> >
> > Maybe I'm misunderstanding and user_timer_pending is just a cached
> > verison of what you said last, but as I said above, I think you can just
> > compare timer->irq.level with the last value the kvm_run struct, and if
> > something changed, you have to exit.
>
> So how would user space know whether the line went up or down? Or didn't
> change at all (if we coalesce with an MMIO exit)?
>
It just samples the line on every exit? It is free to cache the old
value if it wants to detect changes as opposed to recomputing the line
on every exit, but I don't understand why it has to feed it back to the
kernel. Surely userspace has its own per-VCPU state?
> >
> >> +
> >> + /*
> >> + * As long as user space is aware that the timer is pending,
> >> + * we do not need to get new host timer events.
> >> + */
> >
> > yes, correct, but I don't think this concept was clearly reflected in
> > your API text above.
> >
> >> + if (timer->irq.level)
> >> + disable_percpu_irq(host_vtimer_irq);
> >> + else
> >> + enable_percpu_irq(host_vtimer_irq, 0);
> >> + }
> >
> > could we move these two blocks into their own functions instead? That
> > would also give nice names to the huge chunk of complicated
> > functionality, e.g. flush_timer_state_to_user() and
> > flush_timer_state_to_vgic().
>
> That's probably a very useful cleanup, yes :).
>
>
Thanks,
-Christoffer
^ permalink raw reply related
* [PATCH 2/3] gpio: Add a driver for the Raspberry Pi's firmware GPIO calls.
From: Linus Walleij @ 2016-09-23 9:08 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160919161314.25858-2-eric@anholt.net>
On Mon, Sep 19, 2016 at 6:13 PM, Eric Anholt <eric@anholt.net> wrote:
> This driver will be used for accessing the FXL6408 GPIO exander on the
> Pi3. We can't drive it directly from Linux because the firmware is
> continuously polling one of the expander's lines to do its
> undervoltage detection.
>
> Signed-off-by: Eric Anholt <eric@anholt.net>
(...)
> +config GPIO_RASPBERRYPI
> + tristate "Raspberry Pi firmware-based GPIO access"
> + depends on OF_GPIO && RASPBERRYPI_FIRMWARE && (ARCH_BCM2835 || COMPILE_TEST)
> + help
> + Turns on support for using the Raspberry Pi firmware to
> + control GPIO pins. Used for access to the FXL6408 GPIO
> + expander on the Pi 3.
Maybe it should be named GPIO_RPI_FXL6408 ?
(No strong opinion.)
> +#include <linux/err.h>
> +#include <linux/gpio.h>
No, only #include <linux/gpio/driver.h>
> +static int rpi_gpio_dir_in(struct gpio_chip *gc, unsigned off)
> +{
> + /* We don't have direction control. */
> + return -EINVAL;
> +}
> +
> +static int rpi_gpio_dir_out(struct gpio_chip *gc, unsigned off, int val)
> +{
> + /* We don't have direction control. */
> + return -EINVAL;
> +}
IMO this should return OK if you try to set it to the direction
that the line is hardwired for in that case, not just fail everything.
If you return errors here, any generic driver that tries to
set the line as input or output will fail and then require a
second workaround in that driver if it is used on rpi etc etc.
Try to return zero if the consumer asks for the direction that
the line is set to.
Also implement .get_direction(). Doing so will show how to
do the above two calls in the right way.
> +static void rpi_gpio_set(struct gpio_chip *gc, unsigned off, int val)
> +{
> + struct rpi_gpio *rpi = container_of(gc, struct rpi_gpio, gc);
> + u32 packet[2] = { rpi->offset + off, val };
> + int ret;
> +
> + ret = rpi_firmware_property(rpi->firmware,
> + RPI_FIRMWARE_SET_GPIO_STATE,
> + &packet, sizeof(packet));
> + if (ret)
> + dev_err(rpi->dev, "Error setting GPIO %d state: %d\n", off, ret);
> +}
> +
> +static int rpi_gpio_get(struct gpio_chip *gc, unsigned off)
> +{
> + struct rpi_gpio *rpi = container_of(gc, struct rpi_gpio, gc);
> + u32 packet[2] = { rpi->offset + off, 0 };
> + int ret;
> +
> + ret = rpi_firmware_property(rpi->firmware,
> + RPI_FIRMWARE_GET_GPIO_STATE,
> + &packet, sizeof(packet));
> + if (ret) {
> + dev_err(rpi->dev, "Error getting GPIO state: %d\n", ret);
> + return ret;
> + } else if (packet[0]) {
> + dev_err(rpi->dev, "Firmware error getting GPIO state: %d\n",
> + packet[0]);
> + return -EINVAL;
> + } else {
> + return packet[1];
> + }
You can just remove the last } else { } clause and return on a
single line.
Please do it like this though:
return !!packet[1];
So you clamp the returned value to a boolean.
> +static int rpi_gpio_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
> + struct device_node *fw_node;
> + struct rpi_gpio *rpi;
> + u32 ngpio;
> + int ret;
> +
> + rpi = devm_kzalloc(dev, sizeof *rpi, GFP_KERNEL);
> + if (!rpi)
> + return -ENOMEM;
> + rpi->dev = dev;
> +
> + fw_node = of_parse_phandle(np, "firmware", 0);
> + if (!fw_node) {
> + dev_err(dev, "Missing firmware node\n");
> + return -ENOENT;
> + }
> +
> + rpi->firmware = rpi_firmware_get(fw_node);
> + if (!rpi->firmware)
> + return -EPROBE_DEFER;
> +
> + if (of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpio)) {
> + dev_err(dev, "Missing ngpios");
> + return -ENOENT;
> + }
This is suspect you could skip and just hardcode to 8.
> + if (of_property_read_u32(pdev->dev.of_node,
> + "raspberrypi,firmware-gpio-offset",
> + &rpi->offset)) {
> + dev_err(dev, "Missing raspberrypi,firmware-gpio-offset");
> + return -ENOENT;
> + }
Can't you just hardcode this into the driver as well?
> + rpi->gc.label = np->full_name;
> + rpi->gc.owner = THIS_MODULE;
> + rpi->gc.of_node = np;
> + rpi->gc.ngpio = ngpio;
> + rpi->gc.direction_input = rpi_gpio_dir_in;
> + rpi->gc.direction_output = rpi_gpio_dir_out;
Implement .get_direction()
> + rpi->gc.get = rpi_gpio_get;
> + rpi->gc.set = rpi_gpio_set;
> + rpi->gc.can_sleep = true;
> +
> + ret = gpiochip_add(&rpi->gc);
> + if (ret)
> + return ret;
Use devm_gpiochip_add_data() and pass NULL as data
so you can still use the devm* function.
> + platform_set_drvdata(pdev, rpi);
Should not be needed after that.
> + return 0;
> +}
> +
> +static int rpi_gpio_remove(struct platform_device *pdev)
> +{
> + struct rpi_gpio *rpi = platform_get_drvdata(pdev);
> +
> + gpiochip_remove(&rpi->gc);
> +
> + return 0;
> +}
Should be possible to drop after using devm_gpiochip_add_data()
> diff --git a/include/soc/bcm2835/raspberrypi-firmware.h b/include/soc/bcm2835/raspberrypi-firmware.h
> index 3fb357193f09..671ccd00aea2 100644
> --- a/include/soc/bcm2835/raspberrypi-firmware.h
> +++ b/include/soc/bcm2835/raspberrypi-firmware.h
> @@ -73,11 +73,13 @@ enum rpi_firmware_property_tag {
> RPI_FIRMWARE_GET_DISPMANX_RESOURCE_MEM_HANDLE = 0x00030014,
> RPI_FIRMWARE_GET_EDID_BLOCK = 0x00030020,
> RPI_FIRMWARE_GET_DOMAIN_STATE = 0x00030030,
> + RPI_FIRMWARE_GET_GPIO_STATE = 0x00030041,
> RPI_FIRMWARE_SET_CLOCK_STATE = 0x00038001,
> RPI_FIRMWARE_SET_CLOCK_RATE = 0x00038002,
> RPI_FIRMWARE_SET_VOLTAGE = 0x00038003,
> RPI_FIRMWARE_SET_TURBO = 0x00038009,
> RPI_FIRMWARE_SET_DOMAIN_STATE = 0x00038030,
> + RPI_FIRMWARE_SET_GPIO_STATE = 0x00038041,
Can you please merge this orthogonally into the rpi tree to ARM SoC?
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH v5] KVM: arm/arm64: Route vtimer events to user space
From: Alexander Graf @ 2016-09-23 9:10 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <63a1e3a2-f551-017f-b6be-593b53a0b407@redhat.com>
On 23.09.16 10:57, Paolo Bonzini wrote:
>
>
> On 23/09/2016 09:14, Alexander Graf wrote:
>>>> +/*
>>>> + * Synchronize the timer IRQ state with the interrupt controller.
>>>> + */
>>>> static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level)
>>>> {
>>>> int ret;
>>>> struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
>>>>
>>>> timer->active_cleared_last = false;
>>>> timer->irq.level = new_level;
>>>> - trace_kvm_timer_update_irq(vcpu->vcpu_id, timer->irq.irq,
>>>> + trace_kvm_timer_update_irq(vcpu->vcpu_id, host_vtimer_irq,
>>>> timer->irq.level);
>>>> + [...]
>>>> + struct kvm_sync_regs *regs = &vcpu->run->s.regs;
>>>> +
>>>> + /* Populate the timer bitmap for user space */
>>>> + regs->kernel_timer_pending &= ~KVM_ARM_TIMER_VTIMER;
>>>> + if (new_level)
>>>> + regs->kernel_timer_pending |= KVM_ARM_TIMER_VTIMER;
>>>
>>> I think if you got here, it means you have to exit to userspace to
>>> update it of the new state. If you don't want to propagate a return
>>
>> Yes, but we can't exit straight away with our own exit reason because we
>> might be inside an MMIO exit path here which already occupies the
>> exit_reason.
>
> So the idea is that whenever you're here you have one of the following
> cases:
>
> - are coming from kvm_timer_flush_hwstate, and then you exit immediately
> with KVM_EXIT_INTR if needed
>
> - you are coming from the kvm_timer_sync_hwstate just before
> handle_exit. Then if there's a vmexit you have already set
> regs->kernel_timer_pending, if not you'll do a kvm_timer_flush_hwstate soon.
>
> - you are coming from the kvm_timer_sync_hwstate in the middle of
> kvm_arch_vcpu_ioctl_run, and then "continue" will either exit the loop
> immediately (if ret <= 0) or go to kvm_timer_flush_hwstate as in the
> previous case
>
> Right?
Yup :)
>
>>> Maybe I'm misunderstanding and user_timer_pending is just a cached
>>> verison of what you said last, but as I said above, I think you can just
>>> compare timer->irq.level with the last value the kvm_run struct, and if
>>> something changed, you have to exit.
>>
>> So how would user space know whether the line went up or down? Or didn't
>> change at all (if we coalesce with an MMIO exit)?
>
> It would save the status of the line somewhere in its own variable,
> without introducing a relatively delicate API between kernel and user.
>
> I agree that you cannot update kernel_timer_pending only in
> flush_hwstate; otherwise you miss on case (2) when there is a vmexit.
> That has to stay in kvm_timer_sync_hwstate (or it could become a new
> function called at the very end of kvm_arch_vcpu_ioctl_run).
The beauty of having it in the timer update function is that it gets
called from flush_hwstate() as well. That way we catch the update also
in cases where we need it before we enter the vcpu.
> However, I'm not sure why user_timer_pending is necessary. If it is
> just the value you assigned to regs->kernel_timer_pending at the time of
> the previous vmexit, can kvm_timer_flush_hwstate just do
>
> if (timer->prev_kernel_timer_pending != regs->kernel_timer_pending) {
> timer->prev_kernel_timer_pending = regs->kernel_timer_pending;
> return 1;
> }
>
> ? Or even
>
> if (timer->prev_irq_level != timer->irq.level) {
> timer->prev_irq_level = regs->irq.level;
> return 1;
> }
>
> so that regs->kernel_timer_pending remains exclusively
> kvm_timer_sync_hwstate's business?
We could do that too, yes. But it puts more burden on user space - it
would have to ensure that it *always* checks for the pending timer
status. With this approach, user space may opt to only check for timer
state changes on -EINTR and things would still work.
Alex
^ permalink raw reply
* [PATCH v2 0/2] BQL support and fix for a regression issue
From: sunil.kovvuri at gmail.com @ 2016-09-23 9:12 UTC (permalink / raw)
To: linux-arm-kernel
From: Sunil Goutham <sgoutham@cavium.com>
These patches add byte queue limit support and also fixes a regression
issue introduced by commit
'net: thunderx: Use netdev's name for naming VF's interrupts'
Changes from v1:
- As suggested added 'Fixes' tag with commit id of previous commit
which cuased issue.
- Also fixed the missing netdev_tx_reset_queue() function call in
byte queue limits support patch.
Sunil Goutham (2):
net: thunderx: Fix issue with IRQ namimg
net: thunderx: Support for byte queue limits
drivers/net/ethernet/cavium/thunder/nic.h | 2 +-
drivers/net/ethernet/cavium/thunder/nicvf_main.c | 14 ++++++++--
drivers/net/ethernet/cavium/thunder/nicvf_queues.c | 30 ++++++++++++++--------
3 files changed, 33 insertions(+), 13 deletions(-)
--
2.7.4
^ permalink raw reply
* [PATCH v2 1/2] net: thunderx: Fix issue with IRQ namimg
From: sunil.kovvuri at gmail.com @ 2016-09-23 9:12 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474621948-9172-1-git-send-email-sunil.kovvuri@gmail.com>
From: Sunil Goutham <sgoutham@cavium.com>
This patch fixes a regression caused by previous commit
when irq name exceeds 20 byte array if interface's name
size is large.
Fixes: e412621394fa ("net: thunderx: Use netdev's name for naming VF's interrupts")
Signed-off-by: Sunil Goutham <sgoutham@cavium.com>
---
drivers/net/ethernet/cavium/thunder/nic.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
index 18d12d3..3042610 100644
--- a/drivers/net/ethernet/cavium/thunder/nic.h
+++ b/drivers/net/ethernet/cavium/thunder/nic.h
@@ -305,7 +305,7 @@ struct nicvf {
bool msix_enabled;
u8 num_vec;
struct msix_entry msix_entries[NIC_VF_MSIX_VECTORS];
- char irq_name[NIC_VF_MSIX_VECTORS][20];
+ char irq_name[NIC_VF_MSIX_VECTORS][IFNAMSIZ + 15];
bool irq_allocated[NIC_VF_MSIX_VECTORS];
cpumask_var_t affinity_mask[NIC_VF_MSIX_VECTORS];
--
2.7.4
^ permalink raw reply related
* [PATCH v2 2/2] net: thunderx: Support for byte queue limits
From: sunil.kovvuri at gmail.com @ 2016-09-23 9:12 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474621948-9172-1-git-send-email-sunil.kovvuri@gmail.com>
From: Sunil Goutham <sgoutham@cavium.com>
This patch adds support for byte queue limits
Signed-off-by: Sunil Goutham <sgoutham@cavium.com>
---
drivers/net/ethernet/cavium/thunder/nicvf_main.c | 14 ++++++++--
drivers/net/ethernet/cavium/thunder/nicvf_queues.c | 30 ++++++++++++++--------
2 files changed, 32 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
index 7d00162..45a13f7 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
@@ -516,7 +516,8 @@ static int nicvf_init_resources(struct nicvf *nic)
static void nicvf_snd_pkt_handler(struct net_device *netdev,
struct cmp_queue *cq,
struct cqe_send_t *cqe_tx,
- int cqe_type, int budget)
+ int cqe_type, int budget,
+ unsigned int *tx_pkts, unsigned int *tx_bytes)
{
struct sk_buff *skb = NULL;
struct nicvf *nic = netdev_priv(netdev);
@@ -547,6 +548,8 @@ static void nicvf_snd_pkt_handler(struct net_device *netdev,
}
nicvf_put_sq_desc(sq, hdr->subdesc_cnt + 1);
prefetch(skb);
+ (*tx_pkts)++;
+ *tx_bytes += skb->len;
napi_consume_skb(skb, budget);
sq->skbuff[cqe_tx->sqe_ptr] = (u64)NULL;
} else {
@@ -662,6 +665,7 @@ static int nicvf_cq_intr_handler(struct net_device *netdev, u8 cq_idx,
struct cmp_queue *cq = &qs->cq[cq_idx];
struct cqe_rx_t *cq_desc;
struct netdev_queue *txq;
+ unsigned int tx_pkts = 0, tx_bytes = 0;
spin_lock_bh(&cq->lock);
loop:
@@ -701,7 +705,7 @@ loop:
case CQE_TYPE_SEND:
nicvf_snd_pkt_handler(netdev, cq,
(void *)cq_desc, CQE_TYPE_SEND,
- budget);
+ budget, &tx_pkts, &tx_bytes);
tx_done++;
break;
case CQE_TYPE_INVALID:
@@ -730,6 +734,9 @@ done:
netdev = nic->pnicvf->netdev;
txq = netdev_get_tx_queue(netdev,
nicvf_netdev_qidx(nic, cq_idx));
+ if (tx_pkts)
+ netdev_tx_completed_queue(txq, tx_pkts, tx_bytes);
+
nic = nic->pnicvf;
if (netif_tx_queue_stopped(txq) && netif_carrier_ok(netdev)) {
netif_tx_start_queue(txq);
@@ -1160,6 +1167,9 @@ int nicvf_stop(struct net_device *netdev)
netif_tx_disable(netdev);
+ for (qidx = 0; qidx < netdev->num_tx_queues; qidx++)
+ netdev_tx_reset_queue(netdev_get_tx_queue(netdev, qidx));
+
/* Free resources */
nicvf_config_data_transfer(nic, false);
diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_queues.c b/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
index 178c5c7..a4fc501 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
@@ -1082,6 +1082,24 @@ static inline void nicvf_sq_add_cqe_subdesc(struct snd_queue *sq, int qentry,
imm->len = 1;
}
+static inline void nicvf_sq_doorbell(struct nicvf *nic, struct sk_buff *skb,
+ int sq_num, int desc_cnt)
+{
+ struct netdev_queue *txq;
+
+ txq = netdev_get_tx_queue(nic->pnicvf->netdev,
+ skb_get_queue_mapping(skb));
+
+ netdev_tx_sent_queue(txq, skb->len);
+
+ /* make sure all memory stores are done before ringing doorbell */
+ smp_wmb();
+
+ /* Inform HW to xmit all TSO segments */
+ nicvf_queue_reg_write(nic, NIC_QSET_SQ_0_7_DOOR,
+ sq_num, desc_cnt);
+}
+
/* Segment a TSO packet into 'gso_size' segments and append
* them to SQ for transfer
*/
@@ -1141,12 +1159,8 @@ static int nicvf_sq_append_tso(struct nicvf *nic, struct snd_queue *sq,
/* Save SKB in the last segment for freeing */
sq->skbuff[hdr_qentry] = (u64)skb;
- /* make sure all memory stores are done before ringing doorbell */
- smp_wmb();
+ nicvf_sq_doorbell(nic, skb, sq_num, desc_cnt);
- /* Inform HW to xmit all TSO segments */
- nicvf_queue_reg_write(nic, NIC_QSET_SQ_0_7_DOOR,
- sq_num, desc_cnt);
nic->drv_stats.tx_tso++;
return 1;
}
@@ -1219,12 +1233,8 @@ doorbell:
nicvf_sq_add_cqe_subdesc(sq, qentry, tso_sqe, skb);
}
- /* make sure all memory stores are done before ringing doorbell */
- smp_wmb();
+ nicvf_sq_doorbell(nic, skb, sq_num, subdesc_cnt);
- /* Inform HW to xmit new packet */
- nicvf_queue_reg_write(nic, NIC_QSET_SQ_0_7_DOOR,
- sq_num, subdesc_cnt);
return 1;
append_fail:
--
2.7.4
^ permalink raw reply related
* [PATCH v5] KVM: arm/arm64: Route vtimer events to user space
From: Paolo Bonzini @ 2016-09-23 9:15 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <7afbc08e-0ba2-d6fd-c1ed-e5822e05c00b@suse.de>
On 23/09/2016 11:10, Alexander Graf wrote:
>>>> Maybe I'm misunderstanding and user_timer_pending is just a cached
>>>> verison of what you said last, but as I said above, I think you can just
>>>> compare timer->irq.level with the last value the kvm_run struct, and if
>>>> something changed, you have to exit.
>>>
>>> So how would user space know whether the line went up or down? Or didn't
>>> change at all (if we coalesce with an MMIO exit)?
>>
>> It would save the status of the line somewhere in its own variable,
>> without introducing a relatively delicate API between kernel and user.
>>
>> I agree that you cannot update kernel_timer_pending only in
>> flush_hwstate; otherwise you miss on case (2) when there is a vmexit.
>> That has to stay in kvm_timer_sync_hwstate (or it could become a new
>> function called at the very end of kvm_arch_vcpu_ioctl_run).
>
> The beauty of having it in the timer update function is that it gets
> called from flush_hwstate() as well. That way we catch the update also
> in cases where we need it before we enter the vcpu.
Yeah, the timer update function is ideal because it is called from both
flush_hwstate and sync_hwstate.
>> However, I'm not sure why user_timer_pending is necessary. If it is
>> just the value you assigned to regs->kernel_timer_pending at the time of
>> the previous vmexit, can kvm_timer_flush_hwstate just do
>>
>> if (timer->prev_kernel_timer_pending != regs->kernel_timer_pending) {
>> timer->prev_kernel_timer_pending = regs->kernel_timer_pending;
>> return 1;
>> }
>>
>> ? Or even
>>
>> if (timer->prev_irq_level != timer->irq.level) {
>> timer->prev_irq_level = regs->irq.level;
>> return 1;
>> }
>>
>> so that regs->kernel_timer_pending remains exclusively
>> kvm_timer_sync_hwstate's business?
>
> We could do that too, yes. But it puts more burden on user space - it
> would have to ensure that it *always* checks for the pending timer
> status. With this approach, user space may opt to only check for timer
> state changes on -EINTR and things would still work.
That would be overloading EINTR a bit though. But that's a digression,
I don't think it matters much.
On the other hand, what happens if you run new QEMU with old userspace?
With user_timer_pending you'd get an infinite stream of vmexits the
first time the timer fires, wouldn't you? Whereas if you keep it in the
kernel, userspace would simply not get the interrupt (because it doesn't
know about kernel_timer_pending) and think it got a spurious vmexit.
The kernel's IRQ would stay masked and everything would just (not) work
like before your patch?
Thanks,
Paolo
^ permalink raw reply
* [PATCH v5] KVM: arm/arm64: Route vtimer events to user space
From: Alexander Graf @ 2016-09-23 9:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <e4c08096-03bd-3ec3-998d-41643992ab1a@redhat.com>
On 23.09.16 11:15, Paolo Bonzini wrote:
>
>
> On 23/09/2016 11:10, Alexander Graf wrote:
>>>>> Maybe I'm misunderstanding and user_timer_pending is just a cached
>>>>> verison of what you said last, but as I said above, I think you can just
>>>>> compare timer->irq.level with the last value the kvm_run struct, and if
>>>>> something changed, you have to exit.
>>>>
>>>> So how would user space know whether the line went up or down? Or didn't
>>>> change at all (if we coalesce with an MMIO exit)?
>>>
>>> It would save the status of the line somewhere in its own variable,
>>> without introducing a relatively delicate API between kernel and user.
>>>
>>> I agree that you cannot update kernel_timer_pending only in
>>> flush_hwstate; otherwise you miss on case (2) when there is a vmexit.
>>> That has to stay in kvm_timer_sync_hwstate (or it could become a new
>>> function called at the very end of kvm_arch_vcpu_ioctl_run).
>>
>> The beauty of having it in the timer update function is that it gets
>> called from flush_hwstate() as well. That way we catch the update also
>> in cases where we need it before we enter the vcpu.
>
> Yeah, the timer update function is ideal because it is called from both
> flush_hwstate and sync_hwstate.
>
>>> However, I'm not sure why user_timer_pending is necessary. If it is
>>> just the value you assigned to regs->kernel_timer_pending at the time of
>>> the previous vmexit, can kvm_timer_flush_hwstate just do
>>>
>>> if (timer->prev_kernel_timer_pending != regs->kernel_timer_pending) {
>>> timer->prev_kernel_timer_pending = regs->kernel_timer_pending;
>>> return 1;
>>> }
>>>
>>> ? Or even
>>>
>>> if (timer->prev_irq_level != timer->irq.level) {
>>> timer->prev_irq_level = regs->irq.level;
>>> return 1;
>>> }
>>>
>>> so that regs->kernel_timer_pending remains exclusively
>>> kvm_timer_sync_hwstate's business?
>>
>> We could do that too, yes. But it puts more burden on user space - it
>> would have to ensure that it *always* checks for the pending timer
>> status. With this approach, user space may opt to only check for timer
>> state changes on -EINTR and things would still work.
>
> That would be overloading EINTR a bit though. But that's a digression,
> I don't think it matters much.
>
> On the other hand, what happens if you run new QEMU with old userspace?
> With user_timer_pending you'd get an infinite stream of vmexits the
> first time the timer fires, wouldn't you? Whereas if you keep it in the
> kernel, userspace would simply not get the interrupt (because it doesn't
> know about kernel_timer_pending) and think it got a spurious vmexit.
> The kernel's IRQ would stay masked and everything would just (not) work
> like before your patch?
Yes, we'd definitely stay more compatible by tracking it only in the
kernel. I'm not fully convinced that it's the better interface, but
since both Christoffer and you seem to choke on that part, I'll give it
a stab ;).
Alex
^ permalink raw reply
* [RFC PATCH 0/3] efi: MMC proxy support for the UEFI varstore
From: Mark Rutland @ 2016-09-23 9:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAKv+Gu-H=JkXFSm2bO4tFcUEVPs2ueT3za2X+aUjrnGZZSdUvQ@mail.gmail.com>
On Thu, Sep 22, 2016 at 02:37:00PM +0100, Ard Biesheuvel wrote:
> On 22 September 2016 at 13:58, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Thu, Sep 22, 2016 at 12:30:03PM +0100, Ard Biesheuvel wrote:
> > I also think that this needs to go via the USWG (and to the UEFI spec),
> > before we can consider using it.
> The primary issue there is that the variable protocols are
> architectural DXE protocols defined in the PI spec, not the UEFI spec
> (and the MMC host protocol I am using here is not in any spec afaik),
> and so implementing this in a UEFI compliant way is not currently
> possible. Exposing a UEFI protocol like block I/O is not sufficient,
> since the variable protocols are depended upon earlier in the boot
> sequence (in UEFI implementations based on PI).
>
> So yes, this needs to be discussed (and it will be, but not out in the
> open, unfortunately.) However, it is unclear whether it is feasible to
> address this at the UEFI level, or whether we will need to go beyond
> and define something based on the PI spec directly. (/me looks at
> Charles)
That is... less than optimal. :(
> > I have a few other general concerns:
> > * Lifetime guarantees
> >
> > When is it valid for EFI to call the MMC proxy? Can other services
> > (e.g. ACPI) call this?
> >
> > How do we handle kexec/kdump? e.g. how do we teardown the interface
> > before branching to a new kernel, how do we safely tear down a crashed
> > kernel's interface, what can we call before doing so?
> >
> > How do we handle suspend/resume? e.g. is it necessary to re-register
> > upon resume?
>
> All good questions, and exactly the kind of feedback I am seeking.
>
> Tearing down the interface could be as simple as clearing the pointer,
> but some synchronization is probably in order to make sure that no
> calls are in progress.
>
> But to clarify the purpose of this series: are there any concerns
> regarding exposing callbacks to the firmware in general, and for MMC
> access in particular, from the Linux side?
I have a general uneasiness about having UEFI call kernel function
pointers, inverting the usual relationship, but I'll need to think about
that a little further -- the lifetime stuff was the most obvious problem
in that area, but I'm sure there are more. ;)
It would really help to know *when* UEFI is expected/allowed to call
this. For synchronous calls, the FW could return a new
requires-os-support error code, and the OS could then call some API,
figure out what to do, and later send any acknowledgement required back
to UEFI. That might not cover all cases, though.
Thanks,
Mark.
^ permalink raw reply
* [PATCH v5] KVM: arm/arm64: Route vtimer events to user space
From: Paolo Bonzini @ 2016-09-23 9:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <70fc788d-f99e-4ba5-67a2-da9c1a857dfe@suse.de>
On 23/09/2016 11:17, Alexander Graf wrote:
>> >
>> > On the other hand, what happens if you run new QEMU with old userspace?
>> > With user_timer_pending you'd get an infinite stream of vmexits the
>> > first time the timer fires, wouldn't you? Whereas if you keep it in the
>> > kernel, userspace would simply not get the interrupt (because it doesn't
>> > know about kernel_timer_pending) and think it got a spurious vmexit.
>> > The kernel's IRQ would stay masked and everything would just (not) work
>> > like before your patch?
> Yes, we'd definitely stay more compatible by tracking it only in the
> kernel. I'm not fully convinced that it's the better interface, but
> since both Christoffer and you seem to choke on that part, I'll give it
> a stab ;).
Cool! FWIW my suggestion for kernel_timer_pending's name would be
timer_irq_level (nicely matching timer->irq.level in the kernel).
Paolo
^ permalink raw reply
* [PATCH] gpio: Added zynq specific check for special pins on bank zero
From: Linus Walleij @ 2016-09-23 9:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474360324-8168-1-git-send-email-navam@xilinx.com>
On Tue, Sep 20, 2016 at 10:32 AM, Nava kishore Manne
<nava.manne@xilinx.com> wrote:
> From: Nava kishore Manne <nava.manne@xilinx.com>
>
> This patch adds zynq specific check for bank 0 pins 7 and 8
> are special and cannot be used as inputs
>
> Signed-off-by: Nava kishore Manne <navam@xilinx.com>
I'm waiting for a revised version of this patch with comments adressed
and ACKs added.
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH v5] KVM: arm/arm64: Route vtimer events to user space
From: Christoffer Dall @ 2016-09-23 9:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <7afbc08e-0ba2-d6fd-c1ed-e5822e05c00b@suse.de>
On Fri, Sep 23, 2016 at 11:10:46AM +0200, Alexander Graf wrote:
>
>
> On 23.09.16 10:57, Paolo Bonzini wrote:
> >
> >
> > On 23/09/2016 09:14, Alexander Graf wrote:
> >>>> +/*
> >>>> + * Synchronize the timer IRQ state with the interrupt controller.
> >>>> + */
> >>>> static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level)
> >>>> {
> >>>> int ret;
> >>>> struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> >>>>
> >>>> timer->active_cleared_last = false;
> >>>> timer->irq.level = new_level;
> >>>> - trace_kvm_timer_update_irq(vcpu->vcpu_id, timer->irq.irq,
> >>>> + trace_kvm_timer_update_irq(vcpu->vcpu_id, host_vtimer_irq,
> >>>> timer->irq.level);
> >>>> + [...]
> >>>> + struct kvm_sync_regs *regs = &vcpu->run->s.regs;
> >>>> +
> >>>> + /* Populate the timer bitmap for user space */
> >>>> + regs->kernel_timer_pending &= ~KVM_ARM_TIMER_VTIMER;
> >>>> + if (new_level)
> >>>> + regs->kernel_timer_pending |= KVM_ARM_TIMER_VTIMER;
> >>>
> >>> I think if you got here, it means you have to exit to userspace to
> >>> update it of the new state. If you don't want to propagate a return
> >>
> >> Yes, but we can't exit straight away with our own exit reason because we
> >> might be inside an MMIO exit path here which already occupies the
> >> exit_reason.
> >
> > So the idea is that whenever you're here you have one of the following
> > cases:
> >
> > - are coming from kvm_timer_flush_hwstate, and then you exit immediately
> > with KVM_EXIT_INTR if needed
> >
> > - you are coming from the kvm_timer_sync_hwstate just before
> > handle_exit. Then if there's a vmexit you have already set
> > regs->kernel_timer_pending, if not you'll do a kvm_timer_flush_hwstate soon.
> >
> > - you are coming from the kvm_timer_sync_hwstate in the middle of
> > kvm_arch_vcpu_ioctl_run, and then "continue" will either exit the loop
> > immediately (if ret <= 0) or go to kvm_timer_flush_hwstate as in the
> > previous case
> >
> > Right?
>
> Yup :)
>
> >
> >>> Maybe I'm misunderstanding and user_timer_pending is just a cached
> >>> verison of what you said last, but as I said above, I think you can just
> >>> compare timer->irq.level with the last value the kvm_run struct, and if
> >>> something changed, you have to exit.
> >>
> >> So how would user space know whether the line went up or down? Or didn't
> >> change at all (if we coalesce with an MMIO exit)?
> >
> > It would save the status of the line somewhere in its own variable,
> > without introducing a relatively delicate API between kernel and user.
> >
> > I agree that you cannot update kernel_timer_pending only in
> > flush_hwstate; otherwise you miss on case (2) when there is a vmexit.
> > That has to stay in kvm_timer_sync_hwstate (or it could become a new
> > function called at the very end of kvm_arch_vcpu_ioctl_run).
>
> The beauty of having it in the timer update function is that it gets
> called from flush_hwstate() as well. That way we catch the update also
> in cases where we need it before we enter the vcpu.
>
> > However, I'm not sure why user_timer_pending is necessary. If it is
> > just the value you assigned to regs->kernel_timer_pending at the time of
> > the previous vmexit, can kvm_timer_flush_hwstate just do
> >
> > if (timer->prev_kernel_timer_pending != regs->kernel_timer_pending) {
> > timer->prev_kernel_timer_pending = regs->kernel_timer_pending;
> > return 1;
> > }
> >
> > ? Or even
> >
> > if (timer->prev_irq_level != timer->irq.level) {
> > timer->prev_irq_level = regs->irq.level;
> > return 1;
> > }
> >
> > so that regs->kernel_timer_pending remains exclusively
> > kvm_timer_sync_hwstate's business?
>
> We could do that too, yes. But it puts more burden on user space - it
> would have to ensure that it *always* checks for the pending timer
> status. With this approach, user space may opt to only check for timer
> state changes on -EINTR and things would still work.
>
Huh? I thought the whole point was that you could piggy back on an MMIO
exit with a change in the line state, for example. So I don't
understand this.
In any case, exposing internal historical state only used by the kernel
to figure out whether or not it should force an exit if there's not
already one happening, to user space, just feels weird to me, and my
emphasis is on having a clean timer emulation component in the kernel
where the semantics are clear. Have a look at my proposal in the other
mail on this thread.
-Christoffer
^ permalink raw reply
* [PATCH 3/3] arm64: Add the Raspberry Pi firmware's interface to the FXL6408.
From: Linus Walleij @ 2016-09-23 9:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474577091.12009.10.camel@redhat.com>
On Thu, Sep 22, 2016 at 10:44 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On Mo, 2016-09-19 at 17:13 +0100, Eric Anholt wrote:
>> This gets us hotplug detection of HDMI, so that graphics now works at
>> boot. Tested with watching the output of xrandr while plugging and
>> unplugging the HDMI cable.
>
> Very nice. Whole patch series:
>
> Tested-by: Gerd Hoffmann <kraxel@redhat.com>
>
> /me hopes this lands in the 4.9 merge window.
Sorry no way. I have comments on the driver and
bindings.
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH v2 0/3] arm64: kgdb: fix single stepping
From: Greg KH @ 2016-09-23 9:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160923092706.GN30248@linaro.org>
On Fri, Sep 23, 2016 at 06:27:07PM +0900, AKASHI Takahiro wrote:
> On Fri, Sep 23, 2016 at 10:43:41AM +0200, Greg KH wrote:
> > On Fri, Sep 23, 2016 at 05:32:58PM +0900, AKASHI Takahiro wrote:
> > > On Fri, Sep 23, 2016 at 10:16:18AM +0200, Greg KH wrote:
> > > > On Fri, Sep 23, 2016 at 04:33:24PM +0900, AKASHI Takahiro wrote:
> > > > > Kgdb support on arm64 was merged in v3.15, but from its first appearance,
> > > > > "signle step" has never worked well.
> > > > >
> > > > > This patch fixes all the error cases I found so far.
> > > > > The original patch[1] was splitted into three pieces, ones for each case.
> > > > > patch#1, #2 should be applied to all the version, v3.15 and later.
> > > > > pathc#3 only for v3.16 and later.
> > > >
> > > > As this is not a regression (i.e. it has never worked), why is this
> > > > something for stable releases?
> > >
> > > Because, I think, that is a bug.
> > > The author seems to have believed that it worked.
> > > Please see:
> > > commit 44679a4f
> > > Author: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> > > Date: Tue Jan 28 11:20:19 2014 +0000
> > >
> > > arm64: KGDB: Add step debugging support
> >
> > Yes, but again, it didn't work, so this would be a new feature.
>
> Yes, but again, it's a bug of kgdb on arm64 which is supposed
> to have been available since v3.15.
But it wasn't ever working, so it's not a regression or a bugfix, sorry.
> > Please read Documentation/stable_kernel_rules.txt for the requirements
> > of a stable kernel patch. I don't think this series meets those rules.
>
> Please re-think about merging this patch.
It's not even in Linus's tree yet, so there's nothing for me to merge
anywhere.
thanks,
greg k-h
^ permalink raw reply
* [PATCH] arm/arm64: KVM: Add support for ARMv8 AArch32 execution state
From: Christoffer Dall @ 2016-09-23 9:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474614805-18629-1-git-send-email-b18965@freescale.com>
On Fri, Sep 23, 2016 at 03:13:25PM +0800, Alison Wang wrote:
> The ARMv8 architecture supports two execution state, AArch64 and
> AArch32. To support KVM in AArch32 execution state for ARMv8, Cortex-A53
> and Cortex-A72 need to be added for target-specific checks.
>
> Signed-off-by: Alison Wang <alison.wang@nxp.com>
> ---
> arch/arm/include/asm/cputype.h | 2 ++
> arch/arm/kvm/guest.c | 2 ++
> 2 files changed, 4 insertions(+)
>
> diff --git a/arch/arm/include/asm/cputype.h b/arch/arm/include/asm/cputype.h
> index 754f86f..4f8c632 100644
> --- a/arch/arm/include/asm/cputype.h
> +++ b/arch/arm/include/asm/cputype.h
> @@ -75,6 +75,8 @@
> #define ARM_CPU_PART_CORTEX_A12 0x4100c0d0
> #define ARM_CPU_PART_CORTEX_A17 0x4100c0e0
> #define ARM_CPU_PART_CORTEX_A15 0x4100c0f0
> +#define ARM_CPU_PART_CORTEX_A53_AARCH32 0x4100d030
> +#define ARM_CPU_PART_CORTEX_A72_AARCH32 0x4100d080
> #define ARM_CPU_PART_MASK 0xff00fff0
>
> /* DEC implemented cores */
> diff --git a/arch/arm/kvm/guest.c b/arch/arm/kvm/guest.c
> index 9aca920..462a099 100644
> --- a/arch/arm/kvm/guest.c
> +++ b/arch/arm/kvm/guest.c
> @@ -252,6 +252,8 @@ int __attribute_const__ kvm_target_cpu(void)
> {
> switch (read_cpuid_part()) {
> case ARM_CPU_PART_CORTEX_A7:
> + case ARM_CPU_PART_CORTEX_A53_AARCH32:
> + case ARM_CPU_PART_CORTEX_A72_AARCH32:
huh? why are we mapping A53 and A72 cores to an A7 core?
-Christoffer
^ permalink raw reply
* [PATCH v2 0/3] arm64: kgdb: fix single stepping
From: AKASHI Takahiro @ 2016-09-23 9:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160923084341.GA32342@kroah.com>
On Fri, Sep 23, 2016 at 10:43:41AM +0200, Greg KH wrote:
> On Fri, Sep 23, 2016 at 05:32:58PM +0900, AKASHI Takahiro wrote:
> > On Fri, Sep 23, 2016 at 10:16:18AM +0200, Greg KH wrote:
> > > On Fri, Sep 23, 2016 at 04:33:24PM +0900, AKASHI Takahiro wrote:
> > > > Kgdb support on arm64 was merged in v3.15, but from its first appearance,
> > > > "signle step" has never worked well.
> > > >
> > > > This patch fixes all the error cases I found so far.
> > > > The original patch[1] was splitted into three pieces, ones for each case.
> > > > patch#1, #2 should be applied to all the version, v3.15 and later.
> > > > pathc#3 only for v3.16 and later.
> > >
> > > As this is not a regression (i.e. it has never worked), why is this
> > > something for stable releases?
> >
> > Because, I think, that is a bug.
> > The author seems to have believed that it worked.
> > Please see:
> > commit 44679a4f
> > Author: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> > Date: Tue Jan 28 11:20:19 2014 +0000
> >
> > arm64: KGDB: Add step debugging support
>
> Yes, but again, it didn't work, so this would be a new feature.
Yes, but again, it's a bug of kgdb on arm64 which is supposed
to have been available since v3.15.
> One
> that obviously people aren't using in the stable kernels, otherwise they
> would have noticed in the past 2 years about it being broken :)
Totally agree. That is also why I've left this patch untouched
for a long time. But recently a guy asked me about kgdb, saying that
single step didn't work on his platform. He re-discovered this bug.
He doesn't use stable kernels, but Linaro's LSK (v4.4), though :)
> Please read Documentation/stable_kernel_rules.txt for the requirements
> of a stable kernel patch. I don't think this series meets those rules.
Please re-think about merging this patch.
Thanks,
-Takahiro AKASHI
> thanks,
>
> greg k-h
^ permalink raw reply
* [PATCH v14 4/4] CMDQ: save more energy in idle
From: Horng-Shyang Liao @ 2016-09-23 9:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CABb+yY1YqyReK5bii6n5a_rJ6fgpPi=aC3sfKJdomvM5+NG9FA@mail.gmail.com>
On Thu, 2016-09-22 at 13:22 +0530, Jassi Brar wrote:
> On Mon, Sep 5, 2016 at 7:14 AM, HS Liao <hs.liao@mediatek.com> wrote:
> > Use clk_disable_unprepare instead of clk_disable to save more energy
> > when CMDQ is idle.
> >
> > Signed-off-by: HS Liao <hs.liao@mediatek.com>
> > ---
> > drivers/mailbox/mtk-cmdq.c | 54 +++++++++++++++++++++++++++++++++++++++-------
>
> The driver is introduced by second patch of the set, so it makes sense
> to merge this patch into patch 2/4.
Hi Jassi,
Could you take a look at previous discussion between Matthias and me?
http://lkml.iu.edu/hypermail/linux/kernel/1606.2/05239.html
His basic idea is to simplify first working version.
Therefore, I move some code to this patch.
Thanks,
HS
^ permalink raw reply
* [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver
From: Horng-Shyang Liao @ 2016-09-23 9:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CABb+yY1U8WeSWTaeiPJDO1WCBm2AH2QGAiyBextga-KFHwspig@mail.gmail.com>
Hi Jassi,
Please see my inline reply.
On Thu, 2016-09-22 at 13:47 +0530, Jassi Brar wrote:
> On Mon, Sep 5, 2016 at 7:14 AM, HS Liao <hs.liao@mediatek.com> wrote:
[...]
> > +struct cmdq_base *cmdq_register_device(struct device *dev)
> > +{
> > + struct cmdq_base *cmdq_base;
> > + struct resource res;
> > + int subsys;
> > + u32 base;
> > +
> > + if (of_address_to_resource(dev->of_node, 0, &res))
> > + return NULL;
> > + base = (u32)res.start;
> > +
> > + subsys = cmdq_subsys_base_to_id(base >> 16);
> > + if (subsys < 0)
> > + return NULL;
> > +
> > + cmdq_base = devm_kmalloc(dev, sizeof(*cmdq_base), GFP_KERNEL);
> > + if (!cmdq_base)
> > + return NULL;
> > + cmdq_base->subsys = subsys;
> > + cmdq_base->base = base;
> > +
> > + return cmdq_base;
> > +}
> > +EXPORT_SYMBOL(cmdq_register_device);
> > +
> > +struct cmdq_client *cmdq_mbox_create(struct device *dev, int index)
> > +{
> > + struct cmdq_client *client;
> > +
> > + client = kzalloc(sizeof(*client), GFP_KERNEL);
> > + client->client.dev = dev;
> > + client->client.tx_block = false;
> > + client->chan = mbox_request_channel(&client->client, index);
> > + return client;
> > +}
> > +EXPORT_SYMBOL(cmdq_mbox_create);
> > +
> > +int cmdq_task_create(struct device *dev, struct cmdq_task **task_ptr)
> > +{
> > + struct cmdq_task *task;
> > + int err;
> > +
> > + task = kzalloc(sizeof(*task), GFP_KERNEL);
> > + if (!task)
> > + return -ENOMEM;
> > + task->cmdq = dev_get_drvdata(dev);
> > + err = cmdq_task_realloc_cmd_buffer(task, PAGE_SIZE);
> > + if (err < 0) {
> > + kfree(task);
> > + return err;
> > + }
> > + *task_ptr = task;
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(cmdq_task_create);
> > +
> > +static int cmdq_task_append_command(struct cmdq_task *task, enum cmdq_code code,
> > + u32 arg_a, u32 arg_b)
> > +{
> > + u64 *cmd_ptr;
> > + int err;
> > +
> > + if (WARN_ON(task->finalized))
> > + return -EBUSY;
> > + if (unlikely(task->cmd_buf_size + CMDQ_INST_SIZE > task->buf_size)) {
> > + err = cmdq_task_realloc_cmd_buffer(task, task->buf_size * 2);
> > + if (err < 0)
> > + return err;
> > + }
> > + cmd_ptr = task->va_base + task->cmd_buf_size;
> > + (*cmd_ptr) = (u64)((code << CMDQ_OP_CODE_SHIFT) | arg_a) << 32 | arg_b;
> > + task->cmd_buf_size += CMDQ_INST_SIZE;
> > + return 0;
> > +}
> > +
> > +int cmdq_task_write(struct cmdq_task *task, u32 value, struct cmdq_base *base,
> > + u32 offset)
> > +{
> > + u32 arg_a = ((base->base + offset) & CMDQ_ARG_A_WRITE_MASK) |
> > + (base->subsys << CMDQ_SUBSYS_SHIFT);
> > + return cmdq_task_append_command(task, CMDQ_CODE_WRITE, arg_a, value);
> > +}
> > +EXPORT_SYMBOL(cmdq_task_write);
> > +
> > +int cmdq_task_write_mask(struct cmdq_task *task, u32 value,
> > + struct cmdq_base *base, u32 offset, u32 mask)
> > +{
> > + u32 offset_mask = offset;
> > + int err;
> > +
> > + if (mask != 0xffffffff) {
> > + err = cmdq_task_append_command(task, CMDQ_CODE_MASK, 0, ~mask);
> > + if (err < 0)
> > + return err;
> > + offset_mask |= CMDQ_WRITE_ENABLE_MASK;
> > + }
> > + return cmdq_task_write(task, value, base, offset_mask);
> > +}
> > +EXPORT_SYMBOL(cmdq_task_write_mask);
> > +
> > +static const u32 cmdq_event_value[CMDQ_MAX_EVENT] = {
> > + /* Display start of frame(SOF) events */
> > + [CMDQ_EVENT_DISP_OVL0_SOF] = 11,
> > + [CMDQ_EVENT_DISP_OVL1_SOF] = 12,
> > + [CMDQ_EVENT_DISP_RDMA0_SOF] = 13,
> > + [CMDQ_EVENT_DISP_RDMA1_SOF] = 14,
> > + [CMDQ_EVENT_DISP_RDMA2_SOF] = 15,
> > + [CMDQ_EVENT_DISP_WDMA0_SOF] = 16,
> > + [CMDQ_EVENT_DISP_WDMA1_SOF] = 17,
> > + /* Display end of frame(EOF) events */
> > + [CMDQ_EVENT_DISP_OVL0_EOF] = 39,
> > + [CMDQ_EVENT_DISP_OVL1_EOF] = 40,
> > + [CMDQ_EVENT_DISP_RDMA0_EOF] = 41,
> > + [CMDQ_EVENT_DISP_RDMA1_EOF] = 42,
> > + [CMDQ_EVENT_DISP_RDMA2_EOF] = 43,
> > + [CMDQ_EVENT_DISP_WDMA0_EOF] = 44,
> > + [CMDQ_EVENT_DISP_WDMA1_EOF] = 45,
> > + /* Mutex end of frame(EOF) events */
> > + [CMDQ_EVENT_MUTEX0_STREAM_EOF] = 53,
> > + [CMDQ_EVENT_MUTEX1_STREAM_EOF] = 54,
> > + [CMDQ_EVENT_MUTEX2_STREAM_EOF] = 55,
> > + [CMDQ_EVENT_MUTEX3_STREAM_EOF] = 56,
> > + [CMDQ_EVENT_MUTEX4_STREAM_EOF] = 57,
> > + /* Display underrun events */
> > + [CMDQ_EVENT_DISP_RDMA0_UNDERRUN] = 63,
> > + [CMDQ_EVENT_DISP_RDMA1_UNDERRUN] = 64,
> > + [CMDQ_EVENT_DISP_RDMA2_UNDERRUN] = 65,
> > +};
> > +
> > +int cmdq_task_wfe(struct cmdq_task *task, enum cmdq_event event)
> > +{
> > + u32 arg_b;
> > +
> > + if (event >= CMDQ_MAX_EVENT || event < 0)
> > + return -EINVAL;
> > +
> > + /*
> > + * WFE arg_b
> > + * bit 0-11: wait value
> > + * bit 15: 1 - wait, 0 - no wait
> > + * bit 16-27: update value
> > + * bit 31: 1 - update, 0 - no update
> > + */
> > + arg_b = CMDQ_WFE_UPDATE | CMDQ_WFE_WAIT | CMDQ_WFE_WAIT_VALUE;
> > + return cmdq_task_append_command(task, CMDQ_CODE_WFE,
> > + cmdq_event_value[event], arg_b);
> > +}
> > +EXPORT_SYMBOL(cmdq_task_wfe);
> > +
> > +int cmdq_task_clear_event(struct cmdq_task *task, enum cmdq_event event)
> > +{
> > + if (event >= CMDQ_MAX_EVENT || event < 0)
> > + return -EINVAL;
> > +
> > + return cmdq_task_append_command(task, CMDQ_CODE_WFE,
> > + cmdq_event_value[event], CMDQ_WFE_UPDATE);
> > +}
> > +EXPORT_SYMBOL(cmdq_task_clear_event);
> > +
> > +static int cmdq_task_finalize(struct cmdq_task *task)
> > +{
> > + int err;
> > +
> > + if (task->finalized)
> > + return 0;
> > +
> > + /* insert EOC and generate IRQ for each command iteration */
> > + err = cmdq_task_append_command(task, CMDQ_CODE_EOC, 0, CMDQ_EOC_IRQ_EN);
> > + if (err < 0)
> > + return err;
> > +
> > + /* JUMP to end */
> > + err = cmdq_task_append_command(task, CMDQ_CODE_JUMP, 0, CMDQ_JUMP_PASS);
> > + if (err < 0)
> > + return err;
> > +
> > + task->finalized = true;
> > + return 0;
> > +}
> > +
> > +int cmdq_task_flush_async(struct cmdq_client *client, struct cmdq_task *task,
> > + cmdq_async_flush_cb cb, void *data)
> > +{
> > + struct cmdq *cmdq = task->cmdq;
> > + int err;
> > +
> > + mutex_lock(&cmdq->task_mutex);
> > + if (cmdq->suspended) {
> > + dev_err(cmdq->mbox.dev, "%s is called after suspended\n",
> > + __func__);
> > + mutex_unlock(&cmdq->task_mutex);
> > + return -EPERM;
> > + }
> > +
> > + err = cmdq_task_finalize(task);
> > + if (err < 0) {
> > + mutex_unlock(&cmdq->task_mutex);
> > + return err;
> > + }
> > +
> > + INIT_LIST_HEAD(&task->list_entry);
> > + task->cb.cb = cb;
> > + task->cb.data = data;
> > + task->pa_base = dma_map_single(cmdq->mbox.dev, task->va_base,
> > + task->cmd_buf_size, DMA_TO_DEVICE);
> > +
> > + mbox_send_message(client->chan, task);
> > + /* We can send next task immediately, so just call txdone. */
> > + mbox_client_txdone(client->chan, 0);
> > + mutex_unlock(&cmdq->task_mutex);
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(cmdq_task_flush_async);
> > +
> > +struct cmdq_flush_completion {
> > + struct completion cmplt;
> > + bool err;
> > +};
> > +
> > +static void cmdq_task_flush_cb(struct cmdq_cb_data data)
> > +{
> > + struct cmdq_flush_completion *cmplt = data.data;
> > +
> > + cmplt->err = data.err;
> > + complete(&cmplt->cmplt);
> > +}
> > +
> > +int cmdq_task_flush(struct cmdq_client *client, struct cmdq_task *task)
> > +{
> > + struct cmdq_flush_completion cmplt;
> > + int err;
> > +
> > + init_completion(&cmplt.cmplt);
> > + err = cmdq_task_flush_async(client, task, cmdq_task_flush_cb, &cmplt);
> > + if (err < 0)
> > + return err;
> > + wait_for_completion(&cmplt.cmplt);
> > + return cmplt.err ? -EFAULT : 0;
> > +}
> > +EXPORT_SYMBOL(cmdq_task_flush);
> > +
> > +void cmdq_mbox_free(struct cmdq_client *client)
> > +{
> > + mbox_free_channel(client->chan);
> > + kfree(client);
> > +}
> > +EXPORT_SYMBOL(cmdq_mbox_free);
> > +
> All these exported functions implement the protocol, so should not be
> a part of this controller driver. That should go into
> drivers/soc/mediatek/
>
> The controller driver (mtk-cmdq.c) should implement mainly the
> mbox_chan_ops and mbox.of_xlate.
>
I can do that, but I would like to confirm with Matthias in advance.
[...]
> > + cmdq->irq = irq_of_parse_and_map(node, 0);
> >
> why not, cmdq->irq = platform_get_irq(pdev, 0);
Will do
[...]
> > +static struct platform_driver cmdq_drv = {
> > + .probe = cmdq_probe,
> > + .remove = cmdq_remove,
> > + .driver = {
> > + .name = "mtk_cmdq",
> > + .owner = THIS_MODULE,
> >
> please remove the unnecessary .owner field.
Will do
> > + .pm = &cmdq_pm_ops,
> > + .of_match_table = cmdq_of_ids,
> > + }
> > +};
> > +
> > +builtin_platform_driver(cmdq_drv);
> > diff --git a/include/linux/mailbox/mtk-cmdq.h b/include/linux/mailbox/mtk-cmdq.h
> > new file mode 100644
> > index 0000000..c3c924d
> > --- /dev/null
> > +++ b/include/linux/mailbox/mtk-cmdq.h
> >
> The api implemented is Mediateck proprietary, so I think it should be
> include/linux/soc/mediatek/cmdq.h
>
>
> > @@ -0,0 +1,180 @@
> > +/*
> > + * Copyright (c) 2015 MediaTek Inc.
> > + *
> > + * 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.
> > + */
> > +
> > +#ifndef __MTK_CMDQ_H__
> > +#define __MTK_CMDQ_H__
> > +
> > +#include <linux/mailbox_client.h>
> > +#include <linux/mailbox_controller.h>
> >
> Clients should not need to include mailbox_controller.h
This is because client needs to know controller's dev.
Please see my CMDQ v13.
http://www.spinics.net/lists/kernel/msg2327867.html
I add mailbox_controller.h for client to get controller's dev,
so I can remove a node reference in device tree.
Should I revert the modification of CMDQ v13?
> > +#include <linux/platform_device.h>
> > +#include <linux/types.h>
> > +
> > +/* display events in command queue(CMDQ) */
> > +enum cmdq_event {
> > + /* Display start of frame(SOF) events */
> > + CMDQ_EVENT_DISP_OVL0_SOF,
> >
> you may want to explicitly initialise the first element.
Will do
> > + CMDQ_EVENT_DISP_OVL1_SOF,
> > + CMDQ_EVENT_DISP_RDMA0_SOF,
> > + CMDQ_EVENT_DISP_RDMA1_SOF,
> > + CMDQ_EVENT_DISP_RDMA2_SOF,
> > + CMDQ_EVENT_DISP_WDMA0_SOF,
> > + CMDQ_EVENT_DISP_WDMA1_SOF,
> > + /* Display end of frame(EOF) events */
> > + CMDQ_EVENT_DISP_OVL0_EOF,
> > + CMDQ_EVENT_DISP_OVL1_EOF,
> > + CMDQ_EVENT_DISP_RDMA0_EOF,
> > + CMDQ_EVENT_DISP_RDMA1_EOF,
> > + CMDQ_EVENT_DISP_RDMA2_EOF,
> > + CMDQ_EVENT_DISP_WDMA0_EOF,
> > + CMDQ_EVENT_DISP_WDMA1_EOF,
> > + /* Mutex end of frame(EOF) events */
> > + CMDQ_EVENT_MUTEX0_STREAM_EOF,
> > + CMDQ_EVENT_MUTEX1_STREAM_EOF,
> > + CMDQ_EVENT_MUTEX2_STREAM_EOF,
> > + CMDQ_EVENT_MUTEX3_STREAM_EOF,
> > + CMDQ_EVENT_MUTEX4_STREAM_EOF,
> > + /* Display underrun events */
> > + CMDQ_EVENT_DISP_RDMA0_UNDERRUN,
> > + CMDQ_EVENT_DISP_RDMA1_UNDERRUN,
> > + CMDQ_EVENT_DISP_RDMA2_UNDERRUN,
> > + /* Keep this at the end */
> > + CMDQ_MAX_EVENT,
> > +};
> > +
Thanks,
HS
Hi Matthias,
Do you agree with Jassi's comments about moving parts of code back to
soc/mediatek/ ?
If I do it, the part in soc/mediatek/ will be similar to a library.
Could you tell me a good way to handle this situation?
Thanks,
HS
^ permalink raw reply
* [PATCH] arm/arm64: KVM: Add support for ARMv8 AArch32 execution state
From: Marc Zyngier @ 2016-09-23 9:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160923092550.GG9101@cbox>
On 23/09/16 10:25, Christoffer Dall wrote:
> On Fri, Sep 23, 2016 at 03:13:25PM +0800, Alison Wang wrote:
>> The ARMv8 architecture supports two execution state, AArch64 and
>> AArch32. To support KVM in AArch32 execution state for ARMv8, Cortex-A53
>> and Cortex-A72 need to be added for target-specific checks.
>>
>> Signed-off-by: Alison Wang <alison.wang@nxp.com>
>> ---
>> arch/arm/include/asm/cputype.h | 2 ++
>> arch/arm/kvm/guest.c | 2 ++
>> 2 files changed, 4 insertions(+)
>>
>> diff --git a/arch/arm/include/asm/cputype.h b/arch/arm/include/asm/cputype.h
>> index 754f86f..4f8c632 100644
>> --- a/arch/arm/include/asm/cputype.h
>> +++ b/arch/arm/include/asm/cputype.h
>> @@ -75,6 +75,8 @@
>> #define ARM_CPU_PART_CORTEX_A12 0x4100c0d0
>> #define ARM_CPU_PART_CORTEX_A17 0x4100c0e0
>> #define ARM_CPU_PART_CORTEX_A15 0x4100c0f0
>> +#define ARM_CPU_PART_CORTEX_A53_AARCH32 0x4100d030
>> +#define ARM_CPU_PART_CORTEX_A72_AARCH32 0x4100d080
>> #define ARM_CPU_PART_MASK 0xff00fff0
>>
>> /* DEC implemented cores */
>> diff --git a/arch/arm/kvm/guest.c b/arch/arm/kvm/guest.c
>> index 9aca920..462a099 100644
>> --- a/arch/arm/kvm/guest.c
>> +++ b/arch/arm/kvm/guest.c
>> @@ -252,6 +252,8 @@ int __attribute_const__ kvm_target_cpu(void)
>> {
>> switch (read_cpuid_part()) {
>> case ARM_CPU_PART_CORTEX_A7:
>> + case ARM_CPU_PART_CORTEX_A53_AARCH32:
>> + case ARM_CPU_PART_CORTEX_A72_AARCH32:
>
> huh? why are we mapping A53 and A72 cores to an A7 core?
Yeah, that's absolutely disgusting. The only sensible thing to do would
be to implement something "generic", just like we have on the 64bit side.
As it stands, this patch falls into the "No way" bucket.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox