TTY: serial 8250: Support MCR CLK_SEL bit.

Some UARTs have an internal clock predivisor which can be /1 or /4. The initial state is set by hardware at chip reset by an external CLK_SEL pin but can be modified by software using the CLK_SEL bit in MCR.

Currently, on hardware where the CLK_SEL pin is set to /4, the baud rate used by Linux is thus incorrect (4 times too slow).

Although it is possible to fix this just by changing the clock frequency passed to the kernel this has two disadvantages:
* The maximum attainable baud rate will be artificially low.
* The DT or board file data no longer directly matches the schematic.

Hence this patch which:
* Allows the CLK_SEL bit to be forced to /1, /4 or left as is.
* Correctly calculates the effective uartclk according to this divisor

Tested using an Exar xr16c2850 UART.

---
 .../devicetree/bindings/tty/serial/of-serial.txt   |    2 +
 drivers/tty/serial/8250/8250.c                     |   60 ++++++++++++++++----
 drivers/tty/serial/8250/8250.h                     |    1 
 drivers/tty/serial/of_serial.c                     |    2 +
 include/linux/serial_8250.h                        |    2 +
 5 files changed, 56 insertions(+), 11 deletions(-)

index ba385f2..b1c5f85 100644
@@ -27,6 +27,8 @@ Optional properties:
   RTAS and should not be registered.
 - no-loopback-test: set to indicate that the port does not implements loopback
   test mode
+ -clock-predivisor : predivisor for UARTs that support it (0,1 or 4)
+   0 => leave as determined by hardware pin.
 
 Example:
 

index 5a76f9c..01899db 100644
@@ -40,6 +40,7 @@ struct serial8250_config {
 #define UART_CAP_AFE	(1 << 11)	/* MCR-based hw flow control */
 #define UART_CAP_UUE	(1 << 12)	/* UART needs IER bit 6 set (Xscale) */
 #define UART_CAP_RTOIE	(1 << 13)	/* UART needs IER bit 4 set (Xscale, Tegra) */
+#define UART_CAP_CLKSEL (1 << 14)	/* UART has clock select predivisor bit */
 
 #define UART_BUG_QUOT	(1 << 0)	/* UART has buggy quot LSB */
 #define UART_BUG_TXEN	(1 << 1)	/* UART has buggy TX IIR status */

index df443b9..047f59c 100644
@@ -154,6 +154,8 @@ static int __devinit of_platform_serial_probe(struct platform_device *ofdev)
 		struct uart_8250_port port8250;
 		memset(&port8250, 0, sizeof(port8250));
 		port8250.port = port;
+		of_property_read_u32(ofdev->dev.of_node,
+			"clock-predivisor", &port8250.predivisor);
 		ret = serial8250_register_8250_port(&port8250);
 		break;
 	}

index c174c90..39bd8b9 100644
@@ -73,6 +73,8 @@ struct uart_8250_port {
 	unsigned short		capabilities;	/* port capabilities */
 	unsigned short		bugs;		/* port bugs */
 	unsigned int		tx_loadsz;	/* transmit fifo load size */
+	unsigned int		predivisor;	/* 0=HW select */
+	unsigned long		hwclk;		/* Clock before predivisor */
 	unsigned char		acr;
 	unsigned char		ier;
 	unsigned char		lcr;

This seems to be the wrong place to set such stuff. The kernel knows what speed is selected and therefore what clock would be best.

Having a clocksel (and probably an of value indicating which type of clock sel) is a good thing, but it also ought to be set based on the requested baud rate, so a custom set_termios would be better.

That would also handle serial console, and suspend/resume without other tweaks as far as I can see.

It also seems some devices use different clksel algorithms so rather than a CAP_CLKSEL flag we probably need to key it off the uart type.

	->select_clock(port, bitrate);

Thank you for the review.

I agree that would be nicer, however I think there is a problem, at least in the case of the xr16c2850.

This chip is a DUART but the clksel bit, although present for both channels, (in MCR bit 7) is actually shared (as is the external CLKSEL pin). Hence if the kernel determines whether to enable /4 or /1 based on the requested baud rate for one channel it may break the baud rate for the other channel.

This problem also exists with my approach but only to the extent that if the device tree writer tries to set one channel to /4 and the other to /1 the result will not be as intended.

As a general question how should this type of dependency between ports due to them being part of the same chip be handled?

Yes, there may be problems with suspend/resume - I haven't tried yet.

True,

I've also looked at the NXP SC16C850 and SC16C852 data sheets and they, in addition to the /4 controlled by MCR:7, also have an additional 4 bit prescaler register.

They lack the external clksel pin however (which is the main reason for all this - the normal 16 bit divisor is enough to allow _low_ baud rates to be reached without enabling any other prescalers - the only case that is really a problem is when chips that _do_ have the predivisor enabled in hardware cause us not to be able to attain high baud rates).

I'm not sure if the clock selects are independant for the DUART case, they appear to be but I don't have the hardware to test it.

Sorry, I don't understand here.

The same EFR bit needs to be set to enable access to both the SLEEP and CLKSEL bits.
So I've just refactored the existing code to share setting this bit - this shouldn't change the existing behaviour.

Maybe I should have done this as a seperate patch to make it clearer.

If UART_CAP_EFR is not set but UART_CAP_SLEEP or UART_CAP_CLKSEL is set is assumed that it is not necessary to enable access via the EFR (whether that is a valid combination for any existing UART is another question).

Regards,

Martin

Gak.

It sounds to me an even bigger reason for having the kernel 8250 termios code have a ->clock_select() method but you'd need some way to find the pair of the device. Ok thats uglier. Can you at least define the DT as setting the *initial* divisor. That way we can later fix the kernel to be smart about clocks without breaking any assumption about what the DT entry means.

Got it - and yes please make that a separate patch.

Probably not.

Alan