Message-ID: <04b1ceb2-538e-4b1b-be5c-5a81d7a457ad@kernel.org>
Date: Thu, 25 Jul 2024 17:20:00 +0900
From: Damien Le Moal <dlemoal@...nel.org>
To: Rick Wertenbroek <rick.wertenbroek@...il.com>,
 Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc: Niklas Cassel <cassel@...nel.org>, rick.wertenbroek@...g-vd.ch,
 alberto.dassatti@...g-vd.ch, Krzysztof Wilczyński
 <kw@...ux.com>, Kishon Vijay Abraham I <kishon@...nel.org>,
 Bjorn Helgaas <bhelgaas@...gle.com>, Frank Li <Frank.Li@....com>,
 Lars-Peter Clausen <lars@...afoo.de>, linux-pci@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] PCI: endpoint: Introduce 'get_bar' to map fixed
 address BARs in EPC

On 7/25/24 17:06, Rick Wertenbroek wrote:
> On Thu, Jul 25, 2024 at 7:33 AM Manivannan Sadhasivam
> <manivannan.sadhasivam@...aro.org> 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.

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).

-- 
Damien Le Moal
Western Digital Research

Powered by blists - more mailing lists