Change in coreboot[master]: superio/aspeed/ast2400: Add AST2500 support

[Patrick Rudolph (Code Review)]

Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37641 )

Change subject: superio/aspeed/ast2400: Add AST2500 support
......................................................................

superio/aspeed/ast2400: Add AST2500 support

The AST2500 is similar to the AST2400, but it also supports ESPI mode.
In ESPI mode the IRQ level must be 0 and UART3/UART4 aren't usable.

Change-Id: Iea45740427ad56656040e6342f5316ec9d38122f
Signed-off-by: Patrick Rudolph 
---
A src/superio/aspeed/ast2400/chip.h
M src/superio/aspeed/ast2400/superio.c
2 files changed, 42 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/37641/1

diff --git a/src/superio/aspeed/ast2400/chip.h b/src/superio/aspeed/ast2400/chip.h
new file mode 100644
index 0000000..e17274e
--- /dev/null
+++ b/src/superio/aspeed/ast2400/chip.h
@@ -0,0 +1,22 @@
+/*
+ * This file is part of the coreboot project.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __SUPERIO_ASPEED__AST2400_CHIP_H__
+#define __SUPERIO_ASPEED__AST2400_CHIP_H__
+
+struct superio_aspeed_ast2400_config {
+	/* On AST2500 1: ESPI, 0: LPC */
+	bool espi;
+};
+
+#endif /* __SUPERIO_ASPEED__AST2400_CHIP_H__ */
diff --git a/src/superio/aspeed/ast2400/superio.c b/src/superio/aspeed/ast2400/superio.c
index a41bba7..bf297af 100644
--- a/src/superio/aspeed/ast2400/superio.c
+++ b/src/superio/aspeed/ast2400/superio.c
@@ -22,12 +22,23 @@
 #include <superio/common/ssdt.h>
 #include <arch/acpi.h>
 #include "ast2400.h"
+#include "chip.h"

static void ast2400_init(struct device *dev)
 {
+	struct superio_aspeed_ast2400_config *conf = dev->chip_info;
+
    if (!dev->enabled)
    	return;

+	if (conf && conf->espi) {
+		pnp_enter_conf_mode(dev);
+		pnp_set_logical_device(dev);
+		/* In ESPI mode must write 0 to IRQ level on every LDN */
+		pnp_write_config(dev, 0x70, 0);
+		pnp_exit_conf_mode(dev);
+	}
+
    switch (dev->path.pnp.device) {
    case AST2400_KBC:
    	pc_keyboard_init(NO_AUX_DEVICE);
@@ -94,11 +105,19 @@

static void enable_dev(struct device *dev)
 {
+	struct superio_aspeed_ast2400_config *conf = dev->chip_info;
+
+	if (conf && conf->espi) {
+		/* UART3 and UART4 are not usable in ESPI mode */
+		pnp_dev_info[5].function = -1;
+		pnp_dev_info[6].function = -1;
+	}
+
    pnp_enable_devices(dev, &ops, ARRAY_SIZE(pnp_dev_info),
    	pnp_dev_info);
 }

struct chip_operations superio_aspeed_ast2400_ops = {
-	CHIP_NAME("ASpeed AST2400 Super I/O")
+	CHIP_NAME("ASpeed AST2400/AST2500 Super I/O")
    .enable_dev = enable_dev,
 };

[Frans Hendriks (Code Review)]

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37641 )

Change subject: superio/aspeed/ast2400: Add AST2500 support
......................................................................

Patch Set 5: Code-Review+1

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37641 )

Change subject: superio/aspeed/ast2400: Add AST2500 support
......................................................................

Patch Set 5: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/37641/5/src/superio/aspeed/ast2400/... 
File src/superio/aspeed/ast2400/chip.h:

https://review.coreboot.org/c/coreboot/+/37641/5/src/superio/aspeed/ast2400/... 
PS5, Line 19: espi
Maybe rename this to "use_espi", so that the meaning of its value is inferred more easily?

[Felix Held (Code Review)]

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

Change subject: superio/aspeed/ast2400: Add AST2500 support
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37641/5/src/superio/aspeed/ast2400/... 
File src/superio/aspeed/ast2400/superio.c:

https://review.coreboot.org/c/coreboot/+/37641/5/src/superio/aspeed/ast2400/... 
PS5, Line 112: pnp_dev_info[5].function = -1;
             : 		pnp_dev_info[6].function = -1;
i'd prefer iteration over the array elements and matching on the defines assigned to .function instead of hoping that the offsets won't change when the code gets some refactoring.
also assigning a negative number to an unsigned variable is a bad idea; I'd suggest rebasing this onto 37741 and assign PNP_SKIP_FUNCTION to .function instead

[Patrick Rudolph (Code Review)]

Patrick Rudolph has uploaded a new patch set (#6) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/37641 )

Change subject: superio/aspeed/ast2400: Add AST2500 support
......................................................................

superio/aspeed/ast2400: Add AST2500 support

The AST2500 is similar to the AST2400, but it also supports ESPI mode.
In ESPI mode the IRQ level must be 0 and UART3/UART4 aren't usable.

Change-Id: Iea45740427ad56656040e6342f5316ec9d38122f
Signed-off-by: Patrick Rudolph 
---
A src/superio/aspeed/ast2400/chip.h
M src/superio/aspeed/ast2400/superio.c
2 files changed, 45 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/37641/6

[Frans Hendriks (Code Review)]

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37641 )

Change subject: superio/aspeed/ast2400: Add AST2500 support
......................................................................

Patch Set 6: Code-Review+1

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37641 )

Change subject: superio/aspeed/ast2400: Add AST2500 support
......................................................................

Patch Set 6:

(2 comments)

https://review.coreboot.org/c/coreboot/+/37641/5/src/superio/aspeed/ast2400/... 
File src/superio/aspeed/ast2400/chip.h:

https://review.coreboot.org/c/coreboot/+/37641/5/src/superio/aspeed/ast2400/... 
PS5, Line 19: espi
> Maybe rename this to "use_espi", so that the meaning of its value is inferred more easily?

Done

https://review.coreboot.org/c/coreboot/+/37641/5/src/superio/aspeed/ast2400/... 
File src/superio/aspeed/ast2400/superio.c:

https://review.coreboot.org/c/coreboot/+/37641/5/src/superio/aspeed/ast2400/... 
PS5, Line 112: pnp_dev_info[5].function = -1;
             : 		pnp_dev_info[6].function = -1;
> i'd prefer iteration over the array elements and matching on the defines assigned to . […]

Done

[Felix Held (Code Review)]

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

Change subject: superio/aspeed/ast2400: Add AST2500 support
......................................................................

Patch Set 6: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37641 )

Change subject: superio/aspeed/ast2400: Add AST2500 support
......................................................................

superio/aspeed/ast2400: Add AST2500 support

The AST2500 is similar to the AST2400, but it also supports ESPI mode.
In ESPI mode the IRQ level must be 0 and UART3/UART4 aren't usable.

Change-Id: Iea45740427ad56656040e6342f5316ec9d38122f
Signed-off-by: Patrick Rudolph 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/37641
Tested-by: build bot (Jenkins) 
Reviewed-by: Frans Hendriks 
Reviewed-by: Felix Held 
---
A src/superio/aspeed/ast2400/chip.h
M src/superio/aspeed/ast2400/superio.c
2 files changed, 45 insertions(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Felix Held: Looks good to me, approved
  Frans Hendriks: Looks good to me, but someone else must approve

diff --git a/src/superio/aspeed/ast2400/chip.h b/src/superio/aspeed/ast2400/chip.h
new file mode 100644
index 0000000..4f1c5f0
--- /dev/null
+++ b/src/superio/aspeed/ast2400/chip.h
@@ -0,0 +1,22 @@
+/*
+ * This file is part of the coreboot project.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __SUPERIO_ASPEED__AST2400_CHIP_H__
+#define __SUPERIO_ASPEED__AST2400_CHIP_H__
+
+struct superio_aspeed_ast2400_config {
+	/* On AST2500 only 1: ESPI, 0: LPC */
+	bool use_espi;
+};
+
+#endif /* __SUPERIO_ASPEED__AST2400_CHIP_H__ */
diff --git a/src/superio/aspeed/ast2400/superio.c b/src/superio/aspeed/ast2400/superio.c
index a41bba7..37a7c9d 100644
--- a/src/superio/aspeed/ast2400/superio.c
+++ b/src/superio/aspeed/ast2400/superio.c
@@ -22,12 +22,23 @@
 #include <superio/common/ssdt.h>
 #include <arch/acpi.h>
 #include "ast2400.h"
+#include "chip.h"

static void ast2400_init(struct device *dev)
 {
+	struct superio_aspeed_ast2400_config *conf = dev->chip_info;
+
    if (!dev->enabled)
    	return;

+	if (conf && conf->use_espi) {
+		pnp_enter_conf_mode(dev);
+		pnp_set_logical_device(dev);
+		/* In ESPI mode must write 0 to IRQ level on every LDN */
+		pnp_write_config(dev, 0x70, 0);
+		pnp_exit_conf_mode(dev);
+	}
+
    switch (dev->path.pnp.device) {
    case AST2400_KBC:
    	pc_keyboard_init(NO_AUX_DEVICE);
@@ -94,11 +105,22 @@

static void enable_dev(struct device *dev)
 {
+	struct superio_aspeed_ast2400_config *conf = dev->chip_info;
+
+	if (conf && conf->use_espi) {
+		/* UART3 and UART4 are not usable in ESPI mode */
+		for (size_t i = 0; i < ARRAY_SIZE(pnp_dev_info); i++) {
+			if ((pnp_dev_info[i].function == AST2400_SUART3) ||
+			    (pnp_dev_info[i].function == AST2400_SUART4))
+				pnp_dev_info[i].function = PNP_SKIP_FUNCTION;
+		}
+	}
+
    pnp_enable_devices(dev, &ops, ARRAY_SIZE(pnp_dev_info),
    	pnp_dev_info);
 }

struct chip_operations superio_aspeed_ast2400_ops = {
-	CHIP_NAME("ASpeed AST2400 Super I/O")
+	CHIP_NAME("ASpeed AST2400/AST2500 Super I/O")
    .enable_dev = enable_dev,
 };