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
