* [PATCH 4/7] powerpc: Use pr_devel() in do_dcache_icache_coherency()
From: Michael Ellerman @ 2009-06-18 4:13 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <1fdc1f0bb73b6ff2db7f9b394331d5ef5dfc4cb5.1245298429.git.michael@ellerman.id.au>
pr_debug() can now result in code being generated even when DEBUG
is not defined. That's not really desirable in some places.
With CONFIG_DYNAMIC_DEBUG=y:
size before:
text data bss dec hex filename
2036 368 8 2412 96c arch/powerpc/mm/pgtable.o
size after:
text data bss dec hex filename
1677 248 8 1933 78d arch/powerpc/mm/pgtable.o
Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---
arch/powerpc/mm/pgtable.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c
index ae1d67c..627767d 100644
--- a/arch/powerpc/mm/pgtable.c
+++ b/arch/powerpc/mm/pgtable.c
@@ -129,12 +129,12 @@ static pte_t do_dcache_icache_coherency(pte_t pte)
page = pfn_to_page(pfn);
if (!PageReserved(page) && !test_bit(PG_arch_1, &page->flags)) {
- pr_debug("do_dcache_icache_coherency... flushing\n");
+ pr_devel("do_dcache_icache_coherency... flushing\n");
flush_dcache_icache_page(page);
set_bit(PG_arch_1, &page->flags);
}
else
- pr_debug("do_dcache_icache_coherency... already clean\n");
+ pr_devel("do_dcache_icache_coherency... already clean\n");
return __pte(pte_val(pte) | _PAGE_HWEXEC);
}
--
1.6.2.1
^ permalink raw reply related
* [PATCH 5/7] powerpc/cell: Use pr_devel() in axon_msi.c
From: Michael Ellerman @ 2009-06-18 4:13 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <1fdc1f0bb73b6ff2db7f9b394331d5ef5dfc4cb5.1245298429.git.michael@ellerman.id.au>
pr_debug() can now result in code being generated even when DEBUG
is not defined. That's not really desirable in some places.
With CONFIG_DYNAMIC_DEBUG=y:
size before:
text data bss dec hex filename
7083 1616 0 8699 21fb arch/powerpc/../axon_msi.o
size after:
text data bss dec hex filename
5772 1208 0 6980 1b44 arch/powerpc/../axon_msi.o
Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---
arch/powerpc/platforms/cell/axon_msi.c | 22 +++++++++++-----------
1 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/platforms/cell/axon_msi.c b/arch/powerpc/platforms/cell/axon_msi.c
index c71498d..aca5741 100644
--- a/arch/powerpc/platforms/cell/axon_msi.c
+++ b/arch/powerpc/platforms/cell/axon_msi.c
@@ -85,7 +85,7 @@ static inline void axon_msi_debug_setup(struct device_node *dn,
static void msic_dcr_write(struct axon_msic *msic, unsigned int dcr_n, u32 val)
{
- pr_debug("axon_msi: dcr_write(0x%x, 0x%x)\n", val, dcr_n);
+ pr_devel("axon_msi: dcr_write(0x%x, 0x%x)\n", val, dcr_n);
dcr_write(msic->dcr_host, dcr_n, val);
}
@@ -98,7 +98,7 @@ static void axon_msi_cascade(unsigned int irq, struct irq_desc *desc)
int retry = 0;
write_offset = dcr_read(msic->dcr_host, MSIC_WRITE_OFFSET_REG);
- pr_debug("axon_msi: original write_offset 0x%x\n", write_offset);
+ pr_devel("axon_msi: original write_offset 0x%x\n", write_offset);
/* write_offset doesn't wrap properly, so we have to mask it */
write_offset &= MSIC_FIFO_SIZE_MASK;
@@ -108,7 +108,7 @@ static void axon_msi_cascade(unsigned int irq, struct irq_desc *desc)
msi = le32_to_cpu(msic->fifo_virt[idx]);
msi &= 0xFFFF;
- pr_debug("axon_msi: woff %x roff %x msi %x\n",
+ pr_devel("axon_msi: woff %x roff %x msi %x\n",
write_offset, msic->read_offset, msi);
if (msi < NR_IRQS && irq_map[msi].host == msic->irq_host) {
@@ -123,12 +123,12 @@ static void axon_msi_cascade(unsigned int irq, struct irq_desc *desc)
*/
udelay(1);
retry++;
- pr_debug("axon_msi: invalid irq 0x%x!\n", msi);
+ pr_devel("axon_msi: invalid irq 0x%x!\n", msi);
continue;
}
if (retry) {
- pr_debug("axon_msi: late irq 0x%x, retry %d\n",
+ pr_devel("axon_msi: late irq 0x%x, retry %d\n",
msi, retry);
retry = 0;
}
@@ -332,7 +332,7 @@ static int axon_msi_shutdown(struct of_device *device)
struct axon_msic *msic = dev_get_drvdata(&device->dev);
u32 tmp;
- pr_debug("axon_msi: disabling %s\n",
+ pr_devel("axon_msi: disabling %s\n",
msic->irq_host->of_node->full_name);
tmp = dcr_read(msic->dcr_host, MSIC_CTRL_REG);
tmp &= ~MSIC_CTRL_ENABLE & ~MSIC_CTRL_IRQ_ENABLE;
@@ -349,7 +349,7 @@ static int axon_msi_probe(struct of_device *device,
unsigned int virq;
int dcr_base, dcr_len;
- pr_debug("axon_msi: setting up dn %s\n", dn->full_name);
+ pr_devel("axon_msi: setting up dn %s\n", dn->full_name);
msic = kzalloc(sizeof(struct axon_msic), GFP_KERNEL);
if (!msic) {
@@ -403,7 +403,7 @@ static int axon_msi_probe(struct of_device *device,
set_irq_data(virq, msic);
set_irq_chained_handler(virq, axon_msi_cascade);
- pr_debug("axon_msi: irq 0x%x setup for axon_msi\n", virq);
+ pr_devel("axon_msi: irq 0x%x setup for axon_msi\n", virq);
/* Enable the MSIC hardware */
msic_dcr_write(msic, MSIC_BASE_ADDR_HI_REG, msic->fifo_phys >> 32);
@@ -484,13 +484,13 @@ void axon_msi_debug_setup(struct device_node *dn, struct axon_msic *msic)
addr = of_translate_address(dn, of_get_property(dn, "reg", NULL));
if (addr == OF_BAD_ADDR) {
- pr_debug("axon_msi: couldn't translate reg property\n");
+ pr_devel("axon_msi: couldn't translate reg property\n");
return;
}
msic->trigger = ioremap(addr, 0x4);
if (!msic->trigger) {
- pr_debug("axon_msi: ioremap failed\n");
+ pr_devel("axon_msi: ioremap failed\n");
return;
}
@@ -498,7 +498,7 @@ void axon_msi_debug_setup(struct device_node *dn, struct axon_msic *msic)
if (!debugfs_create_file(name, 0600, powerpc_debugfs_root,
msic, &fops_msic)) {
- pr_debug("axon_msi: debugfs_create_file failed!\n");
+ pr_devel("axon_msi: debugfs_create_file failed!\n");
return;
}
}
--
1.6.2.1
^ permalink raw reply related
* [PATCH 6/7] powerpc/pseries: Use pr_devel() in xics.c
From: Michael Ellerman @ 2009-06-18 4:13 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <1fdc1f0bb73b6ff2db7f9b394331d5ef5dfc4cb5.1245298429.git.michael@ellerman.id.au>
pr_debug() can now result in code being generated even when DEBUG
is not defined. That's not really desirable in some places.
With CONFIG_DYNAMIC_DEBUG=y:
size before:
text data bss dec hex filename
7720 5488 296 13504 34c0 platforms/pseries/xics.o
size after:
text data bss dec hex filename
7535 5456 296 13287 33e7 platforms/pseries/xics.o
Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---
arch/powerpc/platforms/pseries/xics.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/xics.c b/arch/powerpc/platforms/pseries/xics.c
index be3581a..419f8a6 100644
--- a/arch/powerpc/platforms/pseries/xics.c
+++ b/arch/powerpc/platforms/pseries/xics.c
@@ -190,10 +190,10 @@ static void xics_unmask_irq(unsigned int virq)
int call_status;
int server;
- pr_debug("xics: unmask virq %d\n", virq);
+ pr_devel("xics: unmask virq %d\n", virq);
irq = (unsigned int)irq_map[virq].hwirq;
- pr_debug(" -> map to hwirq 0x%x\n", irq);
+ pr_devel(" -> map to hwirq 0x%x\n", irq);
if (irq == XICS_IPI || irq == XICS_IRQ_SPURIOUS)
return;
@@ -252,7 +252,7 @@ static void xics_mask_irq(unsigned int virq)
{
unsigned int irq;
- pr_debug("xics: mask virq %d\n", virq);
+ pr_devel("xics: mask virq %d\n", virq);
irq = (unsigned int)irq_map[virq].hwirq;
if (irq == XICS_IPI || irq == XICS_IRQ_SPURIOUS)
@@ -414,7 +414,7 @@ static int xics_host_match(struct irq_host *h, struct device_node *node)
static int xics_host_map(struct irq_host *h, unsigned int virq,
irq_hw_number_t hw)
{
- pr_debug("xics: map virq %d, hwirq 0x%lx\n", virq, hw);
+ pr_devel("xics: map virq %d, hwirq 0x%lx\n", virq, hw);
/* Insert the interrupt mapping into the radix tree for fast lookup */
irq_radix_revmap_insert(xics_host, virq, hw);
--
1.6.2.1
^ permalink raw reply related
* [PATCH 7/7] powerpc: Use pr_devel() in arch/powerpc/mm/mmu_context_nohash.c
From: Michael Ellerman @ 2009-06-18 4:13 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <1fdc1f0bb73b6ff2db7f9b394331d5ef5dfc4cb5.1245298429.git.michael@ellerman.id.au>
pr_debug() can now result in code being generated even when DEBUG
is not defined. That's not really desirable in some places.
With CONFIG_DYNAMIC_DEBUG=y:
size before:
text data bss dec hex filename
1508 48 28 1584 630 powerpc/mm/mmu_context_nohash.o
size after:
text data bss dec hex filename
1088 0 28 1116 45c powerpc/mm/mmu_context_nohash.o
Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---
arch/powerpc/mm/mmu_context_nohash.c | 16 ++++++++--------
1 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/mm/mmu_context_nohash.c b/arch/powerpc/mm/mmu_context_nohash.c
index 8343986..92a1971 100644
--- a/arch/powerpc/mm/mmu_context_nohash.c
+++ b/arch/powerpc/mm/mmu_context_nohash.c
@@ -89,7 +89,7 @@ static unsigned int steal_context_smp(unsigned int id)
id = first_context;
continue;
}
- pr_debug("[%d] steal context %d from mm @%p\n",
+ pr_devel("[%d] steal context %d from mm @%p\n",
smp_processor_id(), id, mm);
/* Mark this mm has having no context anymore */
@@ -126,7 +126,7 @@ static unsigned int steal_context_up(unsigned int id)
/* Pick up the victim mm */
mm = context_mm[id];
- pr_debug("[%d] steal context %d from mm @%p\n", cpu, id, mm);
+ pr_devel("[%d] steal context %d from mm @%p\n", cpu, id, mm);
/* Flush the TLB for that context */
local_flush_tlb_mm(mm);
@@ -180,7 +180,7 @@ void switch_mmu_context(struct mm_struct *prev, struct mm_struct *next)
spin_lock(&context_lock);
#ifndef DEBUG_STEAL_ONLY
- pr_debug("[%d] activating context for mm @%p, active=%d, id=%d\n",
+ pr_devel("[%d] activating context for mm @%p, active=%d, id=%d\n",
cpu, next, next->context.active, next->context.id);
#endif
@@ -189,7 +189,7 @@ void switch_mmu_context(struct mm_struct *prev, struct mm_struct *next)
next->context.active++;
if (prev) {
#ifndef DEBUG_STEAL_ONLY
- pr_debug(" old context %p active was: %d\n",
+ pr_devel(" old context %p active was: %d\n",
prev, prev->context.active);
#endif
WARN_ON(prev->context.active < 1);
@@ -236,7 +236,7 @@ void switch_mmu_context(struct mm_struct *prev, struct mm_struct *next)
next->context.id = id;
#ifndef DEBUG_STEAL_ONLY
- pr_debug("[%d] picked up new id %d, nrf is now %d\n",
+ pr_devel("[%d] picked up new id %d, nrf is now %d\n",
cpu, id, nr_free_contexts);
#endif
@@ -247,7 +247,7 @@ void switch_mmu_context(struct mm_struct *prev, struct mm_struct *next)
* local TLB for it and unmark it before we use it
*/
if (test_bit(id, stale_map[cpu])) {
- pr_debug("[%d] flushing stale context %d for mm @%p !\n",
+ pr_devel("[%d] flushing stale context %d for mm @%p !\n",
cpu, id, next);
local_flush_tlb_mm(next);
@@ -314,13 +314,13 @@ static int __cpuinit mmu_context_cpu_notify(struct notifier_block *self,
switch (action) {
case CPU_ONLINE:
case CPU_ONLINE_FROZEN:
- pr_debug("MMU: Allocating stale context map for CPU %d\n", cpu);
+ pr_devel("MMU: Allocating stale context map for CPU %d\n", cpu);
stale_map[cpu] = kzalloc(CTX_MAP_SIZE, GFP_KERNEL);
break;
#ifdef CONFIG_HOTPLUG_CPU
case CPU_DEAD:
case CPU_DEAD_FROZEN:
- pr_debug("MMU: Freeing stale context map for CPU %d\n", cpu);
+ pr_devel("MMU: Freeing stale context map for CPU %d\n", cpu);
kfree(stale_map[cpu]);
stale_map[cpu] = NULL;
break;
--
1.6.2.1
^ permalink raw reply related
* [PATCH -mm][POWERPC] mpc8xxx : allow SPI without cs.
From: Rini van Zetten @ 2009-06-18 6:19 UTC (permalink / raw)
To: Kumar Gala, spi-devel-general, linuxppc-dev list
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 */
Signed-off-by: Rini van Zetten <rini@arvoo.nl>
---
Changes :
patch against 2.6.30-rc8-mm1
--- 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);
+ }
}
static int of_mpc8xxx_spi_get_chipselects(struct device *dev)
@@ -707,27 +708,29 @@ static int of_mpc8xxx_spi_get_chipselect
enum of_gpio_flags flags;
gpio = of_get_gpio_flags(np, i, &flags);
- if (!gpio_is_valid(gpio)) {
+ if (gpio_is_valid(gpio)) {
+ ret = gpio_request(gpio, dev_name(dev));
+ if (ret) {
+ dev_err(dev, "can't request gpio #%d: %d\n", i, ret);
+ goto err_loop;
+ }
+
+ pinfo->gpios[i] = gpio;
+ pinfo->alow_flags[i] = flags & OF_GPIO_ACTIVE_LOW;
+
+ ret = gpio_direction_output(pinfo->gpios[i],
+ pinfo->alow_flags[i]);
+ if (ret) {
+ dev_err(dev, "can't set output direction for gpio "
+ "#%d: %d\n", i, ret);
+ goto err_loop;
+ }
+ } else if (gpio == -EEXIST) {
+ pinfo->gpios[i] = -EEXIST;
+ } else {
dev_err(dev, "invalid gpio #%d: %d\n", i, gpio);
goto err_loop;
}
-
- ret = gpio_request(gpio, dev_name(dev));
- if (ret) {
- dev_err(dev, "can't request gpio #%d: %d\n", i, ret);
- goto err_loop;
- }
-
- pinfo->gpios[i] = gpio;
- pinfo->alow_flags[i] = flags & OF_GPIO_ACTIVE_LOW;
-
- ret = gpio_direction_output(pinfo->gpios[i],
- pinfo->alow_flags[i]);
- if (ret) {
- dev_err(dev, "can't set output direction for gpio "
- "#%d: %d\n", i, ret);
- goto err_loop;
- }
}
pdata->max_chipselect = ngpios;
--
--
Rini van Zetten
Senior Software Engineer
-------------------------
ARVOO Engineering B.V.
Tasveld 13
3417 XS Montfoort
The Netherlands
E-mail : <mailto:rini@arvoo.com> Rini van Zetten
Web : www.arvoo.com
^ permalink raw reply
* Re: [spi-devel-general] [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
From: Wolfram Sang @ 2009-06-18 6:58 UTC (permalink / raw)
To: Grant Likely
Cc: linuxppc-dev, David Brownell, linux-kernel, spi-devel-general
In-Reply-To: <20090618025030.12363.69402.stgit@localhost.localdomain>
[-- Attachment #1: Type: text/plain, Size: 20824 bytes --]
Hi Grant,
some comments below:
(by the way, have you tested this patch on hardware? I wonder because of the
SSOE-issue, but maybe it works despite the documentation.)
On Wed, Jun 17, 2009 at 08:55:01PM -0600, Grant Likely wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
>
> Adds support for the dedicated SPI device on the Freescale MPC5200(b)
> SoC.
>
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> ---
>
> Hi David,
>
> It's been a while since I've posted this, but I believe I've addressed
> all the outstanding comments against v3, and people are asking me for it.
> The pre-message stuff is all gone and the error handling should be
> better now.
>
> BTW, the premessage stuff was to handle a platform I had where some of
> the CS lines were controlled with a separate SPI transaction to the
> same SPI controller. It almost looks like an SPI bridge. It requires
> more thought before I try again.
>
> Being a new driver, it would be nice to get it into 2.6.31, but
> definitely not critical.
>
> g.
>
> drivers/spi/Kconfig | 8 +
> drivers/spi/Makefile | 1
> drivers/spi/mpc52xx_spi.c | 520 +++++++++++++++++++++++++++++++++++++++
> include/linux/spi/mpc52xx_spi.h | 10 +
> 4 files changed, 539 insertions(+), 0 deletions(-)
> create mode 100644 drivers/spi/mpc52xx_spi.c
> create mode 100644 include/linux/spi/mpc52xx_spi.h
>
>
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 83a185d..1994bcd 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -132,6 +132,14 @@ config SPI_LM70_LLP
> which interfaces to an LM70 temperature sensor using
> a parallel port.
>
> +config SPI_MPC52xx
> + tristate "Freescale MPC52xx SPI (non-PSC) controller support"
> + depends on PPC_MPC52xx && SPI
> + select SPI_MASTER_OF
> + help
> + This drivers supports the MPC52xx SPI controller in master SPI
> + mode.
> +
> config SPI_MPC52xx_PSC
> tristate "Freescale MPC52xx PSC SPI controller"
> depends on PPC_MPC52xx && EXPERIMENTAL
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 5d04519..8de32c7 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_SPI_OMAP_UWIRE) += omap_uwire.o
> obj-$(CONFIG_SPI_OMAP24XX) += omap2_mcspi.o
> obj-$(CONFIG_SPI_ORION) += orion_spi.o
> obj-$(CONFIG_SPI_MPC52xx_PSC) += mpc52xx_psc_spi.o
> +obj-$(CONFIG_SPI_MPC52xx) += mpc52xx_spi.o
> obj-$(CONFIG_SPI_MPC83xx) += spi_mpc83xx.o
> obj-$(CONFIG_SPI_S3C24XX_GPIO) += spi_s3c24xx_gpio.o
> obj-$(CONFIG_SPI_S3C24XX) += spi_s3c24xx.o
> diff --git a/drivers/spi/mpc52xx_spi.c b/drivers/spi/mpc52xx_spi.c
> new file mode 100644
> index 0000000..ef8379b
> --- /dev/null
> +++ b/drivers/spi/mpc52xx_spi.c
> @@ -0,0 +1,520 @@
> +/*
> + * MPC52xx SPI bus driver.
> + *
> + * Copyright (C) 2008 Secret Lab Technologies Ltd.
> + *
> + * This file is released under the GPLv2
> + *
> + * This is the driver for the MPC5200's dedicated SPI controller.
> + *
> + * Note: this driver does not support the MPC5200 PSC in SPI mode. For
> + * that driver see drivers/spi/mpc52xx_psc_spi.c
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/errno.h>
> +#include <linux/of_platform.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/mpc52xx_spi.h>
Is this still needed? See last comment...
> +#include <linux/of_spi.h>
> +#include <linux/io.h>
> +#include <asm/time.h>
Really asm?
> +#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 0x00
> +#define SPI_CTRL1_SPIE (1 << 7)
> +#define SPI_CTRL1_SPE (1 << 6)
> +#define SPI_CTRL1_MSTR (1 << 4)
> +#define SPI_CTRL1_CPOL (1 << 3)
> +#define SPI_CTRL1_CPHA (1 << 2)
> +#define SPI_CTRL1_SSOE (1 << 1)
> +#define SPI_CTRL1_LSBFE (1 << 0)
> +
> +#define SPI_CTRL2 0x01
> +#define SPI_BRR 0x04
> +
> +#define SPI_STATUS 0x05
> +#define SPI_STATUS_SPIF (1 << 7)
> +#define SPI_STATUS_WCOL (1 << 6)
> +#define SPI_STATUS_MODF (1 << 4)
> +
> +#define SPI_DATA 0x09
> +#define SPI_PORTDATA 0x0d
> +#define SPI_DATADIR 0x10
> +
> +/* FSM state return values */
> +#define FSM_STOP 0 /* Nothing more for the state machine to */
> + /* do. If something interesting happens */
> + /* then and IRQ will be received */
s/and/an/? Throughout the comments, there is sometimes a double space.
> +#define FSM_POLL 1 /* need to poll for completion, an IRQ is */
> + /* not expected */
> +#define FSM_CONTINUE 2 /* Keep iterating the state machine */
> +
> +/* Driver internal data */
> +struct mpc52xx_spi {
> + struct spi_master *master;
> + u32 sysclk;
unused?
> + void __iomem *regs;
> + int irq0; /* MODF irq */
> + int irq1; /* SPIF irq */
> + int ipb_freq;
unsigned int will suit it better IMHO.
> +
> + /* 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 :)
> +
> + struct list_head queue; /* queue of pending messages */
> + spinlock_t lock;
> + struct work_struct work;
> +
> +
> + /* Details of current transfer (length, and buffer pointers) */
> + struct spi_message *message; /* current message */
> + struct spi_transfer *transfer; /* current transfer */
> + int (*state)(int irq, struct mpc52xx_spi *ms, u8 status, u8 data);
> + int len;
> + int timestamp;
> + u8 *rx_buf;
> + const u8 *tx_buf;
> + int cs_change;
> +};
> +
> +/*
> + * CS control function
> + */
> +static void mpc52xx_spi_chipsel(struct mpc52xx_spi *ms, int value)
> +{
> + out_8(ms->regs + SPI_PORTDATA, value ? 0 : 0x08);
Magic value.
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.
> +}
> +
> +/*
> + * Start a new transfer. This is called both by the idle state
> + * for the first transfer in a message, and by the wait state when the
> + * previous transfer in a message is complete.
> + */
> +static void mpc52xx_spi_start_transfer(struct mpc52xx_spi *ms)
> +{
> + ms->rx_buf = ms->transfer->rx_buf;
> + ms->tx_buf = ms->transfer->tx_buf;
> + ms->len = ms->transfer->len;
> +
> + /* Activate the chip select */
> + if (ms->cs_change)
> + mpc52xx_spi_chipsel(ms, 1);
> + ms->cs_change = ms->transfer->cs_change;
> +
> + /* Write out the first byte */
> + ms->wcol_tx_timestamp = get_tbl();
> + if (ms->tx_buf)
> + out_8(ms->regs + SPI_DATA, *ms->tx_buf++);
> + else
> + out_8(ms->regs + SPI_DATA, 0);
> +}
> +
> +/* Forward declaration of state handlers */
> +static int mpc52xx_spi_fsmstate_transfer(int irq, struct mpc52xx_spi *ms,
> + u8 status, u8 data);
> +static int mpc52xx_spi_fsmstate_wait(int irq, struct mpc52xx_spi *ms,
> + u8 status, u8 data);
> +
> +/*
> + * IDLE state
> + *
> + * No transfers are in progress; if another transfer is pending then retrieve
> + * it and kick it off. Otherwise, stop processing the state machine
> + */
> +static int
> +mpc52xx_spi_fsmstate_idle(int irq, struct mpc52xx_spi *ms, u8 status, u8 data)
> +{
> + struct spi_device *spi;
> + int spr, sppr;
> + u8 ctrl1;
> +
> + if (status && (irq != NO_IRQ))
> + dev_err(&ms->master->dev, "spurious irq, status=0x%.2x\n",
> + status);
> +
> + /* Check if there is another transfer waiting. */
> + if (list_empty(&ms->queue))
> + return FSM_STOP;
> +
> + /* get the head of the queue */
> + ms->message = list_first_entry(&ms->queue, struct spi_message, queue);
> + list_del_init(&ms->message->queue);
> +
> + /* Setup the controller parameters */
> + ctrl1 = SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR;
> + spi = ms->message->spi;
> + if (spi->mode & SPI_CPHA)
> + ctrl1 |= SPI_CTRL1_CPHA;
> + if (spi->mode & SPI_CPOL)
> + ctrl1 |= SPI_CTRL1_CPOL;
> + if (spi->mode & SPI_LSB_FIRST)
> + ctrl1 |= SPI_CTRL1_LSBFE;
> + out_8(ms->regs + SPI_CTRL1, ctrl1);
> +
> + /* Setup the controller speed */
> + /* minimum divider is '2'. Also, add '1' to force rounding the
> + * divider up. */
> + sppr = ((ms->ipb_freq / ms->message->spi->max_speed_hz) + 1) >> 1;
> + spr = 0;
> + if (sppr < 1)
> + sppr = 1;
> + while (((sppr - 1) & ~0x7) != 0) {
> + sppr = (sppr + 1) >> 1; /* add '1' to force rounding up */
> + spr++;
> + }
> + sppr--; /* sppr quantity in register is offset by 1 */
> + if (spr > 7) {
> + /* Don't overrun limits of SPI baudrate register */
> + spr = 7;
> + sppr = 7;
> + }
> + out_8(ms->regs + SPI_BRR, sppr << 4 | spr); /* Set speed */
> +
> + ms->cs_change = 1;
> + ms->transfer = container_of(ms->message->transfers.next,
> + struct spi_transfer, transfer_list);
> +
> + mpc52xx_spi_start_transfer(ms);
> + ms->state = mpc52xx_spi_fsmstate_transfer;
> +
> + return FSM_CONTINUE;
> +}
> +
> +/*
> + * TRANSFER state
> + *
> + * In the middle of a transfer. If the SPI core has completed processing
> + * a byte, then read out the received data and write out the next byte
> + * (unless this transfer is finished; in which case go on to the wait
> + * state)
> + */
> +static int mpc52xx_spi_fsmstate_transfer(int irq, struct mpc52xx_spi *ms,
> + u8 status, u8 data)
> +{
> + if (!status)
> + return ms->irq0 ? FSM_STOP : FSM_POLL;
> +
> + if (status & SPI_STATUS_WCOL) {
> + /* The SPI controller is stoopid. At slower speeds, it may
> + * raise the SPIF flag before the state machine is actually
> + * finished, which causes a collision (internal to the state
> + * machine only). The manual recommends inserting a delay
> + * between receiving the interrupt and sending the next byte,
> + * but it can also be worked around simply by retrying the
> + * transfer which is what we do here. */
> + ms->wcol_count++;
> + ms->wcol_ticks += get_tbl() - ms->wcol_tx_timestamp;
> + ms->wcol_tx_timestamp = get_tbl();
> + data = 0;
> + if (ms->tx_buf)
> + data = *(ms->tx_buf-1);
spaces around operator.
> + out_8(ms->regs + SPI_DATA, data); /* try again */
> + return FSM_CONTINUE;
> + } else if (status & SPI_STATUS_MODF) {
> + ms->modf_count++;
> + dev_err(&ms->master->dev, "mode fault\n");
> + mpc52xx_spi_chipsel(ms, 0);
> + ms->message->status = -EIO;
> + ms->message->complete(ms->message->context);
> + ms->state = mpc52xx_spi_fsmstate_idle;
> + return FSM_CONTINUE;
> + }
> +
> + /* Read data out of the spi device */
> + ms->byte_count++;
> + if (ms->rx_buf)
> + *ms->rx_buf++ = data;
> +
> + /* Is the transfer complete? */
> + ms->len--;
> + if (ms->len == 0) {
> + ms->timestamp = get_tbl();
> + ms->timestamp += ms->transfer->delay_usecs * tb_ticks_per_usec;
> + ms->state = mpc52xx_spi_fsmstate_wait;
> + return FSM_CONTINUE;
> + }
> +
> + /* Write out the next byte */
> + ms->wcol_tx_timestamp = get_tbl();
> + if (ms->tx_buf)
> + out_8(ms->regs + SPI_DATA, *ms->tx_buf++);
> + else
> + out_8(ms->regs + SPI_DATA, 0);
> +
> + return FSM_CONTINUE;
> +}
> +
> +/*
> + * WAIT state
> + *
> + * A transfer has completed; need to wait for the delay period to complete
> + * before starting the next transfer
> + */
> +static int
> +mpc52xx_spi_fsmstate_wait(int irq, struct mpc52xx_spi *ms, u8 status, u8 data)
> +{
> + if (status && irq)
> + dev_err(&ms->master->dev, "spurious irq, status=0x%.2x\n",
> + status);
> +
> + if (((int)get_tbl()) - ms->timestamp < 0)
> + return FSM_POLL;
> +
> + ms->message->actual_length += ms->transfer->len;
> +
> + /* Check if there is another transfer in this message. If there
> + * aren't then deactivate CS, notify sender, and drop back to idle
> + * to start the next message. */
> + if (ms->transfer->transfer_list.next == &ms->message->transfers) {
> + ms->msg_count++;
> + mpc52xx_spi_chipsel(ms, 0);
> + ms->message->status = 0;
> + ms->message->complete(ms->message->context);
> + ms->state = mpc52xx_spi_fsmstate_idle;
> + return FSM_CONTINUE;
> + }
> +
> + /* There is another transfer; kick it off */
> +
> + if (ms->cs_change)
> + mpc52xx_spi_chipsel(ms, 0);
> +
> + ms->transfer = container_of(ms->transfer->transfer_list.next,
> + struct spi_transfer, transfer_list);
> + mpc52xx_spi_start_transfer(ms);
> + ms->state = mpc52xx_spi_fsmstate_transfer;
> + return FSM_CONTINUE;
> +}
> +
> +/**
> + * mpc52xx_spi_fsm_process - Finite State Machine iteration function
> + * @irq: irq number that triggered the FSM or 0 for polling
> + * @ms: pointer to mpc52xx_spi driver data
> + */
> +static void mpc52xx_spi_fsm_process(int irq, struct mpc52xx_spi *ms)
> +{
> + int rc = FSM_CONTINUE;
> + u8 status, data;
> +
> + while (rc == FSM_CONTINUE) {
> + /* Interrupt cleared by read of STATUS followed by
> + * read of DATA registers */
> + status = in_8(ms->regs + SPI_STATUS);
> + data = in_8(ms->regs + SPI_DATA);
> + rc = ms->state(irq, ms, status, data);
> + }
> +
> + if (rc == FSM_POLL)
> + schedule_work(&ms->work);
> +}
> +
> +/**
> + * mpc52xx_spi_irq - IRQ handler
> + */
> +static irqreturn_t mpc52xx_spi_irq(int irq, void *_ms)
> +{
> + struct mpc52xx_spi *ms = _ms;
> + spin_lock(&ms->lock);
> + mpc52xx_spi_fsm_process(irq, ms);
> + spin_unlock(&ms->lock);
> + return IRQ_HANDLED;
> +}
> +
> +/**
> + * mpc52xx_spi_wq - Workqueue function for polling the state machine
> + */
> +static void mpc52xx_spi_wq(struct work_struct *work)
> +{
> + struct mpc52xx_spi *ms = container_of(work, struct mpc52xx_spi, work);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&ms->lock, flags);
> + mpc52xx_spi_fsm_process(0, ms);
> + spin_unlock_irqrestore(&ms->lock, flags);
> +}
> +
> +/*
> + * spi_master ops
> + */
> +
> +static int mpc52xx_spi_setup(struct spi_device *spi)
> +{
> + if (spi->bits_per_word % 8)
> + return -EINVAL;
> +
> + if (spi->mode & ~(SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST))
> + return -EINVAL;
> +
> + if (spi->chip_select >= spi->master->num_chipselect)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int mpc52xx_spi_transfer(struct spi_device *spi, struct spi_message *m)
> +{
> + struct mpc52xx_spi *ms = spi_master_get_devdata(spi->master);
> + unsigned long flags;
> +
> + m->actual_length = 0;
> + m->status = -EINPROGRESS;
> +
> + spin_lock_irqsave(&ms->lock, flags);
> + list_add_tail(&m->queue, &ms->queue);
> + spin_unlock_irqrestore(&ms->lock, flags);
> + schedule_work(&ms->work);
> +
> + return 0;
> +}
> +
> +/*
> + * OF Platform Bus Binding
> + */
> +static int __devinit mpc52xx_spi_probe(struct of_device *op,
> + const struct of_device_id *match)
> +{
> + struct spi_master *master;
> + struct mpc52xx_spi *ms;
> + void __iomem *regs;
> + int rc;
> +
> + /* MMIO registers */
> + dev_dbg(&op->dev, "probing mpc5200 SPI device\n");
> + regs = of_iomap(op->node, 0);
> + if (!regs)
> + return -ENODEV;
> +
> + /* initialize the device */
> + out_8(regs+SPI_CTRL1, SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR);
spaces around operator.
> + out_8(regs + SPI_CTRL2, 0x0);
> + out_8(regs + SPI_DATADIR, 0xe); /* Set output pins */
> + out_8(regs + SPI_PORTDATA, 0x8); /* Deassert /SS signal */
> +
> + /* Clear the status register and re-read it to check for a MODF
> + * failure. This driver cannot currently handle multiple masters
> + * on the SPI bus. This fault will also occur if the SPI signals
> + * are not connected to any pins (port_config setting) */
> + in_8(regs + SPI_STATUS);
> + in_8(regs + SPI_DATA);
> + if (in_8(regs + SPI_STATUS) & SPI_STATUS_MODF) {
> + dev_err(&op->dev, "mode fault; is port_config correct?\n");
> + rc = -EIO;
> + goto err_init;
> + }
> +
> + dev_dbg(&op->dev, "allocating spi_master struct\n");
> + master = spi_alloc_master(&op->dev, sizeof *ms);
> + if (!master) {
> + rc = -ENOMEM;
> + goto err_alloc;
> + }
> + master->bus_num = -1;
> + master->num_chipselect = 1;
> + master->setup = mpc52xx_spi_setup;
> + master->transfer = mpc52xx_spi_transfer;
> + dev_set_drvdata(&op->dev, master);
> +
> + ms = spi_master_get_devdata(master);
> + ms->master = master;
> + ms->regs = regs;
> + ms->irq0 = irq_of_parse_and_map(op->node, 0);
> + ms->irq1 = irq_of_parse_and_map(op->node, 1);
> + ms->state = mpc52xx_spi_fsmstate_idle;
> + ms->ipb_freq = mpc5xxx_get_bus_frequency(op->node);
> + spin_lock_init(&ms->lock);
> + INIT_LIST_HEAD(&ms->queue);
> + INIT_WORK(&ms->work, mpc52xx_spi_wq);
> +
> + /* Decide if interrupts can be used */
> + if (ms->irq0 && ms->irq1) {
> + rc = request_irq(ms->irq0, mpc52xx_spi_irq, IRQF_SAMPLE_RANDOM,
> + "mpc5200-spi-modf", ms);
> + rc |= request_irq(ms->irq1, mpc52xx_spi_irq, IRQF_SAMPLE_RANDOM,
> + "mpc5200-spi-spiF", ms);
Capital 'F' wanted?
> + if (rc) {
> + free_irq(ms->irq0, ms);
> + free_irq(ms->irq1, ms);
> + ms->irq0 = ms->irq1 = 0;
> + }
> + } else {
> + /* operate in polled mode */
> + ms->irq0 = ms->irq1 = 0;
> + }
> +
> + if (!ms->irq0)
> + dev_info(&op->dev, "using polled mode\n");
> +
> + dev_dbg(&op->dev, "registering spi_master struct\n");
> + rc = spi_register_master(master);
> + if (rc)
> + goto err_register;
> +
> + of_register_spi_devices(master, op->node);
> + dev_info(&ms->master->dev, "registered MPC5200 SPI bus\n");
> +
> + return rc;
> +
> + err_register:
> + dev_err(&ms->master->dev, "initialization failed\n");
> + spi_master_put(master);
> + err_alloc:
> + err_init:
> + iounmap(regs);
> + return rc;
> +}
> +
> +static int __devexit mpc52xx_spi_remove(struct of_device *op)
> +{
> + struct spi_master *master = dev_get_drvdata(&op->dev);
> + struct mpc52xx_spi *ms = spi_master_get_devdata(master);
> +
> + free_irq(ms->irq0, ms);
> + free_irq(ms->irq1, ms);
> +
> + spi_unregister_master(master);
> + spi_master_put(master);
> + iounmap(ms->regs);
> +
> + return 0;
> +}
> +
> +static struct of_device_id mpc52xx_spi_match[] __devinitdata = {
> + { .compatible = "fsl,mpc5200-spi", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, mpc52xx_spi_match);
> +
> +static struct of_platform_driver mpc52xx_spi_of_driver = {
> + .owner = THIS_MODULE,
> + .name = "mpc52xx-spi",
> + .match_table = mpc52xx_spi_match,
> + .probe = mpc52xx_spi_probe,
> + .remove = __exit_p(mpc52xx_spi_remove),
> +};
> +
> +static int __init mpc52xx_spi_init(void)
> +{
> + return of_register_platform_driver(&mpc52xx_spi_of_driver);
> +}
> +module_init(mpc52xx_spi_init);
> +
> +static void __exit mpc52xx_spi_exit(void)
> +{
> + of_unregister_platform_driver(&mpc52xx_spi_of_driver);
> +}
> +module_exit(mpc52xx_spi_exit);
> +
> 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,
> + void (*hook)(struct spi_message *m,
> + void *context),
> + void *hook_context);
> +
> +#endif
This can be dropped for now and reintroduced when needed, I think.
>
>
> ------------------------------------------------------------------------------
> Crystal Reports - New Free Runtime and 30 Day Trial
> Check out the new simplified licensing option that enables unlimited
> royalty-free distribution of the report engine for externally facing
> server and web deployment.
> http://p.sf.net/sfu/businessobjects
> _______________________________________________
> spi-devel-general mailing list
> spi-devel-general@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/spi-devel-general
--
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 1/6] perf_counter: powerpc: Enable use of software counters on 32-bit powerpc
From: Ingo Molnar @ 2009-06-18 9:04 UTC (permalink / raw)
To: Paul Mackerras; +Cc: linuxppc-dev, Peter Zijlstra, linux-kernel
In-Reply-To: <19001.31734.428534.890844@cargo.ozlabs.ibm.com>
* Paul Mackerras <paulus@samba.org> wrote:
> Ingo Molnar writes:
>
> > 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?
>
> Kumar's comments seemed to me to be wanting changes to accommodate
> code that doesn't exist yet, so I think those changes should be
> done later when that code exists and we know exactly what is
> needed. So the current patches are fine as-is IMO.
ok - will queue them up.
Ingo
^ permalink raw reply
* Re: [PATCH 6/6] perf_counter: tools: Makefile tweaks for 64-bit powerpc
From: Ingo Molnar @ 2009-06-18 9:13 UTC (permalink / raw)
To: Paul Mackerras; +Cc: linuxppc-dev, Peter Zijlstra, linux-kernel
In-Reply-To: <19000.55666.866148.559620@cargo.ozlabs.ibm.com>
* Paul Mackerras <paulus@samba.org> wrote:
> This also removes the -Werror flag when building on a 64-bit powerpc
> machine. The userspace definition of u64 is unsigned long rather
> than unsigned long long, meaning that gcc warns every time a u64
> is printed with %Lx or %llx (though that does work properly).
> In future we may use PRI64 etc. for printing 64-bit quantities,
> which would eliminate these warnings.
> +# Don't use -Werror on ppc64; we get warnings due to using
> +# %Lx formats on __u64, which is unsigned long.
> +Werror := -Werror
> +ifeq ($(uname_M),ppc64)
> + Werror :=
> +endif
Note, i left out this bit from the commit - we need to find a better
solution than to allow ugly warnings on PowerPC.
Could we use the kernel's u64 type directly perhaps? That would
allow us to change all __u64 to u64 in all of tools/perf/ which is a
nice clean-up in any case.
Ingo
^ permalink raw reply
* Re: [PATCH] powerpc: Add irqtrace support for 32-bit powerpc (v2)
From: Benjamin Herrenschmidt @ 2009-06-18 9:31 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <20090618034439.0854ADDDB2@ozlabs.org>
On Thu, 2009-06-18 at 13:43 +1000, Benjamin Herrenschmidt wrote:
> Index: linux-work/arch/powerpc/kernel/udbg.c
> ===================================================================
> --- linux-work.orig/arch/powerpc/kernel/udbg.c 2009-06-18
> 13:21:29.000000000 +1000
> +++ linux-work/arch/powerpc/kernel/udbg.c 2009-06-18
> 13:21:39.000000000 +1000
> @@ -65,6 +65,7 @@ void __init udbg_early_init(void)
> #ifdef CONFIG_PPC_EARLY_DEBUG
> console_loglevel = 10;
> #endif
> + register_early_udbg_console();
> }
And that bit of course have nothing to do in that patch :-)
No need for a respin for now though unless we find other problems.
Cheers,
Ben.
^ permalink raw reply
* Re: PowerPC PCI DMA issues (prefetch/coherency?)
From: Chris Pringle @ 2009-06-18 11:24 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev@ozlabs.org list, linux-kernel
In-Reply-To: <4A38ED31.9030705@oxtel.com>
Chris Pringle wrote:
> Chris Pringle wrote:
>>> You could enable CONFIG_NOT_COHERENT_CACHE.
>>>
>> I've just tried this (I had to edit Kconfig in power/platforms to
>> make the build system accept it), and interestingly it's making no
>> difference. I'm using streaming mappings, and are using the
>> pci_map_sg functions to ensure the memory is mapped/flushed
>> correctly. I've also explicitly put in a pci_dma_sync_sg_for_device,
>> however that's also not made any difference. Turning the cpu cache
>> snoop off has the same affect as it did without
>> CONFIG_NOT_COHERENT_CACHE; it gets much worse. Any other ideas?
>>
>> Will back off the low latency patches next, and give 2.6.30 a try -
>> see if that makes any difference.
>>
> Low latency patches made no difference. Tried it with 2.6.30 and it
> now works. There are a couple of commits contributing to the fix,
> including one introduced between 2.6.29-rc8 and 2.6.29 proper in
> powerpc/kernel/head_32.S (couple of commits with the name "Fix Respect
> _PAGE_COHERENT on classic ppc32 SW TLB load machines"). I've tried
> backporting this to 2.6.29-rc8 and it then worked. Backporting to
> 2.6.26 made no difference however, so I suspect there are other things
> fixed which are also contributing.
>
> I'm going to move to 2.6.29/2.6.30 which should resolve our issue.
>
> Thanks to all who have contributed to this thread.
>
The other part of the fix is in asm-powerpc/pgtable32.h. _PAGE_BASE
needs _PAGE_COHERENT in order to work correctly, and in fact there is
now a comment in there to that affect in 2.6.29. Backporting that change
has made it work on 2.6.26. Both this patch, and the fix to head_32.S
are needed for it to work correctly on older kernels.
Chris
--
______________________________
Chris Pringle
Software Engineer
Miranda Technologies Ltd.
Hithercroft Road
Wallingford
Oxfordshire OX10 9DG
UK
Tel. +44 1491 820206
Fax. +44 1491 820001
www.miranda.com
____________________________
Miranda Technologies Limited
Registered in England and Wales CN 02017053
Registered Office: James House, Mere Park, Dedmere Road, Marlow, Bucks, SL7 1FJ
^ permalink raw reply
* [PowerPC] Badness at drivers/char/tty_ldisc.c:210 during shutdown
From: Sachin Sant @ 2009-06-18 12:03 UTC (permalink / raw)
To: linux-kernel; +Cc: linuxppc-dev, alan
I came across the following badness message during shutdown on a Power6 box.
This was with 2.6.30-git12(3fe0344faf7fdcb158bd5c1a9aec960a8d70c8e8)
------------[ cut here ]------------
Badness at drivers/char/tty_ldisc.c:210
NIP: c000000000409428 LR: c000000000409410 CTR: 0000000000000000
REGS: c0000000374f37f0 TRAP: 0700 Not tainted (2.6.30-git12)
MSR: 8000000000029032 <EE,ME,CE,IR,DR> CR: 24000484 XER: 00000001
TASK = c00000003d941ae0[8535] 'vhangup' THREAD: c0000000374f0000 CPU: 1
<6>GPR00: 0000000000000001 c0000000374f3a70 c000000000ef4550 0000000000000001
<6>GPR04: c000000000409410 c00000003e99f000 c000000000406ba0 0000000000000000
<6>GPR08: 0000000000000000 0000000000000000 0000000000000000 c0000000374f3a70
<6>GPR12: 0000000024000488 c000000001002600 00000000ffffffff ffffffffffffffff
<6>GPR16: 00000000320c8a50 0000000000000002 0000000000000000 00000000320b03b0
<6>GPR20: 0000000000000000 0000000000000000 c00000003e1d3d00 0000000000000001
<6>GPR24: 0000000000000000 0000000000000000 0000000000000001 c00000003d4a05e0
<6>GPR28: 0000000000000000 c0000000013ffd38 c000000000e7e610 c0000000374f3a70
NIP [c000000000409428] .tty_ldisc_put+0xbc/0xf4
LR [c000000000409410] .tty_ldisc_put+0xa4/0xf4
Call Trace:
[c0000000374f3a70] [c000000000409410] .tty_ldisc_put+0xa4/0xf4 (unreliable)
[c0000000374f3b10] [c000000000409808] .tty_ldisc_reinit+0x38/0x80
[c0000000374f3ba0] [c00000000040a218] .tty_ldisc_hangup+0x190/0x260
[c0000000374f3c40] [c0000000004000d0] .do_tty_hangup+0x188/0x4c0
[c0000000374f3d20] [c000000000400480] .tty_vhangup_self+0x34/0x54
[c0000000374f3db0] [c0000000001917c8] .sys_vhangup+0x38/0x58
[c0000000374f3e30] [c000000000008534] syscall_exit+0x0/0x40
Instruction dump:
912b0088 4bcd2201 60000000 e87e8008 7f44d378 481c0735 60000000 801b0008
7c09fe70 7d200278 7c004850 54000ffe <0b000000> 7f63db78 4bd7cf0d 60000000
Not sure if this is a new issue. I haven't really paid attention
to messages that are displayed during shutdown.
Line 210 corresponds to the following code.
static void tty_ldisc_put(struct tty_ldisc *ld)
{
......
spin_unlock_irqrestore(&tty_ldisc_lock, flags);
WARN_ON(ld->refcount); <<===
.......
Thanks
-Sachin
--
---------------------------------
Sachin Sant
IBM Linux Technology Center
India Systems and Technology Labs
Bangalore, India
---------------------------------
^ permalink raw reply
* Re: [PATCH -mm][POWERPC] mpc8xxx : allow SPI without cs.
From: Kumar Gala @ 2009-06-18 12:42 UTC (permalink / raw)
To: Anton Vorontsov; +Cc: spi-devel-general, Rini van Zetten, linuxppc-dev list
In-Reply-To: <4A39DC80.7030906@arvoo.nl>
On Jun 18, 2009, at 1:19 AM, 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 */
>
>
>
> Signed-off-by: Rini van Zetten <rini@arvoo.nl>
> ---
> Changes :
> patch against 2.6.30-rc8-mm1
Anton,
Can you review and ack this if you are ok with it.
- k
>
>
> --- 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);
> + }
> }
>
> static int of_mpc8xxx_spi_get_chipselects(struct device *dev)
> @@ -707,27 +708,29 @@ static int of_mpc8xxx_spi_get_chipselect
> enum of_gpio_flags flags;
>
> gpio = of_get_gpio_flags(np, i, &flags);
> - if (!gpio_is_valid(gpio)) {
> + if (gpio_is_valid(gpio)) {
> + ret = gpio_request(gpio, dev_name(dev));
> + if (ret) {
> + dev_err(dev, "can't request gpio #%d: %d\n", i, ret);
> + goto err_loop;
> + }
> +
> + pinfo->gpios[i] = gpio;
> + pinfo->alow_flags[i] = flags & OF_GPIO_ACTIVE_LOW;
> +
> + ret = gpio_direction_output(pinfo->gpios[i],
> + pinfo->alow_flags[i]);
> + if (ret) {
> + dev_err(dev, "can't set output direction for gpio "
> + "#%d: %d\n", i, ret);
> + goto err_loop;
> + }
> + } else if (gpio == -EEXIST) {
> + pinfo->gpios[i] = -EEXIST;
> + } else {
> dev_err(dev, "invalid gpio #%d: %d\n", i, gpio);
> goto err_loop;
> }
> -
> - ret = gpio_request(gpio, dev_name(dev));
> - if (ret) {
> - dev_err(dev, "can't request gpio #%d: %d\n", i, ret);
> - goto err_loop;
> - }
> -
> - pinfo->gpios[i] = gpio;
> - pinfo->alow_flags[i] = flags & OF_GPIO_ACTIVE_LOW;
> -
> - ret = gpio_direction_output(pinfo->gpios[i],
> - pinfo->alow_flags[i]);
> - if (ret) {
> - dev_err(dev, "can't set output direction for gpio "
> - "#%d: %d\n", i, ret);
> - goto err_loop;
> - }
> }
>
> pdata->max_chipselect = ngpios;
> --
>
>
> --
> Rini van Zetten
> Senior Software Engineer
>
> -------------------------
> ARVOO Engineering B.V.
> Tasveld 13
> 3417 XS Montfoort
> The Netherlands
>
> E-mail : <mailto:rini@arvoo.com> Rini van Zetten
>
> Web : www.arvoo.com
>
>
^ permalink raw reply
* Re: [PATCH 5/6] perf_counter: powerpc: Add processor back-end for MPC7450 family
From: Kumar Gala @ 2009-06-18 12:50 UTC (permalink / raw)
To: Paul Mackerras; +Cc: linuxppc-dev, Ingo Molnar, Peter Zijlstra, linux-kernel
In-Reply-To: <19001.31508.901222.288946@cargo.ozlabs.ibm.com>
On Jun 17, 2009, at 6:24 PM, Paul Mackerras wrote:
> Kumar Gala writes:
>
>> This should be something like:
>>
>> obj64-$(CONFIG_PPC_PERF_CTRS)-$(PPC_BOOK3S_64)
>>
>>> +obj32-$(CONFIG_PPC_PERF_CTRS) += mpc7450-pmu.o
>>
>> obj32-$(CONFIG_PPC_PERF_CTRS)-$(PPC_BOOK3S_32)
>>
>> Or use new Kconfig types as I suggested on patch 1/6
>
> Feel free to send a patch making those changes along with adding the
> code to support the Freescale embedded PMU. :)
>
> Paul.
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 ?
- k
^ 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
* 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: 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: [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: [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
* 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: [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
* 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 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: [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: [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
* 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
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