Re: [PATCH V4 2/2] serial: exar: Add RS-485 support for Sealevel XR17V35X based cards

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 7285 bytes --]

On Wed, 6 Sep 2023, Matthew Howell wrote:

> On Wed, 6 Sep 2023, Ilpo Järvinen wrote:
> > On Tue, 5 Sep 2023, Matthew Howell wrote:
> > 
> > > From: Matthew Howell <matthew.howell@sealevel.com
> > >
> > > 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@sealevel.com/
> > >
> > > Signed-off-by: Matthew Howell <matthew.howell@sealevel.com>
> > > ---
> > > 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.

Okay, I guess it's something in generic_rs485_config() then. I've no 
objection in case it's working as is.

...I was badly mislead by the name of generic_rs485_config() btw. I 
thought it was something in core based on its name (although I also 
couldn't recall when that function would have been added to the core) but 
instead it's static inside exar and it would be better to name it so that 
it's obvious it refers to exar rather than "generic".

-- 
 i.


> 
> > 
> > > +
> > > +     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 */
> > >
> >