Assumption about Timer freq In EDK2

[Dhaval Sharma]

Hi All,

I came across this timer implementation where it sounds like a generic RV timer is making assumptions about timer frequency being 100ns (considering 10Mhz Qemu clock). This should be a platform specific policy and not hardcoded one.

We could either get it from DT or CPU HOB or even PCD. Any thoughts?

Reference (timer.h):

comments are confusing, is it 100us or 100ns?

//// RISC-V use 100us timer.// The default timer tick duration is set to 10 ms = 10 * 1000 * 10 100 ns units//

#define DEFAULT_TIMER_TICK_DURATION 100000

Also the logic within TimerDriverSetTimerPeriod()

  mTimerPeriod     = TimerPeriod / 10; // convert unit from 100ns to 1us
  mLastPeriodStart = RiscVReadTimer ();
  SbiSetTimer (mLastPeriodStart + mTimerPeriod);

1uS conversion is something I do not understand why it is required.

--

Thanks!

=D

[Warkentin, Andrei]

I could have sworn I saw a patch being sent for review that made this a Pcd?

 

A

 

--
You received this message because you are subscribed to the Google Groups "RISC-V Firmware Exchange" group.
To unsubscribe from this group and stop receiving emails from it, send an email to fw-exchange...@riscv.org.
To view this discussion on the web visit https://groups.google.com/a/riscv.org/d/msgid/fw-exchange/CAAxYnhQUtHBOR2%2BUp%3DHxe17HkufbvVqz0aWew5gdYdoP2nvtmA%40mail.gmail.com.

[Sunil V L]

The frequency was a PCD variable from the start. There was a fix required to use it in few additional places since SBI expected in ticks.

The input to TimerDriverSetTimerPeriod is in 100ns units (you can see comments in the function header) and hence you see this conversion. So, it is not hard coded and should work for any frequency.

Since now libfdt is added in MdePkg, we can get rid of this PCD variable usage and fetch it from the DT.

To view this discussion on the web visit https://groups.google.com/a/riscv.org/d/msgid/fw-exchange/PH8PR11MB6856181B160F459F35ACA6D6831DA%40PH8PR11MB6856.namprd11.prod.outlook.com.

[Dhaval Sharma]

Hi Sunil,

Actually for me it did not work when I tried with a platform with different freq.

What I see is a fixed macro below. "The frequency was a PCD variable from the start" Can you please point me to the code that is taking this PCD into consideration?

timer.h

#define DEFAULT_TIMER_TICK_DURATION  100000

--

Thanks!

=D

[Sunil V L]

Hi Dhaval,

Have you changed the PCD in your platform's FDF? For ex: For RiscVVirtQemu, it is set here to 10MHz.

RiscVVirt.fdf.inc:SET gUefiCpuPkgTokenSpaceGuid.PcdCpuCoreCrystalClockFrequency = 10000000

You can see the PCD usage in the same TimerDriverSetTimerPeriod and other places.

[Dhaval Sharma]

Thanks Sunil for pointing that out, that function seems to relate to delay loops.

However my question was specifically for the timer interrupt. In the TimerDriverInitialize function, isn't it taking hardcoded value from timer.h?

=D

--

Thanks!

=D

[Sunil V L]

Not really. That is the default time period set during initialization similar to the timer implementation in other architectures. Later, any value can be set using EFI_TIMER_ARCH_PROTOCOL.

[Dhaval Sharma]

Programming any *arbitrary* value during initialization may cause much higher unnecessary churn (imagine a platform going from 10Mhz to 1Ghz clock). If it is the expectation that platform code will do the correct programming then it better be done by platform code only instead of inaccurate programming in the CPU module.

• I can see that ARM is doing that based on a PCD value
• 8254 has also more or less fixed crystal frequencies (~10Mhz) so for them it might be okay too

for us our platforms would vary a lot in terms of tick frequency. Any specific concerns with initializing it with proper platform configurable value? I do see that this is all platform independent code in a way- but maybe a PCD would do?

=D

--

Thanks!

=D

[Sunil V L]

Hi Dhaval,

I don't see an issue introducing the PCD if it adds value. But the PCD Arm uses is from EmbeddedPkg and we can't use it in CpuPkg.  IMO, it is better to get rid of the PCD/constant default period and just use the timer frequency value from DT. Andrei and Tuan may have some opinion or better suggestion.  If it is acceptable, would you mind sending the patch?

Thanks,
Sunil

[Dhaval Sharma]

Yes, I agree. While using it from DT is more scalable, it may not be easily consumable in UefiCpuPkg. CPU Hob we discussed in the past might be slightly better. An implementation similar to InitializeCpu() where it consumes CPU HOB would work out better in my opinion if we really want to avoid PCD root.

-D

--

Thanks!

=D

[Sunil V L]

Hi Dhaval,

Like I mentioned earlier, with libfdt added in MdePkg, UefiCpuPkg can make use it without creating circular dependencies. So, let us use DT itself and keep HOB to only data to as minimum as possible when there is no other easy way.

The advantage of getting info from FDT is, every platform doesn't need to define PCD variable.

Thanks,
Sunil

[Dhaval Sharma]

Hi Sunil,

It sounds like this patch would go much neatly on top of SSTC. When are you planning to submit that? I thought it was imminent, if so, I could wait a bit?

=D

--

Thanks!

=D

[Sunil V L]

Hi Dhaval,

My understanding is, Andrei is going to update the PI spec to add extension bitmap under the CPU HOB which is required for CMO to discover the extension in Pre-Dxe phase. Once that is done, SSTC series will just discover the extension from the CPU HOB and enable SSTC.

However, for your work, getting timer frequency from FDT should not have any dependency on SSTC. It should work with SBI timer also, right?

Thanks,
Sunil

[Dhaval Sharma]

Ohh, so SSTC is going to use CPU HOB and not DT? I was thinking HOB was for hartid and CMO. If SSTC is also being discovered using CPU HOB then yes, I can go ahead and submit a timer patch anyways.

=D

--

Thanks!

=D

[Sunil V L]

I think it is better we have single way/API to check whether extension is supported or not. Since CMO library is same for SEC and DXE, I was thinking HOB is required anyway to discover the extensions and SSTC/MMU can use the same mechanism. Other option is, we can have different CMO library for SEC and DXE phases with duplicated code. In that case, SEC phase library can traverse the FDT every time to check the extension but DXE instance can cache it. Since there is only one time CMO is used in SEC, it is probably ok. This way we don't need to define any thing in PI spec. Andrei, Tuan - thoughts?

[Tuan Phan]

Or we can have a protocol that contain all information that can retrieved from DT. Any modules that need to discover SSTC, etc… can add DEPEX on this protocol. And  user written DXE driver can use that protocol without linking library.

 

To view this discussion on the web visit https://groups.google.com/a/riscv.org/d/msgid/fw-exchange/de61e195-3dec-9101-860d-3338004f6729%40ventanamicro.com.

[Dhaval Sharma]

Maybe Protocol is better from a design perspective as multiple drivers within a phase can consume it seamlessly. Later we just pass it on as a HOB to Dxe phase where it again creates a protocol out of it for Dxe+ drivers to consume?

=D

--

Thanks!

=D

[Tuan Phan]

Very few use the info in pre-DXE so creating protocol make no sense, access DT directly instead. If we use this approach, no need to pass it in HOB, just retrieve from DT at initialization function when creating protocol in DXE phase.

[Sunil V L]

Hi Dhaval,

Can CMO library be split into two? One for SEC and other for DXE?

Whether HOB/protocol, I am concerned about its definition which requires
to be expanded as we add more extensions. If CMO library for SEC phase
can be different from the DXE, then I prefer just relying on FDT since
it already has widely used isa string definition. DXE module can use the
global variable and hence no need to traverse DT again. We have only one
time cache operations are performed in SEC phase. So, a SEC phase
library can just traverse FDT every time.

On 30/08/23 00:16, Tuan Phan wrote:
>
> Very few use the info in pre-DXE so creating protocol make no sense,
> access DT directly instead. If we use this approach, no need to pass
> it in HOB, just retrieve from DT at initialization function when
> creating protocol in DXE phase.
>

> *From: *Dhaval Sharma <dha...@rivosinc.com>
> *Date: *Tuesday, August 29, 2023 at 11:29 AM
> *To: *Tuan Phan <tp...@ventanamicro.com>
> *Cc: *Sunil V L <sun...@ventanamicro.com>, Warkentin, Andrei

> <andrei.w...@intel.com>, RISC-V Firmware Exchange
> <fw-ex...@riscv.org>

> *Subject: *Re: Assumption about Timer freq In EDK2

>
> Maybe Protocol is better from a design perspective as multiple drivers
> within a phase can consume it seamlessly. Later we just pass it on as
> a HOB to Dxe phase where it again creates a protocol out of it for
> Dxe+ drivers to consume?
>
> =D
>
> On Tue, Aug 29, 2023 at 9:36 PM Tuan Phan <tp...@ventanamicro.com> wrote:
>
> Or we can have a protocol that contain all information that can
> retrieved from DT. Any modules that need to discover SSTC, etc…
> can add DEPEX on this protocol. And  user written DXE driver can
> use that protocol without linking library.
>

> *From: *Sunil V L <sun...@ventanamicro.com>
> *Date: *Tuesday, August 29, 2023 at 2:15 AM
> *To: *Dhaval Sharma <dha...@rivosinc.com>
> *Cc: *Warkentin, Andrei <andrei.w...@intel.com>, RISC-V
> Firmware Exchange <fw-ex...@riscv.org>
> *Subject: *Re: Assumption about Timer freq In EDK2

> * I can see that ARM is doing that

> based on a PCD value

> * 8254 has also more or less fixed

> *From:* Dhaval
> Sharma
> <dha...@rivosinc.com>
> <mailto:dha...@rivosinc.com>
>
> *Sent:*

> Wednesday,
> August 23,
> 2023 12:13 AM

> *To:* RISC-V
> Firmware
> Exchange
> <fw-ex...@riscv.org>
> <mailto:fw-ex...@riscv.org>
> *Subject:*

> use *100us
> timer*.// The

> default timer
> tick duration
> is set to 10
> ms = 10 * 1000

> * 10 *100 ns
> *units//

>
> #define
> DEFAULT_TIMER_TICK_DURATION
> 100000
>
> Also the logic
> within TimerDriverSetTimerPeriod()
>

> *mTimerPeriod

>   =
> TimerPeriod /
> 10; // convert
> unit from
> 100ns to 1us

> *

> <https://groups.google.com/a/riscv.org/d/msgid/fw-exchange/CAAxYnhQUtHBOR2%2BUp%3DHxe17HkufbvVqz0aWew5gdYdoP2nvtmA%40mail.gmail.com?utm_medium=email&utm_source=footer>.

>
> --
> You received
> this message
> because you
> are subscribed
> to the Google
> Groups "RISC-V
> Firmware
> Exchange" group.
> To unsubscribe
> from this
> group and stop
> receiving
> emails from
> it, send an
> email to
> fw-exchange...@riscv.org.
> To view this
> discussion on
> the web visit

> https://groups.google.com/a/riscv.org/d/msgid/fw-exchange/PH8PR11MB6856181B160F459F35ACA6D6831DA%40PH8PR11MB6856.namprd11.prod.outlook.com
> <https://groups.google.com/a/riscv.org/d/msgid/fw-exchange/PH8PR11MB6856181B160F459F35ACA6D6831DA%40PH8PR11MB6856.namprd11.prod.outlook.com?utm_medium=email&utm_source=footer>.

> <https://groups.google.com/a/riscv.org/d/msgid/fw-exchange/de61e195-3dec-9101-860d-3338004f6729%40ventanamicro.com?utm_medium=email&utm_source=footer>.
>
>
> --
>
> Thanks!
>
> =D
>

[Dhaval Sharma]

Hi Sunil,

I am implementing DT parser logic in the lines of your SSTC implementation for Timer lib we discussed earlier. I am generalizing your implementation from ISA discovery lib to all RV relevant specific parser lib. Once merged either I can add (with your sign-off) or you can add SSTC discovery part to the same library which gives ISA extn and with that I will re-submit CMO patch. Hope that is fine with you.

Also, assuming FDT HOB will always be available when this library is used (in Sec/Dxe). I see Sec is building this HOB already. If not, we could always pass FDT pointer to this LIb and finding the FDT ptr remains callers responsibility.

=D 

  

=D

--

Thanks!

=D