On Tue, 21 Jun 2022 at 23:43, Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: > > On Tue, Jun 21, 2022 at 11:05:17PM +0200, Robert Marko wrote: > > On Tue, 21 Jun 2022 at 22:32, Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: > > > On Tue, Jun 21, 2022 at 01:23:30PM +0200, Robert Marko wrote: > > > > IPQ8074 has one Gen2 and one Gen3 port, currently the Gen2 port will > > > > cause the system to hang as its using DBI registers in the .init > > > > and those are only accesible after phy_power_on(). > > > > > > Is the fact that IPQ8074 has both a Gen2 and a Gen3 port relevant to > > > this patch? I don't see the connection. > > > > Not really, I can remove it from the description as this only affects the Gen2 > > port, Gen3 support is dependant on the IPQ6018 support getting merged as > > it uses the same controller. > > Great, I think unrelated details confuse things. > > > > I see that qcom_pcie_host_init() does: > > > > > > qcom_pcie_host_init > > > pcie->cfg->ops->init(pcie) > > > phy_power_on(pcie->phy) > > > pcie->cfg->ops->post_init(pcie) > > > > > > and that you're moving DBI register accesses from > > > qcom_pcie_init_2_3_3() to qcom_pcie_post_init_2_3_3(). > > > > > > But I also see DBI register accesses in other .init() functions: > > > > > > qcom_pcie_init_2_1_0 > > > qcom_pcie_init_1_0_0 (oddly out of order) > > > qcom_pcie_init_2_3_2 > > > qcom_pcie_init_2_4_0 > > > > > > Why do these accesses not need to be moved? I assume it's because > > > pcie->phy is an optional PHY and phy_power_on() does nothing on those > > > controllers? > > > > As far as I could figure out from QCA-s 5.4 kernel, various commits, > > and QCA-s attempts to solve this already upstream the Gen2 > > controller in IPQ8074 is a bit special and requires the PHY to be > > powered on before DBI could be accessed or else the board will hang > > as it does for me. > > > > I can only assume that this is an IPQ8074-specific quirk and other > > IP-s are not affected like this, so they were not broken. > > > > Whatever the reason, I think the DBI accesses should be done > > > consistently in .post_init(). I see that Dmitry's previous patches > > > removed all those .post_init() functions, but I think the consistency > > > is worth having. > > > > > > Perhaps we could reorder the patches so this patch comes first, moves > > > the DBI accesses into .post_init(), then Dmitry's patches could be > > > rebased on top to drop the clock handling? > > > > > > > So solve this by splitting the DBI read/writes to .post_init. > > > > I am open to anything to get this fixed properly, you are gonna need > > to point me in the right direction as I am really new to PCI. > > Unless there's a reason *not* to move the DBI accesses for other > variants, I think we should move them all to .post_init(). Otherwise > it's just magic -- there's no indication in the code about why IPQ8074 > needs to be different from all the rest. Hi Bjorn, I am not sure how to do it properly, especially for the 2.1.0 IP that IPQ8064 uses and that is already filled with tweaks to get it properly working. As far as I can tell, the reason why it works for all of the others is that they dont use a PHY driver at all (2.1.0 in IPQ8064 and 2.4.0 in IPQ4019), 1.1.0 in APQ8084 appears to be unused completely as its compatible is not used in any of the DTS-es. 2.3.2 in MSM8996 and MSM8998 also use QMP, so I am not sure why these work. > > a0fd361db8e5 appeared in v5.11, so presumably IPQ8074 has been broken > since then? Are there any problem report URLs or references to the > attempts you mentioned above that we could include here? It has been broken since then, it worked on 5.10 when I started poking around IPQ8074 and after backporting the 5.11 commits to get the Gen3 port working it stopped working, and I traced that down to hanging after a DBI write. This appears to be QCA-s attempt to always power on the PHY before the init: https://patchwork.kernel.org/project/linux-pci/patch/1596036607-11877-6-git-send-email-sivaprak@xxxxxxxxxxxxxx/ > > Since folks may want to backport the fix to v5.11, I'd probably do > this patch by itself to make the backport easier, then add a second > patch to move the DBI accesses for all the other variants. > > My personal opinion is that you should do this based on v5.19-rc1, and > then we can rebase Dmitry's patches on top. That would make all the > patches simpler and it would make yours easier to backport. But > that's the sort of thing Dmitry, Stanimir, Andy, and Bjorn A. could > weigh in on. This patch applies to 5.19 as well, though I will send a v4 with the updated description. Like, I said, I am not sure how to move DBI accesses in other IP-s without breaking them. Regards, Robert > > Bjorn H.
