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

[Heinrich Schuchardt]

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.

> 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.

> 
>>
>> 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.

Best regards

Heinrich

> 
>> +               dp->controller_number = desc->lun;
>> +               break;
>> +       }
>> +
>> +       dp = efi_alloc(sizeof(struct efi_device_path_controller));
>> +       if (!dp)
>> +               return NULL;
>> +
>> +       dp->dp.type           = DEVICE_PATH_TYPE_HARDWARE_DEVICE;
>> +       dp->dp.sub_type       = DEVICE_PATH_SUB_TYPE_CONTROLLER;
>> +       dp->dp.length         = sizeof(*dp);
>> +       dp->controller_number = controller_number;
>> +
>> +       return &dp->dp;
>> +}
>> +#endif
>> +
>>   UCLASS_DRIVER(blk) = {
>>          .id             = UCLASS_BLK,
>>          .name           = "blk",
>>          .post_probe     = blk_post_probe,
>>          .per_device_plat_auto   = sizeof(struct blk_desc),
>> +#if CONFIG_IS_ENABLED(EFI_LOADER)
>> +       .get_dp_node    = blk_get_dp_node,
>> +#endif
>>   };
>> --
>> 2.39.2
>>
> 
> Regards,
> Simon