| Message ID | 20240709152556.52896-1-philmd@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 7/9/24 5:25 PM, Philippe Mathieu-Daudé wrote: > Since v42: > - Stick to spec v4.3 (re-simplified EXT_CSD register & migrate) > - Fill CID register > - Few changes to CSD register > - Implement 'boot-mode' reset timing > - Add 'boot-size' property > > Change required for aspeed branch: > -- >8 -- > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c > index 8c0e36badd..563816b710 100644 > --- a/hw/arm/aspeed.c > +++ b/hw/arm/aspeed.c > @@ -344,3 +344,3 @@ static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, bool emmc, > if (emmc) { > - qdev_prop_set_uint8(card, "boot-config", boot_emmc ? 0x48 : 0x0); > + qdev_prop_set_uint64(card, "boot-size", 1 * MiB); > } > (I'm still reluctant to merge patches 16-18)... Then, please drop all changes related to the boot partitions. I will keep the original patches in my tree and address the feature when I have time. TYPE_EMMC is already great to have. Thanks, C. > --- > > Cédric Le Goater (2): > hw/sd/sdcard: Add emmc_cmd_SET_RELATIVE_ADDR handler (CMD3) > hw/sd/sdcard: Fix SET_BLOCK_COUNT command argument on eMMC (CMD23) > > Joel Stanley (3): > hw/sd/sdcard: Support boot area in emmc image > hw/sd/sdcard: Subtract bootarea size from blk > hw/sd/sdcard: Add boot config support > > Luc Michel (1): > hw/sd/sdcard: Implement eMMC sleep state (CMD5) > > Philippe Mathieu-Daudé (11): > hw/sd/sdcard: Basis for eMMC support > hw/sd/sdcard: Register generic command handlers > hw/sd/sdcard: Register unimplemented command handlers > hw/sd/sdcard: Implement emmc_set_cid() > hw/sd/sdcard: Implement emmc_set_csd() > hw/sd/sdcard: Add mmc_cmd_PROGRAM_CID handler (CMD26) > hw/sd/sdcard: Add eMMC 'boot-size' property > hw/sd/sdcard: Simplify EXT_CSD values for spec v4.3 > hw/sd/sdcard: Migrate ExtCSD 'modes' register > hw/sd/sdcard: Implement eMMC 'boot-mode' > hw/sd/sdcard: Enable TYPE_EMMC card model > > Sai Pavan Boddu (1): > hw/sd/sdcard: Add mmc SWITCH function support (CMD6) > > Vincent Palatin (1): > hw/sd/sdcard: Add emmc_cmd_SEND_EXT_CSD handler (CMD8) > > include/hw/sd/sd.h | 4 + > hw/sd/sd.c | 424 ++++++++++++++++++++++++++++++++++++++++++++- > hw/sd/trace-events | 3 + > 3 files changed, 425 insertions(+), 6 deletions(-) >
On 9/7/24 17:58, Cédric Le Goater wrote: > On 7/9/24 5:25 PM, Philippe Mathieu-Daudé wrote: >> Since v42: >> - Stick to spec v4.3 (re-simplified EXT_CSD register & migrate) >> - Fill CID register >> - Few changes to CSD register >> - Implement 'boot-mode' reset timing >> - Add 'boot-size' property >> >> Change required for aspeed branch: >> -- >8 -- >> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c >> index 8c0e36badd..563816b710 100644 >> --- a/hw/arm/aspeed.c >> +++ b/hw/arm/aspeed.c >> @@ -344,3 +344,3 @@ static void sdhci_attach_drive(SDHCIState *sdhci, >> DriveInfo *dinfo, bool emmc, >> if (emmc) { >> - qdev_prop_set_uint8(card, "boot-config", boot_emmc ? 0x48 >> : 0x0); >> + qdev_prop_set_uint64(card, "boot-size", 1 * MiB); >> } >> (I'm still reluctant to merge patches 16-18)... > > Then, please drop all changes related to the boot partitions. I will keep > the original patches in my tree and address the feature when I have time. > TYPE_EMMC is already great to have. As you mentioned on today's community call, eMMC is a chipset soldered on a board, so our boards exactly knows what size to expect on the card, and whether boot partitions are present or not. I find the way of building the drive image described in patch #16 cumbersome, but I'm OK to make some concession on it, since "it works" as it. If necessary we'll improve and deprecate the current properties. I'll repost and plan to merge as-is (modulo review comments). Regards, Phil.
On 7/9/24 11:39 PM, Philippe Mathieu-Daudé wrote: > On 9/7/24 17:58, Cédric Le Goater wrote: >> On 7/9/24 5:25 PM, Philippe Mathieu-Daudé wrote: >>> Since v42: >>> - Stick to spec v4.3 (re-simplified EXT_CSD register & migrate) >>> - Fill CID register >>> - Few changes to CSD register >>> - Implement 'boot-mode' reset timing >>> - Add 'boot-size' property >>> >>> Change required for aspeed branch: >>> -- >8 -- >>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c >>> index 8c0e36badd..563816b710 100644 >>> --- a/hw/arm/aspeed.c >>> +++ b/hw/arm/aspeed.c >>> @@ -344,3 +344,3 @@ static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, bool emmc, >>> if (emmc) { >>> - qdev_prop_set_uint8(card, "boot-config", boot_emmc ? 0x48 : 0x0); >>> + qdev_prop_set_uint64(card, "boot-size", 1 * MiB); >>> } >>> (I'm still reluctant to merge patches 16-18)... >> >> Then, please drop all changes related to the boot partitions. I will keep >> the original patches in my tree and address the feature when I have time. >> TYPE_EMMC is already great to have. > > As you mentioned on today's community call, eMMC is a chipset soldered > on a board, so our boards exactly knows what size to expect on the card, > and whether boot partitions are present or not. My remark was on the use of 3 blockdevs to represent the eMMC contents. 1 should be enough. Having a "boot-size" property to set the BOOT_SIZE_MULT register is fine. The Aspeed model does some assumption today when installing the first boot area as a boot ROM : aspeed_install_boot_rom(bmc, blk_by_legacy_dinfo(emmc0), 64 * KiB); The "boot-size" property will clarify the settings. Please keep it. Other topic : ROMs need to be installed early and IIRC, user creatable devices are not available at that time. So, when the flash devices are be defined on the command line with : -blockdev node-name=fmc0,driver=file,filename=./flash.img \ -device mx66u51235f,cs=0x0,bus=ssi.0,drive=fmc0 \ the ROM can not be installed and the initial boot will execute in place, using SPI transactions to fetch instructions which is slower. To install a ROM, the -drive API is required. > I find the way of building the drive image described in patch #16 cumbersome, I agree but that's the layout on real HW. The flash layouts are even more complex. > but I'm OK > to make some concession on it, since "it works" as it. If necessary > we'll improve and deprecate the current properties. I don't think we will need to. The properties make sense. > I'll repost and plan to merge as-is (modulo review comments). OK. I will rebase then. Thanks, C.
On 7/10/24 6:58 AM, Cédric Le Goater wrote: > On 7/9/24 11:39 PM, Philippe Mathieu-Daudé wrote: >> On 9/7/24 17:58, Cédric Le Goater wrote: >>> On 7/9/24 5:25 PM, Philippe Mathieu-Daudé wrote: >>>> Since v42: >>>> - Stick to spec v4.3 (re-simplified EXT_CSD register & migrate) >>>> - Fill CID register >>>> - Few changes to CSD register >>>> - Implement 'boot-mode' reset timing >>>> - Add 'boot-size' property >>>> >>>> Change required for aspeed branch: >>>> -- >8 -- >>>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c >>>> index 8c0e36badd..563816b710 100644 >>>> --- a/hw/arm/aspeed.c >>>> +++ b/hw/arm/aspeed.c >>>> @@ -344,3 +344,3 @@ static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, bool emmc, >>>> if (emmc) { >>>> - qdev_prop_set_uint8(card, "boot-config", boot_emmc ? 0x48 : 0x0); >>>> + qdev_prop_set_uint64(card, "boot-size", 1 * MiB); >>>> } >>>> (I'm still reluctant to merge patches 16-18)... >>> >>> Then, please drop all changes related to the boot partitions. I will keep >>> the original patches in my tree and address the feature when I have time. >>> TYPE_EMMC is already great to have. >> >> As you mentioned on today's community call, eMMC is a chipset soldered >> on a board, so our boards exactly knows what size to expect on the card, >> and whether boot partitions are present or not. > > My remark was on the use of 3 blockdevs to represent the eMMC contents. > 1 should be enough. > > Having a "boot-size" property to set the BOOT_SIZE_MULT register is fine. > The Aspeed model does some assumption today when installing the first > boot area as a boot ROM : > > aspeed_install_boot_rom(bmc, blk_by_legacy_dinfo(emmc0), 64 * KiB); > > The "boot-size" property will clarify the settings. Please keep it. 64 * KiB is not related to the eMMC partition size. It is a SRAM limit in which the SoC loads the boot data. Thanks, C.
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c index 8c0e36badd..563816b710 100644 --- a/hw/arm/aspeed.c +++ b/hw/arm/aspeed.c @@ -344,3 +344,3 @@ static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, bool emmc, if (emmc) { - qdev_prop_set_uint8(card, "boot-config", boot_emmc ? 0x48 : 0x0); + qdev_prop_set_uint64(card, "boot-size", 1 * MiB); }