Re: [RFC v4 2/5] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

[Andrew Halaney]

On Sun, Jan 15, 2023 at 05:11:43PM +0530, Krishna Kurapati wrote:
> Currently the DWC3 driver supports only single port controller
> which requires at most one HS and one SS PHY.
> 
> But the DWC3 USB controller can be connected to multiple ports and
> each port can have their own PHYs. Each port of the multiport
> controller can either be HS+SS capable or HS only capable
> Proper quantification of them is required to modify GUSB2PHYCFG
> and GUSB3PIPECTL registers appropriately.
> 
> Add support for detecting, obtaining and configuring phy's supported
> by a multiport controller and limit the max number of ports
> supported to 4.
> 
> Signed-off-by: Harsh Agarwal <quic_harshq@xxxxxxxxxxx>
> Signed-off-by: Krishna Kurapati <quic_kriskura@xxxxxxxxxxx>
> ---
>  drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
>  drivers/usb/dwc3/core.h |  15 +-
>  drivers/usb/dwc3/drd.c  |  14 +-
>  3 files changed, 244 insertions(+), 89 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 476b63618511..7e0a9a598dfd 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c

<snip>

> @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>  	dwc->dis_split_quirk = device_property_read_bool(dev,
>  				"snps,dis-split-quirk");
>  
> +
> +	/*
> +	 * If no mulitport properties are defined, default
> +	 * the port count to '1'.
> +	 */
> +	ret = device_property_read_u32(dev, "num-ports",
> +				&dwc->num_ports);
> +	if (ret)
> +		dwc->num_ports = 1;
> +
> +	ret = device_property_read_u32(dev, "num-ss-ports",
> +				&dwc->num_ss_ports);
> +	if (ret)
> +		dwc->num_ss_ports = 1;

By using this DT property instead of using the number of each phy type you
find you can get into situations where you're writing DWC3_GUSB2PHYCFG, etc,
when there's no phy to go along with it.

I ran into this when testing on sa8540p-ride, which only uses one of the
ports on the multiport controller. I didn't enable the other phys (not
sure if that was smart or not) and overrode phy-names/phys, but did not
override num-ports/num-ss-ports, which resulted in that. Nothing bad
happened on a quick test.. but I thought I'd highlight that as another
downside of decoupling this value from the number of phys you grab.

Here's a patch enabling sa8540p-ride, I'd love if you'd add it to the
series (probably needs clean up after review, and will definitely need
alteration after you update the dt-binding again). If not I'll continue
to test/review so please CC me!: