| Message ID | 20170316175112.27913-3-chris.brandt@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
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
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
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
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
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
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.
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
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
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
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 ;) )
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>; +};
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