| Message ID | 20190620102154.20805-2-vkoul@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: xhci: Add support for Renesas USB controllers | expand |
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
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 :)
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
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
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;