Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
To:     Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>,
        Lad Prabhakar <prabhakar.csengg@gmail.com>
CC:     Andrew Murray <andrew.murray@arm.com>,
        "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-renesas-soc@vger.kernel.org" 
        <linux-renesas-soc@vger.kernel.org>,
        "linux-rockchip@lists.infradead.org" 
        <linux-rockchip@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Rob Herring <robh+dt@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        Kishon Vijay Abraham I <kishon@ti.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Arnd Bergmann <arnd@arndb.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jingoo Han <jingoohan1@gmail.com>,
        Gustavo Pimentel <gustavo.pimentel@synopsys.com>,
        Marek Vasut <marek.vasut+renesas@gmail.com>,
        Shawn Lin <shawn.lin@rock-chips.com>,
        Heiko Stuebner <heiko@sntech.de>
Subject: RE: [PATCH v5 4/7] PCI: endpoint: Add support to handle multiple base
 for mapping outbound memory
Thread-Topic: [PATCH v5 4/7] PCI: endpoint: Add support to handle multiple
 base for mapping outbound memory
Thread-Index: AQHV7k2kfqtu6T0o2UCPs98u98LxN6hMgYXQgAAogACAAAq1EA==
Date:   Tue, 17 Mar 2020 11:04:47 +0000
Message-ID: <TYAPR01MB4544DD58C495D0ED223B1A7BD8F60@TYAPR01MB4544.jpnprd01.prod.outlook.com>
References: <20200228154122.14164-1-prabhakar.mahadev-lad.rj@bp.renesas.com>
 <20200228154122.14164-5-prabhakar.mahadev-lad.rj@bp.renesas.com>
 <TYAPR01MB4544599A72C88666B4F972B7D8F60@TYAPR01MB4544.jpnprd01.prod.outlook.com>
 <OSBPR01MB359001B994CFC0CB45170AB0AAF60@OSBPR01MB3590.jpnprd01.prod.outlook.com>
In-Reply-To: <OSBPR01MB359001B994CFC0CB45170AB0AAF60@OSBPR01MB3590.jpnprd01.prod.outlook.com>
Accept-Language: ja-JP, en-US
Content-Language: ja-JP
received-spf: None (protection.outlook.com: renesas.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 40cdaf7d-7c99-4e70-a0cd-08d7ca6301fc
X-MS-Exchange-CrossTenant-originalarrivaltime: 17 Mar 2020 11:04:47.7342
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 53d82571-da19-47e4-9cb4-625a166a4a2a
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 31GP9C05q0AwyLTkFmi5cw5uvHcx7oYOXaCLBuja4IGUxwxMbBSmCH7emkSm+HLcLBh8H52ron+RvvD5gvQ7MZygP1strqoQvsJ1CPG+i6e5G8ZN+ZwIM3ENAhiU88DA
X-MS-Exchange-Transport-CrossTenantHeadersStamped: TYAPR01MB3616
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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 =3D NULL;
> > > +	unsigned long *bitmap =3D NULL;
> > >  	unsigned int page_shift;
> > > -	int pages;
> > > +	size_t page_size;
> > >  	int bitmap_size;
> > > -
> > > -	if (page_size < PAGE_SIZE)
> > > -		page_size =3D PAGE_SIZE;
> > > -
> > > -	page_shift =3D ilog2(page_size);
> > > -	pages =3D size >> page_shift;
> > > -	bitmap_size =3D BITS_TO_LONGS(pages) * sizeof(long);
> > > -
> > > -	mem =3D kzalloc(sizeof(*mem), GFP_KERNEL);
> > > -	if (!mem) {
> > > -		ret =3D -ENOMEM;
> > > -		goto err;
> > > -	}
> > > -
> > > -	bitmap =3D kzalloc(bitmap_size, GFP_KERNEL);
> > > -	if (!bitmap) {
> > > -		ret =3D -ENOMEM;
> > > -		goto err_mem;
> > > +	int pages;
> > > +	int ret;
> > > +	int i;
> > > +
> > > +	epc->mem_windows =3D 0;
> > > +
> > > +	if (!windows)
> > > +		return -EINVAL;
> > > +
> > > +	if (num_windows <=3D 0)
> > > +		return -EINVAL;
> > > +
> > > +	epc->mem =3D kcalloc(num_windows, sizeof(*mem), GFP_KERNEL);
> > > +	if (!epc->mem)
> > > +		return -EINVAL;
> > > +
> > > +	for (i =3D 0; i < num_windows; i++) {
> > > +		page_size =3D windows[i].page_size;
> > > +		if (page_size < PAGE_SIZE)
> > > +			page_size =3D PAGE_SIZE;
> > > +		page_shift =3D ilog2(page_size);
> > > +		pages =3D windows[i].size >> page_shift;
> > > +		bitmap_size =3D BITS_TO_LONGS(pages) * sizeof(long);
> > > +
> > > +		mem =3D kzalloc(sizeof(*mem), GFP_KERNEL);
> > > +		if (!mem) {
> > > +			ret =3D -ENOMEM;
> > > +			goto err_mem;
> > > +		}
> > > +
> > > +		bitmap =3D kzalloc(bitmap_size, GFP_KERNEL);
> > > +		if (!bitmap) {
> > > +			ret =3D -ENOMEM;
> > > +			goto err_mem;
> > > +		}
> > > +
> > > +		mem->bitmap =3D bitmap;
> > > +		mem->window.phys_base =3D 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_siz=
e.

I'm sorry, but I meant the window member is in the left side (mem->window.p=
hys_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 en=
dpoint device
> where the root's address can be mapped, but where as on other platforms a=
tm 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.
>=20
> 1: 0xfe000000 0 0x80000
> 2: 0xfe100000 0 0x100000
> 3: 0xfe200000 0 0x200000
> 4: 0x30000000 0 0x8000000
> 5: 0x38000000 0 0x8000000
>=20
> 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 pa=
tch.

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 b=
e 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 wind=
ow like below.

+	epc->mem =3D epc->windows[0];
+	epc->num_windows =3D num_windows;

	return 0;

Best regards,
Yoshihiro Shimoda