On Wed, May 17, 2023 at 05:51:45PM +0530, Krishna Kurapati PSSNV wrote:

On 5/17/2023 1:05 PM, Johan Hovold wrote:

quoted

quoted

quoted

quoted

+	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