| Message ID | 20240304-pci-dbi-rework-v9-8-29d433d99cda@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: dwc: ep: Fix DBI access failure for drivers requiring refclk from host | expand |
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?) > + } > + > + /* > + * PTM responder capability can be disabled only after disabling > + * PTM root capability. > + */ > + if (ptm_cap_base) { > + dw_pcie_dbi_ro_wr_en(pci); > + reg = dw_pcie_readl_dbi(pci, ptm_cap_base + PCI_PTM_CAP); > + reg &= ~PCI_PTM_CAP_ROOT; > + dw_pcie_writel_dbi(pci, ptm_cap_base + PCI_PTM_CAP, reg); > + > + reg = dw_pcie_readl_dbi(pci, ptm_cap_base + PCI_PTM_CAP); > + reg &= ~(PCI_PTM_CAP_RES | PCI_PTM_GRANULARITY_MASK); > + dw_pcie_writel_dbi(pci, ptm_cap_base + PCI_PTM_CAP, reg); > + dw_pcie_dbi_ro_wr_dis(pci); From looking at the databook: PCI_PTM_CAP_ROOT: Note: This register field is sticky. PCI_PTM_CAP_RES: Note: This register field is sticky. PCI_PTM_GRANULARITY_MASK: Dbi: if (DBI_RO_WR_EN == 1) then R/W(sticky) else R(sticky) So all these register fields appear to be sticky to me. So I would assume that the PTM related writes could be done in dw_pcie_ep_init_registers(). > + } > + > + dw_pcie_setup(pci); > + dw_pcie_dbi_ro_wr_dis(pci); > +} > + > int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep) > { > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > struct dw_pcie_ep_func *ep_func; > struct device *dev = pci->dev; > struct pci_epc *epc = ep->epc; > - unsigned int offset, ptm_cap_base; > - unsigned int nbars; > u8 hdr_type; > u8 func_no; > - int i, ret; > void *addr; > - u32 reg; > + int ret; > > hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE) & > PCI_HEADER_TYPE_MASK; > @@ -678,38 +707,7 @@ int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep) > if (ep->ops->init) > ep->ops->init(ep); > > - 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); > - } > - > - /* > - * PTM responder capability can be disabled only after disabling > - * PTM root capability. > - */ > - if (ptm_cap_base) { > - dw_pcie_dbi_ro_wr_en(pci); > - reg = dw_pcie_readl_dbi(pci, ptm_cap_base + PCI_PTM_CAP); > - reg &= ~PCI_PTM_CAP_ROOT; > - dw_pcie_writel_dbi(pci, ptm_cap_base + PCI_PTM_CAP, reg); > - > - reg = dw_pcie_readl_dbi(pci, ptm_cap_base + PCI_PTM_CAP); > - reg &= ~(PCI_PTM_CAP_RES | PCI_PTM_GRANULARITY_MASK); > - dw_pcie_writel_dbi(pci, ptm_cap_base + PCI_PTM_CAP, reg); > - dw_pcie_dbi_ro_wr_dis(pci); > - } > - > - dw_pcie_setup(pci); > - dw_pcie_dbi_ro_wr_dis(pci); > + dw_pcie_ep_init_non_sticky_registers(pci); > > return 0; > > @@ -720,6 +718,31 @@ int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep) > } > EXPORT_SYMBOL_GPL(dw_pcie_ep_init_registers); > > +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_linkdown(struct dw_pcie_ep *ep) > +{ > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > + struct pci_epc *epc = ep->epc; > + > + /* > + * Initialize the non-sticky DWC registers as they would've reset post > + * LINK_DOWN. This is specifically needed for drivers not supporting > + * PERST# as they have no way to reinitialize the registers before the > + * link comes back again. > + */ > + dw_pcie_ep_init_non_sticky_registers(pci); > + > + pci_epc_linkdown(epc); > +} > +EXPORT_SYMBOL_GPL(dw_pcie_ep_linkdown); > + > int dw_pcie_ep_init(struct dw_pcie_ep *ep) > { > int ret; > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > index f8e5431a207b..152969545b0a 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.h > +++ b/drivers/pci/controller/dwc/pcie-designware.h > @@ -668,6 +668,7 @@ static inline void __iomem *dw_pcie_own_conf_map_bus(struct pci_bus *bus, > > #ifdef CONFIG_PCIE_DW_EP > void dw_pcie_ep_linkup(struct dw_pcie_ep *ep); > +void dw_pcie_ep_linkdown(struct dw_pcie_ep *ep); > int dw_pcie_ep_init(struct dw_pcie_ep *ep); > int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep); > void dw_pcie_ep_init_notify(struct dw_pcie_ep *ep); > @@ -688,6 +689,10 @@ static inline void dw_pcie_ep_linkup(struct dw_pcie_ep *ep) > { > } > > +static inline void dw_pcie_ep_linkdown(struct dw_pcie_ep *ep) > +{ > +} > + > static inline int dw_pcie_ep_init(struct dw_pcie_ep *ep) > { > return 0; > > -- > 2.25.1 >
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. > > > + } > > + > > + /* > > + * PTM responder capability can be disabled only after disabling > > + * PTM root capability. > > + */ > > + if (ptm_cap_base) { > > + dw_pcie_dbi_ro_wr_en(pci); > > + reg = dw_pcie_readl_dbi(pci, ptm_cap_base + PCI_PTM_CAP); > > + reg &= ~PCI_PTM_CAP_ROOT; > > + dw_pcie_writel_dbi(pci, ptm_cap_base + PCI_PTM_CAP, reg); > > + > > + reg = dw_pcie_readl_dbi(pci, ptm_cap_base + PCI_PTM_CAP); > > + reg &= ~(PCI_PTM_CAP_RES | PCI_PTM_GRANULARITY_MASK); > > + dw_pcie_writel_dbi(pci, ptm_cap_base + PCI_PTM_CAP, reg); > > + dw_pcie_dbi_ro_wr_dis(pci); > > From looking at the databook: > PCI_PTM_CAP_ROOT: > Note: This register field is sticky. > > PCI_PTM_CAP_RES: > Note: This register field is sticky. > > PCI_PTM_GRANULARITY_MASK: > Dbi: if (DBI_RO_WR_EN == 1) then R/W(sticky) else > R(sticky) > > So all these register fields appear to be sticky to me. > So I would assume that the PTM related writes could be > done in dw_pcie_ep_init_registers(). > Sorry, I didn't cross check the registers w/ databook as I was under the assumption that all PTM registers are non-sticky. Will move them. - Mani > > > + } > > + > > + dw_pcie_setup(pci); > > + dw_pcie_dbi_ro_wr_dis(pci); > > +} > > + > > int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep) > > { > > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > struct dw_pcie_ep_func *ep_func; > > struct device *dev = pci->dev; > > struct pci_epc *epc = ep->epc; > > - unsigned int offset, ptm_cap_base; > > - unsigned int nbars; > > u8 hdr_type; > > u8 func_no; > > - int i, ret; > > void *addr; > > - u32 reg; > > + int ret; > > > > hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE) & > > PCI_HEADER_TYPE_MASK; > > @@ -678,38 +707,7 @@ int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep) > > if (ep->ops->init) > > ep->ops->init(ep); > > > > - 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); > > - } > > - > > - /* > > - * PTM responder capability can be disabled only after disabling > > - * PTM root capability. > > - */ > > - if (ptm_cap_base) { > > - dw_pcie_dbi_ro_wr_en(pci); > > - reg = dw_pcie_readl_dbi(pci, ptm_cap_base + PCI_PTM_CAP); > > - reg &= ~PCI_PTM_CAP_ROOT; > > - dw_pcie_writel_dbi(pci, ptm_cap_base + PCI_PTM_CAP, reg); > > - > > - reg = dw_pcie_readl_dbi(pci, ptm_cap_base + PCI_PTM_CAP); > > - reg &= ~(PCI_PTM_CAP_RES | PCI_PTM_GRANULARITY_MASK); > > - dw_pcie_writel_dbi(pci, ptm_cap_base + PCI_PTM_CAP, reg); > > - dw_pcie_dbi_ro_wr_dis(pci); > > - } > > - > > - dw_pcie_setup(pci); > > - dw_pcie_dbi_ro_wr_dis(pci); > > + dw_pcie_ep_init_non_sticky_registers(pci); > > > > return 0; > > > > @@ -720,6 +718,31 @@ int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep) > > } > > EXPORT_SYMBOL_GPL(dw_pcie_ep_init_registers); > > > > +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_linkdown(struct dw_pcie_ep *ep) > > +{ > > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > + struct pci_epc *epc = ep->epc; > > + > > + /* > > + * Initialize the non-sticky DWC registers as they would've reset post > > + * LINK_DOWN. This is specifically needed for drivers not supporting > > + * PERST# as they have no way to reinitialize the registers before the > > + * link comes back again. > > + */ > > + dw_pcie_ep_init_non_sticky_registers(pci); > > + > > + pci_epc_linkdown(epc); > > +} > > +EXPORT_SYMBOL_GPL(dw_pcie_ep_linkdown); > > + > > int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > { > > int ret; > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > > index f8e5431a207b..152969545b0a 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware.h > > +++ b/drivers/pci/controller/dwc/pcie-designware.h > > @@ -668,6 +668,7 @@ static inline void __iomem *dw_pcie_own_conf_map_bus(struct pci_bus *bus, > > > > #ifdef CONFIG_PCIE_DW_EP > > void dw_pcie_ep_linkup(struct dw_pcie_ep *ep); > > +void dw_pcie_ep_linkdown(struct dw_pcie_ep *ep); > > int dw_pcie_ep_init(struct dw_pcie_ep *ep); > > int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep); > > void dw_pcie_ep_init_notify(struct dw_pcie_ep *ep); > > @@ -688,6 +689,10 @@ static inline void dw_pcie_ep_linkup(struct dw_pcie_ep *ep) > > { > > } > > > > +static inline void dw_pcie_ep_linkdown(struct dw_pcie_ep *ep) > > +{ > > +} > > + > > static inline int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > { > > return 0; > > > > -- > > 2.25.1 > >
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); Kind regards, Niklas
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. - Mani
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
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); + } + + /* + * PTM responder capability can be disabled only after disabling + * PTM root capability. + */ + if (ptm_cap_base) { + dw_pcie_dbi_ro_wr_en(pci); + reg = dw_pcie_readl_dbi(pci, ptm_cap_base + PCI_PTM_CAP); + reg &= ~PCI_PTM_CAP_ROOT; + dw_pcie_writel_dbi(pci, ptm_cap_base + PCI_PTM_CAP, reg); + + reg = dw_pcie_readl_dbi(pci, ptm_cap_base + PCI_PTM_CAP); + reg &= ~(PCI_PTM_CAP_RES | PCI_PTM_GRANULARITY_MASK); + dw_pcie_writel_dbi(pci, ptm_cap_base + PCI_PTM_CAP, reg); + dw_pcie_dbi_ro_wr_dis(pci); + } + + dw_pcie_setup(pci); + dw_pcie_dbi_ro_wr_dis(pci); +} + int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep) { struct dw_pcie *pci = to_dw_pcie_from_ep(ep); struct dw_pcie_ep_func *ep_func; struct device *dev = pci->dev; struct pci_epc *epc = ep->epc; - unsigned int offset, ptm_cap_base; - unsigned int nbars; u8 hdr_type; u8 func_no; - int i, ret; void *addr; - u32 reg; + int ret; hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE) & PCI_HEADER_TYPE_MASK; @@ -678,38 +707,7 @@ int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep) if (ep->ops->init) ep->ops->init(ep); - 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); - } - - /* - * PTM responder capability can be disabled only after disabling - * PTM root capability. - */ - if (ptm_cap_base) { - dw_pcie_dbi_ro_wr_en(pci); - reg = dw_pcie_readl_dbi(pci, ptm_cap_base + PCI_PTM_CAP); - reg &= ~PCI_PTM_CAP_ROOT; - dw_pcie_writel_dbi(pci, ptm_cap_base + PCI_PTM_CAP, reg); - - reg = dw_pcie_readl_dbi(pci, ptm_cap_base + PCI_PTM_CAP); - reg &= ~(PCI_PTM_CAP_RES | PCI_PTM_GRANULARITY_MASK); - dw_pcie_writel_dbi(pci, ptm_cap_base + PCI_PTM_CAP, reg); - dw_pcie_dbi_ro_wr_dis(pci); - } - - dw_pcie_setup(pci); - dw_pcie_dbi_ro_wr_dis(pci); + dw_pcie_ep_init_non_sticky_registers(pci); return 0; @@ -720,6 +718,31 @@ int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep) } EXPORT_SYMBOL_GPL(dw_pcie_ep_init_registers); +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_linkdown(struct dw_pcie_ep *ep) +{ + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); + struct pci_epc *epc = ep->epc; + + /* + * Initialize the non-sticky DWC registers as they would've reset post + * LINK_DOWN. This is specifically needed for drivers not supporting + * PERST# as they have no way to reinitialize the registers before the + * link comes back again. + */ + dw_pcie_ep_init_non_sticky_registers(pci); + + pci_epc_linkdown(epc); +} +EXPORT_SYMBOL_GPL(dw_pcie_ep_linkdown); + int dw_pcie_ep_init(struct dw_pcie_ep *ep) { int ret; diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h index f8e5431a207b..152969545b0a 100644 --- a/drivers/pci/controller/dwc/pcie-designware.h +++ b/drivers/pci/controller/dwc/pcie-designware.h @@ -668,6 +668,7 @@ static inline void __iomem *dw_pcie_own_conf_map_bus(struct pci_bus *bus, #ifdef CONFIG_PCIE_DW_EP void dw_pcie_ep_linkup(struct dw_pcie_ep *ep); +void dw_pcie_ep_linkdown(struct dw_pcie_ep *ep); int dw_pcie_ep_init(struct dw_pcie_ep *ep); int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep); void dw_pcie_ep_init_notify(struct dw_pcie_ep *ep); @@ -688,6 +689,10 @@ static inline void dw_pcie_ep_linkup(struct dw_pcie_ep *ep) { } +static inline void dw_pcie_ep_linkdown(struct dw_pcie_ep *ep) +{ +} + static inline int dw_pcie_ep_init(struct dw_pcie_ep *ep) { return 0;
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(-)