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

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

Hi Marvin,

Actually, the device we own has the IN/OUT endpoints with 0x82 and 0x01
respectively.

I double checked the value returned in the USB Debug Device Descriptor for the
device I own, and the values are correct, matching the hard-code configuration
in current code.

I think we can directly use the endpoints value from the descriptor, so please
help to work a series without the PCDs, thanks in advance.

Best Regards,
Hao Wu

> -----Original Message-----
> From: Marvin Häuser [mailto:Marvin.Haeuser@outlook.com]
> Sent: Thursday, June 14, 2018 11:46 PM
> To: edk2-devel@lists.01.org; Wu, Hao A
> Cc: Ni, Ruiyu
> Subject: RE: [PATCH v2] SourceLevelDebugPkg/DebugCommunicationLibUsb:
> Add endpoint configuration.
> 
> 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