| Message ID | 20120709065116.GB11635@S2101-09.ap.freescale.net |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, 9 Jul 2012 14:51:18 +0800 Shawn Guo <shawn.guo@linaro.org> wrote: > On Fri, Jul 06, 2012 at 05:56:34PM -0500, Kim Phillips wrote: > > 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: > > > > diff --git a/arch/powerpc/boot/dts/fsl/qoriq-sec4.2-0.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-sec4.2-0.dtsi > > index 7990e0d..14c933b 100644 > > --- a/arch/powerpc/boot/dts/fsl/qoriq-sec4.2-0.dtsi > > +++ b/arch/powerpc/boot/dts/fsl/qoriq-sec4.2-0.dtsi > > @@ -104,6 +104,14 @@ crypto: crypto@300000 { > > > > sec_mon: sec_mon@314000 { > > compatible = "fsl,sec-v4.2-mon", "fsl,sec-v4.0-mon"; > > + #address-cells = <1>; > > + #size-cells = <1>; > > + ranges = <0 0x314000 0x1000>; > > reg = <0x314000 0x1000>; > > interrupts = <93 2 0 0>; > > + > > + sec_mon_rtc_lp@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. ... > > 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 > > diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi > index 8c90cba..26c42da 100644 > --- a/arch/arm/boot/dts/imx6q.dtsi > +++ b/arch/arm/boot/dts/imx6q.dtsi > @@ -455,8 +455,16 @@ > }; > > snvs@020cc000 { > - reg = <0x020cc000 0x4000>; > - interrupts = <0 19 0x04 0 20 0x04>; > + compatible = "fsl,sec-v4.0-mon", "simple-bus"; ah, "simple-bus" was the missing tweak. 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@p4080ds:~# modprobe rtc-snvs snvs_rtc ffe314034.sec_mon_rtc_lp: rtc core: registered ffe314034.sec_mon_r as rtc1 root@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. Kim
On Mon, Jul 09, 2012 at 07:45:14PM -0500, Kim Phillips wrote: > 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. > 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. > btw, testing on a p4080r2 produces a soft lockup: > > root@p4080ds:~# modprobe rtc-snvs > snvs_rtc ffe314034.sec_mon_rtc_lp: rtc core: registered ffe314034.sec_mon_r as rtc1 > root@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. > 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.
On Tue, 10 Jul 2012 10:32:29 +0800 Shawn Guo <shawn.guo@linaro.org> wrote: > On Mon, Jul 09, 2012 at 07:45:14PM -0500, Kim Phillips wrote: > > 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. > > > 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. how, without the "simple-bus" addition to the sec_mon compatibles list in the imx6 device tree (which isn't present in this patch)? > > btw, testing on a p4080r2 produces a soft lockup: > > > > root@p4080ds:~# modprobe rtc-snvs > > snvs_rtc ffe314034.sec_mon_rtc_lp: rtc core: registered ffe314034.sec_mon_r as rtc1 > > root@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. > > > 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. interesting. When I add +#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. Kim
On Tue, Jul 10, 2012 at 07:02:21PM -0500, Kim Phillips wrote: > 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. > interesting. When I add > > +#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.
On Wed, 11 Jul 2012 10:57:55 +0800 Shawn Guo <shawn.guo@linaro.org> wrote: > On Tue, Jul 10, 2012 at 07:02:21PM -0500, Kim Phillips wrote: > > 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 makes a change to its definition in the device tree documentation. > 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. > 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. > 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. > > interesting. When I add > > > > +#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. Turns out there was a mistake in the QorIQ series documentation that exposed SRTC registers even though the SNVS LP SRTC isn't physically present on any of the Power based parts; I've since notified the documentation team. Thanks, Kim
On Wed, Jul 11, 2012 at 08:50:31PM -0500, Kim Phillips wrote: > On Wed, 11 Jul 2012 10:57:55 +0800 > Shawn Guo <shawn.guo@linaro.org> wrote: > > > On Tue, Jul 10, 2012 at 07:02:21PM -0500, Kim Phillips wrote: > > > 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 makes a change to its definition in the device tree documentation. > > > 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. > I'm primarily talking about drivers/rtc/rtc-snvs.c. There is no change required on drivers/rtc/rtc-snvs.c whether the device is created by a sec_mon driver or "simple-bus" approach. > > 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. > It's never a thing that RTC driver should care. As long as the driver is verified to be functional, it's platform's call to enable the device or not on particular board/system. > > 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. > I don't think it's an ARM-related process. The process you mentioned is only used for particular case where a series touches multiple subsystems while the whole series has to go via one tree in order to maintain "git bisect". Since there is no bisect for a new driver to maintain at all, it should just follow the general process, having patches go via respective subsystem tree to minimize the merge conflicts when different trees get merged together.
diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi index 8c90cba..26c42da 100644 --- a/arch/arm/boot/dts/imx6q.dtsi +++ b/arch/arm/boot/dts/imx6q.dtsi @@ -455,8 +455,16 @@ }; snvs@020cc000 { - reg = <0x020cc000 0x4000>; - interrupts = <0 19 0x04 0 20 0x04>; + compatible = "fsl,sec-v4.0-mon", "simple-bus"; + #address-cells = <1>; + #size-cells = <1>; + ranges = <0 0x020cc000 0x4000>; + + snvs-rtc-lp@34 { + compatible = "fsl,sec-v4.0-mon-rtc-lp"; + reg = <0x34 0x58>; + interrupts = <0 19 0x04 0 20 0x04>; + }; }; epit@020d0000 { /* EPIT1 */