From: Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
To: Johan Hovold <johan@kernel.org>,
Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
Bjorn Andersson <andersson@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Philipp Zabel <p.zabel@pengutronix.de>,
Andy Gross <agross@kernel.org>,
Konrad Dybcio <konrad.dybcio@linaro.org>,
Rob Herring <robh+dt@kernel.org>,
"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
Felipe Balbi <balbi@kernel.org>, <linux-usb@vger.kernel.org>,
<linux-kernel@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>,
<devicetree@vger.kernel.org>, <quic_pkondeti@quicinc.com>,
<quic_ppratap@quicinc.com>, <quic_wcheng@quicinc.com>,
<quic_jackp@quicinc.com>, <quic_harshq@quicinc.com>,
<ahalaney@redhat.com>
Subject: Re: [PATCH v8 6/9] usb: dwc3: qcom: Add multiport controller support for qcom wrapper
Date: Thu, 8 Jun 2023 20:53:24 +0530 [thread overview]
Message-ID: <279fff8b-57e2-cfc8-cd6d-c69d00e71799@quicinc.com> (raw)
In-Reply-To: <ZIGihYS5EacISEFm@hovoldconsulting.com>
On 6/8/2023 3:12 PM, Johan Hovold wrote:
> On Thu, Jun 08, 2023 at 01:21:02AM +0530, Krishna Kurapati PSSNV wrote:
>> On 6/7/2023 5:07 PM, Johan Hovold wrote:
>
>>> 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.
>
> That doesn't sound too convincing. Can't you just iterate over the PHYs
> described in DT and determine the maximum port number used for HS and
> SS?
>
>> 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.
>
> Yes, but if the information is already available in DT it's better to use
> it rather than re-encode it in the driver.
>
Hi Johan,
Are you suggesting that we just do something like
num_ports = max( highest usb2 portnum, highest usb3 port num)
If so, incase the usb2 phy of quad port controller is missing in DT, we
would still read num_usb2_ports as 4 but the usb2_generic_phy[1] would
be NULL and any phy ops would still be NOP. But we would be getting rid
of reading the xhci registers compeltely in core driver.
Thinh, Bjorn, can you also let us know your views on this.
1. Read:
num_usb3_ports = highest usb3 port index in DT
num_usb2_ports = max( highest usb2 port index, num_usb3_ports)
2. Read the same by parsing xhci registers as done in recent versions of
this series.
Regards,
Krishna,
>>> 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@kernel.org/
>>>
>>
>> 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.
>
> I still think we'll end up using callbacks from the xhci/core into the
> glue driver, but dedicated ones rather than using usb_register_notify().
>
> The fixes I posted can work as a stop-gap solution until then.
>
>>> 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.
>
> Right. A bit depending on how your wakeup implementation looks like, it
> seems we can merge multiport support and then address the layering
> issues which are already present in the driver.
>
>> [1]:
>> https://lore.kernel.org/all/4eb26a54-148b-942f-01c6-64e66541de8b@quicinc.com/
>> [2]:
>> https://lore.kernel.org/all/ca729f62-672e-d3de-4069-e2205c97e7d8@linaro.org/
>> [3]:
>> https://lore.kernel.org/all/37fd026e-ecb1-3584-19f3-f8c1e5a9d20a@quicinc.com/
>
> Johan
next prev parent reply other threads:[~2023-06-08 15:23 UTC|newest]
Thread overview: 74+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-14 5:49 [PATCH v8 0/9] Add multiport support for DWC3 controllers Krishna Kurapati
2023-05-14 5:49 ` [PATCH v8 1/9] dt-bindings: usb: qcom,dwc3: Add bindings for SC8280 Multiport Krishna Kurapati
2023-05-14 9:46 ` Krzysztof Kozlowski
2023-05-16 10:59 ` Johan Hovold
2023-05-17 11:10 ` Krishna Kurapati PSSNV
2023-05-17 11:44 ` Johan Hovold
2023-05-17 12:19 ` Krishna Kurapati PSSNV
2023-05-17 12:55 ` Johan Hovold
2023-05-14 5:49 ` [PATCH v8 2/9] dt-bindings: usb: Add bindings for multiport properties on DWC3 controller Krishna Kurapati
2023-05-14 5:49 ` [PATCH v8 3/9] usb: dwc3: core: Access XHCI address space temporarily to read port info Krishna Kurapati
2023-05-15 21:08 ` Bjorn Andersson
2023-05-16 2:12 ` Krishna Kurapati PSSNV
2023-05-16 22:39 ` Thinh Nguyen
2023-05-16 12:11 ` Johan Hovold
2023-05-16 15:02 ` Krishna Kurapati PSSNV
2023-05-17 3:10 ` Krishna Kurapati PSSNV
2023-05-17 3:21 ` Thinh Nguyen
2023-05-17 7:46 ` Johan Hovold
2023-05-17 23:21 ` Thinh Nguyen
2023-06-07 11:56 ` Johan Hovold
2023-05-17 7:35 ` Johan Hovold
2023-05-17 12:21 ` Krishna Kurapati PSSNV
2023-05-17 15:10 ` Johan Hovold
2023-05-14 5:49 ` [PATCH v8 4/9] usb: dwc3: core: Skip setting event buffers for host only controllers Krishna Kurapati
2023-05-15 21:19 ` Bjorn Andersson
2023-05-16 12:17 ` Johan Hovold
2023-05-16 14:28 ` Krishna Kurapati PSSNV
2023-05-14 5:49 ` [PATCH v8 5/9] usb: dwc3: core: Refactor PHY logic to support Multiport Controller Krishna Kurapati
2023-05-15 21:47 ` Bjorn Andersson
2023-05-16 2:31 ` Krishna Kurapati PSSNV
2023-05-17 16:17 ` Johan Hovold
2023-05-14 5:49 ` [PATCH v8 6/9] usb: dwc3: qcom: Add multiport controller support for qcom wrapper Krishna Kurapati
2023-05-15 22:27 ` Bjorn Andersson
2023-05-16 2:19 ` Krishna Kurapati PSSNV
2023-05-17 16:37 ` Johan Hovold
2023-05-20 17:48 ` Krishna Kurapati PSSNV
2023-06-07 11:37 ` Johan Hovold
2023-06-07 19:51 ` Krishna Kurapati PSSNV
2023-06-08 9:42 ` Johan Hovold
2023-06-08 15:23 ` Krishna Kurapati PSSNV [this message]
2023-06-08 17:57 ` Thinh Nguyen
2023-06-09 8:18 ` Johan Hovold
2023-06-09 18:16 ` Thinh Nguyen
2023-06-15 4:20 ` Krishna Kurapati PSSNV
2023-06-15 21:08 ` Thinh Nguyen
2023-06-21 7:38 ` Johan Hovold
2023-06-22 4:39 ` Krishna Kurapati PSSNV
2023-06-21 7:34 ` Johan Hovold
2023-06-22 22:41 ` Thinh Nguyen
2023-05-26 2:55 ` Bjorn Andersson
2023-05-26 15:25 ` Krishna Kurapati PSSNV
2023-06-07 11:44 ` Johan Hovold
2023-06-07 19:55 ` Krishna Kurapati PSSNV
2023-06-08 9:44 ` Johan Hovold
2023-06-07 12:16 ` Johan Hovold
2023-06-27 15:43 ` Johan Hovold
2023-07-02 19:05 ` Krishna Kurapati PSSNV
2023-07-14 9:00 ` Johan Hovold
2023-07-14 10:38 ` Krishna Kurapati PSSNV
2023-07-21 11:16 ` Johan Hovold
2023-07-21 12:10 ` Konrad Dybcio
2023-07-21 12:54 ` Johan Hovold
2023-08-11 16:48 ` Konrad Dybcio
2023-08-12 8:58 ` Krishna Kurapati PSSNV
2023-05-14 5:49 ` [PATCH v8 7/9] arm64: dts: qcom: sc8280xp: Add multiport controller node for SC8280 Krishna Kurapati
2023-05-15 14:26 ` Johan Hovold
2023-05-15 15:32 ` Krishna Kurapati PSSNV
2023-05-16 10:54 ` Johan Hovold
2023-05-16 14:24 ` Krishna Kurapati PSSNV
2023-05-16 14:42 ` Johan Hovold
2023-05-16 14:44 ` Krishna Kurapati PSSNV
2023-05-14 5:49 ` [PATCH v8 8/9] arm64: dts: qcom: sa8295p: Enable tertiary controller and its 4 USB ports Krishna Kurapati
2023-05-14 5:49 ` [PATCH v8 9/9] arm64: dts: qcom: sa8540-ride: Enable first port of tertiary usb controller Krishna Kurapati
2023-05-15 2:40 ` [PATCH v8 0/9] Add multiport support for DWC3 controllers Bjorn Andersson
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=279fff8b-57e2-cfc8-cd6d-c69d00e71799@quicinc.com \
--to=quic_kriskura@quicinc.com \
--cc=Thinh.Nguyen@synopsys.com \
--cc=agross@kernel.org \
--cc=ahalaney@redhat.com \
--cc=andersson@kernel.org \
--cc=balbi@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=johan@kernel.org \
--cc=konrad.dybcio@linaro.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=p.zabel@pengutronix.de \
--cc=quic_harshq@quicinc.com \
--cc=quic_jackp@quicinc.com \
--cc=quic_pkondeti@quicinc.com \
--cc=quic_ppratap@quicinc.com \
--cc=quic_wcheng@quicinc.com \
--cc=robh+dt@kernel.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 read-only IMAP folder(s) and NNTP newsgroup(s).