Hi Petr, thanks for looking at all that mess I'm extracting from GPL sources... I've looking at how things are supposed to be done and re-wrote the RS-485 and half-duplex parts from scratch.
On Wed, Feb 12, 2020 at 01:43:35PM +0100, Petr Štetiar wrote: > Daniel Golle <[email protected]> [2020-02-11 20:33:57]: > > Hi, > > it really still feels somehow weird, that's mainly why I've suggested to > take this through upstream first in my previous email. > > > +@@ -103,10 +106,42 @@ static inline void ar933x_uart_stop_tx_i > > + static inline void ar933x_uart_putc(struct ar933x_uart_port *up, int ch) > > + { > > + unsigned int rdata; > > ++ struct serial_rs485 rs485conf = up->port.rs485; > > + > > + rdata = ch & AR933X_UART_DATA_TX_RX_MASK; > > + rdata |= AR933X_UART_DATA_TX_CSR; > > +- ar933x_uart_write(up, AR933X_UART_DATA_REG, rdata); > > ++ > > ++ if (rs485conf.flags & SER_RS485_ENABLED) { > > ++ unsigned int timeout = 60000; > > ++ unsigned long flags; > > ++ unsigned int status; > > ++ > > ++ /* Disable RX interrupt */ > > ++ spin_lock_irqsave(&up->port.lock, flags); > > FYI this code path: > > ar933x_uart_interrupt > ar933x_uart_tx_chars > ar933x_uart_putc > > has acquired spin_lock, disabled TX interrupt, and this codepath: > > ar933x_uart_console_write > ar933x_uart_console_putchar > ar933x_uart_putc > > has acquired spin_lock and disabled all interrupts already. I agree, I looked at other drivers and it doesn't make sense to put that into the putc() function like Teltonika folks did in their SDK. See my from-scratch re-write following shortly. > > > ++ up->ier &= ~AR933X_UART_INT_RX_VALID; > > ++ ar933x_uart_write(up, AR933X_UART_INT_EN_REG, up->ier); > > that looks like ar933x_uart_stop_rx() copy&paste I've abstracted enabling/disabling the RX interrupt in my re-write. > > > ++ /* wait for transmission to end */ > > ++ do { > > ++ status = ar933x_uart_read(up, AR933X_UART_CS_REG); > > ++ if (--timeout == 0) > > ++ break; > > ++ udelay(1); > > ++ } while ((status & AR933X_UART_CS_TX_BUSY) != 0); > > This above looks like ar933x_uart_wait_xmitr() copy&paste but just done > differently, and I miss the point why ar933x_uart_wait_xmitr() cant be reused > here as well. There is a slight difference there: ar933x_uart_wait_xmitr() waits for the output FIFO to allow for new characters to be put on the FIFO by checking AR933X_UART_DATA_TX_CSR. This is different from checking whether the send buffer has run entirely empty and all characters have been sent on the line which is what AR933X_UART_CS_TX_BUSY checks for and what we want here. > > > ++ ar933x_uart_write(up, AR933X_UART_INT_REG, > > AR933X_UART_INT_RX_VALID); > > ++ /* remove the character from the FIFO */ > > ++ ar933x_uart_write(up, AR933X_UART_DATA_REG, > > AR933X_UART_DATA_RX_CSR); > > I really dont get this part and BTW it possibly breaks `rs485-rx-during-tx` > DTS binding. I've abstracted the half-duplex parts similar to how other drivers did in my rewrite. _______________________________________________ openwrt-devel mailing list [email protected] https://lists.openwrt.org/mailman/listinfo/openwrt-devel
