* Re: [PATCH 1/2] lib: Provide generic atomic64_t implementation
From: Mike Frysinger @ 2009-06-18 23:55 UTC (permalink / raw)
To: Paul Mackerras; +Cc: akpm, torvalds, linux-kernel, linuxppc-dev
In-Reply-To: <18995.20685.227683.561827@cargo.ozlabs.ibm.com>
On Sat, Jun 13, 2009 at 03:10, Paul Mackerras wrote:
> +typedef struct {
> + =C2=A0 =C2=A0 =C2=A0 long long counter;
> +} atomic64_t;
lack of volatile seems odd compared to:
include/linux/types.h:
typedef struct {
volatile int counter;
} atomic_t;
-mike
^ permalink raw reply
* [PATCH] powerpc/85xx: Make eSDHC 1-bit only transfer mode default for MPC8569E-MDS
From: Anton Vorontsov @ 2009-06-18 23:37 UTC (permalink / raw)
To: Kumar Gala; +Cc: linuxppc-dev
For yet unknown reason 4-bit mode doesn't work on MPC8569E-MDS boards,
so make 1-bit mode default. When we resolve the issue, u-boot will
remove sdhci,1-bit-only property from the device tree, while SDHCI
will still work with older u-boots.
Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
arch/powerpc/boot/dts/mpc8569mds.dts | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/arch/powerpc/boot/dts/mpc8569mds.dts b/arch/powerpc/boot/dts/mpc8569mds.dts
index 39c2927..63e8109 100644
--- a/arch/powerpc/boot/dts/mpc8569mds.dts
+++ b/arch/powerpc/boot/dts/mpc8569mds.dts
@@ -229,6 +229,7 @@
/* Filled in by U-Boot */
clock-frequency = <0>;
status = "disabled";
+ sdhci,1-bit-only;
};
crypto@30000 {
--
1.6.3.1
^ permalink raw reply related
* [PATCH] powerpc/85xx: Fix FSL RapidIO probing on MDS boards
From: Anton Vorontsov @ 2009-06-18 23:22 UTC (permalink / raw)
To: Kumar Gala; +Cc: linuxppc-dev
From: Randy Vinson <rvinson@mvista.com>
FSL RapidIO won't probe without a proper compatible entry. This
patch fixes the issue by adding fsl,rapidio-delta compatible to
mpc85xx_ids.
Signed-off-by: Randy Vinson <rvinson@mvista.com>
Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
arch/powerpc/platforms/85xx/mpc85xx_mds.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/arch/powerpc/platforms/85xx/mpc85xx_mds.c b/arch/powerpc/platforms/85xx/mpc85xx_mds.c
index b2c0a43..daf6711 100644
--- a/arch/powerpc/platforms/85xx/mpc85xx_mds.c
+++ b/arch/powerpc/platforms/85xx/mpc85xx_mds.c
@@ -268,6 +268,7 @@ static struct of_device_id mpc85xx_ids[] = {
{ .type = "qe", },
{ .compatible = "fsl,qe", },
{ .compatible = "gianfar", },
+ { .compatible = "fsl,rapidio-delta", },
{},
};
--
1.6.3.1
^ permalink raw reply related
* Re: MPC83xx watchdog reset board dead lock
From: David Hawkins @ 2009-06-18 23:22 UTC (permalink / raw)
To: Leon Woestenberg; +Cc: Linux PPC, Norbert van Bolhuis
In-Reply-To: <c384c5ea0906181601g28e3ac7frf2019b11bc34e057@mail.gmail.com>
Hi Leon,
>>> I'll be testing the design tomorrow on the reference board, I'll
>>> report results in this thread.
>> Interesting.
>> Looking forward to the results.
>>
> Design works as expected on the now slightly modified MPC8313E-RDB
> board.
That's great!
> Kudos to David.
I'm sure you would have come up with a similar solution,
had you had a chance to sleep on it :)
Glad to help out.
Cheers,
Dave
^ permalink raw reply
* Re: [PATCH 1/6] perf_counter: powerpc: Enable use of software counters on 32-bit powerpc
From: Benjamin Herrenschmidt @ 2009-06-18 23:03 UTC (permalink / raw)
To: Ingo Molnar; +Cc: linuxppc-dev, Paul Mackerras, linux-kernel, Peter Zijlstra
In-Reply-To: <20090617142719.GC6846@elte.hu>
On Wed, 2009-06-17 at 16:27 +0200, Ingo Molnar wrote:
> I think it would be nice to have more platform support in .31.
> Perfcounters is a brand-new feature so there's no risk of
> regression. In the end it will depend on Linus to pull of course,
> and BenH can veto it too if he'd like no more PowerPC changes in
> this cycle. Worst-case it's all .32 material.
There have been little PowerPC changes in this cycle and I agree
with you on that it's a nice feature to have with little risk of
regression.
In fact, I also have an up-to-date (and hopefully working)
irqtrace/lockdep patch for 32-bit powerpc (we only do 64-bit right now)
that I'm considering merging this time around, the benefit it brings is
worth the risk I believe.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH 1/6] perf_counter: powerpc: Enable use of software counters on 32-bit powerpc
From: Benjamin Herrenschmidt @ 2009-06-18 23:01 UTC (permalink / raw)
To: Ingo Molnar; +Cc: linuxppc-dev, Paul Mackerras, Peter Zijlstra, linux-kernel
In-Reply-To: <20090617142151.GB6846@elte.hu>
On Wed, 2009-06-17 at 16:21 +0200, Ingo Molnar wrote:
> > This depends on the generic atomic64_t patches, which are now in
> > Linus' tree. Ingo, if you're putting these in, please pull Linus'
> > tree in first.
>
> Note, i've created a new branch, tip:perfcounters/powerpc, so we can
> keep these things separate and Ben can pull them too. I see there
> was some review feedback - do you want to send a v2 version perhaps?
At this stage, I'm happy for you to push them. I was thinking about
moving the files around but that can wait. No serious comments on the
patches themselves on my side.
Cheers
Ben
^ permalink raw reply
* Re: MPC83xx watchdog reset board dead lock
From: Leon Woestenberg @ 2009-06-18 23:01 UTC (permalink / raw)
To: Norbert van Bolhuis, David Hawkins; +Cc: Linux PPC
In-Reply-To: <4A38DE83.1010405@aimvalley.nl>
Hello,
On Wed, Jun 17, 2009 at 2:16 PM, Norbert van
Bolhuis<nvbolhuis@aimvalley.nl> wrote:
>>
>> I'll be testing the design tomorrow on the reference board, I'll
>> report results in this thread.
>
> Interesting.
> Looking forward to the results.
>
Design works as expected on the now slightly modified MPC8313E-RDB
board. Kudos to David.
Cheers,
--
Leon
^ permalink raw reply
* [PATCH v2] fsldma: Add DMA_SLAVE support
From: Ira Snyder @ 2009-06-18 22:53 UTC (permalink / raw)
To: linuxppc-dev, Kumar Gala, Dan Williams, Li Yang
Use the DMA_SLAVE capability of the DMAEngine API to copy/from a
scatterlist into an arbitrary list of hardware address/length pairs.
This allows a single DMA transaction to copy data from several different
devices into a scatterlist at the same time.
This also adds support to enable some controller-specific features such as
external start and external pause for a DMA transaction.
Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
---
After discussion with Dan Williams, this is the second version of the
DMA_SLAVE API for the Freescale DMA controller. I've tested it heavily
with both drivers I have written against this API, an FPGA programmer
and an FPGA data grabber.
Kumar, Dan asked me to add you to the CC list, so you can have a look at
this patch before he adds it to his tree.
The other two small patches I posted earlier are very helpful in testing
this functionality. They make the fsldma driver leave the BWC (bandwidth
control) bits alone on the 83xx controller, as well as making the
external start feature available on 83xx.
In order for the external start/pause features to be useful, the
bandwidth control bits (in the mode register) need to be set before
attempting to use the controller. I could spin a v3 of this patch that
adds a field to struct fsl_dma_slave to set the bits appropriately. Or I
could send another patch for that. Thoughts?
Many thanks to all that have participated in the discussion about this
patch.
v1 -> v2:
* move fsldma.h from include/linux to arch/powerpc/include/asm
* add kerneldoc documentation
arch/powerpc/include/asm/fsldma.h | 134 ++++++++++++++++++++++
drivers/dma/fsldma.c | 224 +++++++++++++++++++++++++++++++++++++
2 files changed, 358 insertions(+), 0 deletions(-)
create mode 100644 arch/powerpc/include/asm/fsldma.h
diff --git a/arch/powerpc/include/asm/fsldma.h b/arch/powerpc/include/asm/fsldma.h
new file mode 100644
index 0000000..1bc71fb
--- /dev/null
+++ b/arch/powerpc/include/asm/fsldma.h
@@ -0,0 +1,134 @@
+/*
+ * Freescale MPC83XX / MPC85XX DMA Controller
+ *
+ * Copyright (c) 2009 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
+ * kind, whether express or implied.
+ */
+
+#ifndef __ARCH_POWERPC_ASM_FSLDMA_H__
+#define __ARCH_POWERPC_ASM_FSLDMA_H__
+
+#include <linux/dmaengine.h>
+
+/*
+ * Definitions for the Freescale DMA controller's DMA_SLAVE implemention
+ *
+ * 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.
+ *
+ * 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.
+ */
+
+/**
+ * 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
+ * @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 {
+
+ /* List of hardware address/length pairs */
+ struct list_head addresses;
+
+ /* Support for extra controller features */
+ 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 f18d1bd..67c3c47 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -29,6 +29,7 @@
#include <linux/dmapool.h>
#include <linux/of_platform.h>
+#include <asm/fsldma.h>
#include "fsldma.h"
static void dma_init(struct fsl_dma_chan *fsl_chan)
@@ -531,6 +532,226 @@ fail:
}
/**
+ * fsl_dma_prep_slave_sg - prepare descriptors for a DMA_SLAVE transaction
+ * @chan: DMA channel
+ * @sgl: scatterlist to transfer to/from
+ * @sg_len: number of entries in @scatterlist
+ * @direction: DMA direction
+ * @flags: DMAEngine flags
+ *
+ * Prepare a set of descriptors for a DMA_SLAVE transaction. Following the
+ * DMA_SLAVE API, this gets the device-specific information from the
+ * chan->private variable.
+ */
+static struct dma_async_tx_descriptor *fsl_dma_prep_slave_sg(
+ struct dma_chan *chan, struct scatterlist *sgl, unsigned int sg_len,
+ enum dma_data_direction direction, unsigned long flags)
+{
+ struct fsl_dma_chan *fsl_chan;
+ struct fsl_desc_sw *first = NULL, *prev = NULL, *new = NULL;
+ struct fsl_dma_slave *slave;
+ struct list_head *tx_list;
+ 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 (!chan)
+ return NULL;
+
+ if (!chan->private)
+ return NULL;
+
+ fsl_chan = to_fsl_chan(chan);
+ slave = chan->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
+ *
+ * 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
+ */
+ 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(fsl_chan);
+ if (!new) {
+ dev_err(fsl_chan->dev, "No free memory for "
+ "link descriptor\n");
+ goto fail;
+ }
+#ifdef FSL_DMA_LD_DEBUG
+ dev_dbg(fsl_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(fsl_chan, &new->hw, copy);
+ set_desc_src(fsl_chan, &new->hw, dma_src);
+ set_desc_dest(fsl_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(fsl_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->async_tx.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(fsl_chan, new);
+
+ /* Enable extra controller features */
+ if (fsl_chan->set_src_loop_size)
+ fsl_chan->set_src_loop_size(fsl_chan, slave->src_loop_size);
+
+ if (fsl_chan->set_dest_loop_size)
+ fsl_chan->set_dest_loop_size(fsl_chan, slave->dst_loop_size);
+
+ if (fsl_chan->toggle_ext_start)
+ fsl_chan->toggle_ext_start(fsl_chan, slave->external_start);
+
+ if (fsl_chan->toggle_ext_pause)
+ fsl_chan->toggle_ext_pause(fsl_chan, slave->external_pause);
+
+ 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->async_tx.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
+ */
+ tx_list = &first->async_tx.tx_list;
+ list_for_each_entry_safe_reverse(new, prev, tx_list, node) {
+ list_del_init(&new->node);
+ dma_pool_free(fsl_chan->desc_pool, new, new->async_tx.phys);
+ }
+
+ return NULL;
+}
+
+static void fsl_dma_device_terminate_all(struct dma_chan *chan)
+{
+ struct fsl_dma_chan *fsl_chan;
+ struct fsl_desc_sw *desc, *tmp;
+ unsigned long flags;
+
+ if (!chan)
+ return;
+
+ fsl_chan = to_fsl_chan(chan);
+
+ /* Halt the DMA engine */
+ dma_halt(fsl_chan);
+
+ spin_lock_irqsave(&fsl_chan->desc_lock, flags);
+
+ /* Remove and free all of the descriptors in the LD queue */
+ list_for_each_entry_safe(desc, tmp, &fsl_chan->ld_queue, node) {
+ list_del(&desc->node);
+ dma_pool_free(fsl_chan->desc_pool, desc, desc->async_tx.phys);
+ }
+
+ spin_unlock_irqrestore(&fsl_chan->desc_lock, flags);
+}
+
+/**
* fsl_dma_update_completed_cookie - Update the completed cookie.
* @fsl_chan : Freescale DMA channel
*/
@@ -955,12 +1176,15 @@ static int __devinit of_fsl_dma_probe(struct of_device *dev,
dma_cap_set(DMA_MEMCPY, fdev->common.cap_mask);
dma_cap_set(DMA_INTERRUPT, fdev->common.cap_mask);
+ dma_cap_set(DMA_SLAVE, fdev->common.cap_mask);
fdev->common.device_alloc_chan_resources = fsl_dma_alloc_chan_resources;
fdev->common.device_free_chan_resources = fsl_dma_free_chan_resources;
fdev->common.device_prep_dma_interrupt = fsl_dma_prep_interrupt;
fdev->common.device_prep_dma_memcpy = fsl_dma_prep_memcpy;
fdev->common.device_is_tx_complete = fsl_dma_is_complete;
fdev->common.device_issue_pending = fsl_dma_memcpy_issue_pending;
+ fdev->common.device_prep_slave_sg = fsl_dma_prep_slave_sg;
+ fdev->common.device_terminate_all = fsl_dma_device_terminate_all;
fdev->common.dev = &dev->dev;
fdev->irq = irq_of_parse_and_map(dev->node, 0);
--
1.5.4.3
^ permalink raw reply related
* Re: [PATCH 5/6] perf_counter: powerpc: Add processor back-end for MPC7450 family
From: Paul Mackerras @ 2009-06-18 21:10 UTC (permalink / raw)
To: Kumar Gala; +Cc: linuxppc-dev, Ingo Molnar, Peter Zijlstra, linux-kernel
In-Reply-To: <E57BFC08-F847-463A-9D0F-55979C4B0524@kernel.crashing.org>
Kumar Gala writes:
> In looking at doing this what suggestions do you have in implementing
> perf_instruction_pointer() if we don't have the equivalent of SIAR.
> Just use regs->nip ?
Yes, exactly. If you don't have SIAR/SDAR/MMCRA, the default
definitions for perf_instruction_pointer and perf_misc_flags in
include/linux/perf_counter.h are fine, and you get them if
perf_misc_flags is not defined.
Paul.
^ permalink raw reply
* Re: [PATCH] RFC: powerpc: expose the multi-bit ops that underlie single-bit ops.
From: Benjamin Herrenschmidt @ 2009-06-18 22:22 UTC (permalink / raw)
To: Geoff Thorpe; +Cc: linuxppc-dev
In-Reply-To: <4A3AA3FE.8090903@freescale.com>
On Thu, 2009-06-18 at 16:30 -0400, Geoff Thorpe wrote:
> I've left the volatile qualifier in the generated API because I didn't
> feel so comfortable changing APIs, but I also added the "memory" clobber
> for all cases - whereas it seems the existing set_bits(), clear_bits(),
> [...] functions didn't declare this... Do you see any issue with having
> the 'volatile' in the prototype as well as the clobber in the asm?
>
> Actually, might as well just respond to the new patch instead... :-) Thx.
I think the story with the memory clobber is that it depends whether
we consider the functions as ordering accesses or not (ie, can
potentially be used with lock/unlock semantics).
The general rule is that those who don't return anything don't need
to have those semantics, and thus could only be advertised as clobbering
p[word] -but- there are issues there. For example, despite the
(relatively new) official _lock/_unlock variants, there's still code
that abuses constructs like test_and_set_bit/clear_bit as locks and in
that case, clear bits needs a clobber.
So I would say at this stage better safe than having to track down
incredibly hard to find bugs, and let's make them all take that clobber.
Cheers,
Ben.
^ permalink raw reply
* Re: killing use of ppc_md.init
From: Benjamin Herrenschmidt @ 2009-06-18 22:18 UTC (permalink / raw)
To: Kumar Gala; +Cc: linuxppc-dev list
In-Reply-To: <C81AEED1-5CEC-42B8-A893-5E6BDDD4EC54@kernel.crashing.org>
On Thu, 2009-06-18 at 09:38 -0500, Kumar Gala wrote:
> ppc_md.init only exists on ppc32 and seems like its pretty useless
> today. The users seem to fall into two classes:
>
> 1. called to do of_platform_bus_probe() - most platforms use
> machine_device_initcall() for this
> 2. some platform init code which seems like it could move into
> setup_arch().
>
> The second one seems to only be on amigaone and chrp. Anyone know if
> there is any harm in moving the amigaone_init() into
> amigaone_setup_arch() and similarly on chrp chrp_init2() into
> chrp_setup_arch().
We might kill it ... and revive it differently :-)
Yes, the current ppc_init() can probably just go.
However, we probably also want to add a call from init/main.c back to
the architectures and ppc_md. in our case that is right after mm_init().
Right now, we do way too many things at setup_arch() (or even before
that on ppc64) which induces all sorts of pain due to having to use
bootmem etc...
Now that slab is available much earlier, before init_IRQ() and
time_init(), we should consider moving a whole bunch of stuff somewhere
later in the boot process to simplify the code etc...
Cheers,
Ben.
^ permalink raw reply
* Re: [RFC PATCH] fsldma: Add DMA_SLAVE support
From: Dan Williams @ 2009-06-18 21:36 UTC (permalink / raw)
To: Ira Snyder; +Cc: linuxppc-dev@ozlabs.org, Li Yang
In-Reply-To: <20090618205024.GA16741@ovro.caltech.edu>
Ira Snyder wrote:
> So, I see a couple of ways of moving forward:
> 1) Keep my implementation, moving the includes to arch/powerpc/include.
> Do we drop the allocation functions?
+1 for option 1. Having it under arch/powerpc/include makes it clear
that it is a powerpc specific api, so keep the allocation routines.
Copy Kumar on the updated patch as I'll need a ppc-dev ack for carrying
this file addition through the dmaengine tree.
> Thanks for all the input Dan. I finally feel like we're getting
> somewhere :)
Thanks for the exchange it always helps to get a good picture of the
underlying design rationale.
Regards,
Dan
^ permalink raw reply
* [PATCH] powerpc: expose the multi-bit ops that underlie single-bit ops.
From: Geoff Thorpe @ 2009-06-18 20:36 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Geoff Thorpe
The bitops.h functions that operate on a single bit in a bitfield are
implemented by operating on the corresponding word location. In all cases
the inner logic is valid if the mask being applied has more than one bit
set, so this patch exposes those inner operations. Indeed, set_bits() was
already available, but it duplicated code from set_bit() (rather than
making the latter a wrapper) - it was also missing the PPC405_ERR77()
workaround and the "volatile" address qualifier present in other APIs.
This corrects that, and exposes the other multi-bit equivalents.
One advantage of these multi-bit forms is that they allow word-sized
variables to essentially be their own spinlocks, eg. very useful for
state machines where an atomic "flags" variable can obviate the need for
any additional locking.
Signed-off-by: Geoff Thorpe <geoff@geoffthorpe.net>
---
arch/powerpc/include/asm/bitops.h | 194 ++++++++++++-------------------------
1 files changed, 61 insertions(+), 133 deletions(-)
diff --git a/arch/powerpc/include/asm/bitops.h b/arch/powerpc/include/asm/bitops.h
index 897eade..56f2f2e 100644
--- a/arch/powerpc/include/asm/bitops.h
+++ b/arch/powerpc/include/asm/bitops.h
@@ -56,174 +56,102 @@
#define BITOP_WORD(nr) ((nr) / BITS_PER_LONG)
#define BITOP_LE_SWIZZLE ((BITS_PER_LONG-1) & ~0x7)
+/* Macro for generating the ***_bits() functions */
+#define DEFINE_BITOP(fn, op, prefix, postfix) \
+static __inline__ void fn(unsigned long mask, \
+ volatile unsigned long *_p) \
+{ \
+ unsigned long old; \
+ unsigned long *p = (unsigned long *)_p; \
+ __asm__ __volatile__ ( \
+ prefix \
+"1:" PPC_LLARX "%0,0,%3\n" \
+ stringify_in_c(op) "%0,%0,%2\n" \
+ PPC405_ERR77(0,%3) \
+ PPC_STLCX "%0,0,%3\n" \
+ "bne- 1b\n" \
+ postfix \
+ : "=&r" (old), "+m" (*p) \
+ : "r" (mask), "r" (p) \
+ : "cc", "memory"); \
+}
+
+DEFINE_BITOP(set_bits, or, "", "")
+DEFINE_BITOP(clear_bits, andc, "", "")
+DEFINE_BITOP(clear_bits_unlock, andc, LWSYNC_ON_SMP, "")
+DEFINE_BITOP(change_bits, xor, "", "")
+
static __inline__ void set_bit(int nr, volatile unsigned long *addr)
{
- unsigned long old;
- unsigned long mask = BITOP_MASK(nr);
- unsigned long *p = ((unsigned long *)addr) + BITOP_WORD(nr);
-
- __asm__ __volatile__(
-"1:" PPC_LLARX "%0,0,%3 # set_bit\n"
- "or %0,%0,%2\n"
- PPC405_ERR77(0,%3)
- PPC_STLCX "%0,0,%3\n"
- "bne- 1b"
- : "=&r" (old), "+m" (*p)
- : "r" (mask), "r" (p)
- : "cc" );
+ set_bits(BITOP_MASK(nr), addr + BITOP_WORD(nr));
}
static __inline__ void clear_bit(int nr, volatile unsigned long *addr)
{
- unsigned long old;
- unsigned long mask = BITOP_MASK(nr);
- unsigned long *p = ((unsigned long *)addr) + BITOP_WORD(nr);
-
- __asm__ __volatile__(
-"1:" PPC_LLARX "%0,0,%3 # clear_bit\n"
- "andc %0,%0,%2\n"
- PPC405_ERR77(0,%3)
- PPC_STLCX "%0,0,%3\n"
- "bne- 1b"
- : "=&r" (old), "+m" (*p)
- : "r" (mask), "r" (p)
- : "cc" );
+ clear_bits(BITOP_MASK(nr), addr + BITOP_WORD(nr));
}
static __inline__ void clear_bit_unlock(int nr, volatile unsigned long *addr)
{
- unsigned long old;
- unsigned long mask = BITOP_MASK(nr);
- unsigned long *p = ((unsigned long *)addr) + BITOP_WORD(nr);
-
- __asm__ __volatile__(
- LWSYNC_ON_SMP
-"1:" PPC_LLARX "%0,0,%3 # clear_bit_unlock\n"
- "andc %0,%0,%2\n"
- PPC405_ERR77(0,%3)
- PPC_STLCX "%0,0,%3\n"
- "bne- 1b"
- : "=&r" (old), "+m" (*p)
- : "r" (mask), "r" (p)
- : "cc", "memory");
+ clear_bits_unlock(BITOP_MASK(nr), addr + BITOP_WORD(nr));
}
static __inline__ void change_bit(int nr, volatile unsigned long *addr)
{
- unsigned long old;
- unsigned long mask = BITOP_MASK(nr);
- unsigned long *p = ((unsigned long *)addr) + BITOP_WORD(nr);
-
- __asm__ __volatile__(
-"1:" PPC_LLARX "%0,0,%3 # change_bit\n"
- "xor %0,%0,%2\n"
- PPC405_ERR77(0,%3)
- PPC_STLCX "%0,0,%3\n"
- "bne- 1b"
- : "=&r" (old), "+m" (*p)
- : "r" (mask), "r" (p)
- : "cc" );
+ change_bits(BITOP_MASK(nr), addr + BITOP_WORD(nr));
+}
+
+/* Like DEFINE_BITOP(), with changes to the arguments to 'op' and the output
+ * operands. */
+#define DEFINE_TESTOP(fn, op, prefix, postfix) \
+static __inline__ unsigned long fn( \
+ unsigned long mask, \
+ volatile unsigned long *_p) \
+{ \
+ unsigned long old, t; \
+ unsigned long *p = (unsigned long *)_p; \
+ __asm__ __volatile__ ( \
+ prefix \
+"1:" PPC_LLARX "%0,0,%3\n" \
+ stringify_in_c(op) "%1,%0,%2\n" \
+ PPC405_ERR77(0,%3) \
+ PPC_STLCX "%1,0,%3\n" \
+ "bne- 1b\n" \
+ postfix \
+ : "=&r" (old), "=&r" (t) \
+ : "r" (mask), "r" (p) \
+ : "cc", "memory"); \
+ return (old & mask); \
}
+DEFINE_TESTOP(test_and_set_bits, or, LWSYNC_ON_SMP, ISYNC_ON_SMP)
+DEFINE_TESTOP(test_and_set_bits_lock, or, "", ISYNC_ON_SMP)
+DEFINE_TESTOP(test_and_clear_bits, andc, LWSYNC_ON_SMP, ISYNC_ON_SMP)
+DEFINE_TESTOP(test_and_change_bits, xor, LWSYNC_ON_SMP, ISYNC_ON_SMP)
+
static __inline__ int test_and_set_bit(unsigned long nr,
volatile unsigned long *addr)
{
- unsigned long old, t;
- unsigned long mask = BITOP_MASK(nr);
- unsigned long *p = ((unsigned long *)addr) + BITOP_WORD(nr);
-
- __asm__ __volatile__(
- LWSYNC_ON_SMP
-"1:" PPC_LLARX "%0,0,%3 # test_and_set_bit\n"
- "or %1,%0,%2 \n"
- PPC405_ERR77(0,%3)
- PPC_STLCX "%1,0,%3 \n"
- "bne- 1b"
- ISYNC_ON_SMP
- : "=&r" (old), "=&r" (t)
- : "r" (mask), "r" (p)
- : "cc", "memory");
-
- return (old & mask) != 0;
+ return test_and_set_bits(BITOP_MASK(nr), addr + BITOP_WORD(nr)) != 0;
}
static __inline__ int test_and_set_bit_lock(unsigned long nr,
volatile unsigned long *addr)
{
- unsigned long old, t;
- unsigned long mask = BITOP_MASK(nr);
- unsigned long *p = ((unsigned long *)addr) + BITOP_WORD(nr);
-
- __asm__ __volatile__(
-"1:" PPC_LLARX "%0,0,%3 # test_and_set_bit_lock\n"
- "or %1,%0,%2 \n"
- PPC405_ERR77(0,%3)
- PPC_STLCX "%1,0,%3 \n"
- "bne- 1b"
- ISYNC_ON_SMP
- : "=&r" (old), "=&r" (t)
- : "r" (mask), "r" (p)
- : "cc", "memory");
-
- return (old & mask) != 0;
+ return test_and_set_bits_lock(BITOP_MASK(nr),
+ addr + BITOP_WORD(nr)) != 0;
}
static __inline__ int test_and_clear_bit(unsigned long nr,
volatile unsigned long *addr)
{
- unsigned long old, t;
- unsigned long mask = BITOP_MASK(nr);
- unsigned long *p = ((unsigned long *)addr) + BITOP_WORD(nr);
-
- __asm__ __volatile__(
- LWSYNC_ON_SMP
-"1:" PPC_LLARX "%0,0,%3 # test_and_clear_bit\n"
- "andc %1,%0,%2 \n"
- PPC405_ERR77(0,%3)
- PPC_STLCX "%1,0,%3 \n"
- "bne- 1b"
- ISYNC_ON_SMP
- : "=&r" (old), "=&r" (t)
- : "r" (mask), "r" (p)
- : "cc", "memory");
-
- return (old & mask) != 0;
+ return test_and_clear_bits(BITOP_MASK(nr), addr + BITOP_WORD(nr)) != 0;
}
static __inline__ int test_and_change_bit(unsigned long nr,
volatile unsigned long *addr)
{
- unsigned long old, t;
- unsigned long mask = BITOP_MASK(nr);
- unsigned long *p = ((unsigned long *)addr) + BITOP_WORD(nr);
-
- __asm__ __volatile__(
- LWSYNC_ON_SMP
-"1:" PPC_LLARX "%0,0,%3 # test_and_change_bit\n"
- "xor %1,%0,%2 \n"
- PPC405_ERR77(0,%3)
- PPC_STLCX "%1,0,%3 \n"
- "bne- 1b"
- ISYNC_ON_SMP
- : "=&r" (old), "=&r" (t)
- : "r" (mask), "r" (p)
- : "cc", "memory");
-
- return (old & mask) != 0;
-}
-
-static __inline__ void set_bits(unsigned long mask, unsigned long *addr)
-{
- unsigned long old;
-
- __asm__ __volatile__(
-"1:" PPC_LLARX "%0,0,%3 # set_bits\n"
- "or %0,%0,%2\n"
- PPC_STLCX "%0,0,%3\n"
- "bne- 1b"
- : "=&r" (old), "+m" (*addr)
- : "r" (mask), "r" (addr)
- : "cc");
+ return test_and_change_bits(BITOP_MASK(nr), addr + BITOP_WORD(nr)) != 0;
}
#include <asm-generic/bitops/non-atomic.h>
--
1.5.6.3
^ permalink raw reply related
* Problem with memcpy on ppc8536
From: Fahd Abidi @ 2009-06-18 20:33 UTC (permalink / raw)
To: linuxppc-dev
[-- Attachment #1: Type: text/plain, Size: 1741 bytes --]
Hello,
I am trying to debug a crash during memcpy while copying data from the
FCM buffer of an mpc8536 to the ddr ram. I debugged memcpy enough
through my BDI3000 to see that the entire contents of the fcm buffer I
want moved are actually moved off to memory location I specify. The
crash comes after it memcpy finishes copying and performs the
instruction "2: cmplwi 0,r5,4":
memcpy:
rlwinm. r7,r5,32-3,3,31 /* r0 = r5 >> 3 */
addi r6,r3,-4
addi r4,r4,-4
beq 2f /* if less than 8 bytes to do */
andi. r0,r6,3 /* get dest word aligned */
mtctr r7
bne 5f
1: lwz r7,4(r4)
lwzu r8,8(r4)
stw r7,4(r6)
stwu r8,8(r6)
bdnz 1b
andi. r5,r5,7
2: cmplwi 0,r5,4
contents of r5 are 0x0 showing that the entire 0x1000 size transfer I
specified correctly finished. At this point I can check the memory
contents through my BDI and verify all the data is in the ddr as I
expect. Now the strange thing happens, if I execute the "cmplwi 0,r5,4"
the program takes an exception and eventually gets struck at exception
vector 0x700 which I saw from start.S is an "Alignment" exception. I am
not sure what this exception means, can someone help me understand what
is happening?
Does this exception somehow mean that memcpy did not move all the data?
Does memcpy expect contents of r5 to be 4 and not 0 when it hits the
cmplwi instruction?
Fahd Abidi
Product Manager - Technical Tools and Development
Ultimate Solutions, Inc.
================================================================
Your Single Source for Professional Development Tools and Embedded
Solutions
Ph: 978-455-3383 x255
Fx: 978-926-3091
Email: fabidi@ultsol.com
Visit: http://www.ultsol.com <http://www.ultsol.com/>
[-- Attachment #2: Type: text/html, Size: 3136 bytes --]
^ permalink raw reply
* Re: [RFC PATCH] fsldma: Add DMA_SLAVE support
From: Ira Snyder @ 2009-06-18 20:50 UTC (permalink / raw)
To: Dan Williams; +Cc: linuxppc-dev@ozlabs.org, Li Yang
In-Reply-To: <4A3A8463.7080601@intel.com>
On Thu, Jun 18, 2009 at 11:16:03AM -0700, Dan Williams wrote:
> Ira Snyder wrote:
>> I can do something similar by calling device_prep_dma_memcpy() lots of
>> times. Once for each page in the scatterlist.
>>
>> This has the advantage of not changing the DMAEngine API.
>>
>> This has the disadvantage of not being a single transaction. The DMA
>> controller will get an interrupt after each memcpy() transaction, clean
>> up descriptors, etc.
>>
>
> Why would it need an interrupt per memcpy? There is a
> DMA_PREP_INTERRUPT flag to gate interrupt generation to the last
> descriptor. This is how async_tx delays callbacks until the last
> operation in a chain has completed. Also, I am not currently seeing how
> your implementation achieves a single hardware transaction. It still
> calls fsl_dma_alloc_descriptor() per address pair?
>
Ok, there are a few things here:
1) an interrupt per memcpy
The *software* using DMAEngine doesn't need one interrupt per memcpy.
That is controlled by the DMA_PREP_INTERRUPT flag, exactly as you
describe.
The Freescale DMA driver DOES use one interrupt per async_tx descriptor.
See drivers/dma/fsldma.c dma_init() and append_ld_queue().
The FSL_DMA_MR_EOTIE flag in dma_init() tells the controller to generate
an interrupt at the end of each transfer. A transfer is (potentially
long) list of address pairs / hardware descriptors.
The FSL_DMA_MR_EOSIE flag in append_ld_queue() tells the controller to
generate an interrupt at the end of transferring this hardware
descriptor (AKA segment). The driver combines multiple memcpy operations
into a single transfer. When the driver combines operations, it adds the
EOSIE flag to the descriptor that would-have-been the end of a transfer.
It uses this interrupt to update the DMA cookie, presumably to speed up
users of dma_sync_wait() when there are lots of users sharing the DMA
controller.
Let me try to explain what will happen with some code:
=== Case 1: Two seperate transfers ===
dma_cookie_t t1, t2;
t1 = dma_async_memcpy_buf_to_buf(...);
dma_async_memcpy_issue_pending();
/*
* some time goes by, the DMA transfer completes,
* and the controller gets the end-of-transfer interrupt
*/
t2 = dma_async_memcpy_buf_to_buf(...);
dma_async_memcpy_issue_pending();
/*
* some time goes by, the DMA transfer completes,
* and the controller gets the end-of-transfer interrupt
*/
This is exactly what I would expect from the API. There are two seperate
transfers, and there are two hardware interrupts. Here is a crude
timeline.
----|----------|----------------------------|----------|-------
| | | |
t1 start t1 end, EOT interrupt t2 start t2 end, EOT interrupt
=== Case 2: Two seperate transfers, merged by the driver ===
t1 = dma_async_memcpy_buf_to_buf(...);
dma_async_memcpy_issue_pending();
/*
* the controller starts executing the transfer, but has not
* finished yet
*/
t2 = dma_async_memcpy_buf_to_buf(...);
/*
* append_ld_queue() sets the EOSIE flag on the last hardware
* descriptor in t1, then sets the next link descriptor to the
* first descriptor in t2
*/
Now there are two transfers, and again two hardware interrupts. Again,
not really a problem. Every part of the API still works as expected.
----|-----------|---------------|---------------------------------|--------
| | | |
t1 start t2 tx_submit() t1 end, EOS interrupt, t2 start t2 end, EOT interrupt
=== Case 3: Two transfers, merged by the driver ===
t1 = dma_async_memcpy_buf_to_buf(...);
t2 = dma_async_memcpy_buf_to_buf(...);
dma_async_memcpy_issue_pending();
After a very careful reading of the driver, I just noticed that if there
is no transfer in progress (as would be expected on a private channel)
then the EOS interrupt never gets set, and the requests are simply
merged together. This would lead to a timeline like this:
----|----------------|----------------------|--------------------------
| | |
t1 start t1 end, t2 start t2 end, EOT interrupt
This is perfectly fine as well. It is the behavior I want.
Some more study of the code shows that the Freescale DMA driver will not
halt the channel as long as the channel is busy, meaning that it will
not clear the external start bits during a transfer.
So, in conclusion, I can call memcpy multiple times and have it all
merged into a single transfer by the driver, with a single hardware
interrupt (at the end-of-transfer) and have everything complete without
halting the DMA controller.
2) Single transaction
I think we're using different terms here. I define a single transaction
to be the time while the DMA controller is busy transferring things.
In case #1 above, there are two transfers. In case #2 above, one
transfer, and two interrupts. In case #3 above, one transfer, one
interrupt.
3) Hardware descriptor per address pair
Yes, there can be many hardware descriptors per address pair. And
therefore many hardware descriptors per transaction, with my definition.
> The api currently allows a call to ->prep_* to generate multiple
> internal descriptors for a single input address (i.e. memcpy in the case
> where the transfer length exceeds the hardware maximum). The slave api
> allows for transfers from a scatterlist to a slave context. I think
> what is bothering me is that the fsldma slave implementation is passing
> a list of sideband addresses rather than a constant address context like
> the existing dw_dmac, so it is different. However, I can now see that
> trying to enforce uniformity in this area is counterproductive. The
> DMA_SLAVE interface will always have irreconcilable differences across
> architectures.
>
Yep, we're in agreement here.
In another driver, I used this DMA_SLAVE API to transfer from hardware
addresses to a scatterlist. I have ~50 blocks, all at different
non-contiguous addresses that I want transferred into a single
scatterlist.
It was awfully convenient to have this happen in a single call, rather
than 50 seperate calls to dma_async_memcpy_buf_to_buf().
>> It also has the disadvantage of not being able to change the
>> controller-specific features I'm using: external start. This feature
>> lets the "3rd device" control the DMA controller. It looks like the
>> atmel-mci driver has a similar feature. The DMAEngine API has no way to
>> expose this type of feature except through DMA_SLAVE.
>
> Yeah, an example of an architecture specific quirk that has no chance of
> being uniformly handled in a generic api.
>
Again, we're in agreement.
>> If it is just the 3 helper routines that are the issue with this patch,
>> I have no problem dropping them and letting each user re-create them
>> themselves.
>
> I think this makes the most sense at this point. We can reserve
> judgement on the approach until the next fsldma-slave user arrives and
> tries to use this implementation for their device. Can we move the
> header file under arch/powerpc/include?
>
Sure, that would be fine.
> [..]
>> A single-transaction scatterlist-to-scatterlist copy would be nice.
>>
>> One of the major advantages of working with the DMA controller is that
>> it automatically handles scatter/gather. Almost all DMA controllers have
>> the feature, but it is totally missing from the high-level API.
>
> The engines I am familiar with (ioatdma and iop-adma) still need a
> hardware descriptor per address pair I do not see how fsldma does this
> any more automatically than those engines? I could see having a helper
> routine that does the mapping and iterating, but in the end the driver
> still sees multiple calls to ->prep (unless of course you are doing this
> in a DMA_SLAVE context, then anything goes).
>
Yep. The Freescale DMA controller behaves exactly as you describe the
ioatdma and iop-dma engines. A helper routine that does the mapping and
iterating would be nice, in my opinion. It took me a while to convince
myself that the nested loops in device_prep_slave_sg() were correct.
Following on the earlier observations in this email, it would be
possible to emulate the slave behavior using just
dma_async_memcpy_buf_to_buf(). With the exception of the external start
bit, of course.
Now, the only thing I would use device_prep_slave_sg() for would be to
set the external start bit. No actual data transfer needs to happen, no
descriptors need to be allocated. Just the "Enable extra controller
features" block from my patch. This seems like an abuse of the DMA_SLAVE
API. What would be returned in that case? An async_tx descriptor is
wrong, because the controller won't be able to free() it...
So, I see a couple of ways of moving forward:
1) Keep my implementation, moving the includes to arch/powerpc/include.
Do we drop the allocation functions?
2) Drop the data transfer part of device_prep_slave_sg(), and just use
it for setting device-specific bits.
Thanks for all the input Dan. I finally feel like we're getting
somewhere :)
Ira
^ permalink raw reply
* Re: [PATCH] RFC: powerpc: expose the multi-bit ops that underlie single-bit ops.
From: Geoff Thorpe @ 2009-06-18 20:30 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev
In-Reply-To: <1245188026.14036.17.camel@pasglop>
Hi Ben et al,
Benjamin Herrenschmidt wrote:
> On Tue, 2009-06-16 at 10:28 -0400, Geoff Thorpe wrote:
[snip]
>>> Maybe we can shrink that file significantly (and avoid the risk for
>>> typos etc...) by generating them all from a macro.
>>>
>>> Something like (typed directly into the mailer :-)
>>>
>>> #define DEFINE_BITOP(op, prefix, postfix) \
>>> asm volatile ( \
>>> prefix \
>>> "1:" PPC_LLARX "%0,0,%3\n" \
>>> __stringify(op) "%1,%0,%2\n" \
>>> PPC405_ERR77(0,%3) \
>>> PPC_STLCX "%1,0,%3\n" \
>>> "bne- 1b\n" \
>>> postfix \
>>> : "=&r" (old), "=&r" (t)
>>> : "r" (mask), "r" (p)
>>> : "cc", "memory")
>>>
>>> and so:
>>>
>>> static inline void set_bits(unsigned long mask, volatile unsigned long *addr)
>>> {
>>> unsigned long old, t;
>>>
>>> DEFINE_BITOP(or, "", "");
>>> }
>>>
>>> static inline void test_and_set_bits(unsigned long mask, volatile unsigned long *addr)
>>> {
>>> unsigned long old, t;
>>>
>>> DEFINE_BITOP(or, LWSYNC_ON_SMP, ISYNC_ON_SMP);
>>>
>>> return (old & mask) != 0;
>>> }
>>>
>>> etc...
>>
>> Sounds good, I'll try working this up and I'll send a new patch shortly.
>
> You can also go totally mad and generate the whole function (both -s and
> non -s variants) from one macro but I wouldn't go that far :-)
I've prepared a new patch, will send it in a moment. It uses two macros
rather than one - as the test_and_***() APIs have a fundamentally
different asm because of the arguments to 'op' as well as the output
operands. However, this split made it possible to generate the entire
"inner" (single-word) function using the macro, rather than just the
inline asm part.
>
>> So can I assume implicitly that changing the set_bits() function to add
>> the 'volatile' qualifier to the prototype (and the missing
>> PPC405_ERR77() workaround) was OK?
>
> The PPC405_ERR77 workaround is definitely needed. The volatile, well, I
> suspect it's useless, but it will remove warnings when callers call
> these on something that is declared as volatile in the first place.
>
> Do x86 use volatile there ? If not, then don't do it on powerpc neither,
> it could well be an historical remain. It's not functionally useful, the
> "memory" clobber in the asm takes care of telling the compiler not to
> mess around I believe.
I've left the volatile qualifier in the generated API because I didn't
feel so comfortable changing APIs, but I also added the "memory" clobber
for all cases - whereas it seems the existing set_bits(), clear_bits(),
[...] functions didn't declare this... Do you see any issue with having
the 'volatile' in the prototype as well as the clobber in the asm?
Actually, might as well just respond to the new patch instead... :-) Thx.
Cheers,
Geoff
^ permalink raw reply
* Re: [Patch 2/6] Introduce PPC64 specific Hardware Breakpoint interfaces
From: K.Prasad @ 2009-06-18 18:20 UTC (permalink / raw)
To: David Gibson
Cc: Michael Neuling, Benjamin Herrenschmidt, linuxppc-dev, paulus,
Alan Stern, Roland McGrath
In-Reply-To: <20090617043224.GL486@yookeroo.seuss>
On Wed, Jun 17, 2009 at 02:32:24PM +1000, David Gibson wrote:
> On Wed, Jun 10, 2009 at 02:38:06PM +0530, K.Prasad wrote:
> > Introduce PPC64 implementation for the generic hardware breakpoint interfaces
> > defined in kernel/hw_breakpoint.c. Enable the HAVE_HW_BREAKPOINT flag and the
> > Makefile.
>
> [snip]
> > +int arch_store_info(struct hw_breakpoint *bp, struct task_struct *tsk)
> > +{
> > + /* Symbol names from user-space are rejected */
> > + if (tsk) {
> > + if (bp->info.name)
> > + return -EINVAL;
> > + else
> > + return 0;
> > + }
> > + /*
> > + * User-space requests will always have the address field populated
> > + * For kernel-addresses, either the address or symbol name can be
> > + * specified.
> > + */
> > + if (bp->info.name)
> > + bp->info.address = (unsigned long)
> > + kallsyms_lookup_name(bp->info.name);
> > + if (bp->info.address)
> > + if (kallsyms_lookup_size_offset(bp->info.address,
> > + &(bp->info.symbolsize), NULL))
> > + return 0;
> > + return -EINVAL;
> > +}
> > +
> > +/*
> > + * Validate the arch-specific HW Breakpoint register settings
> > + */
> > +int arch_validate_hwbkpt_settings(struct hw_breakpoint *bp,
> > + struct task_struct *tsk)
> > +{
> > + int is_kernel, ret = -EINVAL;
> > +
> > + if (!bp)
> > + return ret;
> > +
> > + switch (bp->info.type) {
> > + case HW_BREAKPOINT_READ:
> > + case HW_BREAKPOINT_WRITE:
> > + case HW_BREAKPOINT_RW:
> > + break;
> > + default:
> > + return ret;
> > + }
> > +
> > + if (bp->triggered)
> > + ret = arch_store_info(bp, tsk);
>
> Under what circumstances would triggered be NULL? It's not clear to
> me that you wouldn't still need arch_store_info() in this case.
>
Simple, triggered would be NULL when a user fails to specify it!
This function would return EINVAL if that's the case.
> > +
> > + is_kernel = is_kernel_addr(bp->info.address);
> > + if ((tsk && is_kernel) || (!tsk && !is_kernel))
> > + return -EINVAL;
> > +
> > + return ret;
> > +}
> > +
<snipped>
> > +/*
> > + * Handle debug exception notifications.
> > + */
> > +int __kprobes hw_breakpoint_handler(struct die_args *args)
> > +{
> > + int rc = NOTIFY_STOP;
> > + struct hw_breakpoint *bp;
> > + struct pt_regs *regs = args->regs;
> > + unsigned long dar = regs->dar;
> > + int cpu, is_one_shot, stepped = 1;
> > +
> > + /* Disable breakpoints during exception handling */
> > + set_dabr(0);
> > +
> > + cpu = get_cpu();
> > + /* Determine whether kernel- or user-space address is the trigger */
> > + bp = (hbp_kernel_pos == HBP_NUM) ? current->thread.hbp[0] :
> > + per_cpu(this_hbp_kernel[0], cpu);
> > + /*
> > + * bp can be NULL due to lazy debug register switching
> > + * or due to the delay between updates of hbp_kernel_pos
> > + * and this_hbp_kernel.
> > + */
> > + if (!bp)
> > + goto out;
> > +
> > + is_one_shot = (bp->triggered == ptrace_triggered) ? 1 : 0;
> > + per_cpu(dabr_data, cpu) = (hbp_kernel_pos == HBP_NUM) ?
> > + current->thread.dabr : kdabr;
> > +
> > + /* Verify if dar lies within the address range occupied by the symbol
> > + * being watched. Since we cannot get the symbol size for
> > + * user-space requests we skip this check in that case
> > + */
> > + if ((hbp_kernel_pos == 0) &&
> > + !((bp->info.address <= dar) &&
> > + (dar <= (bp->info.address + bp->info.symbolsize))))
> > + /*
> > + * This exception is triggered not because of a memory access on
> > + * the monitored variable but in the double-word address range
> > + * in which it is contained. We will consume this exception,
> > + * considering it as 'noise'.
> > + */
> > + goto out;
> > +
> > + (bp->triggered)(bp, regs);
>
> So this confuses me. You go to great efforts to step over the
> instruction to generate a SIGTRAP after the instruction, for
> consistency with x86. But that SIGTRAP is *never* used, since the
> only way to set userspace breakpoints is through ptrace at the moment.
> At the same time, the triggered function is called here before the
> instruction is executed, so not consistent with x86 anyway.
>
> It just seems strange to me that sending a SIGTRAP is a special case
> anyway. Why can't sending a SIGTRAP be just a particular triggered
> function.
>
The consistency in the interface across architectures that I referred to
in my previous mail was w.r.t. continuous triggering of breakpoints and
not to implement a trigger-before or trigger-after behaviour across
architectures. In fact, this behaviour differs even on the same
processor depending upon the breakpoint type (trigger-before for
instructions and trigger-after for data in x86), and introducing
uniformity might be a) at the cost of loss of some unique & innovative
uses for each of them b) may not be always possible e.g. trigger-after
to trigger-before.
Hope this resolves the confusion (that I might have inadvertently caused
in my previous mail)!
Thanks,
K.Prasad
^ permalink raw reply
* Re: [RFC PATCH] fsldma: Add DMA_SLAVE support
From: Dan Williams @ 2009-06-18 18:16 UTC (permalink / raw)
To: Ira Snyder; +Cc: linuxppc-dev@ozlabs.org, Li Yang
In-Reply-To: <20090617182947.GB4707@ovro.caltech.edu>
Ira Snyder wrote:
> I can do something similar by calling device_prep_dma_memcpy() lots of
> times. Once for each page in the scatterlist.
>
> This has the advantage of not changing the DMAEngine API.
>
> This has the disadvantage of not being a single transaction. The DMA
> controller will get an interrupt after each memcpy() transaction, clean
> up descriptors, etc.
>
Why would it need an interrupt per memcpy? There is a
DMA_PREP_INTERRUPT flag to gate interrupt generation to the last
descriptor. This is how async_tx delays callbacks until the last
operation in a chain has completed. Also, I am not currently seeing how
your implementation achieves a single hardware transaction. It still
calls fsl_dma_alloc_descriptor() per address pair?
The api currently allows a call to ->prep_* to generate multiple
internal descriptors for a single input address (i.e. memcpy in the case
where the transfer length exceeds the hardware maximum). The slave api
allows for transfers from a scatterlist to a slave context. I think
what is bothering me is that the fsldma slave implementation is passing
a list of sideband addresses rather than a constant address context like
the existing dw_dmac, so it is different. However, I can now see that
trying to enforce uniformity in this area is counterproductive. The
DMA_SLAVE interface will always have irreconcilable differences across
architectures.
> It also has the disadvantage of not being able to change the
> controller-specific features I'm using: external start. This feature
> lets the "3rd device" control the DMA controller. It looks like the
> atmel-mci driver has a similar feature. The DMAEngine API has no way to
> expose this type of feature except through DMA_SLAVE.
Yeah, an example of an architecture specific quirk that has no chance of
being uniformly handled in a generic api.
> If it is just the 3 helper routines that are the issue with this patch,
> I have no problem dropping them and letting each user re-create them
> themselves.
I think this makes the most sense at this point. We can reserve
judgement on the approach until the next fsldma-slave user arrives and
tries to use this implementation for their device. Can we move the
header file under arch/powerpc/include?
[..]
> A single-transaction scatterlist-to-scatterlist copy would be nice.
>
> One of the major advantages of working with the DMA controller is that
> it automatically handles scatter/gather. Almost all DMA controllers have
> the feature, but it is totally missing from the high-level API.
The engines I am familiar with (ioatdma and iop-adma) still need a
hardware descriptor per address pair I do not see how fsldma does this
any more automatically than those engines? I could see having a helper
routine that does the mapping and iterating, but in the end the driver
still sees multiple calls to ->prep (unless of course you are doing this
in a DMA_SLAVE context, then anything goes).
Thanks,
Dan
^ permalink raw reply
* Re: [Patch 4/6] Modify process and processor handling code to recognise hardware debug registers
From: K.Prasad @ 2009-06-18 17:56 UTC (permalink / raw)
To: David Gibson
Cc: Michael Neuling, Benjamin Herrenschmidt, linuxppc-dev, paulus,
Alan Stern, Roland McGrath
In-Reply-To: <20090617041420.GK486@yookeroo.seuss>
On Wed, Jun 17, 2009 at 02:14:20PM +1000, David Gibson wrote:
> On Wed, Jun 10, 2009 at 02:38:18PM +0530, K.Prasad wrote:
> > Modify process handling code to recognise hardware debug registers during copy
> > and flush operations. Introduce a new TIF_DEBUG task flag to indicate a
> > process's use of debug register. Load the debug register values into a
> > new CPU during initialisation.
> >
> > Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
> > ---
> > arch/powerpc/kernel/process.c | 15 +++++++++++++++
> > arch/powerpc/kernel/smp.c | 2 ++
> > 2 files changed, 17 insertions(+)
> >
> > Index: linux-2.6-tip.hbkpt/arch/powerpc/kernel/process.c
> > ===================================================================
> > --- linux-2.6-tip.hbkpt.orig/arch/powerpc/kernel/process.c
> > +++ linux-2.6-tip.hbkpt/arch/powerpc/kernel/process.c
> > @@ -50,6 +50,7 @@
> > #include <asm/syscalls.h>
> > #ifdef CONFIG_PPC64
> > #include <asm/firmware.h>
> > +#include <asm/hw_breakpoint.h>
> > #endif
> > #include <linux/kprobes.h>
> > #include <linux/kdebug.h>
> > @@ -254,8 +255,10 @@ void do_dabr(struct pt_regs *regs, unsig
> > 11, SIGSEGV) == NOTIFY_STOP)
> > return;
> >
> > +#ifndef CONFIG_PPC64
> > if (debugger_dabr_match(regs))
> > return;
> > +#endif
>
> Won't this disable the check for breakpoints set by xmon - but I don't
> see anything in this patch series to convert xmon to use the new
> breakpoint interface instead.
>
As noted by me here:
http://lists.ozlabs.org/pipermail/linuxppc-dev/2009-May/071832.html the
Xmon integration is pending. When I tried to study and integrate Xmon, I
found that the HW Breakpoint triggering was broken as of 2.6.29 kernel
(tested on a Power5 box).
This would mean that if Xmon's hardware breakpoint infrastructure is
used in tandem with the given breakpoint interfaces, they would conflict
with each other resulting in difficult-to-predict behaviour (the last to
grab the register will use it).
I think that tidying up do_dabr() is best done along with Xmon
integration.
> --
> David Gibson | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
> | _way_ _around_!
> http://www.ozlabs.org/~dgibson
Thanks,
K.Prasad
^ permalink raw reply
* killing use of ppc_md.init
From: Kumar Gala @ 2009-06-18 14:38 UTC (permalink / raw)
To: Gerhard Pircher, Benjamin Herrenschmidt; +Cc: linuxppc-dev list
ppc_md.init only exists on ppc32 and seems like its pretty useless
today. The users seem to fall into two classes:
1. called to do of_platform_bus_probe() - most platforms use
machine_device_initcall() for this
2. some platform init code which seems like it could move into
setup_arch().
The second one seems to only be on amigaone and chrp. Anyone know if
there is any harm in moving the amigaone_init() into
amigaone_setup_arch() and similarly on chrp chrp_init2() into
chrp_setup_arch().
- k
^ permalink raw reply
* Re: [spi-devel-general] [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
From: Wolfram Sang @ 2009-06-18 14:26 UTC (permalink / raw)
To: Grant Likely
Cc: linuxppc-dev, David Brownell, linux-kernel, spi-devel-general
In-Reply-To: <fa686aa40906180635g45724119x99cd4d1e4ede59f9@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2272 bytes --]
Hi Grant,
> The double spaces at the end of sentences are intentional. It is
> perhaps a bit quaint and old fashioned, but it is my writing style.
Ah, okay.
> >> +
> >> + /* Statistics */
> >> + int msg_count;
> >> + int wcol_count;
> >> + int wcol_ticks;
> >> + u32 wcol_tx_timestamp;
> >> + int modf_count;
> >> + int byte_count;
> >
> > Hmm, there isn't even a debug-printout for those. Putting #ifdef DEBUG around
> > them will be ugly. Well, I can't make up if this is just overhead or useful
> > debugging aid, so no real objection :)
>
> There used to be a sysfs interface for dumping these, but it was an
> ugly misuse. I'd like to leave these in. I still have the sysfs bits
> in a private patch and I'm going to rework them for debugfs.
Okay. Maybe a comment stating the future use will be nice.
>
> > But I wonder more about the usage of the SS pin and if this chipsel is needed
> > at all (sadly I cannot test as I don't have any board with SPI connected to
> > that device). You define the SS-pin as output, but do not set the SSOE-bit.
> > More, you use the MODF-feature, so the SS-pin should be defined as input?
> > According to Table 17.3 in the PM, you have that pin defined as generic purpose
> > output.
>
> That's right. The SS handling by the SPI device is completely
> useless, so this driver uses it as a GPIO and asserts it manually.
That definately needs a comment :D (perhaps with some more details if you know them).
> The MODF irq is probably irrelevant, but I'd like to leave it in for
> completeness.
But it won't work if the pin is set to output, no? Are you sure there are no
side-effects?
> >> + /* initialize the device */
> >> + out_8(regs+SPI_CTRL1, SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR);
> >
> > spaces around operator.
>
> Intentional to keep from spilling past column 80; but I'll move the
> missing spaces to around the | operators. I think it is easier to
> read that way.
Ah, I remember, we had this topic a while ago :D
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: [PATCH] powerpc: Add configurable -Werror for arch/powerpc
From: Arnd Bergmann @ 2009-06-18 14:22 UTC (permalink / raw)
To: linuxppc-dev; +Cc: linuxppc-dev list, Timur Tabi
In-Reply-To: <ed82fe3e0906150754w125c6283q2468213b59c7f114@mail.gmail.com>
On Monday 15 June 2009, Timur Tabi wrote:
> On Mon, Jun 15, 2009 at 2:50 AM, Michael Ellerman<michael@ellerman.id.au> wrote:
> > arch/powerpc/platforms/chrp/setup.c:378: error: the frame size of 1040 bytes is larger than 1024 bytes
>
> What's so bad about a frame size larger than 1024?
>
It's not necessarily a bug, but all frame sizes in the call chain
combined must never exceed the kernel stack size. 1024 is an
arbitrary limit that we warn about for a single function, because
it's likely that things will break if you have more than one of these.
Arnd <><
^ permalink raw reply
* Re: [PATCH -mm][POWERPC] mpc8xxx : allow SPI without cs.
From: Kumar Gala @ 2009-06-18 14:04 UTC (permalink / raw)
To: avorontsov; +Cc: spi-devel-general, Rini van Zetten, linuxppc-dev list
In-Reply-To: <20090618130955.GA1369@oksana.dev.rtsoft.ru>
On Jun 18, 2009, at 8:09 AM, Anton Vorontsov wrote:
> On Thu, Jun 18, 2009 at 08:19:44AM +0200, Rini van Zetten wrote:
>> This patch adds the possibility to have a spi device without a cs.
>>
>> For example, the dts file should look something like this:
>>
>> spi-controller {
>> gpios = <&pio1 1 0 /* cs0 */
>> 0 /* cs1, no GPIO */
>> &pio2 2 0>; /* cs2 */
>>
>
> Interesting scheme. I guess this is for eSPI controllers that can
> do their own chip-selects, but we want GPIO chip selects in addition
> (or in place of built-in ones), correct?
That is a good question. What HW is this for (I don't think its for
eSPI but I could be wrong).
- k
^ permalink raw reply
* Re: [spi-devel-general] [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
From: Grant Likely @ 2009-06-18 13:35 UTC (permalink / raw)
To: Wolfram Sang
Cc: linuxppc-dev, David Brownell, linux-kernel, spi-devel-general
In-Reply-To: <20090618065814.GA12942@pengutronix.de>
On Thu, Jun 18, 2009 at 12:58 AM, Wolfram Sang<w.sang@pengutronix.de> wrote=
:
> Hi Grant,
>
> some comments below:
Hi Wolfram. Thanks for the review.
> On Wed, Jun 17, 2009 at 08:55:01PM -0600, Grant Likely wrote:
>> +#include <linux/spi/mpc52xx_spi.h>
>
> Is this still needed? See last comment...
No, not needed at all. Will remove.
>> +#include <linux/of_spi.h>
>> +#include <linux/io.h>
>> +#include <asm/time.h>
>
> Really asm?
Yes, really asm. Needed for get_tlb()
>> +#include <asm/mpc52xx.h>
>> +
>> +MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
>> +MODULE_DESCRIPTION("MPC52xx SPI (non-PSC) Driver");
>> +MODULE_LICENSE("GPL");
>> +
>> +/* Register offsets */
>> +#define SPI_CTRL1 =A0 =A00x00
>> +#define SPI_CTRL1_SPIE =A0 =A0 =A0 =A0 =A0 =A0 =A0 (1 << 7)
>> +#define SPI_CTRL1_SPE =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(1 << 6)
>> +#define SPI_CTRL1_MSTR =A0 =A0 =A0 =A0 =A0 =A0 =A0 (1 << 4)
>> +#define SPI_CTRL1_CPOL =A0 =A0 =A0 =A0 =A0 =A0 =A0 (1 << 3)
>> +#define SPI_CTRL1_CPHA =A0 =A0 =A0 =A0 =A0 =A0 =A0 (1 << 2)
>> +#define SPI_CTRL1_SSOE =A0 =A0 =A0 =A0 =A0 =A0 =A0 (1 << 1)
>> +#define SPI_CTRL1_LSBFE =A0 =A0 =A0 =A0 =A0 =A0 =A0(1 << 0)
>> +
>> +#define SPI_CTRL2 =A0 =A00x01
>> +#define SPI_BRR =A0 =A0 =A0 =A0 =A0 =A0 =A00x04
>> +
>> +#define SPI_STATUS =A0 0x05
>> +#define SPI_STATUS_SPIF =A0 =A0 =A0 =A0 =A0 =A0 =A0(1 << 7)
>> +#define SPI_STATUS_WCOL =A0 =A0 =A0 =A0 =A0 =A0 =A0(1 << 6)
>> +#define SPI_STATUS_MODF =A0 =A0 =A0 =A0 =A0 =A0 =A0(1 << 4)
>> +
>> +#define SPI_DATA =A0 =A0 0x09
>> +#define SPI_PORTDATA 0x0d
>> +#define SPI_DATADIR =A00x10
>> +
>> +/* FSM state return values */
>> +#define FSM_STOP =A0 =A0 0 =A0 =A0 =A0 /* Nothing more for the state ma=
chine to */
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* do. =A0If s=
omething interesting happens */
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* then and IR=
Q will be received */
>
> s/and/an/? Throughout the comments, there is sometimes a double space.
The double spaces at the end of sentences are intentional. It is
perhaps a bit quaint and old fashioned, but it is my writing style.
>
>> +#define FSM_POLL =A0 =A0 1 =A0 =A0 =A0 /* need to poll for completion, =
an IRQ is */
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* not expecte=
d */
>> +#define FSM_CONTINUE 2 =A0 =A0 =A0 /* Keep iterating the state machine =
*/
>> +
>> +/* Driver internal data */
>> +struct mpc52xx_spi {
>> + =A0 =A0 struct spi_master *master;
>> + =A0 =A0 u32 sysclk;
>
> unused?
yes, fixed.
>> + =A0 =A0 void __iomem *regs;
>> + =A0 =A0 int irq0; =A0 =A0 =A0 /* MODF irq */
>> + =A0 =A0 int irq1; =A0 =A0 =A0 /* SPIF irq */
>> + =A0 =A0 int ipb_freq;
>
> unsigned int will suit it better IMHO.
right.
>> +
>> + =A0 =A0 /* Statistics */
>> + =A0 =A0 int msg_count;
>> + =A0 =A0 int wcol_count;
>> + =A0 =A0 int wcol_ticks;
>> + =A0 =A0 u32 wcol_tx_timestamp;
>> + =A0 =A0 int modf_count;
>> + =A0 =A0 int byte_count;
>
> Hmm, there isn't even a debug-printout for those. Putting #ifdef DEBUG ar=
ound
> them will be ugly. Well, I can't make up if this is just overhead or usef=
ul
> debugging aid, so no real objection :)
There used to be a sysfs interface for dumping these, but it was an
ugly misuse. I'd like to leave these in. I still have the sysfs bits
in a private patch and I'm going to rework them for debugfs.
> But I wonder more about the usage of the SS pin and if this chipsel is ne=
eded
> at all (sadly I cannot test as I don't have any board with SPI connected =
to
> that device). You define the SS-pin as output, but do not set the SSOE-bi=
t.
> More, you use the MODF-feature, so the SS-pin should be defined as input?
> According to Table 17.3 in the PM, you have that pin defined as generic p=
urpose
> output.
That's right. The SS handling by the SPI device is completely
useless, so this driver uses it as a GPIO and asserts it manually.
The MODF irq is probably irrelevant, but I'd like to leave it in for
completeness.
>> + =A0 =A0 /* initialize the device */
>> + =A0 =A0 out_8(regs+SPI_CTRL1, SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTR=
L1_MSTR);
>
> spaces around operator.
Intentional to keep from spilling past column 80; but I'll move the
missing spaces to around the | operators. I think it is easier to
read that way.
>> diff --git a/include/linux/spi/mpc52xx_spi.h b/include/linux/spi/mpc52xx=
_spi.h
>> new file mode 100644
>> index 0000000..d1004cf
>> --- /dev/null
>> +++ b/include/linux/spi/mpc52xx_spi.h
>> @@ -0,0 +1,10 @@
>> +
>> +#ifndef INCLUDE_MPC5200_SPI_H
>> +#define INCLUDE_MPC5200_SPI_H
>> +
>> +extern void mpc52xx_spi_set_premessage_hook(struct spi_master *master,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 void (*hook)(struct spi_message *m,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0void *context),
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 void *hook_context);
>> +
>> +#endif
>
> This can be dropped for now and reintroduced when needed, I think.
Yeah, this is cruft. Removed.
Thanks!
g.
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [PATCH -mm][POWERPC] mpc8xxx : allow SPI without cs.
From: Anton Vorontsov @ 2009-06-18 13:09 UTC (permalink / raw)
To: Rini van Zetten; +Cc: spi-devel-general, linuxppc-dev list
In-Reply-To: <4A39DC80.7030906@arvoo.nl>
On Thu, Jun 18, 2009 at 08:19:44AM +0200, Rini van Zetten wrote:
> This patch adds the possibility to have a spi device without a cs.
>
> For example, the dts file should look something like this:
>
> spi-controller {
> gpios = <&pio1 1 0 /* cs0 */
> 0 /* cs1, no GPIO */
> &pio2 2 0>; /* cs2 */
>
Interesting scheme. I guess this is for eSPI controllers that can
do their own chip-selects, but we want GPIO chip selects in addition
(or in place of built-in ones), correct?
> Signed-off-by: Rini van Zetten <rini@arvoo.nl>
> ---
> Changes :
> patch against 2.6.30-rc8-mm1
I assume this is v2 already, and I overlooked v1, sorry.
Technically the patch looks OK, but please fix some cosmetics issues.
checkpatch reports:
WARNING: patch prefix 'drivers' exists, appears to be a -p0 patch
WARNING: line over 80 characters
#131: FILE: spi/spi_mpc8xxx.c:714:
+ dev_err(dev, "can't request gpio #%d: %d\n", i, ret);
WARNING: line over 80 characters
#141: FILE: spi/spi_mpc8xxx.c:724:
+ dev_err(dev, "can't set output direction for gpio "
> --- drivers/spi/spi_mpc8xxx.c.org 2009-06-12 10:45:21.000000000 +0200
> +++ drivers/spi/spi_mpc8xxx.c 2009-06-12 10:54:48.000000000 +0200
> @@ -666,9 +666,10 @@ static void mpc8xxx_spi_cs_control(struc
> struct mpc8xxx_spi_probe_info *pinfo = to_of_pinfo(dev->platform_data);
> u16 cs = spi->chip_select;
> int gpio = pinfo->gpios[cs];
> - bool alow = pinfo->alow_flags[cs];
> -
> - gpio_set_value(gpio, on ^ alow);
> + if (gpio != -EEXIST) {
> + bool alow = pinfo->alow_flags[cs];
> + gpio_set_value(gpio, on ^ alow);
Please put an empty line after variable declaration.
Thanks!
--
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2
^ 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