subject:"RE\: \[PATCH v2 01\/10\] PCI\: designware\-ep\: Add multiple PFs support for DWC"

RE: [PATCH v2 01/10] PCI: designware-ep: Add multiple PFs support for DWC

2019-08-28 Thread Xiaowei Bao



> -Original Message-
> From: Andrew Murray 
> Sent: 2019年8月27日 21:11
> To: Xiaowei Bao 
> Cc: bhelg...@google.com; robh...@kernel.org; mark.rutl...@arm.com;
> shawn...@kernel.org; Leo Li ; kis...@ti.com;
> lorenzo.pieral...@arm.co; a...@arndb.de; gre...@linuxfoundation.org; M.h.
> Lian ; Mingkai Hu ; Roy
> Zang ; jingooh...@gmail.com;
> gustavo.pimen...@synopsys.com; linux-...@vger.kernel.org;
> devicet...@vger.kernel.org; linux-ker...@vger.kernel.org;
> linux-arm-ker...@lists.infradead.org; linuxppc-dev@lists.ozlabs.org
> Subject: Re: [PATCH v2 01/10] PCI: designware-ep: Add multiple PFs support
> for DWC
> 
> On Fri, Aug 23, 2019 at 11:50:20PM +, Xiaowei Bao wrote:
> >
> >
> > > -Original Message-
> > > From: Andrew Murray 
> > > Sent: 2019年8月23日 21:25
> > > To: Xiaowei Bao 
> > > Cc: bhelg...@google.com; robh...@kernel.org; mark.rutl...@arm.com;
> > > shawn...@kernel.org; Leo Li ; kis...@ti.com;
> > > lorenzo.pieral...@arm.co; a...@arndb.de; gre...@linuxfoundation.org;
> M.h.
> > > Lian ; Mingkai Hu ; Roy
> > > Zang ; jingooh...@gmail.com;
> > > gustavo.pimen...@synopsys.com; linux-...@vger.kernel.org;
> > > devicet...@vger.kernel.org; linux-ker...@vger.kernel.org;
> > > linux-arm-ker...@lists.infradead.org; linuxppc-dev@lists.ozlabs.org
> > > Subject: Re: [PATCH v2 01/10] PCI: designware-ep: Add multiple PFs
> > > support for DWC
> > >
> > > On Thu, Aug 22, 2019 at 07:22:33PM +0800, Xiaowei Bao wrote:
> > > > Add multiple PFs support for DWC, different PF have different
> > > > config space we use pf-offset property which get from the DTS to
> > > > access the different pF config space.
> > >
> > > It looks like you're missing a --cover-letter again.
> > >
> > > >
> > > > Signed-off-by: Xiaowei Bao 
> > > > ---
> > > > v2:
> > > >  - Remove duplicate redundant code.
> > > >  - Reimplement the PF config space access way.
> > > >
> > > >  drivers/pci/controller/dwc/pcie-designware-ep.c | 122
> > > 
> > > >  drivers/pci/controller/dwc/pcie-designware.c|  59
> 
> > > >  drivers/pci/controller/dwc/pcie-designware.h|  11 ++-
> > > >  3 files changed, 134 insertions(+), 58 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > index 2bf5a35..3e2b740 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > @@ -19,12 +19,17 @@ void dw_pcie_ep_linkup(struct dw_pcie_ep
> *ep)
> > > > pci_epc_linkup(epc);
> > > >  }
> > > >
> > > > -static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum
> > > > pci_barno
> > > bar,
> > > > -  int flags)
> > > > +static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, u8 func_no,
> > > > +  enum pci_barno bar, int flags)
> > > >  {
> > > > u32 reg;
> > > > +   unsigned int func_offset = 0;
> > > > +   struct dw_pcie_ep *ep = >ep;
> > > >
> > > > -   reg = PCI_BASE_ADDRESS_0 + (4 * bar);
> > > > +   if (ep->ops->func_conf_select)
> > > > +   func_offset = ep->ops->func_conf_select(ep, func_no);
> > > > +
> > > > +   reg = func_offset + PCI_BASE_ADDRESS_0 + (4 * bar);
> > >
> > > This pattern of checking if func_conf_select exists and using it to
> > > get an offset is repeated a lot throughout this file. You could move
> > > this functionality into a new function (similar to dw_pcie_read_dbi
> > > etc). Or perhaps a new variant of dw_pcie_writel_ should be created that
> writes takes a func_no argument.
> >
> > Thanks for your comments, I thought about this method before, but
> > there is a issue about the method of access the different func config
> > space, due to our platform use this method that different func have
> > different offset from dbi_base to access the different config space,
> > but others platform maybe use the way that write a register to
> > implement different func config space access, so I think reserve a
> > callback function
> 
> My point here was really to move out duplic

Re: [PATCH v2 01/10] PCI: designware-ep: Add multiple PFs support for DWC

2019-08-27 Thread Andrew Murray

On Fri, Aug 23, 2019 at 11:50:20PM +, Xiaowei Bao wrote:
> 
> 
> > -Original Message-
> > From: Andrew Murray 
> > Sent: 2019年8月23日 21:25
> > To: Xiaowei Bao 
> > Cc: bhelg...@google.com; robh...@kernel.org; mark.rutl...@arm.com;
> > shawn...@kernel.org; Leo Li ; kis...@ti.com;
> > lorenzo.pieral...@arm.co; a...@arndb.de; gre...@linuxfoundation.org; M.h.
> > Lian ; Mingkai Hu ; Roy
> > Zang ; jingooh...@gmail.com;
> > gustavo.pimen...@synopsys.com; linux-...@vger.kernel.org;
> > devicet...@vger.kernel.org; linux-ker...@vger.kernel.org;
> > linux-arm-ker...@lists.infradead.org; linuxppc-dev@lists.ozlabs.org
> > Subject: Re: [PATCH v2 01/10] PCI: designware-ep: Add multiple PFs support
> > for DWC
> > 
> > On Thu, Aug 22, 2019 at 07:22:33PM +0800, Xiaowei Bao wrote:
> > > Add multiple PFs support for DWC, different PF have different config
> > > space we use pf-offset property which get from the DTS to access the
> > > different pF config space.
> > 
> > It looks like you're missing a --cover-letter again.
> > 
> > >
> > > Signed-off-by: Xiaowei Bao 
> > > ---
> > > v2:
> > >  - Remove duplicate redundant code.
> > >  - Reimplement the PF config space access way.
> > >
> > >  drivers/pci/controller/dwc/pcie-designware-ep.c | 122
> > 
> > >  drivers/pci/controller/dwc/pcie-designware.c|  59 
> > >  drivers/pci/controller/dwc/pcie-designware.h|  11 ++-
> > >  3 files changed, 134 insertions(+), 58 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > index 2bf5a35..3e2b740 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > @@ -19,12 +19,17 @@ void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
> > >   pci_epc_linkup(epc);
> > >  }
> > >
> > > -static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno
> > bar,
> > > -int flags)
> > > +static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, u8 func_no,
> > > +enum pci_barno bar, int flags)
> > >  {
> > >   u32 reg;
> > > + unsigned int func_offset = 0;
> > > + struct dw_pcie_ep *ep = >ep;
> > >
> > > - reg = PCI_BASE_ADDRESS_0 + (4 * bar);
> > > + if (ep->ops->func_conf_select)
> > > + func_offset = ep->ops->func_conf_select(ep, func_no);
> > > +
> > > + reg = func_offset + PCI_BASE_ADDRESS_0 + (4 * bar);
> > 
> > This pattern of checking if func_conf_select exists and using it to get an 
> > offset
> > is repeated a lot throughout this file. You could move this functionality 
> > into a
> > new function (similar to dw_pcie_read_dbi etc). Or perhaps a new variant of
> > dw_pcie_writel_ should be created that writes takes a func_no argument.
> 
> Thanks for your comments, I thought about this method before, but there is a 
> issue
> about the method of access the different func config space, due to our 
> platform use
> this method that different func have different offset from dbi_base to access 
> the
> different config space, but others platform maybe use the way that write a 
> register
> to implement different func config space access, so I think reserve a 
> callback function 

My point here was really to move out duplicated code to its own small function.
I wasn't making any comment about (removing) the callback, just that the test 
and
callback could be in one function.

> to different platform to implement the own method, my point is that, if use 
> register 
> method they can implement the code in this function and return offset is 0, 
> if use 
> offset method, they can return the offset value which can be use by 
> dw_pcie_ep driver.

By the way, I haven't looked to see how many of the dw_pcie_write_xxx functions
would benefit from a func_no argument - if there were many calls to
dw_pcie_write_xxx that all used a reg value originated from func_conf_select
then an approach similar to the implementation of dw_pcie_write_dbi could
probably be justified (i.e. rather than change the value of reg) for writing to
functions.

>  
> > 
> > 
> > >   dw_pcie_dbi_ro_wr_en(pci);
> > >   dw_pcie_writel_dbi2(pci, reg, 0x0);
> > >   dw_pcie_writel_dbi(pci, reg, 0x0);
> > 
> > 
> > > @@ -235,7 +257,7 @@ static int dw_pcie_ep_map_addr(st

RE: [PATCH v2 01/10] PCI: designware-ep: Add multiple PFs support for DWC

2019-08-23 Thread Xiaowei Bao



> -Original Message-
> From: Andrew Murray 
> Sent: 2019年8月23日 21:25
> To: Xiaowei Bao 
> Cc: bhelg...@google.com; robh...@kernel.org; mark.rutl...@arm.com;
> shawn...@kernel.org; Leo Li ; kis...@ti.com;
> lorenzo.pieral...@arm.co; a...@arndb.de; gre...@linuxfoundation.org; M.h.
> Lian ; Mingkai Hu ; Roy
> Zang ; jingooh...@gmail.com;
> gustavo.pimen...@synopsys.com; linux-...@vger.kernel.org;
> devicet...@vger.kernel.org; linux-ker...@vger.kernel.org;
> linux-arm-ker...@lists.infradead.org; linuxppc-dev@lists.ozlabs.org
> Subject: Re: [PATCH v2 01/10] PCI: designware-ep: Add multiple PFs support
> for DWC
> 
> On Thu, Aug 22, 2019 at 07:22:33PM +0800, Xiaowei Bao wrote:
> > Add multiple PFs support for DWC, different PF have different config
> > space we use pf-offset property which get from the DTS to access the
> > different pF config space.
> 
> It looks like you're missing a --cover-letter again.
> 
> >
> > Signed-off-by: Xiaowei Bao 
> > ---
> > v2:
> >  - Remove duplicate redundant code.
> >  - Reimplement the PF config space access way.
> >
> >  drivers/pci/controller/dwc/pcie-designware-ep.c | 122
> 
> >  drivers/pci/controller/dwc/pcie-designware.c|  59 
> >  drivers/pci/controller/dwc/pcie-designware.h|  11 ++-
> >  3 files changed, 134 insertions(+), 58 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index 2bf5a35..3e2b740 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -19,12 +19,17 @@ void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
> > pci_epc_linkup(epc);
> >  }
> >
> > -static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno
> bar,
> > -  int flags)
> > +static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, u8 func_no,
> > +  enum pci_barno bar, int flags)
> >  {
> > u32 reg;
> > +   unsigned int func_offset = 0;
> > +   struct dw_pcie_ep *ep = >ep;
> >
> > -   reg = PCI_BASE_ADDRESS_0 + (4 * bar);
> > +   if (ep->ops->func_conf_select)
> > +   func_offset = ep->ops->func_conf_select(ep, func_no);
> > +
> > +   reg = func_offset + PCI_BASE_ADDRESS_0 + (4 * bar);
> 
> This pattern of checking if func_conf_select exists and using it to get an 
> offset
> is repeated a lot throughout this file. You could move this functionality 
> into a
> new function (similar to dw_pcie_read_dbi etc). Or perhaps a new variant of
> dw_pcie_writel_ should be created that writes takes a func_no argument.

Thanks for your comments, I thought about this method before, but there is a 
issue
about the method of access the different func config space, due to our platform 
use
this method that different func have different offset from dbi_base to access 
the
different config space, but others platform maybe use the way that write a 
register
to implement different func config space access, so I think reserve a callback 
function 
to different platform to implement the own method, my point is that, if use 
register 
method they can implement the code in this function and return offset is 0, if 
use 
offset method, they can return the offset value which can be use by dw_pcie_ep 
driver.
 
> 
> 
> > dw_pcie_dbi_ro_wr_en(pci);
> > dw_pcie_writel_dbi2(pci, reg, 0x0);
> > dw_pcie_writel_dbi(pci, reg, 0x0);
> 
> 
> > @@ -235,7 +257,7 @@ static int dw_pcie_ep_map_addr(struct pci_epc
> *epc, u8 func_no,
> > struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> > struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> >
> > -   ret = dw_pcie_ep_outbound_atu(ep, addr, pci_addr, size);
> > +   ret = dw_pcie_ep_outbound_atu(ep, func_no, addr, pci_addr, size);
> > if (ret) {
> > dev_err(pci->dev, "Failed to enable address\n");
> > return ret;
> > @@ -249,11 +271,15 @@ static int dw_pcie_ep_get_msi(struct pci_epc
> *epc, u8 func_no)
> > struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> > struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > u32 val, reg;
> > +   unsigned int func_offset = 0;
> > +
> > +   if (ep->ops->func_conf_select)
> > +   func_offset = ep->ops->func_conf_select(ep, func_no);
> >
> > if (!ep->msi_cap)
> > return -EINVAL;
> >
> > -   reg = ep->msi_cap + PCI_MSI_FLAGS;
> > +   reg = ep->msi_cap + func_offs

Re: [PATCH v2 01/10] PCI: designware-ep: Add multiple PFs support for DWC

2019-08-23 Thread Andrew Murray

On Thu, Aug 22, 2019 at 07:22:33PM +0800, Xiaowei Bao wrote:
> Add multiple PFs support for DWC, different PF have different config space
> we use pf-offset property which get from the DTS to access the different pF
> config space.

It looks like you're missing a --cover-letter again.

> 
> Signed-off-by: Xiaowei Bao 
> ---
> v2:
>  - Remove duplicate redundant code.
>  - Reimplement the PF config space access way.
> 
>  drivers/pci/controller/dwc/pcie-designware-ep.c | 122 
> 
>  drivers/pci/controller/dwc/pcie-designware.c|  59 
>  drivers/pci/controller/dwc/pcie-designware.h|  11 ++-
>  3 files changed, 134 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c 
> b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 2bf5a35..3e2b740 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -19,12 +19,17 @@ void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
>   pci_epc_linkup(epc);
>  }
>  
> -static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar,
> -int flags)
> +static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, u8 func_no,
> +enum pci_barno bar, int flags)
>  {
>   u32 reg;
> + unsigned int func_offset = 0;
> + struct dw_pcie_ep *ep = >ep;
>  
> - reg = PCI_BASE_ADDRESS_0 + (4 * bar);
> + if (ep->ops->func_conf_select)
> + func_offset = ep->ops->func_conf_select(ep, func_no);
> +
> + reg = func_offset + PCI_BASE_ADDRESS_0 + (4 * bar);

This pattern of checking if func_conf_select exists and using it to get an
offset is repeated a lot throughout this file. You could move this
functionality into a new function (similar to dw_pcie_read_dbi etc). Or
perhaps a new variant of dw_pcie_writel_ should be created that writes takes
a func_no argument.
 

>   dw_pcie_dbi_ro_wr_en(pci);
>   dw_pcie_writel_dbi2(pci, reg, 0x0);
>   dw_pcie_writel_dbi(pci, reg, 0x0);


> @@ -235,7 +257,7 @@ static int dw_pcie_ep_map_addr(struct pci_epc *epc, u8 
> func_no,
>   struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>   struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>  
> - ret = dw_pcie_ep_outbound_atu(ep, addr, pci_addr, size);
> + ret = dw_pcie_ep_outbound_atu(ep, func_no, addr, pci_addr, size);
>   if (ret) {
>   dev_err(pci->dev, "Failed to enable address\n");
>   return ret;
> @@ -249,11 +271,15 @@ static int dw_pcie_ep_get_msi(struct pci_epc *epc, u8 
> func_no)
>   struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>   struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>   u32 val, reg;
> + unsigned int func_offset = 0;
> +
> + if (ep->ops->func_conf_select)
> + func_offset = ep->ops->func_conf_select(ep, func_no);
>  
>   if (!ep->msi_cap)
>   return -EINVAL;
>  
> - reg = ep->msi_cap + PCI_MSI_FLAGS;
> + reg = ep->msi_cap + func_offset + PCI_MSI_FLAGS;

This makes me nervous.

>From a PCI viewpoint, each function has it's own capability structure and
within each function there may exist a MSI capability. Yet what we're doing
here is using dw_pcie_ep_find_capability to get the list of capabilities for
function 0, and then applying offsets from that for subsequent functions. I.e.
we're applying DW specific knowledge to find the correct capability, rather
than following the general PCI approach.

I think the above hunk shouldn't be required - but instead
dw_pcie_ep_find_capability is updated to take a func_no parameter.

Have I understood this correctly?

>   val = dw_pcie_readw_dbi(pci, reg);
>   if (!(val & PCI_MSI_FLAGS_ENABLE))
>   return -EINVAL;
> @@ -268,11 +294,15 @@ static int dw_pcie_ep_set_msi(struct pci_epc *epc, u8 
> func_no, u8 interrupts)
>   struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>   struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>   u32 val, reg;
> + unsigned int func_offset = 0;
> +
> + if (ep->ops->func_conf_select)
> + func_offset = ep->ops->func_conf_select(ep, func_no);
>  
>   if (!ep->msi_cap)
>   return -EINVAL;
>  
> - reg = ep->msi_cap + PCI_MSI_FLAGS;
> + reg = ep->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;
> @@ -288,11 +318,15 @@ static int dw_pcie_ep_get_msix(struct pci_epc *epc, u8 
> func_no)
>   struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>   struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>   u32 val, reg;
> + unsigned int func_offset = 0;
> +
> + if (ep->ops->func_conf_select)
> + func_offset = ep->ops->func_conf_select(ep, func_no);
>  
>   if (!ep->msix_cap)
>   return -EINVAL;
>  
> - reg = ep->msix_cap + PCI_MSIX_FLAGS;
> + reg =

RE: [PATCH v2 01/10] PCI: designware-ep: Add multiple PFs support for DWC

Re: [PATCH v2 01/10] PCI: designware-ep: Add multiple PFs support for DWC

RE: [PATCH v2 01/10] PCI: designware-ep: Add multiple PFs support for DWC

Re: [PATCH v2 01/10] PCI: designware-ep: Add multiple PFs support for DWC

4 matches

Site Navigation

Mail list logo

Footer information