| Message ID | 1458770712-10880-8-git-send-email-mmcclint@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On 03/23, Matthew McClintock wrote: > @@ -202,13 +238,6 @@ static int qcom_wdt_remove(struct platform_device *pdev) > return 0; > } > > -static const struct of_device_id qcom_wdt_of_table[] = { > - { .compatible = "qcom,kpss-timer" }, > - { .compatible = "qcom,scss-timer" }, > - { }, > -}; > -MODULE_DEVICE_TABLE(of, qcom_wdt_of_table); > - Leave this here and use of_device_get_match_data() in probe instead.
On Wed, Mar 23, 2016 at 05:05:02PM -0500, Matthew McClintock wrote: > Commit 0dfd582e026a ("watchdog: qcom: use timer devicetree binding") moved > to use the watchdog as a subset timer register block. Some devices have the > watchdog completely standalone with slightly different register offsets as > well so let's account for the differences here. > > Signed-off-by: Matthew McClintock <mmcclint@codeaurora.org> > --- > drivers/watchdog/qcom-wdt.c | 69 ++++++++++++++++++++++++++++++++------------- > 1 file changed, 49 insertions(+), 20 deletions(-) > > diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c > index 20563cc..e46f18d 100644 > --- a/drivers/watchdog/qcom-wdt.c > +++ b/drivers/watchdog/qcom-wdt.c > @@ -19,17 +19,37 @@ > #include <linux/platform_device.h> > #include <linux/watchdog.h> > > -#define WDT_RST 0x38 > -#define WDT_EN 0x40 > -#define WDT_BITE_TIME 0x5C > +enum wdt_reg { > + WDT_RST, > + WDT_EN, > + WDT_BITE_TIME, > +}; > + > +static const u32 reg_offset_data_apcs_tmr[] = { > + [WDT_RST] = 0x38, > + [WDT_EN] = 0x40, > + [WDT_BITE_TIME] = 0x5C, > +}; > + > +static const u32 reg_offset_data_kpss[] = { > + [WDT_RST] = 0x4, > + [WDT_EN] = 0x8, Does this work ? In the datasheet I have in front of me (APQ8064), the watchdog at this address uses different bits. At address 0x40 (eg GSS_A5_APCS_WDT0_EN), bit 0 is the enable bit, and bit 1 enables interrupts. At address 0x08 (eg LPASS_QDSP6SS_WDOG_UNMASKED_INT_EN), bit 0 enables interrupts and bit 1 is undefined. Or does "qcom,kpss-standalone" refer to some other watchdog ? Thanks, Guenter > + [WDT_BITE_TIME] = 0x14, > +}; > > struct qcom_wdt { > struct watchdog_device wdd; > struct clk *clk; > unsigned long rate; > void __iomem *base; > + const u32 *layout; > }; > > +static void __iomem *wdt_addr(struct qcom_wdt *wdt, enum wdt_reg reg) > +{ > + return wdt->base + wdt->layout[reg]; > +} > + > static inline > struct qcom_wdt *to_qcom_wdt(struct watchdog_device *wdd) > { > @@ -40,10 +60,10 @@ static int qcom_wdt_start(struct watchdog_device *wdd) > { > struct qcom_wdt *wdt = to_qcom_wdt(wdd); > > - writel(0, wdt->base + WDT_EN); > - writel(1, wdt->base + WDT_RST); > - writel(wdd->timeout * wdt->rate, wdt->base + WDT_BITE_TIME); > - writel(1, wdt->base + WDT_EN); > + writel(0, wdt_addr(wdt, WDT_EN)); > + writel(1, wdt_addr(wdt, WDT_RST)); > + writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME)); > + writel(1, wdt_addr(wdt, WDT_EN)); > return 0; > } > > @@ -51,7 +71,7 @@ static int qcom_wdt_stop(struct watchdog_device *wdd) > { > struct qcom_wdt *wdt = to_qcom_wdt(wdd); > > - writel(0, wdt->base + WDT_EN); > + writel(0, wdt_addr(wdt, WDT_EN)); > return 0; > } > > @@ -59,7 +79,7 @@ static int qcom_wdt_ping(struct watchdog_device *wdd) > { > struct qcom_wdt *wdt = to_qcom_wdt(wdd); > > - writel(1, wdt->base + WDT_RST); > + writel(1, wdt_addr(wdt, WDT_RST)); > return 0; > } > > @@ -82,10 +102,10 @@ static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action, > */ > timeout = 128 * wdt->rate / 1000; > > - writel(0, wdt->base + WDT_EN); > - writel(1, wdt->base + WDT_RST); > - writel(timeout, wdt->base + WDT_BITE_TIME); > - writel(1, wdt->base + WDT_EN); > + writel(0, wdt_addr(wdt, WDT_EN)); > + writel(1, wdt_addr(wdt, WDT_RST)); > + writel(timeout, wdt_addr(wdt, WDT_BITE_TIME)); > + writel(1, wdt_addr(wdt, WDT_EN)); > > /* > * Actually make sure the above sequence hits hardware before sleeping. > @@ -112,14 +132,29 @@ static const struct watchdog_info qcom_wdt_info = { > .identity = KBUILD_MODNAME, > }; > > +static const struct of_device_id qcom_wdt_of_table[] = { > + { .compatible = "qcom,kpss-timer", .data = reg_offset_data_apcs_tmr }, > + { .compatible = "qcom,scss-timer", .data = reg_offset_data_apcs_tmr }, > + { .compatible = "qcom,kpss-standalone", .data = ®_offset_data_kpss}, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, qcom_wdt_of_table); > + > static int qcom_wdt_probe(struct platform_device *pdev) > { > struct qcom_wdt *wdt; > struct resource *res; > struct device_node *np = pdev->dev.of_node; > + const struct of_device_id *match; > u32 percpu_offset; > int ret; > > + match = of_match_node(qcom_wdt_of_table, np); > + if (!match) { > + dev_err(&pdev->dev, "Unsupported QCOM WDT module\n"); > + return -ENODEV; > + } > + > wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); > if (!wdt) > return -ENOMEM; > @@ -170,6 +205,7 @@ static int qcom_wdt_probe(struct platform_device *pdev) > wdt->wdd.min_timeout = 1; > wdt->wdd.max_timeout = 0x10000000U / wdt->rate; > wdt->wdd.parent = &pdev->dev; > + wdt->layout = match->data; > > /* > * If 'timeout-sec' unspecified in devicetree, assume a 30 second > @@ -202,13 +238,6 @@ static int qcom_wdt_remove(struct platform_device *pdev) > return 0; > } > > -static const struct of_device_id qcom_wdt_of_table[] = { > - { .compatible = "qcom,kpss-timer" }, > - { .compatible = "qcom,scss-timer" }, > - { }, > -}; > -MODULE_DEVICE_TABLE(of, qcom_wdt_of_table); > - > static struct platform_driver qcom_watchdog_driver = { > .probe = qcom_wdt_probe, > .remove = qcom_wdt_remove, > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Mar 25, 2016, at 11:23 AM, Guenter Roeck <linux@roeck-us.net> wrote: > >> -#define WDT_RST 0x38 >> -#define WDT_EN 0x40 >> -#define WDT_BITE_TIME 0x5C >> +enum wdt_reg { >> + WDT_RST, >> + WDT_EN, >> + WDT_BITE_TIME, >> +}; >> + >> +static const u32 reg_offset_data_apcs_tmr[] = { >> + [WDT_RST] = 0x38, >> + [WDT_EN] = 0x40, >> + [WDT_BITE_TIME] = 0x5C, >> +}; >> + >> +static const u32 reg_offset_data_kpss[] = { >> + [WDT_RST] = 0x4, >> + [WDT_EN] = 0x8, > > Does this work ? In the datasheet I have in front of me (APQ8064), the watchdog > at this address uses different bits. At address 0x40 (eg GSS_A5_APCS_WDT0_EN), 0x40 is acps_tmr, and looks fine. > bit 0 is the enable bit, and bit 1 enables interrupts. At address 0x08 (eg > LPASS_QDSP6SS_WDOG_UNMASKED_INT_EN), bit 0 enables interrupts and bit 1 is > undefined. I honestly don’t see anything at 0x8 for either blocks that looks like this. For the new block bit 0 is enabling and bit 1 enabled interrupts. > Or does "qcom,kpss-standalone" refer to some other watchdog ? APQ8064 would be the apcs_tmr block variant which is unchanged. MSM8916 as well as IPQ4019 would use the new kpss variant. I went with block names I found internally here, but I will be the first to admit I am terrible at names. The old block name for APQ was CPU0_ACPS_TMR (where really the watchdog is a subset of a timer block), and on the IPQ4019 it’s called APCS_KPSS_WDT and it’s really just a watchdog block. I kept the same driver because the register’s currently in use were compatible. By the way, I tested this on an IPQ806x and IPQ4019 both new and old blocks. Let me know if you need more details. -M-- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" 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 28, 2016 at 11:55:28AM -0500, Matthew McClintock wrote: > > > On Mar 25, 2016, at 11:23 AM, Guenter Roeck <linux@roeck-us.net> wrote: > > > >> -#define WDT_RST 0x38 > >> -#define WDT_EN 0x40 > >> -#define WDT_BITE_TIME 0x5C > >> +enum wdt_reg { > >> + WDT_RST, > >> + WDT_EN, > >> + WDT_BITE_TIME, > >> +}; > >> + > >> +static const u32 reg_offset_data_apcs_tmr[] = { > >> + [WDT_RST] = 0x38, > >> + [WDT_EN] = 0x40, > >> + [WDT_BITE_TIME] = 0x5C, > >> +}; > >> + > >> +static const u32 reg_offset_data_kpss[] = { > >> + [WDT_RST] = 0x4, > >> + [WDT_EN] = 0x8, > > > > Does this work ? In the datasheet I have in front of me (APQ8064), the watchdog > > at this address uses different bits. At address 0x40 (eg GSS_A5_APCS_WDT0_EN), > > 0x40 is acps_tmr, and looks fine. > > > bit 0 is the enable bit, and bit 1 enables interrupts. At address 0x08 (eg > > LPASS_QDSP6SS_WDOG_UNMASKED_INT_EN), bit 0 enables interrupts and bit 1 is > > undefined. > > I honestly don’t see anything at 0x8 for either blocks that looks like this. For the new block bit 0 is enabling and bit 1 enabled interrupts. > That is from the APQ8064 datasheet. > > Or does "qcom,kpss-standalone" refer to some other watchdog ? > > APQ8064 would be the apcs_tmr block variant which is unchanged. MSM8916 as well as IPQ4019 would use the new kpss variant. > Unfortunately I don't have access to those datasheets. > I went with block names I found internally here, but I will be the first to admit I am terrible at names. The old block name for APQ was CPU0_ACPS_TMR (where really the watchdog is a subset of a timer block), and on the IPQ4019 it’s called APCS_KPSS_WDT and it’s really just a watchdog block. > > I kept the same driver because the register’s currently in use were compatible. By the way, I tested this on an IPQ806x and IPQ4019 both new and old blocks. > The property name should probably be something like 'qcom,kpss-wdt' (or 'qcom,kpss-watchdog' ?), possibly in addition to 'qcom,kpss-ipq4019-wdt' and 'qcom,kpss-msm8916-wdt'. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mar 28, 2016, at 1:13 PM, Guenter Roeck <linux@roeck-us.net> wrote: > >>> bit 0 is the enable bit, and bit 1 enables interrupts. At address 0x08 (eg >>> LPASS_QDSP6SS_WDOG_UNMASKED_INT_EN), bit 0 enables interrupts and bit 1 is >>> undefined. >> >> I honestly don’t see anything at 0x8 for either blocks that looks like this. For the new block bit 0 is enabling and bit 1 enabled interrupts. >> > That is from the APQ8064 datasheet. So taken from the timer offset 0x0208A000 I just have a generic counter register CPU0_APCS_GPT0_CNT at 0x8 What doc are you looking at? -M-- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" 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 28, 2016 at 03:40:58PM -0500, Matthew McClintock wrote: > On Mar 28, 2016, at 1:13 PM, Guenter Roeck <linux@roeck-us.net> wrote: > > > >>> bit 0 is the enable bit, and bit 1 enables interrupts. At address 0x08 (eg > >>> LPASS_QDSP6SS_WDOG_UNMASKED_INT_EN), bit 0 enables interrupts and bit 1 is > >>> undefined. > >> > >> I honestly don’t see anything at 0x8 for either blocks that looks like this. For the new block bit 0 is enabling and bit 1 enabled interrupts. > >> > > That is from the APQ8064 datasheet. > > So taken from the timer offset 0x0208A000 I just have a generic counter register CPU0_APCS_GPT0_CNT at 0x8 > > What doc are you looking at? > "Qualcomm Snapdragon 600 Processor APQ8064 Hardware Register Description" It is available for download from the Qualcomm web site. See chapter 12.10.3, "Watchdog timer registers". The register block is at 0x28882000. Registers are almost the same, except for the offset and the definition of the bits in the enable register. LPASS is "Low Power Audio Subsystem". Maybe it has its own watchdog. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mar 28, 2016, at 4:56 PM, Guenter Roeck <linux@roeck-us.net> wrote: > >> So taken from the timer offset 0x0208A000 I just have a generic counter register CPU0_APCS_GPT0_CNT at 0x8 >> >> What doc are you looking at? >> > "Qualcomm Snapdragon 600 Processor APQ8064 Hardware Register Description" > > It is available for download from the Qualcomm web site. > > See chapter 12.10.3, "Watchdog timer registers". The register block is at > 0x28882000. Registers are almost the same, except for the offset and the > definition of the bits in the enable register. > > LPASS is "Low Power Audio Subsystem". Maybe it has its own watchdog. This block is here: 11.15 KPSS CPU0 Timer Registers (0x0208A000 CPU0_APCS_TMR_BASE) -M-- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" 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/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c index 20563cc..e46f18d 100644 --- a/drivers/watchdog/qcom-wdt.c +++ b/drivers/watchdog/qcom-wdt.c @@ -19,17 +19,37 @@ #include <linux/platform_device.h> #include <linux/watchdog.h> -#define WDT_RST 0x38 -#define WDT_EN 0x40 -#define WDT_BITE_TIME 0x5C +enum wdt_reg { + WDT_RST, + WDT_EN, + WDT_BITE_TIME, +}; + +static const u32 reg_offset_data_apcs_tmr[] = { + [WDT_RST] = 0x38, + [WDT_EN] = 0x40, + [WDT_BITE_TIME] = 0x5C, +}; + +static const u32 reg_offset_data_kpss[] = { + [WDT_RST] = 0x4, + [WDT_EN] = 0x8, + [WDT_BITE_TIME] = 0x14, +}; struct qcom_wdt { struct watchdog_device wdd; struct clk *clk; unsigned long rate; void __iomem *base; + const u32 *layout; }; +static void __iomem *wdt_addr(struct qcom_wdt *wdt, enum wdt_reg reg) +{ + return wdt->base + wdt->layout[reg]; +} + static inline struct qcom_wdt *to_qcom_wdt(struct watchdog_device *wdd) { @@ -40,10 +60,10 @@ static int qcom_wdt_start(struct watchdog_device *wdd) { struct qcom_wdt *wdt = to_qcom_wdt(wdd); - writel(0, wdt->base + WDT_EN); - writel(1, wdt->base + WDT_RST); - writel(wdd->timeout * wdt->rate, wdt->base + WDT_BITE_TIME); - writel(1, wdt->base + WDT_EN); + writel(0, wdt_addr(wdt, WDT_EN)); + writel(1, wdt_addr(wdt, WDT_RST)); + writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME)); + writel(1, wdt_addr(wdt, WDT_EN)); return 0; } @@ -51,7 +71,7 @@ static int qcom_wdt_stop(struct watchdog_device *wdd) { struct qcom_wdt *wdt = to_qcom_wdt(wdd); - writel(0, wdt->base + WDT_EN); + writel(0, wdt_addr(wdt, WDT_EN)); return 0; } @@ -59,7 +79,7 @@ static int qcom_wdt_ping(struct watchdog_device *wdd) { struct qcom_wdt *wdt = to_qcom_wdt(wdd); - writel(1, wdt->base + WDT_RST); + writel(1, wdt_addr(wdt, WDT_RST)); return 0; } @@ -82,10 +102,10 @@ static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action, */ timeout = 128 * wdt->rate / 1000; - writel(0, wdt->base + WDT_EN); - writel(1, wdt->base + WDT_RST); - writel(timeout, wdt->base + WDT_BITE_TIME); - writel(1, wdt->base + WDT_EN); + writel(0, wdt_addr(wdt, WDT_EN)); + writel(1, wdt_addr(wdt, WDT_RST)); + writel(timeout, wdt_addr(wdt, WDT_BITE_TIME)); + writel(1, wdt_addr(wdt, WDT_EN)); /* * Actually make sure the above sequence hits hardware before sleeping. @@ -112,14 +132,29 @@ static const struct watchdog_info qcom_wdt_info = { .identity = KBUILD_MODNAME, }; +static const struct of_device_id qcom_wdt_of_table[] = { + { .compatible = "qcom,kpss-timer", .data = reg_offset_data_apcs_tmr }, + { .compatible = "qcom,scss-timer", .data = reg_offset_data_apcs_tmr }, + { .compatible = "qcom,kpss-standalone", .data = ®_offset_data_kpss}, + { }, +}; +MODULE_DEVICE_TABLE(of, qcom_wdt_of_table); + static int qcom_wdt_probe(struct platform_device *pdev) { struct qcom_wdt *wdt; struct resource *res; struct device_node *np = pdev->dev.of_node; + const struct of_device_id *match; u32 percpu_offset; int ret; + match = of_match_node(qcom_wdt_of_table, np); + if (!match) { + dev_err(&pdev->dev, "Unsupported QCOM WDT module\n"); + return -ENODEV; + } + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); if (!wdt) return -ENOMEM; @@ -170,6 +205,7 @@ static int qcom_wdt_probe(struct platform_device *pdev) wdt->wdd.min_timeout = 1; wdt->wdd.max_timeout = 0x10000000U / wdt->rate; wdt->wdd.parent = &pdev->dev; + wdt->layout = match->data; /* * If 'timeout-sec' unspecified in devicetree, assume a 30 second @@ -202,13 +238,6 @@ static int qcom_wdt_remove(struct platform_device *pdev) return 0; } -static const struct of_device_id qcom_wdt_of_table[] = { - { .compatible = "qcom,kpss-timer" }, - { .compatible = "qcom,scss-timer" }, - { }, -}; -MODULE_DEVICE_TABLE(of, qcom_wdt_of_table); - static struct platform_driver qcom_watchdog_driver = { .probe = qcom_wdt_probe, .remove = qcom_wdt_remove,
Commit 0dfd582e026a ("watchdog: qcom: use timer devicetree binding") moved to use the watchdog as a subset timer register block. Some devices have the watchdog completely standalone with slightly different register offsets as well so let's account for the differences here. Signed-off-by: Matthew McClintock <mmcclint@codeaurora.org> --- drivers/watchdog/qcom-wdt.c | 69 ++++++++++++++++++++++++++++++++------------- 1 file changed, 49 insertions(+), 20 deletions(-)