diff mbox

[v3,2/3] watchdog: orion: Use the reference clock on Armada 375 SoC

Message ID 1415107293-19258-3-git-send-email-ezequiel.garcia@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ezequiel Garcia Nov. 4, 2014, 1:21 p.m. UTC 
The 25 MHz reference clock has better stability so its use is prefered over the
core clock. Change the Armada 375 clock initialization to use this reference
clock. To ensure the driver is compatible with an old devicetree, also provide
a fallback path which will silently return to the previous behavior.

While here, add the clock specification to the binding documentation.

Acked-by: Jason Cooper <jason@lakedaemon.net>
Reviewed-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Tested-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 .../devicetree/bindings/watchdog/marvel.txt        | 13 +++++++
 drivers/watchdog/orion_wdt.c                       | 42 +++++++++++++++++++++-
 2 files changed, 54 insertions(+), 1 deletion(-)

Comments

Gregory CLEMENT Nov. 10, 2014, 2:41 p.m. UTC | #1 
Hi Ezequiel,

[...]

> +static int armada375_wdt_clock_init(struct platform_device *pdev,
> +				    struct orion_watchdog *dev)
> +{
> +	int ret;
> +
> +	dev->clk = of_clk_get_by_name(pdev->dev.of_node, "fixed");
> +	if (!IS_ERR(dev->clk)) {
> +		ret = clk_prepare_enable(dev->clk);
> +		if (ret) {
> +			clk_put(dev->clk);
> +			return ret;
> +		}
> +
> +		atomic_io_modify(dev->reg + TIMER_CTRL,
> +				WDT_AXP_FIXED_ENABLE_BIT,
> +				WDT_AXP_FIXED_ENABLE_BIT);
> +		dev->clk_rate = clk_get_rate(dev->clk);
> +
> +		return 0;
> +	}
> +
> +	/* Mandatory fallback for proper devicetree backward compatibility */
> +	dev->clk = clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(dev->clk))
> +		return PTR_ERR(dev->clk);
> +
> +	ret = clk_prepare_enable(dev->clk);
> +	if (ret) {
> +		clk_put(dev->clk);
> +		return ret;
> +	}
> +
> +	atomic_io_modify(dev->reg + TIMER_CTRL,
> +			WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT),
> +			WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT));
> +	dev->clk_rate = clk_get_rate(dev->clk) / WDT_A370_RATIO;
> +
> +	return 0;
> +}

Shouldn't be possible to do the following:

static int armada375_wdt_clock_init(struct platform_device *pdev,
				    struct orion_watchdog *dev)
{
	if (armadaxp_wdt_clock_init(pdev, dev)) {
		/* Mandatory fallback for proper devicetree backward compatibility */
		return armadaxp_wdt_clock_init(pdev, dev));
	}
	return 0;
}

Actually reusing the armadaxp_wdt_clock_init() function  was also suggested by Thomas
on your first version but I didn't find your answer about it.


Thanks,

Gregory


> +
>  static int armadaxp_wdt_clock_init(struct platform_device *pdev,
>  				   struct orion_watchdog *dev)
>  {
> @@ -394,7 +434,7 @@ static const struct orion_watchdog_data armada375_data = {
>  	.rstout_mask_bit = BIT(10),
>  	.wdt_enable_bit = BIT(8),
>  	.wdt_counter_offset = 0x34,
> -	.clock_init = armada370_wdt_clock_init,
> +	.clock_init = armada375_wdt_clock_init,
>  	.enabled = armada375_enabled,
>  	.start = armada375_start,
>  	.stop = armada375_stop,
>
Ezequiel Garcia Nov. 10, 2014, 2:47 p.m. UTC | #2 
On 11/10/2014 11:41 AM, Gregory CLEMENT wrote:
> Hi Ezequiel,
> 
> [...]
> 
>> +static int armada375_wdt_clock_init(struct platform_device *pdev,
>> +				    struct orion_watchdog *dev)
>> +{
>> +	int ret;
>> +
>> +	dev->clk = of_clk_get_by_name(pdev->dev.of_node, "fixed");
>> +	if (!IS_ERR(dev->clk)) {
>> +		ret = clk_prepare_enable(dev->clk);
>> +		if (ret) {
>> +			clk_put(dev->clk);
>> +			return ret;
>> +		}
>> +
>> +		atomic_io_modify(dev->reg + TIMER_CTRL,
>> +				WDT_AXP_FIXED_ENABLE_BIT,
>> +				WDT_AXP_FIXED_ENABLE_BIT);
>> +		dev->clk_rate = clk_get_rate(dev->clk);
>> +
>> +		return 0;
>> +	}
>> +
>> +	/* Mandatory fallback for proper devicetree backward compatibility */
>> +	dev->clk = clk_get(&pdev->dev, NULL);
>> +	if (IS_ERR(dev->clk))
>> +		return PTR_ERR(dev->clk);
>> +
>> +	ret = clk_prepare_enable(dev->clk);
>> +	if (ret) {
>> +		clk_put(dev->clk);
>> +		return ret;
>> +	}
>> +
>> +	atomic_io_modify(dev->reg + TIMER_CTRL,
>> +			WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT),
>> +			WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT));
>> +	dev->clk_rate = clk_get_rate(dev->clk) / WDT_A370_RATIO;
>> +
>> +	return 0;
>> +}
> 
> Shouldn't be possible to do the following:
> 
> static int armada375_wdt_clock_init(struct platform_device *pdev,
> 				    struct orion_watchdog *dev)
> {
> 	if (armadaxp_wdt_clock_init(pdev, dev)) {
> 		/* Mandatory fallback for proper devicetree backward compatibility */
> 		return armadaxp_wdt_clock_init(pdev, dev));

I guess you meant armada370_wdt_clock_init for the fallback?

> 	}
> 	return 0;
> }
> 
> Actually reusing the armadaxp_wdt_clock_init() function  was also suggested by Thomas
> on your first version but I didn't find your answer about it.
> 

I replied here to the same objection on the clocksource driver:
http://www.spinics.net/lists/linux-watchdog/msg05318.html

I found that it's a fragile practice, just to save a few lines of code.
Someone can go and change the 370/xp clock init, in some way that's
incompatible with 375.

I guess I'm being paranoid, but it's a way to keep the code robust and
we are only duplicating a few lines.
Gregory CLEMENT Nov. 10, 2014, 3:02 p.m. UTC | #3 
On 10/11/2014 15:47, Ezequiel Garcia wrote:
> On 11/10/2014 11:41 AM, Gregory CLEMENT wrote:
>> Hi Ezequiel,
>>
>> [...]
>>
>>> +static int armada375_wdt_clock_init(struct platform_device *pdev,
>>> +				    struct orion_watchdog *dev)
>>> +{
>>> +	int ret;
>>> +
>>> +	dev->clk = of_clk_get_by_name(pdev->dev.of_node, "fixed");
>>> +	if (!IS_ERR(dev->clk)) {
>>> +		ret = clk_prepare_enable(dev->clk);
>>> +		if (ret) {
>>> +			clk_put(dev->clk);
>>> +			return ret;
>>> +		}
>>> +
>>> +		atomic_io_modify(dev->reg + TIMER_CTRL,
>>> +				WDT_AXP_FIXED_ENABLE_BIT,
>>> +				WDT_AXP_FIXED_ENABLE_BIT);
>>> +		dev->clk_rate = clk_get_rate(dev->clk);
>>> +
>>> +		return 0;
>>> +	}
>>> +
>>> +	/* Mandatory fallback for proper devicetree backward compatibility */
>>> +	dev->clk = clk_get(&pdev->dev, NULL);
>>> +	if (IS_ERR(dev->clk))
>>> +		return PTR_ERR(dev->clk);
>>> +
>>> +	ret = clk_prepare_enable(dev->clk);
>>> +	if (ret) {
>>> +		clk_put(dev->clk);
>>> +		return ret;
>>> +	}
>>> +
>>> +	atomic_io_modify(dev->reg + TIMER_CTRL,
>>> +			WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT),
>>> +			WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT));
>>> +	dev->clk_rate = clk_get_rate(dev->clk) / WDT_A370_RATIO;
>>> +
>>> +	return 0;
>>> +}
>>
>> Shouldn't be possible to do the following:
>>
>> static int armada375_wdt_clock_init(struct platform_device *pdev,
>> 				    struct orion_watchdog *dev)
>> {
>> 	if (armadaxp_wdt_clock_init(pdev, dev)) {
>> 		/* Mandatory fallback for proper devicetree backward compatibility */
>> 		return armadaxp_wdt_clock_init(pdev, dev));
> 
> I guess you meant armada370_wdt_clock_init for the fallback?

yes wrong copy and paste

> 
>> 	}
>> 	return 0;
>> }
>>
>> Actually reusing the armadaxp_wdt_clock_init() function  was also suggested by Thomas
>> on your first version but I didn't find your answer about it.
>>
> 
> I replied here to the same objection on the clocksource driver:
> http://www.spinics.net/lists/linux-watchdog/msg05318.html

Thanks i didn't managed to find it.

> 
> I found that it's a fragile practice, just to save a few lines of code.

The purpose was not to save a few line of code but to make the code more
maintainable. If we need to fix something in the one of the aramda370 and
xp function then we also need to do the same in this function.

> Someone can go and change the 370/xp clock init, in some way that's
> incompatible with 375.
> 
> I guess I'm being paranoid, but it's a way to keep the code robust and
> we are only duplicating a few lines.

I see your point and I don't have a strong opinion on my suggestion so:


Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>


Thanks,

Gregory


>
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/watchdog/marvel.txt b/Documentation/devicetree/bindings/watchdog/marvel.txt
index 97223fd..858ed92 100644
--- a/Documentation/devicetree/bindings/watchdog/marvel.txt
+++ b/Documentation/devicetree/bindings/watchdog/marvel.txt
@@ -17,6 +17,18 @@  For "marvell,armada-375-wdt" and "marvell,armada-380-wdt":
 - reg		: A third entry is mandatory and should contain the
                   shared mask/unmask RSTOUT address.
 
+Clocks required for compatibles = "marvell,orion-wdt",
+				  "marvell,armada-370-wdt":
+- clocks : Must contain a single entry describing the clock input
+
+Clocks required for compatibles = "marvell,armada-xp-wdt"
+				  "marvell,armada-375-wdt"
+				  "marvell,armada-380-wdt":
+- clocks : Must contain an entry for each entry in clock-names.
+- clock-names : Must include the following entries:
+  "nbclk" (L2/coherency fabric clock),
+  "fixed" (Reference 25 MHz fixed-clock).
+
 Optional properties:
 
 - interrupts	: Contains the IRQ for watchdog expiration
@@ -30,4 +42,5 @@  Example:
 		interrupts = <3>;
 		timeout-sec = <10>;
 		status = "okay";
+		clocks = <&gate_clk 7>;
 	};
diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
index 00d0741..8cb1ff3 100644
--- a/drivers/watchdog/orion_wdt.c
+++ b/drivers/watchdog/orion_wdt.c
@@ -114,6 +114,46 @@  static int armada370_wdt_clock_init(struct platform_device *pdev,
 	return 0;
 }
 
+static int armada375_wdt_clock_init(struct platform_device *pdev,
+				    struct orion_watchdog *dev)
+{
+	int ret;
+
+	dev->clk = of_clk_get_by_name(pdev->dev.of_node, "fixed");
+	if (!IS_ERR(dev->clk)) {
+		ret = clk_prepare_enable(dev->clk);
+		if (ret) {
+			clk_put(dev->clk);
+			return ret;
+		}
+
+		atomic_io_modify(dev->reg + TIMER_CTRL,
+				WDT_AXP_FIXED_ENABLE_BIT,
+				WDT_AXP_FIXED_ENABLE_BIT);
+		dev->clk_rate = clk_get_rate(dev->clk);
+
+		return 0;
+	}
+
+	/* Mandatory fallback for proper devicetree backward compatibility */
+	dev->clk = clk_get(&pdev->dev, NULL);
+	if (IS_ERR(dev->clk))
+		return PTR_ERR(dev->clk);
+
+	ret = clk_prepare_enable(dev->clk);
+	if (ret) {
+		clk_put(dev->clk);
+		return ret;
+	}
+
+	atomic_io_modify(dev->reg + TIMER_CTRL,
+			WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT),
+			WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT));
+	dev->clk_rate = clk_get_rate(dev->clk) / WDT_A370_RATIO;
+
+	return 0;
+}
+
 static int armadaxp_wdt_clock_init(struct platform_device *pdev,
 				   struct orion_watchdog *dev)
 {
@@ -394,7 +434,7 @@  static const struct orion_watchdog_data armada375_data = {
 	.rstout_mask_bit = BIT(10),
 	.wdt_enable_bit = BIT(8),
 	.wdt_counter_offset = 0x34,
-	.clock_init = armada370_wdt_clock_init,
+	.clock_init = armada375_wdt_clock_init,
 	.enabled = armada375_enabled,
 	.start = armada375_start,
 	.stop = armada375_stop,