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 <[email protected]>
Signed-off-by: Finn Thain <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Finn Thain <[email protected]>
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
>
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 <[email protected]>
> > Signed-off-by: Finn Thain <[email protected]>
>
> 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/[email protected]/
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.
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 <[email protected]>
>>> Signed-off-by: Finn Thain <[email protected]>
>>
>> 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/[email protected]/
>
> 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