From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pf1-f178.google.com (mail-pf1-f178.google.com [209.85.210.178]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 85B6D1103 for ; Thu, 25 Jul 2024 14:07:21 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.210.178 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1721916443; cv=none; b=rLEtp1HrzcHTT68IiKLNDu4ZqhhZsX6ls/kegEpDf2AI6+B0VeQ2rA6Arrw3liqpMZbxpPkjyJE7+0uCxnhihAz6hZwuAv1dMuset1i3Y1iWhFqcJrL7XkK+rrVjxEtwaT/SU+OyYcFl+2vllWpYVRA6OeCXzxt1oAuPXOKzfmY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1721916443; c=relaxed/simple; bh=pksET2uKbCnujR75iT+0cBqjz1m524Ir2g+SdXltX4U=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=ph2/RP0oCj6Bllo1n+IUu/+5gT20L+Huvzq3ZPsJGr2U/ak30A93+XN1cTRajvPk72jGuuqZQpDoeLKtf6aMTRj/Vn4nvF3dyxAgMmJNXwEStj+1u0odh0YMt8VqUUjPx9po6RrNFAFWMoKocVcwnUL4ZtwtvdVmVhq+MN0+ueo= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=QZW0kiTc; arc=none smtp.client-ip=209.85.210.178 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="QZW0kiTc" Received: by mail-pf1-f178.google.com with SMTP id d2e1a72fcca58-70d153fec2fso778295b3a.1 for ; Thu, 25 Jul 2024 07:07:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1721916441; x=1722521241; darn=vger.kernel.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=a3AyL0HlCFj8TAMHagfyvA5AIVg3hc44mddnltY1Iss=; b=QZW0kiTc9n4grvZ2D0RgCSNPvpgUCUSpWZFbhgIqlWi9NsO7/N8Nu5dDkTJsit7u/T TC624i8LTi42QvlShYVs2MXxLy4L39MiEL8dFyNd3T7ZLCY9b7/w8epSc0jUw9X7l3m3 DBZSczBgugtBWyg8kQG7bn/Z/sl9kQ5iU6/8keqZLYGrcqfz1lNOyl7XBu/uM3kp9OKM 1jO9e+Z3x0JuCckwOJiANW7foH4xx4AT6PVdAHoE86y3ZFaOHRMSiVW4KF5uK5LgP7wb ATHL6X+oazBvhz2zdIGg/vEYMRdY3l2EC9iVLRNjUVkLr8HB66p/5EcAxyLyLyODuSPY 9zLA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1721916441; x=1722521241; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=a3AyL0HlCFj8TAMHagfyvA5AIVg3hc44mddnltY1Iss=; b=V1wDbYaXvK8vwmWQOiEHFs9P7jmze2CJIbIiCyH5iwnGc5MXgVbrtt++vDNLSqAU3m G7E5KlsNBxxOQn9hnoPVoiQcKIQYsJhh5bBuSqFqdtDrPaw/mDRfwc5oll418v8xH6me ywJLSM00xaKsj4sZd2IyoIyr4YHutIpLSAr+3duxcoMM2s3ztDxst1VJwg8oCgN9ix19 dP7W3m+vOlrs6TV0UggFFlA/Xs4THjrxfZwN8jic2EV7ywhw6LUP7klsuA3r2MdnrUHC RtSPpoxjdn9pppeEuf5oXPPHKJEPAUvbtPsdjW5+qirR2fdHbrfL5Jq7jSQC79K3m/j5 PeOg== X-Forwarded-Encrypted: i=1; AJvYcCX3+rOE7raL+EtTU/b7u4lKNy2GWfpRjNx/WEsJpHZppmk7tIM6f/D/UoTPNfO1fu1/21nXusyXVZjxxEuMbbM6hTxf5KYSixmfphsk X-Gm-Message-State: AOJu0YwxiwiE2XtxPvZF9k3NopSxZIbT3kjcXsBiARt04CiTqEZJoE5l n6Dt17d9NTKE8HOCHCT1dpWm9thFFxqnbvTbtgk5x3BXQnsdL80qu6zAlVmE5g== X-Google-Smtp-Source: AGHT+IGTByaLYvqaz+8FOAit23AMhCHovH60IZ7pQuKJeMbNd+SUp0ANSh1klXcJl1BPmRODoET20g== X-Received: by 2002:a05:6a00:919f:b0:70d:2693:d215 with SMTP id d2e1a72fcca58-70eae906ad6mr2105027b3a.16.1721916440774; Thu, 25 Jul 2024 07:07:20 -0700 (PDT) Received: from thinkpad ([220.158.156.199]) by smtp.gmail.com with ESMTPSA id d2e1a72fcca58-70ead6e15e2sm1168645b3a.3.2024.07.25.07.07.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 25 Jul 2024 07:07:20 -0700 (PDT) Date: Thu, 25 Jul 2024 19:37:15 +0530 From: Manivannan Sadhasivam To: Damien Le Moal Cc: Rick Wertenbroek , Niklas Cassel , rick.wertenbroek@heig-vd.ch, alberto.dassatti@heig-vd.ch, Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= , Kishon Vijay Abraham I , Bjorn Helgaas , Frank Li , Lars-Peter Clausen , linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/2] PCI: endpoint: Introduce 'get_bar' to map fixed address BARs in EPC Message-ID: <20240725140702.GB2274@thinkpad> References: <20240719115741.3694893-1-rick.wertenbroek@gmail.com> <20240719115741.3694893-2-rick.wertenbroek@gmail.com> <20240725053348.GN2317@thinkpad> <04b1ceb2-538e-4b1b-be5c-5a81d7a457ad@kernel.org> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <04b1ceb2-538e-4b1b-be5c-5a81d7a457ad@kernel.org> On Thu, Jul 25, 2024 at 05:20:00PM +0900, Damien Le Moal wrote: > On 7/25/24 17:06, Rick Wertenbroek wrote: > > On Thu, Jul 25, 2024 at 7:33 AM Manivannan Sadhasivam > > wrote: > >> > >> On Tue, Jul 23, 2024 at 06:48:27PM +0200, Niklas Cassel wrote: > >>> Hello Rick, > >>> > >>> On Tue, Jul 23, 2024 at 06:06:26PM +0200, Rick Wertenbroek wrote: > >>>>> > >>>>> But like you suggested in the other mail, the right thing is to merge > >>>>> alloc_space() and set_bar() anyway. (Basically instead of where EPF drivers > >>>>> currently call set_bar(), the should call alloc_and_set_bar() (or whatever) > >>>>> instead.) > >>>>> > >>>> > >>>> Yes, if we merge both, the code will need to be in the EPC code > >>>> (because of the set_bar), and then the pci_epf_alloc_space (if needed) > >>>> would be called internally in the EPC code and not in the endpoint > >>>> function code. > >>>> > >>>> The only downside, as I said in my other mail, is the very niche case > >>>> where the contents of a BAR should be moved and remain unchanged when > >>>> rebinding a given endpoint function from one controller to another. > >>>> But this is not expected in any endpoint function currently, and with > >>>> the new changes, the endpoint could simply copy the BAR contents to a > >>>> local buffer and then set the contents in the BAR of the new > >>>> controller. > >>>> Anyways, probably no one is moving live functions between controllers, > >>>> and if needed it still can be done, so no problem here... > >>> > >>> I think we need to wait for Mani's opinion, but I've never heard of anyone > >>> doing so, and I agree with your suggested feature to copy the BAR contents > >>> in case anyone actually changes the backing EPC controller to an EPF. > >>> (You would need to stop the EPC to unbind + bind anyway, IIRC.) > >>> > >> > >> Hi Rick/Niklas, > >> > >> TBH, I don't think currently we have an usecase for remapping the EPC to EPF. So > >> we do not need to worry until the actual requirement comes. > >> > >> But I really like combining alloc() and set_bar() APIs. Something I wanted to do > >> for so long but never got around to it. We can use the API name something like > >> pci_epc_alloc_set_bar(). I don't like 'set' in the name, but I guess we need to > >> have it to align with existing APIs. > >> > >> And regarding the implementation, the use of fixed address for BAR is not new. > >> If you look into the pci-epf-mhi.c driver, you can see that I use a fixed BAR0 > >> location that is derived from the controller DT node (MMIO region). > >> > >> But I was thinking of moving this region to EPF node once we add DT support for > >> EPF driver. Because, there can be many EPFs in a single endpoint and each can > >> have upto 6 BARs. We cannot really describe each resource in the controller DT > >> node. > >> > >> Given that you really have a usecase now for multiple BARs, I think it is best > >> if we can add DT support for the EPF drivers and describe the BAR resources in > >> each EPF node. With that, we can hide all the resource allocation within the EPC > >> core without exposing any flag to the EPF drivers. > >> > >> So if the EPF node has a fixed location for BAR and defined in DT, then the new > >> API pci_epf_alloc_set_bar() API will use that address and configure it > >> accordingly. If not, it will just call pci_epf_alloc_space() internally to > >> allocate the memory from coherent region and use it. > >> > >> Wdyt? > >> > >> - Mani > >> > >> -- > >> மணிவண்ணன் சதாசிவம் > > > > Hello Mani, thank you for your feedback. > > > > I don't know if the EPF node in the DT is the right place for the > > following reasons. First, it describes the requirements of the EPF and > > not the restrictions imposed by the EPC (so for example one function > > requires the BAR to use a given physical address and this is described > > in the DT EPF node, but the controller could use any address, it just > > configures the controller to use the address the EPF wants, but in the > > other case (e.g., on FPGA), the EPC can only handle a BAR at a given > > physical address and no other address so this should be in the EPC > > node). Second, it is very static, so the EPC relation EPF would be > > bound in the DT, I prefer being able to bind-unbind from configfs so > > that I can make changes without reboot (e.g., alternate between two or > > more endpoint functions, which may have different BAR requirements and > > configurations). > > > > For the EPFs I really think it is good to keep the BAR requirements in > > the code for the moment, this makes it easier to swap endpoint > > functions and makes development easier as well (e.g., binding several > > different EPFs without reboot of the SoC they run on. In my typical > > tests I bind one function, turn-on the host, do tests, turn-off the > > host, make changes to an EPF, scp the new .ko on the SoC, rebind, > > turn-on the host, etc.). For example, I alternate between pci-epf-test > > (6 bars) and pci-epf-nvme (1 bar) of different sizes. > > > > However, I can see a world where both binding and configuring EPF from > > DT and the way it is currently done (alloc/set bar in code) and bind > > in configfs could exist together as each have their use cases. But > > forcing the use of DT seems impractical. > > I do not think using DT for configuring an EPF *ever* make sense. An init script > on the EP platform can take care of whatever needs to be configured. DT is for > and should be restricted to describing the HW, not things that can be configured > at runtime using the HW information. > No, MHI is a hardware entity. So atleast defining it in DT make sense. But as I mentioned in reply to Rick, I haven't implemented it since it is really not required now (MHI just needs a single BAR and right now the address is derived from EPC node). But for Rick's usecase, I agree with you/Rick that DT doesn't make sense. Thanks for clearing it up. > Doing it this way also makes the EPF code independent on the platform. E.g. if > we ever add an EPF node in the DT, that same EPF would not work on an endpoint > capable platform using UEFI. That is not acceptable for HW generic EPFs (e.g. > nvme, virtio, etc). > Well, those anyway cannot be defined in DT as they are software implementations around hw. - Mani -- மணிவண்ணன் சதாசிவம்