Re: bus_space(9) overrides & resource reservations

[David Young <dyoung%pobox.com@localhost>]

On Sat, May 29, 2010 at 07:18:59PM +0900, Izumi Tsutsui wrote:
> > > > If the name troubles you, is bus_space_tag_refine() better?
> > > 
> > > Aren't many other guys confused by the name "create tag"?
> > 
> > You're asking me? :-)
> 
> Yes.

I cannot answer your question, there is only one of me.

It may be helpful to write and think about it as *extending* a tag.

>  * Did you explain why do you need these new APIs?

MI bus drivers such as ppb(4) and cbb(4) will use bus_space_tag_create()
to install extensions for handling exceptions affecting their
subordinate buses, and to manage resources on subordinate buses.  I am
working on cbb(4), now, where I will extend at least bus_space_map() and
bus_space_unmap() in order to open and close the address windows on the
CardBus bridge.  I have written the attachment, pccbb.extensions, to
give you the flavor of what the extensions could look like.

>  * Are you proposing to have multiple (both MI and MD)
>    "creating tag" functions?
> 
>    You wrote first
> 
> http://mail-index.NetBSD.org/tech-kern/2010/05/27/msg008243.html
> >> > (See atari/dev/if_ne_mb.c for awful examples)
> >> You could use bus_space_tag_create() in MD code such as if_ne_mb.c and
> >> dev/sbus/stp4020.c, but I don't know if it will be worth it.  The code
> >> would look more consistent with MI code.
> 
>    but you also wrote later
> 
> http://mail-index.netbsd.org/tech-kern/2010/05/27/msg008256.html
> >> It sounds like you may be concerned that I intend bus_space_tag_create()
> >> to replace comparable MD routines such as bus_space_tag_alloc() on
> >> sparc.  I do not.
> 
>    Then I'm really confused.

MD code creates the original tags, always.  MI code cannot create a
tag out of nothing using bus_space_tag_create().  To put it another
way, every tag created and extended by bus_space_tag_create() descends
either from an MD tag or an extension of an MD tag.  Remember,
bus_space_tag_create() is an MI API with an MD implementation.  Extended
tags themselves are MD.  Tags remain opaque to MI code.

>    Martin asked if your new API can replace MD code inside #ifdefs
>    in stp4020.c to initialize MD bus_space_tag_t internals.

Ah, ok: it's clear in retrospect that Martin meant in particular the
code that fiddles with the ASI.  No, there's no way that code written
to the new MI API will handle that.  The best that you could do with
bus_space_tag_create() is to install extensions that use (costly)
bswap() calls.

>    Eduardo concerned that touching MD bus_space_tag_t in MI code
>    (to "create" tags) could cause serious messes.

Perhaps I should underscore the fact that an MI extension does not get
access to MD details of a bus_space_tag_t.

>  * If your new bus_space_tag_create() won't replace MD routines,
>    how can it co-exist MD routines many ports already have?
>    bus_space_tag_t prepared in MD routines will also be passed
>    to whole MI drivers.

It will co-exist with MD routines.
 
>  * If MD routines will co-exist, which ones should we use in MD drivers?

Use the one that helps share code or reduce the lines of code. :-)

>  * If MD routines create MD bus_space_tag_t, why do you have to
>    allocate memory by kmem(9) for bus_space_tag_t again?

That's just the way I chose to implement it on x86.  We cannot
overwrite any tag on x86, because it may be used by some other
entity.  For example, every device in this tree shares just two tags,
x86_bus_space_io and x86_bus_space_mem:

                pci
                 |
         +-------+-------+
         |               |
        ppb             ppb
         |               |
        pci             pci
         |               |
        cbb             wm
         |
        ath

cbb cannot extend x86_bus_space_mem without copying it, first, or else
every driver in the system uses cbb's extensions, which is not desired.
Only cbb's children should use cbb's extensions.

> >> +bus_space_tag_create(bus_space_tag_t obst, const uint64_t present,
> >> +    const struct bus_space_overrides *ov, void *ctx, bus_space_tag_t 
> >> *bstp)
>  :
> >> +  bst = kmem_alloc(sizeof(struct bus_space_tag), KM_SLEEP);
> >> +
> >> +          if (pc == NULL)
> >> +          return ENOMEM;
> >> +
> >> +  bst->bst_super = obst;
> 
>    Isn't it enough to fill necessary members in passed bus_space_tag_t
>    in your bus_space_tag_create() because your patch also changes
>    MD bus_space_tag_t internals and MD routines should allocate
>    bus_space_tag_t before it passes bus_space_tag_t to children?

It's up to the implementor whether he uses kmem_alloc(9) or not.

>    In this scope, the name of "create tag" is not acceptable
>    as I wrote in the first message:
>    http://mail-index.NetBSD.org/tech-kern/2010/05/27/msg008240.html

What better name do you have in mind?

>  * Do you seriously consider whether you can also apply your API
>    changes to sparc and all other ports that have more complicated
>    bus_space_tag_t internal structures?

I have considered it.  I want for this to work on macppc and on sparc,
too, and I don't see a problem.

Dave

-- 
David Young             OJC Technologies
dyoung%ojctech.com@localhost      Urbana, IL * (217) 278-3933

static int pccbb_bus_space_map(void *, bus_space_tag_t, bus_addr_t,
    bus_size_t, int, bus_space_handle_t *);
static void pccbb_bus_space_unmap(void *, bus_space_tag_t, bus_space_handle_t,
    bus_size_t);

static const struct bus_space_overrides pccbb_bus_space_overrides = {
          .ov_space_map = pccbb_bus_space_map
        , .ov_space_unmap = pccbb_bus_space_unmap
};

static int
pccbb_bus_space_map(void *ctx, bus_space_tag_t bst, bus_addr_t address,
    bus_size_t size, int flags, bus_space_handle_t *bshp)
{
        struct pccbb_softc *sc = ctx;
        bus_space_tag_t pbst;
        int rc;

        if (bus_space_is_equal(sc->sc_memt, bst)) {
                pbst = sc->sc_memt;
        } else {
                KASSERT(bus_space_is_equal(sc->sc_iot, bst));
                pbst = sc->sc_iot;
        }

        if ((rc = bus_space_map(pbst, address, size, flags, bshp)) != 0)
                return rc;

        /* the space is mapped into kernel address space, and
         * upstream buses' windows are open; now
         * open our address window so that accesses don't fail
         * with, e.g., PCI Master Abort.
         */
        if ((rc = pccbb_open_win(sc, pbst, addr, size, *bshp, flags)) != 0)
                bus_space_unmap(pbst, *bshp, size);
        return rc;
}

static void
pccbb_bus_space_unmap(void *ctx, bus_space_tag_t bst, bus_space_handle_t bsh,
    bus_size_t size)
{
        struct pccbb_softc *sc = ctx;
        bus_space_tag_t pbst;
        int rc;

        if (bus_space_is_equal(sc->sc_memt, bst)) {
                pbst = sc->sc_memt;
        } else {
                KASSERT(bus_space_is_equal(sc->sc_iot, bst));
                pbst = sc->sc_iot;
        }

        rc = pccbb_close_win(sc, pbst, bsh, size);

        KASSERT(rc == 0);

        bus_space_unmap(pbst, bsh, size);
}

void
pccbbattach(device_t parent, device_t self, void *aux)
{
        struct pccbb_softc *sc = device_private(self);
        struct pci_attach_args *pa = aux;
        int rc;

        sc->sc_memt = pa->pa_memt;
        sc->sc_iot = pa->pa_iot;

        rc = bus_space_tag_create(sc->sc_memt,
            BUS_SPACE_OVERRIDE_MAP|BUS_SPACE_OVERRIDE_UNMAP,
            &pccbb_bus_space_overrides, sc, &sc->sc_child_memt);

        if (rc != 0) {
                aprint_error_dev(self, "couldn't create MMIO tag"); 
                return;
        }

        rc = bus_space_tag_create(sc->sc_iot,
            BUS_SPACE_OVERRIDE_MAP|BUS_SPACE_OVERRIDE_UNMAP,
            &pccbb_bus_space_overrides, sc, &sc->sc_child_iot);

        if (rc != 0) {
                bus_space_tag_destroy(sc->sc_child_memt);
                aprint_error_dev(self, "couldn't create I/O tag"); 
                return;
        }

        /* ... */
}