On 29 February 2016 at 23:11, Andrew Baumann
<andrew.baum...@microsoft.com> wrote:
> Signed-off-by: Andrew Baumann <andrew.baum...@microsoft.com>
> ---
> This patch applies on top of the previous series for Windows and
> framebuffer support:
> https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg06387.html
>
> After preparing that, I was disappointed to discover that Raspbian
> won't boot cleanly without the DMA controller. In the hope of beating
> the freeze deadline (it's still February 29 here :-) I'm sending this
> for review.
>
> After applying this patch, it is possible to boot Raspbian to the GUI
> using a command such as:
>
> qemu-system-arm -M raspi2 -kernel raspbian-boot/kernel7.img -sd
> 2015-09-24-raspbian-jessie.img -append "rw earlyprintk loglevel=8
> console=ttyAMA0 dwc_otg.lpm_enable=0 root=/dev/mmcblk0p2 rootwait"
> -dtb raspbian-boot/bcm2709-rpi-2-b.dtb -serial stdio
>
> As before, this derives from the original (out of tree) work of
> Gregory Estrade, Stefan Weil and others to support Raspberry Pi
> 1. This patch in particulary is Gregory's code, which I have cleaned
> up for submission.
Should it have a Signed-off-by: line and/or authorship line
from Gregory, then?
(This is largely about giving credit where due, so it's Gregory's
choice really. I forget whether we've had this discussion before
but I couldn't find anything in a quick sweep through earlier mail
threads.)
> +static void bcm2835_dma_update(BCM2835DMAState *s, unsigned c)
> +{
> + BCM2835DMAChan *ch = &s->chan[c];
> + uint32_t data, xlen, ylen;
> + int16_t dst_stride, src_stride;
> +
> + if (!(s->enable & (1 << c))) {
> + return;
> + }
> +
> + while ((s->enable & (1 << c)) && (ch->conblk_ad != 0)) {
> + /* CB fetch */
> + ch->ti = ldl_phys(&s->dma_as, ch->conblk_ad);
> + ch->source_ad = ldl_phys(&s->dma_as, ch->conblk_ad + 4);
> + ch->dest_ad = ldl_phys(&s->dma_as, ch->conblk_ad + 8);
> + ch->txfr_len = ldl_phys(&s->dma_as, ch->conblk_ad + 12);
> + ch->stride = ldl_phys(&s->dma_as, ch->conblk_ad + 16);
> + ch->nextconbk = ldl_phys(&s->dma_as, ch->conblk_ad + 20);
These should use the ldl_{le,be}_phys functions. (If you care
about modelling the response to "unreadable address" you can
use address_space_ldl_{le,be}.)
> +
> + if (ch->ti & BCM2708_DMA_TDMODE) {
> + /* 2D transfer mode */
> + ylen = (ch->txfr_len >> 16) & 0x3fff;
> + xlen = ch->txfr_len & 0xffff;
> + dst_stride = ch->stride >> 16;
> + src_stride = ch->stride & 0xffff;
> + } else {
> + ylen = 1;
> + xlen = ch->txfr_len;
> + dst_stride = 0;
> + src_stride = 0;
> + }
> +
> + while (ylen != 0) {
> + /* Normal transfer mode */
> + while (xlen != 0) {
> + if (ch->ti & BCM2708_DMA_S_IGNORE) {
> + /* Ignore reads */
> + data = 0;
> + } else {
> + data = ldl_phys(&s->dma_as, ch->source_ad);
> + }
> + if (ch->ti & BCM2708_DMA_S_INC) {
> + ch->source_ad += 4;
> + }
> +
> + if (ch->ti & BCM2708_DMA_D_IGNORE) {
> + /* Ignore writes */
> + } else {
> + stl_phys(&s->dma_as, ch->dest_ad, data);
> + }
> + if (ch->ti & BCM2708_DMA_D_INC) {
> + ch->dest_ad += 4;
> + }
> +
> + /* update remaining transfer length */
> + xlen -= 4;
> + if (ch->ti & BCM2708_DMA_TDMODE) {
> + ch->txfr_len = (ylen << 16) | xlen;
> + } else {
> + ch->txfr_len = xlen;
> + }
> + }
> +
> + if (--ylen != 0) {
> + ch->source_ad += src_stride;
> + ch->dest_ad += dst_stride;
> + }
> + }
> + ch->cs |= BCM2708_DMA_END;
> + if (ch->ti & BCM2708_DMA_INT_EN) {
> + ch->cs |= BCM2708_DMA_INT;
> + s->int_status |= (1 << c);
> + qemu_set_irq(ch->irq, 1);
> + }
> +
> + /* Process next CB */
> + ch->conblk_ad = ch->nextconbk;
> + }
This loop allows a guest to make QEMU lock up (stop responding to monitor
commands, etc) if it feeds the device a circular loop of CBs. On the other
hand I don't think we have a good approach to avoiding this problem,
so never mind.
> + ch->cs &= ~BCM2708_DMA_ACTIVE;
> +}
> +
> +static uint64_t bcm2835_dma_read(BCM2835DMAState *s, hwaddr offset,
> + unsigned size, unsigned c)
> +{
> + BCM2835DMAChan *ch = &s->chan[c];
> + uint32_t res = 0;
> +
> + assert(size == 4);
> + assert(c < BCM2835_DMA_NCHANS);
It's a bit late to assert after you've already used c as
an array index... (the compiler is free to conclude that the
condition must always be true, for instance.)
> +
> + switch (offset) {
> + case BCM2708_DMA_CS:
> + res = ch->cs;
> + break;
> + case BCM2708_DMA_ADDR:
> + res = ch->conblk_ad;
> + break;
> + case BCM2708_DMA_INFO:
> + res = ch->ti;
> + break;
> + case BCM2708_DMA_SOURCE_AD:
> + res = ch->source_ad;
> + break;
> + case BCM2708_DMA_DEST_AD:
> + res = ch->dest_ad;
> + break;
> + case BCM2708_DMA_TXFR_LEN:
> + res = ch->txfr_len;
> + break;
> + case BCM2708_DMA_STRIDE:
> + res = ch->stride;
> + break;
> + case BCM2708_DMA_NEXTCB:
> + res = ch->nextconbk;
> + break;
> + case BCM2708_DMA_DEBUG:
> + res = ch->debug;
> + break;
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset %"HWADDR_PRIx"\n",
> + __func__, offset);
> + break;
> + }
> + return res;
> +}
> +
> +static void bcm2835_dma_write(BCM2835DMAState *s, hwaddr offset,
> + uint64_t value, unsigned size, unsigned c)
> +{
> + BCM2835DMAChan *ch = &s->chan[c];
> + uint32_t oldcs;
> +
> + assert(size == 4);
> + assert(c < BCM2835_DMA_NCHANS);
> +
> + switch (offset) {
> + case BCM2708_DMA_CS:
> + oldcs = ch->cs;
> + if (value & BCM2708_DMA_RESET) {
> + ch->cs |= BCM2708_DMA_RESET;
The comment about this bit earlier says it's self-clearing, but I
don't see where it gets cleared.
Otherwise looks OK.
thanks
-- PMM
