[v2,15/39] efi: Add EFI uclass for media

At present UCLASS_EFI is used to represent an EFI filesystem among other
things. The description of this uclass is "EFI managed devices" which is
pretty vague. The only driver that uses this uclass is in fact not a real
U-Boot driver, since its operations do not include a struct udevice.

Rather than mess with this, create a new UCLASS_EFI_MEDIA uclass to handle
EFI media such as disks. Make this the uclass to use for EFI media so that
it can be used with 'part list', for example.

The existing implementation using UCLASS_EFI remains as is, for
discussion.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v2:
- Add MAINTAINERS entry
- Show the correct interface type with 'part list'
- Update the commit message to explain things better

 MAINTAINERS                      |  3 +++
 arch/sandbox/dts/test.dts        |  4 ++++
 disk/part.c                      |  5 ++++-
 drivers/block/Kconfig            | 23 +++++++++++++++++++++++
 drivers/block/Makefile           |  3 +++
 drivers/block/blk-uclass.c       |  2 +-
 drivers/block/efi-media-uclass.c | 15 +++++++++++++++
 drivers/block/sb_efi_media.c     | 20 ++++++++++++++++++++
 include/dm/uclass-id.h           |  1 +
 test/dm/Makefile                 |  1 +
 test/dm/efi_media.c              | 24 ++++++++++++++++++++++++
 11 files changed, 99 insertions(+), 2 deletions(-)
 create mode 100644 drivers/block/efi-media-uclass.c
 create mode 100644 drivers/block/sb_efi_media.c
 create mode 100644 test/dm/efi_media.c

On 9/25/21 02:30, Simon Glass wrote:
> At present UCLASS_EFI is used to represent an EFI filesystem among other

UCLASS_EFI is for UEFI drivers that provide the driver binding protocol.

Your new UCLASS seems to be for U-Boot drivers consuming the
EFI_BLOCK_IO_PROTOCOL.

I agree that lib/efi_driver/efi_block_device.c is to much intertwined
with the UEFI subsystem to easily merge it with your requirement. But it
at least can serve as a template.

> things. The description of this uclass is "EFI managed devices" which is
> pretty vague. The only driver that uses this uclass is in fact not a real
> U-Boot driver, since its operations do not include a struct udevice.
>
> Rather than mess with this, create a new UCLASS_EFI_MEDIA uclass to handle
> EFI media such as disks. Make this the uclass to use for EFI media so that

In UEFI speak MEDIA relates to:

* block devices
* firmware files
* Bluetooth
* REST services
* etc

Do you only mean block devices?
Why can't you use UCLASS_BLK for your new driver?

Best regards

Heinrich

> it can be used with 'part list', for example.
>
> The existing implementation using UCLASS_EFI remains as is, for
> discussion.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v2:
> - Add MAINTAINERS entry
> - Show the correct interface type with 'part list'
> - Update the commit message to explain things better
>
>   MAINTAINERS                      |  3 +++
>   arch/sandbox/dts/test.dts        |  4 ++++
>   disk/part.c                      |  5 ++++-
>   drivers/block/Kconfig            | 23 +++++++++++++++++++++++
>   drivers/block/Makefile           |  3 +++
>   drivers/block/blk-uclass.c       |  2 +-
>   drivers/block/efi-media-uclass.c | 15 +++++++++++++++
>   drivers/block/sb_efi_media.c     | 20 ++++++++++++++++++++
>   include/dm/uclass-id.h           |  1 +
>   test/dm/Makefile                 |  1 +
>   test/dm/efi_media.c              | 24 ++++++++++++++++++++++++
>   11 files changed, 99 insertions(+), 2 deletions(-)
>   create mode 100644 drivers/block/efi-media-uclass.c
>   create mode 100644 drivers/block/sb_efi_media.c
>   create mode 100644 test/dm/efi_media.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a254b705e73..0c1cbff7d01 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -708,8 +708,11 @@ W:	https://u-boot.readthedocs.io/en/latest/develop/uefi/u-boot_on_efi.html
>   F:	board/efi/efi-x86_app
>   F:	configs/efi-x86_app*
>   F:	doc/develop/uefi/u-boot_on_efi.rst
> +F:	drivers/block/efi-media-uclass.c
> +F:	drivers/block/sb_efi_media.c
>   F:	lib/efi/efi_app.c
>   F:	scripts/build-efi.sh
> +F:	test/dm/efi_media.c
>
>   EFI PAYLOAD
>   M:	Heinrich Schuchardt <xypron.glpk@gmx.de>
> diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
> index b36447c4a79..23b8435a92a 100644
> --- a/arch/sandbox/dts/test.dts
> +++ b/arch/sandbox/dts/test.dts
> @@ -495,6 +495,10 @@
>   		compatible = "sandbox,clk-ccf";
>   	};
>
> +	efi-media {
> +		compatible = "sandbox,efi-media";
> +	};
> +
>   	eth@10002000 {
>   		compatible = "sandbox,eth";
>   		reg = <0x10002000 0x1000>;
> diff --git a/disk/part.c b/disk/part.c
> index a6a8f7052bd..2560f6a50bc 100644
> --- a/disk/part.c
> +++ b/disk/part.c
> @@ -296,8 +296,11 @@ static void print_part_header(const char *type, struct blk_desc *dev_desc)
>   	case IF_TYPE_VIRTIO:
>   		puts("VirtIO");
>   		break;
> +	case IF_TYPE_EFI:
> +		puts("EFI");
> +		break;
>   	default:
> -		puts ("UNKNOWN");
> +		puts("UNKNOWN");
>   		break;
>   	}
>   	printf (" device %d  --   Partition Type: %s\n\n",
> diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
> index 56a4eec05ac..755fdccb574 100644
> --- a/drivers/block/Kconfig
> +++ b/drivers/block/Kconfig
> @@ -61,6 +61,29 @@ config TPL_BLOCK_CACHE
>   	help
>   	  This option enables the disk-block cache in TPL
>
> +config EFI_MEDIA
> +	bool "Support EFI media drivers"
> +	default y if EFI || SANDBOX
> +	help
> +	  Enable this to support media devices on top of UEFI. This enables
> +	  just the uclass so you also need a specific driver to make this do
> +	  anything.
> +
> +	  For sandbox there is a test driver.
> +
> +if EFI_MEDIA
> +
> +config EFI_MEDIA_SANDBOX
> +	bool "Sandbox EFI media driver"
> +	depends on SANDBOX
> +	default y
> +	help
> +	  Enables a simple sandbox media driver, used for testing just the
> +	  EFI_MEDIA uclass. It does not do anything useful, since sandbox does
> +	  not actually support running on top of UEFI.
> +
> +endif  # EFI_MEDIA
> +
>   config IDE
>   	bool "Support IDE controllers"
>   	select HAVE_BLOCK_DEVICE
> diff --git a/drivers/block/Makefile b/drivers/block/Makefile
> index 94ab5c6f906..3778633da1d 100644
> --- a/drivers/block/Makefile
> +++ b/drivers/block/Makefile
> @@ -14,3 +14,6 @@ obj-$(CONFIG_IDE) += ide.o
>   endif
>   obj-$(CONFIG_SANDBOX) += sandbox.o
>   obj-$(CONFIG_$(SPL_TPL_)BLOCK_CACHE) += blkcache.o
> +
> +obj-$(CONFIG_EFI_MEDIA) += efi-media-uclass.o
> +obj-$(CONFIG_EFI_MEDIA_SANDBOX) += sb_efi_media.o
> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
> index 83682dcc181..2db7ce5de20 100644
> --- a/drivers/block/blk-uclass.c
> +++ b/drivers/block/blk-uclass.c
> @@ -44,7 +44,7 @@ static enum uclass_id if_type_uclass_id[IF_TYPE_COUNT] = {
>   	[IF_TYPE_SATA]		= UCLASS_AHCI,
>   	[IF_TYPE_HOST]		= UCLASS_ROOT,
>   	[IF_TYPE_NVME]		= UCLASS_NVME,
> -	[IF_TYPE_EFI]		= UCLASS_EFI,

UCLASS_EFI_MEDIA is only relevant for the EFI app and should not be
compiled into normal U-Boot. This change is bound to break the UEFI
sub-system.

Why don't you define a new if_type for your use case?

blk_get_devnum_by_typename() uses if_type to check if the corresponding
uclass exists. Why is this check needed? If we remove it, we can drop
if_type_uclass_id[] completely. This would be my preferred solution.

Best regards

Heinrich

> +	[IF_TYPE_EFI]		= UCLASS_EFI_MEDIA,
>   	[IF_TYPE_VIRTIO]	= UCLASS_VIRTIO,
>   	[IF_TYPE_PVBLOCK]	= UCLASS_PVBLOCK,
>   };
> diff --git a/drivers/block/efi-media-uclass.c b/drivers/block/efi-media-uclass.c
> new file mode 100644
> index 00000000000..e012f6f2f4c
> --- /dev/null
> +++ b/drivers/block/efi-media-uclass.c
> @@ -0,0 +1,15 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Uclass for EFI media devices
> + *
> + * Copyright 2021 Google LLC
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +
> +UCLASS_DRIVER(efi_media) = {
> +	.id		= UCLASS_EFI_MEDIA,
> +	.name		= "efi_media",
> +	.flags		= DM_UC_FLAG_SEQ_ALIAS,
> +};
> diff --git a/drivers/block/sb_efi_media.c b/drivers/block/sb_efi_media.c
> new file mode 100644
> index 00000000000..52af155a600
> --- /dev/null
> +++ b/drivers/block/sb_efi_media.c
> @@ -0,0 +1,20 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * EFI_MEDIA driver for sandbox
> + *
> + * Copyright 2021 Google LLC
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +
> +static const struct udevice_id sandbox_efi_media_ids[] = {
> +	{ .compatible = "sandbox,efi-media" },
> +	{ }
> +};
> +
> +U_BOOT_DRIVER(sandbox_efi_media) = {
> +	.name		= "sandbox_efi_media",
> +	.id		= UCLASS_EFI_MEDIA,
> +	.of_match	= sandbox_efi_media_ids,
> +};
> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index 3768432b680..34f5e6689d3 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -49,6 +49,7 @@ enum uclass_id {
>   	UCLASS_DSA,		/* Distributed (Ethernet) Switch Architecture */
>   	UCLASS_ECDSA,		/* Elliptic curve cryptographic device */
>   	UCLASS_EFI,		/* EFI managed devices */
> +	UCLASS_EFI_MEDIA,	/* EFI media (e.g. a disk) */
>   	UCLASS_ETH,		/* Ethernet device */
>   	UCLASS_ETH_PHY,		/* Ethernet PHY device */
>   	UCLASS_FIRMWARE,	/* Firmware */
> diff --git a/test/dm/Makefile b/test/dm/Makefile
> index 55162e9499d..e24679eb640 100644
> --- a/test/dm/Makefile
> +++ b/test/dm/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_DMA) += dma.o
>   obj-$(CONFIG_VIDEO_MIPI_DSI) += dsi_host.o
>   obj-$(CONFIG_DM_DSA) += dsa.o
>   obj-$(CONFIG_ECDSA_VERIFY) += ecdsa.o
> +obj-$(CONFIG_EFI_MEDIA_SANDBOX) += efi_media.o
>   obj-$(CONFIG_DM_ETH) += eth.o
>   ifneq ($(CONFIG_EFI_PARTITION),)
>   obj-$(CONFIG_FASTBOOT_FLASH_MMC) += fastboot.o
> diff --git a/test/dm/efi_media.c b/test/dm/efi_media.c
> new file mode 100644
> index 00000000000..e343a0e9c85
> --- /dev/null
> +++ b/test/dm/efi_media.c
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Test for EFI_MEDIA uclass
> + *
> + * Copyright 2021 Google LLC
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <asm/test.h>
> +#include <dm/test.h>
> +#include <test/test.h>
> +#include <test/ut.h>
> +
> +/* Test that we can use the EFI_MEDIA uclass */
> +static int dm_test_efi_media(struct unit_test_state *uts)
> +{
> +	struct udevice *dev;
> +
> +	ut_assertok(uclass_first_device_err(UCLASS_EFI_MEDIA, &dev));
> +
> +	return 0;
> +}
> +DM_TEST(dm_test_efi_media, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
>

Hi Heinrich,

On Sat, 23 Oct 2021 at 06:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
>
> On 9/25/21 02:30, Simon Glass wrote:
> > At present UCLASS_EFI is used to represent an EFI filesystem among other
>
> UCLASS_EFI is for UEFI drivers that provide the driver binding protocol.
>
> Your new UCLASS seems to be for U-Boot drivers consuming the
> EFI_BLOCK_IO_PROTOCOL.
>
> I agree that lib/efi_driver/efi_block_device.c is to much intertwined
> with the UEFI subsystem to easily merge it with your requirement. But it
> at least can serve as a template.

It did.

>
> > things. The description of this uclass is "EFI managed devices" which is
> > pretty vague. The only driver that uses this uclass is in fact not a real
> > U-Boot driver, since its operations do not include a struct udevice.
> >
> > Rather than mess with this, create a new UCLASS_EFI_MEDIA uclass to handle
> > EFI media such as disks. Make this the uclass to use for EFI media so that
>
> In UEFI speak MEDIA relates to:
>
> * block devices
> * firmware files
> * Bluetooth
> * REST services
> * etc
>
> Do you only mean block devices?

Sort of...it is a parent of a block device, like MMC or SCSI.

> Why can't you use UCLASS_BLK for your new driver?

That is the child. I need something to be the parent so that it actually works.

Regards,
Simon