Message-ID: <wvb42kyfcpyii3jql2gm75dd6hqpcd32yat2yb7cg7sl3raw4l@d4mfxk47l6md>
Date: Sat, 10 Jan 2026 23:29:06 +0900
From: Koichiro Den <den@...inux.co.jp>
To: Niklas Cassel <cassel@...nel.org>
Cc: jingoohan1@...il.com, mani@...nel.org, lpieralisi@...nel.org, 
	kwilczynski@...nel.org, robh@...nel.org, bhelgaas@...gle.com, vigneshr@...com, 
	s-vadapalli@...com, hongxing.zhu@....com, l.stach@...gutronix.de, 
	shawnguo@...nel.org, s.hauer@...gutronix.de, kernel@...gutronix.de, 
	festevam@...il.com, minghuan.Lian@....com, mingkai.hu@....com, roy.zang@....com, 
	jesper.nilsson@...s.com, heiko@...ech.de, srikanth.thokala@...el.com, 
	marek.vasut+renesas@...il.com, yoshihiro.shimoda.uh@...esas.com, geert+renesas@...der.be, 
	magnus.damm@...il.com, christian.bruel@...s.st.com, mcoquelin.stm32@...il.com, 
	alexandre.torgue@...s.st.com, thierry.reding@...il.com, jonathanh@...dia.com, 
	hayashi.kunihiko@...ionext.com, mhiramat@...nel.org, kishon@...nel.org, jirislaby@...nel.org, 
	rongqianfeng@...o.com, 18255117159@....com, shawn.lin@...k-chips.com, 
	nicolas.frattaroli@...labora.com, linux.amoon@...il.com, vidyas@...dia.com, Frank.Li@....com, 
	linux-omap@...r.kernel.org, linux-pci@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, imx@...ts.linux.dev, linuxppc-dev@...ts.ozlabs.org, 
	linux-arm-kernel@...s.com, linux-rockchip@...ts.infradead.org, 
	linux-arm-msm@...r.kernel.org, linux-renesas-soc@...r.kernel.org, 
	linux-stm32@...md-mailman.stormreply.com, linux-tegra@...r.kernel.org
Subject: Re: [PATCH v5 3/3] PCI: dwc: ep: Support BAR subrange inbound
 mapping via Address Match Mode iATU

On Fri, Jan 09, 2026 at 09:30:04AM +0100, Niklas Cassel wrote:
> On Fri, Jan 09, 2026 at 04:29:14PM +0900, Koichiro Den wrote:
> > > Does that mean that the usage of this API will be something like:
> > > 
> > > 1) set_bar() ## using BAR match mode, since BAR match mode can write
> > >    the BAR mask to define a BAR size, so that the host can assign a
> > >    PCI address to the BAR.
> > 
> > BAR sizing is done by dw_pcie_ep_set_bar_{programmable,resizable}() before
> > iATU programming regardless of match mode. I keep BAR match mode here just
> > because Address match mode requires a valid base address. That's why I
> > added the `if (!base)` guard.
> > 
> > > 
> > > 2) start() ## start link
> > > 
> > > 3) link up
> > > 
> > > 4) wait for some special command, perhaps NTB_EPF_COMMAND
> > > CMD_CONFIGURE_DOORBELL or NTB_EPF_COMMAND CMD_CONFIGURE_MW
> > > 
> > > 5) set_bar() ## using Address match mode. Because address match mode
> > >    requires that the host has assigned a PCI address to the BAR, we
> > >    can only change the mapping for a BAR after the host has assigned
> > >    PCI addresses for all bars.
> > > 
> > 
> > The overall usage flow matches what I'm assuming.
> 
> Ok, perhaps document something that the submap feature requires that the
> BAR already has been assigned an address (and that it thus has to call
> set_bar() twice, without calling clear_bar() in-between.
> 
> This is a new feature, and since you provide a mapping for the whole BAR,
> I think it is logical for people to incorrectly assume that you could use
> this feature by just calling set_bar() once.
> 
> 
> > > Perhaps you should add some text to:
> > > Documentation/PCI/endpoint/pci-endpoint.rst
> > > 
> > > Because right now the documentation for pci_epc_set_bar() says:
> > > 
> > >    The PCI endpoint function driver should use pci_epc_set_bar() to configure
> > >    the Base Address Register in order for the host to assign PCI addr space.
> > >    Register space of the function driver is usually configured
> > >    using this API.
> > > 
> > > So it is obviously meant to be called *before* the host assigns a PCI
> > > address for the BAR. Now with submap ranges, it appears that it has to
> > > be called *after* the host assigned a PCI address for the BAR.
> > 
> > I agree. As I understand it, the current text seems not to reflect mainline
> > since commit 4284c88fff0e ("PCI: designware-ep: Allow pci_epc_set_bar()
> > update inbound map address"), but anyway I should add explicit
> > documentation for this subrange mapping use case.
> 
> Sure, 4284c88fff0e ("PCI: designware-ep: Allow pci_epc_set_bar() update
> inbound map address") modified so that set_bar() can be called twice,
> without calling clear_bar().
> 
> However, that patch was a mess because:
> 1) It got merged via the NTB tree, even though the PCI maintainer wanted a
>    different design:
>    https://lore.kernel.org/linux-pci/20220818060230.GA12008@thinkpad/T/#m411b3e9f6625da9dde747f624ed6870bef3e8823
> 2) It was buggy:
>    https://github.com/torvalds/linux/commit/c2a57ee0f2f1ad8c970ff58b78a43e85abbdeb7f
> 3) It lacked the proper conditional checks for the feature to work (and it
>    lacked any comments in the source explaining why it was skipping steps):
>    https://github.com/torvalds/linux/commit/3708acbd5f169ebafe1faa519cb28adc56295546
> 4) It failed to update the documentation.
> 5) It failed to add a new flag for this feature in epc_features.
>    I seriously doubt that any non-DWC based EP controller supports changing
>    the inbound address translations without first disabling the BAR.
>    (It probably should have added a epc_features->dynamic_inbound_mapping bit.)
> 

Thanks for pointing me to the context, that really helps.

> 
> So all in all 4284c88fff0e in not the best example :)
> 
> 
> Your new feature (epc_features->subrange_mapping) in epc_features appears
> to depend on epc_features->dynamic_inbound_mapping, so it is a shame that
> we don't have a epc_features->dynamic_inbound_mapping bit, so that this new
> feature could have depended on that bit.
> 
> 	if (epf_bar->use_submap &&
> 	    !(epc_features->dynamic_inbound_mapping &&
> 	      epc_features->subrange_mapping))
> 		return -EINVAL;
> 
> 
> I think adding some documentation is a good step.
> 
> Perhaps we should also introduce a epc_features->dynamic_inbound_mapping bit?
> Since you are making DWC glue drivers return a mutable EPC features, we could
> set this bit in the DWC driver after that commit. What do you think?

As you pointed out, support for dynamic_inbound_mapping is needed
independently of my series. Given that, it would make sense to handle it
either before this series, or to fold it into the next iteration (=v6) of
the series if that is preferred.

If you already have a patch for dynamic_inbound_mapping in mind, I'm happy
to wait for it and help test it.

> 
> I realize that we would not be able to add any actual verification for the
> epc_features->dynamic_inbound_mapping feature itself (in set_bar()), since
> there is no way for set_bar() to know if a BAR has already been configured
> (since struct pci_epc does not have an array of the currently configured BARs).
> 
> But at least it would allow an EPF driver (e.g. vNTB) to know if it can call
> set_bar() twice or not, and can error out if the EPF driver is being used on
> a EPC that doesn't support epc_features->dynamic_inbound_mapping.

That makes sense.

Thanks again,
Koichiro

> 
> 
> Kind regards,
> Niklas

Powered by blists - more mailing lists