Linux-m68k Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/8] Mac ADB IOP driver fixes
@ 2020-05-30 23:17 Finn Thain
  2020-05-30 23:17 ` [PATCH 7/8] macintosh/adb-iop: Implement sending -> idle state transition Finn Thain
                   ` (8 more replies)
  0 siblings, 9 replies; 16+ messages in thread
From: Finn Thain @ 2020-05-30 23:17 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: Joshua Thompson, linux-m68k, linuxppc-dev, linux-kernel,
	Geert Uytterhoeven

The adb-iop driver was never finished. Some deficiencies have become
apparent over the years. For example,

 - Mouse and/or keyboard may stop working if used together

 - SRQ autopoll list cannot be changed

 - Some bugs were found by inspection

This patch series contains fixes for the known bugs in the driver, plus
a few clean-ups.


Finn Thain (8):
  macintosh/adb-iop: Remove dead and redundant code
  macintosh/adb-iop: Correct comment text
  macintosh/adb-iop: Adopt bus reset algorithm from via-macii driver
  macintosh/adb-iop: Access current_req and adb_iop_state when inside
    lock
  macintosh/adb-iop: Resolve static checker warnings
  macintosh/adb-iop: Implement idle -> sending state transition
  macintosh/adb-iop: Implement sending -> idle state transition
  macintosh/adb-iop: Implement SRQ autopolling

 arch/m68k/include/asm/adb_iop.h |   1 +
 drivers/macintosh/adb-iop.c     | 186 +++++++++++++++-----------------
 2 files changed, 86 insertions(+), 101 deletions(-)

-- 
2.26.2


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 2/8] macintosh/adb-iop: Correct comment text
  2020-05-30 23:17 [PATCH 0/8] Mac ADB IOP driver fixes Finn Thain
  2020-05-30 23:17 ` [PATCH 7/8] macintosh/adb-iop: Implement sending -> idle state transition Finn Thain
@ 2020-05-30 23:17 ` Finn Thain
  2020-05-30 23:17 ` [PATCH 8/8] macintosh/adb-iop: Implement SRQ autopolling Finn Thain
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 16+ messages in thread
From: Finn Thain @ 2020-05-30 23:17 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: Joshua Thompson, linux-m68k, linuxppc-dev, linux-kernel

This patch improves comment style and corrects some misunderstandings
in the text.

Cc: Joshua Thompson <funaho@jurai.org>
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 drivers/macintosh/adb-iop.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/macintosh/adb-iop.c b/drivers/macintosh/adb-iop.c
index ce28ff40fb9c..ca3b411b0742 100644
--- a/drivers/macintosh/adb-iop.c
+++ b/drivers/macintosh/adb-iop.c
@@ -101,11 +101,10 @@ static void adb_iop_listen(struct iop_msg *msg)
 
 	req = current_req;
 
-	/* Handle a timeout. Timeout packets seem to occur even after */
-	/* we've gotten a valid reply to a TALK, so I'm assuming that */
-	/* a "timeout" is actually more like an "end-of-data" signal. */
-	/* We need to send back a timeout packet to the IOP to shut   */
-	/* it up, plus complete the current request, if any.          */
+	/* Handle a timeout. Timeout packets seem to occur even after
+	 * we've gotten a valid reply to a TALK, presumably because of
+	 * autopolling.
+	 */
 
 	if (amsg->flags & ADB_IOP_TIMEOUT) {
 		msg->reply[0] = ADB_IOP_TIMEOUT | ADB_IOP_AUTOPOLL;
@@ -115,9 +114,6 @@ static void adb_iop_listen(struct iop_msg *msg)
 			adb_iop_end_req(req, idle);
 		}
 	} else {
-		/* TODO: is it possible for more than one chunk of data  */
-		/*       to arrive before the timeout? If so we need to */
-		/*       use reply_ptr here like the other drivers do.  */
 		if ((adb_iop_state == awaiting_reply) &&
 		    (amsg->flags & ADB_IOP_EXPLICIT)) {
 			req->reply_len = amsg->count + 1;
@@ -152,23 +148,24 @@ static void adb_iop_start(void)
 
 	local_irq_save(flags);
 
-	/* The IOP takes MacII-style packets, so */
-	/* strip the initial ADB_PACKET byte.    */
-
+	/* The IOP takes MacII-style packets, so strip the initial
+	 * ADB_PACKET byte.
+	 */
 	amsg.flags = ADB_IOP_EXPLICIT;
 	amsg.count = req->nbytes - 2;
 
-	/* amsg.data immediately follows amsg.cmd, effectively making */
-	/* amsg.cmd a pointer to the beginning of a full ADB packet.  */
+	/* amsg.data immediately follows amsg.cmd, effectively making
+	 * &amsg.cmd a pointer to the beginning of a full ADB packet.
+	 */
 	memcpy(&amsg.cmd, req->data + 1, req->nbytes - 1);
 
 	req->sent = 1;
 	adb_iop_state = sending;
 	local_irq_restore(flags);
 
-	/* Now send it. The IOP manager will call adb_iop_complete */
-	/* when the packet has been sent.                          */
-
+	/* Now send it. The IOP manager will call adb_iop_complete
+	 * when the message has been sent.
+	 */
 	iop_send_message(ADB_IOP, ADB_CHAN, req, sizeof(amsg), (__u8 *)&amsg,
 			 adb_iop_complete);
 }
-- 
2.26.2


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 6/8] macintosh/adb-iop: Implement idle -> sending state transition
  2020-05-30 23:17 [PATCH 0/8] Mac ADB IOP driver fixes Finn Thain
                   ` (2 preceding siblings ...)
  2020-05-30 23:17 ` [PATCH 8/8] macintosh/adb-iop: Implement SRQ autopolling Finn Thain
@ 2020-05-30 23:17 ` Finn Thain
  2020-05-30 23:17 ` [PATCH 3/8] macintosh/adb-iop: Adopt bus reset algorithm from via-macii driver Finn Thain
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 16+ messages in thread
From: Finn Thain @ 2020-05-30 23:17 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: Joshua Thompson, linux-m68k, linuxppc-dev, linux-kernel

In the present algorithm, the 'idle' state transition does not take
place until there's a bus timeout. Once idle, the driver does not
automatically proceed with the next request.

Change the algorithm so that queued ADB requests will be sent as soon as
the driver becomes idle. This is to take place after the current IOP
message is completed.

Cc: Joshua Thompson <funaho@jurai.org>
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 drivers/macintosh/adb-iop.c | 43 +++++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/macintosh/adb-iop.c b/drivers/macintosh/adb-iop.c
index a2b28e09f2ce..f22792c95d4f 100644
--- a/drivers/macintosh/adb-iop.c
+++ b/drivers/macintosh/adb-iop.c
@@ -54,13 +54,19 @@ struct adb_driver adb_iop_driver = {
 	.reset_bus    = adb_iop_reset_bus
 };
 
-static void adb_iop_end_req(struct adb_request *req, int state)
+static void adb_iop_done(void)
 {
+	struct adb_request *req = current_req;
+
+	adb_iop_state = idle;
+
 	req->complete = 1;
 	current_req = req->next;
 	if (req->done)
 		(*req->done)(req);
-	adb_iop_state = state;
+
+	if (adb_iop_state == idle)
+		adb_iop_start();
 }
 
 /*
@@ -94,37 +100,36 @@ static void adb_iop_complete(struct iop_msg *msg)
 static void adb_iop_listen(struct iop_msg *msg)
 {
 	struct adb_iopmsg *amsg = (struct adb_iopmsg *)msg->message;
-	struct adb_request *req;
 	unsigned long flags;
+	bool req_done = false;
 
 	local_irq_save(flags);
 
-	req = current_req;
-
 	/* Handle a timeout. Timeout packets seem to occur even after
 	 * we've gotten a valid reply to a TALK, presumably because of
 	 * autopolling.
 	 */
 
-	if (amsg->flags & ADB_IOP_TIMEOUT) {
-		msg->reply[0] = ADB_IOP_TIMEOUT | ADB_IOP_AUTOPOLL;
-		msg->reply[1] = 0;
-		msg->reply[2] = 0;
-		if (req && (adb_iop_state != idle)) {
-			adb_iop_end_req(req, idle);
-		}
-	} else {
-		if ((adb_iop_state == awaiting_reply) &&
-		    (amsg->flags & ADB_IOP_EXPLICIT)) {
+	if (amsg->flags & ADB_IOP_EXPLICIT) {
+		if (adb_iop_state == awaiting_reply) {
+			struct adb_request *req = current_req;
+
 			req->reply_len = amsg->count + 1;
 			memcpy(req->reply, &amsg->cmd, req->reply_len);
-		} else {
-			adb_input(&amsg->cmd, amsg->count + 1,
-				  amsg->flags & ADB_IOP_AUTOPOLL);
+
+			req_done = true;
 		}
-		memcpy(msg->reply, msg->message, IOP_MSG_LEN);
+	} else if (!(amsg->flags & ADB_IOP_TIMEOUT)) {
+		adb_input(&amsg->cmd, amsg->count + 1,
+			  amsg->flags & ADB_IOP_AUTOPOLL);
 	}
+
+	msg->reply[0] = ADB_IOP_AUTOPOLL;
 	iop_complete_message(msg);
+
+	if (req_done)
+		adb_iop_done();
+
 	local_irq_restore(flags);
 }
 
-- 
2.26.2


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 8/8] macintosh/adb-iop: Implement SRQ autopolling
  2020-05-30 23:17 [PATCH 0/8] Mac ADB IOP driver fixes Finn Thain
  2020-05-30 23:17 ` [PATCH 7/8] macintosh/adb-iop: Implement sending -> idle state transition Finn Thain
  2020-05-30 23:17 ` [PATCH 2/8] macintosh/adb-iop: Correct comment text Finn Thain
@ 2020-05-30 23:17 ` Finn Thain
  2020-05-31  8:38   ` Geert Uytterhoeven
  2020-05-30 23:17 ` [PATCH 6/8] macintosh/adb-iop: Implement idle -> sending state transition Finn Thain
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 16+ messages in thread
From: Finn Thain @ 2020-05-30 23:17 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: Joshua Thompson, linux-m68k, Geert Uytterhoeven, linux-kernel,
	linuxppc-dev

The adb_driver.autopoll method is needed during ADB bus scan and device
address assignment. Implement this method so that the IOP's list of
device addresses can be updated. When the list is empty, disable SRQ
autopolling.

Cc: Joshua Thompson <funaho@jurai.org>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 arch/m68k/include/asm/adb_iop.h |  1 +
 drivers/macintosh/adb-iop.c     | 32 ++++++++++++++++++++++++++------
 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/arch/m68k/include/asm/adb_iop.h b/arch/m68k/include/asm/adb_iop.h
index 195d7fb1268c..6aecd020e2fc 100644
--- a/arch/m68k/include/asm/adb_iop.h
+++ b/arch/m68k/include/asm/adb_iop.h
@@ -29,6 +29,7 @@
 
 #define ADB_IOP_EXPLICIT	0x80	/* nonzero if explicit command */
 #define ADB_IOP_AUTOPOLL	0x40	/* auto/SRQ polling enabled    */
+#define ADB_IOP_SET_AUTOPOLL	0x20	/* set autopoll device list    */
 #define ADB_IOP_SRQ		0x04	/* SRQ detected                */
 #define ADB_IOP_TIMEOUT		0x02	/* nonzero if timeout          */
 
diff --git a/drivers/macintosh/adb-iop.c b/drivers/macintosh/adb-iop.c
index 8594e4f9a830..f3d1a460fbce 100644
--- a/drivers/macintosh/adb-iop.c
+++ b/drivers/macintosh/adb-iop.c
@@ -7,10 +7,6 @@
  * 1999-07-01 (jmt) - First implementation for new driver architecture.
  *
  * 1999-07-31 (jmt) - First working version.
- *
- * TODO:
- *
- * o Implement SRQ handling.
  */
 
 #include <linux/types.h>
@@ -28,6 +24,7 @@
 
 static struct adb_request *current_req;
 static struct adb_request *last_req;
+static unsigned int autopoll_devs;
 
 static enum adb_iop_state {
 	idle,
@@ -123,7 +120,7 @@ static void adb_iop_listen(struct iop_msg *msg)
 			  amsg->flags & ADB_IOP_AUTOPOLL);
 	}
 
-	msg->reply[0] = ADB_IOP_AUTOPOLL;
+	msg->reply[0] = autopoll_devs ? ADB_IOP_AUTOPOLL : 0;
 	iop_complete_message(msg);
 
 	if (req_done)
@@ -231,9 +228,32 @@ static int adb_iop_write(struct adb_request *req)
 	return 0;
 }
 
+static void adb_iop_set_ap_complete(struct iop_msg *msg)
+{
+	struct adb_iopmsg *amsg = (struct adb_iopmsg *)msg->message;
+
+	autopoll_devs = (amsg->data[1] << 8) | amsg->data[0];
+}
+
 static int adb_iop_autopoll(int devs)
 {
-	/* TODO: how do we enable/disable autopoll? */
+	struct adb_iopmsg amsg;
+	unsigned long flags;
+	unsigned int mask = (unsigned int)devs & 0xFFFE;
+
+	local_irq_save(flags);
+
+	amsg.flags = ADB_IOP_SET_AUTOPOLL | (mask ? ADB_IOP_AUTOPOLL : 0);
+	amsg.count = 2;
+	amsg.cmd = 0;
+	amsg.data[0] = mask & 0xFF;
+	amsg.data[1] = (mask >> 8) & 0xFF;
+
+	iop_send_message(ADB_IOP, ADB_CHAN, NULL, sizeof(amsg), (__u8 *)&amsg,
+			 adb_iop_set_ap_complete);
+
+	local_irq_restore(flags);
+
 	return 0;
 }
 
-- 
2.26.2


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 5/8] macintosh/adb-iop: Resolve static checker warnings
  2020-05-30 23:17 [PATCH 0/8] Mac ADB IOP driver fixes Finn Thain
                   ` (6 preceding siblings ...)
  2020-05-30 23:17 ` [PATCH 1/8] macintosh/adb-iop: Remove dead and redundant code Finn Thain
@ 2020-05-30 23:17 ` Finn Thain
  2020-07-27  7:26 ` [PATCH 0/8] Mac ADB IOP driver fixes Michael Ellerman
  8 siblings, 0 replies; 16+ messages in thread
From: Finn Thain @ 2020-05-30 23:17 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: Joshua Thompson, linux-m68k, linuxppc-dev, linux-kernel

drivers/macintosh/adb-iop.c:215:28: warning: Using plain integer as NULL pointer
drivers/macintosh/adb-iop.c:170:5: warning: symbol 'adb_iop_probe' was not declared. Should it be static?
drivers/macintosh/adb-iop.c:177:5: warning: symbol 'adb_iop_init' was not declared. Should it be static?
drivers/macintosh/adb-iop.c:184:5: warning: symbol 'adb_iop_send_request' was not declared. Should it be static?
drivers/macintosh/adb-iop.c:230:5: warning: symbol 'adb_iop_autopoll' was not declared. Should it be static?
drivers/macintosh/adb-iop.c:236:6: warning: symbol 'adb_iop_poll' was not declared. Should it be static?
drivers/macintosh/adb-iop.c:241:5: warning: symbol 'adb_iop_reset_bus' was not declared. Should it be static?

Cc: Joshua Thompson <funaho@jurai.org>
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 drivers/macintosh/adb-iop.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/macintosh/adb-iop.c b/drivers/macintosh/adb-iop.c
index 7ecc41bc7358..a2b28e09f2ce 100644
--- a/drivers/macintosh/adb-iop.c
+++ b/drivers/macintosh/adb-iop.c
@@ -166,21 +166,21 @@ static void adb_iop_start(void)
 			 adb_iop_complete);
 }
 
-int adb_iop_probe(void)
+static int adb_iop_probe(void)
 {
 	if (!iop_ism_present)
 		return -ENODEV;
 	return 0;
 }
 
-int adb_iop_init(void)
+static int adb_iop_init(void)
 {
 	pr_info("adb: IOP ISM driver v0.4 for Unified ADB\n");
 	iop_listen(ADB_IOP, ADB_CHAN, adb_iop_listen, "ADB");
 	return 0;
 }
 
-int adb_iop_send_request(struct adb_request *req, int sync)
+static int adb_iop_send_request(struct adb_request *req, int sync)
 {
 	int err;
 
@@ -211,7 +211,7 @@ static int adb_iop_write(struct adb_request *req)
 
 	local_irq_save(flags);
 
-	if (current_req != 0) {
+	if (current_req) {
 		last_req->next = req;
 		last_req = req;
 	} else {
@@ -227,18 +227,18 @@ static int adb_iop_write(struct adb_request *req)
 	return 0;
 }
 
-int adb_iop_autopoll(int devs)
+static int adb_iop_autopoll(int devs)
 {
 	/* TODO: how do we enable/disable autopoll? */
 	return 0;
 }
 
-void adb_iop_poll(void)
+static void adb_iop_poll(void)
 {
 	iop_ism_irq_poll(ADB_IOP);
 }
 
-int adb_iop_reset_bus(void)
+static int adb_iop_reset_bus(void)
 {
 	struct adb_request req;
 
-- 
2.26.2


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 7/8] macintosh/adb-iop: Implement sending -> idle state transition
  2020-05-30 23:17 [PATCH 0/8] Mac ADB IOP driver fixes Finn Thain
@ 2020-05-30 23:17 ` Finn Thain
  2020-05-30 23:17 ` [PATCH 2/8] macintosh/adb-iop: Correct comment text Finn Thain
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 16+ messages in thread
From: Finn Thain @ 2020-05-30 23:17 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: Joshua Thompson, linux-m68k, linuxppc-dev, linux-kernel

On leaving the 'sending' state, proceed to the 'idle' state if no reply is
expected. Drop redundant test for adb_iop_state == sending && current_req.

Cc: Joshua Thompson <funaho@jurai.org>
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 drivers/macintosh/adb-iop.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/macintosh/adb-iop.c b/drivers/macintosh/adb-iop.c
index f22792c95d4f..8594e4f9a830 100644
--- a/drivers/macintosh/adb-iop.c
+++ b/drivers/macintosh/adb-iop.c
@@ -77,15 +77,14 @@ static void adb_iop_done(void)
 
 static void adb_iop_complete(struct iop_msg *msg)
 {
-	struct adb_request *req;
 	unsigned long flags;
 
 	local_irq_save(flags);
 
-	req = current_req;
-	if ((adb_iop_state == sending) && req && req->reply_expected) {
+	if (current_req->reply_expected)
 		adb_iop_state = awaiting_reply;
-	}
+	else
+		adb_iop_done();
 
 	local_irq_restore(flags);
 }
-- 
2.26.2


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 4/8] macintosh/adb-iop: Access current_req and adb_iop_state when inside lock
  2020-05-30 23:17 [PATCH 0/8] Mac ADB IOP driver fixes Finn Thain
                   ` (4 preceding siblings ...)
  2020-05-30 23:17 ` [PATCH 3/8] macintosh/adb-iop: Adopt bus reset algorithm from via-macii driver Finn Thain
@ 2020-05-30 23:17 ` Finn Thain
  2020-05-30 23:17 ` [PATCH 1/8] macintosh/adb-iop: Remove dead and redundant code Finn Thain
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 16+ messages in thread
From: Finn Thain @ 2020-05-30 23:17 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: Joshua Thompson, linux-m68k, linuxppc-dev, linux-kernel

Drop the redundant local_irq_save/restore() from adb_iop_start() because
the caller has to do it anyway. This is the pattern used in via-macii.

Cc: Joshua Thompson <funaho@jurai.org>
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 drivers/macintosh/adb-iop.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/macintosh/adb-iop.c b/drivers/macintosh/adb-iop.c
index c3089dacf2e2..7ecc41bc7358 100644
--- a/drivers/macintosh/adb-iop.c
+++ b/drivers/macintosh/adb-iop.c
@@ -137,7 +137,6 @@ static void adb_iop_listen(struct iop_msg *msg)
 
 static void adb_iop_start(void)
 {
-	unsigned long flags;
 	struct adb_request *req;
 	struct adb_iopmsg amsg;
 
@@ -146,8 +145,6 @@ static void adb_iop_start(void)
 	if (!req)
 		return;
 
-	local_irq_save(flags);
-
 	/* The IOP takes MacII-style packets, so strip the initial
 	 * ADB_PACKET byte.
 	 */
@@ -161,7 +158,6 @@ static void adb_iop_start(void)
 
 	req->sent = 1;
 	adb_iop_state = sending;
-	local_irq_restore(flags);
 
 	/* Now send it. The IOP manager will call adb_iop_complete
 	 * when the message has been sent.
@@ -208,13 +204,13 @@ static int adb_iop_write(struct adb_request *req)
 		return -EINVAL;
 	}
 
-	local_irq_save(flags);
-
 	req->next = NULL;
 	req->sent = 0;
 	req->complete = 0;
 	req->reply_len = 0;
 
+	local_irq_save(flags);
+
 	if (current_req != 0) {
 		last_req->next = req;
 		last_req = req;
@@ -223,10 +219,11 @@ static int adb_iop_write(struct adb_request *req)
 		last_req = req;
 	}
 
-	local_irq_restore(flags);
-
 	if (adb_iop_state == idle)
 		adb_iop_start();
+
+	local_irq_restore(flags);
+
 	return 0;
 }
 
-- 
2.26.2


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 3/8] macintosh/adb-iop: Adopt bus reset algorithm from via-macii driver
  2020-05-30 23:17 [PATCH 0/8] Mac ADB IOP driver fixes Finn Thain
                   ` (3 preceding siblings ...)
  2020-05-30 23:17 ` [PATCH 6/8] macintosh/adb-iop: Implement idle -> sending state transition Finn Thain
@ 2020-05-30 23:17 ` Finn Thain
  2020-05-30 23:17 ` [PATCH 4/8] macintosh/adb-iop: Access current_req and adb_iop_state when inside lock Finn Thain
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 16+ messages in thread
From: Finn Thain @ 2020-05-30 23:17 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: Joshua Thompson, linux-m68k, linuxppc-dev, linux-kernel

This algorithm is slightly shorter and avoids the surprising
adb_iop_start() call in adb_iop_poll().

Cc: Joshua Thompson <funaho@jurai.org>
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 drivers/macintosh/adb-iop.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/macintosh/adb-iop.c b/drivers/macintosh/adb-iop.c
index ca3b411b0742..c3089dacf2e2 100644
--- a/drivers/macintosh/adb-iop.c
+++ b/drivers/macintosh/adb-iop.c
@@ -238,24 +238,19 @@ int adb_iop_autopoll(int devs)
 
 void adb_iop_poll(void)
 {
-	if (adb_iop_state == idle)
-		adb_iop_start();
 	iop_ism_irq_poll(ADB_IOP);
 }
 
 int adb_iop_reset_bus(void)
 {
-	struct adb_request req = {
-		.reply_expected = 0,
-		.nbytes = 2,
-		.data = { ADB_PACKET, 0 },
-	};
-
-	adb_iop_write(&req);
-	while (!req.complete) {
-		adb_iop_poll();
-		schedule();
-	}
+	struct adb_request req;
+
+	/* Command = 0, Address = ignored */
+	adb_request(&req, NULL, ADBREQ_NOSEND, 1, ADB_BUSRESET);
+	adb_iop_send_request(&req, 1);
+
+	/* Don't want any more requests during the Global Reset low time. */
+	mdelay(3);
 
 	return 0;
 }
-- 
2.26.2


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/8] macintosh/adb-iop: Remove dead and redundant code
  2020-05-30 23:17 [PATCH 0/8] Mac ADB IOP driver fixes Finn Thain
                   ` (5 preceding siblings ...)
  2020-05-30 23:17 ` [PATCH 4/8] macintosh/adb-iop: Access current_req and adb_iop_state when inside lock Finn Thain
@ 2020-05-30 23:17 ` Finn Thain
  2020-05-30 23:17 ` [PATCH 5/8] macintosh/adb-iop: Resolve static checker warnings Finn Thain
  2020-07-27  7:26 ` [PATCH 0/8] Mac ADB IOP driver fixes Michael Ellerman
  8 siblings, 0 replies; 16+ messages in thread
From: Finn Thain @ 2020-05-30 23:17 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: Joshua Thompson, linux-m68k, linuxppc-dev, linux-kernel

Cc: Joshua Thompson <funaho@jurai.org>
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 drivers/macintosh/adb-iop.c | 29 -----------------------------
 1 file changed, 29 deletions(-)

diff --git a/drivers/macintosh/adb-iop.c b/drivers/macintosh/adb-iop.c
index fca31640e3ef..ce28ff40fb9c 100644
--- a/drivers/macintosh/adb-iop.c
+++ b/drivers/macintosh/adb-iop.c
@@ -18,24 +18,16 @@
 #include <linux/mm.h>
 #include <linux/delay.h>
 #include <linux/init.h>
-#include <linux/proc_fs.h>
 
 #include <asm/macintosh.h>
 #include <asm/macints.h>
 #include <asm/mac_iop.h>
-#include <asm/mac_oss.h>
 #include <asm/adb_iop.h>
 
 #include <linux/adb.h>
 
-/*#define DEBUG_ADB_IOP*/
-
 static struct adb_request *current_req;
 static struct adb_request *last_req;
-#if 0
-static unsigned char reply_buff[16];
-static unsigned char *reply_ptr;
-#endif
 
 static enum adb_iop_state {
 	idle,
@@ -104,22 +96,11 @@ static void adb_iop_listen(struct iop_msg *msg)
 	struct adb_iopmsg *amsg = (struct adb_iopmsg *)msg->message;
 	struct adb_request *req;
 	unsigned long flags;
-#ifdef DEBUG_ADB_IOP
-	int i;
-#endif
 
 	local_irq_save(flags);
 
 	req = current_req;
 
-#ifdef DEBUG_ADB_IOP
-	printk("adb_iop_listen %p: rcvd packet, %d bytes: %02X %02X", req,
-	       (uint)amsg->count + 2, (uint)amsg->flags, (uint)amsg->cmd);
-	for (i = 0; i < amsg->count; i++)
-		printk(" %02X", (uint)amsg->data[i]);
-	printk("\n");
-#endif
-
 	/* Handle a timeout. Timeout packets seem to occur even after */
 	/* we've gotten a valid reply to a TALK, so I'm assuming that */
 	/* a "timeout" is actually more like an "end-of-data" signal. */
@@ -163,9 +144,6 @@ static void adb_iop_start(void)
 	unsigned long flags;
 	struct adb_request *req;
 	struct adb_iopmsg amsg;
-#ifdef DEBUG_ADB_IOP
-	int i;
-#endif
 
 	/* get the packet to send */
 	req = current_req;
@@ -174,13 +152,6 @@ static void adb_iop_start(void)
 
 	local_irq_save(flags);
 
-#ifdef DEBUG_ADB_IOP
-	printk("adb_iop_start %p: sending packet, %d bytes:", req, req->nbytes);
-	for (i = 0; i < req->nbytes; i++)
-		printk(" %02X", (uint)req->data[i]);
-	printk("\n");
-#endif
-
 	/* The IOP takes MacII-style packets, so */
 	/* strip the initial ADB_PACKET byte.    */
 
-- 
2.26.2


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 8/8] macintosh/adb-iop: Implement SRQ autopolling
  2020-05-30 23:17 ` [PATCH 8/8] macintosh/adb-iop: Implement SRQ autopolling Finn Thain
@ 2020-05-31  8:38   ` Geert Uytterhoeven
  2020-05-31 20:01     ` Brad Boyer
  2020-06-01  0:14     ` Finn Thain
  0 siblings, 2 replies; 16+ messages in thread
From: Geert Uytterhoeven @ 2020-05-31  8:38 UTC (permalink / raw)
  To: Finn Thain
  Cc: Benjamin Herrenschmidt, Joshua Thompson, linux-m68k,
	Linux Kernel Mailing List, linuxppc-dev

Hi Finn,

On Sun, May 31, 2020 at 1:20 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> The adb_driver.autopoll method is needed during ADB bus scan and device
> address assignment. Implement this method so that the IOP's list of
> device addresses can be updated. When the list is empty, disable SRQ
> autopolling.
>
> Cc: Joshua Thompson <funaho@jurai.org>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Tested-by: Stan Johnson <userm57@yahoo.com>
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>

Thanks for your patch!

Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>

>  arch/m68k/include/asm/adb_iop.h |  1 +
>  drivers/macintosh/adb-iop.c     | 32 ++++++++++++++++++++++++++------

As this header file is used by a single source file only, perhaps it should
just be absorbed by the latter?
Then you no longer need my Acked-by for future changes ;-)

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 8/8] macintosh/adb-iop: Implement SRQ autopolling
  2020-05-31  8:38   ` Geert Uytterhoeven
@ 2020-05-31 20:01     ` Brad Boyer
  2020-06-01  9:10       ` Geert Uytterhoeven
  2020-06-01  0:14     ` Finn Thain
  1 sibling, 1 reply; 16+ messages in thread
From: Brad Boyer @ 2020-05-31 20:01 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Finn Thain, Benjamin Herrenschmidt, Joshua Thompson, linux-m68k,
	linuxppc-dev

On Sun, May 31, 2020 at 10:38:04AM +0200, Geert Uytterhoeven wrote:
> >  arch/m68k/include/asm/adb_iop.h |  1 +
> 
> As this header file is used by a single source file only, perhaps it should
> just be absorbed by the latter?
> Then you no longer need my Acked-by for future changes ;-)

While I don't really feel involved in this specific change (although I
was one of the testers when the driver was first written), I am a little
curious about the current coding standards. This header is pretty much
just a declaration of the hardware interface, of which there are many
in this directory. Is it better to just define all the offsets and bits
for hardware registers inside the driver? We used to always put them in
headers like this, but is that not the current standard? Would it be
cleaner to put such headers in the directory with the driver effectively
making them private? I seem to see quite a bit of that as well.

Thank you for your advice.

	Brad Boyer
	flar@allandria.com


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 8/8] macintosh/adb-iop: Implement SRQ autopolling
  2020-05-31  8:38   ` Geert Uytterhoeven
  2020-05-31 20:01     ` Brad Boyer
@ 2020-06-01  0:14     ` Finn Thain
  2020-06-01  9:12       ` Geert Uytterhoeven
  1 sibling, 1 reply; 16+ messages in thread
From: Finn Thain @ 2020-06-01  0:14 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Benjamin Herrenschmidt, Joshua Thompson, linux-m68k,
	Linux Kernel Mailing List, linuxppc-dev

On Sun, 31 May 2020, Geert Uytterhoeven wrote:

> Hi Finn,
> 
> On Sun, May 31, 2020 at 1:20 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> > The adb_driver.autopoll method is needed during ADB bus scan and device
> > address assignment. Implement this method so that the IOP's list of
> > device addresses can be updated. When the list is empty, disable SRQ
> > autopolling.
> >
> > Cc: Joshua Thompson <funaho@jurai.org>
> > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > Tested-by: Stan Johnson <userm57@yahoo.com>
> > Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> 
> Thanks for your patch!
> 
> Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
> 

Thanks for your tag.

> >  arch/m68k/include/asm/adb_iop.h |  1 +
> >  drivers/macintosh/adb-iop.c     | 32 ++++++++++++++++++++++++++------
> 
> As this header file is used by a single source file only, perhaps it 
> should just be absorbed by the latter?

Sure, it could be absorbed by both asm/mac_iop.h and 
drivers/macintosh/adb-iop.c but I don't see the point...

> Then you no longer need my Acked-by for future changes ;-)
> 

But you shouldn't need to ack this kind of change in the first place.

IMHO, the arch/m68k/mac maintainer should be the one to ack changes to 
both arch/m68k/include/asm/adb_iop.h and drivers/macintosh/adb-iop.c.

Not that I'm complaining (thanks again for your ack!)

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 8/8] macintosh/adb-iop: Implement SRQ autopolling
  2020-05-31 20:01     ` Brad Boyer
@ 2020-06-01  9:10       ` Geert Uytterhoeven
  0 siblings, 0 replies; 16+ messages in thread
From: Geert Uytterhoeven @ 2020-06-01  9:10 UTC (permalink / raw)
  To: Brad Boyer
  Cc: Finn Thain, Benjamin Herrenschmidt, Joshua Thompson, linux-m68k,
	linuxppc-dev

Hi Brad,

On Sun, May 31, 2020 at 10:01 PM Brad Boyer <flar@allandria.com> wrote:
> On Sun, May 31, 2020 at 10:38:04AM +0200, Geert Uytterhoeven wrote:
> > >  arch/m68k/include/asm/adb_iop.h |  1 +
> > As this header file is used by a single source file only, perhaps it should
> > just be absorbed by the latter?
> > Then you no longer need my Acked-by for future changes ;-)
>
> While I don't really feel involved in this specific change (although I
> was one of the testers when the driver was first written), I am a little
> curious about the current coding standards. This header is pretty much
> just a declaration of the hardware interface, of which there are many
> in this directory. Is it better to just define all the offsets and bits
> for hardware registers inside the driver? We used to always put them in
> headers like this, but is that not the current standard? Would it be
> cleaner to put such headers in the directory with the driver effectively
> making them private? I seem to see quite a bit of that as well.

The idea behind header files is to share definitions among multipe
source files, right? It seems there is only one user of this header
file, so no sharing is involved, and thus these definitions are
de-facto private to the driver.  Hence, the hardware declarations
could be absorbed by the driver source file.

In this case having two separate files makes maintenance more
difficult, as the two files belong to different maintainer spaces
(drivers/macintosh/ and arch/m68k/).

In general, moving header files to the driver directory is an option,
if nothing outside the driver directory needs it.

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 8/8] macintosh/adb-iop: Implement SRQ autopolling
  2020-06-01  0:14     ` Finn Thain
@ 2020-06-01  9:12       ` Geert Uytterhoeven
  2020-06-01 23:49         ` Finn Thain
  0 siblings, 1 reply; 16+ messages in thread
From: Geert Uytterhoeven @ 2020-06-01  9:12 UTC (permalink / raw)
  To: Finn Thain
  Cc: Benjamin Herrenschmidt, Joshua Thompson, linux-m68k,
	Linux Kernel Mailing List, linuxppc-dev

Hi Finn,

On Mon, Jun 1, 2020 at 2:15 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> On Sun, 31 May 2020, Geert Uytterhoeven wrote:
> > On Sun, May 31, 2020 at 1:20 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> > >  arch/m68k/include/asm/adb_iop.h |  1 +
> > >  drivers/macintosh/adb-iop.c     | 32 ++++++++++++++++++++++++++------
> >
> > As this header file is used by a single source file only, perhaps it
> > should just be absorbed by the latter?
>
> Sure, it could be absorbed by both asm/mac_iop.h and
> drivers/macintosh/adb-iop.c but I don't see the point...

asm/mac_iop.h doesn't include asm/adb_iop.h (at least not in my tree,
but perhaps you have plans to change that?), so there's only a single
user.

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 8/8] macintosh/adb-iop: Implement SRQ autopolling
  2020-06-01  9:12       ` Geert Uytterhoeven
@ 2020-06-01 23:49         ` Finn Thain
  0 siblings, 0 replies; 16+ messages in thread
From: Finn Thain @ 2020-06-01 23:49 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Benjamin Herrenschmidt, Joshua Thompson, linux-m68k,
	Linux Kernel Mailing List, linuxppc-dev

On Mon, 1 Jun 2020, Geert Uytterhoeven wrote:

> >
> > Sure, it could be absorbed by both asm/mac_iop.h and 
> > drivers/macintosh/adb-iop.c [...]
> 
> asm/mac_iop.h doesn't include asm/adb_iop.h (at least not in my tree, 
> but perhaps you have plans to change that?), so there's only a single 
> user.

What I meant by "both" was that part of asm/adb_iop.h could be absorbed by 
drivers/macintosh.adb-iop.c and the rest by asm/mac_iop.h. (And some of it 
could be tossed out.) I suspect that much of arch/m68k/include/asm could 
get the same treatment. But I doubt that there is any pay off, because the 
headers rarely change where they relate to hardware characteristics.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/8] Mac ADB IOP driver fixes
  2020-05-30 23:17 [PATCH 0/8] Mac ADB IOP driver fixes Finn Thain
                   ` (7 preceding siblings ...)
  2020-05-30 23:17 ` [PATCH 5/8] macintosh/adb-iop: Resolve static checker warnings Finn Thain
@ 2020-07-27  7:26 ` Michael Ellerman
  8 siblings, 0 replies; 16+ messages in thread
From: Michael Ellerman @ 2020-07-27  7:26 UTC (permalink / raw)
  To: Finn Thain, Benjamin Herrenschmidt
  Cc: Geert Uytterhoeven, linux-kernel, Joshua Thompson, linuxppc-dev,
	linux-m68k

On Sun, 31 May 2020 09:17:03 +1000, Finn Thain wrote:
> The adb-iop driver was never finished. Some deficiencies have become
> apparent over the years. For example,
> 
>  - Mouse and/or keyboard may stop working if used together
> 
>  - SRQ autopoll list cannot be changed
> 
> [...]

Applied to powerpc/next.

[1/8] macintosh/adb-iop: Remove dead and redundant code
      https://git.kernel.org/powerpc/c/8384c82ab0860cd7db2ce4ec403e574f4ee54b6e
[2/8] macintosh/adb-iop: Correct comment text
      https://git.kernel.org/powerpc/c/ff785e179faf4bb06a2f73b8dcde6dabb66a83d2
[3/8] macintosh/adb-iop: Adopt bus reset algorithm from via-macii driver
      https://git.kernel.org/powerpc/c/303511edb859b1fbf48b3c1d1d53b33a6ebd2a2b
[4/8] macintosh/adb-iop: Access current_req and adb_iop_state when inside lock
      https://git.kernel.org/powerpc/c/aac840eca8fec02d594560647130d4e4447e10d9
[5/8] macintosh/adb-iop: Resolve static checker warnings
      https://git.kernel.org/powerpc/c/56b732edda96b1942fff974fd298ea2a2c543b94
[6/8] macintosh/adb-iop: Implement idle -> sending state transition
      https://git.kernel.org/powerpc/c/32226e81704398317e1cc5a82d24c0ef3cc25e5e
[7/8] macintosh/adb-iop: Implement sending -> idle state transition
      https://git.kernel.org/powerpc/c/e2954e5f727fad126258e83259b513988973c166
[8/8] macintosh/adb-iop: Implement SRQ autopolling
      https://git.kernel.org/powerpc/c/c66da95a39ec2bb95544c3def974d96e8c178f57

cheers

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, back to index

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-30 23:17 [PATCH 0/8] Mac ADB IOP driver fixes Finn Thain
2020-05-30 23:17 ` [PATCH 7/8] macintosh/adb-iop: Implement sending -> idle state transition Finn Thain
2020-05-30 23:17 ` [PATCH 2/8] macintosh/adb-iop: Correct comment text Finn Thain
2020-05-30 23:17 ` [PATCH 8/8] macintosh/adb-iop: Implement SRQ autopolling Finn Thain
2020-05-31  8:38   ` Geert Uytterhoeven
2020-05-31 20:01     ` Brad Boyer
2020-06-01  9:10       ` Geert Uytterhoeven
2020-06-01  0:14     ` Finn Thain
2020-06-01  9:12       ` Geert Uytterhoeven
2020-06-01 23:49         ` Finn Thain
2020-05-30 23:17 ` [PATCH 6/8] macintosh/adb-iop: Implement idle -> sending state transition Finn Thain
2020-05-30 23:17 ` [PATCH 3/8] macintosh/adb-iop: Adopt bus reset algorithm from via-macii driver Finn Thain
2020-05-30 23:17 ` [PATCH 4/8] macintosh/adb-iop: Access current_req and adb_iop_state when inside lock Finn Thain
2020-05-30 23:17 ` [PATCH 1/8] macintosh/adb-iop: Remove dead and redundant code Finn Thain
2020-05-30 23:17 ` [PATCH 5/8] macintosh/adb-iop: Resolve static checker warnings Finn Thain
2020-07-27  7:26 ` [PATCH 0/8] Mac ADB IOP driver fixes Michael Ellerman

Linux-m68k Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-m68k/0 linux-m68k/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-m68k linux-m68k/ https://lore.kernel.org/linux-m68k \
		linux-m68k@vger.kernel.org linux-m68k@lists.linux-m68k.org
	public-inbox-index linux-m68k

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-m68k


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git