[PATCH] serial: 8250: Add cpufreq support

[PATCH] serial: 8250: Add cpufreq support

[Chaithrika U S]

On DA850/OMAP-L138 SoC, the PLL which supplies the clock to CPU also feeds the
UART and the UART input frequency can change when the CPU frequency is scaled.

This patch adds cpufreq support for 8250 serial driver. A clk structure member
has been added to the platform and port data structures. This member is used by
the cpufreq notifier callback to get the updated clock rate. The implementation
is based on the cpufreq implementation for Samsung serial driver.

Tested on TI DA850/OMAP-L138 EVM.

Signed-off-by: Chaithrika U S <chaithrika@ti.com>
---
Applies to Linus' kernel tree.

 drivers/serial/8250.c       |   79 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/serial_8250.h |    1 +
 2 files changed, 80 insertions(+), 0 deletions(-)

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index c3e37c8..a64d1de 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -38,6 +38,8 @@
 #include <linux/serial_8250.h>
 #include <linux/nmi.h>
 #include <linux/mutex.h>
+#include <linux/cpufreq.h>
+#include <linux/clk.h>
 
 #include <asm/io.h>
 #include <asm/irq.h>
@@ -156,6 +158,10 @@ struct uart_8250_port {
 	 */
 	void			(*pm)(struct uart_port *port,
 				      unsigned int state, unsigned int old);
+	struct clk		*clk;
+#ifdef CONFIG_CPU_FREQ
+	struct notifier_block	freq_transition;
+#endif
 };
 
 struct irq_info {
@@ -2931,6 +2937,70 @@ void serial8250_resume_port(int line)
 	uart_resume_port(&serial8250_reg, &up->port);
 }
 
+#ifdef CONFIG_CPU_FREQ
+static int serial8250_cpufreq_transition(struct notifier_block *nb,
+					     unsigned long val, void *data)
+{
+	struct uart_8250_port *p;
+	struct uart_port *uport;
+
+	p = container_of(nb, struct uart_8250_port, freq_transition);
+	uport = &p->port;
+
+	if (IS_ERR(p->clk))
+		goto cpu_freq_exit;
+
+	if (p->port.uartclk == clk_get_rate(p->clk))
+		goto cpu_freq_exit;
+
+	p->port.uartclk = clk_get_rate(p->clk);
+	if (val == CPUFREQ_POSTCHANGE) {
+		struct ktermios *termios;
+		struct tty_struct *tty;
+		if (uport->state == NULL)
+			goto cpu_freq_exit;
+
+		tty = uport->state->port.tty;
+		if (tty == NULL)
+			goto cpu_freq_exit;
+
+		termios = tty->termios;
+		if (termios == NULL) {
+			printk(KERN_WARNING "%s: no termios?\n", __func__);
+			goto cpu_freq_exit;
+		}
+
+		serial8250_set_termios(uport, termios, NULL);
+	}
+
+cpu_freq_exit:
+	return 0;
+}
+
+static inline int serial8250_cpufreq_register(struct uart_8250_port *p)
+{
+	p->freq_transition.notifier_call = serial8250_cpufreq_transition;
+
+	return cpufreq_register_notifier(&p->freq_transition,
+					 CPUFREQ_TRANSITION_NOTIFIER);
+}
+
+static inline void serial8250_cpufreq_deregister(struct uart_8250_port *p)
+{
+	cpufreq_unregister_notifier(&p->freq_transition,
+				    CPUFREQ_TRANSITION_NOTIFIER);
+}
+#else
+static inline int serial8250_cpufreq_register(struct uart_8250_port *p)
+{
+	return 0;
+}
+
+static inline void serial8250_cpufreq_deregister(struct uart_8250_port *p)
+{
+}
+#endif
+
 /*
  * Register a set of serial devices attached to a platform device.  The
  * list is terminated with a zero flags entry, which means we expect
@@ -2964,6 +3034,9 @@ static int __devinit serial8250_probe(struct platform_device *dev)
 		port.serial_out		= p->serial_out;
 		port.dev		= &dev->dev;
 		port.irqflags		|= irqflag;
+		if (p->clk)
+			serial8250_ports[i].clk = p->clk;
+
 		ret = serial8250_register_port(&port);
 		if (ret < 0) {
 			dev_err(&dev->dev, "unable to register port at index %d "
@@ -3133,6 +3206,11 @@ int serial8250_register_port(struct uart_port *port)
 		ret = uart_add_one_port(&serial8250_reg, &uart->port);
 		if (ret == 0)
 			ret = uart->port.line;
+
+		ret = serial8250_cpufreq_register(uart);
+		if (ret < 0)
+			printk(KERN_ERR "Failed to add cpufreq notifier\n");
+
 	}
 	mutex_unlock(&serial_mutex);
 
@@ -3152,6 +3230,7 @@ void serial8250_unregister_port(int line)
 	struct uart_8250_port *uart = &serial8250_ports[line];
 
 	mutex_lock(&serial_mutex);
+	serial8250_cpufreq_deregister(uart);
 	uart_remove_one_port(&serial8250_reg, &uart->port);
 	if (serial8250_isa_devs) {
 		uart->port.flags &= ~UPF_BOOT_AUTOCONF;
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index fb46aba..57d6e69 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -23,6 +23,7 @@ struct plat_serial8250_port {
 	resource_size_t	mapbase;	/* resource base */
 	unsigned int	irq;		/* interrupt number */
 	unsigned long	irqflags;	/* request_irq flags */
+	struct clk	*clk;
 	unsigned int	uartclk;	/* UART clock rate */
 	void            *private_data;
 	unsigned char	regshift;	/* register shift */
-- 
1.5.6

Re: [PATCH] serial: 8250: Add cpufreq support

[Alan Cox]

> +static int serial8250_cpufreq_transition(struct notifier_block *nb,
> +					     unsigned long val, void *data)
> +{
> +	struct uart_8250_port *p;
> +	struct uart_port *uport;
> +
> +	p = container_of(nb, struct uart_8250_port, freq_transition);
> +	uport = &p->port;
> +
> +	if (IS_ERR(p->clk))
> +		goto cpu_freq_exit;
> +
> +	if (p->port.uartclk == clk_get_rate(p->clk))
> +		goto cpu_freq_exit;
> +
> +	p->port.uartclk = clk_get_rate(p->clk);
> +	if (val == CPUFREQ_POSTCHANGE) {
> +		struct ktermios *termios;
> +		struct tty_struct *tty;
> +		if (uport->state == NULL)
> +			goto cpu_freq_exit;
> +
> +		tty = uport->state->port.tty;

Need locking on port->tty.

> +		if (tty == NULL)
> +			goto cpu_freq_exit;
> +

Need locking on tty->termios

I'd say this is also probably at the wrong level - why not do it at the
uart_port level instead (ie move the code you have into serial_core using
uart_port) - you've made it all nicely generic already.

Alan

RE: [PATCH] serial: 8250: Add cpufreq support

[Chaithrika U S]

On Wed, Jan 13, 2010 at 19:14:45, Alan Cox wrote:
> > +static int serial8250_cpufreq_transition(struct notifier_block *nb,
> > +					     unsigned long val, void *data)
> > +{
> > +	struct uart_8250_port *p;
> > +	struct uart_port *uport;
> > +
> > +	p = container_of(nb, struct uart_8250_port, freq_transition);
> > +	uport = &p->port;
> > +
> > +	if (IS_ERR(p->clk))
> > +		goto cpu_freq_exit;
> > +
> > +	if (p->port.uartclk == clk_get_rate(p->clk))
> > +		goto cpu_freq_exit;
> > +
> > +	p->port.uartclk = clk_get_rate(p->clk);
> > +	if (val == CPUFREQ_POSTCHANGE) {
> > +		struct ktermios *termios;
> > +		struct tty_struct *tty;
> > +		if (uport->state == NULL)
> > +			goto cpu_freq_exit;
> > +
> > +		tty = uport->state->port.tty;
> 
> Need locking on port->tty.
> 
> > +		if (tty == NULL)
> > +			goto cpu_freq_exit;
> > +
> 
> Need locking on tty->termios
> 
> I'd say this is also probably at the wrong level - why not do it at the
> uart_port level instead (ie move the code you have into serial_core using
> uart_port) - you've made it all nicely generic already.
> 
> Alan
> 	
	
Thank you for the review comments.
I will submit an updated version of patch soon.

Regards, 
Chaithrika

[PATCH] serial: 8250: Add cpufreq support

[Shankarmurthy, Akshay]

From: Chaithrika U S <chaithrika@ti.com>

On DA850/OMAP-L138 SoC, the PLL which supplies the clock to CPU also
feeds the UART and the UART input frequency can change when the CPU
frequency is scaled.

This patch adds cpufreq support for 8250 serial driver. A clk structure
member has been added to the platform and port data structures.This
member is used by the cpufreq notifier callback to get the updated
clock rate. The implementation is based on the cpufreq implementation
for Samsung serial driver.

Tested on TI DA850/OMAP-L138 EVM.

Signed-off-by: Chaithrika U S <chaithrika@ti.com>
Signed-off-by: Shankarmurthy, Akshay <akshay.s@ti.com>
---
This patch was submitted 2 years ago but didn't make it to the mainline. Now i am reposting it.

 drivers/tty/serial/8250/8250.c |   74 ++++++++++++++++++++++++++++++++++++++++
 drivers/tty/serial/8250/8250.h |    5 +++
 include/linux/serial_8250.h    |    1 +
 3 files changed, 80 insertions(+), 0 deletions(-)

diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c
index 9b7336f..b7cfe6c 100644
--- a/drivers/tty/serial/8250/8250.c
+++ b/drivers/tty/serial/8250/8250.c
@@ -38,6 +38,8 @@
 #include <linux/nmi.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
+#include <linux/cpufreq.h>
+#include <linux/clk.h>
 
 #include <asm/io.h>
 #include <asm/irq.h>
@@ -3015,6 +3017,70 @@ void serial8250_resume_port(int line)
 	uart_resume_port(&serial8250_reg, &up->port);
 }
 
+#ifdef CONFIG_CPU_FREQ
+static int serial8250_cpufreq_transition(struct notifier_block *nb,
+					     unsigned long val, void *data)
+{
+	struct uart_8250_port *p;
+	struct uart_port *uport;
+
+	p = container_of(nb, struct uart_8250_port, freq_transition);
+	uport = &p->port;
+
+	if (IS_ERR(p->clk))
+		goto cpu_freq_exit;
+
+	if (p->port.uartclk == clk_get_rate(p->clk))
+		goto cpu_freq_exit;
+
+	p->port.uartclk = clk_get_rate(p->clk);
+	if (val == CPUFREQ_POSTCHANGE) {
+		struct ktermios *termios;
+		struct tty_struct *tty;
+		if (uport->state == NULL)
+			goto cpu_freq_exit;
+
+		tty = uport->state->port.tty;
+		if (tty == NULL)
+			goto cpu_freq_exit;
+
+		termios = tty->termios;
+		if (termios == NULL) {
+			printk(KERN_WARNING "%s: no termios?\n", __func__);
+			goto cpu_freq_exit;
+		}
+
+		serial8250_set_termios(uport, termios, NULL);
+	}
+
+cpu_freq_exit:
+	return 0;
+}
+
+static inline int serial8250_cpufreq_register(struct uart_8250_port *p)
+{
+	p->freq_transition.notifier_call = serial8250_cpufreq_transition;
+
+	return cpufreq_register_notifier(&p->freq_transition,
+					 CPUFREQ_TRANSITION_NOTIFIER);
+}
+
+static inline void serial8250_cpufreq_deregister(struct uart_8250_port *p)
+{
+	cpufreq_unregister_notifier(&p->freq_transition,
+				    CPUFREQ_TRANSITION_NOTIFIER);
+}
+#else
+static inline int serial8250_cpufreq_register(struct uart_8250_port *p)
+{
+	return 0;
+}
+
+static inline void serial8250_cpufreq_deregister(struct uart_8250_port *p)
+{
+}
+#endif
+
 /*
  * Register a set of serial devices attached to a platform device.  The
  * list is terminated with a zero flags entry, which means we expect
@@ -3051,6 +3117,9 @@ static int __devinit serial8250_probe(struct platform_device *dev)
 		port.pm			= p->pm;
 		port.dev		= &dev->dev;
 		port.irqflags		|= irqflag;
+		if (p->clk)
+			serial8250_ports[i].clk = p->clk;
+
 		ret = serial8250_register_port(&port);
 		if (ret < 0) {
 			dev_err(&dev->dev, "unable to register port at index %d "
@@ -3227,6 +3296,10 @@ int serial8250_register_port(struct uart_port *port)
 		ret = uart_add_one_port(&serial8250_reg, &uart->port);
 		if (ret == 0)
 			ret = uart->port.line;
+
+		ret = serial8250_cpufreq_register(uart);
+		if (ret < 0)
+			printk(KERN_ERR "Failed to add cpufreq notifier\n");
 	}
 	mutex_unlock(&serial_mutex);
 
@@ -3246,6 +3319,7 @@ void serial8250_unregister_port(int line)
 	struct uart_8250_port *uart = &serial8250_ports[line];
 
 	mutex_lock(&serial_mutex);
+	serial8250_cpufreq_deregister(uart);
 	uart_remove_one_port(&serial8250_reg, &uart->port);
 	if (serial8250_isa_devs) {
 		uart->port.flags &= ~UPF_BOOT_AUTOCONF;
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index ae027be..be05a03 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -37,6 +37,11 @@ struct uart_8250_port {
 	unsigned char		lsr_saved_flags;
 #define MSR_SAVE_FLAGS UART_MSR_ANY_DELTA
 	unsigned char		msr_saved_flags;
+	struct clk		*clk;
+#ifdef CONFIG_CPU_FREQ
+	struct notifier_block	freq_transition;
+#endif
+
 };
 
 struct old_serial_port {
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index 8f012f8..c2e1ce5 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -23,6 +23,7 @@ struct plat_serial8250_port {
 	resource_size_t	mapbase;	/* resource base */
 	unsigned int	irq;		/* interrupt number */
 	unsigned long	irqflags;	/* request_irq flags */
+	struct clk	*clk;
 	unsigned int	uartclk;	/* UART clock rate */
 	void            *private_data;
 	unsigned char	regshift;	/* register shift */
-- 
1.7.1

Re: [PATCH] serial: 8250: Add cpufreq support

[Alan Cox]

> This patch was submitted 2 years ago but didn't make it to the mainline. Now i am reposting it.

Can you fix it instead of just reposting it ?


> +#ifdef CONFIG_CPU_FREQ
> +static int serial8250_cpufreq_transition(struct notifier_block *nb,
> +					     unsigned long val, void *data)
> +{
> +	struct uart_8250_port *p;
> +	struct uart_port *uport;

What is your locking model ?


> +static inline void serial8250_cpufreq_deregister(struct uart_8250_port *p)

unregister

> +		ret = serial8250_cpufreq_register(uart);
> +		if (ret < 0)
> +			printk(KERN_ERR "Failed to add cpufreq notifier\n");

Why do this for devices that don't care.

Alan

RE: [PATCH] serial: 8250: Add cpufreq support

[Shankarmurthy, Akshay]

Hi,

On Tue, Apr 03, 2012 at 15:25:30, Alan Cox wrote:
> > This patch was submitted 2 years ago but didn't make it to the mainline. Now i am reposting it.
> 
> Can you fix it instead of just reposting it ?
> 
> 
> > +#ifdef CONFIG_CPU_FREQ
> > +static int serial8250_cpufreq_transition(struct notifier_block *nb,
> > +					     unsigned long val, void *data)
> > +{
> > +	struct uart_8250_port *p;
> > +	struct uart_port *uport;
> 
> What is your locking model ?
> 

I will have a look at this and add the lock if necessary.

> 
> > +static inline void serial8250_cpufreq_deregister(struct uart_8250_port *p)
> 
> unregister

Ok. I will change it.

> 
> > +		ret = serial8250_cpufreq_register(uart);
> > +		if (ret < 0)
> > +			printk(KERN_ERR "Failed to add cpufreq notifier\n");
> 
> Why do this for devices that don't care.

This is taken care in the code. If the device's frequency doesn't change after the
change in the cpu frequency then it doesn't go for execution of the function "serial8250_set_termios".
It does execute only if the uart clk frequency is changed with a change in the cpu frequency. 
So registering this for the devices that doesn't care will not affect the system. 


Regards,
Akshay
    
 

Re: [PATCH] serial: 8250: Add cpufreq support

[Alan Cox]

On Thu, 5 Apr 2012 10:40:55 +0000
"Shankarmurthy, Akshay" <akshay.s@ti.com> wrote:

> Hi,
> 
> On Tue, Apr 03, 2012 at 15:25:30, Alan Cox wrote:
> > > This patch was submitted 2 years ago but didn't make it to the mainline. Now i am reposting it.
> > 
> > Can you fix it instead of just reposting it ?
> > 
> > 
> > > +#ifdef CONFIG_CPU_FREQ
> > > +static int serial8250_cpufreq_transition(struct notifier_block *nb,
> > > +					     unsigned long val, void *data)
> > > +{
> > > +	struct uart_8250_port *p;
> > > +	struct uart_port *uport;
> > 
> > What is your locking model ?
> > 
> 
> I will have a look at this and add the lock if necessary.

At the very least you need reference counts held on the tty struct and to
allow for the tty having vanished under you.

> > 
> > > +static inline void serial8250_cpufreq_deregister(struct uart_8250_port *p)
> > 
> > unregister
> 
> Ok. I will change it.
> 
> > 
> > > +		ret = serial8250_cpufreq_register(uart);
> > > +		if (ret < 0)
> > > +			printk(KERN_ERR "Failed to add cpufreq notifier\n");
> > 
> > Why do this for devices that don't care.
> 
> This is taken care in the code. If the device's frequency doesn't change after the
> change in the cpu frequency then it doesn't go for execution of the function "serial8250_set_termios".

But you still call each function and walk the list for each port on each
CPU transition. There are people with 128 ports in their systems and the
cpu frequency change is latency critical.

Nothing should even be getting registered in such cases.

RE: [PATCH] serial: 8250: Add cpufreq support

[Shankarmurthy, Akshay]

On Thu, Apr 05, 2012 at 16:23:22, Alan Cox wrote:
> On Thu, 5 Apr 2012 10:40:55 +0000
> "Shankarmurthy, Akshay" <akshay.s@ti.com> wrote:
> 
> > Hi,
> > 
> > On Tue, Apr 03, 2012 at 15:25:30, Alan Cox wrote:
> > > > This patch was submitted 2 years ago but didn't make it to the mainline. Now i am reposting it.
> > > 
> > > Can you fix it instead of just reposting it ?
> > > 
> > > 
> > > > +#ifdef CONFIG_CPU_FREQ
> > > > +static int serial8250_cpufreq_transition(struct notifier_block *nb,
> > > > +					     unsigned long val, void *data) {
> > > > +	struct uart_8250_port *p;
> > > > +	struct uart_port *uport;
> > > 
> > > What is your locking model ?
> > > 
> > 
> > I will have a look at this and add the lock if necessary.
> 
> At the very least you need reference counts held on the tty struct and to allow for the tty having vanished under you.
> 

I am planning to add "tty->termios_mutex" lock. Any comments ?

> > > 
> > > > +static inline void serial8250_cpufreq_deregister(struct 
> > > > +uart_8250_port *p)
> > > 
> > > unregister
> > 
> > Ok. I will change it.
> > 
> > > 
> > > > +		ret = serial8250_cpufreq_register(uart);
> > > > +		if (ret < 0)
> > > > +			printk(KERN_ERR "Failed to add cpufreq notifier\n");
> > > 
> > > Why do this for devices that don't care.
> > 
> > This is taken care in the code. If the device's frequency doesn't 
> > change after the change in the cpu frequency then it doesn't go for execution of the function "serial8250_set_termios".
> 
> But you still call each function and walk the list for each port on each CPU transition. There are people with 128 ports in their systems and the cpu frequency change is latency critical.
> 
> Nothing should even be getting registered in such cases.
> 

I understand this and planning to avoid unnecessary registration like these. 
As a solution, UART port's cpu freq status will be sent from the board file.
Adding a member to the structure "plat_serial8250_port" which says whether
a port requires cpu freq registration or not. cpu freq register function is
called in the driver depending upon the status of this structure member.
I can update the patch accordingly and send out a version 2 to the list.

Regards,
Akshay

Re: [PATCH] serial: 8250: Add cpufreq support

[Alan Cox]

> > > I will have a look at this and add the lock if necessary.
> > 
> > At the very least you need reference counts held on the tty struct and to allow for the tty having vanished under you.
> > 
> 
> I am planning to add "tty->termios_mutex" lock. Any comments ?

Explain how this will help. You need a reference count to the tty itself.

> I understand this and planning to avoid unnecessary registration like these. 
> As a solution, UART port's cpu freq status will be sent from the board file.
> Adding a member to the structure "plat_serial8250_port" which says whether
> a port requires cpu freq registration or not. cpu freq register function is
> called in the driver depending upon the status of this structure member.
> I can update the patch accordingly and send out a version 2 to the list.

That sounds sensible.

Alan

RE: [PATCH] serial: 8250: Add cpufreq support

[Shankarmurthy, Akshay]

On Thu, Apr 12, 2012 at 15:17:58, Alan Cox wrote:
> > > > I will have a look at this and add the lock if necessary.
> > > 
> > > At the very least you need reference counts held on the tty struct and to allow for the tty having vanished under you.
> > > 
> > 
> > I am planning to add "tty->termios_mutex" lock. Any comments ?
> 
> Explain how this will help. You need a reference count to the tty itself.
> 


I have gone through the serial driver, still I am not able to figure out the requirement
of tty reference count here. Correct me if I am wrong. 
As per my understanding, tty count keeps the count of tty_open and tty_release, that is
how many open and close happened on the tty device file(/dev/ttyS*).
The requirement here is a tty count check, if it is greater than zero then only it should
proceed for further execution. 
Kindly elaborate more on the same if it's not correct.

> 
> Alan
> 

Re: [PATCH] serial: 8250: Add cpufreq support

[Alan Cox]

> As per my understanding, tty count keeps the count of tty_open and tty_release, that is
> how many open and close happened on the tty device file(/dev/ttyS*).
> The requirement here is a tty count check, if it is greater than zero then only it should
> proceed for further execution. 

The physical port has a different lifetime to the struct tty_struct
(which may change many times within the lifetime of the port). You can't
simply go from the port to a tty struct and assume this is safe and will
not change.

> Kindly elaborate more on the same if it's not correct.

Look at how tty_port_tty_get is used