Thank you for the review.
On 19/11/12 13:00, Alan Cox wrote:
* Allows the CLK_SEL bit to be forced to /1, /4 or left as is.
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.
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?
That would also handle serial console, and suspend/resume without other
tweaks as far as I can see.
Yes, there may be problems with suspend/resume - I haven't tried yet.
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.
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.
static void serial8250_set_sleep(struct uart_8250_port *p, int sleep)
{
if (p->capabilities & UART_CAP_SLEEP) {
- if (p->capabilities & UART_CAP_EFR) {
- serial_out(p, UART_LCR, UART_LCR_CONF_MODE_B);
- serial_out(p, UART_EFR, UART_EFR_ECB);
- serial_out(p, UART_LCR, 0);
- }
+ serial8250_enable_extended(p);
serial_out(p, UART_IER, sleep ? UART_IERX_SLEEP : 0);
This seems wrong if theere is no EFR
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
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
