Re: [PATCH 1/5] usb: dwc3-generic: implement Qualcomm wrapper

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

Hi Caleb, Marek,

On jeu., mars 21, 2024 at 06:34, Marek Vasut <marex@denx.de> wrote:

> On 3/13/24 7:22 PM, Caleb Connolly wrote:
>
> [...]
>
>>>> +static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset,
>>>> u32 val)
>>>> +{
>>>> +    u32 reg;
>>>> +
>>>> +    reg = readl(base + offset);
>>>> +    reg |= val;
>>>> +    writel(reg, base + offset);
>>>
>>> Use setbits_le32() .
>>>
>>>> +    /* ensure that above write is through */
>>>> +    readl(base + offset);
>>>
>>> Is this needed ?
>> 
>> I honestly don't know, this is copied from the Linux driver and it seems
>> to be very defensively written. I doubt it's strictly necessary.
>
> Does git log indicate anything ?
>
> I suspect this is some sort of barrier .
>
> [...]
>
>>>> +/* For controllers running without superspeed PHYs */
>>>> +static void dwc3_qcom_select_utmi_clk(void __iomem *qscratch_base)
>>>> +{
>>>> +    /* Configure dwc3 to use UTMI clock as PIPE clock not present */
>>>> +    dwc3_qcom_setbits(qscratch_base, QSCRATCH_GENERAL_CFG,
>>>> +              PIPE_UTMI_CLK_DIS);
>>>> +
>>>> +    udelay(500);
>>>
>>> Isn't there some possibility to poll for completion instead of fixed
>>> delay ? If so, use wait_for_bit or some such .
>> 
>> Not that I'm aware of, no. I think this hardware just has a blanket
>> "writes take X bus cycles to complete" rule or something. It's totally
>> possible that this code was originally written this way to work around
>> some issues on an FPGA prototype or something. Everything seems to still
>> work if I remove the delays so I'll drop them...
>
> Could you possibly ask someone ?
>
> [...]
>
>>>>    static int dwc3_rk_glue_get_ctrl_dev(struct udevice *dev, ofnode *node)
>>>>    {
>>>>        *node = dev_ofnode(dev);
>>>> @@ -506,6 +599,10 @@ static int dwc3_glue_reset_init(struct udevice *dev,
>>>>        else if (ret)
>>>>            return ret;
>>>>    +    if (device_is_compatible(dev, "qcom,dwc3")) {
>>>> +        reset_assert_bulk(&glue->resets);
>>>> +        udelay(500);
>>>
>>> Why this delay here ?
>> 
>> According to the docs, the reset should be asserted for at least 6 sleep
>> clock cycles, that's ~200us on sdm845, but it can vary by platform.
>
> A comment in the code would be nice.
>
> Sorry for the abysmal delay in replies.
>
> btw. the new version of this series is still OK to go in, unless you 
> want to fill in the comments. They can also go in in separate follow up 
> patch.

I'm interested by the answers above as well. As I took in the series [1] (to
avoid delaying it too much), please consider a follow up patch to add a
comment.

[1] https://lore.kernel.org/r/all/171101299073.1017001.16411913317437946645.b4-ty@baylibre.com/