coreboot-gerrit August 2019

coreboot-gerrit@coreboot.org

1 participants
1942 discussions

Change in coreboot[master]: mb/google/kukui: Add panel for Kodama
by Peichao Li (Code Review) 23 Aug '19

23 Aug '19

Peichao Li has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35001 ) Change subject: mb/google/kukui: Add panel for Kodama ...................................................................... mb/google/kukui: Add panel for Kodama Declare the following panel for Kodama: - AUO B101UAN08.3 BUG=b:139699622 TEST=builds Kodama image and working properly Signed-off-by: Peichao Wang <peichao.wang(a)bitland.corp-partner.google.com> Change-Id: I3f688ffd0ece6afac08d353ab5a6cf1cf876b32f --- M src/mainboard/google/kukui/panel_kodama.c 1 file changed, 67 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/35001/1 diff --git a/src/mainboard/google/kukui/panel_kodama.c b/src/mainboard/google/kukui/panel_kodama.c index 8f7ef2b..f702be9 100644 --- a/src/mainboard/google/kukui/panel_kodama.c +++ b/src/mainboard/google/kukui/panel_kodama.c @@ -15,6 +15,72 @@ #include "panel.h" +static struct panel_description AUO_B101UAN08_3 = { + .edid = { + .ascii_string = "B101UAN08.3", + .manufacturer_name = "AUO", + .panel_bits_per_color = 8, + .panel_bits_per_pixel = 24, + .mode = { + .name = "1200x1920@60Hz", + .pixel_clock = 159192, + .lvds_dual_channel = 0, + .refresh = 60, + .ha = 1200, .hbl = 144, .hso = 60, .hspw = 4, + .va = 1920, .vbl = 60, .vso = 34, .vspw = 2, + .phsync = '-', .pvsync = '-', + .x_mm = 135, .y_mm = 216, + }, + }, + .orientation = LB_FB_ORIENTATION_LEFT_UP, + .init = { + INIT_DELAY_CMD(24), + INIT_DCS_CMD(0xB0, 0x01), + INIT_DCS_CMD(0xC0, 0x48), + INIT_DCS_CMD(0xC1, 0x48), + INIT_DCS_CMD(0xC2, 0x47), + INIT_DCS_CMD(0xC3, 0x47), + INIT_DCS_CMD(0xC4, 0x46), + INIT_DCS_CMD(0xC5, 0x46), + INIT_DCS_CMD(0xC6, 0x45), + INIT_DCS_CMD(0xC7, 0x45), + INIT_DCS_CMD(0xC8, 0x64), + INIT_DCS_CMD(0xC9, 0x64), + INIT_DCS_CMD(0xCA, 0x4F), + INIT_DCS_CMD(0xCB, 0x4F), + INIT_DCS_CMD(0xCC, 0x40), + INIT_DCS_CMD(0xCD, 0x40), + INIT_DCS_CMD(0xCE, 0x66), + INIT_DCS_CMD(0xCF, 0x66), + INIT_DCS_CMD(0xD0, 0x4F), + INIT_DCS_CMD(0xD1, 0x4F), + INIT_DCS_CMD(0xD2, 0x41), + INIT_DCS_CMD(0xD3, 0x41), + INIT_DCS_CMD(0xD4, 0x48), + INIT_DCS_CMD(0xD5, 0x48), + INIT_DCS_CMD(0xD6, 0x47), + INIT_DCS_CMD(0xD7, 0x47), + INIT_DCS_CMD(0xD8, 0x46), + INIT_DCS_CMD(0xD9, 0x46), + INIT_DCS_CMD(0xDA, 0x45), + INIT_DCS_CMD(0xDB, 0x45), + INIT_DCS_CMD(0xDC, 0x64), + INIT_DCS_CMD(0xDD, 0x64), + INIT_DCS_CMD(0xDE, 0x4F), + INIT_DCS_CMD(0xDF, 0x4F), + INIT_DCS_CMD(0xE0, 0x40), + INIT_DCS_CMD(0xE1, 0x40), + INIT_DCS_CMD(0xE2, 0x66), + INIT_DCS_CMD(0xE3, 0x66), + INIT_DCS_CMD(0xE4, 0x4F), + INIT_DCS_CMD(0xE5, 0x4F), + INIT_DCS_CMD(0xE6, 0x41), + INIT_DCS_CMD(0xE7, 0x41), + INIT_DELAY_CMD(150), + INIT_END_CMD, + }, +}; + static struct panel_description BOE_TV101WUM_N53 = { .edid = { .ascii_string = "TV101WUM-N53", @@ -331,6 +397,7 @@ }; static struct panel_description *kodama_panels[] = { + [1] = &AUO_B101UAN08_3, [2] = &BOE_TV101WUM_N53, }; -- To view, visit https://review.coreboot.org/c/coreboot/+/35001 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3f688ffd0ece6afac08d353ab5a6cf1cf876b32f Gerrit-Change-Number: 35001 Gerrit-PatchSet: 1 Gerrit-Owner: Peichao Li <peichao.wang(a)bitland.corp-partner.google.com> Gerrit-MessageType: newchange

3 22

Change in coreboot[master]: soc/intel/basecode: Add support for updating ucode loaded via FIT
by Varshit B Pandya (Code Review) 23 Aug '19

23 Aug '19

Varshit B Pandya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/27369 ) Change subject: soc/intel/basecode: Add support for updating ucode loaded via FIT ...................................................................... Patch Set 42: > Patch Set 41: Code-Review-1 > > Needs Documentation. Added Documentation https://review.coreboot.org/c/coreboot/+/34657/11 -- To view, visit https://review.coreboot.org/c/coreboot/+/27369 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iab6ba36a2eb587f331fe522c778e2c430c8eb655 Gerrit-Change-Number: 27369 Gerrit-PatchSet: 42 Gerrit-Owner: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Varshit B Pandya <varshit.b.pandya(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: dhaval v sharma <dhaval.v.sharma(a)intel.com> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: Dhaval Sharma <dhaval.v.sharma(a)intel.corp-partner.google.com> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: rushikesh s kadam <rushikesh.s.kadam(a)intel.com> Gerrit-Comment-Date: Fri, 23 Aug 2019 03:39:13 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: AGESA: Restrict use of -fno-zero-initialized-in-bss
by Kyösti Mälkki (Code Review) 22 Aug '19

22 Aug '19

Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34885 ) Change subject: AGESA: Restrict use of -fno-zero-initialized-in-bss ...................................................................... AGESA: Restrict use of -fno-zero-initialized-in-bss Only apply the flag for libagesa -class. Change-Id: Ide46214d62b2b16e5e1deaa0796be784ed813095 Signed-off-by: Kyösti Mälkki <kyosti.malkki(a)gmail.com> --- M src/vendorcode/amd/agesa/Makefile.inc 1 file changed, 2 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/34885/1 diff --git a/src/vendorcode/amd/agesa/Makefile.inc b/src/vendorcode/amd/agesa/Makefile.inc index d672cf4..da59004 100644 --- a/src/vendorcode/amd/agesa/Makefile.inc +++ b/src/vendorcode/amd/agesa/Makefile.inc @@ -10,9 +10,10 @@ classes-y += libagesa libagesa-y = libagesa-generic-ccopts += -D__LIBAGESA__ +libagesa-generic-ccopts += -fno-zero-initialized-in-bss libagesa-generic-ccopts += $(AGESA_INC) $(AGESA_AUTOINCLUDES) -AGESA_CFLAGS := -march=k8-sse3 -mtune=k8-sse3 -fno-zero-initialized-in-bss -fno-strict-aliasing +AGESA_CFLAGS := -march=k8-sse3 -mtune=k8-sse3 -fno-strict-aliasing CFLAGS_x86_32 += $(AGESA_CFLAGS) CFLAGS_x86_64 += $(AGESA_CFLAGS) -- To view, visit https://review.coreboot.org/c/coreboot/+/34885 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ide46214d62b2b16e5e1deaa0796be784ed813095 Gerrit-Change-Number: 34885 Gerrit-PatchSet: 1 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-MessageType: newchange

3 6

Change in coreboot[master]: Makefile: Write .xcompile to $(obj)
by Raul Rangel (Code Review) 22 Aug '19

22 Aug '19

Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/28101 ) Change subject: Makefile: Write .xcompile to $(obj) ...................................................................... Patch Set 6: (4 comments) > Patch Set 6: > > (6 comments) > > .xcompile is also in the .gitignore file and could be removed from there. I'm going to split this CL so the .xcompile is generated when calling abuild instead of failing because make was unable to locate it. https://review.coreboot.org/c/coreboot/+/28101/6/Makefile File Makefile: https://review.coreboot.org/c/coreboot/+/28101/6/Makefile@467 PS6, Line 467: .xcompile > Remove this one? I think we should leave this here so any stale .xcompile objects get cleaned up. https://review.coreboot.org/c/coreboot/+/28101/6/src/soc/nvidia/tegra124/lp… File src/soc/nvidia/tegra124/lp0/Makefile: https://review.coreboot.org/c/coreboot/+/28101/6/src/soc/nvidia/tegra124/lp… PS6, Line 16: xcompile > Maybe instead of looking for it, just regenerate it here and use it from here? That way we don't ha […] since this file doesn't respect the $(obj) variable I was hesitant to write anything into the src directory. I think this file needs to get converted so it acts like all the other ones. https://review.coreboot.org/c/coreboot/+/28101/6/util/abuild/abuild File util/abuild/abuild: https://review.coreboot.org/c/coreboot/+/28101/6/util/abuild/abuild@471 PS6, Line 471: include $(xcompile) > Doesn't this variable need to be defined in abuild? What if abuild is just run from the command lin […] It's defined by the main Makefile which I now include. The main Makefile is responsible for creating the .xcompile file. https://review.coreboot.org/c/coreboot/+/28101/6/util/abuild/abuild@476 PS6, Line 476: .DEFAULT_GOAL := missing_arches > Should this go in the other abuild commit? It doesn't need to. previously missing_arches was the first rule, now that I include the main Makefile it is required since mising_arches is not the first rule. So it could go in either or. -- To view, visit https://review.coreboot.org/c/coreboot/+/28101 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia83f234447b977efa824751c9674154b77d606b0 Gerrit-Change-Number: 28101 Gerrit-PatchSet: 6 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Comment-Date: Thu, 22 Aug 2019 16:20:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Martin Roth <martinroth(a)google.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/supermicro/x11ssh: Add Supermicro X11SSH-TF
by Angel Pons (Code Review) 22 Aug '19

22 Aug '19

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32734 ) Change subject: mb/supermicro/x11ssh: Add Supermicro X11SSH-TF ...................................................................... Patch Set 74: (21 comments) https://review.coreboot.org/c/coreboot/+/32734/74//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/32734/74//COMMIT_MSG@12 PS74, Line 12: SeaBios SeaBIOS, just like in https://www.seabios.org/SeaBIOS https://review.coreboot.org/c/coreboot/+/32734/74/Documentation/mainboard/s… File Documentation/mainboard/supermicro/x11ssh-tf.md: https://review.coreboot.org/c/coreboot/+/32734/74/Documentation/mainboard/s… PS74, Line 30: S5 resume S5 or S3 resume? https://review.coreboot.org/c/coreboot/+/32734/74/Documentation/mainboard/s… PS74, Line 37: 10GB Nit: That's probably 10 Gigabit (not GigaByte), so 10Gb https://review.coreboot.org/c/coreboot/+/32734/74/Documentation/mainboard/s… File Documentation/mainboard/supermicro/x11ssh_flash.jpg: PS74: Maybe highlight the SOIC-16 flash chip on the top edge of the mainboard as well, with some text saying which is which, or using a different color. https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/… File src/mainboard/supermicro/x11ssh/Kconfig: https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/… PS74, Line 67: : config SUBSYSTEM_VENDOR_ID : hex : default 0x8086 Is this used anywhere? https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/… File src/mainboard/supermicro/x11ssh/Kconfig.name: https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/… PS74, Line 1: BOARD_SUPERMICRO_X11SSH_PLUS_TF : bool "X11SSH+-TF" I don't see any 'X11SSH+-TF', so I'd remove the plus sign and "PLUS" from the Kconfig symbol name https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/… File src/mainboard/supermicro/x11ssh/bootblock.c: https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/… PS74, Line 37: garbeled garbled https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/… File src/mainboard/supermicro/x11ssh/mainboard.c: PS74: I think you can remove this file if it's empty https://review.coreboot.org/c/coreboot/+/32734/68/src/mainboard/supermicro/… File src/mainboard/supermicro/x11ssh/variants/tf/devicetree.cb: https://review.coreboot.org/c/coreboot/+/32734/68/src/mainboard/supermicro/… PS68, Line 210: device pci 01.1 on > Sorry, I meant "link training", not bus. […] According to MNL-1783, the PEG x16 port is split in two, one half is routed to SLOT6 (PCIe x8 slot), and the other half is routed to the LSI SAS3008 (only on the X11SSH-CTF, unpopulated on the X11SSH-TF). Looks like function 0 is for the SAS controller, whereas function 1 corresponds to the SLOT6 PCIe port. https://review.coreboot.org/c/coreboot/+/32734/68/src/mainboard/supermicro/… PS68, Line 211: 4X > X8 slot with X4 data width According to MNL-1783, SLOT6 is x8 electrical. https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/… File src/mainboard/supermicro/x11ssh/variants/tf/devicetree.cb: https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/… PS74, Line 30: "Device4Enable" = "1" This is not accounted for in the devicetree https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/… PS74, Line 114: X550T Maybe mention it's the 10GbE device https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/… PS74, Line 203: device domain 0 on Maybe add a 'subsystemid 0x1234 0x5678 inherit' line right after this line ('device domain 0 on') (of course, not with 0x1234 0x5678, but rather with values from the vendor firmware) https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/… PS74, Line 205: unused Looks like it's routed to the LSI SAS3008 (only on the X11SSH-CTF, unpopulated on the X11SSH-TF). https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/… PS74, Line 208: JPCIE1 SLOT6 Also, while you're at it, please add an empty newline to separate CPU devices from PCH devices, for clarity. https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/… PS74, Line 216: JPCIE1 SLOT4 https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/… PS74, Line 219: device pci 00.0 on end # Aspeed 2400 VGA Are the two nested instances of "device pci 00.0" intentional? https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/… PS74, Line 223: 10GBE Nit: 10GbE (since it's Gbit, not GByte) https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/… PS74, Line 239: device pnp 2e.0 off end This device doesn't seem to exist https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/… PS74, Line 263: device pnp 2e.b on # SUART3 Is this UART connected anywhere? https://review.coreboot.org/c/coreboot/+/32734/74/src/mainboard/supermicro/… PS74, Line 267: device pnp 2e.c on # SUART4 Is this UART connected anywhere? -- To view, visit https://review.coreboot.org/c/coreboot/+/32734 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2edaa4a928de3a065e517c0f20e3302b4b702323 Gerrit-Change-Number: 32734 Gerrit-PatchSet: 74 Gerrit-Owner: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Alexander Couzens <lynxis(a)fe80.eu> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Keno Fischer <keno(a)alumni.harvard.edu> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Name of user not set #1002358 Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: T. Hudson <trammell.hudson(a)gmail.com> Gerrit-Reviewer: Trammell Hudson <hudson(a)trmm.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Christoph Pomaska <github(a)aufmachen.jetzt> Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-CC: Maxim Polyakov <max.senia.poliak(a)gmail.com> Gerrit-CC: Michael Niewöhner Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Thu, 22 Aug 2019 15:20:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Patrick Rudolph <patrick.rudolph(a)9elements.com> Comment-In-Reply-To: Maxim Polyakov <max.senia.poliak(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: libpayload: usbmsc: Factor out usb_msc_force_init() function
by Patrick Georgi (Code Review) 22 Aug '19

22 Aug '19

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34227 ) Change subject: libpayload: usbmsc: Factor out usb_msc_force_init() function ...................................................................... libpayload: usbmsc: Factor out usb_msc_force_init() function We're planning to have a use case with a custom USB device that implements the USB mass storage protocol on its bulk endpoints, but does not have the normal MSC class/protocol interface descriptors and does not support class-specific control requests (Get Max LUN and Bulk-Only Reset). We'd like to identify/enumerate the device via usb_generic_create() in our payload but then reuse all the normal MSC driver code. In order to make that possible, this patch factors a new usb_msc_force_init() function out of usb_msc_init() which will initialize an MSC device without checking its descriptors. It also adds some "quirks" flags that allow devices registered this way to customize behavior of the MSC stack. Change-Id: I50392128409cb2a879954f234149a5e3b060a229 Signed-off-by: Julius Werner <jwerner(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/coreboot/+/34227 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Patrick Georgi <pgeorgi(a)google.com> --- M payloads/libpayload/drivers/usb/usbmsc.c M payloads/libpayload/include/usb/usbmsc.h 2 files changed, 35 insertions(+), 10 deletions(-) Approvals: build bot (Jenkins): Verified Patrick Georgi: Looks good to me, approved diff --git a/payloads/libpayload/drivers/usb/usbmsc.c b/payloads/libpayload/drivers/usb/usbmsc.c index d793e34..2412e99 100644 --- a/payloads/libpayload/drivers/usb/usbmsc.c +++ b/payloads/libpayload/drivers/usb/usbmsc.c @@ -157,6 +157,9 @@ dr.wIndex = 0; dr.wLength = 0; + if (MSC_INST (dev)->quirks & USB_MSC_QUIRK_NO_RESET) + return MSC_COMMAND_FAIL; + /* if any of these fails, detach device, as we are lost */ if (dev->controller->control (dev, OUT, sizeof (dr), &dr, 0, 0) < 0 || clear_stall (MSC_INST (dev)->bulk_in) || @@ -185,7 +188,8 @@ dr.wValue = 0; dr.wIndex = 0; dr.wLength = 1; - if (dev->controller->control (dev, IN, sizeof (dr), &dr, + if (MSC_INST (dev)->quirks & USB_MSC_QUIRK_NO_LUNS || + dev->controller->control (dev, IN, sizeof (dr), &dr, sizeof (msc->num_luns), &msc->num_luns) < 0) msc->num_luns = 0; /* assume only 1 lun if req fails */ msc->num_luns++; /* Get Max LUN returns number of last LUN */ @@ -600,14 +604,6 @@ void usb_msc_init (usbdev_t *dev) { - int i; - - /* init .data before setting .destroy */ - dev->data = NULL; - - dev->destroy = usb_msc_destroy; - dev->poll = usb_msc_poll; - configuration_descriptor_t *cd = (configuration_descriptor_t *) dev->configuration; interface_descriptor_t *interface = @@ -634,6 +630,19 @@ return; } + usb_msc_force_init (dev, 0); +} + +void usb_msc_force_init (usbdev_t *dev, u32 quirks) +{ + int i; + + /* init .data before setting .destroy */ + dev->data = NULL; + + dev->destroy = usb_msc_destroy; + dev->poll = usb_msc_poll; + dev->data = malloc (sizeof (usbmsc_inst_t)); if (!dev->data) fatal("Not enough memory for USB MSC device.\n"); @@ -641,6 +650,7 @@ MSC_INST (dev)->bulk_in = 0; MSC_INST (dev)->bulk_out = 0; MSC_INST (dev)->usbdisk_created = 0; + MSC_INST (dev)->quirks = quirks; for (i = 1; i <= dev->num_endp; i++) { if (dev->endpoints[i].endpoint == 0) diff --git a/payloads/libpayload/include/usb/usbmsc.h b/payloads/libpayload/include/usb/usbmsc.h index f4562a5..8786586 100644 --- a/payloads/libpayload/include/usb/usbmsc.h +++ b/payloads/libpayload/include/usb/usbmsc.h @@ -34,13 +34,24 @@ unsigned int numblocks; endpoint_t *bulk_in; endpoint_t *bulk_out; - u8 usbdisk_created; + u8 quirks : 7; + u8 usbdisk_created : 1; s8 ready; u8 lun; u8 num_luns; void *data; /* For use by consumers of libpayload. */ } usbmsc_inst_t; +/* Possible values for quirks field. */ +enum { + /* Don't check for LUNs (force assumption that there's only one LUN). */ + USB_MSC_QUIRK_NO_LUNS = 1 << 0, + /* Never do a BULK_ONLY reset, just continue. This means that the device + cannot recover from phase errors and won't detach automatically for + unrecoverable errors. Do not use unless you have to. */ + USB_MSC_QUIRK_NO_RESET = 1 << 1, +}; + /* Possible values for ready field. */ enum { USB_MSC_DETACHED = -1, /* Disk detached or out to lunch. */ @@ -56,4 +67,8 @@ int readwrite_blocks_512 (usbdev_t *dev, int start, int n, cbw_direction dir, u8 *buf); int readwrite_blocks (usbdev_t *dev, int start, int n, cbw_direction dir, u8 *buf); +/* Force a device to enumerate as MSC, without checking class/protocol types. + It must still have a bulk endpoint pair and respond to MSC commands. */ +void usb_msc_force_init (usbdev_t *dev, u32 quirks); + #endif -- To view, visit https://review.coreboot.org/c/coreboot/+/34227 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I50392128409cb2a879954f234149a5e3b060a229 Gerrit-Change-Number: 34227 Gerrit-PatchSet: 4 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

Change in coreboot[master]: libpayload: usbmsc: Factor out usb_msc_force_init() function
by Patrick Georgi (Code Review) 22 Aug '19

22 Aug '19

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34227 ) Change subject: libpayload: usbmsc: Factor out usb_msc_force_init() function ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/34227 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I50392128409cb2a879954f234149a5e3b060a229 Gerrit-Change-Number: 34227 Gerrit-PatchSet: 3 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 22 Aug 2019 10:37:24 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: libpayload: usbmsc: Skip zero-length packets at end of data
by Patrick Georgi (Code Review) 22 Aug '19

22 Aug '19

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34485 ) Change subject: libpayload: usbmsc: Skip zero-length packets at end of data ...................................................................... libpayload: usbmsc: Skip zero-length packets at end of data Some broken USB mass storage devices send another zero-length packet at the end of the data part of a transfer if the amount of data was evenly divisible by the packet size (which is pretty much always the case for block reads). This packet will get interpreted as the CSW and screw up the MSC state machine. This patch works around this issue by retrying the CSW transfer when it was received as exactly 0 bytes. This is the same mitigation the Linux kernel uses and harmless for correctly behaving devices. Also tighten validation of the CSW a little, making sure we verify the length before we read any fields and checking the signature in addition to the tag. Change-Id: I24f183f27b2c4f0142ba6c4b35b490c5798d0d21 Signed-off-by: Julius Werner <jwerner(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/coreboot/+/34485 Reviewed-by: Patrick Georgi <pgeorgi(a)google.com> Reviewed-by: Paul Menzel <paulepanter(a)users.sourceforge.net> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M payloads/libpayload/drivers/usb/usbmsc.c 1 file changed, 13 insertions(+), 4 deletions(-) Approvals: build bot (Jenkins): Verified Patrick Georgi: Looks good to me, approved Paul Menzel: Looks good to me, but someone else must approve diff --git a/payloads/libpayload/drivers/usb/usbmsc.c b/payloads/libpayload/drivers/usb/usbmsc.c index 2c7cbe5..d793e34 100644 --- a/payloads/libpayload/drivers/usb/usbmsc.c +++ b/payloads/libpayload/drivers/usb/usbmsc.c @@ -218,14 +218,23 @@ static int get_csw (endpoint_t *ep, csw_t *csw) { - if (ep->dev->controller->bulk (ep, sizeof (csw_t), (u8 *) csw, 1) < 0) { + hci_t *ctrlr = ep->dev->controller; + int ret = ctrlr->bulk (ep, sizeof (csw_t), (u8 *) csw, 1); + + /* Some broken sticks send a zero-length packet at the end of their data + transfer which would show up here. Skip it to get the actual CSW. */ + if (ret == 0) + ret = ctrlr->bulk (ep, sizeof (csw_t), (u8 *) csw, 1); + + if (ret < 0) { clear_stall (ep); - if (ep->dev->controller->bulk - (ep, sizeof (csw_t), (u8 *) csw, 1) < 0) { + if (ctrlr->bulk (ep, sizeof (csw_t), (u8 *) csw, 1) < 0) { return reset_transport (ep->dev); } } - if (csw->dCSWTag != tag) { + if (ret != sizeof(csw_t) || csw->dCSWTag != tag || + csw->dCSWSignature != csw_signature) { + usb_debug ("MSC: received malformed CSW\n"); return reset_transport (ep->dev); } return MSC_COMMAND_OK; -- To view, visit https://review.coreboot.org/c/coreboot/+/34485 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I24f183f27b2c4f0142ba6c4b35b490c5798d0d21 Gerrit-Change-Number: 34485 Gerrit-PatchSet: 5 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

Change in coreboot[master]: Add buffer_to/from_fifo32(_prefix) helpers
by Julius Werner (Code Review) 22 Aug '19

22 Aug '19

Hello Hung-Te Lin, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/34817 to review the following change. Change subject: Add buffer_to/from_fifo32(_prefix) helpers ...................................................................... Add buffer_to/from_fifo32(_prefix) helpers Many peripheral drivers across different SoCs regularly face the same task of piping a transfer buffer into (or reading it out of) a 32-bit FIFO register. Sometimes it's just one register, sometimes a whole array of registers. Sometimes you actually transfer 4 bytes per register read/write, sometimes only 2 (or even 1). Sometimes writes need to be prefixed with one or two command bytes which makes the actual payload buffer "misaligned" in relation to the FIFO and requires a bunch of tricky bit packing logic to get right. Most of the times transfer lengths are not guaranteed to be divisible by 4, which also requires a bunch of logic to treat the potential unaligned end of the transfer correctly. We have a dozen different implementations of this same pattern across coreboot. This patch introduces a new family of helper functions that aims to solve all these use cases once and for all (*fingers crossed*), and demonstrates their use in the Rockchip SPI and I2C drivers. Change-Id: Ifcf37c6d56f949f620c347df05439b05c3b8d77d Signed-off-by: Julius Werner <jwerner(a)chromium.org> --- M payloads/libpayload/include/libpayload.h M payloads/libpayload/libc/lib.c M src/device/Makefile.inc A src/device/mmio.c M src/include/device/mmio.h M src/soc/rockchip/common/i2c.c M src/soc/rockchip/common/spi.c 7 files changed, 183 insertions(+), 54 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/34817/1 diff --git a/payloads/libpayload/include/libpayload.h b/payloads/libpayload/include/libpayload.h index 57a3afc..a25cff3 100644 --- a/payloads/libpayload/include/libpayload.h +++ b/payloads/libpayload/include/libpayload.h @@ -446,6 +446,23 @@ /** + * @defgroup mmio MMIO helper functions + * @{ + */ +void buffer_from_fifo32(void *buffer, size_t size, void *fifo, + int fifo_stride, int fifo_width); +void buffer_to_fifo32_prefix(void *buffer, u32 prefix, int prefsz, size_t size, + void *fifo, int fifo_stride, int fifo_width); +static inline void buffer_to_fifo32(void *buffer, size_t size, void *fifo, + int fifo_stride, int fifo_width) +{ + buffer_to_fifo32_prefix(buffer, size, 0, 0, fifo, + fifo_stride, fifo_width); +} +/** @} */ + + +/** * @defgroup hash Hashing functions * @{ */ diff --git a/payloads/libpayload/libc/lib.c b/payloads/libpayload/libc/lib.c index bead1f8..d761aad 100644 --- a/payloads/libpayload/libc/lib.c +++ b/payloads/libpayload/libc/lib.c @@ -27,6 +27,7 @@ * SUCH DAMAGE. */ +#include <assert.h> #include <libpayload.h> /* @@ -125,3 +126,52 @@ { return NULL; } + +/* + * Reads a transfer buffer from 32-bit FIFO registers. fifo_stride is the + * distance in bytes between registers (e.g. pass 4 for a normal array of 32-bit + * registers or 0 to read everything from the same register). fifo_width is + * the amount of bytes read per register (can be 1 through 4). + */ +void buffer_from_fifo32(void *buffer, size_t size, void *fifo, + int fifo_stride, int fifo_width) +{ + u8 *p = buffer; + int i, j; + + assert(fifo_width > 0 && fifo_width <= sizeof(u32) && + fifo_stride % sizeof(u32) == 0); + + for (i = 0; i < size; i += fifo_width, fifo += fifo_stride) { + u32 val = read32(fifo); + for (j = 0; j < MIN(size - i, fifo_width); j++) + *p++ = (u8)(val >> (j * 8)); + } +} + +/* + * Version of buffer_to_fifo32() that can prepend a prefix of up to fifo_width + * size to the transfer. This is often useful for protocols where a command word + * precedes the actual payload data. The prefix must be packed in the low-order + * bytes of the 'prefix' u32 parameter and any high-order bytes exceeding prefsz + * must be 0. Note that 'size' counts total bytes written, including 'prefsz'. + */ +void buffer_to_fifo32_prefix(void *buffer, u32 prefix, int prefsz, size_t size, + void *fifo, int fifo_stride, int fifo_width) +{ + u8 *p = buffer; + int i, j = prefsz; + + assert(fifo_width > 0 && fifo_width <= sizeof(u32) && + fifo_stride % sizeof(u32) == 0 && prefsz <= fifo_width); + + uint32_t val = prefix; + for (i = 0; i < size; i += fifo_width, fifo += fifo_stride) { + for (; j < MIN(size - i, fifo_width); j++) + val |= *p++ << (j * 8); + write32(fifo, val); + val = 0; + j = 0; + } + +} diff --git a/src/device/Makefile.inc b/src/device/Makefile.inc index baa45be..966ca0d 100644 --- a/src/device/Makefile.inc +++ b/src/device/Makefile.inc @@ -53,3 +53,8 @@ romstage-y += i2c.c ramstage-y += i2c.c ramstage-y += i2c_bus.c + +bootblock-y += mmio.c +verstage-y += mmio.c +romstage-y += mmio.c +ramstage-y += mmio.c diff --git a/src/device/mmio.c b/src/device/mmio.c new file mode 100644 index 0000000..61a1130 --- /dev/null +++ b/src/device/mmio.c @@ -0,0 +1,55 @@ +/* + * 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 <assert.h> +#include <device/mmio.h> + +/* Helper functions for various MMIO access patterns. */ + +void buffer_from_fifo32(void *buffer, size_t size, void *fifo, + int fifo_stride, int fifo_width) +{ + u8 *p = buffer; + int i, j; + + assert(fifo_width > 0 && fifo_width <= sizeof(u32) && + fifo_stride % sizeof(u32) == 0); + + for (i = 0; i < size; i += fifo_width, fifo += fifo_stride) { + u32 val = read32(fifo); + for (j = 0; j < MIN(size - i, fifo_width); j++) + *p++ = (u8)(val >> (j * 8)); + } +} + +void buffer_to_fifo32_prefix(void *buffer, u32 prefix, int prefsz, size_t size, + void *fifo, int fifo_stride, int fifo_width) +{ + u8 *p = buffer; + int i, j = prefsz; + + assert(fifo_width > 0 && fifo_width <= sizeof(u32) && + fifo_stride % sizeof(u32) == 0 && prefsz <= fifo_width); + + uint32_t val = prefix; + for (i = 0; i < size; i += fifo_width, fifo += fifo_stride) { + for (; j < MIN(size - i, fifo_width); j++) + val |= *p++ << (j * 8); + write32(fifo, val); + val = 0; + j = 0; + } + +} diff --git a/src/include/device/mmio.h b/src/include/device/mmio.h index 34be1e8..4b6d1e2 100644 --- a/src/include/device/mmio.h +++ b/src/include/device/mmio.h @@ -17,5 +17,38 @@ #include <arch/mmio.h> #include <endian.h> +#include <types.h> + +/* + * Reads a transfer buffer from 32-bit FIFO registers. fifo_stride is the + * distance in bytes between registers (e.g. pass 4 for a normal array of 32-bit + * registers or 0 to read everything from the same register). fifo_width is + * the amount of bytes read per register (can be 1 through 4). + */ +void buffer_from_fifo32(void *buffer, size_t size, void *fifo, + int fifo_stride, int fifo_width); + +/* + * Version of buffer_to_fifo32() that can prepend a prefix of up to fifo_width + * size to the transfer. This is often useful for protocols where a command word + * precedes the actual payload data. The prefix must be packed in the low-order + * bytes of the 'prefix' u32 parameter and any high-order bytes exceeding prefsz + * must be 0. Note that 'size' counts total bytes written, including 'prefsz'. + */ +void buffer_to_fifo32_prefix(void *buffer, u32 prefix, int prefsz, size_t size, + void *fifo, int fifo_stride, int fifo_width); + +/* + * Writes a transfer buffer into 32-bit FIFO registers. fifo_stride is the + * distance in bytes between registers (e.g. pass 4 for a normal array of 32-bit + * registers or 0 to write everything into the same register). fifo_width is + * the amount of bytes written per register (can be 1 through 4). + */ +static inline void buffer_to_fifo32(void *buffer, size_t size, void *fifo, + int fifo_stride, int fifo_width) +{ + buffer_to_fifo32_prefix(buffer, size, 0, 0, fifo, + fifo_stride, fifo_width); +} #endif diff --git a/src/soc/rockchip/common/i2c.c b/src/soc/rockchip/common/i2c.c index e5f5a9a..cd0ed9b 100644 --- a/src/soc/rockchip/common/i2c.c +++ b/src/soc/rockchip/common/i2c.c @@ -128,25 +128,21 @@ uint8_t *data = segment.buf; int timeout = I2C_TIMEOUT_US; unsigned int bytes_remaining = segment.len; - unsigned int bytes_transferred = 0; - unsigned int words_transferred = 0; - unsigned int rxdata = 0; unsigned int con = 0; - unsigned int i, j; write32(&reg_addr->i2c_mrxaddr, I2C_8BIT | segment.slave << 1 | 1); write32(&reg_addr->i2c_mrxraddr, 0); con = I2C_MODE_TRX | I2C_EN | I2C_ACT2NAK; while (bytes_remaining) { - bytes_transferred = MIN(bytes_remaining, 32); - bytes_remaining -= bytes_transferred; + size_t size = MIN(bytes_remaining, 32); + bytes_remaining -= size; if (!bytes_remaining) con |= I2C_EN | I2C_NAK; - words_transferred = ALIGN_UP(bytes_transferred, 4) / 4; + i2c_info("I2C Read::%zu bytes\n", size); write32(&reg_addr->i2c_ipd, I2C_CLEANI); write32(&reg_addr->i2c_con, con); - write32(&reg_addr->i2c_mrxcnt, bytes_transferred); + write32(&reg_addr->i2c_mrxcnt, size); timeout = I2C_TIMEOUT_US; while (timeout--) { @@ -166,15 +162,8 @@ return I2C_TIMEOUT; } - for (i = 0; i < words_transferred; i++) { - rxdata = read32(&reg_addr->rxdata[i]); - i2c_info("I2c Read::RXDATA[%d] = 0x%x\n", i, rxdata); - for (j = 0; j < 4; j++) { - if ((i * 4 + j) == bytes_transferred) - break; - *data++ = (rxdata >> (j * 8)) & 0xff; - } - } + buffer_from_fifo32(data, size, &reg_addr->rxdata, 4, 4); + data += size; con = I2C_MODE_RX | I2C_EN | I2C_ACT2NAK; } return res; @@ -186,32 +175,22 @@ uint8_t *data = segment.buf; int timeout = I2C_TIMEOUT_US; int bytes_remaining = segment.len + 1; - int bytes_transferred = 0; - int words_transferred = 0; - unsigned int i; - unsigned int j = 1; - u32 txdata = 0; - txdata |= (segment.slave << 1); + /* Prepend one byte for the slave address to the transfer. */ + u32 prefix = segment.slave << 1; + int prefsz = 1; + while (bytes_remaining) { - bytes_transferred = MIN(bytes_remaining, 32); - words_transferred = ALIGN_UP(bytes_transferred, 4) / 4; - for (i = 0; i < words_transferred; i++) { - do { - if ((i * 4 + j) == bytes_transferred) - break; - txdata |= (*data++) << (j * 8); - } while (++j < 4); - write32(&reg_addr->txdata[i], txdata); - j = 0; - i2c_info("I2c Write::TXDATA[%d] = 0x%x\n", i, txdata); - txdata = 0; - } + size_t size = MIN(bytes_remaining, 32); + buffer_to_fifo32_prefix(data, prefix, prefsz, size, + &reg_addr->txdata, 4, 4); + data += size - prefsz; + i2c_info("I2C Write::%zu bytes\n", size); write32(&reg_addr->i2c_ipd, I2C_CLEANI); write32(&reg_addr->i2c_con, I2C_EN | I2C_MODE_TX | I2C_ACT2NAK); - write32(&reg_addr->i2c_mtxcnt, bytes_transferred); + write32(&reg_addr->i2c_mtxcnt, size); timeout = I2C_TIMEOUT_US; while (timeout--) { @@ -232,7 +211,9 @@ return I2C_TIMEOUT; } - bytes_remaining -= bytes_transferred; + bytes_remaining -= size; + prefsz = 0; + prefix = 0; } return res; } diff --git a/src/soc/rockchip/common/spi.c b/src/soc/rockchip/common/spi.c index 7bde433..0307e24 100644 --- a/src/soc/rockchip/common/spi.c +++ b/src/soc/rockchip/common/spi.c @@ -221,23 +221,11 @@ * sychronizing with the SPI clock which is pretty slow. */ if (*bytes_in && !(sr & SR_RF_EMPT)) { - int fifo = read32(&regs->rxflr) & RXFLR_LEVEL_MASK; - int val; - - if (use_16bit) - xferred = fifo * 2; - else - xferred = fifo; + int w = use_16bit ? 2 : 1; + xferred = (read32(&regs->rxflr) & RXFLR_LEVEL_MASK) * w; + buffer_from_fifo32(in_buf, xferred, &regs->rxdr, 0, w); *bytes_in -= xferred; - while (fifo-- > 0) { - val = read32(&regs->rxdr); - if (use_16bit) { - *in_buf++ = val & 0xff; - *in_buf++ = (val >> 8) & 0xff; - } else { - *in_buf++ = val & 0xff; - } - } + in_buf += xferred; } min_xfer -= xferred; -- To view, visit https://review.coreboot.org/c/coreboot/+/34817 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifcf37c6d56f949f620c347df05439b05c3b8d77d Gerrit-Change-Number: 34817 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-MessageType: newchange

3 9

Change in coreboot[master]: Add buffer_to/from_fifo32(_prefix) helpers
by Julius Werner (Code Review) 22 Aug '19

22 Aug '19

Hello Hung-Te Lin, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/34850 to review the following change. Change subject: Add buffer_to/from_fifo32(_prefix) helpers ...................................................................... Add buffer_to/from_fifo32(_prefix) helpers Many peripheral drivers across different SoCs regularly face the same task of piping a transfer buffer into (or reading it out of) a 32-bit FIFO register. Sometimes it's just one register, sometimes a whole array of registers. Sometimes you actually transfer 4 bytes per register read/write, sometimes only 2 (or even 1). Sometimes writes need to be prefixed with one or two command bytes which makes the actual payload buffer "misaligned" in relation to the FIFO and requires a bunch of tricky bit packing logic to get right. Most of the times transfer lengths are not guaranteed to be divisible by 4, which also requires a bunch of logic to treat the potential unaligned end of the transfer correctly. We have a dozen different implementations of this same pattern across coreboot. This patch introduces a new family of helper functions that aims to solve all these use cases once and for all (*fingers crossed*). Change-Id: Ia71f66c1cee530afa4c77c46a838b4de646ffcfb Signed-off-by: Julius Werner <jwerner(a)chromium.org> --- M payloads/libpayload/include/libpayload.h M payloads/libpayload/libc/lib.c M src/device/Makefile.inc A src/device/mmio.c M src/include/device/mmio.h 5 files changed, 167 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/34850/1 diff --git a/payloads/libpayload/include/libpayload.h b/payloads/libpayload/include/libpayload.h index 57a3afc..0f75790 100644 --- a/payloads/libpayload/include/libpayload.h +++ b/payloads/libpayload/include/libpayload.h @@ -446,6 +446,25 @@ /** + * @defgroup mmio MMIO helper functions + * @{ + */ +#if !CONFIG(ARCH_MIPS) +void buffer_from_fifo32(void *buffer, size_t size, void *fifo, + int fifo_stride, int fifo_width); +void buffer_to_fifo32_prefix(void *buffer, u32 prefix, int prefsz, size_t size, + void *fifo, int fifo_stride, int fifo_width); +static inline void buffer_to_fifo32(void *buffer, size_t size, void *fifo, + int fifo_stride, int fifo_width) +{ + buffer_to_fifo32_prefix(buffer, size, 0, 0, fifo, + fifo_stride, fifo_width); +} +#endif +/** @} */ + + +/** * @defgroup hash Hashing functions * @{ */ diff --git a/payloads/libpayload/libc/lib.c b/payloads/libpayload/libc/lib.c index bead1f8..7b20486 100644 --- a/payloads/libpayload/libc/lib.c +++ b/payloads/libpayload/libc/lib.c @@ -27,6 +27,7 @@ * SUCH DAMAGE. */ +#include <assert.h> #include <libpayload.h> /* @@ -125,3 +126,54 @@ { return NULL; } + +#if !CONFIG(ARCH_MIPS) +/* + * Reads a transfer buffer from 32-bit FIFO registers. fifo_stride is the + * distance in bytes between registers (e.g. pass 4 for a normal array of 32-bit + * registers or 0 to read everything from the same register). fifo_width is + * the amount of bytes read per register (can be 1 through 4). + */ +void buffer_from_fifo32(void *buffer, size_t size, void *fifo, + int fifo_stride, int fifo_width) +{ + u8 *p = buffer; + int i, j; + + assert(fifo_width > 0 && fifo_width <= sizeof(u32) && + fifo_stride % sizeof(u32) == 0); + + for (i = 0; i < size; i += fifo_width, fifo += fifo_stride) { + u32 val = read32(fifo); + for (j = 0; j < MIN(size - i, fifo_width); j++) + *p++ = (u8)(val >> (j * 8)); + } +} + +/* + * Version of buffer_to_fifo32() that can prepend a prefix of up to fifo_width + * size to the transfer. This is often useful for protocols where a command word + * precedes the actual payload data. The prefix must be packed in the low-order + * bytes of the 'prefix' u32 parameter and any high-order bytes exceeding prefsz + * must be 0. Note that 'size' counts total bytes written, including 'prefsz'. + */ +void buffer_to_fifo32_prefix(void *buffer, u32 prefix, int prefsz, size_t size, + void *fifo, int fifo_stride, int fifo_width) +{ + u8 *p = buffer; + int i, j = prefsz; + + assert(fifo_width > 0 && fifo_width <= sizeof(u32) && + fifo_stride % sizeof(u32) == 0 && prefsz <= fifo_width); + + uint32_t val = prefix; + for (i = 0; i < size; i += fifo_width, fifo += fifo_stride) { + for (; j < MIN(size - i, fifo_width); j++) + val |= *p++ << (j * 8); + write32(fifo, val); + val = 0; + j = 0; + } + +} +#endif diff --git a/src/device/Makefile.inc b/src/device/Makefile.inc index baa45be..966ca0d 100644 --- a/src/device/Makefile.inc +++ b/src/device/Makefile.inc @@ -53,3 +53,8 @@ romstage-y += i2c.c ramstage-y += i2c.c ramstage-y += i2c_bus.c + +bootblock-y += mmio.c +verstage-y += mmio.c +romstage-y += mmio.c +ramstage-y += mmio.c diff --git a/src/device/mmio.c b/src/device/mmio.c new file mode 100644 index 0000000..61a1130 --- /dev/null +++ b/src/device/mmio.c @@ -0,0 +1,55 @@ +/* + * 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 <assert.h> +#include <device/mmio.h> + +/* Helper functions for various MMIO access patterns. */ + +void buffer_from_fifo32(void *buffer, size_t size, void *fifo, + int fifo_stride, int fifo_width) +{ + u8 *p = buffer; + int i, j; + + assert(fifo_width > 0 && fifo_width <= sizeof(u32) && + fifo_stride % sizeof(u32) == 0); + + for (i = 0; i < size; i += fifo_width, fifo += fifo_stride) { + u32 val = read32(fifo); + for (j = 0; j < MIN(size - i, fifo_width); j++) + *p++ = (u8)(val >> (j * 8)); + } +} + +void buffer_to_fifo32_prefix(void *buffer, u32 prefix, int prefsz, size_t size, + void *fifo, int fifo_stride, int fifo_width) +{ + u8 *p = buffer; + int i, j = prefsz; + + assert(fifo_width > 0 && fifo_width <= sizeof(u32) && + fifo_stride % sizeof(u32) == 0 && prefsz <= fifo_width); + + uint32_t val = prefix; + for (i = 0; i < size; i += fifo_width, fifo += fifo_stride) { + for (; j < MIN(size - i, fifo_width); j++) + val |= *p++ << (j * 8); + write32(fifo, val); + val = 0; + j = 0; + } + +} diff --git a/src/include/device/mmio.h b/src/include/device/mmio.h index 34be1e8..e40b40f 100644 --- a/src/include/device/mmio.h +++ b/src/include/device/mmio.h @@ -17,5 +17,40 @@ #include <arch/mmio.h> #include <endian.h> +#include <types.h> -#endif +#ifndef __ROMCC__ +/* + * Reads a transfer buffer from 32-bit FIFO registers. fifo_stride is the + * distance in bytes between registers (e.g. pass 4 for a normal array of 32-bit + * registers or 0 to read everything from the same register). fifo_width is + * the amount of bytes read per register (can be 1 through 4). + */ +void buffer_from_fifo32(void *buffer, size_t size, void *fifo, + int fifo_stride, int fifo_width); + +/* + * Version of buffer_to_fifo32() that can prepend a prefix of up to fifo_width + * size to the transfer. This is often useful for protocols where a command word + * precedes the actual payload data. The prefix must be packed in the low-order + * bytes of the 'prefix' u32 parameter and any high-order bytes exceeding prefsz + * must be 0. Note that 'size' counts total bytes written, including 'prefsz'. + */ +void buffer_to_fifo32_prefix(void *buffer, u32 prefix, int prefsz, size_t size, + void *fifo, int fifo_stride, int fifo_width); + +/* + * Writes a transfer buffer into 32-bit FIFO registers. fifo_stride is the + * distance in bytes between registers (e.g. pass 4 for a normal array of 32-bit + * registers or 0 to write everything into the same register). fifo_width is + * the amount of bytes written per register (can be 1 through 4). + */ +static inline void buffer_to_fifo32(void *buffer, size_t size, void *fifo, + int fifo_stride, int fifo_width) +{ + buffer_to_fifo32_prefix(buffer, size, 0, 0, fifo, + fifo_stride, fifo_width); +} +#endif /* !__ROMCC__ */ + +#endif /* __DEVICE_MMIO_H__ */ -- To view, visit https://review.coreboot.org/c/coreboot/+/34850 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia71f66c1cee530afa4c77c46a838b4de646ffcfb Gerrit-Change-Number: 34850 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-MessageType: newchange

3 6

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit August 2019