| Message ID | 54CA1ECA.8050000@tul.cz |
|---|---|
| State | Rejected |
| Headers | show |
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
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 >
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
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
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.
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.
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.
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;
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(-)