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: > > > [Some people who received this message don't often get email from ilpo.jarvinen@xxxxxxxxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > > > > > ⚠Caution: External email. Exercise extreme caution with links or attachments.⚠ > > > > > > > > > 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? > > 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. * 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. 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. -- Matthew H. > > > pci_fastcom335_setup() seems to have the same problem? That small part > > > seems to be common code anyway which should be moved into helper, only the > > > trigger threshold seems to differ which can be given in a parameter. > > > > > > > Technically, yes mit has the same problem, but I am not sure it is an > > actual issue and am hesitant make the suggested changes for the > > following reasons: > > > > > > 1) The fastcom card may depend on the behaviour working as-is. > > 2) I only have Sealevel & reference Exar hardware to test, not Fastcom, > > so I have no way to test if the changes negatively impact fastcom > > 3) Finally, while I am not familar with the fastcom 335, it doesn't have > > any dip-switches or jumpers, which leads me to believe it doesn't have a > > way for users to utilize the EN485 pin. If this is the case, then there > > is no end-user benefit to 'fixing' this in pci_fastcom335_setup(). > > > > If someone with a fastcom 335 card is willing to test then I can take a > > stab at the suggested change, but at the moment I see at least some > > 'risk' in making this change, but no 'reward' for doing so. > > Okay, it seems fastcom part of the driver doesn't support rs485. > There's stupid naming in this driver, "generic_rs485_config()" isn't > really that generic it seems (and on top of that, the init flow is > hard to follow). :-/ > > > > -- > i.
