rtc: snvs: add Freescale rtc-snvs driver

Add an RTC driver for Freescale Secure Non-Volatile Storage (SNVS) Low Power (LP) RTC.

Signed-off-by Shawn Guo <[email protected]>

Cc Alessandro Zummo <[email protected]>

drivers/rtc/rtc-snvs.c new file +15 -0

+#ifdef CONFIG_PM
+static int snvs_rtc_suspend(struct device *dev)
+{
+	struct snvs_rtc_data *data = dev_get_drvdata(dev);
+
+	if (device_may_wakeup(dev))
+		enable_irq_wake(data->irq);
+
+	return 0;
+}
+
+static int snvs_rtc_resume(struct device *dev)
+{
+	struct snvs_rtc_data *data = dev_get_drvdata(dev);
+

drivers/rtc/rtc-snvs.c new file +5 -0

+#endif
+
+static const struct of_device_id __devinitconst snvs_dt_ids[] = {
+	{ .compatible = "fsl,sec-v4.0-mon-rtc-lp", },
+	{ /* sentinel */ }

drivers/rtc/rtc-snvs.c new file +10 -0

+		.of_match_table = snvs_dt_ids,
+	},
+	.probe		= snvs_rtc_probe,
+	.remove		= __devexit_p(snvs_rtc_remove),
+};
+module_platform_driver(snvs_rtc_driver);
+
+MODULE_AUTHOR("Freescale Semiconductor, Inc.");
+MODULE_DESCRIPTION("Freescale SNVS RTC Driver");
+MODULE_LICENSE("GPL");

drivers/rtc/rtc-snvs.c line 334

Quoted 2 lines · On Wed, 4 Jul 2012 23:38:14 +0800 Shawn Guo <[email protected]> wrote:

drivers/rtc/rtc-snvs.c new file +2 -0

+static const struct of_device_id __devinitconst snvs_dt_ids[] = {
+	{ .compatible = "fsl,sec-v4.0-mon-rtc-lp", },

the driver should be compatible with "fsl,sec-v4.0-mon", and its probe function search for "fsl,sec-v4.0-mon-rtc-lp" sub-compatibles (and possibly "fsl,sec-v4.0-mon-rtc-hp" in the future?).

Quoted 3 lines · Kim Phillips

On Wed, 4 Jul 2012 23:38:14 +0800
Shawn Guo <[email protected]> wrote:

Quoted 7 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+static const struct of_device_id __devinitconst snvs_dt_ids[] = {
+	{ .compatible = "fsl,sec-v4.0-mon-rtc-lp", },

the driver should be compatible with "fsl,sec-v4.0-mon", and its probe function search for "fsl,sec-v4.0-mon-rtc-lp" sub-compatibles (and possibly "fsl,sec-v4.0-mon-rtc-hp" in the future?).

So you think snvs is all about rtc (lp and hp), nothing else? If that

is true, it's reasonable to do what you suggested here. Otherwise, we should have a snvs driver be compatible with "fsl,sec-v4.0-mon", and this driver should be responsible for populate all the sub-devices of snvs, rtc-lp, rtc-hp, and others. In the latter case, rtc-snvs should just stay as it stands right now.

but it doesn't function as it stands right now, at least on Power. The compatible in the device tree's sec_mon node "fsl,sec-v4.0-mon" and the driver's "fsl,sec-v4.0-mon-rtc-lp" don't match. Here are the device tree changes I used:

arch/powerpc/boot/dts/fsl/qoriq-sec4.2-0.dtsi +1 -0

index 7990e0d..14c933b 100644
@@ -104,6 +104,14 @@ crypto: crypto at 300000 {
 
 sec_mon: sec_mon at 314000 {
        compatible = "fsl,sec-v4.2-mon", "fsl,sec-v4.0-mon";
+       #address-cells = <1>;

btw, I don't see any imx6q.dtsi changes.

Quoted 15 lines · Kim Phillips

but it doesn't function as it stands right now, at least on Power. The compatible in the device tree's sec_mon node "fsl,sec-v4.0-mon" and the driver's "fsl,sec-v4.0-mon-rtc-lp" don't match. Here are the device tree changes I used:

arch/powerpc/boot/dts/fsl/qoriq-sec4.2-0.dtsi +1 -0

index 7990e0d..14c933b 100644
@@ -104,6 +104,14 @@ crypto: crypto at 300000 {
 
 sec_mon: sec_mon at 314000 {
        compatible = "fsl,sec-v4.2-mon", "fsl,sec-v4.0-mon";
+       #address-cells = <1>;

arch/powerpc/boot/dts/fsl/qoriq-sec4.2-0.dtsi crypto: crypto at 300000 { · line 117

Quoted 10 lines

arch/powerpc/boot/dts/fsl/qoriq-sec4.2-0.dtsi +7 -0

+       #size-cells = <1>;
+       ranges = <0 0x314000 0x1000>;
        reg = <0x314000 0x1000>;
        interrupts = <93 2 0 0>;
+
+       sec_mon_rtc_lp at 34 {
+               compatible = "fsl,sec-v4.2-mon-rtc-lp", "fsl,sec-v4.0-mon-rtc-lp";
+               reg = <0x34 0x58>;
+       };
 };

Yes, the way that this dts is written as well as the examples in Documentation/devicetree/bindings/crypto/fsl-sec4.txt assume there is a sec_mon driver. This driver will match "fsl,sec-v4.0-mon" and populate rtc-lp device probed by the rtc-snvs driver created by the patch.

While there is no such sec_mon driver right now. I did a quick tweaking on imx6q.dtsi to have rtc-snvs probed without the need of sec_mon driver.

Quoted 2 lines

btw, I don't see any imx6q.dtsi changes.

See below.

Either way, being a driver of SNVS LP RTC, rtc-snvs should just stand as it stands right now. It should not care about how the device is populated, by DT core or by sec_mon driver.

Regards,
Shawn

arch/arm/boot/dts/imx6q.dtsi +0 -0

index 8c90cba..26c42da 100644

I'm not sure if this is appropriate vs. having common arch code publish SNVS devices; see e.g., the other fsl,* devices in arch/powerpc/platforms/85xx/common.c:mpc85xx_common_ids[].

Either way, I'd like to get support without having to manually tweak the device tree, i.e., the patch doesn't work as-is.

btw, testing on a p4080r2 produces a soft lockup:

root at p4080ds:~# modprobe rtc-snvs
snvs_rtc ffe314034.sec_mon_rtc_lp: rtc core: registered ffe314034.sec_mon_r as rtc1 root at p4080ds:~# hwclock -w -f /dev/rtc1

BUG: soft lockup - CPU#0 stuck for 23s! [kworker/0:0:4]
Modules linked in: rtc_snvs nfsd exportfs
irq event stamp: 54
hardirqs last  enabled at (53): [<c04f90dc>] _raw_spin_unlock_irq+0x30/0x58
hardirqs last disabled at (54): [<c04f8808>] _raw_spin_lock_irq+0x24/0x70
softirqs last  enabled at (0): [<c001eff8>] copy_process+0x2f4/0xde4
softirqs last disabled at (0): [<  (null)>]   (null)
NIP: f10b80b4 LR: f10b8074 CTR: 00000003
REGS: ee071d70 TRAP: 0901   Not tainted  (3.5.0-rc5-00099-gd8715a7-dirty)
MSR: 00029002 <CE,EE,ME>  CR: 42042022  XER: 00000000

TASK = ee06f470[4] 'kworker/0:0' THREAD: ee070000 CPU: 0
GPR00: 00000000 ee071e20 ee06f470 c04f9058 00000001 f10b8074 00000000 00000002
GPR08: 00000001 f10bc054 00000000 00000000 c04f901c
NIP [f10b80b4] snvs_rtc_alarm_irq_enable+0xb4/0xf8 [rtc_snvs]
LR [f10b8074] snvs_rtc_alarm_irq_enable+0x74/0xf8 [rtc_snvs]

Call Trace:
[ee071e20] [f10b8074] snvs_rtc_alarm_irq_enable+0x74/0xf8 [rtc_snvs] (unreliable)
[ee071e40] [c038c118] rtc_timer_do_work+0x184/0x20c
[ee071f10] [c003fc4c] process_one_work+0x1d4/0x48c
[ee071f50] [c0040d4c] worker_thread+0x184/0x358
[ee071f90] [c0046458] kthread+0x84/0x88
[ee071ff0] [c000d434] kernel_thread+0x4c/0x68

Instruction dump:
7c0004ac 7d404c2c 0c0a0000 4c00012c 7c0004ac 7c004c2c 0c000000 4c00012c 7f8a0000 409effdc 7c0004ac 7c004c2c <0c000000> 4c00012c 7c0004ac 7d604c2c

I debugged it to the innermost loop of rtc_write_sync_lp(), where count2 and count3 are always equal. Should there be a timeout in that loop? Do you know if this is because the machine isn't in trusted mode? Any other hints appreciated.

Quoted 4 lines · Kim Phillips

I'm not sure if this is appropriate vs. having common arch code publish SNVS devices; see e.g., the other fsl,* devices in arch/powerpc/platforms/85xx/common.c:mpc85xx_common_ids[].

This is completely beyond the scope of the patch, which merely adds a driver for snvs-lp-rtc device. But the driver does not know how the device is created, and it does not need to know.

Quoted 3 lines

Either way, I'd like to get support without having to manually tweak the device tree, i.e., the patch doesn't work as-is.

As I already said, the patch only adds a driver for snvs-lp-rtc. It should not care about how snvs-lp-rtc device is created on particular platform. The patch does work as-is for imx6q, which is the target of the patch so far.

Quoted 27 lines

btw, testing on a p4080r2 produces a soft lockup:

root at p4080ds:~# modprobe rtc-snvs
snvs_rtc ffe314034.sec_mon_rtc_lp: rtc core: registered ffe314034.sec_mon_r as rtc1 root at p4080ds:~# hwclock -w -f /dev/rtc1

BUG: soft lockup - CPU#0 stuck for 23s! [kworker/0:0:4]
Modules linked in: rtc_snvs nfsd exportfs
irq event stamp: 54
hardirqs last  enabled at (53): [<c04f90dc>] _raw_spin_unlock_irq+0x30/0x58
hardirqs last disabled at (54): [<c04f8808>] _raw_spin_lock_irq+0x24/0x70
softirqs last  enabled at (0): [<c001eff8>] copy_process+0x2f4/0xde4
softirqs last disabled at (0): [<  (null)>]   (null)
NIP: f10b80b4 LR: f10b8074 CTR: 00000003
REGS: ee071d70 TRAP: 0901   Not tainted  (3.5.0-rc5-00099-gd8715a7-dirty)
MSR: 00029002 <CE,EE,ME>  CR: 42042022  XER: 00000000

TASK = ee06f470[4] 'kworker/0:0' THREAD: ee070000 CPU: 0
GPR00: 00000000 ee071e20 ee06f470 c04f9058 00000001 f10b8074 00000000 00000002
GPR08: 00000001 f10bc054 00000000 00000000 c04f901c
NIP [f10b80b4] snvs_rtc_alarm_irq_enable+0xb4/0xf8 [rtc_snvs]
LR [f10b8074] snvs_rtc_alarm_irq_enable+0x74/0xf8 [rtc_snvs]

Call Trace:
[ee071e20] [f10b8074] snvs_rtc_alarm_irq_enable+0x74/0xf8 [rtc_snvs] (unreliable)
[ee071e40] [c038c118] rtc_timer_do_work+0x184/0x20c
[ee071f10] [c003fc4c] process_one_work+0x1d4/0x48c
[ee071f50] [c0040d4c] worker_thread+0x184/0x358
[ee071f90] [c0046458] kthread+0x84/0x88
[ee071ff0] [c000d434] kernel_thread+0x4c/0x68

Quoted 9 lines

Instruction dump:
7c0004ac 7d404c2c 0c0a0000 4c00012c 7c0004ac 7c004c2c 0c000000 4c00012c 7f8a0000 409effdc 7c0004ac 7c004c2c <0c000000> 4c00012c 7c0004ac 7d604c2c

I debugged it to the innermost loop of rtc_write_sync_lp(), where count2 and count3 are always equal. Should there be a timeout in that loop? Do you know if this is because the machine isn't in trusted mode? Any other hints appreciated.

This is probably because readl/writel does not work on powerpc. I experienced the same thing when working on fsl_ssi driver.

My patch only targets imx6q right now.

how, without the "simple-bus" addition to the sec_mon compatibles list in the imx6 device tree (which isn't present in this patch)?

interesting. When I add

(quoted diff) +2 -0

+#define readl in_be32
+#define writel(val, addr)             out_be32(addr, val)

to the top of the driver, the soft-lockup moves to rtc_read_lp_counter(). I'm trying to verify if I have the right h/w revision.

Quoted 3 lines · Kim Phillips

how, without the "simple-bus" addition to the sec_mon compatibles list in the imx6 device tree (which isn't present in this patch)?

This patch does not present anything about sec_mon node.

It's all about sec_mon_rtc_lp node.

The driver itself does not know if the
rtc_lp node is a subnode of sec_mon node at all.

Can you please tell me what exact change you expect me to make on drivers/rtc/rtc-snvs.c? The patch merely adds a lp-rtc driver and should not include any platform specific changes (dts, etc.) The driver and dts changes are two orthogonal parts, and should go via RTC tree and ARCH tree separately.

Quoted 9 lines

interesting. When I add

(quoted diff) +2 -0

+#define readl in_be32
+#define writel(val, addr)             out_be32(addr, val)

to the top of the driver, the soft-lockup moves to rtc_read_lp_counter(). I'm trying to verify if I have the right h/w revision.

You can submit an incremental patch later to make the driver work for powerpc.

Quoted 4 lines · On Wed, 11 Jul 2012 10:57:55 +0800 Shawn Guo <[email protected]> wrote:

Quoted 3 lines · Kim Phillips

how, without the "simple-bus" addition to the sec_mon compatibles list in the imx6 device tree (which isn't present in this patch)?

Quoted 1 line

This patch does not present anything about sec_mon node.

it makes a change to its definition in the device tree documentation.

Quoted 1 line

It's all about sec_mon_rtc_lp node.

the change is that the sec_mon node now includes the sec_mon_rtc_lp node, and they are related, just as in h/w.

Quoted 2 lines

The driver itself does not know if the
rtc_lp node is a subnode of sec_mon node at all.

I understand that, but as it stands (and AFAICT), this patch by itself doesn't enable the driver to be probed on _any_ platform, including ARM.

Quoted 5 lines

Can you please tell me what exact change you expect me to make on drivers/rtc/rtc-snvs.c? The patch merely adds a lp-rtc driver and should not include any platform specific changes (dts, etc.) The driver and dts changes are two orthogonal parts, and should go via RTC tree and ARCH tree separately.

That's odd; I would expect all enabling changes to either be included in the same patch, or at least in the same patchseries, and one subsystem maintainer ack the other. If ARM-related patch processes differ somehow, then so be it.