[PATCH 0/5] rtc: at91sam9: add DT support

[PATCH 0/5] rtc: at91sam9: add DT support

[Boris BREZILLON]

Hello,

This patch series adds DT support to the atmel at91sam9 RTC driver.

It also removes any machine specific inclusions to prepare the migration
to multi platform kernel support, and retain the slow clock to prevent
the CCF from disabling it at the end of boot.

Best Regards,

Boris

Boris BREZILLON (5):
  rtc: at91sam9: remove references to mach specific headers
  rtc: at91sam9: use standard readl/writel functions instead of raw
    versions
  rtc: at91sam9: add DT support
  rtc: at91sam9: retain slow clock and check its rate
  rtc: at91sam9: add DT bindings documentation

 .../devicetree/bindings/rtc/atmel,at91sam9-rtc.txt | 20 +++++++
 drivers/rtc/rtc-at91sam9.c                         | 63 +++++++++++++++++++---
 2 files changed, 75 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/rtc/atmel,at91sam9-rtc.txt

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5] rtc: at91sam9: remove references to mach specific headers

[Boris BREZILLON]

In order to support multi platform kernel drivers should not include
machine specific headers.
Copy RTT macros in the driver code and remove any machine specific
headers.

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
 drivers/rtc/rtc-at91sam9.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/rtc/rtc-at91sam9.c b/drivers/rtc/rtc-at91sam9.c
index 5963743..51f0038 100644
--- a/drivers/rtc/rtc-at91sam9.c
+++ b/drivers/rtc/rtc-at91sam9.c
@@ -22,10 +22,6 @@
 #include <linux/platform_data/atmel.h>
 #include <linux/io.h>
 
-#include <mach/at91_rtt.h>
-#include <mach/cpu.h>
-#include <mach/hardware.h>
-
 /*
  * This driver uses two configurable hardware resources that live in the
  * AT91SAM9 backup power domain (intended to be powered at all times)
@@ -47,6 +43,24 @@
  * registers available, likewise usable for more than "RTC" support.
  */
 
+#define AT91_RTT_MR		0x00			/* Real-time Mode Register */
+#define AT91_RTT_RTPRES		(0xffff << 0)		/* Real-time Timer Prescaler Value */
+#define AT91_RTT_ALMIEN		(1 << 16)		/* Alarm Interrupt Enable */
+#define AT91_RTT_RTTINCIEN	(1 << 17)		/* Real Time Timer Increment Interrupt Enable */
+#define AT91_RTT_RTTRST		(1 << 18)		/* Real Time Timer Restart */
+
+#define AT91_RTT_AR		0x04			/* Real-time Alarm Register */
+#define AT91_RTT_ALMV		(0xffffffff)		/* Alarm Value */
+
+#define AT91_RTT_VR		0x08			/* Real-time Value Register */
+#define AT91_RTT_CRTV		(0xffffffff)		/* Current Real-time Value */
+
+#define AT91_RTT_SR		0x0c			/* Real-time Status Register */
+#define AT91_RTT_ALMS		(1 << 0)		/* Real-time Alarm Status */
+#define AT91_RTT_RTTINC		(1 << 1)		/* Real-time Timer Increment */
+
+#define AT91_SLOW_CLOCK		32768
+
 /*
  * We store ALARM_DISABLED in ALMV to record that no alarm is set.
  * It's also the reset value for that field.
-- 
1.9.1

Re: [PATCH 1/5] rtc: at91sam9: remove references to mach specific headers

[Alexandre Belloni]

On 03/09/2014 at 10:45:30 +0200, Boris Brezillon wrote :
> In order to support multi platform kernel drivers should not include
> machine specific headers.
> Copy RTT macros in the driver code and remove any machine specific
> headers.
> 
> Signed-off-by: Boris BREZILLON <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/rtc/rtc-at91sam9.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-at91sam9.c b/drivers/rtc/rtc-at91sam9.c
> index 5963743..51f0038 100644
> --- a/drivers/rtc/rtc-at91sam9.c
> +++ b/drivers/rtc/rtc-at91sam9.c
> @@ -22,10 +22,6 @@
>  #include <linux/platform_data/atmel.h>
>  #include <linux/io.h>
>  
> -#include <mach/at91_rtt.h>
> -#include <mach/cpu.h>
> -#include <mach/hardware.h>
> -
>  /*
>   * This driver uses two configurable hardware resources that live in the
>   * AT91SAM9 backup power domain (intended to be powered at all times)
> @@ -47,6 +43,24 @@
>   * registers available, likewise usable for more than "RTC" support.
>   */
>  
> +#define AT91_RTT_MR		0x00			/* Real-time Mode Register */
> +#define AT91_RTT_RTPRES		(0xffff << 0)		/* Real-time Timer Prescaler Value */
> +#define AT91_RTT_ALMIEN		(1 << 16)		/* Alarm Interrupt Enable */
> +#define AT91_RTT_RTTINCIEN	(1 << 17)		/* Real Time Timer Increment Interrupt Enable */
> +#define AT91_RTT_RTTRST		(1 << 18)		/* Real Time Timer Restart */
> +
> +#define AT91_RTT_AR		0x04			/* Real-time Alarm Register */
> +#define AT91_RTT_ALMV		(0xffffffff)		/* Alarm Value */
> +
> +#define AT91_RTT_VR		0x08			/* Real-time Value Register */
> +#define AT91_RTT_CRTV		(0xffffffff)		/* Current Real-time Value */
> +
> +#define AT91_RTT_SR		0x0c			/* Real-time Status Register */
> +#define AT91_RTT_ALMS		(1 << 0)		/* Real-time Alarm Status */
> +#define AT91_RTT_RTTINC		(1 << 1)		/* Real-time Timer Increment */
> +
> +#define AT91_SLOW_CLOCK		32768

I would prefer that we get rid of AT91_SLOW_CLOCK or at east not
introduce new use of it.

Maybe we can clk_get_rate() on the provided clock and fall back on 32KHz
if no clock is provided, showing a warning.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/5] rtc: at91sam9: use standard readl/writel functions instead of raw versions

[Boris BREZILLON]

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
 drivers/rtc/rtc-at91sam9.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/rtc/rtc-at91sam9.c b/drivers/rtc/rtc-at91sam9.c
index 51f0038..74a9ca0 100644
--- a/drivers/rtc/rtc-at91sam9.c
+++ b/drivers/rtc/rtc-at91sam9.c
@@ -77,14 +77,14 @@ struct sam9_rtc {
 };
 
 #define rtt_readl(rtc, field) \
-	__raw_readl((rtc)->rtt + AT91_RTT_ ## field)
+	readl((rtc)->rtt + AT91_RTT_ ## field)
 #define rtt_writel(rtc, field, val) \
-	__raw_writel((val), (rtc)->rtt + AT91_RTT_ ## field)
+	writel((val), (rtc)->rtt + AT91_RTT_ ## field)
 
 #define gpbr_readl(rtc) \
-	__raw_readl((rtc)->gpbr)
+	readl((rtc)->gpbr)
 #define gpbr_writel(rtc, val) \
-	__raw_writel((val), (rtc)->gpbr)
+	writel((val), (rtc)->gpbr)
 
 /*
  * Read current time and date in RTC
-- 
1.9.1

[PATCH 3/5] rtc: at91sam9: add DT support

[Boris BREZILLON]

Add of_match_table to the existing driver so that rtc nodes defined in at91
DTs can be attached to this driver.

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
 drivers/rtc/rtc-at91sam9.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/rtc/rtc-at91sam9.c b/drivers/rtc/rtc-at91sam9.c
index 74a9ca0..57014b7 100644
--- a/drivers/rtc/rtc-at91sam9.c
+++ b/drivers/rtc/rtc-at91sam9.c
@@ -454,6 +454,14 @@ static int at91_rtc_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(at91_rtc_pm_ops, at91_rtc_suspend, at91_rtc_resume);
 
+#ifdef CONFIG_OF
+static const struct of_device_id at91_rtc_dt_ids[] = {
+	{ .compatible = "atmel,at91sam9260-rtt" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, at91_rtc_dt_ids);
+#endif
+
 static struct platform_driver at91_rtc_driver = {
 	.probe		= at91_rtc_probe,
 	.remove		= at91_rtc_remove,
@@ -462,6 +470,7 @@ static struct platform_driver at91_rtc_driver = {
 		.name	= "rtc-at91sam9",
 		.owner	= THIS_MODULE,
 		.pm	= &at91_rtc_pm_ops,
+		.of_match_table = of_match_ptr(at91_rtc_dt_ids),
 	},
 };
 
-- 
1.9.1

[PATCH 4/5] rtc: at91sam9: retain slow clock and check its rate

[Boris BREZILLON]

The RTT block is using the slow clock and expect it to run at 32KHz.
Now that we moved to the CCF it's better to retain the clk reference so
that the CCF can't disable the slow clock considering it is unused.

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
 drivers/rtc/rtc-at91sam9.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/rtc/rtc-at91sam9.c b/drivers/rtc/rtc-at91sam9.c
index 57014b7..5c5093b 100644
--- a/drivers/rtc/rtc-at91sam9.c
+++ b/drivers/rtc/rtc-at91sam9.c
@@ -21,6 +21,7 @@
 #include <linux/slab.h>
 #include <linux/platform_data/atmel.h>
 #include <linux/io.h>
+#include <linux/clk.h>
 
 /*
  * This driver uses two configurable hardware resources that live in the
@@ -74,6 +75,7 @@ struct sam9_rtc {
 	u32			imr;
 	void __iomem		*gpbr;
 	int 			irq;
+	struct clk		*sclk;
 };
 
 #define rtt_readl(rtc, field) \
@@ -373,6 +375,25 @@ static int at91_rtc_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	/* Retain slow clk if it is specified in the DT.
+	 * Do not complain if slow clk is missing, but check its rate
+	 * if it is available.
+	 */
+	rtc->sclk = devm_clk_get(&pdev->dev, NULL);
+	if (!IS_ERR(rtc->sclk)) {
+		if (clk_get_rate(rtc->sclk) != AT91_SLOW_CLOCK) {
+			dev_err(&pdev->dev,
+				"Invalid slow clock rate (expecting %lu got %lu)",
+				(unsigned long)AT91_SLOW_CLOCK,
+				clk_get_rate(rtc->sclk));
+			return -EINVAL;
+		}
+
+		ret = clk_prepare_enable(rtc->sclk);
+		if (ret)
+			return ret;
+	}
+
 	/* NOTE:  sam9260 rev A silicon has a ROM bug which resets the
 	 * RTT on at least some reboots.  If you have that chip, you must
 	 * initialize the time from some external source like a GPS, wall
@@ -397,6 +418,9 @@ static int at91_rtc_remove(struct platform_device *pdev)
 	/* disable all interrupts */
 	rtt_writel(rtc, MR, mr & ~(AT91_RTT_ALMIEN | AT91_RTT_RTTINCIEN));
 
+	if (!IS_ERR(rtc->sclk))
+		clk_disable_unprepare(rtc->sclk);
+
 	return 0;
 }
 
-- 
1.9.1

Re: [PATCH 4/5] rtc: at91sam9: retain slow clock and check its rate

[Alexandre Belloni]

On 03/09/2014 at 10:45:33 +0200, Boris Brezillon wrote :
> The RTT block is using the slow clock and expect it to run at 32KHz.
> Now that we moved to the CCF it's better to retain the clk reference so
> that the CCF can't disable the slow clock considering it is unused.
> 
> Signed-off-by: Boris BREZILLON <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/rtc/rtc-at91sam9.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/rtc/rtc-at91sam9.c b/drivers/rtc/rtc-at91sam9.c
> index 57014b7..5c5093b 100644
> --- a/drivers/rtc/rtc-at91sam9.c
> +++ b/drivers/rtc/rtc-at91sam9.c
> @@ -21,6 +21,7 @@
>  #include <linux/slab.h>
>  #include <linux/platform_data/atmel.h>
>  #include <linux/io.h>
> +#include <linux/clk.h>
>  
>  /*
>   * This driver uses two configurable hardware resources that live in the
> @@ -74,6 +75,7 @@ struct sam9_rtc {
>  	u32			imr;
>  	void __iomem		*gpbr;
>  	int 			irq;
> +	struct clk		*sclk;
>  };
>  
>  #define rtt_readl(rtc, field) \
> @@ -373,6 +375,25 @@ static int at91_rtc_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	/* Retain slow clk if it is specified in the DT.
> +	 * Do not complain if slow clk is missing, but check its rate
> +	 * if it is available.
> +	 */
> +	rtc->sclk = devm_clk_get(&pdev->dev, NULL);
> +	if (!IS_ERR(rtc->sclk)) {
> +		if (clk_get_rate(rtc->sclk) != AT91_SLOW_CLOCK) {

I would not bother doing that check but use the value for MR instead of
AT91_SLOW_CLOCK (see my previous mail).

> +			dev_err(&pdev->dev,
> +				"Invalid slow clock rate (expecting %lu got %lu)",
> +				(unsigned long)AT91_SLOW_CLOCK,
> +				clk_get_rate(rtc->sclk));
> +			return -EINVAL;
> +		}
> +
> +		ret = clk_prepare_enable(rtc->sclk);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	/* NOTE:  sam9260 rev A silicon has a ROM bug which resets the
>  	 * RTT on at least some reboots.  If you have that chip, you must
>  	 * initialize the time from some external source like a GPS, wall
> @@ -397,6 +418,9 @@ static int at91_rtc_remove(struct platform_device *pdev)
>  	/* disable all interrupts */
>  	rtt_writel(rtc, MR, mr & ~(AT91_RTT_ALMIEN | AT91_RTT_RTTINCIEN));
>  
> +	if (!IS_ERR(rtc->sclk))
> +		clk_disable_unprepare(rtc->sclk);
> +
>  	return 0;
>  }
>  
> -- 
> 1.9.1
> 

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/5] rtc: at91sam9: retain slow clock and check its rate

[Boris BREZILLON]

On Mon, 8 Sep 2014 19:33:38 +0200
Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote:

> On 03/09/2014 at 10:45:33 +0200, Boris Brezillon wrote :
> > The RTT block is using the slow clock and expect it to run at 32KHz.
> > Now that we moved to the CCF it's better to retain the clk reference so
> > that the CCF can't disable the slow clock considering it is unused.
> > 
> > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> > ---
> >  drivers/rtc/rtc-at91sam9.c | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/drivers/rtc/rtc-at91sam9.c b/drivers/rtc/rtc-at91sam9.c
> > index 57014b7..5c5093b 100644
> > --- a/drivers/rtc/rtc-at91sam9.c
> > +++ b/drivers/rtc/rtc-at91sam9.c
> > @@ -21,6 +21,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/platform_data/atmel.h>
> >  #include <linux/io.h>
> > +#include <linux/clk.h>
> >  
> >  /*
> >   * This driver uses two configurable hardware resources that live in the
> > @@ -74,6 +75,7 @@ struct sam9_rtc {
> >  	u32			imr;
> >  	void __iomem		*gpbr;
> >  	int 			irq;
> > +	struct clk		*sclk;
> >  };
> >  
> >  #define rtt_readl(rtc, field) \
> > @@ -373,6 +375,25 @@ static int at91_rtc_probe(struct platform_device *pdev)
> >  		return ret;
> >  	}
> >  
> > +	/* Retain slow clk if it is specified in the DT.
> > +	 * Do not complain if slow clk is missing, but check its rate
> > +	 * if it is available.
> > +	 */
> > +	rtc->sclk = devm_clk_get(&pdev->dev, NULL);
> > +	if (!IS_ERR(rtc->sclk)) {
> > +		if (clk_get_rate(rtc->sclk) != AT91_SLOW_CLOCK) {
> 
> I would not bother doing that check but use the value for MR instead of
> AT91_SLOW_CLOCK (see my previous mail).

Unfortunately, we can't get rid of this macro without modifying the
clk_lookup table in several arch/arm/mach-at91/<soc-name>.c files in
order to handle non DT/CCF cases (which will remain until all non DT
boards are moved to DT).
But I agree that this should be removed as soon as possible (AFAIR, all
SoCs have already been moved to the CCF).
How about adding a TODO comment ?

> 
> > +			dev_err(&pdev->dev,
> > +				"Invalid slow clock rate (expecting %lu got %lu)",
> > +				(unsigned long)AT91_SLOW_CLOCK,
> > +				clk_get_rate(rtc->sclk));
> > +			return -EINVAL;
> > +		}
> > +
> > +		ret = clk_prepare_enable(rtc->sclk);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> >  	/* NOTE:  sam9260 rev A silicon has a ROM bug which resets the
> >  	 * RTT on at least some reboots.  If you have that chip, you must
> >  	 * initialize the time from some external source like a GPS, wall
> > @@ -397,6 +418,9 @@ static int at91_rtc_remove(struct platform_device *pdev)
> >  	/* disable all interrupts */
> >  	rtt_writel(rtc, MR, mr & ~(AT91_RTT_ALMIEN | AT91_RTT_RTTINCIEN));
> >  
> > +	if (!IS_ERR(rtc->sclk))
> > +		clk_disable_unprepare(rtc->sclk);
> > +
> >  	return 0;
> >  }
> >  
> > -- 
> > 1.9.1
> > 
> 



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

Re: [PATCH 4/5] rtc: at91sam9: retain slow clock and check its rate

[Boris BREZILLON]

On Mon, 8 Sep 2014 21:22:18 +0200
Boris BREZILLON <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:

> On Mon, 8 Sep 2014 19:33:38 +0200
> Alexandre Belloni <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> 
> > On 03/09/2014 at 10:45:33 +0200, Boris Brezillon wrote :
> > > The RTT block is using the slow clock and expect it to run at 32KHz.
> > > Now that we moved to the CCF it's better to retain the clk reference so
> > > that the CCF can't disable the slow clock considering it is unused.
> > > 
> > > Signed-off-by: Boris BREZILLON <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > > ---
> > >  drivers/rtc/rtc-at91sam9.c | 24 ++++++++++++++++++++++++
> > >  1 file changed, 24 insertions(+)
> > > 
> > > diff --git a/drivers/rtc/rtc-at91sam9.c b/drivers/rtc/rtc-at91sam9.c
> > > index 57014b7..5c5093b 100644
> > > --- a/drivers/rtc/rtc-at91sam9.c
> > > +++ b/drivers/rtc/rtc-at91sam9.c
> > > @@ -21,6 +21,7 @@
> > >  #include <linux/slab.h>
> > >  #include <linux/platform_data/atmel.h>
> > >  #include <linux/io.h>
> > > +#include <linux/clk.h>
> > >  
> > >  /*
> > >   * This driver uses two configurable hardware resources that live in the
> > > @@ -74,6 +75,7 @@ struct sam9_rtc {
> > >  	u32			imr;
> > >  	void __iomem		*gpbr;
> > >  	int 			irq;
> > > +	struct clk		*sclk;
> > >  };
> > >  
> > >  #define rtt_readl(rtc, field) \
> > > @@ -373,6 +375,25 @@ static int at91_rtc_probe(struct platform_device *pdev)
> > >  		return ret;
> > >  	}
> > >  
> > > +	/* Retain slow clk if it is specified in the DT.
> > > +	 * Do not complain if slow clk is missing, but check its rate
> > > +	 * if it is available.
> > > +	 */
> > > +	rtc->sclk = devm_clk_get(&pdev->dev, NULL);
> > > +	if (!IS_ERR(rtc->sclk)) {
> > > +		if (clk_get_rate(rtc->sclk) != AT91_SLOW_CLOCK) {
> > 
> > I would not bother doing that check but use the value for MR instead of
> > AT91_SLOW_CLOCK (see my previous mail).
> 
> Unfortunately, we can't get rid of this macro without modifying the
> clk_lookup table in several arch/arm/mach-at91/<soc-name>.c files in
> order to handle non DT/CCF cases (which will remain until all non DT
> boards are moved to DT).

After taking a closer look at what should be modified, I think it's
worth it: there's only 5 impacted files (at91sam9260.c, at91sam9261.c,
at91sam9263.c, at91sam9rl.c and at91sam9g45.c) and adding a clk_lookup
entry is pretty easy.

Moreover we'll end up with a clean driver and won't have to bother
about cleaning it up when dropping non DT boards support.



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/5] rtc: at91sam9: retain slow clock and check its rate

[Nicolas Ferre]

On 08/09/2014 21:37, Boris BREZILLON :
> On Mon, 8 Sep 2014 21:22:18 +0200
> Boris BREZILLON <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> 
>> On Mon, 8 Sep 2014 19:33:38 +0200
>> Alexandre Belloni <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>>
>>> On 03/09/2014 at 10:45:33 +0200, Boris Brezillon wrote :
>>>> The RTT block is using the slow clock and expect it to run at 32KHz.
>>>> Now that we moved to the CCF it's better to retain the clk reference so
>>>> that the CCF can't disable the slow clock considering it is unused.
>>>>
>>>> Signed-off-by: Boris BREZILLON <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>>>> ---
>>>>  drivers/rtc/rtc-at91sam9.c | 24 ++++++++++++++++++++++++
>>>>  1 file changed, 24 insertions(+)
>>>>
>>>> diff --git a/drivers/rtc/rtc-at91sam9.c b/drivers/rtc/rtc-at91sam9.c
>>>> index 57014b7..5c5093b 100644
>>>> --- a/drivers/rtc/rtc-at91sam9.c
>>>> +++ b/drivers/rtc/rtc-at91sam9.c
>>>> @@ -21,6 +21,7 @@
>>>>  #include <linux/slab.h>
>>>>  #include <linux/platform_data/atmel.h>
>>>>  #include <linux/io.h>
>>>> +#include <linux/clk.h>
>>>>  
>>>>  /*
>>>>   * This driver uses two configurable hardware resources that live in the
>>>> @@ -74,6 +75,7 @@ struct sam9_rtc {
>>>>  	u32			imr;
>>>>  	void __iomem		*gpbr;
>>>>  	int 			irq;
>>>> +	struct clk		*sclk;
>>>>  };
>>>>  
>>>>  #define rtt_readl(rtc, field) \
>>>> @@ -373,6 +375,25 @@ static int at91_rtc_probe(struct platform_device *pdev)
>>>>  		return ret;
>>>>  	}
>>>>  
>>>> +	/* Retain slow clk if it is specified in the DT.
>>>> +	 * Do not complain if slow clk is missing, but check its rate
>>>> +	 * if it is available.
>>>> +	 */
>>>> +	rtc->sclk = devm_clk_get(&pdev->dev, NULL);
>>>> +	if (!IS_ERR(rtc->sclk)) {
>>>> +		if (clk_get_rate(rtc->sclk) != AT91_SLOW_CLOCK) {
>>>
>>> I would not bother doing that check but use the value for MR instead of
>>> AT91_SLOW_CLOCK (see my previous mail).
>>
>> Unfortunately, we can't get rid of this macro without modifying the
>> clk_lookup table in several arch/arm/mach-at91/<soc-name>.c files in
>> order to handle non DT/CCF cases (which will remain until all non DT
>> boards are moved to DT).
> 
> After taking a closer look at what should be modified, I think it's
> worth it: there's only 5 impacted files (at91sam9260.c, at91sam9261.c,
> at91sam9263.c, at91sam9rl.c and at91sam9g45.c) and adding a clk_lookup
> entry is pretty easy.
> 
> Moreover we'll end up with a clean driver and won't have to bother
> about cleaning it up when dropping non DT boards support.

I vote for this => +1 ;-)

Bye,
-- 
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/5] rtc: at91sam9: add DT bindings documentation

[Boris BREZILLON]

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
 .../devicetree/bindings/rtc/atmel,at91sam9-rtc.txt   | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/atmel,at91sam9-rtc.txt

diff --git a/Documentation/devicetree/bindings/rtc/atmel,at91sam9-rtc.txt b/Documentation/devicetree/bindings/rtc/atmel,at91sam9-rtc.txt
new file mode 100644
index 0000000..9ca455f
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/atmel,at91sam9-rtc.txt
@@ -0,0 +1,20 @@
+Atmel AT91SAM9260 Real Time Timer
+
+Required properties:
+- compatible: should be: "atmel,at91sam9260-rtt"
+- reg: should contain 2 memory regions
+  * the first one encodes the memory region of the RTT controller
+  * the second one encodes the GPBR (General Purpose Backup Resgisters)
+    memory region used to store the current time
+- interrupts: rtc alarm/event interrupt
+- clocks: should contain one clock pointing the the slow clk
+
+Example:
+
+rtc@fffffe00 {
+	compatible = "atmel,at91sam9260-rtt";
+	reg = <0xfffffd20 0x10
+	       0xfffffd50 0x4>;
+	interrupts = <1 4 7>;
+	clocks = <&clk32k>;
+};
-- 
1.9.1

Re: [PATCH 5/5] rtc: at91sam9: add DT bindings documentation

[Johan Hovold]

On Wed, Sep 03, 2014 at 10:45:34AM +0200, Boris BREZILLON wrote:
> Signed-off-by: Boris BREZILLON <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  .../devicetree/bindings/rtc/atmel,at91sam9-rtc.txt   | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/atmel,at91sam9-rtc.txt
> 
> diff --git a/Documentation/devicetree/bindings/rtc/atmel,at91sam9-rtc.txt b/Documentation/devicetree/bindings/rtc/atmel,at91sam9-rtc.txt
> new file mode 100644
> index 0000000..9ca455f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/atmel,at91sam9-rtc.txt
> @@ -0,0 +1,20 @@
> +Atmel AT91SAM9260 Real Time Timer
> +
> +Required properties:
> +- compatible: should be: "atmel,at91sam9260-rtt"
> +- reg: should contain 2 memory regions
> +  * the first one encodes the memory region of the RTT controller
> +  * the second one encodes the GPBR (General Purpose Backup Resgisters)
> +    memory region used to store the current time
> +- interrupts: rtc alarm/event interrupt
> +- clocks: should contain one clock pointing the the slow clk
> +
> +Example:
> +
> +rtc@fffffe00 {
> +	compatible = "atmel,at91sam9260-rtt";
> +	reg = <0xfffffd20 0x10
> +	       0xfffffd50 0x4>;
> +	interrupts = <1 4 7>;
> +	clocks = <&clk32k>;
> +};

This does not describe the hardware, but rather a specific software
configuration.

The RTT is first of all not an RTC (although it can be used as one in a
specific software configuration). And the second register resource above
is not an RTT register, but a general-purpose backup register could be
used for other purposes (which register to use is currently configurable
for legacy booting using CONFIG_RTC_DRV_AT91SAM9_GPBR).

This was discussed in the thread where I posted an RFC for this last
year (which you linked to in your original submission thread), but no
conclusion was reached:

	http://www.spinics.net/lists/arm-kernel/msg236292.html

Johan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] rtc: at91sam9: add DT bindings documentation

[Boris BREZILLON]

Hi Johan,

On Wed, 10 Sep 2014 14:14:24 +0200
Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> On Wed, Sep 03, 2014 at 10:45:34AM +0200, Boris BREZILLON wrote:
> > Signed-off-by: Boris BREZILLON <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > ---
> >  .../devicetree/bindings/rtc/atmel,at91sam9-rtc.txt   | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/rtc/atmel,at91sam9-rtc.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/rtc/atmel,at91sam9-rtc.txt b/Documentation/devicetree/bindings/rtc/atmel,at91sam9-rtc.txt
> > new file mode 100644
> > index 0000000..9ca455f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/rtc/atmel,at91sam9-rtc.txt
> > @@ -0,0 +1,20 @@
> > +Atmel AT91SAM9260 Real Time Timer
> > +
> > +Required properties:
> > +- compatible: should be: "atmel,at91sam9260-rtt"
> > +- reg: should contain 2 memory regions
> > +  * the first one encodes the memory region of the RTT controller
> > +  * the second one encodes the GPBR (General Purpose Backup Resgisters)
> > +    memory region used to store the current time
> > +- interrupts: rtc alarm/event interrupt
> > +- clocks: should contain one clock pointing the the slow clk
> > +
> > +Example:
> > +
> > +rtc@fffffe00 {
> > +	compatible = "atmel,at91sam9260-rtt";
> > +	reg = <0xfffffd20 0x10
> > +	       0xfffffd50 0x4>;
> > +	interrupts = <1 4 7>;
> > +	clocks = <&clk32k>;
> > +};
> 
> This does not describe the hardware, but rather a specific software
> configuration.
> 
> The RTT is first of all not an RTC (although it can be used as one in a
> specific software configuration). And the second register resource above
> is not an RTT register, but a general-purpose backup register could be
> used for other purposes (which register to use is currently configurable
> for legacy booting using CONFIG_RTC_DRV_AT91SAM9_GPBR).
> 
> This was discussed in the thread where I posted an RFC for this last
> year (which you linked to in your original submission thread), but no
> conclusion was reached:
> 
> 	http://www.spinics.net/lists/arm-kernel/msg236292.html

Yes, I read this thread.

Please, lets just find a solution, even if it's not a perfect one,
because the situation is unacceptable.
We're missing this features since the move to DT because we were not
able to agree on a DT binding...

I know DT bindings are supposed to represent HW parts and not what
they're used for or how they're configured, but do you see any other
real usage of the RTT block ?

BTW, I don't care which binding/implementation is chosen but we need to
sort this out!

Best Regards,

Boris


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] rtc: at91sam9: add DT bindings documentation

[Johan Hovold]

On Wed, Sep 10, 2014 at 02:43:15PM +0200, Boris BREZILLON wrote:
> Hi Johan,
> 
> On Wed, 10 Sep 2014 14:14:24 +0200
> Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> > On Wed, Sep 03, 2014 at 10:45:34AM +0200, Boris BREZILLON wrote:
> > > Signed-off-by: Boris BREZILLON <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > > ---
> > >  .../devicetree/bindings/rtc/atmel,at91sam9-rtc.txt   | 20 ++++++++++++++++++++
> > >  1 file changed, 20 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/rtc/atmel,at91sam9-rtc.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/rtc/atmel,at91sam9-rtc.txt b/Documentation/devicetree/bindings/rtc/atmel,at91sam9-rtc.txt
> > > new file mode 100644
> > > index 0000000..9ca455f
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/rtc/atmel,at91sam9-rtc.txt
> > > @@ -0,0 +1,20 @@
> > > +Atmel AT91SAM9260 Real Time Timer
> > > +
> > > +Required properties:
> > > +- compatible: should be: "atmel,at91sam9260-rtt"
> > > +- reg: should contain 2 memory regions
> > > +  * the first one encodes the memory region of the RTT controller
> > > +  * the second one encodes the GPBR (General Purpose Backup Resgisters)
> > > +    memory region used to store the current time
> > > +- interrupts: rtc alarm/event interrupt
> > > +- clocks: should contain one clock pointing the the slow clk
> > > +
> > > +Example:
> > > +
> > > +rtc@fffffe00 {
> > > +	compatible = "atmel,at91sam9260-rtt";
> > > +	reg = <0xfffffd20 0x10
> > > +	       0xfffffd50 0x4>;
> > > +	interrupts = <1 4 7>;
> > > +	clocks = <&clk32k>;
> > > +};
> > 
> > This does not describe the hardware, but rather a specific software
> > configuration.
> > 
> > The RTT is first of all not an RTC (although it can be used as one in a
> > specific software configuration). And the second register resource above
> > is not an RTT register, but a general-purpose backup register could be
> > used for other purposes (which register to use is currently configurable
> > for legacy booting using CONFIG_RTC_DRV_AT91SAM9_GPBR).
> > 
> > This was discussed in the thread where I posted an RFC for this last
> > year (which you linked to in your original submission thread), but no
> > conclusion was reached:
> > 
> > 	http://www.spinics.net/lists/arm-kernel/msg236292.html
> 
> Yes, I read this thread.

I'm sure you did. I just tried to summarise the main points of it above.

> Please, lets just find a solution, even if it's not a perfect one,
> because the situation is unacceptable.
> We're missing this features since the move to DT because we were not
> able to agree on a DT binding...

Agreed. My suggestion in the thread above was along the lines of generic
use-neutral rtt and gmbr nodes, and then an additional attribute to the
rtt node (which can be set in a specific board dts, when enabling the
rtt) providing a gmbr handle (and register number) for the rtc-at91sam9
driver to use.

This in itself does not resolve which rtt-driver would get bound if
there is ever another one (and the gmbr attribute is present), though.

> I know DT bindings are supposed to represent HW parts and not what
> they're used for or how they're configured, but do you see any other
> real usage of the RTT block ?

It's at least not hard to imagine other uses for the battery-backed up
gmbr registers.

I'll look into how that could be implemented.

> BTW, I don't care which binding/implementation is chosen but we need to
> sort this out!

Ok, let's do that. :)

Johan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] rtc: at91sam9: add DT bindings documentation

[Boris BREZILLON]

On Wed, 10 Sep 2014 14:14:24 +0200
Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> On Wed, Sep 03, 2014 at 10:45:34AM +0200, Boris BREZILLON wrote:
> > Signed-off-by: Boris BREZILLON <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > ---
> >  .../devicetree/bindings/rtc/atmel,at91sam9-rtc.txt   | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/rtc/atmel,at91sam9-rtc.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/rtc/atmel,at91sam9-rtc.txt b/Documentation/devicetree/bindings/rtc/atmel,at91sam9-rtc.txt
> > new file mode 100644
> > index 0000000..9ca455f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/rtc/atmel,at91sam9-rtc.txt
> > @@ -0,0 +1,20 @@
> > +Atmel AT91SAM9260 Real Time Timer
> > +
> > +Required properties:
> > +- compatible: should be: "atmel,at91sam9260-rtt"
> > +- reg: should contain 2 memory regions
> > +  * the first one encodes the memory region of the RTT controller
> > +  * the second one encodes the GPBR (General Purpose Backup Resgisters)
> > +    memory region used to store the current time
> > +- interrupts: rtc alarm/event interrupt
> > +- clocks: should contain one clock pointing the the slow clk
> > +
> > +Example:
> > +
> > +rtc@fffffe00 {
> > +	compatible = "atmel,at91sam9260-rtt";
> > +	reg = <0xfffffd20 0x10
> > +	       0xfffffd50 0x4>;
> > +	interrupts = <1 4 7>;
> > +	clocks = <&clk32k>;
> > +};
> 
> This does not describe the hardware, but rather a specific software
> configuration.
> 
> The RTT is first of all not an RTC (although it can be used as one in a
> specific software configuration). And the second register resource above
> is not an RTT register, but a general-purpose backup register could be
> used for other purposes (which register to use is currently configurable
> for legacy booting using CONFIG_RTC_DRV_AT91SAM9_GPBR).

We could use a syscon device (which exposes a regmap) for the GPBR
block.

rtc@ffffff20 {
	compatible = "atmel,at91sam9260-rtt";
	reg = <0xfffffd20 0x10>;
	interrupts = <1 4 7>;
	clocks = <&clk32k>;
	atmel,time-reg = <&gpbr 0x0>;
};

gpbr: syscon@fffffd50 {
	compatible = "atmel,at91sam9260-gpbr", "syscon";
	reg = <0xfffffd50 0x10>;
	
};

> 
> This was discussed in the thread where I posted an RFC for this last
> year (which you linked to in your original submission thread), but no
> conclusion was reached:
> 
> 	http://www.spinics.net/lists/arm-kernel/msg236292.html
> 
> Johan



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] rtc: at91sam9: add DT bindings documentation

[Johan Hovold]

On Wed, Sep 10, 2014 at 03:20:19PM +0200, Boris BREZILLON wrote:
> On Wed, 10 Sep 2014 14:14:24 +0200
> Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> > This does not describe the hardware, but rather a specific software
> > configuration.
> > 
> > The RTT is first of all not an RTC (although it can be used as one in a
> > specific software configuration). And the second register resource above
> > is not an RTT register, but a general-purpose backup register could be
> > used for other purposes (which register to use is currently configurable
> > for legacy booting using CONFIG_RTC_DRV_AT91SAM9_GPBR).
> 
> We could use a syscon device (which exposes a regmap) for the GPBR
> block.
> 
> rtc@ffffff20 {

rtt

> 	compatible = "atmel,at91sam9260-rtt";
> 	reg = <0xfffffd20 0x10>;
> 	interrupts = <1 4 7>;
> 	clocks = <&clk32k>;
> 	atmel,time-reg = <&gpbr 0x0>;
> };
> 
> gpbr: syscon@fffffd50 {
> 	compatible = "atmel,at91sam9260-gpbr", "syscon";
> 	reg = <0xfffffd50 0x10>;
> 	
> };

Yes, this essentially what I suggested in the thread (and my last reply)
and relying on syscon rather than a custom driver seems like a good
idea. It would allow early access to the registers too with the recently
proposed changes. It would not guarantee any kind of exclusivity,
though, but I guess that's tolerable?

Johan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] rtc: at91sam9: add DT bindings documentation

[Boris BREZILLON]

On Wed, 10 Sep 2014 17:07:02 +0200
Johan Hovold <johan@kernel.org> wrote:

> On Wed, Sep 10, 2014 at 03:20:19PM +0200, Boris BREZILLON wrote:
> > On Wed, 10 Sep 2014 14:14:24 +0200
> > Johan Hovold <johan@kernel.org> wrote:
> 
> > > This does not describe the hardware, but rather a specific software
> > > configuration.
> > > 
> > > The RTT is first of all not an RTC (although it can be used as one in a
> > > specific software configuration). And the second register resource above
> > > is not an RTT register, but a general-purpose backup register could be
> > > used for other purposes (which register to use is currently configurable
> > > for legacy booting using CONFIG_RTC_DRV_AT91SAM9_GPBR).
> > 
> > We could use a syscon device (which exposes a regmap) for the GPBR
> > block.
> > 
> > rtc@ffffff20 {
> 
> rtt
> 
> > 	compatible = "atmel,at91sam9260-rtt";
> > 	reg = <0xfffffd20 0x10>;
> > 	interrupts = <1 4 7>;
> > 	clocks = <&clk32k>;
> > 	atmel,time-reg = <&gpbr 0x0>;
> > };
> > 
> > gpbr: syscon@fffffd50 {
> > 	compatible = "atmel,at91sam9260-gpbr", "syscon";
> > 	reg = <0xfffffd50 0x10>;
> > 	
> > };
> 
> Yes, this essentially what I suggested in the thread (and my last reply)
> and relying on syscon rather than a custom driver seems like a good
> idea. It would allow early access to the registers too with the recently
> proposed changes. It would not guarantee any kind of exclusivity,
> though, but I guess that's tolerable?

Yep, that's one of the concern I had with the syscon/regmap
approach :-(, but I guess I'll give this solution a try and post a new
version of this series ;-).

Can we just leave the rtt as an rtc problem on the side for now and bind
it to the rtc-at91sam9 driver.

If we ever decide to add a new driver using the RTT for another purpose
we will still be able to reference the RTT block like this (and keep
the existing rtt node definition):


rtt-based-rtc {
	compatible = "atmel,rtt-rtc";
	atmel,rtt = <&rtt>;
	atmel,time-reg = <&gpbr 0x0>;
}

rtt-based-xdev {
	compatible = "atmel,rtt-xdev";
	atmel,rtt = <&rtt>;
	/*...*/
}

Regards,

Boris

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

Re: [PATCH 5/5] rtc: at91sam9: add DT bindings documentation

[Johan Hovold]

On Wed, Sep 10, 2014 at 05:31:14PM +0200, Boris BREZILLON wrote:
> On Wed, 10 Sep 2014 17:07:02 +0200
> Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> > Yes, this essentially what I suggested in the thread (and my last reply)
> > and relying on syscon rather than a custom driver seems like a good
> > idea. It would allow early access to the registers too with the recently
> > proposed changes. It would not guarantee any kind of exclusivity,
> > though, but I guess that's tolerable?
> 
> Yep, that's one of the concern I had with the syscon/regmap
> approach :-(, but I guess I'll give this solution a try and post a new
> version of this series ;-).

Perhaps we should see what Nicolas and Jean-Christophe says before
rushing into anything (again). ;)

I remember J-C considered loosing track of what was using a particular
backup register to be a regression. But I guess you can't have it both
ways (e.g. if you also want the early access soon provided by syscon).

I'll refresh my rtt and gmbr-node patches meanwhile, as they should be
needed in some form at least.

> Can we just leave the rtt as an rtc problem on the side for now and bind
> it to the rtc-at91sam9 driver.
> 
> If we ever decide to add a new driver using the RTT for another purpose
> we will still be able to reference the RTT block like this (and keep
> the existing rtt node definition):
> 
> rtt-based-rtc {
> 	compatible = "atmel,rtt-rtc";
> 	atmel,rtt = <&rtt>;
> 	atmel,time-reg = <&gpbr 0x0>;
> }

But why not do this from the start?

> rtt-based-xdev {
> 	compatible = "atmel,rtt-xdev";
> 	atmel,rtt = <&rtt>;
> 	/*...*/
> }

Johan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] rtc: at91sam9: add DT bindings documentation

[Nicolas Ferre]

On 10/09/2014 17:52, Johan Hovold :
> On Wed, Sep 10, 2014 at 05:31:14PM +0200, Boris BREZILLON wrote:
>> On Wed, 10 Sep 2014 17:07:02 +0200
>> Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
>>> Yes, this essentially what I suggested in the thread (and my last reply)
>>> and relying on syscon rather than a custom driver seems like a good
>>> idea. It would allow early access to the registers too with the recently
>>> proposed changes. It would not guarantee any kind of exclusivity,
>>> though, but I guess that's tolerable?
>>
>> Yep, that's one of the concern I had with the syscon/regmap
>> approach :-(, but I guess I'll give this solution a try and post a new
>> version of this series ;-).
> 
> Perhaps we should see what Nicolas and Jean-Christophe says before
> rushing into anything (again). ;)

I said and say it again: keep it simple: if gpbr 0 is used by bootloader
to pass information about the boot media, use gpbr 1, without protection
without anything fancy, please.

atmel,at91-rtt-as-rtc-gpbr = <1>;
is good for me.

We can decide to keep the DT binding as "unstable" and see what happen
in one year from now (I suspect nothing will happen). The result is that
we will have a simple update of this driver without new API or new
sub-system to learn and maintain.

So, all in all, I would just take what Johan or Boris did, and go with
this, now!


> I remember J-C considered loosing track of what was using a particular
> backup register to be a regression. But I guess you can't have it both
> ways (e.g. if you also want the early access soon provided by syscon).
> 
> I'll refresh my rtt and gmbr-node patches meanwhile, as they should be
> needed in some form at least.
> 
>> Can we just leave the rtt as an rtc problem on the side for now and bind
>> it to the rtc-at91sam9 driver.
>>
>> If we ever decide to add a new driver using the RTT for another purpose
>> we will still be able to reference the RTT block like this (and keep
>> the existing rtt node definition):
>>
>> rtt-based-rtc {
>> 	compatible = "atmel,rtt-rtc";
>> 	atmel,rtt = <&rtt>;
>> 	atmel,time-reg = <&gpbr 0x0>;
>> }
> 
> But why not do this from the start?
> 
>> rtt-based-xdev {
>> 	compatible = "atmel,rtt-xdev";
>> 	atmel,rtt = <&rtt>;
>> 	/*...*/
>> }
> 
> Johan
> 
> 


-- 
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] rtc: at91sam9: add DT bindings documentation

[Boris BREZILLON]

On Wed, 10 Sep 2014 17:07:02 +0200
Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> On Wed, Sep 10, 2014 at 03:20:19PM +0200, Boris BREZILLON wrote:
> > On Wed, 10 Sep 2014 14:14:24 +0200
> > Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> > > This does not describe the hardware, but rather a specific software
> > > configuration.
> > > 
> > > The RTT is first of all not an RTC (although it can be used as one in a
> > > specific software configuration). And the second register resource above
> > > is not an RTT register, but a general-purpose backup register could be
> > > used for other purposes (which register to use is currently configurable
> > > for legacy booting using CONFIG_RTC_DRV_AT91SAM9_GPBR).
> > 
> > We could use a syscon device (which exposes a regmap) for the GPBR
> > block.
> > 
> > rtc@ffffff20 {
> 
> rtt
> 
> > 	compatible = "atmel,at91sam9260-rtt";
> > 	reg = <0xfffffd20 0x10>;
> > 	interrupts = <1 4 7>;
> > 	clocks = <&clk32k>;
> > 	atmel,time-reg = <&gpbr 0x0>;
> > };
> > 
> > gpbr: syscon@fffffd50 {
> > 	compatible = "atmel,at91sam9260-gpbr", "syscon";
> > 	reg = <0xfffffd50 0x10>;
> > 	
> > };
> 
> Yes, this essentially what I suggested in the thread (and my last reply)
> and relying on syscon rather than a custom driver seems like a good
> idea. It would allow early access to the registers too with the recently
> proposed changes. It would not guarantee any kind of exclusivity,
> though, but I guess that's tolerable?

I know about the "mfd: syscon: Decouple syscon interface from platform
devices" series, but I wonder why we would need to access GPBR
registers during early boot stages. Do you have something in mind :-)?

> 
> Johan



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] rtc: at91sam9: add DT bindings documentation

[Johan Hovold]

On Wed, Sep 10, 2014 at 05:35:32PM +0200, Boris BREZILLON wrote:
> On Wed, 10 Sep 2014 17:07:02 +0200 Johan Hovold <johan@kernel.org> wrote:

> > Yes, this essentially what I suggested in the thread (and my last reply)
> > and relying on syscon rather than a custom driver seems like a good
> > idea. It would allow early access to the registers too with the recently
> > proposed changes. It would not guarantee any kind of exclusivity,
> > though, but I guess that's tolerable?
> 
> I know about the "mfd: syscon: Decouple syscon interface from platform
> devices" series, but I wonder why we would need to access GPBR
> registers during early boot stages. Do you have something in mind :-)?

Yeah, that's what I was referring to. In the thread from last year,
Jean-Christophe mentioned something about barebox using the
backup-registers. Not sure about the details, though.

Johan

Re: [PATCH 0/5] rtc: at91sam9: add DT support

[Boris BREZILLON]

On Wed,  3 Sep 2014 10:45:29 +0200
Boris BREZILLON <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:

> Hello,
> 
> This patch series adds DT support to the atmel at91sam9 RTC driver.
> 
> It also removes any machine specific inclusions to prepare the migration
> to multi platform kernel support, and retain the slow clock to prevent
> the CCF from disabling it at the end of boot.

Oops, just noticed there already was a proposal adding DT support to
this driver [1].
Adding Johan and Douglas in Cc.

[1]http://lists.infradead.org/pipermail/linux-arm-kernel/2013-April/160453.html

> 
> Best Regards,
> 
> Boris
> 
> Boris BREZILLON (5):
>   rtc: at91sam9: remove references to mach specific headers
>   rtc: at91sam9: use standard readl/writel functions instead of raw
>     versions
>   rtc: at91sam9: add DT support
>   rtc: at91sam9: retain slow clock and check its rate
>   rtc: at91sam9: add DT bindings documentation
> 
>  .../devicetree/bindings/rtc/atmel,at91sam9-rtc.txt | 20 +++++++
>  drivers/rtc/rtc-at91sam9.c                         | 63 +++++++++++++++++++---
>  2 files changed, 75 insertions(+), 8 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/rtc/atmel,at91sam9-rtc.txt
> 



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html