[PATCH 1/2] rtc: tegra: Sort includes alphabetically

[PATCH 1/2] rtc: tegra: Sort includes alphabetically

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The ordering of includes is currently completely arbitrary, making it
impossible to decide where to put new includes. Remove the dilemma by
sort the include list alphabetically.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/rtc/rtc-tegra.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/rtc/rtc-tegra.c b/drivers/rtc/rtc-tegra.c
index 3853ba963bb5..38e662ff1a70 100644
--- a/drivers/rtc/rtc-tegra.c
+++ b/drivers/rtc/rtc-tegra.c
@@ -17,16 +17,17 @@
  * with this program; if not, write to the Free Software Foundation, Inc.,
  * 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
  */
-#include <linux/kernel.h>
+
+#include <linux/delay.h>
 #include <linux/init.h>
-#include <linux/module.h>
-#include <linux/slab.h>
-#include <linux/irq.h>
 #include <linux/io.h>
-#include <linux/delay.h>
-#include <linux/rtc.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/pm.h>
+#include <linux/rtc.h>
+#include <linux/slab.h>
 
 /* set to 1 = busy every eight 32kHz clocks during copy of sec+msec to AHB */
 #define TEGRA_RTC_REG_BUSY			0x004
-- 
2.11.0

[PATCH 2/2] rtc: tegra: Implement clock handling

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Accessing the registers of the RTC block on Tegra requires the module
clock to be enabled. This only works because the RTC module clock will
be enabled by default during early boot. However, because the clock is
unused, the CCF will disable it at late_init time. This causes the RTC
to become unusable afterwards. This can easily be reproduced by trying
to use the RTC:

	$ hwclock --rtc /dev/rtc1

This will hang the system. I ran into this by following up on a report
by Martin Michlmayr that reboot wasn't working on Tegra210 systems. It
turns out that the rtc-tegra driver's ->shutdown() implementation will
hang the CPU, because of the disabled clock, before the system can be
rebooted.

What confused me for a while is that the same driver is used on prior
Tegra generations where the hang can not be observed. However, as Peter
De Schrijver pointed out, this is because on 32-bit Tegra chips the RTC
clock is enabled by the tegra20_timer.c clocksource driver, which uses
the RTC to provide a persistent clock. This code is never enabled on
64-bit Tegra because the persistent clock infrastructure does not exist
of 64-bit ARM.

The proper fix for this is to add proper clock handling to the RTC
driver in order to ensure that the clock is enabled when the driver
requires it. All device trees contain the clock already, therefore
no additional changes are required.

Cc: Peter De Schrijver <pdeschrijver@nvidia.com>
Reported-by: Martin Michlmayr <tbm@cyrius.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/rtc/rtc-tegra.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-tegra.c b/drivers/rtc/rtc-tegra.c
index 38e662ff1a70..d30d57b048d3 100644
--- a/drivers/rtc/rtc-tegra.c
+++ b/drivers/rtc/rtc-tegra.c
@@ -18,6 +18,7 @@
  * 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
  */
 
+#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/init.h>
 #include <linux/io.h>
@@ -60,6 +61,7 @@ struct tegra_rtc_info {
 	struct platform_device	*pdev;
 	struct rtc_device	*rtc_dev;
 	void __iomem		*rtc_base; /* NULL if not initialized. */
+	struct clk		*clk;
 	int			tegra_rtc_irq; /* alarm and periodic irq */
 	spinlock_t		tegra_rtc_lock;
 };
@@ -327,6 +329,14 @@ static int __init tegra_rtc_probe(struct platform_device *pdev)
 	if (info->tegra_rtc_irq <= 0)
 		return -EBUSY;
 
+	info->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(info->clk))
+		return PTR_ERR(info->clk);
+
+	ret = clk_prepare_enable(info->clk);
+	if (ret < 0)
+		return ret;
+
 	/* set context info. */
 	info->pdev = pdev;
 	spin_lock_init(&info->tegra_rtc_lock);
@@ -347,7 +357,7 @@ static int __init tegra_rtc_probe(struct platform_device *pdev)
 		ret = PTR_ERR(info->rtc_dev);
 		dev_err(&pdev->dev, "Unable to register device (err=%d).\n",
 			ret);
-		return ret;
+		goto disable_clk;
 	}
 
 	ret = devm_request_irq(&pdev->dev, info->tegra_rtc_irq,
@@ -357,12 +367,25 @@ static int __init tegra_rtc_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev,
 			"Unable to request interrupt for device (err=%d).\n",
 			ret);
-		return ret;
+		goto disable_clk;
 	}
 
 	dev_notice(&pdev->dev, "Tegra internal Real Time Clock\n");
 
 	return 0;
+
+disable_clk:
+	clk_disable_unprepare(info->clk);
+	return ret;
+}
+
+static int tegra_rtc_remove(struct platform_device *pdev)
+{
+	struct tegra_rtc_info *info = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(info->clk);
+
+	return 0;
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -414,6 +437,7 @@ static void tegra_rtc_shutdown(struct platform_device *pdev)
 
 MODULE_ALIAS("platform:tegra_rtc");
 static struct platform_driver tegra_rtc_driver = {
+	.remove		= tegra_rtc_remove,
 	.shutdown	= tegra_rtc_shutdown,
 	.driver		= {
 		.name	= "tegra_rtc",
-- 
2.11.0

Re: [PATCH 1/2] rtc: tegra: Sort includes alphabetically

[Alexandre Belloni]

On 12/01/2017 at 17:07:42 +0100, Thierry Reding wrote :
> From: Thierry Reding <treding@nvidia.com>
> 
> The ordering of includes is currently completely arbitrary, making it
> impossible to decide where to put new includes. Remove the dilemma by
> sort the include list alphabetically.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/rtc/rtc-tegra.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
Applied both, thanks.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 2/2] rtc: tegra: Implement clock handling

[Peter De Schrijver]

On Thu, Jan 12, 2017 at 05:07:43PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Accessing the registers of the RTC block on Tegra requires the module
> clock to be enabled. This only works because the RTC module clock will
> be enabled by default during early boot. However, because the clock is
> unused, the CCF will disable it at late_init time. This causes the RTC
> to become unusable afterwards. This can easily be reproduced by trying
> to use the RTC:
> 
> 	$ hwclock --rtc /dev/rtc1
> 
> This will hang the system. I ran into this by following up on a report
> by Martin Michlmayr that reboot wasn't working on Tegra210 systems. It
> turns out that the rtc-tegra driver's ->shutdown() implementation will
> hang the CPU, because of the disabled clock, before the system can be
> rebooted.
> 
> What confused me for a while is that the same driver is used on prior
> Tegra generations where the hang can not be observed. However, as Peter
> De Schrijver pointed out, this is because on 32-bit Tegra chips the RTC
> clock is enabled by the tegra20_timer.c clocksource driver, which uses
> the RTC to provide a persistent clock. This code is never enabled on
> 64-bit Tegra because the persistent clock infrastructure does not exist
> of 64-bit ARM.

'on 64-bit ARM' I guess?

Other than that Acked-By Peter De Schrijver <pdeschrijver@nvidia.com>

> 
> The proper fix for this is to add proper clock handling to the RTC
> driver in order to ensure that the clock is enabled when the driver
> requires it. All device trees contain the clock already, therefore
> no additional changes are required.
> 
> Cc: Peter De Schrijver <pdeschrijver@nvidia.com>
> Reported-by: Martin Michlmayr <tbm@cyrius.com>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/rtc/rtc-tegra.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-tegra.c b/drivers/rtc/rtc-tegra.c
> index 38e662ff1a70..d30d57b048d3 100644
> --- a/drivers/rtc/rtc-tegra.c
> +++ b/drivers/rtc/rtc-tegra.c
> @@ -18,6 +18,7 @@
>   * 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
>   */
>  
> +#include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/init.h>
>  #include <linux/io.h>
> @@ -60,6 +61,7 @@ struct tegra_rtc_info {
>  	struct platform_device	*pdev;
>  	struct rtc_device	*rtc_dev;
>  	void __iomem		*rtc_base; /* NULL if not initialized. */
> +	struct clk		*clk;
>  	int			tegra_rtc_irq; /* alarm and periodic irq */
>  	spinlock_t		tegra_rtc_lock;
>  };
> @@ -327,6 +329,14 @@ static int __init tegra_rtc_probe(struct platform_device *pdev)
>  	if (info->tegra_rtc_irq <= 0)
>  		return -EBUSY;
>  
> +	info->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(info->clk))
> +		return PTR_ERR(info->clk);
> +
> +	ret = clk_prepare_enable(info->clk);
> +	if (ret < 0)
> +		return ret;
> +
>  	/* set context info. */
>  	info->pdev = pdev;
>  	spin_lock_init(&info->tegra_rtc_lock);
> @@ -347,7 +357,7 @@ static int __init tegra_rtc_probe(struct platform_device *pdev)
>  		ret = PTR_ERR(info->rtc_dev);
>  		dev_err(&pdev->dev, "Unable to register device (err=%d).\n",
>  			ret);
> -		return ret;
> +		goto disable_clk;
>  	}
>  
>  	ret = devm_request_irq(&pdev->dev, info->tegra_rtc_irq,
> @@ -357,12 +367,25 @@ static int __init tegra_rtc_probe(struct platform_device *pdev)
>  		dev_err(&pdev->dev,
>  			"Unable to request interrupt for device (err=%d).\n",
>  			ret);
> -		return ret;
> +		goto disable_clk;
>  	}
>  
>  	dev_notice(&pdev->dev, "Tegra internal Real Time Clock\n");
>  
>  	return 0;
> +
> +disable_clk:
> +	clk_disable_unprepare(info->clk);
> +	return ret;
> +}
> +
> +static int tegra_rtc_remove(struct platform_device *pdev)
> +{
> +	struct tegra_rtc_info *info = platform_get_drvdata(pdev);
> +
> +	clk_disable_unprepare(info->clk);
> +
> +	return 0;
>  }
>  
>  #ifdef CONFIG_PM_SLEEP
> @@ -414,6 +437,7 @@ static void tegra_rtc_shutdown(struct platform_device *pdev)
>  
>  MODULE_ALIAS("platform:tegra_rtc");
>  static struct platform_driver tegra_rtc_driver = {
> +	.remove		= tegra_rtc_remove,
>  	.shutdown	= tegra_rtc_shutdown,
>  	.driver		= {
>  		.name	= "tegra_rtc",
> -- 
> 2.11.0
>