[v6,06/11] PCI: endpoint: Add support to handle multiple base for mapping outbound memory

Commit Message

Prabhakar April 2, 2020, 7:38 p.m. UTC

R-Car PCIe controller has support to map multiple memory regions for
mapping the outbound memory in local system also the controller limits
single allocation for each region (that is, once a chunk is used from the
region it cannot be used to allocate a new one). This features inspires to
add support for handling multiple memory bases in endpoint framework.

With this patch pci_epc_mem_init() initializes address space for endpoint
controller which support single window and whereas __pci_epc_mem_init()
now accepts pointer to multiple windows supported by endpoint controller.

Signed-off-by: Lad Prabhakar <[e-mail]>
---
 .../pci/controller/cadence/pcie-cadence-ep.c  |   3 +-
 .../pci/controller/dwc/pcie-designware-ep.c   |  16 +-
 drivers/pci/controller/pcie-rockchip-ep.c     |   2 +-
 drivers/pci/endpoint/pci-epc-mem.c            | 195 ++++++++++++------
 include/linux/pci-epc.h                       |  39 ++--
 5 files changed, 174 insertions(+), 81 deletions(-)

Comments

Yoshihiro Shimoda April 3, 2020, 8:23 a.m. UTC | #1

Hi Prabhakar-san,

Thank you for the patch!

> From: Lad Prabhakar, Sent: Friday, April 3, 2020 4:39 AM
> 
> R-Car PCIe controller has support to map multiple memory regions for
> mapping the outbound memory in local system also the controller limits
> single allocation for each region (that is, once a chunk is used from the
> region it cannot be used to allocate a new one). This features inspires to
> add support for handling multiple memory bases in endpoint framework.
> 
> With this patch pci_epc_mem_init() initializes address space for endpoint
> controller which support single window and whereas __pci_epc_mem_init()
> now accepts pointer to multiple windows supported by endpoint controller.
> 
> Signed-off-by: Lad Prabhakar <[e-mail]>
> ---
> @@ -38,61 +38,95 @@ static int pci_epc_mem_get_order(struct pci_epc_mem *mem, size_t size)
>  /**
>   * __pci_epc_mem_init() - initialize the pci_epc_mem structure
>   * @epc: the EPC device that invoked pci_epc_mem_init
> - * @phys_base: the physical address of the base
> - * @size: the size of the address space
> - * @page_size: size of each page
> + * @windows: pointer to windows supported by the device
> + * @num_windows: number of windows device supports
>   *
>   * Invoke to initialize the pci_epc_mem structure used by the
>   * endpoint functions to allocate mapped PCI address.
>   */
> -int __pci_epc_mem_init(struct pci_epc *epc, phys_addr_t phys_base, size_t size,
> -		       size_t page_size)
> +int __pci_epc_mem_init(struct pci_epc *epc, struct pci_epc_mem_window *windows,
> +		       unsigned int num_windows)
>  {
> -	int ret;
> -	struct pci_epc_mem *mem;
> -	unsigned long *bitmap;
> +	struct pci_epc_mem *mem = NULL;
> +	unsigned long *bitmap = NULL;
>  	unsigned int page_shift;
> -	int pages;
> +	size_t page_size;
>  	int bitmap_size;
> +	int pages;
> +	int ret;
> +	int i;
> 
> -	if (page_size < PAGE_SIZE)
> -		page_size = PAGE_SIZE;
> +	epc->num_windows = 0;
> 
> -	page_shift = ilog2(page_size);
> -	pages = size >> page_shift;
> -	bitmap_size = BITS_TO_LONGS(pages) * sizeof(long);
> +	if (!windows || !num_windows)
> +		return -EINVAL;
> 
> -	mem = kzalloc(sizeof(*mem), GFP_KERNEL);
> -	if (!mem) {
> -		ret = -ENOMEM;
> -		goto err;
> -	}
> +	epc->windows = kcalloc(num_windows, sizeof(*mem), GFP_KERNEL);
> +	if (!epc->windows)
> +		return -ENOMEM;
> 
> -	bitmap = kzalloc(bitmap_size, GFP_KERNEL);
> -	if (!bitmap) {
> -		ret = -ENOMEM;
> -		goto err_mem;
> -	}
> +	for (i = 0; i < num_windows; i++) {
> +		page_size = windows[i].page_size;
> +		if (page_size < PAGE_SIZE)
> +			page_size = PAGE_SIZE;
> +		page_shift = ilog2(page_size);
> +		pages = windows[i].size >> page_shift;
> +		bitmap_size = BITS_TO_LONGS(pages) * sizeof(long);
> +
> +		mem = kzalloc(sizeof(*mem), GFP_KERNEL);
> +		if (!mem) {
> +			ret = -ENOMEM;
> +			i -= 1;

nit: We can use i--;

> +			goto err_mem;
> +		}
> +
> +		bitmap = kzalloc(bitmap_size, GFP_KERNEL);
> +		if (!bitmap) {
> +			ret = -ENOMEM;
> +			kfree(mem);
> +			i -= 1;

nit: We can use i--;

<snip>
> @@ -122,31 +167,56 @@ EXPORT_SYMBOL_GPL(pci_epc_mem_exit);
>  void __iomem *pci_epc_mem_alloc_addr(struct pci_epc *epc,
>  				     phys_addr_t *phys_addr, size_t size)
>  {
> -	int pageno;
>  	void __iomem *virt_addr = NULL;
> -	struct pci_epc_mem *mem = epc->mem;
> -	unsigned int page_shift = ilog2(mem->page_size);
> +	struct pci_epc_mem *mem;
> +	unsigned int page_shift;
> +	int pageno = -EINVAL;
>  	int order;
> +	int i;
> 
> -	size = ALIGN(size, mem->page_size);
> -	order = pci_epc_mem_get_order(mem, size);
> -
> -	mutex_lock(&mem->lock);
> -	pageno = bitmap_find_free_region(mem->bitmap, mem->pages, order);
> -	if (pageno < 0)
> -		goto ret;
> +	for (i = 0; i < epc->num_windows; i++) {
> +		mem = epc->windows[i];
> +		mutex_lock(&mem->lock);

This is my feeling though, calling mutex_lock() in the loop seems
to cause overhead. And, if we call mutex_lock() at out-of the loop,
I think we can write single mutex_unlock() calling.

> +		size = ALIGN(size, mem->window.page_size);

I'm sorry I should have realized this in the previous review,
but overwriting this size is possible to cause an issue at second time or more loops.
So, the first argument of ALIGN should be kept for the loop.

> +		order = pci_epc_mem_get_order(mem, size);
> 
> -	*phys_addr = mem->phys_base + ((phys_addr_t)pageno << page_shift);
> -	virt_addr = ioremap(*phys_addr, size);
> -	if (!virt_addr)
> -		bitmap_release_region(mem->bitmap, pageno, order);
> +		pageno = bitmap_find_free_region(mem->bitmap, mem->pages,
> +						 order);
> +		if (pageno >= 0) {
> +			page_shift = ilog2(mem->window.page_size);
> +			*phys_addr = mem->window.phys_base +
> +				((phys_addr_t)pageno << page_shift);
> +			virt_addr = ioremap(*phys_addr, size);
> +			if (!virt_addr)
> +				bitmap_release_region(mem->bitmap,
> +						      pageno, order);
> +			mutex_unlock(&mem->lock);
> +			return virt_addr;

As I mentioned above, if mutex_lock() is called at out-of-loop,
we can use "goto ret;" here like the original code,

> +		}
> +		mutex_unlock(&mem->lock);

and we can remove this.

> +	}
> 
> -ret:
> -	mutex_unlock(&mem->lock);
>  	return virt_addr;
>  }
>  EXPORT_SYMBOL_GPL(pci_epc_mem_alloc_addr);
> 
> +struct pci_epc_mem *pci_epc_get_matching_window(struct pci_epc *epc,
> +						phys_addr_t phys_addr)
> +{
> +	struct pci_epc_mem *mem;
> +	int i;
> +
> +	for (i = 0; i < epc->num_windows; i++) {
> +		mem = epc->windows[i];
> +
> +		if (phys_addr >= mem->window.phys_base &&
> +		    phys_addr < (mem->window.phys_base + mem->window.size))
> +			return mem;
> +	}
> +
> +	return NULL;
> +}
> +
>  /**
>   * pci_epc_mem_free_addr() - free the allocated memory address
>   * @epc: the EPC device on which memory was allocated
> @@ -159,14 +229,23 @@ EXPORT_SYMBOL_GPL(pci_epc_mem_alloc_addr);
>  void pci_epc_mem_free_addr(struct pci_epc *epc, phys_addr_t phys_addr,
>  			   void __iomem *virt_addr, size_t size)
>  {
> +	struct pci_epc_mem *mem;
> +	unsigned int page_shift;
> +	size_t page_size;
>  	int pageno;
> -	struct pci_epc_mem *mem = epc->mem;
> -	unsigned int page_shift = ilog2(mem->page_size);
>  	int order;
> 
> +	mem = pci_epc_get_matching_window(epc, phys_addr);
> +	if (!mem) {
> +		pr_err("failed to get matching window\n");
> +		return;
> +	}
> +
> +	page_size = mem->window.page_size;
> +	page_shift = ilog2(page_size);
>  	iounmap(virt_addr);
> -	pageno = (phys_addr - mem->phys_base) >> page_shift;
> -	size = ALIGN(size, mem->page_size);
> +	pageno = (phys_addr - mem->window.phys_base) >> page_shift;
> +	size = ALIGN(size, page_size);
>  	order = pci_epc_mem_get_order(mem, size);
>  	mutex_lock(&mem->lock);
>  	bitmap_release_region(mem->bitmap, pageno, order);
> diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
> index e0ed9d01f6e5..d5da11cf0f2a 100644
> --- a/include/linux/pci-epc.h
> +++ b/include/linux/pci-epc.h
> @@ -65,20 +65,28 @@ struct pci_epc_ops {
>  	struct module *owner;
>  };
> 
> +/**
> + * struct pci_epc_mem_window - address window of the endpoint controller
> + * @phys_base: physical base address of the PCI address window
> + * @size: the size of the PCI address window
> + * @page_size: size of each page
> + */
> +struct pci_epc_mem_window {
> +	phys_addr_t	phys_base;
> +	size_t		size;
> +	size_t		page_size;
> +};
> +
>  /**
>   * struct pci_epc_mem - address space of the endpoint controller
> - * @phys_base: physical base address of the PCI address space
> - * @size: the size of the PCI address space
> + * @window: address window of the endpoint controller
>   * @bitmap: bitmap to manage the PCI address space
> - * @pages: number of bits representing the address region
> - * @page_size: size of each page
>   * @lock: mutex to protect bitmap
> + * @pages: number of bits representing the address region

Perhaps, we should not change the "@pages" line.

Best regards,
Yoshihiro Shimoda

Prabhakar April 3, 2020, 9:11 a.m. UTC | #2

Hi Shimoda-san,

Thank you for the review.

> -----Original Message-----
> From: Yoshihiro Shimoda <[e-mail]>
> Sent: 03 April 2020 09:23
> To: Prabhakar Mahadev Lad <[e-mail]>; Bjorn Helgaas <[e-mail]>; Rob Herring
> <[e-mail]>; Mark Rutland <[e-mail]>; Geert Uytterhoeven <[e-mail]>; Magnus Damm
> <[e-mail]>; Kishon Vijay Abraham I <[e-mail]>; Lorenzo Pieralisi <[e-mail]>; Marek Vasut
> <[e-mail]>; [e-mail]
> Cc: Catalin Marinas <[e-mail]>; Will Deacon <[e-mail]>; Arnd Bergmann <[e-mail]>; Greg Kroah-Hartman
> <[e-mail]>; Andrew Murray <[e-mail]>; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Chris Paterson <[e-mail]>; Frank Rowand
> <[e-mail]>; Gustavo Pimentel <[e-mail]>; Jingoo Han <[e-mail]>; Simon Horman
> <[e-mail]>; Shawn Lin <[e-mail]>; Tom Joseph <[e-mail]>; Heiko Stuebner
> <[e-mail]>; [e-mail]; Lad Prabhakar <[e-mail]>; Prabhakar Mahadev Lad
> <[e-mail]>
> Subject: RE: [PATCH v6 06/11] PCI: endpoint: Add support to handle multiple base for mapping outbound memory
>
> Hi Prabhakar-san,
>
> Thank you for the patch!
>
> > From: Lad Prabhakar, Sent: Friday, April 3, 2020 4:39 AM
> >
> > R-Car PCIe controller has support to map multiple memory regions for
> > mapping the outbound memory in local system also the controller limits
> > single allocation for each region (that is, once a chunk is used from the
> > region it cannot be used to allocate a new one). This features inspires to
> > add support for handling multiple memory bases in endpoint framework.
> >
> > With this patch pci_epc_mem_init() initializes address space for endpoint
> > controller which support single window and whereas __pci_epc_mem_init()
> > now accepts pointer to multiple windows supported by endpoint controller.
> >
> > Signed-off-by: Lad Prabhakar <[e-mail]>
> > ---
> > @@ -38,61 +38,95 @@ static int pci_epc_mem_get_order(struct pci_epc_mem *mem, size_t size)
> >  /**
> >   * __pci_epc_mem_init() - initialize the pci_epc_mem structure
> >   * @epc: the EPC device that invoked pci_epc_mem_init
> > - * @phys_base: the physical address of the base
> > - * @size: the size of the address space
> > - * @page_size: size of each page
> > + * @windows: pointer to windows supported by the device
> > + * @num_windows: number of windows device supports
> >   *
> >   * Invoke to initialize the pci_epc_mem structure used by the
> >   * endpoint functions to allocate mapped PCI address.
> >   */
> > -int __pci_epc_mem_init(struct pci_epc *epc, phys_addr_t phys_base, size_t size,
> > -       size_t page_size)
> > +int __pci_epc_mem_init(struct pci_epc *epc, struct pci_epc_mem_window *windows,
> > +       unsigned int num_windows)
> >  {
> > -int ret;
> > -struct pci_epc_mem *mem;
> > -unsigned long *bitmap;
> > +struct pci_epc_mem *mem = NULL;
> > +unsigned long *bitmap = NULL;
> >  unsigned int page_shift;
> > -int pages;
> > +size_t page_size;
> >  int bitmap_size;
> > +int pages;
> > +int ret;
> > +int i;
> >
> > -if (page_size < PAGE_SIZE)
> > -page_size = PAGE_SIZE;
> > +epc->num_windows = 0;
> >
> > -page_shift = ilog2(page_size);
> > -pages = size >> page_shift;
> > -bitmap_size = BITS_TO_LONGS(pages) * sizeof(long);
> > +if (!windows || !num_windows)
> > +return -EINVAL;
> >
> > -mem = kzalloc(sizeof(*mem), GFP_KERNEL);
> > -if (!mem) {
> > -ret = -ENOMEM;
> > -goto err;
> > -}
> > +epc->windows = kcalloc(num_windows, sizeof(*mem), GFP_KERNEL);
> > +if (!epc->windows)
> > +return -ENOMEM;
> >
> > -bitmap = kzalloc(bitmap_size, GFP_KERNEL);
> > -if (!bitmap) {
> > -ret = -ENOMEM;
> > -goto err_mem;
> > -}
> > +for (i = 0; i < num_windows; i++) {
> > +page_size = windows[i].page_size;
> > +if (page_size < PAGE_SIZE)
> > +page_size = PAGE_SIZE;
> > +page_shift = ilog2(page_size);
> > +pages = windows[i].size >> page_shift;
> > +bitmap_size = BITS_TO_LONGS(pages) * sizeof(long);
> > +
> > +mem = kzalloc(sizeof(*mem), GFP_KERNEL);
> > +if (!mem) {
> > +ret = -ENOMEM;
> > +i -= 1;
>
> nit: We can use i--;
>
Will change it.

> > +goto err_mem;
> > +}
> > +
> > +bitmap = kzalloc(bitmap_size, GFP_KERNEL);
> > +if (!bitmap) {
> > +ret = -ENOMEM;
> > +kfree(mem);
> > +i -= 1;
>
> nit: We can use i--;
>
As above.

> <snip>
> > @@ -122,31 +167,56 @@ EXPORT_SYMBOL_GPL(pci_epc_mem_exit);
> >  void __iomem *pci_epc_mem_alloc_addr(struct pci_epc *epc,
> >       phys_addr_t *phys_addr, size_t size)
> >  {
> > -int pageno;
> >  void __iomem *virt_addr = NULL;
> > -struct pci_epc_mem *mem = epc->mem;
> > -unsigned int page_shift = ilog2(mem->page_size);
> > +struct pci_epc_mem *mem;
> > +unsigned int page_shift;
> > +int pageno = -EINVAL;
> >  int order;
> > +int i;
> >
> > -size = ALIGN(size, mem->page_size);
> > -order = pci_epc_mem_get_order(mem, size);
> > -
> > -mutex_lock(&mem->lock);
> > -pageno = bitmap_find_free_region(mem->bitmap, mem->pages, order);
> > -if (pageno < 0)
> > -goto ret;
> > +for (i = 0; i < epc->num_windows; i++) {
> > +mem = epc->windows[i];
> > +mutex_lock(&mem->lock);
>
> This is my feeling though, calling mutex_lock() in the loop seems
> to cause overhead. And, if we call mutex_lock() at out-of the loop,
> I think we can write single mutex_unlock() calling.
>
But the mutex is for each window, are you suggesting to add a global mutex ?

> > +size = ALIGN(size, mem->window.page_size);
>
> I'm sorry I should have realized this in the previous review,
> but overwriting this size is possible to cause an issue at second time or more loops.
> So, the first argument of ALIGN should be kept for the loop.
>
Could you please elaborate on this.

> > +order = pci_epc_mem_get_order(mem, size);
> >
> > -*phys_addr = mem->phys_base + ((phys_addr_t)pageno << page_shift);
> > -virt_addr = ioremap(*phys_addr, size);
> > -if (!virt_addr)
> > -bitmap_release_region(mem->bitmap, pageno, order);
> > +pageno = bitmap_find_free_region(mem->bitmap, mem->pages,
> > + order);
> > +if (pageno >= 0) {
> > +page_shift = ilog2(mem->window.page_size);
> > +*phys_addr = mem->window.phys_base +
> > +((phys_addr_t)pageno << page_shift);
> > +virt_addr = ioremap(*phys_addr, size);
> > +if (!virt_addr)
> > +bitmap_release_region(mem->bitmap,
> > +      pageno, order);
> > +mutex_unlock(&mem->lock);
> > +return virt_addr;
>
> As I mentioned above, if mutex_lock() is called at out-of-loop,
> we can use "goto ret;" here like the original code,
>
> > +}
> > +mutex_unlock(&mem->lock);
>
> and we can remove this.
>
> > +}
> >
> > -ret:
> > -mutex_unlock(&mem->lock);
> >  return virt_addr;
> >  }
> >  EXPORT_SYMBOL_GPL(pci_epc_mem_alloc_addr);
> >
> > +struct pci_epc_mem *pci_epc_get_matching_window(struct pci_epc *epc,
> > +phys_addr_t phys_addr)
> > +{
> > +struct pci_epc_mem *mem;
> > +int i;
> > +
> > +for (i = 0; i < epc->num_windows; i++) {
> > +mem = epc->windows[i];
> > +
> > +if (phys_addr >= mem->window.phys_base &&
> > +    phys_addr < (mem->window.phys_base + mem->window.size))
> > +return mem;
> > +}
> > +
> > +return NULL;
> > +}
> > +
> >  /**
> >   * pci_epc_mem_free_addr() - free the allocated memory address
> >   * @epc: the EPC device on which memory was allocated
> > @@ -159,14 +229,23 @@ EXPORT_SYMBOL_GPL(pci_epc_mem_alloc_addr);
> >  void pci_epc_mem_free_addr(struct pci_epc *epc, phys_addr_t phys_addr,
> >     void __iomem *virt_addr, size_t size)
> >  {
> > +struct pci_epc_mem *mem;
> > +unsigned int page_shift;
> > +size_t page_size;
> >  int pageno;
> > -struct pci_epc_mem *mem = epc->mem;
> > -unsigned int page_shift = ilog2(mem->page_size);
> >  int order;
> >
> > +mem = pci_epc_get_matching_window(epc, phys_addr);
> > +if (!mem) {
> > +pr_err("failed to get matching window\n");
> > +return;
> > +}
> > +
> > +page_size = mem->window.page_size;
> > +page_shift = ilog2(page_size);
> >  iounmap(virt_addr);
> > -pageno = (phys_addr - mem->phys_base) >> page_shift;
> > -size = ALIGN(size, mem->page_size);
> > +pageno = (phys_addr - mem->window.phys_base) >> page_shift;
> > +size = ALIGN(size, page_size);
> >  order = pci_epc_mem_get_order(mem, size);
> >  mutex_lock(&mem->lock);
> >  bitmap_release_region(mem->bitmap, pageno, order);
> > diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
> > index e0ed9d01f6e5..d5da11cf0f2a 100644
> > --- a/include/linux/pci-epc.h
> > +++ b/include/linux/pci-epc.h
> > @@ -65,20 +65,28 @@ struct pci_epc_ops {
> >  struct module *owner;
> >  };
> >
> > +/**
> > + * struct pci_epc_mem_window - address window of the endpoint controller
> > + * @phys_base: physical base address of the PCI address window
> > + * @size: the size of the PCI address window
> > + * @page_size: size of each page
> > + */
> > +struct pci_epc_mem_window {
> > +phys_addr_tphys_base;
> > +size_tsize;
> > +size_tpage_size;
> > +};
> > +
> >  /**
> >   * struct pci_epc_mem - address space of the endpoint controller
> > - * @phys_base: physical base address of the PCI address space
> > - * @size: the size of the PCI address space
> > + * @window: address window of the endpoint controller
> >   * @bitmap: bitmap to manage the PCI address space
> > - * @pages: number of bits representing the address region
> > - * @page_size: size of each page
> >   * @lock: mutex to protect bitmap
> > + * @pages: number of bits representing the address region
>
> Perhaps, we should not change the "@pages" line.
>
OK will drop this change.

Cheers,
--Prabhakar

> Best regards,
> Yoshihiro Shimoda



Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647

Yoshihiro Shimoda April 3, 2020, 9:34 a.m. UTC | #3

Hi Prabhakar-san,

> From: Prabhakar Mahadev Lad, Sent: Friday, April 3, 2020 6:12 PM
<snip>
> > > @@ -122,31 +167,56 @@ EXPORT_SYMBOL_GPL(pci_epc_mem_exit);
> > >  void __iomem *pci_epc_mem_alloc_addr(struct pci_epc *epc,
> > >  				     phys_addr_t *phys_addr, size_t size)
> > >  {
> > > -	int pageno;
> > >  	void __iomem *virt_addr = NULL;
> > > -	struct pci_epc_mem *mem = epc->mem;
> > > -	unsigned int page_shift = ilog2(mem->page_size);
> > > +	struct pci_epc_mem *mem;
> > > +	unsigned int page_shift;
> > > +	int pageno = -EINVAL;
> > >  	int order;
> > > +	int i;
> > >
> > > -	size = ALIGN(size, mem->page_size);
> > > -	order = pci_epc_mem_get_order(mem, size);
> > > -
> > > -	mutex_lock(&mem->lock);
> > > -	pageno = bitmap_find_free_region(mem->bitmap, mem->pages, order);
> > > -	if (pageno < 0)
> > > -		goto ret;
> > > +	for (i = 0; i < epc->num_windows; i++) {
> > > +		mem = epc->windows[i];
> > > +		mutex_lock(&mem->lock);
> >
> > This is my feeling though, calling mutex_lock() in the loop seems
> > to cause overhead. And, if we call mutex_lock() at out-of the loop,
> > I think we can write single mutex_unlock() calling.
> >
> But the mutex is for each window, are you suggesting to add a global mutex ?

Oops, that's right. So, I'd like to recall.

> > > +		size = ALIGN(size, mem->window.page_size);
> >
> > I'm sorry I should have realized this in the previous review,
> > but overwriting this size is possible to cause an issue at second time or more loops.
> > So, the first argument of ALIGN should be kept for the loop.
> >
> Could you please elaborate on this.

My concern is the following.

For example, the size of argument of pci_epc_mem_alloc_addr() is 4096.
epc->windows[0].window.page_size = 8192
 --> then the size will be changed to 0.

epc->windows[1].window.page_size = 4096
 --> since the size was changed to 0 on the first loop, the result is 0.
     But, this should be 4096.

Does such a case never happen?
(Or, is my understanding incorrect?)

Best regards,
Yoshihiro Shimoda


> > > +		order = pci_epc_mem_get_order(mem, size);
> > >
> > > -	*phys_addr = mem->phys_base + ((phys_addr_t)pageno << page_shift);
> > > -	virt_addr = ioremap(*phys_addr, size);
> > > -	if (!virt_addr)
> > > -		bitmap_release_region(mem->bitmap, pageno, order);
> > > +		pageno = bitmap_find_free_region(mem->bitmap, mem->pages,
> > > +						 order);
> > > +		if (pageno >= 0) {
> > > +			page_shift = ilog2(mem->window.page_size);
> > > +			*phys_addr = mem->window.phys_base +
> > > +				((phys_addr_t)pageno << page_shift);
> > > +			virt_addr = ioremap(*phys_addr, size);
> > > +			if (!virt_addr)
> > > +				bitmap_release_region(mem->bitmap,
> > > +						      pageno, order);
> > > +			mutex_unlock(&mem->lock);
> > > +			return virt_addr;
> >
> > As I mentioned above, if mutex_lock() is called at out-of-loop,
> > we can use "goto ret;" here like the original code,
> >
> > > +		}
> > > +		mutex_unlock(&mem->lock);
> >
> > and we can remove this.
> >
> > > +	}
> > >
> > > -ret:
> > > -	mutex_unlock(&mem->lock);
> > >  	return virt_addr;
> > >  }
> > >  EXPORT_SYMBOL_GPL(pci_epc_mem_alloc_addr);
> > >
> > > +struct pci_epc_mem *pci_epc_get_matching_window(struct pci_epc *epc,
> > > +						phys_addr_t phys_addr)
> > > +{
> > > +	struct pci_epc_mem *mem;
> > > +	int i;
> > > +
> > > +	for (i = 0; i < epc->num_windows; i++) {
> > > +		mem = epc->windows[i];
> > > +
> > > +		if (phys_addr >= mem->window.phys_base &&
> > > +		    phys_addr < (mem->window.phys_base + mem->window.size))
> > > +			return mem;
> > > +	}
> > > +
> > > +	return NULL;
> > > +}
> > > +
> > >  /**
> > >   * pci_epc_mem_free_addr() - free the allocated memory address
> > >   * @epc: the EPC device on which memory was allocated
> > > @@ -159,14 +229,23 @@ EXPORT_SYMBOL_GPL(pci_epc_mem_alloc_addr);
> > >  void pci_epc_mem_free_addr(struct pci_epc *epc, phys_addr_t phys_addr,
> > >  			   void __iomem *virt_addr, size_t size)
> > >  {
> > > +	struct pci_epc_mem *mem;
> > > +	unsigned int page_shift;
> > > +	size_t page_size;
> > >  	int pageno;
> > > -	struct pci_epc_mem *mem = epc->mem;
> > > -	unsigned int page_shift = ilog2(mem->page_size);
> > >  	int order;
> > >
> > > +	mem = pci_epc_get_matching_window(epc, phys_addr);
> > > +	if (!mem) {
> > > +		pr_err("failed to get matching window\n");
> > > +		return;
> > > +	}
> > > +
> > > +	page_size = mem->window.page_size;
> > > +	page_shift = ilog2(page_size);
> > >  	iounmap(virt_addr);
> > > -	pageno = (phys_addr - mem->phys_base) >> page_shift;
> > > -	size = ALIGN(size, mem->page_size);
> > > +	pageno = (phys_addr - mem->window.phys_base) >> page_shift;
> > > +	size = ALIGN(size, page_size);
> > >  	order = pci_epc_mem_get_order(mem, size);
> > >  	mutex_lock(&mem->lock);
> > >  	bitmap_release_region(mem->bitmap, pageno, order);
> > > diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
> > > index e0ed9d01f6e5..d5da11cf0f2a 100644
> > > --- a/include/linux/pci-epc.h
> > > +++ b/include/linux/pci-epc.h
> > > @@ -65,20 +65,28 @@ struct pci_epc_ops {
> > >  	struct module *owner;
> > >  };
> > >
> > > +/**
> > > + * struct pci_epc_mem_window - address window of the endpoint controller
> > > + * @phys_base: physical base address of the PCI address window
> > > + * @size: the size of the PCI address window
> > > + * @page_size: size of each page
> > > + */
> > > +struct pci_epc_mem_window {
> > > +	phys_addr_t	phys_base;
> > > +	size_t		size;
> > > +	size_t		page_size;
> > > +};
> > > +
> > >  /**
> > >   * struct pci_epc_mem - address space of the endpoint controller
> > > - * @phys_base: physical base address of the PCI address space
> > > - * @size: the size of the PCI address space
> > > + * @window: address window of the endpoint controller
> > >   * @bitmap: bitmap to manage the PCI address space
> > > - * @pages: number of bits representing the address region
> > > - * @page_size: size of each page
> > >   * @lock: mutex to protect bitmap
> > > + * @pages: number of bits representing the address region
> >
> > Perhaps, we should not change the "@pages" line.
> >
> OK will drop this change.
> 
> Cheers,
> --Prabhakar
> 
> > Best regards,
> > Yoshihiro Shimoda

Prabhakar April 3, 2020, 9:47 a.m. UTC | #4

Hi Shimoda-san,

> -----Original Message-----
> From: Yoshihiro Shimoda <[e-mail]>
> Sent: 03 April 2020 10:34
> To: Prabhakar Mahadev Lad <[e-mail]>; Bjorn Helgaas <[e-mail]>; Rob Herring
> <[e-mail]>; Mark Rutland <[e-mail]>; Geert Uytterhoeven <[e-mail]>; Magnus Damm
> <[e-mail]>; Kishon Vijay Abraham I <[e-mail]>; Lorenzo Pieralisi <[e-mail]>; Marek Vasut
> <[e-mail]>; [e-mail]
> Cc: Catalin Marinas <[e-mail]>; Will Deacon <[e-mail]>; Arnd Bergmann <[e-mail]>; Greg Kroah-Hartman
> <[e-mail]>; Andrew Murray <[e-mail]>; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Chris Paterson <[e-mail]>; Frank Rowand
> <[e-mail]>; Gustavo Pimentel <[e-mail]>; Jingoo Han <[e-mail]>; Simon Horman
> <[e-mail]>; Shawn Lin <[e-mail]>; Tom Joseph <[e-mail]>; Heiko Stuebner
> <[e-mail]>; [e-mail]; Lad Prabhakar <[e-mail]>
> Subject: RE: [PATCH v6 06/11] PCI: endpoint: Add support to handle multiple base for mapping outbound memory
>
> Hi Prabhakar-san,
>
> > From: Prabhakar Mahadev Lad, Sent: Friday, April 3, 2020 6:12 PM
> <snip>
> > > > @@ -122,31 +167,56 @@ EXPORT_SYMBOL_GPL(pci_epc_mem_exit);
> > > >  void __iomem *pci_epc_mem_alloc_addr(struct pci_epc *epc,
> > > >       phys_addr_t *phys_addr, size_t size)
> > > >  {
> > > > -int pageno;
> > > >  void __iomem *virt_addr = NULL;
> > > > -struct pci_epc_mem *mem = epc->mem;
> > > > -unsigned int page_shift = ilog2(mem->page_size);
> > > > +struct pci_epc_mem *mem;
> > > > +unsigned int page_shift;
> > > > +int pageno = -EINVAL;
> > > >  int order;
> > > > +int i;
> > > >
> > > > -size = ALIGN(size, mem->page_size);
> > > > -order = pci_epc_mem_get_order(mem, size);
> > > > -
> > > > -mutex_lock(&mem->lock);
> > > > -pageno = bitmap_find_free_region(mem->bitmap, mem->pages, order);
> > > > -if (pageno < 0)
> > > > -goto ret;
> > > > +for (i = 0; i < epc->num_windows; i++) {
> > > > +mem = epc->windows[i];
> > > > +mutex_lock(&mem->lock);
> > >
> > > This is my feeling though, calling mutex_lock() in the loop seems
> > > to cause overhead. And, if we call mutex_lock() at out-of the loop,
> > > I think we can write single mutex_unlock() calling.
> > >
> > But the mutex is for each window, are you suggesting to add a global mutex ?
>
> Oops, that's right. So, I'd like to recall.
>
> > > > +size = ALIGN(size, mem->window.page_size);
> > >
> > > I'm sorry I should have realized this in the previous review,
> > > but overwriting this size is possible to cause an issue at second time or more loops.
> > > So, the first argument of ALIGN should be kept for the loop.
> > >
> > Could you please elaborate on this.
>
> My concern is the following.
>
> For example, the size of argument of pci_epc_mem_alloc_addr() is 4096.
> epc->windows[0].window.page_size = 8192
>  --> then the size will be changed to 0.
>
> epc->windows[1].window.page_size = 4096
>  --> since the size was changed to 0 on the first loop, the result is 0.
>      But, this should be 4096.
>
> Does such a case never happen?
> (Or, is my understanding incorrect?)
>
Good catch, yes that needs fixing probably by having a local variable for size.

Cheers,
--Prabhakar

> Best regards,
> Yoshihiro Shimoda
>
>
> > > > +order = pci_epc_mem_get_order(mem, size);
> > > >
> > > > -*phys_addr = mem->phys_base + ((phys_addr_t)pageno << page_shift);
> > > > -virt_addr = ioremap(*phys_addr, size);
> > > > -if (!virt_addr)
> > > > -bitmap_release_region(mem->bitmap, pageno, order);
> > > > +pageno = bitmap_find_free_region(mem->bitmap, mem->pages,
> > > > + order);
> > > > +if (pageno >= 0) {
> > > > +page_shift = ilog2(mem->window.page_size);
> > > > +*phys_addr = mem->window.phys_base +
> > > > +((phys_addr_t)pageno << page_shift);
> > > > +virt_addr = ioremap(*phys_addr, size);
> > > > +if (!virt_addr)
> > > > +bitmap_release_region(mem->bitmap,
> > > > +      pageno, order);
> > > > +mutex_unlock(&mem->lock);
> > > > +return virt_addr;
> > >
> > > As I mentioned above, if mutex_lock() is called at out-of-loop,
> > > we can use "goto ret;" here like the original code,
> > >
> > > > +}
> > > > +mutex_unlock(&mem->lock);
> > >
> > > and we can remove this.
> > >
> > > > +}
> > > >
> > > > -ret:
> > > > -mutex_unlock(&mem->lock);
> > > >  return virt_addr;
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(pci_epc_mem_alloc_addr);
> > > >
> > > > +struct pci_epc_mem *pci_epc_get_matching_window(struct pci_epc *epc,
> > > > +phys_addr_t phys_addr)
> > > > +{
> > > > +struct pci_epc_mem *mem;
> > > > +int i;
> > > > +
> > > > +for (i = 0; i < epc->num_windows; i++) {
> > > > +mem = epc->windows[i];
> > > > +
> > > > +if (phys_addr >= mem->window.phys_base &&
> > > > +    phys_addr < (mem->window.phys_base + mem->window.size))
> > > > +return mem;
> > > > +}
> > > > +
> > > > +return NULL;
> > > > +}
> > > > +
> > > >  /**
> > > >   * pci_epc_mem_free_addr() - free the allocated memory address
> > > >   * @epc: the EPC device on which memory was allocated
> > > > @@ -159,14 +229,23 @@ EXPORT_SYMBOL_GPL(pci_epc_mem_alloc_addr);
> > > >  void pci_epc_mem_free_addr(struct pci_epc *epc, phys_addr_t phys_addr,
> > > >     void __iomem *virt_addr, size_t size)
> > > >  {
> > > > +struct pci_epc_mem *mem;
> > > > +unsigned int page_shift;
> > > > +size_t page_size;
> > > >  int pageno;
> > > > -struct pci_epc_mem *mem = epc->mem;
> > > > -unsigned int page_shift = ilog2(mem->page_size);
> > > >  int order;
> > > >
> > > > +mem = pci_epc_get_matching_window(epc, phys_addr);
> > > > +if (!mem) {
> > > > +pr_err("failed to get matching window\n");
> > > > +return;
> > > > +}
> > > > +
> > > > +page_size = mem->window.page_size;
> > > > +page_shift = ilog2(page_size);
> > > >  iounmap(virt_addr);
> > > > -pageno = (phys_addr - mem->phys_base) >> page_shift;
> > > > -size = ALIGN(size, mem->page_size);
> > > > +pageno = (phys_addr - mem->window.phys_base) >> page_shift;
> > > > +size = ALIGN(size, page_size);
> > > >  order = pci_epc_mem_get_order(mem, size);
> > > >  mutex_lock(&mem->lock);
> > > >  bitmap_release_region(mem->bitmap, pageno, order);
> > > > diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
> > > > index e0ed9d01f6e5..d5da11cf0f2a 100644
> > > > --- a/include/linux/pci-epc.h
> > > > +++ b/include/linux/pci-epc.h
> > > > @@ -65,20 +65,28 @@ struct pci_epc_ops {
> > > >  struct module *owner;
> > > >  };
> > > >
> > > > +/**
> > > > + * struct pci_epc_mem_window - address window of the endpoint controller
> > > > + * @phys_base: physical base address of the PCI address window
> > > > + * @size: the size of the PCI address window
> > > > + * @page_size: size of each page
> > > > + */
> > > > +struct pci_epc_mem_window {
> > > > +phys_addr_tphys_base;
> > > > +size_tsize;
> > > > +size_tpage_size;
> > > > +};
> > > > +
> > > >  /**
> > > >   * struct pci_epc_mem - address space of the endpoint controller
> > > > - * @phys_base: physical base address of the PCI address space
> > > > - * @size: the size of the PCI address space
> > > > + * @window: address window of the endpoint controller
> > > >   * @bitmap: bitmap to manage the PCI address space
> > > > - * @pages: number of bits representing the address region
> > > > - * @page_size: size of each page
> > > >   * @lock: mutex to protect bitmap
> > > > + * @pages: number of bits representing the address region
> > >
> > > Perhaps, we should not change the "@pages" line.
> > >
> > OK will drop this change.
> >
> > Cheers,
> > --Prabhakar
> >
> > > Best regards,
> > > Yoshihiro Shimoda



Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647

Patch

diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
index 1c173dad67d1..d62eec6bbbbb 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
@@ -450,7 +450,8 @@  int cdns_pcie_ep_setup(struct cdns_pcie_ep *ep)
 		epc->max_functions = 1;
 
 	ret = pci_epc_mem_init(epc, pcie->mem_res->start,
-			       resource_size(pcie->mem_res));
+			       resource_size(pcie->mem_res),
+			       PAGE_SIZE);
 	if (ret < 0) {
 		dev_err(dev, "failed to initialize the memory space\n");
 		goto err_init;
diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 1cdcbd102ce8..a78902cbf2f0 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -412,11 +412,11 @@  int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no,
 		reg = ep->msi_cap + PCI_MSI_DATA_32;
 		msg_data = dw_pcie_readw_dbi(pci, reg);
 	}
-	aligned_offset = msg_addr_lower & (epc->mem->page_size - 1);
+	aligned_offset = msg_addr_lower & (epc->mem->window.page_size - 1);
 	msg_addr = ((u64)msg_addr_upper) << 32 |
 			(msg_addr_lower & ~aligned_offset);
 	ret = dw_pcie_ep_map_addr(epc, func_no, ep->msi_mem_phys, msg_addr,
-				  epc->mem->page_size);
+				  epc->mem->window.page_size);
 	if (ret)
 		return ret;
 
@@ -459,9 +459,9 @@  int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no,
 		return -EPERM;
 	}
 
-	aligned_offset = msg_addr & (epc->mem->page_size - 1);
+	aligned_offset = msg_addr & (epc->mem->window.page_size - 1);
 	ret = dw_pcie_ep_map_addr(epc, func_no, ep->msi_mem_phys,  msg_addr,
-				  epc->mem->page_size);
+				  epc->mem->window.page_size);
 	if (ret)
 		return ret;
 
@@ -477,7 +477,7 @@  void dw_pcie_ep_exit(struct dw_pcie_ep *ep)
 	struct pci_epc *epc = ep->epc;
 
 	pci_epc_mem_free_addr(epc, ep->msi_mem_phys, ep->msi_mem,
-			      epc->mem->page_size);
+			      epc->mem->window.page_size);
 
 	pci_epc_mem_exit(epc);
 }
@@ -610,15 +610,15 @@  int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 	if (ret < 0)
 		epc->max_functions = 1;
 
-	ret = __pci_epc_mem_init(epc, ep->phys_base, ep->addr_size,
-				 ep->page_size);
+	ret = pci_epc_mem_init(epc, ep->phys_base, ep->addr_size,
+			       ep->page_size);
 	if (ret < 0) {
 		dev_err(dev, "Failed to initialize address space\n");
 		return ret;
 	}
 
 	ep->msi_mem = pci_epc_mem_alloc_addr(epc, &ep->msi_mem_phys,
-					     epc->mem->page_size);
+					     epc->mem->window.page_size);
 	if (!ep->msi_mem) {
 		dev_err(dev, "Failed to reserve memory for MSI/MSI-X\n");
 		return -ENOMEM;
diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index d743b0a48988..5eaf36629a75 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -615,7 +615,7 @@  static int rockchip_pcie_ep_probe(struct platform_device *pdev)
 	rockchip_pcie_write(rockchip, BIT(0), PCIE_CORE_PHY_FUNC_CFG);
 
 	err = pci_epc_mem_init(epc, rockchip->mem_res->start,
-			       resource_size(rockchip->mem_res));
+			       resource_size(rockchip->mem_res), PAGE_SIZE);
 	if (err < 0) {
 		dev_err(dev, "failed to initialize the memory space\n");
 		goto err_uninit_port;
diff --git a/drivers/pci/endpoint/pci-epc-mem.c b/drivers/pci/endpoint/pci-epc-mem.c
index abfac1109a13..87effe825f81 100644
--- a/drivers/pci/endpoint/pci-epc-mem.c
+++ b/drivers/pci/endpoint/pci-epc-mem.c
@@ -23,7 +23,7 @@ 
 static int pci_epc_mem_get_order(struct pci_epc_mem *mem, size_t size)
 {
 	int order;
-	unsigned int page_shift = ilog2(mem->page_size);
+	unsigned int page_shift = ilog2(mem->window.page_size);
 
 	size--;
 	size >>= page_shift;
@@ -38,61 +38,95 @@  static int pci_epc_mem_get_order(struct pci_epc_mem *mem, size_t size)
 /**
  * __pci_epc_mem_init() - initialize the pci_epc_mem structure
  * @epc: the EPC device that invoked pci_epc_mem_init
- * @phys_base: the physical address of the base
- * @size: the size of the address space
- * @page_size: size of each page
+ * @windows: pointer to windows supported by the device
+ * @num_windows: number of windows device supports
  *
  * Invoke to initialize the pci_epc_mem structure used by the
  * endpoint functions to allocate mapped PCI address.
  */
-int __pci_epc_mem_init(struct pci_epc *epc, phys_addr_t phys_base, size_t size,
-		       size_t page_size)
+int __pci_epc_mem_init(struct pci_epc *epc, struct pci_epc_mem_window *windows,
+		       unsigned int num_windows)
 {
-	int ret;
-	struct pci_epc_mem *mem;
-	unsigned long *bitmap;
+	struct pci_epc_mem *mem = NULL;
+	unsigned long *bitmap = NULL;
 	unsigned int page_shift;
-	int pages;
+	size_t page_size;
 	int bitmap_size;
+	int pages;
+	int ret;
+	int i;
 
-	if (page_size < PAGE_SIZE)
-		page_size = PAGE_SIZE;
+	epc->num_windows = 0;
 
-	page_shift = ilog2(page_size);
-	pages = size >> page_shift;
-	bitmap_size = BITS_TO_LONGS(pages) * sizeof(long);
+	if (!windows || !num_windows)
+		return -EINVAL;
 
-	mem = kzalloc(sizeof(*mem), GFP_KERNEL);
-	if (!mem) {
-		ret = -ENOMEM;
-		goto err;
-	}
+	epc->windows = kcalloc(num_windows, sizeof(*mem), GFP_KERNEL);
+	if (!epc->windows)
+		return -ENOMEM;
 
-	bitmap = kzalloc(bitmap_size, GFP_KERNEL);
-	if (!bitmap) {
-		ret = -ENOMEM;
-		goto err_mem;
-	}
+	for (i = 0; i < num_windows; i++) {
+		page_size = windows[i].page_size;
+		if (page_size < PAGE_SIZE)
+			page_size = PAGE_SIZE;
+		page_shift = ilog2(page_size);
+		pages = windows[i].size >> page_shift;
+		bitmap_size = BITS_TO_LONGS(pages) * sizeof(long);
+
+		mem = kzalloc(sizeof(*mem), GFP_KERNEL);
+		if (!mem) {
+			ret = -ENOMEM;
+			i -= 1;
+			goto err_mem;
+		}
+
+		bitmap = kzalloc(bitmap_size, GFP_KERNEL);
+		if (!bitmap) {
+			ret = -ENOMEM;
+			kfree(mem);
+			i -= 1;
+			goto err_mem;
+		}
 
-	mem->bitmap = bitmap;
-	mem->phys_base = phys_base;
-	mem->page_size = page_size;
-	mem->pages = pages;
-	mem->size = size;
-	mutex_init(&mem->lock);
+		mem->window.phys_base = windows[i].phys_base;
+		mem->window.size = windows[i].size;
+		mem->window.page_size = page_size;
+		mem->bitmap = bitmap;
+		mem->pages = pages;
+		mutex_init(&mem->lock);
+		epc->windows[i] = mem;
+	}
 
-	epc->mem = mem;
+	epc->mem = epc->windows[0];
+	epc->num_windows = num_windows;
 
 	return 0;
 
 err_mem:
-	kfree(mem);
+	for (; i >= 0; i--) {
+		mem = epc->windows[i];
+		kfree(mem->bitmap);
+		kfree(mem);
+	}
+	kfree(epc->windows);
 
-err:
-return ret;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(__pci_epc_mem_init);
 
+int pci_epc_mem_init(struct pci_epc *epc, phys_addr_t base,
+		     size_t size, size_t page_size)
+{
+	struct pci_epc_mem_window mem_window;
+
+	mem_window.phys_base = base;
+	mem_window.size = size;
+	mem_window.page_size = page_size;
+
+	return __pci_epc_mem_init(epc, &mem_window, 1);
+}
+EXPORT_SYMBOL_GPL(pci_epc_mem_init);
+
 /**
  * pci_epc_mem_exit() - cleanup the pci_epc_mem structure
  * @epc: the EPC device that invoked pci_epc_mem_exit
@@ -102,11 +136,22 @@  EXPORT_SYMBOL_GPL(__pci_epc_mem_init);
  */
 void pci_epc_mem_exit(struct pci_epc *epc)
 {
-	struct pci_epc_mem *mem = epc->mem;
+	struct pci_epc_mem *mem;
+	int i;
+
+	if (!epc->num_windows)
+		return;
+
+	for (i = 0; i <= epc->num_windows; i++) {
+		mem = epc->windows[i];
+		kfree(mem->bitmap);
+		kfree(mem);
+	}
+	kfree(epc->windows);
 
+	epc->windows = NULL;
 	epc->mem = NULL;
-	kfree(mem->bitmap);
-	kfree(mem);
+	epc->num_windows = 0;
 }
 EXPORT_SYMBOL_GPL(pci_epc_mem_exit);
 
@@ -122,31 +167,56 @@  EXPORT_SYMBOL_GPL(pci_epc_mem_exit);
 void __iomem *pci_epc_mem_alloc_addr(struct pci_epc *epc,
 				     phys_addr_t *phys_addr, size_t size)
 {
-	int pageno;
 	void __iomem *virt_addr = NULL;
-	struct pci_epc_mem *mem = epc->mem;
-	unsigned int page_shift = ilog2(mem->page_size);
+	struct pci_epc_mem *mem;
+	unsigned int page_shift;
+	int pageno = -EINVAL;
 	int order;
+	int i;
 
-	size = ALIGN(size, mem->page_size);
-	order = pci_epc_mem_get_order(mem, size);
-
-	mutex_lock(&mem->lock);
-	pageno = bitmap_find_free_region(mem->bitmap, mem->pages, order);
-	if (pageno < 0)
-		goto ret;
+	for (i = 0; i < epc->num_windows; i++) {
+		mem = epc->windows[i];
+		mutex_lock(&mem->lock);
+		size = ALIGN(size, mem->window.page_size);
+		order = pci_epc_mem_get_order(mem, size);
 
-	*phys_addr = mem->phys_base + ((phys_addr_t)pageno << page_shift);
-	virt_addr = ioremap(*phys_addr, size);
-	if (!virt_addr)
-		bitmap_release_region(mem->bitmap, pageno, order);
+		pageno = bitmap_find_free_region(mem->bitmap, mem->pages,
+						 order);
+		if (pageno >= 0) {
+			page_shift = ilog2(mem->window.page_size);
+			*phys_addr = mem->window.phys_base +
+				((phys_addr_t)pageno << page_shift);
+			virt_addr = ioremap(*phys_addr, size);
+			if (!virt_addr)
+				bitmap_release_region(mem->bitmap,
+						      pageno, order);
+			mutex_unlock(&mem->lock);
+			return virt_addr;
+		}
+		mutex_unlock(&mem->lock);
+	}
 
-ret:
-	mutex_unlock(&mem->lock);
 	return virt_addr;
 }
 EXPORT_SYMBOL_GPL(pci_epc_mem_alloc_addr);
 
+struct pci_epc_mem *pci_epc_get_matching_window(struct pci_epc *epc,
+						phys_addr_t phys_addr)
+{
+	struct pci_epc_mem *mem;
+	int i;
+
+	for (i = 0; i < epc->num_windows; i++) {
+		mem = epc->windows[i];
+
+		if (phys_addr >= mem->window.phys_base &&
+		    phys_addr < (mem->window.phys_base + mem->window.size))
+			return mem;
+	}
+
+	return NULL;
+}
+
 /**
  * pci_epc_mem_free_addr() - free the allocated memory address
  * @epc: the EPC device on which memory was allocated
@@ -159,14 +229,23 @@  EXPORT_SYMBOL_GPL(pci_epc_mem_alloc_addr);
 void pci_epc_mem_free_addr(struct pci_epc *epc, phys_addr_t phys_addr,
 			   void __iomem *virt_addr, size_t size)
 {
+	struct pci_epc_mem *mem;
+	unsigned int page_shift;
+	size_t page_size;
 	int pageno;
-	struct pci_epc_mem *mem = epc->mem;
-	unsigned int page_shift = ilog2(mem->page_size);
 	int order;
 
+	mem = pci_epc_get_matching_window(epc, phys_addr);
+	if (!mem) {
+		pr_err("failed to get matching window\n");
+		return;
+	}
+
+	page_size = mem->window.page_size;
+	page_shift = ilog2(page_size);
 	iounmap(virt_addr);
-	pageno = (phys_addr - mem->phys_base) >> page_shift;
-	size = ALIGN(size, mem->page_size);
+	pageno = (phys_addr - mem->window.phys_base) >> page_shift;
+	size = ALIGN(size, page_size);
 	order = pci_epc_mem_get_order(mem, size);
 	mutex_lock(&mem->lock);
 	bitmap_release_region(mem->bitmap, pageno, order);
diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
index e0ed9d01f6e5..d5da11cf0f2a 100644
--- a/include/linux/pci-epc.h
+++ b/include/linux/pci-epc.h
@@ -65,20 +65,28 @@  struct pci_epc_ops {
 	struct module *owner;
 };
 
+/**
+ * struct pci_epc_mem_window - address window of the endpoint controller
+ * @phys_base: physical base address of the PCI address window
+ * @size: the size of the PCI address window
+ * @page_size: size of each page
+ */
+struct pci_epc_mem_window {
+	phys_addr_t	phys_base;
+	size_t		size;
+	size_t		page_size;
+};
+
 /**
  * struct pci_epc_mem - address space of the endpoint controller
- * @phys_base: physical base address of the PCI address space
- * @size: the size of the PCI address space
+ * @window: address window of the endpoint controller
  * @bitmap: bitmap to manage the PCI address space
- * @pages: number of bits representing the address region
- * @page_size: size of each page
  * @lock: mutex to protect bitmap
+ * @pages: number of bits representing the address region
  */
 struct pci_epc_mem {
-	phys_addr_t	phys_base;
-	size_t		size;
+	struct pci_epc_mem_window window;
 	unsigned long	*bitmap;
-	size_t		page_size;
 	int		pages;
 	/* mutex to protect against concurrent access for memory allocation*/
 	struct mutex	lock;
@@ -89,7 +97,11 @@  struct pci_epc_mem {
  * @dev: PCI EPC device
  * @pci_epf: list of endpoint functions present in this EPC device
  * @ops: function pointers for performing endpoint operations
- * @mem: address space of the endpoint controller
+ * @windows: array of address space of the endpoint controller
+ * @mem: first window of the endpoint controller, which corresponds to
+ *       default address space of the endpoint controller supporting
+ *       single window.
+ * @num_windows: number of windows supported by device
  * @max_functions: max number of functions that can be configured in this EPC
  * @group: configfs group representing the PCI EPC device
  * @lock: mutex to protect pci_epc ops
@@ -100,7 +112,9 @@  struct pci_epc {
 	struct device			dev;
 	struct list_head		pci_epf;
 	const struct pci_epc_ops	*ops;
+	struct pci_epc_mem		**windows;
 	struct pci_epc_mem		*mem;
+	unsigned int			num_windows;
 	u8				max_functions;
 	struct config_group		*group;
 	/* mutex to protect against concurrent access of EP controller */
@@ -137,9 +151,6 @@  struct pci_epc_features {
 #define devm_pci_epc_create(dev, ops)    \
 		__devm_pci_epc_create((dev), (ops), THIS_MODULE)
 
-#define pci_epc_mem_init(epc, phys_addr, size)	\
-		__pci_epc_mem_init((epc), (phys_addr), (size), PAGE_SIZE)
-
 static inline void epc_set_drvdata(struct pci_epc *epc, void *data)
 {
 	dev_set_drvdata(&epc->dev, data);
@@ -195,8 +206,10 @@  unsigned int pci_epc_get_first_free_bar(const struct pci_epc_features
 struct pci_epc *pci_epc_get(const char *epc_name);
 void pci_epc_put(struct pci_epc *epc);
 
-int __pci_epc_mem_init(struct pci_epc *epc, phys_addr_t phys_addr, size_t size,
-		       size_t page_size);
+int __pci_epc_mem_init(struct pci_epc *epc, struct pci_epc_mem_window *window,
+		       unsigned int num_windows);
+int pci_epc_mem_init(struct pci_epc *epc, phys_addr_t base,
+		     size_t size, size_t page_size);
 void pci_epc_mem_exit(struct pci_epc *epc);
 void __iomem *pci_epc_mem_alloc_addr(struct pci_epc *epc,
 				     phys_addr_t *phys_addr, size_t size);