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

[Simon Glass]

Hi Heinrich,

On Mon, 27 Mar 2023 at 22:31, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
> On 3/27/23 10:24, Simon Glass wrote:
> > 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?
>
> There is nothing in DM tree we need to change now. As I understood your
> first reply it was not clear to you which EFI device-path node type with
> which information matches which DM tree node:
>
> MMC controller/MMC slot (UCLASS_MMC) -> SD(slot) or MMC(slot)
> Hardware partition (UCLASS_BLK) -> Ctrl()
>
> SCSI drive (UCLASS_SCSI) -> VenHw()
> SCSI LUN (UClASS_BLK) -> Scsi(PUN, LUN)

OK, so this is explaining is why you must attach the functions to the
BLK device rather than the storage device? If so, I understand.

What are VenHw and Ctrl ?

Regrads,
Simon