On jeu., mars 21, 2024 at 11:34, Caleb Connolly <caleb.conno...@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 <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. Right, sorry I missed that. Thanks for pointing it out to me! >> >> [1] >> https://lore.kernel.org/r/all/171101299073.1017001.16411913317437946645.b4...@baylibre.com/ >> > > -- > // Caleb (they/them)