[RFC v2 23/24] m68k/mac: Fix PRAM accessors

[RFC v2 23/24] m68k/mac: Fix PRAM accessors

[Finn Thain]

[-- Attachment #1: mac68k-fix-pram-accessors --]
[-- Type: text/plain, Size: 2924 bytes --]

Signed-off-by: Finn Thain <fthain-PnMVE5gNl/W9dxLkCovUHLpzq4S04n8Q@public.gmane.org>

---

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(-)

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 */

Re: [RFC v2 23/24] m68k/mac: Fix PRAM accessors

[Geert Uytterhoeven]

Hi Finn,

On Sun, Jun 14, 2015 at 9:46 AM, Finn Thain <fthain-PnMVE5gNl/W9dxLkCovUHLpzq4S04n8Q@public.gmane.org> 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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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

Re: [RFC v2 23/24] m68k/mac: Fix PRAM accessors

[Finn Thain]

On Mon, 15 Jun 2015, Geert Uytterhoeven wrote:

> Hi Finn,
> 
> On Sun, Jun 14, 2015 at 9:46 AM, Finn Thain <fthain-PnMVE5gNl/W9dxLkCovUHLpzq4S04n8Q@public.gmane.org> 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
> 

-- 

Re: [RFC v2 23/24] m68k/mac: Fix PRAM accessors

[Finn Thain]

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.

-- 

Re: [RFC v2 23/24] m68k/mac: Fix PRAM accessors

[Geert Uytterhoeven]

Hi Finn,

On Thu, Jun 18, 2015 at 6:49 AM, Finn Thain <fthain-PnMVE5gNl/W9dxLkCovUHLpzq4S04n8Q@public.gmane.org> 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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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

Re: [RFC v2 23/24] m68k/mac: Fix PRAM accessors

[Andreas Schwab]

Finn Thain <fthain-PnMVE5gNl/W9dxLkCovUHLpzq4S04n8Q@public.gmane.org> 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.

-- 
Andreas Schwab, schwab-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."