mbox series

[v9,0/8] Add endpoint driver for R-Car PCIe controller

Message ID 1587666159-6035-1-git-send-email-prabhakar.mahadev-lad.rj@bp.renesas.com
Headers show
Series Add endpoint driver for R-Car PCIe controller | expand

Message

Lad Prabhakar April 23, 2020, 6:22 p.m. UTC
Hi All,

This patch series adds support for endpoint driver for R-Car PCIe controller on
R-Car/RZ-G2x SoC's, this also extends the epf framework to handle multiple windows
supported by the controller for mapping PCI address locally.

Note:
The cadence/rockchip/designware endpoint drivers are build tested only.

Changes for v9 (Re-spun this series as there were minimal changes requested):
* Rebased patches on top of v5.7.rc1
* Replaced mdelay(1) with usleep_range(1000, 1001) in rcar_pcie_ep_assert_intx()
* Added a check for max_functions read from DT to restrict with
  RCAR_EPC_MAX_FUNCTIONS
* Replaced MSICAP0_MMENUM with MSICAP0_MMESE
* Retry ioremap for other windows on failure in pci_epc_mem_alloc_addr()
* Fixed looping for number windows in pci_epc_mem_exit()
* Set maximum to 1 for max-functions in DT binding (I have restored the acks
  from  Rob and Shimoda-san)
* Sorted the entry in MAINTAINERS

Changes for v8:
* Dropped adding R8A774C0 (0x002d) pci-id in pci_ids.h
* Fixed typo in commit message for patch 2/8
* Reworded commit message for patch 5/8 as suggested by Bjorn
* Split up patch to add pci_epc_mem_init() interface to add page_size argument
  as suggested by Bjorn.

Changes for v7:
* Fixed review comments pointed by Shimoda-san
  1] Made DT bindings dual licensed, added Shimoda-san as maintainer and fixed
     the example as its built with #{address,size}-cells = <1>. I have still
     restored the Ack from Rob and Shimoda-san with these changes.
  2] Split up the patches so that they can be picked up by respective subsystem
     patches 1/4-9/11 are now part of this series.
  3] Dropped altering a comment in pci-epc.h
  4] Used a local variable align_size in pci_epc_mem_alloc_addr() so that size
     variable doesn't get overwritten in the loop.
  5] Replaced i-=1 with i--
  6] Replaced rcar with R-Car in patch subject and description.
  7] Set MACCTLR in init() callback

Changes for v6:
1] Rebased patches on endpoint branch of https://git.kernel.org/pub/
   scm/linux/kernel/git/lpieralisi/pci.git/
2] Fixed review comments from Shimoda-san
   a] Made sure defconfig changes were in separate patch
   b] Created rcar_pcie_host/rcar_pcie_ep structures
   c] Added pci-id for R8A774C0
   d] Added entry in MAINTAINERS for dt-binding
   e] Dropped unnecessary braces
3] Added support for msi.

Changes for v5:
1] Rebased patches on next branch of https://git.kernel.org/pub/scm/
   linux/kernel/git/helgaas/pci.git
2] Fixed review comments reported by Kishon while fetching the matching
   window in function pci_epc_get_matching_window()
3] Fixed review comments reported by Bjorn
   a] Split patch up first patch so that its easier to review and incremental
   b] Fixed typos
4] Included Reviewed tag from Rob for the dt-binding patch
5] Fixed issue reported by Nathan for assigning variable to itself

Changes for v4:
1] Fixed dtb_check error reported by Rob
2] Fixed review comments reported by Kishon
   a] Dropped pci_epc_find_best_fit_window()
   b] Fixed initializing mem ptr in __pci_epc_mem_init()
   c] Dropped map_size from pci_epc_mem_window structure

Changes for v3:
1] Fixed review comments from Bjorn and Kishon.
3] Converted to DT schema

Changes for v2:
1] Fixed review comments from Biju for dt-bindings to include an example
   for a tested platform.
2] Fixed review comments from Kishon to extend the features of outbound
   regions in epf framework.
3] Added support to parse outbound-ranges in OF.

Lad Prabhakar (8):
  PCI: rcar: Rename pcie-rcar.c to pcie-rcar-host.c
  PCI: rcar: Move shareable code to a common file
  PCI: rcar: Fix calculating mask for PCIEPAMR register
  PCI: endpoint: Pass page size as argument to pci_epc_mem_init()
  PCI: endpoint: Add support to handle multiple base for mapping
    outbound memory
  dt-bindings: PCI: rcar: Add bindings for R-Car PCIe endpoint
    controller
  PCI: rcar: Add endpoint mode support
  MAINTAINERS: Add file patterns for rcar PCI device tree bindings

 .../devicetree/bindings/pci/rcar-pci-ep.yaml  |   77 ++
 MAINTAINERS                                   |    1 +
 drivers/pci/controller/Kconfig                |   18 +
 drivers/pci/controller/Makefile               |    3 +-
 .../pci/controller/cadence/pcie-cadence-ep.c  |    2 +-
 .../pci/controller/dwc/pcie-designware-ep.c   |   16 +-
 drivers/pci/controller/pcie-rcar-ep.c         |  557 ++++++++
 drivers/pci/controller/pcie-rcar-host.c       | 1065 +++++++++++++++
 drivers/pci/controller/pcie-rcar.c            | 1206 +----------------
 drivers/pci/controller/pcie-rcar.h            |  140 ++
 drivers/pci/controller/pcie-rockchip-ep.c     |    2 +-
 drivers/pci/endpoint/pci-epc-mem.c            |  204 ++-
 include/linux/pci-epc.h                       |   38 +-
 13 files changed, 2078 insertions(+), 1251 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/rcar-pci-ep.yaml
 create mode 100644 drivers/pci/controller/pcie-rcar-ep.c
 create mode 100644 drivers/pci/controller/pcie-rcar-host.c
 create mode 100644 drivers/pci/controller/pcie-rcar.h

Comments

Yoshihiro Shimoda April 24, 2020, 5:59 a.m. UTC | #1
Hi Prabhakar-san,

> From: Lad Prabhakar, Sent: Friday, April 24, 2020 3:23 AM
> 
> pci_epc_mem_init() internally used page size equal to *PAGE_SIZE* to
> manage the address space so instead just pass the page size as a
> argument to pci_epc_mem_init().
> 
> Also make pci_epc_mem_init() as a C function instead of a macro function
> in preparation for adding support for pci-epc-mem core to handle multiple
> windows.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Thank you for the patch!

Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Best regards,
Yoshihiro Shimoda
Yoshihiro Shimoda April 24, 2020, 6 a.m. UTC | #2
Hi Prabhakar-san,

> From: Lad Prabhakar, Sent: Friday, April 24, 2020 3:23 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 pci_epc_multi_mem_init()
> initializes multiple windows supported by endpoint controller.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Thank you for the patch!

Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Best regards,
Yoshihiro Shimoda
Yoshihiro Shimoda April 24, 2020, 6:01 a.m. UTC | #3
Hi Prabhakar-san,

> From: Lad Prabhakar, Sent: Friday, April 24, 2020 3:23 AM
> 
> Add support for R-Car PCIe controller to work in endpoint mode.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Thank you for the patch!

Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Best regards,
Yoshihiro Shimoda
Kishon Vijay Abraham I April 24, 2020, 6:13 a.m. UTC | #4
Hi Prabhakar,

On 4/23/2020 11:52 PM, Lad Prabhakar wrote:
> 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 pci_epc_multi_mem_init()
> initializes multiple windows supported by endpoint controller.

Have a couple of clean-up comments. See below.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  .../pci/controller/dwc/pcie-designware-ep.c   |  16 +-
>  drivers/pci/endpoint/pci-epc-mem.c            | 199 ++++++++++++------
>  include/linux/pci-epc.h                       |  33 ++-
>  3 files changed, 170 insertions(+), 78 deletions(-)
> 
.
.
<snip>
.
.
> diff --git a/drivers/pci/endpoint/pci-epc-mem.c b/drivers/pci/endpoint/pci-epc-mem.c
> index cdd1d3821249..a3466da2a16f 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;
> @@ -36,67 +36,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
> + * pci_epc_multi_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_multi_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->bitmap = bitmap;
> -	mem->phys_base = phys_base;
> -	mem->page_size = page_size;
> -	mem->pages = pages;
> -	mem->size = size;
> -	mutex_init(&mem->lock);
> +		mem = kzalloc(sizeof(*mem), GFP_KERNEL);
> +		if (!mem) {
> +			ret = -ENOMEM;
> +			i--;
> +			goto err_mem;
> +		}
>  
> -	epc->mem = mem;
> +		bitmap = kzalloc(bitmap_size, GFP_KERNEL);
> +		if (!bitmap) {
> +			ret = -ENOMEM;
> +			kfree(mem);
> +			i--;
> +			goto err_mem;
> +		}
> +
> +		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 = epc->windows[0];

"mem" member of EPC looks unnecessary since that value is available at
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);
> +EXPORT_SYMBOL_GPL(pci_epc_multi_mem_init);
>  
>  int pci_epc_mem_init(struct pci_epc *epc, phys_addr_t base,
>  		     size_t size, size_t page_size)
>  {
> -	return __pci_epc_mem_init(epc, base, size, 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_multi_mem_init(epc, &mem_window, 1);
>  }
>  EXPORT_SYMBOL_GPL(pci_epc_mem_init);
>  
> @@ -109,11 +137,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);
>  
> @@ -129,31 +168,60 @@ 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;
> +	size_t align_size;
> +	int pageno;
>  	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);
> +		align_size = ALIGN(size, mem->window.page_size);
> +		order = pci_epc_mem_get_order(mem, align_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, align_size);
> +			if (!virt_addr) {
> +				bitmap_release_region(mem->bitmap,
> +						      pageno, order);
> +				mutex_unlock(&mem->lock);
> +				continue;
> +			}
> +			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
> @@ -166,14 +234,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 5bc1de65849e..cc66bec8be90 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
>   */
>  struct pci_epc_mem {
> -	phys_addr_t	phys_base;
> -	size_t		size;
> +	struct pci_epc_mem_window window;

Don't see any additional value in moving phys_base, size, page_size to a new
structure and again including it here.

Thanks
Kishon
Prabhakar April 24, 2020, 7:46 a.m. UTC | #5
Hi Kishon,

Thank you for the review.

On Fri, Apr 24, 2020 at 7:13 AM Kishon Vijay Abraham I <kishon@ti.com> wrote:
>
> Hi Prabhakar,
>
> On 4/23/2020 11:52 PM, Lad Prabhakar wrote:
> > 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 pci_epc_multi_mem_init()
> > initializes multiple windows supported by endpoint controller.
>
> Have a couple of clean-up comments. See below.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  .../pci/controller/dwc/pcie-designware-ep.c   |  16 +-
> >  drivers/pci/endpoint/pci-epc-mem.c            | 199 ++++++++++++------
> >  include/linux/pci-epc.h                       |  33 ++-
> >  3 files changed, 170 insertions(+), 78 deletions(-)
> >
> .
> .
> <snip>
> .
> .
> > diff --git a/drivers/pci/endpoint/pci-epc-mem.c b/drivers/pci/endpoint/pci-epc-mem.c
> > index cdd1d3821249..a3466da2a16f 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;
> > @@ -36,67 +36,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
> > + * pci_epc_multi_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_multi_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->bitmap = bitmap;
> > -     mem->phys_base = phys_base;
> > -     mem->page_size = page_size;
> > -     mem->pages = pages;
> > -     mem->size = size;
> > -     mutex_init(&mem->lock);
> > +             mem = kzalloc(sizeof(*mem), GFP_KERNEL);
> > +             if (!mem) {
> > +                     ret = -ENOMEM;
> > +                     i--;
> > +                     goto err_mem;
> > +             }
> >
> > -     epc->mem = mem;
> > +             bitmap = kzalloc(bitmap_size, GFP_KERNEL);
> > +             if (!bitmap) {
> > +                     ret = -ENOMEM;
> > +                     kfree(mem);
> > +                     i--;
> > +                     goto err_mem;
> > +             }
> > +
> > +             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 = epc->windows[0];
>
> "mem" member of EPC looks unnecessary since that value is available at
> epc->windows[0].
This was suggested by Shimoda-san, as most of the current  controller
drivers support single region this pointer would be easier to access
the region instead of adding #define EPC_DEFAULT_WINDOW  0 and
accessing  as epc->windows[EPC_DEFAULT_WINDOW];

> > +     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);
> > +EXPORT_SYMBOL_GPL(pci_epc_multi_mem_init);
> >
> >  int pci_epc_mem_init(struct pci_epc *epc, phys_addr_t base,
> >                    size_t size, size_t page_size)
> >  {
> > -     return __pci_epc_mem_init(epc, base, size, 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_multi_mem_init(epc, &mem_window, 1);
> >  }
> >  EXPORT_SYMBOL_GPL(pci_epc_mem_init);
> >
> > @@ -109,11 +137,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);
> >
> > @@ -129,31 +168,60 @@ 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;
> > +     size_t align_size;
> > +     int pageno;
> >       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);
> > +             align_size = ALIGN(size, mem->window.page_size);
> > +             order = pci_epc_mem_get_order(mem, align_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, align_size);
> > +                     if (!virt_addr) {
> > +                             bitmap_release_region(mem->bitmap,
> > +                                                   pageno, order);
> > +                             mutex_unlock(&mem->lock);
> > +                             continue;
> > +                     }
> > +                     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
> > @@ -166,14 +234,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 5bc1de65849e..cc66bec8be90 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
> >   */
> >  struct pci_epc_mem {
> > -     phys_addr_t     phys_base;
> > -     size_t          size;
> > +     struct pci_epc_mem_window window;
>
> Don't see any additional value in moving phys_base, size, page_size to a new
> structure and again including it here.
>
Controllers supporting multiple windows create a list of supported
regions (struct pci_epc_mem_window ) and pass a pointer to
pci_epc_multi_mem_init(), hence this split.

Cheers,
--Prabhakar
Kishon Vijay Abraham I April 24, 2020, 8 a.m. UTC | #6
Hi Prabhakar,

On 4/24/2020 1:16 PM, Lad, Prabhakar wrote:
> Hi Kishon,
> 
> Thank you for the review.
> 
> On Fri, Apr 24, 2020 at 7:13 AM Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>
>> Hi Prabhakar,
>>
>> On 4/23/2020 11:52 PM, Lad Prabhakar wrote:
>>> 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 pci_epc_multi_mem_init()
>>> initializes multiple windows supported by endpoint controller.
>>
>> Have a couple of clean-up comments. See below.
>>>
>>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>>> ---
>>>  .../pci/controller/dwc/pcie-designware-ep.c   |  16 +-
>>>  drivers/pci/endpoint/pci-epc-mem.c            | 199 ++++++++++++------
>>>  include/linux/pci-epc.h                       |  33 ++-
>>>  3 files changed, 170 insertions(+), 78 deletions(-)
>>>
>> .
>> .
>> <snip>
>> .
>> .
>>> diff --git a/drivers/pci/endpoint/pci-epc-mem.c b/drivers/pci/endpoint/pci-epc-mem.c
>>> index cdd1d3821249..a3466da2a16f 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;
>>> @@ -36,67 +36,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
>>> + * pci_epc_multi_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_multi_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->bitmap = bitmap;
>>> -     mem->phys_base = phys_base;
>>> -     mem->page_size = page_size;
>>> -     mem->pages = pages;
>>> -     mem->size = size;
>>> -     mutex_init(&mem->lock);
>>> +             mem = kzalloc(sizeof(*mem), GFP_KERNEL);
>>> +             if (!mem) {
>>> +                     ret = -ENOMEM;
>>> +                     i--;
>>> +                     goto err_mem;
>>> +             }
>>>
>>> -     epc->mem = mem;
>>> +             bitmap = kzalloc(bitmap_size, GFP_KERNEL);
>>> +             if (!bitmap) {
>>> +                     ret = -ENOMEM;
>>> +                     kfree(mem);
>>> +                     i--;
>>> +                     goto err_mem;
>>> +             }
>>> +
>>> +             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 = epc->windows[0];
>>
>> "mem" member of EPC looks unnecessary since that value is available at
>> epc->windows[0].
> This was suggested by Shimoda-san, as most of the current  controller
> drivers support single region this pointer would be easier to access
> the region instead of adding #define EPC_DEFAULT_WINDOW  0 and
> accessing  as epc->windows[EPC_DEFAULT_WINDOW];
> 
>>> +     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);
>>> +EXPORT_SYMBOL_GPL(pci_epc_multi_mem_init);
>>>
>>>  int pci_epc_mem_init(struct pci_epc *epc, phys_addr_t base,
>>>                    size_t size, size_t page_size)
>>>  {
>>> -     return __pci_epc_mem_init(epc, base, size, 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_multi_mem_init(epc, &mem_window, 1);
>>>  }
>>>  EXPORT_SYMBOL_GPL(pci_epc_mem_init);
>>>
>>> @@ -109,11 +137,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);
>>>
>>> @@ -129,31 +168,60 @@ 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;
>>> +     size_t align_size;
>>> +     int pageno;
>>>       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);
>>> +             align_size = ALIGN(size, mem->window.page_size);
>>> +             order = pci_epc_mem_get_order(mem, align_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, align_size);
>>> +                     if (!virt_addr) {
>>> +                             bitmap_release_region(mem->bitmap,
>>> +                                                   pageno, order);
>>> +                             mutex_unlock(&mem->lock);
>>> +                             continue;
>>> +                     }
>>> +                     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
>>> @@ -166,14 +234,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 5bc1de65849e..cc66bec8be90 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
>>>   */
>>>  struct pci_epc_mem {
>>> -     phys_addr_t     phys_base;
>>> -     size_t          size;
>>> +     struct pci_epc_mem_window window;
>>
>> Don't see any additional value in moving phys_base, size, page_size to a new
>> structure and again including it here.
>>
> Controllers supporting multiple windows create a list of supported
> regions (struct pci_epc_mem_window ) and pass a pointer to
> pci_epc_multi_mem_init(), hence this split.

Okay, thanks for clarifying.

Regards
Kishon
Prabhakar April 30, 2020, 8:43 a.m. UTC | #7
Hi Kishon,

On Thu, Apr 23, 2020 at 7:23 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
>
> Hi All,
>
> This patch series adds support for endpoint driver for R-Car PCIe controller on
> R-Car/RZ-G2x SoC's, this also extends the epf framework to handle multiple windows
> supported by the controller for mapping PCI address locally.
>
> Note:
> The cadence/rockchip/designware endpoint drivers are build tested only.
>
> Changes for v9 (Re-spun this series as there were minimal changes requested):
> * Rebased patches on top of v5.7.rc1
> * Replaced mdelay(1) with usleep_range(1000, 1001) in rcar_pcie_ep_assert_intx()
> * Added a check for max_functions read from DT to restrict with
>   RCAR_EPC_MAX_FUNCTIONS
> * Replaced MSICAP0_MMENUM with MSICAP0_MMESE
> * Retry ioremap for other windows on failure in pci_epc_mem_alloc_addr()
> * Fixed looping for number windows in pci_epc_mem_exit()
> * Set maximum to 1 for max-functions in DT binding (I have restored the acks
>   from  Rob and Shimoda-san)
> * Sorted the entry in MAINTAINERS
>
> Changes for v8:
> * Dropped adding R8A774C0 (0x002d) pci-id in pci_ids.h
> * Fixed typo in commit message for patch 2/8
> * Reworded commit message for patch 5/8 as suggested by Bjorn
> * Split up patch to add pci_epc_mem_init() interface to add page_size argument
>   as suggested by Bjorn.
>
> Changes for v7:
> * Fixed review comments pointed by Shimoda-san
>   1] Made DT bindings dual licensed, added Shimoda-san as maintainer and fixed
>      the example as its built with #{address,size}-cells = <1>. I have still
>      restored the Ack from Rob and Shimoda-san with these changes.
>   2] Split up the patches so that they can be picked up by respective subsystem
>      patches 1/4-9/11 are now part of this series.
>   3] Dropped altering a comment in pci-epc.h
>   4] Used a local variable align_size in pci_epc_mem_alloc_addr() so that size
>      variable doesn't get overwritten in the loop.
>   5] Replaced i-=1 with i--
>   6] Replaced rcar with R-Car in patch subject and description.
>   7] Set MACCTLR in init() callback
>
> Changes for v6:
> 1] Rebased patches on endpoint branch of https://git.kernel.org/pub/
>    scm/linux/kernel/git/lpieralisi/pci.git/
> 2] Fixed review comments from Shimoda-san
>    a] Made sure defconfig changes were in separate patch
>    b] Created rcar_pcie_host/rcar_pcie_ep structures
>    c] Added pci-id for R8A774C0
>    d] Added entry in MAINTAINERS for dt-binding
>    e] Dropped unnecessary braces
> 3] Added support for msi.
>
> Changes for v5:
> 1] Rebased patches on next branch of https://git.kernel.org/pub/scm/
>    linux/kernel/git/helgaas/pci.git
> 2] Fixed review comments reported by Kishon while fetching the matching
>    window in function pci_epc_get_matching_window()
> 3] Fixed review comments reported by Bjorn
>    a] Split patch up first patch so that its easier to review and incremental
>    b] Fixed typos
> 4] Included Reviewed tag from Rob for the dt-binding patch
> 5] Fixed issue reported by Nathan for assigning variable to itself
>
> Changes for v4:
> 1] Fixed dtb_check error reported by Rob
> 2] Fixed review comments reported by Kishon
>    a] Dropped pci_epc_find_best_fit_window()
>    b] Fixed initializing mem ptr in __pci_epc_mem_init()
>    c] Dropped map_size from pci_epc_mem_window structure
>
> Changes for v3:
> 1] Fixed review comments from Bjorn and Kishon.
> 3] Converted to DT schema
>
> Changes for v2:
> 1] Fixed review comments from Biju for dt-bindings to include an example
>    for a tested platform.
> 2] Fixed review comments from Kishon to extend the features of outbound
>    regions in epf framework.
> 3] Added support to parse outbound-ranges in OF.
>
> Lad Prabhakar (8):
>   PCI: rcar: Rename pcie-rcar.c to pcie-rcar-host.c
>   PCI: rcar: Move shareable code to a common file
>   PCI: rcar: Fix calculating mask for PCIEPAMR register
>   PCI: endpoint: Pass page size as argument to pci_epc_mem_init()
>   PCI: endpoint: Add support to handle multiple base for mapping
>     outbound memory
Could you please do the needy for the above two patches, so that this
can be picked up by Lorenzo.

Cheers,
--Prabhakar

>   dt-bindings: PCI: rcar: Add bindings for R-Car PCIe endpoint
>     controller
>   PCI: rcar: Add endpoint mode support
>   MAINTAINERS: Add file patterns for rcar PCI device tree bindings
>
>  .../devicetree/bindings/pci/rcar-pci-ep.yaml  |   77 ++
>  MAINTAINERS                                   |    1 +
>  drivers/pci/controller/Kconfig                |   18 +
>  drivers/pci/controller/Makefile               |    3 +-
>  .../pci/controller/cadence/pcie-cadence-ep.c  |    2 +-
>  .../pci/controller/dwc/pcie-designware-ep.c   |   16 +-
>  drivers/pci/controller/pcie-rcar-ep.c         |  557 ++++++++
>  drivers/pci/controller/pcie-rcar-host.c       | 1065 +++++++++++++++
>  drivers/pci/controller/pcie-rcar.c            | 1206 +----------------
>  drivers/pci/controller/pcie-rcar.h            |  140 ++
>  drivers/pci/controller/pcie-rockchip-ep.c     |    2 +-
>  drivers/pci/endpoint/pci-epc-mem.c            |  204 ++-
>  include/linux/pci-epc.h                       |   38 +-
>  13 files changed, 2078 insertions(+), 1251 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/pci/rcar-pci-ep.yaml
>  create mode 100644 drivers/pci/controller/pcie-rcar-ep.c
>  create mode 100644 drivers/pci/controller/pcie-rcar-host.c
>  create mode 100644 drivers/pci/controller/pcie-rcar.h
>
> --
> 2.17.1
>
Lorenzo Pieralisi May 5, 2020, 9:44 a.m. UTC | #8
On Thu, Apr 30, 2020 at 09:43:20AM +0100, Lad, Prabhakar wrote:
> Hi Kishon,
> 
> On Thu, Apr 23, 2020 at 7:23 PM Lad Prabhakar
> <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> >
> > Hi All,
> >
> > This patch series adds support for endpoint driver for R-Car PCIe controller on
> > R-Car/RZ-G2x SoC's, this also extends the epf framework to handle multiple windows
> > supported by the controller for mapping PCI address locally.
> >
> > Note:
> > The cadence/rockchip/designware endpoint drivers are build tested only.
> >
> > Changes for v9 (Re-spun this series as there were minimal changes requested):
> > * Rebased patches on top of v5.7.rc1
> > * Replaced mdelay(1) with usleep_range(1000, 1001) in rcar_pcie_ep_assert_intx()
> > * Added a check for max_functions read from DT to restrict with
> >   RCAR_EPC_MAX_FUNCTIONS
> > * Replaced MSICAP0_MMENUM with MSICAP0_MMESE
> > * Retry ioremap for other windows on failure in pci_epc_mem_alloc_addr()
> > * Fixed looping for number windows in pci_epc_mem_exit()
> > * Set maximum to 1 for max-functions in DT binding (I have restored the acks
> >   from  Rob and Shimoda-san)
> > * Sorted the entry in MAINTAINERS
> >
> > Changes for v8:
> > * Dropped adding R8A774C0 (0x002d) pci-id in pci_ids.h
> > * Fixed typo in commit message for patch 2/8
> > * Reworded commit message for patch 5/8 as suggested by Bjorn
> > * Split up patch to add pci_epc_mem_init() interface to add page_size argument
> >   as suggested by Bjorn.
> >
> > Changes for v7:
> > * Fixed review comments pointed by Shimoda-san
> >   1] Made DT bindings dual licensed, added Shimoda-san as maintainer and fixed
> >      the example as its built with #{address,size}-cells = <1>. I have still
> >      restored the Ack from Rob and Shimoda-san with these changes.
> >   2] Split up the patches so that they can be picked up by respective subsystem
> >      patches 1/4-9/11 are now part of this series.
> >   3] Dropped altering a comment in pci-epc.h
> >   4] Used a local variable align_size in pci_epc_mem_alloc_addr() so that size
> >      variable doesn't get overwritten in the loop.
> >   5] Replaced i-=1 with i--
> >   6] Replaced rcar with R-Car in patch subject and description.
> >   7] Set MACCTLR in init() callback
> >
> > Changes for v6:
> > 1] Rebased patches on endpoint branch of https://git.kernel.org/pub/
> >    scm/linux/kernel/git/lpieralisi/pci.git/
> > 2] Fixed review comments from Shimoda-san
> >    a] Made sure defconfig changes were in separate patch
> >    b] Created rcar_pcie_host/rcar_pcie_ep structures
> >    c] Added pci-id for R8A774C0
> >    d] Added entry in MAINTAINERS for dt-binding
> >    e] Dropped unnecessary braces
> > 3] Added support for msi.
> >
> > Changes for v5:
> > 1] Rebased patches on next branch of https://git.kernel.org/pub/scm/
> >    linux/kernel/git/helgaas/pci.git
> > 2] Fixed review comments reported by Kishon while fetching the matching
> >    window in function pci_epc_get_matching_window()
> > 3] Fixed review comments reported by Bjorn
> >    a] Split patch up first patch so that its easier to review and incremental
> >    b] Fixed typos
> > 4] Included Reviewed tag from Rob for the dt-binding patch
> > 5] Fixed issue reported by Nathan for assigning variable to itself
> >
> > Changes for v4:
> > 1] Fixed dtb_check error reported by Rob
> > 2] Fixed review comments reported by Kishon
> >    a] Dropped pci_epc_find_best_fit_window()
> >    b] Fixed initializing mem ptr in __pci_epc_mem_init()
> >    c] Dropped map_size from pci_epc_mem_window structure
> >
> > Changes for v3:
> > 1] Fixed review comments from Bjorn and Kishon.
> > 3] Converted to DT schema
> >
> > Changes for v2:
> > 1] Fixed review comments from Biju for dt-bindings to include an example
> >    for a tested platform.
> > 2] Fixed review comments from Kishon to extend the features of outbound
> >    regions in epf framework.
> > 3] Added support to parse outbound-ranges in OF.
> >
> > Lad Prabhakar (8):
> >   PCI: rcar: Rename pcie-rcar.c to pcie-rcar-host.c
> >   PCI: rcar: Move shareable code to a common file
> >   PCI: rcar: Fix calculating mask for PCIEPAMR register
> >   PCI: endpoint: Pass page size as argument to pci_epc_mem_init()
> >   PCI: endpoint: Add support to handle multiple base for mapping
> >     outbound memory
> Could you please do the needy for the above two patches, so that this
> can be picked up by Lorenzo.

Yes please. I would kindly ask you to rebase it on top of my
pci/rcar branch - with Kishon's ACK when provided.

Thanks,
Lorenzo

> Cheers,
> --Prabhakar
> 
> >   dt-bindings: PCI: rcar: Add bindings for R-Car PCIe endpoint
> >     controller
> >   PCI: rcar: Add endpoint mode support
> >   MAINTAINERS: Add file patterns for rcar PCI device tree bindings
> >
> >  .../devicetree/bindings/pci/rcar-pci-ep.yaml  |   77 ++
> >  MAINTAINERS                                   |    1 +
> >  drivers/pci/controller/Kconfig                |   18 +
> >  drivers/pci/controller/Makefile               |    3 +-
> >  .../pci/controller/cadence/pcie-cadence-ep.c  |    2 +-
> >  .../pci/controller/dwc/pcie-designware-ep.c   |   16 +-
> >  drivers/pci/controller/pcie-rcar-ep.c         |  557 ++++++++
> >  drivers/pci/controller/pcie-rcar-host.c       | 1065 +++++++++++++++
> >  drivers/pci/controller/pcie-rcar.c            | 1206 +----------------
> >  drivers/pci/controller/pcie-rcar.h            |  140 ++
> >  drivers/pci/controller/pcie-rockchip-ep.c     |    2 +-
> >  drivers/pci/endpoint/pci-epc-mem.c            |  204 ++-
> >  include/linux/pci-epc.h                       |   38 +-
> >  13 files changed, 2078 insertions(+), 1251 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/pci/rcar-pci-ep.yaml
> >  create mode 100644 drivers/pci/controller/pcie-rcar-ep.c
> >  create mode 100644 drivers/pci/controller/pcie-rcar-host.c
> >  create mode 100644 drivers/pci/controller/pcie-rcar.h
> >
> > --
> > 2.17.1
> >
Prabhakar May 5, 2020, 9:47 a.m. UTC | #9
Hi Lorenzo,

On Tue, May 5, 2020 at 10:44 AM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Thu, Apr 30, 2020 at 09:43:20AM +0100, Lad, Prabhakar wrote:
> > Hi Kishon,
> >
> > On Thu, Apr 23, 2020 at 7:23 PM Lad Prabhakar
> > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > >
> > > Hi All,
> > >
> > > This patch series adds support for endpoint driver for R-Car PCIe controller on
> > > R-Car/RZ-G2x SoC's, this also extends the epf framework to handle multiple windows
> > > supported by the controller for mapping PCI address locally.
> > >
> > > Note:
> > > The cadence/rockchip/designware endpoint drivers are build tested only.
> > >
> > > Changes for v9 (Re-spun this series as there were minimal changes requested):
> > > * Rebased patches on top of v5.7.rc1
> > > * Replaced mdelay(1) with usleep_range(1000, 1001) in rcar_pcie_ep_assert_intx()
> > > * Added a check for max_functions read from DT to restrict with
> > >   RCAR_EPC_MAX_FUNCTIONS
> > > * Replaced MSICAP0_MMENUM with MSICAP0_MMESE
> > > * Retry ioremap for other windows on failure in pci_epc_mem_alloc_addr()
> > > * Fixed looping for number windows in pci_epc_mem_exit()
> > > * Set maximum to 1 for max-functions in DT binding (I have restored the acks
> > >   from  Rob and Shimoda-san)
> > > * Sorted the entry in MAINTAINERS
> > >
> > > Changes for v8:
> > > * Dropped adding R8A774C0 (0x002d) pci-id in pci_ids.h
> > > * Fixed typo in commit message for patch 2/8
> > > * Reworded commit message for patch 5/8 as suggested by Bjorn
> > > * Split up patch to add pci_epc_mem_init() interface to add page_size argument
> > >   as suggested by Bjorn.
> > >
> > > Changes for v7:
> > > * Fixed review comments pointed by Shimoda-san
> > >   1] Made DT bindings dual licensed, added Shimoda-san as maintainer and fixed
> > >      the example as its built with #{address,size}-cells = <1>. I have still
> > >      restored the Ack from Rob and Shimoda-san with these changes.
> > >   2] Split up the patches so that they can be picked up by respective subsystem
> > >      patches 1/4-9/11 are now part of this series.
> > >   3] Dropped altering a comment in pci-epc.h
> > >   4] Used a local variable align_size in pci_epc_mem_alloc_addr() so that size
> > >      variable doesn't get overwritten in the loop.
> > >   5] Replaced i-=1 with i--
> > >   6] Replaced rcar with R-Car in patch subject and description.
> > >   7] Set MACCTLR in init() callback
> > >
> > > Changes for v6:
> > > 1] Rebased patches on endpoint branch of https://git.kernel.org/pub/
> > >    scm/linux/kernel/git/lpieralisi/pci.git/
> > > 2] Fixed review comments from Shimoda-san
> > >    a] Made sure defconfig changes were in separate patch
> > >    b] Created rcar_pcie_host/rcar_pcie_ep structures
> > >    c] Added pci-id for R8A774C0
> > >    d] Added entry in MAINTAINERS for dt-binding
> > >    e] Dropped unnecessary braces
> > > 3] Added support for msi.
> > >
> > > Changes for v5:
> > > 1] Rebased patches on next branch of https://git.kernel.org/pub/scm/
> > >    linux/kernel/git/helgaas/pci.git
> > > 2] Fixed review comments reported by Kishon while fetching the matching
> > >    window in function pci_epc_get_matching_window()
> > > 3] Fixed review comments reported by Bjorn
> > >    a] Split patch up first patch so that its easier to review and incremental
> > >    b] Fixed typos
> > > 4] Included Reviewed tag from Rob for the dt-binding patch
> > > 5] Fixed issue reported by Nathan for assigning variable to itself
> > >
> > > Changes for v4:
> > > 1] Fixed dtb_check error reported by Rob
> > > 2] Fixed review comments reported by Kishon
> > >    a] Dropped pci_epc_find_best_fit_window()
> > >    b] Fixed initializing mem ptr in __pci_epc_mem_init()
> > >    c] Dropped map_size from pci_epc_mem_window structure
> > >
> > > Changes for v3:
> > > 1] Fixed review comments from Bjorn and Kishon.
> > > 3] Converted to DT schema
> > >
> > > Changes for v2:
> > > 1] Fixed review comments from Biju for dt-bindings to include an example
> > >    for a tested platform.
> > > 2] Fixed review comments from Kishon to extend the features of outbound
> > >    regions in epf framework.
> > > 3] Added support to parse outbound-ranges in OF.
> > >
> > > Lad Prabhakar (8):
> > >   PCI: rcar: Rename pcie-rcar.c to pcie-rcar-host.c
> > >   PCI: rcar: Move shareable code to a common file
> > >   PCI: rcar: Fix calculating mask for PCIEPAMR register
> > >   PCI: endpoint: Pass page size as argument to pci_epc_mem_init()
> > >   PCI: endpoint: Add support to handle multiple base for mapping
> > >     outbound memory
> > Could you please do the needy for the above two patches, so that this
> > can be picked up by Lorenzo.
>
> Yes please. I would kindly ask you to rebase it on top of my
> pci/rcar branch - with Kishon's ACK when provided.
>
Sure will do that as soon as I get Kishon's Ack.

Cheers,
--Prabhakar

> Thanks,
> Lorenzo
>
> > Cheers,
> > --Prabhakar
> >
> > >   dt-bindings: PCI: rcar: Add bindings for R-Car PCIe endpoint
> > >     controller
> > >   PCI: rcar: Add endpoint mode support
> > >   MAINTAINERS: Add file patterns for rcar PCI device tree bindings
> > >
> > >  .../devicetree/bindings/pci/rcar-pci-ep.yaml  |   77 ++
> > >  MAINTAINERS                                   |    1 +
> > >  drivers/pci/controller/Kconfig                |   18 +
> > >  drivers/pci/controller/Makefile               |    3 +-
> > >  .../pci/controller/cadence/pcie-cadence-ep.c  |    2 +-
> > >  .../pci/controller/dwc/pcie-designware-ep.c   |   16 +-
> > >  drivers/pci/controller/pcie-rcar-ep.c         |  557 ++++++++
> > >  drivers/pci/controller/pcie-rcar-host.c       | 1065 +++++++++++++++
> > >  drivers/pci/controller/pcie-rcar.c            | 1206 +----------------
> > >  drivers/pci/controller/pcie-rcar.h            |  140 ++
> > >  drivers/pci/controller/pcie-rockchip-ep.c     |    2 +-
> > >  drivers/pci/endpoint/pci-epc-mem.c            |  204 ++-
> > >  include/linux/pci-epc.h                       |   38 +-
> > >  13 files changed, 2078 insertions(+), 1251 deletions(-)
> > >  create mode 100644 Documentation/devicetree/bindings/pci/rcar-pci-ep.yaml
> > >  create mode 100644 drivers/pci/controller/pcie-rcar-ep.c
> > >  create mode 100644 drivers/pci/controller/pcie-rcar-host.c
> > >  create mode 100644 drivers/pci/controller/pcie-rcar.h
> > >
> > > --
> > > 2.17.1
> > >
Kishon Vijay Abraham I May 7, 2020, 2:52 a.m. UTC | #10
On 4/23/2020 11:52 PM, Lad Prabhakar wrote:
> pci_epc_mem_init() internally used page size equal to *PAGE_SIZE* to
> manage the address space so instead just pass the page size as a
> argument to pci_epc_mem_init().
> 
> Also make pci_epc_mem_init() as a C function instead of a macro function
> in preparation for adding support for pci-epc-mem core to handle multiple
> windows.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/cadence/pcie-cadence-ep.c | 2 +-
>  drivers/pci/controller/pcie-rockchip-ep.c        | 2 +-
>  drivers/pci/endpoint/pci-epc-mem.c               | 7 +++++++
>  include/linux/pci-epc.h                          | 5 ++---
>  4 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> index 1c173dad67d1..1c15c8352125 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> @@ -450,7 +450,7 @@ 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/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..cdd1d3821249 100644
> --- a/drivers/pci/endpoint/pci-epc-mem.c
> +++ b/drivers/pci/endpoint/pci-epc-mem.c
> @@ -93,6 +93,13 @@ 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)
> +{
> +	return __pci_epc_mem_init(epc, base, size, page_size);
> +}
> +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
> diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
> index e0ed9d01f6e5..5bc1de65849e 100644
> --- a/include/linux/pci-epc.h
> +++ b/include/linux/pci-epc.h
> @@ -137,9 +137,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,6 +192,8 @@ 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 base,
> +		     size_t size, size_t page_size);
>  int __pci_epc_mem_init(struct pci_epc *epc, phys_addr_t phys_addr, size_t size,
>  		       size_t page_size);
>  void pci_epc_mem_exit(struct pci_epc *epc);
>
Kishon Vijay Abraham I May 7, 2020, 2:52 a.m. UTC | #11
On 4/23/2020 11:52 PM, Lad Prabhakar wrote:
> 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 pci_epc_multi_mem_init()
> initializes multiple windows supported by endpoint controller.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  .../pci/controller/dwc/pcie-designware-ep.c   |  16 +-
>  drivers/pci/endpoint/pci-epc-mem.c            | 199 ++++++++++++------
>  include/linux/pci-epc.h                       |  33 ++-
>  3 files changed, 170 insertions(+), 78 deletions(-)
> 
> 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/endpoint/pci-epc-mem.c b/drivers/pci/endpoint/pci-epc-mem.c
> index cdd1d3821249..a3466da2a16f 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;
> @@ -36,67 +36,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
> + * pci_epc_multi_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_multi_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->bitmap = bitmap;
> -	mem->phys_base = phys_base;
> -	mem->page_size = page_size;
> -	mem->pages = pages;
> -	mem->size = size;
> -	mutex_init(&mem->lock);
> +		mem = kzalloc(sizeof(*mem), GFP_KERNEL);
> +		if (!mem) {
> +			ret = -ENOMEM;
> +			i--;
> +			goto err_mem;
> +		}
>  
> -	epc->mem = mem;
> +		bitmap = kzalloc(bitmap_size, GFP_KERNEL);
> +		if (!bitmap) {
> +			ret = -ENOMEM;
> +			kfree(mem);
> +			i--;
> +			goto err_mem;
> +		}
> +
> +		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 = 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);
> +EXPORT_SYMBOL_GPL(pci_epc_multi_mem_init);
>  
>  int pci_epc_mem_init(struct pci_epc *epc, phys_addr_t base,
>  		     size_t size, size_t page_size)
>  {
> -	return __pci_epc_mem_init(epc, base, size, 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_multi_mem_init(epc, &mem_window, 1);
>  }
>  EXPORT_SYMBOL_GPL(pci_epc_mem_init);
>  
> @@ -109,11 +137,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);
>  
> @@ -129,31 +168,60 @@ 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;
> +	size_t align_size;
> +	int pageno;
>  	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);
> +		align_size = ALIGN(size, mem->window.page_size);
> +		order = pci_epc_mem_get_order(mem, align_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, align_size);
> +			if (!virt_addr) {
> +				bitmap_release_region(mem->bitmap,
> +						      pageno, order);
> +				mutex_unlock(&mem->lock);
> +				continue;
> +			}
> +			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
> @@ -166,14 +234,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 5bc1de65849e..cc66bec8be90 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
>   */
>  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 */
> @@ -194,8 +208,9 @@ void pci_epc_put(struct pci_epc *epc);
>  
>  int pci_epc_mem_init(struct pci_epc *epc, phys_addr_t base,
>  		     size_t size, size_t page_size);
> -int __pci_epc_mem_init(struct pci_epc *epc, phys_addr_t phys_addr, size_t size,
> -		       size_t page_size);
> +int pci_epc_multi_mem_init(struct pci_epc *epc,
> +			   struct pci_epc_mem_window *window,
> +			   unsigned int num_windows);
>  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);
>
Kishon Vijay Abraham I May 7, 2020, 3:12 a.m. UTC | #12
Hi Prabhakar,

On 5/5/2020 3:17 PM, Lad, Prabhakar wrote:
> Hi Lorenzo,
> 
> On Tue, May 5, 2020 at 10:44 AM Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
>>
>> On Thu, Apr 30, 2020 at 09:43:20AM +0100, Lad, Prabhakar wrote:
>>> Hi Kishon,
>>>
>>> On Thu, Apr 23, 2020 at 7:23 PM Lad Prabhakar
>>> <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
>>>>
>>>> Hi All,
>>>>
>>>> This patch series adds support for endpoint driver for R-Car PCIe controller on
>>>> R-Car/RZ-G2x SoC's, this also extends the epf framework to handle multiple windows
>>>> supported by the controller for mapping PCI address locally.
>>>>
>>>> Note:
>>>> The cadence/rockchip/designware endpoint drivers are build tested only.
>>>>
>>>> Changes for v9 (Re-spun this series as there were minimal changes requested):
>>>> * Rebased patches on top of v5.7.rc1
>>>> * Replaced mdelay(1) with usleep_range(1000, 1001) in rcar_pcie_ep_assert_intx()
>>>> * Added a check for max_functions read from DT to restrict with
>>>>   RCAR_EPC_MAX_FUNCTIONS
>>>> * Replaced MSICAP0_MMENUM with MSICAP0_MMESE
>>>> * Retry ioremap for other windows on failure in pci_epc_mem_alloc_addr()
>>>> * Fixed looping for number windows in pci_epc_mem_exit()
>>>> * Set maximum to 1 for max-functions in DT binding (I have restored the acks
>>>>   from  Rob and Shimoda-san)
>>>> * Sorted the entry in MAINTAINERS
>>>>
>>>> Changes for v8:
>>>> * Dropped adding R8A774C0 (0x002d) pci-id in pci_ids.h
>>>> * Fixed typo in commit message for patch 2/8
>>>> * Reworded commit message for patch 5/8 as suggested by Bjorn
>>>> * Split up patch to add pci_epc_mem_init() interface to add page_size argument
>>>>   as suggested by Bjorn.
>>>>
>>>> Changes for v7:
>>>> * Fixed review comments pointed by Shimoda-san
>>>>   1] Made DT bindings dual licensed, added Shimoda-san as maintainer and fixed
>>>>      the example as its built with #{address,size}-cells = <1>. I have still
>>>>      restored the Ack from Rob and Shimoda-san with these changes.
>>>>   2] Split up the patches so that they can be picked up by respective subsystem
>>>>      patches 1/4-9/11 are now part of this series.
>>>>   3] Dropped altering a comment in pci-epc.h
>>>>   4] Used a local variable align_size in pci_epc_mem_alloc_addr() so that size
>>>>      variable doesn't get overwritten in the loop.
>>>>   5] Replaced i-=1 with i--
>>>>   6] Replaced rcar with R-Car in patch subject and description.
>>>>   7] Set MACCTLR in init() callback
>>>>
>>>> Changes for v6:
>>>> 1] Rebased patches on endpoint branch of https://git.kernel.org/pub/
>>>>    scm/linux/kernel/git/lpieralisi/pci.git/
>>>> 2] Fixed review comments from Shimoda-san
>>>>    a] Made sure defconfig changes were in separate patch
>>>>    b] Created rcar_pcie_host/rcar_pcie_ep structures
>>>>    c] Added pci-id for R8A774C0
>>>>    d] Added entry in MAINTAINERS for dt-binding
>>>>    e] Dropped unnecessary braces
>>>> 3] Added support for msi.
>>>>
>>>> Changes for v5:
>>>> 1] Rebased patches on next branch of https://git.kernel.org/pub/scm/
>>>>    linux/kernel/git/helgaas/pci.git
>>>> 2] Fixed review comments reported by Kishon while fetching the matching
>>>>    window in function pci_epc_get_matching_window()
>>>> 3] Fixed review comments reported by Bjorn
>>>>    a] Split patch up first patch so that its easier to review and incremental
>>>>    b] Fixed typos
>>>> 4] Included Reviewed tag from Rob for the dt-binding patch
>>>> 5] Fixed issue reported by Nathan for assigning variable to itself
>>>>
>>>> Changes for v4:
>>>> 1] Fixed dtb_check error reported by Rob
>>>> 2] Fixed review comments reported by Kishon
>>>>    a] Dropped pci_epc_find_best_fit_window()
>>>>    b] Fixed initializing mem ptr in __pci_epc_mem_init()
>>>>    c] Dropped map_size from pci_epc_mem_window structure
>>>>
>>>> Changes for v3:
>>>> 1] Fixed review comments from Bjorn and Kishon.
>>>> 3] Converted to DT schema
>>>>
>>>> Changes for v2:
>>>> 1] Fixed review comments from Biju for dt-bindings to include an example
>>>>    for a tested platform.
>>>> 2] Fixed review comments from Kishon to extend the features of outbound
>>>>    regions in epf framework.
>>>> 3] Added support to parse outbound-ranges in OF.
>>>>
>>>> Lad Prabhakar (8):
>>>>   PCI: rcar: Rename pcie-rcar.c to pcie-rcar-host.c
>>>>   PCI: rcar: Move shareable code to a common file
>>>>   PCI: rcar: Fix calculating mask for PCIEPAMR register
>>>>   PCI: endpoint: Pass page size as argument to pci_epc_mem_init()
>>>>   PCI: endpoint: Add support to handle multiple base for mapping
>>>>     outbound memory
>>> Could you please do the needy for the above two patches, so that this
>>> can be picked up by Lorenzo.
>>
>> Yes please. I would kindly ask you to rebase it on top of my
>> pci/rcar branch - with Kishon's ACK when provided.
>>
> Sure will do that as soon as I get Kishon's Ack.

I've given my Acked by on the two endpoint core patches. I've also tested my
endpoint series [1] after applying this series.

Thanks
Kishon

[1] -> http://lore.kernel.org/r/20200506151429.12255-1-kishon@ti.com
> 
> Cheers,
> --Prabhakar
> 
>> Thanks,
>> Lorenzo
>>
>>> Cheers,
>>> --Prabhakar
>>>
>>>>   dt-bindings: PCI: rcar: Add bindings for R-Car PCIe endpoint
>>>>     controller
>>>>   PCI: rcar: Add endpoint mode support
>>>>   MAINTAINERS: Add file patterns for rcar PCI device tree bindings
>>>>
>>>>  .../devicetree/bindings/pci/rcar-pci-ep.yaml  |   77 ++
>>>>  MAINTAINERS                                   |    1 +
>>>>  drivers/pci/controller/Kconfig                |   18 +
>>>>  drivers/pci/controller/Makefile               |    3 +-
>>>>  .../pci/controller/cadence/pcie-cadence-ep.c  |    2 +-
>>>>  .../pci/controller/dwc/pcie-designware-ep.c   |   16 +-
>>>>  drivers/pci/controller/pcie-rcar-ep.c         |  557 ++++++++
>>>>  drivers/pci/controller/pcie-rcar-host.c       | 1065 +++++++++++++++
>>>>  drivers/pci/controller/pcie-rcar.c            | 1206 +----------------
>>>>  drivers/pci/controller/pcie-rcar.h            |  140 ++
>>>>  drivers/pci/controller/pcie-rockchip-ep.c     |    2 +-
>>>>  drivers/pci/endpoint/pci-epc-mem.c            |  204 ++-
>>>>  include/linux/pci-epc.h                       |   38 +-
>>>>  13 files changed, 2078 insertions(+), 1251 deletions(-)
>>>>  create mode 100644 Documentation/devicetree/bindings/pci/rcar-pci-ep.yaml
>>>>  create mode 100644 drivers/pci/controller/pcie-rcar-ep.c
>>>>  create mode 100644 drivers/pci/controller/pcie-rcar-host.c
>>>>  create mode 100644 drivers/pci/controller/pcie-rcar.h
>>>>
>>>> --
>>>> 2.17.1
>>>>
Prabhakar May 7, 2020, 7:34 a.m. UTC | #13
Hi Kishon,

On Thu, May 7, 2020 at 4:13 AM Kishon Vijay Abraham I <kishon@ti.com> wrote:
>
> Hi Prabhakar,
>
> On 5/5/2020 3:17 PM, Lad, Prabhakar wrote:
> > Hi Lorenzo,
> >
> > On Tue, May 5, 2020 at 10:44 AM Lorenzo Pieralisi
> > <lorenzo.pieralisi@arm.com> wrote:
> >>
> >> On Thu, Apr 30, 2020 at 09:43:20AM +0100, Lad, Prabhakar wrote:
> >>> Hi Kishon,
> >>>
> >>> On Thu, Apr 23, 2020 at 7:23 PM Lad Prabhakar
> >>> <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> >>>>
> >>>> Hi All,
> >>>>
> >>>> This patch series adds support for endpoint driver for R-Car PCIe controller on
> >>>> R-Car/RZ-G2x SoC's, this also extends the epf framework to handle multiple windows
> >>>> supported by the controller for mapping PCI address locally.
> >>>>
> >>>> Note:
> >>>> The cadence/rockchip/designware endpoint drivers are build tested only.
> >>>>
> >>>> Changes for v9 (Re-spun this series as there were minimal changes requested):
> >>>> * Rebased patches on top of v5.7.rc1
> >>>> * Replaced mdelay(1) with usleep_range(1000, 1001) in rcar_pcie_ep_assert_intx()
> >>>> * Added a check for max_functions read from DT to restrict with
> >>>>   RCAR_EPC_MAX_FUNCTIONS
> >>>> * Replaced MSICAP0_MMENUM with MSICAP0_MMESE
> >>>> * Retry ioremap for other windows on failure in pci_epc_mem_alloc_addr()
> >>>> * Fixed looping for number windows in pci_epc_mem_exit()
> >>>> * Set maximum to 1 for max-functions in DT binding (I have restored the acks
> >>>>   from  Rob and Shimoda-san)
> >>>> * Sorted the entry in MAINTAINERS
> >>>>
> >>>> Changes for v8:
> >>>> * Dropped adding R8A774C0 (0x002d) pci-id in pci_ids.h
> >>>> * Fixed typo in commit message for patch 2/8
> >>>> * Reworded commit message for patch 5/8 as suggested by Bjorn
> >>>> * Split up patch to add pci_epc_mem_init() interface to add page_size argument
> >>>>   as suggested by Bjorn.
> >>>>
> >>>> Changes for v7:
> >>>> * Fixed review comments pointed by Shimoda-san
> >>>>   1] Made DT bindings dual licensed, added Shimoda-san as maintainer and fixed
> >>>>      the example as its built with #{address,size}-cells = <1>. I have still
> >>>>      restored the Ack from Rob and Shimoda-san with these changes.
> >>>>   2] Split up the patches so that they can be picked up by respective subsystem
> >>>>      patches 1/4-9/11 are now part of this series.
> >>>>   3] Dropped altering a comment in pci-epc.h
> >>>>   4] Used a local variable align_size in pci_epc_mem_alloc_addr() so that size
> >>>>      variable doesn't get overwritten in the loop.
> >>>>   5] Replaced i-=1 with i--
> >>>>   6] Replaced rcar with R-Car in patch subject and description.
> >>>>   7] Set MACCTLR in init() callback
> >>>>
> >>>> Changes for v6:
> >>>> 1] Rebased patches on endpoint branch of https://git.kernel.org/pub/
> >>>>    scm/linux/kernel/git/lpieralisi/pci.git/
> >>>> 2] Fixed review comments from Shimoda-san
> >>>>    a] Made sure defconfig changes were in separate patch
> >>>>    b] Created rcar_pcie_host/rcar_pcie_ep structures
> >>>>    c] Added pci-id for R8A774C0
> >>>>    d] Added entry in MAINTAINERS for dt-binding
> >>>>    e] Dropped unnecessary braces
> >>>> 3] Added support for msi.
> >>>>
> >>>> Changes for v5:
> >>>> 1] Rebased patches on next branch of https://git.kernel.org/pub/scm/
> >>>>    linux/kernel/git/helgaas/pci.git
> >>>> 2] Fixed review comments reported by Kishon while fetching the matching
> >>>>    window in function pci_epc_get_matching_window()
> >>>> 3] Fixed review comments reported by Bjorn
> >>>>    a] Split patch up first patch so that its easier to review and incremental
> >>>>    b] Fixed typos
> >>>> 4] Included Reviewed tag from Rob for the dt-binding patch
> >>>> 5] Fixed issue reported by Nathan for assigning variable to itself
> >>>>
> >>>> Changes for v4:
> >>>> 1] Fixed dtb_check error reported by Rob
> >>>> 2] Fixed review comments reported by Kishon
> >>>>    a] Dropped pci_epc_find_best_fit_window()
> >>>>    b] Fixed initializing mem ptr in __pci_epc_mem_init()
> >>>>    c] Dropped map_size from pci_epc_mem_window structure
> >>>>
> >>>> Changes for v3:
> >>>> 1] Fixed review comments from Bjorn and Kishon.
> >>>> 3] Converted to DT schema
> >>>>
> >>>> Changes for v2:
> >>>> 1] Fixed review comments from Biju for dt-bindings to include an example
> >>>>    for a tested platform.
> >>>> 2] Fixed review comments from Kishon to extend the features of outbound
> >>>>    regions in epf framework.
> >>>> 3] Added support to parse outbound-ranges in OF.
> >>>>
> >>>> Lad Prabhakar (8):
> >>>>   PCI: rcar: Rename pcie-rcar.c to pcie-rcar-host.c
> >>>>   PCI: rcar: Move shareable code to a common file
> >>>>   PCI: rcar: Fix calculating mask for PCIEPAMR register
> >>>>   PCI: endpoint: Pass page size as argument to pci_epc_mem_init()
> >>>>   PCI: endpoint: Add support to handle multiple base for mapping
> >>>>     outbound memory
> >>> Could you please do the needy for the above two patches, so that this
> >>> can be picked up by Lorenzo.
> >>
> >> Yes please. I would kindly ask you to rebase it on top of my
> >> pci/rcar branch - with Kishon's ACK when provided.
> >>
> > Sure will do that as soon as I get Kishon's Ack.
>
> I've given my Acked by on the two endpoint core patches. I've also tested my
> endpoint series [1] after applying this series.
>
Thank you for testing and the Ack's.

Cheers,
--Prabhakar

> Thanks
> Kishon
>
> [1] -> http://lore.kernel.org/r/20200506151429.12255-1-kishon@ti.com
> >
> > Cheers,
> > --Prabhakar
> >
> >> Thanks,
> >> Lorenzo
> >>
> >>> Cheers,
> >>> --Prabhakar
> >>>
> >>>>   dt-bindings: PCI: rcar: Add bindings for R-Car PCIe endpoint
> >>>>     controller
> >>>>   PCI: rcar: Add endpoint mode support
> >>>>   MAINTAINERS: Add file patterns for rcar PCI device tree bindings
> >>>>
> >>>>  .../devicetree/bindings/pci/rcar-pci-ep.yaml  |   77 ++
> >>>>  MAINTAINERS                                   |    1 +
> >>>>  drivers/pci/controller/Kconfig                |   18 +
> >>>>  drivers/pci/controller/Makefile               |    3 +-
> >>>>  .../pci/controller/cadence/pcie-cadence-ep.c  |    2 +-
> >>>>  .../pci/controller/dwc/pcie-designware-ep.c   |   16 +-
> >>>>  drivers/pci/controller/pcie-rcar-ep.c         |  557 ++++++++
> >>>>  drivers/pci/controller/pcie-rcar-host.c       | 1065 +++++++++++++++
> >>>>  drivers/pci/controller/pcie-rcar.c            | 1206 +----------------
> >>>>  drivers/pci/controller/pcie-rcar.h            |  140 ++
> >>>>  drivers/pci/controller/pcie-rockchip-ep.c     |    2 +-
> >>>>  drivers/pci/endpoint/pci-epc-mem.c            |  204 ++-
> >>>>  include/linux/pci-epc.h                       |   38 +-
> >>>>  13 files changed, 2078 insertions(+), 1251 deletions(-)
> >>>>  create mode 100644 Documentation/devicetree/bindings/pci/rcar-pci-ep.yaml
> >>>>  create mode 100644 drivers/pci/controller/pcie-rcar-ep.c
> >>>>  create mode 100644 drivers/pci/controller/pcie-rcar-host.c
> >>>>  create mode 100644 drivers/pci/controller/pcie-rcar.h
> >>>>
> >>>> --
> >>>> 2.17.1
> >>>>
Pavel Machek May 7, 2020, 5:44 p.m. UTC | #14
Hi!


> R-Car/RZ-G2x SoC's, this also extends the epf framework to handle multiple windows
> supported by the controller for mapping PCI address locally.
> 
> Note:
> The cadence/rockchip/designware endpoint drivers are build tested only.
> 
> Changes for v9 (Re-spun this series as there were minimal changes requested):
...
> * Replaced mdelay(1) with usleep_range(1000, 1001) in rcar_pcie_ep_assert_intx()

Are you sure that is good idea? You are requesting 1ms sleep time with 1us tolerance,
I dont believe common systems can do that.

Best regards,
									Pavel
Prabhakar May 7, 2020, 5:53 p.m. UTC | #15
Hi Pavel,

On Thu, May 7, 2020 at 6:44 PM Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
>
>
> > R-Car/RZ-G2x SoC's, this also extends the epf framework to handle multiple windows
> > supported by the controller for mapping PCI address locally.
> >
> > Note:
> > The cadence/rockchip/designware endpoint drivers are build tested only.
> >
> > Changes for v9 (Re-spun this series as there were minimal changes requested):
> ...
> > * Replaced mdelay(1) with usleep_range(1000, 1001) in rcar_pcie_ep_assert_intx()
>
> Are you sure that is good idea? You are requesting 1ms sleep time with 1us tolerance,
> I dont believe common systems can do that.
>
Agreed the systems cannot do that, but the main reason of replacing
mdelay(1) with usleep_range(1000, 1001) was since  pci_epc_raise_irq()
calls mutex_lock() and then this function rcar_pcie_ep_assert_intx(),
so we can assume this function also can sleep. And, according to
Documentation/timers/timers-howto.rst, we should use usleep_range()
instead of mdelay().

Cheers,
--Prabhakar



> Best regards,
>                                                                         Pavel
>
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html