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

From: Bjorn Andersson <quic_bjorande@quicinc.com>
To: Johan Hovold <johan@kernel.org>
Cc: Bjorn Andersson <andersson@kernel.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Wesley Cheng <quic_wcheng@quicinc.com>,
	Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
	Felipe Balbi <balbi@kernel.org>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	<linux-arm-msm@vger.kernel.org>, <linux-usb@vger.kernel.org>,
	<devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
Subject: Re: [PATCH 06/12] usb: dwc3: qcom: Add dwc3 core reference in driver state
Date: Mon, 8 Jan 2024 10:02:23 -0800	[thread overview]
Message-ID: <20240108180223.GK1315173@hu-bjorande-lv.qualcomm.com> (raw)
In-Reply-To: <ZV3xmW0fDWY5-6qZ@hovoldconsulting.com>

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