[PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[AKASHI Takahiro]

With this patch set[1] applied, UEFI subsystem maintains a list of its
disk objects dynamically at runtime based on block device's probing.
(See "issues" below.)

[1] https://github.com/t-akashi/u-boot/tree/efi/dm_disk

For instance,
=> dm tree
 Class     Index  Probed  Driver                Name
-----------------------------------------------------------
 root          0  [ + ]   root_driver           root_driver
 ...
 pci           0  [ + ]   pci_generic_ecam      |-- pcie@10000000
 ...
 ahci          0  [   ]   ahci_pci              |   |-- ahci_pci
 scsi          0  [   ]   ahci_scsi             |   |   `-- ahci_scsi
 usb           0  [   ]   xhci_pci              |   `-- xhci_pci
 ...
=> efi devices
Missing RNG device for EFI_RNG_PROTOCOL
No EFI system partition
Unable to find TPMv2 device
Device           Device Path
================ ====================
000000013eee88d0 /VenHw(..)
000000013ffeb798 /VenHw(..)/Uart(0,0,D,D)
000000013eeeb810 /VenHw(..)/MAC(525252525252,1)
=> scsi rescan
 ...
=> efi devices
Device           Device Path
================ ====================
000000013eee88d0 /VenHw(..)
000000013ffeb798 /VenHw(..)/Uart(0,0,D,D)
000000013eeeb810 /VenHw(..)/MAC(525252525252,1)
000000013eefb730 /VenHw(..)/Scsi(0,0)
000000013eefb840 /VenHw(..)/Scsi(0,0)/HD(1,GPT,ce86c5a7-b32a-488f-a346-88fe698e0edc,0x22,0x4c2a)
000000013eefbc80 /VenHw(..)/Scsi(0,0)/HD(2,GPT,aa80aab9-33e6-42b6-b5db-def2cb8d7844,0x5000,0x1a800)
=> usb start
 ...
=> efi devices
Device           Device Path
================ ====================
000000013eee88d0 /VenHw(..)
000000013ffeb798 /VenHw(..)/Uart(0,0,D,D)
000000013eeeb810 /VenHw(..)/MAC(525252525252,1)
000000013eefb730 /VenHw(..)/Scsi(0,0)
000000013eefb840 /VenHw(..)/Scsi(0,0)/HD(1,GPT,ce86c5a7-b32a-488f-a346-88fe698e0edc,0x22,0x4c2a)
000000013eefbc80 /VenHw(..)/Scsi(0,0)/HD(2,GPT,aa80aab9-33e6-42b6-b5db-def2cb8d7844,0x5000,0x1a800)
000000013ef01330 /VenHw(..)/UsbClass(0x0,0x0,0x9,0x0,0x3)/UsbClass(0x46f4,0x1,0x0,0x0,0x0)
000000013ef014b0 /VenHw(..)/UsbClass(0x0,0x0,0x9,0x0,0x3)/UsbClass(0x46f4,0x1,0x0,0x0,0x0)/HD(1,GPT,ce86c5a7-b32a-488f-a346-88fe698e0edc,0x22,0x4c2a)
000000013ef018f0 /VenHw(..)/UsbClass(0x0,0x0,0x9,0x0,0x3)/UsbClass(0x46f4,0x1,0x0,0x0,0x0)/HD(2,GPT,aa80aab9-33e6-42b6-b5db-def2cb8d7844,0x5000,0x1a800)
=> dm tree
 ...
 pci           0  [ + ]   pci_generic_ecam      |-- pcie@10000000
 ...
 ahci          0  [ + ]   ahci_pci              |   |-- ahci_pci
 scsi          0  [ + ]   ahci_scsi             |   |   `-- ahci_scsi
 blk           2  [ + ]   scsi_blk              |   |       `-- ahci_scsi.id0lun0
 partition     0  [ + ]   blk_partition         |   |           |-- ahci_scsi.id0lun0:1
 partition     1  [ + ]   blk_partition         |   |           `-- ahci_scsi.id0lun0:2
 usb           0  [ + ]   xhci_pci              |   `-- xhci_pci
 usb_hub       0  [ + ]   usb_hub               |       `-- usb_hub
 usb_mass_s    0  [ + ]   usb_mass_storage      |           |-- usb_mass_storage
 blk           3  [ + ]   usb_storage_blk       |           |   `-- usb_mass_storage.lun0
 partition     2  [ + ]   blk_partition         |           |       |-- usb_mass_storage.lun0:1
 partition     3  [ + ]   blk_partition         |           |       `-- usb_mass_storage.lun0:2
 ...

=> usb stop
stopping USB..
=> efi devices
Device           Device Path
================ ====================
000000013eee88d0 /VenHw(..)
000000013ffeb798 /VenHw(..)/Uart(0,0,D,D)
000000013eeeb810 /VenHw(..)/MAC(525252525252,1)
000000013eefb730 /VenHw(..)/Scsi(0,0)
000000013eefb840 /VenHw(..)/Scsi(0,0)/HD(1,GPT,ce86c5a7-b32a-488f-a346-88fe698e0edc,0x22,0x4c2a)
000000013eefbc80 /VenHw(..)/Scsi(0,0)/HD(2,GPT,aa80aab9-33e6-42b6-b5db-def2cb8d7844,0x5000,0x1a800)


Issues:
=======
* The image size of U-Boot may increase. CI build test complains,
  for instance,
    rcar3_salvator-x:
      "u-boot.img exceeds file size limit: ... excess: 0x79c bytes"
    phycore-rk3288:
      "SPL image is too large (size 0x8800 than 0x8000)"

  See [2].

* For removal case, we may need more consideration since removing handles
  unconditionally may end up breaking integrity of handles
  (as some may still be held and referenced to by a UEFI app).

[2] https://dev.azure.com/u-boot/u-boot/_build/results?buildId=3770&view=results


Prerequisite:
=============
* Simon's event[3]

[3] https://lists.denx.de/pipermail/u-boot/2022-March/476844.html


Patchs:
=======
For easy understandings, patches may be categorized into separate groups
of changes.

Patch#1-#7: DM: add device_probe() for later use of events
Patch#8-#10: DM: add a new feature (DM tag)
Patch#11-#15: UEFI: dynamically create/remove efi_disk's for a raw disk
  and its partitions
Patch#16-#17: UEFI: use udevice read/write interfaces
Patch#18-#19: UEFI: fix-up efi_driver, aligning with changes in DM integration


Change history:
===============
v3 (Mar 8, 2022)
* rebased on 2022.04-rc3
* rebased on v2 of Simon's event patch
  (hence removed his patch from this patch set)
* fix a spl-build error in mmc_blk_probe() (patch#3)
* fix a build warning in fsl_ata_probe()/sil_pci_probe() (patch#5)
* remove a problematic log message in efi_disk_probe() (patch#14)

v2 (Feb 10, 2022)
* add/revise an error message if device_probe() fails (patch#3,#5)
* fix a build error in sandbox_spl_defconfig (patch#8)
* fix warnings in 'make htmldocs' (patch#8,#9,#18)
* new commit: split efi_init_obj_list() (patch#14)
* new commit: simplify efi_capsule pytest (patch#21)

v1 (Feb 2, 2022)
* rebased on 2022.04-rc1
* drop patches that have already been merged
* modify a tag-range check with "tag >= DM_TAG_COUNT" (patch#9)
* move dmtag_list to GD (global data) (patch#9)
* add function descriptions and a document about DM tag feature (patch#9,10)
* add tests for DM tag support (patch#11)
* change 'depends on EVENT' to 'select EVENT' for EFI_LOADER (patch#14)
* migrate IF_TYPE_EFI to IF_TYPE_EFI_LOADER (patch#18)

RFCv2 (Dec 10, 2021)
* rebased on 2022-rc3
* re-order and merge some related commits into ones
* call device_probe() in MMC (not bind, but) probe hook (patch#5)
* fix a wrong name of variable (patch#7)
* add patch#9
* invoke device_probe() for virtio devices (patch#10)
* add DM event notification (from Simon) (patch#11)
* add DM tag support (patch#12)
* move UCLASS_PARTITION driver under disk/ (patch#13)
* create partition's dp using its parent's. This change is necessary
  in particular for 'efi_blk' efi_disk (patch#13)
* modify the code so that we will use new features like tags and
  event notification (patch#13,15,16,20)
* rename new functions from blk_read/write() to dev_read/write()
  (patch#17,18)
* isolate changes in efi_driver from the rest (in efi_loader) (patch#19)
* drop the previous patch#22 ("efi_selftest: block device: adjust dp
  for a test") due to the fix in patch#13

RFC (Nov 16, 2021)
* initial RFC

AKASHI Takahiro (19):
  scsi: call device_probe() after scanning
  usb: storage: call device_probe() after scanning
  mmc: call device_probe() after scanning
  nvme: call device_probe() after scanning
  sata: call device_probe() after scanning
  block: ide: call device_probe() after scanning
  virtio: call device_probe() in scanning
  dm: add tag support
  dm: tag: add some document
  test: dm: add tests for tag support
  dm: disk: add UCLASS_PARTITION
  dm: blk: add a device-probe hook for scanning disk partitions
  efi_loader: split efi_init_obj_list() into two stages
  efi_loader: disk: a helper function to create efi_disk objects from
    udevice
  efi_loader: disk: a helper function to delete efi_disk objects
  dm: disk: add read/write interfaces with udevice
  efi_loader: disk: use udevice instead of blk_desc
  efi_loader: disk: not create BLK device for BLK(IF_TYPE_EFI_LOADER)
    devices
  efi_driver: align with efi_disk-dm integration

 cmd/virtio.c                        |  21 +-
 common/board_r.c                    |   2 +-
 common/main.c                       |   7 +-
 common/usb_storage.c                |   4 +
 disk/Makefile                       |   3 +
 disk/disk-uclass.c                  | 247 +++++++++++++++++++++
 doc/develop/driver-model/design.rst |  20 ++
 drivers/ata/dwc_ahsata.c            |   5 +
 drivers/ata/fsl_sata.c              |  11 +
 drivers/ata/sata_mv.c               |   5 +
 drivers/ata/sata_sil.c              |  12 +
 drivers/block/blk-uclass.c          |   4 +
 drivers/block/ide.c                 |   4 +
 drivers/core/Makefile               |   2 +-
 drivers/core/root.c                 |   2 +
 drivers/core/tag.c                  | 139 ++++++++++++
 drivers/mmc/mmc-uclass.c            |  12 +
 drivers/nvme/nvme.c                 |   4 +
 drivers/scsi/scsi.c                 |   5 +
 include/asm-generic/global_data.h   |   4 +
 include/dm/tag.h                    | 110 ++++++++++
 include/dm/uclass-id.h              |   1 +
 include/efi_loader.h                |   6 +-
 include/part.h                      |  18 ++
 lib/efi_driver/efi_block_device.c   |  34 +--
 lib/efi_loader/Kconfig              |   3 +
 lib/efi_loader/efi_disk.c           | 328 ++++++++++++++++++++--------
 lib/efi_loader/efi_setup.c          |  62 +++++-
 test/dm/Makefile                    |   1 +
 test/dm/tag.c                       |  84 +++++++
 30 files changed, 1029 insertions(+), 131 deletions(-)
 create mode 100644 disk/disk-uclass.c
 create mode 100644 drivers/core/tag.c
 create mode 100644 include/dm/tag.h
 create mode 100644 test/dm/tag.c

-- 
2.33.0

[PATCH v3 01/19] scsi: call device_probe() after scanning

[AKASHI Takahiro]

Every time a scsi bus/port is scanned and a new block device is detected,
we want to call device_probe() as it will give us a chance to run
additional post-processings for some purposes.

In particular, support for creating partitions on a device will be added.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 drivers/scsi/scsi.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index d7b33010e469..78d729d809d7 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -595,6 +595,11 @@ static int do_scsi_scan_one(struct udevice *dev, int id, int lun, bool verbose)
 		ata_swap_buf_le16((u16 *)&bdesc->revision, sizeof(bd.revision) / 2);
 	}
 
+	ret = blk_probe_or_unbind(bdev);
+	if (ret < 0)
+		/* TODO: undo create */
+		return ret;
+
 	if (verbose) {
 		printf("  Device %d: ", bdesc->devnum);
 		dev_print(bdesc);
-- 
2.33.0

[PATCH v3 02/19] usb: storage: call device_probe() after scanning

[AKASHI Takahiro]

Every time a usb bus/port is scanned and a new device is detected,
we want to call device_probe() as it will give us a chance to run
additional post-processings for some purposes.

In particular, support for creating partitions on a device will be added.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 common/usb_storage.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/common/usb_storage.c b/common/usb_storage.c
index c9e2d7343ce2..291728f37e0a 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -239,6 +239,10 @@ static int usb_stor_probe_device(struct usb_device *udev)
 			if (ret)
 				return ret;
 		}
+
+		ret = blk_probe_or_unbind(dev);
+		if (ret)
+			return ret;
 	}
 #else
 	/* We don't have space to even probe if we hit the maximum */
-- 
2.33.0

[PATCH v3 03/19] mmc: call device_probe() after scanning

[AKASHI Takahiro]

Every time a mmc bus/port is scanned and a new device is detected,
we want to call device_probe() as it will give us a chance to run
additional post-processings for some purposes.

In particular, support for creating partitions on a device will be added.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
---
 drivers/mmc/mmc-uclass.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c
index b80e838066ca..57da788ad805 100644
--- a/drivers/mmc/mmc-uclass.c
+++ b/drivers/mmc/mmc-uclass.c
@@ -467,6 +467,18 @@ static int mmc_blk_probe(struct udevice *dev)
 		return ret;
 	}
 
+	ret = device_probe(dev);
+	if (ret) {
+		debug("Probing %s failed (err=%d)\n", dev->name, ret);
+
+		if (CONFIG_IS_ENABLED(MMC_UHS_SUPPORT) ||
+		    CONFIG_IS_ENABLED(MMC_HS200_SUPPORT) ||
+		    CONFIG_IS_ENABLED(MMC_HS400_SUPPORT))
+			mmc_deinit(mmc);
+
+		return ret;
+	}
+
 	return 0;
 }
 
-- 
2.33.0

[PATCH v3 04/19] nvme: call device_probe() after scanning

[AKASHI Takahiro]

Every time a nvme bus/port is scanned and a new device is detected,
we want to call device_probe() as it will give us a chance to run
additional post-processings for some purposes.

In particular, support for creating partitions on a device will be added.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 drivers/nvme/nvme.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c
index 1d56517e9969..a305305885ec 100644
--- a/drivers/nvme/nvme.c
+++ b/drivers/nvme/nvme.c
@@ -884,6 +884,10 @@ int nvme_init(struct udevice *udev)
 					 -1, 512, 0, &ns_udev);
 		if (ret)
 			goto free_id;
+
+		ret = blk_probe_or_unbind(ns_udev);
+		if (ret)
+			goto free_id;
 	}
 
 	free(id);
-- 
2.33.0

[PATCH v3 05/19] sata: call device_probe() after scanning

[AKASHI Takahiro]

Every time a sata bus/port is scanned and a new device is detected,
we want to call device_probe() as it will give us a chance to run
additional post-processings for some purposes.

In particular, support for creating partitions on a device will be added.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 drivers/ata/dwc_ahsata.c |  5 +++++
 drivers/ata/fsl_sata.c   | 11 +++++++++++
 drivers/ata/sata_mv.c    |  5 +++++
 drivers/ata/sata_sil.c   | 12 ++++++++++++
 4 files changed, 33 insertions(+)

diff --git a/drivers/ata/dwc_ahsata.c b/drivers/ata/dwc_ahsata.c
index 6d42548087b3..d9fd850c6fae 100644
--- a/drivers/ata/dwc_ahsata.c
+++ b/drivers/ata/dwc_ahsata.c
@@ -1026,6 +1026,11 @@ int dwc_ahsata_scan(struct udevice *dev)
 		return ret;
 	}
 
+	ret = blk_probe_or_unbind(dev);
+	if (ret < 0)
+		/* TODO: undo create */
+		return ret;
+
 	return 0;
 }
 
diff --git a/drivers/ata/fsl_sata.c b/drivers/ata/fsl_sata.c
index e44db0a37458..d1bab931895a 100644
--- a/drivers/ata/fsl_sata.c
+++ b/drivers/ata/fsl_sata.c
@@ -982,6 +982,17 @@ static int fsl_ata_probe(struct udevice *dev)
 			failed_number++;
 			continue;
 		}
+
+		ret = device_probe(dev);
+		if (ret < 0) {
+			debug("Probing %s failed (%d)\n", dev->name, ret);
+			ret = fsl_unbind_device(blk);
+			if (ret)
+				return ret;
+
+			failed_number++;
+			continue;
+		}
 	}
 
 	if (failed_number == nr_ports)
diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index 003222d47be6..a187796dfcdf 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -1099,6 +1099,11 @@ static int sata_mv_probe(struct udevice *dev)
 			continue;
 		}
 
+		ret = blk_probe_or_unbind(dev);
+		if (ret < 0)
+			/* TODO: undo create */
+			continue;
+
 		/* If we got here, the current SATA port was probed
 		 * successfully, so set the probe status to successful.
 		 */
diff --git a/drivers/ata/sata_sil.c b/drivers/ata/sata_sil.c
index a4f0dae4bbd1..b213ebac2fb9 100644
--- a/drivers/ata/sata_sil.c
+++ b/drivers/ata/sata_sil.c
@@ -753,6 +753,18 @@ static int sil_pci_probe(struct udevice *dev)
 			failed_number++;
 			continue;
 		}
+
+		ret = device_probe(dev);
+		if (ret < 0) {
+			debug("Probing %s failed (%d)\n", dev->name, ret);
+			ret = sil_unbind_device(blk);
+			device_unbind(dev);
+			if (ret)
+				return ret;
+
+			failed_number++;
+			continue;
+		}
 	}
 
 	if (failed_number == sata_info.maxport)
-- 
2.33.0

[PATCH v3 06/19] block: ide: call device_probe() after scanning

[AKASHI Takahiro]

Every time an ide bus/port is scanned and a new device is detected,
we want to call device_probe() as it will give us a chance to run
additional post-processings for some purposes.

In particular, support for creating partitions on a device will be added.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 drivers/block/ide.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/block/ide.c b/drivers/block/ide.c
index 63c4cfdc1c21..e8518ff3a11a 100644
--- a/drivers/block/ide.c
+++ b/drivers/block/ide.c
@@ -1123,6 +1123,10 @@ static int ide_probe(struct udevice *udev)
 						 blksz, size, &blk_dev);
 			if (ret)
 				return ret;
+
+			ret = blk_probe_or_unbind(blk_dev);
+			if (ret)
+				return ret;
 		}
 	}
 
-- 
2.33.0

[PATCH v3 07/19] virtio: call device_probe() in scanning

[AKASHI Takahiro]

virtio_init() enumerates all the peripherals that are to be materialised
with udevices(UCLASS_VIRIO) and creates particular device instances
(UCLASS_BlK or whatever else) as children.
On the other hand, device_probe() won't be invoked against those resultant
udevices unlike other ordinary device drivers do in the driver model.

This is particularly inconvenient when we want to add "event notification"
callback so that we will be able to automatically create all efi_disk
objects in a later patch.

With this patch applied, "virtio scan" will work in a similar way
to "scsi rescan", "usb start" or others in term of 'probe' semantics.

I didn't add this change to virtio_init() itself because this function
may be called in board_init_r() (indirectly in board_late_init())
before UEFI subsustem is initialized.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 cmd/virtio.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/cmd/virtio.c b/cmd/virtio.c
index 3dace5344f7e..ea3ed2e631e4 100644
--- a/cmd/virtio.c
+++ b/cmd/virtio.c
@@ -17,8 +17,25 @@ static int do_virtio(struct cmd_tbl *cmdtp, int flag, int argc,
 		     char *const argv[])
 {
 	if (argc == 2 && !strcmp(argv[1], "scan")) {
-		/* make sure all virtio devices are enumerated */
-		virtio_init();
+		/*
+		 * make sure all virtio devices are enumerated.
+		 * Do the same as virtio_init(), but also call
+		 * device_probe() for children (i.e. virtio devices)
+		 */
+		struct udevice *bus, *child;
+		int ret;
+
+		ret = uclass_first_device(UCLASS_VIRTIO, &bus);
+		if (ret)
+			return CMD_RET_FAILURE;
+
+		while (bus) {
+			device_foreach_child_probe(child, bus)
+				;
+			ret = uclass_next_device(&bus);
+			if (ret)
+				break;
+		}
 
 		return CMD_RET_SUCCESS;
 	}
-- 
2.33.0

[PATCH v3 09/19] dm: tag: add some document

[AKASHI Takahiro]

Some basic stuff about tag support is explained under
doc/devlop/driver-model.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 doc/develop/driver-model/design.rst | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/doc/develop/driver-model/design.rst b/doc/develop/driver-model/design.rst
index b0e6337030a1..3e88dc40e6fd 100644
--- a/doc/develop/driver-model/design.rst
+++ b/doc/develop/driver-model/design.rst
@@ -1042,6 +1042,26 @@ data structure might be worthwhile in some rare cases, once we understand
 what the bottlenecks are.
 
 
+Tag Support
+-----------
+
+It is sometimes useful for a subsystem to associate its own private
+data (or object) to a DM device, i.e. struct udevice, to support
+additional features.
+
+Tag support in driver model will give us the ability to do so dynamically
+instead of modifying "udevice" data structure. In the initial release, we
+will support two type of attributes:
+- a pointer with dm_tag_set_ptr(), and
+- an unsigned long with dm_tag_set_val()
+
+For example, UEFI subsystem utilizes the feature to maintain efi_disk
+objects depending on linked udevice's lifecycle.
+
+While the current implementation is quite simple, it will get evolved
+as the feature is more extensively used in U-Boot subsystems.
+
+
 Changes since v1
 ----------------
 
-- 
2.33.0

[PATCH v3 10/19] test: dm: add tests for tag support

[AKASHI Takahiro]

The new test covers all tag-related interfaces.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 test/dm/Makefile |  1 +
 test/dm/tag.c    | 84 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 85 insertions(+)
 create mode 100644 test/dm/tag.c

diff --git a/test/dm/Makefile b/test/dm/Makefile
index d46552fbf320..dc3177dbb7f4 100644
--- a/test/dm/Makefile
+++ b/test/dm/Makefile
@@ -102,6 +102,7 @@ obj-y += syscon.o
 obj-$(CONFIG_RESET_SYSCON) += syscon-reset.o
 obj-$(CONFIG_SYSINFO) += sysinfo.o
 obj-$(CONFIG_SYSINFO_GPIO) += sysinfo-gpio.o
+obj-$(CONFIG_UT_DM) += tag.o
 obj-$(CONFIG_TEE) += tee.o
 obj-$(CONFIG_TIMER) += timer.o
 obj-$(CONFIG_DM_USB) += usb.o
diff --git a/test/dm/tag.c b/test/dm/tag.c
new file mode 100644
index 000000000000..8289954e7c26
--- /dev/null
+++ b/test/dm/tag.c
@@ -0,0 +1,84 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *  DM tag test
+ *
+ *  Copyright (c) 2021 Linaro Limited
+ *                      Author: AKASHI Takahiro
+ */
+
+#include <common.h>
+#include <dm/tag.h>
+#include <dm/test.h> /* DM_TEST() */
+#include <test/test.h> /* struct unit_test_state */
+#include <test/ut.h> /* assertions */
+
+/*
+ * Test dm_tag_ptr() API
+ */
+static int dm_test_tag_ptr(struct unit_test_state *uts)
+{
+	ulong val;
+	void *ptr = NULL;
+
+	ut_assertok(dev_tag_set_ptr(uts->root, DM_TAG_EFI, &val));
+
+	ut_assertok(dev_tag_get_ptr(uts->root, DM_TAG_EFI, &ptr));
+
+	ut_asserteq_ptr(&val, ptr);
+
+	ut_assertok(dev_tag_del(uts->root, DM_TAG_EFI));
+
+	return 0;
+}
+
+DM_TEST(dm_test_tag_ptr, 0);
+
+/*
+ * Test dm_tag_val() API
+ */
+static int dm_test_tag_val(struct unit_test_state *uts)
+{
+	ulong val1 = 0x12345678, val2 = 0;
+
+	ut_assertok(dev_tag_set_val(uts->root, DM_TAG_EFI, val1));
+
+	ut_assertok(dev_tag_get_val(uts->root, DM_TAG_EFI, &val2));
+
+	ut_asserteq_64(val1, val2);
+
+	ut_assertok(dev_tag_del(uts->root, DM_TAG_EFI));
+
+	return 0;
+}
+
+DM_TEST(dm_test_tag_val, 0);
+
+/*
+ * Test against an invalid tag
+ */
+static int dm_test_tag_inval(struct unit_test_state *uts)
+{
+	ulong val;
+
+	ut_asserteq(-EINVAL, dev_tag_set_ptr(uts->root, DM_TAG_COUNT, &val));
+
+	return 0;
+}
+
+DM_TEST(dm_test_tag_inval, 0);
+
+/*
+ * Test dm_tag_del_all() AP:
+ */
+static int dm_test_tag_del_all(struct unit_test_state *uts)
+{
+	ulong val;
+
+	ut_assertok(dev_tag_set_ptr(uts->root, DM_TAG_EFI, &val));
+
+	ut_assertok(dev_tag_del_all(uts->root));
+
+	return 0;
+}
+
+DM_TEST(dm_test_tag_del_all, 0);
-- 
2.33.0

[PATCH v3 11/19] dm: disk: add UCLASS_PARTITION

[AKASHI Takahiro]

With this new function, UCLASS_PARTITION devices will be created as
child nodes of UCLASS_BLK device.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 disk/Makefile          |   3 +
 disk/disk-uclass.c     | 153 +++++++++++++++++++++++++++++++++++++++++
 include/dm/uclass-id.h |   1 +
 include/part.h         |  11 +++
 4 files changed, 168 insertions(+)
 create mode 100644 disk/disk-uclass.c

diff --git a/disk/Makefile b/disk/Makefile
index 6ce5a687b36c..ec37b74f5f40 100644
--- a/disk/Makefile
+++ b/disk/Makefile
@@ -6,6 +6,9 @@
 #ccflags-y += -DET_DEBUG -DDEBUG
 
 obj-$(CONFIG_PARTITIONS)	+= part.o
+ifdef CONFIG_$(SPL_)BLK
+obj-$(CONFIG_PARTITIONS) 	+= disk-uclass.o
+endif
 obj-$(CONFIG_$(SPL_)MAC_PARTITION)   += part_mac.o
 obj-$(CONFIG_$(SPL_)DOS_PARTITION)   += part_dos.o
 obj-$(CONFIG_$(SPL_)ISO_PARTITION)   += part_iso.o
diff --git a/disk/disk-uclass.c b/disk/disk-uclass.c
new file mode 100644
index 000000000000..4918a2f72d1e
--- /dev/null
+++ b/disk/disk-uclass.c
@@ -0,0 +1,153 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *  Software partition device (UCLASS_PARTITION)
+ *
+ *  Copyright (c) 2021 Linaro Limited
+ *			Author: AKASHI Takahiro
+ */
+
+#define LOG_CATEGORY UCLASS_PARTITION
+
+#include <blk.h>
+#include <dm.h>
+#include <log.h>
+#include <part.h>
+#include <vsprintf.h>
+#include <dm/device-internal.h>
+#include <dm/lists.h>
+
+int part_create_block_devices(struct udevice *blk_dev)
+{
+	int part, count;
+	struct blk_desc *desc = dev_get_uclass_plat(blk_dev);
+	struct disk_partition info;
+	struct disk_part *part_data;
+	char devname[32];
+	struct udevice *dev;
+	int ret;
+
+	if (!CONFIG_IS_ENABLED(PARTITIONS) ||
+	    !CONFIG_IS_ENABLED(HAVE_BLOCK_DEVICE))
+		return 0;
+
+	if (device_get_uclass_id(blk_dev) != UCLASS_BLK)
+		return 0;
+
+	/* Add devices for each partition */
+	for (count = 0, part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
+		if (part_get_info(desc, part, &info))
+			continue;
+		snprintf(devname, sizeof(devname), "%s:%d", blk_dev->name,
+			 part);
+
+		ret = device_bind_driver(blk_dev, "blk_partition",
+					 strdup(devname), &dev);
+		if (ret)
+			return ret;
+
+		part_data = dev_get_uclass_plat(dev);
+		part_data->partnum = part;
+		part_data->gpt_part_info = info;
+		count++;
+
+		ret = device_probe(dev);
+		if (ret) {
+			debug("Can't probe\n");
+			count--;
+			device_unbind(dev);
+
+			continue;
+		}
+	}
+	debug("%s: %d partitions found in %s\n", __func__, count,
+	      blk_dev->name);
+
+	return 0;
+}
+
+static ulong blk_part_read(struct udevice *dev, lbaint_t start,
+			   lbaint_t blkcnt, void *buffer)
+{
+	struct udevice *parent;
+	struct disk_part *part;
+	const struct blk_ops *ops;
+
+	parent = dev_get_parent(dev);
+	ops = blk_get_ops(parent);
+	if (!ops->read)
+		return -ENOSYS;
+
+	part = dev_get_uclass_plat(dev);
+	if (start >= part->gpt_part_info.size)
+		return 0;
+
+	if ((start + blkcnt) > part->gpt_part_info.size)
+		blkcnt = part->gpt_part_info.size - start;
+	start += part->gpt_part_info.start;
+
+	return ops->read(parent, start, blkcnt, buffer);
+}
+
+static ulong blk_part_write(struct udevice *dev, lbaint_t start,
+			    lbaint_t blkcnt, const void *buffer)
+{
+	struct udevice *parent;
+	struct disk_part *part;
+	const struct blk_ops *ops;
+
+	parent = dev_get_parent(dev);
+	ops = blk_get_ops(parent);
+	if (!ops->write)
+		return -ENOSYS;
+
+	part = dev_get_uclass_plat(dev);
+	if (start >= part->gpt_part_info.size)
+		return 0;
+
+	if ((start + blkcnt) > part->gpt_part_info.size)
+		blkcnt = part->gpt_part_info.size - start;
+	start += part->gpt_part_info.start;
+
+	return ops->write(parent, start, blkcnt, buffer);
+}
+
+static ulong blk_part_erase(struct udevice *dev, lbaint_t start,
+			    lbaint_t blkcnt)
+{
+	struct udevice *parent;
+	struct disk_part *part;
+	const struct blk_ops *ops;
+
+	parent = dev_get_parent(dev);
+	ops = blk_get_ops(parent);
+	if (!ops->erase)
+		return -ENOSYS;
+
+	part = dev_get_uclass_plat(dev);
+	if (start >= part->gpt_part_info.size)
+		return 0;
+
+	if ((start + blkcnt) > part->gpt_part_info.size)
+		blkcnt = part->gpt_part_info.size - start;
+	start += part->gpt_part_info.start;
+
+	return ops->erase(parent, start, blkcnt);
+}
+
+static const struct blk_ops blk_part_ops = {
+	.read	= blk_part_read,
+	.write	= blk_part_write,
+	.erase	= blk_part_erase,
+};
+
+U_BOOT_DRIVER(blk_partition) = {
+	.name		= "blk_partition",
+	.id		= UCLASS_PARTITION,
+	.ops		= &blk_part_ops,
+};
+
+UCLASS_DRIVER(partition) = {
+	.id		= UCLASS_PARTITION,
+	.per_device_plat_auto	= sizeof(struct disk_part),
+	.name		= "partition",
+};
diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
index 0e26e1d13824..230b1ea528cf 100644
--- a/include/dm/uclass-id.h
+++ b/include/dm/uclass-id.h
@@ -83,6 +83,7 @@ enum uclass_id {
 	UCLASS_P2SB,		/* (x86) Primary-to-Sideband Bus */
 	UCLASS_PANEL,		/* Display panel, such as an LCD */
 	UCLASS_PANEL_BACKLIGHT,	/* Backlight controller for panel */
+	UCLASS_PARTITION,	/* Logical disk partition device */
 	UCLASS_PCH,		/* x86 platform controller hub */
 	UCLASS_PCI,		/* PCI bus */
 	UCLASS_PCI_EP,		/* PCI endpoint device */
diff --git a/include/part.h b/include/part.h
index 53cfbdd87671..95e30e60af10 100644
--- a/include/part.h
+++ b/include/part.h
@@ -253,6 +253,17 @@ void part_set_generic_name(const struct blk_desc *dev_desc,
 	int part_num, char *name);
 
 extern const struct block_drvr block_drvr[];
+
+struct udevice;
+/**
+ * part_create_block_devices - Create block devices for disk partitions
+ *
+ * Create UCLASS_PARTITION udevices for each of disk partitions in @parent
+ *
+ * @blk_dev:	Whole disk device
+ */
+int part_create_block_devices(struct udevice *blk_dev);
+
 #else
 static inline struct blk_desc *blk_get_dev(const char *ifname, int dev)
 { return NULL; }
-- 
2.33.0

[PATCH v3 12/19] dm: blk: add a device-probe hook for scanning disk partitions

[AKASHI Takahiro]

Now that all the block device drivers have enable a probe hook, we will
call part_create_block_devices() to enumerate all the partitions and
create associated udevices when a block device is detected.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 drivers/block/blk-uclass.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
index bee1cd6f0d80..58dc74e71f1e 100644
--- a/drivers/block/blk-uclass.c
+++ b/drivers/block/blk-uclass.c
@@ -717,6 +717,10 @@ static int blk_post_probe(struct udevice *dev)
 		struct blk_desc *desc = dev_get_uclass_plat(dev);
 
 		part_init(desc);
+
+		if (desc->part_type != PART_TYPE_UNKNOWN &&
+		    part_create_block_devices(dev))
+			debug("*** creating partitions failed\n");
 	}
 
 	return 0;
-- 
2.33.0

[PATCH v3 14/19] efi_loader: disk: a helper function to create efi_disk objects from udevice

[AKASHI Takahiro]

Add efi_disk_probe() function.
This function creates an efi_disk object for a raw disk device (UCLASS_BLK)
and additional objects for related partitions (UCLASS_PARTITION).

So this function is expected to be called through driver model's "probe"
interface every time one raw disk device is detected and activated.
We assume that partition devices (UCLASS_PARTITION) have been created
when this function is invoked.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 include/efi_loader.h       |   4 +-
 lib/efi_loader/Kconfig     |   3 +
 lib/efi_loader/efi_disk.c  | 201 +++++++++++++++++++++++--------------
 lib/efi_loader/efi_setup.c |   4 +-
 4 files changed, 133 insertions(+), 79 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index a9075c992687..24ec692c4d09 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -523,8 +523,8 @@ void efi_carve_out_dt_rsv(void *fdt);
 void efi_try_purge_kaslr_seed(void *fdt);
 /* Called by bootefi to make console interface available */
 efi_status_t efi_console_register(void);
-/* Called by bootefi to make all disk storage accessible as EFI objects */
-efi_status_t efi_disk_register(void);
+/* Called by efi_init_obj_list() to initialize efi_disks */
+efi_status_t efi_disk_init(void);
 /* Called by efi_init_obj_list() to install EFI_RNG_PROTOCOL */
 efi_status_t efi_rng_register(void);
 /* Called by efi_init_obj_list() to install EFI_TCG2_PROTOCOL */
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index e5e35fe51f65..7ffd59759359 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -14,6 +14,8 @@ config EFI_LOADER
 	depends on DM_ETH || !NET
 	depends on !EFI_APP
 	default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8
+	select DM_EVENT
+	select EVENT_DYNAMIC
 	select LIB_UUID
 	select PARTITION_UUIDS
 	select HAVE_BLOCK_DEVICE
@@ -41,6 +43,7 @@ config CMD_BOOTEFI_BOOTMGR
 
 config EFI_SETUP_EARLY
 	bool
+	default y
 
 choice
 	prompt "Store for non-volatile UEFI variables"
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index 45127d176869..bf9824b9b1a4 100644
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c
@@ -10,6 +10,9 @@
 #include <common.h>
 #include <blk.h>
 #include <dm.h>
+#include <dm/device-internal.h>
+#include <dm/tag.h>
+#include <event.h>
 #include <efi_loader.h>
 #include <fs.h>
 #include <log.h>
@@ -487,103 +490,153 @@ error:
 	return ret;
 }
 
-/**
- * efi_disk_create_partitions() - create handles and protocols for partitions
+/*
+ * Create a handle for a whole raw disk
+ *
+ * @dev		uclass device (UCLASS_BLK)
  *
- * Create handles and protocols for the partitions of a block device.
+ * Create an efi_disk object which is associated with @dev.
+ * The type of @dev must be UCLASS_BLK.
  *
- * @parent:		handle of the parent disk
- * @desc:		block device
- * @if_typename:	interface type
- * @diskid:		device number
- * @pdevname:		device name
- * Return:		number of partitions created
+ * @return	0 on success, -1 otherwise
  */
-int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
-			       const char *if_typename, int diskid,
-			       const char *pdevname)
+static int efi_disk_create_raw(struct udevice *dev)
 {
-	int disks = 0;
-	char devname[32] = { 0 }; /* dp->str is u16[32] long */
-	int part;
-	struct efi_device_path *dp = NULL;
+	struct efi_disk_obj *disk;
+	struct blk_desc *desc;
+	const char *if_typename;
+	int diskid;
 	efi_status_t ret;
-	struct efi_handler *handler;
 
-	/* Get the device path of the parent */
-	ret = efi_search_protocol(parent, &efi_guid_device_path, &handler);
-	if (ret == EFI_SUCCESS)
-		dp = handler->protocol_interface;
-
-	/* Add devices for each partition */
-	for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
-		struct disk_partition info;
-
-		if (part_get_info(desc, part, &info))
-			continue;
-		snprintf(devname, sizeof(devname), "%s:%x", pdevname,
-			 part);
-		ret = efi_disk_add_dev(parent, dp, if_typename, desc, diskid,
-				       &info, part, NULL);
-		if (ret != EFI_SUCCESS) {
-			log_err("Adding partition %s failed\n", pdevname);
-			continue;
-		}
-		disks++;
+	desc = dev_get_uclass_plat(dev);
+	if_typename = blk_get_if_type_name(desc->if_type);
+	diskid = desc->devnum;
+
+	ret = efi_disk_add_dev(NULL, NULL, if_typename, desc,
+			       diskid, NULL, 0, &disk);
+	if (ret != EFI_SUCCESS) {
+		if (ret == EFI_NOT_READY)
+			log_notice("Disk %s not ready\n", dev->name);
+		else
+			log_err("Adding disk for %s failed\n", dev->name);
+
+		return -1;
+	}
+	if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) {
+		efi_free_pool(disk->dp);
+		efi_delete_handle(&disk->header);
+
+		return -1;
 	}
 
-	return disks;
+	return 0;
 }
 
-/**
- * efi_disk_register() - register block devices
- *
- * U-Boot doesn't have a list of all online disk devices. So when running our
- * EFI payload, we scan through all of the potentially available ones and
- * store them in our object pool.
+/*
+ * Create a handle for a disk partition
  *
- * This function is called in efi_init_obj_list().
+ * @dev		uclass device (UCLASS_PARTITION)
  *
- * TODO(sjg@chromium.org): Actually with CONFIG_BLK, U-Boot does have this.
- * Consider converting the code to look up devices as needed. The EFI device
- * could be a child of the UCLASS_BLK block device, perhaps.
+ * Create an efi_disk object which is associated with @dev.
+ * The type of @dev must be UCLASS_PARTITION.
  *
- * Return:	status code
+ * @return	0 on success, -1 otherwise
  */
-efi_status_t efi_disk_register(void)
+static int efi_disk_create_part(struct udevice *dev)
 {
+	efi_handle_t parent;
+	struct blk_desc *desc;
+	const char *if_typename;
+	struct disk_part *part_data;
+	struct disk_partition *info;
+	unsigned int part;
+	int diskid;
+	struct efi_handler *handler;
+	struct efi_device_path *dp_parent;
 	struct efi_disk_obj *disk;
-	int disks = 0;
 	efi_status_t ret;
+
+	if (dev_tag_get_ptr(dev_get_parent(dev), DM_TAG_EFI, (void **)&parent))
+		return -1;
+
+	desc = dev_get_uclass_plat(dev_get_parent(dev));
+	if_typename = blk_get_if_type_name(desc->if_type);
+	diskid = desc->devnum;
+
+	part_data = dev_get_uclass_plat(dev);
+	part = part_data->partnum;
+	info = &part_data->gpt_part_info;
+
+	ret = efi_search_protocol(parent, &efi_guid_device_path, &handler);
+	if (ret != EFI_SUCCESS)
+		return -1;
+	dp_parent = (struct efi_device_path *)handler->protocol_interface;
+
+	ret = efi_disk_add_dev(parent, dp_parent, if_typename, desc, diskid,
+			       info, part, &disk);
+	if (ret != EFI_SUCCESS) {
+		log_err("Adding partition for %s failed\n", dev->name);
+		return -1;
+	}
+	if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) {
+		efi_free_pool(disk->dp);
+		efi_delete_handle(&disk->header);
+
+		return -1;
+	}
+
+	return 0;
+}
+
+/*
+ * Create efi_disk objects for a block device
+ *
+ * @dev		uclass device (UCLASS_BLK)
+ *
+ * Create efi_disk objects for partitions as well as a raw disk
+ * which is associated with @dev.
+ * The type of @dev must be UCLASS_BLK.
+ * This function is expected to be called at EV_PM_POST_PROBE.
+ *
+ * @return	0 on success, -1 otherwise
+ */
+static int efi_disk_probe(void *ctx, struct event *event)
+{
 	struct udevice *dev;
+	enum uclass_id id;
+	struct udevice *child;
+	int ret;
 
-	for (uclass_first_device_check(UCLASS_BLK, &dev); dev;
-	     uclass_next_device_check(&dev)) {
-		struct blk_desc *desc = dev_get_uclass_plat(dev);
-		const char *if_typename = blk_get_if_type_name(desc->if_type);
+	dev = event->data.dm.dev;
+	id = device_get_uclass_id(dev);
 
-		/* Add block device for the full device */
-		log_info("Scanning disk %s...\n", dev->name);
-		ret = efi_disk_add_dev(NULL, NULL, if_typename,
-					desc, desc->devnum, NULL, 0, &disk);
-		if (ret == EFI_NOT_READY) {
-			log_notice("Disk %s not ready\n", dev->name);
-			continue;
-		}
-		if (ret) {
-			log_err("ERROR: failure to add disk device %s, r = %lu\n",
-				dev->name, ret & ~EFI_ERROR_MASK);
-			continue;
-		}
-		disks++;
+	/* TODO: We won't support partitions in a partition */
+	if (id != UCLASS_BLK)
+		return 0;
+
+	ret = efi_disk_create_raw(dev);
+	if (ret)
+		return -1;
 
-		/* Partitions show up as block devices in EFI */
-		disks += efi_disk_create_partitions(
-					&disk->header, desc, if_typename,
-					desc->devnum, dev->name);
+	device_foreach_child(child, dev) {
+		ret = efi_disk_create_part(child);
+		if (ret)
+			return -1;
 	}
 
-	log_info("Found %d disks\n", disks);
+	return 0;
+}
+
+efi_status_t efi_disk_init(void)
+{
+	int ret;
+
+	ret = event_register("efi_disk add", EVT_DM_POST_PROBE,
+			     efi_disk_probe, NULL);
+	if (ret) {
+		log_err("Event registration for efi_disk add failed\n");
+		return EFI_OUT_OF_RESOURCES;
+	}
 
 	return EFI_SUCCESS;
 }
diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
index de2f34bab537..250eeb2fcd6b 100644
--- a/lib/efi_loader/efi_setup.c
+++ b/lib/efi_loader/efi_setup.c
@@ -198,9 +198,7 @@ static efi_status_t __efi_init_early(void)
 	if (ret != EFI_SUCCESS)
 		goto out;
 
-#ifdef CONFIG_PARTITIONS
-	ret = efi_disk_register();
-#endif
+	ret = efi_disk_init();
 out:
 	return ret;
 }
-- 
2.33.0

[PATCH v3 15/19] efi_loader: disk: a helper function to delete efi_disk objects

[AKASHI Takahiro]

This function is expected to be called, in particular from dm's pre_remove
hook, when associated block devices no longer exist.

Add efi_disk_remove() function.
This function removes an efi_disk object for a raw disk device (UCLASS_BLK)
and related objects for its partitions (UCLASS_PARTITION).

So this function is expected to be called through driver model's "remove"
interface every time a raw disk device is to be disconnected.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 lib/efi_loader/efi_disk.c | 88 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)

diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index bf9824b9b1a4..53bee6654421 100644
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c
@@ -627,6 +627,87 @@ static int efi_disk_probe(void *ctx, struct event *event)
 	return 0;
 }
 
+/*
+ * Delete an efi_disk object for a whole raw disk
+ *
+ * @dev		uclass device (UCLASS_BLK)
+ *
+ * Delete an efi_disk object which is associated with @dev.
+ * The type of @dev must be UCLASS_BLK.
+ *
+ * @return	0 on success, -1 otherwise
+ */
+static int efi_disk_delete_raw(struct udevice *dev)
+{
+	efi_handle_t handle;
+	struct efi_disk_obj *diskobj;
+
+	if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle))
+		return -1;
+
+	diskobj = container_of(handle, struct efi_disk_obj, header);
+	efi_free_pool(diskobj->dp);
+
+	efi_delete_handle(handle);
+	dev_tag_del(dev, DM_TAG_EFI);
+
+	return 0;
+}
+
+/*
+ * Delete an efi_disk object for a disk partition
+ *
+ * @dev		uclass device (UCLASS_PARTITION)
+ *
+ * Delete an efi_disk object which is associated with @dev.
+ * The type of @dev must be UCLASS_PARTITION.
+ *
+ * @return	0 on success, -1 otherwise
+ */
+static int efi_disk_delete_part(struct udevice *dev)
+{
+	efi_handle_t handle;
+	struct efi_disk_obj *diskobj;
+
+	if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle))
+		return -1;
+
+	diskobj = container_of(handle, struct efi_disk_obj, header);
+
+	efi_free_pool(diskobj->dp);
+	efi_delete_handle(handle);
+	dev_tag_del(dev, DM_TAG_EFI);
+
+	return 0;
+}
+
+/*
+ * Delete an efi_disk object for a block device
+ *
+ * @dev		uclass device (UCLASS_BLK or UCLASS_PARTITION)
+ *
+ * Delete an efi_disk object which is associated with @dev.
+ * The type of @dev must be either UCLASS_BLK or UCLASS_PARTITION.
+ * This function is expected to be called at EV_PM_PRE_REMOVE.
+ *
+ * @return	0 on success, -1 otherwise
+ */
+static int efi_disk_remove(void *ctx, struct event *event)
+{
+	enum uclass_id id;
+	struct udevice *dev;
+
+	dev = event->data.dm.dev;
+	id = device_get_uclass_id(dev);
+
+	if (id == UCLASS_BLK)
+		return efi_disk_delete_raw(dev);
+	else if (id == UCLASS_PARTITION)
+		return efi_disk_delete_part(dev);
+	else
+		return 0;
+}
+
 efi_status_t efi_disk_init(void)
 {
 	int ret;
@@ -638,6 +719,13 @@ efi_status_t efi_disk_init(void)
 		return EFI_OUT_OF_RESOURCES;
 	}
 
+	ret = event_register("efi_disk del", EVT_DM_PRE_REMOVE,
+			     efi_disk_remove, NULL);
+	if (ret) {
+		log_err("Event registration for efi_disk del failed\n");
+		return EFI_OUT_OF_RESOURCES;
+	}
+
 	return EFI_SUCCESS;
 }
 
-- 
2.33.0

[PATCH v3 16/19] dm: disk: add read/write interfaces with udevice

[AKASHI Takahiro]

In include/blk.h, Simon suggested:
===>
/*
 * These functions should take struct udevice instead of struct blk_desc,
 * but this is convenient for migration to driver model. Add a 'd' prefix
 * to the function operations, so that blk_read(), etc. can be reserved for
 * functions with the correct arguments.
 */
unsigned long blk_dread(struct blk_desc *block_dev, lbaint_t start,
                        lbaint_t blkcnt, void *buffer);
unsigned long blk_dwrite(struct blk_desc *block_dev, lbaint_t start,
                         lbaint_t blkcnt, const void *buffer);
unsigned long blk_derase(struct blk_desc *block_dev, lbaint_t start,
                         lbaint_t blkcnt);
<===

So new interfaces are provided with this patch.

They are expected to be used everywhere in U-Boot at the end.
The exceptions are block device drivers, partition drivers and efi_disk
which should know details of blk_desc structure.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 disk/disk-uclass.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++
 include/part.h     |  7 ++++
 2 files changed, 101 insertions(+)

diff --git a/disk/disk-uclass.c b/disk/disk-uclass.c
index 4918a2f72d1e..72ff62ebf581 100644
--- a/disk/disk-uclass.c
+++ b/disk/disk-uclass.c
@@ -146,6 +146,100 @@ U_BOOT_DRIVER(blk_partition) = {
 	.ops		= &blk_part_ops,
 };
 
+/*
+ * BLOCK IO APIs
+ */
+static struct blk_desc *dev_get_blk(struct udevice *dev)
+{
+	struct blk_desc *block_dev;
+
+	switch (device_get_uclass_id(dev)) {
+	/*
+	 * We won't support UCLASS_BLK with dev_* interfaces.
+	 */
+	case UCLASS_PARTITION:
+		block_dev = dev_get_uclass_plat(dev_get_parent(dev));
+		break;
+	default:
+		block_dev = NULL;
+		break;
+	}
+
+	return block_dev;
+}
+
+unsigned long dev_read(struct udevice *dev, lbaint_t start,
+		       lbaint_t blkcnt, void *buffer)
+{
+	struct blk_desc *block_dev;
+	const struct blk_ops *ops;
+	struct disk_part *part;
+	lbaint_t start_in_disk;
+	ulong blks_read;
+
+	block_dev = dev_get_blk(dev);
+	if (!block_dev)
+		return -ENOSYS;
+
+	ops = blk_get_ops(dev);
+	if (!ops->read)
+		return -ENOSYS;
+
+	start_in_disk = start;
+	if (device_get_uclass_id(dev) == UCLASS_PARTITION) {
+		part = dev_get_uclass_plat(dev);
+		start_in_disk += part->gpt_part_info.start;
+	}
+
+	if (blkcache_read(block_dev->if_type, block_dev->devnum,
+			  start_in_disk, blkcnt, block_dev->blksz, buffer))
+		return blkcnt;
+	blks_read = ops->read(dev, start, blkcnt, buffer);
+	if (blks_read == blkcnt)
+		blkcache_fill(block_dev->if_type, block_dev->devnum,
+			      start_in_disk, blkcnt, block_dev->blksz, buffer);
+
+	return blks_read;
+}
+
+unsigned long dev_write(struct udevice *dev, lbaint_t start,
+			lbaint_t blkcnt, const void *buffer)
+{
+	struct blk_desc *block_dev;
+	const struct blk_ops *ops;
+
+	block_dev = dev_get_blk(dev);
+	if (!block_dev)
+		return -ENOSYS;
+
+	ops = blk_get_ops(dev);
+	if (!ops->write)
+		return -ENOSYS;
+
+	blkcache_invalidate(block_dev->if_type, block_dev->devnum);
+
+	return ops->write(dev, start, blkcnt, buffer);
+}
+
+unsigned long dev_erase(struct udevice *dev, lbaint_t start,
+			lbaint_t blkcnt)
+{
+	struct blk_desc *block_dev;
+	const struct blk_ops *ops;
+
+	block_dev = dev_get_blk(dev);
+	if (!block_dev)
+		return -ENOSYS;
+
+	ops = blk_get_ops(dev);
+	if (!ops->erase)
+		return -ENOSYS;
+
+	blkcache_invalidate(block_dev->if_type, block_dev->devnum);
+
+	return ops->erase(dev, start, blkcnt);
+}
+
 UCLASS_DRIVER(partition) = {
 	.id		= UCLASS_PARTITION,
 	.per_device_plat_auto	= sizeof(struct disk_part),
diff --git a/include/part.h b/include/part.h
index 95e30e60af10..d4e5cd921db1 100644
--- a/include/part.h
+++ b/include/part.h
@@ -264,6 +264,13 @@ struct udevice;
  */
 int part_create_block_devices(struct udevice *blk_dev);
 
+unsigned long dev_read(struct udevice *dev, lbaint_t start,
+		       lbaint_t blkcnt, void *buffer);
+unsigned long dev_write(struct udevice *dev, lbaint_t start,
+			lbaint_t blkcnt, const void *buffer);
+unsigned long dev_erase(struct udevice *dev, lbaint_t start,
+			lbaint_t blkcnt);
+
 #else
 static inline struct blk_desc *blk_get_dev(const char *ifname, int dev)
 { return NULL; }
-- 
2.33.0

[PATCH v3 17/19] efi_loader: disk: use udevice instead of blk_desc

[AKASHI Takahiro]

In most of all cases, we can avoid using blk_desc which is expected
to be private to udevice(UCLASS_BLK), that is, the data should not
be manipulated outside the device driver unless really needed.

Now efi_disk's internally use dev_read/write() interfaces.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 lib/efi_loader/efi_disk.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index 53bee6654421..0b4f7a9ca317 100644
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c
@@ -36,7 +36,7 @@ const efi_guid_t efi_system_partition_guid = PARTITION_SYSTEM_GUID;
  * @part:	partition
  * @volume:	simple file system protocol of the partition
  * @offset:	offset into disk for simple partition
- * @desc:	internal block device descriptor
+ * @dev:	associated DM device
  */
 struct efi_disk_obj {
 	struct efi_object header;
@@ -48,7 +48,7 @@ struct efi_disk_obj {
 	unsigned int part;
 	struct efi_simple_file_system_protocol *volume;
 	lbaint_t offset;
-	struct blk_desc *desc;
+	struct udevice *dev; /* TODO: move it to efi_object */
 };
 
 /**
@@ -83,14 +83,12 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this,
 			void *buffer, enum efi_disk_direction direction)
 {
 	struct efi_disk_obj *diskobj;
-	struct blk_desc *desc;
 	int blksz;
 	int blocks;
 	unsigned long n;
 
 	diskobj = container_of(this, struct efi_disk_obj, ops);
-	desc = (struct blk_desc *) diskobj->desc;
-	blksz = desc->blksz;
+	blksz = diskobj->media.block_size;
 	blocks = buffer_size / blksz;
 	lba += diskobj->offset;
 
@@ -102,9 +100,9 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this,
 		return EFI_BAD_BUFFER_SIZE;
 
 	if (direction == EFI_DISK_READ)
-		n = blk_dread(desc, lba, blocks, buffer);
+		n = dev_read(diskobj->dev, lba, blocks, buffer);
 	else
-		n = blk_dwrite(desc, lba, blocks, buffer);
+		n = dev_write(diskobj->dev, lba, blocks, buffer);
 
 	/* We don't do interrupts, so check for timers cooperatively */
 	efi_timer_check();
@@ -446,7 +444,6 @@ static efi_status_t efi_disk_add_dev(
 	diskobj->ops = block_io_disk_template;
 	diskobj->ifname = if_typename;
 	diskobj->dev_index = dev_index;
-	diskobj->desc = desc;
 
 	/* Fill in EFI IO Media info (for read/write callbacks) */
 	diskobj->media.removable_media = desc->removable;
@@ -522,6 +519,7 @@ static int efi_disk_create_raw(struct udevice *dev)
 
 		return -1;
 	}
+	disk->dev = dev;
 	if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) {
 		efi_free_pool(disk->dp);
 		efi_delete_handle(&disk->header);
@@ -578,6 +576,7 @@ static int efi_disk_create_part(struct udevice *dev)
 		log_err("Adding partition for %s failed\n", dev->name);
 		return -1;
 	}
+	disk->dev = dev;
 	if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) {
 		efi_free_pool(disk->dp);
 		efi_delete_handle(&disk->header);
@@ -740,20 +739,22 @@ bool efi_disk_is_system_part(efi_handle_t handle)
 {
 	struct efi_handler *handler;
 	struct efi_disk_obj *diskobj;
-	struct disk_partition info;
+	struct udevice *dev;
+	struct disk_part *part;
 	efi_status_t ret;
-	int r;
 
 	/* check if this is a block device */
 	ret = efi_search_protocol(handle, &efi_block_io_guid, &handler);
 	if (ret != EFI_SUCCESS)
 		return false;
 
+	/* find a partition udevice */
 	diskobj = container_of(handle, struct efi_disk_obj, header);
-
-	r = part_get_info(diskobj->desc, diskobj->part, &info);
-	if (r)
+	dev = diskobj->dev;
+	if (!dev || dev->driver->id != UCLASS_PARTITION)
 		return false;
 
-	return !!(info.bootable & PART_EFI_SYSTEM_PARTITION);
+	part = dev_get_uclass_plat(dev);
+
+	return !!(part->gpt_part_info.bootable & PART_EFI_SYSTEM_PARTITION);
 }
-- 
2.33.0

[PATCH v3 18/19] efi_loader: disk: not create BLK device for BLK(IF_TYPE_EFI_LOADER) devices

[AKASHI Takahiro]

When we create an efi_disk device with an UEFI application using driver
binding protocol, the 'efi_driver' framework tries to create
a corresponding block device(UCLASS_BLK/IF_TYPE_EFI). This will lead to
calling a PROBE callback, efi_disk_probe().
In this case, however, we don't need to create another "efi_disk" device
as we already have this device instance.

So we should avoid recursively invoke further processing in the callback
function.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 lib/efi_loader/efi_disk.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index 0b4f7a9ca317..7c3eebf31e22 100644
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c
@@ -603,6 +603,7 @@ static int efi_disk_probe(void *ctx, struct event *event)
 {
 	struct udevice *dev;
 	enum uclass_id id;
+	struct blk_desc *desc;
 	struct udevice *child;
 	int ret;
 
@@ -613,9 +614,16 @@ static int efi_disk_probe(void *ctx, struct event *event)
 	if (id != UCLASS_BLK)
 		return 0;
 
-	ret = efi_disk_create_raw(dev);
-	if (ret)
-		return -1;
+	/*
+	 * avoid creating duplicated objects now that efi_driver
+	 * has already created an efi_disk at this moment.
+	 */
+	desc = dev_get_uclass_plat(dev);
+	if (desc->if_type != IF_TYPE_EFI_LOADER) {
+		ret = efi_disk_create_raw(dev);
+		if (ret)
+			return -1;
+	}
 
 	device_foreach_child(child, dev) {
 		ret = efi_disk_create_part(child);
@@ -639,13 +647,17 @@ static int efi_disk_probe(void *ctx, struct event *event)
 static int efi_disk_delete_raw(struct udevice *dev)
 {
 	efi_handle_t handle;
+	struct blk_desc *desc;
 	struct efi_disk_obj *diskobj;
 
 	if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle))
 		return -1;
 
-	diskobj = container_of(handle, struct efi_disk_obj, header);
-	efi_free_pool(diskobj->dp);
+	desc = dev_get_uclass_plat(dev);
+	if (desc->if_type != IF_TYPE_EFI_LOADER) {
+		diskobj = container_of(handle, struct efi_disk_obj, header);
+		efi_free_pool(diskobj->dp);
+	}
 
 	efi_delete_handle(handle);
 	dev_tag_del(dev, DM_TAG_EFI);
-- 
2.33.0

[PATCH v3 19/19] efi_driver: align with efi_disk-dm integration

[AKASHI Takahiro]

With DM-efi_disk integration, we don't need to explicitly call
efi_disk_create_partitions().

The only thing to do is to associate an efi_disk object to
the corresponding udevice as we skip most of processing in
efi_disk_probe() by the previous commit ("efi_loader: disk: not create
BLK device for BLK(IF_TYPE_EFI) devices").

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 lib/efi_driver/efi_block_device.c | 34 +++++++++----------------------
 1 file changed, 10 insertions(+), 24 deletions(-)

diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c
index 04cb3ef0d4e5..5baa6f87a375 100644
--- a/lib/efi_driver/efi_block_device.c
+++ b/lib/efi_driver/efi_block_device.c
@@ -35,6 +35,7 @@
 #include <malloc.h>
 #include <dm/device-internal.h>
 #include <dm/root.h>
+#include <dm/tag.h>
 
 /*
  * EFI attributes of the udevice handled by this driver.
@@ -106,25 +107,6 @@ static ulong efi_bl_write(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
 	return blkcnt;
 }
 
-/**
- * Create partions for the block device.
- *
- * @handle:	EFI handle of the block device
- * @dev:	udevice of the block device
- * Return:	number of partitions created
- */
-static int efi_bl_bind_partitions(efi_handle_t handle, struct udevice *dev)
-{
-	struct blk_desc *desc;
-	const char *if_typename;
-
-	desc = dev_get_uclass_plat(dev);
-	if_typename = blk_get_if_type_name(desc->if_type);
-
-	return efi_disk_create_partitions(handle, desc, if_typename,
-					  desc->devnum, dev->name);
-}
-
 /**
  * Create a block device for a handle
  *
@@ -139,7 +121,6 @@ static int efi_bl_bind(efi_handle_t handle, void *interface)
 	char *name;
 	struct efi_object *obj = efi_search_obj(handle);
 	struct efi_block_io *io = interface;
-	int disks;
 	struct efi_blk_plat *plat;
 
 	EFI_PRINT("%s: handle %p, interface %p\n", __func__, handle, io);
@@ -173,15 +154,20 @@ static int efi_bl_bind(efi_handle_t handle, void *interface)
 	plat->handle = handle;
 	plat->io = interface;
 
+	/*
+	 * FIXME: necessary because we won't do almost nothing in
+	 * efi_disk_create() when called from device_probe().
+	 */
+	ret = dev_tag_set_ptr(bdev, DM_TAG_EFI, handle);
+	if (ret)
+		/* FIXME: cleanup for bdev */
+		return ret;
+
 	ret = device_probe(bdev);
 	if (ret)
 		return ret;
 	EFI_PRINT("%s: block device '%s' created\n", __func__, bdev->name);
 
-	/* Create handles for the partions of the block device */
-	disks = efi_bl_bind_partitions(handle, bdev);
-	EFI_PRINT("Found %d partitions\n", disks);
-
 	return 0;
 }
 
-- 
2.33.0

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[Heinrich Schuchardt]

On 3/8/22 12:36, AKASHI Takahiro wrote:
> With this patch set[1] applied, UEFI subsystem maintains a list of its
> disk objects dynamically at runtime based on block device's probing.
> (See "issues" below.)
>
> [1] https://github.com/t-akashi/u-boot/tree/efi/dm_disk

On sandbox_defconfig with CONFIG_EFI_SELFTEST=y I see this error which
does not occur without your patches:

Executing 'load file protocol'
lib/efi_selftest/efi_selftest_load_file.c(220):
ERROR: Wrong remaining device path
lib/efi_selftest/efi_selftest_load_file.c(396):
ERROR: Failed to load image
lib/efi_selftest/efi_selftest.c(114):
ERROR: Executing 'load file protocol' failed

Could you, please, check.

Best regards

Heinrich

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[AKASHI Takahiro]

On Tue, Mar 08, 2022 at 01:59:08PM +0100, Heinrich Schuchardt wrote:
> On 3/8/22 12:36, AKASHI Takahiro wrote:
> > With this patch set[1] applied, UEFI subsystem maintains a list of its
> > disk objects dynamically at runtime based on block device's probing.
> > (See "issues" below.)
> > 
> > [1] https://github.com/t-akashi/u-boot/tree/efi/dm_disk
> 
> On sandbox_defconfig with CONFIG_EFI_SELFTEST=y I see this error which
> does not occur without your patches:
> 
> Executing 'load file protocol'
> lib/efi_selftest/efi_selftest_load_file.c(220):
> ERROR: Wrong remaining device path
> lib/efi_selftest/efi_selftest_load_file.c(396):
> ERROR: Failed to load image
> lib/efi_selftest/efi_selftest.c(114):
> ERROR: Executing 'load file protocol' failed
> 
> Could you, please, check.

I will, but I also request you to review *the code* itself.

-Takahiro Akashi

> Best regards
> 
> Heinrich

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[Heinrich Schuchardt]

On 3/8/22 14:04, AKASHI Takahiro wrote:
> On Tue, Mar 08, 2022 at 01:59:08PM +0100, Heinrich Schuchardt wrote:
>> On 3/8/22 12:36, AKASHI Takahiro wrote:
>>> With this patch set[1] applied, UEFI subsystem maintains a list of its
>>> disk objects dynamically at runtime based on block device's probing.
>>> (See "issues" below.)
>>>
>>> [1] https://github.com/t-akashi/u-boot/tree/efi/dm_disk
>>
>> On sandbox_defconfig with CONFIG_EFI_SELFTEST=y I see this error which
>> does not occur without your patches:
>>
>> Executing 'load file protocol'
>> lib/efi_selftest/efi_selftest_load_file.c(220):
>> ERROR: Wrong remaining device path
>> lib/efi_selftest/efi_selftest_load_file.c(396):
>> ERROR: Failed to load image
>> lib/efi_selftest/efi_selftest.c(114):
>> ERROR: Executing 'load file protocol' failed

I missed to put Simon's event series first. With both series the error
vanishes.

Best regards

Heinrich

>>
>> Could you, please, check.
>
> I will, but I also request you to review *the code* itself.
>
> -Takahiro Akashi
>
>> Best regards
>>
>> Heinrich

Re: [PATCH v3 01/19] scsi: call device_probe() after scanning

[Heinrich Schuchardt]

On 3/8/22 12:36, AKASHI Takahiro wrote:
> Every time a scsi bus/port is scanned and a new block device is detected,
> we want to call device_probe() as it will give us a chance to run
> additional post-processings for some purposes.
>
> In particular, support for creating partitions on a device will be added.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
>   drivers/scsi/scsi.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index d7b33010e469..78d729d809d7 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -595,6 +595,11 @@ static int do_scsi_scan_one(struct udevice *dev, int id, int lun, bool verbose)
>   		ata_swap_buf_le16((u16 *)&bdesc->revision, sizeof(bd.revision) / 2);
>   	}
>
> +	ret = blk_probe_or_unbind(bdev);
> +	if (ret < 0)
> +		/* TODO: undo create */

Isn't device_unbind() releasing all resources? What is missing?

Best regards

Heinrich

> +		return ret;
> +
>   	if (verbose) {
>   		printf("  Device %d: ", bdesc->devnum);
>   		dev_print(bdesc);

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[Simon Glass]

Hi,

On Tue, 8 Mar 2022 at 06:29, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 3/8/22 14:04, AKASHI Takahiro wrote:
> > On Tue, Mar 08, 2022 at 01:59:08PM +0100, Heinrich Schuchardt wrote:
> >> On 3/8/22 12:36, AKASHI Takahiro wrote:
> >>> With this patch set[1] applied, UEFI subsystem maintains a list of its
> >>> disk objects dynamically at runtime based on block device's probing.
> >>> (See "issues" below.)
> >>>
> >>> [1] https://github.com/t-akashi/u-boot/tree/efi/dm_disk
> >>
> >> On sandbox_defconfig with CONFIG_EFI_SELFTEST=y I see this error which
> >> does not occur without your patches:
> >>
> >> Executing 'load file protocol'
> >> lib/efi_selftest/efi_selftest_load_file.c(220):
> >> ERROR: Wrong remaining device path
> >> lib/efi_selftest/efi_selftest_load_file.c(396):
> >> ERROR: Failed to load image
> >> lib/efi_selftest/efi_selftest.c(114):
> >> ERROR: Executing 'load file protocol' failed
>
> I missed to put Simon's event series first. With both series the error
> vanishes.

+Tom Rini

I'm really looking forward to getting this all in! It is another one
that has been hanging around for too long.

- Simon

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[Heinrich Schuchardt]

On 3/8/22 12:36, AKASHI Takahiro wrote:
> With this patch set[1] applied, UEFI subsystem maintains a list of its
> disk objects dynamically at runtime based on block device's probing.
> (See "issues" below.)
>
> [1]https://github.com/t-akashi/u-boot/tree/efi/dm_disk

This series together with Simon's series breaks multiple boards due to
size constraints:

https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/11197

Please, investigate how to work around this issue.

Best regards

Heinrich

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[Simon Glass]

Hi,

On Tue, 8 Mar 2022 at 09:49, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 3/8/22 12:36, AKASHI Takahiro wrote:
> > With this patch set[1] applied, UEFI subsystem maintains a list of its
> > disk objects dynamically at runtime based on block device's probing.
> > (See "issues" below.)
> >
> > [1]https://github.com/t-akashi/u-boot/tree/efi/dm_disk
>
> This series together with Simon's series breaks multiple boards due to
> size constraints:
>
> https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/11197
>
> Please, investigate how to work around this issue.

tbs2910 - perhaps we should just drop this board? It doesn't use
DM_SERIAL and still uses OF_EMBED
rcar3_salvator-x - can we put the partition changes behind a config?
phycore-rk3288 - something going on in SPL, perhaps needs an
additional config to disable it?

Regards,
Simon

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[Heinrich Schuchardt]

On 3/8/22 17:56, Simon Glass wrote:
> Hi,
>
> On Tue, 8 Mar 2022 at 09:49, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 3/8/22 12:36, AKASHI Takahiro wrote:
>>> With this patch set[1] applied, UEFI subsystem maintains a list of its
>>> disk objects dynamically at runtime based on block device's probing.
>>> (See "issues" below.)
>>>
>>> [1]https://github.com/t-akashi/u-boot/tree/efi/dm_disk
>>
>> This series together with Simon's series breaks multiple boards due to
>> size constraints:
>>
>> https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/11197
>>
>> Please, investigate how to work around this issue.
>
> tbs2910 - perhaps we should just drop this board? It doesn't use
> DM_SERIAL and still uses OF_EMBED
> rcar3_salvator-x - can we put the partition changes behind a config?

This board came up before. 34f2577e926da ("ARM: renesas: reduce
rcar3_salvator-x image size"). It is generally at the size limit.

> phycore-rk3288 - something going on in SPL, perhaps needs an
> additional config to disable it?

Do we need any of your events on SPL?

Best regards

Heinrich

>
> Regards,
> Simon

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[Simon Glass]

Hi Heinrich,

On Tue, 8 Mar 2022 at 10:26, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 3/8/22 17:56, Simon Glass wrote:
> > Hi,
> >
> > On Tue, 8 Mar 2022 at 09:49, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 3/8/22 12:36, AKASHI Takahiro wrote:
> >>> With this patch set[1] applied, UEFI subsystem maintains a list of its
> >>> disk objects dynamically at runtime based on block device's probing.
> >>> (See "issues" below.)
> >>>
> >>> [1]https://github.com/t-akashi/u-boot/tree/efi/dm_disk
> >>
> >> This series together with Simon's series breaks multiple boards due to
> >> size constraints:
> >>
> >> https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/11197
> >>
> >> Please, investigate how to work around this issue.
> >
> > tbs2910 - perhaps we should just drop this board? It doesn't use
> > DM_SERIAL and still uses OF_EMBED
> > rcar3_salvator-x - can we put the partition changes behind a config?
>
> This board came up before. 34f2577e926da ("ARM: renesas: reduce
> rcar3_salvator-x image size"). It is generally at the size limit.
>
> > phycore-rk3288 - something going on in SPL, perhaps needs an
> > additional config to disable it?
>
> Do we need any of your events on SPL?

Not so far.

Regards,
Simon

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[Soeren Moch]

On 08.03.22 17:56, Simon Glass wrote:
> Hi,
>
> On Tue, 8 Mar 2022 at 09:49, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 3/8/22 12:36, AKASHI Takahiro wrote:
>>> With this patch set[1] applied, UEFI subsystem maintains a list of its
>>> disk objects dynamically at runtime based on block device's probing.
>>> (See "issues" below.)
>>>
>>> [1]https://github.com/t-akashi/u-boot/tree/efi/dm_disk
>>
>> This series together with Simon's series breaks multiple boards due to
>> size constraints:
>>
>> https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/11197
>>
>> Please, investigate how to work around this issue.
>
> tbs2910 - perhaps we should just drop this board? It doesn't use
> DM_SERIAL and still uses OF_EMBED

Are we again at the same point? You are breaking working boards with
(for these boards) useless additions, and all you come up with is
"remove this board". Of course without adding the board maintainer.

What a shame!

Soeren

> rcar3_salvator-x - can we put the partition changes behind a config?
> phycore-rk3288 - something going on in SPL, perhaps needs an
> additional config to disable it?
>
> Regards,
> Simon

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[Simon Glass]

Hi Soeren,

On Tue, 8 Mar 2022 at 12:15, Soeren Moch <smoch@web.de> wrote:
>
>
>
> On 08.03.22 17:56, Simon Glass wrote:
> > Hi,
> >
> > On Tue, 8 Mar 2022 at 09:49, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 3/8/22 12:36, AKASHI Takahiro wrote:
> >>> With this patch set[1] applied, UEFI subsystem maintains a list of its
> >>> disk objects dynamically at runtime based on block device's probing.
> >>> (See "issues" below.)
> >>>
> >>> [1]https://github.com/t-akashi/u-boot/tree/efi/dm_disk
> >>
> >> This series together with Simon's series breaks multiple boards due to
> >> size constraints:
> >>
> >> https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/11197
> >>
> >> Please, investigate how to work around this issue.
> >
> > tbs2910 - perhaps we should just drop this board? It doesn't use
> > DM_SERIAL and still uses OF_EMBED
>
> Are we again at the same point? You are breaking working boards with
> (for these boards) useless additions, and all you come up with is
> "remove this board". Of course without adding the board maintainer.

I'm just expressing reasonable frustration that this board uses
OF_EMBED and does not use DM_SERIAL, after all of this time. Why
should the rest of the U-Boot developers care more about this board
than the maintainer?

Regards,
Simon

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1622 bytes --]

On Tue, Mar 08, 2022 at 02:20:15PM -0700, Simon Glass wrote:
> Hi Soeren,
> 
> On Tue, 8 Mar 2022 at 12:15, Soeren Moch <smoch@web.de> wrote:
> >
> >
> >
> > On 08.03.22 17:56, Simon Glass wrote:
> > > Hi,
> > >
> > > On Tue, 8 Mar 2022 at 09:49, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >>
> > >> On 3/8/22 12:36, AKASHI Takahiro wrote:
> > >>> With this patch set[1] applied, UEFI subsystem maintains a list of its
> > >>> disk objects dynamically at runtime based on block device's probing.
> > >>> (See "issues" below.)
> > >>>
> > >>> [1]https://github.com/t-akashi/u-boot/tree/efi/dm_disk
> > >>
> > >> This series together with Simon's series breaks multiple boards due to
> > >> size constraints:
> > >>
> > >> https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/11197
> > >>
> > >> Please, investigate how to work around this issue.
> > >
> > > tbs2910 - perhaps we should just drop this board? It doesn't use
> > > DM_SERIAL and still uses OF_EMBED
> >
> > Are we again at the same point? You are breaking working boards with
> > (for these boards) useless additions, and all you come up with is
> > "remove this board". Of course without adding the board maintainer.
> 
> I'm just expressing reasonable frustration that this board uses
> OF_EMBED and does not use DM_SERIAL, after all of this time. Why
> should the rest of the U-Boot developers care more about this board
> than the maintainer?

Please keep in mind Simon that we've had zero releases with the
DM_SERIAL migration warning being posted, v2022.04 will be the first
one.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[AKASHI Takahiro]

Heinrich, Simon,

On Tue, Mar 08, 2022 at 05:49:13PM +0100, Heinrich Schuchardt wrote:
> On 3/8/22 12:36, AKASHI Takahiro wrote:
> > With this patch set[1] applied, UEFI subsystem maintains a list of its
> > disk objects dynamically at runtime based on block device's probing.
> > (See "issues" below.)
> > 
> > [1]https://github.com/t-akashi/u-boot/tree/efi/dm_disk
> 
> This series together with Simon's series breaks multiple boards due to
> size constraints:
> 
> https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/11197
> 
> Please, investigate how to work around this issue.

I have already mentioned this size issue in my cover-letter
in order to let reviewers aware of it and discuss a possible solution:

===8<===
Issues:
=======
* The image size of U-Boot may increase. CI build test complains,
  for instance,
    rcar3_salvator-x:
      "u-boot.img exceeds file size limit: ... excess: 0x79c bytes"
    phycore-rk3288:
      "SPL image is too large (size 0x8800 than 0x8000)"

  See [2].

[2] https://dev.azure.com/u-boot/u-boot/_build/results?buildId=3770&view=results
===>8===

I have dug into rcar3_salvator-x case; I removed *all* the commits
in this series and yet enabled CONFIG_EVENT, CONFIG_EVENT_DYNAMIC
and CONFIG_DM_EVENT, which are all required for enabling my patch,
with Simon's patch applied on top of v2022.04-rc3.

Then I still see this size problem:
===8<===
  ...
  MKIMAGE u-boot.img
u-boot.img exceeds file size limit:
  limit:  0x100000 bytes
  actual: 0x100036 bytes
  excess: 0x36 bytes
===>8===

So I have no way to deal with it.

FYI, the combination of EVENT, EVENT_DYNAMIC and DM_EVENT will
increase the binary size by up to 0x1b2 for rcar3_salvator-x and
it seems the binary has almost already reached its maximum size
even now.

-Takahiro Akashi

> Best regards
> 
> Heinrich

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[Simon Glass]

 Hi Tom,

On Tue, 8 Mar 2022 at 17:13, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Mar 08, 2022 at 02:20:15PM -0700, Simon Glass wrote:
> > Hi Soeren,
> >
> > On Tue, 8 Mar 2022 at 12:15, Soeren Moch <smoch@web.de> wrote:
> > >
> > >
> > >
> > > On 08.03.22 17:56, Simon Glass wrote:
> > > > Hi,
> > > >
> > > > On Tue, 8 Mar 2022 at 09:49, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > >>
> > > >> On 3/8/22 12:36, AKASHI Takahiro wrote:
> > > >>> With this patch set[1] applied, UEFI subsystem maintains a list of its
> > > >>> disk objects dynamically at runtime based on block device's probing.
> > > >>> (See "issues" below.)
> > > >>>
> > > >>> [1]https://github.com/t-akashi/u-boot/tree/efi/dm_disk
> > > >>
> > > >> This series together with Simon's series breaks multiple boards due to
> > > >> size constraints:
> > > >>
> > > >> https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/11197
> > > >>
> > > >> Please, investigate how to work around this issue.
> > > >
> > > > tbs2910 - perhaps we should just drop this board? It doesn't use
> > > > DM_SERIAL and still uses OF_EMBED
> > >
> > > Are we again at the same point? You are breaking working boards with
> > > (for these boards) useless additions, and all you come up with is
> > > "remove this board". Of course without adding the board maintainer.
> >
> > I'm just expressing reasonable frustration that this board uses
> > OF_EMBED and does not use DM_SERIAL, after all of this time. Why
> > should the rest of the U-Boot developers care more about this board
> > than the maintainer?
>
> Please keep in mind Simon that we've had zero releases with the
> DM_SERIAL migration warning being posted, v2022.04 will be the first
> one.

Yes, understood :-) For OF_EMBED though...?

It was actually quite hard to add a migration message until we added
the CONFIG_SERIAL base thing and that was a pain to do.

For those of us who take on larger refactors etc., we end up spending
a lot of our time on these few platforms. I'm not picking on tbs2910in
in particular.

Regards,
Simon

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[Simon Glass]

Hi Takahiro,

On Tue, 8 Mar 2022 at 19:10, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
>
> Heinrich, Simon,
>
> On Tue, Mar 08, 2022 at 05:49:13PM +0100, Heinrich Schuchardt wrote:
> > On 3/8/22 12:36, AKASHI Takahiro wrote:
> > > With this patch set[1] applied, UEFI subsystem maintains a list of its
> > > disk objects dynamically at runtime based on block device's probing.
> > > (See "issues" below.)
> > >
> > > [1]https://github.com/t-akashi/u-boot/tree/efi/dm_disk
> >
> > This series together with Simon's series breaks multiple boards due to
> > size constraints:
> >
> > https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/11197
> >
> > Please, investigate how to work around this issue.
>
> I have already mentioned this size issue in my cover-letter
> in order to let reviewers aware of it and discuss a possible solution:
>
> ===8<===
> Issues:
> =======
> * The image size of U-Boot may increase. CI build test complains,
>   for instance,
>     rcar3_salvator-x:
>       "u-boot.img exceeds file size limit: ... excess: 0x79c bytes"
>     phycore-rk3288:
>       "SPL image is too large (size 0x8800 than 0x8000)"
>
>   See [2].
>
> [2] https://dev.azure.com/u-boot/u-boot/_build/results?buildId=3770&view=results
> ===>8===
>
> I have dug into rcar3_salvator-x case; I removed *all* the commits
> in this series and yet enabled CONFIG_EVENT, CONFIG_EVENT_DYNAMIC
> and CONFIG_DM_EVENT, which are all required for enabling my patch,
> with Simon's patch applied on top of v2022.04-rc3.
>
> Then I still see this size problem:
> ===8<===
>   ...
>   MKIMAGE u-boot.img
> u-boot.img exceeds file size limit:
>   limit:  0x100000 bytes
>   actual: 0x100036 bytes
>   excess: 0x36 bytes
> ===>8===
>
> So I have no way to deal with it.
>
> FYI, the combination of EVENT, EVENT_DYNAMIC and DM_EVENT will
> increase the binary size by up to 0x1b2 for rcar3_salvator-x and
> it seems the binary has almost already reached its maximum size
> even now.

So you do need EVENT_DYNAMIC?

Does it make sense to make enabling the partition support an option,
instead of mandatory?

Regards,
Simon

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[AKASHI Takahiro]

Hi Simon,

On Tue, Mar 08, 2022 at 07:34:15PM -0700, Simon Glass wrote:
> Hi Takahiro,
> 
> On Tue, 8 Mar 2022 at 19:10, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> >
> > Heinrich, Simon,
> >
> > On Tue, Mar 08, 2022 at 05:49:13PM +0100, Heinrich Schuchardt wrote:
> > > On 3/8/22 12:36, AKASHI Takahiro wrote:
> > > > With this patch set[1] applied, UEFI subsystem maintains a list of its
> > > > disk objects dynamically at runtime based on block device's probing.
> > > > (See "issues" below.)
> > > >
> > > > [1]https://github.com/t-akashi/u-boot/tree/efi/dm_disk
> > >
> > > This series together with Simon's series breaks multiple boards due to
> > > size constraints:
> > >
> > > https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/11197
> > >
> > > Please, investigate how to work around this issue.
> >
> > I have already mentioned this size issue in my cover-letter
> > in order to let reviewers aware of it and discuss a possible solution:
> >
> > ===8<===
> > Issues:
> > =======
> > * The image size of U-Boot may increase. CI build test complains,
> >   for instance,
> >     rcar3_salvator-x:
> >       "u-boot.img exceeds file size limit: ... excess: 0x79c bytes"
> >     phycore-rk3288:
> >       "SPL image is too large (size 0x8800 than 0x8000)"
> >
> >   See [2].
> >
> > [2] https://dev.azure.com/u-boot/u-boot/_build/results?buildId=3770&view=results
> > ===>8===
> >
> > I have dug into rcar3_salvator-x case; I removed *all* the commits
> > in this series and yet enabled CONFIG_EVENT, CONFIG_EVENT_DYNAMIC
> > and CONFIG_DM_EVENT, which are all required for enabling my patch,
> > with Simon's patch applied on top of v2022.04-rc3.
> >
> > Then I still see this size problem:
> > ===8<===
> >   ...
> >   MKIMAGE u-boot.img
> > u-boot.img exceeds file size limit:
> >   limit:  0x100000 bytes
> >   actual: 0x100036 bytes
> >   excess: 0x36 bytes
> > ===>8===
> >
> > So I have no way to deal with it.
> >
> > FYI, the combination of EVENT, EVENT_DYNAMIC and DM_EVENT will
> > increase the binary size by up to 0x1b2 for rcar3_salvator-x and
> > it seems the binary has almost already reached its maximum size
> > even now.
> 
> So you do need EVENT_DYNAMIC?

Unfortunately, yes.
When I rebased my patch set to your v2, I tried to use *static*
bindings, but some of ut tests, including dm_test_blk_base and
dm_test_blk_usb, failed.

This can happen because, with static bindings, efi's callback function
(efi_disk_probe) is unconditionally called even when UEFI subsystem has
not been initialized yet.

-Takahiro Akashi

> Does it make sense to make enabling the partition support an option,
> instead of mandatory?
> 
> Regards,
> Simon

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2715 bytes --]

On Tue, Mar 08, 2022 at 07:32:59PM -0700, Simon Glass wrote:
>  Hi Tom,
> 
> On Tue, 8 Mar 2022 at 17:13, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Mar 08, 2022 at 02:20:15PM -0700, Simon Glass wrote:
> > > Hi Soeren,
> > >
> > > On Tue, 8 Mar 2022 at 12:15, Soeren Moch <smoch@web.de> wrote:
> > > >
> > > >
> > > >
> > > > On 08.03.22 17:56, Simon Glass wrote:
> > > > > Hi,
> > > > >
> > > > > On Tue, 8 Mar 2022 at 09:49, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > >>
> > > > >> On 3/8/22 12:36, AKASHI Takahiro wrote:
> > > > >>> With this patch set[1] applied, UEFI subsystem maintains a list of its
> > > > >>> disk objects dynamically at runtime based on block device's probing.
> > > > >>> (See "issues" below.)
> > > > >>>
> > > > >>> [1]https://github.com/t-akashi/u-boot/tree/efi/dm_disk
> > > > >>
> > > > >> This series together with Simon's series breaks multiple boards due to
> > > > >> size constraints:
> > > > >>
> > > > >> https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/11197
> > > > >>
> > > > >> Please, investigate how to work around this issue.
> > > > >
> > > > > tbs2910 - perhaps we should just drop this board? It doesn't use
> > > > > DM_SERIAL and still uses OF_EMBED
> > > >
> > > > Are we again at the same point? You are breaking working boards with
> > > > (for these boards) useless additions, and all you come up with is
> > > > "remove this board". Of course without adding the board maintainer.
> > >
> > > I'm just expressing reasonable frustration that this board uses
> > > OF_EMBED and does not use DM_SERIAL, after all of this time. Why
> > > should the rest of the U-Boot developers care more about this board
> > > than the maintainer?
> >
> > Please keep in mind Simon that we've had zero releases with the
> > DM_SERIAL migration warning being posted, v2022.04 will be the first
> > one.
> 
> Yes, understood :-) For OF_EMBED though...?

No deadline and 50 boards.

> It was actually quite hard to add a migration message until we added
> the CONFIG_SERIAL base thing and that was a pain to do.
> 
> For those of us who take on larger refactors etc., we end up spending
> a lot of our time on these few platforms. I'm not picking on tbs2910in
> in particular.

Well, the flip side of the problem here is that there's a number of
platforms with real constraints to them and it keeps being "can we drop
this yet?" without CC'ing the board maintainer on the series that once
again pushes a given platform to the limit.  I would expect no size
growth to tbs2910 for the topic of this series since it disables
EFI_LOADER entirely, so why is it a problem?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[Simon Glass]

Hi Takahiro,

On Tue, 8 Mar 2022 at 19:48, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
>
> Hi Simon,
>
> On Tue, Mar 08, 2022 at 07:34:15PM -0700, Simon Glass wrote:
> > Hi Takahiro,
> >
> > On Tue, 8 Mar 2022 at 19:10, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > >
> > > Heinrich, Simon,
> > >
> > > On Tue, Mar 08, 2022 at 05:49:13PM +0100, Heinrich Schuchardt wrote:
> > > > On 3/8/22 12:36, AKASHI Takahiro wrote:
> > > > > With this patch set[1] applied, UEFI subsystem maintains a list of its
> > > > > disk objects dynamically at runtime based on block device's probing.
> > > > > (See "issues" below.)
> > > > >
> > > > > [1]https://github.com/t-akashi/u-boot/tree/efi/dm_disk
> > > >
> > > > This series together with Simon's series breaks multiple boards due to
> > > > size constraints:
> > > >
> > > > https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/11197
> > > >
> > > > Please, investigate how to work around this issue.
> > >
> > > I have already mentioned this size issue in my cover-letter
> > > in order to let reviewers aware of it and discuss a possible solution:
> > >
> > > ===8<===
> > > Issues:
> > > =======
> > > * The image size of U-Boot may increase. CI build test complains,
> > >   for instance,
> > >     rcar3_salvator-x:
> > >       "u-boot.img exceeds file size limit: ... excess: 0x79c bytes"
> > >     phycore-rk3288:
> > >       "SPL image is too large (size 0x8800 than 0x8000)"
> > >
> > >   See [2].
> > >
> > > [2] https://dev.azure.com/u-boot/u-boot/_build/results?buildId=3770&view=results
> > > ===>8===
> > >
> > > I have dug into rcar3_salvator-x case; I removed *all* the commits
> > > in this series and yet enabled CONFIG_EVENT, CONFIG_EVENT_DYNAMIC
> > > and CONFIG_DM_EVENT, which are all required for enabling my patch,
> > > with Simon's patch applied on top of v2022.04-rc3.
> > >
> > > Then I still see this size problem:
> > > ===8<===
> > >   ...
> > >   MKIMAGE u-boot.img
> > > u-boot.img exceeds file size limit:
> > >   limit:  0x100000 bytes
> > >   actual: 0x100036 bytes
> > >   excess: 0x36 bytes
> > > ===>8===
> > >
> > > So I have no way to deal with it.
> > >
> > > FYI, the combination of EVENT, EVENT_DYNAMIC and DM_EVENT will
> > > increase the binary size by up to 0x1b2 for rcar3_salvator-x and
> > > it seems the binary has almost already reached its maximum size
> > > even now.
> >
> > So you do need EVENT_DYNAMIC?
>
> Unfortunately, yes.
> When I rebased my patch set to your v2, I tried to use *static*
> bindings, but some of ut tests, including dm_test_blk_base and
> dm_test_blk_usb, failed.

OK. Well maybe there is a filesystem in there that is not needed? 1MB
is a huge size! Can we disable EFI_LOADER on that board?

>
> This can happen because, with static bindings, efi's callback function
> (efi_disk_probe) is unconditionally called even when UEFI subsystem has
> not been initialized yet.

Yes, I have seen things like that too.

>
> -Takahiro Akashi
>
> > Does it make sense to make enabling the partition support an option,
> > instead of mandatory?

What about this one? ^^

Regards,
Simon

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[Simon Glass]

Hi Tom,

On Tue, 8 Mar 2022 at 20:00, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Mar 08, 2022 at 07:32:59PM -0700, Simon Glass wrote:
> >  Hi Tom,
> >
> > On Tue, 8 Mar 2022 at 17:13, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Tue, Mar 08, 2022 at 02:20:15PM -0700, Simon Glass wrote:
> > > > Hi Soeren,
> > > >
> > > > On Tue, 8 Mar 2022 at 12:15, Soeren Moch <smoch@web.de> wrote:
> > > > >
> > > > >
> > > > >
> > > > > On 08.03.22 17:56, Simon Glass wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On Tue, 8 Mar 2022 at 09:49, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > >>
> > > > > >> On 3/8/22 12:36, AKASHI Takahiro wrote:
> > > > > >>> With this patch set[1] applied, UEFI subsystem maintains a list of its
> > > > > >>> disk objects dynamically at runtime based on block device's probing.
> > > > > >>> (See "issues" below.)
> > > > > >>>
> > > > > >>> [1]https://github.com/t-akashi/u-boot/tree/efi/dm_disk
> > > > > >>
> > > > > >> This series together with Simon's series breaks multiple boards due to
> > > > > >> size constraints:
> > > > > >>
> > > > > >> https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/11197
> > > > > >>
> > > > > >> Please, investigate how to work around this issue.
> > > > > >
> > > > > > tbs2910 - perhaps we should just drop this board? It doesn't use
> > > > > > DM_SERIAL and still uses OF_EMBED
> > > > >
> > > > > Are we again at the same point? You are breaking working boards with
> > > > > (for these boards) useless additions, and all you come up with is
> > > > > "remove this board". Of course without adding the board maintainer.
> > > >
> > > > I'm just expressing reasonable frustration that this board uses
> > > > OF_EMBED and does not use DM_SERIAL, after all of this time. Why
> > > > should the rest of the U-Boot developers care more about this board
> > > > than the maintainer?
> > >
> > > Please keep in mind Simon that we've had zero releases with the
> > > DM_SERIAL migration warning being posted, v2022.04 will be the first
> > > one.
> >
> > Yes, understood :-) For OF_EMBED though...?
>
> No deadline and 50 boards.

Er, there has been a build message about that since the beginning, so
people ignored it. Do we really need to make the build fail for these
sorts of things? Perhaps so, but it is a sad situation.

>
> > It was actually quite hard to add a migration message until we added
> > the CONFIG_SERIAL base thing and that was a pain to do.
> >
> > For those of us who take on larger refactors etc., we end up spending
> > a lot of our time on these few platforms. I'm not picking on tbs2910in
> > in particular.
>
> Well, the flip side of the problem here is that there's a number of
> platforms with real constraints to them and it keeps being "can we drop
> this yet?" without CC'ing the board maintainer on the series that once
> again pushes a given platform to the limit.  I would expect no size
> growth to tbs2910 for the topic of this series since it disables
> EFI_LOADER entirely, so why is it a problem?

The partition changes are going to add some size anyway, I expect. I
have not actually analysed it though. Perhaps we can just disable a
filesystem?

Regards,
Simon

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[Simon Glass]

Hi Tom,

On Tue, 8 Mar 2022 at 20:10, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Tom,
>
> On Tue, 8 Mar 2022 at 20:00, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Mar 08, 2022 at 07:32:59PM -0700, Simon Glass wrote:
> > >  Hi Tom,
> > >
> > > On Tue, 8 Mar 2022 at 17:13, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Tue, Mar 08, 2022 at 02:20:15PM -0700, Simon Glass wrote:
> > > > > Hi Soeren,
> > > > >
> > > > > On Tue, 8 Mar 2022 at 12:15, Soeren Moch <smoch@web.de> wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > On 08.03.22 17:56, Simon Glass wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > On Tue, 8 Mar 2022 at 09:49, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > >>
> > > > > > >> On 3/8/22 12:36, AKASHI Takahiro wrote:
> > > > > > >>> With this patch set[1] applied, UEFI subsystem maintains a list of its
> > > > > > >>> disk objects dynamically at runtime based on block device's probing.
> > > > > > >>> (See "issues" below.)
> > > > > > >>>
> > > > > > >>> [1]https://github.com/t-akashi/u-boot/tree/efi/dm_disk
> > > > > > >>
> > > > > > >> This series together with Simon's series breaks multiple boards due to
> > > > > > >> size constraints:
> > > > > > >>
> > > > > > >> https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/11197
> > > > > > >>
> > > > > > >> Please, investigate how to work around this issue.
> > > > > > >
> > > > > > > tbs2910 - perhaps we should just drop this board? It doesn't use
> > > > > > > DM_SERIAL and still uses OF_EMBED
> > > > > >
> > > > > > Are we again at the same point? You are breaking working boards with
> > > > > > (for these boards) useless additions, and all you come up with is
> > > > > > "remove this board". Of course without adding the board maintainer.
> > > > >
> > > > > I'm just expressing reasonable frustration that this board uses
> > > > > OF_EMBED and does not use DM_SERIAL, after all of this time. Why
> > > > > should the rest of the U-Boot developers care more about this board
> > > > > than the maintainer?
> > > >
> > > > Please keep in mind Simon that we've had zero releases with the
> > > > DM_SERIAL migration warning being posted, v2022.04 will be the first
> > > > one.
> > >
> > > Yes, understood :-) For OF_EMBED though...?
> >
> > No deadline and 50 boards.
>
> Er, there has been a build message about that since the beginning, so
> people ignored it. Do we really need to make the build fail for these
> sorts of things? Perhaps so, but it is a sad situation.
>
> >
> > > It was actually quite hard to add a migration message until we added
> > > the CONFIG_SERIAL base thing and that was a pain to do.
> > >
> > > For those of us who take on larger refactors etc., we end up spending
> > > a lot of our time on these few platforms. I'm not picking on tbs2910in
> > > in particular.
> >
> > Well, the flip side of the problem here is that there's a number of
> > platforms with real constraints to them and it keeps being "can we drop
> > this yet?" without CC'ing the board maintainer on the series that once
> > again pushes a given platform to the limit.  I would expect no size
> > growth to tbs2910 for the topic of this series since it disables
> > EFI_LOADER entirely, so why is it a problem?
>
> The partition changes are going to add some size anyway, I expect. I
> have not actually analysed it though. Perhaps we can just disable a
> filesystem?

One more thing, if we get around to supporting file access through a
device that is also going to add some size, although perhaps not a
huge amount.

Regards,
SImon

> Simon

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[AKASHI Takahiro]

On Tue, Mar 08, 2022 at 08:10:01PM -0700, Simon Glass wrote:
> Hi Takahiro,
> 
> On Tue, 8 Mar 2022 at 19:48, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > On Tue, Mar 08, 2022 at 07:34:15PM -0700, Simon Glass wrote:
> > > Hi Takahiro,
> > >
> > > On Tue, 8 Mar 2022 at 19:10, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > > >
> > > > Heinrich, Simon,
> > > >
> > > > On Tue, Mar 08, 2022 at 05:49:13PM +0100, Heinrich Schuchardt wrote:
> > > > > On 3/8/22 12:36, AKASHI Takahiro wrote:
> > > > > > With this patch set[1] applied, UEFI subsystem maintains a list of its
> > > > > > disk objects dynamically at runtime based on block device's probing.
> > > > > > (See "issues" below.)
> > > > > >
> > > > > > [1]https://github.com/t-akashi/u-boot/tree/efi/dm_disk
> > > > >
> > > > > This series together with Simon's series breaks multiple boards due to
> > > > > size constraints:
> > > > >
> > > > > https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/11197
> > > > >
> > > > > Please, investigate how to work around this issue.
> > > >
> > > > I have already mentioned this size issue in my cover-letter
> > > > in order to let reviewers aware of it and discuss a possible solution:
> > > >
> > > > ===8<===
> > > > Issues:
> > > > =======
> > > > * The image size of U-Boot may increase. CI build test complains,
> > > >   for instance,
> > > >     rcar3_salvator-x:
> > > >       "u-boot.img exceeds file size limit: ... excess: 0x79c bytes"
> > > >     phycore-rk3288:
> > > >       "SPL image is too large (size 0x8800 than 0x8000)"
> > > >
> > > >   See [2].
> > > >
> > > > [2] https://dev.azure.com/u-boot/u-boot/_build/results?buildId=3770&view=results
> > > > ===>8===
> > > >
> > > > I have dug into rcar3_salvator-x case; I removed *all* the commits
> > > > in this series and yet enabled CONFIG_EVENT, CONFIG_EVENT_DYNAMIC
> > > > and CONFIG_DM_EVENT, which are all required for enabling my patch,
> > > > with Simon's patch applied on top of v2022.04-rc3.
> > > >
> > > > Then I still see this size problem:
> > > > ===8<===
> > > >   ...
> > > >   MKIMAGE u-boot.img
> > > > u-boot.img exceeds file size limit:
> > > >   limit:  0x100000 bytes
> > > >   actual: 0x100036 bytes
> > > >   excess: 0x36 bytes
> > > > ===>8===
> > > >
> > > > So I have no way to deal with it.
> > > >
> > > > FYI, the combination of EVENT, EVENT_DYNAMIC and DM_EVENT will
> > > > increase the binary size by up to 0x1b2 for rcar3_salvator-x and
> > > > it seems the binary has almost already reached its maximum size
> > > > even now.
> > >
> > > So you do need EVENT_DYNAMIC?
> >
> > Unfortunately, yes.
> > When I rebased my patch set to your v2, I tried to use *static*
> > bindings, but some of ut tests, including dm_test_blk_base and
> > dm_test_blk_usb, failed.
> 
> OK. Well maybe there is a filesystem in there that is not needed? 1MB
> is a huge size! Can we disable EFI_LOADER on that board?

Well, EFI_LOADER is by default 'y' for arm64.
Basically, I doubt that this default is reasonable.

> >
> > This can happen because, with static bindings, efi's callback function
> > (efi_disk_probe) is unconditionally called even when UEFI subsystem has
> > not been initialized yet.
> 
> Yes, I have seen things like that too.
> 
> >
> > -Takahiro Akashi
> >
> > > Does it make sense to make enabling the partition support an option,
> > > instead of mandatory?
> 
> What about this one? ^^

First of all, according to my rough attempt,
the patches for adding efi_disk callback functions may increase
the binary size by 0x31c, while the patches for adding UCLASS_PARTITION
adds another 0x3ba.
This means that "enabling the partition support an option" can help a bit
but doesn't help well enough overall.

FYI, adding dev_read/write(udev) interfaces costs another 0x1df.

-Takahiro Akashi

> Regards,
> Simon

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[Mark Kettenis]

> Date: Tue, 8 Mar 2022 22:00:35 -0500
> From: Tom Rini <trini@konsulko.com>
> 
> On Tue, Mar 08, 2022 at 07:32:59PM -0700, Simon Glass wrote:
> >  Hi Tom,
> > 
> > On Tue, 8 Mar 2022 at 17:13, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Tue, Mar 08, 2022 at 02:20:15PM -0700, Simon Glass wrote:
> > > > Hi Soeren,
> > > >
> > > > On Tue, 8 Mar 2022 at 12:15, Soeren Moch <smoch@web.de> wrote:
> > > > >
> > > > >
> > > > >
> > > > > On 08.03.22 17:56, Simon Glass wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On Tue, 8 Mar 2022 at 09:49, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > >>
> > > > > >> On 3/8/22 12:36, AKASHI Takahiro wrote:
> > > > > >>> With this patch set[1] applied, UEFI subsystem maintains a list of its
> > > > > >>> disk objects dynamically at runtime based on block device's probing.
> > > > > >>> (See "issues" below.)
> > > > > >>>
> > > > > >>> [1]https://github.com/t-akashi/u-boot/tree/efi/dm_disk
> > > > > >>
> > > > > >> This series together with Simon's series breaks multiple boards due to
> > > > > >> size constraints:
> > > > > >>
> > > > > >> https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/11197
> > > > > >>
> > > > > >> Please, investigate how to work around this issue.
> > > > > >
> > > > > > tbs2910 - perhaps we should just drop this board? It doesn't use
> > > > > > DM_SERIAL and still uses OF_EMBED
> > > > >
> > > > > Are we again at the same point? You are breaking working boards with
> > > > > (for these boards) useless additions, and all you come up with is
> > > > > "remove this board". Of course without adding the board maintainer.
> > > >
> > > > I'm just expressing reasonable frustration that this board uses
> > > > OF_EMBED and does not use DM_SERIAL, after all of this time. Why
> > > > should the rest of the U-Boot developers care more about this board
> > > > than the maintainer?
> > >
> > > Please keep in mind Simon that we've had zero releases with the
> > > DM_SERIAL migration warning being posted, v2022.04 will be the first
> > > one.
> > 
> > Yes, understood :-) For OF_EMBED though...?
> 
> No deadline and 50 boards.

And neither should there be.  OF_EMBED is the obvious choice when
chainloading U-Boot from a proprietary initial loader that loads ELF
binaries.

Re: [PATCH v3 08/19] dm: add tag support

[Ilias Apalodimas]

Hi Akashi-san,

> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
>  drivers/core/Makefile             |   2 +-
>  drivers/core/root.c               |   2 +
>  drivers/core/tag.c                | 139 ++++++++++++++++++++++++++++++
>  include/asm-generic/global_data.h |   4 +
>  include/dm/tag.h                  | 110 +++++++++++++++++++++++
>  5 files changed, 256 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/core/tag.c
>  create mode 100644 include/dm/tag.h
> 
> diff --git a/drivers/core/Makefile b/drivers/core/Makefile
> index 5edd4e413576..3742e7574525 100644
> --- a/drivers/core/Makefile
> +++ b/drivers/core/Makefile
> @@ -2,7 +2,7 @@
>  #
>  # Copyright (c) 2013 Google, Inc
>  
> -obj-y	+= device.o fdtaddr.o lists.o root.o uclass.o util.o
> +obj-y	+= device.o fdtaddr.o lists.o root.o uclass.o util.o tag.o
>  obj-$(CONFIG_$(SPL_TPL_)ACPIGEN) += acpi.o
>  obj-$(CONFIG_DEVRES) += devres.o
>  obj-$(CONFIG_$(SPL_)DM_DEVICE_REMOVE)	+= device-remove.o
> diff --git a/drivers/core/root.c b/drivers/core/root.c
> index 8efb4256b27e..86b3884fc674 100644
> --- a/drivers/core/root.c
> +++ b/drivers/core/root.c
> @@ -199,6 +199,8 @@ int dm_init(bool of_live)
>  			return ret;
>  	}
>  
> +	INIT_LIST_HEAD((struct list_head *)&gd->dmtag_list);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/core/tag.c b/drivers/core/tag.c
> new file mode 100644
> index 000000000000..6829bcd8806c
> --- /dev/null
> +++ b/drivers/core/tag.c
> @@ -0,0 +1,139 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2021 Linaro Limited
> + *			Author: AKASHI Takahiro
> + */
> +
> +#include <malloc.h>
> +#include <asm/global_data.h>
> +#include <dm/tag.h>
> +#include <linux/err.h>
> +#include <linux/list.h>
> +#include <linux/types.h>
> +
> +struct udevice;
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +int dev_tag_set_ptr(struct udevice *dev, enum dm_tag_t tag, void *ptr)
> +{
> +	struct dmtag_node *node;
> +
> +	if (!dev || tag >= DM_TAG_COUNT)
> +		return -EINVAL;
> +
> +	list_for_each_entry(node, &gd->dmtag_list, sibling) {
> +		if (node->dev == dev && node->tag == tag)
> +			return -EEXIST;
> +	}
> +
> +	node = calloc(sizeof(*node), 1);
> +	if (!node)
> +		return -ENOSPC;

Isn't -ENOMEM better here?

> +
> +	node->dev = dev;
> +	node->tag = tag;
> +	node->ptr = ptr;
> +	list_add_tail(&node->sibling, (struct list_head *)&gd->dmtag_list);
> +
> +	return 0;
> +}
> +
> +int dev_tag_set_val(struct udevice *dev, enum dm_tag_t tag, ulong val)

Is this used anywhere else apart from selftests?

> +{
> +	struct dmtag_node *node;


Thanks
/Ilias

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[Heinrich Schuchardt]

On 3/9/22 06:07, AKASHI Takahiro wrote:
> On Tue, Mar 08, 2022 at 08:10:01PM -0700, Simon Glass wrote:
>> Hi Takahiro,
>>
>> On Tue, 8 Mar 2022 at 19:48, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
>>>
>>> Hi Simon,
>>>
>>> On Tue, Mar 08, 2022 at 07:34:15PM -0700, Simon Glass wrote:
>>>> Hi Takahiro,
>>>>
>>>> On Tue, 8 Mar 2022 at 19:10, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
>>>>>
>>>>> Heinrich, Simon,
>>>>>
>>>>> On Tue, Mar 08, 2022 at 05:49:13PM +0100, Heinrich Schuchardt wrote:
>>>>>> On 3/8/22 12:36, AKASHI Takahiro wrote:
>>>>>>> With this patch set[1] applied, UEFI subsystem maintains a list of its
>>>>>>> disk objects dynamically at runtime based on block device's probing.
>>>>>>> (See "issues" below.)
>>>>>>>
>>>>>>> [1]https://github.com/t-akashi/u-boot/tree/efi/dm_disk
>>>>>>
>>>>>> This series together with Simon's series breaks multiple boards due to
>>>>>> size constraints:
>>>>>>
>>>>>> https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/11197
>>>>>>
>>>>>> Please, investigate how to work around this issue.
>>>>>
>>>>> I have already mentioned this size issue in my cover-letter
>>>>> in order to let reviewers aware of it and discuss a possible solution:
>>>>>
>>>>> ===8<===
>>>>> Issues:
>>>>> =======
>>>>> * The image size of U-Boot may increase. CI build test complains,
>>>>>    for instance,
>>>>>      rcar3_salvator-x:
>>>>>        "u-boot.img exceeds file size limit: ... excess: 0x79c bytes"
>>>>>      phycore-rk3288:
>>>>>        "SPL image is too large (size 0x8800 than 0x8000)"
>>>>>
>>>>>    See [2].
>>>>>
>>>>> [2] https://dev.azure.com/u-boot/u-boot/_build/results?buildId=3770&view=results
>>>>> ===>8===
>>>>>
>>>>> I have dug into rcar3_salvator-x case; I removed *all* the commits
>>>>> in this series and yet enabled CONFIG_EVENT, CONFIG_EVENT_DYNAMIC
>>>>> and CONFIG_DM_EVENT, which are all required for enabling my patch,
>>>>> with Simon's patch applied on top of v2022.04-rc3.
>>>>>
>>>>> Then I still see this size problem:
>>>>> ===8<===
>>>>>    ...
>>>>>    MKIMAGE u-boot.img
>>>>> u-boot.img exceeds file size limit:
>>>>>    limit:  0x100000 bytes
>>>>>    actual: 0x100036 bytes
>>>>>    excess: 0x36 bytes
>>>>> ===>8===
>>>>>
>>>>> So I have no way to deal with it.
>>>>>
>>>>> FYI, the combination of EVENT, EVENT_DYNAMIC and DM_EVENT will
>>>>> increase the binary size by up to 0x1b2 for rcar3_salvator-x and
>>>>> it seems the binary has almost already reached its maximum size
>>>>> even now.
>>>>
>>>> So you do need EVENT_DYNAMIC?
>>>
>>> Unfortunately, yes.
>>> When I rebased my patch set to your v2, I tried to use *static*
>>> bindings, but some of ut tests, including dm_test_blk_base and
>>> dm_test_blk_usb, failed.
>>
>> OK. Well maybe there is a filesystem in there that is not needed? 1MB
>> is a huge size! Can we disable EFI_LOADER on that board?
>
> Well, EFI_LOADER is by default 'y' for arm64.
> Basically, I doubt that this default is reasonable.

All major distros support booting via UEFI. Fedora and Suse have
specifically opted to make this the preferred way to boot on ARM. Same
is true for BSD. So why do you have doubts?

Best regards

Heinrich

>
>>>
>>> This can happen because, with static bindings, efi's callback function
>>> (efi_disk_probe) is unconditionally called even when UEFI subsystem has
>>> not been initialized yet.
>>
>> Yes, I have seen things like that too.
>>
>>>
>>> -Takahiro Akashi
>>>
>>>> Does it make sense to make enabling the partition support an option,
>>>> instead of mandatory?
>>
>> What about this one? ^^
>
> First of all, according to my rough attempt,
> the patches for adding efi_disk callback functions may increase
> the binary size by 0x31c, while the patches for adding UCLASS_PARTITION
> adds another 0x3ba.
> This means that "enabling the partition support an option" can help a bit
> but doesn't help well enough overall.
>
> FYI, adding dev_read/write(udev) interfaces costs another 0x1df.
>
> -Takahiro Akashi
>
>> Regards,
>> Simon

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4049 bytes --]

On Tue, Mar 08, 2022 at 08:10:38PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, 8 Mar 2022 at 20:00, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Mar 08, 2022 at 07:32:59PM -0700, Simon Glass wrote:
> > >  Hi Tom,
> > >
> > > On Tue, 8 Mar 2022 at 17:13, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Tue, Mar 08, 2022 at 02:20:15PM -0700, Simon Glass wrote:
> > > > > Hi Soeren,
> > > > >
> > > > > On Tue, 8 Mar 2022 at 12:15, Soeren Moch <smoch@web.de> wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > On 08.03.22 17:56, Simon Glass wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > On Tue, 8 Mar 2022 at 09:49, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > >>
> > > > > > >> On 3/8/22 12:36, AKASHI Takahiro wrote:
> > > > > > >>> With this patch set[1] applied, UEFI subsystem maintains a list of its
> > > > > > >>> disk objects dynamically at runtime based on block device's probing.
> > > > > > >>> (See "issues" below.)
> > > > > > >>>
> > > > > > >>> [1]https://github.com/t-akashi/u-boot/tree/efi/dm_disk
> > > > > > >>
> > > > > > >> This series together with Simon's series breaks multiple boards due to
> > > > > > >> size constraints:
> > > > > > >>
> > > > > > >> https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/11197
> > > > > > >>
> > > > > > >> Please, investigate how to work around this issue.
> > > > > > >
> > > > > > > tbs2910 - perhaps we should just drop this board? It doesn't use
> > > > > > > DM_SERIAL and still uses OF_EMBED
> > > > > >
> > > > > > Are we again at the same point? You are breaking working boards with
> > > > > > (for these boards) useless additions, and all you come up with is
> > > > > > "remove this board". Of course without adding the board maintainer.
> > > > >
> > > > > I'm just expressing reasonable frustration that this board uses
> > > > > OF_EMBED and does not use DM_SERIAL, after all of this time. Why
> > > > > should the rest of the U-Boot developers care more about this board
> > > > > than the maintainer?
> > > >
> > > > Please keep in mind Simon that we've had zero releases with the
> > > > DM_SERIAL migration warning being posted, v2022.04 will be the first
> > > > one.
> > >
> > > Yes, understood :-) For OF_EMBED though...?
> >
> > No deadline and 50 boards.
> 
> Er, there has been a build message about that since the beginning, so
> people ignored it. Do we really need to make the build fail for these
> sorts of things? Perhaps so, but it is a sad situation.

Yes, in hind-sight, "don't do that" wasn't the right path.  It would be
a good idea to start a different thread and see what / how the platforms
can be migrated away.

> > > It was actually quite hard to add a migration message until we added
> > > the CONFIG_SERIAL base thing and that was a pain to do.
> > >
> > > For those of us who take on larger refactors etc., we end up spending
> > > a lot of our time on these few platforms. I'm not picking on tbs2910in
> > > in particular.
> >
> > Well, the flip side of the problem here is that there's a number of
> > platforms with real constraints to them and it keeps being "can we drop
> > this yet?" without CC'ing the board maintainer on the series that once
> > again pushes a given platform to the limit.  I would expect no size
> > growth to tbs2910 for the topic of this series since it disables
> > EFI_LOADER entirely, so why is it a problem?
> 
> The partition changes are going to add some size anyway, I expect. I
> have not actually analysed it though. Perhaps we can just disable a
> filesystem?

I was a bit too absolutist there, sorry.  Yes, a few hundreds of bytes
here-and-there is probably a non issue.  But it shouldn't be kilobytes.
It really shouldn't push things over the line.

And on the tbs2910 side, Soeren, can you look at enabling LTO for this
platform?  That would likely buy a good bit of space savings.  That
might well be needed to do further DM migrations/etc.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[Simon Glass]

Hi Tom,

On Wed, 9 Mar 2022 at 07:25, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Mar 08, 2022 at 08:10:38PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Tue, 8 Mar 2022 at 20:00, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Tue, Mar 08, 2022 at 07:32:59PM -0700, Simon Glass wrote:
> > > >  Hi Tom,
> > > >
> > > > On Tue, 8 Mar 2022 at 17:13, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Tue, Mar 08, 2022 at 02:20:15PM -0700, Simon Glass wrote:
> > > > > > Hi Soeren,
> > > > > >
> > > > > > On Tue, 8 Mar 2022 at 12:15, Soeren Moch <smoch@web.de> wrote:
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > On 08.03.22 17:56, Simon Glass wrote:
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > On Tue, 8 Mar 2022 at 09:49, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > > >>
> > > > > > > >> On 3/8/22 12:36, AKASHI Takahiro wrote:
> > > > > > > >>> With this patch set[1] applied, UEFI subsystem maintains a list of its
> > > > > > > >>> disk objects dynamically at runtime based on block device's probing.
> > > > > > > >>> (See "issues" below.)
> > > > > > > >>>
> > > > > > > >>> [1]https://github.com/t-akashi/u-boot/tree/efi/dm_disk
> > > > > > > >>
> > > > > > > >> This series together with Simon's series breaks multiple boards due to
> > > > > > > >> size constraints:
> > > > > > > >>
> > > > > > > >> https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/11197
> > > > > > > >>
> > > > > > > >> Please, investigate how to work around this issue.
> > > > > > > >
> > > > > > > > tbs2910 - perhaps we should just drop this board? It doesn't use
> > > > > > > > DM_SERIAL and still uses OF_EMBED
> > > > > > >
> > > > > > > Are we again at the same point? You are breaking working boards with
> > > > > > > (for these boards) useless additions, and all you come up with is
> > > > > > > "remove this board". Of course without adding the board maintainer.
> > > > > >
> > > > > > I'm just expressing reasonable frustration that this board uses
> > > > > > OF_EMBED and does not use DM_SERIAL, after all of this time. Why
> > > > > > should the rest of the U-Boot developers care more about this board
> > > > > > than the maintainer?
> > > > >
> > > > > Please keep in mind Simon that we've had zero releases with the
> > > > > DM_SERIAL migration warning being posted, v2022.04 will be the first
> > > > > one.
> > > >
> > > > Yes, understood :-) For OF_EMBED though...?
> > >
> > > No deadline and 50 boards.
> >
> > Er, there has been a build message about that since the beginning, so
> > people ignored it. Do we really need to make the build fail for these
> > sorts of things? Perhaps so, but it is a sad situation.
>
> Yes, in hind-sight, "don't do that" wasn't the right path.  It would be
> a good idea to start a different thread and see what / how the platforms
> can be migrated away.

I think there is a use case for it now - e.g. booting Apple M1 which
uses a separate bootloader. IMO a .img or .fit file would be better in
some cases but people seem to be allergic to implementing U-Boot
things in their code bases. We have the same requirement for the EFI
app since UEFI does not implement the U-Boot .img file.

So if we are going to support this, perhaps we should create a new
option for it. But honestly I am just too weary to consider yet
another migration. We need to finish some, e.g. Kconfig.

>
> > > > It was actually quite hard to add a migration message until we added
> > > > the CONFIG_SERIAL base thing and that was a pain to do.
> > > >
> > > > For those of us who take on larger refactors etc., we end up spending
> > > > a lot of our time on these few platforms. I'm not picking on tbs2910in
> > > > in particular.
> > >
> > > Well, the flip side of the problem here is that there's a number of
> > > platforms with real constraints to them and it keeps being "can we drop
> > > this yet?" without CC'ing the board maintainer on the series that once
> > > again pushes a given platform to the limit.  I would expect no size
> > > growth to tbs2910 for the topic of this series since it disables
> > > EFI_LOADER entirely, so why is it a problem?
> >
> > The partition changes are going to add some size anyway, I expect. I
> > have not actually analysed it though. Perhaps we can just disable a
> > filesystem?
>
> I was a bit too absolutist there, sorry.  Yes, a few hundreds of bytes
> here-and-there is probably a non issue.  But it shouldn't be kilobytes.
> It really shouldn't push things over the line.
>
> And on the tbs2910 side, Soeren, can you look at enabling LTO for this
> platform?  That would likely buy a good bit of space savings.  That
> might well be needed to do further DM migrations/etc.

Regards,
Simon

Re: [PATCH v3 08/19] dm: add tag support

[AKASHI Takahiro]

On Wed, Mar 09, 2022 at 01:41:04PM +0200, Ilias Apalodimas wrote:
> Hi Akashi-san,
> 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> > ---
> >  drivers/core/Makefile             |   2 +-
> >  drivers/core/root.c               |   2 +
> >  drivers/core/tag.c                | 139 ++++++++++++++++++++++++++++++
> >  include/asm-generic/global_data.h |   4 +
> >  include/dm/tag.h                  | 110 +++++++++++++++++++++++
> >  5 files changed, 256 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/core/tag.c
> >  create mode 100644 include/dm/tag.h
> > 
> > diff --git a/drivers/core/Makefile b/drivers/core/Makefile
> > index 5edd4e413576..3742e7574525 100644
> > --- a/drivers/core/Makefile
> > +++ b/drivers/core/Makefile
> > @@ -2,7 +2,7 @@
> >  #
> >  # Copyright (c) 2013 Google, Inc
> >  
> > -obj-y	+= device.o fdtaddr.o lists.o root.o uclass.o util.o
> > +obj-y	+= device.o fdtaddr.o lists.o root.o uclass.o util.o tag.o
> >  obj-$(CONFIG_$(SPL_TPL_)ACPIGEN) += acpi.o
> >  obj-$(CONFIG_DEVRES) += devres.o
> >  obj-$(CONFIG_$(SPL_)DM_DEVICE_REMOVE)	+= device-remove.o
> > diff --git a/drivers/core/root.c b/drivers/core/root.c
> > index 8efb4256b27e..86b3884fc674 100644
> > --- a/drivers/core/root.c
> > +++ b/drivers/core/root.c
> > @@ -199,6 +199,8 @@ int dm_init(bool of_live)
> >  			return ret;
> >  	}
> >  
> > +	INIT_LIST_HEAD((struct list_head *)&gd->dmtag_list);
> > +
> >  	return 0;
> >  }
> >  
> > diff --git a/drivers/core/tag.c b/drivers/core/tag.c
> > new file mode 100644
> > index 000000000000..6829bcd8806c
> > --- /dev/null
> > +++ b/drivers/core/tag.c
> > @@ -0,0 +1,139 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2021 Linaro Limited
> > + *			Author: AKASHI Takahiro
> > + */
> > +
> > +#include <malloc.h>
> > +#include <asm/global_data.h>
> > +#include <dm/tag.h>
> > +#include <linux/err.h>
> > +#include <linux/list.h>
> > +#include <linux/types.h>
> > +
> > +struct udevice;
> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > +int dev_tag_set_ptr(struct udevice *dev, enum dm_tag_t tag, void *ptr)
> > +{
> > +	struct dmtag_node *node;
> > +
> > +	if (!dev || tag >= DM_TAG_COUNT)
> > +		return -EINVAL;
> > +
> > +	list_for_each_entry(node, &gd->dmtag_list, sibling) {
> > +		if (node->dev == dev && node->tag == tag)
> > +			return -EEXIST;
> > +	}
> > +
> > +	node = calloc(sizeof(*node), 1);
> > +	if (!node)
> > +		return -ENOSPC;
> 
> Isn't -ENOMEM better here?

Ah, yes.

> > +
> > +	node->dev = dev;
> > +	node->tag = tag;
> > +	node->ptr = ptr;
> > +	list_add_tail(&node->sibling, (struct list_head *)&gd->dmtag_list);
> > +
> > +	return 0;
> > +}
> > +
> > +int dev_tag_set_val(struct udevice *dev, enum dm_tag_t tag, ulong val)
> 
> Is this used anywhere else apart from selftests?

Currently, no. It exists just for future simpler use cases.

-Takahiro Akashi

> > +{
> > +	struct dmtag_node *node;
> 
> 
> Thanks
> /Ilias

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[Simon Glass]

Hi Tom, Takahiro,

On Wed, 9 Mar 2022 at 08:33, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Tom,
>
> On Wed, 9 Mar 2022 at 07:25, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Mar 08, 2022 at 08:10:38PM -0700, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Tue, 8 Mar 2022 at 20:00, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Tue, Mar 08, 2022 at 07:32:59PM -0700, Simon Glass wrote:
> > > > >  Hi Tom,
> > > > >
> > > > > On Tue, 8 Mar 2022 at 17:13, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Tue, Mar 08, 2022 at 02:20:15PM -0700, Simon Glass wrote:
> > > > > > > Hi Soeren,
> > > > > > >
> > > > > > > On Tue, 8 Mar 2022 at 12:15, Soeren Moch <smoch@web.de> wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > On 08.03.22 17:56, Simon Glass wrote:
> > > > > > > > > Hi,
> > > > > > > > >
> > > > > > > > > On Tue, 8 Mar 2022 at 09:49, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > > > >>
> > > > > > > > >> On 3/8/22 12:36, AKASHI Takahiro wrote:
> > > > > > > > >>> With this patch set[1] applied, UEFI subsystem maintains a list of its
> > > > > > > > >>> disk objects dynamically at runtime based on block device's probing.
> > > > > > > > >>> (See "issues" below.)
> > > > > > > > >>>
> > > > > > > > >>> [1]https://github.com/t-akashi/u-boot/tree/efi/dm_disk
> > > > > > > > >>
> > > > > > > > >> This series together with Simon's series breaks multiple boards due to
> > > > > > > > >> size constraints:
> > > > > > > > >>
> > > > > > > > >> https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/11197
> > > > > > > > >>
> > > > > > > > >> Please, investigate how to work around this issue.
> > > > > > > > >
> > > > > > > > > tbs2910 - perhaps we should just drop this board? It doesn't use
> > > > > > > > > DM_SERIAL and still uses OF_EMBED
> > > > > > > >
> > > > > > > > Are we again at the same point? You are breaking working boards with
> > > > > > > > (for these boards) useless additions, and all you come up with is
> > > > > > > > "remove this board". Of course without adding the board maintainer.
> > > > > > >
> > > > > > > I'm just expressing reasonable frustration that this board uses
> > > > > > > OF_EMBED and does not use DM_SERIAL, after all of this time. Why
> > > > > > > should the rest of the U-Boot developers care more about this board
> > > > > > > than the maintainer?
> > > > > >
> > > > > > Please keep in mind Simon that we've had zero releases with the
> > > > > > DM_SERIAL migration warning being posted, v2022.04 will be the first
> > > > > > one.
> > > > >
> > > > > Yes, understood :-) For OF_EMBED though...?
> > > >
> > > > No deadline and 50 boards.
> > >
> > > Er, there has been a build message about that since the beginning, so
> > > people ignored it. Do we really need to make the build fail for these
> > > sorts of things? Perhaps so, but it is a sad situation.
> >
> > Yes, in hind-sight, "don't do that" wasn't the right path.  It would be
> > a good idea to start a different thread and see what / how the platforms
> > can be migrated away.
>
> I think there is a use case for it now - e.g. booting Apple M1 which
> uses a separate bootloader. IMO a .img or .fit file would be better in
> some cases but people seem to be allergic to implementing U-Boot
> things in their code bases. We have the same requirement for the EFI
> app since UEFI does not implement the U-Boot .img file.
>
> So if we are going to support this, perhaps we should create a new
> option for it. But honestly I am just too weary to consider yet
> another migration. We need to finish some, e.g. Kconfig.

Back to the original topic, I found that some partition code is
present in SPL and cannot be dropped. This is a long-standing thing
but matters a little more now that there is a driver and
blk_post_probe() contains some code.

This series is foundational in two ways:

- adding driver model support for partitions
- starting the clean-up of the EFI code to better use drive rmodel

Heinrich, can we please hold off on any new EFI changes in -next until
we can get this series landed? Any conflicts will set us back again.

I will see if I can send a little series to clean that up and to
reduce the board sizes for the non-SPL problems. With that, perhaps we
can apply both series within a few days?

[..]

Regards,
Simon

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[Soeren Moch]

On 09.03.22 16:33, Simon Glass wrote:
> Hi Tom,
>
> On Wed, 9 Mar 2022 at 07:25, Tom Rini <trini@konsulko.com> wrote:
>> On Tue, Mar 08, 2022 at 08:10:38PM -0700, Simon Glass wrote:
>>> Hi Tom,
>>>
>>> On Tue, 8 Mar 2022 at 20:00, Tom Rini <trini@konsulko.com> wrote:
>>>> On Tue, Mar 08, 2022 at 07:32:59PM -0700, Simon Glass wrote:
>>>>>   Hi Tom,
>>>>>
>>>>> On Tue, 8 Mar 2022 at 17:13, Tom Rini <trini@konsulko.com> wrote:
>>>>>> On Tue, Mar 08, 2022 at 02:20:15PM -0700, Simon Glass wrote:
>>>>>>> Hi Soeren,
>>>>>>>
>>>>>>> On Tue, 8 Mar 2022 at 12:15, Soeren Moch <smoch@web.de> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 08.03.22 17:56, Simon Glass wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On Tue, 8 Mar 2022 at 09:49, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>>>>>> On 3/8/22 12:36, AKASHI Takahiro wrote:
>>>>>>>>>>> With this patch set[1] applied, UEFI subsystem maintains a list of its
>>>>>>>>>>> disk objects dynamically at runtime based on block device's probing.
>>>>>>>>>>> (See "issues" below.)
>>>>>>>>>>>
>>>>>>>>>>> [1]https://github.com/t-akashi/u-boot/tree/efi/dm_disk
>>>>>>>>>> This series together with Simon's series breaks multiple boards due to
>>>>>>>>>> size constraints:
>>>>>>>>>>
>>>>>>>>>> https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/11197
>>>>>>>>>>
>>>>>>>>>> Please, investigate how to work around this issue.
>>>>>>>>> tbs2910 - perhaps we should just drop this board? It doesn't use
>>>>>>>>> DM_SERIAL and still uses OF_EMBED
>>>>>>>> Are we again at the same point? You are breaking working boards with
>>>>>>>> (for these boards) useless additions, and all you come up with is
>>>>>>>> "remove this board". Of course without adding the board maintainer.
>>>>>>> I'm just expressing reasonable frustration that this board uses
>>>>>>> OF_EMBED and does not use DM_SERIAL, after all of this time. Why
>>>>>>> should the rest of the U-Boot developers care more about this board
>>>>>>> than the maintainer?
I cannot see what is is reasonable here.

I care about this board, what you can simply see by the fact that I even
picked this thread from the mailing list while you "forgot" to cc me.

OF_EMBED and DM_SERIAL are not at all related to EFI or size constraints.

I'm surprised that you can speak for "the rest of the U-Boot
developers", and that you want to push your frustration onto tbs2910
developers and users. Why is it my fault that other people add code size
without guarding config options? Why is it my fault that nobody informed
me that there is again a size problem?
>>>>>> Please keep in mind Simon that we've had zero releases with the
>>>>>> DM_SERIAL migration warning being posted, v2022.04 will be the first
>>>>>> one.
>>>>> Yes, understood :-) For OF_EMBED though...?
>>>> No deadline and 50 boards.
>>> Er, there has been a build message about that since the beginning, so
>>> people ignored it. Do we really need to make the build fail for these
>>> sorts of things? Perhaps so, but it is a sad situation.
>> Yes, in hind-sight, "don't do that" wasn't the right path.  It would be
>> a good idea to start a different thread and see what / how the platforms
>> can be migrated away.
For tbs2910 this is just a workaround for a strange property of the imx
build system. OF_SEPARATE created a broken u-boot.imx when I tried last
time.
> I think there is a use case for it now - e.g. booting Apple M1 which
> uses a separate bootloader. IMO a .img or .fit file would be better in
> some cases but people seem to be allergic to implementing U-Boot
> things in their code bases. We have the same requirement for the EFI
> app since UEFI does not implement the U-Boot .img file.
>
> So if we are going to support this, perhaps we should create a new
> option for it. But honestly I am just too weary to consider yet
> another migration. We need to finish some, e.g. Kconfig.
>
>>>>> It was actually quite hard to add a migration message until we added
>>>>> the CONFIG_SERIAL base thing and that was a pain to do.
>>>>>
>>>>> For those of us who take on larger refactors etc., we end up spending
>>>>> a lot of our time on these few platforms. I'm not picking on tbs2910in
>>>>> in particular.
>>>> Well, the flip side of the problem here is that there's a number of
>>>> platforms with real constraints to them and it keeps being "can we drop
>>>> this yet?" without CC'ing the board maintainer on the series that once
>>>> again pushes a given platform to the limit.  I would expect no size
>>>> growth to tbs2910 for the topic of this series since it disables
>>>> EFI_LOADER entirely, so why is it a problem?
>>> The partition changes are going to add some size anyway, I expect. I
>>> have not actually analysed it though. Perhaps we can just disable a
>>> filesystem?
OK, you did not even analyse where the problem comes from. But disabling
user visible functionality on my board is the natural solution to that?
Strange.
>> I was a bit too absolutist there, sorry.  Yes, a few hundreds of bytes
>> here-and-there is probably a non issue.  But it shouldn't be kilobytes.
>> It really shouldn't push things over the line.
>>
>> And on the tbs2910 side, Soeren, can you look at enabling LTO for this
>> platform?  That would likely buy a good bit of space savings.  That
>> might well be needed to do further DM migrations/etc.
I'm not familiar with LTO in U-Boot, but will have a look at the weekend.

Regards,
Soeren
> Regards,
> Simon

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[Simon Glass]

Hi Soeren,

On Fri, 11 Mar 2022 at 15:43, Soeren Moch <smoch@web.de> wrote:
>
>
>
> On 09.03.22 16:33, Simon Glass wrote:
> > Hi Tom,
> >
> > On Wed, 9 Mar 2022 at 07:25, Tom Rini <trini@konsulko.com> wrote:
> >> On Tue, Mar 08, 2022 at 08:10:38PM -0700, Simon Glass wrote:
> >>> Hi Tom,
> >>>
> >>> On Tue, 8 Mar 2022 at 20:00, Tom Rini <trini@konsulko.com> wrote:
> >>>> On Tue, Mar 08, 2022 at 07:32:59PM -0700, Simon Glass wrote:
> >>>>>   Hi Tom,
> >>>>>
> >>>>> On Tue, 8 Mar 2022 at 17:13, Tom Rini <trini@konsulko.com> wrote:
> >>>>>> On Tue, Mar 08, 2022 at 02:20:15PM -0700, Simon Glass wrote:
> >>>>>>> Hi Soeren,
> >>>>>>>
> >>>>>>> On Tue, 8 Mar 2022 at 12:15, Soeren Moch <smoch@web.de> wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 08.03.22 17:56, Simon Glass wrote:
> >>>>>>>>> Hi,
> >>>>>>>>>
> >>>>>>>>> On Tue, 8 Mar 2022 at 09:49, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>>>>>>>> On 3/8/22 12:36, AKASHI Takahiro wrote:
> >>>>>>>>>>> With this patch set[1] applied, UEFI subsystem maintains a list of its
> >>>>>>>>>>> disk objects dynamically at runtime based on block device's probing.
> >>>>>>>>>>> (See "issues" below.)
> >>>>>>>>>>>
> >>>>>>>>>>> [1]https://github.com/t-akashi/u-boot/tree/efi/dm_disk
> >>>>>>>>>> This series together with Simon's series breaks multiple boards due to
> >>>>>>>>>> size constraints:
> >>>>>>>>>>
> >>>>>>>>>> https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/11197
> >>>>>>>>>>
> >>>>>>>>>> Please, investigate how to work around this issue.
> >>>>>>>>> tbs2910 - perhaps we should just drop this board? It doesn't use
> >>>>>>>>> DM_SERIAL and still uses OF_EMBED
> >>>>>>>> Are we again at the same point? You are breaking working boards with
> >>>>>>>> (for these boards) useless additions, and all you come up with is
> >>>>>>>> "remove this board". Of course without adding the board maintainer.
> >>>>>>> I'm just expressing reasonable frustration that this board uses
> >>>>>>> OF_EMBED and does not use DM_SERIAL, after all of this time. Why
> >>>>>>> should the rest of the U-Boot developers care more about this board
> >>>>>>> than the maintainer?
> I cannot see what is is reasonable here.
>
> I care about this board, what you can simply see by the fact that I even
> picked this thread from the mailing list while you "forgot" to cc me.

This is the patch I sent:

[PATCH 2/5] tbs2910: Disable ext4 write

It shows that you are on cc. What are you referring to?

>
> OF_EMBED and DM_SERIAL are not at all related to EFI or size constraints.
>
> I'm surprised that you can speak for "the rest of the U-Boot
> developers", and that you want to push your frustration onto tbs2910
> developers and users. Why is it my fault that other people add code size
> without guarding config options? Why is it my fault that nobody informed
> me that there is again a size problem?

Your board is up against the limit and this causes problems. Please
take a look and see how you can add some margin. Takahiro's series
does add size and this is unavoidable. See my series of today for some
fixes for the SPL size, but for U-Boot proper we have to accept the
growth.

> >>>>>> Please keep in mind Simon that we've had zero releases with the
> >>>>>> DM_SERIAL migration warning being posted, v2022.04 will be the first
> >>>>>> one.
> >>>>> Yes, understood :-) For OF_EMBED though...?
> >>>> No deadline and 50 boards.
> >>> Er, there has been a build message about that since the beginning, so
> >>> people ignored it. Do we really need to make the build fail for these
> >>> sorts of things? Perhaps so, but it is a sad situation.
> >> Yes, in hind-sight, "don't do that" wasn't the right path.  It would be
> >> a good idea to start a different thread and see what / how the platforms
> >> can be migrated away.
> For tbs2910 this is just a workaround for a strange property of the imx
> build system. OF_SEPARATE created a broken u-boot.imx when I tried last
> time.

OK, that is worth digging in to.


> > I think there is a use case for it now - e.g. booting Apple M1 which
> > uses a separate bootloader. IMO a .img or .fit file would be better in
> > some cases but people seem to be allergic to implementing U-Boot
> > things in their code bases. We have the same requirement for the EFI
> > app since UEFI does not implement the U-Boot .img file.
> >
> > So if we are going to support this, perhaps we should create a new
> > option for it. But honestly I am just too weary to consider yet
> > another migration. We need to finish some, e.g. Kconfig.
> >
> >>>>> It was actually quite hard to add a migration message until we added
> >>>>> the CONFIG_SERIAL base thing and that was a pain to do.
> >>>>>
> >>>>> For those of us who take on larger refactors etc., we end up spending
> >>>>> a lot of our time on these few platforms. I'm not picking on tbs2910in
> >>>>> in particular.
> >>>> Well, the flip side of the problem here is that there's a number of
> >>>> platforms with real constraints to them and it keeps being "can we drop
> >>>> this yet?" without CC'ing the board maintainer on the series that once
> >>>> again pushes a given platform to the limit.  I would expect no size
> >>>> growth to tbs2910 for the topic of this series since it disables
> >>>> EFI_LOADER entirely, so why is it a problem?
> >>> The partition changes are going to add some size anyway, I expect. I
> >>> have not actually analysed it though. Perhaps we can just disable a
> >>> filesystem?
> OK, you did not even analyse where the problem comes from. But disabling
> user visible functionality on my board is the natural solution to that?
> Strange.

As above, please create some space so people can continue to develop.
There are refactors and features updates which require more code
space. It is somewhat rare, but it happens perhaps every year.

> >> I was a bit too absolutist there, sorry.  Yes, a few hundreds of bytes
> >> here-and-there is probably a non issue.  But it shouldn't be kilobytes.
> >> It really shouldn't push things over the line.
> >>
> >> And on the tbs2910 side, Soeren, can you look at enabling LTO for this
> >> platform?  That would likely buy a good bit of space savings.  That
> >> might well be needed to do further DM migrations/etc.
> I'm not familiar with LTO in U-Boot, but will have a look at the weekend.

OK, I suggest getting it several KB under the limit if you can, or
perhaps even drop the limit.

Regards,
Simon

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[Soeren Moch]

Hi Simon,

On 12.03.22 06:02, Simon Glass wrote:
> Hi Soeren,
>
> On Fri, 11 Mar 2022 at 15:43, Soeren Moch<smoch@web.de>  wrote:
>
>
>
> On 09.03.22 16:33, Simon Glass wrote:
>>> Hi Tom,
>>>
>>> On Wed, 9 Mar 2022 at 07:25, Tom Rini<trini@konsulko.com>  wrote:
>>>> On Tue, Mar 08, 2022 at 08:10:38PM -0700, Simon Glass wrote:
>>>>> Hi Tom,
>>>>>
>>>>> On Tue, 8 Mar 2022 at 20:00, Tom Rini<trini@konsulko.com>  wrote:
>>>>>> On Tue, Mar 08, 2022 at 07:32:59PM -0700, Simon Glass wrote:
>>>>>>>    Hi Tom,
>>>>>>>
>>>>>>> On Tue, 8 Mar 2022 at 17:13, Tom Rini<trini@konsulko.com>  wrote:
>>>>>>>> On Tue, Mar 08, 2022 at 02:20:15PM -0700, Simon Glass wrote:
>>>>>>>>> Hi Soeren,
>>>>>>>>>
>>>>>>>>> On Tue, 8 Mar 2022 at 12:15, Soeren Moch<smoch@web.de>  wrote:
>>>>>>>>>>
>>>>>>>>>> On 08.03.22 17:56, Simon Glass wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> On Tue, 8 Mar 2022 at 09:49, Heinrich Schuchardt<xypron.glpk@gmx.de>  wrote:
>>>>>>>>>>>> On 3/8/22 12:36, AKASHI Takahiro wrote:
>>>>>>>>>>>>> With this patch set[1] applied, UEFI subsystem maintains a list of its
>>>>>>>>>>>>> disk objects dynamically at runtime based on block device's probing.
>>>>>>>>>>>>> (See "issues" below.)
>>>>>>>>>>>>>
>>>>>>>>>>>>> [1]https://github.com/t-akashi/u-boot/tree/efi/dm_disk
>>>>>>>>>>>> This series together with Simon's series breaks multiple boards due to
>>>>>>>>>>>> size constraints:
>>>>>>>>>>>>
>>>>>>>>>>>> https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/11197
>>>>>>>>>>>>
>>>>>>>>>>>> Please, investigate how to work around this issue.
>>>>>>>>>>> tbs2910 - perhaps we should just drop this board? It doesn't use
>>>>>>>>>>> DM_SERIAL and still uses OF_EMBED
>>>>>>>>>> Are we again at the same point? You are breaking working boards with
>>>>>>>>>> (for these boards) useless additions, and all you come up with is
>>>>>>>>>> "remove this board". Of course without adding the board maintainer.
>>>>>>>>> I'm just expressing reasonable frustration that this board uses
>>>>>>>>> OF_EMBED and does not use DM_SERIAL, after all of this time. Why
>>>>>>>>> should the rest of the U-Boot developers care more about this board
>>>>>>>>> than the maintainer?
>> I cannot see what is is reasonable here.
>>
>> I care about this board, what you can simply see by the fact that I even
>> picked this thread from the mailing list while you "forgot" to cc me.
> This is the patch I sent:
>
> [PATCH 2/5] tbs2910: Disable ext4 write
>
> It shows that you are on cc. What are you referring to?
I'm referring to the exact same email thread that I answered in, which
btw. is still this exact same thread for this answer. Why should I refer
to the totally different email thread you cited here?
>> OF_EMBED and DM_SERIAL are not at all related to EFI or size constraints.
>>
>> I'm surprised that you can speak for "the rest of the U-Boot
>> developers", and that you want to push your frustration onto tbs2910
>> developers and users. Why is it my fault that other people add code size
>> without guarding config options? Why is it my fault that nobody informed
>> me that there is again a size problem?
> Your board is up against the limit and this causes problems. Please
> take a look and see how you can add some margin. Takahiro's series
> does add size and this is unavoidable. See my series of today for some
> fixes for the SPL size, but for U-Boot proper we have to accept the
> growth.
As it stands here this is just your opinion. Why exactly is this
unavoidable?
>
>>>>>>>> Please keep in mind Simon that we've had zero releases with the
>>>>>>>> DM_SERIAL migration warning being posted, v2022.04 will be the first
>>>>>>>> one.
>>>>>>> Yes, understood :-) For OF_EMBED though...?
>>>>>> No deadline and 50 boards.
>>>>> Er, there has been a build message about that since the beginning, so
>>>>> people ignored it. Do we really need to make the build fail for these
>>>>> sorts of things? Perhaps so, but it is a sad situation.
>>>> Yes, in hind-sight, "don't do that" wasn't the right path.  It would be
>>>> a good idea to start a different thread and see what / how the platforms
>>>> can be migrated away.
>> For tbs2910 this is just a workaround for a strange property of the imx
>> build system. OF_SEPARATE created a broken u-boot.imx when I tried last
>> time.
> OK, that is worth digging in to.
Probably. I'm happy to test whatever someone comes up with.
>
>
>>> I think there is a use case for it now - e.g. booting Apple M1 which
>>> uses a separate bootloader. IMO a .img or .fit file would be better in
>>> some cases but people seem to be allergic to implementing U-Boot
>>> things in their code bases. We have the same requirement for the EFI
>>> app since UEFI does not implement the U-Boot .img file.
>>>
>>> So if we are going to support this, perhaps we should create a new
>>> option for it. But honestly I am just too weary to consider yet
>>> another migration. We need to finish some, e.g. Kconfig.
>>>
>>>>>>> It was actually quite hard to add a migration message until we added
>>>>>>> the CONFIG_SERIAL base thing and that was a pain to do.
>>>>>>>
>>>>>>> For those of us who take on larger refactors etc., we end up spending
>>>>>>> a lot of our time on these few platforms. I'm not picking on tbs2910in
>>>>>>> in particular.
>>>>>> Well, the flip side of the problem here is that there's a number of
>>>>>> platforms with real constraints to them and it keeps being "can we drop
>>>>>> this yet?" without CC'ing the board maintainer on the series that once
>>>>>> again pushes a given platform to the limit.  I would expect no size
>>>>>> growth to tbs2910 for the topic of this series since it disables
>>>>>> EFI_LOADER entirely, so why is it a problem?
>>>>> The partition changes are going to add some size anyway, I expect. I
>>>>> have not actually analysed it though. Perhaps we can just disable a
>>>>> filesystem?
>> OK, you did not even analyse where the problem comes from. But disabling
>> user visible functionality on my board is the natural solution to that?
>> Strange.
> As above, please create some space so people can continue to develop.
> There are refactors and features updates which require more code
> space. It is somewhat rare, but it happens perhaps every year.
It has always been u-boot policy that additional new features should not
break existing boards, usually by disabling these new features in defconfig.
It is also not new that there are boards with size constraints.

If someone causes regressions, then I at least expect that this is
thoroughly analysed.
>>>> I was a bit too absolutist there, sorry.  Yes, a few hundreds of bytes
>>>> here-and-there is probably a non issue.  But it shouldn't be kilobytes.
>>>> It really shouldn't push things over the line.
>>>>
>>>> And on the tbs2910 side, Soeren, can you look at enabling LTO for this
>>>> platform?  That would likely buy a good bit of space savings.  That
>>>> might well be needed to do further DM migrations/etc.
>> I'm not familiar with LTO in U-Boot, but will have a look at the weekend.
> OK, I suggest getting it several KB under the limit if you can, or
> perhaps even drop the limit.
I already reduced tbs2910 image size several times by substantial
amounts. And this is becoming more and more difficult. The size limit is
real.

Thanks Tom for the LTO suggestion, this will buy us another round/./ I
sent a patch for that.

But please, everyone, be careful with additional code size for existing
boards. Additional code size is not unavoidable for disabled new
features. You just did not try hard enough.

Regards,
Soeren
>
> Regards,
> Simon

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 9069 bytes --]

On Mon, Mar 14, 2022 at 09:27:40AM +0100, Soeren Moch wrote:
> Hi Simon,
> 
> On 12.03.22 06:02, Simon Glass wrote:
> > Hi Soeren,
> > 
> > On Fri, 11 Mar 2022 at 15:43, Soeren Moch<smoch@web.de>  wrote:
> > 
> > 
> > 
> > On 09.03.22 16:33, Simon Glass wrote:
> > > > Hi Tom,
> > > > 
> > > > On Wed, 9 Mar 2022 at 07:25, Tom Rini<trini@konsulko.com>  wrote:
> > > > > On Tue, Mar 08, 2022 at 08:10:38PM -0700, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > > 
> > > > > > On Tue, 8 Mar 2022 at 20:00, Tom Rini<trini@konsulko.com>  wrote:
> > > > > > > On Tue, Mar 08, 2022 at 07:32:59PM -0700, Simon Glass wrote:
> > > > > > > >    Hi Tom,
> > > > > > > > 
> > > > > > > > On Tue, 8 Mar 2022 at 17:13, Tom Rini<trini@konsulko.com>  wrote:
> > > > > > > > > On Tue, Mar 08, 2022 at 02:20:15PM -0700, Simon Glass wrote:
> > > > > > > > > > Hi Soeren,
> > > > > > > > > > 
> > > > > > > > > > On Tue, 8 Mar 2022 at 12:15, Soeren Moch<smoch@web.de>  wrote:
> > > > > > > > > > > 
> > > > > > > > > > > On 08.03.22 17:56, Simon Glass wrote:
> > > > > > > > > > > > Hi,
> > > > > > > > > > > > 
> > > > > > > > > > > > On Tue, 8 Mar 2022 at 09:49, Heinrich Schuchardt<xypron.glpk@gmx.de>  wrote:
> > > > > > > > > > > > > On 3/8/22 12:36, AKASHI Takahiro wrote:
> > > > > > > > > > > > > > With this patch set[1] applied, UEFI subsystem maintains a list of its
> > > > > > > > > > > > > > disk objects dynamically at runtime based on block device's probing.
> > > > > > > > > > > > > > (See "issues" below.)
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > [1]https://github.com/t-akashi/u-boot/tree/efi/dm_disk
> > > > > > > > > > > > > This series together with Simon's series breaks multiple boards due to
> > > > > > > > > > > > > size constraints:
> > > > > > > > > > > > > 
> > > > > > > > > > > > > https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/11197
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Please, investigate how to work around this issue.
> > > > > > > > > > > > tbs2910 - perhaps we should just drop this board? It doesn't use
> > > > > > > > > > > > DM_SERIAL and still uses OF_EMBED
> > > > > > > > > > > Are we again at the same point? You are breaking working boards with
> > > > > > > > > > > (for these boards) useless additions, and all you come up with is
> > > > > > > > > > > "remove this board". Of course without adding the board maintainer.
> > > > > > > > > > I'm just expressing reasonable frustration that this board uses
> > > > > > > > > > OF_EMBED and does not use DM_SERIAL, after all of this time. Why
> > > > > > > > > > should the rest of the U-Boot developers care more about this board
> > > > > > > > > > than the maintainer?
> > > I cannot see what is is reasonable here.
> > > 
> > > I care about this board, what you can simply see by the fact that I even
> > > picked this thread from the mailing list while you "forgot" to cc me.
> > This is the patch I sent:
> > 
> > [PATCH 2/5] tbs2910: Disable ext4 write
> > 
> > It shows that you are on cc. What are you referring to?
> I'm referring to the exact same email thread that I answered in, which
> btw. is still this exact same thread for this answer. Why should I refer
> to the totally different email thread you cited here?
> > > OF_EMBED and DM_SERIAL are not at all related to EFI or size constraints.
> > > 
> > > I'm surprised that you can speak for "the rest of the U-Boot
> > > developers", and that you want to push your frustration onto tbs2910
> > > developers and users. Why is it my fault that other people add code size
> > > without guarding config options? Why is it my fault that nobody informed
> > > me that there is again a size problem?
> > Your board is up against the limit and this causes problems. Please
> > take a look and see how you can add some margin. Takahiro's series
> > does add size and this is unavoidable. See my series of today for some
> > fixes for the SPL size, but for U-Boot proper we have to accept the
> > growth.
> As it stands here this is just your opinion. Why exactly is this
> unavoidable?
> > 
> > > > > > > > > Please keep in mind Simon that we've had zero releases with the
> > > > > > > > > DM_SERIAL migration warning being posted, v2022.04 will be the first
> > > > > > > > > one.
> > > > > > > > Yes, understood :-) For OF_EMBED though...?
> > > > > > > No deadline and 50 boards.
> > > > > > Er, there has been a build message about that since the beginning, so
> > > > > > people ignored it. Do we really need to make the build fail for these
> > > > > > sorts of things? Perhaps so, but it is a sad situation.
> > > > > Yes, in hind-sight, "don't do that" wasn't the right path.  It would be
> > > > > a good idea to start a different thread and see what / how the platforms
> > > > > can be migrated away.
> > > For tbs2910 this is just a workaround for a strange property of the imx
> > > build system. OF_SEPARATE created a broken u-boot.imx when I tried last
> > > time.
> > OK, that is worth digging in to.
> Probably. I'm happy to test whatever someone comes up with.
> > 
> > 
> > > > I think there is a use case for it now - e.g. booting Apple M1 which
> > > > uses a separate bootloader. IMO a .img or .fit file would be better in
> > > > some cases but people seem to be allergic to implementing U-Boot
> > > > things in their code bases. We have the same requirement for the EFI
> > > > app since UEFI does not implement the U-Boot .img file.
> > > > 
> > > > So if we are going to support this, perhaps we should create a new
> > > > option for it. But honestly I am just too weary to consider yet
> > > > another migration. We need to finish some, e.g. Kconfig.
> > > > 
> > > > > > > > It was actually quite hard to add a migration message until we added
> > > > > > > > the CONFIG_SERIAL base thing and that was a pain to do.
> > > > > > > > 
> > > > > > > > For those of us who take on larger refactors etc., we end up spending
> > > > > > > > a lot of our time on these few platforms. I'm not picking on tbs2910in
> > > > > > > > in particular.
> > > > > > > Well, the flip side of the problem here is that there's a number of
> > > > > > > platforms with real constraints to them and it keeps being "can we drop
> > > > > > > this yet?" without CC'ing the board maintainer on the series that once
> > > > > > > again pushes a given platform to the limit.  I would expect no size
> > > > > > > growth to tbs2910 for the topic of this series since it disables
> > > > > > > EFI_LOADER entirely, so why is it a problem?
> > > > > > The partition changes are going to add some size anyway, I expect. I
> > > > > > have not actually analysed it though. Perhaps we can just disable a
> > > > > > filesystem?
> > > OK, you did not even analyse where the problem comes from. But disabling
> > > user visible functionality on my board is the natural solution to that?
> > > Strange.
> > As above, please create some space so people can continue to develop.
> > There are refactors and features updates which require more code
> > space. It is somewhat rare, but it happens perhaps every year.
> It has always been u-boot policy that additional new features should not
> break existing boards, usually by disabling these new features in defconfig.
> It is also not new that there are boards with size constraints.
> 
> If someone causes regressions, then I at least expect that this is
> thoroughly analysed.
> > > > > I was a bit too absolutist there, sorry.  Yes, a few hundreds of bytes
> > > > > here-and-there is probably a non issue.  But it shouldn't be kilobytes.
> > > > > It really shouldn't push things over the line.
> > > > > 
> > > > > And on the tbs2910 side, Soeren, can you look at enabling LTO for this
> > > > > platform?  That would likely buy a good bit of space savings.  That
> > > > > might well be needed to do further DM migrations/etc.
> > > I'm not familiar with LTO in U-Boot, but will have a look at the weekend.
> > OK, I suggest getting it several KB under the limit if you can, or
> > perhaps even drop the limit.
> I already reduced tbs2910 image size several times by substantial
> amounts. And this is becoming more and more difficult. The size limit is
> real.
> 
> Thanks Tom for the LTO suggestion, this will buy us another round/./ I
> sent a patch for that.
> 
> But please, everyone, be careful with additional code size for existing
> boards. Additional code size is not unavoidable for disabled new
> features. You just did not try hard enough.

I just want to emphasize the last point here.  We need to make sure
we're making platforms better, not just bigger.  And to preempt "EFI
keeps growing", yes, in reasonably small amounts, implementing a spec
(or fixing bugs against a spec) that the semi vendor is telling everyone
to make sure their hardware is compliant with.  And I check all of the
growth for everyone, all of the time, for everything.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[Simon Glass]

Hi Soeren,

[I think you sent your email with html or something so it is a big
mangled. I'll just add one comment]

On Mon, 14 Mar 2022 at 02:27, Soeren Moch <smoch@web.de> wrote:
>
> Hi Simon,
>
> On 12.03.22 06:02, Simon Glass wrote:
>
> Hi Soeren,
>
> On Fri, 11 Mar 2022 at 15:43, Soeren Moch <smoch@web.de> wrote:
>
>
>
> On 09.03.22 16:33, Simon Glass wrote:
>
> Hi Tom,
>
> On Wed, 9 Mar 2022 at 07:25, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Mar 08, 2022 at 08:10:38PM -0700, Simon Glass wrote:
>
> Hi Tom,
>
> On Tue, 8 Mar 2022 at 20:00, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Mar 08, 2022 at 07:32:59PM -0700, Simon Glass wrote:
>
>   Hi Tom,
>
> On Tue, 8 Mar 2022 at 17:13, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Mar 08, 2022 at 02:20:15PM -0700, Simon Glass wrote:
>
> Hi Soeren,
>
> On Tue, 8 Mar 2022 at 12:15, Soeren Moch <smoch@web.de> wrote:
>
> On 08.03.22 17:56, Simon Glass wrote:
>
> Hi,
>
> On Tue, 8 Mar 2022 at 09:49, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 3/8/22 12:36, AKASHI Takahiro wrote:
>
> With this patch set[1] applied, UEFI subsystem maintains a list of its
> disk objects dynamically at runtime based on block device's probing.
> (See "issues" below.)
>
> [1]https://github.com/t-akashi/u-boot/tree/efi/dm_disk
>
> This series together with Simon's series breaks multiple boards due to
> size constraints:
>
> https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/11197
>
> Please, investigate how to work around this issue.
>
> tbs2910 - perhaps we should just drop this board? It doesn't use
> DM_SERIAL and still uses OF_EMBED
>
> Are we again at the same point? You are breaking working boards with
> (for these boards) useless additions, and all you come up with is
> "remove this board". Of course without adding the board maintainer.
>
> I'm just expressing reasonable frustration that this board uses
> OF_EMBED and does not use DM_SERIAL, after all of this time. Why
> should the rest of the U-Boot developers care more about this board
> than the maintainer?
>
> I cannot see what is is reasonable here.
>
> I care about this board, what you can simply see by the fact that I even
> picked this thread from the mailing list while you "forgot" to cc me.
>
> This is the patch I sent:
>
> [PATCH 2/5] tbs2910: Disable ext4 write
>
> It shows that you are on cc. What are you referring to?
>
> I'm referring to the exact same email thread that I answered in, which btw. is still this exact same thread for this answer. Why should I refer to the totally different email thread you cited here?
>
> OF_EMBED and DM_SERIAL are not at all related to EFI or size constraints.
>
> I'm surprised that you can speak for "the rest of the U-Boot
> developers", and that you want to push your frustration onto tbs2910
> developers and users. Why is it my fault that other people add code size
> without guarding config options? Why is it my fault that nobody informed
> me that there is again a size problem?
>
> Your board is up against the limit and this causes problems. Please
> take a look and see how you can add some margin. Takahiro's series
> does add size and this is unavoidable. See my series of today for some
> fixes for the SPL size, but for U-Boot proper we have to accept the
> growth.
>
> As it stands here this is just your opinion. Why exactly is this unavoidable?
>
> Please keep in mind Simon that we've had zero releases with the
> DM_SERIAL migration warning being posted, v2022.04 will be the first
> one.
>
> Yes, understood :-) For OF_EMBED though...?
>
> No deadline and 50 boards.
>
> Er, there has been a build message about that since the beginning, so
> people ignored it. Do we really need to make the build fail for these
> sorts of things? Perhaps so, but it is a sad situation.
>
> Yes, in hind-sight, "don't do that" wasn't the right path.  It would be
> a good idea to start a different thread and see what / how the platforms
> can be migrated away.
>
> For tbs2910 this is just a workaround for a strange property of the imx
> build system. OF_SEPARATE created a broken u-boot.imx when I tried last
> time.
>
> OK, that is worth digging in to.
>
> Probably. I'm happy to test whatever someone comes up with.
>
>
> I think there is a use case for it now - e.g. booting Apple M1 which
> uses a separate bootloader. IMO a .img or .fit file would be better in
> some cases but people seem to be allergic to implementing U-Boot
> things in their code bases. We have the same requirement for the EFI
> app since UEFI does not implement the U-Boot .img file.
>
> So if we are going to support this, perhaps we should create a new
> option for it. But honestly I am just too weary to consider yet
> another migration. We need to finish some, e.g. Kconfig.
>
> It was actually quite hard to add a migration message until we added
> the CONFIG_SERIAL base thing and that was a pain to do.
>
> For those of us who take on larger refactors etc., we end up spending
> a lot of our time on these few platforms. I'm not picking on tbs2910in
> in particular.
>
> Well, the flip side of the problem here is that there's a number of
> platforms with real constraints to them and it keeps being "can we drop
> this yet?" without CC'ing the board maintainer on the series that once
> again pushes a given platform to the limit.  I would expect no size
> growth to tbs2910 for the topic of this series since it disables
> EFI_LOADER entirely, so why is it a problem?
>
> The partition changes are going to add some size anyway, I expect. I
> have not actually analysed it though. Perhaps we can just disable a
> filesystem?
>
> OK, you did not even analyse where the problem comes from. But disabling
> user visible functionality on my board is the natural solution to that?
> Strange.
>
> As above, please create some space so people can continue to develop.
> There are refactors and features updates which require more code
> space. It is somewhat rare, but it happens perhaps every year.
>
> It has always been u-boot policy that additional new features should not break existing boards, usually by disabling these new features in defconfig.
> It is also not new that there are boards with size constraints.
>
> If someone causes regressions, then I at least expect that this is thoroughly analysed.
>
> I was a bit too absolutist there, sorry.  Yes, a few hundreds of bytes
> here-and-there is probably a non issue.  But it shouldn't be kilobytes.
> It really shouldn't push things over the line.
>
> And on the tbs2910 side, Soeren, can you look at enabling LTO for this
> platform?  That would likely buy a good bit of space savings.  That
> might well be needed to do further DM migrations/etc.
>
> I'm not familiar with LTO in U-Boot, but will have a look at the weekend.
>
> OK, I suggest getting it several KB under the limit if you can, or
> perhaps even drop the limit.
>
> I already reduced tbs2910 image size several times by substantial amounts. And this is becoming more and more difficult. The size limit is real.
>
> Thanks Tom for the LTO suggestion, this will buy us another round. I sent a patch for that.
>
> But please, everyone, be careful with additional code size for existing boards. Additional code size is not unavoidable for disabled new features. You just did not try hard enough.

Please take a look at Tahahiro's series and tell me how we can avoid
adding a driver for partitions, when the whole point of the series is
to add a driver for partitions :-)

Regards,
Simon

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[Soeren Moch]

Hi Simon,

On 14.03.22 18:08, Simon Glass wrote:
> Hi Soeren,
>
> [I think you sent your email with html or something so it is a big
> mangled. I'll just add one comment]
>
> On Mon, 14 Mar 2022 at 02:27, Soeren Moch <smoch@web.de> wrote:
>> Hi Simon,
>>
>> On 12.03.22 06:02, Simon Glass wrote:
>>
>> Hi Soeren,
>>
>> On Fri, 11 Mar 2022 at 15:43, Soeren Moch <smoch@web.de> wrote:
>>
>>
>>
>> On 09.03.22 16:33, Simon Glass wrote:
>>
>> Hi Tom,
>>
>> On Wed, 9 Mar 2022 at 07:25, Tom Rini <trini@konsulko.com> wrote:
>>
>> On Tue, Mar 08, 2022 at 08:10:38PM -0700, Simon Glass wrote:
>>
>> Hi Tom,
>>
>> On Tue, 8 Mar 2022 at 20:00, Tom Rini <trini@konsulko.com> wrote:
>>
>> On Tue, Mar 08, 2022 at 07:32:59PM -0700, Simon Glass wrote:
>>
>>    Hi Tom,
>>
>> On Tue, 8 Mar 2022 at 17:13, Tom Rini <trini@konsulko.com> wrote:
>>
>> On Tue, Mar 08, 2022 at 02:20:15PM -0700, Simon Glass wrote:
>>
>> Hi Soeren,
>>
>> On Tue, 8 Mar 2022 at 12:15, Soeren Moch <smoch@web.de> wrote:
>>
>> On 08.03.22 17:56, Simon Glass wrote:
>>
>> Hi,
>>
>> On Tue, 8 Mar 2022 at 09:49, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 3/8/22 12:36, AKASHI Takahiro wrote:
>>
>> With this patch set[1] applied, UEFI subsystem maintains a list of its
>> disk objects dynamically at runtime based on block device's probing.
>> (See "issues" below.)
>>
>> [1]https://github.com/t-akashi/u-boot/tree/efi/dm_disk
>>
>> This series together with Simon's series breaks multiple boards due to
>> size constraints:
>>
>> https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/11197
>>
>> Please, investigate how to work around this issue.
>>
>> tbs2910 - perhaps we should just drop this board? It doesn't use
>> DM_SERIAL and still uses OF_EMBED
>>
>> Are we again at the same point? You are breaking working boards with
>> (for these boards) useless additions, and all you come up with is
>> "remove this board". Of course without adding the board maintainer.
>>
>> I'm just expressing reasonable frustration that this board uses
>> OF_EMBED and does not use DM_SERIAL, after all of this time. Why
>> should the rest of the U-Boot developers care more about this board
>> than the maintainer?
>>
>> I cannot see what is is reasonable here.
>>
>> I care about this board, what you can simply see by the fact that I even
>> picked this thread from the mailing list while you "forgot" to cc me.
>>
>> This is the patch I sent:
>>
>> [PATCH 2/5] tbs2910: Disable ext4 write
>>
>> It shows that you are on cc. What are you referring to?
>>
>> I'm referring to the exact same email thread that I answered in, which btw. is still this exact same thread for this answer. Why should I refer to the totally different email thread you cited here?
>>
>> OF_EMBED and DM_SERIAL are not at all related to EFI or size constraints.
>>
>> I'm surprised that you can speak for "the rest of the U-Boot
>> developers", and that you want to push your frustration onto tbs2910
>> developers and users. Why is it my fault that other people add code size
>> without guarding config options? Why is it my fault that nobody informed
>> me that there is again a size problem?
>>
>> Your board is up against the limit and this causes problems. Please
>> take a look and see how you can add some margin. Takahiro's series
>> does add size and this is unavoidable. See my series of today for some
>> fixes for the SPL size, but for U-Boot proper we have to accept the
>> growth.
>>
>> As it stands here this is just your opinion. Why exactly is this unavoidable?
>>
>> Please keep in mind Simon that we've had zero releases with the
>> DM_SERIAL migration warning being posted, v2022.04 will be the first
>> one.
>>
>> Yes, understood :-) For OF_EMBED though...?
>>
>> No deadline and 50 boards.
>>
>> Er, there has been a build message about that since the beginning, so
>> people ignored it. Do we really need to make the build fail for these
>> sorts of things? Perhaps so, but it is a sad situation.
>>
>> Yes, in hind-sight, "don't do that" wasn't the right path.  It would be
>> a good idea to start a different thread and see what / how the platforms
>> can be migrated away.
>>
>> For tbs2910 this is just a workaround for a strange property of the imx
>> build system. OF_SEPARATE created a broken u-boot.imx when I tried last
>> time.
>>
>> OK, that is worth digging in to.
>>
>> Probably. I'm happy to test whatever someone comes up with.
>>
>>
>> I think there is a use case for it now - e.g. booting Apple M1 which
>> uses a separate bootloader. IMO a .img or .fit file would be better in
>> some cases but people seem to be allergic to implementing U-Boot
>> things in their code bases. We have the same requirement for the EFI
>> app since UEFI does not implement the U-Boot .img file.
>>
>> So if we are going to support this, perhaps we should create a new
>> option for it. But honestly I am just too weary to consider yet
>> another migration. We need to finish some, e.g. Kconfig.
>>
>> It was actually quite hard to add a migration message until we added
>> the CONFIG_SERIAL base thing and that was a pain to do.
>>
>> For those of us who take on larger refactors etc., we end up spending
>> a lot of our time on these few platforms. I'm not picking on tbs2910in
>> in particular.
>>
>> Well, the flip side of the problem here is that there's a number of
>> platforms with real constraints to them and it keeps being "can we drop
>> this yet?" without CC'ing the board maintainer on the series that once
>> again pushes a given platform to the limit.  I would expect no size
>> growth to tbs2910 for the topic of this series since it disables
>> EFI_LOADER entirely, so why is it a problem?
>>
>> The partition changes are going to add some size anyway, I expect. I
>> have not actually analysed it though. Perhaps we can just disable a
>> filesystem?
>>
>> OK, you did not even analyse where the problem comes from. But disabling
>> user visible functionality on my board is the natural solution to that?
>> Strange.
>>
>> As above, please create some space so people can continue to develop.
>> There are refactors and features updates which require more code
>> space. It is somewhat rare, but it happens perhaps every year.
>>
>> It has always been u-boot policy that additional new features should not break existing boards, usually by disabling these new features in defconfig.
>> It is also not new that there are boards with size constraints.
>>
>> If someone causes regressions, then I at least expect that this is thoroughly analysed.
>>
>> I was a bit too absolutist there, sorry.  Yes, a few hundreds of bytes
>> here-and-there is probably a non issue.  But it shouldn't be kilobytes.
>> It really shouldn't push things over the line.
>>
>> And on the tbs2910 side, Soeren, can you look at enabling LTO for this
>> platform?  That would likely buy a good bit of space savings.  That
>> might well be needed to do further DM migrations/etc.
>>
>> I'm not familiar with LTO in U-Boot, but will have a look at the weekend.
>>
>> OK, I suggest getting it several KB under the limit if you can, or
>> perhaps even drop the limit.
>>
>> I already reduced tbs2910 image size several times by substantial amounts. And this is becoming more and more difficult. The size limit is real.
>>
>> Thanks Tom for the LTO suggestion, this will buy us another round. I sent a patch for that.
>>
>> But please, everyone, be careful with additional code size for existing boards. Additional code size is not unavoidable for disabled new features. You just did not try hard enough.
> Please take a look at Tahahiro's series and tell me how we can avoid
> adding a driver for partitions, when the whole point of the series is
> to add a driver for partitions :-)
If this is just a new driver that I don't need (as before), why is it
enabled for my board and causing regressions?

Regards,
Soeren
>
> Regards,
> Simon

Re: [PATCH v3 00/19] efi_loader: more tightly integrate UEFI disks to driver model

[Simon Glass]

Hi Soeren,

On Mon, 14 Mar 2022 at 13:17, Soeren Moch <smoch@web.de> wrote:
>
> Hi Simon,
>
> On 14.03.22 18:08, Simon Glass wrote:
> > Hi Soeren,
> >
> > [I think you sent your email with html or something so it is a big
> > mangled. I'll just add one comment]
> >
> > On Mon, 14 Mar 2022 at 02:27, Soeren Moch <smoch@web.de> wrote:
> >> Hi Simon,
> >>
> >> On 12.03.22 06:02, Simon Glass wrote:
> >>
> >> Hi Soeren,
> >>
> >> On Fri, 11 Mar 2022 at 15:43, Soeren Moch <smoch@web.de> wrote:
> >>
> >>
> >>
> >> On 09.03.22 16:33, Simon Glass wrote:
> >>
> >> Hi Tom,
> >>
> >> On Wed, 9 Mar 2022 at 07:25, Tom Rini <trini@konsulko.com> wrote:
> >>
> >> On Tue, Mar 08, 2022 at 08:10:38PM -0700, Simon Glass wrote:
> >>
> >> Hi Tom,
> >>
> >> On Tue, 8 Mar 2022 at 20:00, Tom Rini <trini@konsulko.com> wrote:
> >>
> >> On Tue, Mar 08, 2022 at 07:32:59PM -0700, Simon Glass wrote:
> >>
> >>    Hi Tom,
> >>
> >> On Tue, 8 Mar 2022 at 17:13, Tom Rini <trini@konsulko.com> wrote:
> >>
> >> On Tue, Mar 08, 2022 at 02:20:15PM -0700, Simon Glass wrote:
> >>
> >> Hi Soeren,
> >>
> >> On Tue, 8 Mar 2022 at 12:15, Soeren Moch <smoch@web.de> wrote:
> >>
> >> On 08.03.22 17:56, Simon Glass wrote:
> >>
> >> Hi,
> >>
> >> On Tue, 8 Mar 2022 at 09:49, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 3/8/22 12:36, AKASHI Takahiro wrote:
> >>
> >> With this patch set[1] applied, UEFI subsystem maintains a list of its
> >> disk objects dynamically at runtime based on block device's probing.
> >> (See "issues" below.)
> >>
> >> [1]https://github.com/t-akashi/u-boot/tree/efi/dm_disk
> >>
> >> This series together with Simon's series breaks multiple boards due to
> >> size constraints:
> >>
> >> https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/11197
> >>
> >> Please, investigate how to work around this issue.
> >>
> >> tbs2910 - perhaps we should just drop this board? It doesn't use
> >> DM_SERIAL and still uses OF_EMBED
> >>
> >> Are we again at the same point? You are breaking working boards with
> >> (for these boards) useless additions, and all you come up with is
> >> "remove this board". Of course without adding the board maintainer.
> >>
> >> I'm just expressing reasonable frustration that this board uses
> >> OF_EMBED and does not use DM_SERIAL, after all of this time. Why
> >> should the rest of the U-Boot developers care more about this board
> >> than the maintainer?
> >>
> >> I cannot see what is is reasonable here.
> >>
> >> I care about this board, what you can simply see by the fact that I even
> >> picked this thread from the mailing list while you "forgot" to cc me.
> >>
> >> This is the patch I sent:
> >>
> >> [PATCH 2/5] tbs2910: Disable ext4 write
> >>
> >> It shows that you are on cc. What are you referring to?
> >>
> >> I'm referring to the exact same email thread that I answered in, which btw. is still this exact same thread for this answer. Why should I refer to the totally different email thread you cited here?
> >>
> >> OF_EMBED and DM_SERIAL are not at all related to EFI or size constraints.
> >>
> >> I'm surprised that you can speak for "the rest of the U-Boot
> >> developers", and that you want to push your frustration onto tbs2910
> >> developers and users. Why is it my fault that other people add code size
> >> without guarding config options? Why is it my fault that nobody informed
> >> me that there is again a size problem?
> >>
> >> Your board is up against the limit and this causes problems. Please
> >> take a look and see how you can add some margin. Takahiro's series
> >> does add size and this is unavoidable. See my series of today for some
> >> fixes for the SPL size, but for U-Boot proper we have to accept the
> >> growth.
> >>
> >> As it stands here this is just your opinion. Why exactly is this unavoidable?
> >>
> >> Please keep in mind Simon that we've had zero releases with the
> >> DM_SERIAL migration warning being posted, v2022.04 will be the first
> >> one.
> >>
> >> Yes, understood :-) For OF_EMBED though...?
> >>
> >> No deadline and 50 boards.
> >>
> >> Er, there has been a build message about that since the beginning, so
> >> people ignored it. Do we really need to make the build fail for these
> >> sorts of things? Perhaps so, but it is a sad situation.
> >>
> >> Yes, in hind-sight, "don't do that" wasn't the right path.  It would be
> >> a good idea to start a different thread and see what / how the platforms
> >> can be migrated away.
> >>
> >> For tbs2910 this is just a workaround for a strange property of the imx
> >> build system. OF_SEPARATE created a broken u-boot.imx when I tried last
> >> time.
> >>
> >> OK, that is worth digging in to.
> >>
> >> Probably. I'm happy to test whatever someone comes up with.
> >>
> >>
> >> I think there is a use case for it now - e.g. booting Apple M1 which
> >> uses a separate bootloader. IMO a .img or .fit file would be better in
> >> some cases but people seem to be allergic to implementing U-Boot
> >> things in their code bases. We have the same requirement for the EFI
> >> app since UEFI does not implement the U-Boot .img file.
> >>
> >> So if we are going to support this, perhaps we should create a new
> >> option for it. But honestly I am just too weary to consider yet
> >> another migration. We need to finish some, e.g. Kconfig.
> >>
> >> It was actually quite hard to add a migration message until we added
> >> the CONFIG_SERIAL base thing and that was a pain to do.
> >>
> >> For those of us who take on larger refactors etc., we end up spending
> >> a lot of our time on these few platforms. I'm not picking on tbs2910in
> >> in particular.
> >>
> >> Well, the flip side of the problem here is that there's a number of
> >> platforms with real constraints to them and it keeps being "can we drop
> >> this yet?" without CC'ing the board maintainer on the series that once
> >> again pushes a given platform to the limit.  I would expect no size
> >> growth to tbs2910 for the topic of this series since it disables
> >> EFI_LOADER entirely, so why is it a problem?
> >>
> >> The partition changes are going to add some size anyway, I expect. I
> >> have not actually analysed it though. Perhaps we can just disable a
> >> filesystem?
> >>
> >> OK, you did not even analyse where the problem comes from. But disabling
> >> user visible functionality on my board is the natural solution to that?
> >> Strange.
> >>
> >> As above, please create some space so people can continue to develop.
> >> There are refactors and features updates which require more code
> >> space. It is somewhat rare, but it happens perhaps every year.
> >>
> >> It has always been u-boot policy that additional new features should not break existing boards, usually by disabling these new features in defconfig.
> >> It is also not new that there are boards with size constraints.
> >>
> >> If someone causes regressions, then I at least expect that this is thoroughly analysed.
> >>
> >> I was a bit too absolutist there, sorry.  Yes, a few hundreds of bytes
> >> here-and-there is probably a non issue.  But it shouldn't be kilobytes.
> >> It really shouldn't push things over the line.
> >>
> >> And on the tbs2910 side, Soeren, can you look at enabling LTO for this
> >> platform?  That would likely buy a good bit of space savings.  That
> >> might well be needed to do further DM migrations/etc.
> >>
> >> I'm not familiar with LTO in U-Boot, but will have a look at the weekend.
> >>
> >> OK, I suggest getting it several KB under the limit if you can, or
> >> perhaps even drop the limit.
> >>
> >> I already reduced tbs2910 image size several times by substantial amounts. And this is becoming more and more difficult. The size limit is real.
> >>
> >> Thanks Tom for the LTO suggestion, this will buy us another round. I sent a patch for that.
> >>
> >> But please, everyone, be careful with additional code size for existing boards. Additional code size is not unavoidable for disabled new features. You just did not try hard enough.
> > Please take a look at Tahahiro's series and tell me how we can avoid
> > adding a driver for partitions, when the whole point of the series is
> > to add a driver for partitions :-)
> If this is just a new driver that I don't need (as before), why is it
> enabled for my board and causing regressions?

If you disable partition support, you don't need it. Please just take
a look at the series as I think it will make sense.

Regards,
SImon

Re: [PATCH v3 14/19] efi_loader: disk: a helper function to create efi_disk objects from udevice

[Heinrich Schuchardt]

On 3/8/22 12:36, AKASHI Takahiro wrote:
> Add efi_disk_probe() function.
> This function creates an efi_disk object for a raw disk device (UCLASS_BLK)
> and additional objects for related partitions (UCLASS_PARTITION).
>
> So this function is expected to be called through driver model's "probe"
> interface every time one raw disk device is detected and activated.
> We assume that partition devices (UCLASS_PARTITION) have been created
> when this function is invoked.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

Merging all patches up to this one leads to:

/lib/efi_driver/efi_block_device.c:124: undefined reference to
`efi_disk_create_partitions'

As we want to be able to bisect code, please, ensure that after each
patch the code builds.

Best regards

Heinrich

> ---
>   include/efi_loader.h       |   4 +-
>   lib/efi_loader/Kconfig     |   3 +
>   lib/efi_loader/efi_disk.c  | 201 +++++++++++++++++++++++--------------
>   lib/efi_loader/efi_setup.c |   4 +-
>   4 files changed, 133 insertions(+), 79 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index a9075c992687..24ec692c4d09 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -523,8 +523,8 @@ void efi_carve_out_dt_rsv(void *fdt);
>   void efi_try_purge_kaslr_seed(void *fdt);
>   /* Called by bootefi to make console interface available */
>   efi_status_t efi_console_register(void);
> -/* Called by bootefi to make all disk storage accessible as EFI objects */
> -efi_status_t efi_disk_register(void);
> +/* Called by efi_init_obj_list() to initialize efi_disks */
> +efi_status_t efi_disk_init(void);
>   /* Called by efi_init_obj_list() to install EFI_RNG_PROTOCOL */
>   efi_status_t efi_rng_register(void);
>   /* Called by efi_init_obj_list() to install EFI_TCG2_PROTOCOL */
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index e5e35fe51f65..7ffd59759359 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -14,6 +14,8 @@ config EFI_LOADER
>   	depends on DM_ETH || !NET
>   	depends on !EFI_APP
>   	default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8
> +	select DM_EVENT
> +	select EVENT_DYNAMIC
>   	select LIB_UUID
>   	select PARTITION_UUIDS
>   	select HAVE_BLOCK_DEVICE
> @@ -41,6 +43,7 @@ config CMD_BOOTEFI_BOOTMGR
>
>   config EFI_SETUP_EARLY
>   	bool
> +	default y
>
>   choice
>   	prompt "Store for non-volatile UEFI variables"
> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> index 45127d176869..bf9824b9b1a4 100644
> --- a/lib/efi_loader/efi_disk.c
> +++ b/lib/efi_loader/efi_disk.c
> @@ -10,6 +10,9 @@
>   #include <common.h>
>   #include <blk.h>
>   #include <dm.h>
> +#include <dm/device-internal.h>
> +#include <dm/tag.h>
> +#include <event.h>
>   #include <efi_loader.h>
>   #include <fs.h>
>   #include <log.h>
> @@ -487,103 +490,153 @@ error:
>   	return ret;
>   }
>
> -/**
> - * efi_disk_create_partitions() - create handles and protocols for partitions
> +/*
> + * Create a handle for a whole raw disk
> + *
> + * @dev		uclass device (UCLASS_BLK)
>    *
> - * Create handles and protocols for the partitions of a block device.
> + * Create an efi_disk object which is associated with @dev.
> + * The type of @dev must be UCLASS_BLK.
>    *
> - * @parent:		handle of the parent disk
> - * @desc:		block device
> - * @if_typename:	interface type
> - * @diskid:		device number
> - * @pdevname:		device name
> - * Return:		number of partitions created
> + * @return	0 on success, -1 otherwise
>    */
> -int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
> -			       const char *if_typename, int diskid,
> -			       const char *pdevname)
> +static int efi_disk_create_raw(struct udevice *dev)
>   {
> -	int disks = 0;
> -	char devname[32] = { 0 }; /* dp->str is u16[32] long */
> -	int part;
> -	struct efi_device_path *dp = NULL;
> +	struct efi_disk_obj *disk;
> +	struct blk_desc *desc;
> +	const char *if_typename;
> +	int diskid;
>   	efi_status_t ret;
> -	struct efi_handler *handler;
>
> -	/* Get the device path of the parent */
> -	ret = efi_search_protocol(parent, &efi_guid_device_path, &handler);
> -	if (ret == EFI_SUCCESS)
> -		dp = handler->protocol_interface;
> -
> -	/* Add devices for each partition */
> -	for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
> -		struct disk_partition info;
> -
> -		if (part_get_info(desc, part, &info))
> -			continue;
> -		snprintf(devname, sizeof(devname), "%s:%x", pdevname,
> -			 part);
> -		ret = efi_disk_add_dev(parent, dp, if_typename, desc, diskid,
> -				       &info, part, NULL);
> -		if (ret != EFI_SUCCESS) {
> -			log_err("Adding partition %s failed\n", pdevname);
> -			continue;
> -		}
> -		disks++;
> +	desc = dev_get_uclass_plat(dev);
> +	if_typename = blk_get_if_type_name(desc->if_type);
> +	diskid = desc->devnum;
> +
> +	ret = efi_disk_add_dev(NULL, NULL, if_typename, desc,
> +			       diskid, NULL, 0, &disk);
> +	if (ret != EFI_SUCCESS) {
> +		if (ret == EFI_NOT_READY)
> +			log_notice("Disk %s not ready\n", dev->name);
> +		else
> +			log_err("Adding disk for %s failed\n", dev->name);
> +
> +		return -1;
> +	}
> +	if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) {
> +		efi_free_pool(disk->dp);
> +		efi_delete_handle(&disk->header);
> +
> +		return -1;
>   	}
>
> -	return disks;
> +	return 0;
>   }
>
> -/**
> - * efi_disk_register() - register block devices
> - *
> - * U-Boot doesn't have a list of all online disk devices. So when running our
> - * EFI payload, we scan through all of the potentially available ones and
> - * store them in our object pool.
> +/*
> + * Create a handle for a disk partition
>    *
> - * This function is called in efi_init_obj_list().
> + * @dev		uclass device (UCLASS_PARTITION)
>    *
> - * TODO(sjg@chromium.org): Actually with CONFIG_BLK, U-Boot does have this.
> - * Consider converting the code to look up devices as needed. The EFI device
> - * could be a child of the UCLASS_BLK block device, perhaps.
> + * Create an efi_disk object which is associated with @dev.
> + * The type of @dev must be UCLASS_PARTITION.
>    *
> - * Return:	status code
> + * @return	0 on success, -1 otherwise
>    */
> -efi_status_t efi_disk_register(void)
> +static int efi_disk_create_part(struct udevice *dev)
>   {
> +	efi_handle_t parent;
> +	struct blk_desc *desc;
> +	const char *if_typename;
> +	struct disk_part *part_data;
> +	struct disk_partition *info;
> +	unsigned int part;
> +	int diskid;
> +	struct efi_handler *handler;
> +	struct efi_device_path *dp_parent;
>   	struct efi_disk_obj *disk;
> -	int disks = 0;
>   	efi_status_t ret;
> +
> +	if (dev_tag_get_ptr(dev_get_parent(dev), DM_TAG_EFI, (void **)&parent))
> +		return -1;
> +
> +	desc = dev_get_uclass_plat(dev_get_parent(dev));
> +	if_typename = blk_get_if_type_name(desc->if_type);
> +	diskid = desc->devnum;
> +
> +	part_data = dev_get_uclass_plat(dev);
> +	part = part_data->partnum;
> +	info = &part_data->gpt_part_info;
> +
> +	ret = efi_search_protocol(parent, &efi_guid_device_path, &handler);
> +	if (ret != EFI_SUCCESS)
> +		return -1;
> +	dp_parent = (struct efi_device_path *)handler->protocol_interface;
> +
> +	ret = efi_disk_add_dev(parent, dp_parent, if_typename, desc, diskid,
> +			       info, part, &disk);
> +	if (ret != EFI_SUCCESS) {
> +		log_err("Adding partition for %s failed\n", dev->name);
> +		return -1;
> +	}
> +	if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) {
> +		efi_free_pool(disk->dp);
> +		efi_delete_handle(&disk->header);
> +
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * Create efi_disk objects for a block device
> + *
> + * @dev		uclass device (UCLASS_BLK)
> + *
> + * Create efi_disk objects for partitions as well as a raw disk
> + * which is associated with @dev.
> + * The type of @dev must be UCLASS_BLK.
> + * This function is expected to be called at EV_PM_POST_PROBE.
> + *
> + * @return	0 on success, -1 otherwise
> + */
> +static int efi_disk_probe(void *ctx, struct event *event)
> +{
>   	struct udevice *dev;
> +	enum uclass_id id;
> +	struct udevice *child;
> +	int ret;
>
> -	for (uclass_first_device_check(UCLASS_BLK, &dev); dev;
> -	     uclass_next_device_check(&dev)) {
> -		struct blk_desc *desc = dev_get_uclass_plat(dev);
> -		const char *if_typename = blk_get_if_type_name(desc->if_type);
> +	dev = event->data.dm.dev;
> +	id = device_get_uclass_id(dev);
>
> -		/* Add block device for the full device */
> -		log_info("Scanning disk %s...\n", dev->name);
> -		ret = efi_disk_add_dev(NULL, NULL, if_typename,
> -					desc, desc->devnum, NULL, 0, &disk);
> -		if (ret == EFI_NOT_READY) {
> -			log_notice("Disk %s not ready\n", dev->name);
> -			continue;
> -		}
> -		if (ret) {
> -			log_err("ERROR: failure to add disk device %s, r = %lu\n",
> -				dev->name, ret & ~EFI_ERROR_MASK);
> -			continue;
> -		}
> -		disks++;
> +	/* TODO: We won't support partitions in a partition */
> +	if (id != UCLASS_BLK)
> +		return 0;
> +
> +	ret = efi_disk_create_raw(dev);
> +	if (ret)
> +		return -1;
>
> -		/* Partitions show up as block devices in EFI */
> -		disks += efi_disk_create_partitions(
> -					&disk->header, desc, if_typename,
> -					desc->devnum, dev->name);
> +	device_foreach_child(child, dev) {
> +		ret = efi_disk_create_part(child);
> +		if (ret)
> +			return -1;
>   	}
>
> -	log_info("Found %d disks\n", disks);
> +	return 0;
> +}
> +
> +efi_status_t efi_disk_init(void)
> +{
> +	int ret;
> +
> +	ret = event_register("efi_disk add", EVT_DM_POST_PROBE,
> +			     efi_disk_probe, NULL);
> +	if (ret) {
> +		log_err("Event registration for efi_disk add failed\n");
> +		return EFI_OUT_OF_RESOURCES;
> +	}
>
>   	return EFI_SUCCESS;
>   }
> diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> index de2f34bab537..250eeb2fcd6b 100644
> --- a/lib/efi_loader/efi_setup.c
> +++ b/lib/efi_loader/efi_setup.c
> @@ -198,9 +198,7 @@ static efi_status_t __efi_init_early(void)
>   	if (ret != EFI_SUCCESS)
>   		goto out;
>
> -#ifdef CONFIG_PARTITIONS
> -	ret = efi_disk_register();
> -#endif
> +	ret = efi_disk_init();
>   out:
>   	return ret;
>   }

Re: [PATCH v3 11/19] dm: disk: add UCLASS_PARTITION

[Heinrich Schuchardt]

On 3/8/22 12:36, AKASHI Takahiro wrote:
> With this new function, UCLASS_PARTITION devices will be created as
> child nodes of UCLASS_BLK device.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   disk/Makefile          |   3 +
>   disk/disk-uclass.c     | 153 +++++++++++++++++++++++++++++++++++++++++
>   include/dm/uclass-id.h |   1 +
>   include/part.h         |  11 +++
>   4 files changed, 168 insertions(+)
>   create mode 100644 disk/disk-uclass.c
>
> diff --git a/disk/Makefile b/disk/Makefile
> index 6ce5a687b36c..ec37b74f5f40 100644
> --- a/disk/Makefile
> +++ b/disk/Makefile
> @@ -6,6 +6,9 @@
>   #ccflags-y += -DET_DEBUG -DDEBUG
>
>   obj-$(CONFIG_PARTITIONS)	+= part.o
> +ifdef CONFIG_$(SPL_)BLK
> +obj-$(CONFIG_PARTITIONS) 	+= disk-uclass.o

phycore-rk3288_defconfig has CONFIG_SPL_LIBDISK_SUPPORT=n ("Support disk
partitions").
But spl/disk/disk-uclass is built.
This leads to an unnecessary size increase of the SPL image.
With this size increase building fails.

Best regards

Heinrich

> +endif
>   obj-$(CONFIG_$(SPL_)MAC_PARTITION)   += part_mac.o
>   obj-$(CONFIG_$(SPL_)DOS_PARTITION)   += part_dos.o
>   obj-$(CONFIG_$(SPL_)ISO_PARTITION)   += part_iso.o
> diff --git a/disk/disk-uclass.c b/disk/disk-uclass.c
> new file mode 100644
> index 000000000000..4918a2f72d1e
> --- /dev/null
> +++ b/disk/disk-uclass.c
> @@ -0,0 +1,153 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + *  Software partition device (UCLASS_PARTITION)
> + *
> + *  Copyright (c) 2021 Linaro Limited
> + *			Author: AKASHI Takahiro
> + */
> +
> +#define LOG_CATEGORY UCLASS_PARTITION
> +
> +#include <blk.h>
> +#include <dm.h>
> +#include <log.h>
> +#include <part.h>
> +#include <vsprintf.h>
> +#include <dm/device-internal.h>
> +#include <dm/lists.h>
> +
> +int part_create_block_devices(struct udevice *blk_dev)
> +{
> +	int part, count;
> +	struct blk_desc *desc = dev_get_uclass_plat(blk_dev);
> +	struct disk_partition info;
> +	struct disk_part *part_data;
> +	char devname[32];
> +	struct udevice *dev;
> +	int ret;
> +
> +	if (!CONFIG_IS_ENABLED(PARTITIONS) ||
> +	    !CONFIG_IS_ENABLED(HAVE_BLOCK_DEVICE))
> +		return 0;
> +
> +	if (device_get_uclass_id(blk_dev) != UCLASS_BLK)
> +		return 0;
> +
> +	/* Add devices for each partition */
> +	for (count = 0, part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
> +		if (part_get_info(desc, part, &info))
> +			continue;
> +		snprintf(devname, sizeof(devname), "%s:%d", blk_dev->name,
> +			 part);
> +
> +		ret = device_bind_driver(blk_dev, "blk_partition",
> +					 strdup(devname), &dev);
> +		if (ret)
> +			return ret;
> +
> +		part_data = dev_get_uclass_plat(dev);
> +		part_data->partnum = part;
> +		part_data->gpt_part_info = info;
> +		count++;
> +
> +		ret = device_probe(dev);
> +		if (ret) {
> +			debug("Can't probe\n");
> +			count--;
> +			device_unbind(dev);
> +
> +			continue;
> +		}
> +	}
> +	debug("%s: %d partitions found in %s\n", __func__, count,
> +	      blk_dev->name);
> +
> +	return 0;
> +}
> +
> +static ulong blk_part_read(struct udevice *dev, lbaint_t start,
> +			   lbaint_t blkcnt, void *buffer)
> +{
> +	struct udevice *parent;
> +	struct disk_part *part;
> +	const struct blk_ops *ops;
> +
> +	parent = dev_get_parent(dev);
> +	ops = blk_get_ops(parent);
> +	if (!ops->read)
> +		return -ENOSYS;
> +
> +	part = dev_get_uclass_plat(dev);
> +	if (start >= part->gpt_part_info.size)
> +		return 0;
> +
> +	if ((start + blkcnt) > part->gpt_part_info.size)
> +		blkcnt = part->gpt_part_info.size - start;
> +	start += part->gpt_part_info.start;
> +
> +	return ops->read(parent, start, blkcnt, buffer);
> +}
> +
> +static ulong blk_part_write(struct udevice *dev, lbaint_t start,
> +			    lbaint_t blkcnt, const void *buffer)
> +{
> +	struct udevice *parent;
> +	struct disk_part *part;
> +	const struct blk_ops *ops;
> +
> +	parent = dev_get_parent(dev);
> +	ops = blk_get_ops(parent);
> +	if (!ops->write)
> +		return -ENOSYS;
> +
> +	part = dev_get_uclass_plat(dev);
> +	if (start >= part->gpt_part_info.size)
> +		return 0;
> +
> +	if ((start + blkcnt) > part->gpt_part_info.size)
> +		blkcnt = part->gpt_part_info.size - start;
> +	start += part->gpt_part_info.start;
> +
> +	return ops->write(parent, start, blkcnt, buffer);
> +}
> +
> +static ulong blk_part_erase(struct udevice *dev, lbaint_t start,
> +			    lbaint_t blkcnt)
> +{
> +	struct udevice *parent;
> +	struct disk_part *part;
> +	const struct blk_ops *ops;
> +
> +	parent = dev_get_parent(dev);
> +	ops = blk_get_ops(parent);
> +	if (!ops->erase)
> +		return -ENOSYS;
> +
> +	part = dev_get_uclass_plat(dev);
> +	if (start >= part->gpt_part_info.size)
> +		return 0;
> +
> +	if ((start + blkcnt) > part->gpt_part_info.size)
> +		blkcnt = part->gpt_part_info.size - start;
> +	start += part->gpt_part_info.start;
> +
> +	return ops->erase(parent, start, blkcnt);
> +}
> +
> +static const struct blk_ops blk_part_ops = {
> +	.read	= blk_part_read,
> +	.write	= blk_part_write,
> +	.erase	= blk_part_erase,
> +};
> +
> +U_BOOT_DRIVER(blk_partition) = {
> +	.name		= "blk_partition",
> +	.id		= UCLASS_PARTITION,
> +	.ops		= &blk_part_ops,
> +};
> +
> +UCLASS_DRIVER(partition) = {
> +	.id		= UCLASS_PARTITION,
> +	.per_device_plat_auto	= sizeof(struct disk_part),
> +	.name		= "partition",
> +};
> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index 0e26e1d13824..230b1ea528cf 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -83,6 +83,7 @@ enum uclass_id {
>   	UCLASS_P2SB,		/* (x86) Primary-to-Sideband Bus */
>   	UCLASS_PANEL,		/* Display panel, such as an LCD */
>   	UCLASS_PANEL_BACKLIGHT,	/* Backlight controller for panel */
> +	UCLASS_PARTITION,	/* Logical disk partition device */
>   	UCLASS_PCH,		/* x86 platform controller hub */
>   	UCLASS_PCI,		/* PCI bus */
>   	UCLASS_PCI_EP,		/* PCI endpoint device */
> diff --git a/include/part.h b/include/part.h
> index 53cfbdd87671..95e30e60af10 100644
> --- a/include/part.h
> +++ b/include/part.h
> @@ -253,6 +253,17 @@ void part_set_generic_name(const struct blk_desc *dev_desc,
>   	int part_num, char *name);
>
>   extern const struct block_drvr block_drvr[];
> +
> +struct udevice;
> +/**
> + * part_create_block_devices - Create block devices for disk partitions
> + *
> + * Create UCLASS_PARTITION udevices for each of disk partitions in @parent
> + *
> + * @blk_dev:	Whole disk device
> + */
> +int part_create_block_devices(struct udevice *blk_dev);
> +
>   #else
>   static inline struct blk_desc *blk_get_dev(const char *ifname, int dev)
>   { return NULL; }

Re: [PATCH v3 14/19] efi_loader: disk: a helper function to create efi_disk objects from udevice

[AKASHI Takahiro]

Heinrich,

On Sat, Apr 09, 2022 at 01:16:01PM +0200, Heinrich Schuchardt wrote:
> On 3/8/22 12:36, AKASHI Takahiro wrote:
> > Add efi_disk_probe() function.
> > This function creates an efi_disk object for a raw disk device (UCLASS_BLK)
> > and additional objects for related partitions (UCLASS_PARTITION).
> > 
> > So this function is expected to be called through driver model's "probe"
> > interface every time one raw disk device is detected and activated.
> > We assume that partition devices (UCLASS_PARTITION) have been created
> > when this function is invoked.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> 
> Merging all patches up to this one leads to:
> 
> /lib/efi_driver/efi_block_device.c:124: undefined reference to
> `efi_disk_create_partitions'

I wanted any changes related to lib/efi_driver to be separated from the rest of
the patch series, but


> As we want to be able to bisect code, please, ensure that after each
> patch the code builds.

If you want, we can fix a bisect issue either by
a) "squash" patch#19 ("efi_driver: align with efi_disk-dm integration") into
   patch#14 ("efi_loader: disk: a helper function to create"), or
b) squash patch#18 ("efi_loader: disk: not create BLK device for BLK
   (IF_TYPE_EFI_LOADER) devices"), in addition to patch#19, into patch#14

You can do (a) by yourself without any problem as patch#19 is the only change
against lib/efi_driver/efi_block_device.c in this series.
If you like (b), I will post a new version as changing the order of patch#18
will create some conflict.

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> > ---
> >   include/efi_loader.h       |   4 +-
> >   lib/efi_loader/Kconfig     |   3 +
> >   lib/efi_loader/efi_disk.c  | 201 +++++++++++++++++++++++--------------
> >   lib/efi_loader/efi_setup.c |   4 +-
> >   4 files changed, 133 insertions(+), 79 deletions(-)
> > 
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index a9075c992687..24ec692c4d09 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -523,8 +523,8 @@ void efi_carve_out_dt_rsv(void *fdt);
> >   void efi_try_purge_kaslr_seed(void *fdt);
> >   /* Called by bootefi to make console interface available */
> >   efi_status_t efi_console_register(void);
> > -/* Called by bootefi to make all disk storage accessible as EFI objects */
> > -efi_status_t efi_disk_register(void);
> > +/* Called by efi_init_obj_list() to initialize efi_disks */
> > +efi_status_t efi_disk_init(void);
> >   /* Called by efi_init_obj_list() to install EFI_RNG_PROTOCOL */
> >   efi_status_t efi_rng_register(void);
> >   /* Called by efi_init_obj_list() to install EFI_TCG2_PROTOCOL */
> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > index e5e35fe51f65..7ffd59759359 100644
> > --- a/lib/efi_loader/Kconfig
> > +++ b/lib/efi_loader/Kconfig
> > @@ -14,6 +14,8 @@ config EFI_LOADER
> >   	depends on DM_ETH || !NET
> >   	depends on !EFI_APP
> >   	default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8
> > +	select DM_EVENT
> > +	select EVENT_DYNAMIC
> >   	select LIB_UUID
> >   	select PARTITION_UUIDS
> >   	select HAVE_BLOCK_DEVICE
> > @@ -41,6 +43,7 @@ config CMD_BOOTEFI_BOOTMGR
> > 
> >   config EFI_SETUP_EARLY
> >   	bool
> > +	default y
> > 
> >   choice
> >   	prompt "Store for non-volatile UEFI variables"
> > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> > index 45127d176869..bf9824b9b1a4 100644
> > --- a/lib/efi_loader/efi_disk.c
> > +++ b/lib/efi_loader/efi_disk.c
> > @@ -10,6 +10,9 @@
> >   #include <common.h>
> >   #include <blk.h>
> >   #include <dm.h>
> > +#include <dm/device-internal.h>
> > +#include <dm/tag.h>
> > +#include <event.h>
> >   #include <efi_loader.h>
> >   #include <fs.h>
> >   #include <log.h>
> > @@ -487,103 +490,153 @@ error:
> >   	return ret;
> >   }
> > 
> > -/**
> > - * efi_disk_create_partitions() - create handles and protocols for partitions
> > +/*
> > + * Create a handle for a whole raw disk
> > + *
> > + * @dev		uclass device (UCLASS_BLK)
> >    *
> > - * Create handles and protocols for the partitions of a block device.
> > + * Create an efi_disk object which is associated with @dev.
> > + * The type of @dev must be UCLASS_BLK.
> >    *
> > - * @parent:		handle of the parent disk
> > - * @desc:		block device
> > - * @if_typename:	interface type
> > - * @diskid:		device number
> > - * @pdevname:		device name
> > - * Return:		number of partitions created
> > + * @return	0 on success, -1 otherwise
> >    */
> > -int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
> > -			       const char *if_typename, int diskid,
> > -			       const char *pdevname)
> > +static int efi_disk_create_raw(struct udevice *dev)
> >   {
> > -	int disks = 0;
> > -	char devname[32] = { 0 }; /* dp->str is u16[32] long */
> > -	int part;
> > -	struct efi_device_path *dp = NULL;
> > +	struct efi_disk_obj *disk;
> > +	struct blk_desc *desc;
> > +	const char *if_typename;
> > +	int diskid;
> >   	efi_status_t ret;
> > -	struct efi_handler *handler;
> > 
> > -	/* Get the device path of the parent */
> > -	ret = efi_search_protocol(parent, &efi_guid_device_path, &handler);
> > -	if (ret == EFI_SUCCESS)
> > -		dp = handler->protocol_interface;
> > -
> > -	/* Add devices for each partition */
> > -	for (part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
> > -		struct disk_partition info;
> > -
> > -		if (part_get_info(desc, part, &info))
> > -			continue;
> > -		snprintf(devname, sizeof(devname), "%s:%x", pdevname,
> > -			 part);
> > -		ret = efi_disk_add_dev(parent, dp, if_typename, desc, diskid,
> > -				       &info, part, NULL);
> > -		if (ret != EFI_SUCCESS) {
> > -			log_err("Adding partition %s failed\n", pdevname);
> > -			continue;
> > -		}
> > -		disks++;
> > +	desc = dev_get_uclass_plat(dev);
> > +	if_typename = blk_get_if_type_name(desc->if_type);
> > +	diskid = desc->devnum;
> > +
> > +	ret = efi_disk_add_dev(NULL, NULL, if_typename, desc,
> > +			       diskid, NULL, 0, &disk);
> > +	if (ret != EFI_SUCCESS) {
> > +		if (ret == EFI_NOT_READY)
> > +			log_notice("Disk %s not ready\n", dev->name);
> > +		else
> > +			log_err("Adding disk for %s failed\n", dev->name);
> > +
> > +		return -1;
> > +	}
> > +	if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) {
> > +		efi_free_pool(disk->dp);
> > +		efi_delete_handle(&disk->header);
> > +
> > +		return -1;
> >   	}
> > 
> > -	return disks;
> > +	return 0;
> >   }
> > 
> > -/**
> > - * efi_disk_register() - register block devices
> > - *
> > - * U-Boot doesn't have a list of all online disk devices. So when running our
> > - * EFI payload, we scan through all of the potentially available ones and
> > - * store them in our object pool.
> > +/*
> > + * Create a handle for a disk partition
> >    *
> > - * This function is called in efi_init_obj_list().
> > + * @dev		uclass device (UCLASS_PARTITION)
> >    *
> > - * TODO(sjg@chromium.org): Actually with CONFIG_BLK, U-Boot does have this.
> > - * Consider converting the code to look up devices as needed. The EFI device
> > - * could be a child of the UCLASS_BLK block device, perhaps.
> > + * Create an efi_disk object which is associated with @dev.
> > + * The type of @dev must be UCLASS_PARTITION.
> >    *
> > - * Return:	status code
> > + * @return	0 on success, -1 otherwise
> >    */
> > -efi_status_t efi_disk_register(void)
> > +static int efi_disk_create_part(struct udevice *dev)
> >   {
> > +	efi_handle_t parent;
> > +	struct blk_desc *desc;
> > +	const char *if_typename;
> > +	struct disk_part *part_data;
> > +	struct disk_partition *info;
> > +	unsigned int part;
> > +	int diskid;
> > +	struct efi_handler *handler;
> > +	struct efi_device_path *dp_parent;
> >   	struct efi_disk_obj *disk;
> > -	int disks = 0;
> >   	efi_status_t ret;
> > +
> > +	if (dev_tag_get_ptr(dev_get_parent(dev), DM_TAG_EFI, (void **)&parent))
> > +		return -1;
> > +
> > +	desc = dev_get_uclass_plat(dev_get_parent(dev));
> > +	if_typename = blk_get_if_type_name(desc->if_type);
> > +	diskid = desc->devnum;
> > +
> > +	part_data = dev_get_uclass_plat(dev);
> > +	part = part_data->partnum;
> > +	info = &part_data->gpt_part_info;
> > +
> > +	ret = efi_search_protocol(parent, &efi_guid_device_path, &handler);
> > +	if (ret != EFI_SUCCESS)
> > +		return -1;
> > +	dp_parent = (struct efi_device_path *)handler->protocol_interface;
> > +
> > +	ret = efi_disk_add_dev(parent, dp_parent, if_typename, desc, diskid,
> > +			       info, part, &disk);
> > +	if (ret != EFI_SUCCESS) {
> > +		log_err("Adding partition for %s failed\n", dev->name);
> > +		return -1;
> > +	}
> > +	if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) {
> > +		efi_free_pool(disk->dp);
> > +		efi_delete_handle(&disk->header);
> > +
> > +		return -1;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Create efi_disk objects for a block device
> > + *
> > + * @dev		uclass device (UCLASS_BLK)
> > + *
> > + * Create efi_disk objects for partitions as well as a raw disk
> > + * which is associated with @dev.
> > + * The type of @dev must be UCLASS_BLK.
> > + * This function is expected to be called at EV_PM_POST_PROBE.
> > + *
> > + * @return	0 on success, -1 otherwise
> > + */
> > +static int efi_disk_probe(void *ctx, struct event *event)
> > +{
> >   	struct udevice *dev;
> > +	enum uclass_id id;
> > +	struct udevice *child;
> > +	int ret;
> > 
> > -	for (uclass_first_device_check(UCLASS_BLK, &dev); dev;
> > -	     uclass_next_device_check(&dev)) {
> > -		struct blk_desc *desc = dev_get_uclass_plat(dev);
> > -		const char *if_typename = blk_get_if_type_name(desc->if_type);
> > +	dev = event->data.dm.dev;
> > +	id = device_get_uclass_id(dev);
> > 
> > -		/* Add block device for the full device */
> > -		log_info("Scanning disk %s...\n", dev->name);
> > -		ret = efi_disk_add_dev(NULL, NULL, if_typename,
> > -					desc, desc->devnum, NULL, 0, &disk);
> > -		if (ret == EFI_NOT_READY) {
> > -			log_notice("Disk %s not ready\n", dev->name);
> > -			continue;
> > -		}
> > -		if (ret) {
> > -			log_err("ERROR: failure to add disk device %s, r = %lu\n",
> > -				dev->name, ret & ~EFI_ERROR_MASK);
> > -			continue;
> > -		}
> > -		disks++;
> > +	/* TODO: We won't support partitions in a partition */
> > +	if (id != UCLASS_BLK)
> > +		return 0;
> > +
> > +	ret = efi_disk_create_raw(dev);
> > +	if (ret)
> > +		return -1;
> > 
> > -		/* Partitions show up as block devices in EFI */
> > -		disks += efi_disk_create_partitions(
> > -					&disk->header, desc, if_typename,
> > -					desc->devnum, dev->name);
> > +	device_foreach_child(child, dev) {
> > +		ret = efi_disk_create_part(child);
> > +		if (ret)
> > +			return -1;
> >   	}
> > 
> > -	log_info("Found %d disks\n", disks);
> > +	return 0;
> > +}
> > +
> > +efi_status_t efi_disk_init(void)
> > +{
> > +	int ret;
> > +
> > +	ret = event_register("efi_disk add", EVT_DM_POST_PROBE,
> > +			     efi_disk_probe, NULL);
> > +	if (ret) {
> > +		log_err("Event registration for efi_disk add failed\n");
> > +		return EFI_OUT_OF_RESOURCES;
> > +	}
> > 
> >   	return EFI_SUCCESS;
> >   }
> > diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> > index de2f34bab537..250eeb2fcd6b 100644
> > --- a/lib/efi_loader/efi_setup.c
> > +++ b/lib/efi_loader/efi_setup.c
> > @@ -198,9 +198,7 @@ static efi_status_t __efi_init_early(void)
> >   	if (ret != EFI_SUCCESS)
> >   		goto out;
> > 
> > -#ifdef CONFIG_PARTITIONS
> > -	ret = efi_disk_register();
> > -#endif
> > +	ret = efi_disk_init();
> >   out:
> >   	return ret;
> >   }
> 

Re: [PATCH v3 11/19] dm: disk: add UCLASS_PARTITION

[AKASHI Takahiro]

Heinrich,
(and Simon)

On Sat, Apr 09, 2022 at 09:05:07PM +0200, Heinrich Schuchardt wrote:
> On 3/8/22 12:36, AKASHI Takahiro wrote:
> > With this new function, UCLASS_PARTITION devices will be created as
> > child nodes of UCLASS_BLK device.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >   disk/Makefile          |   3 +
> >   disk/disk-uclass.c     | 153 +++++++++++++++++++++++++++++++++++++++++
> >   include/dm/uclass-id.h |   1 +
> >   include/part.h         |  11 +++
> >   4 files changed, 168 insertions(+)
> >   create mode 100644 disk/disk-uclass.c
> > 
> > diff --git a/disk/Makefile b/disk/Makefile
> > index 6ce5a687b36c..ec37b74f5f40 100644
> > --- a/disk/Makefile
> > +++ b/disk/Makefile
> > @@ -6,6 +6,9 @@
> >   #ccflags-y += -DET_DEBUG -DDEBUG
> > 
> >   obj-$(CONFIG_PARTITIONS)	+= part.o
> > +ifdef CONFIG_$(SPL_)BLK
> > +obj-$(CONFIG_PARTITIONS) 	+= disk-uclass.o
> 
> phycore-rk3288_defconfig has CONFIG_SPL_LIBDISK_SUPPORT=n ("Support disk
> partitions").

I think that SPL_LIBDISK_SUPPORT no longer make any sense and is obsolete.

> But spl/disk/disk-uclass is built.

Right, but what is strange is:
- SPL_PARTITION is enabled by "default y if PARTITONS", while
- none of any particular partition types is enabled for spl
  due to *explicit* disablement of
     # CONFIG_SPL_DOS_PARTITION is not set
     # CONFIG_SPL_EFI_PARTITION is not set
  in phycore-rk3288_defconfig.

I think that disk/Kconfig should be revised.

-Takahiro Akashi

> This leads to an unnecessary size increase of the SPL image.
> With this size increase building fails.
> 
> Best regards
> 
> Heinrich
> 
> > +endif
> >   obj-$(CONFIG_$(SPL_)MAC_PARTITION)   += part_mac.o
> >   obj-$(CONFIG_$(SPL_)DOS_PARTITION)   += part_dos.o
> >   obj-$(CONFIG_$(SPL_)ISO_PARTITION)   += part_iso.o
> > diff --git a/disk/disk-uclass.c b/disk/disk-uclass.c
> > new file mode 100644
> > index 000000000000..4918a2f72d1e
> > --- /dev/null
> > +++ b/disk/disk-uclass.c
> > @@ -0,0 +1,153 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + *  Software partition device (UCLASS_PARTITION)
> > + *
> > + *  Copyright (c) 2021 Linaro Limited
> > + *			Author: AKASHI Takahiro
> > + */
> > +
> > +#define LOG_CATEGORY UCLASS_PARTITION
> > +
> > +#include <blk.h>
> > +#include <dm.h>
> > +#include <log.h>
> > +#include <part.h>
> > +#include <vsprintf.h>
> > +#include <dm/device-internal.h>
> > +#include <dm/lists.h>
> > +
> > +int part_create_block_devices(struct udevice *blk_dev)
> > +{
> > +	int part, count;
> > +	struct blk_desc *desc = dev_get_uclass_plat(blk_dev);
> > +	struct disk_partition info;
> > +	struct disk_part *part_data;
> > +	char devname[32];
> > +	struct udevice *dev;
> > +	int ret;
> > +
> > +	if (!CONFIG_IS_ENABLED(PARTITIONS) ||
> > +	    !CONFIG_IS_ENABLED(HAVE_BLOCK_DEVICE))
> > +		return 0;
> > +
> > +	if (device_get_uclass_id(blk_dev) != UCLASS_BLK)
> > +		return 0;
> > +
> > +	/* Add devices for each partition */
> > +	for (count = 0, part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
> > +		if (part_get_info(desc, part, &info))
> > +			continue;
> > +		snprintf(devname, sizeof(devname), "%s:%d", blk_dev->name,
> > +			 part);
> > +
> > +		ret = device_bind_driver(blk_dev, "blk_partition",
> > +					 strdup(devname), &dev);
> > +		if (ret)
> > +			return ret;
> > +
> > +		part_data = dev_get_uclass_plat(dev);
> > +		part_data->partnum = part;
> > +		part_data->gpt_part_info = info;
> > +		count++;
> > +
> > +		ret = device_probe(dev);
> > +		if (ret) {
> > +			debug("Can't probe\n");
> > +			count--;
> > +			device_unbind(dev);
> > +
> > +			continue;
> > +		}
> > +	}
> > +	debug("%s: %d partitions found in %s\n", __func__, count,
> > +	      blk_dev->name);
> > +
> > +	return 0;
> > +}
> > +
> > +static ulong blk_part_read(struct udevice *dev, lbaint_t start,
> > +			   lbaint_t blkcnt, void *buffer)
> > +{
> > +	struct udevice *parent;
> > +	struct disk_part *part;
> > +	const struct blk_ops *ops;
> > +
> > +	parent = dev_get_parent(dev);
> > +	ops = blk_get_ops(parent);
> > +	if (!ops->read)
> > +		return -ENOSYS;
> > +
> > +	part = dev_get_uclass_plat(dev);
> > +	if (start >= part->gpt_part_info.size)
> > +		return 0;
> > +
> > +	if ((start + blkcnt) > part->gpt_part_info.size)
> > +		blkcnt = part->gpt_part_info.size - start;
> > +	start += part->gpt_part_info.start;
> > +
> > +	return ops->read(parent, start, blkcnt, buffer);
> > +}
> > +
> > +static ulong blk_part_write(struct udevice *dev, lbaint_t start,
> > +			    lbaint_t blkcnt, const void *buffer)
> > +{
> > +	struct udevice *parent;
> > +	struct disk_part *part;
> > +	const struct blk_ops *ops;
> > +
> > +	parent = dev_get_parent(dev);
> > +	ops = blk_get_ops(parent);
> > +	if (!ops->write)
> > +		return -ENOSYS;
> > +
> > +	part = dev_get_uclass_plat(dev);
> > +	if (start >= part->gpt_part_info.size)
> > +		return 0;
> > +
> > +	if ((start + blkcnt) > part->gpt_part_info.size)
> > +		blkcnt = part->gpt_part_info.size - start;
> > +	start += part->gpt_part_info.start;
> > +
> > +	return ops->write(parent, start, blkcnt, buffer);
> > +}
> > +
> > +static ulong blk_part_erase(struct udevice *dev, lbaint_t start,
> > +			    lbaint_t blkcnt)
> > +{
> > +	struct udevice *parent;
> > +	struct disk_part *part;
> > +	const struct blk_ops *ops;
> > +
> > +	parent = dev_get_parent(dev);
> > +	ops = blk_get_ops(parent);
> > +	if (!ops->erase)
> > +		return -ENOSYS;
> > +
> > +	part = dev_get_uclass_plat(dev);
> > +	if (start >= part->gpt_part_info.size)
> > +		return 0;
> > +
> > +	if ((start + blkcnt) > part->gpt_part_info.size)
> > +		blkcnt = part->gpt_part_info.size - start;
> > +	start += part->gpt_part_info.start;
> > +
> > +	return ops->erase(parent, start, blkcnt);
> > +}
> > +
> > +static const struct blk_ops blk_part_ops = {
> > +	.read	= blk_part_read,
> > +	.write	= blk_part_write,
> > +	.erase	= blk_part_erase,
> > +};
> > +
> > +U_BOOT_DRIVER(blk_partition) = {
> > +	.name		= "blk_partition",
> > +	.id		= UCLASS_PARTITION,
> > +	.ops		= &blk_part_ops,
> > +};
> > +
> > +UCLASS_DRIVER(partition) = {
> > +	.id		= UCLASS_PARTITION,
> > +	.per_device_plat_auto	= sizeof(struct disk_part),
> > +	.name		= "partition",
> > +};
> > diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> > index 0e26e1d13824..230b1ea528cf 100644
> > --- a/include/dm/uclass-id.h
> > +++ b/include/dm/uclass-id.h
> > @@ -83,6 +83,7 @@ enum uclass_id {
> >   	UCLASS_P2SB,		/* (x86) Primary-to-Sideband Bus */
> >   	UCLASS_PANEL,		/* Display panel, such as an LCD */
> >   	UCLASS_PANEL_BACKLIGHT,	/* Backlight controller for panel */
> > +	UCLASS_PARTITION,	/* Logical disk partition device */
> >   	UCLASS_PCH,		/* x86 platform controller hub */
> >   	UCLASS_PCI,		/* PCI bus */
> >   	UCLASS_PCI_EP,		/* PCI endpoint device */
> > diff --git a/include/part.h b/include/part.h
> > index 53cfbdd87671..95e30e60af10 100644
> > --- a/include/part.h
> > +++ b/include/part.h
> > @@ -253,6 +253,17 @@ void part_set_generic_name(const struct blk_desc *dev_desc,
> >   	int part_num, char *name);
> > 
> >   extern const struct block_drvr block_drvr[];
> > +
> > +struct udevice;
> > +/**
> > + * part_create_block_devices - Create block devices for disk partitions
> > + *
> > + * Create UCLASS_PARTITION udevices for each of disk partitions in @parent
> > + *
> > + * @blk_dev:	Whole disk device
> > + */
> > +int part_create_block_devices(struct udevice *blk_dev);
> > +
> >   #else
> >   static inline struct blk_desc *blk_get_dev(const char *ifname, int dev)
> >   { return NULL; }
> 

Re: [PATCH v3 11/19] dm: disk: add UCLASS_PARTITION

[AKASHI Takahiro]

On Wed, Apr 13, 2022 at 12:16:00PM +0900, AKASHI Takahiro wrote:
> Heinrich,
> (and Simon)
> 
> On Sat, Apr 09, 2022 at 09:05:07PM +0200, Heinrich Schuchardt wrote:
> > On 3/8/22 12:36, AKASHI Takahiro wrote:
> > > With this new function, UCLASS_PARTITION devices will be created as
> > > child nodes of UCLASS_BLK device.
> > > 
> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > ---
> > >   disk/Makefile          |   3 +
> > >   disk/disk-uclass.c     | 153 +++++++++++++++++++++++++++++++++++++++++
> > >   include/dm/uclass-id.h |   1 +
> > >   include/part.h         |  11 +++
> > >   4 files changed, 168 insertions(+)
> > >   create mode 100644 disk/disk-uclass.c
> > > 
> > > diff --git a/disk/Makefile b/disk/Makefile
> > > index 6ce5a687b36c..ec37b74f5f40 100644
> > > --- a/disk/Makefile
> > > +++ b/disk/Makefile
> > > @@ -6,6 +6,9 @@
> > >   #ccflags-y += -DET_DEBUG -DDEBUG
> > > 
> > >   obj-$(CONFIG_PARTITIONS)	+= part.o
> > > +ifdef CONFIG_$(SPL_)BLK
> > > +obj-$(CONFIG_PARTITIONS) 	+= disk-uclass.o
> > 
> > phycore-rk3288_defconfig has CONFIG_SPL_LIBDISK_SUPPORT=n ("Support disk
> > partitions").
> 
> I think that SPL_LIBDISK_SUPPORT no longer make any sense and is obsolete.
> 
> > But spl/disk/disk-uclass is built.
> 
> Right, but what is strange is:
> - SPL_PARTITION is enabled by "default y if PARTITONS", while
> - none of any particular partition types is enabled for spl
>   due to *explicit* disablement of
>      # CONFIG_SPL_DOS_PARTITION is not set
>      # CONFIG_SPL_EFI_PARTITION is not set
>   in phycore-rk3288_defconfig.
> 
> I think that disk/Kconfig should be revised.

I still believe so, but we can also fix the issue specifically on rk3288
by adding
    # CONFIG_SPL_PARTITIONS is not set
    # CONFIG_TPL_PARTITIONS is not set
to phycore-rk3288_defconfig.

Are you satisfied with this solution?

-Takahiro Akashi

> -Takahiro Akashi
> 
> > This leads to an unnecessary size increase of the SPL image.
> > With this size increase building fails.
> > 
> > Best regards
> > 
> > Heinrich
> > 
> > > +endif
> > >   obj-$(CONFIG_$(SPL_)MAC_PARTITION)   += part_mac.o
> > >   obj-$(CONFIG_$(SPL_)DOS_PARTITION)   += part_dos.o
> > >   obj-$(CONFIG_$(SPL_)ISO_PARTITION)   += part_iso.o
> > > diff --git a/disk/disk-uclass.c b/disk/disk-uclass.c
> > > new file mode 100644
> > > index 000000000000..4918a2f72d1e
> > > --- /dev/null
> > > +++ b/disk/disk-uclass.c
> > > @@ -0,0 +1,153 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + *  Software partition device (UCLASS_PARTITION)
> > > + *
> > > + *  Copyright (c) 2021 Linaro Limited
> > > + *			Author: AKASHI Takahiro
> > > + */
> > > +
> > > +#define LOG_CATEGORY UCLASS_PARTITION
> > > +
> > > +#include <blk.h>
> > > +#include <dm.h>
> > > +#include <log.h>
> > > +#include <part.h>
> > > +#include <vsprintf.h>
> > > +#include <dm/device-internal.h>
> > > +#include <dm/lists.h>
> > > +
> > > +int part_create_block_devices(struct udevice *blk_dev)
> > > +{
> > > +	int part, count;
> > > +	struct blk_desc *desc = dev_get_uclass_plat(blk_dev);
> > > +	struct disk_partition info;
> > > +	struct disk_part *part_data;
> > > +	char devname[32];
> > > +	struct udevice *dev;
> > > +	int ret;
> > > +
> > > +	if (!CONFIG_IS_ENABLED(PARTITIONS) ||
> > > +	    !CONFIG_IS_ENABLED(HAVE_BLOCK_DEVICE))
> > > +		return 0;
> > > +
> > > +	if (device_get_uclass_id(blk_dev) != UCLASS_BLK)
> > > +		return 0;
> > > +
> > > +	/* Add devices for each partition */
> > > +	for (count = 0, part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
> > > +		if (part_get_info(desc, part, &info))
> > > +			continue;
> > > +		snprintf(devname, sizeof(devname), "%s:%d", blk_dev->name,
> > > +			 part);
> > > +
> > > +		ret = device_bind_driver(blk_dev, "blk_partition",
> > > +					 strdup(devname), &dev);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		part_data = dev_get_uclass_plat(dev);
> > > +		part_data->partnum = part;
> > > +		part_data->gpt_part_info = info;
> > > +		count++;
> > > +
> > > +		ret = device_probe(dev);
> > > +		if (ret) {
> > > +			debug("Can't probe\n");
> > > +			count--;
> > > +			device_unbind(dev);
> > > +
> > > +			continue;
> > > +		}
> > > +	}
> > > +	debug("%s: %d partitions found in %s\n", __func__, count,
> > > +	      blk_dev->name);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static ulong blk_part_read(struct udevice *dev, lbaint_t start,
> > > +			   lbaint_t blkcnt, void *buffer)
> > > +{
> > > +	struct udevice *parent;
> > > +	struct disk_part *part;
> > > +	const struct blk_ops *ops;
> > > +
> > > +	parent = dev_get_parent(dev);
> > > +	ops = blk_get_ops(parent);
> > > +	if (!ops->read)
> > > +		return -ENOSYS;
> > > +
> > > +	part = dev_get_uclass_plat(dev);
> > > +	if (start >= part->gpt_part_info.size)
> > > +		return 0;
> > > +
> > > +	if ((start + blkcnt) > part->gpt_part_info.size)
> > > +		blkcnt = part->gpt_part_info.size - start;
> > > +	start += part->gpt_part_info.start;
> > > +
> > > +	return ops->read(parent, start, blkcnt, buffer);
> > > +}
> > > +
> > > +static ulong blk_part_write(struct udevice *dev, lbaint_t start,
> > > +			    lbaint_t blkcnt, const void *buffer)
> > > +{
> > > +	struct udevice *parent;
> > > +	struct disk_part *part;
> > > +	const struct blk_ops *ops;
> > > +
> > > +	parent = dev_get_parent(dev);
> > > +	ops = blk_get_ops(parent);
> > > +	if (!ops->write)
> > > +		return -ENOSYS;
> > > +
> > > +	part = dev_get_uclass_plat(dev);
> > > +	if (start >= part->gpt_part_info.size)
> > > +		return 0;
> > > +
> > > +	if ((start + blkcnt) > part->gpt_part_info.size)
> > > +		blkcnt = part->gpt_part_info.size - start;
> > > +	start += part->gpt_part_info.start;
> > > +
> > > +	return ops->write(parent, start, blkcnt, buffer);
> > > +}
> > > +
> > > +static ulong blk_part_erase(struct udevice *dev, lbaint_t start,
> > > +			    lbaint_t blkcnt)
> > > +{
> > > +	struct udevice *parent;
> > > +	struct disk_part *part;
> > > +	const struct blk_ops *ops;
> > > +
> > > +	parent = dev_get_parent(dev);
> > > +	ops = blk_get_ops(parent);
> > > +	if (!ops->erase)
> > > +		return -ENOSYS;
> > > +
> > > +	part = dev_get_uclass_plat(dev);
> > > +	if (start >= part->gpt_part_info.size)
> > > +		return 0;
> > > +
> > > +	if ((start + blkcnt) > part->gpt_part_info.size)
> > > +		blkcnt = part->gpt_part_info.size - start;
> > > +	start += part->gpt_part_info.start;
> > > +
> > > +	return ops->erase(parent, start, blkcnt);
> > > +}
> > > +
> > > +static const struct blk_ops blk_part_ops = {
> > > +	.read	= blk_part_read,
> > > +	.write	= blk_part_write,
> > > +	.erase	= blk_part_erase,
> > > +};
> > > +
> > > +U_BOOT_DRIVER(blk_partition) = {
> > > +	.name		= "blk_partition",
> > > +	.id		= UCLASS_PARTITION,
> > > +	.ops		= &blk_part_ops,
> > > +};
> > > +
> > > +UCLASS_DRIVER(partition) = {
> > > +	.id		= UCLASS_PARTITION,
> > > +	.per_device_plat_auto	= sizeof(struct disk_part),
> > > +	.name		= "partition",
> > > +};
> > > diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> > > index 0e26e1d13824..230b1ea528cf 100644
> > > --- a/include/dm/uclass-id.h
> > > +++ b/include/dm/uclass-id.h
> > > @@ -83,6 +83,7 @@ enum uclass_id {
> > >   	UCLASS_P2SB,		/* (x86) Primary-to-Sideband Bus */
> > >   	UCLASS_PANEL,		/* Display panel, such as an LCD */
> > >   	UCLASS_PANEL_BACKLIGHT,	/* Backlight controller for panel */
> > > +	UCLASS_PARTITION,	/* Logical disk partition device */
> > >   	UCLASS_PCH,		/* x86 platform controller hub */
> > >   	UCLASS_PCI,		/* PCI bus */
> > >   	UCLASS_PCI_EP,		/* PCI endpoint device */
> > > diff --git a/include/part.h b/include/part.h
> > > index 53cfbdd87671..95e30e60af10 100644
> > > --- a/include/part.h
> > > +++ b/include/part.h
> > > @@ -253,6 +253,17 @@ void part_set_generic_name(const struct blk_desc *dev_desc,
> > >   	int part_num, char *name);
> > > 
> > >   extern const struct block_drvr block_drvr[];
> > > +
> > > +struct udevice;
> > > +/**
> > > + * part_create_block_devices - Create block devices for disk partitions
> > > + *
> > > + * Create UCLASS_PARTITION udevices for each of disk partitions in @parent
> > > + *
> > > + * @blk_dev:	Whole disk device
> > > + */
> > > +int part_create_block_devices(struct udevice *blk_dev);
> > > +
> > >   #else
> > >   static inline struct blk_desc *blk_get_dev(const char *ifname, int dev)
> > >   { return NULL; }
> > 

Re: [PATCH v3 11/19] dm: disk: add UCLASS_PARTITION

[Heinrich Schuchardt]

On 4/14/22 08:17, AKASHI Takahiro wrote:
> On Wed, Apr 13, 2022 at 12:16:00PM +0900, AKASHI Takahiro wrote:
>> Heinrich,
>> (and Simon)
>>
>> On Sat, Apr 09, 2022 at 09:05:07PM +0200, Heinrich Schuchardt wrote:
>>> On 3/8/22 12:36, AKASHI Takahiro wrote:
>>>> With this new function, UCLASS_PARTITION devices will be created as
>>>> child nodes of UCLASS_BLK device.
>>>>
>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>>> ---
>>>>    disk/Makefile          |   3 +
>>>>    disk/disk-uclass.c     | 153 +++++++++++++++++++++++++++++++++++++++++
>>>>    include/dm/uclass-id.h |   1 +
>>>>    include/part.h         |  11 +++
>>>>    4 files changed, 168 insertions(+)
>>>>    create mode 100644 disk/disk-uclass.c
>>>>
>>>> diff --git a/disk/Makefile b/disk/Makefile
>>>> index 6ce5a687b36c..ec37b74f5f40 100644
>>>> --- a/disk/Makefile
>>>> +++ b/disk/Makefile
>>>> @@ -6,6 +6,9 @@
>>>>    #ccflags-y += -DET_DEBUG -DDEBUG
>>>>
>>>>    obj-$(CONFIG_PARTITIONS)	+= part.o
>>>> +ifdef CONFIG_$(SPL_)BLK
>>>> +obj-$(CONFIG_PARTITIONS) 	+= disk-uclass.o
>>>
>>> phycore-rk3288_defconfig has CONFIG_SPL_LIBDISK_SUPPORT=n ("Support disk
>>> partitions").
>>
>> I think that SPL_LIBDISK_SUPPORT no longer make any sense and is obsolete.
>>
>>> But spl/disk/disk-uclass is built.
>>
>> Right, but what is strange is:
>> - SPL_PARTITION is enabled by "default y if PARTITONS", while
>> - none of any particular partition types is enabled for spl
>>    due to *explicit* disablement of
>>       # CONFIG_SPL_DOS_PARTITION is not set
>>       # CONFIG_SPL_EFI_PARTITION is not set
>>    in phycore-rk3288_defconfig.
>>
>> I think that disk/Kconfig should be revised.
>
> I still believe so, but we can also fix the issue specifically on rk3288
> by adding
>      # CONFIG_SPL_PARTITIONS is not set
>      # CONFIG_TPL_PARTITIONS is not set
> to phycore-rk3288_defconfig.
>
> Are you satisfied with this solution?

I don't care too much about that specific board. We should avoid
unnecessary SPL size increase for all boards.

The fix should be on the Kconfig level and not on the board level.

Best regards

Heinrich

>
> -Takahiro Akashi
>
>> -Takahiro Akashi
>>
>>> This leads to an unnecessary size increase of the SPL image.
>>> With this size increase building fails.
>>>
>>> Best regards
>>>
>>> Heinrich
>>>
>>>> +endif
>>>>    obj-$(CONFIG_$(SPL_)MAC_PARTITION)   += part_mac.o
>>>>    obj-$(CONFIG_$(SPL_)DOS_PARTITION)   += part_dos.o
>>>>    obj-$(CONFIG_$(SPL_)ISO_PARTITION)   += part_iso.o
>>>> diff --git a/disk/disk-uclass.c b/disk/disk-uclass.c
>>>> new file mode 100644
>>>> index 000000000000..4918a2f72d1e
>>>> --- /dev/null
>>>> +++ b/disk/disk-uclass.c
>>>> @@ -0,0 +1,153 @@
>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>> +/*
>>>> + *  Software partition device (UCLASS_PARTITION)
>>>> + *
>>>> + *  Copyright (c) 2021 Linaro Limited
>>>> + *			Author: AKASHI Takahiro
>>>> + */
>>>> +
>>>> +#define LOG_CATEGORY UCLASS_PARTITION
>>>> +
>>>> +#include <blk.h>
>>>> +#include <dm.h>
>>>> +#include <log.h>
>>>> +#include <part.h>
>>>> +#include <vsprintf.h>
>>>> +#include <dm/device-internal.h>
>>>> +#include <dm/lists.h>
>>>> +
>>>> +int part_create_block_devices(struct udevice *blk_dev)
>>>> +{
>>>> +	int part, count;
>>>> +	struct blk_desc *desc = dev_get_uclass_plat(blk_dev);
>>>> +	struct disk_partition info;
>>>> +	struct disk_part *part_data;
>>>> +	char devname[32];
>>>> +	struct udevice *dev;
>>>> +	int ret;
>>>> +
>>>> +	if (!CONFIG_IS_ENABLED(PARTITIONS) ||
>>>> +	    !CONFIG_IS_ENABLED(HAVE_BLOCK_DEVICE))
>>>> +		return 0;
>>>> +
>>>> +	if (device_get_uclass_id(blk_dev) != UCLASS_BLK)
>>>> +		return 0;
>>>> +
>>>> +	/* Add devices for each partition */
>>>> +	for (count = 0, part = 1; part <= MAX_SEARCH_PARTITIONS; part++) {
>>>> +		if (part_get_info(desc, part, &info))
>>>> +			continue;
>>>> +		snprintf(devname, sizeof(devname), "%s:%d", blk_dev->name,
>>>> +			 part);
>>>> +
>>>> +		ret = device_bind_driver(blk_dev, "blk_partition",
>>>> +					 strdup(devname), &dev);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +
>>>> +		part_data = dev_get_uclass_plat(dev);
>>>> +		part_data->partnum = part;
>>>> +		part_data->gpt_part_info = info;
>>>> +		count++;
>>>> +
>>>> +		ret = device_probe(dev);
>>>> +		if (ret) {
>>>> +			debug("Can't probe\n");
>>>> +			count--;
>>>> +			device_unbind(dev);
>>>> +
>>>> +			continue;
>>>> +		}
>>>> +	}
>>>> +	debug("%s: %d partitions found in %s\n", __func__, count,
>>>> +	      blk_dev->name);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static ulong blk_part_read(struct udevice *dev, lbaint_t start,
>>>> +			   lbaint_t blkcnt, void *buffer)
>>>> +{
>>>> +	struct udevice *parent;
>>>> +	struct disk_part *part;
>>>> +	const struct blk_ops *ops;
>>>> +
>>>> +	parent = dev_get_parent(dev);
>>>> +	ops = blk_get_ops(parent);
>>>> +	if (!ops->read)
>>>> +		return -ENOSYS;
>>>> +
>>>> +	part = dev_get_uclass_plat(dev);
>>>> +	if (start >= part->gpt_part_info.size)
>>>> +		return 0;
>>>> +
>>>> +	if ((start + blkcnt) > part->gpt_part_info.size)
>>>> +		blkcnt = part->gpt_part_info.size - start;
>>>> +	start += part->gpt_part_info.start;
>>>> +
>>>> +	return ops->read(parent, start, blkcnt, buffer);
>>>> +}
>>>> +
>>>> +static ulong blk_part_write(struct udevice *dev, lbaint_t start,
>>>> +			    lbaint_t blkcnt, const void *buffer)
>>>> +{
>>>> +	struct udevice *parent;
>>>> +	struct disk_part *part;
>>>> +	const struct blk_ops *ops;
>>>> +
>>>> +	parent = dev_get_parent(dev);
>>>> +	ops = blk_get_ops(parent);
>>>> +	if (!ops->write)
>>>> +		return -ENOSYS;
>>>> +
>>>> +	part = dev_get_uclass_plat(dev);
>>>> +	if (start >= part->gpt_part_info.size)
>>>> +		return 0;
>>>> +
>>>> +	if ((start + blkcnt) > part->gpt_part_info.size)
>>>> +		blkcnt = part->gpt_part_info.size - start;
>>>> +	start += part->gpt_part_info.start;
>>>> +
>>>> +	return ops->write(parent, start, blkcnt, buffer);
>>>> +}
>>>> +
>>>> +static ulong blk_part_erase(struct udevice *dev, lbaint_t start,
>>>> +			    lbaint_t blkcnt)
>>>> +{
>>>> +	struct udevice *parent;
>>>> +	struct disk_part *part;
>>>> +	const struct blk_ops *ops;
>>>> +
>>>> +	parent = dev_get_parent(dev);
>>>> +	ops = blk_get_ops(parent);
>>>> +	if (!ops->erase)
>>>> +		return -ENOSYS;
>>>> +
>>>> +	part = dev_get_uclass_plat(dev);
>>>> +	if (start >= part->gpt_part_info.size)
>>>> +		return 0;
>>>> +
>>>> +	if ((start + blkcnt) > part->gpt_part_info.size)
>>>> +		blkcnt = part->gpt_part_info.size - start;
>>>> +	start += part->gpt_part_info.start;
>>>> +
>>>> +	return ops->erase(parent, start, blkcnt);
>>>> +}
>>>> +
>>>> +static const struct blk_ops blk_part_ops = {
>>>> +	.read	= blk_part_read,
>>>> +	.write	= blk_part_write,
>>>> +	.erase	= blk_part_erase,
>>>> +};
>>>> +
>>>> +U_BOOT_DRIVER(blk_partition) = {
>>>> +	.name		= "blk_partition",
>>>> +	.id		= UCLASS_PARTITION,
>>>> +	.ops		= &blk_part_ops,
>>>> +};
>>>> +
>>>> +UCLASS_DRIVER(partition) = {
>>>> +	.id		= UCLASS_PARTITION,
>>>> +	.per_device_plat_auto	= sizeof(struct disk_part),
>>>> +	.name		= "partition",
>>>> +};
>>>> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
>>>> index 0e26e1d13824..230b1ea528cf 100644
>>>> --- a/include/dm/uclass-id.h
>>>> +++ b/include/dm/uclass-id.h
>>>> @@ -83,6 +83,7 @@ enum uclass_id {
>>>>    	UCLASS_P2SB,		/* (x86) Primary-to-Sideband Bus */
>>>>    	UCLASS_PANEL,		/* Display panel, such as an LCD */
>>>>    	UCLASS_PANEL_BACKLIGHT,	/* Backlight controller for panel */
>>>> +	UCLASS_PARTITION,	/* Logical disk partition device */
>>>>    	UCLASS_PCH,		/* x86 platform controller hub */
>>>>    	UCLASS_PCI,		/* PCI bus */
>>>>    	UCLASS_PCI_EP,		/* PCI endpoint device */
>>>> diff --git a/include/part.h b/include/part.h
>>>> index 53cfbdd87671..95e30e60af10 100644
>>>> --- a/include/part.h
>>>> +++ b/include/part.h
>>>> @@ -253,6 +253,17 @@ void part_set_generic_name(const struct blk_desc *dev_desc,
>>>>    	int part_num, char *name);
>>>>
>>>>    extern const struct block_drvr block_drvr[];
>>>> +
>>>> +struct udevice;
>>>> +/**
>>>> + * part_create_block_devices - Create block devices for disk partitions
>>>> + *
>>>> + * Create UCLASS_PARTITION udevices for each of disk partitions in @parent
>>>> + *
>>>> + * @blk_dev:	Whole disk device
>>>> + */
>>>> +int part_create_block_devices(struct udevice *blk_dev);
>>>> +
>>>>    #else
>>>>    static inline struct blk_desc *blk_get_dev(const char *ifname, int dev)
>>>>    { return NULL; }
>>>