Patchwork [U-Boot,1/2] efikamx: Configure the pins as GPIOs prior to using gpio_get_value.

login
register
mail settings
Submitter Fabio Estevam
Date Nov. 21, 2011, 2:09 p.m.
Message ID <1321884585-12501-1-git-send-email-fabio.estevam@freescale.com>
Download mbox | patch
Permalink /patch/126794/
State Rejected
Delegated to: Stefano Babic
Headers show

Comments

Fabio Estevam - Nov. 21, 2011, 2:09 p.m.
Configure the pins as GPIOs prior to using gpio_get_value

Cc: Marek Vasut <marek.vasut@gmail.com>
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 board/efikamx/efikamx.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)
Marek Vasut - Nov. 21, 2011, 2:20 p.m.
> Configure the pins as GPIOs prior to using gpio_get_value
> 
> Cc: Marek Vasut <marek.vasut@gmail.com>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  board/efikamx/efikamx.c |   13 +++++++++----
>  1 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/board/efikamx/efikamx.c b/board/efikamx/efikamx.c
> index 3d2cc1a..b911891 100644
> --- a/board/efikamx/efikamx.c
> +++ b/board/efikamx/efikamx.c
> @@ -308,10 +308,14 @@ struct fsl_esdhc_cfg esdhc_cfg[2] = {
> 
>  static inline uint32_t efika_mmc_cd(void)
>  {
> -	if (machine_is_efikamx())
> +	if (machine_is_efikamx()) {
> +		mxc_request_iomux(MX51_PIN_GPIO1_0, IOMUX_CONFIG_ALT1);
>  		return MX51_PIN_GPIO1_0;
> -	else
> +
> +	} else {
> +		mxc_request_iomux(MX51_PIN_EIM_CS2, IOMUX_CONFIG_ALT1);
>  		return MX51_PIN_EIM_CS2;
> +	}
>  }
> 
>  int board_mmc_getcd(u8 *absent, struct mmc *mmc)
> @@ -321,9 +325,10 @@ int board_mmc_getcd(u8 *absent, struct mmc *mmc)
> 
>  	if (cfg->esdhc_base == MMC_SDHC1_BASE_ADDR)
>  		*absent = gpio_get_value(IOMUX_TO_GPIO(cd));
> -	else
> +	else {
> +		mxc_request_iomux(MX51_PIN_GPIO1_8, IOMUX_CONFIG_ALT0);
>  		*absent = gpio_get_value(IOMUX_TO_GPIO(MX51_PIN_GPIO1_8));
> -
> +	}
>  	return 0;
>  }

NAK. There should be some common function for setting up iomux of those pins. 
You souldn't set it in repeatedly called functions.

M
Mike Frysinger - Nov. 21, 2011, 6:53 p.m.
On Monday 21 November 2011 09:20:49 Marek Vasut wrote:
> > Configure the pins as GPIOs prior to using gpio_get_value
> > 
> > +	else {
> > +		mxc_request_iomux(MX51_PIN_GPIO1_8, IOMUX_CONFIG_ALT0);
> >  		*absent = gpio_get_value(IOMUX_TO_GPIO(MX51_PIN_GPIO1_8));
> > -
> > +	}
> >  	return 0;
> >  }
> 
> NAK. There should be some common function for setting up iomux of those
> pins. You souldn't set it in repeatedly called functions.

that's what gpio_request() is for
-mike
Marek Vasut - Nov. 21, 2011, 7:29 p.m.
> On Monday 21 November 2011 09:20:49 Marek Vasut wrote:
> > > Configure the pins as GPIOs prior to using gpio_get_value
> > > 
> > > +	else {
> > > +		mxc_request_iomux(MX51_PIN_GPIO1_8, IOMUX_CONFIG_ALT0);
> > > 
> > >  		*absent = gpio_get_value(IOMUX_TO_GPIO(MX51_PIN_GPIO1_8));
> > > 
> > > -
> > > +	}
> > > 
> > >  	return 0;
> > >  
> > >  }
> > 
> > NAK. There should be some common function for setting up iomux of those
> > pins. You souldn't set it in repeatedly called functions.
> 
> that's what gpio_request() is for

I mean in efika.c ... there should be a common place for these iomux 
configurations being done. This is unrelated to gpio ...

> -mike
Mike Frysinger - Nov. 21, 2011, 8:14 p.m.
On Monday 21 November 2011 14:29:50 Marek Vasut wrote:
> > On Monday 21 November 2011 09:20:49 Marek Vasut wrote:
> > > > Configure the pins as GPIOs prior to using gpio_get_value
> > > > 
> > > > +	else {
> > > > +		mxc_request_iomux(MX51_PIN_GPIO1_8, IOMUX_CONFIG_ALT0);
> > > >  		*absent = gpio_get_value(IOMUX_TO_GPIO(MX51_PIN_GPIO1_8));
> > > > -
> > > > +	}
> > > 
> > > NAK. There should be some common function for setting up iomux of those
> > > pins. You souldn't set it in repeatedly called functions.
> > 
> > that's what gpio_request() is for
> 
> I mean in efika.c ... there should be a common place for these iomux
> configurations being done. This is unrelated to gpio ...

not really ... imo, if someone does gpio_request(PIN), the gpio core should 
take care of putting it into GPIO mode.  people shouldn't have to 
pinmux_request(PIN, GPIO_MODE) before doing gpio_request(PIN).
-mike
Marek Vasut - Nov. 21, 2011, 8:53 p.m.
> On Monday 21 November 2011 14:29:50 Marek Vasut wrote:
> > > On Monday 21 November 2011 09:20:49 Marek Vasut wrote:
> > > > > Configure the pins as GPIOs prior to using gpio_get_value
> > > > > 
> > > > > +	else {
> > > > > +		mxc_request_iomux(MX51_PIN_GPIO1_8, IOMUX_CONFIG_ALT0);
> > > > > 
> > > > >  		*absent = 
gpio_get_value(IOMUX_TO_GPIO(MX51_PIN_GPIO1_8));
> > > > > 
> > > > > -
> > > > > +	}
> > > > 
> > > > NAK. There should be some common function for setting up iomux of
> > > > those pins. You souldn't set it in repeatedly called functions.
> > > 
> > > that's what gpio_request() is for
> > 
> > I mean in efika.c ... there should be a common place for these iomux
> > configurations being done. This is unrelated to gpio ...
> 
> not really ... imo, if someone does gpio_request(PIN), the gpio core should
> take care of putting it into GPIO mode.  people shouldn't have to
> pinmux_request(PIN, GPIO_MODE) before doing gpio_request(PIN).

Of course ... considering there's always one correct setting for the pin to be 
in GPIO mode, which I suspect might not be completely true today anymore.
Mike Frysinger - Nov. 21, 2011, 9:22 p.m.
On Monday 21 November 2011 15:53:45 Marek Vasut wrote:
> > On Monday 21 November 2011 14:29:50 Marek Vasut wrote:
> > > > On Monday 21 November 2011 09:20:49 Marek Vasut wrote:
> > > > > > Configure the pins as GPIOs prior to using gpio_get_value
> > > > > > 
> > > > > > +	else {
> > > > > > +		mxc_request_iomux(MX51_PIN_GPIO1_8, IOMUX_CONFIG_ALT0);
> > > > > >  		*absent = gpio_get_value(IOMUX_TO_GPIO(MX51_PIN_GPIO1_8));
> > > > > > -
> > > > > > +	}
> > > > > 
> > > > > NAK. There should be some common function for setting up iomux of
> > > > > those pins. You souldn't set it in repeatedly called functions.
> > > > 
> > > > that's what gpio_request() is for
> > > 
> > > I mean in efika.c ... there should be a common place for these iomux
> > > configurations being done. This is unrelated to gpio ...
> > 
> > not really ... imo, if someone does gpio_request(PIN), the gpio core
> > should take care of putting it into GPIO mode.  people shouldn't have to
> > pinmux_request(PIN, GPIO_MODE) before doing gpio_request(PIN).
> 
> Of course ... considering there's always one correct setting for the pin to
> be in GPIO mode, which I suspect might not be completely true today
> anymore.

i find it hard to envision a pinmux system where individual pins would have 
different pinmux configurations to get it into GPIO mode.  probably be saner to 
have gpio_request() do the right thing and wait for someone to come forward 
with the unusual setup -- worry about it then.
-mike
Stefano Babic - Nov. 22, 2011, 8:15 a.m.
On 21/11/2011 22:22, Mike Frysinger wrote:

>> Of course ... considering there's always one correct setting for
>> the pin to be in GPIO mode, which I suspect might not be
>> completely true today anymore.
> 
> i find it hard to envision a pinmux system where individual pins
> would have different pinmux configurations to get it into GPIO
> mode.  probably be saner to have gpio_request() do the right thing
> and wait for someone to come forward with the unusual setup --
> worry about it then.

In fact it would be nicer if gpio_request() takes care of the pinmux,
in the way I can see on the davinci SOCs. However, on the IMXs a
single GPIO can be connected (not at the same time, of course) to
different PADs, depending on a general setup (GPR register) or if the
daisy chain in the multiplexer is activated.

Calling gpio_request and passing only the gpio number is not enough on
the IMXs to find where the GPIO is routed.

The second point I will arise is that, mainly due to the different
internal layout but also for historical reasons, the setup and the
provided function for the multiplexer is very different among the SOCs.

Only mx35 and mx5 expone the same interface (mxc_request_iomux), while
mx31/mx25/mx27/mx28 have its own. Because we use and we want to use
the GPIO framework, the gpio driver, common to all IMX SOCs, should be
able to set up the multiplexer independently from the SOC type, that
means we should have the same interface for the multiplexer, and we
have not (yet ?).

Stefano
Mike Frysinger - Nov. 22, 2011, 9:07 p.m.
On Tuesday 22 November 2011 03:15:47 Stefano Babic wrote:
> On 21/11/2011 22:22, Mike Frysinger wrote:
> >> Of course ... considering there's always one correct setting for
> >> the pin to be in GPIO mode, which I suspect might not be
> >> completely true today anymore.
> > 
> > i find it hard to envision a pinmux system where individual pins
> > would have different pinmux configurations to get it into GPIO
> > mode.  probably be saner to have gpio_request() do the right thing
> > and wait for someone to come forward with the unusual setup --
> > worry about it then.
> 
> In fact it would be nicer if gpio_request() takes care of the pinmux,
> in the way I can see on the davinci SOCs. However, on the IMXs a
> single GPIO can be connected (not at the same time, of course) to
> different PADs, depending on a general setup (GPR register) or if the
> daisy chain in the multiplexer is activated.

if it's different physical pins, then perhaps it should be different GPIO 
numbers ?

> The second point I will arise is that, mainly due to the different
> internal layout but also for historical reasons, the setup and the
> provided function for the multiplexer is very different among the SOCs.
> 
> Only mx35 and mx5 expone the same interface (mxc_request_iomux), while
> mx31/mx25/mx27/mx28 have its own. Because we use and we want to use
> the GPIO framework, the gpio driver, common to all IMX SOCs, should be
> able to set up the multiplexer independently from the SOC type, that
> means we should have the same interface for the multiplexer, and we
> have not (yet ?).

this is shaking out in Linux with the pinmux framework, so probably best to 
grit our teeth until that's done and then adopt what they implement.
-mike
Igor Grinberg - Nov. 23, 2011, 7:02 a.m.
Hi,

On 11/22/11 23:07, Mike Frysinger wrote:
> On Tuesday 22 November 2011 03:15:47 Stefano Babic wrote:
>> On 21/11/2011 22:22, Mike Frysinger wrote:
>>>> Of course ... considering there's always one correct setting for
>>>> the pin to be in GPIO mode, which I suspect might not be
>>>> completely true today anymore.
>>>
>>> i find it hard to envision a pinmux system where individual pins
>>> would have different pinmux configurations to get it into GPIO
>>> mode.  probably be saner to have gpio_request() do the right thing
>>> and wait for someone to come forward with the unusual setup --
>>> worry about it then.
>>
>> In fact it would be nicer if gpio_request() takes care of the pinmux,
>> in the way I can see on the davinci SOCs. However, on the IMXs a
>> single GPIO can be connected (not at the same time, of course) to
>> different PADs, depending on a general setup (GPR register) or if the
>> daisy chain in the multiplexer is activated.
> 
> if it's different physical pins, then perhaps it should be different GPIO 
> numbers ?

I think, currently, you are right, but if we look in some other
AF (Alternate Functions) (not GPIO), same AF can be available on
different physical pins. This means that in theory, same GPIO number
can also be connected to different physical pins.
I think it should be board controllable instead of gpiolib hard coded,
or of course pinmux controlled (which is board controllable).

> 
>> The second point I will arise is that, mainly due to the different
>> internal layout but also for historical reasons, the setup and the
>> provided function for the multiplexer is very different among the SOCs.
>>
>> Only mx35 and mx5 expone the same interface (mxc_request_iomux), while
>> mx31/mx25/mx27/mx28 have its own. Because we use and we want to use
>> the GPIO framework, the gpio driver, common to all IMX SOCs, should be
>> able to set up the multiplexer independently from the SOC type, that
>> means we should have the same interface for the multiplexer, and we
>> have not (yet ?).
> 
> this is shaking out in Linux with the pinmux framework, so probably best to 
> grit our teeth until that's done and then adopt what they implement.

This is probably the best way and correct me if I'm wrong,
this means stick with board control over the pinmux
instead of gpiolib.
Stefano Babic - Nov. 23, 2011, 8:19 a.m.
On 22/11/2011 22:07, Mike Frysinger wrote:

> 
> if it's different physical pins, then perhaps it should be
> different GPIO numbers ?

No, they are not. Depending on the "daisy chain" functionality and
based on different alternate function, a single GPIO can be routed to
different physical pins.

> this is shaking out in Linux with the pinmux framework, so probably
> best to grit our teeth until that's done and then adopt what they
> implement.

Agree.

Best regards,
Stefano Babic
Mike Frysinger - Nov. 23, 2011, 7:43 p.m.
On Wednesday 23 November 2011 02:02:25 Igor Grinberg wrote:
> On 11/22/11 23:07, Mike Frysinger wrote:
> > On Tuesday 22 November 2011 03:15:47 Stefano Babic wrote:
> >> On 21/11/2011 22:22, Mike Frysinger wrote:
> >>>> Of course ... considering there's always one correct setting for
> >>>> the pin to be in GPIO mode, which I suspect might not be
> >>>> completely true today anymore.
> >>> 
> >>> i find it hard to envision a pinmux system where individual pins
> >>> would have different pinmux configurations to get it into GPIO
> >>> mode.  probably be saner to have gpio_request() do the right thing
> >>> and wait for someone to come forward with the unusual setup --
> >>> worry about it then.
> >> 
> >> In fact it would be nicer if gpio_request() takes care of the pinmux,
> >> in the way I can see on the davinci SOCs. However, on the IMXs a
> >> single GPIO can be connected (not at the same time, of course) to
> >> different PADs, depending on a general setup (GPR register) or if the
> >> daisy chain in the multiplexer is activated.
> > 
> > if it's different physical pins, then perhaps it should be different GPIO
> > numbers ?
> 
> I think, currently, you are right, but if we look in some other
> AF (Alternate Functions) (not GPIO), same AF can be available on
> different physical pins. This means that in theory, same GPIO number
> can also be connected to different physical pins.
> I think it should be board controllable instead of gpiolib hard coded,
> or of course pinmux controlled (which is board controllable).

i understand AF for peripheral pins, but i'm not seeing the same-gpio-number-
for-different-pins part.

> >> The second point I will arise is that, mainly due to the different
> >> internal layout but also for historical reasons, the setup and the
> >> provided function for the multiplexer is very different among the SOCs.
> >> 
> >> Only mx35 and mx5 expone the same interface (mxc_request_iomux), while
> >> mx31/mx25/mx27/mx28 have its own. Because we use and we want to use
> >> the GPIO framework, the gpio driver, common to all IMX SOCs, should be
> >> able to set up the multiplexer independently from the SOC type, that
> >> means we should have the same interface for the multiplexer, and we
> >> have not (yet ?).
> > 
> > this is shaking out in Linux with the pinmux framework, so probably best
> > to grit our teeth until that's done and then adopt what they implement.
> 
> This is probably the best way and correct me if I'm wrong,
> this means stick with board control over the pinmux
> instead of gpiolib.

*shrug* whatever the maintainer of the gpio/drivers want to do.  as long as 
it's not common code, i won't worry about it for now.
-mike
Mike Frysinger - Nov. 23, 2011, 7:44 p.m.
On Wednesday 23 November 2011 03:19:48 Stefano Babic wrote:
> On 22/11/2011 22:07, Mike Frysinger wrote:
> > if it's different physical pins, then perhaps it should be
> > different GPIO numbers ?
> 
> No, they are not. Depending on the "daisy chain" functionality and
> based on different alternate function, a single GPIO can be routed to
> different physical pins.

the GPIO # can represent the AF as well as the GPIO itself.  this is what bit 
shifts and masks are good at.

there's no requirement that the GPIO #'s be contiguous, or have a specific 
range.
-mike

Patch

diff --git a/board/efikamx/efikamx.c b/board/efikamx/efikamx.c
index 3d2cc1a..b911891 100644
--- a/board/efikamx/efikamx.c
+++ b/board/efikamx/efikamx.c
@@ -308,10 +308,14 @@  struct fsl_esdhc_cfg esdhc_cfg[2] = {
 
 static inline uint32_t efika_mmc_cd(void)
 {
-	if (machine_is_efikamx())
+	if (machine_is_efikamx()) {
+		mxc_request_iomux(MX51_PIN_GPIO1_0, IOMUX_CONFIG_ALT1);
 		return MX51_PIN_GPIO1_0;
-	else
+
+	} else {
+		mxc_request_iomux(MX51_PIN_EIM_CS2, IOMUX_CONFIG_ALT1);
 		return MX51_PIN_EIM_CS2;
+	}
 }
 
 int board_mmc_getcd(u8 *absent, struct mmc *mmc)
@@ -321,9 +325,10 @@  int board_mmc_getcd(u8 *absent, struct mmc *mmc)
 
 	if (cfg->esdhc_base == MMC_SDHC1_BASE_ADDR)
 		*absent = gpio_get_value(IOMUX_TO_GPIO(cd));
-	else
+	else {
+		mxc_request_iomux(MX51_PIN_GPIO1_8, IOMUX_CONFIG_ALT0);
 		*absent = gpio_get_value(IOMUX_TO_GPIO(MX51_PIN_GPIO1_8));
-
+	}
 	return 0;
 }