| Message ID | 20171205140646.30367-2-linux-kernel-dev@beckhoff.com |
|---|---|
| State | New |
| Headers | show |
| Series | add mxc driver for i.MX53 SRTC | expand |
On Tue, Dec 05, 2017 at 03:06:42PM +0100, linux-kernel-dev@beckhoff.com wrote: > From: Patrick Bruenn <p.bruenn@beckhoff.com> > > Document the binding for i.MX53 SRTC implemented by rtc-mxc_v2 > > Signed-off-by: Patrick Bruenn <p.bruenn@beckhoff.com><Paste> > > --- > > To: Alessandro Zummo <a.zummo@towertech.it> > To: Alexandre Belloni <alexandre.belloni@free-electrons.com> > > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) > Cc: linux-rtc@vger.kernel.org (open list:REAL TIME CLOCK (RTC) SUBSYSTEM) > Cc: devicetree@vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) > Cc: linux-kernel@vger.kernel.org (open list) > Cc: Fabio Estevam <fabio.estevam@nxp.com> > Cc: Juergen Borleis <jbe@pengutronix.de> > Cc: Noel Vellemans <Noel.Vellemans@visionbms.com> > Cc: Shawn Guo <shawnguo@kernel.org> > Cc: Sascha Hauer <kernel@pengutronix.de> (maintainer:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE) > Cc: Russell King <linux@armlinux.org.uk> (maintainer:ARM PORT) > Cc: linux-arm-kernel@lists.infradead.org (moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE) > > Cc: Philippe Ombredanne <pombredanne@nexb.com> > Cc: Lothar Waßmann <LW@KARO-electronics.de> > --- > Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > create mode 100644 Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt > > diff --git a/Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt b/Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt > new file mode 100644 > index 000000000000..796e7f4995db > --- /dev/null > +++ b/Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt > @@ -0,0 +1,17 @@ > +* i.MX53 Real Time Clock controller > + > +Required properties: > +- compatible: should be: "fsl,imx53-rtc" > +- reg: physical base address of the controller and length of memory mapped > + region. > +- clocks: should contain the phandle for the rtc clock Shouldn't there be more than 1 here. From what I remember, the ipg clock and the 32k clock? > +- interrupts: rtc alarm interrupt > + > +Example: > + > +srtc@53fa4000 { rtc@... > + compatible = "fsl,imx53-rtc"; > + reg = <0x53fa4000 0x4000>; > + interrupts = <24>; > + clocks = <&clks IMX5_CLK_SRTC_GATE>; > +}; > -- > 2.11.0
>From: Rob Herring [mailto:robh@kernel.org] >Sent: Mittwoch, 6. Dezember 2017 22:55 >On Tue, Dec 05, 2017 at 03:06:42PM +0100, linux-kernel-dev@beckhoff.com >wrote: >> From: Patrick Bruenn <p.bruenn@beckhoff.com> >> >> +++ b/Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt >> @@ -0,0 +1,17 @@ >> +* i.MX53 Real Time Clock controller >> + >> +Required properties: >> +- compatible: should be: "fsl,imx53-rtc" >> +- reg: physical base address of the controller and length of memory >mapped >> + region. >> +- clocks: should contain the phandle for the rtc clock > >Shouldn't there be more than 1 here. From what I remember, the ipg clock >and the 32k clock? Yes, you are right. But if I am reading the original Freescale driver and the reference manual correctly, the second clock is always active. Section "72.3.1.1 Clocks" from the reference manual [1] reads: "SRTC runs on two clock sources, high frequency peripherals clock and low frequency timers clock. The high frequency peripheral IP clock is used by the SRTC peripheral bus interface, control and status registers. The low frequency 32.768 kHz clock is the always-active clock used by the SRTC timers..." That's why I would only include one clock . Should I change this? > >> +- interrupts: rtc alarm interrupt >> + >> +Example: >> + >> +srtc@53fa4000 { > >rtc@... > The rtc for which this series adds support is embedded within a function block called "Secure Real Time Clock". This driver doesn't utilize all of the hardware features by now. But maybe someone else wants to extend the functionalities, later. For that possibility I wanted to name the node "srtc". Should I still change this? I believe you have a much better understanding of what should be done here. I don't want to argue with you, just thought you might not had that information. So if I am wrong just tell me and I will change it without further "complaining". Thank you for taking the time, Patrick [1] https://cache.freescale.com/files/32bit/doc/ref_manual/iMX53RM.pdf --- Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff Registered office: Verl, Germany | Register court: Guetersloh HRA 7075
Hi Patrick, On Mon, Dec 11, 2017 at 5:08 AM, Patrick Brünn <P.Bruenn@beckhoff.com> wrote: >>rtc@... >> > The rtc for which this series adds support is embedded within a function block called > "Secure Real Time Clock". This driver doesn't utilize all of the hardware features by > now. But maybe someone else wants to extend the functionalities, later. > For that possibility I wanted to name the node "srtc". Should I still change this? > > I believe you have a much better understanding of what should be done here. I don't > want to argue with you, just thought you might not had that information. So if I am > wrong just tell me and I will change it without further "complaining". From the Devicetree Specification document: "Generic Names Recommendation The name of a node should be somewhat generic, reflecting the function of the device and not its precise program- ming model. If appropriate, the name should be one of the following choices: ... rtc " So better use 'rtc' as suggested by Rob.
>From: Fabio Estevam [mailto:festevam@gmail.com] >Sent: Dienstag, 12. Dezember 2017 00:08 >Hi Patrick, > Hi Fabio, >On Mon, Dec 11, 2017 at 5:08 AM, Patrick Brünn <P.Bruenn@beckhoff.com> >wrote: > >>>rtc@... >>> >> The rtc for which this series adds support is embedded within a function >block called >> "Secure Real Time Clock". This driver doesn't utilize all of the hardware >features by >> now. But maybe someone else wants to extend the functionalities, later. >> For that possibility I wanted to name the node "srtc". Should I still change >this? >> >> I believe you have a much better understanding of what should be done >here. I don't >> want to argue with you, just thought you might not had that information. So >if I am >> wrong just tell me and I will change it without further "complaining". > >From the Devicetree Specification document: > >"Generic Names Recommendation > >The name of a node should be somewhat generic, reflecting the function >of the device and not its precise program- >ming model. If appropriate, the name should be one of the following choices: >... >rtc >" > >So better use 'rtc' as suggested by Rob. Thanks for this clarification. I will wait a few days for more comments on the rest of the driver and then send a v4. Regards, Patrick Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff Registered office: Verl, Germany | Register court: Guetersloh HRA 7075
On Mon, Dec 11, 2017 at 1:08 AM, Patrick Brünn <P.Bruenn@beckhoff.com> wrote: >>From: Rob Herring [mailto:robh@kernel.org] >>Sent: Mittwoch, 6. Dezember 2017 22:55 >>On Tue, Dec 05, 2017 at 03:06:42PM +0100, linux-kernel-dev@beckhoff.com >>wrote: >>> From: Patrick Bruenn <p.bruenn@beckhoff.com> >>> >>> +++ b/Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt >>> @@ -0,0 +1,17 @@ >>> +* i.MX53 Real Time Clock controller >>> + >>> +Required properties: >>> +- compatible: should be: "fsl,imx53-rtc" >>> +- reg: physical base address of the controller and length of memory >>mapped >>> + region. >>> +- clocks: should contain the phandle for the rtc clock >> >>Shouldn't there be more than 1 here. From what I remember, the ipg clock >>and the 32k clock? > Yes, you are right. But if I am reading the original Freescale driver and the > reference manual correctly, the second clock is always active. > Section "72.3.1.1 Clocks" from the reference manual [1] reads: > "SRTC runs on two clock sources, high frequency peripherals clock and low frequency > timers clock. The high frequency peripheral IP clock is used by the SRTC peripheral bus > interface, control and status registers. The low frequency 32.768 kHz clock is the > always-active clock used by the SRTC timers..." > > That's why I would only include one clock . Should I change this? Some chips supported 32.0kHz and 32.768kHz low freq clocks, so you'd need to be able to query what the frequency is even if you don't need to enable the clock, but maybe that may be gone in MX53. I don't remember. >>> +- interrupts: rtc alarm interrupt >>> + >>> +Example: >>> + >>> +srtc@53fa4000 { >> >>rtc@... >> > The rtc for which this series adds support is embedded within a function block called > "Secure Real Time Clock". This driver doesn't utilize all of the hardware features by > now. But maybe someone else wants to extend the functionalities, later. > For that possibility I wanted to name the node "srtc". Should I still change this? > > I believe you have a much better understanding of what should be done here. I don't > want to argue with you, just thought you might not had that information. So if I am > wrong just tell me and I will change it without further "complaining". Node names should be generic for the class of h/w they are. So yes, it should be "rtc". Rob
diff --git a/Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt b/Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt new file mode 100644 index 000000000000..796e7f4995db --- /dev/null +++ b/Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt @@ -0,0 +1,17 @@ +* i.MX53 Real Time Clock controller + +Required properties: +- compatible: should be: "fsl,imx53-rtc" +- reg: physical base address of the controller and length of memory mapped + region. +- clocks: should contain the phandle for the rtc clock +- interrupts: rtc alarm interrupt + +Example: + +srtc@53fa4000 { + compatible = "fsl,imx53-rtc"; + reg = <0x53fa4000 0x4000>; + interrupts = <24>; + clocks = <&clks IMX5_CLK_SRTC_GATE>; +};