[Review request] sh-sci clock

[Review request] sh-sci clock

[Kuninori Morimoto]

Hi all

Now I'm creating SH7724 sub cpu support patch.
And I would like to use "bus_clk" for SCIFA on sh-sci.
Current sh-sci use module_clk only.
So, I created attached patch to be able to select "bus_clk" or "module_clk".

It works well on SH7724/SH7723/SH7722
But I can not check about other CPU.

Please review it.

** from here **

sh-sci: sh-sci can select bus_clk or module_clk

SCIFA had used module_clk up to now though it used bus_clk.
This patch modify this problem, and checked by SH7723/SH7722.

Signed-off-by: Kuninori Morimoto <morimoto.kuninori@renesas.com>
---
 drivers/serial/sh-sci.c |   18 ++++++++++++++----
 drivers/serial/sh-sci.h |    4 ++--
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/serial/sh-sci.c b/drivers/serial/sh-sci.c
index dbf5357..dd60a1c 100644
--- a/drivers/serial/sh-sci.c
+++ b/drivers/serial/sh-sci.c
@@ -92,6 +92,11 @@ static void sci_stop_tx(struct uart_port *port);
 static struct sci_port sci_ports[SCI_NPORTS];
 static struct uart_driver sci_uart_driver;
 
+static inline const char *get_clk_source(struct sci_port *s)
+{
+	return (s->type = PORT_SCIFA) ? "bus_clk" : "module_clk";
+}
+
 static inline struct sci_port *
 to_sci_port(struct uart_port *uart)
 {
@@ -871,7 +876,7 @@ static int sci_startup(struct uart_port *port)
 		s->enable(port);
 
 #ifdef CONFIG_HAVE_CLK
-	s->clk = clk_get(NULL, "module_clk");
+	s->clk = clk_get(NULL, get_clk_source(s));
 #endif
 
 	sci_request_irq(s);
@@ -1059,7 +1064,8 @@ static void __init sci_init_ports(void)
 		 * XXX: We should use a proper SCI/SCIF clock
 		 */
 		{
-			struct clk *clk = clk_get(NULL, "module_clk");
+			struct clk *clk = clk_get(NULL,
+						  get_clk_source(sci_ports+i));
 			sci_ports[i].port.uartclk = clk_get_rate(clk);
 			clk_put(clk);
 		}
@@ -1147,8 +1153,12 @@ static int __init serial_console_setup(struct console *co, char *options)
 	port->type = serial_console_port->type;
 
 #ifdef CONFIG_HAVE_CLK
-	if (!serial_console_port->clk)
-		serial_console_port->clk = clk_get(NULL, "module_clk");
+	if (!serial_console_port->clk) {
+		serial_console_port->clk +			clk_get(NULL, get_clk_source(serial_console_port));
+		serial_console_port->port.uartclk +			clk_get_rate(serial_console_port->clk);
+	}
 #endif
 
 	if (port->flags & UPF_IOREMAP)
diff --git a/drivers/serial/sh-sci.h b/drivers/serial/sh-sci.h
index d0aa82d..5215b9d 100644
--- a/drivers/serial/sh-sci.h
+++ b/drivers/serial/sh-sci.h
@@ -761,9 +761,9 @@ static inline int sci_rxd_in(struct uart_port *port)
 static inline int scbrr_calc(struct uart_port *port, int bps, int clk)
 {
 	if (port->type = PORT_SCIF)
-		return (clk+16*bps)/(32*bps)-1;
+		return (clk/(32*bps))-1;
 	else
-		return ((clk*2)+16*bps)/(16*bps)-1;
+		return (clk/(16*bps))-1;
 }
 #define SCBRR_VALUE(bps, clk) scbrr_calc(port, bps, clk)
 #elif defined(__H8300H__) || defined(__H8300S__)
-- 
1.5.6.3

Re: [Review request] sh-sci clock

[Paul Mundt]

On Mon, Apr 13, 2009 at 01:17:59PM +0900, Kuninori Morimoto wrote:
> Now I'm creating SH7724 sub cpu support patch.
> And I would like to use "bus_clk" for SCIFA on sh-sci.
> Current sh-sci use module_clk only.
> So, I created attached patch to be able to select "bus_clk" or "module_clk".
> 
> It works well on SH7724/SH7723/SH7722
> But I can not check about other CPU.
> 
> Please review it.
> 
I was afraid this was going to happen sooner or later. This is ok for
2.6.30, but we will have to revisit Magnus' patchset for 2.6.31 to do
this properly. Though I really want to avoid per-port platform devices.

Re: [Review request] sh-sci clock

[Paul Mundt]

On Mon, Apr 13, 2009 at 01:17:59PM +0900, Kuninori Morimoto wrote:
> diff --git a/drivers/serial/sh-sci.h b/drivers/serial/sh-sci.h
> index d0aa82d..5215b9d 100644
> --- a/drivers/serial/sh-sci.h
> +++ b/drivers/serial/sh-sci.h
> @@ -761,9 +761,9 @@ static inline int sci_rxd_in(struct uart_port *port)
>  static inline int scbrr_calc(struct uart_port *port, int bps, int clk)
>  {
>  	if (port->type = PORT_SCIF)
> -		return (clk+16*bps)/(32*bps)-1;
> +		return (clk/(32*bps))-1;
>  	else
> -		return ((clk*2)+16*bps)/(16*bps)-1;
> +		return (clk/(16*bps))-1;
>  }
>  #define SCBRR_VALUE(bps, clk) scbrr_calc(port, bps, clk)
>  #elif defined(__H8300H__) || defined(__H8300S__)

This looks like an unrelated change, can you explain what this is about?

Re: [Review request] sh-sci clock

[morimoto.kuninori]

Dear Paul

> > It works well on SH7724/SH7723/SH7722
> > But I can not check about other CPU.
> > 
> > Please review it.
> > 
> I was afraid this was going to happen sooner or later. This is ok for
> 2.6.30, but we will have to revisit Magnus' patchset for 2.6.31 to do
> this properly. Though I really want to avoid per-port platform devices.

wow I didn't know that.
Thank you.

OK, I will send SH7724 patches without sh-sci. 

Best regards
--
Kuninori Morimoto
 

Re: [Review request] sh-sci clock

[morimoto.kuninori]

Dear Paul

> >  static inline int scbrr_calc(struct uart_port *port, int bps, int clk)
> >  {
> >  	if (port->type = PORT_SCIF)
> > -		return (clk+16*bps)/(32*bps)-1;
> > +		return (clk/(32*bps))-1;
> >  	else
> > -		return ((clk*2)+16*bps)/(16*bps)-1;
> > +		return (clk/(16*bps))-1;
> >  }
> >  #define SCBRR_VALUE(bps, clk) scbrr_calc(port, bps, clk)
> >  #elif defined(__H8300H__) || defined(__H8300S__)
> 
> This looks like an unrelated change, can you explain what this is about?

When SCIFA of SH7723/SH7724 use bus_clk,
SCBRR value calculation will be changed.

Current SCIFA use module_clk,
and above calculation for SH7723 is (mey be) depend on ap325 board.
module_clk of ap325 is 33MHz and bus_clk is 66MHz,
mey be (clk*2) mean it.
But ms7724se module_clk is 33MHz and bus_clk is 83MHz.
So, ms7724se board's SCIFA can not work on it.

And current SCIF(SCIFA also) calculation is wrong.
I don't know why this calculation is used.

return (clk+16*bps)/(32*bps)-1;
=>
return clk/(32*bps) - 1/2;

This calculation might be OK by lucky. 


Best regards
--
Kuninori Morimoto
 

Re: [Review request] sh-sci clock

[Paul Mundt]

On Tue, Apr 14, 2009 at 11:08:50AM +0900, morimoto.kuninori@renesas.com wrote:
> > >  static inline int scbrr_calc(struct uart_port *port, int bps, int clk)
> > >  {
> > >  	if (port->type = PORT_SCIF)
> > > -		return (clk+16*bps)/(32*bps)-1;
> > > +		return (clk/(32*bps))-1;
> > >  	else
> > > -		return ((clk*2)+16*bps)/(16*bps)-1;
> > > +		return (clk/(16*bps))-1;
> > >  }
> > >  #define SCBRR_VALUE(bps, clk) scbrr_calc(port, bps, clk)
> > >  #elif defined(__H8300H__) || defined(__H8300S__)
> > 
> > This looks like an unrelated change, can you explain what this is about?
> 
> When SCIFA of SH7723/SH7724 use bus_clk,
> SCBRR value calculation will be changed.
> 
> Current SCIFA use module_clk,
> and above calculation for SH7723 is (mey be) depend on ap325 board.
> module_clk of ap325 is 33MHz and bus_clk is 66MHz,
> mey be (clk*2) mean it.
> But ms7724se module_clk is 33MHz and bus_clk is 83MHz.
> So, ms7724se board's SCIFA can not work on it.
> 
Ok.

> And current SCIF(SCIFA also) calculation is wrong.
> I don't know why this calculation is used.
> 
Good question.. lets see what git blame has to say about it:

pmundt@dysnomia ~/devel/git/linux-2.6 $ git show ba1d2818
commit ba1d28181c586deec468cc6ae558c0c099f1b956
Author: Nobuhiro Iwamatsu <iwamatsu.nobuhiro@renesas.com>
Date:   Fri Oct 3 17:37:31 2008 +0900

    serial: sh-sci: Add support SCIF of SH7723

    SH7723 has two types of SCIF (SCIF and SCIFA).
    The current sh-sci driver supports only SCIFA, and calculation methods of SCBRR
    are different.
    This patch support this methods.

    Signed-off-by: Nobuhiro Iwamatsu <iwamatsu.nobuhiro@renesas.com>
    Signed-off-by: Paul Mundt <lethal@linux-sh.org>

diff --git a/drivers/serial/sh-sci.h b/drivers/serial/sh-sci.h
index 511c10d..7cd28b2 100644
--- a/drivers/serial/sh-sci.h
+++ b/drivers/serial/sh-sci.h
@@ -789,7 +789,14 @@ static inline int sci_rxd_in(struct uart_port *port)
       defined(CONFIG_CPU_SUBTYPE_SH7721)
 #define SCBRR_VALUE(bps, clk) (((clk*2)+16*bps)/(32*bps)-1)
 #elif defined(CONFIG_CPU_SUBTYPE_SH7723)
-#define SCBRR_VALUE(bps, clk) (((clk*2)+16*bps)/(16*bps)-1)
+static inline int scbrr_calc(struct uart_port *port, int bps, int clk)
+{
+       if (port->type = PORT_SCIF)
+               return (clk+16*bps)/(32*bps)-1;
+       else
+               return ((clk*2)+16*bps)/(16*bps)-1;
+}
+#define SCBRR_VALUE(bps, clk) scbrr_calc(port, bps, clk)
 #elif defined(__H8300H__) || defined(__H8300S__)
 #define SCBRR_VALUE(bps, clk) (((clk*1000/32)/bps)-1)
 #else /* Generic SH */

> return (clk+16*bps)/(32*bps)-1;
> =>
> return clk/(32*bps) - 1/2;
> 
> This calculation might be OK by lucky. 
> 
I have not looked at the SCIFA SCBRR algorithm, though it seems Iwamatsu-san
has.

Please split this part of the patch out separately, and get some feedback
from Iwamatsu-san on it to make sure it doesn't break any boards. Maybe
there is some strange Hitachi ULSI board with magical clock settings.. or
someone copied the wrong line from the manual (possibly because the data
sheet was suffering from accuracy problems).. :-)

Re: [Review request] sh-sci clock

[morimoto.kuninori]

Dear Paul

I'm sorry I took miss on explain

> > Current SCIFA use module_clk,
> > and above calculation for SH7723 is (mey be) depend on ap325 board.
> > module_clk of ap325 is 33MHz and bus_clk is 66MHz,
> > mey be (clk*2) mean it.
> > But ms7724se module_clk is 33MHz and bus_clk is 83MHz.
> > So, ms7724se board's SCIFA can not work on it.

This explain was wrong.
bus_clk = module_clk * 2 is correct on SH7723/SH7724

I'm sorry again.

But SCBRR calculation is wrong. I will send patch.

Best regards
--
Kuninori Morimoto