Re: [edk2] [PATCH v1 1/1] EmbeddedPkg/Drivers: add DwUsbDxe
On Fri, 5 Oct 2018 at 23:46, Leif Lindholm wrote:
>
> On Tue, Aug 21, 2018 at 07:35:13PM +0800, Haojian Zhuang wrote:
> > Add Designware USB 2.0 device driver that is used on HiKey platform.
> >
> > Cc: Leif Lindholm
> > Cc: Ard Biesheuvel
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Haojian Zhuang
> > ---
> > EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.dec | 45 +
> > EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.inf | 52 ++
> > EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.h | 655 ++
> > EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.c | 912
> > 4 files changed, 1664 insertions(+)
>
> Could it be renamed DwUsb2? This seems to match how Synopsys
> themselves refer to it, and what the Linux driver is called.
>
OK
> Other than that, same comments as for DwUsb3Dxe - please move it to
> edk2-platforms and convert it to UEFI driver model with
> NonDiscoverableDeviceRegistrationLib.
UsbDevice isn't supported by NonDiscoverableDeviceRegistrationLib.
Could I add a new type?
>
> Hmm, it also looks to me like there are plenty of things here
> hardcoded for the use as a device for fastboot. I don't object to
> that being the only support submitted, you made it clear when you
> posted it. But the code is completely geared towards this, and I feel
> if someone comes along and want to add the functionality to run it in
> host mode.
I hope to support device first.
>
> I expect I will find the same when I look at the reworked version of
> DwUsb3Dxe.
>
> Is there anything you can do to break out the generic device
> configuration bits from the bits that assume there are two endpoints
> and they are being used for fastboot?
Let me investigate.
>
> >
> > diff --git a/EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.dec
> > b/EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.dec
> > new file mode 100644
> > index ..7eb65e498c04
> > --- /dev/null
> > +++ b/EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.dec
> > @@ -0,0 +1,45 @@
> > +#/** @file
> > +# Framework Module Development Environment Industry Standards
> > +#
> > +# This Package provides headers and libraries that conform to EFI/PI
> > Industry standards.
> > +# Copyright (c) 2007, Intel Corporation. All rights reserved.
> > +# Copyright (c) 2012-2014, ARM Ltd. All rights reserved.
> > +# Copyright (c) 2018, Linaro. All rights reserved.
>
> Same comments as for DwUsb3 - please merge these two into a common one
> for both drivers (if still needed).
>
> > +#
> > +#This program and the accompanying materials are licensed and made
> > available under
> > +#the terms and conditions of the BSD License which accompanies this
> > distribution.
> > +#The full text of the license may be found at
> > +#http://opensource.org/licenses/bsd-license.php
> > +#
> > +#THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> > +#WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
> > IMPLIED.
> > +#
> > +#**/
> > +
> > +[Defines]
> > + DEC_SPECIFICATION = 0x00010019
> > + PACKAGE_NAME = DwUsbDxePkg
> > + PACKAGE_GUID = 114a3be9-10f7-4bf1-81ca-09ac52d4c3d5
> > + PACKAGE_VERSION= 0.1
> > +
> > +
> > +
> > +#
> > +# Include Section - list of Include Paths that are provided by this
> > package.
> > +# Comments are used for Keywords and Module Types.
> > +#
> > +# Supported Module Types:
> > +# BASE SEC PEI_CORE PEIM DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER
> > DXE_SMM_DRIVER DXE_SAL_DRIVER UEFI_DRIVER UEFI_APPLICATION
> > +#
> > +
> > +
> > +[Guids.common]
> > + gDwUsbDxeTokenSpaceGuid = { 0x131c4d02, 0x9449, 0x4ee9, { 0xba,
> > 0x3d, 0x69, 0x50, 0x21, 0x89, 0x26, 0x0b }}
> > +
> > +[Protocols.common]
> > + gDwUsbProtocolGuid= { 0x109fa264, 0x7811, 0x4862, { 0xa9,
> > 0x73, 0x4a, 0xb2, 0xef, 0x2e, 0xe2, 0xff }}
> > +
> > +[PcdsFixedAtBuild.common]
> > + # DwUsb Driver PCDs
> > + gDwUsbDxeTokenSpaceGuid.PcdDwUsbDxeBaseAddress|0x0|UINT32|0x0001
> > + gDwUsbDxeTokenSpaceGuid.PcdSysCtrlBaseAddress|0x0|UINT32|0x0002
>
> I don't see PcdSysCtrlBaseAddress used anywhere in this patch? It also
> doesn't sound like something that should be an aspect of the USB
> controller driver.
>
Yes, PcdSysCtrlBaseAddress isn't used. We could remove it now.
> > diff --git a/EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.inf
> > b/EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.inf
> > new file mode 100644
> > index ..56d518c27c32
> > --- /dev/null
> > +++ b/EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.inf
> > @@ -0,0 +1,52 @@
> > +#/** @file
> > +#
> > +# Copyright (c) 2018, Linaro. All rights reserved.
> > +#
> > +# This program and the accompanying materials are licensed and made
> > available
> > +# under the terms and conditions of the BSD License
Re: [edk2] [PATCH v1 1/1] EmbeddedPkg/Drivers: add DwUsbDxe
On Tue, Aug 21, 2018 at 07:35:13PM +0800, Haojian Zhuang wrote:
> Add Designware USB 2.0 device driver that is used on HiKey platform.
>
> Cc: Leif Lindholm
> Cc: Ard Biesheuvel
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Haojian Zhuang
> ---
> EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.dec | 45 +
> EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.inf | 52 ++
> EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.h | 655 ++
> EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.c | 912
> 4 files changed, 1664 insertions(+)
Could it be renamed DwUsb2? This seems to match how Synopsys
themselves refer to it, and what the Linux driver is called.
Other than that, same comments as for DwUsb3Dxe - please move it to
edk2-platforms and convert it to UEFI driver model with
NonDiscoverableDeviceRegistrationLib.
Hmm, it also looks to me like there are plenty of things here
hardcoded for the use as a device for fastboot. I don't object to
that being the only support submitted, you made it clear when you
posted it. But the code is completely geared towards this, and I feel
if someone comes along and want to add the functionality to run it in
host mode.
I expect I will find the same when I look at the reworked version of DwUsb3Dxe.
Is there anything you can do to break out the generic device
configuration bits from the bits that assume there are two endpoints
and they are being used for fastboot?
>
> diff --git a/EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.dec
> b/EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.dec
> new file mode 100644
> index ..7eb65e498c04
> --- /dev/null
> +++ b/EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.dec
> @@ -0,0 +1,45 @@
> +#/** @file
> +# Framework Module Development Environment Industry Standards
> +#
> +# This Package provides headers and libraries that conform to EFI/PI
> Industry standards.
> +# Copyright (c) 2007, Intel Corporation. All rights reserved.
> +# Copyright (c) 2012-2014, ARM Ltd. All rights reserved.
> +# Copyright (c) 2018, Linaro. All rights reserved.
Same comments as for DwUsb3 - please merge these two into a common one
for both drivers (if still needed).
> +#
> +#This program and the accompanying materials are licensed and made
> available under
> +#the terms and conditions of the BSD License which accompanies this
> distribution.
> +#The full text of the license may be found at
> +#http://opensource.org/licenses/bsd-license.php
> +#
> +#THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +#WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
> IMPLIED.
> +#
> +#**/
> +
> +[Defines]
> + DEC_SPECIFICATION = 0x00010019
> + PACKAGE_NAME = DwUsbDxePkg
> + PACKAGE_GUID = 114a3be9-10f7-4bf1-81ca-09ac52d4c3d5
> + PACKAGE_VERSION= 0.1
> +
> +
> +
> +#
> +# Include Section - list of Include Paths that are provided by this package.
> +# Comments are used for Keywords and Module Types.
> +#
> +# Supported Module Types:
> +# BASE SEC PEI_CORE PEIM DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER
> DXE_SMM_DRIVER DXE_SAL_DRIVER UEFI_DRIVER UEFI_APPLICATION
> +#
> +
> +
> +[Guids.common]
> + gDwUsbDxeTokenSpaceGuid = { 0x131c4d02, 0x9449, 0x4ee9, { 0xba,
> 0x3d, 0x69, 0x50, 0x21, 0x89, 0x26, 0x0b }}
> +
> +[Protocols.common]
> + gDwUsbProtocolGuid= { 0x109fa264, 0x7811, 0x4862, { 0xa9,
> 0x73, 0x4a, 0xb2, 0xef, 0x2e, 0xe2, 0xff }}
> +
> +[PcdsFixedAtBuild.common]
> + # DwUsb Driver PCDs
> + gDwUsbDxeTokenSpaceGuid.PcdDwUsbDxeBaseAddress|0x0|UINT32|0x0001
> + gDwUsbDxeTokenSpaceGuid.PcdSysCtrlBaseAddress|0x0|UINT32|0x0002
I don't see PcdSysCtrlBaseAddress used anywhere in this patch? It also
doesn't sound like something that should be an aspect of the USB
controller driver.
> diff --git a/EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.inf
> b/EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.inf
> new file mode 100644
> index ..56d518c27c32
> --- /dev/null
> +++ b/EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.inf
> @@ -0,0 +1,52 @@
> +#/** @file
> +#
> +# Copyright (c) 2018, Linaro. All rights reserved.
> +#
> +# This program and the accompanying materials are licensed and made
> available
> +# under the terms and conditions of the BSD License which accompanies this
> +# distribution. The full text of the license may be found at
> +# http://opensource.org/licenses/bsd-license.php
> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
> IMPLIED.
> +#
> +#
> +#**/
> +
> +[Defines]
> + INF_VERSION= 0x00010019
> + BASE_NAME = DwUsbDxe
> + FILE_GUID = 72d78ea6-4dee-11e3-8100-f
