On Mon, Mar 04, 2024 at 11:51:04AM +0100, Niklas Cassel wrote: > On Mon, Mar 04, 2024 at 01:47:13PM +0530, Manivannan Sadhasivam wrote: > > On Thu, Feb 29, 2024 at 01:40:29PM +0100, Niklas Cassel wrote: > > > On Sat, Feb 24, 2024 at 12:24:09PM +0530, Manivannan Sadhasivam wrote: > > > > > > Since e.g. qcom-ep.c does a reset_control_assert() during perst > > > assert/deassert, which should clear sticky registers, I think that > > > you should let dw_pcie_ep_cleanup() clean up the BARs using > > > dw_pcie_ep_clear_bar(). > > > > > > > As I mentioned earlier, it is the job of the EPF drivers to clear the BARs since > > they allocate them. I'm trying to reduce the implicit resetting wherever we > > could. > > > > The proper fix is to add the LINK_DOWN callback to EPF drivers and do cleanup. > > I'm planning to submit a series for that after this one. > > Currently, pci-epf-test allocates memory for the BARs in .bind(). > Likewise it frees the memory for the BARs in .unbind(). > > AFAICT, most iATU registers, and most BAR registers are sticky registers, > so they will not get reset on link down. > (The currently selected BAR size, in case of Resizable BAR is an exception.) > > That means that even on link down, we do not need to free the memory, > or change the iATU settings. (This applies to all drivers.) > > > > However, on PERST (for the drivers call dw_pcie_ep_cleanup()), they call > reset_control_assert(), so they will clear sticky registers, which means > that they need to at least re-write the iATU and BAR registers. > (I guess they could free + allocate the memory for the BARs again, > but I don't think that is strictly necessary.) > That is why I suggested that you call dw_pcie_ep_clear_bar() from > dw_pcie_ep_cleanup(). > Sorry, I keep assuming the flow w.r.t PERST# supported platforms :/ My bad! > > > If you free the memory for the BARs in link_down() (this callback exists > for many drivers, even drivers without a PERST handler), where are you > supposted to alloc the memory for the BARs again? > > Allocating them at link_up() is too late (because as soon as the link is > up, the host is allowed to enumerate the EP BARs.) The proper place is to > allocate them when receiving PERST, but not all drivers have a PERST handler. > > (My understanding is that 1) PERST assert 2) PERST deassert 3) link is up.) > > > > unbind() undos what was done in bind(), so shouldn't link_down() undo what was > done in link_up()? With that logic, if you move the alloc to .core_init(), > should we perhaps have a .core_deinit() callback for EPF drivers? > (I guess only drivers which perform a reset during PERST would call this.) > > But considering that free+alloc is not strictly needed, why not just keep > the allocation + free in .bind()/.unbind() ? > (To avoid the need to create a .core_deinit()), and let dw_pcie_ep_cleanup() > call dw_pcie_ep_clear_bar() ? > > I guess my point is that it seems a bit pointless for drivers that do not > clear sticky registers to free+alloc memory on link down, for no good > reason. (Memory might get fragmented over time, so it might not be possible > to perform a big allocation after the device has been running for a really > long time.) > > > > So I'm thinking that we either > 1) Keep the alloc/free in bind/unbind, and let dw_pcie_ep_cleanup() call > dw_pcie_ep_clear_bar(), > or > 2) Introduce a .deinit_core() callback which will free the BARs. > (Because I don't see how you will (re-)allocate memory for all drivers > if you free the memory in link_down().) > I think option 2 is the better solution. In my view, calling dw_pcie_ep_clear_bar() from EPC drivers is a layering violation since the allocation happens from EPF drivers. So clearing the BARs during the deinit() callback that gets called when PERST# assert happens is the way to go. - Mani -- மணிவண்ணன் சதாசிவம்
