Re: [PATCH v4 6/7] ufs/aspeed: Add AST2700 UFS host controller

On Thu, 13 Aug 2026 14:30:38 -0500, Mikail Sadic <[email protected]> wrote:
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index a9238e6217..c1052c74a8 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -187,6 +187,31 @@ static void aspeed_machine_init(MachineState *machine)
>      aspeed_connect_serial_hds_to_uarts(bmc);
>      qdev_realize(DEVICE(bmc->soc), NULL, &error_abort);
>  
> +    /*
> +     * If the SoC instantiated a UFS host controller (AST2700), attach the
> +     * first backend drive to it as logical unit 0.  The controller and its
> +     * UFS bus only exist once the SoC has been realized, so the ufs-lu is
> +     * created here rather than through a controller drive property.
> +     */
> +    if (object_resolve_path_component(OBJECT(bmc->soc), "ufs")) {

This appears to implicitly attach the first if=none backend to UFS LU
0. Why is this needed?

if=none backends are normally left unattached for explicit -device
use, so this may claim a drive intended for another device.

Wouldn't it be better to remove this and require users to attach
the LU explicitly, for example:
-drive if=none,id=ufs0,file=ufs.img,format=raw \
-device ufs-lu,bus=ufs-bus.0,drive=ufs0,lun=0

>
> diff --git a/hw/ufs/aspeed_ufs.c b/hw/ufs/aspeed_ufs.c
> new file mode 100644
> index 0000000000..bfe540b929
> --- /dev/null
> +++ b/hw/ufs/aspeed_ufs.c
> @@ -0,0 +1,67 @@
> [ ... skip 22 lines ... ]
> +struct AspeedUfsState {
> +    SysbusUfsState parent_obj;
> +};
> +
> +struct AspeedUfsClass {
> +    DeviceClass parent_class;

The parent class layout is SysBusDeviceClass, so this should be
`SysBusDeviceClass parent_class`

>
> diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
> index a00238ed74..dedc784926 100644
> --- a/include/hw/arm/aspeed.h
> +++ b/include/hw/arm/aspeed.h
> @@ -62,6 +62,7 @@ struct AspeedMachineClass {
>      uint32_t uart_default;
>      bool sdhci_wp_inverted;
>      bool vbootrom;
> +    uint32_t ufs_block_size;

As noted on the previous patch, UFS does not permit 512-byte
logical blocks. Please drop this field and the corresponding 512-byte
configuration and documentation.

>
> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> index cd68c7f1ca..ae986dea12 100644
> --- a/include/hw/arm/aspeed_soc.h
> +++ b/include/hw/arm/aspeed_soc.h
> @@ -118,6 +119,7 @@ struct AspeedSoCState {
>      AspeedAPB2OPBState fsi[2];
>      AspeedLTPIState ltpi_ctrl[ASPEED_IOEXP_NUM];
>      AspeedAST1700SoCState ioexp[ASPEED_IOEXP_NUM];
> +    SysbusUfsState ufs;

Since this child is initialized as TYPE_ASPEED_UFS, shouldn't this be
AspeedUfsState rather than SysbusUfsState?

-- 
Jeuk Kim <[email protected]>