| Message ID | 4c6c4e3212a929822ec6a8ba09691b465541f648.camel@sealevel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [V8,1/2] serial: exar: Revert "serial: exar: Add support for Sealevel 7xxxC serial cards" | expand |
On Fri, 2023-09-22 at 17:48 +0300, andriy.shevchenko@intel.com wrote: > ⚠Caution: External email. Exercise extreme caution with links or attachments.⚠ > > > Bingo, you have threaded emails! > > On Fri, Sep 22, 2023 at 02:22:11PM +0000, Matthew Howell wrote: > > From: Matthew Howell <matthew.howell@sealevel.com> > > > > Sealevel XR17V35X 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. > > Btw, few ideas for further improvements / questions. > > ... > > > +static int pci_sealevel_setup(struct exar8250 *priv, struct pci_dev *pcidev, > > + struct uart_8250_port *port, int idx) > > +{ > > + int ret; > > + > > + ret = pci_xr17v35x_setup(priv, pcidev, port, idx); > > + if (ret) > > + return ret; > > + > > + port->port.rs485_config = sealevel_rs485_config; > > + > > + return 0; > > +} > > This actually can be embedded into original pci_xr17v35x_setup() as > > if (pdev->subsystem_vendor == PCI_VENDOR_ID_SEALEVEL) > port->port.rs485_config = sealevel_rs485_config; > > ... > That was my original thought prior to the first submission, but I wasn't sure about adding vendor-specific changes into pci_xr17v35x_setup() since it appears that the convention in 8250_exar.c has been for vendors to have their own setup function when they needed to change the init/setup behaviour. If that is not the case though and having this in pci_xr17v35x_setup() is more appropriate I'll be happy to put it there instead of having yet another setup function. > > + SEALEVEL_DEVICE(XR17V4358, pbn_sealevel_16), > > This is kinda worries me. Original Exar card has12 port, why 16 is in use for this one? > Ah, good catch. I had actually forgotten about the 12 port version. Will either fix or make redundant with move to pci_xr17v35x_setup() depending on feedback from my statement above about what is most appropriate. > > + SEALEVEL_DEVICE(XR17V8358, pbn_sealevel_16), > With the above suggestion this will be fixed automatically. > > -- > With Best Regards, > Andy Shevchenko > >
On Fri, Sep 22, 2023 at 03:33:05PM +0000, Matthew Howell wrote: > On Fri, 2023-09-22 at 17:48 +0300, andriy.shevchenko@intel.com wrote: > > On Fri, Sep 22, 2023 at 02:22:11PM +0000, Matthew Howell wrote: ... > > > +static int pci_sealevel_setup(struct exar8250 *priv, struct pci_dev *pcidev, > > > + struct uart_8250_port *port, int idx) > > > +{ > > > + int ret; > > > + > > > + ret = pci_xr17v35x_setup(priv, pcidev, port, idx); > > > + if (ret) > > > + return ret; > > > + > > > + port->port.rs485_config = sealevel_rs485_config; > > > + > > > + return 0; > > > +} > > > > This actually can be embedded into original pci_xr17v35x_setup() as > > > > if (pdev->subsystem_vendor == PCI_VENDOR_ID_SEALEVEL) > > port->port.rs485_config = sealevel_rs485_config; > > That was my original thought prior to the first submission, but I wasn't > sure about adding vendor-specific changes into pci_xr17v35x_setup() > since it appears that the convention in 8250_exar.c has been for vendors > to have their own setup function when they needed to change the > init/setup behaviour. > > If that is not the case though and having this in pci_xr17v35x_setup() Just use the common sense. The case you are now adding is using PCI IDs (vendor and device) that are Exar's. So, I do not see any violation of the above assumption. Checking for _sub_ IDs is fine, it's just a quirk for the Exar based chips. > is more appropriate I'll be happy to put it there instead of having yet > another setup function. ... > > > + SEALEVEL_DEVICE(XR17V4358, pbn_sealevel_16), > > > > This is kinda worries me. Original Exar card has 12 ports, why 16 is in use > > for this one? > > Ah, good catch. I had actually forgotten about the 12 port version. Will > either fix or make redundant with move to pci_xr17v35x_setup() depending > on feedback from my statement above about what is most appropriate. > > > > + SEALEVEL_DEVICE(XR17V8358, pbn_sealevel_16), > > With the above suggestion this will be fixed automatically.
diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c index 3886f78ecbbf..8ab80447fe8c 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,44 @@ 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; + u8 efr; + u8 dld; + int ret; + + ret = generic_rs485_config(port, termios, rs485); + if (ret) + return ret; + + if (rs485->flags & SER_RS485_ENABLED) { + old_lcr = readb(p + UART_LCR); + + /* Set EFR[4]=1 to enable enhanced feature registers */ + efr = readb(p + UART_XR_EFR); + efr |= UART_EFR_ECB; + writeb(efr, p + UART_XR_EFR); + + /* Set MCR to use DTR as Auto-RS485 Enable signal */ + writeb(UART_MCR_OUT1, p + UART_MCR); + + /* Set LCR[7]=1 to enable access to DLD register */ + writeb(old_lcr | UART_LCR_DLAB, p + UART_LCR); + + /* Set DLD[7]=1 for inverted RS485 Enable logic */ + dld = readb(p + UART_EXAR_DLD); + dld |= UART_EXAR_DLD_485_POLARITY; + writeb(dld, p + UART_EXAR_DLD); + + writeb(old_lcr, p + UART_LCR); + } + + return 0; +} + static const struct serial_rs485 generic_rs485_supported = { .flags = SER_RS485_ENABLED, }; @@ -744,6 +785,20 @@ 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; + + ret = pci_xr17v35x_setup(priv, pcidev, port, idx); + if (ret) + return ret; + + port->port.rs485_config = sealevel_rs485_config; + + return 0; +} + static SIMPLE_DEV_PM_OPS(exar_pci_pm, exar_suspend, exar_resume); static const struct exar8250_board pbn_fastcom335_2 = { @@ -809,6 +864,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 +904,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 +935,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 */