[PATCH v3 1/2] ARM: sun4i/sun7i: RTC driver

[maxime.ripard@free-electrons.com (Maxime Ripard)]

On Sat, Nov 16, 2013 at 01:58:35PM +0100, Carlo Caione wrote:
> On Sat, Nov 16, 2013 at 9:51 AM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > Hi Carlo,
> >
> > On Fri, Nov 15, 2013 at 11:50:56PM +0100, Carlo Caione wrote:
> >> >> +static int sunxi_rtc_settime(struct device *dev, struct rtc_time *rtc_tm)
> >> >> +{
> >> >> +     struct sunxi_rtc_dev *chip = dev_get_drvdata(dev);
> >> >> +     u32 date = 0;
> >> >> +     u32 time = 0;
> >> >> +     int year;
> >> >> +     int t;
> >> >> +
> >> >> +     year = rtc_tm->tm_year + 1900;
> >> >
> >> > So, that means that the A10 RTC starts a year 1900, and the A20 at
> >> > year 2010? Why not just put those in your year_offset fieldd directly?
> >>
> >> 1900 is the offset of the parameter tm_year with respect to the current year.
> >> year_offset is used to make tm_year fit inside the limited range of
> >> the year field of the SUNXI_RTC_YMD register.
> >
> > Ok, so 1900 is the RTC's epoch, and tm_year is relative to that
> > epoch. What I still don't get is why you're not defining it as such
> > and use it directly the field your year_offset as such.
> 
> Because for A10 the year field is only 6 bit wide and it can hold only
> 64 years so I cannot use year_offset directly.
> We have the RTC framework where everything is based on 1900 and the
> date register that is only 6 bit wide. Without year_offset I would
> cover only 64 years from 1900 to 1963.
> I need year_offset to shift the base year from 1900 to 2010 such that
> I can use those 6 bit to track years from 2010 to 2073.

I'm sorry, but this is not at all what you have in your driver.

the 6 bit width and the start-at-2010 is set for the A20, not the A10.

> > Moreover, you have some max values that imply that the RTC can't count
> > over either 2100 or 2073, which is kind of weird, since, respectively,
> > it starts at 1900 and 2010, and the year field is documented to be 8
> > bits wide, I'd expect it to go up to 2155 and 2265.
> 
> For A10 the date register is documented to be 6 bit. For A20 it is 8
> bit wide (here the "mask" field in the sunxi_rtc_data_year).
> In the v4 I'm going to expand to 255 the year range for A20 and modify
> the min year to make it starts also at 1900 (it as actually wrong
> having a the min field set to 1970).
> (BTW I just noticed that the indexes for sunxi_rtc_data_year are
> swapped, one more fix in v4).

Ah, yes, I guess you found out already.

Sorry for the confusion.

> >> >> +
> >> >> +     /*
> >> >> +      * wait about 70us to make sure the the time is really written into
> >> >> +      * target
> >> >> +      */
> >> >> +     usleep_range(70, 100);
> >> >
> >> > Isn't the write operation supposed to be done already?
> >>
> >> Yes, it is supposed to be, but this waiting was already present in the
> >> original version of the driver by aw so I'll leave it in v4.
> >
> > I feel like we argue over this again and again.
> >
> > I'm sorry, but saying "allwinner did it that way so we should keep it"
> > is a bad argument. Otherwise we would be using fex, and every other
> > driver Allwinner reimplemented.
> >
> > That being said, either that sleep is needed, and then it should be
> > needed everytime you write to the RTC, which is not the case iirc, and
> > have a comment saying "I have no idea why, but this come from
> > Allwinner and this is actually required", or you don't need it, and
> > then you can just remove it.
> >
> > But having it some-times-but-not-some-other seems pretty weak to me.
> 
> Maxime, as you know documentation for AW's SoC is really weak. I can
> only extrapolate information from the old drivers and the badly
> written comments to the code.
> In those drivers (in all the versions I have) this waiting is always
> present with the same comment I have left in the driver. What I
> imagined is that it is needed not always, but only when I write
> SUNXI_LOSC_CTRL_RTC_HMS_ACC or SUNXI_LOSC_CTRL_RTC_YMD_ACC (again,
> this is just a guess but in the comment there is an explicit reference
> to "time").

My point is that you're not even using that for every access to DHMS.

Let me see what I can find out about this.

> I actually have no idea why and removing it in my tests doesn't affect
> the RTC. So I had two possibilities: 1) leave it in the code because
> somehow it seems important even though I don't know exactly why it is
> (only) there or 2) leave it out knowing that it doesn't affect my
> tests but it could in some particular case or configuration.
> That said, I'm going to do some more tests without the usleep to see
> if there is any problem otherwise I'll delete it in v4.

Ok.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131119/d0566852/attachment.sig>