On Wed, Apr 03, 2024 at 07:02:17PM +0530, Manivannan Sadhasivam wrote: > On Tue, Apr 02, 2024 at 01:18:54PM +0200, Niklas Cassel wrote: > > On Mon, Apr 01, 2024 at 09:20:36PM +0530, Manivannan Sadhasivam wrote: > > > PCIe host controller drivers are supposed to properly reset the endpoint > > > devices during host shutdown/reboot. Currently, Qcom driver doesn't do > > > anything during host shutdown/reboot, resulting in both PERST# and refclk > > > getting disabled at the same time. This prevents the endpoint device > > > firmware to properly reset the state machine. Because, if the refclk is > > > cutoff immediately along with PERST#, access to device specific registers > > > within the endpoint will result in a firmware crash. > > > > > > To address this issue, let's call qcom_pcie_host_deinit() inside the > > > shutdown callback, that asserts PERST# and then cuts off the refclk with a > > > delay of 1ms, thus allowing the endpoint device firmware to properly > > > cleanup the state machine. > > > > Hm... a QCOM EP device could be attached to any of the PCIe RC drivers that > > we have in the kernel, so it seems a bit weird to fix this problem by > > patching the QCOM RC driver only. > > > > Which DBI call is it that causes this problem during perst assert on EP side? > > > > I assume that it is pci-epf-test:deinit() callback that calls > > pci_epc_clear_bar(), which calls dw_pcie_ep_clear_bar(), which will both: > > -clear local data structures, e.g. > > ep->epf_bar[bar] = NULL; > > ep->bar_to_atu[bar] = 0; > > > > but also call: > > __dw_pcie_ep_reset_bar() > > dw_pcie_disable_atu() > > > > > > Do we perhaps need to redesign the .deinit EPF callback? > > > > Considering that we know that .deinit() will only be called on platforms > > where there will be a fundamental core reset, I guess we could do something > > like introduce a __dw_pcie_ep_clear_bar() which will only clear the local > > data structures. (It might not need to do any DBI writes, since the > > fundamental core reset should have reset all values.) > > > > Or perhaps instead of letting pci_epf_test_epc_deinit() call > > pci_epf_test_clear_bar()/__pci_epf_test_clear_bar() directly, perhaps let > > pci_epf_test_epc_deinit() call add a .deinit()/.cleanup() defined in the > > EPC driver. > > > > This EPC .deinit()/.cleanup() callback would then only clear the > > local data structures (no DBI writes...). > > > > Something like that? > > > > It is not just about the EPF test driver. A function driver may need to do many > things to properly reset the state machine. Like in the case of MHI driver, it > needs to reset channel state, mask interrupts etc... and all requires writing to > some registers. So certainly there should be some time before cutting off the > refclk. I was more thinking that perhaps we should think of .deinit() as in how dw_pcie_ep_init() used to be. It was not allowed to have any DBI writes. (The DBI writes were all in dw_pcie_ep_init_complete()). So perhaps we could define that a EPF .deinit() callback is not allowed to have any DBI writes. If we take qcom-ep as an example, as soon as you get a PERST assertion the qcom-ep driver calls notify_deinit(), then asserts the reset control, disables clocks and regulators. Since the PCIe core is held in reset, the hardware is in a well defined state, no? Sure, the data structures e.g. bar_to_iatu[], etc., might be out of sync, but these could be memset():ed no? Since this is a fundamental reset, all registers should be reset to their default state (once reset is deasserted). For a real PCIe card, if you assert + msleep(100) + deassert PERST, surely the endpoint is supposed to be in a good/well defined state, regardless if he REFCLK was cutoff at the exact time as PERST was asserted or not? I would assume that we would want a PCI EPF driver to behave the same way, if possible. Kind regards, Niklas
