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? I thought the LINK_DOWN event can only change LTSSM and some status registers. > > Currently, doing a bind + start link + stop link + unbind + bind + start link, > will not reinitialize all DBI writes for a driver. > (This is also true for a core_init_notifier driver, but there start/stop link > is a no-op, other than enabling the GPIO irq handler.) > > What this will do during the second bind: > -allocate BARs (DBI writes) > -set iATUs (DBI writes) > > What it will not redo is: > -what is done by dw_pcie_ep_init() - which is fine, because this is only > supposed to allocate stuff or get stuff from DT, not actually touch HW > registers. > -what is done by dw_pcie_ep_init_complete() - I think this should be done > since it does DBI writes. E.g. clearing PTM root Cap and fixing Resizable > BAR Cap, calling dw_pcie_setup() which sets max link width etc. > > > > I guess the counter argument could be that... if you need to re-initialize > DBI registers, you could probably do a: > stop link + unbind EPF driver + unbind PCIe-EP driver + bind PCIe-EP driver > + bind EPF driver + start link.. > (But I don't need all that if I use a .core_init_notifier and just toggle > PERST). > > Of course, toggling PERST and starting/stopping the link via sysfs is not > exactly the same thing... > > For me personally, the platform I use do not require an external refclk to > write DBI settings, but I would very much like the HW to be re-initialized > either when stopping/starting the link in sysfs, or by toggling PERST, or > both. > > I think that I will just add a .core_init_notifier to my WIP driver, > even though I do not require an external refclk, and simply call > dw_pcie_ep_init_complete() when receiving a PERST deassert, because I want > _all_ settings (DBI writes) to be reinitialized. > (If we receive a PERST reset request, I think it does make sense to obey > and actually reset/re-write all settings.) > No, you should not add core_init_notifier if your platform receives an active refclk (although my heard wants to have an unified behavior across all platforms). We should fix the DWC core code if you clarify my above suspicion. - Mani > A sysfs stop/start link would still not reinitialize everything... > I think it would be good if the DWC EP driver actually did this too, > but I'm fine if you consider it out of scope of your patch series. > > > Kind regards, > Niklas -- மணிவண்ணன் சதாசிவம்
