lkml.org 
[lkml]   [2020]   [Jun]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
    From
    Date

    On 6/16/2020 1:12 AM, Matthias Kaehlcke wrote:
    > On Thu, Jun 04, 2020 at 04:16:31AM -0700, Stephen Boyd wrote:
    >> Quoting Sandeep Maheswaram (Temp) (2020-06-04 02:43:09)
    >>> On 6/3/2020 11:06 PM, Stephen Boyd wrote:
    >>>> Quoting Sandeep Maheswaram (2020-03-31 22:15:43)
    >>>>> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
    >>>>> index 1dfd024..d33ae86 100644
    >>>>> --- a/drivers/usb/dwc3/dwc3-qcom.c
    >>>>> +++ b/drivers/usb/dwc3/dwc3-qcom.c
    >>>>> @@ -285,6 +307,101 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
    >>>>> return 0;
    >>>>> }
    >>>>>
    >>>>> +
    >>>>> +/**
    >>>>> + * dwc3_qcom_interconnect_init() - Get interconnect path handles
    >>>>> + * @qcom: Pointer to the concerned usb core.
    >>>>> + *
    >>>>> + */
    >>>>> +static int dwc3_qcom_interconnect_init(struct dwc3_qcom *qcom)
    >>>>> +{
    >>>>> + struct device *dev = qcom->dev;
    >>>>> + int ret;
    >>>>> +
    >>>>> + if (!device_is_bound(&qcom->dwc3->dev))
    >>>>> + return -EPROBE_DEFER;
    >>>> How is this supposed to work? I see that this was added in an earlier
    >>>> revision of this patch series but there isn't any mention of why
    >>>> device_is_bound() is used here. It would be great if there was a comment
    >>>> detailing why this is necessary. It sounds like maximum_speed is
    >>>> important?
    >>>>
    >>>> Furthermore, dwc3_qcom_interconnect_init() is called by
    >>>> dwc3_qcom_probe() which is the function that registers the device for
    >>>> qcom->dwc3->dev. If that device doesn't probe between the time it is
    >>>> registered by dwc3_qcom_probe() and this function is called then we'll
    >>>> fail dwc3_qcom_probe() with -EPROBE_DEFER. And that will remove the
    >>>> qcom->dwc3->dev device from the platform bus because we call
    >>>> of_platform_depopulate() on the error path of dwc3_qcom_probe().
    >>>>
    >>>> So isn't this whole thing racy and can potentially lead us to a driver
    >>>> probe loop where the wrapper (dwc3_qcom) and the core (dwc3) are probing
    >>>> and we're trying to time it just right so that driver for dwc3 binds
    >>>> before we setup interconnects? I don't know if dwc3 can communicate to
    >>>> the wrapper but that would be more of a direct way to do this. Or maybe
    >>>> the wrapper should try to read the DT property for maximum speed and
    >>>> fallback to a worst case high bandwidth value if it can't figure it out
    >>>> itself without help from dwc3 core.
    >>>>
    >>> This was added in V4 to address comments from Matthias in V3
    >>>
    >>> https://patchwork.kernel.org/patch/11148587/
    >>>
    >> Yes, that why I said:
    >>
    >> "I see that this was added in an earlier
    >> revision of this patch series but there isn't any mention of why
    >> device_is_bound() is used here. It would be great if there was a comment
    >> detailing why this is necessary. It sounds like maximum_speed is
    >> important?"
    >>
    >> Can you please respond to the rest of my email?
    > I agree with Stephen that using device_is_bound() isn't a good option
    > in this case, when I suggested it I wasn't looking at the big picture
    > of how probing the core driver is triggered, sorry about that.
    >
    > Reading the speed from the DT with usb_get_maximum_speed() as Stephen
    > suggests would be an option, the inconvenient is that we then
    > essentially require the property to be defined, while the core driver
    > gets a suitable value from hardware registers. Not sure if the wrapper
    > driver could read from the same registers.
    >
    > One option could be to poll device_is_bound() for 100 ms (or so), with
    > sleeps between polls. It's not elegant but would probably work if we
    > don't find a better solution.
    if (np)
            ret = dwc3_qcom_of_register_core(pdev);
        else
            ret = dwc3_qcom_acpi_register_core(pdev);

        if (ret) {
            dev_err(dev, "failed to register DWC3 Core, err=%d\n", ret);
            goto depopulate;
        }

        ret = dwc3_qcom_interconnect_init(qcom);
        if (ret)
            goto depopulate;

        qcom->mode = usb_get_dr_mode(&qcom->dwc3->dev);

    Before calling dwc3_qcom_interconnect_init we are checking

        if (ret) {
            dev_err(dev, "failed to register DWC3 Core, err=%d\n", ret);
            goto depopulate;
        }

    Doesn't  this condition confirm the core driver is probed?

    --
    QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

    \
     
     \ /
      Last update: 2020-06-16 06:54    [W:5.276 / U:0.048 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site