* Re: Summary of LPC guest MSI discussion in Santa Fe
From: Don Dutile @ 2016-11-08 19:02 UTC (permalink / raw)
To: Will Deacon, Auger Eric
Cc: drjones-H+wXaHxf7aLQT0dZR+AlfA,
christoffer.dall-QSEj5FYQhm4dnm+yROfE0A,
jason-NLaQJdtUoK4Be96aLqz0jA, kvm-u79uwXL29TY76Z2rM5mHXA,
marc.zyngier-5wv7dgnIgG8, benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r,
punit.agrawal-5wv7dgnIgG8, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
pranav.sawargaonkar-Re5JQEeQqe8AvxtiuMwx3w, arnd-r2nGTMty4D4,
dwmw-vV1OtcyAfmbQXOPxS62xeg, jcm-H+wXaHxf7aLQT0dZR+AlfA,
tglx-hfZtesqFncYOwBW4kG4KsQ,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
eric.auger.pro-Re5JQEeQqe8AvxtiuMwx3w
In-Reply-To: <20161108175457.GK20591-5wv7dgnIgG8@public.gmane.org>
On 11/08/2016 12:54 PM, Will Deacon wrote:
> On Tue, Nov 08, 2016 at 03:27:23PM +0100, Auger Eric wrote:
>> On 08/11/2016 03:45, Will Deacon wrote:
>>> Rather than treat these as separate problems, a better interface is to
>>> tell userspace about a set of reserved regions, and have this include
>>> the MSI doorbell, irrespective of whether or not it can be remapped.
>>> Don suggested that we statically pick an address for the doorbell in a
>>> similar way to x86, and have the kernel map it there. We could even pick
>>> 0xfee00000. If it conflicts with a reserved region on the platform (due
>>> to (4)), then we'd obviously have to (deterministically?) allocate it
>>> somewhere else, but probably within the bottom 4G.
>> This is tentatively achieved now with
>> [1] [RFC v2 0/8] KVM PCIe/MSI passthrough on ARM/ARM64 - Alt II
>> (http://www.mail-archive.com/linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg1264506.html)
> Yup, I saw that fly by. Hopefully some of the internals can be reused
> with the current thinking on user ABI.
>
>>> The next question is how to tell userspace about all of the reserved
>>> regions. Initially, the idea was to extend VFIO, however Alex pointed
>>> out a horrible scenario:
>>>
>>> 1. QEMU spawns a VM on system 0
>>> 2. VM is migrated to system 1
>>> 3. QEMU attempts to passthrough a device using PCI hotplug
>>>
>>> In this scenario, the guest memory map is chosen at step (1), yet there
>>> is no VFIO fd available to determine the reserved regions. Furthermore,
>>> the reserved regions may vary between system 0 and system 1. This pretty
>>> much rules out using VFIO to determine the reserved regions.Alex suggested
>>> that the SMMU driver can advertise the regions via /sys/class/iommu/. This
>>> would solve part of the problem, but migration between systems with
>>> different memory maps can still cause problems if the reserved regions
>>> of the new system conflict with the guest memory map chosen by QEMU.
>>
>> OK so I understand we do not want anymore the VFIO chain capability API
>> (patch 5 of above series) but we prefer a sysfs approach instead.
> Right.
>
>> I understand the sysfs approach which allows the userspace to get the
>> info earlier and independently on VFIO. Keeping in mind current QEMU
>> virt - which is not the only userspace - will not do much from this info
>> until we bring upheavals in virt address space management. So if I am
>> not wrong, at the moment the main action to be undertaken is the
>> rejection of the PCI hotplug in case we detect a collision.
> I don't think so; it should be up to userspace to reject the hotplug.
> If userspace doesn't have support for the regions, then that's fine --
> you just end up in a situation where the CPU page table maps memory
> somewhere that the device can't see. In other words, you'll end up with
> spurious DMA failures, but that's exactly what happens with current systems
> if you passthrough an overlapping region (Robin demonstrated this on Juno).
>
> Additionally, you can imagine some future support where you can tell the
> guest not to use certain regions of its memory for DMA. In this case, you
> wouldn't want to refuse the hotplug in the case of overlapping regions.
>
> Really, I think the kernel side just needs to enumerate the fixed reserved
> regions, place the doorbell at a fixed address and then advertise these
> via sysfs.
>
>> I can respin [1]
>> - studying and taking into account Robin's comments about dm_regions
>> similarities
>> - removing the VFIO capability chain and replacing this by a sysfs API
> Ideally, this would be reusable between different SMMU drivers so the sysfs
> entries have the same format etc.
>
>> Would that be OK?
> Sounds good to me. Are you in a position to prototype something on the qemu
> side once we've got kernel-side agreement?
>
>> What about Alex comments who wanted to report the usable memory ranges
>> instead of unusable memory ranges?
>>
>> Also did you have a chance to discuss the following items:
>> 1) the VFIO irq safety assessment
> The discussion really focussed on system topology, as opposed to properties
> of the doorbell. Regardless of how the device talks to the doorbell, if
> the doorbell can't protect against things like MSI spoofing, then it's
> unsafe. My opinion is that we shouldn't allow passthrough by default on
> systems with unsafe doorbells (we could piggyback on allow_unsafe_interrupts
> cmdline option to VFIO).
>
> A first step would be making all this opt-in, and only supporting GICv3
> ITS for now.
You're trying to support a config that is < GICv3 and no ITS ? ...
That would be the equiv. of x86 pre-intr-remap, and that's why allow_unsafe_interrupts
hook was created ... to enable devel/kick-the-tires.
>> 2) the MSI reserved size computation (is an arbitrary size OK?)
> If we fix the base address, we could fix a size too. However, we'd still
> need to enumerate the doorbells to check that they fit in the region we
> have. If not, then we can warn during boot and treat it the same way as
> a resource conflict (that is, reallocate the region in some deterministic
> way).
>
> Will
^ permalink raw reply
* Summary of LPC guest MSI discussion in Santa Fe
From: Don Dutile @ 2016-11-08 19:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161108175457.GK20591@arm.com>
On 11/08/2016 12:54 PM, Will Deacon wrote:
> On Tue, Nov 08, 2016 at 03:27:23PM +0100, Auger Eric wrote:
>> On 08/11/2016 03:45, Will Deacon wrote:
>>> Rather than treat these as separate problems, a better interface is to
>>> tell userspace about a set of reserved regions, and have this include
>>> the MSI doorbell, irrespective of whether or not it can be remapped.
>>> Don suggested that we statically pick an address for the doorbell in a
>>> similar way to x86, and have the kernel map it there. We could even pick
>>> 0xfee00000. If it conflicts with a reserved region on the platform (due
>>> to (4)), then we'd obviously have to (deterministically?) allocate it
>>> somewhere else, but probably within the bottom 4G.
>> This is tentatively achieved now with
>> [1] [RFC v2 0/8] KVM PCIe/MSI passthrough on ARM/ARM64 - Alt II
>> (http://www.mail-archive.com/linux-kernel at vger.kernel.org/msg1264506.html)
> Yup, I saw that fly by. Hopefully some of the internals can be reused
> with the current thinking on user ABI.
>
>>> The next question is how to tell userspace about all of the reserved
>>> regions. Initially, the idea was to extend VFIO, however Alex pointed
>>> out a horrible scenario:
>>>
>>> 1. QEMU spawns a VM on system 0
>>> 2. VM is migrated to system 1
>>> 3. QEMU attempts to passthrough a device using PCI hotplug
>>>
>>> In this scenario, the guest memory map is chosen at step (1), yet there
>>> is no VFIO fd available to determine the reserved regions. Furthermore,
>>> the reserved regions may vary between system 0 and system 1. This pretty
>>> much rules out using VFIO to determine the reserved regions.Alex suggested
>>> that the SMMU driver can advertise the regions via /sys/class/iommu/. This
>>> would solve part of the problem, but migration between systems with
>>> different memory maps can still cause problems if the reserved regions
>>> of the new system conflict with the guest memory map chosen by QEMU.
>>
>> OK so I understand we do not want anymore the VFIO chain capability API
>> (patch 5 of above series) but we prefer a sysfs approach instead.
> Right.
>
>> I understand the sysfs approach which allows the userspace to get the
>> info earlier and independently on VFIO. Keeping in mind current QEMU
>> virt - which is not the only userspace - will not do much from this info
>> until we bring upheavals in virt address space management. So if I am
>> not wrong, at the moment the main action to be undertaken is the
>> rejection of the PCI hotplug in case we detect a collision.
> I don't think so; it should be up to userspace to reject the hotplug.
> If userspace doesn't have support for the regions, then that's fine --
> you just end up in a situation where the CPU page table maps memory
> somewhere that the device can't see. In other words, you'll end up with
> spurious DMA failures, but that's exactly what happens with current systems
> if you passthrough an overlapping region (Robin demonstrated this on Juno).
>
> Additionally, you can imagine some future support where you can tell the
> guest not to use certain regions of its memory for DMA. In this case, you
> wouldn't want to refuse the hotplug in the case of overlapping regions.
>
> Really, I think the kernel side just needs to enumerate the fixed reserved
> regions, place the doorbell at a fixed address and then advertise these
> via sysfs.
>
>> I can respin [1]
>> - studying and taking into account Robin's comments about dm_regions
>> similarities
>> - removing the VFIO capability chain and replacing this by a sysfs API
> Ideally, this would be reusable between different SMMU drivers so the sysfs
> entries have the same format etc.
>
>> Would that be OK?
> Sounds good to me. Are you in a position to prototype something on the qemu
> side once we've got kernel-side agreement?
>
>> What about Alex comments who wanted to report the usable memory ranges
>> instead of unusable memory ranges?
>>
>> Also did you have a chance to discuss the following items:
>> 1) the VFIO irq safety assessment
> The discussion really focussed on system topology, as opposed to properties
> of the doorbell. Regardless of how the device talks to the doorbell, if
> the doorbell can't protect against things like MSI spoofing, then it's
> unsafe. My opinion is that we shouldn't allow passthrough by default on
> systems with unsafe doorbells (we could piggyback on allow_unsafe_interrupts
> cmdline option to VFIO).
>
> A first step would be making all this opt-in, and only supporting GICv3
> ITS for now.
You're trying to support a config that is < GICv3 and no ITS ? ...
That would be the equiv. of x86 pre-intr-remap, and that's why allow_unsafe_interrupts
hook was created ... to enable devel/kick-the-tires.
>> 2) the MSI reserved size computation (is an arbitrary size OK?)
> If we fix the base address, we could fix a size too. However, we'd still
> need to enumerate the doorbells to check that they fit in the region we
> have. If not, then we can warn during boot and treat it the same way as
> a resource conflict (that is, reallocate the region in some deterministic
> way).
>
> Will
^ permalink raw reply
* Re: Summary of LPC guest MSI discussion in Santa Fe
From: Don Dutile @ 2016-11-08 19:02 UTC (permalink / raw)
To: Will Deacon, Auger Eric
Cc: Alex Williamson, drjones, jason, kvm, marc.zyngier, benh, joro,
punit.agrawal, linux-kernel, arnd, diana.craciun, iommu,
pranav.sawargaonkar, linux-arm-kernel, jcm, tglx, robin.murphy,
dwmw, christoffer.dall, eric.auger.pro
In-Reply-To: <20161108175457.GK20591@arm.com>
On 11/08/2016 12:54 PM, Will Deacon wrote:
> On Tue, Nov 08, 2016 at 03:27:23PM +0100, Auger Eric wrote:
>> On 08/11/2016 03:45, Will Deacon wrote:
>>> Rather than treat these as separate problems, a better interface is to
>>> tell userspace about a set of reserved regions, and have this include
>>> the MSI doorbell, irrespective of whether or not it can be remapped.
>>> Don suggested that we statically pick an address for the doorbell in a
>>> similar way to x86, and have the kernel map it there. We could even pick
>>> 0xfee00000. If it conflicts with a reserved region on the platform (due
>>> to (4)), then we'd obviously have to (deterministically?) allocate it
>>> somewhere else, but probably within the bottom 4G.
>> This is tentatively achieved now with
>> [1] [RFC v2 0/8] KVM PCIe/MSI passthrough on ARM/ARM64 - Alt II
>> (http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1264506.html)
> Yup, I saw that fly by. Hopefully some of the internals can be reused
> with the current thinking on user ABI.
>
>>> The next question is how to tell userspace about all of the reserved
>>> regions. Initially, the idea was to extend VFIO, however Alex pointed
>>> out a horrible scenario:
>>>
>>> 1. QEMU spawns a VM on system 0
>>> 2. VM is migrated to system 1
>>> 3. QEMU attempts to passthrough a device using PCI hotplug
>>>
>>> In this scenario, the guest memory map is chosen at step (1), yet there
>>> is no VFIO fd available to determine the reserved regions. Furthermore,
>>> the reserved regions may vary between system 0 and system 1. This pretty
>>> much rules out using VFIO to determine the reserved regions.Alex suggested
>>> that the SMMU driver can advertise the regions via /sys/class/iommu/. This
>>> would solve part of the problem, but migration between systems with
>>> different memory maps can still cause problems if the reserved regions
>>> of the new system conflict with the guest memory map chosen by QEMU.
>>
>> OK so I understand we do not want anymore the VFIO chain capability API
>> (patch 5 of above series) but we prefer a sysfs approach instead.
> Right.
>
>> I understand the sysfs approach which allows the userspace to get the
>> info earlier and independently on VFIO. Keeping in mind current QEMU
>> virt - which is not the only userspace - will not do much from this info
>> until we bring upheavals in virt address space management. So if I am
>> not wrong, at the moment the main action to be undertaken is the
>> rejection of the PCI hotplug in case we detect a collision.
> I don't think so; it should be up to userspace to reject the hotplug.
> If userspace doesn't have support for the regions, then that's fine --
> you just end up in a situation where the CPU page table maps memory
> somewhere that the device can't see. In other words, you'll end up with
> spurious DMA failures, but that's exactly what happens with current systems
> if you passthrough an overlapping region (Robin demonstrated this on Juno).
>
> Additionally, you can imagine some future support where you can tell the
> guest not to use certain regions of its memory for DMA. In this case, you
> wouldn't want to refuse the hotplug in the case of overlapping regions.
>
> Really, I think the kernel side just needs to enumerate the fixed reserved
> regions, place the doorbell at a fixed address and then advertise these
> via sysfs.
>
>> I can respin [1]
>> - studying and taking into account Robin's comments about dm_regions
>> similarities
>> - removing the VFIO capability chain and replacing this by a sysfs API
> Ideally, this would be reusable between different SMMU drivers so the sysfs
> entries have the same format etc.
>
>> Would that be OK?
> Sounds good to me. Are you in a position to prototype something on the qemu
> side once we've got kernel-side agreement?
>
>> What about Alex comments who wanted to report the usable memory ranges
>> instead of unusable memory ranges?
>>
>> Also did you have a chance to discuss the following items:
>> 1) the VFIO irq safety assessment
> The discussion really focussed on system topology, as opposed to properties
> of the doorbell. Regardless of how the device talks to the doorbell, if
> the doorbell can't protect against things like MSI spoofing, then it's
> unsafe. My opinion is that we shouldn't allow passthrough by default on
> systems with unsafe doorbells (we could piggyback on allow_unsafe_interrupts
> cmdline option to VFIO).
>
> A first step would be making all this opt-in, and only supporting GICv3
> ITS for now.
You're trying to support a config that is < GICv3 and no ITS ? ...
That would be the equiv. of x86 pre-intr-remap, and that's why allow_unsafe_interrupts
hook was created ... to enable devel/kick-the-tires.
>> 2) the MSI reserved size computation (is an arbitrary size OK?)
> If we fix the base address, we could fix a size too. However, we'd still
> need to enumerate the doorbells to check that they fit in the region we
> have. If not, then we can warn during boot and treat it the same way as
> a resource conflict (that is, reallocate the region in some deterministic
> way).
>
> Will
^ permalink raw reply
* KASAN & the vmalloc area
From: Mark Rutland @ 2016-11-08 19:03 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
I see a while back [1] there was a discussion of what to do about KASAN
and vmapped stacks, but it doesn't look like that was solved, judging by
the vmapped stacks pull [2] for v4.9.
I wondered whether anyone had looked at that since?
I have an additional reason to want to dynamically allocate the vmalloc
area shadow: it turns out that KASAN currently interacts rather poorly
with the arm64 ptdump code.
When KASAN is selected, we allocate shadow for the whole vmalloc area,
using common zero pte, pmd, pud tables. Walking over these in the ptdump
code takes a *very* long time (I've seen up to 15 minutes with
KASAN_OUTLINE enabled). For DEBUG_WX [3], this means boot hangs for that
long, too.
If I don't allocate vmalloc shadow (and remove the apparently pointlesss
shadow of the shadow area), and only allocate shadow for the image,
fixmap, vmemmap and so on, that delay gets cut to a few seconds, which
is tolerable for a debug configuration...
... however, things blow up when the kernel touches vmalloc'd memory for
the first time, as we don't install shadow for that dynamically.
Thanks,
Mark.
[1] https://lkml.kernel.org/r/CALCETrWucrYp+yq8RHSDqf93xtg793duByirurzJbLRhrz=tcA at mail.gmail.com
[2] https://lkml.kernel.org/r/20161003092940.GA691 at gmail.com
[3] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-October/464191.html
^ permalink raw reply
* Re: [PATCH 2/4] usb: musb: Fix sleeping function called from invalid context for hdrc glue
From: Johan Hovold @ 2016-11-08 19:03 UTC (permalink / raw)
To: Tony Lindgren
Cc: Johan Hovold, Bin Liu, Boris Brezillon, Greg Kroah-Hartman,
Andreas Kemnade, Felipe Balbi, George Cherian,
Kishon Vijay Abraham I, Ivaylo Dimitrov, Ladislav Michl,
Laurent Pinchart, Sergei Shtylyov,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-omap-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20161108173413.GM2428-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
On Tue, Nov 08, 2016 at 10:34:13AM -0700, Tony Lindgren wrote:
> * Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161108 10:09]:
> > On Mon, Nov 07, 2016 at 02:50:18PM -0700, Tony Lindgren wrote:
> > > @@ -2604,6 +2669,9 @@ static int musb_resume(struct device *dev)
> > > mask = MUSB_DEVCTL_BDEVICE | MUSB_DEVCTL_FSDEV | MUSB_DEVCTL_LSDEV;
> > > if ((devctl & mask) != (musb->context.devctl & mask))
> > > musb->port1_status = 0;
> > > +
> > > + schedule_delayed_work(&musb->pending_resume_work, 0);
> > > +
> >
> > The interactions with system suspend looks a bit suspicious. It seems
> > you need to drain any pending resume work on suspend for example.
> >
> > And then the above should not be needed, right?
>
> Hmm that's an interesting idea. I think that would allow us to get
> rid of the delayed work if we check the list both on resume and
> suspend :)
Note that I was referring to draining the runtime-resume work on system
suspend above. I think we still want the delayed work considering that
we could stay active for long periods of time.
> > In fact, the dsps timer must also be cancelled on suspend, or you could
> > end up calling dsps_check_status() while suspended (it is currently not
> > cancelled until the parent device is suspended, which could be too
> > late).
>
> And then this should no longer be an issue either.
It would still be an issue as a system-suspending device could already
have been runtime-resumed so that dsps_check_status() would be called
directly from the timer function.
Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: BUG: Hung task timeouts in for-4.10/dio
From: Logan Gunthorpe @ 2016-11-08 19:03 UTC (permalink / raw)
To: Jens Axboe, Christoph Hellwig; +Cc: linux-block, Stephen Bates
In-Reply-To: <9fe151a1-2d9f-28ef-25a7-bbcb01d8d972@fb.com>
Hey,
I haven't check 82a78cd, but when I tried reverting the commit in
yesterdays version there were conflicts, as a subsequent patch removed
the defines that the specific patch operated on.
Logan
On 08/11/16 12:01 PM, Jens Axboe wrote:
> On 11/08/2016 11:59 AM, Logan Gunthorpe wrote:
>> Hi Jens,
>>
>> Thanks for the quick reply. I just built 82a78cd and I'm seeing the same
>> problem as reported.
>
> Hmm, very odd. Does it work if you run that branch and revert the commit
> you bisected as troublesome, b685d3d65ac7?
>
^ permalink raw reply
* Re: [QEMU PATCH v2] kvmclock: advance clock by time window between vm_stop and pre_save
From: Marcelo Tosatti @ 2016-11-08 13:32 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: kvm, qemu-devel, Paolo Bonzini, Radim Krčmář,
Juan Quintela, Eduardo Habkost
In-Reply-To: <20161108102255.GC2042@work-vm>
On Tue, Nov 08, 2016 at 10:22:56AM +0000, Dr. David Alan Gilbert wrote:
> * Marcelo Tosatti (mtosatti@redhat.com) wrote:
> > On Mon, Nov 07, 2016 at 08:03:50PM +0000, Dr. David Alan Gilbert wrote:
> > > * Marcelo Tosatti (mtosatti@redhat.com) wrote:
> > > > On Mon, Nov 07, 2016 at 03:46:11PM +0000, Dr. David Alan Gilbert wrote:
> > > > > * Marcelo Tosatti (mtosatti@redhat.com) wrote:
> > > > > > This patch, relative to pre-copy migration codepath,
> > > > > > measures the time between vm_stop() and pre_save(),
> > > > > > which includes copying the remaining RAM to destination,
> > > > > > and advances the clock by that amount.
> > > > > >
> > > > > > In a VM with 5 seconds downtime, this reduces the guest
> > > > > > clock difference on destination from 5s to 0.2s.
> > > > > >
> > > > > > Tested with Linux and Windows 2012 R2 guests with -cpu XXX,+hv-time.
> > > > >
> > > > > One thing that bothers me is that it's only this clock that's
> > > > > getting corrected; doesn't it cause things to get upset when
> > > > > one clock moves and the others dont?
> > > >
> > > > If you are correlating the clocks, then yes.
> > > >
> > > > Older Linux guests get upset (marking the TSC clocksource unstable
> > > > because the watchdog checks TSC vs kvmclock), but there is a workaround for it
> > > > in newer guests
> > > > (kvmclock interface to notify watchdog to not complain).
> > > >
> > > > Note marking TSC clocksource unstable on older guests is harmless
> > > > because kvmclock is the standard clocksource.
> > > >
> > > > For Windows guests, i don't know that Windows correlates between different
> > > > clocks.
> > > >
> > > > That is, there is relative control as to which software reads kvmclock
> > > > or Windows TIMER MSR, so i don't see the need to advance every clock
> > > > exposed.
> > > >
> > > > > Shouldn't the pause delay be recorded somewhere architecturally
> > > > > independent and then be a thing that kvm-clock happens to use and
> > > > > other clocks might as well?
> > > >
> > > > In theory, yes. In practice, i don't see the need for this...
> > >
> > > It seems unlikely to me that x86 is the only one that will want
> > > to do something similar.
> >
> > Can't they copy what kvmclock is doing today?
>
> We shouldn't have copies of code all over should we?
>
> Dave
Fine i'll add a notifier.
^ permalink raw reply
* Re: [Qemu-devel] [QEMU PATCH v2] kvmclock: advance clock by time window between vm_stop and pre_save
From: Marcelo Tosatti @ 2016-11-08 13:32 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: kvm, qemu-devel, Paolo Bonzini, Radim Krčmář,
Juan Quintela, Eduardo Habkost
In-Reply-To: <20161108102255.GC2042@work-vm>
On Tue, Nov 08, 2016 at 10:22:56AM +0000, Dr. David Alan Gilbert wrote:
> * Marcelo Tosatti (mtosatti@redhat.com) wrote:
> > On Mon, Nov 07, 2016 at 08:03:50PM +0000, Dr. David Alan Gilbert wrote:
> > > * Marcelo Tosatti (mtosatti@redhat.com) wrote:
> > > > On Mon, Nov 07, 2016 at 03:46:11PM +0000, Dr. David Alan Gilbert wrote:
> > > > > * Marcelo Tosatti (mtosatti@redhat.com) wrote:
> > > > > > This patch, relative to pre-copy migration codepath,
> > > > > > measures the time between vm_stop() and pre_save(),
> > > > > > which includes copying the remaining RAM to destination,
> > > > > > and advances the clock by that amount.
> > > > > >
> > > > > > In a VM with 5 seconds downtime, this reduces the guest
> > > > > > clock difference on destination from 5s to 0.2s.
> > > > > >
> > > > > > Tested with Linux and Windows 2012 R2 guests with -cpu XXX,+hv-time.
> > > > >
> > > > > One thing that bothers me is that it's only this clock that's
> > > > > getting corrected; doesn't it cause things to get upset when
> > > > > one clock moves and the others dont?
> > > >
> > > > If you are correlating the clocks, then yes.
> > > >
> > > > Older Linux guests get upset (marking the TSC clocksource unstable
> > > > because the watchdog checks TSC vs kvmclock), but there is a workaround for it
> > > > in newer guests
> > > > (kvmclock interface to notify watchdog to not complain).
> > > >
> > > > Note marking TSC clocksource unstable on older guests is harmless
> > > > because kvmclock is the standard clocksource.
> > > >
> > > > For Windows guests, i don't know that Windows correlates between different
> > > > clocks.
> > > >
> > > > That is, there is relative control as to which software reads kvmclock
> > > > or Windows TIMER MSR, so i don't see the need to advance every clock
> > > > exposed.
> > > >
> > > > > Shouldn't the pause delay be recorded somewhere architecturally
> > > > > independent and then be a thing that kvm-clock happens to use and
> > > > > other clocks might as well?
> > > >
> > > > In theory, yes. In practice, i don't see the need for this...
> > >
> > > It seems unlikely to me that x86 is the only one that will want
> > > to do something similar.
> >
> > Can't they copy what kvmclock is doing today?
>
> We shouldn't have copies of code all over should we?
>
> Dave
Fine i'll add a notifier.
^ permalink raw reply
* [RESPIN-PATCH v2 1/2] ARM: EXYNOS: Remove static mapping of SCU SFR
From: Krzysztof Kozlowski @ 2016-11-08 19:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478603984-504-2-git-send-email-pankaj.dubey@samsung.com>
On Tue, Nov 08, 2016 at 04:49:43PM +0530, Pankaj Dubey wrote:
> Lets remove static mapping of SCU SFR mainly used in CORTEX-A9 SoC based boards.
> Instead use mapping from device tree node of SCU.
>
> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
> Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
> ---
> arch/arm/mach-exynos/common.h | 1 +
> arch/arm/mach-exynos/exynos.c | 22 ------------------
> arch/arm/mach-exynos/include/mach/map.h | 2 --
> arch/arm/mach-exynos/platsmp.c | 34 +++++++++++++++++++++-------
> arch/arm/mach-exynos/pm.c | 5 ++--
> arch/arm/mach-exynos/suspend.c | 13 ++++-------
> arch/arm/plat-samsung/include/plat/map-s5p.h | 4 ----
> 7 files changed, 34 insertions(+), 47 deletions(-)
>
> diff --git a/arch/arm/mach-exynos/common.h b/arch/arm/mach-exynos/common.h
> index 9424a8a..dd5d8e8 100644
> --- a/arch/arm/mach-exynos/common.h
> +++ b/arch/arm/mach-exynos/common.h
> @@ -161,6 +161,7 @@ extern void exynos_cpu_restore_register(void);
> extern void exynos_pm_central_suspend(void);
> extern int exynos_pm_central_resume(void);
> extern void exynos_enter_aftr(void);
> +extern int exynos_scu_enable(void);
>
> extern struct cpuidle_exynos_data cpuidle_coupled_exynos_data;
>
> diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c
> index 757fc11..fa08ef9 100644
> --- a/arch/arm/mach-exynos/exynos.c
> +++ b/arch/arm/mach-exynos/exynos.c
> @@ -28,15 +28,6 @@
>
> #include "common.h"
>
> -static struct map_desc exynos4_iodesc[] __initdata = {
> - {
> - .virtual = (unsigned long)S5P_VA_COREPERI_BASE,
> - .pfn = __phys_to_pfn(EXYNOS4_PA_COREPERI),
> - .length = SZ_8K,
> - .type = MT_DEVICE,
> - },
> -};
> -
> static struct platform_device exynos_cpuidle = {
> .name = "exynos_cpuidle",
> #ifdef CONFIG_ARM_EXYNOS_CPUIDLE
> @@ -99,17 +90,6 @@ static int __init exynos_fdt_map_chipid(unsigned long node, const char *uname,
> return 1;
> }
>
> -/*
> - * exynos_map_io
> - *
> - * register the standard cpu IO areas
> - */
> -static void __init exynos_map_io(void)
> -{
> - if (soc_is_exynos4())
> - iotable_init(exynos4_iodesc, ARRAY_SIZE(exynos4_iodesc));
> -}
> -
> static void __init exynos_init_io(void)
> {
> debug_ll_io_init();
> @@ -118,8 +98,6 @@ static void __init exynos_init_io(void)
>
> /* detect cpu id and rev. */
> s5p_init_cpu(S5P_VA_CHIPID);
> -
> - exynos_map_io();
> }
>
> /*
> diff --git a/arch/arm/mach-exynos/include/mach/map.h b/arch/arm/mach-exynos/include/mach/map.h
> index 5fb0040..0eef407 100644
> --- a/arch/arm/mach-exynos/include/mach/map.h
> +++ b/arch/arm/mach-exynos/include/mach/map.h
> @@ -18,6 +18,4 @@
>
> #define EXYNOS_PA_CHIPID 0x10000000
>
> -#define EXYNOS4_PA_COREPERI 0x10500000
> -
> #endif /* __ASM_ARCH_MAP_H */
> diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c
> index a5d6841..94405c7 100644
> --- a/arch/arm/mach-exynos/platsmp.c
> +++ b/arch/arm/mach-exynos/platsmp.c
> @@ -168,6 +168,27 @@ int exynos_cluster_power_state(int cluster)
> S5P_CORE_LOCAL_PWR_EN);
> }
>
> +/**
> + * exynos_scu_enable : enables SCU for Cortex-A9 based system
> + * returns 0 on success else non-zero error code
> + */
> +int exynos_scu_enable(void)
> +{
> + struct device_node *np;
> + void __iomem *scu_base;
> +
> + np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu");
> + scu_base = of_iomap(np, 0);
> + of_node_put(np);
> + if (!scu_base) {
> + pr_err("%s failed to map scu_base\n", __func__);
> + return -ENOMEM;
> + }
> + scu_enable(scu_base);
> + iounmap(scu_base);
> + return 0;
> +}
> +
> static void __iomem *cpu_boot_reg_base(void)
> {
> if (soc_is_exynos4210() && samsung_rev() == EXYNOS4210_REV_1_1)
> @@ -224,11 +245,6 @@ static void write_pen_release(int val)
> sync_cache_w(&pen_release);
> }
>
> -static void __iomem *scu_base_addr(void)
> -{
> - return (void __iomem *)(S5P_VA_SCU);
> -}
> -
> static DEFINE_SPINLOCK(boot_lock);
>
> static void exynos_secondary_init(unsigned int cpu)
> @@ -393,9 +409,11 @@ static void __init exynos_smp_prepare_cpus(unsigned int max_cpus)
>
> exynos_set_delayed_reset_assertion(true);
>
> - if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A9)
> - scu_enable(scu_base_addr());
> -
> + if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A9) {
> + /* if exynos_scu_enable fails, return */
> + if (exynos_scu_enable())
> + return;
Ohhh, someone (e.g. out-of-tree DTS) might be surprised with this.
Please mention such change of behaviour in the commit log (describe the
possible impact of this commit).
> + }
> /*
> * Write the address of secondary startup into the
> * system-wide flags register. The boot monitor waits
> diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
> index 487295f..23db2af 100644
> --- a/arch/arm/mach-exynos/pm.c
> +++ b/arch/arm/mach-exynos/pm.c
> @@ -18,6 +18,7 @@
> #include <linux/cpu_pm.h>
> #include <linux/io.h>
> #include <linux/of.h>
> +#include <linux/of_address.h>
Why do you need this include? Was it coming from mach/map.h?
> #include <linux/soc/samsung/exynos-regs-pmu.h>
> #include <linux/soc/samsung/exynos-pmu.h>
>
> @@ -26,8 +27,6 @@
> #include <asm/suspend.h>
> #include <asm/cacheflush.h>
>
> -#include <mach/map.h>
> -
> #include "common.h"
>
> static inline void __iomem *exynos_boot_vector_addr(void)
> @@ -177,7 +176,7 @@ void exynos_enter_aftr(void)
> cpu_suspend(0, exynos_aftr_finisher);
>
> if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A9) {
> - scu_enable(S5P_VA_SCU);
> + exynos_scu_enable();
> if (call_firmware_op(resume) == -ENOSYS)
> exynos_cpu_restore_register();
> }
> diff --git a/arch/arm/mach-exynos/suspend.c b/arch/arm/mach-exynos/suspend.c
> index 06332f6..c73c857 100644
> --- a/arch/arm/mach-exynos/suspend.c
> +++ b/arch/arm/mach-exynos/suspend.c
> @@ -34,8 +34,6 @@
> #include <asm/smp_scu.h>
> #include <asm/suspend.h>
>
> -#include <mach/map.h>
> -
> #include <plat/pm-common.h>
>
> #include "common.h"
> @@ -461,12 +459,11 @@ static void exynos_pm_resume(void)
> /* For release retention */
> exynos_pm_release_retention();
>
> - if (cpuid == ARM_CPU_PART_CORTEX_A9)
> - scu_enable(S5P_VA_SCU);
> -
> - if (call_firmware_op(resume) == -ENOSYS
> - && cpuid == ARM_CPU_PART_CORTEX_A9)
> - exynos_cpu_restore_register();
> + if (cpuid == ARM_CPU_PART_CORTEX_A9) {
> + exynos_scu_enable();
> + if (call_firmware_op(resume) == -ENOSYS)
> + exynos_cpu_restore_register();
It does not look right. I think you changed the logic here. Previously
if CPU != A9, then call_firmware_op() was executed. Now it won't be.
BTW, don't respin patchset with the same version number. This is
basically v3. To me, increasing version number is always welcomed. It
makes dealign with patches easier.
Best regards,
Krzysztof
> + }
>
> early_wakeup:
>
> diff --git a/arch/arm/plat-samsung/include/plat/map-s5p.h b/arch/arm/plat-samsung/include/plat/map-s5p.h
> index 0fe2828..512ed1f 100644
> --- a/arch/arm/plat-samsung/include/plat/map-s5p.h
> +++ b/arch/arm/plat-samsung/include/plat/map-s5p.h
> @@ -15,10 +15,6 @@
>
> #define S5P_VA_CHIPID S3C_ADDR(0x02000000)
>
> -#define S5P_VA_COREPERI_BASE S3C_ADDR(0x02800000)
> -#define S5P_VA_COREPERI(x) (S5P_VA_COREPERI_BASE + (x))
> -#define S5P_VA_SCU S5P_VA_COREPERI(0x0)
> -
> #define VA_VIC(x) (S3C_VA_IRQ + ((x) * 0x10000))
> #define VA_VIC0 VA_VIC(0)
> #define VA_VIC1 VA_VIC(1)
> --
> 2.7.4
>
^ permalink raw reply
* Re: [RESPIN-PATCH v2 1/2] ARM: EXYNOS: Remove static mapping of SCU SFR
From: Krzysztof Kozlowski @ 2016-11-08 19:04 UTC (permalink / raw)
To: Pankaj Dubey; +Cc: linux-samsung-soc, linux-arm-kernel, krzk, kgene, thomas.ab
In-Reply-To: <1478603984-504-2-git-send-email-pankaj.dubey@samsung.com>
On Tue, Nov 08, 2016 at 04:49:43PM +0530, Pankaj Dubey wrote:
> Lets remove static mapping of SCU SFR mainly used in CORTEX-A9 SoC based boards.
> Instead use mapping from device tree node of SCU.
>
> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
> Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
> ---
> arch/arm/mach-exynos/common.h | 1 +
> arch/arm/mach-exynos/exynos.c | 22 ------------------
> arch/arm/mach-exynos/include/mach/map.h | 2 --
> arch/arm/mach-exynos/platsmp.c | 34 +++++++++++++++++++++-------
> arch/arm/mach-exynos/pm.c | 5 ++--
> arch/arm/mach-exynos/suspend.c | 13 ++++-------
> arch/arm/plat-samsung/include/plat/map-s5p.h | 4 ----
> 7 files changed, 34 insertions(+), 47 deletions(-)
>
> diff --git a/arch/arm/mach-exynos/common.h b/arch/arm/mach-exynos/common.h
> index 9424a8a..dd5d8e8 100644
> --- a/arch/arm/mach-exynos/common.h
> +++ b/arch/arm/mach-exynos/common.h
> @@ -161,6 +161,7 @@ extern void exynos_cpu_restore_register(void);
> extern void exynos_pm_central_suspend(void);
> extern int exynos_pm_central_resume(void);
> extern void exynos_enter_aftr(void);
> +extern int exynos_scu_enable(void);
>
> extern struct cpuidle_exynos_data cpuidle_coupled_exynos_data;
>
> diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c
> index 757fc11..fa08ef9 100644
> --- a/arch/arm/mach-exynos/exynos.c
> +++ b/arch/arm/mach-exynos/exynos.c
> @@ -28,15 +28,6 @@
>
> #include "common.h"
>
> -static struct map_desc exynos4_iodesc[] __initdata = {
> - {
> - .virtual = (unsigned long)S5P_VA_COREPERI_BASE,
> - .pfn = __phys_to_pfn(EXYNOS4_PA_COREPERI),
> - .length = SZ_8K,
> - .type = MT_DEVICE,
> - },
> -};
> -
> static struct platform_device exynos_cpuidle = {
> .name = "exynos_cpuidle",
> #ifdef CONFIG_ARM_EXYNOS_CPUIDLE
> @@ -99,17 +90,6 @@ static int __init exynos_fdt_map_chipid(unsigned long node, const char *uname,
> return 1;
> }
>
> -/*
> - * exynos_map_io
> - *
> - * register the standard cpu IO areas
> - */
> -static void __init exynos_map_io(void)
> -{
> - if (soc_is_exynos4())
> - iotable_init(exynos4_iodesc, ARRAY_SIZE(exynos4_iodesc));
> -}
> -
> static void __init exynos_init_io(void)
> {
> debug_ll_io_init();
> @@ -118,8 +98,6 @@ static void __init exynos_init_io(void)
>
> /* detect cpu id and rev. */
> s5p_init_cpu(S5P_VA_CHIPID);
> -
> - exynos_map_io();
> }
>
> /*
> diff --git a/arch/arm/mach-exynos/include/mach/map.h b/arch/arm/mach-exynos/include/mach/map.h
> index 5fb0040..0eef407 100644
> --- a/arch/arm/mach-exynos/include/mach/map.h
> +++ b/arch/arm/mach-exynos/include/mach/map.h
> @@ -18,6 +18,4 @@
>
> #define EXYNOS_PA_CHIPID 0x10000000
>
> -#define EXYNOS4_PA_COREPERI 0x10500000
> -
> #endif /* __ASM_ARCH_MAP_H */
> diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c
> index a5d6841..94405c7 100644
> --- a/arch/arm/mach-exynos/platsmp.c
> +++ b/arch/arm/mach-exynos/platsmp.c
> @@ -168,6 +168,27 @@ int exynos_cluster_power_state(int cluster)
> S5P_CORE_LOCAL_PWR_EN);
> }
>
> +/**
> + * exynos_scu_enable : enables SCU for Cortex-A9 based system
> + * returns 0 on success else non-zero error code
> + */
> +int exynos_scu_enable(void)
> +{
> + struct device_node *np;
> + void __iomem *scu_base;
> +
> + np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu");
> + scu_base = of_iomap(np, 0);
> + of_node_put(np);
> + if (!scu_base) {
> + pr_err("%s failed to map scu_base\n", __func__);
> + return -ENOMEM;
> + }
> + scu_enable(scu_base);
> + iounmap(scu_base);
> + return 0;
> +}
> +
> static void __iomem *cpu_boot_reg_base(void)
> {
> if (soc_is_exynos4210() && samsung_rev() == EXYNOS4210_REV_1_1)
> @@ -224,11 +245,6 @@ static void write_pen_release(int val)
> sync_cache_w(&pen_release);
> }
>
> -static void __iomem *scu_base_addr(void)
> -{
> - return (void __iomem *)(S5P_VA_SCU);
> -}
> -
> static DEFINE_SPINLOCK(boot_lock);
>
> static void exynos_secondary_init(unsigned int cpu)
> @@ -393,9 +409,11 @@ static void __init exynos_smp_prepare_cpus(unsigned int max_cpus)
>
> exynos_set_delayed_reset_assertion(true);
>
> - if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A9)
> - scu_enable(scu_base_addr());
> -
> + if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A9) {
> + /* if exynos_scu_enable fails, return */
> + if (exynos_scu_enable())
> + return;
Ohhh, someone (e.g. out-of-tree DTS) might be surprised with this.
Please mention such change of behaviour in the commit log (describe the
possible impact of this commit).
> + }
> /*
> * Write the address of secondary startup into the
> * system-wide flags register. The boot monitor waits
> diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
> index 487295f..23db2af 100644
> --- a/arch/arm/mach-exynos/pm.c
> +++ b/arch/arm/mach-exynos/pm.c
> @@ -18,6 +18,7 @@
> #include <linux/cpu_pm.h>
> #include <linux/io.h>
> #include <linux/of.h>
> +#include <linux/of_address.h>
Why do you need this include? Was it coming from mach/map.h?
> #include <linux/soc/samsung/exynos-regs-pmu.h>
> #include <linux/soc/samsung/exynos-pmu.h>
>
> @@ -26,8 +27,6 @@
> #include <asm/suspend.h>
> #include <asm/cacheflush.h>
>
> -#include <mach/map.h>
> -
> #include "common.h"
>
> static inline void __iomem *exynos_boot_vector_addr(void)
> @@ -177,7 +176,7 @@ void exynos_enter_aftr(void)
> cpu_suspend(0, exynos_aftr_finisher);
>
> if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A9) {
> - scu_enable(S5P_VA_SCU);
> + exynos_scu_enable();
> if (call_firmware_op(resume) == -ENOSYS)
> exynos_cpu_restore_register();
> }
> diff --git a/arch/arm/mach-exynos/suspend.c b/arch/arm/mach-exynos/suspend.c
> index 06332f6..c73c857 100644
> --- a/arch/arm/mach-exynos/suspend.c
> +++ b/arch/arm/mach-exynos/suspend.c
> @@ -34,8 +34,6 @@
> #include <asm/smp_scu.h>
> #include <asm/suspend.h>
>
> -#include <mach/map.h>
> -
> #include <plat/pm-common.h>
>
> #include "common.h"
> @@ -461,12 +459,11 @@ static void exynos_pm_resume(void)
> /* For release retention */
> exynos_pm_release_retention();
>
> - if (cpuid == ARM_CPU_PART_CORTEX_A9)
> - scu_enable(S5P_VA_SCU);
> -
> - if (call_firmware_op(resume) == -ENOSYS
> - && cpuid == ARM_CPU_PART_CORTEX_A9)
> - exynos_cpu_restore_register();
> + if (cpuid == ARM_CPU_PART_CORTEX_A9) {
> + exynos_scu_enable();
> + if (call_firmware_op(resume) == -ENOSYS)
> + exynos_cpu_restore_register();
It does not look right. I think you changed the logic here. Previously
if CPU != A9, then call_firmware_op() was executed. Now it won't be.
BTW, don't respin patchset with the same version number. This is
basically v3. To me, increasing version number is always welcomed. It
makes dealign with patches easier.
Best regards,
Krzysztof
> + }
>
> early_wakeup:
>
> diff --git a/arch/arm/plat-samsung/include/plat/map-s5p.h b/arch/arm/plat-samsung/include/plat/map-s5p.h
> index 0fe2828..512ed1f 100644
> --- a/arch/arm/plat-samsung/include/plat/map-s5p.h
> +++ b/arch/arm/plat-samsung/include/plat/map-s5p.h
> @@ -15,10 +15,6 @@
>
> #define S5P_VA_CHIPID S3C_ADDR(0x02000000)
>
> -#define S5P_VA_COREPERI_BASE S3C_ADDR(0x02800000)
> -#define S5P_VA_COREPERI(x) (S5P_VA_COREPERI_BASE + (x))
> -#define S5P_VA_SCU S5P_VA_COREPERI(0x0)
> -
> #define VA_VIC(x) (S3C_VA_IRQ + ((x) * 0x10000))
> #define VA_VIC0 VA_VIC(0)
> #define VA_VIC1 VA_VIC(1)
> --
> 2.7.4
>
^ permalink raw reply
* KASAN & the vmalloc area
From: Mark Rutland @ 2016-11-08 19:03 UTC (permalink / raw)
To: Dmitry Vyukov, Andy Lutomirski
Cc: Andrey Ryabinin, Laura Abbott, Ard Biesheuvel, linux-kernel,
linux-arm-kernel
Hi,
I see a while back [1] there was a discussion of what to do about KASAN
and vmapped stacks, but it doesn't look like that was solved, judging by
the vmapped stacks pull [2] for v4.9.
I wondered whether anyone had looked at that since?
I have an additional reason to want to dynamically allocate the vmalloc
area shadow: it turns out that KASAN currently interacts rather poorly
with the arm64 ptdump code.
When KASAN is selected, we allocate shadow for the whole vmalloc area,
using common zero pte, pmd, pud tables. Walking over these in the ptdump
code takes a *very* long time (I've seen up to 15 minutes with
KASAN_OUTLINE enabled). For DEBUG_WX [3], this means boot hangs for that
long, too.
If I don't allocate vmalloc shadow (and remove the apparently pointlesss
shadow of the shadow area), and only allocate shadow for the image,
fixmap, vmemmap and so on, that delay gets cut to a few seconds, which
is tolerable for a debug configuration...
... however, things blow up when the kernel touches vmalloc'd memory for
the first time, as we don't install shadow for that dynamically.
Thanks,
Mark.
[1] https://lkml.kernel.org/r/CALCETrWucrYp+yq8RHSDqf93xtg793duByirurzJbLRhrz=tcA@mail.gmail.com
[2] https://lkml.kernel.org/r/20161003092940.GA691@gmail.com
[3] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-October/464191.html
^ permalink raw reply
* Re: [B.A.T.M.A.N.] [PATCH 13/17] batman-adv: Consume skb in receive handlers
From: Sven Eckelmann @ 2016-11-08 19:05 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, b.a.t.m.a.n, davem
In-Reply-To: <1478626981.17367.16.camel@edumazet-glaptop3.roam.corp.google.com>
[-- Attachment #1: Type: text/plain, Size: 302 bytes --]
On Dienstag, 8. November 2016 09:43:01 CET Eric Dumazet wrote:
[...]
> Sure, but your patch 13/17 should address this right away.
[...]
Fair enough. I've asked Simon to resubmit the patches with the
"consume_skb -> conditional kfree_skb/consume_skb" patch squashed
into patch 13.
Kind regards,
Sven
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH 13/17] batman-adv: Consume skb in receive handlers
From: Sven Eckelmann @ 2016-11-08 19:05 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Simon Wunderlich, davem, netdev, b.a.t.m.a.n
In-Reply-To: <1478626981.17367.16.camel@edumazet-glaptop3.roam.corp.google.com>
[-- Attachment #1: Type: text/plain, Size: 302 bytes --]
On Dienstag, 8. November 2016 09:43:01 CET Eric Dumazet wrote:
[...]
> Sure, but your patch 13/17 should address this right away.
[...]
Fair enough. I've asked Simon to resubmit the patches with the
"consume_skb -> conditional kfree_skb/consume_skb" patch squashed
into patch 13.
Kind regards,
Sven
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH v10 5/7] x86/cpufeature: Detect CPUID faulting support
From: Thomas Gleixner @ 2016-11-08 19:06 UTC (permalink / raw)
To: Kyle Huey
Cc: Robert O'Callahan, Andy Lutomirski, Ingo Molnar,
H. Peter Anvin, x86, Paolo Bonzini, Radim Krčmář,
Jeff Dike, Richard Weinberger, Alexander Viro, Shuah Khan,
Dave Hansen, Borislav Petkov, Peter Zijlstra, Boris Ostrovsky,
Len Brown, Rafael J. Wysocki, Dmitry Safonov, David Matlack,
linux-kernel, user-mode-linux-devel, user-mode-linux-user,
linux-fsdevel, linux-kselftest, kvm
In-Reply-To: <20161108183956.4521-6-khuey@kylehuey.com>
On Tue, 8 Nov 2016, Kyle Huey wrote:
> Intel supports faulting on the CPUID instruction beginning with Ivy Bridge.
> When enabled, the processor will fault on attempts to execute the CPUID
> instruction with CPL>0. This will allow a ptracer to emulate the CPUID
> instruction.
>
> Bit 31 of MSR_PLATFORM_INFO advertises support for this feature. It is
> documented in detail in Section 2.3.2 of
> http://www.intel.com/content/dam/www/public/us/en/documents/application-notes/virtualization-technology-flexmigration-application-note.pdf
Can you please stick that document into the kernel bugzilla, as it's going
to be on a different place before this gets merged into Linus tree?
See: http://lkml.kernel.org/r/1478631281-5061-1-git-send-email-kan.liang@intel.com
> + static const struct msr_bit msr_bits[] = {
> + { X86_FEATURE_CPUID_FAULT, MSR_PLATFORM_INFO, 31 },
Can you please make that PLATINFO_CPUID_FAULT_BIT instead of 31?
Thanks,
tglx
^ permalink raw reply
* Re: ceph rpm packaging failure -- installed but unpackaged files found
From: Ken Dreyer @ 2016-11-08 19:06 UTC (permalink / raw)
To: Jeff Layton; +Cc: Ceph Development
In-Reply-To: <1478631747.6066.7.camel@redhat.com>
Hi Jeff,
It sounds like we need a new "python-rgw" package for RPMs/DEBs so
these files are properly packaged.
- Ken
On Tue, Nov 8, 2016 at 12:02 PM, Jeff Layton <jlayton@redhat.com> wrote:
> I needed to build some rpms from a make-srpm.sh generated srpm, and hit
> this failure during the packaging phase. Something with the rgw python
> bindings it appears?
>
> The tree here is based on a38d53e1d29eefc35dd5cfb14dc0996253cdc468,
> with a few cephfs patches on top.
>
> Here's the tail end of the rpm build log:
>
> Provides: ceph-debuginfo = 1:11.0.2-1287.gbd4abb0f7496.fc24 ceph-debuginfo(x86-64) = 1:11.0.2-1287.gbd4abb0f7496.fc24
> Requires(rpmlib): rpmlib(CompressedFileNames) <= 3.0.4-1 rpmlib(FileDigests) <= 4.6.0-1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1
> Checking for unpackaged file(s): /usr/lib/rpm/check-files /home/jlayton/rpmbuild/BUILDROOT/ceph-11.0.2-1287.gbd4abb0f7496.fc24.x86_64
> error: Installed (but unpackaged) file(s) found:
> /usr/lib64/python2.7/site-packages/rgw-2.0.0-py2.7.egg-info/PKG-INFO
> /usr/lib64/python2.7/site-packages/rgw-2.0.0-py2.7.egg-info/SOURCES.txt
> /usr/lib64/python2.7/site-packages/rgw-2.0.0-py2.7.egg-info/dependency_links.txt
> /usr/lib64/python2.7/site-packages/rgw-2.0.0-py2.7.egg-info/top_level.txt
> /usr/lib64/python2.7/site-packages/rgw.so
> /usr/lib64/python3.5/site-packages/rgw-2.0.0-py3.5.egg-info/PKG-INFO
> /usr/lib64/python3.5/site-packages/rgw-2.0.0-py3.5.egg-info/SOURCES.txt
> /usr/lib64/python3.5/site-packages/rgw-2.0.0-py3.5.egg-info/dependency_links.txt
> /usr/lib64/python3.5/site-packages/rgw-2.0.0-py3.5.egg-info/top_level.txt
> /usr/lib64/python3.5/site-packages/rgw.cpython-35m-x86_64-linux-gnu.so
>
>
> RPM build errors:
> Installed (but unpackaged) file(s) found:
> /usr/lib64/python2.7/site-packages/rgw-2.0.0-py2.7.egg-info/PKG-INFO
> /usr/lib64/python2.7/site-packages/rgw-2.0.0-py2.7.egg-info/SOURCES.txt
> /usr/lib64/python2.7/site-packages/rgw-2.0.0-py2.7.egg-info/dependency_links.txt
> /usr/lib64/python2.7/site-packages/rgw-2.0.0-py2.7.egg-info/top_level.txt
> /usr/lib64/python2.7/site-packages/rgw.so
> /usr/lib64/python3.5/site-packages/rgw-2.0.0-py3.5.egg-info/PKG-INFO
> /usr/lib64/python3.5/site-packages/rgw-2.0.0-py3.5.egg-info/SOURCES.txt
> /usr/lib64/python3.5/site-packages/rgw-2.0.0-py3.5.egg-info/dependency_links.txt
> /usr/lib64/python3.5/site-packages/rgw-2.0.0-py3.5.egg-info/top_level.txt
> /usr/lib64/python3.5/site-packages/rgw.cpython-35m-x86_64-linux-gnu.so
> --
> Jeff Layton <jlayton@redhat.com>
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH net-next 1/2] bnxt_en: do not call napi_hash_add()
From: Eric Dumazet @ 2016-11-08 19:06 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Michael Chan
From: Eric Dumazet <edumazet@google.com>
This is automatically done from netif_napi_add(), and we want to not
export napi_hash_add() anymore in the following patch.
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Michael Chan <michael.chan@broadcom.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index a042da1ff4b90e9aae4f76db71c99c2c4da321d3..d313b02485a10b2b7995076578ce3632865b475f 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -4954,7 +4954,6 @@ static void bnxt_init_napi(struct bnxt *bp)
bnapi = bp->bnapi[cp_nr_rings];
netif_napi_add(bp->dev, &bnapi->napi,
bnxt_poll_nitroa0, 64);
- napi_hash_add(&bnapi->napi);
}
} else {
bnapi = bp->bnapi[0];
^ permalink raw reply related
* Re: [PATCH 01/10] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus
From: Daniel De Graaf @ 2016-11-08 19:07 UTC (permalink / raw)
To: Boris Ostrovsky, xen-devel
Cc: andrew.cooper3, wei.liu2, ian.jackson, jbeulich, roger.pau
In-Reply-To: <1478468563-5396-2-git-send-email-boris.ostrovsky@oracle.com>
On 11/06/2016 04:42 PM, Boris Ostrovsky wrote:
> This domctl is called when a VCPU is hot-(un)plugged to a guest (via
> 'xl vcpu-set'). While this currently is only intended to be needed by
> PVH guests we will call this domctl for all (x86) guests for consistency.
>
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply
* [PATCH net-next 2/2] net: napi_hash_add() is no longer exported
From: Eric Dumazet @ 2016-11-08 19:07 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Michael Chan
From: Eric Dumazet <edumazet@google.com>
There are no more users except from net/core/dev.c
napi_hash_add() can now be static.
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Michael Chan <michael.chan@broadcom.com>
---
include/linux/netdevice.h | 11 -----------
net/core/dev.c | 3 +--
2 files changed, 1 insertion(+), 13 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 66fd61c681d90d4a7ecc3bf7bae44c2b3b1fe10c..d64135a0ab718b9e119646b74f92901e8fe4b356 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -468,17 +468,6 @@ static inline void napi_complete(struct napi_struct *n)
}
/**
- * napi_hash_add - add a NAPI to global hashtable
- * @napi: NAPI context
- *
- * Generate a new napi_id and store a @napi under it in napi_hash.
- * Used for busy polling (CONFIG_NET_RX_BUSY_POLL).
- * Note: This is normally automatically done from netif_napi_add(),
- * so might disappear in a future Linux version.
- */
-void napi_hash_add(struct napi_struct *napi);
-
-/**
* napi_hash_del - remove a NAPI from global table
* @napi: NAPI context
*
diff --git a/net/core/dev.c b/net/core/dev.c
index 0260ad314506c621215a0e3449392bc01aad55ca..e2148d9845868d7ca5d2c6b853cb0fbe18b32163 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5017,7 +5017,7 @@ EXPORT_SYMBOL(sk_busy_loop);
#endif /* CONFIG_NET_RX_BUSY_POLL */
-void napi_hash_add(struct napi_struct *napi)
+static void napi_hash_add(struct napi_struct *napi)
{
if (test_bit(NAPI_STATE_NO_BUSY_POLL, &napi->state) ||
test_and_set_bit(NAPI_STATE_HASHED, &napi->state))
@@ -5037,7 +5037,6 @@ void napi_hash_add(struct napi_struct *napi)
spin_unlock(&napi_hash_lock);
}
-EXPORT_SYMBOL_GPL(napi_hash_add);
/* Warning : caller is responsible to make sure rcu grace period
* is respected before freeing memory containing @napi
^ permalink raw reply related
* Re: [PATCH 1/2] mfd: pm8921: add support to pm8821
From: Bjorn Andersson @ 2016-11-08 19:07 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: Lee Jones, Rob Herring, Andy Gross, devicetree, linux-kernel,
linux-arm-msm, linux-soc, linux-arm-kernel
In-Reply-To: <1478622577-20699-1-git-send-email-srinivas.kandagatla@linaro.org>
On Tue 08 Nov 08:29 PST 2016, Srinivas Kandagatla wrote:
> This patch adds support to PM8821 PMIC and interrupt support.
> PM8821 is companion device that supplements primary PMIC PM8921 IC.
>
Linus Walleij has a patch out for renaming a lot of things in this file,
so we should probably make sure that lands and then rebase this ontop.
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
> Tested this patch for MPP and IRQ functionality on IFC6410 and SD600 EVAL
> board with mpps PM8821 and PM8921.
>
> .../devicetree/bindings/mfd/qcom-pm8xxx.txt | 1 +
> drivers/mfd/pm8921-core.c | 368 +++++++++++++++++++--
> 2 files changed, 340 insertions(+), 29 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt b/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt
> index 37a088f..8f1b4ec 100644
> --- a/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt
> +++ b/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt
> @@ -11,6 +11,7 @@ voltages and other various functionality to Qualcomm SoCs.
> Definition: must be one of:
> "qcom,pm8058"
> "qcom,pm8921"
> + "qcom,pm8821"
>
> - #address-cells:
> Usage: required
> diff --git a/drivers/mfd/pm8921-core.c b/drivers/mfd/pm8921-core.c
> index 0e3a2ea..28c2470 100644
> --- a/drivers/mfd/pm8921-core.c
> +++ b/drivers/mfd/pm8921-core.c
> @@ -28,16 +28,26 @@
> #include <linux/mfd/core.h>
>
> #define SSBI_REG_ADDR_IRQ_BASE 0x1BB
> -
> -#define SSBI_REG_ADDR_IRQ_ROOT (SSBI_REG_ADDR_IRQ_BASE + 0)
> -#define SSBI_REG_ADDR_IRQ_M_STATUS1 (SSBI_REG_ADDR_IRQ_BASE + 1)
> -#define SSBI_REG_ADDR_IRQ_M_STATUS2 (SSBI_REG_ADDR_IRQ_BASE + 2)
> -#define SSBI_REG_ADDR_IRQ_M_STATUS3 (SSBI_REG_ADDR_IRQ_BASE + 3)
> -#define SSBI_REG_ADDR_IRQ_M_STATUS4 (SSBI_REG_ADDR_IRQ_BASE + 4)
> -#define SSBI_REG_ADDR_IRQ_BLK_SEL (SSBI_REG_ADDR_IRQ_BASE + 5)
> -#define SSBI_REG_ADDR_IRQ_IT_STATUS (SSBI_REG_ADDR_IRQ_BASE + 6)
> -#define SSBI_REG_ADDR_IRQ_CONFIG (SSBI_REG_ADDR_IRQ_BASE + 7)
> -#define SSBI_REG_ADDR_IRQ_RT_STATUS (SSBI_REG_ADDR_IRQ_BASE + 8)
Keep these (per argumentation that follows), but try to name them
appropriately.
> +#define SSBI_PM8821_REG_ADDR_IRQ_BASE 0x100
> +
> +#define SSBI_REG_ADDR_IRQ_ROOT (0)
> +#define SSBI_REG_ADDR_IRQ_M_STATUS1 (1)
> +#define SSBI_REG_ADDR_IRQ_M_STATUS2 (2)
> +#define SSBI_REG_ADDR_IRQ_M_STATUS3 (3)
> +#define SSBI_REG_ADDR_IRQ_M_STATUS4 (4)
> +#define SSBI_REG_ADDR_IRQ_BLK_SEL (5)
> +#define SSBI_REG_ADDR_IRQ_IT_STATUS (6)
> +#define SSBI_REG_ADDR_IRQ_CONFIG (7)
> +#define SSBI_REG_ADDR_IRQ_RT_STATUS (8)
Unnecessary parenthesis.
> +
> +#define PM8821_TOTAL_IRQ_MASTERS 2
Unused.
> +#define PM8821_BLOCKS_PER_MASTER 7
> +#define PM8821_IRQ_MASTER1_SET 0x01
BIT(0), but I would prefer that you just inline this with a comment.
> +#define PM8821_IRQ_CLEAR_OFFSET 0x01
Rather than having a single define for this and add in the base and
block numbers I think you should split it into a master0 and master1
define. (And it's not a offset as much as a register)
> +#define PM8821_IRQ_RT_STATUS_OFFSET 0x0f
Dito
> +#define PM8821_IRQ_MASK_REG_OFFSET 0x08
Dito
> +#define SSBI_REG_ADDR_IRQ_MASTER0 0x30
> +#define SSBI_REG_ADDR_IRQ_MASTER1 0xb0
Fold these two into the registers above.
>
> #define PM_IRQF_LVL_SEL 0x01 /* level select */
> #define PM_IRQF_MASK_FE 0x02 /* mask falling edge */
> @@ -54,30 +64,41 @@
> #define REG_HWREV_2 0x0E8 /* PMIC4 revision 2 */
>
> #define PM8921_NR_IRQS 256
> +#define PM8821_NR_IRQS 112
>
> struct pm_irq_chip {
> struct regmap *regmap;
> spinlock_t pm_irq_lock;
> struct irq_domain *irqdomain;
> + unsigned int irq_reg_base;
> unsigned int num_irqs;
> unsigned int num_blocks;
> unsigned int num_masters;
> u8 config[0];
> };
>
> +struct pm8xxx_data {
> + int num_irqs;
> + unsigned int irq_reg_base;
As far as I can see this is always SSBI_PM8821_REG_ADDR_IRQ_BASE in the
8821 functions and SSBI_REG_ADDR_IRQ_BASE in the pm8xxx functions. If
you have disjunct code paths I think it's better to not obscure this
with a variable.
Try renaming the constants appropriately instead. This also has the
benefit of reducing the size of the patch slightly.
> + const struct irq_domain_ops *irq_domain_ops;
> + void (*irq_handler)(struct irq_desc *desc);
> +};
> +
> static int pm8xxx_read_block_irq(struct pm_irq_chip *chip, unsigned int bp,
> unsigned int *ip)
> {
> int rc;
>
> spin_lock(&chip->pm_irq_lock);
> - rc = regmap_write(chip->regmap, SSBI_REG_ADDR_IRQ_BLK_SEL, bp);
> + rc = regmap_write(chip->regmap,
> + chip->irq_reg_base + SSBI_REG_ADDR_IRQ_BLK_SEL, bp);
> if (rc) {
> pr_err("Failed Selecting Block %d rc=%d\n", bp, rc);
> goto bail;
> }
>
> - rc = regmap_read(chip->regmap, SSBI_REG_ADDR_IRQ_IT_STATUS, ip);
> + rc = regmap_read(chip->regmap,
> + chip->irq_reg_base + SSBI_REG_ADDR_IRQ_IT_STATUS, ip);
> if (rc)
> pr_err("Failed Reading Status rc=%d\n", rc);
> bail:
> @@ -91,14 +112,16 @@ pm8xxx_config_irq(struct pm_irq_chip *chip, unsigned int bp, unsigned int cp)
> int rc;
>
> spin_lock(&chip->pm_irq_lock);
> - rc = regmap_write(chip->regmap, SSBI_REG_ADDR_IRQ_BLK_SEL, bp);
> + rc = regmap_write(chip->regmap,
> + chip->irq_reg_base + SSBI_REG_ADDR_IRQ_BLK_SEL, bp);
> if (rc) {
> pr_err("Failed Selecting Block %d rc=%d\n", bp, rc);
> goto bail;
> }
>
> cp |= PM_IRQF_WRITE;
> - rc = regmap_write(chip->regmap, SSBI_REG_ADDR_IRQ_CONFIG, cp);
> + rc = regmap_write(chip->regmap,
> + chip->irq_reg_base + SSBI_REG_ADDR_IRQ_CONFIG, cp);
> if (rc)
> pr_err("Failed Configuring IRQ rc=%d\n", rc);
> bail:
> @@ -137,8 +160,8 @@ static int pm8xxx_irq_master_handler(struct pm_irq_chip *chip, int master)
> unsigned int blockbits;
> int block_number, i, ret = 0;
>
> - ret = regmap_read(chip->regmap, SSBI_REG_ADDR_IRQ_M_STATUS1 + master,
> - &blockbits);
> + ret = regmap_read(chip->regmap, chip->irq_reg_base +
> + SSBI_REG_ADDR_IRQ_M_STATUS1 + master, &blockbits);
> if (ret) {
> pr_err("Failed to read master %d ret=%d\n", master, ret);
> return ret;
> @@ -165,7 +188,8 @@ static void pm8xxx_irq_handler(struct irq_desc *desc)
>
> chained_irq_enter(irq_chip, desc);
>
> - ret = regmap_read(chip->regmap, SSBI_REG_ADDR_IRQ_ROOT, &root);
> + ret = regmap_read(chip->regmap,
> + chip->irq_reg_base + SSBI_REG_ADDR_IRQ_ROOT, &root);
> if (ret) {
> pr_err("Can't read root status ret=%d\n", ret);
> return;
> @@ -182,6 +206,122 @@ static void pm8xxx_irq_handler(struct irq_desc *desc)
> chained_irq_exit(irq_chip, desc);
> }
>
> +static int pm8821_read_master_irq(const struct pm_irq_chip *chip,
> + int m, unsigned int *master)
> +{
I think you should inline this, as you already have the calls unrolled
in pm8821_irq_handler().
> + unsigned int base;
> +
> + if (!m)
> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
> + else
> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
> +
> + return regmap_read(chip->regmap, base, master);
> +}
> +
> +static int pm8821_read_block_irq(struct pm_irq_chip *chip, int master,
> + u8 block, unsigned int *bits)
> +{
> + int rc;
> +
> + unsigned int base;
Odd empty line between rc and base. (And btw, sorting your local
variables in descending length make things pretty).
> +
> + if (!master)
> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
> + else
> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
> +
> + spin_lock(&chip->pm_irq_lock);
The reason why this is done under a lock in the other case is because
the status register is paged, so you shouldn't need it here.
With this updated I think you can favorably inline this into
pm8821_irq_block_handler().
> +
> + rc = regmap_read(chip->regmap, base + block, bits);
> + if (rc)
> + pr_err("Failed Reading Status rc=%d\n", rc);
> +
> + spin_unlock(&chip->pm_irq_lock);
> + return rc;
> +}
> +
> +static int pm8821_irq_block_handler(struct pm_irq_chip *chip,
> + int master_number, int block)
> +{
> + int pmirq, irq, i, ret;
> + unsigned int bits;
> +
> + ret = pm8821_read_block_irq(chip, master_number, block, &bits);
> + if (ret) {
> + pr_err("Failed reading %d block ret=%d", block, ret);
> + return ret;
> + }
> + if (!bits) {
> + pr_err("block bit set in master but no irqs: %d", block);
> + return 0;
> + }
> +
> + /* Convert block offset to global block number */
> + block += (master_number * PM8821_BLOCKS_PER_MASTER) - 1;
So this is block -= 1 for master 0 and block += 6 for master 1, is the
latter correct?
> +
> + /* Check IRQ bits */
> + for (i = 0; i < 8; i++) {
> + if (bits & BIT(i)) {
> + pmirq = block * 8 + i;
> + irq = irq_find_mapping(chip->irqdomain, pmirq);
> + generic_handle_irq(irq);
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int pm8821_irq_read_master(struct pm_irq_chip *chip,
> + int master_number, u8 master_val)
This isn't so much a matter of "reading master X" as "handle master X".
Also, you don't care about the return value, so no need to return one...
> +{
> + int ret = 0;
> + int block;
> +
> + for (block = 1; block < 8; block++) {
> + if (master_val & BIT(block)) {
> + ret |= pm8821_irq_block_handler(chip,
> + master_number, block);
> + }
> + }
> +
> + return ret;
> +}
> +
> +static void pm8821_irq_handler(struct irq_desc *desc)
> +{
> + struct pm_irq_chip *chip = irq_desc_get_handler_data(desc);
> + struct irq_chip *irq_chip = irq_desc_get_chip(desc);
> + int ret;
> + unsigned int master;
> +
> + chained_irq_enter(irq_chip, desc);
> + /* check master 0 */
> + ret = pm8821_read_master_irq(chip, 0, &master);
> + if (ret) {
> + pr_err("Failed to re:Qad master 0 ret=%d\n", ret);
> + return;
> + }
> +
> + if (master & ~PM8821_IRQ_MASTER1_SET)
Rather than having a define for MASTER1_SET use BIT(0) here and write a
comment like:
"bits 1 through 7 marks the first 7 blocks"
> + pm8821_irq_read_master(chip, 0, master);
> +
and then
"bit 0 is set if second master contains any bits"
Or just skip this optimization and check the two masters unconditionally
in a loop.
> + /* check master 1 */
> + if (!(master & PM8821_IRQ_MASTER1_SET))
> + goto done;
> +
> + ret = pm8821_read_master_irq(chip, 1, &master);
> + if (ret) {
> + pr_err("Failed to read master 1 ret=%d\n", ret);
> + return;
> + }
> +
> + pm8821_irq_read_master(chip, 1, master);
> +
> +done:
> + chained_irq_exit(irq_chip, desc);
> +}
> +
> static void pm8xxx_irq_mask_ack(struct irq_data *d)
> {
> struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
> @@ -254,13 +394,15 @@ static int pm8xxx_irq_get_irqchip_state(struct irq_data *d,
> irq_bit = pmirq % 8;
>
> spin_lock(&chip->pm_irq_lock);
> - rc = regmap_write(chip->regmap, SSBI_REG_ADDR_IRQ_BLK_SEL, block);
> + rc = regmap_write(chip->regmap, chip->irq_reg_base +
> + SSBI_REG_ADDR_IRQ_BLK_SEL, block);
> if (rc) {
> pr_err("Failed Selecting Block %d rc=%d\n", block, rc);
> goto bail;
> }
>
> - rc = regmap_read(chip->regmap, SSBI_REG_ADDR_IRQ_RT_STATUS, &bits);
> + rc = regmap_read(chip->regmap, chip->irq_reg_base +
> + SSBI_REG_ADDR_IRQ_RT_STATUS, &bits);
> if (rc) {
> pr_err("Failed Reading Status rc=%d\n", rc);
> goto bail;
> @@ -299,6 +441,151 @@ static const struct irq_domain_ops pm8xxx_irq_domain_ops = {
> .map = pm8xxx_irq_domain_map,
> };
>
> +static void pm8821_irq_mask_ack(struct irq_data *d)
> +{
> + struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
> + unsigned int base, pmirq = irqd_to_hwirq(d);
> + u8 block, master;
> + int irq_bit, rc;
> +
> + block = pmirq / 8;
> + master = block / PM8821_BLOCKS_PER_MASTER;
> + irq_bit = pmirq % 8;
> + block %= PM8821_BLOCKS_PER_MASTER;
You can deobfuscate this somewhat by instead of testing for !master
below you just do:
if (block < PM8821_BLOCKS_PER_MASTER) {
base =
} else {
base =
block -= PM8821_BLOCKS_PER_MASTER;
}
> +
> + if (!master)
> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
> + else
> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
> +
> + spin_lock(&chip->pm_irq_lock);
The irqchip code grabs a lock on the irq_desc, so this can't race with
unmask - and the regmap_update_bits() is internally protecting the
read/write cycle.
So you shouldn't need to lock around this section.
> + rc = regmap_update_bits(chip->regmap,
> + base + PM8821_IRQ_MASK_REG_OFFSET + block,
> + BIT(irq_bit), BIT(irq_bit));
> +
> + if (rc) {
> + pr_err("Failed to read/write mask IRQ:%d rc=%d\n", pmirq, rc);
> + goto fail;
> + }
> +
> + rc = regmap_update_bits(chip->regmap,
> + base + PM8821_IRQ_CLEAR_OFFSET + block,
> + BIT(irq_bit), BIT(irq_bit));
> +
> + if (rc) {
> + pr_err("Failed to read/write IT_CLEAR IRQ:%d rc=%d\n",
> + pmirq, rc);
> + }
> +
> +fail:
> + spin_unlock(&chip->pm_irq_lock);
> +}
> +
> +static void pm8821_irq_unmask(struct irq_data *d)
> +{
> + struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
> + unsigned int base, pmirq = irqd_to_hwirq(d);
> + int irq_bit, rc;
> + u8 block, master;
> +
> + block = pmirq / 8;
> + master = block / PM8821_BLOCKS_PER_MASTER;
> + irq_bit = pmirq % 8;
> + block %= PM8821_BLOCKS_PER_MASTER;
As mask().
> +
> + if (!master)
> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
> + else
> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
> +
> + spin_lock(&chip->pm_irq_lock);
As mask().
> +
> + rc = regmap_update_bits(chip->regmap,
> + base + PM8821_IRQ_MASK_REG_OFFSET + block,
> + BIT(irq_bit), ~BIT(irq_bit));
> +
> + if (rc)
> + pr_err("Failed to read/write unmask IRQ:%d rc=%d\n", pmirq, rc);
> +
> + spin_unlock(&chip->pm_irq_lock);
> +}
> +
> +static int pm8821_irq_set_type(struct irq_data *d, unsigned int flow_type)
> +{
> +
> + /*
> + * PM8821 IRQ controller does not have explicit software support for
> + * IRQ flow type.
> + */
Is returning "success" here the right thing to do? Shouldn't we just
omit the function? Or did you perhaps hit some clients that wouldn't
deal with that?
> + return 0;
> +}
> +
> +static int pm8821_irq_get_irqchip_state(struct irq_data *d,
> + enum irqchip_irq_state which,
> + bool *state)
> +{
> + struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
> + int pmirq, rc;
> + u8 block, irq_bit, master;
> + unsigned int bits;
> + unsigned int base;
> + unsigned long flags;
> +
> + pmirq = irqd_to_hwirq(d);
> +
> + block = pmirq / 8;
> + master = block / PM8821_BLOCKS_PER_MASTER;
> + irq_bit = pmirq % 8;
> + block %= PM8821_BLOCKS_PER_MASTER;
> +
Simplify as in mask().
> + if (!master)
> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
> + else
> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
> +
> + spin_lock_irqsave(&chip->pm_irq_lock, flags);
No need to lock here as we're just reading a single register.
> +
> + rc = regmap_read(chip->regmap,
> + base + PM8821_IRQ_RT_STATUS_OFFSET + block, &bits);
> + if (rc) {
> + pr_err("Failed Reading Status rc=%d\n", rc);
> + goto bail_out;
> + }
> +
> + *state = !!(bits & BIT(irq_bit));
> +
> +bail_out:
> + spin_unlock_irqrestore(&chip->pm_irq_lock, flags);
> +
> + return rc;
> +}
> +
> +static struct irq_chip pm8821_irq_chip = {
> + .name = "pm8821",
> + .irq_mask_ack = pm8821_irq_mask_ack,
> + .irq_unmask = pm8821_irq_unmask,
> + .irq_set_type = pm8821_irq_set_type,
> + .irq_get_irqchip_state = pm8821_irq_get_irqchip_state,
> + .flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE,
> +};
> +
Regards,
Bjorn
^ permalink raw reply
* [PATCH 1/2] mfd: pm8921: add support to pm8821
From: Bjorn Andersson @ 2016-11-08 19:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478622577-20699-1-git-send-email-srinivas.kandagatla@linaro.org>
On Tue 08 Nov 08:29 PST 2016, Srinivas Kandagatla wrote:
> This patch adds support to PM8821 PMIC and interrupt support.
> PM8821 is companion device that supplements primary PMIC PM8921 IC.
>
Linus Walleij has a patch out for renaming a lot of things in this file,
so we should probably make sure that lands and then rebase this ontop.
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
> Tested this patch for MPP and IRQ functionality on IFC6410 and SD600 EVAL
> board with mpps PM8821 and PM8921.
>
> .../devicetree/bindings/mfd/qcom-pm8xxx.txt | 1 +
> drivers/mfd/pm8921-core.c | 368 +++++++++++++++++++--
> 2 files changed, 340 insertions(+), 29 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt b/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt
> index 37a088f..8f1b4ec 100644
> --- a/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt
> +++ b/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt
> @@ -11,6 +11,7 @@ voltages and other various functionality to Qualcomm SoCs.
> Definition: must be one of:
> "qcom,pm8058"
> "qcom,pm8921"
> + "qcom,pm8821"
>
> - #address-cells:
> Usage: required
> diff --git a/drivers/mfd/pm8921-core.c b/drivers/mfd/pm8921-core.c
> index 0e3a2ea..28c2470 100644
> --- a/drivers/mfd/pm8921-core.c
> +++ b/drivers/mfd/pm8921-core.c
> @@ -28,16 +28,26 @@
> #include <linux/mfd/core.h>
>
> #define SSBI_REG_ADDR_IRQ_BASE 0x1BB
> -
> -#define SSBI_REG_ADDR_IRQ_ROOT (SSBI_REG_ADDR_IRQ_BASE + 0)
> -#define SSBI_REG_ADDR_IRQ_M_STATUS1 (SSBI_REG_ADDR_IRQ_BASE + 1)
> -#define SSBI_REG_ADDR_IRQ_M_STATUS2 (SSBI_REG_ADDR_IRQ_BASE + 2)
> -#define SSBI_REG_ADDR_IRQ_M_STATUS3 (SSBI_REG_ADDR_IRQ_BASE + 3)
> -#define SSBI_REG_ADDR_IRQ_M_STATUS4 (SSBI_REG_ADDR_IRQ_BASE + 4)
> -#define SSBI_REG_ADDR_IRQ_BLK_SEL (SSBI_REG_ADDR_IRQ_BASE + 5)
> -#define SSBI_REG_ADDR_IRQ_IT_STATUS (SSBI_REG_ADDR_IRQ_BASE + 6)
> -#define SSBI_REG_ADDR_IRQ_CONFIG (SSBI_REG_ADDR_IRQ_BASE + 7)
> -#define SSBI_REG_ADDR_IRQ_RT_STATUS (SSBI_REG_ADDR_IRQ_BASE + 8)
Keep these (per argumentation that follows), but try to name them
appropriately.
> +#define SSBI_PM8821_REG_ADDR_IRQ_BASE 0x100
> +
> +#define SSBI_REG_ADDR_IRQ_ROOT (0)
> +#define SSBI_REG_ADDR_IRQ_M_STATUS1 (1)
> +#define SSBI_REG_ADDR_IRQ_M_STATUS2 (2)
> +#define SSBI_REG_ADDR_IRQ_M_STATUS3 (3)
> +#define SSBI_REG_ADDR_IRQ_M_STATUS4 (4)
> +#define SSBI_REG_ADDR_IRQ_BLK_SEL (5)
> +#define SSBI_REG_ADDR_IRQ_IT_STATUS (6)
> +#define SSBI_REG_ADDR_IRQ_CONFIG (7)
> +#define SSBI_REG_ADDR_IRQ_RT_STATUS (8)
Unnecessary parenthesis.
> +
> +#define PM8821_TOTAL_IRQ_MASTERS 2
Unused.
> +#define PM8821_BLOCKS_PER_MASTER 7
> +#define PM8821_IRQ_MASTER1_SET 0x01
BIT(0), but I would prefer that you just inline this with a comment.
> +#define PM8821_IRQ_CLEAR_OFFSET 0x01
Rather than having a single define for this and add in the base and
block numbers I think you should split it into a master0 and master1
define. (And it's not a offset as much as a register)
> +#define PM8821_IRQ_RT_STATUS_OFFSET 0x0f
Dito
> +#define PM8821_IRQ_MASK_REG_OFFSET 0x08
Dito
> +#define SSBI_REG_ADDR_IRQ_MASTER0 0x30
> +#define SSBI_REG_ADDR_IRQ_MASTER1 0xb0
Fold these two into the registers above.
>
> #define PM_IRQF_LVL_SEL 0x01 /* level select */
> #define PM_IRQF_MASK_FE 0x02 /* mask falling edge */
> @@ -54,30 +64,41 @@
> #define REG_HWREV_2 0x0E8 /* PMIC4 revision 2 */
>
> #define PM8921_NR_IRQS 256
> +#define PM8821_NR_IRQS 112
>
> struct pm_irq_chip {
> struct regmap *regmap;
> spinlock_t pm_irq_lock;
> struct irq_domain *irqdomain;
> + unsigned int irq_reg_base;
> unsigned int num_irqs;
> unsigned int num_blocks;
> unsigned int num_masters;
> u8 config[0];
> };
>
> +struct pm8xxx_data {
> + int num_irqs;
> + unsigned int irq_reg_base;
As far as I can see this is always SSBI_PM8821_REG_ADDR_IRQ_BASE in the
8821 functions and SSBI_REG_ADDR_IRQ_BASE in the pm8xxx functions. If
you have disjunct code paths I think it's better to not obscure this
with a variable.
Try renaming the constants appropriately instead. This also has the
benefit of reducing the size of the patch slightly.
> + const struct irq_domain_ops *irq_domain_ops;
> + void (*irq_handler)(struct irq_desc *desc);
> +};
> +
> static int pm8xxx_read_block_irq(struct pm_irq_chip *chip, unsigned int bp,
> unsigned int *ip)
> {
> int rc;
>
> spin_lock(&chip->pm_irq_lock);
> - rc = regmap_write(chip->regmap, SSBI_REG_ADDR_IRQ_BLK_SEL, bp);
> + rc = regmap_write(chip->regmap,
> + chip->irq_reg_base + SSBI_REG_ADDR_IRQ_BLK_SEL, bp);
> if (rc) {
> pr_err("Failed Selecting Block %d rc=%d\n", bp, rc);
> goto bail;
> }
>
> - rc = regmap_read(chip->regmap, SSBI_REG_ADDR_IRQ_IT_STATUS, ip);
> + rc = regmap_read(chip->regmap,
> + chip->irq_reg_base + SSBI_REG_ADDR_IRQ_IT_STATUS, ip);
> if (rc)
> pr_err("Failed Reading Status rc=%d\n", rc);
> bail:
> @@ -91,14 +112,16 @@ pm8xxx_config_irq(struct pm_irq_chip *chip, unsigned int bp, unsigned int cp)
> int rc;
>
> spin_lock(&chip->pm_irq_lock);
> - rc = regmap_write(chip->regmap, SSBI_REG_ADDR_IRQ_BLK_SEL, bp);
> + rc = regmap_write(chip->regmap,
> + chip->irq_reg_base + SSBI_REG_ADDR_IRQ_BLK_SEL, bp);
> if (rc) {
> pr_err("Failed Selecting Block %d rc=%d\n", bp, rc);
> goto bail;
> }
>
> cp |= PM_IRQF_WRITE;
> - rc = regmap_write(chip->regmap, SSBI_REG_ADDR_IRQ_CONFIG, cp);
> + rc = regmap_write(chip->regmap,
> + chip->irq_reg_base + SSBI_REG_ADDR_IRQ_CONFIG, cp);
> if (rc)
> pr_err("Failed Configuring IRQ rc=%d\n", rc);
> bail:
> @@ -137,8 +160,8 @@ static int pm8xxx_irq_master_handler(struct pm_irq_chip *chip, int master)
> unsigned int blockbits;
> int block_number, i, ret = 0;
>
> - ret = regmap_read(chip->regmap, SSBI_REG_ADDR_IRQ_M_STATUS1 + master,
> - &blockbits);
> + ret = regmap_read(chip->regmap, chip->irq_reg_base +
> + SSBI_REG_ADDR_IRQ_M_STATUS1 + master, &blockbits);
> if (ret) {
> pr_err("Failed to read master %d ret=%d\n", master, ret);
> return ret;
> @@ -165,7 +188,8 @@ static void pm8xxx_irq_handler(struct irq_desc *desc)
>
> chained_irq_enter(irq_chip, desc);
>
> - ret = regmap_read(chip->regmap, SSBI_REG_ADDR_IRQ_ROOT, &root);
> + ret = regmap_read(chip->regmap,
> + chip->irq_reg_base + SSBI_REG_ADDR_IRQ_ROOT, &root);
> if (ret) {
> pr_err("Can't read root status ret=%d\n", ret);
> return;
> @@ -182,6 +206,122 @@ static void pm8xxx_irq_handler(struct irq_desc *desc)
> chained_irq_exit(irq_chip, desc);
> }
>
> +static int pm8821_read_master_irq(const struct pm_irq_chip *chip,
> + int m, unsigned int *master)
> +{
I think you should inline this, as you already have the calls unrolled
in pm8821_irq_handler().
> + unsigned int base;
> +
> + if (!m)
> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
> + else
> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
> +
> + return regmap_read(chip->regmap, base, master);
> +}
> +
> +static int pm8821_read_block_irq(struct pm_irq_chip *chip, int master,
> + u8 block, unsigned int *bits)
> +{
> + int rc;
> +
> + unsigned int base;
Odd empty line between rc and base. (And btw, sorting your local
variables in descending length make things pretty).
> +
> + if (!master)
> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
> + else
> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
> +
> + spin_lock(&chip->pm_irq_lock);
The reason why this is done under a lock in the other case is because
the status register is paged, so you shouldn't need it here.
With this updated I think you can favorably inline this into
pm8821_irq_block_handler().
> +
> + rc = regmap_read(chip->regmap, base + block, bits);
> + if (rc)
> + pr_err("Failed Reading Status rc=%d\n", rc);
> +
> + spin_unlock(&chip->pm_irq_lock);
> + return rc;
> +}
> +
> +static int pm8821_irq_block_handler(struct pm_irq_chip *chip,
> + int master_number, int block)
> +{
> + int pmirq, irq, i, ret;
> + unsigned int bits;
> +
> + ret = pm8821_read_block_irq(chip, master_number, block, &bits);
> + if (ret) {
> + pr_err("Failed reading %d block ret=%d", block, ret);
> + return ret;
> + }
> + if (!bits) {
> + pr_err("block bit set in master but no irqs: %d", block);
> + return 0;
> + }
> +
> + /* Convert block offset to global block number */
> + block += (master_number * PM8821_BLOCKS_PER_MASTER) - 1;
So this is block -= 1 for master 0 and block += 6 for master 1, is the
latter correct?
> +
> + /* Check IRQ bits */
> + for (i = 0; i < 8; i++) {
> + if (bits & BIT(i)) {
> + pmirq = block * 8 + i;
> + irq = irq_find_mapping(chip->irqdomain, pmirq);
> + generic_handle_irq(irq);
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int pm8821_irq_read_master(struct pm_irq_chip *chip,
> + int master_number, u8 master_val)
This isn't so much a matter of "reading master X" as "handle master X".
Also, you don't care about the return value, so no need to return one...
> +{
> + int ret = 0;
> + int block;
> +
> + for (block = 1; block < 8; block++) {
> + if (master_val & BIT(block)) {
> + ret |= pm8821_irq_block_handler(chip,
> + master_number, block);
> + }
> + }
> +
> + return ret;
> +}
> +
> +static void pm8821_irq_handler(struct irq_desc *desc)
> +{
> + struct pm_irq_chip *chip = irq_desc_get_handler_data(desc);
> + struct irq_chip *irq_chip = irq_desc_get_chip(desc);
> + int ret;
> + unsigned int master;
> +
> + chained_irq_enter(irq_chip, desc);
> + /* check master 0 */
> + ret = pm8821_read_master_irq(chip, 0, &master);
> + if (ret) {
> + pr_err("Failed to re:Qad master 0 ret=%d\n", ret);
> + return;
> + }
> +
> + if (master & ~PM8821_IRQ_MASTER1_SET)
Rather than having a define for MASTER1_SET use BIT(0) here and write a
comment like:
"bits 1 through 7 marks the first 7 blocks"
> + pm8821_irq_read_master(chip, 0, master);
> +
and then
"bit 0 is set if second master contains any bits"
Or just skip this optimization and check the two masters unconditionally
in a loop.
> + /* check master 1 */
> + if (!(master & PM8821_IRQ_MASTER1_SET))
> + goto done;
> +
> + ret = pm8821_read_master_irq(chip, 1, &master);
> + if (ret) {
> + pr_err("Failed to read master 1 ret=%d\n", ret);
> + return;
> + }
> +
> + pm8821_irq_read_master(chip, 1, master);
> +
> +done:
> + chained_irq_exit(irq_chip, desc);
> +}
> +
> static void pm8xxx_irq_mask_ack(struct irq_data *d)
> {
> struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
> @@ -254,13 +394,15 @@ static int pm8xxx_irq_get_irqchip_state(struct irq_data *d,
> irq_bit = pmirq % 8;
>
> spin_lock(&chip->pm_irq_lock);
> - rc = regmap_write(chip->regmap, SSBI_REG_ADDR_IRQ_BLK_SEL, block);
> + rc = regmap_write(chip->regmap, chip->irq_reg_base +
> + SSBI_REG_ADDR_IRQ_BLK_SEL, block);
> if (rc) {
> pr_err("Failed Selecting Block %d rc=%d\n", block, rc);
> goto bail;
> }
>
> - rc = regmap_read(chip->regmap, SSBI_REG_ADDR_IRQ_RT_STATUS, &bits);
> + rc = regmap_read(chip->regmap, chip->irq_reg_base +
> + SSBI_REG_ADDR_IRQ_RT_STATUS, &bits);
> if (rc) {
> pr_err("Failed Reading Status rc=%d\n", rc);
> goto bail;
> @@ -299,6 +441,151 @@ static const struct irq_domain_ops pm8xxx_irq_domain_ops = {
> .map = pm8xxx_irq_domain_map,
> };
>
> +static void pm8821_irq_mask_ack(struct irq_data *d)
> +{
> + struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
> + unsigned int base, pmirq = irqd_to_hwirq(d);
> + u8 block, master;
> + int irq_bit, rc;
> +
> + block = pmirq / 8;
> + master = block / PM8821_BLOCKS_PER_MASTER;
> + irq_bit = pmirq % 8;
> + block %= PM8821_BLOCKS_PER_MASTER;
You can deobfuscate this somewhat by instead of testing for !master
below you just do:
if (block < PM8821_BLOCKS_PER_MASTER) {
base =
} else {
base =
block -= PM8821_BLOCKS_PER_MASTER;
}
> +
> + if (!master)
> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
> + else
> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
> +
> + spin_lock(&chip->pm_irq_lock);
The irqchip code grabs a lock on the irq_desc, so this can't race with
unmask - and the regmap_update_bits() is internally protecting the
read/write cycle.
So you shouldn't need to lock around this section.
> + rc = regmap_update_bits(chip->regmap,
> + base + PM8821_IRQ_MASK_REG_OFFSET + block,
> + BIT(irq_bit), BIT(irq_bit));
> +
> + if (rc) {
> + pr_err("Failed to read/write mask IRQ:%d rc=%d\n", pmirq, rc);
> + goto fail;
> + }
> +
> + rc = regmap_update_bits(chip->regmap,
> + base + PM8821_IRQ_CLEAR_OFFSET + block,
> + BIT(irq_bit), BIT(irq_bit));
> +
> + if (rc) {
> + pr_err("Failed to read/write IT_CLEAR IRQ:%d rc=%d\n",
> + pmirq, rc);
> + }
> +
> +fail:
> + spin_unlock(&chip->pm_irq_lock);
> +}
> +
> +static void pm8821_irq_unmask(struct irq_data *d)
> +{
> + struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
> + unsigned int base, pmirq = irqd_to_hwirq(d);
> + int irq_bit, rc;
> + u8 block, master;
> +
> + block = pmirq / 8;
> + master = block / PM8821_BLOCKS_PER_MASTER;
> + irq_bit = pmirq % 8;
> + block %= PM8821_BLOCKS_PER_MASTER;
As mask().
> +
> + if (!master)
> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
> + else
> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
> +
> + spin_lock(&chip->pm_irq_lock);
As mask().
> +
> + rc = regmap_update_bits(chip->regmap,
> + base + PM8821_IRQ_MASK_REG_OFFSET + block,
> + BIT(irq_bit), ~BIT(irq_bit));
> +
> + if (rc)
> + pr_err("Failed to read/write unmask IRQ:%d rc=%d\n", pmirq, rc);
> +
> + spin_unlock(&chip->pm_irq_lock);
> +}
> +
> +static int pm8821_irq_set_type(struct irq_data *d, unsigned int flow_type)
> +{
> +
> + /*
> + * PM8821 IRQ controller does not have explicit software support for
> + * IRQ flow type.
> + */
Is returning "success" here the right thing to do? Shouldn't we just
omit the function? Or did you perhaps hit some clients that wouldn't
deal with that?
> + return 0;
> +}
> +
> +static int pm8821_irq_get_irqchip_state(struct irq_data *d,
> + enum irqchip_irq_state which,
> + bool *state)
> +{
> + struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
> + int pmirq, rc;
> + u8 block, irq_bit, master;
> + unsigned int bits;
> + unsigned int base;
> + unsigned long flags;
> +
> + pmirq = irqd_to_hwirq(d);
> +
> + block = pmirq / 8;
> + master = block / PM8821_BLOCKS_PER_MASTER;
> + irq_bit = pmirq % 8;
> + block %= PM8821_BLOCKS_PER_MASTER;
> +
Simplify as in mask().
> + if (!master)
> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER0;
> + else
> + base = chip->irq_reg_base + SSBI_REG_ADDR_IRQ_MASTER1;
> +
> + spin_lock_irqsave(&chip->pm_irq_lock, flags);
No need to lock here as we're just reading a single register.
> +
> + rc = regmap_read(chip->regmap,
> + base + PM8821_IRQ_RT_STATUS_OFFSET + block, &bits);
> + if (rc) {
> + pr_err("Failed Reading Status rc=%d\n", rc);
> + goto bail_out;
> + }
> +
> + *state = !!(bits & BIT(irq_bit));
> +
> +bail_out:
> + spin_unlock_irqrestore(&chip->pm_irq_lock, flags);
> +
> + return rc;
> +}
> +
> +static struct irq_chip pm8821_irq_chip = {
> + .name = "pm8821",
> + .irq_mask_ack = pm8821_irq_mask_ack,
> + .irq_unmask = pm8821_irq_unmask,
> + .irq_set_type = pm8821_irq_set_type,
> + .irq_get_irqchip_state = pm8821_irq_get_irqchip_state,
> + .flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE,
> +};
> +
Regards,
Bjorn
^ permalink raw reply
* Re: [PATCH] gpio: tegra186: Add support for T186 GPIO
From: Stephen Warren @ 2016-11-08 19:07 UTC (permalink / raw)
To: Suresh Mangipudi
Cc: ldewangan-DDmLM1+adcrQT0dZR+AlfA,
linus.walleij-QSEj5FYQhm4dnm+yROfE0A,
gnurou-Re5JQEeQqe8AvxtiuMwx3w,
thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-gpio-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1478083719-14836-1-git-send-email-smangipudi-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
On 11/02/2016 04:48 AM, Suresh Mangipudi wrote:
> Add GPIO driver for T186 based platforms.
> Adds support for MAIN and AON GPIO's from T186.
I'm not sure how you/Thierry will approach merging this with the other
GPIO driver he has, but here's a very quick review of this one in case
it's useful.
> diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
> +#define TEGRA_MAIN_GPIO_PORT_INFO(port, cid, cind, npins) \
A comment indicating what "cid" and "cind" mean (and perhaps the other
parameters too) would be useful.
A brief description of the overall register layout and structure and
differences between the MAIN/AON controllers would be useful.
> +[TEGRA_MAIN_GPIO_PORT_##port] = { \
> + .port_name = #port, \
> + .cont_id = cid, \
> + .port_index = cind, \
Why not make the parameter names match the field names they're assigned to?
> + .valid_pins = npins, \
> + .scr_offset = cid * 0x1000 + cind * 0x40, \
> + .reg_offset = cid * 0x1000 + cind * 0x200, \
While C does define operator precedence rules that make that expression
OK, I personally prefer using () to make it explict:
+ .reg_offset = (cid * 0x1000) + (cind * 0x200), \
That way, the reader doesn't have to think/remember so much.
Also, if these values can be calculated based on .cont_id and
.port_index, I wonder why we need to pre-calculate them here and/or what
else could be pre-calculated from .cont_id/.port_index? I'm tend to
either (a) just store .cont_id and .port_index and calculate everything
from them always, or (b) store just derived data and not both storing
.cont_id/.port_index.
> +static struct tegra_gpio_port_soc_info tegra_main_gpio_cinfo[] = {
> + TEGRA_MAIN_GPIO_PORT_INFO(A, 2, 0, 7),
> + TEGRA_MAIN_GPIO_PORT_INFO(B, 3, 0, 7),
I assume the entries in this file must be in the same order as the DT
binding port IDs? A comment to that effect would be useful.
> +struct tegra_gpio_info;
> +
> +struct tegra_gpio_soc_info {
> + const char *name;
> + const struct tegra_gpio_port_soc_info *port;
> + int nports;
> +};
This isn't information about an SoC; it's information about a
controller, and there are 2 controllers within Tegra186. Rename to
tegra_gpio_ctlr_info?
> +struct tegra_gpio_controller {
> + int controller;
> + int irq;
> + struct tegra_gpio_info *tgi;
> +};
> +
> +struct tegra_gpio_info {
Is this structure per-bank/-port? Also, "info" seems to be used above
for static configuration info/data. I think this should be called
"tegra_gpio_port"?
> + struct device *dev;
> + int nbanks;
> + void __iomem *gpio_regs;
> + void __iomem *scr_regs;
> + struct irq_domain *irq_domain;
> + const struct tegra_gpio_soc_info *soc;
> + struct tegra_gpio_controller tg_contrlr[MAX_GPIO_CONTROLLERS];
> + struct gpio_chip gc;
> + struct irq_chip ic;
> +};
> +#define GPIO_CNTRL_REG(tgi, gpio, roffset) \
> + ((tgi)->gpio_regs + (tgi)->soc->port[GPIO_PORT(gpio)].reg_offset + \
> + (GPIO_REG_DIFF * GPIO_PIN(gpio)) + (roffset))
Writing a static inline function would make formatting and type safety
easier.
> +static void tegra_gpio_update(struct tegra_gpio_info *tgi, u32 gpio,
> + u32 reg_offset, u32 mask, u32 val)
> +{
> + u32 rval;
> +
> + rval = __raw_readl(GPIO_CNTRL_REG(tgi, gpio, reg_offset));
> + rval = (rval & ~mask) | (val & mask);
> + __raw_writel(rval, GPIO_CNTRL_REG(tgi, gpio, reg_offset));
> +}
If you use a regmap object rather than a raw MMIO pointer, I believe
there's already a function that does this read-modify-write.
> +/* This function will return if the GPIO is accessible by CPU */
> +static bool gpio_is_accessible(struct tegra_gpio_info *tgi, u32 offset)
I'd prefer all functions use the same name prefix. "tegra_gpio" seems to
be used so far. Actually, given there's already an existing Tegra GPIO
driver, and this driver covers the new controller(s) in Tegra186, I'd
prefer everything be named tegra186_gpio_xxx.
> + if (cont_id < 0)
> + return false;
That should trigger a checkpatch error due to the presence of two spaces
in the expression. Try running checkpatch and fixing any issues.
> + val = __raw_readl(tgi->scr_regs + scr_offset +
> + (pin * GPIO_SCR_DIFF) + GPIO_SCR_REG);
> +
> + if ((val & GPIO_FULL_ACCESS) == GPIO_FULL_ACCESS)
> + return true;
I'm not entirely convinced about this. It's possible to have only read
or only write access. I believe the CPU can be assigned to an arbitrary
bus master group, whereas the value of GPIO_FULL_ACCESS assumes it's on
group 1. Do we actually need this function at all except for debug? I'd
be tempted to just drop it and let all GPIO accesses be attempted. If
the SCR is configured such that the CPU doesn't have access, writes
should be ignored and reads return valid dummy data. That seems fine to me.
Also, this function isn't consistently used by all client-callable APIs.
I'd expect it to be used from every function that's accessible via a
function pointer in struct gpio_chip, if it's useful.
> +static void tegra_gpio_set(struct gpio_chip *chip, unsigned int offset,
> + int value)
> + tegra_gpio_writel(tgi, val, offset, GPIO_OUT_VAL_REG);
> + tegra_gpio_writel(tgi, 0, offset, GPIO_OUT_CTRL_REG);
Shouldn't this function *just* set the output value; any other setup
should be done solely as part of direction_input/direction_output?
> +static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> + tegra_gpio_enable(ctrlr->tgi, gpio);
Shouldn't this only happen when the client actually calls enable/disable?
> + if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
> + irq_set_handler_locked(d, handle_level_irq);
> + else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
> + irq_set_handler_locked(d, handle_edge_irq);
Shouldn't the handler be set before the IRQ is enabled?
> +static void tegra_gpio_irq_handler(struct irq_desc *desc)
> + for (i = 0; i < MAX_GPIO_PORTS; ++i)
> + port_map[i] = -1;
> +
> + for (i = 0; i < tgi->soc->nports; ++i) {
> + if (tgi->soc->port[i].cont_id == tg_cont->controller)
> + port_map[tgi->soc->port[i].port_index] = i;
> + }
I would have expected the code to use simple math here (iterate over all
ports in the controller) rather than creating some kind of
lookup/mapping table.
> + chained_irq_enter(chip, desc);
> + for (i = 0; i < MAX_GPIO_PORTS; i++) {
> + port = port_map[i];
> + if (port == -1)
> + continue;
> +
> + addr = tgi->soc->port[port].reg_offset;
> + val = __raw_readl(tg_cont->tgi->gpio_regs + addr +
> + GPIO_INT_STATUS_OFFSET + GPIO_STATUS_G1);
> + gpio = tgi->gc.base + (port * 8);
> + for_each_set_bit(pin, &val, 8)
> + generic_handle_irq(gpio_to_irq(gpio + pin));
For edge-sensitive IRQs, doesn't the status need to be cleared before
calling the handler, so that (a) the latched status is cleared, (b) if a
new edge occurs after this point, that fact is recorded and the new IRQ
handled?
> +#ifdef CONFIG_DEBUG_FS
Using a regmap might give you some of the debug logic for free.
> +static int tegra_gpio_probe(struct platform_device *pdev)
> +{
> + struct tegra_gpio_info *tgi;
> + struct resource *res;
> + int bank;
> + int gpio;
> + int ret;
> +
> + for (bank = 0;; bank++) {
> + res = platform_get_resource(pdev, IORESOURCE_IRQ, bank);
> + if (!res)
> + break;
> + }
> + if (!bank) {
> + dev_err(&pdev->dev, "No GPIO Controller found\n");
> + return -ENODEV;
> + }
...
> + tgi->nbanks = bank;
There should be a fixed number of IRQs in DT based on the controller
definition; the value shouldn't be variable.
See Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt
The U-Boot Tegra186 GPIO driver might also be useful as a reference to
how to use the DT binding and structure the driver:
> http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/gpio/tegra186_gpio.c;h=1c681514db9f50e61a9db041ac2067c084db494c;hb=HEAD
> + tgi->gc.ngpio = tgi->soc->nports * 8;
This will leave some gaps in the GPIO numbering, since not all ports
have 8 GPIOs. I think this is the correct thing to do, but IIRC Thierry
found this caused some issues in the GPIO core since it attempts to
query initial status of each GPIO. Did you see this issue during testing?
> +static int __init tegra_gpio_init(void)
> +{
> + return platform_driver_register(&tegra_gpio_driver);
> +}
> +postcore_initcall(tegra_gpio_init);
I would have expected everything to use module_initcall() these days.
^ permalink raw reply
* Re: [PATCH] gpio: tegra186: Add support for T186 GPIO
From: Stephen Warren @ 2016-11-08 19:07 UTC (permalink / raw)
To: Suresh Mangipudi
Cc: ldewangan, linus.walleij, gnurou, thierry.reding, linux-kernel,
linux-gpio, linux-tegra
In-Reply-To: <1478083719-14836-1-git-send-email-smangipudi@nvidia.com>
On 11/02/2016 04:48 AM, Suresh Mangipudi wrote:
> Add GPIO driver for T186 based platforms.
> Adds support for MAIN and AON GPIO's from T186.
I'm not sure how you/Thierry will approach merging this with the other
GPIO driver he has, but here's a very quick review of this one in case
it's useful.
> diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
> +#define TEGRA_MAIN_GPIO_PORT_INFO(port, cid, cind, npins) \
A comment indicating what "cid" and "cind" mean (and perhaps the other
parameters too) would be useful.
A brief description of the overall register layout and structure and
differences between the MAIN/AON controllers would be useful.
> +[TEGRA_MAIN_GPIO_PORT_##port] = { \
> + .port_name = #port, \
> + .cont_id = cid, \
> + .port_index = cind, \
Why not make the parameter names match the field names they're assigned to?
> + .valid_pins = npins, \
> + .scr_offset = cid * 0x1000 + cind * 0x40, \
> + .reg_offset = cid * 0x1000 + cind * 0x200, \
While C does define operator precedence rules that make that expression
OK, I personally prefer using () to make it explict:
+ .reg_offset = (cid * 0x1000) + (cind * 0x200), \
That way, the reader doesn't have to think/remember so much.
Also, if these values can be calculated based on .cont_id and
.port_index, I wonder why we need to pre-calculate them here and/or what
else could be pre-calculated from .cont_id/.port_index? I'm tend to
either (a) just store .cont_id and .port_index and calculate everything
from them always, or (b) store just derived data and not both storing
.cont_id/.port_index.
> +static struct tegra_gpio_port_soc_info tegra_main_gpio_cinfo[] = {
> + TEGRA_MAIN_GPIO_PORT_INFO(A, 2, 0, 7),
> + TEGRA_MAIN_GPIO_PORT_INFO(B, 3, 0, 7),
I assume the entries in this file must be in the same order as the DT
binding port IDs? A comment to that effect would be useful.
> +struct tegra_gpio_info;
> +
> +struct tegra_gpio_soc_info {
> + const char *name;
> + const struct tegra_gpio_port_soc_info *port;
> + int nports;
> +};
This isn't information about an SoC; it's information about a
controller, and there are 2 controllers within Tegra186. Rename to
tegra_gpio_ctlr_info?
> +struct tegra_gpio_controller {
> + int controller;
> + int irq;
> + struct tegra_gpio_info *tgi;
> +};
> +
> +struct tegra_gpio_info {
Is this structure per-bank/-port? Also, "info" seems to be used above
for static configuration info/data. I think this should be called
"tegra_gpio_port"?
> + struct device *dev;
> + int nbanks;
> + void __iomem *gpio_regs;
> + void __iomem *scr_regs;
> + struct irq_domain *irq_domain;
> + const struct tegra_gpio_soc_info *soc;
> + struct tegra_gpio_controller tg_contrlr[MAX_GPIO_CONTROLLERS];
> + struct gpio_chip gc;
> + struct irq_chip ic;
> +};
> +#define GPIO_CNTRL_REG(tgi, gpio, roffset) \
> + ((tgi)->gpio_regs + (tgi)->soc->port[GPIO_PORT(gpio)].reg_offset + \
> + (GPIO_REG_DIFF * GPIO_PIN(gpio)) + (roffset))
Writing a static inline function would make formatting and type safety
easier.
> +static void tegra_gpio_update(struct tegra_gpio_info *tgi, u32 gpio,
> + u32 reg_offset, u32 mask, u32 val)
> +{
> + u32 rval;
> +
> + rval = __raw_readl(GPIO_CNTRL_REG(tgi, gpio, reg_offset));
> + rval = (rval & ~mask) | (val & mask);
> + __raw_writel(rval, GPIO_CNTRL_REG(tgi, gpio, reg_offset));
> +}
If you use a regmap object rather than a raw MMIO pointer, I believe
there's already a function that does this read-modify-write.
> +/* This function will return if the GPIO is accessible by CPU */
> +static bool gpio_is_accessible(struct tegra_gpio_info *tgi, u32 offset)
I'd prefer all functions use the same name prefix. "tegra_gpio" seems to
be used so far. Actually, given there's already an existing Tegra GPIO
driver, and this driver covers the new controller(s) in Tegra186, I'd
prefer everything be named tegra186_gpio_xxx.
> + if (cont_id < 0)
> + return false;
That should trigger a checkpatch error due to the presence of two spaces
in the expression. Try running checkpatch and fixing any issues.
> + val = __raw_readl(tgi->scr_regs + scr_offset +
> + (pin * GPIO_SCR_DIFF) + GPIO_SCR_REG);
> +
> + if ((val & GPIO_FULL_ACCESS) == GPIO_FULL_ACCESS)
> + return true;
I'm not entirely convinced about this. It's possible to have only read
or only write access. I believe the CPU can be assigned to an arbitrary
bus master group, whereas the value of GPIO_FULL_ACCESS assumes it's on
group 1. Do we actually need this function at all except for debug? I'd
be tempted to just drop it and let all GPIO accesses be attempted. If
the SCR is configured such that the CPU doesn't have access, writes
should be ignored and reads return valid dummy data. That seems fine to me.
Also, this function isn't consistently used by all client-callable APIs.
I'd expect it to be used from every function that's accessible via a
function pointer in struct gpio_chip, if it's useful.
> +static void tegra_gpio_set(struct gpio_chip *chip, unsigned int offset,
> + int value)
> + tegra_gpio_writel(tgi, val, offset, GPIO_OUT_VAL_REG);
> + tegra_gpio_writel(tgi, 0, offset, GPIO_OUT_CTRL_REG);
Shouldn't this function *just* set the output value; any other setup
should be done solely as part of direction_input/direction_output?
> +static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> + tegra_gpio_enable(ctrlr->tgi, gpio);
Shouldn't this only happen when the client actually calls enable/disable?
> + if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
> + irq_set_handler_locked(d, handle_level_irq);
> + else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
> + irq_set_handler_locked(d, handle_edge_irq);
Shouldn't the handler be set before the IRQ is enabled?
> +static void tegra_gpio_irq_handler(struct irq_desc *desc)
> + for (i = 0; i < MAX_GPIO_PORTS; ++i)
> + port_map[i] = -1;
> +
> + for (i = 0; i < tgi->soc->nports; ++i) {
> + if (tgi->soc->port[i].cont_id == tg_cont->controller)
> + port_map[tgi->soc->port[i].port_index] = i;
> + }
I would have expected the code to use simple math here (iterate over all
ports in the controller) rather than creating some kind of
lookup/mapping table.
> + chained_irq_enter(chip, desc);
> + for (i = 0; i < MAX_GPIO_PORTS; i++) {
> + port = port_map[i];
> + if (port == -1)
> + continue;
> +
> + addr = tgi->soc->port[port].reg_offset;
> + val = __raw_readl(tg_cont->tgi->gpio_regs + addr +
> + GPIO_INT_STATUS_OFFSET + GPIO_STATUS_G1);
> + gpio = tgi->gc.base + (port * 8);
> + for_each_set_bit(pin, &val, 8)
> + generic_handle_irq(gpio_to_irq(gpio + pin));
For edge-sensitive IRQs, doesn't the status need to be cleared before
calling the handler, so that (a) the latched status is cleared, (b) if a
new edge occurs after this point, that fact is recorded and the new IRQ
handled?
> +#ifdef CONFIG_DEBUG_FS
Using a regmap might give you some of the debug logic for free.
> +static int tegra_gpio_probe(struct platform_device *pdev)
> +{
> + struct tegra_gpio_info *tgi;
> + struct resource *res;
> + int bank;
> + int gpio;
> + int ret;
> +
> + for (bank = 0;; bank++) {
> + res = platform_get_resource(pdev, IORESOURCE_IRQ, bank);
> + if (!res)
> + break;
> + }
> + if (!bank) {
> + dev_err(&pdev->dev, "No GPIO Controller found\n");
> + return -ENODEV;
> + }
...
> + tgi->nbanks = bank;
There should be a fixed number of IRQs in DT based on the controller
definition; the value shouldn't be variable.
See Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt
The U-Boot Tegra186 GPIO driver might also be useful as a reference to
how to use the DT binding and structure the driver:
> http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/gpio/tegra186_gpio.c;h=1c681514db9f50e61a9db041ac2067c084db494c;hb=HEAD
> + tgi->gc.ngpio = tgi->soc->nports * 8;
This will leave some gaps in the GPIO numbering, since not all ports
have 8 GPIOs. I think this is the correct thing to do, but IIRC Thierry
found this caused some issues in the GPIO core since it attempts to
query initial status of each GPIO. Did you see this issue during testing?
> +static int __init tegra_gpio_init(void)
> +{
> + return platform_driver_register(&tegra_gpio_driver);
> +}
> +postcore_initcall(tegra_gpio_init);
I would have expected everything to use module_initcall() these days.
^ permalink raw reply
* Re: [PATCH v1] ufs: introduce setup_hibern8 callback
From: Subhash Jadavani @ 2016-11-08 19:08 UTC (permalink / raw)
To: Kiwoong Kim
Cc: James E.J. Bottomley, linux-scsi, Martin K. Petersen,
vinholikatti, 추헌광, linux-scsi-owner
In-Reply-To: <001f01d23994$719997c0$54ccc740$@samsung.com>
On 2016-11-07 23:48, Kiwoong Kim wrote:
> Some UFS host controller may need to configure some things
> around hibern8 enter/exit
>
> Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
> ---
> drivers/scsi/ufs/ufshcd.c | 10 ++++++++--
> drivers/scsi/ufs/ufshcd.h | 10 ++++++++++
> 2 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index fdb0502..d4a5a9c 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2697,6 +2697,8 @@ static int __ufshcd_uic_hibern8_enter(struct
> ufs_hba *hba)
> int ret;
> struct uic_command uic_cmd = {0};
>
> + ufshcd_vops_setup_hibern8(hba, true, PRE_CHANGE);
> +
> uic_cmd.command = UIC_CMD_DME_HIBER_ENTER;
> ret = ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
>
> @@ -2710,7 +2712,8 @@ static int __ufshcd_uic_hibern8_enter(struct
> ufs_hba *hba)
> */
> if (ufshcd_link_recovery(hba))
> ret = -ENOLINK;
> - }
> + } else
> + ufshcd_vops_setup_hibern8(hba, true, POST_CHANGE);
>
> return ret;
> }
> @@ -2733,13 +2736,16 @@ static int ufshcd_uic_hibern8_exit(struct
> ufs_hba *hba)
> struct uic_command uic_cmd = {0};
> int ret;
>
> + ufshcd_vops_setup_hibern8(hba, false, PRE_CHANGE);
> +
> uic_cmd.command = UIC_CMD_DME_HIBER_EXIT;
> ret = ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
> if (ret) {
> dev_err(hba->dev, "%s: hibern8 exit failed. ret = %d\n",
> __func__, ret);
> ret = ufshcd_link_recovery(hba);
> - }
> + } else
> + ufshcd_vops_setup_hibern8(hba, false, POST_CHANGE);
>
> return ret;
> }
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index b084d89..13504b4 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -265,6 +265,8 @@ struct ufs_pwr_mode_info {
> * to set some things
> * @setup_task_mgmt: called before any task management request is
> issued
> * to set some things
> + * @setup_hibern8: called around hibern8 enter/exit
> + * to configure some things
> * @suspend: called during host controller PM callback
> * @resume: called during host controller PM callback
> * @dbg_register_dump: used to dump controller debug information
> @@ -290,6 +292,7 @@ struct ufs_hba_variant_ops {
> struct ufs_pa_layer_attr *);
> void (*setup_xfer_req)(struct ufs_hba *, int, bool);
> void (*setup_task_mgmt)(struct ufs_hba *, int, u8);
> + void (*setup_hibern8)(struct ufs_hba *, bool, bool);
Can we change the name to "hibern8_notify" ? You may check other
ufs_hba_variant_ops for reference.
> int (*suspend)(struct ufs_hba *, enum ufs_pm_op);
> int (*resume)(struct ufs_hba *, enum ufs_pm_op);
> void (*dbg_register_dump)(struct ufs_hba *hba);
> @@ -821,6 +824,13 @@ static inline void
> ufshcd_vops_setup_task_mgmt(struct ufs_hba *hba,
> return hba->vops->setup_task_mgmt(hba, tag, tm_function);
> }
>
> +static inline void ufshcd_vops_setup_hibern8(struct ufs_hba *hba,
> + bool enter, bool notify)
Using bool to specify whether it is hibern8 enter or hibern8 exit
doesn't seem to be readable. May be you can pass the
UIC_CMD_DME_HIBER_ENTER or UIC_CMD_DME_HIBER_EXIT (in other words use
"enum uic_cmd_dme" type).
also "notify" type should be changed from "bool" to "enum
ufs_notify_change_status".
> +{
> + if (hba->vops && hba->vops->setup_hibern8)
> + return hba->vops->setup_hibern8(hba, enter, notify);
> +}
> +
> static inline int ufshcd_vops_suspend(struct ufs_hba *hba, enum
> ufs_pm_op op)
> {
> if (hba->vops && hba->vops->suspend)
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v4 1/3] leds: Introduce userspace leds driver
From: David Lechner @ 2016-11-08 19:08 UTC (permalink / raw)
To: Jacek Anaszewski, Richard Purdie
Cc: linux-kernel, linux-leds, Marcel Holtmann, Pavel Machek
In-Reply-To: <0bfdfd46-35e5-321c-5759-0cc96be862fd@samsung.com>
On 11/8/16 5:26 AM, Jacek Anaszewski wrote:
> Hi David,
>
>> +struct uleds_device {
>> + struct uleds_user_dev user_dev;
>> + struct led_classdev led_cdev;
>> + struct mutex mutex;
>> + enum uleds_state state;
>> + wait_queue_head_t waitq;
>> + unsigned char brightness;
>
> I've just noticed that this is wrong, since LED subsystem
> brightness type is enum led_brightness, i.e. int.
> LED_FULL (255) value is a legacy enum value that can be overridden
> by max_brightness property.
>
> Please submit a fix so that I could merge it with the original
> patch before sending it upstream.
>
> Thanks,
> Jacek Anaszewski
>
The brightness should be a 32-bit integer then?
^ permalink raw reply
* Re: [PATCH net-next v4] cadence: Add LSO support.
From: Florian Fainelli @ 2016-11-08 19:09 UTC (permalink / raw)
To: Rafal Ozieblo, nicolas.ferre, netdev, linux-kernel
In-Reply-To: <1478612463-15076-1-git-send-email-rafalo@cadence.com>
On 11/08/2016 05:41 AM, Rafal Ozieblo wrote:
> New Cadence GEM hardware support Large Segment Offload (LSO):
> TCP segmentation offload (TSO) as well as UDP fragmentation
> offload (UFO). Support for those features was added to the driver.
>
> Signed-off-by: Rafal Ozieblo <rafalo@cadence.com>
> ---
> -#define MACB_MAX_TX_LEN ((unsigned int)((1 << MACB_TX_FRMLEN_SIZE) - 1))
> -#define GEM_MAX_TX_LEN ((unsigned int)((1 << GEM_TX_FRMLEN_SIZE) - 1))
> +/* Max length of transmit frame must be a multiple of 8 bytes */
> +#define MACB_TX_LEN_ALIGN 8
> +#define MACB_MAX_TX_LEN ((unsigned int)((1 << MACB_TX_FRMLEN_SIZE) - 1) & ~((unsigned int)(MACB_TX_LEN_ALIGN - 1)))
> +#define GEM_MAX_TX_LEN ((unsigned int)((1 << GEM_TX_FRMLEN_SIZE) - 1) & ~((unsigned int)(MACB_TX_LEN_ALIGN - 1)))
>
> #define GEM_MTU_MIN_SIZE ETH_MIN_MTU
> +#define MACB_NETIF_LSO (NETIF_F_TSO | NETIF_F_UFO)
Not a huge fan of this definition, since it is always used in conjuction
with netdev_features_t, having it expanded all the time is kind of nicer
for the reader, but this is just personal preference here.
>
> #define MACB_WOL_HAS_MAGIC_PACKET (0x1 << 0)
> #define MACB_WOL_ENABLED (0x1 << 1)
> @@ -1223,7 +1228,8 @@ static void macb_poll_controller(struct net_device *dev)
>
> static unsigned int macb_tx_map(struct macb *bp,
> struct macb_queue *queue,
> - struct sk_buff *skb)
> + struct sk_buff *skb,
> + unsigned int hdrlen)
> {
> dma_addr_t mapping;
> unsigned int len, entry, i, tx_head = queue->tx_head;
> @@ -1231,14 +1237,27 @@ static unsigned int macb_tx_map(struct macb *bp,
> struct macb_dma_desc *desc;
> unsigned int offset, size, count = 0;
> unsigned int f, nr_frags = skb_shinfo(skb)->nr_frags;
> - unsigned int eof = 1;
> - u32 ctrl;
> + unsigned int eof = 1, mss_mfs = 0;
> + u32 ctrl, lso_ctrl = 0, seq_ctrl = 0;
> +
> + /* LSO */
> + if (skb_shinfo(skb)->gso_size != 0) {
> + if (IPPROTO_UDP == (ip_hdr(skb)->protocol))
Most checks are usually done the other way with the left and right
member swapped.
> + /* UDP - UFO */
> + lso_ctrl = MACB_LSO_UFO_ENABLE;
> + else
> + /* TCP - TSO */
> + lso_ctrl = MACB_LSO_TSO_ENABLE;
> + }
>
> /* Then, map paged data from fragments */
> @@ -1311,6 +1332,20 @@ static unsigned int macb_tx_map(struct macb *bp,
> desc = &queue->tx_ring[entry];
> desc->ctrl = ctrl;
>
> + if (lso_ctrl) {
> + if (lso_ctrl == MACB_LSO_UFO_ENABLE)
> + /* include header and FCS in value given to h/w */
> + mss_mfs = skb_shinfo(skb)->gso_size +
> + skb_transport_offset(skb) + 4;
ETH_FCS_LEN instead of 4?
> +static netdev_features_t macb_features_check(struct sk_buff *skb,
> + struct net_device *dev,
> + netdev_features_t features)
> +{
> + unsigned int nr_frags, f;
> + unsigned int hdrlen;
> +
> + /* Validate LSO compatibility */
> +
> + /* there is only one buffer */
> + if (!skb_is_nonlinear(skb))
> + return features;
> +
> + /* length of header */
> + hdrlen = skb_transport_offset(skb);
> + if (IPPROTO_TCP == (ip_hdr(skb)->protocol))
> + hdrlen += tcp_hdrlen(skb);
Same here, please reverse the left and right members, no need for
parenthesis aground ip_hdr(skb)->protocol.
> +
> + /* For LSO:
> + * When software supplies two or more payload buffers all payload buffers
> + * apart from the last must be a multiple of 8 bytes in size.
> + */
> + if (!IS_ALIGNED(skb_headlen(skb) - hdrlen, MACB_TX_LEN_ALIGN))
> + return features & ~MACB_NETIF_LSO;
> +
> + nr_frags = skb_shinfo(skb)->nr_frags;
> + /* No need to check last fragment */
> + nr_frags--;
> + for (f = 0; f < nr_frags; f++) {
> + const skb_frag_t *frag = &skb_shinfo(skb)->frags[f];
> +
> + if (!IS_ALIGNED(skb_frag_size(frag), MACB_TX_LEN_ALIGN))
> + return features & ~MACB_NETIF_LSO;
> + }
> + return features;
> +}
> +
> static inline int macb_clear_csum(struct sk_buff *skb)
> {
> /* no change for packets without checksum offloading */
> @@ -1374,7 +1456,27 @@ static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
> struct macb *bp = netdev_priv(dev);
> struct macb_queue *queue = &bp->queues[queue_index];
> unsigned long flags;
> - unsigned int count, nr_frags, frag_size, f;
> + unsigned int desc_cnt, nr_frags, frag_size, f;
> + unsigned int is_lso = 0, is_udp, hdrlen;
> +
> + is_lso = (skb_shinfo(skb)->gso_size != 0);
> +
> + if (is_lso) {
> + is_udp = (IPPROTO_UDP == (ip_hdr(skb)->protocol));
Same here, and you may want to declare is_udp as boolean and do this:
is_udp = !!(ip_hdr(skb)->protocl == IPPROTO_UDP);
> +
> + /* length of headers */
> + if (is_udp)
> + /* only queue eth + ip headers separately for UDP */
> + hdrlen = skb_transport_offset(skb);
> + else
> + hdrlen = skb_transport_offset(skb) + tcp_hdrlen(skb);
> + if (skb_headlen(skb) < hdrlen) {
> + netdev_err(bp->dev, "Error - LSO headers fragmented!!!\n");
> + /* if this is required, would need to copy to single buffer */
> + return NETDEV_TX_BUSY;
> + }
>
> + if (is_lso) {
> + if (is_udp)
> + /* zero UDP checksum, not calculated by h/w for UFO */
> + udp_hdr(skb)->check = 0;
is_udp is only set when (is_lso) is checked, so the two conditions are
redundant, just checking is_udp should be enough?
--
Florian
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.