RE: [PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps driver

[Guntupalli, Manikanta]

Hi,

> -----Original Message-----
> From: m.brock@xxxxxxxxxxxxx <m.brock@xxxxxxxxxxxxx>
> Sent: Saturday, November 4, 2023 9:17 PM
> To: Guntupalli, Manikanta <manikanta.guntupalli@xxxxxxx>
> Cc: git (AMD-Xilinx) <git@xxxxxxx>; Simek, Michal
> <michal.simek@xxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx;
> robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx;
> conor+dt@xxxxxxxxxx; linux-serial@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> jirislaby@xxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; Pandey, Radhey
> Shyam <radhey.shyam.pandey@xxxxxxx>; Goud, Srinivas
> <srinivas.goud@xxxxxxx>; Datta, Shubhrajyoti
> <shubhrajyoti.datta@xxxxxxx>; manion05gk@xxxxxxxxx
> Subject: Re: [PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps
> driver
> 
> Manikanta Guntupalli wrote on 2023-10-24 16:48:
> > @@ -193,6 +196,7 @@ MODULE_PARM_DESC(rx_timeout, "Rx timeout, 1-
> 255");
> >   * @clk_rate_change_nb:	Notifier block for clock changes
> >   * @quirks:		Flags for RXBS support.
> >   * @cts_override:	Modem control state override
> > + * @gpiod:		Pointer to the gpio descriptor
> 
> Change gpiod to gpiod_rts maybe?
> Later someone might want to use a gpio for cts/dtr/dsr/dcd/ri as well.
We will fix.
> 
> >   */
> >  struct cdns_uart {
> >  	struct uart_port	*port;
> > @@ -203,10 +207,19 @@ struct cdns_uart {
> >  	struct notifier_block	clk_rate_change_nb;
> >  	u32			quirks;
> >  	bool cts_override;
> > +	struct gpio_desc	*gpiod;
> >  };
> >  struct cdns_platform_data {
> >  	u32 quirks;
> >  };
> > +
> > +struct serial_rs485 cdns_rs485_supported = {
> > +	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND |
> > +		 SER_RS485_RTS_AFTER_SEND,
> > +	.delay_rts_before_send = 1,
> > +	.delay_rts_after_send = 1,
> > +};
> > +
> >  #define to_cdns_uart(_nb) container_of(_nb, struct cdns_uart, \
> >  		clk_rate_change_nb)
> >
> > @@ -305,6 +318,79 @@ static void cdns_uart_handle_rx(void *dev_id,
> > unsigned int isrstatus)
> >  	tty_flip_buffer_push(&port->state->port);
> >  }
> >
> > +/**
> > + * cdns_rs485_config_gpio_rts_high - Configure GPIO/RTS to high
> > + * @cdns_uart: Handle to the cdns_uart  */ static void
> > +cdns_rs485_config_gpio_rts_high(struct cdns_uart
> > *cdns_uart)
> > +{
> > +	u32 val;
> > +
> > +	if (cdns_uart->gpiod) {
> > +		gpiod_set_value(cdns_uart->gpiod, 1);
> > +	} else {
> > +		val = readl(cdns_uart->port->membase +
> CDNS_UART_MODEMCR);
> > +		val &= ~CDNS_UART_MODEMCR_RTS;
> > +		writel(val, cdns_uart->port->membase +
> CDNS_UART_MODEMCR);
> > +	}
> > +}
> > +
> > +/**
> > + * cdns_rs485_config_gpio_rts_low - Configure GPIO/RTS to low
> > + * @cdns_uart: Handle to the cdns_uart  */ static void
> > +cdns_rs485_config_gpio_rts_low(struct cdns_uart
> > *cdns_uart)
> > +{
> > +	u32 val;
> > +
> > +	if (cdns_uart->gpiod) {
> > +		gpiod_set_value(cdns_uart->gpiod, 0);
> > +	} else {
> > +		val = readl(cdns_uart->port->membase +
> CDNS_UART_MODEMCR);
> > +		val |= CDNS_UART_MODEMCR_RTS;
> > +		writel(val, cdns_uart->port->membase +
> CDNS_UART_MODEMCR);
> > +	}
> > +}
> > +
> > +/**
> > + * cdns_rs485_tx_setup - Tx setup specific to rs485
> > + * @cdns_uart: Handle to the cdns_uart  */ static void
> > +cdns_rs485_tx_setup(struct cdns_uart *cdns_uart) {
> > +	if (cdns_uart->port->rs485.flags & SER_RS485_RTS_ON_SEND)
> > +		cdns_rs485_config_gpio_rts_high(cdns_uart);
> > +	else
> > +		cdns_rs485_config_gpio_rts_low(cdns_uart);
> > +}
> > +
> > +/**
> > + * cdns_rs485_rx_setup - Rx setup specific to rs485
> > + * @cdns_uart: Handle to the cdns_uart  */ static void
> > +cdns_rs485_rx_setup(struct cdns_uart *cdns_uart) {
> > +	if (cdns_uart->port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
> > +		cdns_rs485_config_gpio_rts_high(cdns_uart);
> > +	else
> > +		cdns_rs485_config_gpio_rts_low(cdns_uart);
> > +}
> 
> Why not simply create:
> void cdns_rs485_driver_enable(struct cdns_uart *cdns_uart, bool enable)
We will fix.
> 
> And let it handle the rs485.flags itself?
We will fix.
> 
> > +
> > +/**
> > + * cdns_uart_tx_empty -  Check whether TX is empty
> > + * @port: Handle to the uart port structure
> > + *
> > + * Return: TIOCSER_TEMT on success, 0 otherwise  */ static unsigned
> > +int cdns_uart_tx_empty(struct uart_port *port) {
> > +	unsigned int status;
> > +
> > +	status = readl(port->membase + CDNS_UART_SR) &
> > +		       (CDNS_UART_SR_TXEMPTY | CDNS_UART_SR_TACTIVE);
> > +	return (status == CDNS_UART_SR_TXEMPTY) ? TIOCSER_TEMT : 0; }
> > +
> >  /**
> >   * cdns_uart_handle_tx - Handle the bytes to be Txed.
> >   * @dev_id: Id of the UART port
> > @@ -571,6 +657,8 @@ static int cdns_uart_clk_notifier_cb(struct
> > notifier_block *nb,  static void cdns_uart_start_tx(struct uart_port
> > *port)  {
> >  	unsigned int status;
> > +	unsigned long time_out;
> > +	struct cdns_uart *cdns_uart = port->private_data;
> >
> >  	if (uart_tx_stopped(port))
> >  		return;
> > @@ -589,8 +677,31 @@ static void cdns_uart_start_tx(struct uart_port
> > *port)
> >
> >  	writel(CDNS_UART_IXR_TXEMPTY, port->membase +
> CDNS_UART_ISR);
> > +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> > +		cdns_rs485_tx_setup(cdns_uart);
> > +		if (cdns_uart->port->rs485.delay_rts_before_send)
> > +			mdelay(cdns_uart->port-
> >rs485.delay_rts_before_send);
> 
> Would it not be better to start a timer here with a callback that enables the
> TXEMPTY interrupt? That would automatically call cdns_uart_handle_tx().
We will fix.
> 
> > +	}
> > +
> >  	cdns_uart_handle_tx(port);
> >
> > +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> > +		time_out = jiffies + usecs_to_jiffies(TX_TIMEOUT);
> > +		/* Wait for tx completion */
> > +		while ((cdns_uart_tx_empty(cdns_uart->port) !=
> TIOCSER_TEMT) &&
> > +		       time_before(jiffies, time_out))
> > +			cpu_relax();
> > +
> > +		if (cdns_uart->port->rs485.delay_rts_after_send)
> > +			mdelay(cdns_uart->port-
> >rs485.delay_rts_after_send);
> > +
> > +		/*
> > +		 * Default Rx should be setup, because RX signaling path
> > +		 * need to enable to receive data.
> > +		 */
> > +		cdns_rs485_rx_setup(cdns_uart);
> > +	}
> 
> I think this should be done from the TXEMPTY interrupt. And again schedule a
> timer to drop the DE line. You really can do this without using mdelay().
We will get TXEMPTY interrupt multiple times for large data, so doing this from
TXEMPTY interrupt will call this logic multiple times, but this logic need be to called once.
> 
> > +
> >  	/* Enable the TX Empty interrupt */
> >  	writel(CDNS_UART_IXR_TXEMPTY, port->membase +
> CDNS_UART_IER);  } @@
> > -602,6 +713,14 @@ static void cdns_uart_start_tx(struct uart_port
> > *port)
> >  static void cdns_uart_stop_tx(struct uart_port *port)  {
> >  	unsigned int regval;
> > +	struct cdns_uart *cdns_uart = port->private_data;
> > +
> > +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> > +		if (cdns_uart->port->rs485.delay_rts_after_send)
> > +			mdelay(cdns_uart->port-
> >rs485.delay_rts_after_send);
> > +
> > +		cdns_rs485_rx_setup(cdns_uart);
> > +	}
> 
> Again, start a timer and wait for completion?
We will fix.
> 
> >  	regval = readl(port->membase + CDNS_UART_CR);
> >  	regval |= CDNS_UART_CR_TX_DIS;
> > @@ -626,21 +745,6 @@ static void cdns_uart_stop_rx(struct uart_port
> > *port)
> >  	writel(regval, port->membase + CDNS_UART_CR);
> >  }
> >
> > -/**
> > - * cdns_uart_tx_empty -  Check whether TX is empty
> > - * @port: Handle to the uart port structure
> > - *
> > - * Return: TIOCSER_TEMT on success, 0 otherwise
> > - */
> > -static unsigned int cdns_uart_tx_empty(struct uart_port *port)
> > -{
> > -	unsigned int status;
> > -
> > -	status = readl(port->membase + CDNS_UART_SR) &
> > -		       (CDNS_UART_SR_TXEMPTY | CDNS_UART_SR_TACTIVE);
> > -	return (status == CDNS_UART_SR_TXEMPTY) ? TIOCSER_TEMT : 0;
> > -}
> > -
> >  /**
> >   * cdns_uart_break_ctl - Based on the input ctl we have to start or
> > stop
> >   *			transmitting char breaks
> > @@ -829,6 +933,9 @@ static int cdns_uart_startup(struct uart_port
> > *port)
> >  		(CDNS_UART_CR_TXRST | CDNS_UART_CR_RXRST))
> >  		cpu_relax();
> >
> > +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED)
> > +		cdns_rs485_rx_setup(cdns_uart);
> > +
> >  	/*
> >  	 * Clear the RX disable bit and then set the RX enable bit to enable
> >  	 * the receiver.
> > @@ -1455,6 +1562,23 @@ MODULE_DEVICE_TABLE(of,
> cdns_uart_of_match);
> >  /* Temporary variable for storing number of instances */
> >  static int instances;
> >
> > +/**
> > + * cdns_rs485_config - Called when an application calls TIOCSRS485
> > ioctl.
> > + * @port: Pointer to the uart_port structure
> > + * @termios: Pointer to the ktermios structure
> > + * @rs485: Pointer to the serial_rs485 structure
> > + *
> > + * Return: 0
> > + */
> > +static int cdns_rs485_config(struct uart_port *port, struct ktermios
> > *termios,
> > +			     struct serial_rs485 *rs485)
> > +{
> > +	if (rs485->flags & SER_RS485_ENABLED)
> > +		dev_dbg(port->dev, "Setting UART to RS485\n");
> > +
> 
> Shouldn't you force automatic RTS control off here?
> And also call cdns_rs485_rx_setup()
We will fix.
> 
> > +	return 0;
> > +}
> > +
> >  /**
> >   * cdns_uart_probe - Platform driver probe
> >   * @pdev: Pointer to the platform device structure
> > @@ -1463,6 +1587,7 @@ static int instances;
> >   */
> >  static int cdns_uart_probe(struct platform_device *pdev)
> >  {
> > +	u32 val;
> >  	int rc, id, irq;
> >  	struct uart_port *port;
> >  	struct resource *res;
> > @@ -1597,9 +1722,23 @@ static int cdns_uart_probe(struct
> > platform_device *pdev)
> >  	port->private_data = cdns_uart_data;
> >  	port->read_status_mask = CDNS_UART_IXR_TXEMPTY |
> CDNS_UART_IXR_RXTRIG
> > |
> >  			CDNS_UART_IXR_OVERRUN | CDNS_UART_IXR_TOUT;
> > +	port->rs485_config = cdns_rs485_config;
> > +	port->rs485_supported = cdns_rs485_supported;
> >  	cdns_uart_data->port = port;
> >  	platform_set_drvdata(pdev, port);
> >
> > +	rc = uart_get_rs485_mode(port);
> > +	if (rc)
> > +		goto err_out_clk_notifier;
> > +
> > +	cdns_uart_data->gpiod = devm_gpiod_get_optional(&pdev->dev,
> "rts",
> > +							GPIOD_OUT_LOW);
> > +	if (IS_ERR(cdns_uart_data->gpiod)) {
> > +		rc = PTR_ERR(cdns_uart_data->gpiod);
> > +		dev_err(port->dev, "xuartps: devm_gpiod_get_optional
> failed\n");
> > +		goto err_out_clk_notifier;
> > +	}
> > +
> >  	pm_runtime_use_autosuspend(&pdev->dev);
> >  	pm_runtime_set_autosuspend_delay(&pdev->dev,
> > UART_AUTOSUSPEND_TIMEOUT);
> >  	pm_runtime_set_active(&pdev->dev);
> > @@ -1638,6 +1777,16 @@ static int cdns_uart_probe(struct
> > platform_device *pdev)
> >  	cdns_uart_data->cts_override =
> > of_property_read_bool(pdev->dev.of_node,
> >  							     "cts-override");
> >
> > +	if (cdns_uart_data->port->rs485.flags & SER_RS485_ENABLED) {
> > +		if (!cdns_uart_data->gpiod) {
> > +			val = readl(cdns_uart_data->port->membase
> > +				    + CDNS_UART_MODEMCR);
> > +			val |= CDNS_UART_MODEMCR_RTS;
> > +			writel(val, cdns_uart_data->port->membase
> > +			       + CDNS_UART_MODEMCR);
> > +		}
> > +	}
> 
> Simply call cdns_rs485_rx_setup() ?
We will fix.
> 
> > +
> >  	instances++;
> >
> >  	return 0;
> > @@ -1646,6 +1795,7 @@ static int cdns_uart_probe(struct platform_device
> > *pdev)
> >  	pm_runtime_disable(&pdev->dev);
> >  	pm_runtime_set_suspended(&pdev->dev);
> >  	pm_runtime_dont_use_autosuspend(&pdev->dev);
> > +err_out_clk_notifier:
> >  #ifdef CONFIG_COMMON_CLK
> >  	clk_notifier_unregister(cdns_uart_data->uartclk,
> >  			&cdns_uart_data->clk_rate_change_nb);
> 
> Please also modify cdns_uart_[s|g]et_mctrl() so they support rts-gpios.
We will fix.
> 

Thanks,
Manikanta.