* Re: [PATCH v6 0/8] ptp: IEEE 1588 hardware clock support
From: Alan Cox @ 2010-09-27 17:05 UTC (permalink / raw)
To: Christoph Lameter
Cc: John Stultz, Rodolfo Giometti, Arnd Bergmann, Peter Zijlstra,
linux-api, devicetree-discuss, linux-kernel, David Miller, netdev,
Thomas Gleixner, linuxppc-dev, Richard Cochran, linux-arm-kernel,
Krzysztof Halasa
In-Reply-To: <alpine.DEB.2.00.1009271054320.9258@router.home>
On Mon, 27 Sep 2010 10:56:09 -0500 (CDT)
Christoph Lameter <cl@linux.com> wrote:
>
> On Fri, 24 Sep 2010, Alan Cox wrote:
>
> > Whether you add new syscalls or do the fd passing using flags and hide
> > the ugly bits in glibc is another question.
>
> Use device specific ioctls instead of syscalls?
Some of the ioctls are probably not device specific, the job of the OS in
part is to present a unified interface. We already have a mess of HPET
and RTC driver ioctls.
Some of it undoubtedly is device specific.
Alan
^ permalink raw reply
* Re: [PATCH v6 0/8] ptp: IEEE 1588 hardware clock support
From: M. Warner Losh @ 2010-09-27 16:14 UTC (permalink / raw)
To: cl
Cc: richardcochran, peterz, johnstul, devicetree-discuss,
linuxppc-dev, linux-kernel, netdev, linux-api, tglx, giometti,
davem, linux-arm-kernel, khc
In-Reply-To: <alpine.DEB.2.00.1009271038150.9258@router.home>
In message: <alpine.DEB.2.00.1009271038150.9258@router.home>
Christoph Lameter <cl@linux.com> writes:
: On Thu, 23 Sep 2010, john stultz wrote:
: > The design actually avoids most userland induced latency.
: >
: > 1) On the PTP hardware syncing point, the reference packet gets
: > timestamped with the PTP hardware time on arrival. This allows the
: > offset calculation to be done in userland without introducing latency.
:
: The timestamps allows the calculation of the network transmission time I
: guess and therefore its more accurate to calculate that effect out. Ok but
: then the overhead of getting to code in user space (that does the proper
: clock adjustments) is resulting in the addition of a relatively long time
: that is subject to OS scheduling latencies and noises.
The timestamps at the hardware level allow you to factor out variation
caused by OS Scheduling, OS network stack delay and internal buffering
on the NIC. Variation in measurements is what kills accuracy.
When steering a clock by making an error measurement of the phase and
frequency of it, the latency induced by OS scheduling tends to be
unimportant. It is far more important to know when you steered the
clock (called adjtime or friends) than to steer it at any fixed
latency to when the data for the measurements was made. Measuring the
time of steer can tolerate errors in the range of OS scheduling
latencies easily, since that tends to produce a very small effect. It
introduces an error in your expected phase for the next measurement on
the order of the product of the time of steer error times the change
in fractional frequency (abs( 1 - (nu_new / nu_old))). Even if the
estimate is really bad at 100ms, most steers are on the order about
one part per million. This leads to a sub-nanosecond phase error
estimate in the next measurement cycle (a non-accumulating error). A
1ms error leads to maybe tens of picoseconds of estimate error.
This is a common error that I've seen repeated in this thread. The
only reason that it has historically been important is because when
you are doing timestamping in software based on an interrupt, that
stuff does matter.
Warner
^ permalink raw reply
* Re: [PATCH v6 0/8] ptp: IEEE 1588 hardware clock support
From: Christoph Lameter @ 2010-09-27 15:56 UTC (permalink / raw)
To: Alan Cox
Cc: John Stultz, Rodolfo Giometti, Arnd Bergmann, Peter Zijlstra,
linux-api, devicetree-discuss, linux-kernel, David Miller, netdev,
Thomas Gleixner, linuxppc-dev, Richard Cochran, linux-arm-kernel,
Krzysztof Halasa
In-Reply-To: <20100924150246.0e6064b6@lxorguk.ukuu.org.uk>
On Fri, 24 Sep 2010, Alan Cox wrote:
> Whether you add new syscalls or do the fd passing using flags and hide
> the ugly bits in glibc is another question.
Use device specific ioctls instead of syscalls?
^ permalink raw reply
* Re: [PATCH v6 0/8] ptp: IEEE 1588 hardware clock support
From: Christoph Lameter @ 2010-09-27 15:52 UTC (permalink / raw)
To: john stultz
Cc: Rodolfo Giometti, Arnd Bergmann, Peter Zijlstra, linux-api,
devicetree-discuss, linux-kernel, David Miller, netdev,
Thomas Gleixner, linuxppc-dev, Richard Cochran, linux-arm-kernel,
Krzysztof Halasa
In-Reply-To: <1285278136.2587.154.camel@localhost.localdomain>
On Thu, 23 Sep 2010, john stultz wrote:
> > > 3) Further, the PTP hardware counter can be simply set to a new offset
> > > to put it in line with the network time. This could cause trouble with
> > > timekeeping much like unsynced TSCs do.
> >
> > You can do the same for system time.
>
> Settimeofday does allow CLOCK_REALTIME to jump, but the CLOCK_MONOTONIC
> time cannot jump around. Having a clocksource that is non-monotonic
> would break this.
Currently time runs at the same speed. CLOCK_MONOTONIC runs at a offset
to CLOCK_REALTIME. We are creating APIs here that allow time to run at
different speeds.
> The design actually avoids most userland induced latency.
>
> 1) On the PTP hardware syncing point, the reference packet gets
> timestamped with the PTP hardware time on arrival. This allows the
> offset calculation to be done in userland without introducing latency.
The timestamps allows the calculation of the network transmission time I
guess and therefore its more accurate to calculate that effect out. Ok but
then the overhead of getting to code in user space (that does the proper
clock adjustments) is resulting in the addition of a relatively long time
that is subject to OS scheduling latencies and noises.
> 2) On the system syncing side, the proposal for the PPS interrupt allows
> the PTP hardware to trigger an interrupt on the second boundary that
> would take a timestamp of the system time. Then the pps interface allows
> for the timestamp to be read from userland allowing the offset to be
> calculated without introducing additional latency.
Sorry dont really get the whole picture here it seems. Sounds like one is
going through additional unnecessary layers. Why would the PTP hardware
triggger an interrupt? I thought the PTP messages came in via
timestamping and are then processed by software. Then the software is
issuing a hardware interrupt that then triggers the PPS subsystem. And
that is supposed to be better than directly interfacing with the PTP?
> Additionally, even just in userland, it would be easy to bracket two
> reads of the system time around one read of the PTP clock to bound any
> userland latency fairly well. It may not be as good as the PPS interface
> (although that depends on the interrupt latency), but if the accesses
> are all local, it probably could get fairly close.
That sounds hacky.
> > Ok maybe we need some sort of control interface to manage the clock like
> > the others have.
>
> That's what the clock_adjtime call provides.
Ummm... You are managing a hardware device with hardware (driver) specific
settings. That is currently being done via ioctls. Why generalize it?
> > The posix clocks today assumes one notion of real "time" in the kernel.
> > All clocks increase in lockstep (aside from offset updates).
>
> Not true. The cputime clockids do not increment at the same rate (as the
> apps don't always run). Further CLOCK_MONOTONIC_RAW provides a non-freq
> corrected view of CLOCK_MONOTONIC, so it increments at a slightly
> different rate.
cputime clockids are not tracking time but cpu resource use.
> Re-using the fairly nice (Alan of course disagrees :) posix interface
> seems at least a little better for application developers who actually
> have to use the hardware.
Well it may also be confusing for others. The application developers also
will have a hard time using a generic clock interface to control PTP
device specific things like frequencies, rates etc etc. So you always need
to ioctl/device specific control interface regardless.
^ permalink raw reply
* Re: [PATCH v6 0/8] ptp: IEEE 1588 hardware clock support
From: Christoph Lameter @ 2010-09-27 15:37 UTC (permalink / raw)
To: Christian Riesch
Cc: Rodolfo Giometti, Arnd Bergmann, Peter Zijlstra, john stultz,
devicetree-discuss, linux-kernel, David Miller, netdev,
linux-arm-kernel, linux-api, Thomas Gleixner, linuxppc-dev,
Richard Cochran, Alan Cox, Krzysztof Halasa
In-Reply-To: <4C9BC7CE.8020400@riesch.at>
On Thu, 23 Sep 2010, Christian Riesch wrote:
> > > It implies clock tuning in userspace for a potential sub microsecond
> > > accurate clock. The clock accuracy will be limited by user space
> > > latencies and noise. You wont be able to discipline the system clock
> > > accurately.
> >
> > Noise matters, latency doesn't.
>
> Well put! That's why we need hardware support for PTP timestamping to reduce
> the noise, but get along well with the clock servo that is steering the PHC in
> user space.
Even if I buy into the catch phrase above: User space is subject to noise
that the in kernel code is not. If you do the tuning over long intervals
then it hopefully averages out but it still causes jitter effects that
affects the degree of accuracy (or sync) that you can reach. And the noise
varies with the load on the system.
^ permalink raw reply
* Re: [PATCH 1/2] 476: Set CCR2[DSTI] to prevent isync from flushing shadow TLB
From: Dave Kleikamp @ 2010-09-27 15:26 UTC (permalink / raw)
To: Josh Boyer; +Cc: linuxppc-dev list
In-Reply-To: <20100927150434.GA2888@zod.rchland.ibm.com>
On Mon, 2010-09-27 at 11:04 -0400, Josh Boyer wrote:
> On Fri, Sep 24, 2010 at 01:01:36PM -0500, Dave Kleikamp wrote:
> >When the DSTI (Disable Shadow TLB Invalidate) bit is set in the CCR2
> >register, the isync command does not flush the shadow TLB (iTLB & dTLB).
> >
> >However, since the shadow TLB does not contain context information, we
> >want the shadow TLB flushed in situations where we are switching context.
> >In those situations, we explicitly clear the DSTI bit before performing
> >isync, and set it again afterward. We also need to do the same when we
> >perform isync after explicitly flushing the TLB.
> >
> >Th setting of the DSTI bit is dependent on
> >CONFIG_PPC_47x_DISABLE_SHADOW_TLB_INVALIDATE. When we are confident that
> >the feature works as expected, the option can probably be removed.
>
> You're defaulting it to 'y' in the Kconfig. Technically someone could
> turn it off I guess, but practice mostly shows that nobody mucks with
> the defaults. Do you want it to default 'n' for now if you aren't
> confident in it just quite yet?
I think I made it a config option at Ben's request when I first started
this work last year, before being sidetracked by other priorities. I
could either remove the option, or default it to 'n'. It might be best
to just hard-code the behavior to make sure it's exercised, since
there's no 47x hardware in production yet, but we can give Ben a chance
to weigh in with his opinion.
> (Linus also has some kind of gripe with new options being default 'y',
> but I don't recall all the details and I doubt he'd care about something
> in low-level PPC code.)
>
> josh
--
Dave Kleikamp
IBM Linux Technology Center
^ permalink raw reply
* Re: [PATCH RFCv2 1/2] dmaengine: add support for scatterlist to scatterlist transfers
From: Linus Walleij @ 2010-09-27 15:23 UTC (permalink / raw)
To: Ira W. Snyder; +Cc: Per Fridén, Dan Williams, linuxppc-dev, linux-kernel
In-Reply-To: <1285370032-16937-2-git-send-email-iws@ovro.caltech.edu>
2010/9/25 Ira W. Snyder <iws@ovro.caltech.edu>:
> This adds support for scatterlist to scatterlist DMA transfers.
This is a good idea, we have a local function to do this in DMA40 already,
stedma40_memcpy_sg().
> This is
> currently hidden behind a configuration option, which will allow drivers
> which need this functionality to select it individually.
Why? Isn't it better to add this as a new capability flag
if you don't want to announce it? Or is the intent to save
memory footprint?
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 1/2] 476: Set CCR2[DSTI] to prevent isync from flushing shadow TLB
From: Josh Boyer @ 2010-09-27 15:04 UTC (permalink / raw)
To: Dave Kleikamp; +Cc: linuxppc-dev list
In-Reply-To: <1285351297-9999-2-git-send-email-shaggy@linux.vnet.ibm.com>
On Fri, Sep 24, 2010 at 01:01:36PM -0500, Dave Kleikamp wrote:
>When the DSTI (Disable Shadow TLB Invalidate) bit is set in the CCR2
>register, the isync command does not flush the shadow TLB (iTLB & dTLB).
>
>However, since the shadow TLB does not contain context information, we
>want the shadow TLB flushed in situations where we are switching context.
>In those situations, we explicitly clear the DSTI bit before performing
>isync, and set it again afterward. We also need to do the same when we
>perform isync after explicitly flushing the TLB.
>
>Th setting of the DSTI bit is dependent on
>CONFIG_PPC_47x_DISABLE_SHADOW_TLB_INVALIDATE. When we are confident that
>the feature works as expected, the option can probably be removed.
You're defaulting it to 'y' in the Kconfig. Technically someone could
turn it off I guess, but practice mostly shows that nobody mucks with
the defaults. Do you want it to default 'n' for now if you aren't
confident in it just quite yet?
(Linus also has some kind of gripe with new options being default 'y',
but I don't recall all the details and I doubt he'd care about something
in low-level PPC code.)
josh
^ permalink raw reply
* Re: Oops in trace_hardirqs_on (powerpc)
From: Jörg Sommer @ 2010-09-27 12:50 UTC (permalink / raw)
To: Steven Rostedt
Cc: Frederic Weisbecker, Ingo Molnar, linux-kernel, linuxppc-dev
In-Reply-To: <1285184671.26872.127.camel@gandalf.stny.rr.com>
[-- Attachment #1: Type: text/plain, Size: 720 bytes --]
Hello Steven,
Steven Rostedt hat am Wed 22. Sep, 15:44 (-0400) geschrieben:
> Sorry for the late reply, but I was on vacation when you sent this, and
> I missed it while going through email.
>
> Do you still have this issue?
No. I've rebuild my kernel without TRACE_IRQFLAGS and the problem
vanished, as expected. The problem is, that in some cases the stack is
only two frames deep, which causes the macro CALLER_ADDR1 makes an
invalid access. Someone told me, there a workaround for the problem on
i386, too.
% sed -n 2p arch/x86/lib/thunk_32.S
* Trampoline to trace irqs off. (otherwise CALLER_ADDR1 might crash)
Bye, Jörg.
--
Angenehme Worte sind nie wahr,
wahre Worte sind nie angenehm.
[-- Attachment #2: Digital signature http://en.wikipedia.org/wiki/OpenPGP --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: MPC8641D PEX: programming OWBAR in Endpoint mode?
From: tiejun.chen @ 2010-09-26 10:14 UTC (permalink / raw)
To: David Hagood; +Cc: linuxppc-dev
In-Reply-To: <1285415473.2454.15.camel@chumley>
David Hagood wrote:
> On Sat, 2010-09-25 at 17:46 +0800, tiejun.chen wrote:
>> As a summary you have no any issue to access InBound/BAR/LAW.
>
> Correct: full access to the inbound.
>> And I remember there is only one
>> requirement to OutBound, and that is the window address should be aligned based
>> on the size from OWS. Are you sure?
> Yes. As my first test I've been using 4k and 64k sizes, aligned on 16M
> boundaries.
>> And did you try to configure OutBound REGS when RC mode? If so I'm afraid we may
>> miss some errata on EP.
> In RC mode the built-in drivers take over.
Can you confirm the build-in driver can write/read OutBound correctly on your
target?
-Tiejun
>
>
>
^ permalink raw reply
* Re: [PATCH 1/1] Add config option for batched hcalls
From: Olof Johansson @ 2010-09-26 3:49 UTC (permalink / raw)
To: Will Schmidt; +Cc: linuxppc-dev, Anton Blanchard
In-Reply-To: <1285364655.3016.25.camel@lexx>
On Fri, Sep 24, 2010 at 04:44:15PM -0500, Will Schmidt wrote:
>
> Add a config option for the (batched) MULTITCE and BULK_REMOVE h-calls.
>
> By default, these options are on and are beneficial for performance and
> throughput reasons. If disabled, the code will fall back to using less
> optimal TCE and REMOVE hcalls. The ability to easily disable these
> options is useful for some of the PREEMPT_RT related investigation and
> work occurring on Power.
Hi,
I can see why it's useful to enable and disable, but these are all
runtime-checked, wouldn't it be more useful to add a bootarg to handle
it instead of adding some new config options that pretty much everyone
will always go with the defaults on?
The bits are set early, but from looking at where they're used, there
doesn't seem to be any harm in disabling them later on when a bootarg
is convenient to parse and deal with?
It has the benefit of easier on/off testing, if that has any value for
production debug down the road.
-Olof
^ permalink raw reply
* Re: [PATCH] powerpc/5121: pdm360ng: fix touch irq if 8xxx gpio driver is enabled
From: Anatolij Gustschin @ 2010-09-25 20:22 UTC (permalink / raw)
To: Grant Likely; +Cc: linuxppc-dev
In-Reply-To: <20100916023823.GB7049@angua.secretlab.ca>
On Wed, 15 Sep 2010 20:38:23 -0600
Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, Sep 15, 2010 at 10:12:57PM +0200, Anatolij Gustschin wrote:
> > Enabling the MPC8xxx GPIO driver with MPC512x GPIO extension
> > breaks touch screen support on this board since the GPIO
> > interrupt will be mapped to 8xxx GPIO irq host resulting in
> > a not requestable interrupt in the touch screen driver. Fix
> > it by mapping the touch interrupt on 8xxx GPIO irq host.
>
> This looks wrong to me. The touchscreen code should not go mucking
> about in the GPIO controller registers; that is the job of the gpio
> driver.
But if there is no GPIO driver (as it was the case before adding
mpc512x support in the 8xxx gpio driver) or if the driver is not
enabled in the kernel configuration? Then the platform specific
callback (called from touchscreen driver) returns the pin state
and acknowlegdes the interrupt.
> What is the reason that the touch interrupt isn't
> requestable?
The 8xxx gpio driver sets up gpio irq host and installs
the chained irq handler for GPIO interrupt 78 using
set_irq_chained_handler() which sets the status field of
the irq_desc structure to IRQ_NOREQUEST | IRQ_NOPROBE.
Other drivers can't request this GPIO interrupt any more,
request_threaded_irq() checks the IRQ_NOREQUEST status
flag and returns -EINVAL if it is set. The gpio interrupts
for each gpio pin are now handled by the
mpc8xxx_gpio_irq_cascade() handler as they should.
> It looks like the 8xxx gpio driver is designed to hand
> out a separate virq number for each gpio pin (I've not had time to dig
> into details, so you'll need to educate me on the problem details)
Yes, exactly. This patch adds code to request the
board's pen_down gpio pin and to use it's virq number in
the touchscreen driver. The touchscreen driver can
request this virq interrupt and it is now properly handled
by the chained handler in the gpio driver.
Anatolij
^ permalink raw reply
* Re: MPC8641D PEX: programming OWBAR in Endpoint mode?
From: David Hagood @ 2010-09-25 11:51 UTC (permalink / raw)
To: tiejun.chen; +Cc: linuxppc-dev
In-Reply-To: <4C9DC510.8010403@windriver.com>
On Sat, 2010-09-25 at 17:46 +0800, tiejun.chen wrote:
> As a summary you have no any issue to access InBound/BAR/LAW.
Correct: full access to the inbound.
> And I remember there is only one
> requirement to OutBound, and that is the window address should be aligned based
> on the size from OWS. Are you sure?
Yes. As my first test I've been using 4k and 64k sizes, aligned on 16M
boundaries.
>
> And did you try to configure OutBound REGS when RC mode? If so I'm afraid we may
> miss some errata on EP.
In RC mode the built-in drivers take over.
^ permalink raw reply
* Re: MPC8641D PEX: programming OWBAR in Endpoint mode?
From: tiejun.chen @ 2010-09-25 9:46 UTC (permalink / raw)
To: David Hagood; +Cc: linuxppc-dev
In-Reply-To: <1285325425.2454.11.camel@chumley>
David Hagood wrote:
> On Fri, 2010-09-24 at 07:09 +0200, Chen, Tiejun wrote:
>
>> Right but this should be done for RC mode, not for EP mode we're
>> discussing.
>>
>> Tiejun
>
> According to the Freescale documentation, outbound is just as valid for
> endpoint as for root complex - indeed, to generate MSIs from software
Yes.
As a summary you have no any issue to access InBound/BAR/LAW. And actually I'm
always strange that even you cannot configure InBound/BAR/LAW properly, you also
should be allowed to write OutBound REGs. And I remember there is only one
requirement to OutBound, and that is the window address should be aligned based
on the size from OWS. Are you sure?
And did you try to configure OutBound REGS when RC mode? If so I'm afraid we may
miss some errata on EP.
And do you have any response from Freescale?
Cheers
Tiejun
> REQUIRES programming an outbound ATMU to access the host's APIC.
>
> Moreover, ANY PCI endpoint SHOULD be able to do bus master access, and
> that is done by the outbound ATMUs.
>
>
>
^ permalink raw reply
* [PATCH RFCv2 2/2] fsldma: use generic support for scatterlist to scatterlist transfers
From: Ira W. Snyder @ 2010-09-24 23:13 UTC (permalink / raw)
To: linux-kernel; +Cc: Dan Williams, linuxppc-dev
In-Reply-To: <1285370032-16937-1-git-send-email-iws@ovro.caltech.edu>
The fsldma driver uses the DMA_SLAVE API to handle scatterlist to
scatterlist DMA transfers. For quite a while now, it has been possible
to mimic the operation by using the device_prep_dma_memcpy() routine
intelligently.
Now that the DMAEngine API has grown generic support for scatterlist to
scatterlist transfers, this operation is no longer needed. The generic
support is used for scatterlist to scatterlist transfers.
Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
---
arch/powerpc/include/asm/fsldma.h | 115 ++------------------
drivers/dma/fsldma.c | 219 +++++++------------------------------
2 files changed, 48 insertions(+), 286 deletions(-)
diff --git a/arch/powerpc/include/asm/fsldma.h b/arch/powerpc/include/asm/fsldma.h
index debc5ed..dc0bd27 100644
--- a/arch/powerpc/include/asm/fsldma.h
+++ b/arch/powerpc/include/asm/fsldma.h
@@ -1,7 +1,7 @@
/*
* Freescale MPC83XX / MPC85XX DMA Controller
*
- * Copyright (c) 2009 Ira W. Snyder <iws@ovro.caltech.edu>
+ * Copyright (c) 2009-2010 Ira W. Snyder <iws@ovro.caltech.edu>
*
* This file is licensed under the terms of the GNU General Public License
* version 2. This program is licensed "as is" without any warranty of any
@@ -11,127 +11,32 @@
#ifndef __ARCH_POWERPC_ASM_FSLDMA_H__
#define __ARCH_POWERPC_ASM_FSLDMA_H__
-#include <linux/slab.h>
#include <linux/dmaengine.h>
/*
- * Definitions for the Freescale DMA controller's DMA_SLAVE implemention
+ * The Freescale DMA controller has several features that are not accomodated
+ * in the Linux DMAEngine API. Therefore, the generic structure is expanded
+ * to allow drivers to use these features.
*
- * The Freescale DMA_SLAVE implementation was designed to handle many-to-many
- * transfers. An example usage would be an accelerated copy between two
- * scatterlists. Another example use would be an accelerated copy from
- * multiple non-contiguous device buffers into a single scatterlist.
+ * This structure should be passed into the DMAEngine routine device_control()
+ * as in this example:
*
- * A DMA_SLAVE transaction is defined by a struct fsl_dma_slave. This
- * structure contains a list of hardware addresses that should be copied
- * to/from the scatterlist passed into device_prep_slave_sg(). The structure
- * also has some fields to enable hardware-specific features.
+ * chan->device->device_control(chan, DMA_SLAVE_CONFIG, (unsigned long)cfg);
*/
/**
- * struct fsl_dma_hw_addr
- * @entry: linked list entry
- * @address: the hardware address
- * @length: length to transfer
- *
- * Holds a single physical hardware address / length pair for use
- * with the DMAEngine DMA_SLAVE API.
- */
-struct fsl_dma_hw_addr {
- struct list_head entry;
-
- dma_addr_t address;
- size_t length;
-};
-
-/**
* struct fsl_dma_slave
- * @addresses: a linked list of struct fsl_dma_hw_addr structures
+ * @config: the standard Linux DMAEngine API DMA_SLAVE configuration
* @request_count: value for DMA request count
- * @src_loop_size: setup and enable constant source-address DMA transfers
- * @dst_loop_size: setup and enable constant destination address DMA transfers
* @external_start: enable externally started DMA transfers
* @external_pause: enable externally paused DMA transfers
- *
- * Holds a list of address / length pairs for use with the DMAEngine
- * DMA_SLAVE API implementation for the Freescale DMA controller.
*/
-struct fsl_dma_slave {
+struct fsldma_slave_config {
+ struct dma_slave_config config;
- /* List of hardware address/length pairs */
- struct list_head addresses;
-
- /* Support for extra controller features */
unsigned int request_count;
- unsigned int src_loop_size;
- unsigned int dst_loop_size;
bool external_start;
bool external_pause;
};
-/**
- * fsl_dma_slave_append - add an address/length pair to a struct fsl_dma_slave
- * @slave: the &struct fsl_dma_slave to add to
- * @address: the hardware address to add
- * @length: the length of bytes to transfer from @address
- *
- * Add a hardware address/length pair to a struct fsl_dma_slave. Returns 0 on
- * success, -ERRNO otherwise.
- */
-static inline int fsl_dma_slave_append(struct fsl_dma_slave *slave,
- dma_addr_t address, size_t length)
-{
- struct fsl_dma_hw_addr *addr;
-
- addr = kzalloc(sizeof(*addr), GFP_ATOMIC);
- if (!addr)
- return -ENOMEM;
-
- INIT_LIST_HEAD(&addr->entry);
- addr->address = address;
- addr->length = length;
-
- list_add_tail(&addr->entry, &slave->addresses);
- return 0;
-}
-
-/**
- * fsl_dma_slave_free - free a struct fsl_dma_slave
- * @slave: the struct fsl_dma_slave to free
- *
- * Free a struct fsl_dma_slave and all associated address/length pairs
- */
-static inline void fsl_dma_slave_free(struct fsl_dma_slave *slave)
-{
- struct fsl_dma_hw_addr *addr, *tmp;
-
- if (slave) {
- list_for_each_entry_safe(addr, tmp, &slave->addresses, entry) {
- list_del(&addr->entry);
- kfree(addr);
- }
-
- kfree(slave);
- }
-}
-
-/**
- * fsl_dma_slave_alloc - allocate a struct fsl_dma_slave
- * @gfp: the flags to pass to kmalloc when allocating this structure
- *
- * Allocate a struct fsl_dma_slave for use by the DMA_SLAVE API. Returns a new
- * struct fsl_dma_slave on success, or NULL on failure.
- */
-static inline struct fsl_dma_slave *fsl_dma_slave_alloc(gfp_t gfp)
-{
- struct fsl_dma_slave *slave;
-
- slave = kzalloc(sizeof(*slave), gfp);
- if (!slave)
- return NULL;
-
- INIT_LIST_HEAD(&slave->addresses);
- return slave;
-}
-
#endif /* __ARCH_POWERPC_ASM_FSLDMA_H__ */
diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index cea08be..c90b393 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -599,207 +599,64 @@ static struct dma_async_tx_descriptor *fsl_dma_prep_slave_sg(
struct dma_chan *dchan, struct scatterlist *sgl, unsigned int sg_len,
enum dma_data_direction direction, unsigned long flags)
{
- struct fsldma_chan *chan;
- struct fsl_desc_sw *first = NULL, *prev = NULL, *new = NULL;
- struct fsl_dma_slave *slave;
- size_t copy;
-
- int i;
- struct scatterlist *sg;
- size_t sg_used;
- size_t hw_used;
- struct fsl_dma_hw_addr *hw;
- dma_addr_t dma_dst, dma_src;
-
- if (!dchan)
- return NULL;
-
- if (!dchan->private)
- return NULL;
-
- chan = to_fsl_chan(dchan);
- slave = dchan->private;
-
- if (list_empty(&slave->addresses))
- return NULL;
-
- hw = list_first_entry(&slave->addresses, struct fsl_dma_hw_addr, entry);
- hw_used = 0;
-
/*
- * Build the hardware transaction to copy from the scatterlist to
- * the hardware, or from the hardware to the scatterlist
+ * This operation is not supported on the Freescale DMA controller
*
- * If you are copying from the hardware to the scatterlist and it
- * takes two hardware entries to fill an entire page, then both
- * hardware entries will be coalesced into the same page
- *
- * If you are copying from the scatterlist to the hardware and a
- * single page can fill two hardware entries, then the data will
- * be read out of the page into the first hardware entry, and so on
+ * However, we need to provide the function pointer to allow the
+ * device_control() method to work.
*/
- for_each_sg(sgl, sg, sg_len, i) {
- sg_used = 0;
-
- /* Loop until the entire scatterlist entry is used */
- while (sg_used < sg_dma_len(sg)) {
-
- /*
- * If we've used up the current hardware address/length
- * pair, we need to load a new one
- *
- * This is done in a while loop so that descriptors with
- * length == 0 will be skipped
- */
- while (hw_used >= hw->length) {
-
- /*
- * If the current hardware entry is the last
- * entry in the list, we're finished
- */
- if (list_is_last(&hw->entry, &slave->addresses))
- goto finished;
-
- /* Get the next hardware address/length pair */
- hw = list_entry(hw->entry.next,
- struct fsl_dma_hw_addr, entry);
- hw_used = 0;
- }
-
- /* Allocate the link descriptor from DMA pool */
- new = fsl_dma_alloc_descriptor(chan);
- if (!new) {
- dev_err(chan->dev, "No free memory for "
- "link descriptor\n");
- goto fail;
- }
-#ifdef FSL_DMA_LD_DEBUG
- dev_dbg(chan->dev, "new link desc alloc %p\n", new);
-#endif
-
- /*
- * Calculate the maximum number of bytes to transfer,
- * making sure it is less than the DMA controller limit
- */
- copy = min_t(size_t, sg_dma_len(sg) - sg_used,
- hw->length - hw_used);
- copy = min_t(size_t, copy, FSL_DMA_BCR_MAX_CNT);
-
- /*
- * DMA_FROM_DEVICE
- * from the hardware to the scatterlist
- *
- * DMA_TO_DEVICE
- * from the scatterlist to the hardware
- */
- if (direction == DMA_FROM_DEVICE) {
- dma_src = hw->address + hw_used;
- dma_dst = sg_dma_address(sg) + sg_used;
- } else {
- dma_src = sg_dma_address(sg) + sg_used;
- dma_dst = hw->address + hw_used;
- }
-
- /* Fill in the descriptor */
- set_desc_cnt(chan, &new->hw, copy);
- set_desc_src(chan, &new->hw, dma_src);
- set_desc_dst(chan, &new->hw, dma_dst);
-
- /*
- * If this is not the first descriptor, chain the
- * current descriptor after the previous descriptor
- */
- if (!first) {
- first = new;
- } else {
- set_desc_next(chan, &prev->hw,
- new->async_tx.phys);
- }
-
- new->async_tx.cookie = 0;
- async_tx_ack(&new->async_tx);
-
- prev = new;
- sg_used += copy;
- hw_used += copy;
-
- /* Insert the link descriptor into the LD ring */
- list_add_tail(&new->node, &first->tx_list);
- }
- }
-
-finished:
-
- /* All of the hardware address/length pairs had length == 0 */
- if (!first || !new)
- return NULL;
-
- new->async_tx.flags = flags;
- new->async_tx.cookie = -EBUSY;
-
- /* Set End-of-link to the last link descriptor of new list */
- set_ld_eol(chan, new);
-
- /* Enable extra controller features */
- if (chan->set_src_loop_size)
- chan->set_src_loop_size(chan, slave->src_loop_size);
-
- if (chan->set_dst_loop_size)
- chan->set_dst_loop_size(chan, slave->dst_loop_size);
-
- if (chan->toggle_ext_start)
- chan->toggle_ext_start(chan, slave->external_start);
-
- if (chan->toggle_ext_pause)
- chan->toggle_ext_pause(chan, slave->external_pause);
-
- if (chan->set_request_count)
- chan->set_request_count(chan, slave->request_count);
-
- return &first->async_tx;
-
-fail:
- /* If first was not set, then we failed to allocate the very first
- * descriptor, and we're done */
- if (!first)
- return NULL;
-
- /*
- * First is set, so all of the descriptors we allocated have been added
- * to first->tx_list, INCLUDING "first" itself. Therefore we
- * must traverse the list backwards freeing each descriptor in turn
- *
- * We're re-using variables for the loop, oh well
- */
- fsldma_free_desc_list_reverse(chan, &first->tx_list);
return NULL;
}
static int fsl_dma_device_control(struct dma_chan *dchan,
enum dma_ctrl_cmd cmd, unsigned long arg)
{
+ struct fsldma_slave_config *cfg;
struct fsldma_chan *chan;
unsigned long flags;
- /* Only supports DMA_TERMINATE_ALL */
- if (cmd != DMA_TERMINATE_ALL)
- return -ENXIO;
-
if (!dchan)
return -EINVAL;
chan = to_fsl_chan(dchan);
- /* Halt the DMA engine */
- dma_halt(chan);
+ switch (cmd) {
+ case DMA_TERMINATE_ALL:
+ /* Halt the DMA engine */
+ dma_halt(chan);
- spin_lock_irqsave(&chan->desc_lock, flags);
+ 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);
+ /* 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);
- spin_unlock_irqrestore(&chan->desc_lock, flags);
+ spin_unlock_irqrestore(&chan->desc_lock, flags);
+ return 0;
+
+ case DMA_SLAVE_CONFIG:
+
+ cfg = (struct fsldma_slave_config *)arg;
+ if (chan->set_request_count)
+ chan->set_request_count(chan, cfg->request_count);
+
+ if (chan->toggle_ext_start)
+ chan->toggle_ext_start(chan, cfg->external_start);
+
+ if (chan->toggle_ext_pause)
+ chan->toggle_ext_pause(chan, cfg->external_pause);
+
+ /*
+ * TODO: add other features
+ *
+ * I'm not sure how to use the members dma_slave_config to
+ * control the src/dst address hold features.
+ */
+ return 0;
+
+ default:
+ return -ENXIO;
+ }
return 0;
}
--
1.7.1
^ permalink raw reply related
* [PATCH RFCv2 1/2] dmaengine: add support for scatterlist to scatterlist transfers
From: Ira W. Snyder @ 2010-09-24 23:13 UTC (permalink / raw)
To: linux-kernel; +Cc: Dan Williams, linuxppc-dev
In-Reply-To: <1285370032-16937-1-git-send-email-iws@ovro.caltech.edu>
This adds support for scatterlist to scatterlist DMA transfers. This is
currently hidden behind a configuration option, which will allow drivers
which need this functionality to select it individually.
Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
---
drivers/dma/Kconfig | 3 +
drivers/dma/dmaengine.c | 125 +++++++++++++++++++++++++++++++++++++++++++++
include/linux/dmaengine.h | 6 ++
3 files changed, 134 insertions(+), 0 deletions(-)
diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 9520cf0..82d2244 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -89,6 +89,9 @@ config AT_HDMAC
Support the Atmel AHB DMA controller. This can be integrated in
chips such as the Atmel AT91SAM9RL.
+config DMAENGINE_SG_TO_SG
+ bool
+
config FSL_DMA
tristate "Freescale Elo and Elo Plus DMA support"
depends on FSL_SOC
diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 9d31d5e..9238b86 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -972,6 +972,131 @@ dma_async_memcpy_pg_to_pg(struct dma_chan *chan, struct page *dest_pg,
}
EXPORT_SYMBOL(dma_async_memcpy_pg_to_pg);
+#ifdef CONFIG_DMAENGINE_SG_TO_SG
+dma_cookie_t
+dma_async_memcpy_sg_to_sg(struct dma_chan *chan,
+ struct scatterlist *dst_sg, unsigned int dst_nents,
+ struct scatterlist *src_sg, unsigned int src_nents,
+ dma_async_tx_callback cb, void *cb_param)
+{
+ struct dma_device *dev = chan->device;
+ struct dma_async_tx_descriptor *tx;
+ dma_cookie_t cookie = -ENOMEM;
+ size_t dst_avail, src_avail;
+ struct scatterlist *sg;
+ size_t transferred = 0;
+ size_t dst_total = 0;
+ size_t src_total = 0;
+ dma_addr_t dst, src;
+ size_t len;
+ int i;
+
+ if (dst_nents == 0 || src_nents == 0)
+ return -EINVAL;
+
+ if (dst_sg == NULL || src_sg == NULL)
+ return -EINVAL;
+
+ /* get the total count of bytes in each scatterlist */
+ for_each_sg(dst_sg, sg, dst_nents, i)
+ dst_total += sg_dma_len(sg);
+
+ for_each_sg(src_sg, sg, src_nents, i)
+ src_total += sg_dma_len(sg);
+
+ /* get prepared for the loop */
+ dst_avail = sg_dma_len(dst_sg);
+ src_avail = sg_dma_len(src_sg);
+
+ /* run until we are out of descriptors */
+ while (true) {
+
+ /* create the largest transaction possible */
+ len = min_t(size_t, src_avail, dst_avail);
+ if (len == 0)
+ goto fetch;
+
+ dst = sg_dma_address(dst_sg) + sg_dma_len(dst_sg) - dst_avail;
+ src = sg_dma_address(src_sg) + sg_dma_len(src_sg) - src_avail;
+
+ /*
+ * get a descriptor
+ *
+ * we must poll for a descriptor here since the DMAEngine API
+ * does not provide a way for external users to free previously
+ * allocated descriptors
+ */
+ for (;;) {
+ tx = dev->device_prep_dma_memcpy(chan, dst, src, len, 0);
+ if (likely(tx))
+ break;
+
+ dma_async_issue_pending(chan);
+ }
+
+ /* update metadata */
+ transferred += len;
+ dst_avail -= len;
+ src_avail -= len;
+
+ /* if this is the last transfer, setup the callback */
+ if (dst_total == transferred || src_total == transferred) {
+ tx->callback = cb;
+ tx->callback_param = cb_param;
+ }
+
+ /* submit the transaction */
+ cookie = tx->tx_submit(tx);
+ if (dma_submit_error(cookie)) {
+ dev_err(dev->dev, "failed to submit desc\n");
+ return cookie;
+ }
+
+fetch:
+ /* fetch the next dst scatterlist entry */
+ if (dst_avail == 0) {
+
+ /* no more entries: we're done */
+ if (dst_nents == 0)
+ break;
+
+ /* fetch the next entry: if there are no more: done */
+ dst_sg = sg_next(dst_sg);
+ if (dst_sg == NULL)
+ break;
+
+ dst_nents--;
+ dst_avail = sg_dma_len(dst_sg);
+ }
+
+ /* fetch the next src scatterlist entry */
+ if (src_avail == 0) {
+
+ /* no more entries: we're done */
+ if (src_nents == 0)
+ break;
+
+ /* fetch the next entry: if there are no more: done */
+ src_sg = sg_next(src_sg);
+ if (src_sg == NULL)
+ break;
+
+ src_nents--;
+ src_avail = sg_dma_len(src_sg);
+ }
+ }
+
+ /* update counters */
+ preempt_disable();
+ __this_cpu_add(chan->local->bytes_transferred, transferred);
+ __this_cpu_inc(chan->local->memcpy_count);
+ preempt_enable();
+
+ return 0;
+}
+EXPORT_SYMBOL(dma_async_memcpy_sg_to_sg);
+#endif
+
void dma_async_tx_descriptor_init(struct dma_async_tx_descriptor *tx,
struct dma_chan *chan)
{
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index c61d4ca..28803a0 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -632,6 +632,12 @@ dma_cookie_t dma_async_memcpy_buf_to_pg(struct dma_chan *chan,
dma_cookie_t dma_async_memcpy_pg_to_pg(struct dma_chan *chan,
struct page *dest_pg, unsigned int dest_off, struct page *src_pg,
unsigned int src_off, size_t len);
+#ifdef CONFIG_DMAENGINE_SG_TO_SG
+dma_cookie_t dma_async_memcpy_sg_to_sg(struct dma_chan *chan,
+ struct scatterlist *dst_sg, unsigned int dst_nents,
+ struct scatterlist *src_sg, unsigned int src_nents,
+ dma_async_tx_callback cb, void *cb_param);
+#endif
void dma_async_tx_descriptor_init(struct dma_async_tx_descriptor *tx,
struct dma_chan *chan);
--
1.7.1
^ permalink raw reply related
* [PATCH RFCv2 0/2] dma: add support for sg-to-sg transfers
From: Ira W. Snyder @ 2010-09-24 23:13 UTC (permalink / raw)
To: linux-kernel; +Cc: Dan Williams, linuxppc-dev
This series adds support for scatterlist to scatterlist transfers to the
generic DMAEngine API. I have hidden it behind a configuration option to
allow specific drivers that need this functionality to enable it.
This series is intended to lay the groundwork for further changes to the
series titled "CARMA Board Support". That series will be updated when I
have time and hardware to test with.
This series has not been runtime tested yet. I am posting it only to
gain comments before I spend the effort to update the driver that
depends on this.
To help reviewers, I'd like to comment on the architecture of
dma_async_memcpy_sg_to_sg(). It explicitly avoids using descriptor
chaining due to the way that feature interacts with the fsldma
controller's external start feature. To use the external start feature
properly, the in-memory descriptor chain must not be fragmented into
multiple smaller chains. This is what is achieved by submitting all
descriptors without using chaining.
An alternative implementation would create a device_prep_sg_to_sg()
function, and use that to allocate all descriptors in one shot. That
implementation would be safer against allocation failures than this one.
I would recommend against committing this until I've tested it on real
hardware.
Ira W. Snyder (2):
dmaengine: add support for scatterlist to scatterlist transfers
fsldma: use generic support for scatterlist to scatterlist transfers
arch/powerpc/include/asm/fsldma.h | 115 ++------------------
drivers/dma/Kconfig | 4 +
drivers/dma/dmaengine.c | 119 ++++++++++++++++++++
drivers/dma/fsldma.c | 219 +++++++------------------------------
include/linux/dmaengine.h | 10 ++
5 files changed, 181 insertions(+), 286 deletions(-)
^ permalink raw reply
* Re: [PATCH RFCv1 1/2] dmaengine: add support for scatterlist to scatterlist transfers
From: Ira W. Snyder @ 2010-09-24 22:53 UTC (permalink / raw)
To: Dan Williams, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20100924220419.GC24654@ovro.caltech.edu>
On Fri, Sep 24, 2010 at 03:04:19PM -0700, Ira W. Snyder wrote:
> On Fri, Sep 24, 2010 at 02:53:14PM -0700, Dan Williams wrote:
> > On Fri, 2010-09-24 at 14:24 -0700, Ira W. Snyder wrote:
> > > On Fri, Sep 24, 2010 at 01:40:56PM -0700, Dan Williams wrote:
> > > > I don't think any dma channels gracefully handle descriptors that were
> > > > prepped but not submitted. You would probably need to submit the
> > > > backlog, poll for completion, and then return the error.
> > > > Alternatively, the expectation is that descriptor allocations are
> > > > transient, i.e. once previously submitted transactions are completed
> > > > the descriptors will return to the available pool. So you could do
> > > > what async_tx routines do and just poll for a descriptor.
> > > >
> > >
> > > Can you give me an example? Even some pseudocode would help.
> >
> > Here is one from do_async_gen_syndrome() in crypto/async_tx/async_pq.c:
> >
> > /* Since we have clobbered the src_list we are committed
> > * to doing this asynchronously. Drivers force forward
> > * progress in case they can not provide a descriptor
> > */
> > for (;;) {
> > tx = dma->device_prep_dma_pq(chan, dma_dest,
> > &dma_src[src_off],
> > pq_src_cnt,
> > &coefs[src_off], len,
> > dma_flags);
> > if (likely(tx))
> > break;
> > async_tx_quiesce(&submit->depend_tx);
> > dma_async_issue_pending(chan);
> > }
> >
> > > The other DMAEngine functions (dma_async_memcpy_*()) don't do anything
> > > with the descriptor if submit fails. Take for example
> > > dma_async_memcpy_buf_to_buf(). If tx->tx_submit(tx); fails, any code
> > > using it has no way to return the descriptor to the free pool.
> > >
> > > Does tx_submit() implicitly return descriptors to the free pool if it
> > > fails?
> >
> > No, submit() failures are a hold over from when the ioatdma driver used
> > to perform additional descriptor allocation at ->submit() time. After
> > prep() the expectation is that the engine is just waiting to be told
> > "go" and can't fail. The only reason ->submit() retains a return code
> > is to support the "cookie" based method for polling for operation
> > completion. A dma driver should handle all descriptor submission
> > failure scenarios at prep time.
> >
>
> Ok, that's more like what I expected. So we still need the try forever
> code similar to the above. I can add that for the next version.
>
When coding this change, I've noticed one problem that would break my
driver. I cannot issue dma_async_issue_pending() on the channel while
creating the descriptors, since this will start transferring the
previously submitted DMA descriptors. This breaks the external hardware
control requirement.
Imagine this scenario:
1) device is not yet setup for external control (nothing is pulsing the pins)
2) dma_async_memcpy_sg_to_sg()
- this hits an allocation failure, which calls dma_async_issue_pending()
- this causes the DMA engine to start transferring to a device which is
not ready yet
- memory pressure stops, and allocation succeeds again
- some descriptors have been transferred, but not the ones since the
alloc failure
- now the first half of the descriptors (pre alloc failure) have been
transferred
- the second half of the descriptors (post alloc failure) are still
pending
- the dma_async_memcpy_sg_to_sg() returns success: all tx_submit()
succeeded
3) device_control() - setup external control mode
4) dma_async_issue_pending() - start the externally controlled transfer
5) tell the external agent to start controlling the DMA transaction
- now there isn't enough data left, and the external agent fails to
program the FPGAs
I don't mind adding it to the code, since I have enough memory that I
don't ever see allocation failures. It is an embedded system, and we've
been careful not to overcommit memory. I think for all other users, it
would be the appropriate thing to do. Most people don't care if the
scatterlist is copied in two chunks with a time gap in the middle.
An alternative implementation would be to implement
device_prep_sg_to_sg() that returned a struct dma_async_tx_descriptor,
which could then be used as normal by higher layers. This would allow
the driver to allocate / cleanup all descriptors in one shot. This would
be completely robust to this error situation.
Is there one solution you'd prefer over the other? They're both similar
in the amount of code, though duplication would probably be increased in
the device_prep_sg_to_sg() case. If any other driver implements it.
Thanks,
Ira
^ permalink raw reply
* Re: [PATCH RFCv1 1/2] dmaengine: add support for scatterlist to scatterlist transfers
From: Dan Williams @ 2010-09-24 22:20 UTC (permalink / raw)
To: Ira W. Snyder; +Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
In-Reply-To: <20100924220419.GC24654@ovro.caltech.edu>
On Fri, 2010-09-24 at 15:04 -0700, Ira W. Snyder wrote:
> On Fri, Sep 24, 2010 at 02:53:14PM -0700, Dan Williams wrote:
> > What about overrun or underrun do we not care if src_total != dst_total?
> >
> > Otherwise looks ok.
> >
>
> I don't know if we should care about that. The algorithm handles that
> case just fine. It copies the maximum amount it can, which is exactly
> min(src_total, dst_total). Whichever scatterlist runs out of entries
> first is the shortest.
>
> As a real world example, my driver verifies that both scatterlists have
> exactly the right number of bytes available before trying to program the
> hardware.
Ok, just handle the prep failure and I think we are good to go.
--
Dan
^ permalink raw reply
* [PATCH (Option 2)] of/i2c: fix module load order issue caused by of_i2c.c
From: Grant Likely @ 2010-09-24 22:15 UTC (permalink / raw)
To: khali, mikpe; +Cc: rdunlap, linuxppc-dev, linux-kernel, linux-i2c
In-Reply-To: <20100924221313.28357.61419.stgit@angua>
Commit 959e85f7, "i2c: add OF-style registration and binding" caused a
module dependency loop where of_i2c.c calls functions in i2c-core, and
i2c-core calls of_i2c_register_devices() in of_i2c. This means that
when i2c support is built as a module when CONFIG_OF is set, then
neither i2c_core nor of_i2c are able to be loaded.
This patch fixes the problem by moving the of_i2c_register_devices()
calls back into the device drivers. Device drivers already
specifically request the core code to parse the device tree for
devices anyway by setting the of_node pointer, so it isn't a big
deal to also call the registration function. The drivers just become
slightly more verbose.
Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---
drivers/i2c/busses/i2c-cpm.c | 5 +++++
drivers/i2c/busses/i2c-ibm_iic.c | 3 +++
drivers/i2c/busses/i2c-mpc.c | 1 +
drivers/i2c/i2c-core.c | 4 ----
4 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-cpm.c b/drivers/i2c/busses/i2c-cpm.c
index f7bd261..f2de3be 100644
--- a/drivers/i2c/busses/i2c-cpm.c
+++ b/drivers/i2c/busses/i2c-cpm.c
@@ -677,6 +677,11 @@ static int __devinit cpm_i2c_probe(struct platform_device *ofdev,
dev_dbg(&ofdev->dev, "hw routines for %s registered.\n",
cpm->adap.name);
+ /*
+ * register OF I2C devices
+ */
+ of_i2c_register_devices(&cpm->adap);
+
return 0;
out_shut:
cpm_i2c_shutdown(cpm);
diff --git a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ibm_iic.c
index 43ca32f..89eedf4 100644
--- a/drivers/i2c/busses/i2c-ibm_iic.c
+++ b/drivers/i2c/busses/i2c-ibm_iic.c
@@ -761,6 +761,9 @@ static int __devinit iic_probe(struct platform_device *ofdev,
dev_info(&ofdev->dev, "using %s mode\n",
dev->fast_mode ? "fast (400 kHz)" : "standard (100 kHz)");
+ /* Now register all the child nodes */
+ of_i2c_register_devices(adap);
+
return 0;
error_cleanup:
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index a1c419a..b74e6dc 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -632,6 +632,7 @@ static int __devinit fsl_i2c_probe(struct platform_device *op,
dev_err(i2c->dev, "failed to add adapter\n");
goto fail_add;
}
+ of_i2c_register_devices(&i2c->adap);
return result;
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 6649176..a9589f5 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -32,7 +32,6 @@
#include <linux/init.h>
#include <linux/idr.h>
#include <linux/mutex.h>
-#include <linux/of_i2c.h>
#include <linux/of_device.h>
#include <linux/completion.h>
#include <linux/hardirq.h>
@@ -876,9 +875,6 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
if (adap->nr < __i2c_first_dynamic_bus_num)
i2c_scan_static_board_info(adap);
- /* Register devices from the device tree */
- of_i2c_register_devices(adap);
-
/* Notify drivers */
mutex_lock(&core_lock);
bus_for_each_drv(&i2c_bus_type, NULL, adap, __process_new_adapter);
^ permalink raw reply related
* Re: [BUG 2.6.36-rc5] of_i2c.ko <-> i2c-core.ko dependency loop
From: Grant Likely @ 2010-09-24 22:14 UTC (permalink / raw)
To: Jean Delvare, Mikael Pettersson
Cc: Randy Dunlap, linuxppc-dev, linux-kernel, linux-i2c
In-Reply-To: <f1f37e21-f04a-4fef-868c-27c7d2ebe594@email.android.com>
On Fri, Sep 24, 2010 at 7:48 AM, Grant Likely <grant.likely@secretlab.ca> w=
rote:
>
>
> "Jean Delvare" <khali@linux-fr.org> wrote:
>
>>Hi Mikael,
>>
>>On Fri, 24 Sep 2010 12:50:01 +0200, Mikael Pettersson wrote:
>>> Jean Delvare writes:
>>> =A0> As far as I can see this is caused by this commit from Grant:
>>> =A0>
>>> =A0> http://git.kernel.org/?p=3Dlinux/kernel/git/torvalds/linux-2.6.git=
;a=3Dcommitdiff;h=3D959e85f7751c33d1a2dabc5cc3fe2ed0db7052e5
>>> =A0>
>>> =A0> Mikael, can you please try reverting this patch and see if it solv=
es
>>> =A0> your problem?
>>>
>>> Yes, reverting the above commit from 2.6.36-rc5 eliminated the warnings=
,
>>> and I was able to insmod the i2c-{core,dev,powermac}.ko modules.
>>
>>Thanks for testing and reporting. Grant, unless you come up with a fix
>>very quickly, I'll have to revert
>>959e85f7751c33d1a2dabc5cc3fe2ed0db7052e5 for 2.6.36.
>
> I'll get a fix out today.
I've got two different fixes that I'm about to send you. You can
choose the fix that you prefer. The first option moves the offending
function into i2c-core.c. The function parses the device tree data
and creates i2c_device for each i2c device node that it finds. This
is analogous to i2c_scan_static_board_info().
The second options reverts most of the 959e85f7 commit, but keeps the
line that allows of-style matching is retained so that all i2c_devices
on powerpc machines will still bind correctly.
My preferred solution is the first option because the tested code path
does not changed. The offending function is simply moved verbatim.
The second option is a smaller patch, but I can only test one of the
affected drivers. However, I'll let you make the decision.
Both have been build tested on PowerPC and ARM, and run tested on a
PowerPC MPC5200 board.
patches to follow in a few minutes..
g.
^ permalink raw reply
* [PATCH (Option 1)] of/i2c: fix module load order issue caused by of_i2c.c
From: Grant Likely @ 2010-09-24 22:14 UTC (permalink / raw)
To: khali, mikpe; +Cc: rdunlap, linuxppc-dev, linux-kernel, linux-i2c
Commit 959e85f7, "i2c: add OF-style registration and binding" caused a
module dependency loop where of_i2c.c calls functions in i2c-core, and
i2c-core calls of_i2c_register_devices() in of_i2c. This means that
when i2c support is built as a module when CONFIG_OF is set, then
neither i2c_core nor of_i2c are able to be loaded.
This patch fixes the problem by moving the of_i2c_register_devices()
function into the body of i2c_core and renaming it to
i2c_scan_of_devices (of_i2c_register_devices is analogous to the
existing i2c_scan_static_board_info function and so should be named
similarly). This function isn't called by any code outside of
i2c_core, and it must always be present when CONFIG_OF is selected, so
it makes sense to locate it there. When CONFIG_OF is not selected,
of_i2c_register_devices() becomes a no-op.
Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---
drivers/i2c/i2c-core.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++--
drivers/of/of_i2c.c | 57 ---------------------------------------------
include/linux/of_i2c.h | 7 ------
3 files changed, 59 insertions(+), 66 deletions(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 6649176..64a261b 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -32,8 +32,8 @@
#include <linux/init.h>
#include <linux/idr.h>
#include <linux/mutex.h>
-#include <linux/of_i2c.h>
#include <linux/of_device.h>
+#include <linux/of_irq.h>
#include <linux/completion.h>
#include <linux/hardirq.h>
#include <linux/irqflags.h>
@@ -818,6 +818,63 @@ static void i2c_scan_static_board_info(struct i2c_adapter *adapter)
up_read(&__i2c_board_lock);
}
+#ifdef CONFIG_OF
+void i2c_scan_of_devices(struct i2c_adapter *adap)
+{
+ void *result;
+ struct device_node *node;
+
+ /* Only register child devices if the adapter has a node pointer set */
+ if (!adap->dev.of_node)
+ return;
+
+ for_each_child_of_node(adap->dev.of_node, node) {
+ struct i2c_board_info info = {};
+ struct dev_archdata dev_ad = {};
+ const __be32 *addr;
+ int len;
+
+ dev_dbg(&adap->dev, "of_i2c: register %s\n", node->full_name);
+ if (of_modalias_node(node, info.type, sizeof(info.type)) < 0) {
+ dev_err(&adap->dev, "of_i2c: modalias failure on %s\n",
+ node->full_name);
+ continue;
+ }
+
+ addr = of_get_property(node, "reg", &len);
+ if (!addr || (len < sizeof(int))) {
+ dev_err(&adap->dev, "of_i2c: invalid reg on %s\n",
+ node->full_name);
+ continue;
+ }
+
+ info.addr = be32_to_cpup(addr);
+ if (info.addr > (1 << 10) - 1) {
+ dev_err(&adap->dev, "of_i2c: invalid addr=%x on %s\n",
+ info.addr, node->full_name);
+ continue;
+ }
+
+ info.irq = irq_of_parse_and_map(node, 0);
+ info.of_node = of_node_get(node);
+ info.archdata = &dev_ad;
+
+ request_module("%s", info.type);
+
+ result = i2c_new_device(adap, &info);
+ if (result == NULL) {
+ dev_err(&adap->dev, "of_i2c: Failure registering %s\n",
+ node->full_name);
+ of_node_put(node);
+ irq_dispose_mapping(info.irq);
+ continue;
+ }
+ }
+}
+#else
+static inline void i2c_scan_of_devices(struct i2c_adapter *adap) { }
+#endif
+
static int i2c_do_add_adapter(struct i2c_driver *driver,
struct i2c_adapter *adap)
{
@@ -877,7 +934,7 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
i2c_scan_static_board_info(adap);
/* Register devices from the device tree */
- of_i2c_register_devices(adap);
+ i2c_scan_of_devices(adap);
/* Notify drivers */
mutex_lock(&core_lock);
diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c
index 0a694de..e0c3841 100644
--- a/drivers/of/of_i2c.c
+++ b/drivers/of/of_i2c.c
@@ -17,63 +17,6 @@
#include <linux/of_irq.h>
#include <linux/module.h>
-void of_i2c_register_devices(struct i2c_adapter *adap)
-{
- void *result;
- struct device_node *node;
-
- /* Only register child devices if the adapter has a node pointer set */
- if (!adap->dev.of_node)
- return;
-
- dev_dbg(&adap->dev, "of_i2c: walking child nodes\n");
-
- for_each_child_of_node(adap->dev.of_node, node) {
- struct i2c_board_info info = {};
- struct dev_archdata dev_ad = {};
- const __be32 *addr;
- int len;
-
- dev_dbg(&adap->dev, "of_i2c: register %s\n", node->full_name);
-
- if (of_modalias_node(node, info.type, sizeof(info.type)) < 0) {
- dev_err(&adap->dev, "of_i2c: modalias failure on %s\n",
- node->full_name);
- continue;
- }
-
- addr = of_get_property(node, "reg", &len);
- if (!addr || (len < sizeof(int))) {
- dev_err(&adap->dev, "of_i2c: invalid reg on %s\n",
- node->full_name);
- continue;
- }
-
- info.addr = be32_to_cpup(addr);
- if (info.addr > (1 << 10) - 1) {
- dev_err(&adap->dev, "of_i2c: invalid addr=%x on %s\n",
- info.addr, node->full_name);
- continue;
- }
-
- info.irq = irq_of_parse_and_map(node, 0);
- info.of_node = of_node_get(node);
- info.archdata = &dev_ad;
-
- request_module("%s", info.type);
-
- result = i2c_new_device(adap, &info);
- if (result == NULL) {
- dev_err(&adap->dev, "of_i2c: Failure registering %s\n",
- node->full_name);
- of_node_put(node);
- irq_dispose_mapping(info.irq);
- continue;
- }
- }
-}
-EXPORT_SYMBOL(of_i2c_register_devices);
-
static int of_dev_node_match(struct device *dev, void *data)
{
return dev->of_node == data;
diff --git a/include/linux/of_i2c.h b/include/linux/of_i2c.h
index 0efe8d4..1998cf8 100644
--- a/include/linux/of_i2c.h
+++ b/include/linux/of_i2c.h
@@ -15,16 +15,9 @@
#if defined(CONFIG_OF_I2C) || defined(CONFIG_OF_I2C_MODULE)
#include <linux/i2c.h>
-extern void of_i2c_register_devices(struct i2c_adapter *adap);
-
/* must call put_device() when done with returned i2c_client device */
extern struct i2c_client *of_find_i2c_device_by_node(struct device_node *node);
-#else
-static inline void of_i2c_register_devices(struct i2c_adapter *adap)
-{
- return;
-}
#endif /* CONFIG_OF_I2C */
#endif /* __LINUX_OF_I2C_H */
^ permalink raw reply related
* Re: [PATCH 1/8] posix clocks: introduce a syscall for clock tuning.
From: Benjamin Herrenschmidt @ 2010-09-24 22:12 UTC (permalink / raw)
To: Richard Cochran
Cc: Peter Zijlstra, John Stultz, devicetree-discuss, linuxppc-dev,
linux-kernel, David Miller, netdev, linux-api, Thomas Gleixner,
Rodolfo Giometti, Christoph Lameter, linux-arm-kernel,
Krzysztof Halasa
In-Reply-To: <20100924075534.GB5043@riccoc20.at.omicron.at>
> > This list is getting way too much unrelated stuff, which I find
> > annoying, it would be nice if we were all a bit more careful here with
> > our CC lists.
>
> Sorry, I only added device-tree because some one asked me to do so.
>
> http://marc.info/?l=linux-netdev&m=127273157912358
>
> I'll leave it off next time.
That's allright. I'd rather you just post the bindings there than the
whole patch least but no big deal.
I was just fixing my email filters and notice a lot of seemingly
unrelated stuff landing there :-)
Cheers,
Ben.
^ permalink raw reply
* Re: ppc44x - how do i optimize driver for tlb hits
From: Benjamin Herrenschmidt @ 2010-09-24 22:11 UTC (permalink / raw)
To: Ayman El-Khashab; +Cc: linuxppc-dev
In-Reply-To: <20100924130851.GA14016@crust.elkhashab.com>
On Fri, 2010-09-24 at 08:08 -0500, Ayman El-Khashab wrote:
>
> I suppose another option is to to use the kernel profiling option I
> always see but have never used. Is that a viable option to figure out
> what is happening here?
With perf and stochastic sampling ? If you sample fast enough... but
you'll mostly point to your routine I suppose... though it might tell
you statistically where in your code, which -might- help.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH RFCv1 1/2] dmaengine: add support for scatterlist to scatterlist transfers
From: Ira W. Snyder @ 2010-09-24 22:04 UTC (permalink / raw)
To: Dan Williams; +Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
In-Reply-To: <1285365194.21375.22.camel@dwillia2-linux>
On Fri, Sep 24, 2010 at 02:53:14PM -0700, Dan Williams wrote:
> On Fri, 2010-09-24 at 14:24 -0700, Ira W. Snyder wrote:
> > On Fri, Sep 24, 2010 at 01:40:56PM -0700, Dan Williams wrote:
> > > I don't think any dma channels gracefully handle descriptors that were
> > > prepped but not submitted. You would probably need to submit the
> > > backlog, poll for completion, and then return the error.
> > > Alternatively, the expectation is that descriptor allocations are
> > > transient, i.e. once previously submitted transactions are completed
> > > the descriptors will return to the available pool. So you could do
> > > what async_tx routines do and just poll for a descriptor.
> > >
> >
> > Can you give me an example? Even some pseudocode would help.
>
> Here is one from do_async_gen_syndrome() in crypto/async_tx/async_pq.c:
>
> /* Since we have clobbered the src_list we are committed
> * to doing this asynchronously. Drivers force forward
> * progress in case they can not provide a descriptor
> */
> for (;;) {
> tx = dma->device_prep_dma_pq(chan, dma_dest,
> &dma_src[src_off],
> pq_src_cnt,
> &coefs[src_off], len,
> dma_flags);
> if (likely(tx))
> break;
> async_tx_quiesce(&submit->depend_tx);
> dma_async_issue_pending(chan);
> }
>
> > The other DMAEngine functions (dma_async_memcpy_*()) don't do anything
> > with the descriptor if submit fails. Take for example
> > dma_async_memcpy_buf_to_buf(). If tx->tx_submit(tx); fails, any code
> > using it has no way to return the descriptor to the free pool.
> >
> > Does tx_submit() implicitly return descriptors to the free pool if it
> > fails?
>
> No, submit() failures are a hold over from when the ioatdma driver used
> to perform additional descriptor allocation at ->submit() time. After
> prep() the expectation is that the engine is just waiting to be told
> "go" and can't fail. The only reason ->submit() retains a return code
> is to support the "cookie" based method for polling for operation
> completion. A dma driver should handle all descriptor submission
> failure scenarios at prep time.
>
Ok, that's more like what I expected. So we still need the try forever
code similar to the above. I can add that for the next version.
> > Ok, I thought the list was clearer, but this is equally easy. How about
> > the following change that does away with the list completely. Then
> > things should work on ioatdma as well.
> >
> > From d59569ff48a89ef5411af3cf2995af7b742c5cd3 Mon Sep 17 00:00:00 2001
> > From: Ira W. Snyder <iws@ovro.caltech.edu>
> > Date: Fri, 24 Sep 2010 14:18:09 -0700
> > Subject: [PATCH] dma: improve scatterlist to scatterlist transfer
> >
> > This is an improved algorithm to improve support on the Intel I/OAT
> > driver.
> >
> > Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
> > ---
> > drivers/dma/dmaengine.c | 52 +++++++++++++++++++++-----------------------
> > include/linux/dmaengine.h | 3 --
> > 2 files changed, 25 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> > index 57ec1e5..cde775c 100644
> > --- a/drivers/dma/dmaengine.c
> > +++ b/drivers/dma/dmaengine.c
> > @@ -983,10 +983,13 @@ dma_async_memcpy_sg_to_sg(struct dma_chan *chan,
> > struct dma_async_tx_descriptor *tx;
> > dma_cookie_t cookie = -ENOMEM;
> > size_t dst_avail, src_avail;
> > - struct list_head tx_list;
> > + struct scatterlist *sg;
> > size_t transferred = 0;
> > + size_t dst_total = 0;
> > + size_t src_total = 0;
> > dma_addr_t dst, src;
> > size_t len;
> > + int i;
> >
> > if (dst_nents == 0 || src_nents == 0)
> > return -EINVAL;
> > @@ -994,12 +997,17 @@ dma_async_memcpy_sg_to_sg(struct dma_chan *chan,
> > if (dst_sg == NULL || src_sg == NULL)
> > return -EINVAL;
> >
> > + /* get the total count of bytes in each scatterlist */
> > + for_each_sg(dst_sg, sg, dst_nents, i)
> > + dst_total += sg_dma_len(sg);
> > +
> > + for_each_sg(src_sg, sg, src_nents, i)
> > + src_total += sg_dma_len(sg);
> > +
>
> What about overrun or underrun do we not care if src_total != dst_total?
>
> Otherwise looks ok.
>
I don't know if we should care about that. The algorithm handles that
case just fine. It copies the maximum amount it can, which is exactly
min(src_total, dst_total). Whichever scatterlist runs out of entries
first is the shortest.
As a real world example, my driver verifies that both scatterlists have
exactly the right number of bytes available before trying to program the
hardware.
Ira
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox