Re: [PATCH 3/3] usb: xhci-pci: Load ASMedia XHCI controller firmware

15 Jul 2023

...
Date: Fri, 14 Jul 2023 23:30:43 +0200
From: Marek Vasut marex@denx.de
Hi Marek,
Your suggestions are reasonable, but would make the driver deviate
further from its Linux equivalent:
https://github.com/AsahiLinux/linux/blob/asahi/drivers/usb/host/xhci-pci-asm...
Right now I can actually easily diff the two drivers to pick up
improvements.  Changing things the way you suggest would make that
harder.
Cheers,
Mark
...
On 7/14/23 22:43, Mark Kettenis wrote:
...
The ASMedia XHCI controller found on some of the Apple Silicon
machines needs firmware to operate.  Use the file system
firmware loader interface to read the firmware and load it
onto the controller.  This allows keyboards connected to the
type-A ports on these machines to function in U-Boot.
Signed-off-by: Mark Kettenis kettenis@openbsd.org
MAINTAINERS                         |   1 +
  drivers/usb/host/Kconfig            |  11 +
  drivers/usb/host/Makefile           |   1 +
  drivers/usb/host/xhci-pci-asmedia.c | 394 ++++++++++++++++++++++++++++
  drivers/usb/host/xhci-pci.c         |   9 +
  include/usb/xhci.h                  |   3 +
  6 files changed, 419 insertions(+)
  create mode 100644 drivers/usb/host/xhci-pci-asmedia.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 2477923a52..0acf04fff8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -124,6 +124,7 @@ F:	drivers/iommu/apple_dart.c
  F:	drivers/nvme/nvme_apple.c
  F:	drivers/pci/pcie_apple.c
  F:	drivers/pinctrl/pinctrl-apple.c
+F:	drivers/usb/host/xhci-pci-asmedia.c
  F:	drivers/watchdog/apple_wdt.c
  F:	include/configs/apple.h
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 1a883babf4..ad8dfabeeb 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -95,6 +95,17 @@ config USB_XHCI_PCI
   help
     Enables support for the PCI-based xHCI controller.
+config USB_XHCI_PCI_ASMEDIA

bool "Support for ASMedia xHCI USB controller"
depends on USB_XHCI_PCI
default y if ARCH_APPLE
imply FS_LOADER
help
 Enables support for loading firmware for the ASMedia xHCI


 controller.  This is needed on Apple Silicon machines that


 contain ASMedia xHCI controllers without the full USB


 firmware.


config USB_XHCI_RCAR
 bool "Renesas RCar USB 3.0 support"
 default y

diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index 8dad36f936..2aa2d80263 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -52,6 +52,7 @@ obj-$(CONFIG_USB_XHCI_MTK) += xhci-mtk.o
  obj-$(CONFIG_USB_XHCI_MVEBU) += xhci-mvebu.o
  obj-$(CONFIG_USB_XHCI_OMAP) += xhci-omap.o
  obj-$(CONFIG_USB_XHCI_PCI) += xhci-pci.o
+obj-$(CONFIG_USB_XHCI_PCI_ASMEDIA) += xhci-pci-asmedia.o
  obj-$(CONFIG_USB_XHCI_RCAR) += xhci-rcar.o
  obj-$(CONFIG_USB_XHCI_STI) += dwc3-sti-glue.o
  obj-$(CONFIG_USB_XHCI_OCTEON) += dwc3-octeon-glue.o
diff --git a/drivers/usb/host/xhci-pci-asmedia.c b/drivers/usb/host/xhci-pci-asmedia.c
new file mode 100644
index 0000000000..cc1d25bde2
--- /dev/null
+++ b/drivers/usb/host/xhci-pci-asmedia.c
@@ -0,0 +1,394 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+/*


ASMedia xHCI firmware loader



Copyright (C) The Asahi Linux Contributors


*/


+#include <common.h>
+#include <dm.h>
+#include <dm/device_compat.h>
+#include <fs_loader.h>
+#include <linux/iopoll.h>
+#include <pci.h>
+#include <usb.h>
+#include <usb/xhci.h>



+/* Configuration space registers */
+#define ASMT_CFG_CONTROL		0xe0
+#define ASMT_CFG_CONTROL_WRITE		BIT(1)
+#define ASMT_CFG_CONTROL_READ		BIT(0)



+#define ASMT_CFG_SRAM_ADDR		0xe2



+#define ASMT_CFG_SRAM_ACCESS		0xef
+#define ASMT_CFG_SRAM_ACCESS_READ	BIT(6)
+#define ASMT_CFG_SRAM_ACCESS_ENABLE	BIT(7)



+#define ASMT_CFG_DATA_READ0		0xf0
+#define ASMT_CFG_DATA_READ1		0xf4



+#define ASMT_CFG_DATA_WRITE0		0xf8
+#define ASMT_CFG_DATA_WRITE1		0xfc



+#define ASMT_CMD_GET_FWVER		0x8000060840
+#define ASMT_FWVER_ROM			0x010250090816



+/* BAR0 registers */
+#define ASMT_REG_ADDR			0x3000



+#define ASMT_REG_WDATA			0x3004
+#define ASMT_REG_RDATA			0x3008



+#define ASMT_REG_STATUS			0x3009
+#define ASMT_REG_STATUS_BUSY		BIT(7)



+#define ASMT_REG_CODE_WDATA		0x3010
+#define ASMT_REG_CODE_RDATA		0x3018



+#define ASMT_MMIO_CPU_MISC		0x500e
+#define ASMT_MMIO_CPU_MISC_CODE_RAM_WR	BIT(0)



+#define ASMT_MMIO_CPU_MODE_NEXT		0x5040
+#define ASMT_MMIO_CPU_MODE_CUR		0x5041



+#define ASMT_MMIO_CPU_MODE_RAM		BIT(0)
+#define ASMT_MMIO_CPU_MODE_HALFSPEED	BIT(1)



+#define ASMT_MMIO_CPU_EXEC_CTRL		0x5042
+#define ASMT_MMIO_CPU_EXEC_CTRL_RESET	BIT(0)
+#define ASMT_MMIO_CPU_EXEC_CTRL_HALT	BIT(1)



+#define TIMEOUT_USEC			10000
+#define RESET_TIMEOUT_USEC		500000



+static int asmedia_mbox_tx(struct udevice *pdev, u64 data)
+{

u8 op;
int i;

for (i = 0; i < TIMEOUT_USEC; i++) {
dm_pci_read_config8(pdev, ASMT_CFG_CONTROL, &op);


if (!(op & ASMT_CFG_CONTROL_WRITE))


	break;


udelay(1);



Would it be possible to use the wait_for_bit*() here ?
Or read*poll*timeout() ?
...

}

if (op & ASMT_CFG_CONTROL_WRITE) {
dev_err(pdev,


	"Timed out on mailbox tx: 0x%llx\n",


	data);


return -ETIMEDOUT;


}

dm_pci_write_config32(pdev, ASMT_CFG_DATA_WRITE0, data);
dm_pci_write_config32(pdev, ASMT_CFG_DATA_WRITE1, data >> 32);
dm_pci_write_config8(pdev, ASMT_CFG_CONTROL,
	     ASMT_CFG_CONTROL_WRITE);



[...]
...
+static u8 asmedia_read_reg(struct udevice *pdev, struct xhci_hccr *hccr,

	   u16 addr)



+{

void __iomem *regs = (void *)hccr;
u8 status;
int ret;

ret = readb_poll_sleep_timeout(regs + ASMT_REG_STATUS, status,
		       !(status & ASMT_REG_STATUS_BUSY),


		       1000, TIMEOUT_USEC);



if (ret) {
dev_err(pdev,


	"Read reg wait timed out ([%04x])\n", addr);


return ~0;



It might make sense to return the value as parameter, and make return 
value of this function into integer, so error codes can be returned that 
way.
[...]

    