Hi,

(remember to break your lines at 80-columns)

Manish Narani [off-list ref] writes:

<snip>

quoted

quoted

+		goto err;
+	}
+
+	ret = dwc3_xlnx_rst_assert(priv_data->apbrst);
+	if (ret < 0) {
+		dev_err(dev, "%s: %d: Failed to assert reset\n",
+			__func__, __LINE__);

		dev_err(dev, "Failed to assert APB reset\n");

quoted

+		goto err;
+	}
+
+	ret = phy_init(priv_data->usb3_phy);

dwc3 core should be handling this already

The USB controller used in Xilinx ZynqMP platform uses xilinx GT phy
which has 4 GT lanes and can used by 4 peripherals at a time.

At the same time or are they mutually exclusive?

This USB controller uses 1 GT phy lane among the 4 GT lanes. To
configure the GT lane for USB controller, the below sequence is
expected.

1. Assert the USB controller resets.
2. Configure the Xilinx GT phy lane for USB controller (phy_init).
3. De-assert the USB controller resets and configure PIPE.
4. Wait for PLL of the GT lane used by USB to be locked
   (phy_power_on).

it seems like you need to extend the PHY framework and teach it about
lane configuration.

The dwc3 core by default does the phy_init() and phy_power_on() but
the default sequence doesn't work with Xilinx platforms. Because of
this reason, we have introduced this new driver to support the new
sequence.

Instead of teaching the relevant framework about your new requirements
;-)

quoted

quoted

+	if (ret < 0) {
+		dev_err(dev, "%s: %d: Failed to release reset\n",
+			__func__, __LINE__);
+		goto err;
+	}
+
+	/* Set PIPE power present signal */
+	writel(PIPE_POWER_ON, priv_data->regs + PIPE_POWER_OFFSET);
+
+	/* Clear PIPE CLK signal */
+	writel(PIPE_CLK_OFF, priv_data->regs + PIPE_CLK_OFFSET);

shouldn't this be hidden under clk_enable()?

Though its naming suggests something related to clock framework, it is
a register in the Xilinx USB controller space which configures the
PIPE clock coming from Serdes.

PIPE clock is a clock. It just so happens that the source is the PHY
itself.

quoted

quoted

+static int dwc3_xlnx_resume(struct device *dev)
+{
+	struct dwc3_xlnx *priv_data = dev_get_drvdata(dev);
+
+	return clk_bulk_enable(priv_data->num_clocks, priv_data->clks);
+}

you have the same implementation for both types of suspend/resume. Maybe
extract dwc3_xlnx_{suspend,resume}_common() and just call it from both
callbacks?

Going forward we have a plan to add Hibernation handling calls in
dwc3_xlnx_suspend/resume functions.

at that moment and only at that moment, should you be worried about
splitting them.

For that reason, these APIs are kept separate. If you insist, I can
make them common for now and separate them later when I add
hibernation code.

It would be a little better, no?

cheers

-- 
balbi