aspeed/smc: Cleanups and QOMification

[PATCH 05/10] aspeed/smc: Remove the 'flash' attribute from AspeedSMCFlash

Posted by Cédric Le Goater 4 years, 5 months ago

There is no use for it.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ssi/aspeed_smc.h |  1 -
 hw/arm/aspeed.c             | 11 +++++------
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
index 0ea536a44c3a..f32f66f9a838 100644
--- a/include/hw/ssi/aspeed_smc.h
+++ b/include/hw/ssi/aspeed_smc.h
@@ -37,7 +37,6 @@ typedef struct AspeedSMCFlash {
     uint32_t size;
 
     MemoryRegion mmio;
-    DeviceState *flash;
 } AspeedSMCFlash;
 
 #define TYPE_ASPEED_SMC "aspeed.smc"
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 95ce4b1670ac..64c3a7fb66db 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -274,18 +274,17 @@ static void aspeed_board_init_flashes(AspeedSMCState *s,
     int i ;
 
     for (i = 0; i < s->num_cs; ++i) {
-        AspeedSMCFlash *fl = &s->flashes[i];
         DriveInfo *dinfo = drive_get_next(IF_MTD);
         qemu_irq cs_line;
+        DeviceState *dev;
 
-        fl->flash = qdev_new(flashtype);
+        dev = qdev_new(flashtype);
         if (dinfo) {
-            qdev_prop_set_drive(fl->flash, "drive",
-                                blk_by_legacy_dinfo(dinfo));
+            qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo));
         }
-        qdev_realize_and_unref(fl->flash, BUS(s->spi), &error_fatal);
+        qdev_realize_and_unref(dev, BUS(s->spi), &error_fatal);
 
-        cs_line = qdev_get_gpio_in_named(fl->flash, SSI_GPIO_CS, 0);
+        cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
         sysbus_connect_irq(SYS_BUS_DEVICE(s), i + 1, cs_line);
     }
 }
-- 
2.31.1

Re: [PATCH 05/10] aspeed/smc: Remove the 'flash' attribute from AspeedSMCFlash

Posted by Philippe Mathieu-Daudé 4 years, 5 months ago

On 9/7/21 8:58 AM, Cédric Le Goater wrote:
> There is no use for it.

Hmmm this is not the correct justification.

This devices sits on a bus, so its state will be released when
the bus is released. There is no need to release it manually,
so we can remove the reference.

> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/ssi/aspeed_smc.h |  1 -
>  hw/arm/aspeed.c             | 11 +++++------
>  2 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
> index 0ea536a44c3a..f32f66f9a838 100644
> --- a/include/hw/ssi/aspeed_smc.h
> +++ b/include/hw/ssi/aspeed_smc.h
> @@ -37,7 +37,6 @@ typedef struct AspeedSMCFlash {
>      uint32_t size;
>  
>      MemoryRegion mmio;
> -    DeviceState *flash;
>  } AspeedSMCFlash;
>  
>  #define TYPE_ASPEED_SMC "aspeed.smc"
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 95ce4b1670ac..64c3a7fb66db 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -274,18 +274,17 @@ static void aspeed_board_init_flashes(AspeedSMCState *s,
>      int i ;
>  
>      for (i = 0; i < s->num_cs; ++i) {
> -        AspeedSMCFlash *fl = &s->flashes[i];
>          DriveInfo *dinfo = drive_get_next(IF_MTD);
>          qemu_irq cs_line;
> +        DeviceState *dev;
>  
> -        fl->flash = qdev_new(flashtype);
> +        dev = qdev_new(flashtype);
>          if (dinfo) {
> -            qdev_prop_set_drive(fl->flash, "drive",
> -                                blk_by_legacy_dinfo(dinfo));
> +            qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo));
>          }
> -        qdev_realize_and_unref(fl->flash, BUS(s->spi), &error_fatal);
> +        qdev_realize_and_unref(dev, BUS(s->spi), &error_fatal);
>  
> -        cs_line = qdev_get_gpio_in_named(fl->flash, SSI_GPIO_CS, 0);
> +        cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
>          sysbus_connect_irq(SYS_BUS_DEVICE(s), i + 1, cs_line);
>      }
>  }
>

Re: [PATCH 05/10] aspeed/smc: Remove the 'flash' attribute from AspeedSMCFlash

Posted by Cédric Le Goater 4 years, 5 months ago

On 9/7/21 10:36 AM, Philippe Mathieu-Daudé wrote:
> On 9/7/21 8:58 AM, Cédric Le Goater wrote:
>> There is no use for it.
> 
> Hmmm this is not the correct justification.
> 
> This devices sits on a bus, so its state will be released when
> the bus is released. There is no need to release it manually,
> so we can remove the reference.

That's what the code is doing AFAIUI.

This is just removing the AspeedSMCFlash attribute because it is 
not used anywhere else than under aspeed_board_init_flashes().
 
Is there anything else ? I am bit lost by your comment.

Thanks,

C.

> 
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  include/hw/ssi/aspeed_smc.h |  1 -
>>  hw/arm/aspeed.c             | 11 +++++------
>>  2 files changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
>> index 0ea536a44c3a..f32f66f9a838 100644
>> --- a/include/hw/ssi/aspeed_smc.h
>> +++ b/include/hw/ssi/aspeed_smc.h
>> @@ -37,7 +37,6 @@ typedef struct AspeedSMCFlash {
>>      uint32_t size;
>>  
>>      MemoryRegion mmio;
>> -    DeviceState *flash;
>>  } AspeedSMCFlash;
>>  
>>  #define TYPE_ASPEED_SMC "aspeed.smc"
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index 95ce4b1670ac..64c3a7fb66db 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -274,18 +274,17 @@ static void aspeed_board_init_flashes(AspeedSMCState *s,
>>      int i ;
>>  
>>      for (i = 0; i < s->num_cs; ++i) {
>> -        AspeedSMCFlash *fl = &s->flashes[i];
>>          DriveInfo *dinfo = drive_get_next(IF_MTD);
>>          qemu_irq cs_line;
>> +        DeviceState *dev;
>>  
>> -        fl->flash = qdev_new(flashtype);
>> +        dev = qdev_new(flashtype);
>>          if (dinfo) {
>> -            qdev_prop_set_drive(fl->flash, "drive",
>> -                                blk_by_legacy_dinfo(dinfo));
>> +            qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo));
>>          }
>> -        qdev_realize_and_unref(fl->flash, BUS(s->spi), &error_fatal);
>> +        qdev_realize_and_unref(dev, BUS(s->spi), &error_fatal);
>>  
>> -        cs_line = qdev_get_gpio_in_named(fl->flash, SSI_GPIO_CS, 0);
>> +        cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
>>          sysbus_connect_irq(SYS_BUS_DEVICE(s), i + 1, cs_line);
>>      }
>>  }
>>
>

Re: [PATCH 05/10] aspeed/smc: Remove the 'flash' attribute from AspeedSMCFlash

Posted by Philippe Mathieu-Daudé 4 years, 5 months ago

On 9/7/21 11:40 AM, Cédric Le Goater wrote:
> On 9/7/21 10:36 AM, Philippe Mathieu-Daudé wrote:
>> On 9/7/21 8:58 AM, Cédric Le Goater wrote:
>>> There is no use for it.
>>
>> Hmmm this is not the correct justification.
>>
>> This devices sits on a bus, so its state will be released when
>> the bus is released. There is no need to release it manually,
>> so we can remove the reference.
> 
> That's what the code is doing AFAIUI.
> 
> This is just removing the AspeedSMCFlash attribute because it is 
> not used anywhere else than under aspeed_board_init_flashes().
>  
> Is there anything else ? I am bit lost by your comment.

I was thinking of d4e1d8f57eb ("hw/arm/tosa: Encapsulate misc
GPIO handling in a device"), if the device were not created on
a bus, the we'd need to keep this reference, otherwise we'd
leak it.

Anyhow this is board code where we are not releasing anything.

Maybe "There is no need to keep a reference of the flash qdev in the
AspeedSMCFlash state: the SPI bus takes ownership and will release
its resources. Remove AspeedSMCFlash::flash."?

Anyway no big deal with the comment,
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>