On 03/08/2015 06:30 PM, Måns Rullgård wrote:
> 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.
I wrote that for the benefit of those who may be following along, for
whom that might not be obvious.
>> 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.
When I wrote the canary code, I believed that all h/w this driver supports
(besides the original 8250 which is already special-cased) had a scratch
register (I was not aware that the Au1x00/RT2880+ did not).
So this patch restores my original intention to not reinit the port unless
required.
>>> 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.
Ok.
Regards,
Peter Hurley
--
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
