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

[Bjorn Andersson]

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@xxxxxxxxxxx>
> > ---
> >  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