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

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

On jeu., mars 21, 2024 at 11:34, Caleb Connolly <caleb.connolly@linaro.org> wrote:

> Hi,
>
> On 21/03/2024 09:25, Mattijs Korpershoek wrote:
>> 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 ?
>
> Nope :/ it's there from when the driver was first added.
>>>
>>> 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 ?
>
> Yeah I'll ask around, I'm not confident I'll find an answer though.
>>>
>>> [...]
>>>
>>>>>>    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.
>
> The v4 you picked up has a comment explaining this.

Right, sorry I missed that. Thanks for pointing it out to me!

>> 
>> [1] https://lore.kernel.org/r/all/171101299073.1017001.16411913317437946645.b4-ty@baylibre.com/
>> 
>
> -- 
> // Caleb (they/them)