Received-SPF: pass (google.com: domain of linux-kernel+bounces-19934-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 8 Jan 2024 10:02:23 -0800
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
Message-ID: <20240108180223.GK1315173@hu-bjorande-lv.qualcomm.com>
References: <20231016-dwc3-refactor-v1-0-ab4a84165470@quicinc.com>
 <20231016-dwc3-refactor-v1-6-ab4a84165470@quicinc.com>
 <ZV3xmW0fDWY5-6qZ@hovoldconsulting.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
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