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

From: Johan Hovold <johan@kernel.org>
To: Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
Cc: Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Andy Gross <agross@kernel.org>,
	Bjorn Andersson <andersson@kernel.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Felipe Balbi <balbi@kernel.org>,
	linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
	quic_pkondeti@quicinc.com, quic_ppratap@quicinc.com,
	quic_wcheng@quicinc.com, quic_jackp@quicinc.com,
	quic_harshq@quicinc.com, ahalaney@redhat.com
Subject: Re: [PATCH v8 3/9] usb: dwc3: core: Access XHCI address space temporarily to read port info
Date: Wed, 17 May 2023 17:10:38 +0200	[thread overview]
Message-ID: <ZGTubmBViN9rTiZC@hovoldconsulting.com> (raw)
In-Reply-To: <82a8c414-bfa8-5066-fb68-1ac117b76c4e@quicinc.com>

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