Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35762 )
Change subject: sb/intel/ibexpeak: Implement USB current settings ......................................................................
sb/intel/ibexpeak: Implement USB current settings
This is based on the sandybridge settings. The current lookup table comes from the x201 vendor lookup table.
Change-Id: Icea9673623a62e7039d5700100a2ee238478abd1 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/mainboard/lenovo/x201/romstage.c M src/mainboard/packardbell/ms2290/romstage.c M src/southbridge/intel/ibexpeak/Makefile.inc A src/southbridge/intel/ibexpeak/early_usb.c M src/southbridge/intel/ibexpeak/pch.h 5 files changed, 159 insertions(+), 47 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/35762/1
diff --git a/src/mainboard/lenovo/x201/romstage.c b/src/mainboard/lenovo/x201/romstage.c index 04d1d79..633bb21 100644 --- a/src/mainboard/lenovo/x201/romstage.c +++ b/src/mainboard/lenovo/x201/romstage.c @@ -61,6 +61,24 @@ pci_read_config32(PCH_LPC_DEV, ETR3) & ~ETR3_CF9GR); }
+const struct southbridge_usb_port mainboard_usb_ports[] = { + /* Enabled, Current table lookup index, OC map */ + { 1, IF1_557, 0 }, + { 1, IF1_55F, 1 }, + { 1, IF1_74B, 3 }, + { 1, IF1_74B, 3 }, + { 1, IF1_557, 3 }, + { 1, IF1_14B, 3 }, + { 1, IF1_74B, 3 }, + { 1, IF1_74B, 3 }, + { 1, IF1_74B, 4 }, + { 1, IF1_74B, 5 }, + { 1, IF1_55F, 7 }, + { 1, IF1_55F, 7 }, + { 1, IF1_557, 7 }, + { 1, IF1_55F, 7 }, +}; + static void rcba_config(void) { southbridge_configure_default_intmap(); @@ -74,29 +92,7 @@ /* Set reserved bit to 1 */ RCBA32(FD2) = 1;
- static const u32 rcba_dump3[] = { - /* 3500 */ 0x20000557, 0x2000055f, 0x2000074b, 0x2000074b, - /* 3510 */ 0x20000557, 0x2000014b, 0x2000074b, 0x2000074b, - /* 3520 */ 0x2000074b, 0x2000074b, 0x2000055f, 0x2000055f, - /* 3530 */ 0x20000557, 0x2000055f, 0x00000000, 0x00000000, - /* 3540 */ 0x00000000, 0x00000000, 0x00000000, 0x00000000, - /* 3550 */ 0x00000000, 0x00000000, 0x00000000, 0x00000000, - /* 3560 */ 0x00000001, 0x000026a3, 0x00040002, 0x01000052, - /* 3570 */ 0x02000772, 0x16000f8f, 0x1800ff4f, 0x0001d630, - /* 3580 */ 0x00000000, 0x00000000, 0x00000000, 0x00000000, - /* 3590 */ 0x00000000, 0x00000000, 0x00000000, 0x00000000, - /* 35a0 */ 0xfc000201, 0x3c000201, 0x00000000, 0x00000000, - /* 35b0 */ 0x00000000, 0x00000000, 0x00000000, 0x00000000, - /* 35c0 */ 0x00000000, 0x00000000, 0x00000000, 0x00000000, - /* 35d0 */ 0x00000000, 0x00000000, 0x00000000, 0x00000000, - /* 35e0 */ 0x00000000, 0x00000000, 0x00000000, 0x00000000, - /* 35f0 */ 0x00000000, 0x00000000, 0x00000000, 0x00000000, - }; - unsigned i; - for (i = 0; i < sizeof(rcba_dump3) / 4; i++) { - RCBA32(4 * i + 0x3500) = rcba_dump3[i]; - (void)RCBA32(4 * i + 0x3500); - } + early_usb_init(mainboard_usb_ports); }
static inline void write_acpi32(u32 addr, u32 val) diff --git a/src/mainboard/packardbell/ms2290/romstage.c b/src/mainboard/packardbell/ms2290/romstage.c index b4b9755..e2414bd 100644 --- a/src/mainboard/packardbell/ms2290/romstage.c +++ b/src/mainboard/packardbell/ms2290/romstage.c @@ -56,6 +56,25 @@ pci_read_config32(PCH_LPC_DEV, ETR3) & ~ETR3_CF9GR); }
+/* Seems copied from Lenovo Thinkpad x201, might be wrong */ +const struct southbridge_usb_port mainboard_usb_ports[] = { + /* Enabled, Current table lookup index, OC map */ + { 1, IF1_557, 0 }, + { 1, IF1_55F, 1 }, + { 1, IF1_74B, 3 }, + { 1, IF1_74B, 3 }, + { 1, IF1_557, 3 }, + { 1, IF1_14B, 3 }, + { 1, IF1_74B, 3 }, + { 1, IF1_74B, 3 }, + { 1, IF1_74B, 4 }, + { 1, IF1_74B, 5 }, + { 1, IF1_55F, 7 }, + { 1, IF1_55F, 7 }, + { 1, IF1_557, 7 }, + { 1, IF1_55F, 7 }, +}; + static void rcba_config(void) { southbridge_configure_default_intmap(); @@ -69,30 +88,7 @@ /* Set reserved bit to 1 */ RCBA32(FD2) = 1;
- static const u32 rcba_dump3[] = { - /* 3500 */ 0x20000557, 0x2000055f, 0x2000074b, 0x2000074b, - /* 3510 */ 0x20000557, 0x2000014b, 0x2000074b, 0x2000074b, - /* 3520 */ 0x2000074b, 0x2000074b, 0x2000055f, 0x2000055f, - /* 3530 */ 0x20000557, 0x2000055f, 0x00000000, 0x00000000, - /* 3540 */ 0x00000000, 0x00000000, 0x00000000, 0x00000000, - /* 3550 */ 0x00000000, 0x00000000, 0x00000000, 0x00000000, - /* 3560 */ 0x00000001, 0x000026a3, 0x00040002, 0x01000052, - /* 3570 */ 0x02000772, 0x16000f8f, 0x1800ff4f, 0x0001d630, - /* 3580 */ 0x00000000, 0x00000000, 0x00000000, 0x00000000, - /* 3590 */ 0x00000000, 0x00000000, 0x00000000, 0x00000000, - /* 35a0 */ 0xfc000201, 0x3c000201, 0x00000000, 0x00000000, - /* 35b0 */ 0x00000000, 0x00000000, 0x00000000, 0x00000000, - /* 35c0 */ 0x00000000, 0x00000000, 0x00000000, 0x00000000, - /* 35d0 */ 0x00000000, 0x00000000, 0x00000000, 0x00000000, - /* 35e0 */ 0x00000000, 0x00000000, 0x00000000, 0x00000000, - /* 35f0 */ 0x00000000, 0x00000000, 0x00000000, 0x00000000, - }; - unsigned i; - - for (i = 0; i < sizeof(rcba_dump3) / 4; i++) { - RCBA32(4 * i + 0x3500) = rcba_dump3[i]; - (void)RCBA32(4 * i + 0x3500); - } + early_usb_init(mainboard_usb_ports); }
static inline void write_acpi32(u32 addr, u32 val) diff --git a/src/southbridge/intel/ibexpeak/Makefile.inc b/src/southbridge/intel/ibexpeak/Makefile.inc index 9a6b36e..566461d 100644 --- a/src/southbridge/intel/ibexpeak/Makefile.inc +++ b/src/southbridge/intel/ibexpeak/Makefile.inc @@ -43,5 +43,6 @@ romstage-y += early_thermal.c romstage-y += ../bd82x6x/early_rcba.c romstage-y += early_cir.c +romstage-y += early_usb.c
endif diff --git a/src/southbridge/intel/ibexpeak/early_usb.c b/src/southbridge/intel/ibexpeak/early_usb.c new file mode 100644 index 0000000..cdffd97 --- /dev/null +++ b/src/southbridge/intel/ibexpeak/early_usb.c @@ -0,0 +1,64 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2014 Vladimir Serbinenko + * + * 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 <device/mmio.h> +#include <device/pci_ops.h> +#include <device/pci_def.h> +#include <northbridge/intel/sandybridge/sandybridge.h> +#include <southbridge/intel/common/rcba.h> +#include <southbridge/intel/common/pmbase.h> + +#include "pch.h" + +void early_usb_init(const struct southbridge_usb_port *portmap) +{ + u32 reg32; + const u16 currents[] = { 0xf57, 0xf5f, 0x753, 0x75f, + 0x14b, 0x74b, 0x557, 0x757, + 0x55f, 0x54b + }; + int i; + + /* Unlock registers. */ + write_pmbase16(UPRWC, read_pmbase16(UPRWC) | UPRWC_WR_EN); + + for (i = 0; i < 14; i++) + RCBA32_AND_OR(USBIR0 + 4 * i, ~0xfff, currents[portmap[i].current]); + + /* USB Initialization Registers */ + RCBA32(USBIRC) &= ~(1 << 8); + RCBA32_OR(USBIRA, (7 << 12) | (7 << 8) | (7 << 4) | (2 << 0)); + RCBA32_AND_OR(USBIRB, ~0x617f0, (3 << 17) | (1 << 12) | (1 << 10) | (1 << 8) | (4 << 4)); + + reg32 = 0; + for (i = 0; i < 14; i++) + if (!portmap[i].enabled) + reg32 |= (1 << i); + RCBA32(USBPDO) = reg32; + reg32 = 0; + for (i = 0; i < 8; i++) + if (portmap[i].enabled && portmap[i].oc_pin >= 0) + reg32 |= (1 << (i + 8 * portmap[i].oc_pin)); + RCBA32(USBOCM1) = reg32; + reg32 = 0; + for (i = 8; i < 14; i++) + if (portmap[i].enabled && portmap[i].oc_pin >= 4) + reg32 |= (1 << (i - 8 + 8 * (portmap[i].oc_pin - 4))); + RCBA32(USBOCM2) = reg32; + + /* Relock registers. */ + write_pmbase16(UPRWC, 0); +} diff --git a/src/southbridge/intel/ibexpeak/pch.h b/src/southbridge/intel/ibexpeak/pch.h index f0e469a..73e89d4 100644 --- a/src/southbridge/intel/ibexpeak/pch.h +++ b/src/southbridge/intel/ibexpeak/pch.h @@ -69,6 +69,27 @@ void southbridge_configure_default_intmap(void); void pch_setup_cir(int chipset_type);
+enum current_lookup_idx { + IF1_F57 = 0, + IF1_F5F, + IF1_753, + IF1_75F, + IF1_14B, + IF1_74B, + IF1_557, + IF1_757, + IF1_55F, + IF1_54B, +}; + +struct southbridge_usb_port +{ + int enabled; + enum current_lookup_idx current; + int oc_pin; +}; +void early_usb_init(const struct southbridge_usb_port *portmap); + #ifndef __ROMCC__ #include <device/device.h> void pch_enable(struct device *dev); @@ -78,6 +99,10 @@ #define MAINBOARD_POWER_ON 1 #define MAINBOARD_POWER_KEEP 2
+/* PM I/O Space */ +#define UPRWC 0x3c +#define UPRWC_WR_EN (1 << 1) /* USB Per-Port Registers Write Enable */ + /* PCI Configuration Space (D30:F0): PCI2PCI */ #define PSTS 0x06 #define SMLT 0x1b @@ -404,6 +429,36 @@ #define PCH_DISABLE_MEI1 (1 << 1) #define PCH_ENABLE_DBDF (1 << 0)
+/* USB Initialization Registers[13:0] */ +#define USBIR0 0x3500 /* 32bit */ +#define USBIR1 0x3504 /* 32bit */ +#define USBIR2 0x3508 /* 32bit */ +#define USBIR3 0x350c /* 32bit */ +#define USBIR4 0x3510 /* 32bit */ +#define USBIR5 0x3514 /* 32bit */ +#define USBIR6 0x3518 /* 32bit */ +#define USBIR7 0x351c /* 32bit */ +#define USBIR8 0x3520 /* 32bit */ +#define USBIR9 0x3524 /* 32bit */ +#define USBIR10 0x3528 /* 32bit */ +#define USBIR11 0x352c /* 32bit */ +#define USBIR12 0x3530 /* 32bit */ +#define USBIR13 0x3534 /* 32bit */ + +#define USBIRC 0x3564 /* 32bit */ +#define USBIRA 0x3570 /* 32bit */ +#define USBIRB 0x357c /* 32bit */ + +/* Miscellaneous Control Register */ +#define MISCCTL 0x3590 /* 32bit */ +/* USB Port Disable Override */ +#define USBPDO 0x359c /* 32bit */ +/* USB Overcurrent MAP Register */ +#define USBOCM1 0x35a0 /* 32bit */ +#define USBOCM2 0x35a4 /* 32bit */ +/* Rate Matching Hub Wake Control Register */ +#define RMHWKCTL 0x35b0 /* 32bit */ + /* ICH7 PMBASE */ #define PM1_STS 0x00 #define WAK_STS (1 << 15)
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35762 )
Change subject: sb/intel/ibexpeak: Implement USB current settings ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35762/1/src/southbridge/intel/ibexp... File src/southbridge/intel/ibexpeak/early_usb.c:
https://review.coreboot.org/c/coreboot/+/35762/1/src/southbridge/intel/ibexp... PS1, Line 44: RCBA32_AND_OR(USBIRB, ~0x617f0, (3 << 17) | (1 << 12) | (1 << 10) | (1 << 8) | (4 << 4)); line over 96 characters
https://review.coreboot.org/c/coreboot/+/35762/1/src/southbridge/intel/ibexp... File src/southbridge/intel/ibexpeak/pch.h:
https://review.coreboot.org/c/coreboot/+/35762/1/src/southbridge/intel/ibexp... PS1, Line 86: { open brace '{' following struct go on the same line
Hello Alexander Couzens, Patrick Rudolph, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35762
to look at the new patch set (#2).
Change subject: sb/intel/ibexpeak: Implement USB current settings ......................................................................
sb/intel/ibexpeak: Implement USB current settings
This is based on the sandybridge settings. The current lookup table comes from the x201 vendor lookup table.
Tested: USB mouse and webcam still work and current registers are the same as before. USB IR are not but the code follows EDS instead of the register replay.
Change-Id: Icea9673623a62e7039d5700100a2ee238478abd1 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/mainboard/lenovo/x201/romstage.c M src/mainboard/packardbell/ms2290/romstage.c M src/southbridge/intel/ibexpeak/Makefile.inc A src/southbridge/intel/ibexpeak/early_usb.c M src/southbridge/intel/ibexpeak/pch.h 5 files changed, 162 insertions(+), 47 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/35762/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35762 )
Change subject: sb/intel/ibexpeak: Implement USB current settings ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35762/2/src/southbridge/intel/ibexp... File src/southbridge/intel/ibexpeak/pch.h:
https://review.coreboot.org/c/coreboot/+/35762/2/src/southbridge/intel/ibexp... PS2, Line 86: { open brace '{' following struct go on the same line
Hello Alexander Couzens, Patrick Rudolph, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35762
to look at the new patch set (#3).
Change subject: sb/intel/ibexpeak: Implement USB current settings ......................................................................
sb/intel/ibexpeak: Implement USB current settings
This is based on the sandybridge settings. The current lookup table comes from the x201 vendor lookup table.
Tested: USB mouse and webcam still work and current registers are the same as before. USB IR are not but the code follows EDS instead of the register replay.
Change-Id: Icea9673623a62e7039d5700100a2ee238478abd1 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/mainboard/lenovo/x201/romstage.c M src/mainboard/packardbell/ms2290/romstage.c M src/southbridge/intel/ibexpeak/Makefile.inc A src/southbridge/intel/ibexpeak/early_usb.c M src/southbridge/intel/ibexpeak/pch.h 5 files changed, 161 insertions(+), 47 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/35762/3
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35762 )
Change subject: sb/intel/ibexpeak: Implement USB current settings ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35762/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35762/3//COMMIT_MSG@14 PS3, Line 14: register replay. Do you know what is different? Shouldn’t we trust the board manufacturer?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35762 )
Change subject: sb/intel/ibexpeak: Implement USB current settings ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35762/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35762/3//COMMIT_MSG@14 PS3, Line 14: register replay.
Do you know what is different? Shouldn’t we trust the board manufacturer?
Who to trust the vendor firmware developer which may be using newer or older information or a leaked Intel EDS? I can add a comment in the code, that EDS is chosen.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35762 )
Change subject: sb/intel/ibexpeak: Implement USB current settings ......................................................................
Patch Set 3: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/35762/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35762/3//COMMIT_MSG@14 PS3, Line 14: register replay.
Do you know what is different? Shouldn’t we trust the board manufacturer? […]
A comment sounds good.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35762 )
Change subject: sb/intel/ibexpeak: Implement USB current settings ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35762/4/src/southbridge/intel/ibexp... File src/southbridge/intel/ibexpeak/early_usb.c:
https://review.coreboot.org/c/coreboot/+/35762/4/src/southbridge/intel/ibexp... PS4, Line 42: TODO maybe vendor fimware values are better? */ 'fimware' may be misspelled - perhaps 'firmware'?
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35762 )
Change subject: sb/intel/ibexpeak: Implement USB current settings ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35762/5/src/southbridge/intel/ibexp... File src/southbridge/intel/ibexpeak/early_usb.c:
https://review.coreboot.org/c/coreboot/+/35762/5/src/southbridge/intel/ibexp... PS5, Line 42: TODO maybe vendor fimware values are better? */ 'fimware' may be misspelled - perhaps 'firmware'?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35762 )
Change subject: sb/intel/ibexpeak: Implement USB current settings ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35762/4/src/southbridge/intel/ibexp... File src/southbridge/intel/ibexpeak/early_usb.c:
https://review.coreboot.org/c/coreboot/+/35762/4/src/southbridge/intel/ibexp... PS4, Line 42: TODO maybe vendor fimware values are better? */
'fimware' may be misspelled - perhaps 'firmware'?
Done
https://review.coreboot.org/c/coreboot/+/35762/5/src/southbridge/intel/ibexp... File src/southbridge/intel/ibexpeak/early_usb.c:
https://review.coreboot.org/c/coreboot/+/35762/5/src/southbridge/intel/ibexp... PS5, Line 42: TODO maybe vendor fimware values are better? */
'fimware' may be misspelled - perhaps 'firmware'?
Done
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35762 )
Change subject: sb/intel/ibexpeak: Implement USB current settings ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35762/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35762/3//COMMIT_MSG@14 PS3, Line 14: register replay.
A comment sounds good.
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35762 )
Change subject: sb/intel/ibexpeak: Implement USB current settings ......................................................................
Patch Set 8: Code-Review+2
(6 comments)
https://review.coreboot.org/c/coreboot/+/35762/8/src/southbridge/intel/ibexp... File src/southbridge/intel/ibexpeak/early_usb.c:
https://review.coreboot.org/c/coreboot/+/35762/8/src/southbridge/intel/ibexp... PS8, Line 29: const u16 currents[] = { 0xf57, 0xf5f, 0x753, 0x75f, : 0x14b, 0x74b, 0x557, 0x757, : 0x55f, 0x54b please use the 96-character line length limit :D
https://review.coreboot.org/c/coreboot/+/35762/8/src/southbridge/intel/ibexp... PS8, Line 38: 14 magic?
https://review.coreboot.org/c/coreboot/+/35762/8/src/southbridge/intel/ibexp... PS8, Line 51: 14 magic!
https://review.coreboot.org/c/coreboot/+/35762/8/src/southbridge/intel/ibexp... PS8, Line 56: 8 more magic!
https://review.coreboot.org/c/coreboot/+/35762/8/src/southbridge/intel/ibexp... PS8, Line 66: Double space
https://review.coreboot.org/c/coreboot/+/35762/8/src/southbridge/intel/ibexp... File src/southbridge/intel/ibexpeak/pch.h:
https://review.coreboot.org/c/coreboot/+/35762/8/src/southbridge/intel/ibexp... PS8, Line 72: current_lookup_idx Any clue as to what these mean?
Hello Alexander Couzens, Patrick Rudolph, Angel Pons, Paul Menzel, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35762
to look at the new patch set (#9).
Change subject: sb/intel/ibexpeak: Implement USB current settings ......................................................................
sb/intel/ibexpeak: Implement USB current settings
This is based on the sandybridge settings. The current lookup table comes from the x201 vendor lookup table.
Tested: USB mouse and webcam still work and current registers are the same as before. USB IR are not but the code follows EDS instead of the register replay.
Change-Id: Icea9673623a62e7039d5700100a2ee238478abd1 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/mainboard/lenovo/x201/romstage.c M src/mainboard/packardbell/ms2290/romstage.c M src/southbridge/intel/ibexpeak/Makefile.inc A src/southbridge/intel/ibexpeak/early_usb.c M src/southbridge/intel/ibexpeak/pch.h 5 files changed, 165 insertions(+), 47 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/35762/9
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35762 )
Change subject: sb/intel/ibexpeak: Implement USB current settings ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35762/9/src/mainboard/packardbell/m... File src/mainboard/packardbell/ms2290/romstage.c:
https://review.coreboot.org/c/coreboot/+/35762/9/src/mainboard/packardbell/m... PS9, Line 90: early_usb_init(mainboard_usb_ports); Pass ARRAY_SIZE() as call argument, test for equals TOTAL_USB_Ports? Same for snb/ivy?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35762 )
Change subject: sb/intel/ibexpeak: Implement USB current settings ......................................................................
Patch Set 10:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35762/9/src/mainboard/packardbell/m... File src/mainboard/packardbell/ms2290/romstage.c:
https://review.coreboot.org/c/coreboot/+/35762/9/src/mainboard/packardbell/m... PS9, Line 90: early_usb_init(mainboard_usb_ports);
Pass ARRAY_SIZE() as call argument, test for equals TOTAL_USB_Ports? Same for snb/ivy?
Later on this array is declared extern with a fixed size and used in the common romstage, which would defeat the purpose of this? Same holds true for snb/ivy. Should the fixed arraysize be dropped? That would also drop the compiletime warning for excess elements.
https://review.coreboot.org/c/coreboot/+/35762/8/src/southbridge/intel/ibexp... File src/southbridge/intel/ibexpeak/early_usb.c:
https://review.coreboot.org/c/coreboot/+/35762/8/src/southbridge/intel/ibexp... PS8, Line 29: const u16 currents[] = { 0xf57, 0xf5f, 0x753, 0x75f, : 0x14b, 0x74b, 0x557, 0x757, : 0x55f, 0x54b
please use the 96-character line length limit :D
Humans count to 4 easily. I temporarily did that to port the RCBA setting to the new format :)
https://review.coreboot.org/c/coreboot/+/35762/8/src/southbridge/intel/ibexp... File src/southbridge/intel/ibexpeak/pch.h:
https://review.coreboot.org/c/coreboot/+/35762/8/src/southbridge/intel/ibexp... PS8, Line 72: current_lookup_idx
Any clue as to what these mean?
They are related to the topology of the USB ports, but without more complete documentation a lookup table with a handy index enum holding the value's name is the best we can do.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35762 )
Change subject: sb/intel/ibexpeak: Implement USB current settings ......................................................................
Patch Set 10: Code-Review+2
(5 comments)
https://review.coreboot.org/c/coreboot/+/35762/9/src/mainboard/packardbell/m... File src/mainboard/packardbell/ms2290/romstage.c:
https://review.coreboot.org/c/coreboot/+/35762/9/src/mainboard/packardbell/m... PS9, Line 90: early_usb_init(mainboard_usb_ports);
Pass ARRAY_SIZE() as call argument, test for equals TOTAL_USB_Ports? Same for snb/ivy? […]
Declaring it with fixed size is already better, although it leaves the possibility of having too few elements. As long as zero-initialised entries don't cause permanent damage, it's fine like your follow-up work does it.
We can revisit this later.
https://review.coreboot.org/c/coreboot/+/35762/8/src/southbridge/intel/ibexp... File src/southbridge/intel/ibexpeak/early_usb.c:
https://review.coreboot.org/c/coreboot/+/35762/8/src/southbridge/intel/ibexp... PS8, Line 38: 14
magic?
Done
https://review.coreboot.org/c/coreboot/+/35762/8/src/southbridge/intel/ibexp... PS8, Line 51: 14
magic!
Done
https://review.coreboot.org/c/coreboot/+/35762/8/src/southbridge/intel/ibexp... PS8, Line 56: 8
more magic!
Done
https://review.coreboot.org/c/coreboot/+/35762/8/src/southbridge/intel/ibexp... PS8, Line 66:
Double space
Done
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35762 )
Change subject: sb/intel/ibexpeak: Implement USB current settings ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35762/8/src/southbridge/intel/ibexp... File src/southbridge/intel/ibexpeak/early_usb.c:
https://review.coreboot.org/c/coreboot/+/35762/8/src/southbridge/intel/ibexp... PS8, Line 29: const u16 currents[] = { 0xf57, 0xf5f, 0x753, 0x75f, : 0x14b, 0x74b, 0x557, 0x757, : 0x55f, 0x54b
please use the 96-character line length limit :D […]
Done
https://review.coreboot.org/c/coreboot/+/35762/8/src/southbridge/intel/ibexp... File src/southbridge/intel/ibexpeak/pch.h:
https://review.coreboot.org/c/coreboot/+/35762/8/src/southbridge/intel/ibexp... PS8, Line 72: current_lookup_idx
Any clue as to what these mean? […]
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35762 )
Change subject: sb/intel/ibexpeak: Implement USB current settings ......................................................................
Patch Set 10: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/35762/8/src/southbridge/intel/ibexp... File src/southbridge/intel/ibexpeak/early_usb.c:
https://review.coreboot.org/c/coreboot/+/35762/8/src/southbridge/intel/ibexp... PS8, Line 29: const u16 currents[] = { 0xf57, 0xf5f, 0x753, 0x75f, : 0x14b, 0x74b, 0x557, 0x757, : 0x55f, 0x54b
Done
Ah, I see. It's just that my OCD evaluates such an uneven distribution of values to be "hurtfully wrong" :P
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
build bot (Jenkins) (Code Review)
-
Kyösti Mälkki (Code Review)
-
Paul Menzel (Code Review)