Re: com(4) and LSI bug workaround

[Izumi Tsutsui <tsutsui%ceres.dti.ne.jp@localhost>]

kiyohara@ wrote:

> >> > It looks meaningless that the (*sc_vendor_workaround)() hook function
> >> > is inside of MD if (sc->sc_type == COM_TYPE_ARMADAXP) statement.
> >> > 
> >> > Isn't it simpler to prepare a MD hook function that returns
> >> > (possible pre-processed) IIR register value that can be used
> >> > to check if the interrupt for the device is pending or not?
> >> 
> >> I performed operation to a com register into com.c.
> >> I think that it is not so brief to return the value of both IIR(u_char)
> >> and an error moreover.
> > 
> > Sorry I don't understand what you mean.
> > 
> > Do you think the name of "sc_vendor_workaround" is really appropriate
> > for the function that is called only in COM_TYPE_ARMADAXP case?
> 
> I apologize for the first explanation having been lacking.
> 
> I consider calling this method for evasion processing of the problem
> of LSI.  Only ARMADAXP uses it only in comintr() now.

First of all, your guess has no evidence and actually it seems incorrect.

There are some articles that indicate it's a documented future:
http://permalink.gmane.org/gmane.linux.ports.arm.kernel/203948
>> The UART controller used in the Armada 370 and Armada XP SoCs is the
>> Synopsys DesignWare 8250 (aka Synopsys DesignWare ABP UART).
 :
>> The DW APB has an extra interrupt that gets raised when
>> writing to the LCR when busy.

The Linux DesignWare 8250 driver handles these interrupts as following:
http://lxr.linux.no/#linux+v3.11.2/drivers/tty/serial/8250/8250_dw.c#L108
---
 113        if (serial8250_handle_irq(p, iir)) {
 114                return 1;
 115        } else if ((iir & UART_IIR_BUSY) == UART_IIR_BUSY) {
 116                /* Clear the USR and write the LCR again. */
 117                (void)p->serial_in(p, d->usr_reg);
 118                p->serial_out(p, UART_LCR, d->last_lcr);
 119
 120                return 1;
 121        }
---

Although Linux driver says reading USR register clears BUSY bit in it,
it looks also incorrect because it shows chip status.

Then, the original com.c rev 1.309 (which was written by Marbell
and its vendor?) seems absolutely correct for me (though it isn't
16750's future):
http://nxr.netbsd.org/xref/src/sys/dev/ic/com.c?r=1.309#1919
---
1919    /* Handle ns16750-specific busy interrupt. */
1920 #ifdef COM_16750
1921    int timeout;
1922    if ((iir & IIR_BUSY) == IIR_BUSY) {
1923            for (timeout = 10000;
1924                (CSR_READ_1(regsp, COM_REG_USR) & 0x1) != 0; timeout--)
1925                    if (timeout <= 0) {
1926                            aprint_error_dev(sc->sc_dev,
1927                                "timeout while waiting for BUSY interrupt "
1928                                "acknowledge\n");
1929                            mutex_spin_exit(&sc->sc_lock);
1930                            return (0);
1931                    }
1932 
1933            CSR_WRITE_1(regsp, COM_REG_LCR, sc->sc_lcr);
1934            iir = CSR_READ_1(regsp, COM_REG_IIR);
1935    }
---
and your changes against this block should be reverted.

> However, this may be used during the processing from which LSI besides
> the future differs.
> In such a case, is a different method from this prepared?
> 
>   For example
> 
>       void *sc_vendor_workaround{1,2,3...}(void);
> 
> We are evaluating the conditions of COM_TYPE_ARMADAXP and can cope with
> it by one method.

Why should such MD workaround functions be into MI softc?

Isn't it much simpler and explicit to call such a MD workaround
function directly from comintr() and wrap those blocks with
#if COM_XXXMDname > 0 / #endif with "needs-flag" declaration
in the config(9) file?

> When it depends especially, LSI with much fault may come out to a market.

Again, it's your guess.  We should consider how we can/should abstruct
them once after we really see such buggy chips that need workarounds.

Preparing dumb hooks in MI softc without abstruction is not the right
answer.

> > The requirement in the MI interrupt handler is just
> > "whether interrupts are pending or not," isn't it?
> 
> Yes, it is.

Then, all you need is the iir value and you don't have to return errno.

If there were really chip bugs that cause interrupts with NOPEND bit,
preparing a MD function that returns proper IIR values (which might
handle the USR register etc.) seems the right abstruction for me.

---
Izumi Tsutsui