Re: [Qemu-devel] [PATCH 1/2] zynq_gpio: GPIO model for Zynq SoC

[Colin Leitner <colin.leitner@googlemail.com>]

Hi Peter,

thanks for the review! I'll rework the patch ASAP.

> Is it better to just model the GPIO controller as a standalone GPIO,
> and leave the mio vs emio distinction to the SoC/Board level?
> 
> This would mean the bank GPIOs are on the top level entity, and the
> core would then have no EMIO/MIO awareness. This also makes QEMU a
> little less awkward considering there is no sense of MIO and EMIO in
> QEMU to date.

The reason for chosing the MIO/EMIO names was simply for easier mapping
to real hardware where I've been usually confronted with MIO/EMIO.
Changing this to the banks makes sense of course if Xilinx choses to
reuse that IP again.

>> +    /* Outputs float high.  */
>> +    /* FIXME: This is board dependent.  */
> 
> How so? Looks pretty generic to me (not sure what needs fixing here).
> Are you saying that the IO width should truncate based on Zynq
> specifics?

This is a fragment from pl061. If we don't explicitly drive a output
line through the direction register, we assume it floats high. We
still have to drive the qemu IRQ line to some state.

I'll write a better comment.

>> +    zynq_gpio_reset(s);
> 
> Don't reset in init fns. You shuold use a device-reset function ...

Another 1:1 copy from pl061. I'll take the time to read up how the
device model is meant to be implemented correctly.

Thanks again for the review and you'll hear from me shortly.

Regards,
	Colin