[PATCH 0/5] usb: xhci: Add support for Renesas USB controllers

[PATCH 0/5] usb: xhci: Add support for Renesas USB controllers

[Vinod Koul]

This series add support for Renesas USB controllers uPD720201 and uPD720202.
These require firmware to be loaded and in case devices have ROM those can
also be programmed if empty. If ROM is porgrammed, it runs from ROM as well.

This includes two patches from Christian which supported these controllers
w/o ROM and later my patches for ROM support and multiple firmware versions.

Christian Lamparter (2):
  usb: xhci: add firmware loader for uPD720201 and uPD720202 w/o ROM
  usb: xhci: handle uPD720201 and uPD720202 w/o ROM

Vinod Koul (3):
  usb: xhci: Use register defined and field names
  usb: xhci: Add ROM loader for uPD720201
  usb: xhci: allow multiple firmware versions

 drivers/usb/host/xhci-pci.c | 962 ++++++++++++++++++++++++++++++++++++
 1 file changed, 962 insertions(+)

-- 
2.20.1

[PATCH 1/5] usb: xhci: add firmware loader for uPD720201 and uPD720202 w/o ROM

[Vinod Koul]

From: Christian Lamparter <chunkeey@googlemail.com>

This patch adds a firmware loader for the uPD720201K8-711-BAC-A
and uPD720202K8-711-BAA-A variant. Both of these chips are listed
in Renesas' R19UH0078EJ0500 Rev.5.00 "User's Manual: Hardware" as
devices which need the firmware loader on page 2 in order to
work as they "do not support the External ROM".

The "Firmware Download Sequence" is describe in chapter
"7.1 FW Download Interface" R19UH0078EJ0500 Rev.5.00 page 131.

The firmware "K2013080.mem" is available from a USB3.0 Host to
PCIe Adapter (PP2U-E card) "Firmware download" archive. An
alternative version can be sourced from Netgear's WNDR4700 GPL
archives.

The release notes of the PP2U-E's "Firmware Download" ver 2.0.1.3
(2012-06-15) state that the firmware is for the following devices:
 - uPD720201 ES 2.0 sample whose revision ID is 2.
 - uPD720201 ES 2.1 sample & CS sample & Mass product, ID is 3.
 - uPD720202 ES 2.0 sample & CS sample & Mass product, ID is 2.

If someone from Renesas is listening: It would be great, if these
firmwares could be added to linux-firmware.git.

Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 drivers/usb/host/xhci-pci.c | 480 ++++++++++++++++++++++++++++++++++++
 1 file changed, 480 insertions(+)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index c2fe218e051f..a400cf7b39da 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -12,6 +12,8 @@
 #include <linux/slab.h>
 #include <linux/module.h>
 #include <linux/acpi.h>
+#include <linux/firmware.h>
+#include <asm/unaligned.h>
 
 #include "xhci.h"
 #include "xhci-trace.h"
@@ -279,6 +281,458 @@ static void xhci_pme_acpi_rtd3_enable(struct pci_dev *dev)
 static void xhci_pme_acpi_rtd3_enable(struct pci_dev *dev) { }
 #endif /* CONFIG_ACPI */
 
+static const struct renesas_fw_entry {
+	const char *firmware_name;
+	u16 device;
+	u8 revision;
+	u16 expected_version;
+} renesas_fw_table[] = {
+	/*
+	 * Only the uPD720201K8-711-BAC-A or uPD720202K8-711-BAA-A
+	 * are listed in R19UH0078EJ0500 Rev.5.00 as devices which
+	 * need the software loader.
+	 *
+	 * PP2U/ReleaseNote_USB3-201-202-FW.txt:
+	 *
+	 * Note: This firmware is for the following devices.
+	 *  - uPD720201 ES 2.0 sample whose revision ID is 2.
+	 *  - uPD720201 ES 2.1 sample & CS sample & Mass product, ID is 3.
+	 *  - uPD720202 ES 2.0 sample & CS sample & Mass product, ID is 2.
+	 */
+	{ "K2013080.mem", 0x0014, 0x02, 0x2013 },
+	{ "K2013080.mem", 0x0014, 0x03, 0x2013 },
+	{ "K2013080.mem", 0x0015, 0x02, 0x2013 },
+};
+
+static const struct renesas_fw_entry *renesas_needs_fw_dl(struct pci_dev *dev)
+{
+	const struct renesas_fw_entry *entry;
+	size_t i;
+
+	/* This loader will only work with a RENESAS device. */
+	if (!(dev->vendor == PCI_VENDOR_ID_RENESAS))
+		return NULL;
+
+	for (i = 0; i < ARRAY_SIZE(renesas_fw_table); i++) {
+		entry = &renesas_fw_table[i];
+		if (entry->device == dev->device &&
+		    entry->revision == dev->revision)
+			return entry;
+	}
+
+	return NULL;
+}
+
+static int renesas_fw_download_image(struct pci_dev *dev,
+				     const u32 *fw,
+				     size_t step)
+{
+	size_t i;
+	int err;
+	u8 fw_status;
+	bool data0_or_data1;
+
+	/*
+	 * The hardware does alternate between two 32-bit pages.
+	 * (This is because each row of the firmware is 8 bytes).
+	 *
+	 * for even steps we use DATA0, for odd steps DATA1.
+	 */
+	data0_or_data1 = (step & 1) == 1;
+
+	/* step+1. Read "Set DATAX" and confirm it is cleared. */
+	for (i = 0; i < 10000; i++) {
+		err = pci_read_config_byte(dev, 0xF5, &fw_status);
+		if (err)
+			return pcibios_err_to_errno(err);
+		if (!(fw_status & BIT(data0_or_data1)))
+			break;
+
+		udelay(1);
+	}
+	if (i == 10000)
+		return -ETIMEDOUT;
+
+	/*
+	 * step+2. Write FW data to "DATAX".
+	 * "LSB is left" => force little endian
+	 */
+	err = pci_write_config_dword(dev, data0_or_data1 ? 0xFC : 0xF8,
+				     (__force u32) cpu_to_le32(fw[step]));
+	if (err)
+		return pcibios_err_to_errno(err);
+
+	udelay(100);
+
+	/* step+3. Set "Set DATAX". */
+	err = pci_write_config_byte(dev, 0xF5, BIT(data0_or_data1));
+	if (err)
+		return pcibios_err_to_errno(err);
+
+	return 0;
+}
+
+static int renesas_fw_verify(struct pci_dev *dev,
+			     const void *fw_data,
+			     size_t length)
+{
+	const struct renesas_fw_entry *entry = renesas_needs_fw_dl(dev);
+	u16 fw_version_pointer;
+	u16 fw_version;
+
+	if (!entry)
+		return -EINVAL;
+
+	/*
+	 * The Firmware's Data Format is describe in
+	 * "6.3 Data Format" R19UH0078EJ0500 Rev.5.00 page 124
+	 */
+
+	/* "Each row is 8 bytes". => firmware size must be a multiple of 8. */
+	if (length % 8 != 0) {
+		dev_err(&dev->dev, "firmware size is not a multipe of 8.");
+		return -EINVAL;
+	}
+
+	/*
+	 * The bootrom chips of the big brother have sizes up to 64k, let's
+	 * assume that's the biggest the firmware can get.
+	 */
+	if (length < 0x1000 || length >= 0x10000) {
+		dev_err(&dev->dev, "firmware is size %zd is not (4k - 64k).",
+			length);
+		return -EINVAL;
+	}
+
+	/* The First 2 bytes are fixed value (55aa). "LSB on Left" */
+	if (get_unaligned_le16(fw_data) != 0x55aa) {
+		dev_err(&dev->dev, "no valid firmware header found.");
+		return -EINVAL;
+	}
+
+	/* verify the firmware version position and print it. */
+	fw_version_pointer = get_unaligned_le16(fw_data + 4);
+	if (fw_version_pointer + 2 >= length) {
+		dev_err(&dev->dev, "firmware version pointer is outside of the firmware image.");
+		return -EINVAL;
+	}
+
+	fw_version = get_unaligned_le16(fw_data + fw_version_pointer);
+	dev_dbg(&dev->dev, "got firmware version: %02x.", fw_version);
+
+	if (fw_version != entry->expected_version) {
+		dev_err(&dev->dev, "firmware version mismatch, expected version: %02x.",
+			 entry->expected_version);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int renesas_fw_check_running(struct pci_dev *pdev)
+{
+	int err;
+	u8 fw_state;
+
+	/*
+	 * Test if the device is actually needing the firmware. As most
+	 * BIOSes will initialize the device for us. If the device is
+	 * initialized.
+	 */
+	err = pci_read_config_byte(pdev, 0xF4, &fw_state);
+	if (err)
+		return pcibios_err_to_errno(err);
+
+	/*
+	 * Check if "FW Download Lock" is locked. If it is and the FW is
+	 * ready we can simply continue. If the FW is not ready, we have
+	 * to give up.
+	 */
+	if (fw_state & BIT(1)) {
+		dev_dbg(&pdev->dev, "FW Download Lock is engaged.");
+
+		if (fw_state & BIT(4))
+			return 0;
+
+		dev_err(&pdev->dev, "FW Download Lock is set and FW is not ready. Giving Up.");
+		return -EIO;
+	}
+
+	/*
+	 * Check if "FW Download Enable" is set. If someone (us?) tampered
+	 * with it and it can't be resetted, we have to give up too... and
+	 * ask for a forgiveness and a reboot.
+	 */
+	if (fw_state & BIT(0)) {
+		dev_err(&pdev->dev, "FW Download Enable is stale. Giving Up (poweroff/reboot needed).");
+		return -EIO;
+	}
+
+	/* Otherwise, Check the "Result Code" Bits (6:4) and act accordingly */
+	switch ((fw_state & 0x70)) {
+	case 0: /* No result yet */
+		dev_dbg(&pdev->dev, "FW is not ready/loaded yet.");
+
+		/* tell the caller, that this device needs the firmware. */
+		return 1;
+
+	case BIT(4): /* Success, device should be working. */
+		dev_dbg(&pdev->dev, "FW is ready.");
+		return 0;
+
+	case BIT(5): /* Error State */
+		dev_err(&pdev->dev, "hardware is in an error state. Giving up (poweroff/reboot needed).");
+		return -ENODEV;
+
+	default: /* All other states are marked as "Reserved states" */
+		dev_err(&pdev->dev, "hardware is in an invalid state %x. Giving up (poweroff/reboot needed).",
+			(fw_state & 0x70) >> 4);
+		return -EINVAL;
+	}
+}
+
+static int renesas_hw_check_run_stop_busy(struct pci_dev *pdev)
+{
+#if 0
+	u32 val;
+
+	/*
+	 * 7.1.3 Note 3: "... must not set 'FW Download Enable' when
+	 * 'RUN/STOP' of USBCMD Register is set"
+	 */
+	val = readl(hcd->regs + 0x20);
+	if (val & BIT(0)) {
+		dev_err(&pdev->dev, "hardware is busy and can't receive a FW.");
+		return -EBUSY;
+	}
+#endif
+	return 0;
+}
+
+static int renesas_fw_download(struct pci_dev *pdev,
+	const struct firmware *fw, unsigned int retry_counter)
+{
+	const u32 *fw_data = (const u32 *) fw->data;
+	size_t i;
+	int err;
+	u8 fw_status;
+
+	/*
+	 * For more information and the big picture: please look at the
+	 * "Firmware Download Sequence" in "7.1 FW Download Interface"
+	 * of R19UH0078EJ0500 Rev.5.00 page 131
+	 */
+	err = renesas_hw_check_run_stop_busy(pdev);
+	if (err)
+		return err;
+
+	/*
+	 * 0. Set "FW Download Enable" bit in the
+	 * "FW Download Control & Status Register" at 0xF4
+	 */
+	err = pci_write_config_byte(pdev, 0xF4, BIT(0));
+	if (err)
+		return pcibios_err_to_errno(err);
+
+	/* 1 - 10 follow one step after the other. */
+	for (i = 0; i < fw->size / 4; i++) {
+		err = renesas_fw_download_image(pdev, fw_data, i);
+		if (err) {
+			dev_err(&pdev->dev, "Firmware Download Step %zd failed at position %zd bytes with (%d).",
+				 i, i * 4, err);
+			return err;
+		}
+	}
+
+	/*
+	 * This sequence continues until the last data is written to
+	 * "DATA0" or "DATA1". Naturally, we wait until "SET DATA0/1"
+	 * is cleared by the hardware beforehand.
+	 */
+	for (i = 0; i < 10000; i++) {
+		err = pci_read_config_byte(pdev, 0xF5, &fw_status);
+		if (err)
+			return pcibios_err_to_errno(err);
+		if (!(fw_status & (BIT(0) | BIT(1))))
+			break;
+
+		udelay(1);
+	}
+	if (i == 10000)
+		dev_warn(&pdev->dev, "Final Firmware Download step timed out.");
+
+	/*
+	 * 11. After finishing writing the last data of FW, the
+	 * System Software must clear "FW Download Enable"
+	 */
+	err = pci_write_config_byte(pdev, 0xF4, 0);
+	if (err)
+		return pcibios_err_to_errno(err);
+
+	/* 12. Read "Result Code" and confirm it is good. */
+	for (i = 0; i < 10000; i++) {
+		err = pci_read_config_byte(pdev, 0xF4, &fw_status);
+		if (err)
+			return pcibios_err_to_errno(err);
+		if (fw_status & BIT(4))
+			break;
+
+		udelay(1);
+	}
+	if (i == 10000) {
+		/* Timed out / Error - let's see if we can fix this */
+		err = renesas_fw_check_running(pdev);
+		switch (err) {
+		case 0: /*
+			 * we shouldn't end up here.
+			 * maybe it took a little bit longer.
+			 * But all should be well?
+			 */
+			break;
+
+		case 1: /* (No result yet? - we can try to retry) */
+			if (retry_counter < 10) {
+				retry_counter++;
+				dev_warn(&pdev->dev, "Retry Firmware download: %d try.",
+					  retry_counter);
+				return renesas_fw_download(pdev, fw,
+							   retry_counter);
+			}
+			return -ETIMEDOUT;
+
+		default:
+			return err;
+		}
+	}
+	/*
+	 * Optional last step: Engage Firmware Lock
+	 *
+	 * err = pci_write_config_byte(pdev, 0xF4, BIT(2));
+	 * if (err)
+	 *	return pcibios_err_to_errno(err);
+	 */
+
+	return 0;
+}
+
+struct renesas_fw_ctx {
+	struct pci_dev *pdev;
+	const struct pci_device_id *id;
+	bool resume;
+};
+
+static int xhci_pci_probe(struct pci_dev *pdev,
+			  const struct pci_device_id *id);
+
+static void renesas_fw_callback(const struct firmware *fw,
+				void *context)
+{
+	struct renesas_fw_ctx *ctx = context;
+	struct pci_dev *pdev = ctx->pdev;
+	struct device *parent = pdev->dev.parent;
+	int err = -ENOENT;
+
+	if (fw) {
+		err = renesas_fw_verify(pdev, fw->data, fw->size);
+		if (!err) {
+			err = renesas_fw_download(pdev, fw, 0);
+			release_firmware(fw);
+			if (!err) {
+				if (ctx->resume)
+					return;
+
+				err = xhci_pci_probe(pdev, ctx->id);
+				if (!err) {
+					/* everything worked */
+					devm_kfree(&pdev->dev, ctx);
+					return;
+				}
+
+				/* in case of an error - fall through */
+			} else {
+				dev_err(&pdev->dev, "firmware failed to download (%d).",
+					err);
+			}
+		}
+	} else {
+		dev_err(&pdev->dev, "firmware failed to load (%d).", err);
+	}
+
+	dev_info(&pdev->dev, "Unloading driver");
+
+	if (parent)
+		device_lock(parent);
+
+	device_release_driver(&pdev->dev);
+
+	if (parent)
+		device_unlock(parent);
+
+	pci_dev_put(pdev);
+}
+
+static int renesas_fw_alive_check(struct pci_dev *pdev)
+{
+	const struct renesas_fw_entry *entry;
+	int err;
+
+	/* check if we have a eligible RENESAS' uPD720201/2 w/o FW. */
+	entry = renesas_needs_fw_dl(pdev);
+	if (!entry)
+		return 0;
+
+	err = renesas_fw_check_running(pdev);
+	/* Also go ahead, if the firmware is running */
+	if (err == 0)
+		return 0;
+
+	/* At this point, we can be sure that the FW isn't ready. */
+	return err;
+}
+
+static int renesas_fw_download_to_hw(struct pci_dev *pdev,
+				     const struct pci_device_id *id,
+				     bool do_resume)
+{
+	const struct renesas_fw_entry *entry;
+	struct renesas_fw_ctx *ctx;
+	int err;
+
+	/* check if we have a eligible RENESAS' uPD720201/2 w/o FW. */
+	entry = renesas_needs_fw_dl(pdev);
+	if (!entry)
+		return 0;
+
+	err = renesas_fw_check_running(pdev);
+	/* Continue ahead, if the firmware is already running. */
+	if (err == 0)
+		return 0;
+
+	if (err != 1)
+		return err;
+
+	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+	ctx->pdev = pdev;
+	ctx->resume = do_resume;
+	ctx->id = id;
+
+	pci_dev_get(pdev);
+	err = request_firmware_nowait(THIS_MODULE, 1, entry->firmware_name,
+		&pdev->dev, GFP_KERNEL, ctx, renesas_fw_callback);
+	if (err) {
+		pci_dev_put(pdev);
+		return err;
+	}
+
+	/*
+	 * The renesas_fw_callback() callback will continue the probe
+	 * process, once it aquires the firmware.
+	 */
+	return 1;
+}
+
 /* called during probe() after chip reset completes */
 static int xhci_pci_setup(struct usb_hcd *hcd)
 {
@@ -317,6 +771,22 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	struct hc_driver *driver;
 	struct usb_hcd *hcd;
 
+	/*
+	 * Check if this device is a RENESAS uPD720201/2 device.
+	 * Otherwise, we can continue with xhci_pci_probe as usual.
+	 */
+	retval = renesas_fw_download_to_hw(dev, id, false);
+	switch (retval) {
+	case 0:
+		break;
+
+	case 1: /* let it load the firmware and recontinue the probe. */
+		return 0;
+
+	default:
+		return retval;
+	};
+
 	driver = (struct hc_driver *)id->driver_data;
 
 	/* Prevent runtime suspending between USB-2 and USB-3 initialization */
@@ -381,6 +851,16 @@ static void xhci_pci_remove(struct pci_dev *dev)
 {
 	struct xhci_hcd *xhci;
 
+	if (renesas_fw_alive_check(dev)) {
+		/*
+		 * bail out early, if this was a renesas device w/o FW.
+		 * Else we might hit the NMI watchdog in xhci_handsake
+		 * during xhci_reset as part of the driver's unloading.
+		 * which we forced in the renesas_fw_callback().
+		 */
+		return;
+	}
+
 	xhci = hcd_to_xhci(pci_get_drvdata(dev));
 	xhci->xhc_state |= XHCI_STATE_REMOVING;
 
-- 
2.20.1

[PATCH 2/5] usb: xhci: handle uPD720201 and uPD720202 w/o ROM

[Vinod Koul]

From: Christian Lamparter <chunkeey@googlemail.com>

This patch adds a firmware check for the uPD720201K8-711-BAC-A
and uPD720202K8-711-BAA-A variant. Both of these chips are listed
in Renesas' R19UH0078EJ0500 Rev.5.00 "User's Manual: Hardware" as
devices which need a firmware in order to work as they do not have
support to load the firmware from an external ROM.

Currently, the xhci-pci driver is unable to initialize the hcd in
this case. Instead it will wait for 30 seconds and cause a timeout
in xhci_handshake() and fails.

[    5.116990] xhci_hcd 0000:45:00.0: new USB bus registered ...
[   32.335215] xhci_hcd 0000:45:00.0: can't setup: -110
[   32.340179] xhci_hcd 0000:45:00.0: USB bus 2 deregistered
[   32.345587] xhci_hcd 0000:45:00.0: init 0000:45:00.0 fail, -110
[   32.351496] xhci_hcd: probe of 0000:45:00.0 failed with error -110

Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 drivers/usb/host/xhci-pci.c | 59 +++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index a400cf7b39da..38a56128c1ee 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -733,6 +733,55 @@ static int renesas_fw_download_to_hw(struct pci_dev *pdev,
 	return 1;
 }
 
+static int renesas_check_if_fw_dl_is_needed(struct pci_dev *pdev)
+{
+	int err;
+	u8 fw_state;
+
+	/*
+	 * Only the uPD720201K8-711-BAC-A or uPD720202K8-711-BAA-A
+	 * are listed in R19UH0078EJ0500 Rev.5.00 as devices which
+	 * need a firmware in order to work.
+	 *
+	 *  - uPD720202 ES 2.0 sample & CS sample & Mass product, ID is 2.
+	 *  - uPD720201 ES 2.0 sample whose revision ID is 2.
+	 *  - uPD720201 ES 2.1 sample & CS sample & Mass product, ID is 3.
+	 */
+	if (!((pdev->vendor == PCI_VENDOR_ID_RENESAS) &&
+		((pdev->device == 0x0015 && pdev->revision == 0x02) ||
+		 (pdev->device == 0x0014 &&
+		  (pdev->revision == 0x02 || pdev->revision == 0x03)))))
+		return 0;
+
+	/*
+	 * Test if the firmware was uploaded and is running.
+	 * As most BIOSes will initialize the device for us.
+	 */
+	err = pci_read_config_byte(pdev, 0xf4, &fw_state);
+	if (err)
+		return pcibios_err_to_errno(err);
+
+	/* Check the "Result Code" Bits (6:4) and act accordingly */
+	switch (fw_state & 0x70) {
+	case 0: /* No result yet */
+		dev_err(&pdev->dev, "FW is not ready/loaded yet.");
+		return -ENODEV;
+
+	case BIT(4): /* Success, device should be working. */
+		dev_dbg(&pdev->dev, "FW is ready.");
+		return 0;
+
+	case BIT(5): /* Error State */
+		dev_err(&pdev->dev, "HW is in an error state.");
+		return -ENODEV;
+
+	default: /* All other states are marked as "Reserved states" */
+		dev_err(&pdev->dev, "HW is in an invalid state (%x).",
+			(fw_state & 0x70) >> 4);
+		return -EINVAL;
+	}
+}
+
 /* called during probe() after chip reset completes */
 static int xhci_pci_setup(struct usb_hcd *hcd)
 {
@@ -787,6 +836,11 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		return retval;
 	};
 
+	/* Check if this device is a RENESAS uPD720201/2 device. */
+	retval = renesas_check_if_fw_dl_is_needed(dev);
+	if (retval)
+		return retval;
+
 	driver = (struct hc_driver *)id->driver_data;
 
 	/* Prevent runtime suspending between USB-2 and USB-3 initialization */
@@ -988,6 +1042,11 @@ static int xhci_pci_resume(struct usb_hcd *hcd, bool hibernated)
 	if (pdev->vendor == PCI_VENDOR_ID_INTEL)
 		usb_enable_intel_xhci_ports(pdev);
 
+	/* Check if this device is a RENESAS uPD720201/2 device. */
+	retval = renesas_check_if_fw_dl_is_needed(pdev);
+	if (retval)
+		return retval;
+
 	if (xhci->quirks & XHCI_SSIC_PORT_UNUSED)
 		xhci_ssic_port_unused_quirk(hcd, false);
 
-- 
2.20.1

[PATCH 3/5] usb: xhci: Use register defined and field names

[Vinod Koul]

Instead of using register values and fields lets define them and
use in the driver.

Signed-off-by: Vinod Koul <vkoul@kernel.org>
Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Cc: Christian Lamparter <chunkeey@googlemail.com>
---
 drivers/usb/host/xhci-pci.c | 68 +++++++++++++++++++++++++------------
 1 file changed, 47 insertions(+), 21 deletions(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 38a56128c1ee..a9497c6e3568 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -57,6 +57,27 @@
 #define PCI_DEVICE_ID_AMD_PROMONTORYA_1			0x43bc
 #define PCI_DEVICE_ID_ASMEDIA_1042A_XHCI		0x1142
 
+#define RENESAS_FW_VERSION				0x6C
+#define RENESAS_ROM_CONFIG				0xF0
+#define RENESAS_FW_STATUS				0xF4
+#define RENESAS_FW_STATUS_MSB				0xF5
+#define RENESAS_ROM_STATUS				0xF6
+#define RENESAS_ROM_STATUS_MSB				0xF7
+#define RENESAS_DATA0					0xF8
+#define RENESAS_DATA1					0xFC
+
+#define RENESAS_FW_VERSION_FIELD			GENMASK(23, 7)
+#define RENESAS_FW_VERSION_OFFSET			8
+
+#define RENESAS_FW_STATUS_DOWNLOAD_ENABLE		BIT(0)
+#define RENESAS_FW_STATUS_LOCK				BIT(1)
+#define RENESAS_FW_STATUS_RESULT			GENMASK(6, 4)
+  #define RENESAS_FW_STATUS_INVALID			0
+  #define RENESAS_FW_STATUS_SUCCESS			BIT(4)
+  #define RENESAS_FW_STATUS_ERROR			BIT(5)
+#define RENESAS_FW_STATUS_SET_DATA0			BIT(8)
+#define RENESAS_FW_STATUS_SET_DATA1			BIT(9)
+
 static const char hcd_name[] = "xhci_hcd";
 
 static struct hc_driver __read_mostly xhci_pci_hc_driver;
@@ -342,7 +363,8 @@ static int renesas_fw_download_image(struct pci_dev *dev,
 
 	/* step+1. Read "Set DATAX" and confirm it is cleared. */
 	for (i = 0; i < 10000; i++) {
-		err = pci_read_config_byte(dev, 0xF5, &fw_status);
+		err = pci_read_config_byte(dev, RENESAS_FW_STATUS_MSB,
+					   &fw_status);
 		if (err)
 			return pcibios_err_to_errno(err);
 		if (!(fw_status & BIT(data0_or_data1)))
@@ -357,7 +379,8 @@ static int renesas_fw_download_image(struct pci_dev *dev,
 	 * step+2. Write FW data to "DATAX".
 	 * "LSB is left" => force little endian
 	 */
-	err = pci_write_config_dword(dev, data0_or_data1 ? 0xFC : 0xF8,
+	err = pci_write_config_dword(dev, data0_or_data1 ?
+				     RENESAS_DATA1 : RENESAS_DATA0,
 				     (__force u32) cpu_to_le32(fw[step]));
 	if (err)
 		return pcibios_err_to_errno(err);
@@ -365,7 +388,8 @@ static int renesas_fw_download_image(struct pci_dev *dev,
 	udelay(100);
 
 	/* step+3. Set "Set DATAX". */
-	err = pci_write_config_byte(dev, 0xF5, BIT(data0_or_data1));
+	err = pci_write_config_byte(dev, RENESAS_FW_STATUS_MSB,
+				    BIT(data0_or_data1));
 	if (err)
 		return pcibios_err_to_errno(err);
 
@@ -439,7 +463,7 @@ static int renesas_fw_check_running(struct pci_dev *pdev)
 	 * BIOSes will initialize the device for us. If the device is
 	 * initialized.
 	 */
-	err = pci_read_config_byte(pdev, 0xF4, &fw_state);
+	err = pci_read_config_byte(pdev, RENESAS_FW_STATUS, &fw_state);
 	if (err)
 		return pcibios_err_to_errno(err);
 
@@ -448,10 +472,10 @@ static int renesas_fw_check_running(struct pci_dev *pdev)
 	 * ready we can simply continue. If the FW is not ready, we have
 	 * to give up.
 	 */
-	if (fw_state & BIT(1)) {
+	if (fw_state & RENESAS_FW_STATUS_LOCK) {
 		dev_dbg(&pdev->dev, "FW Download Lock is engaged.");
 
-		if (fw_state & BIT(4))
+		if (fw_state & RENESAS_FW_STATUS_SUCCESS)
 			return 0;
 
 		dev_err(&pdev->dev, "FW Download Lock is set and FW is not ready. Giving Up.");
@@ -463,30 +487,30 @@ static int renesas_fw_check_running(struct pci_dev *pdev)
 	 * with it and it can't be resetted, we have to give up too... and
 	 * ask for a forgiveness and a reboot.
 	 */
-	if (fw_state & BIT(0)) {
+	if (fw_state & RENESAS_FW_STATUS_DOWNLOAD_ENABLE) {
 		dev_err(&pdev->dev, "FW Download Enable is stale. Giving Up (poweroff/reboot needed).");
 		return -EIO;
 	}
 
 	/* Otherwise, Check the "Result Code" Bits (6:4) and act accordingly */
-	switch ((fw_state & 0x70)) {
+	switch (fw_state & RENESAS_FW_STATUS_RESULT) {
 	case 0: /* No result yet */
 		dev_dbg(&pdev->dev, "FW is not ready/loaded yet.");
 
 		/* tell the caller, that this device needs the firmware. */
 		return 1;
 
-	case BIT(4): /* Success, device should be working. */
+	case RENESAS_FW_STATUS_SUCCESS: /* Success, device should be working. */
 		dev_dbg(&pdev->dev, "FW is ready.");
 		return 0;
 
-	case BIT(5): /* Error State */
+	case RENESAS_FW_STATUS_ERROR: /* Error State */
 		dev_err(&pdev->dev, "hardware is in an error state. Giving up (poweroff/reboot needed).");
 		return -ENODEV;
 
 	default: /* All other states are marked as "Reserved states" */
-		dev_err(&pdev->dev, "hardware is in an invalid state %x. Giving up (poweroff/reboot needed).",
-			(fw_state & 0x70) >> 4);
+		dev_err(&pdev->dev, "hardware is in an invalid state %lx. Giving up (poweroff/reboot needed).",
+			(fw_state & RENESAS_FW_STATUS_RESULT) >> 4);
 		return -EINVAL;
 	}
 }
@@ -530,7 +554,8 @@ static int renesas_fw_download(struct pci_dev *pdev,
 	 * 0. Set "FW Download Enable" bit in the
 	 * "FW Download Control & Status Register" at 0xF4
 	 */
-	err = pci_write_config_byte(pdev, 0xF4, BIT(0));
+	err = pci_write_config_byte(pdev, RENESAS_FW_STATUS,
+				    RENESAS_FW_STATUS_DOWNLOAD_ENABLE);
 	if (err)
 		return pcibios_err_to_errno(err);
 
@@ -550,7 +575,8 @@ static int renesas_fw_download(struct pci_dev *pdev,
 	 * is cleared by the hardware beforehand.
 	 */
 	for (i = 0; i < 10000; i++) {
-		err = pci_read_config_byte(pdev, 0xF5, &fw_status);
+		err = pci_read_config_byte(pdev, RENESAS_FW_STATUS_MSB,
+					   &fw_status);
 		if (err)
 			return pcibios_err_to_errno(err);
 		if (!(fw_status & (BIT(0) | BIT(1))))
@@ -565,16 +591,16 @@ static int renesas_fw_download(struct pci_dev *pdev,
 	 * 11. After finishing writing the last data of FW, the
 	 * System Software must clear "FW Download Enable"
 	 */
-	err = pci_write_config_byte(pdev, 0xF4, 0);
+	err = pci_write_config_byte(pdev, RENESAS_FW_STATUS, 0);
 	if (err)
 		return pcibios_err_to_errno(err);
 
 	/* 12. Read "Result Code" and confirm it is good. */
 	for (i = 0; i < 10000; i++) {
-		err = pci_read_config_byte(pdev, 0xF4, &fw_status);
+		err = pci_read_config_byte(pdev, RENESAS_FW_STATUS, &fw_status);
 		if (err)
 			return pcibios_err_to_errno(err);
-		if (fw_status & BIT(4))
+		if (fw_status & RENESAS_FW_STATUS_SUCCESS)
 			break;
 
 		udelay(1);
@@ -757,21 +783,21 @@ static int renesas_check_if_fw_dl_is_needed(struct pci_dev *pdev)
 	 * Test if the firmware was uploaded and is running.
 	 * As most BIOSes will initialize the device for us.
 	 */
-	err = pci_read_config_byte(pdev, 0xf4, &fw_state);
+	err = pci_read_config_byte(pdev, RENESAS_FW_STATUS, &fw_state);
 	if (err)
 		return pcibios_err_to_errno(err);
 
 	/* Check the "Result Code" Bits (6:4) and act accordingly */
-	switch (fw_state & 0x70) {
+	switch (fw_state & RENESAS_FW_STATUS_RESULT) {
 	case 0: /* No result yet */
 		dev_err(&pdev->dev, "FW is not ready/loaded yet.");
 		return -ENODEV;
 
-	case BIT(4): /* Success, device should be working. */
+	case RENESAS_FW_STATUS_SUCCESS:
 		dev_dbg(&pdev->dev, "FW is ready.");
 		return 0;
 
-	case BIT(5): /* Error State */
+	case RENESAS_FW_STATUS_ERROR:
 		dev_err(&pdev->dev, "HW is in an error state.");
 		return -ENODEV;
 
-- 
2.20.1

[PATCH 4/5] usb: xhci: Add ROM loader for uPD720201

[Vinod Koul]

uPD720201 supports ROM and allows software to program the ROM and boot
from it. Add support for detecting if ROM is present, if so load the ROM
if not programmed earlier.

Signed-off-by: Vinod Koul <vkoul@kernel.org>
Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Cc: Christian Lamparter <chunkeey@googlemail.com>
---
 drivers/usb/host/xhci-pci.c | 358 ++++++++++++++++++++++++++++++++++++
 1 file changed, 358 insertions(+)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index a9497c6e3568..3574afac44c5 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -78,6 +78,20 @@
 #define RENESAS_FW_STATUS_SET_DATA0			BIT(8)
 #define RENESAS_FW_STATUS_SET_DATA1			BIT(9)
 
+#define RENESAS_ROM_STATUS_ACCESS			BIT(0)
+#define RENESAS_ROM_STATUS_ERASE			BIT(1)
+#define RENESAS_ROM_STATUS_RELOAD			BIT(2)
+#define RENESAS_ROM_STATUS_RESULT			GENMASK(6, 4)
+  #define RENESAS_ROM_STATUS_INVALID			0
+  #define RENESAS_ROM_STATUS_SUCCESS			BIT(4)
+  #define RENESAS_ROM_STATUS_ERROR			BIT(5)
+#define RENESAS_ROM_STATUS_SET_DATA0			BIT(8)
+#define RENESAS_ROM_STATUS_SET_DATA1			BIT(9)
+#define RENESAS_ROM_STATUS_ROM_EXISTS			BIT(15)
+
+#define RENESAS_ROM_ERASE_MAGIC				0x5A65726F
+#define RENESAS_ROM_WRITE_MAGIC				0x53524F4D
+
 static const char hcd_name[] = "xhci_hcd";
 
 static struct hc_driver __read_mostly xhci_pci_hc_driver;
@@ -453,11 +467,75 @@ static int renesas_fw_verify(struct pci_dev *dev,
 	return 0;
 }
 
+static int renesas_check_rom_state(struct pci_dev *pdev)
+{
+	const struct renesas_fw_entry *entry;
+	u16 rom_state;
+	u32 version;
+	bool valid_version = false;
+	int err, i;
+
+	/* check FW version */
+	err = pci_read_config_dword(pdev, RENESAS_FW_VERSION, &version);
+	if (err)
+		return pcibios_err_to_errno(err);
+
+	version &= RENESAS_FW_VERSION_FIELD;
+	version = version >> RENESAS_FW_VERSION_OFFSET;
+	dev_dbg(&pdev->dev, "Found FW version loaded is %x\n", version);
+
+	/* treat version in renesas_fw_table as correct ones */
+	for (i = 0; i < ARRAY_SIZE(renesas_fw_table); i++) {
+		entry = &renesas_fw_table[i];
+		if (version == entry->expected_version) {
+			dev_dbg(&pdev->dev, "Detected valid ROM version..\n");
+			valid_version = true;
+		}
+	}
+
+	/*
+	 * Test if ROM is present and loaded, if so we can skip everything
+	 */
+	err = pci_read_config_word(pdev, RENESAS_ROM_STATUS, &rom_state);
+	if (err)
+		return pcibios_err_to_errno(err);
+
+	if (rom_state & BIT(15)) {
+		/* ROM exists */
+		dev_dbg(&pdev->dev, "ROM exists\n");
+
+		/* Check the "Result Code" Bits (6:4) and act accordingly */
+		switch (rom_state & RENESAS_ROM_STATUS_RESULT) {
+		case RENESAS_ROM_STATUS_SUCCESS:
+			dev_dbg(&pdev->dev, "Success ROM load...");
+			/* we have valid version and status so success */
+			if (valid_version)
+				return 0;
+		case RENESAS_ROM_STATUS_INVALID: /* No result yet */
+			dev_dbg(&pdev->dev, "No result as it is ROM...");
+			/* we have valid version and status so success */
+			if (valid_version)
+				return 0;
+		case RENESAS_ROM_STATUS_ERROR: /* Error State */
+		default: /* All other states are marked as "Reserved states" */
+			dev_err(&pdev->dev, "Invalid ROM..");
+			break;
+		}
+	}
+
+	return -EIO;
+}
+
 static int renesas_fw_check_running(struct pci_dev *pdev)
 {
 	int err;
 	u8 fw_state;
 
+	/* Check if device has ROM and loaded, if so skip everything */
+	err = renesas_check_rom_state(pdev);
+	if (!err)
+		return err;
+
 	/*
 	 * Test if the device is actually needing the firmware. As most
 	 * BIOSes will initialize the device for us. If the device is
@@ -650,17 +728,292 @@ struct renesas_fw_ctx {
 static int xhci_pci_probe(struct pci_dev *pdev,
 			  const struct pci_device_id *id);
 
+static bool renesas_check_rom(struct pci_dev *pdev)
+{
+	u16 rom_status;
+	int retval;
+
+	/* 1. Check if external ROM exists */
+	retval = pci_read_config_word(pdev, RENESAS_ROM_STATUS, &rom_status);
+	if (retval)
+		return false;
+
+	rom_status &= RENESAS_ROM_STATUS_ROM_EXISTS;
+	if (rom_status) {
+		dev_dbg(&pdev->dev, "External ROM exists\n");
+		return true; /* External ROM exists */
+	}
+
+	return false;
+}
+
+static void renesas_rom_erase(struct pci_dev *pdev)
+{
+	int retval, i;
+	u8 status;
+
+	dev_dbg(&pdev->dev, "Performing ROM Erase...\n");
+	retval = pci_write_config_dword(pdev, RENESAS_DATA0,
+					RENESAS_ROM_ERASE_MAGIC);
+	if (retval) {
+		dev_err(&pdev->dev, "ROM erase, magic word write failed: %d\n",
+			pcibios_err_to_errno(retval));
+		return;
+	}
+
+	retval = pci_read_config_byte(pdev, RENESAS_ROM_STATUS, &status);
+	if (retval) {
+		dev_err(&pdev->dev, "ROM status read failed: %d\n",
+			pcibios_err_to_errno(retval));
+		return;
+	}
+	status |= RENESAS_ROM_STATUS_ERASE;
+	retval = pci_write_config_byte(pdev, RENESAS_ROM_STATUS, status);
+	if (retval) {
+		dev_err(&pdev->dev, "ROM erase set word write failed\n");
+		return;
+	}
+
+	/* sleep a bit while ROM is erased */
+	msleep(20);
+
+	for (i = 0; i < 10000; i++) {
+		retval = pci_read_config_byte(pdev, RENESAS_ROM_STATUS,
+					      &status);
+		status &= RENESAS_ROM_STATUS_ERASE;
+		if (!status)
+			break;
+
+		mdelay(10);
+	}
+
+	if (i == 10000)
+		dev_dbg(&pdev->dev, "Chip erase timedout: %x\n", status);
+
+	dev_dbg(&pdev->dev, "ROM Erase... Done success\n");
+}
+
+static bool renesas_download_rom(struct pci_dev *pdev,
+				 const u32 *fw, size_t step)
+{
+	bool data0_or_data1;
+	u8 fw_status;
+	size_t i;
+	int err;
+
+	/*
+	 * The hardware does alternate between two 32-bit pages.
+	 * (This is because each row of the firmware is 8 bytes).
+	 *
+	 * for even steps we use DATA0, for odd steps DATA1.
+	 */
+	data0_or_data1 = (step & 1) == 1;
+
+	/* Read "Set DATAX" and confirm it is cleared. */
+	for (i = 0; i < 10000; i++) {
+		err = pci_read_config_byte(pdev, RENESAS_ROM_STATUS_MSB,
+					   &fw_status);
+		if (err) {
+			dev_err(&pdev->dev, "Read ROM Status failed: %d\n",
+				pcibios_err_to_errno(err));
+			return false;
+		}
+		if (!(fw_status & BIT(data0_or_data1)))
+			break;
+
+		udelay(100);
+	}
+	if (i == 10000) {
+		dev_err(&pdev->dev, "Timeout for Set DATAX step: %zd\n", step);
+		return false;
+	}
+
+	/*
+	 * Write FW data to "DATAX".
+	 * "LSB is left" => force little endian
+	 */
+	err = pci_write_config_dword(pdev, data0_or_data1 ?
+				     RENESAS_DATA1 : RENESAS_DATA0,
+				     (__force u32)cpu_to_le32(fw[step]));
+	if (err) {
+		dev_err(&pdev->dev, "Write to DATAX failed: %d\n",
+			pcibios_err_to_errno(err));
+		return false;
+	}
+
+	udelay(100);
+
+	/* Set "Set DATAX". */
+	err = pci_write_config_byte(pdev, RENESAS_ROM_STATUS_MSB,
+				    BIT(data0_or_data1));
+	if (err) {
+		dev_err(&pdev->dev, "Write config for DATAX failed: %d\n",
+			pcibios_err_to_errno(err));
+		return false;
+	}
+
+	return true;
+}
+
+static bool renesas_setup_rom(struct pci_dev *pdev, const struct firmware *fw)
+{
+	const u32 *fw_data = (const u32 *)fw->data;
+	int err, i;
+	u8 status;
+
+	/* 2. Write magic word to Data0 */
+	err = pci_write_config_dword(pdev, RENESAS_DATA0,
+				     RENESAS_ROM_WRITE_MAGIC);
+	if (err)
+		return false;
+
+	/* 3. Set External ROM access */
+	err = pci_write_config_byte(pdev, RENESAS_ROM_STATUS,
+				    RENESAS_ROM_STATUS_ACCESS);
+	if (err)
+		goto remove_bypass;
+
+	/* 4. Check the result */
+	err = pci_read_config_byte(pdev, RENESAS_ROM_STATUS, &status);
+	if (err)
+		goto remove_bypass;
+	status &= GENMASK(6, 4);
+	if (status) {
+		dev_err(&pdev->dev,
+			"setting external rom failed: %x\n", status);
+		goto remove_bypass;
+	}
+
+	/* 5 to 16 Write FW to DATA0/1 while checking SetData0/1 */
+	for (i = 0; i < fw->size / 4; i++) {
+		err = renesas_download_rom(pdev, fw_data, i);
+		if (!err) {
+			dev_err(&pdev->dev,
+				"ROM Download Step %d failed at position %d bytes\n",
+				 i, i * 4);
+			goto remove_bypass;
+		}
+	}
+
+	/*
+	 * wait till DATA0/1 is cleared
+	 */
+	for (i = 0; i < 1000; i++) {
+		err = pci_read_config_byte(pdev, RENESAS_ROM_STATUS_MSB,
+					   &status);
+		if (err)
+			goto remove_bypass;
+		if (!(status & (BIT(0) | BIT(1))))
+			break;
+
+		udelay(10);
+	}
+	if (i == 1000) {
+		dev_err(&pdev->dev, "Final Firmware ROM Download step timed out\n");
+		goto remove_bypass;
+	}
+
+	/* 17. Remove bypass */
+	err = pci_write_config_byte(pdev, RENESAS_ROM_STATUS, 0);
+	if (err)
+		return false;
+
+	udelay(10);
+
+	/* 18. check result */
+	for (i = 0; i < 10000; i++) {
+		err = pci_read_config_byte(pdev, RENESAS_ROM_STATUS, &status);
+		if (err) {
+			dev_err(&pdev->dev, "Read ROM status failed:%d\n",
+				pcibios_err_to_errno(err));
+			return false;
+		}
+		status &= RENESAS_ROM_STATUS_RESULT;
+		if (status ==  RENESAS_ROM_STATUS_SUCCESS) {
+			dev_dbg(&pdev->dev, "Download ROM success\n");
+			break;
+		}
+		udelay(100);
+	}
+	if (i == 10000) { /* Timed out */
+		dev_err(&pdev->dev,
+			"Download to external ROM TO: %x\n", status);
+		return false;
+	}
+
+	dev_dbg(&pdev->dev, "Download to external ROM scuceeded\n");
+
+	/* Last step set Reload */
+	err = pci_write_config_byte(pdev, RENESAS_ROM_STATUS,
+				    RENESAS_ROM_STATUS_RELOAD);
+	if (err) {
+		dev_err(&pdev->dev, "Set ROM execute failed: %d\n",
+			pcibios_err_to_errno(err));
+		return false;
+	}
+
+	/*
+	 * wait till Reload is cleared
+	 */
+	for (i = 0; i < 10000; i++) {
+		err = pci_read_config_byte(pdev, RENESAS_ROM_STATUS, &status);
+		if (err)
+			return false;
+		if (!(status & RENESAS_ROM_STATUS_RELOAD))
+			break;
+
+		udelay(1);
+	}
+	if (i == 10000) {
+		dev_err(&pdev->dev, "ROM Exec timed out: %x\n", status);
+		return false;
+	}
+
+	return true;
+
+remove_bypass:
+	pci_write_config_byte(pdev, RENESAS_ROM_STATUS, 0);
+	return false;
+}
+
 static void renesas_fw_callback(const struct firmware *fw,
 				void *context)
 {
 	struct renesas_fw_ctx *ctx = context;
 	struct pci_dev *pdev = ctx->pdev;
 	struct device *parent = pdev->dev.parent;
+	bool rom;
 	int err = -ENOENT;
 
 	if (fw) {
 		err = renesas_fw_verify(pdev, fw->data, fw->size);
 		if (!err) {
+			/* Check if the device has external ROM */
+			rom = renesas_check_rom(pdev);
+			if (rom) {
+				/* perfrom chip erase first */
+				renesas_rom_erase(pdev);
+
+				/* lets try loading fw on ROM first */
+				rom = renesas_setup_rom(pdev, fw);
+				if (!rom) {
+					dev_err(&pdev->dev, "ROM load failed, falling back on FW load\n");
+				} else {
+					dev_dbg(&pdev->dev, "ROM load done..\n");
+
+					release_firmware(fw);
+					if (ctx->resume)
+						return;
+
+					err = xhci_pci_probe(pdev, ctx->id);
+					if (!err) {
+						/* everything worked */
+						devm_kfree(&pdev->dev, ctx);
+						return;
+					}
+				}
+			}
+
 			err = renesas_fw_download(pdev, fw, 0);
 			release_firmware(fw);
 			if (!err) {
@@ -779,6 +1132,11 @@ static int renesas_check_if_fw_dl_is_needed(struct pci_dev *pdev)
 		  (pdev->revision == 0x02 || pdev->revision == 0x03)))))
 		return 0;
 
+	/* Check if device has ROM and loaded, if so skip everything */
+	err = renesas_check_rom_state(pdev);
+	if (!err)
+		return err;
+
 	/*
 	 * Test if the firmware was uploaded and is running.
 	 * As most BIOSes will initialize the device for us.
-- 
2.20.1

[PATCH 5/5] usb: xhci: allow multiple firmware versions

[Vinod Koul]

Allow multiple firmware file versions in table and load them in
increasing order as we find them in the file system.

Signed-off-by: Vinod Koul <vkoul@kernel.org>
Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Cc: Christian Lamparter <chunkeey@googlemail.com>
---
 drivers/usb/host/xhci-pci.c | 39 +++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 3574afac44c5..2ee9e6bbabcb 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -333,9 +333,12 @@ static const struct renesas_fw_entry {
 	 *  - uPD720201 ES 2.0 sample whose revision ID is 2.
 	 *  - uPD720201 ES 2.1 sample & CS sample & Mass product, ID is 3.
 	 *  - uPD720202 ES 2.0 sample & CS sample & Mass product, ID is 2.
+	 *
+	 *  Entry expected_version should be kept in increasing order
 	 */
 	{ "K2013080.mem", 0x0014, 0x02, 0x2013 },
 	{ "K2013080.mem", 0x0014, 0x03, 0x2013 },
+	{ "K2026090.mem", 0x0014, 0x03, 0x2026 },
 	{ "K2013080.mem", 0x0015, 0x02, 0x2013 },
 };
 
@@ -358,6 +361,24 @@ static const struct renesas_fw_entry *renesas_needs_fw_dl(struct pci_dev *dev)
 	return NULL;
 }
 
+static const struct
+renesas_fw_entry *renesas_get_next_entry(struct pci_dev *dev,
+					 const struct renesas_fw_entry *entry)
+{
+	const struct renesas_fw_entry *next_entry;
+	size_t i;
+
+	for (i = 0; i < ARRAY_SIZE(renesas_fw_table); i++) {
+		next_entry = &renesas_fw_table[i];
+		if (next_entry->device == dev->device &&
+		    next_entry->revision == dev->revision &&
+		    next_entry->expected_version > entry->expected_version)
+			return next_entry;
+	}
+
+	return NULL;
+}
+
 static int renesas_fw_download_image(struct pci_dev *dev,
 				     const u32 *fw,
 				     size_t step)
@@ -723,6 +744,7 @@ struct renesas_fw_ctx {
 	struct pci_dev *pdev;
 	const struct pci_device_id *id;
 	bool resume;
+	const struct renesas_fw_entry *entry;
 };
 
 static int xhci_pci_probe(struct pci_dev *pdev,
@@ -982,6 +1004,7 @@ static void renesas_fw_callback(const struct firmware *fw,
 	struct renesas_fw_ctx *ctx = context;
 	struct pci_dev *pdev = ctx->pdev;
 	struct device *parent = pdev->dev.parent;
+	const struct renesas_fw_entry *next_entry;
 	bool rom;
 	int err = -ENOENT;
 
@@ -1035,6 +1058,21 @@ static void renesas_fw_callback(const struct firmware *fw,
 		}
 	} else {
 		dev_err(&pdev->dev, "firmware failed to load (%d).", err);
+		/*
+		 * we didn't find firmware, check if we have another
+		 * entry for this device
+		 */
+		next_entry = renesas_get_next_entry(ctx->pdev, ctx->entry);
+		if (next_entry) {
+			ctx->entry = next_entry;
+			dev_dbg(&pdev->dev, "Found next entry, requesting: %s\n",
+				next_entry->firmware_name);
+			request_firmware_nowait(THIS_MODULE, 1,
+						next_entry->firmware_name,
+						&pdev->dev, GFP_KERNEL,
+						ctx, renesas_fw_callback);
+			return;
+		}
 	}
 
 	dev_info(&pdev->dev, "Unloading driver");
@@ -1096,6 +1134,7 @@ static int renesas_fw_download_to_hw(struct pci_dev *pdev,
 	ctx->pdev = pdev;
 	ctx->resume = do_resume;
 	ctx->id = id;
+	ctx->entry = entry;
 
 	pci_dev_get(pdev);
 	err = request_firmware_nowait(THIS_MODULE, 1, entry->firmware_name,
-- 
2.20.1

Re: [PATCH 1/5] usb: xhci: add firmware loader for uPD720201 and uPD720202 w/o ROM

[Greg Kroah-Hartman]

On Thu, Jun 20, 2019 at 03:51:50PM +0530, Vinod Koul wrote:
> From: Christian Lamparter <chunkeey@googlemail.com>
> 
> This patch adds a firmware loader for the uPD720201K8-711-BAC-A
> and uPD720202K8-711-BAA-A variant. Both of these chips are listed
> in Renesas' R19UH0078EJ0500 Rev.5.00 "User's Manual: Hardware" as
> devices which need the firmware loader on page 2 in order to
> work as they "do not support the External ROM".
> 
> The "Firmware Download Sequence" is describe in chapter
> "7.1 FW Download Interface" R19UH0078EJ0500 Rev.5.00 page 131.
> 
> The firmware "K2013080.mem" is available from a USB3.0 Host to
> PCIe Adapter (PP2U-E card) "Firmware download" archive. An
> alternative version can be sourced from Netgear's WNDR4700 GPL
> archives.
> 
> The release notes of the PP2U-E's "Firmware Download" ver 2.0.1.3
> (2012-06-15) state that the firmware is for the following devices:
>  - uPD720201 ES 2.0 sample whose revision ID is 2.
>  - uPD720201 ES 2.1 sample & CS sample & Mass product, ID is 3.
>  - uPD720202 ES 2.0 sample & CS sample & Mass product, ID is 2.
> 
> If someone from Renesas is listening: It would be great, if these
> firmwares could be added to linux-firmware.git.

That paragraph does not need to be in the changelog :)

> Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  drivers/usb/host/xhci-pci.c | 480 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 480 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index c2fe218e051f..a400cf7b39da 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -12,6 +12,8 @@
>  #include <linux/slab.h>
>  #include <linux/module.h>
>  #include <linux/acpi.h>
> +#include <linux/firmware.h>
> +#include <asm/unaligned.h>

asm/ in a driver?  Are you sure???

>  
>  #include "xhci.h"
>  #include "xhci-trace.h"
> @@ -279,6 +281,458 @@ static void xhci_pme_acpi_rtd3_enable(struct pci_dev *dev)
>  static void xhci_pme_acpi_rtd3_enable(struct pci_dev *dev) { }
>  #endif /* CONFIG_ACPI */
>  
> +static const struct renesas_fw_entry {
> +	const char *firmware_name;
> +	u16 device;
> +	u8 revision;
> +	u16 expected_version;
> +} renesas_fw_table[] = {
> +	/*
> +	 * Only the uPD720201K8-711-BAC-A or uPD720202K8-711-BAA-A
> +	 * are listed in R19UH0078EJ0500 Rev.5.00 as devices which
> +	 * need the software loader.
> +	 *
> +	 * PP2U/ReleaseNote_USB3-201-202-FW.txt:
> +	 *
> +	 * Note: This firmware is for the following devices.
> +	 *  - uPD720201 ES 2.0 sample whose revision ID is 2.
> +	 *  - uPD720201 ES 2.1 sample & CS sample & Mass product, ID is 3.
> +	 *  - uPD720202 ES 2.0 sample & CS sample & Mass product, ID is 2.
> +	 */
> +	{ "K2013080.mem", 0x0014, 0x02, 0x2013 },
> +	{ "K2013080.mem", 0x0014, 0x03, 0x2013 },
> +	{ "K2013080.mem", 0x0015, 0x02, 0x2013 },
> +};

No MODULE_FIRMWARE() entries as well?

> +static const struct renesas_fw_entry *renesas_needs_fw_dl(struct pci_dev *dev)
> +{
> +	const struct renesas_fw_entry *entry;
> +	size_t i;
> +
> +	/* This loader will only work with a RENESAS device. */
> +	if (!(dev->vendor == PCI_VENDOR_ID_RENESAS))
> +		return NULL;
> +
> +	for (i = 0; i < ARRAY_SIZE(renesas_fw_table); i++) {
> +		entry = &renesas_fw_table[i];
> +		if (entry->device == dev->device &&
> +		    entry->revision == dev->revision)
> +			return entry;
> +	}
> +
> +	return NULL;
> +}
> +
> +static int renesas_fw_download_image(struct pci_dev *dev,
> +				     const u32 *fw,
> +				     size_t step)
> +{
> +	size_t i;
> +	int err;
> +	u8 fw_status;
> +	bool data0_or_data1;
> +
> +	/*
> +	 * The hardware does alternate between two 32-bit pages.
> +	 * (This is because each row of the firmware is 8 bytes).
> +	 *
> +	 * for even steps we use DATA0, for odd steps DATA1.
> +	 */
> +	data0_or_data1 = (step & 1) == 1;
> +
> +	/* step+1. Read "Set DATAX" and confirm it is cleared. */
> +	for (i = 0; i < 10000; i++) {
> +		err = pci_read_config_byte(dev, 0xF5, &fw_status);
> +		if (err)
> +			return pcibios_err_to_errno(err);
> +		if (!(fw_status & BIT(data0_or_data1)))
> +			break;
> +
> +		udelay(1);
> +	}
> +	if (i == 10000)
> +		return -ETIMEDOUT;
> +
> +	/*
> +	 * step+2. Write FW data to "DATAX".
> +	 * "LSB is left" => force little endian
> +	 */
> +	err = pci_write_config_dword(dev, data0_or_data1 ? 0xFC : 0xF8,
> +				     (__force u32) cpu_to_le32(fw[step]));
> +	if (err)
> +		return pcibios_err_to_errno(err);
> +
> +	udelay(100);
> +
> +	/* step+3. Set "Set DATAX". */
> +	err = pci_write_config_byte(dev, 0xF5, BIT(data0_or_data1));
> +	if (err)
> +		return pcibios_err_to_errno(err);
> +

Shouldn't you just do a read after the write to be sure the write
actually went out on the wire?  Then you shouldn't have to do the
udelay, right?

> +	return 0;
> +}
> +
> +static int renesas_fw_verify(struct pci_dev *dev,
> +			     const void *fw_data,
> +			     size_t length)
> +{
> +	const struct renesas_fw_entry *entry = renesas_needs_fw_dl(dev);
> +	u16 fw_version_pointer;
> +	u16 fw_version;
> +
> +	if (!entry)
> +		return -EINVAL;
> +
> +	/*
> +	 * The Firmware's Data Format is describe in
> +	 * "6.3 Data Format" R19UH0078EJ0500 Rev.5.00 page 124
> +	 */
> +
> +	/* "Each row is 8 bytes". => firmware size must be a multiple of 8. */
> +	if (length % 8 != 0) {
> +		dev_err(&dev->dev, "firmware size is not a multipe of 8.");

"multiple"

> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * The bootrom chips of the big brother have sizes up to 64k, let's
> +	 * assume that's the biggest the firmware can get.
> +	 */
> +	if (length < 0x1000 || length >= 0x10000) {
> +		dev_err(&dev->dev, "firmware is size %zd is not (4k - 64k).",
> +			length);
> +		return -EINVAL;
> +	}
> +
> +	/* The First 2 bytes are fixed value (55aa). "LSB on Left" */
> +	if (get_unaligned_le16(fw_data) != 0x55aa) {
> +		dev_err(&dev->dev, "no valid firmware header found.");
> +		return -EINVAL;
> +	}
> +
> +	/* verify the firmware version position and print it. */
> +	fw_version_pointer = get_unaligned_le16(fw_data + 4);
> +	if (fw_version_pointer + 2 >= length) {
> +		dev_err(&dev->dev, "firmware version pointer is outside of the firmware image.");
> +		return -EINVAL;
> +	}
> +
> +	fw_version = get_unaligned_le16(fw_data + fw_version_pointer);
> +	dev_dbg(&dev->dev, "got firmware version: %02x.", fw_version);
> +
> +	if (fw_version != entry->expected_version) {
> +		dev_err(&dev->dev, "firmware version mismatch, expected version: %02x.",
> +			 entry->expected_version);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int renesas_hw_check_run_stop_busy(struct pci_dev *pdev)
> +{
> +#if 0
> +	u32 val;
> +
> +	/*
> +	 * 7.1.3 Note 3: "... must not set 'FW Download Enable' when
> +	 * 'RUN/STOP' of USBCMD Register is set"
> +	 */
> +	val = readl(hcd->regs + 0x20);
> +	if (val & BIT(0)) {
> +		dev_err(&pdev->dev, "hardware is busy and can't receive a FW.");
> +		return -EBUSY;
> +	}
> +#endif
> +	return 0;
> +}
> +

Is this function still really needed anymore?


> +	/*
> +	 * 11. After finishing writing the last data of FW, the
> +	 * System Software must clear "FW Download Enable"
> +	 */
> +	err = pci_write_config_byte(pdev, 0xF4, 0);
> +	if (err)
> +		return pcibios_err_to_errno(err);
> +
> +	/* 12. Read "Result Code" and confirm it is good. */
> +	for (i = 0; i < 10000; i++) {
> +		err = pci_read_config_byte(pdev, 0xF4, &fw_status);
> +		if (err)
> +			return pcibios_err_to_errno(err);
> +		if (fw_status & BIT(4))
> +			break;
> +
> +		udelay(1);
> +	}

1000 reads???  I've heard of having to read a few times to ensure
something "latched" in the device, but not 1000.  Why so many?

> +	if (i == 10000) {
> +		/* Timed out / Error - let's see if we can fix this */
> +		err = renesas_fw_check_running(pdev);
> +		switch (err) {
> +		case 0: /*
> +			 * we shouldn't end up here.
> +			 * maybe it took a little bit longer.
> +			 * But all should be well?
> +			 */
> +			break;
> +
> +		case 1: /* (No result yet? - we can try to retry) */
> +			if (retry_counter < 10) {
> +				retry_counter++;
> +				dev_warn(&pdev->dev, "Retry Firmware download: %d try.",
> +					  retry_counter);
> +				return renesas_fw_download(pdev, fw,
> +							   retry_counter);

recursion?

> +			}
> +			return -ETIMEDOUT;
> +
> +		default:
> +			return err;
> +		}
> +	}
> +	/*
> +	 * Optional last step: Engage Firmware Lock
> +	 *
> +	 * err = pci_write_config_byte(pdev, 0xF4, BIT(2));
> +	 * if (err)
> +	 *	return pcibios_err_to_errno(err);
> +	 */
> +
> +	return 0;
> +}
> +
> +struct renesas_fw_ctx {
> +	struct pci_dev *pdev;
> +	const struct pci_device_id *id;
> +	bool resume;
> +};
> +
> +static int xhci_pci_probe(struct pci_dev *pdev,
> +			  const struct pci_device_id *id);
> +
> +static void renesas_fw_callback(const struct firmware *fw,
> +				void *context)
> +{
> +	struct renesas_fw_ctx *ctx = context;
> +	struct pci_dev *pdev = ctx->pdev;
> +	struct device *parent = pdev->dev.parent;
> +	int err = -ENOENT;
> +
> +	if (fw) {
> +		err = renesas_fw_verify(pdev, fw->data, fw->size);
> +		if (!err) {
> +			err = renesas_fw_download(pdev, fw, 0);
> +			release_firmware(fw);
> +			if (!err) {
> +				if (ctx->resume)
> +					return;
> +
> +				err = xhci_pci_probe(pdev, ctx->id);
> +				if (!err) {
> +					/* everything worked */
> +					devm_kfree(&pdev->dev, ctx);
> +					return;
> +				}
> +
> +				/* in case of an error - fall through */
> +			} else {
> +				dev_err(&pdev->dev, "firmware failed to download (%d).",
> +					err);
> +			}
> +		}
> +	} else {
> +		dev_err(&pdev->dev, "firmware failed to load (%d).", err);
> +	}

This can be cleaned up a lot by just erroring out when something
happens, and then jumping to the end.  Not this nested if mess.

> +
> +	dev_info(&pdev->dev, "Unloading driver");
> +
> +	if (parent)
> +		device_lock(parent);
> +
> +	device_release_driver(&pdev->dev);
> +
> +	if (parent)
> +		device_unlock(parent);
> +
> +	pci_dev_put(pdev);
> +}
> +
> +static int renesas_fw_alive_check(struct pci_dev *pdev)
> +{
> +	const struct renesas_fw_entry *entry;
> +	int err;
> +
> +	/* check if we have a eligible RENESAS' uPD720201/2 w/o FW. */
> +	entry = renesas_needs_fw_dl(pdev);
> +	if (!entry)
> +		return 0;
> +
> +	err = renesas_fw_check_running(pdev);
> +	/* Also go ahead, if the firmware is running */
> +	if (err == 0)
> +		return 0;
> +
> +	/* At this point, we can be sure that the FW isn't ready. */
> +	return err;

Why not just:
	return renesas_fw_check_running(pdev);
?

thanks,

greg k-h

Re: [PATCH 5/5] usb: xhci: allow multiple firmware versions

[Greg Kroah-Hartman]

On Thu, Jun 20, 2019 at 03:51:54PM +0530, Vinod Koul wrote:
> Allow multiple firmware file versions in table and load them in
> increasing order as we find them in the file system.
> 
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Cc: Christian Lamparter <chunkeey@googlemail.com>
> ---
>  drivers/usb/host/xhci-pci.c | 39 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 3574afac44c5..2ee9e6bbabcb 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -333,9 +333,12 @@ static const struct renesas_fw_entry {
>  	 *  - uPD720201 ES 2.0 sample whose revision ID is 2.
>  	 *  - uPD720201 ES 2.1 sample & CS sample & Mass product, ID is 3.
>  	 *  - uPD720202 ES 2.0 sample & CS sample & Mass product, ID is 2.
> +	 *
> +	 *  Entry expected_version should be kept in increasing order

Why?

>  	 */
>  	{ "K2013080.mem", 0x0014, 0x02, 0x2013 },
>  	{ "K2013080.mem", 0x0014, 0x03, 0x2013 },
> +	{ "K2026090.mem", 0x0014, 0x03, 0x2026 },
>  	{ "K2013080.mem", 0x0015, 0x02, 0x2013 },

Just put a { } entry here at the end and then you don't care about the
order at all.  Much simpler and easier to maintain over time.

thanks,

greg k-h

Re: [PATCH 1/5] usb: xhci: add firmware loader for uPD720201 and uPD720202 w/o ROM

[Vinod Koul]

Thanks Greg for a quick review :)

On 20-06-19, 14:19, Greg Kroah-Hartman wrote:
> On Thu, Jun 20, 2019 at 03:51:50PM +0530, Vinod Koul wrote:
> > From: Christian Lamparter <chunkeey@googlemail.com>
> > 
> > This patch adds a firmware loader for the uPD720201K8-711-BAC-A
> > and uPD720202K8-711-BAA-A variant. Both of these chips are listed
> > in Renesas' R19UH0078EJ0500 Rev.5.00 "User's Manual: Hardware" as
> > devices which need the firmware loader on page 2 in order to
> > work as they "do not support the External ROM".
> > 
> > The "Firmware Download Sequence" is describe in chapter
> > "7.1 FW Download Interface" R19UH0078EJ0500 Rev.5.00 page 131.
> > 
> > The firmware "K2013080.mem" is available from a USB3.0 Host to
> > PCIe Adapter (PP2U-E card) "Firmware download" archive. An
> > alternative version can be sourced from Netgear's WNDR4700 GPL
> > archives.
> > 
> > The release notes of the PP2U-E's "Firmware Download" ver 2.0.1.3
> > (2012-06-15) state that the firmware is for the following devices:
> >  - uPD720201 ES 2.0 sample whose revision ID is 2.
> >  - uPD720201 ES 2.1 sample & CS sample & Mass product, ID is 3.
> >  - uPD720202 ES 2.0 sample & CS sample & Mass product, ID is 2.
> > 
> > If someone from Renesas is listening: It would be great, if these
> > firmwares could be added to linux-firmware.git.
> 
> That paragraph does not need to be in the changelog :)

Sure will drop :)

> >  #include <linux/slab.h>
> >  #include <linux/module.h>
> >  #include <linux/acpi.h>
> > +#include <linux/firmware.h>
> > +#include <asm/unaligned.h>
> 
> asm/ in a driver?  Are you sure???

Not sure :D, will check and remove

> > +static const struct renesas_fw_entry {
> > +	const char *firmware_name;
> > +	u16 device;
> > +	u8 revision;
> > +	u16 expected_version;
> > +} renesas_fw_table[] = {
> > +	/*
> > +	 * Only the uPD720201K8-711-BAC-A or uPD720202K8-711-BAA-A
> > +	 * are listed in R19UH0078EJ0500 Rev.5.00 as devices which
> > +	 * need the software loader.
> > +	 *
> > +	 * PP2U/ReleaseNote_USB3-201-202-FW.txt:
> > +	 *
> > +	 * Note: This firmware is for the following devices.
> > +	 *  - uPD720201 ES 2.0 sample whose revision ID is 2.
> > +	 *  - uPD720201 ES 2.1 sample & CS sample & Mass product, ID is 3.
> > +	 *  - uPD720202 ES 2.0 sample & CS sample & Mass product, ID is 2.
> > +	 */
> > +	{ "K2013080.mem", 0x0014, 0x02, 0x2013 },
> > +	{ "K2013080.mem", 0x0014, 0x03, 0x2013 },
> > +	{ "K2013080.mem", 0x0015, 0x02, 0x2013 },
> > +};
> 
> No MODULE_FIRMWARE() entries as well?

Ah I wasnt even aware we had that, thanks for pointing it out, will add.

> > +static int renesas_fw_download_image(struct pci_dev *dev,
> > +				     const u32 *fw,
> > +				     size_t step)
> > +{
> > +	size_t i;
> > +	int err;
> > +	u8 fw_status;
> > +	bool data0_or_data1;
> > +
> > +	/*
> > +	 * The hardware does alternate between two 32-bit pages.
> > +	 * (This is because each row of the firmware is 8 bytes).
> > +	 *
> > +	 * for even steps we use DATA0, for odd steps DATA1.
> > +	 */
> > +	data0_or_data1 = (step & 1) == 1;
> > +
> > +	/* step+1. Read "Set DATAX" and confirm it is cleared. */
> > +	for (i = 0; i < 10000; i++) {
> > +		err = pci_read_config_byte(dev, 0xF5, &fw_status);
> > +		if (err)
> > +			return pcibios_err_to_errno(err);
> > +		if (!(fw_status & BIT(data0_or_data1)))
> > +			break;
> > +
> > +		udelay(1);
> > +	}
> > +	if (i == 10000)
> > +		return -ETIMEDOUT;
> > +
> > +	/*
> > +	 * step+2. Write FW data to "DATAX".
> > +	 * "LSB is left" => force little endian
> > +	 */
> > +	err = pci_write_config_dword(dev, data0_or_data1 ? 0xFC : 0xF8,
> > +				     (__force u32) cpu_to_le32(fw[step]));
> > +	if (err)
> > +		return pcibios_err_to_errno(err);
> > +
> > +	udelay(100);
> > +
> > +	/* step+3. Set "Set DATAX". */
> > +	err = pci_write_config_byte(dev, 0xF5, BIT(data0_or_data1));
> > +	if (err)
> > +		return pcibios_err_to_errno(err);
> > +
> 
> Shouldn't you just do a read after the write to be sure the write
> actually went out on the wire?  Then you shouldn't have to do the
> udelay, right?

Well I am not sure that is how it works. The register is a DATA register
on the controller. We are writing to the memory of the controller here
and after writing DATA0 and DATA1 we check the Set DATA0 & Set DATA1
bits and write subsequenly only when controller is ready to accept more
data.

I do recall at least for ROM load (writing to NOR flash attached to
controller), we need to wait considerably more before the SetData0/1 was
set and ready for subsequent write

> > +static int renesas_fw_verify(struct pci_dev *dev,
> > +			     const void *fw_data,
> > +			     size_t length)
> > +{
> > +	const struct renesas_fw_entry *entry = renesas_needs_fw_dl(dev);
> > +	u16 fw_version_pointer;
> > +	u16 fw_version;
> > +
> > +	if (!entry)
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * The Firmware's Data Format is describe in
> > +	 * "6.3 Data Format" R19UH0078EJ0500 Rev.5.00 page 124
> > +	 */
> > +
> > +	/* "Each row is 8 bytes". => firmware size must be a multiple of 8. */
> > +	if (length % 8 != 0) {
> > +		dev_err(&dev->dev, "firmware size is not a multipe of 8.");
> 
> "multiple"

Yes will fix

> > +static int renesas_hw_check_run_stop_busy(struct pci_dev *pdev)
> > +{
> > +#if 0
> > +	u32 val;
> > +
> > +	/*
> > +	 * 7.1.3 Note 3: "... must not set 'FW Download Enable' when
> > +	 * 'RUN/STOP' of USBCMD Register is set"
> > +	 */
> > +	val = readl(hcd->regs + 0x20);
> > +	if (val & BIT(0)) {
> > +		dev_err(&pdev->dev, "hardware is busy and can't receive a FW.");
> > +		return -EBUSY;
> > +	}
> > +#endif
> > +	return 0;
> > +}
> > +
> 
> Is this function still really needed anymore?

Nope I will drop it unless Christian objects

> > +	/*
> > +	 * 11. After finishing writing the last data of FW, the
> > +	 * System Software must clear "FW Download Enable"
> > +	 */
> > +	err = pci_write_config_byte(pdev, 0xF4, 0);
> > +	if (err)
> > +		return pcibios_err_to_errno(err);
> > +
> > +	/* 12. Read "Result Code" and confirm it is good. */
> > +	for (i = 0; i < 10000; i++) {
> > +		err = pci_read_config_byte(pdev, 0xF4, &fw_status);
> > +		if (err)
> > +			return pcibios_err_to_errno(err);
> > +		if (fw_status & BIT(4))
> > +			break;
> > +
> > +		udelay(1);
> > +	}
> 
> 1000 reads???  I've heard of having to read a few times to ensure
> something "latched" in the device, but not 1000.  Why so many?

For ROM load it did need significant time, I will check if we can go down
to 100 here

> > +	if (i == 10000) {
> > +		/* Timed out / Error - let's see if we can fix this */
> > +		err = renesas_fw_check_running(pdev);
> > +		switch (err) {
> > +		case 0: /*
> > +			 * we shouldn't end up here.
> > +			 * maybe it took a little bit longer.
> > +			 * But all should be well?
> > +			 */
> > +			break;
> > +
> > +		case 1: /* (No result yet? - we can try to retry) */
> > +			if (retry_counter < 10) {
> > +				retry_counter++;
> > +				dev_warn(&pdev->dev, "Retry Firmware download: %d try.",
> > +					  retry_counter);
> > +				return renesas_fw_download(pdev, fw,
> > +							   retry_counter);
> 
> recursion?

I didnt encounter the need, we should remove it unless Christian objects

> > +static void renesas_fw_callback(const struct firmware *fw,
> > +				void *context)
> > +{
> > +	struct renesas_fw_ctx *ctx = context;
> > +	struct pci_dev *pdev = ctx->pdev;
> > +	struct device *parent = pdev->dev.parent;
> > +	int err = -ENOENT;
> > +
> > +	if (fw) {
> > +		err = renesas_fw_verify(pdev, fw->data, fw->size);
> > +		if (!err) {
> > +			err = renesas_fw_download(pdev, fw, 0);
> > +			release_firmware(fw);
> > +			if (!err) {
> > +				if (ctx->resume)
> > +					return;
> > +
> > +				err = xhci_pci_probe(pdev, ctx->id);
> > +				if (!err) {
> > +					/* everything worked */
> > +					devm_kfree(&pdev->dev, ctx);
> > +					return;
> > +				}
> > +
> > +				/* in case of an error - fall through */
> > +			} else {
> > +				dev_err(&pdev->dev, "firmware failed to download (%d).",
> > +					err);
> > +			}
> > +		}
> > +	} else {
> > +		dev_err(&pdev->dev, "firmware failed to load (%d).", err);
> > +	}
> 
> This can be cleaned up a lot by just erroring out when something
> happens, and then jumping to the end.  Not this nested if mess.

will do

> > +	err = renesas_fw_check_running(pdev);
> > +	/* Also go ahead, if the firmware is running */
> > +	if (err == 0)
> > +		return 0;
> > +
> > +	/* At this point, we can be sure that the FW isn't ready. */
> > +	return err;
> 
> Why not just:
> 	return renesas_fw_check_running(pdev);

sounds good, will change :)

-- 
~Vinod

Re: [PATCH 5/5] usb: xhci: allow multiple firmware versions

[Vinod Koul]

On 20-06-19, 14:20, Greg Kroah-Hartman wrote:
> On Thu, Jun 20, 2019 at 03:51:54PM +0530, Vinod Koul wrote:
> > Allow multiple firmware file versions in table and load them in
> > increasing order as we find them in the file system.
> > 
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > Cc: Christian Lamparter <chunkeey@googlemail.com>
> > ---
> >  drivers/usb/host/xhci-pci.c | 39 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 39 insertions(+)
> > 
> > diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> > index 3574afac44c5..2ee9e6bbabcb 100644
> > --- a/drivers/usb/host/xhci-pci.c
> > +++ b/drivers/usb/host/xhci-pci.c
> > @@ -333,9 +333,12 @@ static const struct renesas_fw_entry {
> >  	 *  - uPD720201 ES 2.0 sample whose revision ID is 2.
> >  	 *  - uPD720201 ES 2.1 sample & CS sample & Mass product, ID is 3.
> >  	 *  - uPD720202 ES 2.0 sample & CS sample & Mass product, ID is 2.
> > +	 *
> > +	 *  Entry expected_version should be kept in increasing order
> 
> Why?

I should have explained more :)
This is required only for the devices which have multiple versions
available, so it will pick one try it out and if not found go to
subsequent one. So I thought it would be make sense to keep the version
values for a specific device (not whole table) ordered so that we can
pick the next entry. Of course if you have better idea to manage two
versions, am all ears :)

> 
> >  	 */
> >  	{ "K2013080.mem", 0x0014, 0x02, 0x2013 },
> >  	{ "K2013080.mem", 0x0014, 0x03, 0x2013 },
> > +	{ "K2026090.mem", 0x0014, 0x03, 0x2026 },
> >  	{ "K2013080.mem", 0x0015, 0x02, 0x2013 },
> 
> Just put a { } entry here at the end and then you don't care about the
> order at all.  Much simpler and easier to maintain over time.
> 

-- 
~Vinod

Re: [PATCH 1/5] usb: xhci: add firmware loader for uPD720201 and uPD720202 w/o ROM

[Christian Lamparter]

On Thursday, June 20, 2019 7:03:58 PM CEST Vinod Koul wrote:
> On 20-06-19, 14:19, Greg Kroah-Hartman wrote:
> > On Thu, Jun 20, 2019 at 03:51:50PM +0530, Vinod Koul wrote:
> > > From: Christian Lamparter <chunkeey@googlemail.com>
> > > 
> > > This patch adds a firmware loader for the uPD720201K8-711-BAC-A
> > > and uPD720202K8-711-BAA-A variant. Both of these chips are listed
> > > in Renesas' R19UH0078EJ0500 Rev.5.00 "User's Manual: Hardware" as
> > > devices which need the firmware loader on page 2 in order to
> > > work as they "do not support the External ROM".
> > > 
> > > The "Firmware Download Sequence" is describe in chapter
> > > "7.1 FW Download Interface" R19UH0078EJ0500 Rev.5.00 page 131.
> > > 
> > > The firmware "K2013080.mem" is available from a USB3.0 Host to
> > > PCIe Adapter (PP2U-E card) "Firmware download" archive. An
> > > alternative version can be sourced from Netgear's WNDR4700 GPL
> > > archives.
> > > 
> > > The release notes of the PP2U-E's "Firmware Download" ver 2.0.1.3
> > > (2012-06-15) state that the firmware is for the following devices:
> > >  - uPD720201 ES 2.0 sample whose revision ID is 2.
> > >  - uPD720201 ES 2.1 sample & CS sample & Mass product, ID is 3.
> > >  - uPD720202 ES 2.0 sample & CS sample & Mass product, ID is 2.
> > > 
> > > If someone from Renesas is listening: It would be great, if these
> > > firmwares could be added to linux-firmware.git.
> > 
> > That paragraph does not need to be in the changelog :)
> 
> Sure will drop :)

... those this mean that there is a firmware now? Do you have a link to it?

> 
> > >  #include <linux/slab.h>
> > >  #include <linux/module.h>
> > >  #include <linux/acpi.h>
> > > +#include <linux/firmware.h>
> > > +#include <asm/unaligned.h>
> > 
> > asm/ in a driver?  Are you sure???
> 
> Not sure :D, will check and remove

I think, as long as there is a "get_unaligned_le16" defined somewhere
it should be fine.

This was a loong ago, the loader was developped on a PowerPC 464, but
from what I remember it was checkpatch that didn't like the "unaligned"
poking around in the firmware below.

> > > +static int renesas_fw_download_image(struct pci_dev *dev,
> > > +				     const u32 *fw,
> > > +				     size_t step)
> > > +{
> > > +	size_t i;
> > > +	int err;
> > > +	u8 fw_status;
> > > +	bool data0_or_data1;
> > > +
> > > +	/*
> > > +	 * The hardware does alternate between two 32-bit pages.
> > > +	 * (This is because each row of the firmware is 8 bytes).
> > > +	 *
> > > +	 * for even steps we use DATA0, for odd steps DATA1.
> > > +	 */
> > > +	data0_or_data1 = (step & 1) == 1;
> > > +
> > > +	/* step+1. Read "Set DATAX" and confirm it is cleared. */
> > > +	for (i = 0; i < 10000; i++) {
> > > +		err = pci_read_config_byte(dev, 0xF5, &fw_status);
> > > +		if (err)
> > > +			return pcibios_err_to_errno(err);
> > > +		if (!(fw_status & BIT(data0_or_data1)))
> > > +			break;
> > > +
> > > +		udelay(1);
> > > +	}
> > > +	if (i == 10000)
> > > +		return -ETIMEDOUT;
> > > +
> > > +	/*
> > > +	 * step+2. Write FW data to "DATAX".
> > > +	 * "LSB is left" => force little endian
> > > +	 */
> > > +	err = pci_write_config_dword(dev, data0_or_data1 ? 0xFC : 0xF8,
> > > +				     (__force u32) cpu_to_le32(fw[step]));
> > > +	if (err)
> > > +		return pcibios_err_to_errno(err);
> > > +
> > > +	udelay(100);
> > > +
> > > +	/* step+3. Set "Set DATAX". */
> > > +	err = pci_write_config_byte(dev, 0xF5, BIT(data0_or_data1));
> > > +	if (err)
> > > +		return pcibios_err_to_errno(err);
> > > +
> > 
> > Shouldn't you just do a read after the write to be sure the write
> > actually went out on the wire?  Then you shouldn't have to do the
> > udelay, right?
> 
> Well I am not sure that is how it works. The register is a DATA register
> on the controller. We are writing to the memory of the controller here
> and after writing DATA0 and DATA1 we check the Set DATA0 & Set DATA1
> bits and write subsequenly only when controller is ready to accept more
> data.
> 
> I do recall at least for ROM load (writing to NOR flash attached to
> controller), we need to wait considerably more before the SetData0/1 was
> set and ready for subsequent write

OffTopic: There's some leeway here. From what I remember you could just push
the data through DATA0 and cut down on the logic. But this was slower than
using both DATA0 and DATA1.

The udelay was placed because I vaguely remember that polling SET DATA0
over and over slowed down the firmware download.
So the intention was to have the 100µs as a baseline and then we don't
slow down and waste more cycles in "step+1".

> 
> > > +static int renesas_hw_check_run_stop_busy(struct pci_dev *pdev)
> > > +{
> > > +#if 0
> > > +	u32 val;
> > > +
> > > +	/*
> > > +	 * 7.1.3 Note 3: "... must not set 'FW Download Enable' when
> > > +	 * 'RUN/STOP' of USBCMD Register is set"
> > > +	 */
> > > +	val = readl(hcd->regs + 0x20);
> > > +	if (val & BIT(0)) {
> > > +		dev_err(&pdev->dev, "hardware is busy and can't receive a FW.");
> > > +		return -EBUSY;
> > > +	}
> > > +#endif
> > > +	return 0;
> > > +}
> > > +
> > 
> > Is this function still really needed anymore?
> 
> Nope I will drop it unless Christian objects

You can drop it. From what I remember it was used for a minimal backup
solution that would simply prevent stuck the xhci-pci modules. (never
heard from Greg or Filipe)

> > > +	/*
> > > +	 * 11. After finishing writing the last data of FW, the
> > > +	 * System Software must clear "FW Download Enable"
> > > +	 */
> > > +	err = pci_write_config_byte(pdev, 0xF4, 0);
> > > +	if (err)
> > > +		return pcibios_err_to_errno(err);
> > > +
> > > +	/* 12. Read "Result Code" and confirm it is good. */
> > > +	for (i = 0; i < 10000; i++) {
> > > +		err = pci_read_config_byte(pdev, 0xF4, &fw_status);
> > > +		if (err)
> > > +			return pcibios_err_to_errno(err);
> > > +		if (fw_status & BIT(4))
> > > +			break;
> > > +
> > > +		udelay(1);
> > > +	}
> > 
> > 1000 reads???  I've heard of having to read a few times to ensure
> > something "latched" in the device, but not 1000.  Why so many?
> 
> For ROM load it did need significant time, I will check if we can go down
> to 100 here
yes, it takes a while! Though you could use a bigger udelay here and do
less retries. 

> > > +	if (i == 10000) {
> > > +		/* Timed out / Error - let's see if we can fix this */
> > > +		err = renesas_fw_check_running(pdev);
> > > +		switch (err) {
> > > +		case 0: /*
> > > +			 * we shouldn't end up here.
> > > +			 * maybe it took a little bit longer.
> > > +			 * But all should be well?
> > > +			 */
> > > +			break;
> > > +
> > > +		case 1: /* (No result yet? - we can try to retry) */
> > > +			if (retry_counter < 10) {
> > > +				retry_counter++;
> > > +				dev_warn(&pdev->dev, "Retry Firmware download: %d try.",
> > > +					  retry_counter);
> > > +				return renesas_fw_download(pdev, fw,
> > > +							   retry_counter);
> > 
> > recursion?
> 
> I didnt encounter the need, we should remove it unless Christian objects

Sure, I think it should be safe to just say that there was a timeout and then abort.

Cheers,
Christian

Re: [PATCH 1/5] usb: xhci: add firmware loader for uPD720201 and uPD720202 w/o ROM

[Vinod Koul]

On 20-06-19, 21:12, Christian Lamparter wrote:
> On Thursday, June 20, 2019 7:03:58 PM CEST Vinod Koul wrote:
> > On 20-06-19, 14:19, Greg Kroah-Hartman wrote:
> > > On Thu, Jun 20, 2019 at 03:51:50PM +0530, Vinod Koul wrote:
> > > > From: Christian Lamparter <chunkeey@googlemail.com>
> > > > 
> > > > This patch adds a firmware loader for the uPD720201K8-711-BAC-A
> > > > and uPD720202K8-711-BAA-A variant. Both of these chips are listed
> > > > in Renesas' R19UH0078EJ0500 Rev.5.00 "User's Manual: Hardware" as
> > > > devices which need the firmware loader on page 2 in order to
> > > > work as they "do not support the External ROM".
> > > > 
> > > > The "Firmware Download Sequence" is describe in chapter
> > > > "7.1 FW Download Interface" R19UH0078EJ0500 Rev.5.00 page 131.
> > > > 
> > > > The firmware "K2013080.mem" is available from a USB3.0 Host to
> > > > PCIe Adapter (PP2U-E card) "Firmware download" archive. An
> > > > alternative version can be sourced from Netgear's WNDR4700 GPL
> > > > archives.
> > > > 
> > > > The release notes of the PP2U-E's "Firmware Download" ver 2.0.1.3
> > > > (2012-06-15) state that the firmware is for the following devices:
> > > >  - uPD720201 ES 2.0 sample whose revision ID is 2.
> > > >  - uPD720201 ES 2.1 sample & CS sample & Mass product, ID is 3.
> > > >  - uPD720202 ES 2.0 sample & CS sample & Mass product, ID is 2.
> > > > 
> > > > If someone from Renesas is listening: It would be great, if these
> > > > firmwares could be added to linux-firmware.git.
> > > 
> > > That paragraph does not need to be in the changelog :)
> > 
> > Sure will drop :)
> 
> ... those this mean that there is a firmware now? Do you have a link to it?

Unfortunately it is not public yet!

> > > >  #include <linux/slab.h>
> > > >  #include <linux/module.h>
> > > >  #include <linux/acpi.h>
> > > > +#include <linux/firmware.h>
> > > > +#include <asm/unaligned.h>
> > > 
> > > asm/ in a driver?  Are you sure???
> > 
> > Not sure :D, will check and remove
> 
> I think, as long as there is a "get_unaligned_le16" defined somewhere
> it should be fine.
> 
> This was a loong ago, the loader was developped on a PowerPC 464, but
> from what I remember it was checkpatch that didn't like the "unaligned"
> poking around in the firmware below.

It seems we have this in include/linux/unaligned/access_ok.h, so I will
add that instead

> > > > +static int renesas_fw_download_image(struct pci_dev *dev,
> > > > +				     const u32 *fw,
> > > > +				     size_t step)
> > > > +{
> > > > +	size_t i;
> > > > +	int err;
> > > > +	u8 fw_status;
> > > > +	bool data0_or_data1;
> > > > +
> > > > +	/*
> > > > +	 * The hardware does alternate between two 32-bit pages.
> > > > +	 * (This is because each row of the firmware is 8 bytes).
> > > > +	 *
> > > > +	 * for even steps we use DATA0, for odd steps DATA1.
> > > > +	 */
> > > > +	data0_or_data1 = (step & 1) == 1;
> > > > +
> > > > +	/* step+1. Read "Set DATAX" and confirm it is cleared. */
> > > > +	for (i = 0; i < 10000; i++) {
> > > > +		err = pci_read_config_byte(dev, 0xF5, &fw_status);
> > > > +		if (err)
> > > > +			return pcibios_err_to_errno(err);
> > > > +		if (!(fw_status & BIT(data0_or_data1)))
> > > > +			break;
> > > > +
> > > > +		udelay(1);
> > > > +	}
> > > > +	if (i == 10000)
> > > > +		return -ETIMEDOUT;
> > > > +
> > > > +	/*
> > > > +	 * step+2. Write FW data to "DATAX".
> > > > +	 * "LSB is left" => force little endian
> > > > +	 */
> > > > +	err = pci_write_config_dword(dev, data0_or_data1 ? 0xFC : 0xF8,
> > > > +				     (__force u32) cpu_to_le32(fw[step]));
> > > > +	if (err)
> > > > +		return pcibios_err_to_errno(err);
> > > > +
> > > > +	udelay(100);
> > > > +
> > > > +	/* step+3. Set "Set DATAX". */
> > > > +	err = pci_write_config_byte(dev, 0xF5, BIT(data0_or_data1));
> > > > +	if (err)
> > > > +		return pcibios_err_to_errno(err);
> > > > +
> > > 
> > > Shouldn't you just do a read after the write to be sure the write
> > > actually went out on the wire?  Then you shouldn't have to do the
> > > udelay, right?
> > 
> > Well I am not sure that is how it works. The register is a DATA register
> > on the controller. We are writing to the memory of the controller here
> > and after writing DATA0 and DATA1 we check the Set DATA0 & Set DATA1
> > bits and write subsequenly only when controller is ready to accept more
> > data.
> > 
> > I do recall at least for ROM load (writing to NOR flash attached to
> > controller), we need to wait considerably more before the SetData0/1 was
> > set and ready for subsequent write
> 
> OffTopic: There's some leeway here. From what I remember you could just push
> the data through DATA0 and cut down on the logic. But this was slower than
> using both DATA0 and DATA1.
> 
> The udelay was placed because I vaguely remember that polling SET DATA0
> over and over slowed down the firmware download.
> So the intention was to have the 100µs as a baseline and then we don't
> slow down and waste more cycles in "step+1".

Yes I have seen that as well, lesser polling helps. I will reduce cycles
and add delay.

Btw would it be possible for you to test the series?

Thanks for quick reply, will post v2 shortly

-- 
~Vinod