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]>
