Patchwork Re: [PATCH v2] rtc: snvs: add Freescale rtc-snvs driver

login
register
mail settings
Submitter Shawn Guo
Date July 9, 2012, 6:51 a.m.
Message ID <20120709065116.GB11635@S2101-09.ap.freescale.net>
Download mbox | patch
Permalink /patch/169697/
State New
Headers show

Comments

Shawn Guo - July 9, 2012, 6:51 a.m.
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
Kim Phillips - July 10, 2012, 12:45 a.m.
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
Shawn Guo - July 10, 2012, 2:32 a.m.
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.
Kim Phillips - July 11, 2012, 12:02 a.m.
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
Shawn Guo - July 11, 2012, 2:57 a.m.
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.
Kim Phillips - July 12, 2012, 1:50 a.m.
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
Shawn Guo - July 12, 2012, 2:50 a.m.
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.

Patch

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 */