Re: [PATCH v2] SourceLevelDebugPkg/DebugCommunicationLibUsb: Add endpoint configuration.

["Wu, Hao A" <hao.a.wu@intel.com>]

Hi Marvin,

One thing to confirm with you. For your using case, do you need to hard-code
the IN/OUT endpoints for EHCI debug device?

If not, I think we can just remove the hard-code endpoints settings in the
current code. And use the endpoints returned from the USB Device Descriptor.
By doing so, I think we can avoid adding those 2 PCDs.

Please let me know your thoughts on this. Thanks in advance.

Best Regards,
Hao Wu

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Marvin Häuser
> Sent: Thursday, June 14, 2018 3:58 AM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A
> Subject: [edk2] [PATCH v2] SourceLevelDebugPkg/DebugCommunicationLibUsb:
> Add endpoint configuration.
> 
> Currently, DebugCommunicationLibUsb uses the hardcoded endpoints 0x82
> and 0x01 to communicate with the EHCI Debug Device. These, however,
> are not standardized and may vary across different hardware.
> To solve this problem, two PCDs have been introduced to configure the
> in and out endpoints of the EHCI Debug Device. These may be set to 0
> to retrieve the endpoints from the USB Device Descriptor directly.
> To ensure maximum compatibility, the PCD defaults have been set to
> the former hardcoded values.
> 
> V2:
>   - Store endpoint data in the USB Debug Port handle sturcture.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com>
> ---
> 
> SourceLevelDebugPkg/Library/DebugCommunicationLibUsb/DebugCommunica
> tionLibUsb.c   | 32 ++++++++++++++++++--
> 
> SourceLevelDebugPkg/Library/DebugCommunicationLibUsb/DebugCommunica
> tionLibUsb.inf |  6 +++-
>  SourceLevelDebugPkg/SourceLevelDebugPkg.dec                                       | 12
> ++++++++
>  3 files changed, 47 insertions(+), 3 deletions(-)
> 
> diff --git
> a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb/DebugCommuni
> cationLibUsb.c
> b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb/DebugCommuni
> cationLibUsb.c
> index d996f80f59e3..a9a9ea07a39b 100644
> ---
> a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb/DebugCommuni
> cationLibUsb.c
> +++
> b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb/DebugCommuni
> cationLibUsb.c
> @@ -132,6 +132,14 @@ typedef struct _USB_DEBUG_PORT_HANDLE{
>    //
>    UINT32       EhciMemoryBase;
>    //
> +  // The usb debug device In endpoint.
> +  //
> +  UINT8        InEndpoint;
> +  //
> +  // The usb debug device Out endpoint.
> +  //
> +  UINT8        OutEndpoint;
> +  //
>    // The Bulk In endpoint toggle bit.
>    //
>    UINT8        BulkInToggle;
> @@ -603,6 +611,8 @@ InitializeUsbDebugHardware (
>    UINT32                    *UsbHCSParam;
>    UINT8                     DebugPortNumber;
>    UINT8                     Length;
> +  UINT8                     InEndpoint;
> +  UINT8                     OutEndpoint;
> 
>    UsbDebugPortRegister = (USB_DEBUG_PORT_REGISTER *)((UINTN)Handle-
> >UsbDebugPortMemoryBase + Handle->DebugPortOffset);
>    UsbHCSParam = (UINT32 *)((UINTN)Handle->EhciMemoryBase + 0x04);
> @@ -722,6 +732,24 @@ InitializeUsbDebugHardware (
>        return RETURN_DEVICE_ERROR;
>      }
> 
> +    //
> +    // Determine the usb debug device endpoints.
> +    //
> +    InEndpoint = PcdGet8 (PcdUsbDebugPortInEndpoint);
> +
> +    if (InEndpoint == 0) {
> +      InEndpoint = UsbDebugPortDescriptor.DebugInEndpoint;
> +    }
> +
> +    OutEndpoint = PcdGet8 (PcdUsbDebugPortOutEndpoint);
> +
> +    if (OutEndpoint == 0) {
> +      OutEndpoint = UsbDebugPortDescriptor.DebugOutEndpoint;
> +    }
> +
> +    Handle->InEndpoint  = InEndpoint;
> +    Handle->OutEndpoint = OutEndpoint;
> +
>      //
>      // enable the usb debug feature.
>      //
> @@ -879,7 +907,7 @@ DebugPortWriteBuffer (
>        Sent = (UINT8)(NumberOfBytes - Total);
>      }
> 
> -    Status = UsbDebugPortOut(UsbDebugPortRegister, Buffer + Total, Sent,
> OUTPUT_PID, 0x7F, 0x01, UsbDebugPortHandle->BulkOutToggle);
> +    Status = UsbDebugPortOut(UsbDebugPortRegister, Buffer + Total, Sent,
> OUTPUT_PID, 0x7F, UsbDebugPortHandle->OutEndpoint, UsbDebugPortHandle-
> >BulkOutToggle);
> 
>      if (RETURN_ERROR(Status)) {
>        return Total;
> @@ -959,7 +987,7 @@ DebugPortPollBuffer (
>      UsbDebugPortRegister->SendPid  = DATA1_PID;
>    }
>    UsbDebugPortRegister->UsbAddress  = 0x7F;
> -  UsbDebugPortRegister->UsbEndPoint = 0x82 & 0x0F;
> +  UsbDebugPortRegister->UsbEndPoint = UsbDebugPortHandle->InEndpoint &
> 0x0F;
> 
>    //
>    // Clearing W/R bit to indicate it's a READ operation
> diff --git
> a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb/DebugCommuni
> cationLibUsb.inf
> b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb/DebugCommuni
> cationLibUsb.inf
> index 028b04afbf00..eb55e5ee0f63 100644
> ---
> a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb/DebugCommuni
> cationLibUsb.inf
> +++
> b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb/DebugCommuni
> cationLibUsb.inf
> @@ -43,9 +43,13 @@ [Pcd]
>    # The pci address of ehci host controller, in which usb debug feature is
> enabled.
>    # The format of pci address please refer to SourceLevelDebugPkg.dec
>    gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdUsbEhciPciAddress              ##
> CONSUMES
> +  # The endpoint that should be used for read transactions from the usb debug
> device.
> +  gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdUsbDebugPortInEndpoint
> ## CONSUMES
> +  # The endpoint that should be used for write transactions to the usb debug
> device.
> +  gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdUsbDebugPortOutEndpoint
> ## CONSUMES
>    # The value of data buffer size used for USB debug port handle.
>    # It should be equal to sizeof (USB_DEBUG_PORT_HANDLE).
> -  gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugPortHandleBufferSize|23
> ## SOMETIMES_CONSUMES
> +
> gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugPortHandleBufferSize|25
> ## SOMETIMES_CONSUMES
> 
>  [LibraryClasses]
>    TimerLib
> diff --git a/SourceLevelDebugPkg/SourceLevelDebugPkg.dec
> b/SourceLevelDebugPkg/SourceLevelDebugPkg.dec
> index b89e9c6ad601..76410444f385 100644
> --- a/SourceLevelDebugPkg/SourceLevelDebugPkg.dec
> +++ b/SourceLevelDebugPkg/SourceLevelDebugPkg.dec
> @@ -72,6 +72,18 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
>    # @Expression  0x80000001 |
> (gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdUsbEhciPciAddress &
> 0xF0000FFF) == 0
> 
> gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdUsbEhciPciAddress|0x000EF000|
> UINT32|0x00000003
> 
> +  ## The endpoint that should be used for read transactions from the usb
> debug device.
> +  #  0:     Determine the endpoint dynamically.
> +  #  other: The endpoint that should be used.
> +  # @Prompt Configure the endpoint to read from the usb debug device.
> +
> gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdUsbDebugPortInEndpoint|0x82|U
> INT8|0x0000000b
> +
> +  ## The endpoint that should be used for write transactions to the usb debug
> device.
> +  #  0:     Determine the endpoint dynamically.
> +  #  other: The endpoint that should be used.
> +  # @Prompt Configure the endpoint to write to the usb debug device.
> +
> gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdUsbDebugPortOutEndpoint|0x01
> |UINT8|0x0000000c
> +
>    ## The mask of exception numbers whose handlers would be ignored and
> cannot be replaced or
>    #  hooked by Debug Agent Library. Masking INT1/INT3 is invalid.
>    # @Prompt Configure exception numbers not to be hooked by Debug Agent.
> --
> 2.17.1.windows.2
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel