Hi Mark, Thank you for the comment.
On Mon, Dec 06, 2021 at 12:41:28PM +0100, Mark Kettenis wrote: > > Date: Mon, 6 Dec 2021 13:16:23 +0900 > > From: AKASHI Takahiro <takahiro.aka...@linaro.org> > > > > On Fri, Dec 03, 2021 at 05:32:49PM +0100, Heinrich Schuchardt wrote: > > > On 11/25/21 06:44, AKASHI Takahiro wrote: > > > > Heinrich, > > > > > > > > On Wed, Nov 24, 2021 at 12:10:32PM +0900, AKASHI Takahiro wrote: > > > > > On Sat, Nov 20, 2021 at 01:54:30PM +0100, Heinrich Schuchardt wrote: > > > > > > Hello Takahiro, > > > > > > > > > > > > in a prior mail we have discussed the creation of device paths for > > > > > > USB > > > > > > mass storage devices. > > > > > > > > > > > > On the sand boxyou get the following devices after 'usb start': > > > > > > > > > > > > Class Index Probed Driver Name > > > > > > ----------------------------------------------------------- > > > > > > usb 0 [ + ] usb_sandbox |-- usb@1 > > > > > > usb_hub 0 [ + ] usb_hub | `-- hub > > > > > > usb_mass_s 0 [ + ] usb_mass_storage | |-- > > > > > > usb_mass_storage > > > > > > blk 3 [ ] usb_storage_blk | | `-- > > > > > > usb_mass_storage.lun0 > > > > > > usb_mass_s 1 [ + ] usb_mass_storage | |-- > > > > > > usb_mass_storage > > > > > > blk 4 [ ] usb_storage_blk | | `-- > > > > > > usb_mass_storage.lun0 > > > > > > usb_mass_s 2 [ + ] usb_mass_storage | `-- > > > > > > usb_mass_storage > > > > > > blk 5 [ ] usb_storage_blk | `-- > > > > > > usb_mass_storage.lun0 > > > > > > > > > > > > For of these storage devices we try to create the same device path > > > > > > which > > > > > > is not allowable: > > > > > > > > > > > > => usb start > > > > > > starting USB... > > > > > > Bus usb@1: scanning bus usb@1 for devices... 5 USB Device(s) found > > > > > > scanning usb for storage devices... 3 Storage Device(s) > > > > > > found > > > > > > => > > > > > > => efidebug dh > > > > > > Scanning disk mmc2.blk... > > > > > > handle 0000000015e34f00, > > > > > > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(2)/SD(0) > > > > > > Scanning disk mmc1.blk... > > > > > > handle 0000000015e36b30, > > > > > > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(1)/SD(1) > > > > > > Scanning disk mmc0.blk... > > > > > > handle 0000000015e35b00, > > > > > > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(2) > > > > > > Scanning disk usb_mass_storage.lun0... > > > > > > handle 0000000015e35c10, > > > > > > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/UsbClass(0x1234,0x5678,0x9,0x0,0x0)/UsbClass(0x1234,0x5678,0x0,0x0,0x0) > > > > > > fs_devread read outside partition 2 > > > > > > Failed to mount ext2 filesystem... > > > > > > BTRFS: superblock end 69632 is larger than device size 512 > > > > > > Scanning disk usb_mass_storage.lun0... > > > > > > handle 0000000015e361f0, > > > > > > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/UsbClass(0x1234,0x5678,0x9,0x0,0x0)/UsbClass(0x1234,0x5678,0x0,0x0,0x0) > > > > > > ERROR: failure to add disk device usb_mass_storage.lun0, r = 20 > > > > > > Error: Cannot initialize UEFI sub-system, r = 20 > > > > > > > > > > > > I will provide a patch that will allow the first USB device to be > > > > > > used > > > > > > and avoids stopping the boot process. But we really have to walk > > > > > > the dm > > > > > > tree to create a device patch for USB devices based on port IDs. > > > > > > > > > > > > We should add a new field to struct uclass_driver: > > > > > > > > > > > > struct efi_device_path *get_node(udevice *dev); > > > > > > > > > > > > This function shall return a pointer to an freshly allocated buffer > > > > > > with > > > > > > the device node for the device. To build the devicepath we can then > > > > > > walk > > > > > > the dm tree. > > > > > > > > > > I'm not sure this is an acceptable solution. > > > > > > > > > > Let me make sure: > > > > > The goal would be to create a device path like > > > > > .../USB(0x1,0x0)/HD(1,...) > > > > > instead of > > > > > > > > > > .../UsbHub(0x0,0x0,0x0,0x3)/UsbMassStorage(0x46f4,0x1,0x0,0x0)/HD(1,...) > > > > > as we already see this format for SCSI: > > > > > .../Scsi(0,0)/HD(1,..) > > > > > > > > > > Right? > > > > > > > > Please try the tweak attached below. > > > > (I think what I did here is trivial.) > > > > > > > > If you like, I will post it as a patch. > > > > (See "10.3.4.5.1 USB Device Path Example".) > > > > > > > > -Takahiro Akashi > > > > > > > > > > > From cda91e9d8144f89f0d73738b338289a7940bbe0e Mon Sep 17 > > > > > > > 00:00:00 2001 > > > > > > > From: AKASHI Takahiro <takahiro.aka...@linaro.org> > > > > > > > Date: Thu, 25 Nov 2021 14:20:08 +0900 > > > > > > > Subject: [PATCH] efi_loader: disk: usb mass-storage > > > > > > > > - use path_usb instead of path_usb_class for existing USB dp nodes > > > > - add a dp node for UCLASS_USB > > > > > > > > => usb start > > > > starting USB... > > > > Bus xhci_pci: Register 8001040 NbrPorts 8 > > > > Starting the controller > > > > USB XHCI 1.00 > > > > scanning bus xhci_pci for devices... 4 USB Device(s) found > > > > scanning usb for storage devices... 2 Storage Device(s) found > > > > => dm tree > > > > Class Index Probed Driver Name > > > > ----------------------------------------------------------- > > > > root 0 [ + ] root_driver root_driver > > > > ... > > > > pci 0 [ + ] pci_generic_ecam |-- pcie@10000000 > > > > pci_generi 0 [ ] pci_generic_drv | |-- pci_0:0.0 > > > > virtio 32 [ + ] virtio-pci.l | |-- virtio-pci.l#0 > > > > ethernet 0 [ + ] virtio-net | | `-- > > > > virtio-net#32 > > > > usb 0 [ + ] xhci_pci | `-- xhci_pci > > > > usb_hub 0 [ + ] usb_hub | `-- usb_hub > > > > usb_dev_ge 0 [ + ] usb_dev_generic_drv | |-- > > > > generic_bus_0_dev_2 > > > > usb_mass_s 0 [ + ] usb_mass_storage | |-- > > > > usb_mass_storage > > > > blk 0 [ ] usb_storage_blk | | `-- > > > > usb_mass_storage.lun0 > > > > usb_mass_s 1 [ + ] usb_mass_storage | `-- > > > > usb_mass_storage > > > > blk 1 [ ] usb_storage_blk | `-- > > > > usb_mass_storage.lun0 > > > > ... > > > > => efidebug devices > > > > Scanning disk usb_mass_storage.lun0... > > > > Scanning disk usb_mass_storage.lun0... > > > > Found 6 disks > > > > Missing RNG device for EFI_RNG_PROTOCOL > > > > ** Unable to read file ubootefi.var ** > > > > Failed to load EFI variables > > > > Unable to find TPMv2 device > > > > Device Device Path > > > > ================ ==================== > > > > 000000013eef3a50 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) > > > > 000000013eefe840 > > > > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USB(0xf3,0x0)/USB(0x0,0x0)/USB(0x2,0x0) > > > > 000000013eefe990 > > > > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USB(0xf3,0x0)/USB(0x0,0x0)/USB(0x2,0x0)/HD(1,0x01,0,0x0,0x99800) > > > > 000000013eefeae0 > > > > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USB(0xf3,0x0)/USB(0x0,0x0)/USB(0x2,0x0)/HD(2,0x01,0,0x99800,0x1800) > > > > 000000013eeff5f0 > > > > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USB(0xf3,0x0)/USB(0x0,0x0)/USB(0x3,0x0) > > > > 000000013eeff990 > > > > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USB(0xf3,0x0)/USB(0x0,0x0)/USB(0x3,0x0)/HD(1,GPT,ce86c5a7-b32a-488f-a346-88fe698e0edc,0x22,0x4c2a) > > > > 000000013eeffda0 > > > > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USB(0xf3,0x0)/USB(0x0,0x0)/USB(0x3,0x0)/HD(2,GPT,aa80aab9-33e6-42b6-b5db-def2cb8d7844,0x5000,0x1a800) > > > > > > I am missing a PCI() node here for the USB root Hub. In the DM tree: Is > > > xhci_pci the root hub and usb_hub a hub connected to this root hub? Or > > > do both nodes together model a root hub? Anyway USB(0xf3,0x0) seems to > > > be wrong. There is no USB hub with a port 243. > > > > As the code suggests, the path was generated this way: > > > > /VenHw(...)/ > > USB(0xf3,0x0)/ <= usb_controller (xhci_pci) > > USB(0x0,0x0)/ <= usb_hub > > USB(0x3,0x0)/ <= usb_mass_storage > > HD(1,GPT,...) > > > > USB(0xf3,0x0) was incorrectly encoded here, instead we will be able > > to generate PCI(xx,yy) by examining > > struct pci_child_plat *pplat = dev_get_parent_plat(dev); > > xx = PCI_DEV(pplat->devfn), yy = PCI_FUNC(pplat->devfn) > > if the dev's parent is a PCI (controller). > > > > But we will have to confirm that this is the only case that > > we need to take care of the parent's bus. > > Please note that any kind of pci component is not encoded > > in a device path under the current UEFI implementation on U-Boot. > > You'll need to take into account any PCI-PCI bridges, so you need to > do this in a loop, walking up the tree until you encounter the PCI > host bridge / root complex ("PciRoot"). Yes, you're right, but the current U-Boot's DM framework doesn't model the root complex. Instead, calling pci_get_controller() would be enough. (Need confirm this.) -> Simon? > The stadard also is of the opinion that PciRoot needs to be an ACPI > device path entry, which obviously doesn't make sense in a DT context. > Using VenHW makes more sense, but maybe UEFI needs to grow support for > proper DT device path entries... Yeah, the current VenHW(e61d73b9-a384-4acc-aeab-82e828f3628b) represents "efi_root" in our UEFI implementation and it is another incompatibility with other UEFI firmware. After all, we might want to insert a dummy PciRoot(0) (as EDK2 does?). > > > Please, see the examples on p. 303ff of the UEFI 2.9 specification: > > > > > > PciRoot(0)/PCI(31,2)/USB(0,0) > > > PciRoot(0)/PCI(31,2)/USB(1,0)/USB(3,0) > > > > > > The first example is a USB controller connected to port 0 of the root hub. > > > The second is a USB controller connected to port 3 of a USB hub which is > > > connected to port 1 of the root hub. > > > > So do you agree that this is a right approach even if it will break > > some sort of compatibility (of device path representation) with the current > > U-Boot UEFI implementation? > > This will break any Boot#### variables that have been set by users > isn't it? I guess there is no way to avoid that, but it probably is a > good idea to only break it once. Indeed. -Takahiro Akashi