Hello Bjorn,
On Wed, Dec 04, 2024 at 11:33:52AM -0600, Bjorn Helgaas wrote:
In subject, maybe "Write BAR_MASK before iATU registers"
Ok. Will fix in v6.
I guess writing BAR_MASK is really configuring the *size* of the BAR?
I am quite sure that you know how host software determines the size of a BAR :)
But yes, writing the BAR_MASK will directly decide a BARs size: https://wiki.osdev.org/PCI#Address_and_size_of_the_BAR
So BAR_MASK is "BAR size - 1".
Maybe the size is the important semantic connection with iATU config?
The connection is: "Field size depends on log2(BAR_MASK+1) in BAR match mode."
So I think it makes sense for the subject to include BAR_MASK rather than BAR size.
On Wed, Nov 27, 2024 at 11:30:17AM +0100, Niklas Cassel wrote:
The DWC Databook description for the LWR_TARGET_RW and LWR_TARGET_HW fields in the IATU_LWR_TARGET_ADDR_OFF_INBOUND_i registers state that: "Field size depends on log2(BAR_MASK+1) in BAR match mode."
Can we include a databook revision and section here to help future maintainers?
Ok. Will fix in v6.
I.e. only the upper bits are writable, and the number of writable bits is dependent on the configured BAR_MASK.
If we do not write the BAR_MASK before writing the iATU registers, we are relying the reset value of the BAR_MASK being larger than the requested size of the first set_bar() call. The reset value of the BAR_MASK is SoC dependent.
Thus, if the first set_bar() call requests a size that is larger than the reset value of the BAR_MASK, the iATU will try to write to read-only bits, which will cause the iATU to end up redirecting to a physical address that is different from the address that was intended.
Thus, we should always write the iATU registers after writing the BAR_MASK.
Apparently we write BAR_MASK and the iATU registers in the wrong order? I assume dw_pcie_ep_inbound_atu() writes the iATU registers.
Yes.
I can't quite connect the commit log with the code change. I assume the dw_pcie_ep_writel_dbi2() and dw_pcie_ep_writel_dbi() writes update BAR_MASK?
dw_pcie_ep_writel_dbi2() writes the BAR_MASK. dw_pcie_ep_writel_dbi() writes the BAR type.
And I guess the problem is that the previous code does:
dw_pcie_ep_inbound_atu # iATU dw_pcie_ep_writel_dbi2 # BAR_MASK (?) dw_pcie_ep_writel_dbi
and the new code basically does this:
if (ep->epf_bar[bar]) { dw_pcie_ep_writel_dbi2 # BAR_MASK (?) dw_pcie_ep_writel_dbi } dw_pcie_ep_inbound_atu # iATU ep->epf_bar[bar] = epf_bar
so the first time we call dw_pcie_ep_set_bar(), we write BAR_MASK before iATU, and if we call dw_pcie_ep_set_bar() again, we skip the BAR_MASK update?
The problem is as described in the commit message: "If we do not write the BAR_MASK before writing the iATU registers, we are relying the reset value of the BAR_MASK being larger than the requested size of the first set_bar() call. The reset value of the BAR_MASK is SoC dependent."
Before: dw_pcie_ep_inbound_atu() # iATU - the writable bits in this write depends on # BAR_MASK, which has not been written yet, thus the # write will be at the mercy of the reset value of # BAR_MASK, which is SoC dependent. dw_pcie_ep_writel_dbi2() # BAR_MASK dw_pcie_ep_writel_dbi() # BAR type
After: dw_pcie_ep_writel_dbi2() # BAR_MASK dw_pcie_ep_writel_dbi() # BAR type dw_pcie_ep_inbound_atu() # iATU - this write is now done after BAR_MASK has # been written, so we know that all the bits in this # write is writable.
Kind regards, Niklas