On Tue, Aug 13, 2024 at 02:33:56PM -0400, Frank Li wrote:
> On Sun, Aug 11, 2024 at 08:12:03PM -0700, Bjorn Andersson wrote:
> > From: Bjorn Andersson <quic_bjorande@xxxxxxxxxxx>
> > drivers/usb/dwc3/dwc3-qcom.c | 310 +++++++++++++++++++++++++++++++++++--------
[..]
> > @@ -302,25 +306,16 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom)
> > /* Only usable in contexts where the role can not change. */
> > static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom)
> > {
> > - struct dwc3 *dwc;
> > -
> > - /*
> > - * FIXME: Fix this layering violation.
> > - */
> > - dwc = platform_get_drvdata(qcom->dwc3);
> > -
> > - /* Core driver may not have probed yet. */
> > - if (!dwc)
> > - return false;
> > + struct dwc3 *dwc = qcom->dwc;
> >
> > return dwc->xhci;
>
> dwc only use once.
>
> return qcom->dwc->xhci?
>
I like it, thanks for the suggestion.
> > }
> >
[..]
> > +/* Convert dev's DeviceTree representation from qcom,dwc3 to qcom,snps-dwc3 binding */
> > +static int dwc3_qcom_convert_legacy_dt(struct device *dev)
> > +{
> > + struct device_node *qcom = dev->of_node;
> > + struct device_node *dwc3;
> > + struct property *prop;
> > + int ret = 0;
> > +
> > + dwc3 = of_get_compatible_child(qcom, "snps,dwc3");
> > + if (!dwc3)
> > + return 0;
> > +
> > + /* We have a child node, but no support for dynamic OF */
> > + if (!IS_ENABLED(CONFIG_OF_DYNAMIC))
> > + return -EINVAL;
> > +
> > + for_each_property_of_node(dwc3, prop) {
> > + if (!strcmp(prop->name, "compatible"))
> > + ;
> > + else if (!strcmp(prop->name, "reg"))
> > + ret = dwc3_qcom_legacy_update_reg(qcom, dwc3);
> > + else if (!strcmp(prop->name, "interrupts"))
> > + ret = dwc3_qcom_legacy_convert_interrupts(qcom, prop);
> > + else
> > + ret = dwc3_qcom_legacy_migrate_prop(qcom, prop);
> > }
> >
> > -node_put:
> > - of_node_put(dwc3_np);
> > + if (ret < 0)
> > + goto err_node_put;
> > +
> > + ret = dwc3_qcom_legacy_migrate_child(qcom, dwc3, "port");
> > + if (ret)
> > + goto err_node_put;
> > +
> > + ret = dwc3_qcom_legacy_migrate_child(qcom, dwc3, "ports");
> > + if (ret)
> > + goto err_node_put;
> > +
> > + of_detach_node(dwc3);
> > + of_node_put(dwc3);
> >
> > + return 0;
> > +
> > +err_node_put:
> > + of_node_put(dwc3);
> > return ret;
> > }
>
> Look like you copy children dwc3's property into current glue node.
> Can you passdown dwc3's node into dwc3_probe(), let dwc3_probe to handle
> these, Or move it into dwc3-core. otherwise, if imx want to do the same
> thing, the the same code will be dupicated.
>
I tried that, as it would have saved me from having to do the dynamic
rewrite.
But the dwc3 core and host are full of device_property_read*(),
phy_get(), platform_get_irq() etc which operates on the dwc->dev.
I think it can be done, but this felt like a cleaner outcome, in
particular once we transition the DeviceTree source.
As you say, there should be a fair amount of room for duplication here,
so perhaps we can move that to a "glue.c" and share it?
[..]
> > @@ -773,10 +937,14 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> > goto reset_assert;
> > }
> >
> > - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + ret = of_address_to_resource(np, 0, &res);
> > + if (ret < 0)
> > + goto clk_disable;
> > + res.end = res.start + SDM845_QSCRATCH_BASE_OFFSET;
> >
> > - qcom->qscratch_base = devm_ioremap_resource(dev, res);
> > + qcom->qscratch_base = devm_ioremap(dev, res.end, SDM845_QSCRATCH_SIZE);
> > if (IS_ERR(qcom->qscratch_base)) {
> > + dev_err(dev, "failed to map qscratch region: %pe\n", qcom->qscratch_base);
>
> dev_err_probe()?
>
Sounds good.
Thank you,
Bjorn
