Message-ID: <aG/lG2r4U4rPrEeB@lizhi-Precision-Tower-5810>
Date: Thu, 10 Jul 2025 12:06:51 -0400
From: Frank Li <Frank.li@....com>
To: Niklas Cassel <cassel@...nel.org>
Cc: Kishon Vijay Abraham I <kishon@...nel.org>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Anup Patel <apatel@...tanamicro.com>, Marc Zyngier <maz@...nel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Danilo Krummrich <dakr@...nel.org>,
	Bjorn Helgaas <bhelgaas@...gle.com>, Arnd Bergmann <arnd@...db.de>,
	Shuah Khan <shuah@...nel.org>, Richard Zhu <hongxing.zhu@....com>,
	Lucas Stach <l.stach@...gutronix.de>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Rob Herring <robh@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Manivannan Sadhasivam <mani@...nel.org>,
	Krzysztof Wilczyński <kwilczynski@...nel.org>,
	dlemoal@...nel.org, jdmason@...zu.us, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-pci@...r.kernel.org,
	linux-kselftest@...r.kernel.org, imx@...ts.linux.dev,
	devicetree@...r.kernel.org
Subject: Re: [PATCH v20 0/9] PCI: EP: Add RC-to-EP doorbell with platform MSI
 controller

On Thu, Jul 10, 2025 at 01:40:25PM +0200, Niklas Cassel wrote:
> Hello Frank,
>
> I tested v20 of your series, but unfortunately, it still doesn't work.
>
> When enabling the doorbell, the programming of the inbound iATU fails:
>
> ## pci_epf_test_enable_doorbell()
> ## keeps the BAR size, and BAR type of a BAR that has already been configured,
> ## but changes the address translation for this BAR to redirect to the GIC ITS
> ## rather than to the memory allocated by pci_epf_alloc_space()
> ## (does not free the memory allocated by pci_epf_alloc_space())
>
> [   39.347502] pci_epf_test_enable_doorbell: msg hi: 0x0 msg low: 0xfe670040
> [   39.348103] pci_epf_test_enable_doorbell: base: 0xfe670000 off: 0x40
> [   39.348658] dw_pcie_ep_inbound_atu index: 1 parent_bus_addr: 0xfe670000 bar: 1 size: 0x100000
> [   39.349403] dw_pcie_prog_ep_inbound_atu parent_bus_addr: 0xfe670000 pci->region_align: 0x10000 IS_ALIGNED: 1
> [   39.350260] dw_pcie_prog_ep_inbound_atu parent_bus_addr: 0xfe670000 size: 0x100000 IS_ALIGNED: 0
> [   39.351028] rockchip-dw-pcie a40000000.pcie-ep: Failed to program IB window
>
> ## pci_epf_test_disable_doorbell()
> ## changes the address translation for this BAR to redirect to the memory
> ## allocated by pci_epf_alloc_space() (which was never freed when enabling the
> ## doorbell)
>
> [   39.351656] dw_pcie_ep_inbound_atu index: 1 parent_bus_addr: 0xa2e00000 bar: 1 size: 0x100000
> [   39.352401] dw_pcie_prog_ep_inbound_atu parent_bus_addr: 0xa2e00000 pci->region_align: 0x10000 IS_ALIGNED: 1
> [   39.353257] dw_pcie_prog_ep_inbound_atu parent_bus_addr: 0xa2e00000 size: 0x100000 IS_ALIGNED: 1
>
>
> The reason why pci_epf_test_enable_doorbell() fails is because of this check:
> https://github.com/torvalds/linux/blob/v6.16-rc5/drivers/pci/controller/dwc/pcie-designware.c#L663
>
> If you want to understand why this very important check is there, it is
> because the DWC controller requires that the physical address programmed in
> the iATU is aligned to the size of the BAR (BAR_MASK+1), see this commit:
> https://github.com/torvalds/linux/commit/129f6af747b2
>
>
> Applying the following patch on top of your v20 series makes things work as
> intended and makes the pcie_ep_doorbell.DOORBELL_TEST selftest pass for me:
>
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index dfdd25cfc003..7d356b0201ae 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -738,9 +738,9 @@ static void pci_epf_test_enable_doorbell(struct pci_epf_test *epf_test,
>  	reg->doorbell_bar = cpu_to_le32(bar);
>
>  	msg = &epf->db_msg[0].msg;
> -	ret = pci_epf_align_inbound_addr(epf, bar, ((u64)msg->address_hi << 32) | msg->address_lo,
> +	ret = pci_epf_align_inbound_addr(epf, epf->bar[bar].size,
> +					((u64)msg->address_hi << 32) | msg->address_lo,
>  					&epf_test->db_bar.phys_addr, &offset);
> -
>  	if (ret)
>  		goto err_doorbell;
>
> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> index c21d8e786eb3..b3d4117182e2 100644
> --- a/drivers/pci/endpoint/pci-epf-core.c
> +++ b/drivers/pci/endpoint/pci-epf-core.c
> @@ -478,44 +478,36 @@ struct pci_epf *pci_epf_create(const char *name)
>  EXPORT_SYMBOL_GPL(pci_epf_create);
>
>  /**
> - * pci_epf_align_inbound_addr() - Align the given address based on the BAR
> - *				 alignment requirement
> + * pci_epf_align_inbound_addr() - Align the given address based on the BAR size
> + *
>   * @epf: the EPF device
> + * @bar_size: the current BAR size
>   * @addr: inbound address to be aligned
> - * @bar: the BAR number corresponding to the given addr
> - * @base: base address matching the @bar alignment requirement.
> + * @base: base address matching the @bar_size alignment requirement.
>   * @off: offset to be added to the @base address.
>   *
> - * Helper function to align input 'addr' to base and offset, which match
> - * BAR's alignment requirement.
> + * Helper function to align input 'addr' to base and offset, when dynamically
> + * changing a BAR.
>   *
>   * The pci_epf_alloc_space() function already accounts for alignment. This is
>   * primarily intended for use with other memory regions not allocated by
>   * pci_epf_alloc_space(), such as peripheral register spaces or the trigger
>   * address for a platform MSI controller.
> + *
> + * Since this function is only used when dynamically changing a BAR (i.e. when
> + * calling set_bar() twice, without ever calling clear_bar(), as calling
> + * clear_bar() would clear the BAR's PCI address assigned by the host), this
> + * function must align to the current BAR size, since we are not clearing the
> + * BAR configuration.
>   */
> -int pci_epf_align_inbound_addr(struct pci_epf *epf, enum pci_barno bar,
> -			      u64 addr, dma_addr_t *base, size_t *off)
> +int pci_epf_align_inbound_addr(struct pci_epf *epf, size_t bar_size, u64 addr,
> +			      dma_addr_t *base, size_t *off)
>  {
> -	const struct pci_epc_features *epc_features;
> -	u64 align;
> -
> -	if (!base || !off)
> +	if (!base || !off || !bar_size)
>  		return -EINVAL;
>
> -	epc_features = pci_epc_get_features(epf->epc, epf->func_no, epf->vfunc_no);
> -	if (!epc_features) {
> -		dev_err(&epf->dev, "epc_features not implemented\n");
> -		return -EOPNOTSUPP;
> -	}
> -
> -	align = epc_features->align;
> -	align = align ? align : 128;
> -	if (epc_features->bar[bar].type == BAR_FIXED)
> -		align = max(epc_features->bar[bar].fixed_size, align);
> -
> -	*base = round_down(addr, align);
> -	*off = addr & (align - 1);
> +	*base = round_down(addr, bar_size);
> +	*off = addr & (bar_size - 1);
>
>  	return 0;
>  }
> diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> index 0ca08f0d05d7..bcc8184325d2 100644
> --- a/include/linux/pci-epf.h
> +++ b/include/linux/pci-epf.h
> @@ -242,8 +242,8 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
>  void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar,
>  			enum pci_epc_interface_type type);
>
> -int pci_epf_align_inbound_addr(struct pci_epf *epf, enum pci_barno bar,
> -			      u64 addr, dma_addr_t *base, size_t *off);
> +int pci_epf_align_inbound_addr(struct pci_epf *epf, size_t bar_size, u64 addr,
> +			      dma_addr_t *base, size_t *off);
>  int pci_epf_bind(struct pci_epf *epf);
>  void pci_epf_unbind(struct pci_epf *epf);
>  int pci_epf_add_vepf(struct pci_epf *epf_pf, struct pci_epf *epf_vf);
>
>
>
>
> However, the more I think about it, considering that this alignment requirement
> is inherent to the DWC controller (other controllers might not have this
> requirement), perhaps pci_epf_align_inbound_addr() should not be a function in
> pci-epf-core.c, perhaps this function would be better suited to live in
> drivers/pci/controller/dwc/pcie-designware-ep.c ?

I think align to bar size is quite make sense now. Even other controllers
have not such requirement, align to bar size still work.

Anyways, it need be align. We still pass down bar number, get bar size
in pci_epf_align_inbound_addr(). So we can fine tune algorithm in future,
such as add field in epc_features.

Frank
>
>
> Kind regards,
> Niklas

Powered by blists - more mailing lists