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

[Caleb Connolly]

Hi,

On 21/03/2024 09:25, Mattijs Korpershoek wrote:
> Hi Caleb, Marek,
> 
> On jeu., mars 21, 2024 at 06:34, Marek Vasut <marex at 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.
> 
> [1] https://lore.kernel.org/r/all/171101299073.1017001.16411913317437946645.b4-ty@baylibre.com/
> 

-- 
// Caleb (they/them)