Re: [PATCH 05/18] tty: serial: samsung_tty: add support for Apple UARTs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 06/02/2021 22.15, Marc Zyngier wrote:
-static int s3c24xx_serial_has_interrupt_mask(struct uart_port *port)
+static int s3c24xx_irq_type(struct uart_port *port)
  {
-	return to_ourport(port)->info->type == PORT_S3C6400;
+	switch (to_ourport(port)->info->type) {
+	case PORT_S3C6400:
+		return IRQ_S3C6400;
+	case PORT_APPLE:
+		return IRQ_APPLE;
+	default:
+		return IRQ_DISCRETE;
+	}
+

nit: For ease of reviewing, it'd be good if you could split this patch
with introducing the S3C6400 and "discrete" support initially, and
only then add the new stuff.

Good idea, will do for v2.

+	if (s3c24xx_irq_type(port) == IRQ_APPLE)
+		s3c24xx_serial_tx_chars(NO_IRQ, ourport);

Instead of directly calling into the handler (which has its own
problems, see below), could you just tickle the interrupt status
register to make an interrupt pending and trigger an actual interrupt?
I have no idea whether the HW supports this kind of trick though.

I thought of that, but I tried really hard to find such a feature with no success. The best I can do is unmask and trigger the *RX* timeout interrupt which will eventually fire but... this doesn't work so well in practice. There is no way to trigger IRQ flags directly (as those bits are write-1-to-clear).

-	spin_lock_irqsave(&port->lock, flags);
+	/* Only lock if called from IRQ context */
+	if (irq != NO_IRQ)
+		spin_lock_irqsave(&port->lock, flags);

Isn't that actually dangerous? What prevents the interrupt from firing
right in the middle of this sequence and create havoc when called from
enable_tx_pio()? I fail to see what you gain with sidestepping the
locking.

The callpath here is:

uart_start -> __uart_start -> (uart_ops.start_tx) s3c24xx_serial_start_tx -> s3c24xx_serial_start_tx_pio -> enable_tx_pio -> s3c24xx_serial_tx_chars

And uart_start takes the uart_port lock. None of the serial functions take the lock because the serial core already does, but obviously the IRQ handler needs to, *if* it's called as an IRQ handler only.

The default should be IRQ_NONE, otherwise the kernel cannot detect a
screaming spurious interrupt.

Good point, and this needs fixing in s3c64xx_serial_handle_irq too then (which is what I based mine off of).

+	ret = request_irq(port->irq, apple_serial_handle_irq, IRQF_SHARED,
+			  s3c24xx_serial_portname(port), ourport);

Why IRQF_SHARED? Do you expect any other device sharing the same line
with this UART?

This also came from s3c64xx_serial_startup and... now I wonder why that one needs it. Maybe on some SoCs it does get shared? Certainly not for discrete rx/tx irq chips (and indeed those don't set the flag)...

CCing Thomas, who added the S3C64xx support (and should probably review this patch); is there a reason for IRQF_SHARED there? NB: v1 breaks the build on arm or with CONFIG_PM_SLEEP, those will be fixed for v2.

Either way, certainly not for Apple SoCs; I'll get rid of IRQF_SHARED for v2.

diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
index 62c22045fe65..59d102b674db 100644
--- a/include/uapi/linux/serial_core.h
+++ b/include/uapi/linux/serial_core.h
@@ -277,4 +277,7 @@
  /* Freescale LINFlexD UART */
  #define PORT_LINFLEXUART	122
+/* Apple Silicon (M1/T8103) UART (Samsung variant) */
+#define PORT_APPLE	123
+

Do you actually need a new port type here? Looking at the driver
itself, it is mainly used to work out the IRQ model. Maybe introducing
a new irq_type field in the port structure would be better than
exposing this to userspace (which should see something that is exactly
the same as a S3C UART).

Well... every S3C variant already has its own port type here.

#define PORT_S3C2410    55
#define PORT_S3C2440    61
#define PORT_S3C2400    67
#define PORT_S3C2412    73
#define PORT_S3C6400    84

If we don't introduce a new one, which one should we pretend to be? :)

I agree that it might make sense to merge all of these into one, though; I don't know what the original reason for splitting them out is. But now that they're part of the userspace API, this might not be a good idea. Though, unsurprisingly, some googling suggests there are zero users of these defines in userspace.

--
Hector Martin "marcan" (marcan@xxxxxxxxx)
Public Key: https://mrcn.st/pub



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux