qe: add function qe_clock_source

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.

---

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(-)

index 3d57d38..da68534 100644
@@ -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
  */

index 0dabe46..d28bc85 100644
@@ -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

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

- k

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

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

- k

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

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

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

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

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

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

-Scott

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

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

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

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

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

- k

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

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

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

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