Peter Hurley <peter@xxxxxxxxxxxxxxxxxx> writes:
> On 03/08/2015 05:04 PM, Måns Rullgård wrote:
>> Peter Hurley <peter@xxxxxxxxxxxxxxxxxx> writes:
>>
>>> Au1x00/RT2800+ doesn't implement the 8250 scratch register (and
>>> this may be true of other h/w currently supported by the 8250 driver);
>>> read back the canary value written to the scratch register to enable
>>> the console h/w restart after resume from system suspend.
>>>
>>> Fixes: 4516d50aabedb ("serial: 8250: Use canary to restart console ...")
>>> Reported-by: Mason <slash.tmp@xxxxxxx>
>>> Signed-off-by: Peter Hurley <peter@xxxxxxxxxxxxxxxxxx>
>>> ---
>>> drivers/tty/serial/8250/8250_core.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
>>> index 87df908..50c80b1 100644
>>> --- a/drivers/tty/serial/8250/8250_core.c
>>> +++ b/drivers/tty/serial/8250/8250_core.c
>>> @@ -3486,7 +3486,8 @@ void serial8250_suspend_port(int line)
>>> port->type != PORT_8250) {
>>> unsigned char canary = 0xa5;
>>> serial_out(up, UART_SCR, canary);
>>> - up->canary = canary;
>>> + if (serial_in(up, UART_SCR) == canary)
>>> + up->canary = canary;
>>> }
>>
>> I think this has the opposite of the intended effect. If there is no
>> scratch register, up->canary will remain zero, and the reset code in
>> serial8250_console_write() won't execute even if it is needed.
>
> That was intended.
>
> Case #1 - UART has UART_SCR
>
> The canary written is read back so up->canary is set to non-zero.
> At each subsequent write to console, the scratch register will be read
> until the canary has changed, at which time the UART will be
> reinitialized and the canary test disabled (on the presumption that
> what changed the canary was a power-off).
>
> Note that if the port does not power-down at system suspend (eg, if
> system suspend failed), the canary test is disabled anyway at port resume.
If UART_SCR exists, it's obvious that nothing is changed by this patch.
> Case #2 - UART does not have UART_SCR
>
> The canary written is not read back properly so up->canary remains zero.
> No attempt is made to test the canary or reinitialize the UART.
> This leaves the uart in the same state it would have been in;
> the canary test is useless here because there continues to be output
> after port suspend but before machine suspend.
It seems to me that the purpose of writing the canary to UART_SCR is to
detect if the port needs reinitialising. If UART_SCR doesn't exist, the
detection cannot work, and we can assume either 1) that the port needs
reinitialising, or 2) that it does not. The current code does #1. You
are proposing a switch to #2. I'd be more careful about making such
changes.
>> As it is currently (without this patch), a missing scratch register will
>> only result in the port possibly being re-initialised unnecessarily,
>> which should be quite harmless.
>
> I'd rather not trigger the reinitialize if it's not necessary; does it
> do anything on your hardware (that doesn't have UART_SCR) that's actually
> useful?
The only power management I know of on this hardware is the manual
on/off switch, so I doubt any of this is relevant. That doesn't mean
some other chip doesn't need it.
--
Måns Rullgård
mans@xxxxxxxxx
--
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
