| Message ID | 20170206185015.12296-14-fu.wei@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v21,01/13] clocksource: arm_arch_timer: introduce two functions to get the frequency from mmio and sysreg. | expand |
Hi Mark On 18 March 2017 at 04:01, Mark Rutland <mark.rutland@arm.com> wrote: > On Tue, Feb 07, 2017 at 02:50:15AM +0800, fu.wei@linaro.org 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
On Tue, Mar 21, 2017 at 01:57:58AM +0800, Fu Wei wrote: > On 18 March 2017 at 04:01, Mark Rutland <mark.rutland@arm.com> wrote: > > On Tue, Feb 07, 2017 at 02:50:15AM +0800, fu.wei@linaro.org wrote: > > 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 That being the case, how does one describe a watchdog that does not have an interrupt? As I mentioned, I think this is an oversight/ambiguity in the spec tat we should address. > > 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? My reading was that the 'a' above meant a single element. i.e. The base server system implements _a_ Generic Watchdog as specified in APPENDIX A: Generic Watchdog. Subsequently in 4.2.5, it is stated: In this scenario, the system wakeup timer or generic watchdog is still required to send its interrupt. ... which only makes sense if there is a single watchdog in the system. Perhaps this is an oversight in the specification. Thanks, Mark.
diff --git a/drivers/acpi/arm64/gtdt.c b/drivers/acpi/arm64/gtdt.c index 29b9acc..3a2eb57 100644 --- a/drivers/acpi/arm64/gtdt.c +++ b/drivers/acpi/arm64/gtdt.c @@ -14,6 +14,7 @@ #include <linux/acpi.h> #include <linux/init.h> #include <linux/kernel.h> +#include <linux/platform_device.h> #include <clocksource/arm_arch_timer.h> @@ -59,6 +60,13 @@ static inline bool is_timer_block(void *platform_timer) return gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK; } +static inline bool is_watchdog(void *platform_timer) +{ + struct acpi_gtdt_header *gh = platform_timer; + + return gh->type == ACPI_GTDT_TYPE_WATCHDOG; +} + static int __init map_gt_gsi(u32 interrupt, u32 flags) { int trigger, polarity; @@ -283,3 +291,88 @@ int __init acpi_arch_timer_mem_init(struct arch_timer_mem *data, return 0; } + +/* + * Initialize a SBSA generic Watchdog platform device info from GTDT + */ +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"); + 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); + if (IS_ERR(pdev)) { + acpi_unregister_gsi(wd->timer_interrupt); + return PTR_ERR(pdev); + } + + return 0; +} + +static int __init gtdt_sbsa_gwdt_init(void) +{ + int ret, i = 0; + void *platform_timer; + struct acpi_table_header *table; + + if (acpi_disabled) + return 0; + + if (ACPI_FAILURE(acpi_get_table(ACPI_SIG_GTDT, 0, &table))) + return -EINVAL; + + ret = acpi_gtdt_init(table, NULL); + if (ret) + return ret; + + 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); + + return ret; +} + +device_initcall(gtdt_sbsa_gwdt_init); 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 select WATCHDOG_CORE help ARM SBSA Generic Watchdog has two stage timeouts: