Re: msk(4) require to sync status buffer

[KIYOHARA Takashi <kiyohara%kk.iij4u.or.jp@localhost>]

Hi! tsutsui-san,


From: Izumi Tsutsui <tsutsui%ceres.dti.ne.jp@localhost>
Date: Sat, 23 Feb 2008 01:33:34 +0900

> kiyohara%kk.iij4u.or.jp@localhost wrote:
> 
> > > > I think that the processing of the status buffer of msk(4) is strange.
> > > > It doesn't do at interrupt handling at all though it does pre-read-sync
> > > > only first.  I think that this cannot read a correct value with like CPU
> > > > of writeback cache (not PC/AT).

> > My KURO-BOX/PRO don't work msk(4).  It CPU is ARM Feroceon for KURO-BOX/PRO
> > with write-back cache.
> 
> I guess more strict bus_dmamap_sync(9) calls are required:

hmmm...
My opinion is different. 



> Index: if_msk.c
> ===================================================================
> RCS file: /cvsroot/src/sys/dev/pci/if_msk.c,v
> retrieving revision 1.16
> diff -u -r1.16 if_msk.c
> --- if_msk.c  7 Feb 2008 01:21:56 -0000       1.16
> +++ if_msk.c  22 Feb 2008 16:30:40 -0000
> @@ -1745,6 +1745,8 @@
>                   BUS_DMASYNC_POSTREAD|BUS_DMASYNC_POSTWRITE);
>  
>               cur_tx = &sc_if->sk_rdata->sk_tx_ring[cons];
> +             MSK_CDTXSYNC(sc_if, cons, 1,
> +                 BUS_DMASYNC_PREREAD);
>               sk_ctl = cur_tx->sk_ctl;
>  #ifdef MSK_DEBUG
>               if (mskdebug >= 2)

Why need it?
msk seems not to update the value of this buffer to me.  For instance,
SK_Y2_TXCTL_LASTFRAG is written with msk_encap() by the driver.


> @@ -1844,6 +1846,7 @@
>       MSK_CDSTSYNC(sc, sc->sk_status_idx,
>           BUS_DMASYNC_POSTREAD|BUS_DMASYNC_POSTWRITE);
>       cur_st = &sc->sk_status_ring[sc->sk_status_idx];
> +     MSK_CDSTSYNC(sc, sc->sk_status_idx, BUS_DMASYNC_PREREAD);
>  
>       while (cur_st->sk_opcode & SK_Y2_STOPC_OWN) {
>               cur_st->sk_opcode &= ~SK_Y2_STOPC_OWN;
> @@ -1877,6 +1880,8 @@
>               MSK_CDSTSYNC(sc, sc->sk_status_idx,
>                   BUS_DMASYNC_POSTREAD|BUS_DMASYNC_POSTWRITE);
>               cur_st = &sc->sk_status_ring[sc->sk_status_idx];
> +             MSK_CDSTSYNC(sc, sc->sk_status_idx,
> +                 BUS_DMASYNC_PREREAD);
>       }
>  
>       if (status & SK_Y2_IMR_BMU) {

We think that we should do pre-read-sync before reading DMA.  It is not
immediately before the driver's reading.


I attached new patch.

Thanks,
--
kiyohara

Index: if_msk.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/if_msk.c,v
retrieving revision 1.16
diff -u -r1.16 if_msk.c
--- if_msk.c    7 Feb 2008 01:21:56 -0000       1.16
+++ if_msk.c    23 Feb 2008 09:00:48 -0000
@@ -798,7 +798,7 @@
 void msk_reset(struct sk_softc *sc)
 {
        u_int32_t imtimer_ticks, reg1;
-       int reg;
+       int reg, i;
 
        DPRINTFN(2, ("msk_reset\n"));
 
@@ -893,6 +893,8 @@
        /* Reset status ring. */
        bzero((char *)sc->sk_status_ring,
            MSK_STATUS_RING_CNT * sizeof(struct msk_status_desc));
+       for (i = 0; i < MSK_STATUS_RING_CNT; i++)
+               MSK_CDSTSYNC(sc, i, BUS_DMASYNC_PREREAD);
        sc->sk_status_idx = 0;
 
        sk_win_write_4(sc, SK_STAT_BMU_CSR, SK_STAT_BMU_RESET);
@@ -1256,8 +1258,6 @@
                goto fail_5;
        }
        sc->sk_status_ring = (struct msk_status_desc *)kva;
-       bzero(sc->sk_status_ring,
-           MSK_STATUS_RING_CNT * sizeof(struct msk_status_desc));
 
        /* Reset the adapter. */
        msk_reset(sc);
@@ -1841,8 +1841,7 @@
                msk_intr_yukon(sc_if1);
        }
 
-       MSK_CDSTSYNC(sc, sc->sk_status_idx,
-           BUS_DMASYNC_POSTREAD|BUS_DMASYNC_POSTWRITE);
+       MSK_CDSTSYNC(sc, sc->sk_status_idx, BUS_DMASYNC_POSTREAD);
        cur_st = &sc->sk_status_ring[sc->sk_status_idx];
 
        while (cur_st->sk_opcode & SK_Y2_STOPC_OWN) {
@@ -1872,12 +1871,17 @@
                        aprint_error("opcode=0x%x\n", cur_st->sk_opcode);
                        break;
                }
+               /* Write back after clearing SK_Y2_STOPC_OWN of opcode */
+               MSK_CDSTSYNC(sc, sc->sk_status_idx,
+                   BUS_DMASYNC_PREREAD|BUS_DMASYNC_PREWRITE);
+
                SK_INC(sc->sk_status_idx, MSK_STATUS_RING_CNT);
 
-               MSK_CDSTSYNC(sc, sc->sk_status_idx,
-                   BUS_DMASYNC_POSTREAD|BUS_DMASYNC_POSTWRITE);
+               MSK_CDSTSYNC(sc, sc->sk_status_idx, BUS_DMASYNC_POSTREAD);
                cur_st = &sc->sk_status_ring[sc->sk_status_idx];
        }
+       /* Invalidate the status buffer that has already been cached */
+       MSK_CDSTSYNC(sc, sc->sk_status_idx, BUS_DMASYNC_PREREAD);
 
        if (status & SK_Y2_IMR_BMU) {
                CSR_WRITE_4(sc, SK_STAT_BMU_CSR, SK_STAT_BMU_IRQ_CLEAR);