Date: Sun, 6 Oct 2013 17:19:36 -0600 From: Warner Losh <imp@bsdimp.com> To: Adrian Chadd <adrian.chadd@gmail.com> Cc: Alexander Motin <mav@freebsd.org>, Ilya Bakulin <ilya@bakulin.de>, "freebsd-embedded@freebsd.org" <freebsd-embedded@freebsd.org> Subject: Re: SDIO for FreeBSD Message-ID: <4FB11076-DD88-43F2-A449-E41D2088A4DD@bsdimp.com> In-Reply-To: <CAJ-Vmon0xr597We=sF5Uhja2qb=viuiqCbQXZgUvp7VX-FsPfA@mail.gmail.com> References: <5251A9D3.1080803@bakulin.de> <5251C911.6020003@FreeBSD.org> <CAJ-Vmon0xr597We=sF5Uhja2qb=viuiqCbQXZgUvp7VX-FsPfA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
I've often thought that having an SDIO protocol for CAM would be where =
we'd wind up because once you start adding in GPIO pin interrupt =
signalling, message "interrupt" signaling, message completion, etc, it =
looks a lot like many of the mechanisms we have in CAM... I'm not at =
all sure that USB is the right model to use, frankly... But it does =
have some superficial similarities...
Warner
On Oct 6, 2013, at 3:05 PM, Adrian Chadd wrote:
> Hi,
>=20
> (Sorry, top-posting from a mobile device. Sorry.)
>=20
> Yes, i agree with mav. If we need so many threads, I think it's a good =
sign that the design needs some more thought.
>=20
> What's USB do?
>=20
> I think here we can get away with doing it via taskqueues. Ie, we post =
interrupts in the same context(s) that we post SDIO command completions.
>=20
> I'd really prefer this to be slightly more thought out. I've no idea =
how you're currently implementing it and as I'm knee deep in the =
AR9344/MIPS74K stuff, I'd like to focus on that until it's up to a point =
that others can jump in and help with the testing and bringup. But I =
think modelling it after USB would be good - have driver methods that =
get called to drive the command transmit/receive and then have the =
interrupt handling be called from the same taskqueue. That way your =
interrupt code would just be something like:
>=20
> driver_intr()
> {
> set_flag_that_states_an_interrupt_occured();
> submit_some_register_read_command_to_the_transmit_queue();
> sdio_start_xfer();
> }
>=20
> .. then sdio_start_xfer() would tell your stack to start a transfer if =
the queue wasn't already running. It would then call the driver method =
to queue another command(s) to the SDIO stack. The SDIO stack can then =
post them to the hardware when it has time.
>=20
> Thanks,
>=20
>=20
>=20
> -adrian
>=20
> On Sunday, October 6, 2013, Alexander Motin wrote:
> On 06.10.2013 21:20, Ilya Bakulin wrote:
> Just a quick status update.
> I have managed to make interrupts work. Was rather silly copy&paste =
mistake.
>=20
> So now I'm able to issue a command to the WiFi card and actually get =
an
> interrupt from
> the controller.
>=20
> I have added several bus methods for setting up interrupt handlers and
> allocating resources,
> both to the MMC stack and to my WiFi driver, as well as mv_sdio SDIO
> controller driver for DreamPlug.
> The code is kept under [1], as usual.
>=20
> I have the fully working stack now and I'd like to discuss the locking
> model for SDIO.
>=20
> So, the SDIO controller detects an interrupt.
> SDIO controller driver handles it its own interrupt thread (I assume
> that we use ITHREAD ISRs).
> All the controller can do at this stage is:
> 1. read device registers to get the interrupt cause;
> 2. clear/set some bits in device registers;
> 3. If there was a command- or data-related interrupt, call req->done()
> method of struct mmc_request,
> which sets MMC_REQ_DONE flag in the request structure and then =
calls
> wakeup() on mmc_request.
> This is what actually happens now. MMC stack which is doing =
msleep()
> waiting for MMC_REQ_DONE
> flag detects this flag and continues with request processing, =
returning
> a result to the upper layer.
> See mmc_wakeup() and mmc_wait_for_req() in sys/dev/mmc/mmc.c
>=20
> Everything is syncronous at this point, no complications.
>=20
> Now the SDIO case. The controller can detect an interrupt at any time,
> then it notifies the MMC stack
> about it. This happens in the ITHREAD of SDIO controller's ISR.
> In MMC stack, we now want to read the "Penging interrupts" register to
> figure out which function has
> raised the interrupt. We cannot, however, issue another request to =
SDIO
> controller since that inhibits
> sleeping and we cannot sleep in ISR. We will also need to call ISRs of
> our child devices that can possibly
> do another weird stuff.
>=20
> So I propose adding a dedicated thread in MMC layer that will just
> process the interrupts from the card.
> It can be started on MMC stack attach, grab MMC lock and then just =
sleep
> on condition variable
> (which will be per-bus, ie for each SDIO card since there can only be
> one SD[IO] card on the bus), giving the lock back.
> When the MMC stack's ISR is called by SDIO controller, it will just
> cv_signal() that variable and do nothing more than that.
> The MMC interrupt servicing thread will then wakeup (and grab the MMC
> lock!),
> it will be able to do another MMC request to read "pending interrupts"
> register
> and call the ISRs of respective child drivers.
> We expect them to be quick and probably do the similar stuff -- just
> signaling CVs
> and/or taskqueue_enqueue()-ing interrupt processing tasks.
>=20
> I am sorry, can't hold from rephrasing the joke: how many threads does =
it take to handle an interrupt? ;)
>=20
> IMHO 3 threads used to handle single hardware interrupt is a good =
point to invent something better. That would be easy if controller and =
MMC layers used the same lock and MMC could send interrupt status =
request asynchronously just from the ISR, but they don't. May be things =
could be much more simple if if would be controller driver obligation to =
read interrupt status from the card instead of jumping up and down over =
the stack and having to use additional threads to decouple the locks? =
That is like in CAM for both ATA and SCSI now it is controller driver =
duty to fetch errors details from the device, even sending additional =
commands if needed.
>=20
> Sorry if I totally misunderstand the concept here, please tell your
> opinions.
>=20
> [1] https://github.com/kibab/freebsd/compare/master...kibab-dplug
> Note that I have made a rebase and pushed the rebased branch back to =
GitHub.
> This is an epic fuckup and it requires everyone to delete the local
> branch and pull it once again,
> as the IDs have changed. Sorry for that, I will do merges next time.
>=20
>=20
> --=20
> Alexander Motin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4FB11076-DD88-43F2-A449-E41D2088A4DD>