diff mbox series

[06/12] usb: dwc3: qcom: Add dwc3 core reference in driver state

Message ID 20231016-dwc3-refactor-v1-6-ab4a84165470@quicinc.com (mailing list archive)
State New, archived
Headers show
Series usb: dwc3: qcom: Flatten dwc3 structure | expand

Commit Message

Bjorn Andersson Oct. 17, 2023, 3:11 a.m. UTC 
In the coming changes the Qualcomm DWC3 glue will be able to either
manage the DWC3 core as a child platform_device, or directly instantiate
it within its own context.

Introduce a reference to the dwc3 core state and make the driver
reference the dwc3 core either the child device or this new reference.

As the new member isn't assigned, and qcom->dwc_dev is assigned in all
current cases, the change should have no functional impact.

Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 drivers/usb/dwc3/dwc3-qcom.c | 100 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 83 insertions(+), 17 deletions(-)

Comments

Johan Hovold Nov. 22, 2023, 12:18 p.m. UTC | #1 
On Mon, Oct 16, 2023 at 08:11:14PM -0700, Bjorn Andersson wrote:
> In the coming changes the Qualcomm DWC3 glue will be able to either
> manage the DWC3 core as a child platform_device, or directly instantiate
> it within its own context.
> 
> Introduce a reference to the dwc3 core state and make the driver
> reference the dwc3 core either the child device or this new reference.
> 
> As the new member isn't assigned, and qcom->dwc_dev is assigned in all
> current cases, the change should have no functional impact.
> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 100 +++++++++++++++++++++++++++++++++++--------
>  1 file changed, 83 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 7c810712d246..901e5050363b 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -67,7 +67,8 @@ struct dwc3_acpi_pdata {
>  struct dwc3_qcom {
>  	struct device		*dev;
>  	void __iomem		*qscratch_base;
> -	struct platform_device	*dwc_dev;
> +	struct platform_device	*dwc_dev; /* only used when core is separate device */
> +	struct dwc3		*dwc; /* not used when core is separate device */

Hmm. This quickly become really messy and hard to maintain. It may be
fine as an intermediate step as part of this series, but why can't you
do the conversion fully so that the Qualcomm glue driver never registers
a core platform device? Is it just about where the core driver looks for
DT properties?

Johan
Bjorn Andersson Jan. 8, 2024, 6:02 p.m. UTC | #2 
On Wed, Nov 22, 2023 at 01:18:33PM +0100, Johan Hovold wrote:
> On Mon, Oct 16, 2023 at 08:11:14PM -0700, Bjorn Andersson wrote:
> > In the coming changes the Qualcomm DWC3 glue will be able to either
> > manage the DWC3 core as a child platform_device, or directly instantiate
> > it within its own context.
> > 
> > Introduce a reference to the dwc3 core state and make the driver
> > reference the dwc3 core either the child device or this new reference.
> > 
> > As the new member isn't assigned, and qcom->dwc_dev is assigned in all
> > current cases, the change should have no functional impact.
> > 
> > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > ---
> >  drivers/usb/dwc3/dwc3-qcom.c | 100 +++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 83 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > index 7c810712d246..901e5050363b 100644
> > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > @@ -67,7 +67,8 @@ struct dwc3_acpi_pdata {
> >  struct dwc3_qcom {
> >  	struct device		*dev;
> >  	void __iomem		*qscratch_base;
> > -	struct platform_device	*dwc_dev;
> > +	struct platform_device	*dwc_dev; /* only used when core is separate device */
> > +	struct dwc3		*dwc; /* not used when core is separate device */
> 
> Hmm. This quickly become really messy and hard to maintain. It may be
> fine as an intermediate step as part of this series, but why can't you
> do the conversion fully so that the Qualcomm glue driver never registers
> a core platform device? Is it just about where the core driver looks for
> DT properties?
> 

In the new driver model, pdev->dev.of_node needs to contain the
resources for both the glue and the core. For most of the information,
that's a matter of copying properties and child nodes from the child
of_node, but e.g. reg and interrupts needs to be merged.

As mentioned in my other reply, extcon is serviced to both nodes, so
without the callbacks that will break, at least - and I'd have to check
to see if the of_graphs can be handled...


That said, part of the reason for doing this shuffle is to make sure
that dwc is always a valid pointer, and while keeping this scheme of two
modes we will not be able to assume this anywhere in the code - and
hence continue to rely on luck.

One way around this would be to follow the of_platform_populate() with a
check to see if the core was registered and if so grab the dwc pointer,
otherwise of_platform_depopulate() the core again and probe defer.

It will come with a penalty for devices running on the old binding, and
we don't protect ourselves from the core being unbound while we're
holding a pointer to its internal data. But it looks like a much better
position to me.

(In this case I think dwc_dev becomes a local variable using during
probe, and the rest of the code would operate on dwc)

Regards,
Bjorn
diff mbox series

Patch

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 7c810712d246..901e5050363b 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -67,7 +67,8 @@  struct dwc3_acpi_pdata {
 struct dwc3_qcom {
 	struct device		*dev;
 	void __iomem		*qscratch_base;
-	struct platform_device	*dwc_dev;
+	struct platform_device	*dwc_dev; /* only used when core is separate device */
+	struct dwc3		*dwc; /* not used when core is separate device */
 	struct clk		**clks;
 	int			num_clocks;
 	struct reset_control	*resets;
@@ -263,7 +264,11 @@  static int dwc3_qcom_interconnect_init(struct dwc3_qcom *qcom)
 		goto put_path_ddr;
 	}
 
-	max_speed = usb_get_maximum_speed(&qcom->dwc_dev->dev);
+	if (qcom->dwc_dev)
+		max_speed = usb_get_maximum_speed(&qcom->dwc_dev->dev);
+	else
+		max_speed = usb_get_maximum_speed(qcom->dev);
+
 	if (max_speed >= USB_SPEED_SUPER || max_speed == USB_SPEED_UNKNOWN) {
 		ret = icc_set_bw(qcom->icc_path_ddr,
 				USB_MEMORY_AVG_SS_BW, USB_MEMORY_PEAK_SS_BW);
@@ -311,7 +316,10 @@  static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom)
 	/*
 	 * FIXME: Fix this layering violation.
 	 */
-	dwc = platform_get_drvdata(qcom->dwc_dev);
+	if (qcom->dwc_dev)
+		dwc = platform_get_drvdata(qcom->dwc_dev);
+	else
+		dwc = qcom->dwc;
 
 	/* Core driver may not have probed yet. */
 	if (!dwc)
@@ -322,10 +330,14 @@  static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom)
 
 static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
 {
-	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc_dev);
 	struct usb_device *udev;
 	struct usb_hcd __maybe_unused *hcd;
+	struct dwc3 *dwc;
 
+	if (qcom->dwc_dev)
+		dwc = platform_get_drvdata(qcom->dwc_dev);
+	else
+		dwc = qcom->dwc;
 	/*
 	 * FIXME: Fix this layering violation.
 	 */
@@ -485,12 +497,17 @@  static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
 static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data)
 {
 	struct dwc3_qcom *qcom = data;
-	struct dwc3	*dwc = platform_get_drvdata(qcom->dwc_dev);
+	struct dwc3	*dwc;
 
 	/* If pm_suspended then let pm_resume take care of resuming h/w */
 	if (qcom->pm_suspended)
 		return IRQ_HANDLED;
 
+	if (qcom->dwc_dev)
+		dwc = platform_get_drvdata(qcom->dwc_dev);
+	else
+		dwc = qcom->dwc;
+
 	/*
 	 * This is safe as role switching is done from a freezable workqueue
 	 * and the wakeup interrupts are disabled as part of resume.
@@ -936,25 +953,33 @@  static int dwc3_qcom_probe(struct platform_device *pdev)
 	if (ret)
 		goto depopulate;
 
-	qcom->mode = usb_get_dr_mode(&qcom->dwc_dev->dev);
+	if (qcom->dwc_dev)
+		qcom->mode = usb_get_dr_mode(&qcom->dwc_dev->dev);
+	else
+		qcom->mode = usb_get_dr_mode(dev);
 
 	/* enable vbus override for device mode */
 	if (qcom->mode != USB_DR_MODE_HOST)
 		dwc3_qcom_vbus_override_enable(qcom, true);
 
-	/* register extcon to override sw_vbus on Vbus change later */
-	ret = dwc3_qcom_register_extcon(qcom);
-	if (ret)
-		goto interconnect_exit;
+	if (qcom->dwc_dev) {
+		/* register extcon to override sw_vbus on Vbus change later */
+		ret = dwc3_qcom_register_extcon(qcom);
+		if (ret)
+			goto interconnect_exit;
+	}
 
 	wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source");
 	device_init_wakeup(&pdev->dev, wakeup_source);
-	device_init_wakeup(&qcom->dwc_dev->dev, wakeup_source);
+	if (qcom->dwc_dev)
+		device_init_wakeup(&qcom->dwc_dev->dev, wakeup_source);
 
 	qcom->is_suspended = false;
-	pm_runtime_set_active(dev);
-	pm_runtime_enable(dev);
-	pm_runtime_forbid(dev);
+	if (qcom->dwc_dev) {
+		pm_runtime_set_active(dev);
+		pm_runtime_enable(dev);
+		pm_runtime_forbid(dev);
+	}
 
 	return 0;
 
@@ -983,6 +1008,9 @@  static void dwc3_qcom_remove(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	int i;
 
+	if (qcom->dwc)
+		dwc3_remove(qcom->dwc);
+
 	device_remove_software_node(&qcom->dwc_dev->dev);
 	if (np)
 		of_platform_depopulate(&pdev->dev);
@@ -998,8 +1026,10 @@  static void dwc3_qcom_remove(struct platform_device *pdev)
 	dwc3_qcom_interconnect_exit(qcom);
 	reset_control_assert(qcom->resets);
 
-	pm_runtime_allow(dev);
-	pm_runtime_disable(dev);
+	if (qcom->dwc_dev) {
+		pm_runtime_allow(dev);
+		pm_runtime_disable(dev);
+	}
 }
 
 static int __maybe_unused dwc3_qcom_pm_suspend(struct device *dev)
@@ -1008,6 +1038,12 @@  static int __maybe_unused dwc3_qcom_pm_suspend(struct device *dev)
 	bool wakeup = device_may_wakeup(dev);
 	int ret;
 
+	if (qcom->dwc) {
+		ret = dwc3_suspend(qcom->dwc);
+		if (ret)
+			return ret;
+	}
+
 	ret = dwc3_qcom_suspend(qcom, wakeup);
 	if (ret)
 		return ret;
@@ -1029,12 +1065,33 @@  static int __maybe_unused dwc3_qcom_pm_resume(struct device *dev)
 
 	qcom->pm_suspended = false;
 
+	if (qcom->dwc) {
+		ret = dwc3_resume(qcom->dwc);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
+static void dwc3_qcom_complete(struct device *dev)
+{
+	struct dwc3_qcom *qcom = dev_get_drvdata(dev);
+
+	if (qcom->dwc)
+		dwc3_complete(qcom->dwc);
+}
+
 static int __maybe_unused dwc3_qcom_runtime_suspend(struct device *dev)
 {
 	struct dwc3_qcom *qcom = dev_get_drvdata(dev);
+	int ret;
+
+	if (qcom->dwc) {
+		ret = dwc3_runtime_suspend(qcom->dwc);
+		if (ret)
+			return ret;
+	}
 
 	return dwc3_qcom_suspend(qcom, true);
 }
@@ -1042,12 +1099,21 @@  static int __maybe_unused dwc3_qcom_runtime_suspend(struct device *dev)
 static int __maybe_unused dwc3_qcom_runtime_resume(struct device *dev)
 {
 	struct dwc3_qcom *qcom = dev_get_drvdata(dev);
+	int ret;
+
+	ret = dwc3_qcom_resume(qcom, true);
+	if (ret)
+		return ret;
 
-	return dwc3_qcom_resume(qcom, true);
+	if (qcom->dwc)
+		return dwc3_runtime_resume(qcom->dwc);
+
+	return 0;
 }
 
 static const struct dev_pm_ops dwc3_qcom_dev_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(dwc3_qcom_pm_suspend, dwc3_qcom_pm_resume)
+	.complete = dwc3_qcom_complete,
 	SET_RUNTIME_PM_OPS(dwc3_qcom_runtime_suspend, dwc3_qcom_runtime_resume,
 			   NULL)
 };