Re: [PATCH 1/2] PCI: endpoint: Introduce 'get_bar' to map fixed address BARs in EPC

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

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@linaro.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.
> 

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

-- 
மணிவண்ணன் சதாசிவம்