> Date: Fri, 14 Jul 2023 23:30:43 +0200 > From: Marek Vasut <[email protected]>
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-asmedia.c 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 <[email protected]> > > --- > > 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. > > [...] >
