On 5/17/2023 1:05 PM, Johan Hovold wrote:
You should not make another copy of xhci_find_next_ext_cap(), but rather
use it directly.
We already have drivers outside of usb/host using this function so it
should be fine to do the same for now:
#include "../host/xhci-ext-caps.h"
This was the approach which we followed when we first introduced the
patch [1]. But Thinh suggested to duplicate code so that we can avoid
any dependency on xhci (which seems to be right). So since its just one
function, I duplicated it here.
Ok, fair enough. I still think we should not be duplicating the
xhci definitions like this even if we were to copy the helper to avoid
any future dependencies on xhci (it's currently an inline function,
which is also not very nice).
I'll take closer look at the rest of the series as there are a few more
of these layering violations which we should try to avoid.
+ offset = dwc3_xhci_find_next_ext_cap(regs, 0,
+ XHCI_EXT_CAPS_PROTOCOL);
+ while (offset) {
+ temp = readl(regs + offset);
+ major_revision = XHCI_EXT_PORT_MAJOR(temp);
+
+ temp = readl(regs + offset + 0x08);
+ if (major_revision == 0x03) {
+ dwc->num_usb3_ports += XHCI_EXT_PORT_COUNT(temp);
+ } else if (major_revision <= 0x02) {
+ dwc->num_usb2_ports += XHCI_EXT_PORT_COUNT(temp);
+ } else {
+ dev_err(dwc->dev,
+ "Unrecognized port major revision %d\n", major_revision);
Perhaps this should be handles as in xhci core by simply warning and
continuing instead.
I broke the loop and went to unmap as we are not sure what values would
be read. Any use of continuing ?
Mostly to align with xhci core which currently handles this case. It
would not not work unless you get rid of the max-ports check below
though.
+ ret = -EINVAL;
+ goto unmap_reg;
+ }
+
+ offset = dwc3_xhci_find_next_ext_cap(regs, offset,
+ XHCI_EXT_CAPS_PROTOCOL);
+ }
+
+ 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).
Hi Johan,
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., ?
Do we have any machines today that support HOST_ONLY_CONFIG indicated in
HWPARAMS0 that support multiport and violate this rule ?
Regards,
Krishna,
