| Message ID | 20170206185015.12296-14-fu.wei@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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? > + 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. [...] > + 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? [...] > 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. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 20, 2017 at 06:09:50PM +0000, Mark Rutland wrote: > 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. It is a requirement of a conforming implementation that there be a generic watchdog (impl as per the appendix). That doesn't preclude an implmentation from providing additional watchdogs (for example, if the processor implements EL3, it is likely that an implementation will include a secure watchdog as well as a non-secure watchdog). The SBSA describes the minimal hardware requirements for a compliant server. scott -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mark, Lurndal, On 21 March 2017 at 02:50, Lurndal, Scott <Scott.Lurndal@cavium.com> wrote: > On Mon, Mar 20, 2017 at 06:09:50PM +0000, Mark Rutland wrote: >> 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. > > It is a requirement of a conforming implementation that there > be a generic watchdog (impl as per the appendix). That doesn't preclude > an implmentation from providing additional watchdogs (for example, if > the processor implements EL3, it is likely that an implementation > will include a secure watchdog as well as a non-secure watchdog). > > The SBSA describes the minimal hardware requirements for a > compliant server. So I think, for the SBSA watchdog: (1) there maybe more then one non-secure watchdog in GTDT (2) we may also need to skip secure watchdogs in GTDT, and only register non-secure watchdogs into platform resources. Please correct me, if I misunderstand something. > > scott
Hi Mark, On 21 March 2017 at 02:09, Mark Rutland <mark.rutland@arm.com> wrote: > 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? I think we may can use "Timer Flags", because all the GSIV come with a flag, if we can define a bit field called "valid" for all GSIV Bit Field Bit Offset Number of bits Description Valid 31 1 This bit indicates the validity of the timer interrupt 1: Interrupt is valid 0: Interrupt is invalid Then we don't need to test the value of GSIV, just test this bit instead. Just my thought, hope this makes sense to all of you :-) > > 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.
On Tue, Mar 21, 2017 at 11:48:02AM +0800, Fu Wei wrote: > Hi Mark, Lurndal, > > >> The base server system implements _a_ Generic Watchdog as > >> specified in APPENDIX A: Generic Watchdog. > > > > It is a requirement of a conforming implementation that there > > be a generic watchdog (impl as per the appendix). That doesn't preclude > > an implmentation from providing additional watchdogs (for example, if > > the processor implements EL3, it is likely that an implementation > > will include a secure watchdog as well as a non-secure watchdog). > > > > The SBSA describes the minimal hardware requirements for a > > compliant server. > > So I think, for the SBSA watchdog: > > (1) there maybe more then one non-secure watchdog in GTDT I suppose a firmware vendor could chose to expose more than the platform minimum to linux. Not quite sure what linux would do with it :-) > > (2) we may also need to skip secure watchdogs in GTDT, > and only register non-secure watchdogs into platform resources. The secure watchdog will, be definition, never be accessible to non-secure EL2 or non-secure EL1, so only in the case of a secure EL1 operating system would the secure watchdog ever be conveyed through ACPI. I don't think secure EL1 is in the scope of the SBSA, or something that needs to be accomodated in the GTDT at this time. scott -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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: