On 07/28/2015 10:07 AM, David Jander wrote:
> On Tue, 28 Jul 2015 08:23:13 -0400
> Peter Hurley <peter@xxxxxxxxxxxxxxxxxx> wrote:
>
>> Hi David,
>>
>> On 07/28/2015 02:07 AM, David Jander wrote:
>>> On Mon, 27 Jul 2015 15:15:59 -0300
>>> Fabio Estevam <fabio.estevam@xxxxxxxxxxxxx> wrote:
>>>
>>>> This reverts commit e95044ba4fee93f5ea8a1a24b2d921e148503833.
>>>>
>>>> Commit e95044ba4fee93 ("tty: serial: imx.c: Reset UART before activating
>>>> interrupts") terribly messes up with the console on mx6 boards, so
>>>> let's revert it.
>>>
>>> IMHO Spewing out a few corrupt characters during startup (which curiously
>>> also appear sometimes due to other reasons), is a lot less of a problem
>>> that a hanging system!
>>
>> Seems you already know the symptoms. Was the console corruption a known issue
>> before submitting? I didn't see anything in the changelog or discussion at
>> submission time?
>
> Honestly I must admit that I have not cared enough about these symptoms until
> now, although I remember regularly seeing a few garbled characters coming from
> the UART during kernel boot, and I thought those might be related to early
> console handover or something similar. I remember seeing this since long
> before DAM support was added for i.MX6.
>
>>> Please don't revert this patch without providing a fix for the crash!
>>
>> This clearly isn't the correct fix, and it makes no sense to build a proper
>> fix on top of a broken one.
>
> If I understand you correctly, you refer to resetting the UART at driver init,
> while it is already transmitting due to something like early-console?
> Is the UART driver really initialized twice for that?
> Because otherwise I don't see how this could do any harm...
> The driver clearly assumes the UART is coming straight out of reset, so if it
> does not, it needs to be reset before the driver can safely do anything.
> Please explain how this can in any way _not_ be the correct solution (or at
> least part of it).
There are many reasons not to do it this way:
1. You're disabling the early console.
2. You're providing a crutch for a broken idea; if some other code in the driver
is expecting the hardware to be initialized before the tty is opened (other than
console which needs to self-sufficient), that code needs to blow up so we can
find it and fix it.
3. It adds significant maintenance burden to have 1 driver do a fundamental
operation differently than the other 100 drivers here.
>> The right way is to revert this and try again.
>
> I am not convinced of that, please explain.
>
>> This time I would look at moving the request_irq() into the startup() method
>> like every other serial driver does.
>
> This sounds indeed like a good solution to avoid the interrupt race, but IMHO
> my patch is a better solution, because the driver still assumes the UART comes
> straight out of reset. This is evident by the fact that the driver only ever
> changes register settings that are not reset defaults. This driver is clearly
> not meant to be used with an UART that has been used before the driver
> initializes it for the first time. The only robust fix for that is to either
> forcibly reset the UART at the beginning or to fix the driver in that it makes
> no assumption about historic register settings, which will lead to basically
> resetting it at startup anyway.
Which is exactly what it already does: imx_startup() => imx_reset().
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
