Re: [PATCH v7 1/2] PCI: designware-ep: Fix DBI access before core init

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

On Sat, Dec 23, 2023 at 01:52:01AM +0000, Niklas Cassel wrote:
> On Thu, Nov 30, 2023 at 07:27:57PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Nov 30, 2023 at 11:22:30AM +0000, Niklas Cassel wrote:
> > > On Thu, Nov 30, 2023 at 12:08:00PM +0530, Manivannan Sadhasivam wrote:
> > > > 
> > > > Looking at this issue again, I think your statement may not be correct. In the
> > > > stop_link() callback, non-core_init_notifier platforms are just disabling the
> > > > LTSSM and enabling it again in start_link(). So all the rest of the
> > > > configurations (DBI etc...) should not be affected during EPF bind/unbind.
> > > 
> > > Sorry for confusing you.
> > > 
> > > When toggling PERST on a driver with a core_init_notifier, you will call
> > > dw_pcie_ep_init_complete() - which will initialize DBI settings, and then
> > > stop/start the link by deasserting/asserting LTSSM.
> > > (perst_assert()/perst_deassert() is functionally the equivalent to start_link()/
> > > stop_link() on non-core_init_notifier platforms.)
> > > 
> > > 
> > > For drivers without a core_init_notifier, they currently don't listen to PERST
> > > input GPIO.
> > > Stopping and starting the link will not call dw_pcie_ep_init_complete(),
> > > so it will not (re-)initialize DBI settings.
> > > 
> > > 
> > > My problem is that a bunch of hardware registers gets reset when receiving
> > > a link-down reset or hot reset. It would be nice to write all DBI settings
> > > when starting the link. That way the link going down for a millisecond, and
> > > gettting immediately reestablished, will not be so bad. A stop + start link
> > > will rewrite the settings. (Just like toggling PERST rewrites the settings.)
> > > 
> > 
> > I got slightly confused by this part. So you are saying that when a user stops
> > the controller through configfs, EP receives the LINK_DOWN event and that causes
> > the EP specific registers (like DBI) to be reset?
> 
> Sorry for taking time to reply.
> (I wanted to make sure that I was 100% understanding what was going on.)
> 
> So to give you a clear example:
> If you:
> 1) start the EP, start the pci-epf-test
> 2) start the RC
> 3) run pci-epf-test
> 4) reboot the RC
> 
> this will trigger a link-down reset IRQ on the EP side (link_req_rst_not).
> 

Right. This is the sane RC reboot scenario. Although, there is no guarantee
that the EP will get LINK_DOWN event before perst_assert (I've seen this with
some RC platforms).

But can you confirm whether your EP is receiving PERST assert/deassert when RC
reboots?

- Mani

> 
> > 
> > I thought the LINK_DOWN event can only change LTSSM and some status registers.
> 
> link_req_rst_not will assert link_down_rst_n, which will assert
> non_sticky_rst_n. So all non-sticky registers will be reset.
> 
> Some thing that has been biting me is that:
> While advertized Resizable BAR sizes are sticky,
> the currently used resizable BAR size is non-sticky.
> 
> So a reboot of the RC will reset the BAR sizes to reset values
> (which on the SoC I'm using is 1 GB...)
> 
> So, there is an advantage of having a .core_init_notifier,
> as this will allow you to reboot the RC without any problems.
> 
> I guess one solution, for DWC glue drivers that don't strictly
> need a refclock is to just call dw_pcie_ep_init_complete() when
> receiving the link-down IRQ (since when you get that, the core
> has already reset non-sticky registers to reset values), and
> then when you get a link-up again, you have already re-inited
> the registers that got reset.
> 
> 
> Kind regards,
> Niklas

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