| Message ID | 20240314-pci-epf-rework-v1-7-6134e6c1d491@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: endpoint: Make host reboot handling more robust | expand |
On Thu, Mar 14, 2024 at 08:53:46PM +0530, Manivannan Sadhasivam wrote: > As per the PCIe base spec r5.0, section 5.2, Link Down event can happen > under any of the following circumstances: > > 1. Fundamental/Hot reset > 2. Link disable transmission by upstream component > 3. Moving from L2/L3 to L0 > > In those cases, Link Down causes some non-sticky DWC registers to loose the > state (like REBAR, 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. > > NOTE: For the sake of code organization, move the dw_pcie_ep_linkup() > definition just above dw_pcie_ep_linkdown(). > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- Reviewed-by: Niklas Cassel <cassel@kernel.org>
Hello Mani, On Thu, Mar 14, 2024 at 08:53:46PM +0530, Manivannan Sadhasivam wrote: > As per the PCIe base spec r5.0, section 5.2, Link Down event can happen > under any of the following circumstances: > > 1. Fundamental/Hot reset > 2. Link disable transmission by upstream component > 3. Moving from L2/L3 to L0 > > In those cases, Link Down causes some non-sticky DWC registers to loose the > state (like REBAR, 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. > > NOTE: For the sake of code organization, move the dw_pcie_ep_linkup() > definition just above dw_pcie_ep_linkdown(). > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/pci/controller/dwc/pcie-designware-ep.c | 93 ++++++++++++++++--------- > drivers/pci/controller/dwc/pcie-designware.h | 5 ++ > 2 files changed, 67 insertions(+), 31 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > index 3893a8c1a11c..5451057ca74b 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > @@ -14,18 +14,6 @@ > #include <linux/pci-epc.h> > #include <linux/pci-epf.h> > > -/** > - * dw_pcie_ep_linkup - Notify EPF drivers about link up event > - * @ep: DWC EP device > - */ > -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); > - > /** > * dw_pcie_ep_init_notify - Notify EPF drivers about EPC initialization > * complete > @@ -672,6 +660,29 @@ 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; > + unsigned int nbars; > + u32 reg, i; > + > + offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR); > + > + 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); > + } > + > + dw_pcie_setup(pci); > + dw_pcie_dbi_ro_wr_dis(pci); > +} > + > /** > * dw_pcie_ep_init_registers - Initialize DWC EP specific registers > * @ep: DWC EP device > @@ -686,13 +697,11 @@ int dw_pcie_ep_init_registers(struct dw_pcie_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; > + u32 ptm_cap_base, reg; > 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; > @@ -755,20 +764,8 @@ 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. > @@ -785,9 +782,6 @@ int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep) > dw_pcie_dbi_ro_wr_dis(pci); > } > > - dw_pcie_setup(pci); > - dw_pcie_dbi_ro_wr_dis(pci); > - Your previous series had: - dw_pcie_setup(pci); - dw_pcie_dbi_ro_wr_dis(pci); + dw_pcie_ep_init_non_sticky_registers(pci); Here. I tested this series, but it did not work for me (the Resizable BARs did not get resized) since you removed the call to dw_pcie_ep_init_non_sticky_registers(). By readding the call to dw_pcie_ep_init_non_sticky_registers(), the BARs get Resized again. BTW do you have a git branch with both your series somewhere? (Possibly even rebased on https://lore.kernel.org/linux-pci/20240320113157.322695-1-cassel@kernel.org/T/#t like you suggested in your other mail.) Kind regards, Niklas
On Wed, Mar 27, 2024 at 07:06:30PM +0100, Niklas Cassel wrote: > Hello Mani, > > On Thu, Mar 14, 2024 at 08:53:46PM +0530, Manivannan Sadhasivam wrote: > > As per the PCIe base spec r5.0, section 5.2, Link Down event can happen > > under any of the following circumstances: > > > > 1. Fundamental/Hot reset > > 2. Link disable transmission by upstream component > > 3. Moving from L2/L3 to L0 > > > > In those cases, Link Down causes some non-sticky DWC registers to loose the > > state (like REBAR, 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. > > > > NOTE: For the sake of code organization, move the dw_pcie_ep_linkup() > > definition just above dw_pcie_ep_linkdown(). > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > drivers/pci/controller/dwc/pcie-designware-ep.c | 93 ++++++++++++++++--------- > > drivers/pci/controller/dwc/pcie-designware.h | 5 ++ > > 2 files changed, 67 insertions(+), 31 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > > index 3893a8c1a11c..5451057ca74b 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > > @@ -14,18 +14,6 @@ > > #include <linux/pci-epc.h> > > #include <linux/pci-epf.h> > > > > -/** > > - * dw_pcie_ep_linkup - Notify EPF drivers about link up event > > - * @ep: DWC EP device > > - */ > > -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); > > - > > /** > > * dw_pcie_ep_init_notify - Notify EPF drivers about EPC initialization > > * complete > > @@ -672,6 +660,29 @@ 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; > > + unsigned int nbars; > > + u32 reg, i; > > + > > + offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR); > > + > > + 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); > > + } > > + > > + dw_pcie_setup(pci); > > + dw_pcie_dbi_ro_wr_dis(pci); > > +} > > + > > /** > > * dw_pcie_ep_init_registers - Initialize DWC EP specific registers > > * @ep: DWC EP device > > @@ -686,13 +697,11 @@ int dw_pcie_ep_init_registers(struct dw_pcie_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; > > + u32 ptm_cap_base, reg; > > 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; > > @@ -755,20 +764,8 @@ 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. > > @@ -785,9 +782,6 @@ int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep) > > dw_pcie_dbi_ro_wr_dis(pci); > > } > > > > - dw_pcie_setup(pci); > > - dw_pcie_dbi_ro_wr_dis(pci); > > - > > Your previous series had: > > - dw_pcie_setup(pci); > - dw_pcie_dbi_ro_wr_dis(pci); > + dw_pcie_ep_init_non_sticky_registers(pci); > > Here. > I tested this series, but it did not work for me (the Resizable BARs did > not get resized) since you removed the call to > dw_pcie_ep_init_non_sticky_registers(). > > By readding the call to dw_pcie_ep_init_non_sticky_registers(), > the BARs get Resized again. > > Ah, looks like rebase has gone bad. Will fix it in v2. > BTW do you have a git branch with both your series somewhere? > (Possibly even rebased on > https://lore.kernel.org/linux-pci/20240320113157.322695-1-cassel@kernel.org/T/#t > like you suggested in your other mail.) > There it is: https://git.codelinaro.org/manivannan.sadhasivam/linux/-/tree/b4/pci-epf-rework - Mani
diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c index 3893a8c1a11c..5451057ca74b 100644 --- a/drivers/pci/controller/dwc/pcie-designware-ep.c +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c @@ -14,18 +14,6 @@ #include <linux/pci-epc.h> #include <linux/pci-epf.h> -/** - * dw_pcie_ep_linkup - Notify EPF drivers about link up event - * @ep: DWC EP device - */ -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); - /** * dw_pcie_ep_init_notify - Notify EPF drivers about EPC initialization * complete @@ -672,6 +660,29 @@ 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; + unsigned int nbars; + u32 reg, i; + + offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR); + + 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); + } + + dw_pcie_setup(pci); + dw_pcie_dbi_ro_wr_dis(pci); +} + /** * dw_pcie_ep_init_registers - Initialize DWC EP specific registers * @ep: DWC EP device @@ -686,13 +697,11 @@ int dw_pcie_ep_init_registers(struct dw_pcie_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; + u32 ptm_cap_base, reg; 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; @@ -755,20 +764,8 @@ 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. @@ -785,9 +782,6 @@ int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep) dw_pcie_dbi_ro_wr_dis(pci); } - dw_pcie_setup(pci); - dw_pcie_dbi_ro_wr_dis(pci); - return 0; err_remove_edma: @@ -797,6 +791,43 @@ int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep) } EXPORT_SYMBOL_GPL(dw_pcie_ep_init_registers); +/** + * dw_pcie_ep_linkup - Notify EPF drivers about Link Up event + * @ep: DWC EP device + */ +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); + +/** + * dw_pcie_ep_linkdown - Notify EPF drivers about Link Down event + * @ep: DWC EP device + * + * Non-sticky registers are also initialized before sending the notification to + * the EPF drivers. This is needed since the registers need to be initialized + * before the link comes back again. + */ +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); + /** * dw_pcie_ep_init - Initialize the endpoint device * @ep: DWC EP device 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;
As per the PCIe base spec r5.0, section 5.2, Link Down event can happen under any of the following circumstances: 1. Fundamental/Hot reset 2. Link disable transmission by upstream component 3. Moving from L2/L3 to L0 In those cases, Link Down causes some non-sticky DWC registers to loose the state (like REBAR, 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. NOTE: For the sake of code organization, move the dw_pcie_ep_linkup() definition just above dw_pcie_ep_linkdown(). Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- drivers/pci/controller/dwc/pcie-designware-ep.c | 93 ++++++++++++++++--------- drivers/pci/controller/dwc/pcie-designware.h | 5 ++ 2 files changed, 67 insertions(+), 31 deletions(-)