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

[Heinrich Schuchardt]

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)

Best regards

Heinrich