Hi Leela,
Please see my comments below.
On Tuesday 17 of September 2013 16:13:42 Leela Krishna Amudala wrote:
> This patch parses the watchdog node to read pmu wdt sys registers
> addresses and do mask/unmask enable/disable of WDT in probe and s2r
> scenarios.
>
> Reviewed-by: Doug Anderson <dianders@xxxxxxxxxxxx>
> Signed-off-by: Leela Krishna Amudala <l.krishna@xxxxxxxxxxx>
> ---
> .../devicetree/bindings/watchdog/samsung-wdt.txt | 14 ++++-
> drivers/watchdog/s3c2410_wdt.c | 56
> ++++++++++++++++++++ 2 files changed, 69 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
> b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt index
> 2aa486c..4c798e3 100644
> --- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
> @@ -7,8 +7,20 @@ occurred.
> Required properties:
> - compatible : should be "samsung,s3c2410-wdt"
Since the WDT block of Exynos 5420 needs some extra configuration in PMU
registers, it is no longer compatible with samsung.s3c2410-wdt. Please
introduce separate compatible ("samsung,exynos5420-wdt") and make the
driver handle the additional configuration only if running on a device with
this compatible value.
I'd suggest introducing quirk system to the driver and adding a
NEEDS_PMU_CONFIG quirk selected by DT match entry with "samsung,exynos5420-
wdt" compatible.
> - reg : base physical address of the controller and length of memory
> mapped - region.
> + region and the optional (addresses and length of memory mapped regions
> + of) PMU registers for masking/unmasking WDT.
> - interrupts : interrupt number to the cpu.
>
> Optional properties:
> - timeout-sec : contains the watchdog timeout in seconds.
> +- reset-mask-bit: bit number in the PMU registers to program mask/unmask
> WDT. +
I believe this is mandatory on Exynos 5420 and unused on previous SoCs. It
should be handled depending on compatible value.
>
[...]
> +static void s3c2410wdt_mask_and_disable_reset(int mask, struct
> s3c2410_wdt *wdt) +{
> + unsigned int value;
> +
> + if (IS_ERR(wdt->pmu_disable_reg) || IS_ERR(wdt->pmu_mask_reset_reg)
> + || (wdt->pmu_mask_bit < 0))
> + return;
This function could be called only if respective quirk is active and the
check above could be dropped.
> +
> + if (mask) {
> + value = readl(wdt->pmu_disable_reg);
> + value |= (1 << wdt->pmu_mask_bit);
> + writel(value, wdt->pmu_disable_reg);
> +
> + value = readl(wdt->pmu_mask_reset_reg);
> + value |= (1 << wdt->pmu_mask_bit);
> + writel(value, wdt->pmu_mask_reset_reg);
> + } else {
> + value = readl(wdt->pmu_disable_reg);
> + value &= ~(1 << wdt->pmu_mask_bit);
> + writel(value, wdt->pmu_disable_reg);
> +
> + value = readl(wdt->pmu_mask_reset_reg);
> + value &= ~(1 << wdt->pmu_mask_bit);
> + writel(value, wdt->pmu_mask_reset_reg);
> + }
This can be greatly simplified by moving readl and writel outside the
conditional block, i.e.
u32 disable, mask_reset;
disable = readl(wdt->pmu_disable_reg);
mask_reset = readl(wdt->pmu_mask_reset_reg);
if (mask) {
disable |= (1 << wdt->pmu_mask_bit);
mask_reset |= (1 << wdt->pmu_mask_bit);
} else {
disable &= ~(1 << wdt->pmu_mask_bit);
mask_reset &= ~(1 << wdt->pmu_mask_bit);
}
writel(disable, wdt->pmu_disable_reg);
writel(mask_reset, wdt->pmu_mask_reset_reg);
> +}
> +
> static int s3c2410wdt_keepalive(struct watchdog_device *wdd)
> {
> struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
> @@ -341,6 +371,8 @@ static int s3c2410wdt_probe(struct platform_device
> *pdev) unsigned int wtcon;
> int started = 0;
> int ret;
> + struct resource *res;
> + unsigned int mask_bit;
>
> DBG("%s: probe=%p\n", __func__, pdev);
>
> @@ -369,6 +401,25 @@ static int s3c2410wdt_probe(struct platform_device
> *pdev) goto err;
> }
The code added below could be handled conditionally and missing PMU
registers could simply trigger an error.
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + wdt->pmu_disable_reg = devm_ioremap_resource(&pdev->dev, res);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> + wdt->pmu_mask_reset_reg = devm_ioremap_resource(&pdev->dev, res);
> +
> + if (!IS_ERR(wdt->pmu_disable_reg) && !IS_ERR(wdt->pmu_mask_reset_reg))
> { + if (pdev->dev.of_node) {
> + if (of_property_read_u32(pdev->dev.of_node,
> + "reset-mask-bit",
> + &mask_bit)) {
> + dev_warn(dev, "reset-mask-bit not specified\n");
> + wdt->pmu_mask_bit = -EINVAL;
> + } else {
> + wdt->pmu_mask_bit = mask_bit;
> + }
> + }
> + }
> +
> DBG("probe: mapped reg_base=%p\n", wdt->reg_base);
>
> wdt->clock = devm_clk_get(dev, "watchdog");
> @@ -444,6 +495,7 @@ static int s3c2410wdt_probe(struct platform_device
> *pdev) (wtcon & S3C2410_WTCON_RSTEN) ? "en" : "dis",
> (wtcon & S3C2410_WTCON_INTEN) ? "en" : "dis");
>
> + s3c2410wdt_mask_and_disable_reset(0, wdt);
The call above (and further call to this function bellow) could happen
conditionally.
Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
