Skip to content
This repository has been archived by the owner on Nov 1, 2023. It is now read-only.

Server: System Date/Time: Improve explanation for GetTime() #58

Closed
afaerber opened this issue Sep 30, 2021 · 7 comments
Closed

Server: System Date/Time: Improve explanation for GetTime() #58

afaerber opened this issue Sep 30, 2021 · 7 comments

Comments

@afaerber
Copy link
Contributor

In section 2.2.7.2 of the Server profile, "System Date/Time", the explanation for GetTime() sounds awkward:

"Must be implemented by firmware" is clear, but what is "to incorporate with the underlying system date/time mechanism" supposed to mean?

Not being a native speaker, I believe correct verb usage is to incorporate sth., not to incorporate with sth.

I am guessing what it really means is to expose the underlying system date/time mechanism?

@changab
Copy link

changab commented Sep 30, 2021

How about that in below? That actually means FW provides the GetTime implementation that works with the implementation-specific system date/time mechanism.

"Must be implemented by firmware and incorporated with the underlying system date/time mechanism."

@afaerber
Copy link
Contributor Author

afaerber commented Oct 1, 2021

No, changing "to incorporate with" to "and incorporated with" does not address my comment that that "with" may be grammatically wrong (or at least highly unusual). Nor does it make the sentence any more understandable to the reader.

I'm guessing what it intends to say is that GetTime() shall read the time from a chip-/system-specific RTC peripheral and return it. Neither of the two sentence variants makes that clear though. Even if we had a better verb, like I suggested, saying that GetTime() shall implement/expose/whatever "the underlying system date/time mechanism" is kind of non-explaining to the reader.

SetTime() below does not have such weird language and just says "must be implemented".

So why not just have GetTime() say "Must be implemented and is not allowed to return EFI_UNSUPPORTED."? That would avoid both the incorporation wording and the system mechanism wording and would leave the how-to to generic UEFI specs.

@changab
Copy link

changab commented Oct 1, 2021

m guessing what it intends to say is that GetTime() shall read the time from a chip-/system-specific RTC peripheral and return it. Neither of the two sentence variants makes that clear though. Even if we had a better verb, like I suggested, saying that GetTime() shall implement/expose/whatever "the underlying system date/time mechanism" is kind of non-explaining to the reader.

The initial sentence was mentioned RTC as the underlying system date/time implementation. But we removed it to avoid the dependency of the specific hardware implementation. for Get/SetTime(), instead the current sentence is used.

@changab
Copy link

changab commented Oct 1, 2021

I will leave the decision to @kumarsankaran, whether or not to simply says "Must be implemented".

@kumarsankaran
Copy link
Collaborator

How about this folks?
GetTime must be implemented by the firmware and is not allowed to return EFI_UNSUPPORTED

@changab
Copy link

changab commented Mar 19, 2022

I have no problem with this. Thanks Kumar.

kumarsankaran added a commit to kumarsankaran/riscv-platform-specs that referenced this issue Mar 19, 2022
kumarsankaran added a commit that referenced this issue Mar 19, 2022
Fixed GetTime requirement. Issue #58
@kumarsankaran
Copy link
Collaborator

Fixed. Thanks.

AlexGhiti pushed a commit to AlexGhiti/riscv-platform-specs that referenced this issue Jun 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants