imx27: pinctrl: GPIO set output value failed

[Markus Pargmann]

Hi Chris,

On Tue, Jan 14, 2014 at 05:13:25PM +0800, Chris Ruehl wrote:
> On Tuesday, January 14, 2014 04:57 PM, Markus Pargmann wrote:
> >Hi,
> >
> >On Tue, Jan 14, 2014 at 01:39:22PM +0800, Chris Ruehl wrote:
> >>Just fall over the Reference manual for the GPIO ports.
> >>Seams the iconfb0 is not addressed right.
> >>
> >>On Tuesday, January 14, 2014 12:13 PM, Chris Ruehl wrote:
> >>>Hi,
> >>>
> >>>some kind of weired problem:
> >>>Some GPIO's can modified via /sysfs  others cannot
> >>>in my case gpio9 works, and gpio10,12 not.
> >>>The pinctrl debug out looks good to me.
> >>>
> >>>running kernel 3.13-rc
> >>>
> >>>Working OK
> >>Table 6.1 of the imx27 reference manual says (Page 6-5,6-6)
> >>>[ 1.553601] imx27-pinctrl 10015000.iomuxc: imx1_pmx_enable, pin
> >>>0x9, function 0, gpio 1, direction 1, oconf 3, iconfa 0, iconfb 0
> >>>[    1.553657] imx27-pinctrl 10015000.iomuxc: write: register
> >>>0xf4415004 offset 18 value 0x3
> >>PTA_OCR1  1001_5004 OK
> >>>[    1.553712] imx27-pinctrl 10015000.iomuxc: write: register
> >>>0xf441500c offset 18 value 0x0
> >>PTA_ICONFA1 1001_500C  OK
> >>>[    1.553766] imx27-pinctrl 10015000.iomuxc: write: register
> >>>0xf4415010 offset 18 value 0x0
> >>PTA_ICONFB1 1001_5014  --->  0xf4415010 seams the wrong address
> >>1001_5010 is the PTA_ICONFA2
> >Yes you are right, it is wrong by 0x4, pinctrl-imx1-core.c:
> >#define MX1_ICONFB 0x10
> >
> >Can you send a patch?
> >
> >Thanks,
> >
> >Markus
> >
> Hi Markus,
> 
> I found the wrong define already and recompile the kernel, but still
> not working for me. Very strange.
> gpio9,13  and 14,16 as input works OK. Only this buggers 10 and 12
> are bugging around.
> 
> a) I replaced a debug output statement after the address correction
> when the pin-id > 16 to get correct
> address output. Here a preview of the patch.
> b) add pin soft reset before set the gpio parameter.

gpios 10 and 12 are working correctly with a previous pin soft reset?

> 
> diff --git a/drivers/pinctrl/pinctrl-imx1-core.c
> b/drivers/pinctrl/pinctrl-imx1-core.c
> index 17aecde..fbaa70e 100644
> --- a/drivers/pinctrl/pinctrl-imx1-core.c
> +++ b/drivers/pinctrl/pinctrl-imx1-core.c
> @@ -27,6 +27,7 @@
>  #include <linux/pinctrl/pinctrl.h>
>  #include <linux/pinctrl/pinmux.h>
>  #include <linux/slab.h>
> +#include <linux/delay.h>
> 
>  #include "core.h"
>  #include "pinctrl-imx1.h"
> @@ -45,9 +46,10 @@ struct imx1_pinctrl {
>  #define MX1_DDIR 0x00
>  #define MX1_OCR 0x04
>  #define MX1_ICONFA 0x0c
> -#define MX1_ICONFB 0x10
> +#define MX1_ICONFB 0x14
>  #define MX1_GIUS 0x20
>  #define MX1_GPR 0x38
> +#define MX1_SWR 0x3C
>  #define MX1_PUEN 0x40
> 
>  #define MX1_PORT_STRIDE 0x100
> @@ -97,13 +99,13 @@ static void imx1_write_2bit(struct imx1_pinctrl
> *ipctl, unsigned int pin_id,
>         u32 old_val;
>         u32 new_val;
> 
> -       dev_dbg(ipctl->dev, "write: register 0x%p offset %d value 0x%x\n",
> -                       reg, offset, value);
> -
>         /* Use the next register if the pin's port pin number is >=16 */
>         if (pin_id % 32 >= 16)
>                 reg += 0x04;
> 
> +       dev_dbg(ipctl->dev, "write: register 0x%p offset %d value 0x%x\n",
> +                       reg, offset, value);
> +
>         /* Get current state of pins */
>         old_val = readl(reg);
>         old_val &= mask;
> @@ -334,6 +336,9 @@ static int imx1_pmx_enable(struct pinctrl_dev
> *pctldev, unsigned selector,
>                                 direction, gpio_oconf, gpio_iconfa,
>                                 gpio_iconfb);
> 
> +               imx1_write_bit(ipctl, pin_id, 1, MX1_SWR);  /* Soft
> reset the pin */

This is over 80 lines, simply put the comment above. Would also be
great if you could add some more description here why a soft reset is
necessary. It is not listed in the reference manual, so this may be
confusing otherwise.

> +               /* reset done within 6 cycles */
> +               usleep_range(10000,20000);
>                 imx1_write_bit(ipctl, pin_id, gpio_in_use, MX1_GIUS);
>                 imx1_write_bit(ipctl, pin_id, direction, MX1_DDIR);
> 
> 
> I will do some more testing and send the patch after, OK for you?

Yes that's ok. You could also seperate the ICONFB bugfix into a seperate
patch.

Regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |