Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33377 )
Change subject: mb/google/octopus: Add API to disable USB devices ......................................................................
Patch Set 2:
(10 comments)
https://review.coreboot.org/c/coreboot/+/33377/2/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/xhci.h:
https://review.coreboot.org/c/coreboot/+/33377/2/src/soc/intel/common/block/... PS2, Line 60: unsued unused
https://review.coreboot.org/c/coreboot/+/33377/2/src/soc/intel/common/block/... PS2, Line 64: * the device tree. It would be good to highlight that: 1. This function relies on UPC_TYPE_INTERNAL in devicetree to decide if a port is internal. 2. If the internal USB device power can be enabled/disabled, it is caller's responsibility to ensure that the device is powered up before calling this function. I am slightly concerned if in the future we decide that the power enabling for certain internal devices needs to happen at OS level, then we might unnecessarily disable the device here. For now, we can just have a note here to make it clear.
https://review.coreboot.org/c/coreboot/+/33377/2/src/soc/intel/common/block/... PS2, Line 69: unsued unused
https://review.coreboot.org/c/coreboot/+/33377/2/src/soc/intel/common/block/... PS2, Line 72: * in the device tree but are not used in certain situations like SKUs. Again, it is important to highlight the difference here i.e. it directly disables the device provided by the caller without checking for its connect status. So, it is caller's responsibility to determine if a device is really present (e.g. SKUs that you mentioned).
https://review.coreboot.org/c/coreboot/+/33377/2/src/soc/intel/common/block/... PS2, Line 74: port_id How do you determine port type USB2 or USB3 from this?
https://review.coreboot.org/c/coreboot/+/33377/2/src/soc/intel/common/block/... PS2, Line 75: Do we need to have one API usb_xhci_disable_unused() which:
Checks if port is internal/external --> If internal: Check connect status and disable if not connected --> If external: Callback to mainboard to check if device is present and disable if not.
With that you don't have to make individual calls for each external device. Thoughts?
https://review.coreboot.org/c/coreboot/+/33377/2/src/soc/intel/common/block/... File src/soc/intel/common/block/xhci/xhci.c:
https://review.coreboot.org/c/coreboot/+/33377/2/src/soc/intel/common/block/... PS2, Line 52: res Check for res not being NULL?
https://review.coreboot.org/c/coreboot/+/33377/2/src/soc/intel/common/block/... PS2, Line 75: if (!xhci) : return; Print an error in this case since it is not expected?
https://review.coreboot.org/c/coreboot/+/33377/2/src/soc/intel/common/block/... PS2, Line 78: Use space instead of tab?
https://review.coreboot.org/c/coreboot/+/33377/2/src/soc/intel/common/block/... PS2, Line 98: port_id I don't think it is correct to assume that port_id would apply to both USB2 and USB3 ports. Caller will have to pass in both information.
-
Furquan Shaikh (Code Review)