[PATCH v2] RTC: PXA: Fix regression of interrupt before ioremap

[PATCH v2] RTC: PXA: Fix regression of interrupt before ioremap

[Petr Cvek]

Interrupts appear before register set of the PXA2xx RTC controller is ioremaped.

This fixes regression from:
  'commit a44802f8fb7e593adabc6ef53c8df45a1717fa9b ("drivers/rtc/rtc-pxa.c: fix alarm can't wake up system issue")'
  'commit 2f6e5f9458646263d3d9ffadd5e11e3d8d15a7d0 ("drivers/rtc: remove IRQF_DISABLED")'

Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
---
 drivers/rtc/rtc-pxa.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-pxa.c b/drivers/rtc/rtc-pxa.c
index 4561f37..d7d83e5 100644
--- a/drivers/rtc/rtc-pxa.c
+++ b/drivers/rtc/rtc-pxa.c
@@ -346,7 +346,7 @@ static int __init pxa_rtc_probe(struct platform_device *pdev)
 		dev_err(dev, "No alarm IRQ resource defined\n");
 		return -ENXIO;
 	}
-	pxa_rtc_open(dev);
+
 	pxa_rtc->base = devm_ioremap(dev, pxa_rtc->ress->start,
 				resource_size(pxa_rtc->ress));
 	if (!pxa_rtc->base) {
@@ -375,6 +375,10 @@ static int __init pxa_rtc_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	ret = pxa_rtc_open(dev);
+	if (ret)
+		return ret;
+
 	device_init_wakeup(dev, 1);
 
 	return 0;
-- 
1.7.12.1

[PATCH v2] RTC: PXA: Fix regression of interrupt before ioremap

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

> Interrupts appear before register set of the PXA2xx RTC controller is ioremaped.
>
> This fixes regression from:
>   'commit a44802f8fb7e593adabc6ef53c8df45a1717fa9b ("drivers/rtc/rtc-pxa.c: fix alarm can't wake up system issue")'
>   'commit 2f6e5f9458646263d3d9ffadd5e11e3d8d15a7d0 ("drivers/rtc: remove IRQF_DISABLED")'
>
> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>

No sorry, I don't like this.
It's not your patch I don't like, it fixes a real problem, but what happens then
if :
 - kernel boots
 - a process opens /dev/rtc0

The real issue is with patch a44802f "drivers/rtc/rtc-pxa.c: fix alarm can't
wake up system issue". I'd rather have you revert a44802f, which makes no sense
to me ...

Leo if you want to comment on it, feel free, and tell me if you tried your patch
with the code in Documentation/rtc.txt ?

Cheers.

--
Robert

[PATCH v2, RFC] RTC: PXA: Fix regression of interrupt before ioremap

[Petr Cvek]

I agree that driver without .open looks ugly, but only thing in rtc-pxa 
.open were two request_irq and I don't think it is good idea to have 
them there (interrupts should be disabled trough register settings and 
not by handler freeing).

I'm not familiar with the linux RTC subsystem, so I don't know if it is 
OK to get interrupt (and rtc_update_irq) without opened /dev/rtc. 
Intuitively I have feeling it is OK, but even if not disabling can be 
done with some register flag.

BTW It seems that kernel have only around 9 drivers in drivers/rtc which 
contain .open function.

OT: rtc-sa1100 seems to be compatible with PXAxxx (it is even in 
Kconfig). Are there any reasons to have two drivers for one SoC?

Petr

On 29.1.2015 20:42, Robert Jarzmik wrote:
> Petr Cvek <petr.cvek@tul.cz> writes:
>
>> Interrupts appear before register set of the PXA2xx RTC controller is ioremaped.
>>
>> This fixes regression from:
>>    'commit a44802f8fb7e593adabc6ef53c8df45a1717fa9b ("drivers/rtc/rtc-pxa.c: fix alarm can't wake up system issue")'
>>    'commit 2f6e5f9458646263d3d9ffadd5e11e3d8d15a7d0 ("drivers/rtc: remove IRQF_DISABLED")'
>>
>> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
>
> No sorry, I don't like this.
> It's not your patch I don't like, it fixes a real problem, but what happens then
> if :
>   - kernel boots
>   - a process opens /dev/rtc0
>
> The real issue is with patch a44802f "drivers/rtc/rtc-pxa.c: fix alarm can't
> wake up system issue". I'd rather have you revert a44802f, which makes no sense
> to me ...
>
> Leo if you want to comment on it, feel free, and tell me if you tried your patch
> with the code in Documentation/rtc.txt ?
>
> Cheers.
>
> --
> Robert
>

[PATCH v2, RFC] RTC: PXA: Fix regression of interrupt before ioremap

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

> I agree that driver without .open looks ugly, but only thing in rtc-pxa .open
> were two request_irq and I don't think it is good idea to have them there
> (interrupts should be disabled trough register settings and not by handler
> freeing).
>
> I'm not familiar with the linux RTC subsystem, so I don't know if it is OK to
> get interrupt (and rtc_update_irq) without opened /dev/rtc. Intuitively I have
> feeling it is OK, but even if not disabling can be done with some register flag.
>
> BTW It seems that kernel have only around 9 drivers in drivers/rtc which contain
> .open function.
>
> OT: rtc-sa1100 seems to be compatible with PXAxxx (it is even in Kconfig). Are
> there any reasons to have two drivers for one SoC?
Yes, there is a reason :
  http://marc.info/?l=linux-arm-kernel&m=122306289606732&w=2

At that time we decided this were 2 different IPs (more or less) sharing the
same IO region and IRQ. 2 IPs for pxa27x and greater, only 1 IP for pxa25x and
lower.

Now you should know that both rtc-sa1100 and rtc-pxa should be able to work
together in the same kernel (at least that was the case so far). The open()
decided who got a grip on the interrupt. This lets userland choose which rtc it
relies on : either the increasing count, or the
day/month/year/hour/minute/second counters (which are independant).

Moreover, if there are multiple rtc device, how on earth can it work, ie. how
can an ioctl() be sent to a specific rtc device if there is no open() ???

Cheers.

--
Robert

[PATCH v2, RFC] RTC: PXA: Fix regression of interrupt before ioremap

[Petr Cvek]

On 2.2.2015 19:33, Robert Jarzmik wrote:
> Petr Cvek <petr.cvek@tul.cz> writes:
>
>> I agree that driver without .open looks ugly, but only thing in rtc-pxa .open
>> were two request_irq and I don't think it is good idea to have them there
>> (interrupts should be disabled trough register settings and not by handler
>> freeing).
>>
>> I'm not familiar with the linux RTC subsystem, so I don't know if it is OK to
>> get interrupt (and rtc_update_irq) without opened /dev/rtc. Intuitively I have
>> feeling it is OK, but even if not disabling can be done with some register flag.
>>
>> BTW It seems that kernel have only around 9 drivers in drivers/rtc which contain
>> .open function.
>>
>> OT: rtc-sa1100 seems to be compatible with PXAxxx (it is even in Kconfig). Are
>> there any reasons to have two drivers for one SoC?
> Yes, there is a reason :
>    http://marc.info/?l=linux-arm-kernel&m=122306289606732&w=2
>
> At that time we decided this were 2 different IPs (more or less) sharing the
> same IO region and IRQ. 2 IPs for pxa27x and greater, only 1 IP for pxa25x and
> lower.
>
> Now you should know that both rtc-sa1100 and rtc-pxa should be able to work
> together in the same kernel (at least that was the case so far). The open()
> decided who got a grip on the interrupt. This lets userland choose which rtc it
> relies on : either the increasing count, or the
> day/month/year/hour/minute/second counters (which are independant).
>

I see, thanks for info, I never thought that these two drivers were 
specially crafted to be able to coexist (if it is good idea I can write 
info for Kconfig help section).

I still don't think it is a good idea to do "mutex" by racing who gets 
both request_irq first, but I don't have better solution other than 
making both drivers fully independent on each other or merging them 
together (probably with checkbox for enabling enhanced features in PXA27x).

Actually only thing I want to know after reverting a44802f is how wakeup 
will work. Because a44802f suggests rtc-pxa needs to have interrupt 
enabled for waking up (and I cannot test it, because suspend subsystem 
on my machine needs to be fixed first).

> Moreover, if there are multiple rtc device, how on earth can it work, ie. how
> can an ioctl() be sent to a specific rtc device if there is no open() ???

It confuses me too, so I tried to look it up and it seems rtc_dev_open() 
in drivers/rtc/rtc-dev.c handles this by:

	err = ops->open ? ops->open(rtc->dev.parent) : 0;
     if (err == 0) {
         spin_lock_irq(&rtc->irq_lock);
         rtc->irq_data = 0;
         spin_unlock_irq(&rtc->irq_lock);

         return 0;
     }

, so without any .open() it just continues with success.

Petr

[PATCH v2, RFC] RTC: PXA: Fix regression of interrupt before ioremap

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

> On 2.2.2015 19:33, Robert Jarzmik wrote:
>> Petr Cvek <petr.cvek@tul.cz> writes:
> Actually only thing I want to know after reverting a44802f is how wakeup will
> work. Because a44802f suggests rtc-pxa needs to have interrupt enabled for
> waking up (and I cannot test it, because suspend subsystem on my machine needs
> to be fixed first).
Process X does :
 - open /dev/rtc0
 - call ioctl(fd, RTC_ALM_SET, &time_of_wakeup)
 - call ioctl(fd, RTC_AIE_ON, 0)
   - either read(fd, &data, sizeof(unsigned long))
   - or does write "mem" > /sys/power/state

 - ... platform sleeps ...
 - alarm time comes up, RTC IP raises the interrupt line
 - because in its pxa_rtc_probe(), the driver called device_init_wakeup(dev, 1),
   the register PWER was set to wakeup the platform if RTC interrupt is raised,
   the platform wakes up

>> Moreover, if there are multiple rtc device, how on earth can it work, ie. how
>> can an ioctl() be sent to a specific rtc device if there is no open() ???
>
> It confuses me too, so I tried to look it up and it seems rtc_dev_open() in
> drivers/rtc/rtc-dev.c handles this by:
>
> 	err = ops->open ? ops->open(rtc->dev.parent) : 0;
>     if (err == 0) {
>         spin_lock_irq(&rtc->irq_lock);
>         rtc->irq_data = 0;
>         spin_unlock_irq(&rtc->irq_lock);
>
>         return 0;
>     }
>
> , so without any .open() it just continues with success.
Yes, true, yet how do you set on a specific RTC block the alarm if you have many
of them on the system ?

Cheers.

-- 
Robert

[PATCH v2, RFC] RTC: PXA: Fix regression of interrupt before ioremap

[Petr Cvek]

On 3.2.2015 19:31, Robert Jarzmik wrote:
> Petr Cvek <petr.cvek@tul.cz> writes:
> 
>> On 2.2.2015 19:33, Robert Jarzmik wrote:
>>> Petr Cvek <petr.cvek@tul.cz> writes:
>> Actually only thing I want to know after reverting a44802f is how wakeup will
>> work. Because a44802f suggests rtc-pxa needs to have interrupt enabled for
>> waking up (and I cannot test it, because suspend subsystem on my machine needs
>> to be fixed first).
> Process X does :
>  - open /dev/rtc0
>  - call ioctl(fd, RTC_ALM_SET, &time_of_wakeup)
>  - call ioctl(fd, RTC_AIE_ON, 0)
>    - either read(fd, &data, sizeof(unsigned long))
>    - or does write "mem" > /sys/power/state
> 
>  - ... platform sleeps ...
>  - alarm time comes up, RTC IP raises the interrupt line
>  - because in its pxa_rtc_probe(), the driver called device_init_wakeup(dev, 1),
>    the register PWER was set to wakeup the platform if RTC interrupt is raised,
>    the platform wakes up
> 

I was thinking more about setting alarm, ending the OS (and all processes), powering down DRAM, SRAM etc. and then waiting for alarm (like x86 BIOS alarm) to restart.

>>> Moreover, if there are multiple rtc device, how on earth can it work, ie. how
>>> can an ioctl() be sent to a specific rtc device if there is no open() ???
>>
>> It confuses me too, so I tried to look it up and it seems rtc_dev_open() in
>> drivers/rtc/rtc-dev.c handles this by:
>>
>> 	err = ops->open ? ops->open(rtc->dev.parent) : 0;
>>     if (err == 0) {
>>         spin_lock_irq(&rtc->irq_lock);
>>         rtc->irq_data = 0;
>>         spin_unlock_irq(&rtc->irq_lock);
>>
>>         return 0;
>>     }
>>
>> , so without any .open() it just continues with success.
> Yes, true, yet how do you set on a specific RTC block the alarm if you have many
> of them on the system ?

I thought it should be possible with ioctl with appropriate /dev/rtcX opened or /sys/class/rtc/rtcX/wakealarm . 

It seems driver still does not work properly (with reverted patch). For first run the /sys/class/rtc/rtcX/wakealarm file is not created, but it is created for next reload of rtc-pxa module. And it seems that it is caused by .can_wakeup somewhere. 

P.S. Testing application from Documentation/rtc.txt seems to run OK.

[PATCH v2, RFC] RTC: PXA: Fix regression of interrupt before ioremap

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

> I was thinking more about setting alarm, ending the OS (and all processes),
> powering down DRAM, SRAM etc. and then waiting for alarm (like x86 BIOS alarm)
> to restart.
Ah yes, this is a case I have not considered before.
Yet I fail to see what the open/close removal patch fixes.

>> Yes, true, yet how do you set on a specific RTC block the alarm if you have many
>> of them on the system ?
>
> I thought it should be possible with ioctl with appropriate /dev/rtcX opened or /sys/class/rtc/rtcX/wakealarm . 
>
> It seems driver still does not work properly (with reverted patch). For first
> run the /sys/class/rtc/rtcX/wakealarm file is not created, but it is created for
> next reload of rtc-pxa module. And it seems that it is caused by .can_wakeup
> somewhere.
Do you know why, and would you have a patch for that ?

Cheers.

-- 
Robert