Change in coreboot[master]: src/superio/nuvoton/nct5104d: soft reset GPIO

[Piotr Kleinschmidt (Code Review)]

Piotr Kleinschmidt has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35482 )

Change subject: src/superio/nuvoton/nct5104d: soft reset GPIO
......................................................................

src/superio/nuvoton/nct5104d: soft reset GPIO

So far, only hard power off could reset GPIOs state to defaults:
IN, Open-drain. Now, defaults are set with every boot to ensure
that GPIOS are not in uknown/unwanted state.

Change-Id: I67878dbab2ddf0deaaa8f5d79416368c6164ba1d
Signed-off-by: Piotr Kleinschmidt 
---
M src/superio/nuvoton/nct5104d/nct5104d.h
M src/superio/nuvoton/nct5104d/superio.c
2 files changed, 24 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/35482/1

diff --git a/src/superio/nuvoton/nct5104d/nct5104d.h b/src/superio/nuvoton/nct5104d/nct5104d.h
index 707e94a..6826bb6 100644
--- a/src/superio/nuvoton/nct5104d/nct5104d.h
+++ b/src/superio/nuvoton/nct5104d/nct5104d.h
@@ -21,15 +21,25 @@
 #include <device/pnp_type.h>

/* SIO global configuration */
+#define LDN_SELECT_CR07     0x07
 #define IRQ_TYPE_SEL_CR10   0x10     /* UARTA,UARTB */
 #define IRQ_TYPE_SEL_CR11   0x11     /* SMI,UARTC,UARTD,WDTO */
 #define GLOBAL_OPTION_CR26  0x26
 #define CR26_LOCK_REG       (1 << 4) /* required to access CR10/CR11 */

+/* LDN 0x07 specific registers */
+#define NCT5104D_GPIO0_IO   0xE0
+#define NCT5104D_GPIO1_IO   0xE4
+
+/* LDN 0x0F specific registers */
+#define NCT5104D_GPIO0_PP_OD   0xE0
+#define NCT5104D_GPIO1_PP_OD   0xE1
+
 /* Logical Device Numbers (LDN). */
 #define NCT5104D_FDC        0x00 /* FDC - not pinned out */
 #define NCT5104D_SP1        0x02 /* UARTA */
 #define NCT5104D_SP2        0x03 /* UARTB */
+#define NCT5104D_GPIO       0x07 /* GPIO In-Out configuration */
 #define NCT5104D_GPIO_WDT   0x08 /* GPIO WDT Interface */
 #define NCT5104D_GPIO_PP_OD 0x0F /* GPIO Push-Pull / Open drain select  */
 #define NCT5104D_SP3        0x10 /* UARTC */
diff --git a/src/superio/nuvoton/nct5104d/superio.c b/src/superio/nuvoton/nct5104d/superio.c
index 40d1200..53eb99d 100644
--- a/src/superio/nuvoton/nct5104d/superio.c
+++ b/src/superio/nuvoton/nct5104d/superio.c
@@ -106,6 +106,19 @@
    pnp_write_config(dev, 0x1c, reg);
 }

+static void reset_gpio(struct device *dev)
+{
+	/* Soft reset GPIOs to default state: IN, Open-drain */
+
+	pnp_write_config(dev, LDN_SELECT_CR07, NCT5104D_GPIO);
+	pnp_write_config(dev, NCT5104D_GPIO0_IO, 0xFF);
+	pnp_write_config(dev, NCT5104D_GPIO1_IO, 0xFF);
+
+	pnp_write_config(dev, LDN_SELECT_CR07, NCT5104D_GPIO_PP_OD);
+	pnp_write_config(dev, NCT5104D_GPIO0_PP_OD, 0xFF);
+	pnp_write_config(dev, NCT5104D_GPIO1_PP_OD, 0xFF);
+}
+
 static void nct5104d_init(struct device *dev)
 {
    struct superio_nuvoton_nct5104d_config *conf = dev->chip_info;
@@ -127,6 +140,7 @@
    	break;
    case NCT5104D_GPIO0:
    case NCT5104D_GPIO1:
+		reset_gpio(dev);
    	route_pins_to_uart(dev, false);
    	break;
    default:

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35482 )

Change subject: src/superio/nuvoton/nct5104d: soft reset GPIO
......................................................................

Patch Set 1:

(5 comments)

https://review.coreboot.org/c/coreboot/+/35482/1/src/superio/nuvoton/nct5104... 
File src/superio/nuvoton/nct5104d/nct5104d.h:

https://review.coreboot.org/c/coreboot/+/35482/1/src/superio/nuvoton/nct5104... 
PS1, Line 24: #define LDN_SELECT_CR07     0x07
unneeded; see my comments on the other file

https://review.coreboot.org/c/coreboot/+/35482/1/src/superio/nuvoton/nct5104... 
PS1, Line 42: #define NCT5104D_GPIO       0x07 /* GPIO In-Out configuration */
already defined in line 50

https://review.coreboot.org/c/coreboot/+/35482/1/src/superio/nuvoton/nct5104... 
File src/superio/nuvoton/nct5104d/superio.c:

https://review.coreboot.org/c/coreboot/+/35482/1/src/superio/nuvoton/nct5104... 
PS1, Line 113: 	pnp_write_config(dev, LDN_SELECT_CR07, NCT5104D_GPIO);
I'd suggest using pnp_set_logical_device here

https://review.coreboot.org/c/coreboot/+/35482/1/src/superio/nuvoton/nct5104... 
PS1, Line 117: 	pnp_write_config(dev, LDN_SELECT_CR07, NCT5104D_GPIO_PP_OD);
same here

https://review.coreboot.org/c/coreboot/+/35482/1/src/superio/nuvoton/nct5104... 
PS1, Line 143: 		reset_gpio(dev);
when for example gpio bank 0 isn't enabled and bank 1 is enabled, bank 0 will also get reset. is this intended?

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35482 )

Change subject: src/superio/nuvoton/nct5104d: soft reset GPIO
......................................................................

Patch Set 1:

(1 comment)

is it also intended that this is done for gpio bank 0 and 1, but not 6 that also seems to be present in the chip?

https://review.coreboot.org/c/coreboot/+/35482/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/35482/1//COMMIT_MSG@11 
PS1, Line 11: uknown
missing an n

[Piotr Kleinschmidt (Code Review)]

Piotr Kleinschmidt has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35482 )

Change subject: src/superio/nuvoton/nct5104d: soft reset GPIO
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35482/1/src/superio/nuvoton/nct5104... 
File src/superio/nuvoton/nct5104d/superio.c:

https://review.coreboot.org/c/coreboot/+/35482/1/src/superio/nuvoton/nct5104... 
PS1, Line 113: 	pnp_write_config(dev, LDN_SELECT_CR07, NCT5104D_GPIO);
> I'd suggest using pnp_set_logical_device here

pnp_set_logical_device will work with LDN 7, but not with LDN F. It's because there isn't LDN F in devictree. Therefore, in nct5104d_init function we don't have access to such dev structure. Is there any other possibility to get to LDN F? Or should I stay with pnp_write_config?

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35482 )

Change subject: src/superio/nuvoton/nct5104d: soft reset GPIO
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35482/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/35482/1//COMMIT_MSG@7 
PS1, Line 7: src/
Remove `src/` from the prefix.

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35482 )

Change subject: src/superio/nuvoton/nct5104d: soft reset GPIO
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35482/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/35482/1//COMMIT_MSG@11 
PS1, Line 11: GPIOS
GPIOs

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35482 )

Change subject: src/superio/nuvoton/nct5104d: soft reset GPIO
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35482/1/src/superio/nuvoton/nct5104... 
File src/superio/nuvoton/nct5104d/superio.c:

https://review.coreboot.org/c/coreboot/+/35482/1/src/superio/nuvoton/nct5104... 
PS1, Line 113: 	pnp_write_config(dev, LDN_SELECT_CR07, NCT5104D_GPIO);
> pnp_set_logical_device will work with LDN 7, but not with LDN F. […]

just add the LDN 0xf to the device's devicetree then

Change in coreboot[master]: superio/nuvoton/nct5104d: soft reset GPIO

[Piotr Kleinschmidt (Code Review)]

Hello Felix Held, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/35482

to look at the new patch set (#2).

Change subject: superio/nuvoton/nct5104d: soft reset GPIO
......................................................................

superio/nuvoton/nct5104d: soft reset GPIO

So far, only hard power off could reset GPIOs state to defaults:
IN, Open-drain. Now, defaults are set with every boot to ensure
that GPIOs are not in unknown/unwanted state.

Change-Id: I67878dbab2ddf0deaaa8f5d79416368c6164ba1d
Signed-off-by: Piotr Kleinschmidt 
---
M src/mainboard/pcengines/apu2/variants/apu2/devicetree.cb
M src/mainboard/pcengines/apu2/variants/apu3/devicetree.cb
M src/mainboard/pcengines/apu2/variants/apu4/devicetree.cb
M src/superio/nuvoton/nct5104d/nct5104d.h
M src/superio/nuvoton/nct5104d/superio.c
5 files changed, 65 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/35482/2

Change in coreboot[master]: superio/nuvoton/nct5104d: soft reset GPIO

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35482 )

Change subject: superio/nuvoton/nct5104d: soft reset GPIO
......................................................................

Patch Set 2:

(2 comments)

please also split the devicetree changes into a separate patch

https://review.coreboot.org/c/coreboot/+/35482/2/src/superio/nuvoton/nct5104... 
File src/superio/nuvoton/nct5104d/superio.c:

https://review.coreboot.org/c/coreboot/+/35482/2/src/superio/nuvoton/nct5104... 
PS2, Line 23: nt gpio0_enabled = 0;
            : int gpio1_enabled = 0;
            : int gpio6_enabled = 0;
this just duplicates information from the devicetree

https://review.coreboot.org/c/coreboot/+/35482/2/src/superio/nuvoton/nct5104... 
PS2, Line 142: 
             : 	if (gpio0_enabled)
             : 		pnp_write_config(dev, NCT5104D_GPIO0_PP_OD, 0xFF);
             : 
             : 	if (gpio1_enabled)
             : 		pnp_write_config(dev, NCT5104D_GPIO1_PP_OD, 0xFF);
             : 
             : 	if (gpio6_enabled)
             : 		pnp_write_config(dev, NCT5104D_GPIO6_PP_OD, 0xFF);
do these writes need to be conditional? if so I'd rather use the information from the devicetree

Change in coreboot[master]: superio/nuvoton/nct5104d: soft reset GPIO

[Piotr Kleinschmidt (Code Review)]

Piotr Kleinschmidt has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35482 )

Change subject: superio/nuvoton/nct5104d: soft reset GPIO
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35482/2/src/superio/nuvoton/nct5104... 
File src/superio/nuvoton/nct5104d/superio.c:

https://review.coreboot.org/c/coreboot/+/35482/2/src/superio/nuvoton/nct5104... 
PS2, Line 142: 
             : 	if (gpio0_enabled)
             : 		pnp_write_config(dev, NCT5104D_GPIO0_PP_OD, 0xFF);
             : 
             : 	if (gpio1_enabled)
             : 		pnp_write_config(dev, NCT5104D_GPIO1_PP_OD, 0xFF);
             : 
             : 	if (gpio6_enabled)
             : 		pnp_write_config(dev, NCT5104D_GPIO6_PP_OD, 0xFF);
> do these writes need to be conditional? if so I'd rather use the information from the devicetree

Yes, they do. We want to reset only enabled gpio banks.

Change in coreboot[master]: superio/nuvoton/nct5104d: Add soft reset GPIO functionality

[Michał Żygowski (Code Review)]

Michał Żygowski has uploaded a new patch set (#3) to the change originally created by Piotr Kleinschmidt. ( https://review.coreboot.org/c/coreboot/+/35482 )

Change subject: superio/nuvoton/nct5104d: Add soft reset GPIO functionality
......................................................................

superio/nuvoton/nct5104d: Add soft reset GPIO functionality

So far, only hard power off could reset GPIOs state to defaults:
IN, Open-drain. Now, defaults are set with every boot to ensure
that GPIOs are not in unknown/unwanted state.

Change-Id: I67878dbab2ddf0deaaa8f5d79416368c6164ba1d
Signed-off-by: Piotr Kleinschmidt 
Signed-off-by: Michał Żygowski 
---
M src/superio/nuvoton/nct5104d/nct5104d.h
M src/superio/nuvoton/nct5104d/superio.c
2 files changed, 59 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/35482/3

Change in coreboot[master]: superio/nuvoton/nct5104d: Add soft reset GPIO functionality

[Michał Żygowski (Code Review)]

Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35482 )

Change subject: superio/nuvoton/nct5104d: Add soft reset GPIO functionality
......................................................................

Patch Set 3:

(9 comments)

https://review.coreboot.org/c/coreboot/+/35482/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/35482/1//COMMIT_MSG@7 
PS1, Line 7: src/
> Remove `src/` from the prefix.

Done

https://review.coreboot.org/c/coreboot/+/35482/1//COMMIT_MSG@11 
PS1, Line 11: GPIOS
> GPIOs

Done

https://review.coreboot.org/c/coreboot/+/35482/1//COMMIT_MSG@11 
PS1, Line 11: uknown
> missing an n

Done

https://review.coreboot.org/c/coreboot/+/35482/1/src/superio/nuvoton/nct5104... 
File src/superio/nuvoton/nct5104d/nct5104d.h:

https://review.coreboot.org/c/coreboot/+/35482/1/src/superio/nuvoton/nct5104... 
PS1, Line 24: #define LDN_SELECT_CR07     0x07
> unneeded; see my comments on the other file

Done

https://review.coreboot.org/c/coreboot/+/35482/1/src/superio/nuvoton/nct5104... 
PS1, Line 42: #define NCT5104D_GPIO       0x07 /* GPIO In-Out configuration */
> already defined in line 50

Done

https://review.coreboot.org/c/coreboot/+/35482/1/src/superio/nuvoton/nct5104... 
File src/superio/nuvoton/nct5104d/superio.c:

https://review.coreboot.org/c/coreboot/+/35482/1/src/superio/nuvoton/nct5104... 
PS1, Line 113: 	pnp_write_config(dev, LDN_SELECT_CR07, NCT5104D_GPIO);
> just add the LDN 0xf to the device's devicetree then

Done in a subsequent patch

https://review.coreboot.org/c/coreboot/+/35482/1/src/superio/nuvoton/nct5104... 
PS1, Line 117: 	pnp_write_config(dev, LDN_SELECT_CR07, NCT5104D_GPIO_PP_OD);
> same here

Done

https://review.coreboot.org/c/coreboot/+/35482/2/src/superio/nuvoton/nct5104... 
File src/superio/nuvoton/nct5104d/superio.c:

https://review.coreboot.org/c/coreboot/+/35482/2/src/superio/nuvoton/nct5104... 
PS2, Line 23: nt gpio0_enabled = 0;
            : int gpio1_enabled = 0;
            : int gpio6_enabled = 0;
> this just duplicates information from the devicetree

Done

https://review.coreboot.org/c/coreboot/+/35482/2/src/superio/nuvoton/nct5104... 
PS2, Line 142: 
             : 	if (gpio0_enabled)
             : 		pnp_write_config(dev, NCT5104D_GPIO0_PP_OD, 0xFF);
             : 
             : 	if (gpio1_enabled)
             : 		pnp_write_config(dev, NCT5104D_GPIO1_PP_OD, 0xFF);
             : 
             : 	if (gpio6_enabled)
             : 		pnp_write_config(dev, NCT5104D_GPIO6_PP_OD, 0xFF);
> Yes, they do. We want to reset only enabled gpio banks.

Done

Change in coreboot[master]: superio/nuvoton/nct5104d: Add soft reset GPIO functionality

[Michał Żygowski (Code Review)]

Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35482 )

Change subject: superio/nuvoton/nct5104d: Add soft reset GPIO functionality
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35482/1/src/superio/nuvoton/nct5104... 
File src/superio/nuvoton/nct5104d/superio.c:

https://review.coreboot.org/c/coreboot/+/35482/1/src/superio/nuvoton/nct5104... 
PS1, Line 143: 		reset_gpio(dev);
> when for example gpio bank 0 isn't enabled and bank 1 is enabled, bank 0 will also get reset. […]

Switch should handle that now

Change in coreboot[master]: superio/nuvoton/nct5104d: Add soft reset GPIO functionality

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35482 )

Change subject: superio/nuvoton/nct5104d: Add soft reset GPIO functionality
......................................................................

Patch Set 3:

Hm, I'm unsure if this functionality is board-dependent or if it something that should be done for every case the SIO will be used in, but I'd guess the former. Also IIRC the functionality added here will overwrite devicetree settings that would be written to the same registers, which might end up being surprising and hard to debug. Haven't really looked into if everything is set up so that setting this via the devicetree would be possible here though; might be an issue that it doesn\t only use registers in the 0xf? range, but also in the 0xe? range. Maybe have a configuration option in the chip.h file to enable this? I'm not sure what would be the best option here. The SIO/devicetree integration isn't that nice and requires a bit too much boilerplate, but that's what we currently have...

Change in coreboot[master]: superio/nuvoton/nct5104d: Add soft reset GPIO functionality

[Michał Żygowski (Code Review)]

Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35482 )

Change subject: superio/nuvoton/nct5104d: Add soft reset GPIO functionality
......................................................................

Patch Set 3:
> Patch Set 3:
>
> Hm, I'm unsure if this functionality is board-dependent or if it something that should be done for every case the SIO will be used in, but I'd guess the former. Also IIRC the functionality added here will overwrite devicetree settings that would be written to the same registers, which might end up being surprising and hard to debug. Haven't really looked into if everything is set up so that setting this via the devicetree would be possible here though; might be an issue that it doesn\t only use registers in the 0xf? range, but also in the 0xe? range. Maybe have a configuration option in the chip.h file to enable this? I'm not sure what would be the best option here. The SIO/devicetree integration isn't that nice and requires a bit too much boilerplate, but that's what we currently have...

This is rather SIO specific. The GPIO configuration is maintained after LPC_RESET/SIO_RESET and kept by VSB(standby power supply). The registers are reset only by RSMRST (not documented in datasheet at all..). If a user plays with GPIOs in the userspace, the GPIO states are maintained after reboot. See for reference:
https://github.com/pcengines/coreboot/issues/314

Research conducted long time ago by us:
https://cloud.3mdeb.com/index.php/s/4iRWE4Xfse3rD2T

I think it's not apu2 specific. The same behavior occurred on apu1 which is an entirely different processor. The datasheet of the NCT5104d can be also found in the public. I leave the judgment to you.

The devicetree integration is not so perfect because of the mixed LDN functionalities Nuvoton introduced in this chip. I also think this way: this chip will not be used in new designs so probably there are only apu1 and apu2 boards which need this mechanism. This boilerplate will be required only for these devices. But if you decide to keep it board-specific, I'm ok with that and will update the patch.

Change in coreboot[master]: superio/nuvoton/nct5104d: Add soft reset GPIO functionality

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35482 )

Change subject: superio/nuvoton/nct5104d: Add soft reset GPIO functionality
......................................................................

Patch Set 3: Code-Review+2

I can't say that I'm completely 100% happy with this, but since setting the pin state to input is a safe default, I have no strong objections to this patch. I just hope that this won't cause someone else to spent too much time on figuring out why this might do something that they don't necessarily expect. In that case this can be mad configurable by an option in the chip.h file of this SIO (not sure if it exists for this specific chip, but for others it does exist) and enabled via a devicetree setting for the boards that want/need this functionality.
If you want, you could add that as a patch on top of this patch train, so that the boards can explicitly enable this behaviour.

Change in coreboot[master]: superio/nuvoton/nct5104d: Add soft reset GPIO functionality

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35482 )

Change subject: superio/nuvoton/nct5104d: Add soft reset GPIO functionality
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35482/3/src/superio/nuvoton/nct5104... 
File src/superio/nuvoton/nct5104d/superio.c:

https://review.coreboot.org/c/coreboot/+/35482/3/src/superio/nuvoton/nct5104... 
PS3, Line 167: conf->irq_trigger_type
something like this and the corresponding variable in chip.h and devicetree setting is what i meant in my last comment. reset_gpio_state might be a good name for that option :)

Change in coreboot[master]: superio/nuvoton/nct5104d: Add soft reset GPIO functionality

[Michał Żygowski (Code Review)]

Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35482 )

Change subject: superio/nuvoton/nct5104d: Add soft reset GPIO functionality
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35482/3/src/superio/nuvoton/nct5104... 
File src/superio/nuvoton/nct5104d/superio.c:

https://review.coreboot.org/c/coreboot/+/35482/3/src/superio/nuvoton/nct5104... 
PS3, Line 167: conf->irq_trigger_type
> something like this and the corresponding variable in chip. […]

Okay, I will push new patches on top then.

Change in coreboot[master]: superio/nuvoton/nct5104d: Add soft reset GPIO functionality

[Felix Held (Code Review)]

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35482 )

Change subject: superio/nuvoton/nct5104d: Add soft reset GPIO functionality
......................................................................

superio/nuvoton/nct5104d: Add soft reset GPIO functionality

So far, only hard power off could reset GPIOs state to defaults:
IN, Open-drain. Now, defaults are set with every boot to ensure
that GPIOs are not in unknown/unwanted state.

Change-Id: I67878dbab2ddf0deaaa8f5d79416368c6164ba1d
Signed-off-by: Piotr Kleinschmidt 
Signed-off-by: Michał Żygowski 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/35482
Tested-by: build bot (Jenkins) 
Reviewed-by: Felix Held 
---
M src/superio/nuvoton/nct5104d/nct5104d.h
M src/superio/nuvoton/nct5104d/superio.c
2 files changed, 59 insertions(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Felix Held: Looks good to me, approved

diff --git a/src/superio/nuvoton/nct5104d/nct5104d.h b/src/superio/nuvoton/nct5104d/nct5104d.h
index 707e94a..9a88105 100644
--- a/src/superio/nuvoton/nct5104d/nct5104d.h
+++ b/src/superio/nuvoton/nct5104d/nct5104d.h
@@ -26,6 +26,16 @@
 #define GLOBAL_OPTION_CR26  0x26
 #define CR26_LOCK_REG       (1 << 4) /* required to access CR10/CR11 */

+/* LDN 0x07 specific registers */
+#define NCT5104D_GPIO0_IO   0xE0
+#define NCT5104D_GPIO1_IO   0xE4
+#define NCT5104D_GPIO6_IO   0xF8
+
+/* LDN 0x0F specific registers */
+#define NCT5104D_GPIO0_PP_OD   0xE0
+#define NCT5104D_GPIO1_PP_OD   0xE1
+#define NCT5104D_GPIO6_PP_OD   0xE6
+
 /* Logical Device Numbers (LDN). */
 #define NCT5104D_FDC        0x00 /* FDC - not pinned out */
 #define NCT5104D_SP1        0x02 /* UARTA */
diff --git a/src/superio/nuvoton/nct5104d/superio.c b/src/superio/nuvoton/nct5104d/superio.c
index 493e0ce..69f54a7 100644
--- a/src/superio/nuvoton/nct5104d/superio.c
+++ b/src/superio/nuvoton/nct5104d/superio.c
@@ -106,6 +106,47 @@
    pnp_write_config(dev, 0x1c, reg);
 }

+static void reset_gpio_default_in(struct device *dev)
+{
+	pnp_set_logical_device(dev);
+
+	/* Soft reset GPIOs to default state: IN */
+	switch (dev->path.pnp.device) {
+	case NCT5104D_GPIO0:
+		pnp_write_config(dev, NCT5104D_GPIO0_IO, 0xFF);
+		break;
+	case NCT5104D_GPIO1:
+		pnp_write_config(dev, NCT5104D_GPIO1_IO, 0xFF);
+		break;
+	case NCT5104D_GPIO6:
+		pnp_write_config(dev, NCT5104D_GPIO6_IO, 0xFF);
+		break;
+	default:
+		break;
+	}
+}
+
+static void reset_gpio_default_od(struct device *dev)
+{
+	struct device *gpio0, *gpio1, *gpio6;
+
+	gpio0 = dev_find_slot_pnp(dev->path.pnp.port, NCT5104D_GPIO0);
+	gpio1 = dev_find_slot_pnp(dev->path.pnp.port, NCT5104D_GPIO1);
+	gpio6 = dev_find_slot_pnp(dev->path.pnp.port, NCT5104D_GPIO6);
+
+	pnp_set_logical_device(dev);
+
+	/* Soft reset GPIOs to default state: Open-drain */
+	if (gpio0 && gpio0->enabled)
+		pnp_write_config(dev, NCT5104D_GPIO0_PP_OD, 0xFF);
+
+	if (gpio1 && gpio1->enabled)
+		pnp_write_config(dev, NCT5104D_GPIO1_PP_OD, 0xFF);
+
+	if (gpio6 && gpio6->enabled)
+		pnp_write_config(dev, NCT5104D_GPIO6_PP_OD, 0xFF);
+}
+
 static void nct5104d_init(struct device *dev)
 {
    struct superio_nuvoton_nct5104d_config *conf = dev->chip_info;
@@ -128,6 +169,13 @@
    case NCT5104D_GPIO0:
    case NCT5104D_GPIO1:
    	route_pins_to_uart(dev, false);
+		reset_gpio_default_in(dev);
+		break;
+	case NCT5104D_GPIO6:
+		reset_gpio_default_in(dev);
+		break;
+	case NCT5104D_GPIO_PP_OD:
+		reset_gpio_default_od(dev);
    	break;
    default:
    	break;
@@ -152,10 +200,10 @@
    { NULL, NCT5104D_SP3,  PNP_IO0 | PNP_IRQ0, 0x07f8, },
    { NULL, NCT5104D_SP4,  PNP_IO0 | PNP_IRQ0, 0x07f8, },
    { NULL, NCT5104D_GPIO_WDT},
-	{ NULL, NCT5104D_GPIO_PP_OD},
    { NULL, NCT5104D_GPIO0},
    { NULL, NCT5104D_GPIO1},
    { NULL, NCT5104D_GPIO6},
+	{ NULL, NCT5104D_GPIO_PP_OD},
    { NULL, NCT5104D_PORT80},
 };