| Message ID | 1409058229-28802-3-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 08/26/14 15:03, Ard Biesheuvel wrote: > To support booting on virtual machines whose interrupt routing is > discovered from the device tree, allow the interrupt numbers to > be redeclared as PcdsDynamic by the platform .dsc > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > ArmPkg/ArmPkg.dec | 2 ++ > ArmPkg/Drivers/TimerDxe/TimerDxe.c | 6 +++++- > 2 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec > index c2551d7c3307..0392af52758f 100644 > --- a/ArmPkg/ArmPkg.dec > +++ b/ArmPkg/ArmPkg.dec > @@ -140,6 +140,8 @@ > # Maximum file size for TFTP servers that do not support 'tsize' extension > gArmTokenSpaceGuid.PcdMaxTftpFileSize|0x01000000|UINT32|0x00000000 > > + > +[PcdsFixedAtBuild.common,PcdsDynamic.common] > # > # ARM Architectural Timer > # > diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c > index 9227be8326b0..2787f05c62be 100644 > --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c > +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c > @@ -36,6 +36,8 @@ EFI_EVENT EfiExitBootServicesEvent = (EFI_EVENT)NULL; > // The current period of the timer interrupt > UINT64 mTimerPeriod = 0; > > +UINTN mArmArchTimerTimerFreq = 0; > + > // Cached copy of the Hardware Interrupt protocol instance > EFI_HARDWARE_INTERRUPT_PROTOCOL *gInterrupt = NULL; > > @@ -144,7 +146,7 @@ TimerDriverSetTimerPeriod ( > // Convert TimerPeriod to micro sec units > TimerTicks = DivU64x32 (TimerPeriod, 10); > > - TimerTicks = MultU64x32 (TimerTicks, (PcdGet32(PcdArmArchTimerFreqInHz)/1000000)); > + TimerTicks = MultU64x32 (TimerTicks, mArmArchTimerTimerFreq); > > ArmArchTimerSetTimerVal((UINTN)TimerTicks); > > @@ -343,6 +345,8 @@ TimerInitialize ( > Status = TimerDriverSetTimerPeriod (&gTimer, 0); > ASSERT_EFI_ERROR (Status); > > + mArmArchTimerTimerFreq = ArmArchTimerGetTimerFreq () / 1000000; > + > // Install secure and Non-secure interrupt handlers > // Note: Because it is not possible to determine the security state of the > // CPU dynamically, we just install interrupt handler for both sec and non-sec > May not be important in practice, but this patch could be cleaner. ArmArchTimerGetTimerFreq() returns an UINTN, so the type of mArmArchTimerTimerFreq is OK thus far. But the second parameter of MultU64x32() should be UINT32, not UINTN. I'd declare mArmArchTimerTimerFreq with type UINT32, use a UINTN temporary with ArmArchTimerGetTimerFreq(), and do an explicit range check & assignment between them. Feel free to ignore this. Another thing I notice is that the ArmPkg.dec hunk changes the allowed PCD types not only for - PcdArmArchTimerSecIntrNum - PcdArmArchTimerIntrNum - PcdArmArchTimerHypIntrNum - PcdArmArchTimerVirtIntrNum but also for PcdArmArchTimerFreqInHz too -- is that intended? The commit message doesn't mention it. And, this patch actually removes one read of PcdArmArchTimerFreqInHz. Thanks, Laszlo ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/
On 26 August 2014 20:32, Laszlo Ersek <lersek@redhat.com> wrote: > On 08/26/14 15:03, Ard Biesheuvel wrote: >> To support booting on virtual machines whose interrupt routing is >> discovered from the device tree, allow the interrupt numbers to >> be redeclared as PcdsDynamic by the platform .dsc >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> ArmPkg/ArmPkg.dec | 2 ++ >> ArmPkg/Drivers/TimerDxe/TimerDxe.c | 6 +++++- >> 2 files changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec >> index c2551d7c3307..0392af52758f 100644 >> --- a/ArmPkg/ArmPkg.dec >> +++ b/ArmPkg/ArmPkg.dec >> @@ -140,6 +140,8 @@ >> # Maximum file size for TFTP servers that do not support 'tsize' extension >> gArmTokenSpaceGuid.PcdMaxTftpFileSize|0x01000000|UINT32|0x00000000 >> >> + >> +[PcdsFixedAtBuild.common,PcdsDynamic.common] >> # >> # ARM Architectural Timer >> # >> diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c >> index 9227be8326b0..2787f05c62be 100644 >> --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c >> +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c >> @@ -36,6 +36,8 @@ EFI_EVENT EfiExitBootServicesEvent = (EFI_EVENT)NULL; >> // The current period of the timer interrupt >> UINT64 mTimerPeriod = 0; >> >> +UINTN mArmArchTimerTimerFreq = 0; >> + >> // Cached copy of the Hardware Interrupt protocol instance >> EFI_HARDWARE_INTERRUPT_PROTOCOL *gInterrupt = NULL; >> >> @@ -144,7 +146,7 @@ TimerDriverSetTimerPeriod ( >> // Convert TimerPeriod to micro sec units >> TimerTicks = DivU64x32 (TimerPeriod, 10); >> >> - TimerTicks = MultU64x32 (TimerTicks, (PcdGet32(PcdArmArchTimerFreqInHz)/1000000)); >> + TimerTicks = MultU64x32 (TimerTicks, mArmArchTimerTimerFreq); >> >> ArmArchTimerSetTimerVal((UINTN)TimerTicks); >> >> @@ -343,6 +345,8 @@ TimerInitialize ( >> Status = TimerDriverSetTimerPeriod (&gTimer, 0); >> ASSERT_EFI_ERROR (Status); >> >> + mArmArchTimerTimerFreq = ArmArchTimerGetTimerFreq () / 1000000; >> + >> // Install secure and Non-secure interrupt handlers >> // Note: Because it is not possible to determine the security state of the >> // CPU dynamically, we just install interrupt handler for both sec and non-sec >> > > May not be important in practice, but this patch could be cleaner. > > ArmArchTimerGetTimerFreq() returns an UINTN, so the type of > mArmArchTimerTimerFreq is OK thus far. > > But the second parameter of MultU64x32() should be UINT32, not UINTN. > I'd declare mArmArchTimerTimerFreq with type UINT32, use a UINTN > temporary with ArmArchTimerGetTimerFreq(), and do an explicit range > check & assignment between them. > > Feel free to ignore this. > > Another thing I notice is that the ArmPkg.dec hunk changes the allowed > PCD types not only for > - PcdArmArchTimerSecIntrNum > - PcdArmArchTimerIntrNum > - PcdArmArchTimerHypIntrNum > - PcdArmArchTimerVirtIntrNum > > but also for PcdArmArchTimerFreqInHz too -- is that intended? The commit > message doesn't mention it. And, this patch actually removes one read of > PcdArmArchTimerFreqInHz. > I have a question pending with Olivier: I don't think it makes sense to use the PCD value for the counter frequency, as we have already established earlier that the system register contains a sane value. Otherwise, it means I should change the frequency PCD to dynamic as well, and read the counter frequency in platform code so we can read the PCD here, which sounds overly complicated to me. So depending on which way we go here, I should either reinstate the PcdGet32 () or make it fixed again. Regarding the types, I will try to address that as well, depending on how Olivier prefers to proceed with this patch.
diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec index c2551d7c3307..0392af52758f 100644 --- a/ArmPkg/ArmPkg.dec +++ b/ArmPkg/ArmPkg.dec @@ -140,6 +140,8 @@ # Maximum file size for TFTP servers that do not support 'tsize' extension gArmTokenSpaceGuid.PcdMaxTftpFileSize|0x01000000|UINT32|0x00000000 + +[PcdsFixedAtBuild.common,PcdsDynamic.common] # # ARM Architectural Timer # diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c index 9227be8326b0..2787f05c62be 100644 --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c @@ -36,6 +36,8 @@ EFI_EVENT EfiExitBootServicesEvent = (EFI_EVENT)NULL; // The current period of the timer interrupt UINT64 mTimerPeriod = 0; +UINTN mArmArchTimerTimerFreq = 0; + // Cached copy of the Hardware Interrupt protocol instance EFI_HARDWARE_INTERRUPT_PROTOCOL *gInterrupt = NULL; @@ -144,7 +146,7 @@ TimerDriverSetTimerPeriod ( // Convert TimerPeriod to micro sec units TimerTicks = DivU64x32 (TimerPeriod, 10); - TimerTicks = MultU64x32 (TimerTicks, (PcdGet32(PcdArmArchTimerFreqInHz)/1000000)); + TimerTicks = MultU64x32 (TimerTicks, mArmArchTimerTimerFreq); ArmArchTimerSetTimerVal((UINTN)TimerTicks); @@ -343,6 +345,8 @@ TimerInitialize ( Status = TimerDriverSetTimerPeriod (&gTimer, 0); ASSERT_EFI_ERROR (Status); + mArmArchTimerTimerFreq = ArmArchTimerGetTimerFreq () / 1000000; + // Install secure and Non-secure interrupt handlers // Note: Because it is not possible to determine the security state of the // CPU dynamically, we just install interrupt handler for both sec and non-sec
To support booting on virtual machines whose interrupt routing is discovered from the device tree, allow the interrupt numbers to be redeclared as PcdsDynamic by the platform .dsc Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- ArmPkg/ArmPkg.dec | 2 ++ ArmPkg/Drivers/TimerDxe/TimerDxe.c | 6 +++++- 2 files changed, 7 insertions(+), 1 deletion(-)