Dear Peter,
On Fri, 31 Jul 2015 18:03:26 -0400
Peter Hurley <peter@xxxxxxxxxxxxxxxxxx> wrote:
> 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.
You might have a point here, but I have seen that imx_startup() is also called
later on during boot, which would also result in a hickup of the console
(which might explain the garbled characters I sometimes see during boot).
In a regular case, I'd say there should be some sort of hand-over mechanism
between the early-console driver and the real imx uart driver, but since the
former is only meant as a low-level debug tool anyway...
> 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.
What you are saying, is exactly what happened. I discovered this problem while
debugging kexec crashes. This is the fix.
And btw, the console is actually what blew up.
> 3. It adds significant maintenance burden to have 1 driver do a fundamental
> operation differently than the other 100 drivers here.
Hmmm... you pretend to say the other 100 drivers also won't work reliably in a
kexec scenario? Well, let's start hunting them down and fix them too ;-)
> >> 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().
No. I'm afraid it doesn't.
imx_reset() alone does not actually reset the imx uart. It just resets the
fifo state-machine. Just resetting the state-machine is fine for imx_startup()
if you know you initialized the UART control registers to your knowledge and
liking before that, but it is not enough if the uart comes from a totally
unknown state (which happens in the kexec scenario or when using some wild
bootloader).
If you are saying that resetting a device in the probe function is not the
right way to ensure it is in a known state, then please explain how it should
be done correctly and I'll gladly try again.
Best regards,
--
David Jander
Protonic Holland.
--
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
