[RFC 3/7] dm: implement get_dp_node for block devices

[Simon Glass]

Hi Heinrich,

On Mon, 27 Mar 2023 at 19:13, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
>
>
> On 3/27/23 06:00, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Mon, 27 Mar 2023 at 06:27, Heinrich Schuchardt
> > <heinrich.schuchardt at canonical.com> wrote:
> >>
> >> Generate a Ctrl() node for block devices.
> >>
> >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
> >> ---
> >>   drivers/block/blk-uclass.c | 56 ++++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 56 insertions(+)
> >
> > Can this go in drivers/block/blk_efi.c ?
> >
> > Can you create a function which returns the path given a device? Then
>
> That function already exists in lib/efi_loader/device_path.

Yes, but I mean one for each media type.

>
> > we are discuss the get_dp_node() member or moving to an event. I
> > favour the event for now.
>
> If for a device we already have an EFI handle with a device-path
> installed, the device-path of its children has to be constructed by
> adding a uclass specific device path node.
>
> Otherwise we have to recurse to the root node to collect all device path
> nodes.

OK

>
> >
> >>
> >> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
> >> index c69fc4d518..08202aaaba 100644
> >> --- a/drivers/block/blk-uclass.c
> >> +++ b/drivers/block/blk-uclass.c
> >> @@ -9,6 +9,7 @@
> >>   #include <common.h>
> >>   #include <blk.h>
> >>   #include <dm.h>
> >> +#include <efi_loader.h>
> >>   #include <log.h>
> >>   #include <malloc.h>
> >>   #include <part.h>
> >> @@ -779,9 +780,64 @@ static int blk_post_probe(struct udevice *dev)
> >>          return 0;
> >>   }
> >>
> >> +#if CONFIG_IS_ENABLED(EFI_LOADER)
> >> +__maybe_unused
> >> +static struct efi_device_path *blk_scsi_get_dp_node(struct udevice *dev)
> >> +{
> >> +       struct efi_device_path_scsi *dp;
> >> +       struct blk_desc *desc = dev_get_uclass_plat(dev);
> >> +
> >> +       dp = efi_alloc(sizeof(struct efi_device_path_scsi));
> >> +       if (!dp)
> >> +               return NULL;
> >> +
> >> +       dp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
> >> +       dp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_SCSI;
> >> +       dp->dp.length = sizeof(*dp);
> >> +       dp->logical_unit_number = desc->lun;
> >> +       dp->target_id = desc->target;
> >> +
> >> +       return &dp->dp;
> >> +}
> >> +
> >> +static struct efi_device_path *blk_get_dp_node(struct udevice *dev)
> >> +{
> >> +       struct efi_device_path_controller *dp;
> >> +       struct blk_desc *desc = dev_get_uclass_plat(dev);
> >> +       u32 controller_number;
> >> +
> >> +       switch (device_get_uclass_id(dev->parent)) {
> >> +#if CONFIG_IS_ENABLED(SCSI)
> >> +       case UCLASS_SCSI:
> >> +               return blk_scsi_get_dp_node(dev);
> >> +#endif
> >> +       case UCLASS_MMC:
> >> +               dp->controller_number = desc->hwpart;
> >> +               break;
> >> +       default:
> >
> > Since this is checking the parent class, it seems to me that this info
> > should be attacked there (the 'media' device) instead of the block
> > device.
>
> For MMC we have these layers:
>
> * MMC controller - UCLASS_MMC
> * MMC slot (or channel) - missing uclass
> * hardware partition - UCLASS_BLK
>
> See https://www.sage-micro.com/us/portfolio-items/s881/
>
> Currently in the DM tree we don't model the MMC controller and the MMC
> slot separately. I think this is a gap that should be closed.
>
> A single UCLASS_MMC (= MMC slot) device can have multiple UCLASS_BLK
> children. One for each hardware partition.
>
> An MMC controller in EFI is modelled as VenHW() node,
> a slot as SD() or eMMC() node.
>
> An eMMC() or SD() node has a field for the slot number but none for the
> hardware partition. Hence, we cannot attach the hardware partition
> information to the UCLASS_MMC device. For the UCLASS_BLK device I use a
> Ctrl() node. We could also consider using a Unit() node but Unit() nodes
> are meant to be used for LUNs.
>
> We have a similar situation for USB and SCSI LUNs.

The current model is to add multiple BLK children for each media
device, if needed. That seems to work OK for now. What do you think?

Regards,
Simon