Hi Prabhakar-san,
Just in my opinion though, automatically adding new line of email client
should be disabled or setting larger characters.
# In my side, I change the setting as 132 characters on Outlook :)
> From: Prabhakar Mahadev Lad, Sent: Tuesday, March 17, 2020 7:04 PM
<snip>
> > > -int __pci_epc_mem_init(struct pci_epc *epc, phys_addr_t phys_base,
> > size_t size,
> > > - size_t page_size)
> > > +int __pci_epc_mem_init(struct pci_epc *epc, struct
> > pci_epc_mem_window *windows,
> > > + 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;
> > > -
> > > - if (page_size < PAGE_SIZE)
> > > - page_size = PAGE_SIZE;
> > > -
> > > - page_shift = ilog2(page_size);
> > > - pages = size >> page_shift;
> > > - bitmap_size = BITS_TO_LONGS(pages) * sizeof(long);
> > > -
> > > - mem = kzalloc(sizeof(*mem), GFP_KERNEL);
> > > - if (!mem) {
> > > - ret = -ENOMEM;
> > > - goto err;
> > > - }
> > > -
> > > - bitmap = kzalloc(bitmap_size, GFP_KERNEL);
> > > - if (!bitmap) {
> > > - ret = -ENOMEM;
> > > - goto err_mem;
> > > + int pages;
> > > + int ret;
> > > + int i;
> > > +
> > > + epc->mem_windows = 0;
> > > +
> > > + if (!windows)
> > > + return -EINVAL;
> > > +
> > > + if (num_windows <= 0)
> > > + return -EINVAL;
> > > +
> > > + epc->mem = kcalloc(num_windows, sizeof(*mem), GFP_KERNEL);
> > > + if (!epc->mem)
> > > + return -EINVAL;
> > > +
> > > + for (i = 0; i < num_windows; i++) {
> > > + page_size = windows[i].page_size;
> > > + if (page_size < PAGE_SIZE)
> > > + page_size = PAGE_SIZE;
> > > + page_shift = ilog2(page_size);
> > > + pages = windows[i].size >> page_shift;
> > > + bitmap_size = BITS_TO_LONGS(pages) * sizeof(long);
> > > +
> > > + mem = kzalloc(sizeof(*mem), GFP_KERNEL);
> > > + if (!mem) {
> > > + ret = -ENOMEM;
> > > + goto err_mem;
> > > + }
> > > +
> > > + bitmap = kzalloc(bitmap_size, GFP_KERNEL);
> > > + if (!bitmap) {
> > > + ret = -ENOMEM;
> > > + goto err_mem;
> > > + }
> > > +
> > > + mem->bitmap = bitmap;
> > > + mem->window.phys_base = windows[i].phys_base;
> >
> > I could not understand why the window member is needed.
> > I think original members (just phys_base and size) are enough.
> > Also, this function doesn't store the page_size to mem->window.page_size.
I'm sorry, but I meant the window member is in the left side (mem->window.phys_base).
In other words, this patch changes the struct pci_epc_mem like below, but
I'm thinking this change is not needed because struct pci_epc will have
multiple windows as "array of address space of the endpoint controller".
---
struct pci_epc_mem {
- phys_addr_t phys_base;
- size_t size;
+ struct pci_epc_mem_window window;
---
> Because, for example on RZ/Gx platforms, following are the windows on endpoint device
> where the root's address can be mapped, but where as on other platforms atm there
> exists just single window to map. Also on RZ/Gx platforms if a window is allocated say of
> size 1K, rest of the window cannot be used for other allocations.
>
> 1: 0xfe000000 0 0x80000
> 2: 0xfe100000 0 0x100000
> 3: 0xfe200000 0 0x200000
> 4: 0x30000000 0 0x8000000
> 5: 0x38000000 0 0x8000000
>
> Struct pci_epc_mem_window, represents the above windows.
Yes, I understood it.
> window.page_size is set by endpoint controller drivers as done in this patch.
I meant the left side. No one change the mem->window.page_size so that
the value seems to be 0. Of course, for now, this is no problem because
no one uses this value though.
<snip>
> > > /**
> > > * 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
> > > + * @pages: number of bits representing the address region
> > > */
> > > struct pci_epc_mem {
> > > - phys_addr_t phys_base;
> > > - size_t size;
> > > + struct pci_epc_mem_window window;
> > > unsigned long *bitmap;
> > > size_t page_size;
> > > int pages;
> > > @@ -85,7 +97,8 @@ 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
> >
> > If my idea is acceptable, this should be "default address space ...".
> >
> Could you please elaborate more on how you would like the structures to be organized.
* @mem: default address space of the endpoint controller.
And, if I assumed the "array of address space of the endpoint controller"
is renamed as struct pci_epc_mem **windows and when __pci_epc_mem_init() is succeeded,
the function should set the mem value right before return as the first window like below.
+ epc->mem = epc->windows[0];
+ epc->num_windows = num_windows;
return 0;
Best regards,
Yoshihiro Shimoda
