Re: [Qemu-devel] [PATCH] hw/arm/pxa2xx_gpio: Correct and register vmstate

[Peter Crosthwaite]

On Wed, Jun 4, 2014 at 4:58 AM, Peter Maydell <address@hidden> wrote:
> On 3 June 2014 19:30, Peter Maydell <address@hidden> wrote:
>> The pxa2xx-gpio device has a VMStateDescription, but it was accidentally
>> never actually registered, and it wasn't quite correct. Remove the
>> 'lines' field (this is a device property, not mutable state), add
>> the missing 'gpsr' and 'prev_level' fields, and set dc->vmsd so it
>> actually gets used.
>>
>> Signed-off-by: Peter Maydell <address@hidden>
>> ---
>>  hw/arm/pxa2xx_gpio.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/pxa2xx_gpio.c b/hw/arm/pxa2xx_gpio.c
>> index 7f75f05..ccf6e44 100644
>> --- a/hw/arm/pxa2xx_gpio.c
>> +++ b/hw/arm/pxa2xx_gpio.c
>> @@ -314,14 +314,15 @@ static const VMStateDescription 
>> vmstate_pxa2xx_gpio_regs = {
>>      .version_id = 1,
>>      .minimum_version_id = 1,
>>      .fields = (VMStateField[]) {
>> -        VMSTATE_INT32(lines, PXA2xxGPIOInfo),
>>          VMSTATE_UINT32_ARRAY(ilevel, PXA2xxGPIOInfo, PXA2XX_GPIO_BANKS),
>>          VMSTATE_UINT32_ARRAY(olevel, PXA2xxGPIOInfo, PXA2XX_GPIO_BANKS),
>>          VMSTATE_UINT32_ARRAY(dir, PXA2xxGPIOInfo, PXA2XX_GPIO_BANKS),
>>          VMSTATE_UINT32_ARRAY(rising, PXA2xxGPIOInfo, PXA2XX_GPIO_BANKS),
>>          VMSTATE_UINT32_ARRAY(falling, PXA2xxGPIOInfo, PXA2XX_GPIO_BANKS),
>>          VMSTATE_UINT32_ARRAY(status, PXA2xxGPIOInfo, PXA2XX_GPIO_BANKS),
>> +        VMSTATE_UINT32_ARRAY(gpsr, PXA2xxGPIOInfo, PXA2XX_GPIO_BANKS),

According to documentation, reading gpsr is undefined, and the only
reason for this gpsr state as implemented in QEMU is return some
arbitrary default when reading the write-only register.

"
4-10
Intel® PXA255 Processor
Developer’s Manual
System Integration Unit
When a GPIO is configured as an output, the state of the pin can be
controlled by writing to either
the GPSR or GPCR. An output pin is set high by writing a one to its
corresponding bit within the
GPSR. To clear an output pin, a one is written to the corresponding
bit within the GPCR. GPSR
and GPCR are write-only registers. Reads return unpredictable values
"

Commit message of 2b76bdc965ba7b4f27133cb345101d9535ddaa79 seems to
suggest the problem was GPSR defaulting to hw_error or some such. No
mention of read-as-written.

e1dad5a615fb4a2d5cd43cbc0fc42f6a0d35f2e9 Documents the same problem
for its sister register GPCR but with different stateless
implementation (returns 31337).

All, in all, I think GPSR is not legitimate device state at all and
probably should be removed:

--- a/hw/arm/pxa2xx_gpio.c
+++ b/hw/arm/pxa2xx_gpio.c
@@ -36,7 +36,6 @@ struct PXA2xxGPIOInfo {
     uint32_t rising[PXA2XX_GPIO_BANKS];
     uint32_t falling[PXA2XX_GPIO_BANKS];
     uint32_t status[PXA2XX_GPIO_BANKS];
-    uint32_t gpsr[PXA2XX_GPIO_BANKS];
     uint32_t gafr[PXA2XX_GPIO_BANKS * 2];

     uint32_t prev_level[PXA2XX_GPIO_BANKS];
@@ -162,10 +161,6 @@ static uint64_t pxa2xx_gpio_read(void *opaque,
hwaddr offset,
         return s->dir[bank];

     case GPSR:         /* GPIO Pin-Output Set registers */
-        printf("%s: Read from a write-only register " REG_FMT "\n",
-                        __FUNCTION__, offset);
-        return s->gpsr[bank];  /* Return last written value.  */
-
     case GPCR:         /* GPIO Pin-Output Clear registers */
         printf("%s: Read from a write-only register " REG_FMT "\n",
                         __FUNCTION__, offset);
@@ -217,7 +212,6 @@ static void pxa2xx_gpio_write(void *opaque, hwaddr offset,
     case GPSR:         /* GPIO Pin-Output Set registers */
         s->olevel[bank] |= value;
         pxa2xx_gpio_handler_update(s);
-        s->gpsr[bank] = value;
         break;

     case GPCR:         /* GPIO Pin-Output Clear registers */

Regards,
Peter

>>          VMSTATE_UINT32_ARRAY(gafr, PXA2xxGPIOInfo, PXA2XX_GPIO_BANKS * 2),
>> +        VMSTATE_UINT32_ARRAY(prev_level, PXA2xxGPIOInfo, PXA2XX_GPIO_BANKS),
>>          VMSTATE_END_OF_LIST(),
>>      },
>>  };
>> @@ -340,6 +341,7 @@ static void pxa2xx_gpio_class_init(ObjectClass *klass, 
>> void *data)
>>      k->init = pxa2xx_gpio_initfn;
>>      dc->desc = "PXA2xx GPIO controller";
>>      dc->props = pxa2xx_gpio_properties;
>> +    dc->vmsd = vmstate_pxa2xx_gpio_regs;
>
> Doh, missing '&', or we don't compile. I'm not going
> to bother resending just for that :-)
>
> thanks
> -- PMM
>