Re: [PATCH v8 6/9] usb: dwc3: qcom: Add multiport controller support for qcom wrapper

[Thinh Nguyen]

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 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.
> 
> Hi Johan,
> 
>   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.

> 
> 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.

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.

Thanks,
Thinh