Re: [PATCH 3/3] Hisilicon/D05: Support SBSA watchdog

[Guo Heyi <heyi.guo@linaro.org>]

On Wed, Mar 07, 2018 at 04:10:23PM +0000, Ard Biesheuvel wrote:
> On 7 March 2018 at 06:55, Heyi Guo <heyi.guo@linaro.org> wrote:
> > From: Chenhui Sun <sunchenhui@huawei.com>
> >
> > Add description of SBSA watchdogs to ACPI GTDT on D05.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Chenhui Sun <sunchenhui@huawei.com>
> > Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > Cc: Graeme Gregory <graeme.gregory@linaro.org>
> > ---
> >  Platform/Hisilicon/D05/D05.dsc                              |  4 ++++
> >  Silicon/Hisilicon/Hi1616/D05AcpiTables/AcpiTablesHi1616.inf |  2 ++
> >  Silicon/Hisilicon/Hi1616/D05AcpiTables/Gtdt.aslc            | 19 +++++++------------
> >  3 files changed, 13 insertions(+), 12 deletions(-)
> >
> > diff --git a/Platform/Hisilicon/D05/D05.dsc b/Platform/Hisilicon/D05/D05.dsc
> > index 0792b0814ea1..22eaf356224d 100644
> > --- a/Platform/Hisilicon/D05/D05.dsc
> > +++ b/Platform/Hisilicon/D05/D05.dsc
> > @@ -418,6 +418,10 @@ [PcdsFixedAtBuild.common]
> >
> >    gHisiTokenSpaceGuid.Pcdsoctype|0x1610
> >
> > +  # SBSA watchdog on Hi1616
> > +  gArmTokenSpaceGuid.PcdGenericWatchdogControlBase|0x40500000
> > +  gArmTokenSpaceGuid.PcdGenericWatchdogRefreshBase|0x40600000
> > +
> >  ################################################################################
> >  #
> >  # Components Section - list of all EDK II Modules needed by this Platform
> > diff --git a/Silicon/Hisilicon/Hi1616/D05AcpiTables/AcpiTablesHi1616.inf b/Silicon/Hisilicon/Hi1616/D05AcpiTables/AcpiTablesHi1616.inf
> > index bb279c8e428e..6955e6145c30 100644
> > --- a/Silicon/Hisilicon/Hi1616/D05AcpiTables/AcpiTablesHi1616.inf
> > +++ b/Silicon/Hisilicon/Hi1616/D05AcpiTables/AcpiTablesHi1616.inf
> > @@ -55,5 +55,7 @@ [FixedPcd]
> >    gArmTokenSpaceGuid.PcdArmArchTimerIntrNum
> >    gArmTokenSpaceGuid.PcdArmArchTimerHypIntrNum
> >    gArmTokenSpaceGuid.PcdArmArchTimerVirtIntrNum
> > +  gArmTokenSpaceGuid.PcdGenericWatchdogControlBase
> > +  gArmTokenSpaceGuid.PcdGenericWatchdogRefreshBase
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterBase
> >
> > diff --git a/Silicon/Hisilicon/Hi1616/D05AcpiTables/Gtdt.aslc b/Silicon/Hisilicon/Hi1616/D05AcpiTables/Gtdt.aslc
> > index 2a9d209c00f0..6bc1bde2a490 100644
> > --- a/Silicon/Hisilicon/Hi1616/D05AcpiTables/Gtdt.aslc
> > +++ b/Silicon/Hisilicon/Hi1616/D05AcpiTables/Gtdt.aslc
> > @@ -29,6 +29,7 @@
> >  #define GTDT_TIMER_ALWAYS_ON_CAPABILITY EFI_ACPI_6_1_GTDT_TIMER_FLAG_ALWAYS_ON_CAPABILITY
> >
> >  #define GTDT_GTIMER_FLAGS           (GTDT_TIMER_ALWAYS_ON_CAPABILITY | GTDT_TIMER_ACTIVE_LOW | GTDT_TIMER_LEVEL_TRIGGERED)
> > +#define WATCHDOG_SPAN  0x20000000
> >
> 
> Please don't use
> 
> gArmTokenSpaceGuid.PcdGenericWatchdogXXXBase
> 
> to describe two different instances of the IP that are %!@ MB apart.
> 
> Instead, you could introduce your own PCDs in the HiSilicon token
> space, but I am also fine with creating local #defines in this file if
> the watchdog is not used anywhere else.

Yes they are not used anywhere else, and the number of SBSA watchdogs is SoC
related, so I will use local #defines. And I think we can simply change the
content of the macros when things change in the future.

Thanks,
Heyi

> 
> 
> >  #pragma pack (1)
> >
> > @@ -57,22 +58,16 @@ EFI_ACPI_6_1_GENERIC_TIMER_DESCRIPTION_TABLES Gtdt = {
> >      FixedPcdGet32 (PcdArmArchTimerHypIntrNum),    // UINT32  NonSecurePL2TimerGSIV
> >      GTDT_GTIMER_FLAGS,                            // UINT32  NonSecurePL2TimerFlags
> >      0xFFFFFFFFFFFFFFFF,                           // UINT64  CntReadBasePhysicalAddress
> > -#ifdef notyet
> > -    PV660_WATCHDOG_COUNT,                          // UINT32  PlatformTimerCount
> > +    HI1616_WATCHDOG_COUNT,                        // UINT32  PlatformTimerCount
> >      sizeof (EFI_ACPI_6_1_GENERIC_TIMER_DESCRIPTION_TABLE) // UINT32 PlatfromTimerOffset
> >    },
> >    {
> > -    EFI_ACPI_6_1_SBSA_GENERIC_WATCHDOG_STRUCTURE_INIT(
> > -        //FixedPcdGet32 (PcdGenericWatchdogRefreshBase), FixedPcdGet32 (PcdGenericWatchdogControlBase), 93, 0),
> > -        0, 0, 0, 0),
> > -    EFI_ACPI_6_1_SBSA_GENERIC_WATCHDOG_STRUCTURE_INIT(
> > -        //FixedPcdGet32 (PcdGenericWatchdogRefreshBase), FixedPcdGet32 (PcdGenericWatchdogControlBase), 94, EFI_ACPI_6_1_GTDT_SBSA_GENERIC_WATCHDOG_FLAG_SECURE_TIMER)
> > -        0, 0, 0, 0)
> > +    EFI_ACPI_5_1_SBSA_GENERIC_WATCHDOG_STRUCTURE_INIT(
> > +      FixedPcdGet32 (PcdGenericWatchdogRefreshBase), FixedPcdGet32 (PcdGenericWatchdogControlBase), 400, 0),
> > +    EFI_ACPI_5_1_SBSA_GENERIC_WATCHDOG_STRUCTURE_INIT(
> > +      FixedPcdGet32 (PcdGenericWatchdogRefreshBase) + WATCHDOG_SPAN, FixedPcdGet32 (PcdGenericWatchdogControlBase) + WATCHDOG_SPAN, 496, 0)
> > +
> >    }
> > -#else /* !notyet */
> > -  0, 0
> > -  }
> > -#endif
> >    };
> >
> >  //
> > --
> > 2.7.4
> >