Skip to contentSkip to conversation

macintosh: Resolve various PMU driver problems

PATCH Superseded Finn Thain 6 participants 27 messages Jun 8, 2018 – Jun 12, 2018

Subsystems (4)

v2 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Finn Thain

author
#0 lore raw

v2 cover macintosh: Resolve various PMU driver problems

This series of patches has the following aims.

1) Eliminate duplicated code. Linux presently has two drivers for the 68HC05-based PMU devices found in Macs: via-pmu and via-pmu68k. There's no value in having separate PMU drivers for each architecture.

2) Avoid further work on via-pmu68k that's not needed for via-pmu.

3) Fix some bugs in the via-pmu driver.

4) Enable the /dev/pmu and /proc/pmu/* userspace APIs on m68k Macs by adopting via-pmu.

5) Improve stability on early 100-series PowerBooks by loading no PMU driver at all. Neither via-pmu nor via-pmu68k supports the early M50753-based PMU device found in these models.

6) Eliminate duplicated RTC accessors for PMU and Cuda. Presently these can be found under both arch/m68k and arch/powerpc.

7) Assist the out-of-tree NuBus PowerMac port to support PMU designs shared with the m68k Mac port (e.g. PowerBooks 190 and 5300).

This patch series has been regression tested on various PowerBooks (190, 520, 3400, Pismo G3) and PowerMacs (Beige G3, G5). These patches did not affect userland utilities. (Note that there is a userland- visible change to the contents of /proc/pmu/interrupts.)

Changed since v1:
1) Added blank lines after 'break' statements in patch 10.
2) Improved patch description for patch 3.
3) Added reviewed-by tags.
4) Split patch 8 to make code review easier.

Finn Thain (12):
macintosh/via-pmu: Fix section mismatch warning macintosh/via-pmu: Add missing mmio accessors macintosh/via-pmu: Don't clear shift register interrupt flag twice macintosh/via-pmu: Enhance state machine with new 'uninitialized'
state
macintosh/via-pmu: Replace via pointer with via1 and via2 pointers macintosh/via-pmu: Add support for m68k PowerBooks macintosh/via-pmu: Make CONFIG_PPC_PMAC Kconfig deps explicit macintosh/via-pmu68k: Don't load driver on unsupported hardware macintosh/via-pmu: Replace via-pmu68k driver with via-pmu driver
macintosh: Use common code to access RTC macintosh/via-pmu: Clean up interrupt statistics macintosh/via-pmu: Disambiguate interrupt statistics

 arch/m68k/configs/mac_defconfig        |   2 +-
 arch/m68k/configs/multi_defconfig      |   2 +-
 arch/m68k/mac/config.c                 |   2 +-
 arch/m68k/mac/misc.c                   | 118 +----
 arch/powerpc/platforms/powermac/time.c |  74 +--
 drivers/macintosh/Kconfig              |  19 +-
 drivers/macintosh/Makefile             |   1 -
 drivers/macintosh/adb.c                |   2 +-
 drivers/macintosh/via-cuda.c           |  34 ++
 drivers/macintosh/via-pmu.c            | 378 ++++++++++-----
 drivers/macintosh/via-pmu68k.c         | 850 ---------------------------------
 include/linux/cuda.h                   |   3 +
 include/linux/pmu.h                    |   3 +
 include/uapi/linux/pmu.h               |   2 -
 14 files changed, 311 insertions(+), 1179 deletions(-)
 delete mode 100644 drivers/macintosh/via-pmu68k.c
Signature · 1 line
2.16.4
author
#1 lore raw

PATCH v2 1/12 macintosh/via-pmu: Fix section mismatch warning

The pmu_init() function has the __init qualifier, but the ops struct that holds a pointer to it does not. This causes a build warning. The driver works fine because the pointer is only dereferenced early.

The function is so small that there's negligible benefit from using the __init qualifier. Remove it to fix the warning, consistent with the other ADB drivers.

---
 drivers/macintosh/via-pmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/macintosh/via-pmu.c +1 -1
index 433dbeddfcf9..fd3c5640d586 100644
@@ -378,7 +378,7 @@ static int pmu_probe(void)
 	return vias == NULL? -ENODEV: 0;
 }
 
-static int __init pmu_init(void)
+static int pmu_init(void)
 {
 	if (vias == NULL)
 		return -ENODEV;
Signature · 1 line
2.16.4
author
#2 lore raw

PATCH v2 2/12 macintosh/via-pmu: Add missing mmio accessors

In reply to Finn Thain

Add missing in_8() accessors to init_pmu() and pmu_sr_intr().

This fixes several sparse warnings:
drivers/macintosh/via-pmu.c:536:29: warning: dereference of noderef expression drivers/macintosh/via-pmu.c:537:33: warning: dereference of noderef expression drivers/macintosh/via-pmu.c:1455:17: warning: dereference of noderef expression drivers/macintosh/via-pmu.c:1456:69: warning: dereference of noderef expression

---
 drivers/macintosh/via-pmu.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/macintosh/via-pmu.c +5 -4
index fd3c5640d586..74065ea410bd 100644
@@ -532,8 +532,9 @@ init_pmu(void)
 	int timeout;
 	struct adb_request req;
 
-	out_8(&via[B], via[B] | TREQ);			/* negate TREQ */
-	out_8(&via[DIRB], (via[DIRB] | TREQ) & ~TACK);	/* TACK in, TREQ out */
+	/* Negate TREQ. Set TACK to input and TREQ to output. */
+	out_8(&via[B], in_8(&via[B]) | TREQ);
+	out_8(&via[DIRB], (in_8(&via[DIRB]) | TREQ) & ~TACK);
 
 	pmu_request(&req, NULL, 2, PMU_SET_INTR_MASK, pmu_intr_mask);
 	timeout =  100000;
@@ -1455,8 +1456,8 @@ pmu_sr_intr(void)
 	struct adb_request *req;
 	int bite = 0;
 
-	if (via[B] & TREQ) {
-		printk(KERN_ERR "PMU: spurious SR intr (%x)\n", via[B]);
+	if (in_8(&via[B]) & TREQ) {
+		printk(KERN_ERR "PMU: spurious SR intr (%x)\n", in_8(&via[B]));
 		out_8(&via[IFR], SR_INT);
 		return NULL;
 	}
Signature · 1 line
2.16.4
author
#3 lore raw

PATCH v2 3/12 macintosh/via-pmu: Don't clear shift register interrupt flag twice

In reply to Finn Thain

The shift register interrupt flag gets cleared in via_pmu_interrupt() and once again in pmu_sr_intr(). Fix this theoretical race condition.

---
 drivers/macintosh/via-pmu.c | 1 -
 1 file changed, 1 deletion(-)
drivers/macintosh/via-pmu.c +0 -1
index 74065ea410bd..4c1bae5380c2 100644
@@ -1458,7 +1458,6 @@ pmu_sr_intr(void)
 
 	if (in_8(&via[B]) & TREQ) {
 		printk(KERN_ERR "PMU: spurious SR intr (%x)\n", in_8(&via[B]));
-		out_8(&via[IFR], SR_INT);
 		return NULL;
 	}
 	/* The ack may not yet be low when we get the interrupt */
Signature · 1 line
2.16.4
author
#4 lore raw

PATCH v2 4/12 macintosh/via-pmu: Enhance state machine with new 'uninitialized' state

In reply to Finn Thain

On 68k Macs, the via/vias pointer can't be used to determine whether the PMU driver has been initialized. For portability, add a new state to indicate that via_find_pmu() succeeded.

After via_find_pmu() executes, testing vias == NULL is equivalent to testing via == NULL. Replace these tests with pmu_state == uninitialized which is simpler and more consistent. No functional change.

---
 drivers/macintosh/via-pmu.c | 44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)
drivers/macintosh/via-pmu.c +22 -22
index 4c1bae5380c2..c4c324fb5fa6 100644
@@ -114,6 +114,7 @@ static volatile unsigned char __iomem *via;
 #define CB1_INT		0x10		/* transition on CB1 input */
 
 static volatile enum pmu_state {
+	uninitialized = 0,
 	idle,
 	sending,
 	intack,
@@ -274,7 +275,7 @@ int __init find_via_pmu(void)
 	u64 taddr;
 	const u32 *reg;
 
-	if (via != 0)
+	if (pmu_state != uninitialized)
 		return 1;
 	vias = of_find_node_by_name(NULL, "via-pmu");
 	if (vias == NULL)
@@ -369,20 +370,19 @@ int __init find_via_pmu(void)
  fail:
 	of_node_put(vias);
 	vias = NULL;
+	pmu_state = uninitialized;
 	return 0;
 }
 
 #ifdef CONFIG_ADB
 static int pmu_probe(void)
 {
-	return vias == NULL? -ENODEV: 0;
+	return pmu_state == uninitialized ? -ENODEV : 0;
 }
 
 static int pmu_init(void)
 {
-	if (vias == NULL)
-		return -ENODEV;
-	return 0;
+	return pmu_state == uninitialized ? -ENODEV : 0;
 }
 #endif /* CONFIG_ADB */
 
@@ -397,7 +397,7 @@ static int __init via_pmu_start(void)
 {
 	unsigned int irq;
 
-	if (vias == NULL)
+	if (pmu_state == uninitialized)
 		return -ENODEV;
 
 	batt_req.complete = 1;
@@ -463,7 +463,7 @@ arch_initcall(via_pmu_start);
  */
 static int __init via_pmu_dev_init(void)
 {
-	if (vias == NULL)
+	if (pmu_state == uninitialized)
 		return -ENODEV;
 
 #ifdef CONFIG_PMAC_BACKLIGHT
@@ -966,7 +966,7 @@ static int pmu_send_request(struct adb_request *req, int sync)
 {
 	int i, ret;
 
-	if ((vias == NULL) || (!pmu_fully_inited)) {
+	if (pmu_state == uninitialized || !pmu_fully_inited) {
 		req->complete = 1;
 		return -ENXIO;
 	}
@@ -1060,7 +1060,7 @@ static int __pmu_adb_autopoll(int devs)
 
 static int pmu_adb_autopoll(int devs)
 {
-	if ((vias == NULL) || (!pmu_fully_inited) || !pmu_has_adb)
+	if (pmu_state == uninitialized || !pmu_fully_inited || !pmu_has_adb)
 		return -ENXIO;
 
 	adb_dev_map = devs;
@@ -1073,7 +1073,7 @@ static int pmu_adb_reset_bus(void)
 	struct adb_request req;
 	int save_autopoll = adb_dev_map;
 
-	if ((vias == NULL) || (!pmu_fully_inited) || !pmu_has_adb)
+	if (pmu_state == uninitialized || !pmu_fully_inited || !pmu_has_adb)
 		return -ENXIO;
 
 	/* anyone got a better idea?? */
@@ -1109,7 +1109,7 @@ pmu_request(struct adb_request *req, void (*done)(struct adb_request *),
 	va_list list;
 	int i;
 
-	if (vias == NULL)
+	if (pmu_state == uninitialized)
 		return -ENXIO;
 
 	if (nbytes < 0 || nbytes > 32) {
@@ -1134,7 +1134,7 @@ pmu_queue_request(struct adb_request *req)
 	unsigned long flags;
 	int nsend;
 
-	if (via == NULL) {
+	if (pmu_state == uninitialized) {
 		req->complete = 1;
 		return -ENXIO;
 	}
@@ -1247,7 +1247,7 @@ pmu_start(void)
 void
 pmu_poll(void)
 {
-	if (!via)
+	if (pmu_state == uninitialized)
 		return;
 	if (disable_poll)
 		return;
@@ -1257,7 +1257,7 @@ pmu_poll(void)
 void
 pmu_poll_adb(void)
 {
-	if (!via)
+	if (pmu_state == uninitialized)
 		return;
 	if (disable_poll)
 		return;
@@ -1272,7 +1272,7 @@ pmu_poll_adb(void)
 void
 pmu_wait_complete(struct adb_request *req)
 {
-	if (!via)
+	if (pmu_state == uninitialized)
 		return;
 	while((pmu_state != idle && pmu_state != locked) || !req->complete)
 		via_pmu_interrupt(0, NULL);
@@ -1288,7 +1288,7 @@ pmu_suspend(void)
 {
 	unsigned long flags;
 
-	if (!via)
+	if (pmu_state == uninitialized)
 		return;
 	
 	spin_lock_irqsave(&pmu_lock, flags);
@@ -1319,7 +1319,7 @@ pmu_resume(void)
 {
 	unsigned long flags;
 
-	if (!via || (pmu_suspended < 1))
+	if (pmu_state == uninitialized || pmu_suspended < 1)
 		return;
 
 	spin_lock_irqsave(&pmu_lock, flags);
@@ -1681,7 +1681,7 @@ pmu_enable_irled(int on)
 {
 	struct adb_request req;
 
-	if (vias == NULL)
+	if (pmu_state == uninitialized)
 		return ;
 	if (pmu_kind == PMU_KEYLARGO_BASED)
 		return ;
@@ -1696,7 +1696,7 @@ pmu_restart(void)
 {
 	struct adb_request req;
 
-	if (via == NULL)
+	if (pmu_state == uninitialized)
 		return;
 
 	local_irq_disable();
@@ -1721,7 +1721,7 @@ pmu_shutdown(void)
 {
 	struct adb_request req;
 
-	if (via == NULL)
+	if (pmu_state == uninitialized)
 		return;
 
 	local_irq_disable();
@@ -1749,7 +1749,7 @@ pmu_shutdown(void)
 int
 pmu_present(void)
 {
-	return via != 0;
+	return pmu_state != uninitialized;
 }
 
 #if defined(CONFIG_SUSPEND) && defined(CONFIG_PPC32)
@@ -2415,7 +2415,7 @@ static struct miscdevice pmu_device = {
 
 static int pmu_device_init(void)
 {
-	if (!via)
+	if (pmu_state == uninitialized)
 		return 0;
 	if (misc_register(&pmu_device) < 0)
 		printk(KERN_ERR "via-pmu: cannot register misc device.\n");
Signature · 1 line
2.16.4
author
#5 lore raw

PATCH v2 5/12 macintosh/via-pmu: Replace via pointer with via1 and via2 pointers

In reply to Finn Thain

On most PowerPC Macs, the PMU driver uses the shift register and IO port B from a single VIA chip.

On 68k and early PowerPC PowerBooks, the driver uses the shift register from one VIA chip together with IO port B from another.

Replace via with via1 and via2 to accommodate this. For the CONFIG_PPC_PMAC case, set via1 = via2 so there is no change.

---
 drivers/macintosh/via-pmu.c | 142 +++++++++++++++++++++-----------------------
 1 file changed, 69 insertions(+), 73 deletions(-)
drivers/macintosh/via-pmu.c +69 -73
index c4c324fb5fa6..2e09137410f6 100644
@@ -76,7 +76,6 @@
 #define BATTERY_POLLING_COUNT	2
 
 static DEFINE_MUTEX(pmu_info_proc_mutex);
-static volatile unsigned char __iomem *via;
 
 /* VIA registers - spaced 0x200 bytes apart */
 #define RS		0x200		/* skip between registers */
@@ -145,6 +144,8 @@ static struct device_node *vias;
 static int pmu_kind = PMU_UNKNOWN;
 static int pmu_fully_inited;
 static int pmu_has_adb;
+static volatile unsigned char __iomem *via1;
+static volatile unsigned char __iomem *via2;
 static struct device_node *gpio_node;
 static unsigned char __iomem *gpio_reg;
 static int gpio_irq = 0;
@@ -340,14 +341,14 @@ int __init find_via_pmu(void)
 	} else
 		pmu_kind = PMU_UNKNOWN;
 
-	via = ioremap(taddr, 0x2000);
-	if (via == NULL) {
+	via1 = via2 = ioremap(taddr, 0x2000);
+	if (via1 == NULL) {
 		printk(KERN_ERR "via-pmu: Can't map address !\n");
 		goto fail_via_remap;
 	}
 	
-	out_8(&via[IER], IER_CLR | 0x7f);	/* disable all intrs */
-	out_8(&via[IFR], 0x7f);			/* clear IFR */
+	out_8(&via1[IER], IER_CLR | 0x7f);	/* disable all intrs */
+	out_8(&via1[IFR], 0x7f);			/* clear IFR */
 
 	pmu_state = idle;
 
@@ -362,8 +363,8 @@ int __init find_via_pmu(void)
 	return 1;
 
  fail_init:
-	iounmap(via);
-	via = NULL;
+	iounmap(via1);
+	via1 = via2 = NULL;
  fail_via_remap:
 	iounmap(gpio_reg);
 	gpio_reg = NULL;
@@ -437,7 +438,7 @@ static int __init via_pmu_start(void)
 	}
 
 	/* Enable interrupts */
-	out_8(&via[IER], IER_SET | SR_INT | CB1_INT);
+	out_8(&via1[IER], IER_SET | SR_INT | CB1_INT);
 
 	pmu_fully_inited = 1;
 
@@ -533,8 +534,8 @@ init_pmu(void)
 	struct adb_request req;
 
 	/* Negate TREQ. Set TACK to input and TREQ to output. */
-	out_8(&via[B], in_8(&via[B]) | TREQ);
-	out_8(&via[DIRB], (in_8(&via[DIRB]) | TREQ) & ~TACK);
+	out_8(&via2[B], in_8(&via2[B]) | TREQ);
+	out_8(&via2[DIRB], (in_8(&via2[DIRB]) | TREQ) & ~TACK);
 
 	pmu_request(&req, NULL, 2, PMU_SET_INTR_MASK, pmu_intr_mask);
 	timeout =  100000;
@@ -1174,7 +1175,7 @@ wait_for_ack(void)
 	 * reported
 	 */
 	int timeout = 4000;
-	while ((in_8(&via[B]) & TACK) == 0) {
+	while ((in_8(&via2[B]) & TACK) == 0) {
 		if (--timeout < 0) {
 			printk(KERN_ERR "PMU not responding (!ack)\n");
 			return;
@@ -1188,23 +1189,19 @@ wait_for_ack(void)
 static inline void
 send_byte(int x)
 {
-	volatile unsigned char __iomem *v = via;
-
-	out_8(&v[ACR], in_8(&v[ACR]) | SR_OUT | SR_EXT);
-	out_8(&v[SR], x);
-	out_8(&v[B], in_8(&v[B]) & ~TREQ);		/* assert TREQ */
-	(void)in_8(&v[B]);
+	out_8(&via1[ACR], in_8(&via1[ACR]) | SR_OUT | SR_EXT);
+	out_8(&via1[SR], x);
+	out_8(&via2[B], in_8(&via2[B]) & ~TREQ);	/* assert TREQ */
+	(void)in_8(&via2[B]);
 }
 
 static inline void
 recv_byte(void)
 {
-	volatile unsigned char __iomem *v = via;
-
-	out_8(&v[ACR], (in_8(&v[ACR]) & ~SR_OUT) | SR_EXT);
-	in_8(&v[SR]);		/* resets SR */
-	out_8(&v[B], in_8(&v[B]) & ~TREQ);
-	(void)in_8(&v[B]);
+	out_8(&via1[ACR], (in_8(&via1[ACR]) & ~SR_OUT) | SR_EXT);
+	in_8(&via1[SR]);		/* resets SR */
+	out_8(&via2[B], in_8(&via2[B]) & ~TREQ);
+	(void)in_8(&via2[B]);
 }
 
 static inline void
@@ -1307,7 +1304,7 @@ pmu_suspend(void)
 		if (!adb_int_pending && pmu_state == idle && !req_awaiting_reply) {
 			if (gpio_irq >= 0)
 				disable_irq_nosync(gpio_irq);
-			out_8(&via[IER], CB1_INT | IER_CLR);
+			out_8(&via1[IER], CB1_INT | IER_CLR);
 			spin_unlock_irqrestore(&pmu_lock, flags);
 			break;
 		}
@@ -1331,7 +1328,7 @@ pmu_resume(void)
 	adb_int_pending = 1;
 	if (gpio_irq >= 0)
 		enable_irq(gpio_irq);
-	out_8(&via[IER], CB1_INT | IER_SET);
+	out_8(&via1[IER], CB1_INT | IER_SET);
 	spin_unlock_irqrestore(&pmu_lock, flags);
 	pmu_poll();
 }
@@ -1456,20 +1453,20 @@ pmu_sr_intr(void)
 	struct adb_request *req;
 	int bite = 0;
 
-	if (in_8(&via[B]) & TREQ) {
-		printk(KERN_ERR "PMU: spurious SR intr (%x)\n", in_8(&via[B]));
+	if (in_8(&via2[B]) & TREQ) {
+		printk(KERN_ERR "PMU: spurious SR intr (%x)\n", in_8(&via2[B]));
 		return NULL;
 	}
 	/* The ack may not yet be low when we get the interrupt */
-	while ((in_8(&via[B]) & TACK) != 0)
+	while ((in_8(&via2[B]) & TACK) != 0)
 			;
 
 	/* if reading grab the byte, and reset the interrupt */
 	if (pmu_state == reading || pmu_state == reading_intr)
-		bite = in_8(&via[SR]);
+		bite = in_8(&via1[SR]);
 
 	/* reset TREQ and wait for TACK to go high */
-	out_8(&via[B], in_8(&via[B]) | TREQ);
+	out_8(&via2[B], in_8(&via2[B]) | TREQ);
 	wait_for_ack();
 
 	switch (pmu_state) {
@@ -1570,17 +1567,17 @@ via_pmu_interrupt(int irq, void *arg)
 	++disable_poll;
 	
 	for (;;) {
-		intr = in_8(&via[IFR]) & (SR_INT | CB1_INT);
+		intr = in_8(&via1[IFR]) & (SR_INT | CB1_INT);
 		if (intr == 0)
 			break;
 		handled = 1;
 		if (++nloop > 1000) {
 			printk(KERN_DEBUG "PMU: stuck in intr loop, "
 			       "intr=%x, ier=%x pmu_state=%d\n",
-			       intr, in_8(&via[IER]), pmu_state);
+			       intr, in_8(&via1[IER]), pmu_state);
 			break;
 		}
-		out_8(&via[IFR], intr);
+		out_8(&via1[IFR], intr);
 		if (intr & CB1_INT) {
 			adb_int_pending = 1;
 			pmu_irq_stats[0]++;
@@ -1762,29 +1759,29 @@ static u32 save_via[8];
 static void
 save_via_state(void)
 {
-	save_via[0] = in_8(&via[ANH]);
-	save_via[1] = in_8(&via[DIRA]);
-	save_via[2] = in_8(&via[B]);
-	save_via[3] = in_8(&via[DIRB]);
-	save_via[4] = in_8(&via[PCR]);
-	save_via[5] = in_8(&via[ACR]);
-	save_via[6] = in_8(&via[T1CL]);
-	save_via[7] = in_8(&via[T1CH]);
+	save_via[0] = in_8(&via1[ANH]);
+	save_via[1] = in_8(&via1[DIRA]);
+	save_via[2] = in_8(&via1[B]);
+	save_via[3] = in_8(&via1[DIRB]);
+	save_via[4] = in_8(&via1[PCR]);
+	save_via[5] = in_8(&via1[ACR]);
+	save_via[6] = in_8(&via1[T1CL]);
+	save_via[7] = in_8(&via1[T1CH]);
 }
 static void
 restore_via_state(void)
 {
-	out_8(&via[ANH], save_via[0]);
-	out_8(&via[DIRA], save_via[1]);
-	out_8(&via[B], save_via[2]);
-	out_8(&via[DIRB], save_via[3]);
-	out_8(&via[PCR], save_via[4]);
-	out_8(&via[ACR], save_via[5]);
-	out_8(&via[T1CL], save_via[6]);
-	out_8(&via[T1CH], save_via[7]);
-	out_8(&via[IER], IER_CLR | 0x7f);	/* disable all intrs */
-	out_8(&via[IFR], 0x7f);				/* clear IFR */
-	out_8(&via[IER], IER_SET | SR_INT | CB1_INT);
+	out_8(&via1[ANH],  save_via[0]);
+	out_8(&via1[DIRA], save_via[1]);
+	out_8(&via1[B],    save_via[2]);
+	out_8(&via1[DIRB], save_via[3]);
+	out_8(&via1[PCR],  save_via[4]);
+	out_8(&via1[ACR],  save_via[5]);
+	out_8(&via1[T1CL], save_via[6]);
+	out_8(&via1[T1CH], save_via[7]);
+	out_8(&via1[IER], IER_CLR | 0x7f);	/* disable all intrs */
+	out_8(&via1[IFR], 0x7f);			/* clear IFR */
+	out_8(&via1[IER], IER_SET | SR_INT | CB1_INT);
 }
 
 #define	GRACKLE_PM	(1<<7)
@@ -2426,33 +2423,33 @@ device_initcall(pmu_device_init);
 
 #ifdef DEBUG_SLEEP
 static inline void 
-polled_handshake(volatile unsigned char __iomem *via)
+polled_handshake(void)
 {
-	via[B] &= ~TREQ; eieio();
-	while ((via[B] & TACK) != 0)
+	via2[B] &= ~TREQ; eieio();
+	while ((via2[B] & TACK) != 0)
 		;
-	via[B] |= TREQ; eieio();
-	while ((via[B] & TACK) == 0)
+	via2[B] |= TREQ; eieio();
+	while ((via2[B] & TACK) == 0)
 		;
 }
 
 static inline void 
-polled_send_byte(volatile unsigned char __iomem *via, int x)
+polled_send_byte(int x)
 {
-	via[ACR] |= SR_OUT | SR_EXT; eieio();
-	via[SR] = x; eieio();
-	polled_handshake(via);
+	via1[ACR] |= SR_OUT | SR_EXT; eieio();
+	via1[SR] = x; eieio();
+	polled_handshake();
 }
 
 static inline int
-polled_recv_byte(volatile unsigned char __iomem *via)
+polled_recv_byte(void)
 {
 	int x;
 
-	via[ACR] = (via[ACR] & ~SR_OUT) | SR_EXT; eieio();
-	x = via[SR]; eieio();
-	polled_handshake(via);
-	x = via[SR]; eieio();
+	via1[ACR] = (via1[ACR] & ~SR_OUT) | SR_EXT; eieio();
+	x = via1[SR]; eieio();
+	polled_handshake();
+	x = via1[SR]; eieio();
 	return x;
 }
 
@@ -2461,7 +2458,6 @@ pmu_polled_request(struct adb_request *req)
 {
 	unsigned long flags;
 	int i, l, c;
-	volatile unsigned char __iomem *v = via;
 
 	req->complete = 1;
 	c = req->data[0];
@@ -2473,21 +2469,21 @@ pmu_polled_request(struct adb_request *req)
 	while (pmu_state != idle)
 		pmu_poll();
 
-	while ((via[B] & TACK) == 0)
+	while ((via2[B] & TACK) == 0)
 		;
-	polled_send_byte(v, c);
+	polled_send_byte(c);
 	if (l < 0) {
 		l = req->nbytes - 1;
-		polled_send_byte(v, l);
+		polled_send_byte(l);
 	}
 	for (i = 1; i <= l; ++i)
-		polled_send_byte(v, req->data[i]);
+		polled_send_byte(req->data[i]);
 
 	l = pmu_data_len[c][1];
 	if (l < 0)
-		l = polled_recv_byte(v);
+		l = polled_recv_byte();
 	for (i = 0; i < l; ++i)
-		req->reply[i + req->reply_len] = polled_recv_byte(v);
+		req->reply[i + req->reply_len] = polled_recv_byte();
 
 	if (req->done)
 		(*req->done)(req);
Signature · 1 line
2.16.4
author
#6 lore raw

PATCH v2 6/12 macintosh/via-pmu: Add support for m68k PowerBooks

In reply to Finn Thain

Put #ifdefs around the Open Firmware, xmon, interrupt dispatch, battery and suspend code. Add the necessary interrupt handling to support m68k PowerBooks.

The pmu_kind value is available to userspace using the PMU_IOC_GET_MODEL ioctl. It is not clear yet what hardware classes are be needed to describe m68k PowerBook models, so pmu_kind is given the provisional value PMU_UNKNOWN.

To find out about the hardware, user programs can use /proc/bootinfo or /proc/hardware, or send the PMU_GET_VERSION command using /dev/adb.

---
 drivers/macintosh/Kconfig   |   2 +-
 drivers/macintosh/via-pmu.c | 101 +++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 91 insertions(+), 12 deletions(-)
drivers/macintosh/Kconfig +1 -1
index 97a420c11eed..9c6452b38c36 100644
@@ -65,7 +65,7 @@ config ADB_CUDA
 	  If unsure say Y.
 
 config ADB_PMU
-	bool "Support for PMU  based PowerMacs"
+	bool "Support for PMU based PowerMacs and PowerBooks"
 	depends on PPC_PMAC
 	help
 	  On PowerBooks, iBooks, and recent iMacs and Power Macintoshes, the
drivers/macintosh/via-pmu.c +90 -11

246 lines not on this page · raw message

Signature · 1 line
2.16.4
author
#7 lore raw

PATCH v2 7/12 macintosh/via-pmu: Make CONFIG_PPC_PMAC Kconfig deps explicit

In reply to Finn Thain

At present, CONFIG_ADB_PMU depends on CONFIG_PPC_PMAC. When this gets relaxed to CONFIG_PPC_PMAC || CONFIG_MAC, those Kconfig symbols with implicit deps on PPC_PMAC will need explicit deps. Add them now. No functional change.

---
 drivers/macintosh/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/macintosh/Kconfig +2 -2
index 9c6452b38c36..26abae4c899d 100644
@@ -79,7 +79,7 @@ config ADB_PMU
 
 config ADB_PMU_LED
 	bool "Support for the Power/iBook front LED"
-	depends on ADB_PMU
+	depends on PPC_PMAC && ADB_PMU
 	select NEW_LEDS
 	select LEDS_CLASS
 	help
@@ -122,7 +122,7 @@ config PMAC_MEDIABAY
 
 config PMAC_BACKLIGHT
 	bool "Backlight control for LCD screens"
-	depends on ADB_PMU && FB = y && (BROKEN || !PPC64)
+	depends on PPC_PMAC && ADB_PMU && FB = y && (BROKEN || !PPC64)
 	select FB_BACKLIGHT
 	help
 	  Say Y here to enable Macintosh specific extensions of the generic
Signature · 1 line
2.16.4
author
#8 lore raw

PATCH v2 8/12 macintosh/via-pmu68k: Don't load driver on unsupported hardware

In reply to Finn Thain

Don't load the via-pmu68k driver on early PowerBooks. The M50753 PMU device found in those models was never supported by this driver. Attempting to load the driver usually causes a boot hang.

Cc Geert Uytterhoeven <[email protected]>
---
 arch/m68k/mac/misc.c           | 6 ++----
 drivers/macintosh/via-pmu68k.c | 4 ----
 include/uapi/linux/pmu.h       | 1 -
 3 files changed, 2 insertions(+), 9 deletions(-)
arch/m68k/mac/misc.c +2 -4
index c68054361615..7ccb799eeb57 100644
@@ -478,8 +478,7 @@ void mac_poweroff(void)
 		cuda_shutdown();
 #endif
 #ifdef CONFIG_ADB_PMU68K
-	} else if (macintosh_config->adb_type == MAC_ADB_PB1
-		|| macintosh_config->adb_type == MAC_ADB_PB2) {
+	} else if (macintosh_config->adb_type == MAC_ADB_PB2) {
 		pmu_shutdown();
 #endif
 	}
@@ -520,8 +519,7 @@ void mac_reset(void)
 		cuda_restart();
 #endif
 #ifdef CONFIG_ADB_PMU68K
-	} else if (macintosh_config->adb_type == MAC_ADB_PB1
-		|| macintosh_config->adb_type == MAC_ADB_PB2) {
+	} else if (macintosh_config->adb_type == MAC_ADB_PB2) {
 		pmu_restart();
 #endif
 	} else if (CPU_IS_030) {
drivers/macintosh/via-pmu68k.c +0 -4
index d545ed45e482..bec8e1837d7d 100644
@@ -175,9 +175,6 @@ static s8 pmu_data_len[256][2] = {
 int __init find_via_pmu(void)
 {
 	switch (macintosh_config->adb_type) {
-	case MAC_ADB_PB1:
-		pmu_kind = PMU_68K_V1;
-		break;
 	case MAC_ADB_PB2:
 		pmu_kind = PMU_68K_V2;
 		break;
@@ -785,7 +782,6 @@ pmu_enable_backlight(int on)
 	    /* first call: get current backlight value */
 	    if (backlight_level < 0) {
 		switch(pmu_kind) {
-		    case PMU_68K_V1:
 		    case PMU_68K_V2:
 			pmu_request(&req, NULL, 3, PMU_READ_NVRAM, 0x14, 0xe);
 			while (!req.complete)
include/uapi/linux/pmu.h +0 -1
index 89cb1acea93a..30f64d46f5db 100644
@@ -93,7 +93,6 @@ enum {
 	PMU_HEATHROW_BASED,	/* PowerBook G3 series */
 	PMU_PADDINGTON_BASED,	/* 1999 PowerBook G3 */
 	PMU_KEYLARGO_BASED,	/* Core99 motherboard (PMU99) */
-	PMU_68K_V1,		/* 68K PMU, version 1 */
 	PMU_68K_V2, 		/* 68K PMU, version 2 */
 };
 
Signature · 1 line
2.16.4
Reviewed-by on v2 8/12
#9 lore raw

Re: [PATCH v2 08/12] macintosh/via-pmu68k: Don't load driver on unsupported hardware

Hi Finn,

Am 08.06.2018 um 14:24 schrieb Finn Thain:

Quoted 11 lines

Don't load the via-pmu68k driver on early PowerBooks. The M50753 PMU device found in those models was never supported by this driver. Attempting to load the driver usually causes a boot hang.

Cc Geert Uytterhoeven <[email protected]>
---
 arch/m68k/mac/misc.c           | 6 ++----
 drivers/macintosh/via-pmu68k.c | 4 ----
 include/uapi/linux/pmu.h       | 1 -
 3 files changed, 2 insertions(+), 9 deletions(-)

...

include/uapi/linux/pmu.h enum { · line 97

Quoted 11 lines
include/uapi/linux/pmu.h +0 -1
index 89cb1acea93a..30f64d46f5db 100644
@@ -93,7 +93,6 @@ enum {
 	PMU_HEATHROW_BASED,	/* PowerBook G3 series */
 	PMU_PADDINGTON_BASED,	/* 1999 PowerBook G3 */
 	PMU_KEYLARGO_BASED,	/* Core99 motherboard (PMU99) */
-	PMU_68K_V1,		/* 68K PMU, version 1 */
 	PMU_68K_V2, 		/* 68K PMU, version 2 */
 };

Is this enum used by any user space code? If so, perhaps rather leave the PMU_68K_V1 in there to avoid upsetting that?

Otherwise,

Cheers,

Michael

#10 lore raw

Re: [PATCH v2 08/12] macintosh/via-pmu68k: Don't load driver on unsupported hardware

Quoted 15 lines · Michael Schmitz

Hi Finn,

Am 08.06.2018 um 14:24 schrieb Finn Thain:

Quoted 11 lines

Don't load the via-pmu68k driver on early PowerBooks. The M50753 PMU device found in those models was never supported by this driver. Attempting to load the driver usually causes a boot hang.

Cc Geert Uytterhoeven <[email protected]>
---
 arch/m68k/mac/misc.c           | 6 ++----
 drivers/macintosh/via-pmu68k.c | 4 ----
 include/uapi/linux/pmu.h       | 1 -
 3 files changed, 2 insertions(+), 9 deletions(-)

...

Quoted 14 lines

include/uapi/linux/pmu.h enum { · line 97

Quoted 11 lines
include/uapi/linux/pmu.h +0 -1
index 89cb1acea93a..30f64d46f5db 100644
@@ -93,7 +93,6 @@ enum {
 	PMU_HEATHROW_BASED,	/* PowerBook G3 series */
 	PMU_PADDINGTON_BASED,	/* 1999 PowerBook G3 */
 	PMU_KEYLARGO_BASED,	/* Core99 motherboard (PMU99) */
-	PMU_68K_V1,		/* 68K PMU, version 1 */
 	PMU_68K_V2, 		/* 68K PMU, version 2 */
 };

Is this enum used by any user space code? If so, perhaps rather leave the PMU_68K_V1 in there to avoid upsetting that?

It also changes the value of PMU_68K_V2, which is an ABI break.

Andreas.

Signature · 3 lines
Andreas Schwab, [email protected]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."
#11 lore raw

Re: [PATCH v2 08/12] macintosh/via-pmu68k: Don't load driver on unsupported hardware

Hi Andreas,

Am 09.06.2018 um 19:14 schrieb Andreas Schwab:

Quoted 32 lines · Michael Schmitz
Quoted 16 lines

Hi Finn,

Am 08.06.2018 um 14:24 schrieb Finn Thain:

Quoted 11 lines

Don't load the via-pmu68k driver on early PowerBooks. The M50753 PMU device found in those models was never supported by this driver. Attempting to load the driver usually causes a boot hang.

Cc Geert Uytterhoeven <[email protected]>
---
 arch/m68k/mac/misc.c           | 6 ++----
 drivers/macintosh/via-pmu68k.c | 4 ----
 include/uapi/linux/pmu.h       | 1 -
 3 files changed, 2 insertions(+), 9 deletions(-)

...

Quoted 14 lines

include/uapi/linux/pmu.h enum { · line 97

Quoted 11 lines
include/uapi/linux/pmu.h +0 -1
index 89cb1acea93a..30f64d46f5db 100644
@@ -93,7 +93,6 @@ enum {
 	PMU_HEATHROW_BASED,	/* PowerBook G3 series */
 	PMU_PADDINGTON_BASED,	/* 1999 PowerBook G3 */
 	PMU_KEYLARGO_BASED,	/* Core99 motherboard (PMU99) */
-	PMU_68K_V1,		/* 68K PMU, version 1 */
 	PMU_68K_V2, 		/* 68K PMU, version 2 */
 };

Is this enum used by any user space code? If so, perhaps rather leave the PMU_68K_V1 in there to avoid upsetting that?

Quoted 1 line

It also changes the value of PMU_68K_V2, which is an ABI break.

Yes, that's what I worry about - but do we know of any users of that particular interface?

Cheers,

Michael

Quoted 3 lines

Andreas.

author
#12 lore raw

Re: [PATCH v2 08/12] macintosh/via-pmu68k: Don't load driver on unsupported hardware

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

Is this enum used by any user space code? If so, perhaps rather leave the PMU_68K_V1 in there to avoid upsetting that?

It also changes the value of PMU_68K_V2, which is an ABI break.

Yes, that's what I worry about - but do we know of any users of that particular interface?

There is no ABI issue AFAIK. The value of pmu_kind is visible to userland only on powerpc. /dev/pmu and /proc/pmu/* do not exist on m68k. This patch series will make these UAPIs available on m68k, and for that reason I've chosen the value PMU_UNKNOWN for pmu_kind.

New pmu_kind values can be defined as and when the need arises. But that would imply a useful classification scheme for pre-PCI powerbooks, and I don't know what that scheme will look like because at this stage there is neither userland nor kernel code to support backlight, buttons and battery for pre-PCI powerbooks.

In anycase, the "v1" and "v2" scheme is obviously inadequate when you consider the range of m68k powerbook models. Also, consider the out-of-tree adaptation of via-pmu by the Nubus-PMac project, which has this ABI break:

include/linux/pmu.h +1 -0
index cafe98d9694..9882a185a52 100644
@@ -90,6 +90,7 @@ enum {
         PMU_HEATHROW_BASED,     /* PowerBook G3 series */
         PMU_PADDINGTON_BASED,   /* 1999 PowerBook G3 */
         PMU_KEYLARGO_BASED,     /* Core99 motherboard (PMU99) */
+        PMU_NUBUS_BASED,        /* 1400, 2300, 5300 */
         PMU_68K_V1,             /* 68K PMU, version 1 */
         PMU_68K_V2,             /* 68K PMU, version 2 */
 };

(BTW, these powerbooks are not "nubus based", they are "pre-PCI", so I wouldn't want this to go upstream in this form. It could be that PMU_NUBUS_BASED should be PMU_UNKNOWN too.)

#13 lore raw

Re: [PATCH v2 08/12] macintosh/via-pmu68k: Don't load driver on unsupported hardware

Quoted 2 lines · Finn Thain

There is no ABI issue AFAIK. The value of pmu_kind is visible to userland only on powerpc. /dev/pmu and /proc/pmu/* do not exist on m68k.

Then why are PMU_68K_V1 and PMU_68K_V2 defined in the first place?

Andreas.

Signature · 3 lines
Andreas Schwab, [email protected]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."
author
#14 lore raw

Re: [PATCH v2 08/12] macintosh/via-pmu68k: Don't load driver on unsupported hardware

Quoted 7 lines · Andreas Schwab
Quoted 2 lines · Finn Thain

There is no ABI issue AFAIK. The value of pmu_kind is visible to userland only on powerpc. /dev/pmu and /proc/pmu/* do not exist on m68k.

Then why are PMU_68K_V1 and PMU_68K_V2 defined in the first place?

I think your question is academic. Mistakes were made, and dead code appears in the kernel headers.

The author of via-pmu68k.c (apparently Joshua M. Thompson) may have a better answer, but if you want my guess, it is simply that pmu_kind was cut and pasted from via-pmu.c, and then given values corresponding to internal kernel system controller type values. See via-pmu68k.c:

        case MAC_ADB_PB1:
                pmu_kind = PMU_68K_V1;
                break;
        case MAC_ADB_PB2:
                pmu_kind = PMU_68K_V2;
                break;

Note that the MAC_ADB_PB1 and MAC_ADB_PB2 categories are quite artificial and were never useful to userland and were never visible to userland. The same applies to PMU_68K_V1 and PMU_68K_V2.

The MAC_ADB_PB1 and MAC_ADB_PB2 categories are used internally by the kernel to distinguish the M50753 devices which have no built-in RTC (MAC_ADB_PB1) from the M68HC05 devices which do have a built-in RTC (MAC_ADB_PB2). This distinction is completely hidden by the kernel behind the RTC UAPIs. It is not useful to userland.

With this patch series, MAC_ADB_PB1/PMU_68K_V1 models will have no PMU driver loaded at all, while MAC_ADB_PB2/PMU_68K_V2 models will have pmu_kind set to PMU_UNKNOWN, and this will become visible to userland for the first time.

Can please you explain why this change is problematic? You seem to be worried about breaking code which doesn't exist, and which, if it did exist, is already broken for other reasons.

Quoted 3 lines

Andreas.

#15 lore raw

Re: [PATCH v2 08/12] macintosh/via-pmu68k: Don't load driver on unsupported hardware

In reply to Finn Thain
Quoted 18 lines · Finn Thain

In anycase, the "v1" and "v2" scheme is obviously inadequate when you consider the range of m68k powerbook models. Also, consider the out-of-tree adaptation of via-pmu by the Nubus-PMac project, which has this ABI break:

include/linux/pmu.h +1 -0
index cafe98d9694..9882a185a52 100644
@@ -90,6 +90,7 @@ enum {
         PMU_HEATHROW_BASED,     /* PowerBook G3 series */
         PMU_PADDINGTON_BASED,   /* 1999 PowerBook G3 */
         PMU_KEYLARGO_BASED,     /* Core99 motherboard (PMU99) */
+        PMU_NUBUS_BASED,        /* 1400, 2300, 5300 */
         PMU_68K_V1,             /* 68K PMU, version 1 */
         PMU_68K_V2,             /* 68K PMU, version 2 */
 };
Quoted 3 lines

(BTW, these powerbooks are not "nubus based", they are "pre-PCI", so I wouldn't want this to go upstream in this form. It could be that PMU_NUBUS_BASED should be PMU_UNKNOWN too.)

Pre-PCI is basically "NUBUS" based even in absence of an actual NuBus slot :-) It has to do with the internal HW architecture. The only ones that aren't are the even older designs (the 68000 based ones).

What's the situation with those NuBus things ? What do they use as a bootloader ? The old Apple one or BootX ? We should merge that port of it's maintained.

Cheers,
Ben.

author
#16 lore raw

Re: [PATCH v2 08/12] macintosh/via-pmu68k: Don't load driver on unsupported hardware

Quoted 4 lines · Benjamin Herrenschmidt

Pre-PCI is basically "NUBUS" based even in absence of an actual NuBus slot :-) It has to do with the internal HW architecture. The only ones that aren't are the even older designs (the 68000 based ones).

There is already some disagreement in the comments in the nubus-pmac code about the suitability of "PMU_NUBUS_BASED" as opposed to e.g. "PMU_WHITNEY_BASED".

Point is, the PMU driver doesn't care about the expansion slots or architecture (Whitney-based PMU appears on m68k and powerpc). So NuBus vs. PCI is a red herring here. The pmu_kind relates to backlight, buttons and battery.

(Leaving aside the PMU driver, if a pre-OpenFirmware Mac has a "slot zero" ROM, one can argue that it is actually a NuBus machine, regardless of any actual expansion slots.)

Quoted 4 lines

What's the situation with those NuBus things ? What do they use as a bootloader ? The old Apple one or BootX ? We should merge that port of it's maintained.

I agree that this code should not languish out-of-tree. But it would need more work before it could reasonably be submitted to reviewers.

I do have some nubus-pmac hardware but I also have more mac/68k driver work to do before I can tackle another architecture.

I don't know what the bootloader situation is, but it looks messy...
http://nubus-pmac.sourceforge.net/#booters

Laurent, does Emile work on these machines?

Quoted 3 lines

Cheers,
Ben.

#17 lore raw

Re: [PATCH v2 08/12] macintosh/via-pmu68k: Don't load driver on unsupported hardware

Quoted 1 line · Finn Thain

On Sun, 10 Jun 2018, Benjamin Herrenschmidt wrote:

...

Quoted 5 lines

I don't know what the bootloader situation is, but it looks messy...
http://nubus-pmac.sourceforge.net/#booters

Laurent, does Emile work on these machines?

No, Emile doesn't work on pmac-nubus, I tried to implement the switch from m68k to ppc, but it has never worked.

Laurent

#18 lore raw

Re: [PATCH v2 08/12] macintosh/via-pmu68k: Don't load driver on unsupported hardware

Hi,

Quoted 11 lines · Laurent Vivier
Quoted 1 line · Finn Thain

On Sun, 10 Jun 2018, Benjamin Herrenschmidt wrote:

...

Quoted 5 lines

I don't know what the bootloader situation is, but it looks messy...
http://nubus-pmac.sourceforge.net/#booters

Laurent, does Emile work on these machines?

No, Emile doesn't work on pmac-nubus, I tried to implement the switch from m68k to ppc, but it has never worked.

On the PowerMac 6100 that I installed Linux on many years ago, I'm pretty sure I used Apple's mkLinux boot loader.

Not sure how that would work with modern kernels and initrds though. Haven't got that hardware anymore so no way try.

Cheers,

Michael

Quoted 2 lines

Laurent

#19 lore raw

Re: [PATCH v2 08/12] macintosh/via-pmu68k: Don't load driver on unsupported hardware

In reply to Finn Thain

Hi Finn,

Quoted 5 lines · Finn Thain
Quoted 5 lines
Quoted 4 lines
Quoted 2 lines

Is this enum used by any user space code? If so, perhaps rather leave the PMU_68K_V1 in there to avoid upsetting that?

It also changes the value of PMU_68K_V2, which is an ABI break.

Quoted 7 lines
Quoted 2 lines

Yes, that's what I worry about - but do we know of any users of that particular interface?

There is no ABI issue AFAIK. The value of pmu_kind is visible to userland only on powerpc. /dev/pmu and /proc/pmu/* do not exist on m68k. This patch series will make these UAPIs available on m68k, and for that reason I've chosen the value PMU_UNKNOWN for pmu_kind.

While /dev/pmu and /proc/pmu/* may not exist on m68k, definitions in include/uapi/linux/pmu.h are part of the ABI, and cannot be changed or removed, unless we are 100% sure there are no users.

If I would write a program interfacing with /dev/pmu and /proc/pmu/*, and needing to check the PMU type, it would have a switch() statement with all existing values defined in <linux/pmu.h>. So that would become broken by your change.

Hence the enum is append-only.

Quoted 8 lines

New pmu_kind values can be defined as and when the need arises. But that would imply a useful classification scheme for pre-PCI powerbooks, and I don't know what that scheme will look like because at this stage there is neither userland nor kernel code to support backlight, buttons and battery for pre-PCI powerbooks.

In anycase, the "v1" and "v2" scheme is obviously inadequate when you consider the range of m68k powerbook models. Also, consider the

New values can be added at the bottom.

Quoted 8 lines

out-of-tree adaptation of via-pmu by the Nubus-PMac project, which has this ABI break:

include/linux/pmu.h +0 -0
index cafe98d9694..9882a185a52 100644

include/linux/pmu.h enum { · line 96

Quoted 7 lines
include/linux/pmu.h +1 -0
@@ -90,6 +90,7 @@ enum {
         PMU_HEATHROW_BASED,     /* PowerBook G3 series */
         PMU_PADDINGTON_BASED,   /* 1999 PowerBook G3 */
         PMU_KEYLARGO_BASED,     /* Core99 motherboard (PMU99) */
+        PMU_NUBUS_BASED,        /* 1400, 2300, 5300 */
         PMU_68K_V1,             /* 68K PMU, version 1 */
         PMU_68K_V2,             /* 68K PMU, version 2 */
 };

That's bad. But as long as the NuBus-PMac project is out-of-tree, the enum values it uses are not part of the Linux ABI, IMHO. During upstreaming, PMU_NUBUS_BASED should be moved to the bottom.

Gr{oetje,eeting}s,

Geert

Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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
#20 lore raw

Re: [PATCH v2 08/12] macintosh/via-pmu68k: Don't load driver on unsupported hardware

Hi Geert,

Am 10.06.2018 um 20:29 schrieb Geert Uytterhoeven:

Quoted 20 lines

Hi Finn,

Quoted 5 lines · Finn Thain
Quoted 5 lines
Quoted 4 lines
Quoted 2 lines

Is this enum used by any user space code? If so, perhaps rather leave the PMU_68K_V1 in there to avoid upsetting that?

It also changes the value of PMU_68K_V2, which is an ABI break.

Quoted 7 lines
Quoted 2 lines

Yes, that's what I worry about - but do we know of any users of that particular interface?

There is no ABI issue AFAIK. The value of pmu_kind is visible to userland only on powerpc. /dev/pmu and /proc/pmu/* do not exist on m68k. This patch series will make these UAPIs available on m68k, and for that reason I've chosen the value PMU_UNKNOWN for pmu_kind.

While /dev/pmu and /proc/pmu/* may not exist on m68k, definitions in include/uapi/linux/pmu.h are part of the ABI, and cannot be changed or removed, unless we are 100% sure there are no users.

Quoted 6 lines

If I would write a program interfacing with /dev/pmu and /proc/pmu/*, and needing to check the PMU type, it would have a switch() statement with all existing values defined in <linux/pmu.h>. So that would become broken by your change.

Hence the enum is append-only.

The PMU type from pmu.h was never exposed to user space on m68k via /proc/pmu/*, and /dev/pmu is used for ioctls to the PMU driver on powerpc only (the 68k PMU driver doesn't have ioctl support). No way that I can see for user space to make use of the PMU type definition from pmu.h, so I suppose we can be sure there are no users.

The m68k PMU types cannot be said to be exposed on powerpc either (which has ioctl support to interrogate the PMU type), as these only return values up to PMU_KEYLARGO_BASED.

Applications like pbbuttonsd or pmud don't use the kernel PMU type at all, but go straight to the PMU via the ADB bus to interrogate the hardware type, so won't be affected either.

Is there any other way besides procfs and ioctl for user space to interrogate the PMU type that I'm missing here?

(I understand that breaking the ABI should not be done as a rule, but this may be a case where we can successfully argue the definitions were never in use, so the rules may be bent a little).

Cheers,

Michael

Quoted 35 lines
Quoted 8 lines

New pmu_kind values can be defined as and when the need arises. But that would imply a useful classification scheme for pre-PCI powerbooks, and I don't know what that scheme will look like because at this stage there is neither userland nor kernel code to support backlight, buttons and battery for pre-PCI powerbooks.

In anycase, the "v1" and "v2" scheme is obviously inadequate when you consider the range of m68k powerbook models. Also, consider the

New values can be added at the bottom.

Quoted 8 lines

out-of-tree adaptation of via-pmu by the Nubus-PMac project, which has this ABI break:

include/linux/pmu.h +0 -0
index cafe98d9694..9882a185a52 100644

include/linux/pmu.h enum { · line 96

Quoted 7 lines
include/linux/pmu.h +1 -0
@@ -90,6 +90,7 @@ enum {
         PMU_HEATHROW_BASED,     /* PowerBook G3 series */
         PMU_PADDINGTON_BASED,   /* 1999 PowerBook G3 */
         PMU_KEYLARGO_BASED,     /* Core99 motherboard (PMU99) */
+        PMU_NUBUS_BASED,        /* 1400, 2300, 5300 */
         PMU_68K_V1,             /* 68K PMU, version 1 */
         PMU_68K_V2,             /* 68K PMU, version 2 */
 };

That's bad. But as long as the NuBus-PMac project is out-of-tree, the enum values it uses are not part of the Linux ABI, IMHO. During upstreaming, PMU_NUBUS_BASED should be moved to the bottom.

Gr{oetje,eeting}s,

Geert

#21 lore raw

Re: [PATCH v2 08/12] macintosh/via-pmu68k: Don't load driver on unsupported hardware

Quoted 1 line · Michael Schmitz

Hi Geert,

Top posting, sorry ...

We are painting that bike shed with way too many coats..

We can keep the existing definitions, stick a comment on them stating "obsolete" and use new number if/when needed.

Ben.

Quoted 48 lines

Am 10.06.2018 um 20:29 schrieb Geert Uytterhoeven:

Quoted 20 lines

Hi Finn,

Quoted 5 lines · Finn Thain
Quoted 5 lines
Quoted 4 lines
Quoted 2 lines

Is this enum used by any user space code? If so, perhaps rather leave the PMU_68K_V1 in there to avoid upsetting that?

It also changes the value of PMU_68K_V2, which is an ABI break.

Quoted 7 lines
Quoted 2 lines

Yes, that's what I worry about - but do we know of any users of that particular interface?

There is no ABI issue AFAIK. The value of pmu_kind is visible to userland only on powerpc. /dev/pmu and /proc/pmu/* do not exist on m68k. This patch series will make these UAPIs available on m68k, and for that reason I've chosen the value PMU_UNKNOWN for pmu_kind.

While /dev/pmu and /proc/pmu/* may not exist on m68k, definitions in include/uapi/linux/pmu.h are part of the ABI, and cannot be changed or removed, unless we are 100% sure there are no users.

Quoted 6 lines

If I would write a program interfacing with /dev/pmu and /proc/pmu/*, and needing to check the PMU type, it would have a switch() statement with all existing values defined in <linux/pmu.h>. So that would become broken by your change.

Hence the enum is append-only.

The PMU type from pmu.h was never exposed to user space on m68k via /proc/pmu/*, and /dev/pmu is used for ioctls to the PMU driver on powerpc only (the 68k PMU driver doesn't have ioctl support). No way that I can see for user space to make use of the PMU type definition from pmu.h, so I suppose we can be sure there are no users.

The m68k PMU types cannot be said to be exposed on powerpc either (which has ioctl support to interrogate the PMU type), as these only return values up to PMU_KEYLARGO_BASED.

Applications like pbbuttonsd or pmud don't use the kernel PMU type at all, but go straight to the PMU via the ADB bus to interrogate the hardware type, so won't be affected either.

Is there any other way besides procfs and ioctl for user space to interrogate the PMU type that I'm missing here?

(I understand that breaking the ABI should not be done as a rule, but this may be a case where we can successfully argue the definitions were never in use, so the rules may be bent a little).

#22 lore raw

Re: [PATCH v2 08/12] macintosh/via-pmu68k: Don't load driver on unsupported hardware

Hi Ben,

I'm glad Finn is caring enough to keep this 20 year old bike shed in good repair, but this may be overdoing it a little indeed. My bad.

A comment on the V1 PMU entry everyone should be OK with, I hope.

Cheers,

Michael

Quoted 61 lines · Benjamin Herrenschmidt
Quoted 1 line · Michael Schmitz

Hi Geert,

Top posting, sorry ...

We are painting that bike shed with way too many coats..

We can keep the existing definitions, stick a comment on them stating "obsolete" and use new number if/when needed.

Ben.

Quoted 48 lines

Am 10.06.2018 um 20:29 schrieb Geert Uytterhoeven:

Quoted 20 lines

Hi Finn,

Quoted 5 lines · Finn Thain
Quoted 5 lines
Quoted 4 lines
Quoted 2 lines

Is this enum used by any user space code? If so, perhaps rather leave the PMU_68K_V1 in there to avoid upsetting that?

It also changes the value of PMU_68K_V2, which is an ABI break.

Quoted 7 lines
Quoted 2 lines

Yes, that's what I worry about - but do we know of any users of that particular interface?

There is no ABI issue AFAIK. The value of pmu_kind is visible to userland only on powerpc. /dev/pmu and /proc/pmu/* do not exist on m68k. This patch series will make these UAPIs available on m68k, and for that reason I've chosen the value PMU_UNKNOWN for pmu_kind.

While /dev/pmu and /proc/pmu/* may not exist on m68k, definitions in include/uapi/linux/pmu.h are part of the ABI, and cannot be changed or removed, unless we are 100% sure there are no users.

Quoted 6 lines

If I would write a program interfacing with /dev/pmu and /proc/pmu/*, and needing to check the PMU type, it would have a switch() statement with all existing values defined in <linux/pmu.h>. So that would become broken by your change.

Hence the enum is append-only.

The PMU type from pmu.h was never exposed to user space on m68k via /proc/pmu/*, and /dev/pmu is used for ioctls to the PMU driver on powerpc only (the 68k PMU driver doesn't have ioctl support). No way that I can see for user space to make use of the PMU type definition from pmu.h, so I suppose we can be sure there are no users.

The m68k PMU types cannot be said to be exposed on powerpc either (which has ioctl support to interrogate the PMU type), as these only return values up to PMU_KEYLARGO_BASED.

Applications like pbbuttonsd or pmud don't use the kernel PMU type at all, but go straight to the PMU via the ADB bus to interrogate the hardware type, so won't be affected either.

Is there any other way besides procfs and ioctl for user space to interrogate the PMU type that I'm missing here?

(I understand that breaking the ABI should not be done as a rule, but this may be a case where we can successfully argue the definitions were never in use, so the rules may be bent a little).

author
#23 lore raw

PATCH v2 9/12 macintosh/via-pmu: Replace via-pmu68k driver with via-pmu driver

In reply to Finn Thain

Now that the PowerMac via-pmu driver supports m68k PowerBooks, switch over to that driver and remove the via-pmu68k driver.

---
 arch/m68k/configs/mac_defconfig   |   2 +-
 arch/m68k/configs/multi_defconfig |   2 +-
 arch/m68k/mac/config.c            |   2 +-
 arch/m68k/mac/misc.c              |  48 +--
 drivers/macintosh/Kconfig         |  13 +-
 drivers/macintosh/Makefile        |   1 -
 drivers/macintosh/adb.c           |   2 +-
 drivers/macintosh/via-pmu68k.c    | 846 --------------------------------------
 include/uapi/linux/pmu.h          |   1 -
 9 files changed, 13 insertions(+), 904 deletions(-)
 delete mode 100644 drivers/macintosh/via-pmu68k.c
arch/m68k/configs/mac_defconfig +1 -1
index 390d4a87441c..ee63f1242e9a 100644
@@ -370,7 +370,7 @@ CONFIG_TCM_PSCSI=m
 CONFIG_ADB=y
 CONFIG_ADB_MACII=y
 CONFIG_ADB_IOP=y
-CONFIG_ADB_PMU68K=y
+CONFIG_ADB_PMU=y
 CONFIG_ADB_CUDA=y
 CONFIG_INPUT_ADBHID=y
 CONFIG_MAC_EMUMOUSEBTN=y
arch/m68k/configs/multi_defconfig +1 -1
index 77be97d82dc3..6421a3da616c 100644
@@ -403,7 +403,7 @@ CONFIG_TCM_PSCSI=m
 CONFIG_ADB=y
 CONFIG_ADB_MACII=y
 CONFIG_ADB_IOP=y
-CONFIG_ADB_PMU68K=y
+CONFIG_ADB_PMU=y
 CONFIG_ADB_CUDA=y
 CONFIG_INPUT_ADBHID=y
 CONFIG_MAC_EMUMOUSEBTN=y
arch/m68k/mac/config.c +1 -1
index e522307db47c..92e80cf0d8aa 100644
@@ -891,7 +891,7 @@ static void __init mac_identify(void)
 #ifdef CONFIG_ADB_CUDA
 	find_via_cuda();
 #endif
-#ifdef CONFIG_ADB_PMU68K
+#ifdef CONFIG_ADB_PMU
 	find_via_pmu();
 #endif
 }
arch/m68k/mac/misc.c +8 -40

102 lines not on this page · raw message

drivers/macintosh/Kconfig +1 -12
index 26abae4c899d..47c350cdfb12 100644
@@ -39,17 +39,6 @@ config ADB_IOP
 	  <http://www.angelfire.com/ca2/dev68k/iopdesc.html> to enable direct
 	  support for it, say 'Y' here.
 
-config ADB_PMU68K
-	bool "Include PMU (Powerbook) ADB driver"
-	depends on ADB && MAC
-	help
-	  Say Y here if want your kernel to support the m68k based Powerbooks.
-	  This includes the PowerBook 140, PowerBook 145, PowerBook 150,
-	  PowerBook 160, PowerBook 165, PowerBook 165c, PowerBook 170,
-	  PowerBook 180, PowerBook, 180c, PowerBook 190cs, PowerBook 520,
-	  PowerBook Duo 210, PowerBook Duo 230, PowerBook Duo 250,
-	  PowerBook Duo 270c, PowerBook Duo 280 and PowerBook Duo 280c.
-
 # we want to change this to something like CONFIG_SYSCTRL_CUDA/PMU
 config ADB_CUDA
 	bool "Support for Cuda/Egret based Macs and PowerMacs"
@@ -66,7 +55,7 @@ config ADB_CUDA
 
 config ADB_PMU
 	bool "Support for PMU based PowerMacs and PowerBooks"
-	depends on PPC_PMAC
+	depends on PPC_PMAC || MAC
 	help
 	  On PowerBooks, iBooks, and recent iMacs and Power Macintoshes, the
 	  PMU is an embedded microprocessor whose primary function is to
drivers/macintosh/Makefile +0 -1
index ee803638e595..49819b1b6f20 100644
@@ -22,7 +22,6 @@ obj-$(CONFIG_PMAC_SMU)		+= smu.o
 obj-$(CONFIG_ADB)		+= adb.o
 obj-$(CONFIG_ADB_MACII)		+= via-macii.o
 obj-$(CONFIG_ADB_IOP)		+= adb-iop.o
-obj-$(CONFIG_ADB_PMU68K)	+= via-pmu68k.o
 obj-$(CONFIG_ADB_MACIO)		+= macio-adb.o
 
 obj-$(CONFIG_THERM_WINDTUNNEL)	+= therm_windtunnel.o
drivers/macintosh/adb.c +1 -1
index 4c8097e0e6fe..76e98f0f7a3e 100644
@@ -65,7 +65,7 @@ static struct adb_driver *adb_driver_list[] = {
 #ifdef CONFIG_ADB_IOP
 	&adb_iop_driver,
 #endif
-#if defined(CONFIG_ADB_PMU) || defined(CONFIG_ADB_PMU68K)
+#ifdef CONFIG_ADB_PMU
 	&via_pmu_driver,
 #endif
 #ifdef CONFIG_ADB_MACIO
drivers/macintosh/via-pmu68k.c deleted +0 -846

846 lines not on this page · raw message

include/uapi/linux/pmu.h +0 -1
index 30f64d46f5db..a2d3c289ed81 100644
@@ -93,7 +93,6 @@ enum {
 	PMU_HEATHROW_BASED,	/* PowerBook G3 series */
 	PMU_PADDINGTON_BASED,	/* 1999 PowerBook G3 */
 	PMU_KEYLARGO_BASED,	/* Core99 motherboard (PMU99) */
-	PMU_68K_V2, 		/* 68K PMU, version 2 */
 };
 
 /* PMU PMU_POWER_EVENTS commands */
Signature · 1 line
2.16.4
author
#24 lore raw

PATCH v2 10/12 macintosh: Use common code to access RTC

In reply to Finn Thain

Now that the 68k Mac port has adopted the via-pmu driver, it must access the PMU RTC using the appropriate command format. The same code can now be used for both m68k and powerpc.

Replace the RTC code that's duplicated in arch/powerpc and arch/m68k with common RTC accessors for Cuda and PMU devices.

Cc Geert Uytterhoeven <[email protected]>
Cc Paul Mackerras <[email protected]>,
Cc Michael Ellerman <[email protected]>
---
 arch/m68k/mac/misc.c                   | 64 ++---------------------------
 arch/powerpc/platforms/powermac/time.c | 74 +---------------------------------
 drivers/macintosh/via-cuda.c           | 34 ++++++++++++++++
 drivers/macintosh/via-pmu.c            | 32 +++++++++++++++
 include/linux/cuda.h                   |  3 ++
 include/linux/pmu.h                    |  3 ++
 6 files changed, 78 insertions(+), 132 deletions(-)
arch/m68k/mac/misc.c +4 -60
index 28090a44fa09..397f9f942a9f 100644
@@ -33,34 +33,6 @@
 static void (*rom_reset)(void);
 
 #ifdef CONFIG_ADB_CUDA
-static long cuda_read_time(void)
-{
-	struct adb_request req;
-	long time;
-
-	if (cuda_request(&req, NULL, 2, CUDA_PACKET, CUDA_GET_TIME) < 0)
-		return 0;
-	while (!req.complete)
-		cuda_poll();
-
-	time = (req.reply[3] << 24) | (req.reply[4] << 16) |
-	       (req.reply[5] << 8) | req.reply[6];
-	return time - RTC_OFFSET;
-}
-
-static void cuda_write_time(long data)
-{
-	struct adb_request req;
-
-	data += RTC_OFFSET;
-	if (cuda_request(&req, NULL, 6, CUDA_PACKET, CUDA_SET_TIME,
-			 (data >> 24) & 0xFF, (data >> 16) & 0xFF,
-			 (data >> 8) & 0xFF, data & 0xFF) < 0)
-		return;
-	while (!req.complete)
-		cuda_poll();
-}
-
 static __u8 cuda_read_pram(int offset)
 {
 	struct adb_request req;
@@ -86,34 +58,6 @@ static void cuda_write_pram(int offset, __u8 data)
 #endif /* CONFIG_ADB_CUDA */
 
 #ifdef CONFIG_ADB_PMU
-static long pmu_read_time(void)
-{
-	struct adb_request req;
-	long time;
-
-	if (pmu_request(&req, NULL, 1, PMU_READ_RTC) < 0)
-		return 0;
-	while (!req.complete)
-		pmu_poll();
-
-	time = (req.reply[1] << 24) | (req.reply[2] << 16) |
-	       (req.reply[3] << 8) | req.reply[4];
-	return time - RTC_OFFSET;
-}
-
-static void pmu_write_time(long data)
-{
-	struct adb_request req;
-
-	data += RTC_OFFSET;
-	if (pmu_request(&req, NULL, 5, PMU_SET_RTC,
-			(data >> 24) & 0xFF, (data >> 16) & 0xFF,
-			(data >> 8) & 0xFF, data & 0xFF) < 0)
-		return;
-	while (!req.complete)
-		pmu_poll();
-}
-
 static __u8 pmu_read_pram(int offset)
 {
 	struct adb_request req;
@@ -635,12 +579,12 @@ int mac_hwclk(int op, struct rtc_time *t)
 #ifdef CONFIG_ADB_CUDA
 		case MAC_ADB_EGRET:
 		case MAC_ADB_CUDA:
-			now = cuda_read_time();
+			now = cuda_get_time();
 			break;
 #endif
 #ifdef CONFIG_ADB_PMU
 		case MAC_ADB_PB2:
-			now = pmu_read_time();
+			now = pmu_get_time();
 			break;
 #endif
 		default:
@@ -671,12 +615,12 @@ int mac_hwclk(int op, struct rtc_time *t)
 #ifdef CONFIG_ADB_CUDA
 		case MAC_ADB_EGRET:
 		case MAC_ADB_CUDA:
-			cuda_write_time(now);
+			cuda_set_time(now);
 			break;
 #endif
 #ifdef CONFIG_ADB_PMU
 		case MAC_ADB_PB2:
-			pmu_write_time(now);
+			pmu_set_time(now);
 			break;
 #endif
 		default:
arch/powerpc/platforms/powermac/time.c +2 -72

94 lines not on this page · raw message

drivers/macintosh/via-cuda.c +34 -0
index 019556136e77..8e7fb2115f10 100644
@@ -771,3 +771,37 @@ cuda_input(unsigned char *buf, int nb)
 	               buf, nb, false);
     }
 }
+
+/* Offset between Unix time (1970-based) and Mac time (1904-based) */
+#define RTC_OFFSET	2082844800
+
+unsigned long cuda_get_time(void)
+{
+	struct adb_request req;
+	unsigned long now;
+
+	if (cuda_request(&req, NULL, 2, CUDA_PACKET, CUDA_GET_TIME) < 0)
+		return 0;
+	while (!req.complete)
+		cuda_poll();
+	if (req.reply_len != 7)
+		pr_err("%s: got %d byte reply\n", __func__, req.reply_len);
+	now = (req.reply[3] << 24) + (req.reply[4] << 16) +
+	      (req.reply[5] << 8) + req.reply[6];
+	return now - RTC_OFFSET;
+}
+
+int cuda_set_time(unsigned long now)
+{
+	struct adb_request req;
+
+	now += RTC_OFFSET;
+	if (cuda_request(&req, NULL, 6, CUDA_PACKET, CUDA_SET_TIME,
+			now >> 24, now >> 16, now >> 8, now) < 0)
+		return -ENXIO;
+	while (!req.complete)
+		cuda_poll();
+	if ((req.reply_len != 3) && (req.reply_len != 7))
+		pr_err("%s: got %d byte reply\n", __func__, req.reply_len);
+	return 0;
+}
drivers/macintosh/via-pmu.c +32 -0
index 22cb7d94e3ce..38d7dd0bdb28 100644
@@ -1820,6 +1820,38 @@ pmu_shutdown(void)
 		;
 }
 
+/* Offset between Unix time (1970-based) and Mac time (1904-based) */
+#define RTC_OFFSET	2082844800
+
+unsigned long pmu_get_time(void)
+{
+	struct adb_request req;
+	unsigned long now;
+
+	if (pmu_request(&req, NULL, 1, PMU_READ_RTC) < 0)
+		return 0;
+	pmu_wait_complete(&req);
+	if (req.reply_len != 4)
+		pr_err("%s: got %d byte reply\n", __func__, req.reply_len);
+	now = (req.reply[0] << 24) + (req.reply[1] << 16) +
+	      (req.reply[2] << 8) + req.reply[3];
+	return now - RTC_OFFSET;
+}
+
+int pmu_set_time(unsigned long now)
+{
+	struct adb_request req;
+
+	now += RTC_OFFSET;
+	if (pmu_request(&req, NULL, 5, PMU_SET_RTC,
+			now >> 24, now >> 16, now >> 8, now) < 0)
+		return -ENXIO;
+	pmu_wait_complete(&req);
+	if (req.reply_len != 0)
+		pr_err("%s: got %d byte reply\n", __func__, req.reply_len);
+	return 0;
+}
+
 int
 pmu_present(void)
 {
include/linux/cuda.h +3 -0
index 056867f09a01..a68669f746e1 100644
@@ -16,4 +16,7 @@ extern int cuda_request(struct adb_request *req,
 			void (*done)(struct adb_request *), int nbytes, ...);
 extern void cuda_poll(void);
 
+extern unsigned long cuda_get_time(void);
+extern int cuda_set_time(unsigned long now);
+
 #endif /* _LINUX_CUDA_H */
include/linux/pmu.h +3 -0
index 9ac8fc60ad49..feefd0bff9cf 100644
@@ -34,6 +34,9 @@ static inline void pmu_resume(void)
 {}
 #endif
 
+extern unsigned long pmu_get_time(void);
+extern int pmu_set_time(unsigned long now);
+
 extern void pmu_enable_irled(int on);
 
 extern void pmu_restart(void);
Signature · 1 line
2.16.4
author
#25 lore raw

PATCH v2 11/12 macintosh/via-pmu: Clean up interrupt statistics

In reply to Finn Thain

Replace an open-coded ffs() with the function call. Simplify an if-else cascade using a switch statement. Correct a typo and an indentation issue.

---
 drivers/macintosh/via-pmu.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)
drivers/macintosh/via-pmu.c +22 -17
index 38d7dd0bdb28..730c10f7fbb7 100644
@@ -1392,7 +1392,8 @@ pmu_resume(void)
 static void
 pmu_handle_data(unsigned char *data, int len)
 {
-	unsigned char ints, pirq;
+	unsigned char ints;
+	int idx;
 	int i = 0;
 
 	asleep = 0;
@@ -1414,25 +1415,24 @@ pmu_handle_data(unsigned char *data, int len)
 		ints &= ~(PMU_INT_ADB_AUTO | PMU_INT_AUTO_SRQ_POLL);
 
 next:
-
 	if (ints == 0) {
 		if (i > pmu_irq_stats[10])
 			pmu_irq_stats[10] = i;
 		return;
 	}
-
-	for (pirq = 0; pirq < 8; pirq++)
-		if (ints & (1 << pirq))
-			break;
-	pmu_irq_stats[pirq]++;
 	i++;
-	ints &= ~(1 << pirq);
+
+	idx = ffs(ints) - 1;
+	ints &= ~BIT(idx);
+
+	pmu_irq_stats[idx]++;
 
 	/* Note: for some reason, we get an interrupt with len=1,
 	 * data[0]==0 after each normal ADB interrupt, at least
 	 * on the Pismo. Still investigating...  --BenH
 	 */
-	if ((1 << pirq) & PMU_INT_ADB) {
+	switch (BIT(idx)) {
+	case PMU_INT_ADB:
 		if ((data[0] & PMU_INT_ADB_AUTO) == 0) {
 			struct adb_request *req = req_awaiting_reply;
 			if (req == 0) {
@@ -1470,25 +1470,28 @@ pmu_handle_data(unsigned char *data, int len)
 				adb_input(data+1, len-1, 1);
 #endif /* CONFIG_ADB */		
 		}
-	}
+		break;
+
 	/* Sound/brightness button pressed */
-	else if ((1 << pirq) & PMU_INT_SNDBRT) {
+	case PMU_INT_SNDBRT:
 #ifdef CONFIG_PMAC_BACKLIGHT
 		if (len == 3)
 			pmac_backlight_set_legacy_brightness_pmu(data[1] >> 4);
 #endif
-	}
+		break;
+
 	/* Tick interrupt */
-	else if ((1 << pirq) & PMU_INT_TICK) {
-		/* Environement or tick interrupt, query batteries */
+	case PMU_INT_TICK:
+		/* Environment or tick interrupt, query batteries */
 		if (pmu_battery_count) {
 			if ((--query_batt_timer) == 0) {
 				query_battery_state();
 				query_batt_timer = BATTERY_POLLING_COUNT;
 			}
 		}
-        }
-	else if ((1 << pirq) & PMU_INT_ENVIRONMENT) {
+		break;
+
+	case PMU_INT_ENVIRONMENT:
 		if (pmu_battery_count)
 			query_battery_state();
 		pmu_pass_intr(data, len);
@@ -1498,7 +1501,9 @@ pmu_handle_data(unsigned char *data, int len)
 			via_pmu_event(PMU_EVT_POWER, !!(data[1]&8));
 			via_pmu_event(PMU_EVT_LID, data[1]&1);
 		}
-	} else {
+		break;
+
+	default:
 	       pmu_pass_intr(data, len);
 	}
 	goto next;
Signature · 1 line
2.16.4
author
#26 lore raw

PATCH v2 12/12 macintosh/via-pmu: Disambiguate interrupt statistics

In reply to Finn Thain

Some of the event counters are overloaded which makes it very difficult to interpret their values.

Counter 0 is supposed to report CB1 interrupts but it can also count PMU_INT_WAITING_CHARGER events.

Counter 1 is supposed to report GPIO interrupts but it can also count other events (depending upon the value of the PMU_INT_ADB bit).

Disambiguate these statistics with dedicated counters for GPIO and CB1 interrupts.

Comments in the MkLinux source code say that the type 0 and type 1 interrupts are model-specific. Label them as "unknown".

This change to the contents of /proc/pmu/interrupts is by necessity visible in userland. However, packages which interact with the PMU (that is, pbbuttonsd, pmac-utils and pmud) don't open this file. AFAIK, user software has no need to poll these counters.

---
The file now looks like this,

  0:          0 (Unknown interrupt (type 0))
  1:          0 (Unknown interrupt (type 1))
  2:          0 (PC-Card eject button)
  3:         23 (Sound/Brightness button)
  4:         74 (ADB message)
  5:          0 (Battery state change)
  6:          0 (Environment interrupt)
  7:        121 (Tick timer)
  8:          0 (Ghost interrupt (zero len))
  9:          1 (Empty interrupt (empty mask))
 10:          2 (Max irqs in a row)

11: 194 (Total CB1 triggered events)
12: 0 (Total GPIO1 triggered events)

rather than this,

0: 194 (Total CB1 triggered events)
1: 0 (Total GPIO1 triggered events)
2: 0 (PC-Card eject button)
3: 23 (Sound/Brightness button)
4: 74 (ADB message)
5: 0 (Battery state change)
6: 0 (Environment interrupt)
7: 121 (Tick timer)

  8:          0 (Ghost interrupt (zero len))
  9:          1 (Empty interrupt (empty mask))
 10:          2 (Max irqs in a row)

If some parser exists for this file, and if this change is problematic, we could increment the driver version number in /proc/pmu/info, to correspond with the format change.

---
 drivers/macintosh/via-pmu.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)
drivers/macintosh/via-pmu.c +12 -8
index 730c10f7fbb7..44919b3b56e0 100644
@@ -172,7 +172,9 @@ static int drop_interrupts;
 static int option_lid_wakeup = 1;
 #endif /* CONFIG_SUSPEND && CONFIG_PPC32 */
 static unsigned long async_req_locks;
-static unsigned int pmu_irq_stats[11];
+
+#define NUM_IRQ_STATS 13
+static unsigned int pmu_irq_stats[NUM_IRQ_STATS];
 
 static struct proc_dir_entry *proc_pmu_root;
 static struct proc_dir_entry *proc_pmu_info;
@@ -884,9 +886,9 @@ static const struct file_operations pmu_info_proc_fops = {
 static int pmu_irqstats_proc_show(struct seq_file *m, void *v)
 {
 	int i;
-	static const char *irq_names[] = {
-		"Total CB1 triggered events",
-		"Total GPIO1 triggered events",
+	static const char *irq_names[NUM_IRQ_STATS] = {
+		"Unknown interrupt (type 0)",
+		"Unknown interrupt (type 1)",
 		"PC-Card eject button",
 		"Sound/Brightness button",
 		"ADB message",
@@ -895,10 +897,12 @@ static int pmu_irqstats_proc_show(struct seq_file *m, void *v)
 		"Tick timer",
 		"Ghost interrupt (zero len)",
 		"Empty interrupt (empty mask)",
-		"Max irqs in a row"
+		"Max irqs in a row",
+		"Total CB1 triggered events",
+		"Total GPIO1 triggered events",
         };
 
-	for (i=0; i<11; i++) {
+	for (i = 0; i < NUM_IRQ_STATS; i++) {
 		seq_printf(m, " %2u: %10u (%s)\n",
 			     i, pmu_irq_stats[i], irq_names[i]);
 	}
@@ -1659,7 +1663,7 @@ via_pmu_interrupt(int irq, void *arg)
 		}
 		if (intr & CB1_INT) {
 			adb_int_pending = 1;
-			pmu_irq_stats[0]++;
+			pmu_irq_stats[11]++;
 		}
 		if (intr & SR_INT) {
 			req = pmu_sr_intr();
@@ -1746,7 +1750,7 @@ gpio1_interrupt(int irq, void *arg)
 			disable_irq_nosync(gpio_irq);
 			gpio_irq_enabled = 0;
 		}
-		pmu_irq_stats[1]++;
+		pmu_irq_stats[12]++;
 		adb_int_pending = 1;
 		spin_unlock_irqrestore(&pmu_lock, flags);
 		via_pmu_interrupt(0, NULL);
Signature · 1 line
2.16.4