Hi,
On 21/03/2024 09:25, Mattijs Korpershoek wrote:
> 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 ?
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...@baylibre.com/
>
--
// Caleb (they/them)
