Change in ...coreboot[master]: drivers/usb/ucsi: Add driver to generate UCSI memory region
Duncan Laurie has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32356
Change subject: drivers/usb/ucsi: Add driver to generate UCSI memory region ......................................................................
drivers/usb/ucsi: Add driver to generate UCSI memory region
The USB Type-C Connector System Software Interface (UCSI) defines a required memory oregion for the OS UCSI driver to use to communicate with the BIOS and EC.
This driver allocates a cbmem region for this required ACPI UCSI memory and generates the ACPI AML code to describe it into the SSDT.
This driver must be paired with an EC implementation that actually instantiates the UCSI device and provides the _DSM method that translates from this shared memory to the EC. In order to connect this driver to the EC the ACPI path of the UCSI device is provided in the mainboard devicetree.
Change-Id: Id5b7fa19436443bc11a6ebe3ce89cd552cee4d85 Signed-off-by: Duncan Laurie dlaurie@google.com --- M src/commonlib/include/commonlib/cbmem_id.h A src/drivers/usb/ucsi/Kconfig A src/drivers/usb/ucsi/Makefile.inc A src/drivers/usb/ucsi/chip.h A src/drivers/usb/ucsi/ucsi.c 5 files changed, 188 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/32356/1
diff --git a/src/commonlib/include/commonlib/cbmem_id.h b/src/commonlib/include/commonlib/cbmem_id.h index af79a59..fb2d3e3 100644 --- a/src/commonlib/include/commonlib/cbmem_id.h +++ b/src/commonlib/include/commonlib/cbmem_id.h @@ -19,6 +19,7 @@
#define CBMEM_ID_ACPI 0x41435049 #define CBMEM_ID_ACPI_GNVS 0x474e5653 +#define CBMEM_ID_ACPI_UCSI 0x55435349 #define CBMEM_ID_AFTER_CAR 0xc4787a93 #define CBMEM_ID_AGESA_RUNTIME 0x41474553 #define CBMEM_ID_AMDMCT_MEMINFO 0x494D454E @@ -81,6 +82,7 @@ #define CBMEM_ID_TO_NAME_TABLE \ { CBMEM_ID_ACPI, "ACPI " }, \ { CBMEM_ID_ACPI_GNVS, "ACPI GNVS " }, \ + { CBMEM_ID_ACPI_UCSI, "ACPI UCSI " }, \ { CBMEM_ID_AGESA_RUNTIME, "AGESA RSVD " }, \ { CBMEM_ID_AFTER_CAR, "AFTER CAR " }, \ { CBMEM_ID_AMDMCT_MEMINFO, "AMDMEM INFO" }, \ diff --git a/src/drivers/usb/ucsi/Kconfig b/src/drivers/usb/ucsi/Kconfig new file mode 100644 index 0000000..22e751d --- /dev/null +++ b/src/drivers/usb/ucsi/Kconfig @@ -0,0 +1,12 @@ +config DRIVERS_USB_UCSI + bool + default n + depends on HAVE_ACPI_TABLES + help + This driver allocates a fixed memory region in cbmem for the EC + implementation of the USB Type-C System Software Interface. + The shared memory region is used for the OS driver to pass + data to the _DSM method, which must be implemented by the EC. + The ACPI path of the EC device is provided in devicetree so the + ACPI operation region can be added to the specific UCSI device + that the EC implements. diff --git a/src/drivers/usb/ucsi/Makefile.inc b/src/drivers/usb/ucsi/Makefile.inc new file mode 100644 index 0000000..a62d58c --- /dev/null +++ b/src/drivers/usb/ucsi/Makefile.inc @@ -0,0 +1 @@ +ramstage-$(CONFIG_DRIVERS_USB_UCSI) += ucsi.c diff --git a/src/drivers/usb/ucsi/chip.h b/src/drivers/usb/ucsi/chip.h new file mode 100644 index 0000000..860d6c5 --- /dev/null +++ b/src/drivers/usb/ucsi/chip.h @@ -0,0 +1,24 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2019 Google LLC + * + * 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 __USB_UCSI_CHIP_H__ +#define __USB_UCSI_CHIP_H__ + +struct drivers_usb_ucsi_config { + /* Location of the UCSI device defined by the EC */ + const char *path; +}; + +#endif /* __USB_UCSI_CHIP_H__ */ diff --git a/src/drivers/usb/ucsi/ucsi.c b/src/drivers/usb/ucsi/ucsi.c new file mode 100644 index 0000000..a697e4f --- /dev/null +++ b/src/drivers/usb/ucsi/ucsi.c @@ -0,0 +1,149 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2019 Google LLC + * + * 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. + */ + +#include <arch/acpi_device.h> +#include <arch/acpigen.h> +#include <cbmem.h> +#include <console/console.h> +#include <device/device.h> +#include <device/path.h> +#include <device/pnp.h> +#include <stdint.h> +#include "chip.h" + +/* + * The UCSI fields are defined in the UCSI specification at + * https://www.intel.com/content/www/us/en/io/universal-serial-bus/usb-type-c-u... + * https://www.intel.com/content/www/us/en/io/universal-serial-bus/bios-impleme... + */ + +static struct fieldlist ucsi_region_fields[] = { + FIELDLIST_NAMESTR("VER0", 8), + FIELDLIST_NAMESTR("VER1", 8), + FIELDLIST_NAMESTR("RSV0", 8), + FIELDLIST_NAMESTR("RSV1", 8), + FIELDLIST_NAMESTR("CCI0", 8), + FIELDLIST_NAMESTR("CCI1", 8), + FIELDLIST_NAMESTR("CCI2", 8), + FIELDLIST_NAMESTR("CCI3", 8), + FIELDLIST_NAMESTR("CTL0", 8), + FIELDLIST_NAMESTR("CTL1", 8), + FIELDLIST_NAMESTR("CTL2", 8), + FIELDLIST_NAMESTR("CTL3", 8), + FIELDLIST_NAMESTR("CTL4", 8), + FIELDLIST_NAMESTR("CTL5", 8), + FIELDLIST_NAMESTR("CTL6", 8), + FIELDLIST_NAMESTR("CTL7", 8), + FIELDLIST_NAMESTR("MGI0", 8), + FIELDLIST_NAMESTR("MGI1", 8), + FIELDLIST_NAMESTR("MGI2", 8), + FIELDLIST_NAMESTR("MGI3", 8), + FIELDLIST_NAMESTR("MGI4", 8), + FIELDLIST_NAMESTR("MGI5", 8), + FIELDLIST_NAMESTR("MGI6", 8), + FIELDLIST_NAMESTR("MGI7", 8), + FIELDLIST_NAMESTR("MGI8", 8), + FIELDLIST_NAMESTR("MGI9", 8), + FIELDLIST_NAMESTR("MGIA", 8), + FIELDLIST_NAMESTR("MGIB", 8), + FIELDLIST_NAMESTR("MGIC", 8), + FIELDLIST_NAMESTR("MGID", 8), + FIELDLIST_NAMESTR("MGIE", 8), + FIELDLIST_NAMESTR("MGIF", 8), + FIELDLIST_NAMESTR("MGO0", 8), + FIELDLIST_NAMESTR("MGO1", 8), + FIELDLIST_NAMESTR("MGO2", 8), + FIELDLIST_NAMESTR("MGO3", 8), + FIELDLIST_NAMESTR("MGO4", 8), + FIELDLIST_NAMESTR("MGO5", 8), + FIELDLIST_NAMESTR("MGO6", 8), + FIELDLIST_NAMESTR("MGO7", 8), + FIELDLIST_NAMESTR("MGO8", 8), + FIELDLIST_NAMESTR("MGO9", 8), + FIELDLIST_NAMESTR("MGOA", 8), + FIELDLIST_NAMESTR("MGOB", 8), + FIELDLIST_NAMESTR("MGOC", 8), + FIELDLIST_NAMESTR("MGOD", 8), + FIELDLIST_NAMESTR("MGOE", 8), + FIELDLIST_NAMESTR("MGOF", 8), +}; +static const size_t ucsi_region_len = ARRAY_SIZE(ucsi_region_fields); + +/* Allocate cbmem region for EC UCSI device to use for OS shared memory */ +static void ucsi_fill_ssdt_generator(struct device *dev) +{ + struct drivers_usb_ucsi_config *config = dev->chip_info; + struct opregion opreg; + void *region_ptr; + + if (!dev->enabled || !config) + return; + if (!config->path) { + printk(BIOS_ERR, "%s: ACPI path for UCSI device required\n", + dev_path(dev)); + return; + } + + region_ptr = cbmem_add(CBMEM_ID_ACPI_UCSI, ucsi_region_len); + if (!region_ptr) + return; + memset(region_ptr, 0, ucsi_region_len); + + opreg.name = "UCSM"; + opreg.regionspace = SYSTEMMEMORY; + opreg.regionoffset = (uintptr_t)region_ptr; + opreg.regionlen = ucsi_region_len; + + acpigen_write_scope(config->path); + acpigen_write_name("_CRS"); + acpigen_write_resourcetemplate_header(); + acpigen_write_mem32fixed(1, (uintptr_t)region_ptr, ucsi_region_len); + acpigen_write_resourcetemplate_footer(); + acpigen_write_opregion(&opreg); + acpigen_write_field(opreg.name, ucsi_region_fields, ucsi_region_len, + FIELD_ANYACC | FIELD_LOCK | FIELD_PRESERVE); + acpigen_pop_len(); /* Scope */ + + printk(BIOS_INFO, "%s: %s at %s\n", config->path, + dev->chip_ops->name, dev_path(dev)); +} + +static const char *ucsi_acpi_name(const struct device *dev) +{ + return "UCSI"; +} + +static struct device_operations ops = { + .read_resources = DEVICE_NOOP, + .set_resources = DEVICE_NOOP, + .enable_resources = DEVICE_NOOP, + .acpi_name = ucsi_acpi_name, + .scan_bus = scan_generic_bus, + .acpi_fill_ssdt_generator = ucsi_fill_ssdt_generator, +}; + +static struct pnp_info info[] = { + { NULL, 0, 0, 0, } +}; + +static void ucsi_enable(struct device *dev) +{ + pnp_enable_devices(dev, &ops, ARRAY_SIZE(info), info); +} + +struct chip_operations drivers_usb_ucsi_ops = { + CHIP_NAME("USB Type-C Connector System Software Interface (UCSI)") + .enable_dev = ucsi_enable +};
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32356 )
Change subject: drivers/usb/ucsi: Add driver to generate UCSI memory region ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32356/1/src/drivers/usb/ucsi/ucsi.c File src/drivers/usb/ucsi/ucsi.c:
https://review.coreboot.org/#/c/32356/1/src/drivers/usb/ucsi/ucsi.c@143 PS1, Line 143: pnp_enable_devices(dev, &ops, ARRAY_SIZE(info), info); Any reason to use the pnp infrastructure? I think, we have a `generic` device type, too.
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32356 )
Change subject: drivers/usb/ucsi: Add driver to generate UCSI memory region ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32356/1/src/drivers/usb/ucsi/ucsi.c File src/drivers/usb/ucsi/ucsi.c:
https://review.coreboot.org/#/c/32356/1/src/drivers/usb/ucsi/ucsi.c@143 PS1, Line 143: pnp_enable_devices(dev, &ops, ARRAY_SIZE(info), info);
Any reason to use the pnp infrastructure? I think, we have a `generic` […]
UCSI is technically PNP0CA0, but this is a bit silly to be sure.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32356 )
Change subject: drivers/usb/ucsi: Add driver to generate UCSI memory region ......................................................................
Patch Set 1:
(1 comment)
I'm struggling to see why you made the cut between generated and static ASL code where you made it. It all depends on the CBMEM address, right? Wouldn't it be enough to generate a named object, e.g.
Name (CBMU, 0x...)
and use that in static code like
OperationRegion (UCSM, SystemMemory, CBMU, UCSI_SIZE)
and something similar for _CRS, etc.
https://review.coreboot.org/#/c/32356/1/src/drivers/usb/ucsi/ucsi.c File src/drivers/usb/ucsi/ucsi.c:
https://review.coreboot.org/#/c/32356/1/src/drivers/usb/ucsi/ucsi.c@85 PS1, Line 85: static void ucsi_fill_ssdt_generator(struct device *dev) I guess, if we'd made this more a library function that is called from EC code, we wouldn't need the `path` setting in the dt?
your call
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32356 )
Change subject: drivers/usb/ucsi: Add driver to generate UCSI memory region ......................................................................
Patch Set 1:
Patch Set 1:
(1 comment)
I'm struggling to see why you made the cut between generated and static ASL code where you made it. It all depends on the CBMEM address, right? Wouldn't it be enough to generate a named object, e.g.
Name (CBMU, 0x...)and use that in static code like
OperationRegion (UCSM, SystemMemory, CBMU, UCSI_SIZE)and something similar for _CRS, etc.
Yes that could work too.
What I really wanted was to put almost everything in the SSDT and just have the EC code provide the _DSM method, but DSDT ASL can't scope back into the SSDT so I couldn't declare the device here. Since I couldn't do that I decided to try and provide a framework and split the memory side into the a driver that could be reused if another EC implements UCSI in the future.
But it was just one of many possible paths. Making it into a library function that just allocates the cbmem region and adds a basic Name() for it is another, I will look at it and see how it turns out.
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32356 )
Change subject: drivers/usb/ucsi: Add driver to generate UCSI memory region ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
(1 comment)
I'm struggling to see why you made the cut between generated and static ASL code where you made it. It all depends on the CBMEM address, right? Wouldn't it be enough to generate a named object, e.g.
Name (CBMU, 0x...)and use that in static code like
OperationRegion (UCSM, SystemMemory, CBMU, UCSI_SIZE)and something similar for _CRS, etc.
Yes that could work too.
What I really wanted was to put almost everything in the SSDT and just have the EC code provide the _DSM method, but DSDT ASL can't scope back into the SSDT so I couldn't declare the device here. Since I couldn't do that I decided to try and provide a framework and split the memory side into the a driver that could be reused if another EC implements UCSI in the future.
But it was just one of many possible paths. Making it into a library function that just allocates the cbmem region and adds a basic Name() for it is another, I will look at it and see how it turns out.
Defining an operationregion in the DSDT with an external IntObj does not seem to work, I end up with undefined references when the kernel boots.
I ended up removing the pointless drivers/usb/ucsi driver and just doing it in the EC driver directly though.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32356
to look at the new patch set (#2).
Change subject: cbmem: Add ID for UCSI ......................................................................
cbmem: Add ID for UCSI
The USB Type-C Connector System Software Interface (UCSI) defines a required memory oregion for the OS UCSI driver to use to communicate with the BIOS and EC.
This provides a CBMEM ID that can be used by drivers to allocate this shared memory region for the UCSI driver to use.
BUG=b:131083691
Change-Id: Id5b7fa19436443bc11a6ebe3ce89cd552cee4d85 Signed-off-by: Duncan Laurie dlaurie@google.com --- M src/commonlib/include/commonlib/cbmem_id.h 1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/32356/2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32356 )
Change subject: cbmem: Add ID for UCSI ......................................................................
Patch Set 2: Code-Review+2
Duncan Laurie has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32356 )
Change subject: cbmem: Add ID for UCSI ......................................................................
cbmem: Add ID for UCSI
The USB Type-C Connector System Software Interface (UCSI) defines a required memory oregion for the OS UCSI driver to use to communicate with the BIOS and EC.
This provides a CBMEM ID that can be used by drivers to allocate this shared memory region for the UCSI driver to use.
BUG=b:131083691
Change-Id: Id5b7fa19436443bc11a6ebe3ce89cd552cee4d85 Signed-off-by: Duncan Laurie dlaurie@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/32356 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/commonlib/include/commonlib/cbmem_id.h 1 file changed, 2 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/commonlib/include/commonlib/cbmem_id.h b/src/commonlib/include/commonlib/cbmem_id.h index 535ba33..2236c95 100644 --- a/src/commonlib/include/commonlib/cbmem_id.h +++ b/src/commonlib/include/commonlib/cbmem_id.h @@ -19,6 +19,7 @@
#define CBMEM_ID_ACPI 0x41435049 #define CBMEM_ID_ACPI_GNVS 0x474e5653 +#define CBMEM_ID_ACPI_UCSI 0x55435349 #define CBMEM_ID_AFTER_CAR 0xc4787a93 #define CBMEM_ID_AGESA_RUNTIME 0x41474553 #define CBMEM_ID_AMDMCT_MEMINFO 0x494D454E @@ -82,6 +83,7 @@ #define CBMEM_ID_TO_NAME_TABLE \ { CBMEM_ID_ACPI, "ACPI " }, \ { CBMEM_ID_ACPI_GNVS, "ACPI GNVS " }, \ + { CBMEM_ID_ACPI_UCSI, "ACPI UCSI " }, \ { CBMEM_ID_AGESA_RUNTIME, "AGESA RSVD " }, \ { CBMEM_ID_AFTER_CAR, "AFTER CAR " }, \ { CBMEM_ID_AMDMCT_MEMINFO, "AMDMEM INFO" }, \
-
Duncan Laurie (Code Review) -
Furquan Shaikh (Code Review)
-
Nico Huber (Code Review)