Hi Caleb, Marek,
On jeu., mars 21, 2024 at 06:34, Marek Vasut <ma...@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...@baylibre.com/
