[OpenWrt-Devel] [PATCH v4 1/2] serial: ar933x_uart: add rs485 support

[Daniel Golle]

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 <daniel at makrotopia.org> [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
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel