* Re: [PATCH v4 2/2] add icswx support
From: Benjamin Herrenschmidt @ 2011-03-04 20:26 UTC (permalink / raw)
To: Tseng-Hui (Frank) Lin; +Cc: tsenglin, linuxppc-dev
In-Reply-To: <1299259794.28840.57.camel@flin.austin.ibm.com>
On Fri, 2011-03-04 at 11:29 -0600, Tseng-Hui (Frank) Lin wrote:
> On Fri, 2011-03-04 at 12:02 +1100, Benjamin Herrenschmidt wrote:
> > On Wed, 2011-03-02 at 11:20 -0600, Tseng-Hui (Frank) Lin wrote:
> >
> > > +#define CPU_FTR_ICSWX LONG_ASM_CONST(0x1000000000000000)
> >
> > Do we want a userspace visible feature as well ? Or some other way to
> > inform userspace that we support icswx ?
> >
> Does a user space program really need to know about icswx? Only
> coprocessor drivers need to know about icswx. Shouldn't user space
> programs talk to the coprocessor drivers instead?
Well, I don't know how you use icswx on P7+, but on Prism it's
definitely issued directly by userspace.
> Thought about that. However, multiple threads can call use_cop() at the
> same time. Without the spinlock being setup in advance, how do I
> guarantee allocating struct copro_data and modifying the pointer in the
> mm_context to be atomic?
You don't need to. You allocate and initialize the structure, and you
compare & swap the pointer. If somebody beat you, you trash your copy.
> > I'm not sure I totally get the point of having an ifdef here. Can't you
> > make it unconditional ? Or do you expect distros to turn that off in
> > which case what's the point ?
> >
> There is only one coprocessor, HFI, using icswx at this moment. The lazy
> switching makes sense. However, in the future, if more types of
> coprocessors are added, the lazy switching may actually be a bad idea.
> This option allows users to turn off the lazy switching.
No user in real life plays with kernel config options. Care to explain
why the lazy switching would be a problem ?
> Same concern as above. I need something initialized in advance to
> guarantee allocating memory and updating the pointer are safe when it
> happens in use_cop().
No you don't, see above.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH v4 2/2] add icswx support
From: Tseng-Hui (Frank) Lin @ 2011-03-04 17:29 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: tsenglin, linuxppc-dev
In-Reply-To: <1299200560.8833.869.camel@pasglop>
On Fri, 2011-03-04 at 12:02 +1100, Benjamin Herrenschmidt wrote:
> On Wed, 2011-03-02 at 11:20 -0600, Tseng-Hui (Frank) Lin wrote:
>
> > +#define CPU_FTR_ICSWX LONG_ASM_CONST(0x1000000000000000)
>
> Do we want a userspace visible feature as well ? Or some other way to
> inform userspace that we support icswx ?
>
Does a user space program really need to know about icswx? Only
coprocessor drivers need to know about icswx. Shouldn't user space
programs talk to the coprocessor drivers instead?
> > index acac35d..b0c2549 100644
> > --- a/arch/powerpc/include/asm/mmu-hash64.h
> > +++ b/arch/powerpc/include/asm/mmu-hash64.h
> > @@ -409,6 +409,14 @@ static inline void subpage_prot_init_new_context(struct mm_struct *mm) { }
> >
> > typedef unsigned long mm_context_id_t;
> >
> > +#ifdef CONFIG_ICSWX
>
> CONFIG_PPC_ICSWX please.
>
Will change.
> > +/*
> > + * Use forward declearation to avoid including linux/spinlock_types.h
> > + * which breaks kernel build.
> > + */
> > +struct spinlock;
> > +#endif /* CONFIG_ICSWX */
> > +
>
> Yuck. Instead put all your fields into a structure called something
> like struct copro_data, make that a fwd declaration and stick a pointer
> to it in the mm_context.
>
> It then only need to be allocated for processes that try to use copros,
> and the definition of that structure can remain local to whatever header
> you have dedicated for that.
>
Thought about that. However, multiple threads can call use_cop() at the
same time. Without the spinlock being setup in advance, how do I
guarantee allocating struct copro_data and modifying the pointer in the
mm_context to be atomic?
> > typedef struct {
> > mm_context_id_t id;
> > u16 user_psize; /* page size index */
> > @@ -423,6 +431,11 @@ typedef struct {
> > #ifdef CONFIG_PPC_SUBPAGE_PROT
> > struct subpage_prot_table spt;
> > #endif /* CONFIG_PPC_SUBPAGE_PROT */
> > +#ifdef CONFIG_ICSWX
> > + struct spinlock *cop_lockp; /* guard acop and cop_pid */
> > + unsigned long acop; /* mask of enabled coprocessor types */
> > + unsigned int cop_pid; /* pid value used with coprocessors */
> > +#endif /* CONFIG_ICSWX */
> > } mm_context_t;
> >
> >
> > diff --git a/arch/powerpc/include/asm/mmu_context.h b/arch/powerpc/include/asm/mmu_context.h
> > index 81fb412..fe0a09a 100644
> > --- a/arch/powerpc/include/asm/mmu_context.h
> > +++ b/arch/powerpc/include/asm/mmu_context.h
> > @@ -32,6 +32,12 @@ extern void __destroy_context(unsigned long context_id);
> > extern void mmu_context_init(void);
> > #endif
> >
> > +#ifdef CONFIG_ICSWX
> > +extern void switch_cop(struct mm_struct *next);
> > +extern int use_cop(unsigned long acop, struct mm_struct *mm);
> > +extern void drop_cop(unsigned long acop, struct mm_struct *mm);
> > +#endif /* CONFIG_ICSWX */
>
> No need to ifdef declarations.
>
> .../...
>
Will change.
> > +
> > +#ifdef CONFIG_ICSWX_LAZY_SWITCH
> > +static DEFINE_PER_CPU(unsigned long, acop_reg);
> > +#define mtspr_acop(val) \
> > + if (__get_cpu_var(acop_reg) != val) { \
> > + get_cpu_var(acop_reg) = val; \
> > + mtspr(SPRN_ACOP, val); \
> > + put_cpu_var(acop_reg); \
> > + }
>
> Why get/put games here since you did a __get in the first place ?
> Doesn't make much sense. This is all inside context switch anyways so
> you just do __ always and don't bother with put.
>
Will change.
> > +#else
> > +#define mtspr_acop(val) mtspr(SPRN_ACOP, val)
> > +#endif /* CONFIG_ICSWX_LAZY_SWITCH */
>
> I'm not sure I totally get the point of having an ifdef here. Can't you
> make it unconditional ? Or do you expect distros to turn that off in
> which case what's the point ?
>
There is only one coprocessor, HFI, using icswx at this moment. The lazy
switching makes sense. However, in the future, if more types of
coprocessors are added, the lazy switching may actually be a bad idea.
This option allows users to turn off the lazy switching.
> > +#define COP_PID_NONE 0
> > +#define COP_PID_MIN (COP_PID_NONE + 1)
> > +#define COP_PID_MAX (0xFFFF)
> > +
> > +static DEFINE_SPINLOCK(mmu_context_acop_lock);
> > +static DEFINE_IDA(cop_ida);
> > +
> > +void switch_cop(struct mm_struct *next)
> > +{
> > + mtspr(SPRN_PID, next->context.cop_pid);
> > + mtspr_acop(next->context.acop);
> > +}
>
> s/mtspr_acop/set_cpu_acop() instead.
>
Will change.
> > +static int new_cop_pid(struct ida *ida, int min_id, int max_id,
> > + spinlock_t *lock)
> > +{
> > + int index;
> > + int err;
> > +
> > +again:
> > + if (!ida_pre_get(ida, GFP_KERNEL))
> > + return -ENOMEM;
> > +
> > + spin_lock(lock);
> > + err = ida_get_new_above(ida, min_id, &index);
> > + spin_unlock(lock);
> > +
> > + if (err == -EAGAIN)
> > + goto again;
> > + else if (err)
> > + return err;
> > +
> > + if (index > max_id) {
> > + spin_lock(lock);
> > + ida_remove(ida, index);
> > + spin_unlock(lock);
> > + return -ENOMEM;
> > + }
> > +
> > + return index;
> > +}
> > +
> > +/*
> > + * Start using a coprocessor.
> > + * @acop: mask of coprocessor to be used.
> > + * @mm: The mm the coprocessor to associate with. Most likely current mm.
> > + *
> > + * Return a positive PID if successful. Negative errno otherwise.
> > + * The returned PID will be fed to the coprocessor to determine if an
> > + * icswx transaction is authenticated.
> > + */
> > +int use_cop(unsigned long acop, struct mm_struct *mm)
> > +{
> > + int cop_pid;
> > +
> > + if (!cpu_has_feature(CPU_FTR_ICSWX))
> > + return -ENODEV;
> > +
> > + if (!mm || !acop)
> > + return -EINVAL;
> > +
> > + spin_lock(mm->context.cop_lockp);
> > + if (mm->context.cop_pid == COP_PID_NONE) {
>
> new_cop_pid goes GFP_KERNEL allocs no ? It even goes at great length to
> drop the mmu_context_acop_lock while doing ide_pre_get() ... but you
> call the whole thing with the mm->context.cop_lockp held. So that's all
> wrong. You need to drop the lock, allocate a PID, take the lock again,
> check if somebody came in and gave you a PID already, if yes, release
> the PID you allocated.
>
> Another option is to use a mutex since none of that is in the context
> switch path right ?
>
I'll take the first option.
> Also do you ever call use_cop for a non-current mm ?
Hmm, no. OK, remove the second parameter, mm.
I wonder if I should change drop_cop() to __drop_cop() and make a new
drop_cop() that calls __drop_cop() with current mm.
> > + cop_pid = new_cop_pid(&cop_ida, COP_PID_MIN, COP_PID_MAX,
> > + &mmu_context_acop_lock);
> > + if (cop_pid < 0) {
> > + spin_unlock(mm->context.cop_lockp);
> > + return cop_pid;
> > + }
> > + mm->context.cop_pid = cop_pid;
> > + if (mm == current->active_mm)
> > + mtspr(SPRN_PID, mm->context.cop_pid);
> > + }
> > + mm->context.acop |= acop;
> > + if (mm == current->active_mm)
> > + mtspr_acop(mm->context.acop);
> > + spin_unlock(mm->context.cop_lockp);
> > + return mm->context.cop_pid;
> > +}
> > +EXPORT_SYMBOL_GPL(use_cop);
> > +
> > +/*
> > + * Stop using a coprocessor.
> > + * @acop: mask of coprocessor to be stopped.
> > + * @mm: The mm the coprocessor associated with.
> > + */
> > +void drop_cop(unsigned long acop, struct mm_struct *mm)
> > +{
> > + if (WARN_ON(!mm))
> > + return;
> > +
> > + spin_lock(mm->context.cop_lockp);
> > + mm->context.acop &= ~acop;
> > + if (mm == current->active_mm)
> > + mtspr_acop(mm->context.acop);
> > + if ((!mm->context.acop) && (mm->context.cop_pid != COP_PID_NONE)) {
> > + spin_lock(&mmu_context_acop_lock);
> > + ida_remove(&cop_ida, mm->context.cop_pid);
> > + spin_unlock(&mmu_context_acop_lock);
> > + mm->context.cop_pid = COP_PID_NONE;
> > + if (mm == current->active_mm)
> > + mtspr(SPRN_PID, mm->context.cop_pid);
> > + }
> > + spin_unlock(mm->context.cop_lockp);
> > +}
> > +EXPORT_SYMBOL_GPL(drop_cop);
> > +
> > +#endif /* CONFIG_ICSWX */
> > +
> > static DEFINE_SPINLOCK(mmu_context_lock);
> > static DEFINE_IDA(mmu_context_ida);
> >
> > @@ -79,6 +245,12 @@ int init_new_context(struct task_struct *tsk, struct mm_struct *mm)
> > slice_set_user_psize(mm, mmu_virtual_psize);
> > subpage_prot_init_new_context(mm);
> > mm->context.id = index;
> > +#ifdef CONFIG_ICSWX
> > + mm->context.cop_lockp = kmalloc(sizeof(spinlock_t), GFP_KERNEL);
> > + if (! mm->context.cop_lockp)
> > + return -ENOMEM;
> > + spin_lock_init(mm->context.cop_lockp);
> > +#endif /* CONFIG_ICSWX */
> >
> > return 0;
> > }
>
> No, do that on the first time a process tries to use it. No need to add
> overhead to every task in the system.
>
Same concern as above. I need something initialized in advance to
guarantee allocating memory and updating the pointer are safe when it
happens in use_cop().
> > @@ -93,6 +265,11 @@ EXPORT_SYMBOL_GPL(__destroy_context);
> >
> > void destroy_context(struct mm_struct *mm)
> > {
> > +#ifdef CONFIG_ICSWX
> > + drop_cop(mm->context.acop, mm);
> > + kfree(mm->context.cop_lockp);
> > + mm->context.cop_lockp = NULL;
> > +#endif /* CONFIG_ICSWX */
> > __destroy_context(mm->context.id);
> > subpage_prot_free(mm);
> > mm->context.id = NO_CONTEXT;
> > diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype
> > index 111138c..0007b66 100644
> > --- a/arch/powerpc/platforms/Kconfig.cputype
> > +++ b/arch/powerpc/platforms/Kconfig.cputype
> > @@ -226,6 +226,49 @@ config VSX
> >
> > If in doubt, say Y here.
> >
> > +config ICSWX
> > + bool "Support for PowerPC icswx coprocessor instruction"
> > + depends on POWER4
> > + default n
> > + ---help---
> > +
> > + Enabling this option to turn on the PowerPC Initiate Coprocessor
> > + Store Word (icswx) coprocessor instruction support for POWER7
> > + or newer processors.
> > +
> > + This option is only useful if you have a processor that supports
> > + icswx coprocessor instruction. It does not have any effect on
> > + processors without icswx coprocessor instruction.
> > +
> > + This option slightly increases kernel memory usage.
> > +
> > + Say N if you do not have a PowerPC processor supporting icswx
> > + instruction and a PowerPC coprocessor.
> > +
> > +config ICSWX_LAZY_SWITCH
> > + bool "Lazy switching coprocessor type registers at context switching"
> > + depends on ICSWX
> > + default y
> > + ---help---
> > +
> > + Coprocessor type register is part of process context. It needs
> > + to be switched at context switching.
> > +
> > + As most machines have a very small number (most likely <= 1)
> > + of coprocessors, there is a good chance that the value of the
> > + coprocessor type register is the same between many processes.
> > + We do not need to change coprocessor type register at context
> > + switching unless the task to switch to has a different value.
> > +
> > + Accessing CPU special purpose registers is far more expensive
> > + than accessing memory. This option uses a per-CPU variable
> > + to track the value of coprocessor type register. The variable
> > + is checked before updating coprocessor type register. The saving
> > + for one access is small but could turn big with the high
> > + frequency of context switching.
> > +
> > + Say Y unless you have multiple coprocessors.
> > +
> > config SPE
> > bool "SPE Support"
> > depends on E200 || (E500 && !PPC_E500MC)
> Ben.
>
Thanks for the comments.
>
^ permalink raw reply
* Re: [PATCH] powerpc: disable MSI using new interface if possible
From: Florian Mickler @ 2011-03-04 11:13 UTC (permalink / raw)
To: Joe Perches; +Cc: Nishanth Aravamudan, linuxppc-dev
In-Reply-To: <1299209359.4338.220.camel@Joe-Laptop>
On Thu, 03 Mar 2011 19:29:19 -0800
Joe Perches <joe@perches.com> wrote:
> > $ git log --pretty=format:"%an %ae" arch/powerpc/platforms/pseries/msi.c
> > Andre Detsch adetsch@br.ibm.com
> > Michael Ellerman michael@ellerman.id.au
> > Michael Ellerman michael@ellerman.id.au
> > Michael Ellerman michael@ellerman.id.au
> > Michael Ellerman michael@ellerman.id.au
> > Michael Ellerman michael@ellerman.id.au
> > Michael Ellerman michael@ellerman.id.au
> > Michael Ellerman michael@ellerman.id.au
> > Michael Ellerman michael@ellerman.id.au
> > Michael Ellerman michael@ellerman.id.au
> > Michael Ellerman michael@ellerman.id.au
> > Michael Ellerman michael@ellerman.id.au
> >
> > And the patch from Andre was also signed off by me.
>
> How nice for you. Last patch from you was
> 2 years ago.
>
The problem is, that it get's pretty slow if you don't do a time
cut-off... i don't know the complexity but i guess it's linear in
(number of files) * (number of commits) * (avg size of diffs) ...
(if you compute the diffstat)
What I'll probably try as soon as I have another few hours to spare
for get_maintainer.pl, is to implement a fall back to older history if
the data basis is not stable enough.
>
> CC'ing inactive non named maintainers via git
> history also draws complaints btw.
>
Yes. I guess, my current version could use an staggered approach where
it does more digging if the results are too volatile... but first
try to be fast ...
Regards,
Flo
^ permalink raw reply
* Re: [PATCH] powerpc: disable MSI using new interface if possible
From: Nishanth Aravamudan @ 2011-03-04 7:24 UTC (permalink / raw)
To: Michael Ellerman; +Cc: paulus, linuxppc-dev, miltonm
In-Reply-To: <1299207684.3630.76.camel@concordia>
On 04.03.2011 [14:01:24 +1100], Michael Ellerman wrote:
> On Thu, 2011-03-03 at 17:41 -0800, Nishanth Aravamudan wrote:
> > On 04.03.2011 [12:05:29 +1100], Michael Ellerman wrote:
> > > On Thu, 2011-03-03 at 11:39 -0800, Nishanth Aravamudan wrote:
> > > > On upcoming hardware, we have a PCI adapter with two functions, one of
> > > > which uses MSI and the other uses MSI-X. This adapter, when MSI is
> > > > disabled using the "old" firmware interface (RTAS_CHANGE_FN), still
> > > > signals an MSI-X interrupt and triggers an EEH. We are working with the
> > > > vendor to ensure that the hardware is not at fault, but if we use the
> > > > "new" interface (RTAS_CHANGE_MSI_FN) to disable MSI, we also
> > > > automatically disable MSI-X and the adapter does not appear to signal
> > > > any stray MSI-X interrupt.
> > >
> > > It seems this could also be a firmware bug, have we heard anything from
> > > them? PAPR explicitly says that RTAS_CHANGE_FN (function=1) should
> > > disable MSI _and_ MSI-X (R1???7.3.10.5.1???1).
> >
> > We're tracking that down too. I think the fact that the interrupt is
> > coming in is a hardware bug in this particular adapter.
> >
> > I'm looking at PAPR again and I see what might be a contradiction:
> >
> > 7.3.10.5.1: "To removing all MSIs, set the Requested Number of
> > Interrupts to zero."
> >
> > Table 71: "Function ... 1: Request to set to a new number of MSI or
> > MSI-X (platform choice) interrupts (including set to 0)"
> >
> > It seems like the Table claims that using RTAS_CHANGE_FN with 0, could
> > change only MSI or MSI-X and still be not a bug?
>
> Yeah I guess you could read it that way, though I think that would be a
> bug.
>
> The idea is that it chooses for you whether it uses MSI or MSI-X. So the
> only sane semantic is that when deconfiguring it deconfigures either,
> ie. both, kinds.
I agree with you that is how it should be :) I'm asking the firmware
folks to make sure I'm not misunderstanding the underlying issue.
> Looking closer at your patch, now I don't understand :)
>
> + /*
> + * disabling MSI with the explicit interface also disables MSI-X
> + */
> + if (rtas_change_msi(pdn, RTAS_CHANGE_MSI_FN, 0) != 0) {
>
>
> So we first disable using function 3, which should:
>
> 3: Request to set to a new number of MSI interrupts (including set to 0)
>
> Which does not mention MSI-X at all, implying it has no effect on them.
> Which contradicts what you see, and the comment in the code?
Thanks for the thorough review!
Per PAPR 2.4 from Power.org, look at the page before that table, page
169:
"Specifying Function 3 (MSI) also disables MSI-X for the specified IOA
function, and likewise specifying Function 4 (MSI-X) disables MSI for
the IOA function....Specifying the Requested Number of Interrupts to
zero for either Function 3 or 4 removes all MSI & MSI-X interrupts from
the IOA function."
So I'm relying on this aspect of PAPR being enforced by the firmware,
which I think it is in my testing.
Thanks,
Nish
^ permalink raw reply
* Re: [PATCH] powerpc: disable MSI using new interface if possible
From: Joe Perches @ 2011-03-04 3:29 UTC (permalink / raw)
To: michael; +Cc: Nishanth Aravamudan, linuxppc-dev, Florian Mickler
In-Reply-To: <1299208016.3630.81.camel@concordia>
On Fri, 2011-03-04 at 14:06 +1100, Michael Ellerman wrote:
> On Thu, 2011-03-03 at 17:41 -0800, Nishanth Aravamudan wrote:
> > On 04.03.2011 [12:05:29 +1100], Michael Ellerman wrote:
> > > Cc: Me :)
> > Sorry! I was in a hurry to get this out the door, my fault. Note, you
> > don't show up per scripts/get_maintainer.pl :)
> No worries, though I will remember never to use get_maintainer.pl, it is
> obviously utterly broken:
Blah, blah, stupid tool doesn't work exactly as I want,
and it doesn't credit me for my over 2 year old patches,
therefore it's not only stupid, it's broken...
$ ./scripts/get_maintainer.pl -f arch/powerpc/platforms/pseries/msi.c
Benjamin Herrenschmidt <benh@kernel.crashing.org> (supporter:LINUX FOR POWERPC...)
Paul Mackerras <paulus@samba.org> (supporter:LINUX FOR POWERPC...)
Grant Likely <grant.likely@secretlab.ca> (maintainer:OPEN FIRMWARE AND...)
linuxppc-dev@lists.ozlabs.org (open list:LINUX FOR POWERPC...)
linux-kernel@vger.kernel.org (open list)
devicetree-discuss@lists.ozlabs.org (open list:OPEN FIRMWARE AND...)
If you don't like how it currently works, suggest
improvements.
> $ git log --pretty=format:"%an %ae" arch/powerpc/platforms/pseries/msi.c
> Andre Detsch adetsch@br.ibm.com
> Michael Ellerman michael@ellerman.id.au
> Michael Ellerman michael@ellerman.id.au
> Michael Ellerman michael@ellerman.id.au
> Michael Ellerman michael@ellerman.id.au
> Michael Ellerman michael@ellerman.id.au
> Michael Ellerman michael@ellerman.id.au
> Michael Ellerman michael@ellerman.id.au
> Michael Ellerman michael@ellerman.id.au
> Michael Ellerman michael@ellerman.id.au
> Michael Ellerman michael@ellerman.id.au
> Michael Ellerman michael@ellerman.id.au
>
> And the patch from Andre was also signed off by me.
How nice for you. Last patch from you was
2 years ago.
$ git log --format="%an %ae %ad" arch/powerpc/platforms/pseries/msi.c
Andre Detsch adetsch@br.ibm.com Wed Nov 4 13:03:19 2009 -0200
Michael Ellerman michael@ellerman.id.au Thu Mar 5 14:44:26 2009 +0000
Michael Ellerman michael@ellerman.id.au Tue Feb 17 00:21:56 2009 +0000
Michael Ellerman michael@ellerman.id.au Tue Feb 17 00:18:49 2009 +0000
Michael Ellerman michael@ellerman.id.au Thu Jan 22 20:54:33 2009 +0000
Michael Ellerman michael@ellerman.id.au Thu Jan 22 20:54:32 2009 +0000
Michael Ellerman michael@ellerman.id.au Thu Jan 22 20:54:31 2009 +0000
Michael Ellerman michael@ellerman.id.au Thu Jan 22 20:54:31 2009 +0000
Michael Ellerman michael@ellerman.id.au Tue Oct 23 14:23:44 2007 +1000
Michael Ellerman michael@ellerman.id.au Thu Sep 20 16:36:50 2007 +1000
Michael Ellerman michael@ellerman.id.au Thu Sep 20 16:36:48 2007 +1000
Michael Ellerman michael@ellerman.id.au Tue May 8 12:58:35 2007 +1000
CC'ing inactive non named maintainers via git
history also draws complaints btw.
^ permalink raw reply
* Re: [PATCH] powerpc: disable MSI using new interface if possible
From: Michael Ellerman @ 2011-03-04 3:06 UTC (permalink / raw)
To: Nishanth Aravamudan; +Cc: Joe Russell, paulus, linuxppc-dev, miltonm
In-Reply-To: <1299202862-10682-1-git-send-email-nacc@us.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 1357 bytes --]
On Thu, 2011-03-03 at 17:41 -0800, Nishanth Aravamudan wrote:
> On 04.03.2011 [12:05:29 +1100], Michael Ellerman wrote:
> > On Thu, 2011-03-03 at 11:39 -0800, Nishanth Aravamudan wrote:
> > > On upcoming hardware, we have a PCI adapter with two functions, one of
..
> > > Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
> > > Cc: Milton Miller <miltonm@bga.com>
> > > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > > Cc: Paul Mackerras <paulus@samba.org>
> >
> > Cc: Me :)
>
> Sorry! I was in a hurry to get this out the door, my fault. Note, you
> don't show up per scripts/get_maintainer.pl :)
No worries, though I will remember never to use get_maintainer.pl, it is
obviously utterly broken:
$ git log --pretty=format:"%an %ae" arch/powerpc/platforms/pseries/msi.c
Andre Detsch adetsch@br.ibm.com
Michael Ellerman michael@ellerman.id.au
Michael Ellerman michael@ellerman.id.au
Michael Ellerman michael@ellerman.id.au
Michael Ellerman michael@ellerman.id.au
Michael Ellerman michael@ellerman.id.au
Michael Ellerman michael@ellerman.id.au
Michael Ellerman michael@ellerman.id.au
Michael Ellerman michael@ellerman.id.au
Michael Ellerman michael@ellerman.id.au
Michael Ellerman michael@ellerman.id.au
Michael Ellerman michael@ellerman.id.au
And the patch from Andre was also signed off by me.
cheers
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCH] powerpc: disable MSI using new interface if possible
From: Michael Ellerman @ 2011-03-04 3:01 UTC (permalink / raw)
To: Nishanth Aravamudan; +Cc: paulus, linuxppc-dev, miltonm
In-Reply-To: <1299202862-10682-1-git-send-email-nacc@us.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 2336 bytes --]
On Thu, 2011-03-03 at 17:41 -0800, Nishanth Aravamudan wrote:
> On 04.03.2011 [12:05:29 +1100], Michael Ellerman wrote:
> > On Thu, 2011-03-03 at 11:39 -0800, Nishanth Aravamudan wrote:
> > > On upcoming hardware, we have a PCI adapter with two functions, one of
> > > which uses MSI and the other uses MSI-X. This adapter, when MSI is
> > > disabled using the "old" firmware interface (RTAS_CHANGE_FN), still
> > > signals an MSI-X interrupt and triggers an EEH. We are working with the
> > > vendor to ensure that the hardware is not at fault, but if we use the
> > > "new" interface (RTAS_CHANGE_MSI_FN) to disable MSI, we also
> > > automatically disable MSI-X and the adapter does not appear to signal
> > > any stray MSI-X interrupt.
> >
> > It seems this could also be a firmware bug, have we heard anything from
> > them? PAPR explicitly says that RTAS_CHANGE_FN (function=1) should
> > disable MSI _and_ MSI-X (R1???7.3.10.5.1???1).
>
> We're tracking that down too. I think the fact that the interrupt is
> coming in is a hardware bug in this particular adapter.
>
> I'm looking at PAPR again and I see what might be a contradiction:
>
> 7.3.10.5.1: "To removing all MSIs, set the Requested Number of
> Interrupts to zero."
>
> Table 71: "Function ... 1: Request to set to a new number of MSI or
> MSI-X (platform choice) interrupts (including set to 0)"
>
> It seems like the Table claims that using RTAS_CHANGE_FN with 0, could
> change only MSI or MSI-X and still be not a bug?
Yeah I guess you could read it that way, though I think that would be a
bug.
The idea is that it chooses for you whether it uses MSI or MSI-X. So the
only sane semantic is that when deconfiguring it deconfigures either,
ie. both, kinds.
Looking closer at your patch, now I don't understand :)
+ /*
+ * disabling MSI with the explicit interface also disables MSI-X
+ */
+ if (rtas_change_msi(pdn, RTAS_CHANGE_MSI_FN, 0) != 0) {
So we first disable using function 3, which should:
3: Request to set to a new number of MSI interrupts (including set to 0)
Which does not mention MSI-X at all, implying it has no effect on them.
Which contradicts what you see, and the comment in the code?
So I think I'm not sure what's going on here :)
cheers
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* [PATCH] powerpc: disable MSI using new interface if possible
From: Nishanth Aravamudan @ 2011-03-04 1:41 UTC (permalink / raw)
To: Michael Ellerman; +Cc: paulus, linuxppc-dev, miltonm
In-Reply-To: <1299200729.3630.52.camel@concordia>
On 04.03.2011 [12:05:29 +1100], Michael Ellerman wrote:
> On Thu, 2011-03-03 at 11:39 -0800, Nishanth Aravamudan wrote:
> > On upcoming hardware, we have a PCI adapter with two functions, one of
> > which uses MSI and the other uses MSI-X. This adapter, when MSI is
> > disabled using the "old" firmware interface (RTAS_CHANGE_FN), still
> > signals an MSI-X interrupt and triggers an EEH. We are working with the
> > vendor to ensure that the hardware is not at fault, but if we use the
> > "new" interface (RTAS_CHANGE_MSI_FN) to disable MSI, we also
> > automatically disable MSI-X and the adapter does not appear to signal
> > any stray MSI-X interrupt.
>
> It seems this could also be a firmware bug, have we heard anything from
> them? PAPR explicitly says that RTAS_CHANGE_FN (function=1) should
> disable MSI _and_ MSI-X (R1???7.3.10.5.1???1).
We're tracking that down too. I think the fact that the interrupt is
coming in is a hardware bug in this particular adapter.
I'm looking at PAPR again and I see what might be a contradiction:
7.3.10.5.1: "To removing all MSIs, set the Requested Number of
Interrupts to zero."
Table 71: "Function ... 1: Request to set to a new number of MSI or
MSI-X (platform choice) interrupts (including set to 0)"
It seems like the Table claims that using RTAS_CHANGE_FN with 0, could
change only MSI or MSI-X and still be not a bug?
It does seem like it should disable both, but from what we've seen with
this adapter, it doesn't appear to.
> > Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
> > Cc: Milton Miller <miltonm@bga.com>
> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Cc: Paul Mackerras <paulus@samba.org>
>
> Cc: Me :)
Sorry! I was in a hurry to get this out the door, my fault. Note, you
don't show up per scripts/get_maintainer.pl :)
> > diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
> > index 1164c34..9434576 100644
> > --- a/arch/powerpc/platforms/pseries/msi.c
> > +++ b/arch/powerpc/platforms/pseries/msi.c
> > @@ -93,8 +93,18 @@ static void rtas_disable_msi(struct pci_dev *pdev)
> > if (!pdn)
> > return;
> >
> > - if (rtas_change_msi(pdn, RTAS_CHANGE_FN, 0) != 0)
> > - pr_debug("rtas_msi: Setting MSIs to 0 failed!\n");
> > + /*
> > + * disabling MSI with the explicit interface also disables MSI-X
> > + */
> > + if (rtas_change_msi(pdn, RTAS_CHANGE_MSI_FN, 0) != 0) {
> > + /*
> > + * may have failed due to lacking
> > + * "ibm,change-msix-capable" property
> > + */
> > + if (rtas_change_msi(pdn, RTAS_CHANGE_FN, 0) != 0) {
> > + pr_debug("rtas_msi: Setting MSIs to 0 failed!\n");
> > + }
> > + }
> > }
>
> This is probably a pretty safe change anyway, ie. use the newer API if
> it is present. The comment is backward though, the call fails because
> the new interface is not implemented, and that fact is signalled by the
> absence of the property.
Updated the comment, thanks:
On upcoming hardware, we have a PCI adapter with two functions, one of
which uses MSI and the other uses MSI-X. This adapter, when MSI is
disabled using the "old" firmware interface (RTAS_CHANGE_FN), still
signals an MSI-X interrupt and triggers an EEH. We are working with the
vendor to ensure that the hardware is not at fault, but if we use the
"new" interface (RTAS_CHANGE_MSI_FN) to disable MSI, we also
automatically disable MSI-X and the adapter does not appear to signal
any stray MSI-X interrupt.
Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
---
arch/powerpc/platforms/pseries/msi.c | 14 ++++++++++++--
1 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
index 1164c34..18ac801 100644
--- a/arch/powerpc/platforms/pseries/msi.c
+++ b/arch/powerpc/platforms/pseries/msi.c
@@ -93,8 +93,18 @@ static void rtas_disable_msi(struct pci_dev *pdev)
if (!pdn)
return;
- if (rtas_change_msi(pdn, RTAS_CHANGE_FN, 0) != 0)
- pr_debug("rtas_msi: Setting MSIs to 0 failed!\n");
+ /*
+ * disabling MSI with the explicit interface also disables MSI-X
+ */
+ if (rtas_change_msi(pdn, RTAS_CHANGE_MSI_FN, 0) != 0) {
+ /*
+ * may have failed because explicit interface is not
+ * present
+ */
+ if (rtas_change_msi(pdn, RTAS_CHANGE_FN, 0) != 0) {
+ pr_debug("rtas_msi: Setting MSIs to 0 failed!\n");
+ }
+ }
}
static int rtas_query_irq_number(struct pci_dn *pdn, int offset)
--
1.7.1
^ permalink raw reply related
* Re: [PATCH 1/2] powerpc: disable MSI using new interface if possible
From: Michael Ellerman @ 2011-03-04 1:05 UTC (permalink / raw)
To: Nishanth Aravamudan; +Cc: linuxppc-dev
In-Reply-To: <1299181164-28432-1-git-send-email-nacc@us.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 2076 bytes --]
On Thu, 2011-03-03 at 11:39 -0800, Nishanth Aravamudan wrote:
> On upcoming hardware, we have a PCI adapter with two functions, one of
> which uses MSI and the other uses MSI-X. This adapter, when MSI is
> disabled using the "old" firmware interface (RTAS_CHANGE_FN), still
> signals an MSI-X interrupt and triggers an EEH. We are working with the
> vendor to ensure that the hardware is not at fault, but if we use the
> "new" interface (RTAS_CHANGE_MSI_FN) to disable MSI, we also
> automatically disable MSI-X and the adapter does not appear to signal
> any stray MSI-X interrupt.
It seems this could also be a firmware bug, have we heard anything from
them? PAPR explicitly says that RTAS_CHANGE_FN (function=1) should
disable MSI _and_ MSI-X (R1–7.3.10.5.1–1).
> Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
> Cc: Milton Miller <miltonm@bga.com>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
Cc: Me :)
> diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
> index 1164c34..9434576 100644
> --- a/arch/powerpc/platforms/pseries/msi.c
> +++ b/arch/powerpc/platforms/pseries/msi.c
> @@ -93,8 +93,18 @@ static void rtas_disable_msi(struct pci_dev *pdev)
> if (!pdn)
> return;
>
> - if (rtas_change_msi(pdn, RTAS_CHANGE_FN, 0) != 0)
> - pr_debug("rtas_msi: Setting MSIs to 0 failed!\n");
> + /*
> + * disabling MSI with the explicit interface also disables MSI-X
> + */
> + if (rtas_change_msi(pdn, RTAS_CHANGE_MSI_FN, 0) != 0) {
> + /*
> + * may have failed due to lacking
> + * "ibm,change-msix-capable" property
> + */
> + if (rtas_change_msi(pdn, RTAS_CHANGE_FN, 0) != 0) {
> + pr_debug("rtas_msi: Setting MSIs to 0 failed!\n");
> + }
> + }
> }
This is probably a pretty safe change anyway, ie. use the newer API if
it is present. The comment is backward though, the call fails because
the new interface is not implemented, and that fact is signalled by the
absence of the property.
cheers
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCH v4 2/2] add icswx support
From: Benjamin Herrenschmidt @ 2011-03-04 1:02 UTC (permalink / raw)
To: Tseng-Hui (Frank) Lin; +Cc: tsenglin, linuxppc-dev
In-Reply-To: <1299086454.28840.10.camel@flin.austin.ibm.com>
On Wed, 2011-03-02 at 11:20 -0600, Tseng-Hui (Frank) Lin wrote:
> +#define CPU_FTR_ICSWX LONG_ASM_CONST(0x1000000000000000)
Do we want a userspace visible feature as well ? Or some other way to
inform userspace that we support icswx ?
> index acac35d..b0c2549 100644
> --- a/arch/powerpc/include/asm/mmu-hash64.h
> +++ b/arch/powerpc/include/asm/mmu-hash64.h
> @@ -409,6 +409,14 @@ static inline void subpage_prot_init_new_context(struct mm_struct *mm) { }
>
> typedef unsigned long mm_context_id_t;
>
> +#ifdef CONFIG_ICSWX
CONFIG_PPC_ICSWX please.
> +/*
> + * Use forward declearation to avoid including linux/spinlock_types.h
> + * which breaks kernel build.
> + */
> +struct spinlock;
> +#endif /* CONFIG_ICSWX */
> +
Yuck. Instead put all your fields into a structure called something
like struct copro_data, make that a fwd declaration and stick a pointer
to it in the mm_context.
It then only need to be allocated for processes that try to use copros,
and the definition of that structure can remain local to whatever header
you have dedicated for that.
> typedef struct {
> mm_context_id_t id;
> u16 user_psize; /* page size index */
> @@ -423,6 +431,11 @@ typedef struct {
> #ifdef CONFIG_PPC_SUBPAGE_PROT
> struct subpage_prot_table spt;
> #endif /* CONFIG_PPC_SUBPAGE_PROT */
> +#ifdef CONFIG_ICSWX
> + struct spinlock *cop_lockp; /* guard acop and cop_pid */
> + unsigned long acop; /* mask of enabled coprocessor types */
> + unsigned int cop_pid; /* pid value used with coprocessors */
> +#endif /* CONFIG_ICSWX */
> } mm_context_t;
>
>
> diff --git a/arch/powerpc/include/asm/mmu_context.h b/arch/powerpc/include/asm/mmu_context.h
> index 81fb412..fe0a09a 100644
> --- a/arch/powerpc/include/asm/mmu_context.h
> +++ b/arch/powerpc/include/asm/mmu_context.h
> @@ -32,6 +32,12 @@ extern void __destroy_context(unsigned long context_id);
> extern void mmu_context_init(void);
> #endif
>
> +#ifdef CONFIG_ICSWX
> +extern void switch_cop(struct mm_struct *next);
> +extern int use_cop(unsigned long acop, struct mm_struct *mm);
> +extern void drop_cop(unsigned long acop, struct mm_struct *mm);
> +#endif /* CONFIG_ICSWX */
No need to ifdef declarations.
.../...
> +
> +#ifdef CONFIG_ICSWX_LAZY_SWITCH
> +static DEFINE_PER_CPU(unsigned long, acop_reg);
> +#define mtspr_acop(val) \
> + if (__get_cpu_var(acop_reg) != val) { \
> + get_cpu_var(acop_reg) = val; \
> + mtspr(SPRN_ACOP, val); \
> + put_cpu_var(acop_reg); \
> + }
Why get/put games here since you did a __get in the first place ?
Doesn't make much sense. This is all inside context switch anyways so
you just do __ always and don't bother with put.
> +#else
> +#define mtspr_acop(val) mtspr(SPRN_ACOP, val)
> +#endif /* CONFIG_ICSWX_LAZY_SWITCH */
I'm not sure I totally get the point of having an ifdef here. Can't you
make it unconditional ? Or do you expect distros to turn that off in
which case what's the point ?
> +#define COP_PID_NONE 0
> +#define COP_PID_MIN (COP_PID_NONE + 1)
> +#define COP_PID_MAX (0xFFFF)
> +
> +static DEFINE_SPINLOCK(mmu_context_acop_lock);
> +static DEFINE_IDA(cop_ida);
> +
> +void switch_cop(struct mm_struct *next)
> +{
> + mtspr(SPRN_PID, next->context.cop_pid);
> + mtspr_acop(next->context.acop);
> +}
s/mtspr_acop/set_cpu_acop() instead.
> +static int new_cop_pid(struct ida *ida, int min_id, int max_id,
> + spinlock_t *lock)
> +{
> + int index;
> + int err;
> +
> +again:
> + if (!ida_pre_get(ida, GFP_KERNEL))
> + return -ENOMEM;
> +
> + spin_lock(lock);
> + err = ida_get_new_above(ida, min_id, &index);
> + spin_unlock(lock);
> +
> + if (err == -EAGAIN)
> + goto again;
> + else if (err)
> + return err;
> +
> + if (index > max_id) {
> + spin_lock(lock);
> + ida_remove(ida, index);
> + spin_unlock(lock);
> + return -ENOMEM;
> + }
> +
> + return index;
> +}
> +
> +/*
> + * Start using a coprocessor.
> + * @acop: mask of coprocessor to be used.
> + * @mm: The mm the coprocessor to associate with. Most likely current mm.
> + *
> + * Return a positive PID if successful. Negative errno otherwise.
> + * The returned PID will be fed to the coprocessor to determine if an
> + * icswx transaction is authenticated.
> + */
> +int use_cop(unsigned long acop, struct mm_struct *mm)
> +{
> + int cop_pid;
> +
> + if (!cpu_has_feature(CPU_FTR_ICSWX))
> + return -ENODEV;
> +
> + if (!mm || !acop)
> + return -EINVAL;
> +
> + spin_lock(mm->context.cop_lockp);
> + if (mm->context.cop_pid == COP_PID_NONE) {
new_cop_pid goes GFP_KERNEL allocs no ? It even goes at great length to
drop the mmu_context_acop_lock while doing ide_pre_get() ... but you
call the whole thing with the mm->context.cop_lockp held. So that's all
wrong. You need to drop the lock, allocate a PID, take the lock again,
check if somebody came in and gave you a PID already, if yes, release
the PID you allocated.
Another option is to use a mutex since none of that is in the context
switch path right ?
Also do you ever call use_cop for a non-current mm ?
> + cop_pid = new_cop_pid(&cop_ida, COP_PID_MIN, COP_PID_MAX,
> + &mmu_context_acop_lock);
> + if (cop_pid < 0) {
> + spin_unlock(mm->context.cop_lockp);
> + return cop_pid;
> + }
> + mm->context.cop_pid = cop_pid;
> + if (mm == current->active_mm)
> + mtspr(SPRN_PID, mm->context.cop_pid);
> + }
> + mm->context.acop |= acop;
> + if (mm == current->active_mm)
> + mtspr_acop(mm->context.acop);
> + spin_unlock(mm->context.cop_lockp);
> + return mm->context.cop_pid;
> +}
> +EXPORT_SYMBOL_GPL(use_cop);
> +
> +/*
> + * Stop using a coprocessor.
> + * @acop: mask of coprocessor to be stopped.
> + * @mm: The mm the coprocessor associated with.
> + */
> +void drop_cop(unsigned long acop, struct mm_struct *mm)
> +{
> + if (WARN_ON(!mm))
> + return;
> +
> + spin_lock(mm->context.cop_lockp);
> + mm->context.acop &= ~acop;
> + if (mm == current->active_mm)
> + mtspr_acop(mm->context.acop);
> + if ((!mm->context.acop) && (mm->context.cop_pid != COP_PID_NONE)) {
> + spin_lock(&mmu_context_acop_lock);
> + ida_remove(&cop_ida, mm->context.cop_pid);
> + spin_unlock(&mmu_context_acop_lock);
> + mm->context.cop_pid = COP_PID_NONE;
> + if (mm == current->active_mm)
> + mtspr(SPRN_PID, mm->context.cop_pid);
> + }
> + spin_unlock(mm->context.cop_lockp);
> +}
> +EXPORT_SYMBOL_GPL(drop_cop);
> +
> +#endif /* CONFIG_ICSWX */
> +
> static DEFINE_SPINLOCK(mmu_context_lock);
> static DEFINE_IDA(mmu_context_ida);
>
> @@ -79,6 +245,12 @@ int init_new_context(struct task_struct *tsk, struct mm_struct *mm)
> slice_set_user_psize(mm, mmu_virtual_psize);
> subpage_prot_init_new_context(mm);
> mm->context.id = index;
> +#ifdef CONFIG_ICSWX
> + mm->context.cop_lockp = kmalloc(sizeof(spinlock_t), GFP_KERNEL);
> + if (! mm->context.cop_lockp)
> + return -ENOMEM;
> + spin_lock_init(mm->context.cop_lockp);
> +#endif /* CONFIG_ICSWX */
>
> return 0;
> }
No, do that on the first time a process tries to use it. No need to add
overhead to every task in the system.
> @@ -93,6 +265,11 @@ EXPORT_SYMBOL_GPL(__destroy_context);
>
> void destroy_context(struct mm_struct *mm)
> {
> +#ifdef CONFIG_ICSWX
> + drop_cop(mm->context.acop, mm);
> + kfree(mm->context.cop_lockp);
> + mm->context.cop_lockp = NULL;
> +#endif /* CONFIG_ICSWX */
> __destroy_context(mm->context.id);
> subpage_prot_free(mm);
> mm->context.id = NO_CONTEXT;
> diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype
> index 111138c..0007b66 100644
> --- a/arch/powerpc/platforms/Kconfig.cputype
> +++ b/arch/powerpc/platforms/Kconfig.cputype
> @@ -226,6 +226,49 @@ config VSX
>
> If in doubt, say Y here.
>
> +config ICSWX
> + bool "Support for PowerPC icswx coprocessor instruction"
> + depends on POWER4
> + default n
> + ---help---
> +
> + Enabling this option to turn on the PowerPC Initiate Coprocessor
> + Store Word (icswx) coprocessor instruction support for POWER7
> + or newer processors.
> +
> + This option is only useful if you have a processor that supports
> + icswx coprocessor instruction. It does not have any effect on
> + processors without icswx coprocessor instruction.
> +
> + This option slightly increases kernel memory usage.
> +
> + Say N if you do not have a PowerPC processor supporting icswx
> + instruction and a PowerPC coprocessor.
> +
> +config ICSWX_LAZY_SWITCH
> + bool "Lazy switching coprocessor type registers at context switching"
> + depends on ICSWX
> + default y
> + ---help---
> +
> + Coprocessor type register is part of process context. It needs
> + to be switched at context switching.
> +
> + As most machines have a very small number (most likely <= 1)
> + of coprocessors, there is a good chance that the value of the
> + coprocessor type register is the same between many processes.
> + We do not need to change coprocessor type register at context
> + switching unless the task to switch to has a different value.
> +
> + Accessing CPU special purpose registers is far more expensive
> + than accessing memory. This option uses a per-CPU variable
> + to track the value of coprocessor type register. The variable
> + is checked before updating coprocessor type register. The saving
> + for one access is small but could turn big with the high
> + frequency of context switching.
> +
> + Say Y unless you have multiple coprocessors.
> +
> config SPE
> bool "SPE Support"
> depends on E200 || (E500 && !PPC_E500MC)
Ben.
^ permalink raw reply
* Re: [PATCH v4 1/2] add icswx support
From: Benjamin Herrenschmidt @ 2011-03-04 0:43 UTC (permalink / raw)
To: Tseng-Hui (Frank) Lin; +Cc: tsenglin, linuxppc-dev
In-Reply-To: <1299086450.28840.9.camel@flin.austin.ibm.com>
On Wed, 2011-03-02 at 11:20 -0600, Tseng-Hui (Frank) Lin wrote:
> Move SPRN_PID declearations in various locations into one place.
Applied.
> Signed-off-by: Tseng-Hui (Frank) Lin <thlin@linux.vnet.ibm.com>
>
> ---
> arch/powerpc/include/asm/reg.h | 10 ++++++++++
> arch/powerpc/include/asm/reg_booke.h | 3 ---
> 2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
> index 125fc1a..bd0d36e 100644
> --- a/arch/powerpc/include/asm/reg.h
> +++ b/arch/powerpc/include/asm/reg.h
> @@ -170,6 +170,16 @@
> #define SPEFSCR_FRMC 0x00000003 /* Embedded FP rounding mode control */
>
> /* Special Purpose Registers (SPRNs)*/
> +
> +#ifdef CONFIG_40x
> +#define SPRN_PID 0x3B1 /* Process ID */
> +#else
> +#define SPRN_PID 0x030 /* Process ID */
> +#ifdef CONFIG_BOOKE
> +#define SPRN_PID0 SPRN_PID/* Process ID Register 0 */
> +#endif
> +#endif
> +
> #define SPRN_CTR 0x009 /* Count Register */
> #define SPRN_DSCR 0x11
> #define SPRN_CTRLF 0x088
> diff --git a/arch/powerpc/include/asm/reg_booke.h b/arch/powerpc/include/asm/reg_booke.h
> index e68c69b..86ad812 100644
> --- a/arch/powerpc/include/asm/reg_booke.h
> +++ b/arch/powerpc/include/asm/reg_booke.h
> @@ -150,8 +150,6 @@
> * or IBM 40x.
> */
> #ifdef CONFIG_BOOKE
> -#define SPRN_PID 0x030 /* Process ID */
> -#define SPRN_PID0 SPRN_PID/* Process ID Register 0 */
> #define SPRN_CSRR0 0x03A /* Critical Save and Restore Register 0 */
> #define SPRN_CSRR1 0x03B /* Critical Save and Restore Register 1 */
> #define SPRN_DEAR 0x03D /* Data Error Address Register */
> @@ -168,7 +166,6 @@
> #define SPRN_TCR 0x154 /* Timer Control Register */
> #endif /* Book E */
> #ifdef CONFIG_40x
> -#define SPRN_PID 0x3B1 /* Process ID */
> #define SPRN_DBCR1 0x3BD /* Debug Control Register 1 */
> #define SPRN_ESR 0x3D4 /* Exception Syndrome Register */
> #define SPRN_DEAR 0x3D5 /* Data Error Address Register */
>
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply
* Re: [PATCH v3 7/9] fsldma: support async_tx dependencies and automatic unmapping
From: Dan Williams @ 2011-03-03 20:51 UTC (permalink / raw)
To: Ira W. Snyder; +Cc: Koul, Vinod, linuxppc-dev, linux-kernel
In-Reply-To: <1299174901-16762-8-git-send-email-iws@ovro.caltech.edu>
On Thu, Mar 3, 2011 at 9:54 AM, Ira W. Snyder <iws@ovro.caltech.edu> wrote:
> Previous to this patch, the dma_run_dependencies() function has been
> called while holding desc_lock. This function can call tx_submit() for
> other descriptors, which may try to re-grab the lock. Avoid this by
> moving the descriptors to be cleaned up to a temporary list, and
> dropping the lock before cleanup.
>
> At the same time, add support for automatic unmapping of src and dst
> buffers, as offered by the DMAEngine API.
Unfortunately, this may be a short lived addition as Russell has put
the the kibosh on how the dmaengine api supports dependencies and
automated unmapping [1]. It really needs to be up to the client to
maintain all the mappings until the dma operation is complete. If we
cross dma mapping domains we need to queisce operations, remap the
buffers and submit the dma to the next channel. The current approach
of using overlapping mappings is broken on at least ARM v6+ platforms.
So I need to rework how md raid submits dependencies and manages the
dma mapping api, and will most likely end up removing dependency
support from the api as I do not see a clean way for this to be
automated behind the client's back. Mapping needs to be sole
responsibility of the client.
Other than that this patch looks good.
--
Dan
[1] http://marc.info/?l=linux-raid&m=129407256602759&w=2
^ permalink raw reply
* Re: [PATCH V12 0/4] ptp: IEEE 1588 hardware clock support
From: David Miller @ 2011-03-03 20:13 UTC (permalink / raw)
To: richardcochran
Cc: giometti, arnd, peterz, linux-api, devicetree-discuss,
linux-kernel, linux, paulus, john.stultz, alan, netdev, vapier,
cl, linuxppc-dev, tglx, linux-arm-kernel, khc
In-Reply-To: <cover.1298878618.git.richard.cochran@omicron.at>
From: Richard Cochran <richardcochran@gmail.com>
Date: Mon, 28 Feb 2011 08:57:03 +0100
> This really might be the last review of the PTP hardware clock patch
> series. These patches apply on top of the timers/core branch in the
> tip tree.
I'm find with the networking portions so:
Acked-by: David S. Miller <davem@davemloft.net>
I am assuming these will be pushed through the timers/core branch
since there are dependencies.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/2] powerpc: disable MSI using new interface if possible
From: Nishanth Aravamudan @ 2011-03-03 19:44 UTC (permalink / raw)
To: linuxppc-dev; +Cc: paulus, miltonm
In-Reply-To: <1299181164-28432-1-git-send-email-nacc@us.ibm.com>
On 03.03.2011 [11:39:23 -0800], Nishanth Aravamudan wrote:
> On upcoming hardware, we have a PCI adapter with two functions, one of
> which uses MSI and the other uses MSI-X. This adapter, when MSI is
> disabled using the "old" firmware interface (RTAS_CHANGE_FN), still
> signals an MSI-X interrupt and triggers an EEH. We are working with the
> vendor to ensure that the hardware is not at fault, but if we use the
> "new" interface (RTAS_CHANGE_MSI_FN) to disable MSI, we also
> automatically disable MSI-X and the adapter does not appear to signal
> any stray MSI-X interrupt.
>
> Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
> Cc: Milton Miller <miltonm@bga.com>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
Sorry, I fudged my git-send-email config, apologies that the Cc's above
weren't actually used in the original!
> ---
>
> This is effectively a PCI quirk, but I'm not sure how to do a
> architecture-specific quirk (looking through the code now). If anyone
> has any pointers, I'd appreciate it!
If this would be preferred, I think I see now how it would be done and
can work on a different patch that only makes this change for this
device.
Thanks,
Nish
> arch/powerpc/platforms/pseries/msi.c | 14 ++++++++++++--
> 1 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
> index 1164c34..9434576 100644
> --- a/arch/powerpc/platforms/pseries/msi.c
> +++ b/arch/powerpc/platforms/pseries/msi.c
> @@ -93,8 +93,18 @@ static void rtas_disable_msi(struct pci_dev *pdev)
> if (!pdn)
> return;
>
> - if (rtas_change_msi(pdn, RTAS_CHANGE_FN, 0) != 0)
> - pr_debug("rtas_msi: Setting MSIs to 0 failed!\n");
> + /*
> + * disabling MSI with the explicit interface also disables MSI-X
> + */
> + if (rtas_change_msi(pdn, RTAS_CHANGE_MSI_FN, 0) != 0) {
> + /*
> + * may have failed due to lacking
> + * "ibm,change-msix-capable" property
> + */
> + if (rtas_change_msi(pdn, RTAS_CHANGE_FN, 0) != 0) {
> + pr_debug("rtas_msi: Setting MSIs to 0 failed!\n");
> + }
> + }
> }
>
> static int rtas_query_irq_number(struct pci_dn *pdn, int offset)
> --
> 1.7.1
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
--
Nishanth Aravamudan <nacc@us.ibm.com>
IBM Linux Technology Center
^ permalink raw reply
* [PATCH 1/2] powerpc: disable MSI using new interface if possible
From: Nishanth Aravamudan @ 2011-03-03 19:39 UTC (permalink / raw)
To: linuxppc-dev
On upcoming hardware, we have a PCI adapter with two functions, one of
which uses MSI and the other uses MSI-X. This adapter, when MSI is
disabled using the "old" firmware interface (RTAS_CHANGE_FN), still
signals an MSI-X interrupt and triggers an EEH. We are working with the
vendor to ensure that the hardware is not at fault, but if we use the
"new" interface (RTAS_CHANGE_MSI_FN) to disable MSI, we also
automatically disable MSI-X and the adapter does not appear to signal
any stray MSI-X interrupt.
Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
Cc: Milton Miller <miltonm@bga.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
---
This is effectively a PCI quirk, but I'm not sure how to do a
architecture-specific quirk (looking through the code now). If anyone
has any pointers, I'd appreciate it!
arch/powerpc/platforms/pseries/msi.c | 14 ++++++++++++--
1 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
index 1164c34..9434576 100644
--- a/arch/powerpc/platforms/pseries/msi.c
+++ b/arch/powerpc/platforms/pseries/msi.c
@@ -93,8 +93,18 @@ static void rtas_disable_msi(struct pci_dev *pdev)
if (!pdn)
return;
- if (rtas_change_msi(pdn, RTAS_CHANGE_FN, 0) != 0)
- pr_debug("rtas_msi: Setting MSIs to 0 failed!\n");
+ /*
+ * disabling MSI with the explicit interface also disables MSI-X
+ */
+ if (rtas_change_msi(pdn, RTAS_CHANGE_MSI_FN, 0) != 0) {
+ /*
+ * may have failed due to lacking
+ * "ibm,change-msix-capable" property
+ */
+ if (rtas_change_msi(pdn, RTAS_CHANGE_FN, 0) != 0) {
+ pr_debug("rtas_msi: Setting MSIs to 0 failed!\n");
+ }
+ }
}
static int rtas_query_irq_number(struct pci_dn *pdn, int offset)
--
1.7.1
^ permalink raw reply related
* [PATCH 2/2] powerpc/msi: clarify call to check_req_msi{,x}
From: Nishanth Aravamudan @ 2011-03-03 19:39 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <1299181164-28432-1-git-send-email-nacc@us.ibm.com>
The RTAS IRQ fixup code relies on a less-than-clear edge condition for
verifying a given device is not using MSI or MSI-X. Make that more clear
with a comment.
Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
Cc: Milton Miller <miltonm@bga.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
---
arch/powerpc/platforms/pseries/msi.c | 7 ++++++-
1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
index 9434576..8c6dbbc 100644
--- a/arch/powerpc/platforms/pseries/msi.c
+++ b/arch/powerpc/platforms/pseries/msi.c
@@ -455,7 +455,12 @@ static void rtas_msi_pci_irq_fixup(struct pci_dev *pdev)
return;
}
- /* No MSI -> MSIs can't have been assigned by fw, leave LSI */
+ /*
+ * No MSI -> MSIs can't have been assigned by fw, leave LSI
+ * The calls below can only provide one of two responses:
+ * < 0: if the device doesn't request MSI or MSI-X
+ * 0: if the device requests at least 1 MSI/MSI-X vector
+ */
if (check_req_msi(pdev, 1) && check_req_msix(pdev, 1)) {
dev_dbg(&pdev->dev, "rtas_msi: no req#msi/x, nothing to do.\n");
return;
--
1.7.1
^ permalink raw reply related
* [PATCH v3 8/9] fsldma: reduce locking during descriptor cleanup
From: Ira W. Snyder @ 2011-03-03 17:55 UTC (permalink / raw)
To: linuxppc-dev; +Cc: dan.j.williams, linux-kernel, Ira W. Snyder
In-Reply-To: <1299174901-16762-1-git-send-email-iws@ovro.caltech.edu>
This merges the fsl_chan_ld_cleanup() function into the dma_do_tasklet()
function to reduce locking overhead. In the best case, we will be able
to keep the DMA controller busy while we are freeing used descriptors.
In all cases, the spinlock is grabbed two times fewer than before on
each transaction.
Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
---
drivers/dma/fsldma.c | 108 +++++++++++++++++++++----------------------------
1 files changed, 46 insertions(+), 62 deletions(-)
diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index 526579d..d300de4 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -882,65 +882,15 @@ static void fsldma_cleanup_descriptor(struct fsldma_chan *chan,
}
/**
- * fsl_chan_ld_cleanup - Clean up link descriptors
- * @chan : Freescale DMA channel
- *
- * This function is run after the queue of running descriptors has been
- * executed by the DMA engine. It will run any callbacks, and then free
- * the descriptors.
- *
- * HARDWARE STATE: idle
- */
-static void fsl_chan_ld_cleanup(struct fsldma_chan *chan)
-{
- struct fsl_desc_sw *desc, *_desc;
- LIST_HEAD(ld_cleanup);
- unsigned long flags;
-
- spin_lock_irqsave(&chan->desc_lock, flags);
-
- /* update the cookie if we have some descriptors to cleanup */
- if (!list_empty(&chan->ld_running)) {
- dma_cookie_t cookie;
-
- desc = to_fsl_desc(chan->ld_running.prev);
- cookie = desc->async_tx.cookie;
-
- chan->completed_cookie = cookie;
- chan_dbg(chan, "completed cookie=%d\n", cookie);
- }
-
- /*
- * move the descriptors to a temporary list so we can drop the lock
- * during the entire cleanup operation
- */
- list_splice_tail_init(&chan->ld_running, &ld_cleanup);
-
- spin_unlock_irqrestore(&chan->desc_lock, flags);
-
- /* Run the callback for each descriptor, in order */
- list_for_each_entry_safe(desc, _desc, &ld_cleanup, node) {
-
- /* Remove from the list of transactions */
- list_del(&desc->node);
-
- /* Run all cleanup for this descriptor */
- fsldma_cleanup_descriptor(chan, desc);
- }
-}
-
-/**
* fsl_chan_xfer_ld_queue - transfer any pending transactions
* @chan : Freescale DMA channel
*
* HARDWARE STATE: idle
+ * LOCKING: must hold chan->desc_lock
*/
static void fsl_chan_xfer_ld_queue(struct fsldma_chan *chan)
{
struct fsl_desc_sw *desc;
- unsigned long flags;
-
- spin_lock_irqsave(&chan->desc_lock, flags);
/*
* If the list of pending descriptors is empty, then we
@@ -948,7 +898,7 @@ static void fsl_chan_xfer_ld_queue(struct fsldma_chan *chan)
*/
if (list_empty(&chan->ld_pending)) {
chan_dbg(chan, "no pending LDs\n");
- goto out_unlock;
+ return;
}
/*
@@ -958,7 +908,7 @@ static void fsl_chan_xfer_ld_queue(struct fsldma_chan *chan)
*/
if (!chan->idle) {
chan_dbg(chan, "DMA controller still busy\n");
- goto out_unlock;
+ return;
}
/*
@@ -996,9 +946,6 @@ static void fsl_chan_xfer_ld_queue(struct fsldma_chan *chan)
dma_start(chan);
chan->idle = false;
-
-out_unlock:
- spin_unlock_irqrestore(&chan->desc_lock, flags);
}
/**
@@ -1008,7 +955,11 @@ out_unlock:
static void fsl_dma_memcpy_issue_pending(struct dma_chan *dchan)
{
struct fsldma_chan *chan = to_fsl_chan(dchan);
+ unsigned long flags;
+
+ spin_lock_irqsave(&chan->desc_lock, flags);
fsl_chan_xfer_ld_queue(chan);
+ spin_unlock_irqrestore(&chan->desc_lock, flags);
}
/**
@@ -1109,20 +1060,53 @@ static irqreturn_t fsldma_chan_irq(int irq, void *data)
static void dma_do_tasklet(unsigned long data)
{
struct fsldma_chan *chan = (struct fsldma_chan *)data;
+ struct fsl_desc_sw *desc, *_desc;
+ LIST_HEAD(ld_cleanup);
unsigned long flags;
chan_dbg(chan, "tasklet entry\n");
- /* run all callbacks, free all used descriptors */
- fsl_chan_ld_cleanup(chan);
-
- /* the channel is now idle */
spin_lock_irqsave(&chan->desc_lock, flags);
+
+ /* update the cookie if we have some descriptors to cleanup */
+ if (!list_empty(&chan->ld_running)) {
+ dma_cookie_t cookie;
+
+ desc = to_fsl_desc(chan->ld_running.prev);
+ cookie = desc->async_tx.cookie;
+
+ chan->completed_cookie = cookie;
+ chan_dbg(chan, "completed_cookie=%d\n", cookie);
+ }
+
+ /*
+ * move the descriptors to a temporary list so we can drop the lock
+ * during the entire cleanup operation
+ */
+ list_splice_tail_init(&chan->ld_running, &ld_cleanup);
+
+ /* the hardware is now idle and ready for more */
chan->idle = true;
- spin_unlock_irqrestore(&chan->desc_lock, flags);
- /* start any pending transactions automatically */
+ /*
+ * Start any pending transactions automatically
+ *
+ * In the ideal case, we keep the DMA controller busy while we go
+ * ahead and free the descriptors below.
+ */
fsl_chan_xfer_ld_queue(chan);
+ spin_unlock_irqrestore(&chan->desc_lock, flags);
+
+ /* Run the callback for each descriptor, in order */
+ list_for_each_entry_safe(desc, _desc, &ld_cleanup, node) {
+
+ /* Remove from the list of transactions */
+ list_del(&desc->node);
+
+ /* Run all cleanup for this descriptor */
+ fsldma_cleanup_descriptor(chan, desc);
+ }
+
chan_dbg(chan, "tasklet exit\n");
}
--
1.7.3.4
^ permalink raw reply related
* [PATCH v3 7/9] fsldma: support async_tx dependencies and automatic unmapping
From: Ira W. Snyder @ 2011-03-03 17:54 UTC (permalink / raw)
To: linuxppc-dev; +Cc: dan.j.williams, linux-kernel, Ira W. Snyder
In-Reply-To: <1299174901-16762-1-git-send-email-iws@ovro.caltech.edu>
Previous to this patch, the dma_run_dependencies() function has been
called while holding desc_lock. This function can call tx_submit() for
other descriptors, which may try to re-grab the lock. Avoid this by
moving the descriptors to be cleaned up to a temporary list, and
dropping the lock before cleanup.
At the same time, add support for automatic unmapping of src and dst
buffers, as offered by the DMAEngine API.
Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
---
drivers/dma/fsldma.c | 131 ++++++++++++++++++++++++++++++++++++--------------
1 files changed, 95 insertions(+), 36 deletions(-)
diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index 6e9ad6e..526579d 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -83,6 +83,11 @@ static void set_desc_cnt(struct fsldma_chan *chan,
hw->count = CPU_TO_DMA(chan, count, 32);
}
+static u32 get_desc_cnt(struct fsldma_chan *chan, struct fsl_desc_sw *desc)
+{
+ return DMA_TO_CPU(chan, desc->hw.count, 32);
+}
+
static void set_desc_src(struct fsldma_chan *chan,
struct fsl_dma_ld_hw *hw, dma_addr_t src)
{
@@ -93,6 +98,16 @@ static void set_desc_src(struct fsldma_chan *chan,
hw->src_addr = CPU_TO_DMA(chan, snoop_bits | src, 64);
}
+static dma_addr_t get_desc_src(struct fsldma_chan *chan,
+ struct fsl_desc_sw *desc)
+{
+ u64 snoop_bits;
+
+ snoop_bits = ((chan->feature & FSL_DMA_IP_MASK) == FSL_DMA_IP_85XX)
+ ? ((u64)FSL_DMA_SATR_SREADTYPE_SNOOP_READ << 32) : 0;
+ return DMA_TO_CPU(chan, desc->hw.src_addr, 64) & ~snoop_bits;
+}
+
static void set_desc_dst(struct fsldma_chan *chan,
struct fsl_dma_ld_hw *hw, dma_addr_t dst)
{
@@ -103,6 +118,16 @@ static void set_desc_dst(struct fsldma_chan *chan,
hw->dst_addr = CPU_TO_DMA(chan, snoop_bits | dst, 64);
}
+static dma_addr_t get_desc_dst(struct fsldma_chan *chan,
+ struct fsl_desc_sw *desc)
+{
+ u64 snoop_bits;
+
+ snoop_bits = ((chan->feature & FSL_DMA_IP_MASK) == FSL_DMA_IP_85XX)
+ ? ((u64)FSL_DMA_DATR_DWRITETYPE_SNOOP_WRITE << 32) : 0;
+ return DMA_TO_CPU(chan, desc->hw.dst_addr, 64) & ~snoop_bits;
+}
+
static void set_desc_next(struct fsldma_chan *chan,
struct fsl_dma_ld_hw *hw, dma_addr_t next)
{
@@ -806,6 +831,57 @@ static int fsl_dma_device_control(struct dma_chan *dchan,
}
/**
+ * fsldma_cleanup_descriptor - cleanup and free a single link descriptor
+ * @chan: Freescale DMA channel
+ * @desc: descriptor to cleanup and free
+ *
+ * This function is used on a descriptor which has been executed by the DMA
+ * controller. It will run any callbacks, submit any dependencies, and then
+ * free the descriptor.
+ */
+static void fsldma_cleanup_descriptor(struct fsldma_chan *chan,
+ struct fsl_desc_sw *desc)
+{
+ struct dma_async_tx_descriptor *txd = &desc->async_tx;
+ struct device *dev = chan->common.device->dev;
+ dma_addr_t src = get_desc_src(chan, desc);
+ dma_addr_t dst = get_desc_dst(chan, desc);
+ u32 len = get_desc_cnt(chan, desc);
+
+ /* Run the link descriptor callback function */
+ if (txd->callback) {
+#ifdef FSL_DMA_LD_DEBUG
+ chan_dbg(chan, "LD %p callback\n", desc);
+#endif
+ txd->callback(txd->callback_param);
+ }
+
+ /* Run any dependencies */
+ dma_run_dependencies(txd);
+
+ /* Unmap the dst buffer, if requested */
+ if (!(txd->flags & DMA_COMPL_SKIP_DEST_UNMAP)) {
+ if (txd->flags & DMA_COMPL_DEST_UNMAP_SINGLE)
+ dma_unmap_single(dev, dst, len, DMA_FROM_DEVICE);
+ else
+ dma_unmap_page(dev, dst, len, DMA_FROM_DEVICE);
+ }
+
+ /* Unmap the src buffer, if requested */
+ if (!(txd->flags & DMA_COMPL_SKIP_SRC_UNMAP)) {
+ if (txd->flags & DMA_COMPL_SRC_UNMAP_SINGLE)
+ dma_unmap_single(dev, src, len, DMA_TO_DEVICE);
+ else
+ dma_unmap_page(dev, src, len, DMA_TO_DEVICE);
+ }
+
+#ifdef FSL_DMA_LD_DEBUG
+ chan_dbg(chan, "LD %p free\n", desc);
+#endif
+ dma_pool_free(chan->desc_pool, desc, txd->phys);
+}
+
+/**
* fsl_chan_ld_cleanup - Clean up link descriptors
* @chan : Freescale DMA channel
*
@@ -818,56 +894,39 @@ static int fsl_dma_device_control(struct dma_chan *dchan,
static void fsl_chan_ld_cleanup(struct fsldma_chan *chan)
{
struct fsl_desc_sw *desc, *_desc;
+ LIST_HEAD(ld_cleanup);
unsigned long flags;
spin_lock_irqsave(&chan->desc_lock, flags);
- /* if the ld_running list is empty, there is nothing to do */
- if (list_empty(&chan->ld_running)) {
- chan_dbg(chan, "no descriptors to cleanup\n");
- goto out_unlock;
+ /* update the cookie if we have some descriptors to cleanup */
+ if (!list_empty(&chan->ld_running)) {
+ dma_cookie_t cookie;
+
+ desc = to_fsl_desc(chan->ld_running.prev);
+ cookie = desc->async_tx.cookie;
+
+ chan->completed_cookie = cookie;
+ chan_dbg(chan, "completed cookie=%d\n", cookie);
}
/*
- * Get the last descriptor, update the cookie to it
- *
- * This is done before callbacks run so that clients can check the
- * status of their DMA transfer inside the callback.
+ * move the descriptors to a temporary list so we can drop the lock
+ * during the entire cleanup operation
*/
- desc = to_fsl_desc(chan->ld_running.prev);
- chan->completed_cookie = desc->async_tx.cookie;
- chan_dbg(chan, "completed_cookie = %d\n", chan->completed_cookie);
+ list_splice_tail_init(&chan->ld_running, &ld_cleanup);
+
+ spin_unlock_irqrestore(&chan->desc_lock, flags);
/* Run the callback for each descriptor, in order */
- list_for_each_entry_safe(desc, _desc, &chan->ld_running, node) {
- dma_async_tx_callback callback;
- void *callback_param;
+ list_for_each_entry_safe(desc, _desc, &ld_cleanup, node) {
- /* Remove from the list of running transactions */
+ /* Remove from the list of transactions */
list_del(&desc->node);
- /* Run the link descriptor callback function */
- callback = desc->async_tx.callback;
- callback_param = desc->async_tx.callback_param;
- if (callback) {
- spin_unlock_irqrestore(&chan->desc_lock, flags);
-#ifdef FSL_DMA_LD_DEBUG
- chan_dbg(chan, "LD %p callback\n", desc);
-#endif
- callback(callback_param);
- spin_lock_irqsave(&chan->desc_lock, flags);
- }
-
- /* Run any dependencies, then free the descriptor */
- dma_run_dependencies(&desc->async_tx);
-#ifdef FSL_DMA_LD_DEBUG
- chan_dbg(chan, "LD %p free\n", desc);
-#endif
- dma_pool_free(chan->desc_pool, desc, desc->async_tx.phys);
+ /* Run all cleanup for this descriptor */
+ fsldma_cleanup_descriptor(chan, desc);
}
-
-out_unlock:
- spin_unlock_irqrestore(&chan->desc_lock, flags);
}
/**
--
1.7.3.4
^ permalink raw reply related
* [PATCH v3 6/9] fsldma: fix controller lockups
From: Ira W. Snyder @ 2011-03-03 17:54 UTC (permalink / raw)
To: linuxppc-dev; +Cc: dan.j.williams, linux-kernel, Ira W. Snyder
In-Reply-To: <1299174901-16762-1-git-send-email-iws@ovro.caltech.edu>
Enabling poisoning in the dmapool API quickly showed that the DMA
controller was fetching descriptors that should not have been in use.
This has caused intermittent controller lockups during testing.
I have been unable to figure out the exact set of conditions which cause
this to happen. However, I believe it is related to the driver using the
hardware registers to track whether the controller is busy or not. The
code can incorrectly decide that the hardware is idle due to lag between
register writes and the hardware actually becoming busy.
To fix this, the driver has been reworked to explicitly track the state
of the hardware, rather than try to guess what it is doing based on the
register values.
This has passed dmatest with 10 threads per channel, 100000 iterations
per thread several times without error. Previously, this would fail
within a few seconds.
Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
---
drivers/dma/fsldma.c | 220 ++++++++++++++++++++++----------------------------
drivers/dma/fsldma.h | 1 +
2 files changed, 99 insertions(+), 122 deletions(-)
diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index 5da1a4a..6e9ad6e 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -68,11 +68,6 @@ static dma_addr_t get_cdar(struct fsldma_chan *chan)
return DMA_IN(chan, &chan->regs->cdar, 64) & ~FSL_DMA_SNEN;
}
-static dma_addr_t get_ndar(struct fsldma_chan *chan)
-{
- return DMA_IN(chan, &chan->regs->ndar, 64);
-}
-
static u32 get_bcr(struct fsldma_chan *chan)
{
return DMA_IN(chan, &chan->regs->bcr, 32);
@@ -143,13 +138,11 @@ static void dma_init(struct fsldma_chan *chan)
case FSL_DMA_IP_85XX:
/* Set the channel to below modes:
* EIE - Error interrupt enable
- * EOSIE - End of segments interrupt enable (basic mode)
* EOLNIE - End of links interrupt enable
* BWC - Bandwidth sharing among channels
*/
DMA_OUT(chan, &chan->regs->mr, FSL_DMA_MR_BWC
- | FSL_DMA_MR_EIE | FSL_DMA_MR_EOLNIE
- | FSL_DMA_MR_EOSIE, 32);
+ | FSL_DMA_MR_EIE | FSL_DMA_MR_EOLNIE, 32);
break;
case FSL_DMA_IP_83XX:
/* Set the channel to below modes:
@@ -168,25 +161,32 @@ static int dma_is_idle(struct fsldma_chan *chan)
return (!(sr & FSL_DMA_SR_CB)) || (sr & FSL_DMA_SR_CH);
}
+/*
+ * Start the DMA controller
+ *
+ * Preconditions:
+ * - the CDAR register must point to the start descriptor
+ * - the MRn[CS] bit must be cleared
+ */
static void dma_start(struct fsldma_chan *chan)
{
u32 mode;
mode = DMA_IN(chan, &chan->regs->mr, 32);
- if ((chan->feature & FSL_DMA_IP_MASK) == FSL_DMA_IP_85XX) {
- if (chan->feature & FSL_DMA_CHAN_PAUSE_EXT) {
- DMA_OUT(chan, &chan->regs->bcr, 0, 32);
- mode |= FSL_DMA_MR_EMP_EN;
- } else {
- mode &= ~FSL_DMA_MR_EMP_EN;
- }
+ if (chan->feature & FSL_DMA_CHAN_PAUSE_EXT) {
+ DMA_OUT(chan, &chan->regs->bcr, 0, 32);
+ mode |= FSL_DMA_MR_EMP_EN;
+ } else {
+ mode &= ~FSL_DMA_MR_EMP_EN;
}
- if (chan->feature & FSL_DMA_CHAN_START_EXT)
+ if (chan->feature & FSL_DMA_CHAN_START_EXT) {
mode |= FSL_DMA_MR_EMS_EN;
- else
+ } else {
+ mode &= ~FSL_DMA_MR_EMS_EN;
mode |= FSL_DMA_MR_CS;
+ }
DMA_OUT(chan, &chan->regs->mr, mode, 32);
}
@@ -760,14 +760,15 @@ static int fsl_dma_device_control(struct dma_chan *dchan,
switch (cmd) {
case DMA_TERMINATE_ALL:
+ spin_lock_irqsave(&chan->desc_lock, flags);
+
/* Halt the DMA engine */
dma_halt(chan);
- spin_lock_irqsave(&chan->desc_lock, flags);
-
/* Remove and free all of the descriptors in the LD queue */
fsldma_free_desc_list(chan, &chan->ld_pending);
fsldma_free_desc_list(chan, &chan->ld_running);
+ chan->idle = true;
spin_unlock_irqrestore(&chan->desc_lock, flags);
return 0;
@@ -805,76 +806,43 @@ static int fsl_dma_device_control(struct dma_chan *dchan,
}
/**
- * fsl_dma_update_completed_cookie - Update the completed cookie.
+ * fsl_chan_ld_cleanup - Clean up link descriptors
* @chan : Freescale DMA channel
*
- * CONTEXT: hardirq
+ * This function is run after the queue of running descriptors has been
+ * executed by the DMA engine. It will run any callbacks, and then free
+ * the descriptors.
+ *
+ * HARDWARE STATE: idle
*/
-static void fsl_dma_update_completed_cookie(struct fsldma_chan *chan)
+static void fsl_chan_ld_cleanup(struct fsldma_chan *chan)
{
- struct fsl_desc_sw *desc;
+ struct fsl_desc_sw *desc, *_desc;
unsigned long flags;
- dma_cookie_t cookie;
spin_lock_irqsave(&chan->desc_lock, flags);
+ /* if the ld_running list is empty, there is nothing to do */
if (list_empty(&chan->ld_running)) {
- chan_dbg(chan, "no running descriptors\n");
+ chan_dbg(chan, "no descriptors to cleanup\n");
goto out_unlock;
}
- /* Get the last descriptor, update the cookie to that */
+ /*
+ * Get the last descriptor, update the cookie to it
+ *
+ * This is done before callbacks run so that clients can check the
+ * status of their DMA transfer inside the callback.
+ */
desc = to_fsl_desc(chan->ld_running.prev);
- if (dma_is_idle(chan))
- cookie = desc->async_tx.cookie;
- else {
- cookie = desc->async_tx.cookie - 1;
- if (unlikely(cookie < DMA_MIN_COOKIE))
- cookie = DMA_MAX_COOKIE;
- }
-
- chan->completed_cookie = cookie;
-
-out_unlock:
- spin_unlock_irqrestore(&chan->desc_lock, flags);
-}
-
-/**
- * fsldma_desc_status - Check the status of a descriptor
- * @chan: Freescale DMA channel
- * @desc: DMA SW descriptor
- *
- * This function will return the status of the given descriptor
- */
-static enum dma_status fsldma_desc_status(struct fsldma_chan *chan,
- struct fsl_desc_sw *desc)
-{
- return dma_async_is_complete(desc->async_tx.cookie,
- chan->completed_cookie,
- chan->common.cookie);
-}
-
-/**
- * fsl_chan_ld_cleanup - Clean up link descriptors
- * @chan : Freescale DMA channel
- *
- * This function clean up the ld_queue of DMA channel.
- */
-static void fsl_chan_ld_cleanup(struct fsldma_chan *chan)
-{
- struct fsl_desc_sw *desc, *_desc;
- unsigned long flags;
-
- spin_lock_irqsave(&chan->desc_lock, flags);
+ chan->completed_cookie = desc->async_tx.cookie;
+ chan_dbg(chan, "completed_cookie = %d\n", chan->completed_cookie);
- chan_dbg(chan, "chan completed_cookie = %d\n", chan->completed_cookie);
+ /* Run the callback for each descriptor, in order */
list_for_each_entry_safe(desc, _desc, &chan->ld_running, node) {
dma_async_tx_callback callback;
void *callback_param;
- if (fsldma_desc_status(chan, desc) == DMA_IN_PROGRESS)
- break;
-
/* Remove from the list of running transactions */
list_del(&desc->node);
@@ -898,6 +866,7 @@ static void fsl_chan_ld_cleanup(struct fsldma_chan *chan)
dma_pool_free(chan->desc_pool, desc, desc->async_tx.phys);
}
+out_unlock:
spin_unlock_irqrestore(&chan->desc_lock, flags);
}
@@ -905,10 +874,7 @@ static void fsl_chan_ld_cleanup(struct fsldma_chan *chan)
* fsl_chan_xfer_ld_queue - transfer any pending transactions
* @chan : Freescale DMA channel
*
- * This will make sure that any pending transactions will be run.
- * If the DMA controller is idle, it will be started. Otherwise,
- * the DMA controller's interrupt handler will start any pending
- * transactions when it becomes idle.
+ * HARDWARE STATE: idle
*/
static void fsl_chan_xfer_ld_queue(struct fsldma_chan *chan)
{
@@ -927,23 +893,16 @@ static void fsl_chan_xfer_ld_queue(struct fsldma_chan *chan)
}
/*
- * The DMA controller is not idle, which means the interrupt
- * handler will start any queued transactions when it runs
- * at the end of the current transaction
+ * The DMA controller is not idle, which means that the interrupt
+ * handler will start any queued transactions when it runs after
+ * this transaction finishes
*/
- if (!dma_is_idle(chan)) {
+ if (!chan->idle) {
chan_dbg(chan, "DMA controller still busy\n");
goto out_unlock;
}
/*
- * TODO:
- * make sure the dma_halt() function really un-wedges the
- * controller as much as possible
- */
- dma_halt(chan);
-
- /*
* If there are some link descriptors which have not been
* transferred, we need to start the controller
*/
@@ -952,15 +911,32 @@ static void fsl_chan_xfer_ld_queue(struct fsldma_chan *chan)
* Move all elements from the queue of pending transactions
* onto the list of running transactions
*/
+ chan_dbg(chan, "idle, starting controller\n");
desc = list_first_entry(&chan->ld_pending, struct fsl_desc_sw, node);
list_splice_tail_init(&chan->ld_pending, &chan->ld_running);
/*
+ * The 85xx DMA controller doesn't clear the channel start bit
+ * automatically at the end of a transfer. Therefore we must clear
+ * it in software before starting the transfer.
+ */
+ if ((chan->feature & FSL_DMA_IP_MASK) == FSL_DMA_IP_85XX) {
+ u32 mode;
+
+ mode = DMA_IN(chan, &chan->regs->mr, 32);
+ mode &= ~FSL_DMA_MR_CS;
+ DMA_OUT(chan, &chan->regs->mr, mode, 32);
+ }
+
+ /*
* Program the descriptor's address into the DMA controller,
* then start the DMA transaction
*/
set_cdar(chan, desc->async_tx.phys);
+ get_cdar(chan);
+
dma_start(chan);
+ chan->idle = false;
out_unlock:
spin_unlock_irqrestore(&chan->desc_lock, flags);
@@ -985,16 +961,18 @@ static enum dma_status fsl_tx_status(struct dma_chan *dchan,
struct dma_tx_state *txstate)
{
struct fsldma_chan *chan = to_fsl_chan(dchan);
- dma_cookie_t last_used;
dma_cookie_t last_complete;
+ dma_cookie_t last_used;
+ unsigned long flags;
- fsl_chan_ld_cleanup(chan);
+ spin_lock_irqsave(&chan->desc_lock, flags);
- last_used = dchan->cookie;
last_complete = chan->completed_cookie;
+ last_used = dchan->cookie;
- dma_set_tx_state(txstate, last_complete, last_used, 0);
+ spin_unlock_irqrestore(&chan->desc_lock, flags);
+ dma_set_tx_state(txstate, last_complete, last_used, 0);
return dma_async_is_complete(cookie, last_complete, last_used);
}
@@ -1005,8 +983,6 @@ static enum dma_status fsl_tx_status(struct dma_chan *dchan,
static irqreturn_t fsldma_chan_irq(int irq, void *data)
{
struct fsldma_chan *chan = data;
- int update_cookie = 0;
- int xfer_ld_q = 0;
u32 stat;
/* save and clear the status register */
@@ -1014,6 +990,7 @@ static irqreturn_t fsldma_chan_irq(int irq, void *data)
set_sr(chan, stat);
chan_dbg(chan, "irq: stat = 0x%x\n", stat);
+ /* check that this was really our device */
stat &= ~(FSL_DMA_SR_CB | FSL_DMA_SR_CH);
if (!stat)
return IRQ_NONE;
@@ -1028,28 +1005,9 @@ static irqreturn_t fsldma_chan_irq(int irq, void *data)
*/
if (stat & FSL_DMA_SR_PE) {
chan_dbg(chan, "irq: Programming Error INT\n");
- if (get_bcr(chan) == 0) {
- /* BCR register is 0, this is a DMA_INTERRUPT async_tx.
- * Now, update the completed cookie, and continue the
- * next uncompleted transfer.
- */
- update_cookie = 1;
- xfer_ld_q = 1;
- }
stat &= ~FSL_DMA_SR_PE;
- }
-
- /*
- * If the link descriptor segment transfer finishes,
- * we will recycle the used descriptor.
- */
- if (stat & FSL_DMA_SR_EOSI) {
- chan_dbg(chan, "irq: End-of-segments INT\n");
- chan_dbg(chan, "irq: clndar 0x%llx, nlndar 0x%llx\n",
- (unsigned long long)get_cdar(chan),
- (unsigned long long)get_ndar(chan));
- stat &= ~FSL_DMA_SR_EOSI;
- update_cookie = 1;
+ if (get_bcr(chan) != 0)
+ chan_err(chan, "Programming Error!\n");
}
/*
@@ -1059,8 +1017,6 @@ static irqreturn_t fsldma_chan_irq(int irq, void *data)
if (stat & FSL_DMA_SR_EOCDI) {
chan_dbg(chan, "irq: End-of-Chain link INT\n");
stat &= ~FSL_DMA_SR_EOCDI;
- update_cookie = 1;
- xfer_ld_q = 1;
}
/*
@@ -1071,25 +1027,44 @@ static irqreturn_t fsldma_chan_irq(int irq, void *data)
if (stat & FSL_DMA_SR_EOLNI) {
chan_dbg(chan, "irq: End-of-link INT\n");
stat &= ~FSL_DMA_SR_EOLNI;
- xfer_ld_q = 1;
}
- if (update_cookie)
- fsl_dma_update_completed_cookie(chan);
- if (xfer_ld_q)
- fsl_chan_xfer_ld_queue(chan);
+ /* check that the DMA controller is really idle */
+ if (!dma_is_idle(chan))
+ chan_err(chan, "irq: controller not idle!\n");
+
+ /* check that we handled all of the bits */
if (stat)
- chan_dbg(chan, "irq: unhandled sr 0x%08x\n", stat);
+ chan_err(chan, "irq: unhandled sr 0x%08x\n", stat);
- chan_dbg(chan, "irq: Exit\n");
+ /*
+ * Schedule the tasklet to handle all cleanup of the current
+ * transaction. It will start a new transaction if there is
+ * one pending.
+ */
tasklet_schedule(&chan->tasklet);
+ chan_dbg(chan, "irq: Exit\n");
return IRQ_HANDLED;
}
static void dma_do_tasklet(unsigned long data)
{
struct fsldma_chan *chan = (struct fsldma_chan *)data;
+ unsigned long flags;
+
+ chan_dbg(chan, "tasklet entry\n");
+
+ /* run all callbacks, free all used descriptors */
fsl_chan_ld_cleanup(chan);
+
+ /* the channel is now idle */
+ spin_lock_irqsave(&chan->desc_lock, flags);
+ chan->idle = true;
+ spin_unlock_irqrestore(&chan->desc_lock, flags);
+
+ /* start any pending transactions automatically */
+ fsl_chan_xfer_ld_queue(chan);
+ chan_dbg(chan, "tasklet exit\n");
}
static irqreturn_t fsldma_ctrl_irq(int irq, void *data)
@@ -1269,6 +1244,7 @@ static int __devinit fsl_dma_chan_probe(struct fsldma_device *fdev,
spin_lock_init(&chan->desc_lock);
INIT_LIST_HEAD(&chan->ld_pending);
INIT_LIST_HEAD(&chan->ld_running);
+ chan->idle = true;
chan->common.device = &fdev->common;
diff --git a/drivers/dma/fsldma.h b/drivers/dma/fsldma.h
index 49189da..9cb5aa5 100644
--- a/drivers/dma/fsldma.h
+++ b/drivers/dma/fsldma.h
@@ -148,6 +148,7 @@ struct fsldma_chan {
int id; /* Raw id of this channel */
struct tasklet_struct tasklet;
u32 feature;
+ bool idle; /* DMA controller is idle */
void (*toggle_ext_pause)(struct fsldma_chan *fsl_chan, int enable);
void (*toggle_ext_start)(struct fsldma_chan *fsl_chan, int enable);
--
1.7.3.4
^ permalink raw reply related
* [PATCH v3 5/9] fsldma: minor codingstyle and consistency fixes
From: Ira W. Snyder @ 2011-03-03 17:54 UTC (permalink / raw)
To: linuxppc-dev; +Cc: dan.j.williams, linux-kernel, Ira W. Snyder
In-Reply-To: <1299174901-16762-1-git-send-email-iws@ovro.caltech.edu>
This fixes some minor violations of the coding style. It also changes
the style of the device_prep_dma_*() function definitions so they are
identical.
Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
---
drivers/dma/fsldma.c | 29 +++++++++++++----------------
drivers/dma/fsldma.h | 4 ++--
2 files changed, 15 insertions(+), 18 deletions(-)
diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index 82b8e9f..5da1a4a 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -89,7 +89,7 @@ static void set_desc_cnt(struct fsldma_chan *chan,
}
static void set_desc_src(struct fsldma_chan *chan,
- struct fsl_dma_ld_hw *hw, dma_addr_t src)
+ struct fsl_dma_ld_hw *hw, dma_addr_t src)
{
u64 snoop_bits;
@@ -99,7 +99,7 @@ static void set_desc_src(struct fsldma_chan *chan,
}
static void set_desc_dst(struct fsldma_chan *chan,
- struct fsl_dma_ld_hw *hw, dma_addr_t dst)
+ struct fsl_dma_ld_hw *hw, dma_addr_t dst)
{
u64 snoop_bits;
@@ -109,7 +109,7 @@ static void set_desc_dst(struct fsldma_chan *chan,
}
static void set_desc_next(struct fsldma_chan *chan,
- struct fsl_dma_ld_hw *hw, dma_addr_t next)
+ struct fsl_dma_ld_hw *hw, dma_addr_t next)
{
u64 snoop_bits;
@@ -118,8 +118,7 @@ static void set_desc_next(struct fsldma_chan *chan,
hw->next_ln_addr = CPU_TO_DMA(chan, snoop_bits | next, 64);
}
-static void set_ld_eol(struct fsldma_chan *chan,
- struct fsl_desc_sw *desc)
+static void set_ld_eol(struct fsldma_chan *chan, struct fsl_desc_sw *desc)
{
u64 snoop_bits;
@@ -338,8 +337,7 @@ static void fsl_chan_toggle_ext_start(struct fsldma_chan *chan, int enable)
chan->feature &= ~FSL_DMA_CHAN_START_EXT;
}
-static void append_ld_queue(struct fsldma_chan *chan,
- struct fsl_desc_sw *desc)
+static void append_ld_queue(struct fsldma_chan *chan, struct fsl_desc_sw *desc)
{
struct fsl_desc_sw *tail = to_fsl_desc(chan->ld_pending.prev);
@@ -380,8 +378,8 @@ static dma_cookie_t fsl_dma_tx_submit(struct dma_async_tx_descriptor *tx)
cookie = chan->common.cookie;
list_for_each_entry(child, &desc->tx_list, node) {
cookie++;
- if (cookie < 0)
- cookie = 1;
+ if (cookie < DMA_MIN_COOKIE)
+ cookie = DMA_MIN_COOKIE;
child->async_tx.cookie = cookie;
}
@@ -402,8 +400,7 @@ static dma_cookie_t fsl_dma_tx_submit(struct dma_async_tx_descriptor *tx)
*
* Return - The descriptor allocated. NULL for failed.
*/
-static struct fsl_desc_sw *fsl_dma_alloc_descriptor(
- struct fsldma_chan *chan)
+static struct fsl_desc_sw *fsl_dma_alloc_descriptor(struct fsldma_chan *chan)
{
struct fsl_desc_sw *desc;
dma_addr_t pdesc;
@@ -427,7 +424,6 @@ static struct fsl_desc_sw *fsl_dma_alloc_descriptor(
return desc;
}
-
/**
* fsl_dma_alloc_chan_resources - Allocate resources for DMA channel.
* @chan : Freescale DMA channel
@@ -537,14 +533,15 @@ fsl_dma_prep_interrupt(struct dma_chan *dchan, unsigned long flags)
/* Insert the link descriptor to the LD ring */
list_add_tail(&new->node, &new->tx_list);
- /* Set End-of-link to the last link descriptor of new list*/
+ /* Set End-of-link to the last link descriptor of new list */
set_ld_eol(chan, new);
return &new->async_tx;
}
-static struct dma_async_tx_descriptor *fsl_dma_prep_memcpy(
- struct dma_chan *dchan, dma_addr_t dma_dst, dma_addr_t dma_src,
+static struct dma_async_tx_descriptor *
+fsl_dma_prep_memcpy(struct dma_chan *dchan,
+ dma_addr_t dma_dst, dma_addr_t dma_src,
size_t len, unsigned long flags)
{
struct fsldma_chan *chan;
@@ -594,7 +591,7 @@ static struct dma_async_tx_descriptor *fsl_dma_prep_memcpy(
new->async_tx.flags = flags; /* client is in control of this ack */
new->async_tx.cookie = -EBUSY;
- /* Set End-of-link to the last link descriptor of new list*/
+ /* Set End-of-link to the last link descriptor of new list */
set_ld_eol(chan, new);
return &first->async_tx;
diff --git a/drivers/dma/fsldma.h b/drivers/dma/fsldma.h
index 113e713..49189da 100644
--- a/drivers/dma/fsldma.h
+++ b/drivers/dma/fsldma.h
@@ -102,8 +102,8 @@ struct fsl_desc_sw {
} __attribute__((aligned(32)));
struct fsldma_chan_regs {
- u32 mr; /* 0x00 - Mode Register */
- u32 sr; /* 0x04 - Status Register */
+ u32 mr; /* 0x00 - Mode Register */
+ u32 sr; /* 0x04 - Status Register */
u64 cdar; /* 0x08 - Current descriptor address register */
u64 sar; /* 0x10 - Source Address Register */
u64 dar; /* 0x18 - Destination Address Register */
--
1.7.3.4
^ permalink raw reply related
* [PATCH v3 9/9] fsldma: make halt behave nicely on all supported controllers
From: Ira W. Snyder @ 2011-03-03 17:55 UTC (permalink / raw)
To: linuxppc-dev; +Cc: dan.j.williams, linux-kernel, Ira W. Snyder
In-Reply-To: <1299174901-16762-1-git-send-email-iws@ovro.caltech.edu>
The original dma_halt() function set the CA (channel abort) bit on both
the 83xx and 85xx controllers. This is incorrect on the 83xx, where this
bit means TEM (transfer error mask) instead. The 83xx doesn't support
channel abort, so we only do this operation on 85xx.
Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
---
drivers/dma/fsldma.c | 19 ++++++++++++++++---
1 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index d300de4..8670a50 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -221,13 +221,26 @@ static void dma_halt(struct fsldma_chan *chan)
u32 mode;
int i;
+ /* read the mode register */
mode = DMA_IN(chan, &chan->regs->mr, 32);
- mode |= FSL_DMA_MR_CA;
- DMA_OUT(chan, &chan->regs->mr, mode, 32);
- mode &= ~(FSL_DMA_MR_CS | FSL_DMA_MR_EMS_EN | FSL_DMA_MR_CA);
+ /*
+ * The 85xx controller supports channel abort, which will stop
+ * the current transfer. On 83xx, this bit is the transfer error
+ * mask bit, which should not be changed.
+ */
+ if ((chan->feature & FSL_DMA_IP_MASK) == FSL_DMA_IP_85XX) {
+ mode |= FSL_DMA_MR_CA;
+ DMA_OUT(chan, &chan->regs->mr, mode, 32);
+
+ mode &= ~FSL_DMA_MR_CA;
+ }
+
+ /* stop the DMA controller */
+ mode &= ~(FSL_DMA_MR_CS | FSL_DMA_MR_EMS_EN);
DMA_OUT(chan, &chan->regs->mr, mode, 32);
+ /* wait for the DMA controller to become idle */
for (i = 0; i < 100; i++) {
if (dma_is_idle(chan))
return;
--
1.7.3.4
^ permalink raw reply related
* [PATCH v3 3/9] fsldma: use channel name in printk output
From: Ira W. Snyder @ 2011-03-03 17:54 UTC (permalink / raw)
To: linuxppc-dev; +Cc: dan.j.williams, linux-kernel, Ira W. Snyder
In-Reply-To: <1299174901-16762-1-git-send-email-iws@ovro.caltech.edu>
This makes debugging the driver much easier when multiple channels are
running concurrently. In addition, you can see how much descriptor
memory each channel has allocated via the dmapool API in sysfs.
Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
---
drivers/dma/fsldma.c | 69 +++++++++++++++++++++++++------------------------
drivers/dma/fsldma.h | 1 +
2 files changed, 36 insertions(+), 34 deletions(-)
diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index 2e1af45..e535cd1 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -37,7 +37,12 @@
#include "fsldma.h"
-static const char msg_ld_oom[] = "No free memory for link descriptor\n";
+#define chan_dbg(chan, fmt, arg...) \
+ dev_dbg(chan->dev, "%s: " fmt, chan->name, ##arg)
+#define chan_err(chan, fmt, arg...) \
+ dev_err(chan->dev, "%s: " fmt, chan->name, ##arg)
+
+static const char msg_ld_oom[] = "No free memory for link descriptor";
/*
* Register Helpers
@@ -207,7 +212,7 @@ static void dma_halt(struct fsldma_chan *chan)
}
if (!dma_is_idle(chan))
- dev_err(chan->dev, "DMA halt timeout!\n");
+ chan_err(chan, "DMA halt timeout!\n");
}
/**
@@ -405,7 +410,7 @@ static struct fsl_desc_sw *fsl_dma_alloc_descriptor(
desc = dma_pool_alloc(chan->desc_pool, GFP_ATOMIC, &pdesc);
if (!desc) {
- dev_dbg(chan->dev, "out of memory for link desc\n");
+ chan_dbg(chan, "out of memory for link descriptor\n");
return NULL;
}
@@ -439,13 +444,11 @@ static int fsl_dma_alloc_chan_resources(struct dma_chan *dchan)
* We need the descriptor to be aligned to 32bytes
* for meeting FSL DMA specification requirement.
*/
- chan->desc_pool = dma_pool_create("fsl_dma_engine_desc_pool",
- chan->dev,
+ chan->desc_pool = dma_pool_create(chan->name, chan->dev,
sizeof(struct fsl_desc_sw),
__alignof__(struct fsl_desc_sw), 0);
if (!chan->desc_pool) {
- dev_err(chan->dev, "unable to allocate channel %d "
- "descriptor pool\n", chan->id);
+ chan_err(chan, "unable to allocate descriptor pool\n");
return -ENOMEM;
}
@@ -491,7 +494,7 @@ static void fsl_dma_free_chan_resources(struct dma_chan *dchan)
struct fsldma_chan *chan = to_fsl_chan(dchan);
unsigned long flags;
- dev_dbg(chan->dev, "Free all channel resources.\n");
+ chan_dbg(chan, "free all channel resources\n");
spin_lock_irqsave(&chan->desc_lock, flags);
fsldma_free_desc_list(chan, &chan->ld_pending);
fsldma_free_desc_list(chan, &chan->ld_running);
@@ -514,7 +517,7 @@ fsl_dma_prep_interrupt(struct dma_chan *dchan, unsigned long flags)
new = fsl_dma_alloc_descriptor(chan);
if (!new) {
- dev_err(chan->dev, msg_ld_oom);
+ chan_err(chan, "%s\n", msg_ld_oom);
return NULL;
}
@@ -551,11 +554,11 @@ static struct dma_async_tx_descriptor *fsl_dma_prep_memcpy(
/* Allocate the link descriptor from DMA pool */
new = fsl_dma_alloc_descriptor(chan);
if (!new) {
- dev_err(chan->dev, msg_ld_oom);
+ chan_err(chan, "%s\n", msg_ld_oom);
goto fail;
}
#ifdef FSL_DMA_LD_DEBUG
- dev_dbg(chan->dev, "new link desc alloc %p\n", new);
+ chan_dbg(chan, "new link desc alloc %p\n", new);
#endif
copy = min(len, (size_t)FSL_DMA_BCR_MAX_CNT);
@@ -639,11 +642,11 @@ static struct dma_async_tx_descriptor *fsl_dma_prep_sg(struct dma_chan *dchan,
/* allocate and populate the descriptor */
new = fsl_dma_alloc_descriptor(chan);
if (!new) {
- dev_err(chan->dev, msg_ld_oom);
+ chan_err(chan, "%s\n", msg_ld_oom);
goto fail;
}
#ifdef FSL_DMA_LD_DEBUG
- dev_dbg(chan->dev, "new link desc alloc %p\n", new);
+ chan_dbg(chan, "new link desc alloc %p\n", new);
#endif
set_desc_cnt(chan, &new->hw, len);
@@ -815,7 +818,7 @@ static void fsl_dma_update_completed_cookie(struct fsldma_chan *chan)
spin_lock_irqsave(&chan->desc_lock, flags);
if (list_empty(&chan->ld_running)) {
- dev_dbg(chan->dev, "no running descriptors\n");
+ chan_dbg(chan, "no running descriptors\n");
goto out_unlock;
}
@@ -863,7 +866,7 @@ static void fsl_chan_ld_cleanup(struct fsldma_chan *chan)
spin_lock_irqsave(&chan->desc_lock, flags);
- dev_dbg(chan->dev, "chan completed_cookie = %d\n", chan->completed_cookie);
+ chan_dbg(chan, "chan completed_cookie = %d\n", chan->completed_cookie);
list_for_each_entry_safe(desc, _desc, &chan->ld_running, node) {
dma_async_tx_callback callback;
void *callback_param;
@@ -879,7 +882,7 @@ static void fsl_chan_ld_cleanup(struct fsldma_chan *chan)
callback_param = desc->async_tx.callback_param;
if (callback) {
spin_unlock_irqrestore(&chan->desc_lock, flags);
- dev_dbg(chan->dev, "LD %p callback\n", desc);
+ chan_dbg(chan, "LD %p callback\n", desc);
callback(callback_param);
spin_lock_irqsave(&chan->desc_lock, flags);
}
@@ -913,7 +916,7 @@ static void fsl_chan_xfer_ld_queue(struct fsldma_chan *chan)
* don't need to do any work at all
*/
if (list_empty(&chan->ld_pending)) {
- dev_dbg(chan->dev, "no pending LDs\n");
+ chan_dbg(chan, "no pending LDs\n");
goto out_unlock;
}
@@ -923,7 +926,7 @@ static void fsl_chan_xfer_ld_queue(struct fsldma_chan *chan)
* at the end of the current transaction
*/
if (!dma_is_idle(chan)) {
- dev_dbg(chan->dev, "DMA controller still busy\n");
+ chan_dbg(chan, "DMA controller still busy\n");
goto out_unlock;
}
@@ -1003,14 +1006,14 @@ static irqreturn_t fsldma_chan_irq(int irq, void *data)
/* save and clear the status register */
stat = get_sr(chan);
set_sr(chan, stat);
- dev_dbg(chan->dev, "irq: channel %d, stat = 0x%x\n", chan->id, stat);
+ chan_dbg(chan, "irq: stat = 0x%x\n", stat);
stat &= ~(FSL_DMA_SR_CB | FSL_DMA_SR_CH);
if (!stat)
return IRQ_NONE;
if (stat & FSL_DMA_SR_TE)
- dev_err(chan->dev, "Transfer Error!\n");
+ chan_err(chan, "Transfer Error!\n");
/*
* Programming Error
@@ -1018,7 +1021,7 @@ static irqreturn_t fsldma_chan_irq(int irq, void *data)
* triger a PE interrupt.
*/
if (stat & FSL_DMA_SR_PE) {
- dev_dbg(chan->dev, "irq: Programming Error INT\n");
+ chan_dbg(chan, "irq: Programming Error INT\n");
if (get_bcr(chan) == 0) {
/* BCR register is 0, this is a DMA_INTERRUPT async_tx.
* Now, update the completed cookie, and continue the
@@ -1035,8 +1038,8 @@ static irqreturn_t fsldma_chan_irq(int irq, void *data)
* we will recycle the used descriptor.
*/
if (stat & FSL_DMA_SR_EOSI) {
- dev_dbg(chan->dev, "irq: End-of-segments INT\n");
- dev_dbg(chan->dev, "irq: clndar 0x%llx, nlndar 0x%llx\n",
+ chan_dbg(chan, "irq: End-of-segments INT\n");
+ chan_dbg(chan, "irq: clndar 0x%llx, nlndar 0x%llx\n",
(unsigned long long)get_cdar(chan),
(unsigned long long)get_ndar(chan));
stat &= ~FSL_DMA_SR_EOSI;
@@ -1048,7 +1051,7 @@ static irqreturn_t fsldma_chan_irq(int irq, void *data)
* and start the next transfer if it exist.
*/
if (stat & FSL_DMA_SR_EOCDI) {
- dev_dbg(chan->dev, "irq: End-of-Chain link INT\n");
+ chan_dbg(chan, "irq: End-of-Chain link INT\n");
stat &= ~FSL_DMA_SR_EOCDI;
update_cookie = 1;
xfer_ld_q = 1;
@@ -1060,7 +1063,7 @@ static irqreturn_t fsldma_chan_irq(int irq, void *data)
* prepare next transfer.
*/
if (stat & FSL_DMA_SR_EOLNI) {
- dev_dbg(chan->dev, "irq: End-of-link INT\n");
+ chan_dbg(chan, "irq: End-of-link INT\n");
stat &= ~FSL_DMA_SR_EOLNI;
xfer_ld_q = 1;
}
@@ -1070,9 +1073,9 @@ static irqreturn_t fsldma_chan_irq(int irq, void *data)
if (xfer_ld_q)
fsl_chan_xfer_ld_queue(chan);
if (stat)
- dev_dbg(chan->dev, "irq: unhandled sr 0x%02x\n", stat);
+ chan_dbg(chan, "irq: unhandled sr 0x%08x\n", stat);
- dev_dbg(chan->dev, "irq: Exit\n");
+ chan_dbg(chan, "irq: Exit\n");
tasklet_schedule(&chan->tasklet);
return IRQ_HANDLED;
}
@@ -1128,7 +1131,7 @@ static void fsldma_free_irqs(struct fsldma_device *fdev)
for (i = 0; i < FSL_DMA_MAX_CHANS_PER_DEVICE; i++) {
chan = fdev->chan[i];
if (chan && chan->irq != NO_IRQ) {
- dev_dbg(fdev->dev, "free channel %d IRQ\n", chan->id);
+ chan_dbg(chan, "free per-channel IRQ\n");
free_irq(chan->irq, chan);
}
}
@@ -1155,19 +1158,16 @@ static int fsldma_request_irqs(struct fsldma_device *fdev)
continue;
if (chan->irq == NO_IRQ) {
- dev_err(fdev->dev, "no interrupts property defined for "
- "DMA channel %d. Please fix your "
- "device tree\n", chan->id);
+ chan_err(chan, "interrupts property missing in device tree\n");
ret = -ENODEV;
goto out_unwind;
}
- dev_dbg(fdev->dev, "request channel %d IRQ\n", chan->id);
+ chan_dbg(chan, "request per-channel IRQ\n");
ret = request_irq(chan->irq, fsldma_chan_irq, IRQF_SHARED,
"fsldma-chan", chan);
if (ret) {
- dev_err(fdev->dev, "unable to request IRQ for DMA "
- "channel %d\n", chan->id);
+ chan_err(chan, "unable to request per-channel IRQ\n");
goto out_unwind;
}
}
@@ -1242,6 +1242,7 @@ static int __devinit fsl_dma_chan_probe(struct fsldma_device *fdev,
fdev->chan[chan->id] = chan;
tasklet_init(&chan->tasklet, dma_do_tasklet, (unsigned long)chan);
+ snprintf(chan->name, sizeof(chan->name), "chan%d", chan->id);
/* Initialize the channel */
dma_init(chan);
diff --git a/drivers/dma/fsldma.h b/drivers/dma/fsldma.h
index ba9f403..113e713 100644
--- a/drivers/dma/fsldma.h
+++ b/drivers/dma/fsldma.h
@@ -135,6 +135,7 @@ struct fsldma_device {
#define FSL_DMA_CHAN_START_EXT 0x00002000
struct fsldma_chan {
+ char name[8]; /* Channel name */
struct fsldma_chan_regs __iomem *regs;
dma_cookie_t completed_cookie; /* The maximum cookie completed */
spinlock_t desc_lock; /* Descriptor operation lock */
--
1.7.3.4
^ permalink raw reply related
* [PATCH v3 4/9] fsldma: improve link descriptor debugging
From: Ira W. Snyder @ 2011-03-03 17:54 UTC (permalink / raw)
To: linuxppc-dev; +Cc: dan.j.williams, linux-kernel, Ira W. Snyder
In-Reply-To: <1299174901-16762-1-git-send-email-iws@ovro.caltech.edu>
This adds better tracking to link descriptor allocations, callbacks, and
frees. This makes it much easier to track errors with link descriptors.
Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
---
drivers/dma/fsldma.c | 21 +++++++++++++++------
1 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index e535cd1..82b8e9f 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -420,6 +420,10 @@ static struct fsl_desc_sw *fsl_dma_alloc_descriptor(
desc->async_tx.tx_submit = fsl_dma_tx_submit;
desc->async_tx.phys = pdesc;
+#ifdef FSL_DMA_LD_DEBUG
+ chan_dbg(chan, "LD %p allocated\n", desc);
+#endif
+
return desc;
}
@@ -470,6 +474,9 @@ static void fsldma_free_desc_list(struct fsldma_chan *chan,
list_for_each_entry_safe(desc, _desc, list, node) {
list_del(&desc->node);
+#ifdef FSL_DMA_LD_DEBUG
+ chan_dbg(chan, "LD %p free\n", desc);
+#endif
dma_pool_free(chan->desc_pool, desc, desc->async_tx.phys);
}
}
@@ -481,6 +488,9 @@ static void fsldma_free_desc_list_reverse(struct fsldma_chan *chan,
list_for_each_entry_safe_reverse(desc, _desc, list, node) {
list_del(&desc->node);
+#ifdef FSL_DMA_LD_DEBUG
+ chan_dbg(chan, "LD %p free\n", desc);
+#endif
dma_pool_free(chan->desc_pool, desc, desc->async_tx.phys);
}
}
@@ -557,9 +567,6 @@ static struct dma_async_tx_descriptor *fsl_dma_prep_memcpy(
chan_err(chan, "%s\n", msg_ld_oom);
goto fail;
}
-#ifdef FSL_DMA_LD_DEBUG
- chan_dbg(chan, "new link desc alloc %p\n", new);
-#endif
copy = min(len, (size_t)FSL_DMA_BCR_MAX_CNT);
@@ -645,9 +652,6 @@ static struct dma_async_tx_descriptor *fsl_dma_prep_sg(struct dma_chan *dchan,
chan_err(chan, "%s\n", msg_ld_oom);
goto fail;
}
-#ifdef FSL_DMA_LD_DEBUG
- chan_dbg(chan, "new link desc alloc %p\n", new);
-#endif
set_desc_cnt(chan, &new->hw, len);
set_desc_src(chan, &new->hw, src);
@@ -882,13 +886,18 @@ static void fsl_chan_ld_cleanup(struct fsldma_chan *chan)
callback_param = desc->async_tx.callback_param;
if (callback) {
spin_unlock_irqrestore(&chan->desc_lock, flags);
+#ifdef FSL_DMA_LD_DEBUG
chan_dbg(chan, "LD %p callback\n", desc);
+#endif
callback(callback_param);
spin_lock_irqsave(&chan->desc_lock, flags);
}
/* Run any dependencies, then free the descriptor */
dma_run_dependencies(&desc->async_tx);
+#ifdef FSL_DMA_LD_DEBUG
+ chan_dbg(chan, "LD %p free\n", desc);
+#endif
dma_pool_free(chan->desc_pool, desc, desc->async_tx.phys);
}
--
1.7.3.4
^ permalink raw reply related
* [PATCH v3 0/9] fsldma: lockup fixes
From: Ira W. Snyder @ 2011-03-03 17:54 UTC (permalink / raw)
To: linuxppc-dev; +Cc: dan.j.williams, linux-kernel, Ira W. Snyder
Hello everyone,
I've been chasing random infrequent controller lockups in the fsldma driver
for a long time. I finally managed to find the problem and fix it. I'm not
quite sure about the exact sequence of events which causes the race
condition, but it is related to using the hardware registers to track the
controller state. See the patch changelogs for more detail.
The problems were quickly found by turning on DMAPOOL_DEBUG inside
mm/dmapool.c. This poisons memory allocated with the dmapool API.
With dmapool poisoning turned on, the dmatest driver would start producing
failures within a few seconds. After this patchset has been applied, I have
run several iterations of the 10 threads per channel, 100000 iterations per
thread test without any problems. I have also tested it with the CARMA
drivers (posted at linuxppc-dev previously), which make use of the external
control features.
While making the previous changes, I noticed that the fsldma driver does
not respect the automatic DMA unmapping of src and dst buffers. I have
added support for this feature. This also required a fix to dmatest, which
was sending incorrect flags.
The "support async_tx dependencies" patch could be split apart from the
automatic unmapping patch if it is desirable. They both touch the same
piece of code, so I thought it was ok to combine them. Let me know.
I would really like to see this go into 2.6.39. I think we can get it
reviewed before then. :)
Much thanks goes to Felix Radensky for testing on a P2020 (85xx DMA IP core).
I wouldn't have been able to track down the problems on 85xx without his
dilligent testing.
v2 -> v3:
- use chan_dbg() and chan_err() macros for channel printk
v1 -> v2:
- reordered patches (dmatest change is first now)
- fix problems on 85xx controller
- only set correct bits for 83xx in dma_halt()
Ira W. Snyder (9):
dmatest: fix automatic buffer unmap type
fsldma: move related helper functions near each other
fsldma: use channel name in printk output
fsldma: improve link descriptor debugging
fsldma: minor codingstyle and consistency fixes
fsldma: fix controller lockups
fsldma: support async_tx dependencies and automatic unmapping
fsldma: reduce locking during descriptor cleanup
fsldma: make halt behave nicely on all supported controllers
drivers/dma/dmatest.c | 7 +-
drivers/dma/fsldma.c | 551 +++++++++++++++++++++++++++----------------------
drivers/dma/fsldma.h | 6 +-
3 files changed, 311 insertions(+), 253 deletions(-)
--
1.7.3.4
^ permalink raw reply
* [PATCH v3 2/9] fsldma: move related helper functions near each other
From: Ira W. Snyder @ 2011-03-03 17:54 UTC (permalink / raw)
To: linuxppc-dev; +Cc: dan.j.williams, linux-kernel, Ira W. Snyder
In-Reply-To: <1299174901-16762-1-git-send-email-iws@ovro.caltech.edu>
This is a purely cosmetic cleanup. It is nice to have related functions
right next to each other in the code.
Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
---
drivers/dma/fsldma.c | 116 +++++++++++++++++++++++++++----------------------
1 files changed, 64 insertions(+), 52 deletions(-)
diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index 4de947a..2e1af45 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -39,33 +39,9 @@
static const char msg_ld_oom[] = "No free memory for link descriptor\n";
-static void dma_init(struct fsldma_chan *chan)
-{
- /* Reset the channel */
- DMA_OUT(chan, &chan->regs->mr, 0, 32);
-
- switch (chan->feature & FSL_DMA_IP_MASK) {
- case FSL_DMA_IP_85XX:
- /* Set the channel to below modes:
- * EIE - Error interrupt enable
- * EOSIE - End of segments interrupt enable (basic mode)
- * EOLNIE - End of links interrupt enable
- * BWC - Bandwidth sharing among channels
- */
- DMA_OUT(chan, &chan->regs->mr, FSL_DMA_MR_BWC
- | FSL_DMA_MR_EIE | FSL_DMA_MR_EOLNIE
- | FSL_DMA_MR_EOSIE, 32);
- break;
- case FSL_DMA_IP_83XX:
- /* Set the channel to below modes:
- * EOTIE - End-of-transfer interrupt enable
- * PRC_RM - PCI read multiple
- */
- DMA_OUT(chan, &chan->regs->mr, FSL_DMA_MR_EOTIE
- | FSL_DMA_MR_PRC_RM, 32);
- break;
- }
-}
+/*
+ * Register Helpers
+ */
static void set_sr(struct fsldma_chan *chan, u32 val)
{
@@ -77,6 +53,30 @@ static u32 get_sr(struct fsldma_chan *chan)
return DMA_IN(chan, &chan->regs->sr, 32);
}
+static void set_cdar(struct fsldma_chan *chan, dma_addr_t addr)
+{
+ DMA_OUT(chan, &chan->regs->cdar, addr | FSL_DMA_SNEN, 64);
+}
+
+static dma_addr_t get_cdar(struct fsldma_chan *chan)
+{
+ return DMA_IN(chan, &chan->regs->cdar, 64) & ~FSL_DMA_SNEN;
+}
+
+static dma_addr_t get_ndar(struct fsldma_chan *chan)
+{
+ return DMA_IN(chan, &chan->regs->ndar, 64);
+}
+
+static u32 get_bcr(struct fsldma_chan *chan)
+{
+ return DMA_IN(chan, &chan->regs->bcr, 32);
+}
+
+/*
+ * Descriptor Helpers
+ */
+
static void set_desc_cnt(struct fsldma_chan *chan,
struct fsl_dma_ld_hw *hw, u32 count)
{
@@ -113,24 +113,49 @@ static void set_desc_next(struct fsldma_chan *chan,
hw->next_ln_addr = CPU_TO_DMA(chan, snoop_bits | next, 64);
}
-static void set_cdar(struct fsldma_chan *chan, dma_addr_t addr)
+static void set_ld_eol(struct fsldma_chan *chan,
+ struct fsl_desc_sw *desc)
{
- DMA_OUT(chan, &chan->regs->cdar, addr | FSL_DMA_SNEN, 64);
-}
+ u64 snoop_bits;
-static dma_addr_t get_cdar(struct fsldma_chan *chan)
-{
- return DMA_IN(chan, &chan->regs->cdar, 64) & ~FSL_DMA_SNEN;
-}
+ snoop_bits = ((chan->feature & FSL_DMA_IP_MASK) == FSL_DMA_IP_83XX)
+ ? FSL_DMA_SNEN : 0;
-static dma_addr_t get_ndar(struct fsldma_chan *chan)
-{
- return DMA_IN(chan, &chan->regs->ndar, 64);
+ desc->hw.next_ln_addr = CPU_TO_DMA(chan,
+ DMA_TO_CPU(chan, desc->hw.next_ln_addr, 64) | FSL_DMA_EOL
+ | snoop_bits, 64);
}
-static u32 get_bcr(struct fsldma_chan *chan)
+/*
+ * DMA Engine Hardware Control Helpers
+ */
+
+static void dma_init(struct fsldma_chan *chan)
{
- return DMA_IN(chan, &chan->regs->bcr, 32);
+ /* Reset the channel */
+ DMA_OUT(chan, &chan->regs->mr, 0, 32);
+
+ switch (chan->feature & FSL_DMA_IP_MASK) {
+ case FSL_DMA_IP_85XX:
+ /* Set the channel to below modes:
+ * EIE - Error interrupt enable
+ * EOSIE - End of segments interrupt enable (basic mode)
+ * EOLNIE - End of links interrupt enable
+ * BWC - Bandwidth sharing among channels
+ */
+ DMA_OUT(chan, &chan->regs->mr, FSL_DMA_MR_BWC
+ | FSL_DMA_MR_EIE | FSL_DMA_MR_EOLNIE
+ | FSL_DMA_MR_EOSIE, 32);
+ break;
+ case FSL_DMA_IP_83XX:
+ /* Set the channel to below modes:
+ * EOTIE - End-of-transfer interrupt enable
+ * PRC_RM - PCI read multiple
+ */
+ DMA_OUT(chan, &chan->regs->mr, FSL_DMA_MR_EOTIE
+ | FSL_DMA_MR_PRC_RM, 32);
+ break;
+ }
}
static int dma_is_idle(struct fsldma_chan *chan)
@@ -185,19 +210,6 @@ static void dma_halt(struct fsldma_chan *chan)
dev_err(chan->dev, "DMA halt timeout!\n");
}
-static void set_ld_eol(struct fsldma_chan *chan,
- struct fsl_desc_sw *desc)
-{
- u64 snoop_bits;
-
- snoop_bits = ((chan->feature & FSL_DMA_IP_MASK) == FSL_DMA_IP_83XX)
- ? FSL_DMA_SNEN : 0;
-
- desc->hw.next_ln_addr = CPU_TO_DMA(chan,
- DMA_TO_CPU(chan, desc->hw.next_ln_addr, 64) | FSL_DMA_EOL
- | snoop_bits, 64);
-}
-
/**
* fsl_chan_set_src_loop_size - Set source address hold transfer size
* @chan : Freescale DMA channel
--
1.7.3.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox