Hi Johan,
On Fri, 12 Aug 2022 at 07:51, Johan Jonker jbx6244@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 jbx6244@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;