[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v7 07/12] dma: Add Xilinx Zynq devcfg device mod
From: |
Alistair Francis |
Subject: |
Re: [Qemu-devel] [PATCH v7 07/12] dma: Add Xilinx Zynq devcfg device model |
Date: |
Thu, 23 Jun 2016 11:08:45 -0700 |
On Thu, Jun 23, 2016 at 6:08 AM, Peter Maydell <address@hidden> wrote:
> On 22 June 2016 at 21:24, Alistair Francis <address@hidden> wrote:
>> Add a minimal model for the devcfg device which is part of Zynq.
>> This model supports DMA capabilities and interrupt generation.
>>
>> Signed-off-by: Peter Crosthwaite <address@hidden>
>> Signed-off-by: Alistair Francis <address@hidden>
>
>> + if (!dmah->src_len && !dmah->dest_len) {
>> + DB_PRINT("dma operation finished\n");
>> + s->regs[R_INT_STS] |= R_INT_STS_DMA_DONE_MASK |
>> + R_INT_STS_DMA_P_DONE_MASK;
>> + s->dma_cmd_fifo_num--;
>> + memmove(s->dma_cmd_fifo, &s->dma_cmd_fifo[1],
>> + sizeof(s->dma_cmd_fifo));
>
> This looks like an off-by-one error in the size argument.
Fixed.
>
>> + }
>> + xlnx_zynq_devcfg_update_ixr(s);
>> + } while (s->dma_cmd_fifo_num);
>> +}
>
>> +static void r_unlock_post_write(RegisterInfo *reg, uint64_t val)
>> +{
>> + XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
>> + const char *device_prefix = object_get_typename(OBJECT(s));
>> +
>> + if (val == R_UNLOCK_MAGIC) {
>> + DB_PRINT("successful unlock\n");
>> + /* BootROM will have already done the actual unlock so no need to do
>> + * anything in successful subsequent unlock
>> + */
>
> I don't understand this comment. Shouldn't we be marking the
> memory region as enabled here, to handle the case of
> "guest locks the device via a bad-unlock-attempt; guest
> unlocks via a good unlock attempt" ? What the guest bootrom
> does or doesn't do isn't usually relevant to the device
> implementation.
Agreed. I have added the ability to unlock.
>
>> + } else { /* bad unlock attempt */
>> + qemu_log_mask(LOG_GUEST_ERROR, "%s: failed unlock\n",
>> device_prefix);
>> + s->regs[R_CTRL] &= ~R_CTRL_PCAP_PR_MASK;
>> + s->regs[R_CTRL] &= ~R_CTRL_PCFG_AES_EN_MASK;
>> + /* core becomes inaccessible */
>> + memory_region_set_enabled(&s->iomem, false);
>> + }
>> +}
>
>> +
>> +static const RegisterAccessInfo xlnx_zynq_devcfg_regs_info[] = {
>> + { .name = "CTRL", .addr = A_CTRL,
>> + .reset = R_CTRL_PCAP_PR_MASK | R_CTRL_PCAP_MODE_MASK | 0x3 << 13,
>> + .rsvd = 0x1 << 28 | 0x3ff << 13 | 0x3 << 13,
>> + .pre_write = r_ctrl_pre_write,
>> + .post_write = r_ctrl_post_write,
>> + },
>> + { .name = "LOCK", .addr = A_LOCK,
>> + .rsvd = MAKE_64BIT_MASK(5, 64 - 5),
>> + .pre_write = r_lock_pre_write,
>> + },
>> + { .name = "CFG", .addr = A_CFG,
>> + .reset = 1 << R_CFG_RFIFO_TH_SHIFT | 1 << R_CFG_WFIFO_TH_SHIFT |
>> 0x8,
>
> I was expecting this to use the R_CFG_RESET value defined earlier
> (or alternatively don't bother defining a #define for it...)
Fixed, it's now using the macro.
>
>
>> +static const VMStateDescription vmstate_xlnx_zynq_devcfg = {
>> + .name = "xlnx_zynq_devcfg",
>> + .version_id = 1,
>> + .minimum_version_id = 1,
>> + .minimum_version_id_old = 1,
>
> You don't need an _old field.
Removed.
>
>> + .fields = (VMStateField[]) {
>> + VMSTATE_STRUCT_ARRAY(dma_cmd_fifo, XlnxZynqDevcfg,
>> + XLNX_ZYNQ_DEVCFG_DMA_CMD_FIFO_LEN, 0,
>> + vmstate_xlnx_zynq_devcfg_dma_cmd,
>> + XlnxZynqDevcfgDMACmd),
>> + VMSTATE_UINT8(dma_cmd_fifo_num, XlnxZynqDevcfg),
>> + VMSTATE_UINT32_ARRAY(regs, XlnxZynqDevcfg, XLNX_ZYNQ_DEVCFG_R_MAX),
>> + VMSTATE_END_OF_LIST()
>> + }
>> +};
>
>
>> diff --git a/include/hw/register.h b/include/hw/register.h
>> index 104d381..3e4d1ae 100644
>> --- a/include/hw/register.h
>> +++ b/include/hw/register.h
>> @@ -208,7 +208,7 @@ MemoryRegion *register_init_block32(DeviceState *owner,
>> enum { R_ ## reg ## _ ## field ## _SHIFT = (shift)}; \
>> enum { R_ ## reg ## _ ## field ## _LENGTH = (length)}; \
>> enum { R_ ## reg ## _ ## field ## _MASK = \
>> - MAKE_64BIT_MASK(shift, length);
>> + MAKE_64BIT_MASK(shift, length)};
>>
>> /* Extract a field from a register */
>> #define FIELD_EX32(storage, reg, field) \
>
> Should this hunk be in some earlier patch ?
Yes it should, I have fixed this as well.
Thanks,
Alistair
>
> thanks
> -- PMM
>
- Re: [Qemu-devel] [PATCH v7 06/12] register: Add block initialise helper, (continued)
[Qemu-devel] [PATCH v7 04/12] register: Define REG and FIELD macros, Alistair Francis, 2016/06/22
[Qemu-devel] [PATCH v7 02/12] register: Add Register API, Alistair Francis, 2016/06/22
[Qemu-devel] [PATCH v7 09/12] irq: Add opaque setter routine, Alistair Francis, 2016/06/22
[Qemu-devel] [PATCH v7 07/12] dma: Add Xilinx Zynq devcfg device model, Alistair Francis, 2016/06/22
[Qemu-devel] [PATCH v7 08/12] xilinx_zynq: Connect devcfg to the Zynq machine model, Alistair Francis, 2016/06/22
[Qemu-devel] [PATCH v7 10/12] register: Add GPIO API, Alistair Francis, 2016/06/22
[Qemu-devel] [PATCH v7 11/12] misc: Introduce ZynqMP IOU SLCR, Alistair Francis, 2016/06/22