[U-Boot] [PATCH] tegra: pinmux: fix FUNCMUX_NDFLASH_KBC_8_BIT

[U-Boot] [PATCH] tegra: pinmux: fix FUNCMUX_NDFLASH_KBC_8_BIT

[Marcel Ziswiler]

From: Lucas Stach <dev@lynxeye.de>

Even the 8-bit case needs KBCB configured, as pin D7 is located in this
pingroup. Also pingroup ATC seems to come out of reset with config set
to NAND, so we need to explictly configure some other function to this
group in order to avoid clashing settings.

Signed-off-by: Lucas Stach <dev@lynxeye.de>
Signed-off-by: Marcel Ziswiler <marcel@ziswiler.com>
Tested-by: Marcel Ziswiler <marcel@ziswiler.com>
---
 arch/arm/mach-tegra/tegra20/funcmux.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/mach-tegra/tegra20/funcmux.c b/arch/arm/mach-tegra/tegra20/funcmux.c
index 0df4a07..f9b6214 100644
--- a/arch/arm/mach-tegra/tegra20/funcmux.c
+++ b/arch/arm/mach-tegra/tegra20/funcmux.c
@@ -252,17 +252,25 @@ int funcmux_select(enum periph_id id, int config)
 			break;
 		case FUNCMUX_NDFLASH_KBC_8_BIT:
 			pinmux_set_func(PMUX_PINGRP_KBCA, PMUX_FUNC_NAND);
+			pinmux_set_func(PMUX_PINGRP_KBCB, PMUX_FUNC_NAND);
 			pinmux_set_func(PMUX_PINGRP_KBCC, PMUX_FUNC_NAND);
 			pinmux_set_func(PMUX_PINGRP_KBCD, PMUX_FUNC_NAND);
 			pinmux_set_func(PMUX_PINGRP_KBCE, PMUX_FUNC_NAND);
 			pinmux_set_func(PMUX_PINGRP_KBCF, PMUX_FUNC_NAND);
 
 			pinmux_tristate_disable(PMUX_PINGRP_KBCA);
+			pinmux_tristate_disable(PMUX_PINGRP_KBCB);
 			pinmux_tristate_disable(PMUX_PINGRP_KBCC);
 			pinmux_tristate_disable(PMUX_PINGRP_KBCD);
 			pinmux_tristate_disable(PMUX_PINGRP_KBCE);
 			pinmux_tristate_disable(PMUX_PINGRP_KBCF);
 
+			/*
+			 * configure pingroup ATC to something unrelated to
+			 * avoid ATC overriding KBC
+			 */
+			pinmux_set_func(PMUX_PINGRP_ATC, PMUX_FUNC_GMI);
+
 			bad_config = 0;
 			break;
 		}
-- 
1.9.3

[U-Boot] [PATCH] tegra: pinmux: fix FUNCMUX_NDFLASH_KBC_8_BIT

[Stephen Warren]

On 03/24/2015 06:29 AM, Marcel Ziswiler wrote:
> From: Lucas Stach <dev@lynxeye.de>
>
> Even the 8-bit case needs KBCB configured, as pin D7 is located in this
> pingroup. Also pingroup ATC seems to come out of reset with config set
> to NAND, so we need to explictly configure some other function to this
> group in order to avoid clashing settings.

I would rather not touch ATC like this; see below.

> diff --git a/arch/arm/mach-tegra/tegra20/funcmux.c b/arch/arm/mach-tegra/tegra20/funcmux.c

Note that this will conflict with:

09f455dca749 ARM: tegra: collect SoC sources into mach-tegra

... which moved that file.

> @@ -252,17 +252,25 @@ int funcmux_select(enum periph_id id, int config)
...
>   		case FUNCMUX_NDFLASH_KBC_8_BIT:
...
> +			/*
> +			 * configure pingroup ATC to something unrelated to
> +			 * avoid ATC overriding KBC
> +			 */
> +			pinmux_set_func(PMUX_PINGRP_ATC, PMUX_FUNC_GMI);

What if ATC is actually used for some purpose other than GMI? This will 
corrupt the pinmux for that pingroup, and prevent the other peripheral 
from working. For example, PERIPH_ID_SDMMC4/FUNCMUX_SDMMC4_ATC_ATD_8BIT 
actively uses ATC, and this change might break it if both are used 
together on one board.

The best approach is to simply not use funcmux at all, but rather 
program the entire pinmux from a table. That guarantees no conflicts. 
Boards using later SoCs take this approach.

Alternatively, have the board code (rather than funcmux) select some 
other value for ATC. This allows the board code to select the exact 
correct value for that pingroup once (thus avoiding multiple changes to 
the pinmux for that pingroup, which could cause glitches), and 
guarantees that the common code will never corrupt that setting.

[U-Boot] [PATCH] tegra: pinmux: fix FUNCMUX_NDFLASH_KBC_8_BIT

[Marcel Ziswiler]

On 24 March 2015 16:21:02 CET, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> diff --git a/arch/arm/mach-tegra/tegra20/funcmux.c
>b/arch/arm/mach-tegra/tegra20/funcmux.c
>
>Note that this will conflict with:
>
>09f455dca749 ARM: tegra: collect SoC sources into mach-tegra
>
>... which moved that file.

OK, sorry. Haven't seen that one yet. Will rebase accordingly.

>What if ATC is actually used for some purpose other than GMI? This will
>
>corrupt the pinmux for that pingroup, and prevent the other peripheral 
>from working. For example, PERIPH_ID_SDMMC4/FUNCMUX_SDMMC4_ATC_ATD_8BIT
>
>actively uses ATC, and this change might break it if both are used 
>together on one board.

Yes, agreed.

>The best approach is to simply not use funcmux at all, but rather 
>program the entire pinmux from a table. That guarantees no conflicts. 
>Boards using later SoCs take this approach.

Yes, that one I knew but nobody does on T20 (yet) plus T20 is kind of special due to its pin groups spanning multiple pins and such.

>Alternatively, have the board code (rather than funcmux) select some 
>other value for ATC. This allows the board code to select the exact 
>correct value for that pingroup once (thus avoiding multiple changes to
>
>the pinmux for that pingroup, which could cause glitches), and 
>guarantees that the common code will never corrupt that setting.

That one for sure would work if above table approach proves to be too cumbersome to do.

Thanks Stephen
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[U-Boot] [PATCH] tegra: pinmux: fix FUNCMUX_NDFLASH_KBC_8_BIT

[Marcel Ziswiler]

On Tue, 2015-03-24 at 09:21 -0600, Stephen Warren wrote:
> > diff --git a/arch/arm/mach-tegra/tegra20/funcmux.c b/arch/arm/mach-tegra/tegra20/funcmux.c
> 
> Note that this will conflict with:
> 
> 09f455dca749 ARM: tegra: collect SoC sources into mach-tegra
> 
> ... which moved that file.

I had another look but believe above mentioned commit actually did the
following:

 arch/arm/cpu/tegra20-common/*       -> arch/arm/mach-tegra/tegra20/*

And my commit was already rebased to latest master and therefore already
took that into account or am I missing something here?

[U-Boot] [PATCH] tegra: pinmux: fix FUNCMUX_NDFLASH_KBC_8_BIT

[Stephen Warren]

On 03/25/2015 11:46 AM, Marcel Ziswiler wrote:
> On Tue, 2015-03-24 at 09:21 -0600, Stephen Warren wrote:
>>> diff --git a/arch/arm/mach-tegra/tegra20/funcmux.c b/arch/arm/mach-tegra/tegra20/funcmux.c
>>
>> Note that this will conflict with:
>>
>> 09f455dca749 ARM: tegra: collect SoC sources into mach-tegra
>>
>> ... which moved that file.
>
> I had another look but believe above mentioned commit actually did the
> following:
>
>   arch/arm/cpu/tegra20-common/*       -> arch/arm/mach-tegra/tegra20/*
>
> And my commit was already rebased to latest master and therefore already
> took that into account or am I missing something here?

Yes, I think I got it backwards; I was confused by the Tegra210 file 
having been added in the old location after the move, or something.

[U-Boot] [PATCH] tegra: pinmux: fix FUNCMUX_NDFLASH_KBC_8_BIT

[Lucas Stach]

Even the 8bit case needs KBCB configured, as pin D7 is located in this
pingroup. Also pingroup ATC seems to come out of reset with config set
to NAND, so we need to explictly configure some other function to this
group in order to avoid clashing settings.

Signed-off-by: Lucas Stach <dev@lynxeye.de>
---
 arch/arm/cpu/tegra20-common/funcmux.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/cpu/tegra20-common/funcmux.c b/arch/arm/cpu/tegra20-common/funcmux.c
index a1c55a6..30fdf1c 100644
--- a/arch/arm/cpu/tegra20-common/funcmux.c
+++ b/arch/arm/cpu/tegra20-common/funcmux.c
@@ -266,17 +266,25 @@ int funcmux_select(enum periph_id id, int config)
 			break;
 		case FUNCMUX_NDFLASH_KBC_8_BIT:
 			pinmux_set_func(PINGRP_KBCA, PMUX_FUNC_NAND);
+			pinmux_set_func(PINGRP_KBCB, PMUX_FUNC_NAND);
 			pinmux_set_func(PINGRP_KBCC, PMUX_FUNC_NAND);
 			pinmux_set_func(PINGRP_KBCD, PMUX_FUNC_NAND);
 			pinmux_set_func(PINGRP_KBCE, PMUX_FUNC_NAND);
 			pinmux_set_func(PINGRP_KBCF, PMUX_FUNC_NAND);
 
 			pinmux_tristate_disable(PINGRP_KBCA);
+			pinmux_tristate_disable(PINGRP_KBCB);
 			pinmux_tristate_disable(PINGRP_KBCC);
 			pinmux_tristate_disable(PINGRP_KBCD);
 			pinmux_tristate_disable(PINGRP_KBCE);
 			pinmux_tristate_disable(PINGRP_KBCF);
 
+			/*
+			 * configure pingroup ATC to something unrelated to
+			 * avoid ATC overriding KBC
+			 */
+			pinmux_set_func(PINGRP_ATC, PMUX_FUNC_GMI);
+
 			bad_config = 0;
 			break;
 		}
-- 
1.8.0.2

[U-Boot] [PATCH] tegra: pinmux: fix FUNCMUX_NDFLASH_KBC_8_BIT

[Stephen Warren]

On 01/21/2013 05:20 PM, Lucas Stach wrote:
> Even the 8bit case needs KBCB configured, as pin D7 is located in this
> pingroup. Also pingroup ATC seems to come out of reset with config set
> to NAND, so we need to explictly configure some other function to this
> group in order to avoid clashing settings.

> diff --git a/arch/arm/cpu/tegra20-common/funcmux.c b/arch/arm/cpu/tegra20-common/funcmux.c

> @@ -266,17 +266,25 @@ int funcmux_select(enum periph_id id, int config)
>  			break;
>  		case FUNCMUX_NDFLASH_KBC_8_BIT:
...
> +			/*
> +			 * configure pingroup ATC to something unrelated to
> +			 * avoid ATC overriding KBC
> +			 */
> +			pinmux_set_func(PINGRP_ATC, PMUX_FUNC_GMI);
> +

This gets a bit dangerous; what if pingroup ATC was already configured
for some function other than NAND or GMI? This code will then break that
setting. I would suggest one of the following alternatives:

1) Use the new pinmux_avoid_func() function implemented in the patch
that I just sent.

2) Move Tegra20 over to the new board-wide pinmux style that Tegra30
uses, where the entire pinmux is initialized in one shot. This will
completely avoid any kind of uninitialized pinmux settings, and to some
extent is the only sensible thing to do on a device like Tegra which has
the potential for conflicts like this patch tries to avoid.

[U-Boot] [PATCH] tegra: pinmux: fix FUNCMUX_NDFLASH_KBC_8_BIT

[Lucas Stach]

Am Dienstag, den 22.01.2013, 09:24 -0700 schrieb Stephen Warren:
> On 01/21/2013 05:20 PM, Lucas Stach wrote:
> > Even the 8bit case needs KBCB configured, as pin D7 is located in this
> > pingroup. Also pingroup ATC seems to come out of reset with config set
> > to NAND, so we need to explictly configure some other function to this
> > group in order to avoid clashing settings.
> 
> > diff --git a/arch/arm/cpu/tegra20-common/funcmux.c b/arch/arm/cpu/tegra20-common/funcmux.c
> 
> > @@ -266,17 +266,25 @@ int funcmux_select(enum periph_id id, int config)
> >  			break;
> >  		case FUNCMUX_NDFLASH_KBC_8_BIT:
> ...
> > +			/*
> > +			 * configure pingroup ATC to something unrelated to
> > +			 * avoid ATC overriding KBC
> > +			 */
> > +			pinmux_set_func(PINGRP_ATC, PMUX_FUNC_GMI);
> > +
> 
> This gets a bit dangerous; what if pingroup ATC was already configured
> for some function other than NAND or GMI? This code will then break that
> setting. I would suggest one of the following alternatives:
> 
> 1) Use the new pinmux_avoid_func() function implemented in the patch
> that I just sent.
> 
> 2) Move Tegra20 over to the new board-wide pinmux style that Tegra30
> uses, where the entire pinmux is initialized in one shot. This will
> completely avoid any kind of uninitialized pinmux settings, and to some
> extent is the only sensible thing to do on a device like Tegra which has
> the potential for conflicts like this patch tries to avoid.
> 
I'll take a look on how much work it is to implement option #2. If it
isn't too much and I find some time in this U-Boot release cycle, I'm
very much inclined to do this the ultimately right way.

Regards,
Lucas

[U-Boot] [PATCH] tegra: pinmux: fix FUNCMUX_NDFLASH_KBC_8_BIT

[Allen Martin]

On Tue, Jan 22, 2013 at 02:27:30PM -0800, Lucas Stach wrote:
> Am Dienstag, den 22.01.2013, 09:24 -0700 schrieb Stephen Warren:
> > On 01/21/2013 05:20 PM, Lucas Stach wrote:
> > > Even the 8bit case needs KBCB configured, as pin D7 is located in this
> > > pingroup. Also pingroup ATC seems to come out of reset with config set
> > > to NAND, so we need to explictly configure some other function to this
> > > group in order to avoid clashing settings.
> > 
> > > diff --git a/arch/arm/cpu/tegra20-common/funcmux.c b/arch/arm/cpu/tegra20-common/funcmux.c
> > 
> > > @@ -266,17 +266,25 @@ int funcmux_select(enum periph_id id, int config)
> > >  			break;
> > >  		case FUNCMUX_NDFLASH_KBC_8_BIT:
> > ...
> > > +			/*
> > > +			 * configure pingroup ATC to something unrelated to
> > > +			 * avoid ATC overriding KBC
> > > +			 */
> > > +			pinmux_set_func(PINGRP_ATC, PMUX_FUNC_GMI);
> > > +
> > 
> > This gets a bit dangerous; what if pingroup ATC was already configured
> > for some function other than NAND or GMI? This code will then break that
> > setting. I would suggest one of the following alternatives:
> > 
> > 1) Use the new pinmux_avoid_func() function implemented in the patch
> > that I just sent.
> > 
> > 2) Move Tegra20 over to the new board-wide pinmux style that Tegra30
> > uses, where the entire pinmux is initialized in one shot. This will
> > completely avoid any kind of uninitialized pinmux settings, and to some
> > extent is the only sensible thing to do on a device like Tegra which has
> > the potential for conflicts like this patch tries to avoid.
> > 
> I'll take a look on how much work it is to implement option #2. If it
> isn't too much and I find some time in this U-Boot release cycle, I'm
> very much inclined to do this the ultimately right way.

I think #2 really is the only way to do it.  The reset state is full
of conflicts and unless everything is initialized at once you end up
chasing your tail where you fix conflict A, but that leads to new
conflict B, and fixing that leads to C, etc.

-Allen
-- 
nvpublic