Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1603602 > unrolled thread

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

Started byMark Rutland <mark.rutland@arm.com>
First post2017-03-17 21:20 +0100
Last post2017-03-20 19:40 +0100
Articles 5 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v21 13/13] acpi/arm64: Add SBSA Generic Watchdog support  in GTDT driver Mark Rutland <mark.rutland@arm.com> - 2017-03-17 21:20 +0100
    Re: [PATCH v21 13/13] acpi/arm64: Add SBSA Generic Watchdog support  in GTDT driver Mark Rutland <mark.rutland@arm.com> - 2017-03-20 19:20 +0100
      Re: [Linaro-acpi] [PATCH v21 13/13] acpi/arm64: Add SBSA Generic  Watchdog support in GTDT driver Fu Wei <fu.wei@linaro.org> - 2017-03-21 05:00 +0100
      Re: [PATCH v21 13/13] acpi/arm64: Add SBSA Generic Watchdog support  in GTDT driver Fu Wei <fu.wei@linaro.org> - 2017-03-21 06:20 +0100
    Re: [PATCH v21 13/13] acpi/arm64: Add SBSA Generic Watchdog support  in GTDT driver Fu Wei <fu.wei@linaro.org> - 2017-03-20 19:40 +0100

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

FromMark Rutland <mark.rutland@arm.com>
Date2017-03-17 21:20 +0100
SubjectRe: [PATCH v21 13/13] acpi/arm64: Add SBSA Generic Watchdog support in GTDT driver
Message-ID<tm6vT-3sM-11@gated-at.bofh.it>
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.

[toc] | [next] | [standalone]

#1604976

FromMark Rutland <mark.rutland@arm.com>
Date2017-03-20 19:20 +0100
Message-ID<tna4s-52-95@gated-at.bofh.it>
In reply to#1603602
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.

[toc] | [prev] | [next] | [standalone]

#1605285 — Re: [Linaro-acpi] [PATCH v21 13/13] acpi/arm64: Add SBSA Generic Watchdog support in GTDT driver

FromFu Wei <fu.wei@linaro.org>
Date2017-03-21 05:00 +0100
SubjectRe: [Linaro-acpi] [PATCH v21 13/13] acpi/arm64: Add SBSA Generic Watchdog support in GTDT driver
Message-ID<tnj7I-68t-23@gated-at.bofh.it>
In reply to#1604976
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



-- 
Best regards,

Fu Wei
Software Engineer
Red Hat

[toc] | [prev] | [next] | [standalone]

#1605315

FromFu Wei <fu.wei@linaro.org>
Date2017-03-21 06:20 +0100
Message-ID<tnkn7-7aS-5@gated-at.bofh.it>
In reply to#1604976
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.



-- 
Best regards,

Fu Wei
Software Engineer
Red Hat

[toc] | [prev] | [next] | [standalone]

#1605040

FromFu Wei <fu.wei@linaro.org>
Date2017-03-20 19:40 +0100
Message-ID<tna4s-52-97@gated-at.bofh.it>
In reply to#1603602
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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web