| Message ID | 1432548193-19569-7-git-send-email-fu.wei@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi Fu Wei, Some minor comments inline. On 2015年05月25日 18:03, fu.wei@linaro.org wrote: > From: Fu Wei <fu.wei@linaro.org> > > Parse SBSA Generic Watchdog Structure in GTDT table of ACPI, > and create a platform device with that information. > This platform device can be used by the ARM SBSA Generic > Watchdog driver. > > Tested-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> > Tested-by: Timur Tabi <timur@codeaurora.org> > Signed-off-by: Fu Wei <fu.wei@linaro.org> > --- > arch/arm64/kernel/acpi.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 145 insertions(+) > > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c > index 8b83955..c95deec 100644 > --- a/arch/arm64/kernel/acpi.c > +++ b/arch/arm64/kernel/acpi.c > @@ -23,6 +23,7 @@ > #include <linux/irqdomain.h> > #include <linux/memblock.h> > #include <linux/of_fdt.h> > +#include <linux/platform_device.h> > #include <linux/smp.h> > > #include <asm/cputype.h> > @@ -343,3 +344,147 @@ void __init acpi_gic_init(void) > > early_acpi_os_unmap_memory((char *)table, tbl_size); > } please add #ifdef CONFIG_ARM_SBSA_WATCHDOG (acpi gtdt code) #endif > + > +static int __init acpi_gtdt_import_sbsa_gwdt(struct acpi_gtdt_watchdog *wd, > + int index) > +{ > + struct platform_device *pdev; > + struct resource *res; > + u32 gsi, flags; > + int irq, trigger, polarity; > + resource_size_t rf_base_phy, cf_base_phy; > + int err = -ENOMEM; > + > + /* > + * Get SBSA Generic Watchdog info > + * from a Watchdog GT type structure in GTDT > + */ > + rf_base_phy = (resource_size_t)wd->refresh_frame_address; > + cf_base_phy = (resource_size_t)wd->control_frame_address; > + gsi = wd->timer_interrupt; > + flags = wd->timer_flags; > + > + pr_debug("GTDT: a Watchdog GT structure(0x%llx/0x%llx gsi:%u flags:0x%x)\n", > + rf_base_phy, cf_base_phy, gsi, flags); > + > + if (!(rf_base_phy && cf_base_phy && gsi)) { > + pr_err("GTDT: failed geting the device info.\n"); > + return -EINVAL; > + } > + > + trigger = (flags & ACPI_GTDT_INTERRUPT_MODE) ? ACPI_EDGE_SENSITIVE > + : ACPI_LEVEL_SENSITIVE; > + polarity = (flags & ACPI_GTDT_INTERRUPT_POLARITY) ? ACPI_ACTIVE_LOW > + : ACPI_ACTIVE_HIGH; > + irq = acpi_register_gsi(NULL, gsi, trigger, polarity); > + if (irq < 0) { > + pr_err("GTDT: failed to register GSI of the Watchdog GT.\n"); > + return -EINVAL; > + } > + > + /* > + * Add a platform device named "sbsa-gwdt" to match the platform driver. > + * "sbsa-gwdt": SBSA(Server Base System Architecture) Generic Watchdog > + * The platform driver can get device info below by matching this name. * The platform driver (drivers/watchdog/sbsa_gwdt.c) can get device info below by matching this name. Adding the file name which will help for review and maintain in my opinion. > + */ > + pdev = platform_device_alloc("sbsa-gwdt", index); > + if (!pdev) > + goto err_unregister_gsi; > + > + res = kcalloc(3, sizeof(*res), GFP_KERNEL); > + if (!res) > + goto err_device_put; > + > + /* > + * According to SBSA specification the size of refresh and control > + * frames of SBSA Generic Watchdog is SZ_4K(Offset 0x000 – 0xFFF). > + */ > + res[0].start = rf_base_phy; > + res[0].end = rf_base_phy + SZ_4K - 1; > + res[0].name = "refresh"; > + res[0].flags = IORESOURCE_MEM; > + > + res[1].start = cf_base_phy; > + res[1].end = cf_base_phy + SZ_4K - 1; > + res[1].name = "control"; > + res[1].flags = IORESOURCE_MEM; > + > + res[2].start = irq; > + res[2].end = res[2].start; > + res[2].name = "ws0"; > + res[2].flags = IORESOURCE_IRQ; > + > + err = platform_device_add_resources(pdev, res, 3); > + if (err) > + goto err_free_res; > + > + err = platform_device_add(pdev); ... > + if (err) > + goto err_free_res; > + > + return 0; How about if (!err) return 0; then no need for goto err_free_res and save two lines of codes. Other than that, Acked-by: Hanjun Guo <hanjun.guo@linaro.org> Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Hi Will, On 26 May 2015 at 08:28, Will Deacon <will.deacon@arm.com> wrote: > On Mon, May 25, 2015 at 11:03:13AM +0100, fu.wei@linaro.org wrote: >> From: Fu Wei <fu.wei@linaro.org> >> >> Parse SBSA Generic Watchdog Structure in GTDT table of ACPI, >> and create a platform device with that information. >> This platform device can be used by the ARM SBSA Generic >> Watchdog driver. >> >> Tested-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> >> Tested-by: Timur Tabi <timur@codeaurora.org> >> Signed-off-by: Fu Wei <fu.wei@linaro.org> >> --- >> arch/arm64/kernel/acpi.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 145 insertions(+) > > Why does this all need to be under arch/arm64? The GTDT really isn't > architecture-specific, so I'd *much* rather it was parsed in the driver code > itself, like we already do for the architected timer. The GIC is an > exception because it's in the MADT, which we need to parse in the arch code > to configure SMP properly. I'm not really against refactoring the code. But the GTDT looks quite specific to ARM.. ---8<---- 5.2.24 Generic Timer Description Table (GTDT) This section describes the format of the Generic Timer Description Table (GTDT), which provides OSPM with information about a system’s Generic Timers configuration. The Generic Timer (GT) is a standard timer interface implemented on ARM processor-based systems. The GT hardware specification can be found at Links to ACPI-Related Documents (http://uefi.org/acpi) under the heading ARM Architecture. The GTDT provides OSPM with information about a system's GT interrupt configurations, for both per-processor timers, and platform (memory-mapped) timers. The GT specification defines the following per-processor timers: • Secure privilege level 1 (EL1) timer, • Non-Secure EL1 timer, • Non-Secure privilege level 2 (EL2) timer, • Virtual timer, and the following Platform (memory-mapped) timers. • GT Block • Server Base System Architecture (SBSA) Generic Watchdog ---8<---- Thanks, Ashwin. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 26 May 2015 at 11:18, Will Deacon <will.deacon@arm.com> wrote: > On Tue, May 26, 2015 at 04:02:56PM +0100, Ashwin Chaugule wrote: >> On 26 May 2015 at 08:28, Will Deacon <will.deacon@arm.com> wrote: >> > On Mon, May 25, 2015 at 11:03:13AM +0100, fu.wei@linaro.org wrote: >> >> From: Fu Wei <fu.wei@linaro.org> >> >> >> >> Parse SBSA Generic Watchdog Structure in GTDT table of ACPI, >> >> and create a platform device with that information. >> >> This platform device can be used by the ARM SBSA Generic >> >> Watchdog driver. >> >> >> >> Tested-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> >> >> Tested-by: Timur Tabi <timur@codeaurora.org> >> >> Signed-off-by: Fu Wei <fu.wei@linaro.org> >> >> --- >> >> arch/arm64/kernel/acpi.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++ >> >> 1 file changed, 145 insertions(+) >> > >> > Why does this all need to be under arch/arm64? The GTDT really isn't >> > architecture-specific, so I'd *much* rather it was parsed in the driver code >> > itself, like we already do for the architected timer. The GIC is an >> > exception because it's in the MADT, which we need to parse in the arch code >> > to configure SMP properly. >> >> I'm not really against refactoring the code. But the GTDT looks quite >> specific to ARM.. >> >> ---8<---- >> 5.2.24 Generic Timer Description Table (GTDT) >> This section describes the format of the Generic Timer Description >> Table (GTDT), which provides >> OSPM with information about a system’s Generic Timers configuration. >> The Generic Timer (GT) is >> a standard timer interface implemented on ARM processor-based systems. >> The GT hardware >> specification can be found at Links to ACPI-Related Documents >> (http://uefi.org/acpi) under the >> heading ARM Architecture. The GTDT provides OSPM with information >> about a system's GT >> interrupt configurations, for both per-processor timers, and platform >> (memory-mapped) timers. >> The GT specification defines the following per-processor timers: >> • Secure privilege level 1 (EL1) timer, >> • Non-Secure EL1 timer, >> • Non-Secure privilege level 2 (EL2) timer, >> • Virtual timer, >> and the following Platform (memory-mapped) timers. >> • GT Block >> • Server Base System Architecture (SBSA) Generic Watchdog >> ---8<---- > > Sure, the device it describes may only ever exist on ARM systems, but by > that logic then we should be moving lots of drivers back under arch/arm[64]. Sure. Not arguing about that. :) You said the GTDT isn't really arch specific. That was a bit confusing. > > The ARM architecture says precisely *nothing* about ACPI, so we should > try to keep arch/arm64/kernel/acpi.c to a minimum and not shovel all sorts > of table conversion code in there for random peripherals. > > Will -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 26 May 2015 at 23:36, Guenter Roeck <linux@roeck-us.net> wrote: > On Tue, May 26, 2015 at 04:18:42PM +0100, Will Deacon wrote: >> On Tue, May 26, 2015 at 04:02:56PM +0100, Ashwin Chaugule wrote: >> > On 26 May 2015 at 08:28, Will Deacon <will.deacon@arm.com> wrote: >> > > On Mon, May 25, 2015 at 11:03:13AM +0100, fu.wei@linaro.org wrote: >> > >> From: Fu Wei <fu.wei@linaro.org> >> > >> >> > >> Parse SBSA Generic Watchdog Structure in GTDT table of ACPI, >> > >> and create a platform device with that information. >> > >> This platform device can be used by the ARM SBSA Generic >> > >> Watchdog driver. >> > >> >> > >> Tested-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> >> > >> Tested-by: Timur Tabi <timur@codeaurora.org> >> > >> Signed-off-by: Fu Wei <fu.wei@linaro.org> >> > >> --- >> > >> arch/arm64/kernel/acpi.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++ >> > >> 1 file changed, 145 insertions(+) >> > > >> > > Why does this all need to be under arch/arm64? The GTDT really isn't >> > > architecture-specific, so I'd *much* rather it was parsed in the driver code >> > > itself, like we already do for the architected timer. The GIC is an >> > > exception because it's in the MADT, which we need to parse in the arch code >> > > to configure SMP properly. >> > >> > I'm not really against refactoring the code. But the GTDT looks quite >> > specific to ARM.. >> > >> > ---8<---- >> > 5.2.24 Generic Timer Description Table (GTDT) >> > This section describes the format of the Generic Timer Description >> > Table (GTDT), which provides >> > OSPM with information about a system’s Generic Timers configuration. >> > The Generic Timer (GT) is >> > a standard timer interface implemented on ARM processor-based systems. >> > The GT hardware >> > specification can be found at Links to ACPI-Related Documents >> > (http://uefi.org/acpi) under the >> > heading ARM Architecture. The GTDT provides OSPM with information >> > about a system's GT >> > interrupt configurations, for both per-processor timers, and platform >> > (memory-mapped) timers. >> > The GT specification defines the following per-processor timers: >> > • Secure privilege level 1 (EL1) timer, >> > • Non-Secure EL1 timer, >> > • Non-Secure privilege level 2 (EL2) timer, >> > • Virtual timer, >> > and the following Platform (memory-mapped) timers. >> > • GT Block >> > • Server Base System Architecture (SBSA) Generic Watchdog >> > ---8<---- >> >> Sure, the device it describes may only ever exist on ARM systems, but by >> that logic then we should be moving lots of drivers back under arch/arm[64]. >> > It is nt the driver, but its instantiation. The question here would be > how and where to instantiate the driver, not where the driver itself > is located. The driver itself is ACPI agnostic. Hi Will, I really don't mind to refactor the code, If we can make this patch better. But for now, I can't see the good reason to move ACPI-relevant code into a watchdog driver. The reasons I put the code here are (1)SBSA watchdog only for ARM64 (2)GTDT only for ARM, design for ARM, (3)For ARM Architecture, only ARM64 support ACPI. For minimizing arch/arm64/kernel/acpi.c, we can't put the code here, and we had better keep these code outside the driver, So do you have any suggestion for the better location of the GTDT code? Great thanks for your help. > > What you are really saying is that you want the driver instantiation > to be moved into the driver. > > Guenter
On 2015年05月27日 00:35, Timur Tabi wrote: > On 05/26/2015 03:28 AM, Hanjun Guo wrote: > >>> early_acpi_os_unmap_memory((char *)table, tbl_size); >>> } >> >> please add >> >> #ifdef CONFIG_ARM_SBSA_WATCHDOG >> (acpi gtdt code) >> #endif > > I don't agree with this. The GTDT should be parsed even if there's no > watchdog driver compiled for this kernel. There are no other #ifdefs in > this file. So what's the point of parse GTDT and alloc memories for it if there is no watchdog driver compiled for the kernel? will the module insmod later even if the CONFIG_ARM_SBSA_WATCHDOG=n? > >>> + * Add a platform device named "sbsa-gwdt" to match the platform >>> driver. >>> + * "sbsa-gwdt": SBSA(Server Base System Architecture) Generic >>> Watchdog >>> + * The platform driver can get device info below by matching this >>> name. >> >> * The platform driver (drivers/watchdog/sbsa_gwdt.c) can get device info >> below by matching this name. >> >> Adding the file name which will help for review and maintain in my >> opinion. > > Except it will cause problems if the driver is renamed or moved. I > don't think this is a good idea, either (sorry!) OK, that's good point. but what I proposed is some hint to which driver will use the data prepared in this file, we can easily understand it in this patchset, but if just review the code in this fiel, I think people will be confused without detail comments. Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Will, As you know, I have moved all the GTDT code to ACPI driver , and simplify the GTDT relevant code in arm_arch_timer.c. That will be in my next patchset. but you can check here : https://git.linaro.org/people/fu.wei/linux.git/shortlog/refs/heads/acpi-topic-sbsa-watchdog_upstream_v4_devel Great thanks for your suggestion! On 27 May 2015 at 18:44, Will Deacon <will.deacon@arm.com> wrote: > On Tue, May 26, 2015 at 05:27:33PM +0100, Fu Wei wrote: >> On 26 May 2015 at 23:36, Guenter Roeck <linux@roeck-us.net> wrote: >> > On Tue, May 26, 2015 at 04:18:42PM +0100, Will Deacon wrote: >> >> Sure, the device it describes may only ever exist on ARM systems, but by >> >> that logic then we should be moving lots of drivers back under arch/arm[64]. >> >> >> > It is nt the driver, but its instantiation. The question here would be >> > how and where to instantiate the driver, not where the driver itself >> > is located. The driver itself is ACPI agnostic. >> >> I really don't mind to refactor the code, If we can make this patch better. >> >> But for now, I can't see the good reason to move ACPI-relevant code >> into a watchdog driver. > > I don't really mind where you move it, just as long as it's outside of > arch/arm64. > >> The reasons I put the code here are >> (1)SBSA watchdog only for ARM64 >> (2)GTDT only for ARM, design for ARM, >> (3)For ARM Architecture, only ARM64 support ACPI. >> >> For minimizing arch/arm64/kernel/acpi.c, we can't put the code here, >> and we had better keep these code outside the driver, >> >> So do you have any suggestion for the better location of the GTDT code? > > I don't understand why you can't do the same as > drivers/clocksource/arm_arch_timer.c and parse the table directly in the > driver. If there are objections from the driver/subsystem maintainers then > it sounds like we need a mechanical ACPI table -> platform device > conversion in the core, like we have for device-tree. > > Will
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 8b83955..c95deec 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -23,6 +23,7 @@ #include <linux/irqdomain.h> #include <linux/memblock.h> #include <linux/of_fdt.h> +#include <linux/platform_device.h> #include <linux/smp.h> #include <asm/cputype.h> @@ -343,3 +344,147 @@ void __init acpi_gic_init(void) early_acpi_os_unmap_memory((char *)table, tbl_size); } + +static int __init acpi_gtdt_import_sbsa_gwdt(struct acpi_gtdt_watchdog *wd, + int index) +{ + struct platform_device *pdev; + struct resource *res; + u32 gsi, flags; + int irq, trigger, polarity; + resource_size_t rf_base_phy, cf_base_phy; + int err = -ENOMEM; + + /* + * Get SBSA Generic Watchdog info + * from a Watchdog GT type structure in GTDT + */ + rf_base_phy = (resource_size_t)wd->refresh_frame_address; + cf_base_phy = (resource_size_t)wd->control_frame_address; + gsi = wd->timer_interrupt; + flags = wd->timer_flags; + + pr_debug("GTDT: a Watchdog GT structure(0x%llx/0x%llx gsi:%u flags:0x%x)\n", + rf_base_phy, cf_base_phy, gsi, flags); + + if (!(rf_base_phy && cf_base_phy && gsi)) { + pr_err("GTDT: failed geting the device info.\n"); + return -EINVAL; + } + + trigger = (flags & ACPI_GTDT_INTERRUPT_MODE) ? ACPI_EDGE_SENSITIVE + : ACPI_LEVEL_SENSITIVE; + polarity = (flags & ACPI_GTDT_INTERRUPT_POLARITY) ? ACPI_ACTIVE_LOW + : ACPI_ACTIVE_HIGH; + irq = acpi_register_gsi(NULL, gsi, trigger, polarity); + if (irq < 0) { + pr_err("GTDT: failed to register GSI of the Watchdog GT.\n"); + return -EINVAL; + } + + /* + * Add a platform device named "sbsa-gwdt" to match the platform driver. + * "sbsa-gwdt": SBSA(Server Base System Architecture) Generic Watchdog + * The platform driver can get device info below by matching this name. + */ + pdev = platform_device_alloc("sbsa-gwdt", index); + if (!pdev) + goto err_unregister_gsi; + + res = kcalloc(3, sizeof(*res), GFP_KERNEL); + if (!res) + goto err_device_put; + + /* + * According to SBSA specification the size of refresh and control + * frames of SBSA Generic Watchdog is SZ_4K(Offset 0x000 – 0xFFF). + */ + res[0].start = rf_base_phy; + res[0].end = rf_base_phy + SZ_4K - 1; + res[0].name = "refresh"; + res[0].flags = IORESOURCE_MEM; + + res[1].start = cf_base_phy; + res[1].end = cf_base_phy + SZ_4K - 1; + res[1].name = "control"; + res[1].flags = IORESOURCE_MEM; + + res[2].start = irq; + res[2].end = res[2].start; + res[2].name = "ws0"; + res[2].flags = IORESOURCE_IRQ; + + err = platform_device_add_resources(pdev, res, 3); + if (err) + goto err_free_res; + + err = platform_device_add(pdev); + if (err) + goto err_free_res; + + return 0; + +err_free_res: + kfree(res); +err_device_put: + platform_device_put(pdev); +err_unregister_gsi: + acpi_unregister_gsi(gsi); + + return err; +} + +/* Initialize SBSA generic Watchdog platform device info from GTDT */ +static int __init acpi_gtdt_sbsa_gwdt_init(struct acpi_table_header *table) +{ + struct acpi_table_gtdt *gtdt; + struct acpi_gtdt_header *header; + void *gtdt_subtable; + int i, gwdt_index; + int ret = 0; + + if (table->revision < 2) { + pr_warn("GTDT: Revision:%d doesn't support Platform Timers.\n", + table->revision); + return 0; + } + + gtdt = container_of(table, struct acpi_table_gtdt, header); + if (!gtdt->platform_timer_count) { + pr_info("GTDT: No Platform Timer structures.\n"); + return 0; + } + + gtdt_subtable = (void *)gtdt + gtdt->platform_timer_offset; + + for (i = 0, gwdt_index = 0; i < gtdt->platform_timer_count; i++) { + if (gtdt_subtable > (void *)table + table->length) { + pr_err("GTDT: subtable pointer overflows, bad table\n"); + return -EINVAL; + } + header = (struct acpi_gtdt_header *)gtdt_subtable; + if (header->type == ACPI_GTDT_TYPE_WATCHDOG) { + ret = acpi_gtdt_import_sbsa_gwdt(gtdt_subtable, + gwdt_index); + if (ret) + pr_err("GTDT: failed to import subtable %d\n", + i); + else + gwdt_index++; + } + gtdt_subtable += header->length; + } + + return ret; +} + +/* Initialize the SBSA Generic Watchdog presented in GTDT */ +static int __init acpi_gtdt_init(void) +{ + if (acpi_disabled) + return 0; + + return acpi_table_parse(ACPI_SIG_GTDT, acpi_gtdt_sbsa_gwdt_init); +} + +arch_initcall(acpi_gtdt_init);