[PATCH 0/2] mpc5200 ac97 gpio reset

[PATCH 0/2] mpc5200 ac97 gpio reset

[Eric Millbrandt]

These patches reimplement the reset fuction in the ac97 to use gpio pins
instead of using the mpc5200 ac97 reset functionality in the psc.  This
avoids a problem in which attached ac97 devices go into "test" mode appear
unresponsive.

These patches were tested on a pcm030 baseboard and on custom hardware with
a wm9715 audio codec/touchscreen controller.

Eric Millbrandt

---

Eric Millbrandt (2):
    powerpc/5200: Export port-config
    sound/soc: mpc5200_psc_ac97: Use gpio pins for cold reset.

 arch/powerpc/boot/dts/lite5200.dts           |    3 +
 arch/powerpc/boot/dts/lite5200b.dts          |    3 +
 arch/powerpc/boot/dts/pcm030.dts             |    3 +
 arch/powerpc/boot/dts/pcm032.dts             |    3 +
 arch/powerpc/include/asm/mpc52xx.h           |    2 +
 arch/powerpc/platforms/52xx/mpc52xx_common.c |   61 +++++++++++++++++++
 sound/soc/fsl/mpc5200_dma.h                  |    5 ++
 sound/soc/fsl/mpc5200_psc_ac97.c             |   83 ++++++++++++++++++++++=
++-
 8 files changed, 159 insertions(+), 4 deletions(-)

-DISCLAIMER: an automatically appended disclaimer may follow. By posting-
-to a public e-mail mailing list I hereby grant permission to distribute-
-and copy this message.-


This e-mail and the information, including any attachments, it contains are=
 intended to be a confidential communication only to the person or entity t=
o whom it is addressed and may contain information that is privileged. If t=
he reader of this message is not the intended recipient, you are hereby not=
ified that any dissemination, distribution or copying of this communication=
 is strictly prohibited. If you have received this communication in error, =
please immediately notify the sender and destroy the original message.

Thank you.

Please consider the environment before printing this email.

[PATCH 1/2] powerpc/5200: Export port-config

[Eric Millbrandt]

Allow device drivers to safely modify port-config.  This allows device driv=
ers
access to gpio pins to manually bit-bang slave devices.

Signed-off-by: Eric Millbrandt <emillbrandt@dekaresearch.com>
---
 arch/powerpc/include/asm/mpc52xx.h           |    2 +
 arch/powerpc/platforms/52xx/mpc52xx_common.c |   61 ++++++++++++++++++++++=
++++
 2 files changed, 63 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/include/asm/mpc52xx.h b/arch/powerpc/include/asm/=
mpc52xx.h
index b664ce7..ebc5a3b 100644
--- a/arch/powerpc/include/asm/mpc52xx.h
+++ b/arch/powerpc/include/asm/mpc52xx.h
@@ -274,7 +274,9 @@ extern void mpc52xx_declare_of_platform_devices(void);
 extern void mpc52xx_map_common_devices(void);
 extern int mpc52xx_set_psc_clkdiv(int psc_id, int clkdiv);
 extern unsigned int mpc52xx_get_xtal_freq(struct device_node *node);
+extern unsigned int mpc52xx_read_port_config(void);
 extern void mpc52xx_restart(char *cmd);
+extern int mpc52xx_write_port_config(u32 mux, int operation);

 /* mpc52xx_gpt.c */
 struct mpc52xx_gpt_priv;
diff --git a/arch/powerpc/platforms/52xx/mpc52xx_common.c b/arch/powerpc/pl=
atforms/52xx/mpc52xx_common.c
index a46bad0..77c685a 100644
--- a/arch/powerpc/platforms/52xx/mpc52xx_common.c
+++ b/arch/powerpc/platforms/52xx/mpc52xx_common.c
@@ -37,6 +37,11 @@ static struct of_device_id mpc52xx_bus_ids[] __initdata =
=3D {
        {}
 };

+static struct of_device_id mpc52xx_gpio_simple[] =3D {
+       { .compatible =3D "fsl,mpc5200-gpio", },
+       {}
+};
+
 /*
  * This variable is mapped in mpc52xx_map_wdt() and used in mpc52xx_restar=
t().
  * Permanent mapping is required because mpc52xx_restart() can be called
@@ -82,6 +87,15 @@ mpc5200_setup_xlb_arbiter(void)
        iounmap(xlb);
 }

+/*
+ * This variable is mapped in mpc52xx_write_port_config() and
+ * mpc52xx_read_port_config().  Permanent mapping is required because
+ * mpc52xx_map_and_lock_port_config() can be called from interrupt context
+ * while node mapping (which calls ioremap()) cannot be used at such point=
.
+ */
+static DEFINE_SPINLOCK(mpc52xx_port_config_lock);
+static u32 __iomem *port_config;
+
 /**
  * mpc52xx_declare_of_platform_devices: register internal devices and chil=
dren
  *                                     of the localplus bus to the of_plat=
form
@@ -117,6 +131,7 @@ void __init
 mpc52xx_map_common_devices(void)
 {
        struct device_node *np;
+       const u32 *regaddr;

        /* mpc52xx_wdt is mapped here and used in mpc52xx_restart,
         * possibly from a interrupt context. wdt is only implement
@@ -135,6 +150,13 @@ mpc52xx_map_common_devices(void)
        np =3D of_find_matching_node(NULL, mpc52xx_cdm_ids);
        mpc52xx_cdm =3D of_iomap(np, 0);
        of_node_put(np);
+
+       /* port_config register */
+       np =3D of_find_matching_node(NULL, mpc52xx_gpio_simple);
+       regaddr =3D of_get_address(np, 0, NULL, NULL);
+       port_config =3D ioremap((u32) of_translate_address(np, regaddr), 0x=
4);
+
+       of_node_put(np);
 }

 /**
@@ -233,3 +255,42 @@ mpc52xx_restart(char *cmd)

        while (1);
 }
+
+/**
+ * mpc52xx_write_port_config: Allow mutually exclusive access to the
+ * port_config register
+ *
+ * @newmux: value to write to port_config
+ * @operation: set to 0 for | or 1 for &
+ */
+int mpc52xx_write_port_config(u32 newmux, int operation)
+{
+       unsigned long flags;
+       u32 mux;
+
+       if (!port_config)
+               return -ENODEV;
+
+       spin_lock_irqsave(&mpc52xx_port_config_lock, flags);
+       mux =3D in_be32(port_config);
+       if (operation)
+               out_be32(port_config, mux & newmux);
+       else
+               out_be32(port_config, mux | newmux);
+       spin_unlock_irqrestore(&mpc52xx_port_config_lock, flags);
+
+       return 0;
+}
+EXPORT_SYMBOL(mpc52xx_write_port_config);
+
+/**
+ * mpc52xx_read_port_config: Return the value of port_config
+ */
+unsigned int mpc52xx_read_port_config(void)
+{
+       if (!port_config)
+               return -ENODEV;
+
+       return in_be32(port_config);
+}
+EXPORT_SYMBOL(mpc52xx_read_port_config);
--
1.6.3.1


This e-mail and the information, including any attachments, it contains are=
 intended to be a confidential communication only to the person or entity t=
o whom it is addressed and may contain information that is privileged. If t=
he reader of this message is not the intended recipient, you are hereby not=
ified that any dissemination, distribution or copying of this communication=
 is strictly prohibited. If you have received this communication in error, =
please immediately notify the sender and destroy the original message.

Thank you.

Please consider the environment before printing this email.

[PATCH 2/2] sound/soc: mpc5200_psc_ac97: Use gpio pins for cold reset.

[Eric Millbrandt]

The implementation of the ac97 "cold" reset is flawed.  If the sync and
output lines are high when reset is asserted the attached ac97 device
may go into test mode.  Avoid this by reconfiguring the psc to gpio mode
and generating the reset manually.

Signed-off-by: Eric Millbrandt <emillbrandt@dekaresearch.com>
---
 arch/powerpc/boot/dts/lite5200.dts  |    3 +
 arch/powerpc/boot/dts/lite5200b.dts |    3 +
 arch/powerpc/boot/dts/pcm030.dts    |    3 +
 arch/powerpc/boot/dts/pcm032.dts    |    3 +
 sound/soc/fsl/mpc5200_dma.h         |    5 ++
 sound/soc/fsl/mpc5200_psc_ac97.c    |   83 +++++++++++++++++++++++++++++++=
++--
 6 files changed, 96 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/boot/dts/lite5200.dts b/arch/powerpc/boot/dts/lit=
e5200.dts
index 82ff2b1..cb4e49b 100644
--- a/arch/powerpc/boot/dts/lite5200.dts
+++ b/arch/powerpc/boot/dts/lite5200.dts
@@ -180,6 +180,9 @@
                //      compatible =3D "fsl,mpc5200-psc-ac97";
                //      cell-index =3D <1>;
                //      reg =3D <0x2200 0x100>;
+               //      gpios =3D <&gpio 7 0              /* AC97_1_RES */
+               //               &gpio_simple 29 0      /* AC97_1_SYNC */
+               //               &gpio_simple 31 0>;    /* AC97_1_SDATA_OUT=
 */
                //      interrupts =3D <2 2 0>;
                //};

diff --git a/arch/powerpc/boot/dts/lite5200b.dts b/arch/powerpc/boot/dts/li=
te5200b.dts
index e45a63b..1fb0ac7 100644
--- a/arch/powerpc/boot/dts/lite5200b.dts
+++ b/arch/powerpc/boot/dts/lite5200b.dts
@@ -184,6 +184,9 @@
                //      compatible =3D "fsl,mpc5200b-psc-ac97","fsl,mpc5200=
-psc-ac97";
                //      cell-index =3D <1>;
                //      reg =3D <0x2200 0x100>;
+               //      gpios =3D <&gpio 7 0              /* AC97_1_RES */
+               //               &gpio_simple 29 0      /* AC97_1_SYNC */
+               //               &gpio_simple 31 0>;    /* AC97_1_SDATA_OUT=
 */
                //      interrupts =3D <2 2 0>;
                //};

diff --git a/arch/powerpc/boot/dts/pcm030.dts b/arch/powerpc/boot/dts/pcm03=
0.dts
index 8a4ec30..0085e0f 100644
--- a/arch/powerpc/boot/dts/pcm030.dts
+++ b/arch/powerpc/boot/dts/pcm030.dts
@@ -189,6 +189,9 @@
                        compatible =3D "mpc5200b-psc-ac97","fsl,mpc5200b-ps=
c-ac97";
                        cell-index =3D <0>;
                        reg =3D <0x2000 0x100>;
+                       gpios =3D <&gpio 7 0              /* AC97_1_RES */
+                                &gpio_simple 29 0      /* AC97_1_SYNC */
+                                &gpio_simple 31 0>;    /* AC97_1_SDATA_OUT=
 */
                        interrupts =3D <2 1 0>;
                };

diff --git a/arch/powerpc/boot/dts/pcm032.dts b/arch/powerpc/boot/dts/pcm03=
2.dts
index 85d857a..76f86d3 100644
--- a/arch/powerpc/boot/dts/pcm032.dts
+++ b/arch/powerpc/boot/dts/pcm032.dts
@@ -189,6 +189,9 @@
                        compatible =3D "fsl,mpc5200b-psc-ac97","fsl,mpc5200=
-psc-ac97";
                        cell-index =3D <0>;
                        reg =3D <0x2000 0x100>;
+                       gpios =3D <&gpio 7 0              /* AC97_1_RES */
+                                &gpio_simple 29 0      /* AC97_1_SYNC */
+                                &gpio_simple 31 0>;    /* AC97_1_SDATA_OUT=
 */
                        interrupts =3D <2 1 0>;
                };

diff --git a/sound/soc/fsl/mpc5200_dma.h b/sound/soc/fsl/mpc5200_dma.h
index 22208b3..9fb0248 100644
--- a/sound/soc/fsl/mpc5200_dma.h
+++ b/sound/soc/fsl/mpc5200_dma.h
@@ -61,6 +61,11 @@ struct psc_dma {
        int id;
        unsigned int slots;

+       /* gpio pins locations for cold reset */
+       int reset_gpio;
+       int sync_gpio;
+       int out_gpio;
+
        /* per-stream data */
        struct psc_dma_stream playback;
        struct psc_dma_stream capture;
diff --git a/sound/soc/fsl/mpc5200_psc_ac97.c b/sound/soc/fsl/mpc5200_psc_a=
c97.c
index e2ee220..bbbf860 100644
--- a/sound/soc/fsl/mpc5200_psc_ac97.c
+++ b/sound/soc/fsl/mpc5200_psc_ac97.c
@@ -9,8 +9,10 @@
  * published by the Free Software Foundation.
  */

+#include <linux/gpio.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
+#include <linux/of_gpio.h>
 #include <linux/of_platform.h>
 #include <linux/delay.h>

@@ -20,12 +22,17 @@

 #include <asm/time.h>
 #include <asm/delay.h>
+#include <asm/mpc52xx.h>
 #include <asm/mpc52xx_psc.h>

 #include "mpc5200_dma.h"
 #include "mpc5200_psc_ac97.h"

 #define DRV_NAME "mpc5200-psc-ac97"
+#define MPC52xx_GPIO_PSC1_MASK 0x7
+#define MPC52xx_GPIO_PSC2_MASK (0x7<<4)
+#define MPC52xx_AC97_PSC1 0x2
+#define MPC52xx_AC97_PSC2 (0x2<<4)

 /* ALSA only supports a single AC97 device so static is recommend here */
 static struct psc_dma *psc_dma;
@@ -100,19 +107,69 @@ static void psc_ac97_warm_reset(struct snd_ac97 *ac97=
)
 {
        struct mpc52xx_psc __iomem *regs =3D psc_dma->psc_regs;

+       mutex_lock(&psc_dma->mutex);
+
        out_be32(&regs->sicr, psc_dma->sicr | MPC52xx_PSC_SICR_AWR);
        udelay(3);
        out_be32(&regs->sicr, psc_dma->sicr);
+
+       mutex_unlock(&psc_dma->mutex);
 }

 static void psc_ac97_cold_reset(struct snd_ac97 *ac97)
 {
        struct mpc52xx_psc __iomem *regs =3D psc_dma->psc_regs;
+       u32 gpio_mux;
+
+       mutex_lock(&psc_dma->mutex);
+
+       /* Reconfigure pin-muxing to gpio */
+       switch (psc_dma->id) {
+       case 0:
+               gpio_mux =3D MPC52xx_GPIO_PSC1_MASK; break;
+       case 1:
+               gpio_mux =3D MPC52xx_GPIO_PSC2_MASK; break;
+       default:
+               dev_err(psc_dma->dev,
+                       "Unable to determine PSC, no cold-reset will be "
+                       "performed\n");
+               return;
+       }
+
+       dev_err(psc_dma->dev, "cold reset\n");
+       mpc52xx_write_port_config(~gpio_mux, 1);
+
+       /* Assert cold reset */
+       gpio_direction_output(psc_dma->sync_gpio, 0);
+       gpio_direction_output(psc_dma->out_gpio, 0);
+       gpio_direction_output(psc_dma->reset_gpio, 0);
+
+       /* Notify the PSC that a cold reset is occurring */
+       out_be32(&regs->sicr, 0);
+       udelay(2);
+
+       /* Deassert reset */
+       gpio_direction_output(psc_dma->reset_gpio, 1);
+       msleep(1);
+
+       /* Restore pin-muxing */
+       switch (psc_dma->id) {
+       case 0:
+               gpio_mux =3D MPC52xx_AC97_PSC1; break;
+       case 1:
+               gpio_mux =3D MPC52xx_AC97_PSC2; break;
+       default:
+               return;
+       }
+
+       mpc52xx_write_port_config(gpio_mux, 0);
+
+       /* Restore the serial interface mode to AC97 */
+       out_be32(&regs->sicr, psc_dma->sicr);
+       out_8(&regs->command, MPC52xx_PSC_TX_ENABLE | MPC52xx_PSC_RX_ENABLE=
);
+
+       mutex_unlock(&psc_dma->mutex);

-       /* Do a cold reset */
-       out_8(&regs->op1, MPC52xx_PSC_OP_RES);
-       udelay(10);
-       out_8(&regs->op0, MPC52xx_PSC_OP_RES);
        msleep(1);
        psc_ac97_warm_reset(ac97);
 }
@@ -287,6 +344,24 @@ static int __devinit psc_ac97_of_probe(struct of_devic=
e *op,
        regs =3D psc_dma->psc_regs;
        ac97.private_data =3D psc_dma;

+       psc_dma->reset_gpio =3D of_get_gpio_flags(op->node, 0, NULL);
+       psc_dma->sync_gpio =3D of_get_gpio_flags(op->node, 1, NULL);
+       psc_dma->out_gpio =3D of_get_gpio_flags(op->node, 2, NULL);
+       if ((psc_dma->reset_gpio < 0) ||
+           (psc_dma->sync_gpio < 0) ||
+           (psc_dma->out_gpio < 0)) {
+               dev_err(&op->dev, "error: cannot get GPIO pins; "
+                       "reset=3D%i sync=3D%i out=3D%i\n",
+                       psc_dma->reset_gpio,
+                       psc_dma->sync_gpio,
+                       psc_dma->out_gpio);
+               return -ENODEV;
+       }
+
+       gpio_request(psc_dma->reset_gpio, "psc_dma-reset");
+       gpio_request(psc_dma->sync_gpio, "psc_dma-sync");
+       gpio_request(psc_dma->out_gpio, "psc_dma-out");
+
        for (i =3D 0; i < ARRAY_SIZE(psc_ac97_dai); i++)
                psc_ac97_dai[i].private_data =3D psc_dma;

--
1.6.3.1


This e-mail and the information, including any attachments, it contains are=
 intended to be a confidential communication only to the person or entity t=
o whom it is addressed and may contain information that is privileged. If t=
he reader of this message is not the intended recipient, you are hereby not=
ified that any dissemination, distribution or copying of this communication=
 is strictly prohibited. If you have received this communication in error, =
please immediately notify the sender and destroy the original message.

Thank you.

Please consider the environment before printing this email.

Re: [PATCH 2/2] sound/soc: mpc5200_psc_ac97: Use gpio pins for cold reset.

[Mark Brown]

On Tue, Jun 08, 2010 at 12:46:02PM -0400, Eric Millbrandt wrote:

> +       switch (psc_dma->id) {
> +       case 0:
> +               gpio_mux = MPC52xx_GPIO_PSC1_MASK; break;
> +       case 1:
> +               gpio_mux = MPC52xx_GPIO_PSC2_MASK; break;

Please don't place the break on the same line as the statement, it's
not good for legibility.

> +       dev_err(psc_dma->dev, "cold reset\n");

You shouldn't be logging this as an error, doing a cold reset is
perfectly normal.

Re: [PATCH 1/2] powerpc/5200: Export port-config

[Grant Likely]

On Tue, Jun 8, 2010 at 10:46 AM, Eric Millbrandt
<emillbrandt@dekaresearch.com> wrote:
> Allow device drivers to safely modify port-config. =A0This allows device =
drivers
> access to gpio pins to manually bit-bang slave devices.
>
> Signed-off-by: Eric Millbrandt <emillbrandt@dekaresearch.com>

Unfortunately, still too much exposure of port-config to drivers for
my liking.  I'd be happier with an API that requests the mode for a
specific pin group instead of directly masking & setting bits....

but of course I've also just said in my other email that I'd be happy
with all of the GPIO AC97reset logic being contained in
arch/powerpc/platforms/52xx which kind of makes the comment moot.  :-)

g.

> ---
> =A0arch/powerpc/include/asm/mpc52xx.h =A0 =A0 =A0 =A0 =A0 | =A0 =A02 +
> =A0arch/powerpc/platforms/52xx/mpc52xx_common.c | =A0 61 ++++++++++++++++=
++++++++++
> =A02 files changed, 63 insertions(+), 0 deletions(-)

Re: [PATCH 0/2] mpc5200 ac97 gpio reset

[Jon Smirl]

On Tue, Jun 8, 2010 at 12:46 PM, Eric Millbrandt
<emillbrandt@dekaresearch.com> wrote:
> These patches reimplement the reset fuction in the ac97 to use gpio pins
> instead of using the mpc5200 ac97 reset functionality in the psc. =A0This
> avoids a problem in which attached ac97 devices go into "test" mode appea=
r
> unresponsive.

I'm aware of this problem but I was unable to solve it when I first
wrote the driver. I didn't have access to a scope so I wasn't able to
figure out what was causing the lock up.  I worked around it by
looping in reset until the chip did actually reset, but Mark wouldn't
let me submit that code.

A while ago Maximilian Mueth <maximilian.mueth@mp-ndt.de> sent me a
note saying another solution was to configure the PSC into AC97 mode
in uboot. His impression was that uboot was setting PSC into its
default mode. Then Linux booted and set the PSC into AC97 mode. The
transition from default mode into AC97 mode caused glitches on the
AC97 reset pins and triggered test mode.

Would making a change in uboot be a better solution? Eric, can you
verify if changing uboot also fixes the problem?

I'm glad were finally getting to the root cause of the problem.


>
> These patches were tested on a pcm030 baseboard and on custom hardware wi=
th
> a wm9715 audio codec/touchscreen controller.
>
> Eric Millbrandt
>
> ---
>
> Eric Millbrandt (2):
> =A0 =A0powerpc/5200: Export port-config
> =A0 =A0sound/soc: mpc5200_psc_ac97: Use gpio pins for cold reset.
>
> =A0arch/powerpc/boot/dts/lite5200.dts =A0 =A0 =A0 =A0 =A0 | =A0 =A03 +
> =A0arch/powerpc/boot/dts/lite5200b.dts =A0 =A0 =A0 =A0 =A0| =A0 =A03 +
> =A0arch/powerpc/boot/dts/pcm030.dts =A0 =A0 =A0 =A0 =A0 =A0 | =A0 =A03 +
> =A0arch/powerpc/boot/dts/pcm032.dts =A0 =A0 =A0 =A0 =A0 =A0 | =A0 =A03 +
> =A0arch/powerpc/include/asm/mpc52xx.h =A0 =A0 =A0 =A0 =A0 | =A0 =A02 +
> =A0arch/powerpc/platforms/52xx/mpc52xx_common.c | =A0 61 ++++++++++++++++=
+++
> =A0sound/soc/fsl/mpc5200_dma.h =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| =A0 =
=A05 ++
> =A0sound/soc/fsl/mpc5200_psc_ac97.c =A0 =A0 =A0 =A0 =A0 =A0 | =A0 83 ++++=
++++++++++++++++++++-
> =A08 files changed, 159 insertions(+), 4 deletions(-)
>
> -DISCLAIMER: an automatically appended disclaimer may follow. By posting-
> -to a public e-mail mailing list I hereby grant permission to distribute-
> -and copy this message.-
>
>
> This e-mail and the information, including any attachments, it contains a=
re intended to be a confidential communication only to the person or entity=
 to whom it is addressed and may contain information that is privileged. If=
 the reader of this message is not the intended recipient, you are hereby n=
otified that any dissemination, distribution or copying of this communicati=
on is strictly prohibited. If you have received this communication in error=
, please immediately notify the sender and destroy the original message.
>
> Thank you.
>
> Please consider the environment before printing this email.
>



--=20
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH 0/2] mpc5200 ac97 gpio reset

[Wolfgang Denk]

Dear Jon Smirl,

In message <AANLkTimIs90kR5uqhdBQ02oSd94dn08sOITm51Ylrl4-@mail.gmail.com> you wrote:
>
> Would making a change in uboot be a better solution? Eric, can you
> verify if changing uboot also fixes the problem?

To me it seems better if the driver itself does what needs to be done
instead of relying on specific settings that may or may not be done in
U-Boot. Keep in mind that drivers may be loaded as modules, and that
we even see cases where the same port serves multiple purposes by
loading different driver modules (yes, this is not exactly a clever
idea, but hardware designers come up with such solutions).


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
The typical page layout program is nothing more  than  an  electronic
light table for cutting and pasting documents.

Re: [PATCH 0/2] mpc5200 ac97 gpio reset

[Mark Brown]

On Wed, Jun 09, 2010 at 08:13:30AM +0200, Wolfgang Denk wrote:
> In message <AANLkTimIs90kR5uqhdBQ02oSd94dn08sOITm51Ylrl4-@mail.gmail.com> you wrote:

> > Would making a change in uboot be a better solution? Eric, can you
> > verify if changing uboot also fixes the problem?

> To me it seems better if the driver itself does what needs to be done
> instead of relying on specific settings that may or may not be done in
> U-Boot. Keep in mind that drivers may be loaded as modules, and that
> we even see cases where the same port serves multiple purposes by
> loading different driver modules (yes, this is not exactly a clever
> idea, but hardware designers come up with such solutions).

I do tend to agree that having the driver be able to cope with things is
a bit more robust - it's not terribly discoverable for users and people
are often justifiably nervous about updating their bootloader.

It might, however, be sensible to make the GPIO based reset be optional
based on having the OF data for the GPIOs.  That way existing DTs will
work without changes and systems that can use the reset implementation
in the controller will be able to do so.

RE: [PATCH 0/2] mpc5200 ac97 gpio reset

[Eric Millbrandt]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
> Sent: Wednesday, June 09, 2010 06:31
> To: Wolfgang Denk
> Cc: Jon Smirl; Eric Millbrandt; linuxppc-dev@lists.ozlabs.org
> Subject: Re: [PATCH 0/2] mpc5200 ac97 gpio reset
--snip--
> It might, however, be sensible to make the GPIO based reset be optional
> based on having the OF data for the GPIOs.  That way existing DTs will
> work without changes and systems that can use the reset implementation
> in the controller will be able to do so.

This is a reasonable request.  I will respin the patch to do this.

Eric Millbrandt

This e-mail and the information, including any attachments, it contains are=
 intended to be a confidential communication only to the person or entity t=
o whom it is addressed and may contain information that is privileged. If t=
he reader of this message is not the intended recipient, you are hereby not=
ified that any dissemination, distribution or copying of this communication=
 is strictly prohibited. If you have received this communication in error, =
please immediately notify the sender and destroy the original message.

Thank you.

Please consider the environment before printing this email.

Re: [PATCH 0/2] mpc5200 ac97 gpio reset

[Grant Likely]

On Wed, Jun 9, 2010 at 4:30 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Jun 09, 2010 at 08:13:30AM +0200, Wolfgang Denk wrote:
>> In message <AANLkTimIs90kR5uqhdBQ02oSd94dn08sOITm51Ylrl4-@mail.gmail.com=
> you wrote:
>
>> > Would making a change in uboot be a better solution? Eric, can you
>> > verify if changing uboot also fixes the problem?
>
>> To me it seems better if the driver itself does what needs to be done
>> instead of relying on specific settings that may or may not be done in
>> U-Boot. Keep in mind that drivers may be loaded as modules, and that
>> we even see cases where the same port serves multiple purposes by
>> loading different driver modules (yes, this is not exactly a clever
>> idea, but hardware designers come up with such solutions).
>
> I do tend to agree that having the driver be able to cope with things is
> a bit more robust - it's not terribly discoverable for users and people
> are often justifiably nervous about updating their bootloader.
>
> It might, however, be sensible to make the GPIO based reset be optional
> based on having the OF data for the GPIOs. =A0That way existing DTs will
> work without changes and systems that can use the reset implementation
> in the controller will be able to do so.

To me, this seems firmly in the realm of silicon bug workaround.
Considering that the pins aren't relocatable (ie, the GPIO numbers
never change), I've got no problem having the GPIO reset logic added
to arch/powerpc/platforms/52xx and hard coding the GPIO numbers.

I completely agree that it should not require a firmware update to get
the hardware to work correctly.

g.

Re: [PATCH 0/2] mpc5200 ac97 gpio reset

[Mark Brown]

On 10 Jun 2010, at 23:07, Grant Likely wrote:

> To me, this seems firmly in the realm of silicon bug workaround.
> Considering that the pins aren't relocatable (ie, the GPIO numbers
> never change), I've got no problem having the GPIO reset logic added
> to arch/powerpc/platforms/52xx and hard coding the GPIO numbers.

Since the pins are fixed I agree - I was assuming that the DT updates
were there because there were multiple pin mux options for the AC'97
controller so we needed the DT to say which were in use on the board.

Re: [PATCH 0/2] mpc5200 ac97 gpio reset

[Grant Likely]

On Thu, Jun 10, 2010 at 5:14 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On 10 Jun 2010, at 23:07, Grant Likely wrote:
>
>> To me, this seems firmly in the realm of silicon bug workaround.
>> Considering that the pins aren't relocatable (ie, the GPIO numbers
>> never change), I've got no problem having the GPIO reset logic added
>> to arch/powerpc/platforms/52xx and hard coding the GPIO numbers.
>
> Since the pins are fixed I agree - I was assuming that the DT updates
> were there because there were multiple pin mux options for the AC'97
> controller so we needed the DT to say which were in use on the board.

:-)  I originally also made the same assumption, until I actually went
and looked at the data sheet.

g.

Re: [PATCH 0/2] mpc5200 ac97 gpio reset

[Jon Smirl]

On Wed, Jun 9, 2010 at 2:13 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Jon Smirl,
>
> In message <AANLkTimIs90kR5uqhdBQ02oSd94dn08sOITm51Ylrl4-@mail.gmail.com>=
 you wrote:
>>
>> Would making a change in uboot be a better solution? Eric, can you
>> verify if changing uboot also fixes the problem?
>
> To me it seems better if the driver itself does what needs to be done
> instead of relying on specific settings that may or may not be done in
> U-Boot. Keep in mind that drivers may be loaded as modules, and that
> we even see cases where the same port serves multiple purposes by
> loading different driver modules (yes, this is not exactly a clever
> idea, but hardware designers come up with such solutions).


Someone with a scope can verify this, but my understanding of what
happens is that uboot by default puts the pins into GPIO mode. Linux
boots and reprograms the pins into AC97 mode. When the pins are
reprogrammed it generates glitches on the reset line. About half of
the time these glitches put the attached codec into test mode. Once
the chip is in test mode it won't respond to normal commands.

Does the opposite happen? What if uboot has the pins in AC97 mode and
Linux reprograms them into GPIO mode. Will it also put the chip into
test mode? The piece of information I've been missing is an
understanding of what is making the glitches that trigger test mode.

Another strategy would be to leave reset as is. If the chip is
unresponsive send it the commands to get it out of test mode. That
could be made part of the reset logic in the Linux driver.


>
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH, =A0 =A0 MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> The typical page layout program is nothing more =A0than =A0an =A0electron=
ic
> light table for cutting and pasting documents.
>



--=20
Jon Smirl
jonsmirl@gmail.com

RE: [PATCH 0/2] mpc5200 ac97 gpio reset

[Eric Millbrandt]

> -----Original Message-----
> From: Jon Smirl [mailto:jonsmirl@gmail.com]
> Sent: Wednesday, June 09, 2010 08:22
> To: Wolfgang Denk
> Cc: Eric Millbrandt; Mark Brown; linuxppc-dev@lists.ozlabs.org
> Subject: Re: [PATCH 0/2] mpc5200 ac97 gpio reset
>
> On Wed, Jun 9, 2010 at 2:13 AM, Wolfgang Denk <wd@denx.de> wrote:
> > Dear Jon Smirl,
> >
> > In message <AANLkTimIs90kR5uqhdBQ02oSd94dn08sOITm51Ylrl4-
> @mail.gmail.com> you wrote:
> >>
> >> Would making a change in uboot be a better solution? Eric, can you
> >> verify if changing uboot also fixes the problem?
> >
> > To me it seems better if the driver itself does what needs to be done
> > instead of relying on specific settings that may or may not be done in
> > U-Boot. Keep in mind that drivers may be loaded as modules, and that
> > we even see cases where the same port serves multiple purposes by
> > loading different driver modules (yes, this is not exactly a clever
> > idea, but hardware designers come up with such solutions).
>
>
> Someone with a scope can verify this, but my understanding of what
> happens is that uboot by default puts the pins into GPIO mode. Linux
> boots and reprograms the pins into AC97 mode. When the pins are
> reprogrammed it generates glitches on the reset line. About half of
> the time these glitches put the attached codec into test mode. Once
> the chip is in test mode it won't respond to normal commands.
>

Most of the pins on the 5200 default to gpio when the chip comes out of res=
et.  U-Boot then sets the pin muxing by writing to the port configuration r=
egister.  This value is defined as CONFIG_SYS_GPS_PORT_CONFIG in the U-Boot=
 board header file.

The implementation of ac97 gives the codec control of the bus clock.  This =
means that the codec is usually be clocking BITCLK when the 5200 pulls the =
reset line low, which is the cause of the problem.  Freescale should have h=
andled this on the silicon by having the ac97 cold reset function pull the =
SYNC and SDATAOUT lines low during cold reset, but instead they just docume=
nted the problem.

>From the MPC5200B user manual:
"Some AC97 devices goes to a test mode, if the Sync line is high during the=
 Res line is low (reset phase). To avoid this behavior the Sync line must b=
e also forced to zero during the reset phase. To do that, the pin muxing sh=
ould switch to GPIO mode and the GPIO control register should be used to co=
ntrol the output lines."

> Another strategy would be to leave reset as is. If the chip is
> unresponsive send it the commands to get it out of test mode. That
> could be made part of the reset logic in the Linux driver.

This is what we used to do when we were using the out-of-tree ac97 driver f=
rom Sylvain Munaut.  We would generate a cold reset/warm reset sequence in =
U-Boot.  We modified the Linux ac97 driver so that the cold reset functions=
 was a no-op.  It did avoid the problem, but it caused other problems.  Som=
etimes it is necessary to cold reset the codec in Linux to resync the ac97 =
bus.

>
>
> >
> >
> > Best regards,
> >
> > Wolfgang Denk
> >
> > --
> > DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> > Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> > The typical page layout program is nothing more  than  an  electronic
> > light table for cutting and pasting documents.
> >
>
>
>
> --
> Jon Smirl
> jonsmirl@gmail.com

Eric Millbrandt

-DISCLAIMER: an automatically appended disclaimer may follow. By posting-
-to a public e-mail mailing list I hereby grant permission to distribute-
-and copy this message.-


This e-mail and the information, including any attachments, it contains are=
 intended to be a confidential communication only to the person or entity t=
o whom it is addressed and may contain information that is privileged. If t=
he reader of this message is not the intended recipient, you are hereby not=
ified that any dissemination, distribution or copying of this communication=
 is strictly prohibited. If you have received this communication in error, =
please immediately notify the sender and destroy the original message.

Thank you.

Please consider the environment before printing this email.

Re: [PATCH 0/2] mpc5200 ac97 gpio reset

[Mark Brown]

On Wed, Jun 09, 2010 at 10:21:40AM -0400, Eric Millbrandt wrote:

[Please fix your MUA to word wrap paragraphs to within 80 characters,
I've reflowed the text below.]

> From the MPC5200B user manual:
> "Some AC97 devices goes to a test mode, if the Sync line is high
> during the Res line is low (reset phase). To avoid this behavior the
> Sync line must be also forced to zero during the reset phase. To do
> that, the pin muxing should switch to GPIO mode and the GPIO control
> register should be used to control the output lines."

Please include this quote in the changelog for the patch, if this a
documented workaround from the vendor that's a very different thing to
something that you've found happens to work on your systems (which is
more what your changelog sounded like).

Re: [PATCH 0/2] mpc5200 ac97 gpio reset

[Jon Smirl]

On Wed, Jun 9, 2010 at 10:32 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Jun 09, 2010 at 10:21:40AM -0400, Eric Millbrandt wrote:
>
> [Please fix your MUA to word wrap paragraphs to within 80 characters,
> I've reflowed the text below.]
>
>> From the MPC5200B user manual:
>> "Some AC97 devices goes to a test mode, if the Sync line is high
>> during the Res line is low (reset phase). To avoid this behavior the
>> Sync line must be also forced to zero during the reset phase. To do
>> that, the pin muxing should switch to GPIO mode and the GPIO control
>> register should be used to control the output lines."
>
> Please include this quote in the changelog for the patch, if this a
> documented workaround from the vendor that's a very different thing to
> something that you've found happens to work on your systems (which is
> more what your changelog sounded like).
>

Mark, is there a way to ask the chip if it is in test mode? We need to
be sure that's whats happening and it isn't some other glitch.


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH 0/2] mpc5200 ac97 gpio reset

[Mark Brown]

On Wed, Jun 09, 2010 at 10:41:23AM -0400, Jon Smirl wrote:
> On Wed, Jun 9, 2010 at 10:32 AM, Mark Brown

> > Please include this quote in the changelog for the patch, if this a
> > documented workaround from the vendor that's a very different thing to
> > something that you've found happens to work on your systems (which is
> > more what your changelog sounded like).

> Mark, is there a way to ask the chip if it is in test mode? We need to
> be sure that's whats happening and it isn't some other glitch.

I'd need to work with the hardware teams to obtain this information, but
I can confirm that having SYNC high while doing a reset is not supported
and so fixing the driver to not do that is a good idea independantly of
what it actually does to the chip.