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
