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

[Niklas Cassel <cassel@kernel.org>]

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.


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

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

We already supply 'struct pci_epf_bar' to .set_bar(). I think the simplest
is just to extend this struct with more members (e.g. requires_fixed_addr,
fixed_addr). Basically 'struct pci_epf_bar' has all the wishes of the EPF
(32-bit/64-bit, IO/MEM BAR, BAR size, etc.).

It is already up to the EPC driver to fail the .set_bar() request if it
can't be satisfied, so pci_epf_setup_bar() could behave like .set_bar().


Kind regards,
Niklas