[PATCH] qe: add function qe_clock_source

[PATCH] qe: add function qe_clock_source

[Timur Tabi]

Add function qe_clock_source() which takes a string containing the name of a
QE clock source (as is typically found in device trees) and returns the
matching enum qe_clock value.

Signed-off-by: Timur Tabi <timur@freescale.com>
---

This patch applies to Kumar's for-2.6.24 branch.

 arch/powerpc/sysdev/qe_lib/qe.c |   65 +++++++++++++++++++++++++++++++++++++++
 include/asm-powerpc/qe.h        |    3 ++
 2 files changed, 68 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/sysdev/qe_lib/qe.c b/arch/powerpc/sysdev/qe_lib/qe.c
index 3d57d38..da68534 100644
--- a/arch/powerpc/sysdev/qe_lib/qe.c
+++ b/arch/powerpc/sysdev/qe_lib/qe.c
@@ -199,6 +199,71 @@ void qe_setbrg(unsigned int brg, unsigned int rate, unsigned int multiplier)
 	out_be32(&qe_immr->brg.brgc[brg - 1], tempval);
 }
 
+/* Convert a string to a QE clock source enum
+ *
+ * This function takes a string, typically from a property in the device
+ * tree, and returns the corresponding "enum qe_clock" value.
+*/
+enum qe_clock qe_clock_source(const char *source)
+{
+	/* This structure is designed so that it's 8 bytes in size */
+	static struct {
+		const char name[6];
+		u16 num;
+	} __attribute__((packed)) sources[QE_CLK_DUMMY] =
+	{
+		{"none", QE_CLK_NONE},
+		{"brg1", QE_BRG1},
+		{"brg2", QE_BRG2},
+		{"brg3", QE_BRG3},
+		{"brg4", QE_BRG4},
+		{"brg5", QE_BRG5},
+		{"brg6", QE_BRG6},
+		{"brg7", QE_BRG7},
+		{"brg8", QE_BRG8},
+		{"brg9", QE_BRG9},
+		{"brg10", QE_BRG10},
+		{"brg11", QE_BRG11},
+		{"brg12", QE_BRG12},
+		{"brg13", QE_BRG13},
+		{"brg14", QE_BRG14},
+		{"brg15", QE_BRG15},
+		{"brg16", QE_BRG16},
+		{"clk1", QE_CLK1},
+		{"clk2", QE_CLK2},
+		{"clk3", QE_CLK3},
+		{"clk4", QE_CLK4},
+		{"clk5", QE_CLK5},
+		{"clk6", QE_CLK6},
+		{"clk7", QE_CLK7},
+		{"clk8", QE_CLK8},
+		{"clk9", QE_CLK9},
+		{"clk10", QE_CLK10},
+		{"clk11", QE_CLK11},
+		{"clk12", QE_CLK12},
+		{"clk13", QE_CLK13},
+		{"clk14", QE_CLK14},
+		{"clk15", QE_CLK15},
+		{"clk16", QE_CLK16},
+		{"clk17", QE_CLK17},
+		{"clk18", QE_CLK18},
+		{"clk19", QE_CLK19},
+		{"clk20", QE_CLK20},
+		{"clk21", QE_CLK21},
+		{"clk22", QE_CLK22},
+		{"clk23", QE_CLK23},
+		{"clk24", QE_CLK24}
+	};
+	unsigned int i;
+
+	for (i = 0; i < QE_CLK_DUMMY; i++)
+		if (strcasecmp(source, sources[i].name) == 0)
+			return (enum qe_clock) sources[i].num;
+
+	return QE_CLK_DUMMY;
+}
+EXPORT_SYMBOL(qe_clock_source);
+
 /* Initialize SNUMs (thread serial numbers) according to
  * QE Module Control chapter, SNUM table
  */
diff --git a/include/asm-powerpc/qe.h b/include/asm-powerpc/qe.h
index 0dabe46..d28bc85 100644
--- a/include/asm-powerpc/qe.h
+++ b/include/asm-powerpc/qe.h
@@ -175,6 +175,9 @@ enum qe_clock {
 	QE_CLK_DUMMY,
 };
 
+/* Convert a string to a QE clock source enum */
+enum qe_clock qe_clock_source(const char *source);
+
 /* QE CMXUCR Registers.
  * There are two UCCs represented in each of the four CMXUCR registers.
  * These values are for the UCC in the LSBs
-- 
1.5.2.4

Re: [PATCH] qe: add function qe_clock_source

[Kumar Gala]

On Oct 9, 2007, at 10:53 AM, Timur Tabi wrote:

> Add function qe_clock_source() which takes a string containing the  
> name of a
> QE clock source (as is typically found in device trees) and returns  
> the
> matching enum qe_clock value.
>
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
>
> This patch applies to Kumar's for-2.6.24 branch.
>
>  arch/powerpc/sysdev/qe_lib/qe.c |   65 ++++++++++++++++++++++++++++ 
> +++++++++++
>  include/asm-powerpc/qe.h        |    3 ++
>  2 files changed, 68 insertions(+), 0 deletions(-)

I'm sure its asking to much of the QE HW for BRGs or CLKs to be  
sequential.

- k

Re: [PATCH] qe: add function qe_clock_source

[Timur Tabi]

Kumar Gala wrote:
> 
> On Oct 9, 2007, at 10:53 AM, Timur Tabi wrote:
> 
>> Add function qe_clock_source() which takes a string containing the 
>> name of a
>> QE clock source (as is typically found in device trees) and returns the
>> matching enum qe_clock value.
>>
>> Signed-off-by: Timur Tabi <timur@freescale.com>
>> ---
>>
>> This patch applies to Kumar's for-2.6.24 branch.
>>
>>  arch/powerpc/sysdev/qe_lib/qe.c |   65 
>> +++++++++++++++++++++++++++++++++++++++
>>  include/asm-powerpc/qe.h        |    3 ++
>>  2 files changed, 68 insertions(+), 0 deletions(-)
> 
> I'm sure its asking to much of the QE HW for BRGs or CLKs to be sequential.

Well, this patch doesn't really address that anomaly.  Its purpose is to help 
get rid of stuff like this in the device tree:

		ucc@2200 {
			...
			rx-clock = <19>;
			tx-clock = <1a>;

19 and 1a are the integer equivalents of enum qe_clock.  As you can imagine, 
that's error prone.

If this patch is accepted, I got another patch that changes qe_setbrg() to 
accept a qe_clock enum.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH] qe: add function qe_clock_source

[Kumar Gala]

On Oct 9, 2007, at 11:01 AM, Timur Tabi wrote:

> Kumar Gala wrote:
>> On Oct 9, 2007, at 10:53 AM, Timur Tabi wrote:
>>> Add function qe_clock_source() which takes a string containing  
>>> the name of a
>>> QE clock source (as is typically found in device trees) and  
>>> returns the
>>> matching enum qe_clock value.
>>>
>>> Signed-off-by: Timur Tabi <timur@freescale.com>
>>> ---
>>>
>>> This patch applies to Kumar's for-2.6.24 branch.
>>>
>>>  arch/powerpc/sysdev/qe_lib/qe.c |   65 ++++++++++++++++++++++++++ 
>>> +++++++++++++
>>>  include/asm-powerpc/qe.h        |    3 ++
>>>  2 files changed, 68 insertions(+), 0 deletions(-)
>> I'm sure its asking to much of the QE HW for BRGs or CLKs to be  
>> sequential.
>
> Well, this patch doesn't really address that anomaly.  Its purpose  
> is to help get rid of stuff like this in the device tree:
>
> 		ucc@2200 {
> 			...
> 			rx-clock = <19>;
> 			tx-clock = <1a>;
>
> 19 and 1a are the integer equivalents of enum qe_clock.  As you can  
> imagine, that's error prone.
>
> If this patch is accepted, I got another patch that changes  
> qe_setbrg() to accept a qe_clock enum.

is 19 the actual value you'd end up using from the HW? or is it  
related to some random enum value?

- k

Re: [PATCH] qe: add function qe_clock_source

[Timur Tabi]

Kumar Gala wrote:

> is 19 the actual value you'd end up using from the HW? or is it related 
> to some random enum value?

Random enum value.  Here's the code in ucc_geth:

	prop = of_get_property(np, "rx-clock", NULL);
	ug_info->uf_info.rx_clock = *prop;


Here's the data type:

struct ucc_fast_info {
	int ucc_num;
	enum qe_clock rx_clock;
	enum qe_clock tx_clock;
	...

As you can see, it doesn't even validate the property.

My uart driver has this:

	rx-clock-source = "BRG5";
	tx-clock-source = "BRG6";


	sprop = of_get_property(np, "rx-clock-source", NULL);
	if (!sprop) {
		printk(KERN_ERR
		       "ucc-uart: missing rx-clock-source in device tree\n");
		kfree(qe_port);
		return -ENODEV;
	}

	qe_port->us_info.rx_clock = of_clock_source(sprop);
	if ((qe_port->us_info.rx_clock < QE_BRG1) ||
	    (qe_port->us_info.rx_clock > QE_BRG16)) {
		printk(KERN_ERR
		       "ucc-uart: rx-clock-source must be a BRG for UART\n");
		kfree(qe_port);
		return -ENODEV;
	}

I will be submitting patches to ucc_geth to fix this problem.


-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH] qe: add function qe_clock_source

[Kumar Gala]

Ok, I went and looked at how rx-clock & tx-clock are spec'd in  
booting-without-of.txt

Why dont we fix this so we have a property that says "BRG" or "CLK"  
and another that has ID = [1.16] for BRG and [1..24] for CLK.

- k

Re: [PATCH] qe: add function qe_clock_source

[Timur Tabi]

Kumar Gala wrote:
> Ok, I went and looked at how rx-clock & tx-clock are spec'd in 
> booting-without-of.txt
> 
> Why dont we fix this so we have a property that says "BRG" or "CLK" and 
> another that has ID = [1.16] for BRG and [1..24] for CLK.

Because sometimes you can use both BRGs and CLKs.  Some operations can be done 
only on one or the other, and some can be done on both.  The enum itself is 
fine, I think.  I'd rather not split up the two, just report an error if you 
specify a clock source that's not support for that particular purpose.

UART, for instance, can use a CLK signal, but I wrote the driver to only 
support BRGs because there are plenty of them and it's a lot simpler.  But 
someone, for instance, could wire up his board to want to use external clocks 
for some UARTs, and so he'd need to modify the driver.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH] qe: add function qe_clock_source

[Scott Wood]

Kumar Gala wrote:
> Ok, I went and looked at how rx-clock & tx-clock are spec'd in  
> booting-without-of.txt
> 
> Why dont we fix this so we have a property that says "BRG" or "CLK"  
> and another that has ID = [1.16] for BRG and [1..24] for CLK.

Or we could keep it as it is, check the beginning of the string for 
"BRG" or "CLK", and binarize the number that follows.

Or we could just stop putting CLK information in the device tree. :-)

-Scott

Re: [PATCH] qe: add function qe_clock_source

[Timur Tabi]

Scott Wood wrote:

> Or we could just stop putting CLK information in the device tree. :-)

If we had a BRG/CLK manager, we probably could do that.  But we don't have 
something like that now.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH] qe: add function qe_clock_source

[Scott Wood]

Timur Tabi wrote:
> UART, for instance, can use a CLK signal, but I wrote the driver to only 
> support BRGs because there are plenty of them and it's a lot simpler.  But 
> someone, for instance, could wire up his board to want to use external clocks 
> for some UARTs, and so he'd need to modify the driver.

In the absence of a BRG, the driver should just not support changing the 
baud rate -- it shouldn't fail to function.

-Scott

Re: [PATCH] qe: add function qe_clock_source

[Scott Wood]

Timur Tabi wrote:
> Scott Wood wrote:
> 
>> Or we could just stop putting CLK information in the device tree. :-)
> 
> If we had a BRG/CLK manager, we probably could do that.  But we don't 
> have something like that now.

I didn't say stop putting BRG information in there -- just CLKs, which 
are static setup that can be done in firmware or the board file.  This 
works fine on CPM1/2.

-Scott

Re: [PATCH] qe: add function qe_clock_source

[Timur Tabi]

Scott Wood wrote:

> In the absence of a BRG, the driver should just not support changing the 
> baud rate -- it shouldn't fail to function.

Since I don't have hardware that can test external clocks, I can't guarantee 
that any such code will work.

I'm sure there are a dozen things I could do to improve the driver as it 
stands, but I need to draw the line somewhere.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH] qe: add function qe_clock_source

[Kumar Gala]

On Oct 9, 2007, at 11:48 AM, Timur Tabi wrote:

> Scott Wood wrote:
>
>> In the absence of a BRG, the driver should just not support  
>> changing the baud rate -- it shouldn't fail to function.
>
> Since I don't have hardware that can test external clocks, I can't  
> guarantee that any such code will work.
>
> I'm sure there are a dozen things I could do to improve the driver  
> as it stands, but I need to draw the line somewhere.

I guess my point is it appears the patch is related to how we  
represent the RX/TX clock information in the device tree.

I'm suggesting that we do something like:

rx-clock-type = "brg" or "clk"
rx-clock-id = 3

I don't see how this doesn't cover what you need in the device tree.

- k

Re: [PATCH] qe: add function qe_clock_source

[Timur Tabi]

Kumar Gala wrote:

> rx-clock-type = "brg" or "clk"
> rx-clock-id = 3
> 
> I don't see how this doesn't cover what you need in the device tree.

That just looks more complicated than what my patch proposes.  Why have two 
properties when one will suffice?  I still need to have a look-up table that 
will convert "3" to QE_BRG3, since the latter is an enum.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH] qe: add function qe_clock_source

[Kumar Gala]

On Oct 9, 2007, at 1:12 PM, Timur Tabi wrote:

> Kumar Gala wrote:
>
>> rx-clock-type = "brg" or "clk"
>> rx-clock-id = 3
>> I don't see how this doesn't cover what you need in the device tree.
>
> That just looks more complicated than what my patch proposes.  Why  
> have two properties when one will suffice?  I still need to have a  
> look-up table that will convert "3" to QE_BRG3, since the latter is  
> an enum.

But that's a function of linux' use.  Remember decouple the device  
tree from how linux does things.

- k

Re: [PATCH] qe: add function qe_clock_source

[Timur Tabi]

Kumar Gala wrote:

> But that's a function of linux' use.  Remember decouple the device tree 
> from how linux does things.

Ok, I just understand what your point is.  I'm proposing that we just put the 
name of the clock source in the device tree, and have a function which 
converts that into an internal representation.  The clock is called "BRG3". 
It's not called BRG, #3 or something like that.  Same thing with the CLK sources.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH] qe: add function qe_clock_source

[Kumar Gala]

On Oct 9, 2007, at 1:17 PM, Timur Tabi wrote:

> Kumar Gala wrote:
>
>> But that's a function of linux' use.  Remember decouple the device  
>> tree from how linux does things.
>
> Ok, I just understand what your point is.  I'm proposing that we  
> just put the name of the clock source in the device tree, and have  
> a function which converts that into an internal representation.   
> The clock is called "BRG3". It's not called BRG, #3 or something  
> like that.  Same thing with the CLK sources.

Ok.  I guess I'm not in favor of changing the device tree to address  
this issue.  I think it would be solved if "dtc" had #define support.

- k

Re: [PATCH] qe: add function qe_clock_source

[Timur Tabi]

Kumar Gala wrote:

> Ok.  I guess I'm not in favor of changing the device tree to address 
> this issue.  I think it would be solved if "dtc" had #define support.

Not really.  The #defines would then need to match the enum, and that's dual 
maintenance.  This method is better because you can use the real name of the 
clock source in the DTS.  That keeps the DTS from having to know the internal 
representation of clock sources.

(BTW, when I said "I just understand", I meant "I just don't understand".)

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH] qe: add function qe_clock_source

[Kumar Gala]

On Oct 9, 2007, at 2:18 PM, Timur Tabi wrote:

> Kumar Gala wrote:
>
>> Ok.  I guess I'm not in favor of changing the device tree to  
>> address this issue.  I think it would be solved if "dtc" had  
>> #define support.
>
> Not really.  The #defines would then need to match the enum, and  
> that's dual maintenance.  This method is better because you can use  
> the real name of the clock source in the DTS.  That keeps the DTS  
> from having to know the internal representation of clock sources.
>
> (BTW, when I said "I just understand", I meant "I just don't  
> understand".)

I misread the idea.  I'm ok with using a string, just use a new prop  
name and deprecate the old names in booting-without-of.txt

Also, I think Scott suggested this but I'd say parse the string for  
"BRG" & "CLK" and the decimal number and use some enum + math rather  
than a big lookup table.

- k