Messages in this thread | | | Date | Fri, 21 Jul 2023 14:15:54 +0530 | Subject | Re: [PATCH v9 06/10] usb: dwc3: qcom: Add support to read IRQ's related to multiport | From | Krishna Kurapati PSSNV <> |
| |
On 7/21/2023 2:05 PM, Johan Hovold wrote: > On Sun, Jul 16, 2023 at 12:31:05AM +0530, Krishna Kurapati PSSNV wrote: >> On 7/14/2023 4:10 PM, Krishna Kurapati PSSNV wrote: >>> On 7/14/2023 2:35 PM, Johan Hovold wrote: > >>>> I haven't had time to look at your latest replies yet, but as I already >>>> said when reviewing v9, it seems you should be using a common helper for >>>> non-mp and mp. > >>> The gist of my mail was to see if I can defer qcom probe when dwc3 >>> probe fails/or doesn't happen on of_plat_pop (which is logical) so that >>> we can move setup_irq to after dwc3_register_core so that we know >>> whether we are MP capable or not. This would help us move all IRQ >>> reading into one function. > >> I see it is difficult to write a common helper. To do so, we need to >> know whether the device is MP capable or not in advance. And since it is >> not possible to know it before of_plat_pop is done, I see only few ways >> to do it: >> >> 1. Based on qcom node compatible string, I can read whether the device >> is MP capable or not and get IRQ's accordingly. > > See, it's not impossible. You can also determine whether you have a > multiport controller from looking at the interrupt names which are > indexed and distinct for MP. > >> 2. Read the port_info in advance but it needs me to go through some DT >> props and try getting this info. Or read xhci regs like we are doing in >> core (which is not good). Also since some Dt props can be missing, is it >> difficult to get the MP capability info before of_plat_pop is done. > > That seem unnecessary currently, but long term we probably need to fix > the design of this driver and defer some setup using callbacks that are > called when the core driver probes. Perhaps now is the time to add such > functionality. > >> 3. Remove IRQ handling completely. Just because the device has IRQ's >> present, I don't see a point in adding them to bindings, and because we >> added them to bindings, we are making a patch to read them (and since >> this is a little challenging, the whole of multiport series is blocked >> although I don't need wakeup support on these interrupts right away). > > Again, no. The devicetree binding should describe the hardware > capabilities and that has nothing to do with whether you need this for > you current project or not. > >> Can't we let the rest of the patches go through and let interrupt >> handling for 2nd, 3rd and 4rth ports be taken care later ? I am asking >> this because I want the rest of the patches which are in good shape now >> (after fixing the nits mentioned) to get merged atleast. I will make >> sure to add interrupt handling later in a different series once this is >> merged once I send v10. > > As I've explained in earlier mails, I don't think that is acceptable as > you'd be dumping your technical debt on the community which will be left > to clean up your mess. > >> Or if there is a simpler way to do it, I would be happy to take any >> suggestions and complete this missing part in this series itself. >
Hi Johan,
Thanks for these comments.
> Using the 'compatible' or 'interrupt-names' properties seems like the > easiest way to determine whether you have an MP controller or not. >
Yes, I can make a common helper to get IRQ's based on compatible. I also provided another implementation (which is more unambiguous and better I feel) on [1]. I will take one path forward based on your review of that patch as well.
Thanks a lot again for the reviews !
[1]: https://lore.kernel.org/all/f6f2456d-0067-6cd6-3282-8cae7c47a2d3@quicinc.com/
Regards, Krishna,
| |