On 8/12/22 17:11, Simon Glass wrote:
> 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.
Hi,
Some comments...
Reduced the number of public CC's.
I have included a patch with call back function as example, so
efi_device_path.c and part.c don't have to be changed for every new block
device.
Make EFI framework code without class specific things.
TODO define { UCLASS_XXXXX, "xxxxx" }, in class driver and not in a fixed
array.
Look up for xxxxx ==> UCLASS_XXXXX needs a lot more work which is out of scope
for my capacity now.
Kind regards,
Johan Jonker
===
This list name should be specified in the class driver.
Lookup functions need a rework then to get rid of this fixed array.
static struct {
enum uclass_id id;
const char *name;
} uclass_idname_str[] = {
{ UCLASS_IDE, "ide" },
{ UCLASS_SCSI, "scsi" },
{ UCLASS_USB, "usb" },
{ UCLASS_MMC, "mmc" },
{ UCLASS_AHCI, "sata" },
{ UCLASS_ROOT, "host" },
{ UCLASS_NVME, "nvme" },
{ UCLASS_EFI_MEDIA, "efi" },
{ UCLASS_EFI_LOADER, "efiloader" },
{ UCLASS_VIRTIO, "virtio" },
{ UCLASS_PVBLOCK, "pvblock" },
{ UCLASS_RKNAND, "rknand" },
};
===
Example for new call back functions:
void rknand_part_header(struct blk_desc *dev_desc)
{
puts("RKNAND");
}
void rknand_dev_print(struct blk_desc *dev_desc)
{
printf ("Vendor: %s Rev: %s Prod: %s\n",
dev_desc->vendor,
dev_desc->revision,
dev_desc->product);
}
extern unsigned int dp_size(struct udevice *dev);
extern void *dp_fill(void *buf, struct udevice *dev);
/* 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)
const efi_guid_t efi_guid_rknand_dev = U_BOOT_RKNAND_DEV_GUID;
unsigned int rknand_dp_size(struct udevice *dev)
{
return dp_size(dev->parent) + sizeof(struct efi_device_path_vendor) + 1;
}
void *rknand_dp_fill(void *buf, struct udevice *dev)
{
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];
}
static int rknand_blk_probe(struct udevice *udev)
{
struct rknand_dev *ndev = dev_get_priv(udev->parent);
struct blk_desc *desc = dev_get_uclass_plat(udev);
debug("%s %d %p ndev = %p %p\n", __func__, __LINE__,
udev, ndev, udev->parent);
ndev->dev = udev;
desc->uclass_id = UCLASS_RKNAND;
desc->lba = ndev->density;
desc->log2blksz = 9;
desc->blksz = 512;
desc->bdev = udev;
desc->devnum = 0;
//======
/* new call back functions */
desc->dev_print = &rknand_dev_print;
desc->part_header = &rknand_part_header;
udev->dp_size = &rknand_dp_size;
udev->dp_fill = &rknand_dp_fill;
//======
sprintf(desc->vendor, "0x%.4x", 0x2207);
memcpy(desc->product, "rknand", sizeof("rknand"));
memcpy(desc->revision, "V1.00", sizeof("V1.00"));
part_init(desc);
return 0;
}
===
>
>>
>> 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;
>From 70ef594197302f3497dac1fa98f419be143257d1 Mon Sep 17 00:00:00 2001
From: Johan Jonker <jbx6...@gmail.com>
Date: Sat, 13 Aug 2022 20:51:51 +0200
Subject: [PATCH 1/2] disk: part: add call back functions for print_part_header
and dev_print
In order to be able to add more block devices without changing
the part.c file add call back functions for print_part_header
and dev_print.
Signed-off-by: Johan Jonker <jbx6...@gmail.com>
---
disk/part.c | 8 ++++++--
include/blk.h | 2 ++
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/disk/part.c b/disk/part.c
index d73318c8..42e88fc7 100644
--- a/disk/part.c
+++ b/disk/part.c
@@ -120,7 +120,9 @@ void dev_print(struct blk_desc *dev_desc)
return;
}
- switch (dev_desc->uclass_id) {
+ if (dev_desc->dev_print) {
+ dev_desc->dev_print(dev_desc);
+ } else switch (dev_desc->uclass_id) {
case UCLASS_SCSI:
printf ("(%d:%d) Vendor: %s Prod.: %s Rev: %s\n",
dev_desc->target,dev_desc->lun,
@@ -248,7 +250,9 @@ static void print_part_header(const char *type, struct
blk_desc *dev_desc)
CONFIG_IS_ENABLED(AMIGA_PARTITION) || \
CONFIG_IS_ENABLED(EFI_PARTITION)
puts ("\nPartition Map for ");
- switch (dev_desc->uclass_id) {
+ if (dev_desc->part_header) {
+ dev_desc->part_header(dev_desc);
+ } else switch (dev_desc->uclass_id) {
case UCLASS_IDE:
puts ("IDE");
break;
diff --git a/include/blk.h b/include/blk.h
index cdc6f0fc..f158f039 100644
--- a/include/blk.h
+++ b/include/blk.h
@@ -69,6 +69,8 @@ struct blk_desc {
lbaint_t lba; /* number of blocks */
unsigned long blksz; /* block size */
int log2blksz; /* for convenience: log2(blksz) */
+ void (*part_header)(struct blk_desc *dev_desc); /*
print_part_header in part.c */
+ void (*dev_print)(struct blk_desc *dev_desc); /*
rknand_dev_print in part.c */
char vendor[BLK_VEN_SIZE + 1]; /* device vendor string */
char product[BLK_PRD_SIZE + 1]; /* device product number */
char revision[BLK_REV_SIZE + 1]; /* firmware revision */
--
2.20.1
>From 8f05d7887955393e0b14cae43f061703694306a4 Mon Sep 17 00:00:00 2001
From: Johan Jonker <jbx6...@gmail.com>
Date: Sat, 13 Aug 2022 21:11:50 +0200
Subject: [PATCH 2/2] lib: efi_loader: efi_device_path: add dp_fill and dp_size
call back functions
In order to be able to add more block devices without changing
the efi_device_path.c file add dp_fill and dp_size call back functions
Signed-off-by: Johan Jonker <jbx6...@gmail.com>
---
include/dm/device.h | 2 ++
lib/efi_loader/efi_device_path.c | 12 ++++++++----
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/include/dm/device.h b/include/dm/device.h
index 12c6ba37..add219be 100644
--- a/include/dm/device.h
+++ b/include/dm/device.h
@@ -194,6 +194,8 @@ struct udevice {
#if CONFIG_IS_ENABLED(DM_DMA)
ulong dma_offset;
#endif
+ unsigned int (*dp_size)(struct udevice *dev);
+ void * (*dp_fill)(void *buf, struct udevice *dev);
};
static inline int dm_udevice_size(void)
diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
index 9787033c..57ac31f1 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -515,7 +515,7 @@ bool efi_dp_is_multi_instance(const struct efi_device_path
*dp)
/* size of device-path not including END node for device and all parents
* up to the root device.
*/
-__maybe_unused static unsigned int dp_size(struct udevice *dev)
+__maybe_unused unsigned int dp_size(struct udevice *dev)
{
if (!dev || !dev->driver)
return sizeof(ROOT);
@@ -529,7 +529,9 @@ __maybe_unused static unsigned int dp_size(struct udevice
*dev)
return dp_size(dev->parent) +
sizeof(struct efi_device_path_mac_addr);
case UCLASS_BLK:
- switch (dev->parent->uclass->uc_drv->id) {
+ if (dev->dp_size) {
+ return dev->dp_size(dev);
+ } else switch (dev->parent->uclass->uc_drv->id) {
#ifdef CONFIG_IDE
case UCLASS_IDE:
return dp_size(dev->parent) +
@@ -600,7 +602,7 @@ __maybe_unused static unsigned int dp_size(struct udevice
*dev)
* @dev device
* Return: pointer to the end of the device path
*/
-__maybe_unused static void *dp_fill(void *buf, struct udevice *dev)
+__maybe_unused void *dp_fill(void *buf, struct udevice *dev)
{
if (!dev || !dev->driver)
return buf;
@@ -631,7 +633,9 @@ __maybe_unused static void *dp_fill(void *buf, struct
udevice *dev)
}
#endif
case UCLASS_BLK:
- switch (dev->parent->uclass->uc_drv->id) {
+ if (dev->dp_fill) {
+ return dev->dp_fill(buf, dev);
+ } else switch (dev->parent->uclass->uc_drv->id) {
#ifdef CONFIG_SANDBOX
case UCLASS_ROOT: {
/* stop traversing parents at this point: */
--
2.20.1
