[PATCH v2 0/5] add mxc driver for i.MX53 SRTC

[PATCH v2 0/5] add mxc driver for i.MX53 SRTC

[linux-kernel-dev]

From: Patrick Bruenn <p.bruenn@beckhoff.com>

Neither rtc-imxdi, rtc-mxc nor rtc-snvs are compatible with i.MX53.

This is driver enables support for the low power domain SRTC features:
- 32-bit MSB of non-rollover time counter
- 32-bit alarm register

Based on:
http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/drivers/rtc/rtc-mxc_v2.c?h=imx_2.6.35_11.09.01

v2:
- have seperate patches for dt-binding, CONFIG option, imx53.dtsi and driver
- add SPDX-License-Identifier and cleanup copyright notice
- replace __raw_readl/writel() with readl/writel()
- fix PM_SLEEP callbacks
- add CONFIG_RTC_DRV_MXC_V2 to build rtc-mxc_v2.c
- remove misleading or obvious comments and fix style of the remaining
- avoid endless loop while waiting for hw
- implement consistent locking; make spinlock a member of dev struct
- enable clk only for register accesses
- remove all udelay() calls since they are obsolete or redundant
  (we are already waiting for register flags to change)
- init platform_data before registering irq callback
- let set_time() fail, when 32 bit rtc counter exceeded
- make names more consistent
- cleanup and reorder includes
- cleanup and remove unused defines

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>

Patrick Bruenn (5):
  dt-bindings: rtc: add bindings for i.MX53 SRTC
  ARM: dts: imx53: add srtc node
  rtc: mxc_v2: add driver for i.MX53 SRTC
  ARM: imx_v4_v5_defconfig: enable RTC_DRV_MXC_V2
  rtc: add mxc driver for i.MX53 SRTC

 .../devicetree/bindings/rtc/rtc-mxc_v2.txt         |  17 +
 arch/arm/boot/dts/imx53.dtsi                       |   4 +-
 arch/arm/configs/imx_v4_v5_defconfig               |   1 +
 drivers/rtc/Kconfig                                |  10 +
 drivers/rtc/Makefile                               |   1 +
 drivers/rtc/rtc-mxc_v2.c                           | 433 +++++++++++++++++++++
 6 files changed, 463 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/rtc/rtc-mxc_v2.txt
 create mode 100644 drivers/rtc/rtc-mxc_v2.c

-- 
2.11.0

[PATCH v2 1/5] dt-bindings: rtc: add bindings for i.MX53 SRTC

[linux-kernel-dev]

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
+- interrupts: rtc alarm interrupt
+
+Example:
+
+srtc@53fa4000 {
+	compatible = "fsl,imx53-rtc";
+	reg = <0x53fa4000 0x4000>;
+	interrupts = <24>;
+	clocks = <&clks IMX5_CLK_SRTC_GATE>;
+};
-- 
2.11.0

Re: [PATCH v2 1/5] dt-bindings: rtc: add bindings for i.MX53 SRTC

[Rob Herring]

On Tue, Dec 05, 2017 at 03:06:42PM +0100, linux-kernel-dev-QonKdJ6Bx35Wk0Htik3J/w@public.gmane.org wrote:
> From: Patrick Bruenn <p.bruenn-QonKdJ6Bx35Wk0Htik3J/w@public.gmane.org>
> 
> Document the binding for i.MX53 SRTC implemented by rtc-mxc_v2
> 
> Signed-off-by: Patrick Bruenn <p.bruenn-QonKdJ6Bx35Wk0Htik3J/w@public.gmane.org><Paste>
> 
> ---
> 
> To: Alessandro Zummo <a.zummo-BfzFCNDTiLLj+vYz1yj4TQ@public.gmane.org>
> To: Alexandre Belloni <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> 
> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
> Cc: linux-rtc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org (open list:REAL TIME CLOCK (RTC) SUBSYSTEM)
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
> Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org (open list)
> Cc: Fabio Estevam <fabio.estevam-3arQi8VN3Tc@public.gmane.org>
> Cc: Juergen Borleis <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Cc: Noel Vellemans <Noel.Vellemans-8UENEgx6w+makBO8gow8eQ@public.gmane.org>
> Cc: Shawn Guo <shawnguo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> (maintainer:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)
> Cc: Russell King <linux-I+IVW8TIWO2tmTQ+vhA3Yw@public.gmane.org> (maintainer:ARM PORT)
> Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org (moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)
> 
> Cc: Philippe Ombredanne <pombredanne-od1rfyK75/E@public.gmane.org>
> Cc: Lothar Waßmann <LW-bxm8fMRDkQLDiMYJYoSAnRvVK+yQ3ZXh@public.gmane.org>
> ---
>  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
--
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 v2 1/5] dt-bindings: rtc: add bindings for i.MX53 SRTC

[Patrick Brünn]

>From: Rob Herring [mailto:robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org]
>Sent: Mittwoch, 6. Dezember 2017 22:55
>On Tue, Dec 05, 2017 at 03:06:42PM +0100, linux-kernel-dev-QonKdJ6Bx35Wk0Htik3J/w@public.gmane.org
>wrote:
>> From: Patrick Bruenn <p.bruenn-QonKdJ6Bx35Wk0Htik3J/w@public.gmane.org>
>>
>> +++ 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



--
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 v2 1/5] dt-bindings: rtc: add bindings for i.MX53 SRTC

[Fabio Estevam]

Hi Patrick,

On Mon, Dec 11, 2017 at 5:08 AM, Patrick Brünn <P.Bruenn-QonKdJ6Bx35Wk0Htik3J/w@public.gmane.org> 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.
--
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 v2 1/5] dt-bindings: rtc: add bindings for i.MX53 SRTC

[Patrick Brünn]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1610 bytes --]

>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


N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·zøœzÚÞz)í…æèw*\x1fjg¬±¨\x1e¶‰šŽŠÝ¢j.ïÛ°\½½MŽúgjÌæa×\x02››–' ™©Þ¢¸\f¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾\a«‘êçzZ+ƒùšŽŠÝ¢j"ú!¶i

Re: [PATCH v2 1/5] dt-bindings: rtc: add bindings for i.MX53 SRTC

[Rob Herring]

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

[PATCH v2 2/5] ARM: dts: imx53: add srtc node

[linux-kernel-dev]

From: Patrick Bruenn <p.bruenn@beckhoff.com>

rtc-mxc_v2 driver will add support for the i.MX53 SRTC

Signed-off-by: Patrick Bruenn <p.bruenn@beckhoff.com>

---

To: Shawn Guo <shawnguo@kernel.org>
To: Sascha Hauer <kernel@pengutronix.de> (maintainer:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)

Cc: Alessandro Zummo <a.zummo@towertech.it>
Cc: 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: 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>
---
 arch/arm/boot/dts/imx53.dtsi | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/imx53.dtsi b/arch/arm/boot/dts/imx53.dtsi
index 589a67c5f796..e4ca9d9ba2fe 100644
--- a/arch/arm/boot/dts/imx53.dtsi
+++ b/arch/arm/boot/dts/imx53.dtsi
@@ -434,12 +434,10 @@
 			};
 
 			srtc: srtc@53fa4000 {
-				compatible = "fsl,imx53-rtc", "fsl,imx25-rtc";
+				compatible = "fsl,imx53-rtc";
 				reg = <0x53fa4000 0x4000>;
 				interrupts = <24>;
-				interrupt-parent = <&tzic>;
 				clocks = <&clks IMX5_CLK_SRTC_GATE>;
-				clock-names = "ipg";
 			};
 
 			iomuxc: iomuxc@53fa8000 {
-- 
2.11.0

Re: [PATCH v2 2/5] ARM: dts: imx53: add srtc node

[Fabio Estevam]

On Tue, Dec 5, 2017 at 12:06 PM,  <linux-kernel-dev@beckhoff.com> wrote:

>                         srtc: srtc@53fa4000 {
> -                               compatible = "fsl,imx53-rtc", "fsl,imx25-rtc";
> +                               compatible = "fsl,imx53-rtc";
>                                 reg = <0x53fa4000 0x4000>;
>                                 interrupts = <24>;
> -                               interrupt-parent = <&tzic>;
>                                 clocks = <&clks IMX5_CLK_SRTC_GATE>;
> -                               clock-names = "ipg";

srtc node has been removed in linux-next.

You need to re-add it.

RE: [PATCH v2 2/5] ARM: dts: imx53: add srtc node

[Patrick Brünn]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1286 bytes --]

>From: Fabio Estevam [mailto:festevam@gmail.com]
>Sent: Dienstag, 5. Dezember 2017 15:14
>On Tue, Dec 5, 2017 at 12:06 PM,  <linux-kernel-dev@beckhoff.com> wrote:
>
>>                         srtc: srtc@53fa4000 {
>> -                               compatible = "fsl,imx53-rtc", "fsl,imx25-rtc";
>> +                               compatible = "fsl,imx53-rtc";
>>                                 reg = <0x53fa4000 0x4000>;
>>                                 interrupts = <24>;
>> -                               interrupt-parent = <&tzic>;
>>                                 clocks = <&clks IMX5_CLK_SRTC_GATE>;
>> -                               clock-names = "ipg";
>
>srtc node has been removed in linux-next.
>
>You need to re-add it.
Thanks for this super-fast responses!
I will wait a few days for more reviewers and then integrate your comments in a v3. If nothing major show up I will wait until the imx53.dtsi revert landed in Linus tree.

Best regards,
Patrick

Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff
Registered office: Verl, Germany | Register court: Guetersloh HRA 7075


N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·zøœzÚÞz)í…æèw*\x1fjg¬±¨\x1e¶‰šŽŠÝ¢j.ïÛ°\½½MŽúgjÌæa×\x02››–' ™©Þ¢¸\f¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾\a«‘êçzZ+ƒùšŽŠÝ¢j"ú!¶i

Re: [PATCH v2 2/5] ARM: dts: imx53: add srtc node

[Fabio Estevam]

On Tue, Dec 5, 2017 at 12:20 PM, Patrick Brünn <P.Bruenn-QonKdJ6Bx35Wk0Htik3J/w@public.gmane.org> wrote:

> I will wait a few days for more reviewers and then integrate your comments in a v3. If nothing major show up I will wait until the imx53.dtsi revert landed in Linus tree.

It is in Linus' tree now as commit e501506d3ea0 ("Revert "ARM: dts:
imx53: add srtc node"").
--
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 v2 3/5] rtc: mxc_v2: add driver for i.MX53 SRTC

[linux-kernel-dev-QonKdJ6Bx35Wk0Htik3J/w]

From: Patrick Bruenn <p.bruenn-QonKdJ6Bx35Wk0Htik3J/w@public.gmane.org>

Add RTC_DRV_MXC_V2 config option

Signed-off-by: Patrick Bruenn <p.bruenn-QonKdJ6Bx35Wk0Htik3J/w@public.gmane.org>

---

To: Alessandro Zummo <a.zummo-BfzFCNDTiLLj+vYz1yj4TQ@public.gmane.org>
To: Alexandre Belloni <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
Cc: linux-rtc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org (open list:REAL TIME CLOCK (RTC) SUBSYSTEM)
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org (open list)
Cc: Fabio Estevam <fabio.estevam-3arQi8VN3Tc@public.gmane.org>
Cc: Juergen Borleis <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Noel Vellemans <Noel.Vellemans-8UENEgx6w+makBO8gow8eQ@public.gmane.org>
Cc: Shawn Guo <shawnguo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> (maintainer:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)
Cc: Russell King <linux-I+IVW8TIWO2tmTQ+vhA3Yw@public.gmane.org> (maintainer:ARM PORT)
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org (moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)

Cc: Philippe Ombredanne <pombredanne-od1rfyK75/E@public.gmane.org>
Cc: Lothar Waßmann <LW-bxm8fMRDkQLDiMYJYoSAnRvVK+yQ3ZXh@public.gmane.org>
---
 drivers/rtc/Kconfig | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index b59a31b079a5..440edebf5c71 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1689,6 +1689,16 @@ config RTC_DRV_MXC
 	   This driver can also be built as a module, if so, the module
 	   will be called "rtc-mxc".
 
+config RTC_DRV_MXC_V2
+	tristate "Freescale MXC Real Time Clock for i.MX53"
+	depends on ARCH_MXC
+	help
+	   If you say yes here you get support for the Freescale MXC
+	   SRTC module in i.MX53 processor.
+
+	   This driver can also be built as a module, if so, the module
+	   will be called "rtc-mxc_v2".
+
 config RTC_DRV_SNVS
 	tristate "Freescale SNVS RTC support"
 	select REGMAP_MMIO
-- 
2.11.0
--
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 v2 3/5] rtc: mxc_v2: add driver for i.MX53 SRTC

[Fabio Estevam]

On Tue, Dec 5, 2017 at 12:06 PM,  <linux-kernel-dev-QonKdJ6Bx35Wk0Htik3J/w@public.gmane.org> wrote:
> From: Patrick Bruenn <p.bruenn-QonKdJ6Bx35Wk0Htik3J/w@public.gmane.org>
>
> Add RTC_DRV_MXC_V2 config option
>
> Signed-off-by: Patrick Bruenn <p.bruenn-QonKdJ6Bx35Wk0Htik3J/w@public.gmane.org>

You should add the Kconfig as part of the patch that introduces the driver.
--
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 v2 4/5] ARM: imx_v4_v5_defconfig: enable RTC_DRV_MXC_V2

[linux-kernel-dev-QonKdJ6Bx35Wk0Htik3J/w]

From: Patrick Bruenn <p.bruenn-QonKdJ6Bx35Wk0Htik3J/w@public.gmane.org>

Enable SRTC driver for i.MX53 in default config

Signed-off-by: Patrick Bruenn <p.bruenn-QonKdJ6Bx35Wk0Htik3J/w@public.gmane.org>

---

To: Shawn Guo <shawnguo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
To: Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> (maintainer:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)

Cc: Alessandro Zummo <a.zummo-BfzFCNDTiLLj+vYz1yj4TQ@public.gmane.org>
Cc: Alexandre Belloni <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
Cc: linux-rtc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org (open list:REAL TIME CLOCK (RTC) SUBSYSTEM)
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org (open list)
Cc: Fabio Estevam <fabio.estevam-3arQi8VN3Tc@public.gmane.org>
Cc: Juergen Borleis <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Noel Vellemans <Noel.Vellemans-8UENEgx6w+makBO8gow8eQ@public.gmane.org>
Cc: Russell King <linux-I+IVW8TIWO2tmTQ+vhA3Yw@public.gmane.org> (maintainer:ARM PORT)
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org (moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)

Cc: Philippe Ombredanne <pombredanne-od1rfyK75/E@public.gmane.org>
Cc: Lothar Waßmann <LW-bxm8fMRDkQLDiMYJYoSAnRvVK+yQ3ZXh@public.gmane.org>
---
 arch/arm/configs/imx_v4_v5_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/configs/imx_v4_v5_defconfig b/arch/arm/configs/imx_v4_v5_defconfig
index ca0f13cafe38..b48efd5ff8a7 100644
--- a/arch/arm/configs/imx_v4_v5_defconfig
+++ b/arch/arm/configs/imx_v4_v5_defconfig
@@ -167,6 +167,7 @@ CONFIG_RTC_DRV_PCF8563=y
 CONFIG_RTC_DRV_IMXDI=y
 CONFIG_RTC_DRV_MC13XXX=y
 CONFIG_RTC_DRV_MXC=y
+CONFIG_RTC_DRV_MXC_V2=y
 CONFIG_DMADEVICES=y
 CONFIG_IMX_DMA=y
 CONFIG_IMX_SDMA=y
-- 
2.11.0
--
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 v2 4/5] ARM: imx_v4_v5_defconfig: enable RTC_DRV_MXC_V2

[Fabio Estevam]

Hi Patrick,


On Tue, Dec 5, 2017 at 12:06 PM,  <linux-kernel-dev-QonKdJ6Bx35Wk0Htik3J/w@public.gmane.org> wrote:

>  arch/arm/configs/imx_v4_v5_defconfig | 1 +

i.mx53 uses imx_v6_v7_defconfig, not imx_v4_v5_defconfig.
--
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 v2 5/5] rtc: add mxc driver for i.MX53 SRTC

[linux-kernel-dev]

From: Patrick Bruenn <p.bruenn@beckhoff.com>

Neither rtc-imxdi, rtc-mxc nor rtc-snvs are compatible with i.MX53.

This is driver enables support for the low power domain SRTC features:
- 32-bit MSB of non-rollover time counter
- 32-bit alarm register

Based on:
http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/drivers/rtc/rtc-mxc_v2.c?h=imx_2.6.35_11.09.01

Signed-off-by: Patrick Bruenn <p.bruenn@beckhoff.com>

---

v2:
- have seperate patches for dt-binding, CONFIG option, imx53.dtsi and driver
- add SPDX-License-Identifier and cleanup copyright notice
- replace __raw_readl/writel() with readl/writel()
- fix PM_SLEEP callbacks
- add CONFIG_RTC_DRV_MXC_V2 to build rtc-mxc_v2.c
- remove misleading or obvious comments and fix style of the remaining
- avoid endless loop while waiting for hw
- implement consistent locking; make spinlock a member of dev struct
- enable clk only for register accesses
- remove all udelay() calls since they are obsolete or redundant
  (we are already waiting for register flags to change)
- init platform_data before registering irq callback
- let set_time() fail, when 32 bit rtc counter exceeded
- make names more consistent
- cleanup and reorder includes
- cleanup and remove unused defines

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>
---
 drivers/rtc/Makefile     |   1 +
 drivers/rtc/rtc-mxc_v2.c | 433 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 434 insertions(+)
 create mode 100644 drivers/rtc/rtc-mxc_v2.c

diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index f2f50c11dc38..dcf60e61ae5c 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -106,6 +106,7 @@ obj-$(CONFIG_RTC_DRV_MT6397)	+= rtc-mt6397.o
 obj-$(CONFIG_RTC_DRV_MT7622)	+= rtc-mt7622.o
 obj-$(CONFIG_RTC_DRV_MV)	+= rtc-mv.o
 obj-$(CONFIG_RTC_DRV_MXC)	+= rtc-mxc.o
+obj-$(CONFIG_RTC_DRV_MXC_V2)	+= rtc-mxc_v2.o
 obj-$(CONFIG_RTC_DRV_NUC900)	+= rtc-nuc900.o
 obj-$(CONFIG_RTC_DRV_OMAP)	+= rtc-omap.o
 obj-$(CONFIG_RTC_DRV_OPAL)	+= rtc-opal.o
diff --git a/drivers/rtc/rtc-mxc_v2.c b/drivers/rtc/rtc-mxc_v2.c
new file mode 100644
index 000000000000..c5a6d2c293bb
--- /dev/null
+++ b/drivers/rtc/rtc-mxc_v2.c
@@ -0,0 +1,433 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Real Time Clock (RTC) Driver for i.MX53
+ * Copyright (c) 2004-2011 Freescale Semiconductor, Inc.
+ * Copyright (c) 2017 Beckhoff Automation GmbH & Co. KG
+ */
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/rtc.h>
+
+#define SRTC_LPPDR_INIT       0x41736166	/* init for glitch detect */
+
+#define SRTC_LPCR_EN_LP       BIT(3)	/* lp enable */
+#define SRTC_LPCR_WAE         BIT(4)	/* lp wakeup alarm enable */
+#define SRTC_LPCR_ALP         BIT(7)	/* lp alarm flag */
+#define SRTC_LPCR_NSA         BIT(11)	/* lp non secure access */
+#define SRTC_LPCR_NVE         BIT(14)	/* lp non valid state exit bit */
+#define SRTC_LPCR_IE          BIT(15)	/* lp init state exit bit */
+
+#define SRTC_LPSR_ALP         BIT(3)	/* lp alarm flag */
+#define SRTC_LPSR_NVES        BIT(14)	/* lp non-valid state exit status */
+#define SRTC_LPSR_IES         BIT(15)	/* lp init state exit status */
+
+#define SRTC_LPSCMR	0x00	/* LP Secure Counter MSB Reg */
+#define SRTC_LPSCLR	0x04	/* LP Secure Counter LSB Reg */
+#define SRTC_LPSAR	0x08	/* LP Secure Alarm Reg */
+#define SRTC_LPCR	0x10	/* LP Control Reg */
+#define SRTC_LPSR	0x14	/* LP Status Reg */
+#define SRTC_LPPDR	0x18	/* LP Power Supply Glitch Detector Reg */
+
+/* max. number of retries to read registers, 120 was max during test */
+#define REG_READ_TIMEOUT 2000
+
+struct mxc_rtc_data {
+	struct rtc_device *rtc;
+	void __iomem *ioaddr;
+	struct clk *clk;
+	spinlock_t lock; /* protects register access */
+	int irq;
+};
+
+/*
+ * This function does write synchronization for writes to the lp srtc block.
+ * To take care of the asynchronous CKIL clock, all writes from the IP domain
+ * will be synchronized to the CKIL domain.
+ * The caller should hold the pdata->lock
+ */
+static inline void mxc_rtc_sync_lp_locked(void __iomem *ioaddr)
+{
+	unsigned int i;
+
+	/* Wait for 3 CKIL cycles */
+	for (i = 0; i < 3; i++) {
+		const u32 count = readl(ioaddr + SRTC_LPSCLR);
+		unsigned int timeout = REG_READ_TIMEOUT;
+
+		while ((readl(ioaddr + SRTC_LPSCLR)) == count) {
+			if (!--timeout) {
+				pr_err("SRTC_LPSCLR stuck! Check your hw.\n");
+				return;
+			}
+		}
+	}
+}
+
+/*
+ * This function updates the RTC alarm registers and then clears all the
+ * interrupt status bits.
+ * The caller should hold the pdata->lock
+ *
+ * @param  alrm         the new alarm value to be updated in the RTC
+ *
+ * @return  0 if successful; non-zero otherwise.
+ */
+static int mxc_rtc_write_alarm_locked(struct mxc_rtc_data *const pdata,
+				      struct rtc_time *alarm_tm)
+{
+	void __iomem *const ioaddr = pdata->ioaddr;
+	unsigned long time;
+
+	rtc_tm_to_time(alarm_tm, &time);
+
+	if (time > U32_MAX) {
+		pr_err("Hopefully I am out of service by then :-(\n");
+		return -EINVAL;
+	}
+
+	writel((u32)time, ioaddr + SRTC_LPSAR);
+
+	/* clear alarm interrupt status bit */
+	writel(SRTC_LPSR_ALP, ioaddr + SRTC_LPSR);
+
+	mxc_rtc_sync_lp_locked(ioaddr);
+	return 0;
+}
+
+/* This function is the RTC interrupt service routine. */
+static irqreturn_t mxc_rtc_interrupt(int irq, void *dev_id)
+{
+	struct platform_device *pdev = dev_id;
+	struct mxc_rtc_data *pdata = platform_get_drvdata(pdev);
+	void __iomem *ioaddr = pdata->ioaddr;
+	unsigned long flags;
+	u32 events = 0;
+	u32 lp_status;
+	u32 lp_cr;
+
+	spin_lock_irqsave(&pdata->lock, flags);
+	if (clk_prepare_enable(pdata->clk)) {
+		spin_unlock_irqrestore(&pdata->lock, flags);
+		return IRQ_NONE;
+	}
+
+	lp_status = readl(ioaddr + SRTC_LPSR);
+	lp_cr = readl(ioaddr + SRTC_LPCR);
+
+	/* update irq data & counter */
+	if (lp_status & SRTC_LPSR_ALP) {
+		if (lp_cr & SRTC_LPCR_ALP)
+			events = (RTC_AF | RTC_IRQF);
+
+		/* disable further lp alarm interrupts */
+		lp_cr &= ~(SRTC_LPCR_ALP | SRTC_LPCR_WAE);
+	}
+
+	/* Update interrupt enables */
+	writel(lp_cr, ioaddr + SRTC_LPCR);
+
+	/* clear interrupt status */
+	writel(lp_status, ioaddr + SRTC_LPSR);
+
+	mxc_rtc_sync_lp_locked(ioaddr);
+	rtc_update_irq(pdata->rtc, 1, events);
+	clk_disable_unprepare(pdata->clk);
+	spin_unlock_irqrestore(&pdata->lock, flags);
+	return IRQ_HANDLED;
+}
+
+/*
+ * Enable clk and aquire spinlock
+ * @return  0 if successful; non-zero otherwise.
+ */
+static int mxc_rtc_lock(struct mxc_rtc_data *const pdata)
+{
+	int ret;
+
+	spin_lock_irq(&pdata->lock);
+	ret = clk_prepare_enable(pdata->clk);
+	if (ret) {
+		spin_unlock_irq(&pdata->lock);
+		return ret;
+	}
+	return 0;
+}
+
+static int mxc_rtc_unlock(struct mxc_rtc_data *const pdata)
+{
+	clk_disable_unprepare(pdata->clk);
+	spin_unlock_irq(&pdata->lock);
+	return 0;
+}
+
+/*
+ * This function reads the current RTC time into tm in Gregorian date.
+ *
+ * @param  tm           contains the RTC time value upon return
+ *
+ * @return  0 if successful; non-zero otherwise.
+ */
+static int mxc_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
+	time_t now;
+	int ret = mxc_rtc_lock(pdata);
+
+	if (ret)
+		return ret;
+
+	now = readl(pdata->ioaddr + SRTC_LPSCMR);
+	rtc_time_to_tm(now, tm);
+	ret = rtc_valid_tm(tm);
+	mxc_rtc_unlock(pdata);
+	return ret;
+}
+
+/*
+ * This function sets the internal RTC time based on tm in Gregorian date.
+ *
+ * @param  tm           the time value to be set in the RTC
+ *
+ * @return  0 if successful; non-zero otherwise.
+ */
+static int mxc_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+	struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
+	time64_t time = rtc_tm_to_time64(tm);
+	int ret;
+
+	if (time > U32_MAX) {
+		dev_err(dev, "RTC exceeded by %llus\n", time - U32_MAX);
+		return -EINVAL;
+	}
+
+	ret = mxc_rtc_lock(pdata);
+	if (ret)
+		return ret;
+
+	writel(time, pdata->ioaddr + SRTC_LPSCMR);
+	mxc_rtc_sync_lp_locked(pdata->ioaddr);
+	return mxc_rtc_unlock(pdata);
+}
+
+/*
+ * This function reads the current alarm value into the passed in \b alrm
+ * argument. It updates the \b alrm's pending field value based on the whether
+ * an alarm interrupt occurs or not.
+ *
+ * @param  alrm         contains the RTC alarm value upon return
+ *
+ * @return  0 if successful; non-zero otherwise.
+ */
+static int mxc_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
+	void __iomem *ioaddr = pdata->ioaddr;
+	int ret;
+
+	ret = mxc_rtc_lock(pdata);
+	if (ret)
+		return ret;
+
+	rtc_time_to_tm(readl(ioaddr + SRTC_LPSAR), &alrm->time);
+	alrm->pending = !!(readl(ioaddr + SRTC_LPSR) & SRTC_LPSR_ALP);
+	return mxc_rtc_unlock(pdata);
+}
+
+/*
+ * Enable/Disable alarm interrupt
+ * The caller should hold the pdata->lock
+ */
+static void mxc_rtc_alarm_irq_enable_locked(struct mxc_rtc_data *pdata,
+					    unsigned int enable)
+{
+	u32 lp_cr = readl(pdata->ioaddr + SRTC_LPCR);
+
+	if (enable)
+		lp_cr |= (SRTC_LPCR_ALP | SRTC_LPCR_WAE);
+	else
+		lp_cr &= ~(SRTC_LPCR_ALP | SRTC_LPCR_WAE);
+
+	writel(lp_cr, pdata->ioaddr + SRTC_LPCR);
+}
+
+static int mxc_rtc_alarm_irq_enable(struct device *dev, unsigned int enable)
+{
+	struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
+	int ret = mxc_rtc_lock(pdata);
+
+	if (ret)
+		return ret;
+
+	mxc_rtc_alarm_irq_enable_locked(pdata, enable);
+	return mxc_rtc_unlock(pdata);
+}
+
+/*
+ * This function sets the RTC alarm based on passed in alrm.
+ *
+ * @param  alrm         the alarm value to be set in the RTC
+ *
+ * @return  0 if successful; non-zero otherwise.
+ */
+static int mxc_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
+	int ret = mxc_rtc_lock(pdata);
+
+	if (ret)
+		return ret;
+
+	ret = mxc_rtc_write_alarm_locked(pdata, &alrm->time);
+	if (!ret) {
+		mxc_rtc_alarm_irq_enable_locked(pdata, alrm->enabled);
+		mxc_rtc_sync_lp_locked(pdata->ioaddr);
+	}
+	mxc_rtc_unlock(pdata);
+	return ret;
+}
+
+static const struct rtc_class_ops mxc_rtc_ops = {
+	.read_time = mxc_rtc_read_time,
+	.set_time = mxc_rtc_set_time,
+	.read_alarm = mxc_rtc_read_alarm,
+	.set_alarm = mxc_rtc_set_alarm,
+	.alarm_irq_enable = mxc_rtc_alarm_irq_enable,
+};
+
+static int mxc_rtc_wait_for_flag(void *__iomem ioaddr, int flag)
+{
+	unsigned int timeout = REG_READ_TIMEOUT;
+
+	while (!(readl(ioaddr) & flag)) {
+		if (!--timeout) {
+			pr_err("Wait timeout for 0x%x@%p!\n", flag, ioaddr);
+			return -EBUSY;
+		}
+	}
+	return 0;
+}
+
+static int mxc_rtc_probe(struct platform_device *pdev)
+{
+	struct mxc_rtc_data *pdata;
+	struct resource *res;
+	void __iomem *ioaddr;
+	int ret = 0;
+
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENODEV;
+
+	pdata->ioaddr = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(pdata->ioaddr))
+		return PTR_ERR(pdata->ioaddr);
+
+	ioaddr = pdata->ioaddr;
+
+	pdata->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(pdata->clk)) {
+		dev_err(&pdev->dev, "unable to get rtc clock!\n");
+		return PTR_ERR(pdata->clk);
+	}
+
+	spin_lock_init(&pdata->lock);
+	pdata->irq = platform_get_irq(pdev, 0);
+	if (pdata->irq < 0)
+		return pdata->irq;
+
+	device_init_wakeup(&pdev->dev, 1);
+
+	ret = clk_prepare_enable(pdata->clk);
+	if (ret)
+		return ret;
+	/* initialize glitch detect */
+	writel(SRTC_LPPDR_INIT, ioaddr + SRTC_LPPDR);
+
+	/* clear lp interrupt status */
+	writel(0xFFFFFFFF, ioaddr + SRTC_LPSR);
+
+	/* move out of init state */
+	writel((SRTC_LPCR_IE | SRTC_LPCR_NSA), ioaddr + SRTC_LPCR);
+	mxc_rtc_wait_for_flag(ioaddr + SRTC_LPSR, SRTC_LPSR_IES);
+
+	/* move out of non-valid state */
+	writel((SRTC_LPCR_IE | SRTC_LPCR_NVE | SRTC_LPCR_NSA |
+		SRTC_LPCR_EN_LP), ioaddr + SRTC_LPCR);
+	mxc_rtc_wait_for_flag(ioaddr + SRTC_LPSR, SRTC_LPSR_NVES);
+
+	clk_disable_unprepare(pdata->clk);
+	platform_set_drvdata(pdev, pdata);
+	ret =
+	    devm_request_irq(&pdev->dev, pdata->irq, mxc_rtc_interrupt, 0,
+			     pdev->name, pdev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "interrupt not available.\n");
+		return ret;
+	}
+
+	pdata->rtc =
+	    devm_rtc_device_register(&pdev->dev, pdev->name, &mxc_rtc_ops,
+				     THIS_MODULE);
+	if (IS_ERR(pdata->rtc))
+		return PTR_ERR(pdata->rtc);
+
+	return 0;
+}
+
+static int __exit mxc_rtc_remove(struct platform_device *pdev)
+{
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int mxc_rtc_suspend(struct device *dev)
+{
+	struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
+
+	if (device_may_wakeup(dev))
+		enable_irq_wake(pdata->irq);
+
+	return 0;
+}
+
+static int mxc_rtc_resume(struct device *dev)
+{
+	struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
+
+	if (device_may_wakeup(dev))
+		disable_irq_wake(pdata->irq);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(mxc_rtc_pm_ops, mxc_rtc_suspend, mxc_rtc_resume);
+
+static const struct of_device_id mxc_ids[] = {
+	{ .compatible = "fsl,imx53-rtc", },
+	{}
+};
+
+static struct platform_driver mxc_rtc_driver = {
+	.driver = {
+		.name = "mxc_rtc_v2",
+		.of_match_table = mxc_ids,
+		.pm = &mxc_rtc_pm_ops,
+	},
+	.probe = mxc_rtc_probe,
+	.remove = mxc_rtc_remove,
+};
+
+module_platform_driver(mxc_rtc_driver);
+
+MODULE_AUTHOR("Freescale Semiconductor, Inc.");
+MODULE_DESCRIPTION("Real Time Clock (RTC) Driver for i.MX53");
+MODULE_LICENSE("GPL");
-- 
2.11.0

Re: [PATCH v2 5/5] rtc: add mxc driver for i.MX53 SRTC

[Sascha Hauer]

On Tue, Dec 05, 2017 at 03:06:46PM +0100, linux-kernel-dev-QonKdJ6Bx35Wk0Htik3J/w@public.gmane.org wrote:
> From: Patrick Bruenn <p.bruenn-QonKdJ6Bx35Wk0Htik3J/w@public.gmane.org>
> 
> Neither rtc-imxdi, rtc-mxc nor rtc-snvs are compatible with i.MX53.
> 
> This is driver enables support for the low power domain SRTC features:
> - 32-bit MSB of non-rollover time counter
> - 32-bit alarm register
> 
> Based on:
> http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/drivers/rtc/rtc-mxc_v2.c?h=imx_2.6.35_11.09.01
> 
> Signed-off-by: Patrick Bruenn <p.bruenn-QonKdJ6Bx35Wk0Htik3J/w@public.gmane.org>
> 
> ---
> 
> v2:
> - have seperate patches for dt-binding, CONFIG option, imx53.dtsi and driver
> - add SPDX-License-Identifier and cleanup copyright notice
> - replace __raw_readl/writel() with readl/writel()
> - fix PM_SLEEP callbacks
> - add CONFIG_RTC_DRV_MXC_V2 to build rtc-mxc_v2.c
> - remove misleading or obvious comments and fix style of the remaining
> - avoid endless loop while waiting for hw
> - implement consistent locking; make spinlock a member of dev struct
> - enable clk only for register accesses
> - remove all udelay() calls since they are obsolete or redundant
>   (we are already waiting for register flags to change)
> - init platform_data before registering irq callback
> - let set_time() fail, when 32 bit rtc counter exceeded
> - make names more consistent
> - cleanup and reorder includes
> - cleanup and remove unused defines
> 
> To: Alessandro Zummo <a.zummo-BfzFCNDTiLLj+vYz1yj4TQ@public.gmane.org>
> To: Alexandre Belloni <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
> Cc: linux-rtc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org (open list:REAL TIME CLOCK (RTC) SUBSYSTEM)
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
> Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org (open list)
> Cc: Fabio Estevam <fabio.estevam-3arQi8VN3Tc@public.gmane.org>
> Cc: Juergen Borleis <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Cc: Noel Vellemans <Noel.Vellemans-8UENEgx6w+makBO8gow8eQ@public.gmane.org>
> Cc: Shawn Guo <shawnguo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> (maintainer:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)
> Cc: Russell King <linux-I+IVW8TIWO2tmTQ+vhA3Yw@public.gmane.org> (maintainer:ARM PORT)
> Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org (moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)
> 
> Cc: Philippe Ombredanne <pombredanne-od1rfyK75/E@public.gmane.org>
> Cc: Lothar Waßmann <LW-bxm8fMRDkQLDiMYJYoSAnRvVK+yQ3ZXh@public.gmane.org>
> ---
>  drivers/rtc/Makefile     |   1 +
>  drivers/rtc/rtc-mxc_v2.c | 433 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 434 insertions(+)
>  create mode 100644 drivers/rtc/rtc-mxc_v2.c
> 
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index f2f50c11dc38..dcf60e61ae5c 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -106,6 +106,7 @@ obj-$(CONFIG_RTC_DRV_MT6397)	+= rtc-mt6397.o
>  obj-$(CONFIG_RTC_DRV_MT7622)	+= rtc-mt7622.o
>  obj-$(CONFIG_RTC_DRV_MV)	+= rtc-mv.o
>  obj-$(CONFIG_RTC_DRV_MXC)	+= rtc-mxc.o
> +obj-$(CONFIG_RTC_DRV_MXC_V2)	+= rtc-mxc_v2.o
>  obj-$(CONFIG_RTC_DRV_NUC900)	+= rtc-nuc900.o
>  obj-$(CONFIG_RTC_DRV_OMAP)	+= rtc-omap.o
>  obj-$(CONFIG_RTC_DRV_OPAL)	+= rtc-opal.o
> diff --git a/drivers/rtc/rtc-mxc_v2.c b/drivers/rtc/rtc-mxc_v2.c
> new file mode 100644
> index 000000000000..c5a6d2c293bb
> --- /dev/null
> +++ b/drivers/rtc/rtc-mxc_v2.c
> @@ -0,0 +1,433 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Real Time Clock (RTC) Driver for i.MX53
> + * Copyright (c) 2004-2011 Freescale Semiconductor, Inc.
> + * Copyright (c) 2017 Beckhoff Automation GmbH & Co. KG
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/rtc.h>
> +
> +#define SRTC_LPPDR_INIT       0x41736166	/* init for glitch detect */
> +
> +#define SRTC_LPCR_EN_LP       BIT(3)	/* lp enable */
> +#define SRTC_LPCR_WAE         BIT(4)	/* lp wakeup alarm enable */
> +#define SRTC_LPCR_ALP         BIT(7)	/* lp alarm flag */
> +#define SRTC_LPCR_NSA         BIT(11)	/* lp non secure access */
> +#define SRTC_LPCR_NVE         BIT(14)	/* lp non valid state exit bit */
> +#define SRTC_LPCR_IE          BIT(15)	/* lp init state exit bit */
> +
> +#define SRTC_LPSR_ALP         BIT(3)	/* lp alarm flag */
> +#define SRTC_LPSR_NVES        BIT(14)	/* lp non-valid state exit status */
> +#define SRTC_LPSR_IES         BIT(15)	/* lp init state exit status */
> +
> +#define SRTC_LPSCMR	0x00	/* LP Secure Counter MSB Reg */
> +#define SRTC_LPSCLR	0x04	/* LP Secure Counter LSB Reg */
> +#define SRTC_LPSAR	0x08	/* LP Secure Alarm Reg */
> +#define SRTC_LPCR	0x10	/* LP Control Reg */
> +#define SRTC_LPSR	0x14	/* LP Status Reg */
> +#define SRTC_LPPDR	0x18	/* LP Power Supply Glitch Detector Reg */
> +
> +/* max. number of retries to read registers, 120 was max during test */
> +#define REG_READ_TIMEOUT 2000
> +
> +struct mxc_rtc_data {
> +	struct rtc_device *rtc;
> +	void __iomem *ioaddr;
> +	struct clk *clk;
> +	spinlock_t lock; /* protects register access */
> +	int irq;
> +};
> +
> +/*
> + * This function does write synchronization for writes to the lp srtc block.
> + * To take care of the asynchronous CKIL clock, all writes from the IP domain
> + * will be synchronized to the CKIL domain.
> + * The caller should hold the pdata->lock
> + */
> +static inline void mxc_rtc_sync_lp_locked(void __iomem *ioaddr)
> +{
> +	unsigned int i;
> +
> +	/* Wait for 3 CKIL cycles */
> +	for (i = 0; i < 3; i++) {
> +		const u32 count = readl(ioaddr + SRTC_LPSCLR);
> +		unsigned int timeout = REG_READ_TIMEOUT;
> +
> +		while ((readl(ioaddr + SRTC_LPSCLR)) == count) {
> +			if (!--timeout) {
> +				pr_err("SRTC_LPSCLR stuck! Check your hw.\n");
> +				return;
> +			}
> +		}
> +	}
> +}
> +
> +/*
> + * This function updates the RTC alarm registers and then clears all the
> + * interrupt status bits.
> + * The caller should hold the pdata->lock
> + *
> + * @param  alrm         the new alarm value to be updated in the RTC
> + *
> + * @return  0 if successful; non-zero otherwise.
> + */
> +static int mxc_rtc_write_alarm_locked(struct mxc_rtc_data *const pdata,
> +				      struct rtc_time *alarm_tm)
> +{
> +	void __iomem *const ioaddr = pdata->ioaddr;
> +	unsigned long time;
> +
> +	rtc_tm_to_time(alarm_tm, &time);
> +
> +	if (time > U32_MAX) {
> +		pr_err("Hopefully I am out of service by then :-(\n");
> +		return -EINVAL;
> +	}

This will never happen as on your target hardware unsigned long is a
32bit type. Not sure what is best to do here. Maybe you should test
the return value of rtc_tm_to_time. ATM it returns 0 unconditionally,
but rtc_tm_to_time could detect when the input time doesn't fit into
its return type and return an error in this case.
Also I just realized that it's unsigned and only overflows in the year
2106. I'm most likely dead then so I don't care that much ;)

> +
> +	writel((u32)time, ioaddr + SRTC_LPSAR);
> +
> +	/* clear alarm interrupt status bit */
> +	writel(SRTC_LPSR_ALP, ioaddr + SRTC_LPSR);
> +
> +	mxc_rtc_sync_lp_locked(ioaddr);
> +	return 0;
> +}
> +
> +/* This function is the RTC interrupt service routine. */
> +static irqreturn_t mxc_rtc_interrupt(int irq, void *dev_id)
> +{
> +	struct platform_device *pdev = dev_id;
> +	struct mxc_rtc_data *pdata = platform_get_drvdata(pdev);
> +	void __iomem *ioaddr = pdata->ioaddr;
> +	unsigned long flags;
> +	u32 events = 0;
> +	u32 lp_status;
> +	u32 lp_cr;
> +
> +	spin_lock_irqsave(&pdata->lock, flags);
> +	if (clk_prepare_enable(pdata->clk)) {
> +		spin_unlock_irqrestore(&pdata->lock, flags);
> +		return IRQ_NONE;
> +	}

You are not allowed to do a clk_prepare under a spinlock. That was the
original reason to split enabling a clk into clk_prepare and clk_enable.
Everything that can block is done in clk_prepare and only non blocking
things are done in clk_enable.
If you want to enable/disable the clock on demand you can clk_prepare()
in probe and clk_enable when you actually need it.

> +
> +	lp_status = readl(ioaddr + SRTC_LPSR);
> +	lp_cr = readl(ioaddr + SRTC_LPCR);
> +
> +	/* update irq data & counter */
> +	if (lp_status & SRTC_LPSR_ALP) {
> +		if (lp_cr & SRTC_LPCR_ALP)
> +			events = (RTC_AF | RTC_IRQF);
> +
> +		/* disable further lp alarm interrupts */
> +		lp_cr &= ~(SRTC_LPCR_ALP | SRTC_LPCR_WAE);
> +	}
> +
> +	/* Update interrupt enables */
> +	writel(lp_cr, ioaddr + SRTC_LPCR);
> +
> +	/* clear interrupt status */
> +	writel(lp_status, ioaddr + SRTC_LPSR);
> +
> +	mxc_rtc_sync_lp_locked(ioaddr);
> +	rtc_update_irq(pdata->rtc, 1, events);
> +	clk_disable_unprepare(pdata->clk);
> +	spin_unlock_irqrestore(&pdata->lock, flags);
> +	return IRQ_HANDLED;
> +}
> +
> +/*
> + * Enable clk and aquire spinlock
> + * @return  0 if successful; non-zero otherwise.
> + */
> +static int mxc_rtc_lock(struct mxc_rtc_data *const pdata)
> +{
> +	int ret;
> +
> +	spin_lock_irq(&pdata->lock);
> +	ret = clk_prepare_enable(pdata->clk);
> +	if (ret) {
> +		spin_unlock_irq(&pdata->lock);
> +		return ret;
> +	}
> +	return 0;
> +}
> +
> +static int mxc_rtc_unlock(struct mxc_rtc_data *const pdata)
> +{
> +	clk_disable_unprepare(pdata->clk);
> +	spin_unlock_irq(&pdata->lock);
> +	return 0;
> +}
> +
> +/*
> + * This function reads the current RTC time into tm in Gregorian date.
> + *
> + * @param  tm           contains the RTC time value upon return
> + *
> + * @return  0 if successful; non-zero otherwise.
> + */
> +static int mxc_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
> +	time_t now;
> +	int ret = mxc_rtc_lock(pdata);
> +
> +	if (ret)
> +		return ret;
> +
> +	now = readl(pdata->ioaddr + SRTC_LPSCMR);
> +	rtc_time_to_tm(now, tm);
> +	ret = rtc_valid_tm(tm);
> +	mxc_rtc_unlock(pdata);

I don't think this needs to be locked.

> +	return ret;
> +}
> +
> +/*
> + * This function sets the internal RTC time based on tm in Gregorian date.
> + *
> + * @param  tm           the time value to be set in the RTC
> + *
> + * @return  0 if successful; non-zero otherwise.
> + */
> +static int mxc_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
> +	time64_t time = rtc_tm_to_time64(tm);
> +	int ret;
> +
> +	if (time > U32_MAX) {
> +		dev_err(dev, "RTC exceeded by %llus\n", time - U32_MAX);
> +		return -EINVAL;
> +	}
> +
> +	ret = mxc_rtc_lock(pdata);
> +	if (ret)
> +		return ret;
> +
> +	writel(time, pdata->ioaddr + SRTC_LPSCMR);
> +	mxc_rtc_sync_lp_locked(pdata->ioaddr);
> +	return mxc_rtc_unlock(pdata);
> +}
> +
> +/*
> + * This function reads the current alarm value into the passed in \b alrm
> + * argument. It updates the \b alrm's pending field value based on the whether
> + * an alarm interrupt occurs or not.
> + *
> + * @param  alrm         contains the RTC alarm value upon return
> + *
> + * @return  0 if successful; non-zero otherwise.
> + */
> +static int mxc_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
> +	void __iomem *ioaddr = pdata->ioaddr;
> +	int ret;
> +
> +	ret = mxc_rtc_lock(pdata);
> +	if (ret)
> +		return ret;
> +
> +	rtc_time_to_tm(readl(ioaddr + SRTC_LPSAR), &alrm->time);
> +	alrm->pending = !!(readl(ioaddr + SRTC_LPSR) & SRTC_LPSR_ALP);
> +	return mxc_rtc_unlock(pdata);
> +}
> +
> +/*
> + * Enable/Disable alarm interrupt
> + * The caller should hold the pdata->lock
> + */
> +static void mxc_rtc_alarm_irq_enable_locked(struct mxc_rtc_data *pdata,
> +					    unsigned int enable)
> +{
> +	u32 lp_cr = readl(pdata->ioaddr + SRTC_LPCR);
> +
> +	if (enable)
> +		lp_cr |= (SRTC_LPCR_ALP | SRTC_LPCR_WAE);
> +	else
> +		lp_cr &= ~(SRTC_LPCR_ALP | SRTC_LPCR_WAE);
> +
> +	writel(lp_cr, pdata->ioaddr + SRTC_LPCR);
> +}
> +
> +static int mxc_rtc_alarm_irq_enable(struct device *dev, unsigned int enable)
> +{
> +	struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
> +	int ret = mxc_rtc_lock(pdata);
> +
> +	if (ret)
> +		return ret;
> +
> +	mxc_rtc_alarm_irq_enable_locked(pdata, enable);
> +	return mxc_rtc_unlock(pdata);
> +}
> +
> +/*
> + * This function sets the RTC alarm based on passed in alrm.
> + *
> + * @param  alrm         the alarm value to be set in the RTC
> + *
> + * @return  0 if successful; non-zero otherwise.
> + */
> +static int mxc_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
> +	int ret = mxc_rtc_lock(pdata);
> +
> +	if (ret)
> +		return ret;
> +
> +	ret = mxc_rtc_write_alarm_locked(pdata, &alrm->time);

Is it worth it to make this a separate function?

> +	if (!ret) {
> +		mxc_rtc_alarm_irq_enable_locked(pdata, alrm->enabled);
> +		mxc_rtc_sync_lp_locked(pdata->ioaddr);
> +	}
> +	mxc_rtc_unlock(pdata);
> +	return ret;
> +}
> +
> +static const struct rtc_class_ops mxc_rtc_ops = {
> +	.read_time = mxc_rtc_read_time,
> +	.set_time = mxc_rtc_set_time,
> +	.read_alarm = mxc_rtc_read_alarm,
> +	.set_alarm = mxc_rtc_set_alarm,
> +	.alarm_irq_enable = mxc_rtc_alarm_irq_enable,
> +};
> +
> +static int mxc_rtc_wait_for_flag(void *__iomem ioaddr, int flag)
> +{
> +	unsigned int timeout = REG_READ_TIMEOUT;
> +
> +	while (!(readl(ioaddr) & flag)) {
> +		if (!--timeout) {
> +			pr_err("Wait timeout for 0x%x@%p!\n", flag, ioaddr);

Please use dev_* functions for printing. In this case the message should
probably be printed from the caller.

> +			return -EBUSY;
> +		}
> +	}
> +	return 0;
> +}
> +
> +static int mxc_rtc_probe(struct platform_device *pdev)
> +{
> +	struct mxc_rtc_data *pdata;
> +	struct resource *res;
> +	void __iomem *ioaddr;
> +	int ret = 0;
> +
> +	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENODEV;
> +
> +	pdata->ioaddr = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(pdata->ioaddr))
> +		return PTR_ERR(pdata->ioaddr);
> +
> +	ioaddr = pdata->ioaddr;
> +
> +	pdata->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(pdata->clk)) {
> +		dev_err(&pdev->dev, "unable to get rtc clock!\n");
> +		return PTR_ERR(pdata->clk);
> +	}
> +
> +	spin_lock_init(&pdata->lock);
> +	pdata->irq = platform_get_irq(pdev, 0);
> +	if (pdata->irq < 0)
> +		return pdata->irq;
> +
> +	device_init_wakeup(&pdev->dev, 1);
> +
> +	ret = clk_prepare_enable(pdata->clk);
> +	if (ret)
> +		return ret;
> +	/* initialize glitch detect */
> +	writel(SRTC_LPPDR_INIT, ioaddr + SRTC_LPPDR);
> +
> +	/* clear lp interrupt status */
> +	writel(0xFFFFFFFF, ioaddr + SRTC_LPSR);
> +
> +	/* move out of init state */
> +	writel((SRTC_LPCR_IE | SRTC_LPCR_NSA), ioaddr + SRTC_LPCR);
> +	xc_rtc_wait_for_flag(ioaddr + SRTC_LPSR, SRTC_LPSR_IES);

If this can fail, shouldn't you test for an error?

> +
> +	/* move out of non-valid state */
> +	writel((SRTC_LPCR_IE | SRTC_LPCR_NVE | SRTC_LPCR_NSA |
> +		SRTC_LPCR_EN_LP), ioaddr + SRTC_LPCR);
> +	mxc_rtc_wait_for_flag(ioaddr + SRTC_LPSR, SRTC_LPSR_NVES);

dito

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
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 v2 5/5] rtc: add mxc driver for i.MX53 SRTC

[Alexandre Belloni]

On 06/12/2017 at 09:36:18 +0100, Sascha Hauer wrote:
> > +/*
> > + * This function updates the RTC alarm registers and then clears all the
> > + * interrupt status bits.
> > + * The caller should hold the pdata->lock
> > + *
> > + * @param  alrm         the new alarm value to be updated in the RTC
> > + *
> > + * @return  0 if successful; non-zero otherwise.
> > + */
> > +static int mxc_rtc_write_alarm_locked(struct mxc_rtc_data *const pdata,
> > +				      struct rtc_time *alarm_tm)
> > +{
> > +	void __iomem *const ioaddr = pdata->ioaddr;
> > +	unsigned long time;
> > +
> > +	rtc_tm_to_time(alarm_tm, &time);
> > +
> > +	if (time > U32_MAX) {
> > +		pr_err("Hopefully I am out of service by then :-(\n");
> > +		return -EINVAL;
> > +	}
> 
> This will never happen as on your target hardware unsigned long is a
> 32bit type. Not sure what is best to do here. Maybe you should test
> the return value of rtc_tm_to_time. ATM it returns 0 unconditionally,
> but rtc_tm_to_time could detect when the input time doesn't fit into
> its return type and return an error in this case.
> Also I just realized that it's unsigned and only overflows in the year
> 2106. I'm most likely dead then so I don't care that much ;)
> 

One solution is to use the 64bit version instead so it doesn't overflow.
This makes the time > U32_MAX work.
Also, I'll send (hopefully soon) a series adding proper range checking
for the whole RTC subsystem. And yes, it not urgent as I don't think I
will care so much in 2106 too ;)

> > +/*
> > + * This function reads the current RTC time into tm in Gregorian date.
> > + *
> > + * @param  tm           contains the RTC time value upon return
> > + *
> > + * @return  0 if successful; non-zero otherwise.
> > + */
> > +static int mxc_rtc_read_time(struct device *dev, struct rtc_time *tm)
> > +{
> > +	struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
> > +	time_t now;
> > +	int ret = mxc_rtc_lock(pdata);
> > +
> > +	if (ret)
> > +		return ret;
> > +
> > +	now = readl(pdata->ioaddr + SRTC_LPSCMR);
> > +	rtc_time_to_tm(now, tm);
> > +	ret = rtc_valid_tm(tm);

This check is useless for two reasons: you know that rtc_time_to_tm will
generate a valid tm and the core always checks the tm anyway.


-- 
Alexandre Belloni, 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 v2 5/5] rtc: add mxc driver for i.MX53 SRTC

[Patrick Brünn]

>From: Alexandre Belloni [mailto:alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org]
>Sent: Mittwoch, 6. Dezember 2017 09:58
>On 06/12/2017 at 09:36:18 +0100, Sascha Hauer wrote:
>> > +/*
>> > + * This function updates the RTC alarm registers and then clears all the
>> > + * interrupt status bits.
>> > + * The caller should hold the pdata->lock
>> > + *
>> > + * @param  alrm         the new alarm value to be updated in the RTC
>> > + *
>> > + * @return  0 if successful; non-zero otherwise.
>> > + */
>> > +static int mxc_rtc_write_alarm_locked(struct mxc_rtc_data *const pdata,
>> > +                                struct rtc_time *alarm_tm)
>> > +{
>> > +  void __iomem *const ioaddr = pdata->ioaddr;
>> > +  unsigned long time;
>> > +
>> > +  rtc_tm_to_time(alarm_tm, &time);
>> > +
>> > +  if (time > U32_MAX) {
>> > +          pr_err("Hopefully I am out of service by then :-(\n");
>> > +          return -EINVAL;
>> > +  }
>>
>> This will never happen as on your target hardware unsigned long is a
>> 32bit type. Not sure what is best to do here. Maybe you should test
>> the return value of rtc_tm_to_time. ATM it returns 0 unconditionally,
>> but rtc_tm_to_time could detect when the input time doesn't fit into
>> its return type and return an error in this case.
>> Also I just realized that it's unsigned and only overflows in the year
>> 2106. I'm most likely dead then so I don't care that much ;)
>>
>
>One solution is to use the 64bit version instead so it doesn't overflow.
>This makes the time > U32_MAX work.
>Also, I'll send (hopefully soon) a series adding proper range checking
>for the whole RTC subsystem. And yes, it not urgent as I don't think I
>will care so much in 2106 too ;)
>
I just noticed that in mxc_rtc_set_time() I am using the 64bit version.
After thinking a while about this issue, I think the 64bit version is better
suited for my use case. It makes explicitly clear that I need to push the
time into a 32bit hw register and "unsigned long" is just by accident the
correct size for me.

>> > +/*
>> > + * This function reads the current RTC time into tm in Gregorian date.
>> > + *
>> > + * @param  tm           contains the RTC time value upon return
>> > + *
>> > + * @return  0 if successful; non-zero otherwise.
>> > + */
>> > +static int mxc_rtc_read_time(struct device *dev, struct rtc_time *tm)
>> > +{
>> > +  struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
>> > +  time_t now;
>> > +  int ret = mxc_rtc_lock(pdata);
>> > +
>> > +  if (ret)
>> > +          return ret;
>> > +
>> > +  now = readl(pdata->ioaddr + SRTC_LPSCMR);
>> > +  rtc_time_to_tm(now, tm);
>> > +  ret = rtc_valid_tm(tm);
>
>This check is useless for two reasons: you know that rtc_time_to_tm will
>generate a valid tm and the core always checks the tm anyway.
I will remove this with the next version

Thanks for your time and help,
Patrick

Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff
Registered office: Verl, Germany | Register court: Guetersloh HRA 7075



--
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 v2 5/5] rtc: add mxc driver for i.MX53 SRTC

[Patrick Brünn]

>From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
>Sent: Mittwoch, 6. Dezember 2017 09:36
>On Tue, Dec 05, 2017 at 03:06:46PM +0100, linux-kernel-dev@beckhoff.com
>wrote:
>> +static int mxc_rtc_write_alarm_locked(struct mxc_rtc_data *const pdata,
>> +                                  struct rtc_time *alarm_tm)
>> +{
>> +    void __iomem *const ioaddr = pdata->ioaddr;
>> +    unsigned long time;
>> +
>> +    rtc_tm_to_time(alarm_tm, &time);
>> +
>> +    if (time > U32_MAX) {
>> +            pr_err("Hopefully I am out of service by then :-(\n");
>> +            return -EINVAL;
>> +    }
>
>This will never happen as on your target hardware unsigned long is a
>32bit type. Not sure what is best to do here. Maybe you should test
>the return value of rtc_tm_to_time. ATM it returns 0 unconditionally,
>but rtc_tm_to_time could detect when the input time doesn't fit into
>its return type and return an error in this case.
>Also I just realized that it's unsigned and only overflows in the year
>2106. I'm most likely dead then so I don't care that much ;)
>
please see my response to Alexandre's follow up

>> +/* This function is the RTC interrupt service routine. */
>> +static irqreturn_t mxc_rtc_interrupt(int irq, void *dev_id)
>> +{
>> +    struct platform_device *pdev = dev_id;
>> +    struct mxc_rtc_data *pdata = platform_get_drvdata(pdev);
>> +    void __iomem *ioaddr = pdata->ioaddr;
>> +    unsigned long flags;
>> +    u32 events = 0;
>> +    u32 lp_status;
>> +    u32 lp_cr;
>> +
>> +    spin_lock_irqsave(&pdata->lock, flags);
>> +    if (clk_prepare_enable(pdata->clk)) {
>> +            spin_unlock_irqrestore(&pdata->lock, flags);
>> +            return IRQ_NONE;
>> +    }
>
>You are not allowed to do a clk_prepare under a spinlock. That was the
>original reason to split enabling a clk into clk_prepare and clk_enable.
>Everything that can block is done in clk_prepare and only non blocking
>things are done in clk_enable.
>If you want to enable/disable the clock on demand you can clk_prepare()
>in probe and clk_enable when you actually need it.
>
Thanks for clarification. To be honest when I read Lothar's suggestion it was
the first time I thought about the idea of keeping the clk disabled most of
the time. I am not very experienced with this. But a rtctest loop run for
hours so I assume it would be okay to keep the clk disabled until hw access.
If there is no objection from somebody who knows the i.MX53 SRTC HW
better, I will stick to the clock on demand model and make sure I avoid
blocking.
>> +
>> +static int mxc_rtc_read_time(struct device *dev, struct rtc_time *tm)
>> +{
>> +    struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
>> +    time_t now;
>> +    int ret = mxc_rtc_lock(pdata);
>> +
>> +    if (ret)
>> +            return ret;
>> +
>> +    now = readl(pdata->ioaddr + SRTC_LPSCMR);
>> +    rtc_time_to_tm(now, tm);
>> +    ret = rtc_valid_tm(tm);
>> +    mxc_rtc_unlock(pdata);
>
>I don't think this needs to be locked.
I will change this to only enable the clock for the readl()

>> +static int mxc_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>> +{
>> +    struct mxc_rtc_data *pdata = dev_get_drvdata(dev);
>> +    int ret = mxc_rtc_lock(pdata);
>> +
>> +    if (ret)
>> +            return ret;
>> +
>> +    ret = mxc_rtc_write_alarm_locked(pdata, &alrm->time);
>
>Is it worth it to make this a separate function?
Maybe not, I think it is an artifact from a refactoring. I will reconsider this
for the next version.
>
>> +    if (!ret) {
>> +            mxc_rtc_alarm_irq_enable_locked(pdata, alrm->enabled);
>> +            mxc_rtc_sync_lp_locked(pdata->ioaddr);
>> +    }
>> +    mxc_rtc_unlock(pdata);
>> +    return ret;
>> +}
>> +
>> +static const struct rtc_class_ops mxc_rtc_ops = {
>> +    .read_time = mxc_rtc_read_time,
>> +    .set_time = mxc_rtc_set_time,
>> +    .read_alarm = mxc_rtc_read_alarm,
>> +    .set_alarm = mxc_rtc_set_alarm,
>> +    .alarm_irq_enable = mxc_rtc_alarm_irq_enable,
>> +};
>> +
>> +static int mxc_rtc_wait_for_flag(void *__iomem ioaddr, int flag)
>> +{
>> +    unsigned int timeout = REG_READ_TIMEOUT;
>> +
>> +    while (!(readl(ioaddr) & flag)) {
>> +            if (!--timeout) {
>> +                    pr_err("Wait timeout for 0x%x@%p!\n", flag, ioaddr);
>
>Please use dev_* functions for printing. In this case the message should
>probably be printed from the caller.
Do you have a link at hand about dev_* vs. pr_*? I just choose pr_err here,
because I would have to change the functions signature to get a device.
However, I will drop the message and move it to the caller.

>> +    /* clear lp interrupt status */
>> +    writel(0xFFFFFFFF, ioaddr + SRTC_LPSR);
>> +
>> +    /* move out of init state */
>> +    writel((SRTC_LPCR_IE | SRTC_LPCR_NSA), ioaddr + SRTC_LPCR);
>> +    xc_rtc_wait_for_flag(ioaddr + SRTC_LPSR, SRTC_LPSR_IES);
>
>If this can fail, shouldn't you test for an error?
very probably
>> +
>> +    /* move out of non-valid state */
>> +    writel((SRTC_LPCR_IE | SRTC_LPCR_NVE | SRTC_LPCR_NSA |
>> +            SRTC_LPCR_EN_LP), ioaddr + SRTC_LPCR);
>> +    mxc_rtc_wait_for_flag(ioaddr + SRTC_LPSR, SRTC_LPSR_NVES);
>
>dito
sure

Thanks,
Patrick
---
Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff
Registered office: Verl, Germany | Register court: Guetersloh HRA 7075

Re: [PATCH v2 5/5] rtc: add mxc driver for i.MX53 SRTC

[Sascha Hauer]

On Wed, Dec 06, 2017 at 10:17:06AM +0000, Patrick Brünn wrote:
> >From: Sascha Hauer [mailto:s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org]
> >Sent: Mittwoch, 6. Dezember 2017 09:36
> >On Tue, Dec 05, 2017 at 03:06:46PM +0100, linux-kernel-dev-QonKdJ6Bx35Wk0Htik3J/w@public.gmane.org
> >wrote:
> >> +static int mxc_rtc_wait_for_flag(void *__iomem ioaddr, int flag)
> >> +{
> >> +    unsigned int timeout = REG_READ_TIMEOUT;
> >> +
> >> +    while (!(readl(ioaddr) & flag)) {
> >> +            if (!--timeout) {
> >> +                    pr_err("Wait timeout for 0x%x@%p!\n", flag, ioaddr);
> >
> >Please use dev_* functions for printing. In this case the message should
> >probably be printed from the caller.
> Do you have a link at hand about dev_* vs. pr_*? I just choose pr_err here,
> because I would have to change the functions signature to get a device.
> However, I will drop the message and move it to the caller.

No, I don't have a link. However, a message printed with pr_err comes
with absolutely no context, so without grepping the source you do not
get a clue which device has a problem. You could add more context
using some prefix either to each message or by using

#define pr_fmt(fmt) "mydriver: " fmt

but then you still not know which instance the message throws. Using
dev_* just does the right thing without much thinking.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
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 v2 5/5] rtc: add mxc driver for i.MX53 SRTC

[Alexandre Belloni]

On 05/12/2017 at 15:06:46 +0100, linux-kernel-dev-QonKdJ6Bx35Wk0Htik3J/w@public.gmane.org wrote:
> +/* This function is the RTC interrupt service routine. */
> +static irqreturn_t mxc_rtc_interrupt(int irq, void *dev_id)
> +{
> +	struct platform_device *pdev = dev_id;
> +	struct mxc_rtc_data *pdata = platform_get_drvdata(pdev);
> +	void __iomem *ioaddr = pdata->ioaddr;
> +	unsigned long flags;
> +	u32 events = 0;
> +	u32 lp_status;
> +	u32 lp_cr;
> +
> +	spin_lock_irqsave(&pdata->lock, flags);
> +	if (clk_prepare_enable(pdata->clk)) {
> +		spin_unlock_irqrestore(&pdata->lock, flags);
> +		return IRQ_NONE;
> +	}
> +
> +	lp_status = readl(ioaddr + SRTC_LPSR);
> +	lp_cr = readl(ioaddr + SRTC_LPCR);
> +
> +	/* update irq data & counter */
> +	if (lp_status & SRTC_LPSR_ALP) {
> +		if (lp_cr & SRTC_LPCR_ALP)
> +			events = (RTC_AF | RTC_IRQF);

I would just call rtc_update_irq here...

> +
> +		/* disable further lp alarm interrupts */
> +		lp_cr &= ~(SRTC_LPCR_ALP | SRTC_LPCR_WAE);
> +	}
> +
> +	/* Update interrupt enables */
> +	writel(lp_cr, ioaddr + SRTC_LPCR);
> +
> +	/* clear interrupt status */
> +	writel(lp_status, ioaddr + SRTC_LPSR);
> +
> +	mxc_rtc_sync_lp_locked(ioaddr);
> +	rtc_update_irq(pdata->rtc, 1, events);

... because calling it here with events == 0 will result in a lot of
work for nothing (the core will walk through the timers and set the
alarm again).


-- 
Alexandre Belloni, 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