Re: [PATCH v2 4/7] watchdog: orion: Add Armada 375/380 SoC support

[Guenter Roeck <linux@roeck-us.net>]

On 03/04/2014 08:07 AM, Ezequiel Garcia wrote:
> This commit adds support for the Armada 375 and Armada 380 SoCs.
>
> This SoC variant has a second RSTOUT register, in addition to the already
> existent, which is shared with the system-controller. To handle this RSTOUT,
> we introduce a new MMIO register 'rstout_mask' to be required on
> 'armada-{375,380}-watchdog' new compatible string.
>
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>   drivers/watchdog/orion_wdt.c | 98 ++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 98 insertions(+)
>
> diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
> index 8fb8e65..e567655 100644
> --- a/drivers/watchdog/orion_wdt.c
> +++ b/drivers/watchdog/orion_wdt.c
> @@ -56,6 +56,7 @@ struct orion_watchdog_data {
>   	int wdt_counter_offset;
>   	int wdt_enable_bit;
>   	int rstout_enable_bit;
> +	int rstout_mask_bit;
>   	int (*clock_init)(struct platform_device *,
>   			  struct orion_watchdog *);
>   	int (*enabled)(struct orion_watchdog *);
> @@ -67,6 +68,7 @@ struct orion_watchdog {
>   	struct watchdog_device wdt;
>   	void __iomem *reg;
>   	void __iomem *rstout;
> +	void __iomem *rstout_mask;
>   	unsigned long clk_rate;
>   	struct clk *clk;
>   	const struct orion_watchdog_data *data;
> @@ -145,6 +147,27 @@ static int orion_wdt_ping(struct watchdog_device *wdt_dev)
>   	return 0;
>   }
>
> +static int armada375_start(struct watchdog_device *wdt_dev)
> +{
> +	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
> +
> +	/* Set watchdog duration */
> +	writel(dev->clk_rate * wdt_dev->timeout,
> +	       dev->reg + dev->data->wdt_counter_offset);
> +
> +	/* Clear the watchdog expiration bit */
> +	atomic_io_modify(dev->reg + TIMER_A370_STATUS, WDT_A370_EXPIRED, 0);
> +
> +	/* Enable watchdog timer */
> +	atomic_io_modify(dev->reg + TIMER_CTRL, dev->data->wdt_enable_bit,
> +						dev->data->wdt_enable_bit);
> +
> +	atomic_io_modify(dev->rstout, dev->data->rstout_enable_bit,
> +				      dev->data->rstout_enable_bit);
> +	atomic_io_modify(dev->rstout_mask, dev->data->rstout_mask_bit, 0);
> +	return 0;
> +}
> +
>   static int armada370_start(struct watchdog_device *wdt_dev)
>   {
>   	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
> @@ -205,6 +228,21 @@ static int orion_stop(struct watchdog_device *wdt_dev)
>   	return 0;
>   }
>
> +static int armada375_stop(struct watchdog_device *wdt_dev)
> +{
> +	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
> +
> +	/* Disable reset on watchdog */
> +	atomic_io_modify(dev->rstout_mask, dev->data->rstout_mask_bit,
> +					   dev->data->rstout_mask_bit);
> +	atomic_io_modify(dev->rstout, dev->data->rstout_enable_bit, 0);
> +
> +	/* Disable watchdog timer */
> +	atomic_io_modify(dev->reg + TIMER_CTRL, dev->data->wdt_enable_bit, 0);
> +
> +	return 0;
> +}
> +
>   static int armada370_stop(struct watchdog_device *wdt_dev)
>   {
>   	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
> @@ -235,6 +273,17 @@ static int orion_enabled(struct orion_watchdog *dev)
>   	return enabled && running;
>   }
>
> +static int armada375_enabled(struct orion_watchdog *dev)
> +{
> +	bool masked, enabled, running;
> +
> +	masked = readl(dev->rstout_mask) & dev->data->rstout_mask_bit;
> +	enabled = readl(dev->rstout) & dev->data->rstout_enable_bit;
> +	running = readl(dev->reg + TIMER_CTRL) & dev->data->wdt_enable_bit;
> +
> +	return !masked && enabled && running;
> +}
> +
>   static int orion_wdt_enabled(struct watchdog_device *wdt_dev)
>   {
>   	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
> @@ -328,6 +377,28 @@ static const struct orion_watchdog_data armadaxp_data = {
>   	.stop = armada370_stop,
>   };
>
> +static const struct orion_watchdog_data armada375_data = {
> +	.rstout_enable_bit = BIT(8),
> +	.rstout_mask_bit = BIT(10),
> +	.wdt_enable_bit = BIT(8),
> +	.wdt_counter_offset = 0x34,
> +	.clock_init = armada370_wdt_clock_init,
> +	.enabled = armada375_enabled,
> +	.start = armada375_start,
> +	.stop = armada375_stop,
> +};
> +
> +static const struct orion_watchdog_data armada380_data = {
> +	.rstout_enable_bit = BIT(8),
> +	.rstout_mask_bit = BIT(10),
> +	.wdt_enable_bit = BIT(8),
> +	.wdt_counter_offset = 0x34,
> +	.clock_init = armadaxp_wdt_clock_init,
> +	.enabled = armada375_enabled,
> +	.start = armada375_start,
> +	.stop = armada375_stop,
> +};
> +
>   static const struct of_device_id orion_wdt_of_match_table[] = {
>   	{
>   		.compatible = "marvell,orion-wdt",
> @@ -341,6 +412,14 @@ static const struct of_device_id orion_wdt_of_match_table[] = {
>   		.compatible = "marvell,armada-xp-wdt",
>   		.data = &armadaxp_data,
>   	},
> +	{
> +		.compatible = "marvell,armada-375-wdt",
> +		.data = &armada375_data,
> +	},
> +	{
> +		.compatible = "marvell,armada-380-wdt",
> +		.data = &armada380_data,
> +	},
>   	{},
>   };
>   MODULE_DEVICE_TABLE(of, orion_wdt_of_match_table);
> @@ -396,6 +475,25 @@ static int orion_wdt_probe(struct platform_device *pdev)
>   		if (!dev->rstout)
>   			return -ENOMEM;
>
> +	} else if (of_device_is_compatible(node, "marvell,armada-375-wdt") ||
> +		   of_device_is_compatible(node, "marvell,armada-380-wdt")) {
> +
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 1);

Looks like each supported watchdog needs this call. Might as well do it earlier
and just once, unless you have a good reason for doing it this way.
To me this just looks like a lot of code replication.

It might also possibly make sense to move all the memory initializations
into a separate function; the probe function gets a bit large.

> +		if (!res)
> +			return -ENODEV;
> +		dev->rstout = devm_ioremap(&pdev->dev, res->start,
> +					   resource_size(res));
> +		if (!dev->rstout)
> +			return -ENOMEM;
> +
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> +		if (!res)
> +			return -ENODEV;
> +		dev->rstout_mask = devm_ioremap(&pdev->dev, res->start,
> +						resource_size(res));

devm_ioremap_resource() is better here.

Guenter

> +		if (!dev->rstout_mask)
> +			return -ENOMEM;
> +
>   	} else {
>   		return -ENODEV;
>   	}
>