* [PATCH 7/9] macintosh/via-macii: Use unsigned type for autopoll_devs variable
2020-06-28 4:23 [PATCH 0/9] Macintosh II ADB driver fixes Finn Thain
@ 2020-06-28 4:23 ` Finn Thain
2020-06-28 4:23 ` [PATCH 2/9] macintosh/via-macii: Poll the device most likely to respond Finn Thain
` (8 subsequent siblings)
9 siblings, 0 replies; 16+ messages in thread
From: Finn Thain @ 2020-06-28 4:23 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Joshua Thompson, Geert Uytterhoeven, linux-m68k, Laurent Vivier,
Mark Cave-Ayland, linuxppc-dev, linux-kernel
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
drivers/macintosh/via-macii.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/macintosh/via-macii.c b/drivers/macintosh/via-macii.c
index e143ddb81de34..447273967e1e8 100644
--- a/drivers/macintosh/via-macii.c
+++ b/drivers/macintosh/via-macii.c
@@ -125,7 +125,7 @@ static bool srq_asserted; /* have to poll for the device that asserted it */
static u8 last_cmd; /* the most recent command byte transmitted */
static u8 last_talk_cmd; /* the most recent Talk command byte transmitted */
static u8 last_poll_cmd; /* the most recent Talk R0 command byte transmitted */
-static int autopoll_devs; /* bits set are device addresses to be polled */
+static unsigned int autopoll_devs; /* bits set are device addresses to poll */
/* Check for MacII style ADB */
static int macii_probe(void)
@@ -291,7 +291,7 @@ static int macii_autopoll(int devs)
local_irq_save(flags);
/* bit 1 == device 1, and so on. */
- autopoll_devs = devs & 0xFFFE;
+ autopoll_devs = (unsigned int)devs & 0xFFFE;
if (!current_req) {
macii_queue_poll();
--
2.26.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 2/9] macintosh/via-macii: Poll the device most likely to respond
2020-06-28 4:23 [PATCH 0/9] Macintosh II ADB driver fixes Finn Thain
2020-06-28 4:23 ` [PATCH 7/9] macintosh/via-macii: Use unsigned type for autopoll_devs variable Finn Thain
@ 2020-06-28 4:23 ` Finn Thain
2020-08-09 18:55 ` Guenter Roeck
2020-06-28 4:23 ` [PATCH 6/9] macintosh/via-macii: Use bool type for reading_reply variable Finn Thain
` (7 subsequent siblings)
9 siblings, 1 reply; 16+ messages in thread
From: Finn Thain @ 2020-06-28 4:23 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Joshua Thompson, Geert Uytterhoeven, linux-m68k, Laurent Vivier,
Mark Cave-Ayland, linuxppc-dev, linux-kernel
Poll the most recently polled device by default, rather than the lowest
device address that happens to be enabled in autopoll_devs. This improves
input latency. Re-use macii_queue_poll() rather than duplicate that logic.
This eliminates a static struct and function.
Fixes: d95fd5fce88f0 ("m68k: Mac II ADB fixes") # v5.0+
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
drivers/macintosh/via-macii.c | 99 +++++++++++++++++++----------------
1 file changed, 53 insertions(+), 46 deletions(-)
diff --git a/drivers/macintosh/via-macii.c b/drivers/macintosh/via-macii.c
index 6aa903529570d..d4f1a65c5f1fd 100644
--- a/drivers/macintosh/via-macii.c
+++ b/drivers/macintosh/via-macii.c
@@ -77,6 +77,10 @@ static volatile unsigned char *via;
#define ST_ODD 0x20 /* ADB state: odd data byte */
#define ST_IDLE 0x30 /* ADB state: idle, nothing to send */
+/* ADB command byte structure */
+#define ADDR_MASK 0xF0
+#define CMD_MASK 0x0F
+
static int macii_init_via(void);
static void macii_start(void);
static irqreturn_t macii_interrupt(int irq, void *arg);
@@ -117,7 +121,8 @@ static int reply_len; /* number of bytes received in reply_buf or req->reply */
static int status; /* VIA's ADB status bits captured upon interrupt */
static int last_status; /* status bits as at previous interrupt */
static int srq_asserted; /* have to poll for the device that asserted it */
-static int command_byte; /* the most recent command byte transmitted */
+static u8 last_cmd; /* the most recent command byte transmitted */
+static u8 last_poll_cmd; /* the most recent Talk R0 command byte transmitted */
static int autopoll_devs; /* bits set are device addresses to be polled */
/* Check for MacII style ADB */
@@ -179,35 +184,49 @@ static int macii_init_via(void)
/* Send an ADB poll (Talk Register 0 command prepended to the request queue) */
static void macii_queue_poll(void)
{
- /* No point polling the active device as it will never assert SRQ, so
- * poll the next device in the autopoll list. This could leave us
- * stuck in a polling loop if an unprobed device is asserting SRQ.
- * In theory, that could only happen if a device was plugged in after
- * probing started. Unplugging it again will break the cycle.
- * (Simply polling the next higher device often ends up polling almost
- * every device (after wrapping around), which takes too long.)
- */
- int device_mask;
- int next_device;
static struct adb_request req;
+ unsigned char poll_command;
+ unsigned int poll_addr;
+ /* This only polls devices in the autopoll list, which assumes that
+ * unprobed devices never assert SRQ. That could happen if a device was
+ * plugged in after the adb bus scan. Unplugging it again will resolve
+ * the problem. This behaviour is similar to MacOS.
+ */
if (!autopoll_devs)
return;
- device_mask = (1 << (((command_byte & 0xF0) >> 4) + 1)) - 1;
- if (autopoll_devs & ~device_mask)
- next_device = ffs(autopoll_devs & ~device_mask) - 1;
- else
- next_device = ffs(autopoll_devs) - 1;
+ /* The device most recently polled may not be the best device to poll
+ * right now. Some other device(s) may have signalled SRQ (the active
+ * device won't do that). Or the autopoll list may have been changed.
+ * Try polling the next higher address.
+ */
+ poll_addr = (last_poll_cmd & ADDR_MASK) >> 4;
+ if ((srq_asserted && last_cmd == last_poll_cmd) ||
+ !(autopoll_devs & (1 << poll_addr))) {
+ unsigned int higher_devs;
+
+ higher_devs = autopoll_devs & -(1 << (poll_addr + 1));
+ poll_addr = ffs(higher_devs ? higher_devs : autopoll_devs) - 1;
+ }
- adb_request(&req, NULL, ADBREQ_NOSEND, 1, ADB_READREG(next_device, 0));
+ /* Send a Talk Register 0 command */
+ poll_command = ADB_READREG(poll_addr, 0);
+
+ /* No need to repeat this Talk command. The transceiver will do that
+ * as long as it is idle.
+ */
+ if (poll_command == last_cmd)
+ return;
+
+ adb_request(&req, NULL, ADBREQ_NOSEND, 1, poll_command);
req.sent = 0;
req.complete = 0;
req.reply_len = 0;
req.next = current_req;
- if (current_req != NULL) {
+ if (WARN_ON(current_req)) {
current_req = &req;
} else {
current_req = &req;
@@ -266,37 +285,22 @@ static int macii_write(struct adb_request *req)
/* Start auto-polling */
static int macii_autopoll(int devs)
{
- static struct adb_request req;
unsigned long flags;
- int err = 0;
local_irq_save(flags);
/* bit 1 == device 1, and so on. */
autopoll_devs = devs & 0xFFFE;
- if (autopoll_devs && !current_req) {
- /* Send a Talk Reg 0. The controller will repeatedly transmit
- * this as long as it is idle.
- */
- adb_request(&req, NULL, ADBREQ_NOSEND, 1,
- ADB_READREG(ffs(autopoll_devs) - 1, 0));
- err = macii_write(&req);
+ if (!current_req) {
+ macii_queue_poll();
+ if (current_req && macii_state == idle)
+ macii_start();
}
local_irq_restore(flags);
- return err;
-}
-static inline int need_autopoll(void)
-{
- /* Was the last command Talk Reg 0
- * and is the target on the autopoll list?
- */
- if ((command_byte & 0x0F) == 0x0C &&
- ((1 << ((command_byte & 0xF0) >> 4)) & autopoll_devs))
- return 0;
- return 1;
+ return 0;
}
/* Prod the chip without interrupts */
@@ -333,7 +337,12 @@ static void macii_start(void)
*/
/* store command byte */
- command_byte = req->data[1];
+ last_cmd = req->data[1];
+
+ /* If this is a Talk Register 0 command, store the command byte */
+ if ((last_cmd & CMD_MASK) == ADB_READREG(0, 0))
+ last_poll_cmd = last_cmd;
+
/* Output mode */
via[ACR] |= SR_OUT;
/* Load data */
@@ -424,10 +433,11 @@ static irqreturn_t macii_interrupt(int irq, void *arg)
if (req->done)
(*req->done)(req);
- if (current_req)
+ if (!current_req)
+ macii_queue_poll();
+
+ if (current_req && macii_state == idle)
macii_start();
- else if (need_autopoll())
- macii_autopoll(autopoll_devs);
}
if (macii_state == idle) {
@@ -507,14 +517,11 @@ static irqreturn_t macii_interrupt(int irq, void *arg)
macii_state = idle;
- /* SRQ seen before, initiate poll now */
- if (srq_asserted)
+ if (!current_req)
macii_queue_poll();
if (current_req)
macii_start();
- else if (need_autopoll())
- macii_autopoll(autopoll_devs);
if (macii_state == idle)
via[B] = (via[B] & ~ST_MASK) | ST_IDLE;
--
2.26.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 2/9] macintosh/via-macii: Poll the device most likely to respond
2020-06-28 4:23 ` [PATCH 2/9] macintosh/via-macii: Poll the device most likely to respond Finn Thain
@ 2020-08-09 18:55 ` Guenter Roeck
2020-08-09 22:58 ` Finn Thain
0 siblings, 1 reply; 16+ messages in thread
From: Guenter Roeck @ 2020-08-09 18:55 UTC (permalink / raw)
To: Finn Thain
Cc: Benjamin Herrenschmidt, Joshua Thompson, Geert Uytterhoeven,
linux-m68k, Laurent Vivier, Mark Cave-Ayland, linuxppc-dev,
linux-kernel
Hi,
On Sun, Jun 28, 2020 at 02:23:12PM +1000, Finn Thain wrote:
> Poll the most recently polled device by default, rather than the lowest
> device address that happens to be enabled in autopoll_devs. This improves
> input latency. Re-use macii_queue_poll() rather than duplicate that logic.
> This eliminates a static struct and function.
>
> Fixes: d95fd5fce88f0 ("m68k: Mac II ADB fixes") # v5.0+
> Tested-by: Stan Johnson <userm57@yahoo.com>
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
With this patch applied, the qemu "q800" emulation no longer works and is stuck
in early boot. Any idea why that might be the case, and/or how to debug it ?
Thanks,
Guenter
> ---
> drivers/macintosh/via-macii.c | 99 +++++++++++++++++++----------------
> 1 file changed, 53 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/macintosh/via-macii.c b/drivers/macintosh/via-macii.c
> index 6aa903529570d..d4f1a65c5f1fd 100644
> --- a/drivers/macintosh/via-macii.c
> +++ b/drivers/macintosh/via-macii.c
> @@ -77,6 +77,10 @@ static volatile unsigned char *via;
> #define ST_ODD 0x20 /* ADB state: odd data byte */
> #define ST_IDLE 0x30 /* ADB state: idle, nothing to send */
>
> +/* ADB command byte structure */
> +#define ADDR_MASK 0xF0
> +#define CMD_MASK 0x0F
> +
> static int macii_init_via(void);
> static void macii_start(void);
> static irqreturn_t macii_interrupt(int irq, void *arg);
> @@ -117,7 +121,8 @@ static int reply_len; /* number of bytes received in reply_buf or req->reply */
> static int status; /* VIA's ADB status bits captured upon interrupt */
> static int last_status; /* status bits as at previous interrupt */
> static int srq_asserted; /* have to poll for the device that asserted it */
> -static int command_byte; /* the most recent command byte transmitted */
> +static u8 last_cmd; /* the most recent command byte transmitted */
> +static u8 last_poll_cmd; /* the most recent Talk R0 command byte transmitted */
> static int autopoll_devs; /* bits set are device addresses to be polled */
>
> /* Check for MacII style ADB */
> @@ -179,35 +184,49 @@ static int macii_init_via(void)
> /* Send an ADB poll (Talk Register 0 command prepended to the request queue) */
> static void macii_queue_poll(void)
> {
> - /* No point polling the active device as it will never assert SRQ, so
> - * poll the next device in the autopoll list. This could leave us
> - * stuck in a polling loop if an unprobed device is asserting SRQ.
> - * In theory, that could only happen if a device was plugged in after
> - * probing started. Unplugging it again will break the cycle.
> - * (Simply polling the next higher device often ends up polling almost
> - * every device (after wrapping around), which takes too long.)
> - */
> - int device_mask;
> - int next_device;
> static struct adb_request req;
> + unsigned char poll_command;
> + unsigned int poll_addr;
>
> + /* This only polls devices in the autopoll list, which assumes that
> + * unprobed devices never assert SRQ. That could happen if a device was
> + * plugged in after the adb bus scan. Unplugging it again will resolve
> + * the problem. This behaviour is similar to MacOS.
> + */
> if (!autopoll_devs)
> return;
>
> - device_mask = (1 << (((command_byte & 0xF0) >> 4) + 1)) - 1;
> - if (autopoll_devs & ~device_mask)
> - next_device = ffs(autopoll_devs & ~device_mask) - 1;
> - else
> - next_device = ffs(autopoll_devs) - 1;
> + /* The device most recently polled may not be the best device to poll
> + * right now. Some other device(s) may have signalled SRQ (the active
> + * device won't do that). Or the autopoll list may have been changed.
> + * Try polling the next higher address.
> + */
> + poll_addr = (last_poll_cmd & ADDR_MASK) >> 4;
> + if ((srq_asserted && last_cmd == last_poll_cmd) ||
> + !(autopoll_devs & (1 << poll_addr))) {
> + unsigned int higher_devs;
> +
> + higher_devs = autopoll_devs & -(1 << (poll_addr + 1));
> + poll_addr = ffs(higher_devs ? higher_devs : autopoll_devs) - 1;
> + }
>
> - adb_request(&req, NULL, ADBREQ_NOSEND, 1, ADB_READREG(next_device, 0));
> + /* Send a Talk Register 0 command */
> + poll_command = ADB_READREG(poll_addr, 0);
> +
> + /* No need to repeat this Talk command. The transceiver will do that
> + * as long as it is idle.
> + */
> + if (poll_command == last_cmd)
> + return;
> +
> + adb_request(&req, NULL, ADBREQ_NOSEND, 1, poll_command);
>
> req.sent = 0;
> req.complete = 0;
> req.reply_len = 0;
> req.next = current_req;
>
> - if (current_req != NULL) {
> + if (WARN_ON(current_req)) {
> current_req = &req;
> } else {
> current_req = &req;
> @@ -266,37 +285,22 @@ static int macii_write(struct adb_request *req)
> /* Start auto-polling */
> static int macii_autopoll(int devs)
> {
> - static struct adb_request req;
> unsigned long flags;
> - int err = 0;
>
> local_irq_save(flags);
>
> /* bit 1 == device 1, and so on. */
> autopoll_devs = devs & 0xFFFE;
>
> - if (autopoll_devs && !current_req) {
> - /* Send a Talk Reg 0. The controller will repeatedly transmit
> - * this as long as it is idle.
> - */
> - adb_request(&req, NULL, ADBREQ_NOSEND, 1,
> - ADB_READREG(ffs(autopoll_devs) - 1, 0));
> - err = macii_write(&req);
> + if (!current_req) {
> + macii_queue_poll();
> + if (current_req && macii_state == idle)
> + macii_start();
> }
>
> local_irq_restore(flags);
> - return err;
> -}
>
> -static inline int need_autopoll(void)
> -{
> - /* Was the last command Talk Reg 0
> - * and is the target on the autopoll list?
> - */
> - if ((command_byte & 0x0F) == 0x0C &&
> - ((1 << ((command_byte & 0xF0) >> 4)) & autopoll_devs))
> - return 0;
> - return 1;
> + return 0;
> }
>
> /* Prod the chip without interrupts */
> @@ -333,7 +337,12 @@ static void macii_start(void)
> */
>
> /* store command byte */
> - command_byte = req->data[1];
> + last_cmd = req->data[1];
> +
> + /* If this is a Talk Register 0 command, store the command byte */
> + if ((last_cmd & CMD_MASK) == ADB_READREG(0, 0))
> + last_poll_cmd = last_cmd;
> +
> /* Output mode */
> via[ACR] |= SR_OUT;
> /* Load data */
> @@ -424,10 +433,11 @@ static irqreturn_t macii_interrupt(int irq, void *arg)
> if (req->done)
> (*req->done)(req);
>
> - if (current_req)
> + if (!current_req)
> + macii_queue_poll();
> +
> + if (current_req && macii_state == idle)
> macii_start();
> - else if (need_autopoll())
> - macii_autopoll(autopoll_devs);
> }
>
> if (macii_state == idle) {
> @@ -507,14 +517,11 @@ static irqreturn_t macii_interrupt(int irq, void *arg)
>
> macii_state = idle;
>
> - /* SRQ seen before, initiate poll now */
> - if (srq_asserted)
> + if (!current_req)
> macii_queue_poll();
>
> if (current_req)
> macii_start();
> - else if (need_autopoll())
> - macii_autopoll(autopoll_devs);
>
> if (macii_state == idle)
> via[B] = (via[B] & ~ST_MASK) | ST_IDLE;
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/9] macintosh/via-macii: Poll the device most likely to respond
2020-08-09 18:55 ` Guenter Roeck
@ 2020-08-09 22:58 ` Finn Thain
2020-08-10 1:16 ` Guenter Roeck
0 siblings, 1 reply; 16+ messages in thread
From: Finn Thain @ 2020-08-09 22:58 UTC (permalink / raw)
To: Guenter Roeck
Cc: Benjamin Herrenschmidt, Joshua Thompson, Geert Uytterhoeven,
linux-m68k, Laurent Vivier, Mark Cave-Ayland, linuxppc-dev,
linux-kernel
On Sun, 9 Aug 2020, Guenter Roeck wrote:
> Hi,
>
> On Sun, Jun 28, 2020 at 02:23:12PM +1000, Finn Thain wrote:
> > Poll the most recently polled device by default, rather than the lowest
> > device address that happens to be enabled in autopoll_devs. This improves
> > input latency. Re-use macii_queue_poll() rather than duplicate that logic.
> > This eliminates a static struct and function.
> >
> > Fixes: d95fd5fce88f0 ("m68k: Mac II ADB fixes") # v5.0+
> > Tested-by: Stan Johnson <userm57@yahoo.com>
> > Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
>
> With this patch applied, the qemu "q800" emulation no longer works and
> is stuck in early boot. Any idea why that might be the case, and/or how
> to debug it ?
>
The problem you're seeing was mentioned in the cover letter,
https://lore.kernel.org/linux-m68k/cover.1593318192.git.fthain@telegraphics.com.au/
Since this series was merged, Linus' tree is no longer compatible with
long-standing QEMU bugs.
The best way to fix this is to upgrade QEMU (latest is 5.1.0-rc3). Or use
the serial console instead of the framebuffer console.
I regret the inconvenience but the alternative was worse: adding code to
Linux to get compatibility with QEMU bugs (which were added to QEMU due to
Linux bugs).
My main concern is correct operation on actual hardware, as always. But
some QEMU developers are working on support for operating systems besides
Linux.
Therefore, I believe that both QEMU and Linux should aim for compatibility
with actual hardware and not bug compatibility with each other.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/9] macintosh/via-macii: Poll the device most likely to respond
2020-08-09 22:58 ` Finn Thain
@ 2020-08-10 1:16 ` Guenter Roeck
0 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2020-08-10 1:16 UTC (permalink / raw)
To: Finn Thain
Cc: Benjamin Herrenschmidt, Joshua Thompson, Geert Uytterhoeven,
linux-m68k, Laurent Vivier, Mark Cave-Ayland, linuxppc-dev,
linux-kernel
Hi,
On 8/9/20 3:58 PM, Finn Thain wrote:
> On Sun, 9 Aug 2020, Guenter Roeck wrote:
>
>> Hi,
>>
>> On Sun, Jun 28, 2020 at 02:23:12PM +1000, Finn Thain wrote:
>>> Poll the most recently polled device by default, rather than the lowest
>>> device address that happens to be enabled in autopoll_devs. This improves
>>> input latency. Re-use macii_queue_poll() rather than duplicate that logic.
>>> This eliminates a static struct and function.
>>>
>>> Fixes: d95fd5fce88f0 ("m68k: Mac II ADB fixes") # v5.0+
>>> Tested-by: Stan Johnson <userm57@yahoo.com>
>>> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
>>
>> With this patch applied, the qemu "q800" emulation no longer works and
>> is stuck in early boot. Any idea why that might be the case, and/or how
>> to debug it ?
>>
>
> The problem you're seeing was mentioned in the cover letter,
> https://lore.kernel.org/linux-m68k/cover.1593318192.git.fthain@telegraphics.com.au/
>
> Since this series was merged, Linus' tree is no longer compatible with
> long-standing QEMU bugs.
>
> The best way to fix this is to upgrade QEMU (latest is 5.1.0-rc3). Or use
> the serial console instead of the framebuffer console.
>
I have no problem with that. Actually, I had checked the qemu commit log,
but somehow I had missed missed the commits there.
> I regret the inconvenience but the alternative was worse: adding code to
> Linux to get compatibility with QEMU bugs (which were added to QEMU due to
> Linux bugs).
>
> My main concern is correct operation on actual hardware, as always. But
> some QEMU developers are working on support for operating systems besides
> Linux.
>
> Therefore, I believe that both QEMU and Linux should aim for compatibility
> with actual hardware and not bug compatibility with each other.
>
I absolutely agree.
I repeated the test on the mainline kernel with qemu v5.1-rc3, and it works.
I also made sure that older versions of Linux still work with the qemu
v5.1.0-rc3. So everything is good, and sorry for the noise.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 6/9] macintosh/via-macii: Use bool type for reading_reply variable
2020-06-28 4:23 [PATCH 0/9] Macintosh II ADB driver fixes Finn Thain
2020-06-28 4:23 ` [PATCH 7/9] macintosh/via-macii: Use unsigned type for autopoll_devs variable Finn Thain
2020-06-28 4:23 ` [PATCH 2/9] macintosh/via-macii: Poll the device most likely to respond Finn Thain
@ 2020-06-28 4:23 ` Finn Thain
2020-06-28 4:23 ` [PATCH 1/9] macintosh/via-macii: Access autopoll_devs when inside lock Finn Thain
` (6 subsequent siblings)
9 siblings, 0 replies; 16+ messages in thread
From: Finn Thain @ 2020-06-28 4:23 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Joshua Thompson, Geert Uytterhoeven, linux-m68k, Laurent Vivier,
Mark Cave-Ayland, linuxppc-dev, linux-kernel
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
drivers/macintosh/via-macii.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/macintosh/via-macii.c b/drivers/macintosh/via-macii.c
index 8d5ef77b4a435..e143ddb81de34 100644
--- a/drivers/macintosh/via-macii.c
+++ b/drivers/macintosh/via-macii.c
@@ -116,7 +116,7 @@ static struct adb_request *current_req; /* first request struct in the queue */
static struct adb_request *last_req; /* last request struct in the queue */
static unsigned char reply_buf[16]; /* storage for autopolled replies */
static unsigned char *reply_ptr; /* next byte in reply_buf or req->reply */
-static int reading_reply; /* store reply in reply_buf else req->reply */
+static bool reading_reply; /* store reply in reply_buf else req->reply */
static int data_index; /* index of the next byte to send from req->data */
static int reply_len; /* number of bytes received in reply_buf or req->reply */
static int status; /* VIA's ADB status bits captured upon interrupt */
@@ -394,7 +394,7 @@ static irqreturn_t macii_interrupt(int irq, void *arg)
WARN_ON((status & ST_MASK) != ST_IDLE);
reply_ptr = reply_buf;
- reading_reply = 0;
+ reading_reply = false;
bus_timeout = false;
srq_asserted = false;
@@ -442,7 +442,7 @@ static irqreturn_t macii_interrupt(int irq, void *arg)
*/
macii_state = reading;
- reading_reply = 0;
+ reading_reply = false;
reply_ptr = reply_buf;
*reply_ptr = last_talk_cmd;
reply_len = 1;
@@ -456,7 +456,7 @@ static irqreturn_t macii_interrupt(int irq, void *arg)
if (req->reply_expected) {
macii_state = reading;
- reading_reply = 1;
+ reading_reply = true;
reply_ptr = req->reply;
*reply_ptr = req->data[1];
reply_len = 1;
@@ -466,7 +466,7 @@ static irqreturn_t macii_interrupt(int irq, void *arg)
} else if ((req->data[1] & OP_MASK) == TALK) {
macii_state = reading;
- reading_reply = 0;
+ reading_reply = false;
reply_ptr = reply_buf;
*reply_ptr = req->data[1];
reply_len = 1;
--
2.26.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 1/9] macintosh/via-macii: Access autopoll_devs when inside lock
2020-06-28 4:23 [PATCH 0/9] Macintosh II ADB driver fixes Finn Thain
` (2 preceding siblings ...)
2020-06-28 4:23 ` [PATCH 6/9] macintosh/via-macii: Use bool type for reading_reply variable Finn Thain
@ 2020-06-28 4:23 ` Finn Thain
2020-08-09 19:01 ` Guenter Roeck
2020-06-28 4:23 ` [PATCH 9/9] macintosh/via-macii: Clarify definition of macii_init() Finn Thain
` (5 subsequent siblings)
9 siblings, 1 reply; 16+ messages in thread
From: Finn Thain @ 2020-06-28 4:23 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Joshua Thompson, Geert Uytterhoeven, linux-m68k, Laurent Vivier,
Mark Cave-Ayland, linuxppc-dev, linux-kernel
The interrupt handler should be excluded when accessing the autopoll_devs
variable.
Fixes: d95fd5fce88f0 ("m68k: Mac II ADB fixes") # v5.0+
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
drivers/macintosh/via-macii.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/macintosh/via-macii.c b/drivers/macintosh/via-macii.c
index ac824d7b2dcfc..6aa903529570d 100644
--- a/drivers/macintosh/via-macii.c
+++ b/drivers/macintosh/via-macii.c
@@ -270,15 +270,12 @@ static int macii_autopoll(int devs)
unsigned long flags;
int err = 0;
+ local_irq_save(flags);
+
/* bit 1 == device 1, and so on. */
autopoll_devs = devs & 0xFFFE;
- if (!autopoll_devs)
- return 0;
-
- local_irq_save(flags);
-
- if (current_req == NULL) {
+ if (autopoll_devs && !current_req) {
/* Send a Talk Reg 0. The controller will repeatedly transmit
* this as long as it is idle.
*/
--
2.26.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 1/9] macintosh/via-macii: Access autopoll_devs when inside lock
2020-06-28 4:23 ` [PATCH 1/9] macintosh/via-macii: Access autopoll_devs when inside lock Finn Thain
@ 2020-08-09 19:01 ` Guenter Roeck
2020-08-09 23:15 ` Finn Thain
0 siblings, 1 reply; 16+ messages in thread
From: Guenter Roeck @ 2020-08-09 19:01 UTC (permalink / raw)
To: Finn Thain
Cc: Benjamin Herrenschmidt, Joshua Thompson, Geert Uytterhoeven,
linux-m68k, Laurent Vivier, Mark Cave-Ayland, linuxppc-dev,
linux-kernel
Hi,
On Sun, Jun 28, 2020 at 02:23:12PM +1000, Finn Thain wrote:
> The interrupt handler should be excluded when accessing the autopoll_devs
> variable.
>
I am quite baffled by this patch. Other than adding an unnecessary lock /
unlock sequence, accessing a variable (which is derived from another
variable) from inside or outside a lock does not make a difference.
If autopoll_devs = devs & 0xfffe is 0 inside the lock, it will just
as much be 0 outside the lock, and vice versa.
Can you explain this in some more detail ? Not that is matters much since
the change already made it into mainline, but I would like to understand
what if anything I am missing here.
Thanks,
Guenter
> Fixes: d95fd5fce88f0 ("m68k: Mac II ADB fixes") # v5.0+
> Tested-by: Stan Johnson <userm57@yahoo.com>
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> ---
> drivers/macintosh/via-macii.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/macintosh/via-macii.c b/drivers/macintosh/via-macii.c
> index ac824d7b2dcfc..6aa903529570d 100644
> --- a/drivers/macintosh/via-macii.c
> +++ b/drivers/macintosh/via-macii.c
> @@ -270,15 +270,12 @@ static int macii_autopoll(int devs)
> unsigned long flags;
> int err = 0;
>
> + local_irq_save(flags);
> +
> /* bit 1 == device 1, and so on. */
> autopoll_devs = devs & 0xFFFE;
>
> - if (!autopoll_devs)
> - return 0;
> -
> - local_irq_save(flags);
> -
> - if (current_req == NULL) {
> + if (autopoll_devs && !current_req) {
> /* Send a Talk Reg 0. The controller will repeatedly transmit
> * this as long as it is idle.
> */
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/9] macintosh/via-macii: Access autopoll_devs when inside lock
2020-08-09 19:01 ` Guenter Roeck
@ 2020-08-09 23:15 ` Finn Thain
0 siblings, 0 replies; 16+ messages in thread
From: Finn Thain @ 2020-08-09 23:15 UTC (permalink / raw)
To: Guenter Roeck
Cc: Benjamin Herrenschmidt, Joshua Thompson, Geert Uytterhoeven,
linux-m68k, Laurent Vivier, Mark Cave-Ayland, linuxppc-dev,
linux-kernel
On Sun, 9 Aug 2020, Guenter Roeck wrote:
> Hi,
>
> On Sun, Jun 28, 2020 at 02:23:12PM +1000, Finn Thain wrote:
> > The interrupt handler should be excluded when accessing the
> > autopoll_devs variable.
> >
>
> I am quite baffled by this patch. Other than adding an unnecessary lock
> / unlock sequence,
The new lock/unlock sequence means that the expression (autopoll_devs &&
!current_req) can be understood to be atomic. That makes it easier for me
to follow (being that both variables are shared state).
> accessing a variable (which is derived from another variable) from
> inside or outside a lock does not make a difference. If autopoll_devs =
> devs & 0xfffe is 0 inside the lock, it will just as much be 0 outside
> the lock, and vice versa.
>
> Can you explain this in some more detail ? Not that is matters much
> since the change already made it into mainline, but I would like to
> understand what if anything I am missing here.
>
I think the new code is more readable and is obviously free of race
conditions. It's not obvious to me why the old code was free of race
conditions but if you can easily establish that by inspection then you are
a better auditor than I am. Regardless, I'll stick with "Keep It Simple,
Stupid".
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 9/9] macintosh/via-macii: Clarify definition of macii_init()
2020-06-28 4:23 [PATCH 0/9] Macintosh II ADB driver fixes Finn Thain
` (3 preceding siblings ...)
2020-06-28 4:23 ` [PATCH 1/9] macintosh/via-macii: Access autopoll_devs when inside lock Finn Thain
@ 2020-06-28 4:23 ` Finn Thain
2020-06-28 4:23 ` [PATCH 5/9] macintosh/via-macii: Handle poll replies correctly Finn Thain
` (4 subsequent siblings)
9 siblings, 0 replies; 16+ messages in thread
From: Finn Thain @ 2020-06-28 4:23 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Joshua Thompson, Geert Uytterhoeven, linux-m68k, Laurent Vivier,
Mark Cave-Ayland, linuxppc-dev, linux-kernel
The function prototype correctly specifies the 'static' storage class.
Let the function definition match the declaration for better readability.
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
drivers/macintosh/via-macii.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/macintosh/via-macii.c b/drivers/macintosh/via-macii.c
index 2f9be4ec7d345..060e03f2264bc 100644
--- a/drivers/macintosh/via-macii.c
+++ b/drivers/macintosh/via-macii.c
@@ -140,7 +140,7 @@ static int macii_probe(void)
}
/* Initialize the driver */
-int macii_init(void)
+static int macii_init(void)
{
unsigned long flags;
int err;
--
2.26.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 5/9] macintosh/via-macii: Handle poll replies correctly
2020-06-28 4:23 [PATCH 0/9] Macintosh II ADB driver fixes Finn Thain
` (4 preceding siblings ...)
2020-06-28 4:23 ` [PATCH 9/9] macintosh/via-macii: Clarify definition of macii_init() Finn Thain
@ 2020-06-28 4:23 ` Finn Thain
2020-06-28 4:23 ` [PATCH 4/9] macintosh/via-macii: Remove read_done state Finn Thain
` (3 subsequent siblings)
9 siblings, 0 replies; 16+ messages in thread
From: Finn Thain @ 2020-06-28 4:23 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Joshua Thompson, Geert Uytterhoeven, linux-m68k, Laurent Vivier,
Mark Cave-Ayland, linuxppc-dev, linux-kernel
Userspace applications may use /dev/adb to send Talk requests. Such
requests always have req->reply_expected == 1. The same is true of Talk
requests sent by the kernel, except for poll requests queued internally
by the via-macii driver. Those requests have req->reply_expected == 0.
Consequently, poll reply packets get treated like autopoll reply packets.
(It doesn't make sense to try to distinguish them.) Always enter 'reading'
state after a poll request, so that the reply gets collected and passed
to adb_input(), and none go missing.
All Talk replies passed to adb_input() come from polling or autopolling,
so call adb_input() with the autopoll parameter set to 1.
Fixes: d95fd5fce88f0 ("m68k: Mac II ADB fixes") # v5.0+
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
drivers/macintosh/via-macii.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/drivers/macintosh/via-macii.c b/drivers/macintosh/via-macii.c
index d29c87943ca46..8d5ef77b4a435 100644
--- a/drivers/macintosh/via-macii.c
+++ b/drivers/macintosh/via-macii.c
@@ -463,6 +463,21 @@ static irqreturn_t macii_interrupt(int irq, void *arg)
via[ACR] &= ~SR_OUT;
x = via[SR];
+ } else if ((req->data[1] & OP_MASK) == TALK) {
+ macii_state = reading;
+
+ reading_reply = 0;
+ reply_ptr = reply_buf;
+ *reply_ptr = req->data[1];
+ reply_len = 1;
+
+ via[ACR] &= ~SR_OUT;
+ x = via[SR];
+
+ req->complete = 1;
+ current_req = req->next;
+ if (req->done)
+ (*req->done)(req);
} else {
macii_state = idle;
@@ -510,8 +525,9 @@ static irqreturn_t macii_interrupt(int irq, void *arg)
current_req = req->next;
if (req->done)
(*req->done)(req);
- } else if (reply_len && autopoll_devs) {
- adb_input(reply_buf, reply_len, 0);
+ } else if (reply_len && autopoll_devs &&
+ reply_buf[0] == last_poll_cmd) {
+ adb_input(reply_buf, reply_len, 1);
}
break;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 4/9] macintosh/via-macii: Remove read_done state
2020-06-28 4:23 [PATCH 0/9] Macintosh II ADB driver fixes Finn Thain
` (5 preceding siblings ...)
2020-06-28 4:23 ` [PATCH 5/9] macintosh/via-macii: Handle poll replies correctly Finn Thain
@ 2020-06-28 4:23 ` Finn Thain
2020-06-28 4:23 ` [PATCH 3/9] macintosh/via-macii: Handle /CTLR_IRQ signal correctly Finn Thain
` (2 subsequent siblings)
9 siblings, 0 replies; 16+ messages in thread
From: Finn Thain @ 2020-06-28 4:23 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Joshua Thompson, Geert Uytterhoeven, linux-m68k, Laurent Vivier,
Mark Cave-Ayland, linuxppc-dev, linux-kernel
The driver state machine may enter the 'read_done' state when leaving the
'idle' or 'reading' state. This transition is pointless, as is the extra
interrupt it requires. The interrupt is produced by the transceiver
(even when it has no data to send) because an extra EVEN/ODD toggle
was signalled by the driver. Drop the extra state to simplify the code.
Fixes: 1da177e4c3f41 ("Linux-2.6.12-rc2") # v5.0+
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
drivers/macintosh/via-macii.c | 70 ++++++++++++++---------------------
1 file changed, 28 insertions(+), 42 deletions(-)
diff --git a/drivers/macintosh/via-macii.c b/drivers/macintosh/via-macii.c
index 6a5cd7de05baf..d29c87943ca46 100644
--- a/drivers/macintosh/via-macii.c
+++ b/drivers/macintosh/via-macii.c
@@ -110,7 +110,6 @@ static enum macii_state {
idle,
sending,
reading,
- read_done,
} macii_state;
static struct adb_request *current_req; /* first request struct in the queue */
@@ -411,8 +410,8 @@ static irqreturn_t macii_interrupt(int irq, void *arg)
reply_len = 1;
} else {
/* bus timeout */
- macii_state = read_done;
reply_len = 0;
+ break;
}
/* set ADB state = even for first data byte */
@@ -471,20 +470,6 @@ static irqreturn_t macii_interrupt(int irq, void *arg)
current_req = req->next;
if (req->done)
(*req->done)(req);
-
- if (!current_req)
- macii_queue_poll();
-
- if (current_req && macii_state == idle)
- macii_start();
-
- if (macii_state == idle) {
- /* reset to shift in */
- via[ACR] &= ~SR_OUT;
- x = via[SR];
- /* set ADB state idle - might get SRQ */
- via[B] = (via[B] & ~ST_MASK) | ST_IDLE;
- }
break;
}
} else {
@@ -511,12 +496,28 @@ static irqreturn_t macii_interrupt(int irq, void *arg)
} else if (status == ST_ODD && reply_len == 2) {
srq_asserted = true;
} else {
- macii_state = read_done;
+ macii_state = idle;
+
+ if (bus_timeout)
+ reply_len = 0;
+
+ if (reading_reply) {
+ struct adb_request *req = current_req;
+
+ req->reply_len = reply_len;
+
+ req->complete = 1;
+ current_req = req->next;
+ if (req->done)
+ (*req->done)(req);
+ } else if (reply_len && autopoll_devs) {
+ adb_input(reply_buf, reply_len, 0);
+ }
+ break;
}
}
- if (macii_state == reading &&
- reply_len < ARRAY_SIZE(reply_buf)) {
+ if (reply_len < ARRAY_SIZE(reply_buf)) {
reply_ptr++;
*reply_ptr = x;
reply_len++;
@@ -526,37 +527,22 @@ static irqreturn_t macii_interrupt(int irq, void *arg)
via[B] ^= ST_MASK;
break;
- case read_done:
- x = via[SR];
-
- if (bus_timeout)
- reply_len = 0;
-
- if (reading_reply) {
- reading_reply = 0;
- req = current_req;
- req->reply_len = reply_len;
- req->complete = 1;
- current_req = req->next;
- if (req->done)
- (*req->done)(req);
- } else if (reply_len && autopoll_devs)
- adb_input(reply_buf, reply_len, 0);
-
- macii_state = idle;
+ default:
+ break;
+ }
+ if (macii_state == idle) {
if (!current_req)
macii_queue_poll();
if (current_req)
macii_start();
- if (macii_state == idle)
+ if (macii_state == idle) {
+ via[ACR] &= ~SR_OUT;
+ x = via[SR];
via[B] = (via[B] & ~ST_MASK) | ST_IDLE;
- break;
-
- default:
- break;
+ }
}
local_irq_restore(flags);
--
2.26.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 3/9] macintosh/via-macii: Handle /CTLR_IRQ signal correctly
2020-06-28 4:23 [PATCH 0/9] Macintosh II ADB driver fixes Finn Thain
` (6 preceding siblings ...)
2020-06-28 4:23 ` [PATCH 4/9] macintosh/via-macii: Remove read_done state Finn Thain
@ 2020-06-28 4:23 ` Finn Thain
2020-06-28 4:23 ` [PATCH 8/9] macintosh/via-macii: Use the stack for reset request storage Finn Thain
2020-07-27 7:26 ` [PATCH 0/9] Macintosh II ADB driver fixes Michael Ellerman
9 siblings, 0 replies; 16+ messages in thread
From: Finn Thain @ 2020-06-28 4:23 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Joshua Thompson, Geert Uytterhoeven, linux-m68k, Laurent Vivier,
Mark Cave-Ayland, linuxppc-dev, linux-kernel
I'm told that the /CTLR_IRQ signal from the ADB transceiver gets
interpreted by MacOS to mean SRQ, bus timeout or end-of-packet depending
on the circumstances, and that Linux's via-macii driver does not
correctly interpret this signal.
Instead, the via-macii driver interprets certain received byte values
(0x00 and 0xFF) as signalling end of packet and bus timeout
(respectively). Problem is, those values can also appear under other
circumstances.
This patch changes the bus timeout, end of packet and SRQ detection logic
to bring it closer to the logic that MacOS reportedly uses.
Fixes: 1da177e4c3f41 ("Linux-2.6.12-rc2") # v5.0+
Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
drivers/macintosh/via-macii.c | 166 ++++++++++++++++++++--------------
1 file changed, 97 insertions(+), 69 deletions(-)
diff --git a/drivers/macintosh/via-macii.c b/drivers/macintosh/via-macii.c
index d4f1a65c5f1fd..6a5cd7de05baf 100644
--- a/drivers/macintosh/via-macii.c
+++ b/drivers/macintosh/via-macii.c
@@ -80,6 +80,8 @@ static volatile unsigned char *via;
/* ADB command byte structure */
#define ADDR_MASK 0xF0
#define CMD_MASK 0x0F
+#define OP_MASK 0x0C
+#define TALK 0x0C
static int macii_init_via(void);
static void macii_start(void);
@@ -119,9 +121,10 @@ static int reading_reply; /* store reply in reply_buf else req->reply */
static int data_index; /* index of the next byte to send from req->data */
static int reply_len; /* number of bytes received in reply_buf or req->reply */
static int status; /* VIA's ADB status bits captured upon interrupt */
-static int last_status; /* status bits as at previous interrupt */
-static int srq_asserted; /* have to poll for the device that asserted it */
+static bool bus_timeout; /* no data was sent by the device */
+static bool srq_asserted; /* have to poll for the device that asserted it */
static u8 last_cmd; /* the most recent command byte transmitted */
+static u8 last_talk_cmd; /* the most recent Talk command byte transmitted */
static u8 last_poll_cmd; /* the most recent Talk R0 command byte transmitted */
static int autopoll_devs; /* bits set are device addresses to be polled */
@@ -170,7 +173,6 @@ static int macii_init_via(void)
/* Set up state: idle */
via[B] |= ST_IDLE;
- last_status = via[B] & (ST_MASK | CTLR_IRQ);
/* Shift register on input */
via[ACR] = (via[ACR] & ~SR_CTRL) | SR_EXT;
@@ -336,13 +338,6 @@ static void macii_start(void)
* And req->nbytes is the number of bytes of real data plus one.
*/
- /* store command byte */
- last_cmd = req->data[1];
-
- /* If this is a Talk Register 0 command, store the command byte */
- if ((last_cmd & CMD_MASK) == ADB_READREG(0, 0))
- last_poll_cmd = last_cmd;
-
/* Output mode */
via[ACR] |= SR_OUT;
/* Load data */
@@ -352,6 +347,9 @@ static void macii_start(void)
macii_state = sending;
data_index = 2;
+
+ bus_timeout = false;
+ srq_asserted = false;
}
/*
@@ -360,15 +358,17 @@ static void macii_start(void)
* generating shift register interrupts (SR_INT) for us. This means there has
* to be activity on the ADB bus. The chip will poll to achieve this.
*
- * The basic ADB state machine was left unchanged from the original MacII code
- * by Alan Cox, which was based on the CUDA driver for PowerMac.
- * The syntax of the ADB status lines is totally different on MacII,
- * though. MacII uses the states Command -> Even -> Odd -> Even ->...-> Idle
- * for sending and Idle -> Even -> Odd -> Even ->...-> Idle for receiving.
- * Start and end of a receive packet are signalled by asserting /IRQ on the
- * interrupt line (/IRQ means the CTLR_IRQ bit in port B; not to be confused
- * with the VIA shift register interrupt. /IRQ never actually interrupts the
- * processor, it's just an ordinary input.)
+ * The VIA Port B output signalling works as follows. After the ADB transceiver
+ * sees a transition on the PB4 and PB5 lines it will crank over the VIA shift
+ * register which eventually raises the SR_INT interrupt. The PB4/PB5 outputs
+ * are toggled with each byte as the ADB transaction progresses.
+ *
+ * Request with no reply expected (and empty transceiver buffer):
+ * CMD -> IDLE
+ * Request with expected reply packet (or with buffered autopoll packet):
+ * CMD -> EVEN -> ODD -> EVEN -> ... -> IDLE
+ * Unsolicited packet:
+ * IDLE -> EVEN -> ODD -> EVEN -> ... -> IDLE
*/
static irqreturn_t macii_interrupt(int irq, void *arg)
{
@@ -388,31 +388,31 @@ static irqreturn_t macii_interrupt(int irq, void *arg)
}
}
- last_status = status;
status = via[B] & (ST_MASK | CTLR_IRQ);
switch (macii_state) {
case idle:
- if (reading_reply) {
- reply_ptr = current_req->reply;
- } else {
- WARN_ON(current_req);
- reply_ptr = reply_buf;
- }
+ WARN_ON((status & ST_MASK) != ST_IDLE);
+
+ reply_ptr = reply_buf;
+ reading_reply = 0;
+
+ bus_timeout = false;
+ srq_asserted = false;
x = via[SR];
- if ((status & CTLR_IRQ) && (x == 0xFF)) {
- /* Bus timeout without SRQ sequence:
- * data is "FF" while CTLR_IRQ is "H"
+ if (!(status & CTLR_IRQ)) {
+ /* /CTLR_IRQ asserted in idle state means we must
+ * read an autopoll reply from the transceiver buffer.
*/
- reply_len = 0;
- srq_asserted = 0;
- macii_state = read_done;
- } else {
macii_state = reading;
*reply_ptr = x;
reply_len = 1;
+ } else {
+ /* bus timeout */
+ macii_state = read_done;
+ reply_len = 0;
}
/* set ADB state = even for first data byte */
@@ -421,13 +421,52 @@ static irqreturn_t macii_interrupt(int irq, void *arg)
case sending:
req = current_req;
- if (data_index >= req->nbytes) {
+
+ if (status == (ST_CMD | CTLR_IRQ)) {
+ /* /CTLR_IRQ de-asserted after the command byte means
+ * the host can continue with the transaction.
+ */
+
+ /* Store command byte */
+ last_cmd = req->data[1];
+ if ((last_cmd & OP_MASK) == TALK) {
+ last_talk_cmd = last_cmd;
+ if ((last_cmd & CMD_MASK) == ADB_READREG(0, 0))
+ last_poll_cmd = last_cmd;
+ }
+ }
+
+ if (status == ST_CMD) {
+ /* /CTLR_IRQ asserted after the command byte means we
+ * must read an autopoll reply. The first byte was
+ * lost because the shift register was an output.
+ */
+ macii_state = reading;
+
+ reading_reply = 0;
+ reply_ptr = reply_buf;
+ *reply_ptr = last_talk_cmd;
+ reply_len = 1;
+
+ /* reset to shift in */
+ via[ACR] &= ~SR_OUT;
+ x = via[SR];
+ } else if (data_index >= req->nbytes) {
req->sent = 1;
- macii_state = idle;
if (req->reply_expected) {
+ macii_state = reading;
+
reading_reply = 1;
+ reply_ptr = req->reply;
+ *reply_ptr = req->data[1];
+ reply_len = 1;
+
+ via[ACR] &= ~SR_OUT;
+ x = via[SR];
} else {
+ macii_state = idle;
+
req->complete = 1;
current_req = req->next;
if (req->done)
@@ -438,25 +477,26 @@ static irqreturn_t macii_interrupt(int irq, void *arg)
if (current_req && macii_state == idle)
macii_start();
- }
- if (macii_state == idle) {
- /* reset to shift in */
- via[ACR] &= ~SR_OUT;
- x = via[SR];
- /* set ADB state idle - might get SRQ */
- via[B] = (via[B] & ~ST_MASK) | ST_IDLE;
+ if (macii_state == idle) {
+ /* reset to shift in */
+ via[ACR] &= ~SR_OUT;
+ x = via[SR];
+ /* set ADB state idle - might get SRQ */
+ via[B] = (via[B] & ~ST_MASK) | ST_IDLE;
+ }
+ break;
}
} else {
via[SR] = req->data[data_index++];
+ }
- if ((via[B] & ST_MASK) == ST_CMD) {
- /* just sent the command byte, set to EVEN */
- via[B] = (via[B] & ~ST_MASK) | ST_EVEN;
- } else {
- /* invert state bits, toggle ODD/EVEN */
- via[B] ^= ST_MASK;
- }
+ if ((via[B] & ST_MASK) == ST_CMD) {
+ /* just sent the command byte, set to EVEN */
+ via[B] = (via[B] & ~ST_MASK) | ST_EVEN;
+ } else {
+ /* invert state bits, toggle ODD/EVEN */
+ via[B] ^= ST_MASK;
}
break;
@@ -465,28 +505,13 @@ static irqreturn_t macii_interrupt(int irq, void *arg)
WARN_ON((status & ST_MASK) == ST_CMD ||
(status & ST_MASK) == ST_IDLE);
- /* Bus timeout with SRQ sequence:
- * data is "XX FF" while CTLR_IRQ is "L L"
- * End of packet without SRQ sequence:
- * data is "XX...YY 00" while CTLR_IRQ is "L...H L"
- * End of packet SRQ sequence:
- * data is "XX...YY 00" while CTLR_IRQ is "L...L L"
- * (where XX is the first response byte and
- * YY is the last byte of valid response data.)
- */
-
- srq_asserted = 0;
if (!(status & CTLR_IRQ)) {
- if (x == 0xFF) {
- if (!(last_status & CTLR_IRQ)) {
- macii_state = read_done;
- reply_len = 0;
- srq_asserted = 1;
- }
- } else if (x == 0x00) {
+ if (status == ST_EVEN && reply_len == 1) {
+ bus_timeout = true;
+ } else if (status == ST_ODD && reply_len == 2) {
+ srq_asserted = true;
+ } else {
macii_state = read_done;
- if (!(last_status & CTLR_IRQ))
- srq_asserted = 1;
}
}
@@ -504,6 +529,9 @@ static irqreturn_t macii_interrupt(int irq, void *arg)
case read_done:
x = via[SR];
+ if (bus_timeout)
+ reply_len = 0;
+
if (reading_reply) {
reading_reply = 0;
req = current_req;
--
2.26.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 8/9] macintosh/via-macii: Use the stack for reset request storage
2020-06-28 4:23 [PATCH 0/9] Macintosh II ADB driver fixes Finn Thain
` (7 preceding siblings ...)
2020-06-28 4:23 ` [PATCH 3/9] macintosh/via-macii: Handle /CTLR_IRQ signal correctly Finn Thain
@ 2020-06-28 4:23 ` Finn Thain
2020-07-27 7:26 ` [PATCH 0/9] Macintosh II ADB driver fixes Michael Ellerman
9 siblings, 0 replies; 16+ messages in thread
From: Finn Thain @ 2020-06-28 4:23 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Joshua Thompson, Geert Uytterhoeven, linux-m68k, Laurent Vivier,
Mark Cave-Ayland, linuxppc-dev, linux-kernel
The adb_request struct can be stored on the stack because the request
is synchronous and is completed before the function returns.
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
drivers/macintosh/via-macii.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/macintosh/via-macii.c b/drivers/macintosh/via-macii.c
index 447273967e1e8..2f9be4ec7d345 100644
--- a/drivers/macintosh/via-macii.c
+++ b/drivers/macintosh/via-macii.c
@@ -313,7 +313,7 @@ static void macii_poll(void)
/* Reset the bus */
static int macii_reset_bus(void)
{
- static struct adb_request req;
+ struct adb_request req;
/* Command = 0, Address = ignored */
adb_request(&req, NULL, ADBREQ_NOSEND, 1, ADB_BUSRESET);
--
2.26.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 0/9] Macintosh II ADB driver fixes
2020-06-28 4:23 [PATCH 0/9] Macintosh II ADB driver fixes Finn Thain
` (8 preceding siblings ...)
2020-06-28 4:23 ` [PATCH 8/9] macintosh/via-macii: Use the stack for reset request storage Finn Thain
@ 2020-07-27 7:26 ` Michael Ellerman
9 siblings, 0 replies; 16+ messages in thread
From: Michael Ellerman @ 2020-07-27 7:26 UTC (permalink / raw)
To: Finn Thain, Benjamin Herrenschmidt
Cc: Mark Cave-Ayland, Geert Uytterhoeven, linux-m68k,
Joshua Thompson, linuxppc-dev, Laurent Vivier, linux-kernel
On Sun, 28 Jun 2020 14:23:12 +1000, Finn Thain wrote:
> Various issues with the via-macii driver have become apparent over the
> years. Some examples:
>
> - A Talk command response can be lost. This can result in phantom devices
> being probed or an incorrect device handler ID being retrieved.
>
> - A reply packet containing a null byte can get truncated. Such packets
> are sometimes generated by ADB keyboards.
>
> [...]
Applied to powerpc/next.
[1/9] macintosh/via-macii: Access autopoll_devs when inside lock
https://git.kernel.org/powerpc/c/59ea38f6b3af5636edf541768a1ed721eeaca99e
[2/9] macintosh/via-macii: Poll the device most likely to respond
https://git.kernel.org/powerpc/c/f93bfeb55255bddaa16597e187a99ae6131b964a
[3/9] macintosh/via-macii: Handle /CTLR_IRQ signal correctly
https://git.kernel.org/powerpc/c/b4d76c28eca369b8105fe3a0a9f396e3fbcd0dd5
[4/9] macintosh/via-macii: Remove read_done state
https://git.kernel.org/powerpc/c/b16b67689baa01a5616b651356df7ad3e47a8763
[5/9] macintosh/via-macii: Handle poll replies correctly
https://git.kernel.org/powerpc/c/624cf5b538b507293ec761797bd8ce0702fefe64
[6/9] macintosh/via-macii: Use bool type for reading_reply variable
https://git.kernel.org/powerpc/c/f87a162572c9f7c839a207c7de6c73ffe54a777c
[7/9] macintosh/via-macii: Use unsigned type for autopoll_devs variable
https://git.kernel.org/powerpc/c/5c0c15a1953a7de2878d7e6f5711fd3322b11faa
[8/9] macintosh/via-macii: Use the stack for reset request storage
https://git.kernel.org/powerpc/c/046ace8256489f32740da07de55a913ca09ce5cf
[9/9] macintosh/via-macii: Clarify definition of macii_init()
https://git.kernel.org/powerpc/c/3327e58a04501e06aa531cdb4044aab214a6254a
cheers
^ permalink raw reply [flat|nested] 16+ messages in thread