On 10 February 2017 at 13:32, Leif Lindholm leif.lindholm@linaro.org wrote:
On Fri, Feb 10, 2017 at 11:45:20AM +0000, Ard Biesheuvel wrote:
On 10 Feb 2017, at 11:37, Leif Lindholm leif.lindholm@linaro.org wrote:
On Thu, Feb 09, 2017 at 10:11:48PM +0800, Haojian Zhuang wrote: Support Designware USB device controller on HiKey platform.
Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Haojian Zhuang haojian.zhuang@linaro.org
.../Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c | 266 +++++++++++++++++++++ .../Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.inf | 46 ++++ 2 files changed, 312 insertions(+) create mode 100644 Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c create mode 100644 Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.inf
diff --git a/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c b/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c new file mode 100644 index 0000000..60ad4d6 --- /dev/null +++ b/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c @@ -0,0 +1,266 @@ +/** @file +* +* Copyright (c) 2015-2017, 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. +* +**/
+#include <Library/BaseMemoryLib.h> +#include <Library/DebugLib.h> +#include <Library/IoLib.h> +#include <Library/TimerLib.h> +#include <Library/UefiBootServicesTableLib.h> +#include <Library/UefiLib.h> +#include <Library/UefiRuntimeServicesTableLib.h>
+#include <Protocol/EmbeddedGpio.h> +#include <Protocol/DwUsb.h>
+#include <Hi6220.h>
+#define USB_SEL_GPIO0_3 3 // GPIO 0_3 +#define USB_5V_HUB_EN 7 // GPIO 0_7 +#define USB_ID_DET_GPIO2_5 21 // GPIO 2_5 +#define USB_VBUS_DET_GPIO2_6 22 // GPIO 2_6
+// Jumper on pin5-6 of J15 determines whether boot to fastboot +#define DETECT_J15_FASTBOOT 24 // GPIO 3_0
+STATIC EMBEDDED_GPIO *mGpio;
+STATIC +VOID +HiKeyDetectUsbModeInit (
- IN VOID
- )
+{
- EFI_STATUS Status;
- /* set pullup on both GPIO2_5 & GPIO2_6. It's required for inupt. */
- MmioWrite32 (0xf8001864, 1);
- MmioWrite32 (0xf8001868, 1);
No magic values please, add #defines.
- Status = gBS->LocateProtocol (&gEmbeddedGpioProtocolGuid, NULL, (VOID **)&mGpio);
- ASSERT_EFI_ERROR (Status);
This should unconditionally print an error and return if failed. Also, it should probably return error and be handled at the call site, instead of continuing as if nothing had happened.
If this driver has a depex on this protocol, it is reasonable (and idiomatic in edk2) to use this pattern imo
Right, I didn't notice that aspect. On the basis that this is an "impossible" error?
Yes, but only if you declare the dependency in the [Depex], which this driver does not do.
The DXE core will only dispatch your driver if the protocol is installed, so your firmware is *badly* broken if you end up here and the protocol is not here.
I still don't feel great about _not_ returning before dereferencing a NULL pointer, and I see both ways of dealing with this in edk2.
But I think if the code stays as it is, there should at least be a comment added - I shouldn't need to refer to other files in order to determine whether a condition is impossible or not.
I agree that it makes sense to annotate a LocateProtocol() call as involving a depex'ed upon protocol.