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

["Marvin Häuser" <Marvin.Haeuser@outlook.com>]

Hey Hao,

I don't require hardcoded endpoint information, but I have only one debug device to test with in the first place.
The static endpoint option was added in case there are Debug Devices around which report incorrect information.
I assumed that was part of the reason the endpoints were hardcoded in the first place.

Do you want me to submit a V2 without the PCDs?

Thanks,
Marvin.

> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Thursday, June 14, 2018 9:03 AM
> To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2-
> devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: RE: [PATCH v2] SourceLevelDebugPkg/DebugCommunicationLibUsb:
> Add endpoint configuration.
> 
> 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/DebugCommun
> ica
> > tionLibUsb.c   | 32 ++++++++++++++++++--
> >
> >
> SourceLevelDebugPkg/Library/DebugCommunicationLibUsb/DebugCommun
> ica
> > tionLibUsb.inf |  6 +++-
> >  SourceLevelDebugPkg/SourceLevelDebugPkg.dec                                       | 12
> > ++++++++
> >  3 files changed, 47 insertions(+), 3 deletions(-)
> >
> > diff --git
> >
> a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb/DebugComm
> uni
> > cationLibUsb.c
> >
> b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb/DebugComm
> uni
> > cationLibUsb.c
> > index d996f80f59e3..a9a9ea07a39b 100644
> > ---
> >
> a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb/DebugComm
> uni
> > cationLibUsb.c
> > +++
> >
> b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb/DebugComm
> uni
> > 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/DebugComm
> uni
> > cationLibUsb.inf
> >
> b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb/DebugComm
> uni
> > cationLibUsb.inf
> > index 028b04afbf00..eb55e5ee0f63 100644
> > ---
> >
> a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb/DebugComm
> uni
> > cationLibUsb.inf
> > +++
> >
> b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb/DebugComm
> uni
> > 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|0x000EF0
> 00|
> > 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|0x
> 82|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|0
> x01
> > |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