[2/5] rtc: Add rtc-sh

Submitted by Chris Brandt on March 16, 2017, 5:51 p.m.

Details

Message ID 20170316175112.27913-3-chris.brandt@renesas.com
State Superseded
Headers show

Commit Message

Chris Brandt March 16, 2017, 5:51 p.m.
rtc-sh is a RTC for SuperH and RZ/A SoCs.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 Documentation/devicetree/bindings/rtc/rtc-sh.txt | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/rtc-sh.txt

Comments

Geert Uytterhoeven March 17, 2017, 8:17 a.m.
Hi Chris,

On Thu, Mar 16, 2017 at 6:51 PM, Chris Brandt <chris.brandt@renesas.com> wrote:
> rtc-sh is a RTC for SuperH and RZ/A SoCs.

an RTC?

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/rtc-sh.txt
> @@ -0,0 +1,19 @@
> +* Real Time Clock for Renesas SH and ARM SoCs
> +
> +Required properties:
> +- compatible: Should be "renesas,r7s72100-rtc" or "renesas,sh-rtc" as a

s/or/and/

> +  fallback.
> +- reg: physical base address and length of memory mapped region.
> +- interrupts: 3 interrupts for alarm, period, and carry.

If there's a need for multiple interrupts, it's always a good idea to use
interrupt-names. The datasheet calls them "arm", "prd", and "cup", but
I wouldn't object against "alarm", "period", and "carry" ;-)

> +- clocks: the clock source for the RTC controller.

The datasheet mentions 3 possible clock sources?

> +Example:
> +rtc: rtc@fcff1000 {
> +       compatible = "renesas,r7s72100-rtc", "renesas,sh-rtc";
> +       reg = <0xfcff1000 0x2e>;
> +       interrupts = <GIC_SPI 276 IRQ_TYPE_EDGE_RISING
> +                     GIC_SPI 277 IRQ_TYPE_EDGE_RISING
> +                     GIC_SPI 278 IRQ_TYPE_EDGE_RISING>;
> +       clocks = <&mstp6_clks R7S72100_CLK_RTC>;
> +       power-domains = <&cpg_clocks>;
> +};

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Chris Brandt March 17, 2017, 1:16 p.m.
Hi Geert,


Happy St Patrick's Day!

    (I know you are not Irish....but you are closer to Ireland than I am ;)


On Friday, March 17, 2017, Geert Uytterhoeven wrote:
> On Thu, Mar 16, 2017 at 6:51 PM, Chris Brandt <chris.brandt@renesas.com>
> wrote:
> > rtc-sh is a RTC for SuperH and RZ/A SoCs.
> 
> an RTC?

Good point. That's more better ;)


> > +Required properties:
> > +- compatible: Should be "renesas,r7s72100-rtc" or "renesas,sh-rtc" as
> > +a
> 
> s/or/and/

OK


> > +  fallback.
> > +- reg: physical base address and length of memory mapped region.
> > +- interrupts: 3 interrupts for alarm, period, and carry.
> 
> If there's a need for multiple interrupts, it's always a good idea to use
> interrupt-names. The datasheet calls them "arm", "prd", and "cup", but I
> wouldn't object against "alarm", "period", and "carry" ;-)

OK. I'll change.
I like the longer names as well ("prd" looks ugly)


> > +- clocks: the clock source for the RTC controller.
> 
> The datasheet mentions 3 possible clock sources?

There is the clock source that does the counting (RTC_X1, RTC_X3, XTAL), and then a clock source that runs the register interface (run off of the p0 clock). So I just need to turn on the p0 clock so I can get at the registers. Note that if you do a hard reset with a RESET button, and all the registers in the chip go back to their POR state, the counter still counts off of the 32KHz XTAL even though the "peripheral clock" is stopped (which just means you can't get to the registers).

In reality, you have to assume the RTC counting source has already been set up by u-boot, or from a past reboot, so I don't do any of that select configuration at all.


Chris
Geert Uytterhoeven March 17, 2017, 1:21 p.m.
Hi Chris,

On Fri, Mar 17, 2017 at 2:16 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> Happy St Patrick's Day!
>
>     (I know you are not Irish....but you are closer to Ireland than I am ;)

;-)

> On Friday, March 17, 2017, Geert Uytterhoeven wrote:
>> On Thu, Mar 16, 2017 at 6:51 PM, Chris Brandt <chris.brandt@renesas.com>
>> wrote:
>> > +- clocks: the clock source for the RTC controller.
>>
>> The datasheet mentions 3 possible clock sources?
>
> There is the clock source that does the counting (RTC_X1, RTC_X3, XTAL), and then a clock source that runs the register interface (run off of the p0 clock). So I just need to turn on the p0 clock so I can get at the registers. Note that if you do a hard reset with a RESET button, and all the registers in the chip go back to their POR state, the counter still counts off of the 32KHz XTAL even though the "peripheral clock" is stopped (which just means you can't get to the registers).
>
> In reality, you have to assume the RTC counting source has already been set up by u-boot, or from a past reboot, so I don't do any of that select configuration at all.

it would still be good to have phandles to the external clock sources as well,
as that describes the hardware topology.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Chris Brandt March 17, 2017, 1:41 p.m.
Hi Geert,

On Friday, March 17, 2017, Geert Uytterhoeven wrote:
> > On Friday, March 17, 2017, Geert Uytterhoeven wrote:
> >> On Thu, Mar 16, 2017 at 6:51 PM, Chris Brandt
> >> <chris.brandt@renesas.com>
> >> wrote:
> >> > +- clocks: the clock source for the RTC controller.
> >>
> >> The datasheet mentions 3 possible clock sources?
> >
> > There is the clock source that does the counting (RTC_X1, RTC_X3, XTAL),
> and then a clock source that runs the register interface (run off of the
> p0 clock). So I just need to turn on the p0 clock so I can get at the
> registers. Note that if you do a hard reset with a RESET button, and all
> the registers in the chip go back to their POR state, the counter still
> counts off of the 32KHz XTAL even though the "peripheral clock" is stopped
> (which just means you can't get to the registers).
> >
> > In reality, you have to assume the RTC counting source has already been
> set up by u-boot, or from a past reboot, so I don't do any of that select
> configuration at all.
> 
> it would still be good to have phandles to the external clock sources as
> well, as that describes the hardware topology.

I'm confused, you mean make new clocks node for RTC_X1 (fixed at 32.768kHZ) and
RTC_X3 (fixed at 4MHz), but then not really do anything with then? (the driver
doesn't need them)


Chris
Geert Uytterhoeven March 17, 2017, 1:55 p.m.
Hi Chris,

On Fri, Mar 17, 2017 at 2:41 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Friday, March 17, 2017, Geert Uytterhoeven wrote:
>> > On Friday, March 17, 2017, Geert Uytterhoeven wrote:
>> >> On Thu, Mar 16, 2017 at 6:51 PM, Chris Brandt
>> >> <chris.brandt@renesas.com>
>> >> wrote:
>> >> > +- clocks: the clock source for the RTC controller.
>> >>
>> >> The datasheet mentions 3 possible clock sources?
>> >
>> > There is the clock source that does the counting (RTC_X1, RTC_X3, XTAL),
>> and then a clock source that runs the register interface (run off of the
>> p0 clock). So I just need to turn on the p0 clock so I can get at the
>> registers. Note that if you do a hard reset with a RESET button, and all
>> the registers in the chip go back to their POR state, the counter still
>> counts off of the 32KHz XTAL even though the "peripheral clock" is stopped
>> (which just means you can't get to the registers).
>> >
>> > In reality, you have to assume the RTC counting source has already been
>> set up by u-boot, or from a past reboot, so I don't do any of that select
>> configuration at all.
>>
>> it would still be good to have phandles to the external clock sources as
>> well, as that describes the hardware topology.
>
> I'm confused, you mean make new clocks node for RTC_X1 (fixed at 32.768kHZ) and
> RTC_X3 (fixed at 4MHz), but then not really do anything with then? (the driver

You still do something with them, as they'll be linked from the rtc device node.

> doesn't need them)

The driver may use them in the future.
E.g. to select among X1, X3, or EXTAL, depending on availability.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Alexandre Belloni March 17, 2017, 3:05 p.m.
Hi,

On 17/03/2017 at 14:55:48 +0100, Geert Uytterhoeven wrote:
> >> it would still be good to have phandles to the external clock sources as
> >> well, as that describes the hardware topology.
> >
> > I'm confused, you mean make new clocks node for RTC_X1 (fixed at 32.768kHZ) and
> > RTC_X3 (fixed at 4MHz), but then not really do anything with then? (the driver
> 
> You still do something with them, as they'll be linked from the rtc device node.
> 
> > doesn't need them)
> 
> The driver may use them in the future.
> E.g. to select among X1, X3, or EXTAL, depending on availability.
> 

I'm on Geert's side here, I wouldn't rely on the bootloader to set
everything up correctly.
Chris Brandt March 20, 2017, 3:05 p.m.
Hi Alexandre and Geert,


On Friday, March 17, 2017, Alexandre Belloni wrote:
> On 17/03/2017 at 14:55:48 +0100, Geert Uytterhoeven wrote:
> > >> it would still be good to have phandles to the external clock
> > >> sources as well, as that describes the hardware topology.
> > >
> > > I'm confused, you mean make new clocks node for RTC_X1 (fixed at
> > > 32.768kHZ) and
> > > RTC_X3 (fixed at 4MHz), but then not really do anything with then?
> > > (the driver
> >
> > You still do something with them, as they'll be linked from the rtc
> device node.
> >
> > > doesn't need them)
> >
> > The driver may use them in the future.
> > E.g. to select among X1, X3, or EXTAL, depending on availability.
> >
> 
> I'm on Geert's side here, I wouldn't rely on the bootloader to set
> everything up correctly.

Part of my hesitation to putting in the clock source selection was that
this RTC block is used in a lot of parts, and it's always the same,
except for the last 3 registers which are different from device to
device because different SoCs have different clock sources options
and ways to set them up. So, making a driver that just 'reads' the
time seemed safer/easier.
Anyway, I'll add the clock setup in if that's the general consensus.


But, before I send out an updated version, what were you thinking
my DT should look like?

Should I make a "count-source" property for user to select what clock
will be used for the actually counting?


	clocks {
		ranges;
		#address-cells = <1>;
		#size-cells = <1>;
            . . .

+		rtc_x1_clk: rtc_x1 {
+			#clock-cells = <0>;
+			compatible = "fixed-clock";
+			/* If clk present, value must be set by board to 32678 */
+			clock-frequency = <0>;
+		};
+
+		rtc_x3_clk: rtc_x3 {
+			#clock-cells = <0>;
+			compatible = "fixed-clock";
+			/* If clk present, value must be set by board to 4000000 */
+			clock-frequency = <0>;
+		};
+




+	rtc: rtc@fcff1000 {
+		compatible = "renesas,r7s72100-rtc", "renesas,sh-rtc";
+		reg = <0xfcff1000 0x2e>;
+		interrupts = <GIC_SPI 276 IRQ_TYPE_EDGE_RISING
+			      GIC_SPI 277 IRQ_TYPE_EDGE_RISING
+			      GIC_SPI 278 IRQ_TYPE_EDGE_RISING>;
+		interrupt-names = "alarm", "period", "carry";
+		clocks = <&mstp6_clks R7S72100_CLK_RTC>, <&rtc_x1_clk>,
+			  <&extal_clk> , <&rtc_x3_clk>;
+		clock-names = "fck", "rtc_x1", "extal", "rtc_x3";
+		power-domains = <&cpg_clocks>;
+		count-source = "rtc_x1";    <<<<<<< this would be in the board dts file
+		status = "disabled";
+	};


Thank you,
Chris
Geert Uytterhoeven March 20, 2017, 4:03 p.m.
Hi Chris,

On Mon, Mar 20, 2017 at 4:05 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Friday, March 17, 2017, Alexandre Belloni wrote:
>> On 17/03/2017 at 14:55:48 +0100, Geert Uytterhoeven wrote:
>> > >> it would still be good to have phandles to the external clock
>> > >> sources as well, as that describes the hardware topology.
>> > > I'm confused, you mean make new clocks node for RTC_X1 (fixed at
>> > > 32.768kHZ) and
>> > > RTC_X3 (fixed at 4MHz), but then not really do anything with then?
>> > > (the driver
>> >
>> > You still do something with them, as they'll be linked from the rtc
>> device node.
>> >
>> > > doesn't need them)
>> >
>> > The driver may use them in the future.
>> > E.g. to select among X1, X3, or EXTAL, depending on availability.
>>
>> I'm on Geert's side here, I wouldn't rely on the bootloader to set
>> everything up correctly.
>
> Part of my hesitation to putting in the clock source selection was that
> this RTC block is used in a lot of parts, and it's always the same,
> except for the last 3 registers which are different from device to
> device because different SoCs have different clock sources options
> and ways to set them up. So, making a driver that just 'reads' the
> time seemed safer/easier.
> Anyway, I'll add the clock setup in if that's the general consensus.
>
> But, before I send out an updated version, what were you thinking
> my DT should look like?
>
> Should I make a "count-source" property for user to select what clock
> will be used for the actually counting?
>
>
>         clocks {
>                 ranges;
>                 #address-cells = <1>;
>                 #size-cells = <1>;
>             . . .
>
> +               rtc_x1_clk: rtc_x1 {
> +                       #clock-cells = <0>;
> +                       compatible = "fixed-clock";
> +                       /* If clk present, value must be set by board to 32678 */
> +                       clock-frequency = <0>;
> +               };
> +
> +               rtc_x3_clk: rtc_x3 {
> +                       #clock-cells = <0>;
> +                       compatible = "fixed-clock";
> +                       /* If clk present, value must be set by board to 4000000 */
> +                       clock-frequency = <0>;
> +               };
> +
>
>
>
>
> +       rtc: rtc@fcff1000 {
> +               compatible = "renesas,r7s72100-rtc", "renesas,sh-rtc";
> +               reg = <0xfcff1000 0x2e>;
> +               interrupts = <GIC_SPI 276 IRQ_TYPE_EDGE_RISING
> +                             GIC_SPI 277 IRQ_TYPE_EDGE_RISING
> +                             GIC_SPI 278 IRQ_TYPE_EDGE_RISING>;
> +               interrupt-names = "alarm", "period", "carry";
> +               clocks = <&mstp6_clks R7S72100_CLK_RTC>, <&rtc_x1_clk>,
> +                         <&extal_clk> , <&rtc_x3_clk>;
> +               clock-names = "fck", "rtc_x1", "extal", "rtc_x3";
> +               power-domains = <&cpg_clocks>;
> +               count-source = "rtc_x1";    <<<<<<< this would be in the board dts file

IMHO that's software policy, not hardware description, so it doesn't
belong in DT.

However, by scanning the list of allowed clock names, and looking at clock
rates, the driver can find a suitable clock itself.

> +               status = "disabled";
> +       };

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Chris Brandt March 20, 2017, 4:26 p.m.
Hi Geert,

On Monday, March 20, 2017, Geert Uytterhoeven wrote:
> > +       rtc: rtc@fcff1000 {
> > +               compatible = "renesas,r7s72100-rtc", "renesas,sh-rtc";
> > +               reg = <0xfcff1000 0x2e>;
> > +               interrupts = <GIC_SPI 276 IRQ_TYPE_EDGE_RISING
> > +                             GIC_SPI 277 IRQ_TYPE_EDGE_RISING
> > +                             GIC_SPI 278 IRQ_TYPE_EDGE_RISING>;
> > +               interrupt-names = "alarm", "period", "carry";
> > +               clocks = <&mstp6_clks R7S72100_CLK_RTC>, <&rtc_x1_clk>,
> > +                         <&extal_clk> , <&rtc_x3_clk>;
> > +               clock-names = "fck", "rtc_x1", "extal", "rtc_x3";
> > +               power-domains = <&cpg_clocks>;
> > +               count-source = "rtc_x1";    <<<<<<< this would be in the
> board dts file
> 
> IMHO that's software policy, not hardware description, so it doesn't
> belong in DT.

Really? I'm so confused about what is and isn't the responsibility of DT.


> However, by scanning the list of allowed clock names, and looking at clock
> rates, the driver can find a suitable clock itself.

Yes, I could make the selection automatic by looking that the clock names (and
I will do that for this driver), but what about the case where a user needs
to select something from a list of possible HW options? In other words, where
does 'software option' selection stuff go if not in the DT?


Thanks,
Chris
Alexandre Belloni March 20, 2017, 10:45 p.m.
On 20/03/2017 at 16:26:46 +0000, Chris Brandt wrote:
> Hi Geert,
> 
> On Monday, March 20, 2017, Geert Uytterhoeven wrote:
> > > +       rtc: rtc@fcff1000 {
> > > +               compatible = "renesas,r7s72100-rtc", "renesas,sh-rtc";
> > > +               reg = <0xfcff1000 0x2e>;
> > > +               interrupts = <GIC_SPI 276 IRQ_TYPE_EDGE_RISING
> > > +                             GIC_SPI 277 IRQ_TYPE_EDGE_RISING
> > > +                             GIC_SPI 278 IRQ_TYPE_EDGE_RISING>;
> > > +               interrupt-names = "alarm", "period", "carry";
> > > +               clocks = <&mstp6_clks R7S72100_CLK_RTC>, <&rtc_x1_clk>,
> > > +                         <&extal_clk> , <&rtc_x3_clk>;
> > > +               clock-names = "fck", "rtc_x1", "extal", "rtc_x3";
> > > +               power-domains = <&cpg_clocks>;
> > > +               count-source = "rtc_x1";    <<<<<<< this would be in the
> > board dts file
> > 
> > IMHO that's software policy, not hardware description, so it doesn't
> > belong in DT.
> 
> Really? I'm so confused about what is and isn't the responsibility of DT.
> 
> 
> > However, by scanning the list of allowed clock names, and looking at clock
> > rates, the driver can find a suitable clock itself.
> 
> Yes, I could make the selection automatic by looking that the clock names (and
> I will do that for this driver), but what about the case where a user needs
> to select something from a list of possible HW options? In other words, where
> does 'software option' selection stuff go if not in the DT?
> 

If you want you can leave the clock selection logic out of the driver as
long as the DT binding is complete. You dont have to use everything that
is described in the DT.

Regarding the selection, I'd say take the first that is present and not
0 however, if something has already been set, you may not want to
override it. (I know, this is not helping a lot ;) )

Patch hide | download patch | download mbox

diff --git a/Documentation/devicetree/bindings/rtc/rtc-sh.txt b/Documentation/devicetree/bindings/rtc/rtc-sh.txt
new file mode 100644
index 0000000..2f0dc6c
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/rtc-sh.txt
@@ -0,0 +1,19 @@ 
+* Real Time Clock for Renesas SH and ARM SoCs
+
+Required properties:
+- compatible: Should be "renesas,r7s72100-rtc" or "renesas,sh-rtc" as a
+  fallback.
+- reg: physical base address and length of memory mapped region.
+- interrupts: 3 interrupts for alarm, period, and carry.
+- clocks: the clock source for the RTC controller.
+
+Example:
+rtc: rtc@fcff1000 {
+	compatible = "renesas,r7s72100-rtc", "renesas,sh-rtc";
+	reg = <0xfcff1000 0x2e>;
+	interrupts = <GIC_SPI 276 IRQ_TYPE_EDGE_RISING
+		      GIC_SPI 277 IRQ_TYPE_EDGE_RISING
+		      GIC_SPI 278 IRQ_TYPE_EDGE_RISING>;
+	clocks = <&mstp6_clks R7S72100_CLK_RTC>;
+	power-domains = <&cpg_clocks>;
+};