Re: [PATCH v2 10/10] PCI: qcom: Implement shutdown() callback to properly reset the endpoint devices

[Niklas Cassel <cassel@kernel.org>]

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