Re: [PATCH v9 08/10] PCI: dwc: ep: Add a generic dw_pcie_ep_linkdown() API to handle LINK_DOWN event

[Niklas Cassel <cassel@kernel.org>]

On Fri, Mar 08, 2024 at 03:16:06PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Mar 08, 2024 at 09:56:33AM +0100, Niklas Cassel wrote:
> > On Fri, Mar 08, 2024 at 11:11:52AM +0530, Manivannan Sadhasivam wrote:
> > > On Thu, Mar 07, 2024 at 10:43:19PM +0100, Niklas Cassel wrote:
> > > > On Mon, Mar 04, 2024 at 02:52:20PM +0530, Manivannan Sadhasivam wrote:
> > > > > The PCIe link can go to LINK_DOWN state in one of the following scenarios:
> > > > > 
> > > > > 1. Fundamental (PERST#)/hot/warm reset
> > > > > 2. Link transition from L2/L3 to L0
> > > > > 
> > > > > In those cases, LINK_DOWN causes some non-sticky DWC registers to loose the
> > > > > state (like REBAR, PTM_CAP etc...). So the drivers need to reinitialize
> > > > > them to function properly once the link comes back again.
> > > > > 
> > > > > This is not a problem for drivers supporting PERST# IRQ, since they can
> > > > > reinitialize the registers in the PERST# IRQ callback. But for the drivers
> > > > > not supporting PERST#, there is no way they can reinitialize the registers
> > > > > other than relying on LINK_DOWN IRQ received when the link goes down. So
> > > > > let's add a DWC generic API dw_pcie_ep_linkdown() that reinitializes the
> > > > > non-sticky registers and also notifies the EPF drivers about link going
> > > > > down.
> > > > > 
> > > > > This API can also be used by the drivers supporting PERST# to handle the
> > > > > scenario (2) mentioned above.
> > > > > 
> > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > > ---
> > > > >  drivers/pci/controller/dwc/pcie-designware-ep.c | 111 ++++++++++++++----------
> > > > >  drivers/pci/controller/dwc/pcie-designware.h    |   5 ++
> > > > >  2 files changed, 72 insertions(+), 44 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > > index 278bdc9b2269..fed4c2936c78 100644
> > > > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > > @@ -14,14 +14,6 @@
> > > > >  #include <linux/pci-epc.h>
> > > > >  #include <linux/pci-epf.h>
> > > > >  
> > > > > -void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
> > > > > -{
> > > > > -	struct pci_epc *epc = ep->epc;
> > > > > -
> > > > > -	pci_epc_linkup(epc);
> > > > > -}
> > > > > -EXPORT_SYMBOL_GPL(dw_pcie_ep_linkup);
> > > > > -
> > > > >  void dw_pcie_ep_init_notify(struct dw_pcie_ep *ep)
> > > > >  {
> > > > >  	struct pci_epc *epc = ep->epc;
> > > > > @@ -603,19 +595,56 @@ static unsigned int dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap)
> > > > >  	return 0;
> > > > >  }
> > > > >  
> > > > > +static void dw_pcie_ep_init_non_sticky_registers(struct dw_pcie *pci)
> > > > > +{
> > > > > +	unsigned int offset, ptm_cap_base;
> > > > > +	unsigned int nbars;
> > > > > +	u32 reg, i;
> > > > > +
> > > > > +	offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR);
> > > > > +	ptm_cap_base = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_PTM);
> > > > > +
> > > > > +	dw_pcie_dbi_ro_wr_en(pci);
> > > > > +
> > > > > +	if (offset) {
> > > > > +		reg = dw_pcie_readl_dbi(pci, offset + PCI_REBAR_CTRL);
> > > > > +		nbars = (reg & PCI_REBAR_CTRL_NBAR_MASK) >>
> > > > > +			PCI_REBAR_CTRL_NBAR_SHIFT;
> > > > > +
> > > > > +		for (i = 0; i < nbars; i++, offset += PCI_REBAR_CTRL)
> > > > > +			dw_pcie_writel_dbi(pci, offset + PCI_REBAR_CAP, 0x0);
> > > > 
> > > > If you look at PCI_REBAR_CAP, you will see that it is sticky,
> > > > but you have to actually read the databook to see that:
> > > > 
> > > > "The RESBAR_CTRL_REG_BAR_SIZE field is automatically updated
> > > > when you write to RESBAR_CAP_REG_0_REG through the DBI."
> > > > 
> > > > So the reason why we need to write this register, even though
> > > > it is sticky, is to update the RESBAR_CTRL_REG_BAR_SIZE register,
> > > > which is not sticky :)
> > > > 
> > > > (Perhaps we should add that as a comment?)
> > > > 
> > > 
> > > Yeah, makes sense.
> > 
> > Note that I add a (unrelated) comment related to REBAR_CAP in this patch:
> > https://lore.kernel.org/linux-pci/20240307111520.3303774-1-cassel@kernel.org/T/#u
> > 
> > But once we move/add code to dw_pcie_ep_init_non_sticky_registers(), I think
> > that it might be a good "rule" to have a small comment for each write in
> > dw_pcie_ep_init_non_sticky_registers() which explains why the code should be
> > in dw_pcie_ep_init_non_sticky_registers() instead of dw_pcie_ep_init_registers(),
> > even if it just a small:
> > 
> > /* Field PCI_XXX_YYY.ZZZ is non-sticky */
> > writel_dbi(pci, offset + PCI_XXX_YYY, 0);
> > 
> 
> Why? The function name itself suggests that we are reinitializing non-sticky
> registers. So a comment for each write is overkill.

So that you know which field it is in the register that you are writing which
you care about (which field it is that is non-sticky).

But I see your point, perhaps it is overkill.


Kind regards,
Niklas