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

[Manivannan Sadhasivam]

On Thu, Jul 25, 2024 at 04:17:03PM +0200, Niklas Cassel wrote:
> On Thu, Jul 25, 2024 at 10:06:38AM +0200, Rick Wertenbroek wrote:
> > 
> > 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).
> 
> I agree that the MHI case (EPF requires a specific host address for the BAR)
> and the FPGA case (EPC requires a specific host address for the BAR),
> is different.
> 
> Right now, for EPC requirements, we have epc_features in the driver that
> describes hardware for this EPC (e.g. fixed size BARs). Right now, I don't
> see a reason to move this to DT (or let a DT alternative co-exist).
> 
> 
> For EPF requirements, the MHI EPF driver exposes several different
> versions (pci_epf_mhi_sa8775p, pci_epf_mhi_sdx55, pci_epf_mhi_sm8450)
> in configfs, and each have their own specific driver data.
> 
> The only negative I can see with this is that it might clutter the
> /sys/kernel/config/pci_ep/functions/ directory. Perhaps it is possible
> to create a /sys/kernel/config/pci_ep/functions/pci_epf_mhi/ directory
> where all the different versions reside?
> 
> Keeping this per-platform data in the MHI EPF driver is fine IMO.
> 
> If you would prefer to create a "pci_epf_mhi" generic version, that instead
> takes this information from configfs, that would be fine too. I would also
> be fine if you created a "pci_epf_mhi" generic version that tries to take
> this information from device tree (as long as it is also possible to supply
> the same information via configfs).
> 
> Good luck getting it accepted by the DT maintainers though. The configfs
> interface was chosen because some developers (including Arnd Bergmann, IIRC)
> didn't like the idea of having EPF specific information in DT.
> 

I know the story and they were right (Arnd/Rob) as the initial version tried to
use DT for everything. But MHI is different as it is a hardware entity, which
not only uses a fixed address, it uses a fixed region that has MHI registers in
hw.

Anyway, I don't have a *strong* motivation to upstream the DT support for it as
the current approach is serving good for now, unless new usecases show up.

> 
> > For combining pci_epf_alloc_space and pci_epc_set_bar into a single
> > function, everyone seems to agree on this one.
> 
> Indeed, but as usual, good naming is one of the hardest problems :)
> 

Respectively agree with you :)

> Instead of pci_epf_alloc_set_bar(), perhaps pci_epf_setup_bar() ?
> If the EPC has a fixed address requirement, it will use that instead of
> allocating memory.
> 
> If the EPF has a fixed address requirement, the API call will only succeed
> if EPC does not have a fixed address requirement.
> 
> Perhaps EPF drivers that have a fixed address requirement could supply
> that as a parameter to the API (and the EPC driver will fail the request
> if it itself has fixed address requirement).
> 

I vary with you here. IMO EPF drivers have no business in knowing the BAR
location as they are independent of controller (mostly except drivers like MHI).
So an EPF driver should call a single API that just allocates/configures the
BAR. For fixed address BAR, EPC core should be able to figure it out using the
EPC features.

For naming, we have 3 proposals as of now:

1. pci_epf_setup_bar() - This looks good, but somewhat collides with the
existing pci_epc_set_bar() API.

2. pci_epc_alloc_set_bar() - Looks ugly, but aligns with the existing APIs.

3. pci_epc_get_bar() - Also looks good, but the usage of 'get' gives the
impression that the BAR is fetched from somewhere, which is true for fixed
address BAR, but not for dynamic BAR.

After looking at these APIs multiple times (confusing at its best), I tend to
feel that pci_epc_get_bar() is the better of the 3.

- Mani

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