Re: [PATCH v8 3/9] usb: dwc3: core: Access XHCI address space temporarily to read port info

[Johan Hovold]

On Wed, May 17, 2023 at 05:51:45PM +0530, Krishna Kurapati PSSNV wrote:
> On 5/17/2023 1:05 PM, Johan Hovold wrote:

> >>>> +	temp = readl(regs + DWC3_XHCI_HCSPARAMS1);
> >>>> +	if (HCS_MAX_PORTS(temp) != (dwc->num_usb3_ports + dwc->num_usb2_ports)) {
> >>>> +		dev_err(dwc->dev,
> >>>> +			"Mismatched reported MAXPORTS (%d)\n", HCS_MAX_PORTS(temp));
> >>>> +		ret = -EINVAL;
> >>>> +		goto unmap_reg;
> >>>> +	}
> >>>
> >>> Not sure this is needed either.
> >>>
> >>> Could this risk regressing platforms which does not have currently have
> >>> all PHYs described in DT?
> >>>
> >> No, it doesn't. AFAIK, this only tells how many ports are present as per
> >> the core consultant configuration of the device. I tried to explain what
> >> would happen incase phy's are not present in DT in [2] & [3].
> > 
> > Right, whether the PHYs are described in DT is not directly related to
> > this.
> > 
> > As long as HCS_MAX_PORTS by definition (assumption) is always
> > (dwc->num_usb3_ports + dwc->num_usb2_ports) any such machines would
> > continue to work.
> > 
> > But if you want to catch machines where this assumption does not hold,
> > you could also end up regressing machines which have so far been working
> > despite these numbers not adding up.
> > 
> > That may be acceptable, but I'm still not sure what the value of this
> > check is (e.g. as xhci core will handle basic sanity checks like usb2 +
> > usb3 <= max_ports).

>    Thanks for the review comments. Ideally the HCC_PARAMS1 must indicate 
> total number of ports supported. If not then I believe the core 
> consultant configuration is wrong.
> 
> According to the spec:
> 
> "The MaxPorts value in the HCSPARAMS1 register defines the number of
> Port Register Sets (e.g. PORTSC, PORTPMSC, and PORTLI register sets)."
> 
> So shouldn't the (usb2+usb3 ports be equal to MaxPorts to ensure each 
> port properly accesses the respective PortSC etc., ?

Sure, that's what is expected, but why do you need to add a check for
this in the glue driver all of a sudden? Your series does not seem to
rely on this. This is the xHCI driver's business (as is parsing these
registers in the first place, really).

Johan