Re: [PATCH V2] PCI: dwc ep: cache config until DBI regs available

[Stephen Warren <swarren@wwwdotorg.org>]

On 12/10/18 9:36 PM, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On 27/11/18 4:39 AM, Stephen Warren wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> Some implementations of the DWC PCIe endpoint controller do not allow
>> access to DBI registers until the attached host has started REFCLK,
>> released PERST, and the endpoint driver has initialized clocking of the
>> DBI registers based on that. One such system is NVIDIA's T194 SoC. The
>> PCIe endpoint subsystem and DWC driver currently don't work on such
>> hardware, since they assume that all endpoint configuration can happen
>> at any arbitrary time.
>>
>> Enhance the DWC endpoint driver to support such systems by caching all
>> endpoint configuration in software, and only writing the configuration
>> to hardware once it's been initialized. This is implemented by splitting
>> all endpoint controller ops into two functions; the first which simply
>> records/caches the desired configuration whenever called by the
>> associated function driver and optionally calls the second, and the
>> second which actually programs the configuration into hardware, which
>> may be called either by the first function, or later when it's known
>> that the DBI registers are available.

>> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c

>> +void dw_pcie_set_regs_available(struct dw_pcie *pci)
>> +{
> 
> When will this function be invoked? Does the wrapper get an interrupt when
> refclk is enabled where this function will be invoked?

Yes, there's an IRQ from the HW that indicates when PEXRST is released. 
I don't recall right now if this IRQ is something that exists for all 
DWC instantiations, or is Tegra-specific.

>> +	struct dw_pcie_ep *ep = &(pci->ep);
>> +	int i;
>> +
>> +	ep->hw_regs_not_available = false;
> 
> This can race with epc_ops.
> 
>> +
>> +	dw_pcie_ep_write_header_regs(ep);
>> +	for_each_set_bit(i, ep->ib_window_map, ep->num_ib_windows) {
>> +		dw_pcie_prog_inbound_atu(pci, i,
>> +			ep->cached_inbound_atus[i].bar,
>> +			ep->cached_inbound_atus[i].cpu_addr,
>> +			ep->cached_inbound_atus[i].as_type);
> 
> Depending on the context in which this function is invoked, programming
> inbound/outbound ATU can also race with EPC ops.
 >
>> +		dw_pcie_ep_set_bar_regs(ep, 0, ep->cached_inbound_atus[i].bar);
>> +	}
>> +	for_each_set_bit(i, ep->ob_window_map, ep->num_ob_windows)
>> +		dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_MEM,
>> +			ep->cached_outbound_atus[i].addr,
>> +			ep->cached_outbound_atus[i].pci_addr,
>> +			ep->cached_outbound_atus[i].size);
>> +	dw_pcie_dbi_ro_wr_en(pci);
>> +	dw_pcie_writew_dbi(pci, PCI_MSI_FLAGS, ep->cached_msi_flags);
>> +	dw_pcie_writew_dbi(pci, PCI_MSIX_FLAGS, ep->cached_msix_flags);
>> +	dw_pcie_dbi_ro_wr_dis(pci);
> 
> IMHO we should add a new epc ops ->epc_init() which indicates if the EPC is
> ready to be initialized or not. Only if the epc_init indicates it's ready to be
> initialized, the endpoint function driver should go ahead with further
> initialization. Or else it should wait for a notification from EPC to indicate
> when it's ready to be initialized.

(Did you mean epf op or epc op?)

I'm not sure how exactly how that would work; do you want the DWC core 
driver or the endpoint subsystem to poll that epc op to find out when 
the HW is ready to be initialized? Or do you envisage the controller 
driver still calling dw_pcie_set_regs_available() (possibly renamed), 
which in turn calls ->epc_init() calls for some reason?

If you don't want to cache the endpoint configuration, perhaps you want:

a) Endpoint function doesn't pro-actively call the endpoint controller 
functions to configure the endpoint.

b) When endpoint HW is ready, the relevant driver calls pci_epc_ready() 
(or whatever name), which lets the core know the HW can be configured. 
Perhaps this schedules a work queue item to implement locking to avoid 
the races you mentioned.

c) Endpoint core calls pci_epf_init() which calls epf op ->init().

One gotcha with this approach, which the caching approach helps avoid:

Once PEXRST is released, the system must respond to PCIe enumeration 
requests within 50ms. Thus, SW must very quickly respond to the IRQ 
indicating PEXRST release and program the endpoint configuration into 
HW. By caching the configuration in the DWC driver and 
immediately/synchronously applying it in the PEXRST IRQ handler, we 
reduce the number of steps and amount of code taken to program the HW, 
so it should get done pretty quickly. If instead we call back into the 
endpoint function driver's ->init() op, we run the risk of that op doing 
other stuff besides just calling the endpoint HW configuration APIs 
(e.g. perhaps the function driver defers memory buffer allocation or 
IOVA programming to that ->init function) which in turns makes it much 
less likely the 50ms requirement will be hit. Perhaps we can solve this 
by naming the op well and providing lots of comments, but my guess is 
that endpoint function authors won't notice that...