| Message ID | 20200113084005.849071-1-vkoul@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | usb: xhci: Add support for Renesas USB controllers | expand |
On Mon, Jan 13, 2020 at 12:42 AM Vinod Koul <vkoul@kernel.org> wrote: > > 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 programmed, 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, > debugfs hook for rom erase and export of xhci-pci functions. > Thanks so much for updating these! They are working ok for me in my testing on db845c. Tested-by: John Stultz <john.stultz@linaro.org> thanks again! -john
On Mon, Jan 13, 2020 at 9:10 PM John Stultz <john.stultz@linaro.org> wrote: > > On Mon, Jan 13, 2020 at 12:42 AM Vinod Koul <vkoul@kernel.org> wrote: > > > > 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 programmed, 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, > > debugfs hook for rom erase and export of xhci-pci functions. > > > > Thanks so much for updating these! They are working ok for me in my > testing on db845c. > > Tested-by: John Stultz <john.stultz@linaro.org> Nice! I'll definitely give this series another try on my WNDR4700 too (PowerPC Arch) this weekend. and from me: Thanks!
hey Christian, On 13-01-20, 21:33, Christian Lamparter wrote: > On Mon, Jan 13, 2020 at 9:10 PM John Stultz <john.stultz@linaro.org> wrote: > > > > On Mon, Jan 13, 2020 at 12:42 AM Vinod Koul <vkoul@kernel.org> wrote: > > > > > > 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 programmed, 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, > > > debugfs hook for rom erase and export of xhci-pci functions. > > > > > > > Thanks so much for updating these! They are working ok for me in my > > testing on db845c. > > > > Tested-by: John Stultz <john.stultz@linaro.org> > > Nice! I'll definitely give this series another try on my WNDR4700 too > (PowerPC Arch) > this weekend. > > and from me: Thanks! Did you get around to test these?
Hello, On Tue, Jan 21, 2020 at 7:46 AM Vinod Koul <vkoul@kernel.org> wrote: > > hey Christian, > > On 13-01-20, 21:33, Christian Lamparter wrote: > > On Mon, Jan 13, 2020 at 9:10 PM John Stultz <john.stultz@linaro.org> wrote: > > > > > > On Mon, Jan 13, 2020 at 12:42 AM Vinod Koul <vkoul@kernel.org> wrote: > > > > > > > > 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 programmed, 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, > > > > debugfs hook for rom erase and export of xhci-pci functions. > > > > > > > > > > Thanks so much for updating these! They are working ok for me in my > > > testing on db845c. > > > > > > Tested-by: John Stultz <john.stultz@linaro.org> > > > > Nice! I'll definitely give this series another try on my WNDR4700 too > > (PowerPC Arch) > > this weekend. > > > > and from me: Thanks! > > Did you get around to test these? Not yet, I was too optimistic that I could get current linux-usb with the patches running on the WNDR4700 (due to APM82181) over the weekend. Do you think that It still counts, if I'm going with 5.4.11 on OpenWrt instead? Because then I just swap out the old patches from my OpenWrt APM821XX branch: <https://git.openwrt.org/?p=openwrt/staging/chunkeey.git;a=commit;h=4dd6f62a36a3724f0363d639cd9e29e04d7b62c0> and don't have to figure out what broke with linux-usb on the APM821xx. Cheers, Christian
On Tuesday, 21 January 2020 21:26:34 CET Christian Lamparter wrote: > Hello, > > On Tue, Jan 21, 2020 at 7:46 AM Vinod Koul <vkoul@kernel.org> wrote: > > > > hey Christian, > > > > On 13-01-20, 21:33, Christian Lamparter wrote: > > > On Mon, Jan 13, 2020 at 9:10 PM John Stultz <john.stultz@linaro.org> wrote: > > > > > > > > On Mon, Jan 13, 2020 at 12:42 AM Vinod Koul <vkoul@kernel.org> wrote: > > > > > > > > > > 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 programmed, 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, > > > > > debugfs hook for rom erase and export of xhci-pci functions. > > > > > > > > > > > > > Thanks so much for updating these! They are working ok for me in my > > > > testing on db845c. > > > > > > > > Tested-by: John Stultz <john.stultz@linaro.org> > > > > > > Nice! I'll definitely give this series another try on my WNDR4700 too > > > (PowerPC Arch) > > > this weekend. > > > > > > and from me: Thanks! > > > > Did you get around to test these? > > Not yet, I was too optimistic that I could get current linux-usb with the > patches running on the WNDR4700 (due to APM82181) over the > weekend. Do you think that It still counts, if I'm going with 5.4.11 on > OpenWrt instead? Because then I just swap out the old patches from > my OpenWrt APM821XX branch: > <https://git.openwrt.org/?p=openwrt/staging/chunkeey.git;a=commit;h=4dd6f62a36a3724f0363d639cd9e29e04d7b62c0> > > and don't have to figure out what broke with linux-usb on the APM821xx. I could get 5.4.11 to boot on the Netgear WNDR4700 :-). (This has a APM82181 SoC (PowerPC 464)) Here's faillog from the "plain xhci-pci" driver: [ 375.481868] xhci_hcd 0000:45:00.0: xHCI Host Controller [ 375.487149] xhci_hcd 0000:45:00.0: new USB bus registered, assigned bus number 1 [ 385.494590] xhci_hcd 0000:45:00.0: can't setup: -110 [ 385.499558] xhci_hcd 0000:45:00.0: USB bus 1 deregistered [ 385.504963] xhci_hcd 0000:45:00.0: init 0000:45:00.0 fail, -110 [ 385.510889] xhci_hcd: probe of 0000:45:00.0 failed with error -110 (Notice how it gets stuck for 10 seconds there). And this is the successlog from the xhci-pci-renesas module [ 391.555559] renesas xhci 0000:45:00.0: xHCI Host Controller [ 391.561171] renesas xhci 0000:45:00.0: new USB bus registered, assigned bus number 1 [ 391.575068] renesas xhci 0000:45:00.0: hcc params 0x014051cf hci version 0x100 quirks 0x0000000101000090 [ 391.586750] hub 1-0:1.0: USB hub found [ 391.592601] hub 1-0:1.0: 2 ports detected [ 391.597199] renesas xhci 0000:45:00.0: xHCI Host Controller [ 391.602797] renesas xhci 0000:45:00.0: new USB bus registered, assigned bus number 2 [ 391.610537] renesas xhci 0000:45:00.0: Host supports USB 3.0 SuperSpeed [ 391.617719] usb usb2: We don't know the algorithms for LPM for this host, disabling LPM. [ 391.626495] hub 2-0:1.0: USB hub found [ 391.630570] hub 2-0:1.0: 2 ports detected this is when I added the usb 3.0-stick: [ 775.403928] usb 2-2: new SuperSpeed Gen 1 USB device number 3 using renesas xhci [ 775.432684] usb-storage 2-2:1.0: USB Mass Storage device detected [ 775.439238] scsi host1: usb-storage 2-2:1.0 [ 776.482556] scsi 1:0:0:0: Direct-Access SanDisk Ultra 1.00 PQ: 0 ANSI: 6 [ 776.492181] sd 1:0:0:0: [sda] 60063744 512-byte logical blocks: (30.8 GB/28.6 GiB) [ 776.501193] sd 1:0:0:0: [sda] Write Protect is off [ 776.507047] sd 1:0:0:0: [sda] Write cache: disabled, read cache: enabled, doesn't support DPO or FUA [ 776.524893] sda: sda1 sda2 [ 776.531062] sd 1:0:0:0: [sda] Attached SCSI removable disk root@(none):/dev# hdparm -t /dev/sda /dev/sda: Timing buffered disk reads: 466 MB in 3.01 seconds = 154.98 MB/sec and this is the log from my usb 2.0-memorystick: [ 1187.113650] usb 2-2: USB disconnect, device number 3 [ 1195.867397] usb 1-2: new high-speed USB device number 2 using renesas xhci [ 1195.895171] usb-storage 1-2:1.0: USB Mass Storage device detected [ 1195.901848] scsi host1: usb-storage 1-2:1.0 [ 1196.962583] scsi 1:0:0:0: Direct-Access SanDisk Cruzer Blade 1.00 PQ: 0 ANSI: 6 [ 1196.978772] sd 1:0:0:0: [sda] 30031872 512-byte logical blocks: (15.4 GB/14.3 GiB) [ 1196.988529] sd 1:0:0:0: [sda] Write Protect is off [ 1196.994498] sd 1:0:0:0: [sda] Write cache: disabled, read cache: enabled, doesn't support DPO or FUA [ 1197.020407] sda: sda1 [ 1197.030458] sd 1:0:0:0: [sda] Attached SCSI removable disk root@(none):/dev# hdparm -t /dev/sda /dev/sda: Timing buffered disk reads: 64 MB in 3.01 seconds = 21.28 MB/sec These speeds for usb3 and usb2 are within what the device can do. So, everything is working fine with the v6. Tested-by: Christian Lamparter <chunkeey@gmail.com> Cheers, Christian
On 24-01-20, 22:38, Christian Lamparter wrote: > On Tuesday, 21 January 2020 21:26:34 CET Christian Lamparter wrote: > > Hello, > > > > On Tue, Jan 21, 2020 at 7:46 AM Vinod Koul <vkoul@kernel.org> wrote: > > > > > > hey Christian, > > > > > > On 13-01-20, 21:33, Christian Lamparter wrote: > > > > On Mon, Jan 13, 2020 at 9:10 PM John Stultz <john.stultz@linaro.org> wrote: > > > > > > > > > > On Mon, Jan 13, 2020 at 12:42 AM Vinod Koul <vkoul@kernel.org> wrote: > > > > > > > > > > > > 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 programmed, 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, > > > > > > debugfs hook for rom erase and export of xhci-pci functions. > > > > > > > > > > > > > > > > Thanks so much for updating these! They are working ok for me in my > > > > > testing on db845c. > > > > > > > > > > Tested-by: John Stultz <john.stultz@linaro.org> > > > > > > > > Nice! I'll definitely give this series another try on my WNDR4700 too > > > > (PowerPC Arch) > > > > this weekend. > > > > > > > > and from me: Thanks! > > > > > > Did you get around to test these? > > > > Not yet, I was too optimistic that I could get current linux-usb with the > > patches running on the WNDR4700 (due to APM82181) over the > > weekend. Do you think that It still counts, if I'm going with 5.4.11 on > > OpenWrt instead? Because then I just swap out the old patches from > > my OpenWrt APM821XX branch: > > <https://git.openwrt.org/?p=openwrt/staging/chunkeey.git;a=commit;h=4dd6f62a36a3724f0363d639cd9e29e04d7b62c0> > > > > and don't have to figure out what broke with linux-usb on the APM821xx. > > I could get 5.4.11 to boot on the Netgear WNDR4700 :-). > (This has a APM82181 SoC (PowerPC 464)) > > Here's faillog from the "plain xhci-pci" driver: > > [ 375.481868] xhci_hcd 0000:45:00.0: xHCI Host Controller > [ 375.487149] xhci_hcd 0000:45:00.0: new USB bus registered, assigned bus number 1 > [ 385.494590] xhci_hcd 0000:45:00.0: can't setup: -110 > [ 385.499558] xhci_hcd 0000:45:00.0: USB bus 1 deregistered > [ 385.504963] xhci_hcd 0000:45:00.0: init 0000:45:00.0 fail, -110 > [ 385.510889] xhci_hcd: probe of 0000:45:00.0 failed with error -110 > > (Notice how it gets stuck for 10 seconds there). > > And this is the successlog from the xhci-pci-renesas module > > [ 391.555559] renesas xhci 0000:45:00.0: xHCI Host Controller > [ 391.561171] renesas xhci 0000:45:00.0: new USB bus registered, assigned bus number 1 > [ 391.575068] renesas xhci 0000:45:00.0: hcc params 0x014051cf hci version 0x100 quirks 0x0000000101000090 > [ 391.586750] hub 1-0:1.0: USB hub found > [ 391.592601] hub 1-0:1.0: 2 ports detected > [ 391.597199] renesas xhci 0000:45:00.0: xHCI Host Controller > [ 391.602797] renesas xhci 0000:45:00.0: new USB bus registered, assigned bus number 2 > [ 391.610537] renesas xhci 0000:45:00.0: Host supports USB 3.0 SuperSpeed > [ 391.617719] usb usb2: We don't know the algorithms for LPM for this host, disabling LPM. > [ 391.626495] hub 2-0:1.0: USB hub found > [ 391.630570] hub 2-0:1.0: 2 ports detected > > this is when I added the usb 3.0-stick: > > [ 775.403928] usb 2-2: new SuperSpeed Gen 1 USB device number 3 using renesas xhci > [ 775.432684] usb-storage 2-2:1.0: USB Mass Storage device detected > [ 775.439238] scsi host1: usb-storage 2-2:1.0 > [ 776.482556] scsi 1:0:0:0: Direct-Access SanDisk Ultra 1.00 PQ: 0 ANSI: 6 > [ 776.492181] sd 1:0:0:0: [sda] 60063744 512-byte logical blocks: (30.8 GB/28.6 GiB) > [ 776.501193] sd 1:0:0:0: [sda] Write Protect is off > [ 776.507047] sd 1:0:0:0: [sda] Write cache: disabled, read cache: enabled, doesn't support DPO or FUA > [ 776.524893] sda: sda1 sda2 > [ 776.531062] sd 1:0:0:0: [sda] Attached SCSI removable disk > > root@(none):/dev# hdparm -t /dev/sda > > /dev/sda: > Timing buffered disk reads: 466 MB in 3.01 seconds = 154.98 MB/sec > > and this is the log from my usb 2.0-memorystick: > > [ 1187.113650] usb 2-2: USB disconnect, device number 3 > [ 1195.867397] usb 1-2: new high-speed USB device number 2 using renesas xhci > [ 1195.895171] usb-storage 1-2:1.0: USB Mass Storage device detected > [ 1195.901848] scsi host1: usb-storage 1-2:1.0 > [ 1196.962583] scsi 1:0:0:0: Direct-Access SanDisk Cruzer Blade 1.00 PQ: 0 ANSI: 6 > [ 1196.978772] sd 1:0:0:0: [sda] 30031872 512-byte logical blocks: (15.4 GB/14.3 GiB) > [ 1196.988529] sd 1:0:0:0: [sda] Write Protect is off > [ 1196.994498] sd 1:0:0:0: [sda] Write cache: disabled, read cache: enabled, doesn't support DPO or FUA > [ 1197.020407] sda: sda1 > [ 1197.030458] sd 1:0:0:0: [sda] Attached SCSI removable disk > > root@(none):/dev# hdparm -t /dev/sda > > /dev/sda: > Timing buffered disk reads: 64 MB in 3.01 seconds = 21.28 MB/sec > > These speeds for usb3 and usb2 are within what the device can do. > So, everything is working fine with the v6. > > Tested-by: Christian Lamparter <chunkeey@gmail.com> Thanks a lot Christian for again testing this. Mathias, any comments on this series..?
On 13/01/2020 09:40, Vinod Koul wrote: > 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 programmed, 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, > debugfs hook for rom erase and export of xhci-pci functions. > I tested this on an AVM FRITZ!Box 3490, backported to 4.19: Firmware upload works fine. However, I'm seeing read errors afterwards which, I suppose, are a different story. Here is the log: [ 498.115808] ifx_pcie_bios_map_irq port 0 dev 0000:01:00.0 slot 0 pin 1 [ 498.121154] ifx_pcie_bios_map_irq dev 0000:01:00.0 irq 144 assigned [ 498.488541] renesas xhci 0000:01:00.0: xHCI Host Controller [ 498.492820] renesas xhci 0000:01:00.0: new USB bus registered, assigned bus number 1 [ 498.506123] renesas xhci 0000:01:00.0: hcc params 0x014051cf hci version 0x100 quirks 0x0000000101000090 [ 498.516869] hub 1-0:1.0: USB hub found [ 498.519631] hub 1-0:1.0: 2 ports detected [ 498.525641] renesas xhci 0000:01:00.0: xHCI Host Controller [ 498.530217] renesas xhci 0000:01:00.0: new USB bus registered, assigned bus number 2 [ 498.537846] renesas xhci 0000:01:00.0: Host supports USB 3.0 SuperSpeed [ 498.545095] usb usb2: We don't know the algorithms for LPM for this host, disabling LPM. [ 498.554921] hub 2-0:1.0: USB hub found [ 498.557588] hub 2-0:1.0: 2 ports detected [ 523.013361] usb 1-1: new full-speed USB device number 2 using renesas xhci [ 523.182725] usb 1-1: no configurations [ 523.185085] usb 1-1: can't read configurations, error -22 [ 523.317423] usb 1-1: new full-speed USB device number 3 using renesas xhci [ 523.493710] usb 1-1: no configurations [ 523.496069] usb 1-1: can't read configurations, error -22 [ 523.501951] usb usb1-port1: attempt power cycle Regards and thanks for the patch, Andreas
On Sunday, 26 January 2020 01:11:35 CET Andreas Böhler wrote: > > On 13/01/2020 09:40, Vinod Koul wrote: > > 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 programmed, 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, > > debugfs hook for rom erase and export of xhci-pci functions. > > > I tested this on an AVM FRITZ!Box 3490, backported to 4.19: Firmware > upload works fine. > > However, I'm seeing read errors afterwards which, I suppose, are a > different story. > > Here is the log: > > [ 498.115808] ifx_pcie_bios_map_irq port 0 dev 0000:01:00.0 slot 0 pin 1 > [ 498.121154] ifx_pcie_bios_map_irq dev 0000:01:00.0 irq 144 assigned > [ 498.488541] renesas xhci 0000:01:00.0: xHCI Host Controller > [ 498.492820] renesas xhci 0000:01:00.0: new USB bus registered, > assigned bus number 1 > [ 498.506123] renesas xhci 0000:01:00.0: hcc params 0x014051cf hci > version 0x100 quirks 0x0000000101000090 > [ 498.516869] hub 1-0:1.0: USB hub found > [ 498.519631] hub 1-0:1.0: 2 ports detected > [ 498.525641] renesas xhci 0000:01:00.0: xHCI Host Controller > [ 498.530217] renesas xhci 0000:01:00.0: new USB bus registered, > assigned bus number 2 > [ 498.537846] renesas xhci 0000:01:00.0: Host supports USB 3.0 SuperSpeed > [ 498.545095] usb usb2: We don't know the algorithms for LPM for this > host, disabling LPM. > [ 498.554921] hub 2-0:1.0: USB hub found > [ 498.557588] hub 2-0:1.0: 2 ports detected > [ 523.013361] usb 1-1: new full-speed USB device number 2 using renesas > xhci > [ 523.182725] usb 1-1: no configurations > [ 523.185085] usb 1-1: can't read configurations, error -22 > [ 523.317423] usb 1-1: new full-speed USB device number 3 using renesas > xhci > [ 523.493710] usb 1-1: no configurations > [ 523.496069] usb 1-1: can't read configurations, error -22 > [ 523.501951] usb usb1-port1: attempt power cycle > Hm, I don't think lantiq's PCI code is upstream... And now that I've seen more errors from your forum post at: <https://forum.openwrt.org/t/fix-xhci-errors-on-renesas-upd70202-fritz-box-3490/53620> I wonder if this has something to do with a similar issue I was facing with the ath9k chip loader in commit: 5a4f2040fd07 ("ath9k: add loader for AR92XX (and older) pci(e)") which later needed a fix for a specifc lantiq byteswap problem in commit: 22d0d5ae7a08 ("ath9k: use iowrite32 over __raw_writel"): | This patch changes the ath9k_pci_owl_loader to use the | same iowrite32 memory accessor that ath9k_pci is using | to communicate with the PCI(e) chip. | | This will fix endian issues that came up during testing | with loaned AVM Fritz!Box 7360 (Lantiq MIPS SoCs + AR9287). The reason was that apparently (I gave back the loaned device), the lantiq PCI silicon does some sneaky byteswaps in special cases. Could this be related? You mentioned in another post that AVM did changes to the xhci driver, can you look if they added changes to the memory accessors? Because this would explain why the APM82181 (PowerPC which is also a BigEndian) had no issues (as it's using a entirely different pcie hardware and code). Cheers, Christian
On 25.1.2020 7.32, Vinod Koul wrote: >>>>>> >>>>>> On Mon, Jan 13, 2020 at 12:42 AM Vinod Koul <vkoul@kernel.org> wrote: >>>>>>> >>>>>>> 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 programmed, 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, >>>>>>> debugfs hook for rom erase and export of xhci-pci functions. >>>>>>> ... > > Mathias, any comments on this series..? > Hi Vinod Sorry about the delay. Maybe a firmware loading driver like this that wraps the xhci pci driver could work. One benefit is that we could skip searching for the right firmware name based on PCI ID. Each of these Renesas controllers now have their own pci_device_id entry in the pci_ids[] table, and could have the supported firmware name(s) in .driver_data. This way we wouldn't need to add the renesas_fw_table[] or maybe even the renesas_needs_fw_dl() function in this series. I realize this can't be easily changed because usb_hcd_pci_probe() takes the pci_device_id pointer as an argument, and expects id.driver_data to be a HC driver pointer. So this turns out to be a question for Greg and Alan: Would it make sense to change usb_hcd_pci_probe() to take a HC driver pointer as an argument instead of a pointer to pci_device_id? pci_device_id pointer is only used to extract the HC driver handle. This way the driver_data could be used for, well, driver data. Heikki actually suggested this some time ago to me, back then the idea was to improve xhci quirks code by using driver_data for quirk flags instead of finding and setting them later. There are a few other opens regarding this series. Mostly because I'm not (yet) familiar with all the details, so I'll just just list them here. - Is it really enough to add the Renesas driver to Makefile before xhci-pci driver to make sure it gets matched and probed based on vendor/device id before xhci-pci driver is matched and probed based on pci class? What if the Renesas driver is a module and xhci-pci compiled in? - Previously probe didn't return before hcd's were added and everything set up. Now with request_firmware_nowait() probe returns early successfully, and the old xhci_pci_probe() which sets up everything is called later by the request firmware callback. So there could be whole new set of races possible. For example pci remove can be called mid firmware loading, or when the old xhci_pci_probe is still setting up things. I understood that a synchronous request_firmware() in probe has its own issues, not sure if there is a good solution for this. - Before the firmware is written to the controller the firmware version is compared against a hardcoded number in the drivers renesas_fw_table[]. This means new firmware versions can't be supported without patching the driver. Is this intentional? - Mathias
Hi Mathias, On 30-01-20, 19:07, Mathias Nyman wrote: > On 25.1.2020 7.32, Vinod Koul wrote: > > > > > > > > > > > > > > On Mon, Jan 13, 2020 at 12:42 AM Vinod Koul <vkoul@kernel.org> wrote: > > > > > > > > > > > > > > > > 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 programmed, 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, > > > > > > > > debugfs hook for rom erase and export of xhci-pci functions. > > > > > > > > > ... > > > > Mathias, any comments on this series..? > > > > Hi Vinod > > Sorry about the delay. > > Maybe a firmware loading driver like this that wraps the xhci pci driver could > work. > > One benefit is that we could skip searching for the right firmware name based > on PCI ID. Each of these Renesas controllers now have their own pci_device_id > entry in the pci_ids[] table, and could have the supported firmware name(s) > in .driver_data. This way we wouldn't need to add the renesas_fw_table[] or > maybe even the renesas_needs_fw_dl() function in this series. > > I realize this can't be easily changed because usb_hcd_pci_probe() takes the > pci_device_id pointer as an argument, and expects id.driver_data to be a > HC driver pointer. > > So this turns out to be a question for Greg and Alan: > > Would it make sense to change usb_hcd_pci_probe() to take a HC driver pointer > as an argument instead of a pointer to pci_device_id? > pci_device_id pointer is only used to extract the HC driver handle. > This way the driver_data could be used for, well, driver data. > > Heikki actually suggested this some time ago to me, back then the idea was to > improve xhci quirks code by using driver_data for quirk flags instead of > finding and setting them later. > > There are a few other opens regarding this series. Mostly because I'm not (yet) > familiar with all the details, so I'll just just list them here. > > - Is it really enough to add the Renesas driver to Makefile before xhci-pci > driver to make sure it gets matched and probed based on vendor/device id > before xhci-pci driver is matched and probed based on pci class? > What if the Renesas driver is a module and xhci-pci compiled in? The driver loading rules are based on level and makefile order. So renesas will always be loaded before xhci driver. This is required since xhci claims all devices, so we need to make sure it loads before. I think we should make it inbuilt driver rather than a module. xhci driver is only inbuilt. If there is a better way to handle this, am all for it. > - Previously probe didn't return before hcd's were added and everything set up. > Now with request_firmware_nowait() probe returns early successfully, and the > old xhci_pci_probe() which sets up everything is called later by the request > firmware callback. So there could be whole new set of races possible. > For example pci remove can be called mid firmware loading, or when the old > xhci_pci_probe is still setting up things. I think this is a valid concern. Let me think about how to solve this.. maybe add a flag in remove which ensure remove doesnt run untill the probe/firmware callback is completed. > I understood that a synchronous request_firmware() in probe has its own > issues, not sure if there is a good solution for this. Yeah with userspace not available, that can be tricky! > - Before the firmware is written to the controller the firmware version is > compared against a hardcoded number in the drivers renesas_fw_table[]. > This means new firmware versions can't be supported without patching the driver. > Is this intentional? Yes, we have only bunch of validated versions. There maybe more in the wild but we dont know. The vendor is not very helpful here :( Across all folks using this, there seems to be only few versions and vendor is not supporting it anymore, so chances of new versions seems remote Thanks
On Thu, 30 Jan 2020, Mathias Nyman wrote: > I realize this can't be easily changed because usb_hcd_pci_probe() takes the > pci_device_id pointer as an argument, and expects id.driver_data to be a > HC driver pointer. > > So this turns out to be a question for Greg and Alan: > > Would it make sense to change usb_hcd_pci_probe() to take a HC driver pointer > as an argument instead of a pointer to pci_device_id? > pci_device_id pointer is only used to extract the HC driver handle. > This way the driver_data could be used for, well, driver data. That seems like a good idea to me. There aren't very many drivers that use usb_hcd_pci_probe(); changing them all should be fairly easy. Alan Stern
On 31.1.2020 10.40, Vinod Koul wrote: > Hi Mathias, > > On 30-01-20, 19:07, Mathias Nyman wrote: >> On 25.1.2020 7.32, Vinod Koul wrote: >>>>>>>> >>>>>>>> On Mon, Jan 13, 2020 at 12:42 AM Vinod Koul <vkoul@kernel.org> wrote: >>>>>>>>> >>>>>>>>> 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 programmed, 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, >>>>>>>>> debugfs hook for rom erase and export of xhci-pci functions. >>>>>>>>> ... >> >> There are a few other opens regarding this series. Mostly because I'm not (yet) >> familiar with all the details, so I'll just just list them here. >> >> - Is it really enough to add the Renesas driver to Makefile before xhci-pci >> driver to make sure it gets matched and probed based on vendor/device id >> before xhci-pci driver is matched and probed based on pci class? >> What if the Renesas driver is a module and xhci-pci compiled in? > > The driver loading rules are based on level and makefile order. So > renesas will always be loaded before xhci driver. This is required since > xhci claims all devices, so we need to make sure it loads before. > > I think we should make it inbuilt driver rather than a module. xhci > driver is only inbuilt. > > If there is a better way to handle this, am all for it. > >> - Previously probe didn't return before hcd's were added and everything set up. >> Now with request_firmware_nowait() probe returns early successfully, and the >> old xhci_pci_probe() which sets up everything is called later by the request >> firmware callback. So there could be whole new set of races possible. >> For example pci remove can be called mid firmware loading, or when the old >> xhci_pci_probe is still setting up things. > > I think this is a valid concern. Let me think about how to solve this.. > maybe add a flag in remove which ensure remove doesnt run untill the > probe/firmware callback is completed. How about initiating async firmware loading before probe is called by using DECLARE_PCI_FIXUP_*() hooks? Probe would then just check if there is a valid running firmware, if not just defer probe until later (return -EPROBE_DEFER). No need for a separate Renesas xhci driver, no worries about which driver claims the device first, no races because of probe returning successfully too early. Can we check the device for a valid running firmware without disrupting a ongoing firmware write? -Mathias
On 31-01-20, 10:47, Alan Stern wrote: > On Thu, 30 Jan 2020, Mathias Nyman wrote: > > > I realize this can't be easily changed because usb_hcd_pci_probe() takes the > > pci_device_id pointer as an argument, and expects id.driver_data to be a > > HC driver pointer. > > > > So this turns out to be a question for Greg and Alan: > > > > Would it make sense to change usb_hcd_pci_probe() to take a HC driver pointer > > as an argument instead of a pointer to pci_device_id? > > pci_device_id pointer is only used to extract the HC driver handle. > > This way the driver_data could be used for, well, driver data. > > That seems like a good idea to me. There aren't very many drivers that > use usb_hcd_pci_probe(); changing them all should be fairly easy. Yup it was easy to do :) I have done this and tested it. Now we can use driver_data for driver data. Though couldn't compile the uhci, seems to have missing Makefile entry.
On 04-02-20, 18:33, Mathias Nyman wrote: > > > > > > There are a few other opens regarding this series. Mostly because I'm not (yet) > > > familiar with all the details, so I'll just just list them here. > > > > > > - Is it really enough to add the Renesas driver to Makefile before xhci-pci > > > driver to make sure it gets matched and probed based on vendor/device id > > > before xhci-pci driver is matched and probed based on pci class? > > > What if the Renesas driver is a module and xhci-pci compiled in? > > > > The driver loading rules are based on level and makefile order. So > > renesas will always be loaded before xhci driver. This is required since > > xhci claims all devices, so we need to make sure it loads before. > > > > I think we should make it inbuilt driver rather than a module. xhci > > driver is only inbuilt. > > > > If there is a better way to handle this, am all for it. > > > > > - Previously probe didn't return before hcd's were added and everything set up. > > > Now with request_firmware_nowait() probe returns early successfully, and the > > > old xhci_pci_probe() which sets up everything is called later by the request > > > firmware callback. So there could be whole new set of races possible. > > > For example pci remove can be called mid firmware loading, or when the old > > > xhci_pci_probe is still setting up things. > > > > I think this is a valid concern. Let me think about how to solve this.. > > maybe add a flag in remove which ensure remove doesnt run untill the > > probe/firmware callback is completed. > > How about initiating async firmware loading before probe is called by using > DECLARE_PCI_FIXUP_*() hooks? Well somehow that does not work :( I tried using DECLARE_PCI_FIXUP_FINAL hook to request the firmware. Doing so from probe works but from fixup fails always! So expect this one, I have done the rest of the changes requested, will send them over soon Thanks