From mboxrd@z Thu Jan 1 00:00:00 1970 From: AKASHI Takahiro Date: Fri, 1 Feb 2019 14:54:27 +0900 Subject: [U-Boot] [RFC 2/3] efi_loader: associate BLK/PARTITION device to efi_disk In-Reply-To: References: <20190129025956.21870-1-takahiro.akashi@linaro.org> <20190129025956.21870-3-takahiro.akashi@linaro.org> Message-ID: <20190201055426.GA20286@linaro.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > 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 > > --- > > 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? * 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. 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? Those said, your suggestion is truly worth considering. Thanks, -Takahiro Akashi > Anyway that's all the thoughts I have at present. Thanks for your > efforts on this. > > Regards, > Simon