Trying to fix ITE-887x parallel/serial driver bugs (including unhandled IRQs)
Joseph Krahn
working on a patch to fix this, but have some questions.
Both drivers (8250_pci.c and parport_pc.c) probe randomly for the
chips control I/O port, instead of using the standard PNP-configured
BAR, and they do so independently, stepping on the previous drivers
configuration attempt. I think the two drivers should be merged into
parport_serial.c because this is a combo chip. However, different
variants of the chip have the same PCI vendor/device IDs, but may have
only serial or only parallel ports. Is it OK to have a driver in
parport_serial.c for chip that has only serial ports? Keeping a
separate driver in 8250_pci.c means having two drivers for the same
PCI device ID.
The IT887x chips include an interrupt controller that maps external
IRQ inputs to serialized IRQs. Apparently, the I/O ports only present
a standard interrupt interface when wired for serialized IRQs. For
normal PCI interrupts, it needs a custom interrupt handler to
communicate directly with the interrupt-controller port. The current
serial driver attempts to disable IRQs, and let the core driver revert
to polling. However, it does this incorrectly, and can produce
unhandled IRQs. (Maybe it was tested on a system with Ser. IRQ?) To
avoid extra interrupt-handler code, especially for a less common chip,
is it OK to intentionally provide polling-only support?
Thanks,
Joe Krahn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Alan Cox
> chips control I/O port, instead of using the standard PNP-configured
> BAR, and they do so independently, stepping on the previous drivers
> configuration attempt. I think the two drivers should be merged into
> parport_serial.c because this is a combo chip. However, different
That seems reasonable.
I don't think your description is accurate entirely. The device is picked
up by PCi scans and the INTCBAR is then set up by Linux having checked
for free address ranges. It's hardly 'random probing' and it isn't a
normal BAR or guaranteed to have been configured by anything beforehand.
> The IT887x chips include an interrupt controller that maps external
> IRQ inputs to serialized IRQs. Apparently, the I/O ports only present
> a standard interrupt interface when wired for serialized IRQs. For
> normal PCI interrupts, it needs a custom interrupt handler to
> communicate directly with the interrupt-controller port. The current
I guess the obvious thing to do would be to provide one. The serial code
already supports several such things. What is actually needed or is the
'special' handler simply shared IRQ support in which case it ought t just
work.
> serial driver attempts to disable IRQs, and let the core driver revert
> to polling. However, it does this incorrectly, and can produce
> unhandled IRQs. (Maybe it was tested on a system with Ser. IRQ?) To
> avoid extra interrupt-handler code, especially for a less common chip,
> is it OK to intentionally provide polling-only support?
I'd strongly prefer it worked as well as possible if the the
documentation to fix it exists. What is involved ?
Alan
Joseph Krahn
>> Both drivers (8250_pci.c and parport_pc.c) probe randomly for the
>> chips control I/O port, instead of using the standard PNP-configured
>> BAR, and they do so independently, stepping on the previous drivers
>> configuration attempt. I think the two drivers should be merged into
>> parport_serial.c because this is a combo chip. However, different
>
> That seems reasonable.
>
> I don't think your description is accurate entirely. The device is picked
> up by PCi scans and the INTCBAR is then set up by Linux having checked
> for free address ranges. It's hardly 'random probing' and it isn't a
> normal BAR or guaranteed to have been configured by anything beforehand.
list of possible port ranges:
static int __devinit sio_ite_8872_probe(struct pci_dev *pdev, int autoirq,
int autodma,
const struct parport_pc_via_data *via)
{
short inta_addr[6] = { 0x2A0, 0x2C0, 0x220, 0x240, 0x1E0 };
...
for (i = 0; i < 5; i++) {
base_res = request_region(inta_addr[i], 32, "it887x");
if (base_res) {
...
>
>> The IT887x chips include an interrupt controller that maps external
>> IRQ inputs to serialized IRQs. Apparently, the I/O ports only present
>> a standard interrupt interface when wired for serialized IRQs. For
>> normal PCI interrupts, it needs a custom interrupt handler to
>> communicate directly with the interrupt-controller port. The current
>
> I guess the obvious thing to do would be to provide one. The serial code
> already supports several such things. What is actually needed or is the
> 'special' handler simply shared IRQ support in which case it ought t just
> work.
The interrupt controller (INTC) provides IRQ status bits in a single
I/O port byte. I only have the datasheet for IT8875F, which is 1
parallel and no serial ports, so my understanding of UART IRQs is sort
of reverse engineering. Polling the UART IIR register shows that it
acts like a normal 16550 UART when set to SERIRQ mode, but I get no
IRQs generated. Setting to standard PCI INTA interrupts, the IRQs are
generated, but the UART IIR registers do not set the IRQ bits.
One possibility is to install custom io_serial_in/out functions. When
the value of IIR is requested, the INTC can be checked, and modify the
result to what it should be for a normal UART. That is a bit of a
hack, but makes it much easier to fit into the existing code.
>
>> serial driver attempts to disable IRQs, and let the core driver revert
>> to polling. However, it does this incorrectly, and can produce
>> unhandled IRQs. (Maybe it was tested on a system with Ser. IRQ?) To
>> avoid extra interrupt-handler code, especially for a less common chip,
>> is it OK to intentionally provide polling-only support?
>
> I'd strongly prefer it worked as well as possible if the the
> documentation to fix it exists. What is involved ?
>
> Alan
>
It is easy to fix the code to use INTCBAR to configure the INTC I/O
port. It is probably not too hard to get a proper driver, if I can
find other users to test different hardware configurations. For
hardware wired to use the chip's serialized IRQ, it probably behaves
like standard PC-style chips, and no special IRQ code is needed. It
would be nice to verify this. Maybe a good approach would be to add
support IRQs, but add a module parameter to disable it.
Joe
Joseph Krahn
> On Mon, Apr 12, 2010 at 12:43 PM, Alan Cox <al...@lxorguk.ukuu.org.uk> wrote:
>> I don't think your description is accurate entirely. The device is picked
>> up by PCi scans and the INTCBAR is then set up by Linux having checked
>> for free address ranges. It's hardly 'random probing' and it isn't a
>> normal BAR or guaranteed to have been configured by anything beforehand.
> Yes, but the drivers don't use INTCBAR. They scan through a herd-coded
> list of possible port ranges:
>
> static int __devinit sio_ite_8872_probe(struct pci_dev *pdev, int autoirq,
> int autodma,
> const struct parport_pc_via_data *via)
> {
> short inta_addr[6] = { 0x2A0, 0x2C0, 0x220, 0x240, 0x1E0 };
> ...
> for (i = 0; i < 5; i++) {
> base_res = request_region(inta_addr[i], 32, "it887x");
> if (base_res) {
> ...
has already been mapped to an available range. The problem with the
recent 8250_pci driver is that it repeats the above process,
requesting an I/O range while the existing range was never released.
This is how I think it should be done:
/* Map INTC control to BAR 0 */
intc_base = pci_resource_start(pdev,0);
intc_res = request_region(intc_base, ITE_887x_POSIO0_IOSIZE,
"it887x_intc");
...
> One possibility is to install custom io_serial_in/out functions. When
> the value of IIR is requested, the INTC can be checked, and modify the
> result to what it should be for a normal UART. That is a bit of a
> hack, but makes it much easier to fit into the existing code.
I am testing a custom io_serial_in() to translate IRQ info. It looks
like a good solution. Based on the current parport_pc driver, the
partport I/O may work like a standard port, and not need any extra IRQ
code.
Instead of adding another UPIO_ definition, it should be possible to
define serial_in and serial_out in the 8250_pci.c quirks init
function. In some cases, but not all, the result from
set_io_from_upio() is allowed to be overridden, so this may be the
intended design. It appears that UPIO_TSI exists only to map
serial_in/out functions.
Thanks,
Alan Cox
> > normal BAR or guaranteed to have been configured by anything beforehand.
> Yes, but the drivers don't use INTCBAR. They scan through a herd-coded
> list of possible port ranges:
They can't just use INTCBAR as it may not have been configured by anything
before the device is probed. We do need to find and assign suitable ports
although this should probably be done via the PCI quirks on PCI setup.
> The interrupt controller (INTC) provides IRQ status bits in a single
> I/O port byte. I only have the datasheet for IT8875F, which is 1
> parallel and no serial ports, so my understanding of UART IRQs is sort
> of reverse engineering. Polling the UART IIR register shows that it
> acts like a normal 16550 UART when set to SERIRQ mode, but I get no
> IRQs generated. Setting to standard PCI INTA interrupts, the IRQs are
> generated, but the UART IIR registers do not set the IRQ bits.
Weird design.
http://kr.ic-on-line.cn/IOL/viewpdf/IT8871F_200216.htm
provides a tiny bit of info on the uart side but I've not found any
complete documentation.
> One possibility is to install custom io_serial_in/out functions. When
> the value of IIR is requested, the INTC can be checked, and modify the
> result to what it should be for a normal UART. That is a bit of a
> hack, but makes it much easier to fit into the existing code.
Seems a good starting point. The serial side code is robust for shared
IRQs.
Alan
Joseph Krahn
>> > for free address ranges. It's hardly 'random probing' and it isn't a
>> > normal BAR or guaranteed to have been configured by anything beforehand.
>
>> Yes, but the drivers don't use INTCBAR. They scan through a herd-coded
>> list of possible port ranges:
>
> They can't just use INTCBAR as it may not have been configured by anything
> before the device is probed. We do need to find and assign suitable ports
> although this should probably be done via the PCI quirks on PCI setup.
>
>> The interrupt controller (INTC) provides IRQ status bits in a single
>> I/O port byte. I only have the datasheet for IT8875F, which is 1
>> parallel and no serial ports, so my understanding of UART IRQs is sort
>> of reverse engineering. Polling the UART IIR register shows that it
>> acts like a normal 16550 UART when set to SERIRQ mode, but I get no
>> IRQs generated. Setting to standard PCI INTA interrupts, the IRQs are
>> generated, but the UART IIR registers do not set the IRQ bits.
>
> Weird design.
>
> http://kr.ic-on-line.cn/IOL/viewpdf/IT8871F_200216.htm
>
> provides a tiny bit of info on the uart side but I've not found any
> complete documentation.
needed in MS-Windows, implying standard I/O interfaces. I think the
chip was originally designed to build into motherboards with
serialized IRQs, and the INTA IRQ quirk is a design flaw.
The ite887x 8250_pci author, Niels de Vos, had the relevant data sheets:
http://www.mail-archive.com/linux-...@vger.kernel.org/msg137333.html
However, an email bounced. I'll see if I can contact someone at the company.
>
>> One possibility is to install custom io_serial_in/out functions. When
>> the value of IIR is requested, the INTC can be checked, and modify the
>> result to what it should be for a normal UART. That is a bit of a
>> hack, but makes it much easier to fit into the existing code.
>
> Seems a good starting point. The serial side code is robust for shared
> IRQs.
>
> Alan
>
My current code moves the driver to parport_serial.c, and 8250_pci.c
only has the code to install the custom serial_in(), to deal with
interrupts.
The different 887x devices can have no serial or no parallel ports,
but all have the same PCI id. My code just lets parport_serial handle
the serial-only and parallel-only cases. Is that approach better than
having two modules that both match the same PCI id?
The code could also be left separate, with parport_serial skipping
devices with no parallel ports. The two device-init routines should
not interfere with each other as long as they both use PCI BAR 0 to
map INTCBAR.
The custom serial_in function only sets the interrupt pending bits for
IIR, and not the interrupt-type bits 1-3. I think the current serial
driver not use these status bits, so this should be OK.
Thanks,
Joe Krahn
Alan Cox
> the serial-only and parallel-only cases. Is that approach better than
> having two modules that both match the same PCI id?
Most distributions load each driver that matches an id anyway so I think
so - puts the complexity in one place.
Alan