On Thu, 2024-04-11 at 13:01 -0400, Matthew Howell wrote: > On Thu, 2024-04-11 at 11:27 +0300, Ilpo Järvinen wrote: > > 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? > > > > The behaviour of the patch at the moment is that changes to > SER_RS485_ENABLED can 'reset' the state of FCTR[5]. For example, if an > application sets SER_RS485_ENABLED to true, then back to false, the card > will lose the Auto-RS485 Enable setting. > > On one hand, this provides users a method of 'resetting' FCTR[5] on a > channel that they do not want to be RS485 in order to work around that > limitation of the EN485 pin, but on the other it a little bit weird to > enabling and hen disabling SER_RS485_ENABLED does not leave you where > you in the state started if EN485 was asserted at boot. > > > > * 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. > > > > Ah, I had missed of_add_property() in the Device Tree kernel API. > > Would it be appropriate to find and modify the device tree node from > exar_pci_probe()? I ask because it looks like all or most device tree > operations in the 8250 driver are confined to 8250_of.c, but I believe > the FCTR is specific to Exar so I think your suggestion of checking the > register value during probe makes sense. > > > > 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. > > If DT already has a mechanism that can be used I would rather do that > than add additional mechanisms for the same thing. > After looking into this some more, I'm not sure if Device Tree will work. I was initially under the impression that DT was present for both embedded and ACPI capable systems due to /sys/devices/LNXSYSTEM being described as being a Device Tree representation of ACPI. However, after looking further I am not sure that this means that DT properties are respected on ACPI systems. Do you know whether this is the case? If not, do you know the ACPI equivalent of "rs485-enabled-at-boot-time"? I've not yet had much luck finding anything. -- Matthew H. > -- > Matthew H.
