On 6/7/2023 5:07 PM, Johan Hovold wrote:
Hi Krishna,
and sorry about the delay in following up on this. As usual, there are
just way too many threads and this one in particular requires a bit of
thought.
Hi Johan,
Thanks for taking the time out and reviewing the patches again.
On Sat, May 20, 2023 at 11:18:52PM +0530, Krishna Kurapati PSSNV wrote:
On 5/17/2023 10:07 PM, Johan Hovold wrote:
I don't think we should be adding more of these layering violations. A
parent device driver has no business messing with the driver data for a
child device which may or may not even have probed yet.
I added a FIXME elsewhere in the driver about fixing up the current
instances that have already snuck in (which in some sense is even worse
by accessing driver data of a grandchild device).
We really need to try sort this mess out and how to properly handle the
interactions between these layers (e.g. glue, dwc3 core and xhci). This
will likely involve adding callbacks from the child to the parent, for
example, when the child is suspending.
I agree with you, but in this case I believe there is no other way we
can find the number of ports present. Unless its a dt property which
parent driver can access the child's of node and get the details. Like
done in v4 [1]. But it would be adding redundant data into DT as pointed
out by Rob and Krzysztof and so we removed these properties.
So there at least two issues with this series:
1. accessing xhci registers from the dwc3 core
2. accessing driver data of a child device
1. The first part about accessing xhci registers goes against the clear
separation between glue, core and xhci that Felipe tried to maintain.
I'm not entirely against doing this from the core driver before
registering the xhci platform device as the registers are unmapped
afterwards. But if this is to be allowed, then the implementation should
be shared with xhci rather than copied verbatim.
The alternative that avoids this issue entirely could indeed be to
simply count the number of PHYs described in DT as Rob initially
suggested. Why would that not work?
The reason why I didn't want to read the Phy's from DT is explained in
[1]. I felt it makes the code unreadable and its very tricky to read the
phy's properly, so we decided we would initialize phy's for all ports
and if a phy is missing in DT, the corresponding member in
dwc->usbX_generic_phy[] would be NULL and any phy op on it would be a NOP.
Also as per Krzysztof suggestion on [2], we can add a compatible to read
number of phy's / ports present. This avoids accessing xhci members
atleast in driver core. But the layering violations would still be present.
2. The driver is already accessing driver data of the child device so
arguably your series is not really making things much worse than they
already are.
I've just sent a couple of fixes to address some of the symptoms of
this layering violation here:
https://lore.kernel.org/lkml/20230607100540.31045-1-johan+linaro@xxxxxxxxxx/
As you pointed out offline to me that using xhci event notifiers I
mentioned on [3], if it is not acceptable to use them as notifications
to check whether we are in host mode, I believe the only way would be to
use the patches you pushed in.
Getting this fixed properly is going to take a bit of work, and
depending on how your multiport wake up implementation is going to look
like, adding support for multiport controllers may not make this much
harder to address.
So perhaps we can indeed merge support for multiport and then get back
to cleaning this up.
So, you are referring to use the patches you pushed today as a partial
way to cleanup layering violations and merge multiport on top of it ? If
so, I am fine with it. I can rebase my changes on top of them.
[1]:
https://lore.kernel.org/all/4eb26a54-148b-942f-01c6-64e66541de8b@xxxxxxxxxxx/
[2]:
https://lore.kernel.org/all/ca729f62-672e-d3de-4069-e2205c97e7d8@xxxxxxxxxx/
[3]:
https://lore.kernel.org/all/37fd026e-ecb1-3584-19f3-f8c1e5a9d20a@xxxxxxxxxxx/
Regards,
Krishna,
