| Message ID | 20150614074613.054681242@telegraphics.com.au (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hi Finn, On Sun, Jun 14, 2015 at 9:46 AM, Finn Thain <fthain@telegraphics.com.au> wrote: > --- linux.orig/arch/m68k/mac/misc.c 2015-06-14 17:46:02.000000000 +1000 > +++ linux/arch/m68k/mac/misc.c 2015-06-14 17:46:03.000000000 +1000 > @@ -284,11 +287,31 @@ static void via_pram_command(int command > > static unsigned char via_pram_read_byte(int offset) > { > - return 0; > + unsigned char temp; > + int addr = ((offset & 0xE0) << 3) | ((offset & 0x1F) << 2); Can you please add #defines for the magic values? > + > + /* Use RTC command 0x38 for XPRAM access, as per MESS source code */ > + via_pram_command(addr | 0x3800 | 0x8001, &temp); It seems 0x38 is already documented in <linux/pmu.h> (see below), or not (it's shifted left by 8 bits?)? > + > + return temp; > } > > static void via_pram_write_byte(unsigned char data, int offset) > { > + unsigned char temp; > + int addr = ((offset & 0xE0) << 3) | ((offset & 0x1F) << 2); > + > + /* Clear the write protect bit */ > + temp = 0x55; > + via_pram_command(0x34 | 0x01, &temp); > + > + /* Write the byte to XPRAM */ > + temp = data; > + via_pram_command(0x3800 | 0x0001 | addr, &temp); > + > + /* Set the write protect bit */ > + temp = 0xD5; > + via_pram_command(0x34 | 0x01, &temp); More magic values... > } > > /* > Index: linux/include/uapi/linux/pmu.h > =================================================================== > --- linux.orig/include/uapi/linux/pmu.h 2015-06-14 17:45:34.000000000 +1000 > +++ linux/include/uapi/linux/pmu.h 2015-06-14 17:46:03.000000000 +1000 > @@ -18,7 +18,9 @@ > #define PMU_POWER_CTRL 0x11 /* control power of some devices */ > #define PMU_ADB_CMD 0x20 /* send ADB packet */ > #define PMU_ADB_POLL_OFF 0x21 /* disable ADB auto-poll */ > +#define PMU_WRITE_XPRAM 0x32 /* write eXtended Parameter RAM */ > #define PMU_WRITE_NVRAM 0x33 /* write non-volatile RAM */ > +#define PMU_READ_XPRAM 0x3a /* read eXtended Parameter RAM */ > #define PMU_READ_NVRAM 0x3b /* read non-volatile RAM */ > #define PMU_SET_RTC 0x30 /* set real-time clock */ > #define PMU_READ_RTC 0x38 /* read real-time clock */ Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Mon, 15 Jun 2015, Geert Uytterhoeven wrote: > Hi Finn, > > On Sun, Jun 14, 2015 at 9:46 AM, Finn Thain <fthain@telegraphics.com.au> wrote: > > --- linux.orig/arch/m68k/mac/misc.c 2015-06-14 17:46:02.000000000 +1000 > > +++ linux/arch/m68k/mac/misc.c 2015-06-14 17:46:03.000000000 +1000 > > @@ -284,11 +287,31 @@ static void via_pram_command(int command > > > > static unsigned char via_pram_read_byte(int offset) > > { > > - return 0; > > + unsigned char temp; > > + int addr = ((offset & 0xE0) << 3) | ((offset & 0x1F) << 2); > > Can you please add #defines for the magic values? This is just marshalling the offset argument. I don't know how to rewrite that code more clearly. I'm open to suggestions. I don't know of any documentation that gives names or meanings to the different bit ranges. I infered the format from the MESS source code, https://github.com/mamedev/mame/blob/master/src/mess/machine/macrtc.c What I found in the MESS source code looks like the result of reverse engineering. Hence the RTC code in this patch also looks like a reverse engineered driver. > > > + > > + /* Use RTC command 0x38 for XPRAM access, as per MESS source code */ > > + via_pram_command(addr | 0x3800 | 0x8001, &temp); > > It seems 0x38 is already documented in <linux/pmu.h> (see below), or not > (it's shifted left by 8 bits?)? No, this is a RTC command not a PMU command. This RTC device is an Apple custom IC that is publicly undocumented. OTOH, the PMU device is well documented, since Apple publicly released PMU driver source code in MkLinux and later in XNU. That's why I've been able to provide #defines for the PMU commands but not the RTC commands. > > > + > > + return temp; > > } > > > > static void via_pram_write_byte(unsigned char data, int offset) > > { > > + unsigned char temp; > > + int addr = ((offset & 0xE0) << 3) | ((offset & 0x1F) << 2); > > + > > + /* Clear the write protect bit */ > > + temp = 0x55; > > + via_pram_command(0x34 | 0x01, &temp); > > + > > + /* Write the byte to XPRAM */ > > + temp = data; > > + via_pram_command(0x3800 | 0x0001 | addr, &temp); > > + > > + /* Set the write protect bit */ > > + temp = 0xD5; > > + via_pram_command(0x34 | 0x01, &temp); > > More magic values... When I have reliable documentation I always define macros. So I agree that "command" bytes like 0x34 and 0x3800 should have names but what are the correct names? Are we constructing an opcode containing RTC register file addresses or are we issuing read/write accesses to chip registers? In my experience with undocumented 68k Mac hardware and its Linux port, codified guesswork is worse than no documentation at all. The only useful RTC documentation I've ever come across is this: http://mac.linux-m68k.org/devel/plushw.php It tells us that the two least significant bits bits must equal 0b01. What would you call that macro? It also tells us that the most significant bit, 0x80, means "read access" but it only mentions early RTC chips and so it does not cover two byte opcodes. Should I have used 0x8080 here? Whatever your opinion of reverse engineered drivers, the changes in this patch are consistent with the rest of the file. E.g. via_read_time() and via_write_time(). If/when we have the chip data needed to correctly define macros for 0x01, 0x0001, 0x80, 0x8000 or 0x8080, I think they should be applied across the entire file, and in a different patch. Inconsistent use of such macros would be undesirable IMHO. > > > } > > > > /* > > Index: linux/include/uapi/linux/pmu.h > > =================================================================== > > --- linux.orig/include/uapi/linux/pmu.h 2015-06-14 17:45:34.000000000 +1000 > > +++ linux/include/uapi/linux/pmu.h 2015-06-14 17:46:03.000000000 +1000 > > @@ -18,7 +18,9 @@ > > #define PMU_POWER_CTRL 0x11 /* control power of some devices */ > > #define PMU_ADB_CMD 0x20 /* send ADB packet */ > > #define PMU_ADB_POLL_OFF 0x21 /* disable ADB auto-poll */ > > +#define PMU_WRITE_XPRAM 0x32 /* write eXtended Parameter RAM */ > > #define PMU_WRITE_NVRAM 0x33 /* write non-volatile RAM */ > > +#define PMU_READ_XPRAM 0x3a /* read eXtended Parameter RAM */ > > #define PMU_READ_NVRAM 0x3b /* read non-volatile RAM */ > > #define PMU_SET_RTC 0x30 /* set real-time clock */ > > #define PMU_READ_RTC 0x38 /* read real-time clock */ > > Gr{oetje,eeting}s, > > Geert >
Hi Geert, Further to my previous email, On Tue, 16 Jun 2015, in which I wrote: > > On Mon, 15 Jun 2015, Geert Uytterhoeven wrote: > > > > > More magic values... > > [...] The only useful RTC documentation I've ever come across is this: > http://mac.linux-m68k.org/devel/plushw.php This document appears to be Inside Macintosh vol. III ch. 2. It describes the early RTC chip that lacks two-byte operations and XPRAM, and pre-dates all Mac hardware supported in mainline Linux. But it does offer some useful data, though not enough to answer all of your criticisms (as I said). > [...] I think they should be applied across the entire file, and in a > different patch. Inconsistent use of such macros would be undesirable > IMHO. So, unless you have other ideas, I will revise this patch and insert an earlier patch to address existing code, and codify what little reliable chip data we have.
Hi Finn, On Thu, Jun 18, 2015 at 6:49 AM, Finn Thain <fthain@telegraphics.com.au> wrote: > On Tue, 16 Jun 2015, in which I wrote: >> On Mon, 15 Jun 2015, Geert Uytterhoeven wrote: >> > More magic values... >> >> [...] The only useful RTC documentation I've ever come across is this: >> http://mac.linux-m68k.org/devel/plushw.php > > This document appears to be Inside Macintosh vol. III ch. 2. It describes > the early RTC chip that lacks two-byte operations and XPRAM, and pre-dates > all Mac hardware supported in mainline Linux. But it does offer some > useful data, though not enough to answer all of your criticisms (as I > said). I understand. >> [...] I think they should be applied across the entire file, and in a >> different patch. Inconsistent use of such macros would be undesirable >> IMHO. > > So, unless you have other ideas, I will revise this patch and insert an > earlier patch to address existing code, and codify what little reliable > chip data we have. Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Finn Thain <fthain@telegraphics.com.au> writes: > When I have reliable documentation I always define macros. So I agree that > "command" bytes like 0x34 and 0x3800 should have names but what are the > correct names? Are we constructing an opcode containing RTC register file > addresses or are we issuing read/write accesses to chip registers? Any name will do as long as the magic constant is not duplicated. It is more important to document the relation between two uses than to have a correct name (which can trivially be updated later). Andreas.
Index: linux/arch/m68k/mac/misc.c =================================================================== --- linux.orig/arch/m68k/mac/misc.c 2015-06-14 17:46:02.000000000 +1000 +++ linux/arch/m68k/mac/misc.c 2015-06-14 17:46:03.000000000 +1000 @@ -119,19 +119,22 @@ static void pmu_write_time(long data) static unsigned char pmu_pram_read_byte(int offset) { struct adb_request req; - if (pmu_request(&req, NULL, 3, PMU_READ_NVRAM, - (offset >> 8) & 0xFF, offset & 0xFF) < 0) + + if (pmu_request(&req, NULL, 3, PMU_READ_XPRAM, + offset & 0xFF, 1) < 0) return 0; while (!req.complete) pmu_poll(); - return req.reply[3]; + + return req.reply[1]; } static void pmu_pram_write_byte(unsigned char data, int offset) { struct adb_request req; - if (pmu_request(&req, NULL, 4, PMU_WRITE_NVRAM, - (offset >> 8) & 0xFF, offset & 0xFF, data) < 0) + + if (pmu_request(&req, NULL, 4, PMU_WRITE_XPRAM, + offset & 0xFF, 1, data) < 0) return; while (!req.complete) pmu_poll(); @@ -284,11 +287,31 @@ static void via_pram_command(int command static unsigned char via_pram_read_byte(int offset) { - return 0; + unsigned char temp; + int addr = ((offset & 0xE0) << 3) | ((offset & 0x1F) << 2); + + /* Use RTC command 0x38 for XPRAM access, as per MESS source code */ + via_pram_command(addr | 0x3800 | 0x8001, &temp); + + return temp; } static void via_pram_write_byte(unsigned char data, int offset) { + unsigned char temp; + int addr = ((offset & 0xE0) << 3) | ((offset & 0x1F) << 2); + + /* Clear the write protect bit */ + temp = 0x55; + via_pram_command(0x34 | 0x01, &temp); + + /* Write the byte to XPRAM */ + temp = data; + via_pram_command(0x3800 | 0x0001 | addr, &temp); + + /* Set the write protect bit */ + temp = 0xD5; + via_pram_command(0x34 | 0x01, &temp); } /* Index: linux/include/uapi/linux/pmu.h =================================================================== --- linux.orig/include/uapi/linux/pmu.h 2015-06-14 17:45:34.000000000 +1000 +++ linux/include/uapi/linux/pmu.h 2015-06-14 17:46:03.000000000 +1000 @@ -18,7 +18,9 @@ #define PMU_POWER_CTRL 0x11 /* control power of some devices */ #define PMU_ADB_CMD 0x20 /* send ADB packet */ #define PMU_ADB_POLL_OFF 0x21 /* disable ADB auto-poll */ +#define PMU_WRITE_XPRAM 0x32 /* write eXtended Parameter RAM */ #define PMU_WRITE_NVRAM 0x33 /* write non-volatile RAM */ +#define PMU_READ_XPRAM 0x3a /* read eXtended Parameter RAM */ #define PMU_READ_NVRAM 0x3b /* read non-volatile RAM */ #define PMU_SET_RTC 0x30 /* set real-time clock */ #define PMU_READ_RTC 0x38 /* read real-time clock */
Signed-off-by: Finn Thain <fthain@telegraphics.com.au> --- Tested on a PowerBook 520 and Quadra 650. --- arch/m68k/mac/misc.c | 35 +++++++++++++++++++++++++++++------ include/uapi/linux/pmu.h | 2 ++ 2 files changed, 31 insertions(+), 6 deletions(-)