RE: [PATCHv7 04/12] PCI: designware-ep: Modify MSI and MSIX CAP way of finding
Hi Rob, > -Original Message- > From: Rob Herring > Sent: 2020年9月11日 2:10 > To: Z.q. Hou > Cc: linux-...@vger.kernel.org; devicet...@vger.kernel.org; > linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; > linuxppc-...@lists.ozlabs.org; bhelg...@google.com; > lorenzo.pieral...@arm.com; shawn...@kernel.org; Leo Li > ; kis...@ti.com; gustavo.pimen...@synopsys.com; > Roy Zang ; jingooh...@gmail.com; > andrew.mur...@arm.com; Mingkai Hu ; M.h. Lian > ; Xiaowei Bao > Subject: Re: [PATCHv7 04/12] PCI: designware-ep: Modify MSI and MSIX CAP > way of finding > > On Tue, Aug 11, 2020 at 05:54:33PM +0800, Zhiqiang Hou wrote: > > From: Xiaowei Bao > > > > Each PF of EP device should have its own MSI or MSIX capabitily > > struct, so create a dw_pcie_ep_func struct and move the msi_cap and > > msix_cap to this struct from dw_pcie_ep, and manage the PFs via a > > list. > > > > Signed-off-by: Xiaowei Bao > > Signed-off-by: Hou Zhiqiang > > --- > > V7: > > - Rebase the patch without functionality change. > > > > .../pci/controller/dwc/pcie-designware-ep.c | 139 > +++--- > > drivers/pci/controller/dwc/pcie-designware.h | 18 ++- > > 2 files changed, 136 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c > > b/drivers/pci/controller/dwc/pcie-designware-ep.c > > index 56bd1cd71f16..4680a51c49c0 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > > @@ -28,6 +28,19 @@ void dw_pcie_ep_init_notify(struct dw_pcie_ep > *ep) > > } EXPORT_SYMBOL_GPL(dw_pcie_ep_init_notify); > > > > +struct dw_pcie_ep_func * > > +dw_pcie_ep_get_func_from_ep(struct dw_pcie_ep *ep, u8 func_no) { > > + struct dw_pcie_ep_func *ep_func; > > + > > + list_for_each_entry(ep_func, >func_list, list) { > > + if (ep_func->func_no == func_no) > > + return ep_func; > > + } > > + > > + return NULL; > > +} > > + > > static unsigned int dw_pcie_ep_func_select(struct dw_pcie_ep *ep, u8 > > func_no) { > > unsigned int func_offset = 0; > > @@ -68,6 +81,47 @@ void dw_pcie_ep_reset_bar(struct dw_pcie *pci, > enum pci_barno bar) > > __dw_pcie_ep_reset_bar(pci, func_no, bar, 0); } > > > > +static u8 __dw_pcie_ep_find_next_cap(struct dw_pcie_ep *ep, u8 > func_no, > > + u8 cap_ptr, u8 cap) > > +{ > > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > + unsigned int func_offset = 0; > > + u8 cap_id, next_cap_ptr; > > + u16 reg; > > + > > + if (!cap_ptr) > > + return 0; > > + > > + func_offset = dw_pcie_ep_func_select(ep, func_no); > > + > > + reg = dw_pcie_readw_dbi(pci, func_offset + cap_ptr); > > + cap_id = (reg & 0x00ff); > > + > > + if (cap_id > PCI_CAP_ID_MAX) > > + return 0; > > + > > + if (cap_id == cap) > > + return cap_ptr; > > + > > + next_cap_ptr = (reg & 0xff00) >> 8; > > + return __dw_pcie_ep_find_next_cap(ep, func_no, next_cap_ptr, cap); } > > + > > +static u8 dw_pcie_ep_find_capability(struct dw_pcie_ep *ep, u8 > > +func_no, u8 cap) { > > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > + unsigned int func_offset = 0; > > + u8 next_cap_ptr; > > + u16 reg; > > + > > + func_offset = dw_pcie_ep_func_select(ep, func_no); > > + > > + reg = dw_pcie_readw_dbi(pci, func_offset + PCI_CAPABILITY_LIST); > > + next_cap_ptr = (reg & 0x00ff); > > + > > + return __dw_pcie_ep_find_next_cap(ep, func_no, next_cap_ptr, cap); } > > These are almost the same as __dw_pcie_find_next_cap and > dw_pcie_find_capability. Please modify them to take a function offset and > work for both host and endpoints. > I sent out v8 patches but without the rework of the functions of finding capability, I will submit a separate patch to do this. > > + > > static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no, > >struct pci_epf_header *hdr) > > { > > @@ -257,13 +311,18 @@ static int dw_pcie_ep_get_msi(struct pci_epc > *epc, u8 func_no) > > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > u32 val, reg; > > unsigned int func_offset = 0; > > + struct dw_pcie_ep_func *ep_func; > > > > - if (!ep->msi_cap) > > + ep_func = dw_pcie_ep_get_func_from_ep(ep, func_no); > > +
RE: [PATCHv7 04/12] PCI: designware-ep: Modify MSI and MSIX CAP way of finding
Hi Rob, Thanks a lot for your comments! > -Original Message- > From: Rob Herring > Sent: 2020年9月11日 2:10 > To: Z.q. Hou > Cc: linux-...@vger.kernel.org; devicet...@vger.kernel.org; > linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; > linuxppc-...@lists.ozlabs.org; bhelg...@google.com; > lorenzo.pieral...@arm.com; shawn...@kernel.org; Leo Li > ; kis...@ti.com; gustavo.pimen...@synopsys.com; > Roy Zang ; jingooh...@gmail.com; > andrew.mur...@arm.com; Mingkai Hu ; M.h. Lian > ; Xiaowei Bao > Subject: Re: [PATCHv7 04/12] PCI: designware-ep: Modify MSI and MSIX CAP > way of finding > > On Tue, Aug 11, 2020 at 05:54:33PM +0800, Zhiqiang Hou wrote: > > From: Xiaowei Bao > > > > Each PF of EP device should have its own MSI or MSIX capabitily > > struct, so create a dw_pcie_ep_func struct and move the msi_cap and > > msix_cap to this struct from dw_pcie_ep, and manage the PFs via a > > list. > > > > Signed-off-by: Xiaowei Bao > > Signed-off-by: Hou Zhiqiang > > --- > > V7: > > - Rebase the patch without functionality change. > > > > .../pci/controller/dwc/pcie-designware-ep.c | 139 > +++--- > > drivers/pci/controller/dwc/pcie-designware.h | 18 ++- > > 2 files changed, 136 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c > > b/drivers/pci/controller/dwc/pcie-designware-ep.c > > index 56bd1cd71f16..4680a51c49c0 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > > @@ -28,6 +28,19 @@ void dw_pcie_ep_init_notify(struct dw_pcie_ep > *ep) > > } EXPORT_SYMBOL_GPL(dw_pcie_ep_init_notify); > > > > +struct dw_pcie_ep_func * > > +dw_pcie_ep_get_func_from_ep(struct dw_pcie_ep *ep, u8 func_no) { > > + struct dw_pcie_ep_func *ep_func; > > + > > + list_for_each_entry(ep_func, >func_list, list) { > > + if (ep_func->func_no == func_no) > > + return ep_func; > > + } > > + > > + return NULL; > > +} > > + > > static unsigned int dw_pcie_ep_func_select(struct dw_pcie_ep *ep, u8 > > func_no) { > > unsigned int func_offset = 0; > > @@ -68,6 +81,47 @@ void dw_pcie_ep_reset_bar(struct dw_pcie *pci, > enum pci_barno bar) > > __dw_pcie_ep_reset_bar(pci, func_no, bar, 0); } > > > > +static u8 __dw_pcie_ep_find_next_cap(struct dw_pcie_ep *ep, u8 > func_no, > > + u8 cap_ptr, u8 cap) > > +{ > > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > + unsigned int func_offset = 0; > > + u8 cap_id, next_cap_ptr; > > + u16 reg; > > + > > + if (!cap_ptr) > > + return 0; > > + > > + func_offset = dw_pcie_ep_func_select(ep, func_no); > > + > > + reg = dw_pcie_readw_dbi(pci, func_offset + cap_ptr); > > + cap_id = (reg & 0x00ff); > > + > > + if (cap_id > PCI_CAP_ID_MAX) > > + return 0; > > + > > + if (cap_id == cap) > > + return cap_ptr; > > + > > + next_cap_ptr = (reg & 0xff00) >> 8; > > + return __dw_pcie_ep_find_next_cap(ep, func_no, next_cap_ptr, cap); } > > + > > +static u8 dw_pcie_ep_find_capability(struct dw_pcie_ep *ep, u8 > > +func_no, u8 cap) { > > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > + unsigned int func_offset = 0; > > + u8 next_cap_ptr; > > + u16 reg; > > + > > + func_offset = dw_pcie_ep_func_select(ep, func_no); > > + > > + reg = dw_pcie_readw_dbi(pci, func_offset + PCI_CAPABILITY_LIST); > > + next_cap_ptr = (reg & 0x00ff); > > + > > + return __dw_pcie_ep_find_next_cap(ep, func_no, next_cap_ptr, cap); } > > These are almost the same as __dw_pcie_find_next_cap and > dw_pcie_find_capability. Please modify them to take a function offset and > work for both host and endpoints. Yes, will rework in next version. > > > + > > static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no, > >struct pci_epf_header *hdr) > > { > > @@ -257,13 +311,18 @@ static int dw_pcie_ep_get_msi(struct pci_epc > *epc, u8 func_no) > > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > u32 val, reg; > > unsigned int func_offset = 0; > > + struct dw_pcie_ep_func *ep_func; > > > > - if (!ep->msi_cap) > > + ep_func = dw_pcie_ep_get_func_from_ep(ep, func_no); > > + if (!ep_func) > > + return -EINVAL; > >
Re: [PATCHv7 04/12] PCI: designware-ep: Modify MSI and MSIX CAP way of finding
On Tue, Aug 11, 2020 at 05:54:33PM +0800, Zhiqiang Hou wrote: > From: Xiaowei Bao > > Each PF of EP device should have its own MSI or MSIX capabitily > struct, so create a dw_pcie_ep_func struct and move the msi_cap > and msix_cap to this struct from dw_pcie_ep, and manage the PFs > via a list. > > Signed-off-by: Xiaowei Bao > Signed-off-by: Hou Zhiqiang > --- > V7: > - Rebase the patch without functionality change. > > .../pci/controller/dwc/pcie-designware-ep.c | 139 +++--- > drivers/pci/controller/dwc/pcie-designware.h | 18 ++- > 2 files changed, 136 insertions(+), 21 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c > b/drivers/pci/controller/dwc/pcie-designware-ep.c > index 56bd1cd71f16..4680a51c49c0 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > @@ -28,6 +28,19 @@ void dw_pcie_ep_init_notify(struct dw_pcie_ep *ep) > } > EXPORT_SYMBOL_GPL(dw_pcie_ep_init_notify); > > +struct dw_pcie_ep_func * > +dw_pcie_ep_get_func_from_ep(struct dw_pcie_ep *ep, u8 func_no) > +{ > + struct dw_pcie_ep_func *ep_func; > + > + list_for_each_entry(ep_func, >func_list, list) { > + if (ep_func->func_no == func_no) > + return ep_func; > + } > + > + return NULL; > +} > + > static unsigned int dw_pcie_ep_func_select(struct dw_pcie_ep *ep, u8 func_no) > { > unsigned int func_offset = 0; > @@ -68,6 +81,47 @@ void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum > pci_barno bar) > __dw_pcie_ep_reset_bar(pci, func_no, bar, 0); > } > > +static u8 __dw_pcie_ep_find_next_cap(struct dw_pcie_ep *ep, u8 func_no, > + u8 cap_ptr, u8 cap) > +{ > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > + unsigned int func_offset = 0; > + u8 cap_id, next_cap_ptr; > + u16 reg; > + > + if (!cap_ptr) > + return 0; > + > + func_offset = dw_pcie_ep_func_select(ep, func_no); > + > + reg = dw_pcie_readw_dbi(pci, func_offset + cap_ptr); > + cap_id = (reg & 0x00ff); > + > + if (cap_id > PCI_CAP_ID_MAX) > + return 0; > + > + if (cap_id == cap) > + return cap_ptr; > + > + next_cap_ptr = (reg & 0xff00) >> 8; > + return __dw_pcie_ep_find_next_cap(ep, func_no, next_cap_ptr, cap); > +} > + > +static u8 dw_pcie_ep_find_capability(struct dw_pcie_ep *ep, u8 func_no, u8 > cap) > +{ > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > + unsigned int func_offset = 0; > + u8 next_cap_ptr; > + u16 reg; > + > + func_offset = dw_pcie_ep_func_select(ep, func_no); > + > + reg = dw_pcie_readw_dbi(pci, func_offset + PCI_CAPABILITY_LIST); > + next_cap_ptr = (reg & 0x00ff); > + > + return __dw_pcie_ep_find_next_cap(ep, func_no, next_cap_ptr, cap); > +} These are almost the same as __dw_pcie_find_next_cap and dw_pcie_find_capability. Please modify them to take a function offset and work for both host and endpoints. > + > static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no, > struct pci_epf_header *hdr) > { > @@ -257,13 +311,18 @@ static int dw_pcie_ep_get_msi(struct pci_epc *epc, u8 > func_no) > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > u32 val, reg; > unsigned int func_offset = 0; > + struct dw_pcie_ep_func *ep_func; > > - if (!ep->msi_cap) > + ep_func = dw_pcie_ep_get_func_from_ep(ep, func_no); > + if (!ep_func) > + return -EINVAL; > + > + if (!ep_func->msi_cap) > return -EINVAL; > > func_offset = dw_pcie_ep_func_select(ep, func_no); > > - reg = ep->msi_cap + func_offset + PCI_MSI_FLAGS; > + reg = ep_func->msi_cap + func_offset + PCI_MSI_FLAGS; > val = dw_pcie_readw_dbi(pci, reg); > if (!(val & PCI_MSI_FLAGS_ENABLE)) > return -EINVAL; > @@ -279,13 +338,18 @@ static int dw_pcie_ep_set_msi(struct pci_epc *epc, u8 > func_no, u8 interrupts) > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > u32 val, reg; > unsigned int func_offset = 0; > + struct dw_pcie_ep_func *ep_func; > + > + ep_func = dw_pcie_ep_get_func_from_ep(ep, func_no); > + if (!ep_func) > + return -EINVAL; > > - if (!ep->msi_cap) > + if (!ep_func->msi_cap) > return -EINVAL; > > func_offset = dw_pcie_ep_func_select(ep, func_no); > > - reg = ep->msi_cap + func_offset + PCI_MSI_FLAGS; > + reg = ep_func->msi_cap + func_offset + PCI_MSI_FLAGS; If msi_cap is now per function, then shouldn't it already include 'func_offset'? > val = dw_pcie_readw_dbi(pci, reg); > val &= ~PCI_MSI_FLAGS_QMASK; > val |= (interrupts << 1) & PCI_MSI_FLAGS_QMASK; > @@ -302,13 +366,18 @@ static int dw_pcie_ep_get_msix(struct pci_epc *epc, u8 > func_no) > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > u32 val, reg; > unsigned int