[PATCH 0/4] Mac IOP driver fixes

[PATCH 0/4] Mac IOP driver fixes

[Finn Thain]

This patch series has several bug fixes for the IOP driver and some
improvements to the debug level log messages.

Geert, please consider pushing these fixes for v5.8, if not the
whole series.


Finn Thain (4):
  m68k/mac: Don't send IOP message until channel is idle
  m68k/mac: Fix IOP status/control register writes
  m68k/mac: Don't send uninitialized data in IOP message reply
  m68k/mac: Improve IOP debug messages

 arch/m68k/mac/iop.c | 60 ++++++++++++++++++++-------------------------
 1 file changed, 26 insertions(+), 34 deletions(-)

-- 
2.26.2

[PATCH 2/4] m68k/mac: Fix IOP status/control register writes

[Finn Thain]

When writing values to the IOP status/control register make sure those
values do not have any extraneous bits that will clear interrupt flags.

To place the SCC IOP into bypass mode would be desirable but this is not
achieved by writing IOP_DMAINACTIVE | IOP_RUN | IOP_AUTOINC | IOP_BYPASS
to the control register. Drop this ineffective register write.

Remove the flawed and unused iop_bypass() function. Make use of the
unused iop_stop() function.

Cc: Joshua Thompson <funaho@jurai.org>
Tested-by: Stan Johnson <userm57@yahoo.com>
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 arch/m68k/mac/iop.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/arch/m68k/mac/iop.c b/arch/m68k/mac/iop.c
index 22c4e2b8bdf2..5fc3b59ba811 100644
--- a/arch/m68k/mac/iop.c
+++ b/arch/m68k/mac/iop.c
@@ -183,7 +183,7 @@ static __inline__ void iop_writeb(volatile struct mac_iop *iop, __u16 addr, __u8
 
 static __inline__ void iop_stop(volatile struct mac_iop *iop)
 {
-	iop->status_ctrl &= ~IOP_RUN;
+	iop->status_ctrl = IOP_AUTOINC;
 }
 
 static __inline__ void iop_start(volatile struct mac_iop *iop)
@@ -191,14 +191,9 @@ static __inline__ void iop_start(volatile struct mac_iop *iop)
 	iop->status_ctrl = IOP_RUN | IOP_AUTOINC;
 }
 
-static __inline__ void iop_bypass(volatile struct mac_iop *iop)
-{
-	iop->status_ctrl |= IOP_BYPASS;
-}
-
 static __inline__ void iop_interrupt(volatile struct mac_iop *iop)
 {
-	iop->status_ctrl |= IOP_IRQ;
+	iop->status_ctrl = IOP_IRQ | IOP_RUN | IOP_AUTOINC;
 }
 
 static int iop_alive(volatile struct mac_iop *iop)
@@ -244,7 +239,6 @@ void __init iop_preinit(void)
 		} else {
 			iop_base[IOP_NUM_SCC] = (struct mac_iop *) SCC_IOP_BASE_QUADRA;
 		}
-		iop_base[IOP_NUM_SCC]->status_ctrl = 0x87;
 		iop_scc_present = 1;
 	} else {
 		iop_base[IOP_NUM_SCC] = NULL;
@@ -256,7 +250,7 @@ void __init iop_preinit(void)
 		} else {
 			iop_base[IOP_NUM_ISM] = (struct mac_iop *) ISM_IOP_BASE_QUADRA;
 		}
-		iop_base[IOP_NUM_ISM]->status_ctrl = 0;
+		iop_stop(iop_base[IOP_NUM_ISM]);
 		iop_ism_present = 1;
 	} else {
 		iop_base[IOP_NUM_ISM] = NULL;
-- 
2.26.2

[PATCH 3/4] m68k/mac: Don't send uninitialized data in IOP message reply

[Finn Thain]

Clear the message reply before calling iop_complete(). This code path is
not normally executed but should that happen let's arrange for consistent
behaviour from the IOP.

Cc: Joshua Thompson <funaho@jurai.org>
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 arch/m68k/mac/iop.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/m68k/mac/iop.c b/arch/m68k/mac/iop.c
index 5fc3b59ba811..8d6946edf2c8 100644
--- a/arch/m68k/mac/iop.c
+++ b/arch/m68k/mac/iop.c
@@ -449,6 +449,7 @@ static void iop_handle_recv(uint iop_num, uint chan)
 		iop_pr_debug("unclaimed message on iop_num %d chan %d\n",
 		             iop_num, chan);
 		iop_pr_debug("%*ph\n", IOP_MSG_LEN, msg->message);
+		memset(msg->reply, 0, IOP_MSG_LEN);
 		iop_complete_message(msg);
 	}
 }
-- 
2.26.2

[PATCH 4/4] m68k/mac: Improve IOP debug messages

[Finn Thain]

Always dump the full message and reply. Avoid printing partial lines
as this output gets mixed up with the output from called functions.
Don't output the state of idle channels.

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

diff --git a/arch/m68k/mac/iop.c b/arch/m68k/mac/iop.c
index 8d6946edf2c8..373a74c99356 100644
--- a/arch/m68k/mac/iop.c
+++ b/arch/m68k/mac/iop.c
@@ -347,8 +347,8 @@ void iop_complete_message(struct iop_msg *msg)
 	int chan = msg->channel;
 	int i,offset;
 
-	iop_pr_debug("msg %p iop_num %d channel %d\n", msg, msg->iop_num,
-	             msg->channel);
+	iop_pr_debug("iop_num %d chan %d reply %*ph\n",
+	             msg->iop_num, msg->channel, IOP_MSG_LEN, msg->reply);
 
 	offset = IOP_ADDR_RECV_MSG + (msg->channel * IOP_MSG_LEN);
 
@@ -372,6 +372,9 @@ static void iop_do_send(struct iop_msg *msg)
 	volatile struct mac_iop *iop = iop_base[msg->iop_num];
 	int i,offset;
 
+	iop_pr_debug("iop_num %d chan %d message %*ph\n",
+	             msg->iop_num, msg->channel, IOP_MSG_LEN, msg->message);
+
 	offset = IOP_ADDR_SEND_MSG + (msg->channel * IOP_MSG_LEN);
 
 	for (i = 0 ; i < IOP_MSG_LEN ; i++, offset++) {
@@ -394,8 +397,6 @@ static void iop_handle_send(uint iop_num, uint chan)
 	struct iop_msg *msg;
 	int i,offset;
 
-	iop_pr_debug("iop_num %d chan %d\n", iop_num, chan);
-
 	iop_writeb(iop, IOP_ADDR_SEND_STATE + chan, IOP_MSG_IDLE);
 
 	if (!(msg = iop_send_queue[iop_num][chan])) return;
@@ -405,6 +406,9 @@ static void iop_handle_send(uint iop_num, uint chan)
 	for (i = 0 ; i < IOP_MSG_LEN ; i++, offset++) {
 		msg->reply[i] = iop_readb(iop, offset);
 	}
+	iop_pr_debug("iop_num %d chan %d reply %*ph\n",
+	             iop_num, chan, IOP_MSG_LEN, msg->reply);
+
 	if (msg->handler) (*msg->handler)(msg);
 	msg->status = IOP_MSGSTATUS_UNUSED;
 	msg = msg->next;
@@ -424,8 +428,6 @@ static void iop_handle_recv(uint iop_num, uint chan)
 	int i,offset;
 	struct iop_msg *msg;
 
-	iop_pr_debug("iop_num %d chan %d\n", iop_num, chan);
-
 	msg = iop_get_unused_msg();
 	msg->iop_num = iop_num;
 	msg->channel = chan;
@@ -437,6 +439,8 @@ static void iop_handle_recv(uint iop_num, uint chan)
 	for (i = 0 ; i < IOP_MSG_LEN ; i++, offset++) {
 		msg->message[i] = iop_readb(iop, offset);
 	}
+	iop_pr_debug("iop_num %d chan %d message %*ph\n",
+	             iop_num, chan, IOP_MSG_LEN, msg->message);
 
 	iop_writeb(iop, IOP_ADDR_RECV_STATE + chan, IOP_MSG_RCVD);
 
@@ -446,9 +450,6 @@ static void iop_handle_recv(uint iop_num, uint chan)
 	if (msg->handler) {
 		(*msg->handler)(msg);
 	} else {
-		iop_pr_debug("unclaimed message on iop_num %d chan %d\n",
-		             iop_num, chan);
-		iop_pr_debug("%*ph\n", IOP_MSG_LEN, msg->message);
 		memset(msg->reply, 0, IOP_MSG_LEN);
 		iop_complete_message(msg);
 	}
@@ -559,35 +560,34 @@ irqreturn_t iop_ism_irq(int irq, void *dev_id)
 	int i,state;
 	u8 events = iop->status_ctrl & (IOP_INT0 | IOP_INT1);
 
-	iop_pr_debug("status %02X\n", iop->status_ctrl);
-
 	do {
+		iop_pr_debug("iop_num %d status %02X\n", iop_num,
+		             iop->status_ctrl);
+
 		/* INT0 indicates state change on an outgoing message channel */
 		if (events & IOP_INT0) {
 			iop->status_ctrl = IOP_INT0 | IOP_RUN | IOP_AUTOINC;
-			iop_pr_debug("new status %02X, send states",
-			             iop->status_ctrl);
 			for (i = 0; i < NUM_IOP_CHAN; i++) {
 				state = iop_readb(iop, IOP_ADDR_SEND_STATE + i);
-				iop_pr_cont(" %02X", state);
 				if (state == IOP_MSG_COMPLETE)
 					iop_handle_send(iop_num, i);
+				else if (state != IOP_MSG_IDLE)
+					iop_pr_debug("chan %d send state %02X\n",
+					             i, state);
 			}
-			iop_pr_cont("\n");
 		}
 
 		/* INT1 for incoming messages */
 		if (events & IOP_INT1) {
 			iop->status_ctrl = IOP_INT1 | IOP_RUN | IOP_AUTOINC;
-			iop_pr_debug("new status %02X, recv states",
-			             iop->status_ctrl);
 			for (i = 0; i < NUM_IOP_CHAN; i++) {
 				state = iop_readb(iop, IOP_ADDR_RECV_STATE + i);
-				iop_pr_cont(" %02X", state);
 				if (state == IOP_MSG_NEW)
 					iop_handle_recv(iop_num, i);
+				else if (state != IOP_MSG_IDLE)
+					iop_pr_debug("chan %d recv state %02X\n",
+					             i, state);
 			}
-			iop_pr_cont("\n");
 		}
 
 		events = iop->status_ctrl & (IOP_INT0 | IOP_INT1);
-- 
2.26.2

[PATCH 1/4] m68k/mac: Don't send IOP message until channel is idle

[Finn Thain]

In the following sequence of calls, iop_do_send() gets called when the
"send" channel is not in the IOP_MSG_IDLE state:

iop_ism_irq()
        iop_handle_send()
                (msg->handler)()
                        iop_send_message()
                iop_do_send()

Avoid this by testing the channel state before calling iop_do_send().

When sending, and iop_send_queue is empty, call iop_do_send() because
the channel is idle. If iop_send_queue is not empty, iop_do_send() will
get called later by iop_handle_send().

Cc: Joshua Thompson <funaho@jurai.org>
Tested-by: Stan Johnson <userm57@yahoo.com>
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 arch/m68k/mac/iop.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/m68k/mac/iop.c b/arch/m68k/mac/iop.c
index d99c7ea08d8c..22c4e2b8bdf2 100644
--- a/arch/m68k/mac/iop.c
+++ b/arch/m68k/mac/iop.c
@@ -415,7 +415,8 @@ static void iop_handle_send(uint iop_num, uint chan)
 	msg->status = IOP_MSGSTATUS_UNUSED;
 	msg = msg->next;
 	iop_send_queue[iop_num][chan] = msg;
-	if (msg) iop_do_send(msg);
+	if (msg && iop_readb(iop, IOP_ADDR_SEND_STATE + chan) == IOP_MSG_IDLE)
+		iop_do_send(msg);
 }
 
 /*
@@ -489,16 +490,12 @@ int iop_send_message(uint iop_num, uint chan, void *privdata,
 
 	if (!(q = iop_send_queue[iop_num][chan])) {
 		iop_send_queue[iop_num][chan] = msg;
+		iop_do_send(msg);
 	} else {
 		while (q->next) q = q->next;
 		q->next = msg;
 	}
 
-	if (iop_readb(iop_base[iop_num],
-	    IOP_ADDR_SEND_STATE + chan) == IOP_MSG_IDLE) {
-		iop_do_send(msg);
-	}
-
 	return 0;
 }
 
-- 
2.26.2

Re: [PATCH 0/4] Mac IOP driver fixes

[Geert Uytterhoeven]

Hi Finn,

On Sun, May 31, 2020 at 1:16 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> This patch series has several bug fixes for the IOP driver and some
> improvements to the debug level log messages.

Thanks for your series!

> Geert, please consider pushing these fixes for v5.8, if not the
> whole series.

I'm afraid it's a bit too late for that, as I expect the v5.8 merge window
to open tonight.  Unless the fix is for a regression in v5.7.
BTW, how does the issue being fixed manifest itself? That's not clear to
me from the patch description.

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

Re: [PATCH 0/4] Mac IOP driver fixes

[Finn Thain]

On Sun, 31 May 2020, Geert Uytterhoeven wrote:

> Hi Finn,
> 
> On Sun, May 31, 2020 at 1:16 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> 
> > This patch series has several bug fixes for the IOP driver and some 
> > improvements to the debug level log messages.
> 
> Thanks for your series!
> 

Thanks for your review and for your diligence in the performance of all of 
your duties as maintainer.

> > Geert, please consider pushing these fixes for v5.8, if not the whole 
> > series.
> 
> I'm afraid it's a bit too late for that, as I expect the v5.8 merge 
> window to open tonight.

Well, it's not that important.

> Unless the fix is for a regression in v5.7.

AFAICT these bugs are as old as the original driver. But I don't think 
that disqualifies them from backporting, which I plan to do that once they 
hit mainline.

> BTW, how does the issue being fixed manifest itself? That's not clear to 
> me from the patch description.
> 

The bugs in the iop driver were found by inspection, in the course of 
debugging adb-iop driver failures that Stan encountered. It's possible 
that the adb-iop driver is not badly affected by these bugs (I don't 
know). It's possible that the iop driver bugs are among the reasons why 
the swim_iop driver was never stabilized. That driver was removed in 
b21a323710e7 ("[PATCH] remove the broken BLK_DEV_SWIM_IOP driver").

Re: [PATCH 0/4] Mac IOP driver fixes

[Brad Boyer]

On Mon, Jun 01, 2020 at 10:05:05AM +1000, Finn Thain wrote:
> > BTW, how does the issue being fixed manifest itself? That's not clear to 
> > me from the patch description.
> > 
> 
> The bugs in the iop driver were found by inspection, in the course of 
> debugging adb-iop driver failures that Stan encountered. It's possible 
> that the adb-iop driver is not badly affected by these bugs (I don't 
> know). It's possible that the iop driver bugs are among the reasons why 
> the swim_iop driver was never stabilized. That driver was removed in 
> b21a323710e7 ("[PATCH] remove the broken BLK_DEV_SWIM_IOP driver").

The swim_iop driver was never completed. As far as I know, the data
transfer code was never written. I remember testing the driver, and
all it could do was detect the drives and do eject. Read and write
were not working. I looked into it at the time, but it's a little
messy and is much more complicated than the ADB driver. For ADB, the
data is small enough that it's packed in the IOP message. That's not
possible for SWIM, so it has a more complex data handling method. If
you look at the code of that driver and the messages, it looks like
the IOP can do DMA but it can't. The SWIM IOP message includes a buffer
pointer, but the IOP chip isn't connected in a way that it can just
access it (and it's getting passed a virtual address anyway). Based on
some of the other information I was given, there's a separate message
protocol on channel 0 (on each IOP) for bulk data transfer.

At the time, I had a diff to fix some bugs in the detection and handle
the status update messages. However, it hardly seemed worth submitting
that as a fix since the driver still wasn't useful without the bulk
data transfer working. I can pass my diff along if you intend to make
an attempt at a working swim_iop driver.

	Brad Boyer
	flar@allandria.com

Re: [PATCH 0/4] Mac IOP driver fixes

[Finn Thain]

On Sun, 31 May 2020, Brad Boyer wrote:

> 
> The swim_iop driver was never completed. As far as I know, the data 
> transfer code was never written. I remember testing the driver, and all 
> it could do was detect the drives and do eject. Read and write were not 
> working. I looked into it at the time, but it's a little messy and is 
> much more complicated than the ADB driver. For ADB, the data is small 
> enough that it's packed in the IOP message. That's not possible for 
> SWIM, so it has a more complex data handling method. If you look at the 
> code of that driver and the messages, it looks like the IOP can do DMA 
> but it can't. The SWIM IOP message includes a buffer pointer, but the 
> IOP chip isn't connected in a way that it can just access it (and it's 
> getting passed a virtual address anyway). Based on some of the other 
> information I was given, there's a separate message protocol on channel 
> 0 (on each IOP) for bulk data transfer.
> 
> At the time, I had a diff to fix some bugs in the detection and handle 
> the status update messages. However, it hardly seemed worth submitting 
> that as a fix since the driver still wasn't useful without the bulk data 
> transfer working. I can pass my diff along if you intend to make an 
> attempt at a working swim_iop driver.
> 

I'm hoping that the SWIM IOP can be used in bypass mode, so it could be 
used with the swim.c driver. I haven't been able to make that work yet.

You may want to send the patch to the mailing list anyway. Or maybe send 
it to the linux-mac68k bug tracker on sourceforge.

Re: [PATCH 0/4] Mac IOP driver fixes

[Brad Boyer]

On Tue, Jun 02, 2020 at 09:32:29AM +1000, Finn Thain wrote:
> I'm hoping that the SWIM IOP can be used in bypass mode, so it could be 
> used with the swim.c driver. I haven't been able to make that work yet.

Putting the IOP in bypass mode would have a couple issues. The obvious
one is that it will break ADB (and because the shift register attached
to the ADB transceiver is part of the IOP chip itself, there's not an
easy fix for that), but there's also some strangeness in the way the
SWIM chip is attached to the IOP chip that looks like it might break
a few things. In particular, it looks like they only enabled using the
SWIM chip in ISM mode and not IWM mode. The notes I have imply that
the normal Mac floppy driver didn't work on a IIfx even in bypass mode.
In particular, note the direct use of a GPIO line on VIA1 in swim_select
in drivers/block/swim.c.  That won't work on an IOP based system as that
input line to SWIM doesn't appear to be hooked up to anything that can
be accessed directly.

Here's the way it's put in _Guide to the Macintosh Family Hardware_
(Second Edition), on page 155:

"An IOP provides the state-control line SEL to the floppy disk drives.
Among other functions, this line selects which of the two heads is to
be used in a double-sided floppy disk drive."

The bit in VIA1 register A that is normally the "vHeadSel" line is
explicitly listed as "Reserved" on the IIfx.

It's definitely possible to access and drive the SWIM chip in bypass
mode, but that doesn't mean it's as transparent as for the SCC driver.
Apparently this external input line is not strictly required when the
SWIM chip is running in ISM mode. However, our driver appears to force
the chip into ISM mode and yet still depends on this input line.

> You may want to send the patch to the mailing list anyway. Or maybe send 
> it to the linux-mac68k bug tracker on sourceforge.

I've pasted it in here so there is a record of it. The fixes are to
properly handle multiple drives (my IIfx has two floppy drives installed)
and to initialize and handle the auto-polling of drive status properly.

	Brad Boyer
	flar@allandria.com

--- swim_iop.c-cvs	Fri Jul 19 22:49:23 2002
+++ swim_iop.c	Sat Jul 27 00:51:30 2002
@@ -5,12 +5,15 @@
  * Written by Joshua M. Thompson (funaho@jurai.org)
  * based on the SWIM3 driver (c) 1996 by Paul Mackerras.
  *
+ * Misc fixes by Brad A. Boyer (flar@allandria.com)
+ *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
  * as published by the Free Software Foundation; either version
  * 2 of the License, or (at your option) any later version.
  *
  * 1999-06-12 (jmt) - Initial implementation.
+ * 2002-07-26 (bab) - Fixed drive number storage and enabled status updates
  */
 
 /*
@@ -148,6 +151,7 @@
 int swimiop_init(void)
 {
 	volatile struct swim_iop_req req;
+	struct swimcmd_startpoll *poll = (struct swimcmd_startpoll *) &req.command[0];
 	struct swimcmd_status *cmd = (struct swimcmd_status *) &req.command[0];
 	struct swim_drvstatus *ds = &cmd->status;
 	struct floppy_state *fs;
@@ -175,6 +179,19 @@
 		return -EBUSY;
 	}
 
+	printk(KERN_ERR "SWIM_IOP: setting up automatic notification.\n");
+
+	swimiop_init_request(&req);
+	poll->code = CMD_START_POLL;
+	if (swimiop_send_request(&req) != 0) {
+		printk(KERN_ERR "SWIM_IOP: failed to request autonotify.\n");
+	} else {
+		while (!req.complete);
+		if (cmd->error != 0) {
+			printk(KERN_ERR "SWIM_IOP: notify setup failed.\n");
+		}
+	}
+
 	printk(KERN_ERR "SWIM_IOP: probing for installed drives.\n");
 
 	for (i = 0 ; i < MAX_FLOPPIES ; i++) {
@@ -199,6 +216,7 @@
 			ds->info.secondary? "secondary" : "primary");
 		swimiop_status_update(floppy_count, ds);
 		fs->state = idle;
+		fs->drive_num = i + 1;
 
 		init_timer(&fs->timeout);
 		floppy_count++;
@@ -214,6 +232,7 @@
 {
 	req->sent = 0;
 	req->complete = 0;
+	req->fs = NULL;
 	req->done = NULL;
 }
 
@@ -254,6 +273,26 @@
 }
 
 /*
+ * Find the correct status slot for given hardware drive number
+ *
+ * This is only used for unsolicited messages
+ *
+ */
+
+int swimiop_find_slot(int drive_num)
+{
+	struct floppy_state *fs;
+	int i;
+
+	for (i = 0 ; i < MAX_FLOPPIES ; i++) {
+		fs = &floppy_states[i];
+		if(fs->drive_num == drive_num)
+			return i;
+	}
+	return -1;
+}
+
+/*
  * Receive a SWIM message from the IOP.
  *
  * This will be called in two cases:
@@ -267,20 +306,32 @@
 	struct swim_iop_req *req;
 	struct swimmsg_status *sm;
 	struct swim_drvstatus *ds;
+	void	(*done)(struct swim_iop_req *);
+	int drive_slot;
 
 	req = current_req;
 
 	switch(msg->status) {
 		case IOP_MSGSTATUS_COMPLETE:
+			done = req->done;
 			memcpy(&req->command[0], &msg->reply[0], sizeof(req->command));
 			req->complete = 1;
-			if (req->done) (*req->done)(req);
+			if (done) done(req);
 			current_req = NULL;
 			break;
 		case IOP_MSGSTATUS_UNSOL:
 			sm = (struct swimmsg_status *) &msg->message[0];
 			ds = &sm->status;
-			swimiop_status_update(sm->drive_num, ds);
+			drive_slot = swimiop_find_slot(sm->drive_num);
+			if(drive_slot >= 0) {
+				printk(KERN_ERR "SWIM_IOP: unit %d changed.\n",
+					drive_slot);
+				swimiop_status_update(drive_slot, ds);
+			} else {
+				printk("SWIM-IOP: Bad message from drive %d\n",
+				       sm->drive_num);
+			}
+			memcpy(&msg->reply[0], &msg->message[0], IOP_MSG_LEN);
 			iop_complete_message(msg);
 			break;
 	}
@@ -347,7 +398,7 @@
 		schedule_timeout(1);
 	}
 	release_drive(fs);
-	return cmd->error;
+	return cmd->error ? -ENXIO : 0;
 }
 
 static ssize_t floppy_read(struct file *filp, char *buf,

Re: [PATCH 0/4] Mac IOP driver fixes

[Finn Thain]

On Mon, 1 Jun 2020, Brad Boyer wrote:

> On Tue, Jun 02, 2020 at 09:32:29AM +1000, Finn Thain wrote:
> > I'm hoping that the SWIM IOP can be used in bypass mode, so it could 
> > be used with the swim.c driver. I haven't been able to make that work 
> > yet.
> 
> Putting the IOP in bypass mode would have a couple issues. The obvious 
> one is that it will break ADB (and because the shift register attached 
> to the ADB transceiver is part of the IOP chip itself, there's not an 
> easy fix for that)

I see.

> but there's also some strangeness in the way the SWIM chip is attached 
> to the IOP chip that looks like it might break a few things. In 
> particular, it looks like they only enabled using the SWIM chip in ISM 
> mode and not IWM mode.

IIUC, swim.c only uses ISM mode.

> The notes I have imply that the normal Mac floppy driver didn't work on 
> a IIfx even in bypass mode.

I abandoned my failed attempt to put the SWIM IOP into bypass mode when I 
finally came across this text in the HW09 tech note:

    "Like the processor which controls floppy disk and ADB I/O, the IIfx 
    has another ASIC to control the SCC, but unlike the former, this 
    processor is capable of running in a special 'IOP Bypass' mode which 
    allows direct access to the SCC."

I suppose the problem being alluded to here is the one you mentioned: to 
put this IOP in bypass would kill ADB functionality (for the duration).

> In particular, note the direct use of a GPIO line on VIA1 in swim_select 
> in drivers/block/swim.c.  That won't work on an IOP based system as that 
> input line to SWIM doesn't appear to be hooked up to anything that can 
> be accessed directly.
> 
> Here's the way it's put in _Guide to the Macintosh Family Hardware_ 
> (Second Edition), on page 155:
> 
> "An IOP provides the state-control line SEL to the floppy disk drives. 
> Among other functions, this line selects which of the two heads is to be 
> used in a double-sided floppy disk drive."
> 
> The bit in VIA1 register A that is normally the "vHeadSel" line is 
> explicitly listed as "Reserved" on the IIfx.
> 

Yes, I was aware of that issue. But the GTMFH 2ed. figure 9-14 indicates 
that (on the IIfx) pin 12 in the drive connector is driven by the SWIM 
HDSEL pin instead of VIA1 Port A output. So, I wrote a patch to attempt to 
get the SWIM to drive this pin. But nothing worked because I never 
succeeded in putting the chip into bypass mode.

> It's definitely possible to access and drive the SWIM chip in bypass 
> mode, but that doesn't mean it's as transparent as for the SCC driver. 

OK.

BTW, I suspect there may be an issue with swim.c because it doesn't work 
on all models on which it should work. I know it works on Quadra 800, 650 
and a few others.

With regard to PowerBooks, we will need to power up the drive. And with 
regard to the IIfx, the A/UX iop.h header file indicates that, in bypass 
mode, the ISM registers have a 2-byte spacing (instead of 512-byte).

But I wish I knew why the driver doesn't work on an LC III, which 
supposedly has a SWIM 2, just like the Quadra 800.

> Apparently this external input line is not strictly required when the 
> SWIM chip is running in ISM mode. However, our driver appears to force 
> the chip into ISM mode and yet still depends on this input line.
> 

I can't comment on that. I don't really understand the ISM or IWM logic in 
any depth.

Re: [PATCH 0/4] Mac IOP driver fixes

[Brad Boyer]

On Tue, Jun 02, 2020 at 01:48:22PM +1000, Finn Thain wrote:
> On Mon, 1 Jun 2020, Brad Boyer wrote:
> > In particular, note the direct use of a GPIO line on VIA1 in swim_select 
> > in drivers/block/swim.c.  That won't work on an IOP based system as that 
> > input line to SWIM doesn't appear to be hooked up to anything that can 
> > be accessed directly.
> > 
> > Here's the way it's put in _Guide to the Macintosh Family Hardware_ 
> > (Second Edition), on page 155:
> > 
> > "An IOP provides the state-control line SEL to the floppy disk drives. 
> > Among other functions, this line selects which of the two heads is to be 
> > used in a double-sided floppy disk drive."
> > 
> > The bit in VIA1 register A that is normally the "vHeadSel" line is 
> > explicitly listed as "Reserved" on the IIfx.
> > 
> 
> Yes, I was aware of that issue. But the GTMFH 2ed. figure 9-14 indicates 
> that (on the IIfx) pin 12 in the drive connector is driven by the SWIM 
> HDSEL pin instead of VIA1 Port A output. So, I wrote a patch to attempt to 
> get the SWIM to drive this pin. But nothing worked because I never 
> succeeded in putting the chip into bypass mode.

There does appear to be a bit in the mode register to directly control
the SEL line to the drive. Nothing in our code appears to use it. You
would write HEDSEL to mode0 to clear the line, and write HEDSEL to
mode1 to set the line. However, it looks like the first bit in the
setup register controls if that line is input or output. Not sure
why we have it named S_INV_WDATA, but the state machine to the actual
floppy drive is way beyond my comprehension. I'll note that the swim3.c
driver uses the SELECT bit in the control register to do the same
basic thing (and it's the same bit value as HEDSEL in swim.c).

I know we at least used to have a problem where the SCC ports only
worked if they were already in bypass mode. There was something we
weren't doing right about setting the bypass mode. I'm not sure if
we ever fixed it. If you turn off the "Compatible Mode" in the
Serial Switch control panel, does the Linux kernel SCC driver still
work afterwards? I know that was a problem at one point.

> With regard to PowerBooks, we will need to power up the drive. And with 
> regard to the IIfx, the A/UX iop.h header file indicates that, in bypass 
> mode, the ISM registers have a 2-byte spacing (instead of 512-byte).

The notes I have indicate the SWIM registers are at the same spacing as
the SCC registers on the other IOP. That's definitely not 512-byte, so
I presume it's 2-byte spacing as that's what the SCC driver looks like
it does.

> But I wish I knew why the driver doesn't work on an LC III, which 
> supposedly has a SWIM 2, just like the Quadra 800.

If I had to guess, the select line isn't in the same place. For example,
on the Mac Portable (which obviously isn't supported in Linux) that line
is apparently in VIA2 register B (as are most of the other bits that
normally show up in register A). Based on looking at the driver, being
unable to drive that line to the disk drive would break lots of stuff,
including the detection of the disk inserted in the drive.

We may need to map which models have this line hooked up where. I
wouldn't be surprised if it's only on VIA1 on some models and only
on the SWIM chip on others. It sounds like IWM didn't even know
about that line, which is why it was attached to the VIA in the
original mac models (on the Macintosh SE, some of them had an IWM
chip and some had the original version of SWIM - it was even
available as an upgrade to get the new chip and ROMs).

> > Apparently this external input line is not strictly required when the 
> > SWIM chip is running in ISM mode. However, our driver appears to force 
> > the chip into ISM mode and yet still depends on this input line.
> > 
> 
> I can't comment on that. I don't really understand the ISM or IWM logic in 
> any depth.

I can't claim to fully understand it, but I can figure out bits and pieces.

	Brad Boyer
	flar@allandria.com

Re: [PATCH 0/4] Mac IOP driver fixes

[Finn Thain]

On Wed, 3 Jun 2020, Brad Boyer wrote:

> On Tue, Jun 02, 2020 at 01:48:22PM +1000, Finn Thain wrote:
> > 
> > Yes, I was aware of that issue. But the GTMFH 2ed. figure 9-14 
> > indicates that (on the IIfx) pin 12 in the drive connector is driven 
> > by the SWIM HDSEL pin instead of VIA1 Port A output. So, I wrote a 
> > patch to attempt to get the SWIM to drive this pin. But nothing worked 
> > because I never succeeded in putting the chip into bypass mode.
> 
> There does appear to be a bit in the mode register to directly control 
> the SEL line to the drive. Nothing in our code appears to use it. You 
> would write HEDSEL to mode0 to clear the line, and write HEDSEL to mode1 
> to set the line. However, it looks like the first bit in the setup 
> register controls if that line is input or output. Not sure why we have 
> it named S_INV_WDATA, but the state machine to the actual floppy drive 
> is way beyond my comprehension. I'll note that the swim3.c driver uses 
> the SELECT bit in the control register to do the same basic thing (and 
> it's the same bit value as HEDSEL in swim.c).
> 

I think that agrees with the patch I wrote a couple of years ago, back 
when I was working on this problem:

diff --git a/arch/m68k/mac/iop.c b/arch/m68k/mac/iop.c
index 392e57c2e5ea..ea77b983ccb9 100644
--- a/arch/m68k/mac/iop.c
+++ b/arch/m68k/mac/iop.c
@@ -130,6 +130,7 @@
 /* Non-zero if the IOPs are present */
 
 int iop_scc_present, iop_ism_present;
+EXPORT_SYMBOL(iop_ism_present);
 
 /* structure for tracking channel listeners */
 
diff --git a/drivers/block/swim.c b/drivers/block/swim.c
index 0e31884a9519..883ff1bc1ea0 100644
--- a/drivers/block/swim.c
+++ b/drivers/block/swim.c
@@ -27,6 +27,7 @@
 #include <linux/platform_device.h>
 
 #include <asm/mac_via.h>
+#include <asm/mac_iop.h>
 
 #define CARDNAME "swim"
 
@@ -135,7 +136,8 @@ struct iwm {
 
 /* bits in setup register */
 
-#define S_INV_WDATA	0x01
+#define S_Q3_OUTPUT	0x01 /* SWIM */
+#define S_INV_WDATA	0x01 /* SWIM 2 */
 #define S_3_5_SELECT	0x02
 #define S_GCR		0x04
 #define S_FCLK_DIV2	0x08
@@ -273,8 +275,13 @@ static inline void swim_select(struct swim __iomem *base, int sel)
 {
 	swim_write(base, phase, RELAX);
 
-	via1_set_head(sel & 0x100);
-
+	if (iop_ism_present) {
+		if (sel & 0x100)
+			swim_write(base, mode1, HEDSEL);
+		else
+			swim_write(base, mode0, HEDSEL);
+	} else
+		via1_set_head(sel & 0x100);
 	swim_write(base, phase, sel & CA_MASK);
 }
 
@@ -646,7 +653,12 @@ static int floppy_open(struct block_device *bdev, fmode_t mode)
 	else
 		fs->ref_count++;
 
-	swim_write(base, setup, S_IBM_DRIVE  | S_FCLK_DIV2);
+	if (iop_ism_present)
+		swim_write(base, setup, S_IBM_DRIVE | S_FCLK_DIV2 |
+					S_Q3_OUTPUT);
+	else
+		swim_write(base, setup, S_IBM_DRIVE | S_FCLK_DIV2);
+
 	udelay(10);
 	swim_drive(base, fs->location);
 	swim_motor(base, ON);

> I know we at least used to have a problem where the SCC ports only 
> worked if they were already in bypass mode. There was something we 
> weren't doing right about setting the bypass mode. I'm not sure if we 
> ever fixed it. If you turn off the "Compatible Mode" in the Serial 
> Switch control panel, does the Linux kernel SCC driver still work 
> afterwards? I know that was a problem at one point.
> 

Yes, it's still a problem. This relates directly to patch 2/4, so I asked 
Stan to confirm this before I sent the present patch series. Apparently 
the IOP_BYPASS bit is read-only. To effect a switch to bypass mode I 
believe that it is necessary to send the right message to the IOP kernel.

> > With regard to PowerBooks, we will need to power up the drive. And 
> > with regard to the IIfx, the A/UX iop.h header file indicates that, in 
> > bypass mode, the ISM registers have a 2-byte spacing (instead of 
> > 512-byte).
> 
> The notes I have indicate the SWIM registers are at the same spacing as 
> the SCC registers on the other IOP. That's definitely not 512-byte, so I 
> presume it's 2-byte spacing as that's what the SCC driver looks like it 
> does.
> 
> > But I wish I knew why the driver doesn't work on an LC III, which 
> > supposedly has a SWIM 2, just like the Quadra 800.
> 
> If I had to guess, the select line isn't in the same place. For example, 
> on the Mac Portable (which obviously isn't supported in Linux) that line 
> is apparently in VIA2 register B (as are most of the other bits that 
> normally show up in register A). Based on looking at the driver, being 
> unable to drive that line to the disk drive would break lots of stuff, 
> including the detection of the disk inserted in the drive.
> 

OK. I see that figure 9-11 has these details. I never noticed that, being 
that the Portable seemed to have no relevance.

We will have to experiment further to find out whether setting this bit 
has any effect on the LC III.

Re: [PATCH 0/4] Mac IOP driver fixes

[Brad Boyer]

On Thu, Jun 04, 2020 at 02:49:24PM +1000, Finn Thain wrote:
> On Wed, 3 Jun 2020, Brad Boyer wrote:
> > On Tue, Jun 02, 2020 at 01:48:22PM +1000, Finn Thain wrote:
> > > 
> > There does appear to be a bit in the mode register to directly control 
> > the SEL line to the drive. Nothing in our code appears to use it. You 
> > would write HEDSEL to mode0 to clear the line, and write HEDSEL to mode1 
> > to set the line. However, it looks like the first bit in the setup 
> > register controls if that line is input or output. Not sure why we have 
> > it named S_INV_WDATA, but the state machine to the actual floppy drive 
> > is way beyond my comprehension. I'll note that the swim3.c driver uses 
> > the SELECT bit in the control register to do the same basic thing (and 
> > it's the same bit value as HEDSEL in swim.c).
> > 
> 
> I think that agrees with the patch I wrote a couple of years ago, back 
> when I was working on this problem:

Yes, that seems to fit with my understanding. In the pinout list I have
for the original SWIM chip, one line is called /Q3/HEDSEL. The pinout
specifically says it can be configured to either be an input (Q3) or
an output (HEDSEL). However, there's another pin in the list that is
simply listed as HEDSEL. Not sure why they would have had the same
output on two separate pins.

> > I know we at least used to have a problem where the SCC ports only 
> > worked if they were already in bypass mode. There was something we 
> > weren't doing right about setting the bypass mode. I'm not sure if we 
> > ever fixed it. If you turn off the "Compatible Mode" in the Serial 
> > Switch control panel, does the Linux kernel SCC driver still work 
> > afterwards? I know that was a problem at one point.
> > 
> 
> Yes, it's still a problem. This relates directly to patch 2/4, so I asked 
> Stan to confirm this before I sent the present patch series. Apparently 
> the IOP_BYPASS bit is read-only. To effect a switch to bypass mode I 
> believe that it is necessary to send the right message to the IOP kernel.

OK. Yes, I have a list of structures and one of them appears to be the
bypass command. The outbound message is 3 bytes (the command code:4, a
byte for a flag with 0 meaning off and 1 meaning on, and an ID byte).
The reply is a byte of error (0 is success) followed by extra bytes
depending on the error value.

One of the listed error values is "driver in use", so it's possible
it will reject the request. We are just taking advantage of the
existing drivers, so probably both the SWIM and ADB drivers are
still running on the IOP.

It looks like shutting down a driver is another command. The command
byte for that is 2, and the second byte is which driver to stop (0/1).
I'm not sure if there's a way to restart the driver after that other
than reloading everything.

> > > But I wish I knew why the driver doesn't work on an LC III, which 
> > > supposedly has a SWIM 2, just like the Quadra 800.
> > 
> > If I had to guess, the select line isn't in the same place. For example, 
> > on the Mac Portable (which obviously isn't supported in Linux) that line 
> > is apparently in VIA2 register B (as are most of the other bits that 
> > normally show up in register A). Based on looking at the driver, being 
> > unable to drive that line to the disk drive would break lots of stuff, 
> > including the detection of the disk inserted in the drive.
> > 
> 
> OK. I see that figure 9-11 has these details. I never noticed that, being 
> that the Portable seemed to have no relevance.
> 
> We will have to experiment further to find out whether setting this bit 
> has any effect on the LC III.

Thinking of those figures, I'll note that Figure 9-13 specifically
shows HDSEL on the SWIM chip on the IIcx/IIci as not connected while
the VIA1 PA5 output line is connected to the pin on the drive cable.
The IIfx version in Figure 9-14 shows the HDSEL line on the SWIM
chip as the only thing connected to the same pin on the drive cable.

Based on some of the other hints I've read, actually connecting
HDSEL on the SWIM chip to the cable instead of having it accessible
directly by the CPU makes it impossible to use the SWIM in IWM mode.
Perhaps in some later models they gave up and decided to just always
use it in ISM mode so they didn't need to use up a pin on the VIA.

	Brad Boyer
	flar@allandria.com

Re: [PATCH 0/4] Mac IOP driver fixes

[Finn Thain]

On Thu, 4 Jun 2020, Brad Boyer wrote:

> I have a list of structures and one of them appears to be the bypass 
> command. The outbound message is 3 bytes (the command code:4, a byte for 
> a flag with 0 meaning off and 1 meaning on, and an ID byte). The reply 
> is a byte of error (0 is success) followed by extra bytes depending on 
> the error value.
> 
> One of the listed error values is "driver in use", so it's possible it 
> will reject the request. We are just taking advantage of the existing 
> drivers, so probably both the SWIM and ADB drivers are still running on 
> the IOP.
> 
> It looks like shutting down a driver is another command. The command 
> byte for that is 2, and the second byte is which driver to stop (0/1). 
> I'm not sure if there's a way to restart the driver after that other 
> than reloading everything.
> 

I think we need to be able to restart the IOP's internal ADB driver. 
Otherwise it's impossible to have ADB input resume when the floppy drives 
go idle for a while or when the swim module exits.

The alternative is to revive swip_iop.c, and the downside there is that 
the block layer API has completely changed, meaning that the driver needs 
a rewrite on the block layer side, as well as needing more work on the IOP 
protocol side.

Re: [PATCH 0/4] Mac IOP driver fixes

[Finn Thain]

On Fri, 5 Jun 2020, Finn Thain wrote:

> On Thu, 4 Jun 2020, Brad Boyer wrote:
> 
> > I have a list of structures and one of them appears to be the bypass 
> > command. The outbound message is 3 bytes (the command code:4, a byte 
> > for a flag with 0 meaning off and 1 meaning on, and an ID byte). The 
> > reply is a byte of error (0 is success) followed by extra bytes 
> > depending on the error value.
> > 
> > One of the listed error values is "driver in use", so it's possible it 
> > will reject the request. We are just taking advantage of the existing 
> > drivers, so probably both the SWIM and ADB drivers are still running 
> > on the IOP.
> > 
> > It looks like shutting down a driver is another command. The command 
> > byte for that is 2, and the second byte is which driver to stop (0/1). 
> > I'm not sure if there's a way to restart the driver after that other 
> > than reloading everything.
> > 
> 
> I think we need to be able to restart the IOP's internal ADB driver. 
> Otherwise it's impossible to have ADB input resume when the floppy 
> drives go idle for a while or when the swim module exits.
> 
> The alternative is to revive swip_iop.c, and the downside there is that 
> the block layer API has completely changed, meaning that the driver 
> needs a rewrite on the block layer side, as well as needing more work on 
> the IOP protocol side.
> 

I should also mention that swim.c has some pretty serious limitations: no 
write support and no GCR support. These features could be added much more 
easily to swim_iop.c than swim.c because the IOP supports them internally.

Re: [PATCH 0/4] Mac IOP driver fixes

[Brad Boyer]

On Fri, Jun 05, 2020 at 02:23:50PM +1000, Finn Thain wrote:
> On Fri, 5 Jun 2020, Finn Thain wrote:
> > I think we need to be able to restart the IOP's internal ADB driver. 
> > Otherwise it's impossible to have ADB input resume when the floppy 
> > drives go idle for a while or when the swim module exits.
> > 
> > The alternative is to revive swip_iop.c, and the downside there is that 
> > the block layer API has completely changed, meaning that the driver 
> > needs a rewrite on the block layer side, as well as needing more work on 
> > the IOP protocol side.
> > 
> 
> I should also mention that swim.c has some pretty serious limitations: no 
> write support and no GCR support. These features could be added much more 
> easily to swim_iop.c than swim.c because the IOP supports them internally.

With all of that, it sounds like it would be easier to bring back and
fix the swim_iop driver in spite of the need to update the interface
to the kernel. I don't know when I might be able to get my IIfx or Q950
back out and running, but I'll see if I can take a look at the code
even without having the hardware ready to actually test it.

Ideally we would have the code to actually do a full reset on an IOP
and bootstrap the IOP kernel and each IOP driver. I suspect the images
for them are somewhere in the ROM. That's likely only necessary if we
start doing a boot where the MacOS hasn't already loaded fully.

	Brad Boyer
	flar@allandria.com

Re: [PATCH 0/4] Mac IOP driver fixes

[Geert Uytterhoeven]

On Sun, May 31, 2020 at 10:41 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Sun, May 31, 2020 at 1:16 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> > This patch series has several bug fixes for the IOP driver and some
> > improvements to the debug level log messages.
>
> Thanks for your series!
>
> > Geert, please consider pushing these fixes for v5.8, if not the
> > whole series.
>
> I'm afraid it's a bit too late for that, as I expect the v5.8 merge window
> to open tonight.  Unless the fix is for a regression in v5.7.

Queued in the m68k for-v5.9 branch.

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