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 > > <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. > > > > 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 -- மணிவண்ணன் சதாசிவம்
