On Wed, 10 Apr 2024, Matthew Howell wrote: > On Wed, 2024-04-10 at 16:49 +0300, Ilpo Järvinen wrote: > > On Mon, 8 Apr 2024, Matthew Howell wrote: > > > On Mon, 2024-04-08 at 19:48 +0300, Ilpo Järvinen wrote: > > > > On Mon, 8 Apr 2024, Matthew Howell wrote: > > > > > > > > > On Wed, 2024-02-21 at 16:16 -0500, Matthew Howell wrote: > > > > > > Allows the use of the EN485 hardware pin by preserving the value of > > > > > > FCTR[5] in pci_xr17v35x_setup(). > > > > > > > > > > > > Per the XR17V35X datasheet, the EN485 hardware pin works by setting > > > > > > FCTR[5] when the pin is active. pci_xr17v35x_setup() prevented the use > > > > > > of EN485 because it overwrote the FCTR register. > > > > > > > > > > > > Signed-off-by: Matthew Howell <matthew.howell@xxxxxxxxxxxx> > > > > > > --- > > > > > > V1 -> V2 > > > > > > Fixed wordwrap in diff > > > > > > > > > > > > diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c > > > > > > index 23366f868..97711606f 100644 > > > > > > --- a/drivers/tty/serial/8250/8250_exar.c > > > > > > +++ b/drivers/tty/serial/8250/8250_exar.c > > > > > > @@ -596,6 +596,7 @@ pci_xr17v35x_setup(struct exar8250 *priv, struct pci_dev *pcidev, > > > > > > unsigned int baud = 7812500; > > > > > > u8 __iomem *p; > > > > > > int ret; > > > > > > + u8 en485mask; > > > > > > > > > > > > port->port.uartclk = baud * 16; > > > > > > port->port.rs485_config = platform->rs485_config; > > > > > > @@ -618,7 +619,8 @@ pci_xr17v35x_setup(struct exar8250 *priv, struct pci_dev *pcidev, > > > > > > p = port->port.membase; > > > > > > > > > > > > writeb(0x00, p + UART_EXAR_8XMODE); > > > > > > - writeb(UART_FCTR_EXAR_TRGD, p + UART_EXAR_FCTR); > > > > > > + en485mask = readb(p + UART_EXAR_FCTR) & UART_FCTR_EXAR_485; > > > > > > + writeb(UART_FCTR_EXAR_TRGD | en485mask, p + UART_EXAR_FCTR); > > > > > > writeb(128, p + UART_EXAR_TXTRG); > > > > > > writeb(128, p + UART_EXAR_RXTRG); > > > > > > > > Why you need to read rs485 state from the register? It should be available > > > > in ->rs485.flags & SER_RS485_ENABLED. > > > > > > > > > > Please correct me if I am wrong, but my understanding is that > > > SER_RS485_ENABLED is set from userspace using the TIOCRS485 IOCTL. > > > > Thought that and device_property_read_bool(dev, > > "linux,rs485-enabled-at-boot-time") > > > > I am not familiar with that property. Is that something that can be set > in userspace or via a kernel parameter? Or is it 'hard-coded' into the > device tree binding for a particular device? Through DT, yes (or ACPI equivalent). > > > However, this is not the only way that the FCTR register can be changed. > > > In particular, per XR17V35X datasheet, the EN485 pin is sampled on > > > power-on and transfers the logic state to FCTR[5]. Our card takes > > > advantage of this to allow users to configure RS485 in scenarios where > > > they cannot, or do not want to, modify their software to set > > > SER_RS485_ENABLED. > > > > > > However, this functionality of the UART does not currently work with > > > this driver because the entire FCTR register is being overwritten, > > > thereby erasing whatever value was written to FCTR[5] on UART power-up. > > > > > > The driver cannot know whether FCTR[5] was set on power-up without > > > reading the FCTR, therefore it must be read. > > > > ??? > > > > Are you saying RS485 is enabled without kernel knowing about it? I don't > > think that's the correct way to do things. > > That is correct. However, I wouldn't say it is an incorrect way of doing > things. Some reasoning/justification below: > > * Kernel/Driver/Software independent RS485 Auto Enable is not new. I > can't speak for other vendors, but Sealevel has been creating products > with this functionality for several decades, dating back to at least > 1994. > > * In those products, the kernel was *necessarily* entirely unaware of > whether RS485 was in use because most OS's and applications did not > support RS485 at the time and as such was implement to be entirely > transparent to the OS and application. > > * Since then, many UARTs have integrated Auto RS485 Enable into a single > package and we have moved towards utilizing this integrated > functionality when it is available. > > * I say the above to point out that there is a long history of RS485 > Auto Enable without kernel involvement, and therefore in my view it is > just as valid to have RS485 Auto Enabled set via hardware without the > kernel being aware as it is for the kernel to be aware. Is this "Auto" thing done with some dip switches/jumpers and is cast to stone after hw init? So driver's .probe() could just read that state and adjust kernel rs485 state based one it? > * Finally, to the day many applications still do not support RS485. > Therefore, having the ability to set RS485 via hardware is still a > valuable feature for users, and one that Exar/Maxlinear seems to have > recognized, as I don't know why they would even have the EN485 pin if > not for this sort of use-case. Once the serial line has been transitioned into RS485 mode, userspace does not need to be aware of it (which can happen through DT so no userspace involvement). So this shouldn't be seen as a problem. > Perhaps the optimal solution would be if there were a method for the > hardware to inform the kernel that it is configured for RS485 Auto > Enable via hardware, but I'm not aware of a supported way of doing this. > The only thing I can think of at the moment is just a check in the > init/setup that sets SER_RS485_ENABLED if FCTR[5] is enabled. I don't > know if this would be considered an improvement though. I don't think such mechanism exists beyond DT currently, but I don't see why it could be added. -- i.
