On Fri, May 13, 2011 at 9:28 PM, Stephen Warren <swarren@xxxxxxxxxx> wrote:
> Grant Likely wrote at Thursday, May 12, 2011 5:02 PM:
>> On Thu, May 12, 2011 at 6:40 PM, Stephen Warren <swarren@xxxxxxxxxx> wrote:
>> > Grant Likely wrote at Monday, May 02, 2011 2:16 PM:
>> >> On Mon, May 2, 2011 at 12:00 PM, Stephen Warren <swarren@xxxxxxxxxx> 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@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?
Sounds right to me.
g.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
