Re: [PATCH v2 1/4] PCI: designware-ep: Allow pci_epc_set_bar() update inbound map address

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Wed, Dec 20, 2023 at 02:14:53PM +0900, Damien Le Moal wrote:
> On 12/20/23 02:59, Manivannan Sadhasivam wrote:
> >>>>> from dw_pcie_ep_set_bar(), also needs to be dropped, so that the iATU
> >>>>> settings will be re-written for platforms with core_init_notifiers.
> >>>>>
> >>>>> Right now, for a platform with a core_init_notifier, if you run
> >>>>> pci_endpoint_test + reboot the RC (so that PERST is asserted + deasserted),
> >>>>> and then run pci_endpoint_test again, then I'm quite sure that
> >>>>> pci_endpoint_test will not pass the second time (because iATU settings
> >>>>> were not rewritten after reset).
> >>>>>
> >>>>> It would be nice if Mani or Vidya could confirm this.
> >>
> >> So problem 2 out of 2 introduced by the patch in $subject is that
> >> DWC drivers with a .core_init_notifier, will perform a reset_control_assert()
> >> to reset the core (which will reset both sticky and non-sticky registers),
> >> which means that the early return in dw_pcie_ep_set_bar():
> >> https://github.com/torvalds/linux/blob/v6.7-rc5/drivers/pci/controller/dwc/pcie-designware-ep.c#L268-L269
> >>
> >> while returning after the iATU settings have been written,
> >> it will return before:
> >>
> >> 	dw_pcie_writel_dbi2(pci, reg_dbi2, lower_32_bits(size - 1));
> >> 	dw_pcie_writel_dbi(pci, reg, flags);
> >>
> >> Which means that, for drivers with a .core_init_notifier, BARx_REG and
> >> BARx_MASK registers will not be written. This means that they will have
> >> reset values for these registers, which means that e.g. the BAR_SIZE
> >> (which is defined by BARx_MASK) might be incorrect for these platforms
> >> because this function returns early.
> >>
> >> I will not send a fix for this problem, I will leave that to you, or Mani,
> >> or Vidya, and hope that people are happy that I simply reported this issue.
> >>
> > 
> > The fix for this issue shouldn't be in the DWC driver but rather in the EPF
> > drivers. Because, EPF drivers are the ones calling pci_epc_set_bar() during
> > bind(). So they have to properly cleanup the resources once the perst assertion
> > happens. This issue also applies to other resources such as DMA channels.
> > 
> > The problem here is, there is a disconnect between DWC (EPC) and EPF drivers.
> > When the perst assertion happens, the event is not passed to EPF drivers
> > resulting in the EPF drivers having no knowledge that they need to give up the
> > resources.
> > 
> > We need to fix this in a sane way and I'm looking into it.
> > 
> > But I really appreciate your finding here and in other thread where we discussed
> > similar issue.
> 
> We have core_init and link_up notifiers for EPF drivers. Adding link_down and
> core_exit notifiers would make a lot of sense :) A core_reset notifier could
> also be a solution.
> 

Yeah, I'm thinking along those lines.

> Adding that would also help EPF drivers to handle links going down temporarily
> (which can happen). Right now, an EPF driver can only deal with such event by
> tracking if it gets multiple link_up events.
> 

There is already link_down notifier that I introduced a while back. But it is
only used by MHI_EPF driver.

- Mani

> 
> -- 
> Damien Le Moal
> Western Digital Research
> 
> 

-- 
மணிவண்ணன் சதாசிவம்