On Thu, Jul 25, 2024 at 10:06:38AM +0200, Rick Wertenbroek wrote: > On Thu, Jul 25, 2024 at 7:33 AM Manivannan Sadhasivam > <manivannan.sadhasivam@xxxxxxxxxx> 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. > Ok, clearly the usecase I have for MHI is not the same as yours. MHI is a hardware entity and it has the registers at a fixed address. So defining it in the DT node made sense to me. But I haven't followed up with my proposal since I thought it is not worth the effort until I see more usecases for DT. That's why I was interested for a moment as I thought I got one :) Anyway, thanks for clearing it up. I now agree that we don't need DT node for your usecase. > 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. > > For combining pci_epf_alloc_space and pci_epc_set_bar into a single > function, everyone seems to agree on this one. > Yes. - Mani -- மணிவண்ணன் சதாசிவம்
