Hi Johan, On Fri, 12 Aug 2022 at 07:51, Johan Jonker <jbx6...@gmail.com> wrote: > > Hi Simon and others, > > Some comments. Have a look if it's useful. > Include is an example of how currently a new Rockchip external RKNAND FTL > block device must be included in U-boot. > Changes must be made all over the place. EFI has now become a somewhat > "obligation" for block devices, then handling > should be made more easy I propose. > > 1: > The creation and removal of block devices is kind of dynamic with the > blk_create_device function. > Is it possible to make the necessary functions in efi_device_path.c and > part.c more flexible as well by > a new "blk_create_efi_device()" and "blk_remove_efi_device()" function? So > everything combined in one function.
Do you mean to automatically create a device with the right uclass? Yes I think that could be done, but I have not looked at it. > > 2: > Make the class list more dynamic/expendable. Each time a new clas is needed > the source code must be changed. > Include a function that returns a generated class number that takes in > account the existing know classes. > ie,: "uclass_create" and "uclass_remove". > > Example block device creation: > > ret = uclass_get_device(UCLASS_RK_IDB, 0, &dev); > > // Be able to use a dynamic generated UCLASS for block devices. > > if (ret) { > printf("no IDB device found\n"); > return CMD_RET_FAILURE; > } > ret = blk_get_device(IF_TYPE_RK_IDB, 0, &bdev); > if (ret) { > ret = blk_create_device(dev, "idb_blk", "blk", > IF_TYPE_RK_IDB, 0, 512, > LBA, &bdev); > if (ret) > return ret; > > ret = blk_probe_or_unbind(bdev); > if (ret) > return ret; > } > > Example block device removal: > > ret = blk_find_device(IF_TYPE_RK_IDB, 0, &bdev); Note that the IF_TYPE goes away with this series and there is just the uclass. > > // Be able to find back a dynamic generated UCLASS. > > if (ret) { > printf("no IDB blk device found\n"); > return 0; > } > > device_remove(bdev, DM_REMOVE_NORMAL); > device_unbind(bdev); > > Make efi functions more flexible by replacing switch() functions by call back > functions for: > > dp_fill > dp_size > dev_print > print_part_header > > Add them with "blk_create_efi_device()" in a structure. > If not defined fall back to standard functions. We are trying to integrate EFI better into U-Boot so that it uses the normal devices. At present it has some parallel infrastructure. Please take a look at your patch after applying this series, then let me know what you think is needed. Regards, Simon > > ==== > > Kind regards, > > Johan Jonker > > On 8/12/22 03:34, Simon Glass wrote: > > The block interface has two separate implementations, one using driver > > model and one not. The latter is really only needed for SPL, where > > size constraints allegedly don't allow use of driver model. Of course > > we still need space for filesystems and other code, so it isn't clear > > that driver model is anything more than the straw that breaks the > > camel's back. > > > > The driver model version uses a uclass ID for the interface time, but > > converts back and forth between that and if_type, which is the legacy > > type. > > > > The HAVE_BLOCK_DEVICE define is mostly a hangover from the old days. > > At present its main purpose is to enable the legacy block implementation > > in SPL. > > > > Finally the use of 'select' to enable BLK does not work very well. It > > causes kconfig errors when another option depends on BLK and it is > > not recommended by the kconfig style guide. > > > > This series aims to clean things up: > > - Enable BLK based on whether different media types are used, but still > > allow boards to disable it > > - Rename HAVE_BLOCK_DEVICE to indicates its real purpose > > - Drop if_type and use the uclass instead > > - Drop some obsolete if_type values > > > > An issue not resolved by this series is that the sandbox host interface > > does not actually have a device. At present it uses the root device, which > > was convenience for the driver model conversion but not really correct. It > > should be possible to clean this up, in a future series. > > > > Another minor issue is the use of UCLASS_USB for a mass-storage device. > > This has been the case for a while and is not addresed by this series, > > other than to add a comment. > > > > Note that this test relies on Tom Rini's series to drop various boards > > including warp and cm_t335 > > > > Finally, a patch is included to make binman put fake files in a > > subdirectory, since repeated runs of certain boards can cause unrelated > > failues (e.g. chromebook_coral) when fake files are left around. > > > > Changes in v2: > > - Update commit message > > - Fix SPL_PARTITIONS too > > - Add SATA also > > - Refer to a suffix, not a prefix > > - Add new patch to handle UCLASS_EFI_MEDIA in dev_print() > > - Add new patch to drop ifname field from struct efi_disk_obj > > - Use conv_uclass_id() instead of the confusing uclass_id_to_uclass_id() > > > > From d0bb794b966a0134a6f321a414b48c28e8894180 Mon Sep 17 00:00:00 2001 > From: Johan Jonker <jbx6...@gmail.com> > Date: Sun, 7 Aug 2022 15:25:55 +0200 > Subject: prepare rknand > > > diff --git a/disk/part.c b/disk/part.c > index 79955c7fb0..3f8b97a6c6 100644 > --- a/disk/part.c > +++ b/disk/part.c > @@ -150,6 +150,7 @@ void dev_print (struct blk_desc *dev_desc) > case IF_TYPE_USB: > case IF_TYPE_NVME: > case IF_TYPE_PVBLOCK: > + case IF_TYPE_RKNAND: > case IF_TYPE_HOST: > printf ("Vendor: %s Rev: %s Prod: %s\n", > dev_desc->vendor, > @@ -293,6 +294,9 @@ static void print_part_header(const char *type, struct > blk_desc *dev_desc) > case IF_TYPE_PVBLOCK: > puts("PV BLOCK"); > break; > + case IF_TYPE_RKNAND: > + puts("RKNAND"); > + break; > case IF_TYPE_VIRTIO: > puts("VirtIO"); > break; > diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c > index 0b5f219d90..28b21836c4 100644 > --- a/drivers/block/blk-uclass.c > +++ b/drivers/block/blk-uclass.c > @@ -33,6 +33,7 @@ static const char *if_typename_str[IF_TYPE_COUNT] = { > [IF_TYPE_VIRTIO] = "virtio", > [IF_TYPE_PVBLOCK] = "pvblock", > [IF_TYPE_RK_IDB] = "idb", > + [IF_TYPE_RKNAND] = "rknand", > }; > > static enum uclass_id if_type_uclass_id[IF_TYPE_COUNT] = { > @@ -51,6 +52,7 @@ static enum uclass_id if_type_uclass_id[IF_TYPE_COUNT] = { > [IF_TYPE_VIRTIO] = UCLASS_VIRTIO, > [IF_TYPE_PVBLOCK] = UCLASS_PVBLOCK, > [IF_TYPE_RK_IDB] = UCLASS_RK_IDB, > + [IF_TYPE_RKNAND] = UCLASS_RKNAND, > }; > > static enum if_type if_typename_to_iftype(const char *if_typename) > diff --git a/include/blk.h b/include/blk.h > index a73cc577a0..56f2415e21 100644 > --- a/include/blk.h > +++ b/include/blk.h > @@ -30,6 +30,7 @@ enum if_type { > IF_TYPE_USB, > IF_TYPE_DOC, > IF_TYPE_MMC, > + IF_TYPE_RKNAND, > IF_TYPE_SD, > IF_TYPE_SATA, > IF_TYPE_HOST, > diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h > index 38a227f006..b102cdf46e 100644 > --- a/include/dm/uclass-id.h > +++ b/include/dm/uclass-id.h > @@ -104,6 +104,7 @@ enum uclass_id { > UCLASS_REGULATOR, /* Regulator device */ > UCLASS_REMOTEPROC, /* Remote Processor device */ > UCLASS_RESET, /* Reset controller device */ > + UCLASS_RKNAND, /* Rockchip nand device with ftl */ > UCLASS_RK_IDB, /* Rockchip IDB device */ > UCLASS_RNG, /* Random Number Generator */ > UCLASS_RTC, /* Real time clock device */ > diff --git a/include/efi_loader.h b/include/efi_loader.h > index 44d426035a..ddc7082ad6 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -145,6 +145,10 @@ static inline efi_status_t efi_launch_capsules(void) > #define U_BOOT_IDB_DEV_GUID \ > EFI_GUID(0xadc021df, 0x5f24, 0x464f, \ > 0x9a, 0x88, 0xdb, 0xee, 0x3f, 0x1d, 0x14, 0x0f) > +/* GUID used as root for Rockchip RKNAND devices */ > +#define U_BOOT_RKNAND_DEV_GUID \ > + EFI_GUID(0xe39f6cbb, 0x055a, 0x45a0, \ > + 0xb2, 0x75, 0x56, 0x0d, 0xa5, 0x22, 0x25, 0x99) > > /* Use internal device tree when starting UEFI application */ > #define EFI_FDT_USE_INTERNAL NULL > diff --git a/lib/efi_loader/efi_device_path.c > b/lib/efi_loader/efi_device_path.c > index b7535373e7..9691f02b2e 100644 > --- a/lib/efi_loader/efi_device_path.c > +++ b/lib/efi_loader/efi_device_path.c > @@ -31,6 +31,9 @@ const efi_guid_t efi_guid_virtio_dev = > U_BOOT_VIRTIO_DEV_GUID; > #if CONFIG_IS_ENABLED(ROCKCHIP_IDB) > const efi_guid_t efi_guid_idb_dev = U_BOOT_IDB_DEV_GUID; > #endif > +#if CONFIG_IS_ENABLED(RKNAND) > +const efi_guid_t efi_guid_rknand_dev = U_BOOT_IDB_DEV_GUID; > +#endif > > /* template END node: */ > static const struct efi_device_path END = { > @@ -578,6 +581,16 @@ __maybe_unused static unsigned int dp_size(struct > udevice *dev) > return dp_size(dev->parent) > + sizeof(struct efi_device_path_vendor) + 1; > #endif > +#if CONFIG_IS_ENABLED(RKNAND) > + case UCLASS_RKNAND: > + /* > + * Rockchip IDB device will be represented > + * as vendor device with extra one byte for > + * device number > + */ > + return dp_size(dev->parent) > + + sizeof(struct efi_device_path_vendor) + 1; > +#endif > #if CONFIG_IS_ENABLED(ROCKCHIP_IDB) > case UCLASS_RK_IDB: > /* > @@ -680,6 +693,23 @@ __maybe_unused static void *dp_fill(void *buf, struct > udevice *dev) > return &dp->vendor_data[1]; > } > #endif > +#if CONFIG_IS_ENABLED(RKNAND) > + case UCLASS_RKNAND: { > + struct efi_device_path_vendor *dp; > + struct blk_desc *desc = dev_get_uclass_plat(dev); > + > + dp_fill(buf, dev->parent); > + dp = buf; > + ++dp; > + dp->dp.type = DEVICE_PATH_TYPE_HARDWARE_DEVICE; > + dp->dp.sub_type = DEVICE_PATH_SUB_TYPE_VENDOR; > + dp->dp.length = sizeof(*dp) + 1; > + memcpy(&dp->guid, &efi_guid_rknand_dev, > + sizeof(efi_guid_t)); > + dp->vendor_data[0] = desc->devnum; > + return &dp->vendor_data[1]; > + } > +#endif > #if CONFIG_IS_ENABLED(ROCKCHIP_IDB) > case UCLASS_RK_IDB: { > struct efi_device_path_vendor *dp;