Hi Timur,
Thanks for your review, :-) comment inline below :
On 5 May 2015 at 05:25, Timur Tabi timur@codeaurora.org wrote:
On 05/04/2015 07:04 AM, fu.wei@linaro.org wrote:
From: Fu Wei fu.wei@linaro.org
Signed-off-by: Fu Wei fu.wei@linaro.org
Like others have said, you need to fix your patch descriptions.
Also, this patch should be 3/6. That is, before patch #4.
yes, will do so
arch/arm64/kernel/acpi.c | 154 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 154 insertions(+)
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 8b83955..833d21e 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,156 @@ void __init acpi_gic_init(void)
early_acpi_os_unmap_memory((char *)table, tbl_size);}
+static void __init *acpi_gtdt_subtable(struct acpi_table_gtdt *gtdt) +{
if (!gtdt->platform_timer_count)return NULL;return ((void *)gtdt + (gtdt->platform_timer_offset));+}
+static int __init acpi_gtdt_set_resource_irq_flags(u32 timer_flags) +{
int flags;switch (timer_flags &(ACPI_GTDT_INTERRUPT_MODE |ACPI_GTDT_INTERRUPT_POLARITY)) {
case (ACPI_GTDT_INTERRUPT_MODE | ACPI_GTDT_INTERRUPT_POLARITY):flags = IORESOURCE_IRQ_LOWEDGE | IORESOURCE_IRQ;pr_debug("GTDT: sbsa-gwdt irq flags is low edge");break;case ACPI_GTDT_INTERRUPT_MODE:flags = IORESOURCE_IRQ_HIGHEDGE | IORESOURCE_IRQ;pr_debug("GTDT: sbsa-gwdt irq flags is high edge");break;case ACPI_GTDT_INTERRUPT_POLARITY:flags = IORESOURCE_IRQ_LOWLEVEL | IORESOURCE_IRQ;pr_debug("GTDT: sbsa-gwdt irq flags is low level");break;default:flags = IORESOURCE_IRQ_HIGHLEVEL | IORESOURCE_IRQ;pr_debug("GTDT: sbsa-gwdt irq flags is high level");}pr_debug("GTDT: sbsa-gwdt irq flags:%d\n", flags);return flags;Blank line before the 'return'
yes, will do
+}
+static int __init acpi_gtdt_import_sbsa_gwdt(struct acpi_gtdt_header *table,
int index)+{
struct acpi_gtdt_watchdog *watchdog;int irq, gsi;int err;u32 flags;void *rf_base_phy, *cf_base_phy;struct platform_device *pdev;struct resource *res;watchdog = container_of(table, struct acpi_gtdt_watchdog, header);/* get SBSA Generic Watchdog device information from GTDT */rf_base_phy = (void *)watchdog->refresh_frame_address;cf_base_phy = (void *)watchdog->control_frame_address;irq = watchdog->timer_interrupt;flags = watchdog->timer_flags;pr_info("GTDT: found a device @ 0x%p/0x%p irq:%d flags:%x\n",rf_base_phy, cf_base_phy, irq, flags);if (!rf_base_phy || !cf_base_phy || !irq) {pr_err("GTDT: fail to get the device info.\n");return -EINVAL;}gsi = map_generic_timer_interrupt(irq, flags);How about using acpi_register_gsi()? That way, you won't need to export map_generic_timer_interrupt().
will try to improve this , ;thanks
pdev = platform_device_alloc("sbsa-gwdt", index);if (!pdev)return -ENOMEM;res = kcalloc(3, sizeof(*res), GFP_KERNEL);if (!res)goto err_free_device;res[0].start = (resource_size_t)rf_base_phy;res[0].end = (resource_size_t)(rf_base_phy + SZ_64K - 1);res[0].name = "refresh_frame";res[0].flags = IORESOURCE_MEM;res[1].start = (resource_size_t)cf_base_phy;res[1].end = (resource_size_t)(cf_base_phy + SZ_64K - 1);res[1].name = "control_frame";res[1].flags = IORESOURCE_MEM;res[2].start = res[2].end = gsi;res[2].name = "ws0";res[2].flags = acpi_gtdt_set_resource_irq_flags(flags);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_free_device:
platform_device_put(pdev);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;int i, timer_count, gwdt_count = 0;struct acpi_gtdt_header *header;void __iomem *gtdt_subtable;if (table->revision < 2) {pr_warn("GTDT: Revision:%d doesn't support SBSA GWDT.\n",table->revision);return -ENODEV;}gtdt = container_of(table, struct acpi_table_gtdt, header);timer_count = gtdt->platform_timer_count;gtdt_subtable = acpi_gtdt_subtable(gtdt);Just merge the contents of acpi_gtdt_subtable() into here. No one else will call acpi_gtdt_subtable().
I may do this in my next patchset
if (!gtdt_subtable) {
pr_warn("GTDT: No Platform Timer structures!\n");return -EINVAL;}for (i = 0; i < timer_count; i++) {header = (struct acpi_gtdt_header *)gtdt_subtable;if (header->type == ACPI_GTDT_TYPE_WATCHDOG) {if (acpi_gtdt_import_sbsa_gwdt(header, i))pr_err("GTDT: adding sbsa-gwdt%d ""platform device fail! subtable:%d\n",
Please no exclamation marks in messages. Try to make your messages like normal English. How about this:
pr_err("GTDT: failed trying to create platform sbsa-gwdt%d\n",gwdt_count);
Good idea , thanks for correcting my English, :-)
However, I'm not crazy about gwdt_count because you never pass that value to acpi_gtdt_import_sbsa_gwdt(). Instead you pass 'i', which is different value.
Sorry, that is a bug indeed.
gwdt_count, i);
gwdt_count++;}gtdt_subtable += header->length;You should add a safety check on the value of gtdt_subtable to make sure it doesn't go past the end of the buffer. timer_count might be corrupted.
yes, you are right!! will do
}
return 0;+}
+/* 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);+}
I think instead of creating this function, you should move it into arch_timer_acpi_init().
I will discuss this with author of arch_timer_acpi_init(), see if that makes sense, but I will do something more there in the following patch, for now , I may keep this code here. but your idea is good :-)
-- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.