Re: [PATCH v12 2/5] usb: renesas-xhci: Add the renesas xhci driver

[Greg Kroah-Hartman]

On Mon, May 04, 2020 at 08:04:38PM +0530, Vinod Koul wrote:
> Hi Mathias,
> 
> On 04-05-20, 16:01, Mathias Nyman wrote:
> > On 30.4.2020 19.59, Vinod Koul wrote:
> > > From: Christian Lamparter <chunkeey@xxxxxxxxxxxxxx>
> > > 
> > > This add a new driver for renesas xhci which is basically a firmware
> > > loader for uPD720201 and uPD720202 w/o ROM. The xhci-pci driver will
> > > invoke this driver for loading/unloading on relevant devices.
> > > 
> > > 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.
> > > 
> > > Signed-off-by: Christian Lamparter <chunkeey@xxxxxxxxxxxxxx>
> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> > > [vkoul: fixed comments:
> > > 	used macros for timeout count and delay
> > > 	removed renesas_fw_alive_check
> > > 	cleaned renesas_fw_callback
> > > 	removed recursion for renesas_fw_download
> > > 	add register defines and field names
> > > 	move to a separate file
> > > 	make fw loader as sync probe so that we execute in probe and
> > >         prevent race
> > > 	export symbols for xhci-pci to use]
> > > Signed-off-by: Vinod Koul <vkoul@xxxxxxxxxx>
> > > ---
> > >  drivers/usb/host/Makefile           |   2 +-
> > >  drivers/usb/host/xhci-pci-renesas.c | 365 ++++++++++++++++++++++++++++
> > >  drivers/usb/host/xhci-pci.h         |  16 ++
> > >  3 files changed, 382 insertions(+), 1 deletion(-)
> > >  create mode 100644 drivers/usb/host/xhci-pci-renesas.c
> > >  create mode 100644 drivers/usb/host/xhci-pci.h
> > > 
> > > diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
> > > index b191361257cc..f3a5a2f01874 100644
> > > --- a/drivers/usb/host/Makefile
> > > +++ b/drivers/usb/host/Makefile
> > > @@ -70,7 +70,7 @@ obj-$(CONFIG_USB_OHCI_HCD_DAVINCI)	+= ohci-da8xx.o
> > >  obj-$(CONFIG_USB_UHCI_HCD)	+= uhci-hcd.o
> > >  obj-$(CONFIG_USB_FHCI_HCD)	+= fhci.o
> > >  obj-$(CONFIG_USB_XHCI_HCD)	+= xhci-hcd.o
> > > -obj-$(CONFIG_USB_XHCI_PCI)	+= xhci-pci.o
> > > +obj-$(CONFIG_USB_XHCI_PCI)	+= xhci-pci.o xhci-pci-renesas.o
> > 
> > Hmm, now we end up with two modules, xhci-pci and xhci-pci-renesas, even if
> > xhci-pci-renesas just includes helper functions to load firmware for renesas.
> 
> Right, these are two modules. Do you forsee an issue with two ko's

Two kos should be fine, but as you aren't giving people the option to
not select this, it's a bit harsh to add it.

Can this be a separate module/config option?  Why force everyone to need
this additional code if they do not have this hardware?

thanks,

greg k-h