Re: mpii_start() vs. mfii_start(): bus_space_write_raw_8(), bus_space_barrier()

[Taylor R Campbell]

> Date: Tue, 11 Oct 2022 16:50:17 +0200
> From: Edgar Fuß 
> 
> void
> mpii_start(struct mpii_softc *sc, struct mpii_ccb *ccb)
> {
>   struct mpii_request_header  *rhp;
>   struct mpii_request_descr   descr;
> #if defined(__LP64__) && 0
>   u_long   *rdp = (u_long *)
> #else

Note the `&& 0' here -- this is dead code!  Only the else branch is
taken.

> #if defined(__LP64__) && 0
>   bus_space_write_raw_8(sc->sc_iot, sc->sc_ioh,
>   MPII_REQ_DESCR_POST_LOW, *rdp);
> #else

Same here.

> 1. __LP64__ handling: Is the LP64 case simply an optimization or is it 
>safer on the relevant platforms?

Depends on the hardware: some hardware supports 64-bit read/write
transactions on registers; some hardware does not, and `64-bit'
registers must be read/written in 32-bit halves.  It may be an
optimization on LP64 platforms, or it may be necessary for the
transaction to be atomic.

> 2. bus_space_write_raw_8(): I can't find any description or references for 
>that function. Should that be bus_space_write_8()?

It appears to have been copied from OpenBSD, whose
bus_space_read/write_raw_* functions appear to correspond to NetBSD's
bus_space_read/write_stream_* functions -- they don't do any byte
order conversion, so the I/O device has to have the same byte order as
the CPU, which is unusual but happens sometimes.

Whether to use the stream or non-stream version depends on how the
device works, for which you'll have to consult the manual and/or do
science on it.  LP64 shouldn't make a differnce here -- the LP64 and
non-LP64 cases should agree on whether to use raw/stream or not, and
they do appear to agree in OpenBSD.

> 3. Single vs. double bus_space_barrier(): It strikes me as odd that 
>mpii_start() has a call between the two bus_space_write_4() calls while 
>mfii_start() hasn't. It also look suspicious to me that both calls use 
>MPII_REQ_DESCR_POST_LOW.

Unless the bus space is mapped with BUS_SPACE_MAP_PREFETCHABLE, the
barrier calls are no-ops and can be flushed.  It is probably a
harmless mistake that both barriers use MPII_REQ_DESCR_POST_LOW -- the
second one was probably meant to use MPII_REQ_DESCR_POST_HIGH.

mpii_start() vs. mfii_start(): bus_space_write_raw_8(), bus_space_barrier()

[Edgar Fuß]

I'm investigating timeout problems with my mpii(4) device (after the driver 
has been converted to MSI(-X). I'm trying to understand both 
sys/dev/pci/mpii.c and mfii.c since they adress the same hardware with 
different firmware.

Comparing mpii_start() with mfii_start(), I'm stumbling over a number of 
differences I don't understand (I've removed some debug statements from 
mpii_start()):


void
mpii_start(struct mpii_softc *sc, struct mpii_ccb *ccb)
{
struct mpii_request_header  *rhp;
struct mpii_request_descr   descr;
#if defined(__LP64__) && 0
u_long   *rdp = (u_long *)
#else
u_int32_t*rdp = (u_int32_t *)
#endif

[...]
bus_dmamap_sync(sc->sc_dmat, MPII_DMA_MAP(sc->sc_requests),
ccb->ccb_offset, sc->sc_request_size,
BUS_DMASYNC_PREREAD | BUS_DMASYNC_PREWRITE);
[...]

#if defined(__LP64__) && 0
bus_space_write_raw_8(sc->sc_iot, sc->sc_ioh,
MPII_REQ_DESCR_POST_LOW, *rdp);
#else
mutex_enter(>sc_req_mtx);
bus_space_write_4(sc->sc_iot, sc->sc_ioh,
MPII_REQ_DESCR_POST_LOW, rdp[0]);
bus_space_barrier(sc->sc_iot, sc->sc_ioh,
MPII_REQ_DESCR_POST_LOW, 8, BUS_SPACE_BARRIER_WRITE);

bus_space_write_4(sc->sc_iot, sc->sc_ioh,
MPII_REQ_DESCR_POST_HIGH, rdp[1]);
bus_space_barrier(sc->sc_iot, sc->sc_ioh,
MPII_REQ_DESCR_POST_LOW, 8, BUS_SPACE_BARRIER_WRITE);
mutex_exit(>sc_req_mtx);
#endif
}


static void
mfii_start(struct mfii_softc *sc, struct mfii_ccb *ccb)
{
uint32_t *r = (uint32_t *)>ccb_req;
#if defined(__LP64__)
uint64_t buf;
#endif

bus_dmamap_sync(sc->sc_dmat, MFII_DMA_MAP(sc->sc_requests),
ccb->ccb_request_offset, MFII_REQUEST_SIZE,
BUS_DMASYNC_PREREAD | BUS_DMASYNC_PREWRITE);

#if defined(__LP64__)
buf = ((uint64_t)r[1] << 32) | r[0];
bus_space_write_8(sc->sc_iot, sc->sc_ioh, MFI_IQPL, buf);
#else
mutex_enter(>sc_post_mtx);
bus_space_write_4(sc->sc_iot, sc->sc_ioh, MFI_IQPL, r[0]);
bus_space_write_4(sc->sc_iot, sc->sc_ioh, MFI_IQPH, r[1]);
bus_space_barrier(sc->sc_iot, sc->sc_ioh,
MFI_IQPL, 8, BUS_SPACE_BARRIER_WRITE);
mutex_exit(>sc_post_mtx);
#endif
}


1. __LP64__ handling: Is the LP64 case simply an optimization or is it 
   safer on the relevant platforms?

2. bus_space_write_raw_8(): I can't find any description or references for 
   that function. Should that be bus_space_write_8()?

3. Single vs. double bus_space_barrier(): It strikes me as odd that 
   mpii_start() has a call between the two bus_space_write_4() calls while 
   mfii_start() hasn't. It also look suspicious to me that both calls use 
   MPII_REQ_DESCR_POST_LOW.

Can someone please enlighten me?