Messages in this thread |  | | From | Hector Martin 'marcan' <> | Subject | Re: [PATCH 05/18] tty: serial: samsung_tty: add support for Apple UARTs | Date | Sun, 7 Feb 2021 18:12:05 +0900 |
| |
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@marcan.st) Public Key: https://mrcn.st/pub
|  |