RE: [PATCH v3 04/11] PCI: designware-ep: Modify MSI and MSIX CAP way of finding

2019-09-02 Thread Xiaowei Bao


> -Original Message-
> From: Andrew Murray 
> Sent: 2019年9月2日 23:07
> To: Xiaowei Bao 
> Cc: robh...@kernel.org; mark.rutl...@arm.com; shawn...@kernel.org; Leo
> Li ; kis...@ti.com; lorenzo.pieral...@arm.com; M.h.
> Lian ; Mingkai Hu ; Roy
> Zang ; jingooh...@gmail.com;
> gustavo.pimen...@synopsys.com; linux-...@vger.kernel.org;
> devicet...@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-arm-ker...@lists.infradead.org; linuxppc-...@lists.ozlabs.org;
> a...@arndb.de; gre...@linuxfoundation.org; Z.q. Hou
> 
> Subject: Re: [PATCH v3 04/11] PCI: designware-ep: Modify MSI and MSIX CAP
> way of finding
> 
> On Mon, Sep 02, 2019 at 11:17:09AM +0800, Xiaowei Bao wrote:
> > Each PF of EP device should have it's own MSI or MSIX capabitily
> > struct, so create a dw_pcie_ep_func struct and remover the msi_cap
> 
> remover?

Sorry. ^_^


> 
> > and msix_cap to this struce, and manage the PFs with a list.
> 
> struce?
> 
> >
> > Signed-off-by: Xiaowei Bao 
> > ---
> > v1:
> >  - This is a new patch, to fix the issue of MSI and MSIX CAP way of
> >finding.
> > v2:
> >  - No change.
> > v3:
> >  - No change.
> 
> This makes it look like you introduced the patch in v1 and haven't changed it
> since.
> 
> I think it's more common to have a history like this:
> 
> ---
> v3:
>  - Introduced new patch, to fix the issue of MSI and MSIX CAP way of
>finding.

OK, thanks, I am not clear the rules, thanks a lot for your help.

Thanks 
Xiaowei

> 
> 
> >
> >  drivers/pci/controller/dwc/pcie-designware-ep.c | 135
> +---
> >  drivers/pci/controller/dwc/pcie-designware.h|  18 +++-
> >  2 files changed, 134 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index c3bc7bd..144eb12 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -19,6 +19,19 @@ void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
> > pci_epc_linkup(epc);
> >  }
> >
> > +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, &ep->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;
> > @@ -59,6 +72,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); }
> 
> Which tree have you based this patchset on? v5.3-rc3 and pci/dwc both
> already have this function (without the func_no). See beb4641a787d
> ("PCI: dwc: Add MSI-X callbacks handler").

There is a commit 7a6854f68 in the latest kernel.

Thanks 
Xiaowei

> 
> > +
> > +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); }
> > +
> >  static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no,
> > 

Re: [PATCH v3 04/11] PCI: designware-ep: Modify MSI and MSIX CAP way of finding

2019-09-02 Thread Andrew Murray
On Mon, Sep 02, 2019 at 11:17:09AM +0800, Xiaowei Bao wrote:
> Each PF of EP device should have it's own MSI or MSIX capabitily
> struct, so create a dw_pcie_ep_func struct and remover the msi_cap

remover?

> and msix_cap to this struce, and manage the PFs with a list.

struce?

> 
> Signed-off-by: Xiaowei Bao 
> ---
> v1:
>  - This is a new patch, to fix the issue of MSI and MSIX CAP way of
>finding.
> v2:
>  - No change.
> v3:
>  - No change.

This makes it look like you introduced the patch in v1 and haven't
changed it since.

I think it's more common to have a history like this:

---
v3:
 - Introduced new patch, to fix the issue of MSI and MSIX CAP way of
   finding.


> 
>  drivers/pci/controller/dwc/pcie-designware-ep.c | 135 
> +---
>  drivers/pci/controller/dwc/pcie-designware.h|  18 +++-
>  2 files changed, 134 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c 
> b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index c3bc7bd..144eb12 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -19,6 +19,19 @@ void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
>   pci_epc_linkup(epc);
>  }
>  
> +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, &ep->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;
> @@ -59,6 +72,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);
> +}

Which tree have you based this patchset on? v5.3-rc3 and pci/dwc both already
have this function (without the func_no). See beb4641a787d
("PCI: dwc: Add MSI-X callbacks handler").

> +
> +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);
> +}
> +
>  static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no,
>  struct pci_epf_header *hdr)
>  {
> @@ -246,13 +300,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;
> @@ -268,13 +327,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;
>   val = dw_pcie_readw_dbi(pci, reg);
>   val &= ~PCI_MSI_FLAGS_QMASK;
>   val |= (interrupts << 1) & PCI_MSI_FLAGS_QMASK;
> @@ -291,13 +35