Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42134 )
Change subject: src/include: Add PnP/HWM update functions ......................................................................
src/include: Add PnP/HWM update functions
RMW (read/modify/write) ops on PnP devices has never been so simple.
Change-Id: Ica01211af2a9a00aed98880844a836f6b7957b14 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/include/device/pnp.h M src/include/device/pnp_ops.h M src/include/superio/hwm5_conf.h 3 files changed, 51 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/42134/1
diff --git a/src/include/device/pnp.h b/src/include/device/pnp.h index 800bcc0..158d40e 100644 --- a/src/include/device/pnp.h +++ b/src/include/device/pnp.h @@ -133,4 +133,29 @@ outb(value, port + 1); }
+/* + * void pnp_update_index(u16 port, u8 reg, u8 mask, u8 or) + * Description: + * This routine updates indexed I/O registers. The reg byte is written + * to the index register at I/O address = port. The value is then read + * from the data register at I/O address = port + 1. This value is ANDed + * with the mask value and then ORed with the or value. Finally, the + * new value is written to the data register at I/O address = port + 1. + * + * Parameters: + * @param[in] u16 port = The address of the port index register. + * @param[in] u8 reg = The offset within the indexed space. + * @param[in] u8 mask = The mask value to apply to the data register value. + * @param[in] u8 or = The or value to apply to the data register value. + */ +static inline void pnp_update_index(u16 port, u8 reg, u8 mask, u8 or) +{ + outb(reg, port); + + uint8_t value = inb(port + 1); + value &= mask; + value |= or; + outb(value, port + 1); +} + #endif /* DEVICE_PNP_H */ diff --git a/src/include/device/pnp_ops.h b/src/include/device/pnp_ops.h index 0cfdd61..783c3ad 100644 --- a/src/include/device/pnp_ops.h +++ b/src/include/device/pnp_ops.h @@ -22,6 +22,12 @@ return pnp_read_index(dev >> 8, reg); }
+static __always_inline void pnp_update_config( + pnp_devfn_t dev, uint8_t reg, uint8_t mask, uint8_t or) +{ + pnp_update_index(dev >> 8, reg, mask, or); +} + static __always_inline void pnp_set_logical_device(pnp_devfn_t dev) { diff --git a/src/include/superio/hwm5_conf.h b/src/include/superio/hwm5_conf.h index 661f3ee..5e9a142 100644 --- a/src/include/superio/hwm5_conf.h +++ b/src/include/superio/hwm5_conf.h @@ -44,4 +44,24 @@ pnp_write_index(base + 5, reg, value); }
+/* + * void pnp_update_hwm5_index(u16 base, u8 reg, u8 mask, u8 or) + * Description: + * This routine updates indexed I/O registers. The reg byte is written + * to the index register at I/O address = port + 5. The value is then read + * from the data register at I/O address = port + 6. This value is ANDed + * with the mask value and then ORed with the or value. Finally, the + * new value is written to the data register at I/O address = port + 6. + * + * Parameters: + * @param[in] u16 port = The address of the port index register. + * @param[in] u8 reg = The offset within the indexed space. + * @param[in] u8 mask = The mask value to apply to the data register value. + * @param[in] u8 or = The or value to apply to the data register value. + */ +static inline void pnp_update_hwm5_index(u16 base, u8 reg, u8 mask, u8 or) +{ + pnp_update_index(base + 5, reg, mask, or); +} + #endif /* DEVICE_PNP_HWM5_CONF_H */
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42134 )
Change subject: src/include: Add PnP/HWM update functions ......................................................................
Patch Set 1: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42134 )
Change subject: src/include: Add PnP/HWM update functions ......................................................................
Patch Set 1:
This change is ready for review.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42134 )
Change subject: src/include: Add PnP/HWM update functions ......................................................................
Patch Set 2: Code-Review-2
I have a better idea...
Hello build bot (Jenkins), HAOUAS Elyes,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42134
to look at the new patch set (#3).
Change subject: src/include: Add PnP/HWM unset_and_set functions ......................................................................
src/include: Add PnP/HWM unset_and_set functions
RMW (read/modify/write) ops on PnP devices has never been so simple.
Change-Id: Ica01211af2a9a00aed98880844a836f6b7957b14 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/include/device/pnp.h M src/include/device/pnp_ops.h M src/include/superio/hwm5_conf.h 3 files changed, 59 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/42134/3
Angel Pons has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/42134 )
Change subject: src/include: Add PnP/HWM unset_and_set functions ......................................................................
Removed Code-Review-2 by Angel Pons th3fanbus@gmail.com
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42134 )
Change subject: src/include: Add PnP/HWM unset_and_set functions ......................................................................
Patch Set 8:
(2 comments)
Please start a discussion about the naming convention on the mailing list. We already have `clrsetbits` and `pci_update_config` (with different semantics, I know). It doesn't feel right to add a third nomenclature during a review that is less visible to most of the community.
https://review.coreboot.org/c/coreboot/+/42134/8/src/include/device/pnp.h File src/include/device/pnp.h:
https://review.coreboot.org/c/coreboot/+/42134/8/src/include/device/pnp.h@98 PS8, Line 98: /* PNP indexed I/O operations */ Not your fault, this code is in the wrong place. I see nothing PnP specific below.
https://review.coreboot.org/c/coreboot/+/42134/8/src/include/device/pnp.h@14... PS8, Line 147: * Performing said negation inside this routine alleviates this problem. It would also work if the negation and cast would be done outside the function. The actual benefit, IMHO, is the implicit conversion to `u8` of the parameter. This way, we can still have overflow warnings (e.g. try to call this with 257) where it matters but can silence them where not (after the negation).
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42134 )
Change subject: src/include: Add PnP/HWM unset_and_set functions ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42134/8/src/include/device/pnp.h File src/include/device/pnp.h:
https://review.coreboot.org/c/coreboot/+/42134/8/src/include/device/pnp.h@14... PS8, Line 147: * Performing said negation inside this routine alleviates this problem.
It would also work if the negation and cast would be done outside the […]
Done
Hello build bot (Jenkins), Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42134
to look at the new patch set (#11).
Change subject: src/include: Add PnP/HWM unset_and_set functions ......................................................................
src/include: Add PnP/HWM unset_and_set functions
RMW (read/modify/write) ops on PnP devices has never been so simple.
The semantics also allow the compiler to emit valid warnings if the input parameters would overflow, which are silenced when the cast is placed outside of the function.
Change-Id: Ica01211af2a9a00aed98880844a836f6b7957b14 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/include/device/pnp.h M src/include/device/pnp_ops.h M src/include/superio/hwm5_conf.h 3 files changed, 67 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/42134/11
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42134 )
Change subject: src/include: Add PnP/HWM unset_and_set functions ......................................................................
Patch Set 11: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42134 )
Change subject: src/include: Add PnP/HWM unset_and_set functions ......................................................................
Patch Set 11:
Patch Set 8:
(2 comments)
Please start a discussion about the naming convention on the mailing list. We already have `clrsetbits` and `pci_update_config` (with different semantics, I know). It doesn't feel right to add a third nomenclature during a review that is less visible to most of the community.
https://mail.coreboot.org/hyperkitty/list/coreboot@coreboot.org/thread/BULIH...
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42134 )
Change subject: src/include: Add PnP/HWM unset_and_set functions ......................................................................
Patch Set 11: Code-Review+1
I like this and it does solve an existing problem. I'd drop the and between unset and set, since the _and adds 4 chars, but not much information. unset_set does sound a bit weird though. only giving a +1 for now to see if someone else might come up with better idea; if not I'd +2 this; probably either with or without the _and dropped
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42134 )
Change subject: src/include: Add PnP/HWM unset_and_set functions ......................................................................
Patch Set 12: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/42134/12/src/include/superio/hwm5_c... File src/include/superio/hwm5_conf.h:
https://review.coreboot.org/c/coreboot/+/42134/12/src/include/superio/hwm5_c... PS12, Line 52: port base
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42134 )
Change subject: src/include: Add PnP/HWM unset_and_set functions ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42134/12/src/include/superio/hwm5_c... File src/include/superio/hwm5_conf.h:
https://review.coreboot.org/c/coreboot/+/42134/12/src/include/superio/hwm5_c... PS12, Line 52: port
base
Uh, the comments above are wrong then
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42134 )
Change subject: src/include: Add PnP/HWM unset_and_set functions ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42134/12/src/include/superio/hwm5_c... File src/include/superio/hwm5_conf.h:
https://review.coreboot.org/c/coreboot/+/42134/12/src/include/superio/hwm5_c... PS12, Line 52: port
Uh, the comments above are wrong then
yep, @param[in] u16 base isn't the address of the index register, but the base. the index register is base + 5.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42134 )
Change subject: src/include: Add PnP/HWM unset_and_set functions ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42134/12/src/include/superio/hwm5_c... File src/include/superio/hwm5_conf.h:
https://review.coreboot.org/c/coreboot/+/42134/12/src/include/superio/hwm5_c... PS12, Line 52: port
yep, @param[in] u16 base isn't the address of the index register, but the base. […]
What I mean is that the comments above say "port" instead of "base", too
I'll make a patch to fix those, and then add this change on top
Hello build bot (Jenkins), Nico Huber, Patrick Rudolph, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42134
to look at the new patch set (#13).
Change subject: src/include: Add PnP/HWM unset_and_set functions ......................................................................
src/include: Add PnP/HWM unset_and_set functions
RMW (read/modify/write) ops on PnP devices has never been so simple.
The semantics also allow the compiler to emit valid warnings if the input parameters would overflow, which are silenced when the cast is placed outside of the function.
Change-Id: Ica01211af2a9a00aed98880844a836f6b7957b14 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/include/device/pnp.h M src/include/device/pnp_ops.h M src/include/superio/hwm5_conf.h 3 files changed, 67 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/42134/13
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42134 )
Change subject: src/include: Add PnP/HWM unset_and_set functions ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42134/12/src/include/superio/hwm5_c... File src/include/superio/hwm5_conf.h:
https://review.coreboot.org/c/coreboot/+/42134/12/src/include/superio/hwm5_c... PS12, Line 52: port
What I mean is that the comments above say "port" instead of "base", too […]
Done
Hello build bot (Jenkins), Nico Huber, Patrick Rudolph, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42134
to look at the new patch set (#14).
Change subject: src/include: Add PnP/HWM unset_and_set functions ......................................................................
src/include: Add PnP/HWM unset_and_set functions
RMW (read/modify/write) ops on PnP devices has never been so simple.
The semantics also allow the compiler to emit valid warnings if the input parameters would overflow, which are silenced when the cast is placed outside of the function.
Change-Id: Ica01211af2a9a00aed98880844a836f6b7957b14 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/include/device/pnp.h M src/include/device/pnp_ops.h M src/include/superio/hwm5_conf.h 3 files changed, 67 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/42134/14
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42134 )
Change subject: src/include: Add PnP/HWM unset_and_set functions ......................................................................
Patch Set 14: Code-Review+1
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42134 )
Change subject: src/include: Add PnP/HWM unset_and_set functions ......................................................................
Patch Set 15: Code-Review+2
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42134 )
Change subject: src/include: Add PnP/HWM unset_and_set functions ......................................................................
src/include: Add PnP/HWM unset_and_set functions
RMW (read/modify/write) ops on PnP devices has never been so simple.
The semantics also allow the compiler to emit valid warnings if the input parameters would overflow, which are silenced when the cast is placed outside of the function.
Change-Id: Ica01211af2a9a00aed98880844a836f6b7957b14 Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/42134 Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Felix Held felix-coreboot@felixheld.de Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/include/device/pnp.h M src/include/device/pnp_ops.h M src/include/superio/hwm5_conf.h 3 files changed, 67 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Felix Held: Looks good to me, but someone else must approve
diff --git a/src/include/device/pnp.h b/src/include/device/pnp.h index cf809d0..635876b 100644 --- a/src/include/device/pnp.h +++ b/src/include/device/pnp.h @@ -133,4 +133,37 @@ outb(value, port + 1); }
+/* + * void pnp_unset_and_set_index(u16 port, u8 reg, u8 unset, u8 set) + * Description: + * This routine unsets and sets bits from indexed I/O registers. The + * reg byte is written to the index register at I/O address = port. + * The value byte to update is data register at I/O address = port + 1. + * + * Unlike and-then-or style operations, no bitwise negation is necessary + * to specify the bits to unset. Because the bitwise negation implicitly + * promotes operands to int before operating, one may have to explicitly + * downcast the result if the data width is smaller than that of an int. + * Since warnings are errors in coreboot, explicit casting is necessary. + * + * Performing said negation inside this routine alleviates this problem, + * while allowing the compiler to warn if the input parameters overflow. + * Casting outside this function would silence valid compiler warnings. + * + * Parameters: + * @param[in] u16 port = The address of the port index register. + * @param[in] u8 reg = The offset within the indexed space. + * @param[in] u8 unset = Bitmask with ones to the bits to unset from the data register. + * @param[in] u8 set = Bitmask with ones to the bits to set from the data register. + */ +static inline void pnp_unset_and_set_index(u16 port, u8 reg, u8 unset, u8 set) +{ + outb(reg, port); + + u8 value = inb(port + 1); + value &= (u8)~unset; + value |= set; + outb(value, port + 1); +} + #endif /* DEVICE_PNP_H */ diff --git a/src/include/device/pnp_ops.h b/src/include/device/pnp_ops.h index 15acf36..15d3115 100644 --- a/src/include/device/pnp_ops.h +++ b/src/include/device/pnp_ops.h @@ -21,6 +21,12 @@ return pnp_read_index(dev >> 8, reg); }
+static __always_inline void pnp_unset_and_set_config( + pnp_devfn_t dev, uint8_t reg, uint8_t unset, uint8_t set) +{ + pnp_unset_and_set_index(dev >> 8, reg, unset, set); +} + static __always_inline void pnp_set_logical_device(pnp_devfn_t dev) { diff --git a/src/include/superio/hwm5_conf.h b/src/include/superio/hwm5_conf.h index 2cf13c6..f26a017 100644 --- a/src/include/superio/hwm5_conf.h +++ b/src/include/superio/hwm5_conf.h @@ -45,4 +45,32 @@ pnp_write_index(base + 5, reg, value); }
+/* + * void pnp_unset_and_set_hwm5_index(u16 base, u8 reg, u8 unset, u8 set) + * Description: + * This routine unsets and sets bits from indexed I/O registers. The + * reg byte is written to the index register at I/O address = base + 5. + * The value byte to update is data register at I/O address = base + 6. + * + * Unlike and-then-or style operations, no bitwise negation is necessary + * to specify the bits to unset. Because the bitwise negation implicitly + * promotes operands to int before operating, one may have to explicitly + * downcast the result if the data width is smaller than that of an int. + * Since warnings are errors in coreboot, explicit casting is necessary. + * + * Performing said negation inside this routine alleviates this problem, + * while allowing the compiler to warn if the input parameters overflow. + * Casting outside this function would silence valid compiler warnings. + * + * Parameters: + * @param[in] u16 base = The address of the base index register. + * @param[in] u8 reg = The offset within the indexed space. + * @param[in] u8 unset = Bitmask with ones to the bits to unset from the data register. + * @param[in] u8 set = Bitmask with ones to the bits to set from the data register. + */ +static inline void pnp_unset_and_set_hwm5_index(u16 base, u8 reg, u8 unset, u8 set) +{ + pnp_unset_and_set_index(base + 5, reg, unset, set); +} + #endif /* DEVICE_PNP_HWM5_CONF_H */
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42134 )
Change subject: src/include: Add PnP/HWM unset_and_set functions ......................................................................
Patch Set 16:
Automatic boot test returned (PASS/FAIL/TOTAL): 8/1/9 "QEMU x86 q35/ich9" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/19936 "QEMU x86 q35/ich9" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19935 "QEMU x86 i440fx/piix4" (x86_64) using payload SeaBIOS : FAIL : https://lava.9esec.io/r/19934 "QEMU x86 i440fx/piix4" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19933 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/19932 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19940 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19939 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/19938 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19937
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Angel Pons (Code Review)
-
Felix Held (Code Review)
-
HAOUAS Elyes (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)