[patch 0/2] iMX51 small fixes

[patch 0/2] iMX51 small fixes

[Arnaud Patard (Rtp)]

Hi,

This series is containing 2 small patches for iMX5 SoC.

The first one is touching at the gpiolib support. Currently, it's always
returning 0 as gpio value when configured as output so I've modified it to
report what we asked. I've only tested it by looking at /sys/kernel/debug/gpio
so I've no idea if this can actively break something.

The second one is less interesting. It's "only" fixing a build failure so not
a lot of things to say.

I've tested them against Sascha's Hauer imx-for-2.6.38 branch but they do apply
on current mainline tree.

Arnaud

[patch 1/2] imx51: fix gpiolib support

[Arnaud Patard (Rtp)]

An embedded and charset-unspecified text was scrubbed...
Name: mx51_gpiolib_out_value.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20101201/cb15b90a/attachment.ksh>

[patch 2/2] Fix imx cpufreq driver as module

[Arnaud Patard (Rtp)]

An embedded and charset-unspecified text was scrubbed...
Name: mxc_cpu_freq_module.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20101201/eb438be8/attachment.ksh>

[patch 1/2] imx51: fix gpiolib support

[Lothar Waßmann]

Hi,

> The gpiolib code is always returning 0 when reading pad values if the pads
> are configured as output. Using DR registers instead of PSR is fixing that.
> 
> Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
> Index: linux-2.6-submit/arch/arm/plat-mxc/gpio.c
> ===================================================================
> --- linux-2.6-submit.orig/arch/arm/plat-mxc/gpio.c	2010-11-24 18:00:47.000000000 +0100
> +++ linux-2.6-submit/arch/arm/plat-mxc/gpio.c	2010-11-24 18:06:54.000000000 +0100
> @@ -276,6 +276,10 @@
>  {
>  	struct mxc_gpio_port *port =
>  		container_of(chip, struct mxc_gpio_port, chip);
> +	u32 l = __raw_readl(port->base + GPIO_GDIR);
> +
> +	if (cpu_is_mx51() && (l & (1 << offset)))
> +		return (__raw_readl(port->base + GPIO_DR) >> offset) & 1;
>  
>  	return (__raw_readl(port->base + GPIO_PSR) >> offset) & 1;
>  }
>
Now you are not reading the actual pin state as with GPIO_PSR, but the
value that has been programmed into the output register (thus you
would not be able to determine whether an Open Drain output that is
programmed to HIGH is driven LOW by an external device).

Note that for being able to read back the actual pin state of an
output, the SION bit in the MUX_CTL register of the corresponding pad
has to be asserted.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[patch 1/2] imx51: fix gpiolib support

[Uwe Kleine-König]

Hello Arnaud,

On Wed, Dec 01, 2010 at 09:37:16AM +0100, Arnaud Patard wrote:
> The gpiolib code is always returning 0 when reading pad values if the pads
> are configured as output. Using DR registers instead of PSR is fixing that.
> 
> Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
> Index: linux-2.6-submit/arch/arm/plat-mxc/gpio.c
> ===================================================================
> --- linux-2.6-submit.orig/arch/arm/plat-mxc/gpio.c	2010-11-24 18:00:47.000000000 +0100
> +++ linux-2.6-submit/arch/arm/plat-mxc/gpio.c	2010-11-24 18:06:54.000000000 +0100
> @@ -276,6 +276,10 @@
>  {
>  	struct mxc_gpio_port *port =
>  		container_of(chip, struct mxc_gpio_port, chip);
> +	u32 l = __raw_readl(port->base + GPIO_GDIR);
> +
> +	if (cpu_is_mx51() && (l & (1 << offset)))
> +		return (__raw_readl(port->base + GPIO_DR) >> offset) & 1;
Maybe only read GDIR on mx51?  Isn't the direction saved by gpiolib
somewhere in RAM?  Is the same needed on other SoCs, too (see below for
my grouping)?  Technically it's not needed at all I think.
(Documentation/gpio.txt has: "However, note that not all platforms can
read the value of output pins; those that can't should always return
zero.".  I didn't check if the implementation conforms here though.)

I wonder if cpu_is_mx51 is the right macro to test, I'd prefer something
like imx_gpio_vX().  IMHO the gpio stuff needs a major overhaul getting
rid of most runtime checks.

We have at least the following groups:

	mx1
	mx21, mx27
	mx25, mx31, mx35, mx51, mxc91231

("at least" in the sense that socs on different lines are different.
I don't guarantee that e.g. mx35 and mxc91231 are really equivalent.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[patch 1/2] imx51: fix gpiolib support

[Arnaud Patard (Rtp)]

Lothar Wa?mann <LW@KARO-electronics.de> writes:

> Hi,

Hi,
>
>> The gpiolib code is always returning 0 when reading pad values if the pads
>> are configured as output. Using DR registers instead of PSR is fixing that.
>> 
>> Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
>> Index: linux-2.6-submit/arch/arm/plat-mxc/gpio.c
>> ===================================================================
>> --- linux-2.6-submit.orig/arch/arm/plat-mxc/gpio.c	2010-11-24 18:00:47.000000000 +0100
>> +++ linux-2.6-submit/arch/arm/plat-mxc/gpio.c	2010-11-24 18:06:54.000000000 +0100
>> @@ -276,6 +276,10 @@
>>  {
>>  	struct mxc_gpio_port *port =
>>  		container_of(chip, struct mxc_gpio_port, chip);
>> +	u32 l = __raw_readl(port->base + GPIO_GDIR);
>> +
>> +	if (cpu_is_mx51() && (l & (1 << offset)))
>> +		return (__raw_readl(port->base + GPIO_DR) >> offset) & 1;
>>  
>>  	return (__raw_readl(port->base + GPIO_PSR) >> offset) & 1;
>>  }
>>
> Now you are not reading the actual pin state as with GPIO_PSR, but the
> value that has been programmed into the output register (thus you
> would not be able to determine whether an Open Drain output that is
> programmed to HIGH is driven LOW by an external device).

yeah, I know. I didn't say it was the right fix. I noticed a problem and
proposed a solution.

>
> Note that for being able to read back the actual pin state of an
> output, the SION bit in the MUX_CTL register of the corresponding pad
> has to be asserted.

Is it working no matter of the MUX_MODE setting ? I thought that it was
working only if the MUX_MODE setting was 0 and not for every possible
MUX_MODE setting.

Arnaud

[patch 1/2] imx51: fix gpiolib support

[Arnaud Patard (Rtp)]

Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de> writes:

> Hello Arnaud,

Hi,
>
> On Wed, Dec 01, 2010 at 09:37:16AM +0100, Arnaud Patard wrote:
>> The gpiolib code is always returning 0 when reading pad values if the pads
>> are configured as output. Using DR registers instead of PSR is fixing that.
>> 
>> Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
>> Index: linux-2.6-submit/arch/arm/plat-mxc/gpio.c
>> ===================================================================
>> --- linux-2.6-submit.orig/arch/arm/plat-mxc/gpio.c	2010-11-24 18:00:47.000000000 +0100
>> +++ linux-2.6-submit/arch/arm/plat-mxc/gpio.c	2010-11-24 18:06:54.000000000 +0100
>> @@ -276,6 +276,10 @@
>>  {
>>  	struct mxc_gpio_port *port =
>>  		container_of(chip, struct mxc_gpio_port, chip);
>> +	u32 l = __raw_readl(port->base + GPIO_GDIR);
>> +
>> +	if (cpu_is_mx51() && (l & (1 << offset)))
>> +		return (__raw_readl(port->base + GPIO_DR) >> offset) & 1;
> Maybe only read GDIR on mx51?  Isn't the direction saved by gpiolib
> somewhere in RAM?  Is the same needed on other SoCs, too (see below for
> my grouping)?  Technically it's not needed at all I think.
> (Documentation/gpio.txt has: "However, note that not all platforms can
> read the value of output pins; those that can't should always return
> zero.".  I didn't check if the implementation conforms here though.)

yeah, I know that the gpiolib allows that and in this case, it's always
returning 0 but while I was debugging an other problem, I wanted to read
output value and it was not working. The manual was not explicitely
saying it was not possible so I thought it was a bug and propose a
fix. Hopefully, sending this patch allows me to get more feedback and
tell me if I got something wrong and there's a better fix.

>
> I wonder if cpu_is_mx51 is the right macro to test, I'd prefer something
> like imx_gpio_vX().  IMHO the gpio stuff needs a major overhaul getting
> rid of most runtime checks.

I used cpu_is_mx51() because I only had tested/seen that on imx515. I
wanted to avoid breaking other systems while trying to fix mine.
As regards getting rid of runtime checks, maybe registering different
gpiochip depending on the SoC would allow to check it a init time and no
more check later.

Arnaud

[patch 1/2] imx51: fix gpiolib support

[Alexander Stein]

On Wednesday 01 December 2010, 11:09:32 Arnaud Patard wrote:
> > Maybe only read GDIR on mx51?  Isn't the direction saved by gpiolib
> > somewhere in RAM?  Is the same needed on other SoCs, too (see below for
> > my grouping)?  Technically it's not needed at all I think.
> > (Documentation/gpio.txt has: "However, note that not all platforms can
> > read the value of output pins; those that can't should always return
> > zero.".  I didn't check if the implementation conforms here though.)
> 
> yeah, I know that the gpiolib allows that and in this case, it's always
> returning 0 but while I was debugging an other problem, I wanted to read
> output value and it was not working. The manual was not explicitely
> saying it was not possible so I thought it was a bug and propose a
> fix. Hopefully, sending this patch allows me to get more feedback and
> tell me if I got something wrong and there's a better fix.
> 
> > I wonder if cpu_is_mx51 is the right macro to test, I'd prefer something
> > like imx_gpio_vX().  IMHO the gpio stuff needs a major overhaul getting
> > rid of most runtime checks.
> 
> I used cpu_is_mx51() because I only had tested/seen that on imx515. I
> wanted to avoid breaking other systems while trying to fix mine.
> As regards getting rid of runtime checks, maybe registering different
> gpiochip depending on the SoC would allow to check it a init time and no
> more check later.

I can conform, that reading from GPIO_DR on mx35 works too. Also checked with 
the description is this register. It seem to show exactly what we want, 
regarding GPIO_GDIR value. So even evaluating GPIO_DIR isn't needed at all.
My fix was to simply replace GPIO_PSR with GPIO_DR.

Best regards,
Alexander

[patch 1/2] imx51: fix gpiolib support

[Arnaud Patard (Rtp)]

Alexander Stein <alexander.stein@systec-electronic.com> writes:

Hi,

> On Wednesday 01 December 2010, 11:09:32 Arnaud Patard wrote:
>> > Maybe only read GDIR on mx51?  Isn't the direction saved by gpiolib
>> > somewhere in RAM?  Is the same needed on other SoCs, too (see below for
>> > my grouping)?  Technically it's not needed at all I think.
>> > (Documentation/gpio.txt has: "However, note that not all platforms can
>> > read the value of output pins; those that can't should always return
>> > zero.".  I didn't check if the implementation conforms here though.)
>> 
>> yeah, I know that the gpiolib allows that and in this case, it's always
>> returning 0 but while I was debugging an other problem, I wanted to read
>> output value and it was not working. The manual was not explicitely
>> saying it was not possible so I thought it was a bug and propose a
>> fix. Hopefully, sending this patch allows me to get more feedback and
>> tell me if I got something wrong and there's a better fix.
>> 
>> > I wonder if cpu_is_mx51 is the right macro to test, I'd prefer something
>> > like imx_gpio_vX().  IMHO the gpio stuff needs a major overhaul getting
>> > rid of most runtime checks.
>> 
>> I used cpu_is_mx51() because I only had tested/seen that on imx515. I
>> wanted to avoid breaking other systems while trying to fix mine.
>> As regards getting rid of runtime checks, maybe registering different
>> gpiochip depending on the SoC would allow to check it a init time and no
>> more check later.
>
> I can conform, that reading from GPIO_DR on mx35 works too. Also checked with 
> the description is this register. It seem to show exactly what we want, 
> regarding GPIO_GDIR value. So even evaluating GPIO_DIR isn't needed at all.
> My fix was to simply replace GPIO_PSR with GPIO_DR.

Please look also at Lothar Wa?mann's comments. I plan to test that later
but if setting the SION bit allows things to work correctly, setting it
by default for the GPIOs may be a better fix than my patch.

Arnaud