8250.c::autoconfig() fails loopback test on MPC824[15]

8250.c::autoconfig() fails loopback test on MPC824[15]

[Guennadi Liakhovetski]

Hi,

I tried using of_serial.c on a (PPC) MPC8241 based system, which has a 
"16650A" compatible double UART built into the SoC. Using of_serial.c 
causes the ports to be autoconfigured, and this fails. The loopback test 
fails, because the MSR register on 824[15] doesn't implement the 
UART_MSR_DCD bit. Question: what's better, teach 8250.c to handle UARTs 
without this bit, or set the UPF_SKIP_TEST bit in of_serial.c for these 
SOCs to skip the loopback test altogether? The latter is certainly easier 
and affects much fewer systems, so, I'd go for that.

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: 8250.c::autoconfig() fails loopback test on MPC824[15]

[Arnd Bergmann]

On Sunday 05 August 2007, Guennadi Liakhovetski wrote:
> I tried using of_serial.c on a (PPC) MPC8241 based system, which has a 
> "16650A" compatible double UART built into the SoC. Using of_serial.c 
> causes the ports to be autoconfigured, and this fails. The loopback test 
> fails, because the MSR register on 824[15] doesn't implement the 
> UART_MSR_DCD bit. Question: what's better, teach 8250.c to handle UARTs 
> without this bit, or set the UPF_SKIP_TEST bit in of_serial.c for these 
> SOCs to skip the loopback test altogether? The latter is certainly easier 
> and affects much fewer systems, so, I'd go for that.

Yes, that sounds good. Just make sure you test the "compatible" property
in the device node for something appropriate. In of_platform_serial_probe(),
you can then do something like

	if (of_device_is_compatible(ofdev, "mpc8241-serial"))
		flags |= UPF_SKIP_TEST;

	Arnd <><

Re: 8250.c::autoconfig() fails loopback test on MPC824[15]

[Guennadi Liakhovetski]

On Sun, 5 Aug 2007, Arnd Bergmann wrote:

> On Sunday 05 August 2007, Guennadi Liakhovetski wrote:
> > I tried using of_serial.c on a (PPC) MPC8241 based system, which has a 
> > "16650A" compatible double UART built into the SoC. Using of_serial.c 
> > causes the ports to be autoconfigured, and this fails. The loopback test 
> > fails, because the MSR register on 824[15] doesn't implement the 
> > UART_MSR_DCD bit. Question: what's better, teach 8250.c to handle UARTs 
> > without this bit, or set the UPF_SKIP_TEST bit in of_serial.c for these 
> > SOCs to skip the loopback test altogether? The latter is certainly easier 
> > and affects much fewer systems, so, I'd go for that.
> 
> Yes, that sounds good. Just make sure you test the "compatible" property
> in the device node for something appropriate. In of_platform_serial_probe(),
> you can then do something like
> 
> 	if (of_device_is_compatible(ofdev, "mpc8241-serial"))
> 		flags |= UPF_SKIP_TEST;

That would be a possibility, but that would mean all 8241/8245 have to 
adjust their .dts. Ok, there are not so many of them in the mainline now 
(in fact, hardly any apart from linkstation:-)), still. Cannot we use 
something already available to just check if we're running on such a CPU? 
Worst case - find and parse cpu node, or maybe using some cpu_feature?

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: 8250.c::autoconfig() fails loopback test on MPC824[15]

[Arnd Bergmann]

On Sunday 05 August 2007, Guennadi Liakhovetski wrote:
> That would be a possibility, but that would mean all 8241/8245 have to 
> adjust their .dts. Ok, there are not so many of them in the mainline now 
> (in fact, hardly any apart from linkstation:-)), still. Cannot we use 
> something already available to just check if we're running on such a CPU? 
> Worst case - find and parse cpu node, or maybe using some cpu_feature?

It's fundamentally a property of the serial controller implementation,
not of the processor, so the cpu_features are the wrong place to put
this. There should at least be a generic way to define thsi in the device
tree so that _future_ trees can just mark the port as compatible with
one that has this bug.

If you want to work around existing systems that don't mention this
in the device_tree, you could perhaps use machine_is(foo) to test
for it.

Another option altogether would be to allow the device node to
specify the linux specific serial port flags in a separate property,
like "linux,uart-port-flags" that contains the same flags that
setserial can set from user space. That would also be useful
if you want to specify UPF_MAGIC_MULTIPLIER on certain high-speed
ports, because it cannot be autoprobed.

	Arnd <><

Re: 8250.c::autoconfig() fails loopback test on MPC824[15]

[Jon Loeliger]

So, like, the other day Arnd Bergmann mumbled:
> On Sunday 05 August 2007, Guennadi Liakhovetski wrote:
> > That would be a possibility, but that would mean all 8241/8245 have to 
> > adjust their .dts. Ok, there are not so many of them in the mainline now 
> > (in fact, hardly any apart from linkstation:-)), still. Cannot we use 

Heck, I'm slowly working on the StorCenter too.. :-)

I supect it will have the same issue in the end, right?

jdl

Re: 8250.c::autoconfig() fails loopback test on MPC824[15]

[Guennadi Liakhovetski]

On Sun, 5 Aug 2007, Jon Loeliger wrote:

> So, like, the other day Arnd Bergmann mumbled:
> > On Sunday 05 August 2007, Guennadi Liakhovetski wrote:
> > > That would be a possibility, but that would mean all 8241/8245 have to 
> > > adjust their .dts. Ok, there are not so many of them in the mainline now 
> > > (in fact, hardly any apart from linkstation:-)), still. Cannot we use 
> 
> Heck, I'm slowly working on the StorCenter too.. :-)
> 
> I supect it will have the same issue in the end, right?

...if you choose to use of_serial.c, yes, if you don't use it and just use 
legacy_serial.c, then you're fine.

BTW, my offer still holds to see if we can build a single kernel for both 
with just specific device-trees, but that's a separate matter:-)

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: 8250.c::autoconfig() fails loopback test on MPC824[15]

[Jon Loeliger]

So, like, the other day Guennadi Liakhovetski mumbled:
> On Sun, 5 Aug 2007, Jon Loeliger wrote:
> 
> ...if you choose to use of_serial.c, yes, if you don't use it and just use 
> legacy_serial.c, then you're fine.

Ah, I see.  OK.

> BTW, my offer still holds to see if we can build a single kernel for both 
> with just specific device-trees, but that's a separate matter:-)

Indeed.  I'm fully on-board with that goal, but it may take a couple
rounds to refine it into that state.

One question I have is the stupid ethernet driver.  Is the driver
that is in mainline now usable on the StorCenter?  I haven't
waded through the supplied vendor patch to verify how it compares
to the r8169 in mainline yet.

Also, what do you want to do about the Kconfig set up for these then?
Specifically, do you want to have "TurboStation" and "StorCenter"?
Or would it make more sense to have something like a generic
"NAS8241" with sub-choices of "storcenter" and "turbostation"?

Thanks,
jdl

Re: 8250.c::autoconfig() fails loopback test on MPC824[15]

[Arnd Bergmann]

On Sunday 05 August 2007, Guennadi Liakhovetski wrote:
> 
> > I supect it will have the same issue in the end, right?
> 
> ...if you choose to use of_serial.c, yes, if you don't use it and just use 
> legacy_serial.c, then you're fine.

But of_serial can be a loadable module, which means you still get into
trouble if you load it, even if the port was originally initialized
by legacy_serial.

Maybe the best solution would be for 824[15] to not claim compatibility
with 8250 at all then. If the device tree contains an entry that matches
what the generic driver looks for, it better be something that can
be handled by that driver.

	Arnd <><

Re: 8250.c::autoconfig() fails loopback test on MPC824[15]

[Segher Boessenkool]

> Maybe the best solution would be for 824[15] to not claim compatibility
> with 8250 at all then.

Or at least it should have a more specific entry for this
"special" 16x50 UART, and that one should be probed first.

> If the device tree contains an entry that matches
> what the generic driver looks for, it better be something that can
> be handled by that driver.

Pretty much; you can't make this rule too strict though,
if a device mostly works with the generic driver, you can
claim compatibility with it -- keep in mind that that can
come back to bite you though, like in this case.  The
advantages do outweigh the disadvantages sometimes, it's
all a tradeoff; avoid it if possible.


Segher

Re: 8250.c::autoconfig() fails loopback test on MPC824[15]

[Guennadi Liakhovetski]

On Mon, 6 Aug 2007, Segher Boessenkool wrote:

> > Maybe the best solution would be for 824[15] to not claim compatibility
> > with 8250 at all then.
> 
> Or at least it should have a more specific entry for this
> "special" 16x50 UART, and that one should be probed first.
> 
> > If the device tree contains an entry that matches
> > what the generic driver looks for, it better be something that can
> > be handled by that driver.
> 
> Pretty much; you can't make this rule too strict though,
> if a device mostly works with the generic driver, you can
> claim compatibility with it -- keep in mind that that can
> come back to bite you though, like in this case.  The
> advantages do outweigh the disadvantages sometimes, it's
> all a tradeoff; avoid it if possible.

Well, the 8250 driver DOES already support devices without the loopback 
test support, that's what that bit there is for, isn't it? So, we just 
have to use it, as well as others do (grep -r UPF_SKIP_TEST 
drivers/serial/). I'll go for the extra compatible property, as suggested, 
seems like the most commonly accepted wa to handle this (and the easiest 
to implement).

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: 8250.c::autoconfig() fails loopback test on MPC824[15]

[Segher Boessenkool]

> Another option altogether would be to allow the device node to
> specify the linux specific serial port flags in a separate property,
> like "linux,uart-port-flags" that contains the same flags that
> setserial can set from user space. That would also be useful
> if you want to specify UPF_MAGIC_MULTIPLIER on certain high-speed
> ports, because it cannot be autoprobed.

Such high-speed ports should simply use a proper "compatible"
entry.  Similarly with the problem at hand.


Segher