coreboot-gerrit June 2023

coreboot-gerrit@coreboot.org

1 participants
5387 discussions

[L] Change in coreboot[master]: libpayload/uhci: Re-write UHCI RH driver w/ generic_hub API
by Felix Held (Code Review) 12 Jun '23

12 Jun '23

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/75504?usp=email ) Change subject: libpayload/uhci: Re-write UHCI RH driver w/ generic_hub API ...................................................................... libpayload/uhci: Re-write UHCI RH driver w/ generic_hub API This is a complete rewrite of the UHCI root-hub driver, based on the xHCI one. We are doing things by the book as far as possible. One special case is uhci_rh_reset_port() which does the reset se- quencing that usually the hardware would do. This abandons some quirks of the old driver: * Ports are not disabled/re-enabled for every attachment anymore. * We solely rely on the Connect Status Change bit to track changes. * Further status changes are now deferred to the next polling round. The latter fixes endless loops in combination with commit 7faff543da (libpayload: usb: Detach unused USB devices). Change-Id: I5211728775eb94dfc23fa82ebf00fe5c99039709 Signed-off-by: Nico Huber <nico.huber(a)secunet.com> Reviewed-on: https://review.coreboot.org/c/coreboot/+/75504 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Julius Werner <jwerner(a)chromium.org> Reviewed-by: Paul Menzel <paulepanter(a)mailbox.org> --- M payloads/libpayload/drivers/usb/uhci_rh.c 1 file changed, 96 insertions(+), 157 deletions(-) Approvals: Paul Menzel: Looks good to me, but someone else must approve build bot (Jenkins): Verified Julius Werner: Looks good to me, approved diff --git a/payloads/libpayload/drivers/usb/uhci_rh.c b/payloads/libpayload/drivers/usb/uhci_rh.c index e7f0da7..5f3d184 100644 --- a/payloads/libpayload/drivers/usb/uhci_rh.c +++ b/payloads/libpayload/drivers/usb/uhci_rh.c @@ -1,6 +1,6 @@ /* * - * Copyright (C) 2008-2010 coresystems GmbH + * Copyright (C) 2013 secunet Security Networks AG * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions @@ -28,184 +28,123 @@ //#define USB_DEBUG -#include <libpayload.h> -#include "uhci.h" +#include <stdint.h> +#include <stdbool.h> +#include <usb/usb.h> + +#include "generic_hub.h" #include "uhci_private.h" +#include "uhci.h" -typedef struct { - int port[2]; -} rh_inst_t; +#define PORTSC(p) (PORTSC1 + ((p) - 1) * sizeof(uint16_t)) +#define PORTSC_CCS (1 << 0) +#define PORTSC_CSC (1 << 1) +#define PORTSC_PED (1 << 2) +#define PORTSC_PEDC (1 << 3) +#define PORTSC_LSDA (1 << 8) +#define PORTSC_PR (1 << 9) +#define PORTSC_SUSP (1 << 12) +#define PORTSC_SC_BITS (PORTSC_CSC | PORTSC_PEDC) -#define RH_INST(dev) ((rh_inst_t*)(dev)->data) - -static void -uhci_rh_enable_port(usbdev_t *dev, int port) +static int +uhci_rh_port_status_changed(usbdev_t *const dev, const int port) { - u16 value; - hci_t *controller = dev->controller; - if (port == 1) - port = PORTSC1; - else if (port == 2) - port = PORTSC2; - else { - usb_debug("Invalid port %d\n", port); - return; - } + const u16 portsc = uhci_reg_read16(dev->controller, PORTSC(port)); + const bool changed = portsc & (PORTSC_CSC | PORTSC_PEDC); - uhci_reg_write16(controller, port, - uhci_reg_read16(controller, port) & ~(1 << 12)); /* wakeup */ + /* always clear all the status change bits */ + uhci_reg_write16(dev->controller, PORTSC(port), portsc); - uhci_reg_write16(controller, port, - uhci_reg_read16(controller, port) | 1 << 9); /* reset */ - mdelay(30); // >10ms - uhci_reg_write16(controller, port, - uhci_reg_read16(controller, port) & ~(1 << 9)); - mdelay(1); // >5.3us per spec, <3ms because some devices make trouble - - uhci_reg_write16(controller, port, - uhci_reg_read16(controller, port) | 1 << 2); /* enable */ - /* wait for controller to enable port */ - /* TOTEST: how long to wait? 100ms for now */ - int timeout = 200; /* time out after 200 * 500us == 100ms */ - do { - value = uhci_reg_read16(controller, port); - udelay(500); timeout--; - } while (((value & (1 << 2)) == 0) && (value & 0x01) && timeout); - if (!timeout) - usb_debug("Warning: uhci_rh: port enabling timed out.\n"); -} - -/* disable root hub */ -static void -uhci_rh_disable_port(usbdev_t *dev, int port) -{ - hci_t *controller = dev->controller; - if (port == 1) - port = PORTSC1; - else if (port == 2) - port = PORTSC2; - else { - usb_debug("Invalid port %d\n", port); - return; - } - uhci_reg_write16(controller, port, - uhci_reg_read16(controller, port) & ~4); - u16 value; - /* wait for controller to disable port */ - /* TOTEST: how long to wait? 100ms for now */ - int timeout = 200; /* time out after 200 * 500us == 100ms */ - do { - value = uhci_reg_read16(controller, port); - udelay(500); timeout--; - } while (((value & (1 << 2)) != 0) && timeout); - if (!timeout) - usb_debug("Warning: uhci_rh: port disabling timed out.\n"); -} - -static void -uhci_rh_scanport(usbdev_t *dev, int port) -{ - int portsc, offset; - if (port == 1) { - portsc = PORTSC1; - offset = 0; - } else if (port == 2) { - portsc = PORTSC2; - offset = 1; - } else { - usb_debug("Invalid port %d\n", port); - return; - } - int devno = RH_INST(dev)->port[offset]; - if ((devno != -1) && (dev->controller->devices[devno] != 0)) { - usb_detach_device(dev->controller, devno); - RH_INST(dev)->port[offset] = -1; - } - uhci_reg_write16(dev->controller, portsc, - uhci_reg_read16(dev->controller, portsc) | (1 << 3) | (1 << 2)); // clear port state change, enable port - - mdelay(100); // wait for signal to stabilize - - if ((uhci_reg_read16(dev->controller, portsc) & 1) != 0) { - // device attached - - uhci_rh_disable_port(dev, port); - uhci_rh_enable_port(dev, port); - - usb_speed speed = ((uhci_reg_read16(dev->controller, portsc) >> 8) & 1); - - RH_INST(dev)->port[offset] = usb_attach_device(dev->controller, dev->address, portsc, speed); - } + return changed; } static int -uhci_rh_report_port_changes(usbdev_t *dev) +uhci_rh_port_connected(usbdev_t *const dev, const int port) { - u16 stored, real; - - stored = (RH_INST(dev)->port[0] == -1); - real = ((uhci_reg_read16(dev->controller, PORTSC1) & 1) == 0); - if (stored != real) { - usb_debug("change on port 1\n"); - return 1; - } - - stored = (RH_INST(dev)->port[1] == -1); - real = ((uhci_reg_read16(dev->controller, PORTSC2) & 1) == 0); - if (stored != real) { - usb_debug("change on port 2\n"); - return 2; - } - - // maybe detach+attach happened between two scans? - - if ((uhci_reg_read16(dev->controller, PORTSC1) & 2) > 0) { - usb_debug("possibly re-attached on port 1\n"); - return 1; - } - if ((uhci_reg_read16(dev->controller, PORTSC2) & 2) > 0) { - usb_debug("possibly re-attached on port 2\n"); - return 2; - } - - // no change - return -1; + const u16 portsc = uhci_reg_read16(dev->controller, PORTSC(port)); + return !!(portsc & PORTSC_CCS); } -static void -uhci_rh_destroy(usbdev_t *dev) +static int +uhci_rh_port_enabled(usbdev_t *const dev, const int port) { - usb_detach_device(dev->controller, 1); - usb_detach_device(dev->controller, 2); - uhci_rh_disable_port(dev, 1); - uhci_rh_disable_port(dev, 2); - free(RH_INST(dev)); + const u16 portsc = uhci_reg_read16(dev->controller, PORTSC(port)); + return !(portsc & PORTSC_SUSP) && (portsc & PORTSC_PED); } -static void -uhci_rh_poll(usbdev_t *dev) +static usb_speed +uhci_rh_port_speed(usbdev_t *const dev, const int port) { - int port; - while ((port = uhci_rh_report_port_changes(dev)) != -1) - uhci_rh_scanport(dev, port); + const u16 portsc = uhci_reg_read16(dev->controller, PORTSC(port)); + const bool low_speed = portsc & PORTSC_LSDA; + + return low_speed ? LOW_SPEED : FULL_SPEED; } +static int +uhci_rh_enable_port(usbdev_t *const dev, const int port) +{ + const u16 portsc = uhci_reg_read16(dev->controller, PORTSC(port)) & ~PORTSC_SUSP; + uhci_reg_write16(dev->controller, PORTSC(port), portsc & ~(PORTSC_SC_BITS | PORTSC_SUSP)); + uhci_reg_write16(dev->controller, PORTSC(port), (portsc & ~PORTSC_SC_BITS) | PORTSC_PED); + return 0; +} + +static int +uhci_rh_disable_port(usbdev_t *const dev, const int port) +{ + const u16 portsc = uhci_reg_read16(dev->controller, PORTSC(port)); + uhci_reg_write16(dev->controller, PORTSC(port), portsc & ~(PORTSC_SC_BITS | PORTSC_PED)); + return 0; +} + +static int +uhci_rh_reset_port(usbdev_t *const dev, const int port) +{ + const u16 portsc = uhci_reg_read16(dev->controller, PORTSC(port)) & ~PORTSC_PR; + + /* Trigger port reset. */ + uhci_reg_write16(dev->controller, PORTSC(port), portsc | PORTSC_PR); + + /* Wait for 15ms (usb20 spec 11.5.1.5: reset should take 10 to 20ms). */ + mdelay(15); + + /* Clear port reset. */ + uhci_reg_write16(dev->controller, PORTSC(port), portsc & ~PORTSC_PR); + + udelay(10); /* Linux waits this long. */ + + /* Enable port. */ + uhci_reg_write16(dev->controller, PORTSC(port), portsc | PORTSC_PED); + + /* Clear status-change bits one last time. */ + uhci_reg_write16(dev->controller, PORTSC(port), portsc | PORTSC_PED | PORTSC_SC_BITS); + + return 0; +} + +static const generic_hub_ops_t uhci_rh_ops = { + .hub_status_changed = NULL, + .port_status_changed = uhci_rh_port_status_changed, + .port_connected = uhci_rh_port_connected, + .port_in_reset = NULL, + .port_enabled = uhci_rh_port_enabled, + .port_speed = uhci_rh_port_speed, + .enable_port = uhci_rh_enable_port, + .disable_port = uhci_rh_disable_port, + .start_port_reset = NULL, + .reset_port = uhci_rh_reset_port, +}; + void uhci_rh_init(usbdev_t *dev) { - dev->destroy = uhci_rh_destroy; - dev->poll = uhci_rh_poll; - - uhci_rh_enable_port(dev, 1); - uhci_rh_enable_port(dev, 2); - dev->data = xmalloc(sizeof(rh_inst_t)); - - RH_INST(dev)->port[0] = -1; - RH_INST(dev)->port[1] = -1; - - /* we can set them here because a root hub _really_ shouldn't - appear elsewhere */ + /* we can set them here because a root hub _really_ shouldn't appear elsewhere */ dev->address = 0; dev->hub = -1; dev->port = -1; + + generic_hub_init(dev, 2, &uhci_rh_ops); + + usb_debug("UHCI: root hub init done\n"); } -- To view, visit https://review.coreboot.org/c/coreboot/+/75504?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5211728775eb94dfc23fa82ebf00fe5c99039709 Gerrit-Change-Number: 75504 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[S] Change in coreboot[master]: soc/intel/cmd/blk/cse: Hook get CSE version into `.final`
by Subrata Banik (Code Review) 12 Jun '23

12 Jun '23

Attention is currently required from: Dinesh Gehlot, Kapil Porwal. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75749?usp=email ) Change subject: soc/intel/cmd/blk/cse: Hook get CSE version into `.final` ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/75749?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifdb82c180b64fbb4575932427be54f544e1c98d4 Gerrit-Change-Number: 75749 Gerrit-PatchSet: 1 Gerrit-Owner: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Comment-Date: Mon, 12 Jun 2023 16:44:36 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: soc/intel/cmd/blk/cse: Hook get CSE version into `.final`
by Subrata Banik (Code Review) 12 Jun '23

12 Jun '23

Attention is currently required from: Dinesh Gehlot, Kapil Porwal. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75749?usp=email ) Change subject: soc/intel/cmd/blk/cse: Hook get CSE version into `.final` ...................................................................... Patch Set 1: (1 comment) File src/soc/intel/common/block/cse/cse.c: https://review.coreboot.org/c/coreboot/+/75749/comment/e4c2b655_aeda4025 : PS1, Line 1437: memory don't need to repeat memory again `CBMEM` itself has MEM in it -- To view, visit https://review.coreboot.org/c/coreboot/+/75749?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifdb82c180b64fbb4575932427be54f544e1c98d4 Gerrit-Change-Number: 75749 Gerrit-PatchSet: 1 Gerrit-Owner: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Comment-Date: Mon, 12 Jun 2023 16:44:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[master]: soc/intel/cmd/blk/cse: Perform cse version store under pertinent option
by Subrata Banik (Code Review) 12 Jun '23

12 Jun '23

Attention is currently required from: Dinesh Gehlot, Kapil Porwal. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75748?usp=email ) Change subject: soc/intel/cmd/blk/cse: Perform cse version store under pertinent option ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: should squash CB:75746 and CB:75748 together and make the default selection set to `Y` -- To view, visit https://review.coreboot.org/c/coreboot/+/75748?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iebe27d3277bb6343cc818069bcbb2dc2e8ef0413 Gerrit-Change-Number: 75748 Gerrit-PatchSet: 1 Gerrit-Owner: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Comment-Date: Mon, 12 Jun 2023 16:38:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[master]: mainboard/google/brya: Enable CSE version store for nissa
by Subrata Banik (Code Review) 12 Jun '23

12 Jun '23

Attention is currently required from: Dinesh Gehlot, Kapil Porwal, Tarun Tuli. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75747?usp=email ) Change subject: mainboard/google/brya: Enable CSE version store for nissa ...................................................................... Patch Set 1: (1 comment) File src/mainboard/google/brya/Kconfig: https://review.coreboot.org/c/coreboot/+/75747/comment/cd6559e2_e742b5ac : PS1, Line 91: if SOC_INTEL_STORE_CSE_VERSION i don't believe we need this dependency ideally SOC_INTEL_STORE_CSE_VERSION should be default enabled for chromeos devices and specific platform will choose SOC_INTEL_STORE_CSE_FPT_PARTITION_VERSION to know the ISH version -- To view, visit https://review.coreboot.org/c/coreboot/+/75747?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8ffee751f29af5b34e3383b0053e1267e496e74d Gerrit-Change-Number: 75747 Gerrit-PatchSet: 1 Gerrit-Owner: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Comment-Date: Mon, 12 Jun 2023 16:35:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[master]: soc/intel/cmd/blk/cse: Add a kconfig option to store CSE version
by Subrata Banik (Code Review) 12 Jun '23

12 Jun '23

Attention is currently required from: Dinesh Gehlot, Kapil Porwal. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75746?usp=email ) Change subject: soc/intel/cmd/blk/cse: Add a kconfig option to store CSE version ...................................................................... Patch Set 1: (1 comment) File src/soc/intel/common/block/cse/Kconfig: https://review.coreboot.org/c/coreboot/+/75746/comment/7cabfe9c_41ac852a : PS1, Line 50: default n shouldn't we eventually make this config default enabled ? -- To view, visit https://review.coreboot.org/c/coreboot/+/75746?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If8a62d00e6ed15fbe595f3729d921df8cef42fdb Gerrit-Change-Number: 75746 Gerrit-PatchSet: 1 Gerrit-Owner: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Comment-Date: Mon, 12 Jun 2023 16:32:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: mb/google/rex/var/rex0: add HID over SPI ACPI driver
by Eran Mitrani (Code Review) 12 Jun '23

12 Jun '23

Attention is currently required from: Matt DeVillier, Paul Menzel, Tarun Tuli. Eran Mitrani has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74885?usp=email ) Change subject: mb/google/rex/var/rex0: add HID over SPI ACPI driver ...................................................................... Patch Set 17: (2 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/74885/comment/99b56765_55178588 : PS2, Line 13: > Why use an ACPI driver and not a “native” one? please see Subrata's response Patchset: PS2: > > why use ASL here vs runtime-generated AML, like i2c touchscreens do? […] Done -- To view, visit https://review.coreboot.org/c/coreboot/+/74885?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id51d385ce350cef23da4184b044c74569f4dd3f0 Gerrit-Change-Number: 74885 Gerrit-PatchSet: 17 Gerrit-Owner: Eran Mitrani <mitrani(a)google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Mon, 12 Jun 2023 16:27:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: mb/google/skyrim: Use CMOS bit to toggle ABL WA for Hynix DRAM
by Martin Roth (Code Review) 12 Jun '23

12 Jun '23

Attention is currently required from: Eric Lai, Felix Held, Jason Glenesk, Jason Nien, Karthik Ramasubramanian, Matt DeVillier, Paul Menzel. Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75698?usp=email ) Change subject: mb/google/skyrim: Use CMOS bit to toggle ABL WA for Hynix DRAM ...................................................................... Patch Set 4: Code-Review+2 (4 comments) File src/mainboard/google/skyrim/bootblock.c: https://review.coreboot.org/c/coreboot/+/75698/comment/5383021a_872d599d : PS2, Line 17: 18 > I'm using the full length allowed the SPD spec for the cbi_part_number array, which is 33 bytes, so […] Acknowledged https://review.coreboot.org/c/coreboot/+/75698/comment/da4a0696_9884a65e : PS2, Line 94: hynix_dram_cmos_check(); > I'm perfectly happy to just restict this to just FF, adding another Kconfig seems unnecessary Agreed - I was just looking for a way to not run it on platforms that didn't need it. It should be on the OEMs to take care of adding the fix for their platforms if needed. File src/mainboard/google/skyrim/bootblock.c: https://review.coreboot.org/c/coreboot/+/75698/comment/e5f97685_8390853b : PS3, Line 32: google_chromeec_cbi_get_dram_part_num > since it's passed directly to the EC via a host cmd, it appears it would overflow if too short. […] Acknowledged https://review.coreboot.org/c/coreboot/+/75698/comment/a0c94987_49389b81 : PS3, Line 35: return; > I think there are reasonable arguments for leaving it and for clearing it. […] Acknowledged -- To view, visit https://review.coreboot.org/c/coreboot/+/75698?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibb6e145f6cdba7270e0a322ef414bf1cb09c5eaa Gerrit-Change-Number: 75698 Gerrit-PatchSet: 4 Gerrit-Owner: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)amd.corp-partner.google.com> Gerrit-Reviewer: Jason Nien <jason.nien(a)amd.corp-partner.google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Jason Nien <jason.nien(a)amd.corp-partner.google.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)amd.corp-partner.google.com> Gerrit-Attention: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Mon, 12 Jun 2023 16:23:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Comment-In-Reply-To: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[master]: soc/intel/cmd/blk/cse: Add a kconfig option to store CSE version
by Subrata Banik (Code Review) 12 Jun '23

12 Jun '23

Attention is currently required from: Dinesh Gehlot, Kapil Porwal. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75746?usp=email ) Change subject: soc/intel/cmd/blk/cse: Add a kconfig option to store CSE version ...................................................................... Patch Set 1: (2 comments) File src/soc/intel/common/block/cse/Kconfig: https://review.coreboot.org/c/coreboot/+/75746/comment/f9006b73_1ea16e99 : PS1, Line 48: SOC_INTEL_STORE_CSE_VERSION nit: SOC_INTEL_STORE_CSE_RW_FW_VERSION https://review.coreboot.org/c/coreboot/+/75746/comment/28e62754_937b11ef : PS1, Line 53: stores CSE firmware version in CBMEM memory. This configuration option allow storing CSE RW (for LITE SKU) firmware version in CBMEM. -- To view, visit https://review.coreboot.org/c/coreboot/+/75746?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If8a62d00e6ed15fbe595f3729d921df8cef42fdb Gerrit-Change-Number: 75746 Gerrit-PatchSet: 1 Gerrit-Owner: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Comment-Date: Mon, 12 Jun 2023 16:20:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: mb/google/skyrim: Use CMOS bit to toggle ABL WA for Hynix DRAM
by Matt DeVillier (Code Review) 12 Jun '23

12 Jun '23

Attention is currently required from: Eric Lai, Felix Held, Jason Glenesk, Jason Nien, Karthik Ramasubramanian, Martin Roth, Paul Menzel. Hello Eric Lai, Felix Held, Jason Glenesk, Jason Nien, Karthik Ramasubramanian, Martin Roth, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/75698?usp=email to look at the new patch set (#4). The following approvals got outdated and were removed: Code-Review+1 by Eric Lai, Verified+1 by build bot (Jenkins) Change subject: mb/google/skyrim: Use CMOS bit to toggle ABL WA for Hynix DRAM ...................................................................... mb/google/skyrim: Use CMOS bit to toggle ABL WA for Hynix DRAM One specific Hynix LPDDR5x DRAM part requires an ABL workaround to eliminate DRAM-related failures during a FAFT test, but due to the use of generic/common SPDs, there is no way for the ABL to determine the DRAM part # itself. Consequently, we will have coreboot check the DRAM part #, and set/clear a CMOS bit as appropriate, which the ABL will check in order to apply (or not apply) the workaround. The ABL already uses byte 0xD of the extended CMOS ports 72/73 for memory context related toggles, so we will use a spare bit there. BUG=b:270499009, b:281614369, b:286338775 BRANCH=skyrim TEST=run FAFT bios tests on frostflow, markarth, and whiterun without any failures. Change-Id: Ibb6e145f6cdba7270e0a322ef414bf1cb09c5eaa Signed-off-by: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> --- M src/mainboard/google/skyrim/bootblock.c 1 file changed, 56 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/75698/4 -- To view, visit https://review.coreboot.org/c/coreboot/+/75698?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibb6e145f6cdba7270e0a322ef414bf1cb09c5eaa Gerrit-Change-Number: 75698 Gerrit-PatchSet: 4 Gerrit-Owner: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)amd.corp-partner.google.com> Gerrit-Reviewer: Jason Nien <jason.nien(a)amd.corp-partner.google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Jason Nien <jason.nien(a)amd.corp-partner.google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)amd.corp-partner.google.com> Gerrit-Attention: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Attention: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: newpatchset

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit June 2023