Re: [RFC PATCH 06/10] usb: xhci: Add Tegra XHCI host-controller driver

[Andrew Bresticker]

On Thu, May 15, 2014 at 2:18 PM, Thierry Reding
<thierry.reding@xxxxxxxxx> wrote:
> On Thu, May 15, 2014 at 01:18:22PM -0700, Andrew Bresticker wrote:
>> Arnd,
>>
>> On Thu, May 15, 2014 at 1:17 AM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
>> > On Wednesday 14 May 2014 17:33:02 Andrew Bresticker wrote:
>> >> +
>> >> +int tegra_xhci_register_mbox_notifier(struct notifier_block *nb)
>> >> +{
>> >> +     int ret;
>> >> +
>> >> +     mutex_lock(&tegra_xhci_mbox_lock);
>> >> +     ret = raw_notifier_chain_register(&tegra_xhci_mbox_notifiers, nb);
>> >> +     mutex_unlock(&tegra_xhci_mbox_lock);
>> >> +
>> >> +     return ret;
>> >> +}
>> >> +EXPORT_SYMBOL(tegra_xhci_register_mbox_notifier);
>> >> +
>> >> +void tegra_xhci_unregister_mbox_notifier(struct notifier_block *nb)
>> >> +{
>> >> +     mutex_lock(&tegra_xhci_mbox_lock);
>> >> +     raw_notifier_chain_unregister(&tegra_xhci_mbox_notifiers, nb);
>> >> +     mutex_unlock(&tegra_xhci_mbox_lock);
>> >> +}
>> >> +EXPORT_SYMBOL(tegra_xhci_unregister_mbox_notifier);
>> >
>> > What driver would use these?
>>
>> It's used by just this driver (the host) and the PHY driver (next
>> patch in series).
>>
>> > My feeling is that if you have a mailbox that is used by multiple
>> > drivers, you should use a proper mailbox driver to operate them,
>> > and have the drivers register with that API instead of a custom one.
>>
>> Ok, will do.
>>
>> >> +     /* Create child xhci-plat device */
>> >> +     memset(xhci_resources, 0, sizeof(xhci_resources));
>> >> +     res = platform_get_resource(to_platform_device(dev), IORESOURCE_IRQ, 0);
>> >> +     if (!res) {
>> >> +             dev_err(dev, "Missing XHCI IRQ\n");
>> >> +             ret = -ENODEV;
>> >> +             goto out;
>> >> +     }
>> >> +     xhci_resources[0].start = res->start;
>> >> +     xhci_resources[0].end = res->end;
>> >> +     xhci_resources[0].flags = res->flags;
>> >> +     xhci_resources[0].name = res->name;
>> >> +     res = platform_get_resource(to_platform_device(dev), IORESOURCE_MEM, 0);
>> >> +     if (!res) {
>> >> +             dev_err(dev, "Missing XHCI registers\n");
>> >> +             ret = -ENODEV;
>> >> +             goto out;
>> >> +     }
>> >> +     xhci_resources[1].start = res->start;
>> >> +     xhci_resources[1].end = res->end;
>> >> +     xhci_resources[1].flags = res->flags;
>> >> +     xhci_resources[1].name = res->name;
>> >> +
>> >> +     xhci = platform_device_alloc("xhci-hcd", PLATFORM_DEVID_AUTO);
>> >> +     if (!xhci) {
>> >> +             dev_err(dev, "Failed to allocate XHCI host\n");
>> >> +             ret = -ENOMEM;
>> >> +             goto out;
>> >> +     }
>> >
>> > This does not feel appropriate at all: Rather than creating a child device,
>> > you should have a specific driver that hooks into functions exported
>> > by the xhci core. See Documentation/driver-model/design-patterns.txt
>>
>> This is how DWC3, currently the only in-tree non-PCI XHCI host driver,
>> is structured - see drivers/usb/dwc3/host.c.  The recently proposed
>> Armada XHCI driver [1] just adds clock support and a hook in
>> xhci-plat's probe() to do the platform-specific initialization.
>
> Ugh... that very much sounds like the midlayer mistake. Doing that is
> not going to scale in the long run. Everybody will just keep adding
> quirks to the initialization until it's become a huge mess.

Agreed - I didn't even bother trying to write the Tegra driver like that.

>> Tegra's XHCI driver initialization is quite a bit more complicated,
>> mainly due to the need for external firmware and specific ordering
>> (e.g. firmware messages should only be enabled after the HCD is
>> created).  I could do away with the xhci-plat sub-device and just
>> create a Tegra hc_driver, but it seems silly to have three XHCI
>> platform drivers structured in three different ways.  USB folks, do
>> you have an opinion on how this should be done?
>
> The tendency in other subsystems (and I think this is also true to some
> degree for USB, though I'm less familiar with it) is to make common
> functions available as a library of helpers so that other drivers can
> use them (either directly or by wrapping them in platform-specific
> implementations). That allows you to keep the platform-specific code
> where it belongs: in the platform-specific driver.

Yes, this is how the non-PCI EHCI host drivers are written.  I'll
structure it more like that for the next spin.

-Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/