usb: xhci: Restore xhci_pci support for Renesas HCs
Subsystems (2)
PATCH 1/1 usb: xhci: Restore xhci_pci support for Renesas HCs
Some Renesas HCs require firmware upload to work, this is handled by the xhci_pci_renesas driver. Other variants of those chips load firmware from onboard flash and can work with xhci_pci alone.
A refactor merged in v6.12 broke the latter configuration and users are finding their hardware not working after upgrading and need to enable the firmware loader which isn't really necessary on their systems.
Let xhci_pci work with those chips as before when the firmware loader is disabled by kernel configuration.
--- drivers/usb/host/xhci-pci.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/usb/host/xhci-pci.c +4 -0
@@ -657,17 +657,21 @@ int xhci_pci_common_probe(struct pci_dev *dev, const struct pci_device_id *id) }EXPORT_SYMBOL_NS_GPL (xhci_pci_common_probe,"xhci" ); +#if IS_ENABLED (CONFIG_USB_XHCI_PCI_RENESAS )static const struct pci_device_id pci_ids_reject[] = {/* handled by xhci-pci-renesas */ {PCI_DEVICE (PCI_VENDOR_ID_RENESAS ,0x0014 ) }, {PCI_DEVICE (PCI_VENDOR_ID_RENESAS ,0x0015 ) }, {/* end: all zeroes */ } }; +#endif static int xhci_pci_probe (struct pci_dev *dev,const struct pci_device_id *id) { +#if IS_ENABLED (CONFIG_USB_XHCI_PCI_RENESAS )if (pci_match_id (pci_ids_reject, dev))return -ENODEV ; +#endif return xhci_pci_common_probe (dev, id); }
Signature · 1 line
2.48.1
Greg Kroah-Hartman
maintainerQuoted 7 lines · Michal Pecio
Some Renesas HCs require firmware upload to work, this is handled by the
xhci_pci_renesasdriver. Other variants of those chips load firmware from onboard flash and can work withxhci_pcialone.A refactor merged in v6.12 broke the latter configuration and users are finding their hardware not working after upgrading and need to enable the firmware loader which isn't really necessary on their systems.
Why isn't it necessary? Those chips want firmware to be uploaded, and if the driver doesn't need it, it should be able to handle it if it is not present.
Quoted 25 lines
Let
xhci_pciwork with those chips as before when the firmware loader is disabled by kernel configuration.Fixes 25f51b76f90f ("xhci-pci: Make xhci-pci-renesas a proper modular driver")Signed-off-by Michal Pecio <[email protected]>--- drivers/usb/host/xhci-pci.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/usb/host/xhci-pci.c +1
-0@@ -657,17 +657,21 @@ int xhci_pci_common_probe(struct pci_dev *dev, const struct pci_device_id *id) }EXPORT_SYMBOL_NS_GPL (xhci_pci_common_probe,"xhci" ); +#if IS_ENABLED (CONFIG_USB_XHCI_PCI_RENESAS )static const struct pci_device_id pci_ids_reject[] = {/* handled by xhci-pci-renesas */ {PCI_DEVICE (PCI_VENDOR_ID_RENESAS ,0x0014 ) }, { PCI_DEVICE(PCI_VENDOR_ID_RENESAS, 0x0015) },
drivers/usb/host/xhci-pci.c int xhci_pci_common_probe(struct pci_dev *dev, const struct pci_device_id *id) · line 674
Quoted 10 lines
drivers/usb/host/xhci-pci.c +3
-0{ /* end: all zeroes */ } }; +#endif static int xhci_pci_probe (struct pci_dev *dev,const struct pci_device_id *id) { +#if IS_ENABLED (CONFIG_USB_XHCI_PCI_RENESAS )if (pci_match_id (pci_ids_reject, dev))return -ENODEV ; +#endif
No, please don't put #ifdef in .c files if at all preventable.
If the firmware isn't there, then the driver should be fixed to keep on working if it determines loading it is not needed. This type of check should not be needed in the xhci-pci.c file.
thanks,
greg k-h
Quoted 5 lines · Greg Kroah-Hartman
Quoted 4 lines · Michal Pecio
Some Renesas HCs require firmware upload to work, this is handled by the
xhci_pci_renesasdriver. Other variants of those chips load firmware from onboard flash and can work withxhci_pcialone.
Quoted 8 lines
Quoted 4 lines
A refactor merged in v6.12 broke the latter configuration and users are finding their hardware not working after upgrading and need to enable the firmware loader which isn't really necessary on their systems.
Why isn't it necessary? Those chips want firmware to be uploaded, and if the driver doesn't need it, it should be able to handle it if it is not present.
Because their chips have already loaded themselves from attached SPI flash, so they are ready to use with xhci_pci like any other HC.
The FW isn't optional, we just usually don't need to load it. And FW for those chips is obscure and not redistributable AFAIK, so people have little reason to enable xhci_pci_renesas if their board has ROM and works without it. It is 'N' by default.
Quoted 1 line
No, please don't put #ifdef in .c files if at all preventable.
Hmm, I suppose I could replace the #ifdef with 'if (IS_ENABLED(...))' and leave the rest to dead code elimination if that's preferred.
Annoyingly, the code to be knocked out is in xhci-pci and it has to stay there to avoid runtime dependency on xhci-pci-renesas, which was the whole point of the refactor.
Quoted 3 lines
If the firmware isn't there, then the driver should be fixed to keep on working if it determines loading it is not needed. This type of check should not be needed in the
xhci-pci.cfile.
This is a check for kernel configuration, not for firmware. If the loader is disabled, we don't want to reject those chips expecting them to be taken care of by the loader, because that is not going to happen.
Regards,
Michal
PATCH v2 1/1 usb: xhci: Restore xhci_pci support for Renesas HCs
In reply to Michal PecioSome Renesas HCs require firmware upload to work, this is handled by the xhci_pci_renesas driver. Other variants of those chips load firmware from a SPI flash and are ready to work with xhci_pci alone.
A refactor merged in v6.12 broke the latter configuration so that users are finding their hardware ignored by the normal driver and are forced to enable the firmware loader which isn't really necessary on their systems.
Let xhci_pci work with those chips as before when the firmware loader is disabled by kernel configuration.
---
new in v2:
- changed #ifdef CONFIG_XXX to if (IS_ENABLED(CONFIG_XXX))
- renamed the PCI IDs table to clarify code intent
- small commit message improvements
drivers/usb/host/xhci-pci.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/usb/host/xhci-pci.c +4 -3
@@ -657,8 +657,8 @@ int xhci_pci_common_probe(struct pci_dev *dev, const struct pci_device_id *id) }EXPORT_SYMBOL_NS_GPL (xhci_pci_common_probe,"xhci" ); -static const struct pci_device_id pci_ids_reject[] = { -/* handled by xhci-pci-renesas */ +/* handled by xhci-pci-renesas if enabled */ +static const struct pci_device_id pci_ids_renesas[] = { {PCI_DEVICE (PCI_VENDOR_ID_RENESAS ,0x0014 ) }, {PCI_DEVICE (PCI_VENDOR_ID_RENESAS ,0x0015 ) }, {/* end: all zeroes */ }
@@ -666,7 +666,8 @@ static const struct pci_device_id pci_ids_reject[] = {static int xhci_pci_probe (struct pci_dev *dev,const struct pci_device_id *id) { -if (pci_match_id (pci_ids_reject, dev)) +if (IS_ENABLED (CONFIG_USB_XHCI_PCI_RENESAS ) && +pci_match_id (pci_ids_renesas, dev))return -ENODEV ;return xhci_pci_common_probe (dev, id);
Signature · 1 line
2.48.1
Quoted 8 lines · Michal Pecio
Some Renesas HCs require firmware upload to work, this is handled by the
xhci_pci_renesasdriver. Other variants of those chips load firmware from a SPI flash and are ready to work withxhci_pcialone.A refactor merged in v6.12 broke the latter configuration so that users are finding their hardware ignored by the normal driver and are forced to enable the firmware loader which isn't really necessary on their systems.
Quoted 8 lines
Let
xhci_pciwork with those chips as before when the firmware loader is disabled by kernel configuration.Fixes 25f51b76f90f ("xhci-pci: Make xhci-pci-renesas a proper modular driver")Signed-off-by Michal Pecio <[email protected]>
Can confirm that this patch restores the old behavior, where xhci-pci supported the Renesas controllers when an external SPI flash is attached. I've tested with a PCIe extension card on arm64 and x86 as well as with a arm64 (Raspberry CM4) based Revolution Pi Connect 4.
Thanks!
Nicolai
Quoted 38 lines
---
new in v2:
- changed #ifdefCONFIG_XXXto if (IS_ENABLED(CONFIG_XXX))
- renamed the PCI IDs table to clarify code intent
- small commit message improvementsdrivers/usb/host/xhci-pci.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)drivers/usb/host/xhci-pci.c +2
-2@@ -657,8 +657,8 @@ int xhci_pci_common_probe(struct pci_dev *dev, const struct pci_device_id *id) }EXPORT_SYMBOL_NS_GPL (xhci_pci_common_probe,"xhci" ); -static const struct pci_device_id pci_ids_reject[] = { -/* handled by xhci-pci-renesas */ +/* handled by xhci-pci-renesas if enabled */ +static const struct pci_device_id pci_ids_renesas[] = { {PCI_DEVICE (PCI_VENDOR_ID_RENESAS ,0x0014 ) }, {PCI_DEVICE (PCI_VENDOR_ID_RENESAS ,0x0015 ) },drivers/usb/host/xhci-pci.c +2
-1{/* end: all zeroes */ }@@ -666,7 +666,8 @@ static const struct pci_device_id pci_ids_reject[] = {static int xhci_pci_probe (struct pci_dev *dev,const struct pci_device_id *id) { -if (pci_match_id (pci_ids_reject, dev)) +if (IS_ENABLED (CONFIG_USB_XHCI_PCI_RENESAS ) && +pci_match_id (pci_ids_renesas, dev))return -ENODEV ;return
xhci_pci_common_probe(dev, id);