* Re: [PATCH/RFC 1/2] 5200: improve i2c bus error recovery
From: Grant Likely @ 2010-02-16 19:31 UTC (permalink / raw)
To: Albrecht Dreß
Cc: Linux PPC Development, Devicetree Discussions,
Ben Dooks (embedded platforms)
In-Reply-To: <1264191475.2224.1@antares>
Hi Albrecht,
Comments below.
On Fri, Jan 22, 2010 at 1:17 PM, Albrecht Dre=DF <albrecht.dress@arcor.de> =
wrote:
> Improve the recovery of the MPC5200B's I2C bus from errors like bus
> hangs.
>
> Signed-off-by: Albrecht Dre=DF <albrecht.dress@arcor.de>
>
> ---
>
> This patch introduces several improvements to the MPC5200B's I2C driver
> as to improve the recovery from error conditions I encountered when
> testing a custom board with several I2C devices attached (eeprom, io
> expander, rtc, sensors). =A0The error conditions included cases where the
> bus if logic of one slave apparently went south, blocking the bus
> completely.
>
> My fixes include:
> 1. make the bus timeout configurable in fsl_i2c_probe(); the default of
> =A0 one second is *way* too long for my use case;
> 2. if a timeout condition occurs in mpc_xfer(), mpc_i2c_fixup() the bus
> =A0 if *any* of the CF, BB and RXAK flags in the MSR is 1. =A0I actually
> =A0 saw different combinations with hangs, not only all three set;
> 3. improve the fixup procedure by calculating the timing needed from the
> =A0 real (configured) bus clock, calculated in mpc_i2c_setclock_52xx().
> =A0 Furthermore, I issue 9 instead of one cycle, as I experienced cases
> =A0 where the single one is not enough (found this tip in a forum). =A0As=
a
> =A0 side effect, the new scheme needs only 81us @375kHz bus clock instead
> =A0 of 150us. =A0I recorded waveforms for 18.4kHz, 85.9kHz and 375kHz, al=
l
> =A0 looking fine, which I can provide if anyone is interested.
These are three separate fixes. Ideally you should submit them in
separate patches to make it easy on poor old reviewers like me. And,
as Ben mentions, this descriptions should be above the '---' line so
it appears in the commit text.
>
> Open questions:
> - is the approach correct at all, in particular the interpretation of
> =A0the flags (#2)?
> - could this code also be used on non-5200 processors?
>
> --- linux-2.6.32-orig/drivers/i2c/busses/i2c-mpc.c =A0 =A0 =A02009-12-03 =
04:51:21.000000000 +0100
> +++ linux-2.6.32/drivers/i2c/busses/i2c-mpc.c =A0 2010-01-22 16:05:13.000=
000000 +0100
> @@ -59,6 +59,7 @@ struct mpc_i2c {
> =A0 =A0 =A0 =A0wait_queue_head_t queue;
> =A0 =A0 =A0 =A0struct i2c_adapter adap;
> =A0 =A0 =A0 =A0int irq;
> + =A0 =A0 =A0 u32 real_clk;
> =A0};
>
> =A0struct mpc_i2c_divider {
> @@ -97,16 +98,32 @@ static irqreturn_t mpc_i2c_isr(int irq,
> =A0*/
> =A0static void mpc_i2c_fixup(struct mpc_i2c *i2c)
> =A0{
> - =A0 =A0 =A0 writeccr(i2c, 0);
> - =A0 =A0 =A0 udelay(30);
> - =A0 =A0 =A0 writeccr(i2c, CCR_MEN);
> - =A0 =A0 =A0 udelay(30);
> - =A0 =A0 =A0 writeccr(i2c, CCR_MSTA | CCR_MTX);
> - =A0 =A0 =A0 udelay(30);
> - =A0 =A0 =A0 writeccr(i2c, CCR_MSTA | CCR_MTX | CCR_MEN);
> - =A0 =A0 =A0 udelay(30);
> - =A0 =A0 =A0 writeccr(i2c, CCR_MEN);
> - =A0 =A0 =A0 udelay(30);
> + =A0 =A0 =A0 if (i2c->real_clk =3D=3D 0) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 writeccr(i2c, 0);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(30);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 writeccr(i2c, CCR_MEN);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(30);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 writeccr(i2c, CCR_MSTA | CCR_MTX);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(30);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 writeccr(i2c, CCR_MSTA | CCR_MTX | CCR_MEN)=
;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(30);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 writeccr(i2c, CCR_MEN);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(30);
> + =A0 =A0 =A0 } else {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 int k;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 u32 delay_val =3D 1000000 / i2c->real_clk +=
1;
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (delay_val < 2)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 delay_val =3D 2;
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 for (k =3D 9; k; k--) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 writeccr(i2c, 0);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 writeccr(i2c, CCR_MSTA | CC=
R_MTX | CCR_MEN);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(delay_val);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 writeccr(i2c, CCR_MEN);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(delay_val << 1);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> + =A0 =A0 =A0 }
This doesn't look right. Why is the old code being preserved? Isn't
it not as reliable? It looks to me that the new block should be the
only path, with delay_val getting hard set to a sane value if real_clk
=3D=3D 0. This approach looks to add complexity to the driver without a
reason other than fear it *might* breaking something.
If the new code is better, then be strong, stand tall, and say in a
loud voice, "this old code is crap. The new stuff is much better."
g.
> =A0}
>
> =A0static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing=
)
> @@ -186,15 +203,18 @@ static const struct mpc_i2c_divider mpc_
> =A0 =A0 =A0 =A0{10240, 0x9d}, {12288, 0x9e}, {15360, 0x9f}
> =A0};
>
> -int mpc_i2c_get_fdr_52xx(struct device_node *node, u32 clock, int presca=
ler)
> +int mpc_i2c_get_fdr_52xx(struct device_node *node, u32 clock, int presca=
ler,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0u32 *real_clk)
> =A0{
> =A0 =A0 =A0 =A0const struct mpc_i2c_divider *div =3D NULL;
> =A0 =A0 =A0 =A0unsigned int pvr =3D mfspr(SPRN_PVR);
> =A0 =A0 =A0 =A0u32 divider;
> =A0 =A0 =A0 =A0int i;
>
> - =A0 =A0 =A0 if (!clock)
> + =A0 =A0 =A0 if (!clock) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 *real_clk =3D 0;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -EINVAL;
> + =A0 =A0 =A0 }
>
> =A0 =A0 =A0 =A0/* Determine divider value */
> =A0 =A0 =A0 =A0divider =3D mpc5xxx_get_bus_frequency(node) / clock;
> @@ -212,7 +232,8 @@ int mpc_i2c_get_fdr_52xx(struct device_n
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0break;
> =A0 =A0 =A0 =A0}
>
> - =A0 =A0 =A0 return div ? (int)div->fdr : -EINVAL;
> + =A0 =A0 =A0 *real_clk =3D mpc5xxx_get_bus_frequency(node) / div->divide=
r;
> + =A0 =A0 =A0 return (int)div->fdr;
> =A0}
>
> =A0static void mpc_i2c_setclock_52xx(struct device_node *node,
> @@ -221,13 +242,14 @@ static void mpc_i2c_setclock_52xx(struct
> =A0{
> =A0 =A0 =A0 =A0int ret, fdr;
>
> - =A0 =A0 =A0 ret =3D mpc_i2c_get_fdr_52xx(node, clock, prescaler);
> + =A0 =A0 =A0 ret =3D mpc_i2c_get_fdr_52xx(node, clock, prescaler, &i2c->=
real_clk);
> =A0 =A0 =A0 =A0fdr =3D (ret >=3D 0) ? ret : 0x3f; /* backward compatibili=
ty */
>
> =A0 =A0 =A0 =A0writeb(fdr & 0xff, i2c->base + MPC_I2C_FDR);
>
> =A0 =A0 =A0 =A0if (ret >=3D 0)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_info(i2c->dev, "clock %d Hz (fdr=3D%d)\=
n", clock, fdr);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_info(i2c->dev, "clock %u Hz (fdr=3D%d)\=
n", i2c->real_clk,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0fdr);
> =A0}
> =A0#else /* !CONFIG_PPC_MPC52xx */
> =A0static void mpc_i2c_setclock_52xx(struct device_node *node,
> @@ -446,10 +468,14 @@ static int mpc_xfer(struct i2c_adapter *
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -EINTR;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (time_after(jiffies, orig_jiffies + HZ)=
) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 u8 status =3D readb(i2c->ba=
se + MPC_I2C_SR);
> +
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dev_dbg(i2c->dev, "timeout=
\n");
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (readb(i2c->base + MPC_I=
2C_SR) =3D=3D
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (CSR_MCF | CSR_MBB =
| CSR_RXAK))
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if ((status & (CSR_MCF | CS=
R_MBB | CSR_RXAK)) !=3D 0) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 writeb(stat=
us & ~CSR_MAL,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0i2c->base + MPC_I2C_SR);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0mpc_i2c_fi=
xup(i2c);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -EIO;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0schedule();
> @@ -540,6 +566,14 @@ static int __devinit fsl_i2c_probe(struc
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0}
>
> + =A0 =A0 =A0 prop =3D of_get_property(op->node, "timeout", &plen);
> + =A0 =A0 =A0 if (prop && plen =3D=3D sizeof(u32)) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 mpc_ops.timeout =3D *prop * HZ / 1000000;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (mpc_ops.timeout < 5)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 mpc_ops.timeout =3D 5;
> + =A0 =A0 =A0 }
> + =A0 =A0 =A0 dev_info(i2c->dev, "timeout %u us\n", mpc_ops.timeout * 100=
0000 / HZ);
> +
> =A0 =A0 =A0 =A0dev_set_drvdata(&op->dev, i2c);
>
> =A0 =A0 =A0 =A0i2c->adap =3D mpc_ops;
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
>
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [PATCH] powerpc/mpc512x: Add gpio driver
From: Grant Likely @ 2010-02-16 19:19 UTC (permalink / raw)
To: Matthias Fuchs; +Cc: linuxppc-dev
In-Reply-To: <201001281448.42965.matthias.fuchs@esd-electronics.com>
Hi Matthias. Thanks for the patch. Some comments below...
On Thu, Jan 28, 2010 at 6:48 AM, Matthias Fuchs
<matthias.fuchs@esd-electronics.com> wrote:
> Signed-off-by: Matthias Fuchs <matthias.fuchs@esd.eu>
Could use a commit log. It is useful to give some details about what
this driver has been developed/tested on.
> ---
> =A0arch/powerpc/include/asm/mpc512x_gpio.h | =A0 30 +++++
> =A0arch/powerpc/platforms/Kconfig =A0 =A0 =A0 =A0 =A0| =A0 =A09 ++
> =A0arch/powerpc/sysdev/Makefile =A0 =A0 =A0 =A0 =A0 =A0| =A0 =A01 +
> =A0arch/powerpc/sysdev/mpc512x_gpio.c =A0 =A0 =A0| =A0179 +++++++++++++++=
++++++++++++++++
Move mpc512x_gpio.c to the arch/powerpc/platforms/512x directory.
Also put the Kconfig changes in arch/powerpc/platform/512x/Kconfig.
> =A04 files changed, 219 insertions(+), 0 deletions(-)
> =A0create mode 100644 arch/powerpc/include/asm/mpc512x_gpio.h
> =A0create mode 100644 arch/powerpc/sysdev/mpc512x_gpio.c
>
> diff --git a/arch/powerpc/include/asm/mpc512x_gpio.h b/arch/powerpc/inclu=
de/asm/mpc512x_gpio.h
> new file mode 100644
> index 0000000..8b6922d
> --- /dev/null
> +++ b/arch/powerpc/include/asm/mpc512x_gpio.h
> @@ -0,0 +1,30 @@
> +/*
> + * Copyright (C) 2010 Matthias Fuchs <matthias.fuchs@esd.eu>, esd gmbh
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2
> + * as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. =A0See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA =A002111-130=
7 =A0USA
> + */
> +#ifndef MPC512X_GPIO_H
> +#define MPC512X_GPIO_H
> +
> +struct mpc512x_gpio_regs {
> + =A0 =A0 =A0 u32 gpdir;
> + =A0 =A0 =A0 u32 gpodr;
> + =A0 =A0 =A0 u32 gpdat;
> + =A0 =A0 =A0 u32 gpier;
> + =A0 =A0 =A0 u32 gpimr;
> + =A0 =A0 =A0 u32 gpicr1;
> + =A0 =A0 =A0 u32 gpicr2;
> +};
Currently this is only used by the GPIO driver. Move the structure
definition into the .c file.
> +
> +#endif /* MPC512X_GPIO_H */
> diff --git a/arch/powerpc/sysdev/mpc512x_gpio.c b/arch/powerpc/sysdev/mpc=
512x_gpio.c
> new file mode 100644
> index 0000000..09f075b
> --- /dev/null
> +++ b/arch/powerpc/sysdev/mpc512x_gpio.c
> @@ -0,0 +1,179 @@
> +/*
> + * MPC512x gpio driver
> + *
> + * Copyright (c) 2010 Matthias Fuchs <matthias.fuchs@esd.eu>, esd gmbh
> + *
> + * derived from ppc4xx gpio driver
> + *
> + * Copyright (c) 2008 Harris Corporation
> + * Copyright (c) 2008 Sascha Hauer <s.hauer@pengutronix.de>, Pengutronix
> + * Copyright (c) MontaVista Software, Inc. 2008.
> + *
> + * Author: Steve Falco <sfalco@harris.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2
> + * as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. =A0See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA =A002111-130=
7 =A0USA
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/spinlock.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#include <linux/gpio.h>
> +#include <linux/types.h>
> +#include <asm/mpc512x_gpio.h>
> +
> +#define GPIO_MASK(gpio) =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(0x80000000 >> (g=
pio))
For clarity, this local define should also be prefixed with MPC5121_
so it is not confused with core gpio code.
> +
> +struct mpc512x_chip {
> + =A0 =A0 =A0 struct of_mm_gpio_chip mm_gc;
> + =A0 =A0 =A0 spinlock_t lock;
> +};
> +
> +/*
> + * GPIO LIB API implementation for GPIOs
> + *
> + * There are a maximum of 32 gpios in each gpio controller.
> + */
> +static inline struct mpc512x_chip *
> +to_mpc512x_gpiochip(struct of_mm_gpio_chip *mm_gc)
> +{
> + =A0 =A0 =A0 return container_of(mm_gc, struct mpc512x_chip, mm_gc);
> +}
> +
> +static int mpc512x_gpio_get(struct gpio_chip *gc, unsigned int gpio)
> +{
> + =A0 =A0 =A0 struct of_mm_gpio_chip *mm_gc =3D to_of_mm_gpio_chip(gc);
> + =A0 =A0 =A0 struct mpc512x_gpio_regs __iomem *regs =3D mm_gc->regs;
> +
> + =A0 =A0 =A0 return in_be32(®s->gpdat) & GPIO_MASK(gpio);
> +}
> +
> +static inline void
> +__mpc512x_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
> +{
> + =A0 =A0 =A0 struct of_mm_gpio_chip *mm_gc =3D to_of_mm_gpio_chip(gc);
> + =A0 =A0 =A0 struct mpc512x_gpio_regs __iomem *regs =3D mm_gc->regs;
> +
> + =A0 =A0 =A0 if (val)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 setbits32(®s->gpdat, GPIO_MASK(gpio));
> + =A0 =A0 =A0 else
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 clrbits32(®s->gpdat, GPIO_MASK(gpio));
Rather than the read/modify/write cycle you're using here, you should
consider maintaining a shadow register for the GPIO output register.
Even though it is the SoC local bus, bus cycles can be expensive when
a shadow reg may already be in the cache.
Also, this may be a bug. If you read the data register, modify just
the bit you care about and then write it back, then you are also
setting the output state of pins currently marked for input. That
doesn't sound bad, but some drivers change the output state by
switching back and forth between input and output mode. Using a
read/modify/write on the data register will break it.
> +}
> +
> +static void
> +mpc512x_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
> +{
> + =A0 =A0 =A0 struct of_mm_gpio_chip *mm_gc =3D to_of_mm_gpio_chip(gc);
> + =A0 =A0 =A0 struct mpc512x_chip *chip =3D to_mpc512x_gpiochip(mm_gc);
> + =A0 =A0 =A0 unsigned long flags;
> +
> + =A0 =A0 =A0 spin_lock_irqsave(&chip->lock, flags);
> +
> + =A0 =A0 =A0 __mpc512x_gpio_set(gc, gpio, val);
> +
> + =A0 =A0 =A0 spin_unlock_irqrestore(&chip->lock, flags);
> +
> + =A0 =A0 =A0 pr_debug("%s: gpio: %d val: %d\n", __func__, gpio, val);
> +}
> +
> +static int mpc512x_gpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
> +{
> + =A0 =A0 =A0 struct of_mm_gpio_chip *mm_gc =3D to_of_mm_gpio_chip(gc);
> + =A0 =A0 =A0 struct mpc512x_chip *chip =3D to_mpc512x_gpiochip(mm_gc);
> + =A0 =A0 =A0 struct mpc512x_gpio_regs __iomem *regs =3D mm_gc->regs;
> + =A0 =A0 =A0 unsigned long flags;
> +
> + =A0 =A0 =A0 spin_lock_irqsave(&chip->lock, flags);
> +
> + =A0 =A0 =A0 /* Disable open-drain function */
> + =A0 =A0 =A0 clrbits32(®s->gpodr, GPIO_MASK(gpio));
> +
> + =A0 =A0 =A0 /* Float the pin */
> + =A0 =A0 =A0 clrbits32(®s->gpdir, GPIO_MASK(gpio));
> +
> + =A0 =A0 =A0 spin_unlock_irqrestore(&chip->lock, flags);
> +
> + =A0 =A0 =A0 return 0;
> +}
> +
> +static int
> +mpc512x_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
> +{
> + =A0 =A0 =A0 struct of_mm_gpio_chip *mm_gc =3D to_of_mm_gpio_chip(gc);
> + =A0 =A0 =A0 struct mpc512x_chip *chip =3D to_mpc512x_gpiochip(mm_gc);
> + =A0 =A0 =A0 struct mpc512x_gpio_regs __iomem *regs =3D mm_gc->regs;
> + =A0 =A0 =A0 unsigned long flags;
> +
> + =A0 =A0 =A0 spin_lock_irqsave(&chip->lock, flags);
> +
> + =A0 =A0 =A0 /* First set initial value */
> + =A0 =A0 =A0 __mpc512x_gpio_set(gc, gpio, val);
> +
> + =A0 =A0 =A0 /* Disable open-drain function */
> + =A0 =A0 =A0 clrbits32(®s->gpodr, GPIO_MASK(gpio));
> +
> + =A0 =A0 =A0 /* Drive the pin */
> + =A0 =A0 =A0 setbits32(®s->gpdir, GPIO_MASK(gpio));
> +
> + =A0 =A0 =A0 spin_unlock_irqrestore(&chip->lock, flags);
> +
> + =A0 =A0 =A0 pr_debug("%s: gpio: %d val: %d\n", __func__, gpio, val);
> +
> + =A0 =A0 =A0 return 0;
> +}
> +
> +static int __init mpc512x_add_gpiochips(void)
> +{
> + =A0 =A0 =A0 struct device_node *np;
> +
> + =A0 =A0 =A0 for_each_compatible_node(np, NULL, "fsl,mpc5121-gpio") {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 int ret;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct mpc512x_chip *mpc512x_gc;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct of_mm_gpio_chip *mm_gc;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct of_gpio_chip *of_gc;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct gpio_chip *gc;
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 mpc512x_gc =3D kzalloc(sizeof(*mpc512x_gc),=
GFP_KERNEL);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!mpc512x_gc) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D -ENOMEM;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 spin_lock_init(&mpc512x_gc->lock);
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 mm_gc =3D &mpc512x_gc->mm_gc;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 of_gc =3D &mm_gc->of_gc;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 gc =3D &of_gc->gc;
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 gc->ngpio =3D 32;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 gc->direction_input =3D mpc512x_gpio_dir_in=
;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 gc->direction_output =3D mpc512x_gpio_dir_o=
ut;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 gc->get =3D mpc512x_gpio_get;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 gc->set =3D mpc512x_gpio_set;
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D of_mm_gpiochip_add(np, mm_gc);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (ret)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 continue;
> +err:
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 pr_err("%s: registration failed with status=
%d\n",
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0np->full_name, ret);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 kfree(mpc512x_gc);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* try others anyway */
> + =A0 =A0 =A0 }
> + =A0 =A0 =A0 return 0;
> +}
> +arch_initcall(mpc512x_add_gpiochips);
Don't do this. Either make this a proper of_platform device driver,
or call mpc512x_add_gpiochips() explicitly from the arch platform
setup code. Otherwise, if the kernel is built for multiplatform, this
function will get executed regardless of the platform.
Cheers,
g.
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [PATCH v7 1/4] i2c-mpc: use __devinit[data] for initialization functions and data
From: Grant Likely @ 2010-02-16 18:40 UTC (permalink / raw)
To: Wolfgang Grandegger
Cc: Ben Dooks, Devicetree-discuss, Linuxppc-dev, Linux-i2c,
Wolfgang Grandegger
In-Reply-To: <1265813711-16794-2-git-send-email-wg@grandegger.com>
On Wed, Feb 10, 2010 at 7:55 AM, Wolfgang Grandegger <wg@grandegger.com> wr=
ote:
> From: Wolfgang Grandegger <wg@denx.de>
>
> "__devinit[data]" has not yet been used for all initialization functions
> and data. To avoid truncating lines, the struct "mpc_i2c_match_data" has
> been renamed to "mpc_i2c_data", which is even the better name.
>
> Signed-off-by: Wolfgang Grandegger <wg@denx.de>
> Tested-by: Wolfram Sang <w.sang@pengutronix.de>
Between patch 1 & 2 is not bisectable. Functions still called
*_setclock in this patch, but referenced as *_setup in the structure.
Please respin.
Also ...
> +static struct mpc_i2c_data mpc_i2c_data_52xx __devinitdata =3D {
> + =A0 =A0 =A0 .setup =3D mpc_i2c_setup_52xx,
> +};
> +
> +static struct mpc_i2c_data mpc_i2c_data_8313 __devinitdata =3D {
> + =A0 =A0 =A0 .setup =3D mpc_i2c_setup_8xxx,
> +};
> +
> +static struct mpc_i2c_data mpc_i2c_data_8543 __devinitdata =3D {
> + =A0 =A0 =A0 .setup =3D mpc_i2c_setup_8xxx,
> + =A0 =A0 =A0 .prescaler =3D 2,
> +};
> +
> +static struct mpc_i2c_data mpc_i2c_data_8544 __devinitdata =3D {
> + =A0 =A0 =A0 .setup =3D mpc_i2c_setup_8xxx,
> + =A0 =A0 =A0 .prescaler =3D 3,
> +};
> +
> =A0static const struct of_device_id mpc_i2c_of_match[] =3D {
> - =A0 =A0 =A0 {.compatible =3D "mpc5200-i2c",
> - =A0 =A0 =A0 =A0.data =3D &(struct mpc_i2c_match_data) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 .setclock =3D mpc_i2c_setcl=
ock_52xx,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 },
> - =A0 =A0 =A0 },
> - =A0 =A0 =A0 {.compatible =3D "fsl,mpc5200b-i2c",
> - =A0 =A0 =A0 =A0.data =3D &(struct mpc_i2c_match_data) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 .setclock =3D mpc_i2c_setcl=
ock_52xx,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 },
> - =A0 =A0 =A0 },
> - =A0 =A0 =A0 {.compatible =3D "fsl,mpc5200-i2c",
> - =A0 =A0 =A0 =A0.data =3D &(struct mpc_i2c_match_data) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 .setclock =3D mpc_i2c_setcl=
ock_52xx,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 },
> - =A0 =A0 =A0 },
> - =A0 =A0 =A0 {.compatible =3D "fsl,mpc8313-i2c",
> - =A0 =A0 =A0 =A0.data =3D &(struct mpc_i2c_match_data) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 .setclock =3D mpc_i2c_setcl=
ock_8xxx,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 },
> - =A0 =A0 =A0 },
> - =A0 =A0 =A0 {.compatible =3D "fsl,mpc8543-i2c",
> - =A0 =A0 =A0 =A0.data =3D &(struct mpc_i2c_match_data) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 .setclock =3D mpc_i2c_setcl=
ock_8xxx,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 .prescaler =3D 2,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 },
> - =A0 =A0 =A0 },
> - =A0 =A0 =A0 {.compatible =3D "fsl,mpc8544-i2c",
> - =A0 =A0 =A0 =A0.data =3D &(struct mpc_i2c_match_data) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 .setclock =3D mpc_i2c_setcl=
ock_8xxx,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 .prescaler =3D 3,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 },
> + =A0 =A0 =A0 {.compatible =3D "mpc5200-i2c", .data =3D &mpc_i2c_data_52x=
x, },
> + =A0 =A0 =A0 {.compatible =3D "fsl,mpc5200b-i2c", .data =3D &mpc_i2c_dat=
a_52xx, },
> + =A0 =A0 =A0 {.compatible =3D "fsl,mpc5200-i2c", .data =3D &mpc_i2c_data=
_52xx, },
> + =A0 =A0 =A0 {.compatible =3D "fsl,mpc8313-i2c", .data =3D &mpc_i2c_data=
_8313, },
> + =A0 =A0 =A0 {.compatible =3D "fsl,mpc8543-i2c", .data =3D &mpc_i2c_data=
_8543, },
> + =A0 =A0 =A0 {.compatible =3D "fsl,mpc8544-i2c", .data =3D &mpc_i2c_data=
_8544, },
... what was wrong with the old format of declaring the .data
structures inline with the match table?
g.
^ permalink raw reply
* Re: [PATCH v3 10/11] powerpc/mpc5121: update mpc5121ads DTS
From: Grant Likely @ 2010-02-16 18:11 UTC (permalink / raw)
To: Anatolij Gustschin; +Cc: linuxppc-dev, wd, dzu, Piotr Ziecik
In-Reply-To: <1265377377-29327-11-git-send-email-agust@denx.de>
On Fri, Feb 5, 2010 at 6:42 AM, Anatolij Gustschin <agust@denx.de> wrote:
> Collects several changes needed after applying
> previous mpc5121 platform and driver patches:
I'll merge this one, but...
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0display@2100 {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 compatible =3D "fsl,mpc5121=
-diu", "fsl-diu";
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 compatible =3D "fsl,mpc5121=
-diu", "fsl,diu";
I want to see the "fsl,diu" value removed. It is meaningless (the
8xxx and 5121 dius are similar, but they are not identical), and the
driver should bind against "fsl,mpc5121-diu" directly. Can you please
send a followup patch to fix this?
Thanks,
g.
^ permalink raw reply
* Re: [PATCH v3 09/11] powerpc/mpc5121: shared DIU framebuffer support
From: Grant Likely @ 2010-02-16 18:06 UTC (permalink / raw)
To: Anatolij Gustschin, York Sun, Kumar Gala
Cc: John Rigby, linuxppc-dev, wd, dzu
In-Reply-To: <1265377377-29327-10-git-send-email-agust@denx.de>
On Fri, Feb 5, 2010 at 6:42 AM, Anatolij Gustschin <agust@denx.de> wrote:
> MPC5121 DIU configuration/setup as initialized by the boot
> loader currently will get lost while booting Linux. As a
> result displaying the boot splash is not possible through
> the boot process.
>
> To prevent this we reserve configured DIU frame buffer
> address range while booting and preserve AOI descriptor
> and gamma table so that DIU continues displaying through
> the whole boot process. On first open from user space
> DIU frame buffer driver releases the reserved frame
> buffer area and continues to operate as usual.
>
> The patch also moves drivers/video/fsl-diu-fb.h file to
> include/linux as we use some DIU structures in platform
> code.
>
> 'diu_ops' callbacks in platform code borrowed from John's
> DIU code.
>
> Signed-off-by: John Rigby <jrigby@gmail.com>
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> ---
[...]
> diff --git a/drivers/video/fsl-diu-fb.c b/drivers/video/fsl-diu-fb.c
> index 72d68b3..29c7f31 100644
> --- a/drivers/video/fsl-diu-fb.c
> +++ b/drivers/video/fsl-diu-fb.c
> @@ -34,7 +34,7 @@
> =A0#include <linux/of_platform.h>
>
> =A0#include <sysdev/fsl_soc.h>
> -#include "fsl-diu-fb.h"
> +#include <linux/fsl-diu-fb.h>
>
> =A0/*
> =A0* These parameters give default parameters
> @@ -178,6 +178,21 @@ static struct fb_videomode __devinitdata fsl_diu_mod=
e_db[] =3D {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0.sync =A0 =A0 =A0 =A0 =A0 =3D FB_SYNC_COMP=
_HIGH_ACT | FB_SYNC_VERT_HIGH_ACT,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0.vmode =A0 =A0 =A0 =A0 =A0=3D FB_VMODE_NON=
INTERLACED
> =A0 =A0 =A0 =A0},
> + =A0 =A0 =A0 {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 .name =A0 =A0 =A0 =A0 =A0 =3D "800x480-60",
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 .refresh =A0 =A0 =A0 =A0=3D 60,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 .xres =A0 =A0 =A0 =A0 =A0 =3D 800,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 .yres =A0 =A0 =A0 =A0 =A0 =3D 480,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 .pixclock =A0 =A0 =A0 =3D 31250,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 .left_margin =A0 =A0=3D 86,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 .right_margin =A0 =3D 42,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 .upper_margin =A0 =3D 33,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 .lower_margin =A0 =3D 10,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 .hsync_len =A0 =A0 =A0=3D 128,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 .vsync_len =A0 =A0 =A0=3D 2,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 .sync =A0 =A0 =A0 =A0 =A0 =3D FB_SYNC_COMP_=
HIGH_ACT | FB_SYNC_VERT_HIGH_ACT,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 .vmode =A0 =A0 =A0 =A0 =A0=3D FB_VMODE_NONI=
NTERLACED
> + =A0 =A0 =A0 },
> =A0};
This hunk bothers me. It looks like the type of data that belongs
either in some common shared .c file, or encoded into the device tree.
It seems to be data about the display panel, instead of data about
the framebuffer driver. I know that the driver already uses this
pattern, but before I merge this patch and further rely on that
pattern, I think it is worth discussing.
Kumar, York, thoughts?
g.
^ permalink raw reply
* Re: [PATCH] spi: Correct SPI clock frequency setting in spi_mpc8xxx
From: Grant Likely @ 2010-02-16 17:39 UTC (permalink / raw)
To: Ernst Schwab; +Cc: spi-devel-general, linuxppc-dev
In-Reply-To: <20100216174349.b0ea60ef.eschwab@online.de>
On Tue, Feb 16, 2010 at 9:43 AM, Ernst Schwab <eschwab@online.de> wrote:
> Grant Likely <grant.likely@secretlab.ca> wrote:
>
>> The change forces the division to always round up instead of down.
>> Please describe (for me now, and for people looking at the commit in
>> the future) the mathematical reason for the changes.
>
> Ok, here are some infos on the problem I observed, and the fix.
Excellent description. Thank you. I'll add this to the commit text
when I apply the patch.
g.
>
> Example:
>
> When specifying spi-max-frequency =3D <10000000> in the device tree,
> the resulting frequency was 11.1 MHz, with spibrg being 133333332.
>
> According to the freescale datasheet [1], the spi clock rate is
> spiclk =3D spibrg / (4 * (pm+1))
>
> The existing code calculated
> =A0 pm =3D mpc8xxx_spi->spibrg / (hz * 4); pm--;
> resulting in pm =3D (int) (3.3333) - 1 =3D 2,
> resulting in spiclk =3D 133333332/(4*(2+1)) =3D 11111111
>
> With the fix,
> =A0pm =3D (mpc8xxx_spi->spibrg - 1) / (hz * 4) + 1; pm--;
> resulting in pm =3D (int) (4.3333) - 1 =3D 3,
> resulting in spiclk =3D 133333332/(4*(3+1)) =3D 8333333
>
> Without the fix, for every desired SPI frequency that
> is not exactly deriveable from spibrg, pm will be too
> small due to rounding down, resulting in a too high SPI clock,
> so we need a pm which is one higher.
>
> For values that are exactly deriveable, spibrg will
> be divideable by (hz*4) without remainder, and
> (int) ((spibrg-1)/(hz*4)) will be one lower than
> (int) (spibrg)/(hz*4), which is compensated by adding 1.
> For these values, the fixed version calculates the same pm
> as the unfixed version.
>
> For all values that are not exactly deriveable,
> spibrg will be not divideable by (hz*4) without
> remainder, and (int) ((spibrg-1)/(hz*4)) will be
> the same as (int) (spibrg)/(hz*4), and the calculated pm will
> be one higher than calculated by the unfixed version.
>
> Regards
> Ernst
>
> References:
> [1] http://www.freescale.com/files/32bit/doc/ref_manual/MPC8315ERM.pdf,
> =A0 =A0page 22-10 -> 1398
> "
> pm: Prescale modulus select. Specifies the divide ratio of the
> prescale divider in the SPI clock generator. The SPI baud rate generator
> clock source (either system clock or system clock divided by 16,
> depending on DIV16 bit) is divided by 4 * ([PM] + 1), a range from 4 to 6=
4.
> The clock has a 50% duty cycle. For example, if the prescale
> modulus is set to PM =3D 0011 and DIV16 is set, the system/SPICLK clock
> ratio will be 16 * (4 * (0011 + 1)) =3D 256.
> "
>
>
>> >
>> > Signed-off-by: Ernst Schwab <eschwab@online.de>
>> > ---
>> > Tested on MPC8314.
>> >
>> > diff -up linux-2.6.33-rc8.orig/drivers/spi/spi_mpc8xxx.c linux-2.6.33-=
rc8/drivers/spi/spi_mpc8xxx.c
>> > --- linux-2.6.33-rc8.orig/drivers/spi/spi_mpc8xxx.c =A0 =A0 2010-02-12=
20:07:45.000000000 +0100
>> > +++ linux-2.6.33-rc8/drivers/spi/spi_mpc8xxx.c =A02010-02-15 14:08:33.=
000000000 +0100
>> > @@ -365,7 +365,7 @@ int mpc8xxx_spi_setup_transfer(struct sp
>> >
>> > =A0 =A0 =A0 =A0if ((mpc8xxx_spi->spibrg / hz) > 64) {
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0cs->hw_mode |=3D SPMODE_DIV16;
>> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 pm =3D mpc8xxx_spi->spibrg / (hz * 64);
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 pm =3D (mpc8xxx_spi->spibrg - 1) / (hz *=
64) + 1;
>> >
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0WARN_ONCE(pm > 16, "%s: Requested speed=
is too low: %d Hz. "
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"Will use %d Hz ins=
tead.\n", dev_name(&spi->dev),
>> > @@ -373,7 +373,7 @@ int mpc8xxx_spi_setup_transfer(struct sp
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (pm > 16)
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pm =3D 16;
>> > =A0 =A0 =A0 =A0} else
>> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 pm =3D mpc8xxx_spi->spibrg / (hz * 4);
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 pm =3D (mpc8xxx_spi->spibrg - 1) / (hz *=
4) + 1;
>> > =A0 =A0 =A0 =A0if (pm)
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pm--;
>> >
>> >
>> >
>> >
>> > --
>> >
>> >
>>
>>
>>
>> --
>> Grant Likely, B.Sc., P.Eng.
>> Secret Lab Technologies Ltd.
>
>
> --
>
>
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [PATCH] spi: Correct SPI clock frequency setting in spi_mpc8xxx
From: Ernst Schwab @ 2010-02-16 16:43 UTC (permalink / raw)
To: Grant Likely; +Cc: spi-devel-general, linuxppc-dev
In-Reply-To: <fa686aa41002160732w1da68f35ifd0d21407b1052e4@mail.gmail.com>
Grant Likely <grant.likely@secretlab.ca> wrote:
> The change forces the division to always round up instead of down.
> Please describe (for me now, and for people looking at the commit in
> the future) the mathematical reason for the changes.
Ok, here are some infos on the problem I observed, and the fix.
Example:
When specifying spi-max-frequency =3D <10000000> in the device tree,
the resulting frequency was 11.1 MHz, with spibrg being 133333332.=20
According to the freescale datasheet [1], the spi clock rate is
spiclk =3D spibrg / (4 * (pm+1))
The existing code calculated =20
pm =3D mpc8xxx_spi->spibrg / (hz * 4); pm--;
resulting in pm =3D (int) (3.3333) - 1 =3D 2,=20
resulting in spiclk =3D 133333332/(4*(2+1)) =3D 11111111
With the fix,=20
pm =3D (mpc8xxx_spi->spibrg - 1) / (hz * 4) + 1; pm--;
resulting in pm =3D (int) (4.3333) - 1 =3D 3,
resulting in spiclk =3D 133333332/(4*(3+1)) =3D 8333333
Without the fix, for every desired SPI frequency that=20
is not exactly deriveable from spibrg, pm will be too=20
small due to rounding down, resulting in a too high SPI clock,
so we need a pm which is one higher.
For values that are exactly deriveable, spibrg will
be divideable by (hz*4) without remainder, and=20
(int) ((spibrg-1)/(hz*4)) will be one lower than=20
(int) (spibrg)/(hz*4), which is compensated by adding 1.
For these values, the fixed version calculates the same pm=20
as the unfixed version.
For all values that are not exactly deriveable,
spibrg will be not divideable by (hz*4) without=20
remainder, and (int) ((spibrg-1)/(hz*4)) will be=20
the same as (int) (spibrg)/(hz*4), and the calculated pm will
be one higher than calculated by the unfixed version.
Regards
Ernst
References:
[1] http://www.freescale.com/files/32bit/doc/ref_manual/MPC8315ERM.pdf,
page 22-10 -> 1398
"
pm: Prescale modulus select. Specifies the divide ratio of the=20
prescale divider in the SPI clock generator. The SPI baud rate generator
clock source (either system clock or system clock divided by 16,=20
depending on DIV16 bit) is divided by 4 * ([PM] + 1), a range from 4 to 64.=
=20
The clock has a 50% duty cycle. For example, if the prescale
modulus is set to PM =3D 0011 and DIV16 is set, the system/SPICLK clock=20
ratio will be 16 * (4 * (0011 + 1)) =3D 256.
"
> >
> > Signed-off-by: Ernst Schwab <eschwab@online.de>
> > ---
> > Tested on MPC8314.
> >
> > diff -up linux-2.6.33-rc8.orig/drivers/spi/spi_mpc8xxx.c linux-2.6.33-r=
c8/drivers/spi/spi_mpc8xxx.c
> > --- linux-2.6.33-rc8.orig/drivers/spi/spi_mpc8xxx.c =A0 =A0 2010-02-12 =
20:07:45.000000000 +0100
> > +++ linux-2.6.33-rc8/drivers/spi/spi_mpc8xxx.c =A02010-02-15 14:08:33.0=
00000000 +0100
> > @@ -365,7 +365,7 @@ int mpc8xxx_spi_setup_transfer(struct sp
> >
> > =A0 =A0 =A0 =A0if ((mpc8xxx_spi->spibrg / hz) > 64) {
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0cs->hw_mode |=3D SPMODE_DIV16;
> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 pm =3D mpc8xxx_spi->spibrg / (hz * 64);
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 pm =3D (mpc8xxx_spi->spibrg - 1) / (hz * =
64) + 1;
> >
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0WARN_ONCE(pm > 16, "%s: Requested speed =
is too low: %d Hz. "
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"Will use %d Hz inst=
ead.\n", dev_name(&spi->dev),
> > @@ -373,7 +373,7 @@ int mpc8xxx_spi_setup_transfer(struct sp
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (pm > 16)
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pm =3D 16;
> > =A0 =A0 =A0 =A0} else
> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 pm =3D mpc8xxx_spi->spibrg / (hz * 4);
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 pm =3D (mpc8xxx_spi->spibrg - 1) / (hz * =
4) + 1;
> > =A0 =A0 =A0 =A0if (pm)
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pm--;
> >
> >
> >
> >
> > --
> >
> >
>=20
>=20
>=20
> --=20
> Grant Likely, B.Sc., P.Eng.
> Secret Lab Technologies Ltd.
--=20
^ permalink raw reply
* Re: [PATCH 3/3] i2c/ibm-iic: drop NO_IRQ
From: Grant Likely @ 2010-02-16 16:01 UTC (permalink / raw)
To: Wolfram Sang; +Cc: linuxppc-dev, linux-i2c, Ben Dooks, Sean MacLennan
In-Reply-To: <1256245888-25210-4-git-send-email-w.sang@pengutronix.de>
On Thu, Oct 22, 2009 at 2:11 PM, Wolfram Sang <w.sang@pengutronix.de> wrote=
:
> Drop NO_IRQ as 0 is the preferred way to describe 'no irq'
> (http://lkml.org/lkml/2005/11/21/221). This change is safe, as the driver=
is
> only used on powerpc, where NO_IRQ is 0 anyhow.
>
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> Cc: Sean MacLennan <smaclennan@pikatech.com>
> Cc: Ben Dooks <ben-linux@fluff.org>
Acked-by: Grant Likely <grant.likely@secretlab.ca>
Ben, will you pick up this series of 3, or would you like me to merge
them through the powerpc tree?
Thanks,
g.
> ---
> =A0drivers/i2c/busses/i2c-ibm_iic.c | =A0 14 +++++++-------
> =A01 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ib=
m_iic.c
> index b1bc6e2..2bef534 100644
> --- a/drivers/i2c/busses/i2c-ibm_iic.c
> +++ b/drivers/i2c/busses/i2c-ibm_iic.c
> @@ -668,12 +668,12 @@ static int __devinit iic_request_irq(struct of_devi=
ce *ofdev,
> =A0 =A0 =A0 =A0int irq;
>
> =A0 =A0 =A0 =A0if (iic_force_poll)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return NO_IRQ;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return 0;
>
> =A0 =A0 =A0 =A0irq =3D irq_of_parse_and_map(np, 0);
> - =A0 =A0 =A0 if (irq =3D=3D NO_IRQ) {
> + =A0 =A0 =A0 if (!irq) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dev_err(&ofdev->dev, "irq_of_parse_and_map=
failed\n");
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return NO_IRQ;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return 0;
> =A0 =A0 =A0 =A0}
>
> =A0 =A0 =A0 =A0/* Disable interrupts until we finish initialization, assu=
mes
> @@ -683,7 +683,7 @@ static int __devinit iic_request_irq(struct of_device=
*ofdev,
> =A0 =A0 =A0 =A0if (request_irq(irq, iic_handler, 0, "IBM IIC", dev)) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dev_err(&ofdev->dev, "request_irq %d faile=
d\n", irq);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/* Fallback to the polling mode */
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return NO_IRQ;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return 0;
> =A0 =A0 =A0 =A0}
>
> =A0 =A0 =A0 =A0return irq;
> @@ -719,7 +719,7 @@ static int __devinit iic_probe(struct of_device *ofde=
v,
> =A0 =A0 =A0 =A0init_waitqueue_head(&dev->wq);
>
> =A0 =A0 =A0 =A0dev->irq =3D iic_request_irq(ofdev, dev);
> - =A0 =A0 =A0 if (dev->irq =3D=3D NO_IRQ)
> + =A0 =A0 =A0 if (!dev->irq)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dev_warn(&ofdev->dev, "using polling mode\=
n");
>
> =A0 =A0 =A0 =A0/* Board specific settings */
> @@ -766,7 +766,7 @@ static int __devinit iic_probe(struct of_device *ofde=
v,
> =A0 =A0 =A0 =A0return 0;
>
> =A0error_cleanup:
> - =A0 =A0 =A0 if (dev->irq !=3D NO_IRQ) {
> + =A0 =A0 =A0 if (dev->irq) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0iic_interrupt_mode(dev, 0);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0free_irq(dev->irq, dev);
> =A0 =A0 =A0 =A0}
> @@ -790,7 +790,7 @@ static int __devexit iic_remove(struct of_device *ofd=
ev)
>
> =A0 =A0 =A0 =A0i2c_del_adapter(&dev->adap);
>
> - =A0 =A0 =A0 if (dev->irq !=3D NO_IRQ) {
> + =A0 =A0 =A0 if (dev->irq) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0iic_interrupt_mode(dev, 0);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0free_irq(dev->irq, dev);
> =A0 =A0 =A0 =A0}
> --
> 1.6.5
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [PATCH 2/3] i2c/cpm: drop NO_IRQ
From: Grant Likely @ 2010-02-16 15:59 UTC (permalink / raw)
To: Wolfram Sang; +Cc: linuxppc-dev, linux-i2c, Ben Dooks
In-Reply-To: <1256245888-25210-3-git-send-email-w.sang@pengutronix.de>
On Thu, Oct 22, 2009 at 2:11 PM, Wolfram Sang <w.sang@pengutronix.de> wrote=
:
> Drop NO_IRQ as 0 is the preferred way to describe 'no irq'
> (http://lkml.org/lkml/2005/11/21/221). This change is safe, as the driver=
is
> only used on powerpc, where NO_IRQ is 0 anyhow.
>
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> Cc: Jochen Friedrich <jochen@scram.de>
> Cc: Ben Dooks <ben-linux@fluff.org>
Acked-by: Grant Likely <grant.likely@secretlab.ca>
> ---
> =A0drivers/i2c/busses/i2c-cpm.c | =A0 =A02 +-
> =A01 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-cpm.c b/drivers/i2c/busses/i2c-cpm.c
> index 9c2e100..16948db 100644
> --- a/drivers/i2c/busses/i2c-cpm.c
> +++ b/drivers/i2c/busses/i2c-cpm.c
> @@ -441,7 +441,7 @@ static int __devinit cpm_i2c_setup(struct cpm_i2c *cp=
m)
> =A0 =A0 =A0 =A0init_waitqueue_head(&cpm->i2c_wait);
>
> =A0 =A0 =A0 =A0cpm->irq =3D of_irq_to_resource(ofdev->node, 0, NULL);
> - =A0 =A0 =A0 if (cpm->irq =3D=3D NO_IRQ)
> + =A0 =A0 =A0 if (!cpm->irq)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -EINVAL;
>
> =A0 =A0 =A0 =A0/* Install interrupt handler. */
> --
> 1.6.5
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [PATCH 1/3] i2c/mpc: drop NO_IRQ
From: Grant Likely @ 2010-02-16 15:59 UTC (permalink / raw)
To: Wolfram Sang; +Cc: linuxppc-dev, linux-i2c, Ben Dooks
In-Reply-To: <1256245888-25210-2-git-send-email-w.sang@pengutronix.de>
On Thu, Oct 22, 2009 at 2:11 PM, Wolfram Sang <w.sang@pengutronix.de> wrote=
:
> Drop NO_IRQ as 0 is the preferred way to describe 'no irq'
> (http://lkml.org/lkml/2005/11/21/221). This change is safe, as the driver=
is
> only used on powerpc, where NO_IRQ is 0 anyhow.
>
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Ben Dooks <ben-linux@fluff.org>
Acked-by: Grant Likely <grant.likely@secretlab.ca>
> ---
> =A0drivers/i2c/busses/i2c-mpc.c | =A0 =A06 +++---
> =A01 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> index d325e86..7e58443 100644
> --- a/drivers/i2c/busses/i2c-mpc.c
> +++ b/drivers/i2c/busses/i2c-mpc.c
> @@ -115,7 +115,7 @@ static int i2c_wait(struct mpc_i2c *i2c, unsigned tim=
eout, int writing)
> =A0 =A0 =A0 =A0u32 x;
> =A0 =A0 =A0 =A0int result =3D 0;
>
> - =A0 =A0 =A0 if (i2c->irq =3D=3D NO_IRQ) {
> + =A0 =A0 =A0 if (!i2c->irq) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0while (!(readb(i2c->base + MPC_I2C_SR) & C=
SR_MIF)) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0schedule();
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (time_after(jiffies, or=
ig_jiffies + timeout)) {
> @@ -520,7 +520,7 @@ static int __devinit fsl_i2c_probe(struct of_device *=
op,
> =A0 =A0 =A0 =A0}
>
> =A0 =A0 =A0 =A0i2c->irq =3D irq_of_parse_and_map(op->node, 0);
> - =A0 =A0 =A0 if (i2c->irq !=3D NO_IRQ) { /* i2c->irq =3D NO_IRQ implies =
polling */
> + =A0 =A0 =A0 if (i2c->irq) { /* no i2c->irq implies polling */
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0result =3D request_irq(i2c->irq, mpc_i2c_i=
sr,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 I=
RQF_SHARED, "i2c-mpc", i2c);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (result < 0) {
> @@ -579,7 +579,7 @@ static int __devexit fsl_i2c_remove(struct of_device =
*op)
> =A0 =A0 =A0 =A0i2c_del_adapter(&i2c->adap);
> =A0 =A0 =A0 =A0dev_set_drvdata(&op->dev, NULL);
>
> - =A0 =A0 =A0 if (i2c->irq !=3D NO_IRQ)
> + =A0 =A0 =A0 if (i2c->irq)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0free_irq(i2c->irq, i2c);
>
> =A0 =A0 =A0 =A0irq_dispose_mapping(i2c->irq);
> --
> 1.6.5
>
>
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [PATCH] spi: Correct SPI clock frequency setting in spi_mpc8xxx
From: Grant Likely @ 2010-02-16 15:32 UTC (permalink / raw)
To: Ernst Schwab, spi-devel-general, Kumar Gala, linuxppc-dev
In-Reply-To: <20100216151240.148f9413.eschwab@online.de>
[cc'd spi-devel-general, linuxppc-dev & Kumar Gala]
On Tue, Feb 16, 2010 at 7:12 AM, Ernst Schwab <eschwab@online.de> wrote:
> From: Ernst Schwab <eschwab@online.de>
>
> Correct SPI clock frequency division factor rounding, preventing clock ra=
tes
> higher than the maximum specified clock frequency being used.
For a patch like this, it helps to also cc the driver's specific
maintainer (Kumar) and to explicitly state your rational so that I
don't need to re-derive your calculations. I'm more likely to merge a
brand new, if potentially broken, driver than to merge a change to an
existing driver that I don't know the impact of.
The change forces the division to always round up instead of down.
Please describe (for me now, and for people looking at the commit in
the future) the mathematical reason for the changes.
Thanks,
g.
>
> Signed-off-by: Ernst Schwab <eschwab@online.de>
> ---
> Tested on MPC8314.
>
> diff -up linux-2.6.33-rc8.orig/drivers/spi/spi_mpc8xxx.c linux-2.6.33-rc8=
/drivers/spi/spi_mpc8xxx.c
> --- linux-2.6.33-rc8.orig/drivers/spi/spi_mpc8xxx.c =A0 =A0 2010-02-12 20=
:07:45.000000000 +0100
> +++ linux-2.6.33-rc8/drivers/spi/spi_mpc8xxx.c =A02010-02-15 14:08:33.000=
000000 +0100
> @@ -365,7 +365,7 @@ int mpc8xxx_spi_setup_transfer(struct sp
>
> =A0 =A0 =A0 =A0if ((mpc8xxx_spi->spibrg / hz) > 64) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0cs->hw_mode |=3D SPMODE_DIV16;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 pm =3D mpc8xxx_spi->spibrg / (hz * 64);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 pm =3D (mpc8xxx_spi->spibrg - 1) / (hz * 64=
) + 1;
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0WARN_ONCE(pm > 16, "%s: Requested speed is=
too low: %d Hz. "
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"Will use %d Hz instea=
d.\n", dev_name(&spi->dev),
> @@ -373,7 +373,7 @@ int mpc8xxx_spi_setup_transfer(struct sp
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (pm > 16)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pm =3D 16;
> =A0 =A0 =A0 =A0} else
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 pm =3D mpc8xxx_spi->spibrg / (hz * 4);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 pm =3D (mpc8xxx_spi->spibrg - 1) / (hz * 4)=
+ 1;
> =A0 =A0 =A0 =A0if (pm)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pm--;
>
>
>
>
> --
>
>
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [PATCH v4 04/11] mtd: Add MPC5121 NAND Flash Controller driver
From: Artem Bityutskiy @ 2010-02-16 8:11 UTC (permalink / raw)
To: Anatolij Gustschin
Cc: Piotr Ziecik, Detlev Zundel, linuxppc-dev, linux-mtd,
David Woodhouse
In-Reply-To: <1266255305-29224-1-git-send-email-agust@denx.de>
On Mon, 2010-02-15 at 18:35 +0100, Anatolij Gustschin wrote:
> Adds NAND Flash Controller driver for MPC5121 Revision 2.
> All device features, except hardware ECC and power management,
> are supported.
>
> Signed-off-by: Piotr Ziecik <kosmo@semihalf.com>
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> Acked-by: Grant Likely <grant.likely@secretlab.ca>
> Cc: <linux-mtd@lists.infradead.org>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: John Rigby <jcrigby@gmail.com>
> ---
> Changes since v3:
> - include 'asm/mpc5121.h' header instead of 'asm/mpc5xxx.h'.
> This change is needed because arch patch adding reset
> module definition was reworked and doesn't add mpc5121
> specific definitions to common header for mpc52xx/mpc5121.
>
> Changes since v2:
> - move the arch bits into separate patch
> (it is the next patch in this series now)
> - use __devinit/__devexit/__devexit_p and __devinitdata
>
> Changes since v1:
> - add logfile with changes since previous version
>
> Changes since the patch version submitted in May 2009:
>
> - move mpc5121_nfc.h to the driver .c as there is only one user
> - remove DRV_VERSION macro
> - replace printk() by dev_*()
> - drop unnecessary .suspend and .resume initializations
> - remove duplicate .name/.owner settings
> - fix mpc5121_nfc_init() to "return of_register_platform_driver(&mpc5121_nfc_driver);"
> - move module_init() to just below the init function
> - remove MODULE_VERSION
> - use "mtd: Add MPC5121 NAND Flash Controller driver" as the subject,
> previously it was "mpc5121: Added NAND Flash Controller driver.
>
> drivers/mtd/nand/Kconfig | 7 +
> drivers/mtd/nand/Makefile | 1 +
> drivers/mtd/nand/mpc5121_nfc.c | 916 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 924 insertions(+), 0 deletions(-)
> create mode 100644 drivers/mtd/nand/mpc5121_nfc.c
Pushed to my l2-mtd-2.6.git / dunno.
--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
^ permalink raw reply
* Re: [PATCH 6/6] powerpc: Use lwsync for acquire barrier if CPU supports it
From: Olof Johansson @ 2010-02-16 6:07 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: npiggin, linuxppc-dev, Anton Blanchard
In-Reply-To: <1266293943.16346.169.camel@pasglop>
On Tue, Feb 16, 2010 at 03:19:03PM +1100, Benjamin Herrenschmidt wrote:
> On Mon, 2010-02-15 at 22:22 -0600, Olof Johansson wrote:
> >
> > Turns out this one hurts PA6T performance quite a bit, lwsync seems to be
> > significantly more expensive there. I see a 25% drop in the microbenchmark
> > doing pthread_lock/unlock loops on two cpus.
> >
> > Taking out the CPU_FTR_LWSYNC will solve it, it's a bit unfortunate since
> > the sync->lwsync changes definitely still can, and should, be done.
>
> So we should use a different feature bit. No biggie. If needed we can
> split them more anyways.
Yeah, fine with me.
-Olof
^ permalink raw reply
* Re: [PATCH] [V5] net: emaclite: adding MDIO and phy lib support
From: David Miller @ 2010-02-16 5:53 UTC (permalink / raw)
To: grant.likely
Cc: Sadanand.Mutyala, netdev, linuxppc-dev, jgarzik, john.linn,
john.williams
In-Reply-To: <fa686aa41002111440x699b6b1ck97b14002ee9fe7b1@mail.gmail.com>
From: Grant Likely <grant.likely@secretlab.ca>
Date: Thu, 11 Feb 2010 15:40:24 -0700
> On Thu, Feb 11, 2010 at 3:12 PM, John Linn <john.linn@xilinx.com> wro=
te:
>> These changes add MDIO and phy lib support to the driver as the
>> IP core now supports the MDIO bus.
>>
>> The MDIO bus and phy are added as a child to the emaclite in the dev=
ice
>> tree as illustrated below.
>>
>> mdio {
>> =A0 =A0 =A0 =A0#address-cells =3D <1>;
>> =A0 =A0 =A0 =A0#size-cells =3D <0>;
>> =A0 =A0 =A0 =A0phy0: phy@7 {
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0compatible =3D "marvell,88e1111";
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0reg =3D <7>;
>> =A0 =A0 =A0 =A0} ;
>> }
>>
>> Signed-off-by: Sadanand Mutyala <Sadanand.Mutyala@xilinx.com>
>> Signed-off-by: John Linn <john.linn@xilinx.com>
> =
> Acked-by: Grant Likely <grant.likely@secretlab.ca>
Applied.
^ permalink raw reply
* Re: [PATCH 6/6] powerpc: Use lwsync for acquire barrier if CPU supports it
From: Benjamin Herrenschmidt @ 2010-02-16 4:19 UTC (permalink / raw)
To: Olof Johansson; +Cc: npiggin, linuxppc-dev, Anton Blanchard
In-Reply-To: <20100216042238.GB12167@lixom.net>
On Mon, 2010-02-15 at 22:22 -0600, Olof Johansson wrote:
>
> Turns out this one hurts PA6T performance quite a bit, lwsync seems to be
> significantly more expensive there. I see a 25% drop in the microbenchmark
> doing pthread_lock/unlock loops on two cpus.
>
> Taking out the CPU_FTR_LWSYNC will solve it, it's a bit unfortunate since
> the sync->lwsync changes definitely still can, and should, be done.
So we should use a different feature bit. No biggie. If needed we can
split them more anyways.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH 1/6] powerpc: Use lwarx hint in spinlocks
From: Olof Johansson @ 2010-02-16 4:16 UTC (permalink / raw)
To: Anton Blanchard; +Cc: npiggin, linuxppc-dev
In-Reply-To: <20100210105728.GA3399@kryten>
On Wed, Feb 10, 2010 at 09:57:28PM +1100, Anton Blanchard wrote:
> v2: We do this only for 64bit until we can verify all 32bit CPUs.
>
> Tested so far: 970 (thanks Ben), POWER5, POWER6, POWER7
> Still to test: RS64, POWER3, POWER4
Tested on PA6T as well, no performance impact (as expected).
-Olof
^ permalink raw reply
* Re: [PATCH 6/6] powerpc: Use lwsync for acquire barrier if CPU supports it
From: Olof Johansson @ 2010-02-16 4:22 UTC (permalink / raw)
To: Anton Blanchard; +Cc: npiggin, linuxppc-dev
In-Reply-To: <20100210111025.GF3399@kryten>
On Wed, Feb 10, 2010 at 10:10:25PM +1100, Anton Blanchard wrote:
>
> Nick Piggin discovered that lwsync barriers around locks were faster than isync
> on 970. That was a long time ago and I completely dropped the ball in testing
> his patches across other ppc64 processors.
>
> Turns out the idea helps on other chips. Using a microbenchmark that
> uses a lot of threads to contend on a global pthread mutex (and therefore a
> global futex), POWER6 improves 8% and POWER7 improves 2%. I checked POWER5
> and while I couldn't measure an improvement, there was no regression.
>
> This patch uses the lwsync patching code to replace the isyncs with lwsyncs
> on CPUs that support the instruction. We were marking POWER3 and RS64 as lwsync
> capable but in reality they treat it as a full sync (ie slow). Remove the
> CPU_FTR_LWSYNC bit from these CPUs so they continue to use the faster isync
> method.
>
> Signed-off-by: Anton Blanchard <anton@samba.org>
Turns out this one hurts PA6T performance quite a bit, lwsync seems to be
significantly more expensive there. I see a 25% drop in the microbenchmark
doing pthread_lock/unlock loops on two cpus.
Taking out the CPU_FTR_LWSYNC will solve it, it's a bit unfortunate since
the sync->lwsync changes definitely still can, and should, be done.
-Olof
^ permalink raw reply
* Re: [PATCH] hwmon: (ams) Fix device removal sequence
From: Christian Kujau @ 2010-02-16 1:33 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Jean Delvare, linuxppc-dev, Stelian Pop, Michael Hanselmann,
LM Sensors
In-Reply-To: <20100131150323.06e0c876@hyperion.delvare>
Ben,
I cannot find those two patches from Jean [0] in your tree, I take it
they'll be included and pushed to Linus in 2.6.34 then?
(Although I had hoped to see them in 2.6.33, I'm "testing"[1] them since
-rc2 on my PowerBook)
Thanks,
Christian.
[0] http://lists.ozlabs.org/pipermail/linuxppc-dev/2010-February/079958.html
http://lists.ozlabs.org/pipermail/linuxppc-dev/2010-February/079959.html
[1] http://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg40983.html
--
BOFH excuse #342:
HTTPD Error 4004 : very old Intel cpu - insufficient processing power
^ permalink raw reply
* Re: register long sp asm("r1") incorrect
From: H. Peter Anvin @ 2010-02-15 22:15 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: kernel list, linuxppc-dev, paulus, Pavel Machek
In-Reply-To: <1266267898.16346.135.camel@pasglop>
On 02/15/2010 01:04 PM, Benjamin Herrenschmidt wrote:
>
> It's true that most other use of it we have are global scope (local_paca
> in r13, glibc use of r2/r13, etc...) afaik, but since r1 itself is the
> stack pointer always, I think they pretty much guarantee it works.
>
It should work, because r1, being the stack pointer, is already marked a
reserved register in gcc.
The reference Pavel is citing bascially states that gcc won't globally
reserve the register, which is true, but it is already reserved anyway.
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
^ permalink raw reply
* Re: register long sp asm("r1") incorrect
From: Benjamin Herrenschmidt @ 2010-02-15 21:04 UTC (permalink / raw)
To: Pavel Machek; +Cc: linuxppc-dev, paulus, kernel list
In-Reply-To: <20100215202854.GA21601@elf.ucw.cz>
On Mon, 2010-02-15 at 21:28 +0100, Pavel Machek wrote:
> On Tue 2010-02-16 06:59:52, Benjamin Herrenschmidt wrote:
> > On Mon, 2010-02-15 at 08:34 +0100, Pavel Machek wrote:
> > > > On Tue, 2010-02-09 at 16:24 +0100, Pavel Machek wrote:
> > > > > ...according to gcc docs, sp should be global, or placement in
> > > > > register is not guaranteed (except at asm boundaries, but there
> > > are
> > > > > none).
> > > >
> > > > Sorry I'm not sure I grok what you mean.
> > >
> > > Well, according to gcc doscs and my experience, local "register int
> > > __asm()" variables only work by accident (or not at all).
> >
> > Hrm... we definitely rely on that for our thread_info() access, and so
> > far it has worked well for us, but I'll poke our gcc folks just in case.
>
> Thanks, and let me know about any results.
All the gcc folks I talked to say something along the lines that there
is no way in hell it doesn't work :-)
It's true that most other use of it we have are global scope (local_paca
in r13, glibc use of r2/r13, etc...) afaik, but since r1 itself is the
stack pointer always, I think they pretty much guarantee it works.
I'm CCing a couple of experts just to be sure.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH 3/3] of/gpio: Introduce of_put_gpio(), add ref counting for OF GPIO chips
From: Anton Vorontsov @ 2010-02-15 20:59 UTC (permalink / raw)
To: Grant Likely
Cc: Michal Simek, David Brownell, devicetree-discuss, linux-kernel,
linuxppc-dev, microblaze-uclinux, David Miller
In-Reply-To: <fa686aa41002151149u158bc2fbn5af16f6bd714d36c@mail.gmail.com>
On Mon, Feb 15, 2010 at 12:49:56PM -0700, Grant Likely wrote:
[...]
> Okay, I'm convinced now. The model is wrong. struct of_gc does need
> to be a member of struct gpio_chip and conditionally compiled against
> CONFIG_OF_GPIO. This locking requirement is just too plain ugly,
And how would you implement of_get_gpio(np, index) then?
You don't have 'struct gpio_chip' in of_get_gpio(), so you can't
get of_gc out of it.
It's possible to lookup all GPIOs (gpio[0..ARCH_NR_GPIOS]) matching
on chip->dev->archdata.node == np, but you're just moving this stuff
into gpiolib, where you'll have to grab global gpio_lock instead of
devtree lock. And just as with np->data_lock or global devtree lock,
you'll have to grab gpio_lock in of_gpio_put(), of_gpio_chip_register
and of_gpio_chip_unregister().
See? Your suggestion doesn't make things better or simpler, instead
it turns the OF GPIO inside out, exposing arch details to the generic
code.
There is really no excuse for the arch-specific (OF) stuff being in
the generic code. Not in the irq subsystem, not in the gpio
subsystem. My implementation actually proves that.
> and
> dereferencing the np->data pointer in this way is dangerous
> (what if
> something that is not struct of_gc is stored there).
Not possible with the safe np->data accessors.
--
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2
^ permalink raw reply
* Re: [PATCH v4 02/11] powerpc/mpc5121: Add machine restart support
From: Wolfram Sang @ 2010-02-15 20:58 UTC (permalink / raw)
To: Anatolij Gustschin; +Cc: linuxppc-dev, Piotr Ziecik, dzu, Wolfgang Denk
In-Reply-To: <1266252693-28530-1-git-send-email-agust@denx.de>
[-- Attachment #1: Type: text/plain, Size: 627 bytes --]
On Mon, Feb 15, 2010 at 05:51:33PM +0100, Anatolij Gustschin wrote:
> Add reset module registers representation and
> machine restart callback for mpc5121 platform.
>
> Signed-off-by: Piotr Ziecik <kosmo@semihalf.com>
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: John Rigby <jcrigby@gmail.com>
> ---
Reviewed-by: Wolfram Sang <w.sang@pengutronix.de>
--
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: register long sp asm("r1") incorrect
From: Pavel Machek @ 2010-02-15 20:28 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, paulus, kernel list
In-Reply-To: <1266263992.16346.125.camel@pasglop>
On Tue 2010-02-16 06:59:52, Benjamin Herrenschmidt wrote:
> On Mon, 2010-02-15 at 08:34 +0100, Pavel Machek wrote:
> > > On Tue, 2010-02-09 at 16:24 +0100, Pavel Machek wrote:
> > > > ...according to gcc docs, sp should be global, or placement in
> > > > register is not guaranteed (except at asm boundaries, but there
> > are
> > > > none).
> > >
> > > Sorry I'm not sure I grok what you mean.
> >
> > Well, according to gcc doscs and my experience, local "register int
> > __asm()" variables only work by accident (or not at all).
>
> Hrm... we definitely rely on that for our thread_info() access, and so
> far it has worked well for us, but I'll poke our gcc folks just in case.
Thanks, and let me know about any results.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply
* Re: register long sp asm("r1") incorrect
From: Benjamin Herrenschmidt @ 2010-02-15 19:59 UTC (permalink / raw)
To: Pavel Machek; +Cc: linuxppc-dev, paulus, kernel list
In-Reply-To: <20100215073405.GB20456@atrey.karlin.mff.cuni.cz>
On Mon, 2010-02-15 at 08:34 +0100, Pavel Machek wrote:
> > On Tue, 2010-02-09 at 16:24 +0100, Pavel Machek wrote:
> > > ...according to gcc docs, sp should be global, or placement in
> > > register is not guaranteed (except at asm boundaries, but there
> are
> > > none).
> >
> > Sorry I'm not sure I grok what you mean.
>
> Well, according to gcc doscs and my experience, local "register int
> __asm()" variables only work by accident (or not at all).
Hrm... we definitely rely on that for our thread_info() access, and so
far it has worked well for us, but I'll poke our gcc folks just in case.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH 3/3] of/gpio: Introduce of_put_gpio(), add ref counting for OF GPIO chips
From: Grant Likely @ 2010-02-15 19:49 UTC (permalink / raw)
To: avorontsov
Cc: Michal Simek, David Brownell, devicetree-discuss, linux-kernel,
linuxppc-dev, microblaze-uclinux, David Miller
In-Reply-To: <20100209191427.GA18263@oksana.dev.rtsoft.ru>
On Tue, Feb 9, 2010 at 12:14 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Tue, Feb 09, 2010 at 10:28:15AM -0700, Grant Likely wrote:
> [...]
>> Rather than having a lock at the device tree data pointer level which
>> mixes usage with potentially many other drivers; wouldn't it make more
>> sense to use a mutex at the of_gc subsystem context?
>
> I don't think so.
>
> of_gc = np->data;
> lock(of_gc); (or lock(devtree))
> <do something with of_gc>
>
> doesn't provide us what we need, i.e. it doesn't guarantee that
> np->data (of_gc) is still alive.
>
> And here:
>
> lock(np->data); (or lock(devtree))
> of_gc = np->data;
> lock(of_gc);
> <do something with of_gc>
>
> The second lock becomes useless (unless you also refcount np->data
> usage and can drop the devtree/np->data lock, and grab some other
> kind of lock, e.g. mutex, but this is silly).
Okay, I'm convinced now. The model is wrong. struct of_gc does need
to be a member of struct gpio_chip and conditionally compiled against
CONFIG_OF_GPIO. This locking requirement is just too plain ugly, and
dereferencing the np->data pointer in this way is dangerous (what if
something that is not struct of_gc is stored there).
Put of_gc into struct gpio_chip, and I'll completely support that approach.
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ 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