| Message ID | 1409733934-14465-6-git-send-email-boris.brezillon@free-electrons.com |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Sep 03, 2014 at 10:45:34AM +0200, Boris BREZILLON wrote: > 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>; > +}; 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
Hi Johan, On Wed, 10 Sep 2014 14:14:24 +0200 Johan Hovold <johan@kernel.org> wrote: > On Wed, Sep 03, 2014 at 10:45:34AM +0200, Boris BREZILLON wrote: > > 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>; > > +}; > > 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
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@kernel.org> wrote: > > > On Wed, Sep 03, 2014 at 10:45:34AM +0200, Boris BREZILLON wrote: > > > 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>; > > > +}; > > > > 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
On Wed, 10 Sep 2014 14:14:24 +0200 Johan Hovold <johan@kernel.org> wrote: > On Wed, Sep 03, 2014 at 10:45:34AM +0200, Boris BREZILLON wrote: > > 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>; > > +}; > > 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
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? Johan
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
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? 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
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@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? > > 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
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
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@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? >> >> 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 > >
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>; +};
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