Il 29/05/2013 15:54, Peter Crosthwaite ha scritto:
> Hi Paolo,
>
> On Wed, May 29, 2013 at 6:51 PM, Paolo Bonzini <pbonz...@redhat.com> wrote:
>> Il 24/05/2013 07:49, peter.crosthwa...@xilinx.com ha scritto:
>>> +static const MemoryRegionOps devcfg_reg_ops = {
>>> + .read = register_read_memory_le,
>>> + .write = register_write_memory_le,
>>> + .endianness = DEVICE_LITTLE_ENDIAN,
>>> + .valid = {
>>> + .min_access_size = 4,
>>> + .max_access_size = 4,
>>
>> What happens if you have registers of mixed size within the same "bank"?
>>
>
> You Probably should change these access restrictions per-register
> accordingly.
Can you add a generic .valid.accepts callback for the register API that
will check against the size of the register at that offset?
> The degenerate case is allowing greater-than-register size or
> misaligned accesses, which is dependent on the behaviour of the memory
> API. I can do some research, but do you know if the memory API
> supports misaligned transactions that span a memory region boundary?
I don't really care about that for now.
But the way to do that would be to add a .impl.accepts method (right now
there's only .valid.accepts). Then you would point the register API's
implementation to .impl.accepts, and use
.impl.accepts = register_accepts,
.valid = {
.min_access_size = 1,
.max_access_size = 4,
}
The memory API will do a RMW operation if needed. Won't make much sense
with write-1-clear bits and the like, but for simple registers it would
work.
Paolo
> If so then there are no concerns, as the memory API will handle it for
> us. If not then I don't see it as an issue as its very rare (at least
> in my experience) for registers to support such strange misaligned or
> mulit-register accesses. That or we can look into memory API for
> answers.
>
>>> + }
>>> +};
>>> +
>>> +static void xilinx_devcfg_realize(DeviceState *dev, Error **errp)
>>> +{
>>> + XilinxDevcfg *s = XILINX_DEVCFG(dev);
>>> + const char *prefix = object_get_canonical_path(OBJECT(dev));
>>> + int i;
>>> +
>>> + for (i = 0; i < R_MAX; ++i) {
>>> + RegisterInfo *r = &s->regs_info[i];
>>> +
>>> + *r = (RegisterInfo) {
>>> + .data = &s->regs[i],
>>> + .data_size = sizeof(uint32_t),
>>> + .access = &xilinx_devcfg_regs_info[i],
>>> + .debug = XILINX_DEVCFG_ERR_DEBUG,
>>> + .prefix = prefix,
>>> + .opaque = s,
>>> + };
>>> + memory_region_init_io(&r->mem, &devcfg_reg_ops, r, "devcfg-regs",
>>> 4);
>>
>> Could you add a register_init function that does register_reset +
>> memory_region_init_io?
>>
>
> I wanted to factor this loop out into a helper as a next step so I
> think its already in my todo list, but i'm confused as to why reset
> and memory_region_init would bundled together - arent they normally in
> Device::reset and Device::realize (or Object::Init) respectively?
>
> Regards,
> Peter
>
>>> + memory_region_add_subregion(&s->iomem, i * 4, &r->mem);
>>
>> Paolo
>>
