Re: [PATCH v13 3/5] usb: xhci: Add support for Renesas controller with memory

[Vinod Koul <vkoul@kernel.org>]

HI Arnd,

On 19-05-20, 09:44, Arnd Bergmann wrote:
> On Tue, May 19, 2020 at 6:53 AM Vinod Koul <vkoul@kernel.org> wrote:
> > On 19-05-20, 00:37, Anders Roxell wrote:
> > > On Mon, 18 May 2020 at 21:57, Vinod Koul <vkoul@kernel.org> wrote:
> > > > On 18-05-20, 19:53, Anders Roxell wrote:
> > > > > On Wed, 6 May 2020 at 08:01, Vinod Koul <vkoul@kernel.org> wrote:
> > > > > >
> > > > > > Some rensas controller like uPD720201 and uPD720202 need firmware to be
> > > > > > loaded. Add these devices in pci table and invoke renesas firmware loader
> > > > > > functions to check and load the firmware into device memory when
> > > > > > required.
> > > > > >
> > > > > > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > > > >
> > > > > Hi, I got a build error when I built an arm64 allmodconfig kernel.
> > > >
> > > > Thanks for this. This is happening as we have default y for USB_XHCI_PCI
> > > > and then we make USB_XHCI_PCI_RENESAS=m. That should be not allowed as
> > > > we export as symbol so both can be inbuilt or modules but USB_XHCI_PCI=y
> > > > and USB_XHCI_PCI_RENESAS=m cant. While it is valid that USB_XHCI_PCI=y|m
> > > > and USB_XHCI_PCI_RENESAS=n
> > > >
> > > > So this seems to get fixed by below for me. I have tested with
> > > >  - both y and m (easy)
> > > >  - make USB_XHCI_PCI_RENESAS=n, USB_XHCI_PCI=y|m works
> > > >  - try making USB_XHCI_PCI=y and USB_XHCI_PCI_RENESAS=m, then
> > > >    USB_XHCI_PCI=m by kbuild :)
> > > >  - try making USB_XHCI_PCI=m and USB_XHCI_PCI_RENESAS=y, kbuild gives
> > > >    error prompt that it will be m due to depends
> > > >
> > > > Thanks to all the fixes done by Arnd which pointed me to this. Pls
> > > > verify
> > >
> > > I was able to build an arm64 allmodconfig kernel with this change.
> >
> > I will send the formal patch and add your name in reported and
> > tested. Thanks for the quick verification
> 
> I just checked the patch and I think this will work correctly in all cases,
> but it still seems a bit backwards:
> 
> > > > diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> > > > index b5c542d6a1c5..92783d175b3f 100644
> > > > --- a/drivers/usb/host/Kconfig
> > > > +++ b/drivers/usb/host/Kconfig
> > > > @@ -40,11 +40,11 @@ config USB_XHCI_DBGCAP
> > > >  config USB_XHCI_PCI
> > > >         tristate
> > > >         depends on USB_PCI
> > > > +       depends on USB_XHCI_PCI_RENESAS || !USB_XHCI_PCI_RENESAS
> > > >         default y
> > > >
> > > >  config USB_XHCI_PCI_RENESAS
> > > >         tristate "Support for additional Renesas xHCI controller with firwmare"
> > > > -       depends on USB_XHCI_PCI
> > > >         ---help---
> > > >           Say 'Y' to enable the support for the Renesas xHCI controller with
> > > >           firwmare. Make sure you have the firwmare for the device and
> > > >
> 
> I think it would have been better to follow the normal driver abstraction here
> and make the renesas xhci a specialized version of the xhci driver with
> its own platform_driver instance that calls into the generic xhci_pci module,
> rather than having the generic code treat it as a quirk.
> 
> That would be more like how we handle all the ehci and ohci variants, though
> I'm not sure how exactly it would work with two drivers having pci_device_id
> tables with non-exclusive members. Presumably the generic driver would
> still have to know that it needs to fail its probe() function on devices that
> need the firmware.

Yeah one of the earlier versions did try this and it wasn't nice. The
xhci driver claims the devices as it registers for the class. Now only
solution is to ensure we load the renesas first and resort to makefile
hacks..

-- 
~Vinod