On Wed, Feb 06, 2019 at 12:15:11PM +0900, AKASHI Takahiro wrote: > On Sat, Feb 02, 2019 at 07:15:53AM -0700, Simon Glass wrote: > > Hi, > > > > On Thu, 31 Jan 2019 at 22:53, AKASHI Takahiro > > <takahiro.aka...@linaro.org> wrote: > > > > > > Hi Simon, > > > > > > Thank you for suggestive comments. > > > I've got no idea of making DM class for EFI protocol. > > > > > > On Wed, Jan 30, 2019 at 06:22:47PM -0700, Simon Glass wrote: > > > > Hi AKASHI, > > > > > > > > On Mon, 28 Jan 2019 at 19:59, AKASHI Takahiro > > > > <takahiro.aka...@linaro.org> wrote: > > > > > > > > > > efi_disk_create() will initialize efi_disk attributes for each device, > > > > > either UCLASS_BLK or UCLASS_PARTITION. > > > > > > > > > > Currently (temporarily), efi_disk_obj structure is embedded into > > > > > blk_desc to hold efi-specific attributes. > > > > > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.aka...@linaro.org> > > > > > --- > > > > > drivers/block/blk-uclass.c | 9 ++ > > > > > drivers/core/device.c | 3 + > > > > > include/blk.h | 24 +++++ > > > > > include/dm/device.h | 4 + > > > > > lib/efi_loader/efi_disk.c | 192 > > > > > ++++++++++++++++++++++++++----------- > > > > > 5 files changed, 174 insertions(+), 58 deletions(-) > > > > > > > > > > > > > I think the objective should be to drop the EFI data structures. > > > > > > More or less so, yes. > > > > > > > So your approach of just including them in DM structures seems about > > > > right to me, as a short-term migration measure. Given the large amount > > > > of code that has built up I don't think it is possible to do it any > > > > other way. > > > > > > Right. > > > > > > > It is very unfortunate though. > > > > > > > > So basically migration could be something like this: > > > > > > > > 1. Move each of the EFI structs into the DM structs one by one > > > > 2. Drop struct members that are not needed and can be calculated from > > > > DM members > > > > 3. Update DM to have 1:1 uclasses for each EFI protocol > > > > 4. Move all the protocol structs into the DM uclasses > > > > 5. Whittle these down until they are just shells used by the API, with > > > > everything going through normal DM calls > > > > > > > > Or would it be better to just start again and rewrite it with the > > > > existing code as a base? > > > > > > > > Looking at it, efi_object is not very useful in DM. It contains two > > > > members: > > > > > > > > 1. link - linked list link, which devices already have, although we > > > > don't have a single list of all them. Instead they are linked into > > > > separate lists for each uclass > > > > > > > > 2. protocols - list of protocols. But DM devices support only one > > > > protocol. Multiple protocols are handled using child devices. E.g a > > > > UCLASS_PMIC device that supports UCLASS_GPIO, UCLASS_REGULATOR and > > > > UCLASS_AUDIO_CODEC would have three children, one for each uclass. So > > > > perhaps with EFI we should create a separate child for each protocol > > > > in a similar way? > > > > > > > > Which comes back to the idea of creating an EFI child device for every > > > > DM device. Perhaps instead we create one EFI child for each protocol > > > > supported by the parent device? > > > > > > Well, "child device as a EFI protocol" is really workable, but > > > I have some concerns: > > > * Can a DM device be an abstract instance with no real hardware? > > > > Yes we do that quite a bit. Even UCLASS_BLK is like this, if you think > > about it. > > OK > > > > * There may be two different types of "children" mixed for an EFI object > > > - some physical hierarchy, say disk partitions for a raw disk > > > - these EFI protocols > > > That is, for example, one raw disk has > > > - partition 1 has > > > - block io protocol > > > - device path protocol > > > - simple file system protocol > > > - partition 2 has > > > - block io protocol > > > - device path protocol > > > - simple file system protocol > > > - block io protocol > > > - device path protocol > > > * device path protocol can be annoying as almost all devices (visible > > > to UEFI) have some sort of device path, while corresponding u-boot > > > notion is, say, "scsi 0:1" which only appears on command interfaces. > > > > Yes. We could use the device path from concatenating device names from > > all parents, perhaps. I've been thinking about adding that to the > > command line as an option. > > Device path is kinda device hierarchy of DM, so it's easily confusing. > Please see an example below. > > > > > > > I'm not sure if those concerns are acceptable. > > > > > > > Another point is that the operations supported by EFI should be in DM > > > > operations structs. For example I think struct > > > > efi_simple_text_output_protocol should have methods which call into > > > > the corresponding uclass operations. > > > > > > I have no idea on those "console" devices yet. > > > > > > > It is confusing that an EFI disk is in fact a partition. Or do I have > > > > that wrong? > > > > > > IMO, EFI disk is any type of EFI object which supports > > > EFI_BLOCK_IO_PROTOCOL. > > > So a raw disk(UCLASS_BLK) as well as its partitions(UCLASS_PARTITION) are > > > EFI disks as well. > > > Is this an answer to you? > > > > Yes OK, I see. > > > > > > > > Those said, your suggestion is truly worth considering. > > > > OK, good. Certainly an interesting project. > > Conversion of efi protocols to uclass device(UCLASS_PROTOCOL) was done > and efi_handle_t is now 'struct udevice *'!
To be clear, what is converted to DM device here is "struct efi_handler," not "protocol interface" structure in UEFI world. The latter is a well-defined data structure in UEFI spec and cannot be treated as a DM device. > I could do this almost "systematically," but the code size doesn't decrease > much. This is no surprise because we rely on udevice only for traversing > efi handles and protocols. We still need lots of "wrapper" layers > on top of block devices, file systems and so on due to differences > of APIs and their semantics. > (Because of this, I don't have good confidence yet that efi protocol > should also be a device in DM.) > > Here is a sample operation: > > => efi dev > Device Device Path > ================ ==================== > 000000007eef9590 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) > => scsi rescan > > Reset SCSI > scanning bus for devices... > Target spinup took 0 ms. > Target spinup took 0 ms. > SATA link 2 timeout. > SATA link 3 timeout. > SATA link 4 timeout. > SATA link 5 timeout. > AHCI 0001.0000 32 slots 6 ports 1.5 Gbps 0x3f impl SATA mode > flags: 64bit ncq only > Device 0: (0:0) Vendor: ATA Prod.: QEMU HARDDISK Rev: 2.5+ > Type: Hard Disk > Capacity: 16.0 MB = 0.0 GB (32768 x 512) > Device 0: (1:0) Vendor: ATA Prod.: QEMU HARDDISK Rev: 2.5+ > Type: Hard Disk > Capacity: 256.0 MB = 0.2 GB (524288 x 512) > => efi devices > Device Device Path > ================ ==================== > 000000007eef9590 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) > 000000007ef04c50 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0) > 000000007ef02d10 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0) > 000000007ef05c70 > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(335544330,MBR,0x086246ba,0x17ff501a0,0x7eee4770) > 000000007ef06270 > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(335544330,MBR,0x086246ba,0x17ff501a0,0x7eee4770) > => efi dh > Handle Protocols > ================ ==================== > 000000007eef9590 Device Path, Device Path To Text, Device Path Utilities, > Unicode Collation 2 > 000000007ef00970 Driver Binding > 000000007eef7b80 Simple Text Output, Simple Text Input, Simple Text Input Ex > 000000007ef04c50 Block IO, Device Path > 000000007ef02d10 Block IO, Device Path > 000000007ef05c70 Block IO, Device Path, Simple File System > 000000007ef06270 Block IO, Device Path, Simple File System > => dm tree > Class index Probed Driver Name > ----------------------------------------------------------- > root 0 [ + ] root_driver root_driver > simple_bus 0 [ ] generic_simple_bus |-- platform@c000000 > virtio 0 [ + ] virtio-mmio |-- virtio_mmio@a000000 > > ... > > pci 0 [ + ] pci_generic_ecam |-- pcie@10000000 > pci_generi 0 [ ] pci_generic_drv | |-- pci_0:0.0 > virtio 32 [ ] virtio-pci.l | |-- virtio-pci.l#0 > ahci 0 [ + ] ahci_pci | `-- ahci_pci > scsi 0 [ + ] ahci_scsi | `-- ahci_scsi > blk 0 [ ] scsi_blk | |-- > ahci_scsi.id0lun0 > efi_protoc 8 [ ] efi_protocol | | |-- (PROTO) > efi_protoc 9 [ ] efi_protocol | | `-- (PROTO) > blk 1 [ ] scsi_blk | `-- > ahci_scsi.id1lun0 > efi_protoc 10 [ ] efi_protocol | |-- (PROTO) > efi_protoc 11 [ ] efi_protocol | |-- (PROTO) > partition 0 [ ] blk_partition | |-- > ahci_scsi.id1lun0:1 > efi_protoc 12 [ ] efi_protocol | | |-- (PROTO) > efi_protoc 13 [ ] efi_protocol | | |-- (PROTO) > efi_protoc 14 [ ] efi_protocol | | `-- (PROTO) > partition 1 [ ] blk_partition | `-- > ahci_scsi.id1lun0:2 > efi_protoc 15 [ ] efi_protocol | |-- (PROTO) > efi_protoc 16 [ ] efi_protocol | |-- (PROTO) > efi_protoc 17 [ ] efi_protocol | `-- (PROTO) > rtc 0 [ ] rtc-pl031 |-- pl031@9010000 > serial 0 [ ] serial_pl01x |-- pl011@9050000 > serial 1 [ + ] serial_pl01x |-- pl011@9000000 > efi_protoc 5 [ ] efi_protocol | |-- (PROTO) > efi_protoc 6 [ ] efi_protocol | |-- (PROTO) > efi_protoc 7 [ ] efi_protocol | `-- (PROTO) > mtd 0 [ + ] cfi_flash |-- flash@0 > firmware 0 [ + ] psci |-- psci > sysreset 0 [ ] psci-sysreset | `-- psci-sysreset I changed this part of output: > efi_object 0 [ ] efi_dumb_object |-- efi_root > efi_protoc 0 [ ] efi_protocol | |-- (PROTO) > efi_protoc 1 [ ] efi_protocol | |-- (PROTO) > efi_protoc 2 [ ] efi_protocol | |-- (PROTO) > efi_protoc 3 [ ] efi_protocol | `-- (PROTO) > efi_object 1 [ ] efi_dumb_object `-- EFI block driver > efi_protoc 4 [ ] efi_protocol `-- (PROTO) to efi_object 0 [ ] efi_dumb_object `-- efi_root efi_protoc 0 [ ] efi_protocol |-- (PROTO) efi_protoc 1 [ ] efi_protocol |-- (PROTO) efi_protoc 2 [ ] efi_protocol |-- (PROTO) efi_protoc 3 [ ] efi_protocol |-- (PROTO) efi 0 [ ] EFI block driver `-- EFI block driver efi_protoc 4 [ ] efi_protocol `-- (PROTO) > As you can see, I have only one efi protocol "driver" and so > there's no specific name given for each protocol now. More specifically, there seem to be a few options: 1-1. to keep one uclass, distinguishing protocols with an internal id, something like IF_TYPE_* for blk_desc 1-2. to keep one uclass, having one driver for one protocol, (Either way, there's not common "operation" btw protocols here.) 2. to give each protocol an unique uclass id Thanks, -Takahiro Akashi > As I'm still debugging my code, I will re-post my patch > once it gets back working. > > Thanks, > -Takahiro Akashi > > > Regards, > > Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot