Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38179 )
Change subject: drivers/pc80/rtc: Clean up some inlined functions ......................................................................
drivers/pc80/rtc: Clean up some inlined functions
Change-Id: Ie73797b4e9a09605a0685f0b03cb85e9a3be93ad Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/drivers/pc80/rtc/mc146818rtc.c M src/drivers/pc80/rtc/mc146818rtc_boot.c M src/include/pc80/mc146818rtc.h 3 files changed, 42 insertions(+), 39 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/38179/1
diff --git a/src/drivers/pc80/rtc/mc146818rtc.c b/src/drivers/pc80/rtc/mc146818rtc.c index 2029b1e..f151910 100644 --- a/src/drivers/pc80/rtc/mc146818rtc.c +++ b/src/drivers/pc80/rtc/mc146818rtc.c @@ -81,6 +81,14 @@ cmos_write(((sum >> 0) & 0x0ff), cks_loc + 1); }
+int cmos_error(void) +{ + unsigned char reg_d; + /* See if the cmos error condition has been flagged */ + reg_d = cmos_read(RTC_REG_D); + return (reg_d & RTC_VRT) == 0; +} + #define RTC_CONTROL_DEFAULT (RTC_24H) #define RTC_FREQ_SELECT_DEFAULT (RTC_REF_CLCK_32KHZ | RTC_RATE_1024HZ)
@@ -406,6 +414,16 @@ return CB_SUCCESS; }
+/* Upon return the caller is guaranteed 244 microseconds to complete any + * RTC operations. wait_uip may be called a single time prior to multiple + * accesses, but sequences requiring more time should call wait_uip again. + */ +static void wait_uip(void) +{ + while (cmos_read(RTC_REG_A) & RTC_UIP) + ; +} + /* * If the CMOS is cleared, the rtc_reg has the invalid date. That * hurts some OSes. Even if we don't set USE_OPTION_TABLE, we need diff --git a/src/drivers/pc80/rtc/mc146818rtc_boot.c b/src/drivers/pc80/rtc/mc146818rtc_boot.c index 3000946..c2ab707 100644 --- a/src/drivers/pc80/rtc/mc146818rtc_boot.c +++ b/src/drivers/pc80/rtc/mc146818rtc_boot.c @@ -19,14 +19,6 @@ #include <option_table.h> #endif
-int cmos_error(void) -{ - unsigned char reg_d; - /* See if the cmos error condition has been flagged */ - reg_d = cmos_read(RTC_REG_D); - return (reg_d & RTC_VRT) == 0; -} - int cmos_chksum_valid(void) { #if CONFIG(USE_OPTION_TABLE) @@ -72,22 +64,22 @@ #error "CONFIG_MAX_REBOOT_CNT too high" #endif
-static inline int boot_count(uint8_t rtc_byte) +static int boot_count(uint8_t rtc_byte) { return rtc_byte >> 4; }
-static inline uint8_t increment_boot_count(uint8_t rtc_byte) +static uint8_t increment_boot_count(uint8_t rtc_byte) { return rtc_byte + (1 << 4); }
-static inline uint8_t boot_set_fallback(uint8_t rtc_byte) +static uint8_t boot_set_fallback(uint8_t rtc_byte) { return rtc_byte & ~RTC_BOOT_NORMAL; }
-static inline int boot_use_normal(uint8_t rtc_byte) +static int boot_use_normal(uint8_t rtc_byte) { return rtc_byte & RTC_BOOT_NORMAL; } diff --git a/src/include/pc80/mc146818rtc.h b/src/include/pc80/mc146818rtc.h index a8221c7..aadfae22 100644 --- a/src/include/pc80/mc146818rtc.h +++ b/src/include/pc80/mc146818rtc.h @@ -114,16 +114,6 @@ return inb(RTC_BASE_PORT + offs + 1); }
-/* Upon return the caller is guaranteed 244 microseconds to complete any - * RTC operations. wait_uip may be called a single time prior to multiple - * accesses, but sequences requiring more time should call wait_uip again. - */ -static inline void wait_uip(void) -{ - while (cmos_read(RTC_REG_A) & RTC_UIP) - ; -} - static inline void cmos_write_inner(unsigned char val, unsigned char addr) { int offs = 0; @@ -135,31 +125,34 @@ outb(val, RTC_BASE_PORT + offs + 1); }
-static inline void cmos_write(unsigned char val, unsigned char addr) -{ - u8 control_state = cmos_read(RTC_CONTROL); - /* There are various places where RTC bits might be hiding, - * eg. the Century / AltCentury byte. So to be safe, disable - * RTC before changing any value. - */ - if ((addr != RTC_CONTROL) && !(control_state & RTC_SET)) - cmos_write_inner(control_state | RTC_SET, RTC_CONTROL); - cmos_write_inner(val, addr); - /* reset to prior configuration */ - if ((addr != RTC_CONTROL) && !(control_state & RTC_SET)) - cmos_write_inner(control_state, RTC_CONTROL); -} - static inline void cmos_disable_rtc(void) { u8 control_state = cmos_read(RTC_CONTROL); - cmos_write(control_state | RTC_SET, RTC_CONTROL); + if (!(control_state & RTC_SET)) + cmos_write_inner(control_state | RTC_SET, RTC_CONTROL); }
static inline void cmos_enable_rtc(void) { u8 control_state = cmos_read(RTC_CONTROL); - cmos_write(control_state & ~RTC_SET, RTC_CONTROL); + if (control_state & RTC_SET) + cmos_write_inner(control_state & ~RTC_SET, RTC_CONTROL); +} + +static inline void cmos_write(unsigned char val, unsigned char addr) +{ + /* There are various places where RTC bits might be hiding, + * eg. the Century / AltCentury byte. So to be safe, disable + * RTC before changing any value. + */ + if (addr != RTC_CONTROL) + cmos_disable_rtc(); + + cmos_write_inner(val, addr); + + /* reset to prior configuration */ + if (addr != RTC_CONTROL) + cmos_enable_rtc(); }
static inline u32 cmos_read32(u8 offset)
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38179 )
Change subject: drivers/pc80/rtc: Clean up some inlined functions ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38179/1/src/include/pc80/mc146818rt... File src/include/pc80/mc146818rtc.h:
https://review.coreboot.org/c/coreboot/+/38179/1/src/include/pc80/mc146818rt... PS1, Line 138: if (control_state & RTC_SET) wasn't obvious on the first sight
https://review.coreboot.org/c/coreboot/+/38179/1/src/include/pc80/mc146818rt... PS1, Line 155: cmos_enable_rtc(); should only be enabled if it was running before cmos_disable_rtc was called()
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38179 )
Change subject: drivers/pc80/rtc: Clean up some inlined functions ......................................................................
Patch Set 1:
(3 comments)
Thank you for the clean-up.
https://review.coreboot.org/c/coreboot/+/38179/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38179/1//COMMIT_MSG@8 PS1, Line 8: Could you please document, why you added the checks?
if (control_state & RTC_SET)
https://review.coreboot.org/c/coreboot/+/38179/1/src/drivers/pc80/rtc/mc1468... File src/drivers/pc80/rtc/mc146818rtc.c:
https://review.coreboot.org/c/coreboot/+/38179/1/src/drivers/pc80/rtc/mc1468... PS1, Line 87: cmos CMOS
https://review.coreboot.org/c/coreboot/+/38179/1/src/drivers/pc80/rtc/mc1468... PS1, Line 417: /* Upon return the caller is guaranteed 244 microseconds to complete any Please use
/* * Upon …
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38179 )
Change subject: drivers/pc80/rtc: Clean up some inlined functions ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38179/1/src/include/pc80/mc146818rt... File src/include/pc80/mc146818rtc.h:
https://review.coreboot.org/c/coreboot/+/38179/1/src/include/pc80/mc146818rt... PS1, Line 155: cmos_enable_rtc();
should only be enabled if it was running before cmos_disable_rtc was called()
Right.. But wouldn't the requirement to stop RTC only apply to the RTC register bank of [0..0x0e] and not the nvram section?
Hello build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38179
to look at the new patch set (#2).
Change subject: drivers/pc80/rtc: Clean up some inlined functions ......................................................................
drivers/pc80/rtc: Clean up some inlined functions
Change-Id: Ie73797b4e9a09605a0685f0b03cb85e9a3be93ad Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/drivers/pc80/rtc/mc146818rtc.c M src/drivers/pc80/rtc/mc146818rtc_boot.c M src/include/pc80/mc146818rtc.h 3 files changed, 47 insertions(+), 45 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/38179/2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38179 )
Change subject: drivers/pc80/rtc: Clean up some inlined functions ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38179/1/src/drivers/pc80/rtc/mc1468... File src/drivers/pc80/rtc/mc146818rtc.c:
https://review.coreboot.org/c/coreboot/+/38179/1/src/drivers/pc80/rtc/mc1468... PS1, Line 87: cmos
CMOS
Done
https://review.coreboot.org/c/coreboot/+/38179/1/src/drivers/pc80/rtc/mc1468... PS1, Line 417: /* Upon return the caller is guaranteed 244 microseconds to complete any
Please use […]
Done
https://review.coreboot.org/c/coreboot/+/38179/1/src/include/pc80/mc146818rt... File src/include/pc80/mc146818rtc.h:
https://review.coreboot.org/c/coreboot/+/38179/1/src/include/pc80/mc146818rt... PS1, Line 138: if (control_state & RTC_SET)
wasn't obvious on the first sight
I have redone this.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38179 )
Change subject: drivers/pc80/rtc: Clean up some inlined functions ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/38179/2/src/drivers/pc80/rtc/mc1468... File src/drivers/pc80/rtc/mc146818rtc.c:
https://review.coreboot.org/c/coreboot/+/38179/2/src/drivers/pc80/rtc/mc1468... PS2, Line 86: reg_d Is this variable needed?
Hello Angel Pons, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38179
to look at the new patch set (#3).
Change subject: drivers/pc80/rtc: Clean up some inlined functions ......................................................................
drivers/pc80/rtc: Clean up some inlined functions
Change-Id: Ie73797b4e9a09605a0685f0b03cb85e9a3be93ad Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/drivers/pc80/rtc/mc146818rtc.c M src/drivers/pc80/rtc/mc146818rtc_boot.c M src/include/pc80/mc146818rtc.h 3 files changed, 45 insertions(+), 45 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/38179/3
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38179 )
Change subject: drivers/pc80/rtc: Clean up some inlined functions ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/38179/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38179/1//COMMIT_MSG@8 PS1, Line 8:
Could you please document, why you added the checks? […]
Ack
https://review.coreboot.org/c/coreboot/+/38179/2/src/drivers/pc80/rtc/mc1468... File src/drivers/pc80/rtc/mc146818rtc.c:
https://review.coreboot.org/c/coreboot/+/38179/2/src/drivers/pc80/rtc/mc1468... PS2, Line 86: reg_d
Is this variable needed?
Done
https://review.coreboot.org/c/coreboot/+/38179/1/src/include/pc80/mc146818rt... File src/include/pc80/mc146818rtc.h:
https://review.coreboot.org/c/coreboot/+/38179/1/src/include/pc80/mc146818rt... PS1, Line 138: if (control_state & RTC_SET)
I have redone this.
Done
https://review.coreboot.org/c/coreboot/+/38179/1/src/include/pc80/mc146818rt... PS1, Line 155: cmos_enable_rtc();
Right.. But wouldn't the requirement to stop RTC only apply to the RTC register bank of [0.. […]
Ah.. there is AltCentury at 0x32 so let's just keep the disable-logic.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38179 )
Change subject: drivers/pc80/rtc: Clean up some inlined functions ......................................................................
Patch Set 3: Code-Review+2
Kyösti Mälkki has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38179 )
Change subject: drivers/pc80/rtc: Clean up some inlined functions ......................................................................
drivers/pc80/rtc: Clean up some inlined functions
Change-Id: Ie73797b4e9a09605a0685f0b03cb85e9a3be93ad Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/38179 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/drivers/pc80/rtc/mc146818rtc.c M src/drivers/pc80/rtc/mc146818rtc_boot.c M src/include/pc80/mc146818rtc.h 3 files changed, 45 insertions(+), 45 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/src/drivers/pc80/rtc/mc146818rtc.c b/src/drivers/pc80/rtc/mc146818rtc.c index 2029b1e..1d2fc97 100644 --- a/src/drivers/pc80/rtc/mc146818rtc.c +++ b/src/drivers/pc80/rtc/mc146818rtc.c @@ -81,6 +81,12 @@ cmos_write(((sum >> 0) & 0x0ff), cks_loc + 1); }
+/* See if the CMOS error condition has been flagged */ +int cmos_error(void) +{ + return (cmos_read(RTC_VALID) & RTC_VRT) == 0; +} + #define RTC_CONTROL_DEFAULT (RTC_24H) #define RTC_FREQ_SELECT_DEFAULT (RTC_REF_CLCK_32KHZ | RTC_RATE_1024HZ)
@@ -90,7 +96,6 @@ bool checksum_invalid = false; bool clear_cmos; size_t i; - uint8_t x;
/* * Avoid clearing pending interrupts and resetting the RTC control @@ -104,8 +109,7 @@ printk(BIOS_DEBUG, "RTC Init\n");
/* See if there has been a CMOS power problem. */ - x = cmos_read(RTC_VALID); - cmos_invalid = !(x & RTC_VRT); + cmos_invalid = cmos_error();
if (CONFIG(USE_OPTION_TABLE)) { /* See if there is a CMOS checksum error */ @@ -118,7 +122,7 @@ }
if (cmos_invalid || invalid) - cmos_write(cmos_read(RTC_CONTROL) | RTC_SET, RTC_CONTROL); + cmos_disable_rtc();
if (invalid || cmos_invalid || checksum_invalid) { if (clear_cmos) { @@ -407,6 +411,17 @@ }
/* + * Upon return the caller is guaranteed 244 microseconds to complete any + * RTC operations. wait_uip may be called a single time prior to multiple + * accesses, but sequences requiring more time should call wait_uip again. + */ +static void wait_uip(void) +{ + while (cmos_read(RTC_REG_A) & RTC_UIP) + ; +} + +/* * If the CMOS is cleared, the rtc_reg has the invalid date. That * hurts some OSes. Even if we don't set USE_OPTION_TABLE, we need * to make sure the date is valid. diff --git a/src/drivers/pc80/rtc/mc146818rtc_boot.c b/src/drivers/pc80/rtc/mc146818rtc_boot.c index 3000946..a52e222 100644 --- a/src/drivers/pc80/rtc/mc146818rtc_boot.c +++ b/src/drivers/pc80/rtc/mc146818rtc_boot.c @@ -19,14 +19,6 @@ #include <option_table.h> #endif
-int cmos_error(void) -{ - unsigned char reg_d; - /* See if the cmos error condition has been flagged */ - reg_d = cmos_read(RTC_REG_D); - return (reg_d & RTC_VRT) == 0; -} - int cmos_chksum_valid(void) { #if CONFIG(USE_OPTION_TABLE) @@ -59,10 +51,10 @@ CBFS_COMPONENT_CMOS_DEFAULT, &length); if (cmos_default) { size_t i; - cmos_disable_rtc(); + u8 control_state = cmos_disable_rtc(); for (i = 14; i < MIN(128, length); i++) cmos_write_inner(cmos_default[i], i); - cmos_enable_rtc(); + cmos_restore_rtc(control_state); } } } @@ -72,22 +64,22 @@ #error "CONFIG_MAX_REBOOT_CNT too high" #endif
-static inline int boot_count(uint8_t rtc_byte) +static int boot_count(uint8_t rtc_byte) { return rtc_byte >> 4; }
-static inline uint8_t increment_boot_count(uint8_t rtc_byte) +static uint8_t increment_boot_count(uint8_t rtc_byte) { return rtc_byte + (1 << 4); }
-static inline uint8_t boot_set_fallback(uint8_t rtc_byte) +static uint8_t boot_set_fallback(uint8_t rtc_byte) { return rtc_byte & ~RTC_BOOT_NORMAL; }
-static inline int boot_use_normal(uint8_t rtc_byte) +static int boot_use_normal(uint8_t rtc_byte) { return rtc_byte & RTC_BOOT_NORMAL; } diff --git a/src/include/pc80/mc146818rtc.h b/src/include/pc80/mc146818rtc.h index a8221c7..e2f4494 100644 --- a/src/include/pc80/mc146818rtc.h +++ b/src/include/pc80/mc146818rtc.h @@ -114,16 +114,6 @@ return inb(RTC_BASE_PORT + offs + 1); }
-/* Upon return the caller is guaranteed 244 microseconds to complete any - * RTC operations. wait_uip may be called a single time prior to multiple - * accesses, but sequences requiring more time should call wait_uip again. - */ -static inline void wait_uip(void) -{ - while (cmos_read(RTC_REG_A) & RTC_UIP) - ; -} - static inline void cmos_write_inner(unsigned char val, unsigned char addr) { int offs = 0; @@ -135,31 +125,34 @@ outb(val, RTC_BASE_PORT + offs + 1); }
-static inline void cmos_write(unsigned char val, unsigned char addr) +static inline u8 cmos_disable_rtc(void) { u8 control_state = cmos_read(RTC_CONTROL); - /* There are various places where RTC bits might be hiding, - * eg. the Century / AltCentury byte. So to be safe, disable - * RTC before changing any value. - */ - if ((addr != RTC_CONTROL) && !(control_state & RTC_SET)) + if (!(control_state & RTC_SET)) cmos_write_inner(control_state | RTC_SET, RTC_CONTROL); - cmos_write_inner(val, addr); - /* reset to prior configuration */ - if ((addr != RTC_CONTROL) && !(control_state & RTC_SET)) + return control_state; +} + +static inline void cmos_restore_rtc(u8 control_state) +{ + if (!(control_state & RTC_SET)) cmos_write_inner(control_state, RTC_CONTROL); }
-static inline void cmos_disable_rtc(void) +static inline void cmos_write(unsigned char val, unsigned char addr) { - u8 control_state = cmos_read(RTC_CONTROL); - cmos_write(control_state | RTC_SET, RTC_CONTROL); -} + u8 control_state;
-static inline void cmos_enable_rtc(void) -{ - u8 control_state = cmos_read(RTC_CONTROL); - cmos_write(control_state & ~RTC_SET, RTC_CONTROL); + /* + * There are various places where RTC bits might be hiding, + * eg. the Century / AltCentury byte. So to be safe, disable + * RTC before changing any value. + */ + if (addr != RTC_CONTROL) + control_state = cmos_disable_rtc(); + cmos_write_inner(val, addr); + if (addr != RTC_CONTROL) + cmos_restore_rtc(control_state); }
static inline u32 cmos_read32(u8 offset)