Change in coreboot[master]: src/superio/nuvoton/nct5104d: assign IO port range to control GPIO
Piotr Kleinschmidt has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35849 )
Change subject: src/superio/nuvoton/nct5104d: assign IO port range to control GPIO ......................................................................
src/superio/nuvoton/nct5104d: assign IO port range to control GPIO
SuperIO GPIOs can be also controled directly through access to I/O register, now.
Change-Id: I4ce99bb44e6f5db684170f4190bdc38a944849f6 Signed-off-by: Piotr Kleinschmidt piotr.kleins@gmail.com --- M src/mainboard/pcengines/apu1/devicetree.cb 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/mainboard/pcengines/apu2/variants/apu5/devicetree.cb M src/superio/nuvoton/nct5104d/chip.h M src/superio/nuvoton/nct5104d/superio.c 7 files changed, 84 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/35849/1
diff --git a/src/mainboard/pcengines/apu1/devicetree.cb b/src/mainboard/pcengines/apu1/devicetree.cb index 2e8b8f4..ffbaa69 100644 --- a/src/mainboard/pcengines/apu1/devicetree.cb +++ b/src/mainboard/pcengines/apu1/devicetree.cb @@ -43,6 +43,7 @@ device pci 14.3 on # LPC 0x439d chip superio/nuvoton/nct5104d register "irq_trigger_type" = "0" + register "enable_wdt1" = "0" device pnp 2e.0 off end device pnp 2e.2 on io 0x60 = 0x3f8 @@ -62,7 +63,9 @@ io 0x60 = 0x2e8 irq 0x70 = 3 end - device pnp 2e.8 off end + device pnp 2e.8 on + io 0x60 = 0x220 + end device pnp 2e.f off end # GPIO0 and GPIO1 are conditionally turned on device pnp 2e.007 off end diff --git a/src/mainboard/pcengines/apu2/variants/apu2/devicetree.cb b/src/mainboard/pcengines/apu2/variants/apu2/devicetree.cb index 6728228..0fff2f8 100644 --- a/src/mainboard/pcengines/apu2/variants/apu2/devicetree.cb +++ b/src/mainboard/pcengines/apu2/variants/apu2/devicetree.cb @@ -45,6 +45,7 @@ device pci 14.3 on # LPC 0x439d chip superio/nuvoton/nct5104d # SIO NCT5104D register "irq_trigger_type" = "0" + register "enable_wdt1" = "0" device pnp 2e.0 off end device pnp 2e.2 on io 0x60 = 0x3f8 @@ -64,7 +65,9 @@ io 0x60 = 0x2e8 irq 0x70 = 3 end - device pnp 2e.8 off end + device pnp 2e.8 on + io 0x60 = 0x220 + end device pnp 2e.f off end # GPIO0 and GPIO1 are conditionally turned on device pnp 2e.007 on end diff --git a/src/mainboard/pcengines/apu2/variants/apu3/devicetree.cb b/src/mainboard/pcengines/apu2/variants/apu3/devicetree.cb index 0c0c21e..bb45fdc 100644 --- a/src/mainboard/pcengines/apu2/variants/apu3/devicetree.cb +++ b/src/mainboard/pcengines/apu2/variants/apu3/devicetree.cb @@ -45,6 +45,7 @@ device pci 14.3 on # LPC 0x439d chip superio/nuvoton/nct5104d # SIO NCT5104D register "irq_trigger_type" = "0" + register "enable_wdt1" = "0" device pnp 2e.0 off end device pnp 2e.2 on io 0x60 = 0x3f8 @@ -64,7 +65,9 @@ io 0x60 = 0x2e8 irq 0x70 = 3 end - device pnp 2e.8 off end + device pnp 2e.8 on + io 0x60 = 0x220 + end device pnp 2e.f off end # GPIO0 and GPIO1 are conditionally turned on device pnp 2e.007 on end diff --git a/src/mainboard/pcengines/apu2/variants/apu4/devicetree.cb b/src/mainboard/pcengines/apu2/variants/apu4/devicetree.cb index c93c04f..1947ce7 100644 --- a/src/mainboard/pcengines/apu2/variants/apu4/devicetree.cb +++ b/src/mainboard/pcengines/apu2/variants/apu4/devicetree.cb @@ -45,6 +45,7 @@ device pci 14.3 on # LPC 0x439d chip superio/nuvoton/nct5104d # SIO NCT5104D register "irq_trigger_type" = "0" + register "enable_wdt1" = "0" device pnp 2e.0 off end device pnp 2e.2 on io 0x60 = 0x3f8 @@ -64,7 +65,9 @@ io 0x60 = 0x2e8 irq 0x70 = 3 end - device pnp 2e.8 off end + device pnp 2e.8 on + io 0x60 = 0x220 + end device pnp 2e.f off end # GPIO0 and GPIO1 are conditionally turned on device pnp 2e.007 on end diff --git a/src/mainboard/pcengines/apu2/variants/apu5/devicetree.cb b/src/mainboard/pcengines/apu2/variants/apu5/devicetree.cb index b6b22cf..4afe265 100644 --- a/src/mainboard/pcengines/apu2/variants/apu5/devicetree.cb +++ b/src/mainboard/pcengines/apu2/variants/apu5/devicetree.cb @@ -45,6 +45,7 @@ device pci 14.3 on # LPC 0x439d chip superio/nuvoton/nct5104d # SIO NCT5104D register "irq_trigger_type" = "0" + register "enable_wdt1" = "0" device pnp 2e.0 off end device pnp 2e.2 on io 0x60 = 0x3f8 @@ -64,7 +65,9 @@ io 0x60 = 0x2e8 irq 0x70 = 3 end - device pnp 2e.8 off end + device pnp 2e.8 on + io 0x60 = 0x220 + end device pnp 2e.f off end device pnp 2e.007 off end device pnp 2e.107 off end diff --git a/src/superio/nuvoton/nct5104d/chip.h b/src/superio/nuvoton/nct5104d/chip.h index d351053..9083dab 100644 --- a/src/superio/nuvoton/nct5104d/chip.h +++ b/src/superio/nuvoton/nct5104d/chip.h @@ -19,6 +19,7 @@
struct superio_nuvoton_nct5104d_config { u8 irq_trigger_type; + u8 enable_wdt1; };
#endif diff --git a/src/superio/nuvoton/nct5104d/superio.c b/src/superio/nuvoton/nct5104d/superio.c index 40d1200..9e5f754 100644 --- a/src/superio/nuvoton/nct5104d/superio.c +++ b/src/superio/nuvoton/nct5104d/superio.c @@ -15,10 +15,12 @@ */
#include <device/pnp.h> +#include <device/device.h> #include <superio/conf_mode.h> #include <stdlib.h> #include "nct5104d.h" #include "chip.h" +#include "console/console.h"
static void set_irq_trigger_type(struct device *dev, bool trig_level) { @@ -106,6 +108,63 @@ pnp_write_config(dev, 0x1c, reg); }
+static void enable_gpio_io_port(struct device *dev, u8 enable_wdt1) +{ + u8 reg; + u16 io_base_address; + u8 uartc_enabled, uartd_enabled; + u8 sio_port; + + sio_port = dev->path.pnp.port; + + pnp_write_config(dev, 0x07, NCT5104D_GPIO_WDT); + + /* Devictree always sets LDN 8 CR30.0 bit (WDT1 enable) + * See if user really wants Watchdog Timer 1 to be enabled + */ + + if(enable_wdt1 != 0) { + reg = pnp_read_config(dev, 0x30); + pnp_write_config(dev, 0x30, reg & 0xFE); + } + + /* Check if UARTC and UARTD are both enabled + * If they are, don't activate GPIO Address Mode + * In any other case - activate GPIO Address Mode + */ + + struct device *uart; + + uart = dev_find_slot_pnp(sio_port, NCT5104D_SP3); + uartc_enabled = uart->enabled; + + uart = dev_find_slot_pnp(sio_port, NCT5104D_SP4); + uartd_enabled = uart->enabled; + + if (!uartc_enabled || !uartd_enabled) { + + /* Check if LDN 8 CR60 and CR61 contain valid IO Base Address + * IO Base Address <100h ; FF8h> + */ + + pnp_write_config(dev, 0x07, NCT5104D_GPIO_WDT); + + io_base_address = pnp_read_config(dev, 0x61); + io_base_address |= (pnp_read_config(dev, 0x60) << 8); + + if (io_base_address < 0x100 || io_base_address > 0xFF8) { + printk(BIOS_ERR, "Invalid io base address %x != " + "<100h ; FF8h> \n", io_base_address); + + return; + } + + /* Set LDN 8 CR 30.1 to activate GPIO Address Mode */ + reg = pnp_read_config(dev, 0x30); + pnp_write_config(dev, 0x30, reg | 0x02); + } +} + static void nct5104d_init(struct device *dev) { struct superio_nuvoton_nct5104d_config *conf = dev->chip_info; @@ -129,6 +188,9 @@ case NCT5104D_GPIO1: route_pins_to_uart(dev, false); break; + case NCT5104D_GPIO_WDT: + enable_gpio_io_port(dev, conf->enable_wdt1); + break; default: break; } @@ -151,7 +213,7 @@ { NULL, NCT5104D_SP2, PNP_IO0 | PNP_IRQ0, 0x07f8, }, { NULL, NCT5104D_SP3, PNP_IO0 | PNP_IRQ0, 0x07f8, }, { NULL, NCT5104D_SP4, PNP_IO0 | PNP_IRQ0, 0x07f8, }, - { NULL, NCT5104D_GPIO_WDT}, + { NULL, NCT5104D_GPIO_WDT, PNP_IO0, 0x07f8, }, { NULL, NCT5104D_GPIO_PP_OD}, { NULL, NCT5104D_GPIO0}, { NULL, NCT5104D_GPIO1},
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35849 )
Change subject: src/superio/nuvoton/nct5104d: assign IO port range to control GPIO ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... File src/superio/nuvoton/nct5104d/superio.c:
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... PS1, Line 124: */ trailing whitespace
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... PS1, Line 126: if(enable_wdt1 != 0) { space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... PS1, Line 157: "<100h ; FF8h> \n", io_base_address); unnecessary whitespace before a quoted newline
Piotr Kleinschmidt has removed Piotr Król from this change. ( https://review.coreboot.org/c/coreboot/+/35849 )
Change subject: src/superio/nuvoton/nct5104d: assign IO port range to control GPIO ......................................................................
Removed reviewer Piotr Król.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35849 )
Change subject: src/superio/nuvoton/nct5104d: assign IO port range to control GPIO ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... File src/superio/nuvoton/nct5104d/superio.c:
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... PS1, Line 120: pnp_write_config(dev, 0x07, NCT5104D_GPIO_WDT); use pnp_set_logical_device from device/pnp_ops.h instead
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... PS1, Line 150: pnp_write_config(dev, 0x07, NCT5104D_GPIO_WDT); use pnp_set_logical_device
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... PS1, Line 152: io_base_address = pnp_read_config(dev, 0x61); : io_base_address |= (pnp_read_config(dev, 0x60) << 8); use pnp_read_iobase instead
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35849 )
Change subject: src/superio/nuvoton/nct5104d: assign IO port range to control GPIO ......................................................................
Patch Set 1: Code-Review-1
(2 comments)
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... File src/superio/nuvoton/nct5104d/superio.c:
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... PS1, Line 127: reg = pnp_read_config(dev, 0x30); : pnp_write_config(dev, 0x30, reg & 0xFE); this should be a pnp_set_enable call; this duplicates functionality that could be done with a device tree setting for the WDT in the SIO in the devicetree
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... PS1, Line 163: reg = pnp_read_config(dev, 0x30); : pnp_write_config(dev, 0x30, reg | 0x02); this also does what pnp_set_enable does
Piotr Kleinschmidt has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35849 )
Change subject: src/superio/nuvoton/nct5104d: assign IO port range to control GPIO ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... File src/superio/nuvoton/nct5104d/superio.c:
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... PS1, Line 127: reg = pnp_read_config(dev, 0x30); : pnp_write_config(dev, 0x30, reg & 0xFE);
this should be a pnp_set_enable call; this duplicates functionality that could be done with a device […]
With a device tree that bit is always set to "1", but the goal of that function is not to enable WDT. That's why there is additional variable 'enable_wdt1' in device tree, by which user really decides if WDT needs to be enabled or disabled.
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... PS1, Line 152: io_base_address = pnp_read_config(dev, 0x61); : io_base_address |= (pnp_read_config(dev, 0x60) << 8);
use pnp_read_iobase instead
In entire file, functions from device/pnp.h are used. Therefore, if I want to include device/pnp_ops.h (which contains pnp_read_iobase function), there are conflicts because both files now contain functions with the same names. To avoid that I prefer still use device/pnp.h and just add pnp_read_iobase function to it. Is that will be ok?
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... PS1, Line 163: reg = pnp_read_config(dev, 0x30); : pnp_write_config(dev, 0x30, reg | 0x02);
this also does what pnp_set_enable does
pnp_set_enable modifies CR30.0 bit. I need to set CR30.1 bit to enable GPIO Base Address mode
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35849 )
Change subject: src/superio/nuvoton/nct5104d: assign IO port range to control GPIO ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35849/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35849/1//COMMIT_MSG@7 PS1, Line 7: src/superio/nuvoton/nct5104d: assign IO port range to control GPIO Please remove `src/` from the prefix.
https://review.coreboot.org/c/coreboot/+/35849/1//COMMIT_MSG@10 PS1, Line 10: to I/O register, now. Now, Super I/O GPIOs can also be controlled directly through access to I/O registers.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35849 )
Change subject: src/superio/nuvoton/nct5104d: assign IO port range to control GPIO ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... File src/superio/nuvoton/nct5104d/superio.c:
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... PS1, Line 127: reg = pnp_read_config(dev, 0x30); : pnp_write_config(dev, 0x30, reg & 0xFE);
With a device tree that bit is always set to "1", but the goal of that function is not to enable WDT […]
ah, so this is to make that setting user-configurable during runtime instead of the devicetree setting that happens at build-time?
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... PS1, Line 152: io_base_address = pnp_read_config(dev, 0x61); : io_base_address |= (pnp_read_config(dev, 0x60) << 8);
In entire file, functions from device/pnp.h are used. […]
oh, pnp_read_iobase is only for the ENV_PNP_SIMPLE_DEVICE case and not ramstage where this code runs. The better solution is probably to check if the devicetree setting for the IO region is within the expected range; when the virtual/real LDN is enabled there, the IO region configuration from there gets written to the chip's register automatically, so no need to read it back from the chip after the devicetree setting got applied there.
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... PS1, Line 163: reg = pnp_read_config(dev, 0x30); : pnp_write_config(dev, 0x30, reg | 0x02);
pnp_set_enable modifies CR30.0 bit. I need to set CR30. […]
pnp_set_enable flips the bit corresponding to the virtual LDN specified by the struct device *dev parameter. that only the base ldn can be enabled/disabled is only a limitation for the simple device case; the function in ramstage we're using here can flip the enable bits for all virtual LDNs.
Piotr Kleinschmidt has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35849 )
Change subject: src/superio/nuvoton/nct5104d: assign IO port range to control GPIO ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... File src/superio/nuvoton/nct5104d/superio.c:
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... PS1, Line 127: reg = pnp_read_config(dev, 0x30); : pnp_write_config(dev, 0x30, reg & 0xFE);
ah, so this is to make that setting user-configurable during runtime instead of the devicetree setti […]
Not exactly. So far, I thought if in devictree Logical Device is turned on, always Control Register 30h bit 0 is set. For LD 8 it is WDT1, but what I want to enable/disable is only CR30h bit 1 (GPIO Base Address Mode). In the comment below you explain that actually any bit can be enabled/disabled according to devictree settings. Therefore, WDT1 bit won't be affected at all, so the above functionality is not needed.
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... PS1, Line 152: io_base_address = pnp_read_config(dev, 0x61); : io_base_address |= (pnp_read_config(dev, 0x60) << 8);
oh, pnp_read_iobase is only for the ENV_PNP_SIMPLE_DEVICE case and not ramstage where this code runs […]
So, as I understand correctly, that function should only check if uartc and uartd are enabled (and possibly disable GPIO Base Address Mode then) because every chip's register, defined with devictree, is already set at this stage?
Hello Felix Held, build bot (Jenkins), Michał Żygowski,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35849
to look at the new patch set (#2).
Change subject: superio/nuvoton/nct5104d: assign IO port range to control GPIO ......................................................................
superio/nuvoton/nct5104d: assign IO port range to control GPIO
Now, Super I/O GPIOs can also be controlled directly through access to I/O registers. LDN 8 and specific I/O port from range <100h; ff9h> need to be enabled in mainboard devictree.
Change-Id: I4ce99bb44e6f5db684170f4190bdc38a944849f6 Signed-off-by: Piotr Kleinschmidt piotr.kleinschmidt@3mdeb.com --- M src/device/pnp_device.c M src/include/device/pnp.h M src/mainboard/pcengines/apu1/devicetree.cb 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/mainboard/pcengines/apu2/variants/apu5/devicetree.cb M src/superio/nuvoton/nct5104d/chip.h M src/superio/nuvoton/nct5104d/nct5104d.h M src/superio/nuvoton/nct5104d/superio.c 10 files changed, 82 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/35849/2
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35849 )
Change subject: superio/nuvoton/nct5104d: assign IO port range to control GPIO ......................................................................
Patch Set 2:
(5 comments)
please split into 3 patches: one with the pnp device change, one with the devicetree changes and one with the sio changes
https://review.coreboot.org/c/coreboot/+/35849/2/src/device/pnp_device.c File src/device/pnp_device.c:
https://review.coreboot.org/c/coreboot/+/35849/2/src/device/pnp_device.c@90 PS2, Line 90: , index parameter missing; see pnp_set_iobase. without this, this would only work for the 1st io range of a ldn
https://review.coreboot.org/c/coreboot/+/35849/2/src/include/device/pnp.h File src/include/device/pnp.h:
https://review.coreboot.org/c/coreboot/+/35849/2/src/include/device/pnp.h@19 PS2, Line 19: , index parameter missing
https://review.coreboot.org/c/coreboot/+/35849/2/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu1/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/35849/2/src/mainboard/pcengines/apu... PS2, Line 65: device pnp 2e.108 on don't remove "device pnp 2e.8 off end". same for the variants devicetrees
https://review.coreboot.org/c/coreboot/+/35849/2/src/superio/nuvoton/nct5104... File src/superio/nuvoton/nct5104d/superio.c:
https://review.coreboot.org/c/coreboot/+/35849/2/src/superio/nuvoton/nct5104... PS2, Line 111: static void disable_gpio_io_port(struct device *dev) this function seems to be rather board-specific to me. also why disable the io-maped gpio interface anyway? the gpio/serial port selection for the pins is via a multiplexer controlled in route_pins_to_uart via some registers that can't be written by the io-mapped gpio control
https://review.coreboot.org/c/coreboot/+/35849/2/src/superio/nuvoton/nct5104... PS2, Line 208: { NULL, NCT5104D_GPIO_IO, PNP_IO0, 0x07f8, }, { NULL, NCT5104D_GPIO_WDT}, shouldn't be removed here; { NULL, NCT5104D_GPIO_IO, PNP_IO0, 0x07f8, } should be additional to that
Piotr Kleinschmidt has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35849 )
Change subject: superio/nuvoton/nct5104d: assign IO port range to control GPIO ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35849/2/src/superio/nuvoton/nct5104... File src/superio/nuvoton/nct5104d/superio.c:
https://review.coreboot.org/c/coreboot/+/35849/2/src/superio/nuvoton/nct5104... PS2, Line 111: static void disable_gpio_io_port(struct device *dev)
this function seems to be rather board-specific to me. […]
I wasn't sure if io-mapped gpio won't collide in some cases if uart is set. However, as you said it won't, io-mapped can be always enabled and therefore that function is not needed anymore.
Michał Żygowski has uploaded a new patch set (#3) to the change originally created by Piotr Kleinschmidt. ( https://review.coreboot.org/c/coreboot/+/35849 )
Change subject: superio/nuvoton/nct5104d: Add virtual LDN for simple GPIO IO control ......................................................................
superio/nuvoton/nct5104d: Add virtual LDN for simple GPIO IO control
Now, Super I/O GPIOs can also be controlled directly through access to I/O registers. VLDN 108 and specific I/O port from range <100h; ff9h> may be enabled in mainboard devictree.
Change-Id: I4ce99bb44e6f5db684170f4190bdc38a944849f6 Signed-off-by: Piotr Kleinschmidt piotr.kleinschmidt@3mdeb.com Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com --- M src/superio/nuvoton/nct5104d/nct5104d.h M src/superio/nuvoton/nct5104d/superio.c 2 files changed, 29 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/35849/3
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35849 )
Change subject: superio/nuvoton/nct5104d: Add virtual LDN for simple GPIO IO control ......................................................................
Patch Set 3:
(11 comments)
https://review.coreboot.org/c/coreboot/+/35849/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35849/1//COMMIT_MSG@7 PS1, Line 7: src/superio/nuvoton/nct5104d: assign IO port range to control GPIO
Please remove `src/` from the prefix.
Done
https://review.coreboot.org/c/coreboot/+/35849/1//COMMIT_MSG@10 PS1, Line 10: to I/O register, now.
Now, Super I/O GPIOs can also be controlled directly through access to I/O registers.
Done
https://review.coreboot.org/c/coreboot/+/35849/2/src/device/pnp_device.c File src/device/pnp_device.c:
https://review.coreboot.org/c/coreboot/+/35849/2/src/device/pnp_device.c@90 PS2, Line 90: ,
index parameter missing; see pnp_set_iobase. […]
Done through generic pnp resources PNP_IO0, so not needed anymore...
https://review.coreboot.org/c/coreboot/+/35849/2/src/include/device/pnp.h File src/include/device/pnp.h:
https://review.coreboot.org/c/coreboot/+/35849/2/src/include/device/pnp.h@19 PS2, Line 19: ,
index parameter missing
As above
https://review.coreboot.org/c/coreboot/+/35849/2/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu1/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/35849/2/src/mainboard/pcengines/apu... PS2, Line 65: device pnp 2e.108 on
don't remove "device pnp 2e.8 off end". […]
Done
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... File src/superio/nuvoton/nct5104d/superio.c:
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... PS1, Line 120: pnp_write_config(dev, 0x07, NCT5104D_GPIO_WDT);
use pnp_set_logical_device from device/pnp_ops. […]
Done
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... PS1, Line 150: pnp_write_config(dev, 0x07, NCT5104D_GPIO_WDT);
use pnp_set_logical_device
Done
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... PS1, Line 152: io_base_address = pnp_read_config(dev, 0x61); : io_base_address |= (pnp_read_config(dev, 0x60) << 8);
So, as I understand correctly, that function should only check if uartc and uartd are enabled (and p […]
Reimplemented the check without pnp_read_iobase
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... PS1, Line 163: reg = pnp_read_config(dev, 0x30); : pnp_write_config(dev, 0x30, reg | 0x02);
pnp_set_enable flips the bit corresponding to the virtual LDN specified by the struct device *dev pa […]
Used struct device for the VLDN to get it disabled if needed via nct5104d_init
https://review.coreboot.org/c/coreboot/+/35849/2/src/superio/nuvoton/nct5104... File src/superio/nuvoton/nct5104d/superio.c:
https://review.coreboot.org/c/coreboot/+/35849/2/src/superio/nuvoton/nct5104... PS2, Line 111: static void disable_gpio_io_port(struct device *dev)
I wasn't sure if io-mapped gpio won't collide in some cases if uart is set. […]
I have decided to leave this function in place due to the possibility of devicetree + Kconfig (for apu board APU2_PINMUX_*) settings mismatch. I do not feel leaving IO port open for GPIOs when none of them are available, but VLDN 108 still enabled. Let me know what you think.
https://review.coreboot.org/c/coreboot/+/35849/2/src/superio/nuvoton/nct5104... PS2, Line 208: { NULL, NCT5104D_GPIO_IO, PNP_IO0, 0x07f8, },
{ NULL, NCT5104D_GPIO_WDT}, shouldn't be removed here; { NULL, NCT5104D_GPIO_IO, PNP_IO0, 0x07f8, } […]
Done
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35849 )
Change subject: superio/nuvoton/nct5104d: Add virtual LDN for simple GPIO IO control ......................................................................
Patch Set 3: Code-Review+1
(1 comment)
looks good to me; printing a warning when the gpio i/o space interface gets disabled when no gpio port is enabled would be good though
https://review.coreboot.org/c/coreboot/+/35849/3/src/superio/nuvoton/nct5104... File src/superio/nuvoton/nct5104d/superio.c:
https://review.coreboot.org/c/coreboot/+/35849/3/src/superio/nuvoton/nct5104... PS3, Line 169: } maybe also print a warning?
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35849 )
Change subject: superio/nuvoton/nct5104d: Add virtual LDN for simple GPIO IO control ......................................................................
Patch Set 3:
looks good to me; printing a warning when the gpio i/o space interface gets disabled when no gpio port is enabled would be good though
do you want to add this in a patch on top of this one or add it to this patch? both fine with me
Michał Żygowski has uploaded a new patch set (#4) to the change originally created by Piotr Kleinschmidt. ( https://review.coreboot.org/c/coreboot/+/35849 )
Change subject: superio/nuvoton/nct5104d: Add virtual LDN for simple GPIO IO control ......................................................................
superio/nuvoton/nct5104d: Add virtual LDN for simple GPIO IO control
Now, Super I/O GPIOs can also be controlled directly through access to I/O registers. VLDN 108 and specific I/O port from range <100h; ff9h> may be enabled in mainboard devictree.
Change-Id: I4ce99bb44e6f5db684170f4190bdc38a944849f6 Signed-off-by: Piotr Kleinschmidt piotr.kleinschmidt@3mdeb.com Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com --- M src/superio/nuvoton/nct5104d/nct5104d.h M src/superio/nuvoton/nct5104d/superio.c 2 files changed, 33 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/35849/4
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35849 )
Change subject: superio/nuvoton/nct5104d: Add virtual LDN for simple GPIO IO control ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... File src/superio/nuvoton/nct5104d/superio.c:
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... PS1, Line 127: reg = pnp_read_config(dev, 0x30); : pnp_write_config(dev, 0x30, reg & 0xFE);
Not exactly. […]
Ack
https://review.coreboot.org/c/coreboot/+/35849/2/src/superio/nuvoton/nct5104... File src/superio/nuvoton/nct5104d/superio.c:
https://review.coreboot.org/c/coreboot/+/35849/2/src/superio/nuvoton/nct5104... PS2, Line 111: static void disable_gpio_io_port(struct device *dev)
I have decided to leave this function in place due to the possibility of devicetree + Kconfig (for a […]
Ack
https://review.coreboot.org/c/coreboot/+/35849/3/src/superio/nuvoton/nct5104... File src/superio/nuvoton/nct5104d/superio.c:
https://review.coreboot.org/c/coreboot/+/35849/3/src/superio/nuvoton/nct5104... PS3, Line 169: }
maybe also print a warning?
Done
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35849 )
Change subject: superio/nuvoton/nct5104d: Add virtual LDN for simple GPIO IO control ......................................................................
Patch Set 4: Code-Review+2
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35849 )
Change subject: superio/nuvoton/nct5104d: Add virtual LDN for simple GPIO IO control ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35849/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35849/4//COMMIT_MSG@11 PS4, Line 11: ff9h the code says 0x7f8
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35849 )
Change subject: superio/nuvoton/nct5104d: Add virtual LDN for simple GPIO IO control ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35849/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35849/4//COMMIT_MSG@11 PS4, Line 11: ff9h
the code says 0x7f8
Isn't it a mask for the IO base that specify the length of the resource to be allocated?
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35849 )
Change subject: superio/nuvoton/nct5104d: Add virtual LDN for simple GPIO IO control ......................................................................
Patch Set 4:
(1 comment)
would be good if you can the commit message then i'll submit the patch
https://review.coreboot.org/c/coreboot/+/35849/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35849/4//COMMIT_MSG@11 PS4, Line 11: ff9h
Isn't it a mask for the IO base that specify the length of the resource to be allocated?
yep, the io mask specifies the resource length and the maximum address where it can be mapped. the 9 at the end in the commit message is certainly wrong, since the resource size is 8 and it needs to be aligned to a multiple of the size so i expect an 8 there
Michał Żygowski has uploaded a new patch set (#5) to the change originally created by Piotr Kleinschmidt. ( https://review.coreboot.org/c/coreboot/+/35849 )
Change subject: superio/nuvoton/nct5104d: Add virtual LDN for simple GPIO IO control ......................................................................
superio/nuvoton/nct5104d: Add virtual LDN for simple GPIO IO control
Now, Super I/O GPIOs can also be controlled directly through access to I/O registers. VLDN 108 and specific I/O port from a range <100h; ff8h> may be enabled in mainboard devicetree.
Change-Id: I4ce99bb44e6f5db684170f4190bdc38a944849f6 Signed-off-by: Piotr Kleinschmidt piotr.kleinschmidt@3mdeb.com Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com --- M src/superio/nuvoton/nct5104d/nct5104d.h M src/superio/nuvoton/nct5104d/superio.c 2 files changed, 33 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/35849/5
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35849 )
Change subject: superio/nuvoton/nct5104d: Add virtual LDN for simple GPIO IO control ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35849/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35849/4//COMMIT_MSG@11 PS4, Line 11: ff9h
yep, the io mask specifies the resource length and the maximum address where it can be mapped. […]
Done
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35849 )
Change subject: superio/nuvoton/nct5104d: Add virtual LDN for simple GPIO IO control ......................................................................
superio/nuvoton/nct5104d: Add virtual LDN for simple GPIO IO control
Now, Super I/O GPIOs can also be controlled directly through access to I/O registers. VLDN 108 and specific I/O port from a range <100h; ff8h> may be enabled in mainboard devicetree.
Change-Id: I4ce99bb44e6f5db684170f4190bdc38a944849f6 Signed-off-by: Piotr Kleinschmidt piotr.kleinschmidt@3mdeb.com Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35849 Reviewed-by: Felix Held felix-coreboot@felixheld.de Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/superio/nuvoton/nct5104d/nct5104d.h M src/superio/nuvoton/nct5104d/superio.c 2 files changed, 33 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 9a88105..3f78c1d 100644 --- a/src/superio/nuvoton/nct5104d/nct5104d.h +++ b/src/superio/nuvoton/nct5104d/nct5104d.h @@ -40,7 +40,6 @@ #define NCT5104D_FDC 0x00 /* FDC - not pinned out */ #define NCT5104D_SP1 0x02 /* UARTA */ #define NCT5104D_SP2 0x03 /* UARTB */ -#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 */ #define NCT5104D_SP4 0x11 /* UARTD */ @@ -48,6 +47,7 @@
/* Virtual Logical Device Numbers (LDN) */ #define NCT5104D_GPIO_V 0x07 /* GPIO - 0,1,6 Interface */ +#define NCT5104D_GPIO_WDT_V 0x08 /* GPIO/WDT Interface */
/* Virtual devices sharing the enables are encoded as follows: VLDN = baseLDN[7:0] | [10:8] bitpos of enable in 0x30 of baseLDN @@ -56,6 +56,9 @@ #define NCT5104D_GPIO1 ((1 << 8) | NCT5104D_GPIO_V) #define NCT5104D_GPIO6 ((6 << 8) | NCT5104D_GPIO_V)
+#define NCT5104D_GPIO_WDT ((0 << 8) | NCT5104D_GPIO_WDT_V) +#define NCT5104D_GPIO_IO ((1 << 8) | NCT5104D_GPIO_WDT_V) + void nct5104d_enable_uartd(pnp_devfn_t dev);
#endif /* SUPERIO_NUVOTON_NCT5104D_H */ diff --git a/src/superio/nuvoton/nct5104d/superio.c b/src/superio/nuvoton/nct5104d/superio.c index 69f54a7..6836d69 100644 --- a/src/superio/nuvoton/nct5104d/superio.c +++ b/src/superio/nuvoton/nct5104d/superio.c @@ -14,7 +14,9 @@ * GNU General Public License for more details. */
+#include <console/console.h> #include <device/pnp.h> +#include <device/device.h> #include <superio/conf_mode.h>
#include "nct5104d.h" @@ -147,6 +149,29 @@ pnp_write_config(dev, NCT5104D_GPIO6_PP_OD, 0xFF); }
+static void disable_gpio_io_port(struct device *dev) +{ + struct device *gpio0, *gpio1, *gpio6; + + /* + * Since UARTC and UARTD share pins with GPIO0 and GPIO1 and the + * GPIO/UART can be selected via Kconfig, check whether at least one of + * GPIOs is enabled and if yes keep the GPIO IO VLDN enabled. If no + * GPIOs are enabled, disable the VLDN in order to protect from invalid + * devicetree + Kconfig settings. + */ + 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); + + if (!((gpio0 && gpio0->enabled) || (gpio1 && gpio1->enabled) || + (gpio6 && gpio6->enabled))) { + dev->enabled = 0; + printk(BIOS_WARNING, "WARNING: GPIO IO port configured," + " but no GPIO enabled. Disabling..."); + } +} + static void nct5104d_init(struct device *dev) { struct superio_nuvoton_nct5104d_config *conf = dev->chip_info; @@ -177,6 +202,9 @@ case NCT5104D_GPIO_PP_OD: reset_gpio_default_od(dev); break; + case NCT5104D_GPIO_IO: + disable_gpio_io_port(dev); + break; default: break; } @@ -204,6 +232,7 @@ { NULL, NCT5104D_GPIO1}, { NULL, NCT5104D_GPIO6}, { NULL, NCT5104D_GPIO_PP_OD}, + { NULL, NCT5104D_GPIO_IO, PNP_IO0, 0x07f8, }, { NULL, NCT5104D_PORT80}, };
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35849 )
Change subject: superio/nuvoton/nct5104d: Add virtual LDN for simple GPIO IO control ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35849/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35849/4//COMMIT_MSG@11 PS4, Line 11: ff9h
yep, the io mask specifies the resource length and the maximum address where it can be mapped. […]
I missed the 9...
I still wonder why other LDNs have 0x7f8 mask, while their IO bases can also be up to 0xff8
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35849 )
Change subject: superio/nuvoton/nct5104d: Add virtual LDN for simple GPIO IO control ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35849/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35849/4//COMMIT_MSG@11 PS4, Line 11: ff9h
I missed the 9... […]
Probably due to copy/paste...
-
build bot (Jenkins) (Code Review) -
Felix Held (Code Review)
-
Michał Żygowski (Code Review)
-
Paul Menzel (Code Review)
-
Piotr Kleinschmidt (Code Review)