Re: + blackfin-on-chip-rtc-controller-driver.patch added to -mm tree

Re: + blackfin-on-chip-rtc-controller-driver.patch added to -mm tree

[Mike Frysinger]

On 3/1/07, David Brownell <david-b@pacbell.net> wrote:
> Bryan, it'd be nice to see a followup patch addressing those
> comments from Paul Mundt, especially about that code which
> will spin-forever-under-spinlock.  That spin should probably
> drop the lock then msleep(1) then restore it before retest...

i've been chatting with Paul on irc about it

> The set_alarm() method needs to enable the alarm irq if the
> "enabled" flag is set, and the read_alarm() method needs to
> report whether the alarm is enabled.

from reading other drivers and the documentation, i couldnt determine
whether this was the standard behavior or whether set_alarm simply set
the alarm time but you still needed to call the ioctl RTC_AIE_ON in
order to actual enable it

> It's unclear why you
> chose to report "pending" (irq issued but not yet acked) since
> that's uselessly transient state on non-polled hardwre.  (That
> flag definition came from EFI, a polled firmware RTC.)

because other drivers do ?  there is no documentation here that covers
what exactly the read_alarm function in the rtc_class_ops is supposed
to do which leaves it up to people looking at other drivers

rtc-sa1100.c for example:
static int sa1100_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
{
    memcpy(&alrm->time, &rtc_alarm, sizeof(struct rtc_time));
    alrm->pending = RTSR & RTSR_AL ? 1 : 0;
    return 0;
}

if someone decides what the behavior is supposed to be here, i'll
happily implement it

> Correct handling of the "enabled" flag in read_alarm() will let
> you remove a redundant seq_printf() in your proc() callback...

i dont understand this ... rtc-sh.c and rtc-sa1100.c both do a
seq_printf() for the alarm irq

> It looks to me like you're handling the time in set_alarm()
> incorrectly.  Do the conversion and test in set_alarm, not
> AIE_ON ... since set_alarm() needs to be able to include AIE_ON
> capability.

the reason for this is so that i do not have to worry about keeping
two structures in sync ... there's the common RTC representation and
then there's the funky Blackfin representation, so by delaying the
conversion until RTC_AIE_ON, i didnt have to worry about keeping these
fields in sync

> The fields tm_{wday,yday,isdst} are never used,
> but you're testing tm_yday.  If you meant to test tm_mday in
> order to distinguish the WKALM_SET and ALM_SET cases, then you
> are mishandling a wraparound case; see how rtc-omap handles
> it.  (If it's 23:00 now, an 01:00 alarm means TOMORROW.  I
> think other RTCs may goof this case too.)

this is exactly why i'm checking tm_yday ... the rtc-dev interface
sets those fields to -1 when using the WKALM ioctls and there's no
other way to distinguish this

if the wraparound case is so common, then perhaps it should be a
helper function in the generic rtc code rather than reimplementing in
every rtc driver ...

> The use of class_device conflicts with the patch series I
> recently resubmitted, see it on rtc-linux@googlegroups.com ...
> if you submit a separate patch, then maybe your Blackfin RTC
> can be merged first.

i'll take a look
-mike

Re: + blackfin-on-chip-rtc-controller-driver.patch added to -mm tree

[Alessandro Zummo]

On Thu, 1 Mar 2007 14:06:49 -0500
"Mike Frysinger" <vapier.adi@gmail.com> wrote:

> > It's unclear why you
> > chose to report "pending" (irq issued but not yet acked) since
> > that's uselessly transient state on non-polled hardwre.  (That
> > flag definition came from EFI, a polled firmware RTC.)
> 
> because other drivers do ?  there is no documentation here that covers
> what exactly the read_alarm function in the rtc_class_ops is supposed
> to do which leaves it up to people looking at other drivers

 there has been a bit of confusion on this topic. I think it's 
 now the time to exactly define what should happen with
 alarms and how they should be reported back to the class.

 I'm usually inclined to agree with David on that particular topic :)

 other than that, I would like to remove the /proc interface in the
 future, so users are advised not to count on it and device
 authors to not add properties.

 

-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it

Re: + blackfin-on-chip-rtc-controller-driver.patch added to -mm tree

[David Brownell]

On Thursday 01 March 2007 11:06 am, Mike Frysinger wrote:

> > The set_alarm() method needs to enable the alarm irq if the
> > "enabled" flag is set, and the read_alarm() method needs to
> > report whether the alarm is enabled.
> 
> from reading other drivers and the documentation,

That particular call came from the RTC interface exposed by EFI,
which is pretty clear on things.  The only notable glitch in
translating it to Linux is that Linux splits "alarm" and "wakeup"
capabilities ... but then, EFI lets you poll that RTC and gives
effectively that same consequence:  alarm not coupled to wakeup,
at least if the system isn't asleep.


> > It's unclear why you
> > chose to report "pending" (irq issued but not yet acked) since
> > that's uselessly transient state on non-polled hardwre.  (That
> > flag definition came from EFI, a polled firmware RTC.)
> 
> because other drivers do ?  there is no documentation here that covers
> what exactly the read_alarm function in the rtc_class_ops is supposed
> to do which leaves it up to people looking at other drivers
> 
> rtc-sa1100.c for example:
> static int sa1100_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> {
>     memcpy(&alrm->time, &rtc_alarm, sizeof(struct rtc_time));
>     alrm->pending = RTSR & RTSR_AL ? 1 : 0;
>     return 0;
> }

Look again, but at 2.6.21-rc2 instead ... ISTR that fix went into RC1,
but that one's been fixed in various trees (like handhelds.org) for
some time.


> if someone decides what the behavior is supposed to be here, i'll
> happily implement it
> 
> > Correct handling of the "enabled" flag in read_alarm() will let
> > you remove a redundant seq_printf() in your proc() callback...
> 
> i dont understand this ... rtc-sh.c and rtc-sa1100.c both do a
> seq_printf() for the alarm irq

Look again at current kernel sources ... bugfixes have been merged
since the codebase you seem to have used as a reference.  I did an
audit of driver handling of those flags; most drivers are now fixed.


> > It looks to me like you're handling the time in set_alarm()
> > incorrectly.  Do the conversion and test in set_alarm, not
> > AIE_ON ... since set_alarm() needs to be able to include AIE_ON
> > capability.
> 
> the reason for this is so that i do not have to worry about keeping
> two structures in sync ... there's the common RTC representation and
> then there's the funky Blackfin representation, so by delaying the
> conversion until RTC_AIE_ON, i didnt have to worry about keeping these
> fields in sync

Yes, Blackfin has an unusually funky representation, but that
doesn't mean you can't do the conversion when the alarm time
comes into the driver.  It's not like the alarm is based on
offset-from-current-time, so that it'd need to be adjusted when
the current time gets updated (like some RTCs)... it's just a
strange way to encode an absolute time.


> > The fields tm_{wday,yday,isdst} are never used,
> > but you're testing tm_yday.  If you meant to test tm_mday in
> > order to distinguish the WKALM_SET and ALM_SET cases, then you
> > are mishandling a wraparound case; see how rtc-omap handles
> > it.  (If it's 23:00 now, an 01:00 alarm means TOMORROW.  I
> > think other RTCs may goof this case too.)
> 
> this is exactly why i'm checking tm_yday ... the rtc-dev interface
> sets those fields to -1 when using the WKALM ioctls and there's no
> other way to distinguish this

Thing is, "man 4 rtc" reports those fields as unused, which means
they can come from userspace with arbitrary values.  You should
stick to testing tm_mday, which *is* used.


> if the wraparound case is so common, then perhaps it should be a
> helper function in the generic rtc code rather than reimplementing in
> every rtc driver ...

You have a point there.  Someone (other than me) should come
up with a patch to handle that.

- Dave

 
> > The use of class_device conflicts with the patch series I
> > recently resubmitted, see it on rtc-linux@googlegroups.com ...
> > if you submit a separate patch, then maybe your Blackfin RTC
> > can be merged first.
> 
> i'll take a look
> -mike
> 

Re: + blackfin-on-chip-rtc-controller-driver.patch added to -mm tree

[Mike Frysinger]

On 3/1/07, David Brownell <david-b@pacbell.net> wrote:
> Look again, but at 2.6.21-rc2 instead ... ISTR that fix went into RC1,
> but that one's been fixed in various trees (like handhelds.org) for
> some time.

this driver was originally written against 2.6.19.x as we havent
gotten the Blackfin arch into mainline yet so it's hard to track the
latest git

i'll pull the latest git and look into the pointers you've posted ...
and send out a sep patch for updating the documentation ;)

> > > It looks to me like you're handling the time in set_alarm()
> > > incorrectly.  Do the conversion and test in set_alarm, not
> > > AIE_ON ... since set_alarm() needs to be able to include AIE_ON
> > > capability.
> >
> > the reason for this is so that i do not have to worry about keeping
> > two structures in sync ... there's the common RTC representation and
> > then there's the funky Blackfin representation, so by delaying the
> > conversion until RTC_AIE_ON, i didnt have to worry about keeping these
> > fields in sync
>
> Yes, Blackfin has an unusually funky representation, but that
> doesn't mean you can't do the conversion when the alarm time
> comes into the driver.  It's not like the alarm is based on
> offset-from-current-time, so that it'd need to be adjusted when
> the current time gets updated (like some RTCs)... it's just a
> strange way to encode an absolute time.

fair enough ... my other concern was that now i'll also have to
translate it back out in rtc_read_alarm so by maintaining everything
in common RTC terms, there would be no overhead

> > > The fields tm_{wday,yday,isdst} are never used,
> > > but you're testing tm_yday.  If you meant to test tm_mday in
> > > order to distinguish the WKALM_SET and ALM_SET cases, then you
> > > are mishandling a wraparound case; see how rtc-omap handles
> > > it.  (If it's 23:00 now, an 01:00 alarm means TOMORROW.  I
> > > think other RTCs may goof this case too.)
> >
> > this is exactly why i'm checking tm_yday ... the rtc-dev interface
> > sets those fields to -1 when using the WKALM ioctls and there's no
> > other way to distinguish this
>
> Thing is, "man 4 rtc" reports those fields as unused, which means
> they can come from userspace with arbitrary values.  You should
> stick to testing tm_mday, which *is* used.

i dont rely on the manpages for kernel interfaces as they get stale
too fast ... i read the files found in Documentation, rtc.txt in this
case

i guess rtc.txt needs more stuff added to it
-mike