[PATCH 1/2] ARM: Tegra: dt: Split out separate Tegra SoC DT

[Stephen Warren]

Grant Likely wrote at Thursday, May 12, 2011 5:02 PM:
> On Thu, May 12, 2011 at 6:40 PM, Stephen Warren <swarren at nvidia.com> wrote:
> > Grant Likely wrote at Monday, May 02, 2011 2:16 PM:
> >> On Mon, May 2, 2011 at 12:00 PM, Stephen Warren <swarren at nvidia.com> wrote:
> >> > Olof Johansson wrote at Sunday, May 01, 2011 8:56 AM:
> >> >> On Fri, Apr 29, 2011 at 10:12:30PM -0600, Stephen Warren wrote:
> >> >> A mostly unrelated question below.
> >> >>
> >> >> [...]
> >> >>
> >> >> > +   serial at 70006000 {
> >> >> > +           compatible = "nvidia,tegra250-uart";
> >> >>
> >> >> I know this is how Grant specified it, but shouldn't these also have
> >> >> a compat for ns16550?
> >> >
> >> > At present, I'm not sure it is technically compatible. If you look at
> >> > drivers/tty/serial/of_serial.c, you'll see:
> >> >
> >> > static struct of_device_id __devinitdata of_platform_serial_table[] =
> {
> >> > ...
> >> >        { .compatible = "ns16550",  .data = (void *)PORT_16550, },
> >> > ...
> >> >        { .compatible = "nvidia,tegra250-uart",  .data = (void *)PORT_XSCALE, },
> >> >
> >> > That PORT_XSCALE is different to the ns16550 entry. Arguably, that
> >> > field should be something that comes from the device tree, just
> >> > like e.g. reg-shift, but it certainly doesn't right now.
> >> >
> >> > Grant, what are your thoughts on this?
> >>
> >> Heh, when I added that line I just mirrored the 'type' of 16550 as
> >> detected by the 8250.c driver when it probes and made a mental note
> >> that it should be revisited.  I don't know the hardware very well, so
> >> I cannot easily say what the right thing to do is, but making it
> >> ns16550 compatible does seem to make sense.
> >
> > I looked into this a bit more and found the following:
> >
> > In all the existing board files for Tegra, none of the
> > plat_serial8250_port initializations specify any type value, and equally
> > the flags do not include UPF_FIXED_TYPE. This causes the UART driver to
> > probe (run tests on) the hardware to determine exactly what type of UART
> > is present.
> >
> > However, of_serial.c does set UPF_FIXED_TYPE, and hence port->type must
> > have a valid value. I suspect it'd be a bad idea to change this since
> > it'd affect a ton of extant PPC platforms.
> >
> > Equally, PORT_XSCALE does imply different HW behavior to any of the
> > other types, so tegra250.dtsi shouldn't just specify that it's compatible
> > with "ns16550".
> 
> I investigated and found the same thing, and that is why I set up the
> current code the way I did; to preserve the current behaviour.
> 
> However, is the current behaviour even right?  My impression from the
> code is that probing for the type of ns16550 is little more than an
> accident.  Is xscale really the best behaviour to select fro this
> device?

The detection for XSCALE port type is solely based on the writability
of a single bit in the IER. The Tegra documentation says this bit is
undefined, so I agree it's probably an accident that Tegra's UART is
being detected as XSCALE. If I disable that part of the auto-detection,
Tegra's UART is detected as a 16550, as expected.

However, that setting doesn't work correctly. Specifically, on Tegra,
there's a bit in the Interrupt Enable Register (IER) that needs to be
set to enable interrupt on RX timeout. Without this, one needs to fill
the RX FIFO up to its attention level before the HW interrupts and
before SW sees any of those characters.

That's one of the two things that the XSCALE port type causes to happen.
The other thing is programming of the IER UUE bit, which is the bit used
in the auto-detection that Tegra apparently doesn't actually implement.

While researching this, I found a patch to 8250.c to add a specific
Tegra port type:

http://nv-tegra.nvidia.com/gitweb/?p=linux-2.6.git;a=commitdiff_plain;h=ff4d0279c458b724d273efd88ee87ff7caf48991

I think the overall answer therefore is to:

a) Upstream that patch, although I'll want to separate out the new port
type definition from the break handling, since I haven't researched the
break handling part of that patch. Plus there are a couple of things I
need to follow up on internally before upstreaming.

b) Update of_serial.c to point the new Tegra entry at PORT_TEGRA instead
of PORT_XSCALE.

c) Update the hard-coded board files to specify port_type = PORT_TEGRA
for the UARTs rather than allowing probing.

Does that seem reasonable to everyone?

Thanks for reading!

-- 
nvpublic