On Sun, Nov 17, 2024 at 3:34 PM Zhaoming Luo <[email protected]> wrote:
> Hi,
Hi,
looks nice to me overall, thanks for working on this! I cannot judge
the actual driver part, so here are some nitpicks about not following
the GNU code style :D I will try to build and test it later.
By the way: apparently gnumach already has some sort of an rtc driver
(at i386/i386at/rtc.c). What do we do about that?
> diff --git a/hurd/rtc.h b/hurd/rtc.h
> new file mode 100644
> index 00000000..6516e86c
> --- /dev/null
> +++ b/hurd/rtc.h
> +struct rtc_time {
> + int tm_sec;
> + int tm_min;
> + int tm_hour;
> + int tm_mday;
> + int tm_mon;
> + int tm_year;
> + int tm_wday;
> + int tm_yday;
> + int tm_isdst;
> +};
Nitpick: please see how struct definitions should be formatted
according to the GNU code style [0].
[0]: https://www.gnu.org/prep/standards/html_node/Formatting.html#Formatting
> diff --git a/rtc/main.c b/rtc/main.c
> new file mode 100644
> index 00000000..daefb95d
> --- /dev/null
> +++ b/rtc/main.c
> +static const struct argp rtc_argp =
> +{ options, parse_opt, 0, "RTC device" };
This would probably be a good place to spell out that RTC stands for
real time clock?
> +int
> +main (int argc, char **argv)
> +{
> + error_t err;
> + mach_port_t bootstrap;
> +
> + argp_parse (&rtc_argp, argc, argv, 0, 0, 0);
> +
> + task_get_bootstrap_port (mach_task_self (), &bootstrap);
> + if (bootstrap == MACH_PORT_NULL)
> + error (1, 0, "Must be started as a translator");
> +
> + /* Reply to our parent */
> + err = trivfs_startup (bootstrap, O_NORW, NULL, NULL, NULL, NULL, &rtccntl);
> + mach_port_deallocate (mach_task_self (), bootstrap);
> + if (err)
> + error (1, err, "trivfs_startup failed");
> +
> + /* Request for permission to do i/o on port numbers 0x70 and 0x71 for
> + accessing RTC registers. */
> + err = ioperm (0x70, 2, true);
> + if (err)
> + error (1, err, "Request IO permission failed");
It's probably a good idea to do this before you reply to the parent,
so you don't end up saying "I'm ready!" and then immediately exit with
an error.
> +error_t
> +trivfs_goaway (struct trivfs_control *fsys, int flags)
> +{
> + exit (0);
> +}
Do you need to do anything to the RTC hardware before you exit to
leave it in a good/well-defined state? Especially if other threads are
accessing it at the same time? (Perhaps not, I don't know.)
> diff --git a/rtc/pioctl-ops.c b/rtc/pioctl-ops.c
> new file mode 100644
> index 00000000..c33403c8
> --- /dev/null
> +++ b/rtc/pioctl-ops.c
> @@ -0,0 +1,223 @@
> +/* Server side implementation for rtc server
> +
> + Copyright (C) 2024 Free Software Foundation, Inc.
> +
> + This file is part of the GNU Hurd.
> +
> + The GNU Hurd is free software; you can redistribute it and/or
> + modify it under the terms of the GNU General Public License as
> + published by the Free Software Foundation; either version 2, or (at
> + your option) any later version.
> +
> + The GNU Hurd is distributed in the hope that it will be useful, but
> + WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program; if not, write to the Free Software
> + Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111, USA. */
> +
> +/* This implementation is largely based on sys-utils/hwclock-cmos.c from
> + util-linux */
> +
> +/* A struct tm has int fields (it is defined in POSIX)
> + tm_sec 0-59, 60 or 61 only for leap seconds
> + tm_min 0-59
> + tm_hour 0-23
> + tm_mday 1-31
> + tm_mon 0-11
> + tm_year number of years since 1900
> + tm_wday 0-6, 0=Sunday
> + tm_yday 0-365
> + tm_isdst >0: yes, 0: no, <0: unknown */
> +
> +#include "rtc_pioctl_S.h"
> +#include <hurd/rtc.h>
> +#include <hurd/hurd_types.h>
> +#include <sys/io.h>
> +#include <stdbool.h>
> +
> +/* Conversions to and from RTC internal format */
> +#define BCD_TO_BIN(val) ((val)=((val)&15) + ((val)>>4)*10)
> +#define BIN_TO_BCD(val) ((val)=(((val)/10)<<4) + (val)%10)
> +
> +/* POSIX uses 1900 as epoch for a struct tm, and 1970 for a time_t. */
> +#define TM_EPOCH 1900
> +
> +static unsigned short clock_ctl_addr = 0x70;
> +static unsigned short clock_data_addr = 0x71;
These should be #define's (or enum members if you prefer), rather than
mutable runtime memory in .data, no?
> +static inline unsigned long cmos_read(unsigned long reg)
> +{
> + outb_p(reg, clock_ctl_addr);
> + return inb_p(clock_data_addr);
> +}
GNU style please:
static inline unsigned long
cmos_read (unsigned long reg)
{
outb_p (reg, clock_ctl_addr);
return inb_p (clock_data_addr);
}
also I believe outb_p's first argument is an 'unsigned char', so why
unsigned long here?
> +
> +static inline void cmos_write(unsigned long reg, unsigned long val)
> +{
> + outb(reg, clock_ctl_addr);
> + outb(val, clock_data_addr);
> +}
> +
> +
> +static inline int cmos_clock_busy(void)
> +{
> + return
> + /* poll bit 7 (UIP) of Control Register A */
> + (cmos_read(10) & 0x80);
You could perhaps write it as (1 << 7) instead of 0x80 then, not that
it matters.
> +}
> +
> +
> +/* 9 RTC_RD_TIME -- Read RTC time */
> +kern_return_t
> +rtc_S_pioctl_rtc_rd_time (struct trivfs_protid *cred, struct rtc_time *tm)
> +{
> + if (!cred)
> + return EOPNOTSUPP;
> + if (!(cred->po->openmodes & O_READ))
> + return EBADF;
> +
> + unsigned char status = 0;
> + unsigned char pmbit = 0;
> +
> + /* When we wait for 100 ms (it takes too long), we exit with error */
> + int time_passed_in_milliseconds = 0;
> + bool read_rtc_successfully = false;
> + while (time_passed_in_milliseconds < 100) {
> + if (!cmos_clock_busy()) {
> + tm->tm_sec = cmos_read(0);
> + tm->tm_min = cmos_read(2);
> + tm->tm_hour = cmos_read(4);
> + tm->tm_wday = cmos_read(6);
> + tm->tm_mday = cmos_read(7);
> + tm->tm_mon = cmos_read(8);
> + tm->tm_year = cmos_read(9);
> + status = cmos_read(11);
> + /* Unless the clock changed while we were reading, consider this
> + a good clock read. */
> + if (tm->tm_sec == cmos_read(0)) {
> + read_rtc_successfully = true;
> + break;
> + }
> + }
GNU code style nitpicks again:
* all local variables should be declared before the actual statements
(like the cred checks),
* braces placement,
* single space between the function name and the opening paren.
(and same for all of the code below)
> + usleep (1000);
> + time_passed_in_milliseconds++;
> + }
> +
> + if (!read_rtc_successfully)
> + return EBUSY;
> +
> + if (!(status & 0x04)) { /* BCD mode - the default */
> + BCD_TO_BIN(tm->tm_sec);
> + BCD_TO_BIN(tm->tm_min);
> + pmbit = (tm->tm_hour & 0x80);
> + tm->tm_hour &= 0x7f;
> + BCD_TO_BIN(tm->tm_hour);
> + BCD_TO_BIN(tm->tm_wday);
> + BCD_TO_BIN(tm->tm_mday);
> + BCD_TO_BIN(tm->tm_mon);
> + BCD_TO_BIN(tm->tm_year);
> + }
> +
> + /* We don't use the century byte of the Hardware Clock since we
> + don't know its address (usually 50 or 55). Here, we follow the
> + advice of the X/Open Base Working Group: "if century is not
> + specified, then values in the range [69-99] refer to years in the
> + twentieth century (1969 to 1999 inclusive), and values in the
> + range [00-68] refer to years in the twenty-first century (2000 to
> + 2068 inclusive)." */
> + tm->tm_wday -= 1;
> + tm->tm_mon -= 1;
> + if (tm->tm_year < 69)
> + tm->tm_year += 100;
> + if (pmbit) {
> + tm->tm_hour += 12;
> + if (tm->tm_hour == 24)
> + tm->tm_hour = 0;
> + }
> +
> + /* don't know whether it's daylight */
> + tm->tm_isdst = -1;
> +
> + return KERN_SUCCESS;
> +}
> +
> +/* 10 RTC_SET_TIME -- Set RTC time */
> +kern_return_t
> +rtc_S_pioctl_rtc_set_time (struct trivfs_protid *cred, struct rtc_time tm)
> +{
> + if (!cred)
> + return EOPNOTSUPP;
> + if (!(cred->po->openmodes & O_WRITE))
> + return EBADF;
> +
> + unsigned char save_control, save_freq_select, pmbit = 0;
> +
> + /* CMOS byte 10 (clock status register A) has 3 bitfields:
> + bit 7: 1 if data invalid, update in progress (read-only bit)
> + (this is raised 224 us before the actual update starts)
> + 6-4 select base frequency
> + 010: 32768 Hz time base (default)
> + 111: reset
> + all other combinations are manufacturer-dependent
> + (e.g.: DS1287: 010 = start oscillator, anything else = stop)
> + 3-0 rate selection bits for interrupt
> + 0000 none (may stop RTC)
> + 0001, 0010 give same frequency as 1000, 1001
> + 0011 122 microseconds (minimum, 8192 Hz)
> + .... each increase by 1 halves the frequency, doubles the period
> + 1111 500 milliseconds (maximum, 2 Hz)
> + 0110 976.562 microseconds (default 1024 Hz) */
> +
> + save_control = cmos_read(11); /* tell the clock it's being set */
> + cmos_write(11, (save_control | 0x80));
> + save_freq_select = cmos_read(10); /* stop and reset prescaler */
> + cmos_write(10, (save_freq_select | 0x70));
> +
> + tm.tm_year %= 100;
> + tm.tm_mon += 1;
> + tm.tm_wday += 1;
> +
> + if (!(save_control & 0x02)) { /* 12hr mode; the default is 24hr
> mode */
> + if (tm.tm_hour == 0)
> + tm.tm_hour = 24;
> + if (tm.tm_hour > 12) {
> + tm.tm_hour -= 12;
> + pmbit = 0x80;
> + }
> + }
> +
> + if (!(save_control & 0x04)) { /* BCD mode - the default */
> + BIN_TO_BCD(tm.tm_sec);
> + BIN_TO_BCD(tm.tm_min);
> + BIN_TO_BCD(tm.tm_hour);
> + BIN_TO_BCD(tm.tm_wday);
> + BIN_TO_BCD(tm.tm_mday);
> + BIN_TO_BCD(tm.tm_mon);
> + BIN_TO_BCD(tm.tm_year);
> + }
> +
> + cmos_write(0, tm.tm_sec);
> + cmos_write(2, tm.tm_min);
> + cmos_write(4, tm.tm_hour | pmbit);
> + cmos_write(6, tm.tm_wday);
> + cmos_write(7, tm.tm_mday);
> + cmos_write(8, tm.tm_mon);
> + cmos_write(9, tm.tm_year);
> +
> + /* The Linux kernel sources, linux/arch/i386/kernel/time.c, have the
> + following comment:
> +
> + The following flags have to be released exactly in this order,
> + otherwise the DS12887 (popular MC146818A clone with integrated
> + battery and quartz) will not reset the oscillator and will not
> + update precisely 500 ms later. You won't find this mentioned in
> + the Dallas Semiconductor data sheets, but who believes data
> + sheets anyway ... -- Markus Kuhn */
> + cmos_write(11, save_control);
> + cmos_write(10, save_freq_select);
Can these writes somehow fail, and if so, can you detect it and report
the error? (I've no idea.)
> +
> + return KERN_SUCCESS;
> +}
Sergey
