Re: [PATCH 4/5] dt-bindings: serial: lantiq: Update for new SoC

Re: [PATCH 4/5] dt-bindings: serial: lantiq: Update for new SoC

[Andy Shevchenko]

On Wed, Aug 07, 2019 at 05:21:34PM +0800, Rahul Tanwar wrote:
> There is a new Intel Atom based Lightning Mountain(LGM) network processor SoC which
> reuses Lantiq ASC serial controller IP. This patch adds new compatible string
> and its expected property value in order to support the driver for LGM as well.

I think it makes sense to convert to YAML before adding new properties.

> 
> Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
> ---
>  Documentation/devicetree/bindings/serial/lantiq_asc.txt | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/serial/lantiq_asc.txt b/Documentation/devicetree/bindings/serial/lantiq_asc.txt
> index 40e81a5818f6..18b45dd13a61 100644
> --- a/Documentation/devicetree/bindings/serial/lantiq_asc.txt
> +++ b/Documentation/devicetree/bindings/serial/lantiq_asc.txt
> @@ -1,10 +1,14 @@
>  Lantiq SoC ASC serial controller
>  
>  Required properties:
> -- compatible : Should be "lantiq,asc"
> +- compatible : Should be "lantiq,asc" or "intel,lgm-asc"
>  - reg : Address and length of the register set for the device
> -- interrupts: the 3 (tx rx err) interrupt numbers. The interrupt specifier
> +- interrupts:
> +  For "lantiq,asc" - the 3 (tx rx err) interrupt numbers. The interrupt specifier
>    depends on the interrupt-parent interrupt controller.
> +	or
> +  For "intel,lgm-asc" - the common interrupt number for all of tx rx & err interrupts
> +  followed by level/sense specifier.
>  
>  Optional properties:
>  - clocks: Should contain frequency clock and gate clock
> @@ -29,3 +33,12 @@ asc1: serial@e100c00 {
>  	interrupt-parent = <&icu0>;
>  	interrupts = <112 113 114>;
>  };
> +
> +asc0: serial@e0a00000 {
> +	compatible = "intel,lgm-asc";
> +	reg = <0xe0a00000 0x1000>;
> +	interrupt-parent = <&ioapic1>;
> +	interrupts = <128 1>;
> +	clocks = <&cgu0 LGM_CLK_NOC4>, <&cgu0 LGM_GCLK_ASC0>;
> +	clock-names = "freq", "asc";
> +};
> -- 
> 2.11.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/5] serial: lantiq: Add SMP support

[Andy Shevchenko]

On Wed, Aug 07, 2019 at 05:21:31PM +0800, Rahul Tanwar wrote:
> The existing driver can only support single core SoC. But new multicore
> platforms which reuse the same driver/IP need SMP support. This patch adds
> multicore support in the driver.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

> 
> Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
> ---
>  drivers/tty/serial/lantiq.c | 47 ++++++++++++++++++++++++++++++---------------
>  1 file changed, 32 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c
> index 9de9f0f239a1..42e27b48e9cc 100644
> --- a/drivers/tty/serial/lantiq.c
> +++ b/drivers/tty/serial/lantiq.c
> @@ -99,7 +99,6 @@
>  static void lqasc_tx_chars(struct uart_port *port);
>  static struct ltq_uart_port *lqasc_port[MAXPORTS];
>  static struct uart_driver lqasc_reg;
> -static DEFINE_SPINLOCK(ltq_asc_lock);
>  
>  struct ltq_uart_port {
>  	struct uart_port	port;
> @@ -110,6 +109,7 @@ struct ltq_uart_port {
>  	unsigned int		tx_irq;
>  	unsigned int		rx_irq;
>  	unsigned int		err_irq;
> +	spinlock_t		lock; /* exclusive access for multi core */
>  };
>  
>  static inline void asc_update_bits(u32 clear, u32 set, void __iomem *reg)
> @@ -135,9 +135,11 @@ static void
>  lqasc_start_tx(struct uart_port *port)
>  {
>  	unsigned long flags;
> -	spin_lock_irqsave(&ltq_asc_lock, flags);
> +	struct ltq_uart_port *ltq_port = to_ltq_uart_port(port);
> +
> +	spin_lock_irqsave(&ltq_port->lock, flags);
>  	lqasc_tx_chars(port);
> -	spin_unlock_irqrestore(&ltq_asc_lock, flags);
> +	spin_unlock_irqrestore(&ltq_port->lock, flags);
>  	return;
>  }
>  
> @@ -245,9 +247,11 @@ lqasc_tx_int(int irq, void *_port)
>  {
>  	unsigned long flags;
>  	struct uart_port *port = (struct uart_port *)_port;
> -	spin_lock_irqsave(&ltq_asc_lock, flags);
> +	struct ltq_uart_port *ltq_port = to_ltq_uart_port(port);
> +
> +	spin_lock_irqsave(&ltq_port->lock, flags);
>  	__raw_writel(ASC_IRNCR_TIR, port->membase + LTQ_ASC_IRNCR);
> -	spin_unlock_irqrestore(&ltq_asc_lock, flags);
> +	spin_unlock_irqrestore(&ltq_port->lock, flags);
>  	lqasc_start_tx(port);
>  	return IRQ_HANDLED;
>  }
> @@ -257,11 +261,13 @@ lqasc_err_int(int irq, void *_port)
>  {
>  	unsigned long flags;
>  	struct uart_port *port = (struct uart_port *)_port;
> -	spin_lock_irqsave(&ltq_asc_lock, flags);
> +	struct ltq_uart_port *ltq_port = to_ltq_uart_port(port);
> +
> +	spin_lock_irqsave(&ltq_port->lock, flags);
>  	/* clear any pending interrupts */
>  	asc_update_bits(0, ASCWHBSTATE_CLRPE | ASCWHBSTATE_CLRFE |
>  		ASCWHBSTATE_CLRROE, port->membase + LTQ_ASC_WHBSTATE);
> -	spin_unlock_irqrestore(&ltq_asc_lock, flags);
> +	spin_unlock_irqrestore(&ltq_port->lock, flags);
>  	return IRQ_HANDLED;
>  }
>  
> @@ -270,10 +276,12 @@ lqasc_rx_int(int irq, void *_port)
>  {
>  	unsigned long flags;
>  	struct uart_port *port = (struct uart_port *)_port;
> -	spin_lock_irqsave(&ltq_asc_lock, flags);
> +	struct ltq_uart_port *ltq_port = to_ltq_uart_port(port);
> +
> +	spin_lock_irqsave(&ltq_port->lock, flags);
>  	__raw_writel(ASC_IRNCR_RIR, port->membase + LTQ_ASC_IRNCR);
>  	lqasc_rx_chars(port);
> -	spin_unlock_irqrestore(&ltq_asc_lock, flags);
> +	spin_unlock_irqrestore(&ltq_port->lock, flags);
>  	return IRQ_HANDLED;
>  }
>  
> @@ -307,11 +315,13 @@ lqasc_startup(struct uart_port *port)
>  {
>  	struct ltq_uart_port *ltq_port = to_ltq_uart_port(port);
>  	int retval;
> +	unsigned long flags;
>  
>  	if (!IS_ERR(ltq_port->clk))
>  		clk_prepare_enable(ltq_port->clk);
>  	port->uartclk = clk_get_rate(ltq_port->freqclk);
>  
> +	spin_lock_irqsave(&ltq_port->lock, flags);
>  	asc_update_bits(ASCCLC_DISS | ASCCLC_RMCMASK, (1 << ASCCLC_RMCOFFSET),
>  		port->membase + LTQ_ASC_CLC);
>  
> @@ -331,6 +341,8 @@ lqasc_startup(struct uart_port *port)
>  	asc_update_bits(0, ASCCON_M_8ASYNC | ASCCON_FEN | ASCCON_TOEN |
>  		ASCCON_ROEN, port->membase + LTQ_ASC_CON);
>  
> +	spin_unlock_irqrestore(&ltq_port->lock, flags);
> +
>  	retval = request_irq(ltq_port->tx_irq, lqasc_tx_int,
>  		0, "asc_tx", port);
>  	if (retval) {
> @@ -367,15 +379,19 @@ static void
>  lqasc_shutdown(struct uart_port *port)
>  {
>  	struct ltq_uart_port *ltq_port = to_ltq_uart_port(port);
> +	unsigned long flags;
> +
>  	free_irq(ltq_port->tx_irq, port);
>  	free_irq(ltq_port->rx_irq, port);
>  	free_irq(ltq_port->err_irq, port);
>  
> +	spin_lock_irqsave(&ltq_port->lock, flags);
>  	__raw_writel(0, port->membase + LTQ_ASC_CON);
>  	asc_update_bits(ASCRXFCON_RXFEN, ASCRXFCON_RXFFLU,
>  		port->membase + LTQ_ASC_RXFCON);
>  	asc_update_bits(ASCTXFCON_TXFEN, ASCTXFCON_TXFFLU,
>  		port->membase + LTQ_ASC_TXFCON);
> +	spin_unlock_irqrestore(&ltq_port->lock, flags);
>  	if (!IS_ERR(ltq_port->clk))
>  		clk_disable_unprepare(ltq_port->clk);
>  }
> @@ -390,6 +406,7 @@ lqasc_set_termios(struct uart_port *port,
>  	unsigned int baud;
>  	unsigned int con = 0;
>  	unsigned long flags;
> +	struct ltq_uart_port *ltq_port = to_ltq_uart_port(port);
>  
>  	cflag = new->c_cflag;
>  	iflag = new->c_iflag;
> @@ -443,7 +460,7 @@ lqasc_set_termios(struct uart_port *port,
>  	/* set error signals  - framing, parity  and overrun, enable receiver */
>  	con |= ASCCON_FEN | ASCCON_TOEN | ASCCON_ROEN;
>  
> -	spin_lock_irqsave(&ltq_asc_lock, flags);
> +	spin_lock_irqsave(&ltq_port->lock, flags);
>  
>  	/* set up CON */
>  	asc_update_bits(0, con, port->membase + LTQ_ASC_CON);
> @@ -471,7 +488,7 @@ lqasc_set_termios(struct uart_port *port,
>  	/* enable rx */
>  	__raw_writel(ASCWHBSTATE_SETREN, port->membase + LTQ_ASC_WHBSTATE);
>  
> -	spin_unlock_irqrestore(&ltq_asc_lock, flags);
> +	spin_unlock_irqrestore(&ltq_port->lock, flags);
>  
>  	/* Don't rewrite B0 */
>  	if (tty_termios_baud_rate(new))
> @@ -589,17 +606,14 @@ lqasc_console_putchar(struct uart_port *port, int ch)
>  static void lqasc_serial_port_write(struct uart_port *port, const char *s,
>  				    u_int count)
>  {
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&ltq_asc_lock, flags);
>  	uart_console_write(port, s, count, lqasc_console_putchar);
> -	spin_unlock_irqrestore(&ltq_asc_lock, flags);
>  }
>  
>  static void
>  lqasc_console_write(struct console *co, const char *s, u_int count)
>  {
>  	struct ltq_uart_port *ltq_port;
> +	unsigned long flags;
>  
>  	if (co->index >= MAXPORTS)
>  		return;
> @@ -608,7 +622,9 @@ lqasc_console_write(struct console *co, const char *s, u_int count)
>  	if (!ltq_port)
>  		return;
>  
> +	spin_lock_irqsave(&ltq_port->lock, flags);
>  	lqasc_serial_port_write(&ltq_port->port, s, count);
> +	spin_unlock_irqrestore(&ltq_port->lock, flags);
>  }
>  
>  static int __init
> @@ -766,6 +782,7 @@ lqasc_probe(struct platform_device *pdev)
>  	ltq_port->rx_irq = irqres[1].start;
>  	ltq_port->err_irq = irqres[2].start;
>  
> +	spin_lock_init(&ltq_port->lock);
>  	lqasc_port[line] = ltq_port;
>  	platform_set_drvdata(pdev, ltq_port);
>  
> -- 
> 2.11.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/5] serial: lantiq: Use proper DT compatible string

[Andy Shevchenko]

On Wed, Aug 07, 2019 at 05:21:32PM +0800, Rahul Tanwar wrote:
> The patch adds change to use explicit string instead of a macro for
> DT compatible string.

For consistency you may need to convert OF_EARLYCON_DECLARE() as well.

Perhaps commit message should explain the rationale, i.e. the following patches
will add another compatible string and thus it makes sense to have them
explicitly mentioned.

> Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
> ---
>  drivers/tty/serial/lantiq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c
> index 42e27b48e9cc..1116261c973e 100644
> --- a/drivers/tty/serial/lantiq.c
> +++ b/drivers/tty/serial/lantiq.c
> @@ -792,7 +792,7 @@ lqasc_probe(struct platform_device *pdev)
>  }
>  
>  static const struct of_device_id ltq_asc_match[] = {
> -	{ .compatible = DRVNAME },
> +	{ .compatible = "lantiq,asc" },
>  	{},
>  };
>  
> -- 
> 2.11.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/5] serial: lantiq: Make IRQ & ISR assignment dynamic

[Andy Shevchenko]

On Wed, Aug 07, 2019 at 05:21:33PM +0800, Rahul Tanwar wrote:
> This driver/IP is reused across multiple SoCs. Older SoCs supported three
> separate IRQs for tx, rx & err interrupts. Newer Lightning Mountain SoC
> supports single IRQ for all of tx/rx/err interrupts. This patch modifies
> the driver design to support dynamic assignment of IRQ resources & ISRs
> based on devicetree node compatible entries.

> +struct ltq_soc_data {
> +	int	(*fetch_irq)(struct platform_device *pdev,
> +				 struct ltq_uart_port *ltq_port);

This can be simple

	int	(*fetch_irq)(struct device *dev, struct ltq_uart_port *ltq_port);

(Note one line and struct device instead of platform_device)

> +	int	(*request_irq)(struct uart_port *port);
> +	void	(*free_irq)(struct uart_port *port);
> +};

> +	retval = ltq_port->soc->request_irq(port);
> +	if(retval)

Space is missed.

>  		return retval;


> +static int fetch_irq_lantiq(struct platform_device *pdev,
> +			    struct ltq_uart_port *ltq_port)
> +{
> +	struct device_node *node = pdev->dev.of_node;
> +	struct uart_port *port = &ltq_port->port;
> +	struct resource irqres[3];
> +	int ret;
> +
> +	ret = of_irq_to_resource_table(node, irqres, 3);
> +	if (ret != 3) {
> +		dev_err(&pdev->dev,
> +			"failed to get IRQs for serial port\n");
> +		return -ENODEV;
> +	}
> +	ltq_port->tx_irq = irqres[0].start;
> +	ltq_port->rx_irq = irqres[1].start;
> +	ltq_port->err_irq = irqres[2].start;
> +	port->irq = irqres[0].start;
> +

> +	return ret;

I'm not sure you need to return known value. 0 will be good enough...

> +}

> +	ltq_port->soc = of_device_get_match_data(&pdev->dev);
> +	ret = ltq_port->soc->fetch_irq(pdev, ltq_port);

> +	if (ret < 0)

...and thus simple...

	if (ret)

...may be used.

> +		return ret;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/5] serial: lantiq: Add support for Lightning Mountain SoC

[Andy Shevchenko]

On Wed, Aug 07, 2019 at 05:21:35PM +0800, Rahul Tanwar wrote:
> This patch adds IRQ & ISR support in the driver for Lightning Mountain SoC.

> +#define ASC_IRNCR_MASK		0x7

GENMASK() ?

> +static irqreturn_t lqasc_irq(int irq, void *p)
> +{
> +	unsigned long flags;
> +	u32 stat;
> +	struct uart_port *port = p;
> +	struct ltq_uart_port *ltq_port = to_ltq_uart_port(port);
> +
> +	spin_lock_irqsave(&ltq_port->lock, flags);
> +	stat = readl(port->membase + LTQ_ASC_IRNCR);
> +	if (!(stat & ASC_IRNCR_MASK)) {
> +		spin_unlock_irqrestore(&ltq_port->lock, flags);
> +		return IRQ_NONE;
> +	}

> +	spin_unlock_irqrestore(&ltq_port->lock, flags);

Are you sure the below does not need a serialization?

If it's not the case, you may unlock the lock immediately after readl().

> +
> +	if (stat & ASC_IRNCR_TIR)
> +		lqasc_tx_int(irq, p);
> +
> +	if (stat & ASC_IRNCR_RIR)
> +		lqasc_rx_int(irq, p);
> +
> +	if (stat & ASC_IRNCR_EIR)
> +		lqasc_err_int(irq, p);
> +
> +	return IRQ_HANDLED;
> +}

> +static int fetch_irq_intel(struct platform_device *pdev,
> +			   struct ltq_uart_port *ltq_port)
> +{
> +	struct uart_port *port = &ltq_port->port;
> +	int ret;
> +
> +	ret = of_irq_get(pdev->dev.of_node, 0);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev,
> +			"failed to fetch IRQ for serial port\n");

> +		return -ENODEV;

	return ret;

> +	}
> +	ltq_port->common_irq = ret;
> +	port->irq = ret;
> +

> +	return ret;

Same as per patch 3, i.e.

	return 0;

> +}

> +static int request_irq_intel(struct uart_port *port)
> +{
> +	struct ltq_uart_port *ltq_port = to_ltq_uart_port(port);
> +	int retval;
> +
> +	retval = request_irq(ltq_port->common_irq, lqasc_irq, 0,
> +			     "asc_irq", port);

> +	if (retval) {
> +		dev_err(port->dev, "failed to request asc_irq\n");
> +		return retval;
> +	}
> +
> +	return 0;

	if (retval)
		dev_err();

	return retval;

> +}

> +
> +

One blank line is enough.

-- 
With Best Regards,
Andy Shevchenko