Message-ID: <1b5847da-780f-47fa-a7a3-2d71f5905334@kernel.org>
Date: Thu, 25 Jul 2024 17:13:43 +0900
From: Damien Le Moal <dlemoal@...nel.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc: Niklas Cassel <cassel@...nel.org>,
 Rick Wertenbroek <rick.wertenbroek@...il.com>, rick.wertenbroek@...g-vd.ch,
 alberto.dassatti@...g-vd.ch, Krzysztof Wilczyński
 <kw@...ux.com>, Kishon Vijay Abraham I <kishon@...nel.org>,
 Bjorn Helgaas <bhelgaas@...gle.com>, Frank Li <Frank.Li@....com>,
 Lars-Peter Clausen <lars@...afoo.de>, linux-pci@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] PCI: endpoint: Introduce 'get_bar' to map fixed
 address BARs in EPC

On 7/25/24 16:40, Manivannan Sadhasivam wrote:
>> Using DT for endpoint functions is nonsense in my opinion: if something needs to
>> be configured, an epf has the configfs interface to get the information it may
>> need. And forcing EPF to have DT node is not scalable anyway.
>>
> 
> Why? We are not using to DT to configure anything. If you try to use DT for that
> purpose then it would be wrong. DT is meant to provide hardware description. In
> this case, the EPF DT node can be used to describe the BAR address that is
> allocated (fixed) in the hardware. I did propose this in the previous Plumbers
> conf [1]. As I said earlier, this information is coming from the EPC node right
> now for MHI which has a hardware entity, but moving it to the dedicated EPF node
> would be the correct approach for scalability.

That does not make any sense. If we have a controller that has fixed addresses
for bars, then that is a hw property and that can go into the epc (PCI
controller) node. EPF is a software driver which has absolutely no business
having DT properties. configfs is made for that. And for special EPF drivers
that work only with particular controllers, e.g. your MHI EPF driver, the epf
can access properties of the epc if it needs information about the hardware
(e.g. a fixed bar address). The epf does not need to have its own node at ll in
the DT.

As a parallel, think of a DT describing a storage controller. The DT does not
describe the storage itself and much less the file system on that storage,
simply because everything can be probed at runtime from the controller DT and
the controller driver. Same thing here. EPF drivers go on top of EPC drivers and
their DT node describing the HW.

> 
> And ofc, that DT node *cannot* be used for anything else (like describing
> VID:PID in configfs etc...). As a nice side effect of the EPF node, we can get
> rid of configfs to create the link between EPC and EPF and start the controller.
> Again, this won't be applicable to non-hw backed EPF.
> 
>> So assuming you meant EPC, I am not sure if defining a fixed bar address in the
>> DT works for all cases. E.g. said address may depend on other hardware on the
>> platform (ex: memory nodes). So the ->get_bar() EPC operation that Rick is
>> proposing makes a lot more sense to me and will can be scaled to many many
>> configurations. Given that the EPF will (indirectly) call that operation, some
>> epf dependent parameters could even be passed.
>>
>> And I agree (I think we all do) that combing alloc bar and get bar under a
>> single API is a lot cleaner. Though I am not a big fan of the name
>> pci_epc_alloc_set_bar() as it implies an allocation, which may not be the case
>> for fixed bars. So a simpler and more generic name like pci_epf_set_bar(),
>> pci_epf_init_bar(), pci_epf_config_bar()... would be way better in my opinion.
>>
> 
> Well, the EPF driver doesn't need to know if the underlying address if fixed or
> dynamic IMO. It should just request BAR memory and the EPC core/controller
> drivers should take care of that.

Yes, I agree, and that is why I said that the name pci_epc_alloc_set_bar() is
not great because it implies allocation of memory, which may not always be
needed depending on the controller and may be on the function driver (e.g. fixed
bars using special memory).


-- 
Damien Le Moal
Western Digital Research

Powered by blists - more mailing lists