Re: [PATCH v21 13/13] acpi/arm64: Add SBSA Generic Watchdog support in GTDT driver

[Fu Wei]

Hi Mark

On 18 March 2017 at 04:01, Mark Rutland <mark.rutland@xxxxxxx> wrote:
> On Tue, Feb 07, 2017 at 02:50:15AM +0800, fu.wei@xxxxxxxxxx wrote:
>> +static int __init gtdt_import_sbsa_gwdt(struct acpi_gtdt_watchdog *wd,
>> +                                     int index)
>> +{
>> +     struct platform_device *pdev;
>> +     int irq = map_gt_gsi(wd->timer_interrupt, wd->timer_flags);
>> +     int no_irq = 1;
>> +
>> +     /*
>> +      * According to SBSA specification the size of refresh and control
>> +      * frames of SBSA Generic Watchdog is SZ_4K(Offset 0x000 â 0xFFF).
>> +      */
>> +     struct resource res[] = {
>> +             DEFINE_RES_MEM(wd->control_frame_address, SZ_4K),
>> +             DEFINE_RES_MEM(wd->refresh_frame_address, SZ_4K),
>> +             DEFINE_RES_IRQ(irq),
>> +     };
>> +
>> +     pr_debug("found a Watchdog (0x%llx/0x%llx gsi:%u flags:0x%x).\n",
>> +              wd->refresh_frame_address, wd->control_frame_address,
>> +              wd->timer_interrupt, wd->timer_flags);
>> +
>> +     if (!(wd->refresh_frame_address && wd->control_frame_address)) {
>> +             pr_err(FW_BUG "failed to get the Watchdog base address.\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     if (!wd->timer_interrupt)
>> +             pr_warn(FW_BUG "failed to get the Watchdog interrupt.\n");
>
> I've not been able to find where the ACPI spec says that zero is not a
> valid GSIV. This may simply be an oversight/ambiguity in the spec.
>
> Is there any statement to that effect?

you are right, zero is a  valid GSIV, I will delete this check. Thanks

>
>> +     else if (irq <= 0)
>> +             pr_warn("failed to map the Watchdog interrupt.\n");
>> +     else
>> +             no_irq = 0;
>> +
>> +     /*
>> +      * Add a platform device named "sbsa-gwdt" to match the platform driver.
>> +      * "sbsa-gwdt": SBSA(Server Base System Architecture) Generic Watchdog
>> +      * The platform driver (like drivers/watchdog/sbsa_gwdt.c)can get device
>> +      * info below by matching this name.
>> +      */
>> +     pdev = platform_device_register_simple("sbsa-gwdt", index, res,
>> +                                            ARRAY_SIZE(res) - no_irq);
>
> This no_irq variable is messy and confusing.
>
> Get rid of no_irq, and replace it with nr_res, initialised to
> ARRAY_SIZE(res). If there's no interrupt, subtract one.

Sure, you are right ,will do

>
> [...]
>
>> +     for_each_platform_timer(platform_timer) {
>> +             if (is_watchdog(platform_timer)) {
>> +                     ret = gtdt_import_sbsa_gwdt(platform_timer, i);
>> +                     if (ret)
>> +                             break;
>> +                     i++;
>> +             }
>> +     }
>> +
>> +     if (i)
>> +             pr_info("found %d SBSA generic Watchdog(s).\n", i);
>
> My reading of SBSA is that there is one watchdog in the system.
>
> Is that not the case?

do you mean:
---------------
4.2.4 Watchdogs
The base server system implements a Generic Watchdog as specified in
APPENDIX A: Generic Watchdog.
---------------

I am not sure about that if this is saying "we only have one SBSA
watchdog in a system"

would you let me know where mention it? Do I miss something?

Thanks :-)

>
> [...]
>
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index acb00b5..c899df1 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -219,6 +219,7 @@ config ARM_SBSA_WATCHDOG
>>       tristate "ARM SBSA Generic Watchdog"
>>       depends on ARM64
>>       depends on ARM_ARCH_TIMER
>> +     depends on ACPI_GTDT || !ACPI
>
> I don't think this is necessary.
>
> This series hasn't touched this driver code at all.

yes, since we are using "select ACPI_GTDT if ACPI" in ARM64, we don't this.
Thanks for pointing it out.
:-)

>
> Thanks,
> Mark.



-- 
Best regards,

Fu Wei
Software Engineer
Red Hat