[U-Boot] [RFC 2/3] efi_loader: associate BLK/PARTITION device to efi_disk

[AKASHI Takahiro]

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.akashi at 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.akashi at 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?
* 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