From: Guenter Roeck <linux@roeck-us.net>
To: Finn Thain <fthain@telegraphics.com.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>,
Joshua Thompson <funaho@jurai.org>,
Geert Uytterhoeven <geert@linux-m68k.org>,
linux-m68k@lists.linux-m68k.org,
Laurent Vivier <lvivier@redhat.com>,
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>,
linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/9] macintosh/via-macii: Poll the device most likely to respond
Date: Sun, 9 Aug 2020 11:55:41 -0700 [thread overview]
Message-ID: <20200809185541.GA133779@roeck-us.net> (raw)
In-Reply-To: <5836f80886ebcfbe5be5fb7e0dc49feed6469712.1593318192.git.fthain@telegraphics.com.au>
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
>
next prev parent reply other threads:[~2020-08-09 18:55 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
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-08-09 18:55 ` Guenter Roeck [this message]
2020-08-09 22:58 ` Finn Thain
2020-08-10 1:16 ` Guenter Roeck
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 ` [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
2020-06-28 4:23 ` [PATCH 9/9] macintosh/via-macii: Clarify definition of macii_init() Finn Thain
2020-06-28 4:23 ` [PATCH 5/9] macintosh/via-macii: Handle poll replies correctly Finn Thain
2020-06-28 4:23 ` [PATCH 4/9] macintosh/via-macii: Remove read_done state Finn Thain
2020-06-28 4:23 ` [PATCH 3/9] macintosh/via-macii: Handle /CTLR_IRQ signal correctly 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200809185541.GA133779@roeck-us.net \
--to=linux@roeck-us.net \
--cc=benh@kernel.crashing.org \
--cc=fthain@telegraphics.com.au \
--cc=funaho@jurai.org \
--cc=geert@linux-m68k.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-m68k@lists.linux-m68k.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=lvivier@redhat.com \
--cc=mark.cave-ayland@ilande.co.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).