LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Mathias Nyman <mathias.nyman@linux.intel.com>
To: Vinod Koul <vkoul@kernel.org>,
	Christian Lamparter <chunkeey@gmail.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: John Stultz <john.stultz@linaro.org>,
	Mathias Nyman <mathias.nyman@intel.com>,
	linux-arm-msm@vger.kernel.org,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>,
	USB list <linux-usb@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Alan Stern <stern@rowland.harvard.edu>,
	Heikki Krogerus <heikki.krogerus@linux.intel.com>
Subject: Re: [PATCH v6 0/5] usb: xhci: Add support for Renesas USB controllers
Date: Thu, 30 Jan 2020 19:07:25 +0200	[thread overview]
Message-ID: <64340358-6682-4ae0-9c06-d72d5a4ff259@linux.intel.com> (raw)
In-Reply-To: <20200125053237.GG2841@vkoul-mobl>

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

  reply	other threads:[~2020-01-30 17:05 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-13  8:40 Vinod Koul
2020-01-13  8:40 ` [PATCH v6 1/5] usb: xhci: export few functions Vinod Koul
2020-01-13  8:40 ` [PATCH v6 2/5] usb: renesas-xhci: Add the renesas xhci driver Vinod Koul
2020-01-13  8:40 ` [PATCH v6 3/5] usb: renesas-xhci: Add ROM loader for uPD720201 Vinod Koul
2020-01-13  8:40 ` [PATCH v6 4/5] usb: renesas-xhci: allow multiple firmware versions Vinod Koul
2020-01-13  8:40 ` [PATCH v6 5/5] usb: xhci: provide a debugfs hook for erasing rom Vinod Koul
2020-01-13 20:09 ` [PATCH v6 0/5] usb: xhci: Add support for Renesas USB controllers John Stultz
2020-01-13 20:33   ` Christian Lamparter
2020-01-21  6:46     ` Vinod Koul
2020-01-21 20:26       ` Christian Lamparter
2020-01-24 21:38         ` Christian Lamparter
2020-01-25  5:32           ` Vinod Koul
2020-01-30 17:07             ` Mathias Nyman [this message]
2020-01-31  8:40               ` Vinod Koul
2020-02-04 16:33                 ` Mathias Nyman
2020-03-12  6:56                   ` Vinod Koul
2020-01-31 15:47               ` Alan Stern
2020-03-10 11:55                 ` Vinod Koul
2020-01-26  0:11 ` Andreas Böhler
2020-01-26 13:07   ` Christian Lamparter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=64340358-6682-4ae0-9c06-d72d5a4ff259@linux.intel.com \
    --to=mathias.nyman@linux.intel.com \
    --cc=bjorn.andersson@linaro.org \
    --cc=chunkeey@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=heikki.krogerus@linux.intel.com \
    --cc=john.stultz@linaro.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=mathias.nyman@intel.com \
    --cc=stern@rowland.harvard.edu \
    --cc=vkoul@kernel.org \
    --cc=yoshihiro.shimoda.uh@renesas.com \
    --subject='Re: [PATCH v6 0/5] usb: xhci: Add support for Renesas USB controllers' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).