Message-ID: <ZILgW5CwfSlBxzNB@hovoldconsulting.com>
Date:   Fri, 9 Jun 2023 10:18:35 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>
Cc:     Krishna Kurapati PSSNV <quic_kriskura@...cinc.com>,
        Bjorn Andersson <andersson@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Andy Gross <agross@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Felipe Balbi <balbi@...nel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "quic_pkondeti@...cinc.com" <quic_pkondeti@...cinc.com>,
        "quic_ppratap@...cinc.com" <quic_ppratap@...cinc.com>,
        "quic_wcheng@...cinc.com" <quic_wcheng@...cinc.com>,
        "quic_jackp@...cinc.com" <quic_jackp@...cinc.com>,
        "quic_harshq@...cinc.com" <quic_harshq@...cinc.com>,
        "ahalaney@...hat.com" <ahalaney@...hat.com>
Subject: Re: [PATCH v8 6/9] usb: dwc3: qcom: Add multiport controller support
 for qcom wrapper

On Thu, Jun 08, 2023 at 05:57:23PM +0000, Thinh Nguyen wrote:
> On Thu, Jun 08, 2023, Krishna Kurapati PSSNV wrote:
> > 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 core will just be looking at the HW capability registers and
> accessing the ports capability. Our programming guide also listed the
> host capability registers in its documentation. We're not driving the
> xhci controller here. We're initializing some of the core configs base
> on its capability.
> 
> We're duplicating the logic here and not exactly doing it verbatim.
> Let's try not to share the whole xhci header where we should not have
> visibility over. Perhaps it makes sense in some other driver, but let's
> not do it here.

The patch series even copied the kernel doc verbatim. This is just not
the way things are supposed to be done upstream. We share defines and
implementations all the time, but we should not be making copies of
them.

> > > > > 
> > > > > 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?
> 
> See below.
> 
> > > > > 
> > > > 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.

> >   Are you suggesting that we just do something like
> > num_ports = max( highest usb2 portnum, highest usb3 port num)
> 
> Why do we want to do this? This makes num_ports ambiguous. Let's not
> sacrifice clarity for some lines of code.

This is not about lines of code, but avoiding the bad practice of
copying code around and, to some degree, maintaining the separation
between the glue, core, and xhci which Felipe (perhaps mistakingly) has
fought for.

If you just need to know how many PHYs you have in DT so that you can
iterate over that internal array, you can just look at the max index in
DT where the indexes are specified in the first place.

Don't get hung up on the current variable names, those can be renamed to
match the implementation. Call it max_ports or whatever.

> > 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.
> 
> DT is not reliable to get this info. As noted, the DT may skip some
> ports and still be fine. However, the driver doesn't know which port
> reflects which port config index without the exact port count.

That's not correct. DT provides the port indexes already, for example:

	phy-names = "usb2-port0", "usb3-port0",
		    "usb2-port1", "usb3-port1",
		    "usb2-port2",
		    "usb2-port3";

So if you just need this to iterate over the PHYs all the information
needed is here.

If you need to access ports which do not have a PHY described in DT,
then this is not going to suffice, but I have not seen anyone claim that
that is needed yet.
 
> More importantly, the host controller that lives on the PCI bus will not
> use DT. This can be useful for some re-configurations if the controller
> is a PCI device and that goes through the dwc3 code path.

Ok, this is a bit hand wavy, but if this ever turns out to be needed it
can also be implemented then.

Or just generalise the xhci implementation for parsing these registers
and reuse that from the start. (As a bonus you'd shrink the kernel text
size by getting rid of that iffy inline implementation.)

Johan

Powered by blists - more mailing lists