On Wed, 6 Sep 2023, Ilpo Järvinen wrote: > On Tue, 5 Sep 2023, Matthew Howell wrote: > > > From: Matthew Howell <matthew.howell@xxxxxxxxxxxx > > > > Sealevel XR1735X based cards utilize DTR to control RS-485 Enable, but the > > current implementation of 8250_exar uses RTS for the auto-RS485-Enable > > mode of the XR17V35X UARTs. This patch implements DTR Auto-RS485 on > > Sealevel cards. > > > > Link: https://lore.kernel.org/all/b0b1863f-40f4-d78e-7bb7-dc4312449d9e@xxxxxxxxxxxx/ > > > > Signed-off-by: Matthew Howell <matthew.howell@xxxxxxxxxxxx> > > --- > > Fixed style issues from previous submission. > > > > diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c > > index 3886f78ecbbf..20d2e7148be5 100644 > > --- a/drivers/tty/serial/8250/8250_exar.c > > +++ b/drivers/tty/serial/8250/8250_exar.c > > @@ -78,6 +78,9 @@ > > > > #define UART_EXAR_RS485_DLY(x) ((x) << 4) > > > > +#define UART_EXAR_DLD 0x02 /* Divisor Fractional */ > > +#define UART_EXAR_DLD_485_POLARITY 0x80 /* RS-485 Enable Signal Polarity */ > > + > > /* > > * IOT2040 MPIO wiring semantics: > > * > > @@ -439,6 +442,34 @@ static int generic_rs485_config(struct uart_port *port, struct ktermios *termios > > return 0; > > } > > > > +static int sealevel_rs485_config(struct uart_port *port, struct ktermios *termios, > > + struct serial_rs485 *rs485) > > +{ > > + u8 __iomem *p = port->membase; > > + u8 old_lcr; > > + > > + generic_rs485_config(port, termios, rs485); > > + > > + if (rs485->flags & SER_RS485_ENABLED) { > > + /* Set EFR[4]=1 to enable enhanced feature registers */ > > + writeb(readb(p + UART_XR_EFR) | UART_EFR_ECB, p + UART_XR_EFR); > > You should split these to 3 lines to make it easier to follow what's the > actual operation taking place here: > efr = readb(...); > efr |= UART_EFR_ECB; > writeb(...); > > ...I'm sorry I didn't realize this last time I looked. Ok. Will fix in next submission. > > + > > + /* Set MCR to use DTR as Auto-RS485 Enable signal */ > > + writeb(UART_MCR_OUT1, p + UART_MCR); > > + > > + /* Store original LCR and set LCR[7]=1 to enable access to DLD register */ > > I guess "store original LCR" path is pretty obvious and can be dropped. Ok. I can drop that part of the comment if it is clear otherwise. > > + old_lcr = readb(p + UART_LCR); > > + writeb(old_lcr | UART_LCR_DLAB, p + UART_LCR); > > + > > + /* Set DLD[7]=1 for inverted RS485 Enable logic */ > > + writeb(readb(p + UART_EXAR_DLD) | UART_EXAR_DLD_485_POLARITY, p + UART_EXAR_DLD); > > Split this as well. > > > + writeb(old_lcr, p + UART_LCR); > > + } > > This function should also disable RS-485 if SER_RS485_ENABLED is set but > currently it does nothing? > > -- > i. That's what I would have thought as well, but I have electrically verified disabling SER_RS485_ENABLED disables RS485 on the Sealevel cards I have tested. I did this by monitoring the serial signal while a program alternates setting and unsetting SER_RS485_ENABLED and verifying the card is tri-stating when SER_RS485_ENABLED is set and fails to tri-state when SER_RS485_ENABLED is not set. I do not know where disabling RS485 is happening in the driver since generic_rs485_config() does not handle this either, but I left it alone since it appears to be working without being handled in the ..._rs485_config() functions. > > > + > > + return 0; > > + } > > + > > static const struct serial_rs485 generic_rs485_supported = { > > .flags = SER_RS485_ENABLED, > > }; > > @@ -744,6 +775,16 @@ static int __maybe_unused exar_resume(struct device *dev) > > return 0; > > } > > > > +static int pci_sealevel_setup(struct exar8250 *priv, struct pci_dev *pcidev, > > + struct uart_8250_port *port, int idx) > > +{ > > + int ret = pci_xr17v35x_setup(priv, pcidev, port, idx); > > + > > + port->port.rs485_config = sealevel_rs485_config; > > + > > + return ret; > > +} > > + > > static SIMPLE_DEV_PM_OPS(exar_pci_pm, exar_suspend, exar_resume); > > > > static const struct exar8250_board pbn_fastcom335_2 = { > > @@ -809,6 +850,17 @@ static const struct exar8250_board pbn_exar_XR17V8358 = { > > .exit = pci_xr17v35x_exit, > > }; > > > > +static const struct exar8250_board pbn_sealevel = { > > + .setup = pci_sealevel_setup, > > + .exit = pci_xr17v35x_exit, > > +}; > > + > > +static const struct exar8250_board pbn_sealevel_16 = { > > + .num_ports = 16, > > + .setup = pci_sealevel_setup, > > + .exit = pci_xr17v35x_exit, > > +}; > > + > > #define CONNECT_DEVICE(devid, sdevid, bd) { \ > > PCI_DEVICE_SUB( \ > > PCI_VENDOR_ID_EXAR, \ > > @@ -838,6 +890,15 @@ static const struct exar8250_board pbn_exar_XR17V8358 = { > > (kernel_ulong_t)&bd \ > > } > > > > +#define SEALEVEL_DEVICE(devid, bd) { \ > > + PCI_DEVICE_SUB( \ > > + PCI_VENDOR_ID_EXAR, \ > > + PCI_DEVICE_ID_EXAR_##devid, \ > > + PCI_VENDOR_ID_SEALEVEL, \ > > + PCI_ANY_ID), 0, 0, \ > > + (kernel_ulong_t)&bd \ > > + } > > + > > static const struct pci_device_id exar_pci_tbl[] = { > > EXAR_DEVICE(ACCESSIO, COM_2S, pbn_exar_XR17C15x), > > EXAR_DEVICE(ACCESSIO, COM_4S, pbn_exar_XR17C15x), > > @@ -860,6 +921,12 @@ static const struct pci_device_id exar_pci_tbl[] = { > > CONNECT_DEVICE(XR17C154, UART_4_485, pbn_connect), > > CONNECT_DEVICE(XR17C158, UART_8_485, pbn_connect), > > > > + SEALEVEL_DEVICE(XR17V352, pbn_sealevel), > > + SEALEVEL_DEVICE(XR17V354, pbn_sealevel), > > + SEALEVEL_DEVICE(XR17V358, pbn_sealevel), > > + SEALEVEL_DEVICE(XR17V4358, pbn_sealevel_16), > > + SEALEVEL_DEVICE(XR17V8358, pbn_sealevel_16), > > + > > IBM_DEVICE(XR17C152, SATURN_SERIAL_ONE_PORT, pbn_exar_ibm_saturn), > > > > /* USRobotics USR298x-OEM PCI Modems */ > > >
