riscvarchive / riscv-platform-specs

RISC-V Profiles and Platform Specification
Creative Commons Attribution 4.0 International
112 stars 39 forks source link

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

Closed afaerber closed 2 years ago

afaerber commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

kumarsankaran commented 2 years ago

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

changab commented 2 years ago

I have no problem with this. Thanks Kumar.

kumarsankaran commented 2 years ago

Fixed. Thanks.