Dear Peter,
On Mon, 03 Aug 2015 08:06:03 -0400
Peter Hurley <peter@xxxxxxxxxxxxxxxxxx> wrote:
> On 08/03/2015 02:20 AM, David Jander wrote:
> >
> > 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,
>
> ... when setserial or getty start and the tty is opened (which may not occur
> in other configurations) ...
What I see when starting the kernel (console messages get printed), the moment
the UART which is also used as console is initialized, I see that only
"imx_set_termios()" gets called and then this message gets logged:
[ 1.322839] console [ttymxc3] enabled
This is the moment the kernel log buffer gets dumped, but imx_startup() is
never called at this time. imx_startup() only gets called once right before
mounting the rootfs and a few times after that (opening console from
user-space I assume).
This means, that if we reset the uart in imx_startup(), we also need to do it
in imx_console_setup().
> > 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,
>
> There's no requirement that an early console must later become a console.
That's a good point.
This means that no UART driver is allowed to mess with it's registers in the
probe function, right?
> > but since the
> > former is only meant as a low-level debug tool anyway...
>
> Early console is already used on production machines, for better or worse.
I see.
> >> 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.
>
> The changelog mentions nothing about console blowing up; the problem the
> changelog describes is that this driver enabled its irq handler prematurely,
> that's why it blew up, which I already explained how to fix.
>
> If something else is broken, please fix that in a separate patch.
Ok, now I understand your point.
> >> 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 ;-)
>
> I think you'll find that doesn't happen with other drivers because they
> enable irq in startup(), not probe(), and the console is initialized to the
> minimum extent necessary to provide polled mode i/o.
I see.
> >>>> 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.
>
> Ok; seems that the function is mis-named then.
Kind of... but the register bit in question is also called "software reset".
> > 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).
>
> Ok, but you're initializing it to a useless known state.
>
> The correct initial state for hardware is either
> 1. the initial termios that the driver declares when it registers, or
> 2. the line settings specified with the console.
>
> The first is done on first open, the second on console register.
>
> > 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.
>
> If the request_irq() is moved into startup(), then it seems the only
> issue is the proper initialization of UCR1 and UCR2, for both startup() and
> console setup.
Ok, now I see why. Thanks for clarifying.
> What purpose does the RMW of UCR1 and UCR2 serve currently in imx_startup()?
> What bits is that trying to preserve? ISTM, imx_startup() should be
> initializing those registers.
Both imx_startup() and imx_console_setup() I guess, right?
> Console initialization is performed in imx_console_setup(). What console
> options are using and how are you specifying them (command line or device
> tree)?
Command-line: console=ttymxc3,115200
Thanks. I'll try again, avoiding to touch the UART in probe.
Does anyone here object to fully initializing the UART in imx_startup() and
imx_console_setup()?
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
